{"thread":{"id":"26039","subject":"Git silently ignores --date when data is not in the correct format","startedAt":"2010-12-13T15:20:07Z","lastAt":"2010-12-21T01:00:36Z","messageCount":5,"participants":["Sergio","Jeff King","Sergio Callegari","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"157962","messageId":"loom.20101213T161633-993@post.gmane.org","threadId":"26039","inReplyTo":null,"subject":"Git silently ignores --date when data is not in the correct format","fromName":"Sergio","fromEmail":"sergio.callegari@gmail.com","sentAt":"2010-12-13T15:20:07Z","receivedAt":"2010-12-13T15:20:07Z","isPatch":false,"sender":{"key":"sergio.callegari@gmail.com","avatar":"https://gravatar.com/avatar/c98f41317e0422c1e630385de0e3970227b8e5ad15f35ba8586066467cc833bc?d=mp&s=160"},"body":"Hi,\n\non 1.7.3.3, I have noticed that git --commit silently ignores the \n--date=<date> switch if <date> is not in the current format.\n\nfor instance\n\ngit --commit --amend --date=\"10.11.2010\" creates a commit with the current\ndate and time, because the --date argument misses the time.\n\npossibly, it would be better to stop with an error message.\n"},{"id":"157971","messageId":"20101213170225.GA16033@sigill.intra.peff.net","threadId":"26039","inReplyTo":"loom.20101213T161633-993@post.gmane.org","subject":"[PATCH/RFC] ident: die on bogus date format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-12-13T17:02:25Z","receivedAt":"2010-12-13T17:02:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 13, 2010 at 03:20:07PM +0000, Sergio wrote:\n\n> on 1.7.3.3, I have noticed that git --commit silently ignores the \n> --date=<date> switch if <date> is not in the current format.\n> \n> for instance\n> \n> git --commit --amend --date=\"10.11.2010\" creates a commit with the current\n> date and time, because the --date argument misses the time.\n> \n> possibly, it would be better to stop with an error message.\n\nYeah, we should definitely be flagging the error. This patch fixes it,\nbut I'm not sure if it is optimal (see below).\n\n-- >8 --\nSubject: [PATCH/RFC] ident: die on bogus date format\n\nIf the user gives \"git commit --date=foobar\", we silently\nignore the --date flag. We should note the error.\n\nThis patch puts the fix at the lowest level of fmt_ident,\nwhich means it also handles GIT_AUTHOR_DATE=foobar, as well.\n\nThere are two down-sides to this approach:\n\n  1. Technically this breaks somebody doing something like\n     \"git commit --date=now\", which happened to work because\n     bogus data is the same as \"now\". Though we do\n     explicitly handle the empty string, so anybody passing\n     an empty variable through the environment will still\n     work.\n\n     If the error is too much, perhaps it can be downgraded\n     to a warning?\n\n  2. The error checking happens _after_ the commit message\n     is written, which can be annoying to the user. We can\n     put explicit checks closer to the beginning of\n     git-commit, but that feels a little hack-ish; suddenly\n     git-commit has to care about how fmt_ident works. Maybe\n     we could simply call fmt_ident earlier?\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ident.c           |    6 ++++--\n t/t7501-commit.sh |    4 ++++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex 9e24388..1c4adb0 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -217,8 +217,10 @@ const char *fmt_ident(const char *name, const char *email,\n \t}\n \n \tstrcpy(date, git_default_date);\n-\tif (!name_addr_only && date_str)\n-\t\tparse_date(date_str, date, sizeof(date));\n+\tif (!name_addr_only && date_str && date_str[0]) {\n+\t\tif (parse_date(date_str, date, sizeof(date)) < 0)\n+\t\t\tdie(\"invalid date format: %s\", date_str);\n+\t}\n \n \ti = copy(buffer, sizeof(buffer), 0, name);\n \ti = add_raw(buffer, sizeof(buffer), i, \" <\");\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 8297cb4..8980738 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -230,6 +230,10 @@ test_expect_success 'amend commit to fix date' '\n \n '\n \n+test_expect_success 'commit complains about bogus date' '\n+\ttest_must_fail git commit --amend --date=10.11.2010\n+'\n+\n test_expect_success 'sign off (1)' '\n \n \techo 1 >positive &&\n-- \n1.7.3.3.784.gccc31.dirty\n"},{"id":"157975","messageId":"4D065F4D.1090807@gmail.com","threadId":"26039","inReplyTo":"20101213170225.GA16033@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] ident: die on bogus date format","fromName":"Sergio Callegari","fromEmail":"sergio.callegari@gmail.com","sentAt":"2010-12-13T18:00:45Z","receivedAt":"2010-12-13T18:00:45Z","isPatch":true,"sender":{"key":"sergio.callegari@gmail.com","avatar":"https://gravatar.com/avatar/c98f41317e0422c1e630385de0e3970227b8e5ad15f35ba8586066467cc833bc?d=mp&s=160"},"body":"Thanks for looking (and coding) into it!\n\nFor my usage case, a warning would be ok too. If the commit generates a warning,\none can notice the warning and --amend the commit.\n\nOne can perhaps have the warning now and maybe an\nerror from version 1.8, so: (i) we are sure that the different behavior does not \nbreak anything and\n(ii) there is time to move the fmt_ident up in the code so that it is invoked \nearlier\n(to avoid the burden of writing the commit message and then seeing the commit \naborted).\n\nBy the way, note that I got into the issue by trying to use --date forgetting \nthe time.\nDon't know if it could make sense to accept the date with some default time in \nthis case.\n\nRegards\n\nSergio\n"},{"id":"158195","messageId":"7vei9ipusf.fsf@alter.siamese.dyndns.org","threadId":"26039","inReplyTo":"20101213170225.GA16033@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] ident: die on bogus date format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-15T21:53:36Z","receivedAt":"2010-12-15T21:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There are two down-sides to this approach:\n>\n>   1. Technically this breaks somebody doing something like\n>      \"git commit --date=now\", which happened to work because\n>      bogus data is the same as \"now\". Though we do\n>      explicitly handle the empty string, so anybody passing\n>      an empty variable through the environment will still\n>      work.\n\nThese days I think the bogodate parser knows what \"now\" is, but you can\nchange the example to use \"ahora\" instead of \"now\" and your argument does\nnot change.  But if you force user to change something in order to work\nwith a new version of git, it is a regression, no matter how small that\nchange is.\n\nHaving said that, I don't think --date=ahora is something we need to worry\nabout within the context of \"git commit\", as the regression feels purely\ntechnical (the author-date defaults to the current time anyway, so there\nis no reason to give --date=ahora to the command, even though giving an\nexplicit date via the flag may have some uses).  On the other hand, as\nfmt_ident() is fairly low-level, there might be other callers to which it\nmade sense to give \"now\" to them, and we wouldn't know without looking.\n\n>      If the error is too much, perhaps it can be downgraded\n>      to a warning?\n\nI think dying is actually Ok for this caller, as we already pass\nIDENT_ERROR_ON_NO_NAME to fmt_ident() expecting it to die for us upon a\nbad input.  Even though I suspect that we do not need to be conditional on\nthis (the only reason ON_NO_NAME exists is because reflogs may record your\nname when you switch branches, and if you are only sightseeing it doesn't\nmatter if your name is \"johndoe@(null)\"), using IDENT_ERROR_ON_NO_DATE may\nbe safer perhaps?\n\n>   2. The error checking happens _after_ the commit message\n>      is written, which can be annoying to the user. We can\n>      put explicit checks closer to the beginning of\n>      git-commit, but that feels a little hack-ish; suddenly\n>      git-commit has to care about how fmt_ident works. Maybe\n>      we could simply call fmt_ident earlier?\n\nAfter determine_author_info() returns to prepare_to_commit(), we have a\ncall to git_committer_info() only to discard the outcome from.  I think\nthis call was an earlier attempt to catch \"You do not exist\" and related\nlow-level errors, and the codepath feels the right place to catch more\nrecent errors like the one under discussion.  Instead of passing 0, how\nabout passing IDENT_ERROR_ON_NO_NAME and IDENT_ERROR_ON_NO_DATE there,\nstore and return its output from the prepare_to_commit(), and then give\nthat string to commit_tree() later in cmd_commit().  We can do this by\nadding a new parameter (strbuf) to prepare_to_commit(), I think.\n"},{"id":"158424","messageId":"7v62uoaqiz.fsf@alter.siamese.dyndns.org","threadId":"26039","inReplyTo":"7vei9ipusf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] ident: die on bogus date format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-21T01:00:36Z","receivedAt":"2010-12-21T01:00:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> After determine_author_info() returns to prepare_to_commit(), we have a\n> call to git_committer_info() only to discard the outcome from....\n> ...  We can do this by\n> adding a new parameter (strbuf) to prepare_to_commit(), I think.\n\nHeh, I was an idiot, and mixed up author and committer in the above.\nBut I think it is worth trying to avoid asking the user to edit when\nwe know that a bad author ident will result in an error later.\n\nHow about doing it like this?\n\n-- >8 --\nSubject: commit: die before asking to edit the log message\n\nWhen determine_author_info() returns to the calling prepare_to_commit(),\nwe already know the pieces of information necessary to determine what\nauthor ident will be used in the final message, but deferred making a call\nto fmt_ident() before the final commit_tree().  Most importantly, we would\nopen the editor to ask the user to compose the log message before it.\n\nAs one important side effect of fmt_ident() is to error out when the given\ninformation is malformed, this resulted in us spawning the editor first\nand then refusing to commit due to error, even though we had enough\ninformation to detect the error before starting the editor, which was\nannoying.\n\nMove the fmt_ident() call to the end of determine_author_info() where we\nhave final determination of author info to rectify this.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/commit.c |   54 ++++++++++++++++++++++++++++++++----------------------\n 1 files changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 66fdd22..3bcb4b7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -69,7 +69,6 @@ static enum {\n static const char *logfile, *force_author;\n static const char *template_file;\n static char *edit_message, *use_message;\n-static char *author_name, *author_email, *author_date;\n static int all, edit_flag, also, interactive, only, amend, signoff;\n static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n static int no_post_rewrite, allow_empty_message;\n@@ -459,7 +458,7 @@ static int is_a_merge(const unsigned char *sha1)\n \n static const char sign_off_header[] = \"Signed-off-by: \";\n \n-static void determine_author_info(void)\n+static void determine_author_info(struct strbuf *author_ident)\n {\n \tchar *name, *email, *date;\n \n@@ -503,10 +502,8 @@ static void determine_author_info(void)\n \n \tif (force_date)\n \t\tdate = force_date;\n-\n-\tauthor_name = name;\n-\tauthor_email = email;\n-\tauthor_date = date;\n+\tstrbuf_addstr(author_ident, fmt_ident(name, email, date,\n+\t\t\t\t\t      IDENT_ERROR_ON_NO_NAME));\n }\n \n static int ends_rfc2822_footer(struct strbuf *sb)\n@@ -550,10 +547,21 @@ static int ends_rfc2822_footer(struct strbuf *sb)\n \treturn 1;\n }\n \n+static char *cut_ident_timestamp_part(char *string)\n+{\n+\tchar *ket = strrchr(string, '>');\n+\tif (!ket || ket[1] != ' ')\n+\t\tdie(\"Malformed ident string: '%s'\", string);\n+\t*++ket = '\\0';\n+\treturn ket;\n+}\n+\n static int prepare_to_commit(const char *index_file, const char *prefix,\n-\t\t\t     struct wt_status *s)\n+\t\t\t     struct wt_status *s,\n+\t\t\t     struct strbuf *author_ident)\n {\n \tstruct stat statbuf;\n+\tstruct strbuf committer_ident = STRBUF_INIT;\n \tint commitable, saved_color_setting;\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *buffer;\n@@ -637,14 +645,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \n \tstrbuf_release(&sb);\n \n-\tdetermine_author_info();\n+\t/* This checks and barfs if author is badly specified */\n+\tdetermine_author_info(author_ident);\n \n \t/* This checks if committer ident is explicitly given */\n-\tgit_committer_info(0);\n+\tstrbuf_addstr(&committer_ident, git_committer_info(0));\n \tif (use_editor && include_status) {\n-\t\tchar *author_ident;\n-\t\tconst char *committer_ident;\n-\n+\t\tchar *ai_tmp, *ci_tmp;\n \t\tif (in_merge)\n \t\t\tfprintf(fp,\n \t\t\t\t\"#\\n\"\n@@ -672,23 +679,21 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tif (only_include_assumed)\n \t\t\tfprintf(fp, \"# %s\\n\", only_include_assumed);\n \n-\t\tauthor_ident = xstrdup(fmt_name(author_name, author_email));\n-\t\tcommitter_ident = fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n-\t\t\t\t\t   getenv(\"GIT_COMMITTER_EMAIL\"));\n-\t\tif (strcmp(author_ident, committer_ident))\n+\t\tai_tmp = cut_ident_timestamp_part(author_ident->buf);\n+\t\tci_tmp = cut_ident_timestamp_part(committer_ident.buf);\n+\t\tif (strcmp(author_ident->buf, committer_ident.buf))\n \t\t\tfprintf(fp,\n \t\t\t\t\"%s\"\n \t\t\t\t\"# Author:    %s\\n\",\n \t\t\t\tident_shown++ ? \"\" : \"#\\n\",\n-\t\t\t\tauthor_ident);\n-\t\tfree(author_ident);\n+\t\t\t\tauthor_ident->buf);\n \n \t\tif (!user_ident_sufficiently_given())\n \t\t\tfprintf(fp,\n \t\t\t\t\"%s\"\n \t\t\t\t\"# Committer: %s\\n\",\n \t\t\t\tident_shown++ ? \"\" : \"#\\n\",\n-\t\t\t\tcommitter_ident);\n+\t\t\t\tcommitter_ident.buf);\n \n \t\tif (ident_shown)\n \t\t\tfprintf(fp, \"#\\n\");\n@@ -697,6 +702,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\ts->use_color = 0;\n \t\tcommitable = run_status(fp, index_file, prefix, 1, s);\n \t\ts->use_color = saved_color_setting;\n+\n+\t\t*ai_tmp = ' ';\n+\t\t*ci_tmp = ' ';\n \t} else {\n \t\tunsigned char sha1[20];\n \t\tconst char *parent = \"HEAD\";\n@@ -712,6 +720,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\telse\n \t\t\tcommitable = index_differs_from(parent, 0);\n \t}\n+\tstrbuf_release(&committer_ident);\n \n \tfclose(fp);\n \n@@ -1246,6 +1255,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n int cmd_commit(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf author_ident = STRBUF_INIT;\n \tconst char *index_file, *reflog_msg;\n \tchar *nl, *p;\n \tunsigned char commit_sha1[20];\n@@ -1273,7 +1283,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t/* Set up everything for writing the commit object.  This includes\n \t   running hooks, writing the trees, and interacting with the user.  */\n-\tif (!prepare_to_commit(index_file, prefix, &s)) {\n+\tif (!prepare_to_commit(index_file, prefix, &s, &author_ident)) {\n \t\trollback_index_files();\n \t\treturn 1;\n \t}\n@@ -1352,11 +1362,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (commit_tree(sb.buf, active_cache_tree->sha1, parents, commit_sha1,\n-\t\t\tfmt_ident(author_name, author_email, author_date,\n-\t\t\t\tIDENT_ERROR_ON_NO_NAME))) {\n+\t\t\tauthor_ident.buf)) {\n \t\trollback_index_files();\n \t\tdie(\"failed to write commit object\");\n \t}\n+\tstrbuf_release(&author_ident);\n \n \tref_lock = lock_any_ref_for_update(\"HEAD\",\n \t\t\t\t\t   initial_commit ? NULL : head_sha1,\n"}]}