diff mbox series

[v5,2/3] difftool: create a tmpdir path without repeated slashes

Message ID 20210923041252.52596-3-davvid@gmail.com (mailing list archive)
State New, archived
Headers show
Series difftool dir-diff symlink bug fix and cleanup patches | expand

Commit Message

David Aguilar Sept. 23, 2021, 4:12 a.m. UTC 
The paths generated by difftool are passed to user-facing diff tools.
Using paths with repeated slashes in them is a cosmetic blemish that
is exposed to users and can be avoided.

Use a strbuf to create the buffer used for the dir-diff tmpdir.
Strip trailing slashes from the value read from TMPDIR to avoid
repeated slashes in the generated paths.

Adjust the error handling to avoid leaking strbufs.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 builtin/difftool.c  | 48 ++++++++++++++++++++++-----------------------
 t/t7800-difftool.sh |  7 +++++++
 2 files changed, 30 insertions(+), 25 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Sept. 24, 2021, 10:35 a.m. UTC | #1 
On Wed, Sep 22 2021, David Aguilar wrote:

> @@ -580,7 +574,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  		flags = 0;
>  	} else
>  		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
> -	rc = run_command_v_opt(helper_argv, flags);
> +	ret = run_command_v_opt(helper_argv, flags);
>  
>  	/* TODO: audit for interaction with sparse-index. */
>  	ensure_full_index(&wtindex);

Just on this part: There was already a logic error in the pre-image
where we'd return error() return values up to cmd_main() et al. This
means that (on POSIX) we return 255, not 1, per the C standard doing
this is "implementation defined".

I think you want the "rc" variable back, and either in another commit or
in some other cleanup this on top or ahead of this patch:

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a867c49067c..6605d17e6bc 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -345,6 +345,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
 	struct hashmap wt_modified, tmp_modified;
 	int indices_loaded = 0;
+	int rc;
 
 	workdir = get_git_work_tree();
 
@@ -574,7 +575,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		flags = 0;
 	} else
 		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-	ret = run_command_v_opt(helper_argv, flags);
+	rc = run_command_v_opt(helper_argv, flags);
 
 	/* TODO: audit for interaction with sparse-index. */
 	ensure_full_index(&wtindex);
@@ -660,7 +661,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	strbuf_release(&tmpdir);
 	UNLEAK(working_tree_dups);
 
-	return ret;
+	return ret < 0 ? 1 : rc;
 }
 
 static int run_file_diff(int prompt, const char *prefix,
David Aguilar Sept. 30, 2021, 5:05 p.m. UTC | #2 
On Fri, Sep 24, 2021 at 3:43 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Wed, Sep 22 2021, David Aguilar wrote:
>
> > @@ -580,7 +574,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
> >               flags = 0;
> >       } else
> >               setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
> > -     rc = run_command_v_opt(helper_argv, flags);
> > +     ret = run_command_v_opt(helper_argv, flags);
> >
> >       /* TODO: audit for interaction with sparse-index. */
> >       ensure_full_index(&wtindex);
>
> Just on this part: There was already a logic error in the pre-image
> where we'd return error() return values up to cmd_main() et al. This
> means that (on POSIX) we return 255, not 1, per the C standard doing
> this is "implementation defined".
>
> I think you want the "rc" variable back, and either in another commit or
> in some other cleanup this on top or ahead of this patch:

Good catch. I submitted an incremental follow-up patch that addresses
this issue.


> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index a867c49067c..6605d17e6bc 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -345,6 +345,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>         const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
>         struct hashmap wt_modified, tmp_modified;
>         int indices_loaded = 0;
> +       int rc;
>
>         workdir = get_git_work_tree();
>
> @@ -574,7 +575,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>                 flags = 0;
>         } else
>                 setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
> -       ret = run_command_v_opt(helper_argv, flags);
> +       rc = run_command_v_opt(helper_argv, flags);
>
>         /* TODO: audit for interaction with sparse-index. */
>         ensure_full_index(&wtindex);
> @@ -660,7 +661,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>         strbuf_release(&tmpdir);
>         UNLEAK(working_tree_dups);
>
> -       return ret;
> +       return ret < 0 ? 1 : rc;
>  }


"return ret < 0 ? 1 : rc;" seems rather subtle.

In the submitted patch we're able to keep this as "return ret;" by
never assigning -1 in the first place.

My rationale is that it's better to not have to apply this fixup after the fact.

Thanks for reviewing.
--
David
diff mbox series

Patch

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 21e055d13a..861a4ec5fa 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -252,16 +252,6 @@  static void changed_files(struct hashmap *result, const char *index_path,
 	strbuf_release(&buf);
 }
 
