On Tue, Mar 25, 2008 at 03:09:20AM -0400, Daniel Danger Bentley wrote:
> Hmm, I understand the concern about using the empty string to mean
> "internal diff".
>
> Here's my concerns with the other UI of having a
> --force-internal-diff. These may be handled by svn's options
> parser library, and thus response of the form "go read this code"
> is totally understandable, but if you could tell me what code to
> read I'd be grateful.
See http://subversion.tigris.org/servlets/ReadMsg?listName=svn&msgNo=32343
for a commit that added a new diff option to the svnlook command.
The process should be very similar for the svn command.
I'm referring you to this diff and not the code itself because:
1) I made that patch, i.e. I can answer questions about it if you have any.
2) You need to understand what needs to be changed, and using both
a diff like that *and* the code will probably get you there quicker.
> What happens if I set --force-internal-diff while also setting
> --diff-cmd="/path/to/my/diff"?
UNIX tools usually implement it so that the flag specified last wins.
I think is perfectly acceptable for us, too.
Or you could also have svn error out if both options are specified,
and print an appropriate message.
Either behaviour is fine.
> What happens if I set force-internal-diff in my config, but
> diff-cmd on the command line?
I suggest we make it so that you can't set --force-internal-diff
in the config file at all.
See the first post of issue #3071 again. It's about *overriding*
the config file, nothing else. As such, the force flag itself
does not need to be in the config file, and hence is unambiguous.
In fact, the reporter himself suggested a flag. He said:
"Maybe something like:
svn diff --no-diff-cmd"
His idea seems oddly familiar to me :)
> It seems to me what we are discussing is one option (which diff
> command should be used), and so it makes sense to specify that
> option in one place.
To me it seems that we are discussing a new option that allows
overriding a setting in the config file with a known default value.
The aim is to get rid of the pain of having to edit the config
file if usually a diff-cmd specified in the config file should
be used, but the internal diff should be used in exceptional cases.
I don't think that's the same as "which diff command should be used".
That question is already covered by --diff-cmd. And if that flag is
not specified at all, then the internal implementation is used,
which is a reasonable example of default behaviour.
> Having "" mean default matches informal user expectation, I
> posit.
No way :)
I posit you like it this way because you've already implemented it,
and because you didn't have to do lots of changes to implement it the
way you did, and because it seems easier to you than adding a new
option :P
> Uninitialized strings in languages that have a default are
> "", and supply the value most like null to specify that you meant
> no value seems consistent.
Many people, even Subversion users, are not programmers, and won't
understand you when you say "But hey, look, it makes perfect sense
this way, the default value is NULL! That is consistent!".
Instead they will probably sigh and think "Oh my god this is just
like when I started using Linux for the first time ever".
Attributing special meaning to an empty string in a user interface
is beyond confusing, it's madness. Subversion has enough UI issues
as it is. (As an aside I myself still have to clean up UI issues on
the tree-conflicts branch, so I'm not innocent either in this respect.
And these fixes may possibly affect trunk, too, but that is a
different topic I'll bring up another day in another thread.).
> I'd love to do what will make this work, but I think that keeping
> it as one option (especially when it's documented that "" is a
> special value instead of a non-sensical one to specify) is going
> to be simplest when integrated over many variables.
I think we should not extend this discussion to other variables
just yet. We should focus on the diff-cmd discussion first, then see
if we can apply the result of that to diff3-cmd as well (since it's
obviously similar), and then see what else can be done.
> The
> discussion on the bug (
> http://subversion.tigris.org/issues/show_bug.cgi?id=3071 ) settled
> with jph saying this wasn't a perfect syntax, but acceptable.
Of course he won't object, because your patch solves his problem,
and he knows about the quirk so he won't run into it without
knowing what hit him.
Other users who read the help text the first time ("empty string...
wait... whaaat?") or accidentally specify an empty string for some
reason and then complain about "unexpected behaviour" might have a
different opinion than jph.
On the other hand, a new override flag is a straightforward UI
design that nobody will have much trouble grasping.
And in other UIs (think GUIs like TortoiseSVN, Subclipse, etc.)
it perfectly matches to a small check box with the text
"ignore diff-cmd option in config file" next to it.
> It's not perfect, but I think it might be the least flawed out
> there.
You are really, really close to making your patch perfect.
Adding an option is not hard! Go for it :)
--
Stefan Sperling <stsp_at_elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
- application/pgp-signature attachment: stored
Received on 2008-03-25 11:53:22 CET