{"thread":{"id":"34127","subject":"[PATCH 1/3] rebase: guard against missing files in read_basic_state()","startedAt":"2013-06-13T16:06:10Z","lastAt":"2013-06-20T19:56:52Z","messageCount":7,"participants":["Ramkumar Ramachandra","Junio C Hamano","Martin von Zweigbergk"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"220736","messageId":"1371139573-28047-1-git-send-email-artagnon@gmail.com","threadId":"34127","inReplyTo":null,"subject":"[PATCH 0/3] Fix a couple of edge cases in autostash","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-13T16:06:10Z","receivedAt":"2013-06-13T16:06:10Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nI apologize for having missed these two trivial cases in the original\nseries.\n\nRamkumar Ramachandra (3):\n  rebase: guard against missing files in read_basic_state()\n  rebase: finish_rebase() in fast-forward rebase\n  rebase: finish_rebase() in noop rebase\n\n git-rebase.sh               |  4 ++++\n t/t3420-rebase-autostash.sh | 22 ++++++++++++++++++++++\n 2 files changed, 26 insertions(+)\n\n-- \n1.8.3.1.381.gf08dd97.dirty\n"},{"id":"220735","messageId":"1371139573-28047-2-git-send-email-artagnon@gmail.com","threadId":"34127","inReplyTo":"1371139573-28047-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/3] rebase: guard against missing files in read_basic_state()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-13T16:06:11Z","receivedAt":"2013-06-13T16:06:11Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n git-rebase.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex d0c11a9..2122fe0 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -84,6 +84,8 @@ keep_empty=\n test \"$(git config --bool rebase.autosquash)\" = \"true\" && autosquash=t\n \n read_basic_state () {\n+\ttest -f \"$state_dir/head-name\" &&\n+\ttest -f \"$state_dir/onto\" &&\n \thead_name=$(cat \"$state_dir\"/head-name) &&\n \tonto=$(cat \"$state_dir\"/onto) &&\n \t# We always write to orig-head, but interactive rebase used to write to\n-- \n1.8.3.1.381.gf08dd97.dirty\n"},{"id":"220737","messageId":"1371139573-28047-3-git-send-email-artagnon@gmail.com","threadId":"34127","inReplyTo":"1371139573-28047-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/3] rebase: finish_rebase() in fast-forward rebase","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-13T16:06:12Z","receivedAt":"2013-06-13T16:06:12Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"In the following case\n\n  $ git rebase master\n  Fast-forwarded autostash-fix to master.\n\nThe autostash is not applied automatically, because this codepath\nforgets to call finish_rebase().  Fix this.  Also add a test to guard\nagainst regressions.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n git-rebase.sh               |  1 +\n t/t3420-rebase-autostash.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 2122fe0..154d4be 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -579,6 +579,7 @@ if test \"$mb\" = \"$orig_head\"\n then\n \tsay \"$(eval_gettext \"Fast-forwarded \\$branch_name to \\$onto_name.\")\"\n \tmove_to_original_branch\n+\tfinish_rebase\n \texit 0\n fi\n \ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 479cbb2..1bde007 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -141,6 +141,17 @@ testrebase() {\n \t'\n }\n \n+test_expect_success \"rebase: fast-forward rebase\" '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard &&\n+\tgit checkout -b behind-feature-branch feature-branch~1 &&\n+\ttest_when_finished git branch -D behind-feature-branch &&\n+\techo dirty >>file1 &&\n+\tgit rebase feature-branch &&\n+\tgrep dirty file1 &&\n+\tgit checkout feature-branch\n+'\n+\n testrebase \"\" .git/rebase-apply\n testrebase \" --merge\" .git/rebase-merge\n testrebase \" --interactive\" .git/rebase-merge\n-- \n1.8.3.1.381.gf08dd97.dirty\n"},{"id":"220738","messageId":"1371139573-28047-4-git-send-email-artagnon@gmail.com","threadId":"34127","inReplyTo":"1371139573-28047-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/3] rebase: finish_rebase() in noop rebase","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-13T16:06:13Z","receivedAt":"2013-06-13T16:06:13Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"In the following case\n\n  $ git rebase master\n  Current branch autostash-fix is up to date.\n\nthe autostash is not applied automatically, because this codepath\nforgets to call finish_rebase().  Fix this.  Also add a test to guard\nagainst regressions.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n git-rebase.sh               |  1 +\n t/t3420-rebase-autostash.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 154d4be..2d5c2bd 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -547,6 +547,7 @@ then\n \t\t# Lazily switch to the target branch if needed...\n \t\ttest -z \"$switch_to\" || git checkout \"$switch_to\" --\n \t\tsay \"$(eval_gettext \"Current branch \\$branch_name is up to date.\")\"\n+\t\tfinish_rebase\n \t\texit 0\n \telse\n \t\tsay \"$(eval_gettext \"Current branch \\$branch_name is up to date, rebase forced.\")\"\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 1bde007..90eb264 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -152,6 +152,17 @@ test_expect_success \"rebase: fast-forward rebase\" '\n \tgit checkout feature-branch\n '\n \n+test_expect_success \"rebase: noop rebase\" '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard &&\n+\tgit checkout -b same-feature-branch feature-branch &&\n+\ttest_when_finished git branch -D same-feature-branch &&\n+\techo dirty >>file1 &&\n+\tgit rebase feature-branch &&\n+\tgrep dirty file1 &&\n+\tgit checkout feature-branch\n+'\n+\n testrebase \"\" .git/rebase-apply\n testrebase \" --merge\" .git/rebase-merge\n testrebase \" --interactive\" .git/rebase-merge\n-- \n1.8.3.1.381.gf08dd97.dirty\n"},{"id":"220774","messageId":"7vy5adskz3.fsf@alter.siamese.dyndns.org","threadId":"34127","inReplyTo":"1371139573-28047-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/3] rebase: guard against missing files in read_basic_state()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-13T22:29:36Z","receivedAt":"2013-06-13T22:29:36Z","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> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n\nThis does not affect correctness; i.e. head_name=$(cat that-file)\nwill error out if the file is missing, right?\n\nA more troublesome is that nobody seems to check the return value of\nthis function.  If head-name, onto or orig-head is missing, is that\nan error condition that should make the callers of read_basic_state\nstop and refuse to proceed?\n\nThe way the && cascade is used seems to indicate that, but up to the\npoint where it sents $verbose. If and only if head-name, onto, orig-head\nand quiet can be read in state-dir, verbose in state-dir is checked\nand only then $verbose is set.\n\nMartin, this seems to be from your series around early Feburary\n2011.  Do you recall why these checks are cascaded this way?\nI do not offhand think of a good reason.\n\n> ---\n>  git-rebase.sh | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index d0c11a9..2122fe0 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -84,6 +84,8 @@ keep_empty=\n>  test \"$(git config --bool rebase.autosquash)\" = \"true\" && autosquash=t\n>  \n>  read_basic_state () {\n> +\ttest -f \"$state_dir/head-name\" &&\n> +\ttest -f \"$state_dir/onto\" &&\n>  \thead_name=$(cat \"$state_dir\"/head-name) &&\n>  \tonto=$(cat \"$state_dir\"/onto) &&\n>  \t# We always write to orig-head, but interactive rebase used to write to\n"},{"id":"220978","messageId":"CANiSa6hUV-xMMASbJf67j0Zn+Mu25CRcvQThNd1EuemhSBB6Rg@mail.gmail.com","threadId":"34127","inReplyTo":"7vy5adskz3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] rebase: guard against missing files in read_basic_state()","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2013-06-16T05:45:17Z","receivedAt":"2013-06-16T05:45:17Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Thu, Jun 13, 2013 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n> A more troublesome is that nobody seems to check the return value of\n> this function.  If head-name, onto or orig-head is missing, is that\n> an error condition that should make the callers of read_basic_state\n> stop and refuse to proceed?\n\nSince we unconditionally write those three (and 'quiet'), it seems\nreasonable to require all of them to be there when continuing, so I\nthink you're right that we should fail fast.\n\n> The way the && cascade is used seems to indicate that, but up to the\n> point where it sents $verbose. If and only if head-name, onto, orig-head\n> and quiet can be read in state-dir, verbose in state-dir is checked\n> and only then $verbose is set.\n>\n> Martin, this seems to be from your series around early Feburary\n> 2011.  Do you recall why these checks are cascaded this way?\n> I do not offhand think of a good reason.\n\nNeither do I. I think the cascading after 'quiet' is just a mistake on\nmy part. The consequences are probably close to none, since if one of\nearlier commands fail, the other files will probably not be there\neither. (Not defending it; I'm happy if it gets fixed, e.g. by making\nit fail fast.)\n"},{"id":"221509","messageId":"7v1u7wtv23.fsf@alter.siamese.dyndns.org","threadId":"34127","inReplyTo":"CANiSa6hUV-xMMASbJf67j0Zn+Mu25CRcvQThNd1EuemhSBB6Rg@mail.gmail.com","subject":"Re: [PATCH 1/3] rebase: guard against missing files in read_basic_state()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-20T19:56:52Z","receivedAt":"2013-06-20T19:56:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk <martinvonz@gmail.com> writes:\n\n> On Thu, Jun 13, 2013 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>>\n>> A more troublesome is that nobody seems to check the return value of\n>> this function.  If head-name, onto or orig-head is missing, is that\n>> an error condition that should make the callers of read_basic_state\n>> stop and refuse to proceed?\n>\n> Since we unconditionally write those three (and 'quiet'), it seems\n> reasonable to require all of them to be there when continuing, so I\n> think you're right that we should fail fast.\n>\n>> The way the && cascade is used seems to indicate that, but up to the\n>> point where it sents $verbose. If and only if head-name, onto, orig-head\n>> and quiet can be read in state-dir, verbose in state-dir is checked\n>> and only then $verbose is set.\n>>\n>> Martin, this seems to be from your series around early Feburary\n>> 2011.  Do you recall why these checks are cascaded this way?\n>> I do not offhand think of a good reason.\n>\n> Neither do I. I think the cascading after 'quiet' is just a mistake on\n> my part. The consequences are probably close to none, since if one of\n> earlier commands fail, the other files will probably not be there\n> either. (Not defending it; I'm happy if it gets fixed, e.g. by making\n> it fail fast.)\n\nI think this is probably the right thing to do, if we want to honor\nthe original intention of the earlier part of && cascade.  Everything\nbefore this new \"|| die\" reads from a file that should always exist\n(e.g. even when not asked to be quiet, that state is not signaled by\nthe lack of $state_dir/quiet, but by having an empty string in it),\nwhile everything after check optional state variable files (e.g. if\n$state_dir/verbose does not exist, it is not an error, but signals\nthat the user did not ask us to be verbose).\n\nNote that applying this patch _could_ uncover latent bug that was\nmasked by the lack of \"die\" here (maybe later codepath may depended\non not having orig_head at all and the only observable effect was\nthat in such a case, both quiet and verbose were silently ignored,\nbecause the control did not reach the GIT_QUIET=... and verbose=t\nassignments.\n\n git-rebase.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex d0c11a9..90506ba 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -95,7 +95,9 @@ read_basic_state () {\n \telse\n \t\torig_head=$(cat \"$state_dir\"/head)\n \tfi &&\n-\tGIT_QUIET=$(cat \"$state_dir\"/quiet) &&\n+\tGIT_QUIET=$(cat \"$state_dir\"/quiet) ||\n+\tdie \"failed to read basic rebase state from $state_dir\"\n+\n \ttest -f \"$state_dir\"/verbose && verbose=t\n \ttest -f \"$state_dir\"/strategy && strategy=\"$(cat \"$state_dir\"/strategy)\"\n \ttest -f \"$state_dir\"/strategy_opts &&\n"}]}