git-rebase skips automatically no more needed commits

Hello,

When rebasing my current branch onto the last master one, there're sometimes some commits which doesn't add anything anymore.

Currently git-rebase produces the following message:

    nothing added to commit but untracked files present (use "git add" to track)

Would it be possible to add a new option to this command so it simply skip the unneeded commit ?

Thanks

Francis Moreau <francis.moro@gmail.com> writes:

Our assumption has always been that it is a notable event that a patch that does not get filtered with internal "git cherry" (which culls patches that are textually identical to those that are already merged to the history you are rebasing onto) becomes totally unneeded and is safe to ask for human confirmation in the form of "rebase --skip" than to ignore it and give potentially incorrect result silently.

Obviously you do not find it a notable event for some reason. We would need to understand why, and if the reason is sensible, it _might_ make sense to allow a user to say "git rebase --ignore-merged" or something when starting the rebase.

Hello Junio,

On Tue, Sep 6, 2011 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Ok then I think this "git cherry" filtering is not working in my case since it seems to me that commit that I cherry-picked are not filtered, please see below.

My use case is the following: I'm maintaining a branch from an upstream project (the kernel one). While the upstream project follows its development cycle (including some fixes), my branch is stuck. I sometime want to includes (or rather backport) some commits that happened later in the development cycle. To do that I use "git cherry-pick".

After some period, I'm allowed to rebase to a more recent commit from the upstream project and this rebase 'cancel' the previous 'git cherry-pick' I did. But for some reasons, git telling me "nothing added to commit ...", which is expected in my case, well I think, hence my question.

Thanks.

Francis Moreau venit, vidit, dixit 06.09.2011 21:28:

Unless you had to resolve a conflict when cherry-picking, the picked commit should be patch-equivalent to the original one, and thus dropped by rebase automatically.

How do the two patches compare:

git show $picked >a git show $original >b git patch-id <a git patch-id <b git diff --no-index a b

Michael

Francis Moreau <francis.moro@gmail.com> writes:

Because "rebase --onto A B C" has to be usable when commit A does not have any ancestry relationship with the history between B and C, I wouldn't be surprised if the command does not look at commits on the history that lead to A that _might_ be related to the ones between B and C. It does not know how far to dig from A and stop, and obviously you do not want to dig down to the beginning of the history. "rebase A B" on the other hand can safely stop digging the history back from A down to where the history leading to B forked (i.e. merge base between A and B).

I do not know if "expected" is the right adjective; understandable---yes, unsurprised---yes, justifiable--- I dunno.

On Wed, Sep 7, 2011 at 6:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

But is it the common case ? That would mean that A and B are part of totaly independant branches which is not that common I think.

I'm using "git rebase --onto A B C" because I want to simplify git's work. I know that any commits between A..B are already in A history and I'm not interested in rebasing them anyways.

Hmm I dont understand why "rebase --onto A B C" wouldn't try to find the merge base between A and B. If it doesn't (which is quite uncommon I guess) then don't do the filtering as we're currently doing but if there's a merge base (common case) then do the filtering.

No ?

On Thu, 8 Sep 2011, Francis Moreau wrote:

Patches that are in both sides of v2.6.39...foo will be filtered, but as Junio said, what is in master is not considered. In your case, since foo was created from the tag, there should of course be no commits in foo..v2.6.39.

What work? Calculating mege-base and patch-ids? But that's exactly what you said you don't want to avoid, no?

Did you mean "B..A" here?

They wouldn't be rebased by running "git rebase A C" either... Maybe I'm misunderstanding something.

It could do that. I think that's what Junio meant by "justifiable--- I dunno". In your case, though, it seems like "git rebase master foo" would do exactly what you want.

Somewhat related, I think "git rebase --onto A B C" should NOT filter out patches that also appear both sides of "B...C", because when "--onto" is used, "B" is only used as a way to calculate the merge-base. I think it would make more sense to filter out from appears in both "B..C" what also appears in "C..A". This has been on my todo list for way too long. Some day I'll send out a patch for it and we'll see what others think.

Martin

