[gentoo-portage-dev] [PATCH] egencache --update-changelogs: fix timestamp assumptions (bug 579292)

[gentoo-portage-dev] [PATCH] egencache --update-changelogs: fix timestamp assumptions (bug 579292)

[Zac Medico]

Since commit times are not necessarily ordered, synchronize the
ChangeLog mtime with the last commit time, and use exact comparison
to detect changes.

X-Gentoo-bug: 579292
X-Gentoo-bug-url: https://bugs.gentoo.org/show_bug.cgi?id=579292
---
 bin/egencache | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/bin/egencache b/bin/egencache
index 0123d57..41612e5 100755
--- a/bin/egencache
+++ b/bin/egencache
@@ -775,12 +775,16 @@ class GenChangeLogs(object):
 			# This cp has not been added to the repo.
 			return
 
+		lmod = long(lmod)
+
 		try:
-			cmod = os.stat('ChangeLog').st_mtime
+			cmod = os.stat('ChangeLog')[stat.ST_MTIME]
 		except OSError:
 			cmod = 0
 
-		if float(cmod) >= float(lmod):
+		# Use exact comparison, since commit times are
+		# not necessarily ordered.
+		if cmod == lmod:
 			return
 
 		try:
@@ -903,6 +907,7 @@ class GenChangeLogs(object):
 				'\n%s\n\n' % '\n'.join(self._wrapper.fill(x) for x in body))
 
 		output.close()
+		os.utime(self._changelog_output, (lmod, lmod))
 
 	def _task_iter(self):
 		if not os.path.isdir(os.environ.get('GIT_DIR', os.path.join(self._repo_path, '.git'))):
-- 
2.7.4

Re: [gentoo-portage-dev] [PATCH] egencache --update-changelogs: fix timestamp assumptions (bug 579292)

[Brian Dolbec]

On Thu,  7 Apr 2016 22:46:25 -0700
Zac Medico <zmedico@gentoo.org> wrote:

> Since commit times are not necessarily ordered, synchronize the
> ChangeLog mtime with the last commit time, and use exact comparison
> to detect changes.
> 
> X-Gentoo-bug: 579292
> X-Gentoo-bug-url: https://bugs.gentoo.org/show_bug.cgi?id=579292
> ---
>  bin/egencache | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/bin/egencache b/bin/egencache
> index 0123d57..41612e5 100755
> --- a/bin/egencache
> +++ b/bin/egencache
> @@ -775,12 +775,16 @@ class GenChangeLogs(object):
>  			# This cp has not been added to the repo.
>  			return
>  
> +		lmod = long(lmod)
> +
>  		try:
> -			cmod = os.stat('ChangeLog').st_mtime
> +			cmod = os.stat('ChangeLog')[stat.ST_MTIME]
>  		except OSError:
>  			cmod = 0
>  
> -		if float(cmod) >= float(lmod):
> +		# Use exact comparison, since commit times are
> +		# not necessarily ordered.
> +		if cmod == lmod:
>  			return
>  
>  		try:
> @@ -903,6 +907,7 @@ class GenChangeLogs(object):
>  				'\n%s\n\n' %
> '\n'.join(self._wrapper.fill(x) for x in body)) 
>  		output.close()
> +		os.utime(self._changelog_output, (lmod, lmod))
>  
>  	def _task_iter(self):
>  		if not os.path.isdir(os.environ.get('GIT_DIR',
> os.path.join(self._repo_path, '.git'))):

the above looks good, but what about:


[19:01] <dwfreed|phone> just use --first-parent
[19:01] <dwfreed|phone> also take into account merge commits
[19:03] <dwfreed|phone> git really complicates ChangeLog generation in general
[19:03] <dwfreed|phone> because your ChangeLog should reflect when these commits became part of master, but you still need to perserve their messages
[19:04] * zmedico is skeptical about the linearizability of the timestamps
[19:04] <dwfreed|phone> if you don't look at merge commits for your timestamps of changes, correct, it is not linear
[19:05] <dwfreed|phone> but if you take a set of commits and determine when they became part of master, it is linear
[19:05] <dol-sen> so: git log --first-parent --format=%ct -1 .
[19:05] <dol-sen> to get the last timestamp of changes to htat pkg
[19:06] <dol-sen> then use that timestamp
[19:06] <dol-sen> lmod = self.grab(['git', self._work_tree, 'log', '--format=%ct', '-1', '.'])
[19:06] <dol-sen> that is the current code
[19:06] <dwfreed|phone> git log -m --first-parent --format=%ct -1 .
[19:06] <dol-sen> so just add the --first-parent option?
[19:07] <dwfreed|phone> you want -m toolmod = self.grab(['git',
self._work_tree, 'log', '--format=%ct', '-1', '.'])

