Skip to content

Commit 0cc4ec1

Browse files
gitstergitster
committed
Merge branch 'da/difftool'
Code clean-up in "git difftool". * da/difftool: difftool: add a missing space to the run_dir_diff() comments difftool: remove an unnecessary call to strbuf_release() difftool: refactor dir-diff to write files using helper functions difftool: create a tmpdir path without repeated slashes
2 parents 404c4a5 + 28c10ec commit 0cc4ec1

File tree

2 files changed

+60
-51
lines changed

2 files changed

+60
-51
lines changed

‎builtin/difftool.c‎

Lines changed: 53 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -252,16 +252,6 @@ static void changed_files(struct hashmap *result, const char *index_path,
252252
strbuf_release(&buf);
253253
}
254254

255-
static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
256-
{
257-
struct strbuf buf = STRBUF_INIT;
258-
strbuf_addstr(&buf, tmpdir);
259-
remove_dir_recursively(&buf, 0);
260-
if (exit_code)
261-
warning(_("failed: %d"), exit_code);
262-
exit(exit_code);
263-
}
264-
265255
static int ensure_leading_directories(char *path)
266256
{
267257
switch (safe_create_leading_directories(path)) {
@@ -330,19 +320,44 @@ static int checkout_path(unsigned mode, struct object_id *oid,
330320
return ret;
331321
}
332322

323+
static void write_file_in_directory(struct strbuf *dir, size_t dir_len,
324+
const char *path, const char *content)
325+
{
326+
add_path(dir, dir_len, path);
327+
ensure_leading_directories(dir->buf);
328+
unlink(dir->buf);
329+
write_file(dir->buf, "%s", content);
330+
}
331+
332+
/* Write the file contents for the left and right sides of the difftool
333+
* dir-diff representation for submodules and symlinks. Symlinks and submodules
334+
* are written as regular text files so that external diff tools can diff them
335+
* as text files, resulting in behavior that is analogous to to what "git diff"
336+
* displays for symlink and submodule diffs.
337+
*/
338+
static void write_standin_files(struct pair_entry *entry,
339+
struct strbuf *ldir, size_t ldir_len,
340+
struct strbuf *rdir, size_t rdir_len)
341+
{
342+
if (*entry->left)
343+
write_file_in_directory(ldir, ldir_len, entry->path, entry->left);
344+
if (*entry->right)
345+
write_file_in_directory(rdir, rdir_len, entry->path, entry->right);
346+
}
347+
333348
static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
334349
struct child_process *child)
335350
{
336-
char tmpdir[PATH_MAX];
337351
struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
338352
struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
339353
struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
340354
struct strbuf wtdir = STRBUF_INIT;
341-
char *lbase_dir, *rbase_dir;
355+
struct strbuf tmpdir = STRBUF_INIT;
356+
char *lbase_dir = NULL, *rbase_dir = NULL;
342357
size_t ldir_len, rdir_len, wtdir_len;
343358
const char *workdir, *tmp;
344359
int ret = 0, i;
345-
FILE *fp;
360+
FILE *fp = NULL;
346361
struct hashmap working_tree_dups = HASHMAP_INIT(working_tree_entry_cmp,
347362
NULL);
348363
struct hashmap submodules = HASHMAP_INIT(pair_cmp, NULL);
@@ -351,7 +366,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
351366
struct pair_entry *entry;
352367
struct index_state wtindex;
353368
struct checkout lstate, rstate;
354-
int rc, flags = RUN_GIT_CMD, err = 0;
369+
int flags = RUN_GIT_CMD, err = 0;
355370
const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
356371
struct hashmap wt_modified, tmp_modified;
357372
int indices_loaded = 0;
@@ -360,11 +375,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
360375

361376
/* Setup temp directories */
362377
tmp = getenv("TMPDIR");
363-
xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
364-
if (!mkdtemp(tmpdir))
365-
return error("could not create '%s'", tmpdir);
366-
strbuf_addf(&ldir, "%s/left/", tmpdir);
367-
strbuf_addf(&rdir, "%s/right/", tmpdir);
378+
strbuf_add_absolute_path(&tmpdir, tmp ? tmp : "/tmp");
379+
strbuf_trim_trailing_dir_sep(&tmpdir);
380+
strbuf_addstr(&tmpdir, "/git-difftool.XXXXXX");
381+
if (!mkdtemp(tmpdir.buf)) {
382+
ret = error("could not create '%s'", tmpdir.buf);
383+
goto finish;
384+
}
385+
strbuf_addf(&ldir, "%s/left/", tmpdir.buf);
386+
strbuf_addf(&rdir, "%s/right/", tmpdir.buf);
368387
strbuf_addstr(&wtdir, workdir);
369388
if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
370389
strbuf_addch(&wtdir, '/');
@@ -535,40 +554,19 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
535554
*/
536555
hashmap_for_each_entry(&submodules, &iter, entry,
537556
entry /* member name */) {
538-
if (*entry->left) {
539-
add_path(&ldir, ldir_len, entry->path);
540-
ensure_leading_directories(ldir.buf);
541-
write_file(ldir.buf, "%s", entry->left);
542-
}
543-
if (*entry->right) {
544-
add_path(&rdir, rdir_len, entry->path);
545-
ensure_leading_directories(rdir.buf);
546-
write_file(rdir.buf, "%s", entry->right);
547-
}
557+
write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len);
548558
}
549559

550560
/*
551-
* Symbolic links require special treatment.The standard "git diff"
561+
* Symbolic links require special treatment. The standard "git diff"
552562
* shows only the link itself, not the contents of the link target.
553563
* This loop replicates that behavior.
554564
*/
555565
hashmap_for_each_entry(&symlinks2, &iter, entry,
556566
entry /* member name */) {
557-
if (*entry->left) {
558-
add_path(&ldir, ldir_len, entry->path);
559-
ensure_leading_directories(ldir.buf);
560-
unlink(ldir.buf);
561-
write_file(ldir.buf, "%s", entry->left);
562-
}
563-
if (*entry->right) {
564-
add_path(&rdir, rdir_len, entry->path);
565-
ensure_leading_directories(rdir.buf);
566-
unlink(rdir.buf);
567-
write_file(rdir.buf, "%s", entry->right);
568-
}
569-
}
570567

571-
strbuf_release(&buf);
568+
write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len);
569+
}
572570

