Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates
On Tue, Aug 30, 2016 at 10:04 PM, Stefan Bellerwrote: > 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
On Wed, Aug 24, 2016 at 4:37 PM, Jacob Kellerwrote: > 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
On Wed, Aug 24, 2016 at 3:52 PM, Stefan Bellerwrote: > 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
On Tue, Aug 23, 2016 at 11:29 PM, Jacob Kellerwrote: > 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
On Tue, Aug 23, 2016 at 4:03 PM, Stefan Bellerwrote: >>> + >>> + 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
>> + >> + 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
On Mon, Aug 15, 2016 at 2:53 PM, Stefan Bellerwrote: > 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