On Thu, 8 Sep 2011, Francis Moreau wrote:

See the section called "SPECIFYING RANGES" in "git --help rev-parse". It doesn't define "both sides", but what I mean by that is "both in v2.6.39..foo and in foo..v2.6.39". I believe that section will answer most of the other questions too.

Ok, that is different. I think you used v2.6.39 instead of foo~10 previously. Assuming that v2.6.39 is the merge base of foo and master and that foo~10 is a later commit than v2.6.39, you are right that "git rebase --onto master foo~10 foo" could potentially filter out patches already in foo..master, without calculating patch-ids for all commits in master..foo for that matter. I think that would make sense and as I said, it has been on my todo list for a long time. If necessary, we could have a flag to disable the filtering e.g. when the user knows that master is part of a completely separate history from foo.

Martin

On Fri, Sep 9, 2011 at 4:23 AM, Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> wrote:

That's what I meant.

you meant master..foo~10, didn't you ?

please let me know when you submitting your work, I'm interested to see it.

Can't git figure this out itself ? (I'm not saying the switch is useless)

Thanks

On Fri, 9 Sep 2011, Francis Moreau wrote:

Yes. (I actually did mean "all commits in master..foo", but "_any_ commits in master..foo~10" is more clear and exact.)

Will do. It's not really that much work, but I'm not sure when I will have time for it. If you or anyone else is interested in doing it, you are of course welcome.

Yes, but this hypothetical flag would only be to speed things up by avoid going to the roots to find out that the histories are disjoint.

Martin

On Fri, Sep 9, 2011 at 3:06 PM, Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> wrote:

I'm a simple mortal user ;)

You're right but in my (mortal) experience, I'm always rebasing branches onto another one which is not disjoint, so I'm assuming it's the (very) common case, but I may be wrong. And what I'm assuming is correct, I think it's make more sense to do the filtering by default unless the flag tells to do otherwise.

Thanks

On Fri, 9 Sep 2011, Martin von Zweigbergk wrote:

It seems to be a little less straight-forward than I had first expected.

We want to get all commits in $oldbase..$branch that do not share patch-id with commits in $oldbase..$newbase. These are the commits prefixed by '+' in 'git cherry $newbase $branch $oldbase'. However, just getting the list of commits is not enough, because we currently don't explicitly provide a list of commits to create patches from, but instead run something like 'git format-patch --ignore-if-in-upstream --stdout | git am --rebasing'.

I can see a few different solutions.

 1. Teach git-format-patch a --stdin option that makes it read the
    list of commits from the command line. git-format-patch currently
    itself walks the history, so I'm not sure how such an option
    should interact with current option. The options should probably
    be mutually exclusive.

 2. Somehow modify git-rebase--am.sh not to depend on format-patch. In
    [1], Junio mentions rewriting git-rebase--am.sh "to have
    format-patch avoid the cost of actually generating the patch text"
    and "when using "am" for rebasing we do not really care anything
    but the commit object names". If all we need is the commit name,
    why would we not use cherry-pick/sequencer instead of git-am?
    Sorry if this makes no sense; I'm not familiar with the git-am
    code.

 3. If I'm reading the code correctly, 'git cherry $upstream $branch
    $limit' does one walk to find patch-ids for "$upstream
    ^$branch". It then walks "$branch ^$upstream ^$limit" and removes
    the commits that have the same patch-id as a commit found in the
    first walk. Since format-patch uses the revision walking
    machinery, we could make it possible to ask git-rev-list directly
    what git-cherry does, i.e. to ask for "commits in a..b that don't
    share patch-id with commits in c..d". We could even make it more
    generic by allowing queries like "commits in
    $rev_list_expression_1 that don't share patch-id with commits in
    $rev_list_expression_2".

I'm not sure which option is better. Option 1 seems easiest to implement. Option 3 seems a bit harder, but may bring more value. I'm just not sure if teaching rev-list about patch-ids is generally useful or if it would feel more like a hack for this specific case. Option 2 is less clear to me and I would probably need more input before implementing, but I trust Junio that it would be a good long-term solution.

