{"thread":{"id":"44426","subject":"[PATCH] branch: remove unused parameter to create_branch()","startedAt":"2016-11-04T15:19:59Z","lastAt":"2016-11-04T16:52:55Z","messageCount":3,"participants":["Tobias Klauser","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"305394","messageId":"20161104151949.13384-1-tklauser@distanz.ch","threadId":"44426","inReplyTo":null,"subject":"[PATCH] branch: remove unused parameter to create_branch()","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-11-04T15:19:49Z","receivedAt":"2016-11-04T15:19:59Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"The name parameter to create_branch() has been unused since commit\n55c4a673070f (\"Prevent force-updating of the current branch\"). Remove\nthe parameter and adjust the callers accordingly. Also remove the\nparameter from the function's documentation comment.\n\nSigned-off-by: Tobias Klauser <tklauser@distanz.ch>\n---\n branch.c           |  3 +--\n branch.h           | 15 +++++++--------\n builtin/branch.c   |  4 ++--\n builtin/checkout.c |  2 +-\n 4 files changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex a5a8dcbd0ed9..0d459b3cfe50 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -228,8 +228,7 @@ N_(\"\\n\"\n \"will track its remote counterpart, you may want to use\\n\"\n \"\\\"git push -u\\\" to set the upstream config as you push.\");\n \n-void create_branch(const char *head,\n-\t\t   const char *name, const char *start_name,\n+void create_branch(const char *name, const char *start_name,\n \t\t   int force, int reflog, int clobber_head,\n \t\t   int quiet, enum branch_track track)\n {\ndiff --git a/branch.h b/branch.h\nindex b2f964933270..8e63d1b6f964 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -4,15 +4,14 @@\n /* Functions for acting on the information about branches. */\n \n /*\n- * Creates a new branch, where head is the branch currently checked\n- * out, name is the new branch name, start_name is the name of the\n- * existing branch that the new branch should start from, force\n- * enables overwriting an existing (non-head) branch, reflog creates a\n- * reflog for the branch, and track causes the new branch to be\n- * configured to merge the remote branch that start_name is a tracking\n- * branch for (if any).\n+ * Creates a new branch, where name is the new branch name, start_name\n+ * is the name of the existing branch that the new branch should start\n+ * from, force enables overwriting an existing (non-head) branch, reflog\n+ * creates a reflog for the branch, and track causes the new branch to\n+ * be configured to merge the remote branch that start_name is a\n+ * tracking branch for (if any).\n  */\n-void create_branch(const char *head, const char *name, const char *start_name,\n+void create_branch(const char *name, const char *start_name,\n \t\t   int force, int reflog,\n \t\t   int clobber_head, int quiet, enum branch_track track);\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d5d93a8c03fe..60cc5c8e8da0 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -807,7 +807,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t * create_branch takes care of setting up the tracking\n \t\t * info and making sure new_upstream is correct\n \t\t */\n-\t\tcreate_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);\n+\t\tcreate_branch(branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);\n \t} else if (unset_upstream) {\n \t\tstruct branch *branch = branch_get(argv[0]);\n \t\tstruct strbuf buf = STRBUF_INIT;\n@@ -853,7 +853,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \n \t\tbranch_existed = ref_exists(branch->refname);\n-\t\tcreate_branch(head, argv[0], (argc == 2) ? argv[1] : head,\n+\t\tcreate_branch(argv[0], (argc == 2) ? argv[1] : head,\n \t\t\t      force, reflog, 0, quiet, track);\n \n \t\t/*\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9b2a5b31d423..512492aad909 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -630,7 +630,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t}\n \t\t}\n \t\telse\n-\t\t\tcreate_branch(old->name, opts->new_branch, new->name,\n+\t\t\tcreate_branch(opts->new_branch, new->name,\n \t\t\t\t      opts->new_branch_force ? 1 : 0,\n \t\t\t\t      opts->new_branch_log,\n \t\t\t\t      opts->new_branch_force ? 1 : 0,\n-- \n2.9.0\n\n\n"},{"id":"305396","messageId":"20161104163012.5r3uivnub3bdkqgr@sigill.intra.peff.net","threadId":"44426","inReplyTo":"20161104151949.13384-1-tklauser@distanz.ch","subject":"Re: [PATCH] branch: remove unused parameter to create_branch()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-04T16:30:12Z","receivedAt":"2016-11-04T16:30:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 04, 2016 at 04:19:49PM +0100, Tobias Klauser wrote:\n\n> The name parameter to create_branch() has been unused since commit\n> 55c4a673070f (\"Prevent force-updating of the current branch\"). Remove\n> the parameter and adjust the callers accordingly. Also remove the\n> parameter from the function's documentation comment.\n\nThis seemed eerily familiar, and it turns out I wrote this as a\npreparatory step for a different topic a while back, but never finished\nit.\n\nSo clearly a good change, though we might want to explain a bit more why\nit's correct that the parameter is unused. Here's what I wrote:\n\n  This function used to have the caller pass in the current value of\n  HEAD, in order to make sure we didn't clobber HEAD.  In 55c4a6730,\n  that logic moved to validate_new_branchname(), which just resolves\n  HEAD itself. The parameter to create_branch is now unused.\n\nI also ended up reformatting the documentation comment, but that's\npurely optional. My patch is below for reference. Feel free to grab any\nbits of it that you agree with.\n\n-- >8 --\nSubject: [PATCH] create_branch: drop unused \"head\" parameter\n\nThis function used to have the caller pass in the current\nvalue of HEAD, in order to make sure we didn't clobber HEAD.\nIn 55c4a6730, that logic moved to validate_new_branchname(),\nwhich just resolves HEAD itself. The parameter to\ncreate_branch is now unused.\n\nSince we have to update and re-wrap the docstring describing\nthe parameters anyway, let's take this opportunity to break\nit out into a list, which makes it easier to find the\nparameters.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n branch.c           |  3 +--\n branch.h           | 22 ++++++++++++++--------\n builtin/branch.c   |  4 ++--\n builtin/checkout.c |  2 +-\n 4 files changed, 18 insertions(+), 13 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex a5a8dcbd0..0d459b3cf 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -228,8 +228,7 @@ N_(\"\\n\"\n \"will track its remote counterpart, you may want to use\\n\"\n \"\\\"git push -u\\\" to set the upstream config as you push.\");\n \n-void create_branch(const char *head,\n-\t\t   const char *name, const char *start_name,\n+void create_branch(const char *name, const char *start_name,\n \t\t   int force, int reflog, int clobber_head,\n \t\t   int quiet, enum branch_track track)\n {\ndiff --git a/branch.h b/branch.h\nindex b2f964933..3103eb9ad 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -4,15 +4,21 @@\n /* Functions for acting on the information about branches. */\n \n /*\n- * Creates a new branch, where head is the branch currently checked\n- * out, name is the new branch name, start_name is the name of the\n- * existing branch that the new branch should start from, force\n- * enables overwriting an existing (non-head) branch, reflog creates a\n- * reflog for the branch, and track causes the new branch to be\n- * configured to merge the remote branch that start_name is a tracking\n- * branch for (if any).\n+ * Creates a new branch, where:\n+ *\n+ *   - name is the new branch name\n+ *\n+ *   - start_name is the name of the existing branch that the new branch should\n+ *     start from\n+ *\n+ *   - force enables overwriting an existing (non-head) branch\n+ *\n+ *   - reflog creates a reflog for the branch\n+ *\n+ *   - track causes the new branch to be configured to merge the remote branch\n+ *     that start_name is a tracking branch for (if any).\n  */\n-void create_branch(const char *head, const char *name, const char *start_name,\n+void create_branch(const char *name, const char *start_name,\n \t\t   int force, int reflog,\n \t\t   int clobber_head, int quiet, enum branch_track track);\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d5d93a8c0..60cc5c8e8 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -807,7 +807,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t * create_branch takes care of setting up the tracking\n \t\t * info and making sure new_upstream is correct\n \t\t */\n-\t\tcreate_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);\n+\t\tcreate_branch(branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);\n \t} else if (unset_upstream) {\n \t\tstruct branch *branch = branch_get(argv[0]);\n \t\tstruct strbuf buf = STRBUF_INIT;\n@@ -853,7 +853,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \n \t\tbranch_existed = ref_exists(branch->refname);\n-\t\tcreate_branch(head, argv[0], (argc == 2) ? argv[1] : head,\n+\t\tcreate_branch(argv[0], (argc == 2) ? argv[1] : head,\n \t\t\t      force, reflog, 0, quiet, track);\n \n \t\t/*\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9b2a5b31d..512492aad 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -630,7 +630,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t}\n \t\t}\n \t\telse\n-\t\t\tcreate_branch(old->name, opts->new_branch, new->name,\n+\t\t\tcreate_branch(opts->new_branch, new->name,\n \t\t\t\t      opts->new_branch_force ? 1 : 0,\n \t\t\t\t      opts->new_branch_log,\n \t\t\t\t      opts->new_branch_force ? 1 : 0,\n-- \n2.11.0.rc0.263.g6f44bc3\n\n"},{"id":"305399","messageId":"20161104165236.GC819@distanz.ch","threadId":"44426","inReplyTo":"20161104163012.5r3uivnub3bdkqgr@sigill.intra.peff.net","subject":"Re: [PATCH] branch: remove unused parameter to create_branch()","fromName":"Tobias Klauser","fromEmail":"tklauser@distanz.ch","sentAt":"2016-11-04T16:52:43Z","receivedAt":"2016-11-04T16:52:55Z","isPatch":true,"sender":{"key":"tklauser@distanz.ch","avatar":"https://avatars.githubusercontent.com/u/539708?v=4"},"body":"On 2016-11-04 at 17:30:12 +0100, Jeff King <peff@peff.net> wrote:\n> On Fri, Nov 04, 2016 at 04:19:49PM +0100, Tobias Klauser wrote:\n> \n> > The name parameter to create_branch() has been unused since commit\n> > 55c4a673070f (\"Prevent force-updating of the current branch\"). Remove\n> > the parameter and adjust the callers accordingly. Also remove the\n> > parameter from the function's documentation comment.\n> \n> This seemed eerily familiar, and it turns out I wrote this as a\n> preparatory step for a different topic a while back, but never finished\n> it.\n> \n> So clearly a good change, though we might want to explain a bit more why\n> it's correct that the parameter is unused. Here's what I wrote:\n> \n>   This function used to have the caller pass in the current value of\n>   HEAD, in order to make sure we didn't clobber HEAD.  In 55c4a6730,\n>   that logic moved to validate_new_branchname(), which just resolves\n>   HEAD itself. The parameter to create_branch is now unused.\n\nAh, I didn't know about the history of this parameter. It clearly makes\nsense to explain this in the patch description.\n\n> I also ended up reformatting the documentation comment, but that's\n> purely optional. My patch is below for reference. Feel free to grab any\n> bits of it that you agree with.\n\nI like your documentation comment much better as IMO it's easier to read\nand to identify the individual parameters.\n\nGiven these facts, I guess it's better if my patch is dropped and yours\nis applied instead :)\n\nThanks a lot!\n"}]}