mbox series

[v5,0/1] mergetool: remove unconflicted lines

Message ID 20201223045358.100754-1-felipe.contreras@gmail.com (mailing list archive)
Headers show
Series mergetool: remove unconflicted lines | expand

Message

Felipe Contreras Dec. 23, 2020, 4:53 a.m. UTC
There's not much to say other that what the commit message of the patch says.

Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back.

Changes since v4:

 * Improved commit message with suggestions from Phillip Wood.

Felipe Contreras (1):
  mergetool: add automerge configuration

 Documentation/config/mergetool.txt |  3 +++
 git-mergetool.sh                   | 17 +++++++++++++++++
 t/t7610-mergetool.sh               | 18 ++++++++++++++++++
 3 files changed, 38 insertions(+)

Range-diff:
1:  776c1fbb97 ! 1:  2dc53f4dda mergetool: add automerge configuration
    @@ Metadata
      ## Commit message ##
         mergetool: add automerge configuration
     
    -    It doesn't make sense to display lines without conflicts in the
    -    different views of all mergetools.
    +    The purpose of mergetools is to resolve conflicts when git cannot
    +    automatically do so.
     
    -    Only the lines that warrant conflict markers should be displayed.
    +    In order to do that git has added markers in the specific areas that
    +    need resolving, which the user must manually fix. The tool is supposed
    +    to help with that.
     
    -    Most people would want this behavior on, but in case some don't; add a
    -    new configuration: mergetool.autoMerge.
    +    However, by passing the original BASE, LOCAL, and REMOTE files, many
    +    changes without conflict are presented to the user when in fact nothing
    +    needs to be done for those.
    +
    +    We can fix that by propagating the final version of the file with the
    +    automatic merge to all the panes of the mergetool (BASE, LOCAL, and
    +    REMOTE), and only make them differ on the places where there are actual
    +    conflicts.
    +
    +    As most people will want the new behavior, we enable it by default.
    +    Users that do not want the new behavior can set the new configuration
    +    mergetool.autoMerge to false.
     
         See Seth House's blog post [1] for the idea, and the rationale.

Comments

Junio C Hamano Dec. 24, 2020, 9:24 a.m. UTC | #1
Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's not much to say other that what the commit message of the patch says.
>
> Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back.
>
> Changes since v4:
>
>  * Improved commit message with suggestions from Phillip Wood.
>
> Felipe Contreras (1):
>   mergetool: add automerge configuration

This breakage is possibly a fallout from either this patch or
1e2ae142 (t7[5-9]*: adjust the references to the default branch name
"main", 2020-11-18).

  https://github.com/git/git/runs/1602803804#step:7:10358

I cannot quite tell how the two strings compared with 'test' on
output line 10355 are different in the output, though.

Thanks.
Felipe Contreras Dec. 24, 2020, 4:16 p.m. UTC | #2
Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > There's not much to say other that what the commit message of the patch says.
> >
> > Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back.
> >
> > Changes since v4:
> >
> >  * Improved commit message with suggestions from Phillip Wood.
> >
> > Felipe Contreras (1):
> >   mergetool: add automerge configuration
> 
> This breakage is possibly a fallout from either this patch or
> 1e2ae142 (t7[5-9]*: adjust the references to the default branch name
> "main", 2020-11-18).
> 
>   https://github.com/git/git/runs/1602803804#step:7:10358

It seems likely it's the mergetool patch.

This regex '/^=======\r\?$/' is supposed to handle the crlf situation.

I can't imagine what would be different in Windows regarding that
situation.
Johannes Schindelin Dec. 30, 2020, 5:47 a.m. UTC | #3
Hi Junio,

On Thu, 24 Dec 2020, Junio C Hamano wrote:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > There's not much to say other that what the commit message of the patch says.
> >
> > Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back.
> >
> > Changes since v4:
> >
> >  * Improved commit message with suggestions from Phillip Wood.
> >
> > Felipe Contreras (1):
> >   mergetool: add automerge configuration
>
> This breakage is possibly a fallout from either this patch or
> 1e2ae142 (t7[5-9]*: adjust the references to the default branch name
> "main", 2020-11-18).
>
>   https://github.com/git/git/runs/1602803804#step:7:10358
>
> I cannot quite tell how the two strings compared with 'test' on
> output line 10355 are different in the output, though.

I spent more time than I cared to spend on this, and still have not quite
figured out what is the fault, but I can state with conviction that the
problem is not even introduced by any merge into `seen`. The
`fc/mergetool-automerge` branch itself is already broken:
https://github.com/gitgitgadget/git/actions/runs/441233234

Ciao,
Dscho
Felipe Contreras Dec. 30, 2020, 10:33 p.m. UTC | #4
Johannes Schindelin wrote:
> On Thu, 24 Dec 2020, Junio C Hamano wrote:
> > This breakage is possibly a fallout from either this patch or
> > 1e2ae142 (t7[5-9]*: adjust the references to the default branch name
> > "main", 2020-11-18).
> >
> >   https://github.com/git/git/runs/1602803804#step:7:10358
> >
> > I cannot quite tell how the two strings compared with 'test' on
> > output line 10355 are different in the output, though.
> 
> I spent more time than I cared to spend on this, and still have not quite
> figured out what is the fault, but I can state with conviction that the
> problem is not even introduced by any merge into `seen`. The
> `fc/mergetool-automerge` branch itself is already broken:
> https://github.com/gitgitgadget/git/actions/runs/441233234

Yes, if you didn't have me blocked and read what I said a week ago in
[1], you would have saved yourself that time.

Seth House has claimed the patch series though.

[1] https://lore.kernel.org/git/5fe4bec2da21a_19c92085f@natae.notmuch/