From: Phillip Wood Date: Thu, 11 Jun 2020 14:59:16 GMT Subject: Re: [PATCH v16 02/14] Make refs_ref_exists public Message-ID: <543c80ab-9e51-ed34-baca-70a1f8b46b2e@gmail.com> In-Reply-To: On 10/06/2020 19:05, Han-Wen Nienhuys wrote: > On Tue, Jun 9, 2020 at 12:36 PM Phillip Wood wrote: >> >> On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote: >>> From: Han-Wen Nienhuys >>> >>> Signed-off-by: Han-Wen Nienhuys >>> --- >>> refs.c | 2 +- >>> refs.h | 2 ++ >>> 2 files changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/refs.c b/refs.c >>> index 12908066b13..812fee47108 100644 >>> --- a/refs.c >>> +++ b/refs.c >>> @@ -311,7 +311,7 @@ int read_ref(const char *refname, struct object_id *oid) >>> return read_ref_full(refname, RESOLVE_REF_READING, oid, NULL); >>> } >>> >>> -static int refs_ref_exists(struct ref_store *refs, const char *refname) >>> +int refs_ref_exists(struct ref_store *refs, const char *refname) >>> { >>> return !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING, NULL, NULL); >>> } >> >> It is a shame that ref_exists() does not take a struct repository. The > > I'm trying to follow the pattern that the rest of the code > establishes; you're right that the code isn't very consistent (the > fact that it uses unlink() rather than go through the ref store in the > first place is an indication of that). I want to avoid starting a > general cleanup tour of the code base, the reftable patch is hard > enough to pull off without. Sure >> The existing code is inconsistent about its repository handling - we >> create the refs with update_ref() which operates on the main repository >> but when checking their existence and deleting them we use a path which >> depends on the repository. I've realized now the answer to my question >> about using delete_ref() in my reply to the previous patch - it does not >> take a repository - maybe it should along with update_ref() but that >> might be more work than you want to do though. > > Why do they take repository arguments anyway? Is it because > rebase/cherry-pick supports recursion into submodules? It was a stepping stone towards that, the git_path mechanism that is used to create git_path_cherry_pick_head() etc was changed to take a struct repository so it could support submodules without forking a separate process. However are still plenty of places where the sequencer code assumes a single repository (it calls update_ref(), delete_ref(), commit_tree_extended(), ...) and the two contributors who did a lot of that work have moved on. With that in mind perhaps we'd be better off just using ref_exists() and delete_ref() in this conversion. The call sites will be easy enough to fixup if those functions are converted to take a struct repository in the future and the result of this patch series will be nicer. I've cc'd dscho and Junio to see what they think. Best Wishes Phillip