[PATCH, RFC] diff: add option to show context between close chunks

[PATCH, RFC] diff: add option to show context between close chunks

[René Scharfe]

This patch adds a new diff option, --inter-chunk-context.  It can be
used to show the context in gaps between chunks, thereby creating a
big chunk out of two close chunks, in order to have an unbroken
context, making reviews easier.

With --inter-chunk-context=1, patches have the same number of lines
as without the option, as only the chunk header is replaced by the
context line it was shadowing.

You can use commit b0b44bc7b26c8c4b4221a377ce6ba174b843cb8d in the
git repo to try out this option; there's a chunk in transport.c
which is just one line away from the next.  (I found this option
helpful in reviewing my own patch before sending. :)

I think it makes sense to make 1, or even 3, the default for this
option for all commands that create patches intended for human
consumption.  The patch keeps the default at 0, though.

There are downsides, of course: values higher than 1 potentially make
the resulting patch longer.  More context means a higher probability
of (perhaps unnecessary) merge conflicts.

Comments?
---
 Documentation/diff-options.txt      |    4 ++
 diff.c                              |    4 ++
 diff.h                              |    1 +
 t/t4030-diff-inter-chunk-context.sh |   75 +++++++++++++++++++++++++++++++++++
 xdiff/xdiff.h                       |    1 +
 xdiff/xemit.c                       |    3 +-
 6 files changed, 87 insertions(+), 1 deletions(-)
 create mode 100755 t/t4030-diff-inter-chunk-context.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c62b45c..e0c6d8c 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -215,6 +215,10 @@ endif::git-format-patch[]
 -w::
 	Shorthand for "--ignore-all-space".
 
+--inter-chunk-context=<lines>::
+	Show the context between diff chunks, up to the specified number
+	of lines, thereby fusing chunks that are close to each other.
+
 --exit-code::
 	Make the program exit with codes similar to diff(1).
 	That is, it exits with 1 if there were differences and
diff --git a/diff.c b/diff.c
index 1c6be89..4a3b486 100644
--- a/diff.c
+++ b/diff.c
@@ -1594,6 +1594,7 @@ static void builtin_diff(const char *name_a,
 		ecbdata.file = o->file;
 		xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
 		xecfg.ctxlen = o->context;
+		xecfg.interchunkctxlen = o->interchunkcontext;
 		xecfg.flags = XDL_EMIT_FUNCNAMES;
 		if (pe)
 			xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);
@@ -2677,6 +2678,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->b_prefix = arg + 13;
 	else if (!strcmp(arg, "--no-prefix"))
 		options->a_prefix = options->b_prefix = "";
