Merge branch 'jk/reading-packed-refs'

An earlier rewrite to use strbuf_getwholeline() instead of fgets(3)
to read packed-refs file revealed that the former is unacceptably
inefficient.

* jk/reading-packed-refs:
t1430: add another refs-escape test
read_packed_refs: avoid double-checking sane refs
strbuf_getwholeline: use getdelim if it is available
strbuf_getwholeline: avoid calling strbuf_grow
strbuf_addch: avoid calling strbuf_grow
config: use getc_unlocked when reading from file
strbuf_getwholeline: use getc_unlocked
git-compat-util: add fallbacks for unlocked stdio
strbuf_getwholeline: use getc macro

diff --git a/Makefile b/Makefile
index 5f3987fe3bd945fb5a84c9f45a8de7da5581f79a..36655d5a16f9cd7ed61c8cdff24cfd66c13d242c 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -359,6 +359,8 @@ all::
 # compiler is detected to support it.
 #
 # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
+#
+# Define HAVE_GETDELIM if your system has the getdelim() function.
 
 GIT-VERSION-FILE: FORCE
        @$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1437,6 +1439,10 @@ ifdef HAVE_BSD_SYSCTL
        BASIC_CFLAGS += -DHAVE_BSD_SYSCTL
 endif
 
+ifdef HAVE_GETDELIM
+       BASIC_CFLAGS += -DHAVE_GETDELIM
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif

diff --git a/config.c b/config.c
index c4424c01388496b5995e19f9601f0c87b9fdd3c0..6d0098fc80df01ce9df4701119557472430b0619 100644 (file)
--- a/config.c
+++ b/config.c
@@ -50,7 +50,7 @@ static struct config_set the_config_set;
 
 static int config_file_fgetc(struct config_source *conf)
 {
-       return fgetc(conf->u.file);
+       return getc_unlocked(conf->u.file);
 }
 
 static int config_file_ungetc(int c, struct config_source *conf)
@@ -1088,7 +1088,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 
        f = fopen(filename, "r");
        if (f) {
+               flockfile(f);
                ret = do_config_from_file(fn, filename, filename, f, data);
+               funlockfile(f);
                fclose(f);
        }
        return ret;

diff --git a/config.mak.uname b/config.mak.uname
index f4e77cb9e5099cd3de723ad98894c92d516f176e..d26665fa54c90a45724c0eb6bdbcbb81c94574a1 100644 (file)
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux)
        HAVE_DEV_TTY = YesPlease
        HAVE_CLOCK_GETTIME = YesPlease
        HAVE_CLOCK_MONOTONIC = YesPlease
+       HAVE_GETDELIM = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
        HAVE_ALLOCA_H = YesPlease

diff --git a/git-compat-util.h b/git-compat-util.h
index bc8fc8cf854e96badfdf4d96673d33b799207ff3..685a0a4063edd2a6700533df7faa2a910df60bb3 100644 (file)
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -883,4 +883,10 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 # define SHELL_PATH "/bin/sh"
 #endif
 
+#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
+#define flockfile(fh)
+#define funlockfile(fh)
+#define getc_unlocked(fh) getc(fh)
+#endif
+
 #endif

diff --git a/refs.c b/refs.c
index 0312f052575046f0dd539b19897b61eddc2862e0..81a455b807e4ef7f79e3cdb252eedfccfa3c7d8a 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -344,8 +344,6 @@ static struct ref_entry *create_ref_entry(const char *refname,
        if (check_name &&
            check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
                die("Reference has invalid format: '%s'", refname);
-       if (!check_name && !refname_is_safe(refname))
-               die("Reference has invalid name: '%s'", refname);
        len = strlen(refname) + 1;
        ref = xmalloc(sizeof(struct ref_entry) + len);
        hashcpy(ref->u.value.sha1, sha1);
@@ -1178,6 +1176,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
                        int flag = REF_ISPACKED;
 
                        if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+                               if (!refname_is_safe(refname))
+                                       die("packed refname is dangerous: %s", refname);
                                hashclr(sha1);
                                flag |= REF_BAD_NAME | REF_ISBROKEN;
                        }
@@ -1323,6 +1323,8 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
                        }
                        if (check_refname_format(refname.buf,
                                                 REFNAME_ALLOW_ONELEVEL)) {
+                               if (!refname_is_safe(refname.buf))
+                                       die("loose refname is dangerous: %s", refname.buf);
                                hashclr(sha1);
                                flag |= REF_BAD_NAME | REF_ISBROKEN;
                        }

