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