[PATCH v6 1/5] difftool: create a tmpdir path without repeated slashes

[PATCH v6 1/5] difftool: create a tmpdir path without repeated slashes

[David Aguilar]

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>
---
This patch is a resend based off of "next" so that this series can be
tracked together.

This patch is unchanged since the v5 was submitted, but there are
new related patches that proceed it.

 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 210da03908..9d2dfd3361 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.887.g8db6ae3373

[PATCH v6 2/5] difftool: add a missing space to the run_dir_diff() comments

[David Aguilar]

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Trivial tweak. Unchanged but resubmitted for ease of application.

 builtin/difftool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 9d2dfd3361..fdaaa86bff 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.887.g8db6ae3373

[PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()

[David Aguilar]

difftool was forwarding the -1 result from error() to cmd_main(), which
is implementation-defined since it is outside of the 0-255 range
specified by POSIX for program exit codes.

Stop assigning the result of error() to `ret`. Assign a value of 1
whenever internal errors are detected instead.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This patch addresses the note from Ævar about returning -1 to cmd_main().

 builtin/difftool.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index fdaaa86bff..e419bd3cd1 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -447,7 +447,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 
 		if (lmode && status != 'C') {
 			if (checkout_path(lmode, &loid, src_path, &lstate)) {
-				ret = error("could not write '%s'", src_path);
+				ret = 1;
+				error("could not write '%s'", src_path);
 				goto finish;
 			}
 		}
@@ -468,8 +469,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 			if (!use_wt_file(workdir, dst_path, &roid)) {
 				if (checkout_path(rmode, &roid, dst_path,
 						  &rstate)) {
-					ret = error("could not write '%s'",
-						    dst_path);
+					ret = 1;
+					error("could not write '%s'", dst_path);
 					goto finish;
 				}
 			} else if (!is_null_oid(&roid)) {
@@ -487,15 +488,16 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 
 				add_path(&rdir, rdir_len, dst_path);
 				if (ensure_leading_directories(rdir.buf)) {
-					ret = error("could not create "
-						    "directory for '%s'",
-						    dst_path);
+					ret = 1;
+					error("could not create directory for '%s'",
+						dst_path);
 					goto finish;
 				}
 				add_path(&wtdir, wtdir_len, dst_path);
 				if (symlinks) {
 					if (symlink(wtdir.buf, rdir.buf)) {
-						ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
+						ret = 1;
+						error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
 						goto finish;
 					}
 				} else {
@@ -504,7 +506,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 						st.st_mode = 0644;
 					if (copy_file(rdir.buf, wtdir.buf,
 						      st.st_mode)) {
-						ret = error("could not copy '%s' to '%s'", wtdir.buf, rdir.buf);
+						ret = 1;
+						error("could not copy '%s' to '%s'", wtdir.buf, rdir.buf);
 						goto finish;
 					}
 				}
@@ -515,7 +518,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	fclose(fp);
 	fp = NULL;
 	if (finish_command(child)) {
-		ret = error("error occurred running diff --raw");
+		ret = 1;
+		error("error occurred running diff --raw");
 		goto finish;
 	}
 
-- 
2.33.0.887.g8db6ae3373

[PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function

[David Aguilar]

Add a write_entry() helper function to handle the unlinking and writing
of the dir-diff submodule and symlink stand-in files.

Use write_entry() inside of the hashmap loops to eliminate duplicate
code and to safeguard the submodules hashmap loop against the
symlink-chasing behavior that 5bafb3576a (difftool: fix symlink-file
writing in dir-diff mode, 2021-09-22) addressed.

The submodules loop should not strictly require the unlink() call that
this is introducing to them, but it does not necessarily hurt them
either beyond the cost of the extra unlink().

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This is cleanup refactoring that Junio suggested when
5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)
touched this area of the code.

 builtin/difftool.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index e419bd3cd1..bbb8b399c2 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -320,6 +320,17 @@ static int checkout_path(unsigned mode, struct object_id *oid,
 	return ret;
 }
 
+static void write_entry(const char *path, const char *content,
+			struct strbuf *buf, size_t len)
+{
+	if (!*content)
+		return;
+	add_path(buf, len, path);
+	ensure_leading_directories(buf->buf);
+	unlink(buf->buf);
+	write_file(buf->buf, "%s", content);
+}
+
 static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 			struct child_process *child)
 {
@@ -533,16 +544,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	 */
 	hashmap_for_each_entry(&submodules, &iter, entry,
 				entry /* member name */) {
-		if (*entry->left) {
-			add_path(&ldir, ldir_len, entry->path);
-			ensure_leading_directories(ldir.buf);
-			write_file(ldir.buf, "%s", entry->left);
-		}
-		if (*entry->right) {
-			add_path(&rdir, rdir_len, entry->path);
-			ensure_leading_directories(rdir.buf);
-			write_file(rdir.buf, "%s", entry->right);
-		}
+		write_entry(entry->path, entry->left, &ldir, ldir_len);
+		write_entry(entry->path, entry->right, &rdir, rdir_len);
 	}
 
 	/*
@@ -552,18 +555,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	 */
 	hashmap_for_each_entry(&symlinks2, &iter, entry,
 				entry /* member name */) {
-		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);
-		}
+
+		write_entry(entry->path, entry->left, &ldir, ldir_len);
+		write_entry(entry->path, entry->right, &rdir, rdir_len);
 	}
 
 	strbuf_release(&buf);
