* Re: [PATCH 3/5] git push: verify refs early
From: Clemens Buchacher @ 2012-02-14 8:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, spearce
In-Reply-To: <7v4nuucrbm.fsf@alter.siamese.dyndns.org>
On Mon, Feb 13, 2012 at 02:16:13PM -0800, Junio C Hamano wrote:
> 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?
That is true. However, my assumption was that the overhead is greater
for HTTP, because the native protocol is full-duplex, while HTTP tears
down the connection and starts from scratch with each request. But to be
honest, I am not confident that this assumption is correct.
So, the stall might be an issue for both the native and the HTTP
protocol, or for neither. We should probably find out and then decide
whether to make this change for both protocols or not at all.
> 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.
Indeed. I wish we could make the ref validation asynchronous. The client
would start sending object data right away, while listening for an
"abort" command on the side-band. But if I understand correctly, that is
not possible for HTTP.
^ permalink raw reply
* Re: Setting up a Git server (+ gitweb) with .htaccess files HOWTO
From: Matthieu Moy @ 2012-02-14 8:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1upyft1b.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> After seeing the section that runs "git init" in a throw-away CGI script,
> I started wondering what the point of this site in forbidding a shell
> access in the first place.
The sysadmin trusts users enough to allow running arbitrary CGI there.
Not giving shell access greatly limits the accidental mis-uses of the
server, or silly attacks by incompetent users (i.e. "students" ;-) ). A
few years ago, people had shell access on most servers, and they were
using it to run seti@home & other heavy stuff there.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 2/5] do not override receive-pack errors
From: Clemens Buchacher @ 2012-02-14 8:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8vk6csx9.fsf@alter.siamese.dyndns.org>
On Mon, Feb 13, 2012 at 01:41:38PM -0800, Junio C Hamano wrote:
> 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"?
Yes, it really is. For example, in t5504 rev-list --verify-objects (it
was turned on for me if called from there) detects the corrupt object.
But the error string is later overwritten with the return value of
update, which is NULL in this case.
That is why I had to change the t5504 tests from a successful git push
to a test_must_fail git push with this fix. To keep the previous
behavior we would have to replace the corrupt blob with a more subtle
corruption that rev-list --verify-objects would not detect, but fsck
would (e.g., a malformed commit header).
> > 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);
> > + }
>
[...]
> 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.
Actually, check_alias_update searches for aliases of cmd in ref_list,
which is a list of refs from all commands, irrespective of their error
status. So this change is correct.
However, after re-reading the code I now have the impression that the
alias detection is not entirely correct. It does find aliases between
symrefs and regular refs. But it does not find aliases between two
symrefs, because ref_list will not contain the actual ref pointed to,
and therefore the code considers neither symref an alias.
But that is independent of the hunk above.
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Jeff King @ 2012-02-14 8:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michal Kiedrowicz, git, Jakub Narebski
In-Reply-To: <7vpqdh999t.fsf@alter.siamese.dyndns.org>
On Mon, Feb 13, 2012 at 11:14:22PM -0800, Junio C Hamano wrote:
> > We could tech diff-highlight to produce diffs
> > marked with -{} and +{} (this is the notation used by Jeff in one of his
> > recent patches) or something like this and then just convert that into
> > HTML markup.
>
> this implementation strategy would not work well, given that the payload
> can contain arbitrary letter sequence (e.g. a Perl script that wants be
> explicit when writing a hashref literal write +{...} to disambiguate it
> from a block). If you are going to modularize diff-highlight and reuse
> it, it needs to learn how to talk HTML to properly escape the payload.
They're both written in perl; perhaps a more sensible solution would be
to lib-ify diff-highlight and use it directly inside gitweb.
-Peff
^ permalink raw reply
* An incremental update to "What's cooking"
From: Junio C Hamano @ 2012-02-14 7:22 UTC (permalink / raw)
To: git
In-Reply-To: <7v4nuuea7r.fsf@alter.siamese.dyndns.org>
Here is the tonight's snapshot as an incremental update to the issue #5
of "What's cooking" for this month.
I'd like to merge jn/merge-no-edit-fix topic to 'maint' and release the
first maintenance release 1.7.9.1 soonish. The topic is merged to 'master'
already, and it is very much appreciated if people can test and eyeball it
to make sure we fixed what is broken in the vanilla 1.7.9 release without
causing regression in unexpected places.
Thanks, and goodnight.
----------------------------------------------------------------
Born topics
[New Topics]
* cb/maint-rev-list-verify-object (2012-02-13) 1 commit
- git rev-list: fix invalid typecast
Fixes an obscure bug in "rev-list --verify" that skipped verification
depending on the phase of the moon, which dates back to 1.7.8.x series.
* cb/maint-t5541-make-server-port-portable (2012-02-13) 1 commit
- t5541: check error message against the real port number used
Test fix.
* cb/receive-pack-keep-errors (2012-02-13) 1 commit
- do not override receive-pack errors
One hunk and the word "override" in the description were a bit iffy.
* cb/transfer-no-progress (2012-02-13) 1 commit
- push/fetch/clone --no-progress suppresses progress output
The transport programs semi-ignored --no-progress and showed progress when
sending their output to a terminal.
* jk/diff-highlight (2012-02-13) 5 commits
- diff-highlight: document some non-optimal cases
- diff-highlight: match multi-line hunks
- diff-highlight: refactor to prepare for multi-line hunks
- diff-highlight: don't highlight whole lines
- diff-highlight: make perl strict and warnings fatal
Updates diff-highlight (in contrib/).
* zj/decimal-width (2012-02-13) 1 commit
- (sign-off???) make lineno_width() from blame reusable for others
(this branch is used by zj/diff-stat-dyncol.)
Refactoring.
* zj/term-columns (2012-02-13) 1 commit
- pager: find out the terminal width before spawning the pager
(this branch is used by zj/diff-stat-dyncol.)
Fixes "git -p cmd" for any subcommand that cares about the true terminal
width.
* hv/submodule-recurse-push (2012-02-13) 3 commits
- push: teach --recurse-submodules the on-demand option
- Refactor submodule push check to use string list instead of integer
- Teach revision walking machinery to walk multiple times sequencially
The bottom one was not clearly explained.
* zj/diff-stat-dyncol (2012-02-13) 2 commits
- diff --stat: use the full terminal width
- Merge branch 'zj/term-columns' into zj/diff-stat-dyncol
(this branch uses zj/decimal-width and zj/term-columns.)
This breaks tests. Perhaps it is not worth using the decimal-width stuff
for this series, at least initially.
--------------------------------------------------
[Cooking]
-* ld/git-p4-expanded-keywords (2012-02-09) 2 commits
+* ld/git-p4-expanded-keywords (2012-02-13) 3 commits
+ - git-p4: more RCS tests
- git-p4: initial demonstration of possible RCS keyword fixup
- git-p4: add test case for RCS keywords
-Waiting for reviews and user reports.
+Waiting for the dust to settle.
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Junio C Hamano @ 2012-02-14 7:14 UTC (permalink / raw)
To: Michal Kiedrowicz; +Cc: git, Jakub Narebski, Jeff King
In-Reply-To: <20120214075439.14f1d2b7@mkiedrowicz.ivo.pl>
Michal Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> I just started to wonder if we couldn't use output from Jeff's
> diff-highlight for gitweb.
That could be a sensible approach, but
> We could tech diff-highlight to produce diffs
> marked with -{} and +{} (this is the notation used by Jeff in one of his
> recent patches) or something like this and then just convert that into
> HTML markup.
this implementation strategy would not work well, given that the payload
can contain arbitrary letter sequence (e.g. a Perl script that wants be
explicit when writing a hashref literal write +{...} to disambiguate it
from a block). If you are going to modularize diff-highlight and reuse
it, it needs to learn how to talk HTML to properly escape the payload.
^ permalink raw reply
* Re: [PATCH 5/5] diff-highlight: document some non-optimal cases
From: Michal Kiedrowicz @ 2012-02-14 6:48 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120213223733.GE19521@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> 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.
I would also add (feel free to reword if my English is not very clear):
3. Sometimes the prefix or suffix is very small but because it exists,
almost whole line is highlighted. For example:
----------------------------------------------
--{This is a very long line}.
++{I like apples}.
----------------------------------------------
or:
----------------------------------------------
-Th-{is is an apple}
+Th+{at was a car}
----------------------------------------------
>
> 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.
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Michal Kiedrowicz @ 2012-02-14 6:54 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Jeff King
In-Reply-To: <m339aivn4z.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > The code that comares lines is based on
> > contrib/diff-highlight/diff-highlight, except that it works with
> > multiline changes too. It also won't highlight lines that are
> > completely different because that would only make the output
> > unreadable. Combined diffs are not supported but a following commit
> > will change it.
>
> I was thinking that if gitweb were to support "diff refinement
> highlighting", it would either use one of *Diff* packages from CPAN,
> or "git diff --word-diff" output.
>
I just started to wonder if we couldn't use output from Jeff's
diff-highlight for gitweb. We could tech diff-highlight to produce diffs
marked with -{} and +{} (this is the notation used by Jeff in one of his
recent patches) or something like this and then just convert that into
HTML markup. Changes in gitweb would be minimal, we would reduce
redundancy and could focus on improving matching algorithm in one place.
^ permalink raw reply
* Re: [PATCH 2/5] diff-highlight: don't highlight whole lines
From: Michal Kiedrowicz @ 2012-02-14 6:35 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120213223247.GB19521@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> 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 :) ).
I'm completely OK with that. I don't care much about shortlog :). It's
nice enough to see that you liked my idea :)
^ permalink raw reply
* Re: [PATCH] diff-highlight: Work for multiline changes too
From: Michal Kiedrowicz @ 2012-02-14 6:28 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120213222702.GA19393@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> 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.
Yeah, I completely agree with that.
> 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.
Very nicely done.
>
> > 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] diff-highlight: Work for multiline changes too
From: Jeff King @ 2012-02-14 6:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michał Kiedrowicz, git
In-Reply-To: <7vk43q9pp6.fsf@alter.siamese.dyndns.org>
On Mon, Feb 13, 2012 at 05:19:33PM -0800, Junio C Hamano wrote:
> 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.
I find black-on-white ugly, too, but I have heard some people find it
more readable. You might try setting color.diff.old to "bold red" to
make it more readable. Depending on your terminal, that may end up as a
brighter shade of red.
Configurable colors for diff-highlight would look like the patch below:
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index c4404d4..f43832b 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -2,11 +2,13 @@
use warnings FATAL => 'all';
use strict;
+use Git;
+
+my $repo = Git->repository;
+my $color_old = $repo->get_color('color.diff.highlightold', 'reverse');
+my $color_new = $repo->get_color('color.diff.highlightnew', 'reverse');
+my $color_end = $repo->get_color('color.diff.highlightend', 'noreverse');
-# Highlight by reversing foreground and background. You could do
-# other things like bold or underline if you prefer.
-my $HIGHLIGHT = "\x1b[7m";
-my $UNHIGHLIGHT = "\x1b[27m";
my $COLOR = qr/\x1b\[[0-9;]*m/;
my $BORING = qr/$COLOR|\s/;
@@ -128,8 +130,8 @@ sub highlight_pair {
}
if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
- return highlight_line(\@a, $pa, $sa),
- highlight_line(\@b, $pb, $sb);
+ return highlight_line(\@a, $pa, $sa, $color_old, $color_end),
+ highlight_line(\@b, $pb, $sb, $color_new, $color_end);
}
else {
return join('', @a),
@@ -144,13 +146,13 @@ sub split_line {
}
sub highlight_line {
- my ($line, $prefix, $suffix) = @_;
+ my ($line, $prefix, $suffix, $highlight, $unhighlight) = @_;
return join('',
@{$line}[0..($prefix-1)],
- $HIGHLIGHT,
+ $highlight,
@{$line}[$prefix..$suffix],
- $UNHIGHLIGHT,
+ $unhighlight,
@{$line}[($suffix+1)..$#$line]
);
}
However, there are two problems:
1. Git's color-parsing support does not understand the "noreverse"
attribute. There is no way to turn off attributes short of doing a
whole "reset". But we don't want to do that here, because we want
whatever other colors were in effect to continue after the
highlight ends. The patch to teach "noreverse" is below (though it
should probably also teach "normal" and "noblink", too).
2. The Git.pm:get_color code requires that we have a repository
object, which means we will die() if we are not in a git
repository. Yet "git config" will do the right thing whether we are
in a repository or not. I would have thought all of the _maybe_self
stuff in Git.pm would handle "Git->get_color" properly, but it
doesn't. That's probably a bug that should be fixed.
I don't especially care about this feature, as I won't use it. But if
you are interested in using diff-highlight and the lack of configurable
colors is blocking you, then I at least know there will be one user and
I don't mind putting a little bit of time into it.
Here's the patch for (1) above.
diff --git a/color.c b/color.c
index e8e2681..b0f53a7 100644
--- a/color.c
+++ b/color.c
@@ -47,9 +47,9 @@ static int parse_color(const char *name, int len)
static int parse_attr(const char *name, int len)
{
- static const int attr_values[] = { 1, 2, 4, 5, 7 };
+ static const int attr_values[] = { 1, 2, 4, 5, 7, 27 };
static const char * const attr_names[] = {
- "bold", "dim", "ul", "blink", "reverse"
+ "bold", "dim", "ul", "blink", "reverse", "noreverse"
};
int i;
for (i = 0; i < ARRAY_SIZE(attr_names); i++) {
@@ -128,7 +128,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,
attr &= ~bit;
if (sep++)
*dst++ = ';';
- *dst++ = '0' + i;
+ dst += sprintf(dst, "%d", i);
}
if (fg >= 0) {
if (sep++)
^ permalink raw reply related
* git-cherry filter
From: Neal Kreitzinger @ 2012-02-14 4:20 UTC (permalink / raw)
To: git
Is there a way to add a pre-git-patch-id filter to git-cherry?
e.g.,
(a) perform "keyword contraction" to the patch before generating the
git-patch-id.
e.g. I want to run a git-cherry to see if two patches are identical other
than keyword expansion values like $User: foo$ vs. $User: bar$. (I would
have to tell git-cherry which keyword formats to "contract".)
(b) ignore comments in the source code.
e.g. I want to run a git-cherry to see if the patches are identical in
regards to executable source. (I would have to tell git-cherry what the
comment rules are for the various source files.)
(c) exclude certain files from the diff (ie., binaries, comment files,
etc.).
e.g. I want to run a git-cherry to see which source code fixes from the old
system have already been applied to the new system regardless of whether
certain files differ (ie., binaries (ie, compile date), comment files (ie.,
fixed a typo), etc.). (I would have to tell git-cherry which
paths/filenames to disregard.)
I suppose these may be git-patch-id options that are passed via git-cherry
like the git-fetch options passable via git-pull.
v/r,
neal
^ permalink raw reply
* Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option
From: Junio C Hamano @ 2012-02-14 3:34 UTC (permalink / raw)
To: Heiko Voigt; +Cc: git, Fredrik Gustafsson, Jens Lehmann
In-Reply-To: <20120213093008.GD15585@t1405.greatnet.de>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> diff --git a/submodule.c b/submodule.c
> index 3c714c2..ff0cfd8 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],
> return needs_pushing->nr;
> }
>
> +static int push_submodule(const char *path)
> +{
> + if (add_submodule_odb(path))
> + return 1;
> +
> + if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
> + struct child_process cp;
> + const char *argv[] = {"push", NULL};
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.argv = argv;
> + cp.env = local_repo_env;
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + if (run_command(&cp))
> + return 0;
> + close(cp.out);
> + }
> +
> + return 1;
> +}
Hmm, this makes me wonder if we fire subprocesses and have them run in
parallel (to a reasonably limited parallelism), it might make the overall
user experience more pleasant, and if we did the same on the fetching
side, it would be even nicer.
We would need to keep track of children and after firing a handful of them
we would need to start waiting for some to finish and collect their exit
status before firing more, and at the end we would need to wait for the
remaining ones and find how each one of them did before returning from
push_unpushed_submodules(). If we were to do so, what are the missing
support we would need from the run_command() subsystem?
> +int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)
> +{
> + int i, ret = 1;
> + struct string_list needs_pushing;
> +
> + memset(&needs_pushing, 0, sizeof(struct string_list));
> + needs_pushing.strdup_strings = 1;
> +
> + if (!check_submodule_needs_pushing(new_sha1, remotes_name, &needs_pushing))
> + return 1;
> +
> + for (i = 0; i < needs_pushing.nr; i++) {
> + const char *path = needs_pushing.items[i].string;
> + fprintf(stderr, "Pushing submodule '%s'\n", path);
> + if (!push_submodule(path)) {
> + fprintf(stderr, "Unable to push submodule '%s'\n", path);
> + ret = 0;
> + }
> + }
^ permalink raw reply
* 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
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