Git silently ignores --date when data is not in the correct format

Git silently ignores --date when data is not in the correct format

[Sergio]

Hi,

on 1.7.3.3, I have noticed that git --commit silently ignores the 
--date=<date> switch if <date> is not in the current format.

for instance

git --commit --amend --date="10.11.2010" creates a commit with the current
date and time, because the --date argument misses the time.

possibly, it would be better to stop with an error message.

[PATCH/RFC] ident: die on bogus date format

[Jeff King]

On Mon, Dec 13, 2010 at 03:20:07PM +0000, Sergio wrote:

> on 1.7.3.3, I have noticed that git --commit silently ignores the 
> --date=<date> switch if <date> is not in the current format.
> 
> for instance
> 
> git --commit --amend --date="10.11.2010" creates a commit with the current
> date and time, because the --date argument misses the time.
> 
> possibly, it would be better to stop with an error message.

Yeah, we should definitely be flagging the error. This patch fixes it,
but I'm not sure if it is optimal (see below).

-- >8 --
Subject: [PATCH/RFC] ident: die on bogus date format

If the user gives "git commit --date=foobar", we silently
ignore the --date flag. We should note the error.

This patch puts the fix at the lowest level of fmt_ident,
which means it also handles GIT_AUTHOR_DATE=foobar, as well.

There are two down-sides to this approach:

  1. Technically this breaks somebody doing something like
     "git commit --date=now", which happened to work because
     bogus data is the same as "now". Though we do
     explicitly handle the empty string, so anybody passing
     an empty variable through the environment will still
     work.

     If the error is too much, perhaps it can be downgraded
     to a warning?

  2. The error checking happens _after_ the commit message
     is written, which can be annoying to the user. We can
     put explicit checks closer to the beginning of
     git-commit, but that feels a little hack-ish; suddenly
     git-commit has to care about how fmt_ident works. Maybe
     we could simply call fmt_ident earlier?

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c           |    6 ++++--
 t/t7501-commit.sh |    4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ident.c b/ident.c
index 9e24388..1c4adb0 100644
--- a/ident.c
+++ b/ident.c
@@ -217,8 +217,10 @@ const char *fmt_ident(const char *name, const char *email,
 	}
 
 	strcpy(date, git_default_date);
-	if (!name_addr_only && date_str)
-		parse_date(date_str, date, sizeof(date));
+	if (!name_addr_only && date_str && date_str[0]) {
+		if (parse_date(date_str, date, sizeof(date)) < 0)
+			die("invalid date format: %s", date_str);
+	}
 
 	i = copy(buffer, sizeof(buffer), 0, name);
 	i = add_raw(buffer, sizeof(buffer), i, " <");
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 8297cb4..8980738 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -230,6 +230,10 @@ test_expect_success 'amend commit to fix date' '
 
 '
 
+test_expect_success 'commit complains about bogus date' '
+	test_must_fail git commit --amend --date=10.11.2010
+'
+
 test_expect_success 'sign off (1)' '
 
 	echo 1 >positive &&
-- 
1.7.3.3.784.gccc31.dirty

Re: [PATCH/RFC] ident: die on bogus date format

[Sergio Callegari]

Thanks for looking (and coding) into it!

For my usage case, a warning would be ok too. If the commit generates a warning,
one can notice the warning and --amend the commit.

One can perhaps have the warning now and maybe an
error from version 1.8, so: (i) we are sure that the different behavior does not 
break anything and
(ii) there is time to move the fmt_ident up in the code so that it is invoked 
earlier
(to avoid the burden of writing the commit message and then seeing the commit 
aborted).

By the way, note that I got into the issue by trying to use --date forgetting 
the time.
Don't know if it could make sense to accept the date with some default time in 
this case.

Regards

Sergio

Re: [PATCH/RFC] ident: die on bogus date format

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> There are two down-sides to this approach:
>
>   1. Technically this breaks somebody doing something like
>      "git commit --date=now", which happened to work because
>      bogus data is the same as "now". Though we do
>      explicitly handle the empty string, so anybody passing
>      an empty variable through the environment will still
>      work.

