Commit _subject_ filter, everywhere

Commit _subject_ filter, everywhere

[@ 2017-11-29 21:10 UTC (permalink / raw)]

Hi Jason, all,

I got this idea to generate a link to the CI server job in front of
each commit message subject. Think small green/red boxes that are
clickable. I figure that getting the build status directly in cgit is
much more efficient than navigating the CI server itself, which
typically requires many clicks to find the build that comes out of a
given commit.

I had high hopes for the the commit-filter= setting, but found that

* commit-filter= is only run when showing a full commit, not other
places. I.e. it is applied when showing the "commit" tab, but not on
"summary", "logs" or "refs".
* commit-filter= is run twice on a commit message. Once for the
subject and once for the body. I expected only once. (Is this a bug?)

Any thoughts on if/how this use case can be supported? A separate
filter? Or simply run commit-filter= everywhere (breaking change)?

Best regards,
Bj?rn Forsman (happy cgit user)

Commit _subject_ filter, everywhere

[john]

On Wed, Nov 29, 2017 at 10:10:00PM +0100, Bjjjrn Forsman wrote:
> I got this idea to generate a link to the CI server job in front of
> each commit message subject. Think small green/red boxes that are
> clickable. I figure that getting the build status directly in cgit is
> much more efficient than navigating the CI server itself, which
> typically requires many clicks to find the build that comes out of a
> given commit.
> 
> I had high hopes for the the commit-filter= setting, but found that
> 
> * commit-filter= is only run when showing a full commit, not other
> places. I.e. it is applied when showing the "commit" tab, but not on
> "summary", "logs" or "refs".
> * commit-filter= is run twice on a commit message. Once for the
> subject and once for the body. I expected only once. (Is this a bug?)
> 
> Any thoughts on if/how this use case can be supported? A separate
> filter? Or simply run commit-filter= everywhere (breaking change)?

I was going to suggest introducing a new subject-filter which is used to
filter the commit subject wherever it is displayed, but there is a
subtlety which makes this more complicated in the summary, log and refs
pages than it is in the commit page.

When we are showing a single-line summary of a commit, the subject is a
hyperlink so adding new content outside the existing subject is not
possible with the obvious filter implementation.

One option would be to push the <a></a> tag generation down into the
filter, but I'd rather avoid that if possible.

What do you think about adding a "filter" which is called before the
commit subject is printed and allows outputting additional HTML content?
("filter" is in quotes since there will not be any content copied
through the program.)

Commit _subject_ filter, everywhere

[@ 2017-12-02 12:33 UTC (permalink / raw)]

Hi John,

On 2 December 2017 at 12:31, John Keeping <john at keeping.me.uk> wrote:
> I was going to suggest introducing a new subject-filter which is used to
> filter the commit subject wherever it is displayed, but there is a
> subtlety which makes this more complicated in the summary, log and refs
> pages than it is in the commit page.
>
> When we are showing a single-line summary of a commit, the subject is a
> hyperlink so adding new content outside the existing subject is not
> possible with the obvious filter implementation.
>
> One option would be to push the <a></a> tag generation down into the
> filter, but I'd rather avoid that if possible.

Aha, I see.

> What do you think about adding a "filter" which is called before the
> commit subject is printed and allows outputting additional HTML content?
> ("filter" is in quotes since there will not be any content copied
> through the program.)

Sounds good to me. It covers my use case.

Perhaps this new "filter" should not have filter in its name.
Suggestion: commit-prefix=.

Oh, and this new command must somehow receive the commit id from cgit.
New environment variable?

Best regards,
Bj?rn Forsman

Commit _subject_ filter, everywhere

[john]

On Sat, Dec 02, 2017 at 01:33:28PM +0100, Bjjjrn Forsman wrote:
> On 2 December 2017 at 12:31, John Keeping <john at keeping.me.uk> wrote:
> > I was going to suggest introducing a new subject-filter which is used to
> > filter the commit subject wherever it is displayed, but there is a
> > subtlety which makes this more complicated in the summary, log and refs
> > pages than it is in the commit page.
> >
> > When we are showing a single-line summary of a commit, the subject is a
> > hyperlink so adding new content outside the existing subject is not
> > possible with the obvious filter implementation.
> >
> > One option would be to push the <a></a> tag generation down into the
> > filter, but I'd rather avoid that if possible.
> 
> Aha, I see.

After I wrote that I realised that for the existing commit-filter we
escape the content before writing to the filter.  So it might be
reasonable to pass the whole <a></a> tag through the filter.

That would make it easy for the filter to put extra content either
before or after the commit subject.  It also makes the filter
integration much simpler, for example in ui-log.c:

