threads / patch / 35416

patchdifftool: Change prompt to display the number of files in the diff queue

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

## tl;dr

2 messages between Nov 28, 2013 and Dec 2, 2013. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Zoltan Klinger· Nov 28, 2013, 00:49 UTC · lore

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(-)
Show changes to 4 files +44 −11

Documentation/git.txt, diff.c, git-difftool--helper.sh, t/t4020-diff-external.sh

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
Junio C Hamano· Dec 2, 2013, 21:08 UTC · re: Zoltan Klinger · lore

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

Zoltan Klinger <zoltan.klinger@gmail.com> writes:
Show 44 quoted lines
> 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.

Show 22 quoted lines
>  +
>  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).

← back to recent threads