These days I think the bogodate parser knows what "now" is, but you can
change the example to use "ahora" instead of "now" and your argument does
not change.  But if you force user to change something in order to work
with a new version of git, it is a regression, no matter how small that
change is.

Having said that, I don't think --date=ahora is something we need to worry
about within the context of "git commit", as the regression feels purely
technical (the author-date defaults to the current time anyway, so there
is no reason to give --date=ahora to the command, even though giving an
explicit date via the flag may have some uses).  On the other hand, as
fmt_ident() is fairly low-level, there might be other callers to which it
made sense to give "now" to them, and we wouldn't know without looking.

>      If the error is too much, perhaps it can be downgraded
>      to a warning?

I think dying is actually Ok for this caller, as we already pass
IDENT_ERROR_ON_NO_NAME to fmt_ident() expecting it to die for us upon a
bad input.  Even though I suspect that we do not need to be conditional on
this (the only reason ON_NO_NAME exists is because reflogs may record your
name when you switch branches, and if you are only sightseeing it doesn't
matter if your name is "johndoe@(null)"), using IDENT_ERROR_ON_NO_DATE may
be safer perhaps?

>   2. The error checking happens _after_ the commit message
>      is written, which can be annoying to the user. We can
>      put explicit checks closer to the beginning of
>      git-commit, but that feels a little hack-ish; suddenly
>      git-commit has to care about how fmt_ident works. Maybe
>      we could simply call fmt_ident earlier?

After determine_author_info() returns to prepare_to_commit(), we have a
call to git_committer_info() only to discard the outcome from.  I think
this call was an earlier attempt to catch "You do not exist" and related
low-level errors, and the codepath feels the right place to catch more
recent errors like the one under discussion.  Instead of passing 0, how
about passing IDENT_ERROR_ON_NO_NAME and IDENT_ERROR_ON_NO_DATE there,
store and return its output from the prepare_to_commit(), and then give
that string to commit_tree() later in cmd_commit().  We can do this by
adding a new parameter (strbuf) to prepare_to_commit(), I think.

Re: [PATCH/RFC] ident: die on bogus date format

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> After determine_author_info() returns to prepare_to_commit(), we have a
> call to git_committer_info() only to discard the outcome from....
> ...  We can do this by
> adding a new parameter (strbuf) to prepare_to_commit(), I think.

Heh, I was an idiot, and mixed up author and committer in the above.
But I think it is worth trying to avoid asking the user to edit when
we know that a bad author ident will result in an error later.

How about doing it like this?

-- >8 --
Subject: commit: die before asking to edit the log message

When determine_author_info() returns to the calling prepare_to_commit(),
we already know the pieces of information necessary to determine what
author ident will be used in the final message, but deferred making a call
to fmt_ident() before the final commit_tree().  Most importantly, we would
open the editor to ask the user to compose the log message before it.

As one important side effect of fmt_ident() is to error out when the given
information is malformed, this resulted in us spawning the editor first
and then refusing to commit due to error, even though we had enough
information to detect the error before starting the editor, which was
annoying.

Move the fmt_ident() call to the end of determine_author_info() where we
have final determination of author info to rectify this.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c |   54 ++++++++++++++++++++++++++++++++----------------------
 1 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 66fdd22..3bcb4b7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -69,7 +69,6 @@ static enum {
 static const char *logfile, *force_author;
 static const char *template_file;
 static char *edit_message, *use_message;
-static char *author_name, *author_email, *author_date;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
@@ -459,7 +458,7 @@ static int is_a_merge(const unsigned char *sha1)
 
 static const char sign_off_header[] = "Signed-off-by: ";
 
-static void determine_author_info(void)
+static void determine_author_info(struct strbuf *author_ident)
 {
 	char *name, *email, *date;
 
@@ -503,10 +502,8 @@ static void determine_author_info(void)
 
 	if (force_date)
 		date = force_date;
-
-	author_name = name;
-	author_email = email;
-	author_date = date;
+	strbuf_addstr(author_ident, fmt_ident(name, email, date,
+					      IDENT_ERROR_ON_NO_NAME));
 }
 
 static int ends_rfc2822_footer(struct strbuf *sb)
@@ -550,10 +547,21 @@ static int ends_rfc2822_footer(struct strbuf *sb)
 	return 1;
 }
 
