Rendering of README.md inline with inner tree view dirs

[andy at warmcat.com (Andy Green)]

On 06/11/2018 11:38 PM, John Keeping wrote:
> On Mon, Jun 11, 2018 at 04:05:38PM +0800, Andy Green wrote:

>> I think what github did comes into its own when you are in the mode of
>> coming new to a project and trying to understand what you are even
>> looking at from the project layout in the filespace.  So they may well
>> be clicking around in the tree view, it's perfect if any project
>> documentation in that dir just magically appears after the already
>> expected navigation context explaining it.
>>
>> Basically any available docs follow along contextually.
>>
>> I think it's desirable under all circumstances, since it's only coming
>> if someone bothered to put relevant docs right there... if no way to do
>> it right now and no better ideas, I will try to understand how cgit
>> works for this tomorrow and see if any ideas.
> 
> I think this is a potentially useful feature; long before GitHub existed
> web interfaces to file servers have included README content with the
> directory listing.

Yes indeed.

> Given the render mode patches you listed above, I think it should be
> reasonably straightforward to add this feature in ui-tree.c with the
> following changes:
> 
> - Add fields to walk_tree_context to remember the filename and object_id
>    of a target README file (this needs some configuration to answer the
>    question: what is a README file?) and populate these during the normal
>    tree walk (probably in ls_item(), being careful to only accept blobs)
> - In ls_tail(), if the walk_tree_context has valid values for those
>    fields, then read the blob content from the object_id and call
>    render_buffer()
> 
> This will re-use the existing render filter for "inline" README content,
> which I think is a good thing.  I think the filenames for READMEs will
> have to be a new configuration (the existing "readme" configuration
> takes a blob ref whereas this option wants a simple filename).

Thanks for the suggestions, and the patches that are doing most of the 
work here.

I got quite far with it, you can see

https://libwebsockets.org/git/libwebsockets/tree/

and

https://libwebsockets.org/git/libwebsockets/tree/minimal-examples

are basically there.  If it's helpful to post a WIP series happy to do it.

1) I did not attempt to have it learn about suitable filenames from the 
config yet, I just have ls_item() looking for "README.md".  I saw 
there's already a way (readme=) for the config to list them for the 
about page... basically now tree view becomes a superset of the 
operation of the about page; the about page content appears in tree view.

So do you have any thoughts about just re-using that list?

2) In the current patches, I allowed for ls_item to discover a 
linked-list of files and render them later one after the other.  Eg, a 
dir of READMEs would render them like that.  It's welcome or preferable 
to just restrict it to one?

3) You can see on the top level of the tree, the README.md references

<img alt="lws-overview" src="./doc-assets/lws-overview.png">

This url format works in github.  In the cgit About view, this resolves to

/git/libwebsockets/about/doc-assets/lws-overview.png

which also serves the right mimetype and content.  So that kind of URL 
format is useful.  But when we render the same markup and relative path 
via /tree/, it tries to show an html page containing the content. 
That's why the picture is missing in the /tree/ view... other pictures 
in that markup are coming with absolute URLs outside of cgit and are 
working.

I can have the direct content from cgit generally, but either the markup 
needs fixing up to

/git/libwebsockets/plain/doc-assets/lws-overview.png

or /tree/ needs to learn to do what /about/ does.

I'm wondering whether mmd2html might grow an environment var to know the 
base part for URLS that want to direct-render from cgit.  Or if better 
to follow what /about/ did in /tree/.

4) Looking at read_sha1_file() in print_object(), I can't immediately 
see who frees that.  Running cgit from the commandline, with valgrind, 
he seems to leak some things.  Since it's a cgi, the policy is we don't 
worry too much about that, or I just miss the idea about where it's freed?

5) I get some gcc 8.1 warnings, I made a couple of patches to get around 
them.  In a couple of places, the code seemed legit really.

     gcc8.1: fix strncpy bounds warnings

     These warnings are coming on default Fedora 28 build and probably 
others using gcc 8.1

     ../shared.c: In function ?expand_macro?:
     ../shared.c:483:3: warning: ?strncpy? specified bound depends on 
the length of the source argument [-Wstringop-overflow=]
        strncpy(name, value, len);
        ^~~~~~~~~~~~~~~~~~~~~~~~~
     ../shared.c:480:9: note: length computed here
        len = strlen(value);
              ^~~~~~~~~~~~~

     ../ui-shared.c: In function ?cgit_repobasename?:
     ../ui-shared.c:135:2: warning: ?strncpy? specified bound 1024 
equals destination size [-Wstringop-truncation]
       strncpy(rvbuf, reponame, sizeof(rvbuf));
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/shared.c b/shared.c
index 21ac8f4..477db0a 100644
--- a/shared.c
+++ b/shared.c
@@ -480,7 +480,7 @@ static char *expand_macro(char *name, int maxlength)
                 len = strlen(value);
                 if (len > maxlength)
                         len = maxlength;
-               strncpy(name, value, len);
+               memcpy(name, value, len);
         }
         return name + len;
  }
diff --git a/ui-shared.c b/ui-shared.c
index 9d8f66b..6656bd5 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -129,11 +129,12 @@ char *cgit_pageurl(const char *reponame, const 
char *pagename,
  const char *cgit_repobasename(const char *reponame)
  {
         /* I assume we don't need to store more than one repo basename */
-       static char rvbuf[1024];
+       static char rvbuf[1025];
         int p;
         const char *rv;
-       strncpy(rvbuf, reponame, sizeof(rvbuf));
-       if (rvbuf[sizeof(rvbuf)-1])
+
+       strncpy(rvbuf, reponame, sizeof(rvbuf) - 1);
+       if (rvbuf[sizeof(rvbuf) - 2])
                 die("cgit_repobasename: truncated repository name 
'%s'", reponame);
         p = strlen(rvbuf)-1;
         /* strip trailing slashes */


and also

     gcc8.1: fix strcat warning

     ../ui-ssdiff.c: In function ?replace_tabs?:
     ../ui-ssdiff.c:142:4: warning: ?strncat? output truncated copying 
between 1 and 8 bytes from a string of length 8 [-Wstringop-truncation]
         strncat(result, spaces, 8 - (strlen(result) % 8));
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/ui-ssdiff.c b/ui-ssdiff.c
index 7f261ed..e520b95 100644
--- a/ui-ssdiff.c
+++ b/ui-ssdiff.c
@@ -118,7 +118,6 @@ static char *replace_tabs(char *line)
         int n_tabs = 0;
         int i;
         char *result;
-       char *spaces = "        ";

         if (linelen == 0) {
                 result = xmalloc(1);
@@ -138,8 +137,17 @@ static char *replace_tabs(char *line)
                         strcat(result, prev_buf);
                         break;
                 } else {
+                       char *p;
+                       int n;
+
                         strncat(result, prev_buf, cur_buf - prev_buf);
-                       strncat(result, spaces, 8 - (strlen(result) % 8));
+
+                       n = strlen(result);
+                       p = result + n;
+                       n = 8 - (n % 8);
+                       while (n--)
+                               *p++ = ' ';
+                       *p = '\0';
                 }
                 prev_buf = cur_buf + 1;
         }

I should include these or the slightly dubious warnings are just 
regarded as an annoyance?

Thanks,

-Andy

> 
> John
>