<p dir="ltr">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.<br></p>
<p dir="ltr">couture</p>
<div class="gmail_quote">On Jul 30, 2014 4:47 PM, "Jamie Couture" <<a href="mailto:jamie.couture@gmail.com">jamie.couture@gmail.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Wed, Jul 30, 2014 at 08:23:21PM +0100, John Keeping wrote:<br>
> On Wed, Jul 30, 2014 at 02:53:21PM -0400, Jamie Couture wrote:<br>
> > When parsing cgitrc users that place 'source-filter' setting after<br>
> > 'scan-path' find their source filter is never run.<br>
> ><br>
> > scan-path=/home/git/repositories<br>
> > source-filter=/usr/local/lib/cgit/filters/syntax-highlighting.sh<br>
> ><br>
> > ui-tree.c will print out our repository objects, but will only consider<br>
> > ctx.repo->source_filter struct and not checking ctx.cfg.source_filter<br>
> ><br>
> > The value would have been set in shared.c, via cgit_add_repo() but this<br>
> > isn't the case when using scan-path, because we have not yet processed<br>
> > the source-filter line in our cgitrc, and thus the source_filter struct<br>
> > will not be initialized.<br>
> ><br>
> > Checking the global configuration in ui-tree.c is necessary to avoid an<br>
> > issue where users declare the source filter after scan-path.<br>
><br>
> I think this is OK because we only fall back if repo->source_filter is<br>
> unset and there is no way to unset the source filter for a repository<br>
> AFAICT. But if we do this for source-filter, why not also about-filter,<br>
> email-filter and commit-filter?<br>
><br>
<br>
Yes, I'm unfairly catering to one setting and not the remaining<br>
filters: most people have this issue with source-filter. I've seen<br>
this question asked enough times I thought it was time something was<br>
done to avoid the subtly.<br>
<br>
> The documentation already makes it clear that settings should be<br>
> configured before "scan-path", and scan-path isn't special here you<br>
> could equally well demonstrate the same effect with:<br>
><br>
<br>
Yes, yet people still make this error. We can try harder to make it<br>
less error prone. This patch doesn't try harder for the other<br>
options, only to the one solution so a more thoughtful patch is<br>
needed.<br>
<br>
> I'm not convinced that this change makes things less confusing, it just<br>
> means that "everything except source-filter must be configured before<br>
> adding repositories", which seems more confusing.<br>
><br>
<br>
Well, 'everything except source-filter' isn't really the case.<br>
<br>
To be clear, the behaviour is that we setup the repo as you pointed<br>
out below, but because we didn't process the appropriate filter<br>
lines yet we did not instantiate the stuct. A full pass of the<br>
configuration has not happened. We would have to make two passes to<br>
know about all filter options, and then copy those settings during<br>
scan-path if that is also defined.<br>
<br>
I think better wording would be 'everything should to be declared<br>
before scan-path and scan-tree', which we know well enough, but<br>
users still run into that issue.<br>
<br>
Users should not have to worry about where they define things.<br>
<br>
> The full list of captured settings is:<br>
><br>
> ret->section = ctx.cfg.section;<br>
> ret->snapshots = ctx.cfg.snapshots;<br>
> ret->enable_commit_graph = ctx.cfg.enable_commit_graph;<br>
> ret->enable_log_filecount = ctx.cfg.enable_log_filecount;<br>
> ret->enable_log_linecount = ctx.cfg.enable_log_linecount;<br>
> ret->enable_remote_branches = ctx.cfg.enable_remote_branches;<br>
> ret->enable_subject_links = ctx.cfg.enable_subject_links;<br>
> ret->max_stats = ctx.cfg.max_stats;<br>
> ret->branch_sort = ctx.cfg.branch_sort;<br>
> ret->commit_sort = ctx.cfg.commit_sort;<br>
> ret->module_link = ctx.cfg.module_link;<br>
> ret->readme = ctx.cfg.readme;<br>
> ret->about_filter = ctx.cfg.about_filter;<br>
> ret->commit_filter = ctx.cfg.commit_filter;<br>
> ret->source_filter = ctx.cfg.source_filter;<br>
> ret->email_filter = ctx.cfg.email_filter;<br>
> ret->clone_url = ctx.cfg.clone_url;<br>
><br>
> And I don't think we want to change all of these to allow the config<br>
> file to be out-of-order.<br>
><br>
<br>
Maybe we do if users are having issues?<br>
<br>
> Perhaps we would be better off making the documentation clearer rather<br>
> than trying to fix some particular cases?<br>
><br>
<br>
I'm okay with this change, as long as it avoids problems for the<br>
user. I don't think it introdces any sort of unwanted or unexpected<br>
behaviour. However, it should try to be more complete with the<br>
remaining filter options.<br>
<br>
I'm fine with this being turfed, but we do get a lot of questions<br>
about scan-path problems.<br>
</blockquote></div>