List for cgit developers and users
 help / color / mirror / Atom feed
* repo.desc as raw html?
@ 2018-06-20  9:04 andy
  2018-06-20 13:17 ` [PATCH] noheader: place branch combo on tabs if no header andy
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: andy @ 2018-06-20  9:04 UTC (permalink / raw)


Hi -

I want to put a repo-specific link to my project mailing list in the 
header area.

But there doesn't seem to be a way to do it.

noheader=1 also removes functionality for branch switching UI, which is 
not desirable.

There's a "homepage" link concept but a) that's positioned in the wrong 
place logically, it's in the vertical context of the repo display area 
not the header, and b) just adding a few more inflexible individual 
settings doesn't sound a good way.

What's the feeling about just changing ui-shared.c where it prints it from

html_txt(ctx.repo->desc);

to

html(ctx.repo->desc);

?  As html I can put what I want in there... homepage and mailing list 
links, text, inline images etc, and because it's in the header, it 
doesn't need to know any detail about what's going on in the repo view.

-Andy


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] noheader: place branch combo on tabs if no header
  2018-06-20  9:04 repo.desc as raw html? andy
@ 2018-06-20 13:17 ` andy
  2018-06-23 10:50   ` john
  2018-06-21  6:46 ` [PATCH 1/2] config: add root-desc-html and repo.desc-html andy
  2018-06-21  6:46 ` [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts andy
  2 siblings, 1 reply; 10+ messages in thread
From: andy @ 2018-06-20 13:17 UTC (permalink / raw)


noheader=1 stops the static page header from being emitted by
cgit, but along with that, the small form that lets the user
change branch context on the page is also lost.

This isn't actually static since it contains a dynamic list of
branches; it can't be reproduced on the user's external header
static html.  So it seems it doesn't belong to the set of header
things that should be disabled.

This patch relocates the branch selection combo and
form on the left of the tabs line if noheader=1.  It doesn't
change anything if noheader is not set.

Signed-off-by: Andy Green <andy at warmcat.com>
---
 ui-shared.c |   34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index c9a34fb..082a6f1 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -958,6 +958,19 @@ static void cgit_print_path_crumbs(char *path)
 	ctx.qry.path = old_path;
 }
 
+static void print_branch_combo_form(void)
+{
+	html("<form method='get'>\n");
+	cgit_add_hidden_formfields(0, 1, ctx.qry.page);
+	html("<select name='h' onchange='this.form.submit();'>\n");
+	for_each_branch_ref(print_branch_option, ctx.qry.head);
+	if (ctx.repo->enable_remote_branches)
+		for_each_remote_ref(print_branch_option, ctx.qry.head);
+	html("</select> ");
+	html("<input type='submit' value='switch'/>");
+	html("</form>");
+}
+
 static void print_header(void)
 {
 	char *logo = NULL, *logo_link = NULL;
@@ -991,15 +1004,7 @@ static void print_header(void)
 		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
 		if (ctx.env.authenticated) {
 			html("</td><td class='form'>");
-			html("<form method='get'>\n");
-			cgit_add_hidden_formfields(0, 1, ctx.qry.page);
-			html("<select name='h' onchange='this.form.submit();'>\n");
-			for_each_branch_ref(print_branch_option, ctx.qry.head);
-			if (ctx.repo->enable_remote_branches)
-				for_each_remote_ref(print_branch_option, ctx.qry.head);
-			html("</select> ");
-			html("<input type='submit' value='switch'/>");
-			html("</form>");
+			print_branch_combo_form();
 		}
 	} else
 		html_txt(ctx.cfg.root_title);
@@ -1023,8 +1028,15 @@ void cgit_print_pageheader(void)
 	if (!ctx.env.authenticated || !ctx.cfg.noheader)
 		print_header();
 
-	html("<table class='tabs'><tr><td>\n");
+	html("<table class='tabs'><tr>\n");
 	if (ctx.env.authenticated && ctx.repo) {
+		if (ctx.cfg.noheader) {
+			html("<td class='form' style='text-align:left'>");
+			print_branch_combo_form();
+			html("</td><td style='text-align:center'>");
+		}
+		html("<td>");
+
 		if (ctx.repo->readme.nr)
 			reporevlink("about", "about", NULL,
 				    hc("about"), ctx.qry.head, NULL,
@@ -1081,6 +1093,8 @@ void cgit_print_pageheader(void)
 		html("</form>\n");
 	} else if (ctx.env.authenticated) {
 		char *currenturl = cgit_currenturl();
+
+		html("<td>");
 		site_link(NULL, "index", NULL, hc("repolist"), NULL, NULL, 0, 1);
 		if (ctx.cfg.root_readme)
 			site_link("about", "about", NULL, hc("about"),



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] config: add root-desc-html and repo.desc-html
  2018-06-20  9:04 repo.desc as raw html? andy
  2018-06-20 13:17 ` [PATCH] noheader: place branch combo on tabs if no header andy
