| Message ID | 20210919015729.98323-3-davvid@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] t7800-difftool: cleanup temporary repositories used by tests | expand |
On Sat, Sep 18, 2021 at 06:57:28PM -0700, David Aguilar wrote: > + /* Remove trailing slashes when $TMPDIR ends in '/'. */ > + while (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') { > + strbuf_setlen(&tmpdir, tmpdir.len - 1); > + } > + strbuf_addstr(&tmpdir, "/git-difftool.XXXXXX"); It sounds like this should remove any directory separator (so on Windows, it should also drop backslashes). You can use is_dir_sep() for that, but better still, you can then do: strbuf_trim_trailing_dir_sep(&tmpdir); -Peff
Am 19.09.21 um 03:57 schrieb David Aguilar: > Use a strbuf to create the buffer used for the dir-diff tmpdir. > Strip trailing slashes "/" from the value read from TMPDIR to avoid > double-slashes in the calculated paths. > > Add a unit test to ensure that double-slashes are not present. I wonder why it is necessary to strip trailing slashes? You even go so far as to add a test case, but then bury the change in a commit with a title that is about a completely different topic. So, which one of the two changes is the "while at it, do that, too" change? > @@ -360,11 +359,17 @@ 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"); > + /* Remove trailing slashes when $TMPDIR ends in '/'. */ > + while (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') { This should most likely be is_dir_sep(tmpdir.buf[tmpdir.len - 1]). -- Hannes
On Sun, Sep 19, 2021 at 2:00 AM Johannes Sixt <j6t@kdbg.org> wrote: > > Am 19.09.21 um 03:57 schrieb David Aguilar: > > Use a strbuf to create the buffer used for the dir-diff tmpdir. > > Strip trailing slashes "/" from the value read from TMPDIR to avoid > > double-slashes in the calculated paths. > > > > Add a unit test to ensure that double-slashes are not present. > > I wonder why it is necessary to strip trailing slashes? You even go so > far as to add a test case, but then bury the change in a commit with a > title that is about a completely different topic. > > So, which one of the two changes is the "while at it, do that, too" change? A better title might be: difftool: use a strbuf to generate a tmpdir path without double-slashes The double-slashes are the point. This patch is a minor cleanup that I found on the path towards fixing the data loss bug in patch 4. Thanks for the heads-up about the confusion ~ I'll reword in the next submission to make this point clearer. Ævar (cc'd) also asked why we have a patch that deletes the temporary repositories used by the tests. It sounds like it's best to leave those in place so the next submission will also drop that patch and will adjust patch 4/4 (now 3/3) so that it also does not remove the temporary repo used by its new test. > > @@ -360,11 +359,17 @@ 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"); > > + /* Remove trailing slashes when $TMPDIR ends in '/'. */ > > + while (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') { > > This should most likely be is_dir_sep(tmpdir.buf[tmpdir.len - 1]). Indeed. Peff also suggested strbuf_strip_trailing_dir_sep(&tmpdir) which is what we have in patch v2. That uses is_dir_sep(). This commit will be reworded, patch 1/4 "t7800-difftool: cleanup temporary repositories" will be dropped, and the final patch will be adjusted to not cleanup its temporary test repository. I'll resend all 3 patches later with these suggestions as "v4". cheers,
Am 19.09.21 um 20:13 schrieb David Aguilar: > A better title might be: > > difftool: use a strbuf to generate a tmpdir path without double-slashes > > The double-slashes are the point. This patch is a minor cleanup that I > found on the path towards fixing the data loss bug in patch 4. > > Thanks for the heads-up about the confusion ~ I'll reword in the next > submission to make this point clearer. Thanks. My primary concern with the patch was actually that it looks like mere code churn that introduces an error by not using is_dir_sep(). This is now settled. But still, without a justification why the slashes should be cleaned up, the patch looks like code churn. You can ignore me if it is obvious for others why we need the patch. -- Hannes
On Sun, Sep 19, 2021 at 11:46 AM Johannes Sixt <j6t@kdbg.org> wrote: > > Am 19.09.21 um 20:13 schrieb David Aguilar: > > A better title might be: > > > > difftool: use a strbuf to generate a tmpdir path without double-slashes > > > > The double-slashes are the point. This patch is a minor cleanup that I > > found on the path towards fixing the data loss bug in patch 4. > > > > Thanks for the heads-up about the confusion ~ I'll reword in the next > > submission to make this point clearer. > > Thanks. My primary concern with the patch was actually that it looks > like mere code churn that introduces an error by not using is_dir_sep(). > This is now settled. > > But still, without a justification why the slashes should be cleaned up, > the patch looks like code churn. You can ignore me if it is obvious for > others why we need the patch. Nah, that's a good point. Thanks for clarifying. I'll hold off on resending until we've reached consensus on this patch. The final bugfix patch is the most important one in the series so perhaps I should reorder the series so that the bugfix comes first, and these cosmetic improvements and typo fixes are the later ones in the series? That'll make it so that the bugfix is easier to backport and not entangled with these prep fixups. I see double-slashes when using the dir-diff feature and they just look wrong to me, but "striving for perfection" is a mere subjective justification. The double-slashes fixup is cosmetic from a technical perspective, but since the paths are exposed to the diff tools it's a cosmetic blemish that users will see front and center. The test environment where I observed TMPDIR containing a trailing slash is macOS, so it's a fairly common setup. One justification is that we should not expose such blemishes to users -- that's what I've written below but perhaps there's a better way to express it? What do you and others think about the proposed message below and whether or not this patch is worth applying? """ 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. Add a unit test to ensure that repeated slashes are not present. """
Am 19.09.21 um 21:28 schrieb David Aguilar: > On Sun, Sep 19, 2021 at 11:46 AM Johannes Sixt <j6t@kdbg.org> wrote: >> >> Am 19.09.21 um 20:13 schrieb David Aguilar: >>> A better title might be: >>> >>> difftool: use a strbuf to generate a tmpdir path without double-slashes >>> >>> The double-slashes are the point. This patch is a minor cleanup that I >>> found on the path towards fixing the data loss bug in patch 4. >>> >>> Thanks for the heads-up about the confusion ~ I'll reword in the next >>> submission to make this point clearer. >> >> Thanks. My primary concern with the patch was actually that it looks >> like mere code churn that introduces an error by not using is_dir_sep(). >> This is now settled. >> >> But still, without a justification why the slashes should be cleaned up, >> the patch looks like code churn. You can ignore me if it is obvious for >> others why we need the patch. > > Nah, that's a good point. Thanks for clarifying. I'll hold off on > resending until we've reached consensus on this patch. > > The final bugfix patch is the most important one in the series so > perhaps I should reorder the series so that the bugfix comes first, > and these cosmetic improvements and typo fixes are the later ones in > the series? That'll make it so that the bugfix is easier to backport > and not entangled with these prep fixups. > > I see double-slashes when using the dir-diff feature and they just > look wrong to me, but "striving for perfection" is a mere subjective > justification. > > The double-slashes fixup is cosmetic from a technical perspective, but > since the paths are exposed to the diff tools it's a cosmetic blemish > that users will see front and center. > > The test environment where I observed TMPDIR containing a trailing > slash is macOS, so it's a fairly common setup. One justification is > that we should not expose such blemishes to users -- that's what I've > written below but perhaps there's a better way to express it? > > What do you and others think about the proposed message below and > whether or not this patch is worth applying? > > """ > 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. > > Add a unit test to ensure that repeated slashes are not present. > > """ With the justification that the path is user-visible you have my full consent to this patch. It shows a careful eye for the detail. -- Hannes
diff --git a/builtin/difftool.c b/builtin/difftool.c index 4d2e772031..2014a2bb9e 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,17 @@ 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"); + /* Remove trailing slashes when $TMPDIR ends in '/'. */ + while (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') { + strbuf_setlen(&tmpdir, tmpdir.len - 1); + } + 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, '/'); @@ -612,7 +617,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); @@ -642,11 +647,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) @@ -658,6 +663,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 a923f193da..3863afcaac 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -454,6 +454,13 @@ run_dir_diff_test 'difftool --dir-diff' ' grep "^file$" output ' +run_dir_diff_test 'difftool --dir-diff avoids double-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 &&
Use a strbuf to create the buffer used for the dir-diff tmpdir. Strip trailing slashes "/" from the value read from TMPDIR to avoid double-slashes in the calculated paths. Add a unit test to ensure that double-slashes are not present. Signed-off-by: David Aguilar <davvid@gmail.com> --- builtin/difftool.c | 32 +++++++++++++++++++------------- t/t7800-difftool.sh | 7 +++++++ 2 files changed, 26 insertions(+), 13 deletions(-)