Re: [PATCH] Add the --submodule-summary option to the diff option family

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 5, 2009, 19:08 UTC
Message-ID: <7vr5thacb4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.0910051027010.4985@pacific.mpi-cbg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 8 quoted lines

>> > +		if (prepare_revision_walk(&rev))
>> > +			message = "(revision walker failed)";
>> 
>> If prepare_revision_walk() failed for whatever reason, can we trust
>> fast_forward/fast_backward at this point?
>
> No, but it is not used in that case, either, because message is not NULL 
> anymore.

It is used in that case a few lines below to decide if you add the third dot. That's why I asked.

Show 6 quoted lines

>> > +	}
>> > +
>> > +	strbuf_addf(&sb, "Submodule %s %s..", path,
>> > +			find_unique_abbrev(one, DEFAULT_ABBREV));
>> > +	if (!fast_backward && !fast_forward)
>> > +		strbuf_addch(&sb, '.');

> Our output methods translate ANSI, so the strbufs only hold the ANSI 
> sequences.

I'll always trust two Johannes's on Windows matters ;-)

Show 11 quoted lines

> I have no idea why "submodule --summary" uses --first-parent, but 
> personally, I would _hate_ it not to see the merged commits in the diff.
>
> For a summary, you might get away with seeing
>
> 	> Merge bla
> 	> Merge blub
> 	> Merge this
> 	> Merge that
>
> but in a diff that does not cut it at all.

As long as bla/blub/this/that are descriptive enough, I do not see at all why you think "summary" is Ok and "diff" is not. If your response were "it is just a matter of taste; to some people (or project) --first-parent is useful and for others it is not", I would understand it, and it would make sense to use (or not use) --first-parent consistently between this codepath and "submodule --summary", though.

> In any case, just to safe-guard against sick minds, I can add a check that 
> says that left, right, and all the merge bases _cannot_ have any flags 
> set, otherwise we output "(you should visit a psychiatrist)" or some such.

I wouldn't suggest adding such a kludge. Being insulting to the user when we hit a corner case _we_ cannot handle does not help anybody, does it?

I see two saner options. Doing this list walking in a subprocess so that you wouldn't have to worry about object flags at all in this case would certainly be easier; the other option obviously is to have a separate object pool ala libgit2, but that would be a much larger change.