+static char *cut_ident_timestamp_part(char *string)
+{
+	char *ket = strrchr(string, '>');
+	if (!ket || ket[1] != ' ')
+		die("Malformed ident string: '%s'", string);
+	*++ket = '\0';
+	return ket;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
-			     struct wt_status *s)
+			     struct wt_status *s,
+			     struct strbuf *author_ident)
 {
 	struct stat statbuf;
+	struct strbuf committer_ident = STRBUF_INIT;
 	int commitable, saved_color_setting;
 	struct strbuf sb = STRBUF_INIT;
 	char *buffer;
@@ -637,14 +645,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 	strbuf_release(&sb);
 
-	determine_author_info();
+	/* This checks and barfs if author is badly specified */
+	determine_author_info(author_ident);
 
 	/* This checks if committer ident is explicitly given */
-	git_committer_info(0);
+	strbuf_addstr(&committer_ident, git_committer_info(0));
 	if (use_editor && include_status) {
-		char *author_ident;
-		const char *committer_ident;
-
+		char *ai_tmp, *ci_tmp;
 		if (in_merge)
 			fprintf(fp,
 				"#\n"
@@ -672,23 +679,21 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (only_include_assumed)
 			fprintf(fp, "# %s\n", only_include_assumed);
 
-		author_ident = xstrdup(fmt_name(author_name, author_email));
-		committer_ident = fmt_name(getenv("GIT_COMMITTER_NAME"),
-					   getenv("GIT_COMMITTER_EMAIL"));
-		if (strcmp(author_ident, committer_ident))
+		ai_tmp = cut_ident_timestamp_part(author_ident->buf);
+		ci_tmp = cut_ident_timestamp_part(committer_ident.buf);
+		if (strcmp(author_ident->buf, committer_ident.buf))
 			fprintf(fp,
 				"%s"
 				"# Author:    %s\n",
 				ident_shown++ ? "" : "#\n",
-				author_ident);
-		free(author_ident);
+				author_ident->buf);
 
 		if (!user_ident_sufficiently_given())
 			fprintf(fp,
 				"%s"
 				"# Committer: %s\n",
 				ident_shown++ ? "" : "#\n",
-				committer_ident);
+				committer_ident.buf);
 
 		if (ident_shown)
 			fprintf(fp, "#\n");
@@ -697,6 +702,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		s->use_color = 0;
 		commitable = run_status(fp, index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
+
+		*ai_tmp = ' ';
+		*ci_tmp = ' ';
 	} else {
 		unsigned char sha1[20];
 		const char *parent = "HEAD";
@@ -712,6 +720,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		else
 			commitable = index_differs_from(parent, 0);
 	}
+	strbuf_release(&committer_ident);
 
 	fclose(fp);
 
@@ -1246,6 +1255,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf author_ident = STRBUF_INIT;
 	const char *index_file, *reflog_msg;
 	char *nl, *p;
 	unsigned char commit_sha1[20];
@@ -1273,7 +1283,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	/* Set up everything for writing the commit object.  This includes
 	   running hooks, writing the trees, and interacting with the user.  */
-	if (!prepare_to_commit(index_file, prefix, &s)) {
+	if (!prepare_to_commit(index_file, prefix, &s, &author_ident)) {
 		rollback_index_files();
 		return 1;
 	}
@@ -1352,11 +1362,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 
 	if (commit_tree(sb.buf, active_cache_tree->sha1, parents, commit_sha1,
-			fmt_ident(author_name, author_email, author_date,
-				IDENT_ERROR_ON_NO_NAME))) {
+			author_ident.buf)) {
 		rollback_index_files();
 		die("failed to write commit object");
 	}
+	strbuf_release(&author_ident);
 
 	ref_lock = lock_any_ref_for_update("HEAD",
 					   initial_commit ? NULL : head_sha1,