[RFC] Why for extmerge plugin needed for QBzr?

[Alexander Belchenko]

Hi all,

I'm very disappointed on our internal model to work with external diff
and merge tools. I've just filed the bug:

https://bugs.launchpad.net/qbzr/+bug/489915

There is many similar bug reports about qconfig etc.
I'd like to change this bad situation.

I have a important question though.
Does QBzr rely on extmerge, and if yes then how?

In my opinion, if we need some functions from extmerge (or difftools to
look broader) we just need to merge these tools into our codebase and
adapt them to our needs. Rely on them as external plugins and provide 2
codepaths for the case when they are installed or not is just waste oof
our efforts. If we need them, let's merge them in. If they will change
in future, we pull latest changes. Let's use them as upstream for us.
Like bzrlib uses ConfigObj library.

All plugins are licensed under GPL. So there is no problems to merge in
my understanding. Does merging other plugins to QBzr will be
infringement of open-source etiquette? Should we beg for approval from
their authors? I hope it's not.

Thoughts?

Alexander.

[Nicholas Allen]

I agree with you 100%. It's weird to have to install ext diff plugin for
Qbzr to be able to launch an external diff viewer. This should be
supported out of the box in QBzr.

Cheers,

Nick

[craig Hewetson]

Hi Alexander

IMO this has more to do with extmerge 3 way merging other than external diff-ing. Not all external diff tools support 3 way merging.

>>Does QBzr rely on extmerge, and if yes then how?

I make use of p4merge for 3-way merging when I get conflicts. IMO its one of the best free tools around for 3 way merging (at least when using linux). So the only way I can use it from the qconflicts dialog is with extmerge plugin installed. Extmerge basically allows users to specify the external merge tool to use and the order in which the BASE, OTHER and THIS files must be passed to it.

I don't like the idea of installing yet another Bazaar plugin, so if you, or anyone, wants to merge the extmerge code into QBzr's codebase I'll support it.
It would be nice to modify qconfig so the user can specify in which order the THIS, BASE and OTHER files must be passed to the configured merge tool in a more user friendly way. Currently the user must specify a string eg:

p4merge %b %o %t %r

Which isn't really that user friendly :)

[Alexander Belchenko]

craig Hewetson пишет:

That's why I want to have internal registry of known tools and required
params for them. So user will need to only select p4merge from the list
of known tools and nothing more. All this "%b %o %t %r" things is our
internal job, and it should not be exposed to user, IMO.

[craig Hewetson]

That's why I want to have internal registry of known tools and required params for them. So user will need to only select p4merge from the list of known tools and nothing more. All this "%b %o %t %r" things is our internal job, and it should not be exposed to user, IMO.

I fully agree with you, I was just answering your question on why its there and why its currently needed.
I'm 100% behind simplifing it for the end user and moving extmerge code into qbzr.

[Alexander Belchenko]

craig Hewetson пишет:

> Hi Alexander
>
> IMO this has more to do with extmerge 3 way merging other than external
> diff-ing. Not all external diff tools support 3 way merging.

I understand, but configuring diff tools is also pain on Windows because there is no Browse button
to select application executable path.

>>>Does QBzr rely on extmerge, and if yes then how?
> I make use of p4merge for 3-way merging when I get conflicts. IMO its
> one of the best free tools around for 3 way merging (at least when using
> linux). So the only way I can use it from the qconflicts dialog is with
> extmerge plugin installed. Extmerge basically allows users to specify
> the external merge tool to use and the order in which the BASE, OTHER
> and THIS files must be passed to it.
>
> I don't like the idea of installing yet another Bazaar plugin, so if
> you, or anyone, wants to merge the extmerge code into QBzr's codebase
> I'll support it.
> It would be nice to modify qconfig so the user can specify in which
> order the THIS, BASE and OTHER files must be passed to the configured
> merge tool in a more user friendly way. Currently the user must specify
> a string eg:
>
> p4merge %b %o %t %r
>
> Which isn't really that user friendly :)

