Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 16/04/18 10:48, Phillip Wood wrote:
> On 14/04/18 14:11, Johannes Schindelin wrote:
>> Hi,
>>
>> On Sat, 14 Apr 2018, Phillip Wood wrote:
>>
>> FWIW I agree with Hannes' patch.
>>
>>> I think 'git am' probably gives all patches the same commit time as well
>>> if the commit date is cached though it wont suffer from the time-travel
>>> problem.
>>
>> I thought that `git am` was the subject of such a complaint recently, but
>> I thought that had been resolved? Apparently I misremember...
> 
> I had a quick look and couldn't see anything about that, it looks to me
> like it just calls commit_tree() and only does anything to change the
> default commit date if '--committer-date-is-author-date' was given.

Ah you were right, I just didn't look far enough back in the history
.(it's a shame it calls reset_ident_date() in a different function to
the one that creates the commit otherwise I would probably have noticed
it when I wrote the original patches)

Best Wishes

Phillip

> 
> Best Wishes
> 
> Phillip
>> Ciao,
>> Dscho
>>
> 

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 16/04/18 06:56, Johannes Sixt wrote:
> 
> Am 15.04.2018 um 23:35 schrieb Junio C Hamano:
>> Ah, do you mean we have an internal sequence like this, when "rebase
>> --continue" wants to conclude an edit/reword?
> 
> Yes, it's only 'reword' that is affected, because then subsequent picks
> are processed by the original process.
> 
>>   - we figure out the committer ident, which grabs a timestamp and
>>     cache it;
>>
>>   - we spawn "commit" to conclude the stopped step, letting it record
>>     its beginning time (which is a bit older than the above) or its
>>     ending time (which is much older due to human typing speed);
> 
> Younger in both cases, of course. According to my tests, we seem to pick
> the beginning time, because the first 'reword'ed commit typically has
> the same timestamp as the preceding picks. Later 'reword'ed commits have
> noticably younger timestamps.
> 
>>   - subsequent "picks" are made in the same process, and share the
>>     timestamp we grabbed in the first step, which is older than the
>>     second one.
>>
>> I guess we'd want a mechanism to tell ident.c layer "discard the
>> cached one, as we are no longer in the same automated sequence", and
>> use that whenever we spawn an editor (or otherwise go interactive).
> 
> Frankly, I think that this caching is overengineered (or prematurly
> optimized). If the design requires that different callers of datestamp()
> must see the same time, then the design is broken. In a fixed design,
> there would be a single call of datestamp() in advance, and then the
> timestamp, which then obviously is a very important piece of data, would
> be passed along as required.

I'm inclined to agree, though it creates complications if we're going to
keep giving commits the same author and committer dates when neither is
explicitly specified.

Best Wishes

Phillip

> 
> -- Hannes

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Junio C Hamano]

Johannes Sixt  writes:

> Am 15.04.2018 um 23:35 schrieb Junio C Hamano:
>> Ah, do you mean we have an internal sequence like this, when "rebase
>> --continue" wants to conclude an edit/reword?
>
> Yes, it's only 'reword' that is affected, because then subsequent
> picks are processed by the original process.