@ 2018-06-21  6:46 ` andy
  2018-06-21  6:46 ` [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts andy
  2 siblings, 0 replies; 10+ messages in thread
From: andy @ 2018-06-21  6:46 UTC (permalink / raw)


These are optional, default-empty raw html strings that
are emitted after the corresponding text-only versions
root-desc and repo.desc when they are used in the header
area.

This provides a flexible way to place buttons or links
in the header region for both the repo index page and
for repos individually.

The existing root-desc and repo.desc keep their original
meaning as a shortish text-only repo description.

Signed-off-by: Andy Green <andy at warmcat.com>
---
 cgit.c       |    4 ++++
 cgit.h       |    2 ++
 cgitrc.5.txt |    8 ++++++++
 3 files changed, 14 insertions(+)

diff --git a/cgit.c b/cgit.c
index bdb2fad..1b819e7 100644
--- a/cgit.c
+++ b/cgit.c
@@ -52,6 +52,8 @@ static void repo_config(struct cgit_repo *repo, const char *name, const char *va
 		repo->clone_url = xstrdup(value);
 	else if (!strcmp(name, "desc"))
 		repo->desc = xstrdup(value);
+	else if (!strcmp(name, "desc-html"))
+		repo->desc_html = xstrdup(value);
 	else if (!strcmp(name, "owner"))
 		repo->owner = xstrdup(value);
 	else if (!strcmp(name, "homepage"))
@@ -142,6 +144,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
 		ctx.cfg.root_desc = xstrdup(value);
+	else if (!strcmp(name, "root-desc-html"))
+		ctx.cfg.root_desc_html = xstrdup(value);
 	else if (!strcmp(name, "root-readme"))
 		ctx.cfg.root_readme = xstrdup(value);
 	else if (!strcmp(name, "css"))
diff --git a/cgit.h b/cgit.h
index 99ea7a2..1094062 100644
--- a/cgit.h
+++ b/cgit.h
@@ -81,6 +81,7 @@ struct cgit_repo {
 	char *name;
 	char *path;
 	char *desc;
+	char *desc_html;
 	char *owner;
 	char *homepage;
 	char *defbranch;
@@ -207,6 +208,7 @@ struct cgit_config {
 	char *robots;
 	char *root_title;
 	char *root_desc;
+	char *root_desc_html;
 	char *root_readme;
 	char *script_name;
 	char *section;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 99fc799..4ddb51e 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -378,6 +378,10 @@ root-desc::
 	Text printed below the heading on the repository index page. Default
 	value: "a fast webinterface for the git dscm".
 
+root-desc-html::
+	Optional additional raw html to show in the header after root-desc.
+	Default value: none.
+
 root-readme::
 	The content of the file specified with this option will be included
 	verbatim below the "about" link on the repository index page. Default
@@ -503,6 +507,10 @@ repo.defbranch::
 repo.desc::
 	The value to show as repository description. Default value: none.
 
+repo.desc-html::
+	Optional additional raw html to show in the header after repo.desc.
+	Default value: none.
+
 repo.email-filter::
 	Override the default email-filter. Default value: none. See also:
 	"enable-filter-overrides". See also: "FILTER API".



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts
  2018-06-20  9:04 repo.desc as raw html? andy
  2018-06-20 13:17 ` [PATCH] noheader: place branch combo on tabs if no header andy
  2018-06-21  6:46 ` [PATCH 1/2] config: add root-desc-html and repo.desc-html andy
@ 2018-06-21  6:46 ` andy
  2018-06-23 10:28   ` john
  2 siblings, 1 reply; 10+ messages in thread
From: andy @ 2018-06-21  6:46 UTC (permalink / raw)


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>
---
 ui-shared.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ui-shared.c b/ui-shared.c
index c8f4d8f..a9ec430 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1013,11 +1013,15 @@ static void print_header(void)
 	html("<tr><td class='sub'>");
 	if (ctx.repo) {
 		html_txt(ctx.repo->desc);
+		if (ctx.repo->desc_html)
+			html(ctx.repo->desc_html);
 		html("</td><td class='sub right'>");
 		html_txt(ctx.repo->owner);
 	} else {
 		if (ctx.cfg.root_desc)
 			html_txt(ctx.cfg.root_desc);
+		if (ctx.cfg.root_desc_html)
+			html(ctx.cfg.root_desc_html);
 	}
 	html("</td></tr></table>\n");
 }



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts
  2018-06-21  6:46 ` [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts andy
@ 2018-06-23 10:28   ` john
  2018-06-23 10:33     ` andy
  0 siblings, 1 reply; 10+ messages in thread
From: john @ 2018-06-23 10:28 UTC (permalink / raw)


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.

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!

>  ui-shared.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index c8f4d8f..a9ec430 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -1013,11 +1013,15 @@ static void print_header(void)
>  	html("<tr><td class='sub'>");
>  	if (ctx.repo) {
>  		html_txt(ctx.repo->desc);
> +		if (ctx.repo->desc_html)
> +			html(ctx.repo->desc_html);
>  		html("</td><td class='sub right'>");
>  		html_txt(ctx.repo->owner);
>  	} else {
>  		if (ctx.cfg.root_desc)
>  			html_txt(ctx.cfg.root_desc);
> +		if (ctx.cfg.root_desc_html)
> +			html(ctx.cfg.root_desc_html);
>  	}
>  	html("</td></tr></table>\n");
>  }


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts
  2018-06-23 10:28   ` john
@ 2018-06-23 10:33     ` andy
  2018-06-23 10:53       ` john
  0 siblings, 1 reply; 10+ messages in thread
From: andy @ 2018-06-23 10:33 UTC (permalink / raw)




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

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

-Andy


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] noheader: place branch combo on tabs if no header
  2018-06-20 13:17 ` [PATCH] noheader: place branch combo on tabs if no header andy
@ 2018-06-23 10:50   ` john
  0 siblings, 0 replies; 10+ messages in thread
From: john @ 2018-06-23 10:50 UTC (permalink / raw)


On Wed, Jun 20, 2018 at 09:17:00PM +0800, Andy Green wrote:
> noheader=1 stops the static page header from being emitted by
> cgit, but along with that, the small form that lets the user
> change branch context on the page is also lost.
> 
> This isn't actually static since it contains a dynamic list of
> branches; it can't be reproduced on the user's external header
> static html.  So it seems it doesn't belong to the set of header
> things that should be disabled.
> 
> This patch relocates the branch selection combo and
> form on the left of the tabs line if noheader=1.  It doesn't
> change anything if noheader is not set.
> 
> Signed-off-by: Andy Green <andy at warmcat.com>

This makes noheader=1 a lot more usable!

I wonder if the branch combo should be somewhere to the right of the
main tabs, but I don't feel strongly either way.

Reviewed-by: John Keeping <john at keeping.me.uk>

> ---
>  ui-shared.c |   34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index c9a34fb..082a6f1 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -958,6 +958,19 @@ static void cgit_print_path_crumbs(char *path)
>  	ctx.qry.path = old_path;
>  }
>  
> +static void print_branch_combo_form(void)
> +{
> +	html("<form method='get'>\n");
> +	cgit_add_hidden_formfields(0, 1, ctx.qry.page);
> +	html("<select name='h' onchange='this.form.submit();'>\n");
> +	for_each_branch_ref(print_branch_option, ctx.qry.head);
> +	if (ctx.repo->enable_remote_branches)
> +		for_each_remote_ref(print_branch_option, ctx.qry.head);
> +	html("</select> ");
> +	html("<input type='submit' value='switch'/>");
> +	html("</form>");
> +}
> +
>  static void print_header(void)
>  {
>  	char *logo = NULL, *logo_link = NULL;
> @@ -991,15 +1004,7 @@ static void print_header(void)
>  		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
>  		if (ctx.env.authenticated) {
>  			html("</td><td class='form'>");
> -			html("<form method='get'>\n");
> -			cgit_add_hidden_formfields(0, 1, ctx.qry.page);
> -			html("<select name='h' onchange='this.form.submit();'>\n");
> -			for_each_branch_ref(print_branch_option, ctx.qry.head);
> -			if (ctx.repo->enable_remote_branches)
> -				for_each_remote_ref(print_branch_option, ctx.qry.head);
> -			html("</select> ");
> -			html("<input type='submit' value='switch'/>");
> -			html("</form>");
> +			print_branch_combo_form();
>  		}
>  	} else
>  		html_txt(ctx.cfg.root_title);
> @@ -1023,8 +1028,15 @@ void cgit_print_pageheader(void)
>  	if (!ctx.env.authenticated || !ctx.cfg.noheader)
>  		print_header();
>  
> -	html("<table class='tabs'><tr><td>\n");
> +	html("<table class='tabs'><tr>\n");
>  	if (ctx.env.authenticated && ctx.repo) {
> +		if (ctx.cfg.noheader) {
> +			html("<td class='form' style='text-align:left'>");
> +			print_branch_combo_form();
> +			html("</td><td style='text-align:center'>");
> +		}
> +		html("<td>");
> +
>  		if (ctx.repo->readme.nr)
>  			reporevlink("about", "about", NULL,
>  				    hc("about"), ctx.qry.head, NULL,
> @@ -1081,6 +1093,8 @@ void cgit_print_pageheader(void)
>  		html("</form>\n");
>  	} else if (ctx.env.authenticated) {
>  		char *currenturl = cgit_currenturl();
> +
> +		html("<td>");
>  		site_link(NULL, "index", NULL, hc("repolist"), NULL, NULL, 0, 1);
>  		if (ctx.cfg.root_readme)
>  			site_link("about", "about", NULL, hc("about"),


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts
  2018-06-23 10:33     ` andy
@ 2018-06-23 10:53       ` john
  2018-06-23 11:08         ` andy
  0 siblings, 1 reply; 10+ messages in thread
From: john @ 2018-06-23 10:53 UTC (permalink / raw)


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.

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts
  2018-06-23 10:53       ` john
@ 2018-06-23 11:08         ` andy
  2018-06-23 16:33           ` john
  0 siblings, 1 reply; 10+ messages in thread
From: andy @ 2018-06-23 11:08 UTC (permalink / raw)




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?

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

-Andy


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts
  2018-06-23 11:08         ` andy
@ 2018-06-23 16:33           ` john
  0 siblings, 0 replies; 10+ messages in thread
From: john @ 2018-06-23 16:33 UTC (permalink / raw)


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-06-23 16:33 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-20  9:04 repo.desc as raw html? andy
2018-06-20 13:17 ` [PATCH] noheader: place branch combo on tabs if no header andy
2018-06-23 10:50   ` john
2018-06-21  6:46 ` [PATCH 1/2] config: add root-desc-html and repo.desc-html andy
2018-06-21  6:46 ` [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts andy
2018-06-23 10:28   ` john
2018-06-23 10:33     ` andy
2018-06-23 10:53       ` john
2018-06-23 11:08         ` andy
2018-06-23 16:33           ` john

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).