git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Sathyajith Bhat <sathya@sathyasays.com>, git@vger.kernel.org
Subject: Re: Segfault in git when using git logs
Date: Tue, 3 Nov 2020 11:15:53 +0100	[thread overview]
Message-ID: <20201103101553.GH24813@szeder.dev> (raw)
In-Reply-To: <20201102144321.GA3962443@coredump.intra.peff.net>

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.


  parent reply	other threads:[~2020-11-03 10:16 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-02 13:59 Segfault in git when using git logs Sathyajith Bhat
2020-11-02 14:43 ` Jeff King
2020-11-02 18:31   ` Junio C Hamano
2020-11-03 10:15   ` SZEDER Gábor [this message]
2020-11-03 11:21     ` Christian Couder
2020-11-03 16:10       ` Elijah Newren
2020-11-03 18:21     ` Jeff King
2020-11-03 18:34       ` Junio C Hamano
2020-11-03 18:57         ` Jeff King
2020-11-03 20:21           ` Junio C Hamano
2020-11-04 13:31             ` Jeff King
2020-11-04 16:26               ` Junio C Hamano
2020-11-04 17:54             ` Re*: " Junio C Hamano
2020-11-04 19:41               ` Jeff King
2020-11-04 20:16                 ` Junio C Hamano
2020-11-04 20:35                   ` [PATCH] log: diagnose -L used with pathspec as an error Junio C Hamano
2020-11-04 21:03                     ` Jeff King
2020-11-03 18:46 ` Segfault in git when using git logs Derrick Stolee
2020-11-03 18:55   ` Sathyajith Bhat
2020-11-03 19:23     ` Jeff King
2020-11-03 20:07       ` Derrick Stolee
2020-11-03 21:04         ` Derrick Stolee
2020-11-04 15:49           ` Sathyajith Bhat

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201103101553.GH24813@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sathya@sathyasays.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).