git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Phillip Wood" <phillip.wood@dunelm.org.uk>
Cc: Johannes Sixt <j6t@kdbg.org>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [RFC PATCH] ident: don't cache default date
Date: Wed, 18 Apr 2018 18:47:59 +0100	[thread overview]
Message-ID: <85ecb584-77a7-f818-14c9-1019873d87f9@talktalk.net> (raw)
In-Reply-To: <87vacoeovh.fsf@evledraar.gmail.com>

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?
> 


  reply	other threads:[~2018-04-18 17:48 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-13 16:52 Bug: rebase -i creates committer time inversions on 'reword' Johannes Sixt
2018-04-14 11:15 ` Phillip Wood
2018-04-14 13:11   ` Johannes Schindelin
2018-04-16  9:48     ` Phillip Wood
2018-04-19  9:17       ` Phillip Wood
2018-04-15 21:35 ` Junio C Hamano
2018-04-16  5:56   ` Johannes Sixt
2018-04-17  1:37     ` Junio C Hamano
2018-04-18 10:19     ` Phillip Wood
2018-04-18 10:22 ` [RFC PATCH] ident: don't cache default date Phillip Wood
2018-04-18 11:27   ` Ævar Arnfjörð Bjarmason
2018-04-18 17:47     ` Phillip Wood [this message]
2018-04-18 18:15       ` Johannes Sixt
2018-04-18 21:15         ` Junio C Hamano
2018-04-19  9:15         ` Phillip Wood
2018-04-20  8:11           ` Johannes Schindelin
2018-04-20  9:41             ` Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=85ecb584-77a7-f818-14c9-1019873d87f9@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).