Re: [PATCH/WIP v3 04/31] am: implement patch queue mechanism

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 24, 2015, 14:59 UTC
Message-ID: <6560adbc8b1842dd369628980f1264c3@www.dscho.org>
In-Reply-To: <1434626743-8552-5-git-send-email-pyokagan@gmail.com>

Hi Paul,

On 2015-06-18 13:25, Paul Tan wrote:

Show 29 quoted lines

> diff --git a/builtin/am.c b/builtin/am.c
> index dbc8836..af68c51 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -6,6 +6,158 @@
>  #include "cache.h"
>  #include "builtin.h"
>  #include "exec_cmd.h"
> +#include "parse-options.h"
> +#include "dir.h"
> +
> +struct am_state {
> +	/* state directory path */
> +	struct strbuf dir;
> +
> +	/* current and last patch numbers, 1-indexed */
> +	int cur;
> +	int last;
> +};
> +
> +/**
> + * Initializes am_state with the default values.
> + */
> +static void am_state_init(struct am_state *state)
> +{
> +	memset(state, 0, sizeof(*state));
> +
> +	strbuf_init(&state->dir, 0);
> +}

With strbufs, we use the initializer STRBUF_INIT. How about using

#define AM_STATE_INIT { STRBUF_INIT, 0, 0 }

here?

Show 22 quoted lines

> +/**
> + * Reads the contents of `file`. The third argument can be used to give a hint
> + * about the file size, to avoid reallocs. Returns number of bytes read on
> + * success, -1 if the file does not exist. If trim is set, trailing whitespace
> + * will be removed from the file contents.
> + */
> +static int read_state_file(struct strbuf *sb, const char *file,
> size_t hint, int trim)
> +{
> +	strbuf_reset(sb);
> +	if (strbuf_read_file(sb, file, hint) >= 0) {
> +		if (trim)
> +			strbuf_trim(sb);
> +
> +		return sb->len;
> +	}
> +
> +	if (errno == ENOENT)
> +		return -1;
> +
> +	die_errno(_("could not read '%s'"), file);
> +}

A couple of thoughts:

- why not reuse the strbuf by making it a part of the am_state()? That way, you can allocate, say, 1024 bytes (should be plenty enough for most of our operations) and just reuse them in all of the functions. We will not make any of this multi-threaded anyway, I don't think.

- Given that we only read short files all the time, why not skip the hint parameter? Especially if we reuse the strbuf, it should be good enough to allocate a reasonable buffer first and then just assume that we do not have to reallocate it all that often anyway.

- Since we only read files from the state directory, why not pass the basename as parameter? That way we can avoid calling `am_path()` explicitly over and over again (and yours truly cannot forget to call `am_path()` in future patches).

- If you agree with these suggestions, the signature would become something like

static void read_state_file(struct am_state *state, const char *basename, int trim);

Show 11 quoted lines

> +/**
> + * Remove the am_state directory.
> + */
> +static void am_destroy(const struct am_state *state)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	strbuf_addstr(&sb, state->dir.buf);
> +	remove_dir_recursively(&sb, 0);
> +	strbuf_release(&sb);
> +}

Given that `remove_dir_recursively()` has to reset the strbuf with the directory's path to the original value before it returns (because it recurses into itself, therefore the value *has* to be reset when returning), we can just call

    remove_dir_recursively(&state->dir, 0);

and do not need another temporary strbuf.

Show 9 quoted lines

> +/**
> + * Increments the patch pointer, and cleans am_state for the application of the
> + * next patch.
> + */
> +static void am_next(struct am_state *state)
> +{
> +	state->cur++;
> +	write_file(am_path(state, "next"), 1, "%d", state->cur);
> +}

Locking and re-checking the contents of "next" before writing the incremented value would probably be a little too paranoid...

(Just saying it out loud, the current code is fine by me.)

Ciao, Dscho