[PATCH] gitweb: Add "next" link to commitdiff view

[PATCH] gitweb: Add "next" link to commitdiff view

[Jakub Narebski]

Add a kind of "next" view in the bottom part of navigation bar for
"commitdiff" view.

For commitdiff between two commits:
  (from: _commit_)
For commitdiff for one single parent commit:
  (parent: _commit_)
For commitdiff for one merge commit
  (merge: _commit_ _commit_ ...)
For commitdiff for root (parentless) commit
  (initial)
where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
display, everything is perhaps unnecessary esc_html on display.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---

Junio (and others), is it what you wanted? The idea was to change
"commitdiff" view only in minimal way, and preserve similarity
to "commit" format. 

SHA1 ids are printed in monospace (fixed width) font. BTW. should
we put '...' after shortened SHA1? Should those '...' be included
in link?


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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c9e57f0..4241d5c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3280,6 +3280,51 @@ sub git_commitdiff {
 	if (!%co) {
 		die_error(undef, "Unknown commit object");
 	}
+
+	# we need to prepare $formats_nav before any parameter munging
+	my $formats_nav;
+	if ($format eq 'html') {
+		$formats_nav =
+			$cgi->a({-href => href(action=>"commitdiff_plain",
+			                       hash=>$hash, hash_parent=>$hash_parent)},
+			        "raw");
+
+		if (defined $hash_parent) {
+			# commitdiff with two commits given
+			my $hash_parent_short = $hash_parent;
+			if ($hash_parent =~ m/^[0-9a-fA-F]{40}$/) {
+				$hash_parent_short = substr($hash_parent, 0, 7);
+			}
+			$formats_nav .=
+				' (from: ' .
+				$cgi->a({-href => href(action=>"commitdiff",
+				                       hash=>$hash_parent)},
+				        esc_html($hash_parent_short)) .
+				')';
+		} elsif (!$co{'parent'}) {
+			# --root commitdiff
+			$formats_nav .= ' (initial)';
+		} elsif (scalar @{$co{'parents'}} == 1) {
+			# single parent commit
+			$formats_nav .=
+				' (parent: ' .
+				$cgi->a({-href => href(action=>"commitdiff",
+				                       hash=>$co{'parent'})},
+				        esc_html(substr($co{'parent'}, 0, 7))) .
+				')';
+		} else {
+			# merge commit
+			$formats_nav .=
+				' (merge: ' .
+				join(' ', map {
+					$cgi->a({-href => href(action=>"commitdiff",
+					                       hash=>$_)},
+					        esc_html(substr($_, 0, 7)));
+				} @{$co{'parents'}} ) .
+				')';
+		}
+	}
+
 	if (!defined $hash_parent) {
 		$hash_parent = $co{'parent'} || '--root';
 	}
@@ -3317,10 +3362,6 @@ sub git_commitdiff {
 	if ($format eq 'html') {
 		my $refs = git_get_references();
 		my $ref = format_ref_marker($refs, $co{'id'});
-		my $formats_nav =
-			$cgi->a({-href => href(action=>"commitdiff_plain",
-			                       hash=>$hash, hash_parent=>$hash_parent)},
-			        "raw");
 
 		git_header_html(undef, $expires);
 		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
-- 
1.4.2.1

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Jakub Narebski]

BTW there are some errors in displaying commitdiff of initial commit, 
take for example a=commitdiff;h=161332a521fe10c41979bcd493d95e2ac562b7f
for git.git repository in gitweb.

Needs fixing.
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Add a kind of "next" view in the bottom part of navigation bar for
> "commitdiff" view.
>
> For commitdiff between two commits:
>   (from: _commit_)
> For commitdiff for one single parent commit:
>   (parent: _commit_)
> For commitdiff for one merge commit
>   (merge: _commit_ _commit_ ...)
> For commitdiff for root (parentless) commit
>   (initial)
> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
> display, everything is perhaps unnecessary esc_html on display.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

Would it even be necessary to use any SHA-1 name in these cases,
I wonder.  Would it make the page less useful if we replace all
of the above _commit_ with a fixed string, say, "parent"?

I always hated gitweb diffs that prefix each filepair with their
full 40-byte SHA-1 blob object names.  It just adds noise to the
output without adding any meaningful information.

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Add a kind of "next" view in the bottom part of navigation bar for
>> "commitdiff" view.
>>
>> For commitdiff between two commits:
>>   (from: _commit_)
Perhaps we should use "(from: _commit_ to: _commit_)" here...

>> For commitdiff for one single parent commit:
>>   (parent: _commit_)
>> For commitdiff for one merge commit
>>   (merge: _commit_ _commit_ ...)
>> For commitdiff for root (parentless) commit
>>   (initial)
>> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
>> display, everything is perhaps unnecessary esc_html on display.
>>
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> 
> Would it even be necessary to use any SHA-1 name in these cases,
> I wonder.  Would it make the page less useful if we replace all
> of the above _commit_ with a fixed string, say, "parent"?

I decided on using _shortened_ SHA1 because I didn't like neither 
"(parent parent ...) " nor "(parent1 parent2 ...)" for merges. Perhaps 
I should have used 8-characters abbreviation, like in git_blame2.
And I was inspired by git-show output for merges:

 commit ff49fae6a547e5c70117970e01c53b64d983cd10
 Merge: 7ad4ee7... 75f9007... 14eab2b... 0b35995... eee4609...

> I always hated gitweb diffs that prefix each filepair with their
> full 40-byte SHA-1 blob object names.  It just adds noise to the
> output without adding any meaningful information.

I always thought about this only as a (somewhat sophisticated) separator 
marking where individual patch (patch for given files) begin. And
a place to click (non-hidden link) for blob before and after. Please 
remember that this gitweb diff header was from the times where we 
didn't have difftree in commitdiff view.

-- 
Jakub Narebski
Poland

[PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Jakub Narebski]

Use fixed string instead of shortened SHA1 identifier of commit
as a name for "mext" link in commitdiff view.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Add a kind of "next" link in the bottom part of navigation bar for
>> "commitdiff" view.
>>
>> For commitdiff between two commits:
>>   (from: _commit_)
>> For commitdiff for one single parent commit:
>>   (parent: _commit_)
>> For commitdiff for one merge commit
>>   (merge: _commit_ _commit_ ...)
>> For commitdiff for root (parentless) commit
>>   (initial)
>> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
>> display, everything is perhaps unnecessary esc_html on display.
> 
> Would it even be necessary to use any SHA-1 name in these cases,
> I wonder.  Would it make the page less useful if we replace all
> of the above _commit_ with a fixed string, say, "parent"?
> 
> I always hated gitweb diffs that prefix each filepair with their
> full 40-byte SHA-1 blob object names.  It just adds noise to the
> output without adding any meaningful information.

Here you have. I used patch on top of previous one instead of amending
previous patch because I think it is interesting to have this history
of this feature.

This patch doesn't add any functionality.


 gitweb/gitweb.perl |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4241d5c..255487d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3291,15 +3291,11 @@ sub git_commitdiff {
 
 		if (defined $hash_parent) {
 			# commitdiff with two commits given
-			my $hash_parent_short = $hash_parent;
-			if ($hash_parent =~ m/^[0-9a-fA-F]{40}$/) {
-				$hash_parent_short = substr($hash_parent, 0, 7);
-			}
 			$formats_nav .=
-				' (from: ' .
+				' (' .
 				$cgi->a({-href => href(action=>"commitdiff",
 				                       hash=>$hash_parent)},
-				        esc_html($hash_parent_short)) .
+				        'from') .
 				')';
 		} elsif (!$co{'parent'}) {
 			# --root commitdiff
@@ -3307,19 +3303,19 @@ sub git_commitdiff {
 		} elsif (scalar @{$co{'parents'}} == 1) {
 			# single parent commit
 			$formats_nav .=
-				' (parent: ' .
+				' (' .
 				$cgi->a({-href => href(action=>"commitdiff",
 				                       hash=>$co{'parent'})},
-				        esc_html(substr($co{'parent'}, 0, 7))) .
+				        'parent') .
 				')';
 		} else {
 			# merge commit
 			$formats_nav .=
-				' (merge: ' .
+				' (' .
 				join(' ', map {
 					$cgi->a({-href => href(action=>"commitdiff",
 					                       hash=>$_)},
-					        esc_html(substr($_, 0, 7)));
+					        'parent');
 				} @{$co{'parents'}} ) .
 				')';
 		}
-- 
1.4.2.1

Re: [PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Petr Baudis]

Dear diary, on Tue, Oct 24, 2006 at 12:08:08AM CEST, I got a letter
where Jakub Narebski <jnareb@gmail.com> said that...
> @@ -3307,19 +3303,19 @@ sub git_commitdiff {
>  		} elsif (scalar @{$co{'parents'}} == 1) {
>  			# single parent commit
>  			$formats_nav .=
> -				' (parent: ' .
> +				' (' .
>  				$cgi->a({-href => href(action=>"commitdiff",
>  				                       hash=>$co{'parent'})},
> -				        esc_html(substr($co{'parent'}, 0, 7))) .
> +				        'parent') .
>  				')';
>  		} else {
>  			# merge commit
>  			$formats_nav .=
> -				' (merge: ' .
> +				' (' .
>  				join(' ', map {
>  					$cgi->a({-href => href(action=>"commitdiff",
>  					                       hash=>$_)},
> -					        esc_html(substr($_, 0, 7)));
> +					        'parent');
>  				} @{$co{'parents'}} ) .
>  				')';
>  		}

