Git development
 help / color / mirror / Atom feed
* Re: [PATCH] clear error message for clone a gitweb URL
From: Liu Yubao @ 2006-11-09  1:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7iy5sma9.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Liu Yubao <yubao.liu@gmail.com> writes:
> 
>> When clone a gitweb URL, git reports "Can't lock ref", it's not clear for users,
>> this patch adds clear error message for this case.
>>
>> diff --git a/fetch.c b/fetch.c
>> index c426c04..40c5183 100644
>> --- a/fetch.c
>> +++ b/fetch.c
>> @@ -266,6 +266,14 @@ int pull(int targets, char **target, con
>>  		if (!write_ref || !write_ref[i])
>>  			continue;
>>  
>> +		if (*write_ref[i] == '\0') {
>> +			if (strncmp(write_ref_log_details, "http", 4) == 0)
>> +				error("Can't feed empty ref, seems you are fetching from a gitweb URL, "
>> +				      "check it in web browser for git URL.");
>> +			else
>> +				error("Can't feed empty ref");
>> +			goto unlock_and_fail;
> 
> You might have got that error by feeding an URL for gitweb, but
> I do not think the code, even with your additions, knows enough
> to tell that the user's mistake isn't other kinds of errors.
> 
> I am afraid that it would cause the user to waste time going
> wild goose chase if you say "seems you are...".  The phrasing
> makes it sound as if the tool _knows_ with some certainty that
> it is more plausible cause of the error than other kinds, while
> it certainly doesn't.
> 

I agree with you, it's not a fault of fetch.c.

> I think the reason it does not notice the breakage much earlier
> is that git-clone does not notice that gitweb URL gives nonsense
> to requests to "http://host/gitweb.cgi/$project/info/refs", so
> your patch to git-clone.sh is probably touching the right place,
> but I still feel the wording is a bit too strong and definitive
> than it should be.
> 
> Perhaps...
> 
> diff --git a/git-clone.sh b/git-clone.sh
> index 3f006d1..7ae69d9 100755
> --- a/git-clone.sh
> +++ b/git-clone.sh
> @@ -46,15 +46,18 @@ Perhaps git-update-server-info needs to
>  	do
>  		name=`expr "z$refname" : 'zrefs/\(.*\)'` &&
>  		case "$name" in
> -		*^*)	continue;;
> -		esac
> +		*^*)	continue ;;
> +		'')	false ;;
> +		esac &&
>  		if test -n "$use_separate_remote" &&
>  		   branch_name=`expr "z$name" : 'zheads/\(.*\)'`
>  		then
>  			tname="remotes/$origin/$branch_name"
>  		else
>  			tname=$name
> -		fi
> +		fi || {
> +			die "info/refs has nonsense $sha1 $refname, are you pulling from the right repository URL?"
> +		}
>  		git-http-fetch -v -a -w "$tname" "$name" "$1/" || exit 1
>  	done <"$clone_tmp/refs"
>  	rm -fr "$clone_tmp"
> 
It works well. Maybe it's better to say "info/refs has nonsense sha1($sha1) 

^ permalink raw reply

* Re: [PATCH] gitweb: protect blob and diff output lines from controls.
From: Junio C Hamano @ 2006-11-09  1:10 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200611090146.25306.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>> 
>>> 1. First, esc_path should _not_ use subroutine which does it's own 
>>> contol characters escaping. That was also a mistake I made in my patch.
>>> Perhaps we should have some quot_html or to_html subroutine which does 
>>> _only_ to_utf8 (decode from Encode module), escapeHTML and optionally 
>>> s/ /&nbsp;/g conversion.
>> 
>> I hated that original arrangement, 
>
> What did you hate, again?

esc_path calling esc_html you mentioned, of course.


>> obviously wrong in the output with the patch you are responding
>> to.  Except that git_blame2 is missing a chomp() on "my $data"
>> after finishing the metainfo loop, that is.
>
> The original (mine) code for esc_path uses esc_html, which did it's
> own partial (very partial) special characters esaping, namely
> \014 (\f) => ^L, \033 (\e) => ^[. So if pathname had form feed character,
> it would be replaced by ^L, not '\f'.

I know -- that is what I meant by "code reuse and consistency".

> You have added quot_cec to esc_html subroutine directly. I don't know
> what is your version of esc_html after the changes you
> made,...

See "pu".

> Well, the pathname has the limit that it must be in single line
> after quoting. The "blob" output is multipage.

I honestly have _no_ idea what distincition you are seeing
here.  Both blob and diff output are processed one line at a
time and its result would be on a single line too.

^ permalink raw reply

* Re: [PATCH] gitweb: protect blob and diff output lines from controls.
From: Jakub Narebski @ 2006-11-09  0:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1wodqxux.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> 1. First, esc_path should _not_ use subroutine which does it's own 
>> contol characters escaping. That was also a mistake I made in my patch.
>> Perhaps we should have some quot_html or to_html subroutine which does 
>> _only_ to_utf8 (decode from Encode module), escapeHTML and optionally 
>> s/ /&nbsp;/g conversion.
> 
> I hated that original arrangement, 

What did you hate, again?

>                                    but I do not see anything 
> obviously wrong in the output with the patch you are responding
> to.  Except that git_blame2 is missing a chomp() on "my $data"
> after finishing the metainfo loop, that is.

The original (mine) code for esc_path uses esc_html, which did it's
own partial (very partial) special characters esaping, namely
\014 (\f) => ^L, \033 (\e) => ^[. So if pathname had form feed character,
it would be replaced by ^L, not '\f'.

You have added quot_cec to esc_html subroutine directly. I don't know
what is your version of esc_html after the changes you made, but
this makes escaping part (quot) in esc_path never invoked. 

>> 2. In my opinion CS is better than CEC for quoting/escaping control 
>> characters in the "bulk" output, namely "blob" output and "text 
>> diff" (patchset body) output. CEC is better for pathnames (which must 
>> fit in one line), and perhaps other one-liners; perhaps not.
> 
> I am more for code reuse and consistency.  If "^L" is more
> readable then we should consistently use it for both contents
> and pathnames.  

Well, the pathname has the limit that it must be in single line
after quoting. The "blob" output is multipage. IMHO CEC like \n, \f,
\t are better in pathnames because this is what ls uses, while CS
for "blob" output is better because editors (including one true
editor being GNU Emacs ;-) uses CS like ^L (there is no end-of-line
as we split on LF and chomp; there is no tab character because line
is untabified first). But that is my opinion.

I think that conrol characters in filenames (in esc_path) should
be encompassed with <span class="cntrl">...</span> and styled.
I'm not sure if in "blob" view they should be styled. For sure
there should be no <span>...</span> for escaped attributed (future
esc_attr). Common to_html/quot_html would give us code reuse (as gives
quot_cec), if not consistent.

>                One of my tests were a symlink that points at a 
> funny filename ;-).

