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

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

[Johannes Sixt]

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.

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;
 }
 

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(&git_default_date);
> +	strbuf_reset(&git_default_date);
> +	datestamp(&git_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 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'

[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'

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> 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(&git_default_date);
> +	strbuf_reset(&git_default_date);
> +	datestamp(&git_default_date);
>  	return git_default_date.buf;
>  }
>  

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 <j6t@kdbg.org> 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 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

[RFC PATCH] ident: don't cache default date

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Now that the sequencer commits without forking when the commit message
isn't edited all the commits that are picked have the same committer
date. If a commit is reworded it's committer date will be a later time
as it is created by running an separate instance of 'git commit'.  If
the reworded commit is follow by further picks, those later commits
will have an earlier committer date than the reworded one. This is
caused by git caching the default date used when GIT_COMMITTER_DATE is
not set. Fix this by not caching the date.

Users expect commits to have the same author and committer dates when
the don't explicitly set them. As the date is now updated each time
git_author_info() or git_committer_info() is run it is possible to end
up with different author and committer dates. Fix this for
'commit-tree', 'notes' and 'merge' by using a single date in
commit_tree_extended() and passing it explicitly to the new functions
git_author_info_with_date() and git_committer_info_with_date() when
neither the author date nor the committer date are explicitly
set. 'commit' always passes the author date to commit_tree_extended()
and relied on the date caching to have the same committer and author
dates when neither was specified. Fix this by setting
GIT_COMMITTER_DATE to be the same as the author date passed to
commit_tree_extended().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Reported-by: Johannes Sixt <j6t@kdbg.org>
---

I'm slightly nervous that setting GIT_COMMITTER_DATE in
builtin/commit.c will break someone's hook script. Maybe it would be
better to add a committer parameter to commit_tree() and
commit_tree_extended().

This needs some tests. I think we could test that the sequencer gives
each commit a new date with 'git rebase -i --exec="sleep 2"
--force-rebase @~2' and test the committer dates of the rebased
commits. I'm not sure if test -gt can cope with numbers that big
though, maybe we'll have to be content with test !=. For 'git commit'
I think doing GIT_EDITOR='sleep 2; echo message >"$1"' and checking
the committer date and author date will work as the author date is set
before the user edits the commit message. I'm not sure how to test
callers of commit_tree() though (that's commit-tree, notes and merge)
as I've not been able to come up with anything that will guarantee the
author and committer dates are different if the code in
commit_tree_extended() that ensures they are the same gets broken.

 builtin/commit.c |  8 ++++++++
 cache.h          |  2 ++
 commit.c         | 22 +++++++++++++++++++---
 ident.c          | 24 ++++++++++++++++++------
 4 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 37fcb55ab0..4ad8317f88 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -584,6 +584,14 @@ static void determine_author_info(struct strbuf *author_ident)
 	export_one("GIT_AUTHOR_NAME", author.name_begin, author.name_end, 0);
 	export_one("GIT_AUTHOR_EMAIL", author.mail_begin, author.mail_end, 0);
 	export_one("GIT_AUTHOR_DATE", author.date_begin, author.tz_end, '@');
+	/*
+	 * Ensure the author and committer dates are identical if nither is
+	 * explicitly set
+	 */
+	if ((!date || !*date) && (!getenv("GIT_COMMITTER_DATE")
+				  || !*getenv("GIT_COMMITTER_DATE")))
+		export_one("GIT_COMMITTER_DATE", author.date_begin,
+			   author.tz_end, '@');
 	free(name);
 	free(email);
 	free(date);
diff --git a/cache.h b/cache.h
index a61b2d3f0d..9cde499507 100644
--- a/cache.h
+++ b/cache.h
@@ -1484,7 +1484,9 @@ int date_overflows(timestamp_t date);
 #define IDENT_NO_DATE	       2
 #define IDENT_NO_NAME	       4
 extern const char *git_author_info(int);
+extern const char *git_author_info_with_date(int, const char*);
 extern const char *git_committer_info(int);
+extern const char *git_committer_info_with_date(int, const char*);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
 extern const char *ident_default_name(void);
diff --git a/commit.c b/commit.c
index 00c99c7272..457cc8b71b 100644
--- a/commit.c
+++ b/commit.c
@@ -1513,6 +1513,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 			 const char *author, const char *sign_commit,
 			 struct commit_extra_header *extra)
 {
+	struct strbuf date_buf = STRBUF_INIT;
 	int result;
 	int encoding_is_utf8;
 	struct strbuf buffer;
@@ -1540,10 +1541,25 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 	}
 
 	/* Person/date information */
-	if (!author)
-		author = git_author_info(IDENT_STRICT);
+	if (!author) {
+		char *author_date = xstrdup_or_null(getenv("GIT_AUTHOR_DATE"));
+		char *committer_date =
+				xstrdup_or_null(getenv("GIT_COMMITTER_DATE"));
+		/*
+		 * If neither the author nor committer dates are explicitly set
+		 * ensure they are identical
+		 */
+		if (!author_date && !committer_date) {
+			datestamp(&date_buf);
+		}
+		author = git_author_info_with_date(IDENT_STRICT, date_buf.buf);
+		free(author_date);
+		free(committer_date);
+	}
 	strbuf_addf(&buffer, "author %s\n", author);
-	strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_STRICT));
+	strbuf_addf(&buffer, "committer %s\n",
+		    git_committer_info_with_date(IDENT_STRICT, date_buf.buf));
+	strbuf_release(&date_buf);
 	if (!encoding_is_utf8)
 		strbuf_addf(&buffer, "encoding %s\n", git_commit_encoding);
 
