Re: [PATCH 1/2] am: handle stray $dotest directory
Ramkumar Ramachandra wrote: Can you tell me how to get shell-script-mode to indent the case statement properly? (I used the default indentation) Never mind; I figured it out: (setq sh-indent-for-case-label 0) (setq sh-indent-for-case-alt '+) Maybe we should dump the relevant parts of my .emacs somewhere in-tree? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Junio C Hamano wrote: I _think_ the new check you added may be too loose. Yep, I totally forgot about the case when git-am.sh is called from an existing script. In that case, it is upto the caller to handle whatever stray directories; we have no business meddling with that. A fix-up may look like this. No, don't leak autostash detail. Just change that condition to if test -d $dotest test -z $rebasing and we're done. I'll send in a re-roll. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Ramkumar Ramachandra artag...@gmail.com writes: The following bug has been observed: $ git am # no input file ^C $ git am --abort Resolve operation not in progress, we are not resuming. This happens because the following test fails: test -d $dotest test -f $dotest/last test -f $dotest/next and the codepath for an am in-progress is not executed. It falls back to the codepath that treats this as a fresh execution. Before rr/rebase-autostash, this condition was test -d $dotest It would incorrectly execute the normal am --abort codepath: git read-tree --reset -u HEAD ORIG_HEAD git reset ORIG_HEAD by incorrectly assuming that an am is in progress (i.e. ORIG_HEAD etc. was written during the previous execution). Notice that $ git am ^C executes nothing of significance, is equivalent to $ mkdir .git/rebase-apply Therefore, the correct solution is to treat .git/rebase-apply as a stray directory and remove it on --abort in the fresh-execution codepath. While at it, tell the user to run git am --abort to get rid of the stray $dotest directory, if she attempts anything else. Reported-by: Junio C Hamano gits...@pobox.com Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- git-am.sh | 14 ++ t/t4150-am.sh | 6 ++ 2 files changed, 20 insertions(+) diff --git a/git-am.sh b/git-am.sh index 1cf3d1d..37ee18b 100755 --- a/git-am.sh +++ b/git-am.sh @@ -506,6 +506,20 @@ then esac rm -f $dotest/dirtyindex else + # Possible stray $dotest directory + if test -d $dotest; then + case $skip,$resolved,$abort in + ,,t) + rm -fr $dotest + exit 0 + ;; + *) + die $(eval_gettext Stray $dotest directory found. +Use \git am --abort\ to remove it.) + ;; + esac These two case arms are indented one level too deep (will locally touch up). + fi + # Make sure we are not given --skip, --resolved, nor --abort test $skip$resolved$abort = || die $(gettext Resolve operation not in progress, we are not resuming.) diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 12f6b02..6c2cc3e 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -363,6 +363,12 @@ test_expect_success 'am --skip works' ' test_cmp expected another ' +test_expect_success 'am --abort removes a stray directory' ' + mkdir .git/rebase-apply + git am --abort + test_path_is_missing .git/rebase-apply +' + test_expect_success 'am --resolved works' ' echo goodbye expected rm -fr .git/rebase-apply -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Ramkumar Ramachandra artag...@gmail.com writes: + die $(eval_gettext Stray $dotest directory found. +Use \git am --abort\ to remove it.) $dotest, or \$dotest? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Junio C Hamano wrote: These two case arms are indented one level too deep (will locally touch up). Thanks. Can you tell me how to get shell-script-mode to indent the case statement properly? (I used the default indentation) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Junio C Hamano wrote: $dotest, or \$dotest? Works fine for me like this. Why do we escape the dollar in the other strings? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Junio C Hamano gits...@pobox.com writes: +# Possible stray $dotest directory +if test -d $dotest; then +case $skip,$resolved,$abort in +,,t) +rm -fr $dotest +exit 0 +;; +*) +die $(eval_gettext Stray $dotest directory found. +Use \git am --abort\ to remove it.) +;; +esac These two case arms are indented one level too deep (will locally touch up). And then the message triggers at the second test in t3420 when applied on top of 587947750bd7 (rebase: implement --[no-]autostash and rebase.autostash, 2013-05-12) or 45acb7592825 (Merge branch 'rr/rebase-autostash', 2013-06-11). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: $dotest, or \$dotest? Works fine for me like this. Why do we escape the dollar in the other strings? The reason would become clear once you think what string you are feeding eval_gettext with if you do not escape. The translators translate a fixed string (possibly with placeholders) to a fixed translated string (possibly with placeholders). eval_gettext Stray $dotest directory found. ... would allow the shell to expand $dotest before eval_gettext sees it, which would mean the string is no longer a constant. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: And then the message triggers at the second test in t3420 when applied on top of 587947750bd7 (rebase: implement --[no-]autostash and rebase.autostash, 2013-05-12) or 45acb7592825 (Merge branch 'rr/rebase-autostash', 2013-06-11). What was triggered? (I didn't understand what you said) The patch applied on top of either of these commits will *BREAK* the second test in t3420. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: The reason would become clear once you think what string you are feeding eval_gettext with if you do not escape. The translators translate a fixed string (possibly with placeholders) to a fixed translated string (possibly with placeholders). eval_gettext Stray $dotest directory found. ... would allow the shell to expand $dotest before eval_gettext sees it, which would mean the string is no longer a constant. Ah. I was scratching my head wondering why $dotest needed to be translated (it's just a path). Stray and directory found are to be translated. You can have infinite possibilities to $dotest that is computed at runtime, so that should be kept as a variable, untranslated in the message template. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] am: handle stray $dotest directory
Junio C Hamano gits...@pobox.com writes: Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: And then the message triggers at the second test in t3420 when applied on top of 587947750bd7 (rebase: implement --[no-]autostash and rebase.autostash, 2013-05-12) or 45acb7592825 (Merge branch 'rr/rebase-autostash', 2013-06-11). What was triggered? (I didn't understand what you said) The patch applied on top of either of these commits will *BREAK* the second test in t3420. Here is how sh -x t3420-*.sh -i ends: ... ++ git rebase unrelated-onto-branch Created autostash: cdca6ca HEAD is now at 0c4d2f1 third commit First, rewinding head to replay your work on top of it... Stray /srv/project/git/git.git/t/trash directory.t3420-rebase-autostash/.git/rebase-apply directory found. Use git am --abort to remove it. + eval_ret=1 + test -z t + test 1 = 0 + test -n '' + test t = t + test -n '' + return 1 + test_failure_ 'rebase: dirty worktree, non-conflicting rebase' ' ... I _think_ the new check you added may be too loose. A fix-up may look like this. git-am.sh | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/git-am.sh b/git-am.sh index 37edfae..3f89cf6 100755 --- a/git-am.sh +++ b/git-am.sh @@ -505,9 +505,9 @@ then exit ;; esac rm -f $dotest/dirtyindex -else - # Possible stray $dotest directory - if test -d $dotest; then +elif test -d $dotest ! test -f $dotest/autostash +then + # stray $dotest directory case $skip,$resolved,$abort in ,,t) rm -fr $dotest @@ -518,8 +518,7 @@ else Use \git am --abort\ to remove it.) ;; esac - fi - +else # Make sure we are not given --skip, --resolved, nor --abort test $skip$resolved$abort = || die $(gettext Resolve operation not in progress, we are not resuming.) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
