On Fri, Sep 30, 2016 at 02:25:43PM -0700, Junio C Hamano wrote:
> > +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.
It's a match for add_to_alternates_file(). Suggestions for a better word
are welcome.
We do need to prepare_alt_odb(), as that is what sets up the
alt_odb_tail pointer. And also, a later prepare() call would overwrite
our entry. We could refactor the alt_odb code, but it seemed simplest
to just make sure we don't add to an unprepared list.
> > + 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.
The name "incoming" is kind of arbitrary and related to the fact that
this is used for receive-pack (though if we were to use it on the
fetching side, I think it would be equally correct). I don't think it
really matters in practice.
> > +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.
But we don't, but they (and anything else) would just sort at the end,
which is OK.
> > + * 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.
Yep, and that's exactly what happens in one spot of the next patch.
My original had just open-coded, but I was happy to see we have
argv_array_pushv() these days, so it's a one-liner.
In the very original version, the receive-pack process did not need to
access the new objects at all (not until ref update time anyway, at
which point they've been migrated). And that's why the environment is
intentionally kept separate, and the caller can feed it to whichever
sub-programs it chooses. But a later version of git that handled shallow
pushes required receive-pack to actually look at the objects, and I
added the add_to_alternates_internal() call you see here.
At that point, it does make me wonder if a better interface would be for
tmp_objdir to just set up the environment variables in the parent
process in the first place, and then restore them upon
tmp_objdir_destroy(). It makes things a bit more automatic, which makes
me hesitate, but I think it would be fine for receive-pack.
I dunno. I mostly left it alone because I did it this way long ago, and
it wasn't broke. Polishing for upstream is an opportunity to fix old
oddities, but I think there is some value in applying a more
battle-tested patch.
-Peff