diff --git a/ident.c b/ident.c
index 327abe557f..f9eb933c2c 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;
 }
 
@@ -427,30 +427,42 @@ const char *fmt_name(const char *name, const char *email)
 	return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE);
 }
 
-const char *git_author_info(int flag)
+const char *git_author_info_with_date(int flag, const char *date_str)
 {
 	if (getenv("GIT_AUTHOR_NAME"))
 		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_AUTHOR_EMAIL"))
 		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 			 getenv("GIT_AUTHOR_EMAIL"),
-			 getenv("GIT_AUTHOR_DATE"),
+			 date_str && *date_str ? date_str :
+					getenv("GIT_AUTHOR_DATE"),
 			 flag);
 }
 
-const char *git_committer_info(int flag)
+const char *git_author_info(int flag)
+{
+	return git_author_info_with_date(flag, NULL);
+}
+
+const char *git_committer_info_with_date(int flag, const char *date_str)
 {
 	if (getenv("GIT_COMMITTER_NAME"))
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 	if (getenv("GIT_COMMITTER_EMAIL"))
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
-			 getenv("GIT_COMMITTER_DATE"),
+			 date_str && *date_str ? date_str :
+					getenv("GIT_COMMITTER_DATE"),
 			 flag);
 }
 
+const char *git_committer_info(int flag)
+{
+	return git_committer_info_with_date(flag, NULL);
+}
+
 static int ident_is_sufficient(int user_ident_explicitly_given)
 {
 #ifndef WINDOWS
-- 
2.17.0

Re: [RFC PATCH] ident: don't cache default date

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 18 2018, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Now that the sequencer commits without forking when the commit message
> isn't edited all the commits that are picked have the same committer
> date. If a commit is reworded it's committer date will be a later time

s/it's/its/

> as it is created by running an separate instance of 'git commit'.  If
> the reworded commit is follow by further picks, those later commits
> will have an earlier committer date than the reworded one. This is
> caused by git caching the default date used when GIT_COMMITTER_DATE is
> not set. Fix this by not caching the date.
>
> Users expect commits to have the same author and committer dates when
> the don't explicitly set them. As the date is now updated each time
> git_author_info() or git_committer_info() is run it is possible to end
> up with different author and committer dates. Fix this for
> 'commit-tree', 'notes' and 'merge' by using a single date in
> commit_tree_extended() and passing it explicitly to the new functions
> git_author_info_with_date() and git_committer_info_with_date() when
> neither the author date nor the committer date are explicitly
> set. 'commit' always passes the author date to commit_tree_extended()
> and relied on the date caching to have the same committer and author
> dates when neither was specified. Fix this by setting
> GIT_COMMITTER_DATE to be the same as the author date passed to
> commit_tree_extended().
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Reported-by: Johannes Sixt <j6t@kdbg.org>
> ---
>
> I'm slightly nervous that setting GIT_COMMITTER_DATE in
> builtin/commit.c will break someone's hook script. Maybe it would be
> better to add a committer parameter to commit_tree() and
> commit_tree_extended().
>
> This needs some tests. I think we could test that the sequencer gives
> each commit a new date with 'git rebase -i --exec="sleep 2"
> --force-rebase @~2' and test the committer dates of the rebased
> commits. I'm not sure if test -gt can cope with numbers that big
> though, maybe we'll have to be content with test !=. For 'git commit'
> I think doing GIT_EDITOR='sleep 2; echo message >"$1"' and checking
> the committer date and author date will work as the author date is set
> before the user edits the commit message. I'm not sure how to test
> callers of commit_tree() though (that's commit-tree, notes and merge)
> as I've not been able to come up with anything that will guarantee the
> author and committer dates are different if the code in
> commit_tree_extended() that ensures they are the same gets broken.

The behavior you're describing where we end up with later commits in the
rebase with earlier CommitDate's definitely sounds like a minor
regression, and yeah we should have tests for this.

My mental model of this is that we shouldn't be trying at all to adjust
the CommitDate in a sequence to be the same, and just make it be
whatever time() returns, except in the case where a date is passed
explicitly.

My cursory reading of this RFC patch is that it's definitely an
improvement because we don't end up with things out of order, but is
there any reason for why we should be trying to aim to keep this
"consistent" within a single "git rebase" command by default, as opposed
to always just writing the current CommitDate at the time we make the
commit, that sounds like the most intuitive thing to me, and I can't see
any downsides with that.

It leaks info about how long it took someone to do the rebase, but so
what?

Re: [RFC PATCH] ident: don't cache default date

[Phillip Wood]

Hi Ævar, thanks for your comments

On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 18 2018, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Now that the sequencer commits without forking when the commit message
>> isn't edited all the commits that are picked have the same committer
>> date. If a commit is reworded it's committer date will be a later time
> 
> s/it's/its/

Thanks I'll change it

>> as it is created by running an separate instance of 'git commit'.  If
>> the reworded commit is follow by further picks, those later commits
>> will have an earlier committer date than the reworded one. This is
>> caused by git caching the default date used when GIT_COMMITTER_DATE is
>> not set. Fix this by not caching the date.
>>
>> Users expect commits to have the same author and committer dates when
>> the don't explicitly set them. As the date is now updated each time
>> git_author_info() or git_committer_info() is run it is possible to end
>> up with different author and committer dates. Fix this for
>> 'commit-tree', 'notes' and 'merge' by using a single date in
>> commit_tree_extended() and passing it explicitly to the new functions
>> git_author_info_with_date() and git_committer_info_with_date() when
>> neither the author date nor the committer date are explicitly
>> set. 'commit' always passes the author date to commit_tree_extended()
>> and relied on the date caching to have the same committer and author
>> dates when neither was specified. Fix this by setting
>> GIT_COMMITTER_DATE to be the same as the author date passed to
>> commit_tree_extended().
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> Reported-by: Johannes Sixt <j6t@kdbg.org>
>> ---
>>
>> I'm slightly nervous that setting GIT_COMMITTER_DATE in
>> builtin/commit.c will break someone's hook script. Maybe it would be
>> better to add a committer parameter to commit_tree() and
>> commit_tree_extended().
>>
>> This needs some tests. I think we could test that the sequencer gives
>> each commit a new date with 'git rebase -i --exec="sleep 2"
>> --force-rebase @~2' and test the committer dates of the rebased
>> commits. I'm not sure if test -gt can cope with numbers that big
>> though, maybe we'll have to be content with test !=. For 'git commit'
>> I think doing GIT_EDITOR='sleep 2; echo message >"$1"' and checking
>> the committer date and author date will work as the author date is set
>> before the user edits the commit message. I'm not sure how to test
>> callers of commit_tree() though (that's commit-tree, notes and merge)
>> as I've not been able to come up with anything that will guarantee the
>> author and committer dates are different if the code in
>> commit_tree_extended() that ensures they are the same gets broken.
> 
> The behavior you're describing where we end up with later commits in the
> rebase with earlier CommitDate's definitely sounds like a minor
> regression, and yeah we should have tests for this.
> 
> My mental model of this is that we shouldn't be trying at all to adjust
> the CommitDate in a sequence to be the same, and just make it be
> whatever time() returns, except in the case where a date is passed
> explicitly.
> 
> My cursory reading of this RFC patch is that it's definitely an
> improvement because we don't end up with things out of order, but is
> there any reason for why we should be trying to aim to keep this
> "consistent" within a single "git rebase" command by default, as opposed
> to always just writing the current CommitDate at the time we make the
> commit, that sounds like the most intuitive thing to me, and I can't see
> any downsides with that.

It's not trying to keep the date "consistent" within a single rebase
command, each commit created by the sequencer gets a date stamp with the
current time when the commit is created. What it is doing is keeping the
author and committer dates the same for commit/commit-tree/merge/notes
when the user does not specify either of them. While I don't think it
particularly matters that they match (so long as the committer date is
later than the author date), it is what git does at the moment and
someone maybe relying on the current behavior.

Best Wishes

Phillip

> 
> It leaks info about how long it took someone to do the rebase, but so
> what?
> 

Re: [RFC PATCH] ident: don't cache default date

[Johannes Sixt]

Am 18.04.2018 um 19:47 schrieb Phillip Wood:
> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Apr 18 2018, Phillip Wood wrote:
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> as it is created by running an separate instance of 'git commit'.  If
>>> the reworded commit is follow by further picks, those later commits
>>> will have an earlier committer date than the reworded one. This is
>>> caused by git caching the default date used when GIT_COMMITTER_DATE is
>>> not set. Fix this by not caching the date.
>>>
>>> Users expect commits to have the same author and committer dates when
>>> the don't explicitly set them. As the date is now updated each time
>>> git_author_info() or git_committer_info() is run it is possible to end
>>> up with different author and committer dates. Fix this for
>>> 'commit-tree', 'notes' and 'merge' by using a single date in
>>> commit_tree_extended() and passing it explicitly to the new functions
>>> git_author_info_with_date() and git_committer_info_with_date() when
>>> neither the author date nor the committer date are explicitly
>>> set. 'commit' always passes the author date to commit_tree_extended()
>>> and relied on the date caching to have the same committer and author
>>> dates when neither was specified. Fix this by setting
>>> GIT_COMMITTER_DATE to be the same as the author date passed to
>>> commit_tree_extended().
>>>
>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> Reported-by: Johannes Sixt <j6t@kdbg.org>
>>> ---
>>>
>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in
>>> builtin/commit.c will break someone's hook script. Maybe it would be
>>> better to add a committer parameter to commit_tree() and
>>> commit_tree_extended().

While I like the basic theme of your patch, I think we should fix this
case in a much simpler way, namely, use the infrastructure that was
introduced for git-am.

I've shamelessly lifted the commit message from your patch.

---- 8< ----
Subject: [PATCH] sequencer: reset the committer date before commits

Now that the sequencer commits without forking when the commit message
isn't edited all the commits that are picked have the same committer
date. If a commit is reworded it's committer date will be a later time
as it is created by running an separate instance of 'git commit'.  If
the reworded commit is follow by further picks, those later commits
will have an earlier committer date than the reworded one. This is
caused by git caching the default date used when GIT_COMMITTER_DATE is
not set. Reset the cached date before a commit is generated
in-process.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 sequencer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index f9d1001dee..f0bac903a0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char *author,
 		goto out;
 	}
 
+	reset_ident_date();
+
 	if (commit_tree_extended(msg->buf, msg->len, &tree, parents,
 				 oid, author, opts->gpg_sign, extra)) {
 		res = error(_("failed to write commit object"));
-- 
2.17.0.69.g0c1d01d9b6

Re: [RFC PATCH] ident: don't cache default date

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> While I like the basic theme of your patch, I think we should fix this
> case in a much simpler way, namely, use the infrastructure that was
> introduced for git-am.

Yup.  reset_ident_date() was introduced by 4d9c7e6f ("am: reset
cached ident date for each patch", 2016-08-01) and the commit
explains very well why it is a good idea to have both the caching
and also the strategic resetting it introduces.

Thanks, all.

> I've shamelessly lifted the commit message from your patch.
>
> ---- 8< ----
> Subject: [PATCH] sequencer: reset the committer date before commits
>
> Now that the sequencer commits without forking when the commit message
> isn't edited all the commits that are picked have the same committer
> date. If a commit is reworded it's committer date will be a later time
> as it is created by running an separate instance of 'git commit'.  If
> the reworded commit is follow by further picks, those later commits
> will have an earlier committer date than the reworded one. This is
> caused by git caching the default date used when GIT_COMMITTER_DATE is
> not set. Reset the cached date before a commit is generated
> in-process.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  sequencer.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index f9d1001dee..f0bac903a0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char *author,
>  		goto out;
>  	}
>  
> +	reset_ident_date();
> +
>  	if (commit_tree_extended(msg->buf, msg->len, &tree, parents,
>  				 oid, author, opts->gpg_sign, extra)) {
>  		res = error(_("failed to write commit object"));

Re: [RFC PATCH] ident: don't cache default date

[Phillip Wood]

On 18/04/18 19:15, Johannes Sixt wrote:
> Am 18.04.2018 um 19:47 schrieb Phillip Wood:
>> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:
>>> On Wed, Apr 18 2018, Phillip Wood wrote:
>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>> as it is created by running an separate instance of 'git commit'.  If
>>>> the reworded commit is follow by further picks, those later commits
>>>> will have an earlier committer date than the reworded one. This is
>>>> caused by git caching the default date used when GIT_COMMITTER_DATE is
>>>> not set. Fix this by not caching the date.
>>>>
>>>> Users expect commits to have the same author and committer dates when
>>>> the don't explicitly set them. As the date is now updated each time
>>>> git_author_info() or git_committer_info() is run it is possible to end
>>>> up with different author and committer dates. Fix this for
>>>> 'commit-tree', 'notes' and 'merge' by using a single date in
>>>> commit_tree_extended() and passing it explicitly to the new functions
>>>> git_author_info_with_date() and git_committer_info_with_date() when
>>>> neither the author date nor the committer date are explicitly
>>>> set. 'commit' always passes the author date to commit_tree_extended()
>>>> and relied on the date caching to have the same committer and author
>>>> dates when neither was specified. Fix this by setting
>>>> GIT_COMMITTER_DATE to be the same as the author date passed to
>>>> commit_tree_extended().
>>>>
>>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>> Reported-by: Johannes Sixt <j6t@kdbg.org>
>>>> ---
>>>>
>>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in
>>>> builtin/commit.c will break someone's hook script. Maybe it would be
>>>> better to add a committer parameter to commit_tree() and
>>>> commit_tree_extended().
> 
> While I like the basic theme of your patch, I think we should fix this
> case in a much simpler way, namely, use the infrastructure that was
> introduced for git-am.
> 
> I've shamelessly lifted the commit message from your patch.

Thanks, that is a better way (I'm annoyed with myself for not having
noticed reset_ident_date() when I edited the function above it)

Best Wishes

Phillip

> ---- 8< ----
> Subject: [PATCH] sequencer: reset the committer date before commits
> 
> Now that the sequencer commits without forking when the commit message
> isn't edited all the commits that are picked have the same committer
> date. If a commit is reworded it's committer date will be a later time
> as it is created by running an separate instance of 'git commit'.  If
> the reworded commit is follow by further picks, those later commits
> will have an earlier committer date than the reworded one. This is
> caused by git caching the default date used when GIT_COMMITTER_DATE is
> not set. Reset the cached date before a commit is generated
> in-process.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  sequencer.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sequencer.c b/sequencer.c
> index f9d1001dee..f0bac903a0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char *author,
>  		goto out;
>  	}
>  
> +	reset_ident_date();
> +
>  	if (commit_tree_extended(msg->buf, msg->len, &tree, parents,
>  				 oid, author, opts->gpg_sign, extra)) {
>  		res = error(_("failed to write commit object"));
> 

Re: [RFC PATCH] ident: don't cache default date

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]

Hi Phillip,

On Thu, 19 Apr 2018, Phillip Wood wrote:

> On 18/04/18 19:15, Johannes Sixt wrote:
> > Am 18.04.2018 um 19:47 schrieb Phillip Wood:
> >> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:
> >>> On Wed, Apr 18 2018, Phillip Wood wrote:
> >>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>>> as it is created by running an separate instance of 'git commit'.  If
> >>>> the reworded commit is follow by further picks, those later commits
> >>>> will have an earlier committer date than the reworded one. This is
> >>>> caused by git caching the default date used when GIT_COMMITTER_DATE is
> >>>> not set. Fix this by not caching the date.
> >>>>
> >>>> Users expect commits to have the same author and committer dates when
> >>>> the don't explicitly set them. As the date is now updated each time
> >>>> git_author_info() or git_committer_info() is run it is possible to end
> >>>> up with different author and committer dates. Fix this for
> >>>> 'commit-tree', 'notes' and 'merge' by using a single date in
> >>>> commit_tree_extended() and passing it explicitly to the new functions
> >>>> git_author_info_with_date() and git_committer_info_with_date() when
> >>>> neither the author date nor the committer date are explicitly
> >>>> set. 'commit' always passes the author date to commit_tree_extended()
> >>>> and relied on the date caching to have the same committer and author
> >>>> dates when neither was specified. Fix this by setting
> >>>> GIT_COMMITTER_DATE to be the same as the author date passed to
> >>>> commit_tree_extended().
> >>>>
> >>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>>> Reported-by: Johannes Sixt <j6t@kdbg.org>
> >>>> ---
> >>>>
> >>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in
> >>>> builtin/commit.c will break someone's hook script. Maybe it would be
> >>>> better to add a committer parameter to commit_tree() and
> >>>> commit_tree_extended().
> > 
> > While I like the basic theme of your patch, I think we should fix this
> > case in a much simpler way, namely, use the infrastructure that was
> > introduced for git-am.
> > 
> > I've shamelessly lifted the commit message from your patch.
> 
> Thanks, that is a better way (I'm annoyed with myself for not having
> noticed reset_ident_date() when I edited the function above it)

Don't be too annoyed. I did remember that The Linus had complained about
something similar and assumed that it had been fixed in the meantime, but
I failed to find it within 30 minutes where I tried to dig through
public-inbox and pu.

Thanks Hannes for remembering, and for coming up with the final form of
the patch!

Ciao,
Dscho

Re: [RFC PATCH] ident: don't cache default date

[Phillip Wood]

On 20/04/18 09:11, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Thu, 19 Apr 2018, Phillip Wood wrote:
> 
>> On 18/04/18 19:15, Johannes Sixt wrote:
>>> Am 18.04.2018 um 19:47 schrieb Phillip Wood:
>>>> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:
>>>>> On Wed, Apr 18 2018, Phillip Wood wrote:
>>>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>>> as it is created by running an separate instance of 'git commit'.  If
>>>>>> the reworded commit is follow by further picks, those later commits
>>>>>> will have an earlier committer date than the reworded one. This is
>>>>>> caused by git caching the default date used when GIT_COMMITTER_DATE is
>>>>>> not set. Fix this by not caching the date.
>>>>>>
>>>>>> Users expect commits to have the same author and committer dates when
>>>>>> the don't explicitly set them. As the date is now updated each time
>>>>>> git_author_info() or git_committer_info() is run it is possible to end
>>>>>> up with different author and committer dates. Fix this for
>>>>>> 'commit-tree', 'notes' and 'merge' by using a single date in
>>>>>> commit_tree_extended() and passing it explicitly to the new functions
>>>>>> git_author_info_with_date() and git_committer_info_with_date() when
>>>>>> neither the author date nor the committer date are explicitly
>>>>>> set. 'commit' always passes the author date to commit_tree_extended()
>>>>>> and relied on the date caching to have the same committer and author
>>>>>> dates when neither was specified. Fix this by setting
>>>>>> GIT_COMMITTER_DATE to be the same as the author date passed to
>>>>>> commit_tree_extended().
>>>>>>
>>>>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>>> Reported-by: Johannes Sixt <j6t@kdbg.org>
>>>>>> ---
>>>>>>
>>>>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in
>>>>>> builtin/commit.c will break someone's hook script. Maybe it would be
>>>>>> better to add a committer parameter to commit_tree() and
>>>>>> commit_tree_extended().
>>>
>>> While I like the basic theme of your patch, I think we should fix this
>>> case in a much simpler way, namely, use the infrastructure that was
>>> introduced for git-am.
>>>
>>> I've shamelessly lifted the commit message from your patch.
>>
>> Thanks, that is a better way (I'm annoyed with myself for not having
>> noticed reset_ident_date() when I edited the function above it)
> 
> Don't be too annoyed. I did remember that The Linus had complained about
> something similar and assumed that it had been fixed in the meantime, but
> I failed to find it within 30 minutes where I tried to dig through
> public-inbox and pu.

Thanks, that makes we feel better about it (thanks for taking the time
to try and find the original mail as well)

Phillip

> Thanks Hannes for remembering, and for coming up with the final form of
> the patch!
> 
> Ciao,
> Dscho
>