patch, 6 partsRe: [PATCH 5/6] line-log: mention both modes in 'blame' and 'log' short help

This series fixes a bug in git blame where any userdiff driver configured via the attributes mechanism is ignored, which more often than not means thatgit blame -L :<funcname> dies for paths that have a configured userdiff driver. That's patch 6/6.

Patches 1-5 are documentation improvements around the line-log functionality.

Notes (I can't add in-patch commentaries using GitGitGadget (yet, I hope 1 [https://github.com/gitgitgadget/gitgitgadget/issues/173]), so I'm providing what would have been an in-patch commentary on patch 6 below):

 * 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.
   
   
 * The fact that this bug has been present ever since git blame learned -L
   :<funcname> makes me a little sad. I think that the fact that the diff 
   attribute (either with a builtin userdiff pattern or with 
   diff.*.xfuncname ) influences way more than just the hunk header, but
   also features like git log -L, git blame -L, and the different options
   that I mention in patch 4/6, is not well-known enough. I'm not saying the
   code or the doc is wrong, but I think it's a visibility issue. Maybe more
   blog posts about that would help.... I noticed that the "Git Attributes"
   chapter in the ProGit book 2
   [https://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes] does not
   even mention the builtin userdiff patterns, so I'll probably do a PR
   there to mention it. Any other ideas for improving discoverability of
   this very cool feature ?

Philippe Blain (6):
  doc: log, gitk: move '-L' description to 'line-range-options.txt'
  doc: line-range: improve formatting
  blame-options.txt: also mention 'funcname' in '-L' description
  doc: add more pointers to gitattributes(5) for userdiff
  line-log: mention both modes in 'blame' and 'log' short help
  blame: enable funcname blaming with userdiff driver

 Documentation/blame-options.txt      |  9 +++++----
 Documentation/diff-options.txt       |  5 ++++-
 Documentation/git-grep.txt           |  6 ++++--
 Documentation/git-log.txt            | 16 ++--------------
 Documentation/gitk.txt               | 21 ++-------------------
 Documentation/line-range-format.txt  | 28 +++++++++++++++-------------
 Documentation/line-range-options.txt | 27 +++++++++++++++++++++++++++
 builtin/blame.c                      |  5 +++--
 builtin/log.c                        |  4 ++--
 t/annotate-tests.sh                  | 18 ++++++++++++++++++
 10 files changed, 82 insertions(+), 57 deletions(-)
 create mode 100644 Documentation/line-range-options.txt

base-commit: 1d1c4a875900d69c7f0a31e44c3e370dc80ab1ce
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-774%2Fphil-blain%2Fblame-funcname-userdiff-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-774/phil-blain/blame-funcname-userdiff-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/774

From: Philippe Blain <levraiphilippeblain@gmail.com>

Make it clearer that a function can be blamed by feeding `git blame` '-L :<funcname>' by mentioning it at the beginnning of the description of the '-L' option.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/blame-options.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

I think it makes tons of sense to mention both <start>,<end> range and <funcname> range in the description.

It is not a new issue you are introducing with this change, but I think the latter does not deserve to be treated as if it were an afterthought by being a mere parenthetical mention.

I agree. I'll address that for the next version.

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

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

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.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 builtin/blame.c     |  2 +-
 t/annotate-tests.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

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

Good find.

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.

Thanks, that change also makes sense.

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.

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.

Philippe Blain <levraiphilippeblain@gmail.com> writes:

That does sound like a sensible way to clean it up.  Thanks.

From: Philippe Blain <levraiphilippeblain@gmail.com>

Several Git commands can make use of the builtin userdiff patterns, but it's not obvious in the documentation. Add pointers to the 'Defining a custom hunk header' part of gitattributes(5) in the description of the following options:

- the '--function-context' option of `git diff` and friends
- the '--function-context' option of `git grep`
- the '-L :<funcname>' option of `git log`, `gitk` and `git blame`

In 'git-grep.txt', take the opportunity to use backticks in the description of '--show-function', and improve the wording of the desription of '--function-context'.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/diff-options.txt      | 5 ++++-
 Documentation/git-grep.txt          | 6 ++++--
 Documentation/line-range-format.txt | 4 +++-
 3 files changed, 11 insertions(+), 4 deletions(-)

On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget <gitgitgadget@gmail.com> wrote:

s/changes/change/

From: Philippe Blain <levraiphilippeblain@gmail.com>

'git blame -h' and 'git log -h' both show '-L <n,m>' and describe this option as "Process only line range n,m, counting from 1". No hint is given that a function name regex can also be used.

Use <range> instead, and expand the description of the option to mention both modes. Remove "counting from 1" as it's uneeded; it's uncommon to refer to the first line of a file as "line 0".

Also, for 'git log', improve the wording to better reflect the long help.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 builtin/blame.c | 3 ++-
 builtin/log.c   | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget <gitgitgadget@gmail.com> wrote:

The "<range>=" bit is redundant and confusing (and ugly). Considering that the description already says "Process only the given line _range_", it should be fine to drop the "<range>=" lead-in. Perhaps:

    Process only the given line range <start>,<end> or :<funcname>"

This might feel too succinct, but that's often true of short -h help. Such succinctness is generally acceptable as long as more detailed documentation can be discovered easily (such as in the 'man' page).

Same comment regarding the rest of the changes in this patch.

Hi Eric, 

Yes, I agree. I've shortened it for v2 (and also made the changes you suggested for 2/6 and 4/6.

Thanks for taking a look,

Philippe.

From: Philippe Blain <levraiphilippeblain@gmail.com>

Improve the formatting of the description of the line-range option '-L' for `git log`, `gitk` and `git blame`:

- Use bold for <start>, <end> and <funcname>
- Use backticks for literals

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/blame-options.txt      |  4 ++--
 Documentation/line-range-format.txt  | 24 ++++++++++++------------
 Documentation/line-range-options.txt |  6 +++---
 3 files changed, 17 insertions(+), 17 deletions(-)

On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget <gitgitgadget@gmail.com> wrote:

My impression is that it is more common in Git documentation for these placeholders to be formatted with backticks rather than as bold (or, if not more common currently, at least is trending that way). That's not to say that my impression is necessarily accurate.

Eric Sunshine <sunshine@sunshineco.com> writes:

I think the impression is fairly in line with the reality. That is not to say that it is a good future to aim for.

My personal preference is

    - use `as-is` for things that must be typed as-is (e.g. `-L`)

    - use 'emph' for things that are placeholder (e.g. '<start>'').

Yes, I compared using 

   git grep '`<' | wc -l 

and 

   git grep "'<" | wc -l

and the same for their closing  counterparts and the backticks win.

Thanks for clarifying. It is indeed what I tried to do.

From: Philippe Blain <levraiphilippeblain@gmail.com>

The description of the '-L' option for `git log` and `gitk` is the same, but is repeated in both 'git-log.txt' and 'gitk.txt'.

Remove the duplication by creating a new file, 'line-range-options.txt', and include it in both files.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/git-log.txt            | 16 ++--------------
 Documentation/gitk.txt               | 21 ++-------------------
 Documentation/line-range-options.txt | 27 +++++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/line-range-options.txt

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

They are moral equivalents but differ slightly, both in an insignificant way (i.e. SP after -L and its parameters) and a significant way (i.e. Note about gitk usage), so "the same, but is repeated" is not sufficient to explain why the contents of the new file looks the way it is.

Makes sense. As to the conditional, I actually think the version with SP after -L do not have to be listed and instead to show just the "stuck" form as the standardised way. If the option takes an optional value, you must have to use the "stuck" form anyway, and showing that you _could_ have SP there unnecessarily throws extra bytes at the reader with no real gain.

It looked puzzling to see the inclusion of line-range-format is lost from here, but it turns out to be OK after all when we look at the new line-range-options file.

As I said, my vote goes to reducing the variations and using only the latter form for simplicity.

This text is the same as both originals, which is good.

I think it may make sense to keep this part, mostly from "keep the result as close to the original as possible while refactoring" principle, but if we only teach the "stuck" form in "git log", it becomes irrelevant---one fewer thing the users need to learn and remember, it becomes easier for them to concentrate on what really matters.

And the answer to my earlier puzzlement is here. I actually would have expected that it would be the matter of moving the common text you moved from gitk/log documentation to this file instead to the top of the line-range-format.txt file, but the latter is included also by blame-options.txt, and the description of the option needs to be somewhat different between blame and log anyway, so I think this is the best we can do at this step in the series.

Looking good so far...

Hi Junio,

OK. I'll expand on that.

I'm not sure I understand here... The <range> argument is not optional here (`git log -L` by itself is meaningless)... I know gitcli(7) recommends the stuck form, but this option has been listed as `-L <range>` in the git-log documentation ever since it appeared... So I did not think it wise to change it, that's why I introduce the conditional.

Thanks,

Philippe.

Philippe Blain <levraiphilippeblain@gmail.com> writes:

I do not think showing alternative ways distracts without adding much value. We should remember that most users are learning Git to only to do things they want to do and not necessarily learning Git to master all the slightly different ways to do the same thing. "You can write option and its argument separately most of the time but you must write option and its argument in the stuck form some other times---you must think if the argument is optional or not, whenyou want to write your argument separately" crams more things to learn than the simple "Write option and its arguments in the stuck form and it always works".

Changes since v1:

 * patch 1: tweaked the message and took Junio's suggestion to only list the
   stuck form if the option in 'git-log' and 'gitk'.
 * patch 2: very minor formatting changes (removed double quotes)
 * patch 3: removed the parentheses as suggested by Junio, and did the same
   for git-log/gitk.
 * patch 4: grammar fix pointed out by Eric
 * patch 5: shortened the help text as suggested by Eric
 * patch 6: changed the approach by initializing 'sb.path' earlier in
   'cmd_blame', along with some of sb's other fields.
 * patches 7-8: new cleanup patches following the new patch 6 to simplify
   the interface of two functions in 'blame.c'

----------------------------------------------------------------------------

This series fixes a bug in git blame where any userdiff driver configured via the attributes mechanism is ignored, which more often than not means thatgit blame -L :<funcname> dies for paths that have a configured userdiff driver. That's patch 6/6.

Patches 1-5 are documentation improvements around the line-log functionality.

Patches 7-8 are code clean-up patches following the changes in patch 6.

The fact that this bug has been present ever since git blame learned -L :<funcname> makes me a little sad. I think that the fact that the diff attribute (either with a builtin userdiff pattern or with diff.*.xfuncname ) influences way more than just the hunk header, but also features like git log -L, git blame -L, and the different options that I mention in patch 4/6, is not well-known enough. I'm not saying the code or the doc is wrong, but I think it's a visibility issue. Maybe more blog posts about that would help.... I noticed that the "Git Attributes" chapter in the ProGit book [1] does not even mention the builtin userdiff patterns, so I'll probably do a PR there to mention it. Any other ideas for improving discoverability of this very cool feature ?

[1] https://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes

CC: Thomas Rast tr@thomasrast.ch [tr@thomasrast.ch], Eric Sunshine 
sunshine@sunshineco.com [sunshine@sunshineco.com], René Scharfe l.s.r@web.de
[l.s.r@web.de]

Philippe Blain (8):
  doc: log, gitk: move '-L' description to 'line-range-options.txt'
  doc: line-range: improve formatting
  blame-options.txt: also mention 'funcname' in '-L' description
  doc: add more pointers to gitattributes(5) for userdiff
  line-log: mention both modes in 'blame' and 'log' short help
  blame: enable funcname blaming with userdiff driver
  blame: simplify 'setup_scoreboard' interface
  blame: simplify 'setup_blame_bloom_data' interface

 Documentation/blame-options.txt      |  9 +++++----
 Documentation/diff-options.txt       |  5 ++++-
 Documentation/git-grep.txt           |  6 ++++--
 Documentation/git-log.txt            | 15 +--------------
 Documentation/gitk.txt               | 20 +-------------------
 Documentation/line-range-format.txt  | 28 +++++++++++++++-------------
 Documentation/line-range-options.txt | 15 +++++++++++++++
 blame.c                              | 16 +++++++---------
 blame.h                              |  4 +---
 builtin/blame.c                      | 11 ++++++-----
 builtin/log.c                        |  4 ++--
 t/annotate-tests.sh                  | 18 ++++++++++++++++++
 12 files changed, 79 insertions(+), 72 deletions(-)
 create mode 100644 Documentation/line-range-options.txt

base-commit: 1d1c4a875900d69c7f0a31e44c3e370dc80ab1ce
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-774%2Fphil-blain%2Fblame-funcname-userdiff-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-774/phil-blain/blame-funcname-userdiff-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/774

Range-diff vs v1:

 1:  96f6f95abc ! 1:  b4fa5fb5ae doc: log, gitk: move '-L' description to 'line-range-options.txt'
     @@ Metadata
       ## Commit message ##
          doc: log, gitk: move '-L' description to 'line-range-options.txt'
      
     -    The description of the '-L' option for `git log` and `gitk` is the
     -    same, but is repeated in both 'git-log.txt' and 'gitk.txt'.
     +    The description of the '-L' option for `git log` and `gitk` is almost
     +    the same, but is repeated in both 'git-log.txt' and 'gitk.txt' (the
     +    difference being that 'git-log.txt' lists the option with a space
     +    after '-L', while 'gitk.txt' lists it as stuck and notes that `gitk`
     +    only understands the stuck form).
      
     -    Remove the duplication by creating a new file, 'line-range-options.txt',
     +    Reduce duplication by creating a new file, 'line-range-options.txt',
          and include it in both files.
      
     +    To simplify the presentation, only list the stuck form for both
     +    commands, and remove the note about `gitk` only understanding the stuck
     +    form.
     +
          Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
      
       ## Documentation/git-log.txt ##
     @@ Documentation/git-log.txt: produced by `--stat`, etc.
      -	`--name-only`, `--name-status`, `--check`) are not currently implemented.
      -+
      -include::line-range-format.txt[]
     -+:git-log: 1
      +include::line-range-options.txt[]
       
       <revision range>::
     @@ Documentation/gitk.txt: linkgit:git-rev-list[1] for a complete list.
      -*not* put a space after `-L`.
      -+
      -include::line-range-format.txt[]
     -+:gitk: 1
      +include::line-range-options.txt[]
       
       <revision range>::
     @@ Documentation/gitk.txt: linkgit:git-rev-list[1] for a complete list.
      
       ## Documentation/line-range-options.txt (new) ##
      @@
     -+ifdef::git-log[]
     -+-L <start>,<end>:<file>::
     -+-L :<funcname>:<file>::
     -+endif::git-log[]
     -+ifdef::gitk[]
      +-L<start>,<end>:<file>::
      +-L:<funcname>:<file>::
     -+endif::gitk[]
      +
      +	Trace the evolution of the line range given by "<start>,<end>"
      +	(or the function name regex <funcname>) within the <file>.  You may
     @@ Documentation/line-range-options.txt (new)
      +	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
      +	`--name-only`, `--name-status`, `--check`) are not currently implemented.
      ++
     -+ifdef::gitk[]
     -+*Note:* gitk (unlike linkgit:git-log[1]) currently only understands
     -+this option if you specify it "glued together" with its argument.  Do
     -+*not* put a space after `-L`.
     -+endif::gitk[]
     -++
      +include::line-range-format.txt[]
 2:  7d3fc0a503 ! 2:  6c44b1c576 doc: line-range: improve formatting
     @@ Documentation/line-range-format.txt
       
       +
      -If ``:<funcname>'' is given in place of <start> and <end>, it is a
     -+If "`:<funcname>`" is given in place of '<start>' and '<end>', it is a
     ++If `:<funcname>` is given in place of '<start>' and '<end>', it is a
       regular expression that denotes the range from the first funcname line
      -that matches <funcname>, up to the next funcname line. ``:<funcname>''
      +that matches '<funcname>', up to the next funcname line. `:<funcname>`
     @@ Documentation/line-range-format.txt
       file.
      
       ## Documentation/line-range-options.txt ##
     -@@ Documentation/line-range-options.txt: ifdef::gitk[]
     +@@
     + -L<start>,<end>:<file>::
       -L:<funcname>:<file>::
     - endif::gitk[]
       
      -	Trace the evolution of the line range given by "<start>,<end>"
      -	(or the function name regex <funcname>) within the <file>.  You may
     -+	Trace the evolution of the line range given by "'<start>,<end>'"
     ++	Trace the evolution of the line range given by '<start>,<end>'
      +	(or the function name regex '<funcname>') within the '<file>'. You may
       	not give any pathspec limiters.  This is currently limited to
       	a walk starting from a single revision, i.e., you may only
 3:  f7b64bf330 ! 3:  d38f0f7e26 blame-options.txt: also mention 'funcname' in '-L' description
     @@ Commit message
          '-L :<funcname>' by mentioning it at the beginnning of the description
          of the '-L' option.
      
     +    Also, in 'line-range-options.txt', which is used for git-log(1) and
     +    gitk(1), do not parenthesize the mention of the ':<funcname>' mode, to
     +    place it on equal footing with the '<start>,<end>' mode.
     +
          Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
      
       ## Documentation/blame-options.txt ##
     @@ Documentation/blame-options.txt
       -L :<funcname>::
      -	Annotate only the given line range. May be specified multiple times.
      -	Overlapping ranges are allowed.
     -+	Annotate only the line range given by '<start>,<end>'
     -+	(or by the function name regex '<funcname>').
     ++	Annotate only the line range given by '<start>,<end>',
     ++	or by the function name regex '<funcname>'.
      +	May be specified multiple times. Overlapping ranges are allowed.
       +
       '<start>' and '<end>' are optional. `-L <start>` or `-L <start>,` spans from
       '<start>' to end of file. `-L ,<end>` spans from start of file to '<end>'.
     +
     + ## Documentation/line-range-options.txt ##
     +@@
     + -L<start>,<end>:<file>::
     + -L:<funcname>:<file>::
     + 
     +-	Trace the evolution of the line range given by '<start>,<end>'
     +-	(or the function name regex '<funcname>') within the '<file>'. You may
     ++	Trace the evolution of the line range given by '<start>,<end>',
     ++	or by the function name regex '<funcname>', within the '<file>'. You may
     + 	not give any pathspec limiters.  This is currently limited to
     + 	a walk starting from a single revision, i.e., you may only
     + 	give zero or one positive revision arguments, and
 4:  da76a10717 ! 4:  9443a681e3 doc: add more pointers to gitattributes(5) for userdiff
     @@ Documentation/diff-options.txt: endif::git-format-patch[]
       -W::
       --function-context::
      -	Show whole surrounding functions of changes.
     -+	Show whole functions as context lines for each changes.
     ++	Show whole function as context lines for each change.
      +	The function names are determined in the same way as
      +	`git diff` works out patch hunk headers (see 'Defining a
      +	custom hunk-header' in linkgit:gitattributes[5]).
 5:  27ef94e9cc ! 5:  ae61d27a38 line-log: mention both modes in 'blame' and 'log' short help
     @@ builtin/blame.c: int cmd_blame(int argc, const char **argv, const char *prefix)
       		OPT_CALLBACK_F('M', NULL, &opt, N_("score"), N_("Find line movements within and across files"), PARSE_OPT_OPTARG, blame_move_callback),
      -		OPT_STRING_LIST('L', NULL, &range_list, N_("n,m"), N_("Process only line range n,m, counting from 1")),
      +		OPT_STRING_LIST('L', NULL, &range_list, N_("range"),
     -+				N_("Process only the given line range (<range>=<start>,<end>) or function (<range>=:<funcname>)")),
     ++				N_("Process only line range <start>,<end> or function :<funcname>")),
       		OPT__ABBREV(&abbrev),
       		OPT_END()
       	};
     @@ builtin/log.c: static void cmd_log_init_finish(int argc, const char **argv, cons
      -		OPT_CALLBACK('L', NULL, &line_cb, "n,m:file",
      -			     N_("Process line range n,m in file, counting from 1"),
      +		OPT_CALLBACK('L', NULL, &line_cb, "range:file",
     -+			     N_("Trace the evolution of the given line range (<range>=<start>,<end>) or function (<range>=:<funcname>) in <file>"),
     ++			     N_("Trace the evolution of line range <start>,<end> or function :<funcname> in <file>"),
       			     log_line_range_callback),
       		OPT_END()
       	};
 6:  a1e1c977d0 ! 6:  c77108e864 blame: enable funcname blaming with userdiff driver
     @@ Commit message
          funcname, 2013-03-28).
      
          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'.
     +    initializing 'sb.path' earlier in 'cmd_blame', when some of its other
     +    fields are initialized, so that it is set when passed to
     +    'parse_range_arg'.
      
          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.
      
     +    Also, use 'sb.path' instead of 'path' when constructing the error
     +    message at line 1114, for consistency.
     +
          Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
      
       ## builtin/blame.c ##
      @@ builtin/blame.c: int cmd_blame(int argc, const char **argv, const char *prefix)
     - 		long bottom, top;
     - 		if (parse_range_arg(range_list.items[range_i].string,
     - 				    nth_line_cb, &sb, lno, anchor,
     --				    &bottom, &top, sb.path,
     -+				    &bottom, &top, path,
     - 				    the_repository->index))
     - 			usage(blame_usage);
     + 	sb.contents_from = contents_from;
     + 	sb.reverse = reverse;
     + 	sb.repo = the_repository;
     ++	sb.path = path;
     + 	build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);
     + 	string_list_clear(&ignore_revs_file_list, 0);
     + 	string_list_clear(&ignore_rev_list, 0);
     +@@ builtin/blame.c: int cmd_blame(int argc, const char **argv, const char *prefix)
       		if ((!lno && (top || bottom)) || lno < bottom)
     + 			die(Q_("file %s has only %lu line",
     + 			       "file %s has only %lu lines",
     +-			       lno), path, lno);
     ++			       lno), sb.path, lno);
     + 		if (bottom < 1)
     + 			bottom = 1;
     + 		if (top < 1 || lno < top)
     +@@ builtin/blame.c: int cmd_blame(int argc, const char **argv, const char *prefix)
     + 	string_list_clear(&range_list, 0);
     + 
     + 	sb.ent = NULL;
     +-	sb.path = path;
     + 
     + 	if (blame_move_score)
     + 		sb.move_score = blame_move_score;
      
       ## t/annotate-tests.sh ##
      @@ t/annotate-tests.sh: test_expect_success 'blame -L ^:RE (absolute: end-of-file)' '
 -:  ---------- > 7:  3b28696e51 blame: simplify 'setup_scoreboard' interface
 -:  ---------- > 8:  7aca3274d2 blame: simplify 'setup_blame_bloom_data' interface

