Re: [PATCH v3 2/2] t3200: tests for new branch.autosetupmerge option "simple"

From: Tao Klerks <tao@klerks.biz>
Date: Mar 1, 2022, 09:59 UTC
Message-ID: <CAPMMpogv995t102biwYVCcvTTdF7beXNMorn3KyWceuRcbAZPA@mail.gmail.com>
In-Reply-To: <220228.86o82r5nzm.gmgdl@evledraar.gmail.com>

On Mon, Feb 28, 2022 at 10:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 14 quoted lines

>
>
> On Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:
>
> > +test_expect_success 'simple tracking works when remote branch name matches' '
> > +     test_create_repo otherserver &&
> > +     test_commit -C otherserver my_commit 1 &&
> > +     git -C otherserver branch feature &&
> > +     git config branch.autosetupmerge simple &&
> > +     git config remote.otherserver.url otherserver &&
> > +     git config remote.otherserver.fetch refs/heads/*:refs/remotes/otherserver/* &&
>
> Shouldn't these use test_config, or if the tests below need them do that
> via a helper, so later added tests don't need to reset this state?

Yes, I will look at this; I was naively (and clearly incorrectly) following a pattern I saw in this same test file.

Show 11 quoted lines

>
> > +     git fetch otherserver &&
> > +     git branch feature otherserver/feature &&
> > +     rm -fr otherserver &&
>
> Instead of "rm -rf" after, do above:
>
>     test_when_finished "rm -rf otherserver" &&
>     git init otherserver
>
> (you don't need "test_create_repo" either, just use "git init")

Will do, thx!

Show 13 quoted lines

>
> > +     test $(git config branch.feature.remote) = otherserver &&
> > +     test $(git config branch.feature.merge) = refs/heads/feature
>
> Use:
>
>     echo otherserver >expect &&
>     git config ... >actual &&
>     test_cmp expect actual
>
> etc., the pattern you're using here will hide git's exit code on
> segfaults, abort() etc., and also makes for less useful debug info on
> failure than test_cmp.

Again, thank you! (I will look at test_cmp_config() as Eric suggested)

Show 22 quoted lines

>
>
> > +'
> > +
> > +test_expect_success 'simple tracking skips when remote branch name does not match' '
> > +     git config branch.autosetupmerge simple &&
> > +     git config remote.local.url . &&
> > +     git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
>
> ditto config setup above, this is quite hard to follow in sequence since
> yo uneed to reason about all existing config. Let's start with a clean
> slate for each test_expect_success and setup the specific config we want
> instead.fallow since
>
> > +     (git show-ref -q refs/remotes/local/main || git fetch local) &&
>
> This likewise hides segfaults etc. Use:
>
>     test_might_fail git show-ref ...
>
> But maybe this whole thing should use "git rev-parse --verify" or
> something?

Honestly, I think this bad pattern is just a premature optimization against a pretty-fast local fetch. Will simplify, and do the same for existing patterns in this file.

Show 10 quoted lines

>
> > +     git branch my-other local/main &&
> > +     test -z "$(git config branch.my-other.remote)" &&
> > +     test -z "$(git config branch.my-other.merge)"
>
> ditto test_cmp comments, but here:
>
>     git ... >out &&
>     test_must_be_empty out
>

OK, will look, thx.