Re: [PATCH v2 2/5] [GSOC] ref-filter: add %(raw) atom

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2021, 16:38 UTC
Message-ID: <xmqqh7glouiw.fsf@gitster.g>
In-Reply-To: <eafb79bad62f13fc8fd70ba1dce3e8fbab870e52.1627031043.git.gitgitgadget@gmail.com>

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

>  struct atom_value {
>  	const char *s;
> +	size_t s_size;
>  	int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
>  		       struct strbuf *err);
>  	uintmax_t value; /* used for sorting when not FIELD_STR */
>  	struct used_atom *atom;
>  };
>  
> +#define ATOM_VALUE_INIT { \
> +	.s_size = -1 \
> +}

This will get turned into size_t that is unsigned, and comparisons like this one and ...

Show 6 quoted lines

>  	case QUOTE_NONE:
> -		strbuf_addstr(s, str);
> +		if (len != -1)
> +			strbuf_add(s, str, len);
> +		else
> +			strbuf_addstr(s, str);

this one ...

Show 6 quoted lines

> +		if (v->s_size != -1)
> +			strbuf_add(&state->stack->output, v->s, v->s_size);
> +		else
> +			strbuf_addstr(&state->stack->output, v->s);
>  	return 0;
>  }

may work as expected, but it makes readers wonder if there is significance to negative values other than -1 (in other words, what does it mean if v->s_size == -2, for example?).

It may make sense to

 * Turn atom_value.s_size field into ssize_t instead of size_t

 * Rewrite (v->s_size != -1) comparison to (v->s_size < 0)

Optionally we could introduce #define ATOM_SIZE_UNSPECIFIED (-1) and use it to initialize .s_size in ATOM_VALUE_INIT, but if we are just going to test "is it negative, then it is not given", then it probably is not needed.

Other changes in the whole series relative to what has been queued looked all sensible to me.

THanks.