Hi Paul,
On 2015-06-18 13:25, Paul Tan wrote:
> 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?
> +/**
> + * 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);
> +/**
> + * 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.
> +/**
> + * 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html