git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [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; 10+ 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] 10+ 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; 10+ 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	[flat|nested] 10+ 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; 10+ 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	[flat|nested] 10+ 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     ` Junio C Hamano
  0 siblings, 2 replies; 10+ 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] 10+ 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; 10+ 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] 10+ 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     ` Junio C Hamano
  1 sibling, 2 replies; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread

* Re: [PATCH 2/2] push: add an advice on unqualified <dst> push
  2018-10-10 21:54     ` Junio C Hamano
@ 2018-10-11  0:19       ` Jeff King
  0 siblings, 0 replies; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ 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-10 21:54     ` Junio C Hamano
2018-10-11  0:19       ` Jeff King

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