threads / patch / 28724

patch, 12 partsUse refs API more consistently

Subject: [PATCH 00/12] Use refs API more consistently

## tl;dr

18 messages between Oct 19, 2011 and Oct 24, 2011. Diffs are folded; open one to read it.

replies: 17people: 2as markdown or json

mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · lore
From: Michael Haggerty <mhagger@alum.mit.edu>

This patch series primarily has to do with using the refs API more consistently within refs.c itself. We want to minimize the surface area for accessing the ref_cache data structure so that when it is changed into a tree (we're on the verge of that change now, I promise!) we don't have to change callers more than necessary. There is also a bit of an eat-your-own-dogfood thing going on here; if for_each_ref() and its friends are good enough for "outsiders", they should be good enough for most internal use.

The first two patches are trivial preparation cleanups.

The third verifies that refnames read from packed-ref files are properly formatted. When the REFNAME_FULL changes becomes available, this check can be tightened up.

The do_for_each_ref_in_{array,arrays}() functions provide an iterator-like interface to iterating over a single ref_array and iterating over two ref_arrays in parallel. These are useful abstractions in and of themselves (the former is used to reimplement repack_without_ref() and is_refname_available() in patches 9 and 12 respectively). And they will be even more useful when references are stored hierarchically.

This series applies on top of mh/ref-api-2. It conflicts with the "Checking full vs. partial refnames" series that I just submitted but not in any fundamental way. I'll be happy to reconcile them for you in a later round when it is clear if and when each series is accepted.

Michael Haggerty (12):
  Rename another local variable name -> refname
  repack_without_ref(): remove temporary
  parse_ref_line(): add a check that the refname is properly formatted
  create_ref_entry(): extract function from add_ref()
  add_ref(): take a (struct ref_entry *) parameter
  do_for_each_ref(): correctly terminate while processesing extra_refs
  do_for_each_ref_in_array(): new function
  do_for_each_ref_in_arrays(): new function
  repack_without_ref(): reimplement using do_for_each_ref_in_array()
  names_conflict(): new function, extracted from is_refname_available()
  names_conflict(): simplify implementation
  is_refname_available(): reimplement using do_for_each_ref_in_array()
 refs.c |  259 ++++++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 162 insertions(+), 97 deletions(-)
-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 01/12] Rename another local variable name -> refname

From: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

I missed this one earlier. (There are probably others too. I hope you don't feel that I'm morally obliged to find every single analogous example...)

 refs.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)
Show changes to refs.c +11 −11
diff --git a/refs.c b/refs.c
index 7e6cea5..f6752a4 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);
 	}
 }

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 02/12] repack_without_ref(): remove temporary

From: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
Trivial and boring.
 refs.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)
Show changes to refs.c +2 −5
diff --git a/refs.c b/refs.c
index f6752a4..431d3c6 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;

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 03/12] parse_ref_line(): add a check that the refname is properly formatted

From: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
Show changes to refs.c +3 −0
diff --git a/refs.c b/refs.c
index 431d3c6..752938c 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;
 }
 

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 04/12] create_ref_entry(): extract function from add_ref()

From: Michael Haggerty <mhagger@alum.mit.edu>
Separate the creation of the ref_entry from its addition to a ref_array.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   36 +++++++++++++++++++++---------------
 1 files changed, 21 insertions(+), 15 deletions(-)
Show changes to refs.c +21 −15
diff --git a/refs.c b/refs.c
index 752938c..acb098c 100644
--- a/refs.c
+++ b/refs.c
@@ -57,27 +57,33 @@ static const char *parse_ref_line(char *line, unsigned char *sha1)
 	return line;
 }
 
