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

From

Rubén Justo <rjusto@gmail.com>

Date

Feb 13, 2024, 19:56 UTC

Message-ID

<b9ca1ab7-f8f6-4fe0-885a-51728d9ec708@gmail.com>

In-Reply-To

<xmqqle7o5f34.fsf@gitster.g>

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

> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> This one happens to be safe currently because "git tag" passes 2 in
> >> opts->padding, but I do not think this is needed.
> >
> > At first glance, I also thought this was not necessary.
> >
> > However, callers of run_column_filter() might forget to check the return
> > value, and the BUG() triggered by the underlying process could be buried
> > and ignored.  Having the BUG() here, in the same process, makes it more
> > noticeable.
> 
> 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.

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.