{"thread":{"id":"51308","subject":"[RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","startedAt":"2019-06-13T22:15:50Z","lastAt":"2019-06-19T22:09:27Z","messageCount":8,"participants":["Emily Shaffer","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"377202","messageId":"20190613221541.10007-1-emilyshaffer@google.com","threadId":"51308","inReplyTo":null,"subject":"[RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-06-13T22:15:41Z","receivedAt":"2019-06-13T22:15:50Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Indicate that --abbrev only works with --abbrev-commit also specified.\nIt seems that simply running `git rev-list --abbrev=5` doesn't\nabbreviate commit OIDs. But the combination of `git rev-list\n--abbrev-commit --abbrev=5` works as expected. Clarify in the\ndocumentation by indicating that --abbrev is an optional addition to the\n--abbrev-commit option. --no-abbrev remains on a separate line as it can\nstill be used to disable OID abbreviation even if --abbrev-commit has\nbeen specified.\n\nSigned-off-by: Emily Shaffer \nChange-Id: If9b1198938e1a3515ae6740241f7b791fb7a88bd\n---\nI thought this was odd when I was working on the other rev-list changes\n- --abbrev doesn't do anything on its own. It looks like it does work by\nitself in other commands, but apparently not in rev-list.\n\nListed this patch as RFC because maybe instead it's better to fix\nsomething so --abbrev can be used alone, or teach --abbrev-commit=.\nIt looks like `git log --abbrev=5` also doesn't work the way one might\nexpect, which makes sense to me, as they use the same internals for\noption parsing (parse_revisions()).\n\nThe manpages for log and rev-list both correctly indicate that\n--abbrev= is an optional addition to --abbrev-commit. `git log -h` is\ngenerated by parse-options tooling and doesn't cover --abbrev-commit at\nall, but `git rev-list` doesn't use an option parser on its own and the\nusage is hardcoded.\n\n - Emily\n\n builtin/rev-list.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 9f31837d30..6ae0087b01 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -49,8 +49,8 @@ static const char rev_list_usage[] =\n \" --objects | --objects-edge\\n\"\n \" --unpacked\\n\"\n \" --header | --pretty\\n\"\n-\" --abbrev= | --no-abbrev\\n\"\n-\" --abbrev-commit\\n\"\n+\" --abbrev-commit [--abbrev=]\\n\"\n+\" --no-abbrev\\n\"\n \" --left-right\\n\"\n \" --count\\n\"\n \" special purpose:\\n\"\n-- \n2.22.0.rc2.383.gf4fbbf30c2-goog\n\n"},{"id":"377243","messageId":"xmqq8su4up14.fsf@gitster-ct.c.googlers.com","threadId":"51308","inReplyTo":"20190613221541.10007-1-emilyshaffer@google.com","subject":"Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-14T16:09:43Z","receivedAt":"2019-06-14T16:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> It looks like `git log --abbrev=5` also doesn't work the way one might\n> expect, which makes sense to me, as they use the same internals for\n> option parsing (parse_revisions()).\n\nI suspect that was primarily because --abbrev-commit was only to\nabbreviate the commit object names on the log header, and --abbrev\nwas to abbreviate the object names seen in the --raw output. These\ndays, even --raw output seems to abbreviate by default, so --abbrev\nalone lost its usefulness quite a bit.\n\n"},{"id":"377245","messageId":"20190614161841.GB30083@sigill.intra.peff.net","threadId":"51308","inReplyTo":"20190613221541.10007-1-emilyshaffer@google.com","subject":"Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-14T16:18:41Z","receivedAt":"2019-06-14T16:18:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 13, 2019 at 03:15:41PM -0700, Emily Shaffer wrote:\n\n> I thought this was odd when I was working on the other rev-list changes\n> - --abbrev doesn't do anything on its own. It looks like it does work by\n> itself in other commands, but apparently not in rev-list.\n> \n> Listed this patch as RFC because maybe instead it's better to fix\n> something so --abbrev can be used alone, or teach --abbrev-commit=.\n> It looks like `git log --abbrev=5` also doesn't work the way one might\n> expect, which makes sense to me, as they use the same internals for\n> option parsing (parse_revisions()).\n> \n> The manpages for log and rev-list both correctly indicate that\n> --abbrev= is an optional addition to --abbrev-commit. `git log -h` is\n> generated by parse-options tooling and doesn't cover --abbrev-commit at\n> all, but `git rev-list` doesn't use an option parser on its own and the\n> usage is hardcoded.\n\nYeah, \"--abbrev\" is a bit tricky here. It is really \"when you abbrev, do\nit to this level\". In \"log\", that means that \"git log --abbrev=5 --raw\"\n_does_ do something useful (it abbreviates the blob hashes). And then\nyou may add \"--abbrev-commit\" on top to ask to abbreviate the commit\nids.\n\nAnd rev-list follows that same pattern, except that rev-list _never_\nshows diff output. You'd traditionally do (and this is how log was\nimplemented once upon a time):\n\n git rev-list HEAD | git diff-tree --stdin --abbrev=5 --raw\n\nBut even there, we are not seeing the commit id output by rev-list. It\ngoes to diff-tree, which then formats it separately. So if you wanted\nabbreviated commits there, you'd add \"--abbrev-commit\" to the diff-tree\ninvocation, not rev-list!\n\nSo no, I cannot see a way in which \"rev-list --abbrev\" is useful without\n\"--abbrev-commit\". Which means that perhaps the former should imply the\nlatter.\n\nAnd as you noticed in your other patch, there is no way to abbreviate\n\"--objects\" output at all. I am not sure I have ever seen a good use for\nthat. Though to be honest, I am not sure that \"--abbrev\" is really all\nthat useful in the first place. Machine-readable output should never\nabbreviate, and human-readable ones generally already do.\n\nBut at any rate, before making any behavior changes it may make sense to\nthink about how they'd interact with \"rev-list --objects\" abbreviation,\nif it were to be added.\n\nAs for the patch itself:\n\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 9f31837d30..6ae0087b01 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -49,8 +49,8 @@ static const char rev_list_usage[] =\n> \" --objects | --objects-edge\\n\"\n> \" --unpacked\\n\"\n> \" --header | --pretty\\n\"\n> -\" --abbrev= | --no-abbrev\\n\"\n> -\" --abbrev-commit\\n\"\n> +\" --abbrev-commit [--abbrev=]\\n\"\n> +\" --no-abbrev\\n\"\n\nSo --no-abbrev clears both --abbrev and --abbrev-commit. That sort of\nmakes sense, though I did not expect it. But it also means that:\n\n --abbrev-commit [--abbrev= | --no-abbrev]\n\nis not right. Possibly:\n\n --abbrev-commit [--abbrev=] | --no-abbrev\n\nwould show the interaction more clearly, but I don't have a strong\nopinion.\n\n-Peff\n"},{"id":"377257","messageId":"20190614205950.GC233791@google.com","threadId":"51308","inReplyTo":"20190614161841.GB30083@sigill.intra.peff.net","subject":"Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-06-14T20:59:50Z","receivedAt":"2019-06-14T20:59:58Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jun 14, 2019 at 12:18:41PM -0400, Jeff King wrote:\n> On Thu, Jun 13, 2019 at 03:15:41PM -0700, Emily Shaffer wrote:\n> \n> > I thought this was odd when I was working on the other rev-list changes\n> > - --abbrev doesn't do anything on its own. It looks like it does work by\n> > itself in other commands, but apparently not in rev-list.\n> > \n> > Listed this patch as RFC because maybe instead it's better to fix\n> > something so --abbrev can be used alone, or teach --abbrev-commit=.\n> > It looks like `git log --abbrev=5` also doesn't work the way one might\n> > expect, which makes sense to me, as they use the same internals for\n> > option parsing (parse_revisions()).\n> > \n> > The manpages for log and rev-list both correctly indicate that\n> > --abbrev= is an optional addition to --abbrev-commit. `git log -h` is\n> > generated by parse-options tooling and doesn't cover --abbrev-commit at\n> > all, but `git rev-list` doesn't use an option parser on its own and the\n> > usage is hardcoded.\n> \n> Yeah, \"--abbrev\" is a bit tricky here. It is really \"when you abbrev, do\n> it to this level\". In \"log\", that means that \"git log --abbrev=5 --raw\"\n> _does_ do something useful (it abbreviates the blob hashes). And then\n> you may add \"--abbrev-commit\" on top to ask to abbreviate the commit\n> ids.\n> \n> And rev-list follows that same pattern, except that rev-list _never_\n> shows diff output. You'd traditionally do (and this is how log was\n> implemented once upon a time):\n> \n> git rev-list HEAD | git diff-tree --stdin --abbrev=5 --raw\n> \n> But even there, we are not seeing the commit id output by rev-list. It\n> goes to diff-tree, which then formats it separately. So if you wanted\n> abbreviated commits there, you'd add \"--abbrev-commit\" to the diff-tree\n> invocation, not rev-list!\n> \n> So no, I cannot see a way in which \"rev-list --abbrev\" is useful without\n> \"--abbrev-commit\". Which means that perhaps the former should imply the\n> latter.\n\nMaybe it should; maybe this patch is a good excuse to do so, and enforce\nmutual exclusion of --abbrev-commit/--abbrev and --no-abbrev. Maybe it's\nalso a good time to add --abbrev-commit=?\n\n> \n> And as you noticed in your other patch, there is no way to abbreviate\n> \"--objects\" output at all. I am not sure I have ever seen a good use for\n> that. Though to be honest, I am not sure that \"--abbrev\" is really all\n> that useful in the first place. Machine-readable output should never\n> abbreviate, and human-readable ones generally already do.\n> \n> But at any rate, before making any behavior changes it may make sense to\n> think about how they'd interact with \"rev-list --objects\" abbreviation,\n> if it were to be added.\n> \n> As for the patch itself:\n> \n> > diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> > index 9f31837d30..6ae0087b01 100644\n> > --- a/builtin/rev-list.c\n> > +++ b/builtin/rev-list.c\n> > @@ -49,8 +49,8 @@ static const char rev_list_usage[] =\n> > \" --objects | --objects-edge\\n\"\n> > \" --unpacked\\n\"\n> > \" --header | --pretty\\n\"\n> > -\" --abbrev= | --no-abbrev\\n\"\n> > -\" --abbrev-commit\\n\"\n> > +\" --abbrev-commit [--abbrev=]\\n\"\n> > +\" --no-abbrev\\n\"\n> \n> So --no-abbrev clears both --abbrev and --abbrev-commit. That sort of\n> makes sense, though I did not expect it. But it also means that:\n> \n> --abbrev-commit [--abbrev= | --no-abbrev]\n> \n> is not right. Possibly:\n> \n> --abbrev-commit [--abbrev=] | --no-abbrev\n> \n> would show the interaction more clearly, but I don't have a strong\n> opinion.\n\nI did consider demonstrating it this way, but when both --abbrev-commit\nand --no-abbrev are used together, we don't complain or reject the\ninvocation - which I would expect if the usage states the two options\nare mutually exclusive.\n\nI've been trying to think of good reasons not to enforce their mutual\nexclusion, and the one I keep coming back to is that --no-abbrev might\nbe desired to override a git config'd abbreviation length - although I\ndidn't check to see whether we have one, maybe we would want one later.\nAnd even in that case, I suppose that --abbrev-commit would not be\nexplicitly added to the call (because we'd infer from the config), or\nthat if it did need to be explicitly added (like if we need the user to\nsay they want abbreviation, but we want to use their configured\npreferred length) then we could still reject the addition of\n--no-abbrev.\n\nSo maybe it makes even more sense to take this patch as an opportunity\nto make these options mutually exclusive... although that checking I\nthink would wind up in revision.c, and therefore widen the impact of\nthe change significantly.\n\nFrom a practical standpoint, I guess you could consider --abbrev-commit\nand --no-abbrev mutually exclusive, but since it's not enforced, I have\na little preference not to document it as though it is...\n\n - Emily\n"},{"id":"377260","messageId":"20190614212714.GA15798@sigill.intra.peff.net","threadId":"51308","inReplyTo":"20190614205950.GC233791@google.com","subject":"Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-14T21:27:14Z","receivedAt":"2019-06-14T21:27:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 14, 2019 at 01:59:50PM -0700, Emily Shaffer wrote:\n\n> > So no, I cannot see a way in which \"rev-list --abbrev\" is useful without\n> > \"--abbrev-commit\". Which means that perhaps the former should imply the\n> > latter.\n> \n> Maybe it should; maybe this patch is a good excuse to do so, and enforce\n> mutual exclusion of --abbrev-commit/--abbrev and --no-abbrev. Maybe it's\n> also a good time to add --abbrev-commit=?\n\nHmm, yeah, I think that would reduce confusion quite a bit. If it were\n\"--abbrev-commit=\", then \"--abbrev\" would not be useful for\nanything in rev-list. It would still work as a historical item, but we\nwould not need or want to advertise it in the usage at all. Good\nsuggestion.\n\n> > is not right. Possibly:\n> > \n> > --abbrev-commit [--abbrev=] | --no-abbrev\n> > \n> > would show the interaction more clearly, but I don't have a strong\n> > opinion.\n> \n> I did consider demonstrating it this way, but when both --abbrev-commit\n> and --no-abbrev are used together, we don't complain or reject the\n> invocation - which I would expect if the usage states the two options\n> are mutually exclusive.\n\nAh, I see. I don't consider \"|\" to indicate an exclusion to the point\nthat the options are rejected. Only that you wouldn't want to use both,\nbecause one counteracts the other. So every \"--no-foo\" is mutually\nexclusive with \"--foo\" in the sense that one override the other. But the\noutcome is \"last one wins\", and not \"whoops, we cannot figure out what\nyou meant\". And that's what the original:\n\n --abbrev= | --no-abbrev\n\nbefore your patch was trying to say (and I suspect there are many other\ncases of \"|\" with this kind of last-one-wins behavior).\n\n> I've been trying to think of good reasons not to enforce their mutual\n> exclusion, and the one I keep coming back to is that --no-abbrev might\n> be desired to override a git config'd abbreviation length - although I\n> didn't check to see whether we have one, maybe we would want one later.\n> And even in that case, I suppose that --abbrev-commit would not be\n> explicitly added to the call (because we'd infer from the config), or\n> that if it did need to be explicitly added (like if we need the user to\n> say they want abbreviation, but we want to use their configured\n> preferred length) then we could still reject the addition of\n> --no-abbrev.\n>\n> So maybe it makes even more sense to take this patch as an opportunity\n> to make these options mutually exclusive... although that checking I\n> think would wind up in revision.c, and therefore widen the impact of\n> the change significantly.\n\nYou can configure core.abbrev, though I'm not sure if it ever requests\nabbreviation itself, or if it simply sets the length when we do happen\nto abbreviate based on command-line options.\n\nBut forgetting config for a moment, last-one-wins is useful even among\ncommand line options. E.g., imagine an alias like this:\n\n [alias]\n mylog = git rev-list --abbrev-commit --pretty=oneline\n\nIt's nice if you can run \"git mylog --no-abbrev\" and have it do what you\nexpect, instead of complaining \"you cannot use --abbrev-commit and\n--no-abbrev together\".\n\nThat's a toy example, but you can imagine more elaborate scripts that\nset some default options, and allow arbitrary per-invocation options to\nbe appended.\n\n-Peff\n"},{"id":"377272","messageId":"20190614225654.GD233791@google.com","threadId":"51308","inReplyTo":"20190614212714.GA15798@sigill.intra.peff.net","subject":"Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-06-14T22:56:54Z","receivedAt":"2019-06-14T22:57:03Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Jun 14, 2019 at 05:27:14PM -0400, Jeff King wrote:\n> On Fri, Jun 14, 2019 at 01:59:50PM -0700, Emily Shaffer wrote:\n> \n> > > So no, I cannot see a way in which \"rev-list --abbrev\" is useful without\n> > > \"--abbrev-commit\". Which means that perhaps the former should imply the\n> > > latter.\n> > \n> > Maybe it should; maybe this patch is a good excuse to do so, and enforce\n> > mutual exclusion of --abbrev-commit/--abbrev and --no-abbrev. Maybe it's\n> > also a good time to add --abbrev-commit=?\n> \n> Hmm, yeah, I think that would reduce confusion quite a bit. If it were\n> \"--abbrev-commit=\", then \"--abbrev\" would not be useful for\n> anything in rev-list. It would still work as a historical item, but we\n> would not need or want to advertise it in the usage at all. Good\n> suggestion.\n\nGiven your comments below, I think rather than enforcing mutual\nexclusion it makes more sense to enforce last-one-wins. But the thinking\nis essentially the same.\n\n> \n> > > is not right. Possibly:\n> > > \n> > > --abbrev-commit [--abbrev=] | --no-abbrev\n> > > \n> > > would show the interaction more clearly, but I don't have a strong\n> > > opinion.\n> > \n> > I did consider demonstrating it this way, but when both --abbrev-commit\n> > and --no-abbrev are used together, we don't complain or reject the\n> > invocation - which I would expect if the usage states the two options\n> > are mutually exclusive.\n> \n> Ah, I see. I don't consider \"|\" to indicate an exclusion to the point\n> that the options are rejected. Only that you wouldn't want to use both,\n> because one counteracts the other. So every \"--no-foo\" is mutually\n> exclusive with \"--foo\" in the sense that one override the other. But the\n> outcome is \"last one wins\", and not \"whoops, we cannot figure out what\n> you meant\". And that's what the original:\n> \n> --abbrev= | --no-abbrev\n> \n> before your patch was trying to say (and I suspect there are many other\n> cases of \"|\" with this kind of last-one-wins behavior).\n\nFor what it's worth, in this case it's not last-one-wins - --no-abbrev\nalways wins:\n\n emilyshaffer@podkayne:~/git [master]$ g rev-list --abbrev-commit\n --no-abbrev --max-count=5 --pretty=oneline HEAD\n b697d92f56511e804b8ba20ccbe7bdc85dc66810 Git 2.22\n 6ee1eaca3e996e69691f515742129645f453e0e8 Merge tag 'l10n-2.22.0-rnd3' of\n git://github.com/git-l10n/git-po\n 0cdb8d2db2f39d1a29636975168c457d2dc0d466 Merge branch 'fr_review' of\n git://github.com/jnavila/git\n d0149200792f579151166a4a5bfae7e66c5d998b Merge branch 'master' of\n git://github.com/alshopov/git-po\n 82eb147dbbbd0221980883e87ca7efd16a939a6f l10n: fr.po: Review French\n translation\n emilyshaffer@podkayne:~/git [master]$ g rev-list --no-abbrev\n --abbrev-commit --max-count=5 --pretty=oneline HEAD\n b697d92f56511e804b8ba20ccbe7bdc85dc66810 Git 2.22\n 6ee1eaca3e996e69691f515742129645f453e0e8 Merge tag 'l10n-2.22.0-rnd3' of\n git://github.com/git-l10n/git-po\n 0cdb8d2db2f39d1a29636975168c457d2dc0d466 Merge branch 'fr_review' of\n git://github.com/jnavila/git\n d0149200792f579151166a4a5bfae7e66c5d998b Merge branch 'master' of\n git://github.com/alshopov/git-po\n 82eb147dbbbd0221980883e87ca7efd16a939a6f l10n: fr.po: Review French\n translation\n emilyshaffer@podkayne:~/git [master]$ g rev-list --abbrev-commit\n --max-count=5 --pretty=oneline HEAD\n b697d92f56 Git 2.22\n 6ee1eaca3e Merge tag 'l10n-2.22.0-rnd3' of\n git://github.com/git-l10n/git-po\n 0cdb8d2db2 Merge branch 'fr_review' of git://github.com/jnavila/git\n d014920079 Merge branch 'master' of git://github.com/alshopov/git-po\n 82eb147dbb l10n: fr.po: Review French translation\n\n> \n> > I've been trying to think of good reasons not to enforce their mutual\n> > exclusion, and the one I keep coming back to is that --no-abbrev might\n> > be desired to override a git config'd abbreviation length - although I\n> > didn't check to see whether we have one, maybe we would want one later.\n> > And even in that case, I suppose that --abbrev-commit would not be\n> > explicitly added to the call (because we'd infer from the config), or\n> > that if it did need to be explicitly added (like if we need the user to\n> > say they want abbreviation, but we want to use their configured\n> > preferred length) then we could still reject the addition of\n> > --no-abbrev.\n> >\n> > So maybe it makes even more sense to take this patch as an opportunity\n> > to make these options mutually exclusive... although that checking I\n> > think would wind up in revision.c, and therefore widen the impact of\n> > the change significantly.\n> \n> You can configure core.abbrev, though I'm not sure if it ever requests\n> abbreviation itself, or if it simply sets the length when we do happen\n> to abbreviate based on command-line options.\n> \n> But forgetting config for a moment, last-one-wins is useful even among\n> command line options. E.g., imagine an alias like this:\n> \n> [alias]\n> mylog = git rev-list --abbrev-commit --pretty=oneline\n> \n> It's nice if you can run \"git mylog --no-abbrev\" and have it do what you\n> expect, instead of complaining \"you cannot use --abbrev-commit and\n> --no-abbrev together\".\n> \n> That's a toy example, but you can imagine more elaborate scripts that\n> set some default options, and allow arbitrary per-invocation options to\n> be appended.\n\nThis makes a lot more sense than the scenarios I was imagining. Thanks.\n\nI think a good solution here is to go and add --abbrev-commit=\nwithout breaking support for --abbrev=; I'm a little more worried\nabout changing --no-abbrev to last-one-wins but I'll take a crack at it\nand see what the test suite says. While I'm at it, I'll check for\nlast-one-wins with multiple instances of --abbrev[-commit]=.\n\nHaving done so, I'll also change the documentation here in rev-list to:\n --abbrev-commit[=] [--abbrev=] | --no-abbrev\n\nSounds like a fun bit of low hanging fruit to me. Hoping to have a short\nturnaround. :)\n\n - Emily\n"},{"id":"377583","messageId":"20190619212159.GA6571@sigill.intra.peff.net","threadId":"51308","inReplyTo":"20190614225654.GD233791@google.com","subject":"Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-19T21:21:59Z","receivedAt":"2019-06-19T21:22:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 14, 2019 at 03:56:54PM -0700, Emily Shaffer wrote:\n\n> > Ah, I see. I don't consider \"|\" to indicate an exclusion to the point\n> > that the options are rejected. Only that you wouldn't want to use both,\n> > because one counteracts the other. So every \"--no-foo\" is mutually\n> > exclusive with \"--foo\" in the sense that one override the other. But the\n> > outcome is \"last one wins\", and not \"whoops, we cannot figure out what\n> > you meant\". And that's what the original:\n> > \n> > --abbrev= | --no-abbrev\n> > \n> > before your patch was trying to say (and I suspect there are many other\n> > cases of \"|\" with this kind of last-one-wins behavior).\n> \n> For what it's worth, in this case it's not last-one-wins - --no-abbrev\n> always wins:\n\nAh, thanks for pointing that; I hadn't noticed. That _is_ unlike most of\nthe rest of Git. I'm tempted to say it's a bug and should be fixed, but\nI worry slightly that it could have an unexpected effect.\n\n> I think a good solution here is to go and add --abbrev-commit=\n> without breaking support for --abbrev=; I'm a little more worried\n> about changing --no-abbrev to last-one-wins but I'll take a crack at it\n> and see what the test suite says. While I'm at it, I'll check for\n> last-one-wins with multiple instances of --abbrev[-commit]=.\n\nI think --abbrev-commit= sounds safe enough, though I worry it may\nget a bit complicated because we'd presumably want to fall back to the\n from --abbrev=. I'll see how your patch turns out. :)\n\nI like the idea of changing --no-abbrev to last-one-wins, as above, but\nthe test suite may not give us that much confidence. These kinds of\ncases are often not well-covered, and we're really worried about the\nwider world of random scripts people have grown over the last 10 years.\nOf course if the test suite does break horribly that might give us extra\ncaution, but I'm not sure \"the test suite does not break\" gives us much\nconfidence.\n\n> Having done so, I'll also change the documentation here in rev-list to:\n> --abbrev-commit[=] [--abbrev=] | --no-abbrev\n\nYeah, that makes sense.\n\n-Peff\n"},{"id":"377589","messageId":"20190619220919.GG100487@google.com","threadId":"51308","inReplyTo":"20190619212159.GA6571@sigill.intra.peff.net","subject":"Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-06-19T22:09:19Z","receivedAt":"2019-06-19T22:09:27Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 19, 2019 at 05:21:59PM -0400, Jeff King wrote:\n> On Fri, Jun 14, 2019 at 03:56:54PM -0700, Emily Shaffer wrote:\n> \n> > > Ah, I see. I don't consider \"|\" to indicate an exclusion to the point\n> > > that the options are rejected. Only that you wouldn't want to use both,\n> > > because one counteracts the other. So every \"--no-foo\" is mutually\n> > > exclusive with \"--foo\" in the sense that one override the other. But the\n> > > outcome is \"last one wins\", and not \"whoops, we cannot figure out what\n> > > you meant\". And that's what the original:\n> > > \n> > > --abbrev= | --no-abbrev\n> > > \n> > > before your patch was trying to say (and I suspect there are many other\n> > > cases of \"|\" with this kind of last-one-wins behavior).\n> > \n> > For what it's worth, in this case it's not last-one-wins - --no-abbrev\n> > always wins:\n> \n> Ah, thanks for pointing that; I hadn't noticed. That _is_ unlike most of\n> the rest of Git. I'm tempted to say it's a bug and should be fixed, but\n> I worry slightly that it could have an unexpected effect.\n> \n> > I think a good solution here is to go and add --abbrev-commit=\n> > without breaking support for --abbrev=; I'm a little more worried\n> > about changing --no-abbrev to last-one-wins but I'll take a crack at it\n> > and see what the test suite says. While I'm at it, I'll check for\n> > last-one-wins with multiple instances of --abbrev[-commit]=.\n> \n> I think --abbrev-commit= sounds safe enough, though I worry it may\n> get a bit complicated because we'd presumably want to fall back to the\n> from --abbrev=. I'll see how your patch turns out. :)\n> \n> I like the idea of changing --no-abbrev to last-one-wins, as above, but\n> the test suite may not give us that much confidence. These kinds of\n> cases are often not well-covered, and we're really worried about the\n> wider world of random scripts people have grown over the last 10 years.\n> Of course if the test suite does break horribly that might give us extra\n> caution, but I'm not sure \"the test suite does not break\" gives us much\n> confidence.\n\nI think at that point, based on what I've seen on the list, there's a\ntendency to include a config option to enable the new behavior. And I\nthink that might pass the tipping point of \"wasted effort\" for me.\nThe change isn't really enabling anything that was impossible before\n(last-one-wins is handy, but it's legacy public behavior now for\n--abbrev). So I'm kind of starting to lean towards doing nothing at all.\nIt's a lot of work, for a not-very-functional change, which needs to be\nspecially configured to even happen... So maybe I'll save my time and\nwork on actual bugs instead. :)\n\n> \n> > Having done so, I'll also change the documentation here in rev-list to:\n> > --abbrev-commit[=] [--abbrev=] | --no-abbrev\n> \n> Yeah, that makes sense.\n> \n> -Peff\n\nThanks for the input, all. I think I'll drop this.\n\n - Emily\n"}]}