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

[Michael J Gruber <git@drmicha.warpmail.net>]

Nguyen Thai Ngoc Duy venit, vidit, dixit 30.11.2009 02:56:
> On Sun, Nov 29, 2009 at 8:57 PM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> 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.
> 
>> 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.

Thanks for the clarification. I see the background behind 17/17 now. I
think git-commit.txt needs a careful update then, because the
explanation of a common feature (git commit file) should not get
confusing because of a not so common feature (skip-worktree/sparse co).

Regarding 16/17: Seeing a FIXED is very misleading. I caught it during
my work on a patch series and was confused. FIXED usually occurs only
while working on a fix, before adjusting the test. So, unless the reroll
of nd/sparse is to happen very soon, I still suggest marking it as
expect_success as proposed, and then modifying the test during the reroll.

Cheers,
Michael