On Oct 1, 2007, at 4:03 PM, Eric Gillespie wrote:
> 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 like this a lot - perhaps I'll try looking at this once I get this
patch in. As long as we don't have to use system(), I'll be happier
this way than the way I've got it now.
>
> 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.
Acutally, I think the terminology is fairly sane - FileMerge (and the
tools I've investigated but not used) tend to use the "left vs right"
change concept with the file on the right being the default (for most
tools).
>
>> 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.
Ah - this is what I get for not checking POSIX spec before using getwd
(NULL)...
>
>> 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.
Can do.
>
>> + /* command base left right destination */
>
> This comment is obscure; rephrase it, or maybe drop it, as the
> arguments[] assignment is clear enough.
I'll probably just drop this in that case...if it still seems clear
now that it's rotted in my mind for a few days.
>
>> + 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.
Will do.
>
>> + 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(...
Will do.
I've gotta go to orchestra right now - I'll work on this patch after
dinner and send it in!
Peace,
Augie
---------------------------------------------------------------------
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:18:03 2007