Re: [lmi] PATCH: wxGrid-based census view

[Greg Chicares]

On 2020-07-14 20:46, Vadim Zeitlin wrote:
> On Tue, 14 Jul 2020 20:09:53 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> On 2020-07-01 13:55, Vadim Zeitlin wrote:
> 
> [...] PR #144 is provided
> just to better show how the new code replaces the old one, which is not
> something you can really see with PR #143, which just adds new code.
> 
> GC> I.e., can I accomplish the total mission just as well if I locally
> GC> remove #144 to avoid confusion?
> 
>  I'm not sure what do you mean by "removing it".

I was thinking of something like this:
  git branch -d xanadu/census-view-only-grid
or the same with a capital '-D' if necessary. There's no danger,
because I never push to github myself: I don't wish to, I don't
have the requisite credentials, and I don't plan to acquire them.

As long as I have a local copy of that remote branch, there's
a chance that I'll type 'git some-command census-view<Tab>'
and do something regrettable.

And, although you say otherwise and I'm sure you're right,
when I examine #144 on github, I see almost nothing...

> GC> PR #143 has forty-five commits, but #144 has only two. Does the second
> GC> of those two commits in #144 represent the forty-five in #143 all
> GC> squashed together?
> 
>  Yes.
> 
> GC> I suppose the answer must be "no" because it's such
> GC> a small commit (five old wxDVC lines replace by five new wxGrid lines);
> 
>  I think you must be looking at the wrong commit. The right one is
> https://github.com/vadz/lmi/pull/144/commits/b3ccd39fc2fa13424fc41b1a41044e2c2955708f
> and it definitely contains more than 5 lines.

Copying the URL from my browser:
  
https://github.com/vadz/lmi/pull/144/commits/b3ccd39fc2fa13424fc41b1a41044e2c2955708f
it's the same as yours. To make assurance doubly sure, I copy your URL
above and paste it into my browser, and I see exactly five pairs of
lines changed, in exactly two files. Experimentally, I tell 'umatrix'
to stop blocking githubassets.com, and now I see pages and pages of
diffs. Okay, so that mystery is solved.

In practice, I'd just "merge" the whole branch and review the changes
in aggregate, using some non-web-based tool of my own choice. I'd
look at individual commits only if I couldn't understand some change.
IOW, I'd merge #143, but review the net effect of all the changes
together, in effect squashing them myself, just as though I were
examining #144.