From: Philippe Blain <levraiphilippeblain@gmail.com>

The description of the '-L' option for `git log` and `gitk` is almost the same, but is repeated in both 'git-log.txt' and 'gitk.txt' (the difference being that 'git-log.txt' lists the option with a space after '-L', while 'gitk.txt' lists it as stuck and notes that `gitk` only understands the stuck form).

Reduce duplication by creating a new file, 'line-range-options.txt', and include it in both files.

To simplify the presentation, only list the stuck form for both commands, and remove the note about `gitk` only understanding the stuck form.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/git-log.txt            | 15 +--------------
 Documentation/gitk.txt               | 20 +-------------------
 Documentation/line-range-options.txt | 15 +++++++++++++++
 3 files changed, 17 insertions(+), 33 deletions(-)
 create mode 100644 Documentation/line-range-options.txt

From: Philippe Blain <levraiphilippeblain@gmail.com>

Make it clearer that a function can be blamed by feeding `git blame` '-L :<funcname>' by mentioning it at the beginnning of the description of the '-L' option.

Also, in 'line-range-options.txt', which is used for git-log(1) and gitk(1), do not parenthesize the mention of the ':<funcname>' mode, to place it on equal footing with the '<start>,<end>' mode.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/blame-options.txt      | 5 +++--
 Documentation/line-range-options.txt | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

