Re: cgit simple-authentication.lua problem

[John Keeping]

On Thu, Jun 15, 2023 at 09:40:35AM +0200, Yazar Strulik wrote:
> I need some help with getting the simple-authentication.lua running.
> 
> I'm using gitolite as my git base with cgit as my front-end and apache2 as
> the webserver.
> The main cgit configuration works but it cant interpret the .lua files when
> using the authentication-filters by adding following line to the cgitrc:
>
> auth-filter=/usr/lib/cgit/filters/simple-authentication.lua

The path needs a "lua:" prefix, like this:

auth-filter=lua:/usr/lib/cgit/filters/simple-authentication.lua

Without that the default "exec:" path is used which tries to execute the
script.

Re: cgit with busybox httpd

[John Keeping]

[Cc: add the mailing list back in]

On Mon, Mar 06, 2023 at 01:01:50PM +0100, Andreas Mahling wrote:
> It seems the 404 is generated by httpd, I think because the url ends with a
> slash httpd treats the part after cgit.cgi not as PATH_INFO (as it should)
> but as a directory. So this seems more a fault of httpd to me.
> 
> Sorry, but I don't understand how to put into QUERY_STRING what now goes
> into PATH_INFO. Do you mean the virtual-root option in cgitrc?
> It is already commented out in my setup, but cgit builds the url with
> PATH_INFO part anyway.

To disable that feature you would have the patch the CGit source and
build your own cgit binary.

There is code in cmd_main() which automatically calculates the virtual
root from other environment variables if they are provided (and it seems
that httpd does provide this detail to CGI scripts).

> Am Mo., 6. März 2023 um 12:41 Uhr schrieb John Keeping :
> 
> > On Mon, Mar 06, 2023 at 11:13:10AM +0100, Andreas Mahling wrote:
> > > I'm in the process to configure a cgit instance for my private network
> > at home.
> > > Because my internet router will be the host for git and cgit, I'm
> > > looking for a ligthweight setup.
> > > I would like to use busybox httpd as webserver, which has a very low
> > > footprint and supports cgi, but no other fancy bells and whistles,
> > > especially no url rewrite.
> > >
> > > It works in principle, but there is a problem with the URLs generated
> > > by cgit: they seem always containing a trailing slash. This leads to a
> > > 404 error thrown by httpd. If I manually remove the slash, everyting
> > > is OK.
> > >
> > > Example given:
> > > http://git/cgi-bin/cgit.cgi/linuxadmin.git/ -> 404
> > > http://git/cgi-bin/cgit.cgi/linuxadmin.git -> Page ist shown
> > >
> > > Is there a way to tell cgit to give up the trailing slash? I'm running
> > > cgit-1.2.3-r3 on Alpine 3.17.2
> >
> > I don't think it's possible to change that behaviour without patching
> > CGit.  Given how URLs are constructed, it looks a bit complicated to fix
> > in all cases, but you could remove the setting of "virtual_root" in
> > cmd_main() to force using query parameters instead of virtual paths in
> > URLs.
> >
> > Can you tell where the 404 is generated?  Is this a case of httpd
> > returning an error when it shouldn't, or is it a behaviour difference
> > that CGit isn't handling correctly - maybe other CGI hosts strip
> > trailing slashes before passing a path to CGit but httpd doesn't?
> >

Re: cgit with busybox httpd

[John Keeping]

On Mon, Mar 06, 2023 at 11:13:10AM +0100, Andreas Mahling wrote:
> I'm in the process to configure a cgit instance for my private network at 
> home.
> Because my internet router will be the host for git and cgit, I'm
> looking for a ligthweight setup.
> I would like to use busybox httpd as webserver, which has a very low
> footprint and supports cgi, but no other fancy bells and whistles,
> especially no url rewrite.
> 
> It works in principle, but there is a problem with the URLs generated
> by cgit: they seem always containing a trailing slash. This leads to a
> 404 error thrown by httpd. If I manually remove the slash, everyting
> is OK.
> 
> Example given:
> http://git/cgi-bin/cgit.cgi/linuxadmin.git/ -> 404
> http://git/cgi-bin/cgit.cgi/linuxadmin.git -> Page ist shown
> 
> Is there a way to tell cgit to give up the trailing slash? I'm running
> cgit-1.2.3-r3 on Alpine 3.17.2

I don't think it's possible to change that behaviour without patching
CGit.  Given how URLs are constructed, it looks a bit complicated to fix
in all cases, but you could remove the setting of "virtual_root" in
cmd_main() to force using query parameters instead of virtual paths in
URLs.

Can you tell where the 404 is generated?  Is this a case of httpd
returning an error when it shouldn't, or is it a behaviour difference
that CGit isn't handling correctly - maybe other CGI hosts strip
trailing slashes before passing a path to CGit but httpd doesn't?

Re: configuring appearance from within repository

[John Keeping]

On Thu, Jan 26, 2023 at 06:42:19PM +0100, Spam Receiver wrote:
> Hey there, I recently found cgit and I'm very happy about it's
> performance, ease of use and overall simplicity. There is only one
> thing that I did not manage to set up yet:
> 
> I want to configure repo-specific settings from within the repo itself.
> For this I tried to enable the git config parsing from within cgit, and
> tried to add the following to .git/config

To confirm, you have "enable-git-config = 1" in the global cgitrc
*before* the scan-path directory?

>[gitweb]
>   owner = test name = test

There should be a line break between "test" and "name" here, right?

> But nothing changed in the web interface. My next attempt involved the
> cgitrc in the repo folder, where I added
> 
>name = test

There must not be any spaces around "=" in cgitrc files, so this should
be:

name=test


Regards,
John

Re: Greater use of the HTML title (for tabs and title bars) in cgit

[John Keeping]

Hi Graham,

On Sun, Nov 06, 2022 at 11:01:25AM +, Graham Perrin wrote:
>  for 1.0
> included:
> 
> > * Show reverse paths in title bar so that browser tab shows filename.
> 
> Please, are there plans to broaden/refine use of titles?

There are no plans - but patches are welcome :-)

> Early thoughts (with examples):
> 
> The title of 
> 
> could show:
> 
> * part of the first (summary) line of the message, abbreviated … if more
> than fifty characters

This seems useful and is probably easy to add.

> * the branch (not main)

Git commits are not associated with a branch, so unless we have an h=...
query argument I don't see any way to derive the correct value to
display for a branch.


Regards,
John

Re: git notes for the Linux kernel

[John Keeping]

On Mon, Oct 17, 2022 at 01:50:04PM +0200, Vegard Nossum wrote:
> I've improved the support for git notes in cgit, including the ability
> to load notes from a separate repository than the one you are viewing.
> 
> My use case is using a separate repository of git notes for the Linux
> kernel to annotate commits with extra cross-referencing information such
> as e.g.:
> 
> - lore links to patch submissions matching the patch,
> - references to subsequent fixes (if the current commit is buggy)
> - mitre links to CVEs
> - references to backports in stable/LTS

This sounds useful, but...

> My hope is that these notes can eventually be displayed on
> git.kernel.org -- at least, we've found the notes invaluable and a huge
> time saver in different types of kernel work. (I'm still in the process
> of working out how to release these notes and/or the scripts generating
> them, but that's a different topic.)
> 
> I tried to submit the git.git patches upstream, but they were rejected
> by the maintainer for not being general enough:
> https://lore.kernel.org/git/20220802075401.2393-1-vegard.nos...@oracle.com/

... this likely blocks inclusion in CGit as I don't think there's any
desire to maintain a fork of git.git

Parts of this series look like they make sense regardless of the
separate repo option - patch 2 looks unrelated and the repo.notes_ref
config option is potentially useful to keep the CGit config separate
from gitconfig (although it should be "repo.notes-ref" for consistency
with other config keys).  Are you interested in splitting those parts
out?


Regards,
John

Re: Downloading objects hangs up around 65kB

[John Keeping]

On Fri, Sep 16, 2022 at 10:55:39PM +0300, Valdis Vītoliņš wrote:
> Probably it is issue on 64-bit ARM architecture, because I tested that it
> works on x86_64 virtual machine.
> 
> Some time ago (because I haven't used HTTPS protocol for repositories
> recently) downloading plain objects/blobs hangs up around 65kB of data.
> 
> You can test it with:
> 
> wget -O "01_About.odp" -d
> "https://odo.lv/git/JTM/plain/doc/presentations/01_About.odp;
> 
> It shows:
> ...
> Saving to: ‘01_About.odp’
> 
> 01_About.odp8%[>   ]
> 63,64K  13,0KB/sin 4,9s
> 
> 2022-09-16 22:32:33 (13,0 KB/s) - Connection closed at byte 65169. Retrying.
> ...
> etc.

This seems very strange (it also happens with the .../tree/... pages
showing the hexdump file content).

My guess is that this is caused by the Apache configuration rather than
anything CGit is doing itself.

Have you tested running cgit outside Apache using something like:

CGIT_CONFIG=/path/to/cgitrc \
QUERY_STRING=url=JTM/plain/doc/presentations/01_About.odb \
cgit

That should give some idea whether CGit itself is returning short data
or if the response is being truncated downstream.


Regards,
John

Re: Angle brackets - < and >

[John Keeping]

On Sat, Sep 17, 2022 at 03:32:48PM +0100, Graham Perrin wrote:
> Commit messages 
> 
> and 
> 
> appear OK.
> 
> In corresponding 
> 
> and 
> ,
> it's as if the closing angle bracket is misinterpreted as part of the
> address.
> 
> Please, might this be a bug in cgit?

Commit message formatting in CGit is extensible and the example filter
shipped with CGit does not do any URL detection, so cgit.freebsd.org
must be using their own filter.

I suggest you report a bug to them.


Regards,
John

Re: Weird interactions betwen cache and module-link

[John Keeping]

On Fri, Mar 25, 2022 at 09:44:48PM +, Gianni Ceccarelli wrote:
> This patch seems to fix the problem.

Nice catch!  This definitely looks like an oversight.

Would you like to type up a proper patch with a commit message and
Signed-off-by certification?  We use the same process as git.git so
their SubmittingPatches document [1] explains this.

[1] https://git-scm.com/docs/SubmittingPatches#sign-off

> diff --git c/cgit.c w/cgit.c
> index 08d81a1..d30e259 100644
> --- c/cgit.c
> +++ w/cgit.c
> @@ -810,6 +810,10 @@ static void print_repo(FILE *f, struct cgit_repo *repo)
>   fprintf(f, "repo.extra-head-content=%s\n", 
> repo->extra_head_content);
>   if (repo->module_link)
>   fprintf(f, "repo.module-link=%s\n", repo->module_link);
> + for (int i = 0; i < repo->submodules.nr; ++i) {
> + struct string_list_item *si=>submodules.items[i];
> + fprintf(f, "repo.module-link.%s=%s\n", si->string, 
> (char*)si->util);
> + }
>   if (repo->section)
>   fprintf(f, "repo.section=%s\n", repo->section);
>   if (repo->homepage)
> 
> 
> -- 
>   Dakkar - 
>   GPG public key fingerprint = A071 E618 DD2C 5901 9574
>6FE2 40EA 9883 7519 3F88
>   key id = 0x75193F88
> 

[PATCH] css: blame: reset font size for oid

[John Keeping]

In Firefox, the hashes in the blame UI are out of step with the line
number and content leading to ever increasing vertical misalignment.

This is caused by the .oid class setting font-size to 90%, so override
this back to 100% for the blame case, bringing the height of lines in
all three columns of the table back into step.

Signed-off-by: John Keeping 
---
 cgit.css | 4 
 1 file changed, 4 insertions(+)

diff --git a/cgit.css b/cgit.css
index dfa144d..1b848cf 100644
--- a/cgit.css
+++ b/cgit.css
@@ -363,6 +363,10 @@ div#cgit table.blame td.lines > div > pre {
top: 0;
 }
 
+div#cgit table.blame .oid {
+   font-size: 100%;
+}
+
 div#cgit table.bin-blob {
margin-top: 0.5em;
border: solid 1px black;
-- 
2.35.1

[PATCH] treewide: use release_commit_memory()

[John Keeping]

Instead of calling two separate Git functions to free memory associated
with a commit object, use Git's wrapper which does this.  This also
counts as a potential future bug fix since release_commit_memory() also
resets the parsed state of the commit, meaning any attempt to use it in
the future will correctly fill out the fields again.

release_commit_memory() does not set parents to zero, so keep that for
additional safety in case CGit checks this without calling
parse_commit() again.

Signed-off-by: John Keeping 
---
 ui-atom.c  | 3 +--
 ui-log.c   | 6 ++
 ui-stats.c | 3 +--
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/ui-atom.c b/ui-atom.c
index 1056f36..83fde7d 100644
--- a/ui-atom.c
+++ b/ui-atom.c
@@ -140,8 +140,7 @@ void cgit_print_atom(char *tip, const char *path, int 
max_count)
}
while ((commit = get_revision()) != NULL) {
add_entry(commit, host);
-   free_commit_buffer(the_repository->parsed_objects, commit);
-   free_commit_list(commit->parents);
+   release_commit_memory(the_repository->parsed_objects, commit);
commit->parents = NULL;
}
html("\n");
diff --git a/ui-log.c b/ui-log.c
index 20774bf..db63424 100644
--- a/ui-log.c
+++ b/ui-log.c
@@ -489,8 +489,7 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char 
*grep, char *pattern
for (i = 0; i < ofs && (commit = get_revision()) != NULL; /* nop 
*/) {
if (show_commit(commit, ))
i++;
-   free_commit_buffer(the_repository->parsed_objects, commit);
-   free_commit_list(commit->parents);
+   release_commit_memory(the_repository->parsed_objects, commit);
commit->parents = NULL;
}
 
@@ -511,8 +510,7 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char 
*grep, char *pattern
i++;
print_commit(commit, );
}
-   free_commit_buffer(the_repository->parsed_objects, commit);
-   free_commit_list(commit->parents);
+   release_commit_memory(the_repository->parsed_objects, commit);
commit->parents = NULL;
}
if (pager) {
diff --git a/ui-stats.c b/ui-stats.c
index 09b3625..40ed6c2 100644
--- a/ui-stats.c
+++ b/ui-stats.c
@@ -241,8 +241,7 @@ static struct string_list collect_stats(const struct 
cgit_period *period)
memset(, 0, sizeof(authors));
while ((commit = get_revision()) != NULL) {
add_commit(, commit, period);
-   free_commit_buffer(the_repository->parsed_objects, commit);
-   free_commit_list(commit->parents);
+   release_commit_memory(the_repository->parsed_objects, commit);
commit->parents = NULL;
}
return authors;
-- 
2.35.1

[PATCH] blame: add a link to the parent commit

[John Keeping]

When walking through the history, it is useful to quickly see the same
file at the previous revision, so add a link to do this.

It would be nice to link to the correct line with an additional
fragment, but this requires significantly more work so it can be done as
an enhancement later.  (ent->s_lno is mostly the right thing, but it is
the line number in the post-image of the target commit whereas the link
is to the parent of that commit, i.e. the pre-image of the target.)

Suggested-by: Alejandro Colomar 
Signed-off-by: John Keeping 
---
 ui-blame.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/ui-blame.c b/ui-blame.c
index 03136f7..3b0bcad 100644
--- a/ui-blame.c
+++ b/ui-blame.c
@@ -54,6 +54,15 @@ static void emit_blame_entry_hash(struct blame_entry *ent)
html("");
free(detail);
 
+   if (!parse_commit(suspect->commit) && suspect->commit->parents) {
+   struct commit *parent = suspect->commit->parents->item;
+
+   html(" ");
+   cgit_blame_link("^", "Blame the previous revision", NULL,
+   ctx.qry.head, oid_to_hex(>object.oid),
+   suspect->path);
+   }
+
while (line++ < ent->num_lines)
html("\n");
 }
-- 
2.35.1

Re: [PATCH] Add "default-tab" and "root-default-tab" configuration options

[John Keeping]

On Sun, Jan 30, 2022 at 06:04:10PM +, equa wrote:
> These options allow the user to specify a page to display at the root
> repository/index location instead of the default summary or repository list.
> 
> Signed-off-by: equa 

We need a real name for the author and sign-off here.

