On Fri Jan 24, 2025 at 06:56, Patrick Steinhardt <ps@xxxxxx> wrote: > On Sun, Jan 12, 2025 at 05:51:22PM +0100, Bence Ferdinandy wrote: >> diff --git a/builtin/fetch.c b/builtin/fetch.c >> index fe2b26c74a..625d45be8b 100644 >> --- a/builtin/fetch.c >> +++ b/builtin/fetch.c >> @@ -1925,7 +1925,8 @@ static int do_fetch(struct transport *transport, >> } >> } >> if (set_head(remote_refs, transport->remote->follow_remote_head, >> - transport->remote->no_warn_branch)) >> + transport->remote->no_warn_branch, >> + transport->remote->mirror)) >> ; >> /* >> * Way too many cases where this can go wrong > > Nit: At this point it might be sensible to simply pass in the remote > itself, which would allow for an easier callsite and less risk of > getting the order of parameters wrong. Thanks, that's a really good point, not to mention inside set_head gtransport is used to also access remote, which seems a bit of an oversight. > >> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh >> index 519f7973e3..c75cfe968f 100755 >> --- a/t/t5505-remote.sh >> +++ b/t/t5505-remote.sh >> @@ -589,6 +589,16 @@ test_expect_success 'add --mirror setting HEAD' ' >> ) >> ' >> >> +test_expect_success 'non-mirror fetch does not interfere with mirror' ' >> + mkdir headnotmain && > > Nit: this can be simplified into `git init --bare -b notmain > headnotmain` so that you don't have to create an empty directory first. > Also, do we want to `test_when_finished rm -rf headnotmain` to clean up > after ourselves? > >> + ( >> + cd headnotmain && >> + git init --bare -b notmain && >> + git remote add -f other ../two && >> + test "$(git symbolic-ref HEAD)" = "refs/heads/notmain" >> + ) >> +' >> + >> test_expect_success 'add --mirror=fetch' ' >> mkdir mirror-fetch && >> git init -b main mirror-fetch/parent && >> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh >> index 2d9587059f..cfa63ae086 100755 >> --- a/t/t5510-fetch.sh >> +++ b/t/t5510-fetch.sh >> @@ -84,6 +84,19 @@ test_expect_success "fetch test remote HEAD" ' >> branch=$(git rev-parse refs/remotes/origin/main) && >> test "z$head" = "z$branch"' >> >> +test_expect_success "fetch test remote HEAD in bare repository" ' >> + cd "$D" && >> + git init --bare barerepo && >> + cd barerepo && > > The `cd` needs to happen in a subshell. ALso, the same comment here > regarding whether we want to have `test_when_finished` to clean up > state. > >> + git remote add upstream ../two && >> + git fetch upstream && >> + git rev-parse --verify refs/remotes/upstream/HEAD && >> + git rev-parse --verify refs/remotes/upstream/main && >> + head=$(git rev-parse refs/remotes/upstream/HEAD) && >> + branch=$(git rev-parse refs/remotes/upstream/main) && >> + test "z$head" = "z$branch"' > > The closing single-quote should be on its own line. > > I see though that you simply follow existing code style, both for the > call to cd(1) and for the single-quote, so these are fine. This test > file could use a makeover, but that is obviously outside of the scope of > this patch series. > > Patrick -- bence.ferdinandy.com
