Re: [PATCH v4 00/16] Render READMEs inline in tree view
On Tue, Jul 3, 2018 at 9:53 PM John Keeping wrote: > 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. Bikeshed granted. :) Yes, that sounds sensible. ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
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 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 or 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. ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
Hey John, On Thu, Jun 28, 2018 at 10:29 AM John Keeping 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'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. ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
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 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 `=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
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 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 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
Re: [PATCH v4 00/16] Render READMEs inline in tree view
Hey John, Thanks tons for your input, as always. On Wed, Jun 27, 2018 at 9:51 PM John Keeping 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 `=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. > > - 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 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 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. - When source-filter is used, cgit prints its usual line numbers. - If source-filter fails, we print our nice hexdump or plaintext printer. - 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? Jason ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
On 06/28/2018 06:46 AM, Jason A. Donenfeld wrote: Hi Andy, I'm happy to engage technically here in order to find the best way of going about this. However, Nobody is paying me to do it and I don't have an endless budget of time to lavish on it (and it seems, neither do you...). If cgit can't do what I need in a reasonable timescale, even with my contribution, my choices are: been shaped only by github. It's not that hard to overstate its importance but in a few ways, unless something in cgit is really better, the way forward is to align, or align and improve... IMHO. I don't see the relevance of whether or not you're paid and how much time you have to implement things, or what predictions are about sink or swim with regards to Github. Mmm I could tell from your reply it's irrelevant to you. That's why I pointed it out, because it's very relevant to me and will inform what I choose to do. I think the general ideas you've presented in these patchsets are good ones, and now it's time to collaborate on the best way to go about doing it. Part of what we're encountering here is the fact that cgit hasn't originally had a robust display/hosting intent; it was mostly around showing the state of the git repository. Now we're trying to grow beyond that. Hence, I'd like to organize this in a sensible way so that such rendering capabilities are available at the heart of cgit, rather than haphazardly added on with a smattering of It's not that I don't understand the reasoning. There is a lot of cruft because the project has been around a long while. But it's not my cruft. contradictory configuration switches. In spite of your pay grade, your time budget, or your timescale, I'm going to make sure this gets done in the right way. I also run a FOSS project that has been around for years and have dealt with hundreds of contributions. These features have been around for 10 years in github and still not in cgit, done right or wrong. I think everyone will be better off if we can find a way. -Andy Jason ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
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. - 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." 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. 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 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=:` 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.; 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 case and the "render not supported" case if we want to do those outside the filter. [1] This could be the exit status
Re: [PATCH v4 00/16] Render READMEs inline in tree view
Hi Andy, I'm happy to engage technically here in order to find the best way of going about this. However, > Nobody is paying me to do it and I don't have an endless budget of time > to lavish on it (and it seems, neither do you...). > If cgit can't do what I need in a reasonable timescale, even with my > contribution, my choices are: > been shaped only by github. It's not that hard to overstate its > importance but in a few ways, unless something in cgit is really better, > the way forward is to align, or align and improve... IMHO. I don't see the relevance of whether or not you're paid and how much time you have to implement things, or what predictions are about sink or swim with regards to Github. I think the general ideas you've presented in these patchsets are good ones, and now it's time to collaborate on the best way to go about doing it. Part of what we're encountering here is the fact that cgit hasn't originally had a robust display/hosting intent; it was mostly around showing the state of the git repository. Now we're trying to grow beyond that. Hence, I'd like to organize this in a sensible way so that such rendering capabilities are available at the heart of cgit, rather than haphazardly added on with a smattering of contradictory configuration switches. In spite of your pay grade, your time budget, or your timescale, I'm going to make sure this gets done in the right way. Jason ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
On 06/28/2018 01:18 AM, Jason A. Donenfeld wrote: Hey Andy, Thanks for this patchset. It looks like this is shaping up into a nice direction. However, I'm a bit concerned about our nobs becoming slightly overlapping and incoherent, and I think that with this series, we should also unify how we handle rendering. Well, I can understand that from your POV. However I also have a POV... "This series" represents my attempt to align cgit to a piece of github that my users will definitely miss. Nobody is paying me to do it and I don't have an endless budget of time to lavish on it (and it seems, neither do you...). If cgit can't do what I need in a reasonable timescale, even with my contribution, my choices are: - become a cgit developer until the happy day comes - maintaining my own OOT branch until it can natively do the same thing - use something else With the current state of this series, cgit would have the following options: - render. - inline-readme - render-filter - 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. Yeah, about has lost all meaning if this series gets in. 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): - We retain commit-filter, source-filter, and owner-filter as we have them now. - We rename about-filter to readme-filter. - 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?] ... what's actually wrong with aligning with The Github Way? - We introduce an options at the global level and at the .repo level of `about-readme=/path/to/absolute/file` and `about-readme= > and `about-readme=:`. The first would replace our original usage of How "about" just drop about-*, and the about tab, ui-about-* etc. Set the readme names using both [repo.]about-readme= for compatibility and [repo.]readme= Almost all projects have a README of some kind at the root of their source tree. It was a standard layout for decades. So this just replaces about. And aligns with github. `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. ... and don't do that, just show the inline README for the rev context. - 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. ... just always display any compatible readme a la github. - We do not introduce render-filter. We do not introduce render.; such extension selection is successfully handled by the various filters themselves already. That I don't know much about. However, we can at least get rid of or repurpose about filter for this, if about if going away. The copyright message says cgit has been around 22 years... since then programmers have been born and since the age of 12 their view of git has been shaped only by github. It's not that hard to overstate its importance but in a few ways, unless something in cgit is really better, the way forward is to align, or align and improve... IMHO. -Andy John, Christian -- what are your thoughts on this? Regards, Jason ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: Fancier Source view [Was: Re: [PATCH v4 00/16] Render READMEs inline in tree view]
On Wed, Jun 27, 2018 at 07:26:13PM +0200, Jason A. Donenfeld wrote: > Splitting out this issue into a different thread, because I think it's > orthogonal to the other topic. > > On Wed, Jun 27, 2018 at 7:18 PM Jason A. Donenfeld wrote: > > 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. > > People click a .c file and they get pygments highlighting. That's > good. People click a .pdf or a .md and they get either a hexdump or a > highlighted .md source. Github has made different behavior more > popular -- namely, rendering the .pdf and the .md. The obvious way to > handle this is augmenting the source-view to spit something different > out. > > Alternatively, my previously proposed readme-filter could be renamed > to render-filter, and we could do everything in one place with one > filter, and decide exclusively on the basis of filetype. > > Either way, we have this issue of line numbers. We have a few ways of > handling this: > > a. Not print them ever when a source-filter/render-filter is > specified, leaving that up to the renderer to do. > b. If the renderer doesn't want line numbers, it can return a special > exit code (197, for example) to indicate that to cgit. > c. If the renderer doesn't want line numbers, it can just omit some > !important css to remove them at the end. > d. We introduce a maze of filetype selection rendering logic options. > e. Something else? > > I'm most inclined to go with a or b, but neither seems entirely > appealing. Thoughts? d. is basically what we have with "render." in this series, but I think e. might be better. Specifically, I think we shouldn't conflate source and rendered views because syntax highlighted Markdown, or SVG, or HTML is still potentially useful. If source and rendered views are distinct, then source view always has line numbers (or it's a hex dump) and we just need the filter to return a special exit status to mean "this file is unsupported", and we disable the rendered view when that happens. As an extension, we could disable the source view if rendering is supported and the file looks like binary, so that the hex dump isn't available when we have a better option. John ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
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. > - 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." 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. 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 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=:` > 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.; > 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 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 ___ CGit mailing list CGit@lists.zx2c4.com
Fancier Source view [Was: Re: [PATCH v4 00/16] Render READMEs inline in tree view]
Splitting out this issue into a different thread, because I think it's orthogonal to the other topic. On Wed, Jun 27, 2018 at 7:18 PM Jason A. Donenfeld wrote: > 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. People click a .c file and they get pygments highlighting. That's good. People click a .pdf or a .md and they get either a hexdump or a highlighted .md source. Github has made different behavior more popular -- namely, rendering the .pdf and the .md. The obvious way to handle this is augmenting the source-view to spit something different out. Alternatively, my previously proposed readme-filter could be renamed to render-filter, and we could do everything in one place with one filter, and decide exclusively on the basis of filetype. Either way, we have this issue of line numbers. We have a few ways of handling this: a. Not print them ever when a source-filter/render-filter is specified, leaving that up to the renderer to do. b. If the renderer doesn't want line numbers, it can return a special exit code (197, for example) to indicate that to cgit. c. If the renderer doesn't want line numbers, it can just omit some !important css to remove them at the end. d. We introduce a maze of filetype selection rendering logic options. e. Something else? I'm most inclined to go with a or b, but neither seems entirely appealing. Thoughts? Jason ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
Hey Andy, Thanks for this patchset. It looks like this is shaping up into a nice direction. However, I'm a bit concerned about our nobs becoming slightly overlapping and incoherent, and I think that with this series, we should also unify how we handle rendering. With the current state of this series, cgit would have the following options: - render. - inline-readme - render-filter - 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): - We retain commit-filter, source-filter, and owner-filter as we have them now. - We rename about-filter to readme-filter. - 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=:` 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. - 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. - We do not introduce render-filter. We do not introduce render.; such extension selection is successfully handled by the various filters themselves already. John, Christian -- what are your thoughts on this? Regards, Jason ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
On 06/23/2018 07:04 PM, John Keeping wrote: On Wed, Jun 20, 2018 at 06:11:57PM +0800, Andy Green wrote: The following series adds config to allow rendering of selected READMEs inline after the tree view, where present in the directory being viewed. Particularly you can use completely relative markdown to inline pictures served from the current repo rev context, eg,  will "just work" showing the png from the current view rev context; this format also works in github. It builds on John Keeping's RENDER mode series from 2016. Typical config to enable it, if you have a README.md looks like inline-readme=README.md render.md=/usr/libexec/cgit/filters/html-converters/md2html You can see examples of it in operation at https://libwebsockets.org/git/libwebsockets/tree https://libwebsockets.org/git/libwebsockets/tree/?h=v3.0-stable https://warmcat.com/git/cgit/tree/ The expected basis these apply on top of is - jk/for-jason - ch/for-jason You can find these patches on top of the expected basis here https://warmcat.com/git/cgit/log/ v4 collects all the reviewed by and implements yesterday's comments Other than the left-over debugging line you've already pointed out yourself, this looks good to me. I suggest you post a final version after the Git 2.18.0 update hits master and we can get this merged. Great, will do. -Andy ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
Re: [PATCH v4 00/16] Render READMEs inline in tree view
On Wed, Jun 20, 2018 at 06:11:57PM +0800, Andy Green wrote: > The following series adds config to allow rendering of > selected READMEs inline after the tree view, where > present in the directory being viewed. > > Particularly you can use completely relative markdown to > inline pictures served from the current repo rev context, > eg, > >  > > will "just work" showing the png from the current view > rev context; this format also works in github. > > It builds on John Keeping's RENDER mode series from 2016. > > Typical config to enable it, if you have a README.md > looks like > > inline-readme=README.md > render.md=/usr/libexec/cgit/filters/html-converters/md2html > > You can see examples of it in operation at > > https://libwebsockets.org/git/libwebsockets/tree > https://libwebsockets.org/git/libwebsockets/tree/?h=v3.0-stable > https://warmcat.com/git/cgit/tree/ > > The expected basis these apply on top of is > > - jk/for-jason > - ch/for-jason > > You can find these patches on top of the expected basis here > > https://warmcat.com/git/cgit/log/ > > v4 collects all the reviewed by and implements yesterday's comments Other than the left-over debugging line you've already pointed out yourself, this looks good to me. I suggest you post a final version after the Git 2.18.0 update hits master and we can get this merged. > --- > > Andy Green (10): > manpage: fix sorting order > ui-tree: ls_tail: add walk table param > config: add global inline-readme list > config: add repo inline-readme list > ui-tree: render any matching README file in tree view > md2html: add asset postfix arg > ui-shared: deduplicate some code in repolink > ui-shared: add helper for generating non-urlencoded links > render: adapt for providing extra filter args for plain > md2html: change css name to not conflict with highlight > > John Keeping (6): > Use string list strdup_strings for mimetypes > Add source page > Parse render filters from the config > ui-tree: split out buffer printing > ui-tree: use render filters to display content > md2html: add asset mapping > > > cgit.c | 35 +- > cgit.css|5 + > cgit.h |6 + > cgitrc.5.txt| 221 > +++ > cmd.c |8 + > filter.c|4 + > filters/html-converters/md2html | 61 ++- > shared.c| 21 > ui-shared.c | 72 ++--- > ui-shared.h |6 + > ui-tree.c | 193 +++--- > ui-tree.h |2 > 12 files changed, 498 insertions(+), 136 deletions(-) > > -- > Signature > ___ > CGit mailing list > CGit@lists.zx2c4.com > https://lists.zx2c4.com/mailman/listinfo/cgit ___ CGit mailing list CGit@lists.zx2c4.com https://lists.zx2c4.com/mailman/listinfo/cgit
