odd behavior with git-rebase

odd behavior with git-rebase

[Neil Horman]

Hey all-
	I hit a strange problem with git rebase and I can't quite decide if its
a design point of the rebase command, or if its happening in error.  When doing
upstream backports of various kernel components I occasionally run accross
commits that, for whatever reason, I don't want/need or can't backport.  When
that happens, I insert an empty commit in my history noting the upstream commit
hash and the reasoning behind why I skipped it (I use git commit -c <hash>
--allow-empty).  If I later rebase this branch, I note that all my empty commits
fail indicating the commit cannot be applied.  I can of course do another git
commit --allow-empty -c <hash>; git rebase --continue, and everything is fine,
but I'd rather it just take the empty commit in the rebase if possible.

I know that git cherry-pick allows for picking of empty commits, and it appears
the rebase script uses cherry-picking significantly, so I'm not sure why this
isn't working, or if its explicitly prevented from working for some reason.

anyone have any insight?

Thanks & Regards
Neil

Re: odd behavior with git-rebase

[Jeff King]

On Fri, Mar 23, 2012 at 02:52:05PM -0400, Neil Horman wrote:

> 	I hit a strange problem with git rebase and I can't quite decide if its
> a design point of the rebase command, or if its happening in error.  When doing
> upstream backports of various kernel components I occasionally run accross
> commits that, for whatever reason, I don't want/need or can't backport.  When
> that happens, I insert an empty commit in my history noting the upstream commit
> hash and the reasoning behind why I skipped it (I use git commit -c <hash>
> --allow-empty).  If I later rebase this branch, I note that all my empty commits
> fail indicating the commit cannot be applied.  I can of course do another git
> commit --allow-empty -c <hash>; git rebase --continue, and everything is fine,
> but I'd rather it just take the empty commit in the rebase if possible.

I think it is even odder than that. If you use plain rebase, the empty
commits are silently omitted. If you do an interactive rebase, you get
the "could not apply" message (and just doing a "continue" creates some
funny error messages and ends up omitting the commit).

I think both of these are bugs. In the first case, the empty commit
appears to be already applied, because it does nothing. But if somebody
bothered to create an empty commit in the first place, they probably
want to keep it, and we should special-case it.

As you've probably guessed, empty commits are not all that common, and I
think this area of git is not well-tested.

-Peff

Re: odd behavior with git-rebase

[Phil Hord]

On Fri, Mar 23, 2012 at 3:54 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 23, 2012 at 02:52:05PM -0400, Neil Horman wrote:
>
>>       I hit a strange problem with git rebase and I can't quite decide if its
>> a design point of the rebase command, or if its happening in error.  When doing
>> upstream backports of various kernel components I occasionally run accross
>> commits that, for whatever reason, I don't want/need or can't backport.  When
>> that happens, I insert an empty commit in my history noting the upstream commit
>> hash and the reasoning behind why I skipped it (I use git commit -c <hash>
>> --allow-empty).  If I later rebase this branch, I note that all my empty commits
>> fail indicating the commit cannot be applied.  I can of course do another git
>> commit --allow-empty -c <hash>; git rebase --continue, and everything is fine,
>> but I'd rather it just take the empty commit in the rebase if possible.
>
> I think it is even odder than that. If you use plain rebase, the empty
> commits are silently omitted. If you do an interactive rebase, you get
> the "could not apply" message (and just doing a "continue" creates some
> funny error messages and ends up omitting the commit).

Coincidentally I ran into this same behavior this week.  But what
bothered me about it was the messages git gave me.  The empty commit
gave me cherry-pick hints instead of rebase ones, including advising
me to "use 'git reset'" to resolve the problem if I don't want this
commit after all.

$ git rebase -i HEAD~10
...
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'
# Not currently on any branch.
nothing to commit (working directory clean)
Could not apply d513504... Some commit message


I'm not sure if this is the norm or if it's a result of some other
things I did in this sequence.  But I've seen it several times now.
I've only tested it on 1.7.10 versions, including RC2.

Phil

Re: odd behavior with git-rebase

[Jeff King]

On Mon, Mar 26, 2012 at 02:31:05PM -0400, Phil Hord wrote:

> Coincidentally I ran into this same behavior this week.  But what
> bothered me about it was the messages git gave me.  The empty commit
> gave me cherry-pick hints instead of rebase ones, including advising
> me to "use 'git reset'" to resolve the problem if I don't want this
> commit after all.
> 
> $ git rebase -i HEAD~10
> ...
> The previous cherry-pick is now empty, possibly due to conflict resolution.
> If you wish to commit it anyway, use:
> 
>     git commit --allow-empty
> 
> Otherwise, please use 'git reset'
> # Not currently on any branch.
> nothing to commit (working directory clean)
> Could not apply d513504... Some commit message
> 
> 
> I'm not sure if this is the norm or if it's a result of some other
> things I did in this sequence.  But I've seen it several times now.
> I've only tested it on 1.7.10 versions, including RC2.

This is easily reproducible on a simple test case:

  commit() {
    echo $1 >$1 && git add $1 && git commit -m $1 && git tag $1
  }

  git init repo &&
  cd repo &&
  commit one &&
  commit two &&
  git commit --allow-empty -m empty &&
  commit three &&
  git checkout -b fork one &&
  commit four &&
  git rebase -i fork master
  git --no-pager log --oneline

(this is the same test case I used without "-i" to check the rebase
skipping behavior).

I agree the mention of cherry-pick is a little confusing. I think the
advice to use "git commit --allow-empty" is still the right thing
(although better still would be to recognize that the commit was empty
in the first place and not stop at all). I think the message is showing
the fact that "rebase -i" is cobbled together from other pieces. I
wonder if the sequencer work would make this a little smoother (I
confess I have not paid much attention to what is happening in that
area).

-Peff

Re: odd behavior with git-rebase

[Junio C Hamano]

Neil Horman <nhorman@tuxdriver.com> writes:

> I know that git cherry-pick allows for picking of empty commits, and it appears
> the rebase script uses cherry-picking significantly, so I'm not sure why this
> isn't working, or if its explicitly prevented from working for some reason.

The primary purpose of "rebase" is (or at least was when it was conceived)
to clean up the existing history, and a part of the cleaning up is not to
replay a patch that ends up being empty.  Even though we try to omit an
already applied patch by using "git cherry" internally when choosing which
commits to replay, a commit that by itself is *not* empty could end up
being empty when a similar change has already been made to the updated
base, and we do want to omit them.

A commit that is empty (i.e. --allow-empty) by itself was a much later
invention than the basic rebase logic, and the rebase may want to be
updated to special case it, but as the default behaviour it is doing the
right thing by not letting an empty commit into the cleaned up history.

Re: odd behavior with git-rebase

[Neil Horman]

On Fri, Mar 23, 2012 at 01:33:30PM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > I know that git cherry-pick allows for picking of empty commits, and it appears
> > the rebase script uses cherry-picking significantly, so I'm not sure why this
> > isn't working, or if its explicitly prevented from working for some reason.
> 
> The primary purpose of "rebase" is (or at least was when it was conceived)
> to clean up the existing history, and a part of the cleaning up is not to
I can understand that, although IMHO it seems equally usefull as a tool for
simply doing what its name implies, moving a history to a new starting point,
e.g. to plainly rebase it.  Thats the use that I have for it anyway.

> replay a patch that ends up being empty.  Even though we try to omit an
> already applied patch by using "git cherry" internally when choosing which
> commits to replay, a commit that by itself is *not* empty could end up
> being empty when a similar change has already been made to the updated
> base, and we do want to omit them.
> 
Is there a way to differentiate a commit that is made empty as the result of a
previous patch in the rebase, and a commit that is simply empty?

> A commit that is empty (i.e. --allow-empty) by itself was a much later
> invention than the basic rebase logic, and the rebase may want to be
> updated to special case it, but as the default behaviour it is doing the
> right thing by not letting an empty commit into the cleaned up history.
I agree, I think perhaps adding an --allow-empty option to the rebase logic, so
that empty commits (or perhaps just initially empty, as opposed to commits made
empty) would be very beneficial.  