> diff --git a/ui-shared.c b/ui-shared.c
> index acd8ab5..17b1e49 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -203,6 +203,9 @@ static void site_url(const char *page, const char 
> *search, const char *sort, int
>  {
>   char *delim = "?";
> 
> + if (!strcmp(page ? page : "", ctx.cfg.root_default_tab))
> + page = NULL;
> +

Why is this necessary?  Is there any requirement to shorten the URL here
or does it just make it shorter?

>   if (always_root || page)
>   html_attr(cgit_rooturl());
>   else {
> @@ -317,6 +320,12 @@ static void reporevlink(const char *page, const char 
> *name, const char *title,
>  {
>   char *delim;
> 
> + if (page
> + && !rev
> + && !path
> + && !strcmp(page, ctx.repo->default_tab))
> + page = NULL;
> +

Same as above, I don't see what advantage this gives.

Re: [PATCH] add 'go-import' meta tag if in a repo

[John Keeping]

Please include some description here about why this change is desirable.

On Sat, Jan 08, 2022 at 04:55:59PM -0700, Derek Stevens wrote:
> Signed-off-by: Derek Stevens 
> ---
>  ui-shared.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index acd8ab5..0f57af6 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -785,6 +785,9 @@ void cgit_print_docstart(void)
>   html_txt(ctx.page.title);
>   html("\n");
>   htmlf("\n", cgit_version);
> + if (ctx.repo)
> +   htmlf("\n",
> + host, cgit_rooturl(), ctx.repo->url, host, cgit_rooturl(), 
> ctx.repo->url);

The indentation looks wrong here...

>   if (ctx.cfg.robots && *ctx.cfg.robots)
>   htmlf("\n", ctx.cfg.robots);
>   

Re: [PATCH 0/2] Support of test results

[John Keeping]

On Mon, Oct 04, 2021 at 01:22:34PM +0300, Sergey Bronnikov wrote:
> From: Sergey Bronnikov 
> 
> Git SCM allows to store information in so called notes [1] and cgit
> allows to show notes that placed to default reference
> ('refs/notes/commits') and attached to certain commits. It is convenient
> to add various useful information related to commit in Git notes.
> 
> Often tests results stored for future analysis in different systems (aka
> 'test report systems'). But one is the logic places to store test
> results is a Git SCM, test report automatically connected to commits and
> easily available for everyone. It even can be automated using git-test [2].

This feels very specifc to one use case that isn't particularly general.
Have you considered any way to make this more generic for displaying
different notes for different purposes?

Did you experiment with changing notes.displayRef in gitconfing and
using the existing commit filter to display those inline in the commit
page?

> How-to use:
> 
> $ cat report.tap
> 1..2
> ok 1 - test_0.py::test_list_once
> ok 2 - test_0.py::test_list_twice
> $ GIT_NOTES_REF=refs/notes/testres git notes add -F report.tap 
> 95aa7d200ee10580c472a1156a11c726046b110f
> $ git notes add -m 'Tested-by: Sergey Bronnikov ' \
>   2a3a0d1a62de2ae5ab4511c15d82a6a0f2c2a930
> $ GIT_NOTES_REF=refs/notes/testres git show 
> 95aa7d200ee10580c472a1156a11c726046b110f
> 
> commit 95aa7d200ee10580c472a1156a11c726046b110f
> Author: Sergey Bronnikov 
> Date:   Tue Dec 22 10:06:10 2020 +0300
> 
> ...
> 
> Notes (testres):
> 1..2
> ok 1 - test_0.py::test_list_once
> ok 2 - test_0.py::test_list_twice
> 
> Test report become available in cgit by clicking to href 'tests' for
> that commit.
> 
> 1. https://git-scm.com/docs/git-notes
> 2. https://github.com/ligurio/git-test
> 3. https://github.com/ligurio/testres/wiki/Using-GIT-as-a-storage
> 
> Sergey Bronnikov (2):
>   ui-commit: add support of test results
>   ui-commit: add testres filter
> 
>  cgit.c  |  6 +++
>  cgit.h  |  4 +-
>  cgitrc.5.txt| 17 +
>  cmd.c   |  8 +++-
>  filter.c|  6 +++
>  filters/testres-example.lua | 21 +++
>  shared.c|  1 +
>  tests/setup.sh  |  2 +
>  tests/t0105-commit.sh   |  7 
>  tests/t0111-filter.sh   |  8 
>  ui-commit.c | 73 -
>  ui-commit.h |  1 +
>  ui-shared.c |  8 +++-
>  ui-shared.h |  3 ++
>  14 files changed, 161 insertions(+), 4 deletions(-)
>  create mode 100755 filters/testres-example.lua
> 
> -- 
> 2.25.1
> 

Re: [PATCH] cache: Tolerate short writes in print_slot

[John Keeping]

On Fri, Sep 10, 2021 at 05:18:41PM +0300, Hristo Venev wrote:
> sendfile() can return after a short read/write, so we may need to call
> it more than once. Furthermore, not all files support sendfile(), so we
> may need to fall back to read/write.

Have you seen these errors in practice, or is this just theoretical?

In recent (since v2.6.33) versions of Linux, all files should support
sendfile(), especially since we expect out_fd to be a socket or pipe.

> On the read/write path, use write_in_full which deals with short writes.
> 
> Signed-off-by: Hristo Venev 
> ---
>  cache.c | 46 --
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/cache.c b/cache.c
> index 55199e8..85cfbd9 100644
> --- a/cache.c
> +++ b/cache.c
> @@ -85,40 +85,42 @@ static int close_slot(struct cache_slot *slot)
>  /* Print the content of the active cache slot (but skip the key). */
>  static int print_slot(struct cache_slot *slot)
>  {
> -#ifdef HAVE_LINUX_SENDFILE
> - off_t start_off;
> - int ret;
> + off_t off;
> + ssize_t i;
> +
> + off = slot->keylen + 1;
>  
> - start_off = slot->keylen + 1;
> +#ifdef HAVE_LINUX_SENDFILE
> + off_t size;

decl-after-stmt if HAVE_LINUX_SENDFILE is set.

> + size = slot->cache_st.st_size;
>  
>   do {
> - ret = sendfile(STDOUT_FILENO, slot->cache_fd, _off,
> - slot->cache_st.st_size - start_off);
> - if (ret < 0) {
> + i = sendfile(STDOUT_FILENO, slot->cache_fd, , size - off);

Why is ret renamed?  i is normally a loop index variable, using it for
the return value here is strange, please stick with "ret".

> + if (i < 0) {
>   if (errno == EAGAIN || errno == EINTR)
>   continue;
> + /* Fall back to read/write on EINVAL */
> + if (errno == EINVAL)
> + break;
>   return errno;
>   }
> - return 0;
> + if (off == size)
> + return 0;
>   } while (1);
> -#else
> - ssize_t i, j;
> +#endif
>  
> - i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET);
> - if (i != slot->keylen + 1)
> + if (lseek(slot->cache_fd, off, SEEK_SET) != off)
>   return errno;
>  
>   do {
> - i = j = xread(slot->cache_fd, slot->buf, sizeof(slot->buf));
> - if (i > 0)
> - j = xwrite(STDOUT_FILENO, slot->buf, i);
> - } while (i > 0 && j == i);
> -
> - if (i < 0 || j != i)
> - return errno;
> - else
> - return 0;
> -#endif
> + i = xread(slot->cache_fd, slot->buf, sizeof(slot->buf));
> + if (i < 0)
> + return errno;
> + if (i == 0)
> + return 0;
> + if (write_in_full(STDOUT_FILENO, slot->buf, i) < 0)
> + return errno;
> + } while (1);
>  }
>  
>  /* Check if the slot has expired */
> -- 
> 2.31.1
> 

Re: OpenBSD - Error reading owner-info for : No such file or directory (2)

[John Keeping]

On Thu, Feb 25, 2021 at 05:00:57PM +1000, Paul W. Rankin wrote:
> I'm running cgit v1.2.3 on OpenBSD 6.8 with httpd and slowcgi. I'm 
> getting tons of log errors in the form:
>  Error reading owner-info for : No such file or directory 
> (2)
> 
> I've found this Stack Overflow question 
> https://stackoverflow.com/questions/48450631/ which suggests that the 
> problem is with the call to getpwuid trying to access /etc/passwd:
> 
>  if ((pwd = getpwuid(st.st_uid)) == NULL) {
>   fprintf(stderr, "Error reading owner-info for %s: %s (%d)\n",
>   path->buf, strerror(errno), errno);
>   break;
>  }
> 
> @ https://git.zx2c4.com/cgit/tree/scan-tree.c#n139
> 
> On OpenBSD, httpd runs in a chroot at /var/www, which would seem to 
> suggest why cgit would not be able to access /etc/passwd
> 
> In my cgitrc I have
>  enable-git-config=0
>  enable-index-owner=0
> so that doesn't help.
> 
> Is there any way to prevent all these error logs? Currently this fills 
> up a log file about once an hour...

Without any changes to the code, if you set "repo.owner" (or
"gitweb.owner" if you have "enable-git-config" set) then this value will
be used and the loop that tries getpwuid() won't be entered.

I think it would be reasonable to provide a way to disable the "owner
from filesystem" code via cgitrc.  It's probably meaningless in many
setups, for example with Gitolite, where all the repositories are owned
by a "git" user anyway.


John

Re: User-configurable log graph option

[John Keeping]

On Thu, Feb 18, 2021 at 02:31:26PM -0800, Kian Kasad wrote:
> I'm using cgit on my website and I'm wondering if it's possible to allow
> the user (i.e. the person visiting the site) to choose whether or not
> the commit graph is displayed on the log page.
> 
> If this isn't currently possible, what do you think about adding such
> functionality? Maybe something like how the diff page lets the user
> choose between a "unified diff" and "ssdiff".

This sounds like a great new feature!

I think we still want to allow the administrator to turn the feature
on/off but if it is permitted, then it makes sense to allow the end user
to choose whether or not they want it enabled.

Patches welcome :-)

Re: [PATCH] Handle tags outside of refs/tags gracefully.

[John Keeping]

On Tue, Jan 05, 2021 at 11:53:03AM +, Gianni Ceccarelli wrote:
> I have found an annoying case…
> 
> In the repository created as per my previous message, I did::
> 
>   $ git tag -a foo
>   $ git rev-parse refs/tags/foo > .git/refs/weird/annotated
>   $ git push origin refs/weird/*:refs/weird/*
> 
> This creates an "annotated tag", which is an object in the store, not
> just a reference.
> 
> Now CGit shows the ``refs/weird/annotated`` as a tag in
> https://www.thenautilus.net/cgit/example/ and several other views, but
> following that link goes to
> https://www.thenautilus.net/cgit/example/tag/?h=refs/weird/annotated
> which says "bad tag reference"
> 
> I hope you'll concur this is not the best behaviour.
> 
> I see several ways to "fix" this:
> 
> * in all the various views, don't show links to annotated tags whose
>   ref is outside of ``refs/tags/``
> * in ``/tag``, try ``refs/tags/$h`` first, and if that doesn't work,
>   try ``$h``, and show that only if it's an annotated tag (not just a
>   reference)
> * create another endpoint (``/atag``?) to show tag objects, and link
>   to that one for annotated tags whose ref is outside of
>   ``refs/tags/``

I think something like option 2 is the right answer here, since that
brings us closer to Git's behaviour.  Does the patch below help?

-- >8 --
Subject: [PATCH] ui-tag: accept tags not under refs/tags/

In log views, we decorate commits with tags under any ref hierarchy,
which generates links to ui-tag with, for example, refs/weird/tag.  By
forcing a refs/tags/ prefix onto this, a 404 not found error is
guaranteed.

Taking inspiration from Git's ref_rev_parse_rules, let's try to resolve
the given tag name raw and relative to refs/ before trying refs/tags/.

Reported-by: Gianni Ceccarelli 
Signed-off-by: John Keeping 
---
 ui-tag.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/ui-tag.c b/ui-tag.c
index 846d5b1..c1f81d1 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -38,17 +38,32 @@ static void print_download_links(char *revname)
html("");
 }
 
+static const char *tag_patterns[] = {
+   "%s",
+   "refs/%s",
+   "refs/tags/%s",
+   NULL
+};
+
 void cgit_print_tag(char *revname)
 {
struct strbuf fullref = STRBUF_INIT;
struct object_id oid;
struct object *obj;
+   const char **pattern;
 
if (!revname)
revname = ctx.qry.head;
 
-   strbuf_addf(, "refs/tags/%s", revname);
-   if (get_oid(fullref.buf, )) {
+   for (pattern = tag_patterns; *pattern; pattern++) {
+   strbuf_reset();
+   strbuf_addf(, *pattern, revname);
+
+   if (!get_oid(fullref.buf, ))
+   break;
+   }
+
+   if (!*pattern) {
cgit_print_error_page(404, "Not found",
"Bad tag reference: %s", revname);
goto cleanup;
-- 
2.30.0

Re: cgit 1.2.3: lighttpd 1.4.57, AlpineLinux [edge]: using cache breaks delivery

[John Keeping]

On Mon, Dec 21, 2020 at 05:26:19PM +0100, Steffen Nurpmeso wrote:
> I discovered today that cgit no longer delivers pages, and it must
> have been like that for some time.  The server looks show
> successful delivery, the cgit cache is populated and rotated just
> correctly, but all cgit delivers is that final error of main() as
> 
>   Error processing page: Invalid argument (22)
> 
> (surely a misconfiguration that this is not a real HTML page,
> i recall it took some time to figure out about about-filter, just
> "doing it" (cat(1))).
> 
> If i set "cache-size=0" then the desired page is delivered.
> Note the cache itself is managed as usual with the default
> cache-size=1999.
> 
> The files in the cache are 0600, i thought maybe that was it, but
> the setting is actively reverted to this mask (i never looked at
> cgit code except grepping the error).  It has always been so, the
> oldest cache entry was
> 
>   -rw---1 lighttpd lighttpd  3023 Mar 18  2018 d220
> 
> I am pretty sure cgit delivered some weeks ago, the most notable
> difference is that AlpineLinux switched to Lighttpd 1.4.56 then
> .57, which seems to have brought tremendous changes under the
> hood, like HTTP/2 support and OCSP as well as support for all the
> different TLS libraries, whereas before it only was OpenSSL and
> compatibles, i think.  We have HTTP/2 not enabled yet.
> 
> Anything i can do about this?

Have you looked at the log output?

This error comes from cgit.c::cmd_main() as a result of
cache.c::cache_process() returning an error.  It looks like all of the
error paths there will write a more detailed error message to stderr.

I don't use lighttpd so I don't know whether that output is captured,
but normally output written to stderr will end up in a log file.

>From a quick look at the code, given that the problem seems to be caused
by updating the web server, my guess is that you'll see the error:

[cgit] error printing cache ...: Invalid argument (22)

and this may be caused by sendfile(2) failing due to some difference in
how the web server is setting up the output file descriptor.  You may
want to rebuild CGit without HAVE_LINUX_SENDFILE and see if that works.

(If it does, we should add better fallback handling for the case where
stdout is not a valid output for sendfile().)


John

Re: Opening each logo-link in a separate browser tab

[John Keeping]

On Thu, Sep 10, 2020 at 11:36:10AM +0200, jean-christophe manciot wrote:
> Is there a special format for that cgitrc keyword to open each
> logo-link in a different tab/window?
> 
> It could be for instance something like :
> logo-link="url" target="_blank"
> 
> where url is a valid Uniform Resource Locator.

There's no way to do this without patching the code in print_header() at
the moment.

You could inject a script via head-include to modify the page or
intercept the link, but of course that won't work for users with scripts
disabled.


John

Re: Possible case-sensitivity issue in module-link

[John Keeping]

On Sat, Aug 29, 2020 at 12:03:49PM +0100, Gianni Ceccarelli wrote:
> I use cgit on my own site https://www.thenautilus.net/cgit/
> 
> Some of my repositories contain submodules, for example
> https://www.thenautilus.net/cgit/Sietima/tree/docs/presentation/
> https://www.thenautilus.net/cgit/thermostat/tree/
> https://www.thenautilus.net/cgit/thermostat/tree/sensor/
> https://www.thenautilus.net/cgit/lego-piano/tree/
> https://www.thenautilus.net/cgit/lego-piano/tree/3d-print
> 
> All those submodules are listed in the repo's config file, for
> example ``thermostat/config`` contains::
> 
>   [cgit "module-link"]
> esp8266-oled-ssd1306 = 
> https://github.com/ThingPulse/esp8266-oled-ssd1306/tree/%s
> DHTesp = https://github.com/beegee-tokyo/DHTesp/tree/%s
> bt-server = https://www.thenautilus.net/cgit/gobbledegook/tree/?id=%s
> 
> and ``lego-piano/config`` contains::
> 
>   [cgit "module-link"]
> RingBuffer = https://github.com/Locoduino/RingBuffer/tree/%s
> ESP8266Audio = https://github.com/earlephilhower/ESP8266Audio/tree/%s
>   [cgit "module-link.3d-print/LEGO"]
> scad = https://github.com/cfinke/LEGO.scad/tree/%s
> 
> You may have noticed that only *some* submodules show up as links in
> the CGit output:
> 
> - ``esp8266-oled-ssd1306``, ``bt-server``, ``LEGO.scad`` work
> - ``DHTesp``, ``RingBuffer``, ``ESP8266Audio`` don't
> 
> My suspicion is that the case of the characters after the last ``.``
> is involved: if they're all lowercase, the link works, if some are
> uppercase, it doesn't.
> 
> Running ``git config -l`` in the ``lego-piano`` repository directory,
> for example, I get::
> 
>   cgit.module-link.ringbuffer=https://github.com/Locoduino/RingBuffer/tree/%s
>   
> cgit.module-link.esp8266audio=https://github.com/earlephilhower/ESP8266Audio/tree/%s
>   
> cgit.module-link.3d-print/LEGO.scad=https://github.com/cfinke/LEGO.scad/tree/%s
> 
> notice that ``ringbuffer`` and ``esp8266audio`` got lowercased, but
> ``LEGO`` didn't. If the configuration is parsed like this, and the
> submodule's path is then matched case-sensitively, the result would be
> what I observe.
> 
> Can anyone confirm? I've been reading the source of CGit plus the bits
> of Git that are linked in, but I haven't found code that would produce
> the behaviour I see.

Yes, Git's config parsing converts all keys to lowercase (see
git/config.c::get_value() and its caller).

Does the patch below fix the bug?

-- >8 --
Subject: [PATCH] shared: compare submodules case-insensitively

If module-link is read from a repository's Git config, then our callback
is invoked with the name converted to lower case (as all Git config keys
are).  This means we can never match a submodule containing uppercase
characters against such a module-link.

Compare submodule names case-insensitively to avoid this problem.  This
may break a use case where two submodules differ only in case, but that
is much less likely than a submodule name using uppercase characters.

Signed-off-by: John Keeping 
---
 shared.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/shared.c b/shared.c
index 609bd2a..777618d 100644
--- a/shared.c
+++ b/shared.c
@@ -77,6 +77,7 @@ struct cgit_repo *cgit_add_repo(const char *url)
ret->owner_filter = ctx.cfg.owner_filter;
ret->clone_url = ctx.cfg.clone_url;
ret->submodules.strdup_strings = 1;
+   ret->submodules.cmp = strcasecmp;
ret->hide = ret->ignore = 0;
return ret;
 }
-- 
2.28.0

Re: renamed default branch, no "Idle" value on index at all

[John Keeping]

On Thu, Aug 20, 2020 at 01:07:49PM +0200, François Kooman wrote:
> I switched the default branch from master to main in some of my 
> repositories. However, it seems cgit does not consider this when 
> determining "Idle" on the index page. It shows nothing in the column "Idle".
> 
> The FAQ [1] has an entry about this, but I am not seeing what the FAQ 
> describes...I modified the HEAD file in the repository on the server:
> 
> # git symbolic-ref HEAD refs/heads/main
> 
> Everything seems to work fine, cgit takes "main" as the default branch. 
> It seems cgit is hard coded to only look at "master" to determine idle 
> on the index?

It looks like the "guess default branch" logic isn't considered for the
index page.

If you set "repo.defbranch" in cgitrc (or via git config in the
repositories) then you can manually specify the default branch and that
will work everywhere.

Alternatively you can use a hook to update an agefile and that will be
read instead of the default branch.  This also allows setting the last
modified time to be the newest across a collection of branches.

I had a quick look at guessing the default branch (from HEAD) for the
repolist, but that's not simple because it requires initialising Git in
each repository (see cgit.c::guess_defbranch() which is used for repo
pages).


John

Re: [feature request] Provide Atom feed for tags

[John Keeping]

On Tue, Aug 13, 2019 at 06:28:24PM +, Yu Franklin wrote:
> Does CGit currently provide Atom feed for tags of a repository?
> Something similar to
> 
> https://github.com/git/git/tags.atom
> 
> I know that we have Atom feed for a branch, but not for all tags of a
> certain repository. Typically repositories use tags for releases; with
> Atom feed for tags we can track releases of a project.

No, this is not implemented at the moment.  It seems like a useful
feature, but I think falls in the "patches welcome" category :-)
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: Nesting repositories under a common folder?

[John Keeping]

On Wed, Jul 24, 2019 at 02:05:38PM -0500, Reuben Popp wrote:
> Excuse me if I sound a bit daft here... can I add javascript to cgit, or
> how else would I collapse reposection classes?

You can use the "head-include" directive in cgitrc to include arbitrary
content in the  element generated by CGit.


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: Nesting repositories under a common folder?

[John Keeping]

On Wed, Jul 24, 2019 at 01:01:27PM -0500, Reuben Popp wrote:
> Excuse me if this has been answered before, but is there a way to nest
> repositories under a common directory visually in cgit?
> 
> For example,
> 
> root_directory (not a repo)
> |- Project A (directory, not a repo)
> |   |- Project A component 1 (repo)
> |   |- Project A component 2 (repo)
> |- Project B (repo)
> |- Project C (directory, not a repo)
> |   |- Project C subproject AA (repo)
> |   |- Project C subproject AB (repo)
> |   |- Project C Archive (directory, not a repo)
> |  |- Project C archived subproject A (repo)
> etc.
> 
> Going to the cgit frontend, I would see the directories (or links) for
> Project A, B and C...

You can't do exactly this, but you can generate sections from the path
to the repository which adds headings to the list of repositories.  See
"section" and "section-from-path" in cgitrc(5).

For hierarchical paths, it's also possible to view a subset of the tree
by going to (for example) /cgit/directory_a/directory_b/ but I don't
think anything generates links to those sublists.

I remember in the past somewhere had added a custom script to collapse
sections by default, but I can't find a reference for that now.  That
should be reasonably simple by matching on the "reposection" class.


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH] ui-shared: restrict to 15 levels

[John Keeping]

On Mon, May 20, 2019 at 09:55:06PM +0200, Jason A. Donenfeld wrote:
> Perhaps a more ideal version of this would be to not print breadcrumbs
> at all for paths that don't exist in the given repo at the given oid.
> 
> Signed-off-by: Jason A. Donenfeld 
> Reported-by: Fydor Wire Snark 
> ---
> I've committed this, and it works. But if anyone would like to give the
> implementation a stab, I think a better approach might be simply
> skipping printing of breadcrumbs in cases where the path doesn't exist
> in the repo. This way we're not limited to some reasonable constant such
> as 15.

That works for some pages, but I'm not sure it works in general (or at
least it's not always trivial to figure out if the path exists).

One example is ui-log, where it's perfectly reasonably to ask for the
log restricted to a path which doesn't exist in HEAD, and I'm not sure
at what point we decide that checking if the path exists at any point in
the history is more expensive than printing a long path here.

>  ui-shared.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index d27a5fd..d2358f2 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -945,12 +945,13 @@ static void cgit_print_path_crumbs(char *path)
>  {
>   char *old_path = ctx.qry.path;
>   char *p = path, *q, *end = path + strlen(path);
> + int levels = 0;
>  
>   ctx.qry.path = NULL;
>   cgit_self_link("root", NULL, NULL);
>   ctx.qry.path = p = path;
>   while (p < end) {
> - if (!(q = strchr(p, '/')))
> + if (!(q = strchr(p, '/')) || levels > 15)
>   q = end;
>   *q = '\0';
>   html_txt("/");
> @@ -958,6 +959,7 @@ static void cgit_print_path_crumbs(char *path)
>   if (q < end)
>   *q = '/';
>   p = q + 1;
> + ++levels;
>   }
>   ctx.qry.path = old_path;
>  }
> -- 
> 2.21.0
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/1] RFC: git: update to v2.21.0-rc0

[John Keeping]

On Thu, Feb 07, 2019 at 11:21:01AM +0100, Christian Hesse wrote:
> From: Christian Hesse 
> 
> Update to git version v2.21.0-rc0. Required changes follow upstream commits:
> 
> * 6a7895fd8a3bd409f2b71ffc355d5142172cc2a0
>   (commit: prepare free_commit_buffer and release_commit_memory for
>   any repo)
> 
> * e092073d643b17c82d72cf692fbfaea9c9796f11
>   (tree.c: make read_tree*() take 'struct repository *')
> 
> * ace5707a803eda0f1dde3d776dc3729d3bc7759a
>   (banned.h: mark strncat() as banned)

Can we pull out the strncat changes and merge them in advance of the
version bump?  I don't think there's any dependency on Git changes for
these.

> Signed-off-by: Christian Hesse 

Even without the strncat changes pulled out, this looks sensible, so:

Reviewed-by: John Keeping 

> ---
>  Makefile|  4 ++--
>  git |  2 +-
>  ui-atom.c   |  2 +-
>  ui-blame.c  |  4 ++--
>  ui-blob.c   |  9 ++---
>  ui-log.c|  4 ++--
>  ui-plain.c  |  3 ++-
>  ui-ssdiff.c |  4 ++--
>  ui-stats.c  |  2 +-
>  ui-tree.c   | 10 ++
>  10 files changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index e690c7f..080426c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -14,8 +14,8 @@ htmldir = $(docdir)
>  pdfdir = $(docdir)
>  mandir = $(prefix)/share/man
>  SHA1_HEADER = 
> -GIT_VER = 2.20.0
> -GIT_URL = https://www.kernel.org/pub/software/scm/git/git-$(GIT_VER).tar.xz
> +GIT_VER = 2.21.0.rc0
> +GIT_URL = 
> https://www.kernel.org/pub/software/scm/git/testing/git-$(GIT_VER).tar.xz
>  INSTALL = install
>  COPYTREE = cp -r
>  MAN5_TXT = $(wildcard *.5.txt)
> diff --git a/git b/git
> index 5d826e9..d62dad7 16
> --- a/git
> +++ b/git
> @@ -1 +1 @@
> -Subproject commit 5d826e972970a784bd7a7bdf587512510097b8c7
> +Subproject commit d62dad7a7dca3f6a65162bf0e52cdf6927958e78
> diff --git a/ui-atom.c b/ui-atom.c
> index 3866823..c182fad 100644
> --- a/ui-atom.c
> +++ b/ui-atom.c
> @@ -140,7 +140,7 @@ void cgit_print_atom(char *tip, char *path, int max_count)
>   }
>   while ((commit = get_revision()) != NULL) {
>   add_entry(commit, host);
> - free_commit_buffer(commit);
> + free_commit_buffer(the_repository->parsed_objects, commit);
>   free_commit_list(commit->parents);
>   commit->parents = NULL;
>   }
> diff --git a/ui-blame.c b/ui-blame.c
> index c52cb9b..644c30a 100644
> --- a/ui-blame.c
> +++ b/ui-blame.c
> @@ -290,8 +290,8 @@ void cgit_print_blame(void)
>   walk_tree_ctx.match_baselen = (path_items.match) ?
>  basedir_len(path_items.match) : -1;
>  
> - read_tree_recursive(commit->maybe_tree, "", 0, 0, , walk_tree,
> - _tree_ctx);
> + read_tree_recursive(the_repository, commit->maybe_tree, "", 0, 0,
> + , walk_tree, _tree_ctx);
>   if (!walk_tree_ctx.state)
>   cgit_print_error_page(404, "Not found", "Not found");
>   else if (walk_tree_ctx.state == 2)
> diff --git a/ui-blob.c b/ui-blob.c
> index 4b6b462..30e2d4b 100644
> --- a/ui-blob.c
> +++ b/ui-blob.c
> @@ -56,7 +56,8 @@ int cgit_ref_path_exists(const char *path, const char *ref, 
> int file_only)
>   goto done;
>   if (oid_object_info(the_repository, , ) != OBJ_COMMIT)
>   goto done;
> - read_tree_recursive(lookup_commit_reference(the_repository, 
> )->maybe_tree, "", 0, 0, , walk_tree, _tree_ctx);
> + read_tree_recursive(the_repository, 
> lookup_commit_reference(the_repository, )->maybe_tree,
> + "", 0, 0, , walk_tree, _tree_ctx);
>  
>  done:
>   free(path_items.match);
> @@ -90,7 +91,8 @@ int cgit_print_file(char *path, const char *head, int 
> file_only)
>   type = oid_object_info(the_repository, , );
>   if (type == OBJ_COMMIT) {
>   commit = lookup_commit_reference(the_repository, );
> - read_tree_recursive(commit->maybe_tree, "", 0, 0, , 
> walk_tree, _tree_ctx);
> + read_tree_recursive(the_repository, commit->maybe_tree,
> + "", 0, 0, , walk_tree, _tree_ctx);
>   if (!walk_tree_ctx.found_path)
>   return -1;
>   type = oid_object_info(the_repository, , );
> @@ -146,7 +148,8 @@ void cgit_print_blob(const char *hex, char *path, const 
> char *head, int file_onl
>  
>   if ((!hex) && type == OBJ_COMMIT && path) {
>   commit = lookup_commit_reference(the_repository, );
> - read_tree_recursive(commit->maybe_tree, "

Re: Slash after /about

[John Keeping]

On Tue, Jan 29, 2019 at 09:21:01AM +, dilyan.palau...@aegee.org wrote:
> For CGI I use thttpd and it tends to remove terminating slashes in the 
> requests.
> 
> cgit wants to have slash after about/ .
> 
> So they do not work together, endless loop happens.  Proposed fix:
> 
> diff --git a/cmd.c b/cmd.c
> --- a/cmd.c
> +++ b/cmd.c
> @@ -40,16 +40,7 @@ static void atom_fn(void)
>  static void about_fn(void)
>  {
> if (ctx.repo) {
> -   size_t path_info_len = ctx.env.path_info ? 
> strlen(ctx.env.path_info) : 0;
> -   if (!ctx.qry.path &&
> -   ctx.qry.url[strlen(ctx.qry.url) - 1] != '/' &&
> -   (!path_info_len || ctx.env.path_info[path_info_len - 1] 
> != '/')) {
> -   char *currenturl = cgit_currenturl();
> -   char *redirect = fmtalloc("%s/", currenturl);
> -   cgit_redirect(redirect, true);
> -   free(currenturl);
> -   free(redirect);
> -   } else if (ctx.repo->readme.nr)
> +   if (ctx.repo->readme.nr)
> cgit_print_repo_readme(ctx.qry.path);
> else if (ctx.repo->homepage)
> cgit_redirect(ctx.repo->homepage, false);

According to commit d703480 ("about: always ensure page has a trailing
slash") the trailing slash is required for easy embedding of links to
other /about/ pages.

I suspect it's possible to append the trailing slash without going round
a redirect loop, but I don't think this patch will work.

(I'm also inclined to agree that thttpd is broken here and should pass
the URL to CGI scripts as it is received.)
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: How to check error during syntax highlighting

[John Keeping]

On Mon, Dec 24, 2018 at 06:37:12PM +0900, Katsuya Horiuchi wrote:
> I have been playing with filters, specifically syntax-highlighting.py,
> in order to render markdown in tree.
> However, I would see empty output when something goes wrong.
> 
> Is it possible to check error during syntax highlighting (i.e. take log)?

The filter's stderr stream may appear in the web server's log file.

I use a small Python program for experimenting with CGit, which will
forward stderr from the filter:

https://github.com/johnkeeping/git-instacgit

You can also simply run CGit in a terminal with a couple of environment
variables set, although this is not the most straightforward mechanism
to set up.  See cgit_url() in tests/setup.sh for an example of this.


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: How to format the content of all markdown files, not just the about pages?

[John Keeping]

On Sun, Dec 23, 2018 at 03:25:00PM +0100, jean-christophe manciot wrote:
> with the following setup, no markdown file is correctly formatted besides
> the about pages:
> *cgit 1.2.1-14-g55ebd5e*
> *git 1:2.20.1-1*
> in */etc/cgitrc*:
> -
> # Format markdown, restructuredtext, manpages, text files, and html files
> # through the right converters
> about-filter=/usr/local/lib/cgit/filters/about-formatting.sh
> 
> ## Search for these files in the root of the default branch of repositories
> ## for coming up with the about page:
> readme=:README.md
> -
> 
> The about pages are perfectly formatted, for instance here
> .
> But any other markdown page is not formatted, for instance here
> .
> Am I missing something or is this a missing feature?

It's not supported at the moment, although there have been various
patches posted to add support for rendered content in the tree view.

The most recent thread ends about here with a summary of the changes
needed to move forward:

https://lists.zx2c4.com/pipermail/cgit/2018-July/004127.html

I'd forgotten that this is still unresolved, so I may see if I have time
to pick it up again between now and the new year.


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/1] css: Change #git font-size to 13px

[John Keeping]

On Sun, Aug 26, 2018 at 02:59:36PM +0200, Petr Vorel wrote:
> Previous size font-size 10pt (which is 13.px) hide underscore,
> when used with highlight with background-color on modified lines.
> Here underscore of crypto_blkcipher is hidden on by background color of
> newly added line.
> 
>  struct p8_aes_ctr_ctx {
> - struct crypto_blkcipher *fallback;
> + struct crypto_skcipher *fallback;
>   struct aes_key enc_key;
>  };
> 
> Signed-off-by: Petr Vorel 
> ---
> This found on kernel cgit, tested locally. See:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/queue-4.14/crypto-vmx-use-skcipher-for-ctr-fallback.patch

Which browser do you see this with?

I've tried Firefox 61.0 and Chromium 69.0.3497.42 against that page and
I can't reproduce the bug you describe.

I'm not necessarily opposed to the change below, but it feels like this
is working around a browser bug rather than fixing a fundamental issue
with the design of the page.

> ---
>  cgit.css | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cgit.css b/cgit.css
> index d4aadbf..be4011a 100644
> --- a/cgit.css
> +++ b/cgit.css
> @@ -2,7 +2,7 @@ div#cgit {
>   padding: 0em;
>   margin: 0em;
>   font-family: sans-serif;
> - font-size: 10pt;
> + font-size: 13px;
>   color: #333;
>   background: white;
>   padding: 4px;
> -- 
> 2.18.0
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/1] filters: generate anchor links from markdown

[John Keeping]

On Sat, Aug 25, 2018 at 11:11:39AM -0600, Jason A. Donenfeld wrote:
> The effect is just anchor links, but it doesn't add an actual ToC, right?

>From the docs [0], it looks like it will add an actual ToC if you have a
placeholder in the document to request one.  But if you don't have that,
then it just generates anchors.

[0] https://python-markdown.github.io/extensions/toc/
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: The logo of the about page is never shown although it is displayed on all other pages

[John Keeping]

On Sat, Aug 25, 2018 at 09:41:44AM +0200, jean-christophe manciot wrote:
> I use *v1.2.1* with the patch regarding the table of contents
> 
>  .
> I have the following setup:
> /etc/cgitrc:
> logo=/cgit-css/cgit.png
> 
> Each repo has its own cgitrc, for instance:
> logo=https://git.sdxlive.com/PPA/plain/logo.png
> logo-link=https://help.ubuntu.com/lts/serverguide/index.html
> 
> The logo is displayed correctly on all pages (summary, refs, log, ...)
> except on the about page where I get:

Can you share a public URL demonstrating the problem?

The code for outputting the page header (including the logo) is shared
for all pages so I can't see why it would output a broken link just on
the about page.
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/1] filters: generate anchor links from markdown

[John Keeping]

On Fri, Jul 13, 2018 at 09:48:27PM +0200, Christian Hesse wrote:
> From: Christian Hesse 
> 
> This makes the markdown filter generate anchor links for headings.
> 
> Signed-off-by: Christian Hesse 
> ---
>  filters/html-converters/md2html | 17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/filters/html-converters/md2html b/filters/html-converters/md2html
> index ebf3856..dc20f42 100755
> --- a/filters/html-converters/md2html
> +++ b/filters/html-converters/md2html
> @@ -3,6 +3,7 @@ import markdown
>  import sys
>  import io
>  from pygments.formatters import HtmlFormatter
> +from markdown.extensions.toc import TocExtension

Does this ship as part of python-markdown, or is it an additional
dependency?