> GC> Here's a more immediate question. From your POV, the first commit
> GC>   2f38b532d5b521ddeab59efcee37ed63990b2dee [PR #143]
> GC>   Upgrade wxWidgets and wxPdfDoc
> GC> may be just a tiny preliminary step that sets the stage, but from
> GC> our POV it seems like half of the total testing effort--because
> GC> all the rest of the commits presumably affect the census manager
> GC> only, but upgrading wx and wxPdfDoc could conceivably introduce
                ^^^^^^^^^^^^^^^^^^^^^^^^^
                i.e., wxWidgets and wxPdfDoc
> 
>  There are actually very few changes in wxPdfDoc, I've only upgraded it to
> get rid of an annoying warning which appeared when using the currently used
> version with the latest wxWidgets. If it's really such a risk to upgrade it
> now, I could cherry-pick only the commit fixing this warning into our
> version. Would this be helpful?

I'm more concerned about regressions in wxWidgets than in wxPdfDoc,
because wxWidgets is a bigger project with more developers making
more changes.

What I'd like to do is upgrade both libraries, at the same time,
as a separate initial step.

> GC> For that reason, what I'd like to do right now is "merge" that
> GC> first commit only, in isolation, and push it to origin/master.
> GC> How can I do that without destroying the history on your branch?
> 
>  I could explain how to do this (and it's not difficult, you'd just have to
> merge a branch containing this commit only), but I think it would be
> simpler if you just applied this commit in any way you see fit, i.e. either
> by running "git cherry-pick 2f38b532d5b521ddeab59efcee37ed63990b2dee" or by
> just doing it manually if it's simpler for you and I could then rebase both
> PRs on your latest master -- this is something that would take me a couple
> of minutes, so it definitely wouldn't be a problem for me. So I think you
> should just do it like this, what do you think?

Okay, I'll cherry-pick that now. And then on Monday or so I'll plan
to cherry-pick the later version (of wxWidgets at least) that you
are currently working on. This way, I can at least get started on
testing it.

>  Also, another complicating factor is that someone has made a further
> change to wxGrid combobox editor (which is used in the new census editor)
> which is supposed to improve its behaviour under GTK and I hoped to
> integrate their PR at https://github.com/wxWidgets/wxWidgets/pull/1941 soon
> and surreptitiously update my PRs to use a newer wxWidgets commit including
> it instead. In fact, I really, really hope to release wxWidgets 3.1.4 in a
> week and I even thought about updating lmi to use it, but I didn't have
> time to test this fully yet and now I don't know if you feel like waiting
> for it or not.

The sequence that's best for us is:
 - cherry-pick and push 2f38b532d5b to savannah now; later...
 - cherry-pick a similar change (incorporating wx pull 1941) at
    the beginning of next week, and push it to savannah then; later...
 - merge the remainder of #143, perhaps all at once
Then we can test after each of those three steps.

Upgrading the wx{Widgets,PdfDoc} scripts has no immediate, automatic
effect for us, when we're routinely just rebuilding lmi itself. We
won't see any effect until we choose to run those scripts. As long as
we don't run them, 2f38b532d5b can have no possible effect on our
production system.

>  So there are several reasonable things to do:
> 
> 1. You can cherry-pick 2f38b532d5b521ddeab59efcee37ed63990b2dee right now.
> 2. You can wait until tomorrow (I'm not sure I can manage this today...)
>    for an updated wx commit including more wxGrid improvements from me.
> 3. You can wait until Sunday for a commit that will be sufficiently close
>    to be almost indistinguishable from the official 3.1.4 release (I'll
>    only make documentation/distribution, but not code changes after this).

Let's do number one now, and number three at the beginning of next week.

>  The rest of this message is just some Git stuff which is not that
> important or interesting, but let me answer this just in case it can be
> useful:
> 
> GC> - I suppose I could merge #143 in totality, locally:
> GC>     git merge 
> GC>     git remote add xanadu https://github.com/vadz/lmi.git
> GC>     git fetch xanadu
> GC>     git checkout master
> GC>     git merge ..xanadu/census-view-grid
> 
>  You can't merge a range of commits, which is what "A..B" produces

[...snip explanation, for which I am most grateful...]

Since you're planning to rebase PR #143 anyway, could you just
wait for Monday, and then give me two tasks:
 - one that only updates wx{Widgets,PdfDoc} for wx pull 1941
     (I needn't trouble you to make a PR for this, because
     it's just a matter of altering two SHA1s, which I can
     do by hand...and if I mistype them, a script failure will
     inform me of my mistake very quickly)
 - a new incarnation of PR #143, rebased on those updated SHA1s

Then, locally, I'll blow away my local 'xanadu' branches and
re-fetch on Monday. (There's probably a simpler one-step method,
but doing 'git-branch -d' first rules out a possibility for error,
and it can't hurt.)

>  You wouldn't be able to do this because this commit will not have the
> current lmi master as its ancestor.

Ah, but you're going to rebase on current lmi origin/master
(plus commit 2f38b532d5b from PR #143, which I'll push soon)
anyway, right? Then everything becomes simple and clean.

Or, rather, you'll wait until Monday or so, when you've reviewed
wx pull 1941, then tell me the new wxWidgets SHA1, and rebase on
that. There's no need for you to rebase twice.

> GC> - I suppose I could cherry-pick that first commit only
> GC>     git cherry-pick 2f38b532d5b521ddeab59efcee37ed63990b2dee
> GC>     git push
> GC> but I fear that's exactly what you don't want me to do.
> 
>  No, this is fine, I'll just rebase the branches on top of this.

Excellent.

> GC> Is there some other, better way to accomplish this?
[...]
>  But if you refer to them as xanadu/branch-name anyhow, you don't even need
> to do this, just rerunning "git fetch xanadu" would be enough (and it will
> tell you that some branches were force-updated, which is just its way of
> warning you about the potential problem mentioned above).

Call me superstitious, but I'd rather remove my local copies of
those branches, and then 'git fetch xanadu' in anticipation of
a simple, clean fetch with no forced-update warnings.