-/* 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)
+static struct ref_entry *create_ref_entry(const char *refname,
+					  const unsigned char *sha1, int flag)
 {
 	int len;
-	struct ref_entry *entry;
+	struct ref_entry *ref;
 
-	/* 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;
+	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_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++] = entry;
+	refs->refs[refs->nr++] = ref;
 }
 
 static int ref_entry_cmp(const void *a, const void *b)

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 05/12] add_ref(): take a (struct ref_entry *) parameter

From: Michael Haggerty <mhagger@alum.mit.edu>

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 <mhagger@alum.mit.edu>
---
 refs.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)
Show changes to refs.c +5 −9
diff --git a/refs.c b/refs.c
index acb098c..ae90993 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);

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 06/12] do_for_each_ref(): correctly terminate while processesing extra_refs

From: Michael Haggerty <mhagger@alum.mit.edu>

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 <mhagger@alum.mit.edu>
---
 refs.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to refs.c +4 −1
diff --git a/refs.c b/refs.c
index ae90993..7aef76c 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;

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 07/12] do_for_each_ref_in_array(): new function

From: Michael Haggerty <mhagger@alum.mit.edu>
Extract function do_for_each_ref_in_array() from do_for_each_ref().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)
Show changes to refs.c +23 −16
diff --git a/refs.c b/refs.c
index 7aef76c..4e60edb 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:

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 08/12] do_for_each_ref_in_arrays(): new function

From: Michael Haggerty <mhagger@alum.mit.edu>
Extract function do_for_each_ref_in_arrays() from do_for_each_ref().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   71 +++++++++++++++++++++++++++++++++++++--------------------------
 1 files changed, 42 insertions(+), 29 deletions(-)
Show changes to refs.c +42 −29
diff --git a/refs.c b/refs.c
index 4e60edb..cd3acf8 100644
--- a/refs.c
+++ b/refs.c
@@ -709,45 +709,58 @@ static int do_for_each_ref_in_array(struct ref_array *array, int offset,
 	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)
+static int do_for_each_ref_in_arrays(struct ref_array *array1,
+				     struct ref_array *array2,
+				     const char *base, each_ref_fn fn, int trim,
+				     int flags, void *cb_data)
 {
-	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);
-
-	retval = do_for_each_ref_in_array(&extra_refs, 0,
-					  base, fn, trim, flags, cb_data);
-	if (retval)
-		goto end_each;
+	int retval;
+	int i1 = 0, i2 = 0;
 
-	while (p < packed->nr && l < loose->nr) {
-		struct ref_entry *entry;
-		int cmp = strcmp(packed->refs[p]->name, loose->refs[l]->name);
-		if (!cmp) {
-			p++;
+	while (1) {
+		struct ref_entry *e1, *e2;
+		int cmp;
+		if (i1 == array1->nr) {
+			return do_for_each_ref_in_array(array2, i2,
+							base, fn, trim, flags, cb_data);
+		}
+		if (i2 == array2->nr) {
+			return do_for_each_ref_in_array(array1, i1,
+							base, fn, trim, flags, cb_data);
+		}
+		e1 = array1->refs[i1];
+		e2 = array2->refs[i2];
+		cmp = strcmp(e1->name, e2->name);
+		if (cmp == 0) {
+			/* Two refs with the same name; ignore the one from array1. */
+			i1++;
 			continue;
 		}
-		if (cmp > 0) {
-			entry = loose->refs[l++];
+		if (cmp < 0) {
+			retval = do_one_ref(base, fn, trim, flags, cb_data, e1);
+			i1++;
 		} else {
-			entry = packed->refs[p++];
+			retval = do_one_ref(base, fn, trim, flags, cb_data, e2);
+			i2++;
 		}
-		retval = do_one_ref(base, fn, trim, flags, cb_data, entry);
 		if (retval)
-			goto end_each;
+			return retval;
 	}
+}
 
-	if (l < loose->nr) {
-		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);
-	}
+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;
+	struct ref_cache *refs = get_ref_cache(submodule);
+
+	retval = do_for_each_ref_in_array(&extra_refs, 0,
+					  base, fn, trim, flags, cb_data);
+	if (!retval)
+		retval = do_for_each_ref_in_arrays(get_packed_refs(refs),
+						   get_loose_refs(refs),
+						   base, fn, trim, flags, cb_data);
 
-end_each:
 	current_ref = NULL;
 	return retval;
 }

-- 
1.7.7
Michael Haggerty· Oct 20, 2011, 07:29 UTC · re: Junio C Hamano · lore

Re: [PATCH 08/12] do_for_each_ref_in_arrays(): new function

On 10/20/2011 12:39 AM, Junio C Hamano wrote:
> Is this necessary?  IOW, is the helper function usable in any context
> other than merge-iterate loose and packed refs?

