From mboxrd@z Thu Jan  1 00:00:00 1970
From: andy at warmcat.com (Andy Green)
Date: Thu, 28 Jun 2018 06:48:49 +0800
Subject: [PATCH v4 00/16] Render READMEs inline in tree view
In-Reply-To: <20180627195143.GP6584@john.keeping.me.uk>
References: <152885510454.7253.3542488576272033383.stgit@mail.warmcat.com>
 <152948941145.29466.10223016890282865269.stgit@mail.warmcat.com>
 <CAHmME9qe3viiXqYkyoGhegKJuQcUfX1J6+XCvpNRexBjNvBFXQ@mail.gmail.com>
 <20180627195143.GP6584@john.keeping.me.uk>
Message-ID: <8e0b16ed-bb16-8003-6a8e-98501422f472@warmcat.com>



On 06/28/2018 03:51 AM, John Keeping wrote:
> 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.

That's also how it seems to be having meddled about in there a bit now. 
In fact just globally the whole "about" thing is a less flexible version 
of what this is trying to do.

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

Just ejecting about* and replace with render will do it and keep the 
number of implementations related to this at 1.

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

It doesn't sound too hard but when would I use it?  Is it a case where 
rendering a list of matches would solve it?

-Andy

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