Re: [PATCH 6/6] blame: enable funcname blaming with userdiff driver

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 29, 2020, 20:40 UTC
Message-ID: <xmqqy2jo6a0z.fsf@gitster.c.googlers.com>
In-Reply-To: <a1e1c977d0978424fb07c97be0479f43a325cbea.1603889270.git.gitgitgadget@gmail.com>

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

Show 23 quoted lines

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