Segfault in git when using git logs

Segfault in git when using git logs

[Sathyajith Bhat]

Hi,

My code editor kept crashing repeatedly and when poked in syslogs, I
found that git was segfaulting. I can repro this easily (steps below).
This is the first time I'm reporting a bug here so I apologize if I've
gotten anything wrong.

Below is the output from git bugreport

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

Try to find commit logs for a file. I can repro this on a new repo as
well as an existing repo.

Simple repro steps

        mkdir git_segfault_test && cd git_segfault_test && echo
"Hello" > hello.log
        git init && git add hello.log && git commit -m "init commit"

Now, use git log to show commit logs using command

        git log  --follow -L 1,1:hello.log -- hello.log

What did you expect to happen? (Expected behavior)
Git should not segfault

What happened instead? (Actual behavior)
Git segfaulted

What's different between what you expected and what actually happened?

Anything else you want to add:
Note that I am not calling the git log commands directly, my editor VS
Code was repeatedly crashing and I searched in syslog and found this
error.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.29.2
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.9.2-arch1-1 #1 SMP PREEMPT Thu, 29 Oct 2020 17:01:28 +0000 x86_64
compiler info: gnuc: 10.2
libc info: glibc: 2.32
$SHELL (typically, interactive shell): /usr/bin/zsh


[Enabled Hooks]



 -- Sathya
http://sathyabh.at | http://sbhat.me
Author, Practical Docker with Python & AWS Community Hero

Re: Segfault in git when using git logs

[Jeff King]

On Mon, Nov 02, 2020 at 03:59:59PM +0200, Sathyajith Bhat wrote:

> Simple repro steps
> 
>         mkdir git_segfault_test && cd git_segfault_test && echo
> "Hello" > hello.log
>         git init && git add hello.log && git commit -m "init commit"
> 
> Now, use git log to show commit logs using command
> 
>         git log  --follow -L 1,1:hello.log -- hello.log
> 
> What did you expect to happen? (Expected behavior)
> Git should not segfault

Thanks for making this reproduction recipe! I can easily see the problem
on my system. Looks like the segfault was introduced by a2bb801f6a
(line-log: avoid unnecessary full tree diffs, 2019-08-21). I've cc'd the
author.

That commit causes the line-log to clear the set of pathspecs, but the
--follow option requires exactly one pathspec (and it even makes sure
the user gives us one, but that happens before we clear it internally).
Something like this makes the segfault go away:

diff --git a/line-log.c b/line-log.c
index 42c5e41f68..f789863928 100644
--- a/line-log.c
+++ b/line-log.c
@@ -847,6 +847,7 @@ static void queue_diffs(struct line_log_data *range,
 		clear_pathspec(&opt->pathspec);
 		parse_pathspec_from_ranges(&opt->pathspec, range);
 	}
+	opt->flags.follow_renames = 0;
 	DIFF_QUEUE_CLEAR(&diff_queued_diff);
 	diff_tree_oid(parent_tree_oid, tree_oid, "", opt);
 	if (opt->detect_rename && diff_might_be_rename()) {

but I'm not clear on how "--follow" and "-L" are supposed to interact. I
wouldn't expect --follow to do anything at all with line-log (nor for it
to be useful to specify pathspecs outside of the -L option). So possibly
this is restoring the behavior prior to that commit, or possibly it's
just papering over a breakage. ;)

-Peff

Re: Segfault in git when using git logs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That commit causes the line-log to clear the set of pathspecs, but the
> --follow option requires exactly one pathspec (and it even makes sure
> the user gives us one, but that happens before we clear it internally).
> Something like this makes the segfault go away:
>
> diff --git a/line-log.c b/line-log.c
> index 42c5e41f68..f789863928 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -847,6 +847,7 @@ static void queue_diffs(struct line_log_data *range,
>  		clear_pathspec(&opt->pathspec);
>  		parse_pathspec_from_ranges(&opt->pathspec, range);
>  	}
> +	opt->flags.follow_renames = 0;
>  	DIFF_QUEUE_CLEAR(&diff_queued_diff);
>  	diff_tree_oid(parent_tree_oid, tree_oid, "", opt);
>  	if (opt->detect_rename && diff_might_be_rename()) {
>
> but I'm not clear on how "--follow" and "-L" are supposed to interact. I
> wouldn't expect --follow to do anything at all with line-log (nor for it
> to be useful to specify pathspecs outside of the -L option). So possibly
> this is restoring the behavior prior to that commit, or possibly it's
> just papering over a breakage. ;)

