You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2013/06/19 01:58:32 UTC
Re: vimdiff wrapper for diff-cmd not working with 1.8
[cc to dev]
Michael Schlottke <m....@aia.rwth-aachen.de> writes:
> I just installed svn 1.8 on our cluster. Before, we used svn 1.7.9 and
> a little vimdiff wrapper (taken, with a few changes, from
> http://svnbook.red-bean.com/nightly/en/svn.advanced.externaldifftools.html#svn.advanced.externaldifftools.diff),
> which worked like a charm when called as "svn diff
> --diff-cmd=diffwrap.py filename". However, with svn 1.8 something
> seems to have changed:
>
> The only lines I see after executing the command above are
>
>> Index: filename
>> ===================================================================
>
> and the terminal hangs. When I enter ":qa!", I get the following line
>
>> Vim: Warning: Output is not to a terminal
>
> and I am back in the terminal. I have not changed anything in the
> wrapper script, and indeed, when I manually use it with our old svn
> version (1.7.9), it still works. Does anyone have an idea what has
> changed in the way the diff-cmd is invoked? And, more importantly, how
> I can change the vimdiff wrapper so it works again?
I invoked vimdiff using a diff-cmd of:
#!/bin/sh
vimdiff $6 $7
this works with 1.7 but fails as you describe in 1.8.
The cause is the conversion of diff to the stream API. The code in
libsvn_client/diff.c:diff_content_changed now gets a Subversion stream
rather than an APR file for output so it does:
/* We deal in streams, but svn_io_run_diff2() deals in file handles,
unfortunately, so we need to make these temporary files, and then
copy the contents to our stream. */
SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
svn_io_file_del_on_pool_cleanup,
scratch_pool, scratch_pool));
and this use of a temporary file prevents the use of an external diff
that expects a terminal.
The only way I see to fix this is to stop using the stream API when the
external diff command wants a terminal. I don't think it is possible to
do this automatically, perhaps we need an --interactive-diff option?
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Michael Schlottke <m....@aia.rwth-aachen.de> writes:
> However, for now I'd prefer a quick-n-dirty solution rather than not
> being able to upgrade at all. I know this is asking for quite
> something, especially since I don't know how to do it myself :-/
Here's my really quick-n-dirty patch used during investigation. It's
not going to be committed. If it breaks you get to keep the pieces:
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 1494268)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -816,13 +816,18 @@ diff_content_changed(svn_boolean_t *wrote_header,
svn_io_file_del_on_pool_cleanup,
scratch_pool, scratch_pool));
+ {
+ apr_file_t *stdout_file;
+ apr_file_open_stdout(&stdout_file, scratch_pool);
+
SVN_ERR(svn_io_run_diff2(".",
diff_cmd_baton->options.for_external.argv,
diff_cmd_baton->options.for_external.argc,
label1, label2,
tmpfile1, tmpfile2,
- &exitcode, outfile, errfile,
+ &exitcode, stdout_file, errfile,
diff_cmd_baton->diff_cmd, scratch_pool));
+ }
SVN_ERR(svn_io_file_close(outfile, scratch_pool));
SVN_ERR(svn_io_file_close(errfile, scratch_pool));
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Michael Schlottke <m....@aia.rwth-aachen.de> writes:
> However, for now I'd prefer a quick-n-dirty solution rather than not
> being able to upgrade at all. I know this is asking for quite
> something, especially since I don't know how to do it myself :-/
Here's my really quick-n-dirty patch used during investigation. It's
not going to be committed. If it breaks you get to keep the pieces:
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 1494268)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -816,13 +816,18 @@ diff_content_changed(svn_boolean_t *wrote_header,
svn_io_file_del_on_pool_cleanup,
scratch_pool, scratch_pool));
+ {
+ apr_file_t *stdout_file;
+ apr_file_open_stdout(&stdout_file, scratch_pool);
+
SVN_ERR(svn_io_run_diff2(".",
diff_cmd_baton->options.for_external.argv,
diff_cmd_baton->options.for_external.argc,
label1, label2,
tmpfile1, tmpfile2,
- &exitcode, outfile, errfile,
+ &exitcode, stdout_file, errfile,
diff_cmd_baton->diff_cmd, scratch_pool));
+ }
SVN_ERR(svn_io_file_close(outfile, scratch_pool));
SVN_ERR(svn_io_file_close(errfile, scratch_pool));
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Michael Schlottke <m....@aia.rwth-aachen.de>.
On Jun 19, 2013, at 09:56 , Ben Reser wrote:
> On Wed, Jun 19, 2013 at 9:11 AM, Michael Schlottke
> <m....@aia.rwth-aachen.de> wrote:
>> Do you have an idea of how hard this is to achieve, or how long it would
>> take to create a patch? I'd be happy to volunteer as a tester…
>> Or do you know of an interim hack that I could use until it is properly
>> fixed?
>
> I don't imagine it'd take very long at all to implement but the
> problem of course is that we really should think carefully how we go
> about doing this. If we can detect this at runtime we probably
> should.
I fully agree that this should be done right, and preferably without adding complexity through additional flags.
However, for now I'd prefer a quick-n-dirty solution rather than not being able to upgrade at all. I know this is
asking for quite something, especially since I don't know how to do it myself :-/
> I'm not sure what platform you're on but using the GUI version of
> vimdiff would get around this for you in the meantime.
Thanks for the tip! Unfortunately, gvim is not an option unfortunately, since most of the machines do not have a proper version
of gvim installed.
Michael
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Michael Schlottke <m....@aia.rwth-aachen.de>.
On Jun 19, 2013, at 12:08 , Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Ben Reser <be...@reser.org> writes:
>>
>>> I don't imagine it'd take very long at all to implement but the
>>> problem of course is that we really should think carefully how we go
>>> about doing this. If we can detect this at runtime we probably
>>> should.
>>
>> I don't see how Subversion can determine that one script needs a
>> terminal while another can use a file, only the user knows that.
>>
>> It's also hard to fix 1.8, how do we pass the information into the
>> client library without changing the API? Perhaps we could recognise a
>> special part of the command name or a special external parameter, so
>>
>> --diff-cmd svn:interactive:myscript
>>
>> or
>>
>> --diff-cmd myscript -x svn:interactive
>>
>> gets a terminal while
>>
>> --diff-cmd myscript
>>
>> gets a file.
>
> Or we could extend the opaque svn_stream_t to make the underlying
> apr_file_t available. Is mixing output to the file and output to
> the stream acceptable or does it introduce output order problems?
I tried & installed both your patches and both work just fine, thank you very much.
Without giving it much thought, I now went for your second patch (it seemed to be the more sophisticated one).
I think something like the "-x svn:interactive" option would be the best solution, since that way the diff-cmd really
just remains a command.
Thank you very much for your quick help! I'll definitely keep tabs on the development of this, I hope that one way
or another it will make it into the svn code base.
Michael
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Michael Schlottke <m....@aia.rwth-aachen.de>.
On Jun 19, 2013, at 12:08 , Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Ben Reser <be...@reser.org> writes:
>>
>>> I don't imagine it'd take very long at all to implement but the
>>> problem of course is that we really should think carefully how we go
>>> about doing this. If we can detect this at runtime we probably
>>> should.
>>
>> I don't see how Subversion can determine that one script needs a
>> terminal while another can use a file, only the user knows that.
>>
>> It's also hard to fix 1.8, how do we pass the information into the
>> client library without changing the API? Perhaps we could recognise a
>> special part of the command name or a special external parameter, so
>>
>> --diff-cmd svn:interactive:myscript
>>
>> or
>>
>> --diff-cmd myscript -x svn:interactive
>>
>> gets a terminal while
>>
>> --diff-cmd myscript
>>
>> gets a file.
>
> Or we could extend the opaque svn_stream_t to make the underlying
> apr_file_t available. Is mixing output to the file and output to
> the stream acceptable or does it introduce output order problems?
I tried & installed both your patches and both work just fine, thank you very much.
Without giving it much thought, I now went for your second patch (it seemed to be the more sophisticated one).
I think something like the "-x svn:interactive" option would be the best solution, since that way the diff-cmd really
just remains a command.
Thank you very much for your quick help! I'll definitely keep tabs on the development of this, I hope that one way
or another it will make it into the svn code base.
Michael
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Ben Reser <be...@reser.org> writes:
>
>> I don't imagine it'd take very long at all to implement but the
>> problem of course is that we really should think carefully how we go
>> about doing this. If we can detect this at runtime we probably
>> should.
>
> I don't see how Subversion can determine that one script needs a
> terminal while another can use a file, only the user knows that.
>
> It's also hard to fix 1.8, how do we pass the information into the
> client library without changing the API? Perhaps we could recognise a
> special part of the command name or a special external parameter, so
>
> --diff-cmd svn:interactive:myscript
>
> or
>
> --diff-cmd myscript -x svn:interactive
>
> gets a terminal while
>
> --diff-cmd myscript
>
> gets a file.
Or we could extend the opaque svn_stream_t to make the underlying
apr_file_t available. Is mixing output to the file and output to
the stream acceptable or does it introduce output order problems?
Index: subversion/include/private/svn_io_private.h
===================================================================
--- subversion/include/private/svn_io_private.h (revision 1494268)
+++ subversion/include/private/svn_io_private.h (working copy)
@@ -90,7 +90,10 @@ svn_stream__set_is_buffered(svn_stream_t *stream,
svn_boolean_t
svn_stream__is_buffered(svn_stream_t *stream);
+apr_file_t *
+svn_stream__aprfile(svn_stream_t *stream);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 1494268)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -51,6 +51,7 @@
#include "private/svn_wc_private.h"
#include "private/svn_diff_private.h"
#include "private/svn_subr_private.h"
+#include "private/svn_io_private.h"
#include "svn_private_config.h"
@@ -809,13 +810,22 @@ diff_content_changed(svn_boolean_t *wrote_header,
/* We deal in streams, but svn_io_run_diff2() deals in file handles,
unfortunately, so we need to make these temporary files, and then
copy the contents to our stream. */
- SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
- svn_io_file_del_on_pool_cleanup,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL,
- svn_io_file_del_on_pool_cleanup,
- scratch_pool, scratch_pool));
+ outfile = svn_stream__aprfile(outstream);
+ if (!outfile)
+ SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
+ svn_io_file_del_on_pool_cleanup,
+ scratch_pool, scratch_pool));
+ else
+ outfilename = NULL;
+ errfile = svn_stream__aprfile(errstream);
+ if (!errfile)
+ SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL,
+ svn_io_file_del_on_pool_cleanup,
+ scratch_pool, scratch_pool));
+ else
+ errfilename = NULL;
+
SVN_ERR(svn_io_run_diff2(".",
diff_cmd_baton->options.for_external.argv,
diff_cmd_baton->options.for_external.argc,
@@ -824,20 +834,28 @@ diff_content_changed(svn_boolean_t *wrote_header,
&exitcode, outfile, errfile,
diff_cmd_baton->diff_cmd, scratch_pool));
- SVN_ERR(svn_io_file_close(outfile, scratch_pool));
- SVN_ERR(svn_io_file_close(errfile, scratch_pool));
+ if (outfilename)
+ SVN_ERR(svn_io_file_close(outfile, scratch_pool));
+ if (errfilename)
+ SVN_ERR(svn_io_file_close(errfile, scratch_pool));
/* Now, open and copy our files to our output streams. */
- SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream,
- scratch_pool),
+ if (outfilename)
+ {
+ SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream,
+ scratch_pool),
+ NULL, NULL, scratch_pool));
+ }
+ if (errfilename)
+ {
+ SVN_ERR(svn_stream_open_readonly(&stream, errfilename,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(errstream,
+ scratch_pool),
NULL, NULL, scratch_pool));
- SVN_ERR(svn_stream_open_readonly(&stream, errfilename,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(errstream,
- scratch_pool),
- NULL, NULL, scratch_pool));
+ }
/* We have a printed a diff for this path, mark it as visited. */
*wrote_header = TRUE;
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1494268)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -56,6 +56,7 @@ struct svn_stream_t {
svn_stream_mark_fn_t mark_fn;
svn_stream_seek_fn_t seek_fn;
svn_stream__is_buffered_fn_t is_buffered_fn;
+ apr_file_t *file; /* Maybe NULL */
};
@@ -81,6 +82,7 @@ svn_stream_create(void *baton, apr_pool_t *pool)
stream->mark_fn = NULL;
stream->seek_fn = NULL;
stream->is_buffered_fn = NULL;
+ stream->file = NULL;
return stream;
}
@@ -913,6 +915,7 @@ svn_stream_from_aprfile2(apr_file_t *file,
svn_stream_set_mark(stream, mark_handler_apr);
svn_stream_set_seek(stream, seek_handler_apr);
svn_stream__set_is_buffered(stream, is_buffered_handler_apr);
+ stream->file = file;
if (! disown)
svn_stream_set_close(stream, close_handler_apr);
@@ -920,6 +923,12 @@ svn_stream_from_aprfile2(apr_file_t *file,
return stream;
}
+apr_file_t *
+svn_stream__aprfile(svn_stream_t *stream)
+{
+ return stream->file;
+}
+
/* Compressed stream support */
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Ben Reser <be...@reser.org> writes:
>
>> I don't imagine it'd take very long at all to implement but the
>> problem of course is that we really should think carefully how we go
>> about doing this. If we can detect this at runtime we probably
>> should.
>
> I don't see how Subversion can determine that one script needs a
> terminal while another can use a file, only the user knows that.
>
> It's also hard to fix 1.8, how do we pass the information into the
> client library without changing the API? Perhaps we could recognise a
> special part of the command name or a special external parameter, so
>
> --diff-cmd svn:interactive:myscript
>
> or
>
> --diff-cmd myscript -x svn:interactive
>
> gets a terminal while
>
> --diff-cmd myscript
>
> gets a file.
Or we could extend the opaque svn_stream_t to make the underlying
apr_file_t available. Is mixing output to the file and output to
the stream acceptable or does it introduce output order problems?
Index: subversion/include/private/svn_io_private.h
===================================================================
--- subversion/include/private/svn_io_private.h (revision 1494268)
+++ subversion/include/private/svn_io_private.h (working copy)
@@ -90,7 +90,10 @@ svn_stream__set_is_buffered(svn_stream_t *stream,
svn_boolean_t
svn_stream__is_buffered(svn_stream_t *stream);
+apr_file_t *
+svn_stream__aprfile(svn_stream_t *stream);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 1494268)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -51,6 +51,7 @@
#include "private/svn_wc_private.h"
#include "private/svn_diff_private.h"
#include "private/svn_subr_private.h"
+#include "private/svn_io_private.h"
#include "svn_private_config.h"
@@ -809,13 +810,22 @@ diff_content_changed(svn_boolean_t *wrote_header,
/* We deal in streams, but svn_io_run_diff2() deals in file handles,
unfortunately, so we need to make these temporary files, and then
copy the contents to our stream. */
- SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
- svn_io_file_del_on_pool_cleanup,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL,
- svn_io_file_del_on_pool_cleanup,
- scratch_pool, scratch_pool));
+ outfile = svn_stream__aprfile(outstream);
+ if (!outfile)
+ SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
+ svn_io_file_del_on_pool_cleanup,
+ scratch_pool, scratch_pool));
+ else
+ outfilename = NULL;
+ errfile = svn_stream__aprfile(errstream);
+ if (!errfile)
+ SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL,
+ svn_io_file_del_on_pool_cleanup,
+ scratch_pool, scratch_pool));
+ else
+ errfilename = NULL;
+
SVN_ERR(svn_io_run_diff2(".",
diff_cmd_baton->options.for_external.argv,
diff_cmd_baton->options.for_external.argc,
@@ -824,20 +834,28 @@ diff_content_changed(svn_boolean_t *wrote_header,
&exitcode, outfile, errfile,
diff_cmd_baton->diff_cmd, scratch_pool));
- SVN_ERR(svn_io_file_close(outfile, scratch_pool));
- SVN_ERR(svn_io_file_close(errfile, scratch_pool));
+ if (outfilename)
+ SVN_ERR(svn_io_file_close(outfile, scratch_pool));
+ if (errfilename)
+ SVN_ERR(svn_io_file_close(errfile, scratch_pool));
/* Now, open and copy our files to our output streams. */
- SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream,
- scratch_pool),
+ if (outfilename)
+ {
+ SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream,
+ scratch_pool),
+ NULL, NULL, scratch_pool));
+ }
+ if (errfilename)
+ {
+ SVN_ERR(svn_stream_open_readonly(&stream, errfilename,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(errstream,
+ scratch_pool),
NULL, NULL, scratch_pool));
- SVN_ERR(svn_stream_open_readonly(&stream, errfilename,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(errstream,
- scratch_pool),
- NULL, NULL, scratch_pool));
+ }
/* We have a printed a diff for this path, mark it as visited. */
*wrote_header = TRUE;
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1494268)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -56,6 +56,7 @@ struct svn_stream_t {
svn_stream_mark_fn_t mark_fn;
svn_stream_seek_fn_t seek_fn;
svn_stream__is_buffered_fn_t is_buffered_fn;
+ apr_file_t *file; /* Maybe NULL */
};
@@ -81,6 +82,7 @@ svn_stream_create(void *baton, apr_pool_t *pool)
stream->mark_fn = NULL;
stream->seek_fn = NULL;
stream->is_buffered_fn = NULL;
+ stream->file = NULL;
return stream;
}
@@ -913,6 +915,7 @@ svn_stream_from_aprfile2(apr_file_t *file,
svn_stream_set_mark(stream, mark_handler_apr);
svn_stream_set_seek(stream, seek_handler_apr);
svn_stream__set_is_buffered(stream, is_buffered_handler_apr);
+ stream->file = file;
if (! disown)
svn_stream_set_close(stream, close_handler_apr);
@@ -920,6 +923,12 @@ svn_stream_from_aprfile2(apr_file_t *file,
return stream;
}
+apr_file_t *
+svn_stream__aprfile(svn_stream_t *stream)
+{
+ return stream->file;
+}
+
/* Compressed stream support */
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Daniel Shahaf <da...@elego.de>.
Michael Schlottke wrote on Wed, Jun 26, 2013 at 17:34:15 +0200:
>
> On Jun 21, 2013, at 15:23 , Philip Martin wrote:
> > Another user raised the issue
> >
> > http://subversion.tigris.org/issues/show_bug.cgi?id=4382
> >
> > Using '--diff-cmd colordiff' to get coloured output no longer works.
> >
> > Here's a solution that requires the user to mark the command as
> > requiring direct access. Log and patch:
> >
> > Allow the user to bypass the temporary spool file when invoking an
> > external diff command. This allows commands that expect to see
> > a terminal to work. The user adds the prefix 'svn:direct:' to the
> > command and Subversion passes the stream's file rather than creating
> > a spool file. So
> >
> > --diff-cmd foo
> >
> > runs foo with a spool file and
> >
> > --diff-cmd svn:direct:foo
> >
> > runs foo with the stream's file.
>
> I can confirm that your patch works for me (OS X Lion and Linux).
> Thanks for the quick fix! Keeping my fingers crossed that this will
> make it into the repository soon.
>
I believe http://svn.apache.org/r1497002 fixes this issue (without the
svn:direct: prefix), and is proposed for backport towards 1.8.1.
Daniel
> Michael
>
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Daniel Shahaf <da...@elego.de>.
Michael Schlottke wrote on Wed, Jun 26, 2013 at 17:34:15 +0200:
>
> On Jun 21, 2013, at 15:23 , Philip Martin wrote:
> > Another user raised the issue
> >
> > http://subversion.tigris.org/issues/show_bug.cgi?id=4382
> >
> > Using '--diff-cmd colordiff' to get coloured output no longer works.
> >
> > Here's a solution that requires the user to mark the command as
> > requiring direct access. Log and patch:
> >
> > Allow the user to bypass the temporary spool file when invoking an
> > external diff command. This allows commands that expect to see
> > a terminal to work. The user adds the prefix 'svn:direct:' to the
> > command and Subversion passes the stream's file rather than creating
> > a spool file. So
> >
> > --diff-cmd foo
> >
> > runs foo with a spool file and
> >
> > --diff-cmd svn:direct:foo
> >
> > runs foo with the stream's file.
>
> I can confirm that your patch works for me (OS X Lion and Linux).
> Thanks for the quick fix! Keeping my fingers crossed that this will
> make it into the repository soon.
>
I believe http://svn.apache.org/r1497002 fixes this issue (without the
svn:direct: prefix), and is proposed for backport towards 1.8.1.
Daniel
> Michael
>
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Michael Schlottke <m....@aia.rwth-aachen.de>.
On Jun 21, 2013, at 15:23 , Philip Martin wrote:
> Another user raised the issue
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=4382
>
> Using '--diff-cmd colordiff' to get coloured output no longer works.
>
> Here's a solution that requires the user to mark the command as
> requiring direct access. Log and patch:
>
> Allow the user to bypass the temporary spool file when invoking an
> external diff command. This allows commands that expect to see
> a terminal to work. The user adds the prefix 'svn:direct:' to the
> command and Subversion passes the stream's file rather than creating
> a spool file. So
>
> --diff-cmd foo
>
> runs foo with a spool file and
>
> --diff-cmd svn:direct:foo
>
> runs foo with the stream's file.
I can confirm that your patch works for me (OS X Lion and Linux). Thanks for the quick fix! Keeping my fingers crossed that this will make it into the repository soon.
Michael
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Michael Schlottke <m....@aia.rwth-aachen.de>.
On Jun 21, 2013, at 15:23 , Philip Martin wrote:
> Another user raised the issue
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=4382
>
> Using '--diff-cmd colordiff' to get coloured output no longer works.
>
> Here's a solution that requires the user to mark the command as
> requiring direct access. Log and patch:
>
> Allow the user to bypass the temporary spool file when invoking an
> external diff command. This allows commands that expect to see
> a terminal to work. The user adds the prefix 'svn:direct:' to the
> command and Subversion passes the stream's file rather than creating
> a spool file. So
>
> --diff-cmd foo
>
> runs foo with a spool file and
>
> --diff-cmd svn:direct:foo
>
> runs foo with the stream's file.
I can confirm that your patch works for me (OS X Lion and Linux). Thanks for the quick fix! Keeping my fingers crossed that this will make it into the repository soon.
Michael
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> It's also hard to fix 1.8, how do we pass the information into the
> client library without changing the API? Perhaps we could recognise a
> special part of the command name or a special external parameter, so
>
> --diff-cmd svn:interactive:myscript
Another user raised the issue
http://subversion.tigris.org/issues/show_bug.cgi?id=4382
Using '--diff-cmd colordiff' to get coloured output no longer works.
Here's a solution that requires the user to mark the command as
requiring direct access. Log and patch:
Allow the user to bypass the temporary spool file when invoking an
external diff command. This allows commands that expect to see
a terminal to work. The user adds the prefix 'svn:direct:' to the
command and Subversion passes the stream's file rather than creating
a spool file. So
--diff-cmd foo
runs foo with a spool file and
--diff-cmd svn:direct:foo
runs foo with the stream's file.
* subversion/include/private/svn_io_private.h
(svn_stream__aprfile, svn_io__file_for_command_stream): New.
* subversion/libsvn_subr/io.c
(svn_io__file_for_command_stream): New.
* subversion/libsvn_subr/stream.c
(struct svn_stream_t): Add file member.
(svn_stream_create, svn_stream_from_aprfile2): Set file member.
(svn_stream__aprfile): New.
* subversion/libsvn_client/diff.c
(diff_content_changed): Use svn_io__file_for_command_stream.
* subversion/svnlook/svnlook.c
(print_diff_tree): Use svn_io__file_for_command_stream.
Index: subversion/include/private/svn_io_private.h
===================================================================
--- subversion/include/private/svn_io_private.h (revision 1495378)
+++ subversion/include/private/svn_io_private.h (working copy)
@@ -90,7 +90,29 @@ svn_stream__set_is_buffered(svn_stream_t *stream,
svn_boolean_t
svn_stream__is_buffered(svn_stream_t *stream);
+/** Return the underlying file, if any, associated with the stream, or
+ * NULL if not available. Accessing the file bypasses the stream and
+ * should only be done when such bypass is acceptable.
+ */
+apr_file_t *
+svn_stream__aprfile(svn_stream_t *stream);
+/** Examine ORIG_COMMAND to determine whether it requests direct
+ * access to the file, if so then return STREAM's file in *FILE and set
+ * *FILENAME to NULL or return an error if the file is not available.
+ * Otherwise return a temporary file in *FILE and the filename in
+ * *FILENAME. Return the true command in * *TRUE_COMMAND.
+ */
+svn_error_t *
+svn_io__file_for_command_stream(apr_file_t **file,
+ const char **filename,
+ const char **true_command,
+ svn_stream_t *stream,
+ const char *orig_command,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 1495378)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -51,6 +51,7 @@
#include "private/svn_wc_private.h"
#include "private/svn_diff_private.h"
#include "private/svn_subr_private.h"
+#include "private/svn_io_private.h"
#include "svn_private_config.h"
@@ -793,6 +794,7 @@ diff_content_changed(svn_boolean_t *wrote_header,
const char *outfilename;
const char *errfilename;
svn_stream_t *stream;
+ const char *diff_cmd;
/* Print out the diff header. */
SVN_ERR(svn_stream_printf_from_utf8(outstream,
@@ -807,11 +809,13 @@ diff_content_changed(svn_boolean_t *wrote_header,
* ### a non-git compatible diff application.*/
/* We deal in streams, but svn_io_run_diff2() deals in file handles,
- unfortunately, so we need to make these temporary files, and then
+ so we need to make these temporary files, and then
copy the contents to our stream. */
- SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
- svn_io_file_del_on_pool_cleanup,
- scratch_pool, scratch_pool));
+ SVN_ERR(svn_io__file_for_command_stream(&outfile, &outfilename,
+ &diff_cmd, outstream,
+ diff_cmd_baton->diff_cmd,
+ scratch_pool, scratch_pool));
+
SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL,
svn_io_file_del_on_pool_cleanup,
scratch_pool, scratch_pool));
@@ -822,17 +826,21 @@ diff_content_changed(svn_boolean_t *wrote_header,
label1, label2,
tmpfile1, tmpfile2,
&exitcode, outfile, errfile,
- diff_cmd_baton->diff_cmd, scratch_pool));
+ diff_cmd, scratch_pool));
- SVN_ERR(svn_io_file_close(outfile, scratch_pool));
+ if (outfilename)
+ SVN_ERR(svn_io_file_close(outfile, scratch_pool));
SVN_ERR(svn_io_file_close(errfile, scratch_pool));
/* Now, open and copy our files to our output streams. */
- SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream,
- scratch_pool),
- NULL, NULL, scratch_pool));
+ if (outfilename)
+ {
+ SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream,
+ scratch_pool),
+ NULL, NULL, scratch_pool));
+ }
SVN_ERR(svn_stream_open_readonly(&stream, errfilename,
scratch_pool, scratch_pool));
SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(errstream,
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1495378)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -2854,7 +2854,44 @@ svn_io_run_cmd(const char *path,
return svn_io_wait_for_cmd(&cmd_proc, cmd, exitcode, exitwhy, pool);
}
+svn_error_t *
+svn_io__file_for_command_stream(apr_file_t **file,
+ const char **filename,
+ const char **true_command,
+ svn_stream_t *stream,
+ const char *orig_command,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ const char direct_magic[] = "svn:direct:";
+ svn_boolean_t direct_required = !strncmp(direct_magic, orig_command,
+ sizeof(direct_magic) - 1);
+
+ if (direct_required)
+ {
+ /* The user doesn't want a temporary file, probably because
+ they want some sort of terminal. We assume they don't
+ care that any output from the command may not be ordered
+ with the stream output. */
+ *file = svn_stream__aprfile(stream);
+ if (!*file)
+ return svn_error_createf(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
+ "stream does not support direct use");
+ *filename = NULL;
+ *true_command = apr_pstrdup(result_pool,
+ orig_command + sizeof(direct_magic) - 1);
+ }
+ else
+ {
+ SVN_ERR(svn_io_open_unique_file3(file, filename, NULL,
+ svn_io_file_del_on_pool_cleanup,
+ scratch_pool, scratch_pool));
+ *true_command = apr_pstrdup(result_pool, orig_command);
+ }
+ return SVN_NO_ERROR;
+}
+
svn_error_t *
svn_io_run_diff2(const char *dir,
const char *const *user_args,
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1495378)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -56,6 +56,7 @@ struct svn_stream_t {
svn_stream_mark_fn_t mark_fn;
svn_stream_seek_fn_t seek_fn;
svn_stream__is_buffered_fn_t is_buffered_fn;
+ apr_file_t *file; /* Maybe NULL */
};
@@ -81,6 +82,7 @@ svn_stream_create(void *baton, apr_pool_t *pool)
stream->mark_fn = NULL;
stream->seek_fn = NULL;
stream->is_buffered_fn = NULL;
+ stream->file = NULL;
return stream;
}
@@ -913,6 +915,7 @@ svn_stream_from_aprfile2(apr_file_t *file,
svn_stream_set_mark(stream, mark_handler_apr);
svn_stream_set_seek(stream, seek_handler_apr);
svn_stream__set_is_buffered(stream, is_buffered_handler_apr);
+ stream->file = file;
if (! disown)
svn_stream_set_close(stream, close_handler_apr);
@@ -920,6 +923,12 @@ svn_stream_from_aprfile2(apr_file_t *file,
return stream;
}
+apr_file_t *
+svn_stream__aprfile(svn_stream_t *stream)
+{
+ return stream->file;
+}
+
/* Compressed stream support */
Index: subversion/svnlook/svnlook.c
===================================================================
--- subversion/svnlook/svnlook.c (revision 1495378)
+++ subversion/svnlook/svnlook.c (working copy)
@@ -57,6 +57,7 @@
#include "private/svn_diff_private.h"
#include "private/svn_cmdline_private.h"
#include "private/svn_fspath.h"
+#include "private/svn_io_private.h"
#include "svn_private_config.h"
@@ -957,6 +958,7 @@ print_diff_tree(svn_stream_t *out_stream,
int exitcode;
const char *orig_label;
const char *new_label;
+ const char *diff_cmd;
diff_cmd_argv = NULL;
diff_cmd_argc = c->diff_options->nelts;
@@ -983,10 +985,11 @@ print_diff_tree(svn_stream_t *out_stream,
SVN_ERR(generate_label(&new_label, root, path, pool));
/* We deal in streams, but svn_io_run_diff2() deals in file
- handles, unfortunately, so we need to make these temporary
+ handles, so we need to make these temporary
files, and then copy the contents to our stream. */
- SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
- svn_io_file_del_on_pool_cleanup, pool, pool));
+ SVN_ERR(svn_io__file_for_command_stream(&outfile, &outfilename,
+ &diff_cmd, out_stream,
+ c->diff_cmd, pool, pool));
SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL,
svn_io_file_del_on_pool_cleanup, pool, pool));
@@ -996,18 +999,22 @@ print_diff_tree(svn_stream_t *out_stream,
orig_label, new_label,
orig_path, new_path,
&exitcode, outfile, errfile,
- c->diff_cmd, pool));
+ diff_cmd, pool));
- SVN_ERR(svn_io_file_close(outfile, pool));
+ if (outfilename)
+ SVN_ERR(svn_io_file_close(outfile, pool));
SVN_ERR(svn_io_file_close(errfile, pool));
/* Now, open and copy our files to our output streams. */
+ if (outfilename)
+ {
+ SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
+ pool, pool));
+ SVN_ERR(svn_stream_copy3(stream,
+ svn_stream_disown(out_stream, pool),
+ NULL, NULL, pool));
+ }
SVN_ERR(svn_stream_for_stderr(&err_stream, pool));
- SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
- pool, pool));
- SVN_ERR(svn_stream_copy3(stream,
- svn_stream_disown(out_stream, pool),
- NULL, NULL, pool));
SVN_ERR(svn_stream_open_readonly(&stream, errfilename,
pool, pool));
SVN_ERR(svn_stream_copy3(stream,
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> It's also hard to fix 1.8, how do we pass the information into the
> client library without changing the API? Perhaps we could recognise a
> special part of the command name or a special external parameter, so
>
> --diff-cmd svn:interactive:myscript
Another user raised the issue
http://subversion.tigris.org/issues/show_bug.cgi?id=4382
Using '--diff-cmd colordiff' to get coloured output no longer works.
Here's a solution that requires the user to mark the command as
requiring direct access. Log and patch:
Allow the user to bypass the temporary spool file when invoking an
external diff command. This allows commands that expect to see
a terminal to work. The user adds the prefix 'svn:direct:' to the
command and Subversion passes the stream's file rather than creating
a spool file. So
--diff-cmd foo
runs foo with a spool file and
--diff-cmd svn:direct:foo
runs foo with the stream's file.
* subversion/include/private/svn_io_private.h
(svn_stream__aprfile, svn_io__file_for_command_stream): New.
* subversion/libsvn_subr/io.c
(svn_io__file_for_command_stream): New.
* subversion/libsvn_subr/stream.c
(struct svn_stream_t): Add file member.
(svn_stream_create, svn_stream_from_aprfile2): Set file member.
(svn_stream__aprfile): New.
* subversion/libsvn_client/diff.c
(diff_content_changed): Use svn_io__file_for_command_stream.
* subversion/svnlook/svnlook.c
(print_diff_tree): Use svn_io__file_for_command_stream.
Index: subversion/include/private/svn_io_private.h
===================================================================
--- subversion/include/private/svn_io_private.h (revision 1495378)
+++ subversion/include/private/svn_io_private.h (working copy)
@@ -90,7 +90,29 @@ svn_stream__set_is_buffered(svn_stream_t *stream,
svn_boolean_t
svn_stream__is_buffered(svn_stream_t *stream);
+/** Return the underlying file, if any, associated with the stream, or
+ * NULL if not available. Accessing the file bypasses the stream and
+ * should only be done when such bypass is acceptable.
+ */
+apr_file_t *
+svn_stream__aprfile(svn_stream_t *stream);
+/** Examine ORIG_COMMAND to determine whether it requests direct
+ * access to the file, if so then return STREAM's file in *FILE and set
+ * *FILENAME to NULL or return an error if the file is not available.
+ * Otherwise return a temporary file in *FILE and the filename in
+ * *FILENAME. Return the true command in * *TRUE_COMMAND.
+ */
+svn_error_t *
+svn_io__file_for_command_stream(apr_file_t **file,
+ const char **filename,
+ const char **true_command,
+ svn_stream_t *stream,
+ const char *orig_command,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 1495378)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -51,6 +51,7 @@
#include "private/svn_wc_private.h"
#include "private/svn_diff_private.h"
#include "private/svn_subr_private.h"
+#include "private/svn_io_private.h"
#include "svn_private_config.h"
@@ -793,6 +794,7 @@ diff_content_changed(svn_boolean_t *wrote_header,
const char *outfilename;
const char *errfilename;
svn_stream_t *stream;
+ const char *diff_cmd;
/* Print out the diff header. */
SVN_ERR(svn_stream_printf_from_utf8(outstream,
@@ -807,11 +809,13 @@ diff_content_changed(svn_boolean_t *wrote_header,
* ### a non-git compatible diff application.*/
/* We deal in streams, but svn_io_run_diff2() deals in file handles,
- unfortunately, so we need to make these temporary files, and then
+ so we need to make these temporary files, and then
copy the contents to our stream. */
- SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
- svn_io_file_del_on_pool_cleanup,
- scratch_pool, scratch_pool));
+ SVN_ERR(svn_io__file_for_command_stream(&outfile, &outfilename,
+ &diff_cmd, outstream,
+ diff_cmd_baton->diff_cmd,
+ scratch_pool, scratch_pool));
+
SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL,
svn_io_file_del_on_pool_cleanup,
scratch_pool, scratch_pool));
@@ -822,17 +826,21 @@ diff_content_changed(svn_boolean_t *wrote_header,
label1, label2,
tmpfile1, tmpfile2,
&exitcode, outfile, errfile,
- diff_cmd_baton->diff_cmd, scratch_pool));
+ diff_cmd, scratch_pool));
- SVN_ERR(svn_io_file_close(outfile, scratch_pool));
+ if (outfilename)
+ SVN_ERR(svn_io_file_close(outfile, scratch_pool));
SVN_ERR(svn_io_file_close(errfile, scratch_pool));
/* Now, open and copy our files to our output streams. */
- SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream,
- scratch_pool),
- NULL, NULL, scratch_pool));
+ if (outfilename)
+ {
+ SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream,
+ scratch_pool),
+ NULL, NULL, scratch_pool));
+ }
SVN_ERR(svn_stream_open_readonly(&stream, errfilename,
scratch_pool, scratch_pool));
SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(errstream,
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1495378)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -2854,7 +2854,44 @@ svn_io_run_cmd(const char *path,
return svn_io_wait_for_cmd(&cmd_proc, cmd, exitcode, exitwhy, pool);
}
+svn_error_t *
+svn_io__file_for_command_stream(apr_file_t **file,
+ const char **filename,
+ const char **true_command,
+ svn_stream_t *stream,
+ const char *orig_command,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ const char direct_magic[] = "svn:direct:";
+ svn_boolean_t direct_required = !strncmp(direct_magic, orig_command,
+ sizeof(direct_magic) - 1);
+
+ if (direct_required)
+ {
+ /* The user doesn't want a temporary file, probably because
+ they want some sort of terminal. We assume they don't
+ care that any output from the command may not be ordered
+ with the stream output. */
+ *file = svn_stream__aprfile(stream);
+ if (!*file)
+ return svn_error_createf(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
+ "stream does not support direct use");
+ *filename = NULL;
+ *true_command = apr_pstrdup(result_pool,
+ orig_command + sizeof(direct_magic) - 1);
+ }
+ else
+ {
+ SVN_ERR(svn_io_open_unique_file3(file, filename, NULL,
+ svn_io_file_del_on_pool_cleanup,
+ scratch_pool, scratch_pool));
+ *true_command = apr_pstrdup(result_pool, orig_command);
+ }
+ return SVN_NO_ERROR;
+}
+
svn_error_t *
svn_io_run_diff2(const char *dir,
const char *const *user_args,
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1495378)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -56,6 +56,7 @@ struct svn_stream_t {
svn_stream_mark_fn_t mark_fn;
svn_stream_seek_fn_t seek_fn;
svn_stream__is_buffered_fn_t is_buffered_fn;
+ apr_file_t *file; /* Maybe NULL */
};
@@ -81,6 +82,7 @@ svn_stream_create(void *baton, apr_pool_t *pool)
stream->mark_fn = NULL;
stream->seek_fn = NULL;
stream->is_buffered_fn = NULL;
+ stream->file = NULL;
return stream;
}
@@ -913,6 +915,7 @@ svn_stream_from_aprfile2(apr_file_t *file,
svn_stream_set_mark(stream, mark_handler_apr);
svn_stream_set_seek(stream, seek_handler_apr);
svn_stream__set_is_buffered(stream, is_buffered_handler_apr);
+ stream->file = file;
if (! disown)
svn_stream_set_close(stream, close_handler_apr);
@@ -920,6 +923,12 @@ svn_stream_from_aprfile2(apr_file_t *file,
return stream;
}
+apr_file_t *
+svn_stream__aprfile(svn_stream_t *stream)
+{
+ return stream->file;
+}
+
/* Compressed stream support */
Index: subversion/svnlook/svnlook.c
===================================================================
--- subversion/svnlook/svnlook.c (revision 1495378)
+++ subversion/svnlook/svnlook.c (working copy)
@@ -57,6 +57,7 @@
#include "private/svn_diff_private.h"
#include "private/svn_cmdline_private.h"
#include "private/svn_fspath.h"
+#include "private/svn_io_private.h"
#include "svn_private_config.h"
@@ -957,6 +958,7 @@ print_diff_tree(svn_stream_t *out_stream,
int exitcode;
const char *orig_label;
const char *new_label;
+ const char *diff_cmd;
diff_cmd_argv = NULL;
diff_cmd_argc = c->diff_options->nelts;
@@ -983,10 +985,11 @@ print_diff_tree(svn_stream_t *out_stream,
SVN_ERR(generate_label(&new_label, root, path, pool));
/* We deal in streams, but svn_io_run_diff2() deals in file
- handles, unfortunately, so we need to make these temporary
+ handles, so we need to make these temporary
files, and then copy the contents to our stream. */
- SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
- svn_io_file_del_on_pool_cleanup, pool, pool));
+ SVN_ERR(svn_io__file_for_command_stream(&outfile, &outfilename,
+ &diff_cmd, out_stream,
+ c->diff_cmd, pool, pool));
SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL,
svn_io_file_del_on_pool_cleanup, pool, pool));
@@ -996,18 +999,22 @@ print_diff_tree(svn_stream_t *out_stream,
orig_label, new_label,
orig_path, new_path,
&exitcode, outfile, errfile,
- c->diff_cmd, pool));
+ diff_cmd, pool));
- SVN_ERR(svn_io_file_close(outfile, pool));
+ if (outfilename)
+ SVN_ERR(svn_io_file_close(outfile, pool));
SVN_ERR(svn_io_file_close(errfile, pool));
/* Now, open and copy our files to our output streams. */
+ if (outfilename)
+ {
+ SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
+ pool, pool));
+ SVN_ERR(svn_stream_copy3(stream,
+ svn_stream_disown(out_stream, pool),
+ NULL, NULL, pool));
+ }
SVN_ERR(svn_stream_for_stderr(&err_stream, pool));
- SVN_ERR(svn_stream_open_readonly(&stream, outfilename,
- pool, pool));
- SVN_ERR(svn_stream_copy3(stream,
- svn_stream_disown(out_stream, pool),
- NULL, NULL, pool));
SVN_ERR(svn_stream_open_readonly(&stream, errfilename,
pool, pool));
SVN_ERR(svn_stream_copy3(stream,
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Ben Reser <be...@reser.org> writes:
> I don't imagine it'd take very long at all to implement but the
> problem of course is that we really should think carefully how we go
> about doing this. If we can detect this at runtime we probably
> should.
I don't see how Subversion can determine that one script needs a
terminal while another can use a file, only the user knows that.
It's also hard to fix 1.8, how do we pass the information into the
client library without changing the API? Perhaps we could recognise a
special part of the command name or a special external parameter, so
--diff-cmd svn:interactive:myscript
or
--diff-cmd myscript -x svn:interactive
gets a terminal while
--diff-cmd myscript
gets a file.
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Michael Schlottke <m....@aia.rwth-aachen.de>.
On Jun 19, 2013, at 09:56 , Ben Reser wrote:
> On Wed, Jun 19, 2013 at 9:11 AM, Michael Schlottke
> <m....@aia.rwth-aachen.de> wrote:
>> Do you have an idea of how hard this is to achieve, or how long it would
>> take to create a patch? I'd be happy to volunteer as a tester…
>> Or do you know of an interim hack that I could use until it is properly
>> fixed?
>
> I don't imagine it'd take very long at all to implement but the
> problem of course is that we really should think carefully how we go
> about doing this. If we can detect this at runtime we probably
> should.
I fully agree that this should be done right, and preferably without adding complexity through additional flags.
However, for now I'd prefer a quick-n-dirty solution rather than not being able to upgrade at all. I know this is
asking for quite something, especially since I don't know how to do it myself :-/
> I'm not sure what platform you're on but using the GUI version of
> vimdiff would get around this for you in the meantime.
Thanks for the tip! Unfortunately, gvim is not an option unfortunately, since most of the machines do not have a proper version
of gvim installed.
Michael
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Ben Reser <be...@reser.org> writes:
> I don't imagine it'd take very long at all to implement but the
> problem of course is that we really should think carefully how we go
> about doing this. If we can detect this at runtime we probably
> should.
I don't see how Subversion can determine that one script needs a
terminal while another can use a file, only the user knows that.
It's also hard to fix 1.8, how do we pass the information into the
client library without changing the API? Perhaps we could recognise a
special part of the command name or a special external parameter, so
--diff-cmd svn:interactive:myscript
or
--diff-cmd myscript -x svn:interactive
gets a terminal while
--diff-cmd myscript
gets a file.
--
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Ben Reser <be...@reser.org>.
On Wed, Jun 19, 2013 at 9:11 AM, Michael Schlottke
<m....@aia.rwth-aachen.de> wrote:
> Do you have an idea of how hard this is to achieve, or how long it would
> take to create a patch? I'd be happy to volunteer as a tester…
> Or do you know of an interim hack that I could use until it is properly
> fixed?
I don't imagine it'd take very long at all to implement but the
problem of course is that we really should think carefully how we go
about doing this. If we can detect this at runtime we probably
should.
I'm not sure what platform you're on but using the GUI version of
vimdiff would get around this for you in the meantime. You'll end up
with one vimdiff window per file. On the Mac for instance all I did
was symlink mvimdiff to mvim (which is essentially equivalent to gvim
on other platforms) and then change the wrapper to use that instead of
vimdiff. I realize it's not ideal but it should give you almost the
same functionality that you had before.
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Ben Reser <be...@reser.org>.
On Wed, Jun 19, 2013 at 9:11 AM, Michael Schlottke
<m....@aia.rwth-aachen.de> wrote:
> Do you have an idea of how hard this is to achieve, or how long it would
> take to create a patch? I'd be happy to volunteer as a tester…
> Or do you know of an interim hack that I could use until it is properly
> fixed?
I don't imagine it'd take very long at all to implement but the
problem of course is that we really should think carefully how we go
about doing this. If we can detect this at runtime we probably
should.
I'm not sure what platform you're on but using the GUI version of
vimdiff would get around this for you in the meantime. You'll end up
with one vimdiff window per file. On the Mac for instance all I did
was symlink mvimdiff to mvim (which is essentially equivalent to gvim
on other platforms) and then change the wrapper to use that instead of
vimdiff. I realize it's not ideal but it should give you almost the
same functionality that you had before.
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Michael Schlottke <m....@aia.rwth-aachen.de>.
On Jun 19, 2013, at 01:58 , Philip Martin wrote:
> [cc to dev]
>
> Michael Schlottke <m....@aia.rwth-aachen.de> writes:
>
>> I just installed svn 1.8 on our cluster. Before, we used svn 1.7.9 and
>> a little vimdiff wrapper (taken, with a few changes, from
>> http://svnbook.red-bean.com/nightly/en/svn.advanced.externaldifftools.html#svn.advanced.externaldifftools.diff),
>> which worked like a charm when called as "svn diff
>> --diff-cmd=diffwrap.py filename". However, with svn 1.8 something
>> seems to have changed:
>>
>> The only lines I see after executing the command above are
>>
>>> Index: filename
>>> ===================================================================
>>
>> and the terminal hangs. When I enter ":qa!", I get the following line
>>
>>> Vim: Warning: Output is not to a terminal
>>
>> and I am back in the terminal. I have not changed anything in the
>> wrapper script, and indeed, when I manually use it with our old svn
>> version (1.7.9), it still works. Does anyone have an idea what has
>> changed in the way the diff-cmd is invoked? And, more importantly, how
>> I can change the vimdiff wrapper so it works again?
>
> I invoked vimdiff using a diff-cmd of:
>
> #!/bin/sh
> vimdiff $6 $7
That's the exact same command I used.
> this works with 1.7 but fails as you describe in 1.8.
>
> The cause is the conversion of diff to the stream API. The code in
> libsvn_client/diff.c:diff_content_changed now gets a Subversion stream
> rather than an APR file for output so it does:
>
> /* We deal in streams, but svn_io_run_diff2() deals in file handles,
> unfortunately, so we need to make these temporary files, and then
> copy the contents to our stream. */
> SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
> svn_io_file_del_on_pool_cleanup,
> scratch_pool, scratch_pool));
>
> and this use of a temporary file prevents the use of an external diff
> that expects a terminal.
>
> The only way I see to fix this is to stop using the stream API when the
> external diff command wants a terminal. I don't think it is possible to
> do this automatically, perhaps we need an --interactive-diff option?
Do you have an idea of how hard this is to achieve, or how long it would take to create a patch? I'd be happy to volunteer as a tester…
Or do you know of an interim hack that I could use until it is properly fixed?
We'd really love to use svn 1.8 (especially with the new merging capabilities), but this is a major hindrance.
Michael
--
Michael Schlottke
SimLab Highly Scalable Fluids & Solids Engineering
Jülich Aachen Research Alliance (JARA-HPC)
RWTH Aachen University
Wüllnerstraße 5a
52062 Aachen
Germany
Phone: +49 (241) 80 95188
Fax: +49 (241) 80 92257
Mail: m.schlottke@aia.rwth-aachen.de
Web: http://www.jara.org/jara-hpc
Re: vimdiff wrapper for diff-cmd not working with 1.8
Posted by Michael Schlottke <m....@aia.rwth-aachen.de>.
On Jun 19, 2013, at 01:58 , Philip Martin wrote:
> [cc to dev]
>
> Michael Schlottke <m....@aia.rwth-aachen.de> writes:
>
>> I just installed svn 1.8 on our cluster. Before, we used svn 1.7.9 and
>> a little vimdiff wrapper (taken, with a few changes, from
>> http://svnbook.red-bean.com/nightly/en/svn.advanced.externaldifftools.html#svn.advanced.externaldifftools.diff),
>> which worked like a charm when called as "svn diff
>> --diff-cmd=diffwrap.py filename". However, with svn 1.8 something
>> seems to have changed:
>>
>> The only lines I see after executing the command above are
>>
>>> Index: filename
>>> ===================================================================
>>
>> and the terminal hangs. When I enter ":qa!", I get the following line
>>
>>> Vim: Warning: Output is not to a terminal
>>
>> and I am back in the terminal. I have not changed anything in the
>> wrapper script, and indeed, when I manually use it with our old svn
>> version (1.7.9), it still works. Does anyone have an idea what has
>> changed in the way the diff-cmd is invoked? And, more importantly, how
>> I can change the vimdiff wrapper so it works again?
>
> I invoked vimdiff using a diff-cmd of:
>
> #!/bin/sh
> vimdiff $6 $7
That's the exact same command I used.
> this works with 1.7 but fails as you describe in 1.8.
>
> The cause is the conversion of diff to the stream API. The code in
> libsvn_client/diff.c:diff_content_changed now gets a Subversion stream
> rather than an APR file for output so it does:
>
> /* We deal in streams, but svn_io_run_diff2() deals in file handles,
> unfortunately, so we need to make these temporary files, and then
> copy the contents to our stream. */
> SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL,
> svn_io_file_del_on_pool_cleanup,
> scratch_pool, scratch_pool));
>
> and this use of a temporary file prevents the use of an external diff
> that expects a terminal.
>
> The only way I see to fix this is to stop using the stream API when the
> external diff command wants a terminal. I don't think it is possible to
> do this automatically, perhaps we need an --interactive-diff option?
Do you have an idea of how hard this is to achieve, or how long it would take to create a patch? I'd be happy to volunteer as a tester…
Or do you know of an interim hack that I could use until it is properly fixed?
We'd really love to use svn 1.8 (especially with the new merging capabilities), but this is a major hindrance.
Michael
--
Michael Schlottke
SimLab Highly Scalable Fluids & Solids Engineering
Jülich Aachen Research Alliance (JARA-HPC)
RWTH Aachen University
Wüllnerstraße 5a
52062 Aachen
Germany
Phone: +49 (241) 80 95188
Fax: +49 (241) 80 92257
Mail: m.schlottke@aia.rwth-aachen.de
Web: http://www.jara.org/jara-hpc