Re: [lmi] wxProgressDialog z-order anomaly

[Greg Chicares]

On 2017-11-04 16:39, Vadim Zeitlin wrote:
> On Sat, 4 Nov 2017 15:41:59 +0000 Greg Chicares <address@hidden> wrote:
> 
> [... many git commands ...]
> 
>  I'm not sure if it's useful to review all of these commands (and their
> results) in details, because you seem to have mostly figured everything out
> correctly already -- except for the root cause of the problem, but then I
> don't see it neither and I suspect that somehow the commands given in your
> email were not executed exactly as written.

My goal is to distill that stuff down to a small collection of reusable
snippets. Having torn everything down yesterday, I reconstruct it (with
a remote name chosen for single-letter autocompletion) thus, reusing
proven snippets:

git remote add xanadu https://github.com/vadz/lmi.git
git fetch xanadu
git branch vz-no-xslfo xanadu/direct-pdf-gen-master
git branch gwc-no-xslfo `git merge-base master vz-no-xslfo`
git push origin gwc-no-xslfo

>  So, first I would check what do we have now. For this I need to know when
> was the PR branch split from master:
> 
>       $ git merge-base master github/menu-refresh-fix
>       f79cec2b455693b9ccf81ce4a0116e88f7d6bf64

git merge-base master xanadu/menu-refresh-fix
f79cec2b455693b9ccf81ce4a0116e88f7d6bf64

> (BTW, just in case: the name of the branch can be seen near the top of the
> page, just under the PR title -- you would also see the git commands
> specifying how you could merge this branch near the bottom of the page if
> you were logged in, but just the branch name should be mostly sufficient).
> This commit is nothing special, it was just the latest master commit in my
> local master when I did this change (notice that I forgot to update it,
> Savannah master did have a couple of more commits even back then).

https://github.com/vadz/lmi/pull/60
| vadz  wants to merge 3 commits into master from menu-refresh-fix

So far so good.

>  It so happens (but this is just a pure coincidence), that this is also the
> merge-base of direct-pdf-gen-master branch and master.

Yes, 'f79cec...' did seem eerily familiar.

> But, even these 2
> branches start from the same master commit, they diverge and contain
> completely different changes, i.e. you have this graph, where the
> horizontal axis is time and goes in the usual direction and I've also shown
> that there were more commits done on master (exactly 11 of them at the time
> of this writing) since then:
> 
> 
>               *---*---... about 150 more ... ---*---* direct-pdf-gen-master
>              /
>     --- *---*---*--- ... 8 more ... ---*---* master
>              \
>               *---*---* menu-refresh-fix
> 
>  So it makes total sense that you can't fast-forward a branch based on
> direct-pdf-gen-master to menu-refresh-fix, they're not collinear. OTOH if
> you did create a new branch starting from the base commit (the one where
> the graph above bifurcates), as you wrote:
> 
> GC> # Establish my own no-xslfo branch for code review.
> GC> git branch gwc-no-xslfo `git merge-base master vz-no-xslfo`
> 
> then you should be able to fast-forward it to menu-refresh fix because they
> would indeed be on the same line. I.e. the command above directly followed
> by
> 
> GC> git merge --ff-only github-vz/menu-refresh-fix
> 
> really should have worked.

Aha. The '-ff-only' merge would have worked on my 'gwc-no-xslfo'
branch, but I'm trying to merge 'menu-refresh-fix' into master.

>  If it didn't, you must have either done some changes to gwc-no-xslfo
> branch or were on a wrong branch. The first can be ascertained by doing
> 
>         $ git log --oneline master..

That command shows nothing, because I'm on master:

/opt/lmi/src/lmi[0]$git log --oneline master..
/opt/lmi/src/lmi[0]$git branch
  gwc-no-xslfo
* master
  vz-no-xslfo

>  And the second can be checked using either "git branch" or "git
> symbolic-ref HEAD" which I have always shown in my zsh prompt.

/opt/lmi/src/lmi[0]$git symbolic-ref HEAD
refs/heads/master

