Re: [PATCH 1/2] t1015: demonstrate directory/file conflict recovery failures

[Elijah Newren <newren@gmail.com>]

Hi Eric,

On Wed, Jul 11, 2018 at 2:21 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jul 10, 2018 at 10:18:33PM -0700, Elijah Newren wrote:
>> Several "recovery" commands outright fail or do not fully recover
>> when directory-file conflicts are present.  This includes:
>>   * git read-tree --reset HEAD
>>   * git am --skip
>>   * git am --abort
>>   * git merge --abort
>>   * git reset --hard
>>
>> Add testcases documenting these shortcomings.
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>> diff --git a/t/t1015-read-index-unmerged.sh b/t/t1015-read-index-unmerged.sh
>> @@ -0,0 +1,123 @@
>> +test_expect_success 'setup modify/delete + directory/file conflict' '
>> +     test_create_repo df_plus_modify_delete &&
>> +     (
>> +             cd df_plus_modify_delete &&
>> +
>> +             printf "a\nb\nc\nd\ne\nf\ng\nh\n" >letters &&
>
> test_write_lines a b c d e f g h >letters &&

Copying and modifying an existing testcase, while forgetting to check
for anachronisms, strikes again.  As always, thanks for reviewing and
catching this; I'll fix it up.

>> +             git add letters &&
>> +             git commit -m initial &&
>> +
>> +             git checkout -b modify &&
>> +             # Throw in letters.txt for sorting order fun
>> +             # ("letters.txt" sorts between "letters" and "letters/file")
>> +             echo i >>letters &&
>> +             echo "version 2" >letters.txt &&
>> +             git add letters letters.txt &&
>> +             git commit -m modified &&
>> +
>> +             git checkout -b delete HEAD^ &&
>> +             git rm letters &&
>> +             mkdir letters &&
>> +             >letters/file &&
>> +             echo "version 1" >letters.txt &&
>> +             git add letters letters.txt &&
>> +             git commit -m deleted
>> +     )
>> +'
>> +
>> +test_expect_failure 'read-tree --reset cleans unmerged entries' '
>> +     test_when_finished "git -C df_plus_modify_delete clean -f" &&
>> +     test_when_finished "git -C df_plus_modify_delete reset --hard" &&
>> +     (
>> +             cd df_plus_modify_delete &&
>> +             ...
>> +     )
>> +'
>
> I wonder how much value these distinct repositories add over not using
> them:

In my opinion, that'd be much worse.  Personally, I think we should
move in the opposite direction and try to migrate more of the
testsuite elsewhere towards clearly independent tests.  A huge pet
peeve of mine is that trying to debug a test often requires working
through dozens and dozens of unrelated tests and their setup just to
understand which part of the repository state is related to the test
at hand and which parts can be ignored.  It's happened enough times
that I just intentionally try to make it clear which tests of mine are
independent by making sure they have their own separate repo (and I
used to do a git reset --hard && git clean -fdqx && rm -rf .git && git
init at the beginning of tests).  If folks have a better suggestion
for how to ensure test independence than using test_create_repo, I'm
all ears, but I'm strongly against just adding more files into the
repo than what previous tests did and continuing running from there.
I feel that's especially important for future readers when dealing
with weird edge and corner cases for merges, but I'd really like to
see that clean separation spread throughout the test suite.

> --- >8 ---
> #!/bin/sh
>
> test_description='Test various callers of read_index_unmerged'
> . ./test-lib.sh
>
> test_expect_success 'setup modify/delete + directory/file conflict' '
>         test_write_lines a b c d e f g h >letters &&
>         git add letters &&
>         git commit -m initial &&
>
>         git checkout -b modify &&
>         # Throw in letters.txt for sorting order fun
>         # ("letters.txt" sorts between "letters" and "letters/file")
>         echo i >>letters &&
>         echo "version 2" >letters.txt &&
>         git add letters letters.txt &&
>         git commit -m modified &&
>
>         git checkout -b delete HEAD^ &&
>         git rm letters &&
>         mkdir letters &&
>         >letters/file &&
>         echo "version 1" >letters.txt &&
>         git add letters letters.txt &&
>         git commit -m deleted
> '
>
> test_expect_failure 'read-tree --reset cleans unmerged entries' '
>         test_when_finished "git clean -f" &&
>         test_when_finished "git reset --hard" &&
>
>         git checkout delete^0 &&
>         test_must_fail git merge modify &&
>
>         git read-tree --reset HEAD &&
>         git ls-files -u >conflicts &&
>         test_must_be_empty conflicts
> '
>
> test_expect_failure 'One reset --hard cleans unmerged entries' '
>         test_when_finished "git clean -f" &&
>         test_when_finished "git reset --hard" &&
>
>         git checkout delete^0 &&
>         test_must_fail git merge modify &&
>
>         git reset --hard &&
>         test_path_is_missing .git/MERGE_HEAD &&
>         git ls-files -u >conflicts &&
>         test_must_be_empty conflicts
> '
>
> test_expect_success 'setup directory/file conflict + simple edit/edit' '
>         test_seq 1 10 >numbers &&
>         git add numbers &&
>         git commit -m initial &&
>
>         git checkout -b d-edit &&
>         mkdir foo &&
>         echo content >foo/bar &&
>         git add foo &&
>         echo 11 >>numbers &&
>         git add numbers &&
>         git commit -m "directory and edit" &&
>
>         git checkout -b f-edit d-edit^1 &&
>         echo content >foo &&
>         git add foo &&
>         echo eleven >>numbers &&
>         git add numbers &&
>         git commit -m "file and edit"
> '
>
> test_expect_failure 'git merge --abort succeeds despite D/F conflict' '
>         test_when_finished "git clean -f" &&
>         test_when_finished "git reset --hard" &&
>
>         git checkout f-edit^0 &&
>         test_must_fail git merge d-edit^0 &&
>
>         git merge --abort &&
>         test_path_is_missing .git/MERGE_HEAD &&
>         git ls-files -u >conflicts &&
>         test_must_be_empty conflicts
> '
>
> test_expect_failure 'git am --skip succeeds despite D/F conflict' '
>         test_when_finished "git clean -f" &&
>         test_when_finished "git reset --hard" &&
>
>         git checkout f-edit^0 &&
>         git format-patch -1 d-edit &&
>         test_must_fail git am -3 0001*.patch &&
>
>         git am --skip &&
>         test_path_is_missing .git/rebase-apply &&
>         git ls-files -u >conflicts &&
>         test_must_be_empty conflicts
> '
>
> test_done
> --- >8 ---