Re: [RFC PATCH 06/15] ref-filter.c: BUG() out on show_ref() with NULL refname

From: René Scharfe <l.s.r@web.de>
Date: Jun 4, 2022, 18:07 UTC
Message-ID: <6679c09e-e691-f360-ace8-aae17ecabdc4@web.de>
In-Reply-To: <RFC-patch-06.15-2d04035d7aa-20220603T183608Z-avarab@gmail.com>

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

Show 25 quoted lines

> Adjust code originally added in 5339bdad96a (ref-filter: introduce
> remote_ref_atom_parser(), 2016-02-17) to BUG() out rather than
> potentially segfault if we get a NULL refname here.
>
> As noted by GCC v12's -fanalyzer that will happen if this follows the
> "refname = NULL" branch added in cc72385fe35 (for-each-ref: let
> upstream/push optionally report the remote name, 2017-10-05).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  ref-filter.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 2413f889f48..91aa8e89268 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1644,7 +1644,9 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>  				    struct branch *branch, const char **s)
>  {
>  	int num_ours, num_theirs;
> -	if (atom->u.remote_ref.option == RR_REF)
> +	if (atom->u.remote_ref.option == RR_REF && !refname)
> +		BUG("must get refname with [...]remote_ref.option == RR_REF");
> +	else if (atom->u.remote_ref.option == RR_REF)

The related code not shown here looks tricky. IIUC refname cannot be NULL if option is RR_REF, because remote_ref_atom_parser() only sets .push_remote for other option values and only if that's set we can get a NULL refname passed to fill_remote_ref_details(). Right?

Do we need to have two different ways to indicate remote-ness? Having just one like after the patch below should reduce the risk of confusion.

>  		*s = show_ref(&atom->u.remote_ref.refname, refname);
>  	else if (atom->u.remote_ref.option == RR_TRACK) {
>  		if (stat_tracking_info(branch, &num_ours, &num_theirs,

diff --git a/ref-filter.c b/ref-filter.c
index 2413f889f4..260257b92c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -183,7 +183,7 @@ static struct used_atom {
 				RR_REF, RR_TRACK, RR_TRACKSHORT, RR_REMOTE_NAME, RR_REMOTE_REF
 			} option;
 			struct refname_atom refname;
-			unsigned int nobracket : 1, push : 1, push_remote : 1;
+			unsigned int nobracket : 1, push : 1;
 		} remote_ref;
 		struct {
 			enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,
@@ -295,10 +295,8 @@ static int remote_ref_atom_parser(struct ref_format *format, struct used_atom *a
 			atom->u.remote_ref.nobracket = 1;
 		else if (!strcmp(s, "remotename")) {
 			atom->u.remote_ref.option = RR_REMOTE_NAME;
-			atom->u.remote_ref.push_remote = 1;
 		} else if (!strcmp(s, "remoteref")) {
 			atom->u.remote_ref.option = RR_REMOTE_REF;
-			atom->u.remote_ref.push_remote = 1;
 		} else {
 			atom->u.remote_ref.option = RR_REF;
 			if (refname_atom_parser_internal(&atom->u.remote_ref.refname,
@@ -1891,7 +1889,8 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 				continue;
 			branch = branch_get(branch_name);

-			if (atom->u.remote_ref.push_remote)
+			if (atom->u.remote_ref.option == RR_REMOTE_NAME ||
+			    atom->u.remote_ref.option == RR_REMOTE_REF)
 				refname = NULL;
 			else {
 				refname = branch_get_push(branch, NULL);