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

On 13-feb-2024 11:39:11, Junio C Hamano wrote:

quoted

Rubén Justo [off-list ref] 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.