Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: >> > + 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. >> > + } >> > + >> > + 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 ;-) > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html