git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Matheus Tavares Bernardino <matheus.bernardino@usp.br>,
	Derrick Stolee <stolee@gmail.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 0/5] Sparse Index: Integrate with 'git add'
Date: Fri, 23 Jul 2021 05:51:52 -0700	[thread overview]
Message-ID: <CABPp-BGBwbV7x8i=OVbPnMgWcfdLrEh3-yUi=Amw+XBwpf7A6Q@mail.gmail.com> (raw)
In-Reply-To: <pull.999.git.1626901619.gitgitgadget@gmail.com>

Hi,

I haven't read the series yet (I'm going to try to read them all
today), but wanted to comment on a couple things in your cover
letter...

On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This patch series re-submits the 'git add' integration with sparse-index.
> The performance gains are the same as before.
>
> It is based on ds/commit-and-checkout-with-sparse-index.
>
> This series was delayed from its initial submission for a couple reasons.
>
> The first was because it was colliding with some changes in
> mt/add-rm-in-sparse-checkout, so now we are far enough along that that
> branch is in our history and we can work forwards.
>
> The other concern was about how 'git add ' should respond when a path
> outside of the sparse-checkout cone exists. One recommendation (that I am
> failing to find a link to the message, sorry) was to disallow adding files
> that would become index entries with SKIP_WORKTREE on. However, as I worked

I think the recommendation was:
  * Permitted: running git add on tracked files with the SKIP_WORKTREE
bit *clear*
  * Disallowed: running git add on tracked files with the
SKIP_WORKTREE bit *set*
  * Disallowed: running git add on untracked files that would become
index entries with the SKIP_WORKTREE bit set

where the latter two exit with an error message that suggests changing
the sparsity specification first.  I think this is what has existed
for some time, other than Matheus adding some error messages to help
the user when their add command doesn't match anything otherwise.

> towards that goal I found that change would cause problems for a realistic
> scenario: merge conflicts outside of the sparse-checkout cone.

...which wouldn't be a problem because these are tracked files whose
SKIP_WORKTREE bit was cleared by the merge machinery (when it marked
them as conflicted and wrote them to the working tree).

> The first patch of this series adds tests that create merge conflicts
> outside of the sparse cone and then presents different ways a user could
> resolve the situation. We want all of them to be feasible, and this
> includes:
>
>  1. Reverting the file to a known version in history.
>  2. Adding the file with its contents on disk.
>  3. Moving the file to a new location in the sparse directory.
>
> Without maintaining the behavior of adding files outside of the
> sparse-checkout cone, we risk confusing users who get into this state. The
> only workaround they would have is to modify their sparse-checkout
> definition which might lead to expanding much more data than they need to
> resolve the conflicts.
>
> For that reason, I stopped trying to limit 'git add' to be within the cone.
> I'm open to suggestions here, but we need an approach that works for
> out-of-cone conflicts.

I believe my above suggestion works for out-of-cone conflicts.  Some
important other details to keep in mind in regards to how we make add
behave:

* We don't want "git add -A [GLOB_OR_DIRECTORY]" to accidentally be
treated as a directive to remove files from the repository (and
naively noticing that SKIP_WORKTREE files are missing but attempting
to operate on them anyway would give this problematic result).
* We don't want "git rm [GLOB_OR_DIRECTORY]" to nuke SKIP_WORKTREE
files; it should only operate on files that are present.
* We need add and rm to be consistent with each other in terms of how
SKIP_WORKTREE files and the sparsity cone are treated

and more generally about not-SKIP_WORKTREE-despite-not-matching-sparsity-paths:

* These files that aren't SKIP_WORKTREE but normally would be are
prone to "disappear" at some random later time after they are made to
match the index.  The disappearing can happen either with an explicit
"git sparse-checkout reapply" (which is fine since it was explicit) or
as a side-effect of various other commands that run through
unpack_trees() since it attempts to heed the sparsity rules.  Users
tend to get confused by the latter case; they'll understand at some
point that it was because the file was outside the sparsity paths, but
the randomness in when it's pulled out as a side-effect of other
commands can be slightly jarring.  So, I'd like to avoid that where we
can easily do so, which I think the recommendation above does.  (As a
side note to these kinds of cases, maybe we want to consider having
the sparsification logic in unpack_trees() first check whether paths
being removed from the working tree and having their SKIP_WORKTREE bit
set not only match the index but also match HEAD?  That'd be a bit
more expensive to check in the sparsification paths, and I'm not sure
they all have the relevant information, but it's an idea...)

> The one place I did continue to update is 'git add --refresh ' to match the
> behavior added by mt/add-rm-in-sparse-checkout which outputs an error
> message. This happens even when the file exists in the working directory,
> but that seems appropriate enough.
>
> Thanks, -Stolee
>
> Derrick Stolee (5):
>   t1092: test merge conflicts outside cone
>   add: allow operating on a sparse-only index
>   pathspec: stop calling ensure_full_index
>   t1092: 'git add --refresh' difference with sparse-index
>   add: ignore outside the sparse-checkout in refresh()
>
>  builtin/add.c                            | 13 ++++-
>  pathspec.c                               |  2 -
>  t/t1092-sparse-checkout-compatibility.sh | 71 ++++++++++++++++++++----
>  3 files changed, 72 insertions(+), 14 deletions(-)
>
>
> base-commit: 71e301501c88399711a1bf8515d1747e92cfbb9b
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-999%2Fderrickstolee%2Fsparse-index%2Fadd-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-999/derrickstolee/sparse-index/add-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/999
> --
> gitgitgadget

  parent reply	other threads:[~2021-07-23 12:52 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21 21:06 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 ` Elijah Newren [this message]
2021-07-23 20:10   ` [PATCH 0/5] Sparse Index: Integrate with 'git add' 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
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='CABPp-BGBwbV7x8i=OVbPnMgWcfdLrEh3-yUi=Amw+XBwpf7A6Q@mail.gmail.com' \
    --to=newren@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=stolee@gmail.com \
    --subject='Re: [PATCH 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).