{"thread":{"id":"54553","subject":"Segfault in git when using git logs","startedAt":"2020-11-02T14:00:42Z","lastAt":"2020-11-04T21:03:30Z","messageCount":23,"participants":["Sathyajith Bhat","Jeff King","Junio C Hamano","SZEDER Gábor","Christian Couder","Elijah Newren","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"408864","messageId":"CAMsWJsyPVQWV++gc2YJriEOEWBELa_xj2G0NWFMSgNYt47swiQ@mail.gmail.com","threadId":"54553","inReplyTo":null,"subject":"Segfault in git when using git logs","fromName":"Sathyajith Bhat","fromEmail":"sathya@sathyasays.com","sentAt":"2020-11-02T13:59:59Z","receivedAt":"2020-11-02T14:00:42Z","isPatch":false,"sender":{"key":"sathya@sathyasays.com","avatar":"https://gravatar.com/avatar/4b0a1061ca15713d07757a0a6c7369ea62aaa3eba46956fd41fcbca8a1152c49?d=mp&s=160"},"body":"Hi,\n\nMy code editor kept crashing repeatedly and when poked in syslogs, I\nfound that git was segfaulting. I can repro this easily (steps below).\nThis is the first time I'm reporting a bug here so I apologize if I've\ngotten anything wrong.\n\nBelow is the output from git bugreport\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\nTry to find commit logs for a file. I can repro this on a new repo as\nwell as an existing repo.\n\nSimple repro steps\n\n mkdir git_segfault_test && cd git_segfault_test && echo\n\"Hello\" > hello.log\n git init && git add hello.log && git commit -m \"init commit\"\n\nNow, use git log to show commit logs using command\n\n git log --follow -L 1,1:hello.log -- hello.log\n\nWhat did you expect to happen? (Expected behavior)\nGit should not segfault\n\nWhat happened instead? (Actual behavior)\nGit segfaulted\n\nWhat's different between what you expected and what actually happened?\n\nAnything else you want to add:\nNote that I am not calling the git log commands directly, my editor VS\nCode was repeatedly crashing and I searched in syslog and found this\nerror.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.29.2\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.9.2-arch1-1 #1 SMP PREEMPT Thu, 29 Oct 2020 17:01:28 +0000 x86_64\ncompiler info: gnuc: 10.2\nlibc info: glibc: 2.32\n$SHELL (typically, interactive shell): /usr/bin/zsh\n\n\n[Enabled Hooks]\n\n\n\n -- Sathya\nhttp://sathyabh.at | http://sbhat.me\nAuthor, Practical Docker with Python & AWS Community Hero\n"},{"id":"408865","messageId":"20201102144321.GA3962443@coredump.intra.peff.net","threadId":"54553","inReplyTo":"CAMsWJsyPVQWV++gc2YJriEOEWBELa_xj2G0NWFMSgNYt47swiQ@mail.gmail.com","subject":"Re: Segfault in git when using git logs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-02T14:43:21Z","receivedAt":"2020-11-02T14:43:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 02, 2020 at 03:59:59PM +0200, Sathyajith Bhat wrote:\n\n> Simple repro steps\n> \n> mkdir git_segfault_test && cd git_segfault_test && echo\n> \"Hello\" > hello.log\n> git init && git add hello.log && git commit -m \"init commit\"\n> \n> Now, use git log to show commit logs using command\n> \n> git log --follow -L 1,1:hello.log -- hello.log\n> \n> What did you expect to happen? (Expected behavior)\n> Git should not segfault\n\nThanks for making this reproduction recipe! I can easily see the problem\non my system. Looks like the segfault was introduced by a2bb801f6a\n(line-log: avoid unnecessary full tree diffs, 2019-08-21). I've cc'd the\nauthor.\n\nThat commit causes the line-log to clear the set of pathspecs, but the\n--follow option requires exactly one pathspec (and it even makes sure\nthe user gives us one, but that happens before we clear it internally).\nSomething like this makes the segfault go away:\n\ndiff --git a/line-log.c b/line-log.c\nindex 42c5e41f68..f789863928 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -847,6 +847,7 @@ static void queue_diffs(struct line_log_data *range,\n \t\tclear_pathspec(&opt->pathspec);\n \t\tparse_pathspec_from_ranges(&opt->pathspec, range);\n \t}\n+\topt->flags.follow_renames = 0;\n \tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n \tdiff_tree_oid(parent_tree_oid, tree_oid, \"\", opt);\n \tif (opt->detect_rename && diff_might_be_rename()) {\n\nbut I'm not clear on how \"--follow\" and \"-L\" are supposed to interact. I\nwouldn't expect --follow to do anything at all with line-log (nor for it\nto be useful to specify pathspecs outside of the -L option). So possibly\nthis is restoring the behavior prior to that commit, or possibly it's\njust papering over a breakage. ;)\n\n-Peff\n"},{"id":"408873","messageId":"xmqqzh3zvce5.fsf@gitster.c.googlers.com","threadId":"54553","inReplyTo":"20201102144321.GA3962443@coredump.intra.peff.net","subject":"Re: Segfault in git when using git logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-02T18:31:30Z","receivedAt":"2020-11-02T18:31:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That commit causes the line-log to clear the set of pathspecs, but the\n> --follow option requires exactly one pathspec (and it even makes sure\n> the user gives us one, but that happens before we clear it internally).\n> Something like this makes the segfault go away:\n>\n> diff --git a/line-log.c b/line-log.c\n> index 42c5e41f68..f789863928 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -847,6 +847,7 @@ static void queue_diffs(struct line_log_data *range,\n> \t\tclear_pathspec(&opt->pathspec);\n> \t\tparse_pathspec_from_ranges(&opt->pathspec, range);\n> \t}\n> +\topt->flags.follow_renames = 0;\n> \tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n> \tdiff_tree_oid(parent_tree_oid, tree_oid, \"\", opt);\n> \tif (opt->detect_rename && diff_might_be_rename()) {\n>\n> but I'm not clear on how \"--follow\" and \"-L\" are supposed to interact. I\n> wouldn't expect --follow to do anything at all with line-log (nor for it\n> to be useful to specify pathspecs outside of the -L option). So possibly\n> this is restoring the behavior prior to that commit, or possibly it's\n> just papering over a breakage. ;)\n\nAnother option is to catch it as \"these options are mutually\nexclusive\" error early before the control reaches this deep in the\ncodeflow, I would think, but I suspect that some people may\nhabitually add the \"--follow\" option in a context where it does not\nmake sense, so \"--follow is silently ignored when other options that\ncontradict it is in effect at the same time\" is OK by me, too.\n\nI do not know if that is the case offhand, but in the ideal\nworld, I would imagine\n\n\tgit log -L1,5:hello.c -C -C -- hello.c goodbye.c\n\tgit log -L1,5:hello.c -C -C\n\nto notice and show that some of these five lines were copied\nwhen or after hello.c was created, and keep following the\norigin of them in goodbye.c, and\n\n\tgit log -L1,5:hello.c -C -C\n\nmay do the same or find better match outside goodbye.c for\nthe origin of these lines and follow them instead, while\n\n\tgit log -L1,5:hello.c -- hello.c\n\tgit log -L1,5:hello.c --no-renames\n\nin the same history may say the commit that actually copied\nthese lines from goodbye.c just added them directly to hello.c\ninstead.\n\nAnd to extend the imagination a bit more,\n\n\tgit log -M -L1,5:hello.c\n\tgit log -L1,5:hello.c\n\tgit log --follow -L1,5:hello.c\n\nin a different history may notice that hello.c was created\nwholesale by renaming hola.c and follow the changes to these\nfive lines down the history. As -M is in effect by default\nthese days, the first two would be equivalent, and \"--follow\"\nwould be a meaningless noiseword in the context of this example\nwhere we are interested only in a single path hello.c in the\nend result, but in the ideal world, meaningless noiseword\nshould become hardless no-op, I would think.\n\nOf course, the above assumes that -L plays well with the\n-M/-C/--follow options and pathspec. If not, then \"they are\nincompatible\" would be the more sensible and easy-to-explain\noption.\n\nThanks.\n\n\n"},{"id":"408986","messageId":"20201103101553.GH24813@szeder.dev","threadId":"54553","inReplyTo":"20201102144321.GA3962443@coredump.intra.peff.net","subject":"Re: Segfault in git when using git logs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-11-03T10:15:53Z","receivedAt":"2020-11-03T10:15:58Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 02, 2020 at 09:43:21AM -0500, Jeff King wrote:\n> On Mon, Nov 02, 2020 at 03:59:59PM +0200, Sathyajith Bhat wrote:\n> \n> > Simple repro steps\n> > \n> > mkdir git_segfault_test && cd git_segfault_test && echo\n> > \"Hello\" > hello.log\n> > git init && git add hello.log && git commit -m \"init commit\"\n> > \n> > Now, use git log to show commit logs using command\n> > \n> > git log --follow -L 1,1:hello.log -- hello.log\n\nWhile Git should never segfault, no matter what, this is a bogus git\ninvocation to begin with: the second sentence in the description of\n'git log -L' clearly states that \"You may not give any pathspec\nlimiters\", so this command should have errored out from early days,\nbut, unfortunately, it was never enforced. This also means that '-L'\nand '--follow' are incompatible, because while the former forbids any\npathspecs, the latter requires exactly one; and line-level\nlog does its own rename following anyway.\n\nVS Code should be fixed to call 'git log -L 1,1:hello.log' instead,\nwithout '--follow' and without pathspec.\n\n> > What did you expect to happen? (Expected behavior)\n> > Git should not segfault\n> \n> Thanks for making this reproduction recipe! I can easily see the problem\n> on my system. Looks like the segfault was introduced by a2bb801f6a\n> (line-log: avoid unnecessary full tree diffs, 2019-08-21). I've cc'd the\n> author.\n> \n> That commit causes the line-log to clear the set of pathspecs, but the\n> --follow option requires exactly one pathspec (and it even makes sure\n> the user gives us one, but that happens before we clear it internally).\n> Something like this makes the segfault go away:\n> \n> diff --git a/line-log.c b/line-log.c\n> index 42c5e41f68..f789863928 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -847,6 +847,7 @@ static void queue_diffs(struct line_log_data *range,\n> \t\tclear_pathspec(&opt->pathspec);\n> \t\tparse_pathspec_from_ranges(&opt->pathspec, range);\n> \t}\n> +\topt->flags.follow_renames = 0;\n> \tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n> \tdiff_tree_oid(parent_tree_oid, tree_oid, \"\", opt);\n> \tif (opt->detect_rename && diff_might_be_rename()) {\n> \n> but I'm not clear on how \"--follow\" and \"-L\" are supposed to interact.\n\nThey shouldn't, I would say. Though it would be great if their\nrename-following logic would be unified. In particular, line-level\nlog does a better job at rename following in some ways, notably it can\ntrack multiple files at once, while '--follow' can only handle a\nsingle file. So I think the rename following logic should be\nextracted from 'line-log.c' and made more generic, and it should be\nused to implement '--follow', removing some restrictions of the\nlatter, not to mention removing the duplicated logic.\n\n(This might be a good GSoC project, though some of Linus' remarks in\n750f7b668f (Finally implement \"git log --follow\", 2007-06-19) like\n\"you did have to know and understand the internal git diff generation\nmachinery pretty well, and had to really be able to follow how commit\ngeneration interacts with generating patches and generating the log\"\nand \"this patch does seem to be firmly in the core \"Linus or Junio\"\nterritory\" are worrying...)\n\n> I\n> wouldn't expect --follow to do anything at all with line-log (nor for it\n> to be useful to specify pathspecs outside of the -L option). So possibly\n> this is restoring the behavior prior to that commit, or possibly it's\n> just papering over a breakage. ;)\n\nPerhaps, though arguably the original breakage was that 'git log\n-L...:file -- file' was meant to error out, but it didn't.\n\n"},{"id":"408990","messageId":"CAP8UFD3SMCBdE8TOP7OKabYyOW4344nNAa=G2hCzxWnN-XjQrA@mail.gmail.com","threadId":"54553","inReplyTo":"20201103101553.GH24813@szeder.dev","subject":"Re: Segfault in git when using git logs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-11-03T11:21:44Z","receivedAt":"2020-11-03T11:22:00Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 3, 2020 at 11:19 AM SZEDER Gábor wrote:\n>\n> On Mon, Nov 02, 2020 at 09:43:21AM -0500, Jeff King wrote:\n\n> > but I'm not clear on how \"--follow\" and \"-L\" are supposed to interact.\n>\n> They shouldn't, I would say. Though it would be great if their\n> rename-following logic would be unified. In particular, line-level\n> log does a better job at rename following in some ways, notably it can\n> track multiple files at once, while '--follow' can only handle a\n> single file. So I think the rename following logic should be\n> extracted from 'line-log.c' and made more generic, and it should be\n> used to implement '--follow', removing some restrictions of the\n> latter, not to mention removing the duplicated logic.\n>\n> (This might be a good GSoC project, though some of Linus' remarks in\n> 750f7b668f (Finally implement \"git log --follow\", 2007-06-19) like\n> \"you did have to know and understand the internal git diff generation\n> machinery pretty well, and had to really be able to follow how commit\n> generation interacts with generating patches and generating the log\"\n> and \"this patch does seem to be firmly in the core \"Linus or Junio\"\n> territory\" are worrying...)\n\nThanks for the suggestion!\n\nFor the Outreachy round starting next December, we have proposed a\nproject to accelerate rename detection and range-diff based on:\n\nhttps://github.com/gitgitgadget/git/issues/519\n\nI am not sure how much it is related to this though.\n"},{"id":"409007","messageId":"CABPp-BGdCOWCEMjXtJChW1oip2Ro1JhNr=pECcbhV8XWM2QBBw@mail.gmail.com","threadId":"54553","inReplyTo":"CAP8UFD3SMCBdE8TOP7OKabYyOW4344nNAa=G2hCzxWnN-XjQrA@mail.gmail.com","subject":"Re: Segfault in git when using git logs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-11-03T16:10:19Z","receivedAt":"2020-11-03T16:10:34Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Nov 3, 2020 at 3:21 AM Christian Couder\n wrote:\n>\n> On Tue, Nov 3, 2020 at 11:19 AM SZEDER Gábor wrote:\n> >\n> > On Mon, Nov 02, 2020 at 09:43:21AM -0500, Jeff King wrote:\n>\n> > > but I'm not clear on how \"--follow\" and \"-L\" are supposed to interact.\n> >\n> > They shouldn't, I would say. Though it would be great if their\n> > rename-following logic would be unified. In particular, line-level\n> > log does a better job at rename following in some ways, notably it can\n> > track multiple files at once, while '--follow' can only handle a\n> > single file. So I think the rename following logic should be\n> > extracted from 'line-log.c' and made more generic, and it should be\n> > used to implement '--follow', removing some restrictions of the\n> > latter, not to mention removing the duplicated logic.\n> >\n> > (This might be a good GSoC project, though some of Linus' remarks in\n> > 750f7b668f (Finally implement \"git log --follow\", 2007-06-19) like\n> > \"you did have to know and understand the internal git diff generation\n> > machinery pretty well, and had to really be able to follow how commit\n> > generation interacts with generating patches and generating the log\"\n> > and \"this patch does seem to be firmly in the core \"Linus or Junio\"\n> > territory\" are worrying...)\n>\n> Thanks for the suggestion!\n>\n> For the Outreachy round starting next December, we have proposed a\n> project to accelerate rename detection and range-diff based on:\n>\n> https://github.com/gitgitgadget/git/issues/519\n>\n> I am not sure how much it is related to this though.\n\nAs worded, the project is only tangentially related. If someone were\nto plumb better rename following through, then this Outreachy project\nmight make the underlying internals faster, but that's it. However,\nit's a certainly an interesting related project.\n\nAlso, I'll note that there is some code coming with merge-ort that\nshould help write an improved --follow implementation, and, in fact,\nwas specially added with that goal in mind. merge-ort adds to\ndiffcore-rename.c the ability to limit rename detection to either\nrelevant_sources (e.g. only consider removed filenames matching these\npaths for rename detection) or relevant_targets (e.g. only include\nadded files matching these paths for rename detection), or both. The\nrelevant_targets was not useful within the merge-ort implementation,\nbut since I was adding relevant_sources it seemed like a natural\npairing; I added it for completeness and was specifically thinking how\nit would be useful for log --follow when I did so. (I also think the\nstrmap API will also make it a bit easier to implement an improved\n--follow.) One of my goals was to use the relevant_targets ability as\npart of an improved log --follow implementation after getting the rest\nof the merge-ort stuff done, but it might be a useful intern project\ninstead.\n"},{"id":"409027","messageId":"20201103182102.GA459792@coredump.intra.peff.net","threadId":"54553","inReplyTo":"20201103101553.GH24813@szeder.dev","subject":"Re: Segfault in git when using git logs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-03T18:21:02Z","receivedAt":"2020-11-03T18:21:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 03, 2020 at 11:15:53AM +0100, SZEDER Gábor wrote:\n\n> > > Now, use git log to show commit logs using command\n> > > \n> > > git log --follow -L 1,1:hello.log -- hello.log\n> \n> While Git should never segfault, no matter what, this is a bogus git\n> invocation to begin with: the second sentence in the description of\n> 'git log -L' clearly states that \"You may not give any pathspec\n> limiters\", so this command should have errored out from early days,\n> but, unfortunately, it was never enforced. This also means that '-L'\n> and '--follow' are incompatible, because while the former forbids any\n> pathspecs, the latter requires exactly one; and line-level\n> log does its own rename following anyway.\n\nThanks for confirming. My \"I am not clear how these should interact\" was\nreally \"this does not make any sense to me\" in my head, but I was afraid\nthat I was missing something. The fact that we document explicitly that\n-L should not be combined with pathspecs makes that much more obvious.\n\n> VS Code should be fixed to call 'git log -L 1,1:hello.log' instead,\n> without '--follow' and without pathspec.\n\nAgreed.\n\nOn our side, I don't think it would be _wrong_ to catch and disallow the\ncombination. But it may be nicer to them if we continue to quietly\nignore --follow and the pathspec in that case, for working with older\nversions. (OTOH, if I understand correctly they're segfaulting every\ntime VS Code is used with v2.29 now, so they may have to accept it as an\nurgent fix anyway).\n\n-Peff\n"},{"id":"409028","messageId":"xmqq361qs31a.fsf@gitster.c.googlers.com","threadId":"54553","inReplyTo":"20201103182102.GA459792@coredump.intra.peff.net","subject":"Re: Segfault in git when using git logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T18:34:09Z","receivedAt":"2020-11-03T18:34:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 03, 2020 at 11:15:53AM +0100, SZEDER Gábor wrote:\n>\n>> > > Now, use git log to show commit logs using command\n>> > > \n>> > > git log --follow -L 1,1:hello.log -- hello.log\n>> \n>> While Git should never segfault, no matter what, this is a bogus git\n>> invocation to begin with: the second sentence in the description of\n>> 'git log -L' clearly states that \"You may not give any pathspec\n>> limiters\", so this command should have errored out from early days,\n>> but, unfortunately, it was never enforced. This also means that '-L'\n>> and '--follow' are incompatible, because while the former forbids any\n>> pathspecs, the latter requires exactly one; and line-level\n>> log does its own rename following anyway.\n>\n> Thanks for confirming. My \"I am not clear how these should interact\" was\n> really \"this does not make any sense to me\" in my head, but I was afraid\n> that I was missing something. The fact that we document explicitly that\n> -L should not be combined with pathspecs makes that much more obvious.\n>\n>> VS Code should be fixed to call 'git log -L 1,1:hello.log' instead,\n>> without '--follow' and without pathspec.\n>\n> Agreed.\n>\n> On our side, I don't think it would be _wrong_ to catch and disallow the\n> combination. But it may be nicer to them if we continue to quietly\n> ignore --follow and the pathspec in that case, for working with older\n> versions. (OTOH, if I understand correctly they're segfaulting every\n> time VS Code is used with v2.29 now, so they may have to accept it as an\n> urgent fix anyway).\n\nSo something like this won't harm VS Code more than we currently do,\nwhile telling users what is wrong with their command line?\n\nWe may still want the \"silently disable follow\" at low-level\nprotection, but that does not give feedback why the end-user option\nis silently ignored, so...\n\n builtin/log.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git c/builtin/log.c w/builtin/log.c\nindex 9f939e6cdf..8811084f02 100644\n--- c/builtin/log.c\n+++ w/builtin/log.c\n@@ -206,6 +206,13 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tif (argc > 1)\n \t\tdie(_(\"unrecognized argument: %s\"), argv[1]);\n \n+\tif (rev->line_level_traverse) {\n+\t\tif (rev->diffopt.filter)\n+\t\t\tdie(_(\"-L: cannot be used with pathspec\"));\n+\t\tif (rev->diffopt.flags.follow_renames)\n+\t\t\tdie(_(\"-L: cannot be used with --follow\"));\n+\t}\n+\n \tmemset(&w, 0, sizeof(w));\n \tuserformat_find_requirements(NULL, &w);\n \n"},{"id":"409031","messageId":"0581272e-6ad2-42d9-0196-b804ee304619@gmail.com","threadId":"54553","inReplyTo":"CAMsWJsyPVQWV++gc2YJriEOEWBELa_xj2G0NWFMSgNYt47swiQ@mail.gmail.com","subject":"Re: Segfault in git when using git logs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-03T18:46:12Z","receivedAt":"2020-11-03T18:46:17Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/2/2020 8:59 AM, Sathyajith Bhat wrote:\n> Anything else you want to add:\n\nHi Sathyajith,\n\nIt seems like others have some good approaches to handling\nthis segfault and making sure we find the right balance\nbetween supporting existing behavior and helping callers\nwho might be using this command incorrectly.\n\n> Note that I am not calling the git log commands directly, my editor VS\n> Code was repeatedly crashing and I searched in syslog and found this\n> error.\n\nI started filling out an issue on the VS Code GitHub repo\nso they could perhaps fix their usage of \"git log\" in this case.\nHowever, I started searching their code for where they might\nbe executing the command, and I didn't find anything!\n\nIs it possible that you have an extension that enables Git\nhistory commands like this?\n\nThe VS Code bug report issue template [1] has this bit of\nadvice to find out if an extension is at fault:\n\n> \n> Does this issue occur when all extensions are disabled?: Yes/No\n\n[1] https://github.com/microsoft/vscode/issues/new?template=bug_report.md\n\nCould you re-launch your editor with \"--disable-extensions\" to\nsee if this is reproduced? Also, which extensions do you have\ninstalled that might be adding Git command calls? We might need\nto find the extension authors instead of the core editor team.\n\nThanks,\n-Stolee\n"},{"id":"409032","messageId":"CAMsWJsxyW=JUh=XGCkGG3LhOsZASwFHtx9b6ziNi_uHa918Scw@mail.gmail.com","threadId":"54553","inReplyTo":"0581272e-6ad2-42d9-0196-b804ee304619@gmail.com","subject":"Re: Segfault in git when using git logs","fromName":"Sathyajith Bhat","fromEmail":"sathya@sathyasays.com","sentAt":"2020-11-03T18:55:52Z","receivedAt":"2020-11-03T18:56:32Z","isPatch":false,"sender":{"key":"sathya@sathyasays.com","avatar":"https://gravatar.com/avatar/4b0a1061ca15713d07757a0a6c7369ea62aaa3eba46956fd41fcbca8a1152c49?d=mp&s=160"},"body":"On Tue, 3 Nov 2020 at 20:46, Derrick Stolee wrote:\n\n> Hi Sathyajith,\n> I started filling out an issue on the VS Code GitHub repo\n> so they could perhaps fix their usage of \"git log\" in this case.\n> However, I started searching their code for where they might\n> be executing the command, and I didn't find anything!\n>\n> Is it possible that you have an extension that enables Git\n> history commands like this?\n\nHi Derek,\n\nI believe it is GitLens [1] which is causing the incorrect behaviour.\nI haven't had any crashes after I disabled GitLens, and that's the\nonly extension I've had which interacts with scm.\n\n[1] https://github.com/eamodio/vscode-gitlens\n\nthanks,\nSathya\n"},{"id":"409033","messageId":"20201103185711.GA461461@coredump.intra.peff.net","threadId":"54553","inReplyTo":"xmqq361qs31a.fsf@gitster.c.googlers.com","subject":"Re: Segfault in git when using git logs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-03T18:57:11Z","receivedAt":"2020-11-03T18:57:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 03, 2020 at 10:34:09AM -0800, Junio C Hamano wrote:\n\n> > On our side, I don't think it would be _wrong_ to catch and disallow the\n> > combination. But it may be nicer to them if we continue to quietly\n> > ignore --follow and the pathspec in that case, for working with older\n> > versions. (OTOH, if I understand correctly they're segfaulting every\n> > time VS Code is used with v2.29 now, so they may have to accept it as an\n> > urgent fix anyway).\n> \n> So something like this won't harm VS Code more than we currently do,\n> while telling users what is wrong with their command line?\n\nYeah, I was wondering if we'd want the patch you sent, or if we should\nturn those die() calls into warning() and disable the flags up front.\n\n> We may still want the \"silently disable follow\" at low-level\n> protection, but that does not give feedback why the end-user option\n> is silently ignored, so...\n\nI'd be just as happy to leave it out, if we think it isn't triggerable.\nThe segfault would let people know we missed a spot. ;)\n\n> diff --git c/builtin/log.c w/builtin/log.c\n> index 9f939e6cdf..8811084f02 100644\n> --- c/builtin/log.c\n> +++ w/builtin/log.c\n> @@ -206,6 +206,13 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n> \tif (argc > 1)\n> \t\tdie(_(\"unrecognized argument: %s\"), argv[1]);\n> \n> +\tif (rev->line_level_traverse) {\n> +\t\tif (rev->diffopt.filter)\n> +\t\t\tdie(_(\"-L: cannot be used with pathspec\"));\n\nShould this be checking rev->diffopt.pathspec.nr?\n\nI could well believe that --diff-filter=A does not work with \"-L\"\neither, but that is a separate story.\n\n-Peff\n"},{"id":"409036","messageId":"20201103192327.GA462936@coredump.intra.peff.net","threadId":"54553","inReplyTo":"CAMsWJsxyW=JUh=XGCkGG3LhOsZASwFHtx9b6ziNi_uHa918Scw@mail.gmail.com","subject":"Re: Segfault in git when using git logs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-03T19:23:27Z","receivedAt":"2020-11-03T19:23:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 03, 2020 at 08:55:52PM +0200, Sathyajith Bhat wrote:\n\n> I believe it is GitLens [1] which is causing the incorrect behaviour.\n> I haven't had any crashes after I disabled GitLens, and that's the\n> only extension I've had which interacts with scm.\n> \n> [1] https://github.com/eamodio/vscode-gitlens\n\nThat makes sense. It seems pretty clear that its log__file() function\nwill produce this bogus combination. Despite having some indication that\nit knows about the documentation forbidding it:\n\n https://github.com/eamodio/vscode-gitlens/blob/6cfd9fdedd7c6ec3bfa732af7c418bbbecdfba54/src/git/git.ts#L805\n\nI don't know that project's code well enough to say whether there is a\nhigher-level bug there (the issue seems to be the \"renames\" flag being\nenabled along with startLine; maybe the caller is wrong to specify\nboth).\n\n-Peff\n"},{"id":"409038","messageId":"0b6657a6-afc2-7862-de43-94868f9419a5@gmail.com","threadId":"54553","inReplyTo":"20201103192327.GA462936@coredump.intra.peff.net","subject":"Re: Segfault in git when using git logs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-03T20:07:15Z","receivedAt":"2020-11-03T20:07:21Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/3/2020 2:23 PM, Jeff King wrote:\n> On Tue, Nov 03, 2020 at 08:55:52PM +0200, Sathyajith Bhat wrote:\n> \n>> I believe it is GitLens [1] which is causing the incorrect behaviour.\n>> I haven't had any crashes after I disabled GitLens, and that's the\n>> only extension I've had which interacts with scm.\n>>\n>> [1] https://github.com/eamodio/vscode-gitlens\n> \n> That makes sense. It seems pretty clear that its log__file() function\n> will produce this bogus combination. Despite having some indication that\n> it knows about the documentation forbidding it:\n> \n> https://github.com/eamodio/vscode-gitlens/blob/6cfd9fdedd7c6ec3bfa732af7c418bbbecdfba54/src/git/git.ts#L805\n> \n> I don't know that project's code well enough to say whether there is a\n> higher-level bug there (the issue seems to be the \"renames\" flag being\n> enabled along with startLine; maybe the caller is wrong to specify\n> both).\n\nThanks, both, for confirming the extension and the likely\nline of code causing this problem. I submitted an issue [1]\non the GitLens repository. I'll continue watching it, but\nfeel free to chime in yourself if you are interested.\n\n[1] https://github.com/eamodio/vscode-gitlens/issues/1139\n\nThanks,\n-Stolee\n"},{"id":"409041","messageId":"xmqqimamqjhl.fsf@gitster.c.googlers.com","threadId":"54553","inReplyTo":"20201103185711.GA461461@coredump.intra.peff.net","subject":"Re: Segfault in git when using git logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-03T20:21:42Z","receivedAt":"2020-11-03T20:21:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 03, 2020 at 10:34:09AM -0800, Junio C Hamano wrote:\n>\n>> > On our side, I don't think it would be _wrong_ to catch and disallow the\n>> > combination. But it may be nicer to them if we continue to quietly\n>> > ignore --follow and the pathspec in that case, for working with older\n>> > versions. (OTOH, if I understand correctly they're segfaulting every\n>> > time VS Code is used with v2.29 now, so they may have to accept it as an\n>> > urgent fix anyway).\n>> \n>> So something like this won't harm VS Code more than we currently do,\n>> while telling users what is wrong with their command line?\n>\n> Yeah, I was wondering if we'd want the patch you sent, or if we should\n> turn those die() calls into warning() and disable the flags up front.\n>\n>> We may still want the \"silently disable follow\" at low-level\n>> protection, but that does not give feedback why the end-user option\n>> is silently ignored, so...\n>\n> I'd be just as happy to leave it out, if we think it isn't triggerable.\n> The segfault would let people know we missed a spot. ;)\n>\n>> diff --git c/builtin/log.c w/builtin/log.c\n>> index 9f939e6cdf..8811084f02 100644\n>> --- c/builtin/log.c\n>> +++ w/builtin/log.c\n>> @@ -206,6 +206,13 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n>> \tif (argc > 1)\n>> \t\tdie(_(\"unrecognized argument: %s\"), argv[1]);\n>> \n>> +\tif (rev->line_level_traverse) {\n>> +\t\tif (rev->diffopt.filter)\n>> +\t\t\tdie(_(\"-L: cannot be used with pathspec\"));\n>\n> Should this be checking rev->diffopt.pathspec.nr?\n\nEmbarrassing but yes ;-).\n"},{"id":"409046","messageId":"233ea345-68ea-fdfb-9c7d-1c2cb4f7e4ba@gmail.com","threadId":"54553","inReplyTo":"0b6657a6-afc2-7862-de43-94868f9419a5@gmail.com","subject":"Re: Segfault in git when using git logs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-03T21:04:06Z","receivedAt":"2020-11-03T21:23:58Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/3/2020 3:07 PM, Derrick Stolee wrote:\n> Thanks, both, for confirming the extension and the likely\n> line of code causing this problem. I submitted an issue [1]\n> on the GitLens repository. I'll continue watching it, but\n> feel free to chime in yourself if you are interested.\n> \n> [1] https://github.com/eamodio/vscode-gitlens/issues/1139\n\nThe issue has been fixed in GitLens, so please update your\nextension to verify.\n\n[1] https://github.com/eamodio/vscode-gitlens/commit/d980c7b831961f932ae68ed6e8ab08f71983b9a9\n[2] https://twitter.com/eamodio/status/1323729485338914817?s=20\n\nThanks,\n-Stolee\n"},{"id":"409076","messageId":"20201104133129.GA3030480@coredump.intra.peff.net","threadId":"54553","inReplyTo":"xmqqimamqjhl.fsf@gitster.c.googlers.com","subject":"Re: Segfault in git when using git logs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T13:31:29Z","receivedAt":"2020-11-04T13:31:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 03, 2020 at 12:21:42PM -0800, Junio C Hamano wrote:\n\n> >> So something like this won't harm VS Code more than we currently do,\n> >> while telling users what is wrong with their command line?\n> >\n> > Yeah, I was wondering if we'd want the patch you sent, or if we should\n> > turn those die() calls into warning() and disable the flags up front.\n> [...]\n> >> diff --git c/builtin/log.c w/builtin/log.c\n\nSince we now have identified the caller that was passing in the bogus\noptions, and it has already been fixed, I'm inclined to say we should go\nwith your die() patch from earlier (modulo the pathspec.nr fixup) and\ncall it done.\n\nDo you want to roll that up with a commit message?\n\n-Peff\n"},{"id":"409092","messageId":"CAMsWJswaTaEaKs+Brsy0YB26n4X4WAbu9J-1U88vZLjcQwKs5w@mail.gmail.com","threadId":"54553","inReplyTo":"233ea345-68ea-fdfb-9c7d-1c2cb4f7e4ba@gmail.com","subject":"Re: Segfault in git when using git logs","fromName":"Sathyajith Bhat","fromEmail":"sathya@sathyasays.com","sentAt":"2020-11-04T15:49:50Z","receivedAt":"2020-11-04T15:50:30Z","isPatch":false,"sender":{"key":"sathya@sathyasays.com","avatar":"https://gravatar.com/avatar/4b0a1061ca15713d07757a0a6c7369ea62aaa3eba46956fd41fcbca8a1152c49?d=mp&s=160"},"body":"On Tue, 3 Nov 2020 at 23:04, Derrick Stolee wrote:\n\n> The issue has been fixed in GitLens, so please update your\n> extension to verify.\n>\n> [1] https://github.com/eamodio/vscode-gitlens/commit/d980c7b831961f932ae68ed6e8ab08f71983b9a9\n> [2] https://twitter.com/eamodio/status/1323729485338914817?s=20\n\nI re-enabled the extension today and was able to get by the whole day\nwithout any crashes - well, there was one but I couldn't repro it or\nfind the logs for the same.\n\nThank you folks for reporting to Gitlens, truly appreciate the work\ngoes behind the scenes\n\nCheers,\nSathya\n"},{"id":"409095","messageId":"xmqqimalnl5b.fsf@gitster.c.googlers.com","threadId":"54553","inReplyTo":"20201104133129.GA3030480@coredump.intra.peff.net","subject":"Re: Segfault in git when using git logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T16:26:24Z","receivedAt":"2020-11-04T16:26:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 03, 2020 at 12:21:42PM -0800, Junio C Hamano wrote:\n>\n>> >> So something like this won't harm VS Code more than we currently do,\n>> >> while telling users what is wrong with their command line?\n>> >\n>> > Yeah, I was wondering if we'd want the patch you sent, or if we should\n>> > turn those die() calls into warning() and disable the flags up front.\n>> [...]\n>> >> diff --git c/builtin/log.c w/builtin/log.c\n>\n> Since we now have identified the caller that was passing in the bogus\n> options, and it has already been fixed, I'm inclined to say we should go\n> with your die() patch from earlier (modulo the pathspec.nr fixup) and\n> call it done.\n>\n> Do you want to roll that up with a commit message?\n\nOK, will try.\n\n"},{"id":"409116","messageId":"xmqq7dr1nh3a.fsf_-_@gitster.c.googlers.com","threadId":"54553","inReplyTo":"xmqqimamqjhl.fsf@gitster.c.googlers.com","subject":"Re*: Segfault in git when using git logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T17:54:01Z","receivedAt":"2020-11-04T17:54:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>>> +\tif (rev->line_level_traverse) {\n>>> +\t\tif (rev->diffopt.filter)\n>>> +\t\t\tdie(_(\"-L: cannot be used with pathspec\"));\n>>\n>> Should this be checking rev->diffopt.pathspec.nr?\n>\n> Embarrassing but yes ;-).\n\nI wonder if rev->prune_data.nr is what matters here, though.\n\nThe prune_data is often identical to diffopt.pathspec, but the\nformer affects the paths that participate in history simplification,\nwhile the latter is used when deciding which paths to show\ndifferences between the commit and its parent(s) and used to widen\nthe set independently from prune_data for the \"--full-diff\" option.\n\n-- >8 --\nSubject: [PATCH] log: diagnose -L used with pathspec as an error\n\nThe -L option is documented to accept no pathspec, but the\ncommand line option parser has allowed the combination without\nchecking so far. Ensure that there is no pathspec when the -L\noption is in effect to fix this.\n\nIncidentally, this change fixes another bug in the command line\noption parser, which has allowed the -L option used together\nwith the --follow option. Because the latter requires exactly\none path given, but the former takes no pathspec, they become\nmutually incompatible automatically. Because the -L option\nfollows renames on its own, there is no reason to give --follow\nat the same time.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/log.c | 3 +++\n t/t4211-line-log.sh | 18 ++++++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 0a7ed4bef9..9d70f3e60b 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -206,6 +206,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tif (argc > 1)\n \t\tdie(_(\"unrecognized argument: %s\"), argv[1]);\n \n+\tif (rev->line_level_traverse && rev->prune_data.nr)\n+\t\tdie(_(\"-L: cannot be used with pathspec\"));\n+\n \tmemset(&w, 0, sizeof(w));\n \tuserformat_find_requirements(NULL, &w);\n \ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 2d1d7b5d19..3d1bd6ed80 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -8,6 +8,24 @@ test_expect_success 'setup (import history)' '\n \tgit reset --hard\n '\n \n+# Basic command line option parsing\n+test_expect_success '-L is incompatible with pathspec' '\n+\t# This may fail due to \"no such path a.c in commit\",\n+\t# or \"-L is incompatible with pathspec\". Either is acceptable.\n+\ttest_must_fail git log -L1,1:a.c -- a.c &&\n+\n+\t# This must fail due to \"-L is incompatible with pathspec\".\n+\ttest_must_fail git log -L1,1:b.c -- b.c &&\n+\n+\t# These must fail due to \"follow requires one pathspec\".\n+\ttest_must_fail git log -L1,1:b.c --follow &&\n+\ttest_must_fail git log --follow -L1,1:b.c &&\n+\n+\t# This may fail due to \"-L is incompatible with pathspec\",\n+\t# or \"-L is incompatible with pathspec\". Either is acceptable.\n+\ttest_must_fail git log --follow -L1,1:b.c -- b.c\n+'\n+\n canned_test_1 () {\n \ttest_expect_$1 \"$2\" \"\n \t\tgit log $2 >actual &&\n-- \n2.29.2-287-gba574db674\n\n"},{"id":"409129","messageId":"20201104194155.GA3060815@coredump.intra.peff.net","threadId":"54553","inReplyTo":"xmqq7dr1nh3a.fsf_-_@gitster.c.googlers.com","subject":"Re: Re*: Segfault in git when using git logs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T19:41:55Z","receivedAt":"2020-11-04T19:41:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2020 at 09:54:01AM -0800, Junio C Hamano wrote:\n\n> >> Should this be checking rev->diffopt.pathspec.nr?\n> [...]\n> \n> I wonder if rev->prune_data.nr is what matters here, though.\n> \n> The prune_data is often identical to diffopt.pathspec, but the\n> former affects the paths that participate in history simplification,\n> while the latter is used when deciding which paths to show\n> differences between the commit and its parent(s) and used to widen\n> the set independently from prune_data for the \"--full-diff\" option.\n\nHmm, yeah, I think you are right. We only care about whether there is an\nentry, so I didn't think \"widen\" would matter. But one form of widening\nis to have no pathspec at all. :)\n\n> -- >8 --\n> Subject: [PATCH] log: diagnose -L used with pathspec as an error\n> \n> The -L option is documented to accept no pathspec, but the\n> command line option parser has allowed the combination without\n> checking so far. Ensure that there is no pathspec when the -L\n> option is in effect to fix this.\n> \n> Incidentally, this change fixes another bug in the command line\n> option parser, which has allowed the -L option used together\n> with the --follow option. Because the latter requires exactly\n> one path given, but the former takes no pathspec, they become\n> mutually incompatible automatically. Because the -L option\n> follows renames on its own, there is no reason to give --follow\n> at the same time.\n\nMakes sense...\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 0a7ed4bef9..9d70f3e60b 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -206,6 +206,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n> \tif (argc > 1)\n> \t\tdie(_(\"unrecognized argument: %s\"), argv[1]);\n> \n> +\tif (rev->line_level_traverse && rev->prune_data.nr)\n> +\t\tdie(_(\"-L: cannot be used with pathspec\"));\n> +\n\nI was thinking this would have to go deeper in the revision code, but\n\"-L\" is strictly a git-log thing. So this looks like the right place to\nadd the check.\n\n> +# Basic command line option parsing\n> +test_expect_success '-L is incompatible with pathspec' '\n> +\t# This may fail due to \"no such path a.c in commit\",\n> +\t# or \"-L is incompatible with pathspec\". Either is acceptable.\n> +\ttest_must_fail git log -L1,1:a.c -- a.c &&\n\nThis test confuses me. What are we looking for here? Presumably we'd\nfail with:\n\n git log -L1,1:a.c\n\ntoo. If the test were \"basic command line parsing\", I could see checking\nthat. But that's only what the comment says. :) I don't see how adding\nin the pathspec is interesting, nor that it matches the test title.\n\n> +\t# This must fail due to \"-L is incompatible with pathspec\".\n> +\ttest_must_fail git log -L1,1:b.c -- b.c &&\n\nRight, this is what we fixed. Would using test_i18ngrep on the stderr be\nbetter than the comment?\n\n> +\t# These must fail due to \"follow requires one pathspec\".\n> +\ttest_must_fail git log -L1,1:b.c --follow &&\n> +\ttest_must_fail git log --follow -L1,1:b.c &&\n\nThese are really tests of --follow, but I don't mind seeing them here as\nreinforcement for the concepts that the commit message claims.\n\n> +\t# This may fail due to \"-L is incompatible with pathspec\",\n> +\t# or \"-L is incompatible with pathspec\". Either is acceptable.\n> +\ttest_must_fail git log --follow -L1,1:b.c -- b.c\n\nShould one of those be \"-L is incompatible with --follow\"? Though of\ncourse we did not add such a check, so we know that it will be \"-L is\nincompatible with pathspec\", even without the --follow.\n\n-Peff\n"},{"id":"409138","messageId":"xmqq361onahn.fsf@gitster.c.googlers.com","threadId":"54553","inReplyTo":"20201104194155.GA3060815@coredump.intra.peff.net","subject":"Re: Re*: Segfault in git when using git logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T20:16:36Z","receivedAt":"2020-11-04T20:16:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +# Basic command line option parsing\n>> +test_expect_success '-L is incompatible with pathspec' '\n>> +\t# This may fail due to \"no such path a.c in commit\",\n>> +\t# or \"-L is incompatible with pathspec\". Either is acceptable.\n>> +\ttest_must_fail git log -L1,1:a.c -- a.c &&\n>\n> This test confuses me. What are we looking for here? Presumably we'd\n> fail with:\n>\n> git log -L1,1:a.c\n>\n> too. If the test were \"basic command line parsing\", I could see checking\n> that. But that's only what the comment says.\n\nYeah, I was undecided to have a single test that covers all (which I\nended up with) or a sequence of individual tests (which I wrote on\nthe title).\n\n>> +\t# This must fail due to \"-L is incompatible with pathspec\".\n>> +\ttest_must_fail git log -L1,1:b.c -- b.c &&\n>\n> Right, this is what we fixed. Would using test_i18ngrep on the stderr be\n> better than the comment?\n\nI do not care either way myself ;-)\n\n>> +\t# These must fail due to \"follow requires one pathspec\".\n>> +\ttest_must_fail git log -L1,1:b.c --follow &&\n>> +\ttest_must_fail git log --follow -L1,1:b.c &&\n>\n> These are really tests of --follow, but I don't mind seeing them here as\n> reinforcement for the concepts that the commit message claims.\n>\n>> +\t# This may fail due to \"-L is incompatible with pathspec\",\n>> +\t# or \"-L is incompatible with pathspec\". Either is acceptable.\n>> +\ttest_must_fail git log --follow -L1,1:b.c -- b.c\n>\n> Should one of those be \"-L is incompatible with --follow\"? Though of\n> course we did not add such a check, so we know that it will be \"-L is\n> incompatible with pathspec\", even without the --follow.\n\nThe comment seems utterly wrong here. I may reroll after taking a\nnap or something ;-)\n"},{"id":"409162","messageId":"xmqqy2jglv29.fsf_-_@gitster.c.googlers.com","threadId":"54553","inReplyTo":"xmqq361onahn.fsf@gitster.c.googlers.com","subject":"[PATCH] log: diagnose -L used with pathspec as an error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-04T20:35:10Z","receivedAt":"2020-11-04T20:35:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The -L option is documented to accept no pathspec, but the\ncommand line option parser has allowed the combination without\nchecking so far. Ensure that there is no pathspec when the -L\noption is in effect to fix this.\n\nIncidentally, this change fixes another bug in the command line\noption parser, which has allowed the -L option used together\nwith the --follow option. Because the latter requires exactly\none path given, but the former takes no pathspec, they become\nmutually incompatible automatically. Because the -L option\nfollows renames on its own, there is no reason to give --follow\nat the same time.\n\nThe new tests say they may fail with \"-L and --follow being\nincompatible\" instead of \"-L and pathspec being imcompatible\".\nCurrently the expected failure can come only from the latter, but\nthis is to futureproof them, in case we decide to add code to\nexplicititly die on -L and --follow used together.\n\nHeled-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/log.c | 3 +++\n t/t4211-line-log.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+)\n\n * This time, as a standalone patch, instead of a comment in a\n discussion thread.\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 0a7ed4bef9..9d70f3e60b 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -206,6 +206,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tif (argc > 1)\n \t\tdie(_(\"unrecognized argument: %s\"), argv[1]);\n \n+\tif (rev->line_level_traverse && rev->prune_data.nr)\n+\t\tdie(_(\"-L: cannot be used with pathspec\"));\n+\n \tmemset(&w, 0, sizeof(w));\n \tuserformat_find_requirements(NULL, &w);\n \ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 2d1d7b5d19..b85c4a8a04 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -8,6 +8,32 @@ test_expect_success 'setup (import history)' '\n \tgit reset --hard\n '\n \n+test_expect_success 'basic command line parsing' '\n+\t# This may fail due to \"no such path a.c in commit\", or\n+\t# \"-L is incompatible with pathspec\", depending on the\n+\t# order the error is checked. Either is acceptable.\n+\ttest_must_fail git log -L1,1:a.c -- a.c &&\n+\n+\t# This must fail due to \"-L is incompatible with pathspec\".\n+\ttest_must_fail git log -L1,1:b.c -- b.c 2>error &&\n+\ttest_i18ngrep \"cannot be used with pathspec\" error &&\n+\n+\t# Note that incompatibility between -L/--follow is not\n+\t# explicitly checked to avoid redundant code and the comments\n+\t# on the following tests are merely for future-proofing.\n+\n+\t# These must fail due to \"follow requires one pathspec\", or\n+\t# \"-L is incompatible with --follow\", depending on the\n+\t# order the error is checked. Either is acceptable.\n+\ttest_must_fail git log -L1,1:b.c --follow &&\n+\ttest_must_fail git log --follow -L1,1:b.c &&\n+\n+\t# This may fail due to \"-L is incompatible with pathspec\", or\n+\t# \"-L is incompatible with --follow\", depending on the\n+\t# order the error is checked. Either is acceptable.\n+\ttest_must_fail git log --follow -L1,1:b.c -- b.c\n+'\n+\n canned_test_1 () {\n \ttest_expect_$1 \"$2\" \"\n \t\tgit log $2 >actual &&\n-- \n2.29.2-287-gba574db674\n\n"},{"id":"409166","messageId":"20201104210327.GG3629238@coredump.intra.peff.net","threadId":"54553","inReplyTo":"xmqqy2jglv29.fsf_-_@gitster.c.googlers.com","subject":"Re: [PATCH] log: diagnose -L used with pathspec as an error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-04T21:03:27Z","receivedAt":"2020-11-04T21:03:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2020 at 12:35:10PM -0800, Junio C Hamano wrote:\n\n> The new tests say they may fail with \"-L and --follow being\n> incompatible\" instead of \"-L and pathspec being imcompatible\".\n> Currently the expected failure can come only from the latter, but\n> this is to futureproof them, in case we decide to add code to\n> explicititly die on -L and --follow used together.\n\nThis explanation makes sense (though s/imcompat/incompat/).\n\n> +test_expect_success 'basic command line parsing' '\n> +\t# This may fail due to \"no such path a.c in commit\", or\n> +\t# \"-L is incompatible with pathspec\", depending on the\n> +\t# order the error is checked. Either is acceptable.\n> +\ttest_must_fail git log -L1,1:a.c -- a.c &&\n> +\n> +\t# This must fail due to \"-L is incompatible with pathspec\".\n> +\ttest_must_fail git log -L1,1:b.c -- b.c 2>error &&\n> +\ttest_i18ngrep \"cannot be used with pathspec\" error &&\n\nThe renaming makes sense...\n\n> +\n> +\t# Note that incompatibility between -L/--follow is not\n> +\t# explicitly checked to avoid redundant code and the comments\n> +\t# on the following tests are merely for future-proofing.\n\n...as does this comment to explain the rest of the tests.\n\n> +\t# These must fail due to \"follow requires one pathspec\", or\n> +\t# \"-L is incompatible with --follow\", depending on the\n> +\t# order the error is checked. Either is acceptable.\n> +\ttest_must_fail git log -L1,1:b.c --follow &&\n> +\ttest_must_fail git log --follow -L1,1:b.c &&\n> +\n> +\t# This may fail due to \"-L is incompatible with pathspec\", or\n> +\t# \"-L is incompatible with --follow\", depending on the\n> +\t# order the error is checked. Either is acceptable.\n> +\ttest_must_fail git log --follow -L1,1:b.c -- b.c\n> +'\n\nThough \"depending on the order\" is a bit of a fiction, because those\nchecks do not exist at all. I'm OK with it because the earlier comment\nexplains what is going. I guess:\n\n # This may fail due to \"-L is incompatible with pathspec\", or\n # \"-L is incompatible with --follow\". We don't have the latter as of\n # the writing of this test, but either would be acceptable if we added\n # it.\n\nwould be an alternative. I doubt it's worth spending too much time\npolishing.\n\n-Peff\n"}]}