This should be IMHO solved rather by better "tree" view support
for symlinks, 'symlink' -> 'target' like in ls -l output.

>> BTW. what had happened with to_qtext post?
> 
> Sorry, I don't recall.

There was quite a bit of discussion about name of _suggested_
filename in blob_plain, blobdiff_plain view, namely the 
  -content_disposition => 'inline; filename="' ...
HTTP header. The result (probably lost in the noise) was to
add to_qtext subroutine for that.

Time to go to sleep...
-- 
Jakub Narebski

^ permalink raw reply

* git-svn can lose changes silently
From: Steven Grimm @ 2006-11-09  0:34 UTC (permalink / raw)
  To: git

git-svn is happy to overwrite changes in the svn repository with no 
warnings. Didn't seem to be known behavior when I mentioned it in #git, 
so here's an example, starting completely from scratch to make it easier 
to reproduce. I'm using git-svn 1.4.3 and svn 1.2.3 on OS X.

First I create a new svn repository and stick a test file there:

% mkdir /tmp/svntest
% cd /tmp/svntest
% svnadmin create svn-repo
% svn co file:///tmp/svntest/svn-repo /tmp/svntest/svn-client
Checked out revision 0.
% cd /tmp/svntest/svn-client
% mkdir -p project/trunk
% echo "initial contents" > project/trunk/testfile
% svn add project/trunk/testfile
svn: 'project/trunk' is not a working copy
% svn add project
A         project
A         project/trunk
A         project/trunk/testfile
% svn commit -m "initial commit"
Adding         project
Adding         project/trunk
Adding         project/trunk/testfile
Transmitting file data .
Committed revision 1.

Now I clone that svn repository using git-svn and pull in the contents 
of the svn repo:

% cd /tmp/svntest
% git-svn init file:///tmp/svntest/svn-repo/project/trunk git-repo 
% cd git-repo
% git-svn fetch
        A       project/trunk/testfile
Committing initial tree 9ca0a5a8cb5ee41744aaf17f859e945f2ebaa7d4
r1 = 63c70a5e17ffb095a31e96b1a56612f1f8423202

Now I create a development branch and commit a change to the test file:

% git-checkout -b devel
% echo "a second line from the git side" >> testfile
% git-commit -a -m "git-side commit"

Now I go make a change on the svn side and commit it.

% cd /tmp/svntest/svn-client/project/trunk
% echo "a second line from the svn side" >> testfile
% svn commit -m "a second svn commit"
Sending        trunk/testfile
Transmitting file data .
Committed revision 2.

At this point the svn repository has the testfile with two lines: 
"initial contents" and "a second line from the svn side". Now, back on 
the git side, I commit my git-side change to svn (here's where the bug 
happens):

% cd /tmp/svntest/git-repo
% git-svn dcommit                     
diff-tree 679c0db253781216b9b72b51f2dfffec5711f1a3~1 
679c0db253781216b9b72b51f2dfffec5711f1a3
        M       testfile
Committed 3
        M       project/trunk/testfile
r2 = 7d7923588ffb41eb756959d71623581df9318603
        M       project/trunk/testfile
r3 = 25a5fefe01389260274bb2617bc36a2cce18f15d
No changes between current HEAD and refs/remotes/git-svn
Hard resetting to the latest refs/remotes/git-svn

Finally, I go back to the svn side and update from the repo:

% cd /tmp/svntest/svn-client
% svn up
U    project/trunk/testfile
Updated to revision 3.
% cat project/trunk/testfile
initial contents
a second line from the git side

The change I checked in from the svn side has vanished without a trace, 
no warning messages or anything.

It is probably not a feature that you can lose changes without knowing 
about it! Even if I'd run git-svn fetch before that commit, it still 
wouldn't help if the svn version of the file changed between the time I 
ran fetch and the time I ran dcommit, totally possible with a busy svn 
repository.

Opinions? Suggestions on fixing it? Do other people agree this is a bug?


^ permalink raw reply

* Re: [PATCH] gitweb: protect blob and diff output lines from controls.
From: Junio C Hamano @ 2006-11-09  0:15 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200611090104.32247.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> 1. First, esc_path should _not_ use subroutine which does it's own 
> contol characters escaping. That was also a mistake I made in my patch.
> Perhaps we should have some quot_html or to_html subroutine which does 
> _only_ to_utf8 (decode from Encode module), escapeHTML and optionally 
> s/ /&nbsp;/g conversion.

I hated that original arrangement, but I do not see anything
obviously wrong in the output with the patch you are responding
to.  Except that git_blame2 is missing a chomp() on "my $data"
after finishing the metainfo loop, that is.

> 2. In my opinion CS is better than CEC for quoting/escaping control 
> characters in the "bulk" output, namely "blob" output and "text 
> diff" (patchset body) output. CEC is better for pathnames (which must 
> fit in one line), and perhaps other one-liners; perhaps not.

I am more for code reuse and consistency.  If "^L" is more
readable then we should consistently use it for both contents
and pathnames.  One of my tests were a symlink that points at a
funny filename ;-).

> BTW. what had happened with to_qtext post?

Sorry, I don't recall.

^ permalink raw reply

* Re: [PATCH/RFC] gitweb: Default to $hash_base or HEAD for $hash in "commit" and "commitdiff" views
From: Junio C Hamano @ 2006-11-09  0:09 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200611082335.28296.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

