From mboxrd@z Thu Jan  1 00:00:00 1970
From: jamie.couture at gmail.com (Jamie Couture)
Date: Wed, 30 Jul 2014 16:47:17 -0400
Subject: [PATCH] ui-tree.c: check source filter if set globally
In-Reply-To: <20140730192321.GJ26927@serenity.lan>
References: <87a97t89to.fsf@columbia.edu>
 <1406746401-24187-1-git-send-email-jamie.couture@gmail.com>
 <20140730192321.GJ26927@serenity.lan>
Message-ID: <20140730204717.GA7022@neptune>

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.