From mboxrd@z Thu Jan  1 00:00:00 1970
From: john at keeping.me.uk (John Keeping)
Date: Tue, 3 Jul 2018 20:53:07 +0100
Subject: [PATCH v4 00/16] Render READMEs inline in tree view
In-Reply-To: <CAHmME9ocGHV+-1QyKLSTDSN6YHvy13kVoGXZ0O39JS+0v8G7Kw@mail.gmail.com>
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>
 <CAHmME9oG4612SsfmpYQ-Ptd=0WK=QX-s1A47u1SLz0ZXX364Jw@mail.gmail.com>
 <20180628082853.GT6584@john.keeping.me.uk>
 <CAHmME9ocGHV+-1QyKLSTDSN6YHvy13kVoGXZ0O39JS+0v8G7Kw@mail.gmail.com>
Message-ID: <20180703195307.GY6584@john.keeping.me.uk>

On Tue, Jul 03, 2018 at 09:34:26PM +0200, Jason A. Donenfeld wrote:
> On Thu, Jun 28, 2018 at 10:29 AM John Keeping <john at keeping.me.uk> wrote:
> > 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.
> 
> Let's do it this way then. Since rendering anyway usually amounts to:
> 
> case "$(printf '%s' "$1" | tr '[:upper:]' '[:lower:]')" in
>         *.markdown|*.mdown|*.md|*.mkd) exec ./md2html; ;;
>         *.rst) exec ./rst2html; ;;
>         *.[1-9]) exec ./man2html; ;;
>         *.htm|*.html) exec cat; ;;
>         *.txt|*) exec ./txt2html; ;;
> esac
> 
> Then we can just reimplement the dispatcher in Lua, and this whole
> issue goes away.

That sounds sensible, although the dispatcher will be getting a bit more
complicated to support the file formats for which we want to output an
<img> or <iframe> tag.

> > 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.
> 
> Right.
> 
> > 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.
> 
> I saw that. And indeed that seems like a good way of doing it.
> 
> >
> > > - 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?
> 
> Yes.
> 
> > Yes, that looks good to me, modulo figuring out exactly how
> > render-filter operates.
> 
> render-filter will operate the same way as the current about-filter,
> with two exceptions:
> 
> - If it returns error 127, then it means rendering wasn't supported
> and we should fall back to source view (in the case of it being loaded
> from /tree).
> - It is passed a full path to the raw files, so that it can generate
> correct relative includes.
> 
> I think with that settled (if you agree), the previous series can be
> reworked to function in this manner? The full plan is looking like:
> 
> - We retain commit-filter, source-filter, and owner-filter as we have them now.
> - We rename about-filter to render-filter.
> - render-filter gets passed a fuller filename with useful path
> information for printing out iframes, img tags, and so forth.
> - render-filter returns 127 if it does not have a renderer for that file.
> - Viewing files in /tree defaults to render-filter. If that returns
> 127, it falls back to source-filter.
> - Viewing files in /source goes straight to source-filter and skips
> render-filter.
> - When render-filter is used in /tree, cgit shows in the top bar a
> helpful link to go to /source.
> - 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.

Yes, this sounds good to me.

I think backwards compatibility also falls out quite easily in this
approach:

- "about-filter" becomes a deprecated alias for "render-filter"
- "readme" becomes a deprecated alias for "about-readme"

If I can have one more bikeshed... I wonder if "about-content" is better
than "about-readme", the latter feels a bit like we're saying this same
thing twice.