Re: [PATCH] optionally disable normalization of working copy files in diff invocations

> Matthias Gerstner wrote:
>> attached is a patch against the current subversion trunk state (revision
>> 1611327) that fixes a problem when using external diff programs that's
>> also been described years ago here:
>>
>> http://svn.haxx.se/users/archive-2008-10/0664.shtml
>>
>> ----
>> When passing the new diff command line option --no-normalization then
>> svn diff won't create and pass on a temporary, normalized version of a
>> file for local working copy files.
>>
>> This normalization is the default behaviour when svn diff encounters
>> files that have the svn:keywords or svn:eol-style properties set, such
>> that the base version and the working copy version of the file have the
>> same format.
>>
>> This makes it impossible to edit diffed files when using an external
>> --diff-cmd that supports editing, because the file passed to the
>> external tool is a temporary file that will be deleted afterwards,
>> instead of the original working copy file.
>>
>> When passing --no-normalization the original file is passed to the diff
>> tool instead. External tools that can ignore whitespace differences
>> (present due to svn:eol-style) can still display decent diffs and the
>> benefit of editing the diffed files in place is helpful.
>> ----

Hi Matthias, thanks for the patch. I understand why this is useful, and basically I agree.

The main point I want to make is that the user interface you chose is an example of the "X Y problem" <http://mywiki.wooledge.org/XyProblem>. What does the user really want? To have their actual working file loaded in the external diff program. What does the user tell Subversion?

  svn diff --don't-normalize-the-content
    # in fact I don't care about normalization but I learnt that
    # this option makes Subversion do what I really want

A better interface, the user would tell Subversion what they really want:

  svn diff --load-my-actual-file-not-a-copy-of-it
    # and I learnt that I won't have normalization in this case

See the difference?

(It's possible Subversion may, today or tomorrow, pass a temporary file for reasons other than normalizing the content. It's also possible that Subversion may, tomorrow, be able to normalize the keyword expansions in the working file, run the diff, and then put them back again; that's not necessarily a good idea for various reasons, but in principle things like that are possible. So disabling normalization does not logically imply getting the original file, and vice-versa.)

I'd like the interface (option name, help text, C symbols, API docs) to say what it means.

Stefan Sperling wrote:
> I'd hope we could address this without public API changes and
> without adding yet another command line option.
>
> How about we make this the default if a third party diff tool is used?
> This way, third party diff tools will always display differences in
> keywords, and possibly EOLs. I don't think this is much of a problem,
> except for cases where all lines are considered different because of
> end-of-line differences. I would hope most 3rd party tools have
> options to control this behaviour. But perhaps my idea is controversial,
> in which case we won't get by without adding a new option for this.

I agree that yet another command-line option is somewhat unwelcome, but, as you know, we're generally careful not to change the user interface unnecessarily, because we don't want to break people's existing scripts and work flows. This does seem to me like a case where that would potentially cause unnecessary disruption to people who are already using an external diff tool and have adapted to the way it currently behaves. For example, I once wrote an external diff tool plug-in script which took Subversion's diff parameters and then found and opened the actual WC working file instead, for this very reason.

It would be better if we could make it easy to get the new behaviour without forcing users of the old behaviour to change their diff set-ups.

Since you point out that this is only relevant when using a third party diff tool, it would be a good idea to make it a configuration file option. It would be silly to be able to configure an external diff tool in the config file (as we already can) but then have to add the '--whatever' option manually on every 'svn diff' command.

Then the question is, is it useful to have a dedicated command-line option as well? I think perhaps not. (There's always the ability to specify a config option on the command line using "--config-option=...".)

> In terms of coding style, I'd use a boolean that switches normal
> form on, rather than off. I find it easier to keep track of this way.
>
> The interaction between use_text_base and your new no_normalization
> flag isn't made clear. You can't have both!
>
> Also, your change only addresses BASE->WORKING diffs, as far as I can tell.
> What about REPOS->WORKING or WORKING->REPOS diffs?

The rest of Stefan's comments, above, I agree with (and I haven't looked further into those or into the patch itself).

- Julian
Received on 2014-07-17 15:47:43 CEST