git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git mailing list" <git@vger.kernel.org>,
	"Thomas Rast" <tr@thomasrast.ch>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"René Scharfe" <l.s.r@web.de>
Subject: Re: [PATCH 6/6] blame: enable funcname blaming with userdiff driver
Date: Sat, 31 Oct 2020 14:02:16 -0400	[thread overview]
Message-ID: <81143637-F77C-49C5-B55A-57E92AC45881@gmail.com> (raw)
In-Reply-To: <xmqqy2jo6a0z.fsf@gitster.c.googlers.com>


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

  reply	other threads:[~2020-10-31 18:02 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
2020-10-31 18:02     ` Philippe Blain [this message]
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=81143637-F77C-49C5-B55A-57E92AC45881@gmail.com \
    --to=levraiphilippeblain@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --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).