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

[Andy Green]

On 06/20/2018 05:55 AM, John Keeping wrote:
> 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?

OK.

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

OK.

>> +	else {
>> +		strbuf_addstr(sb_pre, ctx.cfg.script_name);
>> +		strbuf_addf(sb_post, "?url=%s", ctx.repo->url);
>> +		sb = sb_post;
>> +		delim = "&";

These shouldn't've been left urlencoded... I changed both to "&".

>> +	}
>> +
>> +	if (ctx.repo->url[strlen(ctx.repo->url) - 1] != '/')
>> +		strbuf_addch(sb, '/');
> 
> Maybe add a blank line here since these if blocks are unrelated.

OK.

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

I just copied this exit stuff from repolink... I agree return the const 
char * to rodata pointer is better.

Thanks for the review.

-Andy

>> +}
>> +
>>   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);