[19:06] <dwfreed|phone> git log -m --first-parent --format=%ct -1 .
[19:06] <dol-sen> so just add the --first-parent option?
[19:07] <dwfreed|phone> you want -m too


Don't we need to add the -m --first-parent  ???
-- 
Brian Dolbec <dolsen>

Re: [gentoo-portage-dev] [PATCH] egencache --update-changelogs: fix timestamp assumptions (bug 579292)

[Zac Medico]

On 04/07/2016 11:51 PM, Brian Dolbec wrote:
> the above looks good, but what about:
> 
> 
> [19:01] <dwfreed|phone> just use --first-parent
> [19:01] <dwfreed|phone> also take into account merge commits
> [19:03] <dwfreed|phone> git really complicates ChangeLog generation in general
> [19:03] <dwfreed|phone> because your ChangeLog should reflect when these commits became part of master, but you still need to perserve their messages
> [19:04] * zmedico is skeptical about the linearizability of the timestamps
> [19:04] <dwfreed|phone> if you don't look at merge commits for your timestamps of changes, correct, it is not linear
> [19:05] <dwfreed|phone> but if you take a set of commits and determine when they became part of master, it is linear
> [19:05] <dol-sen> so: git log --first-parent --format=%ct -1 .
> [19:05] <dol-sen> to get the last timestamp of changes to htat pkg
> [19:06] <dol-sen> then use that timestamp
> [19:06] <dol-sen> lmod = self.grab(['git', self._work_tree, 'log', '--format=%ct', '-1', '.'])
> [19:06] <dol-sen> that is the current code
> [19:06] <dwfreed|phone> git log -m --first-parent --format=%ct -1 .
> [19:06] <dol-sen> so just add the --first-parent option?
> [19:07] <dwfreed|phone> you want -m toolmod = self.grab(['git',
> self._work_tree, 'log', '--format=%ct', '-1', '.'])
> 
> [19:06] <dwfreed|phone> git log -m --first-parent --format=%ct -1 .
> [19:06] <dol-sen> so just add the --first-parent option?
> [19:07] <dwfreed|phone> you want -m too
> 
> 
> Don't we need to add the -m --first-parent  ???
> 

I don't know enough about how those options matter for git log behavior,
but I trust that dwfreed has good reasons to recommend them. Maybe we
should add them in a separate patch, possibly with some explanation
about how they are useful in this context.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] egencache --update-changelogs: fix timestamp assumptions (bug 579292)

[Brian Dolbec]

On Fri, 8 Apr 2016 00:29:40 -0700
Zac Medico <zmedico@gentoo.org> wrote:

> On 04/07/2016 11:51 PM, Brian Dolbec wrote:
> > the above looks good, but what about:
> > 
> > 
> > [19:01] <dwfreed|phone> just use --first-parent
> > [19:01] <dwfreed|phone> also take into account merge commits
> > [19:03] <dwfreed|phone> git really complicates ChangeLog generation
> > in general [19:03] <dwfreed|phone> because your ChangeLog should
> > reflect when these commits became part of master, but you still
> > need to perserve their messages [19:04] * zmedico is skeptical
> > about the linearizability of the timestamps [19:04] <dwfreed|phone>
> > if you don't look at merge commits for your timestamps of changes,
> > correct, it is not linear [19:05] <dwfreed|phone> but if you take a
> > set of commits and determine when they became part of master, it is
> > linear [19:05] <dol-sen> so: git log --first-parent --format=%ct
> > -1 . [19:05] <dol-sen> to get the last timestamp of changes to htat
> > pkg [19:06] <dol-sen> then use that timestamp [19:06] <dol-sen>
> > lmod = self.grab(['git', self._work_tree, 'log', '--format=%ct',
> > '-1', '.']) [19:06] <dol-sen> that is the current code [19:06]
> > <dwfreed|phone> git log -m --first-parent --format=%ct -1 . [19:06]
> > <dol-sen> so just add the --first-parent option? [19:07]
> > <dwfreed|phone> you want -m toolmod = self.grab(['git',
> > self._work_tree, 'log', '--format=%ct', '-1', '.'])
> > 
> > [19:06] <dwfreed|phone> git log -m --first-parent --format=%ct -1 .
> > [19:06] <dol-sen> so just add the --first-parent option?
> > [19:07] <dwfreed|phone> you want -m too
> > 
> > 
> > Don't we need to add the -m --first-parent  ???
> >   
> 
> I don't know enough about how those options matter for git log
> behavior, but I trust that dwfreed has good reasons to recommend
> them. Maybe we should add them in a separate patch, possibly with
> some explanation about how they are useful in this context.

