Bk changes --html broken output


#1

I’d like to report 2 bugs in the output generated by bk changes --html ...:

  • the HTML is invalid (bad nesting of <tr> and <td> elements, sometimes not closed with </tr>, …etc.)
  • the bk comments making up the table content is not masked in HTML. If, for example, a ChangeSet contains a comment with special HTML characters like “I’m including <stdio.h> now” it’ll mess up the output.

While most browsers can handle the first topic gracefully, the second one becomes a real issue. Also, the output is ancient HTML 3.2 and always contains <body> and <html> elements, which makes it difficult to embed into an existing web-page like my company’s nightly build reporting web-page.

# bk version
BitKeeper version is bk-7.3.2 for x86-glibc219-linux
Built by: wscott@debian80.bitkeeper.com in /build/bk-7.3.2-wscott/src
Built on: Sat Sep 23 2017 12:20:08 CEST (6 months ago)
Running on: x86-glibc219-linux,4.14.11-300.fc27.i686

#2

So let’s walk so code and see how this stuff works. Think if it as me rambling as I work out the issues.

In src/changes.c we have the code that handles the --html flag:

	longopt	lopts[] = {
		{ "no-meta", 300 },		/* don't show meta files */
						/* seems more like --no attr */
		{ "html", 301 },		/* old -h */
...
	while ((c =
	    getopt(ac, av, "0123456789aBc;Dd;efi;kLmnPqRr;StTu;U;Vv/;x;",
		lopts)) != -1) {
		unless (c == 'L' || c == 'R' || c == 'D' || c == 302) {
			nav = bk_saveArg(nav, av, c);
		}
		switch (c) {
...
		    case 301: opts.html = 1; opts.urls = 0; break;
     }

So it sets opts.html. (opts.urls is the -q option which is also implied)

And later we have this:

	unless (opts.dspec) {
		if (opts.html) {
			spec = opts.verbose ?
			    "dspec-changes-hv" :
			    "dspec-changes-h";

So when you don’t specify the dspec to changes explicitly that lists the file that it used to for the default output of this command.

src/dspec-changes-h looks like this:

# the default dspec used by 'bk changes --html'
# Note: Do not use :USERHOST: as :USER:@:HOST will print @ if no host

$begin {
	"<html><body bgcolor=white>\n"
	"<table align=center bgcolor=black cellspacing=0 "
	"border=0 cellpadding=0>"
	"<tr><td>\n"
	"<table width=100% cellspacing=1 border=0 cellpadding=1>"
	"<tr><td>\n"
}

"<tr bgcolor=lightblue>"
  "<td font size=4>"
    "&nbsp;:Dy:-:Dm:-:Dd: :Th:::Tm:&nbsp;&nbsp;:USER:@:HOST:&nbsp;&nbsp;:REV:"
  "</td>"
"</tr>\n"
$if (:TAGS:) {
	"<tr bgcolor=yellow>"
	  "<td>"
	    $each(:TAGS:) {
		"&nbsp;TAG: (:TAGS:)<br>\n"
	    }
	  "</td>"
	"</tr>\n"
}
"<tr bgcolor=white>"
  "<td>"
    $each(:C:) {
      "&nbsp;(:C:)<br>\n"
    }
  "</td>"
"</tr>\n"

$end {
	"</td></tr></table></table></body></html>\n"
}

As you can see your problem HTML was written by an old C hacker who learned HTML in '97. (@mcvoy)

The first problem can be fixed in that file. The second problem was with the rendering the comments section:

    $each(:C:) {
      "&nbsp;(:C:)<br>\n"
    }

Try replacing that whole block with just :HTML_C: which is coded in src/slib.c:15430

If you get a fix that works, fix the src/dspec-changes-hv file too and then put them in a gist and link them here. I will check them into the bk://bkbits.net/bk/dev branch that holds the not yet released changes.


#3

Thanks, Wayne, for the extremely quick and profound answer. You spotted the right places in the code already, so a fix should not be too hard. However, I’m just a bitkeeper user and no developer, so I can’t fix this myself.

Your hints with specifying a dspec for arbitrary output formats sounds very promising! I’ll check if that can be of some use in the meantime.


#4

I think you can fix this since you seem to understand what needs to happen.

Try this

  • Copy dspec-changes-h from the bk installation directory to some other directory under your control

  • edit that file to make changes you want

  • run bk changes --dspecf=<new file> to see if the changes do what you want.

  • Start with that :HTML_C: change I mentioned

  • keep tweaking file and trying again

Let us know what works.

We have 2 different files the dspec-changes-hv file is used when you want to pass -v to changes.


#5

This is awesome! I had no idea that bitkeeper is so flexible in its output specification (dspec-v2). That offers a whole new field of possibilities. I learned a lot today.

I was able to modify & fix the two mentioned HTML-dspec files (verbose and non-verbose) with respect to both bugs originally reported by me. I will attach two context-diffs with minimal edits. The file dspec-changes-hv (which contains comments from individual file revisions within a ChangeSet) used to loop over the comment-lines - starting each file-revision comment-line with indentation - no longer works, because :HTML_C: outputs a whole pre-formatted block. So indentation is lost. However, it’s still better than producing garbled HTML output with :C:.
So much more styling could be done using modern HTML+CSS, but I didn’t want to go that far.

Hm, since I’m a new user, the system doesn’t allow me uploading my patches. So I will post them inline here:

  • changes to file dspec-changes-h:
*** dspec-changes-h.orig	2018-03-15 15:58:22.000000000 +0100
--- dspec-changes-h	2018-03-15 18:26:41.000000000 +0100
***************
*** 22,33 ****
  	"border=0 cellpadding=0>"
  	"<tr><td>\n"
  	"<table width=100% cellspacing=1 border=0 cellpadding=1>"
! 	"<tr><td>\n"
  }
  
  "<tr bgcolor=lightblue>"
    "<td font size=4>"
!     "&nbsp;:Dy:-:Dm:-:Dd: :Th:::Tm:&nbsp;&nbsp;:USER:@:HOST:&nbsp;&nbsp;:REV:"
    "</td>"
  "</tr>\n"
  $if (:TAGS:) {
--- 22,33 ----
  	"border=0 cellpadding=0>"
  	"<tr><td>\n"
  	"<table width=100% cellspacing=1 border=0 cellpadding=1>"
! 	"\n"
  }
  
  "<tr bgcolor=lightblue>"
    "<td font size=4>"
!     "&nbsp;:D_: :Th:::Tm:&nbsp;&nbsp;:USER:@:HOST:&nbsp;&nbsp;:REV:"
    "</td>"
  "</tr>\n"
  $if (:TAGS:) {
***************
*** 41,52 ****
  }
  "<tr bgcolor=white>"
    "<td>"
!     $each(:C:) {
!       "&nbsp;(:C:)<br>\n"
!     }
    "</td>"
  "</tr>\n"
  
  $end {
! 	"</td></tr></table></table></body></html>\n"
  }
--- 41,50 ----
  }
  "<tr bgcolor=white>"
    "<td>"
!       ":HTML_C:"
    "</td>"
  "</tr>\n"
  
  $end {
! 	"</table></td></tr></table></body></html>\n"
  }
  • patch for dspec-changes-hv:
*** dspec-changes-hv.orig	2018-03-15 16:46:17.000000000 +0100
--- dspec-changes-hv	2018-03-15 19:08:39.000000000 +0100
***************
*** 22,28 ****
  	"border=0 cellpadding=0>"
  	"<tr><td>\n"
  	"<table width=100% cellspacing=1 border=0 cellpadding=1>"
! 	"<tr><td>\n"
  }
  
  
--- 22,28 ----
  	"border=0 cellpadding=0>"
  	"<tr><td>\n"
  	"<table width=100% cellspacing=1 border=0 cellpadding=1>"
! 	"\n"
  }
  
  
***************
*** 35,41 ****
    "><td font size=4>"
      "&nbsp;"
      $if (:CHANGESET:) {
!       ":Dy:-:Dm:-:Dd: :Th:::Tm:&nbsp;&nbsp;:USER:@:HOST:&nbsp;&nbsp;:REV:"
      } $else {
        "&nbsp;:DPN: :REV:"
      }
--- 35,41 ----
    "><td font size=4>"
      "&nbsp;"
      $if (:CHANGESET:) {
!       ":D_: :Th:::Tm:&nbsp;&nbsp;:USER:@:HOST:&nbsp;&nbsp;:REV:"
      } $else {
        "&nbsp;:DPN: :REV:"
      }
***************
*** 52,64 ****
  }
  "<tr bgcolor=white>"
    "<td>"
!     $each(:C:) { 
!       $unless(:CHANGESET:) {
!         "&nbsp;&nbsp;&nbsp;&nbsp;"
!       }
!       "&nbsp;(:C:)<br>\n"
!     }
!     $unless (:C:) {
        $unless(:CHANGESET:) {
          "&nbsp;&nbsp;&nbsp;&nbsp;"
        }
--- 52,60 ----
  }
  "<tr bgcolor=white>"
    "<td>"
!     $if (:C:) {
!       ":HTML_C:"
!     } $else {
        $unless(:CHANGESET:) {
          "&nbsp;&nbsp;&nbsp;&nbsp;"
        }
***************
*** 68,72 ****
  "</tr>\n"
  
  $end {
! 	"</td></tr></table></table></body></html>\n"
  }
--- 64,68 ----
  "</tr>\n"
  
  $end {
! 	"</table></td></tr></table></body></html>\n"
  }

#6

As one of the guys behind some of the ideas in dspecs, I couldn’t be happier that finally someone looked at it and got it.

If you know AWK it’s very awk-like. $begin/$end are from awk as is the wad of code that gets run on each delta.

If you have questions about dspecs post them here or contact dev@bitkeeper.com and I’ll be happy to nerd out about it as much as you like. I think dspecs are neat. Check out

dspec-changes-json

and

dspec-changes-json-v

One of our devs said I couldn’t do json in dspecs. He was wrong :slight_smile:


#7

Our docs don’t make this easy to find, here are the json outputs:

bk changes --json # uses dspec-changes-json

bk changes -v --json # uses dspec-changes-json-v

If you like that sort of thing, those dspecs are a fun read. A careful reading of “bk help dspec” will explain a lot but you can ask us questions, I’m ecstatic to talk about dspecs, I think they are really neat.

I’m fixing the docs and then I’ll do a cset to pick up your changes.

Or if you like, you can bk citool those changes and then do this

bk makepatch -r+ | mail lm@mcvoy.com

and I’ll add that to the history and push it out. I’d prefer that, then you credit for your work.


#8

Cleaned up your diffs and ended up with this:
http://repos.bkbits.net/bk/dev/?PAGE=patch&REV=5aab8e29XF-Qhw5hR4xWGSIZwByHgA

These changes have been pushed to bk://bkbits.net/bk/dev and will be included in the next release.


#9

Hi Larry, thank you for your reply.

I sent you the Bitkeeper patch for ‘bk changes --html’ output by email.

dspec is great. I’m now using a customized version for bk changes to report the HTML I need for my application.

To be honest, I didn’t read the documentation :slight_smile: - my bad.
JSON-format can also be useful for the future. I see that you are implementing a state-machine with $0…$2 variables to keep track of the states while bitkeeper keeps evaluating the dspec-v2 file for each iteration/delta. I like your awk-like language. I can see $json() commands, etc. Is there a complete documentation for dspec-v2 somewhere available?


#10

Woohoo, guys, you seem to work faster than I can type!
Thanks Wayne! Your patch looks great. I saw you also fixed :HTML_C: expansion so that $each(:HTML_C:) can still be used (I didn’t dare to suggest that :slight_smile: ).
Awesome work, guys.