All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches
@ 2021-09-23  4:12 David Aguilar
  2021-09-23  4:12 ` [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode David Aguilar
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: David Aguilar @ 2021-09-23  4:12 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Johannes Schindelin, Alan Blotz,
	Đoàn Trần Công Danh, Eric Sunshine,
	Jeff King

This is a resend of previously submitted patches.

Links to previous discussions:
https://public-inbox.org/git/20210919203832.91207-1-davvid@gmail.com/
https://public-inbox.org/git/20210920220855.18637-1-davvid@gmail.com/
https://public-inbox.org/git/CAPig+cTBfP5_czsPiALcF3tODJmNfXvNkTjqVFRbHCS535d-ng@mail.gmail.com/

Patches 2/3 and 3/3 have been rebased against "next" to resolve
the clash with ab/retire-option-argument.

Patch 1/3 is maint-worthy and can be applied there without conflicts.
The commit message now uses the commit message ref format when referring to
other commits. Its test was adjusted to do "cd xyz" instead of "cd ./xyz".

Patch 2/3 has the strbuf leak avoidance brought up in the v4 round.

Patch 3/3 is trivial and unchanged.

David Aguilar (3):
  difftool: fix symlink-file writing in dir-diff mode
  difftool: create a tmpdir path without repeated slashes
  difftool: add a missing space to the run_dir_diff() comments

 builtin/difftool.c  | 52 +++++++++++++++----------------
 t/t7800-difftool.sh | 75 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 99 insertions(+), 28 deletions(-)

-- 
2.33.0.720.g59ef144b50


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode
  2021-09-23  4:12 [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches David Aguilar
@ 2021-09-23  4:12 ` David Aguilar
  2021-09-23 21:46   ` Junio C Hamano
  2021-09-23  4:12 ` [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes David Aguilar
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: David Aguilar @ 2021-09-23  4:12 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Johannes Schindelin, Alan Blotz,
	Đoàn Trần Công Danh, Eric Sunshine,
	Jeff King

The difftool dir-diff mode handles symlinks by replacing them with their
readlink(2) values. This allows diff tools to see changes to symlinks
as if they were regular text diffs with the old and new path values.
This is analogous to what "git diff" displays when symlinks change.

The temporary diff directories that are created initially contain
symlinks because they get checked-out using a temporary index that
retains the original symlinks as checked-in to the repository.

A bug was introduced when difftool was rewritten in C that made
difftool write the readlink(2) contents into the pointed-to file rather
than the symlink itself. The write was going through the symlink and
writing to its target rather than writing to the symlink path itself.

Replace symlinks with raw text files by unlinking the symlink path
before writing the readlink(2) content into them.

When 18ec800512 (difftool: handle modified symlinks in dir-diff mode,
2017-03-15) added handling for modified symlinks this bug got recorded
in the test suite. The tests included the pointed-to symlink target
paths. These paths were being reported because difftool was erroneously
writing to them, but they should have never been reported nor written.

Correct the modified-symlinks test cases by removing the target files
from the expected output.

Add a test to ensure that symlinks are written with the readlink(2)
values and that the target files contain their original content.

Reported-by: Alan Blotz <work@blotz.org>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 builtin/difftool.c  |  2 ++
 t/t7800-difftool.sh | 67 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index bb9fe7245a..21e055d13a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		if (*entry->left) {
 			add_path(&ldir, ldir_len, entry->path);
 			ensure_leading_directories(ldir.buf);
+			unlink(ldir.buf);
 			write_file(ldir.buf, "%s", entry->left);
 		}
 		if (*entry->right) {
 			add_path(&rdir, rdir_len, entry->path);
 			ensure_leading_directories(rdir.buf);
+			unlink(rdir.buf);
 			write_file(rdir.buf, "%s", entry->right);
 		}
 	}
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index a173f564bc..528e0dabf0 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -674,7 +674,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	rm c &&
 	ln -s d c &&
 	cat >expect <<-EOF &&
