Check in a simple pure-python based Markdown previewer.

Check in a simple pure-python based Markdown previewer.

We are now trying to write more and more of our documentation
as Markdown files that live in the repo, but there's no easy
way to preview changes in a local checkout.

This CL adds a simple previewing tool written in pure Python
that depends on the 'Markdown' python package (also pure Python)
to do all the heavy lifting. It aims to replicate the look
and feel of googlesource.com as closely as possible.

R=nodir@chromium.org, jam@chromium.org
BUG=

Committed: https://crrev.com/abe11cb68dd11d5ee153218ba99a3cdba3a3ced6
Cr-Commit-Position: refs/heads/master@{#352450}

[Dirk Pranke, 2015-09-22 02:44:20]

dpranke@chromium.org changed reviewers:
+ jam@chromium.org

[Dirk Pranke, 2015-09-22 02:46:37]

This is a really bare bones thing, but it works and I've gotten a few requests
for something like this to be checked in.

I'd like to land this (more-or-less) as-is and then add a couple follow-on CLs
to support directory browsing and a couple other things.

Nodir: can you approve the basic functionality?

John: can I get an OWNERS approval?

I will send a note to open-source-third-party-reviews@google.com as well.

[Dirk Pranke, 2015-09-22 03:14:55]

Patchset #1 (id:1) has been deleted

[Dirk Pranke, 2015-09-22 03:15:02]

Patchset #1 (id:20001) has been deleted

[Dirk Pranke, 2015-09-22 03:15:09]

Patchset #1 (id:30001) has been deleted

[scottmg, 2015-09-22 04:58:01]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-09-22 04:58:02]

Thanks for adding this.

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
File tools/md_browser/md_browser.py (right):

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:23: parser =
argparse.ArgumentParser(prog='md_browser')
+4 like an animal! :)

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:51: self.path[1:]))
I guess it doesn't matter for this usage, but you could maybe check for '..'s in
the path.

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:70: contents = self._read(self.path[1:])
it seems like this is already stripping the leading /, but then _read also
checks for a leading '/'?

[nodir, 2015-09-22 16:38:18]

I could not apply this patch, thus couldn't see how it looks like, how close it
is to gitiles, but if it is, looks like a great stuff. If it is indeed, consider
sharing it with a wider audience, e.g. by putting to depot_tools

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/README.md
File tools/md_browser/README.md (right):

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/README...
tools/md_browser/README.md:1: # md\_browser
nit: \_ is unnecessary in most of cases. Markdown does treat it as 'make it
italic' when it is between alphanums like foo_bar

here and below

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/README...
tools/md_browser/README.md:23: 
remove blank lines between items

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
File tools/md_browser/md_browser.py (right):

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:51: self.path[1:]))
On 2015/09/22 04:58:01, scottmg wrote:
> I guess it doesn't matter for this usage, but you could maybe check for '..'s
in
> the path.

os.path.abspath returns a normalized path, so the startswith check below should
be enough

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:58: elif self.path.endswith('.md'):
self.path.lower()... JIC

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:89: if not path.startswith('/'):
this if is unnecessary: os.path.join('/foo', '/bar') == '/bar'

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:91: return codecs.open(path, mode='r',
encoding='utf-8').read()
This seems to leave the fd open?
Shouldn't you

with codes.open(....) as f:
  return f.read()

