| Message ID | patch-1.1-0fdfec624eb-20220531T171908Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ls-tree: test for the regression in 9c4d58ff2c3 | expand |
Hi Ævar, On Tue, 31 May 2022, Ævar Arnfjörð Bjarmason wrote: > Add a test for the regression introduced in my 9c4d58ff2c3 (ls-tree: > split up "fast path" callbacks, 2022-03-23) and fixed in > 350296cc789 (ls-tree: `-l` should not imply recursive listing, > 2022-04-04), and test for the test of ls-tree option/mode combinations > to make sure we don't have other blind spots. That's adding a lot of new test cases, which is not free of cost. > Range-diff: > [... 433 lines, essentially a rewrite...] Pasting 443 lines of range-diff also is not free of cost. > diff --git a/t/t3105-ls-tree-output.sh b/t/t3105-ls-tree-output.sh > new file mode 100755 > index 00000000000..29511d9331b > --- /dev/null > +++ b/t/t3105-ls-tree-output.sh > @@ -0,0 +1,192 @@ > +#!/bin/sh > + > +test_description='ls-tree output' > + > [...] > +' > + > +test_ls_tree_format_mode_output () { > + local opts=$1 && This line does not quote `$1`, but... > + shift && > + cat >expect && > + > + while test $# -gt 0 > + do > + local mode="$1" && > + shift && > + > + test_expect_success "'ls-tree $opts${mode:+ $mode}' output" ' > + git ls-tree ${mode:+$mode }$opts HEAD >actual && > + test_cmp expect actual > + ' > + > + case "$opts" in > + --full-tree) > + test_expect_success "'ls-tree $opts${mode:+ $mode}' output (via subdir, fails)" ' > + test_must_fail git -C dir ls-tree --full-name ${mode:+$mode }$opts HEAD -- ../ > + ' > + ;; > + *) > + test_expect_success "'ls-tree $opts${mode:+ $mode}' output (via subdir)" ' > + git -C dir ls-tree --full-name ${mode:+$mode }$opts HEAD -- ../ >actual && > + test_cmp expect actual > + ' > + ;; > + esac > + done > +} > + > [...] > +## opt = --object-only --abbrev > +test_expect_success 'setup: HEAD_short_* variables' ' > + HEAD_short_gitmodules=$(git rev-parse --short HEAD:.gitmodules) && > + HEAD_short_dir=$(git rev-parse --short HEAD:dir) && > + HEAD_short_top_file=$(git rev-parse --short HEAD:top-file.t) && > + HEAD_short_submodule=$(git rev-parse --short HEAD:submodule) && > + HEAD_short_dir_sub_file=$(git rev-parse --short HEAD:dir/sub-file.t) > +' > +test_ls_tree_format_mode_output "--object-only --abbrev" "" "-t" <<-EOF ... this line passes a first argument that contains spaces. Hence the tests fail in CI: https://github.com/git/git/runs/6703333447?check_suite_focus=true Further, since this failure is outside of any `test_expect_success` or `test_expect_failure`, the error message about this is not even included in the weblogs (but of course it is included in the full logs that are included in the build artifacts). For the record, here is the error message: [...] ok 35 - 'ls-tree --object-only -r' output (via subdir) + git rev-parse --short HEAD:.gitmodules + HEAD_short_gitmodules=6da7993 + git rev-parse --short HEAD:dir + HEAD_short_dir=aba57e0 + git rev-parse --short HEAD:top-file.t + HEAD_short_top_file=02dad95 + git rev-parse --short HEAD:submodule + HEAD_short_submodule=61a63ac + git rev-parse --short HEAD:dir/sub-file.t + HEAD_short_dir_sub_file=a150abd ok 36 - setup: HEAD_short_* variables t3105-ls-tree-output.sh: 20: local: --abbrev: bad variable name FATAL: Unexpected exit with code 2 Note: I am only pointing out why the CI/PR run fails, I have not formed any opinion on the actual code other than noticing a lot of what might be repetitive lines and suspecting that adding this many new test cases increases the runtime of the test suite and thereby adds to developer friction and the benefit (namely, to catch future regressions) might not justify that. But again, I have not fully formed an opinion about this patch. Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> +test_ls_tree_format_mode_output () { >> + local opts=$1 && > > This line does not quote `$1`, but... > > ... this line passes a first argument that contains spaces. Hence the > tests fail in CI: > https://github.com/git/git/runs/6703333447?check_suite_focus=true We had a portability discussion not so in distant past on "local". https://lore.kernel.org/git/xmqqsftc3nd6.fsf@gitster.g/ The conclusion from the thread IIRC is * Very safe local var; var=$1 * Probably OK local var="$1" * Not portable local var=$1 Thanks.
On Thu, Jun 02 2022, Johannes Schindelin wrote: > On Tue, 31 May 2022, Ævar Arnfjörð Bjarmason wrote: > [...] >> +test_ls_tree_format_mode_output () { >> + local opts=$1 && > > This line does not quote `$1`, but... Well spotted, thanks, I don't know how I missed that, will fix it & re-roll, but per changed $subject... > [...] > ... this line passes a first argument that contains spaces. Hence the > tests fail in CI: > https://github.com/git/git/runs/6703333447?check_suite_focus=true > > Further, since this failure is outside of any `test_expect_success` or > `test_expect_failure`, the error message about this is not even included > in the weblogs (but of course it is included in the full logs that are > included in the build artifacts). For the record, here is the error > message: ...this part of it though seems like a pretty bad regression in your merged-to-next js/ci-github-workflow-markup topic, which just happens to be unearthed by this CI failure. On top of master this patch will get this CI failure: https://github.com/avar/git/runs/6675920732?check_suite_focus=true#step:5:598; Ending in ("[...]" edit is mine): ok 35 - 'ls-tree --object-only -r' output (via subdir) + git rev-parse --short HEAD:.gitmodules [...] + HEAD_short_dir_sub_file=a150abd ok 36 - setup: HEAD_short_* variables t3105-ls-tree-output.sh: 20: local: --abbrev: bad variable name FATAL: Unexpected exit with code 2 So here we see that we got to test 36, and then got this error. All of which is after the step we focused on to begin with would have shown: Test Summary Report ------------------- t3105-ls-tree-output.sh (Wstat: 256 Tests: 36 Failed: 0) Non-zero exit status: 1 Parse errors: No plan found in TAP output I.e. telling us we had a TAP parse error, so something like this was going on. But now we'll instead get ("=>" edit is mine, it didn't copy/paste): => Run tests === Failed test: t3105-ls-tree-output === The full logs are in the artifacts attached to this run. Error: Process completed with exit code 1. Where expanding the "=>" in "Run tests" will get us the "Test summary report" from before, but now we have no "ci/print-test-failures.sh" step to emit the output we got from the test, and we just get a cryptic error about t3105 having failed *somewhere*. As noted in the breadcrumb trail leading from "[2]" in [1] this is a fundamental limitation of the approach you picked for --github-workflow-markup. You're tasking the test-lib.sh to emit well-formed output, but as shown here we can die prematurely on "eval" failures. But this does look easy to "solve" with a quicker fix, just bringing back the "ci/print-test-failures.sh" step so you can at least expand it, and not have to go to the "summary" and download the *.zip of the log itself. As that shows we still have the raw log there, it just didn't make it to the new GitHub Markdown formatting mechanism. 1. https://lore.kernel.org/git/220324.8635j7nyvw.gmgdl@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Further, since this failure is outside of any `test_expect_success` or >> `test_expect_failure`, the error message about this is not even included >> in the weblogs (but of course it is included in the full logs that are >> included in the build artifacts). For the record, here is the error >> message: > > ...this part of it though seems like a pretty bad regression in your > merged-to-next js/ci-github-workflow-markup topic, which just happens to > be unearthed by this CI failure. Indeed it makes it impossible to figure it out things like this case. But ... > But this does look easy to "solve" with a quicker fix, just bringing > back the "ci/print-test-failures.sh" step so you can at least expand it, > and not have to go to the "summary" and download the *.zip of the log > itself. As that shows we still have the raw log there, it just didn't > make it to the new GitHub Markdown formatting mechanism. ... it seems a solution is possible? Care to send in a patch (or perhaps Dscho already has a counter-proposal)? Thanks.
On Fri, Jun 03 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> Further, since this failure is outside of any `test_expect_success` or >>> `test_expect_failure`, the error message about this is not even included >>> in the weblogs (but of course it is included in the full logs that are >>> included in the build artifacts). For the record, here is the error >>> message: >> >> ...this part of it though seems like a pretty bad regression in your >> merged-to-next js/ci-github-workflow-markup topic, which just happens to >> be unearthed by this CI failure. > > Indeed it makes it impossible to figure it out things like this > case. But ... > >> But this does look easy to "solve" with a quicker fix, just bringing >> back the "ci/print-test-failures.sh" step so you can at least expand it, >> and not have to go to the "summary" and download the *.zip of the log >> itself. As that shows we still have the raw log there, it just didn't >> make it to the new GitHub Markdown formatting mechanism. > > ... it seems a solution is possible? Care to send in a patch (or > perhaps Dscho already has a counter-proposal)? The only thing I have at the moment is: 1. git revert -m 1 bd37e9e41f5 2. merge: https://lore.kernel.org/git/cover-v6-00.29-00000000000-20220525T094123Z-avarab@gmail.com/ 3. merge: https://lore.kernel.org/git/cover-v6-00.14-00000000000-20220525T100743Z-avarab@gmail.com/ I.e. to pick this in the sequence I'd proposed doing & have tested thoroughly. It also addresses other noted some other regressions in "next", but as noted e.g. in [A] there's other issues in "next", e.g. that even the "raw" trace logs are altered as a side-effect of running with --github-workflow-markup, and of course the major UX slowdowns. So I think the better way forward would be to do that & leave out [B], i.e. make the new output format optional, and wait until outstanding issues are fixed until we flip the default. But do I have something that neatly fixes the issue(s) on top of "next" without a revert & re-apply? No, sorry. In case you want to go for that the resolution for [2] is [C], and a "git rm ci/run-build-and-tests.sh ci/run-static-analysis.sh". A. https://lore.kernel.org/git/patch-v6-13.14-fbe0d99c6b3-20220525T100743Z-avarab@gmail.com/ B. https://lore.kernel.org/git/patch-v6-14.14-0b02b186c87-20220525T100743Z-avarab@gmail.com/ C. diff --git a/Makefile b/Makefile index 19f3756f7eb..c2b0a728df5 100644 --- a/Makefile +++ b/Makefile @@ -3618,3 +3618,4 @@ ci-static-analysis: ci-check-directional-formatting ci-static-analysis: check-builtins ci-static-analysis: check-coccicheck ci-static-analysis: hdr-check +ci-static-analysis: check-pot diff --git a/ci/lib.sh b/ci/lib.sh index 80e89f89b7f..9c54c1330e6 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -270,7 +270,7 @@ linux-TEST-vars) setenv --test GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS 1 setenv --test GIT_TEST_MULTI_PACK_INDEX 1 setenv --test GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP 1 - setenv --test GIT_TEST_ADD_I_USE_BUILTIN 1 + setenv --test GIT_TEST_ADD_I_USE_BUILTIN 0 setenv --test GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME master setenv --test GIT_TEST_WRITE_REV_INDEX 1 setenv --test GIT_TEST_CHECKOUT_WORKERS 2
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Fri, Jun 03 2022, Junio C Hamano wrote: > >> Indeed it makes it impossible to figure it out things like this >> case. But ... >> >>> But this does look easy to "solve" with a quicker fix, just bringing >>> back the "ci/print-test-failures.sh" step so you can at least expand it, >>> and not have to go to the "summary" and download the *.zip of the log >>> itself. As that shows we still have the raw log there, it just didn't >>> make it to the new GitHub Markdown formatting mechanism. >> >> ... it seems a solution is possible? Care to send in a patch (or >> perhaps Dscho already has a counter-proposal)? > > The only thing I have at the moment is: > > 1. git revert -m 1 bd37e9e41f5 > 2. merge: https://lore.kernel.org/git/cover-v6-00.29-00000000000-20220525T094123Z-avarab@gmail.com/ > 3. merge: https://lore.kernel.org/git/cover-v6-00.14-00000000000-20220525T100743Z-avarab@gmail.com/ > > I.e. to pick this in the sequence I'd proposed doing & have tested > thoroughly. I know you two have difference in opinions, but throwing away everything the other party did and forcing your stuff in is not a very effective way to work together. > It also addresses other noted some other regressions in "next", but as > noted e.g. in [A] there's other issues in "next", e.g. that even the > "raw" trace logs are altered as a side-effect of running with > --github-workflow-markup, and of course the major UX slowdowns. Dscho? I know you do not care about the UX slowdown as much as others, but I am sure you do not consider what is in 'next' is perfect. It seems to need further work to go back to the feature parity with what it replaced.
On Tue, Jun 07 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Fri, Jun 03 2022, Junio C Hamano wrote: >> >>> Indeed it makes it impossible to figure it out things like this >>> case. But ... >>> >>>> But this does look easy to "solve" with a quicker fix, just bringing >>>> back the "ci/print-test-failures.sh" step so you can at least expand it, >>>> and not have to go to the "summary" and download the *.zip of the log >>>> itself. As that shows we still have the raw log there, it just didn't >>>> make it to the new GitHub Markdown formatting mechanism. >>> >>> ... it seems a solution is possible? Care to send in a patch (or >>> perhaps Dscho already has a counter-proposal)? >> >> The only thing I have at the moment is: >> >> 1. git revert -m 1 bd37e9e41f5 >> 2. merge: https://lore.kernel.org/git/cover-v6-00.29-00000000000-20220525T094123Z-avarab@gmail.com/ >> 3. merge: https://lore.kernel.org/git/cover-v6-00.14-00000000000-20220525T100743Z-avarab@gmail.com/ >> >> I.e. to pick this in the sequence I'd proposed doing & have tested >> thoroughly. > > I know you two have difference in opinions, but throwing away > everything the other party did and forcing your stuff in is not a > very effective way to work together. I'm suggesting getting Johannes's patches in combined with the changes & bugfixes I'd proposed. So no, not throwing that work away, it would (applied up to 14/14) give you functionally the same end result that's on "next" now as far as the new GitHub Markdown output is concerned. The [3] above has links to the relevant CI output. I had tried to rebase the above [2] on top of "next" before this discussion started, I agree that would be ideal, but it's a much larger logical change that I don't have time to pursue now. I.e. there's a reason I proposed doing it in that order, a logical rebasing of [2] on top of bd37e9e41f5 would involve a lot of backing out of the existing direction taken there. I.e. the whole part where the split by "steps" provides much of the ci/* specific code in bd37e9e41f5 for free. >> It also addresses other noted some other regressions in "next", but as >> noted e.g. in [A] there's other issues in "next", e.g. that even the >> "raw" trace logs are altered as a side-effect of running with >> --github-workflow-markup, and of course the major UX slowdowns. > > Dscho? I know you do not care about the UX slowdown as much as > others, but I am sure you do not consider what is in 'next' is > perfect. It seems to need further work to go back to the feature > parity with what it replaced. Just to be clear [3] up to 14/14 would still exhibit this particular bug, but with 13/14 it wouldn't from the links in [3] the relevant outputs are: "next" (well, similar): https://github.com/avar/git/runs/6571972194?check_suite_focus=true [3] with 14/14: https://github.com/avar/git/runs/6588407676?check_suite_focus=true [3] with 13/14: https://github.com/avar/git/runs/6588579493?check_suite_focus=true I really would like to get is out of this long-running ci/ limbo, perhaps Johannes has some proposed patches, but I don't think fixing the outstanding bugs is going to be trivial or easy. Some of it's hard to tease apart, e.g. the altered *.out logs seem to require some tricky test-lib.sh and test-lib-functions.sh changes. I don't see why wouldn't get all of that code in now though, just hidden behind a flag, that would take the pressure off dealing with the current regressions, [2] with 13/14 would do that. Then once those outstanding issues are fixed we'd just need the one-line 14/14 change to flip the default CI output. But is it the smallest possible change on top of what's now in "next"? No, of course not. But I don't have those hypothetical patches, just the above. That's all I meant in reply to the "care to send in a patch?" upthread.
Hi Junio, On Tue, 7 Jun 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > On Fri, Jun 03 2022, Junio C Hamano wrote: > > > >> Indeed it makes it impossible to figure it out things like this case. > >> But ... > >> > >>> But this does look easy to "solve" with a quicker fix, just bringing > >>> back the "ci/print-test-failures.sh" step so you can at least expand > >>> it, and not have to go to the "summary" and download the *.zip of > >>> the log itself. I agree that re-adding the `ci/print-test-failures.sh` step makes sense, given the recent experience. > >>> As that shows we still have the raw log there, it just didn't make > >>> it to the new GitHub Markdown formatting mechanism. > >> > >> ... it seems a solution is possible? Care to send in a patch (or > >> perhaps Dscho already has a counter-proposal)? I will work on this. > > The only thing I have at the moment is: > > > > 1. git revert -m 1 bd37e9e41f5 > > 2. merge: https://lore.kernel.org/git/cover-v6-00.29-00000000000-20220525T094123Z-avarab@gmail.com/ > > 3. merge: https://lore.kernel.org/git/cover-v6-00.14-00000000000-20220525T100743Z-avarab@gmail.com/ > > > > I.e. to pick this in the sequence I'd proposed doing & have tested > > thoroughly. > > I know you two have difference in opinions, but throwing away everything > the other party did and forcing your stuff in is not a very effective > way to work together. I had already pointed out a rather terrible issue in that 29-strong patch series: Dropping Azure Pipelines support makes it unnecessarily harder to work on Git security issues. And it's not like we have an armada of people working on those. I, for one, am pretty worn out from the recent work. It might not be the intention of that patch series to make my life harder, but it sure would be its impact. And intent does not excuse impact. I therefore had to conclude that the patch series in this form cannot be merged. I recall that other reviews reached the same consensus, that this 29-strong patch series is too unfocused on any particular goal. So maybe calling this "my opinion" is not exactly fair. > > It also addresses other noted some other regressions in "next", but as > > noted e.g. in [A] there's other issues in "next", e.g. that even the > > "raw" trace logs are altered as a side-effect of running with > > --github-workflow-markup, and of course the major UX slowdowns. > > Dscho? I know you do not care about the UX slowdown as much as > others, but I am sure you do not consider what is in 'next' is > perfect. It seems to need further work to go back to the feature > parity with what it replaced. Indeed, I do not consider it perfect. I even said so much in every iteration's cover letter ;-) I will work on bringing back the `print-test-failures` step. Ciao, Dscho
On Wed, Jun 08 2022, Johannes Schindelin wrote: > Hi Junio, > > On Tue, 7 Jun 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >> > On Fri, Jun 03 2022, Junio C Hamano wrote: >> > >> >> Indeed it makes it impossible to figure it out things like this case. >> >> But ... >> >> >> >>> But this does look easy to "solve" with a quicker fix, just bringing >> >>> back the "ci/print-test-failures.sh" step so you can at least expand >> >>> it, and not have to go to the "summary" and download the *.zip of >> >>> the log itself. > > I agree that re-adding the `ci/print-test-failures.sh` step makes sense, > given the recent experience. > >> >>> As that shows we still have the raw log there, it just didn't make >> >>> it to the new GitHub Markdown formatting mechanism. >> >> >> >> ... it seems a solution is possible? Care to send in a patch (or >> >> perhaps Dscho already has a counter-proposal)? > > I will work on this. > >> > The only thing I have at the moment is: >> > >> > 1. git revert -m 1 bd37e9e41f5 >> > 2. merge: https://lore.kernel.org/git/cover-v6-00.29-00000000000-20220525T094123Z-avarab@gmail.com/ >> > 3. merge: https://lore.kernel.org/git/cover-v6-00.14-00000000000-20220525T100743Z-avarab@gmail.com/ >> > >> > I.e. to pick this in the sequence I'd proposed doing & have tested >> > thoroughly. >> >> I know you two have difference in opinions, but throwing away everything >> the other party did and forcing your stuff in is not a very effective >> way to work together. > > I had already pointed out a rather terrible issue in that 29-strong patch > series: Dropping Azure Pipelines support makes it unnecessarily harder to > work on Git security issues. And it's not like we have an armada of people > working on those. I, for one, am pretty worn out from the recent work. > > It might not be the intention of that patch series to make my life harder, > but it sure would be its impact. And intent does not excuse impact. > > I therefore had to conclude that the patch series in this form cannot be > merged. You raised the same concern in February, and I made some practical suggestions for how to proceed with that in: https://lore.kernel.org/git/220222.86y2236ndp.gmgdl@evledraar.gmail.com/ You didn't reply, and here was a reminder in late March: https://lore.kernel.org/git/cover-v2-00.25-00000000000-20220325T182534Z-avarab@gmail.com/ You then had a similar concern, and I replied again in early April saying I'd be happy to acomodate you, if you could reply to that original E-Mail and clarify what you'd like exactly: https://lore.kernel.org/git/220406.86bkxeecoi.gmgdl@evledraar.gmail.com/ And here's a mention of it again in late April: https://lore.kernel.org/git/220421.86fsm66zmz.gmgdl@evledraar.gmail.com/ Then a few weeks ago you brought up the same point, and I replied again you to please reply to that E-mail from back in February: https://lore.kernel.org/git/220524.86y1yrwaw0.gmgdl@evledraar.gmail.com/ So really Johannes, I'm completely fine with accommodating you. But when you suggest that some ad-hoc combination of out-of-tree code and not-used-in-tree code you have must not be touched *and* you're seemingly unwilling to figure out some way forward you're not being very helpful. Instead you just keep repeating that something must not be changed, and when it's pointed out to you that that would block someone else's patches, but would you be OK with X, Y or Z you seemingly just blackhole the replies you get. So can you reply to that this time? Thanks. > I recall that other reviews reached the same consensus, that this > 29-strong patch series is too unfocused on any particular goal. So maybe > calling this "my opinion" is not exactly fair. Yes, that's something others brought up, but it's unrelated to the Azure issue you're raising here. That change was contained within the first 6 or so patches of that series.
diff --git a/t/lib-t3100.sh b/t/lib-t3100.sh new file mode 100644 index 00000000000..eabb5fd8034 --- /dev/null +++ b/t/lib-t3100.sh @@ -0,0 +1,10 @@ +#!/bin/sh + +setup_basic_ls_tree_data () { + mkdir dir && + test_commit dir/sub-file && + test_commit top-file && + git clone . submodule && + git submodule add ./submodule && + git commit -m"add submodule" +} diff --git a/t/t3104-ls-tree-format.sh b/t/t3104-ls-tree-format.sh index 0769a933d69..383896667b6 100755 --- a/t/t3104-ls-tree-format.sh +++ b/t/t3104-ls-tree-format.sh @@ -4,6 +4,7 @@ test_description='ls-tree --format' TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-t3100.sh test_expect_success 'ls-tree --format usage' ' test_expect_code 129 git ls-tree --format=fmt -l HEAD && @@ -12,9 +13,7 @@ test_expect_success 'ls-tree --format usage' ' ' test_expect_success 'setup' ' - mkdir dir && - test_commit dir/sub-file && - test_commit top-file + setup_basic_ls_tree_data ' test_ls_tree_format () { diff --git a/t/t3105-ls-tree-output.sh b/t/t3105-ls-tree-output.sh new file mode 100755 index 00000000000..29511d9331b --- /dev/null +++ b/t/t3105-ls-tree-output.sh @@ -0,0 +1,192 @@ +#!/bin/sh + +test_description='ls-tree output' + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-t3100.sh + +test_expect_success 'ls-tree --format usage' ' + test_expect_code 129 git ls-tree --format=fmt -l HEAD && + test_expect_code 129 git ls-tree --format=fmt --name-only HEAD && + test_expect_code 129 git ls-tree --format=fmt --name-status HEAD +' + +test_expect_success 'setup' ' + setup_basic_ls_tree_data +' + +test_ls_tree_format_mode_output () { + local opts=$1 && + shift && + cat >expect && + + while test $# -gt 0 + do + local mode="$1" && + shift && + + test_expect_success "'ls-tree $opts${mode:+ $mode}' output" ' + git ls-tree ${mode:+$mode }$opts HEAD >actual && + test_cmp expect actual + ' + + case "$opts" in + --full-tree) + test_expect_success "'ls-tree $opts${mode:+ $mode}' output (via subdir, fails)" ' + test_must_fail git -C dir ls-tree --full-name ${mode:+$mode }$opts HEAD -- ../ + ' + ;; + *) + test_expect_success "'ls-tree $opts${mode:+ $mode}' output (via subdir)" ' + git -C dir ls-tree --full-name ${mode:+$mode }$opts HEAD -- ../ >actual && + test_cmp expect actual + ' + ;; + esac + done +} + +# test exact output of option (none, --long, ...) and mode (none and +# -d, -r -t) and combinations +test_expect_success 'setup: HEAD_* variables' ' + HEAD_gitmodules=$(git rev-parse HEAD:.gitmodules) && + HEAD_dir=$(git rev-parse HEAD:dir) && + HEAD_top_file=$(git rev-parse HEAD:top-file.t) && + HEAD_submodule=$(git rev-parse HEAD:submodule) && + HEAD_dir_sub_file=$(git rev-parse HEAD:dir/sub-file.t) +' +## opt = +test_ls_tree_format_mode_output "" "" "-t" <<-EOF + 100644 blob $HEAD_gitmodules .gitmodules + 040000 tree $HEAD_dir dir + 160000 commit $HEAD_submodule submodule + 100644 blob $HEAD_top_file top-file.t + EOF +test_ls_tree_format_mode_output "" "-d" <<-EOF + 040000 tree $HEAD_dir dir + 160000 commit $HEAD_submodule submodule + EOF +test_ls_tree_format_mode_output "" "-r" <<-EOF + 100644 blob $HEAD_gitmodules .gitmodules + 100644 blob $HEAD_dir_sub_file dir/sub-file.t + 160000 commit $HEAD_submodule submodule + 100644 blob $HEAD_top_file top-file.t + EOF +## opt = --long +test_ls_tree_format_mode_output "--long" "" "-t" <<-EOF + 100644 blob $HEAD_gitmodules 61 .gitmodules + 040000 tree $HEAD_dir - dir + 160000 commit $HEAD_submodule - submodule + 100644 blob $HEAD_top_file 9 top-file.t + EOF +test_ls_tree_format_mode_output "--long" "-d" <<-EOF + 040000 tree $HEAD_dir - dir + 160000 commit $HEAD_submodule - submodule + EOF +test_ls_tree_format_mode_output "--long" "-r" <<-EOF + 100644 blob $HEAD_gitmodules 61 .gitmodules + 100644 blob $HEAD_dir_sub_file 13 dir/sub-file.t + 160000 commit $HEAD_submodule - submodule + 100644 blob $HEAD_top_file 9 top-file.t + EOF +## opt = --name-only +test_ls_tree_format_mode_output "--name-only" "" "-t" <<-EOF + .gitmodules + dir + submodule + top-file.t + EOF +test_ls_tree_format_mode_output "--name-only" "-d" <<-EOF + dir + submodule + EOF +test_ls_tree_format_mode_output "--name-only" "-r" <<-EOF + .gitmodules + dir/sub-file.t + submodule + top-file.t + EOF +## opt = --object-only +test_ls_tree_format_mode_output "--object-only" "" "-t" <<-EOF + $HEAD_gitmodules + $HEAD_dir + $HEAD_submodule + $HEAD_top_file + EOF +test_ls_tree_format_mode_output "--object-only" "-d" <<-EOF + $HEAD_dir + $HEAD_submodule + EOF +test_ls_tree_format_mode_output "--object-only" "-r" <<-EOF + $HEAD_gitmodules + $HEAD_dir_sub_file + $HEAD_submodule + $HEAD_top_file + EOF +## opt = --object-only --abbrev +test_expect_success 'setup: HEAD_short_* variables' ' + HEAD_short_gitmodules=$(git rev-parse --short HEAD:.gitmodules) && + HEAD_short_dir=$(git rev-parse --short HEAD:dir) && + HEAD_short_top_file=$(git rev-parse --short HEAD:top-file.t) && + HEAD_short_submodule=$(git rev-parse --short HEAD:submodule) && + HEAD_short_dir_sub_file=$(git rev-parse --short HEAD:dir/sub-file.t) +' +test_ls_tree_format_mode_output "--object-only --abbrev" "" "-t" <<-EOF + $HEAD_short_gitmodules + $HEAD_short_dir + $HEAD_short_submodule + $HEAD_short_top_file + EOF +test_ls_tree_format_mode_output "--object-only --abbrev" "-d" <<-EOF + $HEAD_short_dir + $HEAD_short_submodule + EOF +test_ls_tree_format_mode_output "--object-only --abbrev" "-r" <<-EOF + $HEAD_short_gitmodules + $HEAD_short_dir_sub_file + $HEAD_short_submodule + $HEAD_short_top_file + EOF +## opt = --full-name +test_ls_tree_format_mode_output "--full-name" "" <<-EOF + 100644 blob $HEAD_gitmodules .gitmodules + 040000 tree $HEAD_dir dir + 160000 commit $HEAD_submodule submodule + 100644 blob $HEAD_top_file top-file.t + EOF +test_ls_tree_format_mode_output "--full-name" "-d" <<-EOF + 040000 tree $HEAD_dir dir + 160000 commit $HEAD_submodule submodule + EOF +test_ls_tree_format_mode_output "--full-name" "-r" <<-EOF + 100644 blob $HEAD_gitmodules .gitmodules + 100644 blob $HEAD_dir_sub_file dir/sub-file.t + 160000 commit $HEAD_submodule submodule + 100644 blob $HEAD_top_file top-file.t + EOF +test_ls_tree_format_mode_output "--full-name" "-t" <<-EOF + 100644 blob $HEAD_gitmodules .gitmodules + 040000 tree $HEAD_dir dir + 160000 commit $HEAD_submodule submodule + 100644 blob $HEAD_top_file top-file.t + EOF +## opt = --full-tree +test_ls_tree_format_mode_output "--full-tree" "" "-t" <<-EOF + 100644 blob $HEAD_gitmodules .gitmodules + 040000 tree $HEAD_dir dir + 160000 commit $HEAD_submodule submodule + 100644 blob $HEAD_top_file top-file.t + EOF +test_ls_tree_format_mode_output "--full-tree" "-d" <<-EOF + 040000 tree $HEAD_dir dir + 160000 commit $HEAD_submodule submodule + EOF +test_ls_tree_format_mode_output "--full-tree" "-r" <<-EOF + 100644 blob $HEAD_gitmodules .gitmodules + 100644 blob $HEAD_dir_sub_file dir/sub-file.t + 160000 commit $HEAD_submodule submodule + 100644 blob $HEAD_top_file top-file.t + EOF + +test_done