Re: [RFC/PATCH] t7011: Mark fixed test as such

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Nov 30, 2009, 01:56 UTC
Message-ID: <fcaeb9bf0911291756g1857fe32r8da8e50f0a137119@mail.gmail.com>
In-Reply-To: <4B127DC0.4020108@drmicha.warpmail.net>

On Sun, Nov 29, 2009 at 8:57 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 31 quoted lines

> Nguyen Thai Ngoc Duy venit, vidit, dixit 29.11.2009 09:47:
>> On 11/29/09, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>>> Test 16/17 had been fixed since its introduction in b4d1690 (Teach Git
>>>  to respect skip-worktree bit (reading part), 2009-08-20). So, mark it as
>>>  expect_success rather than expect_failure.
>>>
>>>  Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>>
>> No ACK. See below.
>>
>>>  ---
>>>  I'm actually wondering about 17/17 as well.
>>>  If commit is called with a file name then shouldn't it simply commit the
>>>  current state of the file in the worktree, no matter what the index or
>>>  skip-worktree say? I therefore think 17/17 should be expect_success
>>>  and have no test_must_fail.
>>
>> Both 16/17 and 17/17 ensure that Git won't look at files on worktree
>> if they are marked as skip-worktree (by definition of skip-worktree,
>> you can safely ignore worktree, otherwise you would not mark them as
>> such). 16/17 happens to pass, not because it does not touch worktree,
>> but because the base index does not have "1", which happens to is the
>> same situation in 16/17 (test commit when "1" is gone). The result is
>> OK but it is actually not (17/17 shows this clearer as it commits the
>> worktree version).
>
> On 16/17, I really cannot agree. You explain that you expect the test to
> succeed (we agree here), but that it succeeds for the wrong reasons. So
> it should be either "expect_success", or the test itself should be
> changed so that it really tests what it intends to, otherwise it raises
> a wrong "FIXED". I suggested and submitted the former.

That was my bad in setting up the environment for 16/17. I will fix that in the next roll of nd/sparse.

Show 10 quoted lines

> On 17/17, it's not clear what should happen. "skip-worktree" says ignore
> the worktree and look in the index instead of accessing worktree files.
> But "git commit file" says ignore the index and stage and commit the
> file from the worktree directly. And that is exactly what happens:
>
> You say "git commit file".
> That means "ignore the index".
> That also means that git ignores the skip-worktree bit which is set in
> the index.
> Therefore, file is committed with the content is has in the worktree.

To me, no command should break out skip-worktree mask. In reality I would expect that case 17/17 only happens when a user accidentally leaves a file that is marked skip-worktree and tries to commit it. An error would be more appropriate to keep consistency with other commands ("git diff HEAD -- 1" would show nothing before committing), and to warn the user that he/she is stepping on the border. He/she can then choose to extend worktree area if still wants to commit that file. How does that sound?

> I'm going by the documentation for git-update-index and git-commit. It
> could be that they are wrong, too, but they agree with the code, so
> what's the reference for saying both code and documentation are wrong?

Both code and documentation are for Git without skip-worktree. If you agree with my reasoning above, I will update documentation to reflect this too.

-- 
Duy