From: Philippe Blain <levraiphilippeblain@gmail.com>

Several Git commands can make use of the builtin userdiff patterns, but it's not obvious in the documentation. Add pointers to the 'Defining a custom hunk header' part of gitattributes(5) in the description of the following options:

- the '--function-context' option of `git diff` and friends
- the '--function-context' option of `git grep`
- the '-L :<funcname>' option of `git log`, `gitk` and `git blame`

In 'git-grep.txt', take the opportunity to use backticks in the description of '--show-function', and improve the wording of the desription of '--function-context'.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/diff-options.txt      | 5 ++++-
 Documentation/git-grep.txt          | 6 ++++--
 Documentation/line-range-format.txt | 4 +++-
 3 files changed, 11 insertions(+), 4 deletions(-)

From: Philippe Blain <levraiphilippeblain@gmail.com>

'git blame -h' and 'git log -h' both show '-L <n,m>' and describe this option as "Process only line range n,m, counting from 1". No hint is given that a function name regex can also be used.

Use <range> instead, and expand the description of the option to mention both modes. Remove "counting from 1" as it's uneeded; it's uncommon to refer to the first line of a file as "line 0".

Also, for 'git log', improve the wording to better reflect the long help.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 builtin/blame.c | 3 ++-
 builtin/log.c   | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

