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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2018, 21:35 UTC
Message-ID: <xmqq7ep817bq.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <5f5d5b88-b3ac-ed4f-ee24-6ce2cba2bd55@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 9 quoted lines

> 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).

Show 18 quoted lines

>
> 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(&git_default_date);
> +	strbuf_reset(&git_default_date);
> +	datestamp(&git_default_date);
>  	return git_default_date.buf;
>  }
>