| Message ID | d2624238-048c-ac5b-1d45-e08051202c79@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch: support for shortcuts like @{-1}, completed | expand |
Rubén Justo <rjusto@gmail.com> writes: > 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 <rjusto@gmail.com> > --- > 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. > @@ -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.
On 9/7/22 10:25 PM, Junio C Hamano wrote: Maybe the return 1 when edit_branch_description(), was a typo I was maintaining? >> - } >> - 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; Thanks for you review.
On 9/7/22 11:24 PM, Rubén Justo wrote: > > On 9/7/22 10:25 PM, Junio C Hamano wrote: > > Maybe the return 1 when edit_branch_description(), was a typo I was maintaining? > >>> - } >>> - 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; > > Thanks for you review. > Nevermind. 1 is the return for exit. Sorry. I'm sending a v3 with your suggestions. Thanks again.
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); @@ -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); - } - 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; } else if (copy) { if (!argc) die(_("branch name required"));
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 <rjusto@gmail.com> --- builtin/branch.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)