Hi Gabriela.
It looks
to me like the basic part of this configurable-diff work (without the
cfg-file feature) is
about done, at least to proof-of-concept level, and should soon be
ready to merge to trunk once we finally settle the syntax and parsing
rules. I have finally
got around to trying and reviewing it, and I'll post about that
separately. I think it would be great to get to that stage: getting it
in trunk implies it's all agreed, tested, and will be released and
supported. I suppose you need feedback from me and others in order to
progress in that direction. And so, meanwhile...
Gabriela Gibsonwrote on 2013-10-26:
> I added an internal mechanism to the --invoke-diff-cmd which allows the user to
> automatically or interactively select individual files and their
> respective diff cmds, configurable via an arbitrary config file.
This new config-file
feature. I think the basic idea is totally needed: to be able to
configure that different diff programs (and/or with different arguments)
should be invoked for different kinds of files. For example: use
oodiff[1] for Open Document Text docs, ImageMagick "compare"[2] for
images, and "kdiff3" for plain text files and everything else.
> There are now two optional, internal switches to the invoke-diff-cmd command:
>
> --svn_cfg-file and --svn-cfg-file-query.
>
> The usage in each case is:
>
> --invoke-diff-cmd='(--switch) (path/to/config/file) (default-diff-cmd)'
>
> Those two switches do the following:
>
> 1) --svn-cfg-file causes svn to check the given diff_cmds_config file
> and apply the rules therein automatically on a per-file basis.
>
> If a file is not specifically mentioned in the svn-cfg-file, then
> the given --invoke-diff-cmd is applied.
>
> 2) --svn-cfg-file-query works like --svn-cfg-file, but it will prompt
> the user for every file in the given config-file whether they want
> to use the instruction in the config file, the default
> invoke-diff-cmd or input something completely different.
>
> Note: --svn-cfg-file-query is not implemented yet.
As for the UI, where you specify a --svn-cfg-file switch nested within
the value of another switch, I can only say seems horribly contorted :-)
Maybe you did it that way because it meant less code churn or an easier
starting point for you? It seems to me that one would often want to
specify either a diff command or a diff configuration file but rarely
both at the same time, so a separate option would seem logical to me.
> The config file itself looks like so:
>
> # Note the test1 label
> subversion/svn/svn.c = diff -L "test1" %svn_old% %svn_new%
> # Testing with more than one word in the label
> subversion/libsvn_subr/io.c = diff -L "test 2" %svn_old% %svn_new%
Clearly the configuration file will need to be able to specify which files use which diff command in a way that's a bit more flexible than listing every file individually. At the very least a filename pattern with wildcards is needed (for precedent, see the auto-props configuration). More than that, I think it would be nice to be able to match on the value of svn:mime-type and/or other conditions to make it a bit more powerful than just filenames, but just filename matching would be powerful enough initially.
You mentioned to me that you have specific ideas about why you want a separate config file rather than embedding the configuration in the '~/.subversion/config' file like we do with autoprops configuration. Basically it's because diff preferences can vary per project, which I agree with. Could you elaborate a bit, here?
Thanks for the links, and for your detailed BRANCH-README file that makes it easy to understand everything going on here.
- Julian
[1] <http://craig.mcqueen.id.au/OpenOfficeGraphicalDiff.html>
[2] <http://www.imagemagick.org/Usage/compare/>
> The code itself has a few efficiency issues, but is sufficient for a
> proof-of-concept demonstration.
>
> The revision is located here:
>
> http://svn.apache.org/viewvc?view=revision&revision=r1536020
>
> I sent an old copy of the BRANCH-README, the up-to-date version that
> describes the new feature is here:
>
> http://svn.apache.org/viewvc?view=revision&revision=r1536037
>
> Thanks for looking!
>
> Gabriela
>
Received on 2013-10-30 18:25:29 CET