{"thread":{"id":"54132","subject":"Mismatched HEAD default behavior from git log","startedAt":"2020-08-25T19:16:55Z","lastAt":"2020-08-26T20:20:18Z","messageCount":7,"participants":["Bryan Turner","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"404471","messageId":"CAGyf7-G_ciVpgvvOiH1Fq9kNuWunCpM1fhv3ao_RMXBB0K=HMA@mail.gmail.com","threadId":"54132","inReplyTo":null,"subject":"Mismatched HEAD default behavior from git log","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2020-08-25T19:16:38Z","receivedAt":"2020-08-25T19:16:55Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"When git log is run without revs, it defaults to HEAD. That's\nwell-documented. However, when paired with --ignore-missing, there's a\nbehavior mismatch if all the provided revs are nonexistent.\n\nIf you provide the revs on the command line, like \"git log\n--ignore-missing nonexistent --\" (the trailing -- is just to help git\nlog know \"nonexistent\" isn't intended to be a path), you get no output\nand no error (exit code is 0).\n\nIf you provide the revs via stdin, like \"echo 'nonexistent' | git log\n--ignore-missing --stdin --\", you get every commit reachable from\nHEAD.\n\nIt appears the way --stdin processes input discards nonexistent\ncommits before the machinery that decides whether you provided any\nrevs or not runs, and so if every --stdin rev is discarded then you\nget the default HEAD. If you provide them via the command line,\nthough, then it seems like they're discarded later and you don't get a\ndefault.\n\nI'm not sure whether this is intentional or not (certainly I don't see\nit anywhere in the git log documentation for --ignore-missing or\n--stdin), but it results in a behavior mismatch that's impossible to\nreconcile without requiring extra git processes. I can't always\nprovide HEAD since, if multiple revs are supplied, if any revs exist\nthen HEAD would not be included regardless of whether the revs were\nsupplied via the command line or --stdin.\n\nI've confirmed this behavior as far back as Git 1.8.0 all the way up\nto 2.28.0, so it's by no means new. I can't recall seeing it come up\non the list before, but that doesn't mean it hasn't. Is this a bug, or\na feature?\n\nBest regards,\nBryan Turner\n"},{"id":"404472","messageId":"xmqq8se21pl1.fsf@gitster.c.googlers.com","threadId":"54132","inReplyTo":"CAGyf7-G_ciVpgvvOiH1Fq9kNuWunCpM1fhv3ao_RMXBB0K=HMA@mail.gmail.com","subject":"Re: Mismatched HEAD default behavior from git log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T19:40:42Z","receivedAt":"2020-08-25T19:40:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bryan Turner <bturner@atlassian.com> writes:\n\n> It appears the way --stdin processes input discards nonexistent\n> commits before the machinery that decides whether you provided any\n> revs or not runs, and so if every --stdin rev is discarded then you\n> get the default HEAD. If you provide them via the command line,\n> though, then it seems like they're discarded later and you don't get a\n> default.\n>\n> I'm not sure whether this is intentional or not (certainly I don't see\n> it anywhere in the git log documentation for --ignore-missing or\n> --stdin), but it results in a behavior mismatch that's impossible to\n> reconcile without requiring extra git processes. I can't always\n> provide HEAD since, if multiple revs are supplied, if any revs exist\n> then HEAD would not be included regardless of whether the revs were\n> supplied via the command line or --stdin.\n\nAs the intent for adding the \"--stdin\" option to any subcommand has\nalways been \"we may need to feed many many things, that may bust the\ncommand line length limit, hence we let you feed these things from\nthe standard input, but otherwise there should be no change in\nbehaviour or semantics\", when the behaviour of command line and\n\"--stdin\" differ, it is a bug in the latter.\n"},{"id":"404474","messageId":"20200825194619.GB1419759@coredump.intra.peff.net","threadId":"54132","inReplyTo":"xmqq8se21pl1.fsf@gitster.c.googlers.com","subject":"Re: Mismatched HEAD default behavior from git log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T19:46:19Z","receivedAt":"2020-08-25T19:46:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 12:40:42PM -0700, Junio C Hamano wrote:\n\n> Bryan Turner <bturner@atlassian.com> writes:\n> \n> > It appears the way --stdin processes input discards nonexistent\n> > commits before the machinery that decides whether you provided any\n> > revs or not runs, and so if every --stdin rev is discarded then you\n> > get the default HEAD. If you provide them via the command line,\n> > though, then it seems like they're discarded later and you don't get a\n> > default.\n> >\n> > I'm not sure whether this is intentional or not (certainly I don't see\n> > it anywhere in the git log documentation for --ignore-missing or\n> > --stdin), but it results in a behavior mismatch that's impossible to\n> > reconcile without requiring extra git processes. I can't always\n> > provide HEAD since, if multiple revs are supplied, if any revs exist\n> > then HEAD would not be included regardless of whether the revs were\n> > supplied via the command line or --stdin.\n> \n> As the intent for adding the \"--stdin\" option to any subcommand has\n> always been \"we may need to feed many many things, that may bust the\n> command line length limit, hence we let you feed these things from\n> the standard input, but otherwise there should be no change in\n> behaviour or semantics\", when the behaviour of command line and\n> \"--stdin\" differ, it is a bug in the latter.\n\nAgreed. It also helps in this case that the command-line behavior is\nsensible and the --stdin one is not. :)\n\nI think the solution is probably something like:\n\ndiff --git a/revision.c b/revision.c\nindex 96630e3186..f5bbefa091 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2099,12 +2099,13 @@ static void read_pathspec_from_stdin(struct strbuf *sb,\n \t\tstrvec_push(prune, sb->buf);\n }\n \n-static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t      struct strvec *prune)\n+static int read_revisions_from_stdin(struct rev_info *revs,\n+\t\t\t\t     struct strvec *prune)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n \tint save_warning;\n+\tint got_rev_arg = 0;\n \n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n@@ -2124,12 +2125,14 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n+\t\tgot_rev_arg = 1;\n \t}\n \tif (seen_dashdash)\n \t\tread_pathspec_from_stdin(&sb, prune);\n \n \tstrbuf_release(&sb);\n \twarn_on_object_refname_ambiguity = save_warning;\n+\treturn got_rev_arg;\n }\n \n static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)\n@@ -2754,7 +2757,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n-\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n+\t\t\t\tif (read_revisions_from_stdin(revs, &prune_data))\n+\t\t\t\t\tgot_rev_arg = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \n\nPossibly it would make sense to push that flag into rev_info, though,\nand let handle_revision_arg() set it. That would fix this bug and\nprevent similar ones in other code paths (though we're not likely to get\nrevisions from anywhere else, I suppose).\n\n-Peff\n"},{"id":"404476","messageId":"xmqq4koq1p28.fsf@gitster.c.googlers.com","threadId":"54132","inReplyTo":"20200825194619.GB1419759@coredump.intra.peff.net","subject":"Re: Mismatched HEAD default behavior from git log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T19:51:59Z","receivedAt":"2020-08-25T19:52:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> As the intent for adding the \"--stdin\" option to any subcommand has\n>> always been \"we may need to feed many many things, that may bust the\n>> command line length limit, hence we let you feed these things from\n>> the standard input, but otherwise there should be no change in\n>> behaviour or semantics\", when the behaviour of command line and\n>> \"--stdin\" differ, it is a bug in the latter.\n>\n> Agreed. It also helps in this case that the command-line behavior is\n> sensible and the --stdin one is not. :)\n>\n> I think the solution is probably something like:\n\nYou beat me to it while I was wondering what to do between the local\ngot_rev_arg variable and the revs->rev_input_given field.\n\n\n> diff --git a/revision.c b/revision.c\n> index 96630e3186..f5bbefa091 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2099,12 +2099,13 @@ static void read_pathspec_from_stdin(struct strbuf *sb,\n>  \t\tstrvec_push(prune, sb->buf);\n>  }\n>  \n> -static void read_revisions_from_stdin(struct rev_info *revs,\n> -\t\t\t\t      struct strvec *prune)\n> +static int read_revisions_from_stdin(struct rev_info *revs,\n> +\t\t\t\t     struct strvec *prune)\n>  {\n>  \tstruct strbuf sb;\n>  \tint seen_dashdash = 0;\n>  \tint save_warning;\n> +\tint got_rev_arg = 0;\n>  \n>  \tsave_warning = warn_on_object_refname_ambiguity;\n>  \twarn_on_object_refname_ambiguity = 0;\n> @@ -2124,12 +2125,14 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n>  \t\tif (handle_revision_arg(sb.buf, revs, 0,\n>  \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n>  \t\t\tdie(\"bad revision '%s'\", sb.buf);\n> +\t\tgot_rev_arg = 1;\n>  \t}\n>  \tif (seen_dashdash)\n>  \t\tread_pathspec_from_stdin(&sb, prune);\n>  \n>  \tstrbuf_release(&sb);\n>  \twarn_on_object_refname_ambiguity = save_warning;\n> +\treturn got_rev_arg;\n>  }\n>  \n>  static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)\n> @@ -2754,7 +2757,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\t\t\t}\n>  \t\t\t\tif (revs->read_from_stdin++)\n>  \t\t\t\t\tdie(\"--stdin given twice?\");\n> -\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n> +\t\t\t\tif (read_revisions_from_stdin(revs, &prune_data))\n> +\t\t\t\t\tgot_rev_arg = 1;\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n>  \n>\n> Possibly it would make sense to push that flag into rev_info, though,\n> and let handle_revision_arg() set it. That would fix this bug and\n> prevent similar ones in other code paths (though we're not likely to get\n> revisions from anywhere else, I suppose).\n>\n> -Peff\n"},{"id":"404477","messageId":"20200825195511.GD1419759@coredump.intra.peff.net","threadId":"54132","inReplyTo":"xmqq4koq1p28.fsf@gitster.c.googlers.com","subject":"Re: Mismatched HEAD default behavior from git log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T19:55:11Z","receivedAt":"2020-08-25T19:55:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 12:51:59PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> As the intent for adding the \"--stdin\" option to any subcommand has\n> >> always been \"we may need to feed many many things, that may bust the\n> >> command line length limit, hence we let you feed these things from\n> >> the standard input, but otherwise there should be no change in\n> >> behaviour or semantics\", when the behaviour of command line and\n> >> \"--stdin\" differ, it is a bug in the latter.\n> >\n> > Agreed. It also helps in this case that the command-line behavior is\n> > sensible and the --stdin one is not. :)\n> >\n> > I think the solution is probably something like:\n> \n> You beat me to it while I was wondering what to do between the local\n> got_rev_arg variable and the revs->rev_input_given field.\n\nThat makes me wonder why we need got_rev_arg at all if we have\nrevs->rev_input_given. But I suspect an answer can be found by digging\ninto git-blame. I probably won't do that immediately, so if you want to,\nyou can do so without worrying that we're duplicating work. :)\n\n-Peff\n"},{"id":"404548","messageId":"20200826201305.GA1595824@coredump.intra.peff.net","threadId":"54132","inReplyTo":"20200825195511.GD1419759@coredump.intra.peff.net","subject":"[PATCH] revision: set rev_input_given in handle_revision_arg()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-26T20:13:05Z","receivedAt":"2020-08-26T20:13:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 03:55:11PM -0400, Jeff King wrote:\n\n> > You beat me to it while I was wondering what to do between the local\n> > got_rev_arg variable and the revs->rev_input_given field.\n> \n> That makes me wonder why we need got_rev_arg at all if we have\n> revs->rev_input_given. But I suspect an answer can be found by digging\n> into git-blame. I probably won't do that immediately, so if you want to,\n> you can do so without worrying that we're duplicating work. :)\n\nThis is all my fault, naturally. :) I added rev_input_given a while ago\nbut didn't go far enough. Here's what I think we should do:\n\n-- >8 --\nSubject: [PATCH] revision: set rev_input_given in handle_revision_arg()\n\nCommit 7ba826290a (revision: add rev_input_given flag, 2017-08-02) added\na flag to rev_info to tell whether we got any revision arguments. As\nexplained there, this is necessary because some revision arguments may\nnot produce any pending traversal objects, but should still inhibit\ndefault behaviors (e.g., a glob that matches nothing).\n\nHowever, it only set the flag in the globbing code, but not for\nrevisions we get on the command-line or via stdin. This leads to two\nproblems:\n\n  - the command-line code keeps its own separate got_rev_arg flag; this\n    isn't wrong, but it's confusing and an extra maintenance burden\n\n  - even specifically-named rev arguments might end up not adding any\n    pending objects: if --ignore-missing is set, then specifying a\n    missing object is a noop rather than an error.\n\nAnd that leads to some user-visible bugs:\n\n  - when deciding whether a default rev like \"HEAD\" should kick in, we\n    check both got_rev_arg and rev_input_given. That means that\n    \"--ignore-missing $ZERO_OID\" works on the command-line (where we set\n    got_rev_arg) but not on --stdin (where we don't)\n\n  - when rev-list decides whether it should complain that it wasn't\n    given a starting point, it relies on rev_input_given. So it can't\n    even get the command-line \"--ignore-missing $ZERO_OID\" right\n\nLet's consistently set the flag if we got any revision argument. That\nlets us clean up the redundant got_rev_arg, and fixes both of those bugs\n(but note there are three new tests: we'll confirm the already working\ngit-log command-line case).\n\nA few implementation notes:\n\n  - conceptually we want to set the flag whenever handle_revision_arg()\n    finds an actual revision arg (\"handles\" it, you might say). But it\n    covers a ton of cases with early returns. Rather than annotating\n    each one, we just wrap it and use its success exit-code to set the\n    flag in one spot.\n\n  - the new rev-list test is in t6018, which is titled to cover globs.\n    This isn't exactly a glob, but it made sense to stick it with the\n    other tests that handle the \"even though we got a rev, we have no\n    pending objects\" case, which are globs.\n\n  - the tests check for the oid of a missing object, which it's pretty\n    clear --ignore-missing should ignore. You can see the same behavior\n    with \"--ignore-missing a-ref-that-does-not-exist\", because\n    --ignore-missing treats them both the same. That's perhaps less\n    clearly correct, and we may want to change that in the future. But\n    the way the code and tests here are written, we'd continue to do the\n    right thing even if it does.\n\nReported-by: Bryan Turner <bturner@atlassian.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n revision.c               | 16 +++++++++++-----\n t/t4202-log.sh           | 10 ++++++++++\n t/t6018-rev-list-glob.sh |  5 +++++\n 3 files changed, 26 insertions(+), 5 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 96630e3186..08c2ad23af 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2017,7 +2017,7 @@ static int handle_dotdot(const char *arg,\n \treturn ret;\n }\n \n-int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)\n+static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)\n {\n \tstruct object_context oc;\n \tchar *mark;\n@@ -2092,6 +2092,14 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \treturn 0;\n }\n \n+int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n+{\n+\tint ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);\n+\tif (!ret)\n+\t\trevs->rev_input_given = 1;\n+\treturn ret;\n+}\n+\n static void read_pathspec_from_stdin(struct strbuf *sb,\n \t\t\t\t     struct strvec *prune)\n {\n@@ -2703,7 +2711,7 @@ static void NORETURN diagnose_missing_default(const char *def)\n  */\n int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)\n {\n-\tint i, flags, left, seen_dashdash, got_rev_arg = 0, revarg_opt;\n+\tint i, flags, left, seen_dashdash, revarg_opt;\n \tstruct strvec prune_data = STRVEC_INIT;\n \tconst char *submodule = NULL;\n \tint seen_end_of_options = 0;\n@@ -2792,8 +2800,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tstrvec_pushv(&prune_data, argv + i);\n \t\t\tbreak;\n \t\t}\n-\t\telse\n-\t\t\tgot_rev_arg = 1;\n \t}\n \n \tif (prune_data.nr) {\n@@ -2822,7 +2828,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\topt->tweak(revs, opt);\n \tif (revs->show_merge)\n \t\tprepare_show_merge(revs);\n-\tif (revs->def && !revs->pending.nr && !revs->rev_input_given && !got_rev_arg) {\n+\tif (revs->def && !revs->pending.nr && !revs->rev_input_given) {\n \t\tstruct object_id oid;\n \t\tstruct object *object;\n \t\tstruct object_context oc;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex a0930599aa..56d34ed465 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1850,6 +1850,16 @@ test_expect_success 'log does not default to HEAD when rev input is given' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'do not default to HEAD with ignored object on cmdline' '\n+\tgit log --ignore-missing $ZERO_OID >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'do not default to HEAD with ignored object on stdin' '\n+\techo $ZERO_OID | git log --ignore-missing --stdin >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'set up --source tests' '\n \tgit checkout --orphan source-a &&\n \ttest_commit one &&\ndiff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh\nindex bb5aeac07f..b31ff7eeec 100755\n--- a/t/t6018-rev-list-glob.sh\n+++ b/t/t6018-rev-list-glob.sh\n@@ -345,6 +345,11 @@ test_expect_success 'rev-list should succeed with empty output with empty glob'\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'rev-list should succeed with empty output when ignoring missing' '\n+\tgit rev-list --ignore-missing $ZERO_OID >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'shortlog accepts --glob/--tags/--remotes' '\n \n \tcompare shortlog \"subspace/one subspace/two\" --branches=subspace &&\n-- \n2.28.0.749.gf1242ce4bd\n\n"},{"id":"404549","messageId":"xmqqa6yhupl2.fsf@gitster.c.googlers.com","threadId":"54132","inReplyTo":"20200826201305.GA1595824@coredump.intra.peff.net","subject":"Re: [PATCH] revision: set rev_input_given in handle_revision_arg()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T20:20:09Z","receivedAt":"2020-08-26T20:20:18Z","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> Subject: [PATCH] revision: set rev_input_given in handle_revision_arg()\n>\n> Commit 7ba826290a (revision: add rev_input_given flag, 2017-08-02) added\n> a flag to rev_info to tell whether we got any revision arguments. As\n> explained there, this is necessary because some revision arguments may\n> not produce any pending traversal objects, but should still inhibit\n> default behaviors (e.g., a glob that matches nothing).\n\nAh, that explains the symptom under discussion quite well; the topic\nthat introduced the commit was to fix \"git log --tag=no-such-tag\"\nthat used to default to HEAD.  We should be able to reuse the same\nmechanism to prevent --stdin codepath from falling back to HEAD, of\ncourse.\n\n> However, it only set the flag in the globbing code, but not for\n> revisions we get on the command-line or via stdin. This leads to two\n> problems:\n>\n>   - the command-line code keeps its own separate got_rev_arg flag; this\n>     isn't wrong, but it's confusing and an extra maintenance burden\n>\n>   - even specifically-named rev arguments might end up not adding any\n>     pending objects: if --ignore-missing is set, then specifying a\n>     missing object is a noop rather than an error.\n\nYup, nicely described.\n\n> diff --git a/revision.c b/revision.c\n> index 96630e3186..08c2ad23af 100644\n> --- a/revision.c\n> +++ b/revision.c\n\nThe patch of course looks straightforward and good.  Thanks.\n\n\n"}]}