OK, so here is fast mockup of what I'd like to have for configure merge tool dialog, see attached
picture. Similar dialog (but without Arguments part) will be used to configure external differs.

The idea here as following:

Merge tool (Diff tool) combobox will be used to select one of preconfigured tool.
Based on that user choice we automatically fill Application and Arguments controls with appropriate
values.

User can change these values though. If user will change Arguments then we change name of merge tool
(upper combobox) to something like "$tool (custom)", e.g. "p4merge (custom)".

Something like that.

Also as you see I'd like to use more intuitive variables for Arguments, e.g. $THIS, $BASE, $OTHER
and $RESULT instead of scripting %t %b %o %r (which reminds me makefile language too much). We can
support extmerge syntax transparently for user though, but won't force the user to learn it.

I'm about to clone and read extmerge code right now.



__________ Information from ESET NOD32 Antivirus, version of virus signature database 4640 (20091126) __________

The message was checked by ESET NOD32 Antivirus.

http://www.esetnod32.ru

[craig Hewetson]

Looks good.

Will the "Application:" text box ALSO be editable so that the user can just specify the name of the exe without needing to browse to the full installation path?
Therefore if the exe is in the user's PATH he doesn't have to browse for it. When installing apps in linux I don't really care where its installed as long as I can execute it by name. NOTE: I'm not saying that the browse button should be removed, just that the text box should be editable.

The Arguments text/combo looks good. I suppose the "$" characters could also be removed. Just specifying THIS BASE OTHER RESULT might be good enough.

Will this dialog be opened from qconfig?

[Ian Clatworthy]

Alexander Belchenko wrote:

> OK, so here is fast mockup of what I'd like to have for configure merge tool dialog, see attached
> picture. Similar dialog (but without Arguments part) will be used to configure external differs.

+1.

From qconfig, they should just select from a list of known merge or diff
tools. If they want a new one, they register it as merge or diff tool
IMO using your dialogs, right?

Ian C.

[Alexander Belchenko]

Ian Clatworthy пишет:

Yes, all controls there will be editable, so one can enter any custom configuration.

[Simon Kersey]

On Nov 29, 11:47 pm, Alexander Belchenko <bia...@ukr.net> wrote:
> Ian Clatworthy пишет:
>
> > Alexander Belchenko wrote:
>
> >> OK, so here is fast mockup of what I'd like to have for configure merge tool dialog, see attached
> >> picture. Similar dialog (but without Arguments part) will be used to configure external differs.
>
> > +1.
>
> >>From qconfig, they should just select from a list of known merge or diff
> > tools. If they want a new one, they register it as merge or diff tool
> > IMO using your dialogs, right?
>
> Yes, all controls there will be editable, so one can enter any custom configuration.
>

As someone who uses WinMerge a lot I would like to suggest adding an
alternative interaction model (accessible by checkbox on proposed
dialog?). Instead of invoking the external diff tool for each file I
would like the option to extract all changed files to temp old and new
directories preserving their workingtree dir structure and then invoke
the external tool once on these directories.

[Gary van der Merwe]

* I have not read the whole thread yet.

On Sun, Nov 29, 2009 at 2:37 PM, Alexander Belchenko <bia...@ukr.net> wrote:
> Hi all,
>
> I'm very disappointed on our internal model to work with external diff and
> merge tools. I've just filed the bug:
>
> https://bugs.launchpad.net/qbzr/+bug/489915
>
> There is many similar bug reports about qconfig etc.
> I'd like to change this bad situation.
>
> I have a important question though.
> Does QBzr rely on extmerge, and if yes then how?

The TreeWidget (qcommit/qbrowse) runs "bzr extmerge FILES" via the
subprocess interface.

qconflicts runs "bzr extmerge FILES" in some circumstances, and in
others launches the merge app directly. I can't remember the details
of where it does what.

I would like to see these behave in the same way, and hopefully share
code. qconflicts could maybe be made to use the TreeWidget.

