* [PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper
From: Junio C Hamano @ 2008-11-29 21:16 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Jakub Narebski, Petr Baudis
In-Reply-To: <7vzljiz1qn.fsf@gitster.siamese.dyndns.org>
The gitweb_get_feature() function retrieves the configuration parameters
for the feature (such as the list of snapshot formats or the list of
additional actions), but it is very often used to see if feature is
enabled (which is returned as the first element in the list).
Because accepting the returned list in a scalar context by mistake yields
the number of elements in the array, which is non-zero in all cases, such
a mistake would result in a bug for the latter use, with disabled features
appearing enabled. All existing callers that call the function for this
purpose assign the return value in the list context to retrieve the first
element, but that is only because we fixed careless callers recently.
This add gitweb_check_feature() as a wrapper to gitweb_get_feature() that
can be called safely in the scalar context to see if a feature is enabled
to reduce the risk of future bugs.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Again, this is to demonstrate how I would have liked to see your
patches as a reviewable series. Notice how this naturally justifies
the dropping of parentheses in many lines that begin with "my", and
makes it easier to review? You can review the patch easily by knowing
that callers that have s/get/check/ are now safe to use scalar context.
gitweb/gitweb.perl | 56 +++++++++++++++++++++++++++++++++------------------
1 files changed, 36 insertions(+), 20 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7ab16cd..acc4cfd 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -190,7 +190,7 @@ our %feature = (
# if there is no 'sub' key (no feature-sub), then feature cannot be
# overriden
#
- # use gitweb_get_feature(<feature>) to check if <feature> is enabled
+ # use gitweb_check_feature(<feature>) to check if <feature> is enabled
# Enable the 'blame' blob view, showing the last commit that modified
# each line in the file. This can be very CPU-intensive.
@@ -344,6 +344,22 @@ sub gitweb_get_feature {
return $sub->(@defaults);
}
+# A wrapper to check if a given feature is enabled.
+# With this, you can say
+#
+# my $bool_feat = gitweb_check_feature('bool_feat');
+# gitweb_check_feature('bool_feat') or somecode;
+#
+# instead of
+#
+# my ($bool_feat) = gitweb_get_feature('bool_feat');
+# (gitweb_get_feature('bool_feat'))[0] or somecode;
+#
+sub gitweb_check_feature {
+ return (gitweb_get_feature(@_))[0];
+}
+
+
sub feature_blame {
my ($val) = git_get_project_config('blame', '--bool');
@@ -810,7 +826,7 @@ sub href (%) {
}
}
- my ($use_pathinfo) = gitweb_get_feature('pathinfo');
+ my $use_pathinfo = gitweb_check_feature('pathinfo');
if ($use_pathinfo) {
# try to put as many parameters as possible in PATH_INFO:
# - project name
@@ -2101,7 +2117,7 @@ sub git_get_projects_list {
$filter ||= '';
$filter =~ s/\.git$//;
- my ($check_forks) = gitweb_get_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
if (-d $projects_list) {
# search in directory
@@ -2947,7 +2963,7 @@ EOF
}
print "</div>\n";
- my ($have_search) = gitweb_get_feature('search');
+ my $have_search = gitweb_check_feature('search');
if (defined $project && $have_search) {
if (!defined $searchtext) {
$searchtext = "";
@@ -2961,7 +2977,7 @@ EOF
$search_hash = "HEAD";
}
my $action = $my_uri;
- my ($use_pathinfo) = gitweb_get_feature('pathinfo');
+ my $use_pathinfo = gitweb_check_feature('pathinfo');
if ($use_pathinfo) {
$action .= "/".esc_url($project);
}
@@ -3454,7 +3470,7 @@ sub is_patch_split {
sub git_difftree_body {
my ($difftree, $hash, @parents) = @_;
my ($parent) = $parents[0];
- my ($have_blame) = gitweb_get_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
print "<div class=\"list_head\">\n";
if ($#{$difftree} > 10) {
print(($#{$difftree} + 1) . " files changed:\n");
@@ -3914,7 +3930,7 @@ sub fill_project_list_info {
my ($projlist, $check_forks) = @_;
my @projects;
- my ($show_ctags) = gitweb_get_feature('ctags');
+ my $show_ctags = gitweb_check_feature('ctags');
PROJECT:
foreach my $pr (@$projlist) {
my (@activity) = git_get_last_activity($pr->{'path'});
@@ -3968,7 +3984,7 @@ sub git_project_list_body {
# actually uses global variable $project
my ($projlist, $order, $from, $to, $extra, $no_header) = @_;
- my ($check_forks) = gitweb_get_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
my @projects = fill_project_list_info($projlist, $check_forks);
$order ||= $default_projects_order;
@@ -3988,7 +4004,7 @@ sub git_project_list_body {
@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @projects;
}
- my ($show_ctags) = gitweb_get_feature('ctags');
+ my $show_ctags = gitweb_check_feature('ctags');
if ($show_ctags) {
my %ctags;
foreach my $p (@projects) {
@@ -4428,7 +4444,7 @@ sub git_summary {
my @taglist = git_get_tags_list(16);
my @headlist = git_get_heads_list(16);
my @forklist;
- my ($check_forks) = gitweb_get_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
if ($check_forks) {
@forklist = git_get_projects_list($project);
@@ -4457,7 +4473,7 @@ sub git_summary {
}
# Tag cloud
- my ($show_ctags) = gitweb_get_feature('ctags');
+ my $show_ctags = gitweb_check_feature('ctags');
if ($show_ctags) {
my $ctags = git_get_project_ctags($project);
my $cloud = git_populate_project_tagcloud($ctags);
@@ -4559,7 +4575,7 @@ sub git_blame {
my $fd;
my $ftype;
- gitweb_get_feature('blame')[0]
+ gitweb_check_feature('blame')
or die_error(403, "Blame view not allowed");
die_error(400, "No file name given") unless $file_name;
@@ -4747,7 +4763,7 @@ sub git_blob {
$expires = "+1d";
}
- my ($have_blame) = gitweb_get_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
or die_error(500, "Couldn't cat $file_name, $hash");
my $mimetype = blob_mimetype($fd, $file_name);
@@ -4840,7 +4856,7 @@ sub git_tree {
my $ref = format_ref_marker($refs, $hash_base);
git_header_html();
my $basedir = '';
- my ($have_blame) = gitweb_get_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
if (defined $hash_base && (my %co = parse_commit($hash_base))) {
my @views_nav = ();
if (defined $file_name) {
@@ -5610,7 +5626,7 @@ sub git_history {
}
sub git_search {
- gitweb_get_feature('search')[0] or die_error(403, "Search is disabled");
+ gitweb_check_feature('search') or die_error(403, "Search is disabled");
if (!defined $searchtext) {
die_error(400, "Text field is empty");
}
@@ -5629,11 +5645,11 @@ sub git_search {
if ($searchtype eq 'pickaxe') {
# pickaxe may take all resources of your box and run for several minutes
# with every query - so decide by yourself how public you make this feature
- gitweb_get_feature('pickaxe')[0]
+ gitweb_check_feature('pickaxe')
or die_error(403, "Pickaxe is disabled");
}
if ($searchtype eq 'grep') {
- gitweb_get_feature('grep')[0]
+ gitweb_check_feature('grep')
or die_error(403, "Grep is disabled");
}
@@ -5838,7 +5854,7 @@ insensitive).</p>
<dt><b>commit</b></dt>
<dd>The commit messages and authorship information will be scanned for the given pattern.</dd>
EOT
- my ($have_grep) = gitweb_get_feature('grep');
+ my $have_grep = gitweb_check_feature('grep');
if ($have_grep) {
print <<EOT;
<dt><b>grep</b></dt>
@@ -5855,7 +5871,7 @@ EOT
<dt><b>committer</b></dt>
<dd>Name and e-mail of the committer and date of commit will be scanned for the given pattern.</dd>
EOT
- my ($have_pickaxe) = gitweb_get_feature('pickaxe');
+ my $have_pickaxe = gitweb_check_feature('pickaxe');
if ($have_pickaxe) {
print <<EOT;
<dt><b>pickaxe</b></dt>
@@ -5907,7 +5923,7 @@ sub git_shortlog {
sub git_feed {
my $format = shift || 'atom';
- my ($have_blame) = gitweb_get_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
# Atom: http://www.atomenabled.org/developers/syndication/
# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
--
1.6.0.4.850.g6bd829
^ permalink raw reply related
* Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls
From: Giuseppe Bilotta @ 2008-11-29 22:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narebski, Petr Baudis
In-Reply-To: <7vzljiz1qn.fsf@gitster.siamese.dyndns.org>
On Sat, Nov 29, 2008 at 10:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> ... The change is not
>> about fixing (your proposed commit log message read "gitweb:fixes to
>> gitweb feature check code") as nothing is broken. It is purely about
>> futureproofing and I think we should mark it as such.
>
> Actually, there is a handful clueless/careless callers. Before applying
> any of the check => get patch, let's do this as a fix.
And this is precisely the reason why the first time I sent the patch I
did the restyling in the same go: by not touching the
clueless/careless callers and instead bringing gitweb_check_feature to
act in scalar context, it would automatically fix those broken usages,
and it made sense to convert all array-to-get-a-scalar assignmets as
well.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls
From: Junio C Hamano @ 2008-11-29 22:26 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Jakub Narebski, Petr Baudis
In-Reply-To: <cb7bb73a0811291416h4c227411u61bfe7033f0d2bae@mail.gmail.com>
"Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
> And this is precisely the reason why the first time I sent the patch I
> did the restyling in the same go: by not touching the
> clueless/careless callers and instead bringing gitweb_check_feature to
> act in scalar context, it would automatically fix those broken usages,
... which is very bad for reviewability purposes.
By explicitly fixing them before doing the "this will sweep all the
potential bugs under the rug", you can demonstrate a lot more clearly why
these changes are necessary.
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper
From: Giuseppe Bilotta @ 2008-11-29 22:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narebski, Petr Baudis
In-Reply-To: <7vmyfiz0zf.fsf_-_@gitster.siamese.dyndns.org>
On Sat, Nov 29, 2008 at 10:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> * Again, this is to demonstrate how I would have liked to see your
> patches as a reviewable series. Notice how this naturally justifies
> the dropping of parentheses in many lines that begin with "my", and
> makes it easier to review? You can review the patch easily by knowing
> that callers that have s/get/check/ are now safe to use scalar context.
Yes, I get the point: do less things under the hood. I also still
think that three patches with code going ping-pong are way too much,
though 8-P
For what it's worth, you get my Ack: to your patchset 8-)
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls
From: Jakub Narebski @ 2008-11-29 22:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Giuseppe Bilotta, git, Petr Baudis
In-Reply-To: <7viqq6yxqu.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> "Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
>
> > And this is precisely the reason why the first time I sent the patch I
> > did the restyling in the same go: by not touching the
> > clueless/careless callers and instead bringing gitweb_check_feature to
> > act in scalar context, it would automatically fix those broken usages,
>
> ... which is very bad for reviewability purposes.
>
> By explicitly fixing them before doing the "this will sweep all the
> potential bugs under the rug", you can demonstrate a lot more clearly why
> these changes are necessary.
Well, I think now that it would be best to split this series into
_two_ patches: first Junio's patch fixing (!) gitweb_check_feature()
calls, second original v1 Guiseppe's renaming gitweb_check_feature()
to gitweb_get_feature(), and adding gitweb_check_feature(), and using
gitweb_get_feature() only where needed; optionally fixing "style".
Pure rename IMVHO doesn't look that nice...
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCHv2ter 2/2] gitweb: clean up gitweb_check_feature() calls
From: Jakub Narebski @ 2008-11-29 22:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Giuseppe Bilotta, git, Petr Baudis
In-Reply-To: <200811292337.00380.jnareb@gmail.com>
Jakub Narebski wrote:
>
> Well, I think now that it would be best to split this series into
> _two_ patches: first Junio's patch fixing (!) gitweb_check_feature()
> calls, second original v1 Guiseppe's renaming gitweb_check_feature()
> to gitweb_get_feature(), and adding gitweb_check_feature(), and using
> gitweb_get_feature() only where needed; optionally fixing "style".
It means: first fixup patch, then futureproof patch.
--
Jakub Narebski
Poland
^ permalink raw reply
* gitweb doesn't work with bare repositories
From: Evgeniy Ivanov @ 2008-11-29 23:37 UTC (permalink / raw)
To: git; +Cc: lolkaantimat
Hi,
I have installed gitweb and can't make it work with bare repos.
I have such config:
$my_uri = "http://mysite.org:8000";
$projectroot = "/srv/www/gamekeeper/htdocs/projects";
In projects I have created bare repo:
mkdir some
cd some.git
git --bare init
cd /some_git_repo
git push /srv/www/gamekeeper/htdocs/projects master
Everything fine, but when I click the link on some.git I don't have a
page (just "Error 404").
But if I clone /some_git_repo with some in the projects, I have a link
"some/.git" and it works fine.
Permissions are ok, virtual host is ok since I get main page and have
access to cloned repo.
What can be wrong?
--
Cheers, Evgeniy.
Key fingerprint: F316 B5A1 F6D2 054F CD18 B74A 9540 0ABB 1FE5 67A3
^ permalink raw reply
* Re: A better approach to diffing and merging
From: Boyd Stephen Smith Jr. @ 2008-11-29 23:40 UTC (permalink / raw)
To: Ian Clarke; +Cc: git
In-Reply-To: <823242bd0811291012g15c4d442qa5d7afc9cc762b20@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2374 bytes --]
On Saturday 2008 November 29 12:12, Ian Clarke wrote:
> While I'm no merging expert, it seems that most merging algorithms do
> it on a line-by-line basis, treating source code as nothing but a list
> of lines of text. It got me thinking, what if the merging algorithm
> understood the structure of the source code it is trying to merge?
Unfortunately, this is hard to do in general. Not impossible, but very hard.
Heck, some languages don't really have a formal grammar, or have one that is
undecidable without doing deeper analysis. Perl 6 is supposed to have some
support for language constructs that change the grammar.
Also, this generally takes a lot of time. Automatic merges are only useful if
they take less (or only a little more) time than doing the merge manually.
If your mergetool has to think about something for 30 minutes that you could
have resolved in 5, it's not normally a "win".
Also, it slightly changes the format of a "patch" file. Currently, patch
files are a line-by-line diff. If you instead made changes based on mapping
parse trees to parse trees, you'd (probably) want to
store/transfer/communicate your patches using a different format, to preserve
the proper amount of "context" and make the patch easy to apply. (I.e., do
the hard work once.)
> Any takers? I've set up a Google Group for further discussion, please
> join if interested.
You might look deeper into Darcs development. This level of
pluggable "understanding" of the file(s) being modified fits in well with a
Grand Unified Theory of Patching. Also "understanding" patches better allows
Darcs to reorder patches (and calculate "reverse patches") better -- reducing
the time to do existing automatic merging (or reject the merge as
non-automatable) and make merges automatic that are currently not handled
automatically.
I'm not going to come out and discourage you or other from adding the
functionality to git, but I think there are more useful and practical ways to
improve git. (Line-by-line merging is generally "good enough", the worst
enemy of "good" software.)
--
Boyd Stephen Smith Jr. ,= ,-_-. =.
bss03@volumehost.net ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-'
http://iguanasuicide.org/ \_/
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: gitweb doesn't work with bare repositories
From: Fredrik Skolmli @ 2008-11-30 0:01 UTC (permalink / raw)
To: Evgeniy Ivanov; +Cc: git
In-Reply-To: <4931D23A.10402@gmail.com>
On Sun, Nov 30, 2008 at 02:37:30AM +0300, Evgeniy Ivanov wrote:
> Hi,
Hi.
Just checking now, but are the following corrections right?
> In projects I have created bare repo:
> mkdir some
mkdir some.git
> cd some.git
> git --bare init
> cd /some_git_repo
> git push /srv/www/gamekeeper/htdocs/projects master
git push /srv/www/gamekeeper/htdocs/projects/some.git master
> Permissions are ok, virtual host is ok since I get main page and have
> access to cloned repo.
You're sure the user the webserver is running as has access to the repo as
well?
- Fredrik
--
Kind regards,
Fredrik Skolmli
^ permalink raw reply
* Re: gitweb doesn't work with bare repositories
From: J.H. @ 2008-11-30 0:24 UTC (permalink / raw)
To: Fredrik Skolmli; +Cc: Evgeniy Ivanov, git
In-Reply-To: <20081130000115.GB20403@frsk.net>
As a note kernel.org uses, predominantly, bare repositories and it works
fine there....
- John 'Warthog9' Hawley
Chief Kernel.org Administrator
Fredrik Skolmli wrote:
> On Sun, Nov 30, 2008 at 02:37:30AM +0300, Evgeniy Ivanov wrote:
>
>> Hi,
>>
>
> Hi.
>
> Just checking now, but are the following corrections right?
>
>
>> In projects I have created bare repo:
>> mkdir some
>>
>
> mkdir some.git
>
>
>> cd some.git
>> git --bare init
>> cd /some_git_repo
>> git push /srv/www/gamekeeper/htdocs/projects master
>>
>
> git push /srv/www/gamekeeper/htdocs/projects/some.git master
>
>
>> Permissions are ok, virtual host is ok since I get main page and have
>> access to cloned repo.
>>
>
> You're sure the user the webserver is running as has access to the repo as
> well?
>
> - Fredrik
>
>
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper
From: Junio C Hamano @ 2008-11-30 0:23 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: Junio C Hamano, git, Jakub Narebski, Petr Baudis
In-Reply-To: <cb7bb73a0811291427n7922ab1fq50caedafe7163f9e@mail.gmail.com>
"Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
> On Sat, Nov 29, 2008 at 10:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> * Again, this is to demonstrate how I would have liked to see your
>> patches as a reviewable series. Notice how this naturally justifies
>> the dropping of parentheses in many lines that begin with "my", and
>> makes it easier to review? You can review the patch easily by knowing
>> that callers that have s/get/check/ are now safe to use scalar context.
>
> Yes, I get the point: do less things under the hood. I also still
> think that three patches with code going ping-pong are way too much,
> though 8-P
>
> For what it's worth, you get my Ack: to your patchset 8-)
Well, I am not interested in taking credits for this series. I am very
much interested in reducing my future workload by showing people how an
easily reviewable series should look like.
So if you still think it is "way too much", I did not succeed what I tried
to do X-<.
^ permalink raw reply
* Re: gitweb doesn't work with bare repositories
From: Jakub Narebski @ 2008-11-30 0:25 UTC (permalink / raw)
To: lolkaantimat; +Cc: git
In-Reply-To: <4931D23A.10402@gmail.com>
Evgeniy Ivanov <lolkaantimat@gmail.com> writes:
> I have installed gitweb and can't make it work with bare repos.
Actually gitweb treats all repositories as bare repositories,
and doesn't touch at all neither index nor working area.
> I have such config:
> $my_uri = "http://mysite.org:8000";
> $projectroot = "/srv/www/gamekeeper/htdocs/projects";
>
> In projects I have created bare repo:
I assume that you have done:
$ cd /srv/www/gamekeeper/htdocs/projects
> $ mkdir some
I assume that you meant here
$ mkdir some.git
> $ cd some.git
> $ git --bare init
> $ cd /some_git_repo
> $ git push /srv/www/gamekeeper/htdocs/projects master
I assume that you meant:
$ git push /srv/www/gamekeeper/htdocs/projects/some.git master
>
> Everything fine, but when I click the link on some.git I don't have a
> page (just "Error 404").
What error? Please provide _exact_ error message.
Does the problem persists with the following corrections? Do you
have permissions set up correctly? How did you generate gitweb.cgi?
SAA#1 (Standard admin answer #1): WORKSFORME.
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCHv2 1/2] gitweb: fixes to gitweb feature check code
From: Jakub Narebski @ 2008-11-30 0:31 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <1227904793-1821-2-git-send-email-giuseppe.bilotta@gmail.com>
On Fri, 28 Nov 2008, Giuseppe Bilotta wrote:
> The gitweb_check_feature routine was being used for two different
> purposes: retrieving the actual feature value (such as the list of
> snapshot formats or the list of additional actions), and to check if a
> feature was enabled.
>
> For the latter use, since all features return an array, it led to either
> clumsy code or subtle bugs, with disabled features appearing enabled
> because (0) evaluates to 1.
>
> We fix these bugs, and simplify the code, by separating feature (list)
> value retrieval (gitweb_get_feature) from boolean feature check (i.e.
> retrieving the first/only item in the feature value list). Usage of
> gitweb_check_feature is replaced by gitweb_get_feature where
> appropriate.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
I like the idea of futureproofing gitweb code to avoid further bugs
with checking if feature is enabled. So from me this patch gets Ack.
I have thought at first that perhaps this patch could also remove
requirement that 'default' field is array (reference), replacing a bit
cumbersome
$feature{'blame'}{'default'} = [1];
with simpler
$feature{'blame'}{'default'} = 1;
But now I think that is separate issue, and if we think it is worth
having (in spite of its disadvantages in form of more complicated code
dealing among others with legacy configuration), it should be put in
separate patch.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: timestamps not git-cloned
From: jidanni @ 2008-11-30 0:48 UTC (permalink / raw)
To: git
In-Reply-To: <200811291117.01655.trast@student.ethz.ch>
Well all I know is from the simple user who does e.g.,
# aptitude install linux-doc-2.6.26
# ls -lt /usr/share/doc/linux-doc-2.6.26/Documentation/
he thinks "gosh, can't tell what's new vs. what hasn't changed in years".
OK, now I know why this is tolerable upstream: they all use git.
But for the lowly user downstream who gets what git-archive produces,
it seems like a step backwards: "who threw away the timestamp of when
each file was last changed?".
OK, http://git.or.cz/gitwiki/ContentLimitations says this is by design.
And OK, thinking "file by file" is old fashioned, I read. The non-git
end user should just get used to reading ChangeLogs, if any, and stop
doing ls -lt.
But you must admit, /usr/share/doc/linux-doc-2.6.26/Documentation/
etc. are aimed for reading without git.
Anyways, if just in case any individual file modification time
information can still be pried from the 40 byte IDs or whatever, I
would suggest using it by default in git-archive at least, and maybe
even git-clone etc.
Just letting you know my 'valuable first impressions'. I expect once I
start smoking more of this "git" stuff, I too will become comfortably
numb to aforementioned lowly user problem, so you would never know
unless I hereby first told you before it was too late.
^ permalink raw reply
* Re: [PATCH 0/2] gitweb: patch view
From: Jakub Narebski @ 2008-11-30 1:06 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <1227966071-11104-1-git-send-email-giuseppe.bilotta@gmail.com>
On Sat, 29 Nov 2008, Giuseppe Bilotta wrote:
> I recently discovered that the commitdiff_plain view is not exactly
> something that can be used by git am directly (for example, the subject
> line gets duplicated in the commit message body after using git am).
That's because gitweb generates email-like format "by hand", instead
of using '--format=email' or git-format-patch like in your series. On
the other hand that allows us to add extra headers, namely X-Git-Tag:
(which hasn't best implementation, anyway) and X-Git-Url: with URL
for given output.
> Since I'm not sure if it was the case to fix the plain view because I
> don't know what its intended usage was, I prepared a new view,
> uncreatively called 'patch', that exposes git format-patch output
> directly.
Perhaps 'format_patch' would be better... hmmm... ?
Actually IMHO both 'commitdiff' and 'commitdiff_plain' try to do two
things at once. First to show diff _for_ a commit, i.e. equivalent of
"git show" or "git show --pretty=email", perhaps choosing one of
parents for a merge commit. Then showing commit message for $hash has
sense. The fact that 'commit' view doesn't show patchset, while
'commitdiff' does might be result of historical situation.
Second, to show diff _between_ commits, i.e. equivalent of
"git diff branch master". Then there doesn't make much sense to show
full commit message _only_ for one side of diff. IMHO that should be
main purpose of 'commitdiff' and 'commitdiff_plain' views, or simply
'diff' / 'diff_plain' future views.
What 'patch' view does, what might be not obvious from this description
and from first patch in series, is to show diffs for _series_ of
commits. It means equivalent of "git log -p" or "git whatchanged".
It might make more sense to have plain git-format-patch output, but it
could be useful to have some kind of 'git log -p' HTML output.
So even if 'commitdiff' / 'commitdiff_plain' is fixed, 'patch' whould
still have its place.
By the way, we still might want to add somehow X-Git-Url and X-Git-Tag
headers later to 'patch' ('patchset') output format.
>
> The second patch exposes it from commitdiff view (obviosly), but also
> from shortlog view, when less than 16 patches are begin shown.
Why this nonconfigurable limit?
>
> Giuseppe Bilotta (2):
> gitweb: add patch view
> gitweb: links to patch action in commitdiff and shortlog view
>
> gitweb/gitweb.perl | 35 +++++++++++++++++++++++++++++++++--
> 1 files changed, 33 insertions(+), 2 deletions(-)
Thank you for your work on gitweb
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: timestamps not git-cloned
From: Sitaram Chamarty @ 2008-11-30 1:14 UTC (permalink / raw)
To: Thomas Rast; +Cc: Stephen R. van den Berg, Chris Frey, jidanni, dhruvakm, git
In-Reply-To: <200811291117.01655.trast@student.ethz.ch>
On Sat, Nov 29, 2008 at 3:46 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Stephen R. van den Berg wrote:
>> Chris Frey wrote:
>> >If this is the important bit, perhaps git-archive could be changed
>> >to create tarballs with file timestamps based on their commit dates.
>>
>> Based on the principle of least surprise, I'd consider this a rather good
>> idea.
>
> Unless I'm missing something, this would make git-archive rather more
> expensive than it is now: Tree objects do not record any timestamps,
How many people use git-archive and how many times a day do they use
it? For example, kernel.org seems to put out linux-2.x.y.z.tar.bz2
once every 2 to 7 days.
The overhead of this new option (and certainly it should be an option,
not the default) should be measured not against the old running time,
but against the frequency of usage of the tool. Look at it on those
time scales, it may not be a big deal.
By all accounts, this overhead will not affect the "giterate" [meaning
git-literate ;-)] people too much.
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper
From: Giuseppe Bilotta @ 2008-11-30 1:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narebski, Petr Baudis
In-Reply-To: <7vej0uysbs.fsf@gitster.siamese.dyndns.org>
On Sun, Nov 30, 2008 at 1:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
>
>> On Sat, Nov 29, 2008 at 10:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> * Again, this is to demonstrate how I would have liked to see your
>>> patches as a reviewable series. Notice how this naturally justifies
>>> the dropping of parentheses in many lines that begin with "my", and
>>> makes it easier to review? You can review the patch easily by knowing
>>> that callers that have s/get/check/ are now safe to use scalar context.
>>
>> Yes, I get the point: do less things under the hood. I also still
>> think that three patches with code going ping-pong are way too much,
>> though 8-P
>>
>> For what it's worth, you get my Ack: to your patchset 8-)
>
> Well, I am not interested in taking credits for this series. I am very
> much interested in reducing my future workload by showing people how an
> easily reviewable series should look like.
>
> So if you still think it is "way too much", I did not succeed what I tried
> to do X-<.
At least as far as this patch goes, from my point of view, the
'clueless/careless' usages of gitweb_check_feature are the
conceptually (although not code-wise) correct ones, so they shouldn't
be the ones touched by the patch, considering that in addition to the
futureproofing, (implicit) fixing those usages is one of the main
points of the patch itself.
OTOH, I _do_ get the point about ease of review. But for example (and
for future reference for myself): would you say that it would have
been enough to have a cleaner commit message explicitly mentioning the
fact that formerly incorrect usages of the gitweb_check_feature() were
left intact because they were now correct?
Something like this http://git.oblomov.eu/git/patch/gitweb/check-feature-v3
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* [PATCH] gitweb: fixes to gitweb feature check code
From: Giuseppe Bilotta @ 2008-11-30 1:34 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Petr Baudis, Junio C Hamano, Giuseppe Bilotta
In-Reply-To: <Message-ID: <cb7bb73a0811291731g7f8770f7p89e924c00d2ab004@mail.gmail.com>
The gitweb_check_feature routine was being used for two different
purposes: retrieving the actual feature value (such as the list of
snapshot formats or the list of additional actions), and checking if a
feature was enabled.
This led to subtle bugs in feature checking code: gitweb_check_feature
would return (0) for disabled features, so its use in scalar context
would return true instead of false.
To fix this issue and future-proof the code, we split feature value
retrieval into its own gitweb_get_feature()function , and ensure that
the boolean feature check function gitweb_check_feature() always returns
a scalar (precisely, the first/only item in the feature value list).
Usage of gitweb_check_feature() across gitweb is replaced with
gitweb_get_feature() where appropriate, and list evaluation of
gitweb_check_feature() is demoted to scalar evaluation to prevent
ambiguity. The few previously incorrect uses of gitweb_check_feature()
in scalar context are left untouched because they are now correct.
Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
gitweb/gitweb.perl | 52 +++++++++++++++++++++++++++++++++++-----------------
1 files changed, 35 insertions(+), 17 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 933e137..4246819 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -190,7 +190,9 @@ our %feature = (
# if there is no 'sub' key (no feature-sub), then feature cannot be
# overriden
#
- # use gitweb_check_feature(<feature>) to check if <feature> is enabled
+ # use gitweb_get_feature(<feature>) to retrieve the <feature> value
+ # (an array) or gitweb_check_feature(<feature>) to check if <feature>
+ # is enabled
# Enable the 'blame' blob view, showing the last commit that modified
# each line in the file. This can be very CPU-intensive.
@@ -329,7 +331,8 @@ our %feature = (
'default' => [0]},
);
-sub gitweb_check_feature {
+# retrieve the value of a given feature, as an array
+sub gitweb_get_feature {
my ($name) = @_;
return unless exists $feature{$name};
my ($sub, $override, @defaults) = (
@@ -344,6 +347,21 @@ sub gitweb_check_feature {
return $sub->(@defaults);
}
+# check if a given feature is enabled or not, returning the first (and only)
+# value of the feature. This allows us to write
+# my $bool_feat = gitweb_check_feature('bool_feat');
+# or
+# gitweb_check_feature('bool_feat') or somecode;
+# instead of
+# my ($bool_feat) = gitweb_get_feature('bool_feat');
+# or
+# (gitweb_get_feature('bool_feat'))[0] or somecode;
+# respectively
+sub gitweb_check_feature {
+ return (gitweb_get_feature(@_))[0];
+}
+
+
sub feature_blame {
my ($val) = git_get_project_config('blame', '--bool');
@@ -767,7 +785,7 @@ our $git_dir;
$git_dir = "$projectroot/$project" if $project;
# list of supported snapshot formats
-our @snapshot_fmts = gitweb_check_feature('snapshot');
+our @snapshot_fmts = gitweb_get_feature('snapshot');
@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
# dispatch
@@ -810,7 +828,7 @@ sub href (%) {
}
}
- my ($use_pathinfo) = gitweb_check_feature('pathinfo');
+ my $use_pathinfo = gitweb_check_feature('pathinfo');
if ($use_pathinfo) {
# try to put as many parameters as possible in PATH_INFO:
# - project name
@@ -2101,7 +2119,7 @@ sub git_get_projects_list {
$filter ||= '';
$filter =~ s/\.git$//;
- my ($check_forks) = gitweb_check_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
if (-d $projects_list) {
# search in directory
@@ -2947,7 +2965,7 @@ EOF
}
print "</div>\n";
- my ($have_search) = gitweb_check_feature('search');
+ my $have_search = gitweb_check_feature('search');
if (defined $project && $have_search) {
if (!defined $searchtext) {
$searchtext = "";
@@ -2961,7 +2979,7 @@ EOF
$search_hash = "HEAD";
}
my $action = $my_uri;
- my ($use_pathinfo) = gitweb_check_feature('pathinfo');
+ my $use_pathinfo = gitweb_check_feature('pathinfo');
if ($use_pathinfo) {
$action .= "/".esc_url($project);
}
@@ -3084,7 +3102,7 @@ sub git_print_page_nav {
$arg{'tree'}{'hash'} = $treehead if defined $treehead;
$arg{'tree'}{'hash_base'} = $treebase if defined $treebase;
- my @actions = gitweb_check_feature('actions');
+ my @actions = gitweb_get_feature('actions');
my %repl = (
'%' => '%',
'n' => $project, # project name
@@ -3454,7 +3472,7 @@ sub is_patch_split {
sub git_difftree_body {
my ($difftree, $hash, @parents) = @_;
my ($parent) = $parents[0];
- my ($have_blame) = gitweb_check_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
print "<div class=\"list_head\">\n";
if ($#{$difftree} > 10) {
print(($#{$difftree} + 1) . " files changed:\n");
@@ -3968,7 +3986,7 @@ sub git_project_list_body {
# actually uses global variable $project
my ($projlist, $order, $from, $to, $extra, $no_header) = @_;
- my ($check_forks) = gitweb_check_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
my @projects = fill_project_list_info($projlist, $check_forks);
$order ||= $default_projects_order;
@@ -4428,7 +4446,7 @@ sub git_summary {
my @taglist = git_get_tags_list(16);
my @headlist = git_get_heads_list(16);
my @forklist;
- my ($check_forks) = gitweb_check_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
if ($check_forks) {
@forklist = git_get_projects_list($project);
@@ -4457,7 +4475,7 @@ sub git_summary {
}
# Tag cloud
- my $show_ctags = (gitweb_check_feature('ctags'))[0];
+ my $show_ctags = gitweb_check_feature('ctags');
if ($show_ctags) {
my $ctags = git_get_project_ctags($project);
my $cloud = git_populate_project_tagcloud($ctags);
@@ -4747,7 +4765,7 @@ sub git_blob {
$expires = "+1d";
}
- my ($have_blame) = gitweb_check_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
or die_error(500, "Couldn't cat $file_name, $hash");
my $mimetype = blob_mimetype($fd, $file_name);
@@ -4840,7 +4858,7 @@ sub git_tree {
my $ref = format_ref_marker($refs, $hash_base);
git_header_html();
my $basedir = '';
- my ($have_blame) = gitweb_check_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
if (defined $hash_base && (my %co = parse_commit($hash_base))) {
my @views_nav = ();
if (defined $file_name) {
@@ -5838,7 +5856,7 @@ insensitive).</p>
<dt><b>commit</b></dt>
<dd>The commit messages and authorship information will be scanned for the given pattern.</dd>
EOT
- my ($have_grep) = gitweb_check_feature('grep');
+ my $have_grep = gitweb_check_feature('grep');
if ($have_grep) {
print <<EOT;
<dt><b>grep</b></dt>
@@ -5855,7 +5873,7 @@ EOT
<dt><b>committer</b></dt>
<dd>Name and e-mail of the committer and date of commit will be scanned for the given pattern.</dd>
EOT
- my ($have_pickaxe) = gitweb_check_feature('pickaxe');
+ my $have_pickaxe = gitweb_check_feature('pickaxe');
if ($have_pickaxe) {
print <<EOT;
<dt><b>pickaxe</b></dt>
@@ -5907,7 +5925,7 @@ sub git_shortlog {
sub git_feed {
my $format = shift || 'atom';
- my ($have_blame) = gitweb_check_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
# Atom: http://www.atomenabled.org/developers/syndication/
# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
--
1.5.6.5
^ permalink raw reply related
* Re: [PATCH 0/2] gitweb: patch view
From: Giuseppe Bilotta @ 2008-11-30 1:44 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <200811300206.23240.jnareb@gmail.com>
On Sun, Nov 30, 2008 at 2:06 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 29 Nov 2008, Giuseppe Bilotta wrote:
>
>> I recently discovered that the commitdiff_plain view is not exactly
>> something that can be used by git am directly (for example, the subject
>> line gets duplicated in the commit message body after using git am).
>
> That's because gitweb generates email-like format "by hand", instead
> of using '--format=email' or git-format-patch like in your series. On
> the other hand that allows us to add extra headers, namely X-Git-Tag:
> (which hasn't best implementation, anyway) and X-Git-Url: with URL
> for given output.
> By the way, we still might want to add somehow X-Git-Url and X-Git-Tag
> headers later to 'patch' ('patchset') output format.
Yeah, I've been thinking about it, but I couldn't find an easy and
robust way to do it. Plus, should we add them for each patch, or just
once for the whole patchset?
>> Since I'm not sure if it was the case to fix the plain view because I
>> don't know what its intended usage was, I prepared a new view,
>> uncreatively called 'patch', that exposes git format-patch output
>> directly.
>
> Perhaps 'format_patch' would be better... hmmm... ?
Considering I think commitdiff is ugly and long, you can guess my
opinion on format_patch 8-P. 'patchset' might be a good candidate,
considering it's what it does when both hash_parent and hash are
given.
> Actually IMHO both 'commitdiff' and 'commitdiff_plain' try to do two
> things at once. First to show diff _for_ a commit, i.e. equivalent of
> "git show" or "git show --pretty=email", perhaps choosing one of
> parents for a merge commit. Then showing commit message for $hash has
> sense. The fact that 'commit' view doesn't show patchset, while
> 'commitdiff' does might be result of historical situation.
>
> Second, to show diff _between_ commits, i.e. equivalent of
> "git diff branch master". Then there doesn't make much sense to show
> full commit message _only_ for one side of diff. IMHO that should be
> main purpose of 'commitdiff' and 'commitdiff_plain' views, or simply
> 'diff' / 'diff_plain' future views.
We can probably consider deprecating commitdiff(_plain) and have the
following three views:
* commit(_plain): do what commitdiff(_plain) currently does for a single commit
* diff(_plain): do what commitdiff(_plain) currently does for
parent..hash views, modulo something to be discussed for commit
messages (a shortlog rather maybe?)
* patch[set?][_plain?]: format-patch style output (maybe with option
for HTML stuff too)
> What 'patch' view does, what might be not obvious from this description
> and from first patch in series, is to show diffs for _series_ of
> commits. It means equivalent of "git log -p" or "git whatchanged".
> It might make more sense to have plain git-format-patch output, but it
> could be useful to have some kind of 'git log -p' HTML output.
>
> So even if 'commitdiff' / 'commitdiff_plain' is fixed, 'patch' whould
> still have its place.
Nice to know. Do consider the current version more of a
proof-of-concept that some definitive code.
>> The second patch exposes it from commitdiff view (obviosly), but also
>> from shortlog view, when less than 16 patches are begin shown.
>
> Why this nonconfigurable limit?
Because the patch was actually a quick hack for the proof of concept
8-) I wasn't even sure the patch idea would have been worth it (as
opposed to email-izing commitdiff_plain).
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: A better approach to diffing and merging
From: Brian Dessent @ 2008-11-30 1:56 UTC (permalink / raw)
To: Ian Clarke; +Cc: git
In-Reply-To: <823242bd0811291012g15c4d442qa5d7afc9cc762b20@mail.gmail.com>
Ian Clarke wrote:
> Provide the merge algorithm with the grammar of the programming
> language, perhaps in the form of a Bison grammar file, or some other
> standardized way to represent a grammar.
>
> The merge algorithm then uses this to parse the files to be diffed
> and/or merged into trees, and then the diff and merge are treated as
> operations on these trees. These operations may include creating,
> deleting, or moving nodes or branches, renaming nodes, etc. There has
> been quite a bit (pdf) of academic research on this topic, although I
> haven't yet found off-the-shelf code that will do what we need.
> Still, it shouldn't be terribly hard to implement.
There's a huge flaw in that approach for C/C++: in order to parse C/C++
you have to first preprocess it -- consider the twisty mazes that
#ifdef/#else/#endif can create. But in order to preprocess source code
you need a whole heap of extra information that is not in the repository
(or if it is, cannot be automatically extracted.)
For example, you'd have to know all the -D/-U/-I flags that the makefile
or the user might pass to the compiler. You'd have to replicate the
compiler's complicated header search path algorithm, which can depend on
the directives in the code as well as command line arguments,
environment variables, and values specific to the toolchain. (Don't
forget that you can have code in a repository that's meant to be
cross-compiled and which uses a toolchain that has its own headers and
not the ones in /usr/include.) You'd have to know all the built-in
predefined symbols of that toolchain, e.g. what's the value of
__GNUC_MINOR__ or __GNUC_PATCHLEVEL__, is __mips__ or __i386__ defined,
and on and on. And of course the natural conclusion of this
progression: a change can be perfectly grammatically correct for one
particular platform/toolchain/setting of CFLAGS, and completely broken
for another. There's no way for a VCS to know any of this, it takes
human comprehension.
If you look at a tool like doxygen that attempts to parse C/C++, it
don't actually do full preprocessing, only a very limited subset: it
only expands macros that the user names as relevant in the config file,
and it only preprocesses included headers that match a pathspec the user
provides. Consequently it cannot fully parse the code to see if it's
grammatically correct, only to the limited extent that it can infer the
location where things appear to be defined. And it is easily confused,
e.g. it will "see" code in both halves of an #ifdef section if it wasn't
told anything about the value of the macro in the config file, which can
cause it to incorrectly think that a function or variable was defined
there when in reality that section was discarded.
The idea may have value for langauges that are easy to parse and do not
have all this preprocessor cruft, but I just don't see how it would be
able to provide anything useful for non-trivial changes to real world
C/C++, which require human eyes to decipher.
Brian
^ permalink raw reply
* Re: A better approach to diffing and merging
From: Miklos Vajna @ 2008-11-30 2:54 UTC (permalink / raw)
To: Boyd Stephen Smith Jr.; +Cc: Ian Clarke, git
In-Reply-To: <200811291740.06865.bss03@volumehost.net>
[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]
On Sat, Nov 29, 2008 at 05:40:02PM -0600, "Boyd Stephen Smith Jr." <bss03@volumehost.net> wrote:
> You might look deeper into Darcs development. This level of
> pluggable "understanding" of the file(s) being modified fits in well with a
> Grand Unified Theory of Patching. Also "understanding" patches better allows
> Darcs to reorder patches (and calculate "reverse patches") better -- reducing
> the time to do existing automatic merging (or reject the merge as
> non-automatable) and make merges automatic that are currently not handled
> automatically.
>
> I'm not going to come out and discourage you or other from adding the
> functionality to git, but I think there are more useful and practical ways to
> improve git. (Line-by-line merging is generally "good enough", the worst
> enemy of "good" software.)
I think this was already discussed:
http://thread.gmane.org/gmane.comp.version-control.git/60457/focus=60512
If you mean just looking at the code moves/copies between the trees (but
no other history), then a merge strategy which makes use of git blame's
code move/copy detection would be indeed nice, though nobody created it
so far.
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* [PATCH v3] bisect: teach "skip" to accept a range using "-r|--range" option
From: Christian Couder @ 2008-11-30 6:15 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin, Johannes Sixt; +Cc: git, H. Peter Anvin
The current "git bisect skip" syntax is "git bisect skip [<rev>...]",
so it's already possible to skip a range of revisions using
something like:
$ git bisect skip $(git rev-list A..B)
where A and B are the bounds of the range of commits we want to skip.
In this case A itself will not be skipped, but B will be, because
that's how "git rev-list" works.
This patch teaches "git bisect skip" to accept:
$ git bisect skip --range=A,B
or:
$ git bisect skip -r A,B
to skip the whole range of commits between A included and B included.
Because we believe that users, in most cases, will want the first
bound to be skipped too.
If either A or B is ommited, HEAD will be used instead.
This is done by expanding each range argument using:
git rev-list "${B:-HEAD}" --not $(git rev-parse "${A:-HEAD}^@")
So the following command:
$ git bisect skip A -r B,C D E --range=F, --range ,G
should work as expected.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/git-bisect.txt | 30 +++++++++++++++++++++++-
git-bisect.sh | 52 ++++++++++++++++++++++++++++++++++++++++-
t/t6030-bisect-porcelain.sh | 10 +++++++-
3 files changed, 88 insertions(+), 4 deletions(-)
Junio wrote:
>
> Although I fully realize that the established semantics of A..B in git is
> bottom-exclusive, top-inclusive, and this suggestion breaks the UI
> uniformity by deviating from that convention, I have to wonder if it would
> be more useful if you let the bottom commit (A in your example) also be
> skipped.
I agree that it would be perhaps more usefull to skip the bottom commit.
But in this case I think we should not use the .. notation as it would be
inconsistent. So here is a patch that adds an all inclusive "-r|--range"
option.
This patch adds some documentation and a test case too.
After coming up with this patch, I saw that the previous version has
already been merged into "next" and "master", but perhaps it can be reverted
and this version applied. Otherwise if the previous version is preferred, I
will come up with another patch to just add some documentation and tests on
top of the previous version.
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 39034ec..86f6ad2 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -19,7 +19,7 @@ on the subcommand:
git bisect start [<bad> [<good>...]] [--] [<paths>...]
git bisect bad [<rev>]
git bisect good [<rev>...]
- git bisect skip [<rev>...]
+ git bisect skip [(<rev>|<range>)...]
git bisect reset [<branch>]
git bisect visualize
git bisect replay <logfile>
@@ -164,6 +164,34 @@ But computing the commit to test may be slower afterwards and git may
eventually not be able to tell the first bad among a bad and one or
more "skip"ped commits.
+You can even skip a range of commits, instead of just one commit,
+using option -r|--range. For example:
+
+------------
+$ git bisect skip --range=v2.5,v2.6
+------------
+
+or:
+
+------------
+$ git bisect skip -r v2.5,v2.6
+------------
+
+would mean that no commit between versions 2.5 included and 2.6
+included can be tested.
+
+Note that you must take care that the first commit in the given range
+is an ancestor of the second commit.
+
+Also note that if you don't want to skip the first commit of a range
+you can use something like:
+
+------------
+$ git bisect skip $(git rev-list v2.5..v2.6)
+------------
+
+and the commit pointed to by v2.5 will not be skipped.
+
Cutting down bisection by giving more parameters to bisect start
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/git-bisect.sh b/git-bisect.sh
index 0d0e278..29a5c2e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -9,7 +9,7 @@ git bisect bad [<rev>]
mark <rev> a known-bad revision.
git bisect good [<rev>...]
mark <rev>... known-good revisions.
-git bisect skip [<rev>...]
+git bisect skip [(<rev>|<range>)...]
mark <rev>... untestable revisions.
git bisect next
find next bisection to test and check it out.
@@ -191,6 +191,52 @@ check_expected_revs() {
done
}
+list_range() {
+ case "$1" in
+ *,*) ;; # Ok
+ *) die "Bad range: $1" ;;
+ esac
+
+ _A=$(expr "z$1" : 'z\([^,]*\),.*')
+ _B=$(expr "z$1" : 'z[^,]*,\(.*\)')
+
+ #
+ # We want a range with both bounds included.
+ # ("git rev-list ^A B" would exclude commit A.)
+ # This means we want to exclude starting from the parents of _A,
+ # not starting from _A itself.
+ #
+ git rev-list "${_B:-HEAD}" --not $(git rev-parse "${_A:-HEAD}^@")
+}
+
+bisect_skip() {
+ all=''
+ while test $# != 0
+ do
+ case "$1" in
+ -r|--range*)
+ case "$#,$1" in
+ *,*=*)
+ range=$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
+ 1,*)
+ die "Bad or incomplete range: $1" ;;
+ *)
+ range="$2"; shift ;;
+ esac
+ revs=$(list_range "$range") ||
+ die "Bad rev input: $1"
+ ;;
+ *)
+ revs="'$arg'"
+ ;;
+ esac
+ shift
+ all="$all $revs"
+ done
+ bisect_state 'skip' $all
+}
+
+
bisect_state() {
bisect_autostart
state=$1
@@ -630,8 +676,10 @@ case "$#" in
git bisect -h ;;
start)
bisect_start "$@" ;;
- bad|good|skip)
+ bad|good)
bisect_state "$cmd" "$@" ;;
+ skip)
+ bisect_skip "$@" ;;
next)
# Not sure we want "next" at the UI level anymore.
bisect_next "$@" ;;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 85fa39c..e76546c 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -313,8 +313,16 @@ test_expect_success 'bisect run & skip: find first bad' '
grep "$HASH6 is first bad commit" my_bisect_log.txt
'
-test_expect_success 'bisect starting with a detached HEAD' '
+test_expect_success 'bisect skip range' '
+ git bisect reset &&
+ git bisect start $HASH7 $HASH1 &&
+ git bisect skip --range=$HASH2,$HASH5 &&
+ test "$HASH6" = "$(git rev-parse --verify HEAD)" &&
+ test_must_fail git bisect bad > my_bisect_log.txt &&
+ grep "first bad commit could be any of" my_bisect_log.txt
+'
+test_expect_success 'bisect starting with a detached HEAD' '
git bisect reset &&
git checkout master^ &&
HEAD=$(git rev-parse --verify HEAD) &&
--
1.6.0.4.772.ge8f9a
^ permalink raw reply related
* Re: [PATCH v3] bisect: teach "skip" to accept a range using "-r|--range" option
From: Junio C Hamano @ 2008-11-30 8:24 UTC (permalink / raw)
To: Christian Couder; +Cc: Johannes Schindelin, Johannes Sixt, git, H. Peter Anvin
In-Reply-To: <20081130071511.e279e8bc.chriscool@tuxfamily.org>
Christian Couder <chriscool@tuxfamily.org> writes:
> Junio wrote:
> >
> > Although I fully realize that the established semantics of A..B in git is
> > bottom-exclusive, top-inclusive, and this suggestion breaks the UI
> > uniformity by deviating from that convention, I have to wonder if it would
> > be more useful if you let the bottom commit (A in your example) also be
> > skipped.
>
> I agree that it would be perhaps more usefull to skip the bottom commit.
Actually I do not think it is such a big deal to warrant more code and
complexity. "skip A..B A", if you really wanted to, would work anyway.
^ permalink raw reply
* Re: French git user
From: nadim khemir @ 2008-11-30 8:37 UTC (permalink / raw)
To: git
In-Reply-To: <83zljmo3sw.fsf@kalahari.s2.org>
On Wednesday 26 November 2008 17.22.23 Hannu Koivisto wrote:
> Depending on the audience, one might want to mention that if you
> are using a modern version of zsh as your shell and have activated
> its run-help function (see
> http://zsh.dotsrc.org/Doc/Release/zsh_24.html#SEC218), you can just
> say
>
> git foo -bar <M-h>
Any idea if this also accessible, or is possible to add, in bash?
I don't have it but I already feel I can't live without it.
Chees, Nadim.
^ permalink raw reply
* how to hide some branches
From: Pascal Obry @ 2008-11-30 9:50 UTC (permalink / raw)
To: git list
Hello everyone,
I create a new branch for every new feature/fix I work on. After some
time I have many (too much) branch listed when doing:
$ git branch
I'd like to hide some (not removing them).
Is there a solution for this?
How you people handle this?
Thanks.
Pascal.
--
--|------------------------------------------------------
--| Pascal Obry Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--| http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox