Re: [PATCH] fetch: add --no-update-remote-refs

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 17, 2020, 16:23 UTC
Message-ID: <CAPig+cTK86FPm1kk-P=dvegu+FTzV3k5Z12Etz_jpE=JdyztDQ@mail.gmail.com>
In-Reply-To: <pull.532.git.1579274939431.gitgitgadget@gmail.com>

On Fri, Jan 17, 2020 at 10:29 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -174,6 +174,30 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'
> +test_expect_success 'fetch --no-update-remote-refs keeps existing refs' '
> +       cd "$TRASH_DIRECTORY" &&

Why does the prologue of this test use `cd "$TRASH_DIRECTORY"` when all the other tests use `cd "$D"`?

Show 11 quoted lines

> +       git clone "$D" remote-refs &&
> +       git -C remote-refs rev-parse remotes/origin/master >old &&
> +       git -C remote-refs update-ref refs/remotes/origin/master master~1 &&
> +       git -C remote-refs rev-parse remotes/origin/master >new &&
> +       git -C remote-refs fetch --no-update-remote-refs origin &&
> +       git -C remote-refs rev-parse remotes/origin/master >actual &&
> +       test_cmp new actual &&
> +       git -C remote-refs fetch origin &&
> +       git -C remote-refs rev-parse remotes/origin/master >actual &&
> +       test_cmp old actual
> +'

I wouldn't expect a re-roll just for this (nor do I insist upon such a change), but this is one of those cases when -C hurts, rather than helps, readability, due to the amount of noise it adds to nearly every line of the test. Using a subshell makes for less noisy code:

    git clone "$D" remote-refs &&
    (
        cd remote-refs &&
        git rev-parse remotes/origin/master >old &&
        git update-ref refs/remotes/origin/master master~1 &&
        git rev-parse remotes/origin/master >new &&
        git fetch --no-update-remote-refs origin &&
        git rev-parse remotes/origin/master >actual &&
        test_cmp new actual &&
        git fetch origin &&
        git rev-parse remotes/origin/master >actual &&
        test_cmp old actual
    )

Show 9 quoted lines

> +test_expect_success 'fetch --no-update-remote-refs --prune with refspec' '
> +       git -C remote-refs update-ref refs/remotes/origin/foo/otherbranch master &&
> +       git -C remote-refs update-ref refs/hidden/foo/otherbranch master &&
> +       git -C remote-refs fetch --prune --no-update-remote-refs origin +refs/heads/*:refs/hidden/* &&
> +       git -C remote-refs rev-parse remotes/origin/foo/otherbranch &&
> +       test_must_fail git -C remote-refs rev-parse refs/hidden/foo/otherbranch &&
> +       git -C remote-refs fetch --prune --no-update-remote-refs origin &&
> +       test_must_fail git -C remote-refs rev-parse remotes/origin/foo/otherbranch
> +'

Ditto.