When I added the functionality to the TreeWidget, I use extmerge
because I could reuse the subprocess work I did for extdiff. The
important stuff here is the error reporting (which use to be flaky any
way.) One of the ideas that I had at the time was that we could make
qbzr work with extdiffs that output to stdout, and this would display
in the subprocess dialog. No one has asked for the, so I guess it is
not that important.

> In my opinion, if we need some functions from extmerge (or difftools to look
> broader) we just need to merge these tools into our codebase and adapt them
> to our needs. Rely on them as external plugins and provide 2 codepaths for
> the case when they are installed or not is just waste oof our efforts. If we
> need them, let's merge them in. If they will change in future, we pull
> latest changes. Let's use them as upstream for us.
> Like bzrlib uses ConfigObj library.

difftools has 2 features that core bzr diff --using does not have,
that are cool, and a lack of a feature that is uncool:

* When diffing the working tree, it passes the working tree files to
the extdiff. This allows you to edit these files in the extdiff. The
bzr core bzr diff --using passes temp files, which you can't do any
thing with. This is something that that I think should really be
ported to the core bzr diff --using implementation

* With difftools, if you don't specifiy any files, it passes the whole
directory.

* difftools lacks --new and --old arguments. Which sucks.

So I think for diff, we could reuse the core api (not sub process) to
create the temp files for non working tree files, be we should the
launch the ext diff app our selfs.

For extmerge, I have no problem with doing this. This would allow us
to also make it possible to setup multiple merge apps, that you could
select through the ui. (like ext diff).

> All plugins are licensed under GPL. So there is no problems to merge in my
> understanding. Does merging other plugins to QBzr will be infringement of
> open-source etiquette? Should we beg for approval from their authors? I hope
> it's not.

I don't think we need approval, it would be fine to just let them
know. For extmerge, the code is very small, and I think that 90% of it
will end up getting changed for our needs.




One thing that I would like our diff and extmerge to do, is to get the
apps to open multiple files at once in the apps mdi/tabed interface.
I'm not sure how to exactly how to do this. (This is why I like the
tree behaviour of difftools, but I would prefer it to just open all of
the changed files)

[Martin Pool]

2009/11/29 Alexander Belchenko <bia...@ukr.net>:

> In my opinion, if we need some functions from extmerge (or difftools to look
> broader) we just need to merge these tools into our codebase and adapt them
> to our needs. Rely on them as external plugins and provide 2 codepaths for
> the case when they are installed or not is just waste oof our efforts. If we
> need them, let's merge them in. If they will change in future, we pull
> latest changes. Let's use them as upstream for us.
> Like bzrlib uses ConfigObj library.

Actually I'd really like to merge them into bzrlib and have qbzr use
them from there. I haven't looked at what code changes would be
needed, but I will help you make it happen. I don't know of any
technical or architectural reason we can't do this.

--
Martin <http://launchpad.net/~mbp/>

[Alexander Belchenko]

Gary van der Merwe пишет:

> * I have not read the whole thread yet.
>
> On Sun, Nov 29, 2009 at 2:37 PM, Alexander Belchenko <bia...@ukr.net> wrote:
>> I'm very disappointed on our internal model to work with external diff and
>> merge tools. I've just filed the bug:
>>
>> https://bugs.launchpad.net/qbzr/+bug/489915
>>
>> There is many similar bug reports about qconfig etc.
>> I'd like to change this bad situation.
>>
>> I have a important question though.
>> Does QBzr rely on extmerge, and if yes then how?
>
> The TreeWidget (qcommit/qbrowse) runs "bzr extmerge FILES" via the
> subprocess interface.

extmerge in fact is also run actual merge tool via subprocess (subprocess.call). So implementing the
inside qbzr won't be many work, but provides as better configuration capabilities.

Also I'm poking about ideas of automatic support of 3-way and 2-way merges as well. Because after
merge --weave (or --lca) there is no BASE file, so we'd like to run 2-way merge instead. Many merge
tools supports this mode too.

