* Re: [PATCH] Color support added to git-add--interactive.
From: Jeff King @ 2007-10-13 8:12 UTC (permalink / raw)
To: Dan Zwell
Cc: Git Mailing List, Jonathan del Strother,
Johannes Schindelin <Johannes.Schindelin@gmx.de>Wincent Colaiuta
In-Reply-To: <471045DA.5050902@gmail.com>
On Fri, Oct 12, 2007 at 11:13:14PM -0500, Dan Zwell wrote:
> Recently there was some talk of color for git-add--interactive, but the
> person who said he already had a patch didn't produce it.
Neat, thanks for working on this.
> There is one problem--a block is commented out, because adding the "--color"
> option to git-diff-files somehow breaks git-add--interactive, and I would
I believe it's because add--interactive parses the output of
git-diff-files, so it expects unadorned diffs. I think you may be stuck
re-coloring the diffs yourself, which is a little ugly.
> tabs. Feel free to replace the colors that I chose with something that
> better conforms to the "git style", if there is such a thing.
Two suggestions:
- every color should be configurable (e.g., see diff color options)
- where possible, use existing color config (e.g., for diffs)
You will never come up with a color scheme that satisfies everyone
(e.g., white text on black background versus black text on white
background), so configurability is a good idea (not to mention that
nobody will ever agree on what looks "good").
> +if ($color_config=~"true" || -t STDOUT && $color_config=~"auto") {
Shouldn't these just be 'eq' instead of a regex?
> + print_ansi_color "bold";
> print "$opts->{HEADER}\n";
> + print_ansi_color "clear";
ISTR some terminals had issues with leaving ANSI attributes set across a
newline. That was the reason for the color_fprintf_ln business in
color.[ch]. You might replace this with something like:
print_color_ln 'bold', $opts->{HEADER};
where "print_color_ln" turns off the attribute before the newline.
> + # FIXME: the following breaks git, and I'm not sure why. When
> + # the following is uncommented, git no longer asks whether we
> + # want to add given hunks.
> + #my @diff;
> + #if ($use_color) {
> + # #@diff = run_cmd_pipe(qw(git diff-files --color -p --), $path);
> + #}
> + #else {
> + # #@diff = run_cmd_pipe(qw(git diff-files -p --), $path);
> + #}
> my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
See how we are pulling the diff into lines? Look a few lines below and
you will see that we start parsing without regard to the color.
Unfortunately, that parsed form ends up being output to the user, so we
will have to do colorization at that point (fortunately, diff
colorization with regexes isn't _that_ hard).
> + print_ansi_color "blue";
> print <<\EOF ;
> y - stage this hunk
> n - do not stage this hunk
> @@ -555,6 +582,7 @@ k - leave this hunk undecided, see previous undecided
> hunk
> K - leave this hunk undecided, see previous hunk
> s - split the current hunk into smaller hunks
> EOF
> + print_ansi_color "clear";
Hrm, splitting this with print_color_ln as I mentioned above would be a
little painful. Maybe something like this (totally untested):
# Turn on ansi attributes at the beginning of the string and at
# the beginning of each line, but then turn them off before each
# newline. This should give the effect of covering the whole string
# with the attribute, but not have attributes cross newline boundaries.
sub color_print {
my $attr = shift;
local $_ = shift;
if ($use_color) {
s/^/Term::ANSIColor::color($attr)/mge;
s/\n/Term::ANSIColor::color('reset') . $&/ge;
}
print $_;
}
-Peff
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Johannes Schindelin @ 2007-10-13 12:25 UTC (permalink / raw)
To: Dan Zwell
Cc: Git Mailing List, Jeff King, Wincent Colaiuta,
Jonathan del Strother
In-Reply-To: <471045DA.5050902@gmail.com>
Hi,
[Cc'ed Wincent correctly]
On Fri, 12 Oct 2007, Dan Zwell wrote:
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index be68814..f55d787 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -2,6 +2,18 @@
>
> use strict;
>
> +my $use_color;
> +my $color_config = qx(git config --get color.add-interactive);
> +if ($color_config=~"true" || -t STDOUT && $color_config=~"auto") {
> + $use_color = "true";
> + require Term::ANSIColor;
> +}
Good. If you do not have color enabled, it does not require that package
that is only default with modern Perl.
> @@ -175,7 +187,9 @@ sub list_and_choose {
> if (!$opts->{LIST_FLAT}) {
> print " ";
> }
> + print_ansi_color "bold";
> print "$opts->{HEADER}\n";
> + print_ansi_color "clear";
Here you say "clear", and ...
> }
> for ($i = 0; $i < @stuff; $i++) {
> my $chosen = $chosen[$i] ? '*' : ' ';
> @@ -205,7 +219,9 @@ sub list_and_choose {
>
> return if ($opts->{LIST_ONLY});
>
> + print_ansi_color "bold blue";
> print $opts->{PROMPT};
> + print_ansi_color "reset";
here you say "reset". Is it because of the added colour?
> @@ -338,6 +354,16 @@ sub add_untracked_cmd {
>
> sub parse_diff {
> my ($path) = @_;
> + # FIXME: the following breaks git, and I'm not sure why. When
> + # the following is uncommented, git no longer asks whether we
> + # want to add given hunks.
> + #my @diff;
> + #if ($use_color) {
> + # #@diff = run_cmd_pipe(qw(git diff-files --color -p --), $path);
> + #}
> + #else {
> + # #@diff = run_cmd_pipe(qw(git diff-files -p --), $path);
> + #}
> my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
> my (@hunk) = { TEXT => [] };
This fails because of the next two lines:
for (@diff) {
if (/^@@ /) {
Replace the if with "if (/^[^-+ ]*@@ /)", or something even stricter.
--color adds magic sequences to make color.
I cannot comment on the Perl style ;-)
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Frank Lichtenheld @ 2007-10-13 12:49 UTC (permalink / raw)
To: Jeff King
Cc: Dan Zwell, Git Mailing List, Jonathan del Strother,
Johannes Schindelin, Wincent Colaiuta
In-Reply-To: <20071013081205.GB27533@coredump.intra.peff.net>
On Sat, Oct 13, 2007 at 04:12:06AM -0400, Jeff King wrote:
> > +if ($color_config=~"true" || -t STDOUT && $color_config=~"auto") {
>
> Shouldn't these just be 'eq' instead of a regex?
What would mean you have to chomp it first. But it should at least
be written as =~ /.../ to make it clear that using a regex was a
concious decision here and not an accident.
Gruesse,
--
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
^ permalink raw reply
* Re: git-fast-import crashes
From: Johannes Schindelin @ 2007-10-13 12:58 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Shun Kei Leung, git
In-Reply-To: <20071013032916.GL27899@spearce.org>
Hi,
On Fri, 12 Oct 2007, Shawn O. Pearce wrote:
> Shun Kei Leung <kevinlsk@gmail.com> wrote:
> > Program received signal EXC_BAD_ACCESS, Could not access memory.
> > Reason: KERN_INVALID_ADDRESS at address: 0x64617469
> > in_window (win=0x5004d0, offset=3501) at sha1_file.c:701
> > 701 off_t win_off = win->offset;
> ...
> > (gdb) print win
> > $1 = (struct pack_window *) 0x5004d0
> > (gdb) print *win
> > $2 = {
> > next = 0x64617461,
> > base = 0x20333936 <Address 0x20333936 out of bounds>,
> > offset = 22523564414626158,
> > len = 1685026675,
> > last_used = 795894075,
> > inuse_cnt = 0
> > }
>
> Wow. There's no way that struct pack_window is valid anymore.
>
> [...]
>
> This looks like it is memory corruption (e.g. someone overwriting a
> free'd segment), but that sort of memory corruption is very hard to
> track down.
I found valgrind invaluable to find such errors.
Ciao,
Dscho
^ permalink raw reply
* Re: Git User's Survey 2007 unfinished summary continued
From: Frank Lichtenheld @ 2007-10-13 12:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narebski, git
In-Reply-To: <Pine.LNX.4.64.0710130130380.25221@racer.site>
On Sat, Oct 13, 2007 at 01:46:40AM +0100, Johannes Schindelin wrote:
> On Sat, 13 Oct 2007, Jakub Narebski wrote:
> > Figure out why people find git hard to learn and eliminate those
> > barriers to entry. Make git more task-oriented rather than
> > data-model-oriented the way it is now.
>
> Frankly, expectations like these make me want to bang somebody's head on
> the wall. Why do people expect others to work for them for free? Hard?
It's called "User". And since this is the Git _User's_ Survey, I guess
you will have to live with that. And anyway, where in the above you find
something about "expectation" rather than "suggestion"?
Gruesse,
--
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
^ permalink raw reply
* Re: Git User's Survey 2007 unfinished summary continued
From: Johannes Schindelin @ 2007-10-13 13:04 UTC (permalink / raw)
To: Frank Lichtenheld; +Cc: Jakub Narebski, git
In-Reply-To: <20071013125845.GA31659@planck.djpig.de>
Hi,
On Sat, 13 Oct 2007, Frank Lichtenheld wrote:
> On Sat, Oct 13, 2007 at 01:46:40AM +0100, Johannes Schindelin wrote:
> > On Sat, 13 Oct 2007, Jakub Narebski wrote:
> > > Figure out why people find git hard to learn and eliminate those
> > > barriers to entry. Make git more task-oriented rather than
> > > data-model-oriented the way it is now.
> >
> > Frankly, expectations like these make me want to bang somebody's head
> > on the wall. Why do people expect others to work for them for free?
> > Hard?
>
> It's called "User". And since this is the Git _User's_ Survey, I guess
> you will have to live with that. And anyway, where in the above you find
> something about "expectation" rather than "suggestion"?
"Figure out" sounds to me like an imperative.
But my real point is: these guys know exactly what they find hard in git.
Why don't they just come and tell us?
Just like Carl Worth did, way back when. And it worked, didn't it? Git
1.5 is vastly more user friendly than pre 1.5.
Ciao,
Dscho
^ permalink raw reply
* [RFC] CLI option parsing and usage generation for porcelains
From: Pierre Habouzit @ 2007-10-13 13:29 UTC (permalink / raw)
To: git
Following Kristian momentum, I've reworked his parse_option module
quite a lot, and now have some quite interesting features. The series is
available from git://git.madism.org/git.git (branch ph/strbuf).
The following series is open for comments, it's not 100% ready for
inclusion IMHO, as some details may need to be sorted out first, and
that I've not re-read the patches thoroughly yet. Though I uses the tip
of that branch as my everyday git for 2 weeks or so without any
noticeable issues.
And as examples are always easier to grok:
$ git fetch -h
usage: git-fetch [options] [<repository> <refspec>...]
-q, --quiet be quiet
-v, --verbose be verbose
-a, --append append in .git/FETCH_HEAD
-f, --force force non fast-forwards updates
--no-tags don't follow tags at all
-t, --tags fetch all tags
--depth <depth> deepen history of a shallow clone
Advanced Options
-k, --keep keep downloaded pack
-u, --update-head-ok allow to update the head in the current branch
--upload-pack <path> path to git-upload-pack on the remote
$ git rm -rf xdiff # yeah -rf now works !
rm 'xdiff/xdiff.h'
rm 'xdiff/xdiffi.c'
rm 'xdiff/xdiffi.h'
rm 'xdiff/xemit.c'
rm 'xdiff/xemit.h'
rm 'xdiff/xinclude.h'
rm 'xdiff/xmacros.h'
rm 'xdiff/xmerge.c'
rm 'xdiff/xprepare.c'
rm 'xdiff/xprepare.h'
rm 'xdiff/xtypes.h'
rm 'xdiff/xutils.c'
rm 'xdiff/xutils.h'
^ permalink raw reply
* [bug] gitk can not read history if diff color is enabled
From: Mike Sharov @ 2007-10-13 14:18 UTC (permalink / raw)
To: git
Reproduced in git version 1.5.3.4.217.gbfc1, although it's been this way
for a while. To see the bug, enable color diff with
[diff]
color
and launch gitk on any repository (like the Linux kernel tree). The
result is a dialog box "Can't parse git log output '\x1b[33mcommit" etc.
qgit can still view the history without errors, so I guess it must be
possible to somehow turn off color for a specific query.
--
Mike
msharov@users.sourceforge.net
^ permalink raw reply
* Re: [PATCH] Add a simple option parser.
From: Johannes Schindelin @ 2007-10-13 14:39 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, Junio C Hamano
In-Reply-To: <1192282153-26684-2-git-send-email-madcoder@debian.org>
Hi,
On Sat, 13 Oct 2007, Pierre Habouzit wrote:
> Aggregation of single switches is allowed:
> -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
I'd be more interested in "-rC 0" working... Is that supported, too?
> diff --git a/parse-options.c b/parse-options.c
> new file mode 100644
> index 0000000..07abb50
> --- /dev/null
> +++ b/parse-options.c
> @@ -0,0 +1,227 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +#include "strbuf.h"
> +
> +#define OPT_SHORT 1
> +#define OPT_UNSET 2
> +
> +struct optparse_t {
> + const char **argv;
> + int argc;
> + const char *opt;
> +};
> +
> +static inline const char *get_arg(struct optparse_t *p)
> +{
> + if (p->opt) {
> + const char *res = p->opt;
> + p->opt = NULL;
> + return res;
> + }
> + p->argc--;
> + return *++p->argv;
> +}
This is only used once; I wonder if it is really that more readable having
this as a function in its own right.
> +static inline const char *skippfx(const char *str, const char *prefix)
Personally, I do not like abbreviations like that. They do not save that
much screen estate (skip_prefix is only 4 characters longer, but much more
readable). Same goes for "cnt" later.
> +static int get_value(struct optparse_t *p, struct option *opt, int flags)
> +{
> + if (p->opt && (flags & OPT_UNSET))
> + return opterror(opt, "takes no value", flags);
> +
> + switch (opt->type) {
> + case OPTION_BOOLEAN:
> + if (!(flags & OPT_SHORT) && p->opt)
> + return opterror(opt, "takes no value", flags);
> + if (flags & OPT_UNSET) {
> + *(int *)opt->value = 0;
> + } else {
> + (*(int *)opt->value)++;
> + }
> + return 0;
> +
> + case OPTION_STRING:
> + if (flags & OPT_UNSET) {
> + *(const char **)opt->value = (const char *)NULL;
> + } else {
> + if (!p->opt && p->argc < 1)
> + return opterror(opt, "requires a value", flags);
> + *(const char **)opt->value = get_arg(p);
> + }
> + return 0;
> +
> + case OPTION_INTEGER:
> + if (flags & OPT_UNSET) {
> + *(int *)opt->value = 0;
> + } else {
> + const char *s;
> + if (!p->opt && p->argc < 1)
> + return opterror(opt, "requires a value", flags);
> + *(int *)opt->value = strtol(*p->argv, (char **)&s, 10);
> + if (*s)
> + return opterror(opt, "expects a numerical value", flags);
> + }
> + return 0;
> +
> + default:
> + die("should not happen, someone must be hit on the forehead");
:-P
> +static int parse_long_opt(struct optparse_t *p, const char *arg,
> + struct option *options, int count)
> +{
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + const char *rest;
> + int flags = 0;
> +
> + if (!options[i].long_name)
> + continue;
> +
> + rest = skippfx(arg, options[i].long_name);
> + if (!rest && !strncmp(arg, "no-", 3)) {
> + rest = skippfx(arg + 3, options[i].long_name);
> + flags |= OPT_SHORT;
> + }
Would this not be more intuitive as
if (!prefixcmp(arg, "no-")) {
arg += 3;
flags |= OPT_UNSET;
}
rest = skip_prefix(arg, options[i].long_name);
Hm? (Note that I say UNSET, not SHORT... ;-)
> + if (!rest)
> + continue;
> + if (*rest) {
> + if (*rest != '=')
> + continue;
Is this really no error? For example, "git log
--decorate-walls-and-roofs" would not fail...
> +int parse_options(int argc, const char **argv,
> + struct option *options, int count,
> + const char * const usagestr[], int flags)
Please indent by the same amount.
> + if (arg[1] != '-') {
> + optp.opt = arg + 1;
> + do {
> + if (*optp.opt == 'h')
> + make_usage(usagestr, options, count);
How about calling this "usage_with_options()"? With that name I expected
make_usage() to return a strbuf.
> + if (!arg[2]) { /* "--" */
> + if (!(flags & OPT_COPY_DASHDASH))
> + optp.argc--, optp.argv++;
I would prefer this as
if (!(flags & OPT_COPY_DASHDASH)) {
optp.argc--;
optp.argv++;
}
While I'm at it: could you use "args" instead of "optp"? It is misleading
both in that it not only contains options (but other arguments, too) as in
that it is not a pointer (the trailing "p" is used as an indicator of that
very often, including git's source code).
In the same vein, OPT_COPY_DASHDASH should be named
PARSE_OPT_KEEP_DASHDASH.
> + if (opts->short_name) {
> + strbuf_addf(&sb, "-%c", opts->short_name);
> + }
> + if (opts->long_name) {
> + strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
> + opts->long_name);
> + }
Please lose the curly brackets.
> + if (sb.len - pos <= USAGE_OPTS_WIDTH) {
> + int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
> + strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
> + } else {
> + strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> + opts->help);
> + }
Same here. (And I'd also make sure that the lines are not that long.)
> diff --git a/parse-options.h b/parse-options.h
> new file mode 100644
> index 0000000..4b33d17
> --- /dev/null
> +++ b/parse-options.h
> @@ -0,0 +1,37 @@
> +#ifndef PARSE_OPTIONS_H
> +#define PARSE_OPTIONS_H
> +
> +enum option_type {
> + OPTION_BOOLEAN,
I know that I proposed "BOOLEAN", but actually, you use it more like an
"INCREMENTAL", right?
Other than that: I like it very much.
Ciao,
Dscho
^ permalink raw reply
* Re: [bug] gitk can not read history if diff color is enabled
From: Johannes Schindelin @ 2007-10-13 14:43 UTC (permalink / raw)
To: msharov; +Cc: git
In-Reply-To: <4710D3AA.8000502@softhome.net>
Hi,
On Sat, 13 Oct 2007, Mike Sharov wrote:
> Reproduced in git version 1.5.3.4.217.gbfc1, although it's been this way
> for a while. To see the bug, enable color diff with
>
> [diff]
> color
>
> and launch gitk on any repository (like the Linux kernel tree).
Thanks for the bug report.
This is fixed in http://hjemli.net/git/git/log/?h=q/mb/gitk/log--no-color
(which will be applied without any doubt once our king penguin comes
back).
Ciao,
Dscho
^ permalink raw reply
* Re: [bug] gitk can not read history if diff color is enabled
From: Michele Ballabio @ 2007-10-13 14:49 UTC (permalink / raw)
To: git, msharov
In-Reply-To: <4710D3AA.8000502@softhome.net>
On Saturday 13 October 2007, Mike Sharov wrote:
> Reproduced in git version 1.5.3.4.217.gbfc1, although it's been this way
> for a while. To see the bug, enable color diff with
>
> [diff]
> color
>
> and launch gitk on any repository (like the Linux kernel tree). The
> result is a dialog box "Can't parse git log output '\x1b[33mcommit" etc.
> qgit can still view the history without errors, so I guess it must be
> possible to somehow turn off color for a specific query.
A patch was posted, and is queued here:
http://hjemli.net/git/git/commit/?h=q/mb/gitk/log--no-color
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Wincent Colaiuta @ 2007-10-13 14:45 UTC (permalink / raw)
To: Dan Zwell
Cc: Git Mailing List, Jeff King, Jonathan del Strother,
Johannes Schindelin, Frank Lichtenheld
In-Reply-To: <471045DA.5050902@gmail.com>
El 13/10/2007, a las 6:13, Dan Zwell escribió:
> Dan Zwell<Color-add-interactive.patch.gz>
Based on a couple of the suggestions you've received I made a couple
of changes to your patch and given it a quick try-out. I'm no perl
hacker so there may be better ways.
- as per Jeff's suggestion, changed your print_ansi_color method,
modelling it after the print_color_ln and color_vprintf functions
defined in color.c; accepts a color, a string, and an optional
trailer (where if there is a newline you pass it as the trailer)
- as Johannes pointed out, "clear" and "reset" are not used
consistently even though the Term::ANSIColor documentation says that
they're the same, so settled on "clear"; although in any case, the
changes to the print_ansi_color function mean that it is now the only
site where clearing takes place
- changed the regex as suggested by Johannes, and a couple of others
that are used when splitting hunks
- used more explicit notation for regex as proposed by Frank
Took it for a basic spin here and seems to work. Didn't even think
about implementing user-settable colors.
Cheers,
Wincent
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index be68814..ae3d11e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,28 @@
use strict;
+my $use_color;
+my $color_config = qx(git config --get color.add-interactive);
+if ($color_config =~ /true/ || -t STDOUT && $color_config =~ /auto/) {
+ $use_color = "true";
+ require Term::ANSIColor;
+}
+
+sub print_ansi_color($$;$) {
+ my $color = shift;
+ my $string = shift;
+ my $trailer = shift;
+ if ($use_color) {
+ printf '%s%s%s', Term::ANSIColor::color($color), $string,
+ Term::ANSIColor::color('clear');
+ } else {
+ print $string;
+ }
+ if ($trailer) {
+ print $trailer;
+ }
+}
+
sub run_cmd_pipe {
if ($^O eq 'MSWin32') {
my @invalid = grep {m/[":*]/} @_;
@@ -175,7 +197,7 @@ sub list_and_choose {
if (!$opts->{LIST_FLAT}) {
print " ";
}
- print "$opts->{HEADER}\n";
+ print_ansi_color "bold", "$opts->{HEADER}", "\n";
}
for ($i = 0; $i < @stuff; $i++) {
my $chosen = $chosen[$i] ? '*' : ' ';
@@ -205,7 +227,7 @@ sub list_and_choose {
return if ($opts->{LIST_ONLY});
- print $opts->{PROMPT};
+ print_ansi_color "bold blue", $opts->{PROMPT};
if ($opts->{SINGLETON}) {
print "> ";
}
@@ -338,11 +360,17 @@ sub add_untracked_cmd {
sub parse_diff {
my ($path) = @_;
- my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+ my @diff;
+ if ($use_color) {
+ @diff = run_cmd_pipe(qw(git diff-files --color -p --), $path);
+ }
+ else {
+ @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+ }
my (@hunk) = { TEXT => [] };
for (@diff) {
- if (/^@@ /) {
+ if (/^[^-+ ]*@@ /) {
push @hunk, { TEXT => [] };
}
push @{$hunk[-1]{TEXT}}, $_;
@@ -360,7 +388,7 @@ sub hunk_splittable {
sub parse_hunk_header {
my ($line) = @_;
my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =
- $line =~ /^@@ -(\d+)(?:,(\d+)) \+(\d+)(?:,(\d+)) @@/;
+ $line =~ /^[^-+ ]*@@ -(\d+)(?:,(\d+)) \+(\d+)(?:,(\d+)) @@/;
return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
}
@@ -426,7 +454,7 @@ sub split_hunk {
}
push @{$this->{TEXT}}, $line;
$this->{ADDDEL}++;
- if ($line =~ /^-/) {
+ if ($line =~ /^[^-+ ]*-/) {
$this->{OCNT}++;
}
else {
@@ -483,7 +511,7 @@ sub merge_hunk {
$o_cnt = $n_cnt = 0;
for ($i = 1; $i < @{$prev->{TEXT}}; $i++) {
my $line = $prev->{TEXT}[$i];
- if ($line =~ /^\+/) {
+ if ($line =~ /^[^-+ ]*\+/) {
$n_cnt++;
push @line, $line;
next;
@@ -501,7 +529,7 @@ sub merge_hunk {
for ($i = 1; $i < @{$this->{TEXT}}; $i++) {
my $line = $this->{TEXT}[$i];
- if ($line =~ /^\+/) {
+ if ($line =~ /^[^-+ ]*\+/) {
$n_cnt++;
push @line, $line;
next;
@@ -544,7 +572,7 @@ sub coalesce_overlapping_hunks {
}
sub help_patch_cmd {
- print <<\EOF ;
+ my $help = <<\EOF ;
y - stage this hunk
n - do not stage this hunk
a - stage this and all the remaining hunks
@@ -555,6 +583,7 @@ k - leave this hunk undecided, see previous
undecided hunk
K - leave this hunk undecided, see previous hunk
s - split the current hunk into smaller hunks
EOF
+ print_ansi_color "blue", $_, "\n" foreach (split /[\r\n]/, $help);
}
sub patch_update_cmd {
@@ -619,7 +648,7 @@ sub patch_update_cmd {
for (@{$hunk[$ix]{TEXT}}) {
print;
}
- print "Stage this hunk [y/n/a/d$other/?]? ";
+ print_ansi_color "bold", "Stage this hunk [y/n/a/d$other/?]? ";
my $line = <STDIN>;
if ($line) {
if ($line =~ /^y/i) {
^ permalink raw reply related
* Re: [PATCH] Port builtin-add.c to use the new option parser.
From: Johannes Schindelin @ 2007-10-13 14:47 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, Junio C Hamano, Kristian Høgsberg
In-Reply-To: <1192282153-26684-3-git-send-email-madcoder@debian.org>
Hi,
On Sat, 13 Oct 2007, Pierre Habouzit wrote:
> +static struct option builtin_add_options[] = {
> + OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
> + OPT_BOOLEAN('n', NULL, &show_only, "dry-run"),
> + OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
> + OPT_BOOLEAN('v', NULL, &verbose, "be verbose"),
> + OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files that git already knows about"),
> + OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh stat() informations in the index"),
> +};
I see you terminate the list by a ",". How does this play with the option
parser?
Thinking about this more, I am reverting my stance on the ARRAY_SIZE()
issue. I think if you introduce a "OPTION_NONE = 0" in the enum, then
this single last comma should be enough.
In the same vein, you would not need the NULL in builtin_add_usage[],
right?
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC] CLI option parsing and usage generation for porcelains
From: Wincent Colaiuta @ 2007-10-13 14:53 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git
In-Reply-To: <1192282153-26684-1-git-send-email-madcoder@debian.org>
El 13/10/2007, a las 15:29, Pierre Habouzit escribió:
> The following series is open for comments, it's not 100% ready for
> inclusion IMHO, as some details may need to be sorted out first, and
> that I've not re-read the patches thoroughly yet. Though I uses the
> tip
> of that branch as my everyday git for 2 weeks or so without any
> noticeable issues.
Great to see two things:
- the simplification in the commands switched over to use the options
parser
- the improved readability and usefulness of the options help
Great work, Pierre! I'll take a closer look at this and trial it in
my local Git install for a while to see if any issues come up.
Wincent
^ permalink raw reply
* Re: [PATCH] Add a simple option parser.
From: Pierre Habouzit @ 2007-10-13 14:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0710131519510.25221@racer.site>
[-- Attachment #1: Type: text/plain, Size: 4865 bytes --]
On Sat, Oct 13, 2007 at 02:39:10PM +0000, Johannes Schindelin wrote:
> Hi,
>
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
>
> > Aggregation of single switches is allowed:
> > -rC0 is the same as -r -C 0 (supposing that -C wants an arg).
>
> I'd be more interested in "-rC 0" working... Is that supported, too?
yes it is.
> > +static inline const char *get_arg(struct optparse_t *p)
> > +{
> > + if (p->opt) {
> > + const char *res = p->opt;
> > + p->opt = NULL;
> > + return res;
> > + }
> > + p->argc--;
> > + return *++p->argv;
> > +}
>
> This is only used once; I wonder if it is really that more readable having
> this as a function in its own right.
it's used twice, and it makes the code more readable I believe.
> > +static inline const char *skippfx(const char *str, const char *prefix)
>
> Personally, I do not like abbreviations like that. They do not save that
> much screen estate (skip_prefix is only 4 characters longer, but much more
> readable). Same goes for "cnt" later.
Ack I'll fix that.
> > +static int parse_long_opt(struct optparse_t *p, const char *arg,
> > + struct option *options, int count)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < count; i++) {
> > + const char *rest;
> > + int flags = 0;
> > +
> > + if (!options[i].long_name)
> > + continue;
> > +
> > + rest = skippfx(arg, options[i].long_name);
> > + if (!rest && !strncmp(arg, "no-", 3)) {
> > + rest = skippfx(arg + 3, options[i].long_name);
> > + flags |= OPT_SHORT;
> > + }
>
> Would this not be more intuitive as
>
> if (!prefixcmp(arg, "no-")) {
> arg += 3;
> flags |= OPT_UNSET;
> }
> rest = skip_prefix(arg, options[i].long_name);
Yes, that can be done indeed, but the point is, we have sometimes
option whose long-name is "no-foo" (because it's what makes sense) but I
can rework that.
> Hm? (Note that I say UNSET, not SHORT... ;-)
fsck, good catch.
> > + if (!rest)
> > + continue;
> > + if (*rest) {
> > + if (*rest != '=')
> > + continue;
>
> Is this really no error? For example, "git log
> --decorate-walls-and-roofs" would not fail...
it would be an error, it will yield a "option not found".
> > +int parse_options(int argc, const char **argv,
> > + struct option *options, int count,
> > + const char * const usagestr[], int flags)
>
> Please indent by the same amount.
oops, stupid space vs. tab thing.
> > + if (arg[1] != '-') {
> > + optp.opt = arg + 1;
> > + do {
> > + if (*optp.opt == 'h')
> > + make_usage(usagestr, options, count);
>
> How about calling this "usage_with_options()"? With that name I expected
> make_usage() to return a strbuf.
will do.
> > + if (!arg[2]) { /* "--" */
> > + if (!(flags & OPT_COPY_DASHDASH))
> > + optp.argc--, optp.argv++;
>
> I would prefer this as
>
> if (!(flags & OPT_COPY_DASHDASH)) {
> optp.argc--;
> optp.argv++;
> }
>
> While I'm at it: could you use "args" instead of "optp"? It is misleading
> both in that it not only contains options (but other arguments, too) as in
> that it is not a pointer (the trailing "p" is used as an indicator of that
> very often, including git's source code).
okay.
> In the same vein, OPT_COPY_DASHDASH should be named
> PARSE_OPT_KEEP_DASHDASH.
okay.
>
> > + if (opts->short_name) {
> > + strbuf_addf(&sb, "-%c", opts->short_name);
> > + }
> > + if (opts->long_name) {
> > + strbuf_addf(&sb, opts->short_name ? ", --%s" : "--%s",
> > + opts->long_name);
> > + }
>
> Please lose the curly brackets.
>
> > + if (sb.len - pos <= USAGE_OPTS_WIDTH) {
> > + int pad = USAGE_OPTS_WIDTH - (sb.len - pos) + USAGE_GAP;
> > + strbuf_addf(&sb, "%*s%s\n", pad, "", opts->help);
> > + } else {
> > + strbuf_addf(&sb, "\n%*s%s\n", USAGE_OPTS_WIDTH + USAGE_GAP, "",
> > + opts->help);
> > + }
>
> Same here. (And I'd also make sure that the lines are not that long.)
okay.
>
> > diff --git a/parse-options.h b/parse-options.h
> > new file mode 100644
> > index 0000000..4b33d17
> > --- /dev/null
> > +++ b/parse-options.h
> > @@ -0,0 +1,37 @@
> > +#ifndef PARSE_OPTIONS_H
> > +#define PARSE_OPTIONS_H
> > +
> > +enum option_type {
> > + OPTION_BOOLEAN,
>
> I know that I proposed "BOOLEAN", but actually, you use it more like an
> "INCREMENTAL", right?
yes, I don't like _BOOLEAN either, I would have prefered _FLAG or sth
like that. INCREMENTAL is just too long.
> Other than that: I like it very much.
:P
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] Port builtin-add.c to use the new option parser.
From: Pierre Habouzit @ 2007-10-13 15:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Kristian Høgsberg
In-Reply-To: <Pine.LNX.4.64.0710131544030.25221@racer.site>
[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]
On Sat, Oct 13, 2007 at 02:47:20PM +0000, Johannes Schindelin wrote:
> Hi,
>
> On Sat, 13 Oct 2007, Pierre Habouzit wrote:
>
> > +static struct option builtin_add_options[] = {
> > + OPT_BOOLEAN('i', "interactive", &add_interactive, "interactive picking"),
> > + OPT_BOOLEAN('n', NULL, &show_only, "dry-run"),
> > + OPT_BOOLEAN('f', NULL, &ignored_too, "allow adding otherwise ignored files"),
> > + OPT_BOOLEAN('v', NULL, &verbose, "be verbose"),
> > + OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files that git already knows about"),
> > + OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh stat() informations in the index"),
> > +};
>
> I see you terminate the list by a ",". How does this play with the option
> parser?
>
> Thinking about this more, I am reverting my stance on the ARRAY_SIZE()
> issue. I think if you introduce a "OPTION_NONE = 0" in the enum, then
> this single last comma should be enough.
adding a trailing comma does not add a NULL after that, it's ignored,
you're confused.
┌─(17:00)────
└[artemis] cat a.c
#include <stdio.h>
int main(void) {
const char * const arr[] = { "1", "2", };
printf("%d\n", sizeof(arr) / sizeof(arr[0]));
return 0;
};
┌─(17:00)────
└[artemis] ./a
2
Very few compilers do not grok trailing commas, I always put them
because it avoids spurious diffs for nothing, and that you can reorder
lines easily too.
Note that I don't really like using ARRAY_SIZE either, I kept it that
way, but my taste would rather be to have an "empty" option, and
explicitely mark the end of the array.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Jean-Luc Herren @ 2007-10-13 16:38 UTC (permalink / raw)
To: Wincent Colaiuta, git
In-Reply-To: <19271E58-5C4F-41AF-8F9D-F114F36A34AC@wincent.com>
Hi!
I really like the idea of colorizing git add -i, especially the
prompt. Here are my two cents.
Wincent Colaiuta wrote:
> +sub print_ansi_color($$;$) {
> + my $color = shift;
> + my $string = shift;
> + my $trailer = shift;
None of the other subs in this file have a prototype, so for
consistency I'd suggest to not add it on this function either.
However maybe a patch that adds it to all subs would be welcome.
(I wouldn't see the necessity though.)
And the common way of getting the arguments is reading @_ (see all
other subs in the file). So maybe instead write:
[...]
sub print_ansi_color {
my ($color, $string, $trailer) = @_;
[...]
> + if ($use_color) {
> + printf '%s%s%s', Term::ANSIColor::color($color), $string,
> + Term::ANSIColor::color('clear');
> + } else {
Why use printf when you could directly use print here? It's only
used for concatenating.
> + if ($trailer) {
> + print $trailer;
> + }
This will fail to print $trailer when $trailer happens to be a
string that evaluates to false in bool context, like '0'. Write
this as:
if (defined $trailer) {
print $trailer;
}
IMHO, parsing the output of 'git diff-files --color' is a very bad
idea and it makes all regexes uglier and more difficult to read.
You're much better off recolorizing it yourself, which makes it a
more localized change. Especially, I don't think that you have
any guarantee that escape sequences won't ever contain the
characters '+', '-' or ' ' (space), which would break your code on
lines like this:
> + if ($line =~ /^[^-+ ]*\+/) {
Finally -- and this might be just my eyes -- blue is a very nice
color, but it looks a bit too dark on black background. Maybe
choose a default color that looks reasonable on black *and* white
background.
Cheers,
jlh
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Johannes Schindelin @ 2007-10-13 16:38 UTC (permalink / raw)
To: Wincent Colaiuta
Cc: Dan Zwell, Git Mailing List, Jeff King, Jonathan del Strother,
Frank Lichtenheld
In-Reply-To: <19271E58-5C4F-41AF-8F9D-F114F36A34AC@wincent.com>
Hi,
On Sat, 13 Oct 2007, Wincent Colaiuta wrote:
> Didn't even think about implementing user-settable colors.
FWIW, this is what Documentation/config.txt has to say about it:
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 dubious
whitespace). The values of these variables may be specified as
in color.branch.<slot>.
Hth,
Dscho
^ permalink raw reply
* Re: push fails with unexpected 'matches more than one'
From: Steffen Prohaska @ 2007-10-13 16:51 UTC (permalink / raw)
To: Git Mailing List; +Cc: Shawn O. Pearce
In-Reply-To: <20071013032100.GK27899@spearce.org>
On Oct 13, 2007, at 5:21 AM, Shawn O. Pearce wrote:
> Steffen Prohaska <prohaska@zib.de> wrote:
>> I read carefully through the documentation of git-send-pack and
>> git-rev-parse. The current implementation of git-send-pack is in line
>> with the documented behaviour, as is the implementation of git-rev-
>> parse.
>>
>> So formally everything is correct.
>>
>> But it is completely against my expectation that git-push <remote>
>> <head>
>> can successfully resolve a <head> that git-rev-parse fails to
>> parse. I
>> understand that refs are not revs ;). But nonetheless, I'd expect
>> that a
>> local ref that cannot be parsed by git-rev-parse should also fail
>> to be
>> pushed by git-send-pack. I didn't expect that git-send-pack would
>> locate
>> <head> as someprefix/<head>.
>>
>> Why is my expectation wrong?
>> Or is the current specification of git-send-pack's ref parsing wrong?
>
> I think its a bug. But I didn't write the original code.
>
> Meaning I think what happened here was someone wanted to enable
> git-send-pack to match "master" here with "refs/heads/master" on
> the remote side. One easy way to do that was to see if any ref
> ended with "/master", as that was what the ref here was called.
>
> Way back when that code was written most Git repositories probably
> only ever had that one branch anyway, or maybe two (refs/heads/master
> and refs/heads/origin) so matching the trailing suffix never came
> up as a bug. Nobody ever had two refs that could possibly match.
>
> Then the documentation got expanded to actually document the behavior
> that git-send-pack implemented. Unfortunately that codified the
> bug as documented behavior.
>
>
> So I agree with you Steffen, this is a bug in send-pack, and I run
> up against it every once in a while. I've specifically told my
> coworkers "NEVER, EVER, EVER, create a branch called 'master' that
> isn't exactly refs/heads/master OR ELSE I WILL COME BEAT YOU WITH A
> CLUE STICK". They still create "refs/heads/experiments/master".
> *sigh*.
>
> I think we should fix it. Anyone that is relying on "git push
> $url master" to resolve to "refs/heads/experimental/master" on the
> remote side is already playing with fire. But Junio is (rightfully
> so) very conservative and doesn't like to break a user's scripts.
> We may not be able to fix this until Git 1.6.
I'm working on this and will send patches tomorrow.
Steffen
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Wincent Colaiuta @ 2007-10-13 17:14 UTC (permalink / raw)
To: Jean-Luc Herren; +Cc: git
In-Reply-To: <4710F47D.2070306@gmx.ch>
El 13/10/2007, a las 18:38, Jean-Luc Herren escribió:
> Here are my two cents.
>
> Wincent Colaiuta wrote:
>> +sub print_ansi_color($$;$) {
>> + my $color = shift;
>> + my $string = shift;
>> + my $trailer = shift;
>
> None of the other subs in this file have a prototype, so for
> consistency I'd suggest to not add it on this function either.
> However maybe a patch that adds it to all subs would be welcome.
> (I wouldn't see the necessity though.)
Yes, I saw that the other functions didn't use prototypes and I agree
that consistency would be a good thing. I liked the idea of it in
this case because it makes explicit the fact that the function takes
two params plus a third, optional one. So definitely not necessary,
but a preference of mine. In any case, a change to all the other
functions is a question for a separate patch.
> And the common way of getting the arguments is reading @_ (see all
> other subs in the file). So maybe instead write:
>
> [...]
> sub print_ansi_color {
> my ($color, $string, $trailer) = @_;
> [...]
Yes, I actually did write it that way first but then my doubts about
Perl made me write it the longer way; but if they are equivalent then
I prefer the shorter way.
>> + if ($use_color) {
>> + printf '%s%s%s', Term::ANSIColor::color($color), $string,
>> + Term::ANSIColor::color('clear');
>> + } else {
>
> Why use printf when you could directly use print here? It's only
> used for concatenating.
True. I had may brain in the C-world.
>> + if ($trailer) {
>> + print $trailer;
>> + }
>
> This will fail to print $trailer when $trailer happens to be a
> string that evaluates to false in bool context, like '0'. Write
> this as:
>
> if (defined $trailer) {
> print $trailer;
> }
Again, I actually wrote it that way the first time, and then changed
it, this time because I thought they were the same. Like I said, not
a perl hacker.
> IMHO, parsing the output of 'git diff-files --color' is a very bad
> idea and it makes all regexes uglier and more difficult to read.
> You're much better off recolorizing it yourself, which makes it a
> more localized change.
You're probably right, although it is also duplicating the work
that's already done elsewhere. In general I favor making the simplest
change that would work, and tweaking a few of the regexes did look
simpler than re-implementing the colorization logic.
But the approach you suggest might be more robust, perhaps, seeing as
there's not much to the diff output. As far as I can tell there are
really only five or six different things to look for, and they'd be
fairly easy to catch:
- lines beginning with "@@ " (hunk headers)
- lines beginning with "+" (insertions)
- lines beginning with "-" (deletions)
- lines beginning with " " (context lines, no color)
- lines beginning with "\" (things like "\ No newline at end of
file", again, no color)
- everything else; ie. the diff header stuff (eg "diff --git a/foo b/
foo")
The only special cases seem to be the "+++" and "---" lines in the
header, which look like insertions and deletions when they're not.
Trickier would be the highlighting of dubious whitespace, and that's
when it starts to sound like re-inventing the wheel and duplicating
the logic for the detection that's defined elsewhere (possibly in
diff-lib.c? haven't found the exact spot yet).
> Especially, I don't think that you have
> any guarantee that escape sequences won't ever contain the
> characters '+', '-' or ' ' (space)
Yes, that was one of the things I didn't like about the sloppy
regexes. I couldn't really make them any stricter though because I
wasn't confident about the range of possible characters that might be
included in the escape sequences.
> Finally -- and this might be just my eyes -- blue is a very nice
> color, but it looks a bit too dark on black background. Maybe
> choose a default color that looks reasonable on black *and* white
> background.
Yeah, well I didn't choose the colours and I didn't really want to
get into it. Before being considered for inclusion a patch like this
would need to tap in to the existing config settings for color.diff
and color.diff.<slot> anyway...
Wincent
^ permalink raw reply
* Re: [PATCH] Color support added to git-add--interactive.
From: Jeff King @ 2007-10-13 17:27 UTC (permalink / raw)
To: Wincent Colaiuta
Cc: Dan Zwell, Git Mailing List, Jonathan del Strother,
Johannes Schindelin, Frank Lichtenheld
In-Reply-To: <19271E58-5C4F-41AF-8F9D-F114F36A34AC@wincent.com>
On Sat, Oct 13, 2007 at 04:45:41PM +0200, Wincent Colaiuta wrote:
> - as Johannes pointed out, "clear" and "reset" are not used consistently
> even though the Term::ANSIColor documentation says that they're the same, so
> settled on "clear"; although in any case, the changes to the
> print_ansi_color function mean that it is now the only site where clearing
> takes place
Please use "reset", as that is the term used by the C color code.
> - changed the regex as suggested by Johannes, and a couple of others that
> are used when splitting hunks
I believe there are other places where the diff output is parsed, and
the colors will mess that up, too (e.g., split_hunk). All of those
regexes need to be changed, too. I am a bit concerned that we are
putting intimate knowledge of the colorization scheme here. As much as
it pains me to have two diff colorizers, I wonder if that would be a
better solution than having a diff colorizer, and a colorized diff
parser.
-Peff
^ permalink raw reply
* [PATCH] gitk: Added support for OS X mouse wheel
From: Jonathan del Strother @ 2007-10-13 17:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Väinö Järvelä
(Väinö Järvelä supplied this patch a while ago for 1.5.2. It no longer
applied cleanly, so I'm reposting it.)
MacBook doesn't seem to recognize MouseRelease-4 and -5 events, at all.
So i added a support for the MouseWheel event, which i limited to Tcl/tk
aqua, as i couldn't test it neither on Linux or Windows. Tcl/tk needs to
be updated from the version that is shipped with OS X 10.4 Tiger, for
this patch to work.
Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
gitk | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/gitk b/gitk
index 300fdce..9b3e627 100755
--- a/gitk
+++ b/gitk
@@ -838,11 +838,17 @@ proc makewindow {} {
bindall <1> {selcanvline %W %x %y}
#bindall <B1-Motion> {selcanvline %W %x %y}
if {[tk windowingsystem] == "win32"} {
- bind . <MouseWheel> { windows_mousewheel_redirector %W %X %Y %D }
- bind $ctext <MouseWheel> { windows_mousewheel_redirector %W %X %Y %
D ; break }
+ bind . <MouseWheel> { windows_mousewheel_redirector %W %X %Y
%D }
+ bind $ctext <MouseWheel> { windows_mousewheel_redirector %W %
X %Y %D ; break }
} else {
- bindall <ButtonRelease-4> "allcanvs yview scroll -5 units"
- bindall <ButtonRelease-5> "allcanvs yview scroll 5 units"
+ bindall <ButtonRelease-4> "allcanvs yview scroll -5 units"
+ bindall <ButtonRelease-5> "allcanvs yview scroll 5 units"
+ if {[tk windowingsystem] eq "aqua"} {
+ bindall <MouseWheel> {
+ set delta [expr {- (%D)}]
+ allcanvs yview scroll $delta units
+ }
+ }
}
bindall <2> "canvscan mark %W %x %y"
bindall <B2-Motion> "canvscan dragto %W %x %y"
--
1.5.3.1
^ permalink raw reply related
* Re: [PATCH] Color support added to git-add--interactive.
From: Jeff King @ 2007-10-13 17:51 UTC (permalink / raw)
To: Wincent Colaiuta
Cc: Dan Zwell, Git Mailing List, Jonathan del Strother,
Johannes Schindelin, Frank Lichtenheld
In-Reply-To: <20071013172745.GA2624@coredump.intra.peff.net>
On Sat, Oct 13, 2007 at 01:27:45PM -0400, Jeff King wrote:
> I believe there are other places where the diff output is parsed, and
> the colors will mess that up, too (e.g., split_hunk). All of those
Oops, I see you actually dealt with those already (I just responded to
your cover letter first). Though I am still concerned about the
robustness of the re-parsing scheme.
-Peff
^ permalink raw reply
* Re: Git User's Survey 2007 unfinished summary continued
From: Andreas Ericsson @ 2007-10-13 18:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narebski, git
In-Reply-To: <Pine.LNX.4.64.0710130130380.25221@racer.site>
Johannes Schindelin wrote:
>
> At the GSoC mentor summit, I encountered a rather different stance: people
> did not _know_ what distributed SCM means, and were rather afraid of the
> concept. Some of them seemed to fight changing their known procedures
> tooth and nail. Which is fine by me (I don't have to force anybody to
> use git, thankyouverymuch).
>
I recently attended a fairly large opensource conference in germany, where
companies that somehow do business around the opensource network
monitoring tool named Nagios gather and drink themselves and each other
into insensibility once a year. I ran into much the same issues there, even though
it would have made all our lives a whole lot easier if we could pull patches from
each others repositories.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: [PATCH] Fixing path quoting issues
From: Jonathan del Strother @ 2007-10-13 18:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Junio C Hamano
In-Reply-To: <470DC05A.8020209@viscovery.net>
On 11 Oct 2007, at 07:19, Johannes Sixt wrote:
>> - git-commit -F msg -m amending ."
>> + git-commit -F msg -m amending ."
>
> You fix whitespace...
>
>> test_expect_success \
>> - "using message from other commit" \
>> - "git-commit -C HEAD^ ."
>> + "using message from other commit" \
>> + "git-commit -C HEAD^ ."
>
> ... and you break it. More of these follow. Don't do that, it makes
> patch review unnecessarily hard.
I'm just preparing to release this patch... was that "don't break
whitespace", or "don't try to fix whitespace in a patch that's has
nothing to do with whitespacing-fixing" ?
And while I'm here - tabs are preferred, are they? There seem to be
a mixture of tabs & 4 space indentation.
^ 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