Re: [PATCH v3 2/8] branch: consider refs under 'update-refs'

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 30, 2022, 13:35 UTC
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>

Show 11 quoted lines

>> +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.

Show 5 quoted lines

>> +
>> +    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