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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 11, 2017, 11:15 UTC
Message-ID: <alpine.DEB.2.21.1.1709111259430.4132@virtualbox>
In-Reply-To: <xmqqh8w951ek.fsf@gitster.mtv.corp.google.com>

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