On 13-feb-2024 11:39:11, Junio C Hamano wrote:
> Rubén Justo <rjusto@xxxxxxxxx> 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.
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.
