Re: [PATCH/RFC] Pinning of submodules

[Junio C Hamano]

Jens Lehmann  writes:

> Am 07.09.2015 um 01:43 schrieb Eric Sunshine:
>
>> My concern in asking was that some future person might come up with
>> another scenario which also wants to use a "magic value" and would
>> have to invent / arrive at another "illegal" representation. Hence, an
>> explicit setting might be more appropriate. However, as stated, I
>> don't even use submodules, so I may be far off the mark. I've cc'd a
>> few of the submodule maintainers with the hope that they will chime
>> in.
>
> Added Trevor to the CC, who is the original author of --remote (see
> 06b1abb5b).
>
> While I believe that adding such functionality makes perfect sense,
> I do not find it terribly obvious that setting the branch to '@' will
> make --remote skip this submodule. I wouldn't care so much if we'd
> only use this value internally, but this is user visible (and has to
> be set by the user if she wants to skip a submodule in --remote).
>
> Setting the branch to an empty value feels a bit more natural, but
> I'm not so sure our config handling supports that well (we seem to
> assume in quite some places that empty equals unset). So I tend to
> prefer a new option for that.

As I stare at both the code change and log message of 06b1abb5
(submodule update: add --remote for submodule's upstream changes,
2012-12-19), I cannot shake this feeling that the change to default
submodule.$name.branch to 'master' is doubly misdesigned.

The stated goal of users of --remote is to declare "This submodule
does not care what the top-level integrator happened to have seen at
the tip when the integration of the history of submodule.  It always
follows the tip of a specific branch at the upstream."

If we were to use any default, the only justification would be "the
users of --remote would want this mode of update to happen for all
submodules, and having to specify which specific branch to be
followed is too cumbersome".  But if that is the case, defaulting to
'master' does not make any sense---if it defaulted to 'HEAD' for
each submodule, it may have made some sense, as that is the usual
convention to denote which branch is the default branch in a
repository.

Also Anders' proposal refutes that "when --remote is used, all
submodules should behave that way" assumption.

I wonder if it is a sensible way forward to introduce a new
configuration variable 'submodule.remoteFallBackBranch' so that the
users can customize this line (near l.800 in git-submodule.sh)

 branch=$(get_submodule_config "$name" branch master)

The possible values for that new configuration value would be:

   - an empty string: disable "update --remote" for any submodule
 '$name' for which submodule.$name.branch is not set.

   - 'master': behave the same way as the current code; we can make
 this the default, when submodule.remoteFallBackBranch is unset,
 to ease transition.

   - any user-chosen branch name. On notable example may be 'HEAD',
 which 06b1abb5 (submodule update: add --remote for submodule's
 upstream changes, 2012-12-19) should have chosen as the
 default.

But I am not the target audience of "update --remote", so let's hear
what the real-world use cases are.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] Pinning of submodules

[Anders Ro]

On 07/09/15 22:13, Jens Lehmann wrote:
> Am 07.09.2015 um 01:43 schrieb Eric Sunshine:
>> On Sun, Sep 6, 2015 at 6:08 PM, Anders Ro 
>> wrote:
>>> On 04/09/15 07:02, Eric Sunshine wrote:
 On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro
  wrote:
> git-submodule.sh: pin submodule when branch name is '@'
>
> Setting branch name to '@' for a submodule will disable 'git submodule
> update --remote' calls for that specific submodule. I.e. instead of
> follow the unspecified default choice of master, nothing is being
> updated. This is useful when multiple submodules exist but not all
> should follow the remote branch head.

 With the disclaimer that I'm not a submodule user (to which the
 answer might be obvious): What benefit is there in using a magic
 value like this ("@") over, say, an explicit configuration setting?
>>>
>>>  From what I have understood (not a submodule expert yet) the '@' is an
>>> invalid branch name and should therefore not collide with any current
>>> branches. My idea was to disable the '--remote' option when the user
>>> have explicitly set an invalid branch name to not modify any current
>>> behaviour. Though having an explicit option is of course more
>>> clarifying. The current behaviour though is that empty branch name means
>>> "follow master" which is somewhat unintuitive.
>>
>> My concern in asking was that some future person might come up with
>> another scenario which also wants to use a "magic value" and would
>> have to invent / arrive at another "illegal" representation. Hence, an
>> explicit setting might be more appropriate. However, as stated, I
>> don't even use submodules, so I may be far off the mark. I've cc'd a
>> few of the submodule maintainers with the hope that they will chime
>> in.

