Re: [PATCHv3 6/9] clone: implement optional references

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2016, 16:37 UTC
Message-ID: <xmqq7fbpdhkw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160809040811.21408-8-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 16 quoted lines

> @@ -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.

Show 18 quoted lines

> +		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."