wip/soundfont (and wip/ branches)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

wip/soundfont (and wip/ branches)

Tim Janik-6
Hey Stefan,

I've pushed the latest state of my SF2 conflict resolutions and branch work into wip/soundfont:
        https://github.com/tim-janik/beast/tree/wip/soundfont

It doesn't merge cleanly at this point, so will require some work.

Regarding potential for future conflicts, I'm mostly still busy with merging the remaining
set of .proc procedures into bseapi.idl, but don't have much stuff going on in the GUI atm.

As a general note for everyone, wip/ branches are subject to frequent rebasing.

--
Yours sincerely,
Tim Janik

https://testbit.eu/timj/
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: wip/soundfont (and wip/ branches)

Stefan Westerfeld
    Hi!

On Mon, Sep 19, 2016 at 10:34:50AM +0200, Tim Janik wrote:
> I've pushed the latest state of my SF2 conflict resolutions and branch work into wip/soundfont:
> https://github.com/tim-janik/beast/tree/wip/soundfont
>
> It doesn't merge cleanly at this point, so will require some work.
>
> Regarding potential for future conflicts, I'm mostly still busy with merging the remaining
> set of .proc procedures into bseapi.idl, but don't have much stuff going on in the GUI atm.

Ok, you can pull the branch wip/soundfont from

    http://space.twc.de/public/git/stwbeast.git

I squashed the whole SoundFont stuff into one commit, making it easier to merge
for me. It contains

 - my original submission
 - everything from your wip/soundfont

and was rebased to the BEAST master branch, and fixed to the degree to make it compile.

However if you start BEAST, go to SoundFont repository and click load
and load a sound font, this is what happens:

  beast-0.10.1: bseobject.cc:167: void bse_object_init(BseObject*): Assertion `in_bse_object_new' failed.
  Abgebrochen (Speicherabzug geschrieben)

This sounds like something that should be very easy to fix for you, since you
know what changes to the object system you did that caused this problem.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: wip/soundfont (and wip/ branches)

Tim Janik-6
On 20.09.2016 15:48, Stefan Westerfeld wrote:
> I squashed the whole SoundFont stuff into one commit, making it easier to merge
> for me. It contains
>
>  - my original submission
>  - everything from your wip/soundfont
>
> and was rebased to the BEAST master branch, and fixed to the degree to make it compile.

Hi.
I don't like the idea of squashing the branch for the following reasons:
- Squashing looses attribution of the individual changes.
- Branch history is lost, i.e. which linediff can be attributed to which commit (and its stated intent).
- Also, the signoff is now missing from all commits/changes.

So please add your signoff [1] to changes you make (e.g. use git commit -s), and merges you are editing.

Loosing that information can impose a significant drawback if any of the changes has to be debugged.

> However if you start BEAST, go to SoundFont repository and click load
> and load a sound font, this is what happens:
>
>   beast-0.10.1: bseobject.cc:167: void bse_object_init(BseObject*): Assertion `in_bse_object_new' failed.
>   Abgebrochen (Speicherabzug geschrieben)
>
> This sounds like something that should be very easy to fix for you, since you
> know what changes to the object system you did that caused this problem.

And there we go, seems debugging is the next step neccessary...

Since you used git to merge and resolve conflicts, the repository you did the conflict resolutions in
should have kept the preimages and resolutions for all your conflicts in the rerere cache. [2]
So using that same repository, you should now be able to rebase wip/soundfont onto master, preserving
the individual commits, with git automatically applying the resolutions you previously used from its
rerere cache. That should make rebasing without squashing a lot easier.

Or alternatively, keep the branching point of wip/soundfont the same and just merge master into the
branch tip (might make resolution easier for git). - Thinking of it, this approach is probably even
better here, as rebasing requires editing someone else's commits, while merging clearly attributes
the conflict resolution edits and is non-destructive. [3]

>
>    Cu... Stefan
>

[1] Signed-off-by certifies agreement to the Developer Certificate of Origin: http://developercertificate.org/
[2] See also GIT-RERERE(1).
[3] https://www.atlassian.com/git/tutorials/merging-vs-rebasing/conceptual-overview

--
Yours sincerely,
Tim Janik

