* Re: [RFC/PATCH 2/2] status -s: obey color.status
From: Junio C Hamano @ 2009-11-27 5:15 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <26d0a2022638ad7b75268ca291b8d02a22f1f66c.1259248243.git.git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> * Should I rename wt-status.c's color() into something more unique when
> I export it?
Is it an option to instead move short_unmerged(), short_status() and
friends to wt-status.c from builtin-commit.c? It's been quite a while
since I worked on the code, so I don't recall why it needs such cross
references at low level between two files.
> * Is there any policy regarding use of putchar/puts vs. printf?
J6t addressed it. You have mixture of putchar(' ') and printf(" ") which
looks somewhat funny ;-)
> * The way it is done now I "color" a space, otherwise one would need to
> break down the print statements even more. Since we always color the
> foreground only it is no problem, is it?
Some people do configure to use "reverse". For example, I have:
[diff.color]
old = red reverse
whitespace = blue reverse
[status.color]
updated = green
changed = red
untracked = blue reverse
The output should be consistent between long and short format (I do not
offhand recall what we do for the long format, though).
^ permalink raw reply
* Re: [PATCH] grep: --full-tree
From: Jeff King @ 2009-11-27 6:20 UTC (permalink / raw)
To: James Pickens; +Cc: Junio C Hamano, git
In-Reply-To: <885649360911260956p58c54a54rd887102c9adedcc9@mail.gmail.com>
On Thu, Nov 26, 2009 at 10:56:55AM -0700, James Pickens wrote:
> On Wed, Nov 25, 2009 at 3:20 PM, Jeff King <peff@peff.net> wrote:
> > Sure, there are all those downsides. But what is the other option?
> > Making me use the command line option (or pathspec magic) every single
> > time I invoke git grep?
>
> Yes, but only when you want non-default behavior, not every single time.
Did you miss the part of the thread where I explained that in certain
repos, I want it one way every single time, and in others, I want it the
other way?
So yes, in certain repos, it really is every single time.
> > That is a huge downside to me.
>
> Is it *really*? Does it also bother you that you have to tell standalone
> unix commands like diff and grep what you want them to diff or grep every
> single time you invoke them?
This is a strawman. I am not saying every command-line option should be
made into a configuration option. I am saying that some options,
including this one, would be useful as configuration options. I have
already explained several times in this thread exactly what
characteristics of this option make that so.
And please, questions like "Is it *really*?" don't add anything. Yes,
really, or I wouldn't be having this discussion. This behavior has
bitten me many times while using "git grep". I'm not making it up. Maybe
I am the only one in the world, but I don't see how it makes any sense
to argue that I am not actually annoyed by it.
> I really think that this config option wouldn't even help you, because
> you'll have to remember what that option is set to in each working repo,
> and type the right command based on the setting. That seems worse than
No, the _point_ is that I don't have to remember the right command in
each repo. I can set it up for the workflow that matches that repository
and then issue "git grep" without remembering which type I'm in.
> If you can get the behavior you want using an alias or a script, then I
> suggest you do that. I don't think this config option should be considered
> unless *many* people want it, and so far I count only 1.
Perhaps I am the only one who wants to use the config option per-repo.
But we have already seen support for both behaviors, which means there
are people who will be dissatisfied with either simply leaving the
default or changing the default. And I don't want to speak for Junio,
but he seemed to agree that what you most want would depend on the repo
organization (though I think he may disagree that it is important enough
to merit the hassle of a config option).
-Peff
^ permalink raw reply
* Re: [PATCH] grep: --full-tree
From: Jeff King @ 2009-11-27 6:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8wdup2z2.fsf@alter.siamese.dyndns.org>
On Wed, Nov 25, 2009 at 04:02:57PM -0800, Junio C Hamano wrote:
> Yeah; what is your take on tr/reset-checkout-patch topic, by the way? I
> do not particularly like a configuration that changes the behaviour of a
> command in a drastic way---it will make helping others much harder, but I
> guess it should be Ok?
>
> This may sound like an OffTopic, but because we _are_ discussing
> consistency, it matters.
It is near the top of my to-review queue. Honestly, despite any
arguments I may have made when the original reset/checkout -p series was
posted, I have been pretty happy with the current behavior. I'll take a
look now and respond in more detail in that thread.
-Peff
^ permalink raw reply
* Re: [PATCH] Give the hunk comment its own color
From: Bert Wesarg @ 2009-11-27 6:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7v4oogzo74.fsf@alter.siamese.dyndns.org>
On Fri, Nov 27, 2009 at 03:38, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> may I kindly remind you of this patch.
>
> Yes you may ;-) A more effective would have been a resend but it is
> always appreciated.
>
>> ... If it is only the nen-existing
>> consensus of the default color, than please use the die.
>
> If you are having me go find the mail and apply I would probably use
> "plain" as I suggested.
So I resend with plain as default. And therefore saved one resend ;-)
Bert
>
^ permalink raw reply
* Re: [RFC PATCH] {checkout,reset} -p: make patch direction configurable
From: Jeff King @ 2009-11-27 6:41 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
In-Reply-To: <527e9296b638eb4c9993b3fb0d1c6f51b64f4c2c.1258667920.git.trast@student.ethz.ch>
On Thu, Nov 19, 2009 at 11:03:57PM +0100, Thomas Rast wrote:
> When we implemented the -p mode for checkout, reset and stash, some
> discussion revolved around the involved patch direction.
>
> Make this configurable for reset and checkout with the following
> choices:
>
> index/HEAD other
> forward undo addition undo addition
> mixed undo addition apply removal
> reverse apply removal apply removal
> [...]
> ISTR that Peff wanted this, and maybe some others. I'm not too
> interested because I'm still convinced 'mixed' is the Right Option,
> but it was somewhere deep on my todo stack and maybe you like it ;-)
Actually, I am pretty happy with the current "discard this hunk" most of
the time. It is easy enough to see "you made this change, did you want
to get rid of it?". The one exception is during patch editing. Try
something simple like:
cat >file <<EOF
this
is
a
file
with
some
content
in
it
EOF
git add file
git commit -m base
cat >file <<EOF
this
is
a
file
with
some
other
content
EOF
git checkout -p
Now try to 'e'dit the patch to throw away the addition of "other", but
keep the deletion of the other two lines. Do you find it easy or hard?
Now try it with interactive.checkout.direction set to forward. I find
editing the forward direction _much_ simpler.
Assuming you agree, I'm not sure what that tells us, though. I probably
wouldn't personally set interactive.*.direction for the yes/no part. But
I would find it more convenient to do patch editing always in the
forward direction. I'm worried that it would be too jarring to the user,
though, to see the patch presented in one direction but edit it in the
opposite direction. Maybe it would be nice to have yet another
interactive option to swap between the two for this particular hunk, and
then you could edit the direction you prefer.
Junio raised the question of consistency in another thread. I don't see
a consistency problem here. This is by definition an interactive
procedure. I don't see a reason not to allow the user their preferred
style. But I think if I could swap when editing, I would personally not
care all that much about the other direction.
-Peff
^ permalink raw reply
* Re: [PATCH] Give the hunk comment its own color
From: Jeff King @ 2009-11-27 6:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bert Wesarg, git
In-Reply-To: <7v4oogzo74.fsf@alter.siamese.dyndns.org>
On Thu, Nov 26, 2009 at 06:38:55PM -0800, Junio C Hamano wrote:
> > ... If it is only the nen-existing
> > consensus of the default color, than please use the die.
>
> If you are having me go find the mail and apply I would probably use
> "plain" as I suggested.
As the other person in the discussion, I'll just chime in that I also
think "plain" is the best of the suggested defaults.
-Peff
^ permalink raw reply
* [PATCH v3] Give the hunk comment its own color
From: Bert Wesarg @ 2009-11-27 6:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Bert Wesarg
In-Reply-To: <7v4oogzo74.fsf@alter.siamese.dyndns.org>
Inspired by the coloring of quilt.
Introduce a separate color for the hunk comment part, i.e. the current function.
Whitespace between hunk header and hunk comment is now printed as plain.
The current default is plain.
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Documentation/config.txt | 8 +++---
combine-diff.c | 5 +++-
diff.c | 64 +++++++++++++++++++++++++++++++++++++++++++--
diff.h | 1 +
t/t4034-diff-words.sh | 4 ++-
5 files changed, 73 insertions(+), 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a8e0876..a1e36d7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -635,10 +635,10 @@ color.diff.<slot>::
Use customized color for diff colorization. `<slot>` specifies
which part of the patch to use the specified color, and is one
of `plain` (context text), `meta` (metainformation), `frag`
- (hunk header), `old` (removed lines), `new` (added lines),
- `commit` (commit headers), or `whitespace` (highlighting
- whitespace errors). The values of these variables may be specified as
- in color.branch.<slot>.
+ (hunk header), 'func' (function in hunk header), `old` (removed lines),
+ `new` (added lines), `commit` (commit headers), or `whitespace`
+ (highlighting whitespace errors). The values of these variables may be
+ specified as in color.branch.<slot>.
color.grep::
When set to `always`, always highlight matches. When `false` (or
diff --git a/combine-diff.c b/combine-diff.c
index 5b63af1..6162691 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -524,6 +524,7 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
int i;
unsigned long lno = 0;
const char *c_frag = diff_get_color(use_color, DIFF_FRAGINFO);
+ const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
@@ -588,7 +589,9 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
comment_end = i;
}
if (comment_end)
- putchar(' ');
+ printf("%s%s %s%s", c_reset,
+ c_plain, c_reset,
+ c_func);
for (i = 0; i < comment_end; i++)
putchar(hunk_comment[i]);
}
diff --git a/diff.c b/diff.c
index 0d7f5ea..fd999fb 100644
--- a/diff.c
+++ b/diff.c
@@ -39,6 +39,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_GREEN, /* NEW */
GIT_COLOR_YELLOW, /* COMMIT */
GIT_COLOR_BG_RED, /* WHITESPACE */
+ GIT_COLOR_NORMAL, /* FUNCINFO */
};
static void diff_filespec_load_driver(struct diff_filespec *one);
@@ -60,6 +61,8 @@ static int parse_diff_color_slot(const char *var, int ofs)
return DIFF_COMMIT;
if (!strcasecmp(var+ofs, "whitespace"))
return DIFF_WHITESPACE;
+ if (!strcasecmp(var+ofs, "func"))
+ return DIFF_FUNCINFO;
die("bad config variable '%s'", var);
}
@@ -344,6 +347,63 @@ static void emit_add_line(const char *reset,
}
}
+static void emit_hunk_line(struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+ const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+ const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
+ const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
+ const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
+ const char *orig_line = line;
+ int orig_len = len;
+ const char *frag_start;
+ int frag_len;
+ const char *part_end = NULL;
+ int part_len = 0;
+
+ /* determine length of @ */
+ while (part_len < len && line[part_len] == '@')
+ part_len++;
+
+ /* find end of frag, (Ie. find second @@) */
+ part_end = memmem(line + part_len, len - part_len,
+ line, part_len);
+ if (!part_end)
+ return emit_line(ecbdata->file, frag, reset, line, len);
+ /* calculate total length of frag */
+ part_len = (part_end + part_len) - line;
+
+ /* remember frag part, we emit only if we find a space separator */
+ frag_start = line;
+ frag_len = part_len;
+
+ /* consume hunk header */
+ len -= part_len;
+ line += part_len;
+
+ /*
+ * for empty reminder or empty space sequence (exclusive any newlines
+ * or carriage returns) emit complete original line as FRAGINFO
+ */
+ if (!len || !(part_len = strspn(line, " \t")))
+ return emit_line(ecbdata->file, frag, reset,
+ orig_line, orig_len);
+
+ /* now emit the hunk header as FRAGINFO */
+ emit_line(ecbdata->file, frag, reset, frag_start, frag_len);
+
+ /* print whitespace sep as PLAIN */
+ emit_line(ecbdata->file, plain, reset, line, part_len);
+
+ /* consume whitespace sep */
+ len -= part_len;
+ line += part_len;
+
+ /* print reminder as FUNCINFO */
+ if (len)
+ emit_line(ecbdata->file, func, reset, line, len);
+}
+
static struct diff_tempfile *claim_diff_tempfile(void) {
int i;
for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
@@ -781,9 +841,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
diff_words_flush(ecbdata);
len = sane_truncate_line(ecbdata, line, len);
find_lno(line, ecbdata);
- emit_line(ecbdata->file,
- diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),
- reset, line, len);
+ emit_hunk_line(ecbdata, line, len);
if (line[len-1] != '\n')
putc('\n', ecbdata->file);
return;
diff --git a/diff.h b/diff.h
index 2740421..15fcecd 100644
--- a/diff.h
+++ b/diff.h
@@ -130,6 +130,7 @@ enum color_diff {
DIFF_FILE_NEW = 5,
DIFF_COMMIT = 6,
DIFF_WHITESPACE = 7,
+ DIFF_FUNCINFO = 8,
};
const char *diff_get_color(int diff_use_color, enum color_diff ix);
#define diff_get_color_opt(o, ix) \
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 21db6e9..186eff8 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -8,6 +8,7 @@ test_expect_success setup '
git config diff.color.old red
git config diff.color.new green
+ git config diff.color.func magenta
'
@@ -16,6 +17,7 @@ decrypt_color () {
-e 's/.\[1m/<WHITE>/g' \
-e 's/.\[31m/<RED>/g' \
-e 's/.\[32m/<GREEN>/g' \
+ -e 's/.\[35m/<MAGENTA>/g' \
-e 's/.\[36m/<BROWN>/g' \
-e 's/.\[m/<RESET>/g'
}
@@ -70,7 +72,7 @@ cat > expect <<\EOF
<WHITE>+++ b/post<RESET>
<BROWN>@@ -1 +1 @@<RESET>
<RED>h(4)<RESET><GREEN>h(4),hh[44]<RESET>
-<BROWN>@@ -3,0 +4,4 @@ a = b + c<RESET>
+<BROWN>@@ -3,0 +4,4 @@<RESET> <RESET><MAGENTA>a = b + c<RESET>
<GREEN>aa = a<RESET>
--
tg: (ad7ace7..) bw/func-color (depends on: master)
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2009, #06; Wed, 25)
From: Jeff King @ 2009-11-27 6:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8wduksgq.fsf@alter.siamese.dyndns.org>
On Wed, Nov 25, 2009 at 05:03:33PM -0800, Junio C Hamano wrote:
> * jc/grep-full-tree (2009-11-24) 1 commit.
> - grep: --full-tree
>
> We probably would want test, doc and a configuration variable to make it
> default (or non-default) before we can merge it to 'master'.
I can try to pick this up. But did we reach a decision on having a
configuration variable?
-Peff
^ permalink raw reply
* [PATCH] Add a notice that only certain functions can print color escape codes
From: Johannes Sixt @ 2009-11-27 7:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vy6lt6rh3.fsf@alter.siamese.dyndns.org>
From: Johannes Sixt <j6t@kdbg.org>
We emulate color escape codes on Windows by overriding printf, fprintf,
and fputs. Warn users that these are the only functions that can be used
to print them.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> Michael J Gruber schrieb:
>>> * Is there any policy regarding use of putchar/puts vs. printf?
>> If the printed string contains color escapes that should be obeyed, you
>> can use only fputs, printf, and fprintf. You should not use puts or putchar.
>
> This msysgit-imposed restriction is something even I do not remember
> offhand. Could you please document it somewhere in a file any developer
> would get by checking out the 'master' branch of git.git?
Like this?
color.h | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/color.h b/color.h
index 7d8da6f..edeaa3e 100644
--- a/color.h
+++ b/color.h
@@ -4,6 +4,11 @@
/* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
#define COLOR_MAXLEN 24
+/*
+ * IMPORTANT: Due to the way these color codes are emulated on Windows,
+ * write them only using printf, fprintf, and fputs. In particular,
+ * do not use puts.
+ */
#define GIT_COLOR_NORMAL ""
#define GIT_COLOR_RESET "\033[m"
#define GIT_COLOR_BOLD "\033[1m"
--
1.6.6.rc0.43.g50037
^ permalink raw reply related
* Re: [RFC/PATCH 1/2] status -s: respect the status.relativePaths option
From: Jeff King @ 2009-11-27 7:05 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <62c5bb36940485deefbf73f4bdc3fd45bbea069e.1259248243.git.git@drmicha.warpmail.net>
On Thu, Nov 26, 2009 at 04:24:38PM +0100, Michael J Gruber wrote:
> so that 'status' and 'status -s' in a subdir produce the same file
> names.
Thanks, I had been meaning to look into this (who knew it would only be
two lines ;) ).
Now that short and porcelain formats are diverging a bit, we should
probably have a few tests. I think there may already be a bug in
git commit --dry-run --porcelain
as it looks like commit will use color and relative path configuration
even for the porcelain version.
-Peff
^ permalink raw reply
* Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()
From: Johannes Sixt @ 2009-11-27 7:09 UTC (permalink / raw)
To: kusmabite; +Cc: Junio C Hamano, msysgit, git, dotzenlabs, Alex Riesen
In-Reply-To: <40aa078e0911261537r40b19dffqf019848dcad23fef@mail.gmail.com>
Erik Faye-Lund schrieb:
> Perhaps I can do one better: use memcpy instead of standard
> assignment. The Autoconf manual[1] suggests that it's more portable.
> Something like this:
>
> #ifndef va_copy
> # ifdef __va_copy
> # define va_copy(a,b) __va_copy(a,b)
> # else
> # define va_copy(a,b) memcpy(&a, &b, sizeof (va_list))
> # endif
> #endif
>
> I'll add this to git-compat-util.h this for the next round unless
> someone yells really loud at me.
As I said elsewhere in the thread, I do not see enough reason to add
strbuf_vaddf() only for a syslog() emulation in the first place.
-- Hannes
^ permalink raw reply
* Re: [PATCH] Give the hunk comment its own color
From: Junio C Hamano @ 2009-11-27 7:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Bert Wesarg, git
In-Reply-To: <20091127065202.GD20844@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Nov 26, 2009 at 06:38:55PM -0800, Junio C Hamano wrote:
>
>> > ... If it is only the nen-existing
>> > consensus of the default color, than please use the die.
>>
>> If you are having me go find the mail and apply I would probably use
>> "plain" as I suggested.
>
> As the other person in the discussion, I'll just chime in that I also
> think "plain" is the best of the suggested defaults.
Ok, I tweaked the patch locally and applied.
Thanks.
^ permalink raw reply
* Re: What should a user expect from git log -M -- file
From: Mike Hommey @ 2009-11-27 7:28 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3ocmpmcoq.fsf@localhost.localdomain>
On Thu, Nov 26, 2009 at 09:14:37AM -0800, Jakub Narebski wrote:
> Mike Hommey <mh@glandium.org> writes:
>
> > I recently reorganized a project of mine, and the result is that a lot of
> > files moved from the top directory to a sub directory.
> >
> > Now, I innocently tried to 'git log -M' some of these files in the
> > subdirectories, and well, the history just stops when the file was
> > created. Obviously, if I put both the old and the new location it works,
> > but shouldn't users expect 'git log -M -- file' to try to find the
> > previous path and continue from there ?
>
> What you want is not
>
> git log -M -- file
>
> but
>
> git log --follow file
>
> "git log -M -- file" IIRC first applies path limiting, simplifying
> history, *then* does rename detection, and finally filters output
> (unless --full-diff is used).
That's what I was looking for, thanks. I would suggest to put --follow
closer to -M and -C in the documentation, but the way the git-log
manual is generated (including diff options) makes that impossible :(
Mike
^ permalink raw reply
* Re: [PATCH] Add a notice that only certain functions can print color escape codes
From: Junio C Hamano @ 2009-11-27 7:30 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Michael J Gruber, git
In-Reply-To: <4B0F78F1.4040101@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> Like this?
Yeah, and in addition to "puts", "write(2)" is also not supported, right?
> color.h | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/color.h b/color.h
> index 7d8da6f..edeaa3e 100644
> --- a/color.h
> +++ b/color.h
> @@ -4,6 +4,11 @@
> /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
> #define COLOR_MAXLEN 24
>
> +/*
> + * IMPORTANT: Due to the way these color codes are emulated on Windows,
> + * write them only using printf, fprintf, and fputs. In particular,
> + * do not use puts.
> + */
> #define GIT_COLOR_NORMAL ""
> #define GIT_COLOR_RESET "\033[m"
> #define GIT_COLOR_BOLD "\033[1m"
> --
> 1.6.6.rc0.43.g50037
^ permalink raw reply
* Re: [PATCH] Add a notice that only certain functions can print color escape codes
From: Johannes Sixt @ 2009-11-27 7:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vzl68v2zb.fsf@alter.siamese.dyndns.org>
Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> Like this?
>
> Yeah, and in addition to "puts", "write(2)" is also not supported, right?
Correct, good catch!
--- 8< ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] Add a notice that only certain functions can print color escape codes
We emulate color escape codes on Windows by overriding printf, fprintf,
and fputs. Warn users that these are the only functions that can be used
to print them.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
color.h | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/color.h b/color.h
index 7d8da6f..3cb4b7f 100644
--- a/color.h
+++ b/color.h
@@ -4,6 +4,11 @@
/* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
#define COLOR_MAXLEN 24
+/*
+ * IMPORTANT: Due to the way these color codes are emulated on Windows,
+ * write them only using printf(), fprintf(), and fputs(). In particular,
+ * do not use puts() or write().
+ */
#define GIT_COLOR_NORMAL ""
#define GIT_COLOR_RESET "\033[m"
#define GIT_COLOR_BOLD "\033[1m"
--
1.6.6.rc0.43.g50037
^ permalink raw reply related
* [PATCH] Makefile: determine the list of header files using a glob
From: Johannes Sixt @ 2009-11-27 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
From: Johannes Sixt <j6t@kdbg.org>
The list of header files was incomplete because a number of header files
were added to the code base, but were not added to the list LIB_H that we
have in the Makefile. This meant that no rebuild was triggered if one of
the missing headers was changed because we do not have automatic
dependency tracking, either.
Sidestep the issue by computing the list using $(wildcard).
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Makefile | 63 +------------------------------------------------------------
1 files changed, 2 insertions(+), 61 deletions(-)
diff --git a/Makefile b/Makefile
index 5a0b3d4..279c7e4 100644
--- a/Makefile
+++ b/Makefile
@@ -326,7 +326,6 @@ BUILTIN_OBJS =
BUILT_INS =
COMPAT_CFLAGS =
COMPAT_OBJS =
-LIB_H =
LIB_OBJS =
PROGRAMS =
SCRIPT_PERL =
@@ -429,65 +428,7 @@ export PERL_PATH
LIB_FILE=libgit.a
XDIFF_LIB=xdiff/lib.a
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += attr.h
-LIB_H += blob.h
-LIB_H += builtin.h
-LIB_H += cache.h
-LIB_H += cache-tree.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/cygwin.h
-LIB_H += compat/mingw.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diffcore.h
-LIB_H += diff.h
-LIB_H += dir.h
-LIB_H += fsck.h
-LIB_H += git-compat-util.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hash.h
-LIB_H += help.h
-LIB_H += levenshtein.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-recursive.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack.h
-LIB_H += pack-refs.h
-LIB_H += pack-revindex.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pkt-line.h
-LIB_H += progress.h
-LIB_H += quote.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += sha1-lookup.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += transport.h
-LIB_H += tree.h
-LIB_H += tree-walk.h
-LIB_H += unpack-trees.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += wt-status.h
+LIB_H = $(wildcard *.h */*.h compat/*/*.h)
LIB_OBJS += abspath.o
LIB_OBJS += advice.o
@@ -1611,7 +1552,7 @@ git-remote-curl$X: remote-curl.o http.o http-walker.o $(GITLIBS)
$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
$(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
-$(patsubst git-%$X,%.o,$(PROGRAMS)) git.o: $(LIB_H) $(wildcard */*.h)
+$(patsubst git-%$X,%.o,$(PROGRAMS)) git.o: $(LIB_H)
builtin-revert.o wt-status.o: wt-status.h
$(LIB_FILE): $(LIB_OBJS)
--
1.6.6.rc0.43.g50037
^ permalink raw reply related
* Re: [msysGit] [PATCH/RFC 03/11] mingw: implement syslog
From: Erik Faye-Lund @ 2009-11-27 8:09 UTC (permalink / raw)
To: Johannes Sixt; +Cc: msysgit, git, dotzenlabs
In-Reply-To: <200911262223.22777.j6t@kdbg.org>
On Thu, Nov 26, 2009 at 10:23 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Donnerstag, 26. November 2009, Erik Faye-Lund wrote:
>> + struct strbuf msg;
>> + va_list va;
>> + WORD logtype;
>> +
>> + strbuf_init(&msg, 0);
>> + va_start(va, fmt);
>> + strbuf_vaddf(&msg, fmt, va);
>> + va_end(va);
>
> I would
>
> const char* arg;
> if (strcmp(fmt, "%s"))
> die("format string of syslog() not implemented")
> va_start(va, fmt);
> arg = va_arg(va, char*);
> va_end(va);
>
> because we have exactly one invocation of syslog(), which passes "%s" as
> format string. Then strbuf_vaddf is not needed. Or even simpler: declare the
> function as
>
> void syslog(int priority, const char *fmt, const char*arg);
After reading this again, I agree that this is the best solution. I'll
update for the next iteration.
>> + ReportEventA(ms_eventlog,
>> + logtype,
>> + (WORD)NULL,
>> + (DWORD)NULL,
>> + NULL,
>> + 1,
>> + 0,
>> + (const char **)&msg.buf,
>> + NULL);
>
> Why do you pass NULL and apply casts? The first one gives a warning.
I'll remove the casts for the next iteration.
>
> Did you read this paragraph in the documentation?
> http://msdn.microsoft.com/en-us/library/aa363679(VS.85).aspx
>
> "Note that the string that you log cannot contain %n, where n is an integer
> value (for example, %1) because the event viewer treats it as an insertion
> string. ..."
>
> How are the chances that this condition applies to our use of the function?
>
Ugh, increasingly high since we're adding IPv6 support, I guess.
Perhaps some form of escaping needs to be done?
--
Erik "kusma" Faye-Lund
^ permalink raw reply
* Re: [PATCH] grep: --full-tree
From: Junio C Hamano @ 2009-11-27 8:18 UTC (permalink / raw)
To: Jeff King; +Cc: James Pickens, git
In-Reply-To: <20091127062013.GA20844@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... And I don't want to speak for Junio,
> but he seemed to agree that what you most want would depend on the repo
> organization (though I think he may disagree that it is important enough
> to merit the hassle of a config option).
Oh, I totally agree with you that what's convenient is different per
<project, the role I play in the project> pair.
In my day-job project, I almost always work in a directory four levels
down from the toplevel ("src/lib/u/<something>") and almost always what to
run grep from near the top ("/src" in the strawman syntax to grep from
that directory). I have similar preference when playing with projects I
am not very familiar with---hack in a limited area somewhere deeper, but
grep a lot more widely. So it is very tempting to say that I would want
"full-tree" configured as default in these repositories.
But the conclusion I draw from that observation is that it should be
equally easy for me to invoke both behaviours from the command line, and
not "I want to freeze which default is used for the project by setting a
configuration variable", because I know "the role I play" part changes
from time to time (note that I didn't say "changes over time"---that can
be addressed by "Then you should flip the config when the day comes").
At first sight, git.git is too shallow for "full-tree" vs "current
directory" distinction to make any meaningful difference, especially
because it is very top heavy and I almost always am at the top level
directory. But even there, I can clearly see that I have need for easy
access to both modes. I sometimes play a contributor who is interested in
and is very familiar with a specific area (hence I know in which files to
find strings without resorting to full-tree grep), and other times play a
reviewer role who tries to follow what an area expert, who is much more
familiar than me in some parts of the system, did in his patch. In the
latter case, I would not know in which files to grep offhand, and would
benefit from "tree-wide" option, in order to find out what is done by that
obscure function the area expert used in his patch.
A repository-wide configuration would not help me at all, but a way to
invoke either mode from the command line that is short-and-sweet would
consistently give me the desired result without having to remember what
the default-of-the-day (or default-in-the-repo) is.
But the above is mostly about "Peff wants config, Junio thinks it won't
help him", and is not really a disagreement. As long as we don't use the
existence of configurable defaults as an excuse for making/leaving it very
cumbersome to invoke the non-default mode from command line, "it won't
help some users" is not a reason to block it.
I suspect a per-repo configuration _might_ confuse new people (and people
who help them), and I brought it up as a potential issue myself. That
could be a more valid reason to object to configurable default, but I
haven't formed an opinion how serious a problem it would be in real life;
I should sleep on this one and wait for others' opinions.
Regardless of the above, there are unresolved issues in the --full-tree
patch as posted.
We've been saying that even if the default were changed to grep in the
full tree, limiting to the current directory is easy with a single "." at
the end, but I do not think it is entirely true. I often want to grep in
only "*.h" files but they are spread across the directories. If you do
$ git grep -e frotz -- "*.h"
it finds in paths that match the pathspec in the current directory with
today's default, but after we flip the default (either globally or with
your configuration per-repo), it won't be possible to limit the search to
header files in the current directory and under. It will find in header
files spread over in the whole tree.
Also the --full-tree option I did at the beginning of this thread doesn't
work with the above command line example, as it only kicks in when there
is no pathspec.
Maybe these are not worth solving, and we should keep the current default
and just tell ourselves to go up to the level we want to grep from. That
would be simple, robust and the easiest to explain.
^ permalink raw reply
* Re: [PATCH] Makefile: determine the list of header files using a glob
From: Mike Hommey @ 2009-11-27 8:26 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <4B0F8825.3040107@viscovery.net>
On Fri, Nov 27, 2009 at 09:04:53AM +0100, Johannes Sixt wrote:
> From: Johannes Sixt <j6t@kdbg.org>
>
> The list of header files was incomplete because a number of header files
> were added to the code base, but were not added to the list LIB_H that we
> have in the Makefile. This meant that no rebuild was triggered if one of
> the missing headers was changed because we do not have automatic
> dependency tracking, either.
>
> Sidestep the issue by computing the list using $(wildcard).
I don't know if the current Makefile works with Solaris' make, or if GNU
make has to be used, but $(wildcard) is definitely not supported by
Solaris' make.
Mike
^ permalink raw reply
* Re: [PATCH v3] Give the hunk comment its own color
From: Junio C Hamano @ 2009-11-27 8:38 UTC (permalink / raw)
To: Bert Wesarg; +Cc: Jeff King, git
In-Reply-To: <1259304918-12600-1-git-send-email-bert.wesarg@googlemail.com>
I was slightly surprised that this seems to have differences other than
the flipping of the default color since the last one, especially after you
sounded like you would be sending with only that change.
^ permalink raw reply
* Re: [PATCH v3] Give the hunk comment its own color
From: Bert Wesarg @ 2009-11-27 8:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7v3a40tl9t.fsf@alter.siamese.dyndns.org>
On Fri, Nov 27, 2009 at 09:38, Junio C Hamano <gitster@pobox.com> wrote:
> I was slightly surprised that this seems to have differences other than
> the flipping of the default color since the last one, especially after you
> sounded like you would be sending with only that change.
v3 does only that and adopt the change to the t/t4034-diff-words.sh
test. There I set an explicit value of func to magenta for testing.
Maybe you missed v2 (Message-Id:
<1258557087-31540-1-git-send-email-bert.wesarg@googlemail.com>)? Which
fixed the test and also a small bug.
Bert
>
^ permalink raw reply
* Re: [PATCH] Makefile: determine the list of header files using a glob
From: Johannes Sixt @ 2009-11-27 8:50 UTC (permalink / raw)
To: Mike Hommey; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20091127082624.GA19875@glandium.org>
Mike Hommey schrieb:
> I don't know if the current Makefile works with Solaris' make,...
No, it doesn't. You have to use GNU make anyway.
-- Hannes
^ permalink raw reply
* Re: [PATCH] Makefile: determine the list of header files using a glob
From: Mike Hommey @ 2009-11-27 8:58 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <4B0F92E7.8090403@viscovery.net>
On Fri, Nov 27, 2009 at 09:50:47AM +0100, Johannes Sixt wrote:
> Mike Hommey schrieb:
> > I don't know if the current Makefile works with Solaris' make,...
>
> No, it doesn't. You have to use GNU make anyway.
Then it's fine. But shouldn't that be noted somewhere, like in the
INSTALL file ?
Mike
^ permalink raw reply
* Re: [PATCH v3] Give the hunk comment its own color
From: Junio C Hamano @ 2009-11-27 8:59 UTC (permalink / raw)
To: Bert Wesarg; +Cc: Jeff King, git
In-Reply-To: <36ca99e90911270044o68375902l3a0d2a4afa726a91@mail.gmail.com>
Bert Wesarg <bert.wesarg@googlemail.com> writes:
> Maybe you missed v2 (Message-Id:
> <1258557087-31540-1-git-send-email-bert.wesarg@googlemail.com>)? Which
> fixed the test and also a small bug.
Yeah, that was what happened. Thanks for clarifying.
^ permalink raw reply
* Re: [RFC/PATCH 2/2] status -s: obey color.status
From: Michael J Gruber @ 2009-11-27 9:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vws1cwnu0.fsf@alter.siamese.dyndns.org>
Junio C Hamano venit, vidit, dixit 27.11.2009 06:15:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> * Should I rename wt-status.c's color() into something more unique when
>> I export it?
>
> Is it an option to instead move short_unmerged(), short_status() and
> friends to wt-status.c from builtin-commit.c? It's been quite a while
> since I worked on the code, so I don't recall why it needs such cross
> references at low level between two files.
I didn't notice, but I'll look into it during the rewrite.
>> * Is there any policy regarding use of putchar/puts vs. printf?
>
> J6t addressed it. You have mixture of putchar(' ') and printf(" ") which
> looks somewhat funny ;-)
;) I'm happy with printf("c"), I just thought someone would find printf
overkill for a format less char.
>> * The way it is done now I "color" a space, otherwise one would need to
>> break down the print statements even more. Since we always color the
>> foreground only it is no problem, is it?
>
> Some people do configure to use "reverse". For example, I have:
>
> [diff.color]
> old = red reverse
> whitespace = blue reverse
>
> [status.color]
> updated = green
> changed = red
> untracked = blue reverse
>
> The output should be consistent between long and short format (I do not
> offhand recall what we do for the long format, though).
Oh, I didn't know about reverse. In this case I have to change the code
and leave the space alone. (The one between ?? and the filename.) Will do.
In the long format, only the file name is colored. Note that in the
short format, it does not make sense to color the file name because one
line may represent two pieces of status information. That is why I color
the two status letters and not the file name.
Michael
^ 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