* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Junio C Hamano @ 2012-02-06 5:30 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty
In-Reply-To: <20120206051834.GA5062@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Sure, that's one way to do it. But I don't see any point in not allowing
> "git checkout -b" to be another way of doing it. Is there some other use
> case for "git checkout -b" from an unborn branch? Or is there some
> harmful outcome that can come from doing so that we need to be
> protecting against? Am I missing something?
Mostly because it is wrong at the conceptual level to do so.
git checkout -b foo
is a short-hand for
git checkout -b foo HEAD
which is a short-hand for
git branch foo HEAD &&
git checkout foo
But the last one has no chance of working if you think about it, because
"git branch foo $start" is a way to start a branch at $start and you need
to have something to point at with refs/heads/foo.
So we are breaking the equivalence between these three only when HEAD
points at an unborn branch.
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Junio C Hamano @ 2012-02-06 5:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Michael Haggerty
In-Reply-To: <7vk440a5qw.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> But the last one has no chance of working if you think about it, because
> "git branch foo $start" is a way to start a branch at $start and you need
> to have something to point at with refs/heads/foo.
... which brings us back to your earlier point ...
>> I like your patch better than trying to pass around "0{40}", but:
which is why my conclusion was that "checkout -b" is shifting the
confusion around to different parts.
> So we are breaking the equivalence between these three only when HEAD
> points at an unborn branch.
^ permalink raw reply
* [PATCH 1/6] read-cache: use sha1file for sha1 calculation
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 5:48 UTC (permalink / raw)
To: git; +Cc: Thomas Rast, Joshua Redstone,
Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
read-cache.c | 90 +++++++++++++++-------------------------------------------
1 files changed, 23 insertions(+), 67 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index a51bba1..e9a20b6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -12,6 +12,7 @@
#include "commit.h"
#include "blob.h"
#include "resolve-undo.h"
+#include "csum-file.h"
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really);
@@ -1395,73 +1396,28 @@ int unmerged_index(const struct index_state *istate)
return 0;
}
-#define WRITE_BUFFER_SIZE 8192
-static unsigned char write_buffer[WRITE_BUFFER_SIZE];
-static unsigned long write_buffer_len;
-
-static int ce_write_flush(git_SHA_CTX *context, int fd)
-{
- unsigned int buffered = write_buffer_len;
- if (buffered) {
- git_SHA1_Update(context, write_buffer, buffered);
- if (write_in_full(fd, write_buffer, buffered) != buffered)
- return -1;
- write_buffer_len = 0;
- }
- return 0;
-}
-
-static int ce_write(git_SHA_CTX *context, int fd, void *data, unsigned int len)
+static int ce_write(struct sha1file *f, void *data, unsigned int len)
{
- while (len) {
- unsigned int buffered = write_buffer_len;
- unsigned int partial = WRITE_BUFFER_SIZE - buffered;
- if (partial > len)
- partial = len;
- memcpy(write_buffer + buffered, data, partial);
- buffered += partial;
- if (buffered == WRITE_BUFFER_SIZE) {
- write_buffer_len = buffered;
- if (ce_write_flush(context, fd))
- return -1;
- buffered = 0;
- }
- write_buffer_len = buffered;
- len -= partial;
- data = (char *) data + partial;
- }
- return 0;
+ return sha1write(f, data, len);
}
-static int write_index_ext_header(git_SHA_CTX *context, int fd,
+static int write_index_ext_header(struct sha1file *f,
unsigned int ext, unsigned int sz)
{
ext = htonl(ext);
sz = htonl(sz);
- return ((ce_write(context, fd, &ext, 4) < 0) ||
- (ce_write(context, fd, &sz, 4) < 0)) ? -1 : 0;
+ return ((ce_write(f, &ext, 4) < 0) ||
+ (ce_write(f, &sz, 4) < 0)) ? -1 : 0;
}
-static int ce_flush(git_SHA_CTX *context, int fd)
+static int ce_flush(struct sha1file *f)
{
- unsigned int left = write_buffer_len;
-
- if (left) {
- write_buffer_len = 0;
- git_SHA1_Update(context, write_buffer, left);
- }
-
- /* Flush first if not enough space for SHA1 signature */
- if (left + 20 > WRITE_BUFFER_SIZE) {
- if (write_in_full(fd, write_buffer, left) != left)
- return -1;
- left = 0;
- }
+ unsigned char sha1[20];
+ int fd = sha1close(f, sha1, 0);
- /* Append the SHA1 signature at the end */
- git_SHA1_Final(write_buffer + left, context);
- left += 20;
- return (write_in_full(fd, write_buffer, left) != left) ? -1 : 0;
+ if (fd < 0)
+ return -1;
+ return (write_in_full(fd, sha1, 20) != 20) ? -1 : 0;
}
static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
@@ -1513,7 +1469,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
}
}
-static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce)
+static int ce_write_entry(struct sha1file *f, struct cache_entry *ce)
{
int size = ondisk_ce_size(ce);
struct ondisk_cache_entry *ondisk = xcalloc(1, size);
@@ -1542,7 +1498,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce)
name = ondisk->name;
memcpy(name, ce->name, ce_namelen(ce));
- result = ce_write(c, fd, ondisk, size);
+ result = ce_write(f, ondisk, size);
free(ondisk);
return result;
}
@@ -1574,7 +1530,7 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile
int write_index(struct index_state *istate, int newfd)
{
- git_SHA_CTX c;
+ struct sha1file *f;
struct cache_header hdr;
int i, err, removed, extended;
struct cache_entry **cache = istate->cache;
@@ -1598,8 +1554,8 @@ int write_index(struct index_state *istate, int newfd)
hdr.hdr_version = htonl(extended ? 3 : 2);
hdr.hdr_entries = htonl(entries - removed);
- git_SHA1_Init(&c);
- if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
+ f = sha1fd(newfd, NULL);
+ if (ce_write(f, &hdr, sizeof(hdr)) < 0)
return -1;
for (i = 0; i < entries; i++) {
@@ -1608,7 +1564,7 @@ int write_index(struct index_state *istate, int newfd)
continue;
if (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))
ce_smudge_racily_clean_entry(ce);
- if (ce_write_entry(&c, newfd, ce) < 0)
+ if (ce_write_entry(f, ce) < 0)
return -1;
}
@@ -1617,8 +1573,8 @@ int write_index(struct index_state *istate, int newfd)
struct strbuf sb = STRBUF_INIT;
cache_tree_write(&sb, istate->cache_tree);
- err = write_index_ext_header(&c, newfd, CACHE_EXT_TREE, sb.len) < 0
- || ce_write(&c, newfd, sb.buf, sb.len) < 0;
+ err = write_index_ext_header(f, CACHE_EXT_TREE, sb.len) < 0
+ || ce_write(f, sb.buf, sb.len) < 0;
strbuf_release(&sb);
if (err)
return -1;
@@ -1627,15 +1583,15 @@ int write_index(struct index_state *istate, int newfd)
struct strbuf sb = STRBUF_INIT;
resolve_undo_write(&sb, istate->resolve_undo);
- err = write_index_ext_header(&c, newfd, CACHE_EXT_RESOLVE_UNDO,
+ err = write_index_ext_header(f, CACHE_EXT_RESOLVE_UNDO,
sb.len) < 0
- || ce_write(&c, newfd, sb.buf, sb.len) < 0;
+ || ce_write(f, sb.buf, sb.len) < 0;
strbuf_release(&sb);
if (err)
return -1;
}
- if (ce_flush(&c, newfd) || fstat(newfd, &st))
+ if (ce_flush(f) || fstat(newfd, &st))
return -1;
istate->timestamp.sec = (unsigned int)st.st_mtime;
istate->timestamp.nsec = ST_MTIME_NSEC(st);
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 2/6] csum-file: make sha1 calculation optional
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 5:48 UTC (permalink / raw)
To: git; +Cc: Thomas Rast, Joshua Redstone,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328507319-24687-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
csum-file.c | 16 +++++++++++++---
csum-file.h | 2 +-
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/csum-file.c b/csum-file.c
index 53f5375..4c517d1 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -11,6 +11,12 @@
#include "progress.h"
#include "csum-file.h"
+static void sha1update(struct sha1file *f, const void *data, unsigned offset)
+{
+ if (f->do_sha1)
+ git_SHA1_Update(&f->ctx, data, offset);
+}
+
static void flush(struct sha1file *f, void *buf, unsigned int count)
{
if (0 <= f->check_fd && count) {
@@ -47,7 +53,7 @@ void sha1flush(struct sha1file *f)
unsigned offset = f->offset;
if (offset) {
- git_SHA1_Update(&f->ctx, f->buffer, offset);
+ sha1update(f, f->buffer, offset);
flush(f, f->buffer, offset);
f->offset = 0;
}
@@ -58,7 +64,10 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)
int fd;
sha1flush(f);
- git_SHA1_Final(f->buffer, &f->ctx);
+ if (f->do_sha1)
+ git_SHA1_Final(f->buffer, &f->ctx);
+ else
+ hashclr(f->buffer);
if (result)
hashcpy(result, f->buffer);
if (flags & (CSUM_CLOSE | CSUM_FSYNC)) {
@@ -110,7 +119,7 @@ int sha1write(struct sha1file *f, void *buf, unsigned int count)
buf = (char *) buf + nr;
left -= nr;
if (!left) {
- git_SHA1_Update(&f->ctx, data, offset);
+ sha1update(f, data, offset);
flush(f, data, offset);
offset = 0;
}
@@ -154,6 +163,7 @@ struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp
f->tp = tp;
f->name = name;
f->do_crc = 0;
+ f->do_sha1 = 1;
git_SHA1_Init(&f->ctx);
return f;
}
diff --git a/csum-file.h b/csum-file.h
index 3b540bd..c23ea62 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -12,7 +12,7 @@ struct sha1file {
off_t total;
struct progress *tp;
const char *name;
- int do_crc;
+ int do_crc, do_sha1;
uint32_t crc32;
unsigned char buffer[8192];
};
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 3/6] Stop producing index version 2
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 5:48 UTC (permalink / raw)
To: git; +Cc: Thomas Rast, Joshua Redstone,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328507319-24687-1-git-send-email-pclouds@gmail.com>
read-cache.c learned to produce version 2 or 3 depending on whether
extended cache entries exist in 06aaaa0 (Extend index to save more flags
- 2008-10-01), first released in 1.6.1. The purpose is to keep
compatibility with older git. It's been more than three years since
then and git has reached 1.7.9. Drop support for older git.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
read-cache.c | 8 +++-----
t/t2104-update-index-skip-worktree.sh | 12 ------------
2 files changed, 3 insertions(+), 17 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index e9a20b6..fe6b0e0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1532,26 +1532,24 @@ int write_index(struct index_state *istate, int newfd)
{
struct sha1file *f;
struct cache_header hdr;
- int i, err, removed, extended;
+ int i, err, removed;
struct cache_entry **cache = istate->cache;
int entries = istate->cache_nr;
struct stat st;
- for (i = removed = extended = 0; i < entries; i++) {
+ for (i = removed = 0; i < entries; i++) {
if (cache[i]->ce_flags & CE_REMOVE)
removed++;
/* reduce extended entries if possible */
cache[i]->ce_flags &= ~CE_EXTENDED;
if (cache[i]->ce_flags & CE_EXTENDED_FLAGS) {
- extended++;
cache[i]->ce_flags |= CE_EXTENDED;
}
}
hdr.hdr_signature = htonl(CACHE_SIGNATURE);
- /* for extended format, increase version so older git won't try to read it */
- hdr.hdr_version = htonl(extended ? 3 : 2);
+ hdr.hdr_version = htonl(3);
hdr.hdr_entries = htonl(entries - removed);
f = sha1fd(newfd, NULL);
diff --git a/t/t2104-update-index-skip-worktree.sh b/t/t2104-update-index-skip-worktree.sh
index 1d0879b..8221ffa 100755
--- a/t/t2104-update-index-skip-worktree.sh
+++ b/t/t2104-update-index-skip-worktree.sh
@@ -28,19 +28,11 @@ test_expect_success 'setup' '
git ls-files -t | test_cmp expect.full -
'
-test_expect_success 'index is at version 2' '
- test "$(test-index-version < .git/index)" = 2
-'
-
test_expect_success 'update-index --skip-worktree' '
git update-index --skip-worktree 1 sub/1 &&
git ls-files -t | test_cmp expect.skip -
'
-test_expect_success 'index is at version 3 after having some skip-worktree entries' '
- test "$(test-index-version < .git/index)" = 3
-'
-
test_expect_success 'ls-files -t' '
git ls-files -t | test_cmp expect.skip -
'
@@ -50,8 +42,4 @@ test_expect_success 'update-index --no-skip-worktree' '
git ls-files -t | test_cmp expect.full -
'
-test_expect_success 'index version is back to 2 when there is no skip-worktree entry' '
- test "$(test-index-version < .git/index)" = 2
-'
-
test_done
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 4/6] Introduce index version 4 with global flags
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 5:48 UTC (permalink / raw)
To: git; +Cc: Thomas Rast, Joshua Redstone,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328507319-24687-1-git-send-email-pclouds@gmail.com>
v4 adds 32-bit field to cache header after 32-bit number of entries.
If this field is zero, fall back to v3.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/technical/index-format.txt | 4 ++-
cache.h | 6 +++++
read-cache.c | 31 ++++++++++++++++++++++-------
3 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
index 8930b3f..2b6a38e 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -12,10 +12,12 @@ GIT index format
The signature is { 'D', 'I', 'R', 'C' } (stands for "dircache")
4-byte version number:
- The current supported versions are 2 and 3.
+ The current supported versions are 2, 3 and 4.
32-bit number of index entries.
+ 32-bit flags (version 4 only).
+
- A number of sorted index entries (see below).
- Extensions
diff --git a/cache.h b/cache.h
index 9bd8c2d..c2e884a 100644
--- a/cache.h
+++ b/cache.h
@@ -105,6 +105,11 @@ struct cache_header {
unsigned int hdr_entries;
};
+struct ext_cache_header {
+ struct cache_header h;
+ unsigned int hdr_flags;
+};
+
/*
* The "cache_time" is just the low 32 bits of the
* time. It doesn't matter if it overflows - we only
@@ -314,6 +319,7 @@ static inline unsigned int canon_mode(unsigned int mode)
struct index_state {
struct cache_entry **cache;
unsigned int cache_nr, cache_alloc, cache_changed;
+ unsigned int hdr_flags;
struct string_list *resolve_undo;
struct cache_tree *cache_tree;
struct cache_time timestamp;
diff --git a/read-cache.c b/read-cache.c
index fe6b0e0..fd21af6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1190,7 +1190,9 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)
if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
return error("bad signature");
- if (hdr->hdr_version != htonl(2) && hdr->hdr_version != htonl(3))
+ if (hdr->hdr_version != htonl(2) &&
+ hdr->hdr_version != htonl(3) &&
+ hdr->hdr_version != htonl(4))
return error("bad index version");
git_SHA1_Init(&c);
git_SHA1_Update(&c, hdr, size - 20);
@@ -1320,7 +1322,12 @@ int read_index_from(struct index_state *istate, const char *path)
istate->cache = xcalloc(istate->cache_alloc, sizeof(struct cache_entry *));
istate->initialized = 1;
- src_offset = sizeof(*hdr);
+ if (ntohl(hdr->hdr_version) >= 4) {
+ struct ext_cache_header *ehdr = mmap;
+ istate->hdr_flags = ntohl(ehdr->hdr_flags);
+ src_offset = sizeof(*ehdr);
+ } else
+ src_offset = sizeof(*hdr);
for (i = 0; i < istate->cache_nr; i++) {
struct ondisk_cache_entry *disk_ce;
struct cache_entry *ce;
@@ -1375,6 +1382,7 @@ int discard_index(struct index_state *istate)
resolve_undo_clear_index(istate);
istate->cache_nr = 0;
istate->cache_changed = 0;
+ istate->hdr_flags = 0;
istate->timestamp.sec = 0;
istate->timestamp.nsec = 0;
istate->name_hash_initialized = 0;
@@ -1531,8 +1539,8 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile
int write_index(struct index_state *istate, int newfd)
{
struct sha1file *f;
- struct cache_header hdr;
- int i, err, removed;
+ struct ext_cache_header hdr;
+ int i, err, removed, hdr_size;
struct cache_entry **cache = istate->cache;
int entries = istate->cache_nr;
struct stat st;
@@ -1548,12 +1556,19 @@ int write_index(struct index_state *istate, int newfd)
}
}
- hdr.hdr_signature = htonl(CACHE_SIGNATURE);
- hdr.hdr_version = htonl(3);
- hdr.hdr_entries = htonl(entries - removed);
+ hdr.h.hdr_signature = htonl(CACHE_SIGNATURE);
+ if (istate->hdr_flags) {
+ hdr.h.hdr_version = htonl(4);
+ hdr.hdr_flags = htonl(istate->hdr_flags);
+ hdr_size = sizeof(hdr);
+ } else {
+ hdr.h.hdr_version = htonl(3);
+ hdr_size = sizeof(hdr.h);
+ }
+ hdr.h.hdr_entries = htonl(entries - removed);
f = sha1fd(newfd, NULL);
- if (ce_write(f, &hdr, sizeof(hdr)) < 0)
+ if (ce_write(f, &hdr, hdr_size) < 0)
return -1;
for (i = 0; i < entries; i++) {
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 5/6] Allow to use crc32 as a lighter checksum on index
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 5:48 UTC (permalink / raw)
To: git; +Cc: Thomas Rast, Joshua Redstone,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328507319-24687-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/git-update-index.txt | 12 +++++++-
builtin/update-index.c | 11 +++++++
cache.h | 2 +
read-cache.c | 54 ++++++++++++++++++++++++++++--------
4 files changed, 66 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index a3081f4..2574a4e 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[--add] [--remove | --force-remove] [--replace]
[--refresh] [-q] [--unmerged] [--ignore-missing]
[(--cacheinfo <mode> <object> <file>)...]
- [--chmod=(+|-)x]
+ [--chmod=(+|-)x] [--[no-]crc32]
[--assume-unchanged | --no-assume-unchanged]
[--skip-worktree | --no-skip-worktree]
[--ignore-submodules]
@@ -109,6 +109,16 @@ you will need to handle the situation manually.
set and unset the "skip-worktree" bit for the paths. See
section "Skip-worktree bit" below for more information.
+--crc32::
+--no-crc32::
+ Normally SHA-1 is used to check for index integrity. When the
+ index is large, SHA-1 computation cost can be significant.
+ --crc32 will convert current index to use (cheaper) crc32
+ instead. Note that later writes to index by other commands can
+ convert the index back to SHA-1. Older git versions may not
+ understand crc32 index, --no-crc32 can be used to convert it
+ back to SHA-1.
+
-g::
--again::
Runs 'git update-index' itself on the paths whose index
diff --git a/builtin/update-index.c b/builtin/update-index.c
index a6a23fa..6913226 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -707,6 +707,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
{
int newfd, entries, has_errors = 0, line_termination = '\n';
int read_from_stdin = 0;
+ int do_crc = -1;
int prefix_length = prefix ? strlen(prefix) : 0;
char set_executable_bit = 0;
struct refresh_params refresh_args = {0, &has_errors};
@@ -791,6 +792,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
"(for porcelains) forget saved unresolved conflicts",
PARSE_OPT_NOARG | PARSE_OPT_NONEG,
resolve_undo_clear_callback},
+ OPT_BOOL(0, "crc32", &do_crc,
+ "use crc32 as checksum instead of sha1"),
OPT_END()
};
@@ -852,6 +855,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
}
argc = parse_options_end(&ctx);
+ if (do_crc != -1) {
+ if (do_crc)
+ the_index.hdr_flags |= CACHE_F_CRC;
+ else
+ the_index.hdr_flags &= ~CACHE_F_CRC;
+ active_cache_changed = 1;
+ }
+
if (read_from_stdin) {
struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
diff --git a/cache.h b/cache.h
index c2e884a..7352402 100644
--- a/cache.h
+++ b/cache.h
@@ -105,6 +105,8 @@ struct cache_header {
unsigned int hdr_entries;
};
+#define CACHE_F_CRC 1 /* use crc32 instead of sha1 for index checksum */
+
struct ext_cache_header {
struct cache_header h;
unsigned int hdr_flags;
diff --git a/read-cache.c b/read-cache.c
index fd21af6..a34878e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1185,20 +1185,33 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int reall
static int verify_hdr(struct cache_header *hdr, unsigned long size)
{
+ int do_crc;
git_SHA_CTX c;
unsigned char sha1[20];
if (hdr->hdr_signature != htonl(CACHE_SIGNATURE))
return error("bad signature");
- if (hdr->hdr_version != htonl(2) &&
- hdr->hdr_version != htonl(3) &&
- hdr->hdr_version != htonl(4))
+ if (hdr->hdr_version == htonl(2) ||
+ hdr->hdr_version == htonl(3))
+ do_crc = 0;
+ else if (hdr->hdr_version == htonl(4)) {
+ struct ext_cache_header *ehdr = (struct ext_cache_header *)hdr;
+ do_crc = ntohl(ehdr->hdr_flags) & CACHE_F_CRC;
+ }
+ else
return error("bad index version");
- git_SHA1_Init(&c);
- git_SHA1_Update(&c, hdr, size - 20);
- git_SHA1_Final(sha1, &c);
- if (hashcmp(sha1, (unsigned char *)hdr + size - 20))
- return error("bad index file sha1 signature");
+ if (do_crc) {
+ uint32_t crc = crc32(0, NULL, 0);
+ crc = crc32(crc,(void *) hdr, size - sizeof(uint32_t));
+ if (crc != *(uint32_t*)((unsigned char *)hdr + size - sizeof(uint32_t)))
+ return error("bad index file crc32 signature");
+ } else {
+ git_SHA1_Init(&c);
+ git_SHA1_Update(&c, hdr, size - 20);
+ git_SHA1_Final(sha1, &c);
+ if (hashcmp(sha1, (unsigned char *)hdr + size - 20))
+ return error("bad index file sha1 signature");
+ }
return 0;
}
@@ -1421,11 +1434,24 @@ static int write_index_ext_header(struct sha1file *f,
static int ce_flush(struct sha1file *f)
{
unsigned char sha1[20];
- int fd = sha1close(f, sha1, 0);
+ int fd;
- if (fd < 0)
- return -1;
- return (write_in_full(fd, sha1, 20) != 20) ? -1 : 0;
+ if (f->do_crc) {
+ uint32_t crc;
+
+ assert(f->do_sha1 == 0);
+ sha1flush(f);
+ crc = crc32_end(f);
+ fd = sha1close(f, sha1, 0);
+ if (fd < 0)
+ return -1;
+ return (write_in_full(fd, &crc, sizeof(crc)) != sizeof(crc)) ? -1 : 0;
+ } else {
+ fd = sha1close(f, sha1, 0);
+ if (fd < 0)
+ return -1;
+ return (write_in_full(fd, sha1, 20) != 20) ? -1 : 0;
+ }
}
static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
@@ -1568,6 +1594,10 @@ int write_index(struct index_state *istate, int newfd)
hdr.h.hdr_entries = htonl(entries - removed);
f = sha1fd(newfd, NULL);
+ if (istate->hdr_flags & CACHE_F_CRC) {
+ crc32_begin(f);
+ f->do_sha1 = 0;
+ }
if (ce_write(f, &hdr, hdr_size) < 0)
return -1;
--
1.7.8.36.g69ee2
^ permalink raw reply related
* [PATCH 6/6] Automatically switch to crc32 checksum for index when it's too large
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 5:48 UTC (permalink / raw)
To: git; +Cc: Thomas Rast, Joshua Redstone,
Nguyễn Thái Ngọc Duy
In-Reply-To: <1328507319-24687-1-git-send-email-pclouds@gmail.com>
An experiment with -O3 is done on Intel D510@1.66GHz. At around 250k
entries, index reading time exceeds 0.5s. Switching to crc32 brings it
back lower than 0.2s.
On 4M files index, reading time with SHA-1 takes ~8.4, crc32 2.8s.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
I know no real repositories this size though. gentoo-x86 is "only"
120k. Haven't checked libreoffice repo yet.
On 2M files index, allocating one big block (i.e. reverting debed2a
(read-cache.c: allocate index entries individually - 2011-10-24)
saves about 0.3s. Maybe we can allocate one big block, then malloc
separately when the block is fully used.
Writing time is still high. "git update-index --crc32" on crc32 250k index
takes 0.9s (so writing time is about 0.5s)
A better solution may be narrow clone (or just the narrow checkout
part), where index only contains entries from checked out
subdirectories.
Documentation/config.txt | 7 +++++++
builtin/update-index.c | 1 +
cache.h | 1 +
config.c | 5 +++++
environment.c | 1 +
read-cache.c | 8 ++++++++
6 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..55b7596 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -540,6 +540,13 @@ relatively high IO latencies. With this set to 'true', git will do the
index comparison to the filesystem data in parallel, allowing
overlapping IO's.
+core.crc32IndexThreshold::
+ Usually SHA-1 is used to check for index integerity. When the
+ number of entries in index exceeds this threshold, crc32 will
+ be used instead. Zero means SHA-1 always be used. Negative
+ value disables this threshold (i.e. crc32 or SHA-1 is decided
+ by other means).
+
core.createObject::
You can set this to 'link', in which case a hardlink followed by
a delete of the source are used to make sure that object creation
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6913226..5cb51c7 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -856,6 +856,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
argc = parse_options_end(&ctx);
if (do_crc != -1) {
+ core_crc32_index_threshold = -1;
if (do_crc)
the_index.hdr_flags |= CACHE_F_CRC;
else
diff --git a/cache.h b/cache.h
index 7352402..d05856b 100644
--- a/cache.h
+++ b/cache.h
@@ -610,6 +610,7 @@ extern unsigned long pack_size_limit_cfg;
extern int read_replace_refs;
extern int fsync_object_files;
extern int core_preload_index;
+extern int core_crc32_index_threshold;
extern int core_apply_sparse_checkout;
enum branch_track {
diff --git a/config.c b/config.c
index 40f9c6d..905e071 100644
--- a/config.c
+++ b/config.c
@@ -671,6 +671,11 @@ static int git_default_core_config(const char *var, const char *value)
return 0;
}
+ if (!strcmp(var, "core.crc32indexthreshold")) {
+ core_crc32_index_threshold = git_config_int(var, value);
+ return 0;
+ }
+
if (!strcmp(var, "core.createobject")) {
if (!strcmp(value, "rename"))
object_creation_mode = OBJECT_CREATION_USES_RENAMES;
diff --git a/environment.c b/environment.c
index c93b8f4..9d9dfc2 100644
--- a/environment.c
+++ b/environment.c
@@ -66,6 +66,7 @@ unsigned long pack_size_limit_cfg;
/* Parallel index stat data preload? */
int core_preload_index = 0;
+int core_crc32_index_threshold = 250000;
/* This is set by setup_git_dir_gently() and/or git_default_config() */
char *git_work_tree_cfg;
diff --git a/read-cache.c b/read-cache.c
index a34878e..fd032d8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1582,6 +1582,14 @@ int write_index(struct index_state *istate, int newfd)
}
}
+ if (core_crc32_index_threshold >= 0) {
+ if (core_crc32_index_threshold > 0 &&
+ istate->cache_nr >= core_crc32_index_threshold)
+ istate->hdr_flags |= CACHE_F_CRC;
+ else
+ istate->hdr_flags &= ~CACHE_F_CRC;
+ }
+
hdr.h.hdr_signature = htonl(CACHE_SIGNATURE);
if (istate->hdr_flags) {
hdr.h.hdr_version = htonl(4);
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Jeff King @ 2012-02-06 5:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <7vk440a5qw.fsf@alter.siamese.dyndns.org>
On Sun, Feb 05, 2012 at 09:30:15PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Sure, that's one way to do it. But I don't see any point in not allowing
> > "git checkout -b" to be another way of doing it. Is there some other use
> > case for "git checkout -b" from an unborn branch? Or is there some
> > harmful outcome that can come from doing so that we need to be
> > protecting against? Am I missing something?
>
> Mostly because it is wrong at the conceptual level to do so.
>
> git checkout -b foo
>
> is a short-hand for
>
> git checkout -b foo HEAD
>
> which is a short-hand for
>
> git branch foo HEAD &&
> git checkout foo
>
> But the last one has no chance of working if you think about it, because
> "git branch foo $start" is a way to start a branch at $start and you need
> to have something to point at with refs/heads/foo.
>
> So we are breaking the equivalence between these three only when HEAD
> points at an unborn branch.
I think it is only wrong at the conceptual level because you have
specified the concepts in such a way that it is so. That is how git does
it _now_, but the whole point of this is to change git's behavior to
handle a potentially useful special case. I could also say this: "git
checkout -b foo HEAD" does two things:
1. create a new branch "foo" pointing to the current sha1 of HEAD
2. point the HEAD symref at refs/heads/foo
And then the proposed behavior might amend the first point to say:
1. if HEAD points to an existing ref, then create a new branch...
which is perfectly consistent and simple. It does violate your "X is a
short-hand for Y" above, but why is that a bad thing? It seems you are
arguing against a special case _because_ it is a special case, not
because it is not a reasonable thing to do or expect.
Anyway. I am still not convinced that this is even a useful thing to
want to do, so I am certainly not volunteering to write such a patch. So
perhaps there is no point arguing about it.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Tom Grennan @ 2012-02-06 5:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jasampler
In-Reply-To: <7vvcnkeu2i.fsf@alter.siamese.dyndns.org>
On Sun, Feb 05, 2012 at 03:31:17PM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> @@ -105,16 +107,28 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>> return 0;
>> }
>>
>> + buf = read_sha1_file(sha1, &type, &size);
>> + if (!buf || !size)
>> + return 0;
>> +
>> + if (filter->points_at) {
>> + unsigned char tagged_sha1[20];
>> + if (memcmp("object ", buf, 7) \
>> + || buf[47] != '\n' \
>> + || get_sha1_hex(buf + 7, tagged_sha1) \
>> + || memcmp(filter->points_at, tagged_sha1, 20)) {
>
>Do we need these backslashes at the end of these lines?
No, just an old habit. Thanks.
>> @@ -143,16 +157,20 @@ static int show_reference(const char *refname, const unsigned char *sha1,
>> }
>>
>> static int list_tags(const char **patterns, int lines,
>> - struct commit_list *with_commit)
>> + struct commit_list *with_commit,
>> + unsigned char *points_at)
>> {
>
>It strikes me somewhat odd that you can give a list of commits to filter
>when using "--contains" (e.g. "--contains v1.7.9 --contains 1.7.8.4"), but
>you can only ask for a single object with "--points-at" from the UI point
>of view.
>
>> @@ -375,12 +393,28 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
>> return check_refname_format(sb->buf, 0);
>> }
>>
>> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
>> +{
>> + unsigned char *sha1;
>> +
>> + if (!arg)
>> + return -1;
>> + sha1 = xmalloc(20);
>> + if (get_sha1(arg, sha1)) {
>> + free(sha1);
>> + return error("malformed object name %s", arg);
>> + }
>> + *(unsigned char **)opt->value = sha1;
>> + return 0;
>> +}
>
>We are ignoring earlier --points-at argument without telling the user that
>we do not support more than one.
>
>Would it become too much unnecessary addition of new code if you supported
>multiple --points-at on the command line for the sake of consistency?
OK, I'll implement multiple args to be consistent with contains and patterns.
>> @@ -417,6 +451,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>> PARSE_OPT_LASTARG_DEFAULT,
>> parse_opt_with_commit, (intptr_t)"HEAD",
>> },
>> + {
>> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> + "print only annotated|signed tags of the object",
>> + PARSE_OPT_LASTARG_DEFAULT,
>> + parse_opt_points_at, (intptr_t)"HEAD",
>> + },
>
>I wonder if defaulting to HEAD even makes sense for --points-at. When you
>are chasing a bug and checked out an old version that originally had
>problem, "git tag --contains" that defaults to HEAD does have a value. It
>tells us what releases are potentially contaminated with the buggy commit.
>
>But does a similar use case support points-at that defaults to HEAD?
Yes, the usage, "--points-at <object>..." implies that there is no
default. So, I suppose that NULL more appropriate than "HEAD".
Should I make the "contains" usage indicate that "commit" is optional
like this?
"[--contains [<commit>...]] [--points-at <object>..]"
>Other than that, thanks for a pleasant read.
Thanks,
TomG
^ permalink raw reply
* Re: [PATCH] Fix build problems related to profile-directed optimization
From: Ted Ts'o @ 2012-02-06 5:57 UTC (permalink / raw)
To: Jeff King; +Cc: git, Andi Kleen
In-Reply-To: <20120206041839.GB29365@sigill.intra.peff.net>
On Sun, Feb 05, 2012 at 11:18:39PM -0500, Jeff King wrote:
> > - $ make profile-all
> > - # make prefix=... install
> > + $ make --prefix=/usr PROFILE=BUILD all
> > + # make --prefix=/usr PROFILE=BUILD install
>
> Eh? --prefix?
Oops, configure meme strikes; will fix.
> > +As a caveat: a profile-optimized build takes a *lot* longer since it
> > +is the sources have to be built twice, and in order for the profiling
>
> s/it is//
Thanks, will fix.
> > +ifeq "$(PROFILE)" "GEN"
> > + CFLAGS += -fprofile-generate=$(PROFILE_DIR) -DNO_NORETURN=1
> > + EXTLIBS += -lgcov
> > + export CCACHE_DISABLE=t
> > + V=1
> > +else ifneq "$PROFILE" ""
>
> Did you mean "$(PROFILE)" in the second conditional?
Yes, thanks. Will fix.
- Ted
^ permalink raw reply
* [PATCH] Fix build problems related to profile-directed optimization
From: Theodore Ts'o @ 2012-02-06 6:00 UTC (permalink / raw)
To: git; +Cc: Theodore Ts'o, Andi Kleen
In-Reply-To: <20120206055750.GA6615@thunk.org>
There was a number of problems I ran into when trying the
profile-directed optimizations added by Andi Kleen in git commit
7ddc2710b9. (This was using gcc 4.4 found on many enterprise
distros.)
1) The -fprofile-generate and -fprofile-use commands are incompatible
with ccache; the code ends up looking in the wrong place for the gcda
files based on the ccache object names.
2) If the makefile notices that CFLAGS are different, it will rebuild
all of the binaries. Hence the recipe originally specified by the
INSTALL file ("make profile-all" followed by "make install") doesn't
work. It will appear to work, but the binaries will end up getting
built with no optimization.
This patch fixes this by using an explicit set of options passed via
the PROFILE variable then using this to directly manipulate CFLAGS and
EXTLIBS.
The developer can run "make PROFILE=BUILD all ; sudo make
PROFILE=BUILD install" automatically run a two-pass build with the
test suite run in between as the sample workload for the purpose of
recording profiling information to do the profile-directed
optimization.
Alternatively, the profiling version of binaries can be built using:
make PROFILE=GEN PROFILE_DIR=/var/cache/profile all
make PROFILE=GEN install
and then after git has been used for a while, the optimized version of
the binary can be built as follows:
make PROFILE=USE PROFILE_DIR=/var/cache/profile all
make PROFILE=USE install
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andi Kleen <ak@linux.intel.com>
---
INSTALL | 17 +++++++++++++----
Makefile | 53 +++++++++++++++++++++++++++++++++++------------------
2 files changed, 48 insertions(+), 22 deletions(-)
diff --git a/INSTALL b/INSTALL
index 6fa83fe..58b2b86 100644
--- a/INSTALL
+++ b/INSTALL
@@ -28,16 +28,25 @@ set up install paths (via config.mak.autogen), so you can write instead
If you're willing to trade off (much) longer build time for a later
faster git you can also do a profile feedback build with
- $ make profile-all
- # make prefix=... install
+ $ make prefix=/usr PROFILE=BUILD all
+ # make prefix=/usr PROFILE=BUILD install
This will run the complete test suite as training workload and then
rebuild git with the generated profile feedback. This results in a git
which is a few percent faster on CPU intensive workloads. This
may be a good tradeoff for distribution packagers.
-Note that the profile feedback build stage currently generates
-a lot of additional compiler warnings.
+Or if you just want to install a profile-optimized version of git into
+your home directory, you could run:
+
+ $ make PROFILE=BUILD install
+
+As a caveat: a profile-optimized build takes a *lot* longer since the
+git tree must be built twice, and in order for the profiling
+measurements to work properly, ccache must be disabled and the test
+suite has to be run using only a single CPU. In addition, the profile
+feedback build stage currently generates a lot of additional compiler
+warnings.
Issues of note:
diff --git a/Makefile b/Makefile
index c457c34..719ffca 100644
--- a/Makefile
+++ b/Makefile
@@ -1772,6 +1772,24 @@ ifdef ASCIIDOC7
export ASCIIDOC7
endif
+### profile feedback build
+#
+
+# Can adjust this to be a global directory if you want to do extended
+# data gathering
+PROFILE_DIR := $(CURDIR)
+
+ifeq "$(PROFILE)" "GEN"
+ CFLAGS += -fprofile-generate=$(PROFILE_DIR) -DNO_NORETURN=1
+ EXTLIBS += -lgcov
+ export CCACHE_DISABLE=t
+ V=1
+else ifneq "$(PROFILE)" ""
+ CFLAGS += -fprofile-use=$(PROFILE_DIR) -fprofile-correction -DNO_NORETURN=1
+ export CCACHE_DISABLE=t
+ V=1
+endif
+
# Shell quote (do not use $(call) to accommodate ancient setups);
SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
@@ -1828,7 +1846,17 @@ export DIFF TAR INSTALL DESTDIR SHELL_PATH
SHELL = $(SHELL_PATH)
-all:: shell_compatibility_test $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: shell_compatibility_test
+
+ifeq "$(PROFILE)" "BUILD"
+ifeq ($(filter all,$(MAKECMDGOALS)),all)
+all:: profile-clean
+ $(MAKE) PROFILE=GEN all
+ $(MAKE) PROFILE=GEN -j1 test
+endif
+endif
+
+all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
ifneq (,$X)
$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
endif
@@ -2557,7 +2585,11 @@ distclean: clean
$(RM) configure
$(RM) po/git.pot
-clean:
+profile-clean:
+ $(RM) $(addsuffix *.gcda,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
+ $(RM) $(addsuffix *.gcno,$(addprefix $(PROFILE_DIR)/, $(object_dirs)))
+
+clean: profile-clean
$(RM) *.o block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o vcs-svn/*.o \
builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
@@ -2587,7 +2619,7 @@ ifndef NO_TCLTK
endif
$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
-.PHONY: all install clean strip
+.PHONY: all install profile-clean clean strip
.PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
.PHONY: FORCE cscope
@@ -2697,18 +2729,3 @@ cover_db: coverage-report
cover_db_html: cover_db
cover -report html -outputdir cover_db_html cover_db
-### profile feedback build
-#
-.PHONY: profile-all profile-clean
-
-PROFILE_GEN_CFLAGS := $(CFLAGS) -fprofile-generate -DNO_NORETURN=1
-PROFILE_USE_CFLAGS := $(CFLAGS) -fprofile-use -fprofile-correction -DNO_NORETURN=1
-
-profile-clean:
- $(RM) $(addsuffix *.gcda,$(object_dirs))
- $(RM) $(addsuffix *.gcno,$(object_dirs))
-
-profile-all: profile-clean
- $(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" all
- $(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" -j1 test
- $(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" all
--
1.7.9.107.g8e04a
^ permalink raw reply related
* Re: [RFC/PATCH] tag: add --points-at list option
From: Junio C Hamano @ 2012-02-06 6:25 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, jasampler
In-Reply-To: <20120206054819.GB10489@tgrennan-laptop>
Tom Grennan <tmgrennan@gmail.com> writes:
>>I wonder if defaulting to HEAD even makes sense for --points-at. When you
>>are chasing a bug and checked out an old version that originally had
>>problem, "git tag --contains" that defaults to HEAD does have a value. It
>>tells us what releases are potentially contaminated with the buggy commit.
>>
>>But does a similar use case support points-at that defaults to HEAD?
>
> Yes, the usage, "--points-at <object>..." implies that there is no
> default. So, I suppose that NULL more appropriate than "HEAD".
That's a circular logic.
The usage could very well say "--points-at <object>" and forbid missing
<object>. I think that would make a lot _more_ sense, because I did not
think of offhand any good reason that --points-at should default to HEAD
to support some common usage, and you also seem to be unable to.
^ permalink raw reply
* [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-06 6:27 UTC (permalink / raw)
To: git
I decided to drop the "read config from a ref" bits of the series. There
was some debate about what the proper workflow should be around projects
sharing config. And since I am not part of a project that does so, I
can't contribute anything empirical to the discussion. So I'll let that
part live on in the list archive, and somebody whose project really
wants to experiment with it can feel free to do so.
That leaves the file-inclusion bits:
[1/2]: imap-send: remove dead code
[2/2]: config: add include directive
The first patch is new in this round, and is a necessary cleanup for the
second patch (though it might be worth applying regardless).
The second patch corresponds to patch 1/4 from the previous round. Among
the functional changes are:
1. do not use includes for "git config" in single-file mode
2. do not respect include.foo.path
3. perform cycle and duplicate detection on included files
Doing (3) actually ended up changing the implementation a fair bit. The
original implementation worked purely as a drop-in callback wrapper. But
to suppress duplicates, you need to know not just which files you've
included, but which files you started from. So either
git_config_from_file has to learn about the include mechanism, or each
caller has to manually mark the file it is about to read.
I ended up just teaching the config machinery about the include
mechanism. It meant more changes (because we now pass an extra "include"
argument between the config functions) but I think turns out much more
readable.
That being said, I'm having doubts about the idea of duplicate
suppression at all. Suppose I have a setup like this:
git config -f foo test.value foo
git config test.value repo
git config include.path $PWD/foo
git config --global test.value global
git config --global include.path $PWD/foo
That is, two config files, each of which sets test.value but also
includes "foo". One might assume from this that the output of "git
config test.value" would be "foo". But because of duplicate suppression,
it's not.
We first read the global config, which sets the value to "global", then
includes foo, which overwrites it to "foo". Then we read the repo
config, which sets the value to "repo", and then does _not_ actually
read foo. Because git config is read linearly and later values tend to
overwrite earlier ones, we would want to suppress the _first_ instance
of a file, not the second (or really, the final if it is included many
times). But that is impossible to do without generating a complete graph
of includes and then pruning the early ones.
Sure, this is a trivial toy example, and by looking at all of the
inputs, you can see what is happening and fix it. But the point of
duplicate suppression was that individual config files wouldn't have to
know or care what was being included elsewhere. A slightly more
real-world example is this:
1. A config file "/etc/git-defaults-foo.cfg" sets a bunch of
reasonable defaults for some group of developers.
2. One of the developers really likes these foo defaults, but wants to
override one particular option. So he does this:
git config --global include.path /etc/git-defaults-foo.cfg
git config --global some.option override
3. On one particular repo, though, this dev wants the stock foo
defaults. So he does:
git config --global include.path /etc/git-defaults-foo.cfg
But we ignore the final include request; it's suppressed as a duplicate,
even though it would cancel his override.
So I'm actually thinking I should drop the duplicate suppression and
just do some sort of sanity check on include-depth to break cycles
(i.e., just die because it's a crazy thing to do, and we are really just
trying to tell the user their config is broken rather than go into an
infinite loop). As a bonus, it makes the code much simpler, too.
I'll post the patch with duplicate suppression for reference, but read
it with a grain of salt (or don't bother to read it at all if after
reading the above you think it should be thrown out).
-Peff
^ permalink raw reply
* [PATCH 1/2] imap-send: remove dead code
From: Jeff King @ 2012-02-06 6:29 UTC (permalink / raw)
To: git
In-Reply-To: <20120206062713.GA9699@sigill.intra.peff.net>
The imap-send code was adapted from another project, and
still contains many unused bits of code. One of these bits
contains a type "struct string_list" which bears no
resemblence to the "struct string_list" we use elsewhere in
git. This causes the compiler to complain if git's
string_list ever becomes part of cache.h.
Let's just drop the dead code.
Signed-off-by: Jeff King <peff@peff.net>
---
This is necessary for patch 2, which does include string-list.h in
cache.h. If you read my cover letter, I think patch 2 might not be the
best approach. However, I think it might be worth applying this just as
a cleanup (e.g., even without the build problems, grepping for "struct
string_list" turns up this useless cruft).
imap-send.c | 23 -----------------------
1 files changed, 0 insertions(+), 23 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index e40125a..972ad62 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -42,28 +42,6 @@ struct store_conf {
unsigned trash_remote_new:1, trash_only_new:1;
};
-struct string_list {
- struct string_list *next;
- char string[1];
-};
-
-struct channel_conf {
- struct channel_conf *next;
- char *name;
- struct store_conf *master, *slave;
- char *master_name, *slave_name;
- char *sync_state;
- struct string_list *patterns;
- int mops, sops;
- unsigned max_messages; /* for slave only */
-};
-
-struct group_conf {
- struct group_conf *next;
- char *name;
- struct string_list *channels;
-};
-
/* For message->status */
#define M_RECENT (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */
#define M_DEAD (1<<1) /* expunged */
@@ -71,7 +49,6 @@ struct group_conf {
struct message {
struct message *next;
- /* struct string_list *keywords; */
size_t size; /* zero implies "not fetched" */
int uid;
unsigned char flags, status;
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* [PATCH 2/2] config: add include directive
From: Jeff King @ 2012-02-06 6:31 UTC (permalink / raw)
To: git
In-Reply-To: <20120206062713.GA9699@sigill.intra.peff.net>
[If you haven't read my cover letter, do so now. The surprise ending is
that I think this patch may not be the right approach, and I am posting
it for reference. I don't want anybody to waste time reading and
reviewing this without that context.]
-- >8 --
It can be useful to split your ~/.gitconfig across multiple
files. For example, you might have a "main" file which is
used on many machines, but a small set of per-machine
tweaks. Or you may want to make some of your config public
(e.g., clever aliases) while keeping other data back (e.g.,
your name or other identifying information). Or you may want
to include a number of config options in some subset of your
repos without copying and pasting (e.g., you want to
reference them from the .git/config of participating repos).
This patch introduces an include directive for config files.
It looks like:
[include]
path = /path/to/file
This is syntactically backwards-compatible with existing git
config parsers (i.e., they will see it as another config
entry and ignore it unless you are looking up include.path).
The implementation adds a new "include" parameter to
git_config_from_file to turn on includes. Include
directives are turned on for "regular" git config parsing:
when you call git_config(), or when you make a call to the
"git config" program without using any single-file options.
Includes are off in other cases, including:
1. Parsing of other config-like files, like .gitmodules.
There isn't a real need, and I'd rather be conservative
and avoid unnecessary incompatibility or confusion.
2. Reading single files via "git config". This is for two
reasons:
a. backwards compatibility with scripts looking at
config-like files.
b. inspection of a specific file probably means you
care about just what's in that file, not a general
lookup for "do we have this value anywhere at
all". If that is not the case, the caller can
always specify "--includes".
3. Writing files via "git config"; we want to treat
include.* variables as literal items to be copied (or
modified), and not expand them. So "git config
--unset-all foo.bar" would operate _only_ on
.git/config, not any of its included files (just as it
also does not operate on ~/.gitconfig).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 15 ++++
Documentation/git-config.txt | 5 ++
builtin/clone.c | 2 +-
builtin/config.c | 19 ++++--
builtin/init-db.c | 2 +-
cache.h | 19 ++++-
config.c | 118 ++++++++++++++++++++++++++++------
sequencer.c | 2 +-
setup.c | 2 +-
submodule.c | 2 +-
t/t1305-config-include.sh | 145 ++++++++++++++++++++++++++++++++++++++++++
11 files changed, 295 insertions(+), 36 deletions(-)
create mode 100755 t/t1305-config-include.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..e55dae1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -84,6 +84,17 @@ customary UNIX fashion.
Some variables may require a special value format.
+Includes
+~~~~~~~~
+
+You can include one config file from another by setting the special
+`include.path` variable to the name of the file to be included. The
+included file is expanded immediately, as if its contents had been
+found at the location of the include directive. If the value of the
+`include.path` variable is a relative path, the path is considered to be
+relative to the configuration file in which the include directive was
+found. See below for examples.
+
Example
~~~~~~~
@@ -106,6 +117,10 @@ Example
gitProxy="ssh" for "kernel.org"
gitProxy=default-proxy ; for the rest
+ [include]
+ path = /path/to/foo.inc ; include by absolute path
+ path = foo ; expand "foo" relative to the current file
+
Variables
~~~~~~~~~
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e7ecf5d..aa8303b 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -178,6 +178,11 @@ See also <<FILES>>.
Opens an editor to modify the specified config file; either
'--system', '--global', or repository (default).
+--includes::
+--no-includes::
+ Respect `include.*` directives in config files when looking up
+ values. Defaults to on.
+
[[FILES]]
FILES
-----
diff --git a/builtin/clone.c b/builtin/clone.c
index c62d4b5..32b7808 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -614,7 +614,7 @@ static void write_config(struct string_list *config)
for (i = 0; i < config->nr; i++) {
if (git_config_parse_parameter(config->items[i].string,
- write_one_config, NULL) < 0)
+ write_one_config, NULL, NULL) < 0)
die("unable to write parameters to config file");
}
}
diff --git a/builtin/config.c b/builtin/config.c
index d35c06a..be5d0fb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,6 +25,7 @@ static const char *given_config_file;
static int actions, types;
static const char *get_color_slot, *get_colorbool_slot;
static int end_null;
+static struct config_include_context include = CONFIG_INCLUDE_INIT(-1);
#define ACTION_GET (1<<0)
#define ACTION_GET_ALL (1<<1)
@@ -74,6 +75,7 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "path", &types, "value is a path (file or directory name)", TYPE_PATH),
OPT_GROUP("Other"),
OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"),
+ OPT_BOOL(0, "includes", &include.enabled, "respect include directives on lookup"),
OPT_END(),
};
@@ -214,18 +216,18 @@ static int get_value(const char *key_, const char *regex_)
}
if (do_all && system_wide)
- git_config_from_file(show_config, system_wide, NULL);
+ git_config_from_file(show_config, system_wide, NULL, &include);
if (do_all && global)
- git_config_from_file(show_config, global, NULL);
+ git_config_from_file(show_config, global, NULL, &include);
if (do_all)
- git_config_from_file(show_config, local, NULL);
- git_config_from_parameters(show_config, NULL);
+ git_config_from_file(show_config, local, NULL, &include);
+ git_config_from_parameters(show_config, NULL, &include);
if (!do_all && !seen)
- git_config_from_file(show_config, local, NULL);
+ git_config_from_file(show_config, local, NULL, &include);
if (!do_all && !seen && global)
- git_config_from_file(show_config, global, NULL);
+ git_config_from_file(show_config, global, NULL, &include);
if (!do_all && !seen && system_wide)
- git_config_from_file(show_config, system_wide, NULL);
+ git_config_from_file(show_config, system_wide, NULL, &include);
free(key);
if (regexp) {
@@ -384,6 +386,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
config_exclusive_filename = given_config_file;
}
+ if (include.enabled == -1)
+ include.enabled = !config_exclusive_filename;
+
if (end_null) {
term = '\0';
delim = '\n';
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0dacb8b..de6290f 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -146,7 +146,7 @@ static void copy_templates(const char *template_dir)
strcpy(template_path + template_len, "config");
repository_format_version = 0;
git_config_from_file(check_repository_format_version,
- template_path, NULL);
+ template_path, NULL, NULL);
template_path[template_len] = 0;
if (repository_format_version &&
diff --git a/cache.h b/cache.h
index 9bd8c2d..f75923e 100644
--- a/cache.h
+++ b/cache.h
@@ -7,6 +7,7 @@
#include "advice.h"
#include "gettext.h"
#include "convert.h"
+#include "string-list.h"
#include SHA1_HEADER
#ifndef git_SHA_CTX
@@ -1108,13 +1109,22 @@ extern int update_server_info(int);
#define CONFIG_NOTHING_SET 5
#define CONFIG_INVALID_PATTERN 6
+struct config_include_context {
+ struct string_list seen_paths;
+ int enabled;
+};
+#define CONFIG_INCLUDE_INIT(enable) { STRING_LIST_INIT_DUP, (enable) }
+
typedef int (*config_fn_t)(const char *, const char *, void *);
extern int git_default_config(const char *, const char *, void *);
-extern int git_config_from_file(config_fn_t fn, const char *, void *);
+extern int git_config_from_file(config_fn_t fn, const char *, void *,
+ struct config_include_context *);
extern void git_config_push_parameter(const char *text);
-extern int git_config_from_parameters(config_fn_t fn, void *data);
+extern int git_config_from_parameters(config_fn_t fn, void *data,
+ struct config_include_context *inc);
extern int git_config(config_fn_t fn, void *);
-extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
+extern int git_config_early(config_fn_t fn, void *, const char *repo_config,
+ struct config_include_context *inc);
extern int git_parse_ulong(const char *, unsigned long *);
extern int git_config_int(const char *, const char *);
extern unsigned long git_config_ulong(const char *, const char *);
@@ -1137,7 +1147,8 @@ extern int config_error_nonbool(const char *);
extern const char *get_log_output_encoding(void);
extern const char *get_commit_output_encoding(void);
-extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data,
+ struct config_include_context *inc);
extern const char *config_exclusive_filename;
diff --git a/config.c b/config.c
index 40f9c6d..9b90751 100644
--- a/config.c
+++ b/config.c
@@ -28,6 +28,71 @@ static int zlib_compression_seen;
const char *config_exclusive_filename = NULL;
+static int check_and_mark_include_path(struct config_include_context *inc,
+ const char *path)
+{
+ const char *canonical;
+
+ if (!inc || !inc->enabled)
+ return 0;
+
+ /*
+ * Using real_path would catch more duplicates, but we can't use it
+ * here. It will die if some path components don't exist, and we have
+ * no promise from the path in question actually exists.
+ */
+ canonical = absolute_path(path);
+ if (unsorted_string_list_lookup(&inc->seen_paths, canonical))
+ return 1;
+
+ string_list_append(&inc->seen_paths, canonical);
+ return 0;
+}
+
+static int handle_path_include(const char *path, config_fn_t fn, void *data,
+ struct config_include_context *inc)
+{
+ int ret = 0;
+ struct strbuf buf = STRBUF_INIT;
+
+ /*
+ * Use an absolute path as-is, but interpret relative paths
+ * based on the including config file.
+ */
+ if (!is_absolute_path(path)) {
+ char *slash;
+
+ if (!cf || !cf->name)
+ return error("relative config includes must from from files");
+
+ slash = find_last_dir_sep(cf->name);
+ if (slash)
+ strbuf_add(&buf, cf->name, slash - cf->name + 1);
+ strbuf_addf(&buf, "%s", path);
+ path = buf.buf;
+ }
+
+ if (!access(path, R_OK))
+ ret = git_config_from_file(fn, path, data, inc);
+ strbuf_release(&buf);
+ return ret;
+}
+
+static int handle_config_variable(const char *var, const char *value,
+ config_fn_t fn, void *data,
+ struct config_include_context *inc)
+{
+ int r;
+
+ r = fn(var, value, data);
+ if (!r && inc && inc->enabled && value && !prefixcmp(var, "include.")) {
+ const char *type = var + 8;
+ if (!strcmp(type, "path"))
+ r = handle_path_include(value, fn, data, inc);
+ }
+ return r;
+}
+
static void lowercase(char *p)
{
for (; *p; p++)
@@ -47,8 +112,8 @@ void git_config_push_parameter(const char *text)
strbuf_release(&env);
}
-int git_config_parse_parameter(const char *text,
- config_fn_t fn, void *data)
+int git_config_parse_parameter(const char *text, config_fn_t fn, void *data,
+ struct config_include_context *inc)
{
struct strbuf **pair;
pair = strbuf_split_str(text, '=', 2);
@@ -62,7 +127,10 @@ int git_config_parse_parameter(const char *text,
return error("bogus config parameter: %s", text);
}
lowercase(pair[0]->buf);
- if (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {
+
+ if (handle_config_variable(pair[0]->buf,
+ pair[1] ? pair[1]->buf : NULL,
+ fn, data, inc) < 0) {
strbuf_list_free(pair);
return -1;
}
@@ -70,7 +138,8 @@ int git_config_parse_parameter(const char *text,
return 0;
}
-int git_config_from_parameters(config_fn_t fn, void *data)
+int git_config_from_parameters(config_fn_t fn, void *data,
+ struct config_include_context *inc)
{
const char *env = getenv(CONFIG_DATA_ENVIRONMENT);
char *envw;
@@ -89,7 +158,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)
}
for (i = 0; i < nr; i++) {
- if (git_config_parse_parameter(argv[i], fn, data) < 0) {
+ if (git_config_parse_parameter(argv[i], fn, data, inc) < 0) {
free(argv);
free(envw);
return -1;
@@ -191,7 +260,8 @@ static inline int iskeychar(int c)
return isalnum(c) || c == '-';
}
-static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
+static int get_value(config_fn_t fn, void *data, char *name, unsigned int len,
+ struct config_include_context *inc)
{
int c;
char *value;
@@ -219,7 +289,8 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
if (!value)
return -1;
}
- return fn(name, value, data);
+
+ return handle_config_variable(name, value, fn, data, inc);
}
static int get_extended_base_var(char *name, int baselen, int c)
@@ -277,7 +348,8 @@ static int get_base_var(char *name)
}
}
-static int git_parse_file(config_fn_t fn, void *data)
+static int git_parse_file(config_fn_t fn, void *data,
+ struct config_include_context *inc)
{
int comment = 0;
int baselen = 0;
@@ -327,7 +399,7 @@ static int git_parse_file(config_fn_t fn, void *data)
if (!isalpha(c))
break;
var[baselen] = tolower(c);
- if (get_value(fn, data, var, baselen+1) < 0)
+ if (get_value(fn, data, var, baselen+1, inc) < 0)
break;
}
die("bad config file line %d in %s", cf->linenr, cf->name);
@@ -826,11 +898,15 @@ int git_default_config(const char *var, const char *value, void *dummy)
return 0;
}
-int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+int git_config_from_file(config_fn_t fn, const char *filename, void *data,
+ struct config_include_context *inc)
{
int ret;
FILE *f = fopen(filename, "r");
+ if (check_and_mark_include_path(inc, filename))
+ return 0;
+
ret = -1;
if (f) {
config_file top;
@@ -844,7 +920,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
strbuf_init(&top.value, 1024);
cf = ⊤
- ret = git_parse_file(fn, data);
+ ret = git_parse_file(fn, data, inc);
/* pop config-file parsing state stack */
strbuf_release(&top.value);
@@ -874,17 +950,17 @@ int git_config_system(void)
return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
}
-int git_config_early(config_fn_t fn, void *data, const char *repo_config)
+int git_config_early(config_fn_t fn, void *data, const char *repo_config,
+ struct config_include_context *inc)
{
int ret = 0, found = 0;
const char *home = NULL;
/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
if (config_exclusive_filename)
- return git_config_from_file(fn, config_exclusive_filename, data);
+ return git_config_from_file(fn, config_exclusive_filename, data, NULL);
if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
- ret += git_config_from_file(fn, git_etc_gitconfig(),
- data);
+ ret += git_config_from_file(fn, git_etc_gitconfig(), data, inc);
found += 1;
}
@@ -893,17 +969,17 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
char buf[PATH_MAX];
char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home);
if (!access(user_config, R_OK)) {
- ret += git_config_from_file(fn, user_config, data);
+ ret += git_config_from_file(fn, user_config, data, inc);
found += 1;
}
}
if (repo_config && !access(repo_config, R_OK)) {
- ret += git_config_from_file(fn, repo_config, data);
+ ret += git_config_from_file(fn, repo_config, data, inc);
found += 1;
}
- switch (git_config_from_parameters(fn, data)) {
+ switch (git_config_from_parameters(fn, data, inc)) {
case -1: /* error */
die("unable to parse command-line config");
break;
@@ -921,11 +997,13 @@ int git_config(config_fn_t fn, void *data)
{
char *repo_config = NULL;
int ret;
+ struct config_include_context inc = CONFIG_INCLUDE_INIT(1);
repo_config = git_pathdup("config");
- ret = git_config_early(fn, data, repo_config);
+ ret = git_config_early(fn, data, repo_config, &inc);
if (repo_config)
free(repo_config);
+ string_list_clear(&inc.seen_paths, 0);
return ret;
}
@@ -1313,7 +1391,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
* As a side effect, we make sure to transform only a valid
* existing config file.
*/
- if (git_config_from_file(store_aux, config_filename, NULL)) {
+ if (git_config_from_file(store_aux, config_filename, NULL, NULL)) {
error("invalid config file %s", config_filename);
free(store.key);
if (store.value_regex != NULL) {
diff --git a/sequencer.c b/sequencer.c
index 5fcbcb8..50562b9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -636,7 +636,7 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
if (!file_exists(opts_file))
return;
- if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
+ if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr, NULL) < 0)
die(_("Malformed options sheet: %s"), opts_file);
}
diff --git a/setup.c b/setup.c
index 61c22e6..947fa51 100644
--- a/setup.c
+++ b/setup.c
@@ -329,7 +329,7 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
* is a good one.
*/
snprintf(repo_config, PATH_MAX, "%s/config", gitdir);
- git_config_early(check_repository_format_version, NULL, repo_config);
+ git_config_early(check_repository_format_version, NULL, repo_config, NULL);
if (GIT_REPO_VERSION < repository_format_version) {
if (!nongit_ok)
die ("Expected git repo version <= %d, found %d",
diff --git a/submodule.c b/submodule.c
index 9a28060..4c5b9be 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,7 +116,7 @@ void gitmodules_config(void)
}
if (!gitmodules_is_unmerged)
- git_config_from_file(submodule_config, gitmodules_path.buf, NULL);
+ git_config_from_file(submodule_config, gitmodules_path.buf, NULL, NULL);
strbuf_release(&gitmodules_path);
}
}
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
new file mode 100755
index 0000000..3d5a46d
--- /dev/null
+++ b/t/t1305-config-include.sh
@@ -0,0 +1,145 @@
+#!/bin/sh
+
+test_description='test config file include directives'
+. ./test-lib.sh
+
+test_expect_success 'include file by absolute path' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = \"$PWD/one\"" >.gitconfig &&
+ echo 1 >expect &&
+ git config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'include file by relative path' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ echo 1 >expect &&
+ git config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'chained relative paths' '
+ mkdir subdir &&
+ echo "[test]three = 3" >subdir/three &&
+ echo "[include]path = three" >subdir/two &&
+ echo "[include]path = subdir/two" >.gitconfig &&
+ echo 3 >expect &&
+ git config test.three >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'include options can still be examined' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ echo one >expect &&
+ git config include.path >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'listing includes option and expansion' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ cat >expect <<-\EOF &&
+ include.path=one
+ test.one=1
+ EOF
+ git config --list >actual.full &&
+ grep -v ^core actual.full >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'single file lookup does not expand includes by default' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ test_must_fail git config -f .gitconfig test.one &&
+ test_must_fail git config --global test.one &&
+ echo 1 >expect &&
+ git config --includes -f .gitconfig test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'writing config file does not expand includes' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ git config test.two 2 &&
+ echo 2 >expect &&
+ git config --no-includes test.two >actual &&
+ test_cmp expect actual &&
+ test_must_fail git config --no-includes test.one
+'
+
+test_expect_success 'config modification does not affect includes' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ git config test.one 2 &&
+ echo 1 >expect &&
+ git config -f one test.one >actual &&
+ test_cmp expect actual &&
+ cat >expect <<-\EOF &&
+ 1
+ 2
+ EOF
+ git config --get-all test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'missing include files are ignored' '
+ cat >.gitconfig <<-\EOF &&
+ [include]path = foo
+ [test]value = yes
+ EOF
+ echo yes >expect &&
+ git config test.value >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'absolute includes from command line work' '
+ echo "[test]one = 1" >one &&
+ echo 1 >expect &&
+ git -c include.path="$PWD/one" config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'relative includes from command line fail' '
+ echo "[test]one = 1" >one &&
+ test_must_fail git -c include.path=one config test.one
+'
+
+test_expect_success 'include cycles are detected and broken' '
+ cat >.gitconfig <<-\EOF &&
+ [test]value = gitconfig
+ [include]path = cycle
+ EOF
+ cat >cycle <<-\EOF &&
+ [test]value = cycle
+ [include]path = .gitconfig
+ EOF
+ cat >expect <<-\EOF &&
+ gitconfig
+ cycle
+ EOF
+ git config --get-all test.value >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple includes of the same file are suppressed' '
+ cat >.gitconfig <<-\EOF &&
+ [test]
+ value = base
+ [include]
+ path = A
+ path = B
+ EOF
+ echo "[include]path = C" >A &&
+ echo "[include]path = C" >B &&
+ echo "[test]value = C" >C &&
+ cat >expect <<-\EOF &&
+ base
+ C
+ EOF
+ git config --get-all test.value >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* Re: [RFC/PATCH] tag: add --points-at list option
From: Tom Grennan @ 2012-02-06 6:32 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, krh, jasampler
In-Reply-To: <20120206000420.GC28735@sigill.intra.peff.net>
On Sun, Feb 05, 2012 at 07:04:21PM -0500, Jeff King wrote:
>On Sun, Feb 05, 2012 at 02:28:07PM -0800, Tom Grennan wrote:
>
>> This filters the list for annotated|signed tags of the given object.
>> Example,
>>
>> john$ git tag -s v1.0-john v1.0
>> john$ git tag -l --points-at v1.0
>> v1.0-john
>
>I really like this approach. One big question, and a few small comments:
>
>> +--points-at <object>::
>> + Only list annotated or signed tags of the given object.
>> +
>
>It is unclear to me from this documentation if we will only peel a
>single level, or if we will peel indefinitely. E.g., what will this
>show:
>
> $ git tag one v1.0
> $ git tag two one
> $ git tag --points-at=v1.0
>
>It will clearly show "one", but will it also show "two" (from reading
>the code, I think the answer is "no")? If not, should it?
Actually, neither one nor two would be listed as these are lightweight
tags. In the modified example,
$ git tag -a -m One one v1.0
$ git tag -a -m Two two one
$ git tag --points-at v1.0
one
$ git tag --points-at one
two
one's object is v1.0 whereas two's object is one
>> + buf = read_sha1_file(sha1, &type, &size);
>> + if (!buf || !size)
>> + return 0;
>
>Before your patch, a tag whose sha1 could not be read would get its name
>printed, and then we would later return without printing anything more.
>Now it won't get even the first bit printed.
>
>However, I'm not sure the old behavior wasn't buggy; it would print part
>of the line, but never actually print the newline.
If you prefer, I can restore the old behavior just moving the
condition/return back below the refname print; then add "buf" qualifier
to the following fragment and at each intermediate free.
>> + if (filter->points_at) {
>> + unsigned char tagged_sha1[20];
>> + if (memcmp("object ", buf, 7) \
>> + || buf[47] != '\n' \
>> + || get_sha1_hex(buf + 7, tagged_sha1) \
>> + || memcmp(filter->points_at, tagged_sha1, 20)) {
>> + free(buf);
>> + return 0;
>> + }
>> + }
>
>Hmm, I would have expected to use parse_tag_buffer instead of doing it
>by hand. This is probably a tiny bit more efficient, but I wonder if the
>code complexity is worth it.
I didn't see how to get the object sha out of parse_tag_buffer() to
compare with "point_at". The inline conditions seem simple enough.
>
>> static int list_tags(const char **patterns, int lines,
>> - struct commit_list *with_commit)
>> + struct commit_list *with_commit,
>> + unsigned char *points_at)
>
>Like Junio, I was surprised this did not allow a list.
I agree and will change it.
Thanks,
TomG
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Tom Grennan @ 2012-02-06 6:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jasampler
In-Reply-To: <7v8vkga370.fsf@alter.siamese.dyndns.org>
On Sun, Feb 05, 2012 at 10:25:23PM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>>>I wonder if defaulting to HEAD even makes sense for --points-at. When you
>>>are chasing a bug and checked out an old version that originally had
>>>problem, "git tag --contains" that defaults to HEAD does have a value. It
>>>tells us what releases are potentially contaminated with the buggy commit.
>>>
>>>But does a similar use case support points-at that defaults to HEAD?
>>
>> Yes, the usage, "--points-at <object>..." implies that there is no
>> default. So, I suppose that NULL more appropriate than "HEAD".
>
>That's a circular logic.
>
>The usage could very well say "--points-at <object>" and forbid missing
><object>. I think that would make a lot _more_ sense, because I did not
>think of offhand any good reason that --points-at should default to HEAD
>to support some common usage, and you also seem to be unable to.
Sorry for the miss-communication. I agreed with you - at least I thought I did.
So, "--points-at <object>" should forbid a missing <object>.
I think I can do so by using defval = (intptr_t)NULL instead of "HEAD",
right?
--
TomG
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Johan Herland @ 2012-02-06 6:53 UTC (permalink / raw)
To: Steven Michalske; +Cc: Jakub Narebski, git
In-Reply-To: <EAF9D593-4E0C-4C95-A048-3F6AC8ADD866@gmail.com>
On Mon, Feb 6, 2012 at 01:57, Steven Michalske <smichalske@gmail.com> wrote:
> On Feb 4, 2012, at 11:45 AM, Jakub Narebski wrote:
>> In Mercurial 2.1 there are three available phases: 'public' for
>> published commits, 'draft' for local un-published commits and
>> 'secret' for local un-published commits which are not meant to
>> be published.
>>
>> The phase of a changeset is always equal to or higher than the phase
>> of it's descendants, according to the following order:
>>
>> public < draft < secret
>
> Let's not limit ourselves to just three levels. They are a great start but I propose the following.
>
> published - The commits that are on a public repository that if are rewritten will invoke uprisings.
> general rule here would be to revert or patch, no rewrites.
> based - The commits that the core developers have work based upon. (not just the commits in their repo.)
> general rule is notify your fellow developers before a rewrite.
> shared - The commits that are known to your fellow core developers.
> These commits are known, but have not had work based off of them. Minimal risk to rewrite.
> local - The commits that are local only, no one else has a copy.
> Commits your willing to share, but have not been yet shared, either from actions of you, or a fetch from others.
> restricted or private - The commits that you do not want shared.
> Manually added, think of a branch tip marked as restricted automatically promotes commits to the branch as restricted.
>
> Maybe make these like nice levels, but as two components, publicity 0-100 and rewritability 0-100
> Published is publicity 100 and rewritability 0
> Restricted is publicity 0 and rewritability 100
> Based publicity 75 and rewritability 25
> Shared publicity 50 and rewritability 50
> Local publicity 25 and rewritability 75
> Restricted publicity 0 and rewritability 100
>
> [...]
With all due respect, I believe this is crazy. You're adding an entire
layer of complexity on top of commits that every user has to know
about, and has little or no value to most of them. IMHO, most users
only want Git to help them avoid doing something stupid (rewriting
'public' commits or publishing 'secret' commits), and to do so with
the minimal amount of manual user interaction. The above idea is more
suitable for armchair dictators that want to micromanage their commits
along two arbitrary axes of evil^H^H^H^Hpointlessness. On both axes,
you'll need threshold values where Git starts refusing to
publish/rewrite your commit. Hence, the only thing that matters is
whether the 'publicity'/'rewritability' value is above/below that
threshold, at which point you could save yourself a lot of trouble by
making them simple boolean flags instead.
Having said that, you can use 'git notes' along with existing and
proposed hooks (as described elsewhere in this thread) to implement
whatever crazy commit publishing/rewriting scheme you desire. To
misquote someone famous: I disapprove of what you want to do with Git,
but I will defend to the death your right to make Git do what you want
(in the privacy of your own repos). ;)
Have fun! :)
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Jeff King @ 2012-02-06 7:04 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, krh, jasampler
In-Reply-To: <20120206063213.GC10489@tgrennan-laptop>
On Sun, Feb 05, 2012 at 10:32:13PM -0800, Tom Grennan wrote:
> >> +--points-at <object>::
> >> + Only list annotated or signed tags of the given object.
> >> +
> >
> >It is unclear to me from this documentation if we will only peel a
> >single level, or if we will peel indefinitely. E.g., what will this
> >show:
> >
> > $ git tag one v1.0
> > $ git tag two one
> > $ git tag --points-at=v1.0
> >
> >It will clearly show "one", but will it also show "two" (from reading
> >the code, I think the answer is "no")? If not, should it?
>
> Actually, neither one nor two would be listed as these are lightweight
> tags. In the modified example,
>
> $ git tag -a -m One one v1.0
> $ git tag -a -m Two two one
> $ git tag --points-at v1.0
> one
> $ git tag --points-at one
> two
Hmm. Yeah, I see that now. And re-reading your description, I see that
it is explicit only to read from tag objects. Somehow the name
"points-at" seems a bit misleading to me, then, as it implies being more
inclusive of all pointing, including lightweight tags.
I know that has nothing to do with your use-case though; I just wonder
if there could be a better name. I can't think of one, though, and I'm
not sure we will ever want a more inclusive --points-at, so maybe it is
not worth caring about.
> >Before your patch, a tag whose sha1 could not be read would get its name
> >printed, and then we would later return without printing anything more.
> >Now it won't get even the first bit printed.
> >
> >However, I'm not sure the old behavior wasn't buggy; it would print part
> >of the line, but never actually print the newline.
>
> If you prefer, I can restore the old behavior just moving the
> condition/return back below the refname print; then add "buf" qualifier
> to the following fragment and at each intermediate free.
Thinking on it more, your behavior is at least as good as the old. And
it only comes up in a broken repo, anyway, so trying to come up with
some kind of useful outcome is pointless.
> >> + if (filter->points_at) {
> >> + unsigned char tagged_sha1[20];
> >> + if (memcmp("object ", buf, 7) \
> >> + || buf[47] != '\n' \
> >> + || get_sha1_hex(buf + 7, tagged_sha1) \
> >> + || memcmp(filter->points_at, tagged_sha1, 20)) {
> >> + free(buf);
> >> + return 0;
> >> + }
> >> + }
> >
> >Hmm, I would have expected to use parse_tag_buffer instead of doing it
> >by hand. This is probably a tiny bit more efficient, but I wonder if the
> >code complexity is worth it.
>
> I didn't see how to get the object sha out of parse_tag_buffer() to
> compare with "point_at". The inline conditions seem simple enough.
I think it would be:
struct tag *t = lookup_tag(sha1);
if (parse_tag_buffer(t, buf, size) < 0)
return 0; /* error, possibly should die() */
if (!hashcmp(filter->points_at, t->tagged.sha1))
/* matches */
That might bear a little bit of explanation. Git keeps a struct in
memory for each object, each of which contains a "struct object" at the
beginning. By calling lookup_tag, we either find an existing reference
to the tag with this sha1, or create a new "struct tag". And then we
parse it using the data we've read, storing it in the "struct tag" (for
our use, or for later use). The "tagged" member points to the tagged
object. Which again is a struct object; it may or may not have been
read and parsed, but we definitely know its sha1.
If this seems a little cumbersome, it is because the usual usage is more
like:
struct object *obj = parse_object(sha1);
if (!obj)
die("unable to read %s", sha1_to_hex(sha1));
if (obj->type == OBJ_TAG) {
struct tag *t = (struct tag *)obj;
if (!hashcmp(filter->points_at, t->tagged.sha1))
/* matched */
And then you don't have to bother with calling read_sha1_file at all.
BTW, writing that helped me notice two bugs in your patch:
1. You read up to 47 bytes into the buffer without ever checking
whether size >= 47.
2. You never check whether the object you read from read_sha1_file is
actually a tag.
So your patch would read random heap memory on something like:
blob=`echo foo | git hash-object --stdin -w`
git tag foo $blob
-Peff
^ permalink raw reply
* Re: [PATCH 3/6] Stop producing index version 2
From: Junio C Hamano @ 2012-02-06 7:10 UTC (permalink / raw)
To: spearce
Cc: Nguyễn Thái Ngọc Duy, git, Thomas Rast,
Joshua Redstone
In-Reply-To: <1328507319-24687-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> read-cache.c learned to produce version 2 or 3 depending on whether
> extended cache entries exist in 06aaaa0 (Extend index to save more flags
> - 2008-10-01), first released in 1.6.1. The purpose is to keep
> compatibility with older git. It's been more than three years since
> then and git has reached 1.7.9. Drop support for older git.
Cc'ing this, as I suspect this would surely raise eyebrows of some people
who wanted to get rid of the version 3 format.
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Jeff King @ 2012-02-06 7:13 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <20120206070424.GC9931@sigill.intra.peff.net>
On Mon, Feb 06, 2012 at 02:04:24AM -0500, Jeff King wrote:
> > >Before your patch, a tag whose sha1 could not be read would get its name
> > >printed, and then we would later return without printing anything more.
> > >Now it won't get even the first bit printed.
> > >
> > >However, I'm not sure the old behavior wasn't buggy; it would print part
> > >of the line, but never actually print the newline.
> >
> > If you prefer, I can restore the old behavior just moving the
> > condition/return back below the refname print; then add "buf" qualifier
> > to the following fragment and at each intermediate free.
>
> Thinking on it more, your behavior is at least as good as the old. And
> it only comes up in a broken repo, anyway, so trying to come up with
> some kind of useful outcome is pointless.
Sorry to reverse myself, but I just peeked at the show_reference
function one more time. Unconditionally moving the buffer-reading up
above the "if (!filter->lines)" conditional is not a good idea.
If I do "git tag -l", right now git doesn't have to actually read and
parse each object that has been tagged (lightweight or not). If I use
"git tag -n10", then obviously we do need to read it (and we do). And if
we use your new "--points-at", we also do. But if neither of those
options are in use, it would be nice to avoid the object lookup (it may
not seem like much, but if you have a repo with an insane number of
tags, it can add up).
> BTW, writing that helped me notice two bugs in your patch:
>
> 1. You read up to 47 bytes into the buffer without ever checking
> whether size >= 47.
>
> 2. You never check whether the object you read from read_sha1_file is
> actually a tag.
Hmm, the "filter->lines" code for "git tag -n" makes a similar error. It
should probably print nothing for objects that are not tags.
-Peff
^ permalink raw reply
* Re: Git performance results on a large repository
From: David Mohs @ 2012-02-06 7:10 UTC (permalink / raw)
To: git
In-Reply-To: <243C23AF01622E49BEA3F28617DBF0AD5912CA85@SC-MBX02-5.TheFacebook.com>
Joshua Redstone <joshua.redstone <at> fb.com> writes:
> To get a bit abstract for a moment, in an ideal world, it doesn't seem like
> performance constraints of a source-control-system should dictate how we
> choose to structure our code. Ideally, seems like we should be able to choose
> to structure our code in whatever way we feel maximizes developer
> productivity. If development and code/release management seem easier in a
> single repo, than why not make an SCM that can handle it? This is one reason
> I've been leaning towards figuring out an SCM approach that can work well with
> our current practices rather than changing them as a prerequisite for good SCM
> performance.
I certainly agree with this perspective---that our tools should support our
use cases and not the other way around. However, I'd like you to consider that
the size of this hypothetical repository might be giving you some useful
information on the health of the code it contains. You might consider creating
separate repositories simply to promote good modularization. It would involve
some up-front effort and certainly some pain, but this work itself might be
beneficial to your codebase without even considering the improved performance
of the version control system.
My concern here is that it may be extremely difficult to make a single piece
of software scale for a project that can grow arbitrarily large. You may add
some great performance improvements to git to then find that your bottleneck
is the filesystem. That would enlarge the scope of your work and would likely
make the project more difficult to manage.
If you are able to prove me wrong, the entire software community will benefit
from this work. However, before you embark upon a technical solution to your
problem, I would urge you to consider the possible benefits of a non-technical
solution, specifically restructuring your code and/or teams into more
independent modules. You might find benefits from this approach that extend
beyond source code control, which could make it the solution with the least
amount of overall risk.
Thanks for starting this valuable discussion.
-David
^ permalink raw reply
* Re: [PATCH 1/6] read-cache: use sha1file for sha1 calculation
From: Junio C Hamano @ 2012-02-06 7:34 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Thomas Rast, Joshua Redstone
In-Reply-To: <1328507319-24687-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Having no explanation on any of the patch in the series without cover
letter makes it hard to comment on anything, and not having any numbers
makes it even harder after guessing that this is about some performance
tweaks for 2M entry index cases.
This is open source, and I wouldn't stop you from spending time on anything
that interests you.
But having said that, if you have extra Git time, I would still rather see
you spend it first on tying up loose ends of your topics in flight and
on helping others that touch parts that are related to areas that you have
already thought about, namely:
(1) nd/commit-ignore-i-t-a, which I think should be marketted as fixing
an earlier UI mistake and presented with a clean migration path to
make the updated behaviour the default in the future; and
(2) the negative pathspec thing that resurfaced in disguise as Albert
Yale's "grep --exclude" series.
than playing with the approach of this series. The two reasons I suspect
that spending your time on this series will give us much less value than
the above two topics out of you are:
(1) While I think 2M-entry index is an interesting issue, it does not
affect most of the people; and more importantly
(2) I think the proper way to handle 2M-entry index case is to avoid
having to write and read the whole 2M-entry as a flat table in the
first place, not by weakening how its integrity is assured in order
to micro-tweak the read/write efficiency without re-examining the
flatness of the current in-core index [*1*].
The first patch that reuses the existing csum-file API to older code that
was written before csum-file was invented is probably a good thing to do,
though, independent of the 2M-entry issue.
Thanks.
[Footnote]
*1* A possible approach might be to stuff unmodified trees in the index
without exploding them into its components, and as entries are modified,
lazily expand these "tree" entries, while ensuring the "unmodified" parts
remain unmodified by turning the files in the working tree read-only and
requiring the user to say "git edit" or "git open" or something before
starting to edit. But as I said, I consider this not an ultra-urgent
issue, so I haven't thought things through yet.
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Junio C Hamano @ 2012-02-06 7:41 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120206062713.GA9699@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... But the point of
> duplicate suppression was that individual config files wouldn't have to
> know or care what was being included elsewhere.
I think you wanted to say "the point of inclusion mechanism" is that
individual configuration files would not have to know, and I think I
agree.
> So I'm actually thinking I should drop the duplicate suppression and
> just do some sort of sanity check on include-depth to break cycles
> (i.e., just die because it's a crazy thing to do, and we are really just
> trying to tell the user their config is broken rather than go into an
> infinite loop). As a bonus, it makes the code much simpler, too.
Yeah, I stand corrected. It was a bad suggestion.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox