{"thread":{"id":"33771","subject":"[PATCH v2 0/8] rebase.autostash completed","startedAt":"2013-05-10T14:26:37Z","lastAt":"2013-05-12T03:09:48Z","messageCount":18,"participants":["Ramkumar Ramachandra","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":8},"messages":[{"id":"216872","messageId":"1368196005-5354-1-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":null,"subject":"[PATCH v2 0/8] rebase.autostash completed","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:37Z","receivedAt":"2013-05-10T14:26:37Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThe feature is finished with documentation and tests in this\niteration.  I've written an extensive t3420 which proves that the\nfeature works flawlessly.  Further, I've made every attempt to\nactually explain what I'm doing: I've taken care to inspect all the\nreturn values.\n\nOverall, I'm elated with the design and interface.  I think it is most\nintuitive, while not trading off power/ flexibility.\n\nOne subtle detail that you might disagree with: I report success if\nthe rebase succeeds but the stash application fails.  Are we okay with\nthis?\n\nAlso, does t3420 exercise all the cases sufficiently?  Have I missed\nanything?\n\nEnjoy reading and reviewing this.\n\nRamkumar Ramachandra (8):\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  rebase: prepare to do generic housekeeping\n  am: return control to caller, for housekeeping\n  rebase -i: return control to caller, for housekeeping\n  rebase --merge: return control to caller, for housekeeping\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   |  11 ++--\n git-rebase--merge.sh         |   5 +-\n git-rebase.sh                |  46 +++++++++++++-\n t/t3420-rebase-autostash.sh  | 148 +++++++++++++++++++++++++++++++++++++++++++\n 8 files changed, 233 insertions(+), 18 deletions(-)\n create mode 100755 t/t3420-rebase-autostash.sh\n\n-- \n1.8.3.rc1.52.gc14258d\n"},{"id":"216874","messageId":"1368196005-5354-2-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/8] am: suppress error output from a conditional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:38Z","receivedAt":"2013-05-10T14:26:38Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"In preparation for a later patch that creates $dotest/autostash in\ngit-rebase.sh before anything else happens, don't assume that the\npresence of a $dotest directory implies the existence of the $next and\n$last files.  The check for the files is in a conditional anyway, but\n`cat` is executed on potentially non-existent files.  Suppress this\nerror output.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\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.3.rc1.52.gc14258d\n"},{"id":"216880","messageId":"1368196005-5354-3-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/8] rebase -i: don't error out if $state_dir already exists","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:39Z","receivedAt":"2013-05-10T14:26:39Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"In preparation for a later patch that will create $state_dir/autostash\nin git-rebase.sh before anything else can happen, change a `mkdir\n$state_dir` call to `mkdir -p $state_dir`.  The change is safe,\nbecause this is not a test to detect an in-progress rebase (that is\nalready done much earlier in git-rebase.sh).\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\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 5822b2c..3411139 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -842,7 +842,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\" || die \"Could not create temporary $state_dir\"\n \n : > \"$state_dir\"/interactive || die \"Could not mark as interactive\"\n write_basic_state\n-- \n1.8.3.rc1.52.gc14258d\n"},{"id":"216873","messageId":"1368196005-5354-4-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/8] am: tighten a conditional that checks for $dotest","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:40Z","receivedAt":"2013-05-10T14:26:40Z","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 <artagnon@gmail.com>\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.3.rc1.52.gc14258d\n"},{"id":"216879","messageId":"1368196005-5354-5-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/8] rebase: prepare to do generic housekeeping","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:41Z","receivedAt":"2013-05-10T14:26:41Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"On successful completion of a rebase in git-rebase--$backend.sh, the\n$backend script cleans up on its own and exits.  The cleanup routine\nis however, independent of the $backend, and each $backend script\nunnecessarily duplicates this work:\n\n    rm -rf \"$state_dir\"\n    git gc --auto\n\nPrepare git-rebase.sh for later patches that return control from each\n$backend script back to us, for performing this generic cleanup\nroutine.\n\nAnother advantage is that git-rebase.sh can implement a generic\nfinish_rebase() to possibly do additional tasks in addition to the\ncleanup.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n git-rebase.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 2c692c3..84dc7b0 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.3.rc1.52.gc14258d\n"},{"id":"216875","messageId":"1368196005-5354-6-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/8] am: return control to caller, for housekeeping","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:42Z","receivedAt":"2013-05-10T14:26:42Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"We only need to do these two tasks\n\n    git gc --auto\n    rm -fr \"$dotest\"\n\nourselves if the script was invoked as a standalone program; when\ninvoked with --rebasing (from git-rebase--am.sh), cascade control back\nto the ultimate caller git-rebase.sh to do this for us.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n git-am.sh         | 9 +++++++--\n git-rebase--am.sh | 8 ++++----\n 2 files changed, 11 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..34e3102 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-- \n1.8.3.rc1.52.gc14258d\n"},{"id":"216876","messageId":"1368196005-5354-7-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 6/8] rebase -i: return control to caller, for housekeeping","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:43Z","receivedAt":"2013-05-10T14:26:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Return control to the caller git-rebase.sh to get these two tasks\n\n    rm -fr \"$dotest\"\n    git gc --auto\n\ndone by it.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n git-rebase--interactive.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 3411139..f953d8d 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -628,17 +628,16 @@ 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 1 # not failure; just to break the do_rest loop\n }\n \n+# can only return 0, when the infinite loop breaks\n do_rest () {\n \twhile :\n \tdo\n-\t\tdo_next\n+\t\tdo_next || break\n \tdone\n }\n \n@@ -805,11 +804,13 @@ first and then run 'git rebase --continue' again.\"\n \n \trequire_clean_work_tree \"rebase\"\n \tdo_rest\n+\treturn 0\n \t;;\n skip)\n \tgit rerere clear\n \n \tdo_rest\n+\treturn 0\n \t;;\n edit-todo)\n \tgit stripspace --strip-comments <\"$todo\" >\"$todo\".new\n-- \n1.8.3.rc1.52.gc14258d\n"},{"id":"216878","messageId":"1368196005-5354-8-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 7/8] rebase --merge: return control to caller, for housekeeping","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:44Z","receivedAt":"2013-05-10T14:26:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Return control to the caller git-rebase.sh to get these two tasks\n\n    rm -fr \"$dotest\"\n    git gc --auto\n\ndone by it.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n git-rebase--merge.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex b10f2cf..16d1817 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -96,7 +96,6 @@ finish_rb_merge () {\n \t\t\t\"$GIT_DIR\"/hooks/post-rewrite rebase <\"$state_dir\"/rewritten\n \t\tfi\n \tfi\n-\trm -r \"$state_dir\"\n \tsay All done.\n }\n \n@@ -110,7 +109,7 @@ continue)\n \t\tcontinue_merge\n \tdone\n \tfinish_rb_merge\n-\texit\n+\treturn\n \t;;\n skip)\n \tread_state\n@@ -122,7 +121,7 @@ skip)\n \t\tcontinue_merge\n \tdone\n \tfinish_rb_merge\n-\texit\n+\treturn\n \t;;\n esac\n \n-- \n1.8.3.rc1.52.gc14258d\n"},{"id":"216877","messageId":"1368196005-5354-9-git-send-email-artagnon@gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 8/8] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T14:26:45Z","receivedAt":"2013-05-10T14:26:45Z","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 <artagnon@gmail.com>\n---\n Documentation/config.txt     |   8 +++\n Documentation/git-rebase.txt |  10 +++\n git-rebase.sh                |  43 ++++++++++++-\n t/t3420-rebase-autostash.sh  | 148 +++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 206 insertions(+), 3 deletions(-)\n create mode 100755 t/t3420-rebase-autostash.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 6e53fc5..7fd4035 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 <newbase>::\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 84dc7b0..b58c6d9 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,28 @@ move_to_original_branch () {\n \tesac\n }\n \n+finish_rebase () {\n+\tif test -f \"$state_dir/autostash\"\n+\tthen\n+\t\tstash_sha1=$(cat \"$state_dir/autostash\")\n+\t\tif git stash apply $stash_sha1 2>&1 >/dev/null\n+\t\tthen\n+\t\t\techo \"Applied autostash\"\n+\t\telse\n+\t\t\tref_stash=refs/stash &&\n+\t\t\t: >>\"$GIT_DIR/logs/$ref_stash\" &&\n+\t\t\tgit update-ref -m \"autostash\" $ref_stash $stash_sha1 \\\n+\t\t\t\t|| die \"$(eval_gettext 'Cannot store $stash_sha1')\" &&\n+\t\t\techo \"\n+$(gettext 'Applying autostash resulted in conflicts.\n+Your changes are safe in stash@{0}.\n+You can apply or drop it at any time.')\"\n+\t\tfi\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 +177,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\tfinish_rebase\n \tfi\n \texit $ret\n }\n@@ -248,6 +271,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 +374,7 @@ abort)\n \t\t;;\n \tesac\n \toutput git reset --hard $orig_head\n-\trm -r \"$state_dir\"\n+\tfinish_rebase\n \texit\n \t;;\n edit-todo)\n@@ -487,6 +513,17 @@ case \"$#\" in\n \t;;\n esac\n \n+if test \"$autostash\" = true && ! (require_clean_work_tree) 2>/dev/null\n+then\n+\tstash_sha1=$(git stash create \"autostash\") \\\n+\t\t|| 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 \"$(eval_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,\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nnew file mode 100755\nindex 0000000..8386998\n--- /dev/null\n+++ b/t/t3420-rebase-autostash.sh\n@@ -0,0 +1,148 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2013 Ramkumar Ramachandra\n+#\n+\n+test_description='git rebase --autostash tests'\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo hello-world >file0 &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m \"initial commit\" &&\n+\tgit checkout -b feature-branch &&\n+\techo another-hello >file1 &&\n+\techo goodbye >file2 &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m \"second commit\" &&\n+\techo final-goodbye >file3 &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m \"third commit\"\n+\tgit checkout -b unrelated-onto-branch master &&\n+\techo unrelated >file4 &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m \"unrelated commit\"\n+\tgit checkout -b related-onto-branch master &&\n+\techo conflicting-change >file2 &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m \"related commit\"\n+'\n+\n+testrebase() {\n+\ttype=$1\n+\tdotest=$2\n+\n+\ttest_expect_success \"rebase$type: dirty worktree, non-conflicting rebase\" '\n+\t\ttest_config rebase.autostash true &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout -b rebased-feature-branch feature-branch &&\n+\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\techo dirty >>file3 &&\n+\t\tgit rebase$type unrelated-onto-branch &&\n+\t\tgrep unrelated file4 &&\n+\t\tgrep dirty file3 &&\n+\t\tgit checkout feature-branch\n+\t'\n+\n+\ttest_expect_success \"rebase$type: dirty index, non-conflicting rebase\" '\n+\t\ttest_config rebase.autostash true &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout -b rebased-feature-branch feature-branch &&\n+\t\ttest_when_finished git branch -D rebased-feature-branch\n+\t\techo dirty >>file3 &&\n+\t\tgit add file3 &&\n+\t\tgit rebase$type unrelated-onto-branch &&\n+\t\tgrep unrelated file4 &&\n+\t\tgrep dirty file3 &&\n+\t\tgit checkout feature-branch\n+\t'\n+\n+\ttest_expect_success \"rebase$type: conflicting rebase\" '\n+\t\ttest_config rebase.autostash true &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout -b rebased-feature-branch feature-branch &&\n+\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\techo dirty >>file3 &&\n+\t\ttest_must_fail git rebase$type related-onto-branch &&\n+\t\ttest_path_is_file $dotest/autostash &&\n+\t\t! grep dirty file3 &&\n+\t\trm -rf $dotest &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout feature-branch\n+\t'\n+\n+\ttest_expect_success \"rebase$type: --continue\" '\n+\t\ttest_config rebase.autostash true &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout -b rebased-feature-branch feature-branch &&\n+\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\techo dirty >>file3 &&\n+\t\ttest_must_fail git rebase$type related-onto-branch &&\n+\t\ttest_path_is_file $dotest/autostash &&\n+\t\t! grep dirty file3 &&\n+\t\techo \"conflicting-plus-goodbye\" >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit rebase --continue &&\n+\t\ttest_path_is_missing $dotest/autostash &&\n+\t\tgrep dirty file3 &&\n+\t\tgit checkout feature-branch\n+\t'\n+\n+\ttest_expect_success \"rebase$type: --skip\" '\n+\t\ttest_config rebase.autostash true &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout -b rebased-feature-branch feature-branch &&\n+\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\techo dirty >>file3 &&\n+\t\ttest_must_fail git rebase$type related-onto-branch &&\n+\t\ttest_path_is_file $dotest/autostash &&\n+\t\t! grep dirty file3 &&\n+\t\tgit rebase --skip &&\n+\t\ttest_path_is_missing $dotest/autostash &&\n+\t\tgrep dirty file3 &&\n+\t\tgit checkout feature-branch\n+\t'\n+\n+\ttest_expect_success \"rebase$type: --abort\" '\n+\t\ttest_config rebase.autostash true &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout -b rebased-feature-branch feature-branch &&\n+\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\techo dirty >>file3 &&\n+\t\ttest_must_fail git rebase$type related-onto-branch &&\n+\t\ttest_path_is_file $dotest/autostash &&\n+\t\t! grep dirty file3 &&\n+\t\tgit rebase --abort &&\n+\t\ttest_path_is_missing $dotest/autostash &&\n+\t\tgrep dirty file3 &&\n+\t\tgit checkout feature-branch\n+\t'\n+\n+\ttest_expect_success \"rebase$type: non-conflicting rebase, conflicting stash\" '\n+\t\ttest_config rebase.autostash true &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout -b rebased-feature-branch feature-branch &&\n+\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\techo dirty >file4 &&\n+\t\tgit add file4 &&\n+\t\tgit rebase$type unrelated-onto-branch &&\n+\t\ttest_path_is_missing $dotest &&\n+\t\tgit reset --hard &&\n+\t\tgrep unrelated file4 &&\n+\t\t! grep dirty file4 &&\n+\t\tgit checkout feature-branch &&\n+\t\tgit stash pop &&\n+\t\tgrep dirty file4\n+\t'\n+}\n+\n+testrebase \"\" .git/rebase-apply\n+testrebase \" --merge\" .git/rebase-merge\n+testrebase \" --interactive\" .git/rebase-merge\n+\n+test_done\n-- \n1.8.3.rc1.52.gc14258d\n"},{"id":"216886","messageId":"CAPig+cTAEwA10JY56Fe7NyVMRfvtupbG7ThML33SNx4zcX2Ebw@mail.gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/8] rebase: prepare to do generic housekeeping","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-10T14:33:49Z","receivedAt":"2013-05-10T14:33:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> On successful completion of a rebase in git-rebase--$backend.sh, the\n> $backend script cleans up on its own and exits.  The cleanup routine\n> is however, independent of the $backend, and each $backend script\n> unnecessarily duplicates this work:\n>\n>     rm -rf \"$state_dir\"\n>     git gc --auto\n>\n> Prepare git-rebase.sh for later patches that return control from each\n> $backend script back to us, for performing this generic cleanup\n> routine.\n>\n> Another advantage is that git-rebase.sh can implement a generic\n> finish_rebase() to possibly do additional tasks in addition to the\n> cleanup.\n>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n>  git-rebase.sh | 7 +++++++\n>  1 file changed, 7 insertions(+)\n>\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index 2c692c3..84dc7b0 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -150,6 +150,13 @@ run_specific_rebase () {\n>                 autosquash=\n>         fi\n>         . git-rebase--$type\n> +       ret=$?\n> +       if test $ret = 0\n\nFor numeric comparison, use '-eq' rather than '=', which is for strings.\n\n> +       then\n> +               git gc --auto &&\n> +               rm -rf \"$state_dir\"\n> +       fi\n> +       exit $ret\n>  }\n>\n>  run_pre_rebase_hook () {\n"},{"id":"216889","messageId":"CAPig+cQuWTfDqoF4G_Sk5_9VWKffpqmGC2WxXeF9DCyDxLx16A@mail.gmail.com","threadId":"33771","inReplyTo":"1368196005-5354-9-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 8/8] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-10T14:41:05Z","receivedAt":"2013-05-10T14:41:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> new file mode 100755\n> index 0000000..8386998\n> --- /dev/null\n> +++ b/t/t3420-rebase-autostash.sh\n> @@ -0,0 +1,148 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2013 Ramkumar Ramachandra\n> +#\n> +\n> +test_description='git rebase --autostash tests'\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +       echo hello-world >file0 &&\n> +       git add . &&\n> +       test_tick &&\n> +       git commit -m \"initial commit\" &&\n> +       git checkout -b feature-branch &&\n> +       echo another-hello >file1 &&\n> +       echo goodbye >file2 &&\n> +       git add . &&\n> +       test_tick &&\n> +       git commit -m \"second commit\" &&\n> +       echo final-goodbye >file3 &&\n> +       git add . &&\n> +       test_tick &&\n> +       git commit -m \"third commit\"\n\nBroken &&-chain.\n\n> +       git checkout -b unrelated-onto-branch master &&\n> +       echo unrelated >file4 &&\n> +       git add . &&\n> +       test_tick &&\n> +       git commit -m \"unrelated commit\"\n\nBroken &&-chain.\n\n> +       git checkout -b related-onto-branch master &&\n> +       echo conflicting-change >file2 &&\n> +       git add . &&\n> +       test_tick &&\n> +       git commit -m \"related commit\"\n> +'\n> +\n> +testrebase() {\n> +       type=$1\n> +       dotest=$2\n> +\n> +       test_expect_success \"rebase$type: dirty worktree, non-conflicting rebase\" '\n> +               test_config rebase.autostash true &&\n> +               git reset --hard &&\n> +               git checkout -b rebased-feature-branch feature-branch &&\n> +               test_when_finished git branch -D rebased-feature-branch &&\n> +               echo dirty >>file3 &&\n> +               git rebase$type unrelated-onto-branch &&\n> +               grep unrelated file4 &&\n> +               grep dirty file3 &&\n> +               git checkout feature-branch\n> +       '\n> +\n> +       test_expect_success \"rebase$type: dirty index, non-conflicting rebase\" '\n> +               test_config rebase.autostash true &&\n> +               git reset --hard &&\n> +               git checkout -b rebased-feature-branch feature-branch &&\n> +               test_when_finished git branch -D rebased-feature-branch\n\nBroken &&-chain.\n\n> +               echo dirty >>file3 &&\n> +               git add file3 &&\n> +               git rebase$type unrelated-onto-branch &&\n> +               grep unrelated file4 &&\n> +               grep dirty file3 &&\n> +               git checkout feature-branch\n> +       '\n> +\n> +       test_expect_success \"rebase$type: conflicting rebase\" '\n> +               test_config rebase.autostash true &&\n> +               git reset --hard &&\n> +               git checkout -b rebased-feature-branch feature-branch &&\n> +               test_when_finished git branch -D rebased-feature-branch &&\n> +               echo dirty >>file3 &&\n> +               test_must_fail git rebase$type related-onto-branch &&\n> +               test_path_is_file $dotest/autostash &&\n> +               ! grep dirty file3 &&\n> +               rm -rf $dotest &&\n> +               git reset --hard &&\n> +               git checkout feature-branch\n> +       '\n> +\n> +       test_expect_success \"rebase$type: --continue\" '\n> +               test_config rebase.autostash true &&\n> +               git reset --hard &&\n> +               git checkout -b rebased-feature-branch feature-branch &&\n> +               test_when_finished git branch -D rebased-feature-branch &&\n> +               echo dirty >>file3 &&\n> +               test_must_fail git rebase$type related-onto-branch &&\n> +               test_path_is_file $dotest/autostash &&\n> +               ! grep dirty file3 &&\n> +               echo \"conflicting-plus-goodbye\" >file2 &&\n> +               git add file2 &&\n> +               git rebase --continue &&\n> +               test_path_is_missing $dotest/autostash &&\n> +               grep dirty file3 &&\n> +               git checkout feature-branch\n> +       '\n> +\n> +       test_expect_success \"rebase$type: --skip\" '\n> +               test_config rebase.autostash true &&\n> +               git reset --hard &&\n> +               git checkout -b rebased-feature-branch feature-branch &&\n> +               test_when_finished git branch -D rebased-feature-branch &&\n> +               echo dirty >>file3 &&\n> +               test_must_fail git rebase$type related-onto-branch &&\n> +               test_path_is_file $dotest/autostash &&\n> +               ! grep dirty file3 &&\n> +               git rebase --skip &&\n> +               test_path_is_missing $dotest/autostash &&\n> +               grep dirty file3 &&\n> +               git checkout feature-branch\n> +       '\n> +\n> +       test_expect_success \"rebase$type: --abort\" '\n> +               test_config rebase.autostash true &&\n> +               git reset --hard &&\n> +               git checkout -b rebased-feature-branch feature-branch &&\n> +               test_when_finished git branch -D rebased-feature-branch &&\n> +               echo dirty >>file3 &&\n> +               test_must_fail git rebase$type related-onto-branch &&\n> +               test_path_is_file $dotest/autostash &&\n> +               ! grep dirty file3 &&\n> +               git rebase --abort &&\n> +               test_path_is_missing $dotest/autostash &&\n> +               grep dirty file3 &&\n> +               git checkout feature-branch\n> +       '\n> +\n> +       test_expect_success \"rebase$type: non-conflicting rebase, conflicting stash\" '\n> +               test_config rebase.autostash true &&\n> +               git reset --hard &&\n> +               git checkout -b rebased-feature-branch feature-branch &&\n> +               test_when_finished git branch -D rebased-feature-branch &&\n> +               echo dirty >file4 &&\n> +               git add file4 &&\n> +               git rebase$type unrelated-onto-branch &&\n> +               test_path_is_missing $dotest &&\n> +               git reset --hard &&\n> +               grep unrelated file4 &&\n> +               ! grep dirty file4 &&\n> +               git checkout feature-branch &&\n> +               git stash pop &&\n> +               grep dirty file4\n> +       '\n> +}\n> +\n> +testrebase \"\" .git/rebase-apply\n> +testrebase \" --merge\" .git/rebase-merge\n> +testrebase \" --interactive\" .git/rebase-merge\n> +\n> +test_done\n"},{"id":"216890","messageId":"7vhaia51vh.fsf@alter.siamese.dyndns.org","threadId":"33771","inReplyTo":"1368196005-5354-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/8] am: suppress error output from a conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T14:46:26Z","receivedAt":"2013-05-10T14:46:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> In preparation for a later patch that creates $dotest/autostash in\n> git-rebase.sh before anything else happens, don't assume that the\n> presence of a $dotest directory implies the existence of the $next and\n> $last files.  The check for the files is in a conditional anyway, but\n> `cat` is executed on potentially non-existent files.  Suppress this\n> error output.\n>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n>  git-am.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-am.sh b/git-am.sh\n> index 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\nIf you are introducing \"dotest exists but next/last may not be\npresent\" as a valid new state, it probably should check the presence\nand/or absence of them explicitly, but more importantly, it is a\ngood idea to move \"test $# != 0\" higher. Earlier it did not matter\nbecause when $dotest existed, next/last were supposed to be there\nand absence of them was an error codepath. Now, missing these files\nis not an error but is a perfectly normal state, so checking what\ncan be checked more cheaply makes sense as a general code hygiene.\n\nAs you may already know, I am not taking a patch that is not meant\nfor 'master' to fix regressions in 1.8.3 at this point in the cycle\nafter -rc2; please hold onto this and other patches as they won't\nstay in my mailbox.\n"},{"id":"216903","messageId":"7v8v3m50ds.fsf@alter.siamese.dyndns.org","threadId":"33771","inReplyTo":"CAPig+cTAEwA10JY56Fe7NyVMRfvtupbG7ThML33SNx4zcX2Ebw@mail.gmail.com","subject":"Re: [PATCH 4/8] rebase: prepare to do generic housekeeping","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T15:18:39Z","receivedAt":"2013-05-10T15:18:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Fri, May 10, 2013 at 10:26 AM, Ramkumar Ramachandra\n> <artagnon@gmail.com> wrote:\n>> On successful completion of a rebase in git-rebase--$backend.sh, the\n>> $backend script cleans up on its own and exits.  The cleanup routine\n>> is however, independent of the $backend, and each $backend script\n>> unnecessarily duplicates this work:\n>>\n>>     rm -rf \"$state_dir\"\n>>     git gc --auto\n>>\n>> Prepare git-rebase.sh for later patches that return control from each\n>> $backend script back to us, for performing this generic cleanup\n>> routine.\n>>\n>> Another advantage is that git-rebase.sh can implement a generic\n>> finish_rebase() to possibly do additional tasks in addition to the\n>> cleanup.\n>>\n>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n>> ---\n>>  git-rebase.sh | 7 +++++++\n>>  1 file changed, 7 insertions(+)\n>>\n>> diff --git a/git-rebase.sh b/git-rebase.sh\n>> index 2c692c3..84dc7b0 100755\n>> --- a/git-rebase.sh\n>> +++ b/git-rebase.sh\n>> @@ -150,6 +150,13 @@ run_specific_rebase () {\n>>                 autosquash=\n>>         fi\n>>         . git-rebase--$type\n>> +       ret=$?\n>> +       if test $ret = 0\n>\n> For numeric comparison, use '-eq' rather than '=', which is for strings.\n\nDo not listen to this.\n\nWe know the condition we want is to have $?  with a value that\nstringifies to \"0\" and the above reads much easier.\n\n>\n>> +       then\n>> +               git gc --auto &&\n>> +               rm -rf \"$state_dir\"\n>> +       fi\n>> +       exit $ret\n>>  }\n>>\n>>  run_pre_rebase_hook () {\n"},{"id":"216904","messageId":"7v38tu4zv7.fsf@alter.siamese.dyndns.org","threadId":"33771","inReplyTo":"1368196005-5354-9-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 8/8] rebase: implement --[no-]autostash and rebase.autostash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T15:29:48Z","receivedAt":"2013-05-10T15:29:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> +finish_rebase () {\n> +\tif test -f \"$state_dir/autostash\"\n> +\tthen\n> +\t\tstash_sha1=$(cat \"$state_dir/autostash\")\n> +\t\tif git stash apply $stash_sha1 2>&1 >/dev/null\n> +\t\tthen\n> +\t\t\techo \"Applied autostash\"\n> +\t\telse\n> +\t\t\tref_stash=refs/stash &&\n> +\t\t\t: >>\"$GIT_DIR/logs/$ref_stash\" &&\n> +\t\t\tgit update-ref -m \"autostash\" $ref_stash $stash_sha1 \\\n> +\t\t\t\t|| die \"$(eval_gettext 'Cannot store $stash_sha1')\" &&\n> +\t\t\techo \"\n> +$(gettext 'Applying autostash resulted in conflicts.\n> +...\n\nThis is confusing (or confused):\n\n\tthese && might && fail\n            || die &&\n        success\n\nIt is much easier to read if you just drop && after die and write it\nlike so:\n\t\n\t\tref_stash=refs/stash &&\n\t\t: >>\"$GIT_DIR/logs/$ref_stash\" &&\n\t\tgit update-ref -m \"autostash\" $ref_stash $stash_sha1 ||\n\t\tdie \"$(eval_gettext 'Cannot store $stash_sha1')\"\n\n\t\techo \"... success message comes here...\"\n"},{"id":"216906","messageId":"7vvc6q3l6t.fsf@alter.siamese.dyndns.org","threadId":"33771","inReplyTo":"1368196005-5354-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/8] rebase: prepare to do generic housekeeping","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-10T15:32:10Z","receivedAt":"2013-05-10T15:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> On successful completion of a rebase in git-rebase--$backend.sh, the\n> $backend script cleans up on its own and exits.  The cleanup routine\n> is however, independent of the $backend, and each $backend script\n> unnecessarily duplicates this work:\n>\n>     rm -rf \"$state_dir\"\n>     git gc --auto\n>\n> Prepare git-rebase.sh for later patches that return control from each\n> $backend script back to us, for performing this generic cleanup\n> routine.\n>\n> Another advantage is that git-rebase.sh can implement a generic\n> finish_rebase() to possibly do additional tasks in addition to the\n> cleanup.\n>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n>  git-rebase.sh | 7 +++++++\n>  1 file changed, 7 insertions(+)\n>\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index 2c692c3..84dc7b0 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\nDoesn't this exit look suspicious?  The existing callsites of this\nshell function has a lot of code after them (e.g. when \"continue\"\n$action is given, run_specific_rebase is run) but there is no exit\nafter the call returns, so they may already be expecting the\nfunction not to return, exiting by itself.  But then the last step\nof this function in the original code, \". git-rebase--$type\", would\nbe the one that is causing us to exit, no?\n\nSo it is either (1) the added code is unreachable and unexercised at\nthis point in the series, or (2) my analysis above is incorrect and\n\". git-rebase--$type\" does return to let the caller proceed, but\nthis patch changes the behaviour and breaks the caller.  I think it\nis the former but then the organization of the series does not make\nsense.\n\nPerhaps this should come a bit later in the series?\n\nAt least the log message should mention that this is adding an\nunreachable cruft at this step, if the order is to be kept.\n"},{"id":"216912","messageId":"CALkWK0=hK4Nj24xEYtfnAmrLsKLz+iJXvM4PKL9NHEJDS35mfQ@mail.gmail.com","threadId":"33771","inReplyTo":"7vvc6q3l6t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/8] rebase: prepare to do generic housekeeping","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-10T16:39:29Z","receivedAt":"2013-05-10T16:39:29Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> So it is either (1) the added code is unreachable and unexercised at\n> this point in the series, or\n\nYeah, it's (1).\n\n> Perhaps this should come a bit later in the series?\n\nWhen exactly?  I picked up on your suggestion to separate out the\npreparation-for-$backend-to-return step.  The next three patches\nconvert each of the $backend scripts to return.\n\n> At least the log message should mention that this is adding an\n> unreachable cruft at this step, if the order is to be kept.\n\nOkay, I can update the commit message.\n"},{"id":"217052","messageId":"CALkWK0mtJU_fWK7A2BXN3f5VRT4Qh69PE0nfQaTJ6GU1Z5=wig@mail.gmail.com","threadId":"33771","inReplyTo":"7vhaia51vh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/8] am: suppress error output from a conditional","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-11T23:38:43Z","receivedAt":"2013-05-11T23:38:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> If you are introducing \"dotest exists but next/last may not be\n> present\" as a valid new state, it probably should check the presence\n> and/or absence of them explicitly,\n\nUm, a test -f preceding the cat?  Why, when cat implicitly checks\nexistence anyway?\n\n> but more importantly, it is a\n> good idea to move \"test $# != 0\" higher.\n\nThis?\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 47c1021..1e10e31 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -446,9 +446,9 @@ 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+   test $# != 0 &&\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    rm -fr \"$dotest\"\n\n> Earlier it did not matter\n> because when $dotest existed, next/last were supposed to be there\n> and absence of them was an error codepath. Now, missing these files\n> is not an error but is a perfectly normal state,\n\nIt's not a \"normal state\" for the purposes of git-am.sh.  There is no\ncodepath in the program that depends only on $dotest, but not\nnext/last.  I would frame this as: \"checking for $dotest is not a\nsufficient prerequisite anymore; we have to additionally look for\nnext/last\".  To git-am.sh, an empty $dotest or just a\n$dotest/autostash is equivalent to no $dotest at all.\n\n> so checking what\n> can be checked more cheaply makes sense as a general code hygiene.\n\nYeah, I agree.  See \"am: tighten a conditional that checks for\n$dotest\", where I get away with just checking $dotest/last (and assume\nthat $dotest/next is present by extension).  How do I apply your\nsuggestion to this particular patch?\n"},{"id":"217056","messageId":"7vwqr4vqpv.fsf@alter.siamese.dyndns.org","threadId":"33771","inReplyTo":"CALkWK0mtJU_fWK7A2BXN3f5VRT4Qh69PE0nfQaTJ6GU1Z5=wig@mail.gmail.com","subject":"Re: [PATCH 1/8] am: suppress error output from a conditional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-12T03:09:48Z","receivedAt":"2013-05-12T03:09:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> If you are introducing \"dotest exists but next/last may not be\n>> present\" as a valid new state, it probably should check the presence\n>> and/or absence of them explicitly,\n>\n> Um, a test -f preceding the cat?  Why, when cat implicitly checks\n> existence anyway?\n\nWith that logic, 'test -d \"$dotest\"' on the first line would also be\nredundant, as we read from \"$dotest/last\" and that will catch an\nerror.\n\nIt is more about what is conceptually right.  You need to think what\nthe condition _means_.\n\nWe protect the \"remove $dotest directory\" with the precondition you\nare changing, but what is that precondition really trying to say?\nIf $dotest does not exist, obviously we do not need to remove it,\nbut what is the essence of that sereis of tests?\n\nIt is what the comment says:\n\n    \"We have finished applying all the patches in them\"\n\nEarlier, presense of $dotest _must_ have meant that last and next\nshould exist (otherwise you have a corrupt state and we did want to\nsee error messages), and the check original did was perfectly fine.\n\nNow, a mere presense of $dotest does _not_ mean \"we have finished\napplying all the patches\", because sometimes you create it without\nhaving anything to put in last or next yet.  That is why it starts\nto make sense to do\n\n        if test -d \"$dotest\" &&\n           test -f \"$dotest/last\" &&\n           test -f \"$dotest/next\" &&\n           last=$(cat \"$dotest/last\") &&\n           ...\n"}]}