Agree this is not a "future proof" solution. Though faily a quick one. I
started with a setting but realized it involved a bit more changes since
you should be able to tell the submodule command to pin a submodule
right from the start etc.

> 
> Added Trevor to the CC, who is the original author of --remote (see
> 06b1abb5b).
> 
> While I believe that adding such functionality makes perfect sense,
> I do not find it terribly obvious that setting the branch to '@' will
> make --remote skip this submodule. I wouldn't care so much if we'd
> only use this value internally, but this is user visible (and has to
> be set by the user if she wants to skip a submodule in --remote).
> 
> Setting the branch to an empty value feels a bit more natural, but
> I'm not so sure our config handling supports that well (we seem to
> assume in quite some places that empty equals unset). So I tend to
> prefer a new option for that.

I started out that way, thinking that would solve my initial problem of
having 4 submodules which should follow branch 'develop' and one
submodule that should stay put until explicitly changed. Instead having
empty an branch name gave me 'master' on that module, which was way off
current status of the code to say the least. This was all executed by
Jenkins which either gives you "update remote branches" or "not update
remote branches". Thus no other choice than to change the code.

The current 'git submodule add' takes option '-b' as branch and last
time I tried providing '@' as branch name it did not work. That
indicates that there should be a '-p' option for pinning the submodule
from the start. Having to fiddle with the .gitmodules file after the
fact is not really user friendly (though it have worked for me for a
while). I will have a go at an explicit setting in a week or two and get
back for comments on that. Perhaps some maintainers can give some hints
on adding a new option etc meanwhile. How about option '-p, ---pinned'
and setting 'submodule..pinned = true|false'?

Cleaned-up the test code according to Eric's comments, looks a bit more
stylish actually :).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] Pinning of submodules

[Jens Lehmann]

Am 07.09.2015 um 01:43 schrieb Eric Sunshine:

On Sun, Sep 6, 2015 at 6:08 PM, Anders Ro  wrote:

On 04/09/15 07:02, Eric Sunshine wrote:

On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro  wrote:

git-submodule.sh: pin submodule when branch name is '@'

Setting branch name to '@' for a submodule will disable 'git submodule
update --remote' calls for that specific submodule. I.e. instead of
follow the unspecified default choice of master, nothing is being
updated. This is useful when multiple submodules exist but not all
should follow the remote branch head.


With the disclaimer that I'm not a submodule user (to which the
answer might be obvious): What benefit is there in using a magic
value like this ("@") over, say, an explicit configuration setting?


 From what I have understood (not a submodule expert yet) the '@' is an
invalid branch name and should therefore not collide with any current
branches. My idea was to disable the '--remote' option when the user
have explicitly set an invalid branch name to not modify any current
behaviour. Though having an explicit option is of course more
clarifying. The current behaviour though is that empty branch name means
"follow master" which is somewhat unintuitive.


My concern in asking was that some future person might come up with
another scenario which also wants to use a "magic value" and would
have to invent / arrive at another "illegal" representation. Hence, an
explicit setting might be more appropriate. However, as stated, I
don't even use submodules, so I may be far off the mark. I've cc'd a
few of the submodule maintainers with the hope that they will chime
in.


Added Trevor to the CC, who is the original author of --remote (see
06b1abb5b).

While I believe that adding such functionality makes perfect sense,
I do not find it terribly obvious that setting the branch to '@' will
make --remote skip this submodule. I wouldn't care so much if we'd
only use this value internally, but this is user visible (and has to
be set by the user if she wants to skip a submodule in --remote).

Setting the branch to an empty value feels a bit more natural, but
I'm not so sure our config handling supports that well (we seem to
assume in quite some places that empty equals unset). So I tend to
prefer a new option for that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] Pinning of submodules

[Eric Sunshine]

