[PATCH 0/2] side-by-side-diff fixes

[PATCH 0/2] side-by-side-diff fixes

[plenz]

Two trivial fixes to the side-by-side-diff UI. It seems this UI is not
widely used, otherwise someone would have noticed...

Julius Plenz (2):
  bugfix: in ssdiff, fix line number links
  bugfix: make ss-diff correctly handle tab expansion

 ui-ssdiff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
1.7.12.3-zedat

[PATCH 1/2] bugfix: in ssdiff, fix line number links

[plenz]

Previously, the id_str (i.e. the current or diffed-against commit's
SHA1 ID) was simply concatenated to the URL. Now, prepend an "id="
string so that the links actually point to the right blobs and thus
the exact lines.

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 ui-ssdiff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui-ssdiff.c b/ui-ssdiff.c
index 0cff4b8..7108779 100644
--- a/ui-ssdiff.c
+++ b/ui-ssdiff.c
@@ -230,7 +230,7 @@ static void print_ssdiff_line(char *class,
 	if (old_line_no > 0) {
 		struct diff_filespec *old_file = cgit_get_current_old_file();
 		char *lineno_str = fmt("n%d", old_line_no);
-		char *id_str = fmt("%s#%s", is_null_sha1(old_file->sha1)?"HEAD":sha1_to_hex(old_rev_sha1), lineno_str);
+		char *id_str = fmt("id=%s#%s", is_null_sha1(old_file->sha1)?"HEAD":sha1_to_hex(old_rev_sha1), lineno_str);
 		html("<td class='lineno'><a class='no' href='");
 		html(cgit_fileurl(ctx.repo->url, "tree", old_file->path, id_str));
 		htmlf("' id='%s' name='%s'>%s</a>", lineno_str, lineno_str, lineno_str + 1);
@@ -251,7 +251,7 @@ static void print_ssdiff_line(char *class,
 	if (new_line_no > 0) {
 		struct diff_filespec *new_file = cgit_get_current_new_file();
 		char *lineno_str = fmt("n%d", new_line_no);
-		char *id_str = fmt("%s#%s", is_null_sha1(new_file->sha1)?"HEAD":sha1_to_hex(new_rev_sha1), lineno_str);
+		char *id_str = fmt("id=%s#%s", is_null_sha1(new_file->sha1)?"HEAD":sha1_to_hex(new_rev_sha1), lineno_str);
 		html("<td class='lineno'><a class='no' href='");
 		html(cgit_fileurl(ctx.repo->url, "tree", new_file->path, id_str));
 		htmlf("' id='%s' name='%s'>%s</a>", lineno_str, lineno_str, lineno_str + 1);
-- 
1.7.12.3-zedat

[PATCH 1/2] bugfix: in ssdiff, fix line number links

[Jason]

On Tue, Oct 30, 2012 at 6:56 AM, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> Previously, the id_str (i.e. the current or diffed-against commit's
> SHA1 ID) was simply concatenated to the URL. Now, prepend an "id="
> string so that the links actually point to the right blobs and thus
> the exact lines.

Looks good  - nice find.

Merged into a branch until the other ssdiff patch is worked out too:
http://git.zx2c4.com/cgit/commit/?h=jp/ssdiff&id=2530fde3a7b5f0751a68839bab406a272f154bbd

[PATCH 2/2] bugfix: make ss-diff correctly handle tab expansion

[plenz]

Previously, replace_tabs("foo\tbar") would become "        foobar".

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 ui-ssdiff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui-ssdiff.c b/ui-ssdiff.c
index 7108779..fbb46cf 100644
--- a/ui-ssdiff.c
+++ b/ui-ssdiff.c
@@ -138,9 +138,9 @@ static char *replace_tabs(char *line)
 			strcat(result, prev_buf);
 			break;
 		} else {
+			strncat(result, prev_buf, cur_buf - prev_buf);
 			strcat(result, " ");
 			strncat(result, spaces, 8 - (strlen(result) % 8));
-			strncat(result, prev_buf, cur_buf - prev_buf);
 		}
 		prev_buf = cur_buf + 1;
 	}
-- 
1.7.12.3-zedat

[PATCH 2/2] bugfix: make ss-diff correctly handle tab expansion

[plenz]

Hi,

reading over the patch again, I realize that in the original
behaviour:

>  			strcat(result, " ");
>  			strncat(result, spaces, 8 - (strlen(result) % 8));

... a tab is it least two spaces wide (if strlen == 8*n + 7) and up to
nine spaces wide (if strlen == 8*n). This sounds odd, so I'd suggest
the obvious fix of adding (7 - ((strlen(result)-1) % 8) spaces, since
one space is already there.

What do you think?

Julius

[PATCH 2/2] bugfix: make ss-diff correctly handle tab expansion

[Jason]

I haven't reviewed this carefully yet, but off hand, it seems like a
tab is always between 1 and 8 spaces long.

Let the first column be column 1, and any column be column C. Then,

if C % 8 == 1, spaces := 8
if C % 8 == 2, spaces := 7
if C % 8 == 3, spaces := 6
if C % 8 == 4, spaces := 5
if C % 8 == 5, spaces := 4
if C % 8 == 6, spaces := 3
if C % 8 == 7, spaces := 2
if C % 8 == 0, spaces := 1

This is the intuitive approach visually confirmed by mucking around
inside of VIM. Now let's play with it for 0 indexed columns. We can
just rotate:

if C % 8 == 0, spaces := 8
if C % 8 == 1, spaces := 7
if C % 8 == 2, spaces := 6
if C % 8 == 3, spaces := 5
if C % 8 == 4, spaces := 4
if C % 8 == 5, spaces := 3
if C % 8 == 6, spaces := 2
if C % 8 == 7, spaces := 1


The formula is then just:

8 - ( C % 8 )

Which should be a pretty simple way of calculating it, since "the
0-based column we're at" is just the current length.

Doing it this way, you don't have to hard code the initial space,
since m % n < n.

[PATCH 2/2] bugfix: make ss-diff correctly handle tab expansion

[plenz]

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-30 17:58]:
> Doing it this way, you don't have to hard code the initial space,
> since m % n < n.

Not really true. If you are in the 8th column (i.e., there are 8
characters on the line), you need a tab to be 8 spaces wide.
Otherwise, a tab should pad to the next multiple of 8.

So it's rather: len % 8 == 0 ? 8 : (8 - len % 8)

Julius

[PATCH 2/2] bugfix: make ss-diff correctly handle tab expansion

[Jason]

> len % 8 == 0 ? 8 : (8 - len % 8)


if len % 8 == 0,
    8


8 = 8 - 0
len % 8 := 0
8 = 8 - (len % 8)


so the original formula holds

[PATCH 2/2] bugfix: make ss-diff correctly handle tab expansion

[plenz]

Hi, Jason!

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-30 18:28]:
> > len % 8 == 0 ? 8 : (8 - len % 8)

> if len % 8 == 0,
>     8

> so the original formula holds

You are right. If 0 <= n < 8, then 8 - n is between 1 and 8, which is
what we want. If I see correctly, we can just drop the line that
strcat's the one space and thus repair the error?

Julius

P.S.: I'm a mathematician, you see, I have troubles with numbers
bigger than two. They are so confusing. ;-)