Soon the packed and cached refs will each be stored hierarchically, so iteration through the combined caches will use a call to do_for_each_ref_in_arrays() in each subdirectory, recursively.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 09/12] repack_without_ref(): reimplement using do_for_each_ref_in_array()

From: Michael Haggerty <mhagger@alum.mit.edu>

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 <mhagger@alum.mit.edu>
---
 refs.c |   46 ++++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 18 deletions(-)
Show changes to refs.c +28 −18
diff --git a/refs.c b/refs.c
index cd3acf8..7c2bcab 100644
--- a/refs.c
+++ b/refs.c
@@ -1213,36 +1213,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);
 }
 

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 10/12] names_conflict(): new function, extracted from is_refname_available()

From: Michael Haggerty <mhagger@alum.mit.edu>

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 <mhagger@alum.mit.edu>
---
 refs.c |   43 +++++++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 12 deletions(-)
Show changes to refs.c +31 −12
diff --git a/refs.c b/refs.c
index 7c2bcab..ada691b 100644
--- a/refs.c
+++ b/refs.c
@@ -1088,6 +1088,30 @@ static int remove_empty_directories(const char *file)
 }
 
 /*
+ * 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
  * is non-NULL, ignore potential conflicts with oldrefname (e.g.,
@@ -1097,20 +1121,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;

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 11/12] names_conflict(): simplify implementation

From: Michael Haggerty <mhagger@alum.mit.edu>
Save a bunch of lines of code and a couple of strlen() calls.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   17 ++++-------------
 1 files changed, 4 insertions(+), 13 deletions(-)
Show changes to refs.c +4 −13
diff --git a/refs.c b/refs.c
index ada691b..4bc6041 100644
--- a/refs.c
+++ b/refs.c
@@ -1096,19 +1096,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');
 }
 
 /*

-- 
1.7.7
mhagger@alum.mit.edu· Oct 19, 2011, 21:44 UTC · re: mhagger@alum.mit.edu · lore

[PATCH 12/12] is_refname_available(): reimplement using do_for_each_ref_in_array()

From: Michael Haggerty <mhagger@alum.mit.edu>

This implementation will survive upcoming changes to the ref_array data structure.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   44 ++++++++++++++++++++++++++++++++------------
 1 files changed, 32 insertions(+), 12 deletions(-)
Show changes to refs.c +32 −12
diff --git a/refs.c b/refs.c
index 4bc6041..c41d995 100644
--- a/refs.c
+++ b/refs.c
@@ -1102,6 +1102,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
@@ -1112,18 +1131,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,

-- 
1.7.7
Michael Haggerty· Oct 20, 2011, 07:46 UTC · re: Junio C Hamano · lore

Re: [PATCH 12/12] is_refname_available(): reimplement using do_for_each_ref_in_array()

On 10/20/2011 03:40 AM, Junio C Hamano wrote:
> Hmm, why is this patch and only this one in the series full of whitespace
> violations? Did you use a different settings or something?

This happens rarely; I don't know why. Maybe I copy-pasted snippets from a view in an application that expanded the tabs. Maybe emacs's eliza program has achieved self-awareness and is punishing me for never having properly learned elisp.

The incorrect lines are indented with 7, not 8, spaces so "tabify" didn't help either.

I'll fix in reroll after I've received any other feedback.
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Michael Haggerty· Oct 24, 2011, 11:58 UTC · re: Michael Haggerty · lore

Re: [PATCH 12/12] is_refname_available(): reimplement using do_for_each_ref_in_array()

On 10/20/2011 09:46 AM, Michael Haggerty wrote:
Show 6 quoted lines
> On 10/20/2011 03:40 AM, Junio C Hamano wrote:
>> Hmm, why is this patch and only this one in the series full of whitespace
>> violations? Did you use a different settings or something?
> 
> This happens rarely; I don't know why.  Maybe I copy-pasted snippets
> from a view in an application that expanded the tabs.  [...]

Now I think I know how this happened. When "git diff"'s output goes to a TTY, it passes its output through the pager. The default pager, less, seems to convert tabs into spaces. I probably copy-pasted some output of diff into my editor then removed the first column of '+' characters.

Just another reason why tabs are evil...
:-)
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

← back to recent threads