* Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO
From: Jakub Narebski @ 2008-10-02 22:41 UTC (permalink / raw)
To: Petr Baudis; +Cc: Giuseppe Bilotta, git, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20081002220439.GX10360@machine.or.cz>
Petr Baudis wrote:
> On Thu, Oct 02, 2008 at 11:05:18PM +0200, Giuseppe Bilotta wrote:
> >
> > In preparing the new patchset, I've put a big comment block explaining
> > why we need to set both $hash and $hash_base in this code-path:
> >
> > # we got "project.git/[action/]branch". in this case
> > # we set both $hash and $hash_base because different actions
> > # need one or the other to be set to behave correctly.
> > #
> > # For example, if $hash_base is not set then the blob and
'blob' without $file_name doesn't have sense, but 'tree' does.
Probably should be s/blob/tree/ above.
> > # history links on the page project.git/tree/somebranch will
> > # assume a $hash_base of HEAD instead of the correct
> > # somebranch.
> > # Conversely, not setting $hash will make URLs such as
> > # project.git/shortlog/somebranch display the wrong page.
> > #
> > # Since it also turns out that the unused one is properly
> > # overwritten as needed, setting both is quite safe.
>
> Ok, but is this related to the pathinfo changes? Or is this fixing a
> separate bug? (Not that I would want to bother you with splitting this,
> but you should at least mention it in the commit message... and it's
> fairly isolated anyway.)
Yes, it related to path_info changes. With current code the only way
to get to no $file_name branch of evaluate_path_info() code was to have
URL which looks like project/branch, for which 'shortlog' action was
chosen (if not specified by CGI query parameters), and for 'shortlog'
action the correct way was to use $hash. Now there can be
project/tree/branch to show top directory of given branch, and this
as described required $hash_base set.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [JGIT PATCH 4/5] Expose the critical receive configuration options to JGit
From: Shawn O. Pearce @ 2008-10-02 22:46 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git
In-Reply-To: <200810012254.09169.robin.rosenberg.lists@dewire.com>
Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
>
> The repo configuration setup fails. I'll squash this in
Whoops. Thanks.
> I also noted we try to read ~/.gitconfig which may give us som headaches
> later on.
Oy. Issue #42 on the issue tracker for it. We shouldn't be reading
that during the tests.
--
Shawn.
^ permalink raw reply
* [PATCH] git-stash.sh: fix flawed fix of invalid ref handling (commit da65e7c1)
From: Brandon Casey @ 2008-10-02 23:52 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Git Mailing List
In-Reply-To: <DDpCKmlWUqX7hHbNNF45UGo81L7NuUHMbU3vXSfhHU60An5PUqIvRA@cipher.nrlssc.navy.mil>
The referenced commit tried to fix a flaw in stash's handling of a user
supplied invalid ref. i.e. 'git stash apply fake_ref@{0}' should fail
instead of applying stash@{0}. But, it did so in a naive way by avoiding the
use of the --default option of rev-parse, and instead manually supplied the
default revision if the user supplied an empty command line. This prevented
a common usage scenario of supplying flags on the stash command line (i.e.
non-empty command line) which would be parsed by lower level git commands,
without supplying a specific revision. This should fall back to the default
revision, but now it causes an error. e.g. 'git stash show -p'
The correct fix is to use the --verify option of rev-parse, which fails
properly if an invalid ref is supplied, and still allows falling back to a
default ref when one is not supplied.
Convert stash-drop to use --verify while we're at it, since specifying
multiple revisions for any of these commands is also an error and --verify
makes it so.
Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
The patch is simpler than it looks, since it includes a revert of my
original patch.
Here is 'git diff da65e7c1^:git-stash.sh git-stash.sh':
@@ -144,17 +144,16 @@ show_stash () {
then
flags=--stat
fi
- s=$(git rev-parse --revs-only --no-flags --default $ref_stash "$@")
- w_commit=$(git rev-parse --verify "$s") &&
- b_commit=$(git rev-parse --verify "$s^") &&
+ w_commit=$(git rev-parse --verify --default $ref_stash "$@") &&
+ b_commit=$(git rev-parse --verify "$w_commit^") &&
git diff $flags $b_commit $w_commit
}
@@ -169,7 +168,7 @@ apply_stash () {
# stash records the work tree, and is a merge between the
# base commit (first parent) and the index tree (second parent).
- s=$(git rev-parse --revs-only --no-flags --default $ref_stash "$@") &&
+ s=$(git rev-parse --verify --default $ref_stash "$@") &&
w_tree=$(git rev-parse --verify "$s:") &&
b_tree=$(git rev-parse --verify "$s^1:") &&
i_tree=$(git rev-parse --verify "$s^2:") ||
@@ -229,7 +228,7 @@ drop_stash () {
shift
fi
# Verify supplied argument looks like a stash entry
- s=$(git rev-parse --revs-only --no-flags "$@") &&
+ s=$(git rev-parse --verify "$@") &&
git rev-parse --verify "$s:" > /dev/null 2>&1 &&
git rev-parse --verify "$s^1:" > /dev/null 2>&1 &&
git rev-parse --verify "$s^2:" > /dev/null 2>&1 ||
I already messed this up once, so a review is appreciated. Any reason
--verify wasn't used in the first place?
I know why it wasn't used in drop_stash(), it's because I just copied it
from apply_stash().
-brandon
git-stash.sh | 22 ++++------------------
1 files changed, 4 insertions(+), 18 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 42f626f..b9ace99 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -145,16 +145,8 @@ show_stash () {
flags=--stat
fi
- if test $# = 0
- then
- set x "$ref_stash@{0}"
- shift
- fi
-
- s=$(git rev-parse --revs-only --no-flags "$@")
-
- w_commit=$(git rev-parse --verify "$s") &&
- b_commit=$(git rev-parse --verify "$s^") &&
+ w_commit=$(git rev-parse --verify --default $ref_stash "$@") &&
+ b_commit=$(git rev-parse --verify "$w_commit^") &&
git diff $flags $b_commit $w_commit
}
@@ -170,19 +162,13 @@ apply_stash () {
shift
esac
- if test $# = 0
- then
- set x "$ref_stash@{0}"
- shift
- fi
-
# current index state
c_tree=$(git write-tree) ||
die 'Cannot apply a stash in the middle of a merge'
# stash records the work tree, and is a merge between the
# base commit (first parent) and the index tree (second parent).
- s=$(git rev-parse --revs-only --no-flags "$@") &&
+ s=$(git rev-parse --verify --default $ref_stash "$@") &&
w_tree=$(git rev-parse --verify "$s:") &&
b_tree=$(git rev-parse --verify "$s^1:") &&
i_tree=$(git rev-parse --verify "$s^2:") ||
@@ -242,7 +228,7 @@ drop_stash () {
shift
fi
# Verify supplied argument looks like a stash entry
- s=$(git rev-parse --revs-only --no-flags "$@") &&
+ s=$(git rev-parse --verify "$@") &&
git rev-parse --verify "$s:" > /dev/null 2>&1 &&
git rev-parse --verify "$s^1:" > /dev/null 2>&1 &&
git rev-parse --verify "$s^2:" > /dev/null 2>&1 ||
--
1.6.0.2.323.g7c850
^ permalink raw reply related
* [PATCH] rebase -i: proper prepare-commit-msg hook argument when squashing
From: SZEDER Gábor @ 2008-10-03 0:08 UTC (permalink / raw)
To: git; +Cc: SZEDER Gábor
One would expect that the prepare-commit-msg hook gets 'squash' as the
second argument when squashing commits with 'rebase -i'. However,
that was not the case, as it got 'merge' instead. This patch fixes
the problem.
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
git-rebase--interactive.sh | 17 +++++++++++------
1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index edb6ec6..ec4299a 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -304,23 +304,28 @@ do_next () {
mark_action_done
make_squash_message $sha1 > "$MSG"
+ failed=f
+ author_script=$(get_author_ident_from_commit HEAD)
+ output git reset --soft HEAD^
+ pick_one -n $sha1 || failed=t
case "$(peek_next_command)" in
squash|s)
EDIT_COMMIT=
USE_OUTPUT=output
+ MSG_OPT=-F
+ MSG_FILE="$MSG"
cp "$MSG" "$SQUASH_MSG"
;;
*)
EDIT_COMMIT=-e
USE_OUTPUT=
+ MSG_OPT=
+ MSG_FILE=
rm -f "$SQUASH_MSG" || exit
+ cp -v "$MSG" "$GIT_DIR"/SQUASH_MSG
+ rm -f "$GIT_DIR"/MERGE_MSG || exit
;;
esac
-
- failed=f
- author_script=$(get_author_ident_from_commit HEAD)
- output git reset --soft HEAD^
- pick_one -n $sha1 || failed=t
echo "$author_script" > "$DOTEST"/author-script
if test $failed = f
then
@@ -329,7 +334,7 @@ do_next () {
GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
- $USE_OUTPUT git commit --no-verify -F "$MSG" $EDIT_COMMIT || failed=t
+ $USE_OUTPUT git commit --no-verify $MSG_OPT "$MSG_FILE" $EDIT_COMMIT || failed=t
fi
if test $failed = t
then
--
1.6.0.2.430.gfc53
^ permalink raw reply related
* git rebase -- a suggestion
From: Robin Burchell @ 2008-10-03 0:10 UTC (permalink / raw)
To: git
Hi,
This is my first mail to this list, so I hope I'm not breaking any
form of ettiquette, etc. If I do step on any toes, feel free to bop me
on the head with a rubber mallet, or steer me in the right direction.
That over, I have a suggestion for `git rebase', from the perspective
of a newcomer.
I've been using git instead of svn (and various other VCS) now for
about a month, and am finding it quite a refreshing change.
I have also recently started a collaborative project exclusively with
git (well, pulling changes from a git-svn repo I don't control) which
has been a valuable ..learning experience.
With this in mind, I'd like to mention exactly what I did.
Upstream had issued a new commit, so I, not knowing the possible
dangers used git-svn rebase to pull in the new changes to our tree.
This "appeared" to work fine, but alarm bells were already going off
in my head before I typed the command (I didn't know at the time I
could merge svn trees like I could normal git branches) as I knew that
rebase rewrote history, and I saw it do this to about 300 commits.
It promptly made merging absolute hell with the other few members of
my team, as it would.
Granted - this is a mistake on my part, and probably a common newbie
one, but something that came to mind when thinking about it later:
would it perhaps be an idea to have a way to mark a tree 'public', and
disallow rebase *unless* --force was passed, or it was a public tree?
(Then again, the alternative might be more 'intelligent' for new
users: start off with branches defaulting to private, and marking them
public, disallowing use of rebase, etc).
Thoughts, feedback, etc are welcome.
--
Robin Burchell
^ permalink raw reply
* Re: [PATCH] git commit: Repaint the output format bikeshed (again)
From: Shawn O. Pearce @ 2008-10-03 0:15 UTC (permalink / raw)
To: Jeff King; +Cc: Andreas Ericsson, Git Mailing List
In-Reply-To: <20081002211309.GB29480@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Thu, Oct 02, 2008 at 07:40:28AM +0200, Andreas Ericsson wrote:
> > [... many many many many many things about paints ...]
I think painting is over for now. Time to let the paint dry.
I applied Jeff's patch:
[jk/bikeshed] created bd8098f: "reformat informational commit message"
--
Shawn.
^ permalink raw reply
* Re: [PATCH] gitweb: Identify all summary metadata table rows
From: Shawn O. Pearce @ 2008-10-03 0:30 UTC (permalink / raw)
To: Petr Baudis; +Cc: git, Petr Baudis
In-Reply-To: <1222957505-5150-1-git-send-email-pasky@suse.cz>
Petr Baudis <pasky@suse.cz> wrote:
> In the metadata table of the summary page, all rows have their
> id (or class in case of URL) set now. This for example lets sites
> easily disable fields they do not want to show in their custom
> stylesheet (e.g. they are overly technical or irrelevant for the site).
>
> Many of my other patches depend on this, so I would appreciate to hear
> as soon as possible if someone has an issue with this patch.
Its pretty trivial and painless. So its applied. Marking up unique
elements so you can control them via CSS isn't rocket science. ;-)
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index da474d0..bd8124a 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4070,10 +4070,10 @@ sub git_summary {
>
> print "<div class=\"title\"> </div>\n";
> print "<table class=\"projects_list\">\n" .
> - "<tr><td>description</td><td>" . esc_html($descr) . "</td></tr>\n" .
> - "<tr><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
> + "<tr id=\"metadata_desc\"><td>description</td><td>" . esc_html($descr) . "</td></tr>\n" .
> + "<tr id=\"metadata_owner\"><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
> if (defined $cd{'rfc2822'}) {
> - print "<tr><td>last change</td><td>$cd{'rfc2822'}</td></tr>\n";
> + print "<tr id=\"metadata_lchange\"><td>last change</td><td>$cd{'rfc2822'}</td></tr>\n";
> }
>
> # use per project git URL list in $projectroot/$project/cloneurl
> @@ -4083,7 +4083,7 @@ sub git_summary {
> @url_list = map { "$_/$project" } @git_base_url_list unless @url_list;
> foreach my $git_url (@url_list) {
> next unless $git_url;
> - print "<tr><td>$url_tag</td><td>$git_url</td></tr>\n";
> + print "<tr class=\"metadata_url\"><td>$url_tag</td><td>$git_url</td></tr>\n";
> $url_tag = "";
> }
> print "</table>\n";
--
Shawn.
^ permalink raw reply
* Re: [PATCH] gitweb: Identify all summary metadata table rows
From: Jakub Narebski @ 2008-10-03 1:04 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Petr Baudis, git, Petr Baudis
In-Reply-To: <20081003003053.GT21310@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> Petr Baudis <pasky@suse.cz> wrote:
> > In the metadata table of the summary page, all rows have their
> > id (or class in case of URL) set now. This for example lets sites
> > easily disable fields they do not want to show in their custom
> > stylesheet (e.g. they are overly technical or irrelevant for the site).
> >
> > Many of my other patches depend on this, so I would appreciate to hear
> > as soon as possible if someone has an issue with this patch.
>
> Its pretty trivial and painless. So its applied. Marking up unique
> elements so you can control them via CSS isn't rocket science. ;-)
Somehow original patch didn't appear (yet) on git mailing list;
perhaps it ran afoul vger anti-SPAM filter...
> > + "<tr id=\"metadata_desc\"><td>description</td><td>" . esc_html($descr) . "</td></tr>\n" .
> > + "<tr id=\"metadata_owner\"><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
> > + print "<tr id=\"metadata_lchange\"><td>last change</td><td>$cd{'rfc2822'}</td></tr>\n";
> > + print "<tr class=\"metadata_url\"><td>$url_tag</td><td>$git_url</td></tr>\n";
I like the idea, and I think this can be seen as beginning to use
either microformats, or RDFa. You have marked those fragments for CSS
to show or hide; microformats are about marking those fragments for
machine to parse. Although microformats usually use 'class' (or 'rel'
for links), not 'id' attribute for classifying data...
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO
From: Jakub Narebski @ 2008-10-03 0:48 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <cb7bb73a0810020243v37759f79xfde4c32c49e1a375@mail.gmail.com>
On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:
> On Thu, Oct 2, 2008 at 10:59 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Giuseppe Bilotta wrote:
>>> - $action ||= "shortlog";
>>> - $hash ||= validate_refname($refname);
>>> + $action ||= "shortlog";
>>> + $hash ||= validate_refname($refname);
>>> + $hash_base ||= validate_refname($refname);
>>> }
>>> }
>>
>> This hunk is IMHO incorrect. First, $refname is _either_ $hash, or
>> $hash_base; it cannot be both. Second, in most cases (like the case
>> of 'shortlog' action, either explicit or implicit) it is simply $hash;
>> I think it can be $hash_base when $file_name is not set only in
>> singular exception case of 'tree' view for the top tree (lack of
>> filename is not an error, but is equivalent to $file_name='/').
>
> OTOH, while setting both $hash and $hash_base has worked fine for me
> so far (because the right one is automatically used and apparently
> setting the other doesn't hurt), choosing which one to set is a much
> hairier case. Do you have suggestions for a better way to always make
> it work?
Well, it is either checking $action and setting either $hash or
$hash_base, or setting both, with some comments on why and when it is
needed (as discussed on #git). IIUC $hash_base is needed only for
filename-taking tree actions which acts on top-tree, and therefore
don't need $file_name, like 'project/tree/branch' or related
'project/history/branch' (the latter is practically almost equivalent
to 'project/shortlog/branch' or 'project/branch').
I'm not sure if it wouldn't be better to call validate_refname($refname)
once, either as:
$hash_base ||= $hash ||= validate_refname($refname);
but that might be incorrect in the obscure case of setting $hash via 'h'
CGI query parameter, and letting gitweb to set-up $hash_base via
path_info, so perhaps ($refname is local to evaluate_path_info, IIRC)
$refname = validate_refname($refname);
$hash ||= $refname;
$hash_base ||= $refname;
But that is just nitpicking this fragment of code to death. In short:
either check which of $hash and $hash_base to set in this branch of
conditional, or explain why setting both $hash and $hash_base is needed,
and why it is acceptable, either as comments, or in commit message.
>>> @@ -631,8 +642,15 @@ sub href (%) {
>>> if ($params{-replay}) {
>>> while (my ($name, $symbol) = each %mapping) {
>>> if (!exists $params{$name}) {
>>> - # to allow for multivalued params we use arrayref form
>>> - $params{$name} = [ $cgi->param($symbol) ];
>>> + # the parameter we want to recycle may be either part of the
>>> + # list of CGI parameter, or recovered from PATH_INFO
>>> + if ($cgi->param($symbol)) {
>>> + # to allow for multivalued params we use arrayref form
>>> + $params{$name} = [ $cgi->param($symbol) ];
>>> + } else {
>>> + no strict 'refs';
>>> + $params{$name} = $$name if $$name;
>>
>> I would _perhaps_ add here comment that multivalued parameters can come
>> only from CGI query string, so there is no need for something like:
>>
>> + $params{$name} = (ref($$name) ? @$name : $$name) if $$name;
>>
>>> + }
>>> }
>>> }
>>> }
>>
>> This fragment is a bit of ugly code, hopefully corrected in later patch.
>> I think it would be better to have 'refactor parsing/validation of input
>> parameters' to be very fist patch in series; I am not sure but I suspect
>> that is a kind of bugfix for current "$project/$hash" ('shortlog' view)
>> and "$project/$hash_base:$file_name" ('blob_plain' and 'tree' view)
>> path_info.
>
> But implementing the path_info parsing first makes the input param
> refactoring SO much nicer that I would rather put a comment here
> saying "this code sucks: we should rather collect all input
> parameters" and then clean it up on the subsequent patch.
Why not cleanup first?
When implementing href(..., -replay=>1) I have forgot that some of
gitweb parameters are implicitly passed ($project, because it is needed
in most gitweb links), and some can be passed via path_info ($hash
and/or $hash_base, $file_name). Your code adds $action to the mix, but
it doesn't change the fact that 1.) even before your code -replay case
was incorrect for some path_info links (handcrafted, as gitweb generates
only $project via path_info); 2.) code you have added is a bit ugly.
Besides using variables change a little meaning of -replay, namely
in your code gitweb always sets action, even for non-path_name links
when we started from "default action" (i.e. without action set) links.
I guess this is mainly theoretical issue, as I don't think that default
views use many -replay links.
P.S. with the idea of pushing parameters obtained not from CGI query
string to $cgi->param() via "$cgi->param($name, $value);" or in named
params form "$cgi->(-name=>$name, -value=>$value);" you would not need
to change (a bit hacky, admittedly) href(...,-replay=>1) code.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCHv4] gitweb: refactor input parameters parse/validation
From: Jakub Narebski @ 2008-10-03 1:36 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <1222906234-8182-3-git-send-email-giuseppe.bilotta@gmail.com>
On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:
> Since input parameters can now be obtained both from CGI parameters and
> PATH_INFO, we would like most of the code to be agnostic about the way
> parameters were retrieved.
>
I'd prefer that this cleanup/refactoring patch was _first_ patch in
the series, as we were able to obtain parameters both from CGI query
parameters and from PATH_INFO ($project, $hash or $hash_base+$file_name)
_before_ first patch in this series. So it correct not only issue
introduced by first patch (and fixed somewhat there), but what was
outstanding (but rare because gitweb did not generate such links)
issue.
> We thus collect all the parameters into the new %input_params hash,
> removing the need for ad-hoc kludgy code in href().
Alternate solution would be push data from PATH_INFO in query params
data using (for example)
$cgi->param('a', $action);
or, naming parameters explicitely
$cgi->param(-name=>'a', -value=>$action);
This avoids need for additional variable, reuses current code, and
nicely sidesteps issue whether to use long names for keys ('action',
'file_name') or short ones from CGI query ('a', 'f').
It would probably has to be at least partially written to check which
of those two solutions (%input_params or $cgi->param('a', $a)))
is better.
> As much of the
> parameters validation code is now shared between CGI and PATH_INFO,
> although this requires PATH_INFO parsing to be interleaved into the main
> code instead of being factored out into its own routine.
I'm not sure if it is worth it then to unify parameters validation,
with such drawback.
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
> gitweb/gitweb.perl | 336 ++++++++++++++++++++++++++++------------------------
> 1 files changed, 183 insertions(+), 153 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index f088681..ec4326f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -28,8 +28,10 @@ our $my_url = $cgi->url();
> our $my_uri = $cgi->url(-absolute => 1);
>
> # if we're called with PATH_INFO, we have to strip that
> -# from the URL to find our real URL
> -if (my $path_info = $ENV{"PATH_INFO"}) {
> +# from the URL to find our real URL. PATH_INFO is kept
> +# as it's used later on for parameter extraction
> +my $path_info = $ENV{"PATH_INFO"};
> +if ($path_info) {
> $my_url =~ s,\Q$path_info\E$,,;
> $my_uri =~ s,\Q$path_info\E$,,;
> }
This might be separate patch, if you wanted to increase your commit
count ;-) More seriously I think it should be at least briefly
mentioned in commit message (make $path_info global).
> @@ -390,15 +392,111 @@ $projects_list ||= $projectroot;
>
> # ======================================================================
> # input validation and dispatch
> -our $action = $cgi->param('a');
> -if (defined $action) {
> - if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
> - die_error(400, "Invalid action parameter");
> +
> +# input parameters can be collected from a variety of sources (presently, CGI
> +# and PATH_INFO), so we define an %input_params hash that collects them all
> +# together during validation: this allows subsequent uses (e.g. href()) to be
> +# agnostic of the parameter origin
> +
> +my %input_params = ();
> +
> +# input parameters are stored with the long parameter name as key, so we need
> +# the name -> CGI key mapping here. This will also be used in the href
> +# subroutine to convert parameters to their CGI equivalent.
> +#
> +# XXX: Warning: If you touch this, check the search form for updating,
> +# too.
> +
> +my @cgi_param_mapping = (
> + project => "p",
> + action => "a",
> + file_name => "f",
> + file_parent => "fp",
> + hash => "h",
> + hash_parent => "hp",
> + hash_base => "hb",
> + hash_parent_base => "hpb",
> + page => "pg",
> + order => "o",
> + searchtext => "s",
> + searchtype => "st",
> + snapshot_format => "sf",
> + extra_options => "opt",
> + search_use_regexp => "sr",
> +);
> +my %cgi_param_mapping = @cgi_param_mapping;
> +
> +# we will also need to know the possible actions, for validation
> +my %actions = (
> + "blame" => \&git_blame,
> + "blobdiff" => \&git_blobdiff,
> + "blobdiff_plain" => \&git_blobdiff_plain,
> + "blob" => \&git_blob,
> + "blob_plain" => \&git_blob_plain,
> + "commitdiff" => \&git_commitdiff,
> + "commitdiff_plain" => \&git_commitdiff_plain,
> + "commit" => \&git_commit,
> + "forks" => \&git_forks,
> + "heads" => \&git_heads,
> + "history" => \&git_history,
> + "log" => \&git_log,
> + "rss" => \&git_rss,
> + "atom" => \&git_atom,
> + "search" => \&git_search,
> + "search_help" => \&git_search_help,
> + "shortlog" => \&git_shortlog,
> + "summary" => \&git_summary,
> + "tag" => \&git_tag,
> + "tags" => \&git_tags,
> + "tree" => \&git_tree,
> + "snapshot" => \&git_snapshot,
> + "object" => \&git_object,
> + # those below don't need $project
> + "opml" => \&git_opml,
> + "project_list" => \&git_project_list,
> + "project_index" => \&git_project_index,
> +);
Nice, although I'm not sure if [%@]cgi_param_mapping has to global.
If we use long parameters names as keys, I think it has to, somewhat.
See also comment below.
> +
> +# fill %input_params with the CGI parameters. All values except for 'opt'
> +# should be single values, but opt can be an array. We should probably
> +# build an array of parameters that can be multi-valued, but since for the time
> +# being it's only this one, we just single it out
> +while (my ($name, $symbol) = each %cgi_param_mapping) {
> + if ($symbol eq 'opt') {
> + $input_params{$name} = [$cgi->param($symbol)];
> + } else {
> + $input_params{$name} = $cgi->param($symbol);
> }
> }
If it was chosen to use short (CGI query) parameter names, the above
loop could be replaced by simple
%input_params = $cgi->Vars;
or to be more exact, if we want to have multi-valued parameters stored
as array ref
%input_params = map { [ split /\0/ ] if /\0/; } $cgi->Vars;
See CGI(3pm):
When using this [Vars], the thing you must watch out for are multivalued CGI
parameters. Because a hash cannot distinguish between scalar and list con-
text, multivalued parameters will be returned as a packed string, separated
by the "\0" (null) character. You must split this packed string in order
to get at the individual values.
> -# parameters which are pathnames
> -our $project = $cgi->param('p');
> +# next, we want to parse PATH_INFO (which was already stored in $path_info at
> +# the beginning). This is a little hairy because PATH_INFO parsing needs some
> +# form of parameter validation, so we interleave parsing and validation.
I don't think it is a good idea. In my opinion, for my taste, it would
be better to separate evaluating path_info from the rest.
We could instead introduce convention that if variable (like $project)
is set, then it is assumed to be validated; if it is present only in
the %input_params hash, then it has to be validated.
On the other hand this ordering, first by parameter, then by method
of extraction could be seem quite equally valid. Nevertheless I think
that previous flow with separate evaluate_path_info() and what should
be evaluate_CGI_query() has better encapsulation.
> +#
> +# The accepted PATH_INFO syntax is $project/$action/$hash or
> +# $project/$action/$hash_base:$file_name, where $action may be missing (mostly for
> +# backwards compatibility), so we need to parse and validate the parameters in
> +# this same order.
> +
> +# clear $path_info of any leading /
> +$path_info =~ s,^/+,,;
> +
> +our $project = $input_params{'project'};
> +if ($path_info && !defined $project) {
> + # if $project was not defined by CGI, we try to extract it from
> + # $path_info
> + $project = $path_info;
> + $project =~ s,/+$,,;
> + while ($project && !check_head_link("$projectroot/$project")) {
> + $project =~ s,/*[^/]*$,,;
> + }
> + $input_params{'project'} = $project;
> +} else {
> + # otherwise, we suppress $path_info parsing altogether
> + $path_info = undef;
> +}
> +
> +# project name validation
> if (defined $project) {
> if (!validate_pathname($project) ||
> !(-d "$projectroot/$project") ||
Note that this code does less checking if $project is in path_info than
for the case where it is set by CGI query. Perhaps there should be base
fast check in a loop, and more extensive test later.
> @@ -408,16 +506,66 @@ if (defined $project) {
> undef $project;
> die_error(404, "No such project");
> }
> +
> + # we purge the $project name from the $path_info, preparing it for
> + # subsequent parameters extraction
> + $path_info =~ s,^\Q$project\E/*,, if defined $path_info;
> +} else {
> + # we also suppress $path_info parsing if no project was defined
> + $path_info = undef;
> +}
In evaluate_path_info it was simply 'return if...'; here with mentioned
interleaving it is harder and a bit hacky.
[cut]
> @@ -615,43 +676,12 @@ sub href (%) {
> # default is to use -absolute url() i.e. $my_uri
> my $href = $params{-full} ? $my_url : $my_uri;
[cut removed, or rather moved to beginning, @mapping]
> $params{'project'} = $project unless exists $params{'project'};
>
> if ($params{-replay}) {
> - while (my ($name, $symbol) = each %mapping) {
> - if (!exists $params{$name}) {
> - # the parameter we want to recycle may be either part of the
> - # list of CGI parameter, or recovered from PATH_INFO
> - if ($cgi->param($symbol)) {
> - # to allow for multivalued params we use arrayref form
> - $params{$name} = [ $cgi->param($symbol) ];
> - } else {
> - no strict 'refs';
> - $params{$name} = $$name if $$name;
> - }
> - }
> + while (my ($name, $val) = each %input_params) {
> + $params{$name} = $input_params{$name}
> + unless (exists $params{$name});
Very nice, short code. Should be something like that from
the very beginning.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCHv4] gitweb: generate project/action/hash URLs
From: Jakub Narebski @ 2008-10-03 1:48 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <1222906234-8182-4-git-send-email-giuseppe.bilotta@gmail.com>
On Tue, 2 Oct 2008, Giuseppe Bilotta wrote:
> When generating path info URLs, reduce the number of CGI parameters by
> embedding action and hash_parent:filename or hash in the path.
_Perhaps_ it should be noted that even though gitweb accepted
'project/hash' and 'project/hash_base:file_name' path_info URLs, it
generated links with only 'project/' in path_info.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
> gitweb/gitweb.perl | 32 +++++++++++++++++++++++++++++---
> 1 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ec4326f..2c380ac 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -687,14 +687,40 @@ sub href (%) {
>
> my ($use_pathinfo) = gitweb_check_feature('pathinfo');
> if ($use_pathinfo) {
> - # use PATH_INFO for project name
> + # try to put as many parameters as possible in PATH_INFO:
> + # - project name
> + # - action
> + # - hash or hash_base:filename
> +
> + # Strip any trailing / from $href, or we might get double
> + # slashes when the script is the DirectoryIndex
Perhaps example, like $href='gitweb.example.com/', could be put here.
> + #
I think that we can lose this empty line comment here.
> + $href =~ s,/$,,;
> +
> + # Then add the project name, if present
> $href .= "/".esc_url($params{'project'}) if defined $params{'project'};
> delete $params{'project'};
>
> - # Summary just uses the project path URL
> - if (defined $params{'action'} && $params{'action'} eq 'summary') {
> + # Summary just uses the project path URL, any other action is
> + # added to the URL
> + if (defined $params{'action'}) {
> + $href .= "/".esc_url($params{'action'}) unless $params{'action'} eq 'summary';
> delete $params{'action'};
> }
Good.
> +
> + # Finally, we put either hash_base:file_name or hash
> + if (defined $params{'hash_base'}) {
> + $href .= "/".esc_url($params{'hash_base'});
> + if (defined $params{'file_name'}) {
> + $href .= ":".esc_url($params{'file_name'});
> + delete $params{'file_name'};
> + }
> + delete $params{'hash'};
> + delete $params{'hash_base'};
> + } elsif (defined $params{'hash'}) {
> + $href .= "/".esc_url($params{'hash'});
> + delete $params{'hash'};
> + }
Hmmmm...
Shouldn't the code first check for $file_name, then add either
"$hash_base:$file_name" (url-escaped), or "$hash" (not "$hash_base")?
> }
>
> # now encode the parameters explicitly
Thank you very much for work on improving gitweb.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] builtin-commit: avoid always using reduce_heads()
From: Shawn O. Pearce @ 2008-10-03 2:35 UTC (permalink / raw)
To: Miklos Vajna; +Cc: SZEDER Gabor, jnareb, Johannes.Schindelin, git
In-Reply-To: <1222457868-9864-1-git-send-email-vmiklos@frugalware.org>
Miklos Vajna <vmiklos@frugalware.org> wrote:
> diff --git a/builtin-commit.c b/builtin-commit.c
> index 55e1087..f546cf7 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -1040,6 +1050,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>
> unlink(git_path("MERGE_HEAD"));
> unlink(git_path("MERGE_MSG"));
> + unlink(git_path("MERGE_MODE"));
> unlink(git_path("SQUASH_MSG"));
>
> if (commit_index_files())
Hmmph. Should branch.c and builtin-reset.c clean this new file
up too?
> diff --git a/builtin-merge.c b/builtin-merge.c
> index 5c65a58..20102a0 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -1210,6 +1210,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> merge_msg.len)
> die("Could not write to %s", git_path("MERGE_MSG"));
> close(fd);
> + fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT, 0666);
> + if (fd < 0)
> + die("Could open %s for writing", git_path("MERGE_MODE"));
> + strbuf_reset(&buf);
> + if (!allow_fast_forward)
> + strbuf_addf(&buf, "no-ff");
> + if (write_in_full(fd, buf.buf, buf.len) != buf.len)
Shouldn't we open this file with O_TRUNC to avoid this scenario:
$ git merge --no-ff --no-commit foo
$ git reset --hard
$ git merge --no-commit foo
... *sigh* MERGE_MODE still has "no-ff" in it ...
This is especially true since some porcelain (e.g. git-gui) just
deletes MERGE_HEAD right now and doesn't know about cleaning up
MERGE_MODE. We'd want to at least reset it correctly on the next
invocation to git-merge.
--
Shawn.
^ permalink raw reply
* alias g to git in .gitconfig?
From: Rob Sanheim @ 2008-10-03 2:51 UTC (permalink / raw)
To: git
This is pretty trivial, but I'm a lazy typist.
Is it possible to alias 'g' to git via git config, instead of via
bash? If I do a plain bash alias then none of the nice autocompletion
from git-contrib work with 'g'.
- Rob
^ permalink raw reply
* Re: alias g to git in .gitconfig?
From: Shawn O. Pearce @ 2008-10-03 3:10 UTC (permalink / raw)
To: Rob Sanheim; +Cc: git
In-Reply-To: <fc113d400810021951hf95ff35qb1ccb4af45a71abe@mail.gmail.com>
Rob Sanheim <rsanheim@gmail.com> wrote:
> This is pretty trivial, but I'm a lazy typist.
>
> Is it possible to alias 'g' to git via git config, instead of via
> bash? If I do a plain bash alias then none of the nice autocompletion
> from git-contrib work with 'g'.
No, you'll need to alias 'g' to git in bash, and then if you still
want completion you'll need to also register the compgen to call
_git completion routine. Its two lines:
alias g=git
complete -o default -o nospace -F _git g
--
Shawn.
^ permalink raw reply
* [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"
From: David Bryson @ 2008-10-03 3:39 UTC (permalink / raw)
To: git
Signed-off-by: David Bryson <david@statichacks.org>
I tried to keep with the naming/coding conventions that I found in
remote.c. Feedback welcome.
---
remote.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/remote.c b/remote.c
index 3f3c789..893a739 100644
--- a/remote.c
+++ b/remote.c
@@ -305,6 +305,7 @@ static int handle_config(const char *key, const char *value, void *cb)
{
const char *name;
const char *subkey;
+ const char *v;
struct remote *remote;
struct branch *branch;
if (!prefixcmp(key, "branch.")) {
@@ -314,15 +315,15 @@ static int handle_config(const char *key, const char *value, void *cb)
return 0;
branch = make_branch(name, subkey - name);
if (!strcmp(subkey, ".remote")) {
- if (!value)
- return config_error_nonbool(key);
- branch->remote_name = xstrdup(value);
+ if (git_config_string(&v, key, value) )
+ return -1;
+ branch->remote_name = v;
if (branch == current_branch)
default_remote_name = branch->remote_name;
} else if (!strcmp(subkey, ".merge")) {
- if (!value)
- return config_error_nonbool(key);
- add_merge(branch, xstrdup(value));
+ if (git_config_string(&v, key, value ))
+ return -1;
+ add_merge(branch, v);
}
return 0;
}
@@ -334,9 +335,9 @@ static int handle_config(const char *key, const char *value, void *cb)
return 0;
rewrite = make_rewrite(name, subkey - name);
if (!strcmp(subkey, ".insteadof")) {
- if (!value)
- return config_error_nonbool(key);
- add_instead_of(rewrite, xstrdup(value));
+ if (git_config_string(&v, key, value ))
+ return -1;
+ add_instead_of(rewrite, v);
}
}
if (prefixcmp(key, "remote."))
--
1.6.0.2
^ permalink raw reply related
* Re: [PATCH] git commit: Repaint the output format bikeshed (again)
From: Jeff King @ 2008-10-03 4:24 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Andreas Ericsson, Git Mailing List
In-Reply-To: <20081003001556.GS21310@spearce.org>
On Thu, Oct 02, 2008 at 05:15:56PM -0700, Shawn O. Pearce wrote:
> I think painting is over for now. Time to let the paint dry.
> I applied Jeff's patch:
>
> [jk/bikeshed] created bd8098f: "reformat informational commit message"
Woo! Victory by attrition!
-Peff
^ permalink raw reply
* Re: [PATCH] Use "git_config_string" to simplify "remote.c" code in "handle_config"
From: Andreas Ericsson @ 2008-10-03 5:28 UTC (permalink / raw)
To: David Bryson; +Cc: git
In-Reply-To: <20081003033937.GA11594@eratosthenes.cryptobackpack.org>
David Bryson wrote:
> Signed-off-by: David Bryson <david@statichacks.org>
>
> I tried to keep with the naming/coding conventions that I found in
> remote.c. Feedback welcome.
>
> ---
> remote.c | 19 ++++++++++---------
> 1 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 3f3c789..893a739 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -305,6 +305,7 @@ static int handle_config(const char *key, const char *value, void *cb)
> {
> const char *name;
> const char *subkey;
> + const char *v;
Not very mnemonic. I'm sure you can think up a better name, even if it's
a long one. Git is notoriously sparse when it comes to comments. We rely
instead on self-explanatory code.
> struct remote *remote;
> struct branch *branch;
> if (!prefixcmp(key, "branch.")) {
> @@ -314,15 +315,15 @@ static int handle_config(const char *key, const char *value, void *cb)
> return 0;
> branch = make_branch(name, subkey - name);
> if (!strcmp(subkey, ".remote")) {
> - if (!value)
> - return config_error_nonbool(key);
> - branch->remote_name = xstrdup(value);
> + if (git_config_string(&v, key, value) )
> + return -1;
> + branch->remote_name = v;
> if (branch == current_branch)
> default_remote_name = branch->remote_name;
> } else if (!strcmp(subkey, ".merge")) {
> - if (!value)
> - return config_error_nonbool(key);
> - add_merge(branch, xstrdup(value));
> + if (git_config_string(&v, key, value ))
> + return -1;
> + add_merge(branch, v);
> }
> return 0;
> }
> @@ -334,9 +335,9 @@ static int handle_config(const char *key, const char *value, void *cb)
> return 0;
> rewrite = make_rewrite(name, subkey - name);
> if (!strcmp(subkey, ".insteadof")) {
> - if (!value)
> - return config_error_nonbool(key);
> - add_instead_of(rewrite, xstrdup(value));
> + if (git_config_string(&v, key, value ))
> + return -1;
> + add_instead_of(rewrite, v);
> }
> }
> if (prefixcmp(key, "remote."))
Other than that, the patch looks good.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO
From: Giuseppe Bilotta @ 2008-10-03 5:54 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Petr Baudis, git, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <200810030041.54563.jnareb@gmail.com>
On Fri, Oct 3, 2008 at 12:41 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Petr Baudis wrote:
>> On Thu, Oct 02, 2008 at 11:05:18PM +0200, Giuseppe Bilotta wrote:
>> >
>> > In preparing the new patchset, I've put a big comment block explaining
>> > why we need to set both $hash and $hash_base in this code-path:
>> >
>> > # we got "project.git/[action/]branch". in this case
>> > # we set both $hash and $hash_base because different actions
>> > # need one or the other to be set to behave correctly.
>> > #
>> > # For example, if $hash_base is not set then the blob and
>
> 'blob' without $file_name doesn't have sense, but 'tree' does.
> Probably should be s/blob/tree/ above.
>
>> > # history links on the page project.git/tree/somebranch will
>> > # assume a $hash_base of HEAD instead of the correct
>> > # somebranch.
Note: the blob and history links _in a tree page_ are wrong. The page
itself is correct.
What this means is that if you go to project.git/tree/somebranch you
get the correct list of files and directories for the project at
somebranch's HEAD, but all the links IN that page will point to the
wrong version, because they will have no $hash_base set and will thus
default to HEAD.
You can see this effect for example by going to
http://repo.or.cz/w/git.git/v1.4.0?a=tree <- this hand-crafted path
would never be generated by the old pathinfo code, but it show exactly
what would happen with my pahtinfo code on
http://repo.or.cz/w/git.git/tree/v1.4.0 if $hash_base wasn't set.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [PATCH] rebase -i: proper prepare-commit-msg hook argument when squashing
From: Jeff King @ 2008-10-03 6:01 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Shawn O. Pearce, git
In-Reply-To: <1222992501-943-1-git-send-email-szeder@ira.uka.de>
On Fri, Oct 03, 2008 at 02:08:21AM +0200, SZEDER Gábor wrote:
> + cp -v "$MSG" "$GIT_DIR"/SQUASH_MSG
Sorry, but "cp -v" is not portable. It's not in POSIX, and this breaks
the script for (at least) Solaris.
However, it's not even clear to me why "-v" is used at all, considering
that the "squash" case above does not use it. Is it a debugging
leftover? Am I missing something?
[Aside: My Solaris 8 autobuilder is now running, which was a huge pain.
However, it is very satisfying to catch things like this in "next"
before they hit a wider audience.]
-Peff
^ permalink raw reply
* Re: [PATCHv4] gitweb: parse project/action/hash_base:filename PATH_INFO
From: Giuseppe Bilotta @ 2008-10-03 6:04 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Petr Baudis, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <200810030248.57144.jnareb@gmail.com>
On Fri, Oct 3, 2008 at 2:48 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:
>> OTOH, while setting both $hash and $hash_base has worked fine for me
>> so far (because the right one is automatically used and apparently
>> setting the other doesn't hurt), choosing which one to set is a much
>> hairier case. Do you have suggestions for a better way to always make
>> it work?
>
> Well, it is either checking $action and setting either $hash or
> $hash_base, or setting both, with some comments on why and when it is
> needed (as discussed on #git). IIUC $hash_base is needed only for
> filename-taking tree actions which acts on top-tree, and therefore
> don't need $file_name, like 'project/tree/branch' or related
> 'project/history/branch' (the latter is practically almost equivalent
> to 'project/shortlog/branch' or 'project/branch').
>
> I'm not sure if it wouldn't be better to call validate_refname($refname)
> once, either as:
>
> $hash_base ||= $hash ||= validate_refname($refname);
>
> but that might be incorrect in the obscure case of setting $hash via 'h'
> CGI query parameter, and letting gitweb to set-up $hash_base via
> path_info, so perhaps ($refname is local to evaluate_path_info, IIRC)
>
> $refname = validate_refname($refname);
> $hash ||= $refname;
> $hash_base ||= $refname;
I'll go with this.
> But that is just nitpicking this fragment of code to death. In short:
> either check which of $hash and $hash_base to set in this branch of
> conditional, or explain why setting both $hash and $hash_base is needed,
> and why it is acceptable, either as comments, or in commit message.
Comment is probably better, as long as I remember to move it with the
code it belongs to ;)
>>>> @@ -631,8 +642,15 @@ sub href (%) {
>>>> if ($params{-replay}) {
>>>> while (my ($name, $symbol) = each %mapping) {
>>>> if (!exists $params{$name}) {
>>>> - # to allow for multivalued params we use arrayref form
>>>> - $params{$name} = [ $cgi->param($symbol) ];
>>>> + # the parameter we want to recycle may be either part of the
>>>> + # list of CGI parameter, or recovered from PATH_INFO
>>>> + if ($cgi->param($symbol)) {
>>>> + # to allow for multivalued params we use arrayref form
>>>> + $params{$name} = [ $cgi->param($symbol) ];
>>>> + } else {
>>>> + no strict 'refs';
>>>> + $params{$name} = $$name if $$name;
>>>
>>> I would _perhaps_ add here comment that multivalued parameters can come
>>> only from CGI query string, so there is no need for something like:
>>>
>>> + $params{$name} = (ref($$name) ? @$name : $$name) if $$name;
>>>
>>>> + }
>>>> }
>>>> }
>>>> }
>>>
>>> This fragment is a bit of ugly code, hopefully corrected in later patch.
>>> I think it would be better to have 'refactor parsing/validation of input
>>> parameters' to be very fist patch in series; I am not sure but I suspect
>>> that is a kind of bugfix for current "$project/$hash" ('shortlog' view)
>>> and "$project/$hash_base:$file_name" ('blob_plain' and 'tree' view)
>>> path_info.
>>
>> But implementing the path_info parsing first makes the input param
>> refactoring SO much nicer that I would rather put a comment here
>> saying "this code sucks: we should rather collect all input
>> parameters" and then clean it up on the subsequent patch.
>
> Why not cleanup first?
Because cleaning it up depends on the refactoring, and the refactoring
is much cleaner when path_info already handles $action too.
> When implementing href(..., -replay=>1) I have forgot that some of
> gitweb parameters are implicitly passed ($project, because it is needed
> in most gitweb links), and some can be passed via path_info ($hash
> and/or $hash_base, $file_name). Your code adds $action to the mix, but
> it doesn't change the fact that 1.) even before your code -replay case
> was incorrect for some path_info links (handcrafted, as gitweb generates
> only $project via path_info); 2.) code you have added is a bit ugly.
>
> Besides using variables change a little meaning of -replay, namely
> in your code gitweb always sets action, even for non-path_name links
> when we started from "default action" (i.e. without action set) links.
> I guess this is mainly theoretical issue, as I don't think that default
> views use many -replay links.
Ah the issue of the default action is something I hadn't taken into
consideration, actually. Now the question is, should replay keep
default -> default, or should it go with default -> last incantation?
> P.S. with the idea of pushing parameters obtained not from CGI query
> string to $cgi->param() via "$cgi->param($name, $value);" or in named
> params form "$cgi->(-name=>$name, -value=>$value);" you would not need
> to change (a bit hacky, admittedly) href(...,-replay=>1) code.
Yes, but it would muddy the waters about 'where did this parameter
come from' in case we ever need to know that.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* [PATCH] Makefile: do not set NEEDS_LIBICONV for Solaris 8
From: Jeff King @ 2008-10-03 6:39 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Brandon Casey, git
This breaks my build on Solaris 8, as there is no separate
libiconv.
The history of this line is somewhat convoluted. In 2fd955c
(in November 2005), NEEDS_LIBICONV was turned on for all
Solaris builds, claiming to "fix an error in Solaris 10 by
setting NEEDS_LIBICONV".
Later, e15f545 (in February of 2006) claimed that "Solaris
9+ don't need iconv", and moved NEEDS_LIBICONV into a
section for Solaris 8.
Furthermore, Brandon Casey claims in
<5A1KxlhmUIHe8iXPxnXYuNXsq0Yjlbwkz2eBin3z7ELuL9nK-4tSpw@cipher.nrlssc.navy.mil>
that he does not set NEEDS_LIBICONV for Solaris 7.
So either one of those commits is totally wrong, or there is
some other magic going on where some Solaris installs need
it and others don't.
Given Brandon's statement and my problems on Solaris 8 with
NEEDS_LIBICONV, I am inclined to think the first commit was
bogus, and that NEEDS_LIBICONV shouldn't be set for Solaris
at all by default. If somebody wants to use iconv and has
installed it manually, they can set it in their config.mak.
Signed-off-by: Jeff King <peff@peff.net>
---
Makefile | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/Makefile b/Makefile
index 3abbb4e..308dc70 100644
--- a/Makefile
+++ b/Makefile
@@ -651,7 +651,6 @@ ifeq ($(uname_S),SunOS)
NO_MKDTEMP = YesPlease
OLD_ICONV = UnfortunatelyYes
ifeq ($(uname_R),5.8)
- NEEDS_LIBICONV = YesPlease
NO_UNSETENV = YesPlease
NO_SETENV = YesPlease
NO_C99_FORMAT = YesPlease
--
1.6.0.2.624.g13e35
^ permalink raw reply related
* Re: [PATCHv4] gitweb: refactor input parameters parse/validation
From: Giuseppe Bilotta @ 2008-10-03 7:24 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Petr Baudis, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <200810030336.31469.jnareb@gmail.com>
On Fri, Oct 3, 2008 at 3:36 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:
>
>> Since input parameters can now be obtained both from CGI parameters and
>> PATH_INFO, we would like most of the code to be agnostic about the way
>> parameters were retrieved.
>
> I'd prefer that this cleanup/refactoring patch was _first_ patch in
> the series, as we were able to obtain parameters both from CGI query
> parameters and from PATH_INFO ($project, $hash or $hash_base+$file_name)
> _before_ first patch in this series. So it correct not only issue
> introduced by first patch (and fixed somewhat there), but what was
> outstanding (but rare because gitweb did not generate such links)
> issue.
It's true that the patch makes sense regardless of the rest of the
path_info patch, indeed.
>> We thus collect all the parameters into the new %input_params hash,
>> removing the need for ad-hoc kludgy code in href().
>
> Alternate solution would be push data from PATH_INFO in query params
> data using (for example)
>
> $cgi->param('a', $action);
>
> or, naming parameters explicitely
>
> $cgi->param(-name=>'a', -value=>$action);
>
> This avoids need for additional variable, reuses current code, and
> nicely sidesteps issue whether to use long names for keys ('action',
> 'file_name') or short ones from CGI query ('a', 'f').
>
>
> It would probably has to be at least partially written to check which
> of those two solutions (%input_params or $cgi->param('a', $a)))
> is better.
If we really don't want to care where the parameters came from *at
all*, writing on $cgi->param is likely to be the cleanest solution.
>> As much of the
>> parameters validation code is now shared between CGI and PATH_INFO,
>> although this requires PATH_INFO parsing to be interleaved into the main
>> code instead of being factored out into its own routine.
>
> I'm not sure if it is worth it then to unify parameters validation,
> with such drawback.
Yeah, I don't like it very much either. But it does spare a little on
the validation. OTOH, the amount we spare is not extraordinary, so
it's probably not worth the spaghettization of the code ...
>> # if we're called with PATH_INFO, we have to strip that
>> -# from the URL to find our real URL
>> -if (my $path_info = $ENV{"PATH_INFO"}) {
>> +# from the URL to find our real URL. PATH_INFO is kept
>> +# as it's used later on for parameter extraction
>> +my $path_info = $ENV{"PATH_INFO"};
>> +if ($path_info) {
>> $my_url =~ s,\Q$path_info\E$,,;
>> $my_uri =~ s,\Q$path_info\E$,,;
>> }
>
> This might be separate patch, if you wanted to increase your commit
> count ;-) More seriously I think it should be at least briefly
> mentioned in commit message (make $path_info global).
Note however that the path_info evaluation is _destructive_, so after
evaluation is complete we don't have much of it left.
[snip]
> Nice, although I'm not sure if [%@]cgi_param_mapping has to global.
> If we use long parameters names as keys, I think it has to, somewhat.
> See also comment below.
>> +# fill %input_params with the CGI parameters. All values except for 'opt'
>> +# should be single values, but opt can be an array. We should probably
>> +# build an array of parameters that can be multi-valued, but since for the time
>> +# being it's only this one, we just single it out
>> +while (my ($name, $symbol) = each %cgi_param_mapping) {
>> + if ($symbol eq 'opt') {
>> + $input_params{$name} = [$cgi->param($symbol)];
>> + } else {
>> + $input_params{$name} = $cgi->param($symbol);
>> }
>> }
>
> If it was chosen to use short (CGI query) parameter names, the above
> loop could be replaced by simple
>
> %input_params = $cgi->Vars;
>
> or to be more exact, if we want to have multi-valued parameters stored
> as array ref
>
> %input_params = map { [ split /\0/ ] if /\0/; } $cgi->Vars;
>
>
> See CGI(3pm):
>
> When using this [Vars], the thing you must watch out for are multivalued CGI
> parameters. Because a hash cannot distinguish between scalar and list con-
> text, multivalued parameters will be returned as a packed string, separated
> by the "\0" (null) character. You must split this packed string in order
> to get at the individual values.
Ah, an interesting alternative. This would make parameter copying a
one-liner, almost as good as just using $cgi->param for everything :)
>> -# parameters which are pathnames
>> -our $project = $cgi->param('p');
>> +# next, we want to parse PATH_INFO (which was already stored in $path_info at
>> +# the beginning). This is a little hairy because PATH_INFO parsing needs some
>> +# form of parameter validation, so we interleave parsing and validation.
>
> I don't think it is a good idea. In my opinion, for my taste, it would
> be better to separate evaluating path_info from the rest.
>
> We could instead introduce convention that if variable (like $project)
> is set, then it is assumed to be validated; if it is present only in
> the %input_params hash, then it has to be validated.
>
>
> On the other hand this ordering, first by parameter, then by method
> of extraction could be seem quite equally valid. Nevertheless I think
> that previous flow with separate evaluate_path_info() and what should
> be evaluate_CGI_query() has better encapsulation.
>
>> +#
>> +# The accepted PATH_INFO syntax is $project/$action/$hash or
>> +# $project/$action/$hash_base:$file_name, where $action may be missing (mostly for
>> +# backwards compatibility), so we need to parse and validate the parameters in
>> +# this same order.
>> +
>> +# clear $path_info of any leading /
>> +$path_info =~ s,^/+,,;
>> +
>> +our $project = $input_params{'project'};
>> +if ($path_info && !defined $project) {
>> + # if $project was not defined by CGI, we try to extract it from
>> + # $path_info
>> + $project = $path_info;
>> + $project =~ s,/+$,,;
>> + while ($project && !check_head_link("$projectroot/$project")) {
>> + $project =~ s,/*[^/]*$,,;
>> + }
>> + $input_params{'project'} = $project;
>> +} else {
>> + # otherwise, we suppress $path_info parsing altogether
>> + $path_info = undef;
>> +}
>> +
>> +# project name validation
>> if (defined $project) {
>> if (!validate_pathname($project) ||
>> !(-d "$projectroot/$project") ||
>
> Note that this code does less checking if $project is in path_info than
> for the case where it is set by CGI query. Perhaps there should be base
> fast check in a loop, and more extensive test later.
Uh ... isn't this exactly what's happening? In the loop we just gobble
until we find a git dir. Validation is then done, and it's the _same_
validation for both cases. Why do you say that path_info $project is
less checked?
>> @@ -408,16 +506,66 @@ if (defined $project) {
>> undef $project;
>> die_error(404, "No such project");
>> }
>> +
>> + # we purge the $project name from the $path_info, preparing it for
>> + # subsequent parameters extraction
>> + $path_info =~ s,^\Q$project\E/*,, if defined $path_info;
>> +} else {
>> + # we also suppress $path_info parsing if no project was defined
>> + $path_info = undef;
>> +}
>
> In evaluate_path_info it was simply 'return if...'; here with mentioned
> interleaving it is harder and a bit hacky.
I know.
>> $params{'project'} = $project unless exists $params{'project'};
>>
>> if ($params{-replay}) {
>> - while (my ($name, $symbol) = each %mapping) {
>> - if (!exists $params{$name}) {
>> - # the parameter we want to recycle may be either part of the
>> - # list of CGI parameter, or recovered from PATH_INFO
>> - if ($cgi->param($symbol)) {
>> - # to allow for multivalued params we use arrayref form
>> - $params{$name} = [ $cgi->param($symbol) ];
>> - } else {
>> - no strict 'refs';
>> - $params{$name} = $$name if $$name;
>> - }
>> - }
>> + while (my ($name, $val) = each %input_params) {
>> + $params{$name} = $input_params{$name}
>> + unless (exists $params{$name});
>
> Very nice, short code. Should be something like that from
> the very beginning.
Ok, I'll try working up a patch for params merging before any
path_info extensions.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* [PATCH] gitweb: Support for simple project search form
From: Petr Baudis @ 2008-10-03 7:29 UTC (permalink / raw)
To: git, git; +Cc: Petr Baudis
In-Reply-To: <1222960621-12044-1-git-send-email-pasky@suse.cz>
This is a trivial patch adding support for searching projects by name
and description, making use of the "infrastructure" provided by the
tag cloud generation.
Signed-off-by: Petr Baudis <petr.baudis@novartis.com>
---
gitweb/gitweb.css | 4 ++++
gitweb/gitweb.perl | 12 ++++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 07f5b53..a01eac8 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -435,6 +435,10 @@ div.search {
right: 12px
}
+p.projsearch {
+ text-align: center;
+}
+
td.linenr {
text-align: right;
}
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 07fa1e6..4bc8a12 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3758,11 +3758,14 @@ sub git_project_list_body {
my $pr = $projects[$i];
next if $tagfilter and $show_ctags and not grep { lc $_ eq lc $tagfilter } keys %{$pr->{'ctags'}};
- # Weed out forks
+ next if $searchtext and not $pr->{'path'} =~ /$searchtext/
+ and not $pr->{'descr_long'} =~ /$searchtext/;
+ # Weed out forks or non-matching entries of search
if ($check_forks) {
my $forkbase = $project; $forkbase ||= ''; $forkbase =~ s#\.git$#/#;
$forkbase="^$forkbase" if $forkbase;
- next if not $tagfilter and $pr->{'path'} =~ m#$forkbase.*/.*#; # regexp-safe
+ next if not $searchtext and not $tagfilter and $show_ctags
+ and $pr->{'path'} =~ m#$forkbase.*/.*#; # regexp-safe
}
if ($alternate) {
@@ -4099,6 +4102,11 @@ sub git_project_list {
close $fd;
print "</div>\n";
}
+ print $cgi->startform(-method => "get") .
+ "<p class=\"projsearch\">Search:\n" .
+ $cgi->textfield(-name => "s", -value => $searchtext) . "\n" .
+ "</p>" .
+ $cgi->end_form() . "\n";
git_project_list_body(\@list, $order);
git_footer_html();
}
--
tg: (0d352a0..) t/misc/projsearch (depends on: t/tagcloud/forks)
^ permalink raw reply related
* [PATCH] git-gui: Add a search command to the blame viewer.
From: Alexander Gavrilov @ 2008-10-03 7:36 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <1223019414-24643-1-git-send-email-angavrilov@gmail.com>
One of the largest deficiencies in the blame viewer at
the moment is the impossibility to search for a text
string. This commit fixes it by adding a Firefox-like
search panel to the viewer.
The panel can be shown by pressing F7 or clicking a
menu entry, and is hidden by pressing Esc. Find Next
is available through the F3 key.
Implementation is based on the gitk code, but heavily
refactored. It now also supports case-insensitive
searches, and uses the text box background color to
signal success or failure of the search.
Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
I would have used Ctrl-F, but it is already occupied.
-- Alexander
git-gui.sh | 3 +
lib/blame.tcl | 37 ++++++++++-
lib/search.tcl | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 227 insertions(+), 3 deletions(-)
create mode 100644 lib/search.tcl
diff --git a/git-gui.sh b/git-gui.sh
index 79a108d..4f95139 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -591,6 +591,7 @@ bind . <Visibility> {
if {[is_Windows]} {
wm iconbitmap . -default $oguilib/git-gui.ico
+ set ::tk::AlwaysShowSelection 1
}
######################################################################
@@ -1067,6 +1068,8 @@ set selected_commit_type new
set nullid "0000000000000000000000000000000000000000"
set nullid2 "0000000000000000000000000000000000000001"
+set have_tk85 [expr {[package vcompare $tk_version "8.5"] >= 0}]
+
######################################################################
##
## task management
diff --git a/lib/blame.tcl b/lib/blame.tcl
index 221313c..a45784c 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -21,9 +21,11 @@ field w_amov ; # text column: annotations + move tracking
field w_asim ; # text column: annotations (simple computation)
field w_file ; # text column: actual file data
field w_cviewer ; # pane showing commit message
+field finder ; # find mini-dialog frame
field status ; # status mega-widget instance
field old_height ; # last known height of $w.file_pane
+
# Tk UI colors
#
variable active_color #c0edc5
@@ -59,7 +61,7 @@ field tooltip_timer {} ; # Current timer event for our tooltip
field tooltip_commit {} ; # Commit(s) in tooltip
constructor new {i_commit i_path i_jump} {
- global cursor_ptr
+ global cursor_ptr M1B M1T have_tk85
variable active_color
variable group_colors
@@ -199,6 +201,11 @@ constructor new {i_commit i_path i_jump} {
-width 80 \
-xscrollcommand [list $w.file_pane.out.sbx set] \
-font font_diff
+ if {$have_tk85} {
+ $w_file configure -inactiveselectbackground darkblue
+ }
+ $w_file tag conf found \
+ -background yellow
set w_columns [list $w_amov $w_asim $w_line $w_file]
@@ -219,6 +226,11 @@ constructor new {i_commit i_path i_jump} {
-weight 1
grid rowconfigure $w.file_pane.out 0 -weight 1
+ set finder [::searchbar::new \
+ $w.file_pane.out.ff $w_file \
+ -column [expr {[llength $w_columns] - 1}] \
+ ]
+
set w_cviewer $w.file_pane.cm.t
text $w_cviewer \
-background white \
@@ -259,6 +271,10 @@ constructor new {i_commit i_path i_jump} {
-label [mc "Copy Commit"] \
-command [cb _copycommit]
$w.ctxm add separator
+ $w.ctxm add command \
+ -label [mc "Find Text..."] \
+ -accelerator F7 \
+ -command [list searchbar::show $finder]
menu $w.ctxm.enc
build_encoding_menu $w.ctxm.enc [cb _setencoding]
$w.ctxm add cascade \
@@ -280,9 +296,15 @@ constructor new {i_commit i_path i_jump} {
$i tag conf color$g -background [lindex $group_colors $g]
}
+ if {$i eq $w_file} {
+ $w_file tag raise found
+ }
+ $i tag raise sel
+
$i conf -cursor $cursor_ptr
- $i conf -yscrollcommand [list many2scrollbar \
- $w_columns yview $w.file_pane.out.sby]
+ $i conf -yscrollcommand \
+ "[list ::searchbar::scrolled $finder]
+ [list many2scrollbar $w_columns yview $w.file_pane.out.sby]"
bind $i <Button-1> "
[cb _hide_tooltip]
[cb _click $i @%x,%y]
@@ -319,6 +341,11 @@ constructor new {i_commit i_path i_jump} {
bind $w_cviewer <Tab> "[list focus $w_file];break"
bind $w_cviewer <Button-1> [list focus $w_cviewer]
bind $w_file <Visibility> [list focus $w_file]
+ bind $top <F7> [list searchbar::show $finder]
+ bind $top <Escape> [list searchbar::hide $finder]
+ bind $top <F3> [list searchbar::find_next $finder]
+ bind $top <Shift-F3> [list searchbar::find_prev $finder]
+ catch { bind $top <Shift-Key-XF86_Switch_VT_3> [list searchbar::find_prev $finder] }
grid configure $w.header -sticky ew
grid configure $w.file_pane -sticky nsew
@@ -873,6 +900,10 @@ method _showcommit {cur_w lno} {
foreach i $w_columns {
$i tag conf g$cmit -background $active_color
$i tag raise g$cmit
+ if {$i eq $w_file} {
+ $w_file tag raise found
+ }
+ $i tag raise sel
}
set author_name {}
diff --git a/lib/search.tcl b/lib/search.tcl
new file mode 100644
index 0000000..d292f20
--- /dev/null
+++ b/lib/search.tcl
@@ -0,0 +1,190 @@
+# incremental search panel
+# based on code from gitk, Copyright (C) Paul Mackerras
+
+class searchbar {
+
+field w
+field ctext
+
+field searchstring {}
+field casesensitive 1
+field searchdirn -forwards
+
+field smarktop
+field smarkbot
+
+constructor new {i_w i_text args} {
+ set w $i_w
+ set ctext $i_text
+
+ frame $w
+ label $w.l -text [mc Find:]
+ button $w.bn -text [mc Next] -command [cb find_next]
+ button $w.bp -text [mc Prev] -command [cb find_prev]
+ checkbutton $w.cs -text [mc Case-Sensitive] \
+ -variable ${__this}::casesensitive -command [cb _incrsearch]
+ entry $w.ent -textvariable ${__this}::searchstring -background lightgreen
+ pack $w.l -side left
+ pack $w.cs -side right
+ pack $w.bp -side right
+ pack $w.bn -side right
+ pack $w.ent -side left -expand 1 -fill x
+
+ eval grid conf $w -sticky we $args
+ grid remove $w
+
+ trace add variable searchstring write [cb _incrsearch_cb]
+
+ bind $w <Destroy> [cb delete_this]
+ return $this
+}
+
+method show {} {
+ if {![winfo ismapped $w]} {
+ grid $w
+ }
+ focus -force $w.ent
+}
+
+method hide {} {
+ if {[winfo ismapped $w]} {
+ focus $ctext
+ grid remove $w
+ }
+}
+
+method _get_new_anchor {} {
+ # use start of selection if it is visible,
+ # or the bounds of the visible area
+ set top [$ctext index @0,0]
+ set bottom [$ctext index @0,[winfo height $ctext]]
+ set sel [$ctext tag ranges sel]
+ if {$sel ne {}} {
+ set spos [lindex $sel 0]
+ if {[lindex $spos 0] >= [lindex $top 0] &&
+ [lindex $spos 0] <= [lindex $bottom 0]} {
+ return $spos
+ }
+ }
+ if {$searchdirn eq "-forwards"} {
+ return $top
+ } else {
+ return $bottom
+ }
+}
+
+method _get_wrap_anchor {dir} {
+ if {$dir eq "-forwards"} {
+ return 1.0
+ } else {
+ return end
+ }
+}
+
+method _do_search {start {mlenvar {}} {dir {}} {endbound {}}} {
+ set cmd [list $ctext search]
+ if {$mlenvar ne {}} {
+ upvar $mlenvar mlen
+ lappend cmd -count mlen
+ }
+ if {!$casesensitive} {
+ lappend cmd -nocase
+ }
+ if {$dir eq {}} {
+ set dir $searchdirn
+ }
+ lappend cmd $dir -- $searchstring
+ if {$endbound ne {}} {
+ set here [eval $cmd [list $start] [list $endbound]]
+ } else {
+ set here [eval $cmd [list $start]]
+ if {$here eq {}} {
+ set here [eval $cmd [_get_wrap_anchor $this $dir]]
+ }
+ }
+ return $here
+}
+
+method _incrsearch_cb {name ix op} {
+ after idle [cb _incrsearch]
+}
+
+method _incrsearch {} {
+ $ctext tag remove found 1.0 end
+ if {[catch {$ctext index anchor}]} {
+ $ctext mark set anchor [_get_new_anchor $this]
+ }
+ if {$searchstring ne {}} {
+ set here [_do_search $this anchor mlen]
+ if {$here ne {}} {
+ $ctext see $here
+ $ctext tag remove sel 1.0 end
+ $ctext tag add sel $here "$here + $mlen c"
+ $w.ent configure -background lightgreen
+ _set_marks $this 1
+ } else {
+ $w.ent configure -background lightpink
+ }
+ }
+}
+
+method find_prev {} {
+ find_next $this -backwards
+}
+
+method find_next {{dir -forwards}} {
+ focus $w.ent
+ $w.ent icursor end
+ set searchdirn $dir
+ $ctext mark unset anchor
+ if {$searchstring ne {}} {
+ set start [_get_new_anchor $this]
+ if {$dir eq "-forwards"} {
+ set start "$start + 1c"
+ }
+ set match [_do_search $this $start mlen]
+ $ctext tag remove sel 1.0 end
+ if {$match ne {}} {
+ $ctext see $match
+ $ctext tag add sel $match "$match + $mlen c"
+ }
+ }
+}
+
+method _mark_range {first last} {
+ set mend $first.0
+ while {1} {
+ set match [_do_search $this $mend mlen -forwards $last.end]
+ if {$match eq {}} break
+ set mend "$match + $mlen c"
+ $ctext tag add found $match $mend
+ }
+}
+
+method _set_marks {doall} {
+ set topline [lindex [split [$ctext index @0,0] .] 0]
+ set botline [lindex [split [$ctext index @0,[winfo height $ctext]] .] 0]
+ if {$doall || $botline < $smarktop || $topline > $smarkbot} {
+ # no overlap with previous
+ _mark_range $this $topline $botline
+ set smarktop $topline
+ set smarkbot $botline
+ } else {
+ if {$topline < $smarktop} {
+ _mark_range $this $topline [expr {$smarktop-1}]
+ set smarktop $topline
+ }
+ if {$botline > $smarkbot} {
+ _mark_range $this [expr {$smarkbot+1}] $botline
+ set smarkbot $botline
+ }
+ }
+}
+
+method scrolled {} {
+ if {$searchstring ne {}} {
+ after idle [cb _set_marks 0]
+ }
+}
+
+}
\ No newline at end of file
--
1.6.0.20.g6148bc
^ permalink raw reply related
* [PATCH] git-gui: Fix the blame window shape.
From: Alexander Gavrilov @ 2008-10-03 7:36 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
On modern high-resolution monitors the blame viewer
window is very high, yet too narrow. This patch
makes it gravitate to a more sane resolution, which
takes the font size into account.
It also changes the default text view size to 80% of
the window, and slightly modifies the border decorations
for better appearance.
Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
lib/blame.tcl | 19 +++++++++++++------
1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/lib/blame.tcl b/lib/blame.tcl
index eb61374..221313c 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -69,6 +69,8 @@ constructor new {i_commit i_path i_jump} {
make_toplevel top w
wm title $top [append "[appname] ([reponame]): " [mc "File Viewer"]]
+ set font_w [font measure font_diff "0"]
+
frame $w.header -background gold
label $w.header.commit_l \
-text [mc "Commit:"] \
@@ -114,9 +116,9 @@ constructor new {i_commit i_path i_jump} {
pack $w_path -fill x -side right
pack $w.header.path_l -side right
- panedwindow $w.file_pane -orient vertical
- frame $w.file_pane.out
- frame $w.file_pane.cm
+ panedwindow $w.file_pane -orient vertical -borderwidth 0 -sashwidth 3
+ frame $w.file_pane.out -relief flat -borderwidth 1
+ frame $w.file_pane.cm -relief sunken -borderwidth 1
$w.file_pane add $w.file_pane.out \
-sticky nsew \
-minsize 100 \
@@ -328,9 +330,14 @@ constructor new {i_commit i_path i_jump} {
set req_w [winfo reqwidth $top]
set req_h [winfo reqheight $top]
- set scr_h [expr {[winfo screenheight $top] - 100}]
- if {$req_w < 600} {set req_w 600}
+ set scr_w [expr {[winfo screenwidth $top] - 40}]
+ set scr_h [expr {[winfo screenheight $top] - 120}]
+ set opt_w [expr {$font_w * (80 + 5*3 + 3)}]
+ if {$req_w < $opt_w} {set req_w $opt_w}
+ if {$req_w > $scr_w} {set req_w $scr_w}
+ set opt_h [expr {$req_w*4/3}]
if {$req_h < $scr_h} {set req_h $scr_h}
+ if {$req_h > $opt_h} {set req_h $opt_h}
set g "${req_w}x${req_h}"
wm geometry $top $g
update
@@ -338,7 +345,7 @@ constructor new {i_commit i_path i_jump} {
set old_height [winfo height $w.file_pane]
$w.file_pane sash place 0 \
[lindex [$w.file_pane sash coord 0] 0] \
- [expr {int($old_height * 0.70)}]
+ [expr {int($old_height * 0.80)}]
bind $w.file_pane <Configure> \
"if {{$w.file_pane} eq {%W}} {[cb _resize %h]}"
--
1.6.0.20.g6148bc
^ 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