>  But the most important thing is that even if the merge above had worked,
> this wouldn't really help you at all, because your goal is to integrate my
> changes in master, not some other branch. I don't know if you intentionally
> wanted to combine them with your integration work on PDF generation or not,

No.

> but if you did, then I'd like to strongly argue against this:

We're in agreement here. 'menu-refresh-fix' fixes a repainting anomaly in
production, so I want to put it into master for release at the end of this
month--keeping it separate from the 'no-xslfo' work that will be kept out
of master until it's fully tested.

>  So, after saying all this, let me finally answer the question:
> 
> GC> Okay, so I've figured out several ways to do the wrong thing,
> GC> and how to revert experimental mistakes. But how can I integrate
> GC>   https://github.com/vadz/lmi/pull/60
> GC> without altering history prior to the latest release candidate?
> 
>  First of all, let's forget about direct-pdf-gen-branch and gwc-no-xslfo,
> they still exist as they did before, but it doesn't matter as we won't use
> them at all.
> 
>  Instead, let's start by returning to master:
> 
>         $ git checkout master
> 
> It is presumably always at the latest origin/master (where "origin" is the
> remote corresponding to Savannah repository), but it doesn't hurt to check:
> 
>         $ git fetch origin

/opt/lmi/src/lmi[0]$git checkout master  
Already on 'master'
Your branch is up-to-date with 'origin/master'.
/opt/lmi/src/lmi[0]$git fetch origin   
[...credentials...]
/opt/lmi/src/lmi[0]$

> And if this brings any new changes:
> 
>         $ git merge --ff-only origin/master

/opt/lmi/src/lmi[0]$git merge --ff-only origin/master
Already up-to-date.

>  Now let's examine the new commits in the branch we want to integrate:
> 
>         $ git log ..github-vz/menu-refresh-fix
>         ... shows my 3 commits ...
> 
> We can also check, or confirm, that there are some commits in master that
> have been done since this PR
> 
>         $ git log github-vz/menu-refresh-fix..
>         ... shows your commits postdating my PR ...

Confirmed. BTW, is there some place in git-scm.com/docs/ where the syntax
of <path> arguments is explained? I don't remember ever seeing a leading
or trailing '..', for example. I guess that there's an "empty" argument
there that defaults to the current branch name (and adding 'master' in
the commands above, then rerunning them, seems to confirm that).

>  This shows that fast-forwarding is impossible and we now have a choice:
> either you just merge the branch into master or your replay the changes
> done in the branch onto the newest master.
> 
>  The first choice is the "native Git" way of doing things. A simple
> 
>         $ git merge github-vz/menu-refresh-fix

/opt/lmi/src/lmi[0]$git merge xanadu/menu-refresh-fix
Merge made by the 'recursive' strategy.
 single_choice_popup_menu.cpp | 32 +++++++++++++++-----------------
 single_choice_popup_menu.hpp | 10 ++--------
 2 files changed, 17 insertions(+), 25 deletions(-)

> is enough to do it and while it will indeed create a merge commit, i.e.
> result in
> 
>                               current
>                                master
>     --- *---*---*--- ... ---*---*------------* new master
>              \                              /
>               *------*--------* ------------
>                          menu-refresh-fix
> 
> there is really no problem with this and, in fact, there is an advantage in
> seeing that the 3 commits merged into master as one are parts of
> logically-related changes.

Let's see what that looks like:

/opt/lmi/src/lmi[0]$git log --graph --oneline HEAD~4..HEAD
*   43aeef7cd Merge remote-tracking branch 'xanadu/menu-refresh-fix'
|\  
| * 188309c53 Repaint area covered by SingleChoicePopupMenu after showing it
| * 4bea744df Don't derive SingleChoicePopupMenu from wxWindow
| * c334ae093 Simplify SingleChoicePopupMenu by using wxWidgets functionality
* ad4e45100 Improve command to test mirror synchronization with server
* ff7a3f5b5 Name konsole tabs more klearly
* f8f5ccf00 Synchronize files most recently git-pulled to local mirror

This troubles me for a several reasons. A trivial reason is that I
imagined 'HEAD~4..HEAD' would show the state before the merge plus
three commits, whereas the three commits have become one; perhaps I
just need to learn more about how to specify such arguments.

A bigger reason, though, is that I see a "|\" line but no "|/" line
to match it. It looks like the three commits came out of nowhere
(whereas your ASCII art above shows where they came from), and that
clashes with some sort of conservation law in my mental model.

However, I can resolve that perfectly well by going back farther:

/opt/lmi/src/lmi[0]$git log --graph --oneline f79cec^..HEAD
*   43aeef7cd Merge remote-tracking branch 'xanadu/menu-refresh-fix'
|\  
| * 188309c53 Repaint area covered by SingleChoicePopupMenu after showing it
| * 4bea744df Don't derive SingleChoicePopupMenu from wxWindow
| * c334ae093 Simplify SingleChoicePopupMenu by using wxWidgets functionality
* | ad4e45100 Improve command to test mirror synchronization with server
[...snip nine lines...]
* | 8784138ae Improve documentation
|/  
* f79cec2b4 Enable and apply stable and security upgrades in chroot

and the {|\, |/} symmetry is conserved after all. Therefore, I believe
I really do understand this.

The final reason why this troubles me is: isn't this a fox-trot merge?
If not, why not? Or if so, then why is this one okay when others aren't?

>  But if you want to preserve the linear history, you have another choice:
> replace the 3 commits of the branch on master. This can be done manually by
> using "git cherry-pick" to pick the commits one by one:
> 
>         $ git cherry-pick github-vz/menu-refresh-fix~2
>         $ git cherry-pick github-vz/menu-refresh-fix~1
>         $ git cherry-pick github-vz/menu-refresh-fix
> 
> Or it can be done by cherry-picking them all at once:
> 
>         $ git cherry-pick ..github-vz/menu-refresh-fix
> 
> (you can check using "git log" that this commit selection expression
> results in exactly the same commits).

Okay, let's test the hypothesis that this is the same as laboriously
copy-pasting patches from the github website and applying them with
git-am. I happen to have grabbed the patches that way yesterday, so:

/opt/lmi/src/lmi[0]$git am ../stash/0.patch
Applying: Simplify SingleChoicePopupMenu by using wxWidgets functionality

This is convenient for review, because I can copy the patched file
elsewhere:
/opt/lmi/src/lmi[0]$cp -a single_choice_popup_menu.?pp ../stash
and then diff it with any tool I like (meld, e.g.) against the result
of applying the next patch. (I think there's a way to tell git to
use a specified alternative tool, but that's not what I'm trying
to learn today.)

/opt/lmi/src/lmi[0]$git am ../stash/1.patch                    
Applying: Don't derive SingleChoicePopupMenu from wxWindow
/opt/lmi/src/lmi[0]$git am ../stash/2.patch                    
Applying: Repaint area covered by SingleChoicePopupMenu after showing it

And now the graphical log is linear, and 'HEAD~4..HEAD' means
what I thought it would mean:

/opt/lmi/src/lmi[0]$git log --graph --oneline HEAD~4..HEAD
* 6923da91a Repaint area covered by SingleChoicePopupMenu after showing it
* 07ef56d66 Don't derive SingleChoicePopupMenu from wxWindow
* f019e04ca Simplify SingleChoicePopupMenu by using wxWidgets functionality
* ad4e45100 Improve command to test mirror synchronization with server

This git-am process, though balky, is already deep inside my comfort
zone, so let's see if we can ditch the balkiness:

/opt/lmi/src/lmi[0]$git reset --hard ad4e45100                           
HEAD is now at ad4e45100 Improve command to test mirror synchronization with 
server

/opt/lmi/src/lmi[0]$git cherry-pick ..xanadu/menu-refresh-fix 
[master 1ddf486fc] Simplify SingleChoicePopupMenu by using wxWidgets 
functionality
 Author: Vadim Zeitlin <address@hidden>
 Date: Wed Oct 25 23:02:29 2017 +0200
 2 files changed, 2 insertions(+), 18 deletions(-)
[master 004e4381e] Don't derive SingleChoicePopupMenu from wxWindow
 Author: Vadim Zeitlin <address@hidden>
 Date: Wed Oct 25 23:08:17 2017 +0200
 2 files changed, 6 insertions(+), 9 deletions(-)
[master 86b0c8333] Repaint area covered by SingleChoicePopupMenu after showing 
it
 Author: Vadim Zeitlin <address@hidden>
 Date: Wed Oct 25 23:14:29 2017 +0200
 1 file changed, 11 insertions(+)

Although I cherry-picked everything in one combined operation here
because I had already reviewed each change previously, it's good
to know that I can cherry-pick the commits individually.

/opt/lmi/src/lmi[0]$git log --graph --oneline HEAD~4..HEAD
* 86b0c8333 Repaint area covered by SingleChoicePopupMenu after showing it
* 004e4381e Don't derive SingleChoicePopupMenu from wxWindow
* 1ddf486fc Simplify SingleChoicePopupMenu by using wxWidgets functionality
* ad4e45100 Improve command to test mirror synchronization with server

It does indeed appear equivalent to the git-am procedure. It's not
identical in the sense of matching the SHA1s, but I recall from earlier
discussions of the git-am workflow we use for a proprietary repository
that SHA1s depend upon fine details (e.g., author vs. committer time
stamps) that are convenient to ignore in cases like this.

Just to make sure: if I expand the scope of the git-log argument:

/opt/lmi/src/lmi[0]$git log --graph --oneline f79cec^..HEAD

there's nothing unexpected: it's all linear.

Compared to the "native git" strategy above:
  git merge xanadu/menu-refresh-fix
the advantage of linearity comes at the expense of preserving the
branch-and-merge nature. We can no longer see the merge-base of the
branch, but that's not important anyway because this set of three
commits doesn't collide with any changes in the linear history.

In a way, it's as though you had rebased your changeset on today's
master HEAD, and I had then applied it as a fast-forward merge.
And only after writing that do I understand this:

>  Or, and this is probably the most confusing, but also the most powerful
> and, in practice, most useful, way by doing "git rebase"
> 
>         $ git branch gwc-menu-refresh-fix github-vz/menu-refresh-fix
>         $ git checkout gwc-menu-refresh-fix
[...]
>         $ git rebase master
> 
> It looks like nothing happened as you still have the same (or almost, their
> SHA-1s have changed) commits on your branch. Except that instead of
> 
>     --- *---*---*--- ... 8 more ... ---*---M master
>              \
>               A---B---C gwc-menu-refresh-fix
> 
> things now looks like this:
> 
>     --- *---*---*--- ... 8 more ... ---*---M---A'---B'---C' 
> gwc-menu-refresh-fix
> 
> (notice that master is still unchanged and continues to point to "M").
> 
>  And this is important because it means that _now_ you can fast-forward
> master to this branch, so let's do just this:
> 
>         $ git checkout master
>         $ git merge --ff-only gwc-menu-refresh-fix

....which seems to say exactly the same thing, right?

(I had skipped that section on first reading your email because it
seemed so abstruse. I had to discover the underlying motivation for
myself, I guess; and now I can understand it in retrospect as a
step-by-step guide.)

>  Also note that if you want to change something in my commits before
> applying them, there is a simple way to do it with "git rebase -i": please
> let me know if/when you'd like to know more about it.

As a general rule, I imagine that it's an excellent idea to adopt
your changes as given and only then modify them (if desired) in a
separate, subsequent step. That way, you can see whether or not
I've applied all of your changes faithfully; and, if an error is
introduced, we can determine who introduced it. This seems far
better than a modify-while-adopting approach whereby I commit
altered versions of your changes, with the woeful effect that
the point where a defect was introduced cannot be ascertained.

Thanks for an informative discussion. In this case, I'll use
the cherry-pick method and push the z-order changes presently.