gitweb: Add "next" link to commitdiff view

PATCH In review Jakub Narebski 4 participants 22 messages Jun 15, 2016 – Aug 11, 2016

lore

Share & tools

lore

Jakub Narebski

author

2016-06-15 22:42 UTC #0 lore raw

PATCH 1/1 gitweb: Add "next" link to commitdiff view

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 <[email protected]>

---

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(-)

gitweb/gitweb.perl +45 -4

index c9e57f0..4241d5c 100755
@@ -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);

Signature · 1 line

1.4.2.1

Jakub Narebski

author

2016-06-15 22:42 UTC #1 lore raw

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.

Signature · 2 lines

Jakub Narebski
Poland

Junio C Hamano

2016-06-15 22:42 UTC #2 lore raw

In reply to Jakub Narebski

Quoted 14 lines · 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.

Quoted 1 line

Signed-off-by Jakub Narebski <[email protected]>

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.

Jakub Narebski

author

2016-06-15 22:42 UTC #3 lore raw

Quoted 7 lines · Junio C Hamano

Quoted 5 lines · 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_)

Perhaps we should use "(from: _commit_ to: _commit_)" here...

Quoted 9 lines

Quoted 9 lines
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.

Quoted 5 lines

Quoted 1 line
Signed-off-by Jakub Narebski <[email protected]>
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...

Quoted 3 lines

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.

Signature · 2 lines

Jakub Narebski
Poland

Jakub Narebski

author

2016-06-15 22:42 UTC #4 lore raw

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

In reply to Junio C Hamano

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

Signed-off-by Jakub Narebski <[email protected]>

---

Quoted 16 lines · Junio C Hamano

Quoted 13 lines · Jakub Narebski
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.

Quoted 7 lines

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(-)

gitweb/gitweb.perl +6 ~~-10~~

index 4241d5c..255487d 100755
@@ -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'}} ) .
 				')';
 		}

Signature · 1 line

1.4.2.1

Petr Baudis

2016-06-15 22:42 UTC #5 lore raw

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

Dear diary, on Tue, Oct 24, 2006 at 12:08:08AM CEST, I got a letter where Jakub Narebski <[email protected]> said that...

Quoted 14 lines

gitweb/gitweb.perl +2 -2

@@ -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 .=

gitweb/gitweb.perl sub git_commitdiff { · line 3321

Quoted 10 lines

gitweb/gitweb.perl +2 -2

-				' (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.

Signature · 5 lines

				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*',/((..)*)$/)

Jakub Narebski

author

2016-06-15 22:42 UTC #6 lore raw

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

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 <[email protected]>

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

gitweb/gitweb.perl +7 ~~-10~~

index 4241d5c..9c81214 100755
@@ -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'}} ) .
 				')';
 		}

Signature · 1 line

1.4.2.1

Junio C Hamano

2016-06-15 22:42 UTC #7 lore raw

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

Quoted 8 lines · 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 <[email protected]>
---

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

Jakub Narebski

author

2016-06-15 22:42 UTC #8 lore raw

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

Quoted 12 lines · Junio C Hamano

Quoted 8 lines · 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 <[email protected]>
---
Q1. (amend) to fix what?

Quoted 5 lines · Petr "Pasky" Baudis

Quoted 5 lines

} else {

                      # merge commit
                      $formats_nav .=
-                             ' (merge: ' .
+                             ' (' .

[...]

Quoted 2 lines

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).

Quoted 1 line

Q2. "mext"?

Oops.

Signature · 2 lines

Jakub Narebski
Poland

Petr Baudis

2016-06-15 22:42 UTC #9 lore raw

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

In reply to Jakub Narebski

Dear diary, on Tue, Oct 24, 2006 at 12:08:08AM CEST, I got a letter where Jakub Narebski <[email protected]> said that...

Quoted 4 lines

Use fixed string instead of shortened SHA1 identifier of commit as a name for "mext" link in commitdiff view.
Signed-off-by Jakub Narebski <[email protected]>

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

Quoted 17 lines

Quoted 16 lines · Junio C Hamano
Quoted 13 lines · Jakub Narebski
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.

Quoted 3 lines

Quoted 3 lines
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

Quoted 3 lines

Quoted 3 lines
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.

Signature · 5 lines

				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*',/((..)*)$/)

Jakub Narebski

author

2016-06-15 22:42 UTC #10 lore raw

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

Quoted 12 lines · Petr Baudis

Dear diary, on Tue, Oct 24, 2006 at 12:08:08AM CEST, I got a letter where Jakub Narebski <[email protected]> said that...
Quoted 4 lines
Use fixed string instead of shortened SHA1 identifier of commit as a name for "mext" link in commitdiff view.
Signed-off-by Jakub Narebski <[email protected]>
So I've read what the patch actually does this time, and I really dislike it.
Quoted 2 lines
Quoted 1 line · Junio C Hamano
Jakub Narebski <[email protected]> writes:

[...]

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
For commitdiff for one merge commit
(merge: _commit_ _commit_ ...)

[...]

Quoted 9 lines

Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
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?

Quoted 10 lines

Quoted 3 lines
Quoted 3 lines
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.

Signature · 2 lines

Jakub Narebski
Poland

Petr Baudis

2016-06-15 22:42 UTC #11 lore raw

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

Dear diary, on Tue, Oct 24, 2006 at 01:59:57PM CEST, I got a letter where Jakub Narebski <[email protected]> said that...

Quoted 11 lines

Quoted 10 lines
Quoted 3 lines
Quoted 3 lines
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.

Quoted 2 lines

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.

Signature · 5 lines

				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*',/((..)*)$/)

Junio C Hamano

2016-06-15 22:42 UTC #12 lore raw

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

In reply to Petr Baudis

Quoted 10 lines · Petr Baudis

Quoted 3 lines
Quoted 3 lines
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.).

Petr Baudis

2016-06-15 22:42 UTC #13 lore raw

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

Quoted 13 lines · Junio C Hamano

Quoted 10 lines · Petr Baudis
Quoted 3 lines
Quoted 3 lines
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.

Quoted 2 lines

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?

Quoted 5 lines

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.

Signature · 5 lines

				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*',/((..)*)$/)

Junio C Hamano

2016-06-15 22:42 UTC #14 lore raw

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

Quoted 27 lines · Petr Baudis

Quoted 13 lines · Junio C Hamano
Quoted 10 lines · Petr Baudis
Quoted 3 lines
Quoted 3 lines
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.
Quoted 2 lines
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?

Quoted 8 lines

Quoted 5 lines
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?

Jakub Narebski

author

2016-06-15 22:42 UTC #15 lore raw

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

Quoted 7 lines · Junio C Hamano

Quoted 2 lines · Petr Baudis
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:

Quoted 3 lines

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.

Signature · 3 lines

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Jakub Narebski

author

2016-08-11 20:37 UTC #16 lore raw

In reply to Jakub Narebski

Quoted 14 lines · 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.

Quoted 6 lines

Signed-off-by Jakub Narebski <[email protected]>
---
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.

Signature · 1 line

Jakub Narebski

Luben Tuikov

Acked-by

2016-08-11 19:18 UTC #17 lore raw

Quoted 18 lines · --- Jakub Narebski

Quoted 17 lines · 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 <[email protected]>
---

Quoted 8 lines

Quoted 3 lines
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 <[email protected]>

(not that it matters in any way ;-) )

Luben

Jakub Narebski

author

2016-08-11 19:46 UTC #18 lore raw

Quoted 15 lines · Luben Tuikov

Quoted 14 lines · --- Jakub Narebski
Quoted 13 lines · 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.

[...]

Quoted 6 lines

Quoted 5 lines
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.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

Acked-by Luben Tuikov <[email protected]>
(not that it matters in any way ;-) )

IIRC Junio asked for ACKs.

Signature · 1 line

Jakub Narebski

Luben Tuikov

2016-08-11 20:11 UTC #19 lore raw

Quoted 27 lines · --- Jakub Narebski

Quoted 15 lines · Luben Tuikov
Quoted 14 lines · --- Jakub Narebski
Quoted 13 lines · 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.
[...]
Quoted 10 lines
Quoted 5 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines
Acked-by Luben Tuikov <[email protected]>
(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

Jakub Narebski

author

2016-08-11 20:15 UTC #20 lore raw

Quoted 6 lines · Luben Tuikov

Quoted 2 lines · --- Jakub Narebski
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.

Signature · 1 line

Jakub Narebski

Junio C Hamano

2016-08-11 20:32 UTC #21 lore raw

In reply to Jakub Narebski

Quoted 15 lines · Jakub Narebski

Quoted 14 lines · 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.

Quoted 8 lines

Quoted 6 lines
Signed-off-by Jakub Narebski <[email protected]>
---
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: <[email protected]>

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