Fix rebase autostash

Gábor reported in
https://public-inbox.org/git/[email protected]/ that t5520-pull.sh fails from time to time, and Alban root-caused this to a bug in the built-in rebase.

This patch series fixes that, and while at it also fixes an oversight of yours truly when helping Pratik with his GSoC project, and it also adds a change on top that makes really, really certain that git stash apply interprets the OID passed to it correctly (as opposed to an insanely large number for the stash reflog).

Please note that I based these patches on top of next (they might be most appropriately applied on top of rebase-in-c-6-final, though).

(Sorry for the v2, v1 did not send due to an UTF-8 character in the Cc:
list, a bug that still needs to be fixed in GitGitGadget.)

Johannes Schindelin (3):
rebase (autostash): avoid duplicate call to state_dir_path() rebase (autostash): store the full OID in <state-dir>/autostash rebase (autostash): use an explicit OID to apply the stash

 builtin/rebase.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

base-commit: 500967bb5e73b67708a64a4360867cf2407ba880
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-52%2Fdscho%2Ffix-rebase-autostash-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-52/dscho/fix-rebase-autostash-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/52

Range-diff vs v1:

 1:  88241ad32 = 1:  88241ad32 rebase (autostash): avoid duplicate call to state_dir_path()
 2:  86107a6d0 = 2:  86107a6d0 rebase (autostash): store the full OID in <state-dir>/autostash
 3:  d3b47a4c1 = 3:  07140a71d rebase (autostash): use an explicit OID to apply the stash

gitgitgadget

From: Johannes Schindelin <[email protected]>

We already called that function at this point, and stored the result in the path variable. We might just as well use it ;-)

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 313a8263d..d21504d9f 100644
@@ -251,7 +251,7 @@ static int apply_autostash(struct rebase_options *opts)
 	if (!file_exists(path))
 		return 0;
 
-	if (read_one(state_dir_path("autostash", opts), &autostash))
+	if (read_one(path, &autostash))
 		return error(_("Could not read '%s'"), path);
 	argv_array_pushl(&stash_apply.args,
 			 "stash", "apply", autostash.buf, NULL);

gitgitgadget

From: Johannes Schindelin <[email protected]>

It was reported by Gábor Szeder and analyzed by Alban Gruin that the built-in rebase stores only abbreviated stash hashes in the autostash file.

This is problematic e.g. in t5520-pull.sh, where the abbreviated hash is so short that it sometimes consists only of digits, which are subsequently mistaken ("DWIMmed") for numbers by git stash apply.

Let's align the behavior of the built-in rebase with the scripted rebase and store the full stash hash instead. That makes it a lot less likely that it consists only of digits.

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d21504d9f..418624837 100644
@@ -1375,7 +1375,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			if (safe_create_leading_directories_const(autostash))
 				die(_("Could not create directory for '%s'"),
 				    options.state_dir);
-			write_file(autostash, "%s", buf.buf);
+			write_file(autostash, "%s", oid_to_hex(&oid));
 			printf(_("Created autostash: %s\n"), buf.buf);
 			if (reset_head(&head->object.oid, "reset --hard",
 				       NULL, 0, NULL, NULL) < 0)

gitgitgadget

From: Johannes Schindelin <[email protected]>

When git stash apply <argument> sees an argument that consists only of digits, it tries to be smart and interpret it as stash@{<number>}.

Unfortunately, an all-digit hash (which is unlikely but still possible) is therefore misinterpreted as stash@{<n>} reflog.

To prevent that from happening, let's append ^0 after the stash hash, to make sure that it is interpreted as an OID rather than as a number.

---
 builtin/rebase.c | 2 ++
 1 file changed, 2 insertions(+)

index 418624837..30d58118c 100644
@@ -253,6 +253,8 @@ static int apply_autostash(struct rebase_options *opts)
 
 	if (read_one(path, &autostash))
 		return error(_("Could not read '%s'"), path);
+	/* Ensure that the hash is not mistake for a number */
+	strbuf_addstr(&autostash, "^0");
 	argv_array_pushl(&stash_apply.args,
 			 "stash", "apply", autostash.buf, NULL);
 	stash_apply.git_cmd = 1;

gitgitgadget

Oh, this is clever.

FWIW, all patches look good to me, barring the typo below.

Yeah, we can do this as we know we'd be dealing with a commit-ish. If we made a mistake to use a tree object to represent a stash, this trick wouldn't have been possible ;-)

Will squash this in and add your reviewed-by before queuing.

Thanks, both.

Hi,

Thank you all!
Dscho

Hi Johannes,

this looks good to me, too!

Cheers,
Alban