On 10/06/2020 19:05, Han-Wen Nienhuys wrote:
On Tue, Jun 9, 2020 at 12:36 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote:
From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
---
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
