{"thread":{"id":"59434","subject":"[PATCH 0/2] branch: improve error log on branch not found by checking remotes refs","startedAt":"2023-03-22T09:42:15Z","lastAt":"2023-04-05T11:43:59Z","messageCount":13,"participants":["ClementMabileau via GitGitGadget","ctmbl via GitGitGadget","Junio C Hamano","Clement Mabileau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"473900","messageId":"pull.1476.git.git.1679478126.gitgitgadget@gmail.com","threadId":"59434","inReplyTo":null,"subject":"[PATCH 0/2] branch: improve error log on branch not found by checking remotes refs","fromName":"ClementMabileau via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T09:42:04Z","receivedAt":"2023-03-22T09:42:15Z","isPatch":true,"sender":{"key":"name:ClementMabileau","avatar":null},"body":"when failing to delete a branch with git branch -d because of\nbranch not found, try to find a remote refs matching and if so add\nan hint: Did you forget --remote? to the error message\n\nctmbl (2):\n branch: improve error log on branch not found by checking remotes refs\n Fix mem leak in branch.c due to not-free newly added virtual_name\n variable\n\n builtin/branch.c | 29 ++++++++++++++++++++++++-----\n 1 file changed, 24 insertions(+), 5 deletions(-)\n\n\nbase-commit: 950264636c68591989456e3ba0a5442f93152c1a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1476%2Fctmbl%2Fbranch%2Fdeletion%2Fimprove-error-msg-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1476/ctmbl/branch/deletion/improve-error-msg-v1\nPull-Request: https://github.com/git/git/pull/1476\n-- \ngitgitgadget\n"},{"id":"473901","messageId":"27f27f3afd76fc974350c0c94e20307879eead84.1679478126.git.gitgitgadget@gmail.com","threadId":"59434","inReplyTo":"pull.1476.git.git.1679478126.gitgitgadget@gmail.com","subject":"[PATCH 2/2] Fix mem leak in branch.c due to not-free newly added virtual_name variable","fromName":"ctmbl via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T09:42:06Z","receivedAt":"2023-03-22T09:42:18Z","isPatch":true,"sender":{"key":"name:ctmbl","avatar":null},"body":"From: ctmbl \n\nPrevious commit introduced virtual_name variable which is the name of\nthe branch in case it has been a remote ref (used to check whether the\nuser simply forgot `--remote` flag) but didn't free it.\nCall FREE_AND_NULL(virtual_name) to solve it.\n\nSigned-off-by: Clement Mabileau \n---\n builtin/branch.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 8e768761b9b..697636e2874 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -279,6 +279,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\t\t\t\t| RESOLVE_REF_NO_RECURSE\n \t\t\t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n \t\t\t\t\t\t\t&oid, &flags);\n+\t\t\t\tFREE_AND_NULL(virtual_name);\n \t\t\t\tif (virtual_target)\n \t\t\t\t\terror(_(MISSING_BRANCH_HINT_MSG), bname.buf);\n \t\t\t\telse\n-- \ngitgitgadget\n"},{"id":"473902","messageId":"91cb506968a11fbceba856a40b14bed0c4e647bc.1679478126.git.gitgitgadget@gmail.com","threadId":"59434","inReplyTo":"pull.1476.git.git.1679478126.gitgitgadget@gmail.com","subject":"[PATCH 1/2] branch: improve error log on branch not found by checking remotes refs","fromName":"ctmbl via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T09:42:05Z","receivedAt":"2023-03-22T09:42:20Z","isPatch":true,"sender":{"key":"name:ctmbl","avatar":null},"body":"From: ctmbl \n\nwhen failing to delete a branch with `git branch -d ` because\nof branch not found, try to find a remote refs matching ``\nand if so add an hint:\n`Did you forget --remote?` to the error message\n\nSigned-off-by: Clement Mabileau \n---\n builtin/branch.c | 28 +++++++++++++++++++++++-----\n 1 file changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb9..8e768761b9b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -216,10 +216,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *item;\n \tint branch_name_pos;\n+\tchar* FMT_REMOTES = \"refs/remotes/%s\";\n+\tchar* FMT_BRANCHES = \"refs/heads/%s\";\n \n \tswitch (kinds) {\n \tcase FILTER_REFS_REMOTES:\n-\t\tfmt = \"refs/remotes/%s\";\n+\t\tfmt = FMT_REMOTES;\n \t\t/* For subsequent UI messages */\n \t\tremote_branch = 1;\n \t\tallowed_interpret = INTERPRET_BRANCH_REMOTE;\n@@ -227,7 +229,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tforce = 1;\n \t\tbreak;\n \tcase FILTER_REFS_BRANCHES:\n-\t\tfmt = \"refs/heads/%s\";\n+\t\tfmt = FMT_BRANCHES;\n \t\tallowed_interpret = INTERPRET_BRANCH_LOCAL;\n \t\tbreak;\n \tdefault:\n@@ -263,9 +265,25 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n \t\t\t\t\t&oid, &flags);\n \t\tif (!target) {\n-\t\t\terror(remote_branch\n-\t\t\t ? _(\"remote-tracking branch '%s' not found.\")\n-\t\t\t : _(\"branch '%s' not found.\"), bname.buf);\n+\t\t\tchar* MISSING_REMOTE_REF_ERROR_MSG = \"remote-tracking branch '%s' not found.\";\n+\t\t\tchar* MISSING_BRANCH_ERROR_MSG = \"branch '%s' not found.\";\n+\t\t\tchar* MISSING_BRANCH_HINT_MSG = \"branch '%s' not found.\\n\"\n+\t\t\t\t\t\t\t\t\t\t\t\"Did you forget --remote?\";\n+\n+\t\t\tif (remote_branch) {\n+\t\t\t\terror(_(MISSING_REMOTE_REF_ERROR_MSG), bname.buf);\n+\t\t\t} else {\n+\t\t\t\tchar* virtual_name = mkpathdup(FMT_REMOTES, bname.buf);\n+\t\t\t\tchar* virtual_target = resolve_refdup(virtual_name,\n+\t\t\t\t\t\t\tRESOLVE_REF_READING\n+\t\t\t\t\t\t\t| RESOLVE_REF_NO_RECURSE\n+\t\t\t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n+\t\t\t\t\t\t\t&oid, &flags);\n+\t\t\t\tif (virtual_target)\n+\t\t\t\t\terror(_(MISSING_BRANCH_HINT_MSG), bname.buf);\n+\t\t\t\telse\n+\t\t\t\t\terror(_(MISSING_BRANCH_ERROR_MSG), bname.buf);\n+\t\t\t}\n \t\t\tret = 1;\n \t\t\tcontinue;\n \t\t}\n-- \ngitgitgadget\n\n"},{"id":"473933","messageId":"xmqqmt44enj3.fsf@gitster.g","threadId":"59434","inReplyTo":"27f27f3afd76fc974350c0c94e20307879eead84.1679478126.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] Fix mem leak in branch.c due to not-free newly added virtual_name variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-22T16:52:48Z","receivedAt":"2023-03-22T16:52:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ctmbl via GitGitGadget\" writes:\n\n> From: ctmbl \n>\n> Previous commit introduced virtual_name variable which is the name of\n> the branch in case it has been a remote ref (used to check whether the\n> user simply forgot `--remote` flag) but didn't free it.\n> Call FREE_AND_NULL(virtual_name) to solve it.\n\nDo not introduce a bug in 1/2 and fix it in 2/2, unless it is\nabsolutely necessary.\n\nSquash this one into the previous commit, before sending your\npatches out to the list. The public do not have to learn your\nearlier mistakes, and before sending your patches out is the perfect\nchance for you to pretend to be a person that is more perfect than\nyou are ;-).\n\nThanks.\n"},{"id":"473957","messageId":"bd5207a0-9783-6a15-93da-0904ecd21341@gmail.com","threadId":"59434","inReplyTo":"xmqqmt44enj3.fsf@gitster.g","subject":"Re: [PATCH 2/2] Fix mem leak in branch.c due to not-free newly added virtual_name variable","fromName":"Clement Mabileau","fromEmail":"mabileau.clement@gmail.com","sentAt":"2023-03-22T20:00:54Z","receivedAt":"2023-03-22T20:07:01Z","isPatch":true,"sender":{"key":"mabileau.clement@gmail.com","avatar":"https://avatars.githubusercontent.com/u/79016298?v=4"},"body":"\nOn 22/03/2023 17:52, Junio C Hamano wrote:\n> \"ctmbl via GitGitGadget\" writes:\n> \n>> From: ctmbl \n>>\n>> Previous commit introduced virtual_name variable which is the name of\n>> the branch in case it has been a remote ref (used to check whether the\n>> user simply forgot `--remote` flag) but didn't free it.\n>> Call FREE_AND_NULL(virtual_name) to solve it.\n> \n> Do not introduce a bug in 1/2 and fix it in 2/2, unless it is\n> absolutely necessary.\n\nI didn't know this practice, thanks for letting me know :-)\n\n> Squash this one into the previous commit, before sending your\n> patches out to the list. The public do not have to learn your\n> earlier mistakes, and before sending your patches out is the perfect\n> chance for you to pretend to be a person that is more perfect than\n> you are ;-).\n\nI just did it!\n\nThanks for the review.\n\n"},{"id":"473959","messageId":"pull.1476.v2.git.git.1679515402379.gitgitgadget@gmail.com","threadId":"59434","inReplyTo":"pull.1476.git.git.1679478126.gitgitgadget@gmail.com","subject":"[PATCH v2] branch: improve error log on branch not found by checking remotes refs","fromName":"ClementMabileau via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-22T20:03:22Z","receivedAt":"2023-03-22T20:12:25Z","isPatch":true,"sender":{"key":"name:ClementMabileau","avatar":null},"body":"From: ctmbl \n\nwhen failing to delete a branch with `git branch -d ` because\nof branch not found, try to find a remote refs matching ``\nand if so add an hint:\n`Did you forget --remote?` to the error message\n\nSigned-off-by: Clement Mabileau \n---\n branch: improve error log on branch not found by checking remotes refs\n \n when failing to delete a branch with git branch -d because of\n branch not found, try to find a remote refs matching and if so\n add an hint: Did you forget --remote? to the error message\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1476%2Fctmbl%2Fbranch%2Fdeletion%2Fimprove-error-msg-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1476/ctmbl/branch/deletion/improve-error-msg-v2\nPull-Request: https://github.com/git/git/pull/1476\n\nRange-diff vs v1:\n\n 1: 91cb506968a ! 1: eb95695ace2 branch: improve error log on branch not found by checking remotes refs\n @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo\n +\t\t\t\t\t\t\t| RESOLVE_REF_NO_RECURSE\n +\t\t\t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n +\t\t\t\t\t\t\t&oid, &flags);\n ++\t\t\t\tFREE_AND_NULL(virtual_name);\n +\t\t\t\tif (virtual_target)\n +\t\t\t\t\terror(_(MISSING_BRANCH_HINT_MSG), bname.buf);\n +\t\t\t\telse\n 2: 27f27f3afd7 < -: ----------- Fix mem leak in branch.c due to not-free newly added virtual_name variable\n\n\n builtin/branch.c | 29 ++++++++++++++++++++++++-----\n 1 file changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb9..697636e2874 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -216,10 +216,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *item;\n \tint branch_name_pos;\n+\tchar* FMT_REMOTES = \"refs/remotes/%s\";\n+\tchar* FMT_BRANCHES = \"refs/heads/%s\";\n \n \tswitch (kinds) {\n \tcase FILTER_REFS_REMOTES:\n-\t\tfmt = \"refs/remotes/%s\";\n+\t\tfmt = FMT_REMOTES;\n \t\t/* For subsequent UI messages */\n \t\tremote_branch = 1;\n \t\tallowed_interpret = INTERPRET_BRANCH_REMOTE;\n@@ -227,7 +229,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tforce = 1;\n \t\tbreak;\n \tcase FILTER_REFS_BRANCHES:\n-\t\tfmt = \"refs/heads/%s\";\n+\t\tfmt = FMT_BRANCHES;\n \t\tallowed_interpret = INTERPRET_BRANCH_LOCAL;\n \t\tbreak;\n \tdefault:\n@@ -263,9 +265,26 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n \t\t\t\t\t&oid, &flags);\n \t\tif (!target) {\n-\t\t\terror(remote_branch\n-\t\t\t ? _(\"remote-tracking branch '%s' not found.\")\n-\t\t\t : _(\"branch '%s' not found.\"), bname.buf);\n+\t\t\tchar* MISSING_REMOTE_REF_ERROR_MSG = \"remote-tracking branch '%s' not found.\";\n+\t\t\tchar* MISSING_BRANCH_ERROR_MSG = \"branch '%s' not found.\";\n+\t\t\tchar* MISSING_BRANCH_HINT_MSG = \"branch '%s' not found.\\n\"\n+\t\t\t\t\t\t\t\t\t\t\t\"Did you forget --remote?\";\n+\n+\t\t\tif (remote_branch) {\n+\t\t\t\terror(_(MISSING_REMOTE_REF_ERROR_MSG), bname.buf);\n+\t\t\t} else {\n+\t\t\t\tchar* virtual_name = mkpathdup(FMT_REMOTES, bname.buf);\n+\t\t\t\tchar* virtual_target = resolve_refdup(virtual_name,\n+\t\t\t\t\t\t\tRESOLVE_REF_READING\n+\t\t\t\t\t\t\t| RESOLVE_REF_NO_RECURSE\n+\t\t\t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n+\t\t\t\t\t\t\t&oid, &flags);\n+\t\t\t\tFREE_AND_NULL(virtual_name);\n+\t\t\t\tif (virtual_target)\n+\t\t\t\t\terror(_(MISSING_BRANCH_HINT_MSG), bname.buf);\n+\t\t\t\telse\n+\t\t\t\t\terror(_(MISSING_BRANCH_ERROR_MSG), bname.buf);\n+\t\t\t}\n \t\t\tret = 1;\n \t\t\tcontinue;\n \t\t}\n\nbase-commit: 950264636c68591989456e3ba0a5442f93152c1a\n-- \ngitgitgadget\n"},{"id":"473961","messageId":"xmqqv8iscxuq.fsf@gitster.g","threadId":"59434","inReplyTo":"bd5207a0-9783-6a15-93da-0904ecd21341@gmail.com","subject":"Re: [PATCH 2/2] Fix mem leak in branch.c due to not-free newly added virtual_name variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-22T20:52:45Z","receivedAt":"2023-03-22T20:53:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clement Mabileau writes:\n\n> I just did it!\n\nNice to hear.\n\n> Thanks for the review.\n\nWell, I didn't \"review\" anything, though. I just commented on 2/2\nthat will become moot in your updated version, and I didn't even\nlook at 1/2. \n\nThe change(s) as a whole need to be reviewed by somebody.\n\nThanks.\n"},{"id":"473966","messageId":"xmqq355wctjq.fsf@gitster.g","threadId":"59434","inReplyTo":"pull.1476.v2.git.git.1679515402379.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] branch: improve error log on branch not found by checking remotes refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-22T22:25:45Z","receivedAt":"2023-03-22T22:25:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ClementMabileau via GitGitGadget\" writes:\n\n> From: ctmbl \n\nThis \"name
\" should match what is on \"Signed-off-by:\". I\noffhand do not know where GitGitGadget gets the information, but I\nsuspect the original commit object you created records ctmbl as the\nauthor name, not \"Clement Mabileau\"? I can fix it for this single\ntime, but if you plan to contribute to the project in an ongoing\nbasis, you may want to fix your .git/config (or $HOME/.gitconfig)\nwith a \"[user] name = ...\" entry.\n\n\n> when failing to delete a branch with `git branch -d ` because\n> of branch not found, try to find a remote refs matching ``\n> and if so add an hint:\n> `Did you forget --remote?` to the error message\n\n\n\n> builtin/branch.c | 29 ++++++++++++++++++++++++-----\n> 1 file changed, 24 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index f63fd45edb9..697636e2874 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -216,10 +216,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n> \tstruct string_list_item *item;\n> \tint branch_name_pos;\n> +\tchar* FMT_REMOTES = \"refs/remotes/%s\";\n> +\tchar* FMT_BRANCHES = \"refs/heads/%s\";\n\nIn our codebase, the asterisk sticks to the variable, not to the\ntype. cf. Documentation/CodingGuidelines\n\n - When declaring pointers, the star sides with the variable\n name, i.e. \"char *string\", not \"char* string\" or\n \"char * string\". This makes it easier to understand code\n like \"char *string, c;\".\n\n> @@ -263,9 +265,26 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> \t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n> \t\t\t\t\t&oid, &flags);\n> \t\tif (!target) {\n> -\t\t\terror(remote_branch\n> -\t\t\t ? _(\"remote-tracking branch '%s' not found.\")\n> -\t\t\t : _(\"branch '%s' not found.\"), bname.buf);\n> +\t\t\tchar* MISSING_REMOTE_REF_ERROR_MSG = \"remote-tracking branch '%s' not found.\";\n> +\t\t\tchar* MISSING_BRANCH_ERROR_MSG = \"branch '%s' not found.\";\n> +\t\t\tchar* MISSING_BRANCH_HINT_MSG = \"branch '%s' not found.\\n\"\n> +\t\t\t\t\t\t\t\t\t\t\t\"Did you forget --remote?\";\n\nYou'd need to enclose these text literals inside N_() to tell the\ntools that they are to be translated. e.g.\n\n\tchar *remote_ref_error = N_(\"remote-tracking branch '%s' not found.\");\n\nOtherwise, using the variables inside _() would not let the\nmachinery to find translated strings.\n\nHaving said that, I do not see the point of introducing three\nvariables that are hard to read, misleadingly named with all capital\nletters as if they were macros, and each of which is only used once.\nIt does not make the code that computes which message gets used any\neasier to follow (by hiding long text literal), and it does not help\nreusing the same message elsewhere. For that matter, the two new\nvariables we saw above (again in all caps) are also of dubious merit.\n\n\n\n> +\n> +\t\t\tif (remote_branch) {\n> +\t\t\t\terror(_(MISSING_REMOTE_REF_ERROR_MSG), bname.buf);\n> +\t\t\t} else {\n> +\t\t\t\tchar* virtual_name = mkpathdup(FMT_REMOTES, bname.buf);\n\nHmph. bname.buf at this point has what? \n\nIf the user said \"git branch -d frotz\", we would have 'frotz'\nthere, right? Then we apply FMT_REMOTES (Yuck, now I have to go\nscroll up, see that it is set to \"refs/remotes/%s\", and hope or\nverify that its value hasn't been changed in the meantime---in\nshort, don't introduce that variable. A macro might be OK, but I do\nnot see much point here) and we get \"refs/remotes/frotz\" back.\n\nSo, we check \"refs/remotes/frotz\" here ...\n\n> +\t\t\t\tchar* virtual_target = resolve_refdup(virtual_name,\n> +\t\t\t\t\t\t\tRESOLVE_REF_READING\n> +\t\t\t\t\t\t\t| RESOLVE_REF_NO_RECURSE\n> +\t\t\t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n> +\t\t\t\t\t\t\t&oid, &flags);\n> +\t\t\t\tFREE_AND_NULL(virtual_name);\n\n... but why should we? If your workflow interacts with the original\nrepository you cloned from, you would have remote-tracking branches\nlike \"refs/remotes/origin/frotz\" and it may be plausible that you\nmeant to remove with \"git branch -d frotz\" the remote-tracking\nbranch \"refs/remotes/origin/frotz\". But the new code makes no\neffort to figure out the name of the remote (e.g. 'origin') here,\nand I am not sure what value it adds to check and try to tell the\nuser about \"refs/remotes/frotz\". Or are we assuming that the user\nwould say \"git branch -d origin/frotz\" in such a case?\n\n> +\t\t\t\tif (virtual_target)\n> +\t\t\t\t\terror(_(MISSING_BRANCH_HINT_MSG), bname.buf);\n> +\t\t\t\telse\n> +\t\t\t\t\terror(_(MISSING_BRANCH_ERROR_MSG), bname.buf);\n> +\t\t\t}\n\nAre you leaking virtual_target here?\n\n> \t\t\tret = 1;\n> \t\t\tcontinue;\n> \t\t}\n"},{"id":"473992","messageId":"cffce108-c39d-f13f-9fb6-60624f7e7cea@gmail.com","threadId":"59434","inReplyTo":"xmqq355wctjq.fsf@gitster.g","subject":"Re: [PATCH v2] branch: improve error log on branch not found by checking remotes refs","fromName":"Clement Mabileau","fromEmail":"mabileau.clement@gmail.com","sentAt":"2023-03-23T15:51:22Z","receivedAt":"2023-03-23T15:51:40Z","isPatch":true,"sender":{"key":"mabileau.clement@gmail.com","avatar":"https://avatars.githubusercontent.com/u/79016298?v=4"},"body":"\nOn 22/03/2023 23:25, Junio C Hamano wrote:\n> \"ClementMabileau via GitGitGadget\" writes:\n> \n>> From: ctmbl \n> \n> This \"name
\" should match what is on \"Signed-off-by:\". I\n> offhand do not know where GitGitGadget gets the information, but I\n> suspect the original commit object you created records ctmbl as the\n> author name, not \"Clement Mabileau\"? I can fix it for this single\n> time, but if you plan to contribute to the project in an ongoing\n> basis, you may want to fix your .git/config (or $HOME/.gitconfig)\n> with a \"[user] name = ...\" entry.\n\nWell maybe it got it from the original commit or from my GitHub\nusername, I can't know for sure. But sorry for that :/ , I'll make\nsure it's fixed it for potential future contribution!\n\n> If the user said \"git branch -d frotz\", we would have 'frotz'\n> there, right? Then we apply FMT_REMOTES (Yuck, now I have to go\n> scroll up, see that it is set to \"refs/remotes/%s\", and hope or\n> verify that its value hasn't been changed in the meantime---in\n> short, don't introduce that variable. A macro might be OK, but I do\n> not see much point here) and we get \"refs/remotes/frotz\" back.\n> \n> So, we check \"refs/remotes/frotz\" here ...\n> \n>> [...]\n> \n> ... but why should we? If your workflow interacts with the original\n> repository you cloned from, you would have remote-tracking branches\n> like \"refs/remotes/origin/frotz\" and it may be plausible that you\n> meant to remove with \"git branch -d frotz\" the remote-tracking\n> branch \"refs/remotes/origin/frotz\". But the new code makes no\n> effort to figure out the name of the remote (e.g. 'origin') here,\n> and I am not sure what value it adds to check and try to tell the\n> user about \"refs/remotes/frotz\". Or are we assuming that the user\n> would say \"git branch -d origin/frotz\" in such a case?\n\nBefore fixing anything about the code maybe I should first address your\nlast point which is the interest of the patch in the first place (and I\nshould have started with that...).\n\nA few months earlier, for the first time, I had to delete a remote ref\n(because of a fork I fetched but no longer wanted: maybe a designed\nsolution exists but I'm not aware of it). However, despite being used to\ngit I had a hard time figuring out how to do it, I tried different\nthings, one was `git branch -d origin/` (I recently discovered\nthat it was written in `git branch --help` but I didn't find it at the\ntime). Even googling it proved difficult (because of a poor keyword\nchoice I must confess), most results was dealing with deleting remote\nbranches, such as `git push remote :branch`.\nIn the end, I finally understood that I needed that `--remote` flag and\nreally regretted that there wasn't an hint message to head me towards\nthe solution when I was getting close to it.\n\nNow I hope you'll understand why I suggested this patch. Maybe I'm the\nonly one that ended up in this situation, in this case I'd understand\nthat you would no longer be interested in the patch!\nHowever if you still are, I'll be happy to make the modification you\nasked for.\n\n>> +\t\t\t\tif (virtual_target)\n>> +\t\t\t\t\terror(_(MISSING_BRANCH_HINT_MSG), bname.buf);\n>> +\t\t\t\telse\n>> +\t\t\t\t\terror(_(MISSING_BRANCH_ERROR_MSG), bname.buf);\n>> +\t\t\t}\n> \n> Are you leaking virtual_target here?\n\nYeah probably, I'll fix it along with the other.\n\nThanks for reviewing this!\n"},{"id":"474777","messageId":"ff7bb1f4-e35a-66ad-1116-6bb2b906fed3@gmail.com","threadId":"59434","inReplyTo":"cffce108-c39d-f13f-9fb6-60624f7e7cea@gmail.com","subject":"Re: [PATCH v2] branch: improve error log on branch not found by checking remotes refs","fromName":"Clement Mabileau","fromEmail":"mabileau.clement@gmail.com","sentAt":"2023-04-04T13:30:33Z","receivedAt":"2023-04-04T13:30:38Z","isPatch":true,"sender":{"key":"mabileau.clement@gmail.com","avatar":"https://avatars.githubusercontent.com/u/79016298?v=4"},"body":"\n\nOn 3/23/23 16:51, Clement Mabileau wrote:\n>\n> Well maybe it got it from the original commit or from my GitHub\n> username, I can't know for sure. But sorry for that :/ , I'll make\n> sure it's fixed it for potential future contribution!\n>\n> Before fixing anything about the code maybe I should first address your\n> last point which is the interest of the patch in the first place (and I\n> should have started with that...).\n>\n> A few months earlier, for the first time, I had to delete a remote ref\n> (because of a fork I fetched but no longer wanted: maybe a designed\n> solution exists but I'm not aware of it). However, despite being used to\n> git I had a hard time figuring out how to do it, I tried different\n> things, one was `git branch -d origin/` (I recently discovered\n> that it was written in `git branch --help` but I didn't find it at the\n> time). Even googling it proved difficult (because of a poor keyword\n> choice I must confess), most results was dealing with deleting remote\n> branches, such as `git push remote :branch`.\n> In the end, I finally understood that I needed that `--remote` flag and\n> really regretted that there wasn't an hint message to head me towards\n> the solution when I was getting close to it.\n>\n> Now I hope you'll understand why I suggested this patch. Maybe I'm the\n> only one that ended up in this situation, in this case I'd understand\n> that you would no longer be interested in the patch!\n> However if you still are, I'll be happy to make the modification you\n> asked for.\n>\n>\n> Yeah probably, I'll fix it along with the other.\n>\n> Thanks for reviewing this!\n\nWell, it would be nice to have an answer in order to know if I should \nabandon this patch or not :)\nThanks!\n"},{"id":"474787","messageId":"xmqqmt3n1up1.fsf@gitster.g","threadId":"59434","inReplyTo":"ff7bb1f4-e35a-66ad-1116-6bb2b906fed3@gmail.com","subject":"Re: [PATCH v2] branch: improve error log on branch not found by checking remotes refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-04T16:24:58Z","receivedAt":"2023-04-04T16:25:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clement Mabileau writes:\n\n>> Now I hope you'll understand why I suggested this patch. Maybe I'm the\n>> only one that ended up in this situation, in this case I'd understand\n>> that you would no longer be interested in the patch!\n>> However if you still are, I'll be happy to make the modification you\n>> asked for.\n\nI do not think I made any request to change anything, other than\nwhat procedurally is necessary in order to record the authorship in\nthe resulting commit correctly. I did question why the new behaviour\nwas an improvement and your response helped me understand the\nmotivation better.\n\n> Well, it would be nice to have an answer in order to know if I should\n> abandon this patch or not :)\n\nAh, sorry, I didn't get your response as a conditional \"if you like\nit, I'll work on it further\", as we usually take \"how deeply does\nthe original proposer of a change believes in it\" as a strong hint\nwhen we need to decide if it is something worth pursuing [*1*]. I\nam not so enthused to drop everything else and invest 100% of my\ntime and attention to this change, but I am not opposed to the\nchange being proposed, either. We haven't seen anybody other than\nus two to speak on the review discussion thread of the previous\nround, so I do not know about other developers and users.\n\nThe usual next step by the author is\n\n * Update and resend the patch(es), taking care of not just\n correctness of the code but also making sure that the proposed\n log message reduces the need for those questions asked during the\n review of the previous round [*2*].\n\n * Wait to see other people who find the change favorable.\n\n * After that, the patch may be picked up, advance to 'next' and\n then to a future release.\n\nbut the author can abandon it at any step. After all it is author's\nitch and all we can do here on the list is to give encouragement and\nhelp in polishing it.\n\nThanks.\n\n\n[Footnote]\n\n*1* We do not take it very kindly when somebody says \"I am dreaming\n this and that change, I think it would be great, and if you\n promise it will be included in the next version of Git, I'll\n work on it\", and respond with \"We do not know how good your\n change will be until we see it.\" plus \"If a change is so great,\n we expect you would work on it even only for yourself,its\n greatness will spread by word of the mouth, many people will\n yearn for it, and eventually we would come to you begging.\" A\n change, in which even the original author does not feel it is\n worth their time to invest to perfect, has much less chance to\n be successful.\n\n*2* Reviews on the previous round may have asked \"why is this change\n needed?\" \"what is the intended use case?\" etc. The proposed log\n message is the place to explain these. The goal is to make it\n easier for future readers of \"git log\" to understand so that\n they do not need to ask these questions (unlike reviewers who\n can ask and get answers from the author of the patch, they do\n not have anybody to ask because the author of the patch may not\n be around forever).\n"},{"id":"474831","messageId":"11f9fec6-dbad-4f70-75f3-793eadd62840@gmail.com","threadId":"59434","inReplyTo":"xmqqmt3n1up1.fsf@gitster.g","subject":"Re: [PATCH v2] branch: improve error log on branch not found by checking remotes refs","fromName":"Clement Mabileau","fromEmail":"mabileau.clement@gmail.com","sentAt":"2023-04-05T09:15:58Z","receivedAt":"2023-04-05T09:16:01Z","isPatch":true,"sender":{"key":"mabileau.clement@gmail.com","avatar":"https://avatars.githubusercontent.com/u/79016298?v=4"},"body":"\n\nOn 4/4/23 18:24, Junio C Hamano wrote:\n> Clement Mabileau writes:\n>\n> Ah, sorry, I didn't get your response as a conditional \"if you like\n> it, I'll work on it further\", as we usually take \"how deeply does\n> the original proposer of a change believes in it\" as a strong hint\n> when we need to decide if it is something worth pursuing [*1*]. I\n> am not so enthused to drop everything else and invest 100% of my\n> time and attention to this change, but I am not opposed to the\n> change being proposed, either. We haven't seen anybody other than\n> us two to speak on the review discussion thread of the previous\n> round, so I do not know about other developers and users.\n\nThanks a lot for taking the time to explain the usual process, I must \nconfess I'm not used to it, so the confusing discussion, sorry for that.\n\n> The usual next step by the author is\n>\n> * Update and resend the patch(es), taking care of not just\n> correctness of the code but also making sure that the proposed\n> log message reduces the need for those questions asked during the\n> review of the previous round [*2*].\n>\n> * Wait to see other people who find the change favorable.\n>\n> * After that, the patch may be picked up, advance to 'next' and\n> then to a future release.\n>\n> but the author can abandon it at any step. After all it is author's\n> itch and all we can do here on the list is to give encouragement and\n> help in polishing it.\n>\n> Thanks.\n> [Footnote]\n>\n> *1* We do not take it very kindly when somebody says \"I am dreaming\n> this and that change, I think it would be great, and if you\n> promise it will be included in the next version of Git, I'll\n> work on it\", and respond with \"We do not know how good your\n> change will be until we see it.\" plus \"If a change is so great,\n> we expect you would work on it even only for yourself,its\n> greatness will spread by word of the mouth, many people will\n> yearn for it, and eventually we would come to you begging.\" A\n> change, in which even the original author does not feel it is\n> worth their time to invest to perfect, has much less chance to\n> be successful.\n\nBe sure that I'll work further on my patch with this in mind!\n\n> *2* Reviews on the previous round may have asked \"why is this change\n> needed?\" \"what is the intended use case?\" etc. The proposed log\n> message is the place to explain these. The goal is to make it\n> easier for future readers of \"git log\" to understand so that\n> they do not need to ask these questions (unlike reviewers who\n> can ask and get answers from the author of the patch, they do\n> not have anybody to ask because the author of the patch may not\n> be around forever).\nThis is also good to know, I'm still learning for sure.\n"},{"id":"474837","messageId":"pull.1476.v3.git.git.1680695000257.gitgitgadget@gmail.com","threadId":"59434","inReplyTo":"pull.1476.v2.git.git.1679515402379.gitgitgadget@gmail.com","subject":"[PATCH v3] branch: improve error log on branch not found by checking remotes refs","fromName":"ClementMabileau via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-05T11:43:20Z","receivedAt":"2023-04-05T11:43:59Z","isPatch":true,"sender":{"key":"name:ClementMabileau","avatar":null},"body":"From: Clement Mabileau \n\nNew git users may want to locally delete remote-tracking branches but\ndon't really understand how they are distinguished from branches by git.\nThen one may naively try:\n`git branch -d foo/bar` and get a correct error `branch foo/bar not\nfound` but hard to understand for a newbie, this patch aims to guide one\nin such case.\n\nwhen failing to delete a branch with `git branch -d ` because\nof branch not found, try to find a **remote refs** matching ``\nand if so, add an hint:\n`Did you forget --remote?` to the error message\n\nSigned-off-by: Clement Mabileau \n---\n branch: improve error log on branch not found by checking remotes refs\n \n when failing to delete a branch with git branch -d because of\n branch not found, try to find a remote refs matching and if so\n add an hint: Did you forget --remote? to the error message\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1476%2Fctmbl%2Fbranch%2Fdeletion%2Fimprove-error-msg-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1476/ctmbl/branch/deletion/improve-error-msg-v3\nPull-Request: https://github.com/git/git/pull/1476\n\nRange-diff vs v2:\n\n 1: eb95695ace2 ! 1: 0774f182b50 branch: improve error log on branch not found by checking remotes refs\n @@\n ## Metadata ##\n -Author: ctmbl \n +Author: Clement Mabileau \n \n ## Commit message ##\n branch: improve error log on branch not found by checking remotes refs\n \n + New git users may want to locally delete remote-tracking branches but\n + don't really understand how they are distinguished from branches by git.\n + Then one may naively try:\n + `git branch -d foo/bar` and get a correct error `branch foo/bar not\n + found` but hard to understand for a newbie, this patch aims to guide one\n + in such case.\n +\n when failing to delete a branch with `git branch -d ` because\n - of branch not found, try to find a remote refs matching ``\n - and if so add an hint:\n + of branch not found, try to find a **remote refs** matching ``\n + and if so, add an hint:\n `Did you forget --remote?` to the error message\n \n Signed-off-by: Clement Mabileau \n @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *item;\n \tint branch_name_pos;\n -+\tchar* FMT_REMOTES = \"refs/remotes/%s\";\n -+\tchar* FMT_BRANCHES = \"refs/heads/%s\";\n ++\tconst char *fmt_remotes = \"refs/remotes/%s\";\n \n \tswitch (kinds) {\n \tcase FILTER_REFS_REMOTES:\n -\t\tfmt = \"refs/remotes/%s\";\n -+\t\tfmt = FMT_REMOTES;\n ++\t\tfmt = fmt_remotes;\n \t\t/* For subsequent UI messages */\n \t\tremote_branch = 1;\n \t\tallowed_interpret = INTERPRET_BRANCH_REMOTE;\n -@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds,\n - \t\tforce = 1;\n - \t\tbreak;\n - \tcase FILTER_REFS_BRANCHES:\n --\t\tfmt = \"refs/heads/%s\";\n -+\t\tfmt = FMT_BRANCHES;\n - \t\tallowed_interpret = INTERPRET_BRANCH_LOCAL;\n - \t\tbreak;\n - \tdefault:\n @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n \t\t\t\t\t&oid, &flags);\n @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo\n -\t\t\terror(remote_branch\n -\t\t\t ? _(\"remote-tracking branch '%s' not found.\")\n -\t\t\t : _(\"branch '%s' not found.\"), bname.buf);\n -+\t\t\tchar* MISSING_REMOTE_REF_ERROR_MSG = \"remote-tracking branch '%s' not found.\";\n -+\t\t\tchar* MISSING_BRANCH_ERROR_MSG = \"branch '%s' not found.\";\n -+\t\t\tchar* MISSING_BRANCH_HINT_MSG = \"branch '%s' not found.\\n\"\n -+\t\t\t\t\t\t\t\t\t\t\t\"Did you forget --remote?\";\n -+\n +\t\t\tif (remote_branch) {\n -+\t\t\t\terror(_(MISSING_REMOTE_REF_ERROR_MSG), bname.buf);\n ++\t\t\t\terror(_(\"remote-tracking branch '%s' not found.\"), bname.buf);\n +\t\t\t} else {\n -+\t\t\t\tchar* virtual_name = mkpathdup(FMT_REMOTES, bname.buf);\n -+\t\t\t\tchar* virtual_target = resolve_refdup(virtual_name,\n ++\t\t\t\tchar *virtual_name = mkpathdup(fmt_remotes, bname.buf);\n ++\t\t\t\tchar *virtual_target = resolve_refdup(virtual_name,\n +\t\t\t\t\t\t\tRESOLVE_REF_READING\n +\t\t\t\t\t\t\t| RESOLVE_REF_NO_RECURSE\n +\t\t\t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n +\t\t\t\t\t\t\t&oid, &flags);\n +\t\t\t\tFREE_AND_NULL(virtual_name);\n ++\n +\t\t\t\tif (virtual_target)\n -+\t\t\t\t\terror(_(MISSING_BRANCH_HINT_MSG), bname.buf);\n ++\t\t\t\t\terror(_(\"branch '%s' not found.\\n\"\n ++\t\t\t\t\t\t\"Did you forget --remote?\"),\n ++\t\t\t\t\t\tbname.buf);\n +\t\t\t\telse\n -+\t\t\t\t\terror(_(MISSING_BRANCH_ERROR_MSG), bname.buf);\n ++\t\t\t\t\terror(_(\"branch '%s' not found.\"), bname.buf);\n ++\t\t\t\tFREE_AND_NULL(virtual_target);\n +\t\t\t}\n \t\t\tret = 1;\n \t\t\tcontinue;\n\n\n builtin/branch.c | 25 +++++++++++++++++++++----\n 1 file changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb9..5f035dd5969 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -216,10 +216,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *item;\n \tint branch_name_pos;\n+\tconst char *fmt_remotes = \"refs/remotes/%s\";\n \n \tswitch (kinds) {\n \tcase FILTER_REFS_REMOTES:\n-\t\tfmt = \"refs/remotes/%s\";\n+\t\tfmt = fmt_remotes;\n \t\t/* For subsequent UI messages */\n \t\tremote_branch = 1;\n \t\tallowed_interpret = INTERPRET_BRANCH_REMOTE;\n@@ -263,9 +264,25 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n \t\t\t\t\t&oid, &flags);\n \t\tif (!target) {\n-\t\t\terror(remote_branch\n-\t\t\t ? _(\"remote-tracking branch '%s' not found.\")\n-\t\t\t : _(\"branch '%s' not found.\"), bname.buf);\n+\t\t\tif (remote_branch) {\n+\t\t\t\terror(_(\"remote-tracking branch '%s' not found.\"), bname.buf);\n+\t\t\t} else {\n+\t\t\t\tchar *virtual_name = mkpathdup(fmt_remotes, bname.buf);\n+\t\t\t\tchar *virtual_target = resolve_refdup(virtual_name,\n+\t\t\t\t\t\t\tRESOLVE_REF_READING\n+\t\t\t\t\t\t\t| RESOLVE_REF_NO_RECURSE\n+\t\t\t\t\t\t\t| RESOLVE_REF_ALLOW_BAD_NAME,\n+\t\t\t\t\t\t\t&oid, &flags);\n+\t\t\t\tFREE_AND_NULL(virtual_name);\n+\n+\t\t\t\tif (virtual_target)\n+\t\t\t\t\terror(_(\"branch '%s' not found.\\n\"\n+\t\t\t\t\t\t\"Did you forget --remote?\"),\n+\t\t\t\t\t\tbname.buf);\n+\t\t\t\telse\n+\t\t\t\t\terror(_(\"branch '%s' not found.\"), bname.buf);\n+\t\t\t\tFREE_AND_NULL(virtual_target);\n+\t\t\t}\n \t\t\tret = 1;\n \t\t\tcontinue;\n \t\t}\n\nbase-commit: ae73b2c8f1da39c39335ee76a0f95857712c22a7\n-- \ngitgitgadget\n"}]}