| Message ID | 20210919203832.91207-3-davvid@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | difftool dir-diff symlink bug fix and cleanup patches | expand |
On Sun, Sep 19, 2021 at 4:38 PM David Aguilar <davvid@gmail.com> wrote: > difftool: use a strbuf to create a tmpdir path without repeated slashes > > 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. Mentioning strbuf in the commit message misleads the reviewer into thinking that it somehow merits extra attention and close scrutiny. In fact, the opposite is true: strbuf is just an implementation detail; there is no reason to mention it in the commit message at all. The commit message's emphasis on strbuf distracts the reader from the real purpose of this change, which is to fix a cosmetic issue (and maybe a real issue on Windows in which double-slash can have significance?). So, perhaps: difftool: fold out repeated slashes from TMPDIR Paths generated by difftool are passed to user-facing diff tools. Supplying paths with repeated slashes is a cosmetic blemish that is exposed to users and can be avoided. Therefore, strip trailing slashes from the value of TMPDIR to avoid repeated slashes in the generated paths. > Add a unit test to ensure that repeated slashes are not present. Unless there is something unusual or tricky about the new test that requires extra explanation in the commit message, there is little reason to mention that you're adding a new test, so I'd probably drop this line, as well. After all, the patch easily speaks for itself, and a reviewer can see at a glance that you're adding a new test. > diff --git a/builtin/difftool.c b/builtin/difftool.c > @@ -252,11 +252,10 @@ static void changed_files(struct hashmap *result, const char *index_path, > -static NORETURN void exit_cleanup(const char *tmpdir, int exit_code) > +static NORETURN void exit_cleanup(struct strbuf *buf, int exit_code) > { > - struct strbuf buf = STRBUF_INIT; > - strbuf_addstr(&buf, tmpdir); > - remove_dir_recursively(&buf, 0); > + remove_dir_recursively(buf, 0); > + strbuf_release(buf); > if (exit_code) > warning(_("failed: %d"), exit_code); > exit(exit_code); It feels wrong to be releasing the caller-supplied strbuf; this change makes it harder to reason about ownership. Normally, the entity which allocates a resource should be the one to release it. More on this below... > @@ -333,11 +332,11 @@ static int checkout_path(unsigned mode, struct object_id *oid, > + struct strbuf tmpdir = STRBUF_INIT; > + strbuf_add_absolute_path(&tmpdir, tmp ? tmp : "/tmp"); > + strbuf_trim_trailing_dir_sep(&tmpdir); > + strbuf_addstr(&tmpdir, "/git-difftool.XXXXXX"); > + if (!mkdtemp(tmpdir.buf)) > + return error("could not create '%s'", tmpdir.buf); Leaking the `tmpdir` strbuf here. You'd want: if (!mkdtemp(tmpdir.buf)) { error("could not create '%s'", tmpdir.buf); strbuf_release(&tmpdir); return -1; } > @@ -644,11 +645,11 @@ 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); > + exit_cleanup(&tmpdir, rc); ... [continued from above] Both branches in this conditional terminate the program either directly by `exit(1)` or indirectly through `exit_cleanup(...)`. Yet only the `exit_cleanup(...)` branch releases the strbuf (because you updated `exit_cleanup()` above to do so); the other branch just leaks the strbuf. This is inconsistent. Since we're exiting anyhow, and since `exit_cleanup()` was already leaking its own strbuf even before this patch, and since it feels somewhat dirty to have `exit_cleanup()` responsible for releasing a resource it didn't allocate, it may make sense just to maintain the status-quo and just leak the strbuf before exiting. That is, don't make any changes to `exit_cleanup()`, and let both of these branches leak the strbuf. On the other hand, if you really do want to release the strbuf, then it would be more consistent for both branches in this conditional to do so, not just one. That is, add a strbuf_release() to the `then` arm. > finish: > if (fp) > @@ -660,6 +661,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); Correctly releasing the strbuf. Good.
diff --git a/builtin/difftool.c b/builtin/difftool.c index 21e055d13a..daa438be09 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -252,11 +252,10 @@ 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) +static NORETURN void exit_cleanup(struct strbuf *buf, int exit_code) { - struct strbuf buf = STRBUF_INIT; - strbuf_addstr(&buf, tmpdir); - remove_dir_recursively(&buf, 0); + remove_dir_recursively(buf, 0); + strbuf_release(buf); if (exit_code) warning(_("failed: %d"), exit_code); exit(exit_code); @@ -333,11 +332,11 @@ 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; + struct strbuf tmpdir = STRBUF_INIT; char *lbase_dir, *rbase_dir; size_t ldir_len, rdir_len, wtdir_len; const char *workdir, *tmp; @@ -360,11 +359,13 @@ 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)) + return error("could not create '%s'", tmpdir.buf); + 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, '/'); @@ -614,7 +615,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 +645,11 @@ 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); + exit_cleanup(&tmpdir, rc); finish: if (fp) @@ -660,6 +661,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 07a52fb8e1..0670b617b4 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 &&
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. Add a unit test to ensure that repeated slashes are not present. Signed-off-by: David Aguilar <davvid@gmail.com> --- builtin/difftool.c | 28 +++++++++++++++------------- t/t7800-difftool.sh | 7 +++++++ 2 files changed, 22 insertions(+), 13 deletions(-)