Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2016, 21:25 UTC
- Message-ID
- <xmqqponl84h4.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160930193613.dwpjiw5xps6a3wgj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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.
Show 31 quoted lines
> +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.
Show 5 quoted lines
> + 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.
Show 19 quoted lines
> +/*
> + * 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.
Show 6 quoted lines
> + * 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.