On 13/01/2023 05:28, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <newren@xxxxxxxxx>
t6426.7 (a rename/add testcase) long had a TODO/FIXME comment about
how the test could be improved (with some commented out sample code
that had a few small errors), but those improvements were blocked on
other changes still in progress. The necessary changes were put in
place years ago but the comment was forgotten. Remove and fix the
commented out code section and finally remove the big TODO/FIXME
comment.
Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
---
Thank you for taking care of this FIXME.
t6426: fix TODO about making test more comprehensive
See
https://lore.kernel.org/git/CABPp-BFxK7SGs3wsOfozSw_Uvr-ynr+x8ciPV2Rmfx6Nr4si6g@xxxxxxxxxxxxxx/
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1462%2Fnewren%2Ft6426-fix-todo-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1462/newren/t6426-fix-todo-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1462
t/t6426-merge-skip-unneeded-updates.sh | 56 ++++++++++----------------
1 file changed, 22 insertions(+), 34 deletions(-)
diff --git a/t/t6426-merge-skip-unneeded-updates.sh b/t/t6426-merge-skip-unneeded-updates.sh
index 2bb8e7f09bb..1fcf5d034ed 100755
--- a/t/t6426-merge-skip-unneeded-updates.sh
+++ b/t/t6426-merge-skip-unneeded-updates.sh
@@ -380,40 +380,28 @@ test_expect_success '2c: Modify b & add c VS rename b->c' '
# Make sure c WAS updated
test-tool chmtime --get c >new-mtime &&
- test $(cat old-mtime) -lt $(cat new-mtime)
-
- # FIXME: rename/add conflicts are horribly broken right now;
- # when I get back to my patch series fixing it and
- # rename/rename(2to1) conflicts to bring them in line with
- # how add/add conflicts behave, then checks like the below
- # could be added. But that patch series is waiting until
- # the rename-directory-detection series lands, which this
- # is part of. And in the mean time, I do not want to further
- # enforce broken behavior. So for now, the main test is the
- # one above that err is an empty file.
-
- #git ls-files -s >index_files &&
- #test_line_count = 2 index_files &&
-
- #git rev-parse >actual :2:c :3:c &&
- #git rev-parse >expect A:b A:c &&
- #test_cmp expect actual &&
-
- #git cat-file -p A:b >>merged &&
- #git cat-file -p A:c >>merge-me &&
- #>empty &&
- #test_must_fail git merge-file \
- # -L "Temporary merge branch 1" \
- # -L "" \
- # -L "Temporary merge branch 2" \
- # merged empty merge-me &&
- #sed -e "s/^\([<=>]\)/\1\1\1/" merged >merged-internal &&
-
- #git hash-object c >actual &&
- #git hash-object merged-internal >expect &&
- #test_cmp expect actual &&
-
- #test_path_is_missing b
+ test $(cat old-mtime) -lt $(cat new-mtime) &&
+
+ git ls-files -s >index_files &&
+ test_line_count = 2 index_files &&
+
+ git rev-parse >actual :2:c :3:c &&
+ git rev-parse >expect A:c A:b &&
+ test_cmp expect actual &&
+
+ git cat-file -p A:b >>merge-me &&
+ git cat-file -p A:c >>merged &&
+ >empty &&
+ test_must_fail git merge-file \
+ -L "HEAD" \
+ -L "" \
+ -L "B^0" \
+ merged empty merge-me &&
+ sed -e "s/^\([<=>]\)/\1\1\1/" merged >merged-internal &&
It seems that this line can be dropped, because merged-internal is not
inspected afterwards. None of the other tests in the file do similar
calls to `sed`. Such substitutions with sed are present in
t6422-merge-rename-corner-cases.sh and t6406-merge-attr.sh though.
+
+ test_cmp merged c &&
+
+ test_path_is_missing b
Function test_setup_2c() creates commits in order: commit O (create b),
commit A (modify b, create c), and then commit B (rename b->c).
I would have preferred if "test_path_is_missing b" check was done
several lines higher, just before "test_line_count = 2 index_files".
It feels more natural with this order of commits in setup to check what
happened to file "b" first. It would also mean that all checking of
directory contents is done in one place, before merge conflict in file
"c" is inspected.
I see, however, that all tests in this file follow the pattern of
checking missing files at the very end, and consistency might be
preferable here.
)
'
base-commit: 2b4f5a4e4bb102ac8d967cea653ed753b608193c
Aside from unnecessary call to sed and nitpicking about order of
assertions, the patch looks good to me.
