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

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

[Philippe Blain via GitGitGadget]

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

[PATCH 1/6] doc: log, gitk: move '-L' description to 'line-range-options.txt'

[Philippe Blain via GitGitGadget]

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

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2b8ac5ff88..631dfff77c 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -77,20 +77,8 @@ produced by `--stat`, etc.
 	Intended to speed up tools that read log messages from `git log`
 	output by allowing them to allocate space in advance.
 
--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
-	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
-	<start> and <end> (or <funcname>) must exist in the starting revision.
-	You can specify this option more than once. Implies `--patch`.
-	Patch output can be suppressed using `--no-patch`, but other diff formats
-	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-	`--name-only`, `--name-status`, `--check`) are not currently implemented.
-+
-include::line-range-format.txt[]
+:git-log: 1
+include::line-range-options.txt[]
 
 <revision range>::
 	Show only commits in the specified revision range.  When no
diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
index c653ebb6a8..761b764c18 100644
--- a/Documentation/gitk.txt
+++ b/Documentation/gitk.txt
@@ -98,25 +98,8 @@ linkgit:git-rev-list[1] for a complete list.
 	(See "History simplification" in linkgit:git-log[1] for a more
 	detailed explanation.)
 
--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
-	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
-	<start> and <end> (or <funcname>) must exist in the starting revision.
-	You can specify this option more than once. Implies `--patch`.
-	Patch output can be suppressed using `--no-patch`, but other diff formats
-	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-	`--name-only`, `--name-status`, `--check`) are not currently implemented.
-+
-*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`.
-+
-include::line-range-format.txt[]
+:gitk: 1
+include::line-range-options.txt[]
 
 <revision range>::
 
diff --git a/Documentation/line-range-options.txt b/Documentation/line-range-options.txt
new file mode 100644
index 0000000000..9e3d98d44f
--- /dev/null
+++ b/Documentation/line-range-options.txt
@@ -0,0 +1,27 @@
+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
+	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
+	<start> and <end> (or <funcname>) must exist in the starting revision.
+	You can specify this option more than once. Implies `--patch`.
+	Patch output can be suppressed using `--no-patch`, but other diff formats
+	(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[]
-- 
gitgitgadget

[PATCH 2/6] doc: line-range: improve formatting

[Philippe Blain via GitGitGadget]

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

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 88750af7ae..48bf0eeec5 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -14,8 +14,8 @@
 	Annotate only the given line range. 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>.
+'<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>'.
 +
 include::line-range-format.txt[]
 
diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
index 829676ff98..6ee159b683 100644
--- a/Documentation/line-range-format.txt
+++ b/Documentation/line-range-format.txt
@@ -1,30 +1,30 @@
-<start> and <end> can take one of these forms:
+'<start>' and '<end>' can take one of these forms:
 
 - number
 +
-If <start> or <end> is a number, it specifies an
+If '<start>' or '<end>' is a number, it specifies an
 absolute line number (lines count from 1).
 +
 
-- /regex/
+- `/regex/`
 +
 This form will use the first line matching the given
-POSIX regex. If <start> is a regex, it will search from the end of
+POSIX regex. If '<start>' is a regex, it will search from the end of
 the previous `-L` range, if any, otherwise from the start of file.
-If <start> is ``^/regex/'', it will search from the start of file.
-If <end> is a regex, it will search
-starting at the line given by <start>.
+If '<start>' is `^/regex/`, it will search from the start of file.
+If '<end>' is a regex, it will search
+starting at the line given by '<start>'.
 +
 
 - +offset or -offset
 +
-This is only valid for <end> and will specify a number
-of lines before or after the line given by <start>.
+This is only valid for '<end>' and will specify a number
+of lines before or after the line given by '<start>'.
 
 +
-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>`
 searches from the end of the previous `-L` range, if any, otherwise
-from the start of file. ``^:<funcname>'' searches from the start of
+from the start of file. `^:<funcname>` searches from the start of
 file.
diff --git a/Documentation/line-range-options.txt b/Documentation/line-range-options.txt
index 9e3d98d44f..d2ef1a6d67 100644
--- a/Documentation/line-range-options.txt
+++ b/Documentation/line-range-options.txt
@@ -7,12 +7,12 @@ ifdef::gitk[]
 -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>'"
+	(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
 	give zero or one positive revision arguments, and
-	<start> and <end> (or <funcname>) must exist in the starting revision.
+	'<start>' and '<end>' (or '<funcname>') must exist in the starting revision.
 	You can specify this option more than once. Implies `--patch`.
 	Patch output can be suppressed using `--no-patch`, but other diff formats
 	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-- 
gitgitgadget

[PATCH 3/6] blame-options.txt: also mention 'funcname' in '-L' description

[Philippe Blain via GitGitGadget]

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

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 48bf0eeec5..8b4a90e349 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -11,8 +11,9 @@
 
 -L <start>,<end>::
 -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>').
+	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>'.
-- 
gitgitgadget

[PATCH 4/6] doc: add more pointers to gitattributes(5) for userdiff

[Philippe Blain via GitGitGadget]

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

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 573fb9bb71..39b511bb6b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -695,7 +695,10 @@ endif::git-format-patch[]
 
 -W::
 --function-context::
-	Show whole surrounding functions of changes.
+	Show whole functions as context lines for each changes.
+	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]).
 
 ifndef::git-format-patch[]
 ifndef::git-log[]
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 6077ff01a4..4e0ba8234a 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -241,7 +241,7 @@ providing this option will cause it to die.
 --show-function::
 	Show the preceding line that contains the function name of
 	the match, unless the matching line is a function name itself.
-	The name is determined in the same way as 'git diff' works out
+	The name is determined in the same way as `git diff` works out
 	patch hunk headers (see 'Defining a custom hunk-header' in
 	linkgit:gitattributes[5]).
 
@@ -266,7 +266,9 @@ providing this option will cause it to die.
 	Show the surrounding text from the previous line containing a
 	function name up to the one before the next function name,
 	effectively showing the whole function in which the match was
-	found.
+	found. 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]).
 
 --threads <num>::
 	Number of grep worker threads to use.
diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
index 6ee159b683..ab3c70a1d5 100644
--- a/Documentation/line-range-format.txt
+++ b/Documentation/line-range-format.txt
@@ -27,4 +27,6 @@ regular expression that denotes the range from the first funcname line
 that matches '<funcname>', up to the next funcname line. `:<funcname>`
 searches from the end of the previous `-L` range, if any, otherwise
 from the start of file. `^:<funcname>` searches from the start of
-file.
+file. 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]).
-- 
gitgitgadget

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

[Philippe Blain via GitGitGadget]

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

diff --git a/builtin/blame.c b/builtin/blame.c
index bb0f29300e..05f69211dd 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -889,7 +889,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "contents", &contents_from, N_("file"), N_("Use <file>'s contents as the final image")),
 		OPT_CALLBACK_F('C', NULL, &opt, N_("score"), N_("Find line copies within and across files"), PARSE_OPT_OPTARG, blame_copy_callback),
 		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>)")),
 		OPT__ABBREV(&abbrev),
 		OPT_END()
 	};
diff --git a/builtin/log.c b/builtin/log.c
index 0a7ed4bef9..e6a3bb7803 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -183,8 +183,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 				N_("pattern"), N_("do not decorate refs that match <pattern>")),
 		OPT_CALLBACK_F(0, "decorate", NULL, NULL, N_("decorate options"),
 			       PARSE_OPT_OPTARG, decorate_callback),
-		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>"),
 			     log_line_range_callback),
 		OPT_END()
 	};
-- 
gitgitgadget

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

[Philippe Blain via GitGitGadget]

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

diff --git a/builtin/blame.c b/builtin/blame.c
index 05f69211dd..917fedc635 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1104,7 +1104,7 @@ 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);
 		if ((!lno && (top || bottom)) || lno < bottom)
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index d933af5714..3aee61d2cc 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -479,6 +479,24 @@ test_expect_success 'blame -L ^:RE (absolute: end-of-file)' '
 	check_count -f hello.c -L$n -L^:ma.. F 4 G 1 H 1
 '
 
+test_expect_success 'setup -L :funcname with userdiff driver' '
+	echo "fortran-* diff=fortran" >.gitattributes &&
+	fortran_file=fortran-external-function &&
+	orig_file="$TEST_DIRECTORY/t4018/$fortran_file" &&
+	cp $orig_file . &&
+	git add $fortran_file &&
+	GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" \
+	git commit -m "add fortran file" &&
+	sed -e "s/ChangeMe/IWasChanged/" <"$orig_file" >$fortran_file &&
+	git add $fortran_file &&
+	GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" \
+	git commit -m "change fortran file"
+'
+
+test_expect_success 'blame -L :funcname with userdiff driver' '
+	check_count -f fortran-external-function -L:RIGHT A 7 B 1
+'
+
 test_expect_success 'setup incremental' '
 	(
 	GIT_AUTHOR_NAME=I &&
-- 
gitgitgadget

Re: [PATCH 2/6] doc: line-range: improve formatting

[Eric Sunshine]

On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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>

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.

Re: [PATCH 4/6] doc: add more pointers to gitattributes(5) for userdiff

[Eric Sunshine]

On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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:
> [...]
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> @@ -695,7 +695,10 @@ endif::git-format-patch[]
>  --function-context::
> -       Show whole surrounding functions of changes.
> +       Show whole functions as context lines for each changes.

s/changes/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]).

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

[Eric Sunshine]

On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> '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".
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -889,7 +889,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> -               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>)")),

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.

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

[Philippe Blain]

Hi Eric, 


> Le 28 oct. 2020 à 13:33, Eric Sunshine <sunshine@sunshineco.com> a écrit :
> 
> On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> '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".
>> 
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> @@ -889,7 +889,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>> -               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>)")),
> 
> 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.

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.

Re: [PATCH 1/6] doc: log, gitk: move '-L' description to 'line-range-options.txt'

[Junio C Hamano]

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

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

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.

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

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.

> diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
> index c653ebb6a8..761b764c18 100644
> --- a/Documentation/gitk.txt
> +++ b/Documentation/gitk.txt
> @@ -98,25 +98,8 @@ linkgit:git-rev-list[1] for a complete list.
>  	(See "History simplification" in linkgit:git-log[1] for a more
>  	detailed explanation.)
>  
> --L<start>,<end>:<file>::
> --L:<funcname>:<file>::
> ...
> -	`--name-only`, `--name-status`, `--check`) are not currently implemented.
> -+
> -*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`.
> -+
> -include::line-range-format.txt[]
> +:gitk: 1
> +include::line-range-options.txt[]

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.

> diff --git a/Documentation/line-range-options.txt b/Documentation/line-range-options.txt
> new file mode 100644
> index 0000000000..9e3d98d44f
> --- /dev/null
> +++ b/Documentation/line-range-options.txt
> @@ -0,0 +1,27 @@
> +ifdef::git-log[]
> +-L <start>,<end>:<file>::
> +-L :<funcname>:<file>::
> +endif::git-log[]
> +ifdef::gitk[]
> +-L<start>,<end>:<file>::
> +-L:<funcname>:<file>::
> +endif::gitk[]

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

> +	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
> +	give zero or one positive revision arguments, and
> +	<start> and <end> (or <funcname>) must exist in the starting revision.
> +	You can specify this option more than once. Implies `--patch`.
> +	Patch output can be suppressed using `--no-patch`, but other diff formats
> +	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
> +	`--name-only`, `--name-status`, `--check`) are not currently implemented.

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

> ++
> +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[]

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.


> +include::line-range-format.txt[]

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

Re: [PATCH 2/6] doc: line-range: improve formatting

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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>
>
> 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.

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

Re: [PATCH 3/6] blame-options.txt: also mention 'funcname' in '-L' description

[Junio C Hamano]

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

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

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.

>  -L <start>,<end>::
>  -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>').
> +	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>'.

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

[Junio C Hamano]

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

Re: [PATCH 1/6] doc: log, gitk: move '-L' description to 'line-range-options.txt'

[Philippe Blain]

Hi Junio,

> Le 29 oct. 2020 à 16:16, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> 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'.
> 
> 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.

OK. I'll expand on that.

>> Remove the duplication by creating a new file, 'line-range-options.txt',
>> and include it in both files.
> 
> 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.

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.

Re: [PATCH 2/6] doc: line-range: improve formatting

[Philippe Blain]

> Le 29 oct. 2020 à 16:19, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> On Wed, Oct 28, 2020 at 8:48 AM Philippe Blain via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>> 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>
>> 
>> 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.
> 
> I think the impression is fairly in line with the reality.  

Yes, I compared using 

   git grep '`<' | wc -l 

