| 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 |
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.
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
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 --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; }
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(-)