branch: improve error log on branch not found by checking remotes refs

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

ctmbl (2):
branch: improve error log on branch not found by checking remotes refs Fix mem leak in branch.c due to not-free newly added virtual_name
variable

 builtin/branch.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

base-commit: 950264636c68591989456e3ba0a5442f93152c1a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1476%2Fctmbl%2Fbranch%2Fdeletion%2Fimprove-error-msg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1476/ctmbl/branch/deletion/improve-error-msg-v1
Pull-Request: https://github.com/git/git/pull/1476

gitgitgadget

From: ctmbl <[email protected]>

Previous commit introduced virtual_name variable which is the name of the branch in case it has been a remote ref (used to check whether the user simply forgot --remote flag) but didn't free it. Call FREE_AND_NULL(virtual_name) to solve it.

---
 builtin/branch.c | 1 +
 1 file changed, 1 insertion(+)

index 8e768761b9b..697636e2874 100644
@@ -279,6 +279,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 							| RESOLVE_REF_NO_RECURSE
 							| RESOLVE_REF_ALLOW_BAD_NAME,
 							&oid, &flags);
+				FREE_AND_NULL(virtual_name);
 				if (virtual_target)
 					error(_(MISSING_BRANCH_HINT_MSG), bname.buf);
 				else

gitgitgadget

From: ctmbl <[email protected]>

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 | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

index f63fd45edb9..8e768761b9b 100644
@@ -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";
 
 	switch (kinds) {
 	case FILTER_REFS_REMOTES:
-		fmt = "refs/remotes/%s";
+		fmt = FMT_REMOTES;
 		/* For subsequent UI messages */
 		remote_branch = 1;
 		allowed_interpret = INTERPRET_BRANCH_REMOTE;

@@ -227,7 +229,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		force = 1;
 		break;
 	case FILTER_REFS_BRANCHES:
-		fmt = "refs/heads/%s";
+		fmt = FMT_BRANCHES;
 		allowed_interpret = INTERPRET_BRANCH_LOCAL;
 		break;
 	default:

@@ -263,9 +265,25 @@ 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?";
+
+			if (remote_branch) {
+				error(_(MISSING_REMOTE_REF_ERROR_MSG), bname.buf);
+			} else {
+				char* virtual_name = mkpathdup(FMT_REMOTES, bname.buf);
+				char* virtual_target = resolve_refdup(virtual_name,
+							RESOLVE_REF_READING
+							| RESOLVE_REF_NO_RECURSE
+							| RESOLVE_REF_ALLOW_BAD_NAME,
+							&oid, &flags);
+				if (virtual_target)
+					error(_(MISSING_BRANCH_HINT_MSG), bname.buf);
+				else
+					error(_(MISSING_BRANCH_ERROR_MSG), bname.buf);
+			}
 			ret = 1;
 			continue;
 		}

gitgitgadget

Do not introduce a bug in 1/2 and fix it in 2/2, unless it is absolutely necessary.

Squash this one into the previous commit, before sending your patches out to the list. The public do not have to learn your earlier mistakes, and before sending your patches out is the perfect chance for you to pretend to be a person that is more perfect than you are ;-).

Thanks.

I didn't know this practice, thanks for letting me know :-)

I just did it!

Thanks for the review.

From: ctmbl <[email protected]>

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

---
branch: improve error log on branch not found by checking remotes refs

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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1476%2Fctmbl%2Fbranch%2Fdeletion%2Fimprove-error-msg-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1476/ctmbl/branch/deletion/improve-error-msg-v2
Pull-Request: https://github.com/git/git/pull/1476

Range-diff vs v1:

1: 91cb506968a ! 1: eb95695ace2 branch: improve error log on branch not found by checking remotes refs

     @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo
      +							| RESOLVE_REF_NO_RECURSE
      +							| RESOLVE_REF_ALLOW_BAD_NAME,
      +							&oid, &flags);
     ++				FREE_AND_NULL(virtual_name);
      +				if (virtual_target)
      +					error(_(MISSING_BRANCH_HINT_MSG), bname.buf);
      +				else

2: 27f27f3afd7 < -: ----------- Fix mem leak in branch.c due to not-free newly added virtual_name variable

 builtin/branch.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

