Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> diff --git a/sha1_file.c b/sha1_file.c
> index 9a79c19..65deaf9 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -421,6 +421,12 @@ void add_to_alternates_file(const char *reference)
>  	free(alts);
>  }
>  
> +void add_to_alternates_internal(const char *reference)
> +{
> +	prepare_alt_odb();
> +	link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
> +}
> +

A function _internal being extern felt a bit funny.  We are only
appending so the first one does not have to be reprepare.

> +static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)
> +{
> +	int err;
> +
> +	if (!t)
> +		return 0;
> +
> +	if (t == the_tmp_objdir)
> +		the_tmp_objdir = NULL;
> +
> +	/*
> +	 * This may use malloc via strbuf_grow(), but we should
> +	 * have pre-grown t->path sufficiently so that this
> +	 * doesn't happen in practice.
> +	 */
> +	err = remove_dir_recursively(&t->path, 0);
> +
> +	/*
> +	 * When we are cleaning up due to a signal, we won't bother
> +	 * freeing memory; it may cause a deadlock if the signal
> +	 * arrived while libc's allocator lock is held.
> +	 */
> +	if (!on_signal)
> +		tmp_objdir_free(t);
> +	return err;
> +}
> +
> +int tmp_objdir_destroy(struct tmp_objdir *t)
> +{
> +	return tmp_objdir_destroy_1(t, 0);
> +}

Looks sensible.

> +	t = xmalloc(sizeof(*t));
> +	strbuf_init(&t->path, 0);
> +	argv_array_init(&t->env);
> +
> +	strbuf_addf(&t->path, "%s/incoming-XXXXXX", get_object_directory());

I was wondering where you would put this in.  Inside .git/objects/
sounds good.

> +/*
> + * Make sure we copy packfiles and their associated metafiles in the correct
> + * order. All of these ends_with checks are slightly expensive to do in
> + * the midst of a sorting routine, but in practice it shouldn't matter.
> + * We will have a relatively small number of packfiles to order, and loose
> + * objects exit early in the first line.
> + */
> +static int pack_copy_priority(const char *name)
> +{
> +	if (!starts_with(name, "pack"))
> +		return 0;
> +	if (ends_with(name, ".keep"))
> +		return 1;
> +	if (ends_with(name, ".pack"))
> +		return 2;
> +	if (ends_with(name, ".idx"))
> +		return 3;
> +	return 4;
> +}

Thanks for being careful.  A blind "cp -r" would have ruined the
day.

We do not do bitmaps upon receiving, I guess.

> + *	struct tmp_objdir *t = tmp_objdir_create();
> + *	if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
> + *	    !tmp_objdir_migrate(t))
> + *		printf("success!\n");
> + *	else
> + *		die("failed...tmp_objdir will clean up for us");

Made me briefly wonder if a caller might want to use appropriate
environment to use the tmp-objdir given by the API in addition to
its own, but then such a caller just needs to prepare its own argv-array
and concatenate tmp_objdir_env() before making the opt_cd_env call,
so this is perfectly fine.