From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: Jonathan Tan <jonathantanmy@google.com>,
Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
jabolopes@google.com
Subject: Re: [PATCH v2] sparse-checkout: create leading directory
Date: Sat, 22 Jan 2022 13:06:02 +0100 [thread overview]
Message-ID: <220122.86tudwht17.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <CABPp-BHgRQ+qNvq19wsBsMCR+Cn7FS+0FMYXVyyzEAZTsg-wjA@mail.gmail.com>
On Fri, Jan 21 2022, Elijah Newren wrote:
> On Fri, Jan 21, 2022 at 6:12 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Fri, Jan 21 2022, Jonathan Tan wrote:
>>
> ...
>> This partial application of the fix-up I suggested in
>> https://lore.kernel.org/git/220120.86mtjqks1b.gmgdl@evledraar.gmail.com/
>> leaves the now-unused "blank-template"
>
> Good point; no need to remove something that isn't being created anymore.
>
>>
>> > - added attribution to Jose Lopes for finding and making the first
>> > draft of this patch (after confirming with them)
>> >
>> > Ævar mentioned "git sparse-checkout add" but I think that that is a
>> > different problem - in the "git sparse-checkout init" case, we could get
>> > into this case with a template that does not have .git/info, but in the
>> > "git sparse-checkout add" case, the user would have had to explicitly
>> > remove the info directory. So I'll limit this patch to the "init" case,
>> > for now.
> ...
>>
>> I agree that it's a slightly different problem, but I was just
>> advocating for us testing what happened in these cases.
>>
>> The below fix-up does that.
>
> Different problem...addressed with a "fix-up"? Why would we squash
> extra testing of a different problem into the same commit? I think
> it'd at least deserve its own commit message.
Sure, or split up, or with an amended commit message etc.
>> I think we should use warning_errno() there
>> instead of some specutalite "file may not exist", but with/without this
>> patch these tests show that only the "init" case was broken.
>>
>> As a more general issue I don't understand why "add" and "init" need to
>> be conceptually different operations. If what defines a sparse checkout
>> is just that it has that file and the 2 default patterns, which unless
>> I'm missing something is the case.
>>
>> Why isn't "add" merely an "init"
>> that'll optimistically add whatever pattern you asked for, in addition
>> to doing an "init" if you didn't already?
>>
>> Then "add" and "init" will share the same error recovery behavior, and
>> you won't needlessly have to run "init/add x" just to start using
>> sparse-checkout with a pattern of "x".
>
> The high level idea you propose (" 'add' can initialize sparsity state
> if not currently in a sparse checkout") could make sense. It isn't
> just a straightforward switchover with the various other config
> settings and such, and would also necessitate adding additional
> command line flags to the 'add' subcommand, but it could be done.
> Didn't occur to me before; I'm not sure if such a change in UI would
> be better or worse. I'm inclined to leave it as it is, though,
> especially since...
>
> The low level idea you propose (sharing code down to error paths) does
> not make sense in this particular case, for reasons that are rather
> non-obvious. In particular, we need to be careful to avoid sharing
> some code with "init". The "init" subcommand is deprecated as of
> ba2f3f58ac ("git-sparse-checkout.txt: update to document
> init/set/reapply changes", 2021-12-14). I initially wanted to remove
> the separate "init" codepath, and just forward "git sparse-checkout
> init" calls over to the "git sparse-checkout set" codepath (the only
> difference being that "init" always comes with an empty set of
> directories/patterns). However, "init" had some problematic behavior
> that I didn't want to copy to "set". I also didn't want to kill that
> behavior in "init" for backwards compatibility reasons. And, this
> xfopen call was tied up in that tangle, which means that it will
> definitely remain separate, and thus needs to be fixed in isolation.
....
>> But I've never *actually* used this command, so maybe I'm just missing
>> something obvious...
>>
>> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
>> index 3189d3da965..6b56d9d177f 100755
>> --- a/t/t1091-sparse-checkout-builtin.sh
>> +++ b/t/t1091-sparse-checkout-builtin.sh
>> @@ -80,11 +80,37 @@ test_expect_success 'git sparse-checkout init' '
>> '
>>
>> test_expect_success 'git sparse-checkout init in empty repo' '
>> - test_when_finished rm -rf empty-repo blank-template &&
>> + test_when_finished rm -rf empty-repo &&
>
> This hunk looks like a good fixup to Jonathan's patch.
>
>> git init --template= empty-repo &&
>> git -C empty-repo sparse-checkout init
>> '
>>
>> +test_expect_success 'git sparse-checkout add -- info/sparse-checkout missing' '
>> + test_when_finished "rm -rf empty" &&
>> + git init --template= empty &&
>> + git -C empty sparse-checkout init &&
>> + rm -rf empty/.git/info &&
>> +
>> + cat >expect <<-\EOF &&
>> + fatal: unable to load existing sparse-checkout patterns
>> + EOF
>> + test_expect_code 128 git -C empty sparse-checkout add bar 2>actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'git sparse-checkout list -- info/sparse-checkout missing' '
>> + test_when_finished "rm -rf empty" &&
>> + git init --template= empty &&
>> + git -C empty sparse-checkout init &&
>> + rm -rf empty/.git/info &&
>> +
>> + cat >expect <<-\EOF &&
>> + warning: this worktree is not sparse (sparse-checkout file may not exist)
>> + EOF
>> + git -C empty sparse-checkout list 2>actual &&
>> + test_cmp expect actual
>> +'
>> +
>
> So...you're trying to test what happens when a user intentionally
> bricks their repository?
I'm just saying that it's cheap to add a regression test for this
missing bit of related coverage, so why not add it?
We need to deal with the real world, a repo might be in all sorts of odd
states, including because of a user mistake.
> (Note that `sparse-checkout init` sets core.sparseCheckout=true, as
> explicitly documented. core.sparseCheckout=true instructs git to pay
> attention to $GIT_DIR/info/sparse-checkout for every unpack_trees()
> call that updates the working tree, which basically means nearly any
> significant Git operation involving a worktree update now needs that
> file in order to function. So, your commands told Git that this
> directory is mandatory, and then you nuked the directory.)
*nod*. But in that case shouldn't the errors say that you've configured
core.sparseCheckout=true but you're missing XYZ file?
> Now, if you could find a testcase based on `git worktree add ...`
> (which doesn't create an "info" directory) and then triggers problems
> somehow without the intentional bricking, then what you'd have would
> be more in line with what Jonathan is addressing here, but as it
> stands it's hard to even call your testcases related. There may be
> some merit to testing deliberately broken repositories, but I'm just
> not sure if that's what you really intended and were suggesting. Was
> it? Or am I just missing something here?
Doesn't the worktree case just use the "main" info/*, e.g. for
info/excludes (didn't have time to test it now).
next prev parent reply other threads:[~2022-01-22 12:08 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-20 18:55 [PATCH] sparse-checkout: create leading directory Jonathan Tan
2022-01-20 20:26 ` Junio C Hamano
2022-01-20 21:30 ` Ævar Arnfjörð Bjarmason
2022-01-22 7:58 ` SZEDER Gábor
2022-01-21 17:44 ` [PATCH v2] " Jonathan Tan
2022-01-21 18:48 ` Ævar Arnfjörð Bjarmason
2022-01-22 5:21 ` Elijah Newren
2022-01-22 12:06 ` Ævar Arnfjörð Bjarmason [this message]
2022-01-22 17:29 ` Elijah Newren
2022-01-24 11:17 ` Ævar Arnfjörð Bjarmason
2022-01-24 18:25 ` Jonathan Tan
2022-01-25 5:37 ` Elijah Newren
2022-01-22 2:33 ` Elijah Newren
2022-01-24 18:22 ` Jonathan Tan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220122.86tudwht17.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jabolopes@google.com \
--cc=jonathantanmy@google.com \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).