From ae049c955c8858899467f6c5c0259c48a5294385 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 19 Apr 2014 15:17:25 -0400 Subject: [PATCH 1/5] run_external_diff: use an argv_array for the environment We currently use static buffers and a static array for formatting the environment passed to the external diff. There's nothing wrong in the code, but it is much easier to verify that it is correct if we use a dynamic argv_array. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index 76ae379032..c7b433ffcd 100644 --- a/diff.c +++ b/diff.c @@ -2908,11 +2908,9 @@ static void run_external_diff(const char *pgm, struct diff_options *o) { struct argv_array argv = ARGV_ARRAY_INIT; + struct argv_array env = ARGV_ARRAY_INIT; int retval; 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; @@ -2937,15 +2935,13 @@ static void run_external_diff(const char *pgm, } fflush(NULL); - 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); + argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); + argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr); - retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env); + retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv); remove_tempfile(); argv_array_clear(&argv); + argv_array_clear(&env); if (retval) { fprintf(stderr, "external diff died, stopping at %s.\n", name); exit(1); From 89294d143d42db2540ec587d0bce20c6c7718051 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 19 Apr 2014 15:19:19 -0400 Subject: [PATCH 2/5] run_external_diff: clean up error handling When the external diff reports an error, we try to clean up and die. However, we can make this process a bit simpler: 1. We do not need to bother freeing memory, since we are about to exit. Nor do we need to clean up our tempfiles, since the atexit() handler will do it for us. So we can die as soon as we see the error. 3. We can just call die() rather than fprintf/exit. This does technically change our exit code, but the exit code of "1" is not meaningful here. In fact, it is probably wrong, since "1" from diff usually means "completed successfully, but there were differences". And while we're there, we can mark the error message for translation, and drop the full stop at the end to make it more like our other messages. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 9 +++------ t/t7800-difftool.sh | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/diff.c b/diff.c index c7b433ffcd..174f146d0d 100644 --- a/diff.c +++ b/diff.c @@ -2909,7 +2909,6 @@ static void run_external_diff(const char *pgm, { struct argv_array argv = ARGV_ARRAY_INIT; struct argv_array env = ARGV_ARRAY_INIT; - int retval; struct diff_queue_struct *q = &diff_queued_diff; if (one && two) { @@ -2938,14 +2937,12 @@ static void run_external_diff(const char *pgm, argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr); - retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv); + if (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv)) + die(_("external diff died, stopping at %s"), name); + remove_tempfile(); argv_array_clear(&argv); argv_array_clear(&env); - if (retval) { - fprintf(stderr, "external diff died, stopping at %s.\n", name); - exit(1); - } } static int similarity_index(struct diff_filepair *p) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 5a193c500d..dc30a514bf 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -58,7 +58,7 @@ test_expect_success PERL 'custom tool commands override built-ins' ' test_expect_success PERL 'difftool ignores bad --tool values' ' : >expect && - test_expect_code 1 \ + test_must_fail \ git difftool --no-prompt --tool=bad-tool branch >actual && test_cmp expect actual ' From 5b88caa417ab221c8f8576332fba8d41dd3986d7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 19 Apr 2014 15:19:30 -0400 Subject: [PATCH 3/5] run_external_diff: drop fflush(NULL) This fflush was added in d5535ec (Use run_command() to spawn external diff programs instead of fork/exec., 2007-10-19), because flushing buffers before forking is a good habit. But later, 7d0b18a (Add output flushing before fork(), 2008-08-04) added it to the generic run-command interface, meaning that our flush here is redundant. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 1 - 1 file changed, 1 deletion(-) diff --git a/diff.c b/diff.c index 174f146d0d..4bad556812 100644 --- a/diff.c +++ b/diff.c @@ -2932,7 +2932,6 @@ static void run_external_diff(const char *pgm, argv_array_push(&argv, pgm); argv_array_push(&argv, name); } - fflush(NULL); argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr); From 0d4217d92e3043e23a8960519a51cc7a36ed8914 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 19 Apr 2014 15:20:46 -0400 Subject: [PATCH 4/5] run_external_diff: hoist common bits out of conditional Whether we have diff_filespecs to give to the diff command or not, we always are going to run the program and pass it the pathname. Let's pull that duplicated part out of the conditional to make it more obvious. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index 4bad556812..a360ab5063 100644 --- a/diff.c +++ b/diff.c @@ -2911,13 +2911,14 @@ static void run_external_diff(const char *pgm, struct argv_array env = ARGV_ARRAY_INIT; struct diff_queue_struct *q = &diff_queued_diff; + argv_array_push(&argv, pgm); + argv_array_push(&argv, name); + if (one && two) { struct diff_tempfile *temp_one, *temp_two; const char *othername = (other ? other : name); temp_one = prepare_temp_file(name, one); temp_two = prepare_temp_file(othername, two); - argv_array_push(&argv, pgm); - argv_array_push(&argv, name); argv_array_push(&argv, temp_one->name); argv_array_push(&argv, temp_one->hex); argv_array_push(&argv, temp_one->mode); @@ -2928,9 +2929,6 @@ static void run_external_diff(const char *pgm, argv_array_push(&argv, other); argv_array_push(&argv, xfrm_msg); } - } else { - argv_array_push(&argv, pgm); - argv_array_push(&argv, name); } argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); From f3efe78782b36d68dc71a4f48a7bd3381c6b5669 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 19 Apr 2014 15:22:25 -0400 Subject: [PATCH 5/5] run_external_diff: refactor cmdline setup logic The current logic makes it hard to see what gets put onto the command line in which cases. Pulling out a helper function lets us see that we have two sets of file data, and the second set either uses the original name, or the "other" renamed/copy name. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/diff.c b/diff.c index a360ab5063..680f52db4b 100644 --- a/diff.c +++ b/diff.c @@ -2892,6 +2892,16 @@ static struct diff_tempfile *prepare_temp_file(const char *name, return temp; } +static void add_external_diff_name(struct argv_array *argv, + const char *name, + struct diff_filespec *df) +{ + struct diff_tempfile *temp = prepare_temp_file(name, df); + argv_array_push(argv, temp->name); + argv_array_push(argv, temp->hex); + argv_array_push(argv, temp->mode); +} + /* An external diff command takes: * * diff-cmd name infile1 infile1-sha1 infile1-mode \ @@ -2915,17 +2925,11 @@ static void run_external_diff(const char *pgm, argv_array_push(&argv, name); if (one && two) { - struct diff_tempfile *temp_one, *temp_two; - const char *othername = (other ? other : name); - temp_one = prepare_temp_file(name, one); - temp_two = prepare_temp_file(othername, two); - argv_array_push(&argv, temp_one->name); - argv_array_push(&argv, temp_one->hex); - argv_array_push(&argv, temp_one->mode); - argv_array_push(&argv, temp_two->name); - argv_array_push(&argv, temp_two->hex); - argv_array_push(&argv, temp_two->mode); - if (other) { + add_external_diff_name(&argv, name, one); + if (!other) + add_external_diff_name(&argv, name, two); + else { + add_external_diff_name(&argv, other, two); argv_array_push(&argv, other); argv_array_push(&argv, xfrm_msg); }