On Fri, Sep 14, 2018 at 10:47 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > Break line after pipe "|", not before, and lose the backslash. You > > do not need to over-indent the command on the downstream of the > > pipe, i.e. > > > > awk ... | > > xargs -n1 git -C ... && > > > > Same comment applies elsewhere in this patch, not limited to this file. > > > >> + sort fetched_types -u >unique_types.observed && > > > > Make it a habit not to add dashed options after real arguments, i.e. > > > > sort -u fetched_types > > Done. I'm not sure why I made this mistake, since I usually prefer to order flags before positional args. I didn't actually clean this up in existing code as I did other mistakes, since it is very hard to find and do thoroughly. > >> + echo commit >unique_types.expected && > >> + test_cmp unique_types.observed unique_types.expected && > > > > Always compare "expect" with "actual", not in the reverse order, i.e. > > > > test_cmp expect actual > > > > not > > > > test_cmp actual expect > > Done. > > This is important because test_cmp reports failures by showing you > > an output of "diff expect actual" and from "sh t5616-part*.sh -v" > > you can see what additional/excess things were produced by the test > > over what is expected, prefixed with "+", and what your code failed > > to produce are shown prefixed with "-". Hmm... I didn't know aout the -v flag. That's quite good to know, thanks! > > I notice that patches to other files like 6112 in this series also > spread the above mistakes from existing lines. Please do not view > what you see in these two test scripts before you start touching as > a good example to follow---rather, treat them as antipattern X-<. > 5616 is not as bad as 6112, but they both need to be cleaned up. > > We could alternatively do a post clean-up, but ideally we should > first have a clean-up patch before this series to co. I cleaned up existing tests in a new patchset here: https://public-inbox.org/git/cover.1536969438.git.matvore@xxxxxxxxxx/T/#t - that new patch corrects the pipe placement and test_cmp argument ordering. There is no dependency between this patchset and the new one, though I assume you want to commit the clean-up once first so maintain consistency. Here is an interdiff for this particular patch series (I replaced \t with 8 spaces so it would be readable after my mail client mangles it): diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index 14f251de4..e8da2e858 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -30,7 +30,6 @@ static int gently_parse_list_objects_filter( if (filter_options->choice) { if (errbuf) { - strbuf_init(errbuf, 0); strbuf_addstr( errbuf, _("multiple filter-specs cannot be combined")); @@ -54,7 +53,6 @@ static int gently_parse_list_objects_filter( unsigned long depth; if (!git_parse_ulong(v0, &depth) || depth != 0) { if (errbuf) { - strbuf_init(errbuf, 0); strbuf_addstr( errbuf, _("only 'tree:0' is supported")); @@ -85,10 +83,9 @@ static int gently_parse_list_objects_filter( return 0; } - if (errbuf) { - strbuf_init(errbuf, 0); + if (errbuf) strbuf_addf(errbuf, "invalid filter-spec '%s'", arg); - } + memset(filter_options, 0, sizeof(*filter_options)); return 1; } diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index f02b9ae37..5bc5b4445 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -216,7 +216,7 @@ test_expect_success 'missing non-root tree object and rev-list' ' rm -rf repo && test_create_repo repo && mkdir repo/dir && - echo foo > repo/dir/foo && + echo foo >repo/dir/foo && git -C repo add dir/foo && git -C repo commit -m "commit dir/foo" && diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh index 7a4d49ea1..510d3537f 100755 --- a/t/t5317-pack-objects-filter-objects.sh +++ b/t/t5317-pack-objects-filter-objects.sh @@ -61,7 +61,7 @@ test_expect_success 'verify normal and blob:none packfiles have same commits/tre test_expect_success 'get an error for missing tree object' ' git init r5 && - echo foo > r5/foo && + echo foo >r5/foo && git -C r5 add foo && git -C r5 commit -m "foo" && del=$(git -C r5 rev-parse HEAD^{tree} | sed "s|..|&/|") && @@ -97,7 +97,7 @@ test_expect_success 'grab tree directly when using tree:0' ' git -C r1 verify-pack -v ../commitsonly.pack >objs && awk "/tree|blob/{print \$1}" objs >trees_and_blobs && git -C r1 rev-parse HEAD: >expected && - test_cmp trees_and_blobs expected + test_cmp expected trees_and_blobs ' # Test blob:limit=<n>[kmg] filter. diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 8eeb85fbc..7b6294ca5 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -169,11 +169,12 @@ test_expect_success 'use fsck before and after manually fetching a missing subtr # Make sure we only have commits, and all trees and blobs are missing. git -C dst rev-list master --missing=allow-any --objects >fetched_objects && - awk -f print_1.awk fetched_objects \ - | xargs -n1 git -C dst cat-file -t >fetched_types && - sort fetched_types -u >unique_types.observed && + awk -f print_1.awk fetched_objects | + xargs -n1 git -C dst cat-file -t >fetched_types && + + sort -u fetched_types >unique_types.observed && echo commit >unique_types.expected && - test_cmp unique_types.observed unique_types.expected && + test_cmp unique_types.expected unique_types.observed && # Auto-fetch a tree with cat-file. git -C dst cat-file -p $SUBTREE >tree_contents && @@ -185,11 +186,13 @@ test_expect_success 'use fsck before and after manually fetching a missing subtr # Auto-fetch all remaining trees and blobs with --missing=error git -C dst rev-list master --missing=error --objects >fetched_objects && test_line_count = 70 fetched_objects && - awk -f print_1.awk fetched_objects \ - | xargs -n1 git -C dst cat-file -t >fetched_types && - sort fetched_types -u >unique_types.observed && + + awk -f print_1.awk fetched_objects | + xargs -n1 git -C dst cat-file -t >fetched_types && + + sort -u fetched_types >unique_types.observed && printf "blob\ncommit\ntree\n" >unique_types.expected && - test_cmp unique_types.observed unique_types.expected + test_cmp unique_types.expected unique_types.observed ' test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' ' diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh index a989a7082..6e5c41a68 100755 --- a/t/t6112-rev-list-filters-objects.sh +++ b/t/t6112-rev-list-filters-objects.sh @@ -31,11 +31,13 @@ test_expect_success 'verify blob:none omits all 5 blobs' ' ' test_expect_success 'specify blob explicitly prevents filtering' ' - file_3=$(git -C r1 ls-files -s file.3 \ - | awk -f print_2.awk) && - file_4=$(git -C r1 ls-files -s file.4 \ - | awk -f print_2.awk) && - git -C r1 rev-list HEAD --objects --filter=blob:none HEAD $file_3 >observed && + file_3=$(git -C r1 ls-files -s file.3 | + awk -f print_2.awk) && + + file_4=$(git -C r1 ls-files -s file.4 | + awk -f print_2.awk) && + + git -C r1 rev-list --objects --filter=blob:none HEAD $file_3 >observed && grep -q "$file_3" observed && test_must_fail grep -q "$file_4" observed ' @@ -225,13 +227,14 @@ test_expect_success 'rev-list W/ --missing=print and --missing=allow-any for tre # Test tree:0 filter. test_expect_success 'verify tree:0 includes trees in "filtered" output' ' - git -C r3 rev-list HEAD --quiet --objects --filter-print-omitted --filter=tree:0 \ - | awk -f print_1.awk \ - | sed s/~// \ - | xargs -n1 git -C r3 cat-file -t \ - | sort -u >filtered_types && - printf "blob\ntree\n" > expected && - test_cmp filtered_types expected + git -C r3 rev-list HEAD --quiet --objects --filter-print-omitted --filter=tree:0 | + awk -f print_1.awk | + sed s/~// | + xargs -n1 git -C r3 cat-file -t | + sort -u >filtered_types && + + printf "blob\ntree\n" >expected && + test_cmp expected filtered_types ' # Delete some loose objects and use rev-list, but WITHOUT any filtering.
