Re: [PATCH v5 1/1] mergetool: add automerge configuration

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 24, 2020, 01:36 UTC
Message-ID: <5fe3f083f27cd_7855a20885@natae.notmuch>
In-Reply-To: <xmqqv9cs3uxo.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:

Show 14 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> Ah, I forgot about that one.  I think "the number of conflicts" was
> >> a UI mistake (the original that it mimics is "merge" from RCS suite,
> >> which uses 1 and 2 for "conflicts" and "trouble") but we know we
> >> will get conflicts, so it is wrong to expect success from the
> >> command.  Deliberately ignoring the return status is the right thing
> >> to do.
> >
> > I agree. My bet is that nobody is checking the return status of "git
> > merge-file" to find out the number of conflicts. Plus, how can you check
> > the difference between 255 conflicts and error -1?
> 
> Yup, I already mentioned UI mistake so you do not have to repeat

You said it was a UI mistake, not me. I am a different mind than yours.

This [1] is the first time *you* communicated it was a UI mistake.

This [2] is the first time *I* communicated it was a UI mistake.

I communicated that fact after you, so I did not repeat anything, because I hadn't said that before. *You* did, not *me*.

> it to consume more bandwidth.

This is what is consuming bandwidth.

Not me stating *for the first time* that I agree what you just stated.

You could have skipped what I said *for the first time*, if you didn't find it particularly interesting, and that would have saved bandwidth.

Show 9 quoted lines

> > We could do something like --marker-size=13 to minimize the chances of
> > that happening.
> >
> > In that case I would prefer '/^<\{13\} /' (to avoid too many
> > characters). I see those regexes used elsewhere in git, but I don't know
> > how portable that is.
> 
> If it is used elsewhere with "sed", then that would be OK, but if it
> is not with "sed" but with "grep", that's quite a different story.

In t/t3427-rebase-subtree.sh there is:

  sed -e "s%\([0-9a-f]\{40\} \)files_subtree/%\1%"

Not sure if that counts. There's other places in the tests.

However, I don't see the point if the marker-size is a low enough number, like 7.

Show 12 quoted lines

> > So, do we want those three things?
> >
> >  1. A non-standard marker-size
> >  2. Check beforehand the existence of those markers and disable
> >     automerge
> >  3. Check afterwards the existence of those markers and disable
> >     automerge
> 
> I do not think 3 is needed if we do 2 and I do not think 1 would
> particularly be useful *UNLESS* the code consults with the attribute
> system to see what marker size the path uses to avoid crashing with
> the non-standard marker-size the path already uses.

But what is more likely? a) That the marker-size is 7 (the default), or b) that the marker-size is not the default, but that there's a marker-size attribute *and* the value is precisely 13?

I think a) is way more likely than b).

> So the easiest would be not to do anything for now, with a note
> about known limitations in the doc.  The second easiest would be to
> do 2. alone.  We could do 1. to be more complete but I tend to think
> that it is better to leave it as #leftoverbits.

OK. I think 1. is low-hanging fruit, but I'm fine with not doing anything, or trying 2.

I don't think 2. would be that hard, so I will try that before re-rolling the series.

(unless somebody replies to my other pending arguments)

Cheers.

[1] https://lore.kernel.org/git/xmqqblek8e94.fsf@gitster.c.googlers.com/ [2] https://lore.kernel.org/git/5fe3dd62e12f8_7855a2081f@natae.notmuch/

-- 
Felipe Contreras