On Tue, Feb 13, 2024, at 20:56, Rubén Justo wrote:
> On 13-feb-2024 11:39:11, Junio C Hamano wrote:
>> Rubén Justo <rjusto@xxxxxxxxx> 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.
Thanks for trying that out—some very topical testing!
I will take the night to think about v4. But I will defer to the
reviewers’ judgement on the scope of this series/change.
(All I know is that it can be tricky balancing such defensive checks
with readability and maintanability.)
Thanks
--
Kristoffer Haugsbakk
