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?
