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

[Christian Couder <chriscool@tuxfamily.org>]

From: Junio C Hamano <gitster@pobox.com>
>
> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> +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".

This is what "if_exists" and "if_missing" are all about.

Either:

	the same key already exists regardless of the value

and, in this case, what happens depends on what has been specified using
the "if_exists" configuration variable.

Or:

	the same key DOES NOT already exists regardless of the value

and in this case, what happens depends on what has been specified
using the "if_missing" configuration variable.

>> +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?

Yes, or the item will be inserted into another 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?

Yeah, that could work. On the other hand the other fields of this
special item would not be used for anything.
I will have a look at it.

>> +	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?

The problem is that "prepend" and "append" could be confusing when
related to EXISTS_DO_NOTHING, MISSING_DO_NOTHING and EXISTS_OVERWRITE.

Also WHERE_MIDDLE and WHERE_SORTED could perhaps be added later in the
same enum as WHERE_AFTER and WHERE_BEFORE, and in this case, we would
need to find names for those that are like "prepend" and "append", but
that are also difficult to confuse with the EXISTS_XXX and MISSING_XXX
names.

Thanks,
Christian.