Re: [PATCH] branch: trivial style fix
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 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
Re: [PATCH] branch: trivial style fix
On Tue, Oct 16, 2018 at 4:16 PM Tao Qingyun 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.
Re: [PATCH] branch: trivial style fix
>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).
Re: [PATCH] branch: trivial style fix
On Sat, Oct 06, 2018 at 08:40:33AM +0800, Tao Qingyun wrote:
> >> - for (i = 0; i < argc; i++, strbuf_reset()) {
> >> + for (i = 0; i < argc; i++) {
> >>char *target = NULL;
> >>int flags = 0;
> >>
> >> + strbuf_reset();
> >
> > 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
Re: [PATCH] branch: trivial style fix
Tao Qingyun 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()) {
> + for (i = 0; i < argc; i++) {
> char *target = NULL;
> int flags = 0;
>
> + strbuf_reset();
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(, 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(, colopts, NULL);
> string_list_clear(, 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;
Re: [PATCH] branch: trivial style fix
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()) {
> + for (i = 0; i < argc; i++) {
> char *target = NULL;
> int flags = 0;
>
> + strbuf_reset();
> strbuf_branchname(, 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(, colopts, NULL);
> string_list_clear(, 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
[PATCH] branch: trivial style fix
---
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()) {
+ for (i = 0; i < argc; i++) {
char *target = NULL;
int flags = 0;
+ strbuf_reset();
strbuf_branchname(, 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(, colopts, NULL);
string_list_clear(, 0);
return 0;
- }
- else if (edit_description) {
+ } else if (edit_description) {
const char *branch_name;
struct strbuf branch_ref = STRBUF_INIT;
--
2.19.0
