Re: [PATCH] sparse-checkout.txt: new document with sparse-checkout directions

From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Victoria Dye <vdye@github.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>,
	Matheus Tavares <matheus.bernardino@usp.br>,
	ZheNing Hu <adlternative@gmail.com>
Subject: Re: [PATCH] sparse-checkout.txt: new document with sparse-checkout directions
Date: Mon, 26 Sep 2022 20:05:28 -0700	[thread overview]
Message-ID: <CABPp-BFo_0MaBfyhDBfRby4CSHy8brjMY_8nif2S_Aogn-NqUg@mail.gmail.com> (raw)
In-Reply-To: <xmqqwn9qox1z.fsf@gitster.g>

On Mon, Sep 26, 2022 at 10:38 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +    In the case of am and apply, those commands only operate on the
> > +    working tree, so they are kind of in the same boat as stash.
>
> "apply" does not touch the HEAD but it can touch the index; when it
> operates with the "--cached" or the "--index" option, it should not
> be considered as a working-tree-only command.

Ah, right, good flag.  This helps resolve part of my question, but
gives me a new question as well.

Without --cached or --index, I think we'd need to make `apply` behave
like `stash` and just auto-vivify any files being tweaked.  If we
don't, we'll lose changes from the patch.

"apply --cached" could possibly just update the index.  However, it
appears to have another bug I need to add to the known bugs section.
`apply --cached` updates the index, but the new index entry fails to
carry over the "SKIP_WORKTREE" bit, making it appear there is an
unstaged deletion of the file.  (Users can run `git sparse-checkout
reapply` afterwards as a workaround.).  This is slightly weird for
files with conflicts (created when running `git apply -3 --cached`)
since those files with content conflicts will not be present in the
working tree, but that's in line with the fact that `git apply -3
--cached` refuses to touch the working tree in general.

In line with `--cached`, we could have "apply --index" do updates to
both the index and the working copy, while ensuring any
"SKIP_WORKTREE" bits are preserved for non-conflicted files.  However,
would preserving "SKIP_WORKTREE" bits be weird for users?  On one
hand, `git apply` without `--index` auto-vivifies files and `--index`
says to "also apply changes to the index" -- but preserving
SKIP_WORKTREE bits would make the `--index` flag also affect how the
working tree is treated, which might seem odd.  On the other hand,
merge/cherry-pick/rebase will update files in the index while leaving
the file missing from the working tree when not conflicted, so there
is some precedent for such behavior.  The question might just be
whether `git apply --index` should be more like mergy behavior, or
more like `git apply`/`git stash` behavior.

> "am" is about recording what is in the patch as a commit.

Does that mean it should behave like "apply --index"?  Or more like
cherry-pick?  (This question might be moot depending on what we choose
for "apply --index", in particular, it won't matter if we preserve
SKIP_WORKTREE bits on non-conflicted files.)

> > +    Perhaps `git am` could run `git sparse-checkout reapply`
> > +    automatically afterward and move into a category more similar to
> > +    merge/rebase/cherry-pick, but it'd still be weird because it'd
> > +    vivify files besides just conflicted ones when there are conflicts.
>
> I do not particularly think it is so bad.

For some reason I was thinking of running `git sparse-checkout
reapply` only if the `am` operation succeeded, which would give us a
special one-off command treatment.  If we instead view it as always
running `git sparse-checkout reapply` whether or not we hit conflicts,
or equivalently, if we view `git am` preserving SKIP_WORKTREE bits on
non-conflicted files, then I agree it's not weird anymore and can be
classified in the same group as merge/rebase/cherry-pick.

But something else you said confuses me...

> How would we handle the case where the user modifies paths outside
> the sparse specification and makes a commit out of the result,
> without using "am"?  We should be consistent with that use case, i.e.
>
>     $ edit path/outside/sparse/specification
>     $ git add path/outside/sparse/specification
>     $ git commit
>
> Do we require some "Yes, I am aware that I need to widen my sparse
> specification to do this, because I am now stepping out of it, and I
> understand that my sparse specification becomes wider after doing
> this operation" confirmation with "add" or "commit"?  If not, then I
> think "am" should silently widen just like these commands.  If they
> do, then "am" should also require such an option.  Perhaps call it
> "--widen-sparse" or whatever.

The command
    $ edit path/outside/sparse/specification
doesn't make sense to me; the file (and perhaps also its leading
directories) are missing.  Most editors will probably tell you that
you are editing a new file, but then it's more of a "rewrite from
scratch" than an "edit".

Typically, we'd expect users who want to edit such files to do so by
first running the `add` or `set` subcommands of sparse-checkout to
change their sparse specification so that the file becomes present.
But then it's no longer outside the sparse specification.  So, I'm not
sure how this angle could help guide our direction.

> By the way, I like the term "sparse specification" very much, as
> we should worry about non-cone mode as well.  Please use it
> consistently in this document after getting a concensus that it
> is a good phrase to use from others---I saw some other words
> used after "sparse" elsewhere in this patch.

:-)

> > +    In the case of ls-files, `git ls-files -t` is often used to see what
> > +    is sparse and not, in which case restricting would not make sense.
>
> I suspect that leaving it tree-wide would allow scripters come up
> with Porcelains that restricts to the sparse specification more
> easily.