Ah, OK, that is a good explanation.

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 14/04/18 14:11, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 14 Apr 2018, Phillip Wood wrote:
> 
>> On 13/04/18 17:52, Johannes Sixt wrote:
>>>
>>> I just noticed that all commits in a 70-commit branch have the same
>>> committer timestamp. This is very unusual on Windows, where rebase -i of
>>> such a long branch takes more than one second (but not more than 3 or
>>> so thanks to the builtin nature of the command!).
>>>
>>> And, in fact, if you mark some commits with 'reword' to delay the quick
>>> processing of the patches, then the reworded commits have later time
>>> stamps, but subsequent not reworded commits receive the earlier time
>>> stamp. This is clearly not intended.
>>
>> Oh dear, I think this is probably due to my series making rebase commit
>> in-process when the commit message isn't being edited. I didn't realize
>> that git cached the commit date rather than using the current time when
>> calling commit_tree_extended(). I'll take a look at it next week.
> 
> Thanks.
> 
> However, a quick lock at `git log @{u}.. --format=%ct` in my
> `sequencer-shears` branch thicket (which I rebase frequently on top of
> upstream's `master` using the last known-good `rebase-merges` sub-branch)
> shows that the commits have different-enough commit timestamps. (It is
> satisfying to see that multiple commits were made during the same second,
> of course.)
> 
> So while I cannot find anything in the code that disagrees with Hannes'
> assessment, it looks on the surface as if I did not encounter the bug
> here.
> 
> Curious.

That's strange (I'd have expected the picks after recreated merges to
have the earlier timestamps than the merge), if I do 'git rebase -i
--force-rebase --exec="sleep 2" @~5' then all the new commits have the
same timestamp.

> FWIW I agree with Hannes' patch.
> 
>> I think 'git am' probably gives all patches the same commit time as well
>> if the commit date is cached though it wont suffer from the time-travel
>> problem.
> 
> I thought that `git am` was the subject of such a complaint recently, but
> I thought that had been resolved? Apparently I misremember...

I had a quick look and couldn't see anything about that, it looks to me
like it just calls commit_tree() and only does anything to change the
default commit date if '--committer-date-is-author-date' was given.

Best Wishes

Phillip
> Ciao,
> Dscho
> 

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Johannes Sixt]

Am 15.04.2018 um 23:35 schrieb Junio C Hamano:

Ah, do you mean we have an internal sequence like this, when "rebase
--continue" wants to conclude an edit/reword?


Yes, it's only 'reword' that is affected, because then subsequent picks 
are processed by the original process.



  - we figure out the committer ident, which grabs a timestamp and
cache it;

  - we spawn "commit" to conclude the stopped step, letting it record
its beginning time (which is a bit older than the above) or its
ending time (which is much older due to human typing speed);


Younger in both cases, of course. According to my tests, we seem to pick 
the beginning time, because the first 'reword'ed commit typically has 
the same timestamp as the preceding picks. Later 'reword'ed commits have 
noticably younger timestamps.



  - subsequent "picks" are made in the same process, and share the
timestamp we grabbed in the first step, which is older than the
second one.

I guess we'd want a mechanism to tell ident.c layer "discard the
cached one, as we are no longer in the same automated sequence", and
use that whenever we spawn an editor (or otherwise go interactive).


Frankly, I think that this caching is overengineered (or prematurly 
optimized). If the design requires that different callers of datestamp() 
must see the same time, then the design is broken. In a fixed design, 
there would be a single call of datestamp() in advance, and then the 
timestamp, which then obviously is a very important piece of data, would 
be passed along as required.


-- Hannes

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Junio C Hamano]

Johannes Sixt  writes:

> I just noticed that all commits in a 70-commit branch have the same
> committer timestamp. This is very unusual on Windows, where rebase -i of
> such a long branch takes more than one second (but not more than 3 or
> so thanks to the builtin nature of the command!).
>
> And, in fact, if you mark some commits with 'reword' to delay the quick
> processing of the patches, then the reworded commits have later time
> stamps, but subsequent not reworded commits receive the earlier time
> stamp. This is clearly not intended.

Hmm, I may be missing something without enough caffeine but I am
puzzled how that would be possible.  With a "few picks, an edit, and
a yet more picks" sequence, the first picks may share the same
timestamp due to the git_default_date caching (which I think is a
deliberate design choice we made), an edit that stops will let the
concluding "commit" (either by the end user or invoked internally
via "rebase --continue"), but because that process restarts afresh,
the commits made by "yet more picks" cannot share the timestamp that
was cached for the earliest ones from the same series, no?