and 

   git grep "'<" | wc -l

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

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

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

Re: [PATCH 3/6] blame-options.txt: also mention 'funcname' in '-L' description

[Philippe Blain]

> Le 29 oct. 2020 à 16:25, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> 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.
> 
> 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.

Re: [PATCH 1/6] doc: log, gitk: move '-L' description to 'line-range-options.txt'

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

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

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

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

[Philippe Blain]

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

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

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

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

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

[PATCH v2 0/8] blame: enable funcname blaming with userdiff driver

[Philippe Blain via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v2 1/8] doc: log, gitk: move '-L' description to 'line-range-options.txt'

[Philippe Blain via GitGitGadget]

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

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2b8ac5ff88..dd189a353a 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -77,20 +77,7 @@ produced by `--stat`, etc.
 	Intended to speed up tools that read log messages from `git log`
 	output by allowing them to allocate space in advance.
 
--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
-	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
-	<start> and <end> (or <funcname>) must exist in the starting revision.
-	You can specify this option more than once. Implies `--patch`.
-	Patch output can be suppressed using `--no-patch`, but other diff formats
-	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-	`--name-only`, `--name-status`, `--check`) are not currently implemented.
-+
-include::line-range-format.txt[]
+include::line-range-options.txt[]
 
 <revision range>::
 	Show only commits in the specified revision range.  When no
diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
index c653ebb6a8..d50e9ed10e 100644
--- a/Documentation/gitk.txt
+++ b/Documentation/gitk.txt
@@ -98,25 +98,7 @@ linkgit:git-rev-list[1] for a complete list.
 	(See "History simplification" in linkgit:git-log[1] for a more
 	detailed explanation.)
 
--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
-	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
-	<start> and <end> (or <funcname>) must exist in the starting revision.
-	You can specify this option more than once. Implies `--patch`.
-	Patch output can be suppressed using `--no-patch`, but other diff formats
-	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-	`--name-only`, `--name-status`, `--check`) are not currently implemented.
-+
-*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`.
-+
-include::line-range-format.txt[]
+include::line-range-options.txt[]
 
 <revision range>::
 
diff --git a/Documentation/line-range-options.txt b/Documentation/line-range-options.txt
new file mode 100644
index 0000000000..266263f6b4
--- /dev/null
+++ b/Documentation/line-range-options.txt
@@ -0,0 +1,15 @@
+-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
+	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
+	<start> and <end> (or <funcname>) must exist in the starting revision.
+	You can specify this option more than once. Implies `--patch`.
+	Patch output can be suppressed using `--no-patch`, but other diff formats
+	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
+	`--name-only`, `--name-status`, `--check`) are not currently implemented.
++
+include::line-range-format.txt[]
-- 
gitgitgadget

[PATCH v2 2/8] doc: line-range: improve formatting

[Philippe Blain via GitGitGadget]

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

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 88750af7ae..48bf0eeec5 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -14,8 +14,8 @@
 	Annotate only the given line range. 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>.
+'<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>'.
 +
 include::line-range-format.txt[]
 
diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
index 829676ff98..43759eef89 100644
--- a/Documentation/line-range-format.txt
+++ b/Documentation/line-range-format.txt
@@ -1,30 +1,30 @@
-<start> and <end> can take one of these forms:
+'<start>' and '<end>' can take one of these forms:
 
 - number
 +
-If <start> or <end> is a number, it specifies an
+If '<start>' or '<end>' is a number, it specifies an
 absolute line number (lines count from 1).
 +
 
-- /regex/
+- `/regex/`
 +
 This form will use the first line matching the given
-POSIX regex. If <start> is a regex, it will search from the end of
+POSIX regex. If '<start>' is a regex, it will search from the end of
 the previous `-L` range, if any, otherwise from the start of file.
-If <start> is ``^/regex/'', it will search from the start of file.
-If <end> is a regex, it will search
-starting at the line given by <start>.
+If '<start>' is `^/regex/`, it will search from the start of file.
+If '<end>' is a regex, it will search
+starting at the line given by '<start>'.
 +
 
 - +offset or -offset
 +
-This is only valid for <end> and will specify a number
-of lines before or after the line given by <start>.
+This is only valid for '<end>' and will specify a number
+of lines before or after the line given by '<start>'.
 
 +
-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>`
 searches from the end of the previous `-L` range, if any, otherwise
