Re: [PATCH v3 2/8] branch: consider refs under 'update-refs'
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 1, 2022, 13:40 UTC
- 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:
Show 19 quoted lines
> 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
Show 11 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