For people not used to git, it would be more informative to keep the
'merge' text there.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

[PATCH 2/1 (amend)] gitweb: Use fixed string for "next" link in commitdiff view

[Jakub Narebski]

Use fixed string instead of shortened SHA1 identifier of commit
as a name for "mext" link in commitdiff view.

While at it make sure that we don't mistake option for a second
commit.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4241d5c..9c81214 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3289,17 +3289,14 @@ sub git_commitdiff {
 			                       hash=>$hash, hash_parent=>$hash_parent)},
 			        "raw");
 
-		if (defined $hash_parent) {
+		if (defined $hash_parent && $hash_parent !~ m/^-/) {
 			# commitdiff with two commits given
-			my $hash_parent_short = $hash_parent;
-			if ($hash_parent =~ m/^[0-9a-fA-F]{40}$/) {
-				$hash_parent_short = substr($hash_parent, 0, 7);
-			}
+			# second "commit" is not in fact diff option
 			$formats_nav .=
-				' (from: ' .
+				' (' .
 				$cgi->a({-href => href(action=>"commitdiff",
 				                       hash=>$hash_parent)},
-				        esc_html($hash_parent_short)) .
+				        'from') .
 				')';
 		} elsif (!$co{'parent'}) {
 			# --root commitdiff
@@ -3307,10 +3304,10 @@ sub git_commitdiff {
 		} elsif (scalar @{$co{'parents'}} == 1) {
 			# single parent commit
 			$formats_nav .=
-				' (parent: ' .
+				' (' .
 				$cgi->a({-href => href(action=>"commitdiff",
 				                       hash=>$co{'parent'})},
-				        esc_html(substr($co{'parent'}, 0, 7))) .
+				        'parent') .
 				')';
 		} else {
 			# merge commit
@@ -3319,7 +3316,7 @@ sub git_commitdiff {
 				join(' ', map {
 					$cgi->a({-href => href(action=>"commitdiff",
 					                       hash=>$_)},
-					        esc_html(substr($_, 0, 7)));
+					        'parent');
 				} @{$co{'parents'}} ) .
 				')';
 		}
