Re: [PATCH v7 2/3] submodule update: add --remote for submodule's upstream changes

["W. Trevor King" <wking@tremily.us>]

[-- Attachment #1: Type: text/plain, Size: 4633 bytes --]

On Wed, Dec 12, 2012 at 12:43:23PM -0500, Phil Hord wrote:
> On Tue, Dec 11, 2012 at 1:58 PM, W. Trevor King <wking@tremily.us> wrote:
> > diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> > …
> > +--remote::
> > [snip some --remote documentation]
> > +In order to ensure a current tracking branch state, `update --remote`
> > +fetches the submodule's remote repository before calculating the
> > +SHA-1.  This makes `submodule update --remote --merge` similar to
> > +running `git pull` in the submodule.  If you don't want to fetch (for
> > +something closer to `git merge`), you should use `submodule update
> > +--remote --no-fetch --merge`.
> 
> I assume the same can be said for 'submodue update --remote --rebase',
> right?

Yes.

> I wonder if this can be made merge/rebase-agnostic.  Is it still
> true if I word it like this?:
> 
>    In order to ensure a current tracking branch state, `update --remote`
>    fetches the submodule's remote repository before calculating the
>    SHA-1.  If you don't want to fetch, you should use `submodule update
>     --remote --no-fetch`.

Works for me.  Will change in v8 (which I'll base on 'master').

> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index f969f28..1395079 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -8,7 +8,8 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
> >  USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <repository> [<path>]
> >     or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
> >     or: $dashless [--quiet] init [--] [<path>...]
> > -   or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
> > +   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
> > +ges
> 
> I think there's an unintentionally added line here with "ges".

That is embarrassing :p.  Will fix in v8.

> > +               if test -n "$remote"
> > +               then
> > +                       if test -z "$nofetch"
> > +                       then
> > +                               # Fetch remote before determining tracking $sha1
> > +                               (clear_local_git_env; cd "$sm_path" && git-fetch) ||
> 
> You should 'git fetch $remote_name' here, and of course, initialize
> remote_name before this.  But how can we know the remote_name in the
> first place?  Is it safe to assume the submodule remote names will
> match those in the superproject?

The other git-fetch call from git-submodule.sh is also bare (i.e. no
specified remote).  When the remote needs to be specified, other
portions of git-submodule.sh use $(get_default_remote), which is (I
think) what the user should expect.  v6 of this series had a
configurable remote name, but Junio wasn't keen on the additional
configuration option.  I don't really mind either way.

> 
> > +                               die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
> > +                       fi
> > +                       remote_name=$(get_default_remote)
> 
> This get_default_remote finds the remote for the remote-tracking
> branch for HEAD in the superproject.  It is possible that HEAD !=
> $branch, so we have very few clues to go on here to get a more
> reasonable answer, so I do not have any good suggestions to improve
> this.

For detached HEADs, get_default_remote should fall back to 'origin',
which seems sane.  If the user wants a different default, they've
likely checkout out a branch in the submodule, setup that branch's
remote, and will be using --merge or --rebase.  If anyone expects
users who will be using detached heads to *want* to specify a
different remote than 'origin', that would be a good argument for
reinstating my submodule.<name>.remote patch from v6.

> > +                       sha1=$(clear_local_git_env; cd "$sm_path" &&
> > +                               git rev-parse --verify "${remote_name}/${branch}") ||
> 
> This does assume the submodule remote names will match those in the
> superproject.  Is this safe?

Another good catch.  I should be calling get_default_remote after
cd-ing into the submodule.  Will change in v8.

Thanks for the feedback :)
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]