-		b
 		c
 
 		c
@@ -710,7 +709,6 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	# Deleted symlinks
 	rm -f c &&
 	cat >expect <<-EOF &&
-		b
 		c
 
 	EOF
@@ -723,6 +721,71 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success SYMLINKS 'difftool --dir-diff writes symlinks as raw text' '
+	# Start out on a branch called "branch-init".
+	git init -b branch-init symlink-files &&
+	(
+		cd symlink-files &&
+		# This test ensures that symlinks are written as raw text.
+		# The "cat" tools output link and file contents.
+		git config difftool.cat-left-link.cmd "cat \"\$LOCAL/link\"" &&
+		git config difftool.cat-left-a.cmd "cat \"\$LOCAL/file-a\"" &&
+		git config difftool.cat-right-link.cmd "cat \"\$REMOTE/link\"" &&
+		git config difftool.cat-right-b.cmd "cat \"\$REMOTE/file-b\"" &&
+
+		# Record the empty initial state so that we can come back here
+		# later and not have to consider the any cases where difftool
+		# will create symlinks back into the worktree.
+		test_tick &&
+		git commit --allow-empty -m init &&
+
+		# Create a file called "file-a" with a symlink pointing to it.
+		git switch -c branch-a &&
+		echo a >file-a &&
+		ln -s file-a link &&
+		git add file-a link &&
+		test_tick &&
+		git commit -m link-to-file-a &&
+
+		# Create a file called "file-b" and point the symlink to it.
+		git switch -c branch-b &&
+		echo b >file-b &&
+		rm link &&
+		ln -s file-b link &&
+		git add file-b link &&
+		git rm file-a &&
+		test_tick &&
+		git commit -m link-to-file-b &&
+
+		# Checkout the initial branch so that the --symlinks behavior is
+		# not activated. The two directories should be completely
+		# independent with no symlinks pointing back here.
+		git switch branch-init &&
+
+		# The left link must be "file-a" and "file-a" must contain "a".
+		echo file-a >expect &&
+		git difftool --symlinks --dir-diff --tool cat-left-link \
+			branch-a branch-b >actual &&
+		test_cmp expect actual &&
+
+		echo a >expect &&
+		git difftool --symlinks --dir-diff --tool cat-left-a \
+			branch-a branch-b >actual &&
+		test_cmp expect actual &&
+
+		# The right link must be "file-b" and "file-b" must contain "b".
+		echo file-b >expect &&
+		git difftool --symlinks --dir-diff --tool cat-right-link \
+			branch-a branch-b >actual &&
+		test_cmp expect actual &&
+
+		echo b >expect &&
+		git difftool --symlinks --dir-diff --tool cat-right-b \
+			branch-a branch-b >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'add -N and difftool -d' '
 	test_when_finished git reset --hard &&
 
-- 
2.33.0.720.g59ef144b50


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes
  2021-09-23  4:12 [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches David Aguilar
  2021-09-23  4:12 ` [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode David Aguilar
@ 2021-09-23  4:12 ` David Aguilar
  2021-09-24 10:35   ` Ævar Arnfjörð Bjarmason
  2021-09-23  4:12 ` [PATCH v5 3/3] difftool: add a missing space to the run_dir_diff() comments David Aguilar
  2021-09-23 18:24 ` [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches Junio C Hamano
  3 siblings, 1 reply; 9+ messages in thread
From: David Aguilar @ 2021-09-23  4:12 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Johannes Schindelin, Alan Blotz,
	Đoàn Trần Công Danh, Eric Sunshine,
	Jeff King

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(-)

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 &&
-- 
2.33.0.720.g59ef144b50


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v5 3/3] difftool: add a missing space to the run_dir_diff() comments
  2021-09-23  4:12 [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches David Aguilar
  2021-09-23  4:12 ` [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode David Aguilar
  2021-09-23  4:12 ` [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes David Aguilar
@ 2021-09-23  4:12 ` David Aguilar
  2021-09-23 18:24 ` [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches Junio C Hamano
  3 siblings, 0 replies; 9+ messages in thread
From: David Aguilar @ 2021-09-23  4:12 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Johannes Schindelin, Alan Blotz,
	Đoàn Trần Công Danh, Eric Sunshine,
	Jeff King

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 builtin/difftool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 861a4ec5fa..a972431f6d 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -542,7 +542,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	}
 
 	/*
-	 * Symbolic links require special treatment.The standard "git diff"
+	 * Symbolic links require special treatment. The standard "git diff"
 	 * shows only the link itself, not the contents of the link target.
 	 * This loop replicates that behavior.
 	 */
-- 
2.33.0.720.g59ef144b50


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches
  2021-09-23  4:12 [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches David Aguilar
                   ` (2 preceding siblings ...)
  2021-09-23  4:12 ` [PATCH v5 3/3] difftool: add a missing space to the run_dir_diff() comments David Aguilar
@ 2021-09-23 18:24 ` Junio C Hamano
  3 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2021-09-23 18:24 UTC (permalink / raw)
  To: David Aguilar
  Cc: Git Mailing List, Johannes Schindelin, Alan Blotz,
	Đoàn Trần Công Danh, Eric Sunshine,
	Jeff King

David Aguilar <davvid@gmail.com> writes:

> This is a resend of previously submitted patches.

Very much appreciated.
Will queue.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode
  2021-09-23  4:12 ` [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode David Aguilar
@ 2021-09-23 21:46   ` Junio C Hamano
  2021-09-30 17:03     ` David Aguilar
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2021-09-23 21:46 UTC (permalink / raw)
  To: David Aguilar
  Cc: Git Mailing List, Johannes Schindelin, Alan Blotz,
	Đoàn Trần Công Danh, Eric Sunshine,
	Jeff King

David Aguilar <davvid@gmail.com> writes:

> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index bb9fe7245a..21e055d13a 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  		if (*entry->left) {
>  			add_path(&ldir, ldir_len, entry->path);
>  			ensure_leading_directories(ldir.buf);
> +			unlink(ldir.buf);
>  			write_file(ldir.buf, "%s", entry->left);
>  		}
>  		if (*entry->right) {
>  			add_path(&rdir, rdir_len, entry->path);
>  			ensure_leading_directories(rdir.buf);
> +			unlink(rdir.buf);
>  			write_file(rdir.buf, "%s", entry->right);
>  		}
>  	}

Curiously, this pattern repeats twice in the vicinity of the code.
We cannot see it because it is out of pre-context, but the above is
a body of a loop that iterates over "symlinks2" hashmap.  There is
another identical loop that iterates over "submodules", and we are
not protecting ourselves from following a stray/leftover symbolic
link in the loop.

I wonder if we should do the same to be defensive?  I also wondered
if write_file() should be the one that may want to be doing the
unlink(), but I ran out of time before I finished reading all the
callers to see if that is even a correct thing to do (meaning: some
caller may want to truly overwrite an existing file, and follow
symlinks if there already is, and I didn't audit all callers to see
if there is no such caller).

The two identical looking loops also look like an accident waiting
to happen---a patch like this that wants to touch only one of them
would risk application to the other, wrong, loop if the patch gets
old enough and patch offset grows larger ;-).

Thanks.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes
  2021-09-23  4:12 ` [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes David Aguilar
@ 2021-09-24 10:35   ` Ævar Arnfjörð Bjarmason
  2021-09-30 17:05     ` David Aguilar
  0 siblings, 1 reply; 9+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-24 10:35 UTC (permalink / raw)
  To: David Aguilar
  Cc: Junio C Hamano, Git Mailing List, Johannes Schindelin,
	Alan Blotz, Đoàn Trần Công Danh,
	Eric Sunshine, Jeff King


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,

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode
  2021-09-23 21:46   ` Junio C Hamano
@ 2021-09-30 17:03     ` David Aguilar
  0 siblings, 0 replies; 9+ messages in thread
From: David Aguilar @ 2021-09-30 17:03 UTC (permalink / raw)
  To: Junio C Hamano, Ævar Arnfjörð Bjarmason
  Cc: Git Mailing List, Johannes Schindelin, Alan Blotz,
	Đoàn Trần Công Danh, Eric Sunshine,
	Jeff King

On Thu, Sep 23, 2021 at 2:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> David Aguilar <davvid@gmail.com> writes:
>
> > diff --git a/builtin/difftool.c b/builtin/difftool.c
> > index bb9fe7245a..21e055d13a 100644
> > --- a/builtin/difftool.c
> > +++ b/builtin/difftool.c
> > @@ -557,11 +557,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
> >               if (*entry->left) {
> >                       add_path(&ldir, ldir_len, entry->path);
> >                       ensure_leading_directories(ldir.buf);
> > +                     unlink(ldir.buf);
> >                       write_file(ldir.buf, "%s", entry->left);
> >               }
> >               if (*entry->right) {
> >                       add_path(&rdir, rdir_len, entry->path);
> >                       ensure_leading_directories(rdir.buf);
> > +                     unlink(rdir.buf);
> >                       write_file(rdir.buf, "%s", entry->right);
> >               }
> >       }
>
> Curiously, this pattern repeats twice in the vicinity of the code.
> We cannot see it because it is out of pre-context, but the above is
> a body of a loop that iterates over "symlinks2" hashmap.  There is
> another identical loop that iterates over "submodules", and we are
> not protecting ourselves from following a stray/leftover symbolic
> link in the loop.

I don't think the submodules loop ever runs into a scenario where the
unlink would be relevant but it certainly wouldn't hurt from a defensive
perspective.

>
> I wonder if we should do the same to be defensive?  I also wondered
> if write_file() should be the one that may want to be doing the
> unlink(), but I ran out of time before I finished reading all the
> callers to see if that is even a correct thing to do (meaning: some
> caller may want to truly overwrite an existing file, and follow
> symlinks if there already is, and I didn't audit all callers to see
> if there is no such caller).

From my reading of write_file() usage it seems like we're better
off dealing with this just in difftool only. We'd be doing a wasteful
unlink() in most situations if we handled the unlinks in write_file().


> The two identical looking loops also look like an accident waiting
> to happen---a patch like this that wants to touch only one of them
> would risk application to the other, wrong, loop if the patch gets
> old enough and patch offset grows larger ;-).

Indeed. Lifting this pattern out into a common helper would
help reduce this risk here.

I have a follow-up patch that addresses this and the edge cases
that Ævar pointed out about the exit codes that was just submitted.

They are incremental patches on top of these patches but I resent the
entire series for convenience.
--
David

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes
  2021-09-24 10:35   ` Ævar Arnfjörð Bjarmason
@ 2021-09-30 17:05     ` David Aguilar
  0 siblings, 0 replies; 9+ messages in thread
From: David Aguilar @ 2021-09-30 17:05 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, Git Mailing List, Johannes Schindelin,
	Alan Blotz, Đoàn Trần Công Danh,
	Eric Sunshine, Jeff King

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-09-30 17:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-23  4:12 [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches David Aguilar
2021-09-23  4:12 ` [PATCH v5 1/3] difftool: fix symlink-file writing in dir-diff mode David Aguilar
2021-09-23 21:46   ` Junio C Hamano
2021-09-30 17:03     ` David Aguilar
2021-09-23  4:12 ` [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes David Aguilar
2021-09-24 10:35   ` Ævar Arnfjörð Bjarmason
2021-09-30 17:05     ` David Aguilar
2021-09-23  4:12 ` [PATCH v5 3/3] difftool: add a missing space to the run_dir_diff() comments David Aguilar
2021-09-23 18:24 ` [PATCH v5 0/3] difftool dir-diff symlink bug fix and cleanup patches Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.