You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gabriela Gibson <ga...@gmail.com> on 2013/03/19 12:15:32 UTC
svn diff fix for bug 2044
Hi,
I've made some changes to meet some feature requests regarding svn diff.
Could you please take a look and let me know if I'm on the right track?
thanks,
Gabriela
[[[
Change "svn diff" to allow removal of "-u" and use of arbitrary strings
in place of current hard-coded "-L" switch. This partially resolves.
http://subversion.tigris.org/issues/show_bug.cgi?id=2044
* subversion/svn/cl.h
(svn_cl__opt_state_t) Add two new fields.
* subversion/svn/svn.c
(svn_cl__longopt_t) Add new field.
(svn_cl__options) Add new field and help information.
(svn_cl__cmd_table) Add two new parameters.
(sub_main) Initialize new field.
(sub_main) Add new case.
(sub_main) Add conditional call to svn_config_set.
* subversion/svn/diff-cmd.c
(svn_cl__diff) Add braces and minor indentation issue?
(svn_cl__diff) Change call to svn_client_diff6 to svn_client_diff7.
* subversion/include/svn_config.h
() Add new declarations.
* subversion/include/svn_io.h
(svn_io_run_diff2_2) Add new function.
* subversion/include/svn_client.h
(svn_client_blame) Add new function. svn_client_diff7.
* subversion/libsvn_client/diff.c
(diff_cmd_baton) Add new field.
(diff_content_changed) Replace call to svn_io_run_diff2 with
svn_io_run_diff2_2.
(svn_client_diff7) Add new function.
* subversion/libsvn_subr/io.c
(svn_io_run_diff2_2) Add new function.
]]]
Re: svn diff fix for bug 2044
Posted by Philip Martin <ph...@wandisco.com>.
Gabriela Gibson <ga...@gmail.com> writes:
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 1458203)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -2831,7 +2831,105 @@ svn_io_run_cmd(const char *path,
> return svn_io_wait_for_cmd(&cmd_proc, cmd, exitcode, exitwhy, pool);
> }
>
> +svn_error_t *
> +svn_io_run_diff2_2(const char *dir,
> + const char *const *user_args,
Indentation looks a bit out here.
> + int num_user_args,
> + const char *label1,
> + const char *label2,
> + const char *user_label_string,
> + const char *from,
> + const char *to,
> + int *pexitcode,
> + apr_file_t *outfile,
> + apr_file_t *errfile,
> + const char *diff_cmd,
> + svn_boolean_t ext_string_present,
> + apr_pool_t *pool)
> +{
> + const char **args;
> + int i;
> + int exitcode;
> + int nargs = 4; /* the diff command itself, two paths, plus a trailing NULL */
> + apr_pool_t *subpool = svn_pool_create(pool);
>
> + if (pexitcode == NULL)
> + pexitcode = &exitcode;
> +
> + if (user_args != NULL)
> + nargs += num_user_args;
> + else
> + /* Handling the case where '-u ""' is provided, to avoid adding -u */
> + if (! ext_string_present)
> + nargs += 1; /* -u */
> +
> + if (label1 != NULL)
> + nargs += 2; /* the -L and the label itself */
> + if (label2 != NULL)
> + nargs += 2; /* the -L and the label itself */
> +
> + args = apr_palloc(subpool, nargs * sizeof(char *));
> +
> + i = 0;
> + args[i++] = diff_cmd;
> +
> + if (user_args != NULL)
> + {
> + int j;
> + for (j = 0; j < num_user_args; ++j)
> + args[i++] = user_args[j];
> + }
> + else
> + if (! ext_string_present)
> + args[i++] = "-u"; /* assume -u if the user didn't give us any args */
> +
> + if (label1 != NULL)
> + {
> + if (! user_label_string)
> + args[i++] = "-L";
> + else
> + args[i++] = user_label_string;
> + args[i++] = label1;
> + }
> + if (label2 != NULL)
> + {
> + if (! user_label_string)
> + args[i++] = "-L";
> + else
> + args[i++] = user_label_string;
> + args[i++] = label2;
> + }
> +
> + args[i++] = svn_dirent_local_style(from, subpool);
> + args[i++] = svn_dirent_local_style(to, subpool);
> + args[i++] = NULL;
> +
> + SVN_ERR_ASSERT(i == nargs);
> +
> + SVN_ERR(svn_io_run_cmd(dir, diff_cmd, args, pexitcode, NULL, TRUE,
> + NULL, outfile, errfile, subpool));
> +
> + /* The man page for (GNU) diff describes the return value as:
> +
> + "An exit status of 0 means no differences were found, 1 means
> + some differences were found, and 2 means trouble."
> +
> + A return value of 2 typically occurs when diff cannot read its input
> + or write to its output, but in any case we probably ought to return an
> + error for anything other than 0 or 1 as the output is likely to be
> + corrupt.
> + */
> + if (*pexitcode != 0 && *pexitcode != 1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + _("'%s' returned %d"),
> + svn_dirent_local_style(diff_cmd, pool),
> + *pexitcode);
> +
> + svn_pool_destroy(subpool);
> +
> + return SVN_NO_ERROR;
Don't duplicate all the code like that. Take the code from the old
function svn_io_run_diff2 and use it to write a new function that
implements both svn_io_run_diff2 and svn_io_run_diff2_2 (this new
function may be svn_io_run_diff2_2 itself or it may be a static
function). Then have svn_io_run_diff2 (and svn_io_run_diff2_2 if
necessary) call the new function.
> +}
> +
> svn_error_t *
> svn_io_run_diff2(const char *dir,
> const char *const *user_args,
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1458203)
> +++ subversion/include/svn_client.h (working copy)
> @@ -3008,6 +3008,31 @@ svn_client_blame(const char *path_or_url,
> * @since New in 1.8.
> */
> svn_error_t *
> +svn_client_diff7(const apr_array_header_t *diff_options,
> + const char *path_or_url1,
> + const svn_opt_revision_t *revision1,
> + const char *path_or_url2,
> + const svn_opt_revision_t *revision2,
> + const char *relative_to_dir,
> + svn_depth_t depth,
> + svn_boolean_t ignore_ancestry,
> + svn_boolean_t no_diff_added,
> + svn_boolean_t no_diff_deleted,
> + svn_boolean_t show_copies_as_adds,
> + svn_boolean_t ignore_content_type,
> + svn_boolean_t ignore_properties,
> + const char *user_label_string,
> + svn_boolean_t ext_string_present,
> + svn_boolean_t properties_only,
> + svn_boolean_t use_git_diff_format,
> + const char *header_encoding,
> + svn_stream_t *outstream,
> + svn_stream_t *errstream,
> + const apr_array_header_t *changelists,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
> +svn_error_t *
> svn_client_diff6(const apr_array_header_t *diff_options,
> const char *path_or_url1,
> const svn_opt_revision_t *revision1,
svn_client_diff6 is new in 1.8 so until we make the 1.8 branch we can
simply change the API. You only need svn_client_diff7 if your patch is
applied after 1.8 has branched.
What about svn_client_diff_peg6? Does it need the same change?
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 1458203)
> +++ subversion/libsvn_client/diff.c (working copy)
> +svn_client_diff7(const apr_array_header_t *options,
> + const char *path_or_url1,
> + const svn_opt_revision_t *revision1,
> + const char *path_or_url2,
> + const svn_opt_revision_t *revision2,
> + const char *relative_to_dir,
> + svn_depth_t depth,
> + svn_boolean_t ignore_ancestry,
> + svn_boolean_t no_diff_added,
> + svn_boolean_t no_diff_deleted,
> + svn_boolean_t show_copies_as_adds,
> + svn_boolean_t ignore_content_type,
> + svn_boolean_t ignore_properties,
> + const char *user_label_string,
> + svn_boolean_t ext_string_present,
> + svn_boolean_t properties_only,
> + svn_boolean_t use_git_diff_format,
> + const char *header_encoding,
> + svn_stream_t *outstream,
> + svn_stream_t *errstream,
> + const apr_array_header_t *changelists,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + struct diff_cmd_baton diff_cmd_baton = { 0 };
> + svn_opt_revision_t peg_revision;
> +
> + if (ignore_properties && properties_only)
> + return svn_error_create(SVN_ERR_INCORRECT_PARAMS, NULL,
> + _("Cannot ignore properties and show only "
> + "properties at the same time"));
> +
> + /* We will never do a pegged diff from here. */
> + peg_revision.kind = svn_opt_revision_unspecified;
> +
> + /* setup callback and baton */
> + diff_cmd_baton.orig_path_1 = path_or_url1;
> + diff_cmd_baton.orig_path_2 = path_or_url2;
> +
> + SVN_ERR(set_up_diff_cmd_and_options(&diff_cmd_baton, options,
> + ctx->config, pool));
> + diff_cmd_baton.pool = pool;
> + diff_cmd_baton.outstream = outstream;
> + diff_cmd_baton.errstream = errstream;
> + diff_cmd_baton.header_encoding = header_encoding;
> + diff_cmd_baton.revnum1 = SVN_INVALID_REVNUM;
> + diff_cmd_baton.revnum2 = SVN_INVALID_REVNUM;
> +
> + diff_cmd_baton.user_label_string = user_label_string;
> + diff_cmd_baton.ext_string_present = ext_string_present;
> + diff_cmd_baton.force_binary = ignore_content_type;
> + diff_cmd_baton.ignore_properties = ignore_properties;
> + diff_cmd_baton.properties_only = properties_only;
> + diff_cmd_baton.relative_to_dir = relative_to_dir;
> + diff_cmd_baton.use_git_diff_format = use_git_diff_format;
> + diff_cmd_baton.no_diff_added = no_diff_added;
> + diff_cmd_baton.no_diff_deleted = no_diff_deleted;
> + diff_cmd_baton.no_copyfrom_on_add = show_copies_as_adds;
> +
> + diff_cmd_baton.wc_ctx = ctx->wc_ctx;
> + diff_cmd_baton.ra_session = NULL;
> + diff_cmd_baton.anchor = NULL;
> +
> + return do_diff(&diff_callbacks, &diff_cmd_baton, ctx,
> + path_or_url1, path_or_url2, revision1, revision2,
> + &peg_revision,
> + depth, ignore_ancestry, show_copies_as_adds,
> + use_git_diff_format, changelists, pool);
> +}
> +
> +svn_error_t *
> svn_client_diff6(const apr_array_header_t *options,
> const char *path_or_url1,
> const svn_opt_revision_t *revision1,
Again, don't duplicate the code. This simply goes away if you modify
svn_client_diff6 but if you do introduce svn_client_diff7 then move
svn_client_diff6 to deprecated.c and have it call svn_client_diff7.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn diff fix for bug 2044
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Mar 19, 2013 at 8:56 PM, Gabriela Gibson
<ga...@gmail.com> wrote:
> On 19/03/13 13:09, Julian Foad wrote:
>
> Julian Foad wrote:
>
>> For the record, the summary line of issue #2044 is 'Fully customizable
>> external diff invocations'. (I like to mention the summary alongside
>> the number as I am not good at memorizing issue numbers.) I'm curious
>> about your patch because I am interested in issue #2044 and would like
>> to see how this particular change would fit in.
>>
>> Please could you tell me more precisely what your patch does and why?
>> Of course I could read carefully through your patch to discover the
>> 'what', but not the 'why'.
>
> Hi Julian,
>
> It's not really a patch as such, not yet anyway :> Also, this strictly
> speaking is issue 2074, which was marked as a duplicate of 2044 since it
> partially solves 2074.
Not really about the content of your patch, but I thought I'd
reiterate what Julian said (I feel the same way): please always add
the issue summary when you mention an issue number (both in mails and
in commit messages). Personally I usually do this "inline", so I would
type:
"Also, this strictly speaking is issue 2074 (issue summary goes here),
which was marked as a duplicate of 2044 (other issue summary goes
here), since it partially solves 2074".
Of course, since the summary of 2044 is already mentioned earlier in
the thread, there's no need to repeat it, I'm just giving a typical
example ...
There are other variations, depending on your preference, whether or
not you think it breaks the flow of you sentence, ... (e.g. I also
often just add a footnote marker after the issue number [1], and add
both the link to the issue-tracker (so people can simply click on it)
and the summary in the footnote). The important thing is that the
information is there, and people don't have to go look up the
information just to understand what your email is about.
[1] http://subversion.tigris.org/issues/show_bug.cgi?id=2074
(--extensions '' doesn't work.)
Just my 2 cents ...
--
Johan
Re: svn diff fix for bug 2044
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> [...] For example, one possibility is we could use some sort of
> pattern-substitution in a specified template: we could take a pattern
> argument, in which any text enclosed in angle brackets is a special
> keyword that Subversion replaces with one of a small fixed set of
> things, so
>
> svn diff --invoke-diff='kdiff3 --L1=<label1> <file1> --L2=<label2> <file2>'
Oops, to get my own example self-consistent, I meant:
svn diff --invoke-diff='kdiff3 --L1=<label1> --L2=<label2> <tmpfile1> <tmpfile2>'
or just:
svn diff --invoke-diff='kdiff3 --L1=<label1> --L2=<label2>'
> might cause the diff tool to be invoked as
>
> Arg 0 is >--L1=subversion/include/svn_client.h (revision 1458417)<
> Arg 1 is >--L2=subversion/include/svn_client.h (working copy)<
> Arg 2 is >/home/.../...a4e4.svn-base<
> Arg 3 is >/tmp/svn-BhDgjc<
>
> This is nothing more than a made-up example of the sort of thing we could do.
> For this example, I have assumed there is a rule that if the template
> doesn't include replaceable parameters '<tmpfile1>' and
> '<tmpfile2>' then those will be appended automatically. [...]
- Julian
Re: Issue #2044 - Fully customizable external diff invocations
Posted by Julian Foad <ju...@btopenworld.com>.
Gabriela Gibson wrote:
> On 21/03/13 15:05, Julian Foad wrote:
>> - Does Subversion provide good labels? I have been using
>> 'diff3-cmd' configured to run kdiff3, and the labels Subversion passes
>> to it are like '.mine', '.r1459015' and '.r1459080' -- they don't
>> include the file name at all, which makes it very hard to see what
>> file I'm being asked to merge. (Maybe the diff3-cmd option was
>> never designed to run a GUI diff tool? But I do it.) And for
>> 'merge-tool-cmd' it doesn't appear to pass any labels at the moment.
>> What a lot of inconsistency to sort out. But if the labels passed
>> to the diff-cmd are always good, you don't worry about this yet.
>
> Thank you for the well-thought-out response. It cleared up some
> misconceptions, and yes, I did think of merge there. :)
>
> I have to think about your letter in detail a little more before I can
> say anymore.
Of course. Glad it helps.
> ps.: I think that the issue with kdiff3 is with the label
> handling. The section below suggests to me that kdiff3 is dropping
> some info from the label it has been passed:
>
> <quote>
> g_at_musashi:~/tmp/test-wc$ $SVN diff
> --diff-cmd=/home/g/programming/perlfiles/dump_diff.pl
> Index: testfile
> ===================================================================
> Dumping @ARGV...
> Arg 0 is >-u<
> Arg 1 is >-L<
> Arg 2 is >testfile (revision 1)<
> Arg 3 is >-L<
> Arg 4 is >testfile (working copy)<
[...]
No, your test there is with "svn diff", whereas my issue is about the "diff3-cmd" configuration, which is used to *merge* files during svn update, svn switch or svn merge. I used a script like yours, that log the arguments that are being passed to it, and the exact arguments Subversion passed to it were: -E -m -L .mine -L .r1458321 -L .r1459485 and then the *three* input files.
- Julian
Re: Issue #2044 - Fully customizable external diff invocations
Posted by Gabriela Gibson <ga...@gmail.com>.
On 21/03/13 15:05, Julian Foad wrote:
> - Does Subversion provide good labels? I have been using
> 'diff3-cmd' configured to run kdiff3, and the labels Subversion passes
> to it are like '.mine', '.r1459015' and '.r1459080' -- they don't
> include the file name at all, which makes it very hard to see what
> file I'm being asked to merge. (Maybe the diff3-cmd option was
> never designed to run a GUI diff tool? But I do it.) And for
> 'merge-tool-cmd' it doesn't appear to pass any labels at the moment.
> What a lot of inconsistency to sort out. But if the labels passed
> to the diff-cmd are always good, you don't worry about this yet.
Thank you for the well-thought-out response. It cleared up some
misconceptions, and yes, I did think of merge there. :)
I have to think about your letter in detail a little more before I can
say anymore.
Gabriela
ps.: I think that the issue with kdiff3 is with the label
handling. The section below suggests to me that kdiff3 is dropping
some info from the label it has been passed:
<quote>
g_at_musashi:~/tmp/test-wc$ $SVN diff
--diff-cmd=/home/g/programming/perlfiles/dump_diff.pl
Index: testfile
===================================================================
Dumping @ARGV...
Arg 0 is >-u<
Arg 1 is >-L<
Arg 2 is >testfile (revision 1)<
Arg 3 is >-L<
Arg 4 is >testfile (working copy)<
Arg 5 is
>/home/g/tmp/test-wc/.svn/pristine/91/91b7b0b1e27bfbf7bc646946f35fa972c47c2d32.svn-base<
Arg 6 is >/home/g/tmp/test-wc/testfile<
g_at_musashi:~/tmp/test-wc$
</quote>
Re: Issue #2044 - Fully customizable external diff invocations
Posted by Julian Foad <ju...@btopenworld.com>.
(I've taken the liberty of changing the subject line.)
Gabriela Gibson wrote:
> IIRC Subversion needs to communicate the following file names to the
> user's diff program:
>
> mine, yours, base and output
You're thinking of 'merge', otherwise known as 'diff3', here?
The '--diff-cmd' command-line option (and corresponding 'diff-cmd' config-file option) is used for displaying a diff of a pair of files. The output is assumed just to be displayed on the standard output (console) or in a GUI window; it is not captured by Subversion.
Of course it will be sensible to apply the same kind of design to the merge tool configuration. The '--diff3-cmd' command-line option, and the 'diff3-cmd' and 'merge-tool-cmd' config-file options, are all used for what we call 3-way merging, where two files and a base file (usually their youngest common ancestor) are compared and a merged output file is produced.
I'm not terribly clear about the difference between 'diff3-cmd' and 'merge-tool-cmd'. Given that I've been working on merging for about two years now, it's about time I found out. Subversion assumes the output will be written to a file for the
'merge-tool-cmd' option, but assumes it will be written to standard
output with the 'diff3-cmd' option. I'm not sure why they are different
and this is one of the inconsistencies we should probably clear up now.
Anyway, I recommend you start with 'diff-cmd', and after you've got that working then expand the solution to cover the 'diff3-cmd' and 'merge-tool-cmd' as well.
> It then takes the user input (--diff-cmd) from either the command line
> or a script, and produces a command to run the external diff in order
> to harvest that output.
>
> All we need to do is to render the command line *exactly* as typed
> within the delimiters of (say) %% at the end and start, with the
> exception that %mine %yours, %base and %output will be string
> substituted
[...]
> What do you think?
I will refrain from commenting on your specific syntax suggestion, and just point out that we would do well to adopt one of the many well known standard syntaxes (sadly there is not a single standard standard). Enough developers here have experience of this sort of thing to be able to make a good specific suggestion, but to avoid getting side-tracked by a potentially long-winded discussion of the details at this point, let's assume that we will eventually decide on a suitable syntax, conceptually roughly like you are thinking of, but different in the details.
In order to get something working so that you can experiment with how well the whole pluggable diff tool idea works in practice, you could very well start by implementing your own suggestion; just don't spend too much time on this part, start very simple, and be prepared to replace it later.
Meanwhile, perhaps some of the more interesting higher level issues are:
- Start making a list of popular diff tools
(both command-line and GUI), and the form of command-line we would want to provide for each of them (in terms of the main arguments -- file names and labels -- not caring about the other options they can take).
- What template parameters do we
need to be able to substitute? The file paths; the labels; the extra options specified with "-x"; any other things?
- It could be helpful to use a Wiki page for the above two lists, so that anyone can refer to them at any time while you build them incrementally. And for writing down other aspects of the evolving design.
- Are there some diff tools that don't accept 'label' arguments but just display the given file names? If so, do we need to make Subversion generate temporary file names that are more descriptive than a random unique name such as '.svn/tmp/svn-TiBS24'? In what cases does Subversion pass random unique names and in what cases does it pass understandable paths such as 'trunk/foo/myfile.c'? It may depend on what sort of diff we request: WC-WC diff (such as the default base:working diff, 'svn diff myfile.c'), or repos-WC diff (such as svn diff -r10 myfile.c), or repos-repos diff (such as 'svn diff -r10:20 myfile.c').
- Does Subversion provide good labels? I have been using 'diff3-cmd' configured to run kdiff3, and the labels Subversion passes to it are like '.mine', '.r1459015' and '.r1459080' -- they don't include the file name at all, which makes it very hard to see what file I'm being asked to merge. (Maybe the diff3-cmd option was never designed to run a GUI diff tool? But I do it.) And for 'merge-tool-cmd' it doesn't appear to pass any labels at the moment. What a lot of inconsistency to sort out. But if the labels passed to the diff-cmd are always good, you don't worry about this yet.
- When we support the diff3/merge tool configuration, how are we going to capture the output? One merge tools might write to a file whose name we have to provide, another might write to a file whose name *it* provides (do any of the popular tools do that?), while another might write to standard output. We need to support at least the first and last ways, to avoid forcing users to write a wrapper script for the other cases.
I hope these ideas are useful. Hopefully the answers to many of those questions will turn out to be simple: "all we really need is X and Y".
If it's not too overwhelming to peek into the future, then isse #2447 "Support for external diff commands for non-text types" is something I'm looking towards. Not expecting you to do this as well, but once we've got support for customizable diff commands, then I want us to let the user specify two or more different diff commands, and specify some rules to determine which diff command will be used for a given file, probably based on matching MIME types and filename patterns and such like.
- Julian
Re: svn diff fix for bug 2044
Posted by Gabriela Gibson <ga...@gmail.com>.
Thanks for all the feedback, advice and ideas so far :)
IIRC Subversion needs to communicate the following file names to the
user's diff program:
mine, yours, base and output
It then takes the user input (--diff-cmd) from either the command line
or a script, and produces a command to run the external diff in order
to harvest that output.
All we need to do is to render the command line *exactly* as typed
within the delimiters of (say) %% at the end and start, with the
exception that %mine %yours, %base and %output will be string
substituted by subversion accordingly (a bit like printf does)
A simple case:
-diff-cmd=%%/usr/fancy_diff_cmd %mine %yours %base %output%%
or more complex:
-diff-cmd=%%/usr/fancy_diff_cmd -x "" -L ' ' %mine -Q %yours -Z %base -q
"xyzzy" %output%%
The reason for the un-unix-like "%" syntax is that we don't want to
restrict user's freedom to use any kind of switches or whatever syntax
they want to use.
Also, the %% delimiter keeps it visually simple for the user as well
and already matches the token pattern we're looking for, plus it makes
the quoting issues go away.
What do you think?
Re: svn diff fix for bug 2044
Posted by Branko Čibej <br...@wandisco.com>.
On 20.03.2013 13:22, Philip Martin wrote:
> Turning to the command itself, Subversion would have to parse the string
> and split it into arguments. That gets compilcated if the user wants to
> be able to use quotes to pass whitespace:
>
> diff-cmd='prog --arg "foo bar" <file1> <file2>'
>
> A unix user probably wants prog to see one argument "foo bar", without
> quotes, rather than two arguments foo and bar.
You don't need quotes around the whole expansion in the config file.
Splitting the command into an array is trickier as our config format
currently doesn't have a native notation for that and we'd have to
invent one that's backwards-compatible.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: svn diff fix for bug 2044
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
> I agree. I like the idea of allowing users to specify a template in the
> configuration file to control the way a diff tool is invoked.
> I also like the format you proposed. Do you think it would be reasonable
> to add this new template support to the existing diff-cmd option in the
> ~/.subversion/config file?
>
> diff-cmd='kdiff3 --L1=<label1> <file1> --L2=<label2> <file2>'
>
> Currently, diff-cmd takes just a path to a binary which is then invoked
> with a fixed set of parameters. It seems very unlikely that people are
> already using diff commands which contain strings like "<label1>" etc.
> So such a change could be considered a backward-compatible extension
> of the diff-cmd option.
That's neat. However we have to consider whitespace/quoting issues in
the filenames and the command and that can be tricky. Consider the
filenames/labels first:
On Unix we currently invoke diff without a shell passing an array of
arguments directly to exec. I think your example would work even when
<file1> or <label1> gets expanded to something containing spaces.
On Windows I believe APR concatenates the array of arguments and passes
the resulting string to Windows. Windows quoting is a bit of a mystery
to me but perhaps something like this would work:
diff-cmd='kdiff3 --L1="<label1>" "<file1>" --L2="<label2>" "<file2>"'
The tricky bit is that that will not work on Unix. The extra quotes
would end up getting passed directly to the command which would see them
as part of the filename. Perhaps on Windows we make <file1> expand to
a name surrounded by quotes?
Turning to the command itself, Subversion would have to parse the string
and split it into arguments. That gets compilcated if the user wants to
be able to use quotes to pass whitespace:
diff-cmd='prog --arg "foo bar" <file1> <file2>'
A unix user probably wants prog to see one argument "foo bar", without
quotes, rather than two arguments foo and bar.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn diff fix for bug 2044
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 19, 2013 at 09:28:50PM +0000, Julian Foad wrote:
> Of course, users won't normally want to be typing this configuration
> on the command line, they'll want it to be in a config file. But it's
> useful to have the possibility of doing it on the command line as
> well.
If it's a config file option, it's also a command-line option!
See the --config-option option which can be used to override any
configuration file parameter via the command line.
> Overall, I think we should address the '-u' and '-L' issue as part of
> a larger issue and not implement a solution that specifically provides
> just a way to manage those particular options.
I agree. I like the idea of allowing users to specify a template in the
configuration file to control the way a diff tool is invoked.
I also like the format you proposed. Do you think it would be reasonable
to add this new template support to the existing diff-cmd option in the
~/.subversion/config file?
diff-cmd='kdiff3 --L1=<label1> <file1> --L2=<label2> <file2>'
Currently, diff-cmd takes just a path to a binary which is then invoked
with a fixed set of parameters. It seems very unlikely that people are
already using diff commands which contain strings like "<label1>" etc.
So such a change could be considered a backward-compatible extension
of the diff-cmd option.
Re: svn diff fix for bug 2044
Posted by Julian Foad <ju...@btopenworld.com>.
Gabriela Gibson wrote:
> On 19/03/13 13:09, Julian Foad wrote:
>> For the record, the summary line of issue #2044 is 'Fully customizable
>> external diff invocations'. (I like to mention the summary alongside
>> the number as I am not good at memorizing issue numbers.) I'm curious
>> about your patch because I am interested in issue #2044 and would like
>> to see how this particular change would fit in.
>>
>> Please could you tell me more precisely what your patch does and why?
>> Of course I could read carefully through your patch to discover the
>> 'what', but not the 'why'.
>
> Hi Julian,
>
> It's not really a patch as such, not yet anyway :> Also, this strictly
> speaking is issue 2074, which was marked as a duplicate of 2044 since it
> partially solves 2074.
For my and everyone else's benefit, issue #2074 [1] is titled, "--extensions '' doesn't work." and requests a way to get rid of the '-u' and '-L' options that Subversion supplies when invoking an external diff, for use with diff tools that don't support those switches. The tools 'bbdiff' and 'sdiff' are mentioned, on a Mac.
> Given the following perl script posing as my diff command:
[...]
> The output of this 'diff-command' looks like on a test repository with
> a single change:
[...]
> Arg 0 is >-u<
> Arg 1 is >-L<
> Arg 2 is >testfile (revision 1)<
> Arg 3 is >-L<
> Arg 4 is >testfile (working copy)<
> Arg 5 is >/home/.../.svn/pristine/91/91b7...2d32.svn-base<
> Arg 6 is >/home/g/tmp/test-wc/testfile<
> The goal of my patch is to provide two facilities:
> 1. Allow the complete removal of the "-u" switch.
> 2. Allow the replacement of the "-L" switch
>
> (One question out of 2 above is, should we allow the complete removal
> of the -L as the patch does for the "-u". Currently, there is an
> empty element in argv, the alternative is to add another flag, (say)
> --no-diff-label)
See below.
> The patch adds a 'char *user_label_string' and a 'svn_boolean_t
> ext_string_present' parameter to the internal structures.
Never mind the internal structures yet.
> This part of the patch (once it's completed) allows you to do:
>
> [...] svn diff -x "" --diff-label=""
> --diff-cmd=/home/g/programming/perlfiles/dump_diff.pl
> Index: subversion/include/svn_client.h
> ===================================================================
> Dumping @ARGV...
> Arg 0 is ><
> Arg 1 is >subversion/include/svn_client.h (revision 1458417)<
> Arg 2 is ><
> Arg 3 is >subversion/include/svn_client.h (working copy)<
> Arg 4 is
>> /home/g/trunk_diff4/.svn/pristine/90/908abcdec38f17df77baf339075101ba4471a4e4.svn-base<
> Arg 5 is >/tmp/svn-40JzGW<
I can assure you that's not going to fly with empty arguments. Try it with one or two typical diff utilities such as GNU 'diff' and 'kdiff3'. Yes we certainly need to be able to remove the '-L' arguments completely. I would also say we need to be able to remove the labels themselves (args 1 and 3 here) because I'm sure not all diff tools will accept them.
> or also:
>
> [...] svn diff -x "Hansel" --diff-label="Gretel"
> --diff-cmd=/home/g/programming/perlfiles/dump_diff.pl
> Index: subversion/include/svn_client.h
> ===================================================================
> Dumping @ARGV...
> Arg 0 is >Hansel<
> Arg 1 is >Gretel<
> Arg 2 is >subversion/include/svn_client.h (revision 1458417)<
> Arg 3 is >Gretel<
> Arg 4 is >subversion/include/svn_client.h (working copy)<
> Arg 5 is >/home/.../.svn/pristine/90/908a...a4e4.svn-base<
> Arg 6 is >/tmp/svn-BhDgjc<
Might some diff programs want a different option name for each label, such as "--l1 label1 --l2 label2"? I would expect so. From kdiff3's help:
--L1 alias1 Visible name replacement for input file 1 (base).
--L2 alias2 Visible name replacement for input file 2.
--L3 alias3 Visible name replacement for input file 3.
-L, --fname alias Alternative visible name replacement. Supply this once for every input.
so kdiff3 optionally accepts '--L1' etc., although it has an alternative. That alone makes me suspect there are programs that require options like '--L1'. And do some diff programs require the label to be attached to the option name like "-Llabel1" or "-L:label1" rather than one arg being the option and the next arg being the label? Again, I would expect so.
You'll need to check a few typical diff tools (or their user manuals) to answer those kinds of questions.
Overall, I think command-line options like these are too specific. We don't want to grow a dozen new command-line options as we discover a dozen different ways that external diff tools can be controlled. Rather, we need to look at this in the context of how to configure a generic diff command more easily. For example, one possibility is we could use some sort of pattern-substitution in a specified template: we could take a pattern argument, in which any text enclosed in angle brackets is a special keyword that Subversion replaces with one of a small fixed set of things, so
svn diff --invoke-diff='kdiff3 --L1=<label1> <file1> --L2=<label2> <file2>'
might cause the diff tool to be invoked as
Arg 0 is >--L1=subversion/include/svn_client.h (revision 1458417)<
Arg 1 is >--L2=subversion/include/svn_client.h (working copy)<
Arg 2 is >/home/.../...a4e4.svn-base<
Arg 3 is >/tmp/svn-BhDgjc<
This is nothing more than a made-up example of the sort of thing we could do. For this example, I have assumed there is a rule that if the template doesn't include replaceable parameters '<tmpfile1>' and '<tmpfile2>' then those will be appended automatically. With this kind of template idea, we'd also have to figure out what to do about spaces and quoting, which is a problem that has standard solutions but is not totally trivial.
There are surely other, different ways to allow more flexible configuration.
Of course, users won't normally want to be typing this configuration on the command line, they'll want it to be in a config file. But it's useful to have the possibility of doing it on the command line as well.
Overall, I think we should address the '-u' and '-L' issue as part of a larger issue and not implement a solution that specifically provides just a way to manage those particular options.
- Julian
[1] <http://subversion.tigris.org/issues/show_bug.cgi?id=2074>
Re: svn diff fix for bug 2044
Posted by Gabriela Gibson <ga...@gmail.com>.
On 19/03/13 13:09, Julian Foad wrote:
Julian Foad wrote:
> For the record, the summary line of issue #2044 is 'Fully customizable
> external diff invocations'. (I like to mention the summary alongside
> the number as I am not good at memorizing issue numbers.) I'm curious
> about your patch because I am interested in issue #2044 and would like
> to see how this particular change would fit in.
>
> Please could you tell me more precisely what your patch does and why?
> Of course I could read carefully through your patch to discover the
> 'what', but not the 'why'.
Hi Julian,
It's not really a patch as such, not yet anyway :> Also, this strictly
speaking is issue 2074, which was marked as a duplicate of 2044 since it
partially solves 2074.
Given the following perl script posing as my diff command:
cat << EOF > dump_diff.pl
#!/usr/bin/perl -w
print "Dumping \@ARGV...\n";
for (my $i = 0; $i < @ARGV; $i++) {
print "Arg $i is >$ARGV[$i]<\n";
}
exit 0;
EOF
The output of this 'diff-command' looks like on a test repository with
a single change:
<quote>
g@musashi:~/tmp/test-wc$ $SVN diff
--diff-cmd=/home/g/programming/perlfiles/dump_diff.pl
Index: testfile
===================================================================
Dumping @ARGV...
Arg 0 is >-u<
Arg 1 is >-L<
Arg 2 is >testfile (revision 1)<
Arg 3 is >-L<
Arg 4 is >testfile (working copy)<
Arg 5 is
>/home/g/tmp/test-wc/.svn/pristine/91/91b7b0b1e27bfbf7bc646946f35fa972c47c2d32.svn-base<
Arg 6 is >/home/g/tmp/test-wc/testfile<
g@musashi:~/tmp/test-wc$
</quote>
The goal of my patch is to provide two facilities:
1. Allow the complete removal of the "-u" switch.
2. Allow the replacement of the "-L" switch
(One question out of 2 above is, should we allow the complete removal
of the -L as the patch does for the "-u". Currently, there is an
empty element in argv, the alternative is to add another flag, (say)
--no-diff-label)
The patch adds a 'char *user_label_string' and a 'svn_boolean_t
ext_string_present' parameter to the internal structures.
This part of the patch (once it's completed) allows you to do:
g@musashi:~/trunk_diff4$ ~/trunk_diff4/subversion/svn/svn diff -x ""
--diff-label="" --diff-cmd=/home/g/programming/perlfiles/dump_diff.pl
Index: subversion/include/svn_client.h
===================================================================
Dumping @ARGV...
Arg 0 is ><
Arg 1 is >subversion/include/svn_client.h (revision 1458417)<
Arg 2 is ><
Arg 3 is >subversion/include/svn_client.h (working copy)<
Arg 4 is
>/home/g/trunk_diff4/.svn/pristine/90/908abcdec38f17df77baf339075101ba4471a4e4.svn-base<
Arg 5 is >/tmp/svn-40JzGW<
or also:
g@musashi:~/trunk_diff4$ ~/trunk_diff4/subversion/svn/svn diff -x
"Hansel" --diff-label="Gretel" --diff-cmd=/home/g/programming/perlfiles
/dump_diff.pl
Index: subversion/include/svn_client.h
===================================================================
Dumping @ARGV...
Arg 0 is >Hansel<
Arg 1 is >Gretel<
Arg 2 is >subversion/include/svn_client.h (revision 1458417)<
Arg 3 is >Gretel<
Arg 4 is >subversion/include/svn_client.h (working copy)<
Arg 5 is
>/home/g/trunk_diff4/.svn/pristine/90/908abcdec38f17df77baf339075101ba4471a4e4.svn-base<
Arg 6 is >/tmp/svn-BhDgjc<
otherwise, the behavior is exactly as it was before.
regards,
Gabriela
Re: svn diff fix for bug 2044
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Gabriela.
Gabriela Gibson wrote:
> I've made some changes to meet some feature requests regarding svn diff.
>
> Could you please take a look and let me know if I'm on the right track?
> [[[
> Change "svn diff" to allow removal of "-u" and use of
> arbitrary strings in place of current hard-coded "-L" switch. This
> partially resolves.
> http://subversion.tigris.org/issues/show_bug.cgi?id=2044
For the record, the summary line of issue #2044 is 'Fully customizable external diff invocations'. (I like to mention the summary alongside the number as I am not good at memorizing issue numbers.) I'm curious about your patch because I
am interested in issue #2044 and would like to see how this particular
change would fit in.
Please could you tell me more precisely what your patch does and why? Of course I could read carefully through your patch to discover the 'what', but not the 'why'.
Thanks.
- Julian
>
> * subversion/svn/cl.h
> (svn_cl__opt_state_t) Add two new fields.
>
> * subversion/svn/svn.c
> (svn_cl__longopt_t) Add new field.
> (svn_cl__options) Add new field and help information.
> (svn_cl__cmd_table) Add two new parameters.
> (sub_main) Initialize new field.
> (sub_main) Add new case.
> (sub_main) Add conditional call to svn_config_set.
>
> * subversion/svn/diff-cmd.c
> (svn_cl__diff) Add braces and minor indentation issue?
> (svn_cl__diff) Change call to svn_client_diff6 to svn_client_diff7.
>
> * subversion/include/svn_config.h
> () Add new declarations.
>
> * subversion/include/svn_io.h
> (svn_io_run_diff2_2) Add new function.
>
> * subversion/include/svn_client.h
> (svn_client_blame) Add new function. svn_client_diff7.
>
> * subversion/libsvn_client/diff.c
> (diff_cmd_baton) Add new field.
> (diff_content_changed) Replace call to svn_io_run_diff2 with
> svn_io_run_diff2_2.
> (svn_client_diff7) Add new function.
>
> * subversion/libsvn_subr/io.c
> (svn_io_run_diff2_2) Add new function.
>
> ]]]
>