diff mbox series

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

Message ID 20210930170146.61489-3-davvid@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v6,1/5] difftool: create a tmpdir path without repeated slashes | expand

Commit Message

David Aguilar Sept. 30, 2021, 5:01 p.m. UTC
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(-)

Comments

Junio C Hamano Sept. 30, 2021, 10:06 p.m. UTC | #1
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.
David Aguilar Sept. 30, 2021, 11:25 p.m. UTC | #2
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
Junio C Hamano Oct. 1, 2021, 12:12 a.m. UTC | #3
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.
diff mbox series

Patch

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;
 	}