573571
strbuf_setlen(&ldir, ldir_len);
574572
helper_argv[1] = ldir.buf;
@@ -580,7 +578,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
580578
flags = 0;
581579
} else
582580
setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
583-
rc = run_command_v_opt(helper_argv, flags);
581+
ret = run_command_v_opt(helper_argv, flags);
584582

585583
/* TODO: audit for interaction with sparse-index. */
586584
ensure_full_index(&wtindex);
@@ -614,7 +612,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
614612
if (!indices_loaded) {
615613
struct lock_file lock = LOCK_INIT;
616614
strbuf_reset(&buf);
617-
strbuf_addf(&buf, "%s/wtindex", tmpdir);
615+
strbuf_addf(&buf, "%s/wtindex", tmpdir.buf);
618616
if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
619617
write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {
620618
ret = error("could not write %s", buf.buf);
@@ -644,11 +642,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
644642
}
645643

646644
if (err) {
647-
warning(_("temporary files exist in '%s'."), tmpdir);
645+
warning(_("temporary files exist in '%s'."), tmpdir.buf);
648646
warning(_("you may want to cleanup or recover these."));
649-
exit(1);
650-
} else
651-
exit_cleanup(tmpdir, rc);
647+
ret = 1;
648+
} else {
649+
remove_dir_recursively(&tmpdir, 0);
650+
if (ret)
651+
warning(_("failed: %d"), ret);
652+
}
652653

653654
finish:
654655
if (fp)
@@ -660,8 +661,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
660661
strbuf_release(&rdir);
661662
strbuf_release(&wtdir);
662663
strbuf_release(&buf);
664+
strbuf_release(&tmpdir);
663665

664-
return ret;
666+
return (ret < 0) ? 1 : ret;
665667
}
666668

667669
static int run_file_diff(int prompt, const char *prefix,

‎t/t7800-difftool.sh‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,13 @@ run_dir_diff_test 'difftool --dir-diff' '
453453
grep "^file$" output
454454
'
455455

456+
run_dir_diff_test 'difftool --dir-diff avoids repeated slashes in TMPDIR' '
457+
TMPDIR="${TMPDIR:-/tmp}////" \
458+
git difftool --dir-diff $symlinks --extcmd echo branch >output &&
459+
grep -v // output >actual &&
460+
test_line_count = 1 actual
461+
'
462+
456463
run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
457464
git difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&
458465
grep "^sub$" output &&

0 commit comments

Comments
 (0)