* Re: Commit dates on conflict markers
2023-09-11 23:31 ` Junio C Hamano
@ 2023-09-12 8:57 ` Jeff King
2023-09-12 14:33 ` Elijah Newren
1 sibling, 0 replies; 4+ messages in thread
From: Jeff King @ 2023-09-12 8:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Roger Light, git, Elijah Newren
On Mon, Sep 11, 2023 at 04:31:06PM -0700, Junio C Hamano wrote:
> A change that only shows the commit date without allowing end user
> configuration will *not* be worth doing, but allowing them to use
> placeholders like '%h %s' in "git log --format='%h %s'" (check
> pretty.c for the catalog) would be a good exercise; it should not
> take somebody with an ultra-deep knowledge of how the code works.
FWIW, I had the same thought. But I wasn't quite sure where we even set
these strings, so here are a few thoughts for anybody who wants to work
on it:
- the relevant strings are passed in to ll_merge(); you can grep for
callers and see that there are a number of strings which end up here
(based on what info we actually have).
The most interesting one to start with is probably the call in
merge_3way() of merge-ort.c.
- there we have three object_ids, "o", "a", and "b" representing the
three sides. But these are just blobs. We have strings "ancestor",
"branch1", and "branch2" in merge_options, but we would probably not
want to re-resolve those names. So probably some extra fields need
to go into merge_options.
- I'm not sure if each of those endpoints is always a commit. For a
regular merge, they would be. But in a recursive merge, we'd
sometimes create an intermediate virtual tree. So we'd need to
handle the case that there is no commit (and either fall back to a
more vanilla string, or make a fake commit with reasonable details).
- The current labels are based on the "original" ref names (which I
think are really "what was handed to merge"; so it might be
"master~13" etc) along with the blob path (if it is not the same for
both endpoints). So you'd want more than just passing the format
string to format_commit_message(). You would want an extra
placeholder to represent those values (either ref and pathname
individually, or possibly a single placeholder for "ref and maybe
pathname if it's interesting").
The least-bad way to do that is perhaps to expand the format twice:
once for the special placeholder (quoting any "%" found in the
filename, etc), and then feeding the whole result to the
pretty-print formatter.
I was hoping this might be only a few-line change, but I actually think
it's a bit more complicated than that. But still maybe not _too_ bad.
-Peff
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Commit dates on conflict markers
2023-09-11 23:31 ` Junio C Hamano
2023-09-12 8:57 ` Jeff King
@ 2023-09-12 14:33 ` Elijah Newren
1 sibling, 0 replies; 4+ messages in thread
From: Elijah Newren @ 2023-09-12 14:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Roger Light, git
On Mon, Sep 11, 2023 at 4:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Roger Light <roger@atchoo.org> writes:
>
> > When I carry out a merge with conflicts, it's not always clear when
> > resolving the conflicts which is the correct part of code to use. I
> > sometimes use git blame to guide me as to the age of the different
> > chunks of code and hence what to choose.
> >
> > I was wondering if there might be a way to help include that sort of
> > information directly into the conflict.
> >
> > If you had a single line conflict it would be straightforward to
> > display by including the date the line was last modified alongside the
> > conflict marker:
> >
> > <<<<<<< HEAD date:yesterday
> > print("please")
> > ======= date:10 years ago
> > print("help")
> > >>>>>>> main
> >
> > With a more realistic change with multiple lines and context from
> > different commits, it's not immediately obvious to me that it's
> > possible to do in a way that isn't completely horrible.
>
> Our conflict marker lines do get human readable labels but the
> format used by merge_3way() both in merge-ort and merge-recursive
> backends is hardcoded to be <branchname> ':' <pathname> and it is
> sufficient to let you tell which commit involved in the merge and
> which path in that commit the contents came from.
>
> A change that only shows the commit date without allowing end user
> configuration will *not* be worth doing, but allowing them to use
> placeholders like '%h %s' in "git log --format='%h %s'" (check
> pretty.c for the catalog) would be a good exercise; it should not
> take somebody with an ultra-deep knowledge of how the code works.
Generalizing conflict marker annotations to use other hints may make
sense, but I am not sure that "date" is a good example or reason to
generalize it, for three reasons:
* [No date] Virtual merge bases from recursive merges do not have a
date to associate with them. Do we just make one up? Average the
range of dates of the commits being merged to create the virtual merge
base?
* [Wrong date(s)] The date Roger probably wants is the date when the
conflicting text was introduced to the given side of history, not the
date of the tip of that branch. merge-ort is pretty fundamentally a
three-way merge algorithm, meaning it only ever uses the merge-base
and the two branch tips. I don't want to see that changed either;
other than for computing the merge base, I'm very skeptical of any
movement to make merge-ort depend upon any intermediate commit(s).
Such changes would likely be better placed in an entirely new merge
algorithm, but then you have to write an entirely new merge algorithm,
which is decidedly non-trivial.
* [Too many dates] What if the conflict region had two lines on one
side and each line was added on different dates -- what then to
display for the date for that side of history? The earliest? The
latest? Some kind of weighted average? Feels like a bug report
generator to me.
So, if we generalize conflict marker annotations, we probably ought to
omit "date" from the new possibilities.
^ permalink raw reply [flat|nested] 4+ messages in thread