Re: [RFC/PATCH 1/3] revision.c: tighten up TREESAME handling of merges

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 28, 2013, 18:38 UTC
Message-ID: <7v61z6sdpz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <517CC9AE.30407@bracey.fi>

Kevin Bracey <kevin@bracey.fi> writes:

Show 6 quoted lines

>> Do we want to discard the decoration data when the commit becomes a
>> non-merge?
>
> Would seem reasonable, and would also help make concrete why we update
> TREESAME immediately, and not in update_treesame(), but I didn't spot
> a mechanism to discard decoration. I'll recheck.

What I meant was a simple "free(it)" and a call to add_decoration() to register a NULL for the object whose treesame[] we no longer need.

Show 6 quoted lines

> I could insert an "if (!commit->object.flags & TREESAME)" test to skip
> the entire update. I'd be inclined to do that as the caller of
> update_treesame(). I think update_treesame() itself should be
> general-purpose without assumptions about what changes have been made,
> so it's a pure treesame[]->TREESAME calculation, without TREESAME as
> an input.

Makes sense.

> (Aside - just occurred to me we could swap the loop for
> "strlen(st->treesame) == st->nparents", if we kept a zero terminator
> in the array. Maybe a bit too smart-ass?)

Probably ;-)

Show 14 quoted lines

>>> +	for (pp = &commit->parents;
>>> +	     (parent = *pp) != NULL;
>>> +	     pp = &parent->next, nth_parent++) {
>> I see the reason to change from while to for is because you wanted
>> to count, and I think it makes sense; but it is more readable to
>> initialise the counter here, too, if that is the case. I.e.
>>
>> 	for (pp = &commit->parents, nth_parent = 0;
>> 	     !(parent = *pp);
>> 	     pp = &parent->next, nth_parent++) {
>
> Agree on nth_parent, but  "!(parent = *pp)"  isn't "(parent = *pp) !=
> NULL", mind. Did you mean "!!"? In which case I still prefer it my
> way.

Droppage of NULL check was a typo.  I only meant A part of for(A;B;C).

Show 14 quoted lines

>>>   @@ -773,6 +861,9 @@ static void limit_to_ancestry(struct
>>> commit_list *bottom, struct commit_list *li
>>>   	 * NEEDSWORK: decide if we want to remove parents that are
>>>   	 * not marked with TMP_MARK from commit->parents for commits
>>>   	 * in the resulting list.  We may not want to do that, though.
>>> +	 *
>>> +	 * Maybe it should be considered if we are TREESAME to such
>>> +	 * parents - now possible with stored per-parent flags.
>>>   	 */
>> Hmm, that is certainly a thought.
>
> My comment's wrong though. Reconsidering, what I think needs removing
> is actually off-ancestry parents that we are !TREESAME to, when we are
> TREESAME on the ancestry path.

I thought I read you meant exactly that, i.e. !TREESAME, but now I re-read what is quoted, you did say "we are TREESAME" ;-). I think I agree with you that we do not want any side branch that is not on the ancestry path we are interested in to affect the sameness assigned to the merge commit.

Show 10 quoted lines

> I've realised while testing this that there's been one thing that's
> confused me repeatedly, and I think this comment was an example of
> it. The example in the rev-list-options manual is wrong.
>
>                   *   *
>           .-A---M---N---O---P
>          /*    /   /*  /*  /*
>         I     B   C   D   E
>          \   /*  /   /*  /
>           `-------------'

I've added '*' next to each arc between a commit-pair whose contents at 'foo' are different to the illustration, following the set-up the manual describes. E is the same as I for 'foo' and P would resolve 'foo' to be the same as O.

Show 6 quoted lines

> Contrary to the manual, merge P is !TREESAME to E (or I).  E's base is
> old enough that E isn't up-to-date w.r.t. "foo". Thus merge "P" is no
> longer TREESAME and does become subject to display with the new
> --full-history:
>
>    I  A  B  N  D  O  P

Assuming N's parents-list is rewritten to be A and B, this sounds sensible.

> I believe this is correct, because P is a merge that determined the
> fate of "foo", so merits --full-history inspection. (--simplify-merges
> obviously knocks P back out again: --simplify-merges becomes more
> important if --full-history gets fuller).

E simplifies to I which is already an ancestor of O and is redundant when inspecting P, hence P becomes a single-parent child of O and further simplifies to O because it has the same 'foo'. Makes sense.

> Given this error, and this change, I think this example may want a
> slight rethink. Do we want a proper "messing with other paths but
> TREESAME merge" example? Say if E's parent was O, P would not be
> TREESAME and not included in --full-history.

I am not sure if I follow your last sentence.

Do you mean this topology, where E's sole parent is O, i.e.

              E
             / \
	N---O---P
           /*
          D

and E does not change 'foo' from O? Then P is TREESAME to all its parents and would not have to appear in the full history for the same reason M does not appear in your earlier IABNDOP output, no?

Show 6 quoted lines

>> OK, even though the use of TMP_MARK (meant to be very localized)
>> across two functions feel somewhat yucky, they are file scope
>> statics next to each other and hopefully are called back to back.
>
> Well, by the end of the series you've got two functions setting it, in
> preparation for later input to this function.

As long as "setting" and "using and then cleaning after done" are localized enough so that nobody will be tempted to insert another complex processing in between that might need TMP_MARK we should be OK.