| Message ID | 20250112165125.130400-1-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fetch set_head: fix non-mirror remotes in bare repositories | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: > In b1b713f722 (fetch set_head: handle mirrored bare repositories, > 2024-11-22) it was implicitly assumed that all remotes will be mirrors > in a bare repository, thus fetching a non-mirrored remote could lead to > HEAD pointing to a non-existent reference. Make sure we only overwrite > HEAD if we are in a bare repository and fetching from a mirror. > Otherwise, proceed as normally, and create > refs/remotes/<nonmirrorremote>/HEAD instead. > > Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> > Reported-by: Christian Hesse <list@eworm.de> These should be chronological; somebody reports an issue, the patch gets written, and finally it is sent out with a Sign-off to certify that the patch is not a stolen property. > --- > builtin/fetch.c | 15 ++++++++------- > t/t5505-remote.sh | 10 ++++++++++ > t/t5510-fetch.sh | 13 +++++++++++++ > 3 files changed, 31 insertions(+), 7 deletions(-) We haven't heard from Chritian; has this been tested OK? What the patch does does look sensible. Thanks.
On Thu Jan 23, 2025 at 22:00, Junio C Hamano <gitster@pobox.com> wrote: > Bence Ferdinandy <bence@ferdinandy.com> writes: > >> In b1b713f722 (fetch set_head: handle mirrored bare repositories, >> 2024-11-22) it was implicitly assumed that all remotes will be mirrors >> in a bare repository, thus fetching a non-mirrored remote could lead to >> HEAD pointing to a non-existent reference. Make sure we only overwrite >> HEAD if we are in a bare repository and fetching from a mirror. >> Otherwise, proceed as normally, and create >> refs/remotes/<nonmirrorremote>/HEAD instead. >> >> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> >> Reported-by: Christian Hesse <list@eworm.de> > > These should be chronological; somebody reports an issue, the patch > gets written, and finally it is sent out with a Sign-off to certify > that the patch is not a stolen property. Makes sense, I'll send a v2 in that case. > >> --- >> builtin/fetch.c | 15 ++++++++------- >> t/t5505-remote.sh | 10 ++++++++++ >> t/t5510-fetch.sh | 13 +++++++++++++ >> 3 files changed, 31 insertions(+), 7 deletions(-) > > We haven't heard from Chritian; has this been tested OK? To the extent of the tests I've added, but I'm not aware of anybody else, especially Christian trying it out. > > What the patch does does look sensible. Thanks. Thanks, Bence
On Thu, Jan 23, 2025 at 4:48 PM Bence Ferdinandy <bence@ferdinandy.com> wrote: > On Thu Jan 23, 2025 at 22:00, Junio C Hamano <gitster@pobox.com> wrote: > > Bence Ferdinandy <bence@ferdinandy.com> writes: > >> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> > >> Reported-by: Christian Hesse <list@eworm.de> > > > > These should be chronological; somebody reports an issue, the patch > > gets written, and finally it is sent out with a Sign-off to certify > > that the patch is not a stolen property. > > Makes sense, I'll send a v2 in that case. It's a good idea to check whether Junio has himself fixed this sort of thing when queuing your patch. (In this case, it appears that he has.)
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. > 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
On Fri, Jan 24, 2025 at 12:56 AM Patrick Steinhardt <ps@pks.im> wrote: > On Sun, Jan 12, 2025 at 05:51:22PM +0100, Bence Ferdinandy wrote: > > +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. By way of explanation regarding `cd` in a subshell, see [*]. [*]: https://lore.kernel.org/git/CAPig+cRsAPp1APNJ7W337UNtunETr+Lnn-RcGrAXEFUhN1APyA@mail.gmail.com/
Junio C Hamano <gitster@pobox.com> on Thu, 2025/01/23 13:00: > Bence Ferdinandy <bence@ferdinandy.com> writes: > > > In b1b713f722 (fetch set_head: handle mirrored bare repositories, > > 2024-11-22) it was implicitly assumed that all remotes will be mirrors > > in a bare repository, thus fetching a non-mirrored remote could lead to > > HEAD pointing to a non-existent reference. Make sure we only overwrite > > HEAD if we are in a bare repository and fetching from a mirror. > > Otherwise, proceed as normally, and create > > refs/remotes/<nonmirrorremote>/HEAD instead. > > > > Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> > > Reported-by: Christian Hesse <list@eworm.de> > > These should be chronological; somebody reports an issue, the patch > gets written, and finally it is sent out with a Sign-off to certify > that the patch is not a stolen property. > > > --- > > builtin/fetch.c | 15 ++++++++------- > > t/t5505-remote.sh | 10 ++++++++++ > > t/t5510-fetch.sh | 13 +++++++++++++ > > 3 files changed, 31 insertions(+), 7 deletions(-) > > We haven't heard from Chritian; has this been tested OK? Sorry for the late reply... Yes, with this patch applied git behaves as expected for me. Thanks a lot! > What the patch does does look sensible. Thanks.
Patrick Steinhardt <ps@pks.im> writes: >> 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. Yes, indeed. The change to another script we saw earlier followed the "chdir around only in a subshell" pattern. > 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. Terminating quote can stay, but chdir is a correctness issue that may want to be addressed minimally (i.e. not making things worse, while leaving it for later to clean up the existing ones). Thanks.
Christian Hesse <list@eworm.de> writes: >> > --- >> > builtin/fetch.c | 15 ++++++++------- >> > t/t5505-remote.sh | 10 ++++++++++ >> > t/t5510-fetch.sh | 13 +++++++++++++ >> > 3 files changed, 31 insertions(+), 7 deletions(-) >> >> We haven't heard from Chritian; has this been tested OK? > > Sorry for the late reply... > > Yes, with this patch applied git behaves as expected for me. > Thanks a lot! Thanks.
On Fri Jan 24, 2025 at 06:56, Patrick Steinhardt <ps@pks.im> 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
On Fri Jan 24, 2025 at 17:07, Junio C Hamano <gitster@pobox.com> wrote: > Patrick Steinhardt <ps@pks.im> writes: > >>> 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. > > Yes, indeed. The change to another script we saw earlier followed > the "chdir around only in a subshell" pattern. > >> 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. > > Terminating quote can stay, but chdir is a correctness issue that > may want to be addressed minimally (i.e. not making things worse, > while leaving it for later to clean up the existing ones). I'll be sending the fix with also the terminating quote fixed as well. Actually tests after this which I added for followremotehead followed the correct style so why not here as well ... 5505 remote also has some style issues that came up before so if I find some time I'll probably send a patches cleaning the up both. > > Thanks.
diff --git a/builtin/fetch.c b/builtin/fetch.c index fe2b26c74a..625d45be8b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1618,9 +1618,9 @@ static void report_set_head(const char *remote, const char *head_name, } static int set_head(const struct ref *remote_refs, int follow_remote_head, - const char *no_warn_branch) + const char *no_warn_branch, int mirror) { - int result = 0, create_only, is_bare, was_detached; + int result = 0, create_only, baremirror, was_detached; struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, b_local_head = STRBUF_INIT; const char *remote = gtransport->remote->name; @@ -1655,9 +1655,9 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head, if (!head_name) goto cleanup; - is_bare = is_bare_repository(); - create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !is_bare; - if (is_bare) { + baremirror = is_bare_repository() && mirror; + create_only = follow_remote_head == FOLLOW_REMOTE_ALWAYS ? 0 : !baremirror; + if (baremirror) { strbuf_addstr(&b_head, "HEAD"); strbuf_addf(&b_remote_head, "refs/heads/%s", head_name); } else { @@ -1665,7 +1665,7 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head, strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote, head_name); } /* make sure it's valid */ - if (!is_bare && !refs_ref_exists(refs, b_remote_head.buf)) { + if (!baremirror && !refs_ref_exists(refs, b_remote_head.buf)) { result = 1; goto cleanup; } @@ -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 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 && + ( + 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 && + 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"' + + test_expect_success "fetch test remote HEAD change" ' cd "$D" && cd two &&
In b1b713f722 (fetch set_head: handle mirrored bare repositories, 2024-11-22) it was implicitly assumed that all remotes will be mirrors in a bare repository, thus fetching a non-mirrored remote could lead to HEAD pointing to a non-existent reference. Make sure we only overwrite HEAD if we are in a bare repository and fetching from a mirror. Otherwise, proceed as normally, and create refs/remotes/<nonmirrorremote>/HEAD instead. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> Reported-by: Christian Hesse <list@eworm.de> --- builtin/fetch.c | 15 ++++++++------- t/t5505-remote.sh | 10 ++++++++++ t/t5510-fetch.sh | 13 +++++++++++++ 3 files changed, 31 insertions(+), 7 deletions(-) base-commit: fbe8d3079d4a96aeb4e4529cc93cc0043b759a05