Latest master failing t7401 submodule tests

Latest master failing t7401 submodule tests

[A Large Angry SCM]

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

Re: Latest master failing t7401 submodule tests

[Junio C Hamano]

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

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

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

Re: Latest master failing t7401 submodule tests

[Jeff King]

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

> A Large Angry SCM <gitzilla@gmail.com> writes:
> 
> > 13 to the 18 tests in t7401 submodule test jig are failing with the
> > latest master this morning.
> 
> 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).

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

Re: Latest master failing t7401 submodule tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I did see it on my debian unstable box with with dash as /bin/sh (though

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

Re: Latest master failing t7401 submodule tests

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > I did see it on my debian unstable box with with dash as /bin/sh (though
> 
> 3deea89 (submodule summary: Don't barf when invoked in an empty repo,
> 2010-02-16) looks broken.  It shifts $1 unconditionally when:

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

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

Yes, definitely a bug.

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

Yes, and I think we do that part right.

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

So I think we just need

diff --git a/git-submodule.sh b/git-submodule.sh
index 1ea4143..bf5ea50 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -556,7 +556,7 @@ cmd_summary() {
 	if rev=$(git rev-parse -q --verify --default HEAD ${1+"$1"})
 	then
 		head=$rev
-		shift
+		test -n "$1" && shift
 	elif test -z "$1" -o "$1" = "HEAD"
 	then
 		return

-Peff

Re: Latest master failing t7401 submodule tests

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> I did see it on my debian unstable box with with dash as /bin/sh (though
>
> 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).

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

diff --git a/git-submodule.sh b/git-submodule.sh
index 5869c00..bd3a8d4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -553,15 +553,12 @@ cmd_summary() {
 
 	test $summary_limit = 0 && return
 
-	if rev=$(git rev-parse -q --verify --default HEAD ${1+"$1"})
+	if test $# != 0 && head=$(git rev-parse -q --verify "$1")
 	then
-		head=$rev
 		shift
-	elif test -z "$1" -o "$1" = "HEAD"
-	then
-		return
 	else
-		head="HEAD"
+		git rev-parse -q --verify HEAD >/dev/null || return 0
+		head=HEAD
 	fi
 
 	if [ -n "$files" ]

Re: Latest master failing t7401 submodule tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  - "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?

Re: Latest master failing t7401 submodule tests

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>>>  - "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);
 

What should "git submodule summary" give before an initial commit?

[Junio C Hamano]

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

diff --git a/git-submodule.sh b/git-submodule.sh
index 5869c00..0397f9d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -556,10 +556,10 @@ cmd_summary() {
 	if rev=$(git rev-parse -q --verify --default HEAD ${1+"$1"})
 	then
 		head=$rev
-		shift
+		test $# = 0 || shift
 	elif test -z "$1" -o "$1" = "HEAD"
 	then
-		return
+		head=4b825dc642cb6eb9a060e54bf8d69288fbee4904
 	else
 		head="HEAD"
 	fi

Re: Latest master failing t7401 submodule tests

[Jeff King]

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:

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

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

Re: What should "git submodule summary" give before an initial commit?

[Johan Herland]

On Wednesday 03 March 2010, Junio C Hamano wrote:
> "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(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 5869c00..0397f9d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -556,10 +556,10 @@ cmd_summary() {
>  	if rev=$(git rev-parse -q --verify --default HEAD ${1+"$1"})
>  	then
>  		head=$rev
> -		shift
> +		test $# = 0 || shift
>  	elif test -z "$1" -o "$1" = "HEAD"
>  	then
> -		return
> +		head=4b825dc642cb6eb9a060e54bf8d69288fbee4904
>  	else
>  		head="HEAD"
>  	fi

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

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: What should "git submodule summary" give before an initial commit?

[Jens Lehmann]

Am 03.03.2010 22:58, schrieb Junio C Hamano:
> "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(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 5869c00..0397f9d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -556,10 +556,10 @@ cmd_summary() {
>  	if rev=$(git rev-parse -q --verify --default HEAD ${1+"$1"})
>  	then
>  		head=$rev
> -		shift
> +		test $# = 0 || shift
>  	elif test -z "$1" -o "$1" = "HEAD"
>  	then
> -		return
> +		head=4b825dc642cb6eb9a060e54bf8d69288fbee4904
>  	else
>  		head="HEAD"
>  	fi

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

diff --git a/wt-status.c b/wt-status.c
index 5807fc3..6769c2e 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -459,11 +459,21 @@ static void wt_status_print_changed(struct wt_status *s)
 	wt_status_print_trailer(s);
 }

-static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitted)
+static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitted, int initial)
 {
 	struct child_process sm_summary;
 	char summary_limit[64];
 	char index[PATH_MAX];
+	const char *ref = NULL;
+	if (!uncommitted) {
+		if (s->amend)
+			ref = "HEAD^";
+		else
+			if (initial)
+				ref = "4b825dc642cb6eb9a060e54bf8d69288fbee4904";
+			else
+				ref = "HEAD";
+	}
 	const char *env[] = { index, NULL };
 	const char *argv[] = {
 		"submodule",
@@ -472,7 +482,7 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 		"--for-status",
 		"--summary-limit",
 		summary_limit,
-		uncommitted ? NULL : (s->amend ? "HEAD^" : "HEAD"),
+		ref,
 		NULL
 	};

@@ -581,8 +591,8 @@ void wt_status_print(struct wt_status *s)
 	wt_status_print_unmerged(s);
 	wt_status_print_changed(s);
 	if (s->submodule_summary) {
-		wt_status_print_submodule_summary(s, 0);  /* staged */
-		wt_status_print_submodule_summary(s, 1);  /* unstaged */
+		wt_status_print_submodule_summary(s, 0, s->is_initial);  /* staged */
+		wt_status_print_submodule_summary(s, 1, 0);  /* unstaged */
 	}
 	if (s->show_untracked_files)
 		wt_status_print_untracked(s);

Re: What should "git submodule summary" give before an initial commit?

[Sverre Rabbelier]

Heya,

On Thu, Mar 4, 2010 at 01:36, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> 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):

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

-- 
Cheers,

Sverre Rabbelier

Re: What should "git submodule summary" give before an initial commit?

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> ... pretending we do have a HEAD that's pointing at the empty tree when on
> an unborn branch might fix 'git reset' ...

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.

Re: What should "git submodule summary" give before an initial commit?

[Sverre Rabbelier]

Heya,

On Thu, Mar 4, 2010 at 07:22, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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?

-- 
Cheers,

Sverre Rabbelier

Re: What should "git submodule summary" give before an initial commit?

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> 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?

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 ;-).

Re: What should "git submodule summary" give before an initial commit?

[Sverre Rabbelier]

Heya,

On Thu, Mar 4, 2010 at 07:43, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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.

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

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 ;).

-- 
Cheers,

Sverre Rabbelier