Many thanks for the inspiring feedback!
Julian Foad wrote:
>> * This patch breaks the override --internal-diff for now, because
>> this part has to be revised anyway when the invoke-diff3-cmd part
>> gets added.
> OK, we'll have to decide what should override what. I have not formed
> any opinion on that yet.
Personally I feel that if you define an option on the command line, it
should always override any choice made in the config file for
convenience and flexibility.
Also, I think the --internal-diff-or-merge should turn off whatever
diff/merge arrangements the config file specifies(let's leave the old
command --internal-diff untouched), especially if we add the following
Paul Watson mentioned on the user list:
that he would like to use mime-types to define what diff program to
use for certain files. (note M. Pilatos' RFC in the thread)
Should the config file store pair values, mime-type and the
invoke-diff(n)-cmd to be used?
New command line design example:
invoke-diff-cmd="---MIME-TYPE=custom_mime_type diffProg ---f1 ....
or, if you just want the diff program you select to be applied to any
mime-type you define:
invoke-diff-cmd="diff ---f1 ..."
The corresponding config file entries would look like:
INVOKE_DIFF_MIME_TYPE_1_CMD=myDiffProg ---f1 ...
INVOKE_DIFF_MIME_TYPE_2_CMD=myFavoriteDiff ---f1 ...
INVOKE_MERGE_MIME_TYPE_1_CMD=myMergeProg ---f1 ...
I wasn't sure if this kind of dev talk belongs on the user list, so I
CC'ed Paul Watson for now. Let me know if this should be posted to
the existing thread in users as well.
> I think it would be desirable to be able to create arguments
> that contain the substituted file name or label as well as some other
> text, such as "--file1=foo" or "+foo" (which is a form accepted by
> kdiff3). But although forms such as these are accepted by some of the
> diff programs I tried, it wasn't necessary for any of them. I didn't
> come across any other problems.
Just to clarify, would you like "+---f1" expanding to
+diff_file_name or ---f1+foo to diff_file_name+foo respectively?
> Yes. Also, please document the return value (NULL-terminated argv
I thought for quite a while that apr_array_header_t should be used
instead of the argv array in svn_io_create_custom_diff_cmd(..) but
then it occurred to me that this is API level and that would force
folks to use APR to access this.
Question 1: does/should svn_io_create_custom_diff_cmd live in the API,
i.e. svn_io.c? It's where it is currently because of scope issues,
but not because I considered it API material, and I think that API
users probably should not have access to this --- I may be wrong, but
how and why would people want to use this function, if they would?
Question 2: Given the additional issue of the diff_cmd_baton data
which is passed piecemeal at times because (I guess) of scope issues,
is there a case for a private diff.h file that covers the diff
M. Compilato wrote:
> I like the idea! I note that you don't draw attention to any existing
> issues, but you might want to review issue #2044 and see if your work
> resolves the request there:
Yes, the new command fixes this.
> Also, a minor design nit (sorry, no code review): The "---f1"
> construct is something I've never seen before.
That's why I picked it --- I checked extensively, and no-one uses a
triple dash, so it does exactly what we hope it will: never interfere
with anything that people might do. Also, I think it looks quite
'unixy' and it's easy to read. I expect fewer problems on windows.
Received on 2013-04-23 12:34:26 CEST