Augie Fackler <durin42@gmail.com> writes:
> Greetings all,
>
> I've implemented launching of an external merge tool in the svn =20
> binary. Unfortunately, my inspection of the world indicates that =20
> there is no standard interface for merge tools, so I ended up =20
> borrowing Mercurial's hgmerge shell script and modifying it to work =20
> with the interface I devised.
>
> Right now, the patch assumes the external merge tool accepts 4 =20
> arguments in the following order:
> base left right merged
> where merged is the destination for the file the merge tool will save =20=
>
> out. This reflects our tempfile model really well, but isn't how some =20=
>
> merge tools work (some merge tools seem to want to put the merge in =20
> either base or right, which offends me on some level). I can provide =20
> a script that wraps Apple's FileMerge tool if anyone is interested - =20
> I'm hoping to finish patching up hgmerge to be svn_merge_helper that =20
> will automagically pick the right choice from several options.
This is fine for now, but eventually I'd like to see us use
format strings for this and for diff. Currently, people have to
write scripts to massage GNU diff arguments into whatever their
own diff tool works, and now they'd have to do the same for merge
tools. Instead, we should offer something like this:
# %L = left %R = right
diff-command = mycrazydifftool %R %L
# %B = base %T = theirs %M = mine %G = merged
merge-command = mycrazymergetool %B %T %M %G
I think your use of left and right is incorrect. It's a
three-way merge, without true left and right, though it's
analagous to diff base theirs | patch, i.e. left=base
right=theirs; I'll be fixing the terminology in these structures
soon, hopefully after your patch goes in.
> Note that I've got a couple of nitpicks of my own against this patch:
> 1) I use getwd() - is there some APR/Subversion function I can use =20
> for this? I couldn't find anything, and this seems to work just fine.
No, but use getcwd. getwd leads to buffer overflows, as the
kernel doesn't know how large your buffer is.
> 2) It seems that there should be more ways to set the tool than =20
> $SVNMERGE - probably a config variable. What's the procedure for =20
> adding one? I'd love to go ahead and add that too.
What Sussman said. That can be a separate change, though. The
change looks pretty good, I just have a few nitpicks below.
> Index: subversion/svn/util.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- subversion/svn/util.c (revision 26684)
> +++ subversion/svn/util.c (working copy)
> @@ -27,6 +27,7 @@
> #include <string.h>
> #include <ctype.h>
> #include <assert.h>
> +#include <unistd.h>
>
> #include <apr_errno.h>
> #include <apr_strings.h>
> @@ -193,8 +194,49 @@
> return SVN_NO_ERROR;
> }
>
> +svn_error_t *
> +svn_cl__merge_file_externally(const char *base_path,
> + const char *repos_path,
> + const char *user_path,
> + const char *merged_path,
> + apr_pool_t *pool)
> +{
> + const char *merge_tool;
> + merge_tool =3D getenv("SVNMERGE");
Use apr_env_get instead.
> + /* command base left right destination */
This comment is obscure; rephrase it, or maybe drop it, as the
arguments[] assignment is clear enough.
> + const char *arguments[] =3D { merge_tool, base_path, repos_path,
> + user_path, merged_path, NULL};
> + int exitcode;
> + apr_exit_why_e exitwhy;
> + char *sys_path =3D getwd(NULL);
This behavior of getwd is non-standard; just apr_palloc a buffer
and use getcwd.
> + const char *path =3D svn_path_internal_style(sys_path, pool);
> + free(sys_path);
> + SVN_ERR(svn_io_run_cmd(path, merge_tool, arguments, &exitcode, =20
> &exitwhy, TRUE,
> + NULL, NULL, NULL, pool));
> +
> + return SVN_NO_ERROR;
Just return svn_io_run_cmd(...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Oct 1 22:03:57 2007