-static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
-{
-	struct strbuf buf = STRBUF_INIT;
-	strbuf_addstr(&buf, tmpdir);
-	remove_dir_recursively(&buf, 0);
-	if (exit_code)
-		warning(_("failed: %d"), exit_code);
-	exit(exit_code);
-}
-
 static int ensure_leading_directories(char *path)
 {
 	switch (safe_create_leading_directories(path)) {
@@ -333,16 +323,16 @@  static int checkout_path(unsigned mode, struct object_id *oid,
 static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 			struct child_process *child)
 {
-	char tmpdir[PATH_MAX];
 	struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
 	struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
 	struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
 	struct strbuf wtdir = STRBUF_INIT;
-	char *lbase_dir, *rbase_dir;
+	struct strbuf tmpdir = STRBUF_INIT;
+	char *lbase_dir = NULL, *rbase_dir = NULL;
 	size_t ldir_len, rdir_len, wtdir_len;
 	const char *workdir, *tmp;
 	int ret = 0, i;
-	FILE *fp;
+	FILE *fp = NULL;
 	struct hashmap working_tree_dups = HASHMAP_INIT(working_tree_entry_cmp,
 							NULL);
 	struct hashmap submodules = HASHMAP_INIT(pair_cmp, NULL);
@@ -351,7 +341,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct pair_entry *entry;
 	struct index_state wtindex;
 	struct checkout lstate, rstate;
-	int rc, flags = RUN_GIT_CMD, err = 0;
+	int flags = RUN_GIT_CMD, err = 0;
 	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
 	struct hashmap wt_modified, tmp_modified;
 	int indices_loaded = 0;
@@ -360,11 +350,15 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 
 	/* Setup temp directories */
 	tmp = getenv("TMPDIR");
-	xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
-	if (!mkdtemp(tmpdir))
-		return error("could not create '%s'", tmpdir);
-	strbuf_addf(&ldir, "%s/left/", tmpdir);
-	strbuf_addf(&rdir, "%s/right/", tmpdir);
+	strbuf_add_absolute_path(&tmpdir, tmp ? tmp : "/tmp");
+	strbuf_trim_trailing_dir_sep(&tmpdir);
+	strbuf_addstr(&tmpdir, "/git-difftool.XXXXXX");
+	if (!mkdtemp(tmpdir.buf)) {
+		ret = error("could not create '%s'", tmpdir.buf);
+		goto finish;
+	}
+	strbuf_addf(&ldir, "%s/left/", tmpdir.buf);
+	strbuf_addf(&rdir, "%s/right/", tmpdir.buf);
 	strbuf_addstr(&wtdir, workdir);
 	if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
 		strbuf_addch(&wtdir, '/');
@@ -580,7 +574,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		flags = 0;
 	} else
 		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-	rc = run_command_v_opt(helper_argv, flags);
+	ret = run_command_v_opt(helper_argv, flags);
 
 	/* TODO: audit for interaction with sparse-index. */
 	ensure_full_index(&wtindex);
@@ -614,7 +608,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		if (!indices_loaded) {
 			struct lock_file lock = LOCK_INIT;
 			strbuf_reset(&buf);
-			strbuf_addf(&buf, "%s/wtindex", tmpdir);
+			strbuf_addf(&buf, "%s/wtindex", tmpdir.buf);
 			if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
 			    write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {
 				ret = error("could not write %s", buf.buf);
@@ -644,11 +638,14 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	}
 
 	if (err) {
-		warning(_("temporary files exist in '%s'."), tmpdir);
+		warning(_("temporary files exist in '%s'."), tmpdir.buf);
 		warning(_("you may want to cleanup or recover these."));
-		exit(1);
-	} else
-		exit_cleanup(tmpdir, rc);
+		ret = 1;
+	} else {
+		remove_dir_recursively(&tmpdir, 0);
+		if (ret)
+			warning(_("failed: %d"), ret);
+	}
 
 finish:
 	if (fp)
@@ -660,6 +657,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	strbuf_release(&rdir);
 	strbuf_release(&wtdir);
 	strbuf_release(&buf);
+	strbuf_release(&tmpdir);
 
 	return ret;
 }
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 528e0dabf0..096456292c 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -453,6 +453,13 @@  run_dir_diff_test 'difftool --dir-diff' '
 	grep "^file$" output
 '
 
+run_dir_diff_test 'difftool --dir-diff avoids repeated slashes in TMPDIR' '
+	TMPDIR="${TMPDIR:-/tmp}////" \
+		git difftool --dir-diff $symlinks --extcmd echo branch >output &&
+	grep -v // output >actual &&
+	test_line_count = 1 actual
+'
+
 run_dir_diff_test 'difftool --dir-diff ignores --prompt' '
 	git difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&
 	grep "^sub$" output &&