file disappears after git rebase (missing one commit)

file disappears after git rebase (missing one commit)

[Torgil Svensson]

Hi,

I'm trying to rebase a branch ("msmtp") on another branch ("devel").
The msmtp has a number of commits that are already in the devel branch
(but with different history) and one new commit that adds one file.

$ git clone git://repo.or.cz/msysgit.git
$ cd msysgit
$ git rev-parse origin/msmtp
b11cf4ce6262a7c3b243e3cfdc70e6b44682cb59
$ git rev-parse origin/devel
57aa8405103856106ec0e31453089c33c899c98b
$ git -b checkout devel origin/devel
$ git -b checkout msmtp origin/msmtp
$ git show-branch msmtp devel
* [msmtp] Added msmtp.exe SMTP client
 ! [devel] Add disk summarize tool (du.exe)
--
 + [devel] Add disk summarize tool (du.exe)
 + [devel^] gdb updated to v6.6
 + [devel~2] w32api updated to v3.10
 + [devel~3] Updated gcc to v3.4.5
 + [devel~4] Updated binutils to v2.17.50
 + [devel~5] Remove remnants of the c++ compiler
 + [devel~6] GitMe: only fetch 'master' of msysgit.git
 + [devel~7] GitMe: inline 7z's install script
 + [devel~8] GitMe: avoid dependency on cmd.exe
 + [devel~9] msysGit: adjust for submodule layout
 + [devel~10] msysGit: we have 7zip installed in /share/7-Zip/ now
 + [devel~11] msysGit: 7z cannot update existing installers
 + [devel~12] msysGit: move scripts to /share/msysGit/
 + [devel~13] WinGit: do not pack builtins, but copy them when unpacking
 + [devel~14] Update TODO: Marius squashed two
 + [devel~15] WinGit: strip executables (Issue 25)
 + [devel~16^2] GitMe: fix HTTP transport
 + [devel~16^2^] Undo hacky she-bang fixup
 + [devel~16^2~2] Issue 21: core.autocrlf should be set to true (at
least for end-users)
 + [devel~16^2~3] Add clear script, and remove the clear=clsb alias in profile
 + [devel~16^2~4] Make 7z functional
 + [devel~16^2~5] msys: support for Windows XP x64
 + [devel~16^2~6] Removed all SuperGitMe functionality Also propagated
latest fixes and made installer even smaller
 + [devel~16^2~7] Fixed Issue 37: Errors during install because repo
now has tags
 + [devel~18] Issue 21: core.autocrlf should be set to true (at least
for end-users)
 + [devel~19] Add clear script, and remove the clear=clsb alias in profile
 + [devel~20] Make 7z functional
 + [devel~21] msys: support for Windows XP x64
 + [devel~22] GitMe SuperFetch
 + [devel~23] Latest git submodule
*  [msmtp] Added msmtp.exe SMTP client
*  [msmtp^] gdb updated to v6.6
*  [msmtp~2] w32api updated to v3.10
*  [msmtp~3] Updated gcc to v3.4.5
*  [msmtp~4] Updated binutils to v2.17.50
*  [msmtp~5] Remove remnants of the c++ compiler
*+ [devel~24] GitMe: check if cygwin is in PATH; if so abort installer.
$ find bin -name "msmtp.exe"
bin/msmtp.exe

Note! that this file is added with the commit "*  [msmtp] Added
msmtp.exe SMTP client". After rebase I expect that this commit will be
on top of the devel branch.

$ cat .git/HEAD
ref: refs/heads/msmtp
$ git rebase devel
First, rewinding head to replay your work on top of it...
HEAD is now at 57aa840... Add disk summarize tool (du.exe)
Nothing to do.
$ git show-branch msmtp devel
* [msmtp] Add disk summarize tool (du.exe)
 ! [devel] Add disk summarize tool (du.exe)
--
*+ [msmtp] Add disk summarize tool (du.exe)
$ find bin -name "msmtp.exe"

And the msmtp commit + file is lost.

I've tested on windows (4msysgit.git):
$ git --version
git version 1.5.3.rc4.mingw.2.49.g3314

And on linux (git.git):
$ git --version
git version 1.5.2.5.g0734d

And on linux with next branch (git.git)
$ git version
git version 1.5.3.rc5.843.gdac75


Is this a bug?  Any ideas?

Best regards,

//Torgil

Re: file disappears after git rebase (missing one commit)

[Linus Torvalds]

On Sat, 18 Aug 2007, Torgil Svensson wrote:
>
> $ git rebase devel
> First, rewinding head to replay your work on top of it...
> HEAD is now at 57aa840... Add disk summarize tool (du.exe)
> Nothing to do.

Ok. "git rebase" really does believe that there's nothing to do.

The reason, I think, is that I suspect that the newly added file is a 
binary file, no? That, in turn, will mean that the *patch* will have no 
patch ID (or rather, it will have an empty patch ID) - which in turn will 
make it invisible to "--ignore-if-in-upstream" if there are already some 
*other* patches that also just adds a binary file (which I think there is: 
I think upstream has "Add disk summarize tool (du.exe)" which I assume has 
exactly the same patch fingerprint).

In other words, "git rebase" really is just a series of cherry-picks, but 
it avoids patches that have the same patch ID as something that is already 
upstream. That helps *enormously*, but it so happens that the patch ID's 
don't work really well for binary diffs.

