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

Nguyen Thai Ngoc Duy venit, vidit, dixit 30.11.2009 02:56:

On Sun, Nov 29, 2009 at 8:57 PM, Michael J Gruber
[off-list ref] wrote:

quoted

Nguyen Thai Ngoc Duy venit, vidit, dixit 29.11.2009 09:47:

quoted

On 11/29/09, Michael J Gruber [off-list ref] wrote:

quoted

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 [off-list ref]

No ACK. See below.

quoted

 ---
 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.

quoted

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?

quoted

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