* 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
* Re: [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output
From: Junio C Hamano @ 2012-02-13 21:13 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
In-Reply-To: <1329164235-29955-6-git-send-email-drizzd@aon.at>
Clemens Buchacher <drizzd@aon.at> writes:
> diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
> index 9ee52cf..3683df1 100755
> --- a/t/t5523-push-upstream.sh
> +++ b/t/t5523-push-upstream.sh
> @@ -101,10 +101,11 @@ test_expect_success TTY 'push -q suppresses progress' '
> ! grep "Writing objects" err
> '
>
> -test_expect_failure TTY 'push --no-progress suppresses progress' '
> +test_expect_success TTY 'push --no-progress suppresses progress' '
> ensure_fresh_upstream &&
>
> test_terminal git push -u --no-progress upstream master >out 2>err &&
> + ! grep "Unpacking objects" err &&
> ! grep "Writing objects" err
> '
Very nice to see an old expect-failure turned into expect-success.
> diff --git a/transport.c b/transport.c
> index cac0c06..6074ee9 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -994,10 +994,14 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
> * when a rule is satisfied):
> *
> * 1. Report progress, if force_progress is 1 (ie. --progress).
> + * 2. Don't report progress, if force_progress is 0 (ie. --no-progress).
> * 2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
> * 3. Report progress if isatty(2) is 1.
> **/
I'll turn this into an unnumbered bulletted list, while I tease it apart
to remove textual dependency on the 'verify-refs' extension, which this
fix shouldn't depend on and taken hostage to.
Thanks.
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Michał Kiedrowicz @ 2012-02-13 21:10 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <201202132058.18001.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michal Kiedrowicz wrote:
> > Jakub Narebski <jnareb@gmail.com> wrote:
> > > Jakub Narebski <jnareb@gmail.com> writes:
> [...]
> > > > Anyway I have send to git mailing list a patch series, which in one
> > > > of patches adds esc_html_match_hl($str, $regexp) to highlight
> > > > matches in a string.
> >
> > Yeah, I saw that but after seeing that they accept different arguments
> > I decided to leave them alone.
> >
> > > > Your esc_html_mark_range(), after a
> > > > generalization, could be used as underlying "engine".
> > > >
> > > > Something like this, perhaps (untested):
> >
> > I think I'll leave it to you after merging both these series to
> > master :)
>
> Yes, you are right. Let's do it when we actually need it.
>
Good :)
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Michał Kiedrowicz @ 2012-02-13 21:09 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <201202131944.50886.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> wrote:
> On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:
> > Jakub Narebski <jnareb@gmail.com> wrote:
> > > Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> > I haven't found *examples* on GitHub and Trac sites, but what about
> > these ones:
> >
> > https://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795
> > http://trac.edgewall.org/changeset/10973
>
> Thanks. That is what I meant by "good examples". Perhaps they should
> be put in the commit message?
OK
>
> BTW GitHub is closed source, but we can check what algorithm does Trac
> use for diff refinement highlighting (highlighting changed portions of
> diff).
>
I think it's
http://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py
(see markup intraline_changes()).
> > > It doesn't implement LCS / diff
> > > algorithm like e.g. tkdiff does for its diff refinement highlighting,
> > > isn't it?
> >
> > I doesn't. I share the Jeff's opinion that:
> > a) Jeff's approach is "good enough"
> > b) LCS on bytes could be very confusing if it marked few sets of
> > characters.
>
> I wonder if we can use --diff-words for diff refinement highlighting,
> i.e. LCS on words.
I think we can try it, but I worry about performance of running `git
diff` on every diff chunk.
>
> Anyway Jeff's approach is a bit limited, in that it would work only for
> change that does not involve adding newlines, for example splitting
> overly long line when changing something.
>
> See for example line 1786 (in pre-image) in http://trac.edgewall.org/changeset/10973
>
Yes, I'm aware of that. I was thinking about improving it later ("Let's
start with a simple refinment highlightning and maybe later add more
sophisticated algorithms").
> > > By completly different you mean that they do not have common prefix or
> > > common suffix (at least one of them), isn't it?
>
> BTW. is it "at least one of prefix or suffix are non-empty" or "both prefix
> and suffix are non-empty"?
>
At least one. See:
-a = 42;
+b = 42;
Here prefix is empty but suffix is not.
> > I would also consider ignoring prefixes/suffixes with punctuation, like:
> >
> > - * I like you.
> > + * Alice had a little lamb.
>
> But this patch doesn't implement this feature yet, isn't it?
No, but is a matter of adding
-$prefix_is_space = 0 if ($r[$prefix] !~ /\s/);
+$prefix_is_space = 0 if ($r[$prefix] !~ /\s|[[:punct:]]/);
(and the same for suffix)
> Well, here is another idea: do not highlight if sum of prefix and suffix
> lengths are less than some threshold, e.g. 2 characters not including
> whitespace, or some percentage with respect to total line length.
>
That might be a good idea.
> > > Eeeeeek! First you split into letters, in caller at that, then join?
> > > Why not pass striung ($str suggests string not array of characters),
> > > and use substr instead?
> > >
> > > [Please disregard this and the next paragraph at first reading]
> >
> > I will rename $str to something more self describing.
>
> Please do.
>
> BTW. don't you assume here that both common prefix and common suffix
> are non-empty?
Yes. The caller makes sure they are correct (at least
> > > > +sub format_rem_add_line {
> > > > + my ($rem, $add) = @_;
> > > > + my @r = split(//, $rem);
> > > > + my @a = split(//, $add);
>
> BTW the name of variable can be just @add and @rem.
>
I know they are different scopes but I don't like it. It makes the code
more confusing IMO. But I won't insist.
> > > Shouldn't
> > > $prefix / $prefix_len start from 0, not from 1?
> >
> > It starts from 1 because it skips first +/-. It should become obvious
> > after reading the comment from last patch :).
> >
> > + # In combined diff we must ignore two +/- characters.
> > + $prefix = 2 if ($is_combined);
>
> Anyway comment about that fact would be nice.
Will do.
>
> > > > + my ($prefix_is_space, $suffix_is_space) = (1, 1);
> > > > +
> > > > + while ($prefix < @r && $prefix < @a) {
> > > > + last if ($r[$prefix] ne $a[$prefix]);
> > > > +
> > > > + $prefix_is_space = 0 if ($r[$prefix] !~ /\s/);
> > > > + $prefix++;
> > > > + }
> > >
> > > Ah, I see that it is easier to find common prefix by treating string
> > > as array of characters.
> > >
> > > Though I wonder if it wouldn't be easier to use trick of XOR-ing two
> > > strings (see "Bitwise String Operators" in perlop(1)):
> > >
> > > my $xor = "$rem" ^ "$add";
> > >
> > > and finding starting sequence of "\0", which denote common prefix.
> > >
> > >
> > > Though this and the following is a nice implementation of
> > > algorithm... as it would be implemented in C. Nevermind, it might be
> > > good enough...
> >
> > The splitting and comparing by characters is taken from diff-highlight.
> > I don't think it's worth changing here.
>
> You are right.
>
> I'll try to come with hacky algorithm using string bitwise xor and regexp,
> and benchmark it comparing to your C-like solution, but it can be left for
> later (simple is better than clever, usually).
If you have time :).
> > > > # HTML-format diff context, removed and added lines.
> > > > sub format_ctx_rem_add_lines {
> > > > - my ($ctx, $rem, $add) = @_;
> > > > + my ($ctx, $rem, $add, $is_combined) = @_;
> > > > my (@new_ctx, @new_rem, @new_add);
> > > > + my $num_add_lines = @$add;
> > >
> > > Why is this temporary variable needed? If you are not sure that ==
> > > operator enforces scalar context on both arguments you can always
> > > write
> > >
> > > scalar @$add == scalar @$rem
> >
> > You read my mind.
>
> BTW. the same comment applies to patch adding support for highlighting
> changed part in combined diff.
>
OK
^ permalink raw reply
* GitWeb and atom feed links
From: Heiko W. Rupp @ 2012-02-13 20:43 UTC (permalink / raw)
To: git; +Cc: jnareb
Hi,
( got this email from Junio )
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. 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've traced it down in my copy of gitweb to
sub git_footer_html {
...
$href_params{'action'} = lc($format);
print $cgi->a({-href => href(%href_params),
-title => "$href_params{'-title'} $format feed",
-class => $feed_class}, $format)."\n";
and here to the usage of href()
Locally I've modified that to
$href_params{'action'} = lc($format);
my $hr = "?p=$project&a=";
$hr .= lc($format);
print $cgi->a({-href => $hr,
-title => "$href_params{'-title'} $format feed",
-class => $feed_class}, $format)."\n";
and it works.
Attached is a patch against current master that reflects above change
Heiko
--
Heiko Rupp hwr@pilhuhn.de
Blog: http://javablogs.com/ViewBlog.action?id=14468
^ permalink raw reply
* Re: [BUG] git-merge-octopus creates an empty merge commit with one parent
From: Michał Kiedrowicz @ 2012-02-13 20:53 UTC (permalink / raw)
To: git
In-Reply-To: <7vpqdieclu.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster <at> pobox.com> writes:
> Well, what was missing from my "don't do it then" quote was "Doctor it
> HURTS when I do this."; there is no question that it is not correct if it
> hurts .
All right :)
^ permalink raw reply
* Re: [PATCH 1/5] git rev-list: fix invalid typecast
From: Junio C Hamano @ 2012-02-13 20:48 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
In-Reply-To: <1329164235-29955-2-git-send-email-drizzd@aon.at>
Clemens Buchacher <drizzd@aon.at> writes:
> git rev-list passes rev_list_info, not rev_list objects. Without this
> fix, rev-list enables or disables the --verify-objects option depending
> on a read from an undefined memory location.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
Wow, I simply couldn't believe it, but you are right.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Clemens Buchacher @ 2012-02-13 20:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pete Wyckoff, git, Luke Diamand, Vitor Antunes
In-Reply-To: <7vehtyec64.fsf@alter.siamese.dyndns.org>
On Mon, Feb 13, 2012 at 12:00:35PM -0800, Junio C Hamano wrote:
> Pete Wyckoff <pw@padd.com> writes:
>
> > Users install git-p4 currently by copying the git-p4 script from
> > contrib/fast-import into a local or personal bin directory, and
> > setting up an alias for "git p4" to invoke it.
>
> 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. I was told it is a
feature that git only looks in exec-path now. Maybe I should not have
believed that?
Clemens
^ permalink raw reply
* What's cooking in git.git (Feb 2012, #05; Mon, 13)
From: Junio C Hamano @ 2012-02-13 20:42 UTC (permalink / raw)
To: git
Here are the topics that have been cooking. Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.
This round mosty consists of topics to fix new features introduced in
1.7.9, in preparation for 1.7.9.1 maintenance release. There are still a
few more to come.
You can find the changes described here in the integration branches of the
repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--------------------------------------------------
[New Topics]
* bl/gitweb-project-filter (2012-02-12) 1 commit
(merged to 'next' on 2012-02-13 at 35366b8)
+ gitweb: Harden and improve $project_filter page title
An update to the new feature to "gitweb" to show list of projects for
intermediate levels in directory hierarchies, already slated for 1.7.10.
* dp/i18n-libcharset (2012-02-13) 1 commit
(merged to 'next' on 2012-02-13 at 528de77)
+ Makefile: introduce CHARSET_LIB to link with -lcharset
Some systems need to explicitly link -lcharset to get locale_charset().
* lt/pull-no-edit (2012-02-12) 1 commit
(merged to 'next' on 2012-02-13 at 352f0cb)
+ "git pull" doesn't know "--edit"
For 1.7.10 where "git merge" by default spawns the editor when it can
automatically commit to record the resulting merge.
* mh/war-on-extra-refs (2012-02-12) 7 commits
(merged to 'next' on 2012-02-13 at adb7353)
+ refs: remove the extra_refs API
+ clone: do not add alternate references to extra_refs
+ everything_local(): mark alternate refs as complete
+ fetch-pack.c: inline insert_alternate_refs()
+ fetch-pack.c: rename some parameters from "path" to "refname"
+ clone.c: move more code into the "if (refs)" conditional
+ t5700: document a failure of alternates to affect fetch
Internal API clean-up that is very cleanly done.
--------------------------------------------------
[Graduated to "master"]
* bw/inet-pton-ntop-compat (2012-02-05) 1 commit
(merged to 'next' on 2012-02-06 at 61303e6)
+ Drop system includes from inet_pton/inet_ntop compatibility wrappers
The inclusion order of header files bites Solaris again and this fixes it.
* fc/zsh-completion (2012-02-06) 3 commits
(merged to 'next' on 2012-02-06 at c94dd12)
+ completion: simplify __gitcomp and __gitcomp_nl implementations
+ completion: use ls -1 instead of rolling a loop to do that ourselves
+ completion: work around zsh option propagation bug
Fix git subcommand completion for zsh (in contrib/completion).
* jc/checkout-out-of-unborn (2012-02-06) 1 commit
(merged to 'next' on 2012-02-07 at 60eb328)
+ git checkout -b: allow switching out of an unborn branch
"checkout -b" did not allow switching out of an unborn branch.
* jc/maint-commit-ignore-i-t-a (2012-02-07) 1 commit
(merged to 'next' on 2012-02-10 at e0040cf)
+ commit: ignore intent-to-add entries instead of refusing
Replaces the nd/commit-ignore-i-t-a series that was made unnecessary
complicated by bad suggestions I made earlier.
* jc/maint-mailmap-output (2012-02-06) 1 commit
(merged to 'next' on 2012-02-06 at 0a21425)
+ mailmap: always return a plain mail address from map_user()
map_user() was not rewriting its output correctly, which resulted in the
user visible symptom that "git blame -e" sometimes showed excess '>' at
the end of email addresses.
* jk/maint-tag-show-fixes (2012-02-08) 3 commits
(merged to 'next' on 2012-02-08 at 18459c4)
+ tag: do not show non-tag contents with "-n"
+ tag: die when listing missing or corrupt objects
+ tag: fix output of "tag -n" when errors occur
Bugfixes to "git tag -n" that lacked much error checking.
* jk/prompt-fallback-to-tty (2012-02-03) 2 commits
(merged to 'next' on 2012-02-06 at c0c995a)
+ prompt: fall back to terminal if askpass fails
+ prompt: clean up strbuf usage
The code to ask for password did not fall back to the terminal input when
GIT_ASKPASS is set but does not work (e.g. lack of X with GUI askpass
helper).
* jn/gitweb-search-utf-8 (2012-02-03) 1 commit
(merged to 'next' on 2012-02-05 at 055e446)
+ gitweb: Allow UTF-8 encoded CGI query parameters and path_info
Search box in "gitweb" did not accept non-ASCII characters correctly.
* jn/merge-no-edit-fix (2012-02-09) 1 commit
(merged to 'next' on 2012-02-10 at 014eec9)
+ merge: do not launch an editor on "--no-edit $tag"
In 1.7.9, "merge --no-edit $tag" incorrectly ignored --no-edit.
* mm/empty-loose-error-message (2012-02-06) 1 commit
(merged to 'next' on 2012-02-07 at f119cac)
+ fsck: give accurate error message on empty loose object files
Updates the error message emitted when we see an empty loose object.
* mp/make-cleanse-x-for-exe (2012-02-09) 1 commit
(merged to 'next' on 2012-02-09 at 35cc89d)
+ Explicitly set X to avoid potential build breakage
The makefile allowed environment variable X seep into it result in
command names suffixed with unnecessary strings.
* nd/cache-tree-api-refactor (2012-02-07) 1 commit
(merged to 'next' on 2012-02-08 at a9abbca)
+ cache-tree: update API to take abitrary flags
Code cleanup.
* nd/diffstat-gramnum (2012-02-03) 1 commit
(merged to 'next' on 2012-02-05 at 7335ecc)
+ Use correct grammar in diffstat summary line
The commands in the "git diff" family and "git apply --stat" that count
the number of files changed and the number of lines inserted/deleted have
been updated to match the output from "diffstat". This also opens the
door to i18n this line.
* nd/find-pack-entry-recent-cache-invalidation (2012-02-01) 2 commits
(merged to 'next' on 2012-02-01 at e26aed0)
+ find_pack_entry(): do not keep packed_git pointer locally
+ sha1_file.c: move the core logic of find_pack_entry() into fill_pack_entry()
* nk/ctype-for-perf (2012-02-10) 2 commits
(merged to 'next' on 2012-02-10 at b41c6bb)
+ ctype: implement islower/isupper macro
+ ctype.c only wants git-compat-util.h
* tt/profile-build-fix (2012-02-09) 2 commits
(merged to 'next' on 2012-02-09 at 1c183af)
+ Makefile: fix syntax for older make
(merged to 'next' on 2012-02-07 at c8c5f3f)
+ Fix build problems related to profile-directed optimization
--------------------------------------------------
[Stalled]
* jc/advise-push-default (2011-12-18) 1 commit
- push: hint to use push.default=upstream when appropriate
Peff had a good suggestion outlining an updated code structure so that
somebody new can try to dip his or her toes in the development. Any
takers?
* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits
- fixup! 15eaaf4
- git-svn, perl/Git.pm: extend Git::prompt helper for querying users
- perl/Git.pm: "prompt" helper to honor GIT_ASKPASS and SSH_ASKPASS
The bottom one has been replaced with a rewrite based on comments from
Ævar. The second one needs more work, both in perl/Git.pm and prompt.c, to
give precedence to tty over SSH_ASKPASS when terminal is available.
* jc/split-blob (2012-01-24) 6 commits
- chunked-object: streaming checkout
- chunked-object: fallback checkout codepaths
- bulk-checkin: support chunked-object encoding
- bulk-checkin: allow the same data to be multiply hashed
- new representation types in the packstream
- varint-in-pack: refactor varint encoding/decoding
Not ready.
I finished the streaming checkout codepath, but as explained in 127b177
(bulk-checkin: support chunked-object encoding, 2011-11-30), these are
still early steps of a long and painful journey. At least pack-objects and
fsck need to learn the new encoding for the series to be usable locally,
and then index-pack/unpack-objects needs to learn it to be used remotely.
Given that I heard a lot of noise that people want large files, and that I
was asked by somebody at GitTogether'11 privately for an advice on how to
pay developers (not me) to help adding necessary support, I am somewhat
dissapointed that the original patch series that was sent almost two
months ago still remains here without much comments and updates from the
developer community. I even made the interface to the logic that decides
where to split chunks easily replaceable, and I deliberately made the
logic in the original patch extremely stupid to entice others, especially
the "bup" fanboys, to come up with a better logic, thinking that giving
people an easy target to shoot for, they may be encouraged to help
out. The plan is not working :-(.
* nd/columns (2012-02-08) 15 commits
- column: Fix some compiler and sparse warnings
- column: add a corner-case test to t3200
- columns: minimum coding style fixes
- tag: add --column
- column: support piping stdout to external git-column process
- status: add --column
- branch: add --column
- help: reuse print_columns() for help -a
- column: add column.ui for default column output settings
- column: support columns with different widths
- column: add columnar layout
- Stop starting pager recursively
- Add git-column and column mode parsing
- column: add API to print items in columns
- Save terminal width before setting up pager
The "show list of ..." mode of a handful of commands learn to produce
column-oriented output.
Expecting a reroll.
--------------------------------------------------
[Cooking]
* jn/ancient-meld-support (2012-02-10) 1 commit
(merged to 'next' on 2012-02-13 at 28aca31)
+ mergetools/meld: Use --help output to detect --output support
More reliably tell if the given version of "meld" supports --output
option.
* jk/config-include (2012-02-06) 2 commits
(merged to 'next' on 2012-02-13 at 307ddf6)
+ config: add include directive
+ docs: add a basic description of the config API
An assignment to the include.path pseudo-variable causes the named file
to be included in-place when Git looks up configuration variables.
* jk/userdiff-config-simplify (2012-02-07) 1 commit
(merged to 'next' on 2012-02-10 at e9854c1)
+ drop odd return value semantics from userdiff_config
Code cleanup.
* tg/tag-points-at (2012-02-13) 2 commits
(merged to 'next' on 2012-02-13 at a8f4046)
+ builtin/tag.c: Fix a sparse warning
(merged to 'next' on 2012-02-10 at 4bff88f)
+ tag: add --points-at list option
* jl/maint-submodule-relative (2012-02-09) 2 commits
- submodules: always use a relative path from gitdir to work tree
- submodules: always use a relative path to gitdir
The second one looked iffy.
* ld/git-p4-expanded-keywords (2012-02-09) 2 commits
- git-p4: initial demonstration of possible RCS keyword fixup
- git-p4: add test case for RCS keywords
Waiting for reviews and user reports.
* jk/grep-binary-attribute (2012-02-02) 9 commits
(merged to 'next' on 2012-02-05 at 9dffa7e)
+ grep: pre-load userdiff drivers when threaded
+ grep: load file data after checking binary-ness
+ grep: respect diff attributes for binary-ness
+ grep: cache userdiff_driver in grep_source
+ grep: drop grep_buffer's "name" parameter
+ convert git-grep to use grep_source interface
+ grep: refactor the concept of "grep source" into an object
+ grep: move sha1-reading mutex into low-level code
+ grep: make locking flag global
Fixes a longstanding bug that there was no way to tell "git grep" that a
path may look like text but it is not, which "git diff" can do using the
attributes system. Now "git grep" honors the same "binary" (or "-diff")
attribute.
Will merge to 'master' after 1.7.9.1 settles.
* jk/git-dir-lookup (2012-02-02) 1 commit
(merged to 'next' on 2012-02-05 at 1856d74)
+ standardize and improve lookup rules for external local repos
When you have both .../foo and .../foo.git, "git clone .../foo" did not
favor the former but the latter.
Will merge to 'master' after 1.7.9.1 settles.
* nd/pack-objects-parseopt (2012-02-01) 3 commits
(merged to 'next' on 2012-02-05 at d0dc25d)
+ pack-objects: convert to use parse_options()
+ pack-objects: remove bogus comment
+ pack-objects: do not accept "--index-version=version,"
"pack-objects" learned use parse-options, losing custom command line
parsing code.
Will merge to 'master' after 1.7.9.1 settles.
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Junio C Hamano @ 2012-02-13 20:29 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Tom Grennan, Albert Yale, pclouds, git, krh, jasampler
In-Reply-To: <4F391F5C.1000400@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
>> Having said all that, if your argument against using "^" as negation for
>> for-each-ref *were* with something like this from the beginning:
>>
>> git rev-list --all --exclude-refs=refs/tags/v\*
>>
>> it would have been very different. I would wholeheartedly buy the
>> consistency argument that says
>>
>> git for-each-ref --exclude-refs=refs/tags/v\*
>>
>> ought to give all refs (only because for-each-ref "all" is implied) except
>> for the tagged tips, and
>>
>> git log --all --exclude-refs=refs/tags/v\*
>>
>> should be the notation to produce consistently the same result as
>>
>> git log $(git for-each-ref --format='%(objectname)' --exclude-refs=refs/tags/v\*)
>>
>> but if we used "^" as negated match in for-each-ref argument, we would
>> close the door to give such consistency to log family of commands later.
>
> That *has* been exactly my argument from the beginning [1].
Well, if the only thing you say is "rev-list A B ^C" and you are expecting
that your reader to substitute A with any dashed option like --all, --not
or --stdin, I would have to say you are expecting too much. I wouldn't
even think of substituting A with $(git for-each-ref refs/heads/) in such
an example.
> I cautiously hope that we are now talking about the same thing, even if it
> is not yet clear whether we agree on a conclusion.
I think we are on the same page now. Given that we seem to have settled
for the recent "find in the paths that pathspec matches, but exclude
matches from paths that match these patterns" topic by Albert Yale to
tentatively use separate --exclude [*1*] command line option instead of
mixing the negatives to the usual list of positives at the UI level, it
appears to me that the most sensible way forward would be to expose the
negative match to the UI level is to introduce a similar --exclude-refs on
the command line.
That would eventually allow us to say something like [*2*]:
git log --all \
--exclude-refs=refs/heads/experimental/* \
--exclude-paths=compat/ \
--since=30.days \
-- '*.c'
to ask for recent changes to all C sources outside the compat/ area for
everything except for the experimental topics.
[Footnote]
*1* It might be better to spell this as --exclude-paths, though, if we are
going to call this other exclude-refs-by-pattern --exclude-refs. The
longer names would not bother us with the help of parse-options, and the
commands that need to support both, namely the commands in the log family,
need to allow users to be explicit which exclusion they want anyway, and
having --exclude vs --exclude-refs as options that work on different
dimensions look asymmetric.
*2* "something like" includes using a convenience short-hand on this
command line such as "--branches --exclude-branches=experimental/*"
instead of "--all --exclude-refs=...", but I consider that is icing on the
cake after the first step to define the overall structure settles, so I
would prefer not to go into the tangential details.
^ permalink raw reply
* [PATCH 2/5] do not override receive-pack errors
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>
Receive runs rev-list --verify-objects in order to detect missing
objects. However, such errors are ignored and overridden later.
Instead, consequently ignore all update commands for which an error has
already been detected.
Some tests in t5504 are obsoleted by this change, because invalid
objects are detected even if fsck is not enabled. Instead, they now test
for different error messages depending on whether or not fsck is turned
on. A better fix would be to force a corruption that will be detected by
fsck but not by rev-list.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
builtin/receive-pack.c | 24 +++++++++++++++++-------
t/t5504-fetch-receive-strict.sh | 22 ++++++++++++++++++----
2 files changed, 35 insertions(+), 11 deletions(-)
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);
+ }
string_list_clear(&ref_list, 0);
}
@@ -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);
+ }
}
static struct command *read_head_info(void)
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 8341fc4..35ec294 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -58,6 +58,11 @@ test_expect_success 'fetch with transfer.fsckobjects' '
)
'
+cat >exp <<EOF
+To dst
+! refs/heads/master:refs/heads/test [remote rejected] (missing necessary objects)
+EOF
+
test_expect_success 'push without strict' '
rm -rf dst &&
git init dst &&
@@ -66,7 +71,8 @@ test_expect_success 'push without strict' '
git config fetch.fsckobjects false &&
git config transfer.fsckobjects false
) &&
- git push dst master:refs/heads/test
+ test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+ test_cmp exp act
'
test_expect_success 'push with !receive.fsckobjects' '
@@ -77,9 +83,15 @@ test_expect_success 'push with !receive.fsckobjects' '
git config receive.fsckobjects false &&
git config transfer.fsckobjects true
) &&
- git push dst master:refs/heads/test
+ test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+ test_cmp exp act
'
+cat >exp <<EOF
+To dst
+! refs/heads/master:refs/heads/test [remote rejected] (n/a (unpacker error))
+EOF
+
test_expect_success 'push with receive.fsckobjects' '
rm -rf dst &&
git init dst &&
@@ -88,7 +100,8 @@ test_expect_success 'push with receive.fsckobjects' '
git config receive.fsckobjects true &&
git config transfer.fsckobjects false
) &&
- test_must_fail git push dst master:refs/heads/test
+ test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+ test_cmp exp act
'
test_expect_success 'push with transfer.fsckobjects' '
@@ -98,7 +111,8 @@ test_expect_success 'push with transfer.fsckobjects' '
cd dst &&
git config transfer.fsckobjects true
) &&
- test_must_fail git push dst master:refs/heads/test
+ test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+ test_cmp exp act
'
test_done
--
1.7.9
^ permalink raw reply related
* [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>
By default, progress output is disabled if stderr is not a terminal.
The --progress option can be used to force progress output anyways.
Conversely, --no-progress does not force progress output. In particular,
if stderr is a terminal, progress output is enabled.
This is unintuitive. Change --no-progress to force output off.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
builtin/clone.c | 6 +++---
builtin/fetch-pack.c | 2 +-
builtin/fetch.c | 4 ++--
builtin/push.c | 4 ++--
builtin/send-pack.c | 12 +++++++-----
t/t5523-push-upstream.sh | 3 ++-
transport.c | 6 +++++-
7 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index c62d4b5..09dbb09 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -45,7 +45,7 @@ static char *option_branch = NULL;
static const char *real_git_dir;
static char *option_upload_pack = "git-upload-pack";
static int option_verbosity;
-static int option_progress;
+static int option_progress = -1;
static struct string_list option_config;
static struct string_list option_reference;
@@ -60,8 +60,8 @@ static int opt_parse_reference(const struct option *opt, const char *arg, int un
static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
- OPT_BOOLEAN(0, "progress", &option_progress,
- "force progress reporting"),
+ OPT_BOOL(0, "progress", &option_progress,
+ "force progress reporting"),
OPT_BOOLEAN('n', "no-checkout", &option_no_checkout,
"don't create a checkout"),
OPT_BOOLEAN(0, "bare", &option_bare, "create a bare repository"),
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 6207ecd..a4d3e90 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -736,7 +736,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
}
else {
*av++ = "unpack-objects";
- if (args.quiet)
+ if (args.quiet || args.no_progress)
*av++ = "-q";
}
if (*hdr_arg)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ab18633..65f5f9b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -30,7 +30,7 @@ enum {
};
static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
-static int progress, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static int tags = TAGS_DEFAULT;
static const char *depth;
static const char *upload_pack;
@@ -78,7 +78,7 @@ static struct option builtin_fetch_options[] = {
OPT_BOOLEAN('k', "keep", &keep, "keep downloaded pack"),
OPT_BOOLEAN('u', "update-head-ok", &update_head_ok,
"allow updating of HEAD ref"),
- OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+ OPT_BOOL(0, "progress", &progress, "force progress reporting"),
OPT_STRING(0, "depth", &depth, "depth",
"deepen history of shallow clone"),
{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, "dir",
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..6c373cf 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -19,7 +19,7 @@ static int thin;
static int deleterefs;
static const char *receivepack;
static int verbosity;
-static int progress;
+static int progress = -1;
static const char **refspec;
static int refspec_nr;
@@ -260,7 +260,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
TRANSPORT_PUSH_SET_UPSTREAM),
- OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+ OPT_BOOL(0, "progress", &progress, "force progress reporting"),
OPT_END()
};
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7d514ca..8fc9789 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -58,7 +58,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
argv[i++] = "--thin";
if (args->use_ofs_delta)
argv[i++] = "--delta-base-offset";
- if (args->quiet)
+ if (args->quiet || !args->progress)
argv[i++] = "-q";
if (args->progress)
argv[i++] = "--progress";
@@ -250,6 +250,7 @@ int send_pack(struct send_pack_args *args,
int allow_deleting_refs = 0;
int status_report = 0;
int use_sideband = 0;
+ int quiet_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
@@ -263,8 +264,8 @@ int send_pack(struct send_pack_args *args,
args->use_ofs_delta = 1;
if (server_supports("side-band-64k"))
use_sideband = 1;
- if (!server_supports("quiet"))
- args->quiet = 0;
+ if (server_supports("quiet"))
+ quiet_supported = 1;
if (server_supports("verify-refs"))
args->verify_refs = 1;
@@ -304,13 +305,14 @@ int send_pack(struct send_pack_args *args,
} else {
char *old_hex = sha1_to_hex(ref->old_sha1);
char *new_hex = sha1_to_hex(ref->new_sha1);
+ int quiet = quiet_supported && (args->quiet || !args->progress);
- if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
+ if (!cmds_sent && (status_report || use_sideband || 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" : "",
+ quiet ? " quiet" : "",
args->verify_refs ? " verify-refs" : "");
}
else
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 9ee52cf..3683df1 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -101,10 +101,11 @@ test_expect_success TTY 'push -q suppresses progress' '
! grep "Writing objects" err
'
-test_expect_failure TTY 'push --no-progress suppresses progress' '
+test_expect_success TTY 'push --no-progress suppresses progress' '
ensure_fresh_upstream &&
test_terminal git push -u --no-progress upstream master >out 2>err &&
+ ! grep "Unpacking objects" err &&
! grep "Writing objects" err
'
diff --git a/transport.c b/transport.c
index cac0c06..6074ee9 100644
--- a/transport.c
+++ b/transport.c
@@ -994,10 +994,14 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
* when a rule is satisfied):
*
* 1. Report progress, if force_progress is 1 (ie. --progress).
+ * 2. Don't report progress, if force_progress is 0 (ie. --no-progress).
* 2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
* 3. Report progress if isatty(2) is 1.
**/
- transport->progress = force_progress || (verbosity >= 0 && isatty(2));
+ if (force_progress >= 0)
+ transport->progress = !!force_progress;
+ else
+ transport->progress = verbosity >= 0 && isatty(2);
}
int transport_push(struct transport *transport,
--
1.7.9
^ permalink raw reply related
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