Latest master failing t7401 submodule tests

13 to the 18 tests in t7401 submodule test jig are failing with the latest master this morning.

A Large Angry SCM <gitzilla@gmail.com> writes:

Do you mean v1.7.0.1-138-ga75bab5?

I don't see it with Debian 5.0.4 on x86_64 (my primary box), Fedora 11 nor FreeBSD 8.0 (the latter two are kvm sandboxes pretending to be i686).

On Wed, Mar 03, 2010 at 11:45:19AM -0800, Junio C Hamano wrote:

I did see it on my debian unstable box with with dash as /bin/sh (though IIRC, isn't FreeBSD /bin/sh ash?). It looked like a "shift: can't shift that many" error, but I haven't had time to track it down further yet (I'm installing lighting in a 2-year-old's play kitchen. ;) ).

Maybe the shell selection will give somebody lead. If not, I'll try to look at it again in a few hours.

-Peff

Jeff King <peff@peff.net> writes:

3deea89 (submodule summary: Don't barf when invoked in an empty repo, 2010-02-16) looks broken. It shifts $1 unconditionally when:

 - "git submodule summary", no arguments given and defaults to HEAD which
   is _not_ unborn (we shouldn't shift in this case);

 - "git submodule summary HEAD path...", which is not unborn (we should shift);

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

On Wed, Mar 03, 2010 at 12:32:11PM -0800, Junio C Hamano wrote:

Yeah, I can confirm that the breakage was introduced there (and it looks like for bash, an extra "shift" is a failing command but doesn't make the shell barf, but for dash it is more serious, which explains why tests passed for you).

Yes, definitely a bug.

Yes, and I think we do that part right.

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.

So I think we just need

Jeff King <peff@peff.net> writes:

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?

Junio C Hamano <gitster@pobox.com> writes:

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(-)

"git status" collects the changes for both the index (since HEAD) and the working tree files (since the index), summarizes and shows them. When it is run before the first commit is made, it uses a logic different from the one used in the normal case to gather the information on the index, as we don't have HEAD yet, i.e. instead of "diff-index HEAD", we would run "diff-index emtpy-tree".

How should status.submodulesummary integrate into this framework?

Currently, it blindly runs "git submodule summary", and it gives an extra error message about HEAD not being a commit, and people (me included) misguidedly have spent time on squelching the message.

But I think that was _all wrong_. I do not think "git submodule summary" should fail even when you haven't made your first commit.

If you are before the first commit, we say everything you have in the index is a change you are adding with your next commit (which will be your initial one). If you added a submodule commit to the index, shouldn't "git submodule summary" say "you'll be committing the addition of this subproject"? IOW, shouldn't we be comparing an empty tree to find added submodules, like this, when we haven't made the first commit?

 git-submodule.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

On Wednesday 03 March 2010, Junio C Hamano wrote:

Acked-by: Johan Herland <johan@herland.net>

If you're planning to revert 3deea89 (although the above patch suggests you're not), then please don't revert the t7401 testcases added by that commit. The testcase is useful in any case.

...Johan

Am 03.03.2010 22:58, schrieb Junio C Hamano:

Acked-by: Jens Lehmann <Jens.Lehmann@web.de>

Your patch fixes "git submodule summary" in a freshly created repo for me.

But to make "git status" with status.submodulesummary work as expected, i need something like the following patch on top of current pu (because "git submodule summary --cached HEAD" returns no changes in a freshly created repo):

Heya,

On Thu, Mar 4, 2010 at 01:36, Jens Lehmann <Jens.Lehmann@web.de> wrote:

I think the problem is deeper than that: git doesn't work that great if there's no HEAD yet. Some of our features don't work (as expected) if there's no HEAD. For example, 'git reset' doesn't work, of course, I can use 'git rm --cached .' to achieve the same effect (since all staged changes are guaranteed to be adds), but its' not quite the same. Perhaps it's an idea to look at how we handle not having a head across the board and deal with it at a higher level? Something like pretending we do have a HEAD that's pointing at the empty tree when on an unborn branch might fix 'git reset' (although I'm sure there's all kinds of objections to actually doing that, I'm not suggesting that's what we should do, but that's the kind of solution I think we should look at).

Sverre Rabbelier <srabbelier@gmail.com> writes:

I think "git reset HEAD" _should_ fail, but on the other hand, "git reset" and "git reset paths..." would be Ok before HEAD points at a commit.

Heya,

On Thu, Mar 4, 2010 at 07:22, Junio C Hamano <gitster@pobox.com> wrote:

Yes, that's what I came up with after sending my email, commands that _default_ to HEAD as revision should instead default to an empty commit when there is no HEAD yet. Perhaps something we can add to parse_opt and then make sure that all commands we care about use that feature?

Sverre Rabbelier <srabbelier@gmail.com> writes:

I doubt it would be very much useful as a general feature, as sometimes we default to HEAD because we want a tree-ish (in which case using "empty tree" would be appropriate) and other times because we want an actual commit (e.g. "git commit --amend" should exit, not "amend an empty tree") and we would need to be careful to study each codepath and use the appropriate one.

But you are welcome to try and prove me wrong with a working patch ;-).

Heya,

On Thu, Mar 4, 2010 at 07:43, Junio C Hamano <gitster@pobox.com> wrote:

Yes, I think if we add such a "default to HEAD or empty tree" option to parse_opt we shouldn't just plug it in anywhere we default to HEAD.

Ah, I was hoping perhaps one of our still-in-hiding prospective gsoc students might feel up to the task, I am short on free time lately (I barely have enough time for Melange), the little git time I do have is spent on adding write support to git-remote-hg. So! To all you people that want to be a gsoc student for git, now's your "opportune moment" to impress us ;).

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

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:

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

-Peff

Junio C Hamano <gitster@pobox.com> writes:

IOW, shouldn't the code look more like this?

	if test $# != 0 && head=$(git rev-parse -q --verify "$1")
	then
		shift
	else
		git rev-parse -q --verify HEAD >/dev/null || return 0
		head=HEAD
	fi

That is, if a specific version might be there, we see if it is a version and shift it out only if that is the case. Otherwise we default to HEAD but special case for an unborn HEAD (and report success).

 git-submodule.sh |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)