Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Junio C Hamano]

Stefan Beller  writes:

> On Sat, Dec 10, 2016 at 5:41 AM, vi0oss  wrote:
>> On 12/08/2016 04:38 AM, vi0...@gmail.com wrote:
>>>
>>>  Third review: missing && in test fixed.
>>>
>>
>> Shall something more be done about this or just wait until the patch gets
>> reviewed and integrated?
>
> I have no further comments and think the most recent version you sent
> to the list
> is fine. However others are invited to comment as well. :)

I'll take that as a reviewed-by from you and queue it.

Thanks, both.

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Sat, Dec 10, 2016 at 5:41 AM, vi0oss  wrote:
> On 12/08/2016 04:38 AM, vi0...@gmail.com wrote:
>>
>>  Third review: missing && in test fixed.
>>
>
> Shall something more be done about this or just wait until the patch gets
> reviewed and integrated?

I have no further comments and think the most recent version you sent
to the list
is fine. However others are invited to comment as well. :)

Thanks,
Stefan

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[vi0oss]

On 12/08/2016 04:38 AM, vi0...@gmail.com wrote:

 Third review: missing && in test fixed.
 
Shall something more be done about this or just wait until the patch 
gets reviewed and integrated?

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Jeff King]

On Thu, Dec 08, 2016 at 09:04:46PM +0300, vi0oss wrote:

> Why Git test use &&-chains instead of proper "set -e"?

Because "set -e" comes with all kinds of confusing corner cases. Using
&& chains is annoying, but rarely surprising.

One of my favorite examples is:

  set -e
  (
false
echo 1
  ) || {
echo outcome=$?
false
  }
  echo 2

which prints both "1" and "2".

Inside the subshell, "set -e" has no effect, and you cannot re-enable it
by setting "-e" (it's suppressed entirely because we are on the
left-hand side of an || conditional).

So you could write a function like this:

  foo() {
do_one
do_two
  }

that relies on catching the failure from do_one. And it works here:

  set -e
  foo

but not here:

  set -e
  if foo then
do_something
  fi

And there's no way to make it work without adding back in the
&&-chaining.

-Peff

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Thu, Dec 8, 2016 at 10:04 AM, vi0oss  wrote:
> On 12/08/2016 08:46 PM, Jeff King wrote:
>>
>> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:
>>
>>> On Wed, Dec 7, 2016 at 4:39 PM,   wrote:
>>>
  Previously test contained errorneous
  test_must_fail, which was masked by
  missing &&.
>>>
>>> I wonder if we could make either
>>> the test_must_fail intelligent to detect such a broken && call chain
>>> or the test_expect_success macro to see for those broken chains.
>>>
>>>
>>> I wish we could improve that, but I spend a lot of brain cycles on it at
>>> one point and couldn't come up with a workable solution.
>>>
>>> -Peff
>>>
> Why Git test use &&-chains instead of proper "set -e"?
>

"Because set -e kills the shell and we would want to keep going
until the test suite is finished and display a summary what failed"
would be my first reaction, but let's dig into history:
bb79af9d09 might be helpful on that, but it doesn't explain why
we use && chains.
I could not find any commit explaining the use of && chains.

e1970ce43abf might be interesting (the introduction of the test
suite), as that did not contain && chains.

I guess it would be hard(er) to implement e.g. test_must_fail
in an environment where -e is set.

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[vi0oss]

On 12/08/2016 08:46 PM, Jeff King wrote:

On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:


On Wed, Dec 7, 2016 at 4:39 PM,   wrote:


 Previously test contained errorneous
 test_must_fail, which was masked by
 missing &&.

I wonder if we could make either
the test_must_fail intelligent to detect such a broken && call chain
or the test_expect_success macro to see for those broken chains.


I wish we could improve that, but I spend a lot of brain cycles on it at
one point and couldn't come up with a workable solution.

-Peff


Why Git test use &&-chains instead of proper "set -e"?

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Thu, Dec 8, 2016 at 9:46 AM, Jeff King  wrote:
>
> will both trigger on the &&-chain linter, because it uses a magic exit
> code to detect the breakage. I think the problem is just that the
> &&-chain linter cannot peek inside subshells, and that's where the bug
> was in this case.

Uh, yeah in the subshell, but the patch v2 did have it not in
subshells, I'll take another look.

>
> I wish we could improve that, but I spend a lot of brain cycles on it at
> one point and couldn't come up with a workable solution.

Is it possible to overwrite what happens when you open a subshell with ( ) ?
i.e. I imagine in the global test-setup we'd setup that ( ) not just
opens /bin/sh
but a shell with benefits such that we can execute just one && chain.

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Jeff King]

On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:

> On Wed, Dec 7, 2016 at 4:39 PM,   wrote:
> 
> >
> > Previously test contained errorneous
> > test_must_fail, which was masked by
> > missing &&.
> 
> I wonder if we could make either
> the test_must_fail intelligent to detect such a broken && call chain
> or the test_expect_success macro to see for those broken chains.

I don't think test_must_fail is relevant for &&-chains. Even something
like:

  test_must_fail foo
  bar

or:

  bar
  test_must_fail foo

will both trigger on the &&-chain linter, because it uses a magic exit
code to detect the breakage. I think the problem is just that the
&&-chain linter cannot peek inside subshells, and that's where the bug
was in this case.

I wish we could improve that, but I spend a lot of brain cycles on it at
one point and couldn't come up with a workable solution.

-Peff

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 4:39 PM,   wrote:

>
> Previously test contained errorneous
> test_must_fail, which was masked by
> missing &&.

I wonder if we could make either
the test_must_fail intelligent to detect such a broken && call chain
or the test_expect_success macro to see for those broken chains.

Patch looks good to me except one very minor nit. :)

> +test_expect_success 'nested submodule alternate in works and is actually 
> used' '
> +   test_when_finished "rm -rf supersuper-clone" &&
> +   git clone --recursive --reference supersuper supersuper 
> supersuper-clone &&
> +   (
> +   cd supersuper-clone &&
> +   # test superproject has alternates setup correctly
> +   test_alternate_is_used .git/objects/info/alternates . &&
> +   # immediate submodule has alternate:
> +   test_alternate_is_used 
> .git/modules/subwithsub/objects/info/alternates subwithsub

here is a && missing ;)

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 1:24 PM, vi0oss  wrote:
> On 12/07/2016 11:09 PM, Stefan Beller wrote:
>>>
>>> As submodule's alternate target does not end in .git/objects
>>> (rather .git/modules/qq/objects), this alternate target
>>> path restriction for in add_possible_reference_from_superproject
>>> relates from "*.git/objects" to just */objects".
>>
>> I wonder if this check is too weak and we actually have to check for
>> either .git/objects or modules//objects.
>> When writing the referenced commit I assumed we'd need a stronger check
>> to be safer and not add some random location as a possible alternate.
>>
> 1. Do we really need to check that it is named ".git"? Although
> "git clone --mirror --recursive" is not (directly) supported
> now, user may create one bare repository with [remnants of]
> submodules by converting reqular repository into bare one.
> Why not take advantage of additional information available locally
> in this case?

Oh, great point.

>
> 2. Is the check need to be strict because of we need to traverse
> one directory level up? Normally this "/objects" part is added by
> Git, so just one "../" seems to be OK. User can't specify "--reference
> somerepo/.git/objects", a strange reference can appear only if user
> manually creates alternates. Maybe better to document this case
> instead of restricting the feature?

Not sure I understand what needs better documentation here?

>
> 3. If nonetheless check for ".git/*/objects", then
> a. What functions should be used in Git codebase for such checks?
> b. Should we handle tricks like "smth/.git/../../objects" and so on?

I see we're getting into problems here.

>
> 4. Should we print (or print in verbose mode) each used alternate,
> to inform operator what his or her new clone will depend on?
>
> P.S. Actually I discovered the --recursive --reference feature when tried to
> put reference to a mega-repo with all possible submodules added as remotes.
> I expected --reference to just get though across all submodules, but it
> complained
> to missing "/modules/..." instead (the check went though becase the
> repository
> was named like "megarepo.git", so it did ended in ".git/objects").

Oh :(

With that said, I think the original patch is a sensible approach.

Thanks,
Stefan

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[vi0oss]

On 12/07/2016 11:09 PM, Stefan Beller wrote:

As submodule's alternate target does not end in .git/objects
(rather .git/modules/qq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".

I wonder if this check is too weak and we actually have to check for
either .git/objects or modules//objects.
When writing the referenced commit I assumed we'd need a stronger check
to be safer and not add some random location as a possible alternate.


1. Do we really need to check that it is named ".git"? Although
"git clone --mirror --recursive" is not (directly) supported
now, user may create one bare repository with [remnants of]
submodules by converting reqular repository into bare one.
Why not take advantage of additional information available locally
in this case?

2. Is the check need to be strict because of we need to traverse
one directory level up? Normally this "/objects" part is added by
Git, so just one "../" seems to be OK. User can't specify "--reference
somerepo/.git/objects", a strange reference can appear only if user
manually creates alternates. Maybe better to document this case
instead of restricting the feature?

3. If nonetheless check for ".git/*/objects", then
a. What functions should be used in Git codebase for such checks?
b. Should we handle tricks like "smth/.git/../../objects" and so on?

4. Should we print (or print in verbose mode) each used alternate,
to inform operator what his or her new clone will depend on?

P.S. Actually I discovered the --recursive --reference feature when tried to
put reference to a mega-repo with all possible submodules added as remotes.
I expected --reference to just get though across all submodules, but it 
complained
to missing "/modules/..." instead (the check went though becase the 
repository

was named like "megarepo.git", so it did ended in ".git/objects").

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Junio C Hamano]

Stefan Beller  writes:

> On Wed, Dec 7, 2016 at 12:18 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
 This patch makes all not just the root repository, but also
 all submodules (recursively) have submodule.alternateLocation
 and submodule.alternateErrorStrategy configured, making Git
 search for possible alternates for nested submodules as well.
>>>
>>> Sounds great!
>>
>> Is it safe to assume that all the submodules used recursively by
>> submodules share the same structure upstream?  Does the alternate
>> location mechanism degrades sensibly if this assumption turns out to
>> be false (i.e. "possible alternates" above turns out to be mere
>> possibility and not there)?
>
> According to the last test in the patch, this seems to be doing the
> sensible thing.

OK, that sounds great.  Thanks.

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 12:18 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> This patch makes all not just the root repository, but also
>>> all submodules (recursively) have submodule.alternateLocation
>>> and submodule.alternateErrorStrategy configured, making Git
>>> search for possible alternates for nested submodules as well.
>>
>> Sounds great!
>
> Is it safe to assume that all the submodules used recursively by
> submodules share the same structure upstream?  Does the alternate
> location mechanism degrades sensibly if this assumption turns out to
> be false (i.e. "possible alternates" above turns out to be mere
> possibility and not there)?

According to the last test in the patch, this seems to be doing the
sensible thing.

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Junio C Hamano]

Stefan Beller  writes:

>> This patch makes all not just the root repository, but also
>> all submodules (recursively) have submodule.alternateLocation
>> and submodule.alternateErrorStrategy configured, making Git
>> search for possible alternates for nested submodules as well.
>
> Sounds great!

Is it safe to assume that all the submodules used recursively by
submodules share the same structure upstream?  Does the alternate
location mechanism degrades sensibly if this assumption turns out to
be false (i.e. "possible alternates" above turns out to be mere
possibility and not there)?

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 10:42 AM,   wrote:
> From: Vitaly _Vi Shukela 

Thanks for contributing to Git!
(/me looks up if you have sent patches already as you
seem to know how to do that. :) unrelated side note: Maybe you want
to send a patch for the .mailmap file mapping your two email addresses
together, c.f. "git log -- .mailmap")

>
> Git v2.11 introduced "git clone --recursive --referece ...",
> but it didn't put the alternates for _nested_ submodules.

This message is targeted at people familiar with gits code base,
so we can be more specific. e.g.

In 31224cbdc7 (clone: recursive and reference option triggers
submodule alternates, 2016-08-17) a mechanism was added to
have submodules referenced.  It did not address _nested_
submodules, however.

>
> This patch makes all not just the root repository, but also
> all submodules (recursively) have submodule.alternateLocation
> and submodule.alternateErrorStrategy configured, making Git
> search for possible alternates for nested submodules as well.

Sounds great!

>
> As submodule's alternate target does not end in .git/objects
> (rather .git/modules/qq/objects), this alternate target
> path restriction for in add_possible_reference_from_superproject
> relates from "*.git/objects" to just */objects".

I wonder if this check is too weak and we actually have to check for
either .git/objects or modules//objects.
When writing the referenced commit I assumed we'd need a stronger check
to be safer and not add some random location as a possible alternate.

>
> New tests have been added to t7408-submodule-reference.

Thanks!

>
> Signed-off-by: Vitaly _Vi Shukela 
> ---
>  builtin/submodule--helper.c| 24 --
>  t/t7408-submodule-reference.sh | 73 
> ++
>  2 files changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 4beeda5..93dae62 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
>
> /*
>  * If the alternate object store is another repository, try the
> -* standard layout with .git/modules//objects
> +* standard layout with .git/(modules/)+/objects
>  */
> -   if (ends_with(alt->path, ".git/objects")) {
> +   if (ends_with(alt->path, "/objects")) {
> char *sm_alternate;
> struct strbuf sb = STRBUF_INIT;
> struct strbuf err = STRBUF_INIT;
> @@ -672,6 +672,26 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
> die(_("could not get submodule directory for '%s'"), path);
> git_config_set_in_file(p, "core.worktree",
>relative_path(path, sm_gitdir, _path));
> +
> +   /* setup alternateLocation and alternateErrorStrategy in the cloned 
> submodule if needed */
> +   {

Usually we do not use braces to further nest code, please remove this nesting.

> +   char *sm_alternate = NULL, *error_strategy = NULL;
> +
> +   git_config_get_string("submodule.alternateLocation", 
> _alternate);
> +   if (sm_alternate) {
> +   git_config_set_in_file(p, 
> "submodule.alternateLocation",
> +  sm_alternate);
> +   }
> +   git_config_get_string("submodule.alternateErrorStrategy", 
> _strategy);
> +   if (error_strategy) {
> +   git_config_set_in_file(p, 
> "submodule.alternateErrorStrategy",
> +  error_strategy);
> +   }
> +
> +   free(sm_alternate);
> +   free(error_strategy);
> +   }
> +
> strbuf_release();
> strbuf_release(_path);
> free(sm_gitdir);
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index 1c1e289..7b64725 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -125,4 +125,77 @@ test_expect_success 'ignoring missing submodule 
> alternates passes clone and subm
> )
>  '
>
> +test_expect_success 'preparing second superproject with a nested submodule' '
> +   test_create_repo supersuper &&
> +   (
> +   cd supersuper &&
> +   echo I am super super. >file &&

Usually we quote strings containing white space, e.g. echo "I am ..." >actual

> +   git add file &&
> +   git commit -m B-super-super-initial
> +   git submodule add "file://$base_dir/super" subwithsub &&
> +   git commit -m B-super-super-added &&
> +   git submodule update --init --recursive &&
> +   git repack -ad
> +   ) &&
> +   echo not cleaning supersuper

This echo is left in