{"thread":{"id":"40213","subject":"[PATCH v2] log: diagnose empty HEAD more clearly","startedAt":"2015-08-29T05:04:18Z","lastAt":"2015-08-31T16:36:51Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"268883","messageId":"20150829050418.GA23757@sigill.intra.peff.net","threadId":"40213","inReplyTo":null,"subject":"[PATCH v2] log: diagnose empty HEAD more clearly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-29T05:04:18Z","receivedAt":"2015-08-29T05:04:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 28, 2015 at 02:11:01PM -0700, Junio C Hamano wrote:\n\n> * jk/log-missing-default-HEAD (2015-06-03) 1 commit\n>  - log: diagnose empty HEAD more clearly\n> \n>  \"git init empty && git -C empty log\" said \"bad default revision 'HEAD'\",\n>  which was found to be a bit confusing to new users.\n> \n>  What's the status of this one?\n\nYou and I both provided patches, and you queued mine, but I think there\nwas some question of whether the error messages were actually much\nbetter. Here's a re-roll that tries to improve them by avoiding the word\n\"HEAD\", which the user did not provide in the first place.\n\n-- >8 --\nSubject: log: diagnose empty HEAD more clearly\n\nIf you init or clone an empty repository, the initial\nmessage from running \"git log\" is not very friendly:\n\n  $ git init\n  Initialized empty Git repository in /home/peff/foo/.git/\n  $ git log\n  fatal: bad default revision 'HEAD'\n\nLet's detect this situation and write a more friendly\nmessage:\n\n  $ git log\n  fatal: your current branch 'master' does not have any commits yet\n\nWe also detect the case that 'HEAD' points to a broken ref;\nthis should be even less common, but is easy to see. Note\nthat we do not diagnose all possible cases. We rely on\nresolve_ref, which means we do not get information about\ncomplex cases. E.g., \"--default master\" would use dwim_ref\nto find \"refs/heads/master\", but we notice only that\n\"master\" does not exist. Similarly, a complex sha1\nexpression like \"--default HEAD^2\" will not resolve as a\nref.\n\nBut that's OK. We fall back to a generic error message in\nthose cases, and they are unlikely to be used anyway.\nCatching an empty or broken \"HEAD\" improves the common case,\nand the other cases are not regressed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNote that this doesn't take us any closer to a world where \"git log\" on\nan empty HEAD silently exits with success (which your patch did). I\nthink it is somewhat orthogonal, though. If we wanted to do that we\nwould probably still die for a while (as your patch did), and it would\nmake sense to die using this diagnose function.\n\nSo I'd be happy if you wanted to resurrect yours on top, or squash them\ntogether. But I do not really think it is worth dealing with the\ncompatibility surprises to make the change.\n\n revision.c     | 17 ++++++++++++++++-\n t/t4202-log.sh | 14 ++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 5350139..af2a18e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2187,6 +2187,21 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \treturn 1;\n }\n \n+static void NORETURN diagnose_missing_default(const char *def)\n+{\n+\tunsigned char sha1[20];\n+\tint flags;\n+\tconst char *refname;\n+\n+\trefname = resolve_ref_unsafe(def, 0, sha1, &flags);\n+\tif (!refname || !(flags & REF_ISSYMREF) || (flags & REF_ISBROKEN))\n+\t\tdie(_(\"your current branch appears to be broken\"));\n+\n+\tskip_prefix(refname, \"refs/heads/\", &refname);\n+\tdie(_(\"your current branch '%s' does not have any commits yet\"),\n+\t    refname);\n+}\n+\n /*\n  * Parse revision information, filling in the \"rev_info\" structure,\n  * and removing the used arguments from the argument list.\n@@ -2316,7 +2331,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tstruct object *object;\n \t\tstruct object_context oc;\n \t\tif (get_sha1_with_context(revs->def, 0, sha1, &oc))\n-\t\t\tdie(\"bad default revision '%s'\", revs->def);\n+\t\t\tdiagnose_missing_default(revs->def);\n \t\tobject = get_reference(revs, revs->def, sha1, 0);\n \t\tadd_pending_object_with_mode(revs, object, revs->def, oc.mode);\n \t}\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 35d2d7c..6ede069 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -894,4 +894,18 @@ test_expect_success 'log --graph --no-walk is forbidden' '\n \ttest_must_fail git log --graph --no-walk\n '\n \n+test_expect_success 'log diagnoses bogus HEAD' '\n+\tgit init empty &&\n+\ttest_must_fail git -C empty log 2>stderr &&\n+\ttest_i18ngrep does.not.have.any.commits stderr &&\n+\techo 1234abcd >empty/.git/refs/heads/master &&\n+\ttest_must_fail git -C empty log 2>stderr &&\n+\ttest_i18ngrep broken stderr &&\n+\techo \"ref: refs/heads/invalid.lock\" >empty/.git/HEAD &&\n+\ttest_must_fail git -C empty log 2>stderr &&\n+\ttest_i18ngrep broken stderr &&\n+\ttest_must_fail git -C empty log --default totally-bogus 2>stderr &&\n+\ttest_i18ngrep broken stderr\n+'\n+\n test_done\n-- \n2.5.1.739.g7891f6b\n"},{"id":"269001","messageId":"xmqq4mjfxvmk.fsf@gitster.mtv.corp.google.com","threadId":"40213","inReplyTo":"20150829050418.GA23757@sigill.intra.peff.net","subject":"Re: [PATCH v2] log: diagnose empty HEAD more clearly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-31T16:36:51Z","receivedAt":"2015-08-31T16:36:51Z","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> On Fri, Aug 28, 2015 at 02:11:01PM -0700, Junio C Hamano wrote:\n>\n>> * jk/log-missing-default-HEAD (2015-06-03) 1 commit\n>>  - log: diagnose empty HEAD more clearly\n>> \n>>  \"git init empty && git -C empty log\" said \"bad default revision 'HEAD'\",\n>>  which was found to be a bit confusing to new users.\n>> \n>>  What's the status of this one?\n>\n> You and I both provided patches, and you queued mine, but I think there\n> was some question of whether the error messages were actually much\n> better. Here's a re-roll that tries to improve them by avoiding the word\n> \"HEAD\", which the user did not provide in the first place.\n>\n> -- >8 --\n> Subject: log: diagnose empty HEAD more clearly\n>\n> If you init or clone an empty repository, the initial\n> message from running \"git log\" is not very friendly:\n>\n>   $ git init\n>   Initialized empty Git repository in /home/peff/foo/.git/\n>   $ git log\n>   fatal: bad default revision 'HEAD'\n>\n> Let's detect this situation and write a more friendly\n> message:\n>\n>   $ git log\n>   fatal: your current branch 'master' does not have any commits yet\n>\n> We also detect the case that 'HEAD' points to a broken ref;\n> this should be even less common, but is easy to see. Note\n> that we do not diagnose all possible cases. We rely on\n> resolve_ref, which means we do not get information about\n> complex cases. E.g., \"--default master\" would use dwim_ref\n> to find \"refs/heads/master\", but we notice only that\n> \"master\" does not exist. Similarly, a complex sha1\n> expression like \"--default HEAD^2\" will not resolve as a\n> ref.\n>\n> But that's OK. We fall back to a generic error message in\n> those cases, and they are unlikely to be used anyway.\n> Catching an empty or broken \"HEAD\" improves the common case,\n> and the other cases are not regressed.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Note that this doesn't take us any closer to a world where \"git log\" on\n> an empty HEAD silently exits with success (which your patch did). I\n> think it is somewhat orthogonal, though. If we wanted to do that we\n> would probably still die for a while (as your patch did), and it would\n> make sense to die using this diagnose function.\n>\n> So I'd be happy if you wanted to resurrect yours on top, or squash them\n> together. But I do not really think it is worth dealing with the\n> compatibility surprises to make the change.\n\nThanks, let's take just one baby step and apply this for now.\nOthers can work on top after the dust settles.\n\n\n\n>\n>  revision.c     | 17 ++++++++++++++++-\n>  t/t4202-log.sh | 14 ++++++++++++++\n>  2 files changed, 30 insertions(+), 1 deletion(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 5350139..af2a18e 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2187,6 +2187,21 @@ static int handle_revision_pseudo_opt(const char *submodule,\n>  \treturn 1;\n>  }\n>  \n> +static void NORETURN diagnose_missing_default(const char *def)\n> +{\n> +\tunsigned char sha1[20];\n> +\tint flags;\n> +\tconst char *refname;\n> +\n> +\trefname = resolve_ref_unsafe(def, 0, sha1, &flags);\n> +\tif (!refname || !(flags & REF_ISSYMREF) || (flags & REF_ISBROKEN))\n> +\t\tdie(_(\"your current branch appears to be broken\"));\n> +\n> +\tskip_prefix(refname, \"refs/heads/\", &refname);\n> +\tdie(_(\"your current branch '%s' does not have any commits yet\"),\n> +\t    refname);\n> +}\n> +\n>  /*\n>   * Parse revision information, filling in the \"rev_info\" structure,\n>   * and removing the used arguments from the argument list.\n> @@ -2316,7 +2331,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\tstruct object *object;\n>  \t\tstruct object_context oc;\n>  \t\tif (get_sha1_with_context(revs->def, 0, sha1, &oc))\n> -\t\t\tdie(\"bad default revision '%s'\", revs->def);\n> +\t\t\tdiagnose_missing_default(revs->def);\n>  \t\tobject = get_reference(revs, revs->def, sha1, 0);\n>  \t\tadd_pending_object_with_mode(revs, object, revs->def, oc.mode);\n>  \t}\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 35d2d7c..6ede069 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -894,4 +894,18 @@ test_expect_success 'log --graph --no-walk is forbidden' '\n>  \ttest_must_fail git log --graph --no-walk\n>  '\n>  \n> +test_expect_success 'log diagnoses bogus HEAD' '\n> +\tgit init empty &&\n> +\ttest_must_fail git -C empty log 2>stderr &&\n> +\ttest_i18ngrep does.not.have.any.commits stderr &&\n> +\techo 1234abcd >empty/.git/refs/heads/master &&\n> +\ttest_must_fail git -C empty log 2>stderr &&\n> +\ttest_i18ngrep broken stderr &&\n> +\techo \"ref: refs/heads/invalid.lock\" >empty/.git/HEAD &&\n> +\ttest_must_fail git -C empty log 2>stderr &&\n> +\ttest_i18ngrep broken stderr &&\n> +\ttest_must_fail git -C empty log --default totally-bogus 2>stderr &&\n> +\ttest_i18ngrep broken stderr\n> +'\n> +\n>  test_done\n"}]}