git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: Johannes Sixt <j6t@kdbg.org>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: [RFC PATCH] ident: don't cache default date
Date: Wed, 18 Apr 2018 11:22:36 +0100	[thread overview]
Message-ID: <20180418102236.7183-1-phillip.wood@talktalk.net> (raw)
In-Reply-To: <5f5d5b88-b3ac-ed4f-ee24-6ce2cba2bd55@kdbg.org>

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


  parent reply	other threads:[~2018-04-18 10:23 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 ` Phillip Wood [this message]
2018-04-18 11:27   ` [RFC PATCH] ident: don't cache default date Ævar Arnfjörð Bjarmason
2018-04-18 17:47     ` Phillip Wood
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=20180418102236.7183-1-phillip.wood@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --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).