Re: [PATCH v2 1/2] branch: refactor "edit_description" code path
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 7, 2022, 20:25 UTC
- Message-ID
- <xmqq7d2fszhk.fsf@gitster.g>
- In-Reply-To
- <d2624238-048c-ac5b-1d45-e08051202c79@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
Show 22 quoted lines
> 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.
Show 22 quoted lines
> @@ -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.
Show 9 quoted lines
> - } > - 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.