Re: `--rebase-merges' still failing badly
Hi Michael,
On Wed, 10 Oct 2018, Michael Witten wrote:
> In my opinion, the `--rebase-merges' feature has been broken since the
> beginning, and the builtin version should be fixed before it is moved
> ahead.
Everybody is entitled to an opinion. My opinion differs from yours, and I
am a heavy user of `git rebase -kir`.
The `--rebase-merges` feature is not without problems, of course. I can
name a couple of bugs, but I have a hunch that it is more efficient for me
to just fix them.
> In short: "labels" are brittle; see below for tests.
Sure, let's improve them.
> Also, here are some quick *additional* thoughts:
>
> * Labels should be simply "r0", "r1", ... "rN".
That would not be an improvement.
The *interactive* version of `--rebase-merges` is what I use extensively
to juggle Git for Windows' branch thicket. It would be really bad if I had
to somehow map those label names in my head, rather than having the
intuitively-understood labels.
I would understand if you suggested to try to come up with a better naming
than `branch-point-`. But `r`? That's worse than the current state.
By a lot.
> * Why is the command "label" and not "branch"?
Because it is more versatile than just a branch. It is also branch points.
As a matter of fact, the very first statement is about the `onto` label,
which is not a branch.
> * In my experience, there's a lot of this boiler plate:
>
> pick 12345
> label r1
> reset r0
> merge r1
>
> How about instead, use git's existing ideas:
>
> pick 12345
> reset r0
> merge ORIG_HEAD
Too magic. And you cannot change it easily. I had this very real example,
a couple of times yesterday: A merge was in one of the "branches", and
needed to be moved out of it:
pick abc
label branch-point
merge -C 0123 topic
pick def
label bug-fix
reset branch-point
merge -C 4567 bug-fix
This `merge -C 0123 topic` needed to be moved before the branch point.
Another example where the explicit labeling comes in *real* handy is when
I made a Pull Request in Git for Windows ready for contribution to core
Git. These Pull Requests are normally based on `master`, because that is
what the best PR flow is: you based your contributions as close to the tip
as possible, to avoid merge conflicts (and to test as close to the real,
after-merge thing). This would look like this:
label branch-point
pick 123
pick 456
label pr-0815
reset branch-point
merge -C abc pr-0815
Now, to prepare this for core Git, I have to graft this PR onto the
`master` of *upstream*, in our case I would use the `onto` label for that,
by inserting a `reset onto` just before `pick 123`.
So you see, the current, non-implicit, but very much explicit syntax,
makes all of these tasks *quite* easy, and more importantly,
straight-forward: I did not have to explain this to anyone who I needed to
teach how this works.
Remember: the syntax of the todo list is not optimized to be short. It is
optimized to be *editable*. I need to have a very easy way to juggle
criss-cross-merging branch thickets. And the current syntax, while
chattier than you would like, does the job. Pretty well, even.
> * Why not just `--merges' instead of `--rebase-merges'?
This ship has sailed. It is pointless to discuss this now.
Besides, I believe that in your quest to shorten things, you unfortunately
shortened things too much: it is no longer clear what "merges" means in
the context of `--merges`.
> Unfortunately, both the legacy version and the rewrite of
> `--rebase-merges' display a bug that makes this feature fairly
> unusable in practice;
You will be surprised just how much I would embrace bug fixes, once you
provide any.
> it tries to create a "label" (i.e., a branch name) from a commit log
> summary line, and the result is often invalid (or just plain
> irritating to work with). In particular, it fails on typical
> characters, including at least these:
>
> :/\?.*[]
And of course those are not the only ones. The trick is to reduce runs of
disallowed characters to dashes, as is already done with spaces.
Ciao,
Johannes
>
> To see this, first define some POSIX shell functions:
>
> test()
> {
> (
> set -e
> summary=$1
> d=/tmp/repo # WARNING. CHANGE IF NECESSARY.
> rm -rf "$d"; mkdir -p "$d"; cd "$d"
> git init -q
> echo a > a; git add a; git commit -q -m a
> git branch base
> echo b > b; git add b; git commit -q -m b
> git reset -q --hard HEAD^
> git merge -q --no-ff -m "$summary" ORIG_HEAD
> git log --graph --oneline
> git rebase --rebase-merges base
> ); status=$?
> echo
> return "$status"
> }
>
> Test()
> {
> if test
Re: `--rebase-merges' still failing badly
On Thu, 11 Oct 2018 08:01:40 +0900, Junio wrote: > Michael Witten writes: > >> On Wed, 10 Oct 2018 14:43:46 +0900, Junio wrote: >> >>> We haven't seen much complaints and breakages reported against the >>> two big "rewrite in C" topics around "rebase"; perhaps it is a good >>> time to merge them to 'next' soonish to cook them for a few weeks >>> before moving them to 'master'? >> >> In my opinion, the `--rebase-merges' feature has been broken since the >> beginning, and the builtin version should be fixed before it is moved >> ahead. > > [...] > > If "rebase-merges" has been broken since the beginning, as long as the > "rewrite in C" topics around "rebase" do not make it even worse, I do > not think it is a good move to block the topics moving forward. If the > feature were so broken that it is not practically useful, then people > wouldn't be using it in the versions of Git before the rewrite, so it > won't harm anybody if the same feature in the rewritten version is > equally (or even more severely) broken, as long as the other parts of > the feature works at least equally well compared to the older version. > > We are not in the business of hostage taking. > > What *should* block the rewrited version is a regression, i.e. > something that used to work well no longer works or works differently > in such a way that established workflows need to be adjusted. > > [...] I do not think that is a reason to keep "rewrite in C" waiting in > 'pu'. * Your logic is appealing, and I nearly pursuaded myself by the same reasoning to submit my email as a separate discussion, as you suggest. However, what convinced me otherwise is the following: The closer you move the rewrite to a fast-forward-only public branch name, the more likely downstream projects are going to set up new, long-lived releases around this very useful but nevertheless broken feature. The moment you announce a new release, there are going to be a bunch of people who grab that release and then NEVER look back, and so the rest of us will be stuck with this problem for who knows how long. So, not only is this an appeal to the authors to fix this problem, but its also an appeal to you to make sure that the next major release includes the fix. * Also, I say the following without irony or tongue in cheek: Maybe, no one has complained because few people are using this feature yet, or their commit summaries are simplistic, or they've got workarounds (as I've got). Not only must this feature be turned on explicitly, but `git' has existed for over a decade *without* it; users who are interested in sophisticated management of commit history have already developed other ways to achieve the same result (I know I did), or their commit messages are so simplistic that the bug is never triggered, or they just plan around it by automatically running a quick search/replace for the offending characters or for the irritating "labels". If the last decade has shown us anything, it's that git's fundamentals are so good that programmers can get around any bug on their own, without having to appeal to others for help. And, what is a programmer if not someone who is used to making things Just Work [Damnit]? As an illustration, consider the recent `break' command that is being added to the repertoire of `git rebase -i'. Hell, I (and probably many others) have been doing that for YEARS with: x false No need for a "new" command. I bet that 10 years from now, people will *still* be using their own ways, and will *still* be totally oblivious to the existence of `break'. That is to say, I wouldn't put much faith in the degree to which people report issues. The programming world has a lot of itchy backs, and just as many personal inventions for scratching them. As always, thanks for taking the time to review everyone's input. Sincerely, Michael Witten
Re: `--rebase-merges' still failing badly
Michael Witten writes: > On Wed, 10 Oct 2018 14:43:46 +0900, Junio wrote: > >> We haven't seen much complaints and breakages reported against the >> two big "rewrite in C" topics around "rebase"; perhaps it is a good >> time to merge them to 'next' soonish to cook them for a few weeks >> before moving them to 'master'? > > In my opinion, the `--rebase-merges' feature has been broken since the > beginning, and the builtin version should be fixed before it is moved > ahead. I'll omit the remainder of the message not because I disagree with your suggested improvements to "rebase-merges" (that conversation should happen primarily with Dscho), but because I need to react to the above three lines. If "rebase-merges" has been broken since the beginning, as long as the "rewrite in C" topics around "rebase" do not make it even worse, I do not think it is a good move to block the topics moving forward. If the feature were so broken that it is not practically useful, then people wouldn't be using it in the versions of Git before the rewrite, so it won't harm anybody if the same feature in the rewritten version is equally (or even more severely) broken, as long as the other parts of the feature works at least equally well compared to the older version. We are not in the business of hostage taking. What *should* block the rewrited version is a regression, i.e. something that used to work well no longer works or works differently in such a way that established workflows need to be adjusted. In any case, suggestions to improve "rebase-merges" is a very much welcome thing to be discussed on the list, so thanks for raising the issue. What I wanted to say is that I do not think that is a reason to keep "rewrite in C" waiting in 'pu'.
Re: `--rebase-merges' still failing badly
On Wed, 10 Oct 2018 18:51:17 -, Michael Witten wrote: > merge -# Same as merge -C abcde r1 That should be: merge -# Same as `merge r1'
`--rebase-merges' still failing badly
On Wed, 10 Oct 2018 14:43:46 +0900, Junio wrote:
> We haven't seen much complaints and breakages reported against the
> two big "rewrite in C" topics around "rebase"; perhaps it is a good
> time to merge them to 'next' soonish to cook them for a few weeks
> before moving them to 'master'?
In my opinion, the `--rebase-merges' feature has been broken since the
beginning, and the builtin version should be fixed before it is moved
ahead. In short: "labels" are brittle; see below for tests.
Also, here are some quick *additional* thoughts:
* Labels should be simply "r0", "r1", ... "rN".
* The current, long label names are just cumbersome.
* The embedded comments are already more than enough.
* "r" is short for "revision" or "reset" or "remember", etc.
* "r" is located on a QWERTY keyboard such that it's very
easy to type "rN", where "N" is a number.
* Why is the command "label" and not "branch"? Every other related
command looks like a normal git command: "reset" and "merge".
Make it "branch".
* In my experience, there's a lot of this boiler plate:
pick 12345
label r1
reset r0
merge r1
How about instead, use git's existing ideas:
pick 12345
reset r0
merge ORIG_HEAD
Or, maybe git in general should treat `-' as `ORIG_HEAD' (which
would be similar to how `git checkout' understands `-'), thereby
allowing a very quick idiomatic string of commands:
pick 12345
reset r0
merge -
In truth, I don't really know the semantics of `ORIG_HEAD', so
maybe those should be nailed down and documented more clearly;
I would like it to work as in the following:
pick 12345
# label r1 (pretend)
reset r0 # Store r1 in ORIG_HEAD
pick 67890 # Do NOT touch ORIG_HEAD
merge -# Same as merge -C abcde r1
Anyway, this kind of unspoken behavior would make *writing* a
new history by hand much more pleasant.
* Why not just `--merges' instead of `--rebase-merges'? Or, better
yet, just make it the default behavior; the special option
should instead be:
--flatten
This option would simply tell `git rebase' to prepare an initial
todo list without merges.
Thanks for this great feature.
I'm only complaining so much because it's such a useful feature, and I
want it to be even better, because I'll probably use it A LOT; it
should have been available since the start as a natural consequence of
the way git works.
Sincerely,
Michael Witten
---
Unfortunately, both the legacy version and the rewrite of
`--rebase-merges' display a bug that makes this feature fairly
unusable in practice; it tries to create a "label" (i.e., a branch
name) from a commit log summary line, and the result is often invalid
(or just plain irritating to work with). In particular, it fails on
typical characters, including at least these:
:/\?.*[]
To see this, first define some POSIX shell functions:
test()
{
(
set -e
summary=$1
d=/tmp/repo # WARNING. CHANGE IF NECESSARY.
rm -rf "$d"; mkdir -p "$d"; cd "$d"
git init -q
echo a > a; git add a; git commit -q -m a
git branch base
echo b > b; git add b; git commit -q -m b
git reset -q --hard HEAD^
git merge -q --no-ff -m "$summary" ORIG_HEAD
git log --graph --oneline
git rebase --rebase-merges base
); status=$?
echo
return "$status"
}
Test()
{
if test "$@" 1>/dev/null 2>&1; then
echo 'good'; return 0
else
echo 'fail'; return 1
fi
}
Then, try various commit summaries (see below for results):
test c
test 'combine these into a merge: a and b'
Test ab:
Test a:b
Test :
Test a/b
Test 'Now supports /regex/'
Test ab/
Test /ab
Test /
Test 'a\b'
Test '\'
Test 'Maybe this works?'
Test '?'
Test 'This does not work.'
Test 'This works. Strange!'
Test .git
Test .
Test 'Cast each pointer to *void'
Test '*'
Test 'return a[1] not a[0]'
Test '[ does not work'
Test '['
Test '] does work'
Test ']'
Here are the results of pasting the above commands into my terminal:
$ test c
warning: templates not found in ../install/share/git-core/templates
* 1992d07 (HEAD -> master) c
|\
| * 34555b5 b
|/
* 338db9b (base) a
Successfully rebased and updated refs/heads/master.
$ test 'combine these into a merge: a and b'
warning: templates not found in ../install/share/git-core/templates
* 4202c49 (HEAD -> master) combine these into a merge: a and b