-from the start of file. ``^:<funcname>'' searches from the start of
+from the start of file. `^:<funcname>` searches from the start of
 file.
diff --git a/Documentation/line-range-options.txt b/Documentation/line-range-options.txt
index 266263f6b4..1c90127300 100644
--- a/Documentation/line-range-options.txt
+++ b/Documentation/line-range-options.txt
@@ -1,12 +1,12 @@
 -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 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
-	<start> and <end> (or <funcname>) must exist in the starting revision.
+	'<start>' and '<end>' (or '<funcname>') must exist in the starting revision.
 	You can specify this option more than once. Implies `--patch`.
 	Patch output can be suppressed using `--no-patch`, but other diff formats
 	(namely `--raw`, `--numstat`, `--shortstat`, `--dirstat`, `--summary`,
-- 
gitgitgadget

[PATCH v2 3/8] blame-options.txt: also mention 'funcname' in '-L' description

[Philippe Blain via GitGitGadget]

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

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 48bf0eeec5..dc3bceb6d1 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -11,8 +11,9 @@
 
 -L <start>,<end>::
 -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>'.
+	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>'.
diff --git a/Documentation/line-range-options.txt b/Documentation/line-range-options.txt
index 1c90127300..8e295a62b8 100644
--- a/Documentation/line-range-options.txt
+++ b/Documentation/line-range-options.txt
@@ -1,8 +1,8 @@
 -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
-- 
gitgitgadget

[PATCH v2 4/8] doc: add more pointers to gitattributes(5) for userdiff

[Philippe Blain via GitGitGadget]

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

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 573fb9bb71..72b558ce21 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -695,7 +695,10 @@ endif::git-format-patch[]
 
 -W::
 --function-context::
-	Show whole surrounding functions of 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]).
 
 ifndef::git-format-patch[]
 ifndef::git-log[]
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 6077ff01a4..4e0ba8234a 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -241,7 +241,7 @@ providing this option will cause it to die.
 --show-function::
 	Show the preceding line that contains the function name of
 	the match, unless the matching line is a function name itself.
-	The name is determined in the same way as 'git diff' works out
+	The name is determined in the same way as `git diff` works out
 	patch hunk headers (see 'Defining a custom hunk-header' in
 	linkgit:gitattributes[5]).
 
@@ -266,7 +266,9 @@ providing this option will cause it to die.
 	Show the surrounding text from the previous line containing a
 	function name up to the one before the next function name,
 	effectively showing the whole function in which the match was
-	found.
+	found. 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]).
 
 --threads <num>::
 	Number of grep worker threads to use.
diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
index 43759eef89..9b51e9fb66 100644
--- a/Documentation/line-range-format.txt
+++ b/Documentation/line-range-format.txt
@@ -27,4 +27,6 @@ regular expression that denotes the range from the first funcname line
 that matches '<funcname>', up to the next funcname line. `:<funcname>`
 searches from the end of the previous `-L` range, if any, otherwise
 from the start of file. `^:<funcname>` searches from the start of
