| Message ID | 20210116153554.12604-3-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Sat, Jan 16, 2021 at 04:35:45PM +0100, Ævar Arnfjörð Bjarmason wrote: > diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh > index 354b7f15f7..2e3efeb80e 100755 > --- a/t/t0090-cache-tree.sh > +++ b/t/t0090-cache-tree.sh > @@ -27,20 +27,15 @@ generate_expected_cache_tree_rec () { > printf "SHA $dir (%d entries, %d subtrees)\n" "$entries" "$subtree_count" && > for subtree in $subtrees > do > - cd "$subtree" > - generate_expected_cache_tree_rec "$dir$subtree" || return 1 > - cd .. > + ( > + cd "$subtree" > + generate_expected_cache_tree_rec "$dir$subtree" || return 1 > + ) > done We don't check that "cd" worked either before or after your patch. Should we? After your patch, we "return" from inside a subshell. Is that portable? ISTR issues around that before, but it just have been when we are not in a function at all. Still, I wonder if: for ... do ( cd "$subtree" && generate_expected_cache_tree_rec "$dir$subtree" ) || return 1 done might be more obvious. > -generate_expected_cache_tree () { > - ( > - generate_expected_cache_tree_rec > - ) > -} I wondered what the "rec" was for, but I guess it is "recurse". Not a problem to keep it, but I wonder if it could be dropped in the name of shortness/simplicity (not worth a re-roll for sure, but maybe worth doing so if you re-roll for the above issues). -Peff
On Sun, Jan 17, 2021 at 11:55 AM Jeff King <peff@peff.net> wrote: > On Sat, Jan 16, 2021 at 04:35:45PM +0100, Ævar Arnfjörð Bjarmason wrote: > > for subtree in $subtrees > > do > > + ( > > + cd "$subtree" > > + generate_expected_cache_tree_rec "$dir$subtree" || return 1 > > + ) > > done > > We don't check that "cd" worked either before or after your patch. > Should we? I'd say "yes". > After your patch, we "return" from inside a subshell. Is that portable? > ISTR issues around that before, but it just have been when we are not in > a function at all. Still, I wonder if: > > for ... > do > ( > cd "$subtree" && > generate_expected_cache_tree_rec "$dir$subtree" > ) || return 1 > done > > might be more obvious. Yes, a good recommendation. Normally, we `exit 1` from within subshells[1], but that wouldn't help us exit this loop early, so the `|| return 1` outside the subshell seems a good solution. [1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/
Jeff King <peff@peff.net> writes: > On Sat, Jan 16, 2021 at 04:35:45PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh >> index 354b7f15f7..2e3efeb80e 100755 >> --- a/t/t0090-cache-tree.sh >> +++ b/t/t0090-cache-tree.sh >> @@ -27,20 +27,15 @@ generate_expected_cache_tree_rec () { >> printf "SHA $dir (%d entries, %d subtrees)\n" "$entries" "$subtree_count" && >> for subtree in $subtrees >> do >> - cd "$subtree" >> - generate_expected_cache_tree_rec "$dir$subtree" || return 1 >> - cd .. >> + ( >> + cd "$subtree" >> + generate_expected_cache_tree_rec "$dir$subtree" || return 1 >> + ) >> done > > We don't check that "cd" worked either before or after your patch. > Should we? > > After your patch, we "return" from inside a subshell. Is that portable? > ISTR issues around that before, but it just have been when we are not in > a function at all. Still, I wonder if: > > for ... > do > ( > cd "$subtree" && > generate_expected_cache_tree_rec "$dir$subtree" > ) || return 1 > done Thanks, I missed that bogus/confusing return. > > might be more obvious. > >> -generate_expected_cache_tree () { >> - ( >> - generate_expected_cache_tree_rec >> - ) >> -} > > I wondered what the "rec" was for, but I guess it is "recurse". Not a > problem to keep it, but I wonder if it could be dropped in the name of > shortness/simplicity (not worth a re-roll for sure, but maybe worth > doing so if you re-roll for the above issues). > > -Peff
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh index 354b7f15f7..2e3efeb80e 100755 --- a/t/t0090-cache-tree.sh +++ b/t/t0090-cache-tree.sh @@ -27,20 +27,15 @@ generate_expected_cache_tree_rec () { printf "SHA $dir (%d entries, %d subtrees)\n" "$entries" "$subtree_count" && for subtree in $subtrees do - cd "$subtree" - generate_expected_cache_tree_rec "$dir$subtree" || return 1 - cd .. + ( + cd "$subtree" + generate_expected_cache_tree_rec "$dir$subtree" || return 1 + ) done } -generate_expected_cache_tree () { - ( - generate_expected_cache_tree_rec - ) -} - test_cache_tree () { - generate_expected_cache_tree >expect && + generate_expected_cache_tree_rec >expect && cmp_cache_tree expect }
Change a "cd xyz && work && cd .." pattern introduced in 9c4d6c0297 (cache-tree: Write updated cache-tree after commit, 2014-07-13) to use a sub-shell instead with less indirection. We did actually recover correctly if we failed in this function since we were wrapped in a subshell one function call up. Let's just use the sub-shell at the point where we want to change the directory instead. This also allows us to get rid of the wrapper function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t0090-cache-tree.sh | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)