Patchwork [2,of,5] merge: show list of bids for each file in bid-merge in ui.note()

login
register
mail settings
Submitter Pulkit Goyal
Date Sept. 11, 2020, 7:05 a.m.
Message ID <76753724e6b5453d9540.1599807930@workspace>
Download mbox | patch
Permalink /patch/47131/
State Accepted
Headers show

Comments

Pulkit Goyal - Sept. 11, 2020, 7:05 a.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1598442653 -19800
#      Wed Aug 26 17:20:53 2020 +0530
# Node ID 76753724e6b5453d9540c73b1a1a6f11de818e46
# Parent  c196dc7f6cf18e32a8646ae7cd0b08014f8ffbdf
# EXP-Topic merge-newnode
merge: show list of bids for each file in bid-merge in ui.note()

Earlier, we were showing the list of bids only when we were ambiguously picking.
However, the cases where we unambiguously picked a side may not always be
correct and need to be fixed.

Having list of bids for all files will be helpful in debugging.

Differential Revision: https://phab.mercurial-scm.org/D8966
Yuya Nishihara - Sept. 12, 2020, 3 a.m.
On Fri, 11 Sep 2020 12:35:30 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1598442653 -19800
> #      Wed Aug 26 17:20:53 2020 +0530
> # Node ID 76753724e6b5453d9540c73b1a1a6f11de818e46
> # Parent  c196dc7f6cf18e32a8646ae7cd0b08014f8ffbdf
> # EXP-Topic merge-newnode
> merge: show list of bids for each file in bid-merge in ui.note()
> 
> Earlier, we were showing the list of bids only when we were ambiguously picking.
> However, the cases where we unambiguously picked a side may not always be
> correct and need to be fixed.
> 
> Having list of bids for all files will be helpful in debugging.

Will experienced users can/have to understand the output? If it's just for
debugging, I prefer not seeing it unless necessary.

>    auction for merging merge bids
> -   f1: picking 'get' action
> -   f2: picking 'keep' action
> +   f1:
> +     list of bids:
> +       remote is newer -> g
> +       versions differ -> m
> +     picking 'get' action

Nit: if it's not a debug output, maybe it can be more readable by saying
the action first.

  f1: picking 'get' action
    list of bids:
      ...
Pulkit Goyal - Sept. 14, 2020, 8:14 a.m.
On Sat, Sep 12, 2020 at 8:31 AM Yuya Nishihara <yuya@tcha.org> wrote:
>
> On Fri, 11 Sep 2020 12:35:30 +0530, Pulkit Goyal wrote:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1598442653 -19800
> > #      Wed Aug 26 17:20:53 2020 +0530
> > # Node ID 76753724e6b5453d9540c73b1a1a6f11de818e46
> > # Parent  c196dc7f6cf18e32a8646ae7cd0b08014f8ffbdf
> > # EXP-Topic merge-newnode
> > merge: show list of bids for each file in bid-merge in ui.note()
> >
> > Earlier, we were showing the list of bids only when we were ambiguously picking.
> > However, the cases where we unambiguously picked a side may not always be
> > correct and need to be fixed.
> >
> > Having list of bids for all files will be helpful in debugging.
>
> Will experienced users can/have to understand the output? If it's just for
> debugging, I prefer not seeing it unless necessary.

It's for debugging purposes only. Right now, a lot of times the merge
code does not add an action for an ancestor. This leads to bid merge
doing the wrong thing.
This and showing number of ancestors in this debug output is meant to
track down such cases where `manifestmerge()` is missing adding action
for a file for some ancestors.
>
> >    auction for merging merge bids
> > -   f1: picking 'get' action
> > -   f2: picking 'keep' action
> > +   f1:
> > +     list of bids:
> > +       remote is newer -> g
> > +       versions differ -> m
> > +     picking 'get' action
>
> Nit: if it's not a debug output, maybe it can be more readable by saying
> the action first.
>
>   f1: picking 'get' action
>     list of bids:
>       ...
Yuya Nishihara - Sept. 14, 2020, 10:44 a.m.
On Mon, 14 Sep 2020 13:44:37 +0530, Pulkit Goyal wrote:
> On Sat, Sep 12, 2020 at 8:31 AM Yuya Nishihara <yuya@tcha.org> wrote:
> >
> > On Fri, 11 Sep 2020 12:35:30 +0530, Pulkit Goyal wrote:
> > > # HG changeset patch
> > > # User Pulkit Goyal <7895pulkit@gmail.com>
> > > # Date 1598442653 -19800
> > > #      Wed Aug 26 17:20:53 2020 +0530
> > > # Node ID 76753724e6b5453d9540c73b1a1a6f11de818e46
> > > # Parent  c196dc7f6cf18e32a8646ae7cd0b08014f8ffbdf
> > > # EXP-Topic merge-newnode
> > > merge: show list of bids for each file in bid-merge in ui.note()
> > >
> > > Earlier, we were showing the list of bids only when we were ambiguously picking.
> > > However, the cases where we unambiguously picked a side may not always be
> > > correct and need to be fixed.
> > >
> > > Having list of bids for all files will be helpful in debugging.
> >
> > Will experienced users can/have to understand the output? If it's just for
> > debugging, I prefer not seeing it unless necessary.
> 
> It's for debugging purposes only. Right now, a lot of times the merge
> code does not add an action for an ancestor. This leads to bid merge
> doing the wrong thing.
> This and showing number of ancestors in this debug output is meant to
> track down such cases where `manifestmerge()` is missing adding action
> for a file for some ancestors.