>  sys.stdin = io.TextIOWrapper(sys.stdin.buffer, encoding='utf-8')
>  sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='utf-8')
>  sys.stdout.write('''
> @@ -48,10 +49,14 @@ sys.stdout.write('''
>  line-height: 1;
>  padding-left: 0;
>  margin-left: -22px;
> -top: 15%}
> +top: 15%;
> +}
>  .markdown-body h1:hover a.anchor .mini-icon-link, .markdown-body h2:hover 
> a.anchor .mini-icon-link, .markdown-body h3:hover a.anchor .mini-icon-link, 
> .markdown-body h4:hover a.anchor .mini-icon-link, .markdown-body h5:hover 
> a.anchor .mini-icon-link, .markdown-body h6:hover a.anchor .mini-icon-link {
>  display: inline-block;
>  }
> +div#cgit .markdown-body h1 a.toclink, div#cgit .markdown-body h2 a.toclink, 
> div#cgit .markdown-body h3 a.toclink, div#cgit .markdown-body h4 a.toclink, 
> div#cgit .markdown-body h5 a.toclink, div#cgit .markdown-body h6 a.toclink {
> +color: black;
> +}
>  .markdown-body h1 tt, .markdown-body h1 code, .markdown-body h2 tt, 
> .markdown-body h2 code, .markdown-body h3 tt, .markdown-body h3 code, 
> .markdown-body h4 tt, .markdown-body h4 code, .markdown-body h5 tt, 
> .markdown-body h5 code, .markdown-body h6 tt, .markdown-body h6 code {
>  font-size: inherit;
>  }
> @@ -290,5 +295,13 @@ sys.stdout.write('''
>  sys.stdout.write("")
>  sys.stdout.flush()
>  # Note: you may want to run this through bleach for sanitization
> -markdown.markdownFromFile(output_format="html5", 
> extensions=["markdown.extensions.fenced_code", 
> "markdown.extensions.codehilite", "markdown.extensions.tables"], 
> extension_configs={"markdown.extensions.codehilite":{"css_class":"highlight"}})
> +markdown.markdownFromFile(
> + output_format="html5",
> + extensions=[
> + "markdown.extensions.fenced_code",
> + "markdown.extensions.codehilite",
> + "markdown.extensions.tables",
> + TocExtension(anchorlink=True)],
> + extension_configs={
> + "markdown.extensions.codehilite":{"css_class":"highlight"}})
>  sys.stdout.write("")
> ___
> CGit mailing list
> CGit@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/1] RFC: git: update to v2.19.0-rc0

[John Keeping]

On Tue, Aug 21, 2018 at 09:21:14AM +0200, Christian Hesse wrote:
> From: Christian Hesse 
> 
> Changelog to be writting... :)

More comments below, but it looks like there's a lot of unrelated
cleanups here.  I think only the the_repository parameter addition is
required for Git 2.19.

The other changes mostly look good, but I think they can be split out
into a separate series (which can probably land before Git 2.19 final is
released).

> Signed-off-by: Christian Hesse 
> ---
>  Makefile  |  4 ++--
>  cgit.h|  1 +
>  git   |  2 +-
>  parsing.c |  7 +++
>  shared.c  |  2 +-
>  ui-blame.c|  2 +-
>  ui-blob.c |  6 +++---
>  ui-clone.c|  4 ++--
>  ui-commit.c   |  4 ++--
>  ui-diff.c |  4 ++--
>  ui-log.c  |  4 ++--
>  ui-patch.c| 11 +++
>  ui-plain.c|  2 +-
>  ui-shared.c   |  6 +++---
>  ui-snapshot.c |  4 ++--
>  ui-ssdiff.c   |  9 +
>  ui-tag.c  |  4 ++--
>  ui-tree.c |  4 ++--
>  18 files changed, 42 insertions(+), 38 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 05ea71f..d67c8c6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -14,8 +14,8 @@ htmldir = $(docdir)
>  pdfdir = $(docdir)
>  mandir = $(prefix)/share/man
>  SHA1_HEADER = 
> -GIT_VER = 2.18.0
> -GIT_URL = https://www.kernel.org/pub/software/scm/git/git-$(GIT_VER).tar.gz
> +GIT_VER = 2.19.0.rc0
> +GIT_URL = 
> https://www.kernel.org/pub/software/scm/git/testing/git-$(GIT_VER).tar.gz
>  INSTALL = install
>  COPYTREE = cp -r
>  MAN5_TXT = $(wildcard *.5.txt)
> diff --git a/cgit.h b/cgit.h
> index 32dfd7a..bcc4fce 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -8,6 +8,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/git b/git
> index 53f9a3e..7e8bfb0 16
> --- a/git
> +++ b/git
> @@ -1 +1 @@
> -Subproject commit 53f9a3e157dbbc901a02ac2c73346d375e24978c
> +Subproject commit 7e8bfb0412581daf8f3c89909f1d37844e8610dd
> diff --git a/parsing.c b/parsing.c
> index 12453c2..7b3980e 100644
> --- a/parsing.c
> +++ b/parsing.c
> @@ -63,8 +63,7 @@ static char *substr(const char *head, const char *tail)
>   if (tail < head)
>   return xstrdup("");
>   buf = xmalloc(tail - head + 1);
> - strncpy(buf, head, tail - head);
> - buf[tail - head] = '\0';
> + strlcpy(buf, head, tail - head + 1);

Nice cleanup, but I don't think it is required for Git 2.19!

It's probably worth splitting this out for a separate patch that can
land before Git 2.19.

>   return buf;
>  }
>  
> @@ -78,7 +77,7 @@ static void parse_user(const char *t, char **name, char 
> **email, unsigned long *
>  
>   email_len = ident.mail_end - ident.mail_begin;
>   *email = xmalloc(strlen("<") + email_len + strlen(">") + 1);
> - sprintf(*email, "<%.*s>", email_len, ident.mail_begin);
> + xsnprintf(*email, email_len + 3, "<%.*s>", email_len, 
> ident.mail_begin);

Again, not related to Git 2.19.  I'm not sure about this one, snprintf
isn't adding anything because we know exactly how big the buffer is.
However, I bet static analysis warns about sprintf here even though this
use is safe.

Maybe we should just use a strbuf?

>  
>   if (ident.date_begin)
>   *date = strtoul(ident.date_begin, NULL, 10);

[snip the_repository conversion that all looks good]

> diff --git a/ui-log.c b/ui-log.c
> index d696e20..3bcb657 100644
> --- a/ui-log.c
> +++ b/ui-log.c
> @@ -67,7 +67,7 @@ void show_commit_decorations(struct commit *commit)
>   while (deco) {
>   struct object_id peeled;
>   int is_annotated = 0;
> - strncpy(buf, prettify_refname(deco->name), sizeof(buf) - 1);
> + strlcpy(buf, prettify_refname(deco->name), sizeof(buf));

More cleanup to split out.

>   switch(deco->type) {
>   case DECORATION_NONE:
>   /* If the git-core doesn't recognize it,
> @@ -234,7 +234,7 @@ static void print_commit(struct commit *commit, struct 
> rev_info *revs)
>   strbuf_add(, "\n\n", 2);
>  
>   /* Place wrap_symbol at position i in info->subject */
> - strcpy(info->subject + i, wrap_symbol);
> + strlcpy(info->subject + i, wrap_symbol, subject_len - i 
> + 1);

More cleanup.

>   }
>   }
>   cgit_commit_link(info->subject, NULL, NULL, ctx.qry.head,
> diff --git a/ui-patch.c b/ui-patch.c
> index 8007a11..efd7a34 100644
> --- a/ui-patch.c
> +++ b/ui-patch.c
> @@ -11,13 +11,16 @@
>  #include "html.h"
>  #include "ui-shared.h"
>  
> +/* two SHA1 hashes with two dots in between and termination */
> +#define REV_RANGE_LEN 2 * 40 + 3

Should this use GIT_MAX_HEXSZ ?

> +
>  void cgit_print_patch(const char *new_rev, const char *old_rev,
> const char *prefix)
>  {
>   struct rev_info rev;
>   struct commit *commit;
>   struct object_id 

Re: git-fsck complains about cgit repo

[John Keeping]

On Wed, Aug 15, 2018 at 09:32:18AM -0400, Konstantin Ryabitsev wrote:
> Since cgit is mirrored to git.kernel.org, which gets routine fsck 
> treatment, I started getting the following error reports:
> 
>   error: bad config line 5 in blob .gitmodules
>   error in blob 51dd1eff1edc663674df9ab85d2786a40f7ae3a5: gitmodulesParse: 
> could not parse gitmodules blob
> 
> I can easily reproduce this:
> 
> user@chatter:/tmp$ rpm -q git
> git-2.17.1-3.fc28.x86_64
> user@chatter:/tmp$ git clone --mirror https://git.zx2c4.com/cgit
> Cloning into bare repository 'cgit.git'...
> remote: Counting objects: 7043, done.
> remote: Compressing objects: 100% (2679/2679), done.
> remote: Total 7043 (delta 4933), reused 6211 (delta 4347)
> Receiving objects: 100% (7043/7043), 8.71 MiB | 1.89 MiB/s, done.
> Resolving deltas: 100% (4933/4933), done.
> user@chatter:/tmp$ cd cgit.git/
> user@chatter:/tmp/cgit.git$ git fsck
> Checking object directories: 100% (256/256), done.
> error: bad config line 5 in blob .gitmodules
> error in blob 51dd1eff1edc663674df9ab85d2786a40f7ae3a5: gitmodulesParse: 
> could not parse gitmodules blob
> Checking objects: 100% (7043/7043), done.
> user@chatter:/tmp/cgit.git$ 
> 
> Is that something that can be fixed, or should I just ignore this error?

I think this is historic because CGit had a submodule before
git-submodule was invented (in fact, looking at the history I think
git-submodule was derived from CGit's earlier implementation).

The format of .gitmodules in the original CGit implementation was
simpler than git-submodule uses and that is the source of the complaint.

If you can ignore it just for that blob that would be best, since I hope
we don't introduce invalid .gitsubmodule entries in the future.


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: How to apply about-formatting.sh to appropriate tree files

[John Keeping]

On Sat, Aug 11, 2018 at 01:51:09PM +0200, jean-christophe manciot wrote:
> I could not find a way with v1.2.1 to get any markdown/html files formatted
> correctly when read from the tree.
> I already successfully use the
> "about-filter=/usr/local/lib/cgit/filters/about-formatting.sh" setting in
> /etc/cgitrc, but it only pertains to the about page.
> How can we use this filter for all appropriate tree files?

This is not currently supported.

Some patches were posted a little while ago [1] which add this feature,
but they're not quite ready to be integrated yet.

The best summary of where we want to go is [2].

[1] https://lists.zx2c4.com/pipermail/cgit/2018-June/003978.html
[2] https://lists.zx2c4.com/pipermail/cgit/2018-July/004127.html
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: RFE: changelog between two revisions

[John Keeping]

On Tue, Jul 24, 2018 at 03:14:49PM -0400, Konstantin Ryabitsev wrote:
> A very minor feature request -- it would be nice if there was a way to 
> limit /log/ view to only changes between two revisions. In other words, 
> I was looking for a way to replicate this file:
> 
> https://cdn.kernel.org/pub/linux/kernel/v4.x/ChangeLog-4.17.8
> 
> using the /log/ view, but there's no way to tell it "stop when you reach 
> tag v4.17.7":
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/?h=v4.17.8=1

Something like this?

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/?qt=range=v4.17.7..v4.17.8=1

(although that's a poor example since v4.17.8 only contains a single
patch, this is perhaps clearer:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/?qt=range=v4.17.8..v4.17.9=1
)


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v4 00/16] Render READMEs inline in tree view

[John Keeping]

On Tue, Jul 03, 2018 at 09:34:26PM +0200, Jason A. Donenfeld wrote:
> On Thu, Jun 28, 2018 at 10:29 AM John Keeping  wrote:
> > Yeah, I don't think there's any way to avoid exec'ing twice in source
> > view - we need to run the source filter for output and we need the
> > render filter to tell us whether we should output a link to the rendered
> > content.
> 
> Let's do it this way then. Since rendering anyway usually amounts to:
> 
> case "$(printf '%s' "$1" | tr '[:upper:]' '[:lower:]')" in
> *.markdown|*.mdown|*.md|*.mkd) exec ./md2html; ;;
> *.rst) exec ./rst2html; ;;
> *.[1-9]) exec ./man2html; ;;
> *.htm|*.html) exec cat; ;;
> *.txt|*) exec ./txt2html; ;;
> esac
> 
> Then we can just reimplement the dispatcher in Lua, and this whole
> issue goes away.

That sounds sensible, although the dispatcher will be getting a bit more
complicated to support the file formats for which we want to output an
 or  tag.

> > That's what the asset-path feature elsewhere in this series does,
> > especially if we take your idea of passing the URL path to the file
> > being rendered.  So we should make that change and pass the path to the
> > file instead of just a directory.
> 
> Right.
> 
> > It's /source/ instead of /tree/ in the current series and I think that's
> > better than a query parameter since this is a somewhat different view.
> 
> I saw that. And indeed that seems like a good way of doing it.
> 
> >
> > > - When source-filter is used, cgit prints its usual line numbers.
> > > - If source-filter fails, we print our nice hexdump or plaintext printer.
> >
> > These two are "source-filter usage is unchange", right?
> 
> Yes.
> 
> > Yes, that looks good to me, modulo figuring out exactly how
> > render-filter operates.
> 
> render-filter will operate the same way as the current about-filter,
> with two exceptions:
> 
> - If it returns error 127, then it means rendering wasn't supported
> and we should fall back to source view (in the case of it being loaded
> from /tree).
> - It is passed a full path to the raw files, so that it can generate
> correct relative includes.
> 
> I think with that settled (if you agree), the previous series can be
> reworked to function in this manner? The full plan is looking like:
> 
> - We retain commit-filter, source-filter, and owner-filter as we have them 
> now.
> - We rename about-filter to render-filter.
> - render-filter gets passed a fuller filename with useful path
> information for printing out iframes, img tags, and so forth.
> - render-filter returns 127 if it does not have a renderer for that file.
> - Viewing files in /tree defaults to render-filter. If that returns
> 127, it falls back to source-filter.
> - Viewing files in /source goes straight to source-filter and skips
> render-filter.
> - When render-filter is used in /tree, cgit shows in the top bar a
> helpful link to go to /source.
> - We remove readme and instead introduce readme-filename instead.
> - We introduce a global and a .repo level about-readme that takes the
> above syntax.
> - We introduce tree-readme=1/0.

Yes, this sounds good to me.

I think backwards compatibility also falls out quite easily in this
approach:

- "about-filter" becomes a deprecated alias for "render-filter"
- "readme" becomes a deprecated alias for "about-readme"

If I can have one more bikeshed... I wonder if "about-content" is better
than "about-readme", the latter feels a bit like we're saying this same
thing twice.
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: md2html is inoperative

[John Keeping]

On Thu, Jun 28, 2018 at 12:50:49PM +0200, jean-christophe manciot wrote:
> On Ubuntu 18.10 cosmic
> cgit 1.1+git2.10.2-3build1
> python3: 3.6.5-3
> python3-docutils: 0.14+dfsg-3
> python3-markdown: 2.6.9-1
> python3-pygments: 2.2.0+dfsg-1
> 
> In /etc/cgitrc:
> about-filter=/usr/lib/cgit/filters/about-formatting.sh
> 
> Despite that setting, the README.md file loaded from the  link is
> not formatted correctly. It seems that no conversion takes place.

Are you sure that there isn't a repo-specific override in place for the
about-filter?  And that you're not seeing cached content immediately
after changing the setting?

You can test CGit from the command line to rule out a misconfiguration
in the web server:

CGIT_CONFIG=/etc/cgitrc cgit --query=url=repo/about/

> # about-formatting.sh README.md 

Note that for testing you should try something like:

about-formatting.sh README.md https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v4 16/16] md2html: change css name to not conflict with highlight

[John Keeping]

On Thu, Jun 28, 2018 at 05:58:42AM +0800, Andy Green wrote:
> On 06/28/2018 01:37 AM, Jason A. Donenfeld wrote:
> > This seems like an obvious thing to merge, but I'm actually not so
> > certain I understand its necessity. md2html uses the highlight class.
> > Our css uses the highlight class. You're saying this conflicts with
> > something? From where? Third-party CSS? If that's the case, and if
> 
> I'm saying blame renders wrongly on Fedora 28 for these reasons and the 
> patch fixes it.
> 
> > it's a serious problem to recon with, do we then want to just
> > namespace all of our classes as .cgit subclasses, or as
> > .cgit-highlight, .cgit-whatever prefixes?
> 
> I dunno what you want to do about it, but I wanted it to render 
> correctly on Fedora 28, which this patch is enough to do.

>From the original discussion, I concluded that the problem is that
both Pygments and md2html use "highlight" as a CSS class and previously
we never output both of these on the same page.

For some reason, Andy is getting both of these outputting their styles
in ui-blame.  I can't see how md2html output is ending up in that page
though.


John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v4 00/16] Render READMEs inline in tree view

[John Keeping]

On Thu, Jun 28, 2018 at 01:22:34AM +0200, Jason A. Donenfeld wrote:
> Hey John,
> 
> Thanks tons for your input, as always.
> 
> On Wed, Jun 27, 2018 at 9:51 PM John Keeping  wrote:
> > - It is desirable to have the existing source view in addition to the
> >   rendered content, preferably with syntax highlighting via the source
> >   filter; for example Markdown, HTML or SVG can be sensibly viewed in
> >   both ways
> 
> Got it, so this is the big kicker. Essentially we need a nice way for
> the render filter to fallback to the source filter, as you said, with
> then a `=1` override (and link) if a user explicitly wants the
> source, in order to have both views. That seems like a good idea. This
> would solve the issue with line numbers as well.
> 
> But how to implement this efficiently? With the lua filters, it really
> doesn't matter, but with the exec filters, we perhaps incur the
> penalty of exec'ing twice. Maybe that's acceptable, though. The other
> approach would be for the source-filter to print its own line numbers,
> and so the render filter itself could just fall back to calling
> internally the source filter, and perhaps even indicating what it did
> via the exit code, but I think I like this idea less. A third option
> is to do this selection entirely within cgit via mimetype/fileext
> selection, while I was initially hesitant about this, maybe this is an
> okay approach after all.
> 
> Assuming we go with the first approach -- of the renderer fallback --
> we would then have:
> 
> - render-filter (nee about-filter)
> - source-filter
> 
> Then, if render-filter returns an exit code, cgit assumes we're in
> source filter mode.

Yeah, I don't think there's any way to avoid exec'ing twice in source
view - we need to run the source filter for output and we need the
render filter to tell us whether we should output a link to the rendered
content.

If we've been asked to render and can render, then the render filter can
just run and exit normally, but if it wants to say "unsupported" then we
have to exec the source filter as well.

There is an alternative if we're willing to move away from simple
filters and introduce a new filter type which provides headers before
its output, for example:

Mode: source
Supported-modes: source, render



I'm not convinced that this is a good thing, but the only other way to
avoid exec'ing two processes is to configure in CGit.  My hesitation
with that is that the only way we have to do that is via file extensions
and I don't really like tying everything to a file extension ("README"
is a perfectly fine filename and we should be able to display that as
rendered Asciidoc if that's what it is).

> > - For some content types, the easiest way to render it is to simply
> >   include the file with an iframe; this is the case for images and PDFs
> >   at the moment
> >
> > Now, if the render filter can say "I don't support this" and can do so
> > reasonably efficiently, we can use that to control whether render mode
> > is enabled and whether we use the "fallback to mimetype" to include an
> > iframe.  Or with the asset path extension to the filter arguments, we
> > could have the filter generate the  element itself [2].
> 
> This should certainly be up to the actual render script. This means
> we'll need to path sensible blob paths so that the render script can
> correctly fill in  correctly.

That's what the asset-path feature elsewhere in this series does,
especially if we take your idea of passing the URL path to the file
being rendered.  So we should make that change and pass the path to the
file instead of just a directory.

> > I agree with unifying the concepts, but might want to bikeshed the name
> > since if we go for render mode on all files then it applies to a bit
> > more than just readme files.
> 
> You're right about that. render-filter it is then.
> 
> > I think a distinction between repo-level readme and readme that can
> > apply to each directory is useful.  We can probably also fall back to
> > directory-level readme config using the default branch and root
> > directory if the repo-level config is unset.
> >
> > I'm not sure there's much value in removing blob references from
> > about-readme, although extending it to allow specifying a tree and then
> > looking for the readme filename would be neat.
> 
> Yes, right. So, about-readme takes these values:
> 
> about-readme=/path/to/absolute/file/not/in/a/git/repo
> about-readme=BRANCH:file/in/branch.md
> about-readme=BRANCH:# selects a "readme" file from BRANCH
> about-readme=:file/in/default/branch/stuff.md   # selects stuff.md
> from default bra

Re: [PATCH v3 1/1] snapshot: support tar signature for compressed tar

[John Keeping]

On Wed, Jun 27, 2018 at 06:34:56PM +0200, Jason A. Donenfeld wrote:
> I've merged all the surrounding changes, but I'm not quite satisfied
> with the implementation of this one.
> 
> > +   for (f_tar = cgit_snapshot_formats; strcmp(f_tar->suffix, ".tar") 
> > != 0; f_tar++)
> > +   /* nothing */ ;
> > +
> > +   } else if (starts_with(f->suffix, ".tar") && 
> > cgit_snapshot_get_sig(ref, f_tar)) {
> > +   strbuf_setlen(, strlen(filename.buf) - 
> > strlen(f->suffix));
> > +   strbuf_addstr(, ".tar.asc");
> > +   html(" (");
> > +   cgit_snapshot_link("sig", NULL, NULL, NULL, NULL,
> > +  filename.buf);
> > +   html(")");
> 
> Can we, instead, _not_ special case .tar, but rather just allow for
> all signatures, if the note .asc exists? We don't want to serve
> arbitrary tarballs and archives, because this means load and bandwidth
> for the server that wasn't explicitly opted in by the admin, but all
> signatures are necessarily explicitly uploaded, so why restrict them
> from being downloaded?

I'm not quite sure what you're asking here, this is just printing the
signature link after the snapshow download link.

The idea here is that if you are downloading a .tar.gz then the
signature for the base .tar is better (it's easier to consistently
generate a .tar than it is a .tar.gz), so the admin will choose to
provide .tar.asc instead of .tar.gz.asc.

I would quite like to avoid special-casing .tar in the code like this
and instead allow a fallback option (or even bitmask) in the formats
table as a more generic implementation, but I don't think that's your
complaint here (I also don't think we'll ever add it for other formats,
so hardcoding .tar isn't too bad).


John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: Fancier Source view [Was: Re: [PATCH v4 00/16] Render READMEs inline in tree view]

[John Keeping]

On Wed, Jun 27, 2018 at 07:26:13PM +0200, Jason A. Donenfeld wrote:
> Splitting out this issue into a different thread, because I think it's
> orthogonal to the other topic.
> 
> On Wed, Jun 27, 2018 at 7:18 PM Jason A. Donenfeld  wrote:
> > In adding rendering of arbitrary files in blob view, this is
> > essentially a fancy source view, with the one caveat of our
> > interesting handling of line numbers.
> 
> People click a .c file and they get pygments highlighting. That's
> good. People click a .pdf or a .md and they get either a hexdump or a
> highlighted .md source. Github has made different behavior more
> popular -- namely, rendering the .pdf and the .md. The obvious way to
> handle this is augmenting the source-view to spit something different
> out.
> 
> Alternatively, my previously proposed readme-filter could be renamed
> to render-filter, and we could do everything in one place with one
> filter, and decide exclusively on the basis of filetype.
> 
> Either way, we have this issue of line numbers. We have a few ways of
> handling this:
> 
> a. Not print them ever when a source-filter/render-filter is
> specified, leaving that up to the renderer to do.
> b. If the renderer doesn't want line numbers, it can return a special
> exit code (197, for example) to indicate that to cgit.
> c. If the renderer doesn't want line numbers, it can just omit some
> !important css to remove them at the end.
> d. We introduce a maze of filetype selection rendering logic options.
> e. Something else?
> 
> I'm most inclined to go with a or b, but neither seems entirely
> appealing. Thoughts?

d. is basically what we have with "render." in this series, but I
think e. might be better.  Specifically, I think we shouldn't conflate
source and rendered views because syntax highlighted Markdown, or SVG,
or HTML is still potentially useful.

If source and rendered views are distinct, then source view always has
line numbers (or it's a hex dump) and we just need the filter to return
a special exit status to mean "this file is unsupported", and we disable
the rendered view when that happens.

As an extension, we could disable the source view if rendering is
supported and the file looks like binary, so that the hex dump isn't
available when we have a better option.


John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v4 11/16] md2html: add asset mapping

[John Keeping]

On Wed, Jun 27, 2018 at 07:32:56PM +0200, Jason A. Donenfeld wrote:
> On Wed, Jun 20, 2018 at 12:13 PM Andy Green  wrote:
> 
> > md2html  >
> > The trailing "/" is important.
> 
> Can we make it not important? That is, if the type is always
> explicitly a directory, treat it as such using the usual file name
> joining logic.

Seems sensible.  This was originally posted as a proof of concept, so
would benefit from a bit of tidying.

I'm not actually sure if it does the right think when cfg.virtual_root
is NULL so we may need something a bit more complicated than this (and I
can't see a way that doesn't require the filter to handle both types of
URL).

> Alternatively, and perhaps better, don't introduce a second argument.
> Just pass the full file path in the first argument, and have asset
> mapping always work it out in the right way.

It depends how we think about the first argument.  The URL of the
"current page" when we call the filter will be something like:

/repo/tree/subdir/README.md

but the asset path should point to:

/repo/plain/subdir/

We could just say it's the URL of the input file, which is actually
quite nice if we want the filter to be able to replace the content with
an iframe.

However, I think there's a big benefit in backwards compatibility if we
keep the first argument as a file name and don't change it to a URL.
But that doesn't stop us making the new argument into a URL to the file.


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v4 00/16] Render READMEs inline in tree view

[John Keeping]

Hi Jason,

On Wed, Jun 27, 2018 at 07:18:57PM +0200, Jason A. Donenfeld wrote:
> With the current state of this series, cgit would have the following options:
> 
> - render.
> - inline-readme
> - render-filter

This one is only a concept, not a configuration value (just a note since
I couldn't remember introducing it!)

> - about-filter
> - commit-filter
> - source-filter
> - owner-filter
> - readme=file
> - readme=:file
> 
> Whoa nelly. Of these, about-filter, commit-filter, source-filter, and
> owner-filter all have analogs in the repo.* namespace, which makes
> sense; it seems this was omitted from the render-filter introduced by
> this series. The thing that unifies about-filter, commit-filter,
> source-filter, and owner-filter is that they all modify some aspect of
> the rendered output, either via fork/exec or via lua.
> 
> In adding readme files under the tree view, the obvious observation is
> that this is pretty much the same type of rendering that we're doing
> in about-filter.
> 
> In adding rendering of arbitrary files in blob view, this is
> essentially a fancy source view, with the one caveat of our
> interesting handling of line numbers.
> 
> So, I'd propose the following re-organization (and after we nail it
> down, we can bikeshed about compatibility with old configs, but for
> now let's focus on ideal design):

I'll comment on the proposals inline below, but before doing that let me
try to remember why "render." exists in the first place (it's a
couple of years since I wrote that patch so I'm not entirely sure this
is the rationale I had at the time!)

Thinking though it now, I think about-filter and the render-filter
concept are equivalent.  And for the purpose of putting an inline
directory readme in the tree view, that may be sufficient.

However, my series was adding a new UI mode and there are a couple of
requirements there that I can't see a way to fix without render. or
something equivalent [1]:

- It is desirable to have the existing source view in addition to the
  rendered content, preferably with syntax highlighting via the source
  filter; for example Markdown, HTML or SVG can be sensibly viewed in
  both ways

- For some content types, the easiest way to render it is to simply
  include the file with an iframe; this is the case for images and PDFs
  at the moment

Now, if the render filter can say "I don't support this" and can do so
reasonably efficiently, we can use that to control whether render mode
is enabled and whether we use the "fallback to mimetype" to include an
iframe.  Or with the asset path extension to the filter arguments, we
could have the filter generate the  element itself [2].

> - We retain commit-filter, source-filter, and owner-filter as we have them 
> now.
> - We rename about-filter to readme-filter.

I agree with unifying the concepts, but might want to bikeshed the name
since if we go for render mode on all files then it applies to a bit
more than just readme files.

> - We remove `readme` and instead introduce `readme-filename`, which
> can be specified multiple times as is habit. This would simply take
> the set of filenames considered to be readme files (readme.md,
> readme.txt, etc). [Bikeshed discussion: case insensitive?]
> - We introduce an options at the global level and at the .repo level
> of `about-readme=/path/to/absolute/file` and `about-readme=:`
> and `about-readme=:`. The first would replace our original usage of
> `readme=/path/to=file`, and the second would replace the use of
> `readme=branch:whatever`, specifying an explicit branch (like
> cgit.git's wiki branch), and the third would indicate the default
> branch.

I think a distinction between repo-level readme and readme that can
apply to each directory is useful.  We can probably also fall back to
directory-level readme config using the default branch and root
directory if the repo-level config is unset.

I'm not sure there's much value in removing blob references from
about-readme, although extending it to allow specifying a tree and then
looking for the readme filename would be neat.

> - We introduce an option at the global level and at the .repo level of
> `tree-readme=1/0` to display (or not) the readme under each tree.

Agree.

> - We do not introduce render-filter. We do not introduce render.;
> such extension selection is successfully handled by the various
> filters themselves already.

Agree on render-filter, but we need a better proposal for how to handle
the  case and the "render not supported" case if we want to do
those outside the filter.


[1] This could be the exit status of the render filter, but I'm not sure
how well the "probe" mode will fit it
[2] We do load the content redundantly in this case and we'll have to
make sure that our SIGPIPE handling is correct if the filter exits
without reading all of its stdin, but that's probably acceptable


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com

Re: [PATCH v3 6/6] line-range-highlight: copy URL to clipboard on click

[John Keeping]

On Sun, Jun 24, 2018 at 11:06:45PM +0800, Andy Green wrote:
> On June 24, 2018 9:39:35 PM GMT+08:00, John Keeping  
> wrote:
> >On Sun, Jun 24, 2018 at 08:00:08PM +0800, Andy Green wrote:
> >> On June 24, 2018 7:42:33 PM GMT+08:00, John Keeping
> > wrote:
> >> >On Sun, Jun 24, 2018 at 10:44:54AM +0800, Andy Green wrote:
> >> >> Since the only reason to click on the line number links
> >> >> is to get the corresponding #URL to share, this patch
> >> >> makes that process more convenient by copying the
> >> >> highlit area, be it a single line or a range, to the
> >> >> clipboard on each click of the line number links.
> >> >
> >> >As a user, I'd find this surprising and probably quite annoying.
> >> >
> >> >I strongly prefer that software not overwrite the clipboard contents
> >> >without an explicit request to do so.  A quick survey suggests none
> >of
> >> >GitHub, Gitlab or repo.or.cz (GitWeb) behave in this way.
> >> 
> >> Is there another possible intention behind clicking on the line
> >number
> >> links I am missing?
> >
> >One I can think of (given your recent patches) is to highlight a line
> >when pointing it out to someone else looking at the same display.  I
> >have definitely used this with other web interfaces in the past.
> 
> Well, ok, so he can click on it to show his buddy.  He gets the URL in
> his clipboard.  But his buddy is there he doesn't need it.

And what about the piece of code that was cut into the clipboard just
before the interruption?

> >> If not, literally the only reason to click on them is because you
> >> intend to copy the #URL to the clipboard.
> >> 
> >> Then that is "an explicit request", isn't it?
> >
> >In the X11 world there are separate selection buffers for selecting
> >text
> >and for Ctrl-C/V behaviour [1].  It might not be unreasonable to set
> >PRIMARY to the URL, but setting CLIPBOARD is clearly wrong.
> >
> >My use of "explicit" above is consistent with the use in this spec,
> >unless the user has selected Copy, or pressed Ctrl-C there is no
> >expectation that the clipboard will be overwritten.
> >
> >Certainly for an X11 user overwriting CLIPBOARD when clicking a
> 
> The browser has no idea about that.  Cgit isn't an X11 beast, it feeds
> crossplatform browsers.
> 
> If the browser copies anything to the clipboard by js api, it's always
> the ^C ^V one since the js stuff only has the concept of that one.  So
> in the case the user wants this behaviour, the surprising thing would
> be on some platforms it went in some other "clipboard" (this is not
> possible in js afaik).  The user would be nonplussed if it could and
> then he was sat in front of a windows browser.  So I think the
> observations about that lead nowhere.
> 
> >hyperlink fails the principle of least surprise, and I think that
> 
> Well, I used it and I liked it.  It felt pretty natural.  But an extra
> click won't kill me.
> 
> >applies to all users because this isn't how web pages work: on any
> >other
> >page, if I want to copy a link I right-click and copy the link.  I
> >don't
> >expect the clipboard to be overwritten just because I clicked a
> >hyperlink.
> 
> On github it pops up a sticky popup with an ellipsis because you
> "clicked a hyperlink" in the same conditions.  (Clicking that brings a
> js menu with a copy entry).  Users can handle variation, at least if
> it's indicating to them what's happening and why, and is useful.  At
> least it doesn't seem to have held github back...

That's fine, it doesn't overwrite the clipboard with no notice (and
right-click still works).  My point isn't "we can't provide a new way to
copy to the clipboard", it's "we can't overwrite the clipboard behind
the user's back".

> The link we want to copy is not the ...#nXXX link in the a href when
> there is a range up.  I never tried to intercept "Copy Link", maybe it
> could be done.  But at least the default menu won't do as it is in
> this case.
> 
> I think no point for the two-step menu like github since the only
> option we plan to show atm is Copy (the other useful option is switch
> to blame at that line).  How about a popup that appears on the left
> for a few secs with a copy type icon, or "Copy", in it, that fades
> away if not used?  It's one extra click but still one less than
> github.

This sounds good.  Copying the line range link is clearly useful and
this way the user knows that it's an option and contro

Re: [PATCH v3 6/6] line-range-highlight: copy URL to clipboard on click

[John Keeping]

On Sun, Jun 24, 2018 at 08:00:08PM +0800, Andy Green wrote:
> 
> 
> On June 24, 2018 7:42:33 PM GMT+08:00, John Keeping  
> wrote:
> >On Sun, Jun 24, 2018 at 10:44:54AM +0800, Andy Green wrote:
> >> Since the only reason to click on the line number links
> >> is to get the corresponding #URL to share, this patch
> >> makes that process more convenient by copying the
> >> highlit area, be it a single line or a range, to the
> >> clipboard on each click of the line number links.
> >
> >As a user, I'd find this surprising and probably quite annoying.
> >
> >I strongly prefer that software not overwrite the clipboard contents
> >without an explicit request to do so.  A quick survey suggests none of
> >GitHub, Gitlab or repo.or.cz (GitWeb) behave in this way.
> 
> Is there another possible intention behind clicking on the line number
> links I am missing?

One I can think of (given your recent patches) is to highlight a line
when pointing it out to someone else looking at the same display.  I
have definitely used this with other web interfaces in the past.

> If not, literally the only reason to click on them is because you
> intend to copy the #URL to the clipboard.
> 
> Then that is "an explicit request", isn't it?

In the X11 world there are separate selection buffers for selecting text
and for Ctrl-C/V behaviour [1].  It might not be unreasonable to set
PRIMARY to the URL, but setting CLIPBOARD is clearly wrong.

My use of "explicit" above is consistent with the use in this spec,
unless the user has selected Copy, or pressed Ctrl-C there is no
expectation that the clipboard will be overwritten.

Certainly for an X11 user overwriting CLIPBOARD when clicking a
hyperlink fails the principle of least surprise, and I think that
applies to all users because this isn't how web pages work: on any other
page, if I want to copy a link I right-click and copy the link.  I don't
expect the clipboard to be overwritten just because I clicked a
hyperlink.

[1] https://specifications.freedesktop.org/clipboards-spec/clipboards-0.1.txt

> >> Signed-off-by: Andy Green 
> >> ---
> >>  cgit.js |   36 
> >>  1 file changed, 32 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/cgit.js b/cgit.js
> >> index 2cfad29..e2c3799 100644
> >> --- a/cgit.js
> >> +++ b/cgit.js
> >> @@ -89,8 +89,29 @@ function cgit_line_range_highlight()
> >>e.scrollIntoView(true);
> >>  }
> >>  
> >> +function cgit_copy_clipboard(value)
> >> +{
> >> +  var inp = document.createElement("textarea");
> >> +  var e = document.getElementById("linenumber_td");
> >> +
> >> +  inp.type = "text";
> >> +  inp.value = value;
> >> +  /* hidden style stops it working for clipboard */
> >> +  inp.setAttribute('readonly', '');
> >> +  inp.style.position = "absolute";
> >> +  inp.style.left = "-1000px";
> >> +
> >> +  e.appendChild(inp);
> >> +
> >> +  inp.select();
> >> +
> >> +  document.execCommand("copy");
> >> +
> >> +  inp.remove();
> >> +}
> >> +
> >>  function cgit_line_range_click(e) {
> >> -  var t = e.target.id;
> >> +  var t = e.target.id, cp;
> >>  
> >>cgit_line_range_override = null;
> >>  
> >> @@ -101,13 +122,13 @@ function cgit_line_range_click(e) {
> >> * is called, and override it there.
> >> */
> >>  
> >> -  if (window.location.hash && window.location.hash.indexOf("-") < 0)
> >> +  if (window.location.hash && window.location.hash.indexOf("-") < 0)
> >{
> >>if (parseInt(window.location.hash.substring(2)) <
> >>parseInt(t.substring(1))) /* forwards */
> >> -  cgit_line_range_override =
> >> +  cp = cgit_line_range_override =
> >>window.location + '-' + t.substring(1);
> >>else /* backwards */
> >> -  cgit_line_range_override =
> >> +  cp = cgit_line_range_override =
> >>window.location.href.substring(0,
> >>window.location.href.length -
> >>window.location.hash.length) +
> >> @@ -115,6 +136,13 @@ function cgit_line_range_click(e) {
> >>window.location

Re: [PATCH] cgit.css: add dynamic age update

[John Keeping]

Subject should be "cgit.js: " not cgit.css!

On Sun, Jun 24, 2018 at 03:59:51PM +0800, Andy Green wrote:
> This patch updates the emitted "ages" dynamically on the client side.
> 
> After updating on completion of the document load, it sets a timer
> to update according to the smallest age it found.  If there are any
> ages listed in minutes, then it will update again in 10s (this can
> never happen more than twelve times on a given page then).  When the
> most recent age is in hours, it updates every 5m.  If days, then
> every 30m and so on.
> 
> This keeps the cost of the dynamic updates at worst once per 10s
> and trending towards being trivially cheap after a couple of minutes.
> The updates are done entirely on the client side without contact
> with the server.
> 
> To make this work reliably, since parsing datetimes is unreliable in
> browser js, the unix time is added as an attribute to all age spans.
> 
> To make that reliable cross-platform, the unix time is treated as a
> uint64_t when it is formatted for printing.
> 
> The rules for display conversion of the age is aligned with the
> existing server-side rules in ui-shared.h.
> 
> If the client or server-side time are not synchronized by ntpd etc,
> ages shown on the client will not relate to the original ages computed
> at the server.  The client updates the ages immediately when the
> DOM has finished loading, so in the case the times at the server and
> client are not aligned, this patch changes what the user sees on the
> page to reflect patch age compared to client time.
> 
> If the server and client clocks are aligned, this patch makes no
> difference to what is seen on the page.
> 
> Signed-off-by: Andy Green 

If we're going to start depending on JS, should we take the robust
approach to this and never generate relative times on the server?  Or do
so only for ?

I'd really like to here other people's opinions on this.

Personally, I'm slightly negative because while I do regularly use some
sites that take this approach (including having long-lived tabs open on
pages like this), I always end up refreshing the page anyway when I go
back to the tab, because new content doesn't load without doing so. 

So I'm not sure what this tells me except "it's been at least N
{seconds,minutes,hours,days} since you last refreshed this page".  And
the first thing I'm going to do is hit refresh anyway.

> ---
>  cgit.h  |1 +
>  cgit.js |   58 ++
>  ui-shared.c |2 +-
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/cgit.h b/cgit.h
> index 2579ce4..9b21919 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /* Add isgraph(x) to Git's sane ctype support (see git-compat-util.h) */
>  #undef isgraph
> diff --git a/cgit.js b/cgit.js
> index e2c3799..e13faf2 100644
> --- a/cgit.js
> +++ b/cgit.js
> @@ -145,6 +145,61 @@ function cgit_line_range_click(e) {
>   cgit_copy_clipboard(cp);
>  }
>  
> +/* this follows the logic and suffixes used in ui-shared.c */
> +
> +var cgit_age_classes = [ "age-mins", "age-hours", "age-days",
> "age-weeks","age-months","age-years" ];
> +var cgit_age_suffix =  [ "min.", "hours", "days","weeks",
> "months","years", "years" ];
> +var cgit_age_next =[ 60, 3600,24 * 3600, 7 * 24 * 
> 3600,  30 * 24 * 3600,  365 * 24 * 3600, 365 * 24 * 3600 ];
> +var cgit_age_limit =   [ 7200,   24 * 7200,   7 * 24 * 7200, 30 * 24 * 
> 7200, 365 * 25 * 7200, 365 * 25 * 7200 ];
> +var cgit_update_next = [ 10, 5 * 60,  1800,  24 * 3600,  
> 24 * 3600,   24 * 3600,   24 * 3600 ];
> +
> +function cgit_render_age(e, age)
> +{
> + var t, n;
> +
> + for (n = 0; n < cgit_age_classes.length; n++)
> + if (age < cgit_age_limit[n])
> + break;
> +
> + t = Math.round(age / cgit_age_next[n]) + " " + cgit_age_suffix[n];
> +
> + if (e.textContent != t) {
> + e.textContent = t;
> + if (n == cgit_age_classes.length)
> + n--;
> + if (e.className != cgit_age_classes[n])
> + e.className = cgit_age_classes[n];
> + }
> +}
> +
> +function cgit_aging()
> +{
> + var n, next = 24 * 3600,
> + now_ut = Math.round((new Date().getTime() / 1000));
> +
> + for (n = 0; n < cgit_age_classes.length; n++) {
> + var m, elems = 
> document.getElementsByClassName(cgit_age_classes[n]);
> +
> + if (elems.length && cgit_update_next[n] < next)
> + next = cgit_update_next[n];
> +
> + for (m = 0; m < elems.length; m++) {
> + var age = now_ut - elems[m].getAttribute("ut");
> +
> + cgit_render_age(elems[m], age);
> + }
> + }
> +
> + /*
> +  * We only need to come back when 

Re: [PATCH v3 6/6] line-range-highlight: copy URL to clipboard on click

[John Keeping]

On Sun, Jun 24, 2018 at 10:44:54AM +0800, Andy Green wrote:
> Since the only reason to click on the line number links
> is to get the corresponding #URL to share, this patch
> makes that process more convenient by copying the
> highlit area, be it a single line or a range, to the
> clipboard on each click of the line number links.

As a user, I'd find this surprising and probably quite annoying.

I strongly prefer that software not overwrite the clipboard contents
without an explicit request to do so.  A quick survey suggests none of
GitHub, Gitlab or repo.or.cz (GitWeb) behave in this way.

> Signed-off-by: Andy Green 
> ---
>  cgit.js |   36 
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/cgit.js b/cgit.js
> index 2cfad29..e2c3799 100644
> --- a/cgit.js
> +++ b/cgit.js
> @@ -89,8 +89,29 @@ function cgit_line_range_highlight()
>   e.scrollIntoView(true);
>  }
>  
> +function cgit_copy_clipboard(value)
> +{
> + var inp = document.createElement("textarea");
> + var e = document.getElementById("linenumber_td");
> +
> + inp.type = "text";
> + inp.value = value;
> + /* hidden style stops it working for clipboard */
> + inp.setAttribute('readonly', '');
> + inp.style.position = "absolute";
> + inp.style.left = "-1000px";
> +
> + e.appendChild(inp);
> +
> + inp.select();
> +
> + document.execCommand("copy");
> +
> + inp.remove();
> +}
> +
>  function cgit_line_range_click(e) {
> - var t = e.target.id;
> + var t = e.target.id, cp;
>  
>   cgit_line_range_override = null;
>  
> @@ -101,13 +122,13 @@ function cgit_line_range_click(e) {
>* is called, and override it there.
>*/
>  
> - if (window.location.hash && window.location.hash.indexOf("-") < 0)
> + if (window.location.hash && window.location.hash.indexOf("-") < 0) {
>   if (parseInt(window.location.hash.substring(2)) <
>   parseInt(t.substring(1))) /* forwards */
> - cgit_line_range_override =
> + cp = cgit_line_range_override =
>   window.location + '-' + t.substring(1);
>   else /* backwards */
> - cgit_line_range_override =
> + cp = cgit_line_range_override =
>   window.location.href.substring(0,
>   window.location.href.length -
>   window.location.hash.length) +
> @@ -115,6 +136,13 @@ function cgit_line_range_click(e) {
>   window.location.href.substring(
>   window.location.href.length -
>   window.location.hash.length + 
> 2);
> + } else
> + cp = window.location.href.substring(0,
> +window.location.href.length -
> +window.location.hash.length) +
> + '#n' + t.substring(1);
> +
> + cgit_copy_clipboard(cp);
>  }
>  
>  /* line range highlight */
> 
> ___
> CGit mailing list
> CGit@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v3 5/6] line-range-highlight: onclick handler and range selection

[John Keeping]

On Sun, Jun 24, 2018 at 10:44:49AM +0800, Andy Green wrote:
> This allows the user to select line ranges simply by clicking on the
> line number links.
> 
>  - No selected highlit line, or a range already selected, causes the
> click to highlight just the clicked line as usual.
> 
>  - Clicking on a second line number link when a single line was already
> highlit creates a line range highlight, between the lowest and
> highest line numbers of the already-selected and newly-selected
> line number links.
> 
> The order of the clicks is unimportant, you can click the higher
> line number link first and then the lower to define the range of
> lines equally well.
> 
> The implementation is slightly complicated by our single parent
> onclick handler not being able to interrupt the already ongoing
> processing of the a href #n change from the link click itself.
> 
> Rather than bloat every linenumber link with its own onclick handler
> defeating this default we simply do it with a single parent
> onclick event and apply any computed range url in the existing
> hashchange event handler.
> 
> Signed-off-by: Andy Green 
> ---
>  cgit.js|   39 +++
>  ui-blame.c |2 +-
>  ui-tree.c  |2 +-
>  3 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/cgit.js b/cgit.js
> index 29b2c3d..2cfad29 100644
> --- a/cgit.js
> +++ b/cgit.js
> @@ -1,7 +1,15 @@
> +var cgit_line_range_override;
> +
>  function cgit_line_range_highlight()
>  {
>   var h = window.location.hash, l1 = 0, l2 = 0, e, t;
>  
> + if (cgit_line_range_override) {
> + window.location = cgit_line_range_override;
> + cgit_line_range_override = null;
> + return;
> + }
> +
>   e = document.getElementById("cgit_line_range");
>   if (e) {
>   l1 = e.l1;
> @@ -81,10 +89,41 @@ function cgit_line_range_highlight()
>   e.scrollIntoView(true);
>  }
>  
> +function cgit_line_range_click(e) {
> + var t = e.target.id;
> +
> + cgit_line_range_override = null;
> +
> + /*
> +  * We are called while window location update from the
> +  * click on the #n link is underway.  So stash any
> +  * computed range #url for when the hashchange hander
> +  * is called, and override it there.
> +  */
> +
> + if (window.location.hash && window.location.hash.indexOf("-") < 0)
> + if (parseInt(window.location.hash.substring(2)) <
> + parseInt(t.substring(1))) /* forwards */
> + cgit_line_range_override =
> + window.location + '-' + t.substring(1);
> + else /* backwards */
> + cgit_line_range_override =
> + window.location.href.substring(0,
> + window.location.href.length -
> + window.location.hash.length) +
> + '#n' + t.substring(1) + '-' +
> + window.location.href.substring(
> + window.location.href.length -
> + window.location.hash.length + 
> 2);
> +}
> +
>  /* line range highlight */
>  
>  document.addEventListener("DOMContentLoaded", function() {
>   cgit_line_range_highlight();
> + var e = document.getElementById("linenumber_td");
> + if (e)
> + e.addEventListener("click", cgit_line_range_click, true);
>  }, false);
>  window.addEventListener("hashchange", function() {
>   cgit_line_range_highlight();
> diff --git a/ui-blame.c b/ui-blame.c
> index 50d0580..0ba8a5a 100644
> --- a/ui-blame.c
> +++ b/ui-blame.c
> @@ -170,7 +170,7 @@ static void print_object(const struct object_id *oid, 
> const char *path,
>  
>   /* Line numbers */
>   if (ctx.cfg.enable_tree_linenumbers) {
> - html("");
> + html("");

We normally quote attribute values in all cases (like for "class" here),
and I think we choose kebab-case over snake_case for HTML identifiers,
by the same principle that we do so for CSS classes.

But can we just call this "linenumbers"?  I don't think we need the
element type in the identifier.

>   for (ent = sb.ent; ent; ent = ent->next) {
>   html("");
>   emit_blame_entry_linenumber(ent);
> diff --git a/ui-tree.c b/ui-tree.c
> index e4cb558..6759b11 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -28,7 +28,7 @@ static void print_text_buffer(const char *name, char *buf, 
> unsigned long size)
>   html("\n");
>  
>   if (ctx.cfg.enable_tree_linenumbers) {
> - html("");
> + html("");
>   idx = 0;
>   lineno = 0;
>  
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v3 2/6] ui-shared: line range highlight: introduce javascript

[John Keeping]

The subject is "ui-shared: ..." but should be "cgit.js" now I think.

On Sun, Jun 24, 2018 at 10:44:34AM +0800, Andy Green wrote:
> diff --git a/cgit.js b/cgit.js
> index e69de29..501c98f 100644
> --- a/cgit.js
> +++ b/cgit.js
> @@ -0,0 +1,53 @@
> +function cgit_line_range_highlight()
> +{
> + var h = window.location.hash, l1 = 0, l2 = 0, e, t;
> +
> + l1 = parseInt(h.substring(2));
> + t = h.indexOf("-");
> + if (t >= 1)
> + l2 = parseInt(h.substring(t + 1));
> + else
> + l2 = l1;
> +
> + if (!l1)
> + return;
> +
> + var lh, t = 0, e1, e2, de;
> +
> + e1 = e = document.getElementById('n' + l1);
> + if (!e)
> + return;
> +
> + while (e1) { if (e1.offsetTop) t += e1.offsetTop; e1 = e1.offsetParent; 
> }
> +
> + de = document.createElement("DIV");
> +
> + de.className = "selected-lines";
> + de.style.bottom = e.style.bottom;
> + de.style.top = t + 'px';
> +
> + /* DOM structure is different by one level for /blame/ */
> + e1 = e.parentElement.parentElement.parentNode;
> + if (e1.offsetWidth < e1.parentNode.offsetWidth) /* blame */
> + e1 = e1.parentNode;

I guess this is trying to find the  element, so why not look for
that directly?

while (e1.tagName.toUpperCase() != 'TR')
e1 = e1.parentNode;

> + de.style.width = e1.offsetWidth + 'px';
> + de.style.left = e1.parentNode.parentNode.offsetLeft + 'px';

Again, if we're looking for the  here, can we make this more
robust by looking for it explicitly?

> + de.style.height = ((l2 - l1 + 1) * e.offsetHeight) + 'px';
> +
> + e.parentElement.parentElement.insertBefore(de, 
> e.parentElement.parentElement.firstChild);

If I'm following correctly, e.parentElement.parentElement is either 
in tree view or  in blame view.  Is that expected?

My thinking is that it is correct, but what we're looking for is the
parent of the  element containing the  with the line number ID.
Some comments would really help make sense of these chains of parent
references!

> + while (l1 <= l2) {
> + e1 = document.getElementById('n' + l1++);
> + e1.style.backgroundColor = "yellow";
> + }
> +
> + e.scrollIntoView(true);
> +}
> +
> +/* line range highlight */
> +
> +document.addEventListener("DOMContentLoaded", function() {
> + cgit_line_range_highlight();
> +}, false);
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v3 1/6] config: add js

[John Keeping]

On Sun, Jun 24, 2018 at 10:44:29AM +0800, Andy Green wrote:
> Just like the config allows setting css URL path,
> add a config for setting the js URL path
> 
> Setting the js path to an empty string disables
> emitting the reference to it in the head section.
> 
> Signed-off-by: Andy Green 

For the code changes,

Reviewed-by: John Keeping 

One comment on the documentation below...

> ---
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 99fc799..bdd799f 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -248,6 +248,12 @@ inline-readme::
>   individually also choose to ignore this global list, and create a
>   repo-specific list by using 'repo.inline-readme'.
>  
> +js::
> + Url which specifies the javascript script document to include in all 
> cgit
> + pages.  Default value: "/cgit.js".  Setting this to an empty string will
> + disable the link to this file in the head section and defeat generation
> + of any features needing functions from it.

In this version of the series, I don't think we generate anythink that
calls into JS, so can this last clause be deleted?

>  local-time::
>   Flag which, if set to "1", makes cgit print commit and tag times in the
>   servers timezone. Default value: "0".
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts

[John Keeping]

On Sat, Jun 23, 2018 at 07:08:08PM +0800, Andy Green wrote:
> 
> 
> On 06/23/2018 06:53 PM, John Keeping wrote:
> > On Sat, Jun 23, 2018 at 06:33:38PM +0800, Andy Green wrote:
> >>
> >>
> >> On 06/23/2018 06:28 PM, John Keeping wrote:
> >>> On Thu, Jun 21, 2018 at 02:46:47PM +0800, Andy Green wrote:
> >>>> Where root-desc and repo.desc are used in the header region, also
> >>>> emit their html counterparts afterwards if they are defined.
> >>>>
> >>>> Where root-desc are repo.desc are used outside the header,
> >>>> eg in the repo list, leave it as it is without adding any
> >>>> related html.
> >>>>
> >>>> Signed-off-by: Andy Green 
> >>>> ---
> >>>
> >>> I think this should be squashed with the previous patch since it makes
> >>> it easier to see what's going on.
> >>>
> >>> When I read your initial email on this, I thought we could introduce a
> >>> new HTML version of the description and use that *instead of* the plain
> >>> text one if the HTML variant is available.
> >>
> >> I actually first implemented just rendering what we have as raw html...
> > 
> > I don't think we can do that without introducing an HTML injection risk
> > in configurations that are currently safe.
> 
> If someone else controls the repo.desc, yes it's not nice.
> 
> Isn't it the same problem if someone else controls the repo.desc_html?

No, because repo.desc_html will always be treated as HTML.

If we change the behaviour of CGit for the _existing_ repo.desc then a
configuration which is currently serving plain text will suddenly start
interpreting that as HTML.  Even if we make it very clear in the release
notes that this is happening, it's not great for a simple software
update to change this.

> >>> Having looked at the current implementation of repo->desc, I think
> >>> that's desirable because the reason we don't have a null-check for that
> >>> in the context below is that it will be set to "[no description]" if no
> >>> other value is provided.  If a user has set repo->desc_html, I don't
> >>> think we want to print "[no description]" before showing the HTML
> >>> description!
> >>
> >> I take the point, but it turned out there are two separate kinds of
> >> description here... the text-only, existing one that is used, eg, in the
> >> list of repos.  And a "functional" HTML part that has buttons or
> >> whatever specific to the repo and used on the header part.
> >>
> >> With just treating them as one, the repo list gained meaningless HTML
> >> buttons or pictures or whatever decoration was put there.  The repo list
> >> just wants a short textual description that already exists.  So it
> >> arrived at this, leave that be, and add an optional HTML decoration part.
> > 
> > OK, that makes sense.  Maybe we need the following check, but it is
> > quite ugly!
> > 
> > if (ctx.repo->desc &&
> > (ctx.repo->desc != cgit_default_repo_desc ||
> >  !ctx.repo->html_desc))
> > 
> > that is, show the plain text description only if it has been customised
> > or if there is no HTML description.
> 
> I'm used to looking in the mirror, so it's OK for me :-)
> 
> It could also be done as a filter script that purifies strings given to 
> it and wraps them in canned html.
> 
> Basically the cgit repo probably isn't existing on its own.  It's part 
> of a project that has links relevant to someone who, eg, stumbled on the 
> cgit repo.  In cgit's own case, the cgit header has the author name but 
> no link to its mailing list... that's not right actually.  So somehow 
> there should be a way to integrate the cgit url with other urls strongly 
> likely to be of interest to someone who is interested in the cgit.

I had a look at libwebsockets.org and the additional links look really
nice.

I think the implementation here is fine (maybe with the explanation of
"two kinds of description" in the commit message), but I can see some
people wanting to disable the plain text description and use the HTML
one for everything.  To make that work, we need to use the ugly if
condition above so that cgit_default_repo_desc is hidden (even though
it's ugly, it works and with your explanation above I agree that the
simple if(repo_desc_html) else if(repo_desc) version isn't sufficient).
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v4 00/16] Render READMEs inline in tree view

[John Keeping]

On Wed, Jun 20, 2018 at 06:11:57PM +0800, Andy Green wrote:
> The following series adds config to allow rendering of
> selected READMEs inline after the tree view, where
> present in the directory being viewed.
> 
> Particularly you can use completely relative markdown to
> inline pictures served from the current repo rev context,
> eg,
> 
> ![overview](./doc-assets/overview.png)
> 
> will "just work" showing the png from the current view
> rev context; this format also works in github.
> 
> It builds on John Keeping's RENDER mode series from 2016.
> 
> Typical config to enable it, if you have a README.md
> looks like
> 
> inline-readme=README.md
> render.md=/usr/libexec/cgit/filters/html-converters/md2html
> 
> You can see examples of it in operation at
> 
> https://libwebsockets.org/git/libwebsockets/tree
> https://libwebsockets.org/git/libwebsockets/tree/?h=v3.0-stable
> https://warmcat.com/git/cgit/tree/
> 
> The expected basis these apply on top of is
> 
>  - jk/for-jason
>  - ch/for-jason
> 
> You can find these patches on top of the expected basis here
> 
> https://warmcat.com/git/cgit/log/
> 
> v4 collects all the reviewed by and implements yesterday's comments

Other than the left-over debugging line you've already pointed out
yourself, this looks good to me.

I suggest you post a final version after the Git 2.18.0 update hits
master and we can get this merged.

> ---
> 
> Andy Green (10):
>   manpage: fix sorting order
>   ui-tree: ls_tail: add walk table param
>   config: add global inline-readme list
>   config: add repo inline-readme list
>   ui-tree: render any matching README file in tree view
>   md2html: add asset postfix arg
>   ui-shared: deduplicate some code in repolink
>   ui-shared: add helper for generating non-urlencoded links
>   render: adapt for providing extra filter args for plain
>   md2html: change css name to not conflict with highlight
> 
> John Keeping (6):
>   Use string list strdup_strings for mimetypes
>   Add source page
>   Parse render filters from the config
>   ui-tree: split out buffer printing
>   ui-tree: use render filters to display content
>   md2html: add asset mapping
> 
> 
>  cgit.c  |   35 +-
>  cgit.css|5 +
>  cgit.h  |6 +
>  cgitrc.5.txt|  221 
> +++
>  cmd.c   |8 +
>  filter.c|4 +
>  filters/html-converters/md2html |   61 ++-
>  shared.c|   21 
>  ui-shared.c |   72 ++---
>  ui-shared.h |6 +
>  ui-tree.c   |  193 +++---
>  ui-tree.h   |2 
>  12 files changed, 498 insertions(+), 136 deletions(-)
> 
> --
> Signature
> ___
> CGit mailing list
> CGit@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts

[John Keeping]

On Sat, Jun 23, 2018 at 06:33:38PM +0800, Andy Green wrote:
> 
> 
> On 06/23/2018 06:28 PM, John Keeping wrote:
> > On Thu, Jun 21, 2018 at 02:46:47PM +0800, Andy Green wrote:
> >> Where root-desc and repo.desc are used in the header region, also
> >> emit their html counterparts afterwards if they are defined.
> >>
> >> Where root-desc are repo.desc are used outside the header,
> >> eg in the repo list, leave it as it is without adding any
> >> related html.
> >>
> >> Signed-off-by: Andy Green 
> >> ---
> > 
> > I think this should be squashed with the previous patch since it makes
> > it easier to see what's going on.
> > 
> > When I read your initial email on this, I thought we could introduce a
> > new HTML version of the description and use that *instead of* the plain
> > text one if the HTML variant is available.
> 
> I actually first implemented just rendering what we have as raw html...

I don't think we can do that without introducing an HTML injection risk
in configurations that are currently safe.

> > Having looked at the current implementation of repo->desc, I think
> > that's desirable because the reason we don't have a null-check for that
> > in the context below is that it will be set to "[no description]" if no
> > other value is provided.  If a user has set repo->desc_html, I don't
> > think we want to print "[no description]" before showing the HTML
> > description!
> 
> I take the point, but it turned out there are two separate kinds of 
> description here... the text-only, existing one that is used, eg, in the 
> list of repos.  And a "functional" HTML part that has buttons or 
> whatever specific to the repo and used on the header part.
> 
> With just treating them as one, the repo list gained meaningless HTML 
> buttons or pictures or whatever decoration was put there.  The repo list 
> just wants a short textual description that already exists.  So it 
> arrived at this, leave that be, and add an optional HTML decoration part.

OK, that makes sense.  Maybe we need the following check, but it is
quite ugly!

if (ctx.repo->desc &&
(ctx.repo->desc != cgit_default_repo_desc ||
 !ctx.repo->html_desc))

that is, show the plain text description only if it has been customised
or if there is no HTML description.
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH] noheader: place branch combo on tabs if no header

[John Keeping]

On Wed, Jun 20, 2018 at 09:17:00PM +0800, Andy Green wrote:
> noheader=1 stops the static page header from being emitted by
> cgit, but along with that, the small form that lets the user
> change branch context on the page is also lost.
> 
> This isn't actually static since it contains a dynamic list of
> branches; it can't be reproduced on the user's external header
> static html.  So it seems it doesn't belong to the set of header
> things that should be disabled.
> 
> This patch relocates the branch selection combo and
> form on the left of the tabs line if noheader=1.  It doesn't
> change anything if noheader is not set.
> 
> Signed-off-by: Andy Green 

This makes noheader=1 a lot more usable!

I wonder if the branch combo should be somewhere to the right of the
main tabs, but I don't feel strongly either way.

Reviewed-by: John Keeping 

> ---
>  ui-shared.c |   34 --
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index c9a34fb..082a6f1 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -958,6 +958,19 @@ static void cgit_print_path_crumbs(char *path)
>   ctx.qry.path = old_path;
>  }
>  
> +static void print_branch_combo_form(void)
> +{
> + html("\n");
> + cgit_add_hidden_formfields(0, 1, ctx.qry.page);
> + html("\n");
> + for_each_branch_ref(print_branch_option, ctx.qry.head);
> + if (ctx.repo->enable_remote_branches)
> + for_each_remote_ref(print_branch_option, ctx.qry.head);
> + html(" ");
> + html("");
> + html("");
> +}
> +
>  static void print_header(void)
>  {
>   char *logo = NULL, *logo_link = NULL;
> @@ -991,15 +1004,7 @@ static void print_header(void)
>   cgit_summary_link(ctx.repo->name, ctx.repo->name, NULL, NULL);
>   if (ctx.env.authenticated) {
>   html("");
> - html("\n");
> - cgit_add_hidden_formfields(0, 1, ctx.qry.page);
> - html(" onchange='this.form.submit();'>\n");
> - for_each_branch_ref(print_branch_option, ctx.qry.head);
> - if (ctx.repo->enable_remote_branches)
> - for_each_remote_ref(print_branch_option, 
> ctx.qry.head);
> - html(" ");
> - html("");
> - html("");
> + print_branch_combo_form();
>   }
>   } else
>   html_txt(ctx.cfg.root_title);
> @@ -1023,8 +1028,15 @@ void cgit_print_pageheader(void)
>   if (!ctx.env.authenticated || !ctx.cfg.noheader)
>   print_header();
>  
> - html("\n");
> + html("\n");
>   if (ctx.env.authenticated && ctx.repo) {
> + if (ctx.cfg.noheader) {
> + html("");
> + print_branch_combo_form();
> + html("");
> + }
> + html("");
> +
>   if (ctx.repo->readme.nr)
>   reporevlink("about", "about", NULL,
>   hc("about"), ctx.qry.head, NULL,
> @@ -1081,6 +1093,8 @@ void cgit_print_pageheader(void)
>   html("\n");
>   } else if (ctx.env.authenticated) {
>   char *currenturl = cgit_currenturl();
> +
> + html("");
>   site_link(NULL, "index", NULL, hc("repolist"), NULL, NULL, 0, 
> 1);
>   if (ctx.cfg.root_readme)
>   site_link("about", "about", NULL, hc("about"),
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 2/2] ui-shared: emit root-desc-html and repo.desc-html after their text counterparts

[John Keeping]

On Thu, Jun 21, 2018 at 02:46:47PM +0800, Andy Green wrote:
> Where root-desc and repo.desc are used in the header region, also
> emit their html counterparts afterwards if they are defined.
> 
> Where root-desc are repo.desc are used outside the header,
> eg in the repo list, leave it as it is without adding any
> related html.
> 
> Signed-off-by: Andy Green 
> ---

I think this should be squashed with the previous patch since it makes
it easier to see what's going on.

When I read your initial email on this, I thought we could introduce a
new HTML version of the description and use that *instead of* the plain
text one if the HTML variant is available.

Having looked at the current implementation of repo->desc, I think
that's desirable because the reason we don't have a null-check for that
in the context below is that it will be set to "[no description]" if no
other value is provided.  If a user has set repo->desc_html, I don't
think we want to print "[no description]" before showing the HTML
description!

>  ui-shared.c |4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index c8f4d8f..a9ec430 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -1013,11 +1013,15 @@ static void print_header(void)
>   html("");
>   if (ctx.repo) {
>   html_txt(ctx.repo->desc);
> + if (ctx.repo->desc_html)
> + html(ctx.repo->desc_html);
>   html("");
>   html_txt(ctx.repo->owner);
>   } else {
>   if (ctx.cfg.root_desc)
>   html_txt(ctx.cfg.root_desc);
> + if (ctx.cfg.root_desc_html)
> + html(ctx.cfg.root_desc_html);
>   }
>   html("\n");
>  }
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2 1/5] config: add js

[John Keeping]

On Thu, Jun 21, 2018 at 05:34:49PM +0800, Andy Green wrote:
> Just like the config allows setting css URL path,
> add a config for setting the js URL path
> 
> Signed-off-by: Andy Green 
> ---
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 99fc799..a692aa5 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -248,6 +248,10 @@ inline-readme::
>   individually also choose to ignore this global list, and create a
>   repo-specific list by using 'repo.inline-readme'.
>  
> +js::
> + Url which specifies the javascript script document to include in all 
> cgit
> + pages.  Default value: "/cgit.js".

Should we allow the empty string to disable this option?

>  local-time::
>   Flag which, if set to "1", makes cgit print commit and tag times in the
>   servers timezone. Default value: "0".
> 
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: cache-size implementation downsides

[John Keeping]

On Wed, Jun 20, 2018 at 08:01:11AM +0200, Christian Hesse wrote:
> John Keeping  on Sat, 2018/06/16 16:46:
> > -- >8 --  
> > Subject: [PATCH] cache: close race window when unlocking slots
> 
> You should add a "From:" line for easy git-am. ;)

"git am --scissors" will pick it up from the message headers, won't it?

> > We use POSIX advisory record locks to control access to cache slots, but
> > these have an unhelpful behaviour in that they are released when any
> > file descriptor referencing the file is closed by this process.
> > 
> > Mostly this is okay, since we know we won't be opening the lock file
> > anywhere else, but there is one place that it does matter: when we
> > restore stdout we dup2() over a file descriptor referring to the file,
> > thus closing that descriptor.
> > 
> > Since we restore stdout before unlocking the slot, this creates a window
> > during which the slot content can be overwritten.  The fix is reasonably
> > straightforward: simply restore stdout after unlocking the slot, but the
> > diff is a bit bigger because this requires us to move the temporary
> > stdout FD into struct cache_slot.
> > 
> > Signed-off-by: John Keeping 
> 
> Reviewed-by: Christian Hesse 

Thanks for the review!

> > ---
> >  cache.c | 37 ++---
> >  1 file changed, 14 insertions(+), 23 deletions(-)
> > 
> > diff --git a/cache.c b/cache.c
> > index 0901e6e..2c70be7 100644
> > --- a/cache.c
> > +++ b/cache.c
> > @@ -29,6 +29,7 @@ struct cache_slot {
> > cache_fill_fn fn;
> > int cache_fd;
> > int lock_fd;
> > +   int stdout_fd;
> > const char *cache_name;
> > const char *lock_name;
> > int match;
> 
> Not relevant for this commit, but... Is there a reason that struct cache_slot
> lives in cache.c, not cache.h?

I guess because it's only needed in this file, so we can encapsulate it
here and avoid leaking the implementation details to other parts of the
code.

But this predates my involvement with CGit by a few years.

> > @@ -197,6 +198,13 @@ static int unlock_slot(struct cache_slot *slot, int
> > replace_old_slot) else
> > err = unlink(slot->lock_name);
> >  
> > +   /* Restore stdout and close the temporary FD. */
> > +   if (slot->stdout_fd >= 0) {
> > +   dup2(slot->stdout_fd, STDOUT_FILENO);
> > +   close(slot->stdout_fd);
> > +   slot->stdout_fd = -1;
> > +   }
> > +
> > if (err)
> > return errno;
> >  
> > @@ -208,42 +216,24 @@ static int unlock_slot(struct cache_slot *slot, int
> > replace_old_slot) */
> >  static int fill_slot(struct cache_slot *slot)
> >  {
> > -   int tmp;
> > -
> > /* Preserve stdout */
> > -   tmp = dup(STDOUT_FILENO);
> > -   if (tmp == -1)
> > +   slot->stdout_fd = dup(STDOUT_FILENO);
> > +   if (slot->stdout_fd == -1)
> > return errno;
> >  
> > /* Redirect stdout to lockfile */
> > -   if (dup2(slot->lock_fd, STDOUT_FILENO) == -1) {
> > -   close(tmp);
> > +   if (dup2(slot->lock_fd, STDOUT_FILENO) == -1)
> > return errno;
> > -   }
> >  
> > /* Generate cache content */
> > slot->fn();
> >  
> > /* Make sure any buffered data is flushed to the file */
> > -   if (fflush(stdout)) {
> > -   close(tmp);
> > +   if (fflush(stdout))
> > return errno;
> > -   }
> >  
> > /* update stat info */
> > -   if (fstat(slot->lock_fd, >cache_st)) {
> > -   close(tmp);
> > -   return errno;
> > -   }
> > -
> > -   /* Restore stdout */
> > -   if (dup2(tmp, STDOUT_FILENO) == -1) {
> > -   close(tmp);
> > -   return errno;
> > -   }
> > -
> > -   /* Close the temporary filedescriptor */
> > -   if (close(tmp))
> > +   if (fstat(slot->lock_fd, >cache_st))
> > return errno;
> >  
> > return 0;
> > @@ -393,6 +383,7 @@ int cache_process(int size, const char *path, const
> > char *key, int ttl, strbuf_addstr(, ".lock");
> > slot.fn = fn;
> > slot.ttl = ttl;
> > +   slot.stdout_fd = -1;
> > slot.cache_name = filename.buf;
> > slot.lock_name = lockname.buf;
> > slot.key = key;
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: cache-size implementation downsides

[John Keeping]

On Sat, Jun 16, 2018 at 04:46:21PM +0100, John Keeping wrote:
> On Wed, Jun 13, 2018 at 03:02:42PM -0400, Konstantin Ryabitsev wrote:
> > 2. I have witnessed cache corruption due to collisions (which is
> > a bug in itself). One of our frontends was hit by a lot of agressive
> > crawling of snapshots that raised the load to 60+ (many, many gzip
> > processes). After we blackholed the bot, some of the cache objects for
> > non-snapshot URLs had trailing gzip junk in them, meaning that either
> > two instances were writing to the same file, or something else resulted
> > in cache corruption. This is probably a race condition somewhere in the
> > locking code.
> 
> I've had a look at this, and I think we might end up dropping our lock
> too early thanks to this code (in fill_slot()):
> 
>   /* Restore stdout */
>   if (dup2(tmp, STDOUT_FILENO) == -1) {
> 
> Before this line, STDOUT_FILENO refers to lock_fd which has a POSIX
> advisory record lock on the entire file.  However, the documentation for
> that says:
> 
>*  If a process closes any file descriptor referring to a file, then 
> all
>   of the process's locks on that file are released, regardless of the
>   file descriptor(s)  on  which  the  locks  were  obtained.  This is
>   bad: it means that a process can lose its locks on a file such as
>   /etc/passwd or  /etc/mtab  when for some reason a library function
>   decides to open, read, and close the same file.
> 
> I haven't verified this, but I suspect that dup'ing the original stdout
> over STDOUT_FILENO is equivalent to closing a file descriptor referring
> to our lock file.  And thus the lock is released at this point, which is
> before we rename the lock file over the cache file.
> 
> If that is correct, then there is a window during which a new process
> can open the lock file to write new content and successfully acquire the
> lock on that file even though it is still being used by another process.

I confirmed this behaviour with trace-cmd.

Before:
cgit-7291  : posix_lock_inode: fl=0x0x88020faa6258 
dev=0x8:0x14 ino=0x4e3e13 fl_next=0x(nil) fl_owner=0x0x88007a8f2680 
fl_pid=7291 fl_flags=FL_POSIX fl_type=F_WRLCK fl_start=0 
fl_end=9223372036854775807 ret=0
cgit-7291  : fcntl_setlk:  fl=0x0x88020faa6258 
dev=0x8:0x14 ino=0x4e3e13 fl_next=0x(nil) fl_owner=0x0x88007a8f2680 
fl_pid=7291 fl_flags=FL_POSIX fl_type=F_WRLCK fl_start=0 
fl_end=9223372036854775807 ret=0
cgit-7291  : locks_get_lock_context: dev=0x8:0x14 ino=0x4e3e13 
type=F_UNLCK ctx=0x8801beeb7930
cgit-7291  : posix_lock_inode: fl=0x0xc90003627da8 
dev=0x8:0x14 ino=0x4e3e13 fl_next=0x(nil) fl_owner=0x0x88007a8f2680 
fl_pid=7291 fl_flags=FL_POSIX|FL_CLOSE fl_type=F_UNLCK fl_start=0 
fl_end=9223372036854775807 ret=0
cgit-7291  : locks_remove_posix:   fl=0x0xc90003627da8 
dev=0x8:0x14 ino=0x4e3e13 fl_next=0x(nil) fl_owner=0x0x88007a8f2680 
fl_pid=7291 fl_flags=FL_POSIX|FL_CLOSE fl_type=F_UNLCK fl_start=0 
fl_end=9223372036854775807 ret=0
cgit-7291  : sys_enter_rename: oldname: 0x559bd0bd5830, 
newname: 0x559bd0bd57d0
cgit-7291  : sys_exit_rename:  0x0

After:
cgit-7488  : posix_lock_inode: fl=0x0x8802122c43e8 
dev=0x8:0x14 ino=0x4e3e13 fl_next=0x(nil) fl_owner=0x0x8800310958c0 
fl_pid=7488 fl_flags=FL_POSIX fl_type=F_WRLCK fl_start=0 
fl_end=9223372036854775807 ret=0
cgit-7488  : fcntl_setlk:  fl=0x0x8802122c43e8 
dev=0x8:0x14 ino=0x4e3e13 fl_next=0x(nil) fl_owner=0x0x8800310958c0 
fl_pid=7488 fl_flags=FL_POSIX fl_type=F_WRLCK fl_start=0 
fl_end=9223372036854775807 ret=0
cgit-7488  : sys_enter_rename: oldname: 0x56512cd7f830, 
newname: 0x56512cd7f7d0
cgit-7488  : sys_exit_rename:  0x0
cgit-7488  : locks_get_lock_context: dev=0x8:0x14 ino=0x4e3e13 
type=F_UNLCK ctx=0x8802144daa10
cgit-7488  : posix_lock_inode: fl=0x0xc900038d7da8 
dev=0x8:0x14 ino=0x4e3e13 fl_next=0x0x880006f5b780 
fl_owner=0x0x8800310958c0 fl_pid=7488 fl_flags=FL_POSIX|FL_CLOSE 
fl_type=F_UNLCK fl_start=0 fl_end=9223372036854775807 ret=0
cgit-7488  : locks_remove_posix:   fl=0x0xc900038d7da8 
dev=0x8:0x14 ino=0x4e3e13 fl_next=0x0x880006f5b780 
fl_owner=0x0x8800310958c0 fl_pid=7488 fl_flags=FL_POSIX|FL_CLOSE 
fl_type=F_UNLCK fl_start=0 fl_end=9223372036854775807 ret=0


I'm planning to queue the patch below on jk/for-jason and send a PR in
the next day or two, but it would be nice to get a reviewed-by before I
do that.

> -- >8 --
> Subject: [PATCH] cache: close race window when unlocking slots
> 
> We use POSIX advisory record locks to control access to cache slots, but
&

Re: [PATCH v3 17/17] render: adapt for providing extra filter args for plain

[John Keeping]

On Tue, Jun 19, 2018 at 05:02:52PM +0800, Andy Green wrote:
> This changes the render filter exec part to provide a second
> and third argument, which are used by md2html to fix up the url
> path for "plain" for the repo, eg, "/cgit/plain/" and
> "?h=mybranch", as required by the modifications to md2html in
> the previous patches.
> 
> The combination means cgit becomes able to serve assets using
> markdown urls starting from the repo root dir, without mentioning
> any virtual url part specific to a cgit or other web rendering
> instance, while respecting the version context.
> 
> Eg, continuing the example of the arguments being
> "/cgit/plain/" and "?h=mybranch" from above, if the markdown has
> 
> ![overview](./doc-assets/overview.png)
> 
> the img src will be fixed up to
> 
> "/cgit/plain/doc-assets/overview.png?h=mybranch"
> 
> If the same document is viewed from a different rev in cgit, the
> processed markdown url will change to match the cgit context, even
> though the markdown relative URL is the same for all versions.
> 
> Signed-off-by: Andy Green 

Reviewed-by: John Keeping 

> ---
>  filter.c  |5 -
>  ui-tree.c |   11 +--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/filter.c b/filter.c
> index 4ae4aaa..7c1f188 100644
> --- a/filter.c
> +++ b/filter.c
> @@ -424,6 +424,10 @@ struct cgit_filter *cgit_new_filter(const char *cmd, 
> filter_type filtertype)
>   argument_count = 12;
>   break;
>  
> + case RENDER:
> + argument_count = 3;
> + break;
> +
>   case EMAIL:
>   argument_count = 2;
>   break;
> @@ -434,7 +438,6 @@ struct cgit_filter *cgit_new_filter(const char *cmd, 
> filter_type filtertype)
>  
>   case SOURCE:
>   case ABOUT:
> - case RENDER:
>   argument_count = 1;
>   break;
>  
> diff --git a/ui-tree.c b/ui-tree.c
> index 6ffd4dd..2e94755 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -102,16 +102,23 @@ static void print_buffer(const char *basename, char 
> *buf, unsigned long size)
>  }
>  
>  static void render_buffer(struct cgit_filter *render, const char *name,
> - char *buf, unsigned long size)
> +   char *buf, unsigned long size)
>  {
>   char *filter_arg = xstrdup(name);
> + struct strbuf sb_pre = STRBUF_INIT, sb_post = STRBUF_INIT;
> +
> + cgit_repo_create_url(_pre, _post, "plain", ctx.qry.head, NULL);
> +
> + fprintf(stderr, "'%s' '%s'\n", sb_pre.buf, sb_post.buf);
>  
>   html("");
> - cgit_open_filter(render, filter_arg);
> + cgit_open_filter(render, filter_arg, sb_pre.buf, sb_post.buf);
>   html_raw(buf, size);
>   cgit_close_filter(render);
>   html("");
>  
> + strbuf_release(_pre);
> + strbuf_release(_post);
>   free(filter_arg);
>  }
>  
> 
> ___
> CGit mailing list
> CGit@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

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

[John Keeping]

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 
> ---
>  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?

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

> + else {
> + strbuf_addstr(sb_pre, ctx.cfg.script_name);
> + strbuf_addf(sb_post, "?url=%s", ctx.repo->url);
> + sb = sb_post;
> + delim = "";
> + }
> +
> + if (ctx.repo->url[strlen(ctx.repo->url) - 1] != '/')
> + strbuf_addch(sb, '/');

Maybe add a blank line here since these if blocks are unrelated.

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

> +}
> +
>  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);
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v3 12/17] ui-tree: render any matching README file in tree view

[John Keeping]

On Tue, Jun 19, 2018 at 05:02:27PM +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.

There's only one now, the commit message isn't quite accurate any more!

> Signed-off-by: Andy Green 
> Reviewed-by: John Keeping 
> ---
>  ui-tree.c |   53 -
>  1 file changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/ui-tree.c b/ui-tree.c
> index 1ccbb22..6ffd4dd 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -1,6 +1,6 @@
>  /* ui-tree.c: functions for tree output
>   *
> - * Copyright (C) 2006-2017 cgit Development Team 
> + * Copyright (C) 2006-2018 cgit Development Team 
>   *
>   * Licensed under GNU General Public License v2
>   *   (see COPYING for full license text)
> @@ -12,8 +12,10 @@
>  #include "ui-shared.h"
>  
>  struct walk_tree_context {
> + struct object_id inline_oid;
>   char *curr_rev;
>   char *match_path;
> + char *inline_filename;
>   int state;
>   bool use_render;
>  };
> @@ -325,11 +327,19 @@ static int ls_item(const struct object_id *oid, struct 
> strbuf *base,
>   );
>   } else {
>   char *ext = strrchr(name, '.');
> +
>   strbuf_addstr(, "ls-blob");
>   if (ext)
>   strbuf_addf(, " %s", ext + 1);
> +
>   cgit_tree_link(name, NULL, class.buf, ctx.qry.head,
>  walk_tree_ctx->curr_rev, fullpath.buf);
> +
> + if (!walk_tree_ctx->inline_filename &&
> + string_list_has_string(>inline_readme, name)) {
> + walk_tree_ctx->inline_filename = xstrdup(pathname);
> + oidcpy(_tree_ctx->inline_oid, oid);
> + }
>   }
>   htmlf("%li", size);
>  
> @@ -367,7 +377,46 @@ static void ls_head(void)
>  
>  static void ls_tail(struct walk_tree_context *walk_tree_ctx)
>  {
> + struct cgit_filter *render;
> + enum object_type type;
> + char *buf, *mimetype;
> + unsigned long size;
> +
>   html("\n");
> +
> + if (!walk_tree_ctx->inline_filename)
> + goto done;
> +
> + type = oid_object_info(the_repository, _tree_ctx->inline_oid, 
> );
> + if (type == OBJ_BAD)
> + goto done;
> +
> + buf = read_object_file(_tree_ctx->inline_oid, , );
> + if (!buf)
> + goto done;
> +
> + /* create a vertical gap between tree nav / inline */
> + html("");
> +
> + render = get_render_for_filename(walk_tree_ctx->inline_filename);
> + mimetype = render ? NULL : get_mimetype_for_filename(
> + walk_tree_ctx->inline_filename);
> +
> + htmlf("%s", walk_tree_ctx->inline_filename);
> + html("\n");
> +
> + if (render)
> + render_buffer(render, walk_tree_ctx->inline_filename,
> +   buf, size);
> + else if (mimetype)
> + include_file(walk_tree_ctx->inline_filename, mimetype);
> + else
> + print_buffer(walk_tree_ctx->inline_filename, buf, size);
> +
> + free(mimetype);
> + free(buf);
> +
> +done:
>   cgit_print_layout_end();
>  }
>  
> @@ -478,4 +527,6 @@ void cgit_print_tree(const char *rev, char *path, bool 
> use_render)
>  
>  cleanup:
>   free(walk_tree_ctx.curr_rev);
> + if (walk_tree_ctx.inline_filename)
> + free(walk_tree_ctx.inline_filename);
>  }
> 
> ___
> CGit mailing list
> CGit@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v3 08/17] ui-blame: free read_sha1_file() buffer after use

[John Keeping]

On Tue, Jun 19, 2018 at 05:02:07PM +0800, Andy Green wrote:
> Signed-off-by: Andy Green 

Pushed to jk/for-jason (after rebasing onto master).

> ---
>  ui-blame.c |5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ui-blame.c b/ui-blame.c
> index 8b56554..37e2c68 100644
> --- a/ui-blame.c
> +++ b/ui-blame.c
> @@ -154,7 +154,7 @@ static void print_object(const struct object_id *oid, 
> const char *path,
>   htmlf("blob size (%ldKB)"
> " exceeds display size limit (%dKB).",
> size / 1024, ctx.cfg.max_blob_size);
> - return;
> + goto cleanup;
>   }
>  
>   html("\n\n");
> @@ -213,6 +213,9 @@ static void print_object(const struct object_id *oid, 
> const char *path,
>   html("\n\n");
>  
>   cgit_print_layout_end();
> +
> +cleanup:
> + free(buf);
>  }
>  
>  static int walk_tree(const struct object_id *oid, struct strbuf *base,
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2] blame: css: make blame highlight div absolute and top left

[John Keeping]

On Tue, Jun 19, 2018 at 03:11:52AM +0800, Andy Green wrote:
> On June 19, 2018 2:57:47 AM GMT+08:00, John Keeping  
> wrote:
> >On Mon, Jun 18, 2018 at 02:02:54PM +0800, Andy Green wrote:
> >> Normal operation of blame view requires div.highlight to
> >> have absolute position and set to its parent's top left
> >> for me.
> >> 
> >> Otherwise the grey background boxes indicating the extent of
> >> the patch in the lines td displace the highlit sources, they
> >> start at the bottom of the td.
> >> 
> >> This patch makes the blame highlight div start back up the top of
> >> its parent area and render on top of the grey boxes.
> >> 
> >> Checked on Linux Firefox 60 and Linux Chrome 69.
> >
> >Which browser is this broken in?  I tried Linux Firefox 60 and
> >Chromium 67 and it looks ok without this patch.  (I'm not opposed to
> >the patch in principle, indeed it seems like a sensible change, but
> >I'm curious why I can't reproduce the problem.)
> 
> It's not caused in the browser, I checked the cgit site blame and that
> doesn't have the problem.  I also went back to check just master
> without anything on top, and it's still broken on my box.  I confirmed
> I'm using the latest css, and that snipping the div with the boxes in
> the lines td (using developer tools / inspector in ffox) is also
> enough to have it display correctly.
> 
> Md2html seems to issue inline css style related to "highlight" as part
> of its processing, I think as it is, that may put the actual rendering
> behaviour at the mercy of pygments version or whatever actually
> generates that.  So separating the css to a different name is likely
> needed for robustness anyway.

Sounds sensible.  It looks like we use the same get_style_defs() call in
both syntax-hightlighing.py and md2html.  In syntax-hightlighing.py
the class name is generated by pygments [1] but in md2html we actually
specify the class as a parameter to the codehilite extension.

Your patch series allows these filters to run in the same page for the
first time, so I think what we should do is rename the highlight class
in md2html.  Does that also fix the problem?

[1] It may be possible to override it, I haven't checked the API docs

> My box is fedora 28.
> 
> python3-pygments-2.2.0-10.fc28.noarch
> 
> >> "highlight" div class name is also used in md2html rendering
> >> output.  So this patch solves it by introducing a wrapper
> >> div and new "blame_highlight" css class.
> >> 
> >> Signed-off-by: Andy Green 
> >> ---
> >>  cgit.css   |2 ++
> >>  ui-blame.c |4 ++--
> >>  2 files changed, 4 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/cgit.css b/cgit.css
> >> index da8d9b0..5a85ceb 100644
> >> --- a/cgit.css
> >> +++ b/cgit.css
> >> @@ -162,6 +162,8 @@ div#cgit table.list
> >tr.nohover-highlight:hover:nth-child(odd) {
> >>background: white;
> >>  }
> >>  
> >> +div#cgit div.blame_highlight { position: absolute; top: 0; left: 0;
> >}
> >
> >Is the "left" needed here?  I don't see any problem with setting the
> >position and top attributes, but setting left:0 moves the content right
> >up against the cell boundary where before we had a slight gap.
> 
> OK... it was not offset horizontally anyway so this is overkill.  I'll remove 
> it.
> 
> -Andy
> 
> >> +
> >>  div#cgit table.list th {
> >>font-weight: bold;
> >>/* color: #888;
> >> diff --git a/ui-blame.c b/ui-blame.c
> >> index 6e23f0b..ab44e3f 100644
> >> --- a/ui-blame.c
> >> +++ b/ui-blame.c
> >> @@ -196,7 +196,7 @@ static void print_object(const struct object_id
> >*oid, const char *path,
> >>free((void *)sb.final_buf);
> >>  
> >>/* Lines */
> >> -  html("");
> >> +  html(" ");
> >
> >No need for a space before  here.
> >
> >>if (ctx.repo->source_filter) {
> >>char *filter_arg = xstrdup(basename);
> >>cgit_open_filter(ctx.repo->source_filter, filter_arg);
> >> @@ -207,7 +207,7 @@ static void print_object(const struct object_id
> >*oid, const char *path,
> >>html_txt(buf);
> >>}
> >>  
> >> -  html("");
> >> +  html("");
> >>  
> >>html("\n");
> >>  
> ___
> CGit mailing list
> CGit@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2 14/15] md2html-add-asset-postfix-arg

[John Keeping]

On Tue, Jun 19, 2018 at 11:55:35AM +0800, Andy Green wrote:
> 
> 
> On 06/19/2018 03:21 AM, John Keeping wrote:
> > On Mon, Jun 18, 2018 at 10:58:26AM +0800, Andy Green wrote:
> 
> >>   class AssetMappingExtension(markdown.extensions.Extension):
> >>   
> >>   def __init__(self, **kwargs):
> >> -self.config = {'asset_prefix': ['', 'prefix for relative asset 
> >> URLs']}
> >> +self.config = {'asset_prefix': ['', 'prefix for relative asset 
> >> URLs'], 'asset_postfix': ['', 'postfix for relative asset URLs']}
> > 
> > For style it would be nice to align this under asset_prefix.
> 
> Just worried to upset the python indent monster... it seems to work 
> intended I guess it only cares at the start of statements.

Yeah, if you're inside braces of any kind you can pretty much do what
you want.

> >>   super(AssetMappingExtension, self).__init__(**kwargs)
> >>   
> >>   def extendMarkdown(self, md, md_globals):
> >>   asset_prefix = self.getConfig('asset_prefix')
> >>   if not asset_prefix:
> >>   return
> >> +asset_postfix = self.getConfig('asset_postfix')
> >> +if not asset_postfix:
> >> +return
> > 
> > Is this right?  Should we allow one of these to be empty and still
> > process the other one?  In other words, shouldn't the bail out condition
> > be:
> > 
> >  if not (asset_prefix or asset_postfix):
> >  return
> 
> Yeah... the original code that generated the args always generated a 
> postfix, so there was "no problem".
> 
> However the improved code snips ?h=defaultbranch and can be an empty 
> string.  So I changed this as you suggested.
> 
> > I don't think any change to AssetMappingProcessor is required because
> > urljoin already does the right thing when handed the empty string and
> > the config assure that if no value is specified then that is what we
> > get.
> 
> I am not certain of your meaning here... you mean change from what this 
> patch already does for the reason above?  It must change 
> AssetMappingProcessor generally to do what it's trying to do.

I mean that changing the code here will allow empty strings to be passed
to AssetMappingProcessor.run() which previously was blocked by these
conditions.  But no change is required there because it already does the
right thing when handed an empty string as one of prefix/postfix.
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2 12/15] ui-tree: render any matching README file in tree view

[John Keeping]

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 
> > 
> > A couple of minor style points below, but this looks good.  With or
> > without the style changes:
> > 
> > Reviewed-by: John Keeping 
> 
> >> +  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("%s", name);
> >> +  html("\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
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2 12/15] ui-tree: render any matching README file in tree view

[John Keeping]

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 

A couple of minor style points below, but this looks good.  With or
without the style changes:

Reviewed-by: John Keeping 

> ---
>  ui-tree.c |   60 +++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/ui-tree.c b/ui-tree.c
> index 368cdc5..9f59d18 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -1,6 +1,6 @@
>  /* ui-tree.c: functions for tree output
>   *
> - * Copyright (C) 2006-2017 cgit Development Team 
> + * Copyright (C) 2006-2018 cgit Development Team 
>   *
>   * Licensed under GNU General Public License v2
>   *   (see COPYING for full license text)
> @@ -12,8 +12,10 @@
>  #include "ui-shared.h"
>  
>  struct walk_tree_context {
> + struct object_id inline_oid;
>   char *curr_rev;
>   char *match_path;
> + char *inline_path;
>   int state;
>   bool use_render;
>  };
> @@ -326,11 +328,19 @@ static int ls_item(const struct object_id *oid, struct 
> strbuf *base,
>   );
>   } else {
>   char *ext = strrchr(name, '.');
> +
>   strbuf_addstr(, "ls-blob");
>   if (ext)
>   strbuf_addf(, " %s", ext + 1);
> +
>   cgit_tree_link(name, NULL, class.buf, ctx.qry.head,
>  walk_tree_ctx->curr_rev, fullpath.buf);
> +
> + if (!walk_tree_ctx->inline_path &&
> + string_list_has_string(>inline_readme, name)) {
> + walk_tree_ctx->inline_path = xstrdup(pathname);
> + oidcpy(_tree_ctx->inline_oid, oid);
> + }
>   }
>   htmlf("%li", size);
>  
> @@ -368,7 +378,53 @@ static void ls_head(void)
>  
>  static void ls_tail(struct walk_tree_context *walk_tree_ctx)
>  {
> + struct cgit_filter *render;
> + enum object_type type;
> + char *buf, *mimetype, *name;
> + unsigned long size;
> +
>   html("\n");
> +
> + if (!walk_tree_ctx->inline_path)
> + goto done;
> +
> + type = oid_object_info(the_repository, _tree_ctx->inline_oid, 
> );
> + if (type == OBJ_BAD)
> + goto done;
> +
> + buf = read_object_file(_tree_ctx->inline_oid, , );
> + if (!buf)
> + goto done;
> +
> + /* create a vertical gap between tree nav / inline */
> + html("");
> +
> + 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.

> + if (name)
> + name++;
> + else
> + name = walk_tree_ctx->inline_path;
> +
> + htmlf("%s", name);
> + html("\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
...

> + } else {
> + print_buffer(name, buf, size);
> + }
> +
> + free(mimetype);
> + free(buf);
> +
> +done:
>   cgit_print_layout_end();
>  }
>  
> @@ -479,4 +535,6 @@ void cgit_print_tree(const char *rev, char *path, bool 
> use_render)
>  
>  cleanup:
>   free(walk_tree_ctx.curr_rev);
> + if (walk_tree_ctx.inline_path)
> + free(walk_tree_ctx.inline_path);
>  }
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2 11/15] config: add repo inline-readme list

[John Keeping]

On Mon, Jun 18, 2018 at 10:58:10AM +0800, Andy Green wrote:
> This allows the user to choose to override any global
> inline-readme list for a specific repo, using the
> same kind of semantics as the other repo overrides.
> 
> Signed-off-by: Andy Green 

One small style nit below, but other than that this looks good.

Reviewed-by: John Keeping 

> ---
>  cgit.c   |6 ++
>  cgit.h   |1 +
>  cgitrc.5.txt |   10 ++
>  shared.c |2 ++
>  4 files changed, 19 insertions(+)
> 
> diff --git a/cgit.c b/cgit.c
> index 4ffd61f..7c13327 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -103,6 +103,12 @@ static void repo_config(struct cgit_repo *repo, const 
> char *name, const char *va
>   repo->hide = atoi(value);
>   else if (!strcmp(name, "ignore"))
>   repo->ignore = atoi(value);
> + else if (!strcmp(name, "inline-readme")) {
> + if (repo->inline_readme.items == ctx.cfg.inline_readme.items)
> + string_list_init(>inline_readme, 1);
> +
> + string_list_append(>inline_readme, value);
> + }
>   else if (ctx.cfg.enable_filter_overrides) {

The else if should be on the same line as the closing } here.

>   if (!strcmp(name, "about-filter"))
>   repo->about_filter = cgit_new_filter(value, ABOUT);
> diff --git a/cgit.h b/cgit.h
> index 664f003..0aebeba 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -84,6 +84,7 @@ struct cgit_repo {
>   char *defbranch;
>   char *module_link;
>   struct string_list readme;
> + struct string_list inline_readme;
>   char *section;
>   char *clone_url;
>   char *logo;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 37858af..3f493cb 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -561,6 +561,16 @@ repo.ignore::
>   is not shown in the index and cannot be accessed by providing a direct
>   path. Default value: "0". See also: "repo.hide".
>  
> +repo.inline-readme::
> + Append given filename to the list of filenames to be rendered after the
> + tree navigation in tree view, if present in the directory being viewed. 
>  Eg,
> + 'repo.inline-readme=README.md'.  You may also want a corresponding 
> render.
> + entry for the readme suffix, eg,
> + 'render.md=/usr/libexec/cgit/filters/html-converters/md2html'.
> + If not given, the repo will use any global 'inline-readme=' 
> configuration;
> + if any 'repo.inline-readme' are given only they are used for that repo,
> + and the global 'inline-readme=' list is ignored for that repo.
> +
>  repo.logo::
>   Url which specifies the source of an image which will be used as a logo
>   on this repo's pages. Default value: global logo.
> diff --git a/shared.c b/shared.c
> index 26a9266..8fe5c1b 100644
> --- a/shared.c
> +++ b/shared.c
> @@ -77,6 +77,8 @@ struct cgit_repo *cgit_add_repo(const char *url)
>   ret->clone_url = ctx.cfg.clone_url;
>   ret->submodules.strdup_strings = 1;
>   ret->hide = ret->ignore = 0;
> + ret->inline_readme = ctx.cfg.inline_readme;
> +
>   return ret;
>  }
>  
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2 14/15] md2html-add-asset-postfix-arg

[John Keeping]

On Mon, Jun 18, 2018 at 10:58:26AM +0800, Andy Green wrote:
> Extend md2html with a third argument for URL postfix, like "?h=mybranch"
> 
> Signed-off-by: Andy Green 
> ---
>  filters/html-converters/md2html |   17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/filters/html-converters/md2html b/filters/html-converters/md2html
> index eb5d977..128a61b 100755
> --- a/filters/html-converters/md2html
> +++ b/filters/html-converters/md2html
> @@ -9,31 +9,36 @@ from urllib.parse import urljoin
>  
>  class AssetMappingProcessor(markdown.treeprocessors.Treeprocessor):
>  
> -def __init__(self, asset_prefix):
> +def __init__(self, asset_prefix, asset_postfix):
>  self.asset_prefix = asset_prefix
> +self.asset_postfix = asset_postfix
>  
>  def run(self, root):
>  asset_prefix = self.asset_prefix
> +asset_postfix = self.asset_postfix
>  for img in root.iter('img'):
>  src = img.get('src')
>  if src is None:
>  continue
> -img.set('src', urljoin(asset_prefix, src))
> +img.set('src', urljoin(urljoin(asset_prefix, src), 
> asset_postfix))
>  
>  
>  class AssetMappingExtension(markdown.extensions.Extension):
>  
>  def __init__(self, **kwargs):
> -self.config = {'asset_prefix': ['', 'prefix for relative asset 
> URLs']}
> +self.config = {'asset_prefix': ['', 'prefix for relative asset 
> URLs'], 'asset_postfix': ['', 'postfix for relative asset URLs']}

For style it would be nice to align this under asset_prefix.

>  super(AssetMappingExtension, self).__init__(**kwargs)
>  
>  def extendMarkdown(self, md, md_globals):
>  asset_prefix = self.getConfig('asset_prefix')
>  if not asset_prefix:
>  return
> +asset_postfix = self.getConfig('asset_postfix')
> +if not asset_postfix:
> +return

Is this right?  Should we allow one of these to be empty and still
process the other one?  In other words, shouldn't the bail out condition
be:

if not (asset_prefix or asset_postfix):
return

I don't think any change to AssetMappingProcessor is required because
urljoin already does the right thing when handed the empty string and
the config assure that if no value is specified then that is what we
get.

>  
>  md.treeprocessors.add('asset_mapping',
> -  AssetMappingProcessor(asset_prefix),
> +  AssetMappingProcessor(asset_prefix, 
> asset_postfix),
>'_end')
>  
>  
> @@ -333,8 +338,8 @@ extension_configs = {
>  "markdown.extensions.codehilite":{"css_class":"highlight"}
>  }
>  
> -if len(sys.argv) > 2:
> -extensions.append(AssetMappingExtension(asset_prefix=sys.argv[2]))
> +if len(sys.argv) > 3:
> +
> extensions.append(AssetMappingExtension(asset_prefix=sys.argv[2],asset_postfix=sys.argv[3]))

Can we allow specifying only the prefix here?  Something like:

if len(sys.argv) > 2:
args = {'asset_prefix': sys.argv[2]}
if len(sys.argv) > 3:
args['asset_postfix'] = sys.argv[3]
extensions.append(AssetMappingExtension(**args))

>  
>  # Note: you may want to run this through bleach for sanitization
>  markdown.markdownFromFile(output_format="html5", extensions=extensions, 
> extension_configs=extension_configs)
> 
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2 04/15] Add source page

[John Keeping]

On Mon, Jun 18, 2018 at 10:57:35AM +0800, Andy Green wrote:
> From: John Keeping 
> 
> We are about to introduce rendering of content for the tree view.  This
> source page will allow bypassing the renderer and accessing the content
> of the current tree view.
> 
> Signed-off-by: John Keeping 
> ---

While testing this series, I noticed a slight deficiency in the source
view in that nothing in the header is highlighted.

The following fixup seems like the right thing to do.  What do you
think?

-- >8 --
Subject: [PATCH] fixup! Add source page

Signed-off-by: John Keeping 
---
 ui-shared.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/ui-shared.c b/ui-shared.c
index f358a68..c769ff8 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1006,6 +1006,10 @@ void cgit_print_pageheader(void)
if (ctx.qry.page && !strcmp(ctx.qry.page, "blame"))
cgit_blame_link("blame", NULL, hc("blame"), 
ctx.qry.head,
ctx.qry.sha1, ctx.qry.vpath);
+   else if (ctx.qry.page && !strcmp(ctx.qry.page, "source"))
+   cgit_source_link("tree", NULL, hc("source"),
+ctx.qry.head, ctx.qry.sha1,
+ctx.qry.vpath);
else
cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
   ctx.qry.sha1, ctx.qry.vpath);
-- 
2.17.1

___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH v2] blame: css: make blame highlight div absolute and top left

[John Keeping]

On Mon, Jun 18, 2018 at 02:02:54PM +0800, Andy Green wrote:
> Normal operation of blame view requires div.highlight to
> have absolute position and set to its parent's top left
> for me.
> 
> Otherwise the grey background boxes indicating the extent of
> the patch in the lines td displace the highlit sources, they
> start at the bottom of the td.
> 
> This patch makes the blame highlight div start back up the top of
> its parent area and render on top of the grey boxes.
> 
> Checked on Linux Firefox 60 and Linux Chrome 69.

Which browser is this broken in?  I tried Linux Firefox 60 and Chromium
67 and it looks ok without this patch.  (I'm not opposed to the patch in
principle, indeed it seems like a sensible change, but I'm curious why I
can't reproduce the problem.)

> "highlight" div class name is also used in md2html rendering
> output.  So this patch solves it by introducing a wrapper
> div and new "blame_highlight" css class.
> 
> Signed-off-by: Andy Green 
> ---
>  cgit.css   |2 ++
>  ui-blame.c |4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/cgit.css b/cgit.css
> index da8d9b0..5a85ceb 100644
> --- a/cgit.css
> +++ b/cgit.css
> @@ -162,6 +162,8 @@ div#cgit table.list 
> tr.nohover-highlight:hover:nth-child(odd) {
>   background: white;
>  }
>  
> +div#cgit div.blame_highlight { position: absolute; top: 0; left: 0; }

Is the "left" needed here?  I don't see any problem with setting the
position and top attributes, but setting left:0 moves the content right
up against the cell boundary where before we had a slight gap.

> +
>  div#cgit table.list th {
>   font-weight: bold;
>   /* color: #888;
> diff --git a/ui-blame.c b/ui-blame.c
> index 6e23f0b..ab44e3f 100644
> --- a/ui-blame.c
> +++ b/ui-blame.c
> @@ -196,7 +196,7 @@ static void print_object(const struct object_id *oid, 
> const char *path,
>   free((void *)sb.final_buf);
>  
>   /* Lines */
> - html("");
> + html(" ");

No need for a space before  here.

>   if (ctx.repo->source_filter) {
>   char *filter_arg = xstrdup(basename);
>   cgit_open_filter(ctx.repo->source_filter, filter_arg);
> @@ -207,7 +207,7 @@ static void print_object(const struct object_id *oid, 
> const char *path,
>   html_txt(buf);
>   }
>  
> - html("");
> + html("");
>  
>   html("\n");
>  
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/1] global: remove functionality we deprecated for cgit v1.0

[John Keeping]

On Mon, Jun 18, 2018 at 11:51:41AM +0200, Christian Hesse wrote:
> From: Christian Hesse 
> 
> The man page states these were deprecated for v1.0. We are past v1.1,
> so remove the functionality.
> 
> Signed-off-by: Christian Hesse 

Reviewed-by: John Keeping 

> ---
>  cgit.c| 17 +++--
>  cgit.h|  3 ---
>  cgitrc.5.txt  | 21 -
>  ui-repolist.c |  3 ---
>  ui-shared.c   |  2 --
>  5 files changed, 3 insertions(+), 43 deletions(-)
> 
> diff --git a/cgit.c b/cgit.c
> index ca0a89c..223dfc8 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -111,7 +111,7 @@ static void config_cb(const char *name, const char *value)
>  {
>   const char *arg;
>  
> - if (!strcmp(name, "section") || !strcmp(name, "repo.group"))
> + if (!strcmp(name, "section"))
>   ctx.cfg.section = xstrdup(value);
>   else if (!strcmp(name, "repo.url"))
>   ctx.repo = cgit_add_repo(value);
> @@ -139,20 +139,14 @@ static void config_cb(const char *name, const char 
> *value)
>   ctx.cfg.header = xstrdup(value);
>   else if (!strcmp(name, "logo"))
>   ctx.cfg.logo = xstrdup(value);
> - else if (!strcmp(name, "index-header"))
> - ctx.cfg.index_header = xstrdup(value);
> - else if (!strcmp(name, "index-info"))
> - ctx.cfg.index_info = xstrdup(value);
>   else if (!strcmp(name, "logo-link"))
>   ctx.cfg.logo_link = xstrdup(value);
>   else if (!strcmp(name, "module-link"))
>   ctx.cfg.module_link = xstrdup(value);
>   else if (!strcmp(name, "strict-export"))
>   ctx.cfg.strict_export = xstrdup(value);
> - else if (!strcmp(name, "virtual-root")) {
> + else if (!strcmp(name, "virtual-root"))
>   ctx.cfg.virtual_root = ensure_end(value, '/');
> - } else if (!strcmp(name, "nocache"))
> - ctx.cfg.nocache = atoi(value);
>   else if (!strcmp(name, "noplainemail"))
>   ctx.cfg.noplainemail = atoi(value);
>   else if (!strcmp(name, "noheader"))
> @@ -236,7 +230,7 @@ static void config_cb(const char *name, const char *value)
>   else if (!strcmp(name, "project-list"))
>   ctx.cfg.project_list = xstrdup(expand_macros(value));
>   else if (!strcmp(name, "scan-path"))
> - if (!ctx.cfg.nocache && ctx.cfg.cache_size)
> + if (ctx.cfg.cache_size)
>   process_cached_repolist(expand_macros(value));
>   else if (ctx.cfg.project_list)
>   scan_projects(expand_macros(value),
> @@ -355,7 +349,6 @@ static void prepare_context(void)
>  {
>   memset(, 0, sizeof(ctx));
>   ctx.cfg.agefile = "info/web/last-modified";
> - ctx.cfg.nocache = 0;
>   ctx.cfg.cache_size = 0;
>   ctx.cfg.cache_max_create_time = 5;
>   ctx.cfg.cache_root = CGIT_CACHE_ROOT;
> @@ -973,8 +966,6 @@ static void cgit_parse_args(int argc, const char **argv)
>   }
>   if (skip_prefix(argv[i], "--cache=", )) {
>   ctx.cfg.cache_root = xstrdup(arg);
> - } else if (!strcmp(argv[i], "--nocache")) {
> - ctx.cfg.nocache = 1;
>   } else if (!strcmp(argv[i], "--nohttp")) {
>   ctx.env.no_http = "1";
>   } else if (skip_prefix(argv[i], "--query=", )) {
> @@ -1095,8 +1086,6 @@ int cmd_main(int argc, const char **argv)
>   else
>   ctx.page.expires += ttl * 60;
>   if (!ctx.env.authenticated || (ctx.env.request_method && 
> !strcmp(ctx.env.request_method, "HEAD")))
> - ctx.cfg.nocache = 1;
> - if (ctx.cfg.nocache)
>   ctx.cfg.cache_size = 0;
>   err = cache_process(ctx.cfg.cache_size, ctx.cfg.cache_root,
>   ctx.qry.raw, ttl, process_request);
> diff --git a/cgit.h b/cgit.h
> index 0798dc5..6feca68 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -197,8 +197,6 @@ struct cgit_config {
>   char *footer;
>   char *head_include;
>   char *header;
> - char *index_header;
> - char *index_info;
>   char *logo;
>   char *logo_link;
>   char *mimetype_file;
> @@ -248,7 +246,6 @@ struct cgit_config {
>   int max_repodesc_len;
>   int max_blob_size;
>   int max_stats;
> - int nocache;
>   int noplainemail;
>   int noheader;
>   int renamelimit;
> diff --git 

Re: [PATCH] Update COPYING

[John Keeping]

On Sat, Jun 16, 2018 at 09:57:37PM -0400, Todd Zullinger wrote:
> The address of the Free Software Foundation has changed since the
> license was added in 7640d90 ("Add license file and copyright notices",
> 2006-12-10).  Update the license file from gnu.org¹.
> 
> The only non-whitespace changes are the updated FSF address and two
> references to the L in LGPL changed from Library to Lesser.
> 
> ¹ https://www.gnu.org/licenses/old-licenses/gpl-2.0.txt

Missing sign-off.

Acked-by: John Keeping 

> ---
> This will help users & distributors who download & package cgit.
> 
>  COPYING | 39 +++
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/COPYING b/COPYING
> index 5b6e7c6..d159169 100644
> --- a/COPYING
> +++ b/COPYING
> @@ -1,12 +1,12 @@
> - GNU GENERAL PUBLIC LICENSE
> -Version 2, June 1991
> +GNU GENERAL PUBLIC LICENSE
> +   Version 2, June 1991
>  
> - Copyright (C) 1989, 1991 Free Software Foundation, Inc.
> -   59 Temple Place, Suite 330, Boston, MA  02111-1307  
> USA
> + Copyright (C) 1989, 1991 Free Software Foundation, Inc.,
> + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   Everyone is permitted to copy and distribute verbatim copies
>   of this license document, but changing it is not allowed.
>  
> - Preamble
> +Preamble
>  
>The licenses for most software are designed to take away your
>  freedom to share and change it.  By contrast, the GNU General Public
> @@ -15,7 +15,7 @@ software--to make sure the software is free for all its 
> users.  This
>  General Public License applies to most of the Free Software
>  Foundation's software and to any other program whose authors commit to
>  using it.  (Some other Free Software Foundation software is covered by
> -the GNU Library General Public License instead.)  You can apply it to
> +the GNU Lesser General Public License instead.)  You can apply it to
>  your programs, too.
>  
>When we speak of free software, we are referring to freedom, not
> @@ -55,8 +55,8 @@ patent must be licensed for everyone's free use or not 
> licensed at all.
>  
>The precise terms and conditions for copying, distribution and
>  modification follow.
> -
> - GNU GENERAL PUBLIC LICENSE
> +
> +GNU GENERAL PUBLIC LICENSE
> TERMS AND CONDITIONS FOR COPYING, DISTRIBUTION AND MODIFICATION
>  
>0. This License applies to any program or other work which contains
> @@ -110,7 +110,7 @@ above, provided that you also meet all of these 
> conditions:
>  License.  (Exception: if the Program itself is interactive but
>  does not normally print such an announcement, your work based on
>  the Program is not required to print an announcement.)
> -
> +
>  These requirements apply to the modified work as a whole.  If
>  identifiable sections of that work are not derived from the Program,
>  and can be reasonably considered independent and separate works in
> @@ -168,7 +168,7 @@ access to copy from a designated place, then offering 
> equivalent
>  access to copy the source code from the same place counts as
>  distribution of the source code, even though third parties are not
>  compelled to copy the source along with the object code.
> -
> +
>4. You may not copy, modify, sublicense, or distribute the Program
>  except as expressly provided under this License.  Any attempt
>  otherwise to copy, modify, sublicense or distribute the Program is
> @@ -225,7 +225,7 @@ impose that choice.
>  
>  This section is intended to make thoroughly clear what is believed to
>  be a consequence of the rest of this License.
> -
> +
>8. If the distribution and/or use of the Program is restricted in
>  certain countries either by patents or by copyrighted interfaces, the
>  original copyright holder who places the Program under this License
> @@ -255,7 +255,7 @@ make exceptions for this.  Our decision will be guided by 
> the two goals
>  of preserving the free status of all derivatives of our free software and
>  of promoting the sharing and reuse of software generally.
>  
> - NO WARRANTY
> +NO WARRANTY
>  
>11. BECAUSE THE PROGRAM IS LICENSED FREE OF CHARGE, THERE IS NO WARRANTY
>  FOR THE PROGRAM, TO THE EXTENT PERMITTED BY APPLICABLE LAW.  EXCEPT WHEN
> @@ -277,9 +277,9 @@ YOU OR THIRD PARTIES OR A FAILURE OF THE PROGRAM TO 
> OPERATE WITH ANY OTHER
>  PROGRAMS), EVEN IF SUCH HOLDER OR OTHER PARTY HAS BEEN ADVISED OF

Re: Rendering of README.md inline with inner tree view dirs

[John Keeping]

On Sat, Jun 16, 2018 at 03:12:09PM +0100, John Keeping wrote:
> > >> 3) You can see on the top level of the tree, the README.md references
> > >>
> > >> 
> > >>
> > >> 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/.
> > > 
> > > Making tree do this will break the normal use of tree unless we add some
> > > extra query parameter or path element.  Given that, I think teaching the
> > > renderer to use a path to /about/ is the right thing to do.
> > 
> > OK.  Unfortunately I don't know python very well.  It looks like the 
> > markdown python library is able to be told to use extensions that are 
> > capable to do this
> > 
> > https://python-markdown.github.io/extensions/api/
> > 
> > from the md2html wrapper.  But I don't know enough python to do it.
> > 
> > It's a shame, because in-tree assets correctly follow the ref context 
> > being viewed, eg, if you look at a v2 branch you see v2 pngs, master you 
> > see master pngs etc.
> > 
> > I'll "solve" this part for now by changing the README to use external URLs.
> 
> Yeah, I think we have to solve it by having the filter apply a mapping.
> We have ui-plain which provides the right content for images, but what
> should we do for link targets?
> 
> For the purpose of discussion, consider the following HTML fragment that
> could be generated by rendering a README file:
> 
>   
>   For more details see the dedicated
>   data flow document.
> 
> If dataflow.html is generated from a source file in a similar way, then
> it doesn't exist in the repository and we can't link to it, so the ideal
> output ends up being something like:
> 
>   
>   For more details see the dedicated
>   data flow document.
> 
> The render filter API isn't finalised yet, so we can change the
> parameters that are passed in order to add more information for the
> renderer to use.  At the very least I think we should add a parameter
> for the asset prefix which is essentially the tree path with /tree/
> replaces with /plain/.
> 
> However, I'm not sure how to handle relative links: do we need to pass
> additional parameters for this?  Or can we rely on a render filter doing
> the right thing?

Modifying md2html to use the extension API is reasonably
straightforward.  Below is a modified version which remaps the "src"
attribute on  elements according to a second command line argument,
you can try it out with:

md2html 8 --
#!/usr/bin/env python3
import markdown
import sys
import io
from markdown.util import etree
from pygments.formatters import HtmlFormatter
from urllib.parse import urljoin


class AssetMappingProcessor(markdown.treeprocessors.Treeprocessor):

def __init__(self, asset_prefix):
self.asset_prefix = asset_prefix

def run(self, root):
asset_prefix = self.asset_prefix
for img in root.iter('img'):
src = img.get('src')
if src is None:
continue
img.set('src', urljoin(asset_prefix, src))


class AssetMappingExtension(markdown.extensions.Extension):

def __init__(self, **kwargs):
self.config = {'asset_prefix': ['', 'prefix for relative asset URLs']}
super(AssetMappingExtension, self).__init__(**kwargs)

def extendMarkdown(self, md, md_globals):
asset_prefix = self.getConfig('asset_prefix')
if not asset_prefix:
return

md.treeprocessors.add('asset_mapping',
  AssetMappingProcessor(asset_prefix),

Re: [RFC PATCH] Makefile: work around parallel make issues in docs

[John Keeping]

Hi Todd,

On Sat, Jun 16, 2018 at 12:32:06PM -0400, Todd Zullinger wrote:
> John Keeping wrote:
> > How about the patch below instead?  It's a bigger change to the output
> > format for HTML, but as a side effect it fixes the parallel build.
> 
> It does, but only if the targets are 'doc-man doc-html'.
> For the default 'doc' target which includes 'doc-pdf', the
> issue is still present.
> 
> As a bonus, the output is much nicer, I think.
> 
> We could perhaps work around the 'doc-pdf' failure by adding a
> dep on '($DOC_MAN5)':
> 
> -- >8 --
> diff --git i/Makefile w/Makefile
> index 70f32a4..4879a5a 100644
> --- i/Makefile
> +++ w/Makefile
> @@ -143,7 +143,7 @@ $(DOC_HTML): %.html : %.txt
> $(TXT_TO_HTML) -o $@+ $< && \
> mv $@+ $@
>  
> -$(DOC_PDF): %.pdf : %.txt
> +$(DOC_PDF): %.pdf : $(DOC_MAN5) %.txt
> a2x -f pdf cgitrc.5.txt
>  
>  clean: clean-doc
> -- >8 --
> 
> That's hackish, no doubt.
> 
> We might also want to drop 'doc-pdf' from the default 'doc'
> target.  The alternative is driving the asciidoc pipeline
> for the pdf generation too.  That looks a little more
> involved than doing it for html, but perhaps it's not as bad
> as I think.

I think we can definitely drop doc-pdf from the default output.

I'm half tempted to say we should just delete the PDF output completely
and see if anyone complains, unless you know of anyone using this?

Otherwise, the dependency on $(DOC_MAN5) seems reasonable to me,
probably accompanied with a comment explaining the clash in a2x's
intermediate files.


Regards,
John
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: Bug: cgit shows empty pages, when viewing Tag-Only-Repo

[John Keeping]

On Fri, May 04, 2018 at 08:20:10AM +0200, Björn Lässig wrote:
>  I use repositories for holding some tags.
> Unfortunately cgit shows only
> 
>   Repository seems to be empty
> 
> I looked into the code and saw the problem, but i have not the insight
> to fix this.
> 
> one (small) example for the Problem is:
> 
> https://git.pengutronix.de/cgit/ukl/distrokit/refs/
> 
> git ls-remote https://git.pengutronix.de/git/ukl/distrokit/
> shows a handful of refs.

I had a related patch hanging around which improves the behaviour when
the default branch doesn't exist, and it is easy to expand this to pick
a default ref when there are no branches.

I'm not sure if I sent this to the list in the past; certainly there are
arguments as to why this behaviour isn't desirable.

The requirement for a default ref is baked in throughout the CGit code
base and I don't think changing that is worthwhile, but providing we can
pick some ref as the default, it doesn't necessarily have to be a
branch.  So this patch allows any ref to be the default (albeit with a
strong preference for branches if any exist).

-- >8 --
Subject: [PATCH] cgit: improve default branch selection

If the named default branch does not exist (for example because none is
specified in the config and "master" is not used in the repositry), then
we currently select the first branch in whatever order Git returns them
to us.

Instead, let's use the newest branch (by commit date), since this is
likely to be the most interesting branch in the repository.  This does
make default branch selection more expensive, but people who care should
specify a default branch in their repository configuration.

Further, if a repository has no branches but does have tags, let's pick
the newest tag as the default head [1].  This is a separate pass because
we want to prefer branches if there are any.

[1] In fact, we allow any ref at this point because we want to avoid
falsely claiming that the repository is empty when it isn't.

Signed-off-by: John Keeping 
---
 cgit.c | 46 +++---
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/cgit.c b/cgit.c
index bd9cb3f..43b8c96 100644
--- a/cgit.c
+++ b/cgit.c
@@ -430,7 +430,6 @@ static void prepare_context(void)
 
 struct refmatch {
char *req_ref;
-   char *first_ref;
int match;
 };
 
@@ -442,34 +441,59 @@ static int find_current_ref(const char *refname, const 
struct object_id *oid,
info = (struct refmatch *)cb_data;
if (!strcmp(refname, info->req_ref))
info->match = 1;
-   if (!info->first_ref)
-   info->first_ref = xstrdup(refname);
return info->match;
 }
 
-static void free_refmatch_inner(struct refmatch *info)
+struct newest_ref {
+   char *ref;
+   unsigned long date;
+};
+
+static int find_newest_ref(const char *refname, const struct object_id *oid,
+  int flags, void *cb_data)
 {
-   if (info->first_ref)
-   free(info->first_ref);
+   struct newest_ref *newest = cb_data;
+   struct object *object;
+   unsigned long date;
+
+   object = parse_object(oid);
+   if (object->type == OBJ_COMMIT) {
+   date = ((struct commit *)object)->date;
+   if (date > newest->date) {
+   free(newest->ref);
+   newest->ref = xstrdup(refname);
+   newest->date = date;
+   }
+   }
+   return 0;
 }
 
 static char *find_default_branch(struct cgit_repo *repo)
 {
struct refmatch info;
+   struct newest_ref newest = { NULL };
char *ref;
 
info.req_ref = repo->defbranch;
-   info.first_ref = NULL;
info.match = 0;
for_each_branch_ref(find_current_ref, );
-   if (info.match)
+   if (info.match) {
ref = info.req_ref;
-   else
-   ref = info.first_ref;
+   } else {
+   /*
+* If the default branch isn't found, pick the most recently
+* updated branch, and if there are no branches, check if
+* this is a tag-only repo and pick any ref we can.
+*/
+   for_each_branch_ref(find_newest_ref, );
+   if (!newest.ref)
+   for_each_ref(find_newest_ref, );
+   ref = newest.ref;
+   }
if (ref)
ref = xstrdup(ref);
-   free_refmatch_inner();
 
+   free(newest.ref);
return ref;
 }
 
-- 
2.17.1

___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH] ui-tag: Fix inconsistent capitalization

[John Keeping]

On Sun, Jun 10, 2018 at 06:28:49PM -0400, Jon DeVree wrote:
> Way back in 2009 all of these were lower cased except this one
> occurrence.
> 
> Signed-off-by: Jon DeVree 

Thanks!  I've picked this up in jk/for-jason.

> ---
>  ui-tag.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ui-tag.c b/ui-tag.c
> index 909cde0..2c216d0 100644
> --- a/ui-tag.c
> +++ b/ui-tag.c
> @@ -107,7 +107,7 @@ void cgit_print_tag(char *revname)
>   htmlf("tag name");
>   html_txt(revname);
>   html("\n");
> - html("Tagged object");
> + html("tagged object");
>   cgit_object_link(obj);
>   html("\n");
>   if (ctx.repo->snapshots)
> -- 
> 2.17.1
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/1] snapshot: support special value 'all' to enable all formats

[John Keeping]

On Thu, Jun 07, 2018 at 10:05:50PM +0200, Christian Hesse wrote:
> From: Christian Hesse 
> 
> Signed-off-by: Christian Hesse 

Reviewed-by: John Keeping 

> ---
>  cgitrc.5.txt | 1 +
>  shared.c | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index a9d3d0a..3bfacfa 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -429,6 +429,7 @@ snapshots::
>   Text which specifies the default set of snapshot formats that cgit
>   generates links for. The value is a space-separated list of zero or
>   more of the values "tar", "tar.gz", "tar.bz2", "tar.xz" and "zip".
> + The special value "all" enables all snapshot formats.
>   Default value: none.
>  
>  source-filter::
> diff --git a/shared.c b/shared.c
> index 21ac8f4..0a11e68 100644
> --- a/shared.c
> +++ b/shared.c
> @@ -390,6 +390,9 @@ int cgit_parse_snapshots_mask(const char *str)
>   if (atoi(str))
>   return 1;
>  
> + if (strcmp(str, "all") == 0)
> + return INT_MAX;
> +
>   string_list_split(, str, ' ', -1);
>   string_list_remove_empty_items(, 0);
>  
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: Translation

[John Keeping]

On Mon, Jun 04, 2018 at 10:24:24AM +0200, Malte Kiefer wrote:
> first of all I want to say thank you for this amazing software.
> I start to using it, and I really like it.
> 
> Then I wanted to inquire with you if you think about offering cgit in
> multiple languages?
> I would like to translate cgit into german.

There's some preparatory work to be done to make CGit translatable.
Patches to do this would be welcome, but I don't think anyone currently
has plans to work on this.

At a minimum, we need patches to:

- Mark user-visible strings in the source as translated and generate
  translation catalogs

- Integrate translations into the build and install process so that they
  can be found and loaded when handling a request

- Extract preferred languages from the request and initialise gettext
  with the most preferred language that is available
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH] cgitrc.5: fix auth filter return value documentation

[John Keeping]

We don't treat all return values as invalid!

Signed-off-by: John Keeping 
---
 cgitrc.5.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 4da166c..3e53bbd 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -715,7 +715,7 @@ auth filter::
When the filter action is "body", this filter must write to output the
HTML for displaying the login form, which POSTs to the login url. When
the filter action is "authenticate-cookie", this filter must validate
-   the http cookie and return a 0 if it is invalid or 1 if it is invalid,
+   the http cookie and return a 0 if it is invalid or 1 if it is valid,
in the exit code / close function. If the filter action is
"authenticate-post", this filter receives POST'd parameters on
standard input, and should write a complete CGI response, preferably
-- 
2.17.1

___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH] Encode value and field before calculating cookie digest, the same way secure_value() does

[John Keeping]

On Thu, Apr 12, 2018 at 08:54:31PM +0300, thev...@gmail.com wrote:
> From: Vlad Safronov 
> 
> Bugfix: Encode value and field before calculating cookie digest, the same way 
> as secure_value() does
> so validating will work correctly on encoded values.

Missing sign-off (see [1] for what this means).

[1] https://elinux.org/Developer_Certificate_Of_Origin

However, I don't think this change is correct.  secure_value() places
the encoded strings into the authstr, which is what validate_value() is
reading, so field and value are already URL encoded here.

This is why field is url_decode()'d later in this function before
comparing with the expected value.

> ---
>  filters/simple-authentication.lua | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/filters/simple-authentication.lua 
> b/filters/simple-authentication.lua
> index de34d09..b40a819 100644
> --- a/filters/simple-authentication.lua
> +++ b/filters/simple-authentication.lua
> @@ -230,6 +230,8 @@ function validate_value(expected_field, cookie)
>   return nil
>   end
>  
> + value = url_encode(value)
> + field = url_encode(field)
>   -- Lua hashes strings, so these comparisons are time invariant.
>   if hmac ~= crypto.hmac.digest("sha1", field .. "|" .. value .. "|" .. 
> tostring(expiration) .. "|" .. salt, secret) then
>   return nil
> -- 
> 2.17.0
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH] expand environmentvariables in root-title and root-descr

[John Keeping]

On Thu, Apr 12, 2018 at 11:19:12AM +0200, b.laes...@pengutronix.de wrote:
> From: Björn Lässig 
> 
> For having personanlized cgit configuration i need to use
> 
>   root-desc=$REMOTE_USER@$HTTP_HOST

Missing sign-off (see [1] for what this means).

Also, this needs a corresponding change in cgitrc.5.txt to indicate that
these variables are now subject to macro expansion.

[1] https://elinux.org/Developer_Certificate_Of_Origin

> ---
>  cgit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/cgit.c b/cgit.c
> index bd9cb3f..1d7d67e 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -120,9 +120,9 @@ static void config_cb(const char *name, const char *value)
>   else if (!strcmp(name, "readme"))
>   string_list_append(, xstrdup(value));
>   else if (!strcmp(name, "root-title"))
> - ctx.cfg.root_title = xstrdup(value);
> + ctx.cfg.root_title = xstrdup(expand_macros(value));
>   else if (!strcmp(name, "root-desc"))
> - ctx.cfg.root_desc = xstrdup(value);
> + ctx.cfg.root_desc = xstrdup(expand_macros(value));
>   else if (!strcmp(name, "root-readme"))
>   ctx.cfg.root_readme = xstrdup(value);
>   else if (!strcmp(name, "css"))
> -- 
> 2.11.0
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: Adding short commit id to repo Log tab

[John Keeping]

Sorry for the delay responding.

On Wed, Apr 11, 2018 at 12:00:28PM +0300, Vlad wrote:
> I think adding short (7 chars) commit id to repository Log tab output
> might be more useful than file or line count..
> 
> I have examined the code, it it not easy to add an extra column
> to the output.
> 
> Would you suggest ways how to implement this?

I'm not sure I agree that this is useful, but personally I wouldn't be
opposed to adding this if the code is simple.

I don't think it's hard to do, you just need to add a new  element
in cgit_print_log() and then write the  element in print_commit(),
making sure that columns is also adjusted.  All of the code should be in
ui-log.c unless you add a config switch to enable this feature..
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/1] RFC: git: update to v2.18.0-rc2

[John Keeping]

On Thu, Jun 14, 2018 at 12:52:25AM +0200, Christian Hesse wrote:
> From: Christian Hesse 
> 
> Update to git version v2.18.0-rc2. Required changes follow upstream commits:
> 
> * Convert find_unique_abbrev* to struct object_id
>   (aab9583f7b5ea5463eb3f653a0b4ecac7539dc94)
> * sha1_file: convert read_sha1_file to struct object_id
>   (b4f5aca40e6f77cbabcbf4ff003c3cf30a1830c8)
> * sha1_file: convert sha1_object_info* to object_id
>   (abef9020e3df87c441c9a3a95f592fce5fa49bb9)
> * object-store: move packed_git and packed_git_mru to object store
>   (a80d72db2a73174b3f22142eb2014b33696fd795)
> * treewide: rename tree to maybe_tree
>   (891435d55da80ca3654b19834481205be6bdfe33)
> 
> The changed data types required some of our own functions to be converted
> to struct object_id:
> 
>   ls_item
>   print_dir
>   print_dir_entry
>   print_object
>   single_tree_cb
>   walk_tree
>   write_tree_link
> 
> And finally we use new upstream functions that were added for
> struct object_id:
> 
>   hashcpy -> oidcpy
>   sha1_to_hex -> oid_to_hex
> 
> Signed-off-by: Christian Hesse 

With Andy's fix,

Reviewed-by: John Keeping 

It looks like our interface to libgit.a is completely converted to
struct object_id after this update!

But we still have a few mentions of sha1 in our code.  Most of this is
sha1 and sha2 in struct cgit_query, but these aren't actually hashes
most of the time, they can be any object reference that Git understands,
so I think we should rename them to ref1 and ref2 (along with has_sha1
-> has_ref).  What do you think?

(ui-blob does require sha1 to be hex, but it's a bit of a special case
anyway and I think it's fine to impose an extra requirement there.)

After that, I think all that's left is parsing.c and there's no reason
to do anything there until we now how newhash will alter the commit and
tag object encodings.
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: cache-size implementation downsides

[John Keeping]

On Wed, Jun 13, 2018 at 03:02:42PM -0400, Konstantin Ryabitsev wrote:
> 2. I have witnessed cache corruption due to collisions (which is
> a bug in itself). One of our frontends was hit by a lot of agressive
> crawling of snapshots that raised the load to 60+ (many, many gzip
> processes). After we blackholed the bot, some of the cache objects for
> non-snapshot URLs had trailing gzip junk in them, meaning that either
> two instances were writing to the same file, or something else resulted
> in cache corruption. This is probably a race condition somewhere in the
> locking code.

I've had a look at this, and I think we might end up dropping our lock
too early thanks to this code (in fill_slot()):

/* Restore stdout */
if (dup2(tmp, STDOUT_FILENO) == -1) {

Before this line, STDOUT_FILENO refers to lock_fd which has a POSIX
advisory record lock on the entire file.  However, the documentation for
that says:

   *  If a process closes any file descriptor referring to a file, then all
  of the process's locks on that file are released, regardless of the
  file descriptor(s)  on  which  the  locks  were  obtained.  This is
  bad: it means that a process can lose its locks on a file such as
  /etc/passwd or  /etc/mtab  when for some reason a library function
  decides to open, read, and close the same file.

I haven't verified this, but I suspect that dup'ing the original stdout
over STDOUT_FILENO is equivalent to closing a file descriptor referring
to our lock file.  And thus the lock is released at this point, which is
before we rename the lock file over the cache file.

If that is correct, then there is a window during which a new process
can open the lock file to write new content and successfully acquire the
lock on that file even though it is still being used by another process.

-- >8 --
Subject: [PATCH] cache: close race window when unlocking slots

We use POSIX advisory record locks to control access to cache slots, but
these have an unhelpful behaviour in that they are released when any
file descriptor referencing the file is closed by this process.

Mostly this is okay, since we know we won't be opening the lock file
anywhere else, but there is one place that it does matter: when we
restore stdout we dup2() over a file descriptor referring to the file,
thus closing that descriptor.

Since we restore stdout before unlocking the slot, this creates a window
during which the slot content can be overwritten.  The fix is reasonably
straightforward: simply restore stdout after unlocking the slot, but the
diff is a bit bigger because this requires us to move the temporary
stdout FD into struct cache_slot.

Signed-off-by: John Keeping 
---
 cache.c | 37 ++---
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/cache.c b/cache.c
index 0901e6e..2c70be7 100644
--- a/cache.c
+++ b/cache.c
@@ -29,6 +29,7 @@ struct cache_slot {
cache_fill_fn fn;
int cache_fd;
int lock_fd;
+   int stdout_fd;
const char *cache_name;
const char *lock_name;
int match;
@@ -197,6 +198,13 @@ static int unlock_slot(struct cache_slot *slot, int 
replace_old_slot)
else
err = unlink(slot->lock_name);
 
+   /* Restore stdout and close the temporary FD. */
+   if (slot->stdout_fd >= 0) {
+   dup2(slot->stdout_fd, STDOUT_FILENO);
+   close(slot->stdout_fd);
+   slot->stdout_fd = -1;
+   }
+
if (err)
return errno;
 
@@ -208,42 +216,24 @@ static int unlock_slot(struct cache_slot *slot, int 
replace_old_slot)
  */
 static int fill_slot(struct cache_slot *slot)
 {
-   int tmp;
-
/* Preserve stdout */
-   tmp = dup(STDOUT_FILENO);
-   if (tmp == -1)
+   slot->stdout_fd = dup(STDOUT_FILENO);
+   if (slot->stdout_fd == -1)
return errno;
 
/* Redirect stdout to lockfile */
-   if (dup2(slot->lock_fd, STDOUT_FILENO) == -1) {
-   close(tmp);
+   if (dup2(slot->lock_fd, STDOUT_FILENO) == -1)
return errno;
-   }
 
/* Generate cache content */
slot->fn();
 
/* Make sure any buffered data is flushed to the file */
-   if (fflush(stdout)) {
-   close(tmp);
+   if (fflush(stdout))
return errno;
-   }
 
/* update stat info */
-   if (fstat(slot->lock_fd, >cache_st)) {
-   close(tmp);
-   return errno;
-   }
-
-   /* Restore stdout */
-   if (dup2(tmp, STDOUT_FILENO) == -1) {
-   close(tmp);
-   return errno;
-   }
-
-   /* Close the temporary filedescriptor */
-   if (close(tmp))
+   if (fstat(slot->lock_fd, >cache_st))
return errno;
 
return 0;
@@ -393,6 +383,7 @@

Re: [PATCH 10/11] config: add tree-readme list

[John Keeping]

On Wed, Jun 13, 2018 at 10:02:20AM +0800, Andy Green wrote:
> Allows the user to specify a list of filenames that should be
> rendered inline with tree view, if present in the directory.
> 
> Signed-off-by: Andy Green 

As mentioned in reply to the cover leter, I think this needs to be a
repo config rather than just a global configuration.  It seems likely
that different repositories may have different preferences or
conventions for directory level readme files.

A few more comments below...

> ---
>  cgit.c   |   11 ++-
>  cgit.h   |1 +
>  cgitrc.5.txt |7 +++
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/cgit.c b/cgit.c
> index 975e573..017ce78 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -1,6 +1,6 @@
>  /* cgit.c: cgi for the git scm
>   *
> - * Copyright (C) 2006-2014 cgit Development Team 
> + * Copyright (C) 2006-2018 cgit Development Team 
>   *
>   * Licensed under GNU General Public License v2
>   *   (see COPYING for full license text)
> @@ -39,6 +39,12 @@ static void add_render_filter(const char *name, const char 
> *cmd)
>   item->util = filter;
>  }
>  
> +static void add_tree_readme(const char *name)
> +{
> + string_list_insert(_readme, name);
> +}
> +
> +

Extra blank line?

>  static void process_cached_repolist(const char *path);
>  
>  static void repo_config(struct cgit_repo *repo, const char *name, const char 
> *value)
> @@ -301,6 +307,8 @@ static void config_cb(const char *name, const char *value)
>   add_mimetype(name + 9, value);
>   else if (starts_with(name, "render."))
>   add_render_filter(name + 7, value);
> + else if (!strcmp(name, "tree-readme"))
> + add_tree_readme(value);
>   else if (!strcmp(name, "include"))
>   parse_configfile(expand_macros(value), config_cb);
>  }
> @@ -435,6 +443,7 @@ static void prepare_context(void)
>   ctx.page.etag = NULL;
>   string_list_init(, 1);
>   string_list_init(_filters, 1);
> + string_list_init(_readme, 1);
>   if (ctx.env.script_name)
>   ctx.cfg.script_name = xstrdup(ctx.env.script_name);
>   if (ctx.env.query_string)
> diff --git a/cgit.h b/cgit.h
> index a19742f..1076568 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -261,6 +261,7 @@ struct cgit_config {
>   int branch_sort;
>   int commit_sort;
>   struct string_list mimetypes;
> + struct string_list tree_readme;
>   struct string_list render_filters;
>   struct cgit_filter *about_filter;
>   struct cgit_filter *commit_filter;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 793a0c1..597 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -469,6 +469,13 @@ strict-export::
>   repositories to match those exported by git-daemon. This option must
>   be defined prior to scan-path.
>  
> +tree-readme::
> + Append given filename to the list of filenames to be rendered after the
> + tree navigation in tree view, if present in the directory being viewed. 
>  Eg,
> + 'tree-readme=README.md'.  There must also be a corresponding render.
> + entry for the readme suffix, eg,
> + 'render.md=/usr/libexec/cgit/filters/html-converters/md2html'

I don't think there does need to be a render entry, there could be a
mimetype entry which will allow the content to be displayed in an
iframe, or in the absense of that the file will simply be included
verbatim which may be absolutely fine.

>   will also cause cgit to generate 'virtual urls', i.e. urls like
> 
> ___
> CGit mailing list
> CGit@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 09/11] ui-tree: ls_tail: add walk table param

[John Keeping]

On Wed, Jun 13, 2018 at 10:02:15AM +0800, Andy Green wrote:
> Arrange that walk_tree_ctx is available in ls_tail, we
> will make use of it shortly.
> 
> Signed-off-by: Andy Green 

Reviewed-by: John Keeping 

> ---
>  ui-tree.c |6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ui-tree.c b/ui-tree.c
> index bb10b17..53b5594 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -368,7 +368,7 @@ static void ls_head(void)
>   html("\n");
>  }
>  
> -static void ls_tail(void)
> +static void ls_tail(struct walk_tree_context *walk_tree_ctx)
>  {
>   html("\n");
>   cgit_print_layout_end();
> @@ -390,7 +390,7 @@ static void ls_tree(const struct object_id *oid, char 
> *path, struct walk_tree_co
>  
>   ls_head();
>   read_tree_recursive(tree, "", 0, 1, , ls_item, walk_tree_ctx);
> - ls_tail();
> + ls_tail(walk_tree_ctx);
>  }
>  
>  
> @@ -473,7 +473,7 @@ void cgit_print_tree(const char *rev, char *path, bool 
> use_render)
>  
>   read_tree_recursive(commit->tree, "", 0, 0, , walk_tree, 
> _tree_ctx);
>   if (walk_tree_ctx.state == 1)
> - ls_tail();
> + ls_tail(_tree_ctx);
>   else if (walk_tree_ctx.state == 2)
>   cgit_print_layout_end();
>   else
> 
> ___
> CGit mailing list
> CGit@lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 08/11] ui-tree: print_object: add is_inline param

[John Keeping]

On Wed, Jun 13, 2018 at 10:02:10AM +0800, Andy Green wrote:
> We will reuse print_object to render things inline shortly.
> 
> Add a parameter that lets us adapt its behaviour slightly
> for that case.
> 
> Signed-off-by: Andy Green 
> ---
>  ui-tree.c |7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ui-tree.c b/ui-tree.c
> index fe5dc75..bb10b17 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -158,7 +158,7 @@ static void include_file(const unsigned char *sha1, const 
> char *path,
>  }
>  
>  static void print_object(const unsigned char *sha1, char *path, const char 
> *basename,
> -  const char *rev, bool use_render)
> +  const char *rev, bool use_render, bool is_inline)

I don't think this is the right way to handle this.  There's other
behaviour here that needs to be suppressed if this is inline content,
such as the call to cgit_set_title_from_path(), and I don't think we
want the object header above the readme content.

Additionally, if read_sha1_file() fails we'll output another page header
here.

Since it's not much code overall, I think we probably don't want to
reuse print_object() for the inline rendering, but can instead just do
the render & mimetype lookup and then do:

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

>  {
>   enum object_type type;
>   struct cgit_filter *render;
> @@ -191,7 +191,8 @@ static void print_object(const unsigned char *sha1, char 
> *path, const char *base
>   if (!render && !mimetype)
>   use_render = false;
>  
> - cgit_print_layout_start();
> + if (!is_inline)
> + cgit_print_layout_start();
>   htmlf("blob: %s (", sha1_to_hex(sha1));
>   cgit_plain_link("plain", NULL, NULL, ctx.qry.head,
>   rev, path);
> @@ -415,7 +416,7 @@ static int walk_tree(const unsigned char *sha1, struct 
> strbuf *base,
>   } else {
>   walk_tree_ctx->state = 2;
>   print_object(sha1, buffer.buf, pathname, 
> walk_tree_ctx->curr_rev,
> -  walk_tree_ctx->use_render);
> +  walk_tree_ctx->use_render, 0);
>   strbuf_release();
>  
>   return 0;
> 
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 05/11] ui-tree: use render fileters to display content

[John Keeping]

If you're including these patches in your series, please fix my typo in
the subject ("fileters" has a stray 'e')  :-)

On Wed, Jun 13, 2018 at 10:01:55AM +0800, Andy Green wrote:
> From: John Keeping 
> 
> This allows applying filters to files in the repository, for example to
> render Markdown or AsciiDoc as HTML.
> 
> Signed-off-by: John Keeping 
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 06/11] ui-tree: free read_sha1_file() buffer after use

[John Keeping]

On Wed, Jun 13, 2018 at 10:02:00AM +0800, Andy Green wrote:
> Free up the buffer allocated in read_sha1_file()
> 
> Signed-off-by: Andy Green 

Reviewed-by: John Keeping 

I've extracted this from the series and pushed it on my for-jason
branch: https://git.zx2c4.com/cgit/log/?h=jk/for-jason

> ---
>  ui-tree.c |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ui-tree.c b/ui-tree.c
> index 723e16e..fe5dc75 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -222,6 +222,7 @@ static void print_object(const unsigned char *sha1, char 
> *path, const char *base
>   }
>  
>   free(mimetype);
> + free(buf);
>  }
>  
>  struct single_tree_ctx {
> 
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 07/11] ui-blame: free read_sha1_file() buffer after use

[John Keeping]

On Wed, Jun 13, 2018 at 10:02:05AM +0800, Andy Green wrote:
> Signed-off-by: Andy Green 
> ---
>  ui-blame.c |3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ui-blame.c b/ui-blame.c
> index 17e2d60..a5c7d69 100644
> --- a/ui-blame.c
> +++ b/ui-blame.c
> @@ -206,6 +206,9 @@ static void print_object(const unsigned char *sha1, const 
> char *path,

There's an early return above here after allocating buf, I'm not sure if
we care about freeing buf on that path as well.

If we do, I guess we push free(buf) to the end and add a free_buf label
for the error path to jump to.

>   } else {
>   html_txt(buf);
>   }
> +
> + free(buf);
> +
>   html("");
>  
>   html("\n");
> 
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 00/11] Render READMEs inline in tree view

[John Keeping]

On Thu, Jun 14, 2018 at 11:47:41AM +0800, Andy Green wrote:
> On 06/13/2018 10:01 AM, Andy Green wrote:
> > The following series adds config to allow rendering of
> > selected READMEs inline after the tree view, where
> > present in the directory being viewed.
> > 
> > It builds on John Keeping's RENDER mode series from 2016.
> > 
> > Typical config to enable it, if you have a README.md
> > looks like
> > 
> > tree-readme=README.md
> > render.md=/usr/libexec/cgit/filters/html-converters/md2html
> 
> I have updated this against Christian's git 2.18.0-rc2 patch from 
> earlier, rather than post it to the list again before any comment you 
> can find it here if you're interested.
> 
> https://warmcat.com/git/cgit/
> 
> Presumably this is what should be targeted for the next push rather than 
> current master.

I think that makes sense, yes.  Git 2.18.0 final is due out tomorrow [1]
so we should get the version bump committed pretty soon.


[1] https://tinyurl.com/gitcal
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 1/2] gcc8.1: fix strncpy bounds warnings

[John Keeping]

On Sat, Jun 16, 2018 at 09:12:08PM +0800, Andy Green wrote:
> 
> 
> On June 16, 2018 9:04:48 PM GMT+08:00, John Keeping  
> wrote:
> >On Wed, Jun 13, 2018 at 07:33:59AM +0800, Andy Green wrote:
> >> 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);
> >>  ^
> >> 
> >> strncpy with a computed length via strlen is usually
> >> not the right thing.
> >> 
> >> ../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));
> >>   ^~~
> >> 
> >> add one char of padding and adjust so the code does the same.
> >> 
> >> Signed-off-by: Andy Green 
> >> ---
> >>  shared.c|2 +-
> >>  ui-shared.c |7 ---
> >>  2 files changed, 5 insertions(+), 4 deletions(-)
> >> 
> >> 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);
> >
> >This is a change in behaviour because strncpy is guaranteed to null
> >terminate the output (even writing one beyond len if necessary) whereas
> >memcpy does not.
> 
> Eh... are you sure about that?  It's not my understanding, and --->
> 
> https://linux.die.net/man/3/strncpy
> 
> The strncpy() function is similar, except that at most n bytes of src
> are copied. Warning: If there is no null byte among the first n bytes
> of src, the string placed in dest will not be null-terminated. 

Yes, I'm getting it confused with strncat.  And in this case we do
ensure that the output is null terminated separately.
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: Rendering of README.md inline with inner tree view dirs

[John Keeping]

On Wed, Jun 13, 2018 at 09:47:38AM +0800, Andy Green wrote:
> 
> 
> On 06/12/2018 05:31 PM, John Keeping wrote:
> > On Tue, Jun 12, 2018 at 01:53:27PM +0800, Andy Green wrote:
> >> On 06/11/2018 11:38 PM, John Keeping wrote:
> >>> On Mon, Jun 11, 2018 at 04:05:38PM +0800, Andy Green wrote:
> 
> >> 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?
> > 
> > I think that list is refs to blobs, so you can specify something like:
> > 
> > refs/heads/wiki:README.md
> > 
> > but here we need base filenames to look for in the current directory, so
> > it will have to be a new config variable.
> 
> OK.  I added a new cgit-scope variable that appends filenames to be 
> considered for showing in tree view, eg,
> 
> tree-readme=README.md

This should be overridable at the repo level to match our other
configuration variables which affect this sort of behaviour.

Also, if this was my bikeshed I'd paint it as "inline-readme", but I
don't feel strongly about that.

> >> 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?
> > 
> > My choice would be to take the first matching file, but I don't have a
> > strong opinion on what is the right behaviour.
> 
> I'll leave this as a list for the first try.  If it looks excessive I'll 
> reduce it to just the one.

The code is definitely simpler if it's just one, and I'm not sure what
benefit we get from including multiple files.  But if you have a use
case for multiple readme files, then a list is okay.

> >> 3) You can see on the top level of the tree, the README.md references
> >>
> >> 
> >>
> >> 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/.
> > 
> > Making tree do this will break the normal use of tree unless we add some
> > extra query parameter or path element.  Given that, I think teaching the
> > renderer to use a path to /about/ is the right thing to do.
> 
> OK.  Unfortunately I don't know python very well.  It looks like the 
> markdown python library is able to be told to use extensions that are 
> capable to do this
> 
> https://python-markdown.github.io/extensions/api/
> 
> from the md2html wrapper.  But I don't know enough python to do it.
> 
> It's a shame, because in-tree assets correctly follow the ref context 
> being viewed, eg, if you look at a v2 branch you see v2 pngs, master you 
> see master pngs etc.
> 
> I'll "solve" this part for now by changing the README to use external URLs.

Yeah, I think we have to solve it by having the filter apply a mapping.
We have ui-plain which provides the right content for images, but what
should we do for link targets?

For the purpose of discussion, consider the following HTML fragment that
could be generated by rendering a README file:


For more details see the dedicated
data flow document.

If dataflow.html is generated from a source file in a similar way, then
it doesn't exist in the repository and we can't link to it, so the ideal
output ends up being something like:

Re: [PATCH 2/2] gcc8.1: fix strcat warning

[John Keeping]

On Wed, Jun 13, 2018 at 07:34:07AM +0800, Andy Green wrote:
> ../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));
> ^
> 
> Actually the strncat that was there before intends that its
> stock of spaces gets truncated, and it's not a problem.
> 
> However gcc8.1 is also right, normally truncation is undesirable.
> 
> Make the code do the padding explicitly.
> 
> Signed-off-by: Andy Green 

Reviewed-by: John Keeping 

> ---
>  ui-ssdiff.c |   12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> 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;
>   }
> 
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 2/2] shared: use strbuf for expanding macros

[John Keeping]

Avoid a fixed size buffer by using struct strbuf to build the expanded
value.

Signed-off-by: John Keeping 
---
 shared.c | 73 +---
 1 file changed, 27 insertions(+), 46 deletions(-)

diff --git a/shared.c b/shared.c
index 32d0f46..6ce3990 100644
--- a/shared.c
+++ b/shared.c
@@ -467,69 +467,50 @@ static int is_token_char(char c)
return isalnum(c) || c == '_';
 }
 
-/* Replace name with getenv(name), return pointer to zero-terminating char
+/* Replace name with getenv(name).
  */
-static char *expand_macro(char *name, int maxlength)
+static void expand_macro(struct strbuf *buf, ssize_t start)
 {
-   char *value;
-   int len;
+   const char *name = buf->buf + start;
+   const char *value = getenv(name);
 
-   len = 0;
-   value = getenv(name);
-   if (value) {
-   len = strlen(value);
-   if (len > maxlength)
-   len = maxlength;
-   strncpy(name, value, len);
-   }
-   return name + len;
+   /* Truncate output to remove variable name. */
+   strbuf_setlen(buf, start);
+
+   if (value)
+   strbuf_addstr(buf, value);
 }
 
-#define EXPBUFSIZE (1024 * 8)
-
 /* Replace all tokens prefixed by '$' in the specified text with the
  * value of the named environment variable.
  * NB: the return value is allocated, it must be freed by the caller.
  */
 char *expand_macros(const char *txt)
 {
-   static char result[EXPBUFSIZE];
-   char *p, *start;
-   int len;
+   struct strbuf result = STRBUF_INIT;
+   ssize_t start = -1;
 
-   p = result;
-   start = NULL;
-   while (p < result + EXPBUFSIZE - 1 && txt && *txt) {
-   *p = *txt;
-   if (start) {
+   while (txt && *txt) {
+   if (start >= 0) {
if (!is_token_char(*txt)) {
-   if (p - start > 0) {
-   *p = '\0';
-   len = result + EXPBUFSIZE - start - 1;
-   p = expand_macro(start, len) - 1;
-   }
-   start = NULL;
-   txt--;
+   if (result.len - start >= 0)
+   expand_macro(, start);
+   start = -1;
}
-   p++;
-   txt++;
-   continue;
}
-   if (*txt == '$') {
-   start = p;
-   txt++;
-   continue;
-   }
-   p++;
+
+   if (*txt == '$')
+   start = result.len;
+   else
+   strbuf_addch(, *txt);
+
txt++;
}
-   *p = '\0';
-   if (start && p - start > 0) {
-   len = result + EXPBUFSIZE - start - 1;
-   p = expand_macro(start, len);
-   *p = '\0';
-   }
-   return xstrdup(result);
+
+   if (start >= 0 && result.len - start > 0)
+   expand_macro(, start);
+
+   return strbuf_detach(, NULL);
 }
 
 char *get_mimetype_for_filename(const char *filename)
-- 
2.17.1

___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 1/2] shared: allocate return value from expand_macros()

[John Keeping]

In preparation for switching the implementation of expand_macros away
from a static buffer, return an allocated buffer and update all callers
to release it.

Signed-off-by: John Keeping 
---
 cgit.c  | 33 +++--
 shared.c|  5 ++---
 ui-shared.c |  9 ++---
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/cgit.c b/cgit.c
index bd9cb3f..ca1bc15 100644
--- a/cgit.c
+++ b/cgit.c
@@ -190,7 +190,7 @@ static void config_cb(const char *name, const char *value)
else if (!strcmp(name, "cache-size"))
ctx.cfg.cache_size = atoi(value);
else if (!strcmp(name, "cache-root"))
-   ctx.cfg.cache_root = xstrdup(expand_macros(value));
+   ctx.cfg.cache_root = expand_macros(value);
else if (!strcmp(name, "cache-root-ttl"))
ctx.cfg.cache_root_ttl = atoi(value);
else if (!strcmp(name, "cache-repo-ttl"))
@@ -232,16 +232,20 @@ static void config_cb(const char *name, const char *value)
else if (!strcmp(name, "max-commit-count"))
ctx.cfg.max_commit_count = atoi(value);
else if (!strcmp(name, "project-list"))
-   ctx.cfg.project_list = xstrdup(expand_macros(value));
-   else if (!strcmp(name, "scan-path"))
+   ctx.cfg.project_list = expand_macros(value);
+   else if (!strcmp(name, "scan-path")) {
+   char *expanded = expand_macros(value);
+
if (!ctx.cfg.nocache && ctx.cfg.cache_size)
-   process_cached_repolist(expand_macros(value));
+   process_cached_repolist(expanded);
else if (ctx.cfg.project_list)
-   scan_projects(expand_macros(value),
+   scan_projects(expanded,
  ctx.cfg.project_list, repo_config);
else
-   scan_tree(expand_macros(value), repo_config);
-   else if (!strcmp(name, "scan-hidden-path"))
+   scan_tree(expanded, repo_config);
+
+   free(expanded);
+   } else if (!strcmp(name, "scan-hidden-path"))
ctx.cfg.scan_hidden_path = atoi(value);
else if (!strcmp(name, "section-from-path"))
ctx.cfg.section_from_path = atoi(value);
@@ -287,8 +291,12 @@ static void config_cb(const char *name, const char *value)
ctx.cfg.branch_sort = 0;
} else if (skip_prefix(name, "mimetype.", ))
add_mimetype(arg, value);
-   else if (!strcmp(name, "include"))
-   parse_configfile(expand_macros(value), config_cb);
+   else if (!strcmp(name, "include")) {
+   char *expanded = expand_macros(value);
+
+   parse_configfile(expanded, config_cb);
+   free(expanded);
+   }
 }
 
 static void querystring_cb(const char *name, const char *value)
@@ -1041,6 +1049,7 @@ static int calc_ttl(void)
 int cmd_main(int argc, const char **argv)
 {
const char *path;
+   char *expanded;
int err, ttl;
 
cgit_init_filters();
@@ -1052,7 +1061,11 @@ int cmd_main(int argc, const char **argv)
cgit_repolist.repos = NULL;
 
cgit_parse_args(argc, argv);
-   parse_configfile(expand_macros(ctx.env.cgit_config), config_cb);
+
+   expanded = expand_macros(ctx.env.cgit_config);
+   parse_configfile(expanded, config_cb);
+   free(expanded);
+
ctx.repo = NULL;
http_parse_querystring(ctx.qry.raw, querystring_cb);
 
diff --git a/shared.c b/shared.c
index 21ac8f4..32d0f46 100644
--- a/shared.c
+++ b/shared.c
@@ -489,8 +489,7 @@ static char *expand_macro(char *name, int maxlength)
 
 /* Replace all tokens prefixed by '$' in the specified text with the
  * value of the named environment variable.
- * NB: the return value is a static buffer, i.e. it must be strdup'd
- * by the caller.
+ * NB: the return value is allocated, it must be freed by the caller.
  */
 char *expand_macros(const char *txt)
 {
@@ -530,7 +529,7 @@ char *expand_macros(const char *txt)
p = expand_macro(start, len);
*p = '\0';
}
-   return result;
+   return xstrdup(result);
 }
 
 char *get_mimetype_for_filename(const char *filename)
diff --git a/ui-shared.c b/ui-shared.c
index 9d8f66b..c48f422 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -838,9 +838,12 @@ static void add_clone_urls(void (*fn)(const char *), char 
*txt, char *suffix)
 
 void cgit_add_clone_urls(void (*fn)(const char *))
 {
-   if (ctx.repo->clone_url)
-   add_clone_urls(fn, expand_macros(ctx.repo->clone_url), NULL);
-   else if (ctx.cfg.clone_prefix)
+   if (ctx.repo->clone_url) {
+   char *expanded = expand_macros(ctx.repo->clone_url);