Web lists-archives.com

Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.




Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:

>> Stepping back a bit, I am not sure if it is sane or even valid for the
>> end-user to modify paths outside sparse-checkout area, but that is
>> probably a separate tangent.
>
> That is not at all the scenario that Kevin fixed.

I know, but in this tangent I was reacting to this part of Kevin's
message

>>	create $path
>>	git add $path
>>	git commit
>>	git checkout // where $path is not in the sparse-checkout
>>	git reset HEAD~1

to which I was responding to.

> ...
> Yet without Kevin's fix, `git status` would report that the user *deleted
> files outside the sparse checkout*.
> ...
> That is such an obvious bug, and Kevin's fix is such an obvious
> improvement over the current upstream Git version, that I would think the
> only thing worth discussing is whether the patch goes about it in a way of
> which you approve.

When reviewing any topic, I'd ask three (or four) questions to
myself:

 * Are we solving a right problem?  Is the problem addressed valid?
 * Are we solving it with a right approach?
 * Does the patch implement the approach correctly?

(the fourth is s/correct/efficient/ of the third, which is optional).

and any "no" to an earlier question will make it a moot point to ask
further questions.

Let's imagine a path P that is outside the sparse checkout area.
And we check out a commit that has P.  P would still be recorded in
the index but it would not materialize in the working tree.  "git
status" and friends are asked not to treat this as "locally removed",
to prevent "commit -a" from recording a removal, of course.

Now, let's further imagine that you have a checkout of the same
project but at a commit that does not have P.  Then you reset to
another commit that does have P.  My understanding of what Kevin's
first test wants to demonstrate is that the index is populated with
P (because you did reset to a commit with that path) but it does not
materialize in the working tree (perhaps because that is outside the
sparse checkout area?), yet there is something missing compared to
the earlier case where "git status" and friends are asked not to
treat P as "locally removed".  They instead show P as locally removed,
and "commit -a" would record a removal---that is indeed a problem.

Am I reading the problem description correctly so far?  If so, then
my answer to my first question (are we solving a right problem?) is
yes.

My response to an earlier version of the patch was written while
assuming (read: without thinking deeply but trusting that the
contributor thought things through) that the first two answers were
"yes".  If the right approach were to check out what is in the index
to silence "git status" and friends by matching the index and the
working tree, then the implementation in the patch (i.e. setting up
some cache entry and calling checkout_entry() to make it materialize
in the working tree) looked correct, and that is what I meant by
"Other than that, the patch looks quite cleanly done and well
explained."

But this time, I am trying to see if the approach is good.  I am not
so sure if the approach taken by this patch is so obviously good as
you seem to think.  A logical consequence of the approach "git
status thinks that P appears in the index and missing in the working
tree is a local removal, so let's squelch it by creating the file in
the working tree" is that we will end up fully populating the
working tree with paths that are clearly outside the area the user
designated as the sparse checkout area---surely that may squelch
"status", but to me, it looks like it is breaking what the user
wanted to achieve with "sparse checkout" in the first place.

When we make a sparse checkout that places P outside the checked-out
area, with P in the index and not in the working tree, what asks
"git status" and friends not to treat P as "locally removed"?
Shouldn't "reset HEAD~1" that resurrected P that was missing in the
commit in the state before you did the "reset" be doing the same
(e.g. flipping the bit in the index for path P that says "not having
this in the working tree is not a removal---it is deliberately not
checked out")?  If that is the right approach to solve the issue
(which we established is a right problem to solve already), and the
approach that the patch wants to take is quite the opposite of it,
then my answer to the second question (are we solving the problem
with the right approach?) is no.  And at that point, it is moot to
ask if the code correctly and/or efficiently implements that wrong
solution, isn't it?