| Message ID | 20181005095213.12509-1-taoqy@ls-a.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | branch: trivial style fix | expand |
On Fri, Oct 05, 2018 at 05:52:14PM +0800, Tao Qingyun wrote: > diff --git a/builtin/branch.c b/builtin/branch.c > index c396c41533..52aad0f602 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -222,10 +222,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > if (!head_rev) > die(_("Couldn't look up commit object for HEAD")); > } > - for (i = 0; i < argc; i++, strbuf_reset(&bname)) { > + for (i = 0; i < argc; i++) { > char *target = NULL; > int flags = 0; > > + strbuf_reset(&bname); > strbuf_branchname(&bname, argv[i], allowed_interpret); > free(name); > name = mkpathdup(fmt, bname.buf); This one isn't just style: it switches the reset from the end of the loop to the beginning of the loop. So we'd need to make sure that we are not depending on its contents going into the first iteration, nor coming out of the last one. Looking at the surrounding code, I think it is OK. > @@ -716,8 +717,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > print_columns(&output, colopts, NULL); > string_list_clear(&output, 0); > return 0; > - } > - else if (edit_description) { > + } else if (edit_description) { > const char *branch_name; > struct strbuf branch_ref = STRBUF_INIT; And this one is obviously correct. -Peff
Tao Qingyun <taoqy@ls-a.me> writes: > --- > builtin/branch.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index c396c41533..52aad0f602 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -222,10 +222,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > if (!head_rev) > die(_("Couldn't look up commit object for HEAD")); > } > - for (i = 0; i < argc; i++, strbuf_reset(&bname)) { > + for (i = 0; i < argc; i++) { > char *target = NULL; > int flags = 0; > > + strbuf_reset(&bname); This is not "trivial" nor "style fix", though. It is not "trivial" because it also changes the behaviour. Instead of resetting the strbuf each time after the loop body runs, the new code resets it before the loop body, even for the 0-th iteration where the strbuf hasn't even been used at all. It is not a "style fix" because we do not have a particular reason to avoid using the comma operator to perform two simple expressions with side effects inside a single expression. > strbuf_branchname(&bname, argv[i], allowed_interpret); > free(name); > name = mkpathdup(fmt, bname.buf); > @@ -716,8 +717,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > print_columns(&output, colopts, NULL); > string_list_clear(&output, 0); > return 0; > - } > - else if (edit_description) { > + } else if (edit_description) { This one is a style fix that is trivial. It does not change the semantics a bit, and we do prefer "else if" to be on the same line as the closing "}" of the earlier "if" that opened the matching "{". > const char *branch_name; > struct strbuf branch_ref = STRBUF_INIT;
On Sat, Oct 06, 2018 at 08:40:33AM +0800, Tao Qingyun wrote: > >> - for (i = 0; i < argc; i++, strbuf_reset(&bname)) { > >> + for (i = 0; i < argc; i++) { > >> char *target = NULL; > >> int flags = 0; > >> > >> + strbuf_reset(&bname); > > > > This is not "trivial" nor "style fix", though. > > > > It is not "trivial" because it also changes the behaviour. Instead > > of resetting the strbuf each time after the loop body runs, the new > > code resets it before the loop body, even for the 0-th iteration > > where the strbuf hasn't even been used at all. It is not a "style > > fix" because we do not have a particular reason to avoid using the > > comma operator to perform two simple expressions with side effects > > inside a single expression. > > > Thank you and Jeff for your explanation. I think I get the point now. > > The third part of `for` statement is normally for a step. I think it's > improve readability even it does nothing in the first iteration. > > And, should I drop this part and resend the patch? I'm a newbie :). Sorry for the slow reply. For some reason I do not think your message here made it to the list (but I don't see anything obviously wrong with it). Anyway, yes, I think it is worth dropping this hunk and re-sending the else-if style fix as a separate patch (you may choose to re-send this hunk as its own patch, too, if you want to argue for its inclusion, but there is no sense in holding the actual style fix hostage). -Peff
>Sorry for the slow reply. For some reason I do not think your message >here made it to the list (but I don't see anything obviously wrong with >it). Yes, I cann't send message to the list using my email clinet, I don't know why. The only way I can make it is using `git send-email`(including this message).
On Tue, Oct 16, 2018 at 4:16 PM Tao Qingyun <taoqy@ls-a.me> wrote: > > >Sorry for the slow reply. For some reason I do not think your message > >here made it to the list (but I don't see anything obviously wrong with > >it). > Yes, I cann't send message to the list using my email clinet, I don't > know why. The only way I can make it is using `git send-email`(including > this message). It's almost certainly because your message contains a HTML part. See if you can find how to disable that in your mailer and send plain-text only. The vger.kernel.org mailer just refuses all HTML E-Mail as a spam heuristic.
On Tue, Oct 16, 2018 at 04:27:44PM +0200, Ævar Arnfjörð Bjarmason wrote: > On Tue, Oct 16, 2018 at 4:16 PM Tao Qingyun <taoqy@ls-a.me> wrote: > > > > >Sorry for the slow reply. For some reason I do not think your message > > >here made it to the list (but I don't see anything obviously wrong with > > >it). > > Yes, I cann't send message to the list using my email clinet, I don't > > know why. The only way I can make it is using `git send-email`(including > > this message). > > It's almost certainly because your message contains a HTML part. See > if you can find how to disable that in your mailer and send plain-text > only. The vger.kernel.org mailer just refuses all HTML E-Mail as a > spam heuristic. That was my guess, too, but the message that I got did not have such a part. Weird. -Peff
diff --git a/builtin/branch.c b/builtin/branch.c index c396c41533..52aad0f602 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -222,10 +222,11 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, if (!head_rev) die(_("Couldn't look up commit object for HEAD")); } - for (i = 0; i < argc; i++, strbuf_reset(&bname)) { + for (i = 0; i < argc; i++) { char *target = NULL; int flags = 0; + strbuf_reset(&bname); strbuf_branchname(&bname, argv[i], allowed_interpret); free(name); name = mkpathdup(fmt, bname.buf); @@ -716,8 +717,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) print_columns(&output, colopts, NULL); string_list_clear(&output, 0); return 0; - } - else if (edit_description) { + } else if (edit_description) { const char *branch_name; struct strbuf branch_ref = STRBUF_INIT;