From 71064e6008d13de72de0387a1301a557477cbed2 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 --- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 9dc8475cd3..42f320453a 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 12aeb00a22e589f85e26da833e05bb1a7e412d07 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 --- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 42f320453a..cd6beb96b4 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1374,7 +1374,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 b98e914e4650b876b9049bff1a5a33f4bfda0e0a 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 --- builtin/rebase.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/rebase.c b/builtin/rebase.c index cd6beb96b4..e9995c9a37 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 97bd162ca21dcc190327570ae8e6bd8a54582a23 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 --- 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 e243700660..b6e18566b5 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 ffae8b2f904f0a82417ac24cb2684bbe5ca234e1 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 --- 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 e9995c9a37..bc92c9b529 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1350,7 +1350,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 b6e18566b5..001d6243c9 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 &&