index f63fd45edb9..697636e2874 100644
@@ -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";
 
 	switch (kinds) {
 	case FILTER_REFS_REMOTES:
-		fmt = "refs/remotes/%s";
+		fmt = FMT_REMOTES;
 		/* For subsequent UI messages */
 		remote_branch = 1;
 		allowed_interpret = INTERPRET_BRANCH_REMOTE;

@@ -227,7 +229,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		force = 1;
 		break;
 	case FILTER_REFS_BRANCHES:
-		fmt = "refs/heads/%s";
+		fmt = FMT_BRANCHES;
 		allowed_interpret = INTERPRET_BRANCH_LOCAL;
 		break;
 	default:

@@ -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?";
+
+			if (remote_branch) {
+				error(_(MISSING_REMOTE_REF_ERROR_MSG), bname.buf);
+			} else {
+				char* virtual_name = mkpathdup(FMT_REMOTES, bname.buf);
+				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);
+				if (virtual_target)
+					error(_(MISSING_BRANCH_HINT_MSG), bname.buf);
+				else
+					error(_(MISSING_BRANCH_ERROR_MSG), bname.buf);
+			}
 			ret = 1;
 			continue;
 		}

base-commit: 950264636c68591989456e3ba0a5442f93152c1a

gitgitgadget

Nice to hear.

Well, I didn't "review" anything, though. I just commented on 2/2 that will become moot in your updated version, and I didn't even look at 1/2.

The change(s) as a whole need to be reviewed by somebody.

Thanks.

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.

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

Well maybe it got it from the original commit or from my GitHub username, I can't know for sure. But sorry for that :/ , I'll make sure it's fixed it for potential future contribution!

Before fixing anything about the code maybe I should first address your last point which is the interest of the patch in the first place (and I should have started with that...).

A few months earlier, for the first time, I had to delete a remote ref (because of a fork I fetched but no longer wanted: maybe a designed solution exists but I'm not aware of it). However, despite being used to git I had a hard time figuring out how to do it, I tried different things, one was git branch -d origin/<branch> (I recently discovered that it was written in git branch --help but I didn't find it at the time). Even googling it proved difficult (because of a poor keyword choice I must confess), most results was dealing with deleting remote branches, such as git push remote :branch.
In the end, I finally understood that I needed that --remote flag and really regretted that there wasn't an hint message to head me towards the solution when I was getting close to it.

Now I hope you'll understand why I suggested this patch. Maybe I'm the only one that ended up in this situation, in this case I'd understand that you would no longer be interested in the patch! However if you still are, I'll be happy to make the modification you asked for.

Yeah probably, I'll fix it along with the other.

Thanks for reviewing this!

Well, it would be nice to have an answer in order to know if I should abandon this patch or not :) Thanks!

I do not think I made any request to change anything, other than what procedurally is necessary in order to record the authorship in the resulting commit correctly. I did question why the new behaviour was an improvement and your response helped me understand the motivation better.

Ah, sorry, I didn't get your response as a conditional "if you like it, I'll work on it further", as we usually take "how deeply does the original proposer of a change believes in it" as a strong hint when we need to decide if it is something worth pursuing [*1*]. I am not so enthused to drop everything else and invest 100% of my time and attention to this change, but I am not opposed to the change being proposed, either. We haven't seen anybody other than us two to speak on the review discussion thread of the previous round, so I do not know about other developers and users.

The usual next step by the author is

* Update and resend the patch(es), taking care of not just correctness of the code but also making sure that the proposed log message reduces the need for those questions asked during the review of the previous round [*2*].

* Wait to see other people who find the change favorable.

* After that, the patch may be picked up, advance to 'next' and then to a future release.

but the author can abandon it at any step. After all it is author's itch and all we can do here on the list is to give encouragement and help in polishing it.

Thanks.

[Footnote]

*1* We do not take it very kindly when somebody says "I am dreaming
this and that change, I think it would be great, and if you promise it will be included in the next version of Git, I'll work on it", and respond with "We do not know how good your change will be until we see it." plus "If a change is so great, we expect you would work on it even only for yourself,its greatness will spread by word of the mouth, many people will yearn for it, and eventually we would come to you begging." A change, in which even the original author does not feel it is worth their time to invest to perfect, has much less chance to be successful.

*2* Reviews on the previous round may have asked "why is this change
needed?" "what is the intended use case?" etc. The proposed log message is the place to explain these. The goal is to make it easier for future readers of "git log" to understand so that they do not need to ask these questions (unlike reviewers who can ask and get answers from the author of the patch, they do not have anybody to ask because the author of the patch may not be around forever).

Thanks a lot for taking the time to explain the usual process, I must confess I'm not used to it, so the confusing discussion, sorry for that.

