[PATCH] difftool: Change prompt to display the number of files in the diff queue

[PATCH] difftool: Change prompt to display the number of files in the diff queue

[Zoltan Klinger]

When --prompt option is set, git-difftool displays a prompt for each
modified file to be viewed in an external diff program. At that point it
could be useful to display a counter and the total number of files in
the diff queue.

Below is the current difftool prompt for the first of 5 modified files:
Viewing: 'diff.c'
Launch 'vimdiff' [Y/n]:

Consider the modified prompt:
Viewing (1/5): 'diff.c'
Launch 'vimdiff' [Y/n]:

(1) Modify run_external_diff() function in diff.c to pass a counter and
the total number of files in the diff queue to the external program.

(2) Modify git-difftool--helper.sh script to display the counter and the
diff queue count values in the difftool prompt.

(3) Update git.txt documentation

(4) Update t4020-diff-external.sh test script

Signed-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>
---
 Documentation/git.txt    |  6 +++++-
 diff.c                   | 14 +++++++++++++-
 git-difftool--helper.sh  |  8 +++++---
 t/t4020-diff-external.sh | 27 +++++++++++++++++++++------
 4 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index b73a24a..d8241bb 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -785,9 +785,10 @@ Git Diffs
 	When the environment variable 'GIT_EXTERNAL_DIFF' is set, the
 	program named by it is called, instead of the diff invocation
 	described above.  For a path that is added, removed, or modified,
-        'GIT_EXTERNAL_DIFF' is called with 7 parameters:
+	'GIT_EXTERNAL_DIFF' is called with 9 parameters:
 
 	path old-file old-hex old-mode new-file new-hex new-mode
+	counter total
 +
 where:
 
@@ -795,6 +796,9 @@ where:
                          contents of <old|new>,
 	<old|new>-hex:: are the 40-hexdigit SHA-1 hashes,
 	<old|new>-mode:: are the octal representation of the file modes.
