user/dev discussion of public-inbox itself
 help / color / Atom feed
* broken link in diffstat for new files
@ 2019-07-04 23:11 SZEDER Gábor
  2019-07-05  4:03 ` [PATCH] viewdiff: do not anchor using diffstat comments Eric Wong
  0 siblings, 1 reply; 4+ messages in thread
From: SZEDER Gábor @ 2019-07-04 23:11 UTC (permalink / raw)
  To: meta

Hi,

being able to jump around from diffstat to the diff of a particular
file and back by simply clicking on links is great, but it doesn't
seem to work with newly added files.  Consider this message:

  https://public-inbox.org/git/20190624130226.17293-2-pclouds@gmail.com/

Clicking on the link that is the name of the doc source file or any of
the C source of header files in the diffstat jumps to the diff of the
particular file, but clicking on the link of any of the two new test
files doesn't go anywhere.  Conversely, in the "diff --git a/...
b/..." lines of the doc and source files the word "diff" is a link
pointing back to the diffstat, but in the diff lines of those two new
test files there is no link.

Several other patches in that patch series add new test files, and all
are affected by this issue.


Best,
Gábor

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] viewdiff: do not anchor using diffstat comments
  2019-07-04 23:11 broken link in diffstat for new files SZEDER Gábor
@ 2019-07-05  4:03 ` Eric Wong
  2019-07-05  4:06   ` Eric Wong
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2019-07-05  4:03 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: meta

SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Hi,
> 
> being able to jump around from diffstat to the diff of a particular
> file and back by simply clicking on links is great, but it doesn't
> seem to work with newly added files.  Consider this message:

Glad you noticed this feature :)  I keep forgetting to do more
work on it (fixing interdiffs and such), and also writing
automated tests to catch it.

>   https://public-inbox.org/git/20190624130226.17293-2-pclouds@gmail.com/
> 
> Clicking on the link that is the name of the doc source file or any of
> the C source of header files in the diffstat jumps to the diff of the
> particular file, but clicking on the link of any of the two new test
> files doesn't go anywhere.  Conversely, in the "diff --git a/...
> b/..." lines of the doc and source files the word "diff" is a link
> pointing back to the diffstat, but in the diff lines of those two new
> test files there is no link.

Thanks for the bug report.  The following should fix it,
it's also deployed on public-inbox.org

---------8<--------
Subject: [PATCH] viewdiff: do not anchor using diffstat comments

Diffstat summary comments were added to git last year and
we need to filter them out to get anchors working properly.

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
  https://public-inbox.org/meta/20190704231123.GF20404@szeder.dev/
---
 lib/PublicInbox/ViewDiff.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/ViewDiff.pm b/lib/PublicInbox/ViewDiff.pm
index b7dab81..4669e87 100644
--- a/lib/PublicInbox/ViewDiff.pm
+++ b/lib/PublicInbox/ViewDiff.pm
@@ -41,6 +41,9 @@ my $OID_BLOB = '[a-f0-9]{7,40}';
 my $PATH_A = '"?a/.+|/dev/null';
 my $PATH_B = '"?b/.+|/dev/null';
 
+# cf. git diff.c :: get_compact_summary
+my $DIFFSTAT_COMMENT = qr/\((?:new|gone|(?:(?:new|mode) [\+\-][lx]))\)/;
+
 sub to_html ($$) {
 	$_[0]->linkify_1($_[1]);
 	$_[0]->linkify_2(ascii_html($_[1]));
@@ -89,7 +92,7 @@ sub anchor0 ($$$$$) {
 	# So only do best-effort handling of renames for common cases;
 	# which works well in practice. If projects put "=>", or trailing
 	# spaces in filenames, oh well :P
-	$fn =~ s/ +\z//s;
+	$fn =~ s/(?: *$DIFFSTAT_COMMENT)? *\z//so;
 	$fn =~ s/{(?:.+) => (.+)}/$1/ or $fn =~ s/.* => (.+)/$1/;
 	$fn = git_unquote($fn);
 
-- 
EW

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] viewdiff: do not anchor using diffstat comments
  2019-07-05  4:03 ` [PATCH] viewdiff: do not anchor using diffstat comments Eric Wong
@ 2019-07-05  4:06   ` Eric Wong
  2019-07-05  8:55     ` SZEDER Gábor
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2019-07-05  4:06 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: meta

Eric Wong <e@80x24.org> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >   https://public-inbox.org/git/20190624130226.17293-2-pclouds@gmail.com/

Btw, I'm not sure if the "(new +x)" and all the surrounding
spaces should be clickable.  I have poor coordination, so when I
(rarely) use a mouse I prefer bigger targets to click; but maybe
it can get confusing...

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] viewdiff: do not anchor using diffstat comments
  2019-07-05  4:06   ` Eric Wong
@ 2019-07-05  8:55     ` SZEDER Gábor
  0 siblings, 0 replies; 4+ messages in thread
From: SZEDER Gábor @ 2019-07-05  8:55 UTC (permalink / raw)
  To: Eric Wong; +Cc: meta

On Fri, Jul 05, 2019 at 04:06:18AM +0000, Eric Wong wrote:
> Eric Wong <e@80x24.org> wrote:
> > SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > >   https://public-inbox.org/git/20190624130226.17293-2-pclouds@gmail.com/
> 
> Btw, I'm not sure if the "(new +x)" and all the surrounding
> spaces should be clickable.  I have poor coordination, so when I
> (rarely) use a mouse I prefer bigger targets to click; but maybe
> it can get confusing...

FWIW, I didn't find it odd or surprising that the "(new +x)" is part
of the link, and its color clearly indicates that it is part of the
link.

I haven't yet noticed that all the spaces following the filename are
clickable as well, because they don't look like part of the link
(well, being spaces, they don't have any color...), and haven't
noticed the cursor changing shape when hovering over them.

I'm not so sure about this, and think that it can be
confusing/undesired in some circumstances.  A user can click on any
"inactive" part of the browser window (i.e. what isn't a link, a
button, or any active GUI widget) to focus and raise the window
without any other effects, so they might just happen to click on
those unsuspicious spaces because they don't look like a link, and
then be surprised when the page jumps to corresponding diff.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-04 23:11 broken link in diffstat for new files SZEDER Gábor
2019-07-05  4:03 ` [PATCH] viewdiff: do not anchor using diffstat comments Eric Wong
2019-07-05  4:06   ` Eric Wong
2019-07-05  8:55     ` SZEDER Gábor

user/dev discussion of public-inbox itself

Archives are clonable:
	git clone --mirror https://public-inbox.org/meta
	git clone --mirror http://czquwvybam4bgbro.onion/meta
	git clone --mirror http://hjrcffqmbrq6wope.onion/meta
	git clone --mirror http://ou63pmih66umazou.onion/meta

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.mail.public-inbox.meta
	nntp://ou63pmih66umazou.onion/inbox.comp.mail.public-inbox.meta
	nntp://czquwvybam4bgbro.onion/inbox.comp.mail.public-inbox.meta
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.mail.public-inbox.meta
	nntp://news.gmane.org/gmane.mail.public-inbox.general

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox