Re: Weird revision walk behaviour

From: Kevin Bracey <kevin@bracey.fi>
Date: May 27, 2018, 17:37 UTC
Message-ID: <cb1d7c86-a989-300a-01d2-923e9c29e834@bracey.fi>
In-Reply-To: <869a4045-0527-3dcf-33b3-90de2a45cd51@bracey.fi>

On 24/05/2018 23:26, Kevin Bracey wrote:

Show 18 quoted lines

>
>>> On Wed, May 23, 2018 at 07:10:58PM +0200, SZEDER Gábor wrote:
>>>
>>>>    $ git log --oneline master..ba95710a3b -- ci/
>>>>    ea44c0a594 Merge branch 'bw/protocol-v2' into 
>>>> jt/partial-clone-proto-v2
>>>>
> In this case, we're hitting a merge commit which is not on master, but 
> it has two parents which both are. Which, IIRC, means the merge commit 
> is INTERESTING with two UNINTERESTING parents; and we are TREESAME to 
> only one of them.
>
> The commit changing the logic of TREESAME you identified believes that 
> those TREESAME changes for merges which were intended to improve 
> fuller history modes shouldn't affect the simple history "because 
> partially TREESAME merges are turned into normal commits". Clearly 
> that didn't happen here.
>

Haven't currently got a development environment set up here, but I've been looking at the code.Here's a proposal, untested, as a potential starting point if anyone wants to consider a proper patch.

The simplify_history first-scan logic never actually turned merges into simple commits unless they were TREESAME to a relevant/interesting parent. Anything where the TREESAME parent was UNINTERESTING was retained as a merge, but had its TREESAME flag set, and that permitted later simplification.

With the redefinition of the TREESAME flag, this merge commit is no longer TREESAME, and as the decoration logic to refine TREESAME isn't active for simplify_history, it doesn't get cleaned up (even if it would be in full history?)

I think the answer may be to add an extra post-process step on the initial loop to handle this special case. Something like:

         case REV_TREE_SAME:
             if (!revs->simplify_history || !relevant_commit(p)) {
                 /* Even if a merge with an uninteresting
                  * side branch brought the entire change
                  * we are interested in, we do not want
                  * to lose the other branches of this
                  * merge, so we just keep going.
                  */
                 if (ts)
                     ts->treesame[nth_parent] = 1;
+               /* But we note it for potential later simplification */
+               if (!treesame_parent)
+                    treesame_parent = p;
                 continue;
              }

...

After loop:

+ if (relevant_parents == 0 && revs->simplify_history && treesame_parent) { + treesame_parent->next = NULL;// Repeats code from loop - share somehow? + commit->parents = treesame_parent; + commit->object.flags |= TREESAME; + return; + }

      /*
       * TREESAME is straightforward for single-parent commits. For merge

The other option would be to take off the " || !relevant_commit(p)" test, but I'm assuming that is still needed for other cases.

Kevin