Thanks all, I'll start trying to pick through the rebase logic this week.

Best
Neil

> 
> 
> 

Re: odd behavior with git-rebase

[Junio C Hamano]

Neil Horman <nhorman@tuxdriver.com> writes:

> Is there a way to differentiate a commit that is made empty as the result of a
> previous patch in the rebase, and a commit that is simply empty?

An empty commit has the same tree object as its parent commit.

> I agree, I think perhaps adding an --allow-empty option to the rebase logic, so
> that empty commits (or perhaps just initially empty, as opposed to commits made
> empty) would be very beneficial.

Yeah, that probably may make sense.

Re: odd behavior with git-rebase

[Neil Horman]

On Mon, Mar 26, 2012 at 10:12:48AM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > Is there a way to differentiate a commit that is made empty as the result of a
> > previous patch in the rebase, and a commit that is simply empty?
> 
> An empty commit has the same tree object as its parent commit.
> 
Got it, thanks!

> > I agree, I think perhaps adding an --allow-empty option to the rebase logic, so
> > that empty commits (or perhaps just initially empty, as opposed to commits made
> > empty) would be very beneficial.
> 
> Yeah, that probably may make sense.
> 
Ok, cool, I'll have a patch in a few days, thanks!
Neil

Re: odd behavior with git-rebase

[Neal Kreitzinger]

On 3/26/2012 12:20 PM, Neil Horman wrote:
> On Mon, Mar 26, 2012 at 10:12:48AM -0700, Junio C Hamano wrote:
>> Neil Horman<nhorman@tuxdriver.com>  writes:
>>
>>> I agree, I think perhaps adding an --allow-empty option to the rebase logic, so
>>> that empty commits (or perhaps just initially empty, as opposed to commits made
>>> empty) would be very beneficial.
>>
>> Yeah, that probably may make sense.
>>
> Ok, cool, I'll have a patch in a few days, thanks!
>
IMO, it seems like --allow-empty is an appropriate patch for git-rebase 
(non-interactive), and that git-rebase -i would need a command like 
"k"eep to distinguish which empty commits are not to be discarded and 
which empty commits are ok to discard automatically.  git-rebase -i 
should allow explicit control on a commit by commit basis as opposed to 
blanket rules like "discard all empty commits" or "keep all empty 
commits" that apply to all commits in the rebase-to-do list based on a 
single cli option.

Maybe this is what you plan on doing.  Maybe there is a better command 
name than "k"eep for this.

v/r,
neal

Re: odd behavior with git-rebase

[Phil Hord]

On Mon, Mar 26, 2012 at 5:53 PM, Neal Kreitzinger
<nkreitzinger@gmail.com> wrote:
> On 3/26/2012 12:20 PM, Neil Horman wrote:
>>
>> On Mon, Mar 26, 2012 at 10:12:48AM -0700, Junio C Hamano wrote:
>>>
>>> Neil Horman<nhorman@tuxdriver.com>  writes:
>>>
>>>> I agree, I think perhaps adding an --allow-empty option to the rebase
>>>> logic, so
>>>> that empty commits (or perhaps just initially empty, as opposed to
>>>> commits made
>>>> empty) would be very beneficial.
>>>
>>>
>>> Yeah, that probably may make sense.
>>>
>> Ok, cool, I'll have a patch in a few days, thanks!
>>
> IMO, it seems like --allow-empty is an appropriate patch for git-rebase
> (non-interactive), and that git-rebase -i would need a command like "k"eep
> to distinguish which empty commits are not to be discarded and which empty
> commits are ok to discard automatically.  git-rebase -i should allow
> explicit control on a commit by commit basis as opposed to blanket rules
> like "discard all empty commits" or "keep all empty commits" that apply to
> all commits in the rebase-to-do list based on a single cli option.

But I don't want a 'keep-even-if-empty' option in interactive.  I want
a 'purge-if-empty' option instead.  But I don't want to be bothered
with telling git this for every commit.