-- 
2.33.0.887.g8db6ae3373

[PATCH v6 5/5] difftool: remove an unnecessary call to strbuf_release()

[David Aguilar]

The `buf` strbuf is reused again later in the same function, so there
is no benefit to calling strbuf_release(). The subsequent usage is
already using strbuf_reset() to reset the buffer, so releasing it
early is only going to lead to a wasteful reallocation.

Remove the early call to strbuf_release(). The same strbuf is already
cleaned up in the "finish:" section so nothing is leaked, either.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This is a small cleanup that I noticed while working on the new patches.
There are no previous versions of this patch.

 builtin/difftool.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index bbb8b399c2..859ccc6db3 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -560,8 +560,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		write_entry(entry->path, entry->right, &rdir, rdir_len);
 	}
 
-	strbuf_release(&buf);
-
 	strbuf_setlen(&ldir, ldir_len);
 	helper_argv[1] = ldir.buf;
 	strbuf_setlen(&rdir, rdir_len);
-- 
2.33.0.887.g8db6ae3373

Re: [PATCH v6 1/5] difftool: create a tmpdir path without repeated slashes

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> 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>
> ---
> This patch is a resend based off of "next" so that this series can be
> tracked together.
>
> This patch is unchanged since the v5 was submitted, but there are
> new related patches that proceed it.

OK.  I confirmed that this is unchanged from the first step of v5.

Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> difftool was forwarding the -1 result from error() to cmd_main(), which
> is implementation-defined since it is outside of the 0-255 range
> specified by POSIX for program exit codes.
>
> Stop assigning the result of error() to `ret`. Assign a value of 1
> whenever internal errors are detected instead.

Many existing codepaths take advantage of error() returning -1 and I
do not see anything is wrong to keep that "negative is error"
convention for the value of "ret" variable.  Most lines in this
patch however split that "ret = error(...)" pattern into two
statements and I do not see a very good reason for it.

Worse yet, after applying this patch, there still are at least three
assignments to "ret" that can give it a negative value:

	if (!mkdtemp(tmpdir.buf)) {
		ret = error("could not create '%s'", tmpdir.buf);
		goto finish;
	}

	ret = run_command_v_opt(helper_argv, flags);

	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);
		goto finish;
	}

Among them, the return value from run_command_v_opt() eventually
come from wait_or_whine(), I think, so it may be generic -1 or
it may be WEXITSTATUS() of the child process.

But I am not sure if this partcular caller cares.  It is not
prepared to handle -1 and positive return from run_command_v_opt()
any differently.  So I think a single

-	return ret;
+	return !!ret;

at the end would be much easier to reason about and maintain.

Re: [PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> This is cleanup refactoring that Junio suggested when
> 5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)
> touched this area of the code.

Not really what I would want to take credit for  ;-)

> +static void write_entry(const char *path, const char *content,
> +			struct strbuf *buf, size_t len)
> +{
> +	if (!*content)
> +		return;

I am not sure "this function is unable to write an empty file" is a
limitation we want to give to an otherwise more or less generic
helper function that may be useful in this file (it probably is not
very useful outside difftool, as what add_path() does seems quite
specific to it).

Also, is "write entry" a good name for this helper?  The fact that
the contents came from entry->$side is lost inside this callee.  It
looks more like "create a file for <path> and write <content> to it",
i.e. a variant of write_file() but inside the tree specified by the
extra <buf, len> pair.  So perhaps 

	write_file_in_directory(buf, len, path, content);

to match how the write_file() takes its parameters?  While
write_file() takes a single pathname with the payload, this thing
takes three parameters <buf, len, path> to specify to which
"file-in-directory" the payload is written.

> +	add_path(buf, len, path);
> +	ensure_leading_directories(buf->buf);
> +	unlink(buf->buf);
> +	write_file(buf->buf, "%s", content);
> +}

Other than these two minor points, this looks good to me.

Thanks.

Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()

[David Aguilar]

