{"thread":{"id":"35484","subject":"[PATCH v3] difftool: Change prompt to display the number of files in the diff queue","startedAt":"2013-12-05T23:38:46Z","lastAt":"2013-12-18T06:06:19Z","messageCount":6,"participants":["Zoltan Klinger","Junio C Hamano","Jeff King","David Aguilar"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"231644","messageId":"1386286726-26653-1-git-send-email-zoltan.klinger@gmail.com","threadId":"35484","inReplyTo":null,"subject":"[PATCH v3] difftool: Change prompt to display the number of files in the diff queue","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2013-12-05T23:38:46Z","receivedAt":"2013-12-05T23:38:46Z","isPatch":true,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"When --prompt option is set, git-difftool displays a prompt for each modified\nfile to be viewed in an external diff program. At that point it could be useful\nto display a counter and the total number of files in the diff queue.\n\nBelow is the current difftool prompt for the first of 5 modified files:\nViewing: 'diff.c'\nLaunch 'vimdiff' [Y/n]:\n\nConsider the modified prompt:\nViewing (1/5): 'diff.c'\nLaunch 'vimdiff' [Y/n]:\n\nThe current GIT_EXTERNAL_DIFF mechanism does not tell the number of\npaths in the diff queue nor the current counter. To make this\n\"counter/total\" info available for GIT_EXTERNAL_DIFF programs without\nbreaking existing ones:\n\n(1) Modify run_external_diff() function in diff.c to set one environment\nvariable for a counter and one for the total number of files in the diff\nqueue. The size of the diff queue is already available in the\ndiff_queue_struct. For the counter define a new variable in the\ndiff_options struct and reset it to zero in diff_setup_done() function.\nPre-increment the counter inside the run_external_diff() function.\n\n(2) Modify git-difftool--helper.sh script to display the counter and the diff\nqueue count values in the difftool prompt.\n\n(3) Update git.txt documentation\n\n(4) Update t4020-diff-external.sh test script\n\nSigned-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>\n---\n\nReworked patch to use run_command_v_opt_cd_env() function when invoking\nthe external diff program. Modified test script to use write_script\nhelper function.\n\n Documentation/git.txt    |  9 +++++++++\n diff.c                   | 20 +++++++++++++++++---\n diff.h                   |  2 ++\n git-difftool--helper.sh  |  3 ++-\n t/t4020-diff-external.sh | 14 ++++++++++++++\n 5 files changed, 44 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 4448ce2..10939ac 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -806,6 +806,15 @@ temporary file --- it is removed when 'GIT_EXTERNAL_DIFF' exits.\n +\n For a path that is unmerged, 'GIT_EXTERNAL_DIFF' is called with 1\n parameter, <path>.\n++\n+For each path 'GIT_EXTERNAL_DIFF' is called, two environment variables,\n+'GIT_DIFF_PATH_COUNTER' and 'GIT_DIFF_PATH_TOTAL' are set.\n+\n+'GIT_DIFF_PATH_COUNTER'::\n+\tA 1-based counter incremented by one for every path.\n+\n+'GIT_DIFF_PATH_TOTAL'::\n+\tThe total number of paths.\n \n other\n ~~~~~\ndiff --git a/diff.c b/diff.c\nindex e34bf97..a7d5a47 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2899,11 +2899,16 @@ static void run_external_diff(const char *pgm,\n \t\t\t      struct diff_filespec *one,\n \t\t\t      struct diff_filespec *two,\n \t\t\t      const char *xfrm_msg,\n-\t\t\t      int complete_rewrite)\n+\t\t\t      int complete_rewrite,\n+\t\t\t      struct diff_options *o)\n {\n \tconst char *spawn_arg[10];\n \tint retval;\n \tconst char **arg = &spawn_arg[0];\n+\tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tconst char *env[3] = { NULL };\n+\tchar env_counter[50];\n+\tchar env_total[50];\n \n \tif (one && two) {\n \t\tstruct diff_tempfile *temp_one, *temp_two;\n@@ -2928,7 +2933,14 @@ static void run_external_diff(const char *pgm,\n \t}\n \t*arg = NULL;\n \tfflush(NULL);\n-\tretval = run_command_v_opt(spawn_arg, RUN_USING_SHELL);\n+\n+\tenv[0] = env_counter;\n+\tsnprintf(env_counter, sizeof(env_counter), \"GIT_DIFF_PATH_COUNTER=%d\",\n+\t\t\t++o->diff_path_counter);\n+\tenv[1] = env_total;\n+\tsnprintf(env_total, sizeof(env_total), \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n+\n+\tretval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env);\n \tremove_tempfile();\n \tif (retval) {\n \t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n@@ -3042,7 +3054,7 @@ static void run_diff_cmd(const char *pgm,\n \n \tif (pgm) {\n \t\trun_external_diff(pgm, name, other, one, two, xfrm_msg,\n-\t\t\t\t  complete_rewrite);\n+\t\t\t\t  complete_rewrite, o);\n \t\treturn;\n \t}\n \tif (one && two)\n@@ -3317,6 +3329,8 @@ void diff_setup_done(struct diff_options *options)\n \t\toptions->output_format = DIFF_FORMAT_NO_OUTPUT;\n \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n \t}\n+\n+    options->diff_path_counter = 0;\n }\n \n static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\ndiff --git a/diff.h b/diff.h\nindex e342325..42bd34c 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -164,6 +164,8 @@ struct diff_options {\n \tdiff_prefix_fn_t output_prefix;\n \tint output_prefix_length;\n \tvoid *output_prefix_data;\n+\n+\tint diff_path_counter;\n };\n \n enum color_diff {\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex b00ed95..7ef36b9 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -40,7 +40,8 @@ launch_merge_tool () {\n \t# the user with the real $MERGED name before launching $merge_tool.\n \tif should_prompt\n \tthen\n-\t\tprintf \"\\nViewing: '%s'\\n\" \"$MERGED\"\n+\t\tprintf \"\\nViewing (%s/%s): '%s'\\n\" \"$GIT_DIFF_PATH_COUNTER\" \\\n+\t\t\t\"$GIT_DIFF_PATH_TOTAL\" \"$MERGED\"\n \t\tif use_ext_cmd\n \t\tthen\n \t\t\tprintf \"Launch '%s' [Y/n]: \" \\\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 8a30979..1855d31 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -193,6 +193,20 @@ test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '\n \tGIT_EXTERNAL_DIFF=echo git diff\n '\n \n+write_script external-diff.sh <<\\EOF\n+echo $GIT_DIFF_PATH_COUNTER of $GIT_DIFF_PATH_TOTAL >>counter.txt\n+EOF\n+\n+test_expect_success 'GIT_EXTERNAL_DIFF path counter/total' '\n+\tGIT_EXTERNAL_DIFF=./external-diff.sh git diff &&\n+\techo \"1 of 2\" >expect &&\n+\thead -n 1 counter.txt >actual &&\n+\ttest_cmp expect actual &&\n+\techo \"2 of 2\" >expect &&\n+\ttail -n 1 counter.txt >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '\n \ttouch file.ext &&\n \tgit add file.ext &&\n-- \n1.8.4.4\n"},{"id":"231677","messageId":"xmqqsiu51vz0.fsf@gitster.dls.corp.google.com","threadId":"35484","inReplyTo":"1386286726-26653-1-git-send-email-zoltan.klinger@gmail.com","subject":"Re: [PATCH v3] difftool: Change prompt to display the number of files in the diff queue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-06T22:00:03Z","receivedAt":"2013-12-06T22:00:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zoltan Klinger <zoltan.klinger@gmail.com> writes:\n\n> Reworked patch to use run_command_v_opt_cd_env() function when invoking\n> the external diff program. Modified test script to use write_script\n> helper function.\n\nThanks; will queue with a minor tweak.\n"},{"id":"232074","messageId":"20131216200221.GA23689@sigill.intra.peff.net","threadId":"35484","inReplyTo":"1386286726-26653-1-git-send-email-zoltan.klinger@gmail.com","subject":"Re: [PATCH v3] difftool: Change prompt to display the number of files in the diff queue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-16T20:02:21Z","receivedAt":"2013-12-16T20:02:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 06, 2013 at 10:38:46AM +1100, Zoltan Klinger wrote:\n\n> @@ -2928,7 +2933,14 @@ static void run_external_diff(const char *pgm,\n>  \t}\n>  \t*arg = NULL;\n>  \tfflush(NULL);\n> -\tretval = run_command_v_opt(spawn_arg, RUN_USING_SHELL);\n> +\n> +\tenv[0] = env_counter;\n> +\tsnprintf(env_counter, sizeof(env_counter), \"GIT_DIFF_PATH_COUNTER=%d\",\n> +\t\t\t++o->diff_path_counter);\n\nI don't think we have a particular rule, but our usual style is to line\nup the continued line of arguments with the open-paren of the function,\nlike:\n\n  foo(arg1, arg2,\n      arg3, arg4);\n\n> @@ -3317,6 +3329,8 @@ void diff_setup_done(struct diff_options *options)\n>  \t\toptions->output_format = DIFF_FORMAT_NO_OUTPUT;\n>  \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n>  \t}\n> +\n> +    options->diff_path_counter = 0;\n\nIt's hard to see with the email quoting, but this is a 4-space indent\nrather than the usual 1-tab (which should be 8-wide on the terminals of\nall True Believers).\n\n\nBoth are minor, but worth fixing IMHO (especially the second one). Looks\nlike it's too late for squashing, so here's a patch that can go on top\n(doing it now is still of value, though, as it's less likely to create\nconflicts since nobody is building on top yet).\n\n-- >8 --\nSubject: diff.c: fix some recent whitespace style violations\n\nThese were introduced by ee7fb0b.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn top of zk/difftool-counts.\n\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex a7d5a47..d69cc1b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2936,7 +2936,7 @@ static void run_external_diff(const char *pgm,\n \n \tenv[0] = env_counter;\n \tsnprintf(env_counter, sizeof(env_counter), \"GIT_DIFF_PATH_COUNTER=%d\",\n-\t\t\t++o->diff_path_counter);\n+\t\t ++o->diff_path_counter);\n \tenv[1] = env_total;\n \tsnprintf(env_total, sizeof(env_total), \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n@@ -3330,7 +3330,7 @@ void diff_setup_done(struct diff_options *options)\n \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n \t}\n \n-    options->diff_path_counter = 0;\n+\toptions->diff_path_counter = 0;\n }\n \n static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\n-- \n1.8.5.1.399.g900e7cd\n"},{"id":"232083","messageId":"xmqqbo0go6bx.fsf@gitster.dls.corp.google.com","threadId":"35484","inReplyTo":"20131216200221.GA23689@sigill.intra.peff.net","subject":"Re: [PATCH v3] difftool: Change prompt to display the number of files in the diff queue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-16T21:04:50Z","receivedAt":"2013-12-16T21:04:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"232136","messageId":"20131218052523.GB90546@gmail.com","threadId":"35484","inReplyTo":"1386286726-26653-1-git-send-email-zoltan.klinger@gmail.com","subject":"Re: [PATCH v3] difftool: Change prompt to display the number of files in the diff queue","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-12-18T05:25:25Z","receivedAt":"2013-12-18T05:25:25Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Thanks for the patch, and sorry for the late response.\nI have just a couple of notes below...\n\nOn Fri, Dec 06, 2013 at 10:38:46AM +1100, Zoltan Klinger wrote:\n> diff --git a/diff.c b/diff.c\n> index e34bf97..a7d5a47 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2899,11 +2899,16 @@ static void run_external_diff(const char *pgm,\n>  \t\t\t      struct diff_filespec *one,\n>  \t\t\t      struct diff_filespec *two,\n>  \t\t\t      const char *xfrm_msg,\n> -\t\t\t      int complete_rewrite)\n> +\t\t\t      int complete_rewrite,\n> +\t\t\t      struct diff_options *o)\n\nVery minor nit -- \"o\" is a very terse variable name.\nMaybe \"opts\"?\n\n>  {\n>  \tconst char *spawn_arg[10];\n>  \tint retval;\n>  \tconst char **arg = &spawn_arg[0];\n> +\tstruct diff_queue_struct *q = &diff_queued_diff;\n> +\tconst char *env[3] = { NULL };\n> +\tchar env_counter[50];\n> +\tchar env_total[50];\n\nHard-coded 50; what's the length of the maximum signed int?\n\n\n> diff --git a/diff.h b/diff.h\n> index e342325..42bd34c 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -164,6 +164,8 @@ struct diff_options {\n>  \tdiff_prefix_fn_t output_prefix;\n>  \tint output_prefix_length;\n>  \tvoid *output_prefix_data;\n> +\n> +\tint diff_path_counter;\n>  };\n\nSince these are already \"diff_options\" it seems redundant to call\nthe struct entry the \"diff_path_counter\" when \"path_count\"\nshould be specific enough.  Would it make sense to rename it?\n\nThese are tiny nitpicky style notes; it looks good otherwise.\n\nThanks,\n-- \nDavid\n"},{"id":"232139","messageId":"xmqqmwjyk810.fsf@gitster.dls.corp.google.com","threadId":"35484","inReplyTo":"20131218052523.GB90546@gmail.com","subject":"Re: [PATCH v3] difftool: Change prompt to display the number of files in the diff queue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T06:06:19Z","receivedAt":"2013-12-18T06:06:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Thanks for the patch, and sorry for the late response.\n> I have just a couple of notes below...\n>\n> On Fri, Dec 06, 2013 at 10:38:46AM +1100, Zoltan Klinger wrote:\n>> diff --git a/diff.c b/diff.c\n>> index e34bf97..a7d5a47 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -2899,11 +2899,16 @@ static void run_external_diff(const char *pgm,\n>>  \t\t\t      struct diff_filespec *one,\n>>  \t\t\t      struct diff_filespec *two,\n>>  \t\t\t      const char *xfrm_msg,\n>> -\t\t\t      int complete_rewrite)\n>> +\t\t\t      int complete_rewrite,\n>> +\t\t\t      struct diff_options *o)\n>\n> Very minor nit -- \"o\" is a very terse variable name.\n> Maybe \"opts\"?\n\nThe diff-options parameter passed around in the callchain has always\nbeen \"o\" throughout this file from the beginning of time, though ;-).\n\n>\n>>  {\n>>  \tconst char *spawn_arg[10];\n>>  \tint retval;\n>>  \tconst char **arg = &spawn_arg[0];\n>> +\tstruct diff_queue_struct *q = &diff_queued_diff;\n>> +\tconst char *env[3] = { NULL };\n>> +\tchar env_counter[50];\n>> +\tchar env_total[50];\n>\n> Hard-coded 50; what's the length of the maximum signed int?\n\n;-) A bit of slack is fine, but 50 might be excessive (more than\ntwice as big as necessary).\n\n>> diff --git a/diff.h b/diff.h\n>> index e342325..42bd34c 100644\n>> --- a/diff.h\n>> +++ b/diff.h\n>> @@ -164,6 +164,8 @@ struct diff_options {\n>>  \tdiff_prefix_fn_t output_prefix;\n>>  \tint output_prefix_length;\n>>  \tvoid *output_prefix_data;\n>> +\n>> +\tint diff_path_counter;\n>>  };\n>\n> Since these are already \"diff_options\" it seems redundant to call\n> the struct entry the \"diff_path_counter\" when \"path_count\"\n> should be specific enough.  Would it make sense to rename it?\n\nYeah, makes sense.  diff_options->diff_path_counter++ sounds awful,\nwhile options->path_count++ looks quite tame and reasonable.\n\n> These are tiny nitpicky style notes; it looks good otherwise.\n\nThanks.\n"}]}