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

[PATCH v3] 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]:

The current GIT_EXTERNAL_DIFF mechanism does not tell the number of
paths in the diff queue nor the current counter. To make this
"counter/total" info available for GIT_EXTERNAL_DIFF programs without
breaking existing ones:

(1) Modify run_external_diff() function in diff.c to set one environment
variable for a counter and one for the total number of files in the diff
queue. The size of the diff queue is already available in the
diff_queue_struct. For the counter define a new variable in the
diff_options struct and reset it to zero in diff_setup_done() function.
Pre-increment the counter inside the run_external_diff() function.

(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>
---

Reworked patch to use run_command_v_opt_cd_env() function when invoking
the external diff program. Modified test script to use write_script
helper function.

 Documentation/git.txt    |  9 +++++++++
 diff.c                   | 20 +++++++++++++++++---
 diff.h                   |  2 ++
 git-difftool--helper.sh  |  3 ++-
 t/t4020-diff-external.sh | 14 ++++++++++++++
 5 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4448ce2..10939ac 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -806,6 +806,15 @@ temporary file --- it is removed when 'GIT_EXTERNAL_DIFF' exits.
 +
 For a path that is unmerged, 'GIT_EXTERNAL_DIFF' is called with 1
 parameter, <path>.
++
+For each path 'GIT_EXTERNAL_DIFF' is called, two environment variables,
+'GIT_DIFF_PATH_COUNTER' and 'GIT_DIFF_PATH_TOTAL' are set.
+
+'GIT_DIFF_PATH_COUNTER'::
+	A 1-based counter incremented by one for every path.
+
+'GIT_DIFF_PATH_TOTAL'::
+	The total number of paths.
 
 other
 ~~~~~
diff --git a/diff.c b/diff.c
index e34bf97..a7d5a47 100644
--- a/diff.c
+++ b/diff.c
@@ -2899,11 +2899,16 @@ static void run_external_diff(const char *pgm,
 			      struct diff_filespec *one,
 			      struct diff_filespec *two,
 			      const char *xfrm_msg,
-			      int complete_rewrite)
+			      int complete_rewrite,
+			      struct diff_options *o)
 {
 	const char *spawn_arg[10];
 	int retval;
 	const char **arg = &spawn_arg[0];
+	struct diff_queue_struct *q = &diff_queued_diff;
+	const char *env[3] = { NULL };
+	char env_counter[50];
+	char env_total[50];
 
 	if (one && two) {
 		struct diff_tempfile *temp_one, *temp_two;
@@ -2928,7 +2933,14 @@ static void run_external_diff(const char *pgm,
 	}
 	*arg = NULL;
 	fflush(NULL);
-	retval = run_command_v_opt(spawn_arg, RUN_USING_SHELL);
+
+	env[0] = env_counter;
+	snprintf(env_counter, sizeof(env_counter), "GIT_DIFF_PATH_COUNTER=%d",
+			++o->diff_path_counter);
+	env[1] = env_total;
+	snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr);
+
+	retval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env);
 	remove_tempfile();
 	if (retval) {
 		fprintf(stderr, "external diff died, stopping at %s.\n", name);
@@ -3042,7 +3054,7 @@ static void run_diff_cmd(const char *pgm,
 
 	if (pgm) {
 		run_external_diff(pgm, name, other, one, two, xfrm_msg,
-				  complete_rewrite);
+				  complete_rewrite, o);
 		return;
 	}
 	if (one && two)
@@ -3317,6 +3329,8 @@ void diff_setup_done(struct diff_options *options)
 		options->output_format = DIFF_FORMAT_NO_OUTPUT;
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	}
+
+    options->diff_path_counter = 0;
 }
 
 static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
diff --git a/diff.h b/diff.h
index e342325..42bd34c 100644
--- a/diff.h
+++ b/diff.h
@@ -164,6 +164,8 @@ struct diff_options {
 	diff_prefix_fn_t output_prefix;
 	int output_prefix_length;
 	void *output_prefix_data;
+
+	int diff_path_counter;
 };
 
 enum color_diff {
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index b00ed95..7ef36b9 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -40,7 +40,8 @@ launch_merge_tool () {
 	# 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" "$GIT_DIFF_PATH_COUNTER" \
+			"$GIT_DIFF_PATH_TOTAL" "$MERGED"
 		if use_ext_cmd
 		then
 			printf "Launch '%s' [Y/n]: " \
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 8a30979..1855d31 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -193,6 +193,20 @@ test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '
 	GIT_EXTERNAL_DIFF=echo git diff
 '
 
+write_script external-diff.sh <<\EOF
+echo $GIT_DIFF_PATH_COUNTER of $GIT_DIFF_PATH_TOTAL >>counter.txt
+EOF
+
+test_expect_success 'GIT_EXTERNAL_DIFF path counter/total' '
+	GIT_EXTERNAL_DIFF=./external-diff.sh git diff &&
+	echo "1 of 2" >expect &&
+	head -n 1 counter.txt >actual &&
+	test_cmp expect actual &&
+	echo "2 of 2" >expect &&
+	tail -n 1 counter.txt >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '
 	touch file.ext &&
 	git add file.ext &&
-- 
1.8.4.4

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

[Junio C Hamano]

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

> Reworked patch to use run_command_v_opt_cd_env() function when invoking
> the external diff program. Modified test script to use write_script
> helper function.

Thanks; will queue with a minor tweak.

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

[Jeff King]

On Fri, Dec 06, 2013 at 10:38:46AM +1100, Zoltan Klinger wrote:

> @@ -2928,7 +2933,14 @@ static void run_external_diff(const char *pgm,
>  	}
>  	*arg = NULL;
>  	fflush(NULL);
> -	retval = run_command_v_opt(spawn_arg, RUN_USING_SHELL);
> +
> +	env[0] = env_counter;
> +	snprintf(env_counter, sizeof(env_counter), "GIT_DIFF_PATH_COUNTER=%d",
> +			++o->diff_path_counter);

I don't think we have a particular rule, but our usual style is to line
up the continued line of arguments with the open-paren of the function,
like:

  foo(arg1, arg2,
      arg3, arg4);

> @@ -3317,6 +3329,8 @@ void diff_setup_done(struct diff_options *options)
>  		options->output_format = DIFF_FORMAT_NO_OUTPUT;
>  		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
>  	}
> +
> +    options->diff_path_counter = 0;

It's hard to see with the email quoting, but this is a 4-space indent
rather than the usual 1-tab (which should be 8-wide on the terminals of
all True Believers).


Both are minor, but worth fixing IMHO (especially the second one). Looks
like it's too late for squashing, so here's a patch that can go on top
(doing it now is still of value, though, as it's less likely to create
conflicts since nobody is building on top yet).

-- >8 --
Subject: diff.c: fix some recent whitespace style violations

These were introduced by ee7fb0b.

Signed-off-by: Jeff King <peff@peff.net>
---
On top of zk/difftool-counts.

 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index a7d5a47..d69cc1b 100644
--- a/diff.c
+++ b/diff.c
@@ -2936,7 +2936,7 @@ static void run_external_diff(const char *pgm,
 
 	env[0] = env_counter;
 	snprintf(env_counter, sizeof(env_counter), "GIT_DIFF_PATH_COUNTER=%d",
-			++o->diff_path_counter);
+		 ++o->diff_path_counter);
 	env[1] = env_total;
 	snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr);
 
@@ -3330,7 +3330,7 @@ void diff_setup_done(struct diff_options *options)
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	}
 
-    options->diff_path_counter = 0;
+	options->diff_path_counter = 0;
 }
 
 static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
-- 
1.8.5.1.399.g900e7cd

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

[Junio C Hamano]

Thanks.

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

[David Aguilar]

Thanks for the patch, and sorry for the late response.
I have just a couple of notes below...

On Fri, Dec 06, 2013 at 10:38:46AM +1100, Zoltan Klinger wrote:
> diff --git a/diff.c b/diff.c
> index e34bf97..a7d5a47 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2899,11 +2899,16 @@ static void run_external_diff(const char *pgm,
>  			      struct diff_filespec *one,
>  			      struct diff_filespec *two,
>  			      const char *xfrm_msg,
> -			      int complete_rewrite)
> +			      int complete_rewrite,
> +			      struct diff_options *o)

Very minor nit -- "o" is a very terse variable name.
Maybe "opts"?

>  {
>  	const char *spawn_arg[10];
>  	int retval;
>  	const char **arg = &spawn_arg[0];
> +	struct diff_queue_struct *q = &diff_queued_diff;
> +	const char *env[3] = { NULL };
> +	char env_counter[50];
> +	char env_total[50];

Hard-coded 50; what's the length of the maximum signed int?


> diff --git a/diff.h b/diff.h
> index e342325..42bd34c 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -164,6 +164,8 @@ struct diff_options {
>  	diff_prefix_fn_t output_prefix;
>  	int output_prefix_length;
>  	void *output_prefix_data;
> +
> +	int diff_path_counter;
>  };

Since these are already "diff_options" it seems redundant to call
the struct entry the "diff_path_counter" when "path_count"
should be specific enough.  Would it make sense to rename it?

These are tiny nitpicky style notes; it looks good otherwise.

Thanks,
-- 
David

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Thanks for the patch, and sorry for the late response.
> I have just a couple of notes below...
>
> On Fri, Dec 06, 2013 at 10:38:46AM +1100, Zoltan Klinger wrote:
>> diff --git a/diff.c b/diff.c
>> index e34bf97..a7d5a47 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2899,11 +2899,16 @@ static void run_external_diff(const char *pgm,
>>  			      struct diff_filespec *one,
>>  			      struct diff_filespec *two,
>>  			      const char *xfrm_msg,
>> -			      int complete_rewrite)
>> +			      int complete_rewrite,
>> +			      struct diff_options *o)
>
> Very minor nit -- "o" is a very terse variable name.
> Maybe "opts"?

The diff-options parameter passed around in the callchain has always
been "o" throughout this file from the beginning of time, though ;-).

>
>>  {
>>  	const char *spawn_arg[10];
>>  	int retval;
>>  	const char **arg = &spawn_arg[0];
>> +	struct diff_queue_struct *q = &diff_queued_diff;
>> +	const char *env[3] = { NULL };
>> +	char env_counter[50];
>> +	char env_total[50];
>
> Hard-coded 50; what's the length of the maximum signed int?

;-) A bit of slack is fine, but 50 might be excessive (more than
twice as big as necessary).

>> diff --git a/diff.h b/diff.h
>> index e342325..42bd34c 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -164,6 +164,8 @@ struct diff_options {
>>  	diff_prefix_fn_t output_prefix;
>>  	int output_prefix_length;
>>  	void *output_prefix_data;
>> +
>> +	int diff_path_counter;
>>  };
>
> Since these are already "diff_options" it seems redundant to call
> the struct entry the "diff_path_counter" when "path_count"
> should be specific enough.  Would it make sense to rename it?

Yeah, makes sense.  diff_options->diff_path_counter++ sounds awful,
while options->path_count++ looks quite tame and reasonable.

> These are tiny nitpicky style notes; it looks good otherwise.

Thanks.