Another option is to catch it as "these options are mutually
exclusive" error early before the control reaches this deep in the
codeflow, I would think, but I suspect that some people may
habitually add the "--follow" option in a context where it does not
make sense, so "--follow is silently ignored when other options that
contradict it is in effect at the same time" is OK by me, too.

I do not know if that is the case offhand, but in the ideal
world, I would imagine

	git log -L1,5:hello.c -C -C -- hello.c goodbye.c
	git log -L1,5:hello.c -C -C

to notice and show that some of these five lines were copied
when or after hello.c was created, and keep following the
origin of them in goodbye.c, and

	git log -L1,5:hello.c -C -C

may do the same or find better match outside goodbye.c for
the origin of these lines and follow them instead, while

	git log -L1,5:hello.c -- hello.c
	git log -L1,5:hello.c --no-renames

in the same history may say the commit that actually copied
these lines from goodbye.c just added them directly to hello.c
instead.

And to extend the imagination a bit more,

	git log -M -L1,5:hello.c
	git log -L1,5:hello.c
	git log --follow -L1,5:hello.c

in a different history may notice that hello.c was created
wholesale by renaming hola.c and follow the changes to these
five lines down the history.  As -M is in effect by default
these days, the first two would be equivalent, and "--follow"
would be a meaningless noiseword in the context of this example
where we are interested only in a single path hello.c in the
end result, but in the ideal world, meaningless noiseword
should become hardless no-op, I would think.

Of course, the above assumes that -L plays well with the
-M/-C/--follow options and pathspec.  If not, then "they are
incompatible" would be the more sensible and easy-to-explain
option.

Thanks.

Re: Segfault in git when using git logs

[SZEDER Gábor]

On Mon, Nov 02, 2020 at 09:43:21AM -0500, Jeff King wrote:
> On Mon, Nov 02, 2020 at 03:59:59PM +0200, Sathyajith Bhat wrote:
> 
> > Simple repro steps
> > 
> >         mkdir git_segfault_test && cd git_segfault_test && echo
> > "Hello" > hello.log
> >         git init && git add hello.log && git commit -m "init commit"
> > 
> > Now, use git log to show commit logs using command
> > 
> >         git log  --follow -L 1,1:hello.log -- hello.log

While Git should never segfault, no matter what, this is a bogus git
invocation to begin with: the second sentence in the description of
'git log -L' clearly states that "You may not give any pathspec
limiters", so this command should have errored out from early days,
but, unfortunately, it was never enforced.  This also means that '-L'
and '--follow' are incompatible, because while the former forbids any
pathspecs, the latter requires exactly one; and line-level
log does its own rename following anyway.

VS Code should be fixed to call 'git log -L 1,1:hello.log' instead,
without '--follow' and without pathspec.

> > What did you expect to happen? (Expected behavior)
> > Git should not segfault
> 
> Thanks for making this reproduction recipe! I can easily see the problem
> on my system. Looks like the segfault was introduced by a2bb801f6a
> (line-log: avoid unnecessary full tree diffs, 2019-08-21). I've cc'd the
> author.
> 
> That commit causes the line-log to clear the set of pathspecs, but the
> --follow option requires exactly one pathspec (and it even makes sure
> the user gives us one, but that happens before we clear it internally).
> Something like this makes the segfault go away:
> 
> diff --git a/line-log.c b/line-log.c
> index 42c5e41f68..f789863928 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -847,6 +847,7 @@ static void queue_diffs(struct line_log_data *range,
>  		clear_pathspec(&opt->pathspec);
>  		parse_pathspec_from_ranges(&opt->pathspec, range);
>  	}
> +	opt->flags.follow_renames = 0;
>  	DIFF_QUEUE_CLEAR(&diff_queued_diff);
>  	diff_tree_oid(parent_tree_oid, tree_oid, "", opt);
>  	if (opt->detect_rename && diff_might_be_rename()) {
> 
> but I'm not clear on how "--follow" and "-L" are supposed to interact.

They shouldn't, I would say.  Though it would be great if their
rename-following logic would be unified.  In particular, line-level
log does a better job at rename following in some ways, notably it can
track multiple files at once, while '--follow' can only handle a
single file.  So I think the rename following logic should be
extracted from 'line-log.c' and made more generic, and it should be
used to implement '--follow', removing some restrictions of the
latter, not to mention removing the duplicated logic.

(This might be a good GSoC project, though some of Linus' remarks in
750f7b668f (Finally implement "git log --follow", 2007-06-19) like
"you did have to know and understand the internal git diff generation
machinery pretty well, and had to really be able to follow how commit
generation interacts with generating patches and generating the log"
and "this patch does seem to be firmly in the core "Linus or Junio"
territory" are worrying...)

> I
> wouldn't expect --follow to do anything at all with line-log (nor for it
> to be useful to specify pathspecs outside of the -L option). So possibly
> this is restoring the behavior prior to that commit, or possibly it's
> just papering over a breakage. ;)

Perhaps, though arguably the original breakage was that 'git log
-L...:file -- file' was meant to error out, but it didn't.

Re: Segfault in git when using git logs

[Christian Couder]

On Tue, Nov 3, 2020 at 11:19 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Mon, Nov 02, 2020 at 09:43:21AM -0500, Jeff King wrote:

> > but I'm not clear on how "--follow" and "-L" are supposed to interact.
>
> They shouldn't, I would say.  Though it would be great if their
> rename-following logic would be unified.  In particular, line-level
> log does a better job at rename following in some ways, notably it can
> track multiple files at once, while '--follow' can only handle a
> single file.  So I think the rename following logic should be
> extracted from 'line-log.c' and made more generic, and it should be
> used to implement '--follow', removing some restrictions of the
> latter, not to mention removing the duplicated logic.
>
> (This might be a good GSoC project, though some of Linus' remarks in
> 750f7b668f (Finally implement "git log --follow", 2007-06-19) like
> "you did have to know and understand the internal git diff generation
> machinery pretty well, and had to really be able to follow how commit
> generation interacts with generating patches and generating the log"
> and "this patch does seem to be firmly in the core "Linus or Junio"
> territory" are worrying...)

Thanks for the suggestion!

For the Outreachy round starting next December, we have proposed a
project to accelerate rename detection and range-diff based on:

https://github.com/gitgitgadget/git/issues/519

I am not sure how much it is related to this though.

Re: Segfault in git when using git logs

[Elijah Newren]

On Tue, Nov 3, 2020 at 3:21 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Tue, Nov 3, 2020 at 11:19 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > On Mon, Nov 02, 2020 at 09:43:21AM -0500, Jeff King wrote:
>
> > > but I'm not clear on how "--follow" and "-L" are supposed to interact.
> >
> > They shouldn't, I would say.  Though it would be great if their
> > rename-following logic would be unified.  In particular, line-level
> > log does a better job at rename following in some ways, notably it can
> > track multiple files at once, while '--follow' can only handle a
> > single file.  So I think the rename following logic should be
> > extracted from 'line-log.c' and made more generic, and it should be
> > used to implement '--follow', removing some restrictions of the
> > latter, not to mention removing the duplicated logic.
> >
> > (This might be a good GSoC project, though some of Linus' remarks in
> > 750f7b668f (Finally implement "git log --follow", 2007-06-19) like
> > "you did have to know and understand the internal git diff generation
> > machinery pretty well, and had to really be able to follow how commit
> > generation interacts with generating patches and generating the log"
> > and "this patch does seem to be firmly in the core "Linus or Junio"
> > territory" are worrying...)
>
> Thanks for the suggestion!
>
> For the Outreachy round starting next December, we have proposed a
> project to accelerate rename detection and range-diff based on:
>
> https://github.com/gitgitgadget/git/issues/519
>
> I am not sure how much it is related to this though.

As worded, the project is only tangentially related.  If someone were
to plumb better rename following through, then this Outreachy project
might make the underlying internals faster, but that's it.  However,
it's a certainly an interesting related project.

Also, I'll note that there is some code coming with merge-ort that
should help write an improved --follow implementation, and, in fact,
was specially added with that goal in mind.  merge-ort adds to
diffcore-rename.c the ability to limit rename detection to either
relevant_sources (e.g. only consider removed filenames matching these
paths for rename detection) or relevant_targets (e.g. only include
added files matching these paths for rename detection), or both.  The
relevant_targets was not useful within the merge-ort implementation,
but since I was adding relevant_sources it seemed like a natural
pairing; I added it for completeness and was specifically thinking how
it would be useful for log --follow when I did so.  (I also think the
strmap API will also make it a bit easier to implement an improved
--follow.)  One of my goals was to use the relevant_targets ability as
part of an improved log --follow implementation after getting the rest
of the merge-ort stuff done, but it might be a useful intern project
instead.

Re: Segfault in git when using git logs

[Jeff King]

On Tue, Nov 03, 2020 at 11:15:53AM +0100, SZEDER Gábor wrote:

> > > Now, use git log to show commit logs using command
> > > 
> > >         git log  --follow -L 1,1:hello.log -- hello.log
> 
> While Git should never segfault, no matter what, this is a bogus git
> invocation to begin with: the second sentence in the description of
> 'git log -L' clearly states that "You may not give any pathspec
> limiters", so this command should have errored out from early days,
> but, unfortunately, it was never enforced.  This also means that '-L'
> and '--follow' are incompatible, because while the former forbids any
> pathspecs, the latter requires exactly one; and line-level
> log does its own rename following anyway.

Thanks for confirming. My "I am not clear how these should interact" was
really "this does not make any sense to me" in my head, but I was afraid
that I was missing something. The fact that we document explicitly that
-L should not be combined with pathspecs makes that much more obvious.

> VS Code should be fixed to call 'git log -L 1,1:hello.log' instead,
> without '--follow' and without pathspec.

Agreed.

On our side, I don't think it would be _wrong_ to catch and disallow the
combination. But it may be nicer to them if we continue to quietly
ignore --follow and the pathspec in that case, for working with older
versions. (OTOH, if I understand correctly they're segfaulting every
time VS Code is used with v2.29 now, so they may have to accept it as an
urgent fix anyway).

-Peff

Re: Segfault in git when using git logs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 03, 2020 at 11:15:53AM +0100, SZEDER Gábor wrote:
>
>> > > Now, use git log to show commit logs using command
>> > > 
>> > >         git log  --follow -L 1,1:hello.log -- hello.log
>> 
>> While Git should never segfault, no matter what, this is a bogus git
>> invocation to begin with: the second sentence in the description of
>> 'git log -L' clearly states that "You may not give any pathspec
>> limiters", so this command should have errored out from early days,
>> but, unfortunately, it was never enforced.  This also means that '-L'
>> and '--follow' are incompatible, because while the former forbids any
>> pathspecs, the latter requires exactly one; and line-level
>> log does its own rename following anyway.
>
> Thanks for confirming. My "I am not clear how these should interact" was
> really "this does not make any sense to me" in my head, but I was afraid
> that I was missing something. The fact that we document explicitly that
> -L should not be combined with pathspecs makes that much more obvious.
>
>> VS Code should be fixed to call 'git log -L 1,1:hello.log' instead,
>> without '--follow' and without pathspec.
>
> Agreed.
>
> On our side, I don't think it would be _wrong_ to catch and disallow the
> combination. But it may be nicer to them if we continue to quietly
> ignore --follow and the pathspec in that case, for working with older
> versions. (OTOH, if I understand correctly they're segfaulting every
> time VS Code is used with v2.29 now, so they may have to accept it as an
> urgent fix anyway).

So something like this won't harm VS Code more than we currently do,
while telling users what is wrong with their command line?

We may still want the "silently disable follow" at low-level
protection, but that does not give feedback why the end-user option
is silently ignored, so...

 builtin/log.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git c/builtin/log.c w/builtin/log.c
index 9f939e6cdf..8811084f02 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -206,6 +206,13 @@ 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) {
+		if (rev->diffopt.filter)
+			die(_("-L<range>:<file> cannot be used with pathspec"));
+		if (rev->diffopt.flags.follow_renames)
+			die(_("-L<range>:<file> cannot be used with --follow"));
+	}
+
 	memset(&w, 0, sizeof(w));
 	userformat_find_requirements(NULL, &w);
 

Re: Segfault in git when using git logs

[Jeff King]

On Tue, Nov 03, 2020 at 10:34:09AM -0800, Junio C Hamano wrote:

> > On our side, I don't think it would be _wrong_ to catch and disallow the
> > combination. But it may be nicer to them if we continue to quietly
> > ignore --follow and the pathspec in that case, for working with older
> > versions. (OTOH, if I understand correctly they're segfaulting every
> > time VS Code is used with v2.29 now, so they may have to accept it as an
> > urgent fix anyway).
> 
> So something like this won't harm VS Code more than we currently do,
> while telling users what is wrong with their command line?

Yeah, I was wondering if we'd want the patch you sent, or if we should
turn those die() calls into warning() and disable the flags up front.

> We may still want the "silently disable follow" at low-level
> protection, but that does not give feedback why the end-user option
> is silently ignored, so...

I'd be just as happy to leave it out, if we think it isn't triggerable.
The segfault would let people know we missed a spot. ;)

