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

[Stefan Beller <sbeller@google.com>]

On Wed, Mar 16, 2016 at 5:04 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>
>> 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?
>
> I don't quite understand, AFAIK mkpath() does not create any
> directories as a side-effect. And yes, I just wanted a short way to
> say file_exists(concat(dir, file)) or file_exists(mkpath("%s/%s", dir,
> file)) without cluttering up the code.

My bad. I should not assume functions doing stuff as their name might suggest.
(It "makes the path" but only in terms of creating the right string, not on the
file system, where you'd use functions like safe_create_leading_directories.
I thought all that was implied in mkpath).

>
>> If the dir doesn't exist it can be created in rebase_options_load explicitly?
>
> I don't intend to create any directories if they do not exist.
>
>>> +
>>> +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 ?
>
> This backwards-compatibility code is just a straight port from the
> code in git-rebase.sh.
>
> The usage of orig-head has been around since 2011 with 84df456
> (rebase: extract code for writing basic state, 2011-02-06), so I guess
> if people had issues with it, it would have been reported.

I did not read the rebase shell code, but commented on the C code only.
If this is already in there, let's keep it.
Sorry for the confusion.

Thanks,
Stefan