On Sun, Sep 6, 2015 at 6:08 PM, Anders Ro  wrote:
> On 04/09/15 07:02, Eric Sunshine wrote:
>> On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro  wrote:
>>> git-submodule.sh: pin submodule when branch name is '@'
>>>
>>> Setting branch name to '@' for a submodule will disable 'git submodule
>>> update --remote' calls for that specific submodule. I.e. instead of
>>> follow the unspecified default choice of master, nothing is being
>>> updated. This is useful when multiple submodules exist but not all
>>> should follow the remote branch head.
>>
>> With the disclaimer that I'm not a submodule user (to which the
>> answer might be obvious): What benefit is there in using a magic
>> value like this ("@") over, say, an explicit configuration setting?
>
> From what I have understood (not a submodule expert yet) the '@' is an
> invalid branch name and should therefore not collide with any current
> branches. My idea was to disable the '--remote' option when the user
> have explicitly set an invalid branch name to not modify any current
> behaviour. Though having an explicit option is of course more
> clarifying. The current behaviour though is that empty branch name means
> "follow master" which is somewhat unintuitive.

My concern in asking was that some future person might come up with
another scenario which also wants to use a "magic value" and would
have to invent / arrive at another "illegal" representation. Hence, an
explicit setting might be more appropriate. However, as stated, I
don't even use submodules, so I may be far off the mark. I've cc'd a
few of the submodule maintainers with the hope that they will chime
in.

>>> +test_expect_success 'set branch name to "@" for submodule pinned' '
>>> +(cd super && git config --replace-all submodule.pinned.branch "@") &&
>>> +test "$(cd super && git config --get submodule.pinned.branch)" = "@"
>>
>> What is the last line testing? It appears to be testing the behavior
>> of git-config, which is outside the scope of this test script.
>>
>> Once combined, you can use test_config rather than git-config, since
>> test_config will ensure that the config setting is undone when the
>> test exits.

In light of this recent thread[1] which shows that
test_when_finished() and, consequently, test_config() are
non-functional in subshells, I have to retract the advice to use
test_config() in this situation. Instead, at this time, it probably
would be best to continue using "git-config" as you do here...

>>> +test_expect_success 'remove branch name "@" for submodule pinned (unpin)' '
>>> +(cd super && git config --unset-all submodule.pinned.branch) &&
>>
>> If you take the above advice and use test_config in the previous
>> test, then this won't be needed.

and "git-config --unset-all" here.

Later, once [1] has landed in "master" (or perhaps "next"), this can
be revisited and updated to use "test_config -C"[2].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/277323/focus=277370
[2]: http://thread.gmane.org/gmane.comp.version-control.git/277323/focus=277372
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] Pinning of submodules

[Anders Ro]

On 04/09/15 07:02, Eric Sunshine wrote:
> On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro  wrote:
>> Patch to make it possible to pin submodules so that they are not
>> affected by the --remote option in "git submodule".
> 
> Thanks for the patches. I don't use submodules, so I can't comment
> specifically on this change, however, I can offer some general
> comments on the patches. But first, a piece of advice...
> 
> Use git-send-email to post the patches as proper emails, one email
> per patch, rather than as attachments. Reviewers are going to want to
> write inline comments on the patches, and they can't do so when the
> patches are attachments, so attaching patches discourages reviewers
> from responding.

Thanks for the advice and the code comments, I knew I would get it wrong
somehow :). I did try to follow an IMAP Gmail guide in a hurry. I'll fix
the code and make sure git send-mail works the next time.

>> git-submodule.sh: pin submodule when branch name is '@'
>>
>> Setting branch name to '@' for a submodule will disable 'git submodule
>> update --remote' calls for that specific submodule. I.e. instead of
>> follow the unspecified default choice of master, nothing is being
>> updated. This is useful when multiple submodules exist but not all
>> should follow the remote branch head.
> 
> With the disclaimer that I'm not a submodule user (to which the
> answer might be obvious): What benefit is there in using a magic
> value like this ("@") over, say, an explicit configuration setting?

