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

[Olaf Conradi]

On 18/03/06, Michael Ellerman <michael at ellerman.id.au> wrote:
> Hi Olaf,
>
> Nice work, a few minor comments below ..

I looked at cmd_pull how it implements remember and did more or less the same.

>
> 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.

> >      The branch is the branch you will merge from.  By default, it will
> >      merge the latest revision.  If you specify a revision, that
> > @@ -1812,16 +1816,17 @@
> >      --force is given.
> >      """
> >      takes_args = ['branch?']
> > -    takes_options = ['revision', 'force', 'merge-type', 'reprocess',
> > +    takes_options = ['remember', 'revision', 'force', 'merge-type',
> > 'reprocess',
>
> I'd normally put the new option at the end of the list, but it's not
> that important.

Same reason as above actually :)

> > -    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.

> > -            branch = WorkingTree.open_containing(u'.')[0].branch.get_parent()
> > +            branch = tree.branch.get_parent()
>
> I know the current code uses branch, but it might be clearer to call
> it other_branch while you're changing this code.
>
> >              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.

I'll put the remember option last in the list of options and move the
set_parent above the conflict check. Shall I leave the rest?

Thanks for your comments

 -Olaf