Paul Tan <pyokagan@xxxxxxxxx> 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.
> +/**
> + * 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.
> +/**
> + * 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().
--
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