Re: [PATCH] commit: Remove backward goto in read_craft_line()

> Ralf Thielow <ralf.thielow@googlemail.com> writes:
>
>>> If "--show-c-function" output is the problem, perhaps we should know a bit
>>> better about what C function header looks like?
>>
>> In fact the "--show-c-function" output is the problem. But I think that
>> a change can't be rejected because of another issue.
>
> Well, I never said anything about rejection nor acceptance.
>
>> The style of placing "goto"-statements, which leave a function to the
>> end of that is used in many other projects. And I think
>> it's very usefull.
>
> It is a good discipline to follow in general to place an exceptional case
> at the end if you jump out of the general flow.  But because the affected
> function was so small, its readability doesn't benefit very much from the
> discipline (in other words, you knew about a good discipline, but applied
> it to a wrong function).  The patch was small and obviously correct, so it
> will not hurt, but it will not make the world greatly a better place,
> either.
>
> IOW, it was a "Meh" topic for me.
>
> It was more important to discuss Jonathan's "leave SP at the beginning of
> a goto label to please --show-c-function" from the maintainer's point of
> view, as people may remember it as a rule and start sending patches that
> follow it, which I will need to deal with in the future.  I do not think
> that one is a good rule.
>
> Now that we have dealt with that more important business of letting people
> know that protecting goto labels with a leading SP is _not_ the rule ;-),
> I am happy with this discussion.
>
> And often I forget about the original issue when a discussion reaches this
> stage of happiness.  So thanks for reminding me.
>
> As I said, even though I do not think the particular function you touched
> badly needs the discipline applied, it would not hurt, and it obviously is
> the right thing to do (iow, if the function were written from day one in a
> way your patch reorganized it, we would never accept a patch to change it
> into today's shape of jumping backwards).  For one thing, it would remove
> an example from the codebase people can point at to make excuses when
> responding to a review of their patch that adds a backward goto to a much
> larger function.
>
> Will queue after massaging the log message somewhat.  Thanks.
>