| Message ID | patch-v2-1.1-ed83b3b74ab-20220404T234507Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ls-tree: fix --long implying -r regression in 9c4d58ff2c3 | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Haing written them I guess we could do them post-release, since the > important thing is to validate the changes. As noted in the commit > message we're now testing all combinations of the "mode" and "format" > options. > > builtin/ls-tree.c | 2 +- > t/t3104-ls-tree-format.sh | 126 +++++++++++++++++++++++++++++++++++--- > 2 files changed, 119 insertions(+), 9 deletions(-) > > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 5dac9ee5b9d..e279be8bb63 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -255,7 +255,7 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base, > printf("%06o %s %s %7s\t", data.mode, type_name(data.type), > find_unique_abbrev(data.oid, abbrev), size_text); > show_tree_common_default_long(base, pathname, data.base->len); > - return 1; > + return recurse; > } OK, two people by happenstance wrote the same fix is reassuring ;-) > + cat >expect && > + cat <&6 >expect.-d && > + cat <&7 >expect.-r && > + cat <&8 >expect.-t && Let's not go too cute like this. This forces the caller to remember which among 6, 7, and 8 corresponds to which option. It is too ugly to live. > test_ls_tree_format \ > "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \ > - "" > + "" \ > + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T > + 040000 tree $(git rev-parse HEAD:dir) dir > + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t > + OUT > + 040000 tree $(git rev-parse HEAD:dir) dir > + OUT_D > + 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t > + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t > + OUT_R > + 040000 tree $(git rev-parse HEAD:dir) dir > + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t > + OUT_T I do not actually mind if it were more like this: test_ls_tree_format \ "%(objectmode) %(objecttype) %(objectname)%x09%(path)" <<-EXPECT git ls-tree HEAD 040000 tree $(git rev-parse HEAD:dir) dir 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t git ls-tree -d HEAD 040000 tree $(git rev-parse HEAD:dir) dir git ls-tree -r HEAD 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t git ls-tree -t HEAD 040000 tree $(git rev-parse HEAD:dir) dir 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t EXPECT that is, we only read from the standard input, each section begins with "git ls-tree" command invocation that gets tested, followed by its expected output, and ends at either the end of the input or the beginning of the next section.
On Wed, Apr 06 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Haing written them I guess we could do them post-release, since the >> important thing is to validate the changes. As noted in the commit >> message we're now testing all combinations of the "mode" and "format" >> options. >> >> builtin/ls-tree.c | 2 +- >> t/t3104-ls-tree-format.sh | 126 +++++++++++++++++++++++++++++++++++--- >> 2 files changed, 119 insertions(+), 9 deletions(-) >> >> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c >> index 5dac9ee5b9d..e279be8bb63 100644 >> --- a/builtin/ls-tree.c >> +++ b/builtin/ls-tree.c >> @@ -255,7 +255,7 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base, >> printf("%06o %s %s %7s\t", data.mode, type_name(data.type), >> find_unique_abbrev(data.oid, abbrev), size_text); >> show_tree_common_default_long(base, pathname, data.base->len); >> - return 1; >> + return recurse; >> } > > OK, two people by happenstance wrote the same fix is reassuring ;-) *nod* >> + cat >expect && >> + cat <&6 >expect.-d && >> + cat <&7 >expect.-r && >> + cat <&8 >expect.-t && > > Let's not go too cute like this. This forces the caller to remember > which among 6, 7, and 8 corresponds to which option. It is too ugly > to live. I think it's rather elegant actually, but to be fair it would, per: git grep '<&[1-9]| [1-9]<<-' Be the user with the most FD's using this sort of pattern. >> test_ls_tree_format \ >> "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \ >> - "" >> + "" \ >> + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T >> + 040000 tree $(git rev-parse HEAD:dir) dir >> + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t >> + OUT >> + 040000 tree $(git rev-parse HEAD:dir) dir >> + OUT_D >> + 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t >> + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t >> + OUT_R >> + 040000 tree $(git rev-parse HEAD:dir) dir >> + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t >> + OUT_T > > I do not actually mind if it were more like this: > > test_ls_tree_format \ > "%(objectmode) %(objecttype) %(objectname)%x09%(path)" <<-EXPECT > git ls-tree HEAD > 040000 tree $(git rev-parse HEAD:dir) dir > 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t > git ls-tree -d HEAD > 040000 tree $(git rev-parse HEAD:dir) dir > git ls-tree -r HEAD > 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t > 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t > git ls-tree -t HEAD > 040000 tree $(git rev-parse HEAD:dir) dir > 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t > EXPECT > > that is, we only read from the standard input, each section begins > with "git ls-tree" command invocation that gets tested, followed by > its expected output, and ends at either the end of the input or the > beginning of the next section. I don't see how needing to implementa while-loop parser for a custom format just to get to the end-goal of having those things split up for us is more elegant, when the shell can do that by splitting those up by stream. But if you insist on this being rewritten we could just unroll the loop, and have each test_ls_tree_format specify the full option and one standard input, but doing it in the function was a bit less verbosity. Anyway, I see you replied on Josh's that you'd queue it, so it's unclear to me if you'd like to pick this up with the version with the tests at all, so I won't hack up a "v3" until that's clear...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> + cat >expect && >>> + cat <&6 >expect.-d && >>> + cat <&7 >expect.-r && >>> + cat <&8 >expect.-t && >> >> Let's not go too cute like this. This forces the caller to remember >> which among 6, 7, and 8 corresponds to which option. It is too ugly >> to live. > > I think it's rather elegant actually, but to be fair it would, per: > > git grep '<&[1-9]| [1-9]<<-' > > Be the user with the most FD's using this sort of pattern. Please give a clear explanation why "-d" has to be 6, "-r" 7 and "-t" 8, that can be used by developers as a memory aid to help them write new tests using the helper. Or justify why the developers have to memorize such a meaningless correspondence, if there is no any good reason. Alternatively, you can stop abusing the word "elegant". It is not a synonym to "what I wrote" ;-).
On Wed, Apr 06 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>>> + cat >expect && >>>> + cat <&6 >expect.-d && >>>> + cat <&7 >expect.-r && >>>> + cat <&8 >expect.-t && >>> >>> Let's not go too cute like this. This forces the caller to remember >>> which among 6, 7, and 8 corresponds to which option. It is too ugly >>> to live. >> >> I think it's rather elegant actually, but to be fair it would, per: >> >> git grep '<&[1-9]| [1-9]<<-' >> >> Be the user with the most FD's using this sort of pattern. > > Please give a clear explanation why "-d" has to be 6, "-r" 7 and > "-t" 8, that can be used by developers as a memory aid to help them > write new tests using the helper. It's documented when test-lib.sh does the redirection, since Fabian Stelzer's a6714088e0c (test-lib: make BAIL_OUT() work in tests and prereq, 2021-12-01). I guess a bit of archane knowledge not documented there is that FD #5 is used for the test-lib.sh itself, e.g. BUG(), so if you want that to work properly you can't touch it. But everything as of #6 is generally fair game, secondary helpers like "test_pause" won't work properly, but as far as the test framork is concerned 6-9 is good. So I don't really have a better explanation the same on as why pick "int i" rather than "int x", it's just the prevailing pattern. > Or justify why the developers have to memorize such a meaningless > correspondence, if there is no any good reason. Explained above, I think. > Alternatively, you can stop abusing the word "elegant". It is not a > synonym to "what I wrote" ;-). The "elegant" part is getting away with passing structured data into a shell function, which it's generally resistant to. As much as I'd like to take credit from it I just picked it up from code Ilya wrote in 0445e6f0a12 (test-lib: '--run' to run only specific tests, 2014-04-30). I didn't know about it before then. In any case, you merged down Josh's version without these tests for rc1, so I'm assuming that's a "no" to the upthread "if you'd like to pick this up with the version with the tests at all" in the context of the RC period. So I think we can table this for now, or would you like a version of this without (a)use of these file descriptors to pass in arguments?
On Tue, 5 Apr 2022 01:45:30 +0200, Ævar Arnfjörð Bjarmason wrote: > Indeed. I guess that makes this a proposed v2, > > I refreshed my E-Mail when I was just about to submit this and spotted > that you'd sent your fix in, but I came up with this (in retrospect a > pretty obvious think-o) fix independently, sorry about the bug. > > The tests took me a bit longer though... > > Haing written them I guess we could do them post-release, since the > important thing is to validate the changes. As noted in the commit > message we're now testing all combinations of the "mode" and "format" > options. Thanks for the quickly fix. > + git ls-tree ${opt:+$opt }$opts $opt HEAD >actual && I think maybe the "$opt" here should be removed, because "${opt:+$opt }" will not append it if "$opt" is null or unset, or will append "$opt ", like: diff --git a/t/t3104-ls-tree-format.sh b/t/t3104-ls-tree-format.sh index 520b5a95c0..9d3ad0295e 100755 --- a/t/t3104-ls-tree-format.sh +++ b/t/t3104-ls-tree-format.sh @@ -31,7 +31,7 @@ test_ls_tree_format () { for opt in '' '-d' '-r' '-t' do test_expect_success "'ls-tree $opts${opt:+ $opt}' output" ' - git ls-tree ${opt:+$opt }$opts $opt HEAD >actual && + git ls-tree ${opt:+$opt }$opts HEAD >actual && test_cmp expect${opt:+.$opt} actual ' done -- But I'm not sure it's intentional or inadvertently design or maybe I misunderstood it here. Thanks.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>>> Let's not go too cute like this. This forces the caller to remember >>>> which among 6, 7, and 8 corresponds to which option. It is too ugly >>>> to live. >>> >>> I think it's rather elegant actually, but to be fair it would, per: >>> >>> git grep '<&[1-9]| [1-9]<<-' >>> >>> Be the user with the most FD's using this sort of pattern. >> >> Please give a clear explanation why "-d" has to be 6, "-r" 7 and >> "-t" 8, that can be used by developers as a memory aid to help them >> write new tests using the helper. > > It's documented when test-lib.sh does the redirection, since Fabian > Stelzer's a6714088e0c (test-lib: make BAIL_OUT() work in tests and > prereq, 2021-12-01). Sorry, but that is not what I asked. I know what we use lower file descriptors for, and I didn't ask why we start from 6 (as opposed to 3). The updated helper forces our developers to know that the expected result for "-d" has to go to #6 (not #7 or #8), and "-r" to #7 (not #6 or #8), etc., in order to write new tests using it, and in order to spot a mistake while reviewing such new tests. It is an invitation to unmaintainable mess. Don't encourage it.
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 5dac9ee5b9d..e279be8bb63 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -255,7 +255,7 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base, printf("%06o %s %s %7s\t", data.mode, type_name(data.type), find_unique_abbrev(data.oid, abbrev), size_text); show_tree_common_default_long(base, pathname, data.base->len); - return 1; + return recurse; } static int show_tree_name_only(const struct object_id *oid, struct strbuf *base, diff --git a/t/t3104-ls-tree-format.sh b/t/t3104-ls-tree-format.sh index 0769a933d69..520b5a95c08 100755 --- a/t/t3104-ls-tree-format.sh +++ b/t/t3104-ls-tree-format.sh @@ -23,6 +23,19 @@ test_ls_tree_format () { fmtopts=$3 && shift 2 && + cat >expect && + cat <&6 >expect.-d && + cat <&7 >expect.-r && + cat <&8 >expect.-t && + + for opt in '' '-d' '-r' '-t' + do + test_expect_success "'ls-tree $opts${opt:+ $opt}' output" ' + git ls-tree ${opt:+$opt }$opts $opt HEAD >actual && + test_cmp expect${opt:+.$opt} actual + ' + done + test_expect_success "ls-tree '--format=<$format>' is like options '$opts $fmtopts'" ' git ls-tree $opts -r HEAD >expect && git ls-tree --format="$format" -r $fmtopts HEAD >actual && @@ -39,38 +52,135 @@ test_ls_tree_format () { test_ls_tree_format \ "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \ - "" + "" \ + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT + 040000 tree $(git rev-parse HEAD:dir) dir + OUT_D + 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT_R + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT_T test_ls_tree_format \ "%(objectmode) %(objecttype) %(objectname) %(objectsize:padded)%x09%(path)" \ - "--long" + "--long" \ + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T + 040000 tree $(git rev-parse HEAD:dir) - dir + 100644 blob $(git rev-parse HEAD:top-file.t) 9 top-file.t + OUT + 040000 tree $(git rev-parse HEAD:dir) - dir + OUT_D + 100644 blob $(git rev-parse HEAD:dir/sub-file.t) 13 dir/sub-file.t + 100644 blob $(git rev-parse HEAD:top-file.t) 9 top-file.t + OUT_R + 040000 tree $(git rev-parse HEAD:dir) - dir + 100644 blob $(git rev-parse HEAD:top-file.t) 9 top-file.t + OUT_T test_ls_tree_format \ "%(path)" \ - "--name-only" + "--name-only" \ + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T + dir + top-file.t + OUT + dir + OUT_D + dir/sub-file.t + top-file.t + OUT_R + dir + top-file.t + OUT_T test_ls_tree_format \ "%(objectname)" \ - "--object-only" + "--object-only" \ + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T + $(git rev-parse HEAD:dir) + $(git rev-parse HEAD:top-file.t) + OUT + $(git rev-parse HEAD:dir) + OUT_D + $(git rev-parse HEAD:dir/sub-file.t) + $(git rev-parse HEAD:top-file.t) + OUT_R + $(git rev-parse HEAD:dir) + $(git rev-parse HEAD:top-file.t) + OUT_T test_ls_tree_format \ "%(objectname)" \ "--object-only --abbrev" \ - "--abbrev" + "--abbrev" \ + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T + $(git rev-parse HEAD:dir | test_copy_bytes 7) + $(git rev-parse HEAD:top-file.t| test_copy_bytes 7) + OUT + $(git rev-parse HEAD:dir | test_copy_bytes 7) + OUT_D + $(git rev-parse HEAD:dir/sub-file.t | test_copy_bytes 7) + $(git rev-parse HEAD:top-file.t | test_copy_bytes 7) + OUT_R + $(git rev-parse HEAD:dir | test_copy_bytes 7) + $(git rev-parse HEAD:top-file.t | test_copy_bytes 7) + OUT_T test_ls_tree_format \ "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \ "-t" \ - "-t" + "-t" \ + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT + 040000 tree $(git rev-parse HEAD:dir) dir + OUT_D + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT_R + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT_T test_ls_tree_format \ "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \ "--full-name" \ - "--full-name" + "--full-name" \ + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT + 040000 tree $(git rev-parse HEAD:dir) dir + OUT_D + 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT_R + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT_T test_ls_tree_format \ "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \ "--full-tree" \ - "--full-tree" + "--full-tree" \ + <<-OUT 6<<-OUT_D 7<<-OUT_R 8<<-OUT_T + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT + 040000 tree $(git rev-parse HEAD:dir) dir + OUT_D + 100644 blob $(git rev-parse HEAD:dir/sub-file.t) dir/sub-file.t + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT_R + 040000 tree $(git rev-parse HEAD:dir) dir + 100644 blob $(git rev-parse HEAD:top-file.t) top-file.t + OUT_T test_done