{"thread":{"id":"35416","subject":"[PATCH] difftool: Change prompt to display the number of files in the diff queue","startedAt":"2013-11-28T00:49:54Z","lastAt":"2013-12-02T21:08:26Z","messageCount":2,"participants":["Zoltan Klinger","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"231220","messageId":"1385599794-6002-1-git-send-email-zoltan.klinger@gmail.com","threadId":"35416","inReplyTo":null,"subject":"[PATCH] difftool: Change prompt to display the number of files in the diff queue","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2013-11-28T00:49:54Z","receivedAt":"2013-11-28T00:49:54Z","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\nmodified file to be viewed in an external diff program. At that point it\ncould be useful to display a counter and the total number of files in\nthe 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\n(1) Modify run_external_diff() function in diff.c to pass a counter and\nthe total number of files in the diff queue to the external program.\n\n(2) Modify git-difftool--helper.sh script to display the counter and the\ndiff queue 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 Documentation/git.txt    |  6 +++++-\n diff.c                   | 14 +++++++++++++-\n git-difftool--helper.sh  |  8 +++++---\n t/t4020-diff-external.sh | 27 +++++++++++++++++++++------\n 4 files changed, 44 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex b73a24a..d8241bb 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -785,9 +785,10 @@ Git Diffs\n \tWhen the environment variable 'GIT_EXTERNAL_DIFF' is set, the\n \tprogram named by it is called, instead of the diff invocation\n \tdescribed above.  For a path that is added, removed, or modified,\n-        'GIT_EXTERNAL_DIFF' is called with 7 parameters:\n+\t'GIT_EXTERNAL_DIFF' is called with 9 parameters:\n \n \tpath old-file old-hex old-mode new-file new-hex new-mode\n+\tcounter total\n +\n where:\n \n@@ -795,6 +796,9 @@ where:\n                          contents of <old|new>,\n \t<old|new>-hex:: are the 40-hexdigit SHA-1 hashes,\n \t<old|new>-mode:: are the octal representation of the file modes.\n+\tcounter:: is a numeric value incremented by one for every modified\n+\t\t\t\tfile\n+\ttotal:: is the total number of modified files\n +\n The file parameters can point at the user's working file\n (e.g. `new-file` in \"git-diff-files\"), `/dev/null` (e.g. `old-file`\ndiff --git a/diff.c b/diff.c\nindex e34bf97..938f00a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -37,6 +37,7 @@ static int diff_stat_graph_width;\n static int diff_dirstat_permille_default = 30;\n static struct diff_options default_diff_options;\n static long diff_algorithm;\n+static int diff_display_counter = 1;\n \n static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_RESET,\n@@ -2901,9 +2902,16 @@ static void run_external_diff(const char *pgm,\n \t\t\t      const char *xfrm_msg,\n \t\t\t      int complete_rewrite)\n {\n-\tconst char *spawn_arg[10];\n+\tconst char *spawn_arg[12];\n \tint retval;\n \tconst char **arg = &spawn_arg[0];\n+\tstruct diff_queue_struct *q = &diff_queued_diff;\n+\n+\tstruct strbuf counterstr = STRBUF_INIT;\n+\tstruct strbuf totalstr = STRBUF_INIT;\n+\tstrbuf_addf(&counterstr, \"%d\", diff_display_counter++);\n+\tstrbuf_addf(&totalstr, \"%d\", q->nr);\n+\n \n \tif (one && two) {\n \t\tstruct diff_tempfile *temp_one, *temp_two;\n@@ -2918,6 +2926,8 @@ static void run_external_diff(const char *pgm,\n \t\t*arg++ = temp_two->name;\n \t\t*arg++ = temp_two->hex;\n \t\t*arg++ = temp_two->mode;\n+\t\t*arg++ = counterstr.buf;\n+\t\t*arg++ = totalstr.buf;\n \t\tif (other) {\n \t\t\t*arg++ = other;\n \t\t\t*arg++ = xfrm_msg;\n@@ -2930,6 +2940,8 @@ static void run_external_diff(const char *pgm,\n \tfflush(NULL);\n \tretval = run_command_v_opt(spawn_arg, RUN_USING_SHELL);\n \tremove_tempfile();\n+\tstrbuf_release(&counterstr);\n+\tstrbuf_release(&totalstr);\n \tif (retval) {\n \t\tfprintf(stderr, \"external diff died, stopping at %s.\\n\", name);\n \t\texit(1);\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex b00ed95..4444c26 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -35,12 +35,14 @@ launch_merge_tool () {\n \tLOCAL=\"$2\"\n \tREMOTE=\"$3\"\n \tBASE=\"$1\"\n+\tCOUNTER=\"$4\"\n+\tTOTAL=\"$5\"\n \n \t# $LOCAL and $REMOTE are temporary files so prompt\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\" \"$COUNTER\" \"$TOTAL\" \"$MERGED\"\n \t\tif use_ext_cmd\n \t\tthen\n \t\t\tprintf \"Launch '%s' [Y/n]: \" \\\n@@ -82,7 +84,7 @@ else\n \t# Launch the merge tool on each path provided by 'git diff'\n \twhile test $# -gt 6\n \tdo\n-\t\tlaunch_merge_tool \"$1\" \"$2\" \"$5\"\n-\t\tshift 7\n+\t\tlaunch_merge_tool \"$1\" \"$2\" \"$5\" \"$8\" \"$9\"\n+\t\tshift 9\n \tdone\n fi\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 8a30979..a8cf9c6 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -8,7 +8,9 @@ test_expect_success setup '\n \n \ttest_tick &&\n \techo initial >file &&\n-\tgit add file &&\n+\techo foo >file2 &&\n+\techo bar >file3 &&\n+\tgit add file file2 file3 &&\n \tgit commit -m initial &&\n \n \ttest_tick &&\n@@ -18,16 +20,20 @@ test_expect_success setup '\n \n \ttest_tick &&\n \techo third >file\n+\techo quux >file2\n+\techo quux >file3\n '\n \n test_expect_success 'GIT_EXTERNAL_DIFF environment' '\n \n \tGIT_EXTERNAL_DIFF=echo git diff | {\n-\t\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\t\tread path oldfile oldhex oldmode newfile newhex newmode counter total &&\n \t\ttest \"z$path\" = zfile &&\n \t\ttest \"z$oldmode\" = z100644 &&\n \t\ttest \"z$newhex\" = \"z$_z40\" &&\n \t\ttest \"z$newmode\" = z100644 &&\n+\t\ttest \"z$counter\" = z1 &&\n+\t\ttest \"z$total\" = z3 &&\n \t\toh=$(git rev-parse --verify HEAD:file) &&\n \t\ttest \"z$oh\" = \"z$oldhex\"\n \t}\n@@ -49,14 +55,17 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '\n '\n \n test_expect_success SYMLINKS 'typechange diff' '\n+\tgit checkout -- file2 file3 &&\n \trm -f file &&\n \tln -s elif file &&\n \tGIT_EXTERNAL_DIFF=echo git diff  | {\n-\t\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\t\tread path oldfile oldhex oldmode newfile newhex newmode counter total &&\n \t\ttest \"z$path\" = zfile &&\n \t\ttest \"z$oldmode\" = z100644 &&\n \t\ttest \"z$newhex\" = \"z$_z40\" &&\n \t\ttest \"z$newmode\" = z120000 &&\n+\t\ttest \"z$counter\" = z1 &&\n+\t\ttest \"z$total\" = z1 &&\n \t\toh=$(git rev-parse --verify HEAD:file) &&\n \t\ttest \"z$oh\" = \"z$oldhex\"\n \t} &&\n@@ -70,11 +79,13 @@ test_expect_success 'diff.external' '\n \techo third >file &&\n \ttest_config diff.external echo &&\n \tgit diff | {\n-\t\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\t\tread path oldfile oldhex oldmode newfile newhex newmode counter total &&\n \t\ttest \"z$path\" = zfile &&\n \t\ttest \"z$oldmode\" = z100644 &&\n \t\ttest \"z$newhex\" = \"z$_z40\" &&\n \t\ttest \"z$newmode\" = z100644 &&\n+\t\ttest \"z$counter\" = z1 &&\n+\t\ttest \"z$total\" = z1 &&\n \t\toh=$(git rev-parse --verify HEAD:file) &&\n \t\ttest \"z$oh\" = \"z$oldhex\"\n \t}\n@@ -101,11 +112,13 @@ test_expect_success 'diff attribute' '\n \techo >.gitattributes \"file diff=parrot\" &&\n \n \tgit diff | {\n-\t\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\t\tread path oldfile oldhex oldmode newfile newhex newmode counter total &&\n \t\ttest \"z$path\" = zfile &&\n \t\ttest \"z$oldmode\" = z100644 &&\n \t\ttest \"z$newhex\" = \"z$_z40\" &&\n \t\ttest \"z$newmode\" = z100644 &&\n+\t\ttest \"z$counter\" = z1 &&\n+\t\ttest \"z$total\" = z1 &&\n \t\toh=$(git rev-parse --verify HEAD:file) &&\n \t\ttest \"z$oh\" = \"z$oldhex\"\n \t}\n@@ -134,11 +147,13 @@ test_expect_success 'diff attribute' '\n \techo >.gitattributes \"file diff=color\" &&\n \n \tgit diff | {\n-\t\tread path oldfile oldhex oldmode newfile newhex newmode &&\n+\t\tread path oldfile oldhex oldmode newfile newhex newmode counter total &&\n \t\ttest \"z$path\" = zfile &&\n \t\ttest \"z$oldmode\" = z100644 &&\n \t\ttest \"z$newhex\" = \"z$_z40\" &&\n \t\ttest \"z$newmode\" = z100644 &&\n+\t\ttest \"z$counter\" = z1 &&\n+\t\ttest \"z$total\" = z1 &&\n \t\toh=$(git rev-parse --verify HEAD:file) &&\n \t\ttest \"z$oh\" = \"z$oldhex\"\n \t}\n-- \n1.8.4.4\n"},{"id":"231390","messageId":"xmqqfvqbq7ud.fsf@gitster.dls.corp.google.com","threadId":"35416","inReplyTo":"1385599794-6002-1-git-send-email-zoltan.klinger@gmail.com","subject":"Re: [PATCH] difftool: Change prompt to display the number of files in the diff queue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-02T21:08:26Z","receivedAt":"2013-12-02T21:08:26Z","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> When --prompt option is set, git-difftool displays a prompt for each\n> modified file to be viewed in an external diff program. At that point it\n> could be useful to display a counter and the total number of files in\n> the diff queue.\n>\n> Below is the current difftool prompt for the first of 5 modified files:\n> Viewing: 'diff.c'\n> Launch 'vimdiff' [Y/n]:\n>\n> Consider the modified prompt:\n> Viewing (1/5): 'diff.c'\n> Launch 'vimdiff' [Y/n]:\n>\n> (1) Modify run_external_diff() function in diff.c to pass a counter and\n> the total number of files in the diff queue to the external program.\n>\n> (2) Modify git-difftool--helper.sh script to display the counter and the\n> diff queue count values in the difftool prompt.\n>\n> (3) Update git.txt documentation\n>\n> (4) Update t4020-diff-external.sh test script\n>\n> Signed-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>\n> ---\n>  Documentation/git.txt    |  6 +++++-\n>  diff.c                   | 14 +++++++++++++-\n>  git-difftool--helper.sh  |  8 +++++---\n>  t/t4020-diff-external.sh | 27 +++++++++++++++++++++------\n>  4 files changed, 44 insertions(+), 11 deletions(-)\n>\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index b73a24a..d8241bb 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -785,9 +785,10 @@ Git Diffs\n>  \tWhen the environment variable 'GIT_EXTERNAL_DIFF' is set, the\n>  \tprogram named by it is called, instead of the diff invocation\n>  \tdescribed above.  For a path that is added, removed, or modified,\n> -        'GIT_EXTERNAL_DIFF' is called with 7 parameters:\n> +\t'GIT_EXTERNAL_DIFF' is called with 9 parameters:\n>  \n>  \tpath old-file old-hex old-mode new-file new-hex new-mode\n> +\tcounter total\n\nAs the \"git difftool\" is not the only thing that reads using the\nGIT_EXTERNAL_DIFF mechanism (it is for general consumption by end\nuser scripts), I wonder how/if this change breaks existing scripts.\nI do recall writing an EXTERNAL_DIFF script myself that began by\nswitching on $# (i.e. the number of arguments) to check the state of\nthe given path, like this:\n\n\tcase $# in\n        1)\n        \t... handle unmerged path ...\n                ;;\n\t7)\n        \t... handle comparison ...\n\t\t;;\n\t*)\n        \tdie \"Unexpected number of arguments to $0: $#\"\n                ;;\n\tesac\n\nwhich will be broken by this change. Updating such scripts is\ntrivial but that does not change the fact that this change is\nforcing an unnecessary work on our users to adjust their scripts\nthat have been working perfectly fine.  So I think this, if we were\nto apply, may need a compatibility warning in large flashing red\nletters in the release notes.\n\n>  +\n>  where:\n>  \n> @@ -795,6 +796,9 @@ where:\n>                           contents of <old|new>,\n>  \t<old|new>-hex:: are the 40-hexdigit SHA-1 hashes,\n>  \t<old|new>-mode:: are the octal representation of the file modes.\n> +\tcounter:: is a numeric value incremented by one for every modified\n> +\t\t\t\tfile\n> +\ttotal:: is the total number of modified files\n>  +\n>  The file parameters can point at the user's working file\n>  (e.g. `new-file` in \"git-diff-files\"), `/dev/null` (e.g. `old-file`\n> diff --git a/diff.c b/diff.c\n> index e34bf97..938f00a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -37,6 +37,7 @@ static int diff_stat_graph_width;\n>  static int diff_dirstat_permille_default = 30;\n>  static struct diff_options default_diff_options;\n>  static long diff_algorithm;\n> +static int diff_display_counter = 1;\n\nThere should be a place somewhere, e.g. diff_setup_done(), to reset\nthis counter to 0 (and the site that uses the variable should\npre-increment it instead of relying the initial value being 1), so\nthat a single program could later run the diff machinery more than\nonce for different set of files.  This counter may actually belong\nto diff_options, just like existing \"found_changes\" and\n\"found_follow\" fields are there to keep track of state of the diff\nmachinery per invocation.\n\nHaving said all that, the fact that the current arrangement since we\nintroduced GIT_EXTERNAL_DIFF mechanism does not tell how many paths\nthere are in the output is indeed bad.  If a script that uses\nGIT_EXTERNAL_DIFF wants to first collect all the paths and the\nparameters and then show everything in a single UI, such a script\nmay want to (1) start collecting the paths and args to a persistent\nplace (e.g. starting a GUI diff daemon for the first path it gets,\nor starting a new temporary file), (2) keep collecting the paths and\nargs, and then (3) after collecting all, present diff for all paths\nit obtained, but it is impossible because there is no cue when a\nseries of external diff calls starts or ends.\n\nAnd this \"counter/total\" mechanism could be one possible solution to\nit (another possibility is to make an extra dummy call to signal the\nend, perhaps with no parameters---the one that is collecting can\nthen know how many paths there are and which one is the Nth path).\n"}]}