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

From: Paul Tan <pyokagan@gmail.com>
Date: Jun 19, 2015, 12:49 UTC
Message-ID: <CACRoPnSVWNOPPJAhJS1S-HbHUEVJ1p_jz-dnFmuk8Hh1NaMLwg@mail.gmail.com>
In-Reply-To: <xmqq7fr0eoho.fsf@gitster.dls.corp.google.com>

On Fri, Jun 19, 2015 at 4:43 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

> Paul Tan <pyokagan@gmail.com> writes:
>
>> 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;
>
> Is this a temporary variable you will append "/patch", etc. to form
> a different string to use for fopen() etc., or do you use separate
> strbufs for things like that and this is only used to initialize
> them?
>
>  - If the former then "dir" is a misnomer.
>
>  - If the latter, then it probably does not have to be a strbuf;
>    rather, it should probably be a "const char *".  Unless you pass
>    this directly to functions that take a strbuf, such as
>    remove_dir_recursively(), that is.

It's the latter, and yes it does not need to be an strbuf since it will not usually change.

However, I don't think it should be a const char*, as it means that the API user has to keep track of the lifetime of the "dir" string. Note that we will have to git_pathdup() as git_path() returns a static buffer:

char *dir = git_pathdup("rebase-apply");
struct am_state state;
am_state_init(&state);
state.dir = dir;
...stuff...
if (foo) {
     ... separate code path ...
     am_state_release(&state);
     free(dir);
     return 0;
}
... separate code path ...
am_state_release(&state);
free(dir);
return 0;

Note the additional memory bookkeeping.

Instead, I would rather the am_state struct take ownership of the "dir" string and free it in am_state_release(). So dir will be a char*:

struct am_state state;
am_state_init(&state, git_path("rebase-apply"));
... stuff ...
if (foo) {
     ... separate code path ...
     am_state_release(&state);
     return 0;
}
... separate code path ...
am_state_release(&state);
return 0;

Show 9 quoted lines

>> +/**
>> + * Release memory allocated by an am_state.
>> + */
>
> Everybody else in this file seems to say "Initializes", "Returns",
> "Reads", etc.  While I personally prefer to use imperative
> (i.e. give command to this function to "release memory allocated"),
> you would want to be consistent throughout the file; "Releases
> memory" would make it so.

OK

Show 19 quoted lines

>> +/**
>> + * Setup a new am session for applying patches
>> + */
>> +static void am_setup(struct am_state *state)
>> +{
>> +     if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
>> +             die_errno(_("failed to create directory '%s'"), state->dir.buf);
>> +
>> +     write_file(am_path(state, "next"), 1, "%d", state->cur);
>> +
>> +     write_file(am_path(state, "last"), 1, "%d", state->last);
>
> These two lines are closely related pair; drop the blank in between.
>
> I am tno sure if write_file() is an appropriate thing to use,
> though.  What happens when you get interrupted after opening the
> file but before you manage to write and close?  Shouldn't we be
> doing the usual "write to temp, close and then rename to final"
> dance?  This comment applies to all the other use of write_file().

We could, although I don't think it would help us much. The state directory is made up of many different files, so even if we made modifications to single files atomic, there's no point if we terminate unexpectedly in the middle of writing multiple files to the state directory as the state, as a whole, is corrupted anyway.

So, we must be able to make updates to the entire state directory as a single transaction, which is a more difficult problem...

Furthermore, while applying patches, we may face abnormal termination between e.g. the patch application and commit, so I think it is safe to say that if abnormal termination occurs, we will not be able to reliably --resume or --skip anyway, and the user should just run --abort to go back to the last known state.

However, it would be an improvement if we wrote the "next" and "last" files last, as we use their presence to determine if we have an am session in progress or not, so if we terminate in am_setup() we will just have a stray directory. I will make this change in the next iteration.

Regards, Paul