Re: [PATCH v2 1/2] branch: refactor "edit_description" code path
From: Junio C Hamano <hidden>
Date: 2022-09-07 20:26:04
Rubén Justo [off-list ref] writes:
quoted hunk
Minor refactoring to reduce the number of returns in the switch case handling the "edit_description" option, so the calls to strbuf_release can also be reduced. New resources to be added also do not need to be released in multiple places. Signed-off-by: Rubén Justo <redacted> --- builtin/branch.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)diff --git a/builtin/branch.c b/builtin/branch.c index 55cd9a6e99..5229cb796f 100644 --- a/builtin/branch.c +++ b/builtin/branch.c@@ -614,7 +614,7 @@ static int edit_branch_description(const char *branch_name) strbuf_reset(&buf); if (launch_editor(edit_description(), &buf, NULL)) { strbuf_release(&buf); - return -1; + return 1; } strbuf_stripspace(&buf, 1);
Our API convention is to signal a failure with negative return value. Granted that this is not a general API but is merely a helper function in the implementation of a single command, it would be less confusing if you sticked to the convention. Unless there is a compelling reason not to, that is.
quoted hunk
@@ -791,6 +791,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) } else if (edit_description) { const char *branch_name; struct strbuf branch_ref = STRBUF_INIT; + int ret = 0; if (!argc) { if (filter.detached)@@ -803,19 +804,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix) strbuf_addf(&branch_ref, "refs/heads/%s", branch_name); if (!ref_exists(branch_ref.buf)) { - strbuf_release(&branch_ref); - if (!argc) - return error(_("No commit on branch '%s' yet."), + ret = error(_("No commit on branch '%s' yet."), branch_name); else - return error(_("No branch named '%s'."), + ret = error(_("No branch named '%s'."), branch_name);
OK. These are good uses of a new variable 'ret'. Note that error() returns negative one.
- } - strbuf_release(&branch_ref); + } else + ret = edit_branch_description(branch_name); - if (edit_branch_description(branch_name)) - return 1; + strbuf_release(&branch_ref); + return ret;
When editor failed, we leaked branch_ref strbuf, but we no longer do. Which is good. This makes cmd_branch() return -1 (when we see error() call) or 1 (when edit_branch_description() fails and returns 1). I would suggest to * Fix the return value of edit_branch_description() so that it signals a failure by returning -1 * cmd_branch() to return (or call exit() with) -ret, as ret has 0 when everything is peachy, and negative in any error code paths.