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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