> qconflicts runs "bzr extmerge FILES" in some circumstances, and in
> others launches the merge app directly. I can't remember the details
> of where it does what.
>
> I would like to see these behave in the same way, and hopefully share
> code. qconflicts could maybe be made to use the TreeWidget.

That's approx what I have in mind when said that we should reconcile our internal model: we should
have one place to manage settings and use these settings to run merge tools.

> When I added the functionality to the TreeWidget, I use extmerge
> because I could reuse the subprocess work I did for extdiff. The
> important stuff here is the error reporting (which use to be flaky any
> way.) One of the ideas that I had at the time was that we could make
> qbzr work with extdiffs that output to stdout, and this would display
> in the subprocess dialog. No one has asked for the, so I guess it is
> not that important.

As I said I don't see reasons to not move the same approach into qbzr, and still continue using
subprocess. Just call the tool directly via subprocess (or maybe better QProcess)

>> In my opinion, if we need some functions from extmerge (or difftools to look
>> broader) we just need to merge these tools into our codebase and adapt them
>> to our needs. Rely on them as external plugins and provide 2 codepaths for
>> the case when they are installed or not is just waste oof our efforts. If we
>> need them, let's merge them in. If they will change in future, we pull
>> latest changes. Let's use them as upstream for us.
>> Like bzrlib uses ConfigObj library.
>
> difftools has 2 features that core bzr diff --using does not have,
> that are cool, and a lack of a feature that is uncool:
>
> * When diffing the working tree, it passes the working tree files to
> the extdiff. This allows you to edit these files in the extdiff. The
> bzr core bzr diff --using passes temp files, which you can't do any
> thing with. This is something that that I think should really be
> ported to the core bzr diff --using implementation

Agreed. Built-in diff --using has many limitations, which very unpleasant especially on Windows.

> * With difftools, if you don't specifiy any files, it passes the whole
> directory.

I've read the code of difftools and as I remember it has nice registry of knwon tools, so difftools
"knows" which tools supports diff of the trees. But this support has one serious flow IMO: there is
need some hacking to support renames. External diff tools knows nothing about rename of file in bzr,
so when you just dump revision tree to temp directory and run diff between revision tree and working
tree (as whole directories) you'll end up with situation: file foo deleted, file bar added when foo
renamed to bar.

This is hard problem. What you think about it?

In my opinion we at least can ask user, does she want to follow renames, and when dump revision
tree, we should rename files to match working tree.

> * difftools lacks --new and --old arguments. Which sucks.

Why for it's needed?

> So I think for diff, we could reuse the core api (not sub process) to
> create the temp files for non working tree files, be we should the
> launch the ext diff app our selfs.

As I wrote above we may want to adapt the code to create temp files to follow renames.

> For extmerge, I have no problem with doing this. This would allow us
> to also make it possible to setup multiple merge apps, that you could
> select through the ui. (like ext diff).

I'm not sure why you think we need support multiple choices for tools? Can you explain?

>> All plugins are licensed under GPL. So there is no problems to merge in my
>> understanding. Does merging other plugins to QBzr will be infringement of
>> open-source etiquette? Should we beg for approval from their authors? I hope
>> it's not.
>
> I don't think we need approval, it would be fine to just let them
> know. For extmerge, the code is very small, and I think that 90% of it
> will end up getting changed for our needs.

Yep.

> One thing that I would like our diff and extmerge to do, is to get the
> apps to open multiple files at once in the apps mdi/tabed interface.
> I'm not sure how to exactly how to do this. (This is why I like the
> tree behaviour of difftools, but I would prefer it to just open all of
> the changed files)

I think this is out of our control, so I don't see a reason to put efforts here.

[Alexander Belchenko]

Martin Pool пишет:

Extmerge is very small plugin, and if we merge/reimplement it inside qbzr it won't be much work.

Difftools is another case, but as Gary pointed in his mails it does not support --old and --new
which seems important sometimes. Also built-in diff --using written by Aaron does not allow diff
against real working tree for the reasons I don't understand. And 3rd: there is problem with diff
over trees with renames. I think all mentioned obstacles will require more love than you think. So
if we implement them inside qbzr first and then will talk how to move them into core, it would be
more productive for us.

