From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 01 Jul 2022 13:40:30 GMT
Subject: Re: [PATCH v3 2/8] branch: consider refs under 'update-refs'
Message-ID: <85541613-cf4b-8631-0aa9-2a80ef4b5213@gmail.com>
In-Reply-To: <b0c596d6-f4aa-9575-4d79-64bd112d0baf@github.com>

Hi Stolee

On 30/06/2022 14:35, Derrick Stolee wrote:
> 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.

That's an important distinction that I'd failed to notice

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

That would be nice, even just having a #define for 
"rebase-merge/update-refs" and using that here and in the other patch 
would mean we're not hard coding the filename in two different places.

Best Wishes

Phillip

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