[PATCH 2/2] bugfix: make ss-diff correctly handle tab expansion

[Jason]

On Tue, Oct 30, 2012 at 6:52 PM, Julius Plenz <plenz at cis.fu-berlin.de>wrote:
>
> You are right. If 0 <= n < 8, then 8 - n is between 1 and 8, which is
> what we want.


Hi Julius,

Did you ever finish up v2 of this patch? I want to release a new version
soon, and have this fix with it.

Jason

[PATCH v2] bugfix: make ss-diff correctly handle tab expansion

[plenz]

Previously, replace_tabs("foo\tbar") would become "        foobar".

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 ui-ssdiff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ui-ssdiff.c b/ui-ssdiff.c
index c54f7a0..6fe2268 100644
--- a/ui-ssdiff.c
+++ b/ui-ssdiff.c
@@ -138,9 +138,8 @@ static char *replace_tabs(char *line)
 			strcat(result, prev_buf);
 			break;
 		} else {
-			strcat(result, " ");
-			strncat(result, spaces, 8 - (strlen(result) % 8));
 			strncat(result, prev_buf, cur_buf - prev_buf);
+			strncat(result, spaces, 8 - (strlen(result) % 8));
 		}
 		prev_buf = cur_buf + 1;
 	}
-- 
1.7.12.3-zedat

[PATCH v2] bugfix: make ss-diff correctly handle tab expansion

[Jason]

Merged at last:

http://git.zx2c4.com/cgit/commit/?id=225c8aba3171156fb917abe043ea55797e2cc1f9