[v4,0/3] difftool dir-diff symlink bug fix and cleanup patches

Message

David Aguilar Sept. 19, 2021, 8:38 p.m. UTC

This patch series fixes a regression in difftool that can lead to data loss.
The symlink-file writing in difftool's dir-diff mode has been fixed to no
longer write-through to the symlink targets.

Please consider patching older Git versions with the fix from 1/3.

Changes since last time:
- This series has been reordered so that 1/3 is the patch that fixes the bug.
  The subsequent patches are cleanup.
- Patch 1/4 from before, which removed the tmp test repos, has been dropped.
- Patch 1/3 was updated to not remove its tmp test repo.
- Patch 1/3 was updated to consistently use "echo" in its tests.
- Patch 1/3 was updated to fix a "syminks" -> "symlinks" test comment typo.
- Patch 2/3 was reworded to improve the repeated slashes justification.
- Patch 3/3 "add a missing space to the ... comments" is unchanged.

David Aguilar (3):
  difftool: fix symlink-file writing in dir-diff mode
  difftool: use a strbuf to create a tmpdir path without double-slashes
  difftool: add a missing space to the run_dir_diff() comments

 builtin/difftool.c  | 32 ++++++++++---------
 t/t7800-difftool.sh | 75 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 91 insertions(+), 16 deletions(-)

Comments

Junio C Hamano Sept. 20, 2021, 6:39 p.m. UTC | #1

David Aguilar <[e-mail]> writes:

> This patch series fixes a regression in difftool that can lead to data loss.
> The symlink-file writing in difftool's dir-diff mode has been fixed to no
> longer write-through to the symlink targets.

v4 with no backreference?  Do you have a message-id for the previous
three rounds handy?

> Please consider patching older Git versions with the fix from 1/3.

meaing 1/3 would be done on top of maint (v2.33.0), and the other
two can be on a separate branch that starts on a commit that is a
merge of the 1/3's branch into 'master'?

Thanks.

David Aguilar Sept. 20, 2021, 9:43 p.m. UTC | #2

On Mon, Sep 20, 2021 at 11:39 AM Junio C Hamano <[e-mail]> wrote:
>
> David Aguilar <[e-mail]> writes:
>
> > This patch series fixes a regression in difftool that can lead to data loss.
> > The symlink-file writing in difftool's dir-diff mode has been fixed to no
> > longer write-through to the symlink targets.
>
> v4 with no backreference?  Do you have a message-id for the previous
> three rounds handy?

<[e-mail]>

v1 was dead on arrival so v2 was really the first. There was no v3 --
a different patch in the series went to v3 so I bumped the entire
series to v4.


> > Please consider patching older Git versions with the fix from 1/3.
>
> meaing 1/3 would be done on top of maint (v2.33.0), and the other
> two can be on a separate branch that starts on a commit that is a
> merge of the 1/3's branch into 'master'?
>
> Thanks.

Thanks. Patch 3/3 seems trivially correct so I won't resend that either.

Ævar had notes about 2/3 which can be split off to a separate topic,
so that's the only one I'll plan to resend all by itself --in-reply-to
that thread.

I see that it's already in "seen". I can send a replacement patch
shortly if that's not too much trouble; dropping "strbuf" from the
subject line in the commit message and the note about the test would
be good to cleanup. The strbuf leaks will be addressed in the
replacement patch.
--
David

Junio C Hamano Sept. 22, 2021, 6:43 p.m. UTC | #3

David Aguilar <[e-mail]> writes:


> Thanks. Patch 3/3 seems trivially correct so I won't resend that either.
>
> Ævar had notes about 2/3 which can be split off to a separate topic,
> so that's the only one I'll plan to resend all by itself --in-reply-to
> that thread.

Please don't do this.  Your topic is not the only one I am looking
at, and being able to find the whole thing together is always more
efficient than having to go back, re-read the discussion and pick
the latest iteration of each step individually (which will not work
if the topic needs reordering of the patches in it anyway).

> I see that it's already in "seen". I can send a replacement patch
> shortly ...

Please do not read anything in a topic being (or not being, for that
matter) "seen".  It means "the maintainer has seen it and thought it
might be interesting at one point of time in the past" and nothing
more.  It certainly does not mean the maintainer is keeping track of
how the topic is evolving and knows which piece will be replaced and
which piece is already done.

Thanks.