I recently had a long-running branch to clean up.  It was polluted
with commits pulled in by a ham-fisted  developer collaborating on
this and another branch.  He's not quite got the git mental model yet
and he had lots of commits doing things and then undoing them later
on.  Rebase scares him.

So I did a lot of interactive rebasing on this branch to reorder the
"good" change commits to the front of the line where they could be
pushed to code review sooner.  In the meantime, I wanted to keep the
rest of the branch in place so I could see what was left to tackle.

I cherry-picked replacements for many of the "good" commits -- from
their original topic branches HamFist swiped them from -- so I would
have the current, reviewable commit to push. Then I tested the
long-running branch on top of these commits.  This involved about 8 or
10 passes through 'git rebase -i master' for one reason or another.

On this branch of 40 commits, git interrupted me about 10 times on
each pass to ask me what to do.  The reason is always one of these:

  1. There is a new conflict I need to resolve
      examine / mergetool / test / --continue

  2. There is a rerere autoresolved conflict git wants me to approve
      examine / test / --continue

  3. There are no changes left in this commit because either
        a. they were introduced into earlier commits, or
        b. git-rerere-membered that I don't want those changes
      examine / --skip

I went through this process about 5 or 6 times as I massaged the stink
out of this branch.  Cases 2 and 3 became more common as I went along.
 But git always wanted to stop and ask my approval before continuing.
It was frustrating.

I always had my original branch to go compare to.  This one is really
a trial rework of these commits.  So I wish I could tell git to only
bother me when he sees a new conflict.  Don't stop and ask me for
something every 3 or 4 commits.

I really wanted something like this:

   $ git rebase --purge-empty --accept-rerere-authority -i master

So, even though this is an "interactive" rebase, I wish git would do
more of the busywork for me.  That is, I only want it to be as
interactive as it needs to be, and no more so.

Phil

Re: odd behavior with git-rebase

[Phil Hord]

On Wed, Mar 28, 2012 at 2:18 AM, Neal Kreitzinger
<nkreitzinger@gmail.com> wrote:
> On 3/26/2012 5:53 PM, Phil Hord wrote:
>> On Mon, Mar 26, 2012 at 5:53 PM, Neal Kreitzinger
>> <nkreitzinger@gmail.com> wrote:
>>> On 3/26/2012 12:20 PM, Neil Horman wrote:
>>>>
>>>> On Mon, Mar 26, 2012 at 10:12:48AM -0700, Junio C Hamano wrote:
>>>>>
>>>>> Neil Horman<nhorman@tuxdriver.com> writes:
>>>>>
>>>>>> I agree, I think perhaps adding an --allow-empty option to
>>>>>> the rebase logic, so that empty commits (or perhaps just
>>>>>> initially empty, as opposed to commits made empty) would be
>>>>>> very beneficial.
>>>>>
>>>>>
>>>>> Yeah, that probably may make sense.
>>>>>
>>>> Ok, cool, I'll have a patch in a few days, thanks!
>>>>
>>> IMO, it seems like --allow-empty is an appropriate patch for
>>> git-rebase (non-interactive), and that git-rebase -i would need a
>>> command like "k"eep to distinguish which empty commits are not to
>>> be discarded and which empty commits are ok to discard
>>> automatically. git-rebase -i should allow explicit control on a
>>> commit by commit basis as opposed to blanket rules like "discard
>>> all empty commits" or "keep all empty commits" that apply to all
>>> commits in the rebase-to-do list based on a single cli option.
>>
>> But I don't want a 'keep-even-if-empty' option in interactive.
>
> That's why its called an option.  Don't use it if you don't want it.  ;-)

Yeah, I didn't mean I don't want it to exist in git.  I just meant
that it's not what I'm seeking atm.

But you seem to have forgotten that you were arguing against it at the
beginning of the paragraph I was responding to.

>> I want a 'purge-if-empty' option instead.
>
> That's the default behavior currently.  We're not proposing to change that.

It ostensibly is the current behavior.  But Peff and I think we've
seen it misbehave in --interactive mode.  However, I may have been
confused by its doppelgänger, the "rerere autoresolved to empty
commit" situation, where the behavior is to announce "Could not apply"
and pause the process to wait for human intervention.

