On Mon, Feb 28, 2022 at 10:39 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > > 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. > > > + 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! > > > + 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) > > > > +' > > + > > +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. > > > + 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.
