Re: [PATCH v2 04/20] refs_verify_refname_available(): implement once for all backends

[Michael Haggerty]

On 04/07/2017 01:20 PM, Duy Nguyen wrote:
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  
> wrote:
>> It turns out that we can now implement
>> `refs_verify_refname_available()` based on the other virtual
>> functions, so there is no need for it to be defined at the backend
>> level. Instead, define it once in `refs.c` and remove the
>> `files_backend` definition.
>>
>> Signed-off-by: Michael Haggerty 
>> ---
>>  refs.c   | 85 
>> ++--
>>  refs.h   |  2 +-
>>  refs/files-backend.c | 39 +---
> 
> Much appreciated. This will make future backends simpler to implement as well.
> 
>> +   iter = refs_ref_iterator_begin(refs, dirname.buf, 0,
>> +  DO_FOR_EACH_INCLUDE_BROKEN);
>> +   while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
>> +   if (skip &&
>> +   string_list_has_string(skip, iter->refname))
>> +   continue;
>> +
>> +   strbuf_addf(err, "'%s' exists; cannot create '%s'",
>> +   iter->refname, refname);
>> +   ok = ref_iterator_abort(iter);
> 
> Saving the return code in "ok" seems redundant because you don't use
> it. Or did you want to check that ok == ITER_DONE or die() too?

True, setting `ok` here is redundant. I don't think there's much point
worrying about whether `ref_iterator_abort()` fails here, since we've
already gotten the information that we require.

I'll remove it.

Thanks,
Michael

Re: [PATCH v2 04/20] refs_verify_refname_available(): implement once for all backends

[Duy Nguyen]

On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  wrote:
> It turns out that we can now implement
> `refs_verify_refname_available()` based on the other virtual
> functions, so there is no need for it to be defined at the backend
> level. Instead, define it once in `refs.c` and remove the
> `files_backend` definition.
>
> Signed-off-by: Michael Haggerty 
> ---
>  refs.c   | 85 
> ++--
>  refs.h   |  2 +-
>  refs/files-backend.c | 39 +---

Much appreciated. This will make future backends simpler to implement as well.

> +   iter = refs_ref_iterator_begin(refs, dirname.buf, 0,
> +  DO_FOR_EACH_INCLUDE_BROKEN);
> +   while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
> +   if (skip &&
> +   string_list_has_string(skip, iter->refname))
> +   continue;
> +
> +   strbuf_addf(err, "'%s' exists; cannot create '%s'",
> +   iter->refname, refname);
> +   ok = ref_iterator_abort(iter);

Saving the return code in "ok" seems redundant because you don't use
it. Or did you want to check that ok == ITER_DONE or die() too?

> +   goto cleanup;
> +   }
> +
> +   if (ok != ITER_DONE)
> +   die("BUG: error while iterating over references");
> +
> +   extra_refname = find_descendant_ref(dirname.buf, extras, skip);
> +   if (extra_refname)
> +   strbuf_addf(err, "cannot process '%s' and '%s' at the same 
> time",
> +   refname, extra_refname);
> +   else
> +   ret = 0;
> +
> +cleanup:
> +   strbuf_release();
> +   strbuf_release();
> +   return ret;
>  }
-- 
Duy

[PATCH v2 04/20] refs_verify_refname_available(): implement once for all backends

[Michael Haggerty]

It turns out that we can now implement
`refs_verify_refname_available()` based on the other virtual
functions, so there is no need for it to be defined at the backend
level. Instead, define it once in `refs.c` and remove the
`files_backend` definition.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 85 ++--
 refs.h   |  2 +-
 refs/files-backend.c | 39 +---
 refs/refs-internal.h |  7 -
 4 files changed, 92 insertions(+), 41 deletions(-)

diff --git a/refs.c b/refs.c
index aeb704ab92..2cf531b9c7 100644
--- a/refs.c
+++ b/refs.c
@@ -5,6 +5,7 @@
 #include "cache.h"
 #include "hashmap.h"
 #include "lockfile.h"
+#include "iterator.h"
 #include "refs.h"
 #include "refs/refs-internal.h"
 #include "object.h"
@@ -1661,11 +1662,91 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
 int refs_verify_refname_available(struct ref_store *refs,
  const char *refname,
- const struct string_list *extra,
+ const struct string_list *extras,
  const struct string_list *skip,
  struct strbuf *err)
 {
-   return refs->be->verify_refname_available(refs, refname, extra, skip, 
err);
+   const char *slash;
+   const char *extra_refname;
+   struct strbuf dirname = STRBUF_INIT;
+   struct strbuf referent = STRBUF_INIT;
+   struct object_id oid;
+   unsigned int type;
+   struct ref_iterator *iter;
+   int ok;
+   int ret = -1;
+
+   /*
+* For the sake of comments in this function, suppose that
+* refname is "refs/foo/bar".
+*/
+
+   assert(err);
+
+   strbuf_grow(, strlen(refname) + 1);
+   for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, 
'/')) {
+   /* Expand dirname to the new prefix, not including the trailing 
slash: */
+   strbuf_add(, refname + dirname.len, slash - refname - 
dirname.len);
+
+   /*
+* We are still at a leading dir of the refname (e.g.,
+* "refs/foo"; if there is a reference with that name,
+* it is a conflict, *unless* it is in skip.
+*/
+   if (skip && string_list_has_string(skip, dirname.buf))
+   continue;
+
+   if (!refs_read_raw_ref(refs, dirname.buf, oid.hash, , 
)) {
+   strbuf_addf(err, "'%s' exists; cannot create '%s'",
+   dirname.buf, refname);
+   goto cleanup;
+   }
+
+   if (extras && string_list_has_string(extras, dirname.buf)) {
+   strbuf_addf(err, "cannot process '%s' and '%s' at the 
same time",
+   refname, dirname.buf);
+   goto cleanup;
+   }
+   }
+
+   /*
+* We are at the leaf of our refname (e.g., "refs/foo/bar").
+* There is no point in searching for a reference with that
+* name, because a refname isn't considered to conflict with
+* itself. But we still need to check for references whose
+* names are in the "refs/foo/bar/" namespace, because they
+* *do* conflict.
+*/
+   strbuf_addstr(, refname + dirname.len);
+   strbuf_addch(, '/');
+
+   iter = refs_ref_iterator_begin(refs, dirname.buf, 0,
+  DO_FOR_EACH_INCLUDE_BROKEN);
+   while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
+   if (skip &&
+   string_list_has_string(skip, iter->refname))
+   continue;
+
+   strbuf_addf(err, "'%s' exists; cannot create '%s'",
+   iter->refname, refname);
+   ok = ref_iterator_abort(iter);
+   goto cleanup;
+   }
+
+   if (ok != ITER_DONE)
+   die("BUG: error while iterating over references");
+
+   extra_refname = find_descendant_ref(dirname.buf, extras, skip);
+   if (extra_refname)
+   strbuf_addf(err, "cannot process '%s' and '%s' at the same 
time",
+   refname, extra_refname);
+   else
+   ret = 0;
+
+cleanup:
+   strbuf_release();
+   strbuf_release();
+   return ret;
 }
 
 int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
diff --git a/refs.h b/refs.h
index 49e97d7d5f..07cf4cd41b 100644
--- a/refs.h
+++ b/refs.h
@@ -97,7 +97,7 @@ int read_ref(const char *refname, unsigned char *sha1);
 
 int refs_verify_refname_available(struct ref_store *refs,
  const char *refname,
- const struct string_list *extra,
+ const struct string_list *extras,