> diff --git c/builtin/log.c w/builtin/log.c
> index 9f939e6cdf..8811084f02 100644
> --- c/builtin/log.c
> +++ w/builtin/log.c
> @@ -206,6 +206,13 @@ 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) {
> +		if (rev->diffopt.filter)
> +			die(_("-L<range>:<file> cannot be used with pathspec"));

Should this be checking rev->diffopt.pathspec.nr?

I could well believe that --diff-filter=A does not work with "-L"
either, but that is a separate story.

-Peff

Re: Segfault in git when using git logs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 03, 2020 at 10:34:09AM -0800, Junio C Hamano wrote:
>
>> > On our side, I don't think it would be _wrong_ to catch and disallow the
>> > combination. But it may be nicer to them if we continue to quietly
>> > ignore --follow and the pathspec in that case, for working with older
>> > versions. (OTOH, if I understand correctly they're segfaulting every
>> > time VS Code is used with v2.29 now, so they may have to accept it as an
>> > urgent fix anyway).
>> 
>> So something like this won't harm VS Code more than we currently do,
>> while telling users what is wrong with their command line?
>
> Yeah, I was wondering if we'd want the patch you sent, or if we should
> turn those die() calls into warning() and disable the flags up front.
>
>> We may still want the "silently disable follow" at low-level
>> protection, but that does not give feedback why the end-user option
>> is silently ignored, so...
>
> I'd be just as happy to leave it out, if we think it isn't triggerable.
> The segfault would let people know we missed a spot. ;)
>
>> diff --git c/builtin/log.c w/builtin/log.c
>> index 9f939e6cdf..8811084f02 100644
>> --- c/builtin/log.c
>> +++ w/builtin/log.c
>> @@ -206,6 +206,13 @@ 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) {
>> +		if (rev->diffopt.filter)
>> +			die(_("-L<range>:<file> cannot be used with pathspec"));
>
> Should this be checking rev->diffopt.pathspec.nr?

Embarrassing but yes ;-).

