{"thread":{"id":"49487","subject":"[PATCH] branch: trivial style fix","startedAt":"2018-10-05T09:54:03Z","lastAt":"2018-10-16T15:33:25Z","messageCount":9,"participants":["Tao Qingyun","Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"359663","messageId":"20181005095213.12509-1-taoqy@ls-a.me","threadId":"49487","inReplyTo":null,"subject":"[PATCH] branch: trivial style fix","fromName":"Tao Qingyun","fromEmail":"taoqy@ls-a.me","sentAt":"2018-10-05T09:52:14Z","receivedAt":"2018-10-05T09:54:03Z","isPatch":true,"sender":{"key":"taoqy@ls-a.me","avatar":"https://avatars.githubusercontent.com/u/38895844?v=4"},"body":"---\n builtin/branch.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex c396c41533..52aad0f602 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -222,10 +222,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (!head_rev)\n \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n \t}\n-\tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n+\tfor (i = 0; i < argc; i++) {\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n \n+\t\tstrbuf_reset(&bname);\n \t\tstrbuf_branchname(&bname, argv[i], allowed_interpret);\n \t\tfree(name);\n \t\tname = mkpathdup(fmt, bname.buf);\n@@ -716,8 +717,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n \t\treturn 0;\n-\t}\n-\telse if (edit_description) {\n+\t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n \n-- \n2.19.0\n\n\n\n"},{"id":"359695","messageId":"20181005162152.GA11254@sigill.intra.peff.net","threadId":"49487","inReplyTo":"20181005095213.12509-1-taoqy@ls-a.me","subject":"Re: [PATCH] branch: trivial style fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-05T16:21:53Z","receivedAt":"2018-10-05T16:21:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 05, 2018 at 05:52:14PM +0800, Tao Qingyun wrote:\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index c396c41533..52aad0f602 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -222,10 +222,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n>  \t\tif (!head_rev)\n>  \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n>  \t}\n> -\tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n> +\tfor (i = 0; i < argc; i++) {\n>  \t\tchar *target = NULL;\n>  \t\tint flags = 0;\n>  \n> +\t\tstrbuf_reset(&bname);\n>  \t\tstrbuf_branchname(&bname, argv[i], allowed_interpret);\n>  \t\tfree(name);\n>  \t\tname = mkpathdup(fmt, bname.buf);\n\nThis one isn't just style: it switches the reset from the end of the\nloop to the beginning of the loop. So we'd need to make sure that we are\nnot depending on its contents going into the first iteration, nor coming\nout of the last one.\n\nLooking at the surrounding code, I think it is OK.\n\n> @@ -716,8 +717,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \t\tprint_columns(&output, colopts, NULL);\n>  \t\tstring_list_clear(&output, 0);\n>  \t\treturn 0;\n> -\t}\n> -\telse if (edit_description) {\n> +\t} else if (edit_description) {\n>  \t\tconst char *branch_name;\n>  \t\tstruct strbuf branch_ref = STRBUF_INIT;\n\nAnd this one is obviously correct.\n\n-Peff\n"},{"id":"359698","messageId":"xmqqy3bcwdb4.fsf@gitster-ct.c.googlers.com","threadId":"49487","inReplyTo":"20181005095213.12509-1-taoqy@ls-a.me","subject":"Re: [PATCH] branch: trivial style fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-05T16:47:27Z","receivedAt":"2018-10-05T16:47:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Qingyun <taoqy@ls-a.me> writes:\n\n> ---\n>  builtin/branch.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index c396c41533..52aad0f602 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -222,10 +222,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n>  \t\tif (!head_rev)\n>  \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n>  \t}\n> -\tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n> +\tfor (i = 0; i < argc; i++) {\n>  \t\tchar *target = NULL;\n>  \t\tint flags = 0;\n>  \n> +\t\tstrbuf_reset(&bname);\n\nThis is not \"trivial\" nor \"style fix\", though.\n\nIt is not \"trivial\" because it also changes the behaviour.  Instead\nof resetting the strbuf each time after the loop body runs, the new\ncode resets it before the loop body, even for the 0-th iteration\nwhere the strbuf hasn't even been used at all.  It is not a \"style\nfix\" because we do not have a particular reason to avoid using the\ncomma operator to perform two simple expressions with side effects\ninside a single expression.\n\n>  \t\tstrbuf_branchname(&bname, argv[i], allowed_interpret);\n>  \t\tfree(name);\n>  \t\tname = mkpathdup(fmt, bname.buf);\n> @@ -716,8 +717,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \t\tprint_columns(&output, colopts, NULL);\n>  \t\tstring_list_clear(&output, 0);\n>  \t\treturn 0;\n> -\t}\n> -\telse if (edit_description) {\n> +\t} else if (edit_description) {\n\nThis one is a style fix that is trivial.  It does not change the\nsemantics a bit, and we do prefer \"else if\" to be on the same line\nas the closing \"}\" of the earlier \"if\" that opened the matching \"{\".\n\n>  \t\tconst char *branch_name;\n>  \t\tstruct strbuf branch_ref = STRBUF_INIT;\n"},{"id":"360550","messageId":"20181015193525.GA8349@sigill.intra.peff.net","threadId":"49487","inReplyTo":"87bm87x5z2.fsf@ls-a.me","subject":"Re: [PATCH] branch: trivial style fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-15T19:35:26Z","receivedAt":"2018-10-15T19:35:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 06, 2018 at 08:40:33AM +0800, Tao Qingyun wrote:\n\n> >> -\tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n> >> +\tfor (i = 0; i < argc; i++) {\n> >>  \t\tchar *target = NULL;\n> >>  \t\tint flags = 0;\n> >>  \n> >> +\t\tstrbuf_reset(&bname);\n> >\n> > This is not \"trivial\" nor \"style fix\", though.\n> >\n> > It is not \"trivial\" because it also changes the behaviour.  Instead\n> > of resetting the strbuf each time after the loop body runs, the new\n> > code resets it before the loop body, even for the 0-th iteration\n> > where the strbuf hasn't even been used at all.  It is not a \"style\n> > fix\" because we do not have a particular reason to avoid using the\n> > comma operator to perform two simple expressions with side effects\n> > inside a single expression.\n> >\n> Thank you and Jeff for your explanation. I think I get the point now.\n> \n> The third part of `for` statement is normally for a step. I think it's\n> improve readability even it does nothing in the first iteration.\n> \n> And, should I drop this part and resend the patch? I'm a newbie :).\n\nSorry for the slow reply. For some reason I do not think your message\nhere made it to the list (but I don't see anything obviously wrong with\nit).\n\nAnyway, yes, I think it is worth dropping this hunk and re-sending the\nelse-if style fix as a separate patch (you may choose to re-send this\nhunk as its own patch, too, if you want to argue for its inclusion, but\nthere is no sense in holding the actual style fix hostage).\n\n-Peff\n"},{"id":"360624","messageId":"20181016141405.2022-1-taoqy@ls-a.me","threadId":"49487","inReplyTo":"20181015193525.GA8349@sigill.intra.peff.net","subject":"Re: [PATCH] branch: trivial style fix","fromName":"Tao Qingyun","fromEmail":"taoqy@ls-a.me","sentAt":"2018-10-16T14:14:05Z","receivedAt":"2018-10-16T14:14:16Z","isPatch":true,"sender":{"key":"taoqy@ls-a.me","avatar":"https://avatars.githubusercontent.com/u/38895844?v=4"},"body":">Sorry for the slow reply. For some reason I do not think your message\n>here made it to the list (but I don't see anything obviously wrong with\n>it).\nYes, I cann't send message to the list using my email clinet, I don't \nknow why. The only way I can make it is using `git send-email`(including\nthis message).\n\n\n"},{"id":"360625","messageId":"20181016141919.2268-1-taoqy@ls-a.me","threadId":"49487","inReplyTo":"20181005095213.12509-1-taoqy@ls-a.me","subject":"[PATCH v2] branch: trivial style fix","fromName":"Tao Qingyun","fromEmail":"taoqy@ls-a.me","sentAt":"2018-10-16T14:19:20Z","receivedAt":"2018-10-16T14:19:52Z","isPatch":true,"sender":{"key":"taoqy@ls-a.me","avatar":"https://avatars.githubusercontent.com/u/38895844?v=4"},"body":"Signed-off-by: Tao Qingyun <taoqy@ls-a.me>\n---\n builtin/branch.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex c396c41533..0aa3dac27b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -716,8 +716,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n \t\treturn 0;\n-\t}\n-\telse if (edit_description) {\n+\t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n \n-- \n2.19.0\n\n\n\n"},{"id":"360626","messageId":"CACBZZX5yfjturpWnMoLu_yso+NLS3eOWjSDaRnXunvN3bbS0_w@mail.gmail.com","threadId":"49487","inReplyTo":"20181016141405.2022-1-taoqy@ls-a.me","subject":"Re: [PATCH] branch: trivial style fix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-16T14:27:44Z","receivedAt":"2018-10-16T14:27:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Oct 16, 2018 at 4:16 PM Tao Qingyun <taoqy@ls-a.me> wrote:\n>\n> >Sorry for the slow reply. For some reason I do not think your message\n> >here made it to the list (but I don't see anything obviously wrong with\n> >it).\n> Yes, I cann't send message to the list using my email clinet, I don't\n> know why. The only way I can make it is using `git send-email`(including\n> this message).\n\nIt's almost certainly because your message contains a HTML part. See\nif you can find how to disable that in your mailer and send plain-text\nonly. The vger.kernel.org mailer just refuses all HTML E-Mail as a\nspam heuristic.\n"},{"id":"360633","messageId":"20181016153255.GB29432@sigill.intra.peff.net","threadId":"49487","inReplyTo":"CACBZZX5yfjturpWnMoLu_yso+NLS3eOWjSDaRnXunvN3bbS0_w@mail.gmail.com","subject":"Re: [PATCH] branch: trivial style fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-16T15:32:55Z","receivedAt":"2018-10-16T15:32:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 16, 2018 at 04:27:44PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Oct 16, 2018 at 4:16 PM Tao Qingyun <taoqy@ls-a.me> wrote:\n> >\n> > >Sorry for the slow reply. For some reason I do not think your message\n> > >here made it to the list (but I don't see anything obviously wrong with\n> > >it).\n> > Yes, I cann't send message to the list using my email clinet, I don't\n> > know why. The only way I can make it is using `git send-email`(including\n> > this message).\n> \n> It's almost certainly because your message contains a HTML part. See\n> if you can find how to disable that in your mailer and send plain-text\n> only. The vger.kernel.org mailer just refuses all HTML E-Mail as a\n> spam heuristic.\n\nThat was my guess, too, but the message that I got did not have such a\npart. Weird.\n\n-Peff\n"},{"id":"360634","messageId":"20181016153322.GC29432@sigill.intra.peff.net","threadId":"49487","inReplyTo":"20181016141919.2268-1-taoqy@ls-a.me","subject":"Re: [PATCH v2] branch: trivial style fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-16T15:33:22Z","receivedAt":"2018-10-16T15:33:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 16, 2018 at 10:19:20PM +0800, Tao Qingyun wrote:\n\n> Signed-off-by: Tao Qingyun <taoqy@ls-a.me>\n> ---\n>  builtin/branch.c | 3 +--\n>  1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index c396c41533..0aa3dac27b 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -716,8 +716,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \t\tprint_columns(&output, colopts, NULL);\n>  \t\tstring_list_clear(&output, 0);\n>  \t\treturn 0;\n> -\t}\n> -\telse if (edit_description) {\n> +\t} else if (edit_description) {\n>  \t\tconst char *branch_name;\n>  \t\tstruct strbuf branch_ref = STRBUF_INIT;\n\nYep, this looks reasonable. Normally I would complain about an empty\ncommit message, but there is really not much else to say. ;)\n\n-Peff\n"}]}