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

[Paul Tan <pyokagan@gmail.com>]

On Wed, Jun 24, 2015 at 10:59 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> 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?

Later in the patch series am_state_init() will also take into account
config settings when filling up the default values. e.g. see patches
25/31[1] or 31/31[2]. I think that is reasonable: the purpose of
am_state_init() is to initialize the am_state struct with the default
values, and the default values can be set by the user through the
config settings.

This means, though, that we can't use initializers without introducing
global variables.

[1] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972
[2] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972

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

But too much usage of this temporary strbuf may lead to a situation
where one function calls another, and both functions write to the
strbuf and clobber its contents.

Besides, if we are talking about read_state_file(), it takes an
external strbuf, so it gives the caller the freedom to choose which
strbuf it uses (e.g. if it is stack allocated or in the am_state
struct). I think it's more flexible this way.

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

Doh! Right, the hint parameter is quite useless, since in am_load() we
use the same strbuf anyway. (And strbuf_init() can set a hint as well)
I've removed it on my side.

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

Makes sense. After all, this function is called read_STATE_file() ;-)

> - 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);

So for now, my function signature is

static void read_state_file(struct strbuf *sb, const 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.

Ah right. Although, state->dir is not an strbuf anymore on my side. As
Junio[3] rightfully noted, state->dir is not modified by the am_*()
API, so it's been changed to a char*. Which means an strbuf is
required to be passed to remove_dir_recursively();

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

Yeah, Junio did bring something like that[3]. I'm still thinking about
it, though I don't think I would like this issue to block the patch
series since it's a delicate issue, and git-am.sh does not do anything
special either.

For now though, I've moved all the write_file()s into a central
am_save() function, so if we want to do any locking or syncing it
would be easy to modify am_save(), and then all the callers will
benefit.

[3] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972