On 9/10/22 21:11, Junio C Hamano wrote:
> Rubén Justo <rjusto@xxxxxxxxx> writes:
>
> * cmd_foo() should not return an negative value.
Yes, the refactoring we already discussed early in this thread.
>
> * branch_name used in the calls to error() could point at buf.buf
> that holds the expansion of @{-1}, but buf was released way too
> early, leading to a use-after-free.
:-( good catch, thanks. Removing the refactoring commit was not
carefully done.
>
> * Style: if/else if/else cascade whose one arm has multiple
> statements and requires {braces} around it should have {braces}
> around all of its arms.
Ok.
>
> * each arm in the top-level if/else if/else cascade for "git
> branch" subcommands were more or less independent, and there
> wasn't anything common that they need to execute after exiting
> the cascade. Unconditionally returning from the arm for the
> edit-description subcommand seems to make the logic flow easier
> to read.
Mmm, I don't feel the same here, we already discussed about this. Maybe?:
diff --git a/builtin/branch.c b/builtin/branch.c
index 17853225fa..307073cc47 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -817,7 +817,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_release(&branch_ref);
strbuf_release(&buf);
- return ret;
+ if (ret)
+ return ret; /* some failure happened */
} else if (copy) {
if (!argc)
die(_("branch name required"));
not much important, though.
You can squash the changes in the commit or if you need me to send a v6,
please let me know. Thank you for your careful review.
