[MERGE RFC] Added option --remember to merge command

[Michael Ellerman]

On 3/18/06, Olaf Conradi <oohlaf at gmail.com> wrote:
> On 18/03/06, Michael Ellerman <michael at ellerman.id.au> wrote:
> > On 3/18/06, Olaf Conradi <oohlaf at gmail.com> wrote:
> > > === modified file 'a/bzrlib/builtins.py'
> > > --- a/bzrlib/builtins.py
> > > +++ b/bzrlib/builtins.py
> > > @@ -1779,6 +1779,10 @@
> > >
> > >  class cmd_merge(Command):
> > >      """Perform a three-way merge.
> > > +
> > > +    If there is no default location set, the first merge will set it. After
> > > +    that, you can omit the location to use the default.  To change the
> > > +    default, use --remember.
> >
> > I think you should put this help text below the existing help text,
> > perhaps just before the examples.
>
> I just copied this from cmd_pull, which mentions this as first paragraph.

Ok, in that case we should fix cmd_pull also. That's not the first
thing a user needs to know about pull when they read the help.

> > > -    def run(self, branch=None, revision=None, force=False, merge_type=None,
> > > +    def run(self, branch=None, remember=False, revision=None,
> > > force=False, merge_type=None,
> > >              show_base=False, reprocess=False):
> >
> > However this is not a good idea, by putting remember in front of
> > revision you've changed the order of the arguments. So if someone is
> > calling run() without using keyword arguments they will break
> > (although _probably_ no one is), just put remember as the last option.
>
> Hmm, ok, did not think about that, I just used the same order as pull
> (which also has remember in front of revision). I didn't even know one
> could leave out the keywords. I'll move it to the end.

For this code it's probably ok, because all the callers should be
using the keywords, but in general it's best not to change the order
because you might break existing code that doesn't use the keywords.
Probably best to move it to the end, just for safety.

> > >              if conflict_count != 0:
> > >                  return 1
> > >              else:
> > > +                if tree.branch.get_parent() is None or remember:
> > > +                    tree.branch.set_parent(branch)
> >
> > I actually think you should do this before the if block. Having
> > conflicts in the merge isn't really an error, so it'd be confusing if
> > I did:
> >
> > # bzr merge --remember ../other
> > bzr: conflicts detected in merge, but that's ok, just fix them :D
> > #
> > ...
> > # bzr merge ../other
> > bzr: Error: No merge location known or specified.
> >
> > Because I said --remember and bzr didn't give me an error, so why
> > didn't it remember it!
>
> Ok, I will change this. In my first version I put it in front of merge
> even, but thought what if the location is not reachable or some other
> error occurs, so I moved it down.
>
> You are right though that in case of a merge conflict the location
> should be remembered. I'll move it in front of the conflict if.

Sounds good.

Otherwise it looks good to me.

cheers