Re: [PATCH v2 1/2] tmp-objdir: new API for creating temporary writable databases

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2021, 17:39 UTC
Message-ID: <xmqqr1aphbue.fsf@gitster.g>
In-Reply-To: <20211206085300.GA26699@neerajsi-x1.localdomain>

Neeraj Singh <nksingh85@gmail.com> writes:

Show 18 quoted lines

>> > +	if (tmp_objdir)
>> > +		tmp_objdir_reapply_primary_odb(tmp_objdir, old_cwd, new_cwd);
>> >  	free(path);
>> >  }
>> 
>> This is called during set_git_dir(), which happens fairly early in
>> the set-up sequence.  I wonder if there is a real use case that
>> creates a tmp-objdir that early in the process to require this
>> unapply-reapply sequence.
>> 
>
> The lack of this code was causing a failure, I believe in
> t2107-update-index-basic.sh: "--refresh triggers late setup_work_tree".
>
> This problem came up after applying: https://lore.kernel.org/git/4a40fd4a29a468b9ce320bc7b22f19e5a526fad6.1637020263.git.gitgitgadget@gmail.com/
>
> I thought it would be best to fix this in the tmp-objdir code so that
> callers could plug/unplug bulk checkin without any subtle surprises.

OK, I think that is fine.

As a slightly-related tangent that is outside the topic, I think we should revisit "update-index", which is one of the oldest plumbing commands with its own quirks. I do not offhand see why it needs to sprinkle this many setup_work_tree() calls everywhere. Having an index to work on means we must have a working tree to update and/or refresh from. We should be able to get away with the NEED_WORK_TREE bit in the git.c::commands[] table for this command. If this were a more recent command, I may suspect that there were valid reasons like "in this particular mode, update-index must work inside a bare repository" to force us to take this unusual program structure, but because this is probably a lot older than NEED_WORK_TREE bit, I would not be surprised if the answer were "nobody noticed the ugliness so far".

> Given that this patch series introduces functions with no users, are you
> going to hold off on putting this into 'next' until another next-worthy
> patch series is ready?

Even without any existing callers, as long as we see Reviewed-by: by Elijah, who we know will have to build on top of this series, I think this can and should go to 'next'.

Thanks.