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

Hi,

On Mon, 5 Oct 2009, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

quoted

quoted

+		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.

Well, fair enough.

The answer is: yes, we can still trust fast_forward/fast_backward, as 
there is no question that if the first merge base (which must be the only 
merge base by definition, in this case) is either "left" or "right", it is 
fast_forward or fast_backward, respectively.

So: no worries.

quoted

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.

You may be used to git.git's quality of naming the branches you merge.

Sadly, this is not the common case.

quoted

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?

Well, I was a little exasperated when I wrote that that you want to handle 
that case.

But of course, I should heed Postel's law, and handle the case. Maybe say 
something like "(uses superproject's commits)".

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.

The reason why I insist avoiding a subprocess is performance.  The same 
reason holds for a separate object pool: it would just impede the speed, 
AFAICT.

Besides, I vividly remember what happened to a patch I posted to be able 
to just clear the current object pool.  And I cannot imagine a patch 
introducing a second pool to be any less complicated.

If you really want the case I illustrated (that the submodule actually 
contains commits that already have been shown in the superproject) to be 
handled showing the correct submodule summary (and with --first-parent, I 
think you will agree that it is a summary, even if it is embedded in a 
diff), I could imagine calling a subprocess (for simplicity reasons) _iff_ 
left, right, or any of the merge bases has a flag set.

But I really, really, really want to avoid a fork() in the common case.  I 
do have some users on Windows, and I do have a few submodules in that 
project.  Having too many fork() calls there would just give Git a bad 
reputation.  And it has enough of that, it does not need more.

Ciao,
Dscho