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@xxxxxxxxxx> 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:
signature.asc
Description: OpenPGP digital signature