Re: [PATCH 03/13] init: unconditionally create the "info" directory

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Dec 20 2021, Derrick Stolee wrote:

> On 12/20/2021 11:13 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Dec 20 2021, Derrick Stolee wrote:
>> 
>>> On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote:
>>>> But we've also grown a hard dependency on this directory within git
>>>> itself. Since 94c0956b609 (sparse-checkout: create builtin with 'list'
>>>> subcommand, 2019-11-21) released with v2.25.0 the "git
>>>> sparse-checkout" command has wanted to add exclusions to
>>>> "info/sparse-checkout". It didn't check or create the leading
>>>> directory, so if it's omitted the command will die.
>>>
>>>> Even if that behavior were fixed we'd be left with older versions of
>>>> "git" dying if that was attempted if they used a repository
>>>> initialized without a template.
>>>
>>> This, I don't understand. Why can't we add a
>>> safe_create_leading_directories() to any place where we try to
>>> create a sparse-checkout file?
>>>
>>> This would fix situations where older versions were init'd with a
>>> different template or if the user deleted the info dir. The change
>>> you've made here doesn't fix those cases, which is what you are
>>> claiming is the reason to not do the other fix that seems like it
>>> would.
>>>
>>> What am I misunderstanding here?
>> 
>> I'll clarify that a bit in any re-roll.
>> 
>> Pedantically nothing changes, i.e. you can create a repository with an
>> empty template now, and it'll break on both the sparse-checkout on that
>> version, and any previous version that had that un-noticed issue.
>
> You continue after this with more motivations for adding 'init' 
> unconditionally, which I am not fighting.
>
> What I _am_ saying is important is that if we are trying to write
> a file to a known location and its parent directory doesn't exist,
> then we should create it. Not doing so is a bug and should be
> fixed, no matter how rare such a thing is to occur. As you've
> shown, it is not required to have an info directory until we need
> one (e.g. for sparse-checkout or an excludes file).
>
> If you're not planning to add that to this series, then I'll add it
> to my list. I do think it would fit well into this one, though.

Ah, you mean for the case where "git sparse-checkout" will fail because
.git/info/ doesn't exist now (e.g. because it's an existing repo with
--template=).

Yes that bug will not be fixed by this series. I'd welcome a patch to
fix it & could even integrate it with this series.

But I just found it rather "meh" and not that worth fixing. I.e. that it
hasn't been noticed or reported until now shows that this is a very rare
mode of operation. It seems most people just "git init" or "git clone",
or maybe almost nobody uses "sparse-checkout". I don't know.

I did try to fix it in an earlier version of this.

Maybe I went overboard with that, but I didn't just sprinkle
safe_create_leading_directories() to every caller as you suggest, but
rather wanted to change the API so that cases like:

        sparse_filename = get_sparse_checkout_filename();
        res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL, 0);
        free(sparse_filename);

Would just call some function saying "add to sparse excludes", and not
care about the filename. Since having the API at that levels means you
end up with a lot of accounting work of getting the filename, freeing it
etc.

E.g. this as another example from add_patterns_cone_mode() (the function
makes no other use of sparse_filename):

	char *sparse_filename = get_sparse_checkout_filename();
        [...]
        if (add_patterns_from_file_to_list(sparse_filename, "", 0,
                                           &existing, NULL, 0))
                die(_("unable to load existing sparse-checkout patterns"));
        free(sparse_filename);

But per the upthread rationale I figured "meh" on that and that just
changing "git init" would fix the problem going forward in practice, so
I didn't pursue it.

Won't unconditionally adding a safe_create_leading_directories() also
close the door to having a core.sparseCheckoutFile similar to
core.excludesFile (but maybe it makes no sense). I.e. we'd be free to
"mkdir -p" the .git/info, but if the user is pointing it to some other
path then blindly creating that possibly nested path might be confusing,
as opposed to just immediately erroring out.

So...

All of which is to say that if you'd like to untangle it I'll review it,
and would be happy to include it in this series. But for the
--no-template change I thought I'd just try to sidestep that particular
aspect.