-file.
+file. 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]).
-- 
gitgitgadget

[PATCH v2 5/8] line-log: mention both modes in 'blame' and 'log' short help

[Philippe Blain via GitGitGadget]

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

diff --git a/builtin/blame.c b/builtin/blame.c
index bb0f29300e..224b6f18d4 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -889,7 +889,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "contents", &contents_from, N_("file"), N_("Use <file>'s contents as the final image")),
 		OPT_CALLBACK_F('C', NULL, &opt, N_("score"), N_("Find line copies within and across files"), PARSE_OPT_OPTARG, blame_copy_callback),
 		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 line range <start>,<end> or function :<funcname>")),
 		OPT__ABBREV(&abbrev),
 		OPT_END()
 	};
diff --git a/builtin/log.c b/builtin/log.c
index 0a7ed4bef9..c87ce09325 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -183,8 +183,8 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 				N_("pattern"), N_("do not decorate refs that match <pattern>")),
 		OPT_CALLBACK_F(0, "decorate", NULL, NULL, N_("decorate options"),
 			       PARSE_OPT_OPTARG, decorate_callback),
-		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 line range <start>,<end> or function :<funcname> in <file>"),
 			     log_line_range_callback),
 		OPT_END()
 	};
-- 
gitgitgadget

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

[Philippe Blain via GitGitGadget]

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

diff --git a/builtin/blame.c b/builtin/blame.c
index 224b6f18d4..f2e528fcbc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1081,6 +1081,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	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);
@@ -1110,7 +1111,7 @@ 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)
@@ -1135,7 +1136,6 @@ 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;
diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index d933af5714..3aee61d2cc 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -479,6 +479,24 @@ test_expect_success 'blame -L ^:RE (absolute: end-of-file)' '
 	check_count -f hello.c -L$n -L^:ma.. F 4 G 1 H 1
 '
 
+test_expect_success 'setup -L :funcname with userdiff driver' '
+	echo "fortran-* diff=fortran" >.gitattributes &&
+	fortran_file=fortran-external-function &&
+	orig_file="$TEST_DIRECTORY/t4018/$fortran_file" &&
+	cp $orig_file . &&
+	git add $fortran_file &&
+	GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" \
+	git commit -m "add fortran file" &&
+	sed -e "s/ChangeMe/IWasChanged/" <"$orig_file" >$fortran_file &&
+	git add $fortran_file &&
+	GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" \
+	git commit -m "change fortran file"
+'
+
+test_expect_success 'blame -L :funcname with userdiff driver' '
+	check_count -f fortran-external-function -L:RIGHT A 7 B 1
+'
+
 test_expect_success 'setup incremental' '
 	(
 	GIT_AUTHOR_NAME=I &&
-- 
gitgitgadget

[PATCH v2 7/8] blame: simplify 'setup_scoreboard' interface

[Philippe Blain via GitGitGadget]

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

diff --git a/blame.c b/blame.c
index 686845b2b4..128cb7ba55 100644
--- a/blame.c
+++ b/blame.c
@@ -2764,7 +2764,6 @@ void init_scoreboard(struct blame_scoreboard *sb)
 }
 
 void setup_scoreboard(struct blame_scoreboard *sb,
-		      const char *path,
 		      struct blame_origin **orig)
 {
 	const char *final_commit_name = NULL;
@@ -2803,7 +2802,7 @@ void setup_scoreboard(struct blame_scoreboard *sb,
 		setup_work_tree();
 		sb->final = fake_working_tree_commit(sb->repo,
 						     &sb->revs->diffopt,
-						     path, sb->contents_from);
+						     sb->path, sb->contents_from);
 		add_pending_object(sb->revs, &(sb->final->object), ":");
 	}
 
