Re: [PATCH v2 2/2] submodule: drop the top-level requirement

From: John Keeping <john@keeping.me.uk>
Date: Apr 18, 2013, 14:56 UTC
Message-ID: <20130418145617.GA2278@serenity.lan>
In-Reply-To: <CALkWK0==bnrRJDk1daANoZOOVLwZOPM+YhwOZs7yimoYBoyXyA@mail.gmail.com>

On Thu, Apr 18, 2013 at 08:16:42PM +0530, Ramkumar Ramachandra wrote:

Show 19 quoted lines

> John Keeping wrote:
> > Use the new rev-parse --prefix option to process all paths given to the
> > submodule command, dropping the requirement that it be run from the
> > top-level of the repository.
> 
> Yay!
> 
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 79bfaac..bbf7983 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -112,6 +115,7 @@ resolve_relative_url ()
> >  #
> >  module_list()
> >  {
> > +       eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
> 
> Nit: Why not use "--" to disambiguate between options and arguments,
> like you showed in your intended usage?

Erm... it does.  What's before "$@"?

Ah, you mean after "set". In this case, rev-parse will keep the "--" we supply to it, so we get that from there and do not want to give set an extra one.

Show 8 quoted lines

> So, this essentially converts all the paths specified in "$@" to
> toplevel-relative paths.  Beautiful, as this is practically the only
> change you require in each function.
> 
> > +       sm_path="$wt_prefix$sm_path"
> 
> Wait, isn't sm_path the value of submodule.<name>.path in .gitmodules?
>  Why does it need to be prefixed?

I think you've lost too much context here. This is in cmd_add and at this point sm_path holds the argument supplied by the user, which we must adjust as it will be relative to the current directory.

We should probably be testing for an absolute path here though.

Show 8 quoted lines

> > @@ -942,6 +948,7 @@ cmd_summary() {
> >         fi
> >
> >         cd_to_toplevel
> > +       eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
> 
> Um, what about other functions?  Yeah, it looks like all of them
> except this one call module_list().

Exactly.  Apart from this and cmd_add everything uses module_list.

Show 29 quoted lines

> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> > index ff26535..7795f21 100755
> > --- a/t/t7400-submodule-basic.sh
> > +++ b/t/t7400-submodule-basic.sh
> > @@ -212,6 +212,23 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
> >         test_cmp empty untracked
> >  '
> >
> > +test_expect_success 'submodule add in subdir' '
> > +       echo "refs/heads/master" >expect &&
> > +       >empty &&
> 
> Nit: Use : >empty.  It's clearer.
> 
> > +       (
> > +               mkdir addtest/sub &&
> 
> Why is the mkdir inside the subshell?  The next statement (cd) onwards
> should be in the subshell, to save you cd'ing back.
>
> > +               cd addtest/sub &&
> > +               git submodule add "$submodurl" ../realsubmod3 &&
> > +               git submodule init
> > +       ) &&
> > +
> > +       rm -f heads head untracked &&
> 
> Huh?  What is this in the middle?  The next statement (call to
> inspect) write-truncates them anyway, so this is unnecessary.

I just followed the surrounding tests here. I think it's better for follow the pattern of the surrounding code here than get this one test perfect. A cleanup can follow on top if someone wants to do it.