| Message ID | xmqq7dr1nh3a.fsf_-_@gitster.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Re*: Segfault in git when using git logs | expand |
On Wed, Nov 04, 2020 at 09:54:01AM -0800, Junio C Hamano wrote: > >> Should this be checking rev->diffopt.pathspec.nr? > [...] > > I wonder if rev->prune_data.nr is what matters here, though. > > The prune_data is often identical to diffopt.pathspec, but the > former affects the paths that participate in history simplification, > while the latter is used when deciding which paths to show > differences between the commit and its parent(s) and used to widen > the set independently from prune_data for the "--full-diff" option. Hmm, yeah, I think you are right. We only care about whether there is an entry, so I didn't think "widen" would matter. But one form of widening is to have no pathspec at all. :) > -- >8 -- > Subject: [PATCH] log: diagnose -L used with pathspec as an error > > The -L option is documented to accept no pathspec, but the > command line option parser has allowed the combination without > checking so far. Ensure that there is no pathspec when the -L > option is in effect to fix this. > > Incidentally, this change fixes another bug in the command line > option parser, which has allowed the -L option used together > with the --follow option. Because the latter requires exactly > one path given, but the former takes no pathspec, they become > mutually incompatible automatically. Because the -L option > follows renames on its own, there is no reason to give --follow > at the same time. Makes sense... > diff --git a/builtin/log.c b/builtin/log.c > index 0a7ed4bef9..9d70f3e60b 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -206,6 +206,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, > if (argc > 1) > die(_("unrecognized argument: %s"), argv[1]); > > + if (rev->line_level_traverse && rev->prune_data.nr) > + die(_("-L<range>:<file> cannot be used with pathspec")); > + I was thinking this would have to go deeper in the revision code, but "-L" is strictly a git-log thing. So this looks like the right place to add the check. > +# Basic command line option parsing > +test_expect_success '-L is incompatible with pathspec' ' > + # This may fail due to "no such path a.c in commit", > + # or "-L is incompatible with pathspec". Either is acceptable. > + test_must_fail git log -L1,1:a.c -- a.c && This test confuses me. What are we looking for here? Presumably we'd fail with: git log -L1,1:a.c too. If the test were "basic command line parsing", I could see checking that. But that's only what the comment says. :) I don't see how adding in the pathspec is interesting, nor that it matches the test title. > + # This must fail due to "-L is incompatible with pathspec". > + test_must_fail git log -L1,1:b.c -- b.c && Right, this is what we fixed. Would using test_i18ngrep on the stderr be better than the comment? > + # These must fail due to "follow requires one pathspec". > + test_must_fail git log -L1,1:b.c --follow && > + test_must_fail git log --follow -L1,1:b.c && These are really tests of --follow, but I don't mind seeing them here as reinforcement for the concepts that the commit message claims. > + # This may fail due to "-L is incompatible with pathspec", > + # or "-L is incompatible with pathspec". Either is acceptable. > + test_must_fail git log --follow -L1,1:b.c -- b.c Should one of those be "-L is incompatible with --follow"? Though of course we did not add such a check, so we know that it will be "-L is incompatible with pathspec", even without the --follow. -Peff
Jeff King <peff@peff.net> writes: >> +# Basic command line option parsing >> +test_expect_success '-L is incompatible with pathspec' ' >> + # This may fail due to "no such path a.c in commit", >> + # or "-L is incompatible with pathspec". Either is acceptable. >> + test_must_fail git log -L1,1:a.c -- a.c && > > This test confuses me. What are we looking for here? Presumably we'd > fail with: > > git log -L1,1:a.c > > too. If the test were "basic command line parsing", I could see checking > that. But that's only what the comment says. Yeah, I was undecided to have a single test that covers all (which I ended up with) or a sequence of individual tests (which I wrote on the title). >> + # This must fail due to "-L is incompatible with pathspec". >> + test_must_fail git log -L1,1:b.c -- b.c && > > Right, this is what we fixed. Would using test_i18ngrep on the stderr be > better than the comment? I do not care either way myself ;-) >> + # These must fail due to "follow requires one pathspec". >> + test_must_fail git log -L1,1:b.c --follow && >> + test_must_fail git log --follow -L1,1:b.c && > > These are really tests of --follow, but I don't mind seeing them here as > reinforcement for the concepts that the commit message claims. > >> + # This may fail due to "-L is incompatible with pathspec", >> + # or "-L is incompatible with pathspec". Either is acceptable. >> + test_must_fail git log --follow -L1,1:b.c -- b.c > > Should one of those be "-L is incompatible with --follow"? Though of > course we did not add such a check, so we know that it will be "-L is > incompatible with pathspec", even without the --follow. The comment seems utterly wrong here. I may reroll after taking a nap or something ;-)
diff --git a/builtin/log.c b/builtin/log.c index 0a7ed4bef9..9d70f3e60b 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -206,6 +206,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix, if (argc > 1) die(_("unrecognized argument: %s"), argv[1]); + if (rev->line_level_traverse && rev->prune_data.nr) + die(_("-L<range>:<file> cannot be used with pathspec")); + memset(&w, 0, sizeof(w)); userformat_find_requirements(NULL, &w); diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh index 2d1d7b5d19..3d1bd6ed80 100755 --- a/t/t4211-line-log.sh +++ b/t/t4211-line-log.sh @@ -8,6 +8,24 @@ test_expect_success 'setup (import history)' ' git reset --hard ' +# Basic command line option parsing +test_expect_success '-L is incompatible with pathspec' ' + # This may fail due to "no such path a.c in commit", + # or "-L is incompatible with pathspec". Either is acceptable. + test_must_fail git log -L1,1:a.c -- a.c && + + # This must fail due to "-L is incompatible with pathspec". + test_must_fail git log -L1,1:b.c -- b.c && + + # These must fail due to "follow requires one pathspec". + test_must_fail git log -L1,1:b.c --follow && + test_must_fail git log --follow -L1,1:b.c && + + # This may fail due to "-L is incompatible with pathspec", + # or "-L is incompatible with pathspec". Either is acceptable. + test_must_fail git log --follow -L1,1:b.c -- b.c +' + canned_test_1 () { test_expect_$1 "$2" " git log $2 >actual &&