@@ -2846,12 +2845,12 @@ void setup_scoreboard(struct blame_scoreboard *sb,
 		sb->final_buf_size = o->file.size;
 	}
 	else {
-		o = get_origin(sb->final, path);
+		o = get_origin(sb->final, sb->path);
 		if (fill_blob_sha1_and_mode(sb->repo, o))
-			die(_("no such path %s in %s"), path, final_commit_name);
+			die(_("no such path %s in %s"), sb->path, final_commit_name);
 
 		if (sb->revs->diffopt.flags.allow_textconv &&
-		    textconv_object(sb->repo, path, o->mode, &o->blob_oid, 1, (char **) &sb->final_buf,
+		    textconv_object(sb->repo, sb->path, o->mode, &o->blob_oid, 1, (char **) &sb->final_buf,
 				    &sb->final_buf_size))
 			;
 		else
@@ -2861,7 +2860,7 @@ void setup_scoreboard(struct blame_scoreboard *sb,
 		if (!sb->final_buf)
 			die(_("cannot read blob %s for path %s"),
 			    oid_to_hex(&o->blob_oid),
-			    path);
+			    sb->path);
 	}
 	sb->num_read_blob++;
 	prepare_lines(sb);
diff --git a/blame.h b/blame.h
index b6bbee4147..e8c185c8ef 100644
--- a/blame.h
+++ b/blame.h
@@ -181,7 +181,6 @@ const char *blame_nth_line(struct blame_scoreboard *sb, long lno);
 
 void init_scoreboard(struct blame_scoreboard *sb);
 void setup_scoreboard(struct blame_scoreboard *sb,
-		      const char *path,
 		      struct blame_origin **orig);
 void setup_blame_bloom_data(struct blame_scoreboard *sb,
 			    const char *path);
diff --git a/builtin/blame.c b/builtin/blame.c
index f2e528fcbc..5d8f4c4599 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1085,7 +1085,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	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);
-	setup_scoreboard(&sb, path, &o);
+	setup_scoreboard(&sb, &o);
 
 	/*
 	 * Changed-path Bloom filters are disabled when looking
-- 
gitgitgadget

[PATCH v2 8/8] blame: simplify 'setup_blame_bloom_data' interface

[Philippe Blain via GitGitGadget]

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

diff --git a/blame.c b/blame.c
index 128cb7ba55..9042aa3f2b 100644
--- a/blame.c
+++ b/blame.c
@@ -2887,8 +2887,7 @@ struct blame_entry *blame_entry_prepend(struct blame_entry *head,
 	return new_head;
 }
 
-void setup_blame_bloom_data(struct blame_scoreboard *sb,
-			    const char *path)
+void setup_blame_bloom_data(struct blame_scoreboard *sb)
 {
 	struct blame_bloom_data *bd;
 	struct bloom_filter_settings *bs;
@@ -2908,7 +2907,7 @@ void setup_blame_bloom_data(struct blame_scoreboard *sb,
 	bd->nr = 0;
 	ALLOC_ARRAY(bd->keys, bd->alloc);
 
-	add_bloom_key(bd, path);
+	add_bloom_key(bd, sb->path);
 
 	sb->bloom_data = bd;
 }
diff --git a/blame.h b/blame.h
index e8c185c8ef..38bde535b3 100644
--- a/blame.h
+++ b/blame.h
@@ -182,8 +182,7 @@ const char *blame_nth_line(struct blame_scoreboard *sb, long lno);
 void init_scoreboard(struct blame_scoreboard *sb);
 void setup_scoreboard(struct blame_scoreboard *sb,
 		      struct blame_origin **orig);
-void setup_blame_bloom_data(struct blame_scoreboard *sb,
-			    const char *path);
+void setup_blame_bloom_data(struct blame_scoreboard *sb);
 void cleanup_scoreboard(struct blame_scoreboard *sb);
 
 struct blame_entry *blame_entry_prepend(struct blame_entry *head,
diff --git a/builtin/blame.c b/builtin/blame.c
index 5d8f4c4599..2f8b06e589 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1092,7 +1092,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	 * for copies.
 	 */
 	if (!(opt & PICKAXE_BLAME_COPY))
-		setup_blame_bloom_data(&sb, path);
+		setup_blame_bloom_data(&sb);
 
 	lno = sb.num_lines;
 
-- 
gitgitgadget