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

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

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