>From what I have understood (not a submodule expert yet) the '@' is an
invalid branch name and should therefore not collide with any current
branches. My idea was to disable the '--remote' option when the user
have explicitly set an invalid branch name to not modify any current
behaviour. Though having an explicit option is of course more
clarifying. The current behaviour though is that empty branch name means
"follow master" which is somewhat unintuitive.

>> Signed-off-by: Anders Ro 
>> ---
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 25b1ddf..1bb2bb1 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -843,7 +843,8 @@ Maybe you want to use 'update --init'?")"
>>   die "$(eval_gettext "Unable to find current revision in submodule path 
>> '\$displaypath'")"
>>   fi
>>
>> - if test -n "$remote"
>> + # Fetch latest in remote unless branch name in config is '@'
>> + if test -n "$remote" -a "$branch" != "@"
> 
> The -a option to 'test' is not portable[1] and is considered obsolete
> by POSIX[2]. Use "test foo && test bar" instead.
> 
> [1]: 
> http://www.gnu.org/software/autoconf/manual/autoconf.html#index-g_t_0040command_007btest_007d-1793
> [2]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html
> 
>>   then
>>   if test -z "$nofetch"
>>   then
>> @@ -857,6 +858,12 @@ Maybe you want to use 'update --init'?")"
>>   die "$(eval_gettext "Unable to find current ${remote_name}/${branch} 
>> revision in submodule path '\$sm_path'")"
>>   fi
>>
>> + # Inform that the current sm is pinned and use of '--remote' ignored
>> + if test -n "$remote" -a "$branch" = "@"
> 
> Ditto.
> 
>> + then
>> + say "$(eval_gettext "Submodule path '\$displaypath' pinned, remote update 
>> ignored")"
>> + fi
>> +
> 
>> Subject: [PATCH 2/2] t7412: add test case for pinned submodules
>>
>> Signed-off-by: Anders Ro 
>> ---
>> diff --git a/t/t7412-submodule-pinning.sh b/t/t7412-submodule-pinning.sh
>> new file mode 100755
>> index 000..2844b1e
>> --- /dev/null
>> +++ b/t/t7412-submodule-pinning.sh
>> @@ -0,0 +1,73 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2015 Anders Ronnbrant
>> +#
>> +
>> +test_description="Branch name '@' disables submodule update --remote calls"
>> +
>> +. ./test-lib.sh
>> +
>> +get_sha() {
>> +  cd $1 && git rev-list --max-count=1 HEAD
>> +}
> 
> A few issues:
> 
> Indent with tabs (width 8), not spaces. (This comment applies to the
> entire patch).
> 
> Style for shell scripts on this project is to have a space before
> "()".
> 
> Taking a hint from t/test-lib-functions.sh:test_cmp_rev(), use "git
> rev-parse --verify" instead.
> 
> It's a bit ugly that this does "cd $1" without ever balancing it with
> a return to the original directory. If someone later comes along and
> writes:
> 
> get_sha1 fiddle >fiddle-id
> 
> in a new test, then that person will be surprised to find that the
> current working directory changed out from under him. The current
> patch doesn't experience this problem since it always invokes it as
> $(get_sha1 fiddle), but it could be made more robust by either
> wrapping it in a subshell so that the 'cd' is undone when the
> subshell exits, or by using "-C $1".
> 
> Taking the above comments into account:
> 
> get_sha () {
> git -C "$1" rev-parse --verify HEAD
> }
> 
>> +equal_sha() {
>> +  test "$(get_sha $1)" = "$(get_sha $2)"
>> +}
>> +
>> 

Re: [PATCH/RFC] Pinning of submodules

[Eric Sunshine]

On Wed, Sep 2, 2015 at 7:34 PM, Anders Ro  wrote:
> Patch to make it possible to pin submodules so that they are not
> affected by the --remote option in "git submodule".

Thanks for the patches. I don't use submodules, so I can't comment
specifically on this change, however, I can offer some general
comments on the patches. But first, a piece of advice...

Use git-send-email to post the patches as proper emails, one email
per patch, rather than as attachments. Reviewers are going to want to
write inline comments on the patches, and they can't do so when the
patches are attachments, so attaching patches discourages reviewers
from responding.

