git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Philippe Blain <levraiphilippeblain@gmail.com>,
	Philippe Blain <levraiphilippeblain@gmail.com>
Subject: [PATCH v2 0/8] blame: enable funcname blaming with userdiff driver
Date: Sun, 01 Nov 2020 17:28:39 +0000	[thread overview]
Message-ID: <pull.774.v2.git.1604251727.gitgitgadget@gmail.com> (raw)
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

  parent reply	other threads:[~2020-11-01 17:29 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-28 12:47 [PATCH 0/6] blame: enable funcname blaming with userdiff driver Philippe Blain via GitGitGadget
2020-10-28 12:47 ` [PATCH 1/6] doc: log, gitk: move '-L' description to 'line-range-options.txt' Philippe Blain via GitGitGadget
2020-10-29 20:16   ` Junio C Hamano
2020-10-31 17:18     ` Philippe Blain
2020-10-31 17:29       ` Junio C Hamano
2020-10-28 12:47 ` [PATCH 2/6] doc: line-range: improve formatting Philippe Blain via GitGitGadget
2020-10-28 17:23   ` Eric Sunshine
2020-10-29 20:19     ` Junio C Hamano
2020-10-31 17:20       ` Philippe Blain
2020-10-28 12:47 ` [PATCH 3/6] blame-options.txt: also mention 'funcname' in '-L' description Philippe Blain via GitGitGadget
2020-10-29 20:25   ` Junio C Hamano
2020-10-31 17:22     ` Philippe Blain
2020-10-28 12:47 ` [PATCH 4/6] doc: add more pointers to gitattributes(5) for userdiff Philippe Blain via GitGitGadget
2020-10-28 17:26   ` Eric Sunshine
2020-10-28 12:47 ` [PATCH 5/6] line-log: mention both modes in 'blame' and 'log' short help Philippe Blain via GitGitGadget
2020-10-28 17:33   ` Eric Sunshine
2020-10-29 12:43     ` Philippe Blain
2020-10-28 12:47 ` [PATCH 6/6] blame: enable funcname blaming with userdiff driver Philippe Blain via GitGitGadget
2020-10-29 20:40   ` Junio C Hamano
2020-10-31 18:02     ` Philippe Blain
2020-10-31 18:58       ` Junio C Hamano
2020-11-01 17:28 ` Philippe Blain via GitGitGadget [this message]
2020-11-01 17:28   ` [PATCH v2 1/8] doc: log, gitk: move '-L' description to 'line-range-options.txt' Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 2/8] doc: line-range: improve formatting Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 3/8] blame-options.txt: also mention 'funcname' in '-L' description Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 4/8] doc: add more pointers to gitattributes(5) for userdiff Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 5/8] line-log: mention both modes in 'blame' and 'log' short help Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 6/8] blame: enable funcname blaming with userdiff driver Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 7/8] blame: simplify 'setup_scoreboard' interface Philippe Blain via GitGitGadget
2020-11-01 17:28   ` [PATCH v2 8/8] blame: simplify 'setup_blame_bloom_data' interface Philippe Blain via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.774.v2.git.1604251727.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=levraiphilippeblain@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).