Re: [PATCH] t5510-fetch: upgrade to a more modern style
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 2, 2022, 01:14 UTC
- Message-ID
- <220402.86v8vsmg5r.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20220331175412.305968-1-gitter.spiros@gmail.com>
On Thu, Mar 31 2022, Elia Pinto wrote:
> Clean up the code style so all the tests, and not just a few, > that chdir around isolate themselves in a subshell.
Sounds sensible.
Show 16 quoted lines
> test_expect_success "fetch test" ' > - cd "$D" && > - echo >file updated by origin && > - git commit -a -m "updated by origin" && > - cd two && > - git fetch && > - git rev-parse --verify refs/heads/one && > - mine=$(git rev-parse refs/heads/one) && > - his=$(cd ../one && git rev-parse refs/heads/main) && > - test "z$mine" = "z$his" > + ( > + cd "$D" && > + echo >file updated by origin && > + git commit -a -m "updated by origin" && > + ( > + cd two &&
Why the two levels of subshelling though? We don't need a new one every time we change directories, or do we?
The point is usually to avoid cd-ing in our main shell, not that each level needs a new shell & indentation...
Show 5 quoted lines
> - test_cmp expected actual' > + ( > + cd "$D" && > + ( > + cd three &&
ditto..
> + git fetch && > + git rev-parse --verify refs/heads/two && > + git rev-parse --verify refs/heads/one &&
FWIW an alternative here is to use git -C "$D/three", but that may end up being too verbose..
Show 35 quoted lines
> test_expect_success 'fetch --prune handles overlapping refspecs' ' > - cd "$D" && > - git update-ref refs/pull/42/head main && > - git clone . prune-overlapping && > - cd prune-overlapping && > - git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && > - > - git fetch --prune origin && > - git rev-parse origin/main && > - git rev-parse origin/pr/42 && > - > - git config --unset-all remote.origin.fetch && > - git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && > - git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* && > - > - git fetch --prune origin && > - git rev-parse origin/main && > - git rev-parse origin/pr/42 > + ( > + cd "$D" && > + git update-ref refs/pull/42/head main && > + git clone . prune-overlapping && > + cd prune-overlapping && > + git config --add remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && > + git fetch --prune origin && > + git rev-parse origin/main && > + git rev-parse origin/pr/42 && > + git config --unset-all remote.origin.fetch && > + git config remote.origin.fetch refs/pull/*/head:refs/remotes/origin/pr/* && > + git config --add remote.origin.fetch refs/heads/*:refs/remotes/origin/* && > + git fetch --prune origin && > + git rev-parse origin/main && > + git rev-parse origin/pr/42 > + ) > '
Please don't lose grouping whitespace while at it. I.e. the pre-image intentionally splits "steps" by \n\n.
Show 50 quoted lines
> > test_expect_success 'fetch --prune --tags prunes branches but not tags' ' > - cd "$D" && > - git clone . prune-tags && > - cd prune-tags && > - git tag sometag main && > - # Create what looks like a remote-tracking branch from an earlier > - # fetch that has since been deleted from the remote: > - git update-ref refs/remotes/origin/fake-remote main && > - > - git fetch --prune --tags origin && > - git rev-parse origin/main && > - test_must_fail git rev-parse origin/fake-remote && > - git rev-parse sometag > + ( > + cd "$D" && > + git clone . prune-tags && > + cd prune-tags && > + git tag sometag main && > + # Create what looks like a remote-tracking branch from an earlier > + # fetch that has since been deleted from the remote: > + git update-ref refs/remotes/origin/fake-remote main && > + git fetch --prune --tags origin && > + git rev-parse origin/main && > + test_must_fail git rev-parse origin/fake-remote && > + git rev-parse sometag > + ) > ' > > test_expect_success 'fetch --prune --tags with branch does not prune other things' ' > - cd "$D" && > - git clone . prune-tags-branch && > - cd prune-tags-branch && > - git tag sometag main && > - git update-ref refs/remotes/origin/extrabranch main && > - > - git fetch --prune --tags origin main && > - git rev-parse origin/extrabranch && > - git rev-parse sometag > + ( > + cd "$D" && > + git clone . prune-tags-branch && > + cd prune-tags-branch && > + git tag sometag main && > + git update-ref refs/remotes/origin/extrabranch main && > + git fetch --prune --tags origin main && > + git rev-parse origin/extrabranch && > + git rev-parse sometag > + ) > '
Skimming these these seem like much of the same code over & over again with tiny variations. Perhaps even better would be splitting much of this into a helper function(s)?
Show 14 quoted lines
> - git -C atomic fetch --atomic origin && > - git -C atomic rev-parse origin/atomic-branch >actual && > - test_cmp expected actual && > - test $oid = "$(git -C atomic rev-parse --verify FETCH_HEAD)" > + ( > + cd "$D" && > + git clone . atomic && > + git branch atomic-branch && > + oid=$(git rev-parse atomic-branch) && > + echo "$oid" >expected && > + git -C atomic fetch --atomic origin && > + git -C atomic rev-parse origin/atomic-branch >actual && > + test_cmp expected actual && > + test $oid = "$(git -C atomic rev-parse --verify FETCH_HEAD)"
speaking of modern style, perhaps it's worth it to fix these exit code hiding issues? I.e. use test_cmp, test_cmp_rev etc.
Show 9 quoted lines
> + head_oid=$(git rev-parse HEAD) && > + cat >expected <<-EOF && > + prepared > + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1 > + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2 > + committed > + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1 > + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2 > + EOF
There was a discussion on-list the other day about how this particular here-doc style is the odd one out, and we'd prefer the content aligned with the "cat".
So if we're re-indenting all of these that would be a nice change while we're at it, particularly as it would make the diff smaller, they'd already be at the "right" indent level.