Re: Rebase doesn't restore branch pointer back on out of memory

Hi,

I'd like to report a bug in git (observed on git version 1.7.11.msysgit.1). When you do a rebase and it fails due to out of memory exception, rebased branch pointer is changed but commits are not rebased. That makes commits that you are rebasing unreachable (except via reflog):

» git lg
* 4c60761 - (origin/master, origin/HEAD, master) ...

» git rebase master sql_script First, rewinding head to replay your work on top of it... fatal: Out of memory? mmap failed: No error

» git lg
* 4c60761 - (HEAD, origin/master, origin/HEAD, sql_script, master) ...

» git reflog sql_script 4c60761 sql_script@{0}: rebase finished: refs/heads/sql_script onto 4c60761303fccbb0860b28e8094ad17ae8b01d07 13555ed sql_script@{1}: branch: Reset to sql_script@{1}

Expected behaviour:
- restore branch to pre-rebase location on out of memory exception
- not to fall with out of memory in the first place. But for our
repository that could be fixed only after either:
--- a) msysgit would have x64 binary (currently it's not available)
--- b) rebase -m option could be used by default somehow (currently
it's not possible so specify default -m)

-- Alexander Kostikov

On 10/03/2012 03:47 PM, Alexander Kostikov wrote:

There are already some logic in "rebase" that will handles failures. And in the case of failures, the behavior is that "rebase" will just stop and not modify the branch. That allows you can go back to the pre-rebase state by "rebase --abort".

In your case, it's possible that "rebase" is failing at unexpected places, and the error wasn't caught. I tried a few simple cases by forcing some commands to fail during a rebase, but I couldn't reproduce the behavior that you're having. It might help if we can figure out which part of "rebase" or "git" is failing (or running out of memory).

And since you're using msysgit, I guess another possible source of the problem is be that msysgit is not catching the error properly, or not relying the error back to git properly.

On 10/03/2012 06:35 PM, Alexander Kostikov wrote:

I meant "rebase --abort" would be available *if* the error was caught by "rebase". But in your case, "rebase" is probably dying somewhere and the error was not caught, causing "rebase" to think that everything completed successfully, and go ahead to update the branch.

There isn't any built-in to git itself. But one way to get more info is 
running the rebase command this way:
     env SHELLOPTS="verbose" git rebase <your arguments>

That should print out every shell command that rebase executes. Having the last page of that output should give us enough context as to where it's failing.

Just a wild guess: rebase is probably failing at the "format-patch" command. It'd also be interesting to see if "rebase -i" will also workaround the issue. But like you said, there's no way set "-i" or "-m" as the default.

Full script is uploaded to https://dl.dropbox.com/u/10828740/rebase.log Here is the last page:

-----------------------------------[code]
if test -s "$dotest"/rewritten; then
    git notes copy --for-rewrite=rebase < "$dotest"/rewritten
    if test -x "$GIT_DIR"/hooks/post-rewrite; then
        "$GIT_DIR"/hooks/post-rewrite rebase < "$dotest"/rewritten
    fi
fi

rm -fr "$dotest" git gc --auto git rev-parse HEAD

ret=$? test 0 != $ret -a -d "$state_dir" && write_basic_state exit $ret -----------------------------------[/code]

rebase -i fails with different error:

» git rebase -i master rebase_debug fatal: Out of memory, malloc failed (tried to allocate 458753 bytes)

Do you need verbose log for it as well?

-- Alexander

On Thu, Oct 4, 2012 at 8:13 AM, Andrew Wong <andrew.kw.w.lists@gmail.com> wrote:

Also in case of rebase -i the branch pointer is not changed. Thus nothing to fix there.

-- Alexander

On Thu, Oct 4, 2012 at 2:09 PM, Alexander Kostikov <alex.kostikov@gmail.com> wrote:

On 10/04/2012 05:09 PM, Alexander Kostikov wrote:

Judging from that log, I'm pretty sure "rebase" is failing at "format-patch". I was able to reproduce the issue you're having: "rebase" finished and modified the branch even though it actually failed.

"rebase" is not catching that error. I'll try to come up with a patch to fix it later tonight, so that "rebase" will fail correctly. And when it does, you'll be able to do "rebase --abort" to go back to your original state.

Thanks, Andrew! I'm looking forward for the patch.

On Thu, Oct 4, 2012 at 3:52 PM, Andrew Wong <andrew.kw.w@gmail.com> wrote:

'format-patch' is failing due to out of memory, and the error not being caught. So 'rebase' thinks 'am' has completed successfully and continue on with cleanup. i.e. move_to_original_branch So the user loses commits from the original head, and have to rely on reflog to return to the original head.

Since the exit status of 'format-patch' is not available, we have to use || with 'format-patch' to handle the error. Also, when 'format-patch' fails, the state_dir does not necessarily exist, so I'm putting the 'format-patch-failed' file inside GIT_DIR. Is there a better location to put such a file?

The way I handle the error feels a bit bruteforced. Any suggestions on a better way to handle the error?

I also thought about separating 'format-patch' and 'am' into two separate commands, and use an intermediate file to store the output of 'format-patch'. But the intermediate file could get very big, so it didn't seem like a good idea.

Andrew Wong (1):
  rebase: Handle cases where format-patch fails

 git-rebase--am.sh | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

'format-patch' could fail due to reasons such as out of memory. Such failures are not detected or handled, which causes rebase to incorrectly think that it completed successfully and continue with cleanup. i.e. calling move_to_original_branch

Since only the exit status of the last command in the pipeline is available, we rely on || to detect whether 'format-patch' has failed.

Also print messages to help user with how to recover from such failures.

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 git-rebase--am.sh | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

Andrew Wong <andrew.kw.w@gmail.com> writes:

Please make sure there is no marker-file that was leftover from previous invocation or whatever reason, e.g.

	rm -f "$GIT_DIR/format-patch-failed"
        (
		git format-patch -k --stdout --full-index --ignore-if-in-upstream \
			--src-prefix=a/ --dst-prefix=b/ \
			--no-renames $root_flag "$revisions" ||
		echo $? >"$GIT_DIR"/format-patch-failed
	) |
  	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg"

But when format-patch dies for whatever reason, it is likely that the partial output will cause "am" to barf on the last part of it (either "missing patch text" if it stops in the middle of commit log message, or "corrupt patch" if it stops in the middle of a hunk). It may make sense to make this all-or-none, i.e. when format-patch fails, you do not even start "am", something like...

	rm -f "$GIT_DIR/patch-input"
        if ! git format-patch -k --stdout >"$GIT_DIR/patch-input" \
	        --full-index --ignore-if-in-upstream \
		--src-prefix=a/ --dst-prefix=b/ \
		--no-renames $root_flag "$revisions"
	then
		... format-patch barfed, here is how to deal with it...
	else
        	git am <"$GIT_DIR/patch-input" $git_am_opt ...
	fi
	rm -f "$GIT_DIR/patch-input"

but I wonder what the performance implication would be for normal cases.

Here's an alternate method for handling 'format-patch' failing. We remove the pipe between 'format-patch' and 'am' by storing the patch in an intermediate file. This means we can gurantee that 'am' is always invokved with the complete input.

I did some timing on rebasing 500 commits from the git repo. The patch file had a size of 6.9MB, but the overall timings between the pipe approach and intermediate file approach are approximately the same. I did the tests on a Linux machine, so I don't know what the impact would be on other platforms, or in repos with larger files and perhaps binary files.

Andrew Wong (1):
  rebase: Handle cases where format-patch fails

 git-rebase--am.sh | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

'format-patch' could fail due to reasons such as out of memory. Such failures are not detected or handled, which causes rebase to incorrectly think that it completed successfully and continue with cleanup. i.e. calling move_to_original_branch

Instead of using a pipe, we separate 'format-patch' and 'am' by using an intermediate file. This gurantees that we can invoke 'am' with the complete input, or not invoking 'am' at all if 'format-patch' failed.

Also print messages to help user with how to recover from such failures.

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 git-rebase--am.sh | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

Andrew Wong <andrew.kw.w@gmail.com> writes:

Is it just me?  I find this construct

	if ! cmd && ret=$?
        then

very hard to wrap my mind around.  Why not

	git format-patch ... just as before ... \
          ... >"$GIT_DIR/formatted-patches" || {
		# error handling or advices come here...
                rm -f "$GIT_DIR/formatted-patches"
		exit 1
	}

	git am ... just as before ... "$GIT_DIR/formatted-patches" || {
		# possibly another error handling or advices come here...
		rm -f "$GIT_DIR/formatted-patches"
		exit 1
	}

without changing anything else?

You _know_ format-patch failed, not just "seems to have", at this point, no? Why is it impossible to abort?

What have we done before reaching to this point? We know we are doing the basic "git rebase", without any funny "-m/-i/-p" business, so the only thing we have done are (1) detached HEAD at the new onto, (2) set ORIG_HEAD to point at the original tip of the branch being rebased (or the commit we were sitting at, if we are rebasing a detached history), and (3) head_name has the refname of the original branch (or detached HEAD) and branch_name has the name of the branch (or HEAD).

Shouldn't we be just rewinding what we have done so far and error the whole thing out instead? Perhaps the first "# error handling or advises come here..." part may simply be

	case "$head_name" in
	refs/heads/*)
		git checkout "$head_name"
                ;;
	*)
		git checkout "$orig_head"
                ;;
	esac
	cat >&2 <<-\EOF
	Error was found while preparing the patches ($revisions) to
        replay on the rewound head. You cannot rebase this history.
        EOF

or something like that. The format-patch output (and its error) may be of interest in getting help going forward.

For the 'format-patch' part, originally I was going to do something like:

	git format-patch ... || {
		...
	}

But later I thought it's better to use a consistent style as the following 'am' part.

For the 'am' part, if we kept the following line at the end of the if-block:

	fi && move_to_original_branch

then, before exiting the if-block, we would have to do something like:

	test 0 != $ret && false

which seems a bit ugly to me. So I removed the use of '&&', and rearrange the 'write_basic_state' and 'move_to_original_branch' to make the logic flow a bit better and easier to read.

Andrew Wong (1):
  rebase: Handle cases where format-patch fails

 git-rebase--am.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 6 deletions(-)

'format-patch' could fail due to reasons such as out of memory. Such failures are not detected or handled, which causes rebase to incorrectly think that it completed successfully and continue with cleanup. i.e. calling move_to_original_branch

Instead of using a pipe, we separate 'format-patch' and 'am' by using an intermediate file. This gurantees that we can invoke 'am' with the complete input, or not invoking 'am' at all if 'format-patch' failed.

Also remove the use of '&&' at the end of the if-block, and rearrange the 'write_basic_state' and 'move_to_original_branch' to make the logic flow a bit better and easier to read.

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 git-rebase--am.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 6 deletions(-)

Sorry to bother but I was wondering what would be the release version that would have this patch.

-- Alexander

On Wed, Oct 10, 2012 at 8:54 PM, Andrew Wong <andrew.kw.w@gmail.com> wrote:

-- Alexander Kostikov

Alexander Kostikov <alex.kostikov@gmail.com> writes:

That depends on how well the people who are interested in this change test it to smoke out potential issues (if any) in it.

It currently is on the 'pu' branch.