Okay, then let's switch to ui.debug() and leave the ui.note() messages
unmodified.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1154,37 +1154,35 @@  def calculateupdates(
         repo.ui.note(_(b'\nauction for merging merge bids\n'))
         mresult = mergeresult()
         for f, bids in sorted(fbids.items()):
+            repo.ui.note(_(b" %s:\n   list of bids:\n") % f)
+            for m, l in sorted(bids.items()):
+                for _f, args, msg in l:
+                    repo.ui.note(b'     %s -> %s\n' % (msg, m))
             # bids is a mapping from action method to list af actions
             # Consensus?
             if len(bids) == 1:  # all bids are the same kind of method
                 m, l = list(bids.items())[0]
                 if all(a == l[0] for a in l[1:]):  # len(bids) is > 1
-                    repo.ui.note(_(b" %s: consensus for %s\n") % (f, m))
+                    repo.ui.note(_(b"   consensus for %s\n") % (m))
                     mresult.addfile(f, *l[0])
                     continue
             # If keep is an option, just do it.
             if mergestatemod.ACTION_KEEP in bids:
-                repo.ui.note(_(b" %s: picking 'keep' action\n") % f)
+                repo.ui.note(_(b"   picking 'keep' action\n"))
                 mresult.addfile(f, *bids[mergestatemod.ACTION_KEEP][0])
                 continue
             # If there are gets and they all agree [how could they not?], do it.
             if mergestatemod.ACTION_GET in bids:
                 ga0 = bids[mergestatemod.ACTION_GET][0]
                 if all(a == ga0 for a in bids[mergestatemod.ACTION_GET][1:]):
-                    repo.ui.note(_(b" %s: picking 'get' action\n") % f)
+                    repo.ui.note(_(b"   picking 'get' action\n"))
                     mresult.addfile(f, *ga0)
                     continue
             # TODO: Consider other simple actions such as mode changes
             # Handle inefficient democrazy.
-            repo.ui.note(_(b' %s: multiple bids for merge action:\n') % f)
-            for m, l in sorted(bids.items()):
-                for _f, args, msg in l:
-                    repo.ui.note(b'  %s -> %s\n' % (msg, m))
             # Pick random action. TODO: Instead, prompt user when resolving
             m, l = list(bids.items())[0]
-            repo.ui.warn(
-                _(b' %s: ambiguous merge - picked %s action\n') % (f, m)
-            )
+            repo.ui.warn(_(b'   ambiguous merge - picked %s action\n') % (m))
             mresult.addfile(f, *l[0])
             continue
         repo.ui.note(_(b'end of auction\n\n'))
diff --git a/tests/test-merge-criss-cross.t b/tests/test-merge-criss-cross.t
--- a/tests/test-merge-criss-cross.t
+++ b/tests/test-merge-criss-cross.t
@@ -150,8 +150,16 @@  Redo merge with merge.preferancestor="*"
    f2: remote unchanged -> k
   
   auction for merging merge bids
-   f1: picking 'get' action
-   f2: picking 'keep' action
+   f1:
+     list of bids:
+       remote is newer -> g
+       versions differ -> m
+     picking 'get' action
+   f2:
+     list of bids:
+       remote unchanged -> k
+       versions differ -> m
+     picking 'keep' action
   end of auction
   
    f1: remote is newer -> g
@@ -193,8 +201,16 @@  The other way around:
    f2: remote is newer -> g
   
   auction for merging merge bids
-   f1: picking 'keep' action
-   f2: picking 'get' action
+   f1:
+     list of bids:
+       remote unchanged -> k
+       versions differ -> m
+     picking 'keep' action
+   f2:
+     list of bids:
+       remote is newer -> g
+       versions differ -> m
+     picking 'get' action
   end of auction
   
    f2: remote is newer -> g
@@ -231,8 +247,16 @@  Verify how the output looks and and how 
   resolving manifests
   
   auction for merging merge bids
-   f1: picking 'get' action
-   f2: picking 'keep' action
+   f1:
+     list of bids:
+       remote is newer -> g
+       versions differ -> m
+     picking 'get' action
+   f2:
+     list of bids:
+       remote unchanged -> k
+       versions differ -> m
+     picking 'keep' action
   end of auction
   
   getting f1
@@ -258,8 +282,16 @@  Verify how the output looks and and how 
    f2: remote unchanged -> k
   
   auction for merging merge bids
-   f1: picking 'get' action
-   f2: picking 'keep' action
+   f1:
+     list of bids:
+       remote is newer -> g
+       versions differ -> m
+     picking 'get' action
+   f2:
+     list of bids:
+       remote unchanged -> k
+       versions differ -> m
+     picking 'keep' action
   end of auction
   
    f1: remote is newer -> g
@@ -344,10 +376,11 @@  http://stackoverflow.com/questions/93500
   resolving manifests
   
   auction for merging merge bids
-   x: multiple bids for merge action:
-    versions differ -> m
-    versions differ -> m
-   x: ambiguous merge - picked m action
+   x:
+     list of bids:
+       versions differ -> m
+       versions differ -> m
+     ambiguous merge - picked m action
   end of auction
   
   merging x
@@ -431,9 +464,19 @@  Verify that the old context ancestor wor
    d2/b: remote created -> g
   
   auction for merging merge bids
-   d1/a: consensus for r
-   d1/b: consensus for r
-   d2/b: consensus for g
+   d1/a:
+     list of bids:
+       other deleted -> r
+     consensus for r
+   d1/b:
+     list of bids:
+       other deleted -> r
+     consensus for r
+   d2/b:
+     list of bids:
+       remote created -> g
+       remote created -> g
+     consensus for g
   end of auction
   
    d1/a: other deleted -> r