Re: [PATCH v6 02/11] trailer: process trailers from stdin and arguments

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2014, 22:52 UTC
Message-ID: <xmqqiors1cn4.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140304194810.14249.7378.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 11 quoted lines

> Implement the logic to process trailers from stdin and arguments.
>
> At the beginning trailers from stdin are in their own in_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 in_tok list.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---

Thanks.

This round is marked as the sixth, but I still see quite a many style issues, which I expect not to see from long timers without being told. Somewhat disappointing...

Show 10 quoted lines

>  trailer.c | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 197 insertions(+)
>
> diff --git a/trailer.c b/trailer.c
> index db93a63..a0124f2 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -47,3 +47,200 @@ static size_t alnum_len(const char *buf, size_t len)
> ...
> +			if ((where == WHERE_AFTER ? in_tok->next : in_tok->previous) == arg_tok)

Overlong line that does not have to be.

			if ((where == WHERE_AFTER
                             ? in_tok->next : in_tok->previous) == arg_tok)

or something?

> +static void update_last(struct trailer_item **last)
> +{
> +	if (*last)
> +		while((*last)->next != NULL)

Style.  SP between control keyword and the expression.

Show 7 quoted lines

> +			*last = (*last)->next;
> +}
> +
> +static void update_first(struct trailer_item **first)
> +{
> +	if (*first)
> +		while((*first)->previous != NULL)

Ditto.

Show 6 quoted lines

> +static void process_trailers_lists(struct trailer_item **in_tok_first,
> ...
> +	/* Process input from end to start */
> +	for (in_tok = *in_tok_last; in_tok; in_tok = in_tok->previous) {
> +		process_input_token(in_tok, arg_tok_first, WHERE_AFTER);
> +	}

Needless brace pair.

> +	/* Process input from start to end */
> +	for (in_tok = *in_tok_first; in_tok; in_tok = in_tok->next) {
> +		process_input_token(in_tok, arg_tok_first, WHERE_BEFORE);
> +	}

Ditto.