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

[John Keeping <john@keeping.me.uk>]

On Thu, Apr 18, 2013 at 08:16:42PM +0530, Ramkumar Ramachandra wrote:
> 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.

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

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

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