{"thread":{"id":"48280","subject":"Bug: rebase -i creates committer time inversions on 'reword'","startedAt":"2018-04-13T16:52:44Z","lastAt":"2018-04-20T09:41:15Z","messageCount":17,"participants":["Johannes Sixt","Phillip Wood","Johannes Schindelin","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"344615","messageId":"5f5d5b88-b3ac-ed4f-ee24-6ce2cba2bd55@kdbg.org","threadId":"48280","inReplyTo":null,"subject":"Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-04-13T16:52:28Z","receivedAt":"2018-04-13T16:52:44Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"I just noticed that all commits in a 70-commit branch have the same\ncommitter timestamp. This is very unusual on Windows, where rebase -i of\nsuch a long branch takes more than one second (but not more than 3 or\nso thanks to the builtin nature of the command!).\n\nAnd, in fact, if you mark some commits with 'reword' to delay the quick\nprocessing of the patches, then the reworded commits have later time\nstamps, but subsequent not reworded commits receive the earlier time\nstamp. This is clearly not intended.\n\nPerhaps something like this below is needed.\n\ndiff --git a/ident.c b/ident.c\nindex 327abe557f..2c6bff7b9d 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -178,8 +178,8 @@ const char *ident_default_email(void)\n \n static const char *ident_default_date(void)\n {\n-\tif (!git_default_date.len)\n-\t\tdatestamp(&git_default_date);\n+\tstrbuf_reset(&git_default_date);\n+\tdatestamp(&git_default_date);\n \treturn git_default_date.buf;\n }\n \n\n"},{"id":"344659","messageId":"f56b84fa-9286-3a7b-fc5b-2307c9ff5b5e@philandanna.no-ip.org","threadId":"48280","inReplyTo":"5f5d5b88-b3ac-ed4f-ee24-6ce2cba2bd55@kdbg.org","subject":"Re: Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Phillip Wood","fromEmail":"phil@philandanna.no-ip.org","sentAt":"2018-04-14T11:15:39Z","receivedAt":"2018-04-14T11:24:43Z","isPatch":false,"sender":{"key":"phil@philandanna.no-ip.org","avatar":null},"body":"\nOn 13/04/18 17:52, Johannes Sixt wrote:\n> \n> I just noticed that all commits in a 70-commit branch have the same\n> committer timestamp. This is very unusual on Windows, where rebase -i of\n> such a long branch takes more than one second (but not more than 3 or\n> so thanks to the builtin nature of the command!).\n> \n> And, in fact, if you mark some commits with 'reword' to delay the quick\n> processing of the patches, then the reworded commits have later time\n> stamps, but subsequent not reworded commits receive the earlier time\n> stamp. This is clearly not intended.\n\nOh dear, I think this is probably due to my series making rebase commit\nin-process when the commit message isn't being edited. I didn't realize\nthat git cached the commit date rather than using the current time when\ncalling commit_tree_extended(). I'll take a look at it next week. I\nthink 'git am' probably gives all patches the same commit time as well\nif the commit date is cached though it wont suffer from the time-travel\nproblem.\n\nBest Wishes\n\nPhillip\n\n> Perhaps something like this below is needed.\n> \n> diff --git a/ident.c b/ident.c\n> index 327abe557f..2c6bff7b9d 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -178,8 +178,8 @@ const char *ident_default_email(void)\n> \n> static const char *ident_default_date(void)\n> {\n> -\tif (!git_default_date.len)\n> -\t\tdatestamp(&git_default_date);\n> +\tstrbuf_reset(&git_default_date);\n> +\tdatestamp(&git_default_date);\n> \treturn git_default_date.buf;\n> }\n> \n> \n\n"},{"id":"344661","messageId":"nycvar.QRO.7.76.6.1804141444060.234@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48280","inReplyTo":"f56b84fa-9286-3a7b-fc5b-2307c9ff5b5e@philandanna.no-ip.org","subject":"Re: Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-14T13:11:28Z","receivedAt":"2018-04-14T13:11:37Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 14 Apr 2018, Phillip Wood wrote:\n\n> On 13/04/18 17:52, Johannes Sixt wrote:\n> > \n> > I just noticed that all commits in a 70-commit branch have the same\n> > committer timestamp. This is very unusual on Windows, where rebase -i of\n> > such a long branch takes more than one second (but not more than 3 or\n> > so thanks to the builtin nature of the command!).\n> > \n> > And, in fact, if you mark some commits with 'reword' to delay the quick\n> > processing of the patches, then the reworded commits have later time\n> > stamps, but subsequent not reworded commits receive the earlier time\n> > stamp. This is clearly not intended.\n> \n> Oh dear, I think this is probably due to my series making rebase commit\n> in-process when the commit message isn't being edited. I didn't realize\n> that git cached the commit date rather than using the current time when\n> calling commit_tree_extended(). I'll take a look at it next week.\n\nThanks.\n\nHowever, a quick lock at `git log @{u}.. --format=%ct` in my\n`sequencer-shears` branch thicket (which I rebase frequently on top of\nupstream's `master` using the last known-good `rebase-merges` sub-branch)\nshows that the commits have different-enough commit timestamps. (It is\nsatisfying to see that multiple commits were made during the same second,\nof course.)\n\nSo while I cannot find anything in the code that disagrees with Hannes'\nassessment, it looks on the surface as if I did not encounter the bug\nhere.\n\nCurious.\n\nFWIW I agree with Hannes' patch.\n\n> I think 'git am' probably gives all patches the same commit time as well\n> if the commit date is cached though it wont suffer from the time-travel\n> problem.\n\nI thought that `git am` was the subject of such a complaint recently, but\nI thought that had been resolved? Apparently I misremember...\n\nCiao,\nDscho\n"},{"id":"344757","messageId":"xmqq7ep817bq.fsf@gitster-ct.c.googlers.com","threadId":"48280","inReplyTo":"5f5d5b88-b3ac-ed4f-ee24-6ce2cba2bd55@kdbg.org","subject":"Re: Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-15T21:35:37Z","receivedAt":"2018-04-15T21:37:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> I just noticed that all commits in a 70-commit branch have the same\n> committer timestamp. This is very unusual on Windows, where rebase -i of\n> such a long branch takes more than one second (but not more than 3 or\n> so thanks to the builtin nature of the command!).\n>\n> And, in fact, if you mark some commits with 'reword' to delay the quick\n> processing of the patches, then the reworded commits have later time\n> stamps, but subsequent not reworded commits receive the earlier time\n> stamp. This is clearly not intended.\n\nHmm, I may be missing something without enough caffeine but I am\npuzzled how that would be possible. With a \"few picks, an edit, and\na yet more picks\" sequence, the first picks may share the same\ntimestamp due to the git_default_date caching (which I think is a\ndeliberate design choice we made), an edit that stops will let the\nconcluding \"commit\" (either by the end user or invoked internally\nvia \"rebase --continue\"), but because that process restarts afresh,\nthe commits made by \"yet more picks\" cannot share the timestamp that\nwas cached for the earliest ones from the same series, no?\n\nAh, do you mean we have an internal sequence like this, when \"rebase\n--continue\" wants to conclude an edit/reword?\n\n - we figure out the committer ident, which grabs a timestamp and\n cache it;\n\n - we spawn \"commit\" to conclude the stopped step, letting it record\n its beginning time (which is a bit older than the above) or its\n ending time (which is much older due to human typing speed);\n\n - subsequent \"picks\" are made in the same process, and share the\n timestamp we grabbed in the first step, which is older than the\n second one.\n\nI guess we'd want a mechanism to tell ident.c layer \"discard the\ncached one, as we are no longer in the same automated sequence\", and\nuse that whenever we spawn an editor (or otherwise go interactive).\n\n>\n> Perhaps something like this below is needed.\n>\n> diff --git a/ident.c b/ident.c\n> index 327abe557f..2c6bff7b9d 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -178,8 +178,8 @@ const char *ident_default_email(void)\n> \n> static const char *ident_default_date(void)\n> {\n> -\tif (!git_default_date.len)\n> -\t\tdatestamp(&git_default_date);\n> +\tstrbuf_reset(&git_default_date);\n> +\tdatestamp(&git_default_date);\n> \treturn git_default_date.buf;\n> }\n> \n"},{"id":"344776","messageId":"06c5bd54-f1b0-7fe5-6aa8-870e0ae4487d@kdbg.org","threadId":"48280","inReplyTo":"xmqq7ep817bq.fsf@gitster-ct.c.googlers.com","subject":"Re: Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-04-16T05:56:52Z","receivedAt":"2018-04-16T05:57:00Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.04.2018 um 23:35 schrieb Junio C Hamano:\n> Ah, do you mean we have an internal sequence like this, when \"rebase\n> --continue\" wants to conclude an edit/reword?\n\nYes, it's only 'reword' that is affected, because then subsequent picks \nare processed by the original process.\n\n> - we figure out the committer ident, which grabs a timestamp and\n> cache it;\n> \n> - we spawn \"commit\" to conclude the stopped step, letting it record\n> its beginning time (which is a bit older than the above) or its\n> ending time (which is much older due to human typing speed);\n\nYounger in both cases, of course. According to my tests, we seem to pick \nthe beginning time, because the first 'reword'ed commit typically has \nthe same timestamp as the preceding picks. Later 'reword'ed commits have \nnoticably younger timestamps.\n\n> - subsequent \"picks\" are made in the same process, and share the\n> timestamp we grabbed in the first step, which is older than the\n> second one.\n> \n> I guess we'd want a mechanism to tell ident.c layer \"discard the\n> cached one, as we are no longer in the same automated sequence\", and\n> use that whenever we spawn an editor (or otherwise go interactive).\n\nFrankly, I think that this caching is overengineered (or prematurly \noptimized). If the design requires that different callers of datestamp() \nmust see the same time, then the design is broken. In a fixed design, \nthere would be a single call of datestamp() in advance, and then the \ntimestamp, which then obviously is a very important piece of data, would \nbe passed along as required.\n\n-- Hannes\n"},{"id":"344781","messageId":"ba17074b-865e-32ba-908d-0d94bedc50ad@talktalk.net","threadId":"48280","inReplyTo":"nycvar.QRO.7.76.6.1804141444060.234@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-16T09:48:17Z","receivedAt":"2018-04-16T09:48:28Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/04/18 14:11, Johannes Schindelin wrote:\n> Hi,\n> \n> On Sat, 14 Apr 2018, Phillip Wood wrote:\n> \n>> On 13/04/18 17:52, Johannes Sixt wrote:\n>>>\n>>> I just noticed that all commits in a 70-commit branch have the same\n>>> committer timestamp. This is very unusual on Windows, where rebase -i of\n>>> such a long branch takes more than one second (but not more than 3 or\n>>> so thanks to the builtin nature of the command!).\n>>>\n>>> And, in fact, if you mark some commits with 'reword' to delay the quick\n>>> processing of the patches, then the reworded commits have later time\n>>> stamps, but subsequent not reworded commits receive the earlier time\n>>> stamp. This is clearly not intended.\n>>\n>> Oh dear, I think this is probably due to my series making rebase commit\n>> in-process when the commit message isn't being edited. I didn't realize\n>> that git cached the commit date rather than using the current time when\n>> calling commit_tree_extended(). I'll take a look at it next week.\n> \n> Thanks.\n> \n> However, a quick lock at `git log @{u}.. --format=%ct` in my\n> `sequencer-shears` branch thicket (which I rebase frequently on top of\n> upstream's `master` using the last known-good `rebase-merges` sub-branch)\n> shows that the commits have different-enough commit timestamps. (It is\n> satisfying to see that multiple commits were made during the same second,\n> of course.)\n> \n> So while I cannot find anything in the code that disagrees with Hannes'\n> assessment, it looks on the surface as if I did not encounter the bug\n> here.\n> \n> Curious.\n\nThat's strange (I'd have expected the picks after recreated merges to\nhave the earlier timestamps than the merge), if I do 'git rebase -i\n--force-rebase --exec=\"sleep 2\" @~5' then all the new commits have the\nsame timestamp.\n\n> FWIW I agree with Hannes' patch.\n> \n>> I think 'git am' probably gives all patches the same commit time as well\n>> if the commit date is cached though it wont suffer from the time-travel\n>> problem.\n> \n> I thought that `git am` was the subject of such a complaint recently, but\n> I thought that had been resolved? Apparently I misremember...\n\nI had a quick look and couldn't see anything about that, it looks to me\nlike it just calls commit_tree() and only does anything to change the\ndefault commit date if '--committer-date-is-author-date' was given.\n\nBest Wishes\n\nPhillip\n> Ciao,\n> Dscho\n> \n\n"},{"id":"344854","messageId":"xmqqa7u2wr2j.fsf@gitster-ct.c.googlers.com","threadId":"48280","inReplyTo":"06c5bd54-f1b0-7fe5-6aa8-870e0ae4487d@kdbg.org","subject":"Re: Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-17T01:37:56Z","receivedAt":"2018-04-17T01:38:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 15.04.2018 um 23:35 schrieb Junio C Hamano:\n>> Ah, do you mean we have an internal sequence like this, when \"rebase\n>> --continue\" wants to conclude an edit/reword?\n>\n> Yes, it's only 'reword' that is affected, because then subsequent\n> picks are processed by the original process.\n\nAh, OK, that is a good explanation.\n\n"},{"id":"344974","messageId":"808c222a-c566-3654-4082-cc0e04a4ad20@talktalk.net","threadId":"48280","inReplyTo":"06c5bd54-f1b0-7fe5-6aa8-870e0ae4487d@kdbg.org","subject":"Re: Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-18T10:19:30Z","receivedAt":"2018-04-18T10:19:37Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/04/18 06:56, Johannes Sixt wrote:\n> \n> Am 15.04.2018 um 23:35 schrieb Junio C Hamano:\n>> Ah, do you mean we have an internal sequence like this, when \"rebase\n>> --continue\" wants to conclude an edit/reword?\n> \n> Yes, it's only 'reword' that is affected, because then subsequent picks\n> are processed by the original process.\n> \n>>   - we figure out the committer ident, which grabs a timestamp and\n>>     cache it;\n>>\n>>   - we spawn \"commit\" to conclude the stopped step, letting it record\n>>     its beginning time (which is a bit older than the above) or its\n>>     ending time (which is much older due to human typing speed);\n> \n> Younger in both cases, of course. According to my tests, we seem to pick\n> the beginning time, because the first 'reword'ed commit typically has\n> the same timestamp as the preceding picks. Later 'reword'ed commits have\n> noticably younger timestamps.\n> \n>>   - subsequent \"picks\" are made in the same process, and share the\n>>     timestamp we grabbed in the first step, which is older than the\n>>     second one.\n>>\n>> I guess we'd want a mechanism to tell ident.c layer \"discard the\n>> cached one, as we are no longer in the same automated sequence\", and\n>> use that whenever we spawn an editor (or otherwise go interactive).\n> \n> Frankly, I think that this caching is overengineered (or prematurly\n> optimized). If the design requires that different callers of datestamp()\n> must see the same time, then the design is broken. In a fixed design,\n> there would be a single call of datestamp() in advance, and then the\n> timestamp, which then obviously is a very important piece of data, would\n> be passed along as required.\n\nI'm inclined to agree, though it creates complications if we're going to\nkeep giving commits the same author and committer dates when neither is\nexplicitly specified.\n\nBest Wishes\n\nPhillip\n\n> \n> -- Hannes\n\n"},{"id":"344975","messageId":"20180418102236.7183-1-phillip.wood@talktalk.net","threadId":"48280","inReplyTo":"5f5d5b88-b3ac-ed4f-ee24-6ce2cba2bd55@kdbg.org","subject":"[RFC PATCH] ident: don't cache default date","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-18T10:22:36Z","receivedAt":"2018-04-18T10:23:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nNow that the sequencer commits without forking when the commit message\nisn't edited all the commits that are picked have the same committer\ndate. If a commit is reworded it's committer date will be a later time\nas it is created by running an separate instance of 'git commit'. If\nthe reworded commit is follow by further picks, those later commits\nwill have an earlier committer date than the reworded one. This is\ncaused by git caching the default date used when GIT_COMMITTER_DATE is\nnot set. Fix this by not caching the date.\n\nUsers expect commits to have the same author and committer dates when\nthe don't explicitly set them. As the date is now updated each time\ngit_author_info() or git_committer_info() is run it is possible to end\nup with different author and committer dates. Fix this for\n'commit-tree', 'notes' and 'merge' by using a single date in\ncommit_tree_extended() and passing it explicitly to the new functions\ngit_author_info_with_date() and git_committer_info_with_date() when\nneither the author date nor the committer date are explicitly\nset. 'commit' always passes the author date to commit_tree_extended()\nand relied on the date caching to have the same committer and author\ndates when neither was specified. Fix this by setting\nGIT_COMMITTER_DATE to be the same as the author date passed to\ncommit_tree_extended().\n\nSigned-off-by: Phillip Wood \nReported-by: Johannes Sixt \n---\n\nI'm slightly nervous that setting GIT_COMMITTER_DATE in\nbuiltin/commit.c will break someone's hook script. Maybe it would be\nbetter to add a committer parameter to commit_tree() and\ncommit_tree_extended().\n\nThis needs some tests. I think we could test that the sequencer gives\neach commit a new date with 'git rebase -i --exec=\"sleep 2\"\n--force-rebase @~2' and test the committer dates of the rebased\ncommits. I'm not sure if test -gt can cope with numbers that big\nthough, maybe we'll have to be content with test !=. For 'git commit'\nI think doing GIT_EDITOR='sleep 2; echo message >\"$1\"' and checking\nthe committer date and author date will work as the author date is set\nbefore the user edits the commit message. I'm not sure how to test\ncallers of commit_tree() though (that's commit-tree, notes and merge)\nas I've not been able to come up with anything that will guarantee the\nauthor and committer dates are different if the code in\ncommit_tree_extended() that ensures they are the same gets broken.\n\n builtin/commit.c | 8 ++++++++\n cache.h | 2 ++\n commit.c | 22 +++++++++++++++++++---\n ident.c | 24 ++++++++++++++++++------\n 4 files changed, 47 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 37fcb55ab0..4ad8317f88 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -584,6 +584,14 @@ static void determine_author_info(struct strbuf *author_ident)\n \texport_one(\"GIT_AUTHOR_NAME\", author.name_begin, author.name_end, 0);\n \texport_one(\"GIT_AUTHOR_EMAIL\", author.mail_begin, author.mail_end, 0);\n \texport_one(\"GIT_AUTHOR_DATE\", author.date_begin, author.tz_end, '@');\n+\t/*\n+\t * Ensure the author and committer dates are identical if nither is\n+\t * explicitly set\n+\t */\n+\tif ((!date || !*date) && (!getenv(\"GIT_COMMITTER_DATE\")\n+\t\t\t\t || !*getenv(\"GIT_COMMITTER_DATE\")))\n+\t\texport_one(\"GIT_COMMITTER_DATE\", author.date_begin,\n+\t\t\t author.tz_end, '@');\n \tfree(name);\n \tfree(email);\n \tfree(date);\ndiff --git a/cache.h b/cache.h\nindex a61b2d3f0d..9cde499507 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1484,7 +1484,9 @@ int date_overflows(timestamp_t date);\n #define IDENT_NO_DATE\t 2\n #define IDENT_NO_NAME\t 4\n extern const char *git_author_info(int);\n+extern const char *git_author_info_with_date(int, const char*);\n extern const char *git_committer_info(int);\n+extern const char *git_committer_info_with_date(int, const char*);\n extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);\n extern const char *fmt_name(const char *name, const char *email);\n extern const char *ident_default_name(void);\ndiff --git a/commit.c b/commit.c\nindex 00c99c7272..457cc8b71b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1513,6 +1513,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,\n \t\t\t const char *author, const char *sign_commit,\n \t\t\t struct commit_extra_header *extra)\n {\n+\tstruct strbuf date_buf = STRBUF_INIT;\n \tint result;\n \tint encoding_is_utf8;\n \tstruct strbuf buffer;\n@@ -1540,10 +1541,25 @@ int commit_tree_extended(const char *msg, size_t msg_len,\n \t}\n \n \t/* Person/date information */\n-\tif (!author)\n-\t\tauthor = git_author_info(IDENT_STRICT);\n+\tif (!author) {\n+\t\tchar *author_date = xstrdup_or_null(getenv(\"GIT_AUTHOR_DATE\"));\n+\t\tchar *committer_date =\n+\t\t\t\txstrdup_or_null(getenv(\"GIT_COMMITTER_DATE\"));\n+\t\t/*\n+\t\t * If neither the author nor committer dates are explicitly set\n+\t\t * ensure they are identical\n+\t\t */\n+\t\tif (!author_date && !committer_date) {\n+\t\t\tdatestamp(&date_buf);\n+\t\t}\n+\t\tauthor = git_author_info_with_date(IDENT_STRICT, date_buf.buf);\n+\t\tfree(author_date);\n+\t\tfree(committer_date);\n+\t}\n \tstrbuf_addf(&buffer, \"author %s\\n\", author);\n-\tstrbuf_addf(&buffer, \"committer %s\\n\", git_committer_info(IDENT_STRICT));\n+\tstrbuf_addf(&buffer, \"committer %s\\n\",\n+\t\t git_committer_info_with_date(IDENT_STRICT, date_buf.buf));\n+\tstrbuf_release(&date_buf);\n \tif (!encoding_is_utf8)\n \t\tstrbuf_addf(&buffer, \"encoding %s\\n\", git_commit_encoding);\n \ndiff --git a/ident.c b/ident.c\nindex 327abe557f..f9eb933c2c 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -178,8 +178,8 @@ const char *ident_default_email(void)\n \n static const char *ident_default_date(void)\n {\n-\tif (!git_default_date.len)\n-\t\tdatestamp(&git_default_date);\n+\tstrbuf_reset(&git_default_date);\n+\tdatestamp(&git_default_date);\n \treturn git_default_date.buf;\n }\n \n@@ -427,30 +427,42 @@ const char *fmt_name(const char *name, const char *email)\n \treturn fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE);\n }\n \n-const char *git_author_info(int flag)\n+const char *git_author_info_with_date(int flag, const char *date_str)\n {\n \tif (getenv(\"GIT_AUTHOR_NAME\"))\n \t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \tif (getenv(\"GIT_AUTHOR_EMAIL\"))\n \t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_AUTHOR_NAME\"),\n \t\t\t getenv(\"GIT_AUTHOR_EMAIL\"),\n-\t\t\t getenv(\"GIT_AUTHOR_DATE\"),\n+\t\t\t date_str && *date_str ? date_str :\n+\t\t\t\t\tgetenv(\"GIT_AUTHOR_DATE\"),\n \t\t\t flag);\n }\n \n-const char *git_committer_info(int flag)\n+const char *git_author_info(int flag)\n+{\n+\treturn git_author_info_with_date(flag, NULL);\n+}\n+\n+const char *git_committer_info_with_date(int flag, const char *date_str)\n {\n \tif (getenv(\"GIT_COMMITTER_NAME\"))\n \t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n \tif (getenv(\"GIT_COMMITTER_EMAIL\"))\n \t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \treturn fmt_ident(getenv(\"GIT_COMMITTER_NAME\"),\n \t\t\t getenv(\"GIT_COMMITTER_EMAIL\"),\n-\t\t\t getenv(\"GIT_COMMITTER_DATE\"),\n+\t\t\t date_str && *date_str ? date_str :\n+\t\t\t\t\tgetenv(\"GIT_COMMITTER_DATE\"),\n \t\t\t flag);\n }\n \n+const char *git_committer_info(int flag)\n+{\n+\treturn git_committer_info_with_date(flag, NULL);\n+}\n+\n static int ident_is_sufficient(int user_ident_explicitly_given)\n {\n #ifndef WINDOWS\n-- \n2.17.0\n\n"},{"id":"344977","messageId":"87vacoeovh.fsf@evledraar.gmail.com","threadId":"48280","inReplyTo":"20180418102236.7183-1-phillip.wood@talktalk.net","subject":"Re: [RFC PATCH] ident: don't cache default date","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-18T11:27:14Z","receivedAt":"2018-04-18T11:27:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 18 2018, Phillip Wood wrote:\n\n> From: Phillip Wood \n>\n> Now that the sequencer commits without forking when the commit message\n> isn't edited all the commits that are picked have the same committer\n> date. If a commit is reworded it's committer date will be a later time\n\ns/it's/its/\n\n> as it is created by running an separate instance of 'git commit'. If\n> the reworded commit is follow by further picks, those later commits\n> will have an earlier committer date than the reworded one. This is\n> caused by git caching the default date used when GIT_COMMITTER_DATE is\n> not set. Fix this by not caching the date.\n>\n> Users expect commits to have the same author and committer dates when\n> the don't explicitly set them. As the date is now updated each time\n> git_author_info() or git_committer_info() is run it is possible to end\n> up with different author and committer dates. Fix this for\n> 'commit-tree', 'notes' and 'merge' by using a single date in\n> commit_tree_extended() and passing it explicitly to the new functions\n> git_author_info_with_date() and git_committer_info_with_date() when\n> neither the author date nor the committer date are explicitly\n> set. 'commit' always passes the author date to commit_tree_extended()\n> and relied on the date caching to have the same committer and author\n> dates when neither was specified. Fix this by setting\n> GIT_COMMITTER_DATE to be the same as the author date passed to\n> commit_tree_extended().\n>\n> Signed-off-by: Phillip Wood \n> Reported-by: Johannes Sixt \n> ---\n>\n> I'm slightly nervous that setting GIT_COMMITTER_DATE in\n> builtin/commit.c will break someone's hook script. Maybe it would be\n> better to add a committer parameter to commit_tree() and\n> commit_tree_extended().\n>\n> This needs some tests. I think we could test that the sequencer gives\n> each commit a new date with 'git rebase -i --exec=\"sleep 2\"\n> --force-rebase @~2' and test the committer dates of the rebased\n> commits. I'm not sure if test -gt can cope with numbers that big\n> though, maybe we'll have to be content with test !=. For 'git commit'\n> I think doing GIT_EDITOR='sleep 2; echo message >\"$1\"' and checking\n> the committer date and author date will work as the author date is set\n> before the user edits the commit message. I'm not sure how to test\n> callers of commit_tree() though (that's commit-tree, notes and merge)\n> as I've not been able to come up with anything that will guarantee the\n> author and committer dates are different if the code in\n> commit_tree_extended() that ensures they are the same gets broken.\n\nThe behavior you're describing where we end up with later commits in the\nrebase with earlier CommitDate's definitely sounds like a minor\nregression, and yeah we should have tests for this.\n\nMy mental model of this is that we shouldn't be trying at all to adjust\nthe CommitDate in a sequence to be the same, and just make it be\nwhatever time() returns, except in the case where a date is passed\nexplicitly.\n\nMy cursory reading of this RFC patch is that it's definitely an\nimprovement because we don't end up with things out of order, but is\nthere any reason for why we should be trying to aim to keep this\n\"consistent\" within a single \"git rebase\" command by default, as opposed\nto always just writing the current CommitDate at the time we make the\ncommit, that sounds like the most intuitive thing to me, and I can't see\nany downsides with that.\n\nIt leaks info about how long it took someone to do the rebase, but so\nwhat?\n"},{"id":"344998","messageId":"85ecb584-77a7-f818-14c9-1019873d87f9@talktalk.net","threadId":"48280","inReplyTo":"87vacoeovh.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH] ident: don't cache default date","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-18T17:47:59Z","receivedAt":"2018-04-18T17:48:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar, thanks for your comments\n\nOn 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Apr 18 2018, Phillip Wood wrote:\n> \n>> From: Phillip Wood \n>>\n>> Now that the sequencer commits without forking when the commit message\n>> isn't edited all the commits that are picked have the same committer\n>> date. If a commit is reworded it's committer date will be a later time\n> \n> s/it's/its/\n\nThanks I'll change it\n\n>> as it is created by running an separate instance of 'git commit'. If\n>> the reworded commit is follow by further picks, those later commits\n>> will have an earlier committer date than the reworded one. This is\n>> caused by git caching the default date used when GIT_COMMITTER_DATE is\n>> not set. Fix this by not caching the date.\n>>\n>> Users expect commits to have the same author and committer dates when\n>> the don't explicitly set them. As the date is now updated each time\n>> git_author_info() or git_committer_info() is run it is possible to end\n>> up with different author and committer dates. Fix this for\n>> 'commit-tree', 'notes' and 'merge' by using a single date in\n>> commit_tree_extended() and passing it explicitly to the new functions\n>> git_author_info_with_date() and git_committer_info_with_date() when\n>> neither the author date nor the committer date are explicitly\n>> set. 'commit' always passes the author date to commit_tree_extended()\n>> and relied on the date caching to have the same committer and author\n>> dates when neither was specified. Fix this by setting\n>> GIT_COMMITTER_DATE to be the same as the author date passed to\n>> commit_tree_extended().\n>>\n>> Signed-off-by: Phillip Wood \n>> Reported-by: Johannes Sixt \n>> ---\n>>\n>> I'm slightly nervous that setting GIT_COMMITTER_DATE in\n>> builtin/commit.c will break someone's hook script. Maybe it would be\n>> better to add a committer parameter to commit_tree() and\n>> commit_tree_extended().\n>>\n>> This needs some tests. I think we could test that the sequencer gives\n>> each commit a new date with 'git rebase -i --exec=\"sleep 2\"\n>> --force-rebase @~2' and test the committer dates of the rebased\n>> commits. I'm not sure if test -gt can cope with numbers that big\n>> though, maybe we'll have to be content with test !=. For 'git commit'\n>> I think doing GIT_EDITOR='sleep 2; echo message >\"$1\"' and checking\n>> the committer date and author date will work as the author date is set\n>> before the user edits the commit message. I'm not sure how to test\n>> callers of commit_tree() though (that's commit-tree, notes and merge)\n>> as I've not been able to come up with anything that will guarantee the\n>> author and committer dates are different if the code in\n>> commit_tree_extended() that ensures they are the same gets broken.\n> \n> The behavior you're describing where we end up with later commits in the\n> rebase with earlier CommitDate's definitely sounds like a minor\n> regression, and yeah we should have tests for this.\n> \n> My mental model of this is that we shouldn't be trying at all to adjust\n> the CommitDate in a sequence to be the same, and just make it be\n> whatever time() returns, except in the case where a date is passed\n> explicitly.\n> \n> My cursory reading of this RFC patch is that it's definitely an\n> improvement because we don't end up with things out of order, but is\n> there any reason for why we should be trying to aim to keep this\n> \"consistent\" within a single \"git rebase\" command by default, as opposed\n> to always just writing the current CommitDate at the time we make the\n> commit, that sounds like the most intuitive thing to me, and I can't see\n> any downsides with that.\n\nIt's not trying to keep the date \"consistent\" within a single rebase\ncommand, each commit created by the sequencer gets a date stamp with the\ncurrent time when the commit is created. What it is doing is keeping the\nauthor and committer dates the same for commit/commit-tree/merge/notes\nwhen the user does not specify either of them. While I don't think it\nparticularly matters that they match (so long as the committer date is\nlater than the author date), it is what git does at the moment and\nsomeone maybe relying on the current behavior.\n\nBest Wishes\n\nPhillip\n\n> \n> It leaks info about how long it took someone to do the rebase, but so\n> what?\n> \n\n"},{"id":"345001","messageId":"d4aee85d-956f-ac93-b909-6dfeb5ea8fd7@kdbg.org","threadId":"48280","inReplyTo":"85ecb584-77a7-f818-14c9-1019873d87f9@talktalk.net","subject":"Re: [RFC PATCH] ident: don't cache default date","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-04-18T18:15:04Z","receivedAt":"2018-04-18T18:15:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.04.2018 um 19:47 schrieb Phillip Wood:\n> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:\n>> On Wed, Apr 18 2018, Phillip Wood wrote:\n>>> From: Phillip Wood \n>>> as it is created by running an separate instance of 'git commit'. If\n>>> the reworded commit is follow by further picks, those later commits\n>>> will have an earlier committer date than the reworded one. This is\n>>> caused by git caching the default date used when GIT_COMMITTER_DATE is\n>>> not set. Fix this by not caching the date.\n>>>\n>>> Users expect commits to have the same author and committer dates when\n>>> the don't explicitly set them. As the date is now updated each time\n>>> git_author_info() or git_committer_info() is run it is possible to end\n>>> up with different author and committer dates. Fix this for\n>>> 'commit-tree', 'notes' and 'merge' by using a single date in\n>>> commit_tree_extended() and passing it explicitly to the new functions\n>>> git_author_info_with_date() and git_committer_info_with_date() when\n>>> neither the author date nor the committer date are explicitly\n>>> set. 'commit' always passes the author date to commit_tree_extended()\n>>> and relied on the date caching to have the same committer and author\n>>> dates when neither was specified. Fix this by setting\n>>> GIT_COMMITTER_DATE to be the same as the author date passed to\n>>> commit_tree_extended().\n>>>\n>>> Signed-off-by: Phillip Wood \n>>> Reported-by: Johannes Sixt \n>>> ---\n>>>\n>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in\n>>> builtin/commit.c will break someone's hook script. Maybe it would be\n>>> better to add a committer parameter to commit_tree() and\n>>> commit_tree_extended().\n\nWhile I like the basic theme of your patch, I think we should fix this\ncase in a much simpler way, namely, use the infrastructure that was\nintroduced for git-am.\n\nI've shamelessly lifted the commit message from your patch.\n\n---- 8< ----\nSubject: [PATCH] sequencer: reset the committer date before commits\n\nNow that the sequencer commits without forking when the commit message\nisn't edited all the commits that are picked have the same committer\ndate. If a commit is reworded it's committer date will be a later time\nas it is created by running an separate instance of 'git commit'. If\nthe reworded commit is follow by further picks, those later commits\nwill have an earlier committer date than the reworded one. This is\ncaused by git caching the default date used when GIT_COMMITTER_DATE is\nnot set. Reset the cached date before a commit is generated\nin-process.\n\nSigned-off-by: Johannes Sixt \n---\n sequencer.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f9d1001dee..f0bac903a0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char *author,\n \t\tgoto out;\n \t}\n \n+\treset_ident_date();\n+\n \tif (commit_tree_extended(msg->buf, msg->len, &tree, parents,\n \t\t\t\t oid, author, opts->gpg_sign, extra)) {\n \t\tres = error(_(\"failed to write commit object\"));\n-- \n2.17.0.69.g0c1d01d9b6\n"},{"id":"345018","messageId":"xmqqvacogqs4.fsf@gitster-ct.c.googlers.com","threadId":"48280","inReplyTo":"d4aee85d-956f-ac93-b909-6dfeb5ea8fd7@kdbg.org","subject":"Re: [RFC PATCH] ident: don't cache default date","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-18T21:15:23Z","receivedAt":"2018-04-18T21:15:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> While I like the basic theme of your patch, I think we should fix this\n> case in a much simpler way, namely, use the infrastructure that was\n> introduced for git-am.\n\nYup. reset_ident_date() was introduced by 4d9c7e6f (\"am: reset\ncached ident date for each patch\", 2016-08-01) and the commit\nexplains very well why it is a good idea to have both the caching\nand also the strategic resetting it introduces.\n\nThanks, all.\n\n> I've shamelessly lifted the commit message from your patch.\n>\n> ---- 8< ----\n> Subject: [PATCH] sequencer: reset the committer date before commits\n>\n> Now that the sequencer commits without forking when the commit message\n> isn't edited all the commits that are picked have the same committer\n> date. If a commit is reworded it's committer date will be a later time\n> as it is created by running an separate instance of 'git commit'. If\n> the reworded commit is follow by further picks, those later commits\n> will have an earlier committer date than the reworded one. This is\n> caused by git caching the default date used when GIT_COMMITTER_DATE is\n> not set. Reset the cached date before a commit is generated\n> in-process.\n>\n> Signed-off-by: Johannes Sixt \n> ---\n> sequencer.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index f9d1001dee..f0bac903a0 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char *author,\n> \t\tgoto out;\n> \t}\n> \n> +\treset_ident_date();\n> +\n> \tif (commit_tree_extended(msg->buf, msg->len, &tree, parents,\n> \t\t\t\t oid, author, opts->gpg_sign, extra)) {\n> \t\tres = error(_(\"failed to write commit object\"));\n"},{"id":"345081","messageId":"65ca9629-46b1-2963-9b6b-99f12a18689d@talktalk.net","threadId":"48280","inReplyTo":"d4aee85d-956f-ac93-b909-6dfeb5ea8fd7@kdbg.org","subject":"Re: [RFC PATCH] ident: don't cache default date","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-19T09:15:15Z","receivedAt":"2018-04-19T09:15:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 18/04/18 19:15, Johannes Sixt wrote:\n> Am 18.04.2018 um 19:47 schrieb Phillip Wood:\n>> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:\n>>> On Wed, Apr 18 2018, Phillip Wood wrote:\n>>>> From: Phillip Wood \n>>>> as it is created by running an separate instance of 'git commit'. If\n>>>> the reworded commit is follow by further picks, those later commits\n>>>> will have an earlier committer date than the reworded one. This is\n>>>> caused by git caching the default date used when GIT_COMMITTER_DATE is\n>>>> not set. Fix this by not caching the date.\n>>>>\n>>>> Users expect commits to have the same author and committer dates when\n>>>> the don't explicitly set them. As the date is now updated each time\n>>>> git_author_info() or git_committer_info() is run it is possible to end\n>>>> up with different author and committer dates. Fix this for\n>>>> 'commit-tree', 'notes' and 'merge' by using a single date in\n>>>> commit_tree_extended() and passing it explicitly to the new functions\n>>>> git_author_info_with_date() and git_committer_info_with_date() when\n>>>> neither the author date nor the committer date are explicitly\n>>>> set. 'commit' always passes the author date to commit_tree_extended()\n>>>> and relied on the date caching to have the same committer and author\n>>>> dates when neither was specified. Fix this by setting\n>>>> GIT_COMMITTER_DATE to be the same as the author date passed to\n>>>> commit_tree_extended().\n>>>>\n>>>> Signed-off-by: Phillip Wood \n>>>> Reported-by: Johannes Sixt \n>>>> ---\n>>>>\n>>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in\n>>>> builtin/commit.c will break someone's hook script. Maybe it would be\n>>>> better to add a committer parameter to commit_tree() and\n>>>> commit_tree_extended().\n> \n> While I like the basic theme of your patch, I think we should fix this\n> case in a much simpler way, namely, use the infrastructure that was\n> introduced for git-am.\n> \n> I've shamelessly lifted the commit message from your patch.\n\nThanks, that is a better way (I'm annoyed with myself for not having\nnoticed reset_ident_date() when I edited the function above it)\n\nBest Wishes\n\nPhillip\n\n> ---- 8< ----\n> Subject: [PATCH] sequencer: reset the committer date before commits\n> \n> Now that the sequencer commits without forking when the commit message\n> isn't edited all the commits that are picked have the same committer\n> date. If a commit is reworded it's committer date will be a later time\n> as it is created by running an separate instance of 'git commit'. If\n> the reworded commit is follow by further picks, those later commits\n> will have an earlier committer date than the reworded one. This is\n> caused by git caching the default date used when GIT_COMMITTER_DATE is\n> not set. Reset the cached date before a commit is generated\n> in-process.\n> \n> Signed-off-by: Johannes Sixt \n> ---\n> sequencer.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index f9d1001dee..f0bac903a0 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char *author,\n> \t\tgoto out;\n> \t}\n> \n> +\treset_ident_date();\n> +\n> \tif (commit_tree_extended(msg->buf, msg->len, &tree, parents,\n> \t\t\t\t oid, author, opts->gpg_sign, extra)) {\n> \t\tres = error(_(\"failed to write commit object\"));\n> \n\n"},{"id":"345082","messageId":"041aabb4-4e61-30a1-2ab0-3ae1ddd47c38@talktalk.net","threadId":"48280","inReplyTo":"ba17074b-865e-32ba-908d-0d94bedc50ad@talktalk.net","subject":"Re: Bug: rebase -i creates committer time inversions on 'reword'","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-19T09:17:40Z","receivedAt":"2018-04-19T09:17:45Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/04/18 10:48, Phillip Wood wrote:\n> On 14/04/18 14:11, Johannes Schindelin wrote:\n>> Hi,\n>>\n>> On Sat, 14 Apr 2018, Phillip Wood wrote:\n>>\n>> FWIW I agree with Hannes' patch.\n>>\n>>> I think 'git am' probably gives all patches the same commit time as well\n>>> if the commit date is cached though it wont suffer from the time-travel\n>>> problem.\n>>\n>> I thought that `git am` was the subject of such a complaint recently, but\n>> I thought that had been resolved? Apparently I misremember...\n> \n> I had a quick look and couldn't see anything about that, it looks to me\n> like it just calls commit_tree() and only does anything to change the\n> default commit date if '--committer-date-is-author-date' was given.\n\nAh you were right, I just didn't look far enough back in the history\n.(it's a shame it calls reset_ident_date() in a different function to\nthe one that creates the commit otherwise I would probably have noticed\nit when I wrote the original patches)\n\nBest Wishes\n\nPhillip\n\n> \n> Best Wishes\n> \n> Phillip\n>> Ciao,\n>> Dscho\n>>\n> \n\n"},{"id":"345197","messageId":"nycvar.QRO.7.76.6.1804201010120.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48280","inReplyTo":"65ca9629-46b1-2963-9b6b-99f12a18689d@talktalk.net","subject":"Re: [RFC PATCH] ident: don't cache default date","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-20T08:11:45Z","receivedAt":"2018-04-20T08:12:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Thu, 19 Apr 2018, Phillip Wood wrote:\n\n> On 18/04/18 19:15, Johannes Sixt wrote:\n> > Am 18.04.2018 um 19:47 schrieb Phillip Wood:\n> >> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:\n> >>> On Wed, Apr 18 2018, Phillip Wood wrote:\n> >>>> From: Phillip Wood \n> >>>> as it is created by running an separate instance of 'git commit'. If\n> >>>> the reworded commit is follow by further picks, those later commits\n> >>>> will have an earlier committer date than the reworded one. This is\n> >>>> caused by git caching the default date used when GIT_COMMITTER_DATE is\n> >>>> not set. Fix this by not caching the date.\n> >>>>\n> >>>> Users expect commits to have the same author and committer dates when\n> >>>> the don't explicitly set them. As the date is now updated each time\n> >>>> git_author_info() or git_committer_info() is run it is possible to end\n> >>>> up with different author and committer dates. Fix this for\n> >>>> 'commit-tree', 'notes' and 'merge' by using a single date in\n> >>>> commit_tree_extended() and passing it explicitly to the new functions\n> >>>> git_author_info_with_date() and git_committer_info_with_date() when\n> >>>> neither the author date nor the committer date are explicitly\n> >>>> set. 'commit' always passes the author date to commit_tree_extended()\n> >>>> and relied on the date caching to have the same committer and author\n> >>>> dates when neither was specified. Fix this by setting\n> >>>> GIT_COMMITTER_DATE to be the same as the author date passed to\n> >>>> commit_tree_extended().\n> >>>>\n> >>>> Signed-off-by: Phillip Wood \n> >>>> Reported-by: Johannes Sixt \n> >>>> ---\n> >>>>\n> >>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in\n> >>>> builtin/commit.c will break someone's hook script. Maybe it would be\n> >>>> better to add a committer parameter to commit_tree() and\n> >>>> commit_tree_extended().\n> > \n> > While I like the basic theme of your patch, I think we should fix this\n> > case in a much simpler way, namely, use the infrastructure that was\n> > introduced for git-am.\n> > \n> > I've shamelessly lifted the commit message from your patch.\n> \n> Thanks, that is a better way (I'm annoyed with myself for not having\n> noticed reset_ident_date() when I edited the function above it)\n\nDon't be too annoyed. I did remember that The Linus had complained about\nsomething similar and assumed that it had been fixed in the meantime, but\nI failed to find it within 30 minutes where I tried to dig through\npublic-inbox and pu.\n\nThanks Hannes for remembering, and for coming up with the final form of\nthe patch!\n\nCiao,\nDscho"},{"id":"345203","messageId":"8ec9129f-5a27-1ece-91d8-0883dacadce9@talktalk.net","threadId":"48280","inReplyTo":"nycvar.QRO.7.76.6.1804201010120.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [RFC PATCH] ident: don't cache default date","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-04-20T09:41:05Z","receivedAt":"2018-04-20T09:41:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/04/18 09:11, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Thu, 19 Apr 2018, Phillip Wood wrote:\n> \n>> On 18/04/18 19:15, Johannes Sixt wrote:\n>>> Am 18.04.2018 um 19:47 schrieb Phillip Wood:\n>>>> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:\n>>>>> On Wed, Apr 18 2018, Phillip Wood wrote:\n>>>>>> From: Phillip Wood \n>>>>>> as it is created by running an separate instance of 'git commit'. If\n>>>>>> the reworded commit is follow by further picks, those later commits\n>>>>>> will have an earlier committer date than the reworded one. This is\n>>>>>> caused by git caching the default date used when GIT_COMMITTER_DATE is\n>>>>>> not set. Fix this by not caching the date.\n>>>>>>\n>>>>>> Users expect commits to have the same author and committer dates when\n>>>>>> the don't explicitly set them. As the date is now updated each time\n>>>>>> git_author_info() or git_committer_info() is run it is possible to end\n>>>>>> up with different author and committer dates. Fix this for\n>>>>>> 'commit-tree', 'notes' and 'merge' by using a single date in\n>>>>>> commit_tree_extended() and passing it explicitly to the new functions\n>>>>>> git_author_info_with_date() and git_committer_info_with_date() when\n>>>>>> neither the author date nor the committer date are explicitly\n>>>>>> set. 'commit' always passes the author date to commit_tree_extended()\n>>>>>> and relied on the date caching to have the same committer and author\n>>>>>> dates when neither was specified. Fix this by setting\n>>>>>> GIT_COMMITTER_DATE to be the same as the author date passed to\n>>>>>> commit_tree_extended().\n>>>>>>\n>>>>>> Signed-off-by: Phillip Wood \n>>>>>> Reported-by: Johannes Sixt \n>>>>>> ---\n>>>>>>\n>>>>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in\n>>>>>> builtin/commit.c will break someone's hook script. Maybe it would be\n>>>>>> better to add a committer parameter to commit_tree() and\n>>>>>> commit_tree_extended().\n>>>\n>>> While I like the basic theme of your patch, I think we should fix this\n>>> case in a much simpler way, namely, use the infrastructure that was\n>>> introduced for git-am.\n>>>\n>>> I've shamelessly lifted the commit message from your patch.\n>>\n>> Thanks, that is a better way (I'm annoyed with myself for not having\n>> noticed reset_ident_date() when I edited the function above it)\n> \n> Don't be too annoyed. I did remember that The Linus had complained about\n> something similar and assumed that it had been fixed in the meantime, but\n> I failed to find it within 30 minutes where I tried to dig through\n> public-inbox and pu.\n\nThanks, that makes we feel better about it (thanks for taking the time\nto try and find the original mail as well)\n\nPhillip\n\n> Thanks Hannes for remembering, and for coming up with the final form of\n> the patch!\n> \n> Ciao,\n> Dscho\n> \n\n"}]}