+	counter:: is a numeric value incremented by one for every modified
+				file
+	total:: is the total number of modified files
 +
 The file parameters can point at the user's working file
 (e.g. `new-file` in "git-diff-files"), `/dev/null` (e.g. `old-file`
diff --git a/diff.c b/diff.c
index e34bf97..938f00a 100644
--- a/diff.c
+++ b/diff.c
@@ -37,6 +37,7 @@ static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
 static long diff_algorithm;
+static int diff_display_counter = 1;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -2901,9 +2902,16 @@ static void run_external_diff(const char *pgm,
 			      const char *xfrm_msg,
 			      int complete_rewrite)
 {
-	const char *spawn_arg[10];
+	const char *spawn_arg[12];
 	int retval;
 	const char **arg = &spawn_arg[0];
+	struct diff_queue_struct *q = &diff_queued_diff;
+
+	struct strbuf counterstr = STRBUF_INIT;
+	struct strbuf totalstr = STRBUF_INIT;
+	strbuf_addf(&counterstr, "%d", diff_display_counter++);
+	strbuf_addf(&totalstr, "%d", q->nr);
+
 
 	if (one && two) {
 		struct diff_tempfile *temp_one, *temp_two;
@@ -2918,6 +2926,8 @@ static void run_external_diff(const char *pgm,
 		*arg++ = temp_two->name;
 		*arg++ = temp_two->hex;
 		*arg++ = temp_two->mode;
+		*arg++ = counterstr.buf;
+		*arg++ = totalstr.buf;
 		if (other) {
 			*arg++ = other;
 			*arg++ = xfrm_msg;
@@ -2930,6 +2940,8 @@ static void run_external_diff(const char *pgm,
 	fflush(NULL);
 	retval = run_command_v_opt(spawn_arg, RUN_USING_SHELL);
 	remove_tempfile();
+	strbuf_release(&counterstr);
+	strbuf_release(&totalstr);
 	if (retval) {
 		fprintf(stderr, "external diff died, stopping at %s.\n", name);
 		exit(1);
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index b00ed95..4444c26 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -35,12 +35,14 @@ launch_merge_tool () {
 	LOCAL="$2"
 	REMOTE="$3"
 	BASE="$1"
+	COUNTER="$4"
+	TOTAL="$5"
 
 	# $LOCAL and $REMOTE are temporary files so prompt
 	# the user with the real $MERGED name before launching $merge_tool.
 	if should_prompt
 	then
-		printf "\nViewing: '%s'\n" "$MERGED"
+		printf "\nViewing (%s/%s): '%s'\n" "$COUNTER" "$TOTAL" "$MERGED"
 		if use_ext_cmd
 		then
 			printf "Launch '%s' [Y/n]: " \
@@ -82,7 +84,7 @@ else
 	# Launch the merge tool on each path provided by 'git diff'
 	while test $# -gt 6
 	do
-		launch_merge_tool "$1" "$2" "$5"
-		shift 7
+		launch_merge_tool "$1" "$2" "$5" "$8" "$9"
+		shift 9
 	done
 fi
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 8a30979..a8cf9c6 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -8,7 +8,9 @@ test_expect_success setup '
 
 	test_tick &&
 	echo initial >file &&
-	git add file &&
+	echo foo >file2 &&
+	echo bar >file3 &&
+	git add file file2 file3 &&
 	git commit -m initial &&
 
 	test_tick &&
@@ -18,16 +20,20 @@ test_expect_success setup '
 
 	test_tick &&
 	echo third >file
+	echo quux >file2
+	echo quux >file3
 '
 
 test_expect_success 'GIT_EXTERNAL_DIFF environment' '
 
 	GIT_EXTERNAL_DIFF=echo git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
+		read path oldfile oldhex oldmode newfile newhex newmode counter total &&
 		test "z$path" = zfile &&
 		test "z$oldmode" = z100644 &&
 		test "z$newhex" = "z$_z40" &&
 		test "z$newmode" = z100644 &&
+		test "z$counter" = z1 &&
+		test "z$total" = z3 &&
 		oh=$(git rev-parse --verify HEAD:file) &&
 		test "z$oh" = "z$oldhex"
 	}
@@ -49,14 +55,17 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '
 '
 
 test_expect_success SYMLINKS 'typechange diff' '
+	git checkout -- file2 file3 &&
 	rm -f file &&
 	ln -s elif file &&
 	GIT_EXTERNAL_DIFF=echo git diff  | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
+		read path oldfile oldhex oldmode newfile newhex newmode counter total &&
 		test "z$path" = zfile &&
 		test "z$oldmode" = z100644 &&
 		test "z$newhex" = "z$_z40" &&
 		test "z$newmode" = z120000 &&
+		test "z$counter" = z1 &&
+		test "z$total" = z1 &&
 		oh=$(git rev-parse --verify HEAD:file) &&
 		test "z$oh" = "z$oldhex"
 	} &&
@@ -70,11 +79,13 @@ test_expect_success 'diff.external' '
 	echo third >file &&
 	test_config diff.external echo &&
 	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
+		read path oldfile oldhex oldmode newfile newhex newmode counter total &&
 		test "z$path" = zfile &&
 		test "z$oldmode" = z100644 &&
 		test "z$newhex" = "z$_z40" &&
 		test "z$newmode" = z100644 &&
+		test "z$counter" = z1 &&
+		test "z$total" = z1 &&
 		oh=$(git rev-parse --verify HEAD:file) &&
 		test "z$oh" = "z$oldhex"
 	}
@@ -101,11 +112,13 @@ test_expect_success 'diff attribute' '
 	echo >.gitattributes "file diff=parrot" &&
 
 	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
+		read path oldfile oldhex oldmode newfile newhex newmode counter total &&
 		test "z$path" = zfile &&
 		test "z$oldmode" = z100644 &&
 		test "z$newhex" = "z$_z40" &&
 		test "z$newmode" = z100644 &&
+		test "z$counter" = z1 &&
+		test "z$total" = z1 &&
 		oh=$(git rev-parse --verify HEAD:file) &&
 		test "z$oh" = "z$oldhex"
 	}
@@ -134,11 +147,13 @@ test_expect_success 'diff attribute' '
 	echo >.gitattributes "file diff=color" &&
 
 	git diff | {
-		read path oldfile oldhex oldmode newfile newhex newmode &&
+		read path oldfile oldhex oldmode newfile newhex newmode counter total &&
 		test "z$path" = zfile &&
 		test "z$oldmode" = z100644 &&
 		test "z$newhex" = "z$_z40" &&
 		test "z$newmode" = z100644 &&
+		test "z$counter" = z1 &&
+		test "z$total" = z1 &&
 		oh=$(git rev-parse --verify HEAD:file) &&
 		test "z$oh" = "z$oldhex"
 	}
-- 
1.8.4.4

Re: [PATCH] difftool: Change prompt to display the number of files in the diff queue

[Junio C Hamano]

Zoltan Klinger <zoltan.klinger@gmail.com> writes:

> When --prompt option is set, git-difftool displays a prompt for each
> modified file to be viewed in an external diff program. At that point it
> could be useful to display a counter and the total number of files in
> the diff queue.
>
> Below is the current difftool prompt for the first of 5 modified files:
> Viewing: 'diff.c'
> Launch 'vimdiff' [Y/n]:
>
> Consider the modified prompt:
> Viewing (1/5): 'diff.c'
> Launch 'vimdiff' [Y/n]:
>
> (1) Modify run_external_diff() function in diff.c to pass a counter and
> the total number of files in the diff queue to the external program.
>
> (2) Modify git-difftool--helper.sh script to display the counter and the
> diff queue count values in the difftool prompt.
>
> (3) Update git.txt documentation
>
> (4) Update t4020-diff-external.sh test script
>
> Signed-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>
> ---
>  Documentation/git.txt    |  6 +++++-
>  diff.c                   | 14 +++++++++++++-
>  git-difftool--helper.sh  |  8 +++++---
>  t/t4020-diff-external.sh | 27 +++++++++++++++++++++------
>  4 files changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index b73a24a..d8241bb 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -785,9 +785,10 @@ Git Diffs
>  	When the environment variable 'GIT_EXTERNAL_DIFF' is set, the
>  	program named by it is called, instead of the diff invocation
>  	described above.  For a path that is added, removed, or modified,
> -        'GIT_EXTERNAL_DIFF' is called with 7 parameters:
> +	'GIT_EXTERNAL_DIFF' is called with 9 parameters:
>  
>  	path old-file old-hex old-mode new-file new-hex new-mode
> +	counter total

As the "git difftool" is not the only thing that reads using the
GIT_EXTERNAL_DIFF mechanism (it is for general consumption by end
user scripts), I wonder how/if this change breaks existing scripts.
I do recall writing an EXTERNAL_DIFF script myself that began by
switching on $# (i.e. the number of arguments) to check the state of
the given path, like this:

	case $# in
        1)
        	... handle unmerged path ...
                ;;
	7)
        	... handle comparison ...
		;;
	*)
        	die "Unexpected number of arguments to $0: $#"
                ;;
	esac

which will be broken by this change. Updating such scripts is
trivial but that does not change the fact that this change is
forcing an unnecessary work on our users to adjust their scripts
that have been working perfectly fine.  So I think this, if we were
to apply, may need a compatibility warning in large flashing red
letters in the release notes.

>  +
>  where:
>  
> @@ -795,6 +796,9 @@ where:
>                           contents of <old|new>,
>  	<old|new>-hex:: are the 40-hexdigit SHA-1 hashes,
>  	<old|new>-mode:: are the octal representation of the file modes.
> +	counter:: is a numeric value incremented by one for every modified
> +				file
> +	total:: is the total number of modified files
>  +
>  The file parameters can point at the user's working file
>  (e.g. `new-file` in "git-diff-files"), `/dev/null` (e.g. `old-file`
> diff --git a/diff.c b/diff.c
> index e34bf97..938f00a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -37,6 +37,7 @@ static int diff_stat_graph_width;
>  static int diff_dirstat_permille_default = 30;
>  static struct diff_options default_diff_options;
>  static long diff_algorithm;
> +static int diff_display_counter = 1;

There should be a place somewhere, e.g. diff_setup_done(), to reset
this counter to 0 (and the site that uses the variable should
pre-increment it instead of relying the initial value being 1), so
that a single program could later run the diff machinery more than
once for different set of files.  This counter may actually belong
to diff_options, just like existing "found_changes" and
"found_follow" fields are there to keep track of state of the diff
machinery per invocation.

Having said all that, the fact that the current arrangement since we
introduced GIT_EXTERNAL_DIFF mechanism does not tell how many paths
there are in the output is indeed bad.  If a script that uses
GIT_EXTERNAL_DIFF wants to first collect all the paths and the
parameters and then show everything in a single UI, such a script
may want to (1) start collecting the paths and args to a persistent
place (e.g. starting a GUI diff daemon for the first path it gets,
or starting a new temporary file), (2) keep collecting the paths and
args, and then (3) after collecting all, present diff for all paths
it obtained, but it is impossible because there is no cue when a
series of external diff calls starts or ends.

And this "counter/total" mechanism could be one possible solution to
it (another possibility is to make an extra dummy call to signal the
end, perhaps with no parameters---the one that is collecting can
then know how many paths there are and which one is the Nth path).