Ah, do you mean we have an internal sequence like this, when "rebase
--continue" wants to conclude an edit/reword?

 - we figure out the committer ident, which grabs a timestamp and
   cache it;

 - we spawn "commit" to conclude the stopped step, letting it record
   its beginning time (which is a bit older than the above) or its
   ending time (which is much older due to human typing speed);

 - subsequent "picks" are made in the same process, and share the
   timestamp we grabbed in the first step, which is older than the
   second one.

I guess we'd want a mechanism to tell ident.c layer "discard the
cached one, as we are no longer in the same automated sequence", and
use that whenever we spawn an editor (or otherwise go interactive).

>
> Perhaps something like this below is needed.
>
> diff --git a/ident.c b/ident.c
> index 327abe557f..2c6bff7b9d 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -178,8 +178,8 @@ const char *ident_default_email(void)
>  
>  static const char *ident_default_date(void)
>  {
> - if (!git_default_date.len)
> - datestamp(_default_date);
> + strbuf_reset(_default_date);
> + datestamp(_default_date);
>   return git_default_date.buf;
>  }
>  

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Johannes Schindelin]

Hi,

On Sat, 14 Apr 2018, Phillip Wood wrote:

> On 13/04/18 17:52, Johannes Sixt wrote:
> > 
> > I just noticed that all commits in a 70-commit branch have the same
> > committer timestamp. This is very unusual on Windows, where rebase -i of
> > such a long branch takes more than one second (but not more than 3 or
> > so thanks to the builtin nature of the command!).
> > 
> > And, in fact, if you mark some commits with 'reword' to delay the quick
> > processing of the patches, then the reworded commits have later time
> > stamps, but subsequent not reworded commits receive the earlier time
> > stamp. This is clearly not intended.
> 
> Oh dear, I think this is probably due to my series making rebase commit
> in-process when the commit message isn't being edited. I didn't realize
> that git cached the commit date rather than using the current time when
> calling commit_tree_extended(). I'll take a look at it next week.

Thanks.

However, a quick lock at `git log @{u}.. --format=%ct` in my
`sequencer-shears` branch thicket (which I rebase frequently on top of
upstream's `master` using the last known-good `rebase-merges` sub-branch)
shows that the commits have different-enough commit timestamps. (It is
satisfying to see that multiple commits were made during the same second,
of course.)

So while I cannot find anything in the code that disagrees with Hannes'
assessment, it looks on the surface as if I did not encounter the bug
here.

Curious.

FWIW I agree with Hannes' patch.

> I think 'git am' probably gives all patches the same commit time as well
> if the commit date is cached though it wont suffer from the time-travel
> problem.

I thought that `git am` was the subject of such a complaint recently, but
I thought that had been resolved? Apparently I misremember...

Ciao,
Dscho

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 13/04/18 17:52, Johannes Sixt wrote:
> 
> I just noticed that all commits in a 70-commit branch have the same
> committer timestamp. This is very unusual on Windows, where rebase -i of
> such a long branch takes more than one second (but not more than 3 or
> so thanks to the builtin nature of the command!).
> 
> And, in fact, if you mark some commits with 'reword' to delay the quick
> processing of the patches, then the reworded commits have later time
> stamps, but subsequent not reworded commits receive the earlier time
> stamp. This is clearly not intended.

Oh dear, I think this is probably due to my series making rebase commit
in-process when the commit message isn't being edited. I didn't realize
that git cached the commit date rather than using the current time when
calling commit_tree_extended(). I'll take a look at it next week. I
think 'git am' probably gives all patches the same commit time as well
if the commit date is cached though it wont suffer from the time-travel
problem.

Best Wishes

Phillip

> Perhaps something like this below is needed.
> 
> diff --git a/ident.c b/ident.c
> index 327abe557f..2c6bff7b9d 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -178,8 +178,8 @@ const char *ident_default_email(void)
>  
>  static const char *ident_default_date(void)
>  {
> - if (!git_default_date.len)
> - datestamp(_default_date);
> + strbuf_reset(_default_date);
> + datestamp(_default_date);
>   return git_default_date.buf;
>  }
>  
>