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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 22, 2021, 08:06 UTC
Message-ID: <877dhivkei.fsf@evledraar.gmail.com>
In-Reply-To: <ecd41b370e62cc88f1606569ec700eaca837fa1f.1626939557.git.gitgitgadget@gmail.com>

On Thu, Jul 22 2021, ZheNing Hu via GitGitGadget wrote:

> [..] add a new member in
> `struct atom_value`: `s_size`, which can record raw object size, it
> can help us add raw object data to the buffer or compare two buffers
> which contain raw object data.

I didn't look in detail, just one nit/comment, no need to re-roll for this:

Show 11 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_S_SIZE_INIT (-1)
> +

This and then later doing (this appears to be the only initialization?):

Show 6 quoted lines

>  	if (format->need_color_reset_at_eol) {
>  		struct atom_value resetv;
> +		resetv.s_size = ATOM_VALUE_S_SIZE_INIT;
>  		resetv.s = GIT_COLOR_RESET;
>  		if (append_atom(&resetv, &state, error_buf)) {
>  			pop_stack_element(&state.stack);

Is a rather unusual pattern, more typical would be:

	struct atom_value { .... }
	#define ATOM_VALUE_INIT { \
		.s_size = -1, \
	}

Then:

	struct atom_value resetv = ATOM_VALUE_INIT;

You could keep that "#define ATOM_VALUE_S_SIZE_INIT (-1)" to check the init value, personally I think it's just fine to hardcode the -1 literal and do "size < 0" checks, which we us as convention in a lot of other places for "not initialized".