From: Philippe Blain <levraiphilippeblain@gmail.com>

Improve the formatting of the description of the line-range option '-L' for `git log`, `gitk` and `git blame`:

- Use bold for <start>, <end> and <funcname>
- Use backticks for literals

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/blame-options.txt      |  4 ++--
 Documentation/line-range-format.txt  | 24 ++++++++++++------------
 Documentation/line-range-options.txt |  6 +++---
 3 files changed, 17 insertions(+), 17 deletions(-)

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

Enable funcname blaming for paths using specific userdiff drivers by initializing 'sb.path' earlier in 'cmd_blame', when some of its other fields are initialized, so that it is set when passed to 'parse_range_arg'.

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.

Also, use 'sb.path' instead of 'path' when constructing the error message at line 1114, for consistency.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 builtin/blame.c     |  4 ++--
 t/annotate-tests.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

From: Philippe Blain <levraiphilippeblain@gmail.com>

The penultimate commit moved the initialization of 'sb.path' in 'builtin/blame.c::cmd_blame' before the call to 'blame.c::setup_blame_bloom_data'. Since 'cmd_blame' is the only caller of 'setup_blame_bloom_data', it is now unnecessary for 'setup_blame_bloom_data' to receive 'path' as a separate argument, as 'sb.path' is already initialized.

Remove this argument from setup_blame_bloom_data's interface and use the 'path' field of the 'sb' 'struct blame_scoreboard' instead.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 blame.c         | 5 ++---
 blame.h         | 3 +--
 builtin/blame.c | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

From: Philippe Blain <levraiphilippeblain@gmail.com>

The previous commit moved the initialization of 'sb.path' in 'builtin/blame.c::cmd_blame' before the call to 'blame.c::setup_scoreboard'. Since 'cmd_blame' is the only caller of 'setup_scoreboard', it is now unnecessary for 'setup_scoreboard' to receive 'path' as a separate argument, as 'sb.path' is already initialized.

Remove this argument from setup_scoreboard's interface and use the 'path' field of the 'sb' 'struct blame_scoreboard' instead.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 blame.c         | 11 +++++------
 blame.h         |  1 -
 builtin/blame.c |  2 +-
 3 files changed, 6 insertions(+), 8 deletions(-)