From mboxrd@z Thu Jan  1 00:00:00 1970
From: andy at warmcat.com (Andy Green)
Date: Tue, 19 Jun 2018 09:55:18 +0800
Subject: [PATCH v2 12/15] ui-tree: render any matching README file in tree
 view
In-Reply-To: <20180618193613.GU1922@john.keeping.me.uk>
References: <152928998685.10419.7869045561776063625.stgit@mail.warmcat.com>
 <152929069579.10419.11978742686135883967.stgit@mail.warmcat.com>
 <20180618193613.GU1922@john.keeping.me.uk>
Message-ID: <0fea8736-a55c-3306-4185-7b0424119a43@warmcat.com>



On 06/19/2018 03:36 AM, John Keeping wrote:
> On Mon, Jun 18, 2018 at 10:58:15AM +0800, Andy Green wrote:
>> While listing the items in tree view, we collect a list
>> of any filenames that match any tree-readme entries from the
>> config file.
>>
>> After the tree view has been shown, we iterate through any
>> collected readme files rendering them inline.
>>
>> Signed-off-by: Andy Green <andy at warmcat.com>
> 
> A couple of minor style points below, but this looks good.  With or
> without the style changes:
> 
> Reviewed-by: John Keeping <john at keeping.me.uk>

>> +	render = get_render_for_filename(walk_tree_ctx->inline_path);
>> +	mimetype = render ? NULL : get_mimetype_for_filename(
>> +				walk_tree_ctx->inline_path);
>> +
>> +	name = strrchr(walk_tree_ctx->inline_path, '/');
> 
> Isn't this impossible?   inline_path is a filename at a single level of
> the tree and Git forbids directory separators there.

Yes... it gets copied from a var "pathname"... I changed the name to 
inline_filename to remove this confusion in the new code anyway.  And I 
removed name here and just use walk_tree_ctx->inline_filename.

>> +	if (name)
>> +		name++;
>> +	else
>> +		name = walk_tree_ctx->inline_path;
>> +
>> +	htmlf("<h2>%s</h2>", name);
>> +	html("<div class=blob>&nbsp;</div>\n");
>> +
>> +	if (render || mimetype) {
>> +		if (render)
>> +			render_buffer(render, name, buf, size);
>> +		else
>> +			include_file(walk_tree_ctx->inline_path, mimetype);
> 
> We can lose a level of indentation here by writing it as:
> 
> 	if (render)
> 		...
> 	else if (mimetype)
> 		...
> 	else
> 		...

OK.  Actually this was a modified cut-n-paste from your patch's 
implementation in print_object().  So I also changed that code to follow 
this scheme.

	if (use_render) {
		if (render)
			render_buffer(render, basename, buf, size);
		else
			include_file(path, mimetype);
	} else {
		print_buffer(basename, buf, size);
	}

became

         if (!use_render)
                 print_buffer(basename, buf, size);
         else if (render)
                 render_buffer(render, basename, buf, size);
         else
                 include_file(path, mimetype);

-Andy