On Mon, Jun 07, 2010 at 01:26:49PM +0200, Stefan Sperling wrote:
> On Sun, Jun 06, 2010 at 02:49:16PM -0000, dan...@apache.org wrote:
> > URL: http://svn.apache.org/viewvc?rev=951871&view=rev
> > Log:
> > First small step towards using the 'git unidiff' format for 'svn diff'.
> >
> > The parts that writes to the output stream are ifdef'd with
> > SVN_EXPERIMENTAL_PATCH since five diff-tests needs to be adjusted and I
> > don't want to change the testsuite before we're 100 % certain that we
> > want to use the git diff format as our standard format and not as an
> > optional one.
> >
> > * subversion/libsvn_client/diff.c
> > (print_git_diff_header): New.
> > (diff_cmd_baton): Add field 'deleted'.
> > (diff_content_changed): Call print_git_diff_header() and adjust the
> > labels before asking libsvn_diff to produce the actual diff.
> > (diff_file_deleted_with_diff): Note in the diff_cmd_baton that we have
> > a deleted path.
> > (svn_client_diff5
> > svn_client_diff_peg5): Initialize diff_cmd_baton.deleted to FALSE.
> >
> > +#ifdef SVN_EXPERIMENTAL_PATCH
> > +/*
> > + * Print a git diff header for PATH to the stream OS using HEADER_ENCODING.
> > + * The header lines are determined from what operation is performed on the
> > + * file using DELETED, COPIED, MOVED and ADDED. When the
> > + * operation is a move or copy, copyfrom_path is used to determine the
> > + * source. All allocations are done in RESULT_POOL. */
> > +static svn_error_t *
> > +print_git_diff_header(svn_stream_t *os, const char *copyfrom_path,
> > + svn_boolean_t deleted, svn_boolean_t copied,
> > + svn_boolean_t moved, svn_boolean_t added,
> > + const char *header_encoding, const char *path,
> > + apr_pool_t *result_pool)
> > +{
>
> Why not make a couple of small functions, instead of a big one
> that behaves differently according to parameters?
>
> print_git_diff_header_added()
> print_git_diff_header_deleted()
> print_git_diff_header_copied()
> print_git_diff_header_moved()
Done in r952205. Will fix writing the labels inside them too, although
with the labels we are creating strings instead of writing to a stream.
We'll see if I can come up with better names.
> >
> > /* Generate a label for the diff output for file PATH at revision REVNUM.
> > @@ -535,6 +597,8 @@ diff_content_changed(const char *path,
> > (os, 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,
> > _("Cannot display: file marked as a binary type.%s"),
> > @@ -577,6 +641,11 @@ diff_content_changed(const char *path,
> > /* 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.*/
>
> I'd say print the extra information even if an external diff application
> is used. The external diff application likely won't bother figuring out
> whether we moved or copied something. And even if it did, I'd rather
> have external diff tools rely on Subversion to produce this information.
Ok. It's fine either way for me. I'll wait with fixing the 'external
diff-tool' and 'binary diff' cases until the code has stabilized. Don't
want to sprinkle to many #define's around.
Side-node: It's too bad the labels are printed inside libsvn_diff. The
rest of the git header lines are created in libsvn_client. For
consistency, it would be better if they were written at the same place.
Maybe I should rev the libsvn_diff funcs?
Thanks,
Daniel