[Alexander Belchenko]

Simon Kersey пишет:

This mode is already supported if you have difftools plugin installed.

That's why I'm talking about the option to merge difftools into qbzr.

[Alexander Belchenko]

BTW, it's a shame that we don't have fallback to default external merge tool yet. And I talk not
about meld, which most likely absent on Windows. There is already configured cross-platform merge
resolution tool for any user on any platform: editor option from bazaar.conf.

[Gary van der Merwe]

On Tue, Dec 1, 2009 at 6:51 AM, Alexander Belchenko <bia...@ukr.net> wrote:
> Gary van der Merwe пишет:

>> * With difftools, if you don't specifiy any files, it passes the whole
>> directory.

This is actually not that important for me. I only like it because it
allows me to open multiple files at once.

>> * difftools lacks --new and --old arguments. Which sucks.
>
> Why for it's needed?

It makes it possible to diff revisions from different branches.

>> For extmerge, I have no problem with doing this. This would allow us
>> to also make it possible to setup multiple merge apps, that you could
>> select through the ui. (like ext diff).
>
> I'm not sure why you think we need support multiple choices for tools? Can you explain?

On ubuntu, I some times use meld, and some times Kompare. On windows I
some times use Araxis Merge, and some times SourceGear DiffMerge. This
would be a nice to have, but not important.

[Alexander Belchenko]

Gary van der Merwe пишет:
> On Tue, Dec 1, 2009 at 6:51 AM, Alexander Belchenko <bia...@ukr.net> wrote:
>> Gary van der Merwe пишет:
>>> * With difftools, if you don't specifiy any files, it passes the whole
>>> directory.
>
> This is actually not that important for me. I only like it because it
> allows me to open multiple files at once.

In one window?

>>> * difftools lacks --new and --old arguments. Which sucks.
>> Why for it's needed?
>
> It makes it possible to diff revisions from different branches.

One more reason to have internal implementation for this, I guess.

>>> For extmerge, I have no problem with doing this. This would allow us
>>> to also make it possible to setup multiple merge apps, that you could
>>> select through the ui. (like ext diff).
>> I'm not sure why you think we need support multiple choices for tools? Can you explain?
>
> On ubuntu, I some times use meld, and some times Kompare. On windows I
> some times use Araxis Merge, and some times SourceGear DiffMerge. This
> would be a nice to have, but not important.

I need to think about this more. I have sketch of new configure dialog
in my mind, but it's good only for the case when there is only one
supported tool.

For many tools I need another sketch.

[Gary van der Merwe]

On Tue, Dec 1, 2009 at 10:50 AM, Alexander Belchenko <bia...@ukr.net> wrote:
> Gary van der Merwe пишет:
>>>> * With difftools, if you don't specifiy any files, it passes the whole
>>>> directory.
>> This is actually not that important for me. I only like it because it
>> allows me to open multiple files at once.
>
> In one window?

Yes.

>>>> For extmerge, I have no problem with doing this. This would allow us
>>>> to also make it possible to setup multiple merge apps, that you could
>>>> select through the ui. (like ext diff).
>>>
>>> I'm not sure why you think we need support multiple choices for tools?
>>> Can you explain?
>>
>> On ubuntu, I some times use meld, and some times Kompare. On windows I
>> some times use Araxis Merge, and some times SourceGear DiffMerge. This
>> would be a nice to have, but not important.
>
> I need to think about this more. I have sketch of new configure dialog in my
> mind, but it's good only for the case when there is only one supported tool.
>
> For many tools I need another sketch.

Like I said, this is may be not to important.

[Alexander Belchenko]

I can imagine following need to have different tools: to handle
different file types, e.g. separate tools to merge images, office
documents or just XML files. But this not quite "several generic merge
tools", in my case we may want to have file extensions associations
between file types and tools.