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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 11 Sep 2017, Junio C Hamano wrote:

> 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. Just have a quick look
at the regression test: in a sparse checkout, the user checked out a
branch, then called `reset` to switch to a different commit. No file was
touched by the user outside the sparse checkout.

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.

For example, you mentioned that you would want to move the declaration of
`two` and `was_missing` into the conditional code block. That is a valid
suggestion for `was_missing` (but of course not for `two`, which is used
in the condition of the code block). That suggestion is more about code
style (and of course easily fixed by Kevin using Edit>Refactor>Move
Definition Location in VS), though, than about the correctness of the post
image.

Much more interesting would be a review of the conditional code block. And
I am not talking about the camelCasing of `ceBefore` (which will be fixed
as easily by Edit>Refactor>Rename). I am talking about the stuff where
tools cannot help, but where your experience is necessary: is it correct
to use make_cache_entry()/checkout_entry() in this case? Are the
parameters passed to those functions correct? Is the call to
cache_name_pos() followed by ce_skip_worktree() the best way to find out
whether the file that is absent was not actually deleted by the user, or
is there a less CPU-intensive way, seeing as we are already guaranteed to
iterate over the queue diff in alphabetical order?

I understand that those latter questions are a lot harder to answer, sorry
about that.

Ciao,
Dscho