They really matter when the tree gets crappy/stale merge commits from
pull requests which play with the way the tree is considered.  In this
particular case, it will ignore the branch history and only consider the
merge commit that connects it to master.  Which is when we want to make
the changelog entry, not when the (possibly stale) user commit was made.

-- 
Brian Dolbec <dolsen>

Re: [gentoo-portage-dev] [PATCH] egencache --update-changelogs: fix timestamp assumptions (bug 579292)

[Zac Medico]

On 04/08/2016 06:12 PM, Brian Dolbec wrote:
> On Fri, 8 Apr 2016 00:29:40 -0700
> Zac Medico <zmedico@gentoo.org> wrote:
> 
>> On 04/07/2016 11:51 PM, Brian Dolbec wrote:
>>> the above looks good, but what about:
>>>
>>>
>>> [19:01] <dwfreed|phone> just use --first-parent
>>> [19:01] <dwfreed|phone> also take into account merge commits
>>> [19:03] <dwfreed|phone> git really complicates ChangeLog generation
>>> in general [19:03] <dwfreed|phone> because your ChangeLog should
>>> reflect when these commits became part of master, but you still
>>> need to perserve their messages [19:04] * zmedico is skeptical
>>> about the linearizability of the timestamps [19:04] <dwfreed|phone>
>>> if you don't look at merge commits for your timestamps of changes,
>>> correct, it is not linear [19:05] <dwfreed|phone> but if you take a
>>> set of commits and determine when they became part of master, it is
>>> linear [19:05] <dol-sen> so: git log --first-parent --format=%ct
>>> -1 . [19:05] <dol-sen> to get the last timestamp of changes to htat
>>> pkg [19:06] <dol-sen> then use that timestamp [19:06] <dol-sen>
>>> lmod = self.grab(['git', self._work_tree, 'log', '--format=%ct',
>>> '-1', '.']) [19:06] <dol-sen> that is the current code [19:06]
>>> <dwfreed|phone> git log -m --first-parent --format=%ct -1 . [19:06]
>>> <dol-sen> so just add the --first-parent option? [19:07]
>>> <dwfreed|phone> you want -m toolmod = self.grab(['git',
>>> self._work_tree, 'log', '--format=%ct', '-1', '.'])
>>>
>>> [19:06] <dwfreed|phone> git log -m --first-parent --format=%ct -1 .
>>> [19:06] <dol-sen> so just add the --first-parent option?
>>> [19:07] <dwfreed|phone> you want -m too
>>>
>>>
>>> Don't we need to add the -m --first-parent  ???
>>>   
>>
>> I don't know enough about how those options matter for git log
>> behavior, but I trust that dwfreed has good reasons to recommend
>> them. Maybe we should add them in a separate patch, possibly with
>> some explanation about how they are useful in this context.
> 
> They really matter when the tree gets crappy/stale merge commits from
> pull requests which play with the way the tree is considered.  In this
> particular case, it will ignore the branch history and only consider the
> merge commit that connects it to master.  Which is when we want to make
> the changelog entry, not when the (possibly stale) user commit was made.

Bug filed:

https://bugs.gentoo.org/show_bug.cgi?id=579402
-- 
Thanks,
Zac