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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 18, 2015, 21:23 UTC
Message-ID: <xmqqpp4sd83f.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1434626743-8552-9-git-send-email-pyokagan@gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 26 quoted lines

> Implement applying the patch to the index using git-apply.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  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.

Show 9 quoted lines

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