https://testbit.eu/timj/
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: wip/soundfont (and wip/ branches)

Stefan Westerfeld
   Hi!

On Tue, Sep 20, 2016 at 07:09:52PM +0200, Tim Janik wrote:

> On 20.09.2016 15:48, Stefan Westerfeld wrote:
> > I squashed the whole SoundFont stuff into one commit, making it easier to merge
> > for me. It contains
> >
> >  - my original submission
> >  - everything from your wip/soundfont
> >
> > and was rebased to the BEAST master branch, and fixed to the degree to make it compile.
>
> Hi.
> I don't like the idea of squashing the branch for the following reasons:
> - Squashing looses attribution of the individual changes.
> - Branch history is lost, i.e. which linediff can be attributed to which commit (and its stated intent).
> - Also, the signoff is now missing from all commits/changes.
>
> So please add your signoff [1] to changes you make (e.g. use git commit -s), and merges you are editing.
Ok.

> Loosing that information can impose a significant drawback if any of the changes has to be debugged.

Right. So if preserving commits/author/signoff is important, we can either
rebase or merge. I didn't want to rebase because the problem is that if you
have 10 commits, and are rebasing commit 3, it can be that you fix something
that already has been fixed in some later commit. Therefore squashing appeared
to be the better option.

But ok, merging as described below would also solve my issue, because then the
branch would still be treated as a whole entity (doing rebase follows the
history of the branch step by step which can cause problems).


> > However if you start BEAST, go to SoundFont repository and click load
> > and load a sound font, this is what happens:
> >
> >   beast-0.10.1: bseobject.cc:167: void bse_object_init(BseObject*): Assertion `in_bse_object_new' failed.
> >   Abgebrochen (Speicherabzug geschrieben)
> >
> > This sounds like something that should be very easy to fix for you, since you
> > know what changes to the object system you did that caused this problem.
>
> And there we go, seems debugging is the next step neccessary...

Ok. I know that it didn't fail back then when I wrote the code, so I had hoped
that this was caused by infrastructure changes, and you'd recognize the error
message and know what to do. Well, I'll try to figure out myself what needs to
be changed.

> Since you used git to merge and resolve conflicts, the repository you did the conflict resolutions in
> should have kept the preimages and resolutions for all your conflicts in the rerere cache. [2]
> So using that same repository, you should now be able to rebase wip/soundfont onto master, preserving
> the individual commits, with git automatically applying the resolutions you previously used from its
> rerere cache. That should make rebasing without squashing a lot easier.
>
> Or alternatively, keep the branching point of wip/soundfont the same and just merge master into the
> branch tip (might make resolution easier for git). - Thinking of it, this approach is probably even
> better here, as rebasing requires editing someone else's commits, while merging clearly attributes
> the conflict resolution edits and is non-destructive. [3]

Are you sure? If I merge master into the wip/soundfont branch, as in

  (on wip/soundfont)$ git merge master

then this will generate a commit with houndreds of changes that are completely
unrelated to the soundfont branch (because it will merge every single thing
that has changed in master since then), and among these changes a few that are
related.

I can do this, but it doesn't make the actual merge changes as obvious as for
instance my squash commit or rebasing.

It would work the other way round, by starting from a new branch from master:

  (on master)$ yybranch wip/soundfont-merge
  (on master)$ yywarp wip/soundfont-merge
  (on wip/soundfont-merge)$ git merge wip/soundfont

which is basically the same as just merging the branch into master, except that
it is done in an extra branch, and it would

 - show the changes clearly (unlike merging master into wip/soundfont)
 - not edit or squash old commits (lossless)
 - the resulting branch can be pushed to origin and should merge cleanly
 - however it would not have linear history any longer

It sounds like a plausible way to go.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: wip/soundfont (and wip/ branches)

Tim Janik-6
On 21.09.2016 12:49, Stefan Westerfeld wrote:

>>> However if you start BEAST, go to SoundFont repository and click load
>>> and load a sound font, this is what happens:
>>>
>>>   beast-0.10.1: bseobject.cc:167: void bse_object_init(BseObject*): Assertion `in_bse_object_new' failed.
>>>   Abgebrochen (Speicherabzug geschrieben)
>>>
>>> This sounds like something that should be very easy to fix for you, since you
>>> know what changes to the object system you did that caused this problem.
>>
>> And there we go, seems debugging is the next step neccessary...
>
> Ok. I know that it didn't fail back then when I wrote the code, so I had hoped
> that this was caused by infrastructure changes, and you'd recognize the error
> message and know what to do. Well, I'll try to figure out myself what needs to
> be changed.