I will be moving soon and I don't know how much time I will have to implement any of this. Still, I would be happy to hear your opinions. Maybe I'm just missing something and there is even a simple solution to my problem.

Martin

[1] http://thread.gmane.org/gmane.comp.version-control.git/180976/focus=181085

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

What I said is "all 'am' need to use from its input while rebasing is the commit object name"; that is very different from "we have only commit object name so we must use cherry-pick" or "because we have commit object name, we can afford to use cherry-pick".

Look for $rebasing in git-am.sh and notice that:

 - We run get-author-ident-from-commit on $commit to keep the authorship
   information in $dotest/author-script for later use, instead of letting
   them read from $dotest/info prepared by "mailinfo".

 - My "fixing it would be just the matter of doing this" patch in a
   separate thread we had recently ("mailinfo" would obviously have
   trouble telling where the message ends whilerebasing a change with
   patch text in the commit log message) adds a similar logic to extract
   the patch text out of $commit in the same place as above, to override
   what "mailinfo" prepares in $dotest/patch.

 - Even with the above two differences from the normal "separate the
   metainfo and patch from the mailbox, and use them" codepath, $rebasing
   codepath uses the same "git apply && git write-tree && git commit-tree"
   flow.

Use of "apply && write-tree && commit-tree" is to avoid the merge overhead of using cherry-pick, and historically, it used to matter a lot, because starting merge-recursive was very slow (it was a Python script). Even though we have merge-recursive rewritten in C these days, it probably still matters, especially for large-ish projects, because merge will trash the cached-tree information in the index, making write-tree inefficient.

Hi,

On Wed, 14 Sep 2011, Junio C Hamano wrote:

Right. I didn't mean to suggest that we should use cherry-pick.

Thanks! I had no idea about these "tricks" in git-am.sh. Now I understand much better what you meant. So we are currently getting the metainfo from the commit rather than from the mailbox. With your patch, we would also get the patch body from the commit. The only thing remaining after that is the commit log message, correct?

Once all that git-am reads from the mailbox while in $rebasing mode is the commit name, it would be as small change to teach it to read a list of commit names instead. Is this what you meant or did you really mean for format-patch to generate output without patch body? At this point the name 'am' becomes quite misleading, but maybe that's not worth worrying about.

Thanks, Martin

On Thu, 15 Sep 2011, Martin von Zweigbergk wrote:

[CC'ing Ramkunar regarding the footnote]

It turns out the commit log message have been taken from the commit for ages. Sorry about not checking the code first.

I have started implementing the changes to git-am.sh now. I have made it not use mailinfo when in $rebasing mode, as you suggested. So now git-am works even if only a list of

  From $sha1 Mon Sep 17 00:00:00 2001

lines are fed to it. Since only the $sha1 is needed, we should probably make --rebasing imply a patch-format is just a simple list of revisions. Since the --rebasing flag is only for internal use, this should be safe when it comes to backwards compatibility. [1]

Now the question is how to generate such lines, without the overhead of a full format-patch call. We still want the --ignore-if-in-upstream functionality, so we can not use plain rev-list. We also want it to work with --root, so we can not use git-cherry. Finally, we can not use 'git rev-list --cherry-pick --right-only' since it seems to be inherently for symmetric ranges and therefore does not support git-cherry's <limit> parameter.

If I'm not mistaken, we would have to teach some new option to some command, but which one?

It doesn't feel right to teach format-patch a --commit-name-only, simply because it would then no longer be a patch.

It's probably easiest to teach git-cherry a --root flag. Does that sound like the right solution to people? I think it makes sense for such an option to exist.

More involved would be to teach rev-list --cherry-pick also for asymmetric ranges. I would imagine that "--cherry-pick $branch --not $newbase $oldbase" could be defined as "all commits in $branch that are not patch-equivalent to any commits in $newbase or $oldbase". Just s/patch-equivalent/identical/ and you have the regular definition of the rev-list, right? There is of course no need to actually calculate the patch-id for any commits are not in "$branch --not $newbase $oldbase" (identical => patch-equivalent).

I would go for teaching git-cherry --root, unless the "rev-list --cherry-pick for asymmetric ranges" option would be useful elsewhere. Either way, teaching git-cherry --root would not be a bad thing, I think.

Martin

 [1] Long term, would it make sense to teach 'git cherry-pick' a '-2'
     option instead of using the '-3' option to 'git am'? (Where 'git
     cherry-pick -2' would try "apply && write-tree && commit-tree"
     before falling back to merging.)

Hi Martin,

Martin von Zweigbergk writes:

Ah, nice :)

