Subject: Re: [PATCH v4 05/21] range-diff: also show the diff between patches

From: Thomas Gummerer <[email-masked]>
Date: Sun, 29 Jul 2018 20:03:59 +0100

On 07/21, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email-masked]>
> 
> Just like tbdiff, we now show the diff between matching patches. This is
> a "diff of two diffs", so it can be a bit daunting to read for the
> beginner.
> 
> An alternative would be to display an interdiff, i.e. the hypothetical
> diff which is the result of first reverting the old diff and then
> applying the new diff.
> 
> Especially when rebasing often, an interdiff is often not feasible,
> though: if the old diff cannot be applied in reverse (due to a moving
> upstream), an interdiff can simply not be inferred.
> 
> This commit brings `range-diff` closer to feature parity with regard
> to tbdiff.
> 
> To make `git range-diff` respect e.g. color.diff.* settings, we have
> to adjust git_branch_config() accordingly.
> 
> Note: while we now parse diff options such as --color, the effect is not
> yet the same as in tbdiff, where also the commit pairs would be colored.
> This is left for a later commit.
> 
> Note also: while tbdiff accepts the `--no-patches` option to suppress
> these diffs between patches, we prefer the `-s` option that is
> automatically supported via our use of diff_opt_parse().

One slightly unfortunate thing here is that we don't show these
options in 'git range-diff -h', which would be nice to have.  I don't
know if that's possible in git right now, if it's not easily possible,
I definitely wouldn't want to delay this series for that, and we could
just add it to the list of possible future enhancements that other
people mentioned.

> Signed-off-by: Johannes Schindelin <[email-masked]>
> ---
>  builtin/range-diff.c | 25 ++++++++++++++++++++++---
>  range-diff.c         | 34 +++++++++++++++++++++++++++++++---
>  range-diff.h         |  4 +++-
>  3 files changed, 56 insertions(+), 7 deletions(-)
>
> [...]
> 
> diff --git a/range-diff.c b/range-diff.c
> index 2d94200d3..71883a4b7 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -6,6 +6,7 @@
>  #include "hashmap.h"
>  #include "xdiff-interface.h"
>  #include "linear-assignment.h"
> +#include "diffcore.h"
>  
>  struct patch_util {
>  	/* For the search for an exact match */
> @@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
>  	return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
>  }
>  
> -static void output(struct string_list *a, struct string_list *b)
> +static struct diff_filespec *get_filespec(const char *name, const char *p)
> +{
> +	struct diff_filespec *spec = alloc_filespec(name);
> +
> +	fill_filespec(spec, &null_oid, 0, 0644);
> +	spec->data = (char *)p;
> +	spec->size = strlen(p);
> +	spec->should_munmap = 0;
> +	spec->is_stdin = 1;
> +
> +	return spec;
> +}
> +
> +static void patch_diff(const char *a, const char *b,
> +			      struct diff_options *diffopt)
> +{
> +	diff_queue(&diff_queued_diff,
> +		   get_filespec("a", a), get_filespec("b", b));
> +
> +	diffcore_std(diffopt);
> +	diff_flush(diffopt);
> +}
> +
> +static void output(struct string_list *a, struct string_list *b,
> +		   struct diff_options *diffopt)
>  {
>  	int i = 0, j = 0;
>  
> @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct string_list *b)
>  			printf("%d: %s ! %d: %s\n",
>  			       b_util->matching + 1, short_oid(a_util),
>  			       j + 1, short_oid(b_util));
> +			if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))

Looking at this line, it looks like it would be easy to support
'--no-patches' as well, which may be slightly easier to understand that
'-s' to someone new to the command.  But again that can be added later
if someone actually cares about it.

> +				patch_diff(a->items[b_util->matching].string,
> +					   b->items[j].string, diffopt);
>  			a_util->shown = 1;
>  			j++;
>  		}
> @@ -307,7 +335,7 @@ static void output(struct string_list *a, struct string_list *b)
>  }
>  
>  int show_range_diff(const char *range1, const char *range2,
> -		    int creation_factor)
> +		    int creation_factor, struct diff_options *diffopt)
>  {
>  	int res = 0;
>  
> @@ -322,7 +350,7 @@ int show_range_diff(const char *range1, const char *range2,
>  	if (!res) {
>  		find_exact_matches(&branch1, &branch2);
>  		get_correspondences(&branch1, &branch2, creation_factor);
> -		output(&branch1, &branch2);
> +		output(&branch1, &branch2, diffopt);
>  	}
>  
>  	string_list_clear(&branch1, 1);
> diff --git a/range-diff.h b/range-diff.h
> index dd30449c4..aea9d43f3 100644
> --- a/range-diff.h
> +++ b/range-diff.h
> @@ -1,7 +1,9 @@
>  #ifndef BRANCH_DIFF_H
>  #define BRANCH_DIFF_H
>  
> +#include "diff.h"
> +
>  int show_range_diff(const char *range1, const char *range2,
> -		    int creation_factor);
> +		    int creation_factor, struct diff_options *diffopt);
>  
>  #endif
> -- 
> gitgitgadget
>