From mboxrd@z Thu Jan  1 00:00:00 1970
From: john at keeping.me.uk (John Keeping)
Date: Wed, 27 Jun 2018 20:51:43 +0100
Subject: [PATCH v4 00/16] Render READMEs inline in tree view
In-Reply-To: <CAHmME9qe3viiXqYkyoGhegKJuQcUfX1J6+XCvpNRexBjNvBFXQ@mail.gmail.com>
References: <152885510454.7253.3542488576272033383.stgit@mail.warmcat.com>
 <152948941145.29466.10223016890282865269.stgit@mail.warmcat.com>
 <CAHmME9qe3viiXqYkyoGhegKJuQcUfX1J6+XCvpNRexBjNvBFXQ@mail.gmail.com>
Message-ID: <20180627195143.GP6584@john.keeping.me.uk>

Hi Jason,

On Wed, Jun 27, 2018 at 07:18:57PM +0200, Jason A. Donenfeld wrote:
> With the current state of this series, cgit would have the following options:
> 
> - render.<ext>
> - inline-readme
> - render-filter

This one is only a concept, not a configuration value (just a note since
I couldn't remember introducing it!)

> - about-filter
> - commit-filter
> - source-filter
> - owner-filter
> - readme=file
> - readme=:file
> 
> Whoa nelly. Of these, about-filter, commit-filter, source-filter, and
> owner-filter all have analogs in the repo.* namespace, which makes
> sense; it seems this was omitted from the render-filter introduced by
> this series. The thing that unifies about-filter, commit-filter,
> source-filter, and owner-filter is that they all modify some aspect of
> the rendered output, either via fork/exec or via lua.
> 
> In adding readme files under the tree view, the obvious observation is
> that this is pretty much the same type of rendering that we're doing
> in about-filter.
> 
> In adding rendering of arbitrary files in blob view, this is
> essentially a fancy source view, with the one caveat of our
> interesting handling of line numbers.
> 
> So, I'd propose the following re-organization (and after we nail it
> down, we can bikeshed about compatibility with old configs, but for
> now let's focus on ideal design):

I'll comment on the proposals inline below, but before doing that let me
try to remember why "render.<ext>" exists in the first place (it's a
couple of years since I wrote that patch so I'm not entirely sure this
is the rationale I had at the time!)

Thinking though it now, I think about-filter and the render-filter
concept are equivalent.  And for the purpose of putting an inline
directory readme in the tree view, that may be sufficient.

However, my series was adding a new UI mode and there are a couple of
requirements there that I can't see a way to fix without render.<ext> or
something equivalent [1]:

- 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

- 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].

> - We retain commit-filter, source-filter, and owner-filter as we have them now.
> - We rename about-filter to readme-filter.

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.

> - We remove `readme` and instead introduce `readme-filename`, which
> can be specified multiple times as is habit. This would simply take
> the set of filenames considered to be readme files (readme.md,
> readme.txt, etc). [Bikeshed discussion: case insensitive?]
> - We introduce an options at the global level and at the .repo level
> of `about-readme=/path/to/absolute/file` and `about-readme=<BRANCH>:`
> and `about-readme=:`. The first would replace our original usage of
> `readme=/path/to=file`, and the second would replace the use of
> `readme=branch:whatever`, specifying an explicit branch (like
> cgit.git's wiki branch), and the third would indicate the default
> branch.

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.

> - We introduce an option at the global level and at the .repo level of
> `tree-readme=1/0` to display (or not) the readme under each tree.

Agree.

> - We do not introduce render-filter. We do not introduce render.<ext>;
> such extension selection is successfully handled by the various
> filters themselves already.

Agree on render-filter, but we need a better proposal for how to handle
the <iframe> case and the "render not supported" case if we want to do
those outside the filter.


[1] This could be the exit status of the render filter, but I'm not sure
    how well the "probe" mode will fit it
[2] We do load the content redundantly in this case and we'll have to
    make sure that our SIGPIPE handling is correct if the filter exits
    without reading all of its stdin, but that's probably acceptable


Regards,
John