From: Mark Jamsek <mark@jamsek.com>
Subject: Re: tweak got merge error message
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Fri, 23 Jun 2023 00:55:47 +1000

Stefan Sperling <stsp@stsp.name> wrote:
> The new wording below is a bit longer but I hope it will be less
> confusing to new users, especially if they do not know Git very well.
> It is based in part on how the -n option is documented in the manual.
> 
> before:
> 
> got: merge is a fast-forward; this is incompatible with got merge -n
> 
> after:
> 
> got: there are no changes to merge since refs/heads/newbranch is \
>   already based on refs/heads/master; merge cannot be interrupted \
>   for amending; -n: option cannot be used
> 
> I also considered dropping this check entirely. But since -n is not
> the default behaviour it makes sense to stop the operation and let
> the user reconsider their approach.

I agree on both counts. It is a bit verbose, but sometimes a little
extra explanation is in order; your commit message nails this precisely.
It's better not to assume endusers understand technical nomenclature
too. And, yes, don't drop this check. The user has asked for it, so we
should explain why it can't be done.

ok


> -----------------------------------------------
>  reword user-facing error message which mentions "fast-forward"
>  
>  For user-facing messages it is better to avoid technical jargon like
>  this and instead spell out what the fast-forward situation implies: that
>  one branch is already based on another.
>  
> diff b73055ebbea59457c1aef2cf7c473ea2898b434b 15fdc4b7bab4dbe54aa6c2a8c401c62882f70a0a
> commit - b73055ebbea59457c1aef2cf7c473ea2898b434b
> commit + 15fdc4b7bab4dbe54aa6c2a8c401c62882f70a0a
> blob - ea8a08baed68affba73cafa577c3125e9756466a
> blob + 61dfa9d2bbab0c9bcc68905af78e9f8c9270bfbe
> --- got/got.c
> +++ got/got.c
> @@ -13319,9 +13319,11 @@ cmd_merge(int argc, char *argv[])
>  		if (yca_id && got_object_id_cmp(wt_branch_tip, yca_id) == 0) {
>  			struct got_pathlist_head paths;
>  			if (interrupt_merge) {
> -				error = got_error_msg(GOT_ERR_SAME_BRANCH,
> -				    "merge is a fast-forward; this is "
> -				    "incompatible with got merge -n");
> +				error = got_error_fmt(GOT_ERR_BAD_OPTION,
> +				    "there are no changes to merge since %s "
> +				    "is already based on %s; merge cannot be "
> +				    "interrupted for amending; -n",
> +				    branch_name, got_ref_get_name(wt_branch));
>  				goto done;
>  			}
>  			printf("Forwarding %s to %s\n",
> blob - c7c5ce296704a7d0c3345092eccebd5837fcd453
> blob + 6ca6d041c1b17b5198707cc33b19f9fefccf07e0
> --- include/got_error.h
> +++ include/got_error.h
> @@ -171,7 +171,7 @@
>  #define GOT_ERR_BAD_TAG_SIGNATURE 154
>  #define GOT_ERR_VERIFY_TAG_SIGNATURE 155
>  #define GOT_ERR_SIGNING_TAG	156
> -/* 157 is currently unused */
> +#define GOT_ERR_BAD_OPTION 157
>  #define GOT_ERR_BAD_QUERYSTRING	158
>  #define GOT_ERR_INTEGRATE_BRANCH 159
>  #define GOT_ERR_BAD_REQUEST	160
> blob - 765efe263e09f28dfd6b925eaa64ee0ffe226a3b
> blob + 14daf3a57c9be57d4f707ce25da1a357e09ada88
> --- lib/error.c
> +++ lib/error.c
> @@ -217,6 +217,7 @@ static const struct got_error got_errors[] = {
>  	{ GOT_ERR_BAD_TAG_SIGNATURE, "invalid tag signature" },
>  	{ GOT_ERR_VERIFY_TAG_SIGNATURE, "cannot verify signature" },
>  	{ GOT_ERR_SIGNING_TAG, "unable to sign tag" },
> +	{ GOT_ERR_BAD_OPTION, "option cannot be used" },
>  	{ GOT_ERR_BAD_QUERYSTRING, "invalid query string" },
>  	{ GOT_ERR_INTEGRATE_BRANCH, "will not integrate into a reference "
>  	    "outside the \"refs/heads/\" reference namespace" },
> blob - 92b590aa2ef2958738fb2d4e8419cb879fc9bf5c
> blob + cd813c681b68090d962e708e8d375740e5ac421c
> --- regress/cmdline/merge.sh
> +++ regress/cmdline/merge.sh
> @@ -415,9 +415,16 @@ test_merge_forward() {
>  		test_done "$testroot" "1"
>  		return 1
>  	fi
> -	echo -n "got: merge is a fast-forward; " > $testroot/stderr.expected 
> -	echo "this is incompatible with got merge -n" \
> +
> +	echo -n "got: there are no changes to merge since " \
> +		> $testroot/stderr.expected
> +	echo -n "refs/heads/newbranch is already based on " \
>  		>> $testroot/stderr.expected
> +	echo -n "refs/heads/master; merge cannot be interrupted " \
> +		>> $testroot/stderr.expected
> +	echo "for amending; -n: option cannot be used" \
> +		>> $testroot/stderr.expected
> +
>  	cmp -s $testroot/stderr.expected $testroot/stderr
>  	ret=$?
>  	if [ $ret -ne 0 ]; then


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68