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

["Rubén Justo" <rjusto@gmail.com>]

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.

Thinking again about this; you are right.  That BUG() in
run_column_filter() does not make sense in this series.

It is addressing a different error, and perhaps a solution could be:

--- >8 ---
Subject: [PATCH] tag: error when git-column fails

If the user asks for the list of tags to be displayed in columns
("--columns"), a child git-column process is used to format the output
as expected.

In a rare situation where we encounter a problem spawning that child
process, we will work erroneously.

Make noticeable we're having a problem executing git-column, so the user
can act accordingly.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/tag.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 37473ac21f..30532b76d5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -530,7 +530,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			struct column_options copts;
 			memset(&copts, 0, sizeof(copts));
 			copts.padding = 2;
-			run_column_filter(colopts, &copts);
+			if (run_column_filter(colopts, &copts))
+				die(_("could not start 'git column'")
 		}
 		filter.name_patterns = argv;
 		ret = list_tags(&filter, sorting, &format);
-- 
2.43.0