archive mirror
 help / color / mirror / Atom feed
From: Johannes Schindelin <>
To: Johannes Schindelin via GitGitGadget <>
Cc:, Elijah Newren <>
Subject: Re: [PATCH 1/3] merge-tree -z: always show the original file name first
Date: Fri, 19 Aug 2022 11:17:17 +0200 (CEST)	[thread overview]
Message-ID: <44pr310n-rpr1-0660-0961-386rsq9q11o1@tzk.qr> (raw)
In-Reply-To: <>

Hi all (and I guess in particular Elijah),

On Fri, 19 Aug 2022, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/t/ b/t/
> index 35f94957fce..bc580a242ac 100755
> --- a/t/
> +++ b/t/
> @@ -131,11 +131,12 @@ test_expect_success 'setup non-content conflicts' '
>  test_expect_success 'remerge-diff with non-content conflicts' '
>  	git log -1 --oneline resolution >tmp &&
>  	cat <<-EOF >>tmp &&
> +	diff --git a/file_or_directory b/file_or_directory
> +	remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.
>  	diff --git a/file_or_directory~HASH (side1) b/wanted_content
>  	similarity index 100%
>  	rename from file_or_directory~HASH (side1)
>  	rename to wanted_content
> -	remerge CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.
>  	diff --git a/letters b/letters
>  	remerge CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).
>  	diff --git a/letters_side2 b/letters_side2

I would have liked to avoid touching this file altogether, of course, but
when I investigated, I came to the conclusion that while this patch still
does not produce the best possible outcome for remerge diffs, it does
improve upon the current situation.

The thing is, when mentioning that we had to rename `file_or_directory` to
`file_or_directory~HASH (side1)` to be able to write the file because a
directory of the same name already was in the way, I would actually have
expected this to come under the diff header

	diff --git a/file_or_directory b/file_or_directory~HASH (side1)

Previously, it was under the header

	diff --git a/file_or_directory~HASH (side1) b/wanted_content

and with this patch it is under

	diff --git a/file_or_directory b/file_or_directory

which is still not ideal: It mentions only the original file name, not the
munged one.

When I looked into the implementation details I figured out that the
information is not quite at our fingertips. In
[`create_filepairs_for_header_only_notifications()`][create_filepairs], we
have access to the primary filename (in the above example,
`file_or_directory`) as the key of the entry in the strmap
`o->additional_path_headers`, which is [populated from
`path_messages`][additional-headers], which in turn [points to the
`conflicts` strmap][path_messages]. Note: this code has changed between
v2.37.2 and the current main branch, in v2.37.2 the information is still
copied from `conflicts` to `path_messages`.

So basically, when we generate that diff header, we know the primary path
(great!) and we have access to a strmap that points to a string list of
conflict messages. So from where do we get that munged path name? The
string list of conflict messages [has `util` pointers to `struct
logical_conflict_info` data][conflicts-strmap], which does contain the
list of involved paths. This should be enough, right? But:

- `struct logical_conflict_info` is declared locally in `merge-ort.c`, we
  do not have access to that information in `diff.c` (nor would we really
  _want_ to let that `merge-ort` implementation detail spill over into
  `diff.c`), and even if we _could_ declare it in `merge-ort.h`, there is
  still this problem:

- The path list in `struct logical_conflict_info` is arbitrary-sized, i.e.
  we could even have _three_ paths in there, and that does not fit into a
  standard diff header.

- Additionally, we generate _one_ diff header for _all_ of the conflict
  messages listed for a particular primary path in
  `additional_path_headers`. But each of these conflict messages relates
  to its very own list of non-primary paths. For example, one conflict
  message could talk about a file/directory conflict, another one could
  talk about conflicting renames to two different filenames (in a
  recursive merge, both is possible in the same merge). It is simply
  impossible to combine all of those conflict messages under a single
  diff header that matches all of the involved paths.

- Even aside from that, the remerge diff code seems to have a usability
  wart (or even bug) where restricting the diff via a pathspec that
  matches a secondary path but not the primary path of a conflict (think:
  renames) would _not_ show the conflict message to the user, it would be
  shown only if the pathspec also includes the primary path.

In general, I found it hard to think of a _really_ ideal design where to
present that remerge conflict information. After all, in a remerge diff,
we do not even write out any files, therefore that conflict message
suggesting that we renamed the file to a munged name is somewhat
misleading. Sure, we should mention the conflict between the file and the
directory, but since nothing was written to disk, there is not actually
any _need_ to mention a munged filename at all. And that goes even for the
part of the remerge diff shown in the patch quoted above, where it talks
about `file_or_directory~HASH (side1)` being renamed... That file never
had that filename...

So I am not quite sure where we want the remerge design to go from here.

In any case, this remerge aspect is safely outside the scope of this here
patch series, which means that this here patch series should not be
concerned with it. In this email, I just wanted to mention _why_ I did not
include any patch to address the remerge issues any further.



  reply	other threads:[~2022-08-19  9:17 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-19  6:57 [PATCH 0/3] Show original filenames in merge tree Johannes Schindelin via GitGitGadget
2022-08-19  6:57 ` [PATCH 1/3] merge-tree -z: always show the original file name first Johannes Schindelin via GitGitGadget
2022-08-19  9:17   ` Johannes Schindelin [this message]
2022-08-20 23:22     ` Elijah Newren
2022-08-20 23:17   ` Elijah Newren
2022-08-21  2:00     ` Elijah Newren
2022-08-22 20:12     ` Johannes Schindelin
2022-08-23  6:24       ` Elijah Newren
2022-08-26 15:35         ` Johannes Schindelin
2022-08-30  2:13           ` Elijah Newren
2022-08-19  6:57 ` [PATCH 2/3] merge-tree: show the original file names in the conflict output Johannes Schindelin via GitGitGadget
2022-08-19  6:57 ` [PATCH 3/3] t4301: add a test case involving a rename, type change & modification Johannes Schindelin via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=44pr310n-rpr1-0660-0961-386rsq9q11o1@tzk.qr \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).