* Re: fatal: git grep: cannot generate relative filenames containing '..'
From: George Spelvin @ 2009-01-18 16:58 UTC (permalink / raw)
To: gitster, azure; +Cc: linux, git
In-Reply-To: <83vdsefz9j.fsf@kalahari.s2.org>
Hannu Koivisto <azure@iki.fi> wrote:
> It turns out that this "entire git tree" is practically my only use
> case when using normal (find and) grep and when I first tried git
> grep, I actually expected it to do just that with no path specified.
>
> Why? Because of the way at least git diff and git log work. I
> thought that just like with them, "git grep foo" would search the
> entire git tree and I would have to say "git grep foo ." to limit to
> the current directory and its subdirectories. git-grep(1)'s "Look
> for specified patterns in the working tree files..." at least
> didn't seem to disagree with my expectation so I was a bit puzzled.
>
> So I'd rather see git grep behave in a way consistent with git log
> and git diff (I realize that would change current behaviour instead
> of extending it).
D'oh. You're completely right. Space-dot is trivial to type if you
want the current directory only, and that is more consistent.
Could that be considered for 1.7?
^ permalink raw reply
* Re: [PATCH/RFC] git-am: Make it easier to see which patch failed
From: Jeff King @ 2009-01-18 17:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stephan Beyer, Junio C Hamano, Jonas Flodén, git
In-Reply-To: <alpine.DEB.1.00.0901181711090.3586@pacific.mpi-cbg.de>
On Sun, Jan 18, 2009 at 05:17:43PM +0100, Johannes Schindelin wrote:
> > Yes, I'm surprised Junio doesn't remember the mass conversions we
> > already had to do (4b7cc26a and 293623ed). But looking at the date, I
> > guess it _has_ been a year and a half. :)
>
> Hey, be nice to Junio. Have you seen the amount of mails on this list
> recently? I think Junio's the only one really reading all of them; even
> if you were right, he would be entitled to a nicer reminder.
I didn't mean to be mean. On the contrary, I was surprised because _he_
usually is the one reminding _me_ about such fixes. I guess Junio is
human, after all. :)
> But you are wrong. And Stephan is wrong, too.
>
> The name "FIRSTLINE" suggests that it is indeed a first line, and
> consequently cannot contain a newline.
It is not "this is a problem because it might contain a newline" but "this
is a problem because it might contain an escape sequence, _an example_
of which is a \n newline." So the question is whether you can guarantee
that $FIRSTLINE does not contain a backslash. Which I don't think is the
case here.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] color: make it easier for non-config to parse color specs
From: René Scharfe @ 2009-01-18 17:10 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <20090117153229.GA27071@coredump.intra.peff.net>
Jeff King schrieb:
> We have very featureful color-parsing routines which are
> used for color.diff.* and other options. Let's make it
> easier to use those routines from other parts of the code.
>
> This patch adds a color_parse_mem() helper function which
> takes a length-bounded string instead of a NUL-terminated
> one. While the helper is only a few lines long, it is nice
> to abstract this out so that:
>
> - callers don't forget to free() the temporary buffer
>
> - right now, it is implemented in terms of color_parse().
> But it would be more efficient to reverse this and
> implement color_parse in terms of color_parse_mem.
Thusly?
diff --git a/color.c b/color.c
index fc0b72a..14eac93 100644
--- a/color.c
+++ b/color.c
@@ -41,6 +41,11 @@ static int parse_attr(const char *name, int len)
void color_parse(const char *value, const char *var, char *dst)
{
+ color_parse_mem(value, strlen(value), var, dst);
+}
+
+void color_parse_mem(const char *value, int len, const char *var, char *dst)
+{
const char *ptr = value;
int attr = -1;
int fg = -2;
@@ -52,18 +57,22 @@ void color_parse(const char *value, const char *var, char *dst)
}
/* [fg [bg]] [attr] */
- while (*ptr) {
+ while (len > 0) {
const char *word = ptr;
- int val, len = 0;
+ int val, wordlen = 0;
- while (word[len] && !isspace(word[len]))
- len++;
+ while (len > 0 && !isspace(word[wordlen])) {
+ wordlen++;
+ len--;
+ }
- ptr = word + len;
- while (*ptr && isspace(*ptr))
+ ptr = word + wordlen;
+ while (len > 0 && isspace(*ptr)) {
ptr++;
+ len--;
+ }
- val = parse_color(word, len);
+ val = parse_color(word, wordlen);
if (val >= -1) {
if (fg == -2) {
fg = val;
@@ -75,7 +84,7 @@ void color_parse(const char *value, const char *var, char *dst)
}
goto bad;
}
- val = parse_attr(word, len);
+ val = parse_attr(word, wordlen);
if (val < 0 || attr != -1)
goto bad;
attr = val;
^ permalink raw reply related
* Re: [PATCH 2/2] expand --pretty=format color options
From: René Scharfe @ 2009-01-18 17:13 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <20090117153846.GB27071@coredump.intra.peff.net>
Jeff King schrieb:
> Currently, the only colors available to --pretty=format
> users are red, green, and blue. Rather than expand it with a
> few new colors, this patch makes the usual config color
> syntax available, including more colors, backgrounds, and
> attributes.
>
> Because colors are no longer bounded to a single word (e.g.,
> %Cred), this uses a more advanced syntax that features a
> beginning and end delimiter (but the old syntax still
> works). So you can now do:
>
> git log --pretty=tformat:'%C(yellow)%h%C(reset) %s'
>
> to emulate --pretty=oneline, or even
>
> git log --pretty=tformat:'%C(cyan magenta bold)%s%C(reset)'
>
> if you want to relive the awesomeness of 4-color CGA.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> René: I saw you mention an end goal of implementing the other --pretty
> formats in terms of --pretty=format:. So maybe this will help just a
> little.
Nice!
Another step would be for --pretty=format to respect the color config,
i.e. it shouldn't print any colour codes if colouring is turned off or
if set to auto while writing to file or pipe.
René
^ permalink raw reply
* [PATCH] builtin-fsck: fix off by one head count
From: Christian Couder @ 2009-01-18 17:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
According to the man page, if "git fsck" is passed one or more
heads, it should verify connectivity and validity of only objects
reachable from the heads it is passed.
But before this patch, when it was passed only one head, then it
behaved as if no heads were passed (that is it used the index file,
all references in $GIT_DIR/refs/* and all reflogs). In fact it just
ignored the first head it was passed.
It looks like this bug was introduced when "builtin-fsck.c" was
changed to use "parse_options" (commit
5ac0a2063e8f824f6e8ffb4d18de74c55aae7131).
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-fsck.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
Sorry if you receive this patch more than once, but it
seems that something did not work when I tried to send
it to the mailing list before.
diff --git a/builtin-fsck.c b/builtin-fsck.c
index 5a08ec1..fb35554 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -629,7 +629,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
}
heads = 0;
- for (i = 1; i < argc; i++) {
+ for (i = 0; i < argc; i++) {
const char *arg = argv[i];
if (!get_sha1(arg, head_sha1)) {
struct object *obj = lookup_object(head_sha1);
--
1.6.1.85.g8ffc
^ permalink raw reply related
* Re: [PATCH 1/2] color: make it easier for non-config to parse color specs
From: Jeff King @ 2009-01-18 17:28 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <4973628C.8080501@lsrfire.ath.cx>
On Sun, Jan 18, 2009 at 06:10:36PM +0100, René Scharfe wrote:
> > - right now, it is implemented in terms of color_parse().
> > But it would be more efficient to reverse this and
> > implement color_parse in terms of color_parse_mem.
>
> Thusly?
Yes, except for the bugs you introduced. :)
> +void color_parse_mem(const char *value, int len, const char *var, char *dst)
> +{
> const char *ptr = value;
> int attr = -1;
> int fg = -2;
What's missing in the context here (because it wasn't changed) is:
> if (!strcasecmp(value, "reset")) {
> strcpy(dst, "\033[m");
> return;
> }
which doesn't work, since our string is actually something like
"reset)\0" or even "reset)some totally unrelated string". So we would
need a "memcasecmp" here.
And then in the error case, we call:
> die("bad color value '%s' for variable '%s'", value, var);
which is also bogus.
I don't know if this is really even worth it. The timing difference is
pretty minimal:
$ time ./git log --pretty=tformat:'%Credfoo%Creset' >/dev/null
real 0m0.673s
user 0m0.652s
sys 0m0.016s
$ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
real 0m0.692s
user 0m0.660s
sys 0m0.032s
That's about 1 microsecond per commit.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] color: make it easier for non-config to parse color specs
From: Jeff King @ 2009-01-18 17:36 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <20090118172802.GA17434@coredump.intra.peff.net>
On Sun, Jan 18, 2009 at 12:28:02PM -0500, Jeff King wrote:
> I don't know if this is really even worth it. The timing difference is
> pretty minimal:
BTW, if we really care about every inch of performance in
--pretty=format:, it would probably make sense to pre-parse the string
into a mini-bytecode. So any complex parsing or lookup that is not
dependent on the commit itself can be done once, instead of per-commit.
I don't think it would make a huge performance difference now, but there
has been talk of more complex substitution syntax (and this %C() is an
example), which would probably benefit.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] expand --pretty=format color options
From: Jeff King @ 2009-01-18 17:37 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <49736331.8010003@lsrfire.ath.cx>
On Sun, Jan 18, 2009 at 06:13:21PM +0100, René Scharfe wrote:
> Another step would be for --pretty=format to respect the color config,
> i.e. it shouldn't print any colour codes if colouring is turned off or
> if set to auto while writing to file or pipe.
That makes sense to me. In theory, we could offer an "always use this
color" and a "conditionally use this color" substitution. But I don't
really see why anyone would want the "always use this color" one (they
could just say --color, then).
-Peff
^ permalink raw reply
* Re: [PATCH/RFC] git-am: Make it easier to see which patch failed
From: Johannes Schindelin @ 2009-01-18 17:44 UTC (permalink / raw)
To: Jeff King; +Cc: Stephan Beyer, Junio C Hamano, Jonas Flodén, git
In-Reply-To: <20090118170711.GA17055@coredump.intra.peff.net>
Hi.
On Sun, 18 Jan 2009, Jeff King wrote:
> On Sun, Jan 18, 2009 at 05:17:43PM +0100, Johannes Schindelin wrote:
>
> > The name "FIRSTLINE" suggests that it is indeed a first line, and
> > consequently cannot contain a newline.
>
> It is not "this is a problem because it might contain a newline" but
> "this is a problem because it might contain an escape sequence, _an
> example_ of which is a \n newline." So the question is whether you can
> guarantee that $FIRSTLINE does not contain a backslash. Which I don't
> think is the case here.
Oh. Okay, so I was wrong. But only because dash's echo behaves in a
strange way: it makes "-e" a noop?
Ciao,
Dscho
^ permalink raw reply
* Re: meaning of --8<--
From: Jeff King @ 2009-01-18 17:44 UTC (permalink / raw)
To: Markus Heidelberg; +Cc: git
In-Reply-To: <200901181656.37813.markus.heidelberg@web.de>
On Sun, Jan 18, 2009 at 04:56:37PM +0100, Markus Heidelberg wrote:
> I've seen lines like "--8<--" several times on this list, but have no
> clue what it is about. OK, seems like it's used to insert diffs in the
> middle of a mail message.
> But is this a common convention or git specific and handled by git-am?
> Is it documented anywhere?
As others have explained, it is supposed to be scissors, and it is not
handled by git-am. But the real point of it is to help the person
applying (e.g., Junio) manually separate cover letter from the commit
message. So one way of sending cover material is putting it after the
"---" and before the diff:
Subject: [PATCH] first line of commit message
more commit message
---
Here is cover letter material that doesn't go in the commit message.
It is stripped automatically by git-am.
diff --git ...
but that is often unnatural to read, because the cover letter material
often introduces the commit, especially when you are replying in the
middle of a thread. So then you end up with:
Subject: Re: whatever thread you're in
Somebody else said:
> blah blah blah
I disagree. You should do it like this instead:
-- >8 --
first line of commit message
more commit message
---
diff --git ...
git-am will put everything down to the "---" into the commit message,
but it is simple enough to amend away everything else.
So in that sense, it doesn't really matter _what_ the separator is, but
it should be visibly obvious to a human so that they can fix up the
commit message. The only exception is that it should _not_ be "---",
because that is treated specially by git-am.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] color: make it easier for non-config to parse color specs
From: René Scharfe @ 2009-01-18 17:45 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <20090118172802.GA17434@coredump.intra.peff.net>
Jeff King schrieb:
> On Sun, Jan 18, 2009 at 06:10:36PM +0100, René Scharfe wrote:
>
>>> - right now, it is implemented in terms of color_parse().
>>> But it would be more efficient to reverse this and
>>> implement color_parse in terms of color_parse_mem.
>> Thusly?
>
> Yes, except for the bugs you introduced. :)
Eeek! :)
>> +void color_parse_mem(const char *value, int len, const char *var, char *dst)
>> +{
>> const char *ptr = value;
>> int attr = -1;
>> int fg = -2;
>
> What's missing in the context here (because it wasn't changed) is:
>
>> if (!strcasecmp(value, "reset")) {
>> strcpy(dst, "\033[m");
>> return;
>> }
>
> which doesn't work, since our string is actually something like
> "reset)\0" or even "reset)some totally unrelated string". So we would
> need a "memcasecmp" here.
if (!strncasecmp(value, "reset", len)) {
> And then in the error case, we call:
>
>> die("bad color value '%s' for variable '%s'", value, var);
>
> which is also bogus.
die("bad color value '%.*s' for variable '%s', len, value, var);
> I don't know if this is really even worth it. The timing difference is
> pretty minimal:
>
> $ time ./git log --pretty=tformat:'%Credfoo%Creset' >/dev/null
> real 0m0.673s
> user 0m0.652s
> sys 0m0.016s
> $ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
> real 0m0.692s
> user 0m0.660s
> sys 0m0.032s
>
> That's about 1 microsecond per commit.
Hmm, not too much overhead, agreed, but it would still be nice to avoid it.
René
^ permalink raw reply
* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules
From: Lars Hjemli @ 2009-01-18 17:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0901181635290.3586@pacific.mpi-cbg.de>
On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>
>> diff --git a/environment.c b/environment.c
>> @@ -159,3 +161,13 @@ int set_git_dir(const char *path)
>> setup_git_env();
>> return 0;
>> }
>> +
>> +int get_traverse_gitlinks()
>> +{
>> + return traverse_gitlinks;
>> +}
>> +
>> +void set_traverse_gitlinks(int traverse)
>> +{
>> + traverse_gitlinks = traverse;
>> +}
>
> If you have full accessors anyway, it is much easier and cleaner to make
> this a global variable to begin with.
>
> However, environment.c is reserved for things that come from the config
> and can be overridden by the user. That is certainly not the case for
> traverse_gitlinks.
>
> But let's think about it again: should traverse_gitlinks be a global
> varible at all? I think not. It should be a per-call decision.
Yes, it might be a cleaner solution to add an extra parameter to
read_tree_recursive(), and since it currently has only 7 callsites the
patch wouldn't be too big either. Junio, what do you think?
>> +/* Try to add the objectdb of a submodule */
>> +int add_gitlink_odb(char *relpath)
>
> This wants to be static.
Thanks
>
>> +{
>> + const char *odbpath;
>> + struct stat st;
>> +
>> + odbpath = read_gitfile_gently(mkpath("%s/.git", relpath));
>> + if (!odbpath)
>> + odbpath = mkpath("%s/.git/objects", relpath);
>> +
>> + if (stat(odbpath, &st))
>> + return 1;
>> +
>> + return add_alt_odb(odbpath);
>> +}
>> +
>> +/* Check if we should recurse into the specified submodule */
>> +int traverse_gitlink(char *path, const unsigned char *commit_sha1,
>
> This, too.
Thanks again ;-)
>
>> + struct tree **subtree)
>> +{
>> + unsigned char sha1[20];
>> + int linked_odb = 0;
>> + struct commit *commit;
>> + void *buffer;
>> + enum object_type type;
>> + unsigned long size;
>> +
>> + hashcpy(sha1, commit_sha1);
>> + if (!add_gitlink_odb(path)) {
>> + linked_odb = 1;
>> + if (resolve_gitlink_ref(path, "HEAD", sha1))
>> + die("Unable to lookup HEAD in %s", path);
>> + }
>
> Why would you want to continue if add_gitlink_odb() did not find a checked
> out submodule?
>
> Seems you want to fall back to look in the superproject's object database.
> But I think that is wrong, as I have a superproject with many platform
> dependent submodules, only one of which is checked out, and for
> convenience, the submodules all live in the superproject's repository.
Actually, I want this to work for bare repositories by specifying the
submodule odbs in the alternates file. So if the current submodule odb
wasn't found my plan was to check if the commit object was accessible
anyways but don't die() if it wasn't.
>> + commit = lookup_commit(sha1);
>> + if (!commit)
>> + die("traverse_gitlink(): internal error");
>
> s/internal error/could not access commit '%s' of submodule '%s'",
> sha1_to_hex(sha1), path);/
Ok (I belive this codepath is virtually impossible to hit, hence the
"internal error", but I could of course be mistaken).
>> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
>> return -1;
>> continue;
>> }
>> + if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
>
> Like I said, traverse_gitlinks should be a flag to read_tree_recursive.
> So preferably, you should add a parameter 'flags' and make that option an
> enum.
>
>> + int retval;
>> + char *newbase;
>> + struct tree *subtree;
>> + unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
>
> Nit: Long line.
Will fix
>
>> +
>> + newbase = xmalloc(baselen + 1 + pathlen);
>> + memcpy(newbase, base, baselen);
>> + memcpy(newbase + baselen, entry.path, pathlen);
>> + newbase[baselen + pathlen] = 0;
>
> We have strbufs for that.
>
>> + if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
>> + free(newbase);
>> + continue;
>> + }
>> + newbase[baselen + pathlen] = '/';
>
> ... to avoid this off-by-one.
Actually, I don't think this is off-by-one, since (baselen + pathlen +
1) is passed along to read_tree_recursive() as the new baselen. But
using strbufs might be cleaner anyways.
Thanks for the review.
--
larsh
^ permalink raw reply
* Re: [PATCH/RFC] git-am: Make it easier to see which patch failed
From: Jeff King @ 2009-01-18 17:49 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stephan Beyer, Junio C Hamano, Jonas Flodén, git
In-Reply-To: <alpine.DEB.1.00.0901181843230.3586@pacific.mpi-cbg.de>
On Sun, Jan 18, 2009 at 06:44:01PM +0100, Johannes Schindelin wrote:
> Oh. Okay, so I was wrong. But only because dash's echo behaves in a
> strange way: it makes "-e" a noop?
Right. "-e" isn't in POSIX at all, and it is a SysV-ism to allow escapes
in any argument (I don't know if "-e" was introduced by GNU people, or
came from elsewhere).
See here for the gory details:
http://www.opengroup.org/onlinepubs/009695399/utilities/echo.html
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] color: make it easier for non-config to parse color specs
From: Jeff King @ 2009-01-18 18:06 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <49736A9C.1040601@lsrfire.ath.cx>
On Sun, Jan 18, 2009 at 06:45:00PM +0100, René Scharfe wrote:
> > need a "memcasecmp" here.
>
> if (!strncasecmp(value, "reset", len)) {
Thanks. I knew there must be some stock function to do this, but for
some reason I just could not think of strncasecmp.
> die("bad color value '%.*s' for variable '%s', len, value, var);
Except that we've been shortening "len" during the course of the
function, so it is generally 0 at this point.
> > $ time ./git log --pretty=tformat:'%Credfoo%Creset' >/dev/null
> > real 0m0.673s
> > user 0m0.652s
> > sys 0m0.016s
> > $ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
> > real 0m0.692s
> > user 0m0.660s
> > sys 0m0.032s
> >
> > That's about 1 microsecond per commit.
>
> Hmm, not too much overhead, agreed, but it would still be nice to avoid it.
Here are the numbers with your fixes:
$ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
real 0m0.677s
user 0m0.668s
sys 0m0.008s
which puts the difference well into the noise region (I actually did get
one run with your patch that was just as fast as the original).
Here is an updated version of patch 1/2, squashing in your
color_mem_parse update, your follow-on fix, and a fix for the die().
-- >8 --
color: make it easier for non-config to parse color specs
We have very featureful color-parsing routines which are
used for color.diff.* and other options. Let's make it
easier to use those routines from other parts of the code.
This patch converts color_parse to color_parse_mem, taking a
length-bounded string instead of a NUL-terminated one. We
keep color_parse as a wrapper which takes a normal string.
Thanks to René Scharfe for rewriting the color_parse
implementation.
This also changes the error string for an invalid color not
to mention the word "config", since it is not always
appropriate (and when it is, the context is obvious since
the offending config variable is given).
Finally, while we are in the area, we clean up the parameter
names in the declaration of color_parse; the var and value
parameters were reversed from the actual implementation.
---
color.c | 31 +++++++++++++++++++++----------
color.h | 3 ++-
2 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/color.c b/color.c
index fc0b72a..915d7a9 100644
--- a/color.c
+++ b/color.c
@@ -41,29 +41,40 @@ static int parse_attr(const char *name, int len)
void color_parse(const char *value, const char *var, char *dst)
{
+ color_parse_mem(value, strlen(value), var, dst);
+}
+
+void color_parse_mem(const char *value, int value_len, const char *var,
+ char *dst)
+{
const char *ptr = value;
+ int len = value_len;
int attr = -1;
int fg = -2;
int bg = -2;
- if (!strcasecmp(value, "reset")) {
+ if (!strncasecmp(value, "reset", len)) {
strcpy(dst, "\033[m");
return;
}
/* [fg [bg]] [attr] */
- while (*ptr) {
+ while (len > 0) {
const char *word = ptr;
- int val, len = 0;
+ int val, wordlen = 0;
- while (word[len] && !isspace(word[len]))
- len++;
+ while (len > 0 && !isspace(word[wordlen])) {
+ wordlen++;
+ len--;
+ }
- ptr = word + len;
- while (*ptr && isspace(*ptr))
+ ptr = word + wordlen;
+ while (len > 0 && isspace(*ptr)) {
ptr++;
+ len--;
+ }
- val = parse_color(word, len);
+ val = parse_color(word, wordlen);
if (val >= -1) {
if (fg == -2) {
fg = val;
@@ -75,7 +86,7 @@ void color_parse(const char *value, const char *var, char *dst)
}
goto bad;
}
- val = parse_attr(word, len);
+ val = parse_attr(word, wordlen);
if (val < 0 || attr != -1)
goto bad;
attr = val;
@@ -115,7 +126,7 @@ void color_parse(const char *value, const char *var, char *dst)
*dst = 0;
return;
bad:
- die("bad config value '%s' for variable '%s'", value, var);
+ die("bad color value '%.*s' for variable '%s'", value_len, value, var);
}
int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
diff --git a/color.h b/color.h
index 6cf5c88..7066099 100644
--- a/color.h
+++ b/color.h
@@ -16,7 +16,8 @@ extern int git_use_color_default;
int git_color_default_config(const char *var, const char *value, void *cb);
int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
-void color_parse(const char *var, const char *value, char *dst);
+void color_parse(const char *value, const char *var, char *dst);
+void color_parse_mem(const char *value, int len, const char *var, char *dst);
int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
--
1.6.1.266.g3b9d0.dirty
^ permalink raw reply related
* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules
From: Johannes Schindelin @ 2009-01-18 18:33 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <8c5c35580901180945u17a69140vff2736765ee6073@mail.gmail.com>
Hi,
On Sun, 18 Jan 2009, Lars Hjemli wrote:
> On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Sun, 18 Jan 2009, Lars Hjemli wrote:
> >
> >> + struct tree **subtree)
> >> +{
> >> + unsigned char sha1[20];
> >> + int linked_odb = 0;
> >> + struct commit *commit;
> >> + void *buffer;
> >> + enum object_type type;
> >> + unsigned long size;
> >> +
> >> + hashcpy(sha1, commit_sha1);
> >> + if (!add_gitlink_odb(path)) {
> >> + linked_odb = 1;
> >> + if (resolve_gitlink_ref(path, "HEAD", sha1))
> >> + die("Unable to lookup HEAD in %s", path);
> >> + }
> >
> > Why would you want to continue if add_gitlink_odb() did not find a checked
> > out submodule?
> >
> > Seems you want to fall back to look in the superproject's object database.
> > But I think that is wrong, as I have a superproject with many platform
> > dependent submodules, only one of which is checked out, and for
> > convenience, the submodules all live in the superproject's repository.
>
> Actually, I want this to work for bare repositories by specifying the
> submodule odbs in the alternates file. So if the current submodule odb
> wasn't found my plan was to check if the commit object was accessible
> anyways but don't die() if it wasn't.
Please make that an explicit option (cannot think of a good name, though),
otherwise I will not be able to use your feature. Making it the default
would be inconsistent with the rest of our submodules framework.
> >> + commit = lookup_commit(sha1);
> >> + if (!commit)
> >> + die("traverse_gitlink(): internal error");
> >
> > s/internal error/could not access commit '%s' of submodule '%s'",
> > sha1_to_hex(sha1), path);/
>
> Ok (I belive this codepath is virtually impossible to hit, hence the
> "internal error", but I could of course be mistaken).
You make it a function that is exported to other parts of Git in cache.h.
So you might just as well expect it to be used by other parts at some
stage.
Ciao,
Dscho
^ permalink raw reply
* John (zzz) Doe <john.doe@xz> (Comment)
From: Junio C Hamano @ 2009-01-18 18:50 UTC (permalink / raw)
To: Kirill Smelkov; +Cc: Johannes Schindelin, git
In-Reply-To: <20090118145429.GA27522@roro3.zxlink>
Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> On Fri, Jan 16, 2009 at 12:54:28PM +0100, Johannes Schindelin wrote:
> ...
>> > From: A U Thor (MonikeR) <a.u@thor.xz>
>>
>> It is Philippe Bruhat (BooK), who sometimes forgets the closing
>> parenthesis, and who is listed in .mailmap without the moniker.
It was not "forgets", but is an artifact that older mailinfo removed
parenthesis incorrectly (see below).
> So now I don't understand what to do.
>
> From one hand RFC822 says '(...)' is a comment, and from the other hand,
> we have a use case where one guy wants this to stay.
> ...
> commit 49bebfbe18dac296e5e246884bd98c1f90be9676
> Author: Kirill Smelkov <kirr@landau.phys.spbu.ru>
> Date: Tue Jan 13 01:21:04 2009 +0300
>
> mailinfo: more smarter removal of rfc822 comments from 'From'
>
> As described in RFC822 (3.4.3 COMMENTS, and A.1.4.), comments, as e.g.
>
> John (zzz) Doe <john.doe@xz> (Comment)
>
> should "NOT [be] included in the destination mailbox"
The above quote from the RFC is irrelevant. Note that it is only about
how you extract the e-mail address, discarding everything else.
What mailinfo wants to do is to separate the human-readable name and the
e-mail address, and we want to use _both_ results from it.
We separate a few example From: lines like this:
Kirill Smelkov <kirr@smelkov.xz>
==> AUTHOR_EMAIL="kirr@smelkov.xz" AUTHOR_NAME="Kirill Smelkov"
kirr@smelkov.xz (Kirill Smelkov)
==> AUTHOR_EMAIL="kirr@smelkov.xz" AUTHOR_NAME="Kirill Smelkov"
Traditionally, the way people spelled their name on From: line has been
either one of the above form. Typically comment form (i.e. the second
one) adds the name at the end, while "Name <addr>" form has the name at
the front. But I do not think RFC requires that, primarily because it is
all about discarding non-address part to find the e-mail address aka
"destination mailbox". It does not specify how humans should interpret
the human readable name and the comment.
Now, why is the name not AUTHOR_NAME="(Kirill Smelkov)" in the latter
form?
It is just common sense transformation. Otherwise it looks simply ugly,
and it is obvious that the parentheses is not part of the name of the
person who used "kirr@smelkov.xz (Kirill Smelkov)" on his From: line.
So we can separate "John (zzz) Doe <john.doe@xz> (Comment)" into:
AUTHOR_EMAIL=john.doe@xz
AUTHOR_NAME="John (zzz) Doe (Comment)"
and leave it like so, I think.
^ permalink raw reply
* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules
From: Junio C Hamano @ 2009-01-18 19:00 UTC (permalink / raw)
To: Lars Hjemli; +Cc: René Scharfe, git
In-Reply-To: <8c5c35580901180837i6e835d98ob8875ce1b8ad3011@mail.gmail.com>
Lars Hjemli <hjemli@gmail.com> writes:
> I like the idea, but it will require thorough review of all
> read_tree_recursive() consumers. So now we've got three different
> approaches:
> * me: global setting
> * dscho: parameter to read_tree_recursive()
> * you: accept the return value from the callback function
>
> Junio, what would you prefer?
As usual René has the best taste in designing things ;-)
^ permalink raw reply
* Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()
From: Johannes Schindelin @ 2009-01-18 19:06 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <1232265877-3649-3-git-send-email-mh@glandium.org>
Hi,
On Sun, 18 Jan 2009, Mike Hommey wrote:
> diff --git a/transport.c b/transport.c
> index 56831c5..6919ff1 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> free(ref);
> }
>
> + http_cleanup();
> + free(refs_url);
> return refs;
> }
You cannot http_cleanup() here, as http-push calls that function, but
continues to want to use curl.
Ciao,
Dscho
^ permalink raw reply
* Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()
From: Johannes Schindelin @ 2009-01-18 19:11 UTC (permalink / raw)
To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0901182005230.3586@pacific.mpi-cbg.de>
Hi,
On Sun, 18 Jan 2009, Johannes Schindelin wrote:
> On Sun, 18 Jan 2009, Mike Hommey wrote:
>
> > diff --git a/transport.c b/transport.c
> > index 56831c5..6919ff1 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> > free(ref);
> > }
> >
> > + http_cleanup();
> > + free(refs_url);
> > return refs;
> > }
>
> You cannot http_cleanup() here, as http-push calls that function, but
> continues to want to use curl.
Worse, a http clone will hit the same issue.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 3/7 v2] git_extract_argv0_path(): Move check for valid argv0 from caller to callee
From: Johannes Sixt @ 2009-01-18 19:16 UTC (permalink / raw)
To: Steffen Prohaska; +Cc: Junio C Hamano, git, Johannes Schindelin
In-Reply-To: <1232280015-8144-4-git-send-email-prohaska@zib.de>
On Sonntag, 18. Januar 2009, Steffen Prohaska wrote:
> This simplifies the calling code.
But it could really be squashed into the previous patch, after fixing...
> @@ -23,6 +23,9 @@ const char *system_path(const char *path)
>
> const char *git_extract_argv0_path(const char *argv0)
> {
> + if (!argv0 || !*argv0)
> + return 0;
> +
> const char *slash = argv0 + strlen(argv0);
... this declaration after statement.
-- Hannes
^ permalink raw reply
* Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()
From: Mike Hommey @ 2009-01-18 19:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0901182005230.3586@pacific.mpi-cbg.de>
On Sun, Jan 18, 2009 at 08:06:17PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Mike Hommey wrote:
>
> > diff --git a/transport.c b/transport.c
> > index 56831c5..6919ff1 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> > free(ref);
> > }
> >
> > + http_cleanup();
> > + free(refs_url);
> > return refs;
> > }
>
> You cannot http_cleanup() here, as http-push calls that function, but
> continues to want to use curl.
Are you really sure? It doesn't seem so, to me.
Mike
^ permalink raw reply
* Re: [WIP Patch 04/12] Use the new http API in http_fetch_ref()
From: Mike Hommey @ 2009-01-18 19:21 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0901181607210.3586@pacific.mpi-cbg.de>
On Sun, Jan 18, 2009 at 04:10:38PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Mike Hommey wrote:
>
> > diff --git a/http.c b/http.c
> > index 82534cf..0c9504b 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -604,34 +604,17 @@ int http_fetch_ref(const char *base, struct ref *ref)
> > {
> > char *url;
> > struct strbuf buffer = STRBUF_INIT;
> > - struct active_request_slot *slot;
> > - struct slot_results results;
> > - int ret;
> > + int ret = -1;
> >
> > url = quote_ref_url(base, ref->name);
> > - slot = get_active_slot();
> > - slot->results = &results;
> > - curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
> > - curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> > - curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
> > - curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> > - if (start_active_slot(slot)) {
> > - run_active_slot(slot);
> > - if (results.curl_result == CURLE_OK) {
> > - strbuf_rtrim(&buffer);
> > - if (buffer.len == 40)
> > - ret = get_sha1_hex(buffer.buf, ref->old_sha1);
> > - else if (!prefixcmp(buffer.buf, "ref: ")) {
> > - ref->symref = xstrdup(buffer.buf + 5);
> > - ret = 0;
> > - } else
> > - ret = 1;
> > - } else {
> > - ret = error("Couldn't get %s for %s\n%s",
> > - url, ref->name, curl_errorstr);
> > + if (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
> > + strbuf_rtrim(&buffer);
> > + if (buffer.len == 40)
> > + ret = get_sha1_hex(buffer.buf, ref->old_sha1);
> > + else if (!prefixcmp(buffer.buf, "ref: ")) {
> > + ref->symref = xstrdup(buffer.buf + 5);
> > + ret = 0;
> > }
> > - } else {
> > - ret = error("Unable to start request");
> > }
>
> Why not keep that error?
It should be handled in http_request, I'd say...
Mike
^ permalink raw reply
* Re: [WIP Patch 08/12] Use the new http API in update_remote_info_refs()
From: Mike Hommey @ 2009-01-18 19:23 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0901181615070.3586@pacific.mpi-cbg.de>
On Sun, Jan 18, 2009 at 04:18:16PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Mike Hommey wrote:
>
> >
> > Signed-off-by: Mike Hommey <mh@glandium.org>
> > ---
> > http-push.c | 29 ++++++++++-------------------
> > 1 files changed, 10 insertions(+), 19 deletions(-)
> >
> > diff --git a/http-push.c b/http-push.c
> > index e0b4f5a..7627860 100644
> > --- a/http-push.c
> > +++ b/http-push.c
> > @@ -1960,29 +1960,20 @@ static void update_remote_info_refs(struct remote_lock *lock)
> > static int remote_exists(const char *path)
> > {
>
> Heh, I see where your commit subject comes from, but it should rather
> mention the function "remote_exists()"...
>
> > char *url = xmalloc(strlen(remote->url) + strlen(path) + 1);
> > - struct active_request_slot *slot;
> > - struct slot_results results;
> > - int ret = -1;
> > + int ret;
> >
> > sprintf(url, "%s%s", remote->url, path);
> >
> > - slot = get_active_slot();
> > - slot->results = &results;
> > - curl_easy_setopt(slot->curl, CURLOPT_URL, url);
> > - curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
> > -
> > - if (start_active_slot(slot)) {
> > - run_active_slot(slot);
> > - if (results.http_code == 404)
> > - ret = 0;
> > - else if (results.curl_result == CURLE_OK)
> > - ret = 1;
> > - else
> > - fprintf(stderr, "HEAD HTTP error %ld\n", results.http_code);
> > - } else {
> > - fprintf(stderr, "Unable to start HEAD request\n");
> > + switch (http_get_strbuf(url, NULL, 0)) {
> > + case HTTP_OK:
> > + ret = 1;
> > + break;
> > + case HTTP_MISSING_TARGET:
> > + ret = 0;
> > + break;
> > + default:
> > + ret = -1;
> > }
>
> Does http_get_strbuf() already show the error? Not as far as I can see,
> even if it would make sense, no? At least you'll have to "return
> error(...)".
As I said, it has some error handling regressions ;)
Mike
^ permalink raw reply
* Re: [PATCH/RFC v1 1/1] +5 cases (4 fail), diff whitespace tests
From: Keith Cascio @ 2009-01-18 19:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd4elys21.fsf@gitster.siamese.dyndns.org>
On Sat, 17 Jan 2009, Junio C Hamano wrote:
> Hmm. Are these three supposed to be orthogonal?
The semantics of those 3 flags are not orthogonal, no. Their relationship
amongst each other is one of transitive implication:
-w implies the other two
-b implies --ignore-space-at-eol
--ignore-space-at-eol implies only itself
Therefore, it is never *necessary* to specify more than one of these flags on
the command line. However, it is not hard to imagine scenarios where software
wrappers around git (e.g. GUIs, etc), generate command lines with more than one
of these flags. I thought about it, and it seems unreasonable to make it an
error to specify more than one, since a new user might not immediately grasp the
way they imply each other. I think Git could and should treat it as a legal
case. I contacted Dscho about fixing it, but he is busy so I will submit a fix
patch myself.
^ permalink raw reply
* Re: [PATCH 3/7 v2] git_extract_argv0_path(): Move check for valid argv0 from caller to callee
From: Johannes Sixt @ 2009-01-18 19:28 UTC (permalink / raw)
To: Steffen Prohaska; +Cc: Junio C Hamano, git, Johannes Schindelin
In-Reply-To: <200901182016.56395.j6t@kdbg.org>
On Sonntag, 18. Januar 2009, Johannes Sixt wrote:
> On Sonntag, 18. Januar 2009, Steffen Prohaska wrote:
> > This simplifies the calling code.
>
> But it could really be squashed into the previous patch, after fixing...
>
> > @@ -23,6 +23,9 @@ const char *system_path(const char *path)
> >
> > const char *git_extract_argv0_path(const char *argv0)
> > {
> > + if (!argv0 || !*argv0)
> > + return 0;
> > +
> > const char *slash = argv0 + strlen(argv0);
>
> ... this declaration after statement.
And we prefer NULL over 0 for the null pointer.
The series is nicely done, thank you! I am using it (the previous round)
without problems so far. I hope we can get this in RSN.
-- Hannes
^ 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