-- >8 --
diff --git a/ui-log.c b/ui-log.c
index 2d2bb31..f2a7852 100644
--- a/ui-log.c
+++ b/ui-log.c
@@ -234,8 +234,11 @@ static void print_commit(struct commit *commit, struct rev_info *revs)
 			strcpy(info->subject + i, wrap_symbol);
 		}
 	}
+	cgit_open_filter(ctx.repo->subject_filter,
+			 oid_to_hex(&commit->object.oid));
 	cgit_commit_link(info->subject, NULL, NULL, ctx.qry.head,
 			 oid_to_hex(&commit->object.oid), ctx.qry.vpath);
+	cgit_close_filter(ctx.repo->subject_filter);
 	show_commit_decorations(commit);
 	html("</td><td>");
 	cgit_open_filter(ctx.repo->email_filter, info->author_email, "log");
-- 8< --

> > What do you think about adding a "filter" which is called before the
> > commit subject is printed and allows outputting additional HTML content?
> > ("filter" is in quotes since there will not be any content copied
> > through the program.)
> 
> Sounds good to me. It covers my use case.
> 
> Perhaps this new "filter" should not have filter in its name.
> Suggestion: commit-prefix=.
> 
> Oh, and this new command must somehow receive the commit id from cgit.
> New environment variable?

We normally use arguments for this, which is all handled via the
cgit_open_filter() function.

I expect Lua filters may be preferred for this use case to avoid forking
50 child processes when displaying the log view.

Commit _subject_ filter, everywhere

[@ 2017-12-02 14:44 UTC (permalink / raw)]

Hi John,

On 2 December 2017 at 13:49, John Keeping <john at keeping.me.uk> wrote:
> After I wrote that I realised that for the existing commit-filter we
> escape the content before writing to the filter.  So it might be
> reasonable to pass the whole <a></a> tag through the filter.
>
> That would make it easy for the filter to put extra content either
> before or after the commit subject.  It also makes the filter
> integration much simpler, for example in ui-log.c:
>
> -- >8 --
> diff --git a/ui-log.c b/ui-log.c
> index 2d2bb31..f2a7852 100644
> --- a/ui-log.c
> +++ b/ui-log.c
> @@ -234,8 +234,11 @@ static void print_commit(struct commit *commit, struct rev_info *revs)
>                         strcpy(info->subject + i, wrap_symbol);
>                 }
>         }
> +       cgit_open_filter(ctx.repo->subject_filter,
> +                        oid_to_hex(&commit->object.oid));
>         cgit_commit_link(info->subject, NULL, NULL, ctx.qry.head,
>                          oid_to_hex(&commit->object.oid), ctx.qry.vpath);
> +       cgit_close_filter(ctx.repo->subject_filter);
>         show_commit_decorations(commit);
>         html("</td><td>");
>         cgit_open_filter(ctx.repo->email_filter, info->author_email, "log");
> -- 8< --

