git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 7/7] blame: actually use the diff opts parsed from the command line

From
René Scharfe <l.s.r@web.de>
Date
Aug 23, 2016, 09:56 UTC
Message-ID
<fe96d9e6-b306-0b57-8f24-6106b7e285cb@web.de>
In-Reply-To
<8192012a6bf725e0460522f9e67bab83b613127a.1471864378.git.mhagger@alum.mit.edu>
Am 22.08.2016 um 13:22 schrieb Michael Haggerty:
Show 6 quoted lines
> "git blame" already parsed generic diff options from the command line
> via diff_opt_parse(), but instead of passing the resulting xdl_opts to
> xdi_diff(), it sent its own xdl_opts, which only reflected the values of
> the self-parsed options "-w" and "--minimal". Instead, rely on
> diff_opt_parse() to parse all of the diff options, including "-w" and
> "--minimal", and pass the resulting xdl_opts to xdi_diff().

Sounds useful: It allows more fine-grained control over which whitespace changes to ignore and which diff algorithm to use. There is a bit of overlap (e.g. with -b meaning show blank boundaries vs. ignore whitespace changes), but with your patch blame's own options still take precedence, so there should be no unpleasant surprises.

Show 6 quoted lines
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> Somebody who knows more about how diff operations are configured
> should please review this. I'm not certain that the change as
> implemented won't have other unwanted side-effects, though of course
> I checked that the test suite runs correctly.
I don't qualify, but I'll comment anyway..
>  builtin/blame.c        |  11 ++--
>  t/t4059-diff-indent.sh | 160 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 165 insertions(+), 6 deletions(-)
>  create mode 100755 t/t4059-diff-indent.sh

This new test doesn't call git blame. Does it belong to a different commit? And shouldn't the change to blame.c stand on its own, outside of this series?

Show 30 quoted lines
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 7ec7823..cde2d15 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -48,11 +48,12 @@ static int show_root;
>  static int reverse;
>  static int blank_boundary;
>  static int incremental;
> -static int xdl_opts;
>  static int abbrev = -1;
>  static int no_whole_file_rename;
>  static int show_progress;
>
> +static struct rev_info revs;
> +
>  static struct date_mode blame_date_mode = { DATE_ISO8601 };
>  static size_t blame_date_width;
>
> @@ -137,11 +138,12 @@ struct progress_info {
>  static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b,
>  		      xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)
>  {
> -	xpparam_t xpp = {0};
> +	xpparam_t xpp;
>  	xdemitconf_t xecfg = {0};
>  	xdemitcb_t ecb = {NULL};
>
> -	xpp.flags = xdl_opts;
> +	memset(&xpp, 0, sizeof(xpp));
> +	xpp.flags = revs.diffopt.xdl_opts;

Why call memset instead of using a static initializer? The intent of this patch is just to change the .flags assignment, isn't it?

Show 17 quoted lines
>  	xecfg.hunk_func = hunk_func;
>  	ecb.priv = cb_data;
>  	return xdi_diff(file_a, file_b, &xpp, &xecfg, &ecb);
> @@ -2517,7 +2519,6 @@ static int blame_move_callback(const struct option *option, const char *arg, int
>
>  int cmd_blame(int argc, const char **argv, const char *prefix)
>  {
> -	struct rev_info revs;
>  	const char *path;
>  	struct scoreboard sb;
>  	struct origin *o;
> @@ -2548,8 +2549,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  		OPT_BIT('l', NULL, &output_option, N_("Show long commit SHA1 (Default: off)"), OUTPUT_LONG_OBJECT_NAME),
>  		OPT_BIT('s', NULL, &output_option, N_("Suppress author name and timestamp (Default: off)"), OUTPUT_NO_AUTHOR),
>  		OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
> -		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
> -		OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),

This removes -w and --minimal from blame's short help; diff options should be mentioned somehow in exchange for that loss. Or perhaps they should be mentioned in git-rev-list(1)? (git blame -h points to git-rev-list(1) already.)

Documentation/git-blame.txt needs an update as well.
>  		OPT_STRING('S', NULL, &revs_file, N_("file"), N_("Use revisions from <file> instead of calling git-rev-list")),
>  		OPT_STRING(0, "contents", &contents_from, N_("file"), N_("Use <file>'s contents as the final image")),
>  		{ OPTION_CALLBACK, 'C', NULL, &opt, N_("score"), N_("Find line copies within and across files"), PARSE_OPT_OPTARG, blame_copy_callback },
Previous: Michael HaggertyNext: Michael Haggerty
Message 10 of 13 in “Better heuristics make prettier diffs”
  1. 0/7 Better heuristics make prettier diffsMichael Haggerty, Aug 22, 2016
  2. 2/7 xdl_change_compact(): only use heuristic if group can't be matchedMichael Haggerty, Aug 22, 2016
  3. 1/7 xdl_change_compact(): fix compaction heuristic to adjust ixoMichael Haggerty, Aug 22, 2016
  4. 6/7 diff: improve positioning of add/delete blocks in diffsMichael Haggerty, Aug 22, 2016
  5. 3/7 is_blank_line(): take a single xrecord_t as argumentMichael Haggerty, Aug 22, 2016
  6. 5/7 xdl_change_compact(): introduce the concept of a change groupMichael Haggerty, Aug 22, 2016
  7. Junio C HamanoAug 23, 2016
  8. 4/7 recs_match(): take two xrecord_t pointers as argumentsMichael Haggerty, Aug 22, 2016
  9. 7/7 blame: actually use the diff opts parsed from the command lineMichael Haggerty, Aug 22, 2016
  10. René ScharfeAug 23, 2016
  11. Michael HaggertySep 5, 2016
  12. Junio C HamanoSep 7, 2016
  13. Junio C HamanoAug 23, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.