[PATCH v3 16/17] ui-shared: add helper for generating non-urlencoded links

[John Keeping]

On Tue, Jun 19, 2018 at 05:02:47PM +0800, Andy Green wrote:
> We are going to have to produce plain links in the next patch.
> But depending on config, the links are not simple.
> 
> Reproduce the logic in repolink() to generate correctly-
> formatted links in a strbuf, without urlencoding, in a reusable
> helper cgit_repo_create_url().
> 
> Signed-off-by: Andy Green <andy at warmcat.com>
> ---
>  ui-shared.c |   30 ++++++++++++++++++++++++++++++
>  ui-shared.h |    3 +++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index 21bbded..79c39a8 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -221,6 +221,36 @@ void cgit_index_link(const char *name, const char *title, const char *class,
>  	site_link(NULL, name, title, class, pattern, sort, ofs, always_root);
>  }
>  
> +char *cgit_repo_create_url(struct strbuf *sb_pre, struct strbuf *sb_post,
> +			    const char *page, const char *head, const char *path)
> +{
> +	const char *delim = "?";
> +	struct strbuf *sb = sb_pre;

Does this variable serve any point?  Why not rename the parameter?

> +
> +	if (ctx.cfg.virtual_root)
> +		strbuf_addf(sb_pre, "%s%s", ctx.cfg.virtual_root, ctx.repo->url);

NIT: I think we normally put the braces in around a oneline if with a
multi-line else clause.

> +	else {
> +		strbuf_addstr(sb_pre, ctx.cfg.script_name);
> +		strbuf_addf(sb_post, "?url=%s", ctx.repo->url);
> +		sb = sb_post;
> +		delim = "&";
> +	}
> +
> +	if (ctx.repo->url[strlen(ctx.repo->url) - 1] != '/')
> +		strbuf_addch(sb, '/');

Maybe add a blank line here since these if blocks are unrelated.

> +	if (page) {
> +		strbuf_addf(sb, "%s/", page);
> +		if (path)
> +			strbuf_addstr(sb, path);
> +	}
> +
> +	if (head && ctx.repo->defbranch && strcmp(head, ctx.repo->defbranch)) {
> +		strbuf_addf(sb_post, "%sh=%s", delim, head);
> +		delim = "&";
> +	}
> +	return fmt("%s", delim);

delim is always a character constant, so I think we can just return it
as "const char *" here.  This also avoids any worry about the lifetime
of fmt()'s result.

> +}
> +
>  static char *repolink(const char *title, const char *class, const char *page,
>  		      const char *head, const char *path)
>  {
> diff --git a/ui-shared.h b/ui-shared.h
> index c105b3b..679d2f2 100644
> --- a/ui-shared.h
> +++ b/ui-shared.h
> @@ -59,6 +59,9 @@ extern void cgit_object_link(struct object *obj);
>  
>  extern void cgit_submodule_link(const char *class, char *path,
>  				const char *rev);
> +extern char *cgit_repo_create_url(struct strbuf *sb_pre, struct strbuf *sb_post,
> +				  const char *page, const char *head,
> +				  const char *path);
>  
>  extern void cgit_print_layout_start(void);
>  extern void cgit_print_layout_end(void);