Re: [PATCH v10 02/12] Iterate over the "refs/" namespace in for_each_[raw]ref
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Apr 30, 2020, 21:17 UTC
- Message-ID
- <20200430211714.GE77802@google.com>
- In-Reply-To
- <45fd65f72e097dcabba6ea15b1d54c85e7271593.1588018418.git.gitgitgadget@gmail.com>
On Mon, Apr 27, 2020 at 08:13:28PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
Show 5 quoted lines
> > > This happens implicitly in the files/packed ref backend; making it > explicit simplifies adding alternate ref storage backends, such as > reftable.
As an outsider to this part of the codebase, a little more explanation could be handy in the commit message. I found the backends you mentioned in refs, and it seems like they're the only two, but it's not obvious how this delta is related to those backends. Furthermore, grepping looks like this function whose behavior is changing is being called from somewhere else, with no change to that function (and it looks like the callsite's callback doesn't check whether a ref begins with refs/ or not).
All this to say - it's hard to convince myself this is a safe change, and I'd really like to read more to understand why you made it.
Show 33 quoted lines
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> refs.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index b8759116cd0..05e05579408 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1569,7 +1569,7 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix,
>
> int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
> {
> - return do_for_each_ref(refs, "", fn, 0, 0, cb_data);
> + return do_for_each_ref(refs, "refs/", fn, 0, 0, cb_data);
> }
>
> int for_each_ref(each_ref_fn fn, void *cb_data)
> @@ -1629,8 +1629,8 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
>
> int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
> {
> - return do_for_each_ref(refs, "", fn, 0,
> - DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
> + return do_for_each_ref(refs, "refs/", fn, 0, DO_FOR_EACH_INCLUDE_BROKEN,
> + cb_data);
> }
>
> int for_each_rawref(each_ref_fn fn, void *cb_data)
> --
> gitgitgadget
>