From: jamie.couture at gmail.com (Jamie Couture)
Subject: [PATCH] ui-tree.c: check source filter if set globally
Date: Wed, 30 Jul 2014 17:06:23 -0400 [thread overview]
Message-ID: <CAH-DoZ6-cH2WuH3kBeQK501P-tPU9f_4AcnOveUfZ8xAQs=uAw@mail.gmail.com> (raw)
In-Reply-To: <20140730204717.GA7022@neptune>
Actually I have another idea. Why not defer scan path until the rest of
the config file is processed? Thus cgit_add_repo() can do its setup
properly. I'll give it another shot in a bit.
couture
On Jul 30, 2014 4:47 PM, "Jamie Couture" <jamie.couture at gmail.com> wrote:
> On Wed, Jul 30, 2014 at 08:23:21PM +0100, John Keeping wrote:
> > On Wed, Jul 30, 2014 at 02:53:21PM -0400, Jamie Couture wrote:
> > > When parsing cgitrc users that place 'source-filter' setting after
> > > 'scan-path' find their source filter is never run.
> > >
> > > scan-path=/home/git/repositories
> > > source-filter=/usr/local/lib/cgit/filters/syntax-highlighting.sh
> > >
> > > ui-tree.c will print out our repository objects, but will only consider
> > > ctx.repo->source_filter struct and not checking ctx.cfg.source_filter
> > >
> > > The value would have been set in shared.c, via cgit_add_repo() but this
> > > isn't the case when using scan-path, because we have not yet processed
> > > the source-filter line in our cgitrc, and thus the source_filter struct
> > > will not be initialized.
> > >
> > > Checking the global configuration in ui-tree.c is necessary to avoid an
> > > issue where users declare the source filter after scan-path.
> >
> > I think this is OK because we only fall back if repo->source_filter is
> > unset and there is no way to unset the source filter for a repository
> > AFAICT. But if we do this for source-filter, why not also about-filter,
> > email-filter and commit-filter?
> >
>
> Yes, I'm unfairly catering to one setting and not the remaining
> filters: most people have this issue with source-filter. I've seen
> this question asked enough times I thought it was time something was
> done to avoid the subtly.
>
> > The documentation already makes it clear that settings should be
> > configured before "scan-path", and scan-path isn't special here you
> > could equally well demonstrate the same effect with:
> >
>
> Yes, yet people still make this error. We can try harder to make it
> less error prone. This patch doesn't try harder for the other
> options, only to the one solution so a more thoughtful patch is
> needed.
>
> > I'm not convinced that this change makes things less confusing, it just
> > means that "everything except source-filter must be configured before
> > adding repositories", which seems more confusing.
> >
>
> Well, 'everything except source-filter' isn't really the case.
>
> To be clear, the behaviour is that we setup the repo as you pointed
> out below, but because we didn't process the appropriate filter
> lines yet we did not instantiate the stuct. A full pass of the
> configuration has not happened. We would have to make two passes to
> know about all filter options, and then copy those settings during
> scan-path if that is also defined.
>
> I think better wording would be 'everything should to be declared
> before scan-path and scan-tree', which we know well enough, but
> users still run into that issue.
>
> Users should not have to worry about where they define things.
>
> > The full list of captured settings is:
> >
> > ret->section = ctx.cfg.section;
> > ret->snapshots = ctx.cfg.snapshots;
> > ret->enable_commit_graph = ctx.cfg.enable_commit_graph;
> > ret->enable_log_filecount = ctx.cfg.enable_log_filecount;
> > ret->enable_log_linecount = ctx.cfg.enable_log_linecount;
> > ret->enable_remote_branches = ctx.cfg.enable_remote_branches;
> > ret->enable_subject_links = ctx.cfg.enable_subject_links;
> > ret->max_stats = ctx.cfg.max_stats;
> > ret->branch_sort = ctx.cfg.branch_sort;
> > ret->commit_sort = ctx.cfg.commit_sort;
> > ret->module_link = ctx.cfg.module_link;
> > ret->readme = ctx.cfg.readme;
> > ret->about_filter = ctx.cfg.about_filter;
> > ret->commit_filter = ctx.cfg.commit_filter;
> > ret->source_filter = ctx.cfg.source_filter;
> > ret->email_filter = ctx.cfg.email_filter;
> > ret->clone_url = ctx.cfg.clone_url;
> >
> > And I don't think we want to change all of these to allow the config
> > file to be out-of-order.
> >
>
> Maybe we do if users are having issues?
>
> > Perhaps we would be better off making the documentation clearer rather
> > than trying to fix some particular cases?
> >
>
> I'm okay with this change, as long as it avoids problems for the
> user. I don't think it introdces any sort of unwanted or unexpected
> behaviour. However, it should try to be more complete with the
> remaining filter options.
>
> I'm fine with this being turfed, but we do get a lot of questions
> about scan-path problems.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140730/c7179c32/attachment-0001.html>
next prev parent reply other threads:[~2014-07-30 21:06 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-28 21:14 source-filter isn't getting applied njn2118
2014-07-28 22:38 ` john
[not found] ` <CA+TJm37SY1PwBuYgTpn=bQz0D7LcGdwwSZb=38ZbHK_DX1j5JA@mail.gmail.com>
2014-07-29 3:59 ` Fwd: " njn2118
2014-07-29 7:37 ` john
2014-07-30 4:17 ` njn2118
2014-07-30 4:21 ` njn2118
2014-07-30 7:57 ` john
2014-07-30 18:53 ` [PATCH] ui-tree.c: check source filter if set globally jamie.couture
2014-07-30 19:23 ` john
2014-07-30 20:47 ` jamie.couture
2014-07-30 21:06 ` jamie.couture [this message]
2014-07-30 21:14 ` john
2014-07-31 1:05 ` jamie.couture
2014-07-31 1:08 ` jamie.couture
2014-08-01 15:38 ` Jason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAH-DoZ6-cH2WuH3kBeQK501P-tPU9f_4AcnOveUfZ8xAQs=uAw@mail.gmail.com' \
--to=cgit@lists.zx2c4.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).