Re: [PATCH 5/8] builtin/clone: move setup of alternates for non-shared local clones

[Toon Claes <toon@iotcl.com>]

Patrick Steinhardt <ps@pks.im> writes:

> Similar as in the preceding commit, move the setup of alternates for
> local clones with "--no-shared" into `collect_alternates()`. With this
> step, the complete setup of alternates is now handled by that function.
>
> Note that besides moving stuff around, it also fixes a bug: previously,
> we did not know to resolve the referenced repository's common directory.
> Consequently, when referencing a worktree we failed to resolve
> alternates. But as `collect_alternates()` already knows to resolve the
> commondir for "--local" we can simply reuse this resolved path for our
> purpose.
>
> Add two tests, the first one of which exercises this bug to avoid future
> regressions. The second patch ensures that we properly handle relative
> alternates for a referenced worktree.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/clone.c            | 34 +++++++++++++++++++++++-----------
>  t/t5604-clone-reference.sh | 25 +++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 08c8f5a94f..2e3473fddf 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -181,7 +181,7 @@ static int add_one_alternate(struct string_list_item *item, void *cb_data)
>  	return 0;
>  }
>  
> -static void copy_alternates(struct strbuf *src, const char *src_repo)
> +static void read_alternates(struct strvec *alternates, const char *src_repo)
>  {
>  	/*
>  	 * Read from the source objects/info/alternates file
> @@ -195,29 +195,41 @@ static void copy_alternates(struct strbuf *src, const char *src_repo)
>  	 * to turn entries with paths relative to the original
>  	 * absolute, so that they can be used in the new repository.
>  	 */
> -	FILE *in = xfopen(src->buf, "r");
> +	FILE *in;
> +	struct strbuf path = STRBUF_INIT;
>  	struct strbuf line = STRBUF_INIT;
>  
> +	strbuf_addf(&path, "%s/objects/info/alternates", src_repo);
> +
> +	in = fopen(path.buf, "r");
> +	if (!in) {
> +		if (errno == ENOENT)
> +			goto out;
> +		die_errno("could not read alternates file '%s'", path.buf);
> +	}
> +
>  	while (strbuf_getline(&line, in) != EOF) {
>  		char *abs_path;
>  		if (!line.len || line.buf[0] == '#')
>  			continue;
>  		if (is_absolute_path(line.buf)) {
> -			odb_add_to_alternates_file(the_repository->objects,
> -						   line.buf);
> +			strvec_push(alternates, line.buf);
>  			continue;
>  		}
>  		abs_path = mkpathdup("%s/objects/%s", src_repo, line.buf);
>  		if (!normalize_path_copy(abs_path, abs_path))
> -			odb_add_to_alternates_file(the_repository->objects,
> -						   abs_path);
> +			strvec_push(alternates, abs_path);
>  		else
>  			warning("skipping invalid relative alternate: %s/%s",
>  				src_repo, line.buf);
>  		free(abs_path);
>  	}
> +
> +out:
> +	strbuf_release(&path);
>  	strbuf_release(&line);
> -	fclose(in);
> +	if (in)
> +		fclose(in);

Why not put this before the `out` label and remove the if?

>  }

-- 
Laters,
Toon