Re: [PATCH 0/5] Sparse index: integrate with commit and checkout

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 12, 2021, 18:46 UTC
Message-ID: <b362c428-eec9-39e3-55a0-0738431e1d98@gmail.com>
In-Reply-To: <CABPp-BF_i1QRCXaeKzqoc6Q2=3un-wku7aKUEdBbXfeVfTG8xg@mail.gmail.com>

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:
>>

...

Show 34 quoted lines

> I've read over these patches and didn't find any problems in them in
> my reading; however, since it builds upon ds/status-with-sparse-index
> (v7)...
> 
> I decided to retry some of my ideas and testing on Patch 10/16 of v7,
> over at https://lore.kernel.org/git/CABPp-BHwTAKwFiWQ0-2P=_g+7HLK5FfOAz-uujRjLou1fXT3zw@mail.gmail.com/
> 
> It turns out that the block you added there is now triggered by t1092
> after this series, and the testcase won't pass without that block.  It
> might be clearer to move that code fragment, or perhaps the whole
> patch, into this series...though the code fragment as is has
> introduced a bug.  If you take t1092 test 12 ("diff with
> directory/file conflicts") and modify it so that before the
> 
>     git checkout df-conflict
> 
> invocation from sparse-index, you first run:
> 
>     $ git sparse-checkout disable
>     $ echo more stuff >>folder1/edited-content
>     $ git add -u
>     $ git diff HEAD   # note the changes
>     $ git sparse-checkout init --cone --sparse-index
>     $ git diff HEAD   # note the changes are still there
>     $ git checkout df-conflict  # no error??  What about the
> conflicting changes?
>     $ git diff HEAD
> 
> then the last command will show that the staged changes from before
> the commit have simply been discarded.  In short, this makes the
> series behave like --force was passed with sparse directory entries,
> when --force wasn't passed.
> 
> So we've still got some directory/file conflict issues.

You are absolutely right that this seems strange. In fact, there is a behavior change during 'git checkout' for sparse-checkouts in general, but also my sparse-index change creates an additional change in this case.

Here is a test that demonstrates the issue:

test_expect_success 'staged directory/file conflict' '
	init_repos &&

	test_sparse_match git sparse-checkout disable &&
	write_script edit-contents <<-\EOF &&
	echo text >>folder1/edited-content
	EOF
	run_on_all ../edit-contents &&

	test_all_match git add folder1/edited-content &&
	test_all_match git diff HEAD &&
	git -C sparse-checkout sparse-checkout init --cone --no-sparse-index &&
	git -C sparse-index sparse-checkout init --cone --sparse-index &&
	test_all_match git diff HEAD &&

	# Sparse-checkouts handle this conflict differently than
	# full checkouts, as they consider the file "folder1" to
	# be deleted in favor of the staged file
	# "folder1/edited-content".
	test_sparse_match git checkout df-conflict &&
	test_sparse_match git diff HEAD
'

The sparse-index case drops all staged changes during the 'git checkout df-conflict' command, so the test fails on that line.

That final diff looks like this in the sparse-checkout repo (no sparse index):

diff --git a/folder1 b/folder1
deleted file mode 100644
index d95f3ad..0000000
--- a/folder1
+++ /dev/null
@@ -1 +0,0 @@
-content
diff --git a/folder1/edited-content b/folder1/edited-content
new file mode 100644
index 0000000..8e27be7
--- /dev/null
+++ b/folder1/edited-content
@@ -0,0 +1 @@
+text

This is a strange case in that we have a staged tree that is
outside of the sparse-checkout cone. When running the 'git
checkout df-conflict' command, the twoway_merge() method
receives the following values:

 current: "folder1/" (tree OID)
 oldtree: "" (NULL OID)
 newtree: "folder1" (blob OID)

Is this value for 'oldtree' correct? It seems strange to me,
so I'll look further into it.

Clearly, the resolution that was presented in the previous
patch was incorrect so I will try to understand this
situation better.

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.

Thanks,
-Stolee