| Message ID | patch-03.13-784b7947512-20211212T201308Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests + init: don't rely on templates & add --no-template + config | expand |
On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote: > In preceding commits the test suite has been taught to run without a > template directory, but in doing so we needed to fix code that relied > on the "hooks" and "branches" directories. > > The "hooks" code was all specific to our own test suite. The > "branches" directory is intentionally created, but has been "slightly > deprecated" for a while, so it's not created when not using the > default template. > > However "info" is different. Trying to omit its creation would lead to > a lot of test suite failures. Many of these we should arguably fix, > the common pattern being to add an exclude to "info/excludes". This would be painful to add because of the impact on the test suite. That I understand. > 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? > So let's just bite the bullet and make the "info" directory mandatory, > and document it as such. Let's also note that in the documentation > that this doesn't apply to the "hooks" and "branches" directories. I have no objection to this approach, but we should still do the other thing. Thanks, -Stolee
On Mon, Dec 20 2021, Derrick Stolee wrote: > On 12/12/2021 3:13 PM, Ævar Arnfjörð Bjarmason wrote: >> In preceding commits the test suite has been taught to run without a >> template directory, but in doing so we needed to fix code that relied >> on the "hooks" and "branches" directories. >> >> The "hooks" code was all specific to our own test suite. The >> "branches" directory is intentionally created, but has been "slightly >> deprecated" for a while, so it's not created when not using the >> default template. >> >> However "info" is different. Trying to omit its creation would lead to >> a lot of test suite failures. Many of these we should arguably fix, >> the common pattern being to add an exclude to "info/excludes". > > This would be painful to add because of the impact on the test suite. > That I understand. > >> 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. But in practice I think it wouldn't have been a big deal, because while you could omit or specify a custom template it was somewhat of a hassle, and even somewhat undocumented. Whereas with this series allowing you to easily configure it with init.templateDir=false it becomes trivial. That was another motivation of mine for adding this, I'd like to not have N copies of that template crud all over my systems. So I think in practice we need to be more conservative about cross-version interaction here. It's not just a matter of "if", but also of a new "how" making that "if" more common. I.e. needing to interact with an empty-template .git directory. We also have non-git.git code to worry about, e.g. us breaking any user-custom script that might do: #!/bin/sh -e git init "$1" cd "$1" # *Boom* under init.templateDir=false, unless we keep ".git/info" echo <my ignores> >.git/info/excludes So I just don't think it's worth it. Let's just create .git/info unconditionally like we create .git/objects/{info,pack} now. It's unrelated to this, but if this gets in I would eventually like to submit a change to make some version of init.templateDir=false the default. That's a bit more untandling, but I think ultimately beneficial. E.g. the sample hooks are documented in "git help githooks" along with general hook documentation. Such a change would ideally involve splitting that out (maybe to a a "gitrepository-sample-hooks(5)"). That's another reason for why I'd like to make init.templateDir=false play nicely with existing in-tree and out-of-tree expectations. >> So let's just bite the bullet and make the "info" directory mandatory, >> and document it as such. Let's also note that in the documentation >> that this doesn't apply to the "hooks" and "branches" directories. > > I have no objection to this approach, but we should still do the > other thing. All that being said I don't really mind not creating a .git/info if the consensus sways that way. It's a bit more painful when it comes to the tests, but not *that* painful. I had it mostly working before abandoning it for this approach.
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. Thanks, -Stolee
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.
Derrick Stolee <stolee@gmail.com> writes: > 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. Historically, "git init" relied on the templates to create necessary directories, and the subcommands in turn learned to depend on the presence of these directories. At the same time we allowed that the templates can be customized by the end users. It was a bug, exactly for the reason you said above. Before we talk about creating 'info' directory directly in "git init" or anything done in this topic, we should fix the existing bug, and the right fix is to use safe-create-leading-directories. With that, it may become unnecessary to have this "create 'info' in 'init'".
On Mon, Dec 20 2021, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> 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. > > Historically, "git init" relied on the templates to create necessary > directories, and the subcommands in turn learned to depend on the > presence of these directories. > > At the same time we allowed that the templates can be customized by > the end users. It was a bug, exactly for the reason you said above. > > Before we talk about creating 'info' directory directly in "git > init" or anything done in this topic, we should fix the existing > bug, and the right fix is to use safe-create-leading-directories. > > With that, it may become unnecessary to have this "create 'info' in > 'init'". I don't see why we'd consider that as a worthwhile direction to go in. The "git-init" documentation states: This command creates an empty Git repository - basically a `.git` directory with subdirectories for `objects`, `refs/heads`, `refs/tags`, and template files. I.e. we promise to create "objects", but not "objects/{info,pack}", even though we've done so since f49fb35d0d5 (git-init-db: create "pack" subdirectory under objects, 2005-06-27) and d57306c7945 (Create objects/info/ directory in init-db., 2005-08-20). Our test suite reveals our own assumptions, but it's also indicative of assumptions others have made. It's cheap to create .git/info unconditionally, and we create similar empty subdirectories, so why not do it for .git/info? Why would we needlessly break widely documented out-of-tree recipies like: some-user-excludes >.git/info/excludes Which yes, rely on something you can't strictly rely on, but is true enough most of the time that people do. So I don't see what finding and fixing every instance of assuming .git/info in the test suite buys us. After doing that we'd be back to square one of having to decide if exposing a mode that effectively did the same could be overly pedantic in that case, or cheaply cater to out-of-tree code that made the same assumption. That's the question this topic is mainly trying to address. It's also worthwhile to fix the in-tree dependency in sparse-checkout, but I don't see why we'd insist that one can't be done without the other.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I don't see why we'd consider that as a worthwhile direction to go > in. The "git-init" documentation states: > > This command creates an empty Git repository - basically a `.git` > directory with subdirectories for `objects`, `refs/heads`, > `refs/tags`, and template files. > > I.e. we promise to create "objects", but not "objects/{info,pack}", even > though we've done so since f49fb35d0d5 (git-init-db: create "pack" > subdirectory under objects, 2005-06-27) and d57306c7945 (Create > objects/info/ directory in init-db., 2005-08-20). I view it as a documentation bug, though. The only thing we need to promise is that (1) the directory prepared by "git init" will be recognised as a repository by the current and future versions of Git (and hopefully the existing ones, too), and (2) versions of Git will not barf due to missing directory and stuff due to end-user mischief around custom templates. I do not think we even need refs/heads/ directory and I do not see that, especially with a presence with "basically" there, the sentence promises that we will always create refs/tags/ directory. For (1), only objects/, refs/ and HEAD is necessary, as long as (2) is satisfied by lazy creation of leading paths. We would want to be able to cope with a repository that lost .git/info directory due to loss of it in custom templates anyway. Just like we create leading missing directories lazily for any hierarchy under .git/, we should create .git/info on-demand. Pre-creating .git/info might be nice, but becomes much less important after it happens, and that is why I view it as a much lower priority than what Derrick suggested.
On Mon, Dec 20 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> I don't see why we'd consider that as a worthwhile direction to go >> in. The "git-init" documentation states: >> >> This command creates an empty Git repository - basically a `.git` >> directory with subdirectories for `objects`, `refs/heads`, >> `refs/tags`, and template files. >> >> I.e. we promise to create "objects", but not "objects/{info,pack}", even >> though we've done so since f49fb35d0d5 (git-init-db: create "pack" >> subdirectory under objects, 2005-06-27) and d57306c7945 (Create >> objects/info/ directory in init-db., 2005-08-20). > > I view it as a documentation bug, though. > > The only thing we need to promise is that (1) the directory prepared > by "git init" will be recognised as a repository by the current and > future versions of Git (and hopefully the existing ones, too), and > (2) versions of Git will not barf due to missing directory and stuff > due to end-user mischief around custom templates. I do not think we > even need refs/heads/ directory and I do not see that, especially > with a presence with "basically" there, the sentence promises that > we will always create refs/tags/ directory. For (1), only objects/, > refs/ and HEAD is necessary, as long as (2) is satisfied by lazy > creation of leading paths. Do you think that we should stop creating objects/{pack,info} then, provided that the few test failures that assume them are cleand up (which for at least one of them is much easier than the top-level "info")? > We would want to be able to cope with a repository that lost > .git/info directory due to loss of it in custom templates anyway. > Just like we create leading missing directories lazily for any > hierarchy under .git/, we should create .git/info on-demand. > > Pre-creating .git/info might be nice, but becomes much less > important after it happens, and that is why I view it as a much > lower priority than what Derrick suggested. Yes, we agree that we should deal with it not being created, that we don't is a bug in sparse-checkout. That's still a separate question from whether it's worthwhile investment of both our and user time to skip creating them and deal with the resulting churn in the tests and needlessly break code in the wild. It seems as though you're saying that any fixes or changes in this area would be incomplete without moving us towards the most pedantic and minimalist interpretation possible when doing a "git init", is that right? I.e. to end up with this .git from "git init --template=" (the same as now plus rmdir .git/{objects,refs}/*): $ tree -a -p . └── [drwxr-xr-x] .git ├── [-rw-r--r--] config ├── [-rw-r--r--] HEAD ├── [drwxr-xr-x] objects └── [drwxr-xr-x] refs 3 directories, 2 files I can work towards that with these patches, it just seems to me to be needlessly creating potential issues for little or no benefit.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > It seems as though you're saying that any fixes or changes in this area > would be incomplete without moving us towards the most pedantic and > minimalist interpretation possible when doing a "git init", is that > right? Not at all. Belt and suspenders is a good way forward. I am just saying that on-demand creation is more important than the other one.
On Mon, Dec 20 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> It seems as though you're saying that any fixes or changes in this area >> would be incomplete without moving us towards the most pedantic and >> minimalist interpretation possible when doing a "git init", is that >> right? > > Not at all. Belt and suspenders is a good way forward. I am just > saying that on-demand creation is more important than the other one. So you do agree that we should create .git/info whatever the template says (as this series does), or not?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Mon, Dec 20 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> It seems as though you're saying that any fixes or changes in this area >>> would be incomplete without moving us towards the most pedantic and >>> minimalist interpretation possible when doing a "git init", is that >>> right? >> >> Not at all. Belt and suspenders is a good way forward. I am just >> saying that on-demand creation is more important than the other one. > > So you do agree that we should create .git/info whatever the template > says (as this series does), or not? Probably "should" is too strong a word. I do not mind "mkdir -p" existing, but I think it is more important to spend our engineering effort to make sure there are necessary on-demand creation of leading paths.
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. Just so we'll avoid stepping on each other's toes, what's the status of your plan/non-plan to work on that more isolated fix, perhaps you have one that's unsubmitted?
On 1/12/2022 7:42 AM, Ævar Arnfjörð Bjarmason wrote: > > 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. > > Just so we'll avoid stepping on each other's toes, what's the status of > your plan/non-plan to work on that more isolated fix, perhaps you have > one that's unsubmitted? I do not have one that is unsubmitted. I was hoping that you would include it in a v2 to this series. I might have been quicker to volunteer to create one had I not been sidelined for two weeks, but right now I have a lot to catch up on so don't have the time. Thanks, -Stolee
On Tue, Jan 18 2022, Derrick Stolee wrote: > On 1/12/2022 7:42 AM, Ævar Arnfjörð Bjarmason wrote: >> >> 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. >> >> Just so we'll avoid stepping on each other's toes, what's the status of >> your plan/non-plan to work on that more isolated fix, perhaps you have >> one that's unsubmitted? > > I do not have one that is unsubmitted. I was hoping that you would > include it in a v2 to this series. I might have been quicker to > volunteer to create one had I not been sidelined for two weeks, but > right now I have a lot to catch up on so don't have the time. Good to hear that you're better (or back?, not 100% sure what "sidelined" here means). I'll try to get to re-rolling it with a fix for that sparse-checkout issue, hopefully sooner than later. Just wanted to avoid potential duplicate work. Thanks.
diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt index 1a2ef4c1505..eb58ab08817 100644 --- a/Documentation/gitrepository-layout.txt +++ b/Documentation/gitrepository-layout.txt @@ -160,7 +160,10 @@ branches:: and not likely to be found in modern repositories. This directory is ignored if $GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/branches" will be used instead. - ++ +This directory is created by the default linkgit:git-init[1] +template. It will not be created when using a custom template that +doesn't contain it. hooks:: Hooks are customization scripts used by various Git @@ -171,6 +174,10 @@ hooks:: Read linkgit:githooks[5] for more details about each hook. This directory is ignored if $GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/hooks" will be used instead. ++ +This directory is created by the default linkgit:git-init[1] +template. It will not be created when using a custom template that +doesn't contain it. common:: When multiple working trees are used, most of files in @@ -190,6 +197,14 @@ info:: Additional information about the repository is recorded in this directory. This directory is ignored if $GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/info" will be used instead. ++ +This directory is created by the default linkgit:git-init[1] +template. ++ +It will be created even when when using a custom template that doesn't +contain it. On older versions of git this was not the case, as various +tools came to rely on its creation (including parts of git itself) +it's now unconditionally (re-)created on 'git init'. info/refs:: This file helps dumb transports discover what refs are diff --git a/builtin/init-db.c b/builtin/init-db.c index 3cf834eddd2..75495c9c8c6 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -222,6 +222,12 @@ static int create_default_files(const char *original_git_dir, struct strbuf err = STRBUF_INIT; const char *work_tree = get_git_work_tree(); + /* + * We may not have a info/ if the template explicitly omitted + * it. + */ + safe_create_dir(git_path("info"), 1); + /* * We must make sure command-line options continue to override any * values we might have just re-read from the config.
In preceding commits the test suite has been taught to run without a template directory, but in doing so we needed to fix code that relied on the "hooks" and "branches" directories. The "hooks" code was all specific to our own test suite. The "branches" directory is intentionally created, but has been "slightly deprecated" for a while, so it's not created when not using the default template. However "info" is different. Trying to omit its creation would lead to a lot of test suite failures. Many of these we should arguably fix, the common pattern being to add an exclude to "info/excludes". 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. So let's just bite the bullet and make the "info" directory mandatory, and document it as such. Let's also note that in the documentation that this doesn't apply to the "hooks" and "branches" directories. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/gitrepository-layout.txt | 17 ++++++++++++++++- builtin/init-db.c | 6 ++++++ 2 files changed, 22 insertions(+), 1 deletion(-)