On 10/06/2020 19:05, Han-Wen Nienhuys wrote:

On Tue, Jun 9, 2020 at 12:36 PM Phillip Wood [off-list ref] wrote:

quoted

On 05/06/2020 19:03, Han-Wen Nienhuys via GitGitGadget wrote:

quoted

From: Han-Wen Nienhuys <redacted>

Signed-off-by: Han-Wen Nienhuys <redacted>
---
  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

quoted

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