If you want me to guess, it could be related to bse_object_new now hardcoding the C++ object types that are paired with a GType object.
But you're right it needs further investigation.

> Are you sure? If I merge master into the wip/soundfont branch, as in
>
>   (on wip/soundfont)$ git merge master
>
> then this will generate a commit with houndreds of changes that are completely
> unrelated to the soundfont branch (because it will merge every single thing
> that has changed in master since then), and among these changes a few that are
> related.
>
> I can do this, but it doesn't make the actual merge changes as obvious as for
> instance my squash commit or rebasing.
>
> It would work the other way round, by starting from a new branch from master:
>
>   (on master)$ yybranch wip/soundfont-merge
>   (on master)$ yywarp wip/soundfont-merge
>   (on wip/soundfont-merge)$ git merge wip/soundfont
>
> which is basically the same as just merging the branch into master, except that
> it is done in an extra branch, and it would
>
>  - show the changes clearly (unlike merging master into wip/soundfont)
>  - not edit or squash old commits (lossless)
>  - the resulting branch can be pushed to origin and should merge cleanly
>  - however it would not have linear history any longer
>
> It sounds like a plausible way to go.

If I'm not mistaken, both ways will create the exact same kind of merge commit, the only thing different is just that the resulting branch you end up with is either named wip/soundfont-merge or wip/soundfont.

But maybe I'm missing something...

>
>    Cu... Stefan
>

--
Yours sincerely,
Tim Janik

https://testbit.eu/timj/
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: wip/soundfont (and wip/ branches)

Stefan Westerfeld
   Hi!

On Wed, Sep 21, 2016 at 05:42:31PM +0200, Tim Janik wrote:

