You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jeremy Whitlock <jc...@gmail.com> on 2007/06/01 02:54:32 UTC
[PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Hi All,
Attached is the patch used to resolve issue 2784. Here is a synopsis:
This patch adds the "--accept" or "-a" flag support to the Subversion
command line. Right now, only resolved subcommand is using it. This
new flag will also be used by the following subcommands:
merge
switch
update
I will be creating separate issues for each implementation of the
"--accept" flag for each subcommand and patches will be created for
each. Working on this also will mean revving the bindings to handle
this new functionality. Just like with making the other subcommands
support this flag, each binding will be working on as separate issues.
All related issues will be created shortly after this email. As for
the log message for this patch, it will be at the bottom of this
message. Thanks to all that have helped during this patch.
Take care,
Jeremy
[[[
Add option to resolve conflicts by selecting a specific file by adding
support to the svn executable to handle the "--accept" flag. The
"resovled" subcommand is the only subcommand using this new flag as
part of this patch. (Issue 2784)
* subversion/include/svn_client.h
(svn_client_resolved): Revved to svn_client_resolved2() to handle
the svn_accept_t enum.
* subversion/include/svn_types.h
(svn_accept_t): Created an enum to handle the different accept options.
(svn_accept_to_word): Function prototype for getting the string
representation of the accept options.
(svn_accept_from_word): Function prototype to return the proper
svn_accept_t based on the accept word.
* subversion/include/svn_wc.h
(svn_wc_walk_entries): Revved to svn_wc_walk_entries4 to handle the
svn_accept_t enum.
(svn_wc_resolved_conflict): Revved to svn_wc_resolved_conflict3 to
handle the svn_accept_t enum.
* subversion/libsvn_client/resolved.c
(svn_client_resolved): Implementation of svn_client_resolved2()
which now handles the svn_accept_t as an argument.
* subversion/libsvn_subr/kitchensink.c
(svn_accept_to_word): Implementation of svn_accept_to_word() which
will take an svn_accept_t enum and return a string representation of
that enum.
(svn_accept_from_word): Implementation of svn_accept_from_word()
which will return an svn_accept_t based on the passed accept string.
* subversion/libsvn_wc/adm_ops.c
(resolve_conflict_on_entry): Updated this function to handle an
svn_accept_t argument. This is also where the real work of handling
the accept flag happens.
(resolve_callback_baton): Updated the structure to have a
placeholder for the svn_accept_t argument so that
resolve_found_entry_callback() can pass the necessary information to
resolve_conflict_on_entry().
(svn_wc_resolved_conflict): Implementation of
svn_wc_resolved_conflict3() which now handles the svn_accept_t as an
argument.
* subversion/svn/cl.h
(svn_cl__opt_state_t): Updated to have a place holder for the accept
flag passed to the svn executable.
* subversion/svn/main.c
(svn_cl__options): Updated to have all necessary information for the
svn executable to provide help for, and handle, the accept flag in
both long and short format.
(svn_cl__cmd_table): Updated to have the resolved subcommand handle
the passing of the accept flag.
(svn_cl__limit_opt): Added case statement to handle parsing the
accept flag and erroring when an invalid argument was passed. If a
valid argument is passed, it is then turned into the svn_accept_t enum
based on the svn_accept_from_word() function in
subversion/libsvn_subr/kitchensink.c.
* subversion/svn/resolved-cmd.c
(svn_cl__resolved): Updated to call the revved svn_client_resolved()
which is now at svn_client_resolved2().
* subversion/tests/cmdline/basic_test.py
(automatic_conflict_resolution): Created a new test to test the
handling of the new accept flag for the resolved subcommand.
(test_list): Added the automatic_conflict_resolution test to the test list.
]]]
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Mark Phippard" <ma...@gmail.com> writes:
> Famous last words, but I do not see why if we are in the middle of a
> merge process and we detect a conflict and know enough to put markers
> in the file, that it would be that much more difficult to have a flag
> that says "nope, just use that version" for this hunk.
Famous last words, indeed :). But it sounds reasonable.
> I guess where I differ from you, is that with the exception of binary
> files, I cannot imagine where the current behavior would ever be
> desirable. Even worse, is that I think the presence of the option
> would create the desire for the hunk-based behavior. What is the use
> case where when merging an entire project if it received a conflict,
> you'd just want to bail on all the merge that could be done and just
> accept one of the revisions in it's entirety?
Release branches (--accept=theirs).
Anyway, as i said, i'm not arguing against the behavior you
describe; i think it's an improvement. But an improvement is all
i see it as.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Mark Phippard <ma...@gmail.com>.
On 6/1/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> "Mark Phippard" <ma...@gmail.com> writes:
>
> > We discussed a bit on IRC. I'd say the conclusion was that the
> > current approach is good for resolved command, but for the others, it
> > would need to be a hunk-based approached where this option only
> > applied to the hunks that were in conflict. Here is a transcript:
>
> Unless i'm missing something, i understood correctly. I still
> don't see why it *needs* to be hunk-based, even for
> merge/update/switch. I'm certainly not opposed to that
> improvement! But, are we letting the perfect be the enemy of the
> good? Or maybe handling it by hunk is easier than i think.
Famous last words, but I do not see why if we are in the middle of a
merge process and we detect a conflict and know enough to put markers
in the file, that it would be that much more difficult to have a flag
that says "nope, just use that version" for this hunk.
I guess where I differ from you, is that with the exception of binary
files, I cannot imagine where the current behavior would ever be
desirable. Even worse, is that I think the presence of the option
would create the desire for the hunk-based behavior. What is the use
case where when merging an entire project if it received a conflict,
you'd just want to bail on all the merge that could be done and just
accept one of the revisions in it's entirety?
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Mark Phippard" <ma...@gmail.com> writes:
> We discussed a bit on IRC. I'd say the conclusion was that the
> current approach is good for resolved command, but for the others, it
> would need to be a hunk-based approached where this option only
> applied to the hunks that were in conflict. Here is a transcript:
Unless i'm missing something, i understood correctly. I still
don't see why it *needs* to be hunk-based, even for
merge/update/switch. I'm certainly not opposed to that
improvement! But, are we letting the perfect be the enemy of the
good? Or maybe handling it by hunk is easier than i think.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Mark Phippard <ma...@gmail.com>.
On 6/1/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> "Mark Phippard" <ma...@gmail.com> writes:
>
> > This leads me to wonder if that is not what should be happening here
> > as well. If the files are unmergeable (binary) it would do what it is
> > doing now. But if these are text files, it seems like it ought to
> > merge them and use this setting to resolve the conflicts it encounters
> > along the way.
> >
> > Thoughts? Am I out of my mind?
>
> I've proposed this feature in a couple different forms over the
> years ;->. The most recent proposal was inspired by Perforce's
> -am, -ay, and -at options: --accept={merged,yours,theirs} . And,
> all i was looking for is what Jeremy has implemented. That's
> also all Perforce implements.
>
> Now, if it's possible for us to do even better, and do this at
> the individual patch hunk level, even better! I think that is a
> much bigger chunk of work, though.
We discussed a bit on IRC. I'd say the conclusion was that the
current approach is good for resolved command, but for the others, it
would need to be a hunk-based approached where this option only
applied to the hunks that were in conflict. Here is a transcript:
dlr_h: markphip: I didn't really see a problem WRT the command-line.
dlr_h: It's definitely an issue for UIs with potential for more
involved merging interfaces.
markphip: so you would be in favor of implementing this as-is for
commands like merge/up/sw?
dlr_h: ...where a merge conflict resolution callback would be in order.
dlr_h: --accept would "conflict" with a merge conflict resolution callback, no?
markphip: I assume a callback would take precedence
markphip: I just do not think the --accept option in this form makes
sense for those other commands
dlr_h: For a command-line merge conflict resolution mode, the accept
logic would need to be extended to the chunk level.
dlr_h: And, include an option like "edit".
markphip: right, that is all I meant
*pburba thinks we should elide these options
markphip: It couldn't let the merge process create the three files,
and then select one of them and resolve it, like svn resolved does
markphip: except for binaries
dlr_h: even binaries might someday have a "smart resolution" mode,
with a merge tool which understands their format
markphip: I think what Jeremy did is OK for resolved
dlr_h: me too
markphip: I think the other commands need to merge what they can, and
as you say, this could be like a built-in callback to select one of
the revisions when there are conflicts
markphip: favor might be a better term than select here
dlr_h: favor implies you still might not choose it
dlr_h: you might want to summarize this discussion in a follow-up email
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Mark Phippard" <ma...@gmail.com> writes:
> This leads me to wonder if that is not what should be happening here
> as well. If the files are unmergeable (binary) it would do what it is
> doing now. But if these are text files, it seems like it ought to
> merge them and use this setting to resolve the conflicts it encounters
> along the way.
>
> Thoughts? Am I out of my mind?
I've proposed this feature in a couple different forms over the
years ;->. The most recent proposal was inspired by Perforce's
-am, -ay, and -at options: --accept={merged,yours,theirs} . And,
all i was looking for is what Jeremy has implemented. That's
also all Perforce implements.
Now, if it's possible for us to do even better, and do this at
the individual patch hunk level, even better! I think that is a
much bigger chunk of work, though.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Mark Phippard <ma...@gmail.com>.
On 5/31/07, Jeremy Whitlock <jc...@gmail.com> wrote:
> Hi All,
> Attached is the patch used to resolve issue 2784. Here is a synopsis:
>
> This patch adds the "--accept" or "-a" flag support to the Subversion
> command line. Right now, only resolved subcommand is using it. This
> new flag will also be used by the following subcommands:
>
> merge
> switch
> update
>
> I will be creating separate issues for each implementation of the
> "--accept" flag for each subcommand and patches will be created for
> each. Working on this also will mean revving the bindings to handle
> this new functionality. Just like with making the other subcommands
> support this flag, each binding will be working on as separate issues.
> All related issues will be created shortly after this email. As for
> the log message for this patch, it will be at the bottom of this
> message. Thanks to all that have helped during this patch.
>
This is not a patch review.
Sorry to be sending this so late in the process, but I just thought of
it. I even got out of bed to send this before I lose the thought.
Also, I entered this issue and pointed Jeremy at it to see if he was
interested, so it is not like I do not want to see it implemented.
Anyway, in thinking about how this new option would be implemented in
svn merge, it just suddenly occurred to me that we might be doing this
wrong.
Everything we are doing makes perfect sense if this is a binary file.
But if this is a text file, I am not sure we are doing what a user
would expect.
In my experience, when using a graphical merge tool, it will often
walk you through conflicts. You have various options such as "use
mine" or "use theirs" to resolve each conflict. There is usually an
option to "use mine for all" or "use theirs for all". We are sort of
doing that here.
The difference is that we are not merging the files at all. We are
simply taking one of the three existing versions of the file. I am
not sure that is what a user would expect. Perhaps with the resolved
command it is sort of clear, I am not sure if it is or not. But if I
did this on svn merge, I think I would be expecting it to merge the
files, and use this parameter only to resolve the conflicts it
encounters within the merge.
This leads me to wonder if that is not what should be happening here
as well. If the files are unmergeable (binary) it would do what it is
doing now. But if these are text files, it seems like it ought to
merge them and use this setting to resolve the conflicts it encounters
along the way.
Thoughts? Am I out of my mind?
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Eric,
When I said that, "now" meant the email you sent and that Karl
reiterated upon. :)
Take care,
Jeremy
On 6/8/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> Jeremy, i'll get to your other message today, i promise :)
>
> "Jeremy Whitlock" <jc...@gmail.com> writes:
>
> > Hi All,
> > Makes sense to me. Remember, this has been discussed and until
> > now, no one had actually made a concrete request to change the
> > verbiage. The verbiage will be changes in all necessary locations.
>
> Well, hold on now!
>
> > On 6/8/07, Karl Fogel <kf...@red-bean.com> wrote:
> > > Eric Gillespie <ep...@pretzelnet.org> writes:
> > > > Unfortunately, i don't think we've finished painting this
> > > > particular bikeshed yet ;->. I finally went and got myself a
> > > > conflict so i could see if we could take name inspiration from
> > > > there. So, how about svn_accept_{old,new,work} with strings
> > > > "left", "new", and "working"?
> > >
> > > I can't believe I'm writing this, http://green.bikeshed.com/, etc, but:
> > > let's avoid needless grammar variations. svn_accept_{old,new,working}
> > > would perfectly match the adjectives we actually use.
>
> It's not just a bikeshed; this is a UI consitency question. The
> files are called "left", "right", and "working". It seems
> logical to me to use those names, not expect the user to run a
> map in his head.
>
> As for the svn_accept_t values, yeah, bikeshed, who cares. I
> thought we should try to match the conflict fields in
> svn_wc_entry_t, but maybe it's better to match the strings.
>
> --
> Eric Gillespie <*> epg@pretzelnet.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
Jeremy, i'll get to your other message today, i promise :)
"Jeremy Whitlock" <jc...@gmail.com> writes:
> Hi All,
> Makes sense to me. Remember, this has been discussed and until
> now, no one had actually made a concrete request to change the
> verbiage. The verbiage will be changes in all necessary locations.
Well, hold on now!
> On 6/8/07, Karl Fogel <kf...@red-bean.com> wrote:
> > Eric Gillespie <ep...@pretzelnet.org> writes:
> > > Unfortunately, i don't think we've finished painting this
> > > particular bikeshed yet ;->. I finally went and got myself a
> > > conflict so i could see if we could take name inspiration from
> > > there. So, how about svn_accept_{old,new,work} with strings
> > > "left", "new", and "working"?
> >
> > I can't believe I'm writing this, http://green.bikeshed.com/, etc, but:
> > let's avoid needless grammar variations. svn_accept_{old,new,working}
> > would perfectly match the adjectives we actually use.
It's not just a bikeshed; this is a UI consitency question. The
files are called "left", "right", and "working". It seems
logical to me to use those names, not expect the user to run a
map in his head.
As for the svn_accept_t values, yeah, bikeshed, who cares. I
thought we should try to match the conflict fields in
svn_wc_entry_t, but maybe it's better to match the strings.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Hi All,
Makes sense to me. Remember, this has been discussed and until
now, no one had actually made a concrete request to change the
verbiage. The verbiage will be changes in all necessary locations.
Take care,
Jeremy
On 6/8/07, Karl Fogel <kf...@red-bean.com> wrote:
> Eric Gillespie <ep...@pretzelnet.org> writes:
> > Unfortunately, i don't think we've finished painting this
> > particular bikeshed yet ;->. I finally went and got myself a
> > conflict so i could see if we could take name inspiration from
> > there. So, how about svn_accept_{old,new,work} with strings
> > "left", "new", and "working"?
>
> I can't believe I'm writing this, http://green.bikeshed.com/, etc, but:
> let's avoid needless grammar variations. svn_accept_{old,new,working}
> would perfectly match the adjectives we actually use.
>
> -Karl
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Karl Fogel <kf...@red-bean.com>.
Eric Gillespie <ep...@pretzelnet.org> writes:
> Unfortunately, i don't think we've finished painting this
> particular bikeshed yet ;->. I finally went and got myself a
> conflict so i could see if we could take name inspiration from
> there. So, how about svn_accept_{old,new,work} with strings
> "left", "new", and "working"?
I can't believe I'm writing this, http://green.bikeshed.com/, etc, but:
let's avoid needless grammar variations. svn_accept_{old,new,working}
would perfectly match the adjectives we actually use.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Eric,
Thanks for working with me on this. I think we both figured this
was much easier than it was especially since there are some verbiage
conflicts in the Subversion source that complicated this. I
appreciate your patience and guidance. I will make sure to take what
I've learned and use it in other patch submissions. The next things
I'll do are related to this. I'd like to update all four bindings to
work with this new functionality and there is also a revved function
that most uses of are still using the deprecated version. I'd like to
fix that too.
Take care,
Jeremy
On 6/12/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
>
> > Eric,
> > Can you explain? I'm not familiar with the accept(2) system call.
>
> http://www.opengroup.org/onlinepubs/009695399/functions/accept.html
>
> But it doesn't matter, the point is that identifier is already
> taken. I'm going to commit now, but two things to keep in mind
> for future submissions: use spaces instead of tabs, and keep
> lines at 79 columns or less. I fixed one or two of these.
>
> Thanks!
>
> --
> Eric Gillespie <*> epg@pretzelnet.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> Eric,
> Can you explain? I'm not familiar with the accept(2) system call.
http://www.opengroup.org/onlinepubs/009695399/functions/accept.html
But it doesn't matter, the point is that identifier is already
taken. I'm going to commit now, but two things to keep in mind
for future submissions: use spaces instead of tabs, and keep
lines at 79 columns or less. I fixed one or two of these.
Thanks!
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Eric,
Can you explain? I'm not familiar with the accept(2) system call.
Take care,
Jeremy
On 6/12/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> Eric Gillespie <ep...@pretzelnet.org> writes:
>
> > "Jeremy Whitlock" <jc...@gmail.com> writes:
> >
> > > Hi All,
> > > Attached is revision three of the patch for issue 2784, automatic
> > > conflict resolution. You should notice the proper verbiage being used
> > > in the patch and that the changes Eric suggested last have been taken
> > > into account during this new version of the patch.
> >
> > Looks good to me. Building and testing now, then commit.
>
> Oops, there's a minor problem: your accept variables shadow the
> accept(2) system call.
>
> --
> Eric Gillespie <*> epg@pretzelnet.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
Eric Gillespie <ep...@pretzelnet.org> writes:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
>
> > Hi All,
> > Attached is revision three of the patch for issue 2784, automatic
> > conflict resolution. You should notice the proper verbiage being used
> > in the patch and that the changes Eric suggested last have been taken
> > into account during this new version of the patch.
>
> Looks good to me. Building and testing now, then commit.
Oops, there's a minor problem: your accept variables shadow the
accept(2) system call.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> Hi All,
> Attached is revision three of the patch for issue 2784, automatic
> conflict resolution. You should notice the proper verbiage being used
> in the patch and that the changes Eric suggested last have been taken
> into account during this new version of the patch.
Looks good to me. Building and testing now, then commit.
Thanks!
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Hi All,
Attached is revision three of the patch for issue 2784, automatic
conflict resolution. You should notice the proper verbiage being used
in the patch and that the changes Eric suggested last have been taken
into account during this new version of the patch.
Take care,
Jeremy
[[[
Add option to resolve conflicts by selecting a specific file by adding
support to the svn executable to handle the "--accept" flag. The
"resolved" subcommand is the only subcommand using this new flag as
part of this patch. (Issue 2784)
* subversion/include/svn_client.h
(svn_client_resolved): Revved to svn_client_resolved2() to handle the
svn_accept_t enum.
* subversion/include/svn_types.h
(svn_accept_t): Created an enum to handle the different accept options.
(svn_accept_from_word): Function prototype to return the proper
svn_accept_t based on the accept word.
* subversion/include/svn_wc.h
(svn_wc_resolved_conflict): Revved to svn_wc_resolved_conflict3 to
handle the svn_accept_t enum.
* subversion/libsvn_client/resolved.c
(svn_client_resolved): Implementation of svn_client_resolved2()
which now handles svn_accept_t as an argument.
* subversion/libsvn_subr/kitchensink.c
(svn_accept_from_word): Implementation of svn_accept_from_word()
which will return an svn_accept_t based on the passed accept string.
* subversion/libsvn_wc/adm_ops.c
(resolve_conflict_on_entry): Updated this function to handle an
svn_accept_t argument. This is also where the real work of
automatic conflict resolution happens.
(resolve_callback_baton): Updated the structure to have a
placeholder for the svn_accept_t argument so that
resolve_found_entry_callback() can pass the necessary information
to resolve_conflict_on_entry().
(svn_wc_resolved_conflict): Implementation of
svn_wc_resolved_conflict3() which now handles the svn_accept_t as
an argument.
* subversion/svn/cl.h
(svn_cl__opt_state_t): Updated to have a place holder for the accept
flag passed to the svn executable.
(svn_cl__long_opt_t): Updated to handle the svn_accept_t as a long
svn executable argument.
* subversion/svn/cl.h
(svn_cl__opt_state_t): Updated to have a place holder for the accept
flag passed to the svn executable.
(svn_cl__long_opt_t): Updated to handle the svn_accept_t as a long
svn executable argument.
* subversion/svn/main.c
(svn_cl__options): Updated to have all necessary information for the
svn executable to provide help for, and handle, the accept flag.
(svn_cl__cmd_table): Updated to have the resolved subcommand handle
the passing of the accept flag.
(svn_cl__limit_opt): Added case statement to handle parsing the
accept flag and erroring when an invalid argument was passed. If a
valid argument is passed, it is then turned into the svn_accept_t enum
based on the svn_accept_from_word() function in
subversion/libsvn_subr/kitchensink.c.
* subversion/svn/resolved-cmd.c
(svn_cl__resolved): Updated to call the revved svn_client_resolved()
which is now at svn_client_resolved2().
* subversion/tests/cmdline/basic_test.py
(automatic_conflict_resolution): Created a new test to test the
handling of the new accept flag for the resolved subcommand.
(test_list): Added the automatic_conflict_resolution test to the test list.
]]]
On 6/10/07, Jeremy Whitlock <jc...@gmail.com> wrote:
> Karl,
> I agree and I usually don't hesitate but there has been some
> disagreement both ways and I wanted to see if there was someone other
> than Eric and myself that could put the nail in the coffin so we could
> put this to rest.
>
> Take care,
>
> Jeremy
>
> On 6/10/07, Karl Fogel <kf...@red-bean.com> wrote:
> > "Jeremy Whitlock" <jc...@gmail.com> writes:
> > > I can do whatever *we* decide is right. I think accepting them
> > > all makes sense since the verbiage can be misleading if you do not.
> > > Can someone else agree with this approach so I can resubmit a new
> > > patch?
> >
> > I agree.
> >
> > But you don't have to wait for someone else to agree to submit a new
> > patch -- if you're pretty sure agreement is likely, you can just post
> > the revised patch and wait for someone to disagree with the approach :-).
> >
>
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Karl,
I agree and I usually don't hesitate but there has been some
disagreement both ways and I wanted to see if there was someone other
than Eric and myself that could put the nail in the coffin so we could
put this to rest.
Take care,
Jeremy
On 6/10/07, Karl Fogel <kf...@red-bean.com> wrote:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
> > I can do whatever *we* decide is right. I think accepting them
> > all makes sense since the verbiage can be misleading if you do not.
> > Can someone else agree with this approach so I can resubmit a new
> > patch?
>
> I agree.
>
> But you don't have to wait for someone else to agree to submit a new
> patch -- if you're pretty sure agreement is likely, you can just post
> the revised patch and wait for someone to disagree with the approach :-).
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Karl Fogel <kf...@red-bean.com>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> I can do whatever *we* decide is right. I think accepting them
> all makes sense since the verbiage can be misleading if you do not.
> Can someone else agree with this approach so I can resubmit a new
> patch?
I agree.
But you don't have to wait for someone else to agree to submit a new
patch -- if you're pretty sure agreement is likely, you can just post
the revised patch and wait for someone to disagree with the approach :-).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Hi All,
I can do whatever *we* decide is right. I think accepting them
all makes sense since the verbiage can be misleading if you do not.
Can someone else agree with this approach so I can resubmit a new
patch?
Take care,
Jeremy
On 6/8/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
>
> > Hi All,
> > I think I know why there is a slight verbiage disagreement. When
> > you create a conflict as part of an update/switch, the naming
> > convention is:
> >
> > .rOLD, .rNEW and .mine
> >
> > When you create a conflict as part of a merge, the naming convention is:
> >
> > .merge-left.rOLD, .merge-right.rNEW and .working
>
> Well, crap. I didn't realize we had this inconsistency out
> there. And i bet OLD and NEW are backwards when i'm back-dating
> (e.g. at r37 and svn up -r30). Sigh.
>
> So, why don't we accept both? --accept=left == --accept=old and
> --accept=right == --accept=new and the svn_accept_t values can
> stay left/right, since that's what we really have in all cases anyway.
>
> Thanks, Jeremy, for sticking with what originally looked like
> such a simple task ;->.
>
> --
> Eric Gillespie <*> epg@pretzelnet.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> Hi All,
> I think I know why there is a slight verbiage disagreement. When
> you create a conflict as part of an update/switch, the naming
> convention is:
>
> .rOLD, .rNEW and .mine
>
> When you create a conflict as part of a merge, the naming convention is:
>
> .merge-left.rOLD, .merge-right.rNEW and .working
Well, crap. I didn't realize we had this inconsistency out
there. And i bet OLD and NEW are backwards when i'm back-dating
(e.g. at r37 and svn up -r30). Sigh.
So, why don't we accept both? --accept=left == --accept=old and
--accept=right == --accept=new and the svn_accept_t values can
stay left/right, since that's what we really have in all cases anyway.
Thanks, Jeremy, for sticking with what originally looked like
such a simple task ;->.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Hi All,
I think I know why there is a slight verbiage disagreement. When
you create a conflict as part of an update/switch, the naming
convention is:
.rOLD, .rNEW and .mine
When you create a conflict as part of a merge, the naming convention is:
.merge-left.rOLD, .merge-right.rNEW and .working
Since the accept flag should be capable of automatically resolving any
conflict, what verbiage makes sense? There are two different sets of
verbiage now and we need to decide what to use. I think the reason
we've had some verbiage conflicts to this point is that some people
see this from the merge perspective and some see it from the
update/switch perspective.
Take care,
Jeremy
On 6/8/07, Jeremy Whitlock <jc...@gmail.com> wrote:
> Eric,
> Thanks for the clarification. I'm good now.
>
> Take care,
>
> Jeremy
>
> On 6/8/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> > "Jeremy Whitlock" <jc...@gmail.com> writes:
> >
> > > What is the real purpose of svn_accept_invalid? I can see it being
> > > returned from svn_accept_from_word when an invalid accept word is
> > > passed as an argument but beyond that, how else do you see it being
> > > used?
> >
> > That's it. The point is this: you have an error condition when
> > someone says --accept=goat. That means your from_word function
> > needs to return something other than a valid value.
> >
> > > I also think that the command line parsing should handle this
> > > as an error when you pass an invalid value as the accept flag. This
> > > is how I do it now but you suggest otherwise. Can you explain?
> >
> > No, i don't suggest otherwise :). Hope it's clear now.
> >
> > > The other things you mentioned are valid. Sometimes it is easy to
> > > look over small things like docstrings when trying to get code
> > > working. Please bear with me with this patch as I'm learning the
> > > Subversion coding standards and API as I go along.
> >
> > Hey, no problem!
> >
> > --
> > Eric Gillespie <*> epg@pretzelnet.org
> >
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Eric,
Thanks for the clarification. I'm good now.
Take care,
Jeremy
On 6/8/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
>
> > What is the real purpose of svn_accept_invalid? I can see it being
> > returned from svn_accept_from_word when an invalid accept word is
> > passed as an argument but beyond that, how else do you see it being
> > used?
>
> That's it. The point is this: you have an error condition when
> someone says --accept=goat. That means your from_word function
> needs to return something other than a valid value.
>
> > I also think that the command line parsing should handle this
> > as an error when you pass an invalid value as the accept flag. This
> > is how I do it now but you suggest otherwise. Can you explain?
>
> No, i don't suggest otherwise :). Hope it's clear now.
>
> > The other things you mentioned are valid. Sometimes it is easy to
> > look over small things like docstrings when trying to get code
> > working. Please bear with me with this patch as I'm learning the
> > Subversion coding standards and API as I go along.
>
> Hey, no problem!
>
> --
> Eric Gillespie <*> epg@pretzelnet.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> What is the real purpose of svn_accept_invalid? I can see it being
> returned from svn_accept_from_word when an invalid accept word is
> passed as an argument but beyond that, how else do you see it being
> used?
That's it. The point is this: you have an error condition when
someone says --accept=goat. That means your from_word function
needs to return something other than a valid value.
> I also think that the command line parsing should handle this
> as an error when you pass an invalid value as the accept flag. This
> is how I do it now but you suggest otherwise. Can you explain?
No, i don't suggest otherwise :). Hope it's clear now.
> The other things you mentioned are valid. Sometimes it is easy to
> look over small things like docstrings when trying to get code
> working. Please bear with me with this patch as I'm learning the
> Subversion coding standards and API as I go along.
Hey, no problem!
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Eric,
Thanks for the feedback. I'll resubmit another patch. I do have
one thing to discuss:
What is the real purpose of svn_accept_invalid? I can see it being
returned from svn_accept_from_word when an invalid accept word is
passed as an argument but beyond that, how else do you see it being
used? I also think that the command line parsing should handle this
as an error when you pass an invalid value as the accept flag. This
is how I do it now but you suggest otherwise. Can you explain?
The other things you mentioned are valid. Sometimes it is easy to
look over small things like docstrings when trying to get code
working. Please bear with me with this patch as I'm learning the
Subversion coding standards and API as I go along.
Take care,
Jeremy
On 6/7/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
>
> > [[[
> > Add option to resolve conflicts by selecting a specific file by adding
> > support to the svn executable to handle the "--accept" flag. The
> > "resovled" subcommand is the only subcommand using this new flag as
> ^^^^^^^^
>
> resolved
>
> > Index: subversion/include/svn_types.h
> > ===================================================================
> > --- subversion/include/svn_types.h (revision 25291)
> > +++ subversion/include/svn_types.h (working copy)
> > @@ -199,6 +199,34 @@
> > svn_recursive
> > };
> >
> > +/** The concept of automatic conflict resolution.
> > + *
> > + * @since New in 1.5
> > + */
> > +typedef enum
> > +{
>
> I think you want 'svn_accept_invalid = -1' here; see below.
>
> > + /* Resolve the conflict as usual */
> > + svn_accept_default,
> > +
> > + /* Resolve the conflict with the pre-update base file */
> > + svn_accept_orig,
> > +
> > + /* Resolve the conflict with the pre-update working copy file */
> > + svn_accept_mine,
> > +
> > + /* Resolve the conflict with the during-update repository file, new base */
> > + svn_accept_base,
> > +
> > +} svn_accept_t;
>
> Unfortunately, i don't think we've finished painting this
> particular bikeshed yet ;->. I finally went and got myself a
> conflict so i could see if we could take name inspiration from
> there. So, how about svn_accept_{old,new,work} with strings
> "left", "new", and "working"?
>
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h (revision 25291)
> > +++ subversion/include/svn_wc.h (working copy)
> > @@ -2474,6 +2474,23 @@
> > void *cancel_baton,
> > apr_pool_t *pool);
> >
> > +/**
> > + * Just like "" but with the ability to pass an svn_accept_t argument
> > + * for automatical conflict resolution handling.
> > + *
> > + * @since New in 1.5
>
> You have this backwards. Move the svn_wc_resolved_conflict2
> docstring here and document the new parameter. Then add a
> "similar to"/deprecated comment for the old one; see
> svn_wc_adm_open3 for example. svn_client_resolved2, too.
>
> > Index: subversion/libsvn_wc/adm_ops.c
>
> > @@ -2519,6 +2524,25 @@
> > svn_boolean_t was_present, need_feedback = FALSE;
> > apr_uint64_t modify_flags = 0;
> > svn_wc_entry_t *entry = svn_wc_entry_dup(orig_entry, pool);
> > +
> > + /* Handle automatic conflict resolution before the temporary files are
> > + * deleted. */
> > + if (accept != svn_accept_default)
> > + {
> > + const char *base_name;
> > +
> > + if (accept == svn_accept_orig) {
> > + base_name = entry->conflict_old;
> > + } else if (accept == svn_accept_mine) {
> > + base_name = entry->conflict_wrk;
> > + } else if (accept == svn_accept_base) {
> > + base_name = entry->conflict_new;
> > + } /* Should never be anything else thanks to the svn_accept_from_word() */
>
> Not all callers use svn_accept_from_word; maybe just say "Ignore
> invalid values". If someone passes some random value instead of
> one of the svn_accept_ values, that's his problem.
>
> > @@ -2594,6 +2618,8 @@
> > svn_boolean_t resolve_text;
> > /* TRUE if property conflicts are to be resolved. */
> > svn_boolean_t resolve_props;
> > + /* Placeholder for the type of automatic conflict resolution to perform */
> > + svn_accept_t accept;
>
> Drop "Placeholder for the".
>
> > Index: subversion/libsvn_subr/kitchensink.c
> > ===================================================================
> > --- subversion/libsvn_subr/kitchensink.c (revision 25291)
> > +++ subversion/libsvn_subr/kitchensink.c (working copy)
> > @@ -32,6 +32,18 @@
> > return uuid_str;
> > }
> >
> > +svn_accept_t
> > +svn_accept_from_word(const char *word)
> > +{
> > + if (strcmp(word, "orig") == 0)
> > + return svn_accept_orig;
> > + if (strcmp(word, "mine") == 0)
> > + return svn_accept_mine;
> > + if (strcmp(word, "base") == 0)
> > + return svn_accept_base;
> > + /* Return default, which means no automatic conflict resolution */
> > + return svn_accept_default;
>
> This comment is wrong, as the only svn_accept_from_word caller
> treats svn_accept_default as an error.
>
> > Index: subversion/svn/main.c
>
> > @@ -1381,6 +1385,21 @@
> > case 'g':
> > opt_state.use_merge_history = TRUE;
> > break;
> > + case svn_cl__accept_opt:
> > + opt_state.accept = svn_accept_from_word(opt_arg);
> > +
> > + /* We need to make sure that the value passed to the accept flag
> > + * was one of the three expected. Since default is what gets
> > + * set when one of the three expected are not passed, checking for
> > + * this as part of the command line parsing makes sense. */
> > + if (opt_state.accept == svn_accept_default)
> > + {
> > + return svn_cmdline_handle_exit_error
> > + (svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("'%s' is not a valid accept value; try "
> > + "'orig', 'mine', or 'base'"),
> > + opt_arg), pool, "svn: ");
> > + }
>
> You've conflated default behavior with malformed input. I
> preferred how you were using svn_accept_unknown for this earlier
> (though i think svn_accept_invalid is a better name).
>
> --
> Eric Gillespie <*> epg@pretzelnet.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> [[[
> Add option to resolve conflicts by selecting a specific file by adding
> support to the svn executable to handle the "--accept" flag. The
> "resovled" subcommand is the only subcommand using this new flag as
^^^^^^^^
resolved
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 25291)
> +++ subversion/include/svn_types.h (working copy)
> @@ -199,6 +199,34 @@
> svn_recursive
> };
>
> +/** The concept of automatic conflict resolution.
> + *
> + * @since New in 1.5
> + */
> +typedef enum
> +{
I think you want 'svn_accept_invalid = -1' here; see below.
> + /* Resolve the conflict as usual */
> + svn_accept_default,
> +
> + /* Resolve the conflict with the pre-update base file */
> + svn_accept_orig,
> +
> + /* Resolve the conflict with the pre-update working copy file */
> + svn_accept_mine,
> +
> + /* Resolve the conflict with the during-update repository file, new base */
> + svn_accept_base,
> +
> +} svn_accept_t;
Unfortunately, i don't think we've finished painting this
particular bikeshed yet ;->. I finally went and got myself a
conflict so i could see if we could take name inspiration from
there. So, how about svn_accept_{old,new,work} with strings
"left", "new", and "working"?
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 25291)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -2474,6 +2474,23 @@
> void *cancel_baton,
> apr_pool_t *pool);
>
> +/**
> + * Just like "" but with the ability to pass an svn_accept_t argument
> + * for automatical conflict resolution handling.
> + *
> + * @since New in 1.5
You have this backwards. Move the svn_wc_resolved_conflict2
docstring here and document the new parameter. Then add a
"similar to"/deprecated comment for the old one; see
svn_wc_adm_open3 for example. svn_client_resolved2, too.
> Index: subversion/libsvn_wc/adm_ops.c
> @@ -2519,6 +2524,25 @@
> svn_boolean_t was_present, need_feedback = FALSE;
> apr_uint64_t modify_flags = 0;
> svn_wc_entry_t *entry = svn_wc_entry_dup(orig_entry, pool);
> +
> + /* Handle automatic conflict resolution before the temporary files are
> + * deleted. */
> + if (accept != svn_accept_default)
> + {
> + const char *base_name;
> +
> + if (accept == svn_accept_orig) {
> + base_name = entry->conflict_old;
> + } else if (accept == svn_accept_mine) {
> + base_name = entry->conflict_wrk;
> + } else if (accept == svn_accept_base) {
> + base_name = entry->conflict_new;
> + } /* Should never be anything else thanks to the svn_accept_from_word() */
Not all callers use svn_accept_from_word; maybe just say "Ignore
invalid values". If someone passes some random value instead of
one of the svn_accept_ values, that's his problem.
> @@ -2594,6 +2618,8 @@
> svn_boolean_t resolve_text;
> /* TRUE if property conflicts are to be resolved. */
> svn_boolean_t resolve_props;
> + /* Placeholder for the type of automatic conflict resolution to perform */
> + svn_accept_t accept;
Drop "Placeholder for the".
> Index: subversion/libsvn_subr/kitchensink.c
> ===================================================================
> --- subversion/libsvn_subr/kitchensink.c (revision 25291)
> +++ subversion/libsvn_subr/kitchensink.c (working copy)
> @@ -32,6 +32,18 @@
> return uuid_str;
> }
>
> +svn_accept_t
> +svn_accept_from_word(const char *word)
> +{
> + if (strcmp(word, "orig") == 0)
> + return svn_accept_orig;
> + if (strcmp(word, "mine") == 0)
> + return svn_accept_mine;
> + if (strcmp(word, "base") == 0)
> + return svn_accept_base;
> + /* Return default, which means no automatic conflict resolution */
> + return svn_accept_default;
This comment is wrong, as the only svn_accept_from_word caller
treats svn_accept_default as an error.
> Index: subversion/svn/main.c
> @@ -1381,6 +1385,21 @@
> case 'g':
> opt_state.use_merge_history = TRUE;
> break;
> + case svn_cl__accept_opt:
> + opt_state.accept = svn_accept_from_word(opt_arg);
> +
> + /* We need to make sure that the value passed to the accept flag
> + * was one of the three expected. Since default is what gets
> + * set when one of the three expected are not passed, checking for
> + * this as part of the command line parsing makes sense. */
> + if (opt_state.accept == svn_accept_default)
> + {
> + return svn_cmdline_handle_exit_error
> + (svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("'%s' is not a valid accept value; try "
> + "'orig', 'mine', or 'base'"),
> + opt_arg), pool, "svn: ");
> + }
You've conflated default behavior with malformed input. I
preferred how you were using svn_accept_unknown for this earlier
(though i think svn_accept_invalid is a better name).
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Hello All,
Attached is revision 2 of the patch to add automatic conflict
resolution to the resolved command as part of issue 2784. All of
Eric's suggestions have been implemented:
Removed all noticed trailing spaces
Made the --accept flag a long-only flag
Updated the verbiage in the help output and source to match the
acceptable accept flag options
Removed unnecessary/unused code
Refactored for the removal of the unnecessary code
The commit log message for the suggested patch is below, and should be
more inline with the submission process. Thanks for all who have
helped and feel free to request another revision of this patch.
Take care,
Jeremy
[[[
Add option to resolve conflicts by selecting a specific file by adding
support to the svn executable to handle the "--accept" flag. The
"resovled" subcommand is the only subcommand using this new flag as
part of this patch. (Issue 2784)
* subversion/include/svn_client.h
(svn_client_resolved): Revved to svn_client_resolved2() to handle
the svn_accept_t enum.
* subversion/include/svn_types.h
(svn_accept_t): Created an enum to handle the different accept options.
(svn_accept_from_word): Function prototype to return the proper
svn_accept_t based on the accept word.
* subversion/include/svn_wc.h
(svn_wc_resolved_conflict): Revved to svn_wc_resolved_conflict3 to
handle the svn_accept_t enum.
* subversion/libsvn_client/resolved.c
(svn_client_resolved): Implementation of svn_client_resolved2()
which now handles the svn_accept_t as an argument.
* subversion/libsvn_subr/kitchensink.c
(svn_accept_from_word): Implementation of svn_accept_from_word()
which will return an svn_accept_t based on the passed accept string.
* subversion/libsvn_wc/adm_ops.c
(resolve_conflict_on_entry): Updated this function to handle an
svn_accept_t argument. This is also where the real work of handling
the accept flag happens.
(resolve_callback_baton): Updated the structure to have a
placeholder for the svn_accept_t argument so that
resolve_found_entry_callback() can pass the necessary information to
resolve_conflict_on_entry().
(svn_wc_resolved_conflict): Implementation of
svn_wc_resolved_conflict3() which now handles the svn_accept_t as
an argument.
* subversion/svn/cl.h
(svn_cl__opt_state_t): Updated to have a place holder for the accept
flag passed to the svn executable.
(svn_cl__long_opt_t): Updated to handle the svn_accept_t as a long
svn executable argument.
* subversion/svn/main.c
(svn_cl__options): Updated to have all necessary information for the
svn executable to provide help for, and handle, the accept flag in
both long and short format.
(svn_cl__cmd_table): Updated to have the resolved subcommand handle
the passing of the accept flag.
(svn_cl__limit_opt): Added case statement to handle parsing the
accept flag and erroring when an invalid argument was passed. If a
valid argument is passed, it is then turned into the svn_accept_t enum
based on the svn_accept_from_word() function in
subversion/libsvn_subr/kitchensink.c.
* subversion/svn/resolved-cmd.c
(svn_cl__resolved): Updated to call the revved svn_client_resolved()
which is now at svn_client_resolved2().
* subversion/tests/cmdline/basic_test.py
(automatic_conflict_resolution): Created a new test to test the
handling of the new accept flag for the resolved subcommand.
(test_list): Added the automatic_conflict_resolution test to the test list.
]]]
On 6/1/07, Jeremy Whitlock <jc...@gmail.com> wrote:
> Hi All,
> I will come out with a new patch that takes care of the things
> brought up by Eric. Please feel free to make other suggestions as
> well.
>
> Take care,
>
> Jeremy
>
> On 6/1/07, Karl Fogel <kf...@red-bean.com> wrote:
> > "Jeremy Whitlock" <jc...@gmail.com> writes:
> > > Excellent feedback. How do we handle these things? Do I need to
> > > resubmit a new patch?
> >
> > That's the usual way. It's normal for a patch to go through many
> > rounds of review/modify before being accepted into the code base,
> > don't worry.
> >
>
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Hi All,
I will come out with a new patch that takes care of the things
brought up by Eric. Please feel free to make other suggestions as
well.
Take care,
Jeremy
On 6/1/07, Karl Fogel <kf...@red-bean.com> wrote:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
> > Excellent feedback. How do we handle these things? Do I need to
> > resubmit a new patch?
>
> That's the usual way. It's normal for a patch to go through many
> rounds of review/modify before being accepted into the code base,
> don't worry.
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Karl Fogel <kf...@red-bean.com>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> Excellent feedback. How do we handle these things? Do I need to
> resubmit a new patch?
That's the usual way. It's normal for a patch to go through many
rounds of review/modify before being accepted into the code base,
don't worry.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Excellent feedback. How do we handle these things? Do I need to
resubmit a new patch?
On 6/1/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
>
> > Trailing white space: Yeah. That is a good ol' Eclipse feature...I'll
> > remember this next time.
>
> Well, i don't think anyone but me cares.
>
> > svn_accept_unknown being redundant: It isn't. I use it for a few
> > checks throughout the code. If you come into through the command
> > line, this is handled automatically but if you use the libraries, I
> > thought it would be safe to check for it. If not, feel free to remove
> > upon patching.
>
> In resolve_conflict_on_entry, test svn_accept_merged instead of
> unknown or empty. Ah, svn_accept_from_word would need to return
> svn_accept_unknown when it's passed a bogus value.
> svn_cl__resolved need not see it, though.
>
> So, the only two places that should deal with svn_accept_unknown
> are svn_accept_from_word and any caller thereof.
>
> > Negative values: I used the --depth flag as an example. Maybe I
> > should had done something different.
>
> Huh. That looks odd, too. Well, whatever :).
>
> > Code left over from a previous attempt: Not sure. I revved the
> > function as requested.
>
> Requested by whom? I remember you mentioned revving this at one
> point, and i said this function had no business knowing about
> this, and that there should be a baton you could use instead. I
> see that you did take the baton approach, and your revved
> function isn't referenced anywhere at all.
>
> > svn_accept_to_word never used: I followed the --depth flag handling
> > for this as well. I planned on using but I guess it was never
> > required.
>
> That one's used, though.
>
> --
> Eric Gillespie <*> epg@pretzelnet.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> Trailing white space: Yeah. That is a good ol' Eclipse feature...I'll
> remember this next time.
Well, i don't think anyone but me cares.
> svn_accept_unknown being redundant: It isn't. I use it for a few
> checks throughout the code. If you come into through the command
> line, this is handled automatically but if you use the libraries, I
> thought it would be safe to check for it. If not, feel free to remove
> upon patching.
In resolve_conflict_on_entry, test svn_accept_merged instead of
unknown or empty. Ah, svn_accept_from_word would need to return
svn_accept_unknown when it's passed a bogus value.
svn_cl__resolved need not see it, though.
So, the only two places that should deal with svn_accept_unknown
are svn_accept_from_word and any caller thereof.
> Negative values: I used the --depth flag as an example. Maybe I
> should had done something different.
Huh. That looks odd, too. Well, whatever :).
> Code left over from a previous attempt: Not sure. I revved the
> function as requested.
Requested by whom? I remember you mentioned revving this at one
point, and i said this function had no business knowing about
this, and that there should be a baton you could use instead. I
see that you did take the baton approach, and your revved
function isn't referenced anywhere at all.
> svn_accept_to_word never used: I followed the --depth flag handling
> for this as well. I planned on using but I guess it was never
> required.
That one's used, though.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Jeremy Whitlock <jc...@gmail.com>.
Eric,
Replying inline would be hard to follow so please accept my responses here:
Continuing lines need to be indented: Thanks for the tip. I wasn't
sure if it mattered since it was wrapped by my email application but
now I know.
Trailing white space: Yeah. That is a good ol' Eclipse feature...I'll
remember this next time.
svn_accept_unknown being redundant: It isn't. I use it for a few
checks throughout the code. If you come into through the command
line, this is handled automatically but if you use the libraries, I
thought it would be safe to check for it. If not, feel free to remove
upon patching.
Negative values: I used the --depth flag as an example. Maybe I
should had done something different.
Match enum values in comment: I agree. I overlooked this when
refactoring as the original suggestion was to use orig, mine and repo
which repo was later changed to base.
Code left over from a previous attempt: Not sure. I revved the
function as requested.
Unrelated whitespace change: Another Eclipse feature.
svn_accept_to_word never used: I followed the --depth flag handling
for this as well. I planned on using but I guess it was never
required.
Thanks for reviewing.
Take care,
Jeremy
On 6/1/07, Eric Gillespie <ep...@pretzelnet.org> wrote:
> "Jeremy Whitlock" <jc...@gmail.com> writes:
>
> > Hi All,
> > Attached is the patch used to resolve issue 2784. Here is a synopsis:
> >
> > This patch adds the "--accept" or "-a" flag support to the Subversion
> > command line. Right now, only resolved subcommand is using it. This
> > new flag will also be used by the following subcommands:
>
> I really don't think we should start out with a short option, if
> we need one at all.
>
> > [[[
> > Add option to resolve conflicts by selecting a specific file by adding
> > support to the svn executable to handle the "--accept" flag. The
> > "resovled" subcommand is the only subcommand using this new flag as
> > part of this patch. (Issue 2784)
> >
> > * subversion/include/svn_client.h
> > (svn_client_resolved): Revved to svn_client_resolved2() to handle
> > the svn_accept_t enum.
>
> Continuing lines should be indented with the rest.
>
> Now, for the patch. Looks good! I don't think the Subversion
> project cares about random trailing white space, since the code
> is *full* of it. But it drives me nuts. Your patch is full of
> it. Vim folks can get spacehi.vim, emacs folks can (setq
> show-trailing-whitespace t).
>
> > Index: subversion/include/svn_types.h
> > ===================================================================
> > --- subversion/include/svn_types.h (revision 25247)
> > +++ subversion/include/svn_types.h (working copy)
> > @@ -199,6 +199,48 @@
> > svn_recursive
> > };
> >
> > +/** The concept of automatic conflict resolution.
> > + *
> > + * @since New in 1.5
> > + */
> > + typedef enum
> > + {
> > + /* The order of the accept options is not important */
> > +
> > + /* Accept option unknown or ignored */
> > + svn_accept_unknown = -2,
>
> It looks like this is redundant. Perhaps rename svn_accept_empty
> to svn_accept_merged (--accept=merged == leave conflicted, the
> default) and use that instead?
>
> > + /* Resolve the conflict with the pre-update base file */
> > + svn_accept_orig = -1,
>
> And why are these negative? Why specify values at all?
>
> > + /* Resolve the conflict the usual way */
> > + svn_accept_empty = 0,
> > +
> > + /* Resolve the conflict with the pre-update working copy file */
> > + svn_accept_mine = 1,
> > +
> > + /* Resolve the conflict with the during-update repository file, new base */
> > + svn_accept_base = 2,
> > +
> > + } svn_accept_t;
> > +
> > + /** Return a constant string expressing @a accept as an English word,
> > + * e.g., "orig", "mine", or "repo". The string is not localized.
>
> I think these words should match the enum values. I don't like
> the words you've chosen, either. "orig" and "repo" are both from
> the repository. I believe the standard terms are "orig",
> "theirs", and "mine". At least, that's what i see most places,
> including Perforce.
>
> Actually, it looks like Perforce says "base". I think "orig" is
> more common, but maybe i'm imagining things...
>
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h (revision 25247)
> > +++ subversion/include/svn_wc.h (working copy)
> > @@ -1583,7 +1583,8 @@
> > * be a file or dir. Call callbacks in @a walk_callbacks, passing
> > * @a walk_baton to each. Use @a pool for looping, recursion, and to
> > * allocate all entries returned. @a adm_access must be an access baton
> > - * for @a path.
> > + * for @a path. The @a accept argument is used passed to @a walk_callbacks
> > + * for automatic conflict resolution support.
> > *
> > * If @a cancel_func is non-null, call it with @a cancel_baton to determine
> > * if the client has cancelled the operation.
> > @@ -1603,6 +1604,23 @@
> > *
> > * @since New in 1.5.
> > */
> > +svn_error_t *svn_wc_walk_entries4(const char *path,
> > + svn_wc_adm_access_t *adm_access,
> > + const svn_wc_entry_callbacks2_t
> > + *walk_callbacks,
> > + void *walk_baton,
> > + svn_boolean_t show_hidden,
> > + svn_accept_t accept,
> > + svn_cancel_func_t cancel_func,
> > + void *cancel_baton,
> > + apr_pool_t *pool);
> > +
> > +/**
> > + * Similar to svn_wc_walk_entries4(), but without automatic conflict
> > + * resolution support.
> > + *
> > + * @deprecated Provided for backward compatibility with the 1.4 API.
> > + */
> > svn_error_t *svn_wc_walk_entries3(const char *path,
> > svn_wc_adm_access_t *adm_access,
> > const svn_wc_entry_callbacks2_t
>
> I guess this is left-over from a previous attempt?
>
> > Index: subversion/libsvn_wc/adm_ops.c
> > ===================================================================
> > --- subversion/libsvn_wc/adm_ops.c (revision 25247)
> > +++ subversion/libsvn_wc/adm_ops.c (working copy)
>
> > @@ -2687,7 +2733,6 @@
> > SVN_ERR(svn_wc_walk_entries2(path, adm_access,
> > &resolve_walk_callbacks, baton,
> > FALSE, cancel_func, cancel_baton, pool));
> > -
> > }
> >
> > return SVN_NO_ERROR;
>
> unrelated whitespace change
>
> > Index: subversion/libsvn_subr/kitchensink.c
> > ===================================================================
> > --- subversion/libsvn_subr/kitchensink.c (revision 25247)
> > +++ subversion/libsvn_subr/kitchensink.c (working copy)
> > @@ -32,7 +32,44 @@
> > return uuid_str;
> > }
> >
> > +const char *
> > +svn_accept_to_word(svn_accept_t accept)
>
> This is never used.
>
> --
> Eric Gillespie <*> epg@pretzelnet.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Jeremy Whitlock" <jc...@gmail.com> writes:
> Hi All,
> Attached is the patch used to resolve issue 2784. Here is a synopsis:
>
> This patch adds the "--accept" or "-a" flag support to the Subversion
> command line. Right now, only resolved subcommand is using it. This
> new flag will also be used by the following subcommands:
I really don't think we should start out with a short option, if
we need one at all.
> [[[
> Add option to resolve conflicts by selecting a specific file by adding
> support to the svn executable to handle the "--accept" flag. The
> "resovled" subcommand is the only subcommand using this new flag as
> part of this patch. (Issue 2784)
>
> * subversion/include/svn_client.h
> (svn_client_resolved): Revved to svn_client_resolved2() to handle
> the svn_accept_t enum.
Continuing lines should be indented with the rest.
Now, for the patch. Looks good! I don't think the Subversion
project cares about random trailing white space, since the code
is *full* of it. But it drives me nuts. Your patch is full of
it. Vim folks can get spacehi.vim, emacs folks can (setq
show-trailing-whitespace t).
> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 25247)
> +++ subversion/include/svn_types.h (working copy)
> @@ -199,6 +199,48 @@
> svn_recursive
> };
>
> +/** The concept of automatic conflict resolution.
> + *
> + * @since New in 1.5
> + */
> + typedef enum
> + {
> + /* The order of the accept options is not important */
> +
> + /* Accept option unknown or ignored */
> + svn_accept_unknown = -2,
It looks like this is redundant. Perhaps rename svn_accept_empty
to svn_accept_merged (--accept=merged == leave conflicted, the
default) and use that instead?
> + /* Resolve the conflict with the pre-update base file */
> + svn_accept_orig = -1,
And why are these negative? Why specify values at all?
> + /* Resolve the conflict the usual way */
> + svn_accept_empty = 0,
> +
> + /* Resolve the conflict with the pre-update working copy file */
> + svn_accept_mine = 1,
> +
> + /* Resolve the conflict with the during-update repository file, new base */
> + svn_accept_base = 2,
> +
> + } svn_accept_t;
> +
> + /** Return a constant string expressing @a accept as an English word,
> + * e.g., "orig", "mine", or "repo". The string is not localized.
I think these words should match the enum values. I don't like
the words you've chosen, either. "orig" and "repo" are both from
the repository. I believe the standard terms are "orig",
"theirs", and "mine". At least, that's what i see most places,
including Perforce.
Actually, it looks like Perforce says "base". I think "orig" is
more common, but maybe i'm imagining things...
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 25247)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1583,7 +1583,8 @@
> * be a file or dir. Call callbacks in @a walk_callbacks, passing
> * @a walk_baton to each. Use @a pool for looping, recursion, and to
> * allocate all entries returned. @a adm_access must be an access baton
> - * for @a path.
> + * for @a path. The @a accept argument is used passed to @a walk_callbacks
> + * for automatic conflict resolution support.
> *
> * If @a cancel_func is non-null, call it with @a cancel_baton to determine
> * if the client has cancelled the operation.
> @@ -1603,6 +1604,23 @@
> *
> * @since New in 1.5.
> */
> +svn_error_t *svn_wc_walk_entries4(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + const svn_wc_entry_callbacks2_t
> + *walk_callbacks,
> + void *walk_baton,
> + svn_boolean_t show_hidden,
> + svn_accept_t accept,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool);
> +
> +/**
> + * Similar to svn_wc_walk_entries4(), but without automatic conflict
> + * resolution support.
> + *
> + * @deprecated Provided for backward compatibility with the 1.4 API.
> + */
> svn_error_t *svn_wc_walk_entries3(const char *path,
> svn_wc_adm_access_t *adm_access,
> const svn_wc_entry_callbacks2_t
I guess this is left-over from a previous attempt?
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 25247)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -2687,7 +2733,6 @@
> SVN_ERR(svn_wc_walk_entries2(path, adm_access,
> &resolve_walk_callbacks, baton,
> FALSE, cancel_func, cancel_baton, pool));
> -
> }
>
> return SVN_NO_ERROR;
unrelated whitespace change
> Index: subversion/libsvn_subr/kitchensink.c
> ===================================================================
> --- subversion/libsvn_subr/kitchensink.c (revision 25247)
> +++ subversion/libsvn_subr/kitchensink.c (working copy)
> @@ -32,7 +32,44 @@
> return uuid_str;
> }
>
> +const char *
> +svn_accept_to_word(svn_accept_t accept)
This is never used.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org