Be sure that I'll work further on my patch with this in mind!

This is also good to know, I'm still learning for sure.

From: Clement Mabileau <[email protected]>

New git users may want to locally delete remote-tracking branches but don't really understand how they are distinguished from branches by git. Then one may naively try:
git branch -d foo/bar and get a correct error branch foo/bar not found but hard to understand for a newbie, this patch aims to guide one in such case.

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

---
branch: improve error log on branch not found by checking remotes refs

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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1476%2Fctmbl%2Fbranch%2Fdeletion%2Fimprove-error-msg-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1476/ctmbl/branch/deletion/improve-error-msg-v3
Pull-Request: https://github.com/git/git/pull/1476

Range-diff vs v2:

1: eb95695ace2 ! 1: 0774f182b50 branch: improve error log on branch not found by checking remotes refs

     @@
       ## Metadata ##
     -Author: ctmbl <[email protected]>
     +Author: Clement Mabileau <[email protected]>
      
       ## Commit message ##
          branch: improve error log on branch not found by checking remotes refs
      
     +    New git users may want to locally delete remote-tracking branches but
     +    don't really understand how they are distinguished from branches by git.
     +    Then one may naively try:
     +    `git branch -d foo/bar` and get a correct error `branch foo/bar not
     +    found` but hard to understand for a newbie, this patch aims to guide one
     +    in such case.
     +
          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:
     +    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
      
          Signed-off-by: Clement Mabileau <[email protected]>
     @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo
       	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";
     ++	const char *fmt_remotes = "refs/remotes/%s";
       
       	switch (kinds) {
       	case FILTER_REFS_REMOTES:
      -		fmt = "refs/remotes/%s";
     -+		fmt = FMT_REMOTES;
     ++		fmt = fmt_remotes;
       		/* For subsequent UI messages */
       		remote_branch = 1;
       		allowed_interpret = INTERPRET_BRANCH_REMOTE;
     -@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds,
     - 		force = 1;
     - 		break;
     - 	case FILTER_REFS_BRANCHES:
     --		fmt = "refs/heads/%s";
     -+		fmt = FMT_BRANCHES;
     - 		allowed_interpret = INTERPRET_BRANCH_LOCAL;
     - 		break;
     - 	default:
      @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds,
       					| RESOLVE_REF_ALLOW_BAD_NAME,
       					&oid, &flags);
     @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo
      -			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?";
     -+
      +			if (remote_branch) {
     -+				error(_(MISSING_REMOTE_REF_ERROR_MSG), bname.buf);
     ++				error(_("remote-tracking branch '%s' not found."), bname.buf);
      +			} else {
     -+				char* virtual_name = mkpathdup(FMT_REMOTES, bname.buf);
     -+				char* virtual_target = resolve_refdup(virtual_name,
     ++				char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
     ++				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);
     ++
      +				if (virtual_target)
     -+					error(_(MISSING_BRANCH_HINT_MSG), bname.buf);
     ++					error(_("branch '%s' not found.\n"
     ++						"Did you forget --remote?"),
     ++						bname.buf);
      +				else
     -+					error(_(MISSING_BRANCH_ERROR_MSG), bname.buf);
     ++					error(_("branch '%s' not found."), bname.buf);
     ++				FREE_AND_NULL(virtual_target);
      +			}
       			ret = 1;
       			continue;

 builtin/branch.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

index f63fd45edb9..5f035dd5969 100644
@@ -216,10 +216,11 @@ 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;
+	const char *fmt_remotes = "refs/remotes/%s";
 
 	switch (kinds) {
 	case FILTER_REFS_REMOTES:
-		fmt = "refs/remotes/%s";
+		fmt = fmt_remotes;
 		/* For subsequent UI messages */
 		remote_branch = 1;
 		allowed_interpret = INTERPRET_BRANCH_REMOTE;

@@ -263,9 +264,25 @@ 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);
+			if (remote_branch) {
+				error(_("remote-tracking branch '%s' not found."), bname.buf);
+			} else {
+				char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
+				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);
+
+				if (virtual_target)
+					error(_("branch '%s' not found.\n"
+						"Did you forget --remote?"),
+						bname.buf);
+				else
+					error(_("branch '%s' not found."), bname.buf);
+				FREE_AND_NULL(virtual_target);
+			}
 			ret = 1;
 			continue;
 		}

base-commit: ae73b2c8f1da39c39335ee76a0f95857712c22a7

gitgitgadget