> On 21.09.2016 12:49, Stefan Westerfeld wrote:
> >>> However if you start BEAST, go to SoundFont repository and click load
> >>> and load a sound font, this is what happens:
> >>>
> >>>   beast-0.10.1: bseobject.cc:167: void bse_object_init(BseObject*): Assertion `in_bse_object_new' failed.
> >>>   Abgebrochen (Speicherabzug geschrieben)
> >>>
> >>> This sounds like something that should be very easy to fix for you, since you
> >>> know what changes to the object system you did that caused this problem.
> >>
> >> And there we go, seems debugging is the next step neccessary...
> >
> > Ok. I know that it didn't fail back then when I wrote the code, so I had hoped
> > that this was caused by infrastructure changes, and you'd recognize the error
> > message and know what to do. Well, I'll try to figure out myself what needs to
> > be changed.
>
> If you want me to guess, it could be related to bse_object_new now hardcoding the C++ object types that are paired with a GType object.
> But you're right it needs further investigation.

Fixed, by replacing g_object_new with bse_object_new, see below.

> > Are you sure? If I merge master into the wip/soundfont branch, as in
> >
> >   (on wip/soundfont)$ git merge master
> >
> > then this will generate a commit with houndreds of changes that are completely
> > unrelated to the soundfont branch (because it will merge every single thing
> > that has changed in master since then), and among these changes a few that are
> > related.
> >
> > I can do this, but it doesn't make the actual merge changes as obvious as for
> > instance my squash commit or rebasing.
> >
> > It would work the other way round, by starting from a new branch from master:
> >
> >   (on master)$ yybranch wip/soundfont-merge
> >   (on master)$ yywarp wip/soundfont-merge
> >   (on wip/soundfont-merge)$ git merge wip/soundfont
> >
> > which is basically the same as just merging the branch into master, except that
> > it is done in an extra branch, and it would
> >
> >  - show the changes clearly (unlike merging master into wip/soundfont)
> >  - not edit or squash old commits (lossless)
> >  - the resulting branch can be pushed to origin and should merge cleanly
> >  - however it would not have linear history any longer
> >
> > It sounds like a plausible way to go.
>
> If I'm not mistaken, both ways will create the exact same kind of merge commit, the only thing different is just that the resulting branch you end up with is either named wip/soundfont-merge or wip/soundfont.
>
> But maybe I'm missing something...

I tried it, while merging master into wip/soundfont-merge there are the following
problems:

 * git diff [ foo.cc ] will show all changes in master, that is a lot
 * it can be workarounded using explicit git diff master [ foo.cc ]
 * also, git status produces a looong list of stuff, due to the number
   of changes in master

also after the merge gitk will show every change in master if you select
the merge commit and look at the history.

So the next thing I tried was reversing it, and create wip/soundfont-merge
as proposed:

 * git diff shows the expected thing (diffs created by soundfont code)
 * git status shows the expected thing (status changes by soundfont code)

after merge gitk will show the soundfont branch history.

So it seems that to git, the merging process is not completely symmetric and
its better to think of it as merging a topic-like branch onto a master-like
branch.

Anyway, you can now pull the branch (produced in this way)

    wip/soundfont-merge

from

    http://space.twc.de/public/git/stwbeast.git

it also contains two additional commits after the merge:

 * the g_object_new triggering assertion problem
 * I also moved the replay network from zintern/ to res/.

so I was able to test that loading soundfonts and playback works.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: wip/soundfont (and wip/ branches)

Tim Janik-6
On 21.09.2016 21:08, Stefan Westerfeld wrote:
>> If you want me to guess, it could be related to bse_object_new now hardcoding the C++ object types that are paired with a GType object.
>> But you're right it needs further investigation.
>
> Fixed, by replacing g_object_new with bse_object_new, see below.

Ok.

> I tried it, while merging master into wip/soundfont-merge there are the following
> problems:
>
>  * git diff [ foo.cc ] will show all changes in master, that is a lot
>  * it can be workarounded using explicit git diff master [ foo.cc ]
>  * also, git status produces a looong list of stuff, due to the number
>    of changes in master
>
> also after the merge gitk will show every change in master if you select
> the merge commit and look at the history.
>
> So the next thing I tried was reversing it, and create wip/soundfont-merge
> as proposed:
>
>  * git diff shows the expected thing (diffs created by soundfont code)
>  * git status shows the expected thing (status changes by soundfont code)
>
> after merge gitk will show the soundfont branch history.

Hm, gitk shows a combined diff for the merge... Just learning how to read that from git-diff(1) ;-)

With git diff, I suspect you view this:
  git diff 1698dc960d2607323551a72c59d581efdbe6e1fc^!
I.e.:
  *   1698d # Merge branch 'wip/soundfont' into wip/soundfont-merge (8 days ago) <Stefan Westerfeld>
Right?

> So it seems that to git, the merging process is not completely symmetric and
> its better to think of it as merging a topic-like branch onto a master-like
> branch.

Yeah, I think that's simply dependent on the order of the merge parents in the merge commit.
What I meant was the file tree should end up the same, right?

> Anyway, you can now pull the branch (produced in this way)
>
>     wip/soundfont-merge

Merge commit:
>   Merge branch 'wip/soundfont' into wip/soundfont-merge
>
>    This merge creates a version of the wip/soundfont branch that cleanly applies
>    to the current master branch.

This should say something like "merge with 'master'" (like the description
mentions), instead of referring to a temporary branch that'll never be pushed
into the public beast repo.

> --------------------------- bse/bseproject.genprc.cc ---------------------------

This should have never been checked in.

> +++ bse/bseproject.proc
> + METHOD (BseProject, get-sound-font-repo) {

Please try to move this into bseapi.idl, we're trying to get rid of all *.proc files.

> diff --cc bse/bsesoundfontrepo.proc

Same here, please try adding this to bseapi.idl.

> it also contains two additional commits after the merge:
>
>  * the g_object_new triggering assertion problem
>  * I also moved the replay network from zintern/ to res/.

This looks wierd, master doesn't have zintern/Makefile.am anymore, your merge
commit doesn't seem to reintroduce it, but the

> so I was able to test that loading soundfonts and playback works.

Ok, that's an important step already, thanks so far.

The merge doesn't apply cleanly to my local tree which has a bunch more proc->bseapi.idl moves in it.

I'll do my best to push this to github in the net few days and can need your help resolving the conflicts after that.

>    Cu... Stefan

--
Yours sincerely,
Tim Janik

https://testbit.eu/timj/
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: wip/soundfont (and wip/ branches)

Stefan Westerfeld
   Hi!

On Thu, Sep 29, 2016 at 02:54:27PM +0200, Tim Janik wrote:

> On 21.09.2016 21:08, Stefan Westerfeld wrote:
> >> If you want me to guess, it could be related to bse_object_new now hardcoding the C++ object types that are paired with a GType object.
>
> >
> > Fixed, by replacing g_object_new with bse_object_new, see below.
>
> Ok.
>
> > I tried it, while merging master into wip/soundfont-merge there are the following
> > problems:
> >
> >  * git diff [ foo.cc ] will show all changes in master, that is a lot
> >  * it can be workarounded using explicit git diff master [ foo.cc ]
> >  * also, git status produces a looong list of stuff, due to the number
> >    of changes in master
> >
> > also after the merge gitk will show every change in master if you select
> > the merge commit and look at the history.
> >
> > So the next thing I tried was reversing it, and create wip/soundfont-merge
> > as proposed:
> >
> >  * git diff shows the expected thing (diffs created by soundfont code)
> >  * git status shows the expected thing (status changes by soundfont code)
> >
> > after merge gitk will show the soundfont branch history.
>
> Hm, gitk shows a combined diff for the merge... Just learning how to read that from git-diff(1) ;-)
>
> With git diff, I suspect you view this:
>   git diff 1698dc960d2607323551a72c59d581efdbe6e1fc^!
> I.e.:
>   *   1698d # Merge branch 'wip/soundfont' into wip/soundfont-merge (8 days ago) <Stefan Westerfeld>
> Right?

Right.

> > So it seems that to git, the merging process is not completely symmetric and
> > its better to think of it as merging a topic-like branch onto a master-like
> > branch.
>
> Yeah, I think that's simply dependent on the order of the merge parents in the merge commit.
> What I meant was the file tree should end up the same, right?

Yes it should.

> > Anyway, you can now pull the branch (produced in this way)
> >
> >     wip/soundfont-merge
>
> Merge commit:
> >   Merge branch 'wip/soundfont' into wip/soundfont-merge
> >
> >    This merge creates a version of the wip/soundfont branch that cleanly applies
> >    to the current master branch.
>
> This should say something like "merge with 'master'" (like the description
> mentions), instead of referring to a temporary branch that'll never be pushed
> into the public beast repo.

Ok. I'll try to do it better for new submissions.

> > --------------------------- bse/bseproject.genprc.cc ---------------------------
>
> This should have never been checked in.

Right. I deleted it in new proposed branch.

> > +++ bse/bseproject.proc
> > + METHOD (BseProject, get-sound-font-repo) {
>
> Please try to move this into bseapi.idl, we're trying to get rid of all *.proc files.

Ok, I fixed this, moved get-sound-font-repo to bseapi.idl.

> > diff --cc bse/bsesoundfontrepo.proc
>
> Same here, please try adding this to bseapi.idl.

Done.

> > it also contains two additional commits after the merge:
> >
> >  * the g_object_new triggering assertion problem
> >  * I also moved the replay network from zintern/ to res/.
>
> This looks wierd, master doesn't have zintern/Makefile.am anymore, your merge
> commit doesn't seem to reintroduce it, but the

It's gone now anyway.

> > so I was able to test that loading soundfonts and playback works.
>
> Ok, that's an important step already, thanks so far.
>
> The merge doesn't apply cleanly to my local tree which has a bunch more proc->bseapi.idl moves in it.
>
> I'll do my best to push this to github in the net few days and can need your help resolving the conflicts after that.

Thanks for doing that, it gave me a reference implementation (bsewave & co) for
the new procedures introduced in the soundfont patch.

My improved branch as now available as

  wip/soundfont-merge

from

  http://space.twc.de/public/git/stwbeast.git

It fixes all the issues you mentioned here, basically it just moves the
procedures to bseapi.idl. So it should merge cleanly with your master. I also
fixed some signedness warnings. I also tested soundfont load and playback: this
works.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast