* Re: [PATCH] Fix build problems related to profile-directed optimization
From: Jeff King @ 2012-02-06 4:18 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: git, Andi Kleen
In-Reply-To: <1328489090-14178-1-git-send-email-tytso@mit.edu>
On Sun, Feb 05, 2012 at 07:44:50PM -0500, Theodore Ts'o wrote:
> diff --git a/INSTALL b/INSTALL
> index 6fa83fe..5b7eec1 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
Eh? --prefix?
> +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//
> diff --git a/Makefile b/Makefile
> index c457c34..8cea247 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1772,6 +1772,24 @@ ifdef ASCIIDOC7
> [...]
> +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
Did you mean "$(PROFILE)" in the second conditional?
-Peff
^ permalink raw reply
* Re: [PATCH] branch --edit-description: protect against mistyped branch name
From: Jeff King @ 2012-02-06 4:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <7vaa4wda60.fsf_-_@alter.siamese.dyndns.org>
On Sun, Feb 05, 2012 at 05:26:31PM -0800, Junio C Hamano wrote:
> This incidentally also errors out --edit-description when the HEAD points
> at an unborn branch (immediately after "init", or "checkout --orphan"),
> because at that point, you do not even have any commit that is part of
> your history and there is no point in describing how this particular
> branch is different from the branch it forked off of, which is the useful
> bit of information the branch description is designed to capture.
>
> We may want to special case the unborn case later, but that is outside the
> scope of this patch to prevent more common mistakes before 1.7.9 series
> gains too much widespread use.
That sounds OK to me. I'm not even sure people will want to use
"--edit-description" on an unborn pointed-to branch or not (I mentioned
it only as "this is a plausible use case to me that we might be
breaking"). I think people will still be figuring out workflows around
it. So it's not a big deal to wait and see.
-Peff
^ permalink raw reply
* Re: Specifying revisions in the future
From: Miles Bader @ 2012-02-06 4:28 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Philip Oakley, Jakub Narebski, Matthieu Moy, jpaugh, git
In-Reply-To: <m2obtcx4i2.fsf@igel.home>
Andreas Schwab <schwab@linux-m68k.org> writes:
> The rule should be to follow the leftmost parent as far as possible.
> That means that X+2->D is B.
It might also be reasonable (and safer -- the user may not actually
realize when there's an ambiguating branch-point) to simply have it
abort with an error ("ambiguous future-ref specification") when
there's any doubt... I suspect most uses would be very simple "+1"
etc., and not crossing branch points.
-miles
--
`There are more things in heaven and earth, Horatio,
Than are dreamt of in your philosophy.'
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Jeff King @ 2012-02-06 4:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <7vobtcbtqa.fsf@alter.siamese.dyndns.org>
On Sun, Feb 05, 2012 at 06:06:53PM -0800, Junio C Hamano wrote:
> Subject: [PATCH] git checkout -b: allow switching out of an unborn branch
>
> Running "git checkout -b another" immediately after "git init" when you do
> not even have a commit on 'master' is forbidden, with a readable message:
>
> $ git checkout -b another
> fatal: You are on a branch yet to be born
>
> It is readable but not easily understandable unless the user knows what
> "yet to be born" really means.
>
> So let's try allowing it and see what happens. I strongly suspect that
> this may just shift the confusion one step further without adding much
> value to the resulting system, because the next question that would come
> to somebody who does not understand what "yet to be born" is is "why don't
> I see 'master' in the output from 'git branch' command?", and the new
> warning may not be descriptive enough to explain what the user is doing.
I thought the concern wasn't confusion at the error message, but rather
"how do I start a new repository with a branch named something besides
'master'?"
You would expect:
git init
git checkout -b foo
to work, but it doesn't. And there's no easy way to do what you want
(you have to resort to plumbing to put the value in HEAD). So the issue
is not a bad error message or a confusing situation, but that the user
wants to accomplish X, and we don't provide a reasonable way to do it.
I suspect it hasn't come up so far because the "X" in this case is not
something people generally want to do. I.e., they are almost always
cloning and making a new branch from old history. If they have a
brand-new repo, they almost certainly don't actually care what the
branch is called.
And perhaps in that case we should be discouraging them from calling it
something besides master (because while master is mostly convention,
there are a few magic spots in the code where it is treated specially,
and departing from the convention for no good reason should be
discouraged).
So I don't see "this is just shifting confusion" as a real argument. But
you could argue that it is enabling the user to do something stupid and
pointless, and for that reason it should be disallowed (and in that
case, it might be better for the error to say "what you are doing is
stupid and pointless").
-Peff
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Andrew Ardill @ 2012-02-06 4:42 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Michael Haggerty
In-Reply-To: <20120206043012.GD29365@sigill.intra.peff.net>
On 6 February 2012 15:30, Jeff King <peff@peff.net> wrote:
> And perhaps in that case we should be discouraging them from calling it
> something besides master (because while master is mostly convention,
> there are a few magic spots in the code where it is treated specially,
> and departing from the convention for no good reason should be
> discouraged).
What exactly are the areas where 'master' is treated specially? I
agree that people should be discouraged from needlessly abandoning
convention, however I think users should have the ability to name
their branches as they see fit.
If I am forced to abandon code targeted at the 'master' naming
convention in order to use my desired naming convention, we should fix
that. Additionally, if I have to either manually set a branch name
with plumbing commands, or delete existing branches that are generated
automatically with no option not to generate them, we should improve
the porcelain to cover these cracks.
In general, I think it plausible that in some use cases the term
'master' might be misleading or inappropriate and users should not be
punished for that.
Regards,
Andrew Ardill
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Jeff King @ 2012-02-06 5:06 UTC (permalink / raw)
To: Andrew Ardill; +Cc: Junio C Hamano, git, Michael Haggerty
In-Reply-To: <CAH5451ndjozo8-Cx3+Vc84TCjKJvCnSuOUsQS5cnqXsdc=8fMQ@mail.gmail.com>
On Mon, Feb 06, 2012 at 03:42:24PM +1100, Andrew Ardill wrote:
> On 6 February 2012 15:30, Jeff King <peff@peff.net> wrote:
>
> > And perhaps in that case we should be discouraging them from calling it
> > something besides master (because while master is mostly convention,
> > there are a few magic spots in the code where it is treated specially,
> > and departing from the convention for no good reason should be
> > discouraged).
>
> What exactly are the areas where 'master' is treated specially? I
> agree that people should be discouraged from needlessly abandoning
> convention, however I think users should have the ability to name
> their branches as they see fit.
Fairly minor stuff. Between the top of my head and a quick grep:
1. Some transports (like git://) are incapable of communicating the
destination of a remote's symbolic ref (they see only that HEAD
points to some specific sha1). But things like "clone" want to know
where the HEAD is pointing to set up the remotes/$origin/HEAD
link. We can guess that if HEAD and some branch have the same sha1,
that HEAD is pointing to that branch. But you might have two or
more such branches pointing to the same spot. In this case, we
prefer "master" over other branches.
This code is in guess_remote_head.
2. When merging, if the current branch is named "master", the default
merge message says "Merge branch foo". Otherwise, it says "Merge
branch foo into bar".
3. It looks like the antique "branches" file format defaults to
fetching "master" when no branch specifier is given. I doubt anyone
is still using this file format these days.
I was actually surprised how infrequently the term "master" comes up in
a grep of the code. So while I wouldn't call my search exhaustive, I did
inspect every match in the C code.
> If I am forced to abandon code targeted at the 'master' naming
> convention in order to use my desired naming convention, we should fix
> that. Additionally, if I have to either manually set a branch name
> with plumbing commands, or delete existing branches that are generated
> automatically with no option not to generate them, we should improve
> the porcelain to cover these cracks.
>
> In general, I think it plausible that in some use cases the term
> 'master' might be misleading or inappropriate and users should not be
> punished for that.
I kind of agree that we shouldn't be unnecessarily restrictive. On the
other hand, I am stretching to find the plausible reason that one would
want to throw away the normal convention. Code aside, it simply
introduces a slight communication barrier when talking with other git
users, and for that reason should be something you don't do lightly. I
don't recall seeing anybody complain seriously about it in the past six
years of git's existence.
-Peff
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Junio C Hamano @ 2012-02-06 5:15 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty
In-Reply-To: <20120206043012.GD29365@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I thought the concern wasn't confusion at the error message, but rather
> "how do I start a new repository with a branch named something besides
> 'master'?"
>
> You would expect:
>
> git init
> git checkout -b foo
>
> to work, but it doesn't. And there's no easy way to do what you want
> (you have to resort to plumbing to put the value in HEAD). So the issue
> is not a bad error message or a confusing situation, but that the user
> wants to accomplish X, and we don't provide a reasonable way to do it.
I think the right interface for "I want to use 'foo' instead of 'master'
like everybody else" would be:
$ git init --some-option foo
I wouldn't have any issue with that.
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Jeff King @ 2012-02-06 5:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <7vty34a6fd.fsf@alter.siamese.dyndns.org>
On Sun, Feb 05, 2012 at 09:15:34PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I thought the concern wasn't confusion at the error message, but rather
> > "how do I start a new repository with a branch named something besides
> > 'master'?"
> >
> > You would expect:
> >
> > git init
> > git checkout -b foo
> >
> > to work, but it doesn't. And there's no easy way to do what you want
> > (you have to resort to plumbing to put the value in HEAD). So the issue
> > is not a bad error message or a confusing situation, but that the user
> > wants to accomplish X, and we don't provide a reasonable way to do it.
>
> I think the right interface for "I want to use 'foo' instead of 'master'
> like everybody else" would be:
>
> $ git init --some-option foo
>
> I wouldn't have any issue with that.
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?
-Peff
^ permalink raw reply
* 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
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