git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <stolee@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>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 0/5] Sparse index: integrate with commit and checkout
Date: Sat, 17 Jul 2021 08:37:07 -0700	[thread overview]
Message-ID: <CABPp-BGCnV-xHH-+S58pqFFhPbPj_0Rt=_QUf_ShAbTCyW9deA@mail.gmail.com> (raw)
In-Reply-To: <c127ceed-10fd-9ad0-e858-db79bec0cf8d@gmail.com>

On Fri, Jul 16, 2021 at 6:59 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 7/12/2021 2:46 PM, Derrick Stolee wrote:
> > On 7/9/2021 5:26 PM, Elijah Newren wrote:
> >> On Mon, Jun 28, 2021 at 7:13 PM Derrick Stolee via GitGitGadget
> >> <gitgitgadget@gmail.com> wrote:
> >>>
> ...
> > Further, I expect it to be simpler to modify the behavior
> > here to match the full checkout case than to make the
> > sparse-index case match the normal sparse-checkout case.
> > The "natural" thing would be to keep the staged "folder1/"
> > directory, but that would present as adding all contained
> > content, not just the single staged entry.
> Taking a closer look at the full checkout case, I discovered that the
> 'git checkout df-conflict' command succeeds in the full checkout case if I
> apply it directly to the 'master' branch. In that situation, it completely
> removes the staged change to folder1/edited-content! This seems like
> incorrect behavior, and has nothing to do with the sparse-checkout feature.

I was not able to reproduce.  Do you have other modifications to git,
or is there some other special setup required to trigger the bug that
I am missing in reading the paragraph above?  Here's what I see:

<Add an "exit 1 &&" right after "init_repos &&" in the 'diff with
directory/file conflicts' test, run until first failure, then:

$ cd trash directory.t1092-sparse-checkout-compatibility/full-checkout
$ git reset --hard
$ git checkout rename-in-to-out
$ echo more stuff >>folder1/edited-content
$ git add -u
$ git checkout df-conflict
error: Your local changes to the following files would be overwritten
by checkout:
folder1/edited-content
Please commit your changes or stash them before you switch branches.
Aborting

This looks like the expected behavior to me, and is what I'd also
expect from the sparse-checkout and sparse-index cases.

> It just happens that a sparse-checkout will have a _different_ kind of
> incorrect behavior!
>
> However, when adding the test on top of the ds/status-with-sparse-index
> branch, the full checkout case matches the sparse-checkout! I bisected
> this to the additions of files adjacent to folder1/ (folder1. folder1-,
> etc) in e669ffb (t1092: expand repository data shape, 2021-07-14). If I
> switch the test to conflict on folder2, then I get the strange behavior
> that I was noticing on 'master'.
>
> Some very subtle things are going on here, and they don't necessarily
> involve the sparse index. Adding the sparse index to the mix creates a
> third incorrect behavior to this already-broken case.
>
> If we agree that the correct thing to do here is to reject the merge and
> fail the command, then I can start working on making that change in
> isolation (because _none_ of the existing behaviors are correct).

Yes, rejecting the merge is the correct behavior.  This is implied by
the existing documentation for both the --merge and --force options to
checkout.

> That leaves a question as to whether we should hold up this series for
> that reason, or if I should pursue a fix to this kind of conflict as a
> forward fix on top of it. What do you think, Elijah and Junio?

I only dug in and found the sparse-checkout/sparse-index bugs because
the D/F changes you made to twoway_merge() looked clearly wrong to me
and I was trying to find a case that would demonstrate it and make it
easier for you to fix up.  I still think the patch is wrong and that
it adds a bug.  If you can drop that patch, and still get correct
behavior in your tests, then I think we can ignore other bugs in this
area, but I'm not happy with that particular patch.  If you need that
patch, then it needs to be corrected, which probably means figuring
out all these bugs.

  reply	other threads:[~2021-07-17 15:37 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-29  2:13 [PATCH 0/5] Sparse index: integrate with commit and checkout Derrick Stolee via GitGitGadget
2021-06-29  2:13 ` [PATCH 1/5] p2000: add 'git checkout -' test and decrease depth Derrick Stolee via GitGitGadget
2021-06-29  2:13 ` [PATCH 2/5] p2000: compress repo names Derrick Stolee via GitGitGadget
2021-06-29  2:13 ` [PATCH 3/5] commit: integrate with sparse-index Derrick Stolee via GitGitGadget
2021-06-29  2:13 ` [PATCH 4/5] sparse-index: recompute cache-tree Derrick Stolee via GitGitGadget
2021-06-29  2:13 ` [PATCH 5/5] checkout: stop expanding sparse indexes Derrick Stolee via GitGitGadget
2021-07-09 21:26 ` [PATCH 0/5] Sparse index: integrate with commit and checkout Elijah Newren
2021-07-12 18:46   ` Derrick Stolee
2021-07-16 13:59     ` Derrick Stolee
2021-07-17 15:37       ` Elijah Newren [this message]
2021-07-19 14:05         ` Derrick Stolee
2021-07-20 20:14 ` [PATCH v2 0/7] " Derrick Stolee via GitGitGadget
2021-07-20 20:14   ` [PATCH v2 1/7] p2000: add 'git checkout -' test and decrease depth Derrick Stolee via GitGitGadget
2021-07-20 20:14   ` [PATCH v2 2/7] p2000: compress repo names Derrick Stolee via GitGitGadget
2021-07-20 20:14   ` [PATCH v2 3/7] commit: integrate with sparse-index Derrick Stolee via GitGitGadget
2021-07-20 20:14   ` [PATCH v2 4/7] sparse-index: recompute cache-tree Derrick Stolee via GitGitGadget
2021-07-20 20:14   ` [PATCH v2 5/7] checkout: stop expanding sparse indexes Derrick Stolee via GitGitGadget
2021-07-20 20:14   ` [PATCH v2 6/7] t1092: document bad 'git checkout' behavior Derrick Stolee via GitGitGadget
2021-07-20 20:14   ` [PATCH v2 7/7] unpack-trees: resolve sparse-directory/file conflicts Derrick Stolee via GitGitGadget
2021-07-22  4:19     ` Elijah Newren
2021-07-22  4:22   ` [PATCH v2 0/7] Sparse index: integrate with commit and checkout Elijah Newren

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-BGCnV-xHH-+S58pqFFhPbPj_0Rt=_QUf_ShAbTCyW9deA@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 \
    /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).