Re: [PATCH jh/notes-merge-in-git-dir-worktree] fixup! t3310 on Windows

[Johan Herland <johan@herland.net>]

On Wed, Mar 14, 2012 at 12:59, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 3/14/2012 12:39, schrieb Johan Herland:
>> On Wed, Mar 14, 2012 at 09:39, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> From: Johannes Sixt <j6t@kdbg.org>
>>>
>>> On Windows, a directory cannot be removed while it is the working
>>> directory of a process. "git notes merge --commit" attempts to remove
>>> .git/NOTES_MERGE_WORKTREE, but during the test the directory was still
>>> "occupied" by the shell. Move the command out of the subshell to release
>>> the directory.
>>>
>>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>>> ---
>>>  Feel free to squash this into 1/2.
>>>
>>>  t/t3310-notes-merge-manual-resolve.sh |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
>>> index d6d6ac6..6351877 100755
>>> --- a/t/t3310-notes-merge-manual-resolve.sh
>>> +++ b/t/t3310-notes-merge-manual-resolve.sh
>>> @@ -565,9 +565,9 @@ test_expect_success 'switch cwd before committing notes merge' '
>>>        (
>>>                cd .git/NOTES_MERGE_WORKTREE &&
>>>                echo "foo" > $(git rev-parse HEAD) &&
>>> -               echo "bar" >> $(git rev-parse HEAD) &&
>>> -               git notes merge --commit
>>> +               echo "bar" >> $(git rev-parse HEAD)
>>>        ) &&
>>> +       git notes merge --commit &&
>>
>> NAK. This defeats the entire purpose of this test. The bug that we're
>> trying to solve is exactly the situation where the user has changed
>> into the .git/NOTES_MERGE_WORKTREE directory, and invokes 'git notes
>> merge --commit' from within. We need to find a different solution for
>> this on Windows. Maybe we should just abort 'git notes merge
>> --commit/--abort' if the current directory is within
>> .git/NOTES_MERGE_WORKTREE (and we're on Windows)?
>
> Isn't this an indication that something *VERY* wrong is happening? How do
> you explain to POSIX people that you have just pulled the rug unter their
> feet?
>
> $ git notes merge --commit
> $ git notes
> fatal: Unable to read current working directory: No such file or directory

True.

> I doubt that the use-case that is tested here makes sense.

As David wrote, the use case is likely to pop up among regular users.
We can't simply ignore it.

> Or .git/NOTES_MERGE_WORKTREE should not be removed. Would it be an option
> to clear it out only when it is needed, right before it is filled again?

Maybe, but then we wouldn't be able to warn or abort in the case where
there is a previous unfinished notes merge, and the user tries to
start a new notes merge. Instead, we'd silently overwrite the previous
unfinished notes merge...

Maybe it's better to simply detect if cwd is inside
.git/NOTES_MERGE_WORKTREE, and then abort, telling the user to chdir
out before trying again?


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net