[PATCH v4 00/16] Render READMEs inline in tree view

[John Keeping]

On Thu, Jun 28, 2018 at 01:22:34AM +0200, Jason A. Donenfeld wrote:
> Hey John,
> 
> Thanks tons for your input, as always.
> 
> On Wed, Jun 27, 2018 at 9:51 PM John Keeping <john at keeping.me.uk> wrote:
> > - It is desirable to have the existing source view in addition to the
> >   rendered content, preferably with syntax highlighting via the source
> >   filter; for example Markdown, HTML or SVG can be sensibly viewed in
> >   both ways
> 
> Got it, so this is the big kicker. Essentially we need a nice way for
> the render filter to fallback to the source filter, as you said, with
> then a `&source=1` override (and link) if a user explicitly wants the
> source, in order to have both views. That seems like a good idea. This
> would solve the issue with line numbers as well.
> 
> But how to implement this efficiently? With the lua filters, it really
> doesn't matter, but with the exec filters, we perhaps incur the
> penalty of exec'ing twice. Maybe that's acceptable, though. The other
> approach would be for the source-filter to print its own line numbers,
> and so the render filter itself could just fall back to calling
> internally the source filter, and perhaps even indicating what it did
> via the exit code, but I think I like this idea less. A third option
> is to do this selection entirely within cgit via mimetype/fileext
> selection, while I was initially hesitant about this, maybe this is an
> okay approach after all.
> 
> Assuming we go with the first approach -- of the renderer fallback --
> we would then have:
> 
> - render-filter (nee about-filter)
> - source-filter
> 
> Then, if render-filter returns an exit code, cgit assumes we're in
> source filter mode.

Yeah, I don't think there's any way to avoid exec'ing twice in source
view - we need to run the source filter for output and we need the
render filter to tell us whether we should output a link to the rendered
content.

If we've been asked to render and can render, then the render filter can
just run and exit normally, but if it wants to say "unsupported" then we
have to exec the source filter as well.

There is an alternative if we're willing to move away from simple
filters and introduce a new filter type which provides headers before
its output, for example:

	Mode: source
	Supported-modes: source, render

	<content...>

I'm not convinced that this is a good thing, but the only other way to
avoid exec'ing two processes is to configure in CGit.  My hesitation
with that is that the only way we have to do that is via file extensions
and I don't really like tying everything to a file extension ("README"
is a perfectly fine filename and we should be able to display that as
rendered Asciidoc if that's what it is).

> > - For some content types, the easiest way to render it is to simply
> >   include the file with an iframe; this is the case for images and PDFs
> >   at the moment
> >
> > Now, if the render filter can say "I don't support this" and can do so
> > reasonably efficiently, we can use that to control whether render mode
> > is enabled and whether we use the "fallback to mimetype" to include an
> > iframe.  Or with the asset path extension to the filter arguments, we
> > could have the filter generate the <iframe> element itself [2].
> 
> This should certainly be up to the actual render script. This means
> we'll need to path sensible blob paths so that the render script can
> correctly fill in <img src="..." or populate a pdf.js output
> correctly.

That's what the asset-path feature elsewhere in this series does,
especially if we take your idea of passing the URL path to the file
being rendered.  So we should make that change and pass the path to the
file instead of just a directory.

> > I agree with unifying the concepts, but might want to bikeshed the name
> > since if we go for render mode on all files then it applies to a bit
> > more than just readme files.
> 
> You're right about that. render-filter it is then.
> 
> > I think a distinction between repo-level readme and readme that can
> > apply to each directory is useful.  We can probably also fall back to
> > directory-level readme config using the default branch and root
> > directory if the repo-level config is unset.
> >
> > I'm not sure there's much value in removing blob references from
> > about-readme, although extending it to allow specifying a tree and then
> > looking for the readme filename would be neat.
> 
> Yes, right. So, about-readme takes these values:
> 
> about-readme=/path/to/absolute/file/not/in/a/git/repo
> about-readme=BRANCH:file/in/branch.md
> about-readme=BRANCH:    # selects a "readme" file from BRANCH
> about-readme=:file/in/default/branch/stuff.md   # selects stuff.md
> from default branch
> about-readme=:    # selects a "readme" file from the default branch
> 
> This winds up being pretty powerful and relatively simple to
> implement. What is a "readme" file? Well, that's given by
> readme-filename, as mentioned earlier.
> 
> Organizationally, since now we're going to be introducing rendering
> into quite a few places, I think we should do this in ui-render.c.
> 
> Putting all of this together, we now have the following:
> 
> - We retain commit-filter, source-filter, and owner-filter as we have them now.
> - We rename about-filter to render-filter.
> - render-filter receives a fuller filename with useful path
> information for printing out iframes, img tags, and so forth.
> - We remove readme and instead introduce readme-filename instead.
> - We introduce a global and a .repo level about-readme that takes the
> above syntax.
> - We introduce tree-readme=1/0.
> - Viewing files in /tree defaults to render-filter. If that returns
> error, it falls back to source-filter.
> - Viewing files with /tree?source=1 goes straight to source-filter.

It's /source/ instead of /tree/ in the current series and I think that's
better than a query parameter since this is a somewhat different view.

> - When source-filter is used, cgit prints its usual line numbers.
> - If source-filter fails, we print our nice hexdump or plaintext printer.

These two are "source-filter usage is unchange", right?

> - When render-filter is used, cgit shows in the top bar a helpful link
> to go to the source view.
> 
> Does that seem like a good synthesis of ideas? Or still more things to consider?

Yes, that looks good to me, modulo figuring out exactly how
render-filter operates.


John