?

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:91: return codecs.open(path, mode='r',
encoding='utf-8').read()
mode='r' is unnecessary, it is default

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:95: contents = self._read(os.path.join(SRC_DIR,
'tools', 'md_browser',
nit: I think moving the line break to the position after self._read( would be
more readable

[scottmg, 2015-09-22 17:32:46]

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
File tools/md_browser/md_browser.py (right):

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:51: self.path[1:]))
On 2015/09/22 16:38:18, nodir wrote:
> On 2015/09/22 04:58:01, scottmg wrote:
> > I guess it doesn't matter for this usage, but you could maybe check for
'..'s
> in
> > the path.
> 
> os.path.abspath returns a normalized path, so the startswith check below
should
> be enough

(I meant that by entering localhost:8080/../../../../../../../../autoexec.bat
that you could read things that you sort of shouldn't be able to. But it also
probably doesn't matter here.)

[Dirk Pranke, 2015-09-22 21:16:27]

Updated. Please take another look?

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/README.md
File tools/md_browser/README.md (right):

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/README...
tools/md_browser/README.md:1: # md\_browser
On 2015/09/22 16:38:18, nodir wrote:
> nit: \_ is unnecessary in most of cases. Markdown does treat it as 'make it
> italic' when it is between alphanums like foo_bar
> 
> here and below

I think Python's markdown does this, but I feel like I've seen Gitiles *not* do
this by default. That said, I'll make your changes and we can see :).

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/README...
tools/md_browser/README.md:23: 
On 2015/09/22 16:38:18, nodir wrote:
> remove blank lines between items

Acknowledged.

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
File tools/md_browser/md_browser.py (right):

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:23: parser =
argparse.ArgumentParser(prog='md_browser')
On 2015/09/22 04:58:01, scottmg wrote:
> +4 like an animal! :)

will fix. stupid chromium coding standards :).

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:51: self.path[1:]))
On 2015/09/22 17:32:46, scottmg wrote:
> On 2015/09/22 16:38:18, nodir wrote:
> > On 2015/09/22 04:58:01, scottmg wrote:
> > > I guess it doesn't matter for this usage, but you could maybe check for
> '..'s
> > in
> > > the path.
> > 
> > os.path.abspath returns a normalized path, so the startswith check below
> should
> > be enough
> 
> (I meant that by entering localhost:8080/../../../../../../../../autoexec.bat
> that you could read things that you sort of shouldn't be able to. But it also
> probably doesn't matter here.)

Right, but Nodir's saying (correctly, I believe) that abspath() will normalize
joined path, at which point the check on line 52 will stop you from looking at
files outside of the tree.

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:58: elif self.path.endswith('.md'):
On 2015/09/22 16:38:18, nodir wrote:
> self.path.lower()... JIC

Done, though I worry about what other sorts of weirdness this might let get
through.

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:70: contents = self._read(self.path[1:])
On 2015/09/22 04:58:01, scottmg wrote:
> it seems like this is already stripping the leading /, but then _read also
> checks for a leading '/'?

Made things more consistent.

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:89: if not path.startswith('/'):
On 2015/09/22 16:38:18, nodir wrote:
> this if is unnecessary: os.path.join('/foo', '/bar') == '/bar'

true, but the code was confusing regardless. Made things more consistent here as
well.

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:91: return codecs.open(path, mode='r',
encoding='utf-8').read()
On 2015/09/22 16:38:18, nodir wrote:
> mode='r' is unnecessary, it is default

Done, and good catch on the fd.

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:95: contents = self._read(os.path.join(SRC_DIR,
'tools', 'md_browser',
On 2015/09/22 16:38:18, nodir wrote:
> nit: I think moving the line break to the position after self._read( would be
> more readable

Once I switch to 2-space indents the whole thing fits on one line.

[scottmg, 2015-09-22 21:26:39]

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
File tools/md_browser/md_browser.py (right):

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/md_bro...
tools/md_browser/md_browser.py:51: self.path[1:]))
On 2015/09/22 21:16:27, Dirk Pranke wrote:
> On 2015/09/22 17:32:46, scottmg wrote:
> > On 2015/09/22 16:38:18, nodir wrote:
> > > On 2015/09/22 04:58:01, scottmg wrote:
> > > > I guess it doesn't matter for this usage, but you could maybe check for
> > '..'s
> > > in
> > > > the path.
> > > 
> > > os.path.abspath returns a normalized path, so the startswith check below
> > should
> > > be enough
> > 
> > (I meant that by entering
localhost:8080/../../../../../../../../autoexec.bat
> > that you could read things that you sort of shouldn't be able to. But it
also
> > probably doesn't matter here.)
> 
> Right, but Nodir's saying (correctly, I believe) that abspath() will normalize
> joined path, at which point the check on line 52 will stop you from looking at
> files outside of the tree.

Oh, you're right. Sorry.

[nodir, 2015-09-22 21:48:56]

lgtm

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/README.md
File tools/md_browser/README.md (right):

https://codereview.chromium.org/1356203004/diff/50001/tools/md_browser/README...
tools/md_browser/README.md:1: # md\_browser
On 2015/09/22 21:16:26, Dirk Pranke wrote:
> On 2015/09/22 16:38:18, nodir wrote:
> > nit: \_ is unnecessary in most of cases. Markdown does treat it as 'make it
> > italic' when it is between alphanums like foo_bar
> > 
> > here and below
> 
> I think Python's markdown does this, but I feel like I've seen Gitiles *not*
do
> this by default. That said, I'll make your changes and we can see :).

Proof:
https://chrome-internal.googlesource.com/infra/infra_internal/+/master/doc/se...

[jam, 2015-09-23 18:18:51]

rubberstamp lgtm

[Dirk Pranke, 2015-10-05 21:27:48]

opensource-licensing approval was granted (via email).

[Dirk Pranke, 2015-10-05 21:27:53]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-10-05 21:29:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356203004/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356203004/70001

[commit-bot: I haz the power, 2015-10-05 21:42:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-05 21:42:47]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dirk Pranke, 2015-10-05 22:04:04]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-10-05 22:04:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1356203004/#ps90001
(title: "fix license file")

[commit-bot: I haz the power, 2015-10-05 22:06:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356203004/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356203004/90001

[commit-bot: I haz the power, 2015-10-05 22:47:07]

Committed patchset #3 (id:90001)

[commit-bot: I haz the power, 2015-10-05 22:48:18]

Patchset 3 (id:??) landed as
https://crrev.com/abe11cb68dd11d5ee153218ba99a3cdba3a3ced6
Cr-Commit-Position: refs/heads/master@{#352450}

[Kunihiko Sakamoto, 2015-10-06 01:00:45]

On 2015/10/05 22:48:18, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/abe11cb68dd11d5ee153218ba99a3cdba3a3ced6
> Cr-Commit-Position: refs/heads/master@{#352450}

checklicenses.py is complaining:

http://build.chromium.org/p/chromium/builders/Linux/builds/67561

[Dirk Pranke, 2015-10-06 01:17:01]

On 2015/10/06 01:00:45, Kunihiko Sakamoto wrote:
> On 2015/10/05 22:48:18, commit-bot: I haz the power wrote:
> > Patchset 3 (id:??) landed as
> > https://crrev.com/abe11cb68dd11d5ee153218ba99a3cdba3a3ced6
> > Cr-Commit-Position: refs/heads/master@{#352450}
> 
> checklicenses.py is complaining:
> 
> http://build.chromium.org/p/chromium/builders/Linux/builds/67561

Hmm. I wonder how it got past the trybots. I'll revert this for now and dig into
it tomorrow.

[Dirk Pranke, 2015-10-06 01:17:38]

A revert of this CL (patchset #3 id:90001) has been created in
https://codereview.chromium.org/1389543003/ by dpranke@chromium.org.

The reason for reverting is: checklicenses.py is complaining:

http://build.chromium.org/p/chromium/builders/Linux/builds/67561
.

[scottmg, 2015-10-06 04:02:06]

On 2015/10/06 01:17:01, Dirk Pranke wrote:
> On 2015/10/06 01:00:45, Kunihiko Sakamoto wrote:
> > On 2015/10/05 22:48:18, commit-bot: I haz the power wrote:
> > > Patchset 3 (id:??) landed as
> > > https://crrev.com/abe11cb68dd11d5ee153218ba99a3cdba3a3ced6
> > > Cr-Commit-Position: refs/heads/master@{#352450}
> > 
> > checklicenses.py is complaining:
> > 
> > http://build.chromium.org/p/chromium/builders/Linux/builds/67561
> 
> Hmm. I wonder how it got past the trybots. I'll revert this for now and dig
into
> it tomorrow.

Maybe it's because checklicenses.py is a 'scripts', but those are only for
"tests"
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
so when analyze.py didn't need to compile, it skipped all of those.

[Dirk Pranke, 2015-10-06 23:41:20]

On 2015/10/06 04:02:06, scottmg wrote:
> Maybe it's because checklicenses.py is a 'scripts', but those are only for
> "tests"
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
> so when analyze.py didn't need to compile, it skipped all of those.

Yeah, I think you're right.

[jam, 2015-10-13 20:37:20]

On 2015/10/06 23:41:20, Dirk Pranke wrote:
> On 2015/10/06 04:02:06, scottmg wrote:
> > Maybe it's because checklicenses.py is a 'scripts', but those are only for
> > "tests"
> >
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
> > so when analyze.py didn't need to compile, it skipped all of those.
> 
> Yeah, I think you're right.

btw there's a bug and fix for this, but I can't find the bug anymore.

[jam, 2015-10-13 20:49:28]

On 2015/10/13 20:37:20, jam wrote:
> On 2015/10/06 23:41:20, Dirk Pranke wrote:
> > On 2015/10/06 04:02:06, scottmg wrote:
> > > Maybe it's because checklicenses.py is a 'scripts', but those are only for
> > > "tests"
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
> > > so when analyze.py didn't need to compile, it skipped all of those.
> > 
> > Yeah, I think you're right.
> 
> btw there's a bug and fix for this, but I can't find the bug anymore.

ok here's the bug https://code.google.com/p/chromium/issues/detail?id=462308
which is fixed. the fix only looks at a limited set of file extensions:
https://codereview.chromium.org/1346173004/diff/20001/scripts/slave/recipes/c...

we could perhaps add .py to it?

[Dirk Pranke, 2015-10-13 21:02:52]

On 2015/10/13 20:49:28, jam wrote:
> ok here's the bug https://code.google.com/p/chromium/issues/detail?id=462308
> which is fixed. the fix only looks at a limited set of file extensions:
>
https://codereview.chromium.org/1346173004/diff/20001/scripts/slave/recipes/c...
> 
> we could perhaps add .py to it?

Yeah, that sounds like it would be reasonable.