From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Elia Pinto <gitter.spiros@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] t5510-fetch: upgrade to a more modern style
Date: Sat, 02 Apr 2022 03:14:37 +0200 [thread overview]
Message-ID: <220402.86v8vsmg5r.gmgdl@evledraar.gmail.com> (raw)
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.
> 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...
> - 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..
> 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.
>
> 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)?
> - 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.
> + 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.
prev parent reply other threads:[~2022-04-02 1:24 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-31 17:54 [PATCH] t5510-fetch: upgrade to a more modern style Elia Pinto
2022-04-01 20:11 ` Junio C Hamano
2022-04-02 7:32 ` Elia Pinto
2022-04-02 1:14 ` Ævar Arnfjörð Bjarmason [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220402.86v8vsmg5r.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitter.spiros@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).