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

On Tue, Jun 19, 2018 at 09:55:18AM +0800, Andy Green wrote:
> 
> 
> 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);

I think the point is that the logic is different, in that this version
effectively has use_render always true, whereas the version in
print_object() must allow the caller to disable render/mimetype handling
even if those variables are non-null.

It's personal taste, but I think the positive logic is clearer to read.