[PATCH v5 1/1] ui-shared: allow to split the repository link

[PATCH v5 1/1] ui-shared: allow to split the repository link

[petr.vorel]

Teach cgit split the repository link in the top of repository "summary"
view. This emulates the same behaviour as it's in gitweb.

This behaviour is not implemented for repositories which have
"repo.name" set different than "repo.url".

This feature is controlled by a new config variable:
"split-summary-repo-link" (disabled by default).

Signed-off-by: Petr Vorel <petr.vorel at gmail.com>
---
v5: Rename config variable to split-summary-repo-link
v4: Implement suggestions from John Keeping (thanks for them :-)):
* use strchr() instead of strtok_r() and variadic arrays
* use cgit_repourl() and html_*() instead of cgit_summary_link().
* add comment why we compare page.url and page.name
v3: New config variable "summary-enable-split-repo-link", minor cleanup.
v2: Minor cleanup.
---
 cgit.c         |  3 +++
 cgit.h         |  1 +
 cgitrc.5.txt   |  6 ++++++
 tests/setup.sh |  1 +
 ui-shared.c    | 29 ++++++++++++++++++++++++++++-
 5 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/cgit.c b/cgit.c
index fc482be..a133226 100644
--- a/cgit.c
+++ b/cgit.c
@@ -246,6 +246,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.section_sort = atoi(value);
 	else if (!strcmp(name, "source-filter"))
 		ctx.cfg.source_filter = cgit_new_filter(value, SOURCE);
+	else if (!strcmp(name, "split-summary-repo-link"))
+		ctx.cfg.split_summary_repo_link = atoi(value);
 	else if (!strcmp(name, "summary-log"))
 		ctx.cfg.summary_log = atoi(value);
 	else if (!strcmp(name, "summary-branches"))
@@ -388,6 +390,7 @@ static void prepare_context(void)
 	ctx.cfg.section = "";
 	ctx.cfg.repository_sort = "name";
 	ctx.cfg.section_sort = 1;
+	ctx.cfg.split_summary_repo_link = 0;
 	ctx.cfg.summary_branches = 10;
 	ctx.cfg.summary_log = 10;
 	ctx.cfg.summary_tags = 10;
