| Message ID | 20240709144931.1146528-1-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] builtin/push: call set_refspecs after validating remote | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > - Added more details to the commit message to clarify the issue at hand. > Since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we > reset the remote URL if the provided URL is empty. When a user of > 'remotes_remote_get' tries to fetch a remote with an empty repo name, > the function initializes the remote via 'make_remote'. But the remote is > still not a valid remote, since the URL is empty, so it tries to add the > URL alias using 'add_url_alias'. This in-turn will call 'add_url', but > since the URL is empty we call 'strvec_clear' on the `remote->url`. Back > in 'remotes_remote_get', we again check if the remote is valid, which > fails, so we return 'NULL' for the 'struct remote *' value That's better, but it still talks only about the implementation and control flow description without mentioning what end-user action it breaks. We see in the new test this: $ git push "" main but is that something we even want to support? Before 9badf97c (remote: allow resetting url list, 2024-06-14), the command would have failed with different way [*1*]. Is it merely "this is a nonsense request and must fail, but we do not want to hit BUG in general"? I think it is the latter, but leaving it unsaid is confusing. How about starting it more like... When an end-user runs "git push" with an empty string for the remote repository name, e.g. $ git push '' main "git push" fails with a BUG(). This is because ... or something. cmd_push() calls set_refspecs() on a repo (whose name is ""), which then calls remote.c:remote_get() on it, which calls remote.c:make_remote() to obtain a remote structure. But during this calling sequence especially down from remote_get(), there are inconsistent decisions made for an empty string as a name. It is not a NULL pointer, so it does not benefit from the default refspecs by calling get_default=remotes_remote_for_branch, valid_remote_nick() considers it is not a valid nick so we do not read remotes or branches configuration file, but we still think a name was given (even though it is an empty string) and try to do something useful by calling add_url_alias(). It is a mess and whoever designed this should be shot [*2*] ;-). In any case, an obvious additional fix on top of your change might be to do something like this: diff --git i/remote.c w/remote.c index 5fa046c8f8..d7f9ba3571 100644 --- i/remote.c +++ w/remote.c @@ -682,7 +682,7 @@ remotes_remote_get_1( struct remote *ret; int name_given = 0; - if (name) + if (name && *name) name_given = 1; else name = get_default(remote_state, remote_state->current_branch, which would give us the default remote name, and we would not call add_url_alias() with a bogus empty string to nuke the list. > - Cleaned up 'set_refspecs' by now accepting a remote instead of trying > to obtain one. The last one, which does make sense, is not mentioned in the proposed log message. Lazy remote creation does not help the updated caller because it already has one. > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > + > TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > @@ -38,6 +41,13 @@ test_expect_success 'detect missing sha1 expressions early' ' > test_cmp expect rp-ran > ' > > +# We need to use an existing local_ref so that the remote is mapped to > +# it in 'builtin/push.c:set_refspecs()'. Hmph, it is OK to force 'main' like the above as a workaround, but wouldn't it be sufficient to do $ git push "" HEAD:refs/heads/main or something to avoid having to know what our current branch is? Thanks. [Footnote } *1* For example, before Peff's series, here is what I got: $ git push "" HEAD:master fatal: no path specified; see 'git help pull' for valid url syntax It comes from transport_push() -> get_refs_via_connect() -> parse_connect_url() code path. *2* It probably is me writing in the original in shell script, so I can safely say this without offending anybody.
Junio C Hamano <gitster@pobox.com> writes: > Is it merely "this is a nonsense request and must fail, but we do > not want to hit BUG in general"? I think it is the latter, but > leaving it unsaid is confusing. How about starting it more like... A bit of clarification. "it is the latter" -> "that is what you meant". In any earlier draft I had another possibility but I removed it before sending the message. > When an end-user runs "git push" with an empty string for the > remote repository name, e.g. > > $ git push '' main > > "git push" fails with a BUG(). This is because ... > > or something. Another clarificaiton. ... with a BUG(). -> ... with a BUG(). Even though this is a nonsense request that we want to fail, we shouldn't hit a BUG(). Instead we want to give a sensible error message, e.g., 'bad repository'". Let's make a habit of not stopping at saying what is bad, and also saying what we want instead explicitly. Thanks.
On Tue, Jul 09, 2024 at 11:44:25AM -0700, Junio C Hamano wrote: > In any case, an obvious additional fix on top of your change might > be to do something like this: > > diff --git i/remote.c w/remote.c > index 5fa046c8f8..d7f9ba3571 100644 > --- i/remote.c > +++ w/remote.c > @@ -682,7 +682,7 @@ remotes_remote_get_1( > struct remote *ret; > int name_given = 0; > > - if (name) > + if (name && *name) > name_given = 1; > else > name = get_default(remote_state, remote_state->current_branch, > > which would give us the default remote name, and we would not call > add_url_alias() with a bogus empty string to nuke the list. FWIW, I almost suggested something like this earlier. The outcome will be the same (remote_get(), etc, will return NULL), but I think it removes the "this is surprising" comment from my earlier email and makes things much more explicit. (I also agree with everything else you said in your review). -Peff
Jeff King <peff@peff.net> writes: > On Tue, Jul 09, 2024 at 11:44:25AM -0700, Junio C Hamano wrote: > >> In any case, an obvious additional fix on top of your change might >> be to do something like this: >> >> diff --git i/remote.c w/remote.c >> index 5fa046c8f8..d7f9ba3571 100644 >> --- i/remote.c >> +++ w/remote.c >> @@ -682,7 +682,7 @@ remotes_remote_get_1( >> struct remote *ret; >> int name_given = 0; >> >> - if (name) >> + if (name && *name) >> name_given = 1; >> else >> name = get_default(remote_state, remote_state->current_branch, >> >> which would give us the default remote name, and we would not call >> add_url_alias() with a bogus empty string to nuke the list. > > FWIW, I almost suggested something like this earlier. The outcome will > be the same (remote_get(), etc, will return NULL), but I think it > removes the "this is surprising" comment from my earlier email and makes > things much more explicit. > > (I also agree with everything else you said in your review). Heh, thanks. I should prepare to shoot myself then ;-)
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> - Added more details to the commit message to clarify the issue at hand. > >> Since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we >> reset the remote URL if the provided URL is empty. When a user of >> 'remotes_remote_get' tries to fetch a remote with an empty repo name, >> the function initializes the remote via 'make_remote'. But the remote is >> still not a valid remote, since the URL is empty, so it tries to add the >> URL alias using 'add_url_alias'. This in-turn will call 'add_url', but >> since the URL is empty we call 'strvec_clear' on the `remote->url`. Back >> in 'remotes_remote_get', we again check if the remote is valid, which >> fails, so we return 'NULL' for the 'struct remote *' value > > That's better, but it still talks only about the implementation and > control flow description without mentioning what end-user action it > breaks. We see in the new test this: > > $ git push "" main > > but is that something we even want to support? Before 9badf97c > (remote: allow resetting url list, 2024-06-14), the command would > have failed with different way [*1*]. > > Is it merely "this is a nonsense request and must fail, but we do > not want to hit BUG in general"? I think it is the latter, but > leaving it unsaid is confusing. How about starting it more like... > > When an end-user runs "git push" with an empty string for the > remote repository name, e.g. > > $ git push '' main > > "git push" fails with a BUG(). This is because ... > > or something. > Thanks, I was focused on the technical aspect of it that I didn't really spill out the user interaction, you're right, it should start this way. I will modify accordingly. > cmd_push() calls set_refspecs() on a repo (whose name is ""), which > then calls remote.c:remote_get() on it, which calls > remote.c:make_remote() to obtain a remote structure. > > But during this calling sequence especially down from remote_get(), > there are inconsistent decisions made for an empty string as a name. > It is not a NULL pointer, so it does not benefit from the default > refspecs by calling get_default=remotes_remote_for_branch, > valid_remote_nick() considers it is not a valid nick so we do not > read remotes or branches configuration file, but we still think a > name was given (even though it is an empty string) and try to do > something useful by calling add_url_alias(). > > It is a mess and whoever designed this should be shot [*2*] ;-). > > In any case, an obvious additional fix on top of your change might > be to do something like this: > > diff --git i/remote.c w/remote.c > index 5fa046c8f8..d7f9ba3571 100644 > --- i/remote.c > +++ w/remote.c > @@ -682,7 +682,7 @@ remotes_remote_get_1( > struct remote *ret; > int name_given = 0; > > - if (name) > + if (name && *name) > name_given = 1; > else > name = get_default(remote_state, remote_state->current_branch, > > which would give us the default remote name, and we would not call > add_url_alias() with a bogus empty string to nuke the list. > I'm a bit skeptical of making this change. Mostly from the user's perspective. With my patch currently: $ git push "" refs/heads/master fatal: bad repository '' But with this added, we'd be doing $ git push "" refs/heads/master Everything up-to-date This is because we actually obtained the default remote here. Isn't this confusing from a user's perspective? I mean I agree that an empty repo name is something we should support, but it also shouldn't be something we simply ignore? >> - Cleaned up 'set_refspecs' by now accepting a remote instead of trying >> to obtain one. > > The last one, which does make sense, is not mentioned in the > proposed log message. Lazy remote creation does not help the > updated caller because it already has one. > >> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main >> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >> + >> TEST_PASSES_SANITIZE_LEAK=true >> . ./test-lib.sh >> >> @@ -38,6 +41,13 @@ test_expect_success 'detect missing sha1 expressions early' ' >> test_cmp expect rp-ran >> ' >> >> +# We need to use an existing local_ref so that the remote is mapped to >> +# it in 'builtin/push.c:set_refspecs()'. > > Hmph, it is OK to force 'main' like the above as a workaround, but > wouldn't it be sufficient to do > > $ git push "" HEAD:refs/heads/main > > or something to avoid having to know what our current branch is? > > Thanks. > So if we're simply testing my patch, this is definitely enough. But I wanted to also keep in mind the state before this patch. Wherein the only way the flow would be triggered is if we provide a local_ref, providing ':' follows a different path in 'set_refspecs'. > > > [Footnote } > > *1* For example, before Peff's series, here is what I got: > > $ git push "" HEAD:master > fatal: no path specified; see 'git help pull' for valid url syntax > > It comes from transport_push() -> get_refs_via_connect() -> > parse_connect_url() code path. > > *2* It probably is me writing in the original in shell script, so I > can safely say this without offending anybody.
Karthik Nayak <karthik.188@gmail.com> writes: >> In any case, an obvious additional fix on top of your change might >> be to do something like this: >> >> diff --git i/remote.c w/remote.c >> index 5fa046c8f8..d7f9ba3571 100644 >> --- i/remote.c >> +++ w/remote.c >> @@ -682,7 +682,7 @@ remotes_remote_get_1( >> struct remote *ret; >> int name_given = 0; >> >> - if (name) >> + if (name && *name) >> name_given = 1; >> else >> name = get_default(remote_state, remote_state->current_branch, >> >> which would give us the default remote name, and we would not call >> add_url_alias() with a bogus empty string to nuke the list. >> > > I'm a bit skeptical of making this change. Mostly from the user's > perspective. > ... > This is because we actually obtained the default remote here. Isn't this > confusing from a user's perspective? I agree with you 100%. I haven't checked the ramifications of such a change to other code paths, and callers of remote_get() are many. With your fix, the above is not necessary and it certainly can be left well outside the scope of the current topic. > I mean I agree that an empty repo > name is something we should support, but it also shouldn't be something > we simply ignore? For the sake of simplicity of the UI design, I think an empty repo name (giving an empty string explicitly where a repository nickname or URL is expected) should be forbidden. The above change lets "" stand for the default remote (which is different from "simply" ignoring), and is confusing, because we never did that historically. Unless we advertise it as a new "feature" [*], that is (and I do not believe it is such a great feature myself). > So if we're simply testing my patch, this is definitely enough. But I > wanted to also keep in mind the state before this patch. Wherein the > only way the flow would be triggered is if we provide a local_ref, > providing ':' follows a different path in 'set_refspecs'. OK. If so we may want to have both, so that we won't regress in the other code paths? Thanks. [Footnote] * When you want to interact with the remote you usually work with but want to affect a custom set of refs, with the current UI, you'd need to remember what you call that usual remote (typically "origin") and say "git [push|fetch] origin $refspec". The above change may be seen as a feature that allows you to use an empty string in place of the remote that has to be named explicitly on the command line.
On Wed, Jul 10, 2024 at 06:12:21AM -0700, Karthik Nayak wrote: > > In any case, an obvious additional fix on top of your change might > > be to do something like this: > > > > diff --git i/remote.c w/remote.c > > index 5fa046c8f8..d7f9ba3571 100644 > > --- i/remote.c > > +++ w/remote.c > > @@ -682,7 +682,7 @@ remotes_remote_get_1( > > struct remote *ret; > > int name_given = 0; > > > > - if (name) > > + if (name && *name) > > name_given = 1; > > else > > name = get_default(remote_state, remote_state->current_branch, > > > > which would give us the default remote name, and we would not call > > add_url_alias() with a bogus empty string to nuke the list. > > > > I'm a bit skeptical of making this change. Mostly from the user's > perspective. > > With my patch currently: > > $ git push "" refs/heads/master > fatal: bad repository '' > > But with this added, we'd be doing > > $ git push "" refs/heads/master > Everything up-to-date > > This is because we actually obtained the default remote here. Isn't this > confusing from a user's perspective? I mean I agree that an empty repo > name is something we should support, but it also shouldn't be something > we simply ignore? Oh, I misread Junio's patch in my earlier response. I was focused on not setting name_given, which I thought would result in a NULL return value, and didn't notice that it would also mean using the default remote. Something like: diff --git a/remote.c b/remote.c index 7f6406aaa2..883cf6086e 100644 --- a/remote.c +++ b/remote.c @@ -703,7 +703,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name, if (!valid_remote(ret)) read_branches_file(remote_state, ret); } - if (name_given && !valid_remote(ret)) + if (name_given && *name && !valid_remote(ret)) add_url_alias(remote_state, ret, name); if (!valid_remote(ret)) return NULL; was more what I was thinking. That is, inhibit the empty string explicitly rather than letting the emergent behavior of add_url_alias() do it for us. Or maybe even just: diff --git a/remote.c b/remote.c index 7f6406aaa2..a0b166131f 100644 --- a/remote.c +++ b/remote.c @@ -690,9 +690,11 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name, struct remote *ret; int name_given = 0; - if (name) + if (name) { + if (!name) + return NULL; name_given = 1; - else + } else name = get_default(remote_state, remote_state->current_branch, &name_given); to bail immediately. But all of that would be internal refactoring / cleanup on top of your patch. The user-facing behavior would be the same. -Peff
Jeff King <peff@peff.net> writes: > On Wed, Jul 10, 2024 at 06:12:21AM -0700, Karthik Nayak wrote: > >> > In any case, an obvious additional fix on top of your change might >> > be to do something like this: >> > >> > diff --git i/remote.c w/remote.c >> > index 5fa046c8f8..d7f9ba3571 100644 >> > --- i/remote.c >> > +++ w/remote.c >> > @@ -682,7 +682,7 @@ remotes_remote_get_1( >> > struct remote *ret; >> > int name_given = 0; >> > >> > - if (name) >> > + if (name && *name) >> > name_given = 1; >> > else >> > name = get_default(remote_state, remote_state->current_branch, >> > >> > which would give us the default remote name, and we would not call >> > add_url_alias() with a bogus empty string to nuke the list. >> > >> >> I'm a bit skeptical of making this change. Mostly from the user's >> perspective. >> >> With my patch currently: >> >> $ git push "" refs/heads/master >> fatal: bad repository '' >> >> But with this added, we'd be doing >> >> $ git push "" refs/heads/master >> Everything up-to-date >> >> This is because we actually obtained the default remote here. Isn't this >> confusing from a user's perspective? I mean I agree that an empty repo >> name is something we should support, but it also shouldn't be something >> we simply ignore? > > Oh, I misread Junio's patch in my earlier response. I was focused on not > setting name_given, which I thought would result in a NULL return value, > and didn't notice that it would also mean using the default remote. > Something like: > > diff --git a/remote.c b/remote.c > index 7f6406aaa2..883cf6086e 100644 > --- a/remote.c > +++ b/remote.c > @@ -703,7 +703,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name, > if (!valid_remote(ret)) > read_branches_file(remote_state, ret); > } > - if (name_given && !valid_remote(ret)) > + if (name_given && *name && !valid_remote(ret)) > add_url_alias(remote_state, ret, name); > if (!valid_remote(ret)) > return NULL; > > was more what I was thinking. That is, inhibit the empty string > explicitly rather than letting the emergent behavior of add_url_alias() > do it for us. Or maybe even just: > > diff --git a/remote.c b/remote.c > index 7f6406aaa2..a0b166131f 100644 > --- a/remote.c > +++ b/remote.c > @@ -690,9 +690,11 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name, > struct remote *ret; > int name_given = 0; > > - if (name) > + if (name) { > + if (!name) > + return NULL; > name_given = 1; > - else > + } else > name = get_default(remote_state, remote_state->current_branch, > &name_given); > > > to bail immediately. > > But all of that would be internal refactoring / cleanup on top of your > patch. The user-facing behavior would be the same. > > -Peff These should work as intended on top of my patch. But I will skip doing these changes for now. I do see the merit but I think it is also okay the way it is now.
On Thu, Jul 11, 2024 at 02:35:08AM -0700, Karthik Nayak wrote: > > But all of that would be internal refactoring / cleanup on top of your > > patch. The user-facing behavior would be the same. > > These should work as intended on top of my patch. But I will skip doing > these changes for now. I do see the merit but I think it is also okay > the way it is now. Yep, that is perfectly fine with me. Thanks for working on this! -Peff
diff --git a/builtin/push.c b/builtin/push.c index 8260c6e46a..7a67398124 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -96,9 +96,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref, refspec_append(refspec, ref); } -static void set_refspecs(const char **refs, int nr, const char *repo) +static void set_refspecs(const char **refs, int nr, struct remote *remote) { - struct remote *remote = NULL; struct ref *local_refs = NULL; int i; @@ -124,17 +123,10 @@ static void set_refspecs(const char **refs, int nr, const char *repo) local_refs = get_local_heads(); /* Does "ref" uniquely name our ref? */ - if (count_refspec_match(ref, local_refs, &matched) != 1) { + if (count_refspec_match(ref, local_refs, &matched) != 1) refspec_append(&rs, ref); - } else { - /* lazily grab remote */ - if (!remote) - remote = remote_get(repo); - if (!remote) - BUG("must get a remote for repo '%s'", repo); - + else refspec_append_mapped(&rs, ref, remote, matched); - } } else refspec_append(&rs, ref); } @@ -630,10 +622,8 @@ int cmd_push(int argc, const char **argv, const char *prefix) if (tags) refspec_append(&rs, "refs/tags/*"); - if (argc > 0) { + if (argc > 0) repo = argv[0]; - set_refspecs(argv + 1, argc - 1, repo); - } remote = pushremote_get(repo); if (!remote) { @@ -649,6 +639,9 @@ int cmd_push(int argc, const char **argv, const char *prefix) " git push <name>\n")); } + if (argc > 0) + set_refspecs(argv + 1, argc - 1, remote); + if (remote->mirror) flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE); diff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh index 0247137cb3..54427252a8 100755 --- a/t/t5529-push-errors.sh +++ b/t/t5529-push-errors.sh @@ -2,6 +2,9 @@ test_description='detect some push errors early (before contacting remote)' +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh @@ -38,6 +41,13 @@ test_expect_success 'detect missing sha1 expressions early' ' test_cmp expect rp-ran ' +# We need to use an existing local_ref so that the remote is mapped to +# it in 'builtin/push.c:set_refspecs()'. +test_expect_success 'detect empty remote' ' + test_must_fail git push "" main 2> stderr && + grep "fatal: bad repository ${SQ}${SQ}" stderr +' + test_expect_success 'detect ambiguous refs early' ' git branch foo && git tag foo &&