Re: Latest master failing t7401 submodule tests

Junio C Hamano [off-list ref] writes:

Jeff King [off-list ref] writes:

quoted

quoted

 - "git submodule summary path...", defaults to HEAD which is _not_ unborn
   (we shouldn't shift).

I don't think this is a problem. We do:

  git rev-parse -q --verify --default HEAD path

and it correctly reports failure, so we never do the problematic shift.

Stepping back a bit, why do we even special case an unborn branch case in
the first place?  

    rm -fr one && git init one && cd one && git diff HEAD

would diagnose it as an error (we may want to sugarcoat "ambiguous
argument" error message, but that is a tangent).

I may be able to buy "status/diff internally calls submodule summary, and
that codepath needs to special case a submodule on an unborn branch _for
such and such reasons_" if the reasoning is sound, but even if that is the
case, shouldn't that special case be triggered explicitly by the caller of
"submodule summary" with an option?

Continuing to mutter to myself...  I am suspecting that the right solution
to the issue $gmane/140066 raised may be your "dwim-ref fix in 003c6ab
(dwim_ref: fix dangling symref warning, 2010-02-16) and a patch along the
line of the attached (with 3deea89 reverted of course).

We _might_ also want to revert 003c6ab, though it is more or less an
independent issue.

 wt-status.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 5807fc3..1cca3aa 100644
--- a/wt-status.c
+++ b/wt-status.c

@@ -476,6 +476,9 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 		NULL
 	};
 
+	if (s->is_initial && !uncommitted)
+		return;
+
 	sprintf(summary_limit, "%d", s->submodule_summary);
 	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", s->index_file);
 

On Wed, Mar 03, 2010 at 01:28:01PM -0800, Junio C Hamano wrote:

Continuing to mutter to myself...  I am suspecting that the right solution
to the issue $gmane/140066 raised may be your "dwim-ref fix in 003c6ab
(dwim_ref: fix dangling symref warning, 2010-02-16) and a patch along the
line of the attached (with 3deea89 reverted of course).

I am totally clueless about submodules, not having ever actually used
them myself. So I will let others weigh in on whether "git submodule
summary" on an unborn branch makes any sense. But:

  1. _if_ it is not a sensible thing, then your patch below:

Seems like the right thing, to protect git-status users.

  2. If it is sensible, then the hunk we both posted (to check for args
     before shift) makes sense to me. Whether the "compare against empty
     tree" bit makes sense is beyond my submodule cluelessness to
     determine (but it intuitively sounds right to me).

In either case, I think that:

We _might_ also want to revert 003c6ab, though it is more or less an
independent issue.

reverting 003c6ab is not a good idea. As far as I am concerned, it was a
bugfix.

-Peff