Rubén Justo <rjusto@xxxxxxxxx> writes: > Stick to the convention of exit with 1 on error. May break some > third party tool that expects -1 on "No branch named 'bla'" or > "No commit on branch 'bla' yet." errors. > > Test added to avoid regresion on this behaviour. > --- > > Junio, here is the patch for the change you suggested [1]. My concern is > this might break some third party tool or script. If no one thinks this > is an issue, I can squash it with the patch I'm responding to. > > > [1] https://lore.kernel.org/git/xmqq7d2fszhk.fsf@gitster.g/ > > builtin/branch.c | 6 +++--- > t/t3200-branch.sh | 8 ++++++++ > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index b1f6519cd9..01b6364f58 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -810,11 +810,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > else > ret = error(_("No branch named '%s'."), > branch_name); > - } else if (edit_branch_description(branch_name)) > - ret = 1; > + } else > + ret = edit_branch_description(branch_name); > > strbuf_release(&branch_ref); > - return ret; > + return -ret; If edit_branch_description() returns -1 on failure, this would consistently exit with 1 on failure, so it does make sense. It is bad to exit with a value outside 0..255 for portability reasons, but we wrap exit() to chomp off the higher bits, so in practice it may be fine to exit(-1) but still the above is a good clean-up, I would think.