Try this patch - see if it helps. Totally untested! It will enable patch 
ID's on binary diffs too, which should avoid this issue.

		Linus

---
diff --git a/patch-ids.c b/patch-ids.c
index a288fac..4a3432e 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -122,6 +122,7 @@ int init_patch_ids(struct patch_ids *ids)
 	memset(ids, 0, sizeof(*ids));
 	diff_setup(&ids->diffopts);
 	ids->diffopts.recursive = 1;
+	ids->diffopts.binary = 1;
 	if (diff_setup_done(&ids->diffopts) < 0)
 		return error("diff_setup_done failed");
 	return 0;

Re: file disappears after git rebase (missing one commit)

[Torgil Svensson]

On 8/18/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> The reason, I think, is that I suspect that the newly added file is a
> binary file, no?

Yes, that's correct.


> That, in turn, will mean that the *patch* will have no
> patch ID (or rather, it will have an empty patch ID) - which in turn will
> make it invisible to "--ignore-if-in-upstream" if there are already some
> *other* patches that also just adds a binary file (which I think there is:
> I think upstream has "Add disk summarize tool (du.exe)" which I assume has
> exactly the same patch fingerprint).

The "du.exe" is only in the devel branch but there is five other
patches that meets your criteria.


> In other words, "git rebase" really is just a series of cherry-picks, but
> it avoids patches that have the same patch ID as something that is already
> upstream. That helps *enormously*, but it so happens that the patch ID's
> don't work really well for binary diffs.

Git cherry-pick seems to work on that particular patch:

$ git cherry-pick b11cf4ce6262a7c3b243e3cfdc70e6b44682cb59
Finished one cherry-pick.
Created commit 92a58d3: Added msmtp.exe SMTP client
 1 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 bin/msmtp.exe


> Try this patch - see if it helps. Totally untested! It will enable patch
> ID's on binary diffs too, which should avoid this issue.

That didn't help. Same symptom.

//Torgil

Re: file disappears after git rebase (missing one commit)

[Linus Torvalds]

On Sat, 18 Aug 2007, Torgil Svensson wrote:
> 
> > In other words, "git rebase" really is just a series of cherry-picks, 
> > but it avoids patches that have the same patch ID as something that is 
> > already upstream. That helps *enormously*, but it so happens that the 
> > patch ID's don't work really well for binary diffs.
> 
> Git cherry-pick seems to work on that particular patch:

Yes, cherry-picking itself works, it's just that "git rebase" probably 
won't even *try* to cherry-pick it because it thinks it is already 
applied.

> > Try this patch - see if it helps. Totally untested! It will enable 
> > patch ID's on binary diffs too, which should avoid this issue.
> 
> That didn't help. Same symptom.

Yeah, I was thinking about the external "git-patch-id" program, which 
actually takes the diff and looks at it from there. But 
"--ignore-if-in-upstream" does its own binary file testing, and doesn't 
use the generic diff code at all. 

So the following patch is likely much better..

		Linus

---
 diff.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 97cc5bc..a7e7671 100644
--- a/diff.c
+++ b/diff.c
@@ -2919,10 +2919,6 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 				fill_mmfile(&mf2, p->two) < 0)
 			return error("unable to read files to diff");
 
-		/* Maybe hash p->two? into the patch id? */
-		if (diff_filespec_is_binary(p->two))
-			continue;
-
 		len1 = remove_space(p->one->path, strlen(p->one->path));
 		len2 = remove_space(p->two->path, strlen(p->two->path));
 		if (p->one->mode == 0)

Re: file disappears after git rebase (missing one commit)

[Torgil Svensson]

On 8/18/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Yeah, I was thinking about the external "git-patch-id" program, which
> actually takes the diff and looks at it from there. But
> "--ignore-if-in-upstream" does its own binary file testing, and doesn't
> use the generic diff code at all.
>
> So the following patch is likely much better..

This patch made the difference and solved the issue for me. Thanks

//Torgil

Take binary diffs into account for "git rebase"

[Linus Torvalds]

We used to not generate a patch ID for binary diffs, but that means that 
some commits may be skipped as being identical to already-applied diffs 
when doing a rebase.

So just delete the code that skips the binary diff. At the very least, 
we'd want the filenames to be part of the patch ID, but we might also want 
to generate some hash for the binary diff itself too.

This fixes an issue noticed by Torgil Svensson.

Tested-by: Torgil Svensson <torgil.svensson@gmail.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Junio, you might want to do as the comment says, instead of just hashing 
whatever random binary patch. Your call.

On Sat, 18 Aug 2007, Torgil Svensson wrote:
> 
> This patch made the difference and solved the issue for me. Thanks

 diff.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index 97cc5bc..a7e7671 100644
--- a/diff.c
+++ b/diff.c
@@ -2919,10 +2919,6 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 				fill_mmfile(&mf2, p->two) < 0)
 			return error("unable to read files to diff");
 
-		/* Maybe hash p->two? into the patch id? */
-		if (diff_filespec_is_binary(p->two))
-			continue;
-
 		len1 = remove_space(p->one->path, strlen(p->one->path));
 		len2 = remove_space(p->two->path, strlen(p->two->path));
 		if (p->one->mode == 0)