Re: [PATCH v5 25/26] refs: break out ref conflict checks
On 10/28/2015 03:14 AM, David Turner wrote: > Create new function verify_no_descendants, to hold one of the ref > conflict checks used in verify_refname_available. Multiple backends > will need this function, so it goes in the common code. > > rename_ref_available also moves to the common code, because alternate > backends might need it and it has no files-backend-specific code. It is preferred that log messages be written in the imperative voice: ... so move it to the common code. Also move rename_ref_available ... > Signed-off-by: David Turner > Signed-off-by: Junio C Hamano > Signed-off-by: Michael Haggerty > --- > refs-be-files.c | 49 - > refs.c | 38 ++ > refs.h | 15 +++ > 3 files changed, 61 insertions(+), 41 deletions(-) > > diff --git a/refs-be-files.c b/refs-be-files.c > index 0eabea9..97a5983 100644 > --- a/refs-be-files.c > +++ b/refs-be-files.c > @@ -729,6 +729,7 @@ static int verify_refname_available_dir(const char > *refname, > struct strbuf *err) > { > const char *slash; > + const char *extra_refname; > int pos; > struct strbuf dirname = STRBUF_INIT; > int ret = -1; > @@ -834,33 +835,15 @@ static int verify_refname_available_dir(const char > *refname, > } > } > > - if (extras) { > - /* > - * Check for entries in extras that start with > - * "$refname/". We do that by looking for the place > - * where "$refname/" would be inserted in extras. If > - * there is an entry at that position that starts with > - * "$refname/" and is not in skip, then we have a > - * conflict. > - */ In your version, the above comment is replaced with /* Look for the place where dirname would be inserted in extras. */ which is pretty obvious, I think, from the following line. It's debatable whether the long-winded old comment was worthwhile, but I think the new comment is clearly not. I suggest either restoring a longer explanation or eliminating the comment altogether. > - for (pos = string_list_find_insert_index(extras, dirname.buf, > 0); > - pos < extras->nr; pos++) { > - const char *extra_refname = extras->items[pos].string; > - > - if (!starts_with(extra_refname, dirname.buf)) > - break; > - > - if (!skip || !string_list_has_string(skip, > extra_refname)) { > - strbuf_addf(err, "cannot process '%s' and '%s' > at the same time", > - refname, extra_refname); > - goto cleanup; > - } > - } > + 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; > } Project practice is to leave out unnecessary braces. > > - /* No conflicts were found */ > - ret = 0; > - > cleanup: > strbuf_release(&dirname); > return ret; > @@ -2460,22 +2443,6 @@ out: > [...] > diff --git a/refs.c b/refs.c > index 056c172..5d8b6ea 100644 > --- a/refs.c > +++ b/refs.c > [...] > diff --git a/refs.h b/refs.h > index f97a2e4..88fea3e 100644 > --- a/refs.h > +++ b/refs.h > [...] > @@ -623,6 +625,19 @@ int files_log_ref_write(const char *refname, const > unsigned char *old_sha1, > const unsigned char *new_sha1, const char *msg, > int flags, struct strbuf *err); > > +/* > + * Check for entries in extras that are within the specified > + * directory, where dirname is a reference directory name including > + * the trailing slash (e.g., "refs/heads/master/"). Ignore any > + * conflicting references that are found in skip. If there is a > + * conflicting reference, return its name. > + * > + * extras and skip must be sorted lists of reference names. skip can > + * be NULL; extras cannot. The last sentence is incorrect; the function can handle extras == NULL. > + */ > +const char *find_descendant_ref(const char *dirname, > + const struct string_list *extras, > + const struct string_list *skip); > > enum expire_reflog_flags { > EXPIRE_REFLOGS_DRY_RUN = 1 << 0, > Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 25/26] refs: break out ref conflict checks
Create new function verify_no_descendants, to hold one of the ref conflict checks used in verify_refname_available. Multiple backends will need this function, so it goes in the common code. rename_ref_available also moves to the common code, because alternate backends might need it and it has no files-backend-specific code. Signed-off-by: David Turner Signed-off-by: Junio C Hamano Signed-off-by: Michael Haggerty --- refs-be-files.c | 49 - refs.c | 38 ++ refs.h | 15 +++ 3 files changed, 61 insertions(+), 41 deletions(-) diff --git a/refs-be-files.c b/refs-be-files.c index 0eabea9..97a5983 100644 --- a/refs-be-files.c +++ b/refs-be-files.c @@ -729,6 +729,7 @@ static int verify_refname_available_dir(const char *refname, struct strbuf *err) { const char *slash; + const char *extra_refname; int pos; struct strbuf dirname = STRBUF_INIT; int ret = -1; @@ -834,33 +835,15 @@ static int verify_refname_available_dir(const char *refname, } } - if (extras) { - /* -* Check for entries in extras that start with -* "$refname/". We do that by looking for the place -* where "$refname/" would be inserted in extras. If -* there is an entry at that position that starts with -* "$refname/" and is not in skip, then we have a -* conflict. -*/ - for (pos = string_list_find_insert_index(extras, dirname.buf, 0); -pos < extras->nr; pos++) { - const char *extra_refname = extras->items[pos].string; - - if (!starts_with(extra_refname, dirname.buf)) - break; - - if (!skip || !string_list_has_string(skip, extra_refname)) { - strbuf_addf(err, "cannot process '%s' and '%s' at the same time", - refname, extra_refname); - goto cleanup; - } - } + 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; } - /* No conflicts were found */ - ret = 0; - cleanup: strbuf_release(&dirname); return ret; @@ -2460,22 +2443,6 @@ out: return ret; } -static int rename_ref_available(const char *oldname, const char *newname) -{ - struct string_list skip = STRING_LIST_INIT_NODUP; - struct strbuf err = STRBUF_INIT; - int ret; - - string_list_insert(&skip, oldname); - ret = !verify_refname_available(newname, NULL, &skip, &err); - if (!ret) - error("%s", err.buf); - - string_list_clear(&skip, 0); - strbuf_release(&err); - return ret; -} - static int write_ref_to_lockfile(struct ref_lock *lock, const unsigned char *sha1, struct strbuf *err); static int commit_ref_update(struct ref_lock *lock, diff --git a/refs.c b/refs.c index 056c172..5d8b6ea 100644 --- a/refs.c +++ b/refs.c @@ -1027,3 +1027,41 @@ enum peel_status peel_object(const unsigned char *name, unsigned char *sha1) hashcpy(sha1, o->sha1); return PEEL_PEELED; } + +const char *find_descendant_ref(const char *dirname, + const struct string_list *extras, + const struct string_list *skip) +{ + int pos; + if (!extras) + return NULL; + + /* Look for the place where dirname would be inserted in extras. */ + for (pos = string_list_find_insert_index(extras, dirname, 0); +pos < extras->nr; pos++) { + const char *extra_refname = extras->items[pos].string; + + if (!starts_with(extra_refname, dirname)) + break; + + if (!skip || !string_list_has_string(skip, extra_refname)) + return extra_refname; + } + return NULL; +} + +int rename_ref_available(const char *oldname, const char *newname) +{ + struct string_list skip = STRING_LIST_INIT_NODUP; + struct strbuf err = STRBUF_INIT; + int ret; + + string_list_insert(&skip, oldname); + ret = !verify_refname_available(newname, NULL, &skip, &err); + if (!ret) + error("%s", err.buf); + + string_list_clear(&skip, 0); + strbuf_release(&err); + return ret; +} diff --git a/refs.h b/refs.h index f97a2e4..88fea3e 100644 --- a/refs.h +++ b/refs.h @@ -362,6 +362,8 @@ int pack_refs(unsig