I applied that patch, with s/subject_filter/commit_filter/, and
updated my commit-filter to output a small SVG (borrowed from
https://shields.io/) in front of the commit message.

Here is the result:

https://bforsman.name/cgit/nixos-config/log/

I think it's looking good!

On small annoyance though is that the CI status element, when loaded,
causes the commit messages to move sideways. Ideally, the CI status
(or whatever the user chose to attach to the commit) would not cause
the layout to move (unless it's very big). I guess that's a
conflicting goal between having a "commit prefix" and a dedicated
column for this extra info. Another alternative is _appending_ the
element, which a filter program has the ability to do. Then the layout
would be stable, but the CI status elements would not align nicely in
a column anymore. Hmm... :-)

> We normally use arguments for this, which is all handled via the
> cgit_open_filter() function.
>
> I expect Lua filters may be preferred for this use case to avoid forking
> 50 child processes when displaying the log view.

Good point.

Best regards,
Bj?rn Forsman

Commit _subject_ filter, everywhere

[john]

On Sat, Dec 02, 2017 at 03:44:15PM +0100, Bjjjrn Forsman wrote:
> On 2 December 2017 at 13:49, John Keeping <john at keeping.me.uk> wrote:
> > After I wrote that I realised that for the existing commit-filter we
> > escape the content before writing to the filter.  So it might be
> > reasonable to pass the whole <a></a> tag through the filter.
> >
> > That would make it easy for the filter to put extra content either
> > before or after the commit subject.  It also makes the filter
> > integration much simpler, for example in ui-log.c:
> >
> > -- >8 --
> > diff --git a/ui-log.c b/ui-log.c
> > index 2d2bb31..f2a7852 100644
> > --- a/ui-log.c
> > +++ b/ui-log.c
> > @@ -234,8 +234,11 @@ static void print_commit(struct commit *commit, struct rev_info *revs)
> >                         strcpy(info->subject + i, wrap_symbol);
> >                 }
> >         }
> > +       cgit_open_filter(ctx.repo->subject_filter,
> > +                        oid_to_hex(&commit->object.oid));
> >         cgit_commit_link(info->subject, NULL, NULL, ctx.qry.head,
> >                          oid_to_hex(&commit->object.oid), ctx.qry.vpath);
> > +       cgit_close_filter(ctx.repo->subject_filter);
> >         show_commit_decorations(commit);
> >         html("</td><td>");
> >         cgit_open_filter(ctx.repo->email_filter, info->author_email, "log");
> > -- 8< --
> 
> I applied that patch, with s/subject_filter/commit_filter/, and
> updated my commit-filter to output a small SVG (borrowed from
> https://shields.io/) in front of the commit message.
> 
> Here is the result:
> 
> https://bforsman.name/cgit/nixos-config/log/
> 
> I think it's looking good!
> 
> On small annoyance though is that the CI status element, when loaded,
> causes the commit messages to move sideways. Ideally, the CI status
> (or whatever the user chose to attach to the commit) would not cause
> the layout to move (unless it's very big). I guess that's a
> conflicting goal between having a "commit prefix" and a dedicated
> column for this extra info. Another alternative is _appending_ the
> element, which a filter program has the ability to do. Then the layout
> would be stable, but the CI status elements would not align nicely in
> a column anymore. Hmm... :-)

I wonder if it would look better with a smaller symbol, like the
Libravatar icons for the author names here:

	https://git.zx2c4.com/cgit/log/

Maybe something like the small tick/cross icons GitHub and GitLab use
for build status, for example:

	https://github.com/rust-lang/rust/branches/all

	https://gitlab.com/gitlab-org/gitlab-ce/commits/master

Commit _subject_ filter, everywhere

[@ 2017-12-02 17:14 UTC (permalink / raw)]

Hi John,

On 2 December 2017 at 18:10, John Keeping <john at keeping.me.uk> wrote:
> On Sat, Dec 02, 2017 at 03:44:15PM +0100, Bjjjrn Forsman wrote:
>> Here is the result:
>>
>> https://bforsman.name/cgit/nixos-config/log/
>>
>> I think it's looking good!
>
> I wonder if it would look better with a smaller symbol, like the
> Libravatar icons for the author names here:
>
>         https://git.zx2c4.com/cgit/log/
>
> Maybe something like the small tick/cross icons GitHub and GitLab use
> for build status, for example:
>
>         https://github.com/rust-lang/rust/branches/all
>
>         https://gitlab.com/gitlab-org/gitlab-ce/commits/master

Indeed, those look better.

(I didn't plan to use that big/long SVG in the end, it was just the
first I found.)

Best regards,
Bj?rn Forsman

Commit _subject_ filter, everywhere

[@ 2017-12-02 19:00 UTC (permalink / raw)]

On 2 December 2017 at 18:14, Bj?rn Forsman <bjorn.forsman at gmail.com> wrote:
> Hi John,
>
> On 2 December 2017 at 18:10, John Keeping <john at keeping.me.uk> wrote:
>> On Sat, Dec 02, 2017 at 03:44:15PM +0100, Bjjjrn Forsman wrote:
>>> Here is the result:
>>>
>>> https://bforsman.name/cgit/nixos-config/log/
>>>
>>> I think it's looking good!
>>
>> I wonder if it would look better with a smaller symbol, like the
>> Libravatar icons for the author names here:
>>
>>         https://git.zx2c4.com/cgit/log/
>>
>> Maybe something like the small tick/cross icons GitHub and GitLab use
>> for build status, for example:
>>
>>         https://github.com/rust-lang/rust/branches/all
>>
>>         https://gitlab.com/gitlab-org/gitlab-ce/commits/master
>
> Indeed, those look better.
>
> (I didn't plan to use that big/long SVG in the end, it was just the
> first I found.)

I did two changes:

1. Borrow SVG from gitlab (thanks for the hint!). Smaller, more
suitable icon. Also, it's a good thing to have a shape that indicates
pass/fail, instead of relying only on color.
2. Specify a fixed size for the SVG in the commit-filter. This
prevents a the page layout from changing when the SVG is finally
loaded.

https://bforsman.name/cgit/nixos-config/log/

The indicator is still listed in the "commit" column, but as it's much
smaller now, I feel that's fine.

Best regards,
Bj?rn Forsman