-- 
1.4.2.1

Re: [PATCH 2/1 (amend)] gitweb: Use fixed string for "next" link in commitdiff view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Use fixed string instead of shortened SHA1 identifier of commit
> as a name for "mext" link in commitdiff view.
>
> While at it make sure that we don't mistake option for a second
> commit.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---

Q1. (amend) to fix what?
Q2. "mext"?

Re: [PATCH 2/1 (amend)] gitweb: Use fixed string for "next" link in commitdiff view

[Jakub Narebski]

Dnia wtorek 24. października 2006 11:17, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Use fixed string instead of shortened SHA1 identifier of commit
>> as a name for "mext" link in commitdiff view.
>>
>> While at it make sure that we don't mistake option for a second
>> commit.
>>
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>> ---
> 
> Q1. (amend) to fix what?

Petr "Pasky" Baudis wrote:
>>               } else {
>>                       # merge commit
>>                       $formats_nav .=
>> -                             ' (merge: ' .
>> +                             ' (' .
[...]
> For people not used to git, it would be more informative to keep the
> 'merge' text there.

It also fixes detection if it is diff of two specified commits. Although
$hash_parent is set to '--root' for rootloess commit later, we could
have it set by hand. This is also preparation for later using '--cc'
(although we wouldn't generate such URL, I don't think).

> Q2. "mext"?
Oops.

-- 
Jakub Narebski
Poland

Re: [PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Petr Baudis]

Dear diary, on Tue, Oct 24, 2006 at 12:08:08AM CEST, I got a letter
where Jakub Narebski <jnareb@gmail.com> said that...
> Use fixed string instead of shortened SHA1 identifier of commit
> as a name for "mext" link in commitdiff view.
> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

So I've read what the patch actually does this time, and I really
dislike it.

> Junio C Hamano wrote:
> > Jakub Narebski <jnareb@gmail.com> writes:
> > 
> >> Add a kind of "next" link in the bottom part of navigation bar for
> >> "commitdiff" view.
> >>
> >> For commitdiff between two commits:
> >>   (from: _commit_)
> >> For commitdiff for one single parent commit:
> >>   (parent: _commit_)
> >> For commitdiff for one merge commit
> >>   (merge: _commit_ _commit_ ...)
> >> For commitdiff for root (parentless) commit
> >>   (initial)
> >> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
> >> display, everything is perhaps unnecessary esc_html on display.
> > 
> > I always hated gitweb diffs that prefix each filepair with their
> > full 40-byte SHA-1 blob object names.  It just adds noise to the
> > output without adding any meaningful information.

I agree, using the shortened SHA1 would be definitely an improvement
here, but

> > Would it even be necessary to use any SHA-1 name in these cases,
> > I wonder.  Would it make the page less useful if we replace all
> > of the above _commit_ with a fixed string, say, "parent"?

I really disagree here - what's the point of not using SHA-1? The extra
string carries zero information in comparison with the previous state
and I just can't see how it *improves* stuff. If you're walking in a
maze and making marks on walls, it's still more useful if you have
corridors named by "A", "B", "C", "D" on junctions if you sometimes want
to walk back to the marked corridors.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Re: [PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Jakub Narebski]

Petr Baudis wrote:
> Dear diary, on Tue, Oct 24, 2006 at 12:08:08AM CEST, I got a letter
> where Jakub Narebski <jnareb@gmail.com> said that...
>> Use fixed string instead of shortened SHA1 identifier of commit
>> as a name for "mext" link in commitdiff view.
>> 
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> 
> So I've read what the patch actually does this time, and I really
> dislike it.
> 
>> Junio C Hamano wrote:
>>> Jakub Narebski <jnareb@gmail.com> writes:
[...]
>>>> For commitdiff for one merge commit
>>>>   (merge: _commit_ _commit_ ...)
[...]
>>>> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
>>>> display, everything is perhaps unnecessary esc_html on display.
>>> 
>>> I always hated gitweb diffs that prefix each filepair with their
>>> full 40-byte SHA-1 blob object names.  It just adds noise to the
>>> output without adding any meaningful information.
> 
> I agree, using the shortened SHA1 would be definitely an improvement
> here, but

I'm planning on revamping gitweb diff output. Any ideas?

>>> Would it even be necessary to use any SHA-1 name in these cases,
>>> I wonder.  Would it make the page less useful if we replace all
>>> of the above _commit_ with a fixed string, say, "parent"?
> 
> I really disagree here - what's the point of not using SHA-1? The extra
> string carries zero information in comparison with the previous state
> and I just can't see how it *improves* stuff. If you're walking in a
> maze and making marks on walls, it's still more useful if you have
> corridors named by "A", "B", "C", "D" on junctions if you sometimes want
> to walk back to the marked corridors.

That's why instead of resending the patch/amending the commit, I have
put changing shortened SHA1 to constant name in new patch.

By the way, because those links to parents commitdiffs are just ordinary
links, you can see when those links are visited.
-- 
Jakub Narebski
Poland

Re: [PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Petr Baudis]

Dear diary, on Tue, Oct 24, 2006 at 01:59:57PM CEST, I got a letter
where Jakub Narebski <jnareb@gmail.com> said that...
> >>> Would it even be necessary to use any SHA-1 name in these cases,
> >>> I wonder.  Would it make the page less useful if we replace all
> >>> of the above _commit_ with a fixed string, say, "parent"?
> > 
> > I really disagree here - what's the point of not using SHA-1? The extra
> > string carries zero information in comparison with the previous state
> > and I just can't see how it *improves* stuff. If you're walking in a
> > maze and making marks on walls, it's still more useful if you have
> > corridors named by "A", "B", "C", "D" on junctions if you sometimes want
> > to walk back to the marked corridors.
> 
> That's why instead of resending the patch/amending the commit, I have
> put changing shortened SHA1 to constant name in new patch.

Huh, I don't quite understand. You threw away all the "A", "B", "C", "D"
and kept only the marks.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Re: [PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

>> > Would it even be necessary to use any SHA-1 name in these cases,
>> > I wonder.  Would it make the page less useful if we replace all
>> > of the above _commit_ with a fixed string, say, "parent"?
>
> I really disagree here - what's the point of not using SHA-1? The extra
> string carries zero information in comparison with the previous state
> and I just can't see how it *improves* stuff. If you're walking in a
> maze and making marks on walls, it's still more useful if you have
> corridors named by "A", "B", "C", "D" on junctions if you sometimes want
> to walk back to the marked corridors.

I think people would recognize A B C D as names but not 40- or
8- hexadecimal letters.

I do not care much either way, actually, but I think it might
make more sense to use abbreviated object names.  On the other
hand it may be Ok to have full 40 letters depending on the
layout (e.g. the set of merge parents are shown on a single line
in which case it would not fit, etc.).

Re: [PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Petr Baudis]

On Tue, Oct 24, 2006 at 07:17:28PM CEST, Junio C Hamano wrote:
> Petr Baudis <pasky@suse.cz> writes:
> 
> >> > Would it even be necessary to use any SHA-1 name in these cases,
> >> > I wonder.  Would it make the page less useful if we replace all
> >> > of the above _commit_ with a fixed string, say, "parent"?
> >
> > I really disagree here - what's the point of not using SHA-1? The extra
> > string carries zero information in comparison with the previous state
> > and I just can't see how it *improves* stuff. If you're walking in a
> > maze and making marks on walls, it's still more useful if you have
> > corridors named by "A", "B", "C", "D" on junctions if you sometimes want
> > to walk back to the marked corridors.
> 
> I think people would recognize A B C D as names but not 40- or
> 8- hexadecimal letters.

40-digit hex numbers is insane, I agree. But at least I personally tend
to recognize 8-digit hex numbers when dancing around them intensively
for a few minutes. Besides, it can be just "now I took the 8c5 way",
which is much easier to train your neurons too than "now I took the
fourth, uh, or was it the fifth parent? one, two, three, four, fifth...
hmm, what's in the statusbar?".

My point is that this does not improve the situation, and some people
(me) think it makes it worse, so what's the point of the change?

> I do not care much either way, actually, but I think it might
> make more sense to use abbreviated object names.  On the other
> hand it may be Ok to have full 40 letters depending on the
> layout (e.g. the set of merge parents are shown on a single line
> in which case it would not fit, etc.).

Yes, I'm all for abbreviated names, but I'm against just writing
"parent" everywhere.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

Re: [PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> On Tue, Oct 24, 2006 at 07:17:28PM CEST, Junio C Hamano wrote:
>> Petr Baudis <pasky@suse.cz> writes:
>> 
>> >> > Would it even be necessary to use any SHA-1 name in these cases,
>> >> > I wonder.  Would it make the page less useful if we replace all
>> >> > of the above _commit_ with a fixed string, say, "parent"?
>> >
>> > I really disagree here - what's the point of not using SHA-1? The extra
>> > string carries zero information in comparison with the previous state
>> > and I just can't see how it *improves* stuff. If you're walking in a
>> > maze and making marks on walls, it's still more useful if you have
>> > corridors named by "A", "B", "C", "D" on junctions if you sometimes want
>> > to walk back to the marked corridors.
>> 
>> I think people would recognize A B C D as names but not 40- or
>> 8- hexadecimal letters.
>
> 40-digit hex numbers is insane, I agree. But at least I personally tend
> to recognize 8-digit hex numbers when dancing around them intensively
> for a few minutes. Besides, it can be just "now I took the 8c5 way",
> which is much easier to train your neurons too than "now I took the
> fourth, uh, or was it the fifth parent? one, two, three, four, fifth...
> hmm, what's in the statusbar?".
>
> My point is that this does not improve the situation, and some people
> (me) think it makes it worse, so what's the point of the change?
>
>> I do not care much either way, actually, but I think it might
>> make more sense to use abbreviated object names.  On the other
>> hand it may be Ok to have full 40 letters depending on the
>> layout (e.g. the set of merge parents are shown on a single line
>> in which case it would not fit, etc.).
>
> Yes, I'm all for abbreviated names, but I'm against just writing
> "parent" everywhere.

Fully agreed.  Please make it so, if you so are inclined,
perhaps between you and Jakub?

Re: [PATCH 2/1] gitweb: Use fixed string for "next" link in commitdiff view

[Jakub Narebski]

Junio C Hamano wrote:
> Petr Baudis <pasky@suse.cz> writes:

>> Yes, I'm all for abbreviated names, but I'm against just writing
>> "parent" everywhere.
> 
> Fully agreed.  Please make it so, if you so are inclined,
> perhaps between you and Jakub?

I'm all for abbreviated names (that was my first solution, wasn't it).
This patch was because of Junio mail:
> Would it even be necessary to use any SHA-1 name in these cases,
> I wonder.  Would it make the page less useful if we replace all
> of the above _commit_ with a fixed string, say, "parent"?

So please drop this patch.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Jakub Narebski]

Jakub Narebski wrote:
> Add a kind of "next" view in the bottom part of navigation bar for
> "commitdiff" view.
> 
> For commitdiff between two commits:
>   (from: _commit_)
> For commitdiff for one single parent commit:
>   (parent: _commit_)
> For commitdiff for one merge commit
>   (merge: _commit_ _commit_ ...)
> For commitdiff for root (parentless) commit
>   (initial)
> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
> display, everything is perhaps unnecessary esc_html on display.
> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> 
> Junio (and others), is it what you wanted? The idea was to change
> "commitdiff" view only in minimal way, and preserve similarity
> to "commit" format.

Any reasons not to accept this patch? I find it very useful.

The idea to use fixed string instead of shortened SHA-1 of commit
was abandoned after some discussion in this thread.
-- 
Jakub Narebski

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Jakub Narebski wrote:
>> Add a kind of "next" view in the bottom part of navigation bar for
>> "commitdiff" view.
>> 
>> For commitdiff between two commits:
>>   (from: _commit_)
>> For commitdiff for one single parent commit:
>>   (parent: _commit_)
>> For commitdiff for one merge commit
>>   (merge: _commit_ _commit_ ...)
>> For commitdiff for root (parentless) commit
>>   (initial)
>> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
>> display, everything is perhaps unnecessary esc_html on display.
>> 
>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>> ---
>> 
>> Junio (and others), is it what you wanted? The idea was to change
>> "commitdiff" view only in minimal way, and preserve similarity
>> to "commit" format.
>
> Any reasons not to accept this patch?

Nothing other than it was lost in the noise (I am partly
responsible for) that followed the patch.

	Message-ID: <200610230037.57183.jnareb@gmail.com>

Who acked this in the discussion?  I'd like to add "Acked-by:"
for them when I make the commit.

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Jakub Narebski wrote:
> > Add a kind of "next" view in the bottom part of navigation bar for
> > "commitdiff" view.
> > 
> > For commitdiff between two commits:
> >   (from: _commit_)
> > For commitdiff for one single parent commit:
> >   (parent: _commit_)
> > For commitdiff for one merge commit
> >   (merge: _commit_ _commit_ ...)
> > For commitdiff for root (parentless) commit
> >   (initial)
> > where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
> > display, everything is perhaps unnecessary esc_html on display.
> > 
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > 
> > Junio (and others), is it what you wanted? The idea was to change
> > "commitdiff" view only in minimal way, and preserve similarity
> > to "commit" format.
> 
> Any reasons not to accept this patch? I find it very useful.
> 
> The idea to use fixed string instead of shortened SHA-1 of commit
> was abandoned after some discussion in this thread.

I prefer using the commit-8 without any "..." postfixed.  Anyone who
knows squat about git knows very well what a commit-8 is when they
see one -- the first 8 hexadecimal digits of the full SHA-1.

I like using "next" only when there is a "prev" right next to it,
i.e. based on _context_, something like this:
   << prev next >>
where "<< prev" is hyperlinked, and "next >>" is also.

I looked at your patch (first email in this thread) and from
what I gathered that it does, I like it (haven't tried it yet).

What I like about it is the clear information it conveys:
it gives me both the commit-8 and what that commit-8 _is_.

I sometimes remember commit-8s and this helps me a lot when
I move the mouse pointer to an xterm and want to do
git-show <commit-8>, and it futhermore helps me _know_ the
relationship between that commit-8 and the rest of the
information in the window frame gitweb is showing, (parent, etc).

Acked-by: Luben Tuikov <ltuikov@yahoo.com>
(not that it matters in any way ;-) )

   Luben

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Jakub Narebski]

Luben Tuikov wrote:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
>> Jakub Narebski wrote:
>>> Add a kind of "next" view in the bottom part of navigation bar for
>>> "commitdiff" view.
>>> 
>>> For commitdiff between two commits:
>>>   (from: _commit_)
>>> For commitdiff for one single parent commit:
>>>   (parent: _commit_)
>>> For commitdiff for one merge commit
>>>   (merge: _commit_ _commit_ ...)
>>> For commitdiff for root (parentless) commit
>>>   (initial)
>>> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
>>> display, everything is perhaps unnecessary esc_html on display.
[...]
>> 
>> Any reasons not to accept this patch? I find it very useful.
>> 
>> The idea to use fixed string instead of shortened SHA-1 of commit
>> was abandoned after some discussion in this thread.
> 
> I prefer using the commit-8 without any "..." postfixed.  Anyone who
> knows squat about git knows very well what a commit-8 is when they
> see one -- the first 8 hexadecimal digits of the full SHA-1.
> 
> I like using "next" only when there is a "prev" right next to it,
> i.e. based on _context_, something like this:
>    << prev next>>
> where "<< prev" is hyperlinked, and "next>>" is also.

Unfortunately this is simply not possible in this case, as links in git
are only from commit to paren(a), in one direction only.

Besides as you can see we can have more than one "next>>" link: for merge
commit there are multiple parents. Well, for branch points there are
multiple commits which have given commit as (one of) its parent(s), so
there can be multiple "<<prev" links as well.

> I looked at your patch (first email in this thread) and from
> what I gathered that it does, I like it (haven't tried it yet).
> 
> What I like about it is the clear information it conveys:
> it gives me both the commit-8 and what that commit-8 _is_.

It is commit-7, but that can be easily changed.

> Acked-by: Luben Tuikov <ltuikov@yahoo.com>
> (not that it matters in any way ;-) )

IIRC Junio asked for ACKs.
-- 
Jakub Narebski

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Luben Tuikov]

--- Jakub Narebski <jnareb@gmail.com> wrote:
> Luben Tuikov wrote:
> > --- Jakub Narebski <jnareb@gmail.com> wrote:
> >> Jakub Narebski wrote:
> >>> Add a kind of "next" view in the bottom part of navigation bar for
> >>> "commitdiff" view.
> >>> 
> >>> For commitdiff between two commits:
> >>>   (from: _commit_)
> >>> For commitdiff for one single parent commit:
> >>>   (parent: _commit_)
> >>> For commitdiff for one merge commit
> >>>   (merge: _commit_ _commit_ ...)
> >>> For commitdiff for root (parentless) commit
> >>>   (initial)
> >>> where _link_ denotes hyperlink. SHA1 is shortened to 7 characters on
> >>> display, everything is perhaps unnecessary esc_html on display.
> [...]
> >> 
> >> Any reasons not to accept this patch? I find it very useful.
> >> 
> >> The idea to use fixed string instead of shortened SHA-1 of commit
> >> was abandoned after some discussion in this thread.
> > 
> > I prefer using the commit-8 without any "..." postfixed.  Anyone who
> > knows squat about git knows very well what a commit-8 is when they
> > see one -- the first 8 hexadecimal digits of the full SHA-1.
> > 
> > I like using "next" only when there is a "prev" right next to it,
> > i.e. based on _context_, something like this:
> >    << prev next>>
> > where "<< prev" is hyperlinked, and "next>>" is also.
> 
> Unfortunately this is simply not possible in this case, as links in git
> are only from commit to paren(a), in one direction only.

I was not suggesting that.  I simply saw a "next" suggestion in this
thread and wanted to mention about "next", "prev" and context.

> It is commit-7, but that can be easily changed.

Please do change it to "commit-8" -- it'd be consistent with the rest
of GIT.

> > Acked-by: Luben Tuikov <ltuikov@yahoo.com>
> > (not that it matters in any way ;-) )
> 
> IIRC Junio asked for ACKs.

In this email, it seems that he's asking for, quoting,
 "Acked-by:":
http://marc.theaimsgroup.com/?l=git&m=116206206008374&w=2

      Luben

Re: [PATCH] gitweb: Add "next" link to commitdiff view

[Jakub Narebski]

Luben Tuikov wrote:
> --- Jakub Narebski <jnareb@gmail.com> wrote:
>>
>> It is commit-7, but that can be easily changed.
> 
> Please do change it to "commit-8" -- it'd be consistent with the rest
> of GIT.

Well, some parts of GIT use "commit-8" (for example git-describe),
other use "commit-7...": this includes git-diff both in --raw
(git diff --raw, as opposed to git-diff-tree), git-diff in it's patch 
format in "index <hash>..<hash>" extended header, in "Merge:" line for 
merges in git-show output.

-- 
Jakub Narebski