Re: [PATCH v2] branch: improve error log on branch not found by checking remotes refs

[Junio C Hamano <gitster@pobox.com>]

"ClementMabileau via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ctmbl <mabileau.clement@gmail.com>

This "name <address>" should match what is on "Signed-off-by:".  I
offhand do not know where GitGitGadget gets the information, but I
suspect the original commit object you created records ctmbl as the
author name, not "Clement Mabileau"?  I can fix it for this single
time, but if you plan to contribute to the project in an ongoing
basis, you may want to fix your .git/config (or $HOME/.gitconfig)
with a "[user] name = ..." entry.


> when failing to delete a branch with `git branch -d <branch>` because
> of branch not found, try to find a remote refs matching `<branch>`
> and if so add an hint:
> `Did you forget --remote?` to the error message



>  builtin/branch.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index f63fd45edb9..697636e2874 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -216,10 +216,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
>  	struct string_list_item *item;
>  	int branch_name_pos;
> +	char* FMT_REMOTES = "refs/remotes/%s";
> +	char* FMT_BRANCHES = "refs/heads/%s";

In our codebase, the asterisk sticks to the variable, not to the
type.  cf. Documentation/CodingGuidelines

 - When declaring pointers, the star sides with the variable
   name, i.e. "char *string", not "char* string" or
   "char * string".  This makes it easier to understand code
   like "char *string, c;".

> @@ -263,9 +265,26 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  					| RESOLVE_REF_ALLOW_BAD_NAME,
>  					&oid, &flags);
>  		if (!target) {
> -			error(remote_branch
> -			      ? _("remote-tracking branch '%s' not found.")
> -			      : _("branch '%s' not found."), bname.buf);
> +			char* MISSING_REMOTE_REF_ERROR_MSG = "remote-tracking branch '%s' not found.";
> +			char* MISSING_BRANCH_ERROR_MSG = "branch '%s' not found.";
> +			char* MISSING_BRANCH_HINT_MSG = "branch '%s' not found.\n"
> +											"Did you forget --remote?";

You'd need to enclose these text literals inside N_() to tell the
tools that they are to be translated.  e.g.

	char *remote_ref_error = N_("remote-tracking branch '%s' not found.");

Otherwise, using the variables inside _() would not let the
machinery to find translated strings.

Having said that, I do not see the point of introducing three
variables that are hard to read, misleadingly named with all capital
letters as if they were macros, and each of which is only used once.
It does not make the code that computes which message gets used any
easier to follow (by hiding long text literal), and it does not help
reusing the same message elsewhere.  For that matter, the two new
variables we saw above (again in all caps) are also of dubious merit.



> +
> +			if (remote_branch) {
> +				error(_(MISSING_REMOTE_REF_ERROR_MSG), bname.buf);
> +			} else {
> +				char* virtual_name = mkpathdup(FMT_REMOTES, bname.buf);

Hmph.  bname.buf at this point has what?  

If the user said "git branch -d frotz", we would have 'frotz'
there, right?  Then we apply FMT_REMOTES (Yuck, now I have to go
scroll up, see that it is set to "refs/remotes/%s", and hope or
verify that its value hasn't been changed in the meantime---in
short, don't introduce that variable.  A macro might be OK, but I do
not see much point here) and we get "refs/remotes/frotz" back.

So, we check "refs/remotes/frotz" here ...

> +				char* virtual_target = resolve_refdup(virtual_name,
> +							RESOLVE_REF_READING
> +							| RESOLVE_REF_NO_RECURSE
> +							| RESOLVE_REF_ALLOW_BAD_NAME,
> +							&oid, &flags);
> +				FREE_AND_NULL(virtual_name);

... but why should we?  If your workflow interacts with the original
repository you cloned from, you would have remote-tracking branches
like "refs/remotes/origin/frotz" and it may be plausible that you
meant to remove with "git branch -d frotz" the remote-tracking
branch "refs/remotes/origin/frotz".  But the new code makes no
effort to figure out the name of the remote (e.g. 'origin') here,
and I am not sure what value it adds to check and try to tell the
user about "refs/remotes/frotz".  Or are we assuming that the user
would say "git branch -d origin/frotz" in such a case?

> +				if (virtual_target)
> +					error(_(MISSING_BRANCH_HINT_MSG), bname.buf);
> +				else
> +					error(_(MISSING_BRANCH_ERROR_MSG), bname.buf);
> +			}

Are you leaking virtual_target here?

>  			ret = 1;
>  			continue;
>  		}