> git-submodule.sh: pin submodule when branch name is '@'
>
> Setting branch name to '@' for a submodule will disable 'git submodule
> update --remote' calls for that specific submodule. I.e. instead of
> follow the unspecified default choice of master, nothing is being
> updated. This is useful when multiple submodules exist but not all
> should follow the remote branch head.

With the disclaimer that I'm not a submodule user (to which the
answer might be obvious): What benefit is there in using a magic
value like this ("@") over, say, an explicit configuration setting?

> Signed-off-by: Anders Ro 
> ---
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 25b1ddf..1bb2bb1 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -843,7 +843,8 @@ Maybe you want to use 'update --init'?")"
>   die "$(eval_gettext "Unable to find current revision in submodule path 
> '\$displaypath'")"
>   fi
>
> - if test -n "$remote"
> + # Fetch latest in remote unless branch name in config is '@'
> + if test -n "$remote" -a "$branch" != "@"

The -a option to 'test' is not portable[1] and is considered obsolete
by POSIX[2]. Use "test foo && test bar" instead.

[1]: 
http://www.gnu.org/software/autoconf/manual/autoconf.html#index-g_t_0040command_007btest_007d-1793
[2]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html

>   then
>   if test -z "$nofetch"
>   then
> @@ -857,6 +858,12 @@ Maybe you want to use 'update --init'?")"
>   die "$(eval_gettext "Unable to find current ${remote_name}/${branch} 
> revision in submodule path '\$sm_path'")"
>   fi
>
> + # Inform that the current sm is pinned and use of '--remote' ignored
> + if test -n "$remote" -a "$branch" = "@"

Ditto.

> + then
> + say "$(eval_gettext "Submodule path '\$displaypath' pinned, remote update 
> ignored")"
> + fi
> +

> Subject: [PATCH 2/2] t7412: add test case for pinned submodules
>
> Signed-off-by: Anders Ro 
> ---
> diff --git a/t/t7412-submodule-pinning.sh b/t/t7412-submodule-pinning.sh
> new file mode 100755
> index 000..2844b1e
> --- /dev/null
> +++ b/t/t7412-submodule-pinning.sh
> @@ -0,0 +1,73 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2015 Anders Ronnbrant
> +#
> +
> +test_description="Branch name '@' disables submodule update --remote calls"
> +
> +. ./test-lib.sh
> +
> +get_sha() {
> +  cd $1 && git rev-list --max-count=1 HEAD
> +}

A few issues:

Indent with tabs (width 8), not spaces. (This comment applies to the
entire patch).

Style for shell scripts on this project is to have a space before
"()".

Taking a hint from t/test-lib-functions.sh:test_cmp_rev(), use "git
rev-parse --verify" instead.

It's a bit ugly that this does "cd $1" without ever balancing it with
a return to the original directory. If someone later comes along and
writes:

get_sha1 fiddle >fiddle-id

in a new test, then that person will be surprised to find that the
current working directory changed out from under him. The current
patch doesn't experience this problem since it always invokes it as
$(get_sha1 fiddle), but it could be made more robust by either
wrapping it in a subshell so that the 'cd' is undone when the
subshell exits, or by using "-C $1".

Taking the above comments into account:

get_sha () {
git -C "$1" rev-parse --verify HEAD
}

> +equal_sha() {
> +  test "$(get_sha $1)" = "$(get_sha $2)"
> +}
> +
> +not_equal_sha() {
> +  test "$(get_sha $1)" != "$(get_sha $2)"
> +}

Style: space before "()" on these two function declarations

> +test_expect_success 'setup submodule tree structure' '
> +for i in 1 2 3; do echo file$i > file$i; git add file$i; git commit -m 
> file$i; done &&

Style: write each command of the 'for' loop on its own line,
including 'do', don't use semi-colons

Style: no space after redirection operator: >file$i

Keep the &&-chain intact, and end the chain with "|| return $?" so
that the 'for' loop correctly exits if any contained command fails.

for i in 1 2 3
do
echo file$i >file$i &&
git add file$i &&
git commit -m file$i || return $?
done

> +test_tick &&

Why is this needed? There doesn't seem to be any obvious reason for
its presence, and the test still passes without it.

> +git clone . super &&
> +git clone . follow &&
> +git