>> Probably the latter; the user might be hand-crafting a URL
>> (maybe learned a commit object name from mailing list and
>> cutting and pasting) and mispasted the long hexadecimal string.
>> Silently giving HEAD may leave the user confused than "oops, we
>> do not see that commit object".
>  
> No, if there is 'h' (hash) parameter provided, then gitweb tries
> to use this. HEAD is used _only_ if nether hash, nor hash_base
> are provided, i.e. for URL like below
>
>   URL?p=project.git;a=commit
>
> i.e. without neither 'h' nor 'hb'.

Ah, that one I agree it makes sense to use HEAD.  Also I am all
for giving a more readable error message.

^ permalink raw reply

* Re: [PATCH] gitweb: protect blob and diff output lines from controls.
From: Jakub Narebski @ 2006-11-09  0:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpsbxqzre.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> This reuses the quot_cec to protect blob and text diff output
> from leaking control characters.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---
>  * requesting extra sets of eyeballs.

This changes the "blob" and "text diff" output somewhat, as earlier it 
used Control key Sequence (CS) representation for some non-whitespace 
control characters (not "\t' not '\n'), namely replacing form feed (FF) 
('\f', '\014') with ^L and escape (ESC) ('\e', '\033') with ^[.

And (what is not said in the commit message) it additionally esc_html 
some title elements (the subroutine should be I think named esc_attr).

The problems are:
1. First, esc_path should _not_ use subroutine which does it's own 
contol characters escaping. That was also a mistake I made in my patch.
Perhaps we should have some quot_html or to_html subroutine which does 
_only_ to_utf8 (decode from Encode module), escapeHTML and optionally 
s/ /&nbsp;/g conversion.

2. In my opinion CS is better than CEC for quoting/escaping control 
characters in the "bulk" output, namely "blob" output and "text 
diff" (patchset body) output. CEC is better for pathnames (which must 
fit in one line), and perhaps other one-liners; perhaps not. I'm not 
sure what quoting to choose for esc_attr, but there we could use even 
--no-control-chars quoting (replacing any control character by '?'); 
but perhaps in some cases like git_print_page_path subroutine CEC is 
better.

BTW. what had happened with to_qtext post?
-- 
Jakub Narebski

^ permalink raw reply

* Re: What's in git.git
From: Junio C Hamano @ 2006-11-09  0:02 UTC (permalink / raw)
  To: git
In-Reply-To: <7v8ximwrm3.fsf@assigned-by-dhcp.cox.net>

Sorry, I made a mistake of pushing out git-pickaxe before making
it take over git-blame.  So I will fix it up by renaming it to
git-blame.

That is, unless there are too many people whose fingers have
already been trained to type "git-pickaxe" from 'next'
experience (you can obviously count me as one of these people).
In which case I will keep git-pickaxe as a backward compatible
synonym just like we still have git-annotate.

It might also make sense to eventually remove the other synonym
git-annotate but that would be a longer term issue and should be
handled separately.

^ permalink raw reply

* [PATCH] gitweb: protect blob and diff output lines from controls.
From: Junio C Hamano @ 2006-11-08 23:34 UTC (permalink / raw)
  To: Jakub Narebski, Petr Baudis; +Cc: git

This reuses the quot_cec to protect blob and text diff output
from leaking control characters.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * requesting extra sets of eyeballs.

 gitweb/gitweb.perl |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f46d678..b5b1011 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -597,11 +597,10 @@ sub esc_html ($;%) {
 
 	$str = to_utf8($str);
 	$str = escapeHTML($str);
-	$str =~ s/\014/^L/g; # escape FORM FEED (FF) character (e.g. in COPYING file)
-	$str =~ s/\033/^[/g; # "escape" ESCAPE (\e) character (e.g. commit 20a3847d8a5032ce41f90dcc68abfb36e6fee9b1)
 	if ($opts{'-nbsp'}) {
 		$str =~ s/ /&nbsp;/g;
 	}
+	$str =~ s|([[:cntrl:]])|(($1 ne "\t") ? quot_cec($1) : $1)|eg;
 	return $str;
 }
 
@@ -1900,17 +1899,17 @@ sub git_print_page_path {
 			$fullname .= ($fullname ? '/' : '') . $dir;
 			print $cgi->a({-href => href(action=>"tree", file_name=>$fullname,
 			                             hash_base=>$hb),
-			              -title => $fullname}, esc_path($dir));
+			              -title => esc_html($fullname)}, esc_path($dir));
 			print " / ";
 		}
 		if (defined $type && $type eq 'blob') {
 			print $cgi->a({-href => href(action=>"blob_plain", file_name=>$file_name,
 			                             hash_base=>$hb),
-			              -title => $name}, esc_path($basename));
+			              -title => esc_html($name)}, esc_path($basename));
 		} elsif (defined $type && $type eq 'tree') {
 			print $cgi->a({-href => href(action=>"tree", file_name=>$file_name,
 			                             hash_base=>$hb),
-			              -title => $name}, esc_path($basename));
+			              -title => esc_html($name)}, esc_path($basename));
 			print " / ";
 		} else {
 			print esc_path($basename);
-- 
1.4.4.rc1.g659d


^ permalink raw reply related

* Re: [PATCH/RFC] gitweb: Default to $hash_base or HEAD for $hash in "commit" and "commitdiff" views
From: Jakub Narebski @ 2006-11-08 22:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvelpr34d.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Set $hash parameter to $hash_base || "HEAD" if it is not set (if it is
>> not true to be more exact). This allows [hand-edited] URLs with 'action'
>> "commit" or "commitdiff" but without 'hash' parameter.
>>
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>> ---
>> RFC because I want to ask if we should default to HEAD if hash is not
>> provided for commit and commitdiff views, or should we error out with
>> more reasonable error message.
> 
> Probably the latter; the user might be hand-crafting a URL
> (maybe learned a commit object name from mailing list and
> cutting and pasting) and mispasted the long hexadecimal string.
> Silently giving HEAD may leave the user confused than "oops, we
> do not see that commit object".
 
No, if there is 'h' (hash) parameter provided, then gitweb tries
to use this. HEAD is used _only_ if nether hash, nor hash_base
are provided, i.e. for URL like below

  URL?p=project.git;a=commit

i.e. without neither 'h' nor 'hb'.


But if it is (it being defaulting to HEAD, _like git_) wrong solution,
still it would be better to show 'commit not provided' error instead of
current 'bad commit' error.
-- 
Jakub Narebski

^ permalink raw reply

* Re: win2k/cygwin cannot handle even moderately sized packs
From: Shawn Pearce @ 2006-11-08 22:28 UTC (permalink / raw)
  To: Alex Riesen; +Cc: Jakub Narebski, Junio C Hamano, git
In-Reply-To: <20061108213314.GA4437@steel.home>

Alex Riesen <fork0@t-online.de> wrote:
> Shawn Pearce, Wed, Nov 08, 2006 18:11:31 +0100:
> > The garbage creation is to account for the 2-4 windows required
> > by most applications.  Most of the time each window is unused;
> > we really only have two windows in use during delta decompression,
> > at all other times we really only have 1 window in use.  The commit
> > parsing applications don't keep the commit window in use when they
> > go access a tree or a blob.
> 
> So they actually can call unuse_pack to unmap the window,
> but it's kept for caching reasons?

Actually very few parts of the code even know about the windows.
Really the only parts that know it are the ones that directly
access the pack file, which is mostly restricted to sha1_file.c.

So since all access is through the more public interfaces what
you find is that the application code never keeps the window.
We are always doing use_pack/unuse_pack on every object access.
So the window is almost never in use.  So if we didn't hang onto
it in an LRU we would be in a world of hurt performance wise.

> > I could be wrong.  It may not matter.  But I think its crazy to
> > unmap otherwise valid mappings just because 2 bytes are on the
> > wrong side of an arbitrary boundary.
> 
> You're right, would be unfortunate to remap too often.
> 
> use_pack always maps at least 20 bytes, if I understand in_window and
> its use correctly. Actually, now I'm staring at it longer, I think the
> interface I suggested does almost the same, just allows to configure
> (well, hint at) the amount of bytes to be mapped in.

True; but if you look nobody wants more than 20 bytes.  They either
want <20 for the object header or 20 for the base object id in
a delta.  Otherwise they are shoving the data into zlib which
doesn't care.  No need to configure it, just shove it in.
 
> I still can't let go of the idea to get as much data as possible with
> just one call to sliding window code. Calling use_pack for every byte
> just does not seem right.

True.  But the only other idea I have is to copy the data into a
buffer for the caller.  Which we use only for the header section,
being that its small...  we already copy the delta base (20 bytes)
onto the stack during decompression.  Might as well copy the header
to decompress it.  Then you can batch up the range checks to at
worst no more than 2 range checks per header.

-- 

^ permalink raw reply

* Re: [PATCH/RFC] gitweb: Default to $hash_base or HEAD for $hash in "commit" and "commitdiff" views
From: Junio C Hamano @ 2006-11-08 22:21 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200611082311.35161.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> Set $hash parameter to $hash_base || "HEAD" if it is not set (if it is
> not true to be more exact). This allows [hand-edited] URLs with 'action'
> "commit" or "commitdiff" but without 'hash' parameter.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> RFC because I want to ask if we should default to HEAD if hash is not
> provided for commit and commitdiff views, or should we error out with
> more reasonable error message.

Probably the latter; the user might be hand-crafting a URL
(maybe learned a commit object name from mailing list and
cutting and pasting) and mispasted the long hexadecimal string.
Silently giving HEAD may leave the user confused than "oops, we
do not see that commit object".

^ permalink raw reply

* [PATCH/RFC] gitweb: Default to $hash_base or HEAD for $hash in "commit" and "commitdiff" views
From: Jakub Narebski @ 2006-11-08 22:11 UTC (permalink / raw)
  To: git

Set $hash parameter to $hash_base || "HEAD" if it is not set (if it is
not true to be more exact). This allows [hand-edited] URLs with 'action'
"commit" or "commitdiff" but without 'hash' parameter.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
RFC because I want to ask if we should default to HEAD if hash is not
provided for commit and commitdiff views, or should we error out with
more reasonable error message.

 gitweb/gitweb.perl |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3118cb0..8313517 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3391,6 +3391,7 @@ sub git_log {
 }
 
 sub git_commit {
+	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash);
 	if (!%co) {
 		die_error(undef, "Unknown commit object");
@@ -3668,6 +3669,7 @@ sub git_blobdiff_plain {
 
 sub git_commitdiff {
 	my $format = shift || 'html';
+	$hash ||= $hash_base || "HEAD";
 	my %co = parse_commit($hash);
 	if (!%co) {
 		die_error(undef, "Unknown commit object");
-- 
1.4.3.4

^ permalink raw reply related

* Re: [PATCH 2/2] gitweb: New improved formatting of chunk header in diff
From: Jakub Narebski @ 2006-11-08 21:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzmb1r4yu.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> O.K. This one introduced new feature, and wasn't needed for earlier 
>> accepted patch (output empty patches) to have sense. And it is rc1 
>> phase...
> 
> I personally do not mind minor gitweb 'feature' updates in -rc
> cycle, just like I am planning to see if there are gitk updates
> I haven't pulled from Paulus (I think there is at least one
> patch from last month or so), unless the changes do not break it
> so badly.

I'll resend corrected patch then...
 
>> By the way, where I can find proper specifiction of unified diff format? 
>> Do I understand correctly that bot from and to ranges can be without 
>> number of lines part if it simplifies to 0?
> 
> When Linus did apply.c and I did diff.c, we primarily worked off
> of sources to GNU patch.
> 
> There is a POSIX draft proposal now.
> 
> http://www.opengroup.org/austin/mailarchives/ag-review/msg02077.html
> 
> See also updates about the proposal.
> 
> http://thread.gmane.org/gmane.comp.version-control.git/29331/focus=29389

Thanks a lot.

Personally I think that we should push for git extended headers to be
(as an option) in POSIX for unifed diff... well, with the exception
of index and similarity lines. ;-)

-- 
Jakub Narebski

^ permalink raw reply

* Re: [PATCH 2/2] gitweb: New improved formatting of chunk header in diff
From: Junio C Hamano @ 2006-11-08 21:41 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200611082158.43652.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> O.K. This one introduced new feature, and wasn't needed for earlier 
> accepted patch (output empty patches) to have sense. And it is rc1 
> phase...

I personally do not mind minor gitweb 'feature' updates in -rc
cycle, just like I am planning to see if there are gitk updates
I haven't pulled from Paulus (I think there is at least one
patch from last month or so), unless the changes do not break it
so badly.

> By the way, where I can find proper specifiction of unified diff format? 
> Do I understand correctly that bot from and to ranges can be without 
> number of lines part if it simplifies to 0?

When Linus did apply.c and I did diff.c, we primarily worked off
of sources to GNU patch.

There is a POSIX draft proposal now.

http://www.opengroup.org/austin/mailarchives/ag-review/msg02077.html

See also updates about the proposal.

http://thread.gmane.org/gmane.comp.version-control.git/29331/focus=29389

^ permalink raw reply

* Re: win2k/cygwin cannot handle even moderately sized packs
From: Alex Riesen @ 2006-11-08 21:33 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: Jakub Narebski, Junio C Hamano, git
In-Reply-To: <20061108171131.GA13487@spearce.org>

Shawn Pearce, Wed, Nov 08, 2006 18:11:31 +0100:
> > >All true.  However what happens when the header spans two windows?
> > >Lets say I have the first 4 MiB mapped and the next 4 MiB mapped in
> > >a different window; these are not necessarily at the same locations
> > >within memory.  Now if an object header is split over these two
> > >then some bytes are at the end of the first window and the rest
> > >are at the start of the next window.
> > 
> > Assuming these are adjacent windows, we can just increment counters on the
> > all touched pages (at least the two together) and return the pointer into
> > the lowest page. Otherwise - time for garbage collection (why produce the
> > garbage at all, btw?) and remap.
> 
> They are adjacent in the pack file but not necessarily in virtual memory!

Oh, right! Don't know why I thought the mapped regions would be
connected.

> The garbage creation is to account for the 2-4 windows required
> by most applications.  Most of the time each window is unused;
> we really only have two windows in use during delta decompression,
> at all other times we really only have 1 window in use.  The commit
> parsing applications don't keep the commit window in use when they
> go access a tree or a blob.

So they actually can call unuse_pack to unmap the window,
but it's kept for caching reasons?

> Consequently we want the garbage there.  Actually I shouldn't have
> used garbage: the correct term would be LRU managed cache.  :-)
> When we need a new window and we would exceed our maximum limit
> (128 MiB in my implementation) we unmap the least recently used
> window which is not currently in use.

Yep, noticed that :) Just wondered why.