+	else if (opt_arg(arg, '\0', "inter-chunk-context",
+			 &options->interchunkcontext))
+		;
 	else if (!prefixcmp(arg, "--output=")) {
 		options->file = fopen(arg + strlen("--output="), "w");
 		options->close_file = 1;
diff --git a/diff.h b/diff.h
index a49d865..6003024 100644
--- a/diff.h
+++ b/diff.h
@@ -77,6 +77,7 @@ struct diff_options {
 	const char *a_prefix, *b_prefix;
 	unsigned flags;
 	int context;
+	int interchunkcontext;
 	int break_opt;
 	int detect_rename;
 	int skip_stat_unmatch;
diff --git a/t/t4030-diff-inter-chunk-context.sh b/t/t4030-diff-inter-chunk-context.sh
new file mode 100755
index 0000000..448f1b9
--- /dev/null
+++ b/t/t4030-diff-inter-chunk-context.sh
@@ -0,0 +1,75 @@
+#!/bin/sh
+
+test_description='diff chunk merging'
+
+. ./test-lib.sh
+
+f() {
+	i=1
+	echo $1
+	while test $i -le $2
+	do
+		echo $i
+		i=$(expr $i + 1)
+	done
+	echo $3
+}
+
+test_expect_success 'setup' '
+	f a 1 b >f1 &&
+	f a 2 b >f2 &&
+	f a 3 b >f3 &&
+	git add f1 f2 f3 &&
+	git commit -q -m. f1 f2 f3 &&
+	f x 1 y >f1 &&
+	f x 2 y >f2 &&
+	f x 3 y >f3
+'
+
+t() {
+	case "$2" in
+	'')	cmd="diff -U$1" ;;
+	*)	cmd="diff -U$1 --inter-chunk-context=$2" ;;
+	esac
+	label="$cmd, $3 common $4"
+
+	test_expect_success "$label: count chunks ($5)" "
+		test $(git $cmd f$3 | grep '^@@ ' | wc -l) = $5
+	"
+
+	expected="expected.$1.$3.$5"
+	test -f $expected &&
+	test_expect_success "$label: check output" "
+		git $cmd f$3 >actual &&
+		test_cmp $expected actual
+	"
+}
+
+cat <<EOF >expected.0.1.1 || exit 1
+diff --git a/f1 b/f1
+index f1e80ce..ae397d0 100644
+--- a/f1
++++ b/f1
+@@ -1,3 +1,3 @@
+-a
++x
+ 1
+-b
++y
+EOF
+
+# ctx	intrctx	common	lines	chunks
+t 0	''	1	line	2
+t 0	0	1	line	2
+t 0	1	1	line	1
+t 0	2	1	line	1
+t 0	''	2	lines	2
+t 0	0	2	lines	2
+t 0	1	2	lines	2
+t 0	2	2	lines	1
+t 1	''	3	lines	2
+t 1	0	3	lines	2
+t 1	1	3	lines	1
+t 1	2	3	lines	1
+
+test_done
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index deebe02..d8f14e6 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -84,6 +84,7 @@ typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long
 
 typedef struct s_xdemitconf {
 	long ctxlen;
+	long interchunkctxlen;
 	unsigned long flags;
 	find_func_t find_func;
 	void *find_func_priv;
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d3d9c84..3bf2581 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -60,9 +60,10 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *
  */
 static xdchange_t *xdl_get_hunk(xdchange_t *xscr, xdemitconf_t const *xecfg) {
 	xdchange_t *xch, *xchp;
+	long max_common = 2 * xecfg->ctxlen + xecfg->interchunkctxlen;
 
 	for (xchp = xscr, xch = xscr->next; xch; xchp = xch, xch = xch->next)
-		if (xch->i1 - (xchp->i1 + xchp->chg1) > 2 * xecfg->ctxlen)
+		if (xch->i1 - (xchp->i1 + xchp->chg1) > max_common)
 			break;
 
 	return xchp;
-- 
1.6.0.2.287.g3791f

Re: [PATCH, RFC] diff: add option to show context between close chunks

[Johannes Sixt]

René Scharfe schrieb:
> This patch adds a new diff option, --inter-chunk-context.  It can be
> used to show the context in gaps between chunks, thereby creating a
> big chunk out of two close chunks, in order to have an unbroken
> context, making reviews easier.
> 
> With --inter-chunk-context=1, patches have the same number of lines
> as without the option, as only the chunk header is replaced by the
> context line it was shadowing.
> 
> You can use commit b0b44bc7b26c8c4b4221a377ce6ba174b843cb8d in the
> git repo to try out this option; there's a chunk in transport.c
> which is just one line away from the next.  (I found this option
> helpful in reviewing my own patch before sending. :)
> 
> I think it makes sense to make 1, or even 3, the default for this
> option for all commands that create patches intended for human
> consumption.  The patch keeps the default at 0, though.
> 
> There are downsides, of course: values higher than 1 potentially make
> the resulting patch longer.  More context means a higher probability
> of (perhaps unnecessary) merge conflicts.
> 
> Comments?

Why can't you just use -U6 instead instead of --inter-chunk-context=3? If
this is intended for human consumption anyway, then you can just as well
increase the overall number of context lines: You get extra context lines
in the places where hunks are not fused, but this cannot be a disadvantage
for the targeted audience.

-- Hannes

Re: [PATCH, RFC] diff: add option to show context between close chunks

[René Scharfe]

Johannes Sixt schrieb:
> Why can't you just use -U6 instead instead of --inter-chunk-context=3? If
> this is intended for human consumption anyway, then you can just as well
> increase the overall number of context lines: You get extra context lines
> in the places where hunks are not fused, but this cannot be a disadvantage
> for the targeted audience.

The extra lines _are_ a disadvantage for me, since each chunk gets them
while only close ones need them -- that's wasted space.  The intent is
to save time by not having to apply and rediff a patch on a mailing list
in order to see the hidden text; part of the savings would be lost if
the reader had to skip over extra lines.

Besides, this option is probably most useful if set by default on
machines not controlled by me.  E.g. I wouldn't want to change an option
on a gitweb page, I'd want to get fused chunks by default.  I wouldn't
want to write "best viewed with -U6" in a patch description, I'd want it
to just work (e.g. for gitk users).

I have to admit my main motivation was that one line gap, where a chunk
header hid an interesting line of context.  Showing it didn't change the
length of the patch, so I found this to be a sad wastage.

Optimizing patches for readability makes sense since they are such an
important part of our communication here.  I think --i-c-c=1 results in
an obvious win without much of a downside.  I was surprised to find that
almost 4% of the chunks in git 1.6.0 would be eliminated by that option:

   $ git log -p v1.6.0 | grep -c '^@@ '
   60544
   $ git log -p v1.6.0 --inter-chunk-context=1 | grep -c '^@@ '
   58471

But perhaps a higher value would be even better?

   $ git log v1.6.0 | wc -l
   225441
   $ git log -p v1.6.0 | wc -l
   1736188
   $ git log -p --inter-chunk-context=10 v1.6.0 | wc -l
   1779048
   $ git log -p -U8 v1.6.0 | wc -l
   2214448

Well, I don't know if those patches are easier to read (haven't looked
at them), but I imagine that some related changes are presented with the
full context between them (e.g. changes to loop header and footer,
function signature and body).  The numbers only show that it's
affordable (3% more lines with -i-c-c=10, 30% more lines with the
comparable -U8).

(Why 10?  With the default of -U3, a chunk is 6 lines of context plus at
least one line of actual change.  Two chunks are at least 14 lines long,
thus only 10 more fit into 24 lines, i.e. a terminal window..)

René

Re: [PATCH, RFC] diff: add option to show context between close chunks

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> I think it makes sense to make 1, or even 3, the default for this
> option for all commands that create patches intended for human
> consumption.  The patch keeps the default at 0, though.

I think defaulting to 1 would make sense, or alternatively, just
hardcoding that behaviour without any new option.  That would give you
more information with the same number of patch lines, iow, upside without
any downside.

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d3d9c84..3bf2581 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -60,9 +60,9 @@ static int xdl_emit_record(xdfile_t *xdf, 
  */
 static xdchange_t *xdl_get_hunk(xdchange_t *xscr, xdemitconf_t const *xecfg) {
 	xdchange_t *xch, *xchp;
 
 	for (xchp = xscr, xch = xscr->next; xch; xchp = xch, xch = xch->next)
-		if (xch->i1 - (xchp->i1 + xchp->chg1) > 2 * xecfg->ctxlen)
+		if (xch->i1 - (xchp->i1 + xchp->chg1) > 2 * xecfg->ctxlen + 1)
 			break;
 
 	return xchp;

Re: [PATCH, RFC] diff: add option to show context between close chunks

[Johannes Sixt]

René Scharfe schrieb:
> I have to admit my main motivation was that one line gap, where a chunk
> header hid an interesting line of context.  Showing it didn't change the
> length of the patch, so I found this to be a sad wastage.

"Wastage" is relative. For a given patch, the one line of context that was
hidden by the hunk header would be welcome by a human reader, but it is
not necessarily useful if the patch is to be applied, in particular, if it
is applied to a version of the file that has *more* than one line between
the hunk contexts. This is the reason that diff does not produce 7 lines
of context between changes in -U3 mode ("you asked for 3 lines of context,
you get 3 lines of context").

BTW, nomenclature seems to have settled at the word "hunk", not "chunk".

-- Hannes

Re: [PATCH, RFC] diff: add option to show context between close chunks

[Johannes Sixt]

Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> I think it makes sense to make 1, or even 3, the default for this
>> option for all commands that create patches intended for human
>> consumption.  The patch keeps the default at 0, though.
> 
> I think defaulting to 1 would make sense, or alternatively, just
> hardcoding that behaviour without any new option.  That would give you
> more information with the same number of patch lines, iow, upside without
> any downside.

Are you sure about the "without any downside" part? The extra context line
inhibits that the patch applies cleanly to a version of the file that has
that very line modified (including a different number of lines).

-- Hannes

Re: [PATCH, RFC] diff: add option to show context between close chunks

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
>> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>> 
>>> I think it makes sense to make 1, or even 3, the default for this
>>> option for all commands that create patches intended for human
>>> consumption.  The patch keeps the default at 0, though.
>> 
>> I think defaulting to 1 would make sense, or alternatively, just
>> hardcoding that behaviour without any new option.  That would give you
>> more information with the same number of patch lines, iow, upside without
>> any downside.
>
> Are you sure about the "without any downside" part? The extra context line
> inhibits that the patch applies cleanly to a version of the file that has
> that very line modified (including a different number of lines).

Yeah.  René wanted this for _human consumption_, not mechanical patch
application, so "hardcoding" literally there in the very low level of the
diff callchain is not quite right (it would affect format-patch which is
primarily for mechanical application).

I guess you could make the hardcoded value 1 for everybody else and 0 for
format-patch.

Re: [PATCH, RFC] diff: add option to show context between close chunks

[Jeff King]

On Tue, Oct 21, 2008 at 12:12:17AM -0700, Junio C Hamano wrote:

> Yeah.  René wanted this for _human consumption_, not mechanical patch
> application, so "hardcoding" literally there in the very low level of the
> diff callchain is not quite right (it would affect format-patch which is
> primarily for mechanical application).
> 
> I guess you could make the hardcoded value 1 for everybody else and 0 for
> format-patch.

I see your reasoning, but at the same time, a large portion of patches I
read are from format-patch (and René even said that he was trying to
save the user from the "apply then diff just to look at the patch"
annoyance). And I have personally, as a patch submitter, created some
format-patch output sent to the git list with -U5 to combine hunks and
make it more readable for reviewers.

Not to mention that I sometimes apply or post the output of "git diff".

To me that it implies that either:

 - the increased chance of conflict is not a problem in practice, and we
   should have the option on by default everywhere

 - it is a problem, in which case we should ask the user to turn on the
   feature manually instead of second-guessing how they will use the
   resulting patch (which they might not even know, since they are
   making assumptions about how other people might use the patch, and
   they must decide for their situation between shipping something that
   is more readable but slightly more conflict prone, or as easy to
   apply as possible)

-Peff

Re: [PATCH, RFC] diff: add option to show context between close chunks

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1363 bytes --]

On Tue, 21 Oct 2008, Johannes Sixt wrote:

> Junio C Hamano schrieb:
> > René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> > 
> >> I think it makes sense to make 1, or even 3, the default for this
> >> option for all commands that create patches intended for human
> >> consumption.  The patch keeps the default at 0, though.
> > 
> > I think defaulting to 1 would make sense, or alternatively, just
> > hardcoding that behaviour without any new option.  That would give you
> > more information with the same number of patch lines, iow, upside without
> > any downside.
> 
> Are you sure about the "without any downside" part? The extra context line
> inhibits that the patch applies cleanly to a version of the file that has
> that very line modified (including a different number of lines).

We could start allowing "fuzz" by default in the case of a patch with more 
context than we'd expect to see. That is, git-apply would ignore context 
lines more than 3 lines away from any changed lines, sharing the 
assumption of our patch-generation side that lines that far away don't 
matter in general. (Now, if people were in the habit of including as 
context additional lines in fragile locations, this wouldn't be a good 
assumption, but I doubt anybody would be able to identify such lines to 
include them).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH, RFC] diff: add option to show context between close chunks

[René Scharfe]

Johannes Sixt schrieb:
> René Scharfe schrieb:
>> I have to admit my main motivation was that one line gap, where a chunk
>> header hid an interesting line of context.  Showing it didn't change the
>> length of the patch, so I found this to be a sad wastage.
> 
> "Wastage" is relative. For a given patch, the one line of context that was
> hidden by the hunk header would be welcome by a human reader, but it is
> not necessarily useful if the patch is to be applied, in particular, if it
> is applied to a version of the file that has *more* than one line between
> the hunk contexts. This is the reason that diff does not produce 7 lines
> of context between changes in -U3 mode ("you asked for 3 lines of context,
> you get 3 lines of context").

Yes, that's an interesting example of the possible "merge conflicts" I
mentioned in my original mail, and one I didn't think of.  And since I
don't do any merges myself, I don't know how much of a problem this is.

As Daniel writes, one could teach git-apply to ignore extra context.  It
should even be possible to infer the -U option used to create a patch
and to remove any extra lines (which might get complicated for patches
that change both the start and the end of a file, though).

Also, I'd like to know how many patches of a given repo would be have
created such a problem, but I can't think of a way to count them at the
moment.  I need some sleep first.

> BTW, nomenclature seems to have settled at the word "hunk", not "chunk".

While http://en.wikipedia.org/wiki/Diff uses both and defines "chunk" as
"change hunk", the GNU patch(1) manpage uses "hunk" throughout.  "Hunk"
it is, then. :)

René

Re: [PATCH, RFC] diff: add option to show context between close chunks

[René Scharfe]

Jeff King schrieb:
> On Tue, Oct 21, 2008 at 12:12:17AM -0700, Junio C Hamano wrote:
> 
>> Yeah.  René wanted this for _human consumption_, not mechanical patch
>> application, so "hardcoding" literally there in the very low level of the
>> diff callchain is not quite right (it would affect format-patch which is
>> primarily for mechanical application).
>>
>> I guess you could make the hardcoded value 1 for everybody else and 0 for
>> format-patch.
> 
> I see your reasoning, but at the same time, a large portion of patches I
> read are from format-patch (and René even said that he was trying to
> save the user from the "apply then diff just to look at the patch"
> annoyance). And I have personally, as a patch submitter, created some
> format-patch output sent to the git list with -U5 to combine hunks and
> make it more readable for reviewers.
> 
> Not to mention that I sometimes apply or post the output of "git diff".

Well, yes, perhaps I was trying to get ahead of myself.  I sure would
like to see everyone create patches with fused hunks (because they are
easier to read), but step 1 is to have the option to create such patches
at all.  We should then try it out for some time or verify its
usefulness statistically and only then turn it on by default.  Or
perhaps throw it away, depending on the results.

And I consider the output of format-patch and git-diff to be intended
primarily for human consumption.

> To me that it implies that either:
> 
>  - the increased chance of conflict is not a problem in practice, and we
>    should have the option on by default everywhere
> 
>  - it is a problem, in which case we should ask the user to turn on the
>    feature manually instead of second-guessing how they will use the
>    resulting patch (which they might not even know, since they are
>    making assumptions about how other people might use the patch, and
>    they must decide for their situation between shipping something that
>    is more readable but slightly more conflict prone, or as easy to
>    apply as possible)

To decide which one it is, I'd like to see numbers: how many times would
a patch with fused hunks have led to a problem for e.g. the kernel repo?
  What is the optimal default value (0, 1, even more)?  Before even
thinking about how to get these stats, I'd better head for bed for
today, though..

René