Re: [PATCH v5 02/14] trailer: process trailers from file and arguments

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <chriscool@tuxfamily.org> writes:

> This patch implements the logic that process trailers
> from file and arguments.
>
> At the beginning trailers from file are in their own
> infile_tok doubly linked list, and trailers from
> arguments are in their own arg_tok doubly linked list.
>
> The lists are traversed and when an arg_tok should be
> "applied", it is removed from its list and inserted
> into the infile_tok list.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  trailer.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 187 insertions(+)
>
> diff --git a/trailer.c b/trailer.c
> index f129b5a..ba0cfe0 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -46,3 +46,190 @@ static inline size_t alnum_len(const char *buf, int len)
>  	while (--len >= 0 && !isalnum(buf[len]));
>  	return (size_t) len + 1;
>  }
> +
> +static void add_arg_to_infile(struct trailer_item *infile_tok,
> +			      struct trailer_item *arg_tok)
> +{
> +	if (arg_tok->conf->where == WHERE_AFTER) {
> +		arg_tok->next = infile_tok->next;
> +		infile_tok->next = arg_tok;
> +		arg_tok->previous = infile_tok;
> +		if (arg_tok->next)
> +			arg_tok->next->previous = arg_tok;
> +	} else {
> +		arg_tok->previous = infile_tok->previous;
> +		infile_tok->previous = arg_tok;
> +		arg_tok->next = infile_tok;
> +		if (arg_tok->previous)
> +			arg_tok->previous->next = arg_tok;
> +	}
> +}
> +
> +static int check_if_different(struct trailer_item *infile_tok,
> +			      struct trailer_item *arg_tok,
> +			      int alnum_len, int check_all)
> +{
> +	enum action_where where = arg_tok->conf->where;
> +	do {
> +		if (!infile_tok)
> +			return 1;
> +		if (same_trailer(infile_tok, arg_tok, alnum_len))
> +			return 0;
> +		/*
> +		 * if we want to add a trailer after another one,
> +		 * we have to check those before this one
> +		 */
> +		infile_tok = (where == WHERE_AFTER) ? infile_tok->previous : infile_tok->next;
> +	} while (check_all);
> +	return 1;
> +}
> +
> +static void apply_arg_if_exist(struct trailer_item *infile_tok,
> +			       struct trailer_item *arg_tok,
> +			       int alnum_len)
> +{
> +	switch (arg_tok->conf->if_exist) {
> +	case EXIST_DO_NOTHING:
> +		free(arg_tok);
> +		break;
> +	case EXIST_OVERWRITE:
> +		free((char *)infile_tok->value);
> +		infile_tok->value = xstrdup(arg_tok->value);
> +		free(arg_tok);
> +		break;
> +	case EXIST_ADD:
> +		add_arg_to_infile(infile_tok, arg_tok);
> +		break;
> +	case EXIST_ADD_IF_DIFFERENT:
> +		if (check_if_different(infile_tok, arg_tok, alnum_len, 1))
> +			add_arg_to_infile(infile_tok, arg_tok);
> +		else
> +			free(arg_tok);
> +		break;
> +	case EXIST_ADD_IF_DIFFERENT_NEIGHBOR:
> +		if (check_if_different(infile_tok, arg_tok, alnum_len, 0))
> +			add_arg_to_infile(infile_tok, arg_tok);
> +		else
> +			free(arg_tok);
> +		break;

Makes me wonder if people want a rule to say "if the same key
already exists, regardless of the value".

> +	}
> +}
> +
> +static void remove_from_list(struct trailer_item *item,
> +			     struct trailer_item **first)
> +{
> +	if (item->next)
> +		item->next->previous = item->previous;
> +	if (item->previous)
> +		item->previous->next = item->next;
> +	else
> +		*first = item->next;
> +}

Will callers free the item that now is not on the list?

> +static struct trailer_item *remove_first(struct trailer_item **first)
> +{
> +	struct trailer_item *item = *first;
> +	*first = item->next;
> +	if (item->next) {
> +		item->next->previous = NULL;
> +		item->next = NULL;
> +	}
> +	return item;
> +}
> +
> +static void process_infile_tok(struct trailer_item *infile_tok,
> +			       struct trailer_item **arg_tok_first,
> +			       enum action_where where)
> +{
> +	struct trailer_item *arg_tok;
> +	struct trailer_item *next_arg;
> +
> +	int tok_alnum_len = alnum_len(infile_tok->token, strlen(infile_tok->token));
> +	for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
> +		next_arg = arg_tok->next;
> +		if (same_token(infile_tok, arg_tok, tok_alnum_len) &&
> +		    arg_tok->conf->where == where) {
> +			remove_from_list(arg_tok, arg_tok_first);
> +			apply_arg_if_exist(infile_tok, arg_tok, tok_alnum_len);
> +			/*
> +			 * If arg has been added to infile,
> +			 * then we need to process it too now.
> +			 */
> +			if ((where == WHERE_AFTER ? infile_tok->next : infile_tok->previous) == arg_tok)
> +				infile_tok = arg_tok;
> +		}
> +	}
> +}
> +
> +static void update_last(struct trailer_item **last)
> +{
> +	if (*last)
> +		while((*last)->next != NULL)
> +			*last = (*last)->next;
> +}
> +
> +static void update_first(struct trailer_item **first)
> +{
> +	if (*first)
> +		while((*first)->previous != NULL)
> +			*first = (*first)->previous;
> +}
> +
> +static void apply_arg_if_missing(struct trailer_item **infile_tok_first,
> +				 struct trailer_item **infile_tok_last,
> +				 struct trailer_item *arg_tok)
> +{

Makes me wonder if it would make the code simpler to keep an anchor
item "struct trailer_item" that is off heap, and pass that single
anchor item around, using its next/prev fields as the first and the
last.  Wouldn't it let you remove the special cases for the first
and last item?

> +	struct trailer_item **infile_tok;
> +	enum action_where where;
> +
> +	switch (arg_tok->conf->if_missing) {
> +	case MISSING_DO_NOTHING:
> +		free(arg_tok);
> +		break;
> +	case MISSING_ADD:
> +		where = arg_tok->conf->where;
> +		infile_tok = (where == WHERE_AFTER) ? infile_tok_last : infile_tok_first;
> +		if (*infile_tok) {
> +			add_arg_to_infile(*infile_tok, arg_tok);
> +			*infile_tok = arg_tok;
> +		} else {
> +			*infile_tok_first = arg_tok;
> +			*infile_tok_last = arg_tok;
> +		}
> +		break;

This piece makes me wonder if "after" is a good name.  prepend and
append, perhaps?