Subject: Re: [PATCH/WIP v3 08/31] am: apply patch with git-apply

From: Junio C Hamano <[email-masked]>
Date: Thu, 18 Jun 2015 14:23:16 -0700

Paul Tan <[email-masked]> writes:

> Implement applying the patch to the index using git-apply.
>
> Signed-off-by: Paul Tan <[email-masked]>
> ---
>  builtin/am.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index d6434e4..296a5fc 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -27,6 +27,18 @@ static int is_empty_file(const char *filename)
>  	return !st.st_size;
>  }
>  
> +/**
> + * Returns the first line of msg
> + */
> +static const char *firstline(const char *msg)
> +{
> +	static struct strbuf sb = STRBUF_INIT;
> +
> +	strbuf_reset(&sb);
> +	strbuf_add(&sb, msg, strchrnul(msg, '\n') - msg);
> +	return sb.buf;
> +}

Hmm.  This is not wrong per-se but a more efficient way to do it may
be to have a helper function that returns a bytecount of the first
line of the msg, i.e. strchrnul(msg, '\n') - msg.  Then a caller can
do

	printf("Applying: %.*s", linelen(msg), msg);

instead of

	printf("Applying: %s", firstline(msg));

relying on that the firstline() copies the contents to a static
strbuf that does not have to be freed.

> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	cp.git_cmd = 1;
> +
> +	argv_array_push(&cp.args, "apply");
> +
> +	argv_array_push(&cp.args, "--index");
> +
> +	argv_array_push(&cp.args, am_path(state, "patch"));

You seem to like blank lines a lot ;-)  While it is a good tool to
separate different groups while grouping related things together,
these three argv-push calls are intimately related, and reads better
without blanks in between.

Looks nicely done so far...