Using git-format-patch for anything except emailing patches to the maintainer/ list is a mistake imo.

Either this, or we have to write another wrapper around git-patch-id.

Yeah, in the wake of scripts already doing "apply && write-tree && commit-tree" by hand, I think this proposal for git-cherry-pick makes a lot of sense. I'm not sure if the option should be called '-2' though.

Thanks.

-- Ram

Hi Ram and Thomas,

On Sat, 24 Sep 2011, Ramkumar Ramachandra wrote:

I think I was confused here; git-cherry should work just fine. Simply leaving out the limit, or setting it to $onto, makes the walk go all the way to the roots. After thinking a bit more about this, I'm instead wondering what the purpose of the --root flag to git-rebase is. Thomas introduced it in 190f532 (rebase: learn to rebase root commit, 2009-01-05).

If I understand correctly, it was introduced to solve exactly the same reason problem that made Francis start this thread -- to avoid re-applying patches that are already in $onto. In Thomas's patch, git-rebase started passing $onto..$orig_head instead of $upstream..$orig_head to git-format-patch. It also started passing --root to git-format-patch, but that was made unneccessary a few days later in 68c2ec7 (format-patch: show patch text for the root commit, 2009-01-10).

After my patches (that are not yet ready) that calculate revisions as "git cherry $onto $orig_head $upstream", I don't think there should be any need for the --root flag. The only place the flag is checked now (in my current work tree, that is) is when deciding how to interpret the remaining arguments. Although I have not tried (temporarily) rewriting all the test cases from

  git rebase --root --onto upstream branch

to

  git rebase upstream branch

, I think it should work. What do you think, Thomas? I saw that "--root" is also passed to the hook. Should that value be passed to the hook also when the old base is not explicitly a root (by "rebase --root"), but only implicitly so (by an $onto that is disjoint from $branch)?

Martin

Martin von Zweigbergk wrote:

Not quite; I wrote it because at the time, there was no way to transplant git history onto a git-svn "make empty subdir" commit for later dcommitting. So the main point was really

I still think it would be natural for a user to want a way to say "all the way back to the root commit". At least for me the "full" rebase invocation is

  git rebase --onto onto base branch

In that mindset it comes natural to replace 'base' with --root if you want to go all the way.

Maybe I don't trust rebase enough to do what I want ;-)

I think I did it that way because if you use --root, the base/upstream argument is missing, and the hook needs to know that.

If the user specifies an upstream that is disjoint from the branch itself, the hook gets the upstream argument and can presumably work it out from there. So you could perhaps save the hook some trouble if you *know* that it's a disjoint rebase, but I wouldn't spend too much time on that.

On Mon, Sep 26, 2011 at 5:10 AM, Thomas Rast <trast@student.ethz.ch> wrote:

But that was only due to the bug/limitation in format-patch that you (?) fixed a few days later, no?

Sure, I can understand that. Still, it would just be an alternative syntax and nothing else. So I think I should also update the documentation after my other patches to make it clear that that is the case. We also have to think about backwards compatibility, so I'm obviously not suggesting that we should stop supporting the flag; I just wanted to make sure that it is not technically needed.

The hook would still have to be able to handle both cases (i.e. getting the upstream argument as "--root" or simply a commit that happens to be disjoint from the the branch-to-be-rebased). I believe that has been the case since that bug in format-patch was fixed. So if we were to change git-rebase so it no longer passes the --root flag to the hook, I think any (correctly written) hooks should still work. I don't see much reason to change it, though; if the user uses the --root syntax, we can pass the flag, otherwise we don't.

Martin