git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Thomas Rast" <tr@thomasrast.ch>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"René Scharfe" <l.s.r@web.de>,
	"Philippe Blain" <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH 6/6] blame: enable funcname blaming with userdiff driver
Date: Thu, 29 Oct 2020 13:40:28 -0700	[thread overview]
Message-ID: <xmqqy2jo6a0z.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <a1e1c977d0978424fb07c97be0479f43a325cbea.1603889270.git.gitgitgadget@gmail.com> (Philippe Blain via GitGitGadget's message of "Wed, 28 Oct 2020 12:47:50 +0000")

"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> In blame.c::cmd_blame, we send the 'path' field of the 'sb' 'struct
> blame_scoreboard' as the 'path' argument to
> 'line-range.c::parse_range_arg', but 'sb.path' is not set yet; it's set
> to the local variable 'path' a few lines later at line 1137.
>
> This 'path' argument is only used in 'parse_range_arg' if we are blaming
> a funcname, i.e. `git blame -L :<funcname> <path>`, and in that case it
> is sent to 'parse_range_funcname', where it is used to determine if a
> userdiff driver should be used for said <path> to match the given
> funcname.
>
> Since 'path' is yet unset, the userdiff driver is never used, so we fall
> back to the default funcname regex, which is usually not appropriate for
> paths that are set to use a specific userdiff driver, and thus either we
> match some unrelated lines, or we die with
>
>     fatal: -L parameter '<funcname>' starting at line 1: no match
>
> This has been the case ever since `git blame` learned to blame a
> funcname in 13b8f68c1f (log -L: :pattern:file syntax to find by
> funcname, 2013-03-28).

Good find.

> Enable funcname blaming for paths using specific userdiff drivers by
> sending the local variable 'path' to 'parse_range_arg' instead of the
> yet unset 'sb.path'.

Hmph.  The reason why sb.path is not set to path at this point but
later gets set is?  I am wondering if this is leaving an unneeded
room for sb.path and path to diverge by mistake.  IOW, I wonder if
it is better to set sb.path as early as we figure out which path we
are interested in, forget we have local variable "path" and use
sb.path consistently throughout the code.

But that is merely a potential future clean-up.  The local variable
path is still used one more time in the error message given when
this parse_range_arg() fails, so at least this change makes the use
of path more consistent.  I like the simplicity of this fix.

> Add a regression test in 'annotate-tests.sh', which is sourced in
> t8001-annotate.sh and t8002-blame.sh, leveraging an existing file used
> to test the userdiff patterns in t4018-diff-funcname.

Thanks, that change also makes sense.


  reply	other threads:[~2020-10-29 20:41 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-28 12:47 [PATCH 0/6] " Philippe Blain via GitGitGadget
2020-10-28 12:47 ` [PATCH 1/6] doc: log, gitk: move '-L' description to 'line-range-options.txt' Philippe Blain via GitGitGadget
2020-10-29 20:16   ` Junio C Hamano
2020-10-31 17:18     ` Philippe Blain
2020-10-31 17:29       ` Junio C Hamano
2020-10-28 12:47 ` [PATCH 2/6] doc: line-range: improve formatting Philippe Blain via GitGitGadget
2020-10-28 17:23   ` Eric Sunshine
2020-10-29 20:19     ` Junio C Hamano
2020-10-31 17:20       ` Philippe Blain
2020-10-28 12:47 ` [PATCH 3/6] blame-options.txt: also mention 'funcname' in '-L' description Philippe Blain via GitGitGadget
2020-10-29 20:25   ` Junio C Hamano
2020-10-31 17:22     ` Philippe Blain
2020-10-28 12:47 ` [PATCH 4/6] doc: add more pointers to gitattributes(5) for userdiff Philippe Blain via GitGitGadget
2020-10-28 17:26   ` Eric Sunshine
2020-10-28 12:47 ` [PATCH 5/6] line-log: mention both modes in 'blame' and 'log' short help Philippe Blain via GitGitGadget
2020-10-28 17:33   ` Eric Sunshine
2020-10-29 12:43     ` Philippe Blain
2020-10-28 12:47 ` [PATCH 6/6] blame: enable funcname blaming with userdiff driver Philippe Blain via GitGitGadget
2020-10-29 20:40   ` Junio C Hamano [this message]
2020-10-31 18:02     ` Philippe Blain
2020-10-31 18:58       ` Junio C Hamano
2020-11-01 17:28 ` [PATCH v2 0/8] " Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 1/8] doc: log, gitk: move '-L' description to 'line-range-options.txt' Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 2/8] doc: line-range: improve formatting Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 3/8] blame-options.txt: also mention 'funcname' in '-L' description Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 4/8] doc: add more pointers to gitattributes(5) for userdiff Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 5/8] line-log: mention both modes in 'blame' and 'log' short help Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 6/8] blame: enable funcname blaming with userdiff driver Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 7/8] blame: simplify 'setup_scoreboard' interface Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 8/8] blame: simplify 'setup_blame_bloom_data' interface Philippe Blain via GitGitGadget

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=xmqqy2jo6a0z.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=l.s.r@web.de \
    --cc=levraiphilippeblain@gmail.com \
    --cc=sunshine@sunshineco.com \
    --cc=tr@thomasrast.ch \
    --subject='Re: [PATCH 6/6] blame: enable funcname blaming with userdiff driver' \
    /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

Code repositories for project(s) associated with this 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).