Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

2016-08-31 Thread Jacob Keller
On Tue, Aug 30, 2016 at 10:04 PM, Stefan Beller  wrote:
> On Wed, Aug 24, 2016 at 4:37 PM, Jacob Keller  wrote:
>
>> Yes that seems reasonable.
>>
>> Thanks,
>> Jake
>
> I reviewed all your comments and you seem to be ok with including this
> series as it is queued currently?
>
> Thanks,
> Stefan

Yea based on what's in Junio's tree, I think we've squashed in all the
suggested changes, unless there have been more suggestions I missed.

Regards,
Jake


Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

2016-08-30 Thread Stefan Beller
On Wed, Aug 24, 2016 at 4:37 PM, Jacob Keller  wrote:

> Yes that seems reasonable.
>
> Thanks,
> Jake

I reviewed all your comments and you seem to be ok with including this
series as it is queued currently?

Thanks,
Stefan


Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

2016-08-24 Thread Jacob Keller
On Wed, Aug 24, 2016 at 3:52 PM, Stefan Beller  wrote:
> On Tue, Aug 23, 2016 at 11:29 PM, Jacob Keller  wrote:
>> On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller  wrote:
> +
> +   if (option_recursive) {
> +   if (option_required_reference.nr &&
> +   option_optional_reference.nr)
> +   die(_("clone --recursive is not compatible with "
> + "both --reference and 
> --reference-if-able"));

 So if you have multiple references that don't all match we basically
 just refuse to allow recursive?

 Would it be better to simply assume that we want to die on missing
 references instead of failing the clone here?
>>>
>>> The new config options are per repo (or even set globally), and not
>>> per alternate. And as we communicate the [if-able] part via the config
>>> options to the submodules it is not feasible to transport both
>>> kinds of (reference-or-die and reference-but-ignore-misses).
>>>
>>> That is why I introduced this check in the first place. If we'd go back
>>> to the drawing board and come up with a solution that is on a
>>> "per alternate" basis we could allow such things.
>>>
 That is, treat it so
 that multiple reference and reference-if-able will die, and only info
 if we got only reference-if-able?

 Probably what's here is fine, and mixing reference and
 reference-if-able doesn't make much sense.
>>>
>>> I think the reference-if-able doesn't make sense for one project alone
>>> as you can easily script around that, but is only useful if you have
>>> submodules in a partially checked out superproject that you want
>>> to reference to.
>>>
>>> Thanks,
>>> Stefan
>>
>> I'm not sure there is a better design.  How are alternates stored? In
>> a config section?
>
> Alternates are stored in .git/objects/info/alternates
> with each alternate in a new line. On that file (from
> (man gitrepository-layout):
>
> objects/info/alternates
>
> This file records paths to alternate object stores that this object store
> borrows objects from, one pathname per line. Note that not only native
> Git tools use it locally, but the HTTP fetcher also tries to use it remotely;
> this will usually work if you have relative paths (relative to the object
> database, not to the repository!) in your alternates file, but it will not 
> work
> if you use absolute paths unless the absolute path in filesystem and web
> URL is the same. See also objects/info/http-alternates.
>
> So changing that file is out of question.
> Ideally we would have a flag for each path here, though.
>
>> Or is there some way we can store the is-able per
>> alternate and look it up when adding them to submodule?
>
> I guess we could invent a file as alternate-flags that is matches
> line by line to the alternates file.
>
> I don't think we'd want to go that way for now as it would really only
> help in an edge case?
>
> If we later find out we need the flag on a per-alternate basis we can
> still come up with a solution and just not set these config variables,
> so I think we'll be fine for now with this approach.
>

Yes that seems reasonable.

Thanks,
Jake
--
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: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

2016-08-24 Thread Stefan Beller
On Tue, Aug 23, 2016 at 11:29 PM, Jacob Keller  wrote:
> On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller  wrote:
 +
 +   if (option_recursive) {
 +   if (option_required_reference.nr &&
 +   option_optional_reference.nr)
 +   die(_("clone --recursive is not compatible with "
 + "both --reference and --reference-if-able"));
>>>
>>> So if you have multiple references that don't all match we basically
>>> just refuse to allow recursive?
>>>
>>> Would it be better to simply assume that we want to die on missing
>>> references instead of failing the clone here?
>>
>> The new config options are per repo (or even set globally), and not
>> per alternate. And as we communicate the [if-able] part via the config
>> options to the submodules it is not feasible to transport both
>> kinds of (reference-or-die and reference-but-ignore-misses).
>>
>> That is why I introduced this check in the first place. If we'd go back
>> to the drawing board and come up with a solution that is on a
>> "per alternate" basis we could allow such things.
>>
>>> That is, treat it so
>>> that multiple reference and reference-if-able will die, and only info
>>> if we got only reference-if-able?
>>>
>>> Probably what's here is fine, and mixing reference and
>>> reference-if-able doesn't make much sense.
>>
>> I think the reference-if-able doesn't make sense for one project alone
>> as you can easily script around that, but is only useful if you have
>> submodules in a partially checked out superproject that you want
>> to reference to.
>>
>> Thanks,
>> Stefan
>
> I'm not sure there is a better design.  How are alternates stored? In
> a config section?

Alternates are stored in .git/objects/info/alternates
with each alternate in a new line. On that file (from
(man gitrepository-layout):

objects/info/alternates

This file records paths to alternate object stores that this object store
borrows objects from, one pathname per line. Note that not only native
Git tools use it locally, but the HTTP fetcher also tries to use it remotely;
this will usually work if you have relative paths (relative to the object
database, not to the repository!) in your alternates file, but it will not work
if you use absolute paths unless the absolute path in filesystem and web
URL is the same. See also objects/info/http-alternates.

So changing that file is out of question.
Ideally we would have a flag for each path here, though.

> Or is there some way we can store the is-able per
> alternate and look it up when adding them to submodule?

I guess we could invent a file as alternate-flags that is matches
line by line to the alternates file.

I don't think we'd want to go that way for now as it would really only
help in an edge case?

If we later find out we need the flag on a per-alternate basis we can
still come up with a solution and just not set these config variables,
so I think we'll be fine for now with this approach.

Thanks,
Stefan
--
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: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

2016-08-24 Thread Jacob Keller
On Tue, Aug 23, 2016 at 4:03 PM, Stefan Beller  wrote:
>>> +
>>> +   if (option_recursive) {
>>> +   if (option_required_reference.nr &&
>>> +   option_optional_reference.nr)
>>> +   die(_("clone --recursive is not compatible with "
>>> + "both --reference and --reference-if-able"));
>>
>> So if you have multiple references that don't all match we basically
>> just refuse to allow recursive?
>>
>> Would it be better to simply assume that we want to die on missing
>> references instead of failing the clone here?
>
> The new config options are per repo (or even set globally), and not
> per alternate. And as we communicate the [if-able] part via the config
> options to the submodules it is not feasible to transport both
> kinds of (reference-or-die and reference-but-ignore-misses).
>
> That is why I introduced this check in the first place. If we'd go back
> to the drawing board and come up with a solution that is on a
> "per alternate" basis we could allow such things.
>
>> That is, treat it so
>> that multiple reference and reference-if-able will die, and only info
>> if we got only reference-if-able?
>>
>> Probably what's here is fine, and mixing reference and
>> reference-if-able doesn't make much sense.
>
> I think the reference-if-able doesn't make sense for one project alone
> as you can easily script around that, but is only useful if you have
> submodules in a partially checked out superproject that you want
> to reference to.
>
> Thanks,
> Stefan

I'm not sure there is a better design.  How are alternates stored? In
a config section? Or is there some way we can store the is-able per
alternate and look it up when adding them to submodule?

Thanks,
Jake
--
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: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

2016-08-23 Thread Stefan Beller
>> +
>> +   if (option_recursive) {
>> +   if (option_required_reference.nr &&
>> +   option_optional_reference.nr)
>> +   die(_("clone --recursive is not compatible with "
>> + "both --reference and --reference-if-able"));
>
> So if you have multiple references that don't all match we basically
> just refuse to allow recursive?
>
> Would it be better to simply assume that we want to die on missing
> references instead of failing the clone here?

The new config options are per repo (or even set globally), and not
per alternate. And as we communicate the [if-able] part via the config
options to the submodules it is not feasible to transport both
kinds of (reference-or-die and reference-but-ignore-misses).

That is why I introduced this check in the first place. If we'd go back
to the drawing board and come up with a solution that is on a
"per alternate" basis we could allow such things.

> That is, treat it so
> that multiple reference and reference-if-able will die, and only info
> if we got only reference-if-able?
>
> Probably what's here is fine, and mixing reference and
> reference-if-able doesn't make much sense.

I think the reference-if-able doesn't make sense for one project alone
as you can easily script around that, but is only useful if you have
submodules in a partially checked out superproject that you want
to reference to.

Thanks,
Stefan
--
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: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

2016-08-23 Thread Jacob Keller
On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller  wrote:
> When `--recursive` and `--reference` is given, it is reasonable to
> expect that the submodules are created with references to the submodules
> of the given alternate for the superproject.
>
>   An initial attempt to do this was presented to the mailing list, which
>   used flags that are passed around ("--super-reference") that instructed
>   the submodule clone to look for a reference in the submodules of the
>   referenced superproject. This is not well thought out, as any further
>   `submodule update` should also respect the initial setup.
>
>   When a new  submodule is added to the superproject and the alternate
>   of the superproject does not know about that submodule yet, we rather
>   error out informing the user instead of being unclear if we did or did
>   not use a submodules alternate.
>
> To solve this problem introduce new options that store the configuration
> for what the user wanted originally.
>
> Signed-off-by: Stefan Beller 
> ---
>  Documentation/config.txt   | 12 ++
>  builtin/clone.c| 19 +
>  builtin/submodule--helper.c| 87 
> ++
>  t/t7408-submodule-reference.sh | 43 +
>  4 files changed, 161 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index bc1c433..602f43a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2837,6 +2837,18 @@ submodule.fetchJobs::
> in parallel. A value of 0 will give some reasonable default.
> If unset, it defaults to 1.
>
> +submodule.alternateLocation::
> +   Specifies how the submodules obtain alternates when submodules are
> +   cloned. Possible values are `no`, `superproject`.
> +   By default `no` is assumed, which doesn't add references. When the
> +   value is set to `superproject` the submodule to be cloned computes
> +   its alternates location relative to the superprojects alternate.
> +
> +submodule.alternateErrorStrategy
> +   Specifies how to treat errors with the alternates for a submodule
> +   as computed via `submodule.alternateLocation`. Possible values are
> +   `ignore`, `info`, `die`.
> +
>  tag.forceSignAnnotated::
> A boolean to specify whether annotated tags created should be GPG 
> signed.
> If `--annotate` is specified on the command line, it takes
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0182665..404c5e8 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char 
> *prefix)
> else
> fprintf(stderr, _("Cloning into '%s'...\n"), dir);
> }
> +
> +   if (option_recursive) {
> +   if (option_required_reference.nr &&
> +   option_optional_reference.nr)
> +   die(_("clone --recursive is not compatible with "
> + "both --reference and --reference-if-able"));

So if you have multiple references that don't all match we basically
just refuse to allow recursive?

Would it be better to simply assume that we want to die on missing
references instead of failing the clone here? That is, treat it so
that multiple reference and reference-if-able will die, and only info
if we got only reference-if-able?

Probably what's here is fine, and mixing reference and
reference-if-able doesn't make much sense.

Thanks,
Jake
--
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