* Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer
From: Junio C Hamano @ 2012-02-14 3:28 UTC (permalink / raw)
To: Heiko Voigt; +Cc: git, Fredrik Gustafsson, Jens Lehmann
In-Reply-To: <20120213092900.GC15585@t1405.greatnet.de>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> This allows us to tell the user which submodules have not been pushed.
> Additionally this is helpful when we want to automatically try to push
> submodules that have not been pushed.
Makes sense.
> diff --git a/submodule.c b/submodule.c
> index 645ff5d..3c714c2 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,
> void *data)
> {
> int i;
> - int *needs_pushing = data;
> + struct string_list *needs_pushing = data;
>
> for (i = 0; i < q->nr; i++) {
> struct diff_filepair *p = q->queue[i];
> if (!S_ISGITLINK(p->two->mode))
> continue;
> if (submodule_needs_pushing(p->two->path, p->two->sha1)) {
> - *needs_pushing = 1;
> - break;
> + if (!string_list_has_string(needs_pushing, p->two->path))
> + string_list_insert(needs_pushing, p->two->path);
Does string_list API have "look for this and insert if it doesn't exist
but otherwise don't do anything"? Running get_entry_index() to answer
has_string() once and then calling it again to find where to insert to
respond to insert() looks a bit wasteful.
Just wondering.
> }
> }
> }
>
> -
> -static void commit_need_pushing(struct commit *commit, int *needs_pushing)
> +static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)
> {
> struct rev_info rev;
>
> @@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)
> diff_tree_combined_merge(commit, 1, &rev);
> }
>
> -int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
> +int check_submodule_needs_pushing(unsigned char new_sha1[20],
> + const char *remotes_name, struct string_list *needs_pushing)
> {
> struct rev_info rev;
> struct commit *commit;
> const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
> int argc = ARRAY_SIZE(argv) - 1;
> char *sha1_copy;
> - int needs_pushing = 0;
> +
> struct strbuf remotes_arg = STRBUF_INIT;
>
> strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
> @@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
> if (prepare_revision_walk(&rev))
> die("revision walk setup failed");
>
> - while ((commit = get_revision(&rev)) && !needs_pushing)
> - commit_need_pushing(commit, &needs_pushing);
> + while ((commit = get_revision(&rev)) != NULL)
> + commit_need_pushing(commit, needs_pushing);
Now the helper function to find list of submodules that need pushing given
one commit starting to look more and more misnamed. It used to be "learn
if something needs pushing", but now it is "find what needs pushing".
Can somebody think of a good adjective to describe a submodule (or a set
of submodules) in this state, so that we can name this helper function
find_blue_submodules(), if the adjective were "blue"?
"Unpushed" submodule is the word used in the later part of the patch.
^ permalink raw reply
* Re: [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially
From: Junio C Hamano @ 2012-02-14 1:33 UTC (permalink / raw)
To: Heiko Voigt; +Cc: git, Fredrik Gustafsson, Jens Lehmann
In-Reply-To: <20120213092730.GB15585@t1405.greatnet.de>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> Previously it was not possible to iterate revisions twice using the
> revision walking api. We add a reset_revision_walk() which clears the
> used flags. This allows us to do multiple sequencial revision walks.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
I am kind of surprised that this is already its 5th round.
> diff --git a/object.c b/object.c
> index 6b06297..6291ce9 100644
> --- a/object.c
> +++ b/object.c
> @@ -275,3 +275,14 @@ void object_array_remove_duplicates(struct object_array *array)
> array->nr = dst;
> }
> }
> +
> +void clear_object_flags(unsigned flags)
> +{
> + int i;
> + struct object *obj;
> +
> + for (i=0; i < obj_hash_size; i++) {
> + if ((obj = obj_hash[i]) && obj->flags & flags)
> + obj->flags &= ~flags;
> + }
> +}
Minimally,
void clear_object_flags(unsigned flags)
{
int i;
for (i = 0; i < obj_hash_size; i++) {
struct object *obj = obj_hash[i];
if (obj && (obj->flags & flags))
obj->flags &= ~flags;
}
}
I am not sure if the "If there is any bit set we care about, drop them"
buys us anything, though.
> diff --git a/revision.c b/revision.c
> index c97d834..77ce6bd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2061,6 +2061,11 @@ static void set_children(struct rev_info *revs)
> }
> }
>
> +void reset_revision_walk()
void reset_revision_walk(void)
> +{
> + clear_object_flags(SEEN | ADDED | SHOWN);
> +}
But is this really the right API? After a particular program finishes
using the revision walker, wouldn't it want to clear both the set of these
standard flag bits used by the traversal machinery, as well as whatever
program specific bits it used to mark the objects with?
> diff --git a/revision.h b/revision.h
> index b8e9223..3535733 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
> ...
> +extern void reset_revision_walk();
Likewise, "extern void reset_revision_walk(void);".
> diff --git a/submodule.c b/submodule.c
> index 9a28060..645ff5d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
> while ((commit = get_revision(&rev)) && !needs_pushing)
> commit_need_pushing(commit, &needs_pushing);
>
> + reset_revision_walk();
> free(sha1_copy);
> strbuf_release(&remotes_arg);
>
> @@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,
> if (in_merge_bases(b, &commit, 1))
> add_object_array(o, NULL, &merges);
> }
> + reset_revision_walk();
>
> /* Now we've got all merges that contain a and b. Prune all
> * merges that contain another found merge and save them in
These two hunk look like a *BUGFIX* to me (certainly it does not look like
this is an addition of any new feature).
What bug does this fix, and how is the current submodule code broken
without this patch? Can you describe the problem in the log message, and
add a test to demonstrate the existing breakage?
^ permalink raw reply
* Re: [PATCH] diff-highlight: Work for multiline changes too
From: Junio C Hamano @ 2012-02-14 1:19 UTC (permalink / raw)
To: Jeff King; +Cc: Michał Kiedrowicz, git
In-Reply-To: <20120214002209.GA23171@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I chose reverse because I like the way it looks, and because it should
> Just Work if people have selected alternate colors (I never dreamed
> somebody would use "reverse" all the time, as I find it horribly ugly.
> But to each his own).
I also find it ugly, but I am on black-letters on white background window,
and I do not see my terminal's red very well, so it is hard to tell the
old from the context if I use the "diff.color.old=red" default; that is
the primary reason for my setting.
^ permalink raw reply
* git-p4 useclientspec broken?
From: Laurent Charrière @ 2012-02-14 0:47 UTC (permalink / raw)
To: git
Since I've upgraded to 1.7.9 (on OS X Lion, FWIW), git-p4 submit fails
to apply any patches if I use useclientspec=true when cloning.
My p4 client is as follows:
Client: malibu
(...)
Root: /Users/lcharriere/Documents/Perforce/all
(...)
View:
//sandbox/... //malibu/sandbox/...
//depot/... //malibu/depot/...
Sequence of steps to reproduce:
$ git p4 clone //sandbox/lcharriere/foo --use-client-spec
$ cd foo && find .
./.git
(...)
./sandbox/lcharriere/foo/.gitignore
./sandbox/lcharriere/foo/foo.py
-- This is new behavior to me, BTW. Previously, I would have seen
./.git
(...)
./.gitignore
./foo.py
$ cat "test" >> sandbox/lcharriere/foo/.gitignore
$ git commit -a -m "test"
git commit -a -m "test"
[master 7398144] test
1 files changed, 1 insertions(+), 0 deletions(-)
$ git p4 submit
Perforce checkout for depot path //sandbox/lcharriere/foo/ located at
/Users/lcharriere/Documents/Perforce/all/sandbox/lcharriere/foo/
Synchronizing p4 checkout...
... - file(s) up-to-date.
Applying 739814457a8faa84dc0bddd830f671569576b177 test
sandbox/lcharriere/foo/.gitignore - file(s) not on client.
error: sandbox/lcharriere/foo/.gitignore: No such file or directory
Unfortunately applying the change failed!
What do you want to do?
[s]kip this patch / [a]pply the patch forcibly and with .rej files /
[w]rite the patch to a file (patch.txt)
I tried to follow what's going on with pdb:
* self.depotPath is //sandbox/lcharriere/foo, so git-p4 chdir's to
/Users/lcharriere/Documents/Perforce/all/sandbox/lcharriere/foo/
* In P4Submit.applyCommit, line 926 is:
p4_edit(path)
At this point path is 'sandbox/lcharriere/foo/.gitignore'
I'm guessing this is why the p4 executable doesn't find it. The path
should be .gitignore. Is it possible that the new behavior I mentioned
above of reproducing the depot hierarchy when useclientspec is true is
having unintended side effects, or is a bug?
^ permalink raw reply
* Re: [PATCH v3] diff --stat: use the full terminal width
From: Junio C Hamano @ 2012-02-14 1:08 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, pclouds, Michael J Gruber
In-Reply-To: <1329057019-29983-1-git-send-email-zbyszek@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> Use as many columns as necessary for filenames, as few columns as
> necessary for change counts, and up to 40 columns for the histogram.
>
> Some projects (especially in Java), have long filename paths, with
> nested directories or long individual filenames. When files are
> renamed, the stat output can be almost useless. If the middle part
s/the stat output/the name part in &/;
> between { and } is long (because the file was moved to a completely
> different directory), then most of the path would be truncated.
>
> It makes sense to detect and use the full terminal width and display
> full filenames if possible.
>
> If commits changing a lot of lines are displayed in a wide terminal
> window (200 or more columns), and the +- graph would use the full
> width, the output would look bad. Messages wrapped to about 80
> columns would be interspersed with very long +- lines. It makes
> sense to limit the width of the histogram to a fixed value, even if more
I do not think the graph ++++++--- part is "histogram", which is a name
for a specific type of graph that depicts a distribution of data.
We show the number of lines changed in a graph. Unless people come up
with a better name, I would suggest calling it just "the graph part", or
simply "the graph", throughout the patch.
> columns are available. This fixed value is subjectively hard-coded to
> be 40 columns, which seems to work well for git.git and linux-2.6.git and
> some other repositories.
>
> If there isn't enough columns to print both the filename and the histogram,
> at least 5/8 of available space is devoted to filenames. On a standard 80 column
> terminal, or if not connected to a terminal and using the default of 80 columns,
> this gives the same partition as before.
>
> Number of columns required for change counts is computed based on
> the maximum number of changed lines. This means that usually a few more
> columns will be available for the filenames and the histogram.
>
> Tests are added for various combinations of long filename and big change
> count and ways to specify widths.
>
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
> ...
> diff --git a/diff.c b/diff.c
> index 7e15426..7abcbe9 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1327,7 +1327,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> int i, len, add, del, adds = 0, dels = 0;
> uintmax_t max_change = 0, max_len = 0;
> int total_files = data->nr;
> - int width, name_width, count;
> + int width, name_width, graph_width, number_width, count;
> const char *reset, *add_c, *del_c;
> const char *line_prefix = "";
> int extra_shown = 0;
> @@ -1341,25 +1341,13 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> line_prefix = msg->buf;
> }
>
> - width = options->stat_width ? options->stat_width : 80;
> - name_width = options->stat_name_width ? options->stat_name_width : 50;
> - count = options->stat_count ? options->stat_count : data->nr;
It was somewhat distracting that you moved this "count =" below, which I
do not think was necessary.
> - /* Sanity: give at least 5 columns to the graph,
> - * but leave at least 10 columns for the name.
> - */
> - if (width < 25)
> - width = 25;
> - if (name_width < 10)
> - name_width = 10;
> - else if (width < name_width + 15)
> - name_width = width - 15;
Removal of this sanity check is fine as long as sanity is kept by the new
code in the later part. This was primarily so that people won't specify
impossible values (e.g. what happens when name_width that is wider than
the total width is given).
> ...
> + count = options->stat_count ? options->stat_count : data->nr;
> +
> @@ -1380,19 +1368,63 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> }
> count = i; /* min(count, data->nr) */
>
> - /* Compute the width of the graph part;
> - * 10 is for one blank at the beginning of the line plus
> - * " | count " between the name and the graph.
> + /* We have width = stat_width or term_columns() columns total.
Just a style, but in the more recent part of the codebase,
/*
* We have width = ....
is preferred.
> + * We want a maximum of min(max_len, stat_name_width) for the name part.
> + * We want a maximum of min(max_change, 40) for the +- part.
> + * We also need 1 for " " and 4 + decimal_width(max_change)
> + * for " | NNNN " and one for " " at the end, altogether
> + * 6 + decimal_width(max_change).
The math looks correct but 'one for " " at the end' sounds as if you are
printing a SP, but I think we simply avoid printing anything, so perhaps
rewrite it to "we leave one column at the end" or something.
> + * If there's not enough space, we will use stat_name_width
> + * or 5/8*width for filename, and the rest for constant
"A or B for filename"---unclear how it picks between A or B.
"A or B, whichever is shorter, for filename", perhaps?
> + * elements + histogram, but no more than 40 for the histogram.
> + * (5/8 gives 50 for filename and 30 for constant parts and
> + * histogram for the standard terminal size).
> *
> - * From here on, name_width is the width of the name area,
> - * and width is the width of the graph area.
> + * In other words: stat_width limits the maximum width, and
> + * stat_name_width fixes the maximum width of the filename,
> + * and is also used to divide available columns if there
> + * aren't enough.
> */
> - name_width = (name_width < max_len) ? name_width : max_len;
> - if (width < (name_width + 10) + max_change)
> - width = width - (name_width + 10);
> - else
> - width = max_change;
> + width = options->stat_width ? options->stat_width : term_columns();
> + number_width = decimal_width(max_change);
> + /* first sizes that are wanted */
Missing verb; "first, compute sizes that are ..."?
> + graph_width = max_change < 40 ? max_change : 40;
> + name_width = (options->stat_name_width > 0 &&
> + options->stat_name_width < max_len) ?
> + options->stat_name_width : max_len;
mental note: name_width can be limited to max_len, and graph_width may be
quite small when max_change is small. The total graph may be much smaller
than the terminal width in such a case (and it is not a wasted space on
the right hand side of the terminal).
> + /* sanity: guarantee a minimum and maximum width */
> + if (width < 25)
> + width = 25;
> +
> + if (name_width + number_width + 6 + graph_width > width) {
> + if (graph_width > width * 3/8 - number_width - 6)
> + graph_width = width * 3/8 - number_width - 6;
> + if (graph_width > 40)
> + graph_width = 40;
> + if (name_width > width - number_width - 6 - graph_width)
> + name_width = width - number_width - 6 - graph_width;
> + else
> + graph_width = width - number_width - 6 - name_width;
> + }
>
> + /* More sanity: give at least 5 columns to the graph,
Hrm, having to have a separate "More sanity" is ugly, and you seem to
already know it "This should already be satisfied...".
Can the logic above be tightened to make this "mopping up" unnecessary?
> + * but leave at least 10 columns for the name.
> + *
> + * This should already be satisfied, unless max_change is
> + * really huge. If the window is extemely narrow, this might
> + * overflow available columns.
> + */
> + if (name_width < 10 && max_len >= 10)
> + name_width = 10;
The logic up to this point uses the same "25 <= width, 10 <= name_width"
as the original to ensure that graph and the fixed part can use at least
15 columns. And then you do this:
> + if (graph_width < 5 && max_change >= 5)
> + graph_width = 5;
which means you are allocating 10 columns for fixed part. In the
original, it was OK to give fixed number of columns for fixed part like
this, as it always gave fixed 5 columns to show the number. Don't you
need to adjust that 10 depending on the decimal_width(max_change), now
your number_width flexes?
This patch also breaks many existing tests that need to be adjusted for
the change to use decimal_width(max_change), which I do not care to fix up
for you. Perhaps that part of the change needs to be split out into a
separate patch.
Among the tests it breaks is t1200-tutorial.sh, which means that the
tutorial document that has illustration of the sample output also needs to
be updated before the final round.
Thanks.
^ permalink raw reply
* Re: [PATCH] diff-highlight: Work for multiline changes too
From: Jeff King @ 2012-02-14 0:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michał Kiedrowicz, git
In-Reply-To: <7v8vk6b7pq.fsf@alter.siamese.dyndns.org>
On Mon, Feb 13, 2012 at 04:05:05PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I ended up pulling your changes out into a few distinct commits. That
> > made it easier for me to review and understand what was going on (and
> > hopefully ditto for other reviewers, or people who end up bisecting or
> > reading the log later). I'll post that series in a moment.
>
> This is all nice. As I am lazy and this is a long neglected contrib/
> material I didn't even know it existed ;-), I am tempted to apply them
> directly on top of 'master'.
Yeah, given the contrib nature, I'm comfortable just applying. I wanted
to wait on the attribution question from Michał before sending a final
version, though.
> This shows the first hunk of your "diff-highlight: refactor to prepare for
> multi-line hunks" like this to me, by the way.
>
> @@ -23,7 +23,7 @@ while (<>) {
> $window[2] =~ /^$COLOR*\+/ &&
> $window[3] !~ /^$COLOR*\+/) {
> print shift @window;
> {- show_pair}(shift @window, shift @window);
> + show_{hunk}(shift @window, shift @window);
> }
> else {
> print shift @window;
>
> Is this intended, or is setting "diff.color.old = red reverse" not
> supported (without the custom configuration, the leading blank on the old
> line is not highlighted)?
No, it's not intended, and should be:
- show-{pair}...
+ show+{hunk}...
(and appears that way with my config). I suspect it is because the
default highlight color is a subset of your "old" configured color. It
is ANSI "reverse video", but sadly that does not work as a toggle (i.e.,
it does not reverse your reverse, but simply is a no-op).
I chose reverse because I like the way it looks, and because it should
Just Work if people have selected alternate colors (I never dreamed
somebody would use "reverse" all the time, as I find it horribly ugly.
But to each his own). We could read the highlight color from
diff.highlight{Old,New}. That would also let people highlight in purple
or something if they care.
-Peff
^ permalink raw reply
* diff --stat
From: Junio C Hamano @ 2012-02-14 0:11 UTC (permalink / raw)
To: git
Hrm, what is wrong with this picture?
$ git -c diff.color.old=red show --format='%s' --stat
Merge branch 'jk/diff-highlight' into pu
contrib/diff-highlight/README | 109 ++++++++++++++++++++++++++++++--
contrib/diff-highlight/diff-highlight | 109 ++++++++++++++++++++++++---------
2 files changed, 181 insertions(+), 37 deletions(-)
They both have 109 lines changed but the end of the graph lines do not
coincide...
^ permalink raw reply
* Re: [PATCH] diff-highlight: Work for multiline changes too
From: Junio C Hamano @ 2012-02-14 0:05 UTC (permalink / raw)
To: Jeff King; +Cc: Michał Kiedrowicz, git
In-Reply-To: <20120213222702.GA19393@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I ended up pulling your changes out into a few distinct commits. That
> made it easier for me to review and understand what was going on (and
> hopefully ditto for other reviewers, or people who end up bisecting or
> reading the log later). I'll post that series in a moment.
This is all nice. As I am lazy and this is a long neglected contrib/
material I didn't even know it existed ;-), I am tempted to apply them
directly on top of 'master'.
This shows the first hunk of your "diff-highlight: refactor to prepare for
multi-line hunks" like this to me, by the way.
@@ -23,7 +23,7 @@ while (<>) {
$window[2] =~ /^$COLOR*\+/ &&
$window[3] !~ /^$COLOR*\+/) {
print shift @window;
{- show_pair}(shift @window, shift @window);
+ show_{hunk}(shift @window, shift @window);
}
else {
print shift @window;
Is this intended, or is setting "diff.color.old = red reverse" not
supported (without the custom configuration, the leading blank on the old
line is not highlighted)?
^ permalink raw reply
* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Pete Wyckoff @ 2012-02-13 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Luke Diamand, git, Vitor Antunes
In-Reply-To: <7vwr7rgsuw.fsf@alter.siamese.dyndns.org>
gitster@pobox.com wrote on Sun, 12 Feb 2012 22:17 -0800:
> Luke Diamand <luke@diamand.org> writes:
>
> > On 12/02/12 18:13, Pete Wyckoff wrote:
> >> The git-p4 code is in a single python script down in
> >> contrib/fast-import now. I'd like to move it up to the top-level
> >> source directory of git to make it easier to build and
> >> distribute. Git-p4 already takes advantage of the git
> >> infrastructure for documentation and testing, as well as the
> >> community support (Junio, many reviewers).
> >
> > About time this was done. There's still a few oddities around but far
> > fewer than there used to be. I don't know if Junio has some rules on
> > what a command needs before it graduates from contrib though.
>
> I try not to play a dictator around here. The primary thing I hesitated so
> far about git-p4 is that it is useless if you live in the open source only
> world, iow without proprietary software.
Yes, sad. Git-p4 at least helps people who have no choice but to
use p4, e.g., when working in a corporate environment.
It's been a big help developing git-p4 inside the git source tree
already. Having git-p4 be an installed component would make it
easier on users. "make install" puts the script where it goes.
Or better, they get it through their OS distribution.
-- Pete
^ permalink raw reply
* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Pete Wyckoff @ 2012-02-13 23:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Clemens Buchacher, git, Luke Diamand, Vitor Antunes
In-Reply-To: <7vhayuctwm.fsf@alter.siamese.dyndns.org>
gitster@pobox.com wrote on Mon, 13 Feb 2012 13:20 -0800:
> Clemens Buchacher <drizzd@aon.at> writes:
>
> >> Erm,... do you really need the alias if you add git-p4 in a directory on
> >> your $PATH?
> >
> > With recent git versions, this has stopped working.
>
> Erm, I am confused.
>
> $ git --exec-path
> /home/junio/g/Debian-6.X-x86_64/git-jch/libexec/git-core
> $ type git-hello
> bash: type: git-hello: not found
> $ cat >~/bin/common/git-hello <<EOF
> #!/bin/sh
> echo hello world
> EOF
> $ chmod +x ~/bin/common/git-hello
> $ type git-hello
> git-hello is /home/junio/bin/common/git-hello
> $ git hello
> hello world
>
> What am I missing???
Neat. I never knew this worked. That lets me remove quite a
few aliases. Apparently this has been possible since the
conversion from git.sh to git.c.
I tried to find where in the documentation this is talked about,
or where it should go. This doesn't feel like the best spot,
though.
------------8<-----------
>From 574669898aa891ffe3e785b280ac36177116658e Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Mon, 13 Feb 2012 18:17:10 -0500
Subject: [PATCH] document git-<command> can be found in PATH
Explain up front to users that arbitrary git "commands" can
be found anywhere in the PATH. For example, ~/bin/git-hello
will be invoked by "git hello".
---
Documentation/git.txt | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index f7e201f..0ef7f40 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -30,6 +30,8 @@ introduction.
The '<command>' is either a name of a Git command (see below) or an alias
as defined in the configuration file (see linkgit:git-config[1]).
+A '<command>' can also refer to an executable with the name git-'<command>'
+anywhere in your PATH.
Formatted and hyperlinked version of the latest git
documentation can be viewed at
--
1.7.9.193.g1d4a5.dirty
^ permalink raw reply related
* Re: [PATCH 2/2] Rename lineno_width to decimal_width and export it
From: Junio C Hamano @ 2012-02-13 23:29 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, pclouds, Michael J Gruber
In-Reply-To: <1329056180-29796-1-git-send-email-zbyszek@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> This function will be used in calculating diff --stat graph width.
> The name is changed because the function works for any number.
> The function is moved from builtins/blame.c to pager.c because it
> will be used not only in builtins/blame.c.
I'd prefer to see description of preliminary changes phrased without
depending too heavily on things that hasn't happened when possible.
Making a generic helper function to count digits necessary to print a
cardinal number available to future callers is a good thing by itself,
even if the "dynamic --stat width computation" turned out to be a bad
idea for whatever reason (I am not saying it is a bad idea here).
Perhaps like this.
Subject: make lineno_width() from blame reusable for others
builtin/blame.c has a helper function to compute how many columns
we need to show a line-number, whose implementation is reusable as
a more generic helper function to count the number of columns
necessary to show any cardinal number.
Rename it to decimal_width(), move it to pager.c and export it for
use by future callers.
And you can say something like "I'll be using this in 'diff --stat' in
later patches" after the three-dash line.
> ---
Sign-off before the three-dash line?
> +/*
> + * How many columns do we need to show numbers in decimal?
s/numbers/this number/;
> + */
> +int decimal_width(int number)
Don't we want to make the argument "unsigned number" instead?
> +{
> + int i, width;
> +
> + for (width = 1, i = 10; i <= number; width++)
> + i *= 10;
> + return width;
> +}
^ permalink raw reply
* Re: [PATCH 1/2] Save terminal width before setting up pager and export term_columns()
From: Junio C Hamano @ 2012-02-13 23:00 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, pclouds, Michael J Gruber
In-Reply-To: <1329055953-29632-1-git-send-email-zbyszek@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> term_columns() checks for terminal width via ioctl(2). After
> redirecting, stdin is no longer terminal to get terminal width.
s/stdin/stdout/
> Check terminal width and save it before redirecting stdin in
> setup_pager() by calling term_columns().
>
> Move term_columns() to pager.c and export it in cache.h.
>
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
Thanks.
It probably is worth mentioning what the end-user visible effect of this
change is somewhere in the log message.
I somehow find "term_columns_cache" a funny name for this variable and
does not describe what it does. Unlike a real cache, we cannot discard it
and re-read it even if we later wanted to.
I am tempted to rewrite the patch like this to update other minor style
issues.
-- >8 --
From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Sun, 12 Feb 2012 15:12:32 +0100
Subject: [PATCH] pager: find out the terminal width before spawning the pager
term_columns() checks for terminal width via ioctl(2) on the standard
output, but we spawn the pager too early for this check to be useful.
The effect of this buglet can be observed by opening a wide terminal and
running "git -p help --all", which still shows 80-column output, while
"git help --all" uses the full terminal width. Run the check before we
spawn the pager to fix this.
While at it, move term_columns() to pager.c and export it from cache.h so
that callers other than the help subsystem can use it.
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 1 +
help.c | 22 ----------------------
| 43 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 22 deletions(-)
diff --git a/cache.h b/cache.h
index 79c612f..c7e3b4d 100644
--- a/cache.h
+++ b/cache.h
@@ -1172,6 +1172,7 @@ extern void setup_pager(void);
extern const char *pager_program;
extern int pager_in_use(void);
extern int pager_use_color;
+extern int term_columns(void);
extern const char *editor_program;
extern const char *askpass_program;
diff --git a/help.c b/help.c
index cbbe966..14eefc9 100644
--- a/help.c
+++ b/help.c
@@ -5,28 +5,6 @@
#include "help.h"
#include "common-cmds.h"
-/* most GUI terminals set COLUMNS (although some don't export it) */
-static int term_columns(void)
-{
- char *col_string = getenv("COLUMNS");
- int n_cols;
-
- if (col_string && (n_cols = atoi(col_string)) > 0)
- return n_cols;
-
-#ifdef TIOCGWINSZ
- {
- struct winsize ws;
- if (!ioctl(1, TIOCGWINSZ, &ws)) {
- if (ws.ws_col)
- return ws.ws_col;
- }
- }
-#endif
-
- return 80;
-}
-
void add_cmdname(struct cmdnames *cmds, const char *name, int len)
{
struct cmdname *ent = xmalloc(sizeof(*ent) + len + 1);
--git a/pager.c b/pager.c
index 975955b..e06cfa0 100644
--- a/pager.c
+++ b/pager.c
@@ -76,6 +76,12 @@ void setup_pager(void)
if (!pager)
return;
+ /*
+ * force computing the width of the terminal before we redirect
+ * the standard output to the pager.
+ */
+ (void) term_columns();
+
setenv("GIT_PAGER_IN_USE", "true", 1);
/* spawn the pager */
@@ -110,3 +116,40 @@ int pager_in_use(void)
env = getenv("GIT_PAGER_IN_USE");
return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
}
+
+/*
+ * Return cached value (if set) or $COLUMNS (if set and positive) or
+ * ioctl(1, TIOCGWINSZ).ws_col (if positive) or 80.
+ *
+ * $COLUMNS even if set, is usually not exported, so
+ * the variable can be used to override autodection.
+ * This behaviour conforms to The Single UNIX Specification, Version 2
+ * (http://pubs.opengroup.org/onlinepubs/7908799/xbd/envvar.html#tag_002_003).
+ */
+int term_columns(void)
+{
+ static int term_columns_at_startup;
+
+ char *col_string;
+ int n_cols;
+
+ if (term_columns_at_startup)
+ return term_columns_at_startup;
+
+ term_columns_at_startup = 80;
+
+ col_string = getenv("COLUMNS");
+ if (col_string && (n_cols = atoi(col_string)) > 0)
+ term_columns_at_startup = n_cols;
+#ifdef TIOCGWINSZ
+ else {
+ struct winsize ws;
+ if (!ioctl(1, TIOCGWINSZ, &ws)) {
+ if (ws.ws_col) {
+ term_columns_at_startup = ws.ws_col;
+ }
+ }
+ }
+#endif
+ return term_columns_at_startup;
+}
--
1.7.9.300.gd47e4
^ permalink raw reply related
* [PATCH 5/5] diff-highlight: document some non-optimal cases
From: Jeff King @ 2012-02-13 22:37 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <20120213222702.GA19393@sigill.intra.peff.net>
The diff-highlight script works on heuristics, so it can be
wrong. Let's document some of the wrong-ness in case
somebody feels like working on it.
Signed-off-by: Jeff King <peff@peff.net>
---
These were just some that I considered while looking at the output of
the original and the current code. Suggestions are welcome for more.
contrib/diff-highlight/README | 93 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index 4a58579..502e03b 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -57,3 +57,96 @@ following in your git configuration:
show = diff-highlight | less
diff = diff-highlight | less
---------------------------------------------
+
+Bugs
+----
+
+Because diff-highlight relies on heuristics to guess which parts of
+changes are important, there are some cases where the highlighting is
+more distracting than useful. Fortunately, these cases are rare in
+practice, and when they do occur, the worst case is simply a little
+extra highlighting. This section documents some cases known to be
+sub-optimal, in case somebody feels like working on improving the
+heuristics.
+
+1. Two changes on the same line get highlighted in a blob. For example,
+ highlighting:
+
+----------------------------------------------
+-foo(buf, size);
++foo(obj->buf, obj->size);
+----------------------------------------------
+
+ yields (where the inside of "+{}" would be highlighted):
+
+----------------------------------------------
+-foo(buf, size);
++foo(+{obj->buf, obj->}size);
+----------------------------------------------
+
+ whereas a more semantically meaningful output would be:
+
+----------------------------------------------
+-foo(buf, size);
++foo(+{obj->}buf, +{obj->}size);
+----------------------------------------------
+
+ Note that doing this right would probably involve a set of
+ content-specific boundary patterns, similar to word-diff. Otherwise
+ you get junk like:
+
+-----------------------------------------------------
+-this line has some -{i}nt-{ere}sti-{ng} text on it
++this line has some +{fa}nt+{a}sti+{c} text on it
+-----------------------------------------------------
+
+ which is less readable than the current output.
+
+2. The multi-line matching assumes that lines in the pre- and post-image
+ match by position. This is often the case, but can be fooled when a
+ line is removed from the top and a new one added at the bottom (or
+ vice versa). Unless the lines in the middle are also changed, diffs
+ will show this as two hunks, and it will not get highlighted at all
+ (which is good). But if the lines in the middle are changed, the
+ highlighting can be misleading. Here's a pathological case:
+
+-----------------------------------------------------
+-one
+-two
+-three
+-four
++two 2
++three 3
++four 4
++five 5
+-----------------------------------------------------
+
+ which gets highlighted as:
+
+-----------------------------------------------------
+-one
+-t-{wo}
+-three
+-f-{our}
++two 2
++t+{hree 3}
++four 4
++f+{ive 5}
+-----------------------------------------------------
+
+ because it matches "two" to "three 3", and so forth. It would be
+ nicer as:
+
+-----------------------------------------------------
+-one
+-two
+-three
+-four
++two +{2}
++three +{3}
++four +{4}
++five 5
+-----------------------------------------------------
+
+ which would probably involve pre-matching the lines into pairs
+ according to some heuristic.
--
1.7.8.4.17.g2df81
^ permalink raw reply related
* [PATCH 4/5] diff-highlight: match multi-line hunks
From: Jeff King @ 2012-02-13 22:36 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <20120213222702.GA19393@sigill.intra.peff.net>
Currently we only bother highlighting single-line hunks. The
rationale was that the purpose of highlighting is to point
out small changes between two similar lines that are
otherwise hard to see. However, that meant we missed similar
cases where two lines were changed together, like:
-foo(buf);
-bar(buf);
+foo(obj->buf);
+bar(obj->buf);
Each of those changes is simple, and would benefit from
highlighting (the "obj->" parts in this case).
This patch considers whole hunks at a time. For now, we
consider only the case where the hunk has the same number of
removed and added lines, and assume that the lines from each
segment correspond one-to-one. While this is just a
heuristic, in practice it seems to generate sensible
results (especially because we now omit highlighting on
completely-changed lines, so when our heuristic is wrong, we
tend to avoid highlighting at all).
Based on an original idea and implementation by Michał
Kiedrowicz.
Signed-off-by: Jeff King <peff@peff.net>
---
Same attribution statement applies as to patch 2 (in fact, patches 1 and
3 could be attributed to you, too).
This version has the missing documentation fixes. The implementation is
a little different than yours. I rearranged the parsing in a manner that
was a little more obvious to me, and I pulled out the "don't highlight
if the number of lines don't match" case into its own conditional, which
makes it more obvious where to work if somebody wants to try doing
something fancier.
contrib/diff-highlight/README | 16 ++++---
contrib/diff-highlight/diff-highlight | 70 ++++++++++++++++++++-------------
2 files changed, 52 insertions(+), 34 deletions(-)
diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index 1b7b6df..4a58579 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -14,13 +14,15 @@ Instead, this script post-processes the line-oriented diff, finds pairs
of lines, and highlights the differing segments. It's currently very
simple and stupid about doing these tasks. In particular:
- 1. It will only highlight a pair of lines if they are the only two
- lines in a hunk. It could instead try to match up "before" and
- "after" lines for a given hunk into pairs of similar lines.
- However, this may end up visually distracting, as the paired
- lines would have other highlighted lines in between them. And in
- practice, the lines which most need attention called to their
- small, hard-to-see changes are touching only a single line.
+ 1. It will only highlight hunks in which the number of removed and
+ added lines is the same, and it will pair lines within the hunk by
+ position (so the first removed line is compared to the first added
+ line, and so forth). This is simple and tends to work well in
+ practice. More complex changes don't highlight well, so we tend to
+ exclude them due to the "same number of removed and added lines"
+ restriction. Or even if we do try to highlight them, they end up
+ not highlighting because of our "don't highlight if the whole line
+ would be highlighted" rule.
2. It will find the common prefix and suffix of two lines, and
consider everything in the middle to be "different". It could
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 279d211..c4404d4 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -10,23 +10,28 @@ my $UNHIGHLIGHT = "\x1b[27m";
my $COLOR = qr/\x1b\[[0-9;]*m/;
my $BORING = qr/$COLOR|\s/;
-my @window;
+my @removed;
+my @added;
+my $in_hunk;
while (<>) {
- # We highlight only single-line changes, so we need
- # a 4-line window to make a decision on whether
- # to highlight.
- push @window, $_;
- next if @window < 4;
- if ($window[0] =~ /^$COLOR*(\@| )/ &&
- $window[1] =~ /^$COLOR*-/ &&
- $window[2] =~ /^$COLOR*\+/ &&
- $window[3] !~ /^$COLOR*\+/) {
- print shift @window;
- show_hunk(shift @window, shift @window);
+ if (!$in_hunk) {
+ print;
+ $in_hunk = /^$COLOR*\@/;
+ }
+ elsif (/^$COLOR*-/) {
+ push @removed, $_;
+ }
+ elsif (/^$COLOR*\+/) {
+ push @added, $_;
}
else {
- print shift @window;
+ show_hunk(\@removed, \@added);
+ @removed = ();
+ @added = ();
+
+ print;
+ $in_hunk = /^$COLOR*[\@ ]/;
}
# Most of the time there is enough output to keep things streaming,
@@ -42,26 +47,37 @@ while (<>) {
}
}
-# Special case a single-line hunk at the end of file.
-if (@window == 3 &&
- $window[0] =~ /^$COLOR*(\@| )/ &&
- $window[1] =~ /^$COLOR*-/ &&
- $window[2] =~ /^$COLOR*\+/) {
- print shift @window;
- show_hunk(shift @window, shift @window);
-}
-
-# And then flush any remaining lines.
-while (@window) {
- print shift @window;
-}
+# Flush any queued hunk (this can happen when there is no trailing context in
+# the final diff of the input).
+show_hunk(\@removed, \@added);
exit 0;
sub show_hunk {
my ($a, $b) = @_;
- print highlight_pair($a, $b);
+ # If one side is empty, then there is nothing to compare or highlight.
+ if (!@$a || !@$b) {
+ print @$a, @$b;
+ return;
+ }
+
+ # If we have mismatched numbers of lines on each side, we could try to
+ # be clever and match up similar lines. But for now we are simple and
+ # stupid, and only handle multi-line hunks that remove and add the same
+ # number of lines.
+ if (@$a != @$b) {
+ print @$a, @$b;
+ return;
+ }
+
+ my @queue;
+ for (my $i = 0; $i < @$a; $i++) {
+ my ($rm, $add) = highlight_pair($a->[$i], $b->[$i]);
+ print $rm;
+ push @queue, $add;
+ }
+ print @queue;
}
sub highlight_pair {
--
1.7.8.4.17.g2df81
^ permalink raw reply related
* [PATCH 3/5] diff-highlight: refactor to prepare for multi-line hunks
From: Jeff King @ 2012-02-13 22:33 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <20120213222702.GA19393@sigill.intra.peff.net>
The current code structure assumes that we will only look at
a pair of lines at any given time, and that the end result
should always be to output that pair. However, we want to
eventually handle multi-line hunks, which will involve
collating pairs of removed/added lines. Let's refactor the
code to return highlighted pairs instead of printing them.
Signed-off-by: Jeff King <peff@peff.net>
---
You did a similar refactoring in your patch, but I found pulling it out
made the next patch a lot more readable.
contrib/diff-highlight/diff-highlight | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 0d8df84..279d211 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -23,7 +23,7 @@ while (<>) {
$window[2] =~ /^$COLOR*\+/ &&
$window[3] !~ /^$COLOR*\+/) {
print shift @window;
- show_pair(shift @window, shift @window);
+ show_hunk(shift @window, shift @window);
}
else {
print shift @window;
@@ -48,7 +48,7 @@ if (@window == 3 &&
$window[1] =~ /^$COLOR*-/ &&
$window[2] =~ /^$COLOR*\+/) {
print shift @window;
- show_pair(shift @window, shift @window);
+ show_hunk(shift @window, shift @window);
}
# And then flush any remaining lines.
@@ -58,7 +58,13 @@ while (@window) {
exit 0;
-sub show_pair {
+sub show_hunk {
+ my ($a, $b) = @_;
+
+ print highlight_pair($a, $b);
+}
+
+sub highlight_pair {
my @a = split_line(shift);
my @b = split_line(shift);
@@ -106,12 +112,12 @@ sub show_pair {
}
if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
- print highlight(\@a, $pa, $sa);
- print highlight(\@b, $pb, $sb);
+ return highlight_line(\@a, $pa, $sa),
+ highlight_line(\@b, $pb, $sb);
}
else {
- print join('', @a);
- print join('', @b);
+ return join('', @a),
+ join('', @b);
}
}
@@ -121,7 +127,7 @@ sub split_line {
split /($COLOR*)/;
}
-sub highlight {
+sub highlight_line {
my ($line, $prefix, $suffix) = @_;
return join('',
--
1.7.8.4.17.g2df81
^ permalink raw reply related
* [PATCH 2/5] diff-highlight: don't highlight whole lines
From: Jeff King @ 2012-02-13 22:32 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <20120213222702.GA19393@sigill.intra.peff.net>
If you have a change like:
-foo
+bar
we end up highlighting the entirety of both lines (since the
whole thing is changed). But the point of diff highlighting
is to pinpoint the specific change in a pair of lines that
are mostly identical. In this case, the highlighting is just
noise, since there is nothing to pinpoint, and we are better
off doing nothing.
The implementation looks for "interesting" pairs by checking
to see whether they actually have a matching prefix or
suffix that does not simply consist of colorization and
whitespace. However, the implementation makes it easy to
plug in other heuristics, too, like:
1. Depending on the source material, the set of "boring"
characters could be tweaked to include language-specific
stuff (like braces or semicolons for C).
2. Instead of saying "an interesting line has at least one
character of prefix or suffix", we could require that
less than N percent of the line be highlighted.
The simple "ignore whitespace, and highlight if there are
any matched characters" implemented by this patch seems to
give good results on git.git. I'll leave experimentation
with other heuristics to somebody who has a dataset that
does not look good with the current code.
Based on an original idea and implementation by Michał
Kiedrowicz.
Signed-off-by: Jeff King <peff@peff.net>
---
Regarding attribution: I kept myself as author, because I rewrote it a
bit and figured that I would take primary responsibility for bugs in
this patch, and in the long run would be responsible for maintaining it.
But the idea and the substance of the patch are yours, and I would be
happy to list you as author if you prefer getting the credit that way
(after all, it bumps your shortlog numbers :) ).
The implementation is similar to yours, but pulls some of the decisions
out into a separate function to make tweaking the above heuristics
easier.
contrib/diff-highlight/diff-highlight | 28 ++++++++++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index c3302dd..0d8df84 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -8,6 +8,7 @@ use strict;
my $HIGHLIGHT = "\x1b[7m";
my $UNHIGHLIGHT = "\x1b[27m";
my $COLOR = qr/\x1b\[[0-9;]*m/;
+my $BORING = qr/$COLOR|\s/;
my @window;
@@ -104,8 +105,14 @@ sub show_pair {
}
}
- print highlight(\@a, $pa, $sa);
- print highlight(\@b, $pb, $sb);
+ if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
+ print highlight(\@a, $pa, $sa);
+ print highlight(\@b, $pb, $sb);
+ }
+ else {
+ print join('', @a);
+ print join('', @b);
+ }
}
sub split_line {
@@ -125,3 +132,20 @@ sub highlight {
@{$line}[($suffix+1)..$#$line]
);
}
+
+# Pairs are interesting to highlight only if we are going to end up
+# highlighting a subset (i.e., not the whole line). Otherwise, the highlighting
+# is just useless noise. We can detect this by finding either a matching prefix
+# or suffix (disregarding boring bits like whitespace and colorization).
+sub is_pair_interesting {
+ my ($a, $pa, $sa, $b, $pb, $sb) = @_;
+ my $prefix_a = join('', @$a[0..($pa-1)]);
+ my $prefix_b = join('', @$b[0..($pb-1)]);
+ my $suffix_a = join('', @$a[($sa+1)..$#$a]);
+ my $suffix_b = join('', @$b[($sb+1)..$#$b]);
+
+ return $prefix_a !~ /^$COLOR*-$BORING*$/ ||
+ $prefix_b !~ /^$COLOR*\+$BORING*$/ ||
+ $suffix_a !~ /^$BORING*$/ ||
+ $suffix_b !~ /^$BORING*$/;
+}
--
1.7.8.4.17.g2df81
^ permalink raw reply related
* [PATCH 1/5] diff-highlight: make perl strict and warnings fatal
From: Jeff King @ 2012-02-13 22:28 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <20120213222702.GA19393@sigill.intra.peff.net>
These perl features can catch bugs, and we shouldn't be
violating any of the strict rules or creating any warnings,
so let's turn them on.
Signed-off-by: Jeff King <peff@peff.net>
---
contrib/diff-highlight/diff-highlight | 3 +++
1 file changed, 3 insertions(+)
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index d893898..c3302dd 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -1,5 +1,8 @@
#!/usr/bin/perl
+use warnings FATAL => 'all';
+use strict;
+
# Highlight by reversing foreground and background. You could do
# other things like bold or underline if you prefer.
my $HIGHLIGHT = "\x1b[7m";
--
1.7.8.4.17.g2df81
^ permalink raw reply related
* Re: [PATCH] diff-highlight: Work for multiline changes too
From: Jeff King @ 2012-02-13 22:27 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328910433-2539-1-git-send-email-michal.kiedrowicz@gmail.com>
On Fri, Feb 10, 2012 at 10:47:13PM +0100, Michał Kiedrowicz wrote:
> contrib/diff-highlight/diff-highlight | 96 ++++++++++++++++++++++-----------
> 1 files changed, 65 insertions(+), 31 deletions(-)
Thanks for sending. I looked at a whole bunch of patches, and I was
pleasantly surprised to find how infrequently we hit false positives in
practice. In fact, the only things that looked worse with your patch
were places where your patch happened to turn on highlighting for lines
where the existing heuristics already were a little ugly (i.e., the
problem was not your patch, but that the existing heuristic is sometimes
non-optimal).
I ended up pulling your changes out into a few distinct commits. That
made it easier for me to review and understand what was going on (and
hopefully ditto for other reviewers, or people who end up bisecting or
reading the log later). I'll post that series in a moment.
> After looking at outputs I noticed that it can also ignore lines with
> prefixes/suffixes that consist only of punctuation (asterisk, semicolon, dot,
> etc), because otherwise whole line is highlighted except for terminating
> punctuation.
I missed this note when I applied the patch and started looking at the
outputs, and ended up having a similar thought. However, I don't know
that it buys much in practice, and it's nice to be fairly agnostic about
content. I did leave that open to easy tweaking in my series, though.
[1/5]: diff-highlight: make perl strict and warnings fatal
[2/5]: diff-highlight: don't highlight whole lines
[3/5]: diff-highlight: refactor to prepare for multi-line hunks
[4/5]: diff-highlight: match multi-line hunks
[5/5]: diff-highlight: document some non-optimal cases
-Peff
^ permalink raw reply
* Re: [PATCH] Support wrapping commit messages when you read them
From: Junio C Hamano @ 2012-02-13 22:25 UTC (permalink / raw)
To: Sidney San Martín; +Cc: git
In-Reply-To: <46957CEB-5E48-4C11-8428-9A88C3810548@sidneysm.com>
Sidney San Martín <s@sidneysm.com> writes:
>> After all, SCM is merely a method to help communication between
>> developers, and sticking to the common denominator is a proven good way to
>> make sure everybody involved in the project can use what is recorded in
>> the repository. This is not limited only to the log message, but equally
>> applies to filenames (e.g. don't create xt_tcpmss.c and xt_TCPMSS.c in the
>> same directory if you want your project extractable on case insensitive
>> filesystems) and even to the sources.
>>
>> You need to justify the cause a bit better. Why is such a new logic
>> justified?
>
> You’re right, that sentence doesn't say anything.
>
> I agree that projects need to have standards for their commit messages,
> but I also think that line wrapping should be taken care of by the
> computer so that the humans can think about the content of their commit
> messages. It's easier for everyone.
I just typed M-q to wrap the above paragraph from you to make it readable.
"Computers are good at automating" is true, and that is why real editors
give an easy way to auto-wrap long prose in a paragraph while composing.
But "computers are good at automating" is not a convincing justification
to let the composer leave unreasonably long lines in the commit log object
and force the reader side to line-wrap the mess only to fix it up.
^ permalink raw reply
* Re: GitWeb and atom feed links
From: Jakub Narebski @ 2012-02-13 22:17 UTC (permalink / raw)
To: Heiko W. Rupp; +Cc: git
In-Reply-To: <F3741779-8DDA-4275-BB68-24D02297C702@pilhuhn.de>
Heiko W. Rupp wrote:
>
> when you e.g. look at http://git.kernel.org/?p=git/git.git;a=summary
> and then the lower right, there are two buttons for feeds.
Yes, [Atom] and [RSS], for different formats of the same feed.
> If you click on e.g. atom, you end up with an url of
> http://git.kernel.org/?p=git/git.git;a=atom
> where the output is not a feed in atom format, but plain html with
> tables etc.
>
> If you change the url to http://git.kernel.org/?p=git/git.git&a=atom
> the output is a correct atom feed (same for rss).
I don't know what is the source of bug you are seeing; I suspect some
trouble with output caching that git.kernel.org fork of gitweb has added,
but this isn't it. Those two forms of 'atom' URL are equivalent.
Wikipedia says in http://en.wikipedia.org/wiki/Query_string:
"* The query string is composed of a series of field-value pairs.
* The field-value pairs are each separated by an equals sign. The
equals sign may be omitted if the value is an empty string.
* The series of pairs is separated by the ampersand, '&' or
semicolon, ';'.
[...]
W3C recommends that all web servers support semicolon separators
in the place of ampersand separators.[4]"
[4]: http://www.w3.org/TR/1999/REC-html401-19991224/appendix/notes.html#h-B.2.2
"B.2.2 Ampersands in URI attribute values
----------------------------------------
The URI that is constructed when a form is submitted may be used as an
anchor-style link (e.g., the href attribute for the A element).
Unfortunately, the use of the "&" character to separate form fields
interacts with its use in SGML attribute values to delimit character
entity references. For example, to use the URI "http://host/?x=1&y=2"
as a linking URI, it must be written <A href="http://host/?x=1&y=2">
or <A href="http://host/?x=1&y=2">.
We recommend that HTTP server implementors, and in particular, CGI
implementors support the use of ";" in place of "&" to save authors
the trouble of escaping "&" characters in this manner."
CGI(3pm) says:
"-newstyle_urls
Separate the name=value pairs in CGI parameter query strings with semi-
colons rather than ampersands. For example:
?name=fred;age=24;favorite_color=3
Semicolon-delimited query strings are always accepted, and will be
emitted by self_url() and query_string(). newstyle_urls became the
default in version 2.64."
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 3/5] git push: verify refs early
From: Junio C Hamano @ 2012-02-13 22:16 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, spearce
In-Reply-To: <1329164235-29955-4-git-send-email-drizzd@aon.at>
Clemens Buchacher <drizzd@aon.at> writes:
> I suppose with some effort, this could be done for smart HTTP as well.
> But I am not sure if we actually want the overhead of the additional
> ping-pong for HTTP.
Hrm, I am confused.
The updated protocol exchange, if I am reading your patch correctly, would
go like this (S stands for the sender, R for the receiver):
R: Here are the tips of my refs
----
S: I'd like to update your refs this way
----
+ R: No you cannot because all updates will fail, go away
or
+ R: You may proceed, as some updates may succeed
----
S: Here is the packfile
----
R: Here is how I processed your request
Given that this makes the sender stall for both smart HTTP and native
protocol, don't your worries about the additional ping-pong apply equally
to both transports?
If it is not worth doing for smart HTTP, I wonder if it is worth doing for
native transport. After all, "all updates will fail" is hopefully the
less likely case, and with this protocol extension, we end up penalizing
the common case with an extra stall for everybody, regardless of the
transport.
I dunno.
All other patches in this "series" looked nice fixes and improvements, but
I am not sure about this change. At least I am not yet convinced.
> builtin/receive-pack.c | 83 ++++++++++++++++++++++++++++++++++++++----------
> builtin/send-pack.c | 43 +++++++++++++++++--------
> send-pack.h | 3 +-
> 3 files changed, 97 insertions(+), 32 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 0afb8b2..0129d9c 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -34,6 +34,8 @@ static int unpack_limit = 100;
> static int report_status;
> static int use_sideband;
> static int quiet;
> +static int verify_refs;
> +static int stateless_rpc;
> static int prefer_ofs_delta = 1;
> static int auto_update_server_info;
> static int auto_gc = 1;
> @@ -123,7 +125,7 @@ static void show_ref(const char *path, const unsigned char *sha1)
> else
> packet_write(1, "%s %s%c%s%s\n",
> sha1_to_hex(sha1), path, 0,
> - " report-status delete-refs side-band-64k quiet",
> + " report-status delete-refs side-band-64k quiet verify-refs",
> prefer_ofs_delta ? " ofs-delta" : "");
> sent_capabilities = 1;
> }
> @@ -410,14 +412,13 @@ static void refuse_unconfigured_deny_delete_current(void)
> rp_error("%s", refuse_unconfigured_deny_delete_current_msg[i]);
> }
>
> -static const char *update(struct command *cmd)
> +static const char *verify_ref(struct command *cmd)
> {
> const char *name = cmd->ref_name;
> struct strbuf namespaced_name_buf = STRBUF_INIT;
> const char *namespaced_name;
> unsigned char *old_sha1 = cmd->old_sha1;
> unsigned char *new_sha1 = cmd->new_sha1;
> - struct ref_lock *lock;
>
> /* only refs/... are allowed */
> if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
> @@ -444,12 +445,6 @@ static const char *update(struct command *cmd)
> }
> }
>
> - if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
> - error("unpack should have generated %s, "
> - "but I can't find it!", sha1_to_hex(new_sha1));
> - return "bad pack";
> - }
> -
> if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
> if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
> rp_error("denying ref deletion for %s", name);
> @@ -473,6 +468,27 @@ static const char *update(struct command *cmd)
> }
> }
>
> + return NULL;
> +}
> +
> +static const char *update(struct command *cmd)
> +{
> + const char *name = cmd->ref_name;
> + struct strbuf namespaced_name_buf = STRBUF_INIT;
> + const char *namespaced_name;
> + unsigned char *old_sha1 = cmd->old_sha1;
> + unsigned char *new_sha1 = cmd->new_sha1;
> + struct ref_lock *lock;
> +
> + strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
> + namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
> +
> + if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
> + error("unpack should have generated %s, "
> + "but I can't find it!", sha1_to_hex(new_sha1));
> + return "bad pack";
> + }
> +
> if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
> !is_null_sha1(old_sha1) &&
> !prefixcmp(name, "refs/heads/")) {
> @@ -692,10 +708,41 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
> return -1; /* end of list */
> }
>
> +static int verify_ref_commands(struct command *commands)
> +{
> + unsigned char sha1[20];
> + int commands_ok;
> + struct command *cmd;
> +
> + free(head_name_to_free);
> + head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
> +
> + commands_ok = 0;
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + cmd->error_string = verify_ref(cmd);
> + if (!cmd->error_string)
> + commands_ok++;
> + }
> +
> + if (verify_refs && !stateless_rpc) {
> + struct strbuf buf = STRBUF_INIT;
> +
> + packet_buf_write(&buf, "verify-refs %s\n",
> + commands_ok > 0 ? "ok" : "no valid refs");
> +
> + if (use_sideband)
> + send_sideband(1, 1, buf.buf, buf.len, use_sideband);
> + else
> + safe_write(1, buf.buf, buf.len);
> + strbuf_release(&buf);
> + }
> +
> + return commands_ok;
> +}
> +
> static void execute_commands(struct command *commands, const char *unpacker_error)
> {
> struct command *cmd;
> - unsigned char sha1[20];
>
> if (unpacker_error) {
> for (cmd = commands; cmd; cmd = cmd->next)
> @@ -718,9 +765,6 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
>
> check_aliased_updates(commands);
>
> - free(head_name_to_free);
> - head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
> -
> for (cmd = commands; cmd; cmd = cmd->next) {
> if (cmd->error_string)
> continue;
> @@ -766,6 +810,8 @@ static struct command *read_head_info(void)
> use_sideband = LARGE_PACKET_MAX;
> if (parse_feature_request(feature_list, "quiet"))
> quiet = 1;
> + if (parse_feature_request(feature_list, "verify-refs"))
> + verify_refs = 1;
> }
> cmd = xcalloc(1, sizeof(struct command) + len - 80);
> hashcpy(cmd->old_sha1, old_sha1);
> @@ -905,7 +951,6 @@ static int delete_only(struct command *commands)
> int cmd_receive_pack(int argc, const char **argv, const char *prefix)
> {
> int advertise_refs = 0;
> - int stateless_rpc = 0;
> int i;
> char *dir = NULL;
> struct command *commands;
> @@ -962,11 +1007,15 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
> return 0;
>
> if ((commands = read_head_info()) != NULL) {
> + int commands_ok;
> const char *unpack_status = NULL;
>
> - if (!delete_only(commands))
> - unpack_status = unpack();
> - execute_commands(commands, unpack_status);
> + commands_ok = verify_ref_commands(commands);
> + if (!verify_refs || commands_ok > 0) {
> + if (!delete_only(commands))
> + unpack_status = unpack();
> + execute_commands(commands, unpack_status);
> + }
> if (pack_lockfile)
> unlink_or_warn(pack_lockfile);
> if (report_status)
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 71f258e..7d514ca 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -265,6 +265,8 @@ int send_pack(struct send_pack_args *args,
> use_sideband = 1;
> if (!server_supports("quiet"))
> args->quiet = 0;
> + if (server_supports("verify-refs"))
> + args->verify_refs = 1;
>
> if (!remote_refs) {
> fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
> @@ -303,12 +305,13 @@ int send_pack(struct send_pack_args *args,
> char *old_hex = sha1_to_hex(ref->old_sha1);
> char *new_hex = sha1_to_hex(ref->new_sha1);
>
> - if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
> - packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
> + if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
> + packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
> old_hex, new_hex, ref->name, 0,
> status_report ? " report-status" : "",
> use_sideband ? " side-band-64k" : "",
> - args->quiet ? " quiet" : "");
> + args->quiet ? " quiet" : "",
> + args->verify_refs ? " verify-refs" : "");
> }
> else
> packet_buf_write(&req_buf, "%s %s %s",
> @@ -341,17 +344,29 @@ int send_pack(struct send_pack_args *args,
> in = demux.out;
> }
>
> - if (new_refs && cmds_sent) {
> - if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> - for (ref = remote_refs; ref; ref = ref->next)
> - ref->status = REF_STATUS_NONE;
> - if (args->stateless_rpc)
> - close(out);
> - if (git_connection_is_socket(conn))
> - shutdown(fd[0], SHUT_WR);
> - if (use_sideband)
> - finish_async(&demux);
> - return -1;
> + if (cmds_sent) {
> + int verify_refs_status = 0;
> +
> + if (args->verify_refs && !args->stateless_rpc) {
> + char line[1000];
> + int len = packet_read_line(in, line, sizeof(line));
> + if (len < 15 || memcmp(line, "verify-refs ", 12))
> + return error("did not receive remote status");
> + verify_refs_status = memcmp(line, "verify-refs ok\n", 15);
> + }
> +
> + if (!verify_refs_status && new_refs) {
> + if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> + for (ref = remote_refs; ref; ref = ref->next)
> + ref->status = REF_STATUS_NONE;
> + if (args->stateless_rpc)
> + close(out);
> + if (git_connection_is_socket(conn))
> + shutdown(fd[0], SHUT_WR);
> + if (use_sideband)
> + finish_async(&demux);
> + return -1;
> + }
> }
> }
> if (args->stateless_rpc && cmds_sent)
> diff --git a/send-pack.h b/send-pack.h
> index 05d7ab1..87edaa5 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -11,7 +11,8 @@ struct send_pack_args {
> use_thin_pack:1,
> use_ofs_delta:1,
> dry_run:1,
> - stateless_rpc:1;
> + stateless_rpc:1,
> + verify_refs:1;
> };
>
> int send_pack(struct send_pack_args *args,
^ permalink raw reply
* Re: [PATCH 2/5] do not override receive-pack errors
From: Junio C Hamano @ 2012-02-13 21:41 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
In-Reply-To: <1329164235-29955-3-git-send-email-drizzd@aon.at>
Clemens Buchacher <drizzd@aon.at> writes:
> Receive runs rev-list --verify-objects in order to detect missing
> objects. However, such errors are ignored and overridden later.
This makes me worried (not about the patch, but about the current code).
Are there codepaths where an earlier pass of verify-objects mark a cmd as
bad with a non-NULL error_string, and later code that checks other aspect
of the push says the update does not violate its criteria, and flips the
non-NULL error_string back to NULL? Or is the only offence you found in
such later code that it fills error_string with its own non-NULL string
when it finds a violation (and otherwise does not touch error_string)?
In other words, is this really "ignored and overridden", not merely
"overwritten"?
In the following review, I assumed that you meant "overwritten".
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index fa7448b..0afb8b2 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -642,8 +642,10 @@ static void check_aliased_updates(struct command *commands)
> }
> sort_string_list(&ref_list);
>
> - for (cmd = commands; cmd; cmd = cmd->next)
> - check_aliased_update(cmd, &ref_list);
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (!cmd->error_string)
> + check_aliased_update(cmd, &ref_list);
> + }
While I agree with the general concept of this patch (i.e. if we know an
error exists for a particular ref update, we would want to keep the first
one without overwriting it with another error), I am not sure if this hunk
is correct. This checks cross reactivity between multiple cmds that can
arise when an update made by one will affect the previous value assumed
for another cmd because the former cmd updates a symref whose the target
is what the later cmd wants to update. If we have already decided the
former cmd is deemed to fail and skip this check, we would not catch that
the latter cmd is trying to make an inconsistent update request, and we
would end up ignoring that case. Is that the right thing to do?
> @@ -707,8 +709,10 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
> set_connectivity_errors(commands);
>
> if (run_receive_hook(commands, pre_receive_hook, 0)) {
> - for (cmd = commands; cmd; cmd = cmd->next)
> - cmd->error_string = "pre-receive hook declined";
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (!cmd->error_string)
> + cmd->error_string = "pre-receive hook declined";
> + }
> return;
> }
>
> @@ -717,9 +721,15 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
> free(head_name_to_free);
> head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
>
> - for (cmd = commands; cmd; cmd = cmd->next)
> - if (!cmd->skip_update)
> - cmd->error_string = update(cmd);
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (cmd->error_string)
> + continue;
> +
> + if (cmd->skip_update)
> + continue;
> +
> + cmd->error_string = update(cmd);
> + }
> }
These two hunks look good.
^ permalink raw reply
* Re: [PATCH] Support wrapping commit messages when you read them
From: Sidney San Martín @ 2012-02-13 21:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfwg99dom.fsf@alter.siamese.dyndns.org>
Hey Junio,
I apologize for the delay, I do want to keep working on this idea. I had to put it down but should have more time now if you're willing to keep talking about it.
Thanks for taking the time to look at my first patch.
On Dec 25, 2011, at 4:57 AM, Junio C Hamano wrote:
>> Fairly simpleminded line wrapping that makes commit messages
>> readable if they weren’t wrapped by the committer.
>
> This does not say anything useful, other than "this is a naïve
> implementation of message wrapper" and invites "So what?".
>
> The most simple-minded solution is to reject such commits with crappy log
> message.
>
> After all, SCM is merely a method to help communication between
> developers, and sticking to the common denominator is a proven good way to
> make sure everybody involved in the project can use what is recorded in
> the repository. This is not limited only to the log message, but equally
> applies to filenames (e.g. don't create xt_tcpmss.c and xt_TCPMSS.c in the
> same directory if you want your project extractable on case insensitive
> filesystems) and even to the sources.
>
> You need to justify the cause a bit better. Why is such a new logic
> justified?
You’re right, that sentence doesn't say anything.
I agree that projects need to have standards for their commit messages, but I also think that line wrapping should be taken care of by the computer so that the humans can think about the content of their commit messages. It's easier for everyone.
It also makes sense to not assume the user is using an 80-column terminal. Like I mentioned in another email, other tools work this way (e.g. manpages). It turns out that "git help" already has code to detect the width of the terminal, and it formats its output to fit it. I want to adapt that logic for this feature.
How about replacing that paragraph with this:
“Git didn’t previously support formatting commit messages for a user’s terminal, and the common practice has been to pre-wrap commit messages to under 80 columns. This is necessary for some projects, especially those which trade patches over email where mail clients might damage longer lines, but in many cases it’s only done so that the messages are readable in "git log" and the like. Supporting line wrapping in git lets users choose to leave their commit messages unwrapped and have them formatted for their terminal when displayed.”
>> - Use strbuf_add_wrapped_text() to do the dirty work
>> - Detect simple lists which begin with "+ ", "* ", or "- " and indent
>> them appropriately (like this line)
>> - Print lines which begin with whitespace as-is (e.g. code samples)
>
> I suspect the above would make it more palatable than format=flowed
> brought up in earlier discussions, which is unsuitable for nothing but
> straight text.
>
>> Add --wrap[=<width>] and --no-wrap to commands that pretty-print commit
>> messages, and add log.wrap and log.wrap.width configuration options.
>
> Why do you need two separate options and configurations that look as if
> they are independent but in reality not? If you say "no wrap", there is
> no room for you to say "wrap width is 72".
>
> I would expect something like:
>
> --log-message-wrap, --log-message-wrap=72, --log-message-wrap=no
>
> with --log-message-wrap=yes as a synonym for --log-message-wrap to give
> consistency. The corresponding configuraiton would be log.messageWrap
> whose values could be the usual bool-or-int.
I stole this from other options: --progress/--no-progress, --color/--color=[<when>]/--no-color, --track/--no-track, etc.
The separate wrap/wrap.width config options were so that you could set it separately to auto or always and also specify a width. But, I don't know if that's needed anymore. See below.
>> log.wrap defaults to never, and can be set to never/false, auto/true,
>> or always. If auto, hijack want_color() to decide whether it’s
>> appropriate to use line wrapping. (This is a little hacky, but as far
>> as I can tell the conditions for auto color and auto wrapping are the
>> same.
>
> Why does coloring have _anything_ to do with line wrapping? Maybe your
> personaly preference might be "wrap and color if interactive terminal" but
> that is conflating two unrelated concepts. A user may not expect coloring
> on a dumb interactive terminal, but wrapping may still be useful.
It doesn’t — I used want_color() so that I could get the patch out there without making other changes to the codebase or duplicating its code, so you could comment on the rest of it. I'll get it out of the next version of the patch, which I'll try to get to you later today or tomorrow.
>> log.wrap.width defaults to 80.
>
> This does not deserve a comment as I already rejected the "two
> configuration" approach, but do not use three-level names this way. We try
> to reserve three-level names only for cases where the second level is used
> for an unbound collection (e.g. "remote.$name.url", "branch.$name.merge").
> that is user-specified.
OK, that was a misunderstanding on my part. Actually, I would be in favor of getting rid of that option completely, moving in support for detecting the terminal width from "git help" and making it just a boolean, auto-only. How does that sound?
Sidney
^ permalink raw reply
* Re: [PATCH 4/5] t5541: use configured port number
From: Junio C Hamano @ 2012-02-13 21:23 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
In-Reply-To: <1329164235-29955-5-git-send-email-drizzd@aon.at>
Good eyes. Thanks.
^ permalink raw reply
* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Junio C Hamano @ 2012-02-13 21:20 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Pete Wyckoff, git, Luke Diamand, Vitor Antunes
In-Reply-To: <20120213203709.GA31671@ecki>
Clemens Buchacher <drizzd@aon.at> writes:
>> Erm,... do you really need the alias if you add git-p4 in a directory on
>> your $PATH?
>
> With recent git versions, this has stopped working.
Erm, I am confused.
$ git --exec-path
/home/junio/g/Debian-6.X-x86_64/git-jch/libexec/git-core
$ type git-hello
bash: type: git-hello: not found
$ cat >~/bin/common/git-hello <<EOF
#!/bin/sh
echo hello world
EOF
$ chmod +x ~/bin/common/git-hello
$ type git-hello
git-hello is /home/junio/bin/common/git-hello
$ git hello
hello world
What am I missing???
^ 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