Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Stefan Beller <sbeller@google.com>]

On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> These functions can be used for loading and saving common rebase options
> into a state directory.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  rebase-common.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  rebase-common.h |  4 ++++
>  2 files changed, 73 insertions(+)
>
> diff --git a/rebase-common.c b/rebase-common.c
> index 5a49ac4..1835f08 100644
> --- a/rebase-common.c
> +++ b/rebase-common.c
> @@ -26,3 +26,72 @@ void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src)
>         *dst = *src;
>         *src = tmp;
>  }
> +
> +static int state_file_exists(const char *dir, const char *file)
> +{
> +       return file_exists(mkpath("%s/%s", dir, file));
> +}

How is this specific to the state file? All it does is create the
leading directory
if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
have the same
result without actually creating the directory if it doesn't exist as
a side effect?

If the dir doesn't exist it can be created in rebase_options_load explicitly?


> +
> +static int read_state_file(struct strbuf *sb, const char *dir, const char *file)
> +{
> +       const char *path = mkpath("%s/%s", dir, file);
> +       strbuf_reset(sb);
> +       if (strbuf_read_file(sb, path, 0) >= 0)
> +               return sb->len;
> +       else
> +               return error(_("could not read '%s'"), path);
> +}
> +
> +int rebase_options_load(struct rebase_options *opts, const char *dir)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       const char *filename;
> +
> +       /* opts->orig_refname */
> +       if (read_state_file(&sb, dir, "head-name") < 0)
> +               return -1;
> +       strbuf_trim(&sb);
> +       if (starts_with(sb.buf, "refs/heads/"))
> +               opts->orig_refname = strbuf_detach(&sb, NULL);
> +       else if (!strcmp(sb.buf, "detached HEAD"))
> +               opts->orig_refname = NULL;
> +       else
> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, "head-name"));
> +
> +       /* opts->onto */
> +       if (read_state_file(&sb, dir, "onto") < 0)
> +               return -1;
> +       strbuf_trim(&sb);
> +       if (get_oid_hex(sb.buf, &opts->onto) < 0)
> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, "onto"));
> +
> +       /*
> +        * We always write to orig-head, but interactive rebase used to write
> +        * to head. Fall back to reading from head to cover for the case that
> +        * the user upgraded git with an ongoing interactive rebase.
> +        */
> +       filename = state_file_exists(dir, "orig-head") ? "orig-head" : "head";
> +       if (read_state_file(&sb, dir, filename) < 0)
> +               return -1;

So from here on we always use "orig-head" instead of "head" for
interactive rebase.
Would people ever rely on the (internal) file name and have e.g.
scripts which operate
on the "head" file ?


> +       strbuf_trim(&sb);
> +       if (get_oid_hex(sb.buf, &opts->orig_head) < 0)
> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, filename));
> +
> +       strbuf_release(&sb);
> +       return 0;
> +}
> +
> +static int write_state_text(const char *dir, const char *file, const char *string)
> +{
> +       return write_file(mkpath("%s/%s", dir, file), "%s", string);
> +}

Same comment as on checking the state files existence. I'm not sure if the side
effect of creating the dir is better done explicitly where it is used.
The concat of dir and
file name can still be done in the helper though? (If the helper is
needed at all then)

> +
> +void rebase_options_save(const struct rebase_options *opts, const char *dir)
> +{
> +       const char *head_name = opts->orig_refname;
> +       if (!head_name)
> +               head_name = "detached HEAD";
> +       write_state_text(dir, "head-name", head_name);
> +       write_state_text(dir, "onto", oid_to_hex(&opts->onto));
> +       write_state_text(dir, "orig-head", oid_to_hex(&opts->orig_head));
> +}
> diff --git a/rebase-common.h b/rebase-common.h
> index db5146a..051c056 100644
> --- a/rebase-common.h
> +++ b/rebase-common.h
> @@ -20,4 +20,8 @@ void rebase_options_release(struct rebase_options *);
>
>  void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src);
>
> +int rebase_options_load(struct rebase_options *, const char *dir);
> +
> +void rebase_options_save(const struct rebase_options *, const char *dir);
> +
>  #endif /* REBASE_COMMON_H */
> --
> 2.7.0
>