git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Jeff King <peff@peff.net>
To: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 2/2] push: add an advice on unqualified <dst> push
Date: Wed, 10 Oct 2018 16:55:06 -0400
Message-ID: <20181010205505.GB12949@sigill.intra.peff.net> (raw)
In-Reply-To: <20181010104145.25610-3-avarab@gmail.com>

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

  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-10 10:41 [PATCH 0/2] " Æ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 [this message]
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

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=20181010205505.GB12949@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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