From 4e497795f4c019d4d852608404139cfc3d1b30e6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 22 Oct 2018 15:15:02 -0700 Subject: [PATCH 1/5] rebase (autostash): avoid duplicate call to state_dir_path() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We already called that function at this point, and stored the result in the `path` variable. We might just as well use it ;-) Signed-off-by: Johannes Schindelin Reviewed-by: SZEDER Gábor Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 4bfba202dc..84884d48cc 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -250,7 +250,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); From 6d7b6743095dce90526cfe2aaea77bba48b806e8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 22 Oct 2018 15:15:03 -0700 Subject: [PATCH 2/5] rebase (autostash): store the full OID in /autostash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Signed-off-by: Johannes Schindelin Reviewed-by: SZEDER Gábor Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 84884d48cc..0041ea65d6 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1376,7 +1376,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) From 9001cceeb6855fe997d81301d35b481c98176a38 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 22 Oct 2018 15:15:05 -0700 Subject: [PATCH 3/5] rebase (autostash): use an explicit OID to apply the stash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `git stash apply ` sees an argument that consists only of digits, it tries to be smart and interpret it as `stash@{}`. Unfortunately, an all-digit hash (which is unlikely but still possible) is therefore misinterpreted as `stash@{}` 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. Signed-off-by: Johannes Schindelin Reviewed-by: SZEDER Gábor Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- builtin/rebase.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/rebase.c b/builtin/rebase.c index 0041ea65d6..4c664f958c 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -252,6 +252,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 mistaken for a number */ + strbuf_addstr(&autostash, "^0"); argv_array_pushl(&stash_apply.args, "stash", "apply", autostash.buf, NULL); stash_apply.git_cmd = 1; From 5a4dc1ea8478fb0a086b7404195696e5270fbd42 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 23 Oct 2018 12:57:16 -0700 Subject: [PATCH 4/5] rebase --autostash: demonstrate a problem with dirty submodules It has been reported that dirty submodules cause problems with the built-in rebase when it is asked to autostash. The symptom is: fatal: Unexpected stash response: '' This patch adds a regression test that demonstrates that bug. Original report: https://github.com/git-for-windows/git/issues/1820 Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- t/t3420-rebase-autostash.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index 0c4eefec76..7eb9f90419 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -351,4 +351,14 @@ test_expect_success 'autostash is saved on editor failure with conflict' ' test_cmp expected file0 ' +test_expect_failure 'autostash with dirty submodules' ' + test_when_finished "git reset --hard && git checkout master" && + git checkout -b with-submodule && + git submodule add ./ sub && + test_tick && + git commit -m add-submodule && + echo changed >sub/file0 && + git rebase -i --autostash HEAD +' + test_done From b1811acbb41c296889c7701e5ec7d5462df9797b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 23 Oct 2018 12:57:17 -0700 Subject: [PATCH 5/5] rebase --autostash: fix issue with dirty submodules Since we cannot stash dirty submodules, there is no use in requiring them to be clean (or stash them when they are not). This brings the built-in rebase in line with the previous, scripted version, which also did not care about dirty submodules (but it was admittedly not very easy to figure that out). This fixes https://github.com/git-for-windows/git/issues/1820 Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- builtin/rebase.c | 2 +- t/t3420-rebase-autostash.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 4c664f958c..d184038d0c 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1352,7 +1352,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) update_index_if_able(&the_index, &lock_file); rollback_lock_file(&lock_file); - if (has_unstaged_changes(0) || has_uncommitted_changes(0)) { + if (has_unstaged_changes(1) || has_uncommitted_changes(1)) { const char *autostash = state_dir_path("autostash", &options); struct child_process stash = CHILD_PROCESS_INIT; diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index 7eb9f90419..f355c6825a 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -351,7 +351,7 @@ test_expect_success 'autostash is saved on editor failure with conflict' ' test_cmp expected file0 ' -test_expect_failure 'autostash with dirty submodules' ' +test_expect_success 'autostash with dirty submodules' ' test_when_finished "git reset --hard && git checkout master" && git checkout -b with-submodule && git submodule add ./ sub &&