[PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts

[John Keeping]

On Sat, Jun 23, 2018 at 07:08:08PM +0800, Andy Green wrote:
> 
> 
> On 06/23/2018 06:53 PM, John Keeping wrote:
> > On Sat, Jun 23, 2018 at 06:33:38PM +0800, Andy Green wrote:
> >>
> >>
> >> On 06/23/2018 06:28 PM, John Keeping wrote:
> >>> On Thu, Jun 21, 2018 at 02:46:47PM +0800, Andy Green wrote:
> >>>> Where root-desc and repo.desc are used in the header region, also
> >>>> emit their html counterparts afterwards if they are defined.
> >>>>
> >>>> Where root-desc are repo.desc are used outside the header,
> >>>> eg in the repo list, leave it as it is without adding any
> >>>> related html.
> >>>>
> >>>> Signed-off-by: Andy Green <andy at warmcat.com>
> >>>> ---
> >>>
> >>> I think this should be squashed with the previous patch since it makes
> >>> it easier to see what's going on.
> >>>
> >>> When I read your initial email on this, I thought we could introduce a
> >>> new HTML version of the description and use that *instead of* the plain
> >>> text one if the HTML variant is available.
> >>
> >> I actually first implemented just rendering what we have as raw html...
> > 
> > I don't think we can do that without introducing an HTML injection risk
> > in configurations that are currently safe.
> 
> If someone else controls the repo.desc, yes it's not nice.
> 
> Isn't it the same problem if someone else controls the repo.desc_html?

No, because repo.desc_html will always be treated as HTML.

If we change the behaviour of CGit for the _existing_ repo.desc then a
configuration which is currently serving plain text will suddenly start
interpreting that as HTML.  Even if we make it very clear in the release
notes that this is happening, it's not great for a simple software
update to change this.

> >>> Having looked at the current implementation of repo->desc, I think
> >>> that's desirable because the reason we don't have a null-check for that
> >>> in the context below is that it will be set to "[no description]" if no
> >>> other value is provided.  If a user has set repo->desc_html, I don't
> >>> think we want to print "[no description]" before showing the HTML
> >>> description!
> >>
> >> I take the point, but it turned out there are two separate kinds of
> >> description here... the text-only, existing one that is used, eg, in the
> >> list of repos.  And a "functional" HTML part that has buttons or
> >> whatever specific to the repo and used on the header part.
> >>
> >> With just treating them as one, the repo list gained meaningless HTML
> >> buttons or pictures or whatever decoration was put there.  The repo list
> >> just wants a short textual description that already exists.  So it
> >> arrived at this, leave that be, and add an optional HTML decoration part.
> > 
> > OK, that makes sense.  Maybe we need the following check, but it is
> > quite ugly!
> > 
> > 	if (ctx.repo->desc &&
> > 	    (ctx.repo->desc != cgit_default_repo_desc ||
> > 	     !ctx.repo->html_desc))
> > 
> > that is, show the plain text description only if it has been customised
> > or if there is no HTML description.
> 
> I'm used to looking in the mirror, so it's OK for me :-)
> 
> It could also be done as a filter script that purifies strings given to 
> it and wraps them in canned html.
> 
> Basically the cgit repo probably isn't existing on its own.  It's part 
> of a project that has links relevant to someone who, eg, stumbled on the 
> cgit repo.  In cgit's own case, the cgit header has the author name but 
> no link to its mailing list... that's not right actually.  So somehow 
> there should be a way to integrate the cgit url with other urls strongly 
> likely to be of interest to someone who is interested in the cgit.

I had a look at libwebsockets.org and the additional links look really
nice.

I think the implementation here is fine (maybe with the explanation of
"two kinds of description" in the commit message), but I can see some
people wanting to disable the plain text description and use the HTML
one for everything.  To make that work, we need to use the ugly if
condition above so that cgit_default_repo_desc is hidden (even though
it's ugly, it works and with your explanation above I agree that the
simple if(repo_desc_html) else if(repo_desc) version isn't sufficient).