>> But I don't want to be bothered with telling git this for every
>> commit.
>
> You only tell it which empty commits you want to "k"eep (to not
> "auto-purge").  In your case, you don't want to keep any so you don't have
> to tell it anything.  The default behavior will purge them all (empty
> commits) because you didn't mark any as "k"eep.
>
>
>> I recently had a long-running branch to clean up.
>
> I have users with branches over a year old.

I don't understand this comment.  Is this a pissing contest?

>> It was polluted with commits pulled in by a ham-fisted developer
>> collaborating on this and another branch.
>
> My users cp files from other worktrees and do merges manually without using
> git-merge, git-rebase, or git-cherry-pick.
>
>
>> He's not quite got the git mental model yet and he had lots of
>> commits doing things and then undoing them later on.
>
> Most of my users think git is cvs 3.0.

I see what you did there.  I like that one.

>
>> Rebase scares him.
>
> git as a whole scares most of my users.
>
>> So I did a lot of interactive rebasing on this branch to reorder the
>> "good" change commits to the front of the line where they could be
>> pushed to code review sooner. In the meantime, I wanted to keep the
>> rest of the branch in place so I could see what was left to tackle.
>
> You should create a "backup" branch of the before state so you don't get
> blamed for breaking their stuff.

Apologies for failing to mention that his branch remained "backed up"
in origin/shared/dumbass as well as on a local branch and in my
reflog.

>> I cherry-picked replacements for many of the "good" commits -- from
>> their original topic branches HamFist swiped them from -- so I would
>> have the current, reviewable commit to push. Then I tested the
>> long-running branch on top of these commits. This involved about 8
>> or 10 passes through 'git rebase -i master' for one reason or
>> another.
>
> Sounds like you may also be a little scared of git-rebase yourself.  Be a
> man and do it all in one pass.  (You have the backup branch to startover if
> need be.)  ;-)

Oh, it IS a pissing contest then.  Whip it out, boy!

No, I'm not scared of rebase.  There were 40 commits full of crap and
gems.  I made several pass through rebase-interactive not because
git-rebase is a problem but simply because I was sifting methodically
for gems and flushing turds.

>> On this branch of 40 commits, git interrupted me about 10 times on
>> each pass to ask me what to do. The reason is always one of these:
>>
>> 1. There is a new conflict I need to resolve
>> examine / mergetool / test / --continue
>>
>> 2. There is a rerere autoresolved conflict git wants me to approve
>> examine / test / --continue
>
> You trust rerere?  I take back my earlier comment, you are a brave man.

Uh huh. That's what I thought!. Hmmph.

> Also, you're starting to sound like some of my users who think that git
> magically does their work for them.

Git does do my work for me, and it does require more work from me.
But in this case, it saved me sifting the same turds over and over.

I do trust rerere to redo what it previously remembered I did for
resolution.  In that respect, rerere is only as reliable as I am.  But
I also have a backup that I'll consult periodically, these commits are
headed for the CI-Server and human reviewers, and there's plenty of
time to catch my stupid mistakes.


>> 3. There are no changes left in this commit because either a. they
>> were introduced into earlier commits, or b. git-rerere-membered that
>> I don't want those changes examine / --skip
>
> git-rerere-membered -- that's a good one.  I felt like
> git-rerere-dismembered my merge the one time I tried it.  But seriously, I
> have no experience with this --skip scenario.

I think rerere seemed to butcher my first ugly merge, too.  But it was
really my fault.  I knew I was on a throwaway branch previously, so I
was not as careful with the merge.  Later when I revisited it to do it
"for real", rerere stepped in and was as un-careful as I had been.
:-)  I think he was rubbing my nose in it.


