{"thread":{"id":"59557","subject":"[PATCH] coccinelle: add and apply branch_get() rules","startedAt":"2023-04-06T20:38:55Z","lastAt":"2023-04-22T22:30:14Z","messageCount":9,"participants":["Rubén Justo","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474951","messageId":"4cb4b69c-bd14-dfbd-6d06-59a7cd7e8c94@gmail.com","threadId":"59557","inReplyTo":null,"subject":"[PATCH] coccinelle: add and apply branch_get() rules","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-06T20:34:56Z","receivedAt":"2023-04-06T20:38:55Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"There are three supported ways to obtain a \"struct branch *\" for the\ncurrently checked out branch, in the current worktree, using the API\nbranch_get(): branch_get(NULL), branch_get(\"\") and branch_get(\"HEAD\").\n\nThe first one is the recommended [1][2] and optimal usage. Let's add\ntwo coccinelle rules to convert the latter two into the first one.\n\n 1. f019d08ea6 (API documentation for remote.h, 2008-02-19)\n\n 2. d27eb356bf (remote: move doc to remote.h and refspec.h, 2019-11-17)\n\nSigned-off-by: Rubén Justo \n---\n builtin/fetch.c | 2 +-\n builtin/pull.c | 8 ++++----\n contrib/coccinelle/branch_get.cocci | 10 ++++++++++\n 3 files changed, 15 insertions(+), 5 deletions(-)\n create mode 100644 contrib/coccinelle/branch_get.cocci\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7221e57f35..45d81c8e02 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1738,7 +1738,7 @@ static int do_fetch(struct transport *transport,\n \tcommit_fetch_head(&fetch_head);\n \n \tif (set_upstream) {\n-\t\tstruct branch *branch = branch_get(\"HEAD\");\n+\t\tstruct branch *branch = branch_get(NULL);\n \t\tstruct ref *rm;\n \t\tstruct ref *source_ref = NULL;\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 56f679d94a..fbb1cbea0a 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -332,7 +332,7 @@ static const char *config_get_ff(void)\n */\n static enum rebase_type config_get_rebase(int *rebase_unspecified)\n {\n-\tstruct branch *curr_branch = branch_get(\"HEAD\");\n+\tstruct branch *curr_branch = branch_get(NULL);\n \tconst char *value;\n \n \tif (curr_branch) {\n@@ -437,7 +437,7 @@ static int get_only_remote(struct remote *remote, void *cb_data)\n */\n static void NORETURN die_no_merge_candidates(const char *repo, const char **refspecs)\n {\n-\tstruct branch *curr_branch = branch_get(\"HEAD\");\n+\tstruct branch *curr_branch = branch_get(NULL);\n \tconst char *remote = curr_branch ? curr_branch->remote_name : NULL;\n \n \tif (*refspecs) {\n@@ -710,7 +710,7 @@ static const char *get_upstream_branch(const char *remote)\n \tif (!rm)\n \t\treturn NULL;\n \n-\tcurr_branch = branch_get(\"HEAD\");\n+\tcurr_branch = branch_get(NULL);\n \tif (!curr_branch)\n \t\treturn NULL;\n \n@@ -774,7 +774,7 @@ static int get_rebase_fork_point(struct object_id *fork_point, const char *repo,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tcurr_branch = branch_get(\"HEAD\");\n+\tcurr_branch = branch_get(NULL);\n \tif (!curr_branch)\n \t\treturn -1;\n \ndiff --git a/contrib/coccinelle/branch_get.cocci b/contrib/coccinelle/branch_get.cocci\nnew file mode 100644\nindex 0000000000..3ec5b59723\n--- /dev/null\n+++ b/contrib/coccinelle/branch_get.cocci\n@@ -0,0 +1,10 @@\n+@@\n+@@\n+- branch_get(\"HEAD\")\n++ branch_get(NULL)\n+\n+@@\n+@@\n+- branch_get(\"\")\n++ branch_get(NULL)\n+\n-- \n2.34.1\n"},{"id":"475004","messageId":"xmqqjzynlm9i.fsf@gitster.g","threadId":"59557","inReplyTo":"4cb4b69c-bd14-dfbd-6d06-59a7cd7e8c94@gmail.com","subject":"Re: [PATCH] coccinelle: add and apply branch_get() rules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-07T15:55:53Z","receivedAt":"2023-04-07T15:56:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> There are three supported ways to obtain a \"struct branch *\" for the\n> currently checked out branch, in the current worktree, using the API\n> branch_get(): branch_get(NULL), branch_get(\"\") and branch_get(\"HEAD\").\n>\n> The first one is the recommended [1][2] and optimal usage. Let's add\n> two coccinelle rules to convert the latter two into the first one.\n>\n> 1. f019d08ea6 (API documentation for remote.h, 2008-02-19)\n>\n> 2. d27eb356bf (remote: move doc to remote.h and refspec.h, 2019-11-17)\n\nCiting commits in the past is not an optimal way to justify a\nrecommendation, though. It does not show that these recommendations\nmade earlier are still current. Phrasing it this way\n\n Among them, the comment in remote.h for \"struct branch\"\n recommends branch_get(NULL) for HEAD.\n\nmight be a way to say that it comes from the current source, and\nwhen future readers of \"git log\" stumbles on this commit, they will\nalso understand why the change was made.\n\n> diff --git a/contrib/coccinelle/branch_get.cocci b/contrib/coccinelle/branch_get.cocci\n> new file mode 100644\n> index 0000000000..3ec5b59723\n> --- /dev/null\n> +++ b/contrib/coccinelle/branch_get.cocci\n> @@ -0,0 +1,10 @@\n> +@@\n> +@@\n> +- branch_get(\"HEAD\")\n> ++ branch_get(NULL)\n> +@@\n> +@@\n> +- branch_get(\"\")\n> ++ branch_get(NULL)\n> +\n\nI am not sure about these rules. Noybody is passing \"\" to ask for\nHEAD in the current code. Neither\n\n $ git log -S'branch_get(\"\")'\n\nshows anything. The first one does modify existing calls, but there\nare many calls to branch_get() that pass a computed value in a\nstrbuf or a variable. Do we know they are not passing \"HEAD\" or \"\"?\n\nStepping back a bit. What is the ultimate goal for this change?\n\nAre we going to insist that the currently checked out branch MUST be\nasked for by passing NULL and not \"\" or \"HEAD\" to branch_get()? If\nthat is the goal, then it almost makes me wonder if we should just\ndo the attached patch, plus your changes to the callers, without\nadding any new Coccinelle rules, and finding and fixing the fallouts\nby inspecting all the call graph that leads to branch_get().\n\nIf that is not the goal, and we will keep acepting NULL, \"\", and \"HEAD\"\nas equivalents, the value of updating callers with literal \"HEAD\" to\npass NULL is rather dubious.\n\nTo be fair, I do not think we would not see if \"branch_get(NULL)\nmust be the only way to ask for the current branch\" is a good goal,\nuntil we at least try a little. Maybe during such a conversion that\nstarts by erroring when the function is called with \"HEAD\" or \"\"\n(attached below), we might find that we need to change an existing\ncaller (or many of them) to do something silly like this:\n\n return_type a_caller(const char *branch_name, ...)\n {\n-\tstruct branch *branch = branch_get(branch_name);\n+\tstruct branch *branch;\n+\n+\tif (branch_name &&\n+\t (!strcmp(branch_name, \"HEAD\") || !*branch_name))\n+\t\tbranch = branch_get(NULL);\n+\telse\n+\t\tbranch = branch_get(branch_name);\n\t... use branch_name ...\n\t... use branch ...\n\nin which case we may start doubtint the goal to always use NULL for\n\"HEAD\".\n\nSo, I dunno. The patch does not make anything worse (other than\nadding two extra Coccinelle rules), but to me, the ultimate goal\n(\"where does it want to take us\") is unclear.\n\n remote.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git i/remote.c w/remote.c\nindex 3a831cb530..3788dd3fa0 100644\n--- i/remote.c\n+++ w/remote.c\n@@ -1829,8 +1829,10 @@ struct branch *branch_get(const char *name)\n \tstruct branch *ret;\n \n \tread_config(the_repository);\n-\tif (!name || !*name || !strcmp(name, \"HEAD\"))\n+\tif (!name)\n \t\tret = the_repository->remote_state->current_branch;\n+\telse if (!*name || !strcmp(name, \"HEAD\"))\n+\t\tBUG(\"use NULL for HEAD to call branch_get()\");\n \telse\n \t\tret = make_branch(the_repository->remote_state, name,\n \t\t\t\t strlen(name));\n"},{"id":"475006","messageId":"xmqqa5zjllt4.fsf@gitster.g","threadId":"59557","inReplyTo":"xmqqjzynlm9i.fsf@gitster.g","subject":"Re: [PATCH] coccinelle: add and apply branch_get() rules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-07T16:05:43Z","receivedAt":"2023-04-07T16:06:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> To be fair, I do not think we would not see if \"branch_get(NULL)\n> must be the only way to ask for the current branch\" is a good goal,\n> until we at least try a little. Maybe during such a conversion that\n\nArgh. My double negatives are horrible. What I meant is that we\nmay not know until we try, at least a little bit.\n\nSorry for being a bad writer.\n\n"},{"id":"475017","messageId":"376aca6d-1b09-9bf9-c258-81e8ed2443c2@gmail.com","threadId":"59557","inReplyTo":"xmqqjzynlm9i.fsf@gitster.g","subject":"Re: [PATCH] coccinelle: add and apply branch_get() rules","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-07T19:09:44Z","receivedAt":"2023-04-07T19:10:02Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 07-abr-2023 08:55:53, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > There are three supported ways to obtain a \"struct branch *\" for the\n> > currently checked out branch, in the current worktree, using the API\n> > branch_get(): branch_get(NULL), branch_get(\"\") and branch_get(\"HEAD\").\n> >\n> > The first one is the recommended [1][2] and optimal usage. Let's add\n> > two coccinelle rules to convert the latter two into the first one.\n> >\n> > 1. f019d08ea6 (API documentation for remote.h, 2008-02-19)\n> >\n> > 2. d27eb356bf (remote: move doc to remote.h and refspec.h, 2019-11-17)\n> \n> Citing commits in the past is not an optimal way to justify a\n> recommendation, though.\n\nWell, my intention is to state that the recommendation is not recent.\nPerhaps it is confusing to not state clearly that it is also current.\n\n> > diff --git a/contrib/coccinelle/branch_get.cocci b/contrib/coccinelle/branch_get.cocci\n> > new file mode 100644\n> > index 0000000000..3ec5b59723\n> > --- /dev/null\n> > +++ b/contrib/coccinelle/branch_get.cocci\n> > @@ -0,0 +1,10 @@\n> > +@@\n> > +@@\n> > +- branch_get(\"HEAD\")\n> > ++ branch_get(NULL)\n> > +@@\n> > +@@\n> > +- branch_get(\"\")\n> > ++ branch_get(NULL)\n> > +\n> \n> I am not sure about these rules. Noybody is passing \"\" to ask for\n> HEAD in the current code. Neither\n> \n> $ git log -S'branch_get(\"\")'\n\nI'm not sure if there is any path that might use \"\", but the\nconsideration is there since introduced in cf818348f1 (Report\ninformation on branches from remote.h, 2007-09-10).\n\n> \n> shows anything. The first one does modify existing calls, but there\n> are many calls to branch_get() that pass a computed value in a\n> strbuf or a variable. Do we know they are not passing \"HEAD\" or \"\"?\n> \n> Stepping back a bit. What is the ultimate goal for this change?\n\nOf course, as you pointed out, there are usages where a computed value\nis used, perhaps coming from the user, which might end up specifying\n\"HEAD\". Those usages of branch_get() are not considered here. Not even\nindirect ones.\n\nHaving said that, the goal in this change is to aid following, now and\nin the future, when using a literal with branch_get(), the\nrecommendation we already have. Which, IMHO, is also the optimal usage.\n\nAs a collateral, we save some cycles; either at runtime, avoiding the if\n(!strcmp(\"HEAD\", \"HEAD\")) to the user; or better, at compile time,\nsaving the compiler from optimizing out that strcmp.\n\nI have to admit I have this change in mind, not in the current form, but\nin the same direction, since my patches for builtin/branch.c, a few\nmonths ago. When, reviewing the use of branch_get() I was a bit\nconfused.\n\nThanks.\n"},{"id":"475042","messageId":"xmqqjzymf0wt.fsf@gitster.g","threadId":"59557","inReplyTo":"376aca6d-1b09-9bf9-c258-81e8ed2443c2@gmail.com","subject":"Re: [PATCH] coccinelle: add and apply branch_get() rules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-08T22:45:54Z","receivedAt":"2023-04-08T22:46:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> On 07-abr-2023 08:55:53, Junio C Hamano wrote:\n>> Rubén Justo writes:\n>> \n>> > There are three supported ways to obtain a \"struct branch *\" for the\n>> > currently checked out branch, in the current worktree, using the API\n>> > branch_get(): branch_get(NULL), branch_get(\"\") and branch_get(\"HEAD\").\n>> >\n>> > The first one is the recommended [1][2] and optimal usage. Let's add\n>> > two coccinelle rules to convert the latter two into the first one.\n>> >\n>> > 1. f019d08ea6 (API documentation for remote.h, 2008-02-19)\n>> >\n>> > 2. d27eb356bf (remote: move doc to remote.h and refspec.h, 2019-11-17)\n>> \n>> Citing commits in the past is not an optimal way to justify a\n>> recommendation, though.\n>\n> Well, my intention is to state that the recommendation is not recent.\n> Perhaps it is confusing to not state clearly that it is also current.\n\nNo matter how long ago the recommendation was originally written, we\nshould by default consider that anything that appears in the current\nset of sources is still current.\n\nIf it is stale and there is a better recommendation, you of course\nare welcome to update it, and if you were writing such a patch, it\nmay make sense to explain the situation like:\n\n In the comment for \"struct branch\" in remote.h, we recommend to\n use branch_get(NULL) to find out the branch currently checked\n out. This recommendation dates back to f019d08e (API\n documentation for remote.h, 2008-02-19) in a separate\n documentation but later moved by d27eb356 (remote: move doc to\n remote.h and refspec.h, 2019-11-17) to the current location.\n\n However, the recommendation is out of date because ...\n\nor something. But that is not what this patch is about, is it?\n\nI do not think you really gain anything by showing that they date\nback long time, without making your position clear between \"yes, it\nis a very long-standing tradition and majority of the existing code\nconforms to it\" and \"this ancient recommendation is iffy, and I\nthink it should be updated\".\n\nWhat you need to justify this change is to say that the\nrecommendation _is_ current, anyway, so I do not know why you are\narguing against my suggestion to improve your proposed log message.\n\n>> Stepping back a bit. What is the ultimate goal for this change?\n>\n> Of course, as you pointed out, there are usages where a computed value\n> is used, perhaps coming from the user, which might end up specifying\n> \"HEAD\". Those usages of branch_get() are not considered here. Not even\n> indirect ones.\n\nThat is what I found problematic, because I do not think this\nparticular change will get us closer to the endgame of not feedling\n\"\" or \"HEAD\", if ...\n\n> I have to admit I have this change in mind, not in the current form, but\n> in the same direction, since my patches for builtin/branch.c, a few\n> months ago. When, reviewing the use of branch_get() I was a bit\n> confused.\n\n... it is the ultimate goal. These Coccinelle rules would not help\nus fish out existing callers that receive string \"HEAD\" from their\ncallers and pass them unmodified to call branch_get() and convert\nthem to pass NULL, for example. And for doing something like that\nand encode that into another set of Coccinelle rules, it would take\nauditing more and more indirect callers that reach branch_get(), but\nif we were doing that, we can do the \"passing HEAD or empty is a\nBUG\" patch to protect the function from future callers mistakingly\npassing \"HEAD\" or \"\" without Coccinelle rules that are rather costly\nto the CI.\n\nThe above assumes that it is a good thing to declare that NULL is\nthe only permitted way to ask for the branch currently checked out.\nI am a bit skeptical about that, though.\n"},{"id":"475051","messageId":"d01d9fc8-0112-eae3-0792-1e75912720e2@gmail.com","threadId":"59557","inReplyTo":"xmqqjzymf0wt.fsf@gitster.g","subject":"Re: [PATCH] coccinelle: add and apply branch_get() rules","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-09T07:43:06Z","receivedAt":"2023-04-09T07:47:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 08-abr-2023 15:45:54, Junio C Hamano wrote:\n\n> I do not know why you are\n> arguing against my suggestion to improve your proposed log message.\n\nSorry, that's not my intention. The recommendation still stands and the\nmessage was not clear about it.\n\n> >> Stepping back a bit. What is the ultimate goal for this change?\n> >\n> > Of course, as you pointed out, there are usages where a computed value\n> > is used, perhaps coming from the user, which might end up specifying\n> > \"HEAD\". Those usages of branch_get() are not considered here. Not even\n> > indirect ones.\n> \n> That is what I found problematic, because I do not think this\n> particular change will get us closer to the endgame of not feedling\n> \"\" or \"HEAD\", if ...\n\nThe objective in this patch is to avoid having in the codebase\nbranch_get(\"HEAD\") in favor of branch_get(NULL). Because that's what we\nrecommend and, anyway, a smart compiler is going to optimize out that\nstrcmp with two literals. Therefore, we follow the recommendations and\nsave some compiler effort in the way.\n\nBut, branch_get() cannot stop supporting a computed value that ends\nbeing \"HEAD\", as a way to refer to the current branch. However, maybe\nyou are suggesting so...\n"},{"id":"475490","messageId":"230416.86ildvsyt6.gmgdl@evledraar.gmail.com","threadId":"59557","inReplyTo":"4cb4b69c-bd14-dfbd-6d06-59a7cd7e8c94@gmail.com","subject":"Re: [PATCH] coccinelle: add and apply branch_get() rules","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-16T13:56:56Z","receivedAt":"2023-04-16T14:10:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 06 2023, Rubén Justo wrote:\n\n> There are three supported ways to obtain a \"struct branch *\" for the\n> currently checked out branch, in the current worktree, using the API\n> branch_get(): branch_get(NULL), branch_get(\"\") and branch_get(\"HEAD\").\n>\n> The first one is the recommended [1][2] and optimal usage. Let's add\n> two coccinelle rules to convert the latter two into the first one.\n>\n> 1. f019d08ea6 (API documentation for remote.h, 2008-02-19)\n>\n> 2. d27eb356bf (remote: move doc to remote.h and refspec.h, 2019-11-17)\n\nI wondered why it is that we don't just make passing \"HEAD\" an error,\nand what I thought was the case is why: It's because we use this API\nboth for \"internal\" callers like what you modify below, but also for\npassing e.g. a \"HEAD\" as an argv element directly to the API, and don't\nwant every command-line interface to hardcode the \"HEAD\" == NULL. So\nthat makes sense.\n\nBut do we need to support \"\" at all? changing branch_get() so that we do:\n\n\tif (name && !*name)\n\t\tBUG(\"pass NULL, not \\\"\\\"\");\n\nPasses all our tests, but perhaps we have insufficient coverage.\n\n> Signed-off-by: Rubén Justo \n> ---\n> builtin/fetch.c | 2 +-\n> builtin/pull.c | 8 ++++----\n> contrib/coccinelle/branch_get.cocci | 10 ++++++++++\n\nWe've typically named these rules after the API itself, in this case\nthis is in remote.c, maybe we can just add a remote.cocci?\n\n> 3 files changed, 15 insertions(+), 5 deletions(-)\n> create mode 100644 contrib/coccinelle/branch_get.cocci\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 7221e57f35..45d81c8e02 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1738,7 +1738,7 @@ static int do_fetch(struct transport *transport,\n> \tcommit_fetch_head(&fetch_head);\n> \n> \tif (set_upstream) {\n> -\t\tstruct branch *branch = branch_get(\"HEAD\");\n> +\t\tstruct branch *branch = branch_get(NULL);\n> \t\tstruct ref *rm;\n> \t\tstruct ref *source_ref = NULL;\n\nI wonder if we shouldn't just change all of thes to a new inline helper\nwith a more obvious name, perhaps current_branch()?\n> diff --git a/contrib/coccinelle/branch_get.cocci b/contrib/coccinelle/branch_get.cocci\n> new file mode 100644\n> index 0000000000..3ec5b59723\n> --- /dev/null\n> +++ b/contrib/coccinelle/branch_get.cocci\n> @@ -0,0 +1,10 @@\n> +@@\n> +@@\n> +- branch_get(\"HEAD\")\n> ++ branch_get(NULL)\n> +\n> +@@\n> +@@\n> +- branch_get(\"\")\n> ++ branch_get(NULL)\n> +\n\nYou don't need this duplication, see\ncontrib/coccinelle/the_repository.cocci.\n\nI think this should do the trick, although it's untested:\n\t\n\t@@\n\t@@\n\t branch_get(\n\t(\n\t- \"HEAD\"\n\t+ NULL\n\t|\n\t- \"\"\n\t+ NULL\n\t)\n\t )\n\t\nA rule structured like that makes it clear that we're not changing the\nname, but just the argument.\n\n"},{"id":"475496","messageId":"c544be9f-896b-29d5-be26-fb7bcd0f2fc5@gmail.com","threadId":"59557","inReplyTo":"230416.86ildvsyt6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] coccinelle: add and apply branch_get() rules","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-16T23:26:08Z","receivedAt":"2023-04-16T23:28:23Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 16-abr-2023 15:56:56, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Apr 06 2023, Rubén Justo wrote:\n> \n> > There are three supported ways to obtain a \"struct branch *\" for the\n> > currently checked out branch, in the current worktree, using the API\n> > branch_get(): branch_get(NULL), branch_get(\"\") and branch_get(\"HEAD\").\n> >\n> > The first one is the recommended [1][2] and optimal usage. Let's add\n> > two coccinelle rules to convert the latter two into the first one.\n> >\n> > 1. f019d08ea6 (API documentation for remote.h, 2008-02-19)\n> >\n> > 2. d27eb356bf (remote: move doc to remote.h and refspec.h, 2019-11-17)\n> \n> I wondered why it is that we don't just make passing \"HEAD\" an error,\n> and what I thought was the case is why: It's because we use this API\n> both for \"internal\" callers like what you modify below, but also for\n> passing e.g. a \"HEAD\" as an argv element directly to the API, and don't\n> want every command-line interface to hardcode the \"HEAD\" == NULL. So\n> that makes sense.\n> \n> But do we need to support \"\" at all? changing branch_get() so that we do:\n> \n> \tif (name && !*name)\n> \t\tBUG(\"pass NULL, not \\\"\\\"\");\n> \n> Passes all our tests, but perhaps we have insufficient coverage.\n\nI haven't found a use case where it is used, but the consideration is\nthere since it was introduced in cf818348f1 (Report information on\nbranches from remote.h, 2007-09-10).\n\nWe can introduce that BUG() and see what happens. But I'm not sure the\nchange is worth the potential noise.\n\n> \n> > Signed-off-by: Rubén Justo \n> > ---\n> > builtin/fetch.c | 2 +-\n> > builtin/pull.c | 8 ++++----\n> > contrib/coccinelle/branch_get.cocci | 10 ++++++++++\n> \n> We've typically named these rules after the API itself, in this case\n> this is in remote.c, maybe we can just add a remote.cocci?\n\nIs the new rule worth the cost in the CI? That's what I'm thinking\nabout now. Maybe adding the rule to the message for future reference is\nenough.\n\n> \n> > 3 files changed, 15 insertions(+), 5 deletions(-)\n> > create mode 100644 contrib/coccinelle/branch_get.cocci\n> >\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index 7221e57f35..45d81c8e02 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -1738,7 +1738,7 @@ static int do_fetch(struct transport *transport,\n> > \tcommit_fetch_head(&fetch_head);\n> > \n> > \tif (set_upstream) {\n> > -\t\tstruct branch *branch = branch_get(\"HEAD\");\n> > +\t\tstruct branch *branch = branch_get(NULL);\n> > \t\tstruct ref *rm;\n> > \t\tstruct ref *source_ref = NULL;\n> \n> I wonder if we shouldn't just change all of thes to a new inline helper\n> with a more obvious name, perhaps current_branch()?\n\nI had the same idea and explored it a bit. I ended up thinking that I\nwas introducing a _new_ way of using the API. So, I dunno.\n\n> > diff --git a/contrib/coccinelle/branch_get.cocci b/contrib/coccinelle/branch_get.cocci\n> > new file mode 100644\n> > index 0000000000..3ec5b59723\n> > --- /dev/null\n> > +++ b/contrib/coccinelle/branch_get.cocci\n> > @@ -0,0 +1,10 @@\n> > +@@\n> > +@@\n> > +- branch_get(\"HEAD\")\n> > ++ branch_get(NULL)\n> > +\n> > +@@\n> > +@@\n> > +- branch_get(\"\")\n> > ++ branch_get(NULL)\n> > +\n> \n> You don't need this duplication, see\n> contrib/coccinelle/the_repository.cocci.\n> \n> I think this should do the trick, although it's untested:\n> \t\n> \t@@\n> \t@@\n> \t branch_get(\n> \t(\n> \t- \"HEAD\"\n> \t+ NULL\n> \t|\n> \t- \"\"\n> \t+ NULL\n> \t)\n> \t )\n> \t\n> A rule structured like that makes it clear that we're not changing the\n> name, but just the argument.\n> \n\nThanks.\n"},{"id":"475885","messageId":"456b2a6f-eedb-a75a-2299-06ee6e7f3a47@gmail.com","threadId":"59557","inReplyTo":"4cb4b69c-bd14-dfbd-6d06-59a7cd7e8c94@gmail.com","subject":"[PATCH v2] follow usage recommendations for branch_get()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-04-22T22:27:24Z","receivedAt":"2023-04-22T22:30:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Our recommendation is to use branch_get(NULL) to obtain the 'struct\nbranch*' for the currently checked out branch in the current worktree.\nWhile branch_get(\"HEAD\") produces the same result, it does not follow\nthe recommended usage and may cause confusion.\n\nLet's change some calls to branch_get() we currently have in our\ncodebase that do not follow the recommendation, applying the following\nsemantic patch:\n\n @@\n @@\n - branch_get(\"HEAD\")\n + branch_get(NULL)\n\nSigned-off-by: Rubén Justo \n---\n builtin/fetch.c | 2 +-\n builtin/pull.c | 8 ++++----\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ab623f41b4..3c1806aae9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1755,7 +1755,7 @@ static int do_fetch(struct transport *transport,\n \tcommit_fetch_head(&fetch_head);\n \n \tif (set_upstream) {\n-\t\tstruct branch *branch = branch_get(\"HEAD\");\n+\t\tstruct branch *branch = branch_get(NULL);\n \t\tstruct ref *rm;\n \t\tstruct ref *source_ref = NULL;\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 967368ebc6..f93e8610e0 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -335,7 +335,7 @@ static const char *config_get_ff(void)\n */\n static enum rebase_type config_get_rebase(int *rebase_unspecified)\n {\n-\tstruct branch *curr_branch = branch_get(\"HEAD\");\n+\tstruct branch *curr_branch = branch_get(NULL);\n \tconst char *value;\n \n \tif (curr_branch) {\n@@ -440,7 +440,7 @@ static int get_only_remote(struct remote *remote, void *cb_data)\n */\n static void NORETURN die_no_merge_candidates(const char *repo, const char **refspecs)\n {\n-\tstruct branch *curr_branch = branch_get(\"HEAD\");\n+\tstruct branch *curr_branch = branch_get(NULL);\n \tconst char *remote = curr_branch ? curr_branch->remote_name : NULL;\n \n \tif (*refspecs) {\n@@ -713,7 +713,7 @@ static const char *get_upstream_branch(const char *remote)\n \tif (!rm)\n \t\treturn NULL;\n \n-\tcurr_branch = branch_get(\"HEAD\");\n+\tcurr_branch = branch_get(NULL);\n \tif (!curr_branch)\n \t\treturn NULL;\n \n@@ -777,7 +777,7 @@ static int get_rebase_fork_point(struct object_id *fork_point, const char *repo,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tcurr_branch = branch_get(\"HEAD\");\n+\tcurr_branch = branch_get(NULL);\n \tif (!curr_branch)\n \t\treturn -1;\n \n-- \n2.34.1\n"}]}