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

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 [off-list ref] wrote:

quoted

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

quoted

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.

quoted

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

quoted

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

quoted

+                       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