Re: [PATCH v2] Let submodule command exit with error status if path does not exist

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Aug 13, 2012, 16:39 UTC
Message-ID: <20120813163911.GA6418@book.hvoigt.net>
In-Reply-To: <7vy5lkfyx1.fsf@alter.siamese.dyndns.org>

Hi Junio,

thanks for such a thorough review.

On Sat, Aug 11, 2012 at 10:43:22PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > I did not know that you prefer a space after the function name. I simply
> > imitated the style from C and there we do not have spaces. It makes the
> > style rules a bit more complicated. Wouldn't it be nicer to have the
> > same as in C so we have less rules?
> 
> I do not think so, as they are different languages.  The call site
> of C functions have name and opening parenthesis without a SP in
> between.  The call site of shell functions do not even have
> parentheses.
> 
> In any case, personal preferences (including mine) do not matter
> much, as there is no "this is scientificly superiour" in styles.

How about I update CodingGuidelines according to the rules you suggested? Then other people know how we prefer bash functions and if statements to look like.

Show 18 quoted lines

> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index aac575e..48014f2 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -109,7 +109,8 @@ resolve_relative_url ()
> >  #
> >  module_list()
> >  {
> > -	git ls-files --error-unmatch --stage -- "$@" |
> > +	(git ls-files --error-unmatch --stage -- "$@" ||
> > +		echo '160000 0000000000000000000000000000000000000000 0	') |
> 
> Is there a reason why the sentinel has to have the same mode pattern
> as a GITLINK entry, NULL SHA-1, stage #0?  Or is the "path" being
> empty all that matters?
> 
> Ah, OK, you did not want to touch the perl script downstream.  I
> would have preferred a comment to document that, i.e.

I only described it in the commit message, sorry. Next time I will add a comment.

Show 12 quoted lines

> > @@ -385,6 +386,10 @@ cmd_foreach()
> >  	module_list |
> >  	while read mode sha1 stage sm_path
> >  	do
> > +		if test -z "$sm_path"; then
> > +			exit 1
> 
> Style:
> 
> 	if test -z "$sm_path"
> 	then
> 		exit 1

See above. If you agree I would add this style to the guidelines.

Show 12 quoted lines

> I know module_list would have said "error: pathspec 'no-such' did
> not match any file(s) known to git.  Did you forget to git add"
> already, but because that comes at the very end of the input to the
> filter written in perl (and with the way the filter is coded, it
> will stay at the end), I am not sure if the user would notice it if
> we exit like this.  By the time we hit this exit, we would have seen
> "Entering $sm_path..." followed by whatever message given while in
> the submodule for all the submodules that comes out of module_list,
> no?
> 
> How about doing it this way to avoid that issue, to make sure we die
> immediately after the typo is diagnosed without touching anything?

I like it, your approach combines the atomicity of my first patch with the efficiency of not calling ls-files twice. I was hesitant to change to much code just to get the exit code right, but your approach looks good to me.

Should I send an updated patch? Or do you just want to squash this in. Since now only the tests are from me what should we do with the ownership?

>  git-submodule.sh | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)

[...]

Cheers Heiko