Re: [PATCH v3 2/2] column: guard against negative padding

> On 13-feb-2024 11:39:11, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>> The point of BUG() is to help developers catch the silly breakage
>> before it excapes from the lab, and we can expect these careless
>> developers to ignore the return value.  But "column --padding=-1"
>> invoked as a subprocess will show a human-readable error message
>> to such a developer, so it is less important than the BUG() in the
>> other place.
>>
>> There is no black or white decision, but this one is much less
>> darker gray than the other one is.
>
> I've checked this, without that BUG(), and the result has not been
> pretty:
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 37473ac21f..e15dfa73d2 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>                 if (column_active(colopts)) {
>                         struct column_options copts;
>                         memset(&copts, 0, sizeof(copts));
> -                       copts.padding = 2;
> +                       copts.padding = -1;
>                         run_column_filter(colopts, &copts);
>                 }
>                 filter.name_patterns = argv;
>
> I can imagine a future change that opens that current "2" to the user.
> And the possible report from a user who tries "-1" would not be easy.
>
> But I agree with you, that BUG() does not leave a good taste in the
> mouth.
>
> Maybe we should refactor run_column_filter(), I don't know, but I think
> that is outside of the scope of this series.