On Thu, Sep 30, 2021 at 3:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> David Aguilar <davvid@gmail.com> writes:
>
> > difftool was forwarding the -1 result from error() to cmd_main(), which
> > is implementation-defined since it is outside of the 0-255 range
> > specified by POSIX for program exit codes.
> >
> > Stop assigning the result of error() to `ret`. Assign a value of 1
> > whenever internal errors are detected instead.
>
> Many existing codepaths take advantage of error() returning -1 and I
> do not see anything is wrong to keep that "negative is error"
> convention for the value of "ret" variable.  Most lines in this
> patch however split that "ret = error(...)" pattern into two
> statements and I do not see a very good reason for it.
>
> Worse yet, after applying this patch, there still are at least three
> assignments to "ret" that can give it a negative value:

Indeed.

>
>         if (!mkdtemp(tmpdir.buf)) {
>                 ret = error("could not create '%s'", tmpdir.buf);
>                 goto finish;
>         }
>
>         ret = run_command_v_opt(helper_argv, flags);
>
>         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);
>                 goto finish;
>         }
>
> Among them, the return value from run_command_v_opt() eventually
> come from wait_or_whine(), I think, so it may be generic -1 or
> it may be WEXITSTATUS() of the child process.
>
> But I am not sure if this particular caller cares.  It is not

The property I was trying to maintain is that we would forward the result
from the child process in most situations, so we should try and forward
the result from run_command_v_opt() whenever possible.

But for the others we would have to add an "ret = 1" there,
and that doesn't seem worth it since it's too hard to maintain.



> prepared to handle -1 and positive return from run_command_v_opt()
> any differently.  So I think a single
>
> -       return ret;
> +       return !!ret;
>
> at the end would be much easier to reason about and maintain.

Hmm I don't think we can use "return !!ret".

In C this does a bool cast so we lose the positive value from the
underlying diff tool when the value is quantized to 0/1 via !!ret.

That suggests that Ævar's sug is better...

    return (ret < 0) ? 1 : ret;

If that sounds good I can send a replacement series that squashes this into the
repeated-symlinks patch. It doesn't seem like we'll need a separate
patch for this.

--
David

Re: [PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function

[David Aguilar]

On Thu, Sep 30, 2021 at 3:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> David Aguilar <davvid@gmail.com> writes:
>
> > This is cleanup refactoring that Junio suggested when
> > 5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)
> > touched this area of the code.
>
> Not really what I would want to take credit for  ;-)

Likewise, even I don't like to take credit for my scrappy patches sometimes ;-)

I'll reword this to avoid mentioning the review context.


> > +static void write_entry(const char *path, const char *content,
> > +                     struct strbuf *buf, size_t len)
> > +{
> > +     if (!*content)
> > +             return;
>
> I am not sure "this function is unable to write an empty file" is a
> limitation we want to give to an otherwise more or less generic
> helper function that may be useful in this file (it probably is not
> very useful outside difftool, as what add_path() does seems quite
> specific to it).


Good point, I'll move the conditional checks into the call sites
rather than having it in the helper. It'll read a little more clearly that
way as well.


> Also, is "write entry" a good name for this helper?  The fact that
> the contents came from entry->$side is lost inside this callee.  It
> looks more like "create a file for <path> and write <content> to it",
> i.e. a variant of write_file() but inside the tree specified by the
> extra <buf, len> pair.  So perhaps
>
>         write_file_in_directory(buf, len, path, content);
>
> to match how the write_file() takes its parameters?  While
> write_file() takes a single pathname with the payload, this thing
> takes three parameters <buf, len, path> to specify to which
> "file-in-directory" the payload is written.
>
> > +     add_path(buf, len, path);
> > +     ensure_leading_directories(buf->buf);
> > +     unlink(buf->buf);
> > +     write_file(buf->buf, "%s", content);
> > +}
>
> Other than these two minor points, this looks good to me.

write_file_in_directory() is much clearer. I'll adjust the signature
in the next version.

Thanks for the review. I'll wait to hear back about 3/5 before sending v7.
-- 
David

Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>> Among them, the return value from run_command_v_opt() eventually
>> come from wait_or_whine(), I think, so it may be generic -1 or
>> it may be WEXITSTATUS() of the child process.
>>
>> But I am not sure if this particular caller cares.  It is not
>
> The property I was trying to maintain is that we would forward the result
> from the child process in most situations, so we should try and forward
> the result from run_command_v_opt() whenever possible.

Oh, if it were the case, then I agree that !!ret would not be
sufficient.

I just had an impression that this particular caller of the
run_command_v_opt() did not care, as it did not special case -1
returned from the function as different from what came from
WEXITSTATUS().

> That suggests that Ævar's sug is better...
>
>     return (ret < 0) ? 1 : ret;

Yup, that would work, as long as exit status 1 from the external
diff tool is not all that special (in which case, we cannot tell
it from 1 that came from ret = error()).

Thanks.