> I could be wrong.  It may not matter.  But I think its crazy to
> unmap otherwise valid mappings just because 2 bytes are on the
> wrong side of an arbitrary boundary.

You're right, would be unfortunate to remap too often.

use_pack always maps at least 20 bytes, if I understand in_window and
its use correctly. Actually, now I'm staring at it longer, I think the
interface I suggested does almost the same, just allows to configure
(well, hint at) the amount of bytes to be mapped in.

I still can't let go of the idea to get as much data as possible with
just one call to sliding window code. Calling use_pack for every byte
just does not seem right.

^ permalink raw reply

* Re: [PATCH 2/2] gitweb: New improved formatting of chunk header in diff
From: Jakub Narebski @ 2006-11-08 20:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfyctsmbm.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> This regresses on hunks of form "@@ -n +m,l @@" and friends, so
> I dropped it for now.

O.K. This one introduced new feature, and wasn't needed for earlier 
accepted patch (output empty patches) to have sense. And it is rc1 
phase...

By the way, where I can find proper specifiction of unified diff format? 
Do I understand correctly that bot from and to ranges can be without 
number of lines part if it simplifies to 0?
-- 
Jakub Narebski

^ permalink raw reply

* Re: [PATCH] clear error message for clone a gitweb URL
From: Junio C Hamano @ 2006-11-08 20:42 UTC (permalink / raw)
  To: Liu Yubao; +Cc: git
