In looking over the client diff APIs, I noticed the output parameters are file handles, not streams. This feels...odd to me, so I hacked together the attached patch which updates the APIs to use output streams. It isn't ready to commit just yet, but before doing the backward compat dance (rev'ing an API before it's even released, ugh), I thought I'd post it here for comments or concerns first.
The one gotcha is that running an external diff command still requires files, so this patch creates temporary ones and then copies them to the stream. Right now, this is a limitation of our own APIs, but fundamentally it's because APR wants an apr_file_t to plug into the stdout and stderr of processes it launches. Ideally, we'd find some way of mimicking an apr_file_t with a stream, but that feels like Future Work. Incidentally, one motivation for having streams instead of files is compressed pristines. At some point, I'd like to use streams as *input* to things like diff and merge, which is a prerequisite for storing stuff in compressed form and uncompressing it via a stream. (If that doesn't make much sense, it's 'cause I'm still working out the details in my head.) -Hyrum -- uberSVN: Apache Subversion Made Easy http://www.uberSVN.com/
Index: subversion/include/svn_client.h =================================================================== --- subversion/include/svn_client.h (revision 1170202) +++ subversion/include/svn_client.h (working copy) @@ -2831,8 +2831,8 @@ svn_client_diff5(const apr_array_header_t *diff_op svn_boolean_t ignore_content_type, svn_boolean_t use_git_diff_format, const char *header_encoding, - apr_file_t *outfile, - apr_file_t *errfile, + svn_stream_t *outstream, + svn_stream_t *errstream, const apr_array_header_t *changelists, svn_client_ctx_t *ctx, apr_pool_t *pool); @@ -2962,8 +2962,8 @@ svn_client_diff_peg5(const apr_array_header_t *dif svn_boolean_t ignore_content_type, svn_boolean_t use_git_diff_format, const char *header_encoding, - apr_file_t *outfile, - apr_file_t *errfile, + svn_stream_t *outstream, + svn_stream_t *errstream, const apr_array_header_t *changelists, svn_client_ctx_t *ctx, apr_pool_t *pool); Index: subversion/svn/diff-cmd.c =================================================================== --- subversion/svn/diff-cmd.c (revision 1170202) +++ subversion/svn/diff-cmd.c (working copy) @@ -166,8 +166,8 @@ svn_cl__diff(apr_getopt_t *os, svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx; apr_array_header_t *options; apr_array_header_t *targets; - apr_file_t *outfile, *errfile; - apr_status_t status; + svn_stream_t *outstream; + svn_stream_t *errstream; const char *old_target, *new_target; apr_pool_t *iterpool; svn_boolean_t pegged_diff = FALSE; @@ -180,12 +180,10 @@ svn_cl__diff(apr_getopt_t *os, else options = NULL; - /* Get an apr_file_t representing stdout and stderr, which is where + /* Get an svn_stream_t representing stdout and stderr, which is where we'll have the external 'diff' program print to. */ - if ((status = apr_file_open_stdout(&outfile, pool))) - return svn_error_wrap_apr(status, _("Can't open stdout")); - if ((status = apr_file_open_stderr(&errfile, pool))) - return svn_error_wrap_apr(status, _("Can't open stderr")); + SVN_ERR(svn_stream_for_stdout(&outstream, pool)); + SVN_ERR(svn_stream_for_stderr(&errstream, pool)); if (opt_state->xml) { @@ -367,8 +365,7 @@ svn_cl__diff(apr_getopt_t *os, opt_state->force, opt_state->use_git_diff_format, svn_cmdline_output_encoding(pool), - outfile, - errfile, + outstream, errstream, opt_state->changelists, ctx, iterpool)); } @@ -412,8 +409,7 @@ svn_cl__diff(apr_getopt_t *os, opt_state->force, opt_state->use_git_diff_format, svn_cmdline_output_encoding(pool), - outfile, - errfile, + outstream, errstream, opt_state->changelists, ctx, iterpool)); } Index: subversion/libsvn_client/deprecated.c =================================================================== --- subversion/libsvn_client/deprecated.c (revision 1170202) +++ subversion/libsvn_client/deprecated.c (working copy) @@ -854,11 +854,14 @@ svn_client_diff4(const apr_array_header_t *options svn_client_ctx_t *ctx, apr_pool_t *pool) { + svn_stream_t *outstream = svn_stream_from_aprfile2(outfile, TRUE, pool); + svn_stream_t *errstream = svn_stream_from_aprfile2(errfile, TRUE, pool); + return svn_client_diff5(options, path1, revision1, path2, revision2, relative_to_dir, depth, ignore_ancestry, no_diff_deleted, FALSE, ignore_content_type, FALSE, header_encoding, - outfile, errfile, changelists, ctx, pool); + outstream, errstream, changelists, ctx, pool); } svn_error_t * @@ -943,6 +946,9 @@ svn_client_diff_peg4(const apr_array_header_t *opt svn_client_ctx_t *ctx, apr_pool_t *pool) { + svn_stream_t *outstream = svn_stream_from_aprfile2(outfile, TRUE, pool); + svn_stream_t *errstream = svn_stream_from_aprfile2(errfile, TRUE, pool); + return svn_client_diff_peg5(options, path, peg_revision, @@ -956,8 +962,8 @@ svn_client_diff_peg4(const apr_array_header_t *opt ignore_content_type, FALSE, header_encoding, - outfile, - errfile, + outstream, + errstream, changelists, ctx, pool); Index: subversion/libsvn_client/diff.c =================================================================== --- subversion/libsvn_client/diff.c (revision 1170205) +++ subversion/libsvn_client/diff.c (working copy) @@ -69,32 +69,7 @@ static const char under_string[] = /* Utilities */ -/* Wrapper for apr_file_printf(), which see. FORMAT is a utf8-encoded - string after it is formatted, so this function can convert it to - ENCODING before printing. */ -static svn_error_t * -file_printf_from_utf8(apr_file_t *fptr, const char *encoding, - const char *format, ...) - __attribute__ ((format(printf, 3, 4))); -static svn_error_t * -file_printf_from_utf8(apr_file_t *fptr, const char *encoding, - const char *format, ...) -{ - va_list ap; - const char *buf, *buf_apr; - va_start(ap, format); - buf = apr_pvsprintf(apr_file_pool_get(fptr), format, ap); - va_end(ap); - - SVN_ERR(svn_utf_cstring_from_utf8_ex2(&buf_apr, buf, encoding, - apr_file_pool_get(fptr))); - - return svn_io_file_write_full(fptr, buf_apr, strlen(buf_apr), - NULL, apr_file_pool_get(fptr)); -} - - /* A helper function for display_prop_diffs. Output the differences between the mergeinfo stored in ORIG_MERGEINFO_VAL and NEW_MERGEINFO_VAL in a human-readable form to FILE, using ENCODING. Use POOL for temporary @@ -103,7 +78,7 @@ static svn_error_t * display_mergeinfo_diff(const char *old_mergeinfo_val, const char *new_mergeinfo_val, const char *encoding, - apr_file_t *file, + svn_stream_t *outstream, apr_pool_t *pool) { apr_hash_t *old_mergeinfo_hash, *new_mergeinfo_hash, *added, *deleted; @@ -132,10 +107,10 @@ display_mergeinfo_diff(const char *old_mergeinfo_v SVN_ERR(svn_rangelist_to_string(&merge_revstr, merge_revarray, pool)); - SVN_ERR(file_printf_from_utf8(file, encoding, - _(" Reverse-merged %s:r%s%s"), - from_path, merge_revstr->data, - APR_EOL_STR)); + SVN_ERR(svn_stream_printf_from_utf8(outstream, encoding, pool, + _(" Reverse-merged %s:r%s%s"), + from_path, merge_revstr->data, + APR_EOL_STR)); } for (hi = apr_hash_first(pool, added); @@ -147,10 +122,10 @@ display_mergeinfo_diff(const char *old_mergeinfo_v SVN_ERR(svn_rangelist_to_string(&merge_revstr, merge_revarray, pool)); - SVN_ERR(file_printf_from_utf8(file, encoding, - _(" Merged %s:r%s%s"), - from_path, merge_revstr->data, - APR_EOL_STR)); + SVN_ERR(svn_stream_printf_from_utf8(outstream, encoding, pool, + _(" Merged %s:r%s%s"), + from_path, merge_revstr->data, + APR_EOL_STR)); } return SVN_NO_ERROR; @@ -528,7 +503,7 @@ print_git_diff_header(svn_stream_t *os, } /* A helper func that writes out verbal descriptions of property diffs - to FILE. Of course, the apr_file_t will probably be the 'outfile' + to FILE. Of course, OUTSTREAM will probably be whatever was passed to svn_client_diff5, which is probably stdout. ### FIXME needs proper docstring @@ -549,7 +524,7 @@ display_prop_diffs(const apr_array_header_t *propc svn_revnum_t rev1, svn_revnum_t rev2, const char *encoding, - apr_file_t *file, + svn_stream_t *outstream, const char *relative_to_dir, svn_boolean_t show_diff_header, svn_boolean_t use_git_diff_format, @@ -596,39 +571,33 @@ display_prop_diffs(const apr_array_header_t *propc /* ### Should we show the paths in platform specific format, * ### diff_content_changed() does not! */ - SVN_ERR(file_printf_from_utf8(file, encoding, - "Index: %s" APR_EOL_STR - "%s" APR_EOL_STR, - path, equal_string)); + SVN_ERR(svn_stream_printf_from_utf8(outstream, encoding, pool, + "Index: %s" APR_EOL_STR + "%s" APR_EOL_STR, + path, equal_string)); if (use_git_diff_format) - { - svn_stream_t *os; + SVN_ERR(print_git_diff_header(outstream, &label1, &label2, + svn_diff_op_modified, + path1, path2, rev1, rev2, NULL, + SVN_INVALID_REVNUM, + encoding, pool)); - os = svn_stream_from_aprfile2(file, TRUE, pool); - SVN_ERR(print_git_diff_header(os, &label1, &label2, - svn_diff_op_modified, - path1, path2, rev1, rev2, NULL, - SVN_INVALID_REVNUM, - encoding, pool)); - SVN_ERR(svn_stream_close(os)); - } - - SVN_ERR(file_printf_from_utf8(file, encoding, + SVN_ERR(svn_stream_printf_from_utf8(outstream, encoding, pool, "--- %s" APR_EOL_STR "+++ %s" APR_EOL_STR, label1, label2)); } - SVN_ERR(file_printf_from_utf8(file, encoding, - _("%sProperty changes on: %s%s"), - APR_EOL_STR, - use_git_diff_format ? path1 : path, - APR_EOL_STR)); + SVN_ERR(svn_stream_printf_from_utf8(outstream, encoding, pool, + _("%sProperty changes on: %s%s"), + APR_EOL_STR, + use_git_diff_format ? path1 : path, + APR_EOL_STR)); - SVN_ERR(file_printf_from_utf8(file, encoding, "%s" APR_EOL_STR, - under_string)); + SVN_ERR(svn_stream_printf_from_utf8(outstream, encoding, pool, + "%s" APR_EOL_STR, under_string)); iterpool = svn_pool_create(pool); for (i = 0; i < propchanges->nelts; i++) @@ -659,15 +628,16 @@ display_prop_diffs(const apr_array_header_t *propc action = "Deleted"; else action = "Modified"; - SVN_ERR(file_printf_from_utf8(file, encoding, "%s: %s%s", action, - propchange->name, APR_EOL_STR)); + SVN_ERR(svn_stream_printf_from_utf8(outstream, encoding, iterpool, + "%s: %s%s", action, + propchange->name, APR_EOL_STR)); if (strcmp(propchange->name, SVN_PROP_MERGEINFO) == 0) { const char *orig = original_value ? original_value->data : NULL; const char *val = propchange->value ? propchange->value->data : NULL; svn_error_t *err = display_mergeinfo_diff(orig, val, encoding, - file, iterpool); + outstream, iterpool); /* Issue #3896: If we can't pretty-print mergeinfo differences because invalid mergeinfo is present, then don't let the diff @@ -684,7 +654,6 @@ display_prop_diffs(const apr_array_header_t *propc } { - svn_stream_t *os = svn_stream_from_aprfile2(file, TRUE, iterpool); svn_diff_t *diff; svn_diff_file_options_t options = { 0 }; const svn_string_t *tmp; @@ -713,13 +682,13 @@ display_prop_diffs(const apr_array_header_t *propc * UNIX patch could apply the property diff to, so we use "##" * instead of "@@" as the default hunk delimiter for property diffs. * We also supress the diff header. */ - SVN_ERR(svn_diff_mem_string_output_unified2(os, diff, FALSE, "##", + SVN_ERR(svn_diff_mem_string_output_unified2(outstream, diff, FALSE, + "##", svn_dirent_local_style(path, iterpool), svn_dirent_local_style(path, iterpool), encoding, orig, val, iterpool)); - SVN_ERR(svn_stream_close(os)); } } @@ -753,8 +722,8 @@ struct diff_cmd_baton { } options; apr_pool_t *pool; - apr_file_t *outfile; - apr_file_t *errfile; + svn_stream_t *outstream; + svn_stream_t *errstream; const char *header_encoding; @@ -846,7 +815,7 @@ diff_props_changed(svn_wc_notify_state_t *state, diff_cmd_baton->revnum1, diff_cmd_baton->revnum2, diff_cmd_baton->header_encoding, - diff_cmd_baton->outfile, + diff_cmd_baton->outstream, diff_cmd_baton->relative_to_dir, show_diff_header, diff_cmd_baton->use_git_diff_format, @@ -915,17 +884,14 @@ diff_content_changed(const char *path, { int exitcode; apr_pool_t *subpool = svn_pool_create(diff_cmd_baton->pool); - svn_stream_t *os; const char *rel_to_dir = diff_cmd_baton->relative_to_dir; - apr_file_t *errfile = diff_cmd_baton->errfile; + svn_stream_t *errstream = diff_cmd_baton->errstream; + svn_stream_t *outstream = diff_cmd_baton->outstream; const char *label1, *label2; svn_boolean_t mt1_binary = FALSE, mt2_binary = FALSE; const char *path1 = diff_cmd_baton->orig_path_1; const char *path2 = diff_cmd_baton->orig_path_2; - /* Get a stream from our output file. */ - os = svn_stream_from_aprfile2(diff_cmd_baton->outfile, TRUE, subpool); - /* Generate the diff headers. */ SVN_ERR(adjust_paths_for_diff_labels(&path, &path1, &path2, rel_to_dir, subpool)); @@ -944,35 +910,35 @@ diff_content_changed(const char *path, if (! diff_cmd_baton->force_binary && (mt1_binary || mt2_binary)) { /* Print out the diff header. */ - SVN_ERR(svn_stream_printf_from_utf8 - (os, diff_cmd_baton->header_encoding, subpool, + SVN_ERR(svn_stream_printf_from_utf8(outstream, + diff_cmd_baton->header_encoding, subpool, "Index: %s" APR_EOL_STR "%s" APR_EOL_STR, path, equal_string)); /* ### Print git diff headers. */ - SVN_ERR(svn_stream_printf_from_utf8 - (os, diff_cmd_baton->header_encoding, subpool, + SVN_ERR(svn_stream_printf_from_utf8(outstream, + diff_cmd_baton->header_encoding, subpool, _("Cannot display: file marked as a binary type.%s"), APR_EOL_STR)); if (mt1_binary && !mt2_binary) - SVN_ERR(svn_stream_printf_from_utf8 - (os, diff_cmd_baton->header_encoding, subpool, + SVN_ERR(svn_stream_printf_from_utf8(outstream, + diff_cmd_baton->header_encoding, subpool, "svn:mime-type = %s" APR_EOL_STR, mimetype1)); else if (mt2_binary && !mt1_binary) - SVN_ERR(svn_stream_printf_from_utf8 - (os, diff_cmd_baton->header_encoding, subpool, + SVN_ERR(svn_stream_printf_from_utf8(outstream, + diff_cmd_baton->header_encoding, subpool, "svn:mime-type = %s" APR_EOL_STR, mimetype2)); else if (mt1_binary && mt2_binary) { if (strcmp(mimetype1, mimetype2) == 0) - SVN_ERR(svn_stream_printf_from_utf8 - (os, diff_cmd_baton->header_encoding, subpool, + SVN_ERR(svn_stream_printf_from_utf8(outstream, + diff_cmd_baton->header_encoding, subpool, "svn:mime-type = %s" APR_EOL_STR, mimetype1)); else - SVN_ERR(svn_stream_printf_from_utf8 - (os, diff_cmd_baton->header_encoding, subpool, + SVN_ERR(svn_stream_printf_from_utf8(outstream, + diff_cmd_baton->header_encoding, subpool, "svn:mime-type = (%s, %s)" APR_EOL_STR, mimetype1, mimetype2)); } @@ -985,25 +951,52 @@ diff_content_changed(const char *path, if (diff_cmd_baton->diff_cmd) { + apr_file_t *outfile; + apr_file_t *errfile; + const char *outfilename; + const char *errfilename; + svn_stream_t *stream; + /* Print out the diff header. */ - SVN_ERR(svn_stream_printf_from_utf8 - (os, diff_cmd_baton->header_encoding, subpool, + SVN_ERR(svn_stream_printf_from_utf8(outstream, + diff_cmd_baton->header_encoding, subpool, "Index: %s" APR_EOL_STR "%s" APR_EOL_STR, path, equal_string)); - /* Close the stream (flush) */ - SVN_ERR(svn_stream_close(os)); /* ### Do we want to add git diff headers here too? I'd say no. The * ### 'Index' and '===' line is something subversion has added. The rest * ### is up to the external diff application. We may be dealing with * ### 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 + copy the contents to our stream. */ + SVN_ERR(svn_io_open_unique_file3(&outfile, &outfilename, NULL, + svn_io_file_del_on_pool_cleanup, + subpool, subpool)); + SVN_ERR(svn_io_open_unique_file3(&errfile, &errfilename, NULL, + svn_io_file_del_on_pool_cleanup, + subpool, subpool)); + 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, diff_cmd_baton->outfile, errfile, + &exitcode, outfile, errfile, diff_cmd_baton->diff_cmd, subpool)); + + SVN_ERR(svn_io_file_close(outfile, subpool)); + SVN_ERR(svn_io_file_close(errfile, subpool)); + + /* Now, open and copy our files to our output streams. */ + SVN_ERR(svn_stream_open_readonly(&stream, outfilename, + subpool, subpool)); + SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(outstream, subpool), + NULL, NULL, subpool)); + SVN_ERR(svn_stream_open_readonly(&stream, errfilename, + subpool, subpool)); + SVN_ERR(svn_stream_copy3(stream, svn_stream_disown(errstream, subpool), + NULL, NULL, subpool)); } else /* use libsvn_diff to generate the diff */ { @@ -1017,8 +1010,8 @@ diff_content_changed(const char *path, diff_cmd_baton->use_git_diff_format) { /* Print out the diff header. */ - SVN_ERR(svn_stream_printf_from_utf8 - (os, diff_cmd_baton->header_encoding, subpool, + SVN_ERR(svn_stream_printf_from_utf8(outstream, + diff_cmd_baton->header_encoding, subpool, "Index: %s" APR_EOL_STR "%s" APR_EOL_STR, path, equal_string)); @@ -1033,7 +1026,8 @@ diff_content_changed(const char *path, &tmp_path2, path, diff_cmd_baton->orig_path_2, diff_cmd_baton->ra_session, diff_cmd_baton->wc_ctx, diff_cmd_baton->wc_root_abspath, subpool)); - SVN_ERR(print_git_diff_header(os, &label1, &label2, operation, + SVN_ERR(print_git_diff_header(outstream, &label1, &label2, + operation, tmp_path1, tmp_path2, rev1, rev2, copyfrom_path, copyfrom_rev, @@ -1043,8 +1037,8 @@ diff_content_changed(const char *path, /* Output the actual diff */ if (svn_diff_contains_diffs(diff) || diff_cmd_baton->force_empty) - SVN_ERR(svn_diff_file_output_unified3 - (os, diff, tmpfile1, tmpfile2, label1, label2, + SVN_ERR(svn_diff_file_output_unified3(outstream, diff, + tmpfile1, tmpfile2, label1, label2, diff_cmd_baton->header_encoding, rel_to_dir, diff_cmd_baton->options.for_internal->show_c_function, subpool)); @@ -1054,9 +1048,6 @@ diff_content_changed(const char *path, APR_HASH_KEY_STRING, path); } - - /* Close the stream (flush) */ - SVN_ERR(svn_stream_close(os)); } /* ### todo: someday we'll need to worry about whether we're going @@ -1203,9 +1194,8 @@ diff_file_deleted(svn_wc_notify_state_t *state, if (diff_cmd_baton->no_diff_deleted) { - SVN_ERR(file_printf_from_utf8( - diff_cmd_baton->outfile, - diff_cmd_baton->header_encoding, + SVN_ERR(svn_stream_printf_from_utf8(diff_cmd_baton->outstream, + diff_cmd_baton->header_encoding, scratch_pool, "Index: %s (deleted)" APR_EOL_STR "%s" APR_EOL_STR, path, equal_string)); } @@ -2286,8 +2276,8 @@ svn_client_diff5(const apr_array_header_t *options svn_boolean_t ignore_content_type, svn_boolean_t use_git_diff_format, const char *header_encoding, - apr_file_t *outfile, - apr_file_t *errfile, + svn_stream_t *outstream, + svn_stream_t *errstream, const apr_array_header_t *changelists, svn_client_ctx_t *ctx, apr_pool_t *pool) @@ -2305,8 +2295,8 @@ svn_client_diff5(const apr_array_header_t *options SVN_ERR(set_up_diff_cmd_and_options(&diff_cmd_baton, options, ctx->config, pool)); diff_cmd_baton.pool = pool; - diff_cmd_baton.outfile = outfile; - diff_cmd_baton.errfile = errfile; + 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; @@ -2342,8 +2332,8 @@ svn_client_diff_peg5(const apr_array_header_t *opt svn_boolean_t ignore_content_type, svn_boolean_t use_git_diff_format, const char *header_encoding, - apr_file_t *outfile, - apr_file_t *errfile, + svn_stream_t *outstream, + svn_stream_t *errstream, const apr_array_header_t *changelists, svn_client_ctx_t *ctx, apr_pool_t *pool) @@ -2357,8 +2347,8 @@ svn_client_diff_peg5(const apr_array_header_t *opt SVN_ERR(set_up_diff_cmd_and_options(&diff_cmd_baton, options, ctx->config, pool)); diff_cmd_baton.pool = pool; - diff_cmd_baton.outfile = outfile; - diff_cmd_baton.errfile = errfile; + 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;