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

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Oct 31, 2020, 18:02 UTC
Message-ID: <81143637-F77C-49C5-B55A-57E92AC45881@gmail.com>
In-Reply-To: <xmqqy2jo6a0z.fsf@gitster.c.googlers.com>

Show 40 quoted lines

> Le 29 oct. 2020 à 16:40, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "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.

I asked myself the same question, and could not come up with a good answer. I don't know if you read what I wrote in the cover letter (which I would have included in the in-patch commentary for this here patch if GitGitGadget had this feature), but I'm copying it here just in case you did not:

* In patch 6, I considered fixing that bug in a different way by
  initializing sb.path inside blame.c::setup_scoreboard. This function
  already receives 'path' as its second argument, so changing that would not
  impact the API. Probably Thomas thought that sb.path was already
  initialized in sb when he modified builtin/blame.c::prepare_blame_range 
  to also send sb.path to line-range.c::parse_range_arg in 13b8f68c1f (log
  -L: :pattern:file syntax to find by funcname, 2013-03-28).

  Initializing the path in setup_scoreboard would mean we could also
  simplify the API of blame.c::setup_blame_bloom_data since it would not
  need to receive path separately and so its second argument could be
  removed. I went for the simpler alternative of just sending 'path' to 
  parse_range_arg instead of sb.path since it felt simpler, but if we feel
  it would be better to actually initialize sb.path in setup_scoreboard,
  I'll gladly tweak that for v2.

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

I also like its simplicity, and that's why I chose to submit this as v1. But I completely agree with you that it is "dangerous" in the sense that some further modifications to the code could then make the same mistake and use 'sb.path' thinking it is defined when it is not.

So I'm thinking of instead initializing it in setup_scoreboard for v2.

Cheers,

Philippe.