diff --git a/strbuf.c b/strbuf.c
index 88cafd4a70b8179a4e911c18704fb4ab0f2a21f5..0d4f4e54ec1ff1f0d37155e8049476d802a7c7e5 100644 (file)
--- a/strbuf.c
+++ b/strbuf.c
@@ -435,6 +435,47 @@ int strbuf_getcwd(struct strbuf *sb)
        return -1;
 }
 
+#ifdef HAVE_GETDELIM
+int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
+{
+       ssize_t r;
+
+       if (feof(fp))
+               return EOF;
+
+       strbuf_reset(sb);
+
+       /* Translate slopbuf to NULL, as we cannot call realloc on it */
+       if (!sb->alloc)
+               sb->buf = NULL;
+       r = getdelim(&sb->buf, &sb->alloc, term, fp);
+
+       if (r > 0) {
+               sb->len = r;
+               return 0;
+       }
+       assert(r == -1);
+
+       /*
+        * Normally we would have called xrealloc, which will try to free
+        * memory and recover. But we have no way to tell getdelim() to do so.
+        * Worse, we cannot try to recover ENOMEM ourselves, because we have
+        * no idea how many bytes were read by getdelim.
+        *
+        * Dying here is reasonable. It mirrors what xrealloc would do on
+        * catastrophic memory failure. We skip the opportunity to free pack
+        * memory and retry, but that's unlikely to help for a malloc small
+        * enough to hold a single line of input, anyway.
+        */
+       if (errno == ENOMEM)
+               die("Out of memory, getdelim failed");
+
+       /* Restore slopbuf that we moved out of the way before */
+       if (!sb->buf)
+               strbuf_init(sb, 0);
+       return EOF;
+}
+#else
 int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 {
        int ch;
@@ -443,18 +484,22 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
                return EOF;
 
        strbuf_reset(sb);
-       while ((ch = fgetc(fp)) != EOF) {
-               strbuf_grow(sb, 1);
+       flockfile(fp);
+       while ((ch = getc_unlocked(fp)) != EOF) {
+               if (!strbuf_avail(sb))
+                       strbuf_grow(sb, 1);
                sb->buf[sb->len++] = ch;
                if (ch == term)
                        break;
        }
+       funlockfile(fp);
        if (ch == EOF && sb->len == 0)
                return EOF;
 
        sb->buf[sb->len] = '\0';
        return 0;
 }
+#endif
 
 int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
 {

diff --git a/strbuf.h b/strbuf.h
index 1883494ca3ad4931640c2a295c94800e287c1664..01c5c6371b8e43686f335704a67a59a1d82dbe3b 100644 (file)
--- a/strbuf.h
+++ b/strbuf.h
@@ -205,7 +205,8 @@ extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
  */
 static inline void strbuf_addch(struct strbuf *sb, int c)
 {
-       strbuf_grow(sb, 1);
+       if (!strbuf_avail(sb))
+               strbuf_grow(sb, 1);
        sb->buf[sb->len++] = c;
        sb->buf[sb->len] = '\0';
 }

diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 468e85621aadaa4a14678ea4a9bf2b673e4ad810..16d0b8bd1a5b6b6a2701fea1c4f35858cec9528b 100755 (executable)
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -68,6 +68,14 @@ test_expect_success 'branch -D cannot delete non-ref in .git dir' '
        test_cmp expect .git/my-private-file
 '
 
+test_expect_success 'branch -D cannot delete ref in .git dir' '
+       git rev-parse HEAD >.git/my-private-file &&
+       git rev-parse HEAD >expect &&
+       git branch foo/legit &&
+       test_must_fail git branch -D foo////./././../../../my-private-file &&
+       test_cmp expect .git/my-private-file
+'
+
 test_expect_success 'branch -D cannot delete absolute path' '
        git branch -f extra &&
        test_must_fail git branch -D "$(pwd)/.git/refs/heads/extra" &&