diff --git a/cgit.h b/cgit.h
index 325432b..bfd1dfb 100644
--- a/cgit.h
+++ b/cgit.h
@@ -254,6 +254,7 @@ struct cgit_config {
 	int section_from_path;
 	int snapshots;
 	int section_sort;
+	int split_summary_repo_link;
 	int summary_branches;
 	int summary_log;
 	int summary_tags;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 2e1912d..a84fc10 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -438,6 +438,12 @@ summary-branches::
 	Specifies the number of branches to display in the repository "summary"
 	view. Default value: "10".
 
+split-summary-repo-link::
+	Flag which, when set to "1", will make cgit split the repository link in
+	the top of repository "summary" view. This emulates the same behaviour as
+	it's in gitweb. This behaviour is not implemented for repositories which
+	have "repo.name" set different than "repo.url". Default value: "0".
+
 summary-log::
 	Specifies the number of log entries to display in the repository
 	"summary" view. Default value: "10".
diff --git a/tests/setup.sh b/tests/setup.sh
index 7590f04..4f445dc 100755
--- a/tests/setup.sh
+++ b/tests/setup.sh
@@ -109,6 +109,7 @@ enable-log-filecount=1
 enable-log-linecount=1
 summary-log=5
 summary-branches=5
+split-summary-repo-link=0
 summary-tags=5
 clone-url=git://example.org/\$CGIT_REPO_URL.git
 enable-filter-overrides=1
diff --git a/ui-shared.c b/ui-shared.c
index 9a38aa9..e86ea58 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -937,7 +937,34 @@ static void print_header(void)
 	if (ctx.repo) {
 		cgit_index_link("index", NULL, NULL, NULL, NULL, 0, 1);
 		html(" : ");
-		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
+
+		/*
+		 * NOTE: If repo.name and repo.url are different, we ignore
+		 * split_summary_repo_link flag and don't split link as it
+		 * wouldn't make sense to split the path.
+		 * */
+		if (ctx.cfg.split_summary_repo_link &&
+			!(strcmp(ctx.repo->name, ctx.repo->url))) {
+			char *name = ctx.repo->name;
+			char *start = name;
+			for (;;) {
+				char *delim = strchr(start, '/');
+				if (delim)
+					*delim = '\0';
+
+				html_link_open(cgit_repourl(name), NULL, NULL);
+				html_ntxt(strlen(start), start);
+				html_link_close();
+
+				if (!delim)
+					break;
+				*delim = '/';
+				html("/");
+				start = delim + 1;
+			}
+		} else
+			cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
+
 		if (ctx.env.authenticated) {
 			html("</td><td class='form'>");
 			html("<form method='get' action=''>\n");
-- 
2.8.1

[PATCH v5 1/1] ui-shared: allow to split the repository link

[petr.vorel]

Hi there,

any objections to this patch?


Kind regards,
Petr

> Teach cgit split the repository link in the top of repository "summary"
> view. This emulates the same behaviour as it's in gitweb.

> This behaviour is not implemented for repositories which have
> "repo.name" set different than "repo.url".

> This feature is controlled by a new config variable:
> "split-summary-repo-link" (disabled by default).

> Signed-off-by: Petr Vorel <petr.vorel at gmail.com>
> ---
> v5: Rename config variable to split-summary-repo-link
> v4: Implement suggestions from John Keeping (thanks for them :-)):
> * use strchr() instead of strtok_r() and variadic arrays
> * use cgit_repourl() and html_*() instead of cgit_summary_link().
> * add comment why we compare page.url and page.name
> v3: New config variable "summary-enable-split-repo-link", minor cleanup.
> v2: Minor cleanup.
> ---
>  cgit.c         |  3 +++
>  cgit.h         |  1 +
>  cgitrc.5.txt   |  6 ++++++
>  tests/setup.sh |  1 +
>  ui-shared.c    | 29 ++++++++++++++++++++++++++++-
>  5 files changed, 39 insertions(+), 1 deletion(-)

> diff --git a/cgit.c b/cgit.c
> index fc482be..a133226 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -246,6 +246,8 @@ static void config_cb(const char *name, const char *value)
>  		ctx.cfg.section_sort = atoi(value);
>  	else if (!strcmp(name, "source-filter"))
>  		ctx.cfg.source_filter = cgit_new_filter(value, SOURCE);
> +	else if (!strcmp(name, "split-summary-repo-link"))
> +		ctx.cfg.split_summary_repo_link = atoi(value);
>  	else if (!strcmp(name, "summary-log"))
>  		ctx.cfg.summary_log = atoi(value);
>  	else if (!strcmp(name, "summary-branches"))
> @@ -388,6 +390,7 @@ static void prepare_context(void)
>  	ctx.cfg.section = "";
>  	ctx.cfg.repository_sort = "name";
>  	ctx.cfg.section_sort = 1;
> +	ctx.cfg.split_summary_repo_link = 0;
>  	ctx.cfg.summary_branches = 10;
>  	ctx.cfg.summary_log = 10;
>  	ctx.cfg.summary_tags = 10;
> diff --git a/cgit.h b/cgit.h
> index 325432b..bfd1dfb 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -254,6 +254,7 @@ struct cgit_config {
>  	int section_from_path;
>  	int snapshots;
>  	int section_sort;
> +	int split_summary_repo_link;
>  	int summary_branches;
>  	int summary_log;
>  	int summary_tags;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 2e1912d..a84fc10 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -438,6 +438,12 @@ summary-branches::
>  	Specifies the number of branches to display in the repository "summary"
>  	view. Default value: "10".

> +split-summary-repo-link::
> +	Flag which, when set to "1", will make cgit split the repository link in
> +	the top of repository "summary" view. This emulates the same behaviour as
> +	it's in gitweb. This behaviour is not implemented for repositories which
> +	have "repo.name" set different than "repo.url". Default value: "0".
> +
>  summary-log::
>  	Specifies the number of log entries to display in the repository
>  	"summary" view. Default value: "10".
> diff --git a/tests/setup.sh b/tests/setup.sh
> index 7590f04..4f445dc 100755
> --- a/tests/setup.sh
> +++ b/tests/setup.sh
> @@ -109,6 +109,7 @@ enable-log-filecount=1
>  enable-log-linecount=1
>  summary-log=5
>  summary-branches=5
> +split-summary-repo-link=0
>  summary-tags=5
>  clone-url=git://example.org/\$CGIT_REPO_URL.git
>  enable-filter-overrides=1
> diff --git a/ui-shared.c b/ui-shared.c
> index 9a38aa9..e86ea58 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -937,7 +937,34 @@ static void print_header(void)
>  	if (ctx.repo) {
>  		cgit_index_link("index", NULL, NULL, NULL, NULL, 0, 1);
>  		html(" : ");
> -		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
> +
> +		/*
> +		 * NOTE: If repo.name and repo.url are different, we ignore
> +		 * split_summary_repo_link flag and don't split link as it
> +		 * wouldn't make sense to split the path.
> +		 * */
> +		if (ctx.cfg.split_summary_repo_link &&
> +			!(strcmp(ctx.repo->name, ctx.repo->url))) {
> +			char *name = ctx.repo->name;
> +			char *start = name;
> +			for (;;) {
> +				char *delim = strchr(start, '/');
> +				if (delim)
> +					*delim = '\0';
> +
> +				html_link_open(cgit_repourl(name), NULL, NULL);
> +				html_ntxt(strlen(start), start);
> +				html_link_close();
> +
> +				if (!delim)
> +					break;
> +				*delim = '/';
> +				html("/");
> +				start = delim + 1;
> +			}
> +		} else
> +			cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
> +
>  		if (ctx.env.authenticated) {
>  			html("</td><td class='form'>");
>  			html("<form method='get' action=''>\n");

[PATCH v5 1/1] ui-shared: allow to split the repository link

[petr.vorel]

Dear Jason,

any objections to this patch?
Anything I can do for it to be merged?
https://lists.zx2c4.com/pipermail/cgit/2016-May/003052.html
https://lists.zx2c4.com/pipermail/cgit/2016-May/003038.html


Kind regards,
Petr


> Teach cgit split the repository link in the top of repository "summary"
> view. This emulates the same behaviour as it's in gitweb.

> This behaviour is not implemented for repositories which have
> "repo.name" set different than "repo.url".

> This feature is controlled by a new config variable:
> "split-summary-repo-link" (disabled by default).

> Signed-off-by: Petr Vorel <petr.vorel at gmail.com>
> ---
> v5: Rename config variable to split-summary-repo-link
> v4: Implement suggestions from John Keeping (thanks for them :-)):
> * use strchr() instead of strtok_r() and variadic arrays
> * use cgit_repourl() and html_*() instead of cgit_summary_link().
> * add comment why we compare page.url and page.name
> v3: New config variable "summary-enable-split-repo-link", minor cleanup.
> v2: Minor cleanup.
> ---
>  cgit.c         |  3 +++
>  cgit.h         |  1 +
>  cgitrc.5.txt   |  6 ++++++
>  tests/setup.sh |  1 +
>  ui-shared.c    | 29 ++++++++++++++++++++++++++++-
>  5 files changed, 39 insertions(+), 1 deletion(-)

> diff --git a/cgit.c b/cgit.c
> index fc482be..a133226 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -246,6 +246,8 @@ static void config_cb(const char *name, const char *value)
>  		ctx.cfg.section_sort = atoi(value);
>  	else if (!strcmp(name, "source-filter"))
>  		ctx.cfg.source_filter = cgit_new_filter(value, SOURCE);
> +	else if (!strcmp(name, "split-summary-repo-link"))
> +		ctx.cfg.split_summary_repo_link = atoi(value);
>  	else if (!strcmp(name, "summary-log"))
>  		ctx.cfg.summary_log = atoi(value);
>  	else if (!strcmp(name, "summary-branches"))
> @@ -388,6 +390,7 @@ static void prepare_context(void)
>  	ctx.cfg.section = "";
>  	ctx.cfg.repository_sort = "name";
>  	ctx.cfg.section_sort = 1;
> +	ctx.cfg.split_summary_repo_link = 0;
>  	ctx.cfg.summary_branches = 10;
>  	ctx.cfg.summary_log = 10;
>  	ctx.cfg.summary_tags = 10;
> diff --git a/cgit.h b/cgit.h
> index 325432b..bfd1dfb 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -254,6 +254,7 @@ struct cgit_config {
>  	int section_from_path;
>  	int snapshots;
>  	int section_sort;
> +	int split_summary_repo_link;
>  	int summary_branches;
>  	int summary_log;
>  	int summary_tags;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 2e1912d..a84fc10 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -438,6 +438,12 @@ summary-branches::
>  	Specifies the number of branches to display in the repository "summary"
>  	view. Default value: "10".

> +split-summary-repo-link::
> +	Flag which, when set to "1", will make cgit split the repository link in
> +	the top of repository "summary" view. This emulates the same behaviour as
> +	it's in gitweb. This behaviour is not implemented for repositories which
> +	have "repo.name" set different than "repo.url". Default value: "0".
> +
>  summary-log::
>  	Specifies the number of log entries to display in the repository
>  	"summary" view. Default value: "10".
> diff --git a/tests/setup.sh b/tests/setup.sh
> index 7590f04..4f445dc 100755
> --- a/tests/setup.sh
> +++ b/tests/setup.sh
> @@ -109,6 +109,7 @@ enable-log-filecount=1
>  enable-log-linecount=1
>  summary-log=5
>  summary-branches=5
> +split-summary-repo-link=0
>  summary-tags=5
>  clone-url=git://example.org/\$CGIT_REPO_URL.git
>  enable-filter-overrides=1
> diff --git a/ui-shared.c b/ui-shared.c
> index 9a38aa9..e86ea58 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -937,7 +937,34 @@ static void print_header(void)
>  	if (ctx.repo) {
>  		cgit_index_link("index", NULL, NULL, NULL, NULL, 0, 1);
>  		html(" : ");
> -		cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
> +
> +		/*
> +		 * NOTE: If repo.name and repo.url are different, we ignore
> +		 * split_summary_repo_link flag and don't split link as it
> +		 * wouldn't make sense to split the path.
> +		 * */
> +		if (ctx.cfg.split_summary_repo_link &&
> +			!(strcmp(ctx.repo->name, ctx.repo->url))) {
> +			char *name = ctx.repo->name;
> +			char *start = name;
> +			for (;;) {
> +				char *delim = strchr(start, '/');
> +				if (delim)
> +					*delim = '\0';
> +
> +				html_link_open(cgit_repourl(name), NULL, NULL);
> +				html_ntxt(strlen(start), start);
> +				html_link_close();
> +
> +				if (!delim)
> +					break;
> +				*delim = '/';
> +				html("/");
> +				start = delim + 1;
> +			}
> +		} else
> +			cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
> +
>  		if (ctx.env.authenticated) {
>  			html("</td><td class='form'>");
>  			html("<form method='get' action=''>\n");

[PATCH v5 1/1] ui-shared: allow to split the repository link

[Jason]

It looks mostly good to me. I'll get this in the next release.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20160830/a6206316/attachment.html>

[PATCH v5 1/1] ui-shared: allow to split the repository link

[Jason]

Hi Petr,

Sorry this has taken so long to review properly. Comments inline below:

On Thu, May 12, 2016 at 5:44 PM, Petr Vorel <petr.vorel at gmail.com> wrote:
> This behaviour is not implemented for repositories which have
> "repo.name" set different than "repo.url".

That's annoying. Why? Is there a way to make the behavior consistant?
Is gitweb inconsistent like this too?

>
> This feature is controlled by a new config variable:
> "split-summary-repo-link" (disabled by default).

I realize that John asked for a config variable in v3. But I do wonder
if this really is so necessary. In what case would somebody _not_ want
this behavior?

> +split-summary-repo-link::
> +       Flag which, when set to "1", will make cgit split the repository link in
> +       the top of repository "summary" view. This emulates the same behaviour as
> +       it's in gitweb. This behaviour is not implemented for repositories which
> +       have "repo.name" set different than "repo.url". Default value: "0".

Grammatical errors. No need to mention gitweb.

[PATCH v5 1/1] ui-shared: allow to split the repository link

[petr.vorel]

Hi Jason,

> Sorry this has taken so long to review properly. Comments inline below:
Thanks a lot for your review. I really appreciate!

> On Thu, May 12, 2016 at 5:44 PM, Petr Vorel <petr.vorel at gmail.com> wrote:
> > This behaviour is not implemented for repositories which have
> > "repo.name" set different than "repo.url".

> That's annoying. Why? Is there a way to make the behavior consistant?
> Is gitweb inconsistent like this too?

Cgit allow user to have different link and name. This is valid configuration, but how to
cope with 3) and 4)? Don't have gitweb installed to test it.

# 1) ok
repo.url=section/subsection/myrepo
repo.path=/var/spool/cgit/myrepo/
repo.desc=url only

# 2) ok
repo.url=section/subsection/myrepo2
repo.path=/var/spool/cgit/myrepo2/
repo.desc=url only

# 3) different last part of path
repo.url=section/subsection/myrepo3
repo.name=section/subsection/myrepo3-different-name
repo.path=/var/spool/cgit/myrepo3/
repo.desc=path and name different: url=section/subsection/myrepo3, name=section/subsection/myrepo3-different-name

# 4) paths are completely different
repo.url=section/myrepo4
repo.name=section/subsection/myrepo4
repo.path=/var/spool/cgit/myrepo4/
repo.desc=path and name different: url=section/myrepo4, name=different-section/different-subsection/myrepo4

Any idea how to cope with it? I thought to use this feature only if repo.name and repo.url
are the same.

> > This feature is controlled by a new config variable:
> > "split-summary-repo-link" (disabled by default).

> I realize that John asked for a config variable in v3. But I do wonder
> if this really is so necessary. In what case would somebody _not_ want
> this behavior?
I don't think so, I'd also remove the config variable.

> > +split-summary-repo-link::
> > +       Flag which, when set to "1", will make cgit split the repository link in
> > +       the top of repository "summary" view. This emulates the same behaviour as
> > +       it's in gitweb. This behaviour is not implemented for repositories which
> > +       have "repo.name" set different than "repo.url". Default value: "0".

> Grammatical errors. No need to mention gitweb.


Kind regards,
Petr

[PATCH v5 1/1] ui-shared: allow to split the repository link

[Jason]

On Thu, Nov 24, 2016 at 6:32 PM, Petr Vorel <petr.vorel at gmail.com> wrote:
> Any idea how to cope with it? I thought to use this feature only if repo.name and repo.url
> are the same.

One way might be to always use the `name` part and not the `url`, but
to ensure that any clickable links are actually useful. Namely, ensure
that any clickable links go to subtrees that contain other
repositories. This would also help collapse single-child directories,
like what we now do in tree view.

@John - any opinions on this?

>> I realize that John asked for a config variable in v3. But I do wonder
>> if this really is so necessary. In what case would somebody _not_ want
>> this behavior?
> I don't think so, I'd also remove the config variable.

I'll defer to John, then, to defend having a config variable.

[PATCH v5 1/1] ui-shared: allow to split the repository link

[john]

On Thu, Nov 24, 2016 at 06:42:32PM +0100, Jason A. Donenfeld wrote:
> On Thu, Nov 24, 2016 at 6:32 PM, Petr Vorel <petr.vorel at gmail.com> wrote:
> > Any idea how to cope with it? I thought to use this feature only if repo.name and repo.url
> > are the same.
> 
> One way might be to always use the `name` part and not the `url`, but
> to ensure that any clickable links are actually useful. Namely, ensure
> that any clickable links go to subtrees that contain other
> repositories. This would also help collapse single-child directories,
> like what we now do in tree view.
> 
> @John - any opinions on this?

I originally suggested using this only when repo.name and repo.url are
the same and I still can't think of a good behaviour for when they
differ.

I guess if there is a common path prefix between repo.name and repo.url
then we could implement this behaviour for that, but that feels like
we're getting too far into edge cases.

Basically, this feels like a good initial implementation that punts the
complexity for us to deal with when/if someone has a use case.  It also
has the advantage of being easy to explain in documentation.

> >> I realize that John asked for a config variable in v3. But I do wonder
> >> if this really is so necessary. In what case would somebody _not_ want
> >> this behavior?
> > I don't think so, I'd also remove the config variable.
> 
> I'll defer to John, then, to defend having a config variable.

It looks like my original argument was essentially "some people might
object to the change in behaviour", so I have no problem with just
changing it and adding a config variable later if people scream.

[PATCH v5 1/1] ui-shared: allow to split the repository link

[petr.vorel]

Hi Jason, John,

thank you both for your reviews!

> On Thu, Nov 24, 2016 at 06:42:32PM +0100, Jason A. Donenfeld wrote:
> > On Thu, Nov 24, 2016 at 6:32 PM, Petr Vorel <petr.vorel at gmail.com> wrote:
> > > Any idea how to cope with it? I thought to use this feature only if repo.name and repo.url
> > > are the same.

> > One way might be to always use the `name` part and not the `url`, but
> > to ensure that any clickable links are actually useful. Namely, ensure
> > that any clickable links go to subtrees that contain other
> > repositories. This would also help collapse single-child directories,
> > like what we now do in tree view.

> > @John - any opinions on this?

> I originally suggested using this only when repo.name and repo.url are
> the same and I still can't think of a good behaviour for when they
> differ.

> I guess if there is a common path prefix between repo.name and repo.url
> then we could implement this behaviour for that, but that feels like
> we're getting too far into edge cases.

> Basically, this feels like a good initial implementation that punts the
> complexity for us to deal with when/if someone has a use case.  It also
> has the advantage of being easy to explain in documentation.
Any conclusion on this? Keep it the same as it is in v5 or make items of common prefix
clickable? I'd personally prefer the first variant as users might get confused with the
second variant.


> > >> I realize that John asked for a config variable in v3. But I do wonder
> > >> if this really is so necessary. In what case would somebody _not_ want
> > >> this behavior?
> > > I don't think so, I'd also remove the config variable.

> > I'll defer to John, then, to defend having a config variable.

> It looks like my original argument was essentially "some people might
> object to the change in behaviour", so I have no problem with just
> changing it and adding a config variable later if people scream.
Right, I'll drop it in v6.


Kind regards,
Petr