[PATCH v2 03/11] resolve_ref_unsafe(): reverse the logic of the symref conditional

[Michael Haggerty]

It is clearer if the check whether a loose reference file is a symref
is followed immediately by the code to handle the symref, rather than
the current code, which has the if statement the other way around.

Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
refs.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index ceba23c..771941b 100644
--- a/refs.c
+++ b/refs.c
@@ -1497,35 +1497,35 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
len--;
buffer[len] = '\0';

- /*
- * Is it a symbolic ref?
- */
- if (!starts_with(buffer, "ref:")) {
- /*
- * Please note that FETCH_HEAD has a second
- * line containing other data.
- */
- if (get_sha1_hex(buffer, sha1) ||
- (buffer[40] != '\0' && !isspace(buffer[40]))) {
+ if (starts_with(buffer, "ref:")) {
+ /* It is a symbolic ref */
+ if (flag)
+ *flag |= REF_ISSYMREF;
+ buf = buffer + 4;
+ while (isspace(*buf))
+ buf++;
+ if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
if (flag)
*flag |= REF_ISBROKEN;
errno = EINVAL;
return NULL;
}
- return refname;
+ refname = strcpy(refname_buffer, buf);
+ continue;
}
- if (flag)
- *flag |= REF_ISSYMREF;
- buf = buffer + 4;
- while (isspace(*buf))
- buf++;
- if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
+
+ /*
+ * It must be a normal ref. Please note that
+ * FETCH_HEAD has a second line containing other data.
+ */
+ if (get_sha1_hex(buffer, sha1) ||
+ (buffer[40] != '\0' && !isspace(buffer[40]))) {
if (flag)
*flag |= REF_ISBROKEN;
errno = EINVAL;
return NULL;
}
- refname = strcpy(refname_buffer, buf);
+ return refname;
}
}

[Michael Haggerty]

=46rom: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail.com>

The return value of handle_missing_loose_ref() was either NULL to
signify an error or the input parameter refname on success. So instead
of returning a string, just return a 0 on success or -1 on error, so
the reader doesn't have to wonder what string the return value points
at.

Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
refs.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index ffd45e9..ceba23c 100644
--- a/refs.c
+++ b/refs.c
@@ -1370,10 +1370,10 @@ static struct ref_entry *get_packed_ref(const c=
har *refname)
* A loose ref file doesn't exist; check for a packed ref. The
* options are forwarded from resolve_safe_unsafe().
*/
-static const char *handle_missing_loose_ref(const char *refname,
- unsigned char *sha1,
- int reading,
- int *flag)
+static int handle_missing_loose_ref(const char *refname,
+ unsigned char *sha1,
+ int reading,
+ int *flag)
{
struct ref_entry *entry;
=20
@@ -1386,14 +1386,14 @@ static const char *handle_missing_loose_ref(con=
st char *refname,
hashcpy(sha1, entry->u.value.sha1);
if (flag)
*flag |=3D REF_ISPACKED;
- return refname;
+ return 0;
}
/* The reference is not a packed reference, either. */
if (reading) {
- return NULL;
+ return -1;
} else {
hashclr(sha1);
- return refname;
+ return 0;
}
}
=20
@@ -1437,10 +1437,12 @@ const char *resolve_ref_unsafe(const char *refn=
ame, unsigned char *sha1, int rea
*/
stat_ref:
if (lstat(path, &st) < 0) {
- if (errno =3D=3D ENOENT)
- return handle_missing_loose_ref(refname, sha1,
- reading, flag);
- else
+ if (errno =3D=3D ENOENT) {
+ if (handle_missing_loose_ref(refname, sha1,
+ reading, flag))
+ return NULL;
+ return refname;
+ } else
return NULL;
}
=20
--=20
2.1.1

[Michael Haggerty]

=46rom: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail.com>

In the beginning, we had resolve_ref() that returns a buffer owned by
this function. Then we started to move away from that direction because
the buffer could be overwritten by the next resolve_ref() call and
introduced two new functions: resolve_ref_unsafe() and resolve_refdup()=
=2E
The static buffer is still kept internally.

This patch makes the core of resolve_ref use a strbuf instead of static
buffer. Which makes resolve_refdup() more efficient (no need to copy
from the static buffer to a new buffer). It also removes the (random?)
256 char limit. In future, resolve_ref() could be used directly without
going through resolve_refdup() wrapper.

A minor bonus. resolve_ref(dup) are now more thread-friendly (although =
I'm
not 100% sure if they are thread-safe yet).

Signed-off-by: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail=
=2Ecom>
Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
cache.h | 1 +
refs.c | 115 +++++++++++++++++++++++++++++++++++++-------------------=
--------
2 files changed, 67 insertions(+), 49 deletions(-)

diff --git a/cache.h b/cache.h
index 3e6a914..e36084d 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,6 +1004,7 @@ extern int read_ref(const char *refname, unsigned=
char *sha1);
*/
extern const char *resolve_ref_unsafe(const char *ref, unsigned char *=
sha1, int reading, int *flag);
extern char *resolve_refdup(const char *ref, unsigned char *sha1, int =
reading, int *flag);
+extern int resolve_ref(const char *refname, struct strbuf *result, uns=
igned char *sha1, int reading, int *flag);
=20
extern int dwim_ref(const char *str, int len, unsigned char *sha1, cha=
r **ref);
extern int dwim_log(const char *str, int len, unsigned char *sha1, cha=
r **ref);
diff --git a/refs.c b/refs.c
index 020ee3f..29ea7e0 100644
--- a/refs.c
+++ b/refs.c
@@ -1397,34 +1397,41 @@ static int handle_missing_loose_ref(const char =
*refname,
}
}
=20
-/* This function needs to return a meaningful errno on failure */
-const char *resolve_ref_unsafe(const char *refname, unsigned char *sha=
1, int reading, int *flag)
+/*
+ * 'result' content will be destroyed. Its value may be undefined if
+ * resolve_ref returns -1.
+ *
+ * This function needs to return a meaningful errno on failure
+ */
+int resolve_ref(const char *refname, struct strbuf *result,
+ unsigned char *sha1, int reading, int *flag)
{
+ struct strbuf buffer =3D STRBUF_INIT;
int depth =3D MAXDEPTH;
- ssize_t len;
- char buffer[256];
- static char refname_buffer[256];
+ int ret =3D -1;
=20
if (flag)
*flag =3D 0;
=20
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
errno =3D EINVAL;
- return NULL;
+ return -1;
}
=20
+ strbuf_reset(result);
+ strbuf_addstr(result, refname);
+
for (;;) {
char path[PATH_MAX];
struct stat st;
const char *buf;
- int fd;
=20
if (--depth < 0) {
errno =3D ELOOP;
- return NULL;
+ break;
}
=20
- git_snpath(path, sizeof(path), "%s", refname);
+ git_snpath(path, sizeof(path), "%s", result->buf);
=20
/*
* We might have to loop back here to avoid a race
@@ -1437,30 +1444,26 @@ const char *resolve_ref_unsafe(const char *refn=
ame, unsigned char *sha1, int rea
*/
stat_ref:
if (lstat(path, &st) < 0) {
- if (errno =3D=3D ENOENT) {
- if (handle_missing_loose_ref(refname, sha1,
- reading, flag))
- return NULL;
- return refname;
- } else
- return NULL;
+ if (errno =3D=3D ENOENT)
+ ret =3D handle_missing_loose_ref(result->buf, sha1,
+ reading, flag);
+ break;
}
=20
/* Follow "normalized" - ie "refs/.." symlinks by hand */
if (S_ISLNK(st.st_mode)) {
- len =3D readlink(path, buffer, sizeof(buffer)-1);
- if (len < 0) {
+ /* no need to reset buffer, strbuf_readlink does that */
+ if (strbuf_readlink(&buffer, path, 256) < 0) {
if (errno =3D=3D ENOENT || errno =3D=3D EINVAL)
/* inconsistent with lstat; retry */
goto stat_ref;
else
- return NULL;
+ break;
}
- buffer[len] =3D 0;
- if (starts_with(buffer, "refs/") &&
- !check_refname_format(buffer, 0)) {
- strcpy(refname_buffer, buffer);
- refname =3D refname_buffer;
+ if (starts_with(buffer.buf, "refs/") &&
+ !check_refname_format(buffer.buf, 0)) {
+ strbuf_reset(result);
+ strbuf_addbuf(result, &buffer);
if (flag)
*flag |=3D REF_ISSYMREF;
continue;
@@ -1470,34 +1473,24 @@ const char *resolve_ref_unsafe(const char *refn=
ame, unsigned char *sha1, int rea
/* Is it a directory? */
if (S_ISDIR(st.st_mode)) {
errno =3D EISDIR;
- return NULL;
+ break;
}
=20
/*
* Anything else, just open it and try to use it as
* a ref
*/
- fd =3D open(path, O_RDONLY);
- if (fd < 0) {
+ strbuf_reset(&buffer);
+ if (strbuf_read_file(&buffer, path, 256) < 0) {
if (errno =3D=3D ENOENT)
/* inconsistent with lstat; retry */
goto stat_ref;
else
- return NULL;
- }
- len =3D read_in_full(fd, buffer, sizeof(buffer)-1);
- if (len < 0) {
- int save_errno =3D errno;
- close(fd);
- errno =3D save_errno;
- return NULL;
+ break;
}
- close(fd);
- while (len && isspace(buffer[len-1]))
- len--;
- buffer[len] =3D '\0';
+ strbuf_rtrim(&buffer);
=20
- if (skip_prefix(buffer, "ref:", &buf)) {
+ if (skip_prefix(buffer.buf, "ref:", &buf)) {
/* It is a symbolic ref */
if (flag)
*flag |=3D REF_ISSYMREF;
@@ -1507,9 +1500,10 @@ const char *resolve_ref_unsafe(const char *refna=
me, unsigned char *sha1, int rea
if (flag)
*flag |=3D REF_ISBROKEN;
errno =3D EINVAL;
- return NULL;
+ break;
}
- refname =3D strcpy(refname_buffer, buf);
+ strbuf_reset(result);
+ strbuf_add(result, buf, buffer.buf + buffer.len - buf);
continue;
}
=20
@@ -1517,21 +1511,44 @@ const char *resolve_ref_unsafe(const char *refn=
ame, unsigned char *sha1, int rea
* It must be a normal ref. Please note that
* FETCH_HEAD has a second line containing other data.
*/
- if (get_sha1_hex(buffer, sha1) ||
- (buffer[40] !=3D '\0' && !isspace(buffer[40]))) {
+ if (get_sha1_hex(buffer.buf, sha1) ||
+ (buffer.buf[40] !=3D '\0' && !isspace(buffer.buf[40]))) {
if (flag)
*flag |=3D REF_ISBROKEN;
errno =3D EINVAL;
- return NULL;
- }
- return refname;
+ } else
+ ret =3D 0;
+ break;
}
+ strbuf_release(&buffer);
+ return ret;
+}
+
+const char *resolve_ref_unsafe(const char *refname, unsigned char *sha=
1, int reading, int *flag)
+{
+ static struct strbuf buf =3D STRBUF_INIT;
+
+ if (!resolve_ref(refname, &buf, sha1, reading, flag))
+ /*
+ * Please note: true to the "unsafe" name of this
+ * function, we return a pointer to our internal
+ * memory here rather than passing ownership to the
+ * caller by calling strbuf_detach():
+ */
+ return buf.buf;
+ else
+ return NULL;
}
=20
char *resolve_refdup(const char *ref, unsigned char *sha1, int reading=
, int *flag)
{
- const char *ret =3D resolve_ref_unsafe(ref, sha1, reading, flag);
- return ret ? xstrdup(ret) : NULL;
+ struct strbuf buf =3D STRBUF_INIT;
+ if (!resolve_ref(ref, &buf, sha1, reading, flag))
+ return strbuf_detach(&buf, NULL);
+ else {
+ strbuf_release(&buf);
+ return NULL;
+ }
}
=20
/* The argument to filter_refs */
--=20
2.1.1

[Michael Haggerty]

=46rom: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail.com>

resolve_gitlink_ref_recursive() did about the same thing as
parse_ref(), but didn't know as many tricks. It also had another
random limit of 128 bytes for symrefs. So base resolve_gitlink_ref()
on parse_ref() instead.

Signed-off-by: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail=
=2Ecom>
Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
refs.c | 69 ++++++++++++++++++++++++++--------------------------------=
--------
1 file changed, 27 insertions(+), 42 deletions(-)

diff --git a/refs.c b/refs.c
index e1aa6a4..a7c8abd 100644
--- a/refs.c
+++ b/refs.c
@@ -1299,48 +1299,11 @@ static int resolve_gitlink_packed_ref(struct re=
f_cache *refs,
return 0;
}
=20
-static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
- const char *refname, unsigned char *sha1,
- int recursion)
-{
- int fd, len;
- char buffer[128], *p;
- char *path;
-
- if (recursion > MAXDEPTH)
- return -1;
- path =3D *refs->name
- ? git_path_submodule(refs->name, "%s", refname)
- : git_path("%s", refname);
- fd =3D open(path, O_RDONLY);
- if (fd < 0)
- return resolve_gitlink_packed_ref(refs, refname, sha1);
-
- len =3D read(fd, buffer, sizeof(buffer)-1);
- close(fd);
- if (len < 0)
- return -1;
- while (len && isspace(buffer[len-1]))
- len--;
- buffer[len] =3D 0;
-
- /* Was it a detached head or an old-fashioned symlink? */
- if (!get_sha1_hex(buffer, sha1))
- return 0;
-
- /* Symref? */
- if (strncmp(buffer, "ref:", 4))
- return -1;
- p =3D buffer + 4;
- while (isspace(*p))
- p++;
-
- return resolve_gitlink_ref_recursive(refs, p, sha1, recursion+1);
-}
-
int resolve_gitlink_ref(const char *path, const char *refname, unsigne=
d char *sha1)
{
- int len =3D strlen(path), retval;
+ struct strbuf result =3D STRBUF_INIT;
+ int len =3D strlen(path), parseval, ret;
+ int depth =3D MAXDEPTH;
char *submodule;
struct ref_cache *refs;
=20
@@ -1352,8 +1315,30 @@ int resolve_gitlink_ref(const char *path, const =
char *refname, unsigned char *sh
refs =3D get_ref_cache(submodule);
free(submodule);
=20
- retval =3D resolve_gitlink_ref_recursive(refs, refname, sha1, 0);
- return retval;
+ strbuf_addstr(&result, refname);
+ do {
+ if (--depth < 0) {
+ errno =3D ELOOP;
+ ret =3D -1;
+ goto out;
+ }
+ path =3D *refs->name
+ ? git_path_submodule(refs->name, "%s", result.buf)
+ : git_path("%s", result.buf);
+ parseval =3D parse_ref(path, &result, sha1, NULL);
+ } while (!parseval);
+
+ if (parseval =3D=3D 1) {
+ ret =3D 0;
+ } else if (parseval =3D=3D -2) {
+ ret =3D resolve_gitlink_packed_ref(refs, result.buf, sha1) ? -1 : 0;
+ } else {
+ ret =3D -1;
+ }
+
+out:
+ strbuf_release(&result);
+ return ret;
}
=20
/*
--=20
2.1.1

[Michael Haggerty]

=46rom: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail.com>

This will allow the function to be used in a later patch.

Signed-off-by: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail=
=2Ecom>
Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
strbuf.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 0346e74..f1fec58 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -482,15 +482,18 @@ int strbuf_getwholeline_fd(struct strbuf *sb, int=
fd, int term)
=20
int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
{
- int fd, len;
+ int fd, len, saved_errno;
=20
fd =3D open(path, O_RDONLY);
if (fd < 0)
return -1;
len =3D strbuf_read(sb, fd, hint);
+ saved_errno =3D errno;
close(fd);
- if (len < 0)
+ if (len < 0) {
+ errno =3D saved_errno;
return -1;
+ }
=20
return len;
}
--=20
2.1.1

[Michael Haggerty]

At this point we know that refs->name is a non-empty string, so we
know we don't have to look up the reference in the main repository.

Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
refs.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 8edcc3b..3f4b95a 100644
--- a/refs.c
+++ b/refs.c
@@ -1303,9 +1303,7 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh
ret = -1;
goto out;
}
- path = *refs->name
- ? git_path_submodule(refs->name, "%s", result.buf)
- : git_path("%s", result.buf);
+ path = git_path_submodule(refs->name, "%s", result.buf);
parseval = parse_ref(path, &result, sha1, NULL);
} while (!parseval);

[Michael Haggerty]

This limit was added in

0ebde32 (Add 'resolve_gitlink_ref()' helper function - 2007-04-09)

Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
Theoretically, somebody else might be relying on
resolve_gitlink_ref_recursive() to fail for too-long reference names
to prevent path.c's pitiful error handling from returning "/bad-path/"
and causing a nonsensical file lookup. I doubt it, but if somebody is
worried about it we could leave out this patch and instead build the
MAXREFLEN check into parse_ref().

Long-term, I think we should fix up path.c to remove its PATH_MAX
limits. I've started working on that.

refs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 9f2a0f8..e1aa6a4 100644
--- a/refs.c
+++ b/refs.c
@@ -1279,7 +1279,6 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs)

/* We allow "recursive" symbolic refs. Only within reason, though */
#define MAXDEPTH 5
-#define MAXREFLEN (1024)

/*
* Called by resolve_gitlink_ref_recursive() after it failed to read
@@ -1308,7 +1307,7 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
char buffer[128], *p;
char *path;

- if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)
+ if (recursion > MAXDEPTH)
return -1;
path = *refs->name
? git_path_submodule(refs->name, "%s", refname)

[Michael Haggerty]

=46rom: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail.com>

This function will soon have a second caller.

Signed-off-by: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail=
=2Ecom>
Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---

I leave parse_ref() a public function for the reason explained by
Duy [1].

[1] http://article.gmane.org/gmane.comp.version-control.git/254274

cache.h | 11 ++++
refs.c | 200 ++++++++++++++++++++++++++++++++++----------------------=
--------
2 files changed, 118 insertions(+), 93 deletions(-)

diff --git a/cache.h b/cache.h
index e36084d..d2b8399 100644
--- a/cache.h
+++ b/cache.h
@@ -1005,6 +1005,17 @@ extern int read_ref(const char *refname, unsigne=
d char *sha1);
extern const char *resolve_ref_unsafe(const char *ref, unsigned char *=
sha1, int reading, int *flag);
extern char *resolve_refdup(const char *ref, unsigned char *sha1, int =
reading, int *flag);
extern int resolve_ref(const char *refname, struct strbuf *result, uns=
igned char *sha1, int reading, int *flag);
+/*
+ * Given a ref in "ref" and its path, returns
+ *
+ * -2 failed to open with ENOENT, the caller is responsible for
+ * checking missing loose ref (see resolve_ref for example)
+ * -1 if there's an error, "refname" can no longer be trusted, "flag"=
may
+ * be set. errno is valid.
+ * 0 this is a symref, "refname" now contains the linked ref
+ * +1 normal ref, "sha1" is valid
+ */
+extern int parse_ref(const char *path, struct strbuf *refname, unsigne=
d char *sha1, int *flag);
=20
extern int dwim_ref(const char *str, int len, unsigned char *sha1, cha=
r **ref);
extern int dwim_log(const char *str, int len, unsigned char *sha1, cha=
r **ref);
diff --git a/refs.c b/refs.c
index 29ea7e0..3acd83e 100644
--- a/refs.c
+++ b/refs.c
@@ -1397,6 +1397,104 @@ static int handle_missing_loose_ref(const char =
*refname,
}
}
=20
+int parse_ref(const char *path, struct strbuf *refname,
+ unsigned char *sha1, int *flag)
+{
+ struct strbuf buffer =3D STRBUF_INIT;
+ struct stat st;
+ const char *buf;
+ int ret;
+
+ /*
+ * We might have to loop back here to avoid a race condition:
+ * first we lstat() the file, then we try to read it as a link
+ * or as a file. But if somebody changes the type of the file
+ * (file <-> directory <-> symlink) between the lstat() and
+ * reading, then we don't want to report that as an error but
+ * rather try again starting with the lstat().
+ */
+stat_ref:
+ if (lstat(path, &st) < 0)
+ return errno =3D=3D ENOENT ? -2 : -1;
+
+ /* Follow "normalized" - ie "refs/.." symlinks by hand */
+ if (S_ISLNK(st.st_mode)) {
+ struct strbuf new_path =3D STRBUF_INIT;
+ if (strbuf_readlink(&new_path, path, 256) < 0) {
+ strbuf_release(&new_path);
+ if (errno =3D=3D ENOENT || errno =3D=3D EINVAL)
+ /* inconsistent with lstat; retry */
+ goto stat_ref;
+ else
+ return -1;
+ }
+ if (starts_with(new_path.buf, "refs/") &&
+ !check_refname_format(new_path.buf, 0)) {
+ strbuf_reset(refname);
+ strbuf_addbuf(refname, &new_path);
+ if (flag)
+ *flag |=3D REF_ISSYMREF;
+ strbuf_release(&new_path);
+ return 0;
+ }
+ strbuf_release(&new_path);
+ }
+
+ /* Is it a directory? */
+ if (S_ISDIR(st.st_mode)) {
+ errno =3D EISDIR;
+ return -1;
+ }
+
+ /*
+ * Anything else, just open it and try to use it as
+ * a ref
+ */
+ if (strbuf_read_file(&buffer, path, 256) < 0) {
+ strbuf_release(&buffer);
+ if (errno =3D=3D ENOENT)
+ /* inconsistent with lstat; retry */
+ goto stat_ref;
+ else
+ return -1;
+ }
+ strbuf_rtrim(&buffer);
+
+ if (skip_prefix(buffer.buf, "ref:", &buf)) {
+ /* It is a symbolic ref */
+ if (flag)
+ *flag |=3D REF_ISSYMREF;
+ while (isspace(*buf))
+ buf++;
+ if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
+ if (flag)
+ *flag |=3D REF_ISBROKEN;
+ strbuf_release(&buffer);
+ errno =3D EINVAL;
+ return -1;
+ }
+ strbuf_reset(refname);
+ strbuf_add(refname, buf, buffer.buf + buffer.len - buf);
+ strbuf_release(&buffer);
+ return 0;
+ }
+
+ /*
+ * Please note that FETCH_HEAD has a second line
+ * containing other data.
+ */
+ if (get_sha1_hex(buffer.buf, sha1) ||
+ (buffer.buf[40] !=3D '\0' && !isspace(buffer.buf[40]))) {
+ if (flag)
+ *flag |=3D REF_ISBROKEN;
+ errno =3D EINVAL;
+ ret =3D -1;
+ } else
+ ret =3D 1;
+ strbuf_release(&buffer);
+ return ret;
+}
+
/*
* 'result' content will be destroyed. Its value may be undefined if
* resolve_ref returns -1.
@@ -1406,9 +1504,8 @@ static int handle_missing_loose_ref(const char *r=
efname,
int resolve_ref(const char *refname, struct strbuf *result,
unsigned char *sha1, int reading, int *flag)
{
- struct strbuf buffer =3D STRBUF_INIT;
int depth =3D MAXDEPTH;
- int ret =3D -1;
+ int ret =3D 0;
=20
if (flag)
*flag =3D 0;
@@ -1421,107 +1518,24 @@ int resolve_ref(const char *refname, struct st=
rbuf *result,
strbuf_reset(result);
strbuf_addstr(result, refname);
=20
- for (;;) {
+ while (!ret) {
char path[PATH_MAX];
- struct stat st;
- const char *buf;
=20
if (--depth < 0) {
errno =3D ELOOP;
+ ret =3D -1;
break;
}
=20
git_snpath(path, sizeof(path), "%s", result->buf);
-
- /*
- * We might have to loop back here to avoid a race
- * condition: first we lstat() the file, then we try
- * to read it as a link or as a file. But if somebody
- * changes the type of the file (file <-> directory
- * <-> symlink) between the lstat() and reading, then
- * we don't want to report that as an error but rather
- * try again starting with the lstat().
- */
- stat_ref:
- if (lstat(path, &st) < 0) {
- if (errno =3D=3D ENOENT)
- ret =3D handle_missing_loose_ref(result->buf, sha1,
- reading, flag);
- break;
+ ret =3D parse_ref(path, result, sha1, flag);
+ if (ret =3D=3D -2) {
+ ret =3D handle_missing_loose_ref(result->buf, sha1,
+ reading, flag);
+ ret =3D ret ? -1 : 1;
}
-
- /* Follow "normalized" - ie "refs/.." symlinks by hand */
- if (S_ISLNK(st.st_mode)) {
- /* no need to reset buffer, strbuf_readlink does that */
- if (strbuf_readlink(&buffer, path, 256) < 0) {
- if (errno =3D=3D ENOENT || errno =3D=3D EINVAL)
- /* inconsistent with lstat; retry */
- goto stat_ref;
- else
- break;
- }
- if (starts_with(buffer.buf, "refs/") &&
- !check_refname_format(buffer.buf, 0)) {
- strbuf_reset(result);
- strbuf_addbuf(result, &buffer);
- if (flag)
- *flag |=3D REF_ISSYMREF;
- continue;
- }
- }
-
- /* Is it a directory? */
- if (S_ISDIR(st.st_mode)) {
- errno =3D EISDIR;
- break;
- }
-
- /*
- * Anything else, just open it and try to use it as
- * a ref
- */
- strbuf_reset(&buffer);
- if (strbuf_read_file(&buffer, path, 256) < 0) {
- if (errno =3D=3D ENOENT)
- /* inconsistent with lstat; retry */
- goto stat_ref;
- else
- break;
- }
- strbuf_rtrim(&buffer);
-
- if (skip_prefix(buffer.buf, "ref:", &buf)) {
- /* It is a symbolic ref */
- if (flag)
- *flag |=3D REF_ISSYMREF;
- while (isspace(*buf))
- buf++;
- if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
- if (flag)
- *flag |=3D REF_ISBROKEN;
- errno =3D EINVAL;
- break;
- }
- strbuf_reset(result);
- strbuf_add(result, buf, buffer.buf + buffer.len - buf);
- continue;
- }
-
- /*
- * It must be a normal ref. Please note that
- * FETCH_HEAD has a second line containing other data.
- */
- if (get_sha1_hex(buffer.buf, sha1) ||
- (buffer.buf[40] !=3D '\0' && !isspace(buffer.buf[40]))) {
- if (flag)
- *flag |=3D REF_ISBROKEN;
- errno =3D EINVAL;
- } else
- ret =3D 0;
- break;
}
- strbuf_release(&buffer);
- return ret;
+ return ret > 0 ? 0 : -1;
}
=20
const char *resolve_ref_unsafe(const char *refname, unsigned char *sha=
1, int reading, int *flag)
--=20
2.1.1

[Michael Haggerty]

Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
refs.c | 31 +++++++++++--------------------
1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/refs.c b/refs.c
index a7c8abd..8edcc3b 100644
--- a/refs.c
+++ b/refs.c
@@ -1280,25 +1280,6 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs)
/* We allow "recursive" symbolic refs. Only within reason, though */
#define MAXDEPTH 5

-/*
- * Called by resolve_gitlink_ref_recursive() after it failed to read
- * from the loose refs in ref_cache refs. Find <refname> in the
- * packed-refs file for the submodule.
- */
-static int resolve_gitlink_packed_ref(struct ref_cache *refs,
- const char *refname, unsigned char *sha1)
-{
- struct ref_entry *ref;
- struct ref_dir *dir = get_packed_refs(refs);
-
- ref = find_ref(dir, refname);
- if (ref == NULL)
- return -1;
-
- hashcpy(sha1, ref->u.value.sha1);
- return 0;
-}
-
int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1)
{
struct strbuf result = STRBUF_INIT;
@@ -1331,7 +1312,17 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh
if (parseval == 1) {
ret = 0;
} else if (parseval == -2) {
- ret = resolve_gitlink_packed_ref(refs, result.buf, sha1) ? -1 : 0;
+ /* Loose ref doesn't exist; check for a packed ref */
+ struct ref_entry *entry;
+ struct ref_dir *dir = get_packed_refs(refs);
+
+ entry = find_ref(dir, result.buf);
+ if (entry) {
+ hashcpy(sha1, entry->u.value.sha1);
+ ret = 0;
+ } else {
+ ret = -1;
+ }
} else {
ret = -1;
}

[Michael Haggerty]

It only had one remaining caller.

Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
refs.c | 70 ++++++++++++++++++++++++++++--------------------------------------
1 file changed, 30 insertions(+), 40 deletions(-)

diff --git a/refs.c b/refs.c
index 3acd83e..9f2a0f8 100644
--- a/refs.c
+++ b/refs.c
@@ -1366,37 +1366,6 @@ static struct ref_entry *get_packed_ref(const char *refname)
return find_ref(get_packed_refs(&ref_cache), refname);
}

-/*
- * A loose ref file doesn't exist; check for a packed ref. The
- * options are forwarded from resolve_safe_unsafe().
- */
-static int handle_missing_loose_ref(const char *refname,
- unsigned char *sha1,
- int reading,
- int *flag)
-{
- struct ref_entry *entry;
-
- /*
- * The loose reference file does not exist; check for a packed
- * reference.
- */
- entry = get_packed_ref(refname);
- if (entry) {
- hashcpy(sha1, entry->u.value.sha1);
- if (flag)
- *flag |= REF_ISPACKED;
- return 0;
- }
- /* The reference is not a packed reference, either. */
- if (reading) {
- return -1;
- } else {
- hashclr(sha1);
- return 0;
- }
-}
-
int parse_ref(const char *path, struct strbuf *refname,
unsigned char *sha1, int *flag)
{
@@ -1505,7 +1474,7 @@ int resolve_ref(const char *refname, struct strbuf *result,
unsigned char *sha1, int reading, int *flag)
{
int depth = MAXDEPTH;
- int ret = 0;
+ int ret;

if (flag)
*flag = 0;
@@ -1518,24 +1487,45 @@ int resolve_ref(const char *refname, struct strbuf *result,
strbuf_reset(result);
strbuf_addstr(result, refname);

- while (!ret) {
+ do {
char path[PATH_MAX];

if (--depth < 0) {
errno = ELOOP;
- ret = -1;
- break;
+ return -1;
}

git_snpath(path, sizeof(path), "%s", result->buf);
ret = parse_ref(path, result, sha1, flag);
- if (ret == -2) {
- ret = handle_missing_loose_ref(result->buf, sha1,
- reading, flag);
- ret = ret ? -1 : 1;
+ } while (!ret);
+
+ if (ret == 1) {
+ return 0;
+ } else if (ret == -2) {
+ /*
+ * The loose reference file does not exist; check for a packed
+ * reference.
+ */
+ struct ref_entry *entry;
+
+ entry = get_packed_ref(result->buf);
+ if (entry) {
+ hashcpy(sha1, entry->u.value.sha1);
+ if (flag)
+ *flag |= REF_ISPACKED;
+ return 0;
+ }
+
+ /* The reference is not a packed reference, either. */
+ if (reading) {
+ return -1;
+ } else {
+ hashclr(sha1);
+ return 0;
}
+ } else {
+ return -1;
}
- return ret > 0 ? 0 : -1;
}

const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)

[Michael Haggerty]

=46rom: Nguy=E1=BB=85n Th=C3=A1i Ng=E1=BB=8Dc Duy <***@gmail.com>

This requires buf to be declared (const char *), which is how it is
used anyway.

Signed-off-by: Michael Haggerty <***@alum.mit.edu>
---
refs.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 771941b..020ee3f 100644
--- a/refs.c
+++ b/refs.c
@@ -1416,7 +1416,7 @@ const char *resolve_ref_unsafe(const char *refnam=
e, unsigned char *sha1, int rea
for (;;) {
char path[PATH_MAX];
struct stat st;
- char *buf;
+ const char *buf;
int fd;
=20
if (--depth < 0) {
@@ -1497,11 +1497,10 @@ const char *resolve_ref_unsafe(const char *refn=
ame, unsigned char *sha1, int rea
len--;
buffer[len] =3D '\0';
=20
- if (starts_with(buffer, "ref:")) {
+ if (skip_prefix(buffer, "ref:", &buf)) {
/* It is a symbolic ref */
if (flag)
*flag |=3D REF_ISSYMREF;
- buf =3D buffer + 4;
while (isspace(*buf))
buf++;
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
--=20
2.1.1

[Junio C Hamano]

This is a rif on Duy's oldish patch series [1]. I started reviewing
his patch series, but found that some of his patches did multiple
things, making it harder to review. I started pulling it apart into
smaller changes to aid my review, and I guess I got carried away :-/

Hmmm...

You are aware of another large change in flight in the same area,
and after having reviewed that series a few times I was hoping that
you would have a better understanding of how ready the other topic
is. And then I see this series that conflicts with that other
topic. Is this your way to say that the other topic is not quite
ready?

[Junio C Hamano]

Post by Junio C Hamano

This is a rif on Duy's oldish patch series [1]. I started reviewing
his patch series, but found that some of his patches did multiple
things, making it harder to review. I started pulling it apart into
smaller changes to aid my review, and I guess I got carried away :-/

Hmmm...
You are aware of another large change in flight in the same area,
and after having reviewed that series a few times I was hoping that
you would have a better understanding of how ready the other topic
is. And then I see this series that conflicts with that other
topic. Is this your way to say that the other topic is not quite
ready?

The last one was a rhetorical question and I regret that it came out
a bit harsher than I intended to. I just wanted to see a bit better
inter-developer coordination, that's all.

Thanks.

[Michael Haggerty]

Post by Junio C Hamano

This is a rif on Duy's oldish patch series [1]. I started reviewing
his patch series, but found that some of his patches did multiple
things, making it harder to review. I started pulling it apart into
smaller changes to aid my review, and I guess I got carried away :-/

Hmmm...
You are aware of another large change in flight in the same area,
and after having reviewed that series a few times I was hoping that
you would have a better understanding of how ready the other topic
is. And then I see this series that conflicts with that other
topic. Is this your way to say that the other topic is not quite
ready?

No, not at all. To be honest, I thought that the changes in this patch
series were localized in an area different than Ronnie and Jonathan's
patches were touching, but stupidly I didn't check for conflicts. Sorry
about that.

There is nothing urgent in this patch series, so I suggest we just put
it back on ice and I will reroll after the dust has settled on
rs/ref-transactions. The conflicts shouldn't be super hard to resolve
(the series don't overlap much *conceptually*), but I'd rather not have
to do it multiple times.

Regarding rs/ref-transaction, I will reply on that thread.

Michael

[Duy Nguyen]

As far as I know, Duy isn't actively working on this, so I hope my
reroll is not unwelcome.

I couldn't be happier that someone does the work for me and I still
benefit from it ;)