[PATCH v2 0/8] blame: enable funcname blaming with userdiff driver
- From
Philippe Blain via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Nov 1, 2020, 17:28 UTC
- Message-ID
- <pull.774.v2.git.1604251727.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.774.git.1603889270.gitgitgadget@gmail.com>
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