Re: [PATCHv3 6/9] clone: implement optional references
Stefan Beller writes: > Thinking about that we may want to not rely on such a hack, > but make it clearer. But that is far from sufficient, isn't it? You'd need to bypass "not a local repository", "shallow" and "is grafted" anyway, so in that sense, that hack is not doing much. If modules/X (or modules/X/) does not exist, it won't pass these checks, so burdening the readers with slash trickery is not really worth their time. -- 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: [PATCHv3 6/9] clone: implement optional references
On Tue, Aug 9, 2016 at 10:54 AM, Junio C Hamano wrote: > Junio C Hamano writes: > >> If I am reading the implementation of real_path_internal() >> correctly, the most relevant reason that an "if-able" repository >> cannot be used causes real_path_if_valid() to return NULL. > > Oops, too many proofreading and rephrasing. Please insert "I do not > think that" before "the most relevant". Side note: It took me a while to mentally process and reread this. It would have been easier with just s/the most relevant reason/I do not think that the most relevant reason/ but that is just me being used to s/ expressions by now. I think our emails nearly crossed. In the other thread I wondered if you had different expectations on the -if-able part than I do. For the expected use case I do expect that the missing path is the most relevant thing (the submodule is not checked out). Of course the submodule may be cloned shallow as it was recommended in the .gitmodules file, so we should also care about that use case. > Let's say your superproject borrows from ~/w/super, whose submodule > repositories (if cloned) are directly in "~/w/super/.git/modules/". > When you clone a submodule X for your superproject, you allow it to > borrow from "~/w/super/.git/modules/X" if there is one available. which is why we need to pass in /X/ with an ending slash. See the comment in 8/9. + /* +* We need to end the new path with '/' to mark it as a dir, +* otherwise a submodule name containing '/' will be broken +* as the last part of a missing submodule reference would +* be taken as a file name. +*/ + strbuf_addf(&sb, "modules/%s/", sas->submodule_name); Thinking about that we may want to not rely on such a hack, but make it clearer. 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: [PATCHv3 6/9] clone: implement optional references
Junio C Hamano writes: > If I am reading the implementation of real_path_internal() > correctly, the most relevant reason that an "if-able" repository > cannot be used causes real_path_if_valid() to return NULL. Oops, too many proofreading and rephrasing. Please insert "I do not think that" before "the most relevant". -- 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: [PATCHv3 6/9] clone: implement optional references
Stefan Beller writes:
> @@ -283,11 +286,22 @@ static void strip_trailing_slashes(char *dir)
> static int add_one_reference(struct string_list_item *item, void *cb_data)
> {
> char *ref_git;
> - const char *repo;
> + const char *repo, *ref_git_s;
> + int *required = cb_data;
> struct strbuf alternate = STRBUF_INIT;
>
> - /* Beware: read_gitfile(), real_path() and mkpath() return static
> buffer */
> - ref_git = xstrdup(real_path(item->string));
> + ref_git_s = *required ?
> + real_path(item->string) :
> + real_path_if_valid(item->string);
> + if (!ref_git_s) {
> + warning(_("Not using proposed alternate %s"), item->string);
If I am reading the implementation of real_path_internal()
correctly, the most relevant reason that an "if-able" repository
cannot be used causes real_path_if_valid() to return NULL.
Let's say your superproject borrows from ~/w/super, whose submodule
repositories (if cloned) are directly in "~/w/super/.git/modules/".
When you clone a submodule X for your superproject, you allow it to
borrow from "~/w/super/.git/modules/X" if there is one available.
I think both real_path() and real_path_if_valid() happily returns
the real path to "~/w/super/.git/modules/" with "X" concatenated at
the end, as long as that 'modules' directory exists, even when there
is no X inside.
Using if_valid() is still necessary to avoid a totally bogus path to
cause real_path() to barf. I am just saying that this warning is
not so interesting, and a real check, "do we have a repository
there? If not, don't add it as an alternate" must be somewhere
else.
> + return 0;
> + } else
> + /*
> + * Beware: read_gitfile(), real_path() and mkpath()
> + * return static buffer
> + */
> + ref_git = xstrdup(ref_git_s);
>
> repo = read_gitfile(ref_git);
> if (!repo)
> @@ -304,7 +318,8 @@ static int add_one_reference(struct string_list_item
> *item, void *cb_data)
> } else if (!is_directory(mkpath("%s/objects", ref_git))) {
> struct strbuf sb = STRBUF_INIT;
> if (get_common_dir(&sb, ref_git))
> - die(_("reference repository '%s' as a linked checkout
> is not supported yet."),
> + die(_("reference repository '%s' as a "
> + "linked checkout is not supported yet."),
> item->string);
And curiously, this is not such a check. This is an unrelated change.
> die(_("reference repository '%s' is not a local repository."),
> item->string);
You need to arrange this die() not to trigger when you are asked to
borrow from there if there is one to borrow from. I see a few other
die() following this check to avoid using shallow repositories and
repositories with grafts, but I think they all should turn into a
"Nah, this is unusable, and I was asked to borrow _only_ if the
repository is usable, so I won't use it."
--
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
