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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 20, 2021, 18:16 UTC
Message-ID: <211220.86zgovt9bi.gmgdl@evledraar.gmail.com>
In-Reply-To: <d2399072-ce9d-b654-42b4-d08d973c488e@gmail.com>

On Mon, Dec 20 2021, Derrick Stolee wrote:

Show 46 quoted lines

> 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.