Re: Segfault in git when using git logs

[Jeff King]

On Tue, Nov 03, 2020 at 12:21:42PM -0800, Junio C Hamano wrote:

> >> So something like this won't harm VS Code more than we currently do,
> >> while telling users what is wrong with their command line?
> >
> > Yeah, I was wondering if we'd want the patch you sent, or if we should
> > turn those die() calls into warning() and disable the flags up front.
> [...]
> >> diff --git c/builtin/log.c w/builtin/log.c

Since we now have identified the caller that was passing in the bogus
options, and it has already been fixed, I'm inclined to say we should go
with your die() patch from earlier (modulo the pathspec.nr fixup) and
call it done.

Do you want to roll that up with a commit message?

-Peff

Re: Segfault in git when using git logs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 03, 2020 at 12:21:42PM -0800, Junio C Hamano wrote:
>
>> >> So something like this won't harm VS Code more than we currently do,
>> >> while telling users what is wrong with their command line?
>> >
>> > Yeah, I was wondering if we'd want the patch you sent, or if we should
>> > turn those die() calls into warning() and disable the flags up front.
>> [...]
>> >> diff --git c/builtin/log.c w/builtin/log.c
>
> Since we now have identified the caller that was passing in the bogus
> options, and it has already been fixed, I'm inclined to say we should go
> with your die() patch from earlier (modulo the pathspec.nr fixup) and
> call it done.
>
> Do you want to roll that up with a commit message?

OK, will try.

Re*: Segfault in git when using git logs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>> +	if (rev->line_level_traverse) {
>>> +		if (rev->diffopt.filter)
>>> +			die(_("-L<range>:<file> cannot be used with pathspec"));
>>
>> Should this be checking rev->diffopt.pathspec.nr?
>
> Embarrassing but yes ;-).

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.

-- >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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c       |  3 +++
 t/t4211-line-log.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

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 &&
-- 
2.29.2-287-gba574db674

Re: Re*: Segfault in git when using git logs

[Jeff King]

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

Re: Re*: Segfault in git when using git logs

[Junio C Hamano]

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 ;-)

[PATCH] log: diagnose -L used with pathspec as an error

[Junio C Hamano]

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.

The new tests say they may fail with "-L and --follow being
incompatible" instead of "-L and pathspec being imcompatible".
Currently the expected failure can come only from the latter, but
this is to futureproof them, in case we decide to add code to
explicititly die on -L and --follow used together.

Heled-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c       |  3 +++
 t/t4211-line-log.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

 * This time, as a standalone patch, instead of a comment in a
   discussion thread.

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..b85c4a8a04 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -8,6 +8,32 @@ test_expect_success 'setup (import history)' '
 	git reset --hard
 '
 
+test_expect_success 'basic command line parsing' '
+	# This may fail due to "no such path a.c in commit", or
+	# "-L is incompatible with pathspec", depending on the
+	# order the error is checked.  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 2>error &&
+	test_i18ngrep "cannot be used with pathspec" error &&
+
+	# Note that incompatibility between -L/--follow is not
+	# explicitly checked to avoid redundant code and the comments
+	# on the following tests are merely for future-proofing.
+
+	# These must fail due to "follow requires one pathspec", or
+	# "-L is incompatible with --follow", depending on the
+	# order the error is checked.  Either is acceptable.
+	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 --follow", depending on the
+	# order the error is checked.  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 &&
-- 
2.29.2-287-gba574db674

Re: [PATCH] log: diagnose -L used with pathspec as an error

[Jeff King]

On Wed, Nov 04, 2020 at 12:35:10PM -0800, Junio C Hamano wrote:

> The new tests say they may fail with "-L and --follow being
> incompatible" instead of "-L and pathspec being imcompatible".
> Currently the expected failure can come only from the latter, but
> this is to futureproof them, in case we decide to add code to
> explicititly die on -L and --follow used together.

This explanation makes sense (though s/imcompat/incompat/).

> +test_expect_success 'basic command line parsing' '
> +	# This may fail due to "no such path a.c in commit", or
> +	# "-L is incompatible with pathspec", depending on the
> +	# order the error is checked.  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 2>error &&
> +	test_i18ngrep "cannot be used with pathspec" error &&

The renaming makes sense...

> +
> +	# Note that incompatibility between -L/--follow is not
> +	# explicitly checked to avoid redundant code and the comments
> +	# on the following tests are merely for future-proofing.

...as does this comment to explain the rest of the tests.

> +	# These must fail due to "follow requires one pathspec", or
> +	# "-L is incompatible with --follow", depending on the
> +	# order the error is checked.  Either is acceptable.
> +	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 --follow", depending on the
> +	# order the error is checked.  Either is acceptable.
> +	test_must_fail git log --follow -L1,1:b.c -- b.c
> +'

Though "depending on the order" is a bit of a fiction, because those
checks do not exist at all. I'm OK with it because the earlier comment
explains what is going. I guess:

  # This may fail due to "-L is incompatible with pathspec", or
  # "-L is incompatible with --follow". We don't have the latter as of
  # the writing of this test, but either would be acceptable if we added
  # it.

would be an alternative. I doubt it's worth spending too much time
polishing.

-Peff

Re: Segfault in git when using git logs

[Derrick Stolee]

On 11/2/2020 8:59 AM, Sathyajith Bhat wrote:
> Anything else you want to add:

Hi Sathyajith,

It seems like others have some good approaches to handling
this segfault and making sure we find the right balance
between supporting existing behavior and helping callers
who might be using this command incorrectly.

> Note that I am not calling the git log commands directly, my editor VS
> Code was repeatedly crashing and I searched in syslog and found this
> error.

I started filling out an issue on the VS Code GitHub repo
so they could perhaps fix their usage of "git log" in this case.
However, I started searching their code for where they might
be executing the command, and I didn't find anything!

Is it possible that you have an extension that enables Git
history commands like this?

The VS Code bug report issue template [1] has this bit of
advice to find out if an extension is at fault:

> <!-- Launch with `code --disable-extensions` to check. -->
> Does this issue occur when all extensions are disabled?: Yes/No

[1] https://github.com/microsoft/vscode/issues/new?template=bug_report.md

Could you re-launch your editor with "--disable-extensions" to
see if this is reproduced? Also, which extensions do you have
installed that might be adding Git command calls? We might need
to find the extension authors instead of the core editor team.

Thanks,
-Stolee

Re: Segfault in git when using git logs

[Sathyajith Bhat]

On Tue, 3 Nov 2020 at 20:46, Derrick Stolee <stolee@gmail.com> wrote:

> Hi Sathyajith,
> I started filling out an issue on the VS Code GitHub repo
> so they could perhaps fix their usage of "git log" in this case.
> However, I started searching their code for where they might
> be executing the command, and I didn't find anything!
>
> Is it possible that you have an extension that enables Git
> history commands like this?

Hi Derek,

I believe it is GitLens [1] which is causing the incorrect behaviour.
I haven't had any crashes after I disabled GitLens, and that's the
only extension I've had which interacts with scm.

[1] https://github.com/eamodio/vscode-gitlens

thanks,
Sathya

Re: Segfault in git when using git logs

[Jeff King]

On Tue, Nov 03, 2020 at 08:55:52PM +0200, Sathyajith Bhat wrote:

> I believe it is GitLens [1] which is causing the incorrect behaviour.
> I haven't had any crashes after I disabled GitLens, and that's the
> only extension I've had which interacts with scm.
> 
> [1] https://github.com/eamodio/vscode-gitlens

That makes sense. It seems pretty clear that its log__file() function
will produce this bogus combination. Despite having some indication that
it knows about the documentation forbidding it:

  https://github.com/eamodio/vscode-gitlens/blob/6cfd9fdedd7c6ec3bfa732af7c418bbbecdfba54/src/git/git.ts#L805

I don't know that project's code well enough to say whether there is a
higher-level bug there (the issue seems to be the "renames" flag being
enabled along with startLine; maybe the caller is wrong to specify
both).

-Peff

Re: Segfault in git when using git logs

[Derrick Stolee]

On 11/3/2020 2:23 PM, Jeff King wrote:
> On Tue, Nov 03, 2020 at 08:55:52PM +0200, Sathyajith Bhat wrote:
> 
>> I believe it is GitLens [1] which is causing the incorrect behaviour.
>> I haven't had any crashes after I disabled GitLens, and that's the
>> only extension I've had which interacts with scm.
>>
>> [1] https://github.com/eamodio/vscode-gitlens
> 
> That makes sense. It seems pretty clear that its log__file() function
> will produce this bogus combination. Despite having some indication that
> it knows about the documentation forbidding it:
> 
>   https://github.com/eamodio/vscode-gitlens/blob/6cfd9fdedd7c6ec3bfa732af7c418bbbecdfba54/src/git/git.ts#L805
> 
> I don't know that project's code well enough to say whether there is a
> higher-level bug there (the issue seems to be the "renames" flag being
> enabled along with startLine; maybe the caller is wrong to specify
> both).

Thanks, both, for confirming the extension and the likely
line of code causing this problem. I submitted an issue [1]
on the GitLens repository. I'll continue watching it, but
feel free to chime in yourself if you are interested.

[1] https://github.com/eamodio/vscode-gitlens/issues/1139

Thanks,
-Stolee

Re: Segfault in git when using git logs

[Derrick Stolee]

On 11/3/2020 3:07 PM, Derrick Stolee wrote:
> Thanks, both, for confirming the extension and the likely
> line of code causing this problem. I submitted an issue [1]
> on the GitLens repository. I'll continue watching it, but
> feel free to chime in yourself if you are interested.
> 
> [1] https://github.com/eamodio/vscode-gitlens/issues/1139

The issue has been fixed in GitLens, so please update your
extension to verify.

[1] https://github.com/eamodio/vscode-gitlens/commit/d980c7b831961f932ae68ed6e8ab08f71983b9a9
[2] https://twitter.com/eamodio/status/1323729485338914817?s=20

Thanks,
-Stolee

Re: Segfault in git when using git logs

[Sathyajith Bhat]

On Tue, 3 Nov 2020 at 23:04, Derrick Stolee <stolee@gmail.com> wrote:

> The issue has been fixed in GitLens, so please update your
> extension to verify.
>
> [1] https://github.com/eamodio/vscode-gitlens/commit/d980c7b831961f932ae68ed6e8ab08f71983b9a9
> [2] https://twitter.com/eamodio/status/1323729485338914817?s=20

I re-enabled the extension today and was able to get by the whole day
without any crashes - well, there was one but I couldn't repro it or
find the logs for the same.

Thank you folks for reporting to Gitlens, truly appreciate the work
goes behind the scenes

Cheers,
Sathya