In-Reply-To: <4551864D.3010301@gmail.com>

Liu Yubao <yubao.liu@gmail.com> writes:

> When clone a gitweb URL, git reports "Can't lock ref", it's not clear for users,
> this patch adds clear error message for this case.
>
> diff --git a/fetch.c b/fetch.c
> index c426c04..40c5183 100644
> --- a/fetch.c
> +++ b/fetch.c
> @@ -266,6 +266,14 @@ int pull(int targets, char **target, con
>  		if (!write_ref || !write_ref[i])
>  			continue;
>  
> +		if (*write_ref[i] == '\0') {
> +			if (strncmp(write_ref_log_details, "http", 4) == 0)
> +				error("Can't feed empty ref, seems you are fetching from a gitweb URL, "
> +				      "check it in web browser for git URL.");
> +			else
> +				error("Can't feed empty ref");
> +			goto unlock_and_fail;

You might have got that error by feeding an URL for gitweb, but
I do not think the code, even with your additions, knows enough
to tell that the user's mistake isn't other kinds of errors.

I am afraid that it would cause the user to waste time going
wild goose chase if you say "seems you are...".  The phrasing
makes it sound as if the tool _knows_ with some certainty that
it is more plausible cause of the error than other kinds, while
it certainly doesn't.

I think the reason it does not notice the breakage much earlier
is that git-clone does not notice that gitweb URL gives nonsense
to requests to "http://host/gitweb.cgi/$project/info/refs", so
your patch to git-clone.sh is probably touching the right place,
but I still feel the wording is a bit too strong and definitive
than it should be.

Perhaps...

diff --git a/git-clone.sh b/git-clone.sh
index 3f006d1..7ae69d9 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -46,15 +46,18 @@ Perhaps git-update-server-info needs to
 	do
 		name=`expr "z$refname" : 'zrefs/\(.*\)'` &&
 		case "$name" in
-		*^*)	continue;;
-		esac
+		*^*)	continue ;;
+		'')	false ;;
+		esac &&
 		if test -n "$use_separate_remote" &&
 		   branch_name=`expr "z$name" : 'zheads/\(.*\)'`
 		then
 			tname="remotes/$origin/$branch_name"
 		else
 			tname=$name
-		fi
+		fi || {
+			die "info/refs has nonsense $sha1 $refname, are you pulling from the right repository URL?"
+		}
 		git-http-fetch -v -a -w "$tname" "$name" "$1/" || exit 1
 	done <"$clone_tmp/refs"
 	rm -fr "$clone_tmp"



^ permalink raw reply related

* Re: [PATCH 2/2] gitweb: New improved formatting of chunk header in diff
From: Junio C Hamano @ 2006-11-08 20:41 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200611081800.16001.jnareb@gmail.com>

This regresses on hunks of form "@@ -n +m,l @@" and friends, so
I dropped it for now.

I've applied other three patches but with manual tweaks while I
demangled the linewrap, so please sanity check the result after
it mirrors out.  I also noticed that we unconditionally give
blame link in "diff-tree --name-status" part of commitdiff, so
I've fixed it up as well.

I think you still leak control characters in commit messages and
htmlized blob text view; and perhaps others like blame.  I have
made quot_cec into a separate sub that is callable from outside
esc_path, so these places should be able to reuse it to make
control characters "visible and safe" and keep the presentation
consistent.  The non-path callers would not feed tabs into the
function (I haven't looked at the code but I suspect you would
replace them into runs of spaces to the next tab stop by hand),
but other controls need to be quoted.



^ permalink raw reply

* Re: win2k/cygwin cannot handle even moderately sized packs
From: Christopher Faylor @ 2006-11-08 19:22 UTC (permalink / raw)
  To: Alex Riesen, git
In-Reply-To: <81b0412b0611070555u1833cc8ci1d37d45782562df8@mail.gmail.com>

On Tue, Nov 07, 2006 at 02:55:01PM +0100, Alex Riesen wrote:
>>So the problem is probably memory fragmentation.
>
>probably.
>
>>You might have more joy if you allocated one HUGE chunk immediately on
>>startup to use for the pack, and then kept re-using that chunk.
>
>Well, it is not _one_ chunk. The windows/cygwin abomin...combination

I would like to ask you, once again, to exercise some adult self-control
when you feel compelled to answer questions about Cygwin.  If you need
to vent your frustration in some direction, I'd suggest getting a dog.
They can look very contrite when you yell at them even if they didn't
actually do anything wrong.

>may take an issue with this: it seem to copy complete address space
>at fork, which even for such a small packs I have here takes system
>down lightly (yes, I tried it).

Yes, Cygwin copies the heap on fork since Windows doesn't implement fork.

FWIW, Cygwin's malloc is based on Doug Lea's malloc.  It reverts to
using mmap when allocating memory above a certain threshhold.

cgf
--
Christopher Faylor			spammer? ->	aaaspam@sourceware.org
Cygwin Co-Project Leader				aaaspam@duffek.com

^ permalink raw reply

* Re: [PATCH 1/2] gitweb: New improved patchset view
From: Jakub Narebski @ 2006-11-08 19:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vejsdu5p5.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
> 
> > Jakub Narebski <jnareb@gmail.com> writes:
> >
> >> ---
> >>  gitweb/gitweb.css  |   66 +++++++++++++++----
> >>  gitweb/gitweb.perl |  183 
> >> ++++++++++++++++++++++++++++++++++------------------
> >>  2 files changed, 173 insertions(+), 76 deletions(-)
> >
> > Linewrap?
> >
> >> 
----------------------------------------------------------------------
> >>  ## functions returning short HTML fragments, or transforming HTML 
> >> fragments
> >
> > ...
> 
> No need to resend -- will manage.

Too late... ;-)

-- 
Jakub Narebski

^ permalink raw reply

* Re: [PATCH 1/2] gitweb: New improved patchset view
From: Junio C Hamano @ 2006-11-08 18:57 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <7virhpu73w.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> writes:

> Jakub Narebski <jnareb@gmail.com> writes:
>
>> ---
>>  gitweb/gitweb.css  |   66 +++++++++++++++----
>>  gitweb/gitweb.perl |  183 
>> ++++++++++++++++++++++++++++++++++------------------
>>  2 files changed, 173 insertions(+), 76 deletions(-)
>
> Linewrap?
>
>> ----------------------------------------------------------------------
>>  ## functions returning short HTML fragments, or transforming HTML 
>> fragments
>
> ...

No need to resend -- will manage.

^ permalink raw reply

* Re: [PATCH 2/2] gitweb: Use character or octal escape codes (and add span.cntrl) in esc_path
From: Jakub Narebski @ 2006-11-08 18:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmz71u787.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Instead of simply hiding control characters in esc_path by replacing
>> them with '?', use Character Escape Codes (CEC) i.e. alphabetic
>> backslash sequences like those found in C programming language and
>> many other languages influenced by it, such as Java and Perl.  If
>> control characted doesn't have corresponding character escape code,
>> use octal char sequence to escape it.
>>
>> Additionally use 'span' element with 'cntrl' attribute to mark escaped
>> control characters. Add style for span.cntrl in the CSS.
> 
> Would have preferred the 'span' thing to be part of [1/2]...

I've misunderstood you then. I'm sorry.

> Didn't I ask quot to be freestanding sub, not nested?  That
> would make UPR vs CEC experiments easier and more pleasant.

In what way having freestanding and not nested sub would "make
UPR vs CEC experiments easier and more pleasant"?

If it is important, I can send patch moving quot sub outside
esc_path (and perhaps also unq sub outside unquote), or send
corrected patch... or you can correct patch by hand...

-- 
Jakub Narebski

^ permalink raw reply

* [PATCH 1/2 (amend)] gitweb: New improved patchset view
From: Jakub Narebski @ 2006-11-08 18:41 UTC (permalink / raw)
  To: git
In-Reply-To: <200611081759.41498.jnareb@gmail.com>

Replace "gitweb diff header" with its full sha1 of blobs and replace
it by "git diff" header and extended diff header. Change also somewhat
highlighting of diffs.

Added `file_type_long' subroutine to convert file mode in octal to
file type description (only for file modes which used by git).

Changes:
* "gitweb diff header" which looked for example like below:
    file:_<sha1 before>_ -> file:_<sha1 after>_
  where 'file' is file type and '<sha1>' is full sha1 of blob is
  changed to
    diff --git _a/<file before>_ _b/<file after>_
  In both cases links are visible and use default link style. If file
  is added, a/<file> is not hyperlinked. If file is deleted, b/<file>
  is not hyperlinked.
* there is added "extended diff header", with <path> and <hash>
  hyperlinked (and <hash> shortened to 7 characters), and <mode>
  explained: '<mode>' is extended to '<mode> (<file type description>)',
  where added text is slightly lighter to easy distinguish that it
  was added (and it is difference from git-diff output).
* from-file/to-file two-line header lines have slightly darker color
  than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
  boundary, and top margin of 2px, both barely visible.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I'm sorry for previous version with linewrap.

 gitweb/gitweb.css  |   66 +++++++++++++++----
 gitweb/gitweb.perl |  183 ++++++++++++++++++++++++++++++++++------------------
 2 files changed, 173 insertions(+), 76 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index e19e6bc..974b47f 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -298,40 +298,82 @@ td.mode {
 	font-family: monospace;
 }
 
-div.diff a.list {
+/* styling of diffs (patchsets): commitdiff and blobdiff views */
+div.diff.header,
+div.diff.extended_header {
+	white-space: normal;
+}
+
+div.diff.header {
+	font-weight: bold;
+
+	background-color: #edece6;
+
+	margin-top: 4px;
+	padding: 4px 0px 2px 0px;
+	border: solid #d9d8d1;
+	border-width: 1px 0px 1px 0px;
+}
+
+div.diff.header a.path {
+	text-decoration: underline;
+}
+
+div.diff.extended_header,
+div.diff.extended_header a.path,
+div.diff.extended_header a.hash {
+	color: #777777;
+}
+
+div.diff.extended_header .info {
+	color: #b0b0b0;
+}
+
+div.diff.extended_header {
+	background-color: #f6f5ee;
+	padding: 2px 0px 2px 0px;
+}
+
+div.diff a.path,
+div.diff a.hash {
 	text-decoration: none;
 }
 
-div.diff a.list:hover {
+div.diff a.path:hover,
+div.diff a.hash:hover {
 	text-decoration: underline;
 }
 
-div.diff.to_file a.list,
-div.diff.to_file,
+div.diff.to_file a.path,
+div.diff.to_file {
+	color: #007000;
+}
+
 div.diff.add {
 	color: #008800;
 }
 
-div.diff.from_file a.list,
-div.diff.from_file,
+div.diff.from_file a.path,
+div.diff.from_file {
+	color: #aa0000;
+}
+
 div.diff.rem {
 	color: #cc0000;
 }
 
 div.diff.chunk_header {
 	color: #990099;
+
+	border: dotted #ffe0ff;
+	border-width: 1px 0px 0px 0px;
+	margin-top: 2px;
 }
 
 div.diff.incomplete {
 	color: #cccccc;
 }
 
-div.diff_info {
-	font-family: monospace;
-	color: #000099;
-	background-color: #edece6;
-	font-style: italic;
-}
 
 div.index_include {
 	border: solid #d9d8d1;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c9b16b5..2cf8e60 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -780,6 +780,32 @@ sub file_type {
 	}
 }
 
+# convert file mode in octal to file type description string
+sub file_type_long {
+	my $mode = shift;
+
+	if ($mode !~ m/^[0-7]+$/) {
+		return $mode;
+	} else {
+		$mode = oct $mode;
+	}
+
+	if (S_ISDIR($mode & S_IFMT)) {
+		return "directory";
+	} elsif (S_ISLNK($mode)) {
+		return "symlink";
+	} elsif (S_ISREG($mode)) {
+		if ($mode & S_IXUSR) {
+			return "executable";
+		} else {
+			return "file";
+		};
+	} else {
+		return "unknown";
+	}
+}
+
+
 ## ----------------------------------------------------------------------
 ## functions returning short HTML fragments, or transforming HTML fragments
 ## which don't beling to other sections
@@ -2166,6 +2192,7 @@ sub git_patchset_body {
 	my $in_header = 0;
 	my $patch_found = 0;
 	my $diffinfo;
+	my (%from, %to);
 
 	print "<div class=\"patchset\">\n";
 
@@ -2176,6 +2203,10 @@ sub git_patchset_body {
 		if ($patch_line =~ m/^diff /) { # "git diff" header
 			# beginning of patch (in patchset)
 			if ($patch_found) {
+				# close extended header for previous empty patch
+				if ($in_header) {
+					print "</div>\n" # class="diff extended_header"
+				}
 				# close previous patch
 				print "</div>\n"; # class="patch"
 			} else {
@@ -2184,89 +2215,113 @@ sub git_patchset_body {
 			}
 			print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n";
 
+			# read and prepare patch information
 			if (ref($difftree->[$patch_idx]) eq "HASH") {
+				# pre-parsed (or generated by hand)
 				$diffinfo = $difftree->[$patch_idx];
 			} else {
 				$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
 			}
+			$from{'file'} = $diffinfo->{'from_file'} || $diffinfo->{'file'};
+			$to{'file'}   = $diffinfo->{'to_file'}   || $diffinfo->{'file'};
+			if ($diffinfo->{'status'} ne "A") { # not new (added) file
+				$from{'href'} = href(action=>"blob", hash_base=>$hash_parent,
+				                     hash=>$diffinfo->{'from_id'},
+				                     file_name=>$from{'file'});
+			}
+			if ($diffinfo->{'status'} ne "D") { # not deleted file
+				$to{'href'} = href(action=>"blob", hash_base=>$hash,
+				                   hash=>$diffinfo->{'to_id'},
+				                   file_name=>$to{'file'});
+			}
 			$patch_idx++;
 
-			if ($diffinfo->{'status'} eq "A") { # added
-				print "<div class=\"diff_info\">" . file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'to_id'}) . " (new)" .
-				      "</div>\n"; # class="diff_info"
-
-			} elsif ($diffinfo->{'status'} eq "D") { # deleted
-				print "<div class=\"diff_info\">" . file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'from_id'}) . " (deleted)" .
-				      "</div>\n"; # class="diff_info"
-
-			} elsif ($diffinfo->{'status'} eq "R" || # renamed
-			         $diffinfo->{'status'} eq "C" || # copied
-			         $diffinfo->{'status'} eq "2") { # with two filenames (from git_blobdiff)
-				print "<div class=\"diff_info\">" .
-				      file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'from_file'})},
-				              $diffinfo->{'from_id'}) .
-				      " -> " .
-				      file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'to_file'})},
-				              $diffinfo->{'to_id'});
-				print "</div>\n"; # class="diff_info"
-
-			} else { # modified, mode changed, ...
-				print "<div class=\"diff_info\">" .
-				      file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'from_id'}) .
-				      " -> " .
-				      file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'to_id'});
-				print "</div>\n"; # class="diff_info"
+			# print "git diff" header
+			$patch_line =~ s!^(diff (.*?) )"?a/.*$!$1!;
+			if ($from{'href'}) {
+				$patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"},
+				                       'a/' . esc_path($from{'file'}));
+			} else { # file was added
+				$patch_line .= 'a/' . esc_path($from{'file'});
+			}
+			$patch_line .= ' ';
+			if ($to{'href'}) {
+				$patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"},
+				                       'b/' . esc_path($to{'file'}));
+			} else { # file was deleted
+				$patch_line .= 'b/' . esc_path($to{'file'});
 			}
 
-			#print "<div class=\"diff extended_header\">\n";
+			print "<div class=\"diff header\">$patch_line</div>\n";
+			print "<div class=\"diff extended_header\">\n";
 			$in_header = 1;
 			next LINE;
-		} # start of patch in patchset
+		}
 
+		if ($in_header) {
+			if ($patch_line !~ m/^---/) {
+				# match <path>
+				if ($patch_line =~ s!^((copy|rename) from ).*$!$1! && $from{'href'}) {
+					$patch_line .= $cgi->a({-href=>$from{'href'}, -class=>"path"},
+					                        esc_path($from{'file'}));
+				}
+				if ($patch_line =~ s!^((copy|rename) to ).*$!$1! && $to{'href'}) {
+					$patch_line = $cgi->a({-href=>$to{'href'}, -class=>"path"},
+					                      esc_path($to{'file'}));
+				}
+				# match <mode>
+				if ($patch_line =~ m/\s(\d{6})$/) {
+					$patch_line .= '<span class="info"> (' .
+					               file_type_long($1) .
+					               ')</span>';
+				}
+				# match <hash>
+				if ($patch_line =~ m/^index/) {
+					my ($from_link, $to_link);
+					if ($from{'href'}) {
+						$from_link = $cgi->a({-href=>$from{'href'}, -class=>"hash"},
+						                     substr($diffinfo->{'from_id'},0,7));
+					} else {
+						$from_link = '0' x 7;
+					}
+					if ($to{'href'}) {
+						$to_link = $cgi->a({-href=>$to{'href'}, -class=>"hash"},
+						                   substr($diffinfo->{'to_id'},0,7));
+					} else {
+						$to_link = '0' x 7;
+					}
+					my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
+					$patch_line =~ s!$from_id\.\.$to_id!$from_link..$to_link!;
+				}
+				print $patch_line . "<br/>\n";
 
-		if ($in_header && $patch_line =~ m/^---/) {
-			#print "</div>\n"; # class="diff extended_header"
-			$in_header = 0;
+			} else {
+				#$in_header && $patch_line =~ m/^---/;
+				print "</div>\n"; # class="diff extended_header"
+				$in_header = 0;
+
+				if ($from{'href'}) {
+					$patch_line = '--- a/' .
+					              $cgi->a({-href=>$from{'href'}, -class=>"path"},
+					                      esc_path($from{'file'}));
+				}
+				print "<div class=\"diff from_file\">$patch_line</div>\n";
 
-			my $file = $diffinfo->{'from_file'};
-			$file  ||= $diffinfo->{'file'};
-			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-			                               hash=>$diffinfo->{'from_id'}, file_name=>$file),
-			                -class => "list"}, esc_path($file));
-			$patch_line =~ s|a/.*$|a/$file|g;
-			print "<div class=\"diff from_file\">$patch_line</div>\n";
+				$patch_line = <$fd>;
+				chomp $patch_line;
 
-			$patch_line = <$fd>;
-			chomp $patch_line;
+				#$patch_line =~ m/^+++/;
+				if ($to{'href'}) {
+					$patch_line = '+++ b/' .
+					              $cgi->a({-href=>$to{'href'}, -class=>"path"},
+					                      esc_path($to{'file'}));
+				}
+				print "<div class=\"diff to_file\">$patch_line</div>\n";
 
-			#$patch_line =~ m/^+++/;
-			$file    = $diffinfo->{'to_file'};
-			$file  ||= $diffinfo->{'file'};
-			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-			                               hash=>$diffinfo->{'to_id'}, file_name=>$file),
-			                -class => "list"}, esc_path($file));
-			$patch_line =~ s|b/.*|b/$file|g;
-			print "<div class=\"diff to_file\">$patch_line</div>\n";
+			}
 
 			next LINE;
 		}
-		next LINE if $in_header;
 
 		print format_diff_line($patch_line);
 	}
-- 
1.4.3.4

^ permalink raw reply related

* Re: [PATCH 1/2] gitweb: New improved patchset view
From: Junio C Hamano @ 2006-11-08 18:27 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200611081759.41498.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> ---
>  gitweb/gitweb.css  |   66 +++++++++++++++----
>  gitweb/gitweb.perl |  183 
> ++++++++++++++++++++++++++++++++++------------------
>  2 files changed, 173 insertions(+), 76 deletions(-)

Linewrap?

> ----------------------------------------------------------------------
>  ## functions returning short HTML fragments, or transforming HTML 
> fragments

...

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox