user/dev discussion of public-inbox itself
 help / color / Atom feed
* view: why is the diff line number incremented by one?
@ 2020-05-09 17:32 Kyle Meyer
  2020-05-09 18:24 ` Eric Wong
  0 siblings, 1 reply; 3+ messages in thread
From: Kyle Meyer @ 2020-05-09 17:32 UTC (permalink / raw)
  To: meta

Diff links position the line one beyond what I expect.  Here's a hunk at
<https://public-inbox.org/meta/20200509082738.23602-2-e@yhbt.net/>:

diff --git a/lib/PublicInbox/Eml.pm b/lib/PublicInbox/Eml.pm
index 4508bd84..80e7c1af 100644
--- a/lib/PublicInbox/Eml.pm
+++ b/lib/PublicInbox/Eml.pm
@@ -71,11 +71,11 @@ sub re_memo ($) {
 # compatible with our uses of Email::MIME
 sub new {
 	my $ref = ref($_[1]) ? $_[1] : \(my $cpy = $_[1]);
-	if ($$ref =~ /(?:\r?\n(\r?\n))/gs) { # likely
+	if ($$ref =~ /\r?\n(\r?\n)/s) { # likely
 		# This can modify $$ref in-place and to avoid memcpy/memmove
 		# on a potentially large $$ref.  It does need to make a
 		# copy for $hdr, though.  Idea stolen from Email::Simple
-		my $hdr = substr($$ref, 0, pos($$ref), ''); # sv_chop on $$ref
+		my $hdr = substr($$ref, 0, $+[0], ''); # sv_chop on $$ref
 		substr($hdr, -(length($1))) = ''; # lower SvCUR
 		bless { hdr => \$hdr, crlf => $1, bdy => $ref }, __PACKAGE__;
 	} elsif ($$ref =~ /^[a-z0-9-]+[ \t]*:/ims && $$ref =~ /(\r?\n)\z/s) {


The link at "-71,11" is
<https://public-inbox.org/meta/4508bd84/s/?b=lib/PublicInbox/Eml.pm#n72>.
When I follow it, I'm taken to line 72, one line below the first context
line above.  I haven't been able to come up with a reason why +1 would
be preferable here, and I didn't spot any explanation when looking
around the code.  It looks like it'd just be a matter of making the
two-line change below, but perhaps that causes breakage that I didn't
notice with my light testing.

diff --git a/lib/PublicInbox/ViewDiff.pm b/lib/PublicInbox/ViewDiff.pm
index 6fe9a0d7..536bb9e3 100644
--- a/lib/PublicInbox/ViewDiff.pm
+++ b/lib/PublicInbox/ViewDiff.pm
@@ -50,12 +50,12 @@ sub diff_hunk ($$$$) {
 
 	if (defined($spfx) && defined($oid_a) && defined($oid_b)) {
 		my ($n) = ($ca =~ /^-([0-9]+)/);
-		$n = defined($n) ? do { ++$n; "#n$n" } : '';
+		$n = defined($n) ? "#n$n" : '';
 
 		$$dst .= qq(@@ <a\nhref="$spfx$oid_a/s/$dctx->{Q}$n">$ca</a>);
 
 		($n) = ($cb =~ /^\+([0-9]+)/);
-		$n = defined($n) ? do { ++$n; "#n$n" } : '';
+		$n = defined($n) ? "#n$n" : '';
 		$$dst .= qq( <a\nhref="$spfx$oid_b/s/$dctx->{Q}$n">$cb</a> @@);
 	} else {
 		$$dst .= "@@ $ca $cb @@";

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

* Re: view: why is the diff line number incremented by one?
  2020-05-09 17:32 view: why is the diff line number incremented by one? Kyle Meyer
@ 2020-05-09 18:24 ` Eric Wong
  2020-05-09 18:57   ` [PATCH] viewdiff: don't increment the reported hunk line number Kyle Meyer
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Wong @ 2020-05-09 18:24 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: meta

Kyle Meyer <kyle@kyleam.com> wrote:
> Diff links position the line one beyond what I expect.  Here's a hunk at
> <https://public-inbox.org/meta/20200509082738.23602-2-e@yhbt.net/>:

<snip>

> The link at "-71,11" is
> <https://public-inbox.org/meta/4508bd84/s/?b=lib/PublicInbox/Eml.pm#n72>.
> When I follow it, I'm taken to line 72, one line below the first context
> line above.  I haven't been able to come up with a reason why +1 would
> be preferable here, and I didn't spot any explanation when looking
> around the code.  It looks like it'd just be a matter of making the
> two-line change below, but perhaps that causes breakage that I didn't
> notice with my light testing.

Good question, it's been that way since the beginning...

It might have been due to the "0" for new files and line numbers
counting starting at "1".  But hunks aren't hyperlinks for new files,
since the blob OID already is.

> diff --git a/lib/PublicInbox/ViewDiff.pm b/lib/PublicInbox/ViewDiff.pm
> index 6fe9a0d7..536bb9e3 100644
> --- a/lib/PublicInbox/ViewDiff.pm
> +++ b/lib/PublicInbox/ViewDiff.pm
> @@ -50,12 +50,12 @@ sub diff_hunk ($$$$) {
>  
>  	if (defined($spfx) && defined($oid_a) && defined($oid_b)) {
>  		my ($n) = ($ca =~ /^-([0-9]+)/);
> -		$n = defined($n) ? do { ++$n; "#n$n" } : '';
> +		$n = defined($n) ? "#n$n" : '';
>  
>  		$$dst .= qq(@@ <a\nhref="$spfx$oid_a/s/$dctx->{Q}$n">$ca</a>);
>  
>  		($n) = ($cb =~ /^\+([0-9]+)/);
> -		$n = defined($n) ? do { ++$n; "#n$n" } : '';
> +		$n = defined($n) ? "#n$n" : '';
>  		$$dst .= qq( <a\nhref="$spfx$oid_b/s/$dctx->{Q}$n">$cb</a> @@);
>  	} else {
>  		$$dst .= "@@ $ca $cb @@";

I'll probably take this patch, soon, got a commit message?
Given the massive change Eml is, 1.5.0 will probably be released, soon.

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

* [PATCH] viewdiff: don't increment the reported hunk line number
  2020-05-09 18:24 ` Eric Wong
@ 2020-05-09 18:57   ` Kyle Meyer
  0 siblings, 0 replies; 3+ messages in thread
From: Kyle Meyer @ 2020-05-09 18:57 UTC (permalink / raw)
  To: Eric Wong; +Cc: meta

Eric Wong writes:

> I'll probably take this patch, soon, got a commit message?

Sure.  Thanks.

-- >8 --
Subject: [PATCH] viewdiff: don't increment the reported hunk line number

For a diff hunk starting at line N, diff_hunk() constructs the link
with "#n(N + 1)".  This sends the viewer one line below the first
context line.  Although this is minor and may not even be noticed,
there's not an obvious reason to increment the line number, so switch
to using the reported value as is.
---
 lib/PublicInbox/ViewDiff.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/ViewDiff.pm b/lib/PublicInbox/ViewDiff.pm
index 6fe9a0d7..536bb9e3 100644
--- a/lib/PublicInbox/ViewDiff.pm
+++ b/lib/PublicInbox/ViewDiff.pm
@@ -50,12 +50,12 @@ sub diff_hunk ($$$$) {
 
 	if (defined($spfx) && defined($oid_a) && defined($oid_b)) {
 		my ($n) = ($ca =~ /^-([0-9]+)/);
-		$n = defined($n) ? do { ++$n; "#n$n" } : '';
+		$n = defined($n) ? "#n$n" : '';
 
 		$$dst .= qq(@@ <a\nhref="$spfx$oid_a/s/$dctx->{Q}$n">$ca</a>);
 
 		($n) = ($cb =~ /^\+([0-9]+)/);
-		$n = defined($n) ? do { ++$n; "#n$n" } : '';
+		$n = defined($n) ? "#n$n" : '';
 		$$dst .= qq( <a\nhref="$spfx$oid_b/s/$dctx->{Q}$n">$cb</a> @@);
 	} else {
 		$$dst .= "@@ $ca $cb @@";
-- 
2.26.1


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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-09 17:32 view: why is the diff line number incremented by one? Kyle Meyer
2020-05-09 18:24 ` Eric Wong
2020-05-09 18:57   ` [PATCH] viewdiff: don't increment the reported hunk line number Kyle Meyer

user/dev discussion of public-inbox itself

Archives are clonable:
	git clone --mirror http://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

Example config snippet for mirrors

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.io/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.git