From: Jeff King <peff@peff.net>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jonathan Nieder <jrnieder@gmail.com>,
Sergey Organov <sorganov@gmail.com>,
Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file
Date: Tue, 28 Sep 2021 18:29:17 -0400 [thread overview]
Message-ID: <YVOXPTjsp9lrxmS6@coredump.intra.peff.net> (raw)
In-Reply-To: <ed71913886e19ccc276b382de707b4bab7834d12.1630376800.git.gitgitgadget@gmail.com>
On Tue, Aug 31, 2021 at 02:26:35AM +0000, Elijah Newren via GitGitGadget wrote:
> There are several considerations here:
> * We have to pick file(s) where we write these conflict messages too
> * We want to make it as clear as possible that it's not a real file
> but a set of messages about another file
> * We want conflict messages about a file to appear near the file in
> question in a diff, preferably immediately preceding the file in
> question
> * Related to the above, per-file conflict messages are preferred
> over lumping all conflict messages into one big file
>
> To achive the above:
> * We put the conflict messages for $filename in
> $filename[0:-1] + " " + $filename[-1] + ".conflict_msg"
> or, in words, we insert a space before the final character of
> the filename and then also add ".conflict_msg" at the end.
It took me a minute to understand the space thing. I thought at first it
was about avoiding conflicts with existing names (and while it might
help in practice, it's not a guarantee). But I think it's about the
"appear preceding the file" goal. The space sorts before any other
printable character in the final position.
That's...simultaneously clever and gross. My biggest complaint is that
the space looks like a bug in the output.
Using another character like "." might not be too bad, as it's also
fairly early in the ascii table. But it's really this "do it before the
last character" thing that is key to getting the ordering right.
Just brainstorming some alternatives:
- we have diff.orderFile, etc. Could we stuff this data into a less
confusing name (even just "$filename.conflict_msg"), and then provide
a custom ordering to the diff code? I think it could be done by
generating a static ordering ahead of time, but it might even just be
possible to tell diffcore_order() to take the ".conflict_msg"
extension into account in its comparison function.
- there can be other non-diff data between the individual segments. For
example, "patch" will skip over non-diff lines. And certainly in Git
we have our own custom headers. I'm wondering if we could attach
these annotations to the diff-pair somehow, and then show something
like:
diff --git a/foo.c b/foo.c
index 1234abcd..5678cdef 100644
conflict modify/delete foo.c
--- a/foo.c
+++ b/foo.c
@@ some actual diff starts here @@
Obviously such a thing can't really be applied. But then you wouldn't
want to apply the addition of "my.file e.conflict_msg" either.
I dunno. The latter especially is definitely more work, and requires a
bit more cooperation between the merge and diff code. In particular, you
can't just feed a straight tree to the diff anymore. We have to hold
back the annotations, and then apply them to the resulting diff. But I
think the output is much more pleasing to the eye.
-Peff
next prev parent reply other threads:[~2021-09-28 22:29 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-31 2:26 [PATCH 0/7] Add a new --remerge-diff capability to show & log Elijah Newren via GitGitGadget
2021-08-31 2:26 ` [PATCH 1/7] merge-ort: mark a few more conflict messages as omittable Elijah Newren via GitGitGadget
2021-08-31 21:06 ` Junio C Hamano
2021-09-01 0:03 ` Elijah Newren
2021-09-01 17:19 ` Junio C Hamano
2021-08-31 2:26 ` [PATCH 2/7] merge-ort: add ability to record conflict messages in a file Elijah Newren via GitGitGadget
2021-09-28 22:29 ` Jeff King [this message]
2021-09-29 6:25 ` Elijah Newren
2021-09-29 16:14 ` Junio C Hamano
2021-09-29 16:31 ` Elijah Newren
2021-09-30 7:58 ` Jeff King
2021-09-30 8:09 ` Ævar Arnfjörð Bjarmason
2021-10-01 2:07 ` Elijah Newren
2021-10-01 5:28 ` Jeff King
2021-08-31 2:26 ` [PATCH 3/7] ll-merge: add API for capturing warnings in a strbuf instead of stderr Elijah Newren via GitGitGadget
2021-09-28 22:37 ` Jeff King
2021-09-28 23:49 ` Junio C Hamano
2021-09-29 4:03 ` Elijah Newren
2021-08-31 2:26 ` [PATCH 4/7] merge-ort: capture and print ll-merge warnings in our preferred fashion Elijah Newren via GitGitGadget
2021-09-28 22:39 ` Jeff King
2021-09-30 16:53 ` Ævar Arnfjörð Bjarmason
2021-10-01 1:54 ` Elijah Newren
2021-10-01 7:23 ` Ævar Arnfjörð Bjarmason
2021-08-31 2:26 ` [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs Elijah Newren via GitGitGadget
2021-09-28 7:55 ` Ævar Arnfjörð Bjarmason
2021-09-29 4:22 ` Elijah Newren
2021-09-30 7:41 ` Jeff King
2021-09-30 14:17 ` Ævar Arnfjörð Bjarmason
2021-10-01 3:55 ` Elijah Newren
2021-09-28 23:17 ` Jeff King
2021-09-29 4:08 ` Junio C Hamano
2021-09-30 7:33 ` Jeff King
2021-09-30 13:16 ` Ævar Arnfjörð Bjarmason
2021-09-30 21:00 ` Jeff King
2021-10-01 3:11 ` Elijah Newren
2021-10-01 7:30 ` Ævar Arnfjörð Bjarmason
2021-10-01 8:03 ` Elijah Newren
2021-10-01 4:26 ` Elijah Newren
2021-10-01 5:27 ` Jeff King
2021-10-01 7:43 ` Ævar Arnfjörð Bjarmason
2021-09-29 5:05 ` Elijah Newren
2021-09-30 7:26 ` Jeff King
2021-09-30 7:46 ` Jeff King
2021-09-30 20:06 ` Junio C Hamano
2021-10-01 3:59 ` Elijah Newren
2021-10-01 16:36 ` Junio C Hamano
2021-10-01 2:31 ` Elijah Newren
2021-10-01 5:21 ` Jeff King
2021-09-30 19:25 ` Neeraj Singh
2021-09-30 20:19 ` Junio C Hamano
2021-09-30 20:00 ` Junio C Hamano
2021-10-01 2:23 ` Elijah Newren
2021-08-31 2:26 ` [PATCH 6/7] show, log: provide a --remerge-diff capability Elijah Newren via GitGitGadget
2021-08-31 9:19 ` Sergey Organov
2021-09-28 8:01 ` Ævar Arnfjörð Bjarmason
2021-09-29 4:00 ` Elijah Newren
2021-08-31 2:26 ` [PATCH 7/7] doc/diff-options: explain the new --remerge-diff option Elijah Newren via GitGitGadget
2021-08-31 11:05 ` [PATCH 0/7] Add a new --remerge-diff capability to show & log Bagas Sanjaya
2021-08-31 16:16 ` Elijah Newren
2021-08-31 20:03 ` Junio C Hamano
2021-08-31 20:23 ` Elijah Newren
2021-09-01 21:07 ` Junio C Hamano
2021-09-01 21:42 ` Elijah Newren
2021-09-01 21:55 ` Junio C Hamano
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YVOXPTjsp9lrxmS6@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=jrnieder@gmail.com \
--cc=newren@gmail.com \
--cc=sorganov@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.