{"thread":{"id":"33581","subject":"[PATCH 0/7] Introduce rebase.autostash","startedAt":"2013-04-23T14:01:57Z","lastAt":"2013-04-25T14:30:44Z","messageCount":28,"participants":["Ramkumar Ramachandra","Martin von Zweigbergk","Junio C Hamano","Phil Hord","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"215219","messageId":"1366725724-1016-1-git-send-email-artagnon@gmail.com","threadId":"33581","inReplyTo":null,"subject":"[PATCH 0/7] Introduce rebase.autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T14:01:57Z","receivedAt":"2013-04-23T14:01:57Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Yes!\n\nAfter struggling with shell for a couple of days, I finally managed to\nachieve what I set out to do: implement a rebase.autostash without\nleaking the autostash detail into specific rebases. And I'm\nabsolutely elated with my approach. Patches [1/7] to [6/7] make no\nreference to autostash, and pretend to be nice by-the-way changes\nunrelated to any new feature. [7/7] suddenly introduces a little bit\nof code in git-rebase.sh, and everything just starts working\nmagically.\n\nOkay, now for the bad news. I've mostly just worked backward from\ntests, so I can't justify all my changs fully. Also, there are no new\ntests, because I don't know what to test: we pretty much have to run\nthe entire rebase testsuite with rebase.autostash turned on, right?\n\nIf you can't wait for a few weeks (reviews, iterations) and want to\nuse this immediately, just pull in the rebase.autostash branch from\ngh:artagnon/git and enjoy.\n\nCheers.\n\nRamkumar Ramachandra (7):\n am: suppress error output from a conditional\n rebase -i: don't error out if $state_dir already exists\n am: tighten a conditional that checks for $dotest\n am: don't do housekeeping when rebasing\n rebase -i: return control to the caller, for housekeeping\n sh-setup: introduce require_clean_work_tree --quiet\n rebase: implement --[no-]autostash and rebase.autostash\n\n Documentation/config.txt | 8 ++++++++\n Documentation/git-rebase.txt | 10 ++++++++++\n git-am.sh | 15 ++++++++++-----\n git-rebase--am.sh | 8 ++++----\n git-rebase--interactive.sh | 14 ++++++--------\n git-rebase.sh | 41 ++++++++++++++++++++++++++++++++++++++++-\n git-sh-setup.sh | 10 +++++++---\n 7 files changed, 85 insertions(+), 21 deletions(-)\n\n-- \n1.8.2.1.578.ga933817\n"},{"id":"215221","messageId":"1366725724-1016-2-git-send-email-artagnon@gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/7] am: suppress error output from a conditional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T14:01:58Z","receivedAt":"2013-04-23T14:01:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"When testing if the $dotest directory exists, and if $next is greater\nthan $last, the script currently executes `cat` on files that might\nnot exist. So, suppress the error output from `cat`.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n git-am.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex c092855..88aa438 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -446,8 +446,8 @@ done\n # If the dotest directory exists, but we have finished applying all the\n # patches in them, clear it out.\n if test -d \"$dotest\" &&\n- last=$(cat \"$dotest/last\") &&\n- next=$(cat \"$dotest/next\") &&\n+ last=$(cat \"$dotest/last\" 2>/dev/null) &&\n+ next=$(cat \"$dotest/next\" 2>/dev/null) &&\n test $# != 0 &&\n test \"$next\" -gt \"$last\"\n then\n-- \n1.8.2.1.578.ga933817\n"},{"id":"215224","messageId":"1366725724-1016-3-git-send-email-artagnon@gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/7] rebase -i: don't error out if $state_dir already exists","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T14:01:59Z","receivedAt":"2013-04-23T14:01:59Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Practically speaking, the only reason why a `mkdir $state_dir` would\nfail is because $state_dir already exists. There is no problem in\nthis case, and we can proceed as usual. So, change the `mkdir` call\nto `mkdir -p`, and strip the `|| die`.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n git-rebase--interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 048a140..cc3a9a7 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -837,7 +837,7 @@ then\n fi\n \n orig_head=$(git rev-parse --verify HEAD) || die \"No HEAD?\"\n-mkdir \"$state_dir\" || die \"Could not create temporary $state_dir\"\n+mkdir -p \"$state_dir\"\n \n : > \"$state_dir\"/interactive || die \"Could not mark as interactive\"\n write_basic_state\n-- \n1.8.2.1.578.ga933817\n"},{"id":"215220","messageId":"1366725724-1016-4-git-send-email-artagnon@gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/7] am: tighten a conditional that checks for $dotest","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T14:02:00Z","receivedAt":"2013-04-23T14:02:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"We currently assume that, if a $dotest directory exists, an am had\nbeen called earlier. This assumption might get our conditional to\nmatch a stray $dotest directory created somewhere else, and result in\nfailures down the line. So, tighten the conditional by additionally\nlooking for the file $dotest/last.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n git-am.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 88aa438..f4ef8fc 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -454,7 +454,7 @@ then\n rm -fr \"$dotest\"\n fi\n \n-if test -d \"$dotest\"\n+if test -d \"$dotest\" && test -f \"$dotest/last\"\n then\n \tcase \"$#,$skip$resolved$abort\" in\n \t0,*t*)\n-- \n1.8.2.1.578.ga933817\n"},{"id":"215222","messageId":"1366725724-1016-5-git-send-email-artagnon@gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/7] am: don't do housekeeping when rebasing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T14:02:01Z","receivedAt":"2013-04-23T14:02:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Perform these two tasks:\n\n git gc --auto\n rm -fr \"$dotest\"\n\nonly when not called with --rebasing (from git-rebase--am.sh).\nOtherwise, return control to the caller so that it can do the needful.\nThe advantage of doing this is that the caller can implement a generic\ncleanup routine (that possibly does other things) independent of\nspecific rebases.\n\nPatch the ultimate caller, git-rebase.sh, to perform these two steps\nfor the moment. Later patches will add functionality.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n git-am.sh | 9 +++++++--\n git-rebase--am.sh | 8 ++++----\n git-rebase.sh | 7 +++++++\n 3 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex f4ef8fc..47c1021 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -904,5 +904,10 @@ if test -s \"$dotest\"/rewritten; then\n fi\n fi\n \n-rm -fr \"$dotest\"\n-git gc --auto\n+# If am was called with --rebasing (from git-rebase--am), it's up to\n+# the caller to take care of housekeeping.\n+if ! test -f \"$dotest/rebasing\"\n+then\n+\trm -fr \"$dotest\"\n+\tgit gc --auto\n+fi\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex f84854f..8230094 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -7,12 +7,12 @@ case \"$action\" in\n continue)\n \tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n \tmove_to_original_branch\n-\texit\n+\treturn $?\n \t;;\n skip)\n \tgit am --skip --resolvemsg=\"$resolvemsg\" &&\n \tmove_to_original_branch\n-\texit\n+\treturn $?\n \t;;\n esac\n \n@@ -56,7 +56,7 @@ else\n \n \t\tAs a result, git cannot rebase them.\n \t\tEOF\n-\t\texit $?\n+\t\treturn $?\n \tfi\n \n \tgit am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\" <\"$GIT_DIR/rebased-patches\"\n@@ -68,7 +68,7 @@ fi\n if test 0 != $ret\n then\n \ttest -d \"$state_dir\" && write_basic_state\n-\texit $ret\n+\treturn $ret\n fi\n \n move_to_original_branch\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex b2f1c76..8412d81 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -150,6 +150,13 @@ run_specific_rebase () {\n \t\tautosquash=\n \tfi\n \t. git-rebase--$type\n+\tret=$?\n+\tif test $ret = 0\n+\tthen\n+\t\tgit gc --auto &&\n+\t\trm -rf \"$state_dir\"\n+\tfi\n+\texit $ret\n }\n \n run_pre_rebase_hook () {\n-- \n1.8.2.1.578.ga933817\n"},{"id":"215223","messageId":"1366725724-1016-6-git-send-email-artagnon@gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/7] rebase -i: return control to the caller, for housekeeping","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T14:02:02Z","receivedAt":"2013-04-23T14:02:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"On a successful interactive rebase, git-rebase--interactive.sh\ncurrently cleans up and exits on its own. Instead of doing these\ntwo things ourselves:\n\n rm -fr \"$dotest\"\n git gc --auto\n\nlet us return control to the caller (git-rebase.sh), to do the\nneedful. The advantage of doing this is that the caller can implement\na generic cleanup routine (and possibly other things) independent of\nspecific rebases.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n git-rebase--interactive.sh | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex cc3a9a7..9514e31 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -597,7 +597,7 @@ do_next () {\n \t\tfi\n \t\t;;\n \tesac\n-\ttest -s \"$todo\" && return\n+\ttest -s \"$todo\" && return 1\n \n \tcomment_for_reflog finish &&\n \tnewhead=$(git rev-parse HEAD) &&\n@@ -623,17 +623,15 @@ do_next () {\n \t\t\"$GIT_DIR\"/hooks/post-rewrite rebase < \"$rewritten_list\"\n \t\ttrue # we don't care if this hook failed\n \tfi &&\n-\trm -rf \"$state_dir\" &&\n-\tgit gc --auto &&\n \twarn \"Successfully rebased and updated $head_name.\"\n \n-\texit\n+\treturn 0\n }\n \n do_rest () {\n \twhile :\n \tdo\n-\t\tdo_next\n+\t\tdo_next && break\n \tdone\n }\n \n@@ -799,12 +797,12 @@ first and then run 'git rebase --continue' again.\"\n \trecord_in_rewritten \"$(cat \"$state_dir\"/stopped-sha)\"\n \n \trequire_clean_work_tree \"rebase\"\n-\tdo_rest\n+\tdo_rest && return 0\n \t;;\n skip)\n \tgit rerere clear\n \n-\tdo_rest\n+\tdo_rest && return 0\n \t;;\n edit-todo)\n \tgit stripspace --strip-comments <\"$todo\" >\"$todo\".new\n-- \n1.8.2.1.578.ga933817\n"},{"id":"215225","messageId":"1366725724-1016-7-git-send-email-artagnon@gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 6/7] sh-setup: introduce require_clean_work_tree --quiet","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T14:02:03Z","receivedAt":"2013-04-23T14:02:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Some callers might want to know whether or not the worktree is clean,\nand require_clean_work_tree() has the logic for this. The current\nimplementation of the function prints a message and exits if the\nworktree wasn't clean. Introduce a --quiet switch to get it to report\nthe status of the worktree back to the caller.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n git-sh-setup.sh | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 2f78359..5fa22a8 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -172,6 +172,7 @@ require_clean_work_tree () {\n \n \tif ! git diff-files --quiet --ignore-submodules\n \tthen\n+\t\ttest \"$1\" = \"--quiet\" ||\n \t\techo >&2 \"Cannot $1: You have unstaged changes.\"\n \t\terr=1\n \tfi\n@@ -180,9 +181,11 @@ require_clean_work_tree () {\n \tthen\n \t\tif [ $err = 0 ]\n \t\tthen\n-\t\t echo >&2 \"Cannot $1: Your index contains uncommitted changes.\"\n+\t\t\ttest \"$1\" = \"--quiet\" ||\n+\t\t\techo >&2 \"Cannot $1: Your index contains uncommitted changes.\"\n \t\telse\n-\t\t echo >&2 \"Additionally, your index contains uncommitted changes.\"\n+\t\t\ttest \"$1\" = \"--quiet\" ||\n+\t\t\techo >&2 \"Additionally, your index contains uncommitted changes.\"\n \t\tfi\n \t\terr=1\n \tfi\n@@ -190,8 +193,9 @@ require_clean_work_tree () {\n \tif [ $err = 1 ]\n \tthen\n \t\ttest -n \"$2\" && echo >&2 \"$2\"\n-\t\texit 1\n+\t\ttest \"$1\" = \"--quiet\" || exit 1\n \tfi\n+\treturn $err\n }\n \n # Generate a sed script to parse identities from a commit.\n-- \n1.8.2.1.578.ga933817\n"},{"id":"215226","messageId":"1366725724-1016-8-git-send-email-artagnon@gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 7/7] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T14:02:04Z","receivedAt":"2013-04-23T14:02:04Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"This new feature allows a rebase to be executed on a dirty worktree.\nIt works by creating a temporary stash and storing it in\n$state_dir/autostash before the operation, and applying it after a\nsuccessful operation. It will be removed along with the $state_dir if\nthe operation is aborted.\n\nThe feature creates a special stash that does not affect the normal\nstash's reflogs, and will therefore be invisible to the end user.\nThis special stash is essentially a dangling merge commit which has\nreasonable lifetime specified by gc.pruneexpire (default 2 weeks).\n\nMost significantly, this feature means that a caller like pull (with\npull.rebase set to true) can easily be patched to remove the\nrequire_clean_work_tree restriction.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n Documentation/config.txt | 8 ++++++++\n Documentation/git-rebase.txt | 10 ++++++++++\n git-rebase.sh | 38 +++++++++++++++++++++++++++++++++++---\n 3 files changed, 53 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c67038b..03ad701 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1867,6 +1867,14 @@ rebase.stat::\n rebase.autosquash::\n \tIf set to true enable '--autosquash' option by default.\n \n+rebase.autostash::\n+\tWhen set to true, automatically create a temporary stash\n+\tbefore the operation begins, and apply it after the operation\n+\tends. This means that you can run rebase on a dirty worktree.\n+\tHowever, use with care: the final stash application after a\n+\tsuccessful rebase might result in non-trivial conflicts.\n+\tDefaults to false.\n+\n receive.autogc::\n \tBy default, git-receive-pack will run \"git-gc --auto\" after\n \treceiving data from git-push and updating refs. You can stop\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex aca8405..c84854a 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -208,6 +208,9 @@ rebase.stat::\n rebase.autosquash::\n \tIf set to true enable '--autosquash' option by default.\n \n+rebase.autostash::\n+\tIf set to true enable '--autostash' option by default.\n+\n OPTIONS\n -------\n --onto ::\n@@ -394,6 +397,13 @@ If the '--autosquash' option is enabled by default using the\n configuration variable `rebase.autosquash`, this option can be\n used to override and disable this setting.\n \n+--[no-]autostash::\n+\tAutomatically create a temporary stash before the operation\n+\tbegins, and apply it after the operation ends. This means\n+\tthat you can run rebase on a dirty worktree. However, use\n+\twith care: the final stash application after a successful\n+\trebase might result in non-trivial conflicts.\n+\n --no-ff::\n \tWith --interactive, cherry-pick all rebased commits instead of\n \tfast-forwarding over the unchanged ones. This ensures that the\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 8412d81..c8fddfe 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -13,6 +13,7 @@ git-rebase --continue | --abort | --skip | --edit-todo\n Available options are\n v,verbose! display a diffstat of what changed upstream\n q,quiet! be quiet. implies --no-stat\n+autostash! automatically stash/stash pop before and after\n onto=! rebase onto given branch instead of upstream\n p,preserve-merges! try to recreate merges instead of ignoring them\n s,strategy=! use the given merge strategy\n@@ -64,6 +65,7 @@ apply_dir=\"$GIT_DIR\"/rebase-apply\n verbose=\n diffstat=\n test \"$(git config --bool rebase.stat)\" = true && diffstat=t\n+autostash=\"$(git config --bool rebase.autostash || echo false)\"\n git_am_opt=\n rebase_root=\n force_rebase=\n@@ -143,6 +145,24 @@ move_to_original_branch () {\n \tesac\n }\n \n+apply_autostash () {\n+\tif test -f \"$state_dir/autostash\"\n+\tthen\n+\t\tstash_sha1=$(cat \"$state_dir/autostash\")\n+\t\tgit stash apply $stash_sha1 2>&1 >/dev/null ||\n+\t\tdie \"\n+$(eval_gettext 'Applying autostash resulted in conflicts.\n+Either fix the conflicts now, or run\n+\tgit reset --hard\n+and apply the stash on your desired branch:\n+\tgit stash apply $stash_sha1\n+at any time.')\" &&\n+\t\techo \"Applied autostash\"\n+\tfi\n+\tgit gc --auto &&\n+\trm -rf \"$state_dir\"\n+}\n+\n run_specific_rebase () {\n \tif [ \"$interactive_rebase\" = implied ]; then\n \t\tGIT_EDITOR=:\n@@ -153,8 +173,7 @@ run_specific_rebase () {\n \tret=$?\n \tif test $ret = 0\n \tthen\n-\t\tgit gc --auto &&\n-\t\trm -rf \"$state_dir\"\n+\t\tapply_autostash\n \tfi\n \texit $ret\n }\n@@ -248,6 +267,9 @@ do\n \t--stat)\n \t\tdiffstat=t\n \t\t;;\n+\t--autostash)\n+\t\tautostash=true\n+\t\t;;\n \t-v)\n \t\tverbose=t\n \t\tdiffstat=t\n@@ -348,7 +370,7 @@ abort)\n \t\t;;\n \tesac\n \toutput git reset --hard $orig_head\n-\trm -r \"$state_dir\"\n+\tapply_autostash\n \texit\n \t;;\n edit-todo)\n@@ -487,6 +509,16 @@ case \"$#\" in\n \t;;\n esac\n \n+if test \"$autostash\" = true && ! require_clean_work_tree --quiet\n+then\n+\tstash_sha1=$(git stash create) || die \"$(gettext \"Cannot autostash\")\" &&\n+\tmkdir -p \"$state_dir\" &&\n+\techo $stash_sha1 >\"$state_dir/autostash\" &&\n+\tstash_abbrev=$(git rev-parse --short $stash_sha1) &&\n+\techo \"$(gettext \"Created autostash: $stash_abbrev\")\" &&\n+\tgit reset --hard\n+fi\n+\n require_clean_work_tree \"rebase\" \"$(gettext \"Please commit or stash them.\")\"\n \n # Now we are rebasing commits $upstream..$orig_head (or with --root,\n-- \n1.8.2.1.578.ga933817\n"},{"id":"215228","messageId":"CANiSa6i+jsZUBgeQcQq2C=d009dU=i6+g35-HoofedaEJRmzTg@mail.gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/7] am: suppress error output from a conditional","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2013-04-23T14:38:37Z","receivedAt":"2013-04-23T14:38:37Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra\n wrote:\n> When testing if the $dotest directory exists, and if $next is greater\n> than $last\n\nWhen can that happen? If one edits the todo?\n"},{"id":"215229","messageId":"CANiSa6gMA8ZHNjaGu+hG4HxUCa5g+s00E=kGMyqeSv5NNRCCeQ@mail.gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/7] rebase -i: don't error out if $state_dir already exists","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2013-04-23T14:45:02Z","receivedAt":"2013-04-23T14:45:02Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra\n wrote:\n> Practically speaking, the only reason why a `mkdir $state_dir` would\n> fail is because $state_dir already exists.\n\nWould we ever get to this point in the code if it already exists?\n\nAlso, I had the feeling that the check it might fail if the user does\nnot have permissions to create the directory. Is there always\nsomething else that will fail first?\n"},{"id":"215246","messageId":"7vhaixjjlo.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"CANiSa6i+jsZUBgeQcQq2C=d009dU=i6+g35-HoofedaEJRmzTg@mail.gmail.com","subject":"Re: [PATCH 1/7] am: suppress error output from a conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T16:29:55Z","receivedAt":"2013-04-23T16:29:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk writes:\n\n> On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra\n> wrote:\n>> When testing if the $dotest directory exists, and if $next is greater\n>> than $last\n>\n> When can that happen? If one edits the todo?\n\nMore importantly, that condition is an unexpected error, which the\nuser may need to supply to help diagnosing the issue better, isn't\nit?\n"},{"id":"215248","messageId":"CALkWK0mTGFiuYhsJMf0x9YrO2ke21YmhzhiQQmqLqG61=pVFdw@mail.gmail.com","threadId":"33581","inReplyTo":"CANiSa6i+jsZUBgeQcQq2C=d009dU=i6+g35-HoofedaEJRmzTg@mail.gmail.com","subject":"Re: [PATCH 1/7] am: suppress error output from a conditional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T16:31:25Z","receivedAt":"2013-04-23T16:31:25Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Martin von Zweigbergk wrote:\n> On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra\n> wrote:\n>> When testing if the $dotest directory exists, and if $next is greater\n>> than $last\n>\n> When can that happen? If one edits the todo?\n\nWhen git-rebase.sh creates a $dotest directory with just an autostash file :)\n\nLike I said in my cover letter, I really wrote [7/7] first, and kept\nmodifying things until all the tests passed. So, I can't really\njustify these changes independent of [7/7].\n"},{"id":"215249","messageId":"7vd2tljjcx.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"1366725724-1016-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/7] rebase -i: don't error out if $state_dir already exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T16:35:10Z","receivedAt":"2013-04-23T16:35:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Practically speaking, the only reason why a `mkdir $state_dir` would\n> fail is because $state_dir already exists. There is no problem in\n> this case, and we can proceed as usual.\n\nThat was started as a cheap way to detect and avoid running the same\nrebase -i while another one is already in progress, way back when we\ndid not have a separate \"git-rebase--*backend*\" scripts, I think.\n\nIf we have a separate safety measure to prevent it, lifting it may\nbe safe, but in a sane case, $state_dir should _not_ exist when we\nstart the command, and mkdir should _not_ fail to create it.\n\nSo I do not see why this change could be an improvement. I would\nunderstand it if some future changes may have already created the\nstate dir from a separate codepath (which may be an indication of a\npoor design of integrating that separate codepath, but that is a\ndifferent matter) and mkdir needs to be turned into \"mkdir -p\" here,\nbut I do not think removal of \"|| die\" is warranted even in such a\ncase.\n\n> So, change the `mkdir` call\n> to `mkdir -p`, and strip the `|| die`.\n>\n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> git-rebase--interactive.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 048a140..cc3a9a7 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -837,7 +837,7 @@ then\n> fi\n> \n> orig_head=$(git rev-parse --verify HEAD) || die \"No HEAD?\"\n> -mkdir \"$state_dir\" || die \"Could not create temporary $state_dir\"\n> +mkdir -p \"$state_dir\"\n> \n> : > \"$state_dir\"/interactive || die \"Could not mark as interactive\"\n> write_basic_state\n"},{"id":"215251","messageId":"CALkWK0mDMV7Bh_1BfsOFwX37KRUhNw-96xcsmvk0pRqUinppeQ@mail.gmail.com","threadId":"33581","inReplyTo":"7vd2tljjcx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/7] rebase -i: don't error out if $state_dir already exists","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T16:38:01Z","receivedAt":"2013-04-23T16:38:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> So I do not see why this change could be an improvement. I would\n> understand it if some future changes may have already created the\n> state dir from a separate codepath (which may be an indication of a\n> poor design of integrating that separate codepath, but that is a\n> different matter) and mkdir needs to be turned into \"mkdir -p\" here,\n> but I do not think removal of \"|| die\" is warranted even in such a\n> case.\n\nYeah, it's [7/7] creating a $state_dir/autostash before anything else\ncan happen. Okay, if I put the \"|| die\" back in, does it require any\nmore justification?\n"},{"id":"215252","messageId":"7v8v49jivl.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"1366725724-1016-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/7] am: tighten a conditional that checks for $dotest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T16:45:34Z","receivedAt":"2013-04-23T16:45:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> We currently assume that, if a $dotest directory exists, an am had\n> been called earlier. This assumption might get our conditional to\n> match a stray $dotest directory created somewhere else,...\n\nHmm. that explanation sounds like that is sweeping a real issue\nunder the rug. Shouldn't your patch fixing that creator of the\nstray one? That stray one that errorneously creates $dotest, even\nwhen it is not applying patch messages, may create the directory\nwith 'last' but without 'next' or the messages, so checking only\n'last' does not sound like solving any problem.\n\nIf this were done as a part of an addition that has to create\n$dotest even when it does not (yet) deposit patches in there, the\nway that particular addition creates and uses $dotest may justify\nwhy testing only 'last' is sufficient. But as a standalone change,\nneither the patch text or the above explanation makes much sense to\nme.\n\n> diff --git a/git-am.sh b/git-am.sh\n> index 88aa438..f4ef8fc 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -454,7 +454,7 @@ then\n> rm -fr \"$dotest\"\n> fi\n> \n> -if test -d \"$dotest\"\n> +if test -d \"$dotest\" && test -f \"$dotest/last\"\n\n> then\n> \tcase \"$#,$skip$resolved$abort\" in\n> \t0,*t*)\n"},{"id":"215253","messageId":"7v4nexjim0.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"1366725724-1016-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/7] am: don't do housekeeping when rebasing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T16:51:19Z","receivedAt":"2013-04-23T16:51:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Perform these two tasks:\n>\n> git gc --auto\n> rm -fr \"$dotest\"\n>\n> only when not called with --rebasing (from git-rebase--am.sh).\n> Otherwise, return control to the caller so that it can do the needful.\n> The advantage of doing this is that the caller can implement a generic\n> cleanup routine (that possibly does other things) independent of\n> specific rebases.\n\nThe above justification and the patch text make a lot of sense, but\nthey make me wonder why there is no corresponding removal from the\nother git-rebase--*backend*.\n\nUnconditional changes from \"exit\" to \"return $?\" looked somewhat\nsuspicious, but this is a sourced script so it should be safe.\n\n> git-am.sh | 9 +++++++--\n> git-rebase--am.sh | 8 ++++----\n> git-rebase.sh | 7 +++++++\n> 3 files changed, 18 insertions(+), 6 deletions(-)\n>\n> diff --git a/git-am.sh b/git-am.sh\n> index f4ef8fc..47c1021 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -904,5 +904,10 @@ if test -s \"$dotest\"/rewritten; then\n> fi\n> fi\n> \n> -rm -fr \"$dotest\"\n> -git gc --auto\n> +# If am was called with --rebasing (from git-rebase--am), it's up to\n> +# the caller to take care of housekeeping.\n> +if ! test -f \"$dotest/rebasing\"\n> +then\n> +\trm -fr \"$dotest\"\n> +\tgit gc --auto\n> +fi\n> diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n> index f84854f..8230094 100644\n> --- a/git-rebase--am.sh\n> +++ b/git-rebase--am.sh\n> @@ -7,12 +7,12 @@ case \"$action\" in\n> continue)\n> \tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n> \tmove_to_original_branch\n> -\texit\n> +\treturn $?\n> \t;;\n> skip)\n> \tgit am --skip --resolvemsg=\"$resolvemsg\" &&\n> \tmove_to_original_branch\n> -\texit\n> +\treturn $?\n> \t;;\n> esac\n> \n> @@ -56,7 +56,7 @@ else\n> \n> \t\tAs a result, git cannot rebase them.\n> \t\tEOF\n> -\t\texit $?\n> +\t\treturn $?\n> \tfi\n> \n> \tgit am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\" <\"$GIT_DIR/rebased-patches\"\n> @@ -68,7 +68,7 @@ fi\n> if test 0 != $ret\n> then\n> \ttest -d \"$state_dir\" && write_basic_state\n> -\texit $ret\n> +\treturn $ret\n> fi\n> \n> move_to_original_branch\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index b2f1c76..8412d81 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -150,6 +150,13 @@ run_specific_rebase () {\n> \t\tautosquash=\n> \tfi\n> \t. git-rebase--$type\n> +\tret=$?\n> +\tif test $ret = 0\n> +\tthen\n> +\t\tgit gc --auto &&\n> +\t\trm -rf \"$state_dir\"\n> +\tfi\n> +\texit $ret\n> }\n> \n> run_pre_rebase_hook () {\n"},{"id":"215254","messageId":"CANiSa6hmh1d7ECBCAm+AKFV4KXh7eoHfk7pL0RjaqZj4XbKhpw@mail.gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 5/7] rebase -i: return control to the caller, for housekeeping","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2013-04-23T16:51:28Z","receivedAt":"2013-04-23T16:51:28Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Tue, Apr 23, 2013 at 7:02 AM, Ramkumar Ramachandra\n wrote:\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index cc3a9a7..9514e31 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -597,7 +597,7 @@ do_next () {\n> fi\n> ;;\n> esac\n> - test -s \"$todo\" && return\n> + test -s \"$todo\" && return 1\n\nUnlike the other cases below, this seems to be replacing success by\nfailure. What does it mean in practice that $todo is empty?\n\n> comment_for_reflog finish &&\n> newhead=$(git rev-parse HEAD) &&\n> @@ -623,17 +623,15 @@ do_next () {\n> \"$GIT_DIR\"/hooks/post-rewrite rebase < \"$rewritten_list\"\n> true # we don't care if this hook failed\n> fi &&\n> - rm -rf \"$state_dir\" &&\n> - git gc --auto &&\n> warn \"Successfully rebased and updated $head_name.\"\n>\n> - exit\n> + return 0\n\nSo after this patch, the \"warning\" will coming before gc is run. It's\na change, but it seems fine. gc usually only prints a few line, right?\n\n> }\n>\n> do_rest () {\n> while :\n> do\n> - do_next\n> + do_next && break\n> done\n> }\n\nNormally one would break if unsuccessful. What would fail if this was\nreplaced by \"do_next || break\" and the above \".. && return 1\" was \"..\n&& return\". I assume that was your first attempt, but why did it not\nwork?\n"},{"id":"215255","messageId":"7vzjwpi3rp.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"1366725724-1016-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 5/7] rebase -i: return control to the caller, for housekeeping","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T16:57:14Z","receivedAt":"2013-04-23T16:57:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> On a successful interactive rebase, git-rebase--interactive.sh\n> currently cleans up and exits on its own. Instead of doing these\n> two things ourselves:\n>\n> rm -fr \"$dotest\"\n> git gc --auto\n>\n> let us return control to the caller (git-rebase.sh), to do the\n> needful. The advantage of doing this is that the caller can implement\n> a generic cleanup routine (and possibly other things) independent of\n> specific rebases.\n>\n> Signed-off-by: Ramkumar Ramachandra \n> ---\n\nAnd this answers the question in my review for [4/7]. It would make\nsense to have these two patch subseries asn three patches (prepare\ngit-rebase.sh, and then convert each backends separately), or a\nsingle patch; two patches like this does not make much sense to me.\n\n> git-rebase--interactive.sh | 12 +++++-------\n> 1 file changed, 5 insertions(+), 7 deletions(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index cc3a9a7..9514e31 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -597,7 +597,7 @@ do_next () {\n> \t\tfi\n> \t\t;;\n> \tesac\n> -\ttest -s \"$todo\" && return\n> +\ttest -s \"$todo\" && return 1\n> \n> \tcomment_for_reflog finish &&\n> \tnewhead=$(git rev-parse HEAD) &&\n> @@ -623,17 +623,15 @@ do_next () {\n> \t\t\"$GIT_DIR\"/hooks/post-rewrite rebase < \"$rewritten_list\"\n> \t\ttrue # we don't care if this hook failed\n> \tfi &&\n> -\trm -rf \"$state_dir\" &&\n> -\tgit gc --auto &&\n> \twarn \"Successfully rebased and updated $head_name.\"\n> \n> -\texit\n> +\treturn 0\n> }\n> \n> do_rest () {\n> \twhile :\n> \tdo\n> -\t\tdo_next\n> +\t\tdo_next && break\n> \tdone\n> }\n\nThis is somewhat suspicious. We used to die when do_next failed, or\nlet do_next exit with success.\n\nBut now you let do_rest return (what does it return???)...\n\n> \n> @@ -799,12 +797,12 @@ first and then run 'git rebase --continue' again.\"\n> \trecord_in_rewritten \"$(cat \"$state_dir\"/stopped-sha)\"\n> \n> \trequire_clean_work_tree \"rebase\"\n> -\tdo_rest\n> +\tdo_rest && return 0\n\n... and its caller reports success here only when it succeeds. What\nhappens do_rest returns a failure?\n\n> \t;;\n> skip)\n> \tgit rerere clear\n> \n> -\tdo_rest\n> +\tdo_rest && return 0\n> \t;;\n> edit-todo)\n> \tgit stripspace --strip-comments <\"$todo\" >\"$todo\".new\n"},{"id":"215256","messageId":"7vvc7di3lp.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"1366725724-1016-7-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 6/7] sh-setup: introduce require_clean_work_tree --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T17:00:50Z","receivedAt":"2013-04-23T17:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Some callers might want to know whether or not the worktree is clean,\n> and require_clean_work_tree() has the logic for this. The current\n> implementation of the function prints a message and exits if the\n> worktree wasn't clean. Introduce a --quiet switch to get it to report\n> the status of the worktree back to the caller.\n\nThis makes 75% sense, but I find it an extremely bad taste to tie\nexit vs return with --quiet. They are orthogonal and unrelated\nconcepts.\n\nDoing this\n\n\tif (require_clean_work_tree) 2>/dev/null\n then\n\t\t: happy, the working tree is clean\n\telse\n\t\t... let's stash ...\n\tfi\n\nwithout changing anything else may be better than adding such a\nconflating --quiet option.\n\n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> git-sh-setup.sh | 10 +++++++---\n> 1 file changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index 2f78359..5fa22a8 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -172,6 +172,7 @@ require_clean_work_tree () {\n> \n> \tif ! git diff-files --quiet --ignore-submodules\n> \tthen\n> +\t\ttest \"$1\" = \"--quiet\" ||\n> \t\techo >&2 \"Cannot $1: You have unstaged changes.\"\n> \t\terr=1\n> \tfi\n> @@ -180,9 +181,11 @@ require_clean_work_tree () {\n> \tthen\n> \t\tif [ $err = 0 ]\n> \t\tthen\n> -\t\t echo >&2 \"Cannot $1: Your index contains uncommitted changes.\"\n> +\t\t\ttest \"$1\" = \"--quiet\" ||\n> +\t\t\techo >&2 \"Cannot $1: Your index contains uncommitted changes.\"\n> \t\telse\n> -\t\t echo >&2 \"Additionally, your index contains uncommitted changes.\"\n> +\t\t\ttest \"$1\" = \"--quiet\" ||\n> +\t\t\techo >&2 \"Additionally, your index contains uncommitted changes.\"\n> \t\tfi\n> \t\terr=1\n> \tfi\n> @@ -190,8 +193,9 @@ require_clean_work_tree () {\n> \tif [ $err = 1 ]\n> \tthen\n> \t\ttest -n \"$2\" && echo >&2 \"$2\"\n> -\t\texit 1\n> +\t\ttest \"$1\" = \"--quiet\" || exit 1\n> \tfi\n> +\treturn $err\n> }\n> \n> # Generate a sed script to parse identities from a commit.\n"},{"id":"215257","messageId":"7vr4i1i3a9.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"1366725724-1016-8-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 7/7] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T17:07:42Z","receivedAt":"2013-04-23T17:07:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> +apply_autostash () {\n> +\tif test -f \"$state_dir/autostash\"\n> +\tthen\n> +\t\tstash_sha1=$(cat \"$state_dir/autostash\")\n> +\t\tgit stash apply $stash_sha1 2>&1 >/dev/null ||\n> +\t\tdie \"\n> +$(eval_gettext 'Applying autostash resulted in conflicts.\n> +Either fix the conflicts now, or run\n> +\tgit reset --hard\n> +and apply the stash on your desired branch:\n> +\tgit stash apply $stash_sha1\n> +at any time.')\" &&\n> +\t\techo \"Applied autostash\"\n\nThat && looks funny. What does it even mean for die to succeed and\ngive control back to you for a chance to say \"Applied\"?\n\n\tstash apply || die\n\techo applied\n\nwould be far more logical.\n\n> +\tfi\n> +\tgit gc --auto &&\n> +\trm -rf \"$state_dir\"\n> +}\n\nThis is somewhat worrysome. After getting the \"oops, we cannot\napply\" message, the \"$stash_sha1\" in the message on the terminal is\nthe ONLY indication of where the (presumably precious) local change\nthe user used to have can be recovered from.\n\nYou do not \"rm -fr $state_dir\" in such a case, so perhaps telling\nthe user the location of that \"autostash\" file may help her\nsomewhat.\n\nFor that matter, wouldn't it be a lot simpler to put the autostash\nref somewhere in refs/ hierarchy, instead of storing an object name\nof the stash that keeps (presumably precious) local change of the\nuser in a plain-text file that is not at all known by \"gc\"?\n\n> +\n> run_specific_rebase () {\n> \tif [ \"$interactive_rebase\" = implied ]; then\n> \t\tGIT_EDITOR=:\n> @@ -153,8 +173,7 @@ run_specific_rebase () {\n> \tret=$?\n> \tif test $ret = 0\n> \tthen\n> -\t\tgit gc --auto &&\n> -\t\trm -rf \"$state_dir\"\n> +\t\tapply_autostash\n> \tfi\n> \texit $ret\n> }\n> @@ -248,6 +267,9 @@ do\n> \t--stat)\n> \t\tdiffstat=t\n> \t\t;;\n> +\t--autostash)\n> +\t\tautostash=true\n> +\t\t;;\n> \t-v)\n> \t\tverbose=t\n> \t\tdiffstat=t\n> @@ -348,7 +370,7 @@ abort)\n> \t\t;;\n> \tesac\n> \toutput git reset --hard $orig_head\n> -\trm -r \"$state_dir\"\n> +\tapply_autostash\n> \texit\n> \t;;\n> edit-todo)\n> @@ -487,6 +509,16 @@ case \"$#\" in\n> \t;;\n> esac\n> \n> +if test \"$autostash\" = true && ! require_clean_work_tree --quiet\n> +then\n> +\tstash_sha1=$(git stash create) || die \"$(gettext \"Cannot autostash\")\" &&\n> +\tmkdir -p \"$state_dir\" &&\n> +\techo $stash_sha1 >\"$state_dir/autostash\" &&\n> +\tstash_abbrev=$(git rev-parse --short $stash_sha1) &&\n> +\techo \"$(gettext \"Created autostash: $stash_abbrev\")\" &&\n> +\tgit reset --hard\n> +fi\n> +\n> require_clean_work_tree \"rebase\" \"$(gettext \"Please commit or stash them.\")\"\n> \n> # Now we are rebasing commits $upstream..$orig_head (or with --root,\n"},{"id":"215258","messageId":"7vmwspi2ag.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"CALkWK0mTGFiuYhsJMf0x9YrO2ke21YmhzhiQQmqLqG61=pVFdw@mail.gmail.com","subject":"Re: [PATCH 1/7] am: suppress error output from a conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T17:29:11Z","receivedAt":"2013-04-23T17:29:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Martin von Zweigbergk wrote:\n>> On Tue, Apr 23, 2013 at 7:01 AM, Ramkumar Ramachandra\n>> wrote:\n>>> When testing if the $dotest directory exists, and if $next is greater\n>>> than $last\n>>\n>> When can that happen? If one edits the todo?\n>\n> When git-rebase.sh creates a $dotest directory with just an autostash file :)\n>\n> Like I said in my cover letter, I really wrote [7/7] first, and kept\n> modifying things until all the tests passed. So, I can't really\n> justify these changes independent of [7/7].\n\nThat explains a lot of mystery ;-)\n\nPerhaps all of these \"oops, 7/7 breaks this and that so let's work\nthem around\" can be avoided if the series did not store the object\nname of the stash that records the local changes as a plain text\nfile inside a $dotest/ directory, and instead used a dedicated ref\nsomewhere under refs/ hierarchy, no?\n"},{"id":"215259","messageId":"CALkWK0kzVPyOyz8PFPyYnCFS+pBQD_ninuFKUiJ4fhkeiUdeNA@mail.gmail.com","threadId":"33581","inReplyTo":"7vmwspi2ag.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] am: suppress error output from a conditional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T17:31:51Z","receivedAt":"2013-04-23T17:31:51Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Perhaps all of these \"oops, 7/7 breaks this and that so let's work\n> them around\" can be avoided if the series did not store the object\n> name of the stash that records the local changes as a plain text\n> file inside a $dotest/ directory, and instead used a dedicated ref\n> somewhere under refs/ hierarchy, no?\n\nExactly. Which is why I was asking for named stashes on the other\nthread. Much cleaner than keeping track of it yourself.\n"},{"id":"215260","messageId":"7vip3di21r.fsf@alter.siamese.dyndns.org","threadId":"33581","inReplyTo":"7vr4i1i3a9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-23T17:34:24Z","receivedAt":"2013-04-23T17:34:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ramkumar Ramachandra writes:\n>\n>> +apply_autostash () {\n>> +\tif test -f \"$state_dir/autostash\"\n>> +\tthen\n>> +\t\tstash_sha1=$(cat \"$state_dir/autostash\")\n>> +\t\tgit stash apply $stash_sha1 2>&1 >/dev/null ||\n>> +\t\tdie \"\n>> +$(eval_gettext 'Applying autostash resulted in conflicts.\n>> +Either fix the conflicts now, or run\n>> +\tgit reset --hard\n>> +and apply the stash on your desired branch:\n>> +\tgit stash apply $stash_sha1\n>> +at any time.')\" &&\n>> +\t\techo \"Applied autostash\"\n>\n> That && looks funny. What does it even mean for die to succeed and\n> give control back to you for a chance to say \"Applied\"?\n>\n> \tstash apply || die\n> \techo applied\n>\n> would be far more logical.\n>\n>> +\tfi\n>> +\tgit gc --auto &&\n>> +\trm -rf \"$state_dir\"\n>> +}\n>\n> This is somewhat worrisome.\n\nOne more thing on this function. [4/7] (and [5/7]) justified nicely\nwhy it is a good idea to have a central place to do the clean-up\ntasks. apply_autostash is a poor name for that \"clean-up\" function.\nThe central clean-up may happen to involve applying a stash in this\nversion, but applying stash will not stay the entirety of the\nclean-up work forever. The entirety of the clean-up work used to be\njust 'git gc --auto && rm -fr \"$state_dir\"' for eternity, and this\nseries is adding something else. It is not hard to imagine somebody\nelse would want to add other kinds of clean-up tasks in here.\n\nPerhaps \"finish_rebase\" or something?\n"},{"id":"215261","messageId":"CALkWK0nbY6EcBmsOFPVtKoajiZnKacFKBVmpSYw9pJw3pQ4fgA@mail.gmail.com","threadId":"33581","inReplyTo":"7vip3di21r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-23T17:37:13Z","receivedAt":"2013-04-23T17:37:13Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Perhaps \"finish_rebase\" or something?\n\nSure. That makes sense.\n\nI was too busy struggling with shell code to notice these things ;)\n"},{"id":"215275","messageId":"CABURp0onypkDFiovYP4s0UuBV+oFp3rPv1Jq8dGbxZhKU_sgsQ@mail.gmail.com","threadId":"33581","inReplyTo":"1366725724-1016-8-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 7/7] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2013-04-23T22:45:52Z","receivedAt":"2013-04-23T22:45:52Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Tue, Apr 23, 2013 at 10:02 AM, Ramkumar Ramachandra\n wrote:\n> This new feature allows a rebase to be executed on a dirty worktree.\n> It works by creating a temporary stash and storing it in\n> $state_dir/autostash before the operation, and applying it after a\n> successful operation. It will be removed along with the $state_dir if\n> the operation is aborted.\n>\n> The feature creates a special stash that does not affect the normal\n> stash's reflogs, and will therefore be invisible to the end user.\n> This special stash is essentially a dangling merge commit which has\n> reasonable lifetime specified by gc.pruneexpire (default 2 weeks).\n>\n> Most significantly, this feature means that a caller like pull (with\n> pull.rebase set to true) can easily be patched to remove the\n> require_clean_work_tree restriction.\n>\n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> Documentation/config.txt | 8 ++++++++\n> Documentation/git-rebase.txt | 10 ++++++++++\n> git-rebase.sh | 38 +++++++++++++++++++++++++++++++++++---\n> 3 files changed, 53 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index c67038b..03ad701 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1867,6 +1867,14 @@ rebase.stat::\n> rebase.autosquash::\n> If set to true enable '--autosquash' option by default.\n>\n> +rebase.autostash::\n> + When set to true, automatically create a temporary stash\n> + before the operation begins, and apply it after the operation\n> + ends. This means that you can run rebase on a dirty worktree.\n> + However, use with care: the final stash application after a\n> + successful rebase might result in non-trivial conflicts.\n> + Defaults to false.\n> +\n> receive.autogc::\n> By default, git-receive-pack will run \"git-gc --auto\" after\n> receiving data from git-push and updating refs. You can stop\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index aca8405..c84854a 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -208,6 +208,9 @@ rebase.stat::\n> rebase.autosquash::\n> If set to true enable '--autosquash' option by default.\n>\n> +rebase.autostash::\n> + If set to true enable '--autostash' option by default.\n> +\n> OPTIONS\n> -------\n> --onto ::\n> @@ -394,6 +397,13 @@ If the '--autosquash' option is enabled by default using the\n> configuration variable `rebase.autosquash`, this option can be\n> used to override and disable this setting.\n>\n> +--[no-]autostash::\n> + Automatically create a temporary stash before the operation\n> + begins, and apply it after the operation ends. This means\n> + that you can run rebase on a dirty worktree. However, use\n> + with care: the final stash application after a successful\n> + rebase might result in non-trivial conflicts.\n> +\n> --no-ff::\n> With --interactive, cherry-pick all rebased commits instead of\n> fast-forwarding over the unchanged ones. This ensures that the\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index 8412d81..c8fddfe 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -13,6 +13,7 @@ git-rebase --continue | --abort | --skip | --edit-todo\n> Available options are\n> v,verbose! display a diffstat of what changed upstream\n> q,quiet! be quiet. implies --no-stat\n> +autostash! automatically stash/stash pop before and after\n> onto=! rebase onto given branch instead of upstream\n> p,preserve-merges! try to recreate merges instead of ignoring them\n> s,strategy=! use the given merge strategy\n> @@ -64,6 +65,7 @@ apply_dir=\"$GIT_DIR\"/rebase-apply\n> verbose=\n> diffstat=\n> test \"$(git config --bool rebase.stat)\" = true && diffstat=t\n> +autostash=\"$(git config --bool rebase.autostash || echo false)\"\n> git_am_opt=\n> rebase_root=\n> force_rebase=\n> @@ -143,6 +145,24 @@ move_to_original_branch () {\n> esac\n> }\n>\n> +apply_autostash () {\n> + if test -f \"$state_dir/autostash\"\n> + then\n> + stash_sha1=$(cat \"$state_dir/autostash\")\n> + git stash apply $stash_sha1 2>&1 >/dev/null ||\n> + die \"\n> +$(eval_gettext 'Applying autostash resulted in conflicts.\n> +Either fix the conflicts now, or run\n> + git reset --hard\n> +and apply the stash on your desired branch:\n> + git stash apply $stash_sha1\n> +at any time.')\" &&\n> + echo \"Applied autostash\"\n> + fi\n> + git gc --auto &&\n> + rm -rf \"$state_dir\"\n> +}\n\nBecause I am in a git-rebase which has apparently failed, I would\nexpect 'git rebase --abort' would save me here. But it does not and\nyou have given me some unique instructions to try to recover. I\nsuppose rebase--abort cannot be made to recover in this case because\nthis is a rebase-wrapper and all of my rebase-state is already\ndiscarded. But I would much prefer to have the normal \"undo\"-ability\nof git-rebase here, once I realize I have made a mistake or\nencountered conflicts I am not prepared to handle right now.\n\n\n> +\n> run_specific_rebase () {\n> if [ \"$interactive_rebase\" = implied ]; then\n> GIT_EDITOR=:\n> @@ -153,8 +173,7 @@ run_specific_rebase () {\n> ret=$?\n> if test $ret = 0\n> then\n> - git gc --auto &&\n> - rm -rf \"$state_dir\"\n> + apply_autostash\n> fi\n> exit $ret\n> }\n> @@ -248,6 +267,9 @@ do\n> --stat)\n> diffstat=t\n> ;;\n> + --autostash)\n> + autostash=true\n> + ;;\n> -v)\n> verbose=t\n> diffstat=t\n> @@ -348,7 +370,7 @@ abort)\n> ;;\n> esac\n> output git reset --hard $orig_head\n> - rm -r \"$state_dir\"\n> + apply_autostash\n> exit\n> ;;\n> edit-todo)\n> @@ -487,6 +509,16 @@ case \"$#\" in\n> ;;\n> esac\n>\n> +if test \"$autostash\" = true && ! require_clean_work_tree --quiet\n> +then\n> + stash_sha1=$(git stash create) || die \"$(gettext \"Cannot autostash\")\" &&\n> + mkdir -p \"$state_dir\" &&\n> + echo $stash_sha1 >\"$state_dir/autostash\" &&\n> + stash_abbrev=$(git rev-parse --short $stash_sha1) &&\n> + echo \"$(gettext \"Created autostash: $stash_abbrev\")\" &&\n> + git reset --hard\n> +fi\n> +\n> require_clean_work_tree \"rebase\" \"$(gettext \"Please commit or stash them.\")\"\n>\n> # Now we are rebasing commits $upstream..$orig_head (or with --root,\n> --\n> 1.8.2.1.578.ga933817\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"215299","messageId":"CALkWK0=JsR9txrrnWZKyrTpv-s+ZgVz-cL6ZsMn2R+wz6rE4FA@mail.gmail.com","threadId":"33581","inReplyTo":"CABURp0onypkDFiovYP4s0UuBV+oFp3rPv1Jq8dGbxZhKU_sgsQ@mail.gmail.com","subject":"Re: [PATCH 7/7] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-24T08:27:44Z","receivedAt":"2013-04-24T08:27:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Phil Hord wrote:\n> Because I am in a git-rebase which has apparently failed, I would\n> expect 'git rebase --abort' would save me here. But it does not and\n> you have given me some unique instructions to try to recover. I\n> suppose rebase--abort cannot be made to recover in this case because\n> this is a rebase-wrapper and all of my rebase-state is already\n> discarded. But I would much prefer to have the normal \"undo\"-ability\n> of git-rebase here, once I realize I have made a mistake or\n> encountered conflicts I am not prepared to handle right now.\n\nYou're asking for a hammer solution, when I'm advocating a solution\nthat offers more flexibility and control. Commits and worktree\nchanges are fundamentally two different things, and I treat them\ndifferently.\n\nrebase.autostash is simply a shortcut for:\n\n $ git stash && git rebase ... && git stash pop\n\nExcept that your stash is not blocked during the rebase process: we\nuse a special stash. If the last 'git stash pop' fails, do you do\nthis?\n\n $ git reset --hard HEAD@{1}\n $ git stash pop\n # snip, snip ...\n # redo the entire rebase\n\nI _never_ find myself doing this; in your hammer solution, you're\nadvocating that we always do this.\n\nThe stash is a powerful tool when used properly: a stash isn't\nattached to any branch, and therefore perfectly designed to keep small\ntemporary worktree changes for a short period of time.\nrebase.autostash is _not_ a way to take away power from the user, or\nsave the user from learning how to use stash.\n\nThat said, the current implementation is very rough and I will improve\nit in the next iterations. If the apply fails, I will push the\nchanges onto stash@{0}, and let the user do a 'git stash pop' instead\nof having to remember (or copy out) a SHA-1 displayed in the terminal\noutput. Essentially, this leaves the user in the exact same state as\nif she had done a 'git stash && git rebase ... && git stash pop'.\n"},{"id":"215341","messageId":"vpq8v47q0ji.fsf@grenoble-inp.fr","threadId":"33581","inReplyTo":"CABURp0onypkDFiovYP4s0UuBV+oFp3rPv1Jq8dGbxZhKU_sgsQ@mail.gmail.com","subject":"Re: [PATCH 7/7] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-24T17:52:01Z","receivedAt":"2013-04-24T17:52:01Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Phil Hord writes:\n\n> Because I am in a git-rebase which has apparently failed, I would\n> expect 'git rebase --abort' would save me here. \n\nMore generally, if I \"git rebase --abort\" in the middle of a rebase (not\nnecessarily at the end), I'd expect the stash to be restored. Right now,\nif I read correctly, \"git rebase --abort\" would discard the stashed\nchanges without giving me the sha1 I'd need to use to recover it :-(.\n\nRecovering the stash should be doable with stg like\n\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -341,6 +341,11 @@ abort)\n ;;\n esac\n output git reset --hard $orig_head\n+ if test -f \"$state_dir/autostash\"\n+ then\n+ stash_sha1=$(cat \"$state_dir/autostash\")\n+ git stash apply $stash_sha1\n+ fi\n rm -r \"$state_dir\"\n exit\n ;;\n\nthe \"stash apply\" should succeed without conflict by construction\nbecause the stash is applied on the commit it was created.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"215444","messageId":"CALkWK0n6gDTBk0G+gm6UHgMb_ueLGMZQyYVP_D24G8uXCyjXvw@mail.gmail.com","threadId":"33581","inReplyTo":"CANiSa6hmh1d7ECBCAm+AKFV4KXh7eoHfk7pL0RjaqZj4XbKhpw@mail.gmail.com","subject":"Re: [PATCH 5/7] rebase -i: return control to the caller, for housekeeping","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-25T14:30:44Z","receivedAt":"2013-04-25T14:30:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Martin von Zweigbergk wrote:\n> Normally one would break if unsuccessful. What would fail if this was\n> replaced by \"do_next || break\" and the above \".. && return 1\" was \"..\n> && return\". I assume that was your first attempt, but why did it not\n> work?\n\nThanks. This was a major thinko on my part, but I don't remember\nexactly why I got confused. I'll explain this patch properly in the\nnext iteration.\n"}]}