git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Ævar Arnfjörð Bjarmason  <avarab@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Stefan Beller <sbeller@google.com>,
	Ævar Arnfjörð Bjarmason  <avarab@gmail.com>
Subject: [PATCH v4 0/7] fixes for unqualified <dst> push
Date: Tue, 13 Nov 2018 19:52:38 +0000
Message-ID: <20181113195245.14296-1-avarab@gmail.com> (raw)
In-Reply-To: <20181026230741.23321-1-avarab@gmail.com>

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


  parent reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-10 10:41 [PATCH 0/2] add an advice on " Æ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             ` Ævar Arnfjörð Bjarmason [this message]
2018-11-14  7:00               ` [PATCH v4 0/7] " 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

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181113195245.14296-1-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox