[PATCH 00/12] Use refs API more consistently

[PATCH 00/12] Use refs API more consistently

[mhagger]

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[Junio C Hamano]

Is this necessary?  IOW, is the helper function usable in any context
other than merge-iterate loose and packed refs?

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

[Michael Haggerty]

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/

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[mhagger]

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(-)

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

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

[Junio C Hamano]

Hmm, why is this patch and only this one in the series full of whitespace
violations? Did you use a different settings or something?

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

[Michael Haggerty]

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/

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

[Michael Haggerty]

On 10/20/2011 09:46 AM, Michael Haggerty wrote:
> 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/