On Thu, Sep 11, 2025 at 6:29 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> >> + if (!parse_sign_mode(arg, val))
> >> + return 0;
> >> +
> >> + return error("Unknown %s mode: %s", opt->long_name, arg);
> >
> > Would it make sense to maybe reverse the error handling and say
> > something like:
> >
> > if (parse_sign_mode(arg, val) < 0)
> > return error("Unknown %s mode: %s", opt->long_name, arg);
> >
> > return 0;
> >
> > That reads a bit more natural to me at least.
Before the above part of the code, there is:
if (unset)
return 0;
So it felt natural to me to have a flow where we continue to return 0
(success) if we can and go on otherwise.
> I also thought that the original had a "Huh?" factor, but my
> reaction was more like "wouldn't it be easier to see if these are
> both sides of if/else?", i.e.
>
> if (parse_sign_mode(...))
> return error(...);
> else
> return 0;
>
> which made me think that the differences were mostly subjective and
> refrained from commenting.
I agree that the difference is very subjective. If there was something
like for example:
```
if (A)
return 0;
if (B)
return 0;
...
if (F)
return 0;
return -1
```
It would feel strange if the function ended instead with:
```
if (!F)
return -1;
return 0;
```
> But after I see your version, I tend to agree that it is easier to
> see the flow if the most-straightforward success case ran through to
> the end, while exceptional cases (i.e. an error return, or the
> initial "unset" case) did early returns. I have to add, however,
> that I feel that this is mostly subjective and falls into "once it
> is in the tree, it's not really worth the patch noise to go and fix
> it up" category. If we will have v2 of this series, I do prefer to
> see it written more in your way, though.
Fine, I have written it in the way Patrick suggested in V2.
Thanks.
