From: Derrick Stolee <derrickstolee@github.com>
Date: Thu, 30 Jun 2022 13:35:34 GMT
Subject: Re: [PATCH v3 2/8] branch: consider refs under 'update-refs'
Message-ID: <b0c596d6-f4aa-9575-4d79-64bd112d0baf@github.com>
In-Reply-To: <f27d563b-d011-9983-8f7d-52232b052608@gmail.com>

On 6/30/2022 5:32 AM, Phillip Wood wrote:
> On 28/06/2022 14:25, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <derrickstolee@github.com>

>> +int sequencer_get_update_refs_state(const char *wt_dir,
>> +                    struct string_list *refs)
>> +{
>> +    int result = 0;
>> +    struct stat st;
>> +    FILE *fp = NULL;
>> +    struct strbuf ref = STRBUF_INIT;
>> +    struct strbuf hash = STRBUF_INIT;
>> +    char *path = xstrfmt("%s/rebase-merge/update-refs", wt_dir);
> 
> I think it would make sense to introduce rebase_path_update_refs() in this patch rather than later in the series

The biggest difference is that rebase_path_update_refs() only
gives the path for the current worktree, while this method needs
to read the file for any worktree.

There is likely some opportunity to create rebase_path_update_refs()
in a different way that serves both purposes.

>> +
>> +    if (stat(path, &st))
>> +        goto cleanup;
> 
> What's the reason for stating the file first, rather than just letting fopen() fail if it does not exist?

Not sure what I was looking at that gave this pattern, but you're
right that it isn't necessary.

Thanks,
-Stolee