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 <[email protected]> > > > > A couple of minor style points below, but this looks good. With or > > without the style changes: > > > > Reviewed-by: John Keeping <[email protected]> > > >> + 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> </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. _______________________________________________ CGit mailing list [email protected] https://lists.zx2c4.com/mailman/listinfo/cgit
