From 0eba4d80cc7922b8577860cc02ccb26f7073a68c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:41 +0200 Subject: [PATCH 01/11] Rename another local variable name -> refname Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/refs.c b/refs.c index 7e6cea5458..f6752a489d 100644 --- a/refs.c +++ b/refs.c @@ -317,11 +317,11 @@ static void get_ref_dir(struct ref_cache *refs, const char *base, if (dir) { struct dirent *de; int baselen = strlen(base); - char *ref = xmalloc(baselen + 257); + char *refname = xmalloc(baselen + 257); - memcpy(ref, base, baselen); + memcpy(refname, base, baselen); if (baselen && base[baselen-1] != '/') - ref[baselen++] = '/'; + refname[baselen++] = '/'; while ((de = readdir(dir)) != NULL) { unsigned char sha1[20]; @@ -337,31 +337,31 @@ static void get_ref_dir(struct ref_cache *refs, const char *base, continue; if (has_extension(de->d_name, ".lock")) continue; - memcpy(ref + baselen, de->d_name, namelen+1); + memcpy(refname + baselen, de->d_name, namelen+1); refdir = *refs->name - ? git_path_submodule(refs->name, "%s", ref) - : git_path("%s", ref); + ? git_path_submodule(refs->name, "%s", refname) + : git_path("%s", refname); if (stat(refdir, &st) < 0) continue; if (S_ISDIR(st.st_mode)) { - get_ref_dir(refs, ref, array); + get_ref_dir(refs, refname, array); continue; } if (*refs->name) { hashclr(sha1); flag = 0; - if (resolve_gitlink_ref(refs->name, ref, sha1) < 0) { + if (resolve_gitlink_ref(refs->name, refname, sha1) < 0) { hashclr(sha1); flag |= REF_BROKEN; } } else - if (!resolve_ref(ref, sha1, 1, &flag)) { + if (!resolve_ref(refname, sha1, 1, &flag)) { hashclr(sha1); flag |= REF_BROKEN; } - add_ref(ref, sha1, flag, array, NULL); + add_ref(refname, sha1, flag, array, NULL); } - free(ref); + free(refname); closedir(dir); } } From 117e8b6037ea48b8c72a72970f85ed6ed8530285 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:42 +0200 Subject: [PATCH 02/11] repack_without_ref(): remove temporary Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/refs.c b/refs.c index f6752a489d..431d3c661d 100644 --- a/refs.c +++ b/refs.c @@ -1190,12 +1190,10 @@ static struct lock_file packlock; static int repack_without_ref(const char *refname) { struct ref_array *packed; - struct ref_entry *ref; int fd, i; packed = get_packed_refs(get_ref_cache(NULL)); - ref = search_ref_array(packed, refname); - if (ref == NULL) + if (search_ref_array(packed, refname) == NULL) return 0; fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0); if (fd < 0) { @@ -1206,8 +1204,7 @@ static int repack_without_ref(const char *refname) for (i = 0; i < packed->nr; i++) { char line[PATH_MAX + 100]; int len; - - ref = packed->refs[i]; + struct ref_entry *ref = packed->refs[i]; if (!strcmp(refname, ref->name)) continue; From 011e9393ed442ecddc5c16e6776f78e543afb996 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:43 +0200 Subject: [PATCH 03/11] parse_ref_line(): add a check that the refname is properly formatted Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/refs.c b/refs.c index 431d3c661d..752938c23c 100644 --- a/refs.c +++ b/refs.c @@ -51,6 +51,9 @@ static const char *parse_ref_line(char *line, unsigned char *sha1) return NULL; line[len] = 0; + if (check_refname_format(line, REFNAME_ALLOW_ONELEVEL)) + return NULL; + return line; } From 6ac6c1f4b453b5bb90b27f273f9a143cc27b448d Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:44 +0200 Subject: [PATCH 04/11] create_ref_entry(): extract function from add_ref() Separate the creation of the ref_entry from its addition to a ref_array. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/refs.c b/refs.c index 752938c23c..acb098c3d2 100644 --- a/refs.c +++ b/refs.c @@ -57,27 +57,33 @@ static const char *parse_ref_line(char *line, unsigned char *sha1) return line; } +static struct ref_entry *create_ref_entry(const char *refname, + const unsigned char *sha1, int flag) +{ + int len; + struct ref_entry *ref; + + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT)) + die("Reference has invalid format: '%s'", refname); + len = strlen(refname) + 1; + ref = xmalloc(sizeof(struct ref_entry) + len); + hashcpy(ref->sha1, sha1); + hashclr(ref->peeled); + memcpy(ref->name, refname, len); + ref->flag = flag; + return ref; +} + /* Add a ref_entry to the end of the ref_array (unsorted). */ static void add_ref(const char *refname, const unsigned char *sha1, int flag, struct ref_array *refs, - struct ref_entry **new_entry) + struct ref_entry **new_ref) { - int len; - struct ref_entry *entry; - - /* Allocate it and add it in.. */ - len = strlen(refname) + 1; - entry = xmalloc(sizeof(struct ref_entry) + len); - hashcpy(entry->sha1, sha1); - hashclr(entry->peeled); - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT)) - die("Reference has invalid format: '%s'", refname); - memcpy(entry->name, refname, len); - entry->flag = flag; - if (new_entry) - *new_entry = entry; + struct ref_entry *ref = create_ref_entry(refname, sha1, flag); + if (new_ref) + *new_ref = ref; ALLOC_GROW(refs->refs, refs->nr + 1, refs->alloc); - refs->refs[refs->nr++] = entry; + refs->refs[refs->nr++] = ref; } static int ref_entry_cmp(const void *a, const void *b) From 20be9176340fc1016112b09879c0e3d7a2307fd9 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:45 +0200 Subject: [PATCH 05/11] add_ref(): take a (struct ref_entry *) parameter Take a pointer to the ref_entry to add to the array, rather than creating the ref_entry within the function. This opens the way to having multiple kinds of ref_entries. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/refs.c b/refs.c index acb098c3d2..ae9099300a 100644 --- a/refs.c +++ b/refs.c @@ -75,13 +75,8 @@ static struct ref_entry *create_ref_entry(const char *refname, } /* Add a ref_entry to the end of the ref_array (unsorted). */ -static void add_ref(const char *refname, const unsigned char *sha1, - int flag, struct ref_array *refs, - struct ref_entry **new_ref) +static void add_ref(struct ref_array *refs, struct ref_entry *ref) { - struct ref_entry *ref = create_ref_entry(refname, sha1, flag); - if (new_ref) - *new_ref = ref; ALLOC_GROW(refs->refs, refs->nr + 1, refs->alloc); refs->refs[refs->nr++] = ref; } @@ -266,7 +261,8 @@ static void read_packed_refs(FILE *f, struct ref_array *array) refname = parse_ref_line(refline, sha1); if (refname) { - add_ref(refname, sha1, flag, array, &last); + last = create_ref_entry(refname, sha1, flag); + add_ref(array, last); continue; } if (last && @@ -281,7 +277,7 @@ static void read_packed_refs(FILE *f, struct ref_array *array) void add_extra_ref(const char *refname, const unsigned char *sha1, int flag) { - add_ref(refname, sha1, flag, &extra_refs, NULL); + add_ref(&extra_refs, create_ref_entry(refname, sha1, flag)); } void clear_extra_refs(void) @@ -368,7 +364,7 @@ static void get_ref_dir(struct ref_cache *refs, const char *base, hashclr(sha1); flag |= REF_BROKEN; } - add_ref(refname, sha1, flag, array, NULL); + add_ref(array, create_ref_entry(refname, sha1, flag)); } free(refname); closedir(dir); From 7748941eaafb44dc1d9fb331fd5e0085a33de8f4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:46 +0200 Subject: [PATCH 06/11] do_for_each_ref(): correctly terminate while processesing extra_refs If the user-supplied function returns a nonzero value while processing extra_refs, terminate without processing the rest of the list. This probably has no practical importance, but makes the handling of extra_refs a little bit more consistent with the handling of other refs. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index ae9099300a..7aef76c717 100644 --- a/refs.c +++ b/refs.c @@ -706,8 +706,11 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn struct ref_array *extra = &extra_refs; - for (i = 0; i < extra->nr; i++) + for (i = 0; i < extra->nr; i++) { retval = do_one_ref(base, fn, trim, flags, cb_data, extra->refs[i]); + if (retval) + goto end_each; + } while (p < packed->nr && l < loose->nr) { struct ref_entry *entry; From 3a0faa27a0c213708f4bc34eae22573f8928b365 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:47 +0200 Subject: [PATCH 07/11] do_for_each_ref_in_array(): new function Extract function do_for_each_ref_in_array() from do_for_each_ref(). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/refs.c b/refs.c index 7aef76c717..4e60edba2d 100644 --- a/refs.c +++ b/refs.c @@ -696,21 +696,31 @@ fallback: return -1; } +static int do_for_each_ref_in_array(struct ref_array *array, int offset, + const char *base, + each_ref_fn fn, int trim, int flags, void *cb_data) +{ + int i; + for (i = offset; i < array->nr; i++) { + int retval = do_one_ref(base, fn, trim, flags, cb_data, array->refs[i]); + if (retval) + return retval; + } + return 0; +} + static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn, int trim, int flags, void *cb_data) { - int retval = 0, i, p = 0, l = 0; + int retval = 0, p = 0, l = 0; struct ref_cache *refs = get_ref_cache(submodule); struct ref_array *packed = get_packed_refs(refs); struct ref_array *loose = get_loose_refs(refs); - struct ref_array *extra = &extra_refs; - - for (i = 0; i < extra->nr; i++) { - retval = do_one_ref(base, fn, trim, flags, cb_data, extra->refs[i]); - if (retval) - goto end_each; - } + retval = do_for_each_ref_in_array(&extra_refs, 0, + base, fn, trim, flags, cb_data); + if (retval) + goto end_each; while (p < packed->nr && l < loose->nr) { struct ref_entry *entry; @@ -730,14 +740,11 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn } if (l < loose->nr) { - p = l; - packed = loose; - } - - for (; p < packed->nr; p++) { - retval = do_one_ref(base, fn, trim, flags, cb_data, packed->refs[p]); - if (retval) - goto end_each; + retval = do_for_each_ref_in_array(loose, l, + base, fn, trim, flags, cb_data); + } else { + retval = do_for_each_ref_in_array(packed, p, + base, fn, trim, flags, cb_data); } end_each: From 782aee6c3813782a161c6d4e83386b9ecc5c01d8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:49 +0200 Subject: [PATCH 08/11] repack_without_ref(): reimplement using do_for_each_ref_in_array() It costs a bit of boilerplate, but it means that the function can be ignorant of how cached refs are stored. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/refs.c b/refs.c index 4e60edba2d..38417b9e22 100644 --- a/refs.c +++ b/refs.c @@ -1200,36 +1200,46 @@ struct ref_lock *lock_any_ref_for_update(const char *refname, return lock_ref_sha1_basic(refname, old_sha1, flags, NULL); } +struct repack_without_ref_sb { + const char *refname; + int fd; +}; + +static int repack_without_ref_fn(const char *refname, const unsigned char *sha1, + int flags, void *cb_data) +{ + struct repack_without_ref_sb *data = cb_data; + char line[PATH_MAX + 100]; + int len; + + if (!strcmp(data->refname, refname)) + return 0; + len = snprintf(line, sizeof(line), "%s %s\n", + sha1_to_hex(sha1), refname); + /* this should not happen but just being defensive */ + if (len > sizeof(line)) + die("too long a refname '%s'", refname); + write_or_die(data->fd, line, len); + return 0; +} + static struct lock_file packlock; static int repack_without_ref(const char *refname) { + struct repack_without_ref_sb data; struct ref_array *packed; - int fd, i; packed = get_packed_refs(get_ref_cache(NULL)); if (search_ref_array(packed, refname) == NULL) return 0; - fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0); - if (fd < 0) { + data.refname = refname; + data.fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0); + if (data.fd < 0) { unable_to_lock_error(git_path("packed-refs"), errno); return error("cannot delete '%s' from packed refs", refname); } - - for (i = 0; i < packed->nr; i++) { - char line[PATH_MAX + 100]; - int len; - struct ref_entry *ref = packed->refs[i]; - - if (!strcmp(refname, ref->name)) - continue; - len = snprintf(line, sizeof(line), "%s %s\n", - sha1_to_hex(ref->sha1), ref->name); - /* this should not happen but just being defensive */ - if (len > sizeof(line)) - die("too long a refname '%s'", ref->name); - write_or_die(fd, line, len); - } + do_for_each_ref_in_array(packed, 0, "", repack_without_ref_fn, 0, 0, &data); return commit_lock_file(&packlock); } From 53699dc56be36a7237961b758c7e3ca5d0f21ee0 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:50 +0200 Subject: [PATCH 09/11] names_conflict(): new function, extracted from is_refname_available() This costs an extra strlen() in the loop, but even that small price will be clawed back in the next patch. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/refs.c b/refs.c index 38417b9e22..445dd6369f 100644 --- a/refs.c +++ b/refs.c @@ -1074,6 +1074,30 @@ static int remove_empty_directories(const char *file) return result; } +/* + * Return true iff refname1 and refname2 conflict with each other. + * Two reference names conflict if one of them exactly matches the + * leading components of the other; e.g., "foo/bar" conflicts with + * both "foo" and with "foo/bar/baz" but not with "foo/bar" or + * "foo/barbados". + */ +static int names_conflict(const char *refname1, const char *refname2) +{ + int len1 = strlen(refname1); + int len2 = strlen(refname2); + int cmplen; + const char *lead; + + if (len1 < len2) { + cmplen = len1; + lead = refname2; + } else { + cmplen = len2; + lead = refname1; + } + return !strncmp(refname1, refname2, cmplen) && lead[cmplen] == '/'; +} + /* * Return true iff a reference named refname could be created without * conflicting with the name of an existing reference. If oldrefname @@ -1084,20 +1108,15 @@ static int remove_empty_directories(const char *file) static int is_refname_available(const char *refname, const char *oldrefname, struct ref_array *array) { - int i, namlen = strlen(refname); /* e.g. 'foo/bar' */ + int i; for (i = 0; i < array->nr; i++ ) { struct ref_entry *entry = array->refs[i]; - /* entry->name could be 'foo' or 'foo/bar/baz' */ - if (!oldrefname || strcmp(oldrefname, entry->name)) { - int len = strlen(entry->name); - int cmplen = (namlen < len) ? namlen : len; - const char *lead = (namlen < len) ? entry->name : refname; - if (!strncmp(refname, entry->name, cmplen) && - lead[cmplen] == '/') { - error("'%s' exists; cannot create '%s'", - entry->name, refname); - return 0; - } + if (oldrefname && !strcmp(oldrefname, entry->name)) + continue; + if (names_conflict(refname, entry->name)) { + error("'%s' exists; cannot create '%s'", + entry->name, refname); + return 0; } } return 1; From 44db76087047c3899a6b0e5269e794d6dc165e12 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:51 +0200 Subject: [PATCH 10/11] names_conflict(): simplify implementation Save a bunch of lines of code and a couple of strlen() calls. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/refs.c b/refs.c index 445dd6369f..37bc002dec 100644 --- a/refs.c +++ b/refs.c @@ -1083,19 +1083,10 @@ static int remove_empty_directories(const char *file) */ static int names_conflict(const char *refname1, const char *refname2) { - int len1 = strlen(refname1); - int len2 = strlen(refname2); - int cmplen; - const char *lead; - - if (len1 < len2) { - cmplen = len1; - lead = refname2; - } else { - cmplen = len2; - lead = refname1; - } - return !strncmp(refname1, refname2, cmplen) && lead[cmplen] == '/'; + for (; *refname1 && *refname1 == *refname2; refname1++, refname2++) + ; + return (*refname1 == '\0' && *refname2 == '/') + || (*refname1 == '/' && *refname2 == '\0'); } /* From caa80697e2189695592c03aa7d8832dcda09ea30 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 19 Oct 2011 23:44:52 +0200 Subject: [PATCH 11/11] is_refname_available(): reimplement using do_for_each_ref_in_array() This implementation will survive upcoming changes to the ref_array data structure. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 44 ++++++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/refs.c b/refs.c index 37bc002dec..59f1db2952 100644 --- a/refs.c +++ b/refs.c @@ -1089,6 +1089,25 @@ static int names_conflict(const char *refname1, const char *refname2) || (*refname1 == '/' && *refname2 == '\0'); } +struct name_conflict_cb { + const char *refname; + const char *oldrefname; + const char *conflicting_refname; +}; + +static int name_conflict_fn(const char *existingrefname, const unsigned char *sha1, + int flags, void *cb_data) +{ + struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data; + if (data->oldrefname && !strcmp(data->oldrefname, existingrefname)) + return 0; + if (names_conflict(data->refname, existingrefname)) { + data->conflicting_refname = existingrefname; + return 1; + } + return 0; +} + /* * Return true iff a reference named refname could be created without * conflicting with the name of an existing reference. If oldrefname @@ -1099,18 +1118,19 @@ static int names_conflict(const char *refname1, const char *refname2) static int is_refname_available(const char *refname, const char *oldrefname, struct ref_array *array) { - int i; - for (i = 0; i < array->nr; i++ ) { - struct ref_entry *entry = array->refs[i]; - if (oldrefname && !strcmp(oldrefname, entry->name)) - continue; - if (names_conflict(refname, entry->name)) { - error("'%s' exists; cannot create '%s'", - entry->name, refname); - return 0; - } - } - return 1; + struct name_conflict_cb data; + data.refname = refname; + data.oldrefname = oldrefname; + data.conflicting_refname = NULL; + + if (do_for_each_ref_in_array(array, 0, "", name_conflict_fn, + 0, DO_FOR_EACH_INCLUDE_BROKEN, + &data)) { + error("'%s' exists; cannot create '%s'", + data.conflicting_refname, refname); + return 0; + } + return 1; } static struct ref_lock *lock_ref_sha1_basic(const char *refname,