* [PATCH 0/2] add an advice on unqualified <dst> push @ 2018-10-10 10:41 Ævar Arnfjörð Bjarmason 2018-10-10 10:41 ` [PATCH 1/2] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason 2018-10-10 10:41 ` [PATCH 2/2] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-10 10:41 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Years ago I accidentally deleted the "master" branch at work (due to git push origin $emptyvar:master), and while I could tell from the reflogs what SHA-1 I needed on the other side ran into the fairly cryptic error message, certainly to me when the adrenaline is flowing and you've just ruined something in production. So this series makes that error message better, and suggests to the user how they can fix the situation, first I needed to mark some strings in remote.c for i18n. It would be better if we could just give the user a full command to copy/paste, i.e. what the ran but with refs/{heads,tags}/<their-ref>, but between passing the remote name down, and handling any push options I think it's better for now just to suggest the refspec. Ævar Arnfjörð Bjarmason (2): i18n: remote.c: mark error(...) messages for translation push: add an advice on unqualified <dst> push Documentation/config.txt | 7 ++++ advice.c | 2 + advice.h | 1 + remote.c | 86 +++++++++++++++++++++++++++++++--------- t/t5505-remote.sh | 25 ++++++++++++ 5 files changed, 102 insertions(+), 19 deletions(-) -- 2.19.1.390.gf3a00b506f ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 1/2] i18n: remote.c: mark error(...) messages for translation 2018-10-10 10:41 [PATCH 0/2] add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-10-10 10:41 ` Ævar Arnfjörð Bjarmason 2018-10-10 20:55 ` Jeff King 2018-10-10 10:41 ` [PATCH 2/2] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-10 10:41 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Mark up the error(...) messages in remote.c for translation. The likes of "unable to push to unqualified destination" are relatively big parts of the UI, i.e. error messages shown when "git push" fails. I don't think any of these are plumbing, an the entire test suite passes when running the tests with GIT_GETTEXT_POISON=1 (after building with GETTEXT_POISON). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/remote.c b/remote.c index 682f2a01f9..cc5553acc2 100644 --- a/remote.c +++ b/remote.c @@ -406,7 +406,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!remote->receivepack) remote->receivepack = v; else - error("more than one receivepack given, using the first"); + error(_("more than one receivepack given, using the first")); } else if (!strcmp(subkey, "uploadpack")) { const char *v; if (git_config_string(&v, key, value)) @@ -414,7 +414,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!remote->uploadpack) remote->uploadpack = v; else - error("more than one uploadpack given, using the first"); + error(_("more than one uploadpack given, using the first")); } else if (!strcmp(subkey, "tagopt")) { if (!strcmp(value, "--no-tags")) remote->fetch_tags = -1; @@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs, int find_src = !query->src; if (find_src && !query->dst) - error("query_refspecs_multiple: need either src or dst"); + error(_("query_refspecs_multiple: need either src or dst")); for (i = 0; i < rs->nr; i++) { struct refspec_item *refspec = &rs->items[i]; @@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query) char **result = find_src ? &query->src : &query->dst; if (find_src && !query->dst) - return error("query_refspecs: need either src or dst"); + return error(_("query_refspecs: need either src or dst")); for (i = 0; i < rs->nr; i++) { struct refspec_item *refspec = &rs->items[i]; @@ -995,12 +995,12 @@ static int match_explicit_lhs(struct ref *src, * way to delete 'other' ref at the remote end. */ if (try_explicit_object_name(rs->src, match) < 0) - return error("src refspec %s does not match any.", rs->src); + return error(_("src refspec %s does not match any."), rs->src); if (allocated_match) *allocated_match = 1; return 0; default: - return error("src refspec %s matches more than one.", rs->src); + return error(_("src refspec %s matches more than one."), rs->src); } } @@ -1041,30 +1041,30 @@ static int match_explicit(struct ref *src, struct ref *dst, if (starts_with(dst_value, "refs/")) matched_dst = make_linked_ref(dst_value, dst_tail); else if (is_null_oid(&matched_src->new_oid)) - error("unable to delete '%s': remote ref does not exist", + error(_("unable to delete '%s': remote ref does not exist"), dst_value); else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else - error("unable to push to unqualified destination: %s\n" - "The destination refspec neither matches an " - "existing ref on the remote nor\n" - "begins with refs/, and we are unable to " - "guess a prefix based on the source ref.", + error(_("unable to push to unqualified destination: %s\n" + "The destination refspec neither matches an " + "existing ref on the remote nor\n" + "begins with refs/, and we are unable to " + "guess a prefix based on the source ref."), dst_value); break; default: matched_dst = NULL; - error("dst refspec %s matches more than one.", + error(_("dst refspec %s matches more than one."), dst_value); break; } if (!matched_dst) return -1; if (matched_dst->peer_ref) - return error("dst ref %s receives from more than one src.", - matched_dst->name); + return error(_("dst ref %s receives from more than one src."), + matched_dst->name); else { matched_dst->peer_ref = allocated_src ? matched_src : @@ -1763,7 +1763,7 @@ int get_fetch_map(const struct ref *remote_refs, if (!starts_with((*rmp)->peer_ref->name, "refs/") || check_refname_format((*rmp)->peer_ref->name, 0)) { struct ref *ignore = *rmp; - error("* Ignoring funny ref '%s' locally", + error(_("* Ignoring funny ref '%s' locally"), (*rmp)->peer_ref->name); *rmp = (*rmp)->next; free(ignore->peer_ref); @@ -2131,7 +2131,7 @@ static int parse_push_cas_option(struct push_cas_option *cas, const char *arg, i else if (!colon[1]) oidclr(&entry->expect); else if (get_oid(colon + 1, &entry->expect)) - return error("cannot parse expected object name '%s'", colon + 1); + return error(_("cannot parse expected object name '%s'"), colon + 1); return 0; } -- 2.19.1.390.gf3a00b506f ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH 1/2] i18n: remote.c: mark error(...) messages for translation 2018-10-10 10:41 ` [PATCH 1/2] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason @ 2018-10-10 20:55 ` Jeff King 0 siblings, 0 replies; 53+ messages in thread From: Jeff King @ 2018-10-10 20:55 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano On Wed, Oct 10, 2018 at 10:41:44AM +0000, Ævar Arnfjörð Bjarmason wrote: > Mark up the error(...) messages in remote.c for translation. The likes > of "unable to push to unqualified destination" are relatively big > parts of the UI, i.e. error messages shown when "git push" fails. > > I don't think any of these are plumbing, an the entire test suite > passes when running the tests with GIT_GETTEXT_POISON=1 (after > building with GETTEXT_POISON). So obviously the second patch is much more interesting, and I focused most of my comments there. ;) But this one seems like an obvious improvement. -Peff ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-10 10:41 [PATCH 0/2] add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-10-10 10:41 ` [PATCH 1/2] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason @ 2018-10-10 10:41 ` Ævar Arnfjörð Bjarmason 2018-10-10 20:55 ` Jeff King 1 sibling, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-10 10:41 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Improve the error message added in f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23), which before this change looks like this: $ git push avar v2.19.0^{commit}:newbranch -n error: unable to push to unqualified destination: newbranch The destination refspec neither matches an existing ref on the remote nor begins with refs/, and we are unable to guess a prefix based on the source ref. error: failed to push some refs to 'git@github.com:avar/git.git' This message needed to be read very carefully to spot how to fix the error, i.e. to push to refs/heads/newbranch, and it didn't use the advice system (since initial addition of the error predated it). Fix both of those, now the message will look like this instead: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: unable to push to unqualified destination: newbranch hint: The destination refspec neither matches an existing hint: ref on the remote nor begins with refs/, and we are hint: unable to guess a prefix based on the source ref. hint: hint: The <src> part of the refspec is a commit object. hint: Did you mean to create a new branch by pushing to hint: 'v2.19.0^{commit}:refs/heads/newbranch'? error: failed to push some refs to 'git@github.com:avar/git.git' When trying to push a tag, tree or a blob we suggest that perhaps the user meant to push them to refs/tags/ instead. The if/else duplication for all of OBJ_{COMMIT,TAG,TREE,BLOB} is unfortunate, but is required to correctly mark the messages for translation. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/config.txt | 7 +++++ advice.c | 2 ++ advice.h | 1 + remote.c | 62 +++++++++++++++++++++++++++++++++++----- t/t5505-remote.sh | 25 ++++++++++++++++ 5 files changed, 90 insertions(+), 7 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 1546833213..fd455e2739 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -320,6 +320,13 @@ advice.*:: tries to overwrite a remote ref that points at an object that is not a commit-ish, or make the remote ref point at an object that is not a commit-ish. + pushAmbigiousRefName:: + Shown when linkgit:git-push[1] gives up trying to + guess based on the source and destination refs what + remote ref namespace the source belongs in, but where + we can still suggest that the user push to either + refs/heads/* or refs/tags/* based on the type of the + source object. statusHints:: Show directions on how to proceed from the current state in the output of linkgit:git-status[1], in diff --git a/advice.c b/advice.c index 3561cd64e9..84e9d0168d 100644 --- a/advice.c +++ b/advice.c @@ -9,6 +9,7 @@ int advice_push_non_ff_matching = 1; int advice_push_already_exists = 1; int advice_push_fetch_first = 1; int advice_push_needs_force = 1; +int advice_push_ambiguous_ref_name = 1; int advice_status_hints = 1; int advice_status_u_option = 1; int advice_commit_before_merge = 1; @@ -62,6 +63,7 @@ static struct { { "pushAlreadyExists", &advice_push_already_exists }, { "pushFetchFirst", &advice_push_fetch_first }, { "pushNeedsForce", &advice_push_needs_force }, + { "pushAmbigiousRefName", &advice_push_ambiguous_ref_name }, { "statusHints", &advice_status_hints }, { "statusUoption", &advice_status_u_option }, { "commitBeforeMerge", &advice_commit_before_merge }, diff --git a/advice.h b/advice.h index ab24df0fd0..d2445cab8b 100644 --- a/advice.h +++ b/advice.h @@ -9,6 +9,7 @@ extern int advice_push_non_ff_matching; extern int advice_push_already_exists; extern int advice_push_fetch_first; extern int advice_push_needs_force; +extern int advice_push_ambiguous_ref_name; extern int advice_status_hints; extern int advice_status_u_option; extern int advice_commit_before_merge; diff --git a/remote.c b/remote.c index cc5553acc2..78fa2d9aff 100644 --- a/remote.c +++ b/remote.c @@ -13,6 +13,7 @@ #include "mergesort.h" #include "argv-array.h" #include "commit-reach.h" +#include "advice.h" enum map_direction { FROM_SRC, FROM_DST }; @@ -1046,13 +1047,60 @@ static int match_explicit(struct ref *src, struct ref *dst, else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); - } else - error(_("unable to push to unqualified destination: %s\n" - "The destination refspec neither matches an " - "existing ref on the remote nor\n" - "begins with refs/, and we are unable to " - "guess a prefix based on the source ref."), - dst_value); + } else { + struct object_id oid; + enum object_type type; + + error("unable to push to unqualified destination: %s", dst_value); + if (!advice_push_ambiguous_ref_name) + break; + if (get_oid(matched_src->name, &oid)) + BUG("'%s' is not a valid object, " + "match_explicit_lhs() should catch this!", + matched_src->name); + type = oid_object_info(the_repository, &oid, NULL); + if (type == OBJ_COMMIT) { + + advise(_("The destination refspec neither matches an existing\n" + "ref on the remote nor begins with refs/, and we are\n" + "unable to guess a prefix based on the source ref.\n" + "\n" + "The <src> part of the refspec is a commit object.\n" + "Did you mean to create a new branch by pushing to\n" + "'%s:refs/heads/%s'?"), + matched_src->name, dst_value); + } else if (type == OBJ_TAG) { + advise(_("The destination refspec neither matches an existing\n" + "ref on the remote nor begins with refs/, and we are\n" + "unable to guess a prefix based on the source ref.\n" + "\n" + "The <src> part of the refspec is a tag object.\n" + "Did you mean to create a new tag by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src->name, dst_value); + } else if (type == OBJ_TREE) { + advise(_("The destination refspec neither matches an existing\n" + "ref on the remote nor begins with refs/, and we are\n" + "unable to guess a prefix based on the source ref.\n" + "\n" + "The <src> part of the refspec is a tree object.\n" + "Did you mean to tag a new tree by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src->name, dst_value); + } else if (type == OBJ_BLOB) { + advise(_("The destination refspec neither matches an existing\n" + "ref on the remote nor begins with refs/, and we are\n" + "unable to guess a prefix based on the source ref.\n" + "\n" + "The <src> part of the refspec is a blob object.\n" + "Did you mean to tag a new blob by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src->name, dst_value); + } else { + BUG("'%s' should be commit/tag/tree/blob, is '%d'", + matched_src->name, type); + } + } break; default: matched_dst = NULL; diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index d2a2cdd453..1eabf06aa4 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1222,4 +1222,29 @@ test_expect_success 'add remote matching the "insteadOf" URL' ' git remote add backup xyz@example.com ' +test_expect_success 'unqualified refspec DWIM and advice' ' + test_when_finished "(cd test && git tag -d some-tag)" && + ( + cd test && + git tag -a -m "Some tag" some-tag master && + for type in commit tag tree blob + do + if test "$type" = "blob" + then + oid=$(git rev-parse some-tag:file) + else + oid=$(git rev-parse some-tag^{$type}) + fi && + test_must_fail git push origin $oid:dst -n 2>err && + test_i18ngrep "error: unable to push" err && + test_i18ngrep "hint: Did you mean" err && + test_must_fail git -c advice.pushAmbigiousRefName=false \ + push origin $oid:dst -n 2>err && + test_i18ngrep "error: unable to push" err && + test_i18ngrep ! "hint: Did you mean" err + done + ) +' + + test_done -- 2.19.1.390.gf3a00b506f ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-10 10:41 ` [PATCH 2/2] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-10-10 20:55 ` Jeff King 2018-10-10 21:23 ` Ævar Arnfjörð Bjarmason 2018-10-10 21:54 ` [PATCH 2/2] push: add an advice on unqualified <dst> push Junio C Hamano 0 siblings, 2 replies; 53+ messages in thread From: Jeff King @ 2018-10-10 20:55 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano On Wed, Oct 10, 2018 at 10:41:45AM +0000, Ævar Arnfjörð Bjarmason wrote: > Improve the error message added in f8aae12034 ("push: allow > unqualified dest refspecs to DWIM", 2008-04-23), which before this > change looks like this: > > $ git push avar v2.19.0^{commit}:newbranch -n > error: unable to push to unqualified destination: newbranch > The destination refspec neither matches an existing ref on the remote nor > begins with refs/, and we are unable to guess a prefix based on the source ref. > error: failed to push some refs to 'git@github.com:avar/git.git' Thanks for looking into this. Despite being largely responsible for that message myself, I always cringe when I see it because it's so opaque. > This message needed to be read very carefully to spot how to fix the > error, i.e. to push to refs/heads/newbranch, and it didn't use the > advice system (since initial addition of the error predated it). > > Fix both of those, now the message will look like this instead: > > $ ./git-push avar v2.19.0^{commit}:newbranch -n > error: unable to push to unqualified destination: newbranch > hint: The destination refspec neither matches an existing > hint: ref on the remote nor begins with refs/, and we are > hint: unable to guess a prefix based on the source ref. > hint: > hint: The <src> part of the refspec is a commit object. > hint: Did you mean to create a new branch by pushing to > hint: 'v2.19.0^{commit}:refs/heads/newbranch'? > error: failed to push some refs to 'git@github.com:avar/git.git' > > When trying to push a tag, tree or a blob we suggest that perhaps the > user meant to push them to refs/tags/ instead. This is much better, and I love the customized behavior based on the object type. I wonder if we could reword the first paragraph to be a little less confusing, and spell out what we tried already. E.g., something like: The destination you provided is not a full refname (i.e., starting with "ref"). Git tried to guess what you meant by: - looking for a matching branch or tag on the remote side - looking at the refname of the local source but neither worked. The <src> part of the refspec is a commit object. Did you mean... I'm not sure about saying "branch or tag" in the first bullet. It's friendlier to most users, but less technically correct (if you said "notes/foo", I believe we'd match an existing "refs/notes/foo", because it's really just using the normal lookup rules). Also, as an aside, I wonder if we should allow "heads/foo" to work as "refs/heads/foo" (even when no such ref already exists). But that is totally orthogonal to changing the message. > The if/else duplication for all of OBJ_{COMMIT,TAG,TREE,BLOB} is > unfortunate, but is required to correctly mark the messages for > translation. I think it would probably be OK to put the first paragraph in its own variable. I know we try to avoid translation lego, but I'd think paragraphs are separate units. Or are you worried about how to get them into the same advise() call? I don't know that we need to, but we could also plug one into the other with a "%s" (and leave a translator note). > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 1546833213..fd455e2739 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -320,6 +320,13 @@ advice.*:: > tries to overwrite a remote ref that points at an > object that is not a commit-ish, or make the remote > ref point at an object that is not a commit-ish. > + pushAmbigiousRefName:: > + Shown when linkgit:git-push[1] gives up trying to > + guess based on the source and destination refs what > + remote ref namespace the source belongs in, but where > + we can still suggest that the user push to either > + refs/heads/* or refs/tags/* based on the type of the > + source object. I guess you could argue that this is "ambiguous", but usually we'd use that term to mean "there were two branches that matched on the other side". But here it's actually "no branches matched" (actually, it makes me wonder what we'd do pushing "foo" when that name is ambiguous on the other side). So I wonder if this ought to be pushUnqualifiedRefname or something. > @@ -1046,13 +1047,60 @@ static int match_explicit(struct ref *src, struct ref *dst, > else if ((dst_guess = guess_ref(dst_value, matched_src))) { > matched_dst = make_linked_ref(dst_guess, dst_tail); > free(dst_guess); > - } else > - error(_("unable to push to unqualified destination: %s\n" > - "The destination refspec neither matches an " > - "existing ref on the remote nor\n" > - "begins with refs/, and we are unable to " > - "guess a prefix based on the source ref."), > - dst_value); > + } else { > + struct object_id oid; > + enum object_type type; > + > + error("unable to push to unqualified destination: %s", dst_value); > + if (!advice_push_ambiguous_ref_name) > + break; This break feels funny, because it controls flow much larger than this if/else. It does the right thing now, but it must remain in sync with what comes at the end of that long string of advise() messages. Can we just do it as: if (advice_push_ambiguous_ref_name) { struct object_id oid; enum object_type type; if (get_oid(...)) etc... } instead? That pushes your indentation one level in, but I think the whole conditional body might be cleaner in a helper function anyway. -Peff ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-10 20:55 ` Jeff King @ 2018-10-10 21:23 ` Ævar Arnfjörð Bjarmason 2018-10-11 0:16 ` Jeff King 2018-10-11 22:45 ` Junio C Hamano 2018-10-10 21:54 ` [PATCH 2/2] push: add an advice on unqualified <dst> push Junio C Hamano 1 sibling, 2 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-10 21:23 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano On Wed, Oct 10 2018, Jeff King wrote: > On Wed, Oct 10, 2018 at 10:41:45AM +0000, Ævar Arnfjörð Bjarmason wrote: > >> Improve the error message added in f8aae12034 ("push: allow >> unqualified dest refspecs to DWIM", 2008-04-23), which before this >> change looks like this: >> >> $ git push avar v2.19.0^{commit}:newbranch -n >> error: unable to push to unqualified destination: newbranch >> The destination refspec neither matches an existing ref on the remote nor >> begins with refs/, and we are unable to guess a prefix based on the source ref. >> error: failed to push some refs to 'git@github.com:avar/git.git' > > Thanks for looking into this. Despite being largely responsible for that > message myself, I always cringe when I see it because it's so opaque. > >> This message needed to be read very carefully to spot how to fix the >> error, i.e. to push to refs/heads/newbranch, and it didn't use the >> advice system (since initial addition of the error predated it). >> >> Fix both of those, now the message will look like this instead: >> >> $ ./git-push avar v2.19.0^{commit}:newbranch -n >> error: unable to push to unqualified destination: newbranch >> hint: The destination refspec neither matches an existing >> hint: ref on the remote nor begins with refs/, and we are >> hint: unable to guess a prefix based on the source ref. >> hint: >> hint: The <src> part of the refspec is a commit object. >> hint: Did you mean to create a new branch by pushing to >> hint: 'v2.19.0^{commit}:refs/heads/newbranch'? >> error: failed to push some refs to 'git@github.com:avar/git.git' >> >> When trying to push a tag, tree or a blob we suggest that perhaps the >> user meant to push them to refs/tags/ instead. > > This is much better, and I love the customized behavior based on the > object type. > > I wonder if we could reword the first paragraph to be a little less > confusing, and spell out what we tried already. E.g., something like: > > The destination you provided is not a full refname (i.e., starting > with "ref"). Git tried to guess what you meant by: > > - looking for a matching branch or tag on the remote side > > - looking at the refname of the local source > > but neither worked. > > The <src> part of the refspec is a commit object. > Did you mean... Yeah that makes sense. I was trying to avoid touching the existing wording to make this more surgical, but you came up with it, and since you don't like it I'll just change that too. > I'm not sure about saying "branch or tag" in the first bullet. It's > friendlier to most users, but less technically correct (if you said > "notes/foo", I believe we'd match an existing "refs/notes/foo", because > it's really just using the normal lookup rules). > > Also, as an aside, I wonder if we should allow "heads/foo" to work as > "refs/heads/foo" (even when no such ref already exists). But that is > totally orthogonal to changing the message. > >> The if/else duplication for all of OBJ_{COMMIT,TAG,TREE,BLOB} is >> unfortunate, but is required to correctly mark the messages for >> translation. > > I think it would probably be OK to put the first paragraph in its own > variable. I know we try to avoid translation lego, but I'd think > paragraphs are separate units. Or are you worried about how to get them > into the same advise() call? I don't know that we need to, but we could > also plug one into the other with a "%s" (and leave a translator note). To be honest from being on the code side of a much bigger i18n effort (the MediaWiki/WikiMedia software) back in the early days of my career I just do this sort of thing reflexively, because from experience when I started trying to simplify stuff by making assumptions I was wrong every time. Although in that case there were >100+ languages, so maybe we can get away with this. In this case one red flag I see is that we make a reference to "the source ref" in the first paragraph, and then in the second we'll either talk about "commit", "tag" or "blob" etc. Now imagine a language where those words have different genders, and where even secondary references to those things ("the source ref") spill over and need to be changed too. You also get languages where a message that stretches multiple paragraphs flows more naturally if the wording is re-arranged, even between paragraphs. This is why document translation systems generally split things by sections at best (not paragraphs), or just by whole documents. >> diff --git a/Documentation/config.txt b/Documentation/config.txt >> index 1546833213..fd455e2739 100644 >> --- a/Documentation/config.txt >> +++ b/Documentation/config.txt >> @@ -320,6 +320,13 @@ advice.*:: >> tries to overwrite a remote ref that points at an >> object that is not a commit-ish, or make the remote >> ref point at an object that is not a commit-ish. >> + pushAmbigiousRefName:: >> + Shown when linkgit:git-push[1] gives up trying to >> + guess based on the source and destination refs what >> + remote ref namespace the source belongs in, but where >> + we can still suggest that the user push to either >> + refs/heads/* or refs/tags/* based on the type of the >> + source object. > > I guess you could argue that this is "ambiguous", but usually we'd use > that term to mean "there were two branches that matched on the other > side". But here it's actually "no branches matched" (actually, it makes > me wonder what we'd do pushing "foo" when that name is ambiguous on the > other side). > > So I wonder if this ought to be pushUnqualifiedRefname or something. Yeah that sounds better. Will change it. >> @@ -1046,13 +1047,60 @@ static int match_explicit(struct ref *src, struct ref *dst, >> else if ((dst_guess = guess_ref(dst_value, matched_src))) { >> matched_dst = make_linked_ref(dst_guess, dst_tail); >> free(dst_guess); >> - } else >> - error(_("unable to push to unqualified destination: %s\n" >> - "The destination refspec neither matches an " >> - "existing ref on the remote nor\n" >> - "begins with refs/, and we are unable to " >> - "guess a prefix based on the source ref."), >> - dst_value); >> + } else { >> + struct object_id oid; >> + enum object_type type; >> + >> + error("unable to push to unqualified destination: %s", dst_value); >> + if (!advice_push_ambiguous_ref_name) >> + break; > > This break feels funny, because it controls flow much larger than this > if/else. It does the right thing now, but it must remain in sync with > what comes at the end of that long string of advise() messages. > > Can we just do it as: > > if (advice_push_ambiguous_ref_name) { > struct object_id oid; > enum object_type type; > > if (get_oid(...)) > etc... > } > > instead? That pushes your indentation one level in, but I think the > whole conditional body might be cleaner in a helper function anyway. I started out with that and found myself really constrained by the 72 char ceiling which I'm already smashing through with these ~95 character lines (but at least it's under 100!). But sure, we can do with 8 more. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-10 21:23 ` Ævar Arnfjörð Bjarmason @ 2018-10-11 0:16 ` Jeff King 2018-10-11 22:45 ` Junio C Hamano 1 sibling, 0 replies; 53+ messages in thread From: Jeff King @ 2018-10-11 0:16 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano On Wed, Oct 10, 2018 at 11:23:25PM +0200, Ævar Arnfjörð Bjarmason wrote: > > I wonder if we could reword the first paragraph to be a little less > > confusing, and spell out what we tried already. E.g., something like: > > > > The destination you provided is not a full refname (i.e., starting > > with "ref"). Git tried to guess what you meant by: > > > > - looking for a matching branch or tag on the remote side > > > > - looking at the refname of the local source > > > > but neither worked. > > > > The <src> part of the refspec is a commit object. > > Did you mean... > > Yeah that makes sense. I was trying to avoid touching the existing > wording to make this more surgical, but you came up with it, and since > you don't like it I'll just change that too. I certainly know the feeling of trying to avoid wording bikeshed discussions. But in this instance, please feel free to aggressively rewrite that old message. ;) What I wrote above was off-the-cuff, and I also do not mind if you use it as a starting point to make improvements (or take it wholesale if you really like it). > > I think it would probably be OK to put the first paragraph in its own > > variable. I know we try to avoid translation lego, but I'd think > > paragraphs are separate units. Or are you worried about how to get them > > into the same advise() call? I don't know that we need to, but we could > > also plug one into the other with a "%s" (and leave a translator note). > > To be honest from being on the code side of a much bigger i18n effort > (the MediaWiki/WikiMedia software) back in the early days of my career I > just do this sort of thing reflexively, because from experience when I > started trying to simplify stuff by making assumptions I was wrong every > time. > [...] OK, I'm happy to defer to your judgement here. I have very little translation experience myself. > > Can we just do it as: > > > > if (advice_push_ambiguous_ref_name) { > > struct object_id oid; > > enum object_type type; > > > > if (get_oid(...)) > > etc... > > } > > > > instead? That pushes your indentation one level in, but I think the > > whole conditional body might be cleaner in a helper function anyway. > > I started out with that and found myself really constrained by the 72 > char ceiling which I'm already smashing through with these ~95 character > lines (but at least it's under 100!). But sure, we can do with 8 more. That's why I suggested the helper function. :) I'm also not opposed to pulling messages out to static file-level variables, even if they're only used once. Sometimes it's nice to have them left-aligned (or close to it) to see how they'll actually look in a terminal. -Peff ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-10 21:23 ` Ævar Arnfjörð Bjarmason 2018-10-11 0:16 ` Jeff King @ 2018-10-11 22:45 ` Junio C Hamano 2018-10-26 15:45 ` Ævar Arnfjörð Bjarmason ` (8 more replies) 1 sibling, 9 replies; 53+ messages in thread From: Junio C Hamano @ 2018-10-11 22:45 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, git Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Oct 10 2018, Jeff King wrote: > >> This is much better, and I love the customized behavior based on the >> object type. >> >> I wonder if we could reword the first paragraph to be a little less >> confusing, and spell out what we tried already. E.g., something like: >> ... > > Yeah that makes sense. I was trying to avoid touching the existing > wording to make this more surgical, but you came up with it, and since > you don't like it I'll just change that too. OK, for now I'll mark these two patches "read" in my inbox and forget about them, expecting that a reroll of 2/2 with improved messages would appear not in too distant future. Thanks, both. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-11 22:45 ` Junio C Hamano @ 2018-10-26 15:45 ` Ævar Arnfjörð Bjarmason 2018-10-29 1:00 ` Junio C Hamano 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason ` (7 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 15:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git On Thu, Oct 11 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Wed, Oct 10 2018, Jeff King wrote: >> >>> This is much better, and I love the customized behavior based on the >>> object type. >>> >>> I wonder if we could reword the first paragraph to be a little less >>> confusing, and spell out what we tried already. E.g., something like: >>> ... >> >> Yeah that makes sense. I was trying to avoid touching the existing >> wording to make this more surgical, but you came up with it, and since >> you don't like it I'll just change that too. > > OK, for now I'll mark these two patches "read" in my inbox and > forget about them, expecting that a reroll of 2/2 with improved > messages would appear not in too distant future. I was going to submit an update to this, as an additional improvement can anyone think of a reason not to always infer that we'd like a new branch if the LHS of the refspec starts with refs/remotes/* ? u git (push-advice-on-unqualified-src-2 $>) $ ./git-push avar refs/remotes/origin/master:newbranch -n To github.com:avar/git.git * [new branch] origin/master -> newbranch u git (push-advice-on-unqualified-src-2 $>) $ git diff diff --git a/remote.c b/remote.c index 5b679df02d..949a9bd079 100644 --- a/remote.c +++ b/remote.c @@ -969,7 +969,8 @@ static char *guess_ref(const char *name, struct ref *peer) if (!r) return NULL; - if (starts_with(r, "refs/heads/")) + if (starts_with(r, "refs/heads/") || + starts_with(r, "refs/remotes/")) strbuf_addstr(&buf, "refs/heads/"); else if (starts_with(r, "refs/tags/")) strbuf_addstr(&buf, "refs/tags/"); Maybe we need to be really paranoid here and also check if it's a "commit", i.e. you could setup a refspec like: fetch = +refs/tags/*:refs/remotes/origin-tags/* ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-26 15:45 ` Ævar Arnfjörð Bjarmason @ 2018-10-29 1:00 ` Junio C Hamano 2018-10-29 4:17 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-10-29 1:00 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, git Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I was going to submit an update to this, as an additional improvement > can anyone think of a reason not to always infer that we'd like a new > branch if the LHS of the refspec starts with refs/remotes/* ? Depends on what purpose the remote you are pushing into serves. My instinct tells me that it may be more likely to be emulating the case where the remote, which is hosted on a box on which for some reason it is cumbersome for you to get an interactive shell prompt, did the same fetch as your local repository and stored the same value in its remote-tracking branch than creating a local branch. I do not say it is entirely unlikely that the push wants to create a local branch there, though. It can be a way to "reprint" what somebody else published as their local branch, which you copied to your remote-tracking branches, to the destination of your push. I just felt that it is less likely. To put it another way, I would think both of these two have at most the same probability that the push wants to go to a local branch: git push refs/remotes/foo:foo git push <any random sha1 expression>:foo and I would further say that the former is less likely than the latter that it wants to create a local branch, because it is more plausible that it wants to create a similar remote-tracking branch there. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-29 1:00 ` Junio C Hamano @ 2018-10-29 4:17 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-10-29 4:17 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, git Junio C Hamano <gitster@pobox.com> writes: > To put it another way, I would think both of these two have at most > the same probability that the push wants to go to a local branch: > > git push refs/remotes/foo:foo > git push <any random sha1 expression>:foo > > and I would further say that the former is less likely than the > latter that it wants to create a local branch, because it is more > plausible that it wants to create a similar remote-tracking branch > there. This needs clarification. I do not mean "it is more plausible that it wants remote-tracking rather than local". What I meant was that between the two cases, pushing refs/remotes/foo:foo is more likely a sign that the user wants to create a local branch than pushing other random sha1 expression, like 40eaf9377fe649:foo, is. ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 0/7] fixes for unqualified <dst> push 2018-10-11 22:45 ` Junio C Hamano 2018-10-26 15:45 ` Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 ` Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason ` (8 more replies) 2018-10-26 19:27 ` [PATCH v2 1/7] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason ` (6 subsequent siblings) 8 siblings, 9 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason This has grown to a 7-part series for v2 (from 2 patches). This addresse all the feedback for v1 and then some. Ævar Arnfjörð Bjarmason (7): remote.c: add braces in anticipation of a follow-up change i18n: remote.c: mark error(...) messages for translation push: improve the error shown on unqualified <dst> push push: move unqualified refname error into a function push: add an advice on unqualified <dst> push push: test that <src> doesn't DWYM if <dst> is unqualified push: add DWYM support for "git push refs/remotes/...:<dst>" Documentation/config.txt | 7 +++ advice.c | 2 + advice.h | 1 + remote.c | 124 +++++++++++++++++++++++++++++++-------- t/t5505-remote.sh | 57 ++++++++++++++++++ 5 files changed, 166 insertions(+), 25 deletions(-) -- 2.19.1.759.g500967bb5e ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 0/8] fixes for unqualified <dst> push 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 2018-11-02 6:52 ` Jeff King ` (8 more replies) 2018-10-26 23:07 ` [PATCH v3 1/8] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason ` (7 subsequent siblings) 8 siblings, 9 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason After sending out v2 I noticed I didn't update the examples in a couple of the commit messages, and figured I'd finish this up by adding a patch to document how this works in the "git-push" manpage. This behavior has never been properly documented. range-diff with v2: 1: ca8eb6dc28 = 1: ca8eb6dc28 remote.c: add braces in anticipation of a follow-up change 2: b0e15b6ff1 = 2: b0e15b6ff1 i18n: remote.c: mark error(...) messages for translation 3: 052fc5860e = 3: 052fc5860e push: improve the error shown on unqualified <dst> push 4: e6aa2e360f = 4: e6aa2e360f push: move unqualified refname error into a function 5: 57840952b2 ! 5: dcf566e16e push: add an advice on unqualified <dst> push @@ -13,8 +13,10 @@ error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - - Looking for a ref that matches newbranch on the remote side. - - Looking at the refname of the local source. + - Looking for a ref that matches 'newbranch' on the remote side. + - Checking if the <src> being pushed ('v2.19.0^{commit}') + is a ref in "refs/{heads,tags}/". If so we add a corresponding + refs/{heads,tags}/ prefix on the remote side. Neither worked, so we gave up. You must fully-qualify the ref. hint: The <src> part of the refspec is a commit object. 6: a2d98855cc = 6: 92ff753437 push: test that <src> doesn't DWYM if <dst> is unqualified 7: 4e1953da82 ! 7: 58eeb0f3f3 push: add DWYM support for "git push refs/remotes/...:<dst>" @@ -3,22 +3,44 @@ push: add DWYM support for "git push refs/remotes/...:<dst>" Add DWYM support for pushing a ref in refs/remotes/* when the <dst> - ref is unqualified, e.g.: + ref is unqualified. Now instead of erroring out we support e.g.: - git push origin refs/remotes/origin/master:upstream-master + $ ./git-push avar refs/remotes/origin/master:upstream-master -n + To github.com:avar/git.git + * [new branch] origin/master -> upstream-master Before this we wouldn't know what do do with refs/remotes/origin/master, now we'll look it up and discover that it's a commit (or tag) and add a refs/{heads,tags}/ prefix to <dst> as appropriate. + The error message emitted when we still don't know what to do has been + amended to note that this is something we tried: + + $ ./git-push avar v2.19.0^{commit}:newbranch -n + error: The destination you provided is not a full refname (i.e., + starting with "refs/"). We tried to guess what you meant by: + + - Looking for a ref that matches 'newbranch' on the remote side. + - Checking if the <src> being pushed ('v2.19.0^{commit}') + is a ref in "refs/{heads,tags}/". If so we add a corresponding + refs/{heads,tags}/ prefix on the remote side. + - Checking if the <src> being pushed ('v2.19.0^{commit}') + is a commit or tag in "refs/remotes/*". Then we infer a + corresponding refs/{heads,tags} on the remote side. + + None of those worked, so we gave up. You must fully-qualify the ref. + hint: The <src> part of the refspec is a commit object. + hint: Did you mean to create a new branch by pushing to + hint: 'v2.19.0^{commit}:refs/heads/newbranch'? + I'm bending over backwards and assuming that someone might have hacked in remote tracking tags (see [1] for a discussion of how that can be done), but punting on any tree or blob objects found under refs/remotes/*. This is the first use of the %N$<fmt> style of printf format in - the *.[ch] files in our codebase, but it's supported by POSIX[2] and + the *.[ch] files in our codebase. It's supported by POSIX[2] and there's existing uses for it in po/*.po files, so hopefully it won't cause any trouble. It's more obvious for translators than to have a 3rd argument to the function identical to the 2nd, by re-using the 2nd -: ---------- > 8: bc171b0312 push doc: document the DWYM behavior pushing to unqualified <dst> Ævar Arnfjörð Bjarmason (8): remote.c: add braces in anticipation of a follow-up change i18n: remote.c: mark error(...) messages for translation push: improve the error shown on unqualified <dst> push push: move unqualified refname error into a function push: add an advice on unqualified <dst> push push: test that <src> doesn't DWYM if <dst> is unqualified push: add DWYM support for "git push refs/remotes/...:<dst>" push doc: document the DWYM behavior pushing to unqualified <dst> Documentation/config.txt | 7 +++ Documentation/git-push.txt | 27 ++++++++ advice.c | 2 + advice.h | 1 + remote.c | 124 +++++++++++++++++++++++++++++-------- t/t5505-remote.sh | 57 +++++++++++++++++ 6 files changed, 193 insertions(+), 25 deletions(-) -- 2.19.1.759.g500967bb5e ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 0/8] fixes for unqualified <dst> push 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason @ 2018-11-02 6:52 ` Jeff King 2018-11-13 19:52 ` [PATCH v4 0/7] " Ævar Arnfjörð Bjarmason ` (7 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Jeff King @ 2018-11-02 6:52 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, Stefan Beller On Fri, Oct 26, 2018 at 11:07:33PM +0000, Ævar Arnfjörð Bjarmason wrote: > After sending out v2 I noticed I didn't update the examples in a > couple of the commit messages, and figured I'd finish this up by > adding a patch to document how this works in the "git-push" > manpage. This behavior has never been properly documented. range-diff > with v2: I read over these patches and the comments from Junio. Overall, I like the direction of the first half 5, and tend to agree with the review that the "refs/remotes" thing is going too far. I didn't dig down into reviewing each line, since it looked like Junio already did, and there's probably a re-roll coming. -Peff ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 0/7] fixes for unqualified <dst> push 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason 2018-11-02 6:52 ` Jeff King @ 2018-11-13 19:52 ` Ævar Arnfjörð Bjarmason 2018-11-14 7:00 ` Junio C Hamano 2018-11-13 19:52 ` [PATCH v4 1/7] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason ` (6 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason I've finally re-rolled this. There wasn't consensus for the change to add DWYM behavior to refs/remotes/*, so I've dropped it, 7/7 is still there testing the current behavior of what we do with that now, since we didn't have any tests for that. This should address all feedback on v3, except I haven't reworded the "this is what we do, even though you didn't do that now" message Junio wasn't happy with in https://public-inbox.org/git/xmqq8t2h5opm.fsf@gitster-ct.c.googlers.com/ I agree that the wording he was critiquing doesn't make sense at that point in the series, but I think if you look over to 5/7 the final state makes more sense. I.e. our message is of the form: Error, you tried to do X. Sometimes we DWYM, but only in cases: - A - B hint: Did you mean to do this thing that'll give you DWYM behavior like what we'd do for "B"? I think in that case it makes sense to mention "B" in the message to call out what exactly it is we DWYM on, why we didn't do it now, and what you should do if that was what you meant. Not mentioning B would be more confusing. But maybe there's still disagreement on that and we can work out the details of the wording, but now that a bunch of bugs have been fixed & the controversial part ejected that'll be easier. 1: ca8eb6dc28 = 1: a6b6a5bba5 remote.c: add braces in anticipation of a follow-up change 2: b0e15b6ff1 = 2: 3335fcedc8 i18n: remote.c: mark error(...) messages for translation 3: 052fc5860e ! 3: 18a5a685e7 push: improve the error shown on unqualified <dst> push @@ -22,10 +22,10 @@ - Looking for a ref that matches 'newbranch' on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') - is a ref in "refs/{heads,tags}/". If so we add a - corresponding refs/{heads,tags}/ prefix on the remote side. + is a ref in "refs/{heads,tags}/". If so we add a corresponding + refs/{heads,tags}/ prefix on the remote side. - Neither worked, so we gave up. You must fully-qualify the ref. + Neither worked, so we gave up. You must fully qualify the ref. error: failed to push some refs to 'git@github.com:avar/git.git' This improvement is the result of on-list discussion in [1] and [2], @@ -73,7 +73,7 @@ + " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" + " refs/{heads,tags}/ prefix on the remote side.\n" + "\n" -+ "Neither worked, so we gave up. You must fully-qualify the ref."), ++ "Neither worked, so we gave up. You must fully qualify the ref."), + dst_value, matched_src->name); } break; 4: e6aa2e360f ! 4: a10d286cf6 push: move unqualified refname error into a function @@ -34,7 +34,7 @@ + " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" + " refs/{heads,tags}/ prefix on the remote side.\n" + "\n" -+ "Neither worked, so we gave up. You must fully-qualify the ref."), ++ "Neither worked, so we gave up. You must fully qualify the ref."), + dst_value, matched_src_name); +} + @@ -59,7 +59,7 @@ - " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" - " refs/{heads,tags}/ prefix on the remote side.\n" - "\n" -- "Neither worked, so we gave up. You must fully-qualify the ref."), +- "Neither worked, so we gave up. You must fully qualify the ref."), - dst_value, matched_src->name); + show_push_unqualified_ref_name_error(dst_value, + matched_src->name); 5: dcf566e16e ! 5: 6f34c6f753 push: add an advice on unqualified <dst> push @@ -18,7 +18,7 @@ is a ref in "refs/{heads,tags}/". If so we add a corresponding refs/{heads,tags}/ prefix on the remote side. - Neither worked, so we gave up. You must fully-qualify the ref. + Neither worked, so we gave up. You must fully qualify the ref. hint: The <src> part of the refspec is a commit object. hint: Did you mean to create a new branch by pushing to hint: 'v2.19.0^{commit}:refs/heads/newbranch'? @@ -34,9 +34,9 @@ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> - diff --git a/Documentation/config.txt b/Documentation/config.txt - --- a/Documentation/config.txt - +++ b/Documentation/config.txt + diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt + --- a/Documentation/config/advice.txt + +++ b/Documentation/config/advice.txt @@ tries to overwrite a remote ref that points at an object that is not a commit-ish, or make the remote @@ -107,7 +107,7 @@ * <remote> <src>:<dst>" push, and "being pushed ('%s')" is @@ "\n" - "Neither worked, so we gave up. You must fully-qualify the ref."), + "Neither worked, so we gave up. You must fully qualify the ref."), dst_value, matched_src_name); + + if (!advice_push_unqualified_ref_name) @@ -158,6 +158,7 @@ + ( + cd test && + git tag -a -m "Some tag" some-tag master && ++ exit_with=true && + for type in commit tag tree blob + do + if test "$type" = "blob" @@ -172,8 +173,10 @@ + test_must_fail git -c advice.pushUnqualifiedRefName=false \ + push origin $oid:dst 2>err && + test_i18ngrep "error: The destination you" err && -+ test_i18ngrep ! "hint: Did you mean" err -+ done ++ test_i18ngrep ! "hint: Did you mean" err || ++ exit_with=false ++ done && ++ $exit_with + ) +' + 6: 92ff753437 ! 6: 86cb0bbd95 push: test that <src> doesn't DWYM if <dst> is unqualified @@ -4,14 +4,22 @@ Add a test asserting that "git push origin <src>:<dst>" where <src> is a branch, tag, tree or blob in refs/remotes/* doesn't DWYM when <dst> - is unqualified. This has never worked, but there's been no test for - this behavior. + is unqualified. This has never been the case, but there haven't been + any tests for this behavior. See f88395ac23 ("Renaming push.", 2005-08-03), bb9fca80ce ("git-push: Update description of refspecs and add examples", 2007-06-09) and f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23) which are most relevant commits that have changed or - documented the behavior of this feature in the past. + documented the behavior of the DWYM feature in the past. + + These tests were originally meant to lead up to a patch that made + refs/remotes/* on the LHS imply refs/heads/* on the RHS, see [1]. That + patch proved controversial and may not ever land in git.git, but we + should have the tests that remind us what the current behavior is in + case it's ever changed. + + 1. https://public-inbox.org/git/20181026230741.23321-8-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> @@ -25,30 +33,29 @@ +test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and advice' ' + ( + cd two && -+ git tag -a -m "Some tag" some-tag master && ++ git tag -a -m "Some tag" my-tag master && + git update-ref refs/trees/my-head-tree HEAD^{tree} && + git update-ref refs/blobs/my-file-blob HEAD:file + ) && + ( + cd test && -+ git config --add remote.two.fetch "+refs/tags/*:refs/remotes/two-tags/*" && -+ git config --add remote.two.fetch "+refs/trees/*:refs/remotes/two-trees/*" && -+ git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/two-blobs/*" && ++ git config --add remote.two.fetch "+refs/tags/*:refs/remotes/tags-from-two/*" && ++ git config --add remote.two.fetch "+refs/trees/*:refs/remotes/trees-from-two/*" && ++ git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/blobs-from-two/*" && + git fetch --no-tags two && + + test_must_fail git push origin refs/remotes/two/another:dst 2>err && + test_i18ngrep "error: The destination you" err && + -+ test_must_fail git push origin refs/remotes/two-tags/some-tag:dst-tag 2>err && ++ test_must_fail git push origin refs/remotes/tags-from-two/my-tag:dst-tag 2>err && + test_i18ngrep "error: The destination you" err && + -+ test_must_fail git push origin refs/remotes/two-trees/my-head-tree:dst-tree 2>err && ++ test_must_fail git push origin refs/remotes/trees-from-two/my-head-tree:dst-tree 2>err && + test_i18ngrep "error: The destination you" err && + -+ test_must_fail git push origin refs/remotes/two-blobs/my-file-blob:dst-blob 2>err && ++ test_must_fail git push origin refs/remotes/blobs-from-two/my-file-blob:dst-blob 2>err && + test_i18ngrep "error: The destination you" err + ) +' -+ test_done 7: 58eeb0f3f3 < -: ---------- push: add DWYM support for "git push refs/remotes/...:<dst>" 8: bc171b0312 ! 7: d5e800cb87 push doc: document the DWYM behavior pushing to unqualified <dst> @@ -39,10 +39,6 @@ +* If <src> resolves to a ref starting with refs/heads/ or refs/tags/, + then prepend that to <dst>. + -+* If <src> starts with refs/remotes/ check if that reference refers to -+ a commit or tag, then refs/heads/ or refs/tags/ to <dst> as -+ appropriate. -+ +* Other ambiguity resolutions might be added in the future, but for + now any other cases will error out with an error indicating what we + tried, and depending on the `advice.pushUnqualifiedRefname` Ævar Arnfjörð Bjarmason (7): remote.c: add braces in anticipation of a follow-up change i18n: remote.c: mark error(...) messages for translation push: improve the error shown on unqualified <dst> push push: move unqualified refname error into a function push: add an advice on unqualified <dst> push push: test that <src> doesn't DWYM if <dst> is unqualified push doc: document the DWYM behavior pushing to unqualified <dst> Documentation/config/advice.txt | 7 +++ Documentation/git-push.txt | 23 +++++++ advice.c | 2 + advice.h | 1 + remote.c | 106 ++++++++++++++++++++++++-------- t/t5505-remote.sh | 55 +++++++++++++++++ 6 files changed, 169 insertions(+), 25 deletions(-) -- 2.19.1.1182.g4ecb1133ce ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 0/7] fixes for unqualified <dst> push 2018-11-13 19:52 ` [PATCH v4 0/7] " Ævar Arnfjörð Bjarmason @ 2018-11-14 7:00 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-11-14 7:00 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Thanks for an update; will replace. ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 1/7] remote.c: add braces in anticipation of a follow-up change 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason 2018-11-02 6:52 ` Jeff King 2018-11-13 19:52 ` [PATCH v4 0/7] " Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 ` Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 2/7] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason ` (5 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason The CodingGuidelines say "When there are multiple arms to a conditional and some of them require braces, enclose even a single line block in braces for consistency.". Fix the code in match_explicit() to conform. While I'm at it change the if/else if/else in guess_ref() to use braces. This is not currently needed, but a follow-up change will add a new multi-line condition to that logic. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/remote.c b/remote.c index b850f2feb3..695b379a44 100644 --- a/remote.c +++ b/remote.c @@ -968,12 +968,13 @@ static char *guess_ref(const char *name, struct ref *peer) if (!r) return NULL; - if (starts_with(r, "refs/heads/")) + if (starts_with(r, "refs/heads/")) { strbuf_addstr(&buf, "refs/heads/"); - else if (starts_with(r, "refs/tags/")) + } else if (starts_with(r, "refs/tags/")) { strbuf_addstr(&buf, "refs/tags/"); - else + } else { return NULL; + } strbuf_addstr(&buf, name); return strbuf_detach(&buf, NULL); @@ -1038,21 +1039,22 @@ static int match_explicit(struct ref *src, struct ref *dst, case 1: break; case 0: - if (starts_with(dst_value, "refs/")) + if (starts_with(dst_value, "refs/")) { matched_dst = make_linked_ref(dst_value, dst_tail); - else if (is_null_oid(&matched_src->new_oid)) + } else if (is_null_oid(&matched_src->new_oid)) { error("unable to delete '%s': remote ref does not exist", dst_value); - else if ((dst_guess = guess_ref(dst_value, matched_src))) { + } else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); - } else + } else { error("unable to push to unqualified destination: %s\n" "The destination refspec neither matches an " "existing ref on the remote nor\n" "begins with refs/, and we are unable to " "guess a prefix based on the source ref.", dst_value); + } break; default: matched_dst = NULL; -- 2.19.1.1182.g4ecb1133ce ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 2/7] i18n: remote.c: mark error(...) messages for translation 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2018-11-13 19:52 ` [PATCH v4 1/7] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 ` Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 3/7] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Mark up the error(...) messages in remote.c for translation. The likes of "unable to push to unqualified destination" are relatively big parts of the UI, i.e. error messages shown when "git push" fails. I don't think any of these are plumbing, an the entire test suite passes when running the tests with GIT_GETTEXT_POISON=1 (after building with GETTEXT_POISON). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/remote.c b/remote.c index 695b379a44..35fe8a178f 100644 --- a/remote.c +++ b/remote.c @@ -406,7 +406,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!remote->receivepack) remote->receivepack = v; else - error("more than one receivepack given, using the first"); + error(_("more than one receivepack given, using the first")); } else if (!strcmp(subkey, "uploadpack")) { const char *v; if (git_config_string(&v, key, value)) @@ -414,7 +414,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!remote->uploadpack) remote->uploadpack = v; else - error("more than one uploadpack given, using the first"); + error(_("more than one uploadpack given, using the first")); } else if (!strcmp(subkey, "tagopt")) { if (!strcmp(value, "--no-tags")) remote->fetch_tags = -1; @@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs, int find_src = !query->src; if (find_src && !query->dst) - error("query_refspecs_multiple: need either src or dst"); + error(_("query_refspecs_multiple: need either src or dst")); for (i = 0; i < rs->nr; i++) { struct refspec_item *refspec = &rs->items[i]; @@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query) char **result = find_src ? &query->src : &query->dst; if (find_src && !query->dst) - return error("query_refspecs: need either src or dst"); + return error(_("query_refspecs: need either src or dst")); for (i = 0; i < rs->nr; i++) { struct refspec_item *refspec = &rs->items[i]; @@ -996,12 +996,12 @@ static int match_explicit_lhs(struct ref *src, * way to delete 'other' ref at the remote end. */ if (try_explicit_object_name(rs->src, match) < 0) - return error("src refspec %s does not match any.", rs->src); + return error(_("src refspec %s does not match any."), rs->src); if (allocated_match) *allocated_match = 1; return 0; default: - return error("src refspec %s matches more than one.", rs->src); + return error(_("src refspec %s matches more than one."), rs->src); } } @@ -1041,32 +1041,33 @@ static int match_explicit(struct ref *src, struct ref *dst, case 0: if (starts_with(dst_value, "refs/")) { matched_dst = make_linked_ref(dst_value, dst_tail); + } else if (is_null_oid(&matched_src->new_oid)) { - error("unable to delete '%s': remote ref does not exist", + error(_("unable to delete '%s': remote ref does not exist"), dst_value); } else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - error("unable to push to unqualified destination: %s\n" - "The destination refspec neither matches an " - "existing ref on the remote nor\n" - "begins with refs/, and we are unable to " - "guess a prefix based on the source ref.", + error(_("unable to push to unqualified destination: %s\n" + "The destination refspec neither matches an " + "existing ref on the remote nor\n" + "begins with refs/, and we are unable to " + "guess a prefix based on the source ref."), dst_value); } break; default: matched_dst = NULL; - error("dst refspec %s matches more than one.", + error(_("dst refspec %s matches more than one."), dst_value); break; } if (!matched_dst) return -1; if (matched_dst->peer_ref) - return error("dst ref %s receives from more than one src.", - matched_dst->name); + return error(_("dst ref %s receives from more than one src."), + matched_dst->name); else { matched_dst->peer_ref = allocated_src ? matched_src : @@ -1797,7 +1798,7 @@ int get_fetch_map(const struct ref *remote_refs, if (!starts_with((*rmp)->peer_ref->name, "refs/") || check_refname_format((*rmp)->peer_ref->name, 0)) { struct ref *ignore = *rmp; - error("* Ignoring funny ref '%s' locally", + error(_("* Ignoring funny ref '%s' locally"), (*rmp)->peer_ref->name); *rmp = (*rmp)->next; free(ignore->peer_ref); @@ -2165,7 +2166,7 @@ static int parse_push_cas_option(struct push_cas_option *cas, const char *arg, i else if (!colon[1]) oidclr(&entry->expect); else if (get_oid(colon + 1, &entry->expect)) - return error("cannot parse expected object name '%s'", colon + 1); + return error(_("cannot parse expected object name '%s'"), colon + 1); return 0; } -- 2.19.1.1182.g4ecb1133ce ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 3/7] push: improve the error shown on unqualified <dst> push 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2018-11-13 19:52 ` [PATCH v4 2/7] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 ` Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 4/7] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Improve the error message added in f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23), which before this change looks like this: $ git push avar v2.19.0^{commit}:newbranch -n error: unable to push to unqualified destination: newbranch The destination refspec neither matches an existing ref on the remote nor begins with refs/, and we are unable to guess a prefix based on the source ref. error: failed to push some refs to 'git@github.com:avar/git.git' This message needed to be read very carefully to spot how to fix the error, i.e. to push to refs/heads/newbranch. Now the message will look like this instead: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - Looking for a ref that matches 'newbranch' on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a ref in "refs/{heads,tags}/". If so we add a corresponding refs/{heads,tags}/ prefix on the remote side. Neither worked, so we gave up. You must fully qualify the ref. error: failed to push some refs to 'git@github.com:avar/git.git' This improvement is the result of on-list discussion in [1] and [2], as well as my own fixes / reformatting / phrasing on top. The suggestion by Jeff on-list was to make that second bullet point "Looking at the refname of the local source.". The version being added here is more verbose, but also more accurate. saying "local source" could refer to any ref in the local refstore, including something in refs/remotes/*. A later change will teach guess_ref() to infer a ref type from remote-tracking refs, so let's not confuse the two. While I'm at it, add a "TRANSLATORS" comment since the message has gotten more complex and it's not as clear what the two %s's refer to. 1. https://public-inbox.org/git/20181010205505.GB12949@sigill.intra.peff.net/ 2. https://public-inbox.org/git/xmqqbm81lb7c.fsf@gitster-ct.c.googlers.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/remote.c b/remote.c index 35fe8a178f..15da4019c3 100644 --- a/remote.c +++ b/remote.c @@ -1049,12 +1049,22 @@ static int match_explicit(struct ref *src, struct ref *dst, matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - error(_("unable to push to unqualified destination: %s\n" - "The destination refspec neither matches an " - "existing ref on the remote nor\n" - "begins with refs/, and we are unable to " - "guess a prefix based on the source ref."), - dst_value); + /* + * TRANSLATORS: "matches '%s'%" is the <dst> + * part of "git push <remote> <src>:<dst>" + * push, and "being pushed ('%s')" is the + * <src>. + */ + error(_("The destination you provided is not a full refname (i.e.,\n" + "starting with \"refs/\"). We tried to guess what you meant by:\n" + "\n" + "- Looking for a ref that matches '%s' on the remote side.\n" + "- Checking if the <src> being pushed ('%s')\n" + " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" + " refs/{heads,tags}/ prefix on the remote side.\n" + "\n" + "Neither worked, so we gave up. You must fully qualify the ref."), + dst_value, matched_src->name); } break; default: -- 2.19.1.1182.g4ecb1133ce ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 4/7] push: move unqualified refname error into a function 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2018-11-13 19:52 ` [PATCH v4 3/7] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 ` Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 5/7] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason A follow-up change will extend this error message with the advice facility. Doing so would make the indentation too deeply nested for comfort. So let's split this into a helper function. There's no changes to the wording here. Just code moving & re-indentation, and re-flowing the "TRANSLATORS" comment. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/remote.c b/remote.c index 15da4019c3..ba8abf4d32 100644 --- a/remote.c +++ b/remote.c @@ -1005,6 +1005,26 @@ static int match_explicit_lhs(struct ref *src, } } +static void show_push_unqualified_ref_name_error(const char *dst_value, + const char *matched_src_name) +{ + /* + * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push + * <remote> <src>:<dst>" push, and "being pushed ('%s')" is + * the <src>. + */ + error(_("The destination you provided is not a full refname (i.e.,\n" + "starting with \"refs/\"). We tried to guess what you meant by:\n" + "\n" + "- Looking for a ref that matches '%s' on the remote side.\n" + "- Checking if the <src> being pushed ('%s')\n" + " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" + " refs/{heads,tags}/ prefix on the remote side.\n" + "\n" + "Neither worked, so we gave up. You must fully qualify the ref."), + dst_value, matched_src_name); +} + static int match_explicit(struct ref *src, struct ref *dst, struct ref ***dst_tail, struct refspec_item *rs) @@ -1049,22 +1069,8 @@ static int match_explicit(struct ref *src, struct ref *dst, matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - /* - * TRANSLATORS: "matches '%s'%" is the <dst> - * part of "git push <remote> <src>:<dst>" - * push, and "being pushed ('%s')" is the - * <src>. - */ - error(_("The destination you provided is not a full refname (i.e.,\n" - "starting with \"refs/\"). We tried to guess what you meant by:\n" - "\n" - "- Looking for a ref that matches '%s' on the remote side.\n" - "- Checking if the <src> being pushed ('%s')\n" - " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" - " refs/{heads,tags}/ prefix on the remote side.\n" - "\n" - "Neither worked, so we gave up. You must fully qualify the ref."), - dst_value, matched_src->name); + show_push_unqualified_ref_name_error(dst_value, + matched_src->name); } break; default: -- 2.19.1.1182.g4ecb1133ce ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 5/7] push: add an advice on unqualified <dst> push 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason ` (5 preceding siblings ...) 2018-11-13 19:52 ` [PATCH v4 4/7] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 ` Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 6/7] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 7/7] push doc: document the DWYM behavior pushing to unqualified <dst> Ævar Arnfjörð Bjarmason 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Add an advice to the recently improved error message added in f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23). Now with advice.pushUnqualifiedRefName=true (on by default) we show a hint about how to proceed: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - Looking for a ref that matches 'newbranch' on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a ref in "refs/{heads,tags}/". If so we add a corresponding refs/{heads,tags}/ prefix on the remote side. Neither worked, so we gave up. You must fully qualify the ref. hint: The <src> part of the refspec is a commit object. hint: Did you mean to create a new branch by pushing to hint: 'v2.19.0^{commit}:refs/heads/newbranch'? error: failed to push some refs to 'git@github.com:avar/git.git' When trying to push a tag, tree or a blob we suggest that perhaps the user meant to push them to refs/tags/ instead. The if/else duplication for all of OBJ_{COMMIT,TAG,TREE,BLOB} is unfortunate, but is required to correctly mark the messages for translation. See the discussion in <87r2gxebsi.fsf@evledraar.gmail.com> about that. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/config/advice.txt | 7 +++++++ advice.c | 2 ++ advice.h | 1 + remote.c | 37 +++++++++++++++++++++++++++++++++ t/t5505-remote.sh | 28 +++++++++++++++++++++++++ 5 files changed, 75 insertions(+) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index 57fcd4c862..88620429ea 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -30,6 +30,13 @@ advice.*:: tries to overwrite a remote ref that points at an object that is not a commit-ish, or make the remote ref point at an object that is not a commit-ish. + pushUnqualifiedRefname:: + Shown when linkgit:git-push[1] gives up trying to + guess based on the source and destination refs what + remote ref namespace the source belongs in, but where + we can still suggest that the user push to either + refs/heads/* or refs/tags/* based on the type of the + source object. statusHints:: Show directions on how to proceed from the current state in the output of linkgit:git-status[1], in diff --git a/advice.c b/advice.c index 5f35656409..567209aa79 100644 --- a/advice.c +++ b/advice.c @@ -9,6 +9,7 @@ int advice_push_non_ff_matching = 1; int advice_push_already_exists = 1; int advice_push_fetch_first = 1; int advice_push_needs_force = 1; +int advice_push_unqualified_ref_name = 1; int advice_status_hints = 1; int advice_status_u_option = 1; int advice_commit_before_merge = 1; @@ -63,6 +64,7 @@ static struct { { "pushAlreadyExists", &advice_push_already_exists }, { "pushFetchFirst", &advice_push_fetch_first }, { "pushNeedsForce", &advice_push_needs_force }, + { "pushUnqualifiedRefName", &advice_push_unqualified_ref_name }, { "statusHints", &advice_status_hints }, { "statusUoption", &advice_status_u_option }, { "commitBeforeMerge", &advice_commit_before_merge }, diff --git a/advice.h b/advice.h index 696bf0e7d2..f875f8cd8d 100644 --- a/advice.h +++ b/advice.h @@ -9,6 +9,7 @@ extern int advice_push_non_ff_matching; extern int advice_push_already_exists; extern int advice_push_fetch_first; extern int advice_push_needs_force; +extern int advice_push_unqualified_ref_name; extern int advice_status_hints; extern int advice_status_u_option; extern int advice_commit_before_merge; diff --git a/remote.c b/remote.c index ba8abf4d32..f7477b8eb6 100644 --- a/remote.c +++ b/remote.c @@ -13,6 +13,7 @@ #include "mergesort.h" #include "argv-array.h" #include "commit-reach.h" +#include "advice.h" enum map_direction { FROM_SRC, FROM_DST }; @@ -1008,6 +1009,9 @@ static int match_explicit_lhs(struct ref *src, static void show_push_unqualified_ref_name_error(const char *dst_value, const char *matched_src_name) { + struct object_id oid; + enum object_type type; + /* * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push * <remote> <src>:<dst>" push, and "being pushed ('%s')" is @@ -1023,6 +1027,39 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, "\n" "Neither worked, so we gave up. You must fully qualify the ref."), dst_value, matched_src_name); + + if (!advice_push_unqualified_ref_name) + return; + + if (get_oid(matched_src_name, &oid)) + BUG("'%s' is not a valid object, " + "match_explicit_lhs() should catch this!", + matched_src_name); + type = oid_object_info(the_repository, &oid, NULL); + if (type == OBJ_COMMIT) { + advise(_("The <src> part of the refspec is a commit object.\n" + "Did you mean to create a new branch by pushing to\n" + "'%s:refs/heads/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_TAG) { + advise(_("The <src> part of the refspec is a tag object.\n" + "Did you mean to create a new tag by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_TREE) { + advise(_("The <src> part of the refspec is a tree object.\n" + "Did you mean to tag a new tree by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_BLOB) { + advise(_("The <src> part of the refspec is a blob object.\n" + "Did you mean to tag a new blob by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else { + BUG("'%s' should be commit/tag/tree/blob, is '%d'", + matched_src_name, type); + } } static int match_explicit(struct ref *src, struct ref *dst, diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index d2a2cdd453..f069cbcc24 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1222,4 +1222,32 @@ test_expect_success 'add remote matching the "insteadOf" URL' ' git remote add backup xyz@example.com ' +test_expect_success 'unqualified <dst> refspec DWIM and advice' ' + test_when_finished "(cd test && git tag -d some-tag)" && + ( + cd test && + git tag -a -m "Some tag" some-tag master && + exit_with=true && + for type in commit tag tree blob + do + if test "$type" = "blob" + then + oid=$(git rev-parse some-tag:file) + else + oid=$(git rev-parse some-tag^{$type}) + fi && + test_must_fail git push origin $oid:dst 2>err && + test_i18ngrep "error: The destination you" err && + test_i18ngrep "hint: Did you mean" err && + test_must_fail git -c advice.pushUnqualifiedRefName=false \ + push origin $oid:dst 2>err && + test_i18ngrep "error: The destination you" err && + test_i18ngrep ! "hint: Did you mean" err || + exit_with=false + done && + $exit_with + ) +' + + test_done -- 2.19.1.1182.g4ecb1133ce ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 6/7] push: test that <src> doesn't DWYM if <dst> is unqualified 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason ` (6 preceding siblings ...) 2018-11-13 19:52 ` [PATCH v4 5/7] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 ` Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 7/7] push doc: document the DWYM behavior pushing to unqualified <dst> Ævar Arnfjörð Bjarmason 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Add a test asserting that "git push origin <src>:<dst>" where <src> is a branch, tag, tree or blob in refs/remotes/* doesn't DWYM when <dst> is unqualified. This has never been the case, but there haven't been any tests for this behavior. See f88395ac23 ("Renaming push.", 2005-08-03), bb9fca80ce ("git-push: Update description of refspecs and add examples", 2007-06-09) and f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23) which are most relevant commits that have changed or documented the behavior of the DWYM feature in the past. These tests were originally meant to lead up to a patch that made refs/remotes/* on the LHS imply refs/heads/* on the RHS, see [1]. That patch proved controversial and may not ever land in git.git, but we should have the tests that remind us what the current behavior is in case it's ever changed. 1. https://public-inbox.org/git/20181026230741.23321-8-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5505-remote.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index f069cbcc24..883b32efa0 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1249,5 +1249,32 @@ test_expect_success 'unqualified <dst> refspec DWIM and advice' ' ) ' +test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and advice' ' + ( + cd two && + git tag -a -m "Some tag" my-tag master && + git update-ref refs/trees/my-head-tree HEAD^{tree} && + git update-ref refs/blobs/my-file-blob HEAD:file + ) && + ( + cd test && + git config --add remote.two.fetch "+refs/tags/*:refs/remotes/tags-from-two/*" && + git config --add remote.two.fetch "+refs/trees/*:refs/remotes/trees-from-two/*" && + git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/blobs-from-two/*" && + git fetch --no-tags two && + + test_must_fail git push origin refs/remotes/two/another:dst 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/tags-from-two/my-tag:dst-tag 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/trees-from-two/my-head-tree:dst-tree 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/blobs-from-two/my-file-blob:dst-blob 2>err && + test_i18ngrep "error: The destination you" err + ) +' test_done -- 2.19.1.1182.g4ecb1133ce ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 7/7] push doc: document the DWYM behavior pushing to unqualified <dst> 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason ` (7 preceding siblings ...) 2018-11-13 19:52 ` [PATCH v4 6/7] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 ` Ævar Arnfjörð Bjarmason 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-13 19:52 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Document the DWYM behavior that kicks in when pushing to an unqualified <dst> reference. This behavior was added in f88395ac23 ("Renaming push.", 2005-08-03) and f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23), and somewhat documented in bb9fca80ce ("git-push: Update description of refspecs and add examples", 2007-06-09), but has never been fully documented. The closest we got to having documented it was the description in the commit message for f8aae12034, which I've borrowed from in writing this documentation. Let's also refer to this new documentation from the existing documentation we had (added in bb9fca80ce). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/git-push.txt | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index a5fc54aeab..6a8a0d958b 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -73,6 +73,26 @@ be omitted--such a push will update a ref that `<src>` normally updates without any `<refspec>` on the command line. Otherwise, missing `:<dst>` means to update the same ref as the `<src>`. + +If <dst> doesn't start with `refs/` (e.g. `refs/heads/master`) we will +try to infer where in `refs/*` on the destination <repository> it +belongs based on the the type of <src> being pushed and whether <dst> +is ambiguous. ++ +-- +* If <dst> unambiguously refers to a ref on the <repository> remote, + then push to that ref. + +* If <src> resolves to a ref starting with refs/heads/ or refs/tags/, + then prepend that to <dst>. + +* Other ambiguity resolutions might be added in the future, but for + now any other cases will error out with an error indicating what we + tried, and depending on the `advice.pushUnqualifiedRefname` + configuration (see linkgit:git-config[1]) suggest what refs/ + namespace you may have wanted to push to. + +-- ++ The object referenced by <src> is used to update the <dst> reference on the remote side. Whether this is allowed depends on where in `refs/*` the <dst> reference lives as described in detail below, in @@ -591,6 +611,9 @@ the ones in the examples below) can be configured as the default for `refs/remotes/satellite/master`) in the `mothership` repository; do the same for `dev` and `satellite/dev`. + +See the section describing `<refspec>...` above for a discussion of +the matching semantics. ++ This is to emulate `git fetch` run on the `mothership` using `git push` that is run in the opposite direction in order to integrate the work done on `satellite`, and is often necessary when you can -- 2.19.1.1182.g4ecb1133ce ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 1/8] remote.c: add braces in anticipation of a follow-up change 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 2/8] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason ` (6 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason The CodingGuidelines say "When there are multiple arms to a conditional and some of them require braces, enclose even a single line block in braces for consistency.". Fix the code in match_explicit() to conform. While I'm at it change the if/else if/else in guess_ref() to use braces. This is not currently needed, but a follow-up change will add a new multi-line condition to that logic. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/remote.c b/remote.c index 81f4f01b00..18cae48daa 100644 --- a/remote.c +++ b/remote.c @@ -968,12 +968,13 @@ static char *guess_ref(const char *name, struct ref *peer) if (!r) return NULL; - if (starts_with(r, "refs/heads/")) + if (starts_with(r, "refs/heads/")) { strbuf_addstr(&buf, "refs/heads/"); - else if (starts_with(r, "refs/tags/")) + } else if (starts_with(r, "refs/tags/")) { strbuf_addstr(&buf, "refs/tags/"); - else + } else { return NULL; + } strbuf_addstr(&buf, name); return strbuf_detach(&buf, NULL); @@ -1038,21 +1039,22 @@ static int match_explicit(struct ref *src, struct ref *dst, case 1: break; case 0: - if (starts_with(dst_value, "refs/")) + if (starts_with(dst_value, "refs/")) { matched_dst = make_linked_ref(dst_value, dst_tail); - else if (is_null_oid(&matched_src->new_oid)) + } else if (is_null_oid(&matched_src->new_oid)) { error("unable to delete '%s': remote ref does not exist", dst_value); - else if ((dst_guess = guess_ref(dst_value, matched_src))) { + } else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); - } else + } else { error("unable to push to unqualified destination: %s\n" "The destination refspec neither matches an " "existing ref on the remote nor\n" "begins with refs/, and we are unable to " "guess a prefix based on the source ref.", dst_value); + } break; default: matched_dst = NULL; -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 2/8] i18n: remote.c: mark error(...) messages for translation 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 1/8] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 3/8] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason ` (5 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Mark up the error(...) messages in remote.c for translation. The likes of "unable to push to unqualified destination" are relatively big parts of the UI, i.e. error messages shown when "git push" fails. I don't think any of these are plumbing, an the entire test suite passes when running the tests with GIT_GETTEXT_POISON=1 (after building with GETTEXT_POISON). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/remote.c b/remote.c index 18cae48daa..5cb3d00bfb 100644 --- a/remote.c +++ b/remote.c @@ -406,7 +406,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!remote->receivepack) remote->receivepack = v; else - error("more than one receivepack given, using the first"); + error(_("more than one receivepack given, using the first")); } else if (!strcmp(subkey, "uploadpack")) { const char *v; if (git_config_string(&v, key, value)) @@ -414,7 +414,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!remote->uploadpack) remote->uploadpack = v; else - error("more than one uploadpack given, using the first"); + error(_("more than one uploadpack given, using the first")); } else if (!strcmp(subkey, "tagopt")) { if (!strcmp(value, "--no-tags")) remote->fetch_tags = -1; @@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs, int find_src = !query->src; if (find_src && !query->dst) - error("query_refspecs_multiple: need either src or dst"); + error(_("query_refspecs_multiple: need either src or dst")); for (i = 0; i < rs->nr; i++) { struct refspec_item *refspec = &rs->items[i]; @@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query) char **result = find_src ? &query->src : &query->dst; if (find_src && !query->dst) - return error("query_refspecs: need either src or dst"); + return error(_("query_refspecs: need either src or dst")); for (i = 0; i < rs->nr; i++) { struct refspec_item *refspec = &rs->items[i]; @@ -996,12 +996,12 @@ static int match_explicit_lhs(struct ref *src, * way to delete 'other' ref at the remote end. */ if (try_explicit_object_name(rs->src, match) < 0) - return error("src refspec %s does not match any.", rs->src); + return error(_("src refspec %s does not match any."), rs->src); if (allocated_match) *allocated_match = 1; return 0; default: - return error("src refspec %s matches more than one.", rs->src); + return error(_("src refspec %s matches more than one."), rs->src); } } @@ -1041,32 +1041,33 @@ static int match_explicit(struct ref *src, struct ref *dst, case 0: if (starts_with(dst_value, "refs/")) { matched_dst = make_linked_ref(dst_value, dst_tail); + } else if (is_null_oid(&matched_src->new_oid)) { - error("unable to delete '%s': remote ref does not exist", + error(_("unable to delete '%s': remote ref does not exist"), dst_value); } else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - error("unable to push to unqualified destination: %s\n" - "The destination refspec neither matches an " - "existing ref on the remote nor\n" - "begins with refs/, and we are unable to " - "guess a prefix based on the source ref.", + error(_("unable to push to unqualified destination: %s\n" + "The destination refspec neither matches an " + "existing ref on the remote nor\n" + "begins with refs/, and we are unable to " + "guess a prefix based on the source ref."), dst_value); } break; default: matched_dst = NULL; - error("dst refspec %s matches more than one.", + error(_("dst refspec %s matches more than one."), dst_value); break; } if (!matched_dst) return -1; if (matched_dst->peer_ref) - return error("dst ref %s receives from more than one src.", - matched_dst->name); + return error(_("dst ref %s receives from more than one src."), + matched_dst->name); else { matched_dst->peer_ref = allocated_src ? matched_src : @@ -1765,7 +1766,7 @@ int get_fetch_map(const struct ref *remote_refs, if (!starts_with((*rmp)->peer_ref->name, "refs/") || check_refname_format((*rmp)->peer_ref->name, 0)) { struct ref *ignore = *rmp; - error("* Ignoring funny ref '%s' locally", + error(_("* Ignoring funny ref '%s' locally"), (*rmp)->peer_ref->name); *rmp = (*rmp)->next; free(ignore->peer_ref); @@ -2133,7 +2134,7 @@ static int parse_push_cas_option(struct push_cas_option *cas, const char *arg, i else if (!colon[1]) oidclr(&entry->expect); else if (get_oid(colon + 1, &entry->expect)) - return error("cannot parse expected object name '%s'", colon + 1); + return error(_("cannot parse expected object name '%s'"), colon + 1); return 0; } -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 3/8] push: improve the error shown on unqualified <dst> push 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2018-10-26 23:07 ` [PATCH v3 2/8] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 2018-10-29 5:03 ` Junio C Hamano 2018-10-26 23:07 ` [PATCH v3 4/8] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Improve the error message added in f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23), which before this change looks like this: $ git push avar v2.19.0^{commit}:newbranch -n error: unable to push to unqualified destination: newbranch The destination refspec neither matches an existing ref on the remote nor begins with refs/, and we are unable to guess a prefix based on the source ref. error: failed to push some refs to 'git@github.com:avar/git.git' This message needed to be read very carefully to spot how to fix the error, i.e. to push to refs/heads/newbranch. Now the message will look like this instead: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - Looking for a ref that matches 'newbranch' on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a ref in "refs/{heads,tags}/". If so we add a corresponding refs/{heads,tags}/ prefix on the remote side. Neither worked, so we gave up. You must fully-qualify the ref. error: failed to push some refs to 'git@github.com:avar/git.git' This improvement is the result of on-list discussion in [1] and [2], as well as my own fixes / reformatting / phrasing on top. The suggestion by Jeff on-list was to make that second bullet point "Looking at the refname of the local source.". The version being added here is more verbose, but also more accurate. saying "local source" could refer to any ref in the local refstore, including something in refs/remotes/*. A later change will teach guess_ref() to infer a ref type from remote-tracking refs, so let's not confuse the two. While I'm at it, add a "TRANSLATORS" comment since the message has gotten more complex and it's not as clear what the two %s's refer to. 1. https://public-inbox.org/git/20181010205505.GB12949@sigill.intra.peff.net/ 2. https://public-inbox.org/git/xmqqbm81lb7c.fsf@gitster-ct.c.googlers.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/remote.c b/remote.c index 5cb3d00bfb..f4b438ff74 100644 --- a/remote.c +++ b/remote.c @@ -1049,12 +1049,22 @@ static int match_explicit(struct ref *src, struct ref *dst, matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - error(_("unable to push to unqualified destination: %s\n" - "The destination refspec neither matches an " - "existing ref on the remote nor\n" - "begins with refs/, and we are unable to " - "guess a prefix based on the source ref."), - dst_value); + /* + * TRANSLATORS: "matches '%s'%" is the <dst> + * part of "git push <remote> <src>:<dst>" + * push, and "being pushed ('%s')" is the + * <src>. + */ + error(_("The destination you provided is not a full refname (i.e.,\n" + "starting with \"refs/\"). We tried to guess what you meant by:\n" + "\n" + "- Looking for a ref that matches '%s' on the remote side.\n" + "- Checking if the <src> being pushed ('%s')\n" + " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" + " refs/{heads,tags}/ prefix on the remote side.\n" + "\n" + "Neither worked, so we gave up. You must fully-qualify the ref."), + dst_value, matched_src->name); } break; default: -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 3/8] push: improve the error shown on unqualified <dst> push 2018-10-26 23:07 ` [PATCH v3 3/8] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-10-29 5:03 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-10-29 5:03 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Improve the error message added in f8aae12034 ("push: allow > unqualified dest refspecs to DWIM", 2008-04-23), which before this > change looks like this: > > $ git push avar v2.19.0^{commit}:newbranch -n > error: unable to push to unqualified destination: newbranch > The destination refspec neither matches an existing ref on the remote nor > begins with refs/, and we are unable to guess a prefix based on the source ref. > error: failed to push some refs to 'git@github.com:avar/git.git' > > This message needed to be read very carefully to spot how to fix the > error, i.e. to push to refs/heads/newbranch. Now the message will look > like this instead: > > $ ./git-push avar v2.19.0^{commit}:newbranch -n > error: The destination you provided is not a full refname (i.e., > starting with "refs/"). We tried to guess what you meant by: > > - Looking for a ref that matches 'newbranch' on the remote side. > - Checking if the <src> being pushed ('v2.19.0^{commit}') > is a ref in "refs/{heads,tags}/". If so we add a > corresponding refs/{heads,tags}/ prefix on the remote side. If so, we would have added ..., but we couldn't, because <src> was not such a local ref. But is that a useful/actionable piece of information? The user said v2.19.0^{commit} because there was no such local ref the user could have used instead to allow the DWIM to work on the destination side. Perhaps it may be a good thing to remember for the next time, but it does not help the user at all while redoing this failed push. > Neither worked, so we gave up. You must fully-qualify the ref. > error: failed to push some refs to 'git@github.com:avar/git.git' I am not sure if this is an improvement, quite honestly. The only part that directly matters to the end user and is is more understandable than the original is "You must fully qualify the ref" (by the way, that dash is probably not what you want). As I already said, "if this were local ref we can guess the location, it would have worked better" is not relevant to the end user, so it is a better use of the extra bytes to explain what it is to "fully" qualify the ref, than telling what would have helped us make a better guess. Perhaps something along the lines of... 'newbranch' does not match any existing ref on the remote side. Please fully specify the destination refname starting from "refs/" (e.g. "v2.19.0^{commit}:refs/heads/newbranch" for creating a "newbranch" branch). ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 4/8] push: move unqualified refname error into a function 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2018-10-26 23:07 ` [PATCH v3 3/8] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 5/8] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason A follow-up change will extend this error message with the advice facility. Doing so would make the indentation too deeply nested for comfort. So let's split this into a helper function. There's no changes to the wording here. Just code moving & re-indentation, and re-flowing the "TRANSLATORS" comment. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/remote.c b/remote.c index f4b438ff74..c7a0b9c46f 100644 --- a/remote.c +++ b/remote.c @@ -1005,6 +1005,26 @@ static int match_explicit_lhs(struct ref *src, } } +static void show_push_unqualified_ref_name_error(const char *dst_value, + const char *matched_src_name) +{ + /* + * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push + * <remote> <src>:<dst>" push, and "being pushed ('%s')" is + * the <src>. + */ + error(_("The destination you provided is not a full refname (i.e.,\n" + "starting with \"refs/\"). We tried to guess what you meant by:\n" + "\n" + "- Looking for a ref that matches '%s' on the remote side.\n" + "- Checking if the <src> being pushed ('%s')\n" + " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" + " refs/{heads,tags}/ prefix on the remote side.\n" + "\n" + "Neither worked, so we gave up. You must fully-qualify the ref."), + dst_value, matched_src_name); +} + static int match_explicit(struct ref *src, struct ref *dst, struct ref ***dst_tail, struct refspec_item *rs) @@ -1049,22 +1069,8 @@ static int match_explicit(struct ref *src, struct ref *dst, matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - /* - * TRANSLATORS: "matches '%s'%" is the <dst> - * part of "git push <remote> <src>:<dst>" - * push, and "being pushed ('%s')" is the - * <src>. - */ - error(_("The destination you provided is not a full refname (i.e.,\n" - "starting with \"refs/\"). We tried to guess what you meant by:\n" - "\n" - "- Looking for a ref that matches '%s' on the remote side.\n" - "- Checking if the <src> being pushed ('%s')\n" - " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" - " refs/{heads,tags}/ prefix on the remote side.\n" - "\n" - "Neither worked, so we gave up. You must fully-qualify the ref."), - dst_value, matched_src->name); + show_push_unqualified_ref_name_error(dst_value, + matched_src->name); } break; default: -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 5/8] push: add an advice on unqualified <dst> push 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2018-10-26 23:07 ` [PATCH v3 4/8] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 2018-10-29 5:14 ` Junio C Hamano 2018-10-26 23:07 ` [PATCH v3 6/8] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Add an advice to the recently improved error message added in f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23). Now with advice.pushUnqualifiedRefName=true (on by default) we show a hint about how to proceed: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - Looking for a ref that matches 'newbranch' on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a ref in "refs/{heads,tags}/". If so we add a corresponding refs/{heads,tags}/ prefix on the remote side. Neither worked, so we gave up. You must fully-qualify the ref. hint: The <src> part of the refspec is a commit object. hint: Did you mean to create a new branch by pushing to hint: 'v2.19.0^{commit}:refs/heads/newbranch'? error: failed to push some refs to 'git@github.com:avar/git.git' When trying to push a tag, tree or a blob we suggest that perhaps the user meant to push them to refs/tags/ instead. The if/else duplication for all of OBJ_{COMMIT,TAG,TREE,BLOB} is unfortunate, but is required to correctly mark the messages for translation. See the discussion in <87r2gxebsi.fsf@evledraar.gmail.com> about that. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/config.txt | 7 +++++++ advice.c | 2 ++ advice.h | 1 + remote.c | 37 +++++++++++++++++++++++++++++++++++++ t/t5505-remote.sh | 25 +++++++++++++++++++++++++ 5 files changed, 72 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 552827935a..8ca465702e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -320,6 +320,13 @@ advice.*:: tries to overwrite a remote ref that points at an object that is not a commit-ish, or make the remote ref point at an object that is not a commit-ish. + pushUnqualifiedRefname:: + Shown when linkgit:git-push[1] gives up trying to + guess based on the source and destination refs what + remote ref namespace the source belongs in, but where + we can still suggest that the user push to either + refs/heads/* or refs/tags/* based on the type of the + source object. statusHints:: Show directions on how to proceed from the current state in the output of linkgit:git-status[1], in diff --git a/advice.c b/advice.c index 3561cd64e9..3089a4ca65 100644 --- a/advice.c +++ b/advice.c @@ -9,6 +9,7 @@ int advice_push_non_ff_matching = 1; int advice_push_already_exists = 1; int advice_push_fetch_first = 1; int advice_push_needs_force = 1; +int advice_push_unqualified_ref_name = 1; int advice_status_hints = 1; int advice_status_u_option = 1; int advice_commit_before_merge = 1; @@ -62,6 +63,7 @@ static struct { { "pushAlreadyExists", &advice_push_already_exists }, { "pushFetchFirst", &advice_push_fetch_first }, { "pushNeedsForce", &advice_push_needs_force }, + { "pushUnqualifiedRefName", &advice_push_unqualified_ref_name }, { "statusHints", &advice_status_hints }, { "statusUoption", &advice_status_u_option }, { "commitBeforeMerge", &advice_commit_before_merge }, diff --git a/advice.h b/advice.h index ab24df0fd0..9a2f8b5226 100644 --- a/advice.h +++ b/advice.h @@ -9,6 +9,7 @@ extern int advice_push_non_ff_matching; extern int advice_push_already_exists; extern int advice_push_fetch_first; extern int advice_push_needs_force; +extern int advice_push_unqualified_ref_name; extern int advice_status_hints; extern int advice_status_u_option; extern int advice_commit_before_merge; diff --git a/remote.c b/remote.c index c7a0b9c46f..93f802509d 100644 --- a/remote.c +++ b/remote.c @@ -13,6 +13,7 @@ #include "mergesort.h" #include "argv-array.h" #include "commit-reach.h" +#include "advice.h" enum map_direction { FROM_SRC, FROM_DST }; @@ -1008,6 +1009,9 @@ static int match_explicit_lhs(struct ref *src, static void show_push_unqualified_ref_name_error(const char *dst_value, const char *matched_src_name) { + struct object_id oid; + enum object_type type; + /* * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push * <remote> <src>:<dst>" push, and "being pushed ('%s')" is @@ -1023,6 +1027,39 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, "\n" "Neither worked, so we gave up. You must fully-qualify the ref."), dst_value, matched_src_name); + + if (!advice_push_unqualified_ref_name) + return; + + if (get_oid(matched_src_name, &oid)) + BUG("'%s' is not a valid object, " + "match_explicit_lhs() should catch this!", + matched_src_name); + type = oid_object_info(the_repository, &oid, NULL); + if (type == OBJ_COMMIT) { + advise(_("The <src> part of the refspec is a commit object.\n" + "Did you mean to create a new branch by pushing to\n" + "'%s:refs/heads/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_TAG) { + advise(_("The <src> part of the refspec is a tag object.\n" + "Did you mean to create a new tag by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_TREE) { + advise(_("The <src> part of the refspec is a tree object.\n" + "Did you mean to tag a new tree by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_BLOB) { + advise(_("The <src> part of the refspec is a blob object.\n" + "Did you mean to tag a new blob by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else { + BUG("'%s' should be commit/tag/tree/blob, is '%d'", + matched_src_name, type); + } } static int match_explicit(struct ref *src, struct ref *dst, diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index d2a2cdd453..2e58721f98 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1222,4 +1222,29 @@ test_expect_success 'add remote matching the "insteadOf" URL' ' git remote add backup xyz@example.com ' +test_expect_success 'unqualified <dst> refspec DWIM and advice' ' + test_when_finished "(cd test && git tag -d some-tag)" && + ( + cd test && + git tag -a -m "Some tag" some-tag master && + for type in commit tag tree blob + do + if test "$type" = "blob" + then + oid=$(git rev-parse some-tag:file) + else + oid=$(git rev-parse some-tag^{$type}) + fi && + test_must_fail git push origin $oid:dst 2>err && + test_i18ngrep "error: The destination you" err && + test_i18ngrep "hint: Did you mean" err && + test_must_fail git -c advice.pushUnqualifiedRefName=false \ + push origin $oid:dst 2>err && + test_i18ngrep "error: The destination you" err && + test_i18ngrep ! "hint: Did you mean" err + done + ) +' + + test_done -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 5/8] push: add an advice on unqualified <dst> push 2018-10-26 23:07 ` [PATCH v3 5/8] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-10-29 5:14 ` Junio C Hamano 2018-11-02 6:47 ` Jeff King 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-10-29 5:14 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > + if (!advice_push_unqualified_ref_name) > + return; > + > + if (get_oid(matched_src_name, &oid)) > + BUG("'%s' is not a valid object, " > + "match_explicit_lhs() should catch this!", > + matched_src_name); > + type = oid_object_info(the_repository, &oid, NULL); > + if (type == OBJ_COMMIT) { > + advise(_("The <src> part of the refspec is a commit object.\n" > + "Did you mean to create a new branch by pushing to\n" > + "'%s:refs/heads/%s'?"), > + matched_src_name, dst_value); Good, except that "git push $there notes/commits^0:newnotes" may not want to become a branch and neither may "git push $there stash:wip", I think it is a reasonable piece of advice we'd give by default. I do not think it is worth the effort of inspecting the tree of the commit object to special case notes and stash ;-) > + } else if (type == OBJ_TAG) { > + advise(_("The <src> part of the refspec is a tag object.\n" > + "Did you mean to create a new tag by pushing to\n" > + "'%s:refs/tags/%s'?"), > + matched_src_name, dst_value); Good. > + } else if (type == OBJ_TREE) { > + advise(_("The <src> part of the refspec is a tree object.\n" > + "Did you mean to tag a new tree by pushing to\n" > + "'%s:refs/tags/%s'?"), > + matched_src_name, dst_value); > + } else if (type == OBJ_BLOB) { > + advise(_("The <src> part of the refspec is a blob object.\n" > + "Did you mean to tag a new blob by pushing to\n" > + "'%s:refs/tags/%s'?"), > + matched_src_name, dst_value); These two are questionable, but assuming that heads and tags are the only two hiearchies people would push into, they are acceptable choices. > + } else { > + BUG("'%s' should be commit/tag/tree/blob, is '%d'", > + matched_src_name, type); > + } > } > > static int match_explicit(struct ref *src, struct ref *dst, > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh > index d2a2cdd453..2e58721f98 100755 > --- a/t/t5505-remote.sh > +++ b/t/t5505-remote.sh > @@ -1222,4 +1222,29 @@ test_expect_success 'add remote matching the "insteadOf" URL' ' > git remote add backup xyz@example.com > ' > > +test_expect_success 'unqualified <dst> refspec DWIM and advice' ' > + test_when_finished "(cd test && git tag -d some-tag)" && > + ( > + cd test && > + git tag -a -m "Some tag" some-tag master && > + for type in commit tag tree blob > + do > + if test "$type" = "blob" > + then > + oid=$(git rev-parse some-tag:file) > + else > + oid=$(git rev-parse some-tag^{$type}) > + fi && > + test_must_fail git push origin $oid:dst 2>err && > + test_i18ngrep "error: The destination you" err && > + test_i18ngrep "hint: Did you mean" err && > + test_must_fail git -c advice.pushUnqualifiedRefName=false \ > + push origin $oid:dst 2>err && > + test_i18ngrep "error: The destination you" err && > + test_i18ngrep ! "hint: Did you mean" err Any failure in the &&-chain (or the last grep) would not terminate the for loop, so for the purpose of determining the success of this test_expect_success, the last "blob" iteration is the only thing that matters. Which is probably not what you want. If testing all of these is important, then you can do this: ( exit_with=true && for type in ... do ... many ... && ... many ... && ... tests ... || exit_with=false done $exit_with ) or just say "|| exit" and leave the loop (and the subprocess running it) early on the first failure. > + done > + ) > +' > + > + > test_done ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 5/8] push: add an advice on unqualified <dst> push 2018-10-29 5:14 ` Junio C Hamano @ 2018-11-02 6:47 ` Jeff King 0 siblings, 0 replies; 53+ messages in thread From: Jeff King @ 2018-11-02 6:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git, Stefan Beller On Mon, Oct 29, 2018 at 02:14:02PM +0900, Junio C Hamano wrote: > Any failure in the &&-chain (or the last grep) would not terminate > the for loop, so for the purpose of determining the success of this > test_expect_success, the last "blob" iteration is the only thing > that matters. > > Which is probably not what you want. If testing all of these is > important, then you can do this: > > ( > exit_with=true && > for type in ... > do > ... many ... && > ... many ... && > ... tests ... || > exit_with=false > done > $exit_with > ) > > or just say "|| exit" and leave the loop (and the subprocess running > it) early on the first failure. You can skip the subshell and just "|| return 1" from the chain inside the loop. We run the test snippets inside an extra layer of function-call exactly to allow that. -Peff ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 6/8] push: test that <src> doesn't DWYM if <dst> is unqualified 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason ` (5 preceding siblings ...) 2018-10-26 23:07 ` [PATCH v3 5/8] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 2018-10-29 5:19 ` Junio C Hamano 2018-10-26 23:07 ` [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 8/8] push doc: document the DWYM behavior pushing to unqualified <dst> Ævar Arnfjörð Bjarmason 8 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Add a test asserting that "git push origin <src>:<dst>" where <src> is a branch, tag, tree or blob in refs/remotes/* doesn't DWYM when <dst> is unqualified. This has never worked, but there's been no test for this behavior. See f88395ac23 ("Renaming push.", 2005-08-03), bb9fca80ce ("git-push: Update description of refspecs and add examples", 2007-06-09) and f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23) which are most relevant commits that have changed or documented the behavior of this feature in the past. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5505-remote.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 2e58721f98..979a13b415 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1246,5 +1246,33 @@ test_expect_success 'unqualified <dst> refspec DWIM and advice' ' ) ' +test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and advice' ' + ( + cd two && + git tag -a -m "Some tag" some-tag master && + git update-ref refs/trees/my-head-tree HEAD^{tree} && + git update-ref refs/blobs/my-file-blob HEAD:file + ) && + ( + cd test && + git config --add remote.two.fetch "+refs/tags/*:refs/remotes/two-tags/*" && + git config --add remote.two.fetch "+refs/trees/*:refs/remotes/two-trees/*" && + git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/two-blobs/*" && + git fetch --no-tags two && + + test_must_fail git push origin refs/remotes/two/another:dst 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/two-tags/some-tag:dst-tag 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/two-trees/my-head-tree:dst-tree 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/two-blobs/my-file-blob:dst-blob 2>err && + test_i18ngrep "error: The destination you" err + ) +' + test_done -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 6/8] push: test that <src> doesn't DWYM if <dst> is unqualified 2018-10-26 23:07 ` [PATCH v3 6/8] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason @ 2018-10-29 5:19 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-10-29 5:19 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Add a test asserting that "git push origin <src>:<dst>" where <src> is > a branch, tag, tree or blob in refs/remotes/* doesn't DWYM when <dst> > is unqualified. This has never worked, but there's been no test for > this behavior. "has never worked" sounded as if there is a breakage, but that is not what meant here. We didn't DWIM overly agressively (which would have led us to possibly push into a wrong place) and correctly rejected the push instead, right? > +test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and advice' ' > + ( > + cd two && > + git tag -a -m "Some tag" some-tag master && > + git update-ref refs/trees/my-head-tree HEAD^{tree} && > + git update-ref refs/blobs/my-file-blob HEAD:file > + ) && > + ( > + cd test && > + git config --add remote.two.fetch "+refs/tags/*:refs/remotes/two-tags/*" && > + git config --add remote.two.fetch "+refs/trees/*:refs/remotes/two-trees/*" && > + git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/two-blobs/*" && > + git fetch --no-tags two && > + > + test_must_fail git push origin refs/remotes/two/another:dst 2>err && > + test_i18ngrep "error: The destination you" err && > + > + test_must_fail git push origin refs/remotes/two-tags/some-tag:dst-tag 2>err && This made me go "Huh? some-tag is one tag; what is the other tag in two-tags/ hierarchy?" I think you meant by "two-tags" a hierarchy to store tags taken from the remote "two"; calling it "tags-from-two" may have avoided such a confusion. > + test_i18ngrep "error: The destination you" err && > + > + test_must_fail git push origin refs/remotes/two-trees/my-head-tree:dst-tree 2>err && > + test_i18ngrep "error: The destination you" err && > + > + test_must_fail git push origin refs/remotes/two-blobs/my-file-blob:dst-blob 2>err && > + test_i18ngrep "error: The destination you" err > + ) > +' > + > > test_done ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason ` (6 preceding siblings ...) 2018-10-26 23:07 ` [PATCH v3 6/8] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 2018-10-29 5:24 ` Junio C Hamano 2018-10-29 7:06 ` Junio C Hamano 2018-10-26 23:07 ` [PATCH v3 8/8] push doc: document the DWYM behavior pushing to unqualified <dst> Ævar Arnfjörð Bjarmason 8 siblings, 2 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Add DWYM support for pushing a ref in refs/remotes/* when the <dst> ref is unqualified. Now instead of erroring out we support e.g.: $ ./git-push avar refs/remotes/origin/master:upstream-master -n To github.com:avar/git.git * [new branch] origin/master -> upstream-master Before this we wouldn't know what do do with refs/remotes/origin/master, now we'll look it up and discover that it's a commit (or tag) and add a refs/{heads,tags}/ prefix to <dst> as appropriate. The error message emitted when we still don't know what to do has been amended to note that this is something we tried: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - Looking for a ref that matches 'newbranch' on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a ref in "refs/{heads,tags}/". If so we add a corresponding refs/{heads,tags}/ prefix on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a commit or tag in "refs/remotes/*". Then we infer a corresponding refs/{heads,tags} on the remote side. None of those worked, so we gave up. You must fully-qualify the ref. hint: The <src> part of the refspec is a commit object. hint: Did you mean to create a new branch by pushing to hint: 'v2.19.0^{commit}:refs/heads/newbranch'? I'm bending over backwards and assuming that someone might have hacked in remote tracking tags (see [1] for a discussion of how that can be done), but punting on any tree or blob objects found under refs/remotes/*. This is the first use of the %N$<fmt> style of printf format in the *.[ch] files in our codebase. It's supported by POSIX[2] and there's existing uses for it in po/*.po files, so hopefully it won't cause any trouble. It's more obvious for translators than to have a 3rd argument to the function identical to the 2nd, by re-using the 2nd it's clear that we're continuing to talk about the <src> part of the refspec. 1. https://public-inbox.org/git/87zi1jxjqn.fsf@evledraar.gmail.com/ 2. http://pubs.opengroup.org/onlinepubs/7908799/xsh/fprintf.html Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 20 +++++++++++++++++++- t/t5505-remote.sh | 14 +++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/remote.c b/remote.c index 93f802509d..c243e3d89e 100644 --- a/remote.c +++ b/remote.c @@ -973,6 +973,21 @@ static char *guess_ref(const char *name, struct ref *peer) strbuf_addstr(&buf, "refs/heads/"); } else if (starts_with(r, "refs/tags/")) { strbuf_addstr(&buf, "refs/tags/"); + } else if (starts_with(r, "refs/remotes/")) { + struct object_id oid; + enum object_type type; + + if (get_oid(peer->name, &oid)) + BUG("'%s' is not a valid object, " + "match_explicit_lhs() should catch this!", + peer->name); + type = oid_object_info(the_repository, &oid, NULL); + if (type == OBJ_COMMIT) + strbuf_addstr(&buf, "refs/heads/"); + else if (type == OBJ_TAG) + strbuf_addstr(&buf, "refs/tags/"); + else + return NULL; } else { return NULL; } @@ -1024,8 +1039,11 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, "- Checking if the <src> being pushed ('%s')\n" " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" " refs/{heads,tags}/ prefix on the remote side.\n" + "- Checking if the <src> being pushed ('%2$s')\n" + " is a commit or tag in \"refs/remotes/*\". Then we infer a\n" + " corresponding refs/{heads,tags} on the remote side.\n" "\n" - "Neither worked, so we gave up. You must fully-qualify the ref."), + "None of those worked, so we gave up. You must fully-qualify the ref."), dst_value, matched_src_name); if (!advice_push_unqualified_ref_name) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 979a13b415..a6337b50e4 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1260,11 +1260,15 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/two-blobs/*" && git fetch --no-tags two && - test_must_fail git push origin refs/remotes/two/another:dst 2>err && - test_i18ngrep "error: The destination you" err && - - test_must_fail git push origin refs/remotes/two-tags/some-tag:dst-tag 2>err && - test_i18ngrep "error: The destination you" err && + echo commit >expected && + git push origin refs/remotes/two/another:dst && + git -C ../one cat-file -t refs/heads/dst >actual && + test_cmp expected actual && + + echo tag >expected && + git push origin refs/remotes/two-tags/some-tag:dst-tag && + git -C ../one cat-file -t refs/tags/dst-tag >actual && + test_cmp expected actual && test_must_fail git push origin refs/remotes/two-trees/my-head-tree:dst-tree 2>err && test_i18ngrep "error: The destination you" err && -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-10-26 23:07 ` [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" Ævar Arnfjörð Bjarmason @ 2018-10-29 5:24 ` Junio C Hamano 2018-10-29 8:13 ` Ævar Arnfjörð Bjarmason 2018-10-29 7:06 ` Junio C Hamano 1 sibling, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-10-29 5:24 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Add DWYM support for pushing a ref in refs/remotes/* when the <dst> I think most people call it do-what-*I*-mean, not do-what-you-mean. > ref is unqualified. Now instead of erroring out we support e.g.: > > $ ./git-push avar refs/remotes/origin/master:upstream-master -n > To github.com:avar/git.git > * [new branch] origin/master -> upstream-master > > Before this we wouldn't know what do do with > refs/remotes/origin/master, now we'll look it up and discover that > it's a commit (or tag) and add a refs/{heads,tags}/ prefix to <dst> as > appropriate. I am not sure if this is a good change, as I already hinted earlier. If I were doing "git push" to any of my publishing places, I would be irritated if "refs/remotes/ko/master:something" created a local "something" branch at the desitnation, instead of erroring out. On the other hand, I do not think I mind all that much if a src that is a tag object to automatically go to refs/tags/ (having a tag object in refs/remotes/** is rare enough to matter in the first place). ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-10-29 5:24 ` Junio C Hamano @ 2018-10-29 8:13 ` Ævar Arnfjörð Bjarmason 2018-11-01 4:18 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-29 8:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Stefan Beller On Mon, Oct 29 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Add DWYM support for pushing a ref in refs/remotes/* when the <dst> > > I think most people call it do-what-*I*-mean, not do-what-you-mean. FWIW I picked this up from the perl list where both are used depending on context. I.e. DW[IY]M depending on if the sentence is one where you'd describe things from a first or second-person perspective "I implemented this to DWIM, and it'll DWYM if you invoke it as...". Also "dwimerry"[1]. >> ref is unqualified. Now instead of erroring out we support e.g.: >> >> $ ./git-push avar refs/remotes/origin/master:upstream-master -n >> To github.com:avar/git.git >> * [new branch] origin/master -> upstream-master >> >> Before this we wouldn't know what do do with >> refs/remotes/origin/master, now we'll look it up and discover that >> it's a commit (or tag) and add a refs/{heads,tags}/ prefix to <dst> as >> appropriate. > > I am not sure if this is a good change, as I already hinted earlier. > If I were doing "git push" to any of my publishing places, I would > be irritated if "refs/remotes/ko/master:something" created a local > "something" branch at the desitnation, instead of erroring out. > > On the other hand, I do not think I mind all that much if a src that > is a tag object to automatically go to refs/tags/ (having a tag > object in refs/remotes/** is rare enough to matter in the first > place). Yeah maybe this is going too far. I don't think so, but happy to me challenged on that point :) I don't think so because the only reason I've ever needed this is because I deleted some branch accidentally and am using a push from "remotes/*" to bring it back. I.e. I'll always want branch-for-branch, not to push that as a tag. Of course it isn't actually a "branch", but just a commit object (depending on the refspec configuration), so we can't quite make that hard assumption. But I figured screwing with how refs/remotes/* works by manually adding new refspecs is such an advanced feature that the people doing it are probably all here on-list, and would be the sort of users advanced enough to either know the semantics or try this with -n. Whereas the vast majority of users won't ever screw with it, and if they ever push from remotes/* to another remote almost definitely want a new branch under the new name. 1. https://www.urbandictionary.com/define.php?term=DWYM ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-10-29 8:13 ` Ævar Arnfjörð Bjarmason @ 2018-11-01 4:18 ` Junio C Hamano 2018-11-05 11:31 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-11-01 4:18 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> On the other hand, I do not think I mind all that much if a src that >> is a tag object to automatically go to refs/tags/ (having a tag >> object in refs/remotes/** is rare enough to matter in the first >> place). > > Yeah maybe this is going too far. I don't think so, but happy to me > challenged on that point :) > > I don't think so because the only reason I've ever needed this is > because I deleted some branch accidentally and am using a push from > "remotes/*" to bring it back. I.e. I'll always want branch-for-branch, > not to push that as a tag. Oh, I didn't consider pushing it out as a tag, but now you bring it up, I think that it also would make sense in a workflow to tell your colleages to look at (sort of like how people use pastebin---"look here, this commit has the kind of change I have in mind in this discussion") some random commit and the commit happens to be sitting at a tip of a remote-trackig branch. Instead of pushing it out as a branch or a remote-tracking branch, which has strong connotations of inviting others to build on top, pushing it out as a tag would make more sense in that context. And as I mentioned already, I think it would equally likely, if not more likely, for people like me to push remotes/** out as a remote-tracking branch (rather than a local branch) of the repository I'm pushing into. So I tend to agree that this is going too far. If the original motivating example was not an ingredient of everyday workflow, but was an one-off "recovery", I'd rather see people forced to be more careful by requiring "push origin/frotz:refs/heads/frotz" rather than incorrectly DWIDNM "push origin/frotz:frotz" and ending up with creating refs/tags/frotz or refs/remotes/origin/frotz, which also are plausible choices depending on what the user is trying to recover from, which the sending end would not know (the side on which the accidental loss of a ref happened earlier is on the remote repository that would be receiving this push, and it _might_ know). As to the entirety of the series, - I do not think this step 7, and its documentation in step 8, are particularly a good idea, in their current shape. Pushing tag objects to refs/tags/ is probably a good idea, but pushing a commit as local branch heads are necessarily not. - Step 6 is probably a good documentation on the cases in which we make and do not make guess on the unqualified push destination. - Step 5 and earlier looked like good changes. If we were to salvage some parts of step 7 (and step 8), we'd probably need fb7c2268 ("SQUASH???", 2018-10-29) to number all the placeholders in the printf format string. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-11-01 4:18 ` Junio C Hamano @ 2018-11-05 11:31 ` Ævar Arnfjörð Bjarmason 2018-11-05 12:29 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-11-05 11:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Stefan Beller I'll re-roll this. Hopefully sooner than later. I'll leave out the later part of this series as it's more controversial and we can discuss that later on its own. Meanwhile just some replies to this (while I remember): > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> On the other hand, I do not think I mind all that much if a src that >>> is a tag object to automatically go to refs/tags/ (having a tag >>> object in refs/remotes/** is rare enough to matter in the first >>> place). >> >> Yeah maybe this is going too far. I don't think so, but happy to me >> challenged on that point :) >> >> I don't think so because the only reason I've ever needed this is >> because I deleted some branch accidentally and am using a push from >> "remotes/*" to bring it back. I.e. I'll always want branch-for-branch, >> not to push that as a tag. > > Oh, I didn't consider pushing it out as a tag, but now you bring it > up, I think that it also would make sense in a workflow to tell your > colleages to look at (sort of like how people use pastebin---"look > here, this commit has the kind of change I have in mind in this > discussion") some random commit and the commit happens to be sitting > at a tip of a remote-trackig branch. Instead of pushing it out as a > branch or a remote-tracking branch, which has strong connotations of > inviting others to build on top, pushing it out as a tag would make > more sense in that context. > > And as I mentioned already, I think it would equally likely, if not > more likely, for people like me to push remotes/** out as a > remote-tracking branch (rather than a local branch) of the > repository I'm pushing into. > > So I tend to agree that this is going too far. If the original > motivating example was not an ingredient of everyday workflow, but > was an one-off "recovery", I'd rather see people forced to be more > careful by requiring "push origin/frotz:refs/heads/frotz" rather > than incorrectly DWIDNM "push origin/frotz:frotz" and ending up with > creating refs/tags/frotz or refs/remotes/origin/frotz, which also > are plausible choices depending on what the user is trying to > recover from, which the sending end would not know (the side on > which the accidental loss of a ref happened earlier is on the remote > repository that would be receiving this push, and it _might_ know). Yeah this example was bad, but now since I've written it I've become more convinced that it's the right way to go, just that I didn't explain it well. E.g. just now I did: git push avar avar/some-branch:some-branch-wip git push avar HEAD -f # I was on 'some-branch' Because I'd regretted taking the "some-branch" name with some WIP code, but didn't want to lose the work, so I wanted to swap. It's also arbitrary and contrary to the distributed nature of git that we're treating branches from other repos differently than the ones from ours. After all sometimes "other" is just the repo on my laptop or server. I shouldn't need to jump through hoops to re-push stuff from my "other" repo anymore than from the local repo. Yes refs/remotes/* isn't guaranteed to be "other repo's branches" in the same way our local refs/heads/* is, and this series bends over backwards to check if someone's (ab)used it for that, but I think for the purposes of the default UI we should treat it as "branches from other remotes" if we find commit objects there. So I think the *only* thing we should be checking for the purposes of this DWYM feature is what local type of object (branch or tag) we find on the LHS of the refspec. And if we're sure of its type push to refs/{heads,tags}/* as appropriate. I don't think it makes any sense as you suggest to move away from "guess based on existing type" to breaking the dwimmery because we found the branch in some other place. The user's pushing "newref" from an existing branch, let's just assume the new thing is a branch, whether the source is local or not. > As to the entirety of the series, > > - I do not think this step 7, and its documentation in step 8, are > particularly a good idea, in their current shape. Pushing tag > objects to refs/tags/ is probably a good idea, but pushing a > commit as local branch heads are necessarily not. > > - Step 6 is probably a good documentation on the cases in which we > make and do not make guess on the unqualified push destination. > > - Step 5 and earlier looked like good changes. > > If we were to salvage some parts of step 7 (and step 8), we'd > probably need fb7c2268 ("SQUASH???", 2018-10-29) to number all the > placeholders in the printf format string. Thanks. As noted will try to re-roll this soon. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-11-05 11:31 ` Ævar Arnfjörð Bjarmason @ 2018-11-05 12:29 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-11-05 12:29 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > After all sometimes "other" is just the repo on my laptop or server. I > shouldn't need to jump through hoops to re-push stuff from my "other" > repo anymore than from the local repo. > > Yes refs/remotes/* isn't guaranteed to be "other repo's branches" in the > same way our local refs/heads/* is, and this series bends over backwards > to check if someone's (ab)used it for that, but I think for the purposes > of the default UI we should treat it as "branches from other remotes" if > we find commit objects there. I do not think there is *any* disagreement between us that refs/remotes/* is where somebody else's branches are stored. The fundamental difference between us is what this "push" is doing. You are limiting yourself to a single use case where you push to publish to the remote repository, as if the work on somebody else's commit were exactly like your own, hence you believe the push should go to refs/heads/* of the receiving repository. I am also allowing an additional use case I often have to use, where I emulate a fetch done at the receiving repository by pushing into it. I have refs/remotes/* by fetching somebody else's branches locally, and instead of doing the same fetch over there by logging into it and doing "git fetch" interactively, I push refs/remotes/* I happen to have locally into refs/remotes/* there. And I happen to think both are equally valid workflows, and there is no strong reason to think everybody would intuitively expect a push of my local refs/remotes/* will go to refs/heads/* of the receiving repository like you do. I'd rather want to make sure we do not make wrong guesses and force the user to "recover". > So I think the *only* thing we should be checking for the purposes of > this DWYM feature is what local type of object (branch or tag) we find > on the LHS of the refspec. And if we're sure of its type push to > refs/{heads,tags}/* as appropriate. That is insufficient as long as you do not consider refs/remotes/* as one of the equally valid possibilities that sits next to refs/heads and res/tags/. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-10-26 23:07 ` [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" Ævar Arnfjörð Bjarmason 2018-10-29 5:24 ` Junio C Hamano @ 2018-10-29 7:06 ` Junio C Hamano 2018-10-29 7:57 ` Junio C Hamano 2018-10-29 8:05 ` Ævar Arnfjörð Bjarmason 1 sibling, 2 replies; 53+ messages in thread From: Junio C Hamano @ 2018-10-29 7:06 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > This is the first use of the %N$<fmt> style of printf format in > the *.[ch] files in our codebase. It's supported by POSIX[2] and > there's existing uses for it in po/*.po files,... For now, I'll eject this from 'pu', as I had spent way too much time trying to make it and other topics work there. CC remote.o remote.c: In function 'show_push_unqualified_ref_name_error': remote.c:1035:2: error: $ operand number used after format without operand number [-Werror=format=] error(_("The destination you provided is not a full refname (i.e.,\n" ^~~~~ cc1: all warnings being treated as errors Makefile:2323: recipe for target 'remote.o' failed make: *** [remote.o] Error 1 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-10-29 7:06 ` Junio C Hamano @ 2018-10-29 7:57 ` Junio C Hamano 2018-10-29 8:05 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-10-29 7:57 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Stefan Beller Junio C Hamano <gitster@pobox.com> writes: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> This is the first use of the %N$<fmt> style of printf format in >> the *.[ch] files in our codebase. It's supported by POSIX[2] and >> there's existing uses for it in po/*.po files,... > > For now, I'll eject this from 'pu', as I had spent way too much time > trying to make it and other topics work there. > > CC remote.o > remote.c: In function 'show_push_unqualified_ref_name_error': > remote.c:1035:2: error: $ operand number used after format without operand number [-Werror=format=] > error(_("The destination you provided is not a full refname (i.e.,\n" > ^~~~~ > cc1: all warnings being treated as errors > Makefile:2323: recipe for target 'remote.o' failed > make: *** [remote.o] Error 1 I'll redo 'pu' with the following fix-up on top. remote.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/remote.c b/remote.c index c243e3d89e..1927c4b376 100644 --- a/remote.c +++ b/remote.c @@ -1035,8 +1035,8 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, error(_("The destination you provided is not a full refname (i.e.,\n" "starting with \"refs/\"). We tried to guess what you meant by:\n" "\n" - "- Looking for a ref that matches '%s' on the remote side.\n" - "- Checking if the <src> being pushed ('%s')\n" + "- Looking for a ref that matches '%1$s' on the remote side.\n" + "- Checking if the <src> being pushed ('%2$s')\n" " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" " refs/{heads,tags}/ prefix on the remote side.\n" "- Checking if the <src> being pushed ('%2$s')\n" -- 2.19.1-593-gc670b1f876 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-10-29 7:06 ` Junio C Hamano 2018-10-29 7:57 ` Junio C Hamano @ 2018-10-29 8:05 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-29 8:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Stefan Beller On Mon, Oct 29 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> This is the first use of the %N$<fmt> style of printf format in >> the *.[ch] files in our codebase. It's supported by POSIX[2] and >> there's existing uses for it in po/*.po files,... > > For now, I'll eject this from 'pu', as I had spent way too much time > trying to make it and other topics work there. I was compiling with DEVELOPER=1 but as it turns out: CFLAGS="-O0" DEVELOPER=1 Wasn't doing what I thought, i.e. we just take 'CFLAGS' from the command-line and don't add any of the DEVELOPER #leftoverbits to it. Will fix this and other issues raised. > CC remote.o > remote.c: In function 'show_push_unqualified_ref_name_error': > remote.c:1035:2: error: $ operand number used after format without operand number [-Werror=format=] > error(_("The destination you provided is not a full refname (i.e.,\n" > ^~~~~ > cc1: all warnings being treated as errors > Makefile:2323: recipe for target 'remote.o' failed > make: *** [remote.o] Error 1 Will fix this and other issues raised. FWIW clang gives a much better error about the actual issue: remote.c:1042:46: error: cannot mix positional and non-positional arguments in format string [-Werror,-Wformat] "- Checking if the <src> being pushed ('%2$s')\n" I.e. this on top fixes it: - "- Looking for a ref that matches '%s' on the remote side.\n" - "- Checking if the <src> being pushed ('%s')\n" + "- Looking for a ref that matches '%1$s' on the remote side.\n" + "- Checking if the <src> being pushed ('%2$s')\n" Maybe this whole thing isn't worth it and I should just do: @@ -1042 +1042 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, - "- Checking if the <src> being pushed ('%2$s')\n" + "- Checking if the <src> being pushed ('%s')\n" @@ -1047 +1047 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, - dst_value, matched_src_name); + dst_value, matched_src_name, matched_src_name); But I'm leaning on the side of keeping it for the self-documentation aspect of "this is a repeated parameter". Your objections to this whole thing being a stupid idea non-withstanding. ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 8/8] push doc: document the DWYM behavior pushing to unqualified <dst> 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason ` (7 preceding siblings ...) 2018-10-26 23:07 ` [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 ` Ævar Arnfjörð Bjarmason 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 23:07 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Stefan Beller, Ævar Arnfjörð Bjarmason Document the DWYM behavior that kicks in when pushing to an unqualified <dst> reference. This behavior was added in f88395ac23 ("Renaming push.", 2005-08-03) and f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23), and somewhat documented in bb9fca80ce ("git-push: Update description of refspecs and add examples", 2007-06-09), but has never been fully documented. The closest we got to having documented it was the description in the commit message for f8aae12034, which I've borrowed from in writing this documentation. Let's also refer to this new documentation from the existing documentation we had (added in bb9fca80ce). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/git-push.txt | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index a5fc54aeab..fb95c2e395 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -73,6 +73,30 @@ be omitted--such a push will update a ref that `<src>` normally updates without any `<refspec>` on the command line. Otherwise, missing `:<dst>` means to update the same ref as the `<src>`. + +If <dst> doesn't start with `refs/` (e.g. `refs/heads/master`) we will +try to infer where in `refs/*` on the destination <repository> it +belongs based on the the type of <src> being pushed and whether <dst> +is ambiguous. ++ +-- +* If <dst> unambiguously refers to a ref on the <repository> remote, + then push to that ref. + +* If <src> resolves to a ref starting with refs/heads/ or refs/tags/, + then prepend that to <dst>. + +* If <src> starts with refs/remotes/ check if that reference refers to + a commit or tag, then refs/heads/ or refs/tags/ to <dst> as + appropriate. + +* Other ambiguity resolutions might be added in the future, but for + now any other cases will error out with an error indicating what we + tried, and depending on the `advice.pushUnqualifiedRefname` + configuration (see linkgit:git-config[1]) suggest what refs/ + namespace you may have wanted to push to. + +-- ++ The object referenced by <src> is used to update the <dst> reference on the remote side. Whether this is allowed depends on where in `refs/*` the <dst> reference lives as described in detail below, in @@ -591,6 +615,9 @@ the ones in the examples below) can be configured as the default for `refs/remotes/satellite/master`) in the `mothership` repository; do the same for `dev` and `satellite/dev`. + +See the section describing `<refspec>...` above for a discussion of +the matching semantics. ++ This is to emulate `git fetch` run on the `mothership` using `git push` that is run in the opposite direction in order to integrate the work done on `satellite`, and is often necessary when you can -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v2 1/7] remote.c: add braces in anticipation of a follow-up change 2018-10-11 22:45 ` Junio C Hamano 2018-10-26 15:45 ` Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 ` Ævar Arnfjörð Bjarmason 2018-10-26 21:05 ` Stefan Beller 2018-10-26 19:27 ` [PATCH v2 2/7] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason ` (5 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason The CodingGuidelines say "When there are multiple arms to a conditional and some of them require braces, enclose even a single line block in braces for consistency.". Fix the code in match_explicit() to conform. While I'm at it change the if/else if/else in guess_ref() to use braces. This is not currently needed, but a follow-up change will add a new multi-line condition to that logic. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/remote.c b/remote.c index 81f4f01b00..18cae48daa 100644 --- a/remote.c +++ b/remote.c @@ -968,12 +968,13 @@ static char *guess_ref(const char *name, struct ref *peer) if (!r) return NULL; - if (starts_with(r, "refs/heads/")) + if (starts_with(r, "refs/heads/")) { strbuf_addstr(&buf, "refs/heads/"); - else if (starts_with(r, "refs/tags/")) + } else if (starts_with(r, "refs/tags/")) { strbuf_addstr(&buf, "refs/tags/"); - else + } else { return NULL; + } strbuf_addstr(&buf, name); return strbuf_detach(&buf, NULL); @@ -1038,21 +1039,22 @@ static int match_explicit(struct ref *src, struct ref *dst, case 1: break; case 0: - if (starts_with(dst_value, "refs/")) + if (starts_with(dst_value, "refs/")) { matched_dst = make_linked_ref(dst_value, dst_tail); - else if (is_null_oid(&matched_src->new_oid)) + } else if (is_null_oid(&matched_src->new_oid)) { error("unable to delete '%s': remote ref does not exist", dst_value); - else if ((dst_guess = guess_ref(dst_value, matched_src))) { + } else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); - } else + } else { error("unable to push to unqualified destination: %s\n" "The destination refspec neither matches an " "existing ref on the remote nor\n" "begins with refs/, and we are unable to " "guess a prefix based on the source ref.", dst_value); + } break; default: matched_dst = NULL; -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 1/7] remote.c: add braces in anticipation of a follow-up change 2018-10-26 19:27 ` [PATCH v2 1/7] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason @ 2018-10-26 21:05 ` Stefan Beller 0 siblings, 0 replies; 53+ messages in thread From: Stefan Beller @ 2018-10-26 21:05 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, Jeff King On Fri, Oct 26, 2018 at 12:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > The CodingGuidelines say "When there are multiple arms to a > conditional and some of them require braces, enclose even a single > line block in braces for consistency.". Fix the code in > match_explicit() to conform. A tangent from a bystander: This sounds like a lovely transformation that we'd want to apply to the whole tree (Who wouldn't want to conform to our self-imposed coding guidelines?), so I tried coming up with a coccinelle patch to do that, but I did not manage to produce such a patch, yet. So far I am at @@ expression E1; statement S1, S2; @@ {<... if (E1) { S1; -} else +} else { S2; +} ...>} but this doesn't transform the simple test program that I wrote. > While I'm at it change the if/else if/else in guess_ref() to use > braces. This is not currently needed, but a follow-up change will add > a new multi-line condition to that logic. The patch looks good. Stefan ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 2/7] i18n: remote.c: mark error(...) messages for translation 2018-10-11 22:45 ` Junio C Hamano ` (2 preceding siblings ...) 2018-10-26 19:27 ` [PATCH v2 1/7] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 ` Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 3/7] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Mark up the error(...) messages in remote.c for translation. The likes of "unable to push to unqualified destination" are relatively big parts of the UI, i.e. error messages shown when "git push" fails. I don't think any of these are plumbing, an the entire test suite passes when running the tests with GIT_GETTEXT_POISON=1 (after building with GETTEXT_POISON). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/remote.c b/remote.c index 18cae48daa..5cb3d00bfb 100644 --- a/remote.c +++ b/remote.c @@ -406,7 +406,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!remote->receivepack) remote->receivepack = v; else - error("more than one receivepack given, using the first"); + error(_("more than one receivepack given, using the first")); } else if (!strcmp(subkey, "uploadpack")) { const char *v; if (git_config_string(&v, key, value)) @@ -414,7 +414,7 @@ static int handle_config(const char *key, const char *value, void *cb) if (!remote->uploadpack) remote->uploadpack = v; else - error("more than one uploadpack given, using the first"); + error(_("more than one uploadpack given, using the first")); } else if (!strcmp(subkey, "tagopt")) { if (!strcmp(value, "--no-tags")) remote->fetch_tags = -1; @@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs, int find_src = !query->src; if (find_src && !query->dst) - error("query_refspecs_multiple: need either src or dst"); + error(_("query_refspecs_multiple: need either src or dst")); for (i = 0; i < rs->nr; i++) { struct refspec_item *refspec = &rs->items[i]; @@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query) char **result = find_src ? &query->src : &query->dst; if (find_src && !query->dst) - return error("query_refspecs: need either src or dst"); + return error(_("query_refspecs: need either src or dst")); for (i = 0; i < rs->nr; i++) { struct refspec_item *refspec = &rs->items[i]; @@ -996,12 +996,12 @@ static int match_explicit_lhs(struct ref *src, * way to delete 'other' ref at the remote end. */ if (try_explicit_object_name(rs->src, match) < 0) - return error("src refspec %s does not match any.", rs->src); + return error(_("src refspec %s does not match any."), rs->src); if (allocated_match) *allocated_match = 1; return 0; default: - return error("src refspec %s matches more than one.", rs->src); + return error(_("src refspec %s matches more than one."), rs->src); } } @@ -1041,32 +1041,33 @@ static int match_explicit(struct ref *src, struct ref *dst, case 0: if (starts_with(dst_value, "refs/")) { matched_dst = make_linked_ref(dst_value, dst_tail); + } else if (is_null_oid(&matched_src->new_oid)) { - error("unable to delete '%s': remote ref does not exist", + error(_("unable to delete '%s': remote ref does not exist"), dst_value); } else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - error("unable to push to unqualified destination: %s\n" - "The destination refspec neither matches an " - "existing ref on the remote nor\n" - "begins with refs/, and we are unable to " - "guess a prefix based on the source ref.", + error(_("unable to push to unqualified destination: %s\n" + "The destination refspec neither matches an " + "existing ref on the remote nor\n" + "begins with refs/, and we are unable to " + "guess a prefix based on the source ref."), dst_value); } break; default: matched_dst = NULL; - error("dst refspec %s matches more than one.", + error(_("dst refspec %s matches more than one."), dst_value); break; } if (!matched_dst) return -1; if (matched_dst->peer_ref) - return error("dst ref %s receives from more than one src.", - matched_dst->name); + return error(_("dst ref %s receives from more than one src."), + matched_dst->name); else { matched_dst->peer_ref = allocated_src ? matched_src : @@ -1765,7 +1766,7 @@ int get_fetch_map(const struct ref *remote_refs, if (!starts_with((*rmp)->peer_ref->name, "refs/") || check_refname_format((*rmp)->peer_ref->name, 0)) { struct ref *ignore = *rmp; - error("* Ignoring funny ref '%s' locally", + error(_("* Ignoring funny ref '%s' locally"), (*rmp)->peer_ref->name); *rmp = (*rmp)->next; free(ignore->peer_ref); @@ -2133,7 +2134,7 @@ static int parse_push_cas_option(struct push_cas_option *cas, const char *arg, i else if (!colon[1]) oidclr(&entry->expect); else if (get_oid(colon + 1, &entry->expect)) - return error("cannot parse expected object name '%s'", colon + 1); + return error(_("cannot parse expected object name '%s'"), colon + 1); return 0; } -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v2 3/7] push: improve the error shown on unqualified <dst> push 2018-10-11 22:45 ` Junio C Hamano ` (3 preceding siblings ...) 2018-10-26 19:27 ` [PATCH v2 2/7] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 ` Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 4/7] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Improve the error message added in f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23), which before this change looks like this: $ git push avar v2.19.0^{commit}:newbranch -n error: unable to push to unqualified destination: newbranch The destination refspec neither matches an existing ref on the remote nor begins with refs/, and we are unable to guess a prefix based on the source ref. error: failed to push some refs to 'git@github.com:avar/git.git' This message needed to be read very carefully to spot how to fix the error, i.e. to push to refs/heads/newbranch. Now the message will look like this instead: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - Looking for a ref that matches 'newbranch' on the remote side. - Checking if the <src> being pushed ('v2.19.0^{commit}') is a ref in "refs/{heads,tags}/". If so we add a corresponding refs/{heads,tags}/ prefix on the remote side. Neither worked, so we gave up. You must fully-qualify the ref. error: failed to push some refs to 'git@github.com:avar/git.git' This improvement is the result of on-list discussion in [1] and [2], as well as my own fixes / reformatting / phrasing on top. The suggestion by Jeff on-list was to make that second bullet point "Looking at the refname of the local source.". The version being added here is more verbose, but also more accurate. saying "local source" could refer to any ref in the local refstore, including something in refs/remotes/*. A later change will teach guess_ref() to infer a ref type from remote-tracking refs, so let's not confuse the two. While I'm at it, add a "TRANSLATORS" comment since the message has gotten more complex and it's not as clear what the two %s's refer to. 1. https://public-inbox.org/git/20181010205505.GB12949@sigill.intra.peff.net/ 2. https://public-inbox.org/git/xmqqbm81lb7c.fsf@gitster-ct.c.googlers.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/remote.c b/remote.c index 5cb3d00bfb..f4b438ff74 100644 --- a/remote.c +++ b/remote.c @@ -1049,12 +1049,22 @@ static int match_explicit(struct ref *src, struct ref *dst, matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - error(_("unable to push to unqualified destination: %s\n" - "The destination refspec neither matches an " - "existing ref on the remote nor\n" - "begins with refs/, and we are unable to " - "guess a prefix based on the source ref."), - dst_value); + /* + * TRANSLATORS: "matches '%s'%" is the <dst> + * part of "git push <remote> <src>:<dst>" + * push, and "being pushed ('%s')" is the + * <src>. + */ + error(_("The destination you provided is not a full refname (i.e.,\n" + "starting with \"refs/\"). We tried to guess what you meant by:\n" + "\n" + "- Looking for a ref that matches '%s' on the remote side.\n" + "- Checking if the <src> being pushed ('%s')\n" + " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" + " refs/{heads,tags}/ prefix on the remote side.\n" + "\n" + "Neither worked, so we gave up. You must fully-qualify the ref."), + dst_value, matched_src->name); } break; default: -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v2 4/7] push: move unqualified refname error into a function 2018-10-11 22:45 ` Junio C Hamano ` (4 preceding siblings ...) 2018-10-26 19:27 ` [PATCH v2 3/7] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 ` Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 5/7] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason A follow-up change will extend this error message with the advice facility. Doing so would make the indentation too deeply nested for comfort. So let's split this into a helper function. There's no changes to the wording here. Just code moving & re-indentation, and re-flowing the "TRANSLATORS" comment. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/remote.c b/remote.c index f4b438ff74..c7a0b9c46f 100644 --- a/remote.c +++ b/remote.c @@ -1005,6 +1005,26 @@ static int match_explicit_lhs(struct ref *src, } } +static void show_push_unqualified_ref_name_error(const char *dst_value, + const char *matched_src_name) +{ + /* + * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push + * <remote> <src>:<dst>" push, and "being pushed ('%s')" is + * the <src>. + */ + error(_("The destination you provided is not a full refname (i.e.,\n" + "starting with \"refs/\"). We tried to guess what you meant by:\n" + "\n" + "- Looking for a ref that matches '%s' on the remote side.\n" + "- Checking if the <src> being pushed ('%s')\n" + " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" + " refs/{heads,tags}/ prefix on the remote side.\n" + "\n" + "Neither worked, so we gave up. You must fully-qualify the ref."), + dst_value, matched_src_name); +} + static int match_explicit(struct ref *src, struct ref *dst, struct ref ***dst_tail, struct refspec_item *rs) @@ -1049,22 +1069,8 @@ static int match_explicit(struct ref *src, struct ref *dst, matched_dst = make_linked_ref(dst_guess, dst_tail); free(dst_guess); } else { - /* - * TRANSLATORS: "matches '%s'%" is the <dst> - * part of "git push <remote> <src>:<dst>" - * push, and "being pushed ('%s')" is the - * <src>. - */ - error(_("The destination you provided is not a full refname (i.e.,\n" - "starting with \"refs/\"). We tried to guess what you meant by:\n" - "\n" - "- Looking for a ref that matches '%s' on the remote side.\n" - "- Checking if the <src> being pushed ('%s')\n" - " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" - " refs/{heads,tags}/ prefix on the remote side.\n" - "\n" - "Neither worked, so we gave up. You must fully-qualify the ref."), - dst_value, matched_src->name); + show_push_unqualified_ref_name_error(dst_value, + matched_src->name); } break; default: -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v2 5/7] push: add an advice on unqualified <dst> push 2018-10-11 22:45 ` Junio C Hamano ` (5 preceding siblings ...) 2018-10-26 19:27 ` [PATCH v2 4/7] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 ` Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 6/7] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 7/7] push: add DWYM support for "git push refs/remotes/...:<dst>" Ævar Arnfjörð Bjarmason 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Add an advice to the recently improved error message added in f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23). Now with advice.pushUnqualifiedRefName=true (on by default) we show a hint about how to proceed: $ ./git-push avar v2.19.0^{commit}:newbranch -n error: The destination you provided is not a full refname (i.e., starting with "refs/"). We tried to guess what you meant by: - Looking for a ref that matches newbranch on the remote side. - Looking at the refname of the local source. Neither worked, so we gave up. You must fully-qualify the ref. hint: The <src> part of the refspec is a commit object. hint: Did you mean to create a new branch by pushing to hint: 'v2.19.0^{commit}:refs/heads/newbranch'? error: failed to push some refs to 'git@github.com:avar/git.git' When trying to push a tag, tree or a blob we suggest that perhaps the user meant to push them to refs/tags/ instead. The if/else duplication for all of OBJ_{COMMIT,TAG,TREE,BLOB} is unfortunate, but is required to correctly mark the messages for translation. See the discussion in <87r2gxebsi.fsf@evledraar.gmail.com> about that. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/config.txt | 7 +++++++ advice.c | 2 ++ advice.h | 1 + remote.c | 37 +++++++++++++++++++++++++++++++++++++ t/t5505-remote.sh | 25 +++++++++++++++++++++++++ 5 files changed, 72 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 552827935a..8ca465702e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -320,6 +320,13 @@ advice.*:: tries to overwrite a remote ref that points at an object that is not a commit-ish, or make the remote ref point at an object that is not a commit-ish. + pushUnqualifiedRefname:: + Shown when linkgit:git-push[1] gives up trying to + guess based on the source and destination refs what + remote ref namespace the source belongs in, but where + we can still suggest that the user push to either + refs/heads/* or refs/tags/* based on the type of the + source object. statusHints:: Show directions on how to proceed from the current state in the output of linkgit:git-status[1], in diff --git a/advice.c b/advice.c index 3561cd64e9..3089a4ca65 100644 --- a/advice.c +++ b/advice.c @@ -9,6 +9,7 @@ int advice_push_non_ff_matching = 1; int advice_push_already_exists = 1; int advice_push_fetch_first = 1; int advice_push_needs_force = 1; +int advice_push_unqualified_ref_name = 1; int advice_status_hints = 1; int advice_status_u_option = 1; int advice_commit_before_merge = 1; @@ -62,6 +63,7 @@ static struct { { "pushAlreadyExists", &advice_push_already_exists }, { "pushFetchFirst", &advice_push_fetch_first }, { "pushNeedsForce", &advice_push_needs_force }, + { "pushUnqualifiedRefName", &advice_push_unqualified_ref_name }, { "statusHints", &advice_status_hints }, { "statusUoption", &advice_status_u_option }, { "commitBeforeMerge", &advice_commit_before_merge }, diff --git a/advice.h b/advice.h index ab24df0fd0..9a2f8b5226 100644 --- a/advice.h +++ b/advice.h @@ -9,6 +9,7 @@ extern int advice_push_non_ff_matching; extern int advice_push_already_exists; extern int advice_push_fetch_first; extern int advice_push_needs_force; +extern int advice_push_unqualified_ref_name; extern int advice_status_hints; extern int advice_status_u_option; extern int advice_commit_before_merge; diff --git a/remote.c b/remote.c index c7a0b9c46f..93f802509d 100644 --- a/remote.c +++ b/remote.c @@ -13,6 +13,7 @@ #include "mergesort.h" #include "argv-array.h" #include "commit-reach.h" +#include "advice.h" enum map_direction { FROM_SRC, FROM_DST }; @@ -1008,6 +1009,9 @@ static int match_explicit_lhs(struct ref *src, static void show_push_unqualified_ref_name_error(const char *dst_value, const char *matched_src_name) { + struct object_id oid; + enum object_type type; + /* * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push * <remote> <src>:<dst>" push, and "being pushed ('%s')" is @@ -1023,6 +1027,39 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, "\n" "Neither worked, so we gave up. You must fully-qualify the ref."), dst_value, matched_src_name); + + if (!advice_push_unqualified_ref_name) + return; + + if (get_oid(matched_src_name, &oid)) + BUG("'%s' is not a valid object, " + "match_explicit_lhs() should catch this!", + matched_src_name); + type = oid_object_info(the_repository, &oid, NULL); + if (type == OBJ_COMMIT) { + advise(_("The <src> part of the refspec is a commit object.\n" + "Did you mean to create a new branch by pushing to\n" + "'%s:refs/heads/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_TAG) { + advise(_("The <src> part of the refspec is a tag object.\n" + "Did you mean to create a new tag by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_TREE) { + advise(_("The <src> part of the refspec is a tree object.\n" + "Did you mean to tag a new tree by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else if (type == OBJ_BLOB) { + advise(_("The <src> part of the refspec is a blob object.\n" + "Did you mean to tag a new blob by pushing to\n" + "'%s:refs/tags/%s'?"), + matched_src_name, dst_value); + } else { + BUG("'%s' should be commit/tag/tree/blob, is '%d'", + matched_src_name, type); + } } static int match_explicit(struct ref *src, struct ref *dst, diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index d2a2cdd453..2e58721f98 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1222,4 +1222,29 @@ test_expect_success 'add remote matching the "insteadOf" URL' ' git remote add backup xyz@example.com ' +test_expect_success 'unqualified <dst> refspec DWIM and advice' ' + test_when_finished "(cd test && git tag -d some-tag)" && + ( + cd test && + git tag -a -m "Some tag" some-tag master && + for type in commit tag tree blob + do + if test "$type" = "blob" + then + oid=$(git rev-parse some-tag:file) + else + oid=$(git rev-parse some-tag^{$type}) + fi && + test_must_fail git push origin $oid:dst 2>err && + test_i18ngrep "error: The destination you" err && + test_i18ngrep "hint: Did you mean" err && + test_must_fail git -c advice.pushUnqualifiedRefName=false \ + push origin $oid:dst 2>err && + test_i18ngrep "error: The destination you" err && + test_i18ngrep ! "hint: Did you mean" err + done + ) +' + + test_done -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v2 6/7] push: test that <src> doesn't DWYM if <dst> is unqualified 2018-10-11 22:45 ` Junio C Hamano ` (6 preceding siblings ...) 2018-10-26 19:27 ` [PATCH v2 5/7] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 ` Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 7/7] push: add DWYM support for "git push refs/remotes/...:<dst>" Ævar Arnfjörð Bjarmason 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Add a test asserting that "git push origin <src>:<dst>" where <src> is a branch, tag, tree or blob in refs/remotes/* doesn't DWYM when <dst> is unqualified. This has never worked, but there's been no test for this behavior. See f88395ac23 ("Renaming push.", 2005-08-03), bb9fca80ce ("git-push: Update description of refspecs and add examples", 2007-06-09) and f8aae12034 ("push: allow unqualified dest refspecs to DWIM", 2008-04-23) which are most relevant commits that have changed or documented the behavior of this feature in the past. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5505-remote.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 2e58721f98..979a13b415 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1246,5 +1246,33 @@ test_expect_success 'unqualified <dst> refspec DWIM and advice' ' ) ' +test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and advice' ' + ( + cd two && + git tag -a -m "Some tag" some-tag master && + git update-ref refs/trees/my-head-tree HEAD^{tree} && + git update-ref refs/blobs/my-file-blob HEAD:file + ) && + ( + cd test && + git config --add remote.two.fetch "+refs/tags/*:refs/remotes/two-tags/*" && + git config --add remote.two.fetch "+refs/trees/*:refs/remotes/two-trees/*" && + git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/two-blobs/*" && + git fetch --no-tags two && + + test_must_fail git push origin refs/remotes/two/another:dst 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/two-tags/some-tag:dst-tag 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/two-trees/my-head-tree:dst-tree 2>err && + test_i18ngrep "error: The destination you" err && + + test_must_fail git push origin refs/remotes/two-blobs/my-file-blob:dst-blob 2>err && + test_i18ngrep "error: The destination you" err + ) +' + test_done -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v2 7/7] push: add DWYM support for "git push refs/remotes/...:<dst>" 2018-10-11 22:45 ` Junio C Hamano ` (7 preceding siblings ...) 2018-10-26 19:27 ` [PATCH v2 6/7] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 ` Ævar Arnfjörð Bjarmason 8 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-10-26 19:27 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Add DWYM support for pushing a ref in refs/remotes/* when the <dst> ref is unqualified, e.g.: git push origin refs/remotes/origin/master:upstream-master Before this we wouldn't know what do do with refs/remotes/origin/master, now we'll look it up and discover that it's a commit (or tag) and add a refs/{heads,tags}/ prefix to <dst> as appropriate. I'm bending over backwards and assuming that someone might have hacked in remote tracking tags (see [1] for a discussion of how that can be done), but punting on any tree or blob objects found under refs/remotes/*. This is the first use of the %N$<fmt> style of printf format in the *.[ch] files in our codebase, but it's supported by POSIX[2] and there's existing uses for it in po/*.po files, so hopefully it won't cause any trouble. It's more obvious for translators than to have a 3rd argument to the function identical to the 2nd, by re-using the 2nd it's clear that we're continuing to talk about the <src> part of the refspec. 1. https://public-inbox.org/git/87zi1jxjqn.fsf@evledraar.gmail.com/ 2. http://pubs.opengroup.org/onlinepubs/7908799/xsh/fprintf.html Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- remote.c | 20 +++++++++++++++++++- t/t5505-remote.sh | 14 +++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/remote.c b/remote.c index 93f802509d..c243e3d89e 100644 --- a/remote.c +++ b/remote.c @@ -973,6 +973,21 @@ static char *guess_ref(const char *name, struct ref *peer) strbuf_addstr(&buf, "refs/heads/"); } else if (starts_with(r, "refs/tags/")) { strbuf_addstr(&buf, "refs/tags/"); + } else if (starts_with(r, "refs/remotes/")) { + struct object_id oid; + enum object_type type; + + if (get_oid(peer->name, &oid)) + BUG("'%s' is not a valid object, " + "match_explicit_lhs() should catch this!", + peer->name); + type = oid_object_info(the_repository, &oid, NULL); + if (type == OBJ_COMMIT) + strbuf_addstr(&buf, "refs/heads/"); + else if (type == OBJ_TAG) + strbuf_addstr(&buf, "refs/tags/"); + else + return NULL; } else { return NULL; } @@ -1024,8 +1039,11 @@ static void show_push_unqualified_ref_name_error(const char *dst_value, "- Checking if the <src> being pushed ('%s')\n" " is a ref in \"refs/{heads,tags}/\". If so we add a corresponding\n" " refs/{heads,tags}/ prefix on the remote side.\n" + "- Checking if the <src> being pushed ('%2$s')\n" + " is a commit or tag in \"refs/remotes/*\". Then we infer a\n" + " corresponding refs/{heads,tags} on the remote side.\n" "\n" - "Neither worked, so we gave up. You must fully-qualify the ref."), + "None of those worked, so we gave up. You must fully-qualify the ref."), dst_value, matched_src_name); if (!advice_push_unqualified_ref_name) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 979a13b415..a6337b50e4 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1260,11 +1260,15 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and git config --add remote.two.fetch "+refs/blobs/*:refs/remotes/two-blobs/*" && git fetch --no-tags two && - test_must_fail git push origin refs/remotes/two/another:dst 2>err && - test_i18ngrep "error: The destination you" err && - - test_must_fail git push origin refs/remotes/two-tags/some-tag:dst-tag 2>err && - test_i18ngrep "error: The destination you" err && + echo commit >expected && + git push origin refs/remotes/two/another:dst && + git -C ../one cat-file -t refs/heads/dst >actual && + test_cmp expected actual && + + echo tag >expected && + git push origin refs/remotes/two-tags/some-tag:dst-tag && + git -C ../one cat-file -t refs/tags/dst-tag >actual && + test_cmp expected actual && test_must_fail git push origin refs/remotes/two-trees/my-head-tree:dst-tree 2>err && test_i18ngrep "error: The destination you" err && -- 2.19.1.759.g500967bb5e ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-10 20:55 ` Jeff King 2018-10-10 21:23 ` Ævar Arnfjörð Bjarmason @ 2018-10-10 21:54 ` Junio C Hamano 2018-10-11 0:19 ` Jeff King 1 sibling, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-10-10 21:54 UTC (permalink / raw) To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git Jeff King <peff@peff.net> writes: >> Fix both of those, now the message will look like this instead: >> >> $ ./git-push avar v2.19.0^{commit}:newbranch -n >> error: unable to push to unqualified destination: newbranch >> hint: The destination refspec neither matches an existing >> hint: ref on the remote nor begins with refs/, and we are >> hint: unable to guess a prefix based on the source ref. >> hint: >> hint: The <src> part of the refspec is a commit object. >> hint: Did you mean to create a new branch by pushing to >> hint: 'v2.19.0^{commit}:refs/heads/newbranch'? >> error: failed to push some refs to 'git@github.com:avar/git.git' >> >> When trying to push a tag, tree or a blob we suggest that perhaps the >> user meant to push them to refs/tags/ instead. > > This is much better, and I love the customized behavior based on the > object type. > > I wonder if we could reword the first paragraph to be a little less > confusing, and spell out what we tried already. E.g., something like: > > The destination you provided is not a full refname (i.e., starting > with "ref"). Git tried to guess what you meant by: s|ref|refs/|; I fully agree that "unqualified destination" was a poor way to communicate the failure to those who would likely hit this error path, because somebody who can ell what's qualified and what's not would not be triggering the error in the first place. > - looking for a matching branch or tag on the remote side > > - looking at the refname of the local source > > but neither worked. > > The <src> part of the refspec is a commit object. > Did you mean... Looks great. > I'm not sure about saying "branch or tag" in the first bullet. It's > friendlier to most users, but less technically correct (if you said > "notes/foo", I believe we'd match an existing "refs/notes/foo", because > it's really just using the normal lookup rules). An alternative may be "looking for a ref that matches %s on the remote side". I am no longer a total newbie, so I cannot tell how well that message would help one to connect notes/foo one just typed with refs/notes/foo that potentially exists on the remote side. > Also, as an aside, I wonder if we should allow "heads/foo" to work as > "refs/heads/foo" (even when no such ref already exists). But that is > totally orthogonal to changing the message. I am neutral on this point but agree that it is better done outside this patch. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push 2018-10-10 21:54 ` [PATCH 2/2] push: add an advice on unqualified <dst> push Junio C Hamano @ 2018-10-11 0:19 ` Jeff King 0 siblings, 0 replies; 53+ messages in thread From: Jeff King @ 2018-10-11 0:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git On Thu, Oct 11, 2018 at 06:54:15AM +0900, Junio C Hamano wrote: > > I'm not sure about saying "branch or tag" in the first bullet. It's > > friendlier to most users, but less technically correct (if you said > > "notes/foo", I believe we'd match an existing "refs/notes/foo", because > > it's really just using the normal lookup rules). > > An alternative may be "looking for a ref that matches %s on the > remote side". I am no longer a total newbie, so I cannot tell how > well that message would help one to connect notes/foo one just typed > with refs/notes/foo that potentially exists on the remote side. Yeah. Really, it would be nice to imply that it somehow does the same DWIM lookup that we do for local refs. But I didn't know how to say that. Possibly we could refer to the documentation, but it's buried in git-rev-parse. > > Also, as an aside, I wonder if we should allow "heads/foo" to work as > > "refs/heads/foo" (even when no such ref already exists). But that is > > totally orthogonal to changing the message. > > I am neutral on this point but agree that it is better done outside > this patch. Yeah, definitely. I would almost call it a leftover bit, but I think the subtlety is not in the code, but in whether it is a good thing to be doing (i.e., too many false positives). -Peff ^ permalink raw reply [flat|nested] 53+ messages in thread
end of thread, other threads:[~2018-11-14 7:00 UTC | newest] Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-10 10:41 [PATCH 0/2] add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-10-10 10:41 ` [PATCH 1/2] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason 2018-10-10 20:55 ` Jeff King 2018-10-10 10:41 ` [PATCH 2/2] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-10-10 20:55 ` Jeff King 2018-10-10 21:23 ` Ævar Arnfjörð Bjarmason 2018-10-11 0:16 ` Jeff King 2018-10-11 22:45 ` Junio C Hamano 2018-10-26 15:45 ` Ævar Arnfjörð Bjarmason 2018-10-29 1:00 ` Junio C Hamano 2018-10-29 4:17 ` Junio C Hamano 2018-10-26 19:27 ` [PATCH v2 0/7] fixes for " Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason 2018-11-02 6:52 ` Jeff King 2018-11-13 19:52 ` [PATCH v4 0/7] " Ævar Arnfjörð Bjarmason 2018-11-14 7:00 ` Junio C Hamano 2018-11-13 19:52 ` [PATCH v4 1/7] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 2/7] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 3/7] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 4/7] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 5/7] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 6/7] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason 2018-11-13 19:52 ` [PATCH v4 7/7] push doc: document the DWYM behavior pushing to unqualified <dst> Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 1/8] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 2/8] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 3/8] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-10-29 5:03 ` Junio C Hamano 2018-10-26 23:07 ` [PATCH v3 4/8] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 5/8] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-10-29 5:14 ` Junio C Hamano 2018-11-02 6:47 ` Jeff King 2018-10-26 23:07 ` [PATCH v3 6/8] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason 2018-10-29 5:19 ` Junio C Hamano 2018-10-26 23:07 ` [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>" Ævar Arnfjörð Bjarmason 2018-10-29 5:24 ` Junio C Hamano 2018-10-29 8:13 ` Ævar Arnfjörð Bjarmason 2018-11-01 4:18 ` Junio C Hamano 2018-11-05 11:31 ` Ævar Arnfjörð Bjarmason 2018-11-05 12:29 ` Junio C Hamano 2018-10-29 7:06 ` Junio C Hamano 2018-10-29 7:57 ` Junio C Hamano 2018-10-29 8:05 ` Ævar Arnfjörð Bjarmason 2018-10-26 23:07 ` [PATCH v3 8/8] push doc: document the DWYM behavior pushing to unqualified <dst> Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 1/7] remote.c: add braces in anticipation of a follow-up change Ævar Arnfjörð Bjarmason 2018-10-26 21:05 ` Stefan Beller 2018-10-26 19:27 ` [PATCH v2 2/7] i18n: remote.c: mark error(...) messages for translation Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 3/7] push: improve the error shown on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 4/7] push: move unqualified refname error into a function Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 5/7] push: add an advice on unqualified <dst> push Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 6/7] push: test that <src> doesn't DWYM if <dst> is unqualified Ævar Arnfjörð Bjarmason 2018-10-26 19:27 ` [PATCH v2 7/7] push: add DWYM support for "git push refs/remotes/...:<dst>" Ævar Arnfjörð Bjarmason 2018-10-10 21:54 ` [PATCH 2/2] push: add an advice on unqualified <dst> push Junio C Hamano 2018-10-11 0:19 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.