git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Matheus Tavares Bernardino <matheus.bernardino@usp.br>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v2 0/5] Sparse Index: Integrate with 'git add'
Date: Thu, 29 Jul 2021 10:49:32 -0400	[thread overview]
Message-ID: <2a24488f-d0c8-d1f4-7115-36ac0745658d@gmail.com> (raw)
In-Reply-To: <CABPp-BGUTg=GarkhP0MwjWKWmDyRJiEL2J75wFz52y2xi_50mw@mail.gmail.com>

On 7/28/2021 10:57 PM, Elijah Newren wrote:
> On Wed, Jul 28, 2021 at 8:03 PM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 7/28/2021 7:13 PM, Elijah Newren wrote:
>>> On Mon, Jul 26, 2021 at 9:18 AM Derrick Stolee via GitGitGadget
>>> <gitgitgadget@gmail.com> wrote:
>> ...
>>>>  * a full proposal for what to do with "git (add|mv|rm)" and paths outside
>>>>    the cone is delayed to another series (with an RFC round) because the
>>>>    behavior of the sparse-index matches a full index with sparse-checkout.
>>>
>>> I think this makes sense.
>>>
>>> I've read through the patches, and I like this version...with one
>>> exception.  Can we mark the test added in patch 1 under
>>>
>>>      # 3. Rename the file to another sparse filename and
>>>      #    accept conflict markers as resolved content.
>>>
>>> as NEEDSWORK or even MAYNEEDWORK?
>>
>> I have no objection to adding a blurb such as:
>>
>>         # NEEDSWORK: allowing adds outside the sparse cone can be
>>         # confusingto users, as the file can disappear from the
>>         # worktree without warning in later Git commands.
>>
> 
> Sounds great to me other than the simple typo (s/confusingto/confusing to/)
> 
>> And perhaps I'm misunderstanding the situation a bit, but that
>> seems to apply not just to this third case, but all of them. I
>> don't see why the untracked case is special compared to the
>> tracked case. More investigation may be required on my part.
> 
> The possible cases for files outside the sparsity patterns are:
>   a) untracked
>   b) tracked and SKIP_WORKTREE
>   c) tracked and !SKIP_WORKTREE (e.g. because merge conflicts)
> 
> From the above set, we've been talking about untracked and I think
> we're on the same page about those.  Case (b) was already corrected by
> Matheus a number of releases back; git-add will throw an error
> explaining the situation and prevent the adding.  The error tells the
> user to expand their sparsity set to work on those files.  For case
> (c), you are right that those are problematic in the same way (they
> can disappear later after a git-add)...but we're also in the situation
> where the only way to get rid of the conflicting stages is to run git
> add.  So, in my mind, case (c) puts us between a rock and a hard
> place, and we probably need to allow the git-add.

I appreciate this additional context. Thanks.
 
>>>  I'm still quite unconvinced that it
>>> is testing for correct behavior, and don't want to paint ourselves
>>> into a corner.  In particular, we don't allow folks to "git add
>>> $IGNORED_FILE" without a --force override because it's likely to be a
>>> mistake.
>>
>> I agree about ignored files, and that is true whether or not they
>> are in the sparse cone.
> 
> Yes, and...
> 
>>> I think the same logic holds for adding untracked files
>>> outside the sparsity cone.
> 
> In my opinion, "outside the sparsity cone" is another form of "being
> ignored", and in my mind should be treated similarly -- it should
> generally require an override to add such files.  (Case (c) possibly
> being an exception, though maybe even it shouldn't be.)

I don't hold that same interpretation. I think of it instead as
"hidden" files, but they still matter. I also think that advising
one to adjust their sparsity patterns might be dangerous because
not all users know the ramifications of doing that. They might
accidentally download an enormous amount of data to correct a
single file.

Having an override seems like the best option, and we can hopefully
make it consistent across all the cases and commands.

...

> Trying to get out of a corner we paint ourselves into with
> sparse-checkout would be massively harder, which is why I keep harping
> on this kind of thing.  I'm very concerned it's happening even despite
> my numerous comments and worries about it.
...
> I'm totally fine with such changes not being part of this series.  I
> just don't want a test_expect_success that checks for behavior that I
> consider buggy unless it comes with a disclaimer that it's checking
> for existing rather than expected behavior.

I understand your perspective. I'll send a v3 soon that adds a
comment on top of the entire test signalling the things we talked
about here: this is a documentation of behavior, not an endorsement,
and we should probably change it because users can get confused.

Thanks,
-Stolee

  reply	other threads:[~2021-07-29 14:49 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21 21:06 [PATCH " Derrick Stolee via GitGitGadget
2021-07-21 21:06 ` [PATCH 1/5] t1092: test merge conflicts outside cone Derrick Stolee via GitGitGadget
2021-07-23 17:34   ` Elijah Newren
2021-07-23 17:44     ` Eric Sunshine
2021-07-23 17:47       ` Elijah Newren
2021-07-26 14:10     ` Derrick Stolee
2021-07-21 21:06 ` [PATCH 2/5] add: allow operating on a sparse-only index Derrick Stolee via GitGitGadget
2021-07-21 22:19   ` Junio C Hamano
2021-07-21 22:50     ` Derrick Stolee
2021-07-23 17:45   ` Elijah Newren
2021-07-26 13:11     ` Derrick Stolee
2021-07-26 13:33     ` Derrick Stolee
2021-07-21 21:06 ` [PATCH 3/5] pathspec: stop calling ensure_full_index Derrick Stolee via GitGitGadget
2021-07-23 18:17   ` Elijah Newren
2021-07-21 21:06 ` [PATCH 4/5] t1092: 'git add --refresh' difference with sparse-index Derrick Stolee via GitGitGadget
2021-07-21 21:06 ` [PATCH 5/5] add: ignore outside the sparse-checkout in refresh() Derrick Stolee via GitGitGadget
2021-07-23 19:46   ` Elijah Newren
2021-07-23 12:51 ` [PATCH 0/5] Sparse Index: Integrate with 'git add' Elijah Newren
2021-07-23 20:10   ` Elijah Newren
2021-07-26 15:18 ` [PATCH v2 " Derrick Stolee via GitGitGadget
2021-07-26 15:18   ` [PATCH v2 1/5] t1092: test merge conflicts outside cone Derrick Stolee via GitGitGadget
2021-07-26 15:18   ` [PATCH v2 2/5] add: allow operating on a sparse-only index Derrick Stolee via GitGitGadget
2021-07-26 15:18   ` [PATCH v2 3/5] pathspec: stop calling ensure_full_index Derrick Stolee via GitGitGadget
2021-07-26 15:18   ` [PATCH v2 4/5] add: ignore outside the sparse-checkout in refresh() Derrick Stolee via GitGitGadget
2021-07-26 15:18   ` [PATCH v2 5/5] add: remove ensure_full_index() with --renormalize Derrick Stolee via GitGitGadget
2021-07-28 23:13   ` [PATCH v2 0/5] Sparse Index: Integrate with 'git add' Elijah Newren
2021-07-29  2:03     ` Derrick Stolee
2021-07-29  2:57       ` Elijah Newren
2021-07-29 14:49         ` Derrick Stolee [this message]
2021-07-30 12:52           ` Elijah Newren
2021-07-29 14:52   ` [PATCH v3 " Derrick Stolee via GitGitGadget
2021-07-29 14:52     ` [PATCH v3 1/5] t1092: test merge conflicts outside cone Derrick Stolee via GitGitGadget
2021-07-29 14:52     ` [PATCH v3 2/5] add: allow operating on a sparse-only index Derrick Stolee via GitGitGadget
2021-07-29 14:52     ` [PATCH v3 3/5] pathspec: stop calling ensure_full_index Derrick Stolee via GitGitGadget
2021-07-29 14:52     ` [PATCH v3 4/5] add: ignore outside the sparse-checkout in refresh() Derrick Stolee via GitGitGadget
2021-07-29 14:52     ` [PATCH v3 5/5] add: remove ensure_full_index() with --renormalize Derrick Stolee via GitGitGadget
2021-07-29 14:58     ` [PATCH v3 0/5] Sparse Index: Integrate with 'git add' Elijah Newren
2021-07-29 23:00     ` Junio C Hamano

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=2a24488f-d0c8-d1f4-7115-36ac0745658d@gmail.com \
    --to=stolee@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=matheus.bernardino@usp.br \
    --cc=newren@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v2 0/5] Sparse Index: Integrate with '\''git add'\''' \
    /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

Code repositories for project(s) associated with this 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).