Merge branch 'ab/push-dwim-dst'
authorJunio C Hamano <gitster@pobox.com>
Fri, 4 Jan 2019 21:33:33 +0000 (13:33 -0800)
committerJunio C Hamano <gitster@pobox.com>
Fri, 4 Jan 2019 21:33:34 +0000 (13:33 -0800)
"git push $there $src:$dst" rejects when $dst is not a fully
qualified refname and not clear what the end user meant. The
codepath has been taught to give a clearer error message, and also
guess where the push should go by taking the type of the pushed
object into account (e.g. a tag object would want to go under
refs/tags/).

* ab/push-dwim-dst:
push doc: document the DWYM behavior pushing to unqualified <dst>
push: test that <src> doesn't DWYM if <dst> is unqualified
push: add an advice on unqualified <dst> push
push: move unqualified refname error into a function
push: improve the error shown on unqualified <dst> push
i18n: remote.c: mark error(...) messages for translation
remote.c: add braces in anticipation of a follow-up change

Documentation/config/advice.txt
Documentation/git-push.txt
advice.c
advice.h
remote.c
t/t5505-remote.sh
index 57fcd4c8622fc6e996308a1412a27100419a16be..88620429eacf857c7ba7941314c9b443b35b58ce 100644 (file)
@@ -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
index a5fc54aeabccbeac32c73a610dbee45bfb5b0746..6a8a0d958bc63db19bfa4786e85e9e14061f1661 100644 (file)
@@ -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
index 5f35656409b1d51abf111efa5bbcc7f5d570aaf0..567209aa79afee0443dd0d7049343e1a004ae784 100644 (file)
--- 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 },
index 696bf0e7d29ee107c5faf10a59985c0f49612495..f875f8cd8da5fdb6e570770ecc7ab4461745f827 100644 (file)
--- 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;
index 70aba02c7453b8153b08a1f66852e5bae80707e6..670dd448130d20325f5f9f1de8afd0f011535db2 100644 (file)
--- 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 };
 
@@ -968,12 +969,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);
@@ -1004,6 +1006,62 @@ 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
+        * 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);
+
+       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,
                          struct ref ***dst_tail,
                          struct refspec_item *rs)
@@ -1038,21 +1096,18 @@ 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
-                       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 {
+                       show_push_unqualified_ref_name_error(dst_value,
+                                                            matched_src->name);
+               }
                break;
        default:
                matched_dst = NULL;
index d2a2cdd453396b1dba5525f32b6325ac8a10d95d..883b32efa024d97ac74e9ae5763dc8a218251675 100755 (executable)
@@ -1222,4 +1222,59 @@ 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_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