>> I went through this process about 5 or 6 times as I massaged the
>> stink out of this branch. Cases 2 and 3 became more common as I
>> went along. But git always wanted to stop and ask my approval before
>> continuing. It was frustrating.
>>
>> I always had my original branch to go compare to. This one is really
>> a trial rework of these commits. So I wish I could tell git to only
>> bother me when he sees a new conflict. Don't stop and ask me for
>> something every 3 or 4 commits.
>>
>> I really wanted something like this:
>>
>> $ git rebase --purge-empty --accept-rerere-authority -i master
>>
>
> --accept-rerere-authority sounds like another recent thread (maybe from
> you).

Maybe.


>> So, even though this is an "interactive" rebase, I wish git would do
>> more of the busywork for me. That is, I only want it to be as
>> interactive as it needs to be, and no more so.
>>
> FWIW, I think we may be a little spoiled.  git rebase -i is one of the, if
> not the, most powerful, flexible, amazing change control commands in
> existence if you think about it.  I guess we could switch to another SCM
> that does it better.  Oh, I forgot, there isn't one.

Yes, definitely spoiled.  rebase-interactive,  add-patch, and
checkout-patch have changed the way I manage my project.  I have a
much better project history because of them, but I'm also much more
intimate with my SCM than I ever was under Subversion, cvs, SourceSafe
or tar.  :-)

I never thought I would be excited about an SCM, but I've become the
resident evangelist at $dayjob.

> Hopefully, my sympathetically-inspired comments based on some shared
> experiences are helpful at some level.  :-)

Yes, thanks.  Sometimes I'm long-winded.  I usually tone it down after
a couple of rewrites, but this time I left most of the elucidating
back story in.

Thanks for reading along.

Phil

Re: odd behavior with git-rebase

[Junio C Hamano]

Neal Kreitzinger <nkreitzinger@gmail.com> writes:

> On 3/26/2012 5:53 PM, Phil Hord wrote:
> ...
>>  2. There is a rerere autoresolved conflict git wants me to approve
>>  examine / test / --continue
>
> You trust rerere?  I take back my earlier comment,...

He does "rerere" and then "examine"s, doesn't he?  So it is not "blindly
trust", but "trust and verify".

>>  I really wanted something like this:
>>
>>  $ git rebase --purge-empty --accept-rerere-authority -i master
>>
> --accept-rerere-authority sounds like another recent thread (maybe
> from you).

It was from a different person and on a different command, but I think
they are going in the same direction.  The "--rerere-autoupdate" option of
"git am", "git rebase" and "git merge" could learn to be a bool-plus, a
stronger form than "yes/no", such that it makes a commit if everything
auto-resolves cleanly (and rerere.autoupdate configuration could learn to
be a bool-plus as well).

While I am hesitant to endorse such a mode of operation, as it can invite
sloppy users hurt themselves, I do not fundamentally oppose it as an
option.  It could save time for diligent users who know when to examine
and verify the result, in order to avoid casting the result with blind
faith in mechanical merges.

Re: odd behavior with git-rebase

[Phil Hord]

On Mon, Mar 26, 2012 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
>
>> Is there a way to differentiate a commit that is made empty as the result of a
>> previous patch in the rebase, and a commit that is simply empty?
>
> An empty commit has the same tree object as its parent commit.
>
>> I agree, I think perhaps adding an --allow-empty option to the rebase logic, so
>> that empty commits (or perhaps just initially empty, as opposed to commits made
>> empty) would be very beneficial.
>
> Yeah, that probably may make sense.


Can we have three behaviors?

A: Current mode, stop and error on empty commits
B: --keep-empty, to retain empty commits without further notice
C: --purge-empty, to remove empty commits without further notice

Phil

Re: odd behavior with git-rebase

[Neil Horman]

On Mon, Mar 26, 2012 at 02:29:24PM -0400, Phil Hord wrote:
> On Mon, Mar 26, 2012 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Neil Horman <nhorman@tuxdriver.com> writes:
> >
> >> Is there a way to differentiate a commit that is made empty as the result of a
> >> previous patch in the rebase, and a commit that is simply empty?
> >
> > An empty commit has the same tree object as its parent commit.
> >
> >> I agree, I think perhaps adding an --allow-empty option to the rebase logic, so
> >> that empty commits (or perhaps just initially empty, as opposed to commits made
> >> empty) would be very beneficial.
> >
> > Yeah, that probably may make sense.
> 
> 
> Can we have three behaviors?
> 
> A: Current mode, stop and error on empty commits
> B: --keep-empty, to retain empty commits without further notice
> C: --purge-empty, to remove empty commits without further notice
> 
Yeah, I've got most of --keep-empty in a private branch here now.  I was calling
it allow-empty, but given (C) above, I like --keep-empty better.

I'll add --purge-empty to me todo list. and augment the rebase code to pass
these options along.

One more question - The options for cherry-pick are currently mostly merged with
git revert.  Are there any opinions on the applicability of
--keep-empty/--purge-empty to reverts?  

Regards
Neil

Re: odd behavior with git-rebase

[Jay Soffian]

On Mon, Mar 26, 2012 at 2:29 PM, Phil Hord <phil.hord@gmail.com> wrote:
> Can we have three behaviors?
>
> A: Current mode, stop and error on empty commits
> B: --keep-empty, to retain empty commits without further notice
> C: --purge-empty, to remove empty commits without further notice

FWIW, filter-branch uses "--prune-empty", should we wish to be consistent.

j.

Re: odd behavior with git-rebase

[Neal Kreitzinger]

On 3/23/2012 1:52 PM, Neil Horman wrote:
> Hey all-
> 	I hit a strange problem with git rebase and I can't quite decide if its
> a design point of the rebase command, or if its happening in error.  When doing
> upstream backports of various kernel components I occasionally run accross
> commits that, for whatever reason, I don't want/need or can't backport.  When
> that happens, I insert an empty commit in my history noting the upstream commit
> hash and the reasoning behind why I skipped it (I use git commit -c<hash>
> --allow-empty).  If I later rebase this branch, I note that all my empty commits
> fail indicating the commit cannot be applied.  I can of course do another git
> commit --allow-empty -c<hash>; git rebase --continue, and everything is fine,
> but I'd rather it just take the empty commit in the rebase if possible.
>
> I know that git cherry-pick allows for picking of empty commits, and it appears
> the rebase script uses cherry-picking significantly, so I'm not sure why this
> isn't working, or if its explicitly prevented from working for some reason.
>
> anyone have any insight?
>
FWIW, I'm not sure what you mean by "backport", but IMHO backporting a 
critical fix to an earlier version seems by nature to be a cherry-pick 
operation as opposed to a rebase operation.  A rebase implies "I want 
everything" -- that doesn't sound like a backport.  A cherry-pick 
implies "I only want certain things" -- that sounds like a backport. 
Maybe your really using rebase to cherry-pick several commits.  Using 
your technique of "empty commit placeholders", it seems you could end up 
with quite a lot of "empty" commit placeholders which doesn't seem to 
make much sense.  Why would you want a bunch of empty commit 
placeholders in your older version bugfix history saying "I didn't want 
this, but its in the newer version."  (who cares?).  Isn't having the 
stuff you do want recorded as commits enough to make it clear what you 
brought over?  You could even edit the "cherry-picked" (or rebased) 
commit messages to document the sha1 of the commit being cherry picked 
from the newer version.  That seems to make more sense to document what 
you did, as opposed to documenting what you didn't do.

I'm not a git expert or a kernel developer, but find this subject 
relevant and interesting.  Our "New" system was forked from our "old" 
system.  We bring over fixes/features from "old" system to "new" system. 
  "New" system hast limited field testing accumulated, and gets 
features/fixes from "old" system which has extensive productional 
testing ongoing.  When doing so, many "old" system changes are not 
wanted as they are irrelevent to the "new" system.  Having an empty 
commit for them makes no sense.

Food for thought.  Maybe my cooking as bad.  Maybe I'll learn a new recipe.

v/r,
neal

Re: odd behavior with git-rebase

[Neil Horman]

On Mon, Mar 26, 2012 at 10:27:57AM -0500, Neal Kreitzinger wrote:
> On 3/23/2012 1:52 PM, Neil Horman wrote:
> >Hey all-
> >	I hit a strange problem with git rebase and I can't quite decide if its
> >a design point of the rebase command, or if its happening in error.  When doing
> >upstream backports of various kernel components I occasionally run accross
> >commits that, for whatever reason, I don't want/need or can't backport.  When
> >that happens, I insert an empty commit in my history noting the upstream commit
> >hash and the reasoning behind why I skipped it (I use git commit -c<hash>
> >--allow-empty).  If I later rebase this branch, I note that all my empty commits
> >fail indicating the commit cannot be applied.  I can of course do another git
> >commit --allow-empty -c<hash>; git rebase --continue, and everything is fine,
> >but I'd rather it just take the empty commit in the rebase if possible.
> >
> >I know that git cherry-pick allows for picking of empty commits, and it appears
> >the rebase script uses cherry-picking significantly, so I'm not sure why this
> >isn't working, or if its explicitly prevented from working for some reason.
> >
> >anyone have any insight?
> >
> FWIW, I'm not sure what you mean by "backport", but IMHO backporting
> a critical fix to an earlier version seems by nature to be a
> cherry-pick operation as opposed to a rebase operation.  A rebase
I work on RHEL, among other things, keeping network drivers up to date with
upstream bugfixes/features/etc.  When I say 'backport' I mean exactly that,
backporting an upstream fix/feature/change to various RHEL kernel versions.

Nominally, I would love to be able to do a merge from upstream to just take
everything, but for various and sundry reasons we can't take all upstream
changes (ABI commitments, core API availabliilty, etc).  So In my workflow, I
review each commit to the drivers I'm working on, and either cherry-pick them
back to the relevant RHEL kernel, or I add an empty commit on my private working
branch using the upstream changelog entry, referencing the upstream commit hash.
That way I have a positive indicator that I've looked at a given upstream
commit, and decided not to cherry-pick it.  Prior to integrating with the
mainline kernel I do an interactive rebase of my private branch, and drop any
commits that I've marked as empty.

However, during the period that I'm backporting a driver, I occasionally have
need to do a rebase where I want to keep my empty commits (perhaps I've made a
mistake in cleaning up a previous cherry-pick, or want to rebase my branch to
the origin/master to pick up a common fix).  At those times, I like to keep my
history in tact, empty commits and all, so I can hold on to my notes about which
commits I've reviewed and their dispositions in my work.

> implies "I want everything" -- that doesn't sound like a backport.
Rebases aren't equivalent to "I wan't everything" (at least not always).  I
think what you're thinking of is a merge. A rebase (in the sense that I'm using
it), is either just a movement of my work branch to a newer base, or a replay of
my history to correct an error.

> A cherry-pick implies "I only want certain things" -- that sounds
> like a backport. Maybe your really using rebase to cherry-pick
> several commits.  Using your technique of "empty commit
> placeholders", it seems you could end up with quite a lot of "empty"
> commit placeholders which doesn't seem to make much sense.  Why
See above, the empty commits are really just personal notes.  I expunge them
prior to integration with master.  The empty commits are just personal notes, an
inline area to indiate to myself what upstream commits I have (and have yet to)
consider.

> would you want a bunch of empty commit placeholders in your older
> version bugfix history saying "I didn't want this, but its in the
> newer version."  (who cares?).  Isn't having the stuff you do want
I care, for the purposes of my backporting work..

> recorded as commits enough to make it clear what you brought over?
> You could even edit the "cherry-picked" (or rebased) commit messages
> to document the sha1 of the commit being cherry picked from the
> newer version.  That seems to make more sense to document what you
Yes, I do that, using cherry-pick -x.

> did, as opposed to documenting what you didn't do.
> 
 I document both what I selected to backport, and what I opted not to backport
and why (using the empty commit logs).  

Regardless, the rational behind my specific case doesn't matter too much.  We
allow the creation of empty commits (via git commit --allow-empty) because
explicitly empty commits are sometimes useful.  It would seems to me that, while
it makes sense that rebasing cleaning empty commits by default is quite sane, it
would make good sense to add an allow-empty option to rebase to let them
survive.  I'm looking into adding that option now.

Regards
Neil