Re: [PATCHv3 6/9] clone: implement optional references
From: Junio C Hamano <hidden>
Date: 2016-08-09 16:37:30
Stefan Beller [off-list ref] writes:
quoted hunk
@@ -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.
quoted hunk
+ 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."