Re: [RFC/PATCH] Make --full-history consider more merges

Kevin Bracey [off-list ref] writes:

If simplify_history is set, and we do want ancestry, then it doesn't
matter about the TREESAME definition because it shows all merges,
regardless of the TREESAME flag. Thus adding "--parents" to the above
command means it can find it, but only because it drags _every_ merge
into consideration. Should that be necessary?

Futher, if we add simplify_merges, then TREESAME becomes a problem.
After merge simplification, we can be left with a single-parent
commit that doesn't match its parent but is skipped due to its
TREESAME flag being set: it was TREESAME to a parent that got dropped.

I think the correction is that TREESAME for a commit needs to be
redefined to be "this commit matches all its (remaining) parents",
rather than "this commit matches at least 1 of its (remaining)
parents". And when we eliminate parents, we have to check for the
flag changing, but we should have been doing that anyway.

Thanks, I think this analysis is quite correct.  Marking a commit
with TREESAME means the commit is not worth showing.  If we are
simplifying side branches away, remaining parents may be reduced to
1 and "all its remaining parents" would be the same as "at least 1
of its remaining parents", but if we want to keep side branches that
matter in explaining the history, we do not want to paint the merge
with TREESAME unless it matches with all its remaining parents.

If we redefine TREESAME, then recalculation becomes necessary to
handle the "normal" case - the merge will not initially be TREESAME,
as it differs from B and C. But once B and C are eliminated, we
should then determine that the commit is TREESAME from matching
its remaining parent A. Simplification should only ever "increase"
the sameness, so we can avoid recalculation if TREESAME is already
set. Without this recalculation, 6016.7 fails.

So I believe that if reduce_heads() does eliminate any parents, and
TREESAME wasn't set, then we have to recalculate TREESAME, by
running over the remaining parents. A call to
try_to_simplify_commit() does the job, but it feels hacky, and
obviously it's slow. It would be nice if we could remember
"per-parent TREESAME" flags, and shuffle them when we change
parents, but that could be fiddly.

Also if limit_to_ancestry() did eliminate parents from outside the
ancestry, as per the NEEDSWORK comment, then that would also want
to do the TREESAME recalculation. (And it could conceivably be very
useful, helping pick up only the merges that went against your base
commit).

Yeah, I agree to that.

So, given all that, revised patch below:

---

Junio C Hamano [off-list ref] writes:

quoted

So, given all that, revised patch below:

I tried to squeeze the minimum test I sent $gmane/220919 to the test
suite.  I think the "do not use --parents option for this test"
switch needs to be cleaned up a bit more, but it fails without your
patch and does pass with your patch.

I somehow was hoping that your fix to TREESAME semantics would also
correct the known breakage documented in that test, but it seems
that I was too greedy ;-)

Thanks.

 t/t6012-rev-list-simplify.sh | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
index dd6dc84..4e55872 100755
--- a/t/t6012-rev-list-simplify.sh
+++ b/t/t6012-rev-list-simplify.sh

@@ -14,21 +14,24 @@ unnote () {
 
 test_expect_success setup '
 	echo "Hi there" >file &&
-	git add file &&
-	test_tick && git commit -m "Initial file" &&
+	echo "initial" >lost &&
+	git add file lost &&
+	test_tick && git commit -m "Initial file and lost" &&
 	note A &&
 
 	git branch other-branch &&
 
 	echo "Hello" >file &&
-	git add file &&
-	test_tick && git commit -m "Modified file" &&
+	echo "second" >lost &&
+	git add file lost &&
+	test_tick && git commit -m "Modified file and lost" &&
 	note B &&
 
 	git checkout other-branch &&
 
 	echo "Hello" >file &&
-	git add file &&
+	>lost &&
+	git add file lost &&
 	test_tick && git commit -m "Modified the file identically" &&
 	note C &&
 

@@ -37,7 +40,9 @@ test_expect_success setup '
 	test_tick && git commit -m "Add another file" &&
 	note D &&
 
-	test_tick && git merge -m "merge" master &&
+	test_tick &&
+	test_must_fail git merge -m "merge" master &&
+	>lost && git commit -a -m "merge" &&
 	note E &&
 
 	echo "Yet another" >elif &&

@@ -110,4 +115,16 @@ check_result 'I B A' -- file
 check_result 'I B A' --topo-order -- file
 check_result 'H' --first-parent -- another-file
 
+check_result 'E C B A' --full-history E -- lost
+test_expect_success 'full history simplification without parent' '
+	printf "%s\n" E C B A >expect &&
+	git log --pretty="$FMT" --full-history E -- lost |
+	unnote >actual &&
+	sed -e "s/^.*	\([^ ]*\) .*/\1/" >check <actual &&
+	test_cmp expect check || {
+		cat actual
+		false
+	}
+'
+
 test_done

-- 
1.8.2.1-730-gf07461b

On 25/04/2013 04:59, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

quoted

So, given all that, revised patch below:

I tried to squeeze the minimum test I sent $gmane/220919 to the test
suite.  I think the "do not use --parents option for this test"
switch needs to be cleaned up a bit more, but it fails without your
patch and does pass with your patch.

I somehow was hoping that your fix to TREESAME semantics would also
correct the known breakage documented in that test, but it seems
that I was too greedy ;-)

Thanks for the test addition. Maybe we will be able to satisfy your 
greed in this series. There could be more worth doing here, and I think 
getting TREESAME precise is key.

I think I do want to take the step of storing "treesame per parent". And 
once we do that, as well as avoiding the expensive re-diff, we have much 
richer information readily available as a simplification input (and output).

I'm working on a patch that does this - filling in an initial treesame[] 
array as a decoration in try_to_simplify_commit() is easy, and 
maintaining the array through later parent rewrites isn't as onerous as 
I feared - there are only a few places that rewrite parents after the 
initial scan. With a couple of helper functions to do things like 
"delete nth", I think it'll be quite tidy.

I believe that simplify_merges itself needs at least one addition, and 
could use the treesame[] array to do it: if after doing reduce_heads, a 
commit is now different to all remaining parents, but there was a 
TREESAME parent eliminated, that parent should be reinstated. That would 
clearly highlight missed merges, showing both that "older" TREESAME 
parent and the newer !TREESAME parent that would have been taken in a 
normal merge.

And maybe there's more simplify_merges could do, if it had this full 
TREESAME information available.

(But even after you do all this stuff to get the right commits out, we 
then hit a niggle of mine that gitk forces --cc diffs - even if it shows 
shows the offending merge commit, you can't get it to do a diff...)

Kevin