{"thread":{"id":"13709","subject":"[PATCH] bisect: use \"$GIT_DIR/BISECT_START\" to check if we are bisecting","startedAt":"2008-05-28T16:57:02Z","lastAt":"2008-05-29T04:28:24Z","messageCount":4,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"77951","messageId":"20080528185702.4dcd2955.chriscool@tuxfamily.org","threadId":"13709","inReplyTo":null,"subject":"[PATCH] bisect: use \"$GIT_DIR/BISECT_START\" to check if we are bisecting","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-05-28T16:57:02Z","receivedAt":"2008-05-28T16:57:02Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"It seems simpler and safer to use the BISECT_START file everywhere\nto decide if we are bisecting or not, instead of using it in some\nplaces and BISECT_NAMES in other places.\n\nIn commit 6459c7c6786aa9bda0c7a095c9db66c36da0e5f0 (Nov 18 2007,\nBisect: use \"$GIT_DIR/BISECT_NAMES\" to check if we are bisecting.),\nwe decided to use BISECT_NAMES but code changed a lot and we now\nhave to check BISECT_START first in the \"bisect_start\" function\nanyway.\n\nThis patch also makes things a little bit safer by creating\nthe BISECT_START file first and deleting it last, and also by\nadding checks in \"bisect_clean_state\".\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n git-bisect.sh |   29 +++++++++++++----------------\n 1 files changed, 13 insertions(+), 16 deletions(-)\n\n\tThis is a cleanup to add on top of the detached HEAD series.\n\tThanks.\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 4bcbace..991b2ef 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -44,7 +44,7 @@ sq() {\n }\n \n bisect_autostart() {\n-\ttest -f \"$GIT_DIR/BISECT_NAMES\" || {\n+\ttest -s \"$GIT_DIR/BISECT_START\" || {\n \t\techo >&2 'You need to start by \"git bisect start\"'\n \t\tif test -t 0\n \t\tthen\n@@ -98,7 +98,7 @@ bisect_start() {\n \t#\n \t# Get rid of any old bisect state.\n \t#\n-\tbisect_clean_state\n+\tbisect_clean_state || exit\n \n \t#\n \t# Check for one bad and then some good revisions.\n@@ -146,8 +146,8 @@ bisect_start() {\n \t#\n \t# Write new start state.\n \t#\n-\tsq \"$@\" >\"$GIT_DIR/BISECT_NAMES\" &&\n \techo \"$start_head\" >\"$GIT_DIR/BISECT_START\" &&\n+\tsq \"$@\" >\"$GIT_DIR/BISECT_NAMES\" &&\n \teval \"$eval\" &&\n \techo \"git-bisect start$orig_args\" >>\"$GIT_DIR/BISECT_LOG\" || exit\n \t#\n@@ -226,7 +226,7 @@ bisect_next_check() {\n \t\t;;\n \t*)\n \t\tTHEN=''\n-\t\ttest -f \"$GIT_DIR/BISECT_NAMES\" || {\n+\t\ttest -s \"$GIT_DIR/BISECT_START\" || {\n \t\t\techo >&2 'You need to start by \"git bisect start\".'\n \t\t\tTHEN='then '\n \t\t}\n@@ -392,16 +392,12 @@ bisect_visualize() {\n }\n \n bisect_reset() {\n-\ttest -f \"$GIT_DIR/BISECT_NAMES\" || {\n+\ttest -s \"$GIT_DIR/BISECT_START\" || {\n \t\techo \"We are not bisecting.\"\n \t\treturn\n \t}\n \tcase \"$#\" in\n-\t0) if [ -s \"$GIT_DIR/BISECT_START\" ]; then\n-\t       branch=`cat \"$GIT_DIR/BISECT_START\"`\n-\t   else\n-\t       branch=master\n-\t   fi ;;\n+\t0) branch=$(cat \"$GIT_DIR/BISECT_START\") ;;\n \t1) git show-ref --verify --quiet -- \"refs/heads/$1\" ||\n \t       die \"$1 does not seem to be a valid branch\"\n \t   branch=\"$1\" ;;\n@@ -416,14 +412,15 @@ bisect_clean_state() {\n \tgit for-each-ref --format='%(refname) %(objectname)' refs/bisect/\\* |\n \twhile read ref hash\n \tdo\n-\t\tgit update-ref -d $ref $hash\n+\t\tgit update-ref -d $ref $hash || exit\n \tdone\n-\trm -f \"$GIT_DIR/BISECT_START\"\n-\trm -f \"$GIT_DIR/BISECT_LOG\"\n-\trm -f \"$GIT_DIR/BISECT_NAMES\"\n-\trm -f \"$GIT_DIR/BISECT_RUN\"\n+\trm -f \"$GIT_DIR/BISECT_LOG\" &&\n+\trm -f \"$GIT_DIR/BISECT_NAMES\" &&\n+\trm -f \"$GIT_DIR/BISECT_RUN\" &&\n \t# Cleanup head-name if it got left by an old version of git-bisect\n-\trm -f \"$GIT_DIR/head-name\"\n+\trm -f \"$GIT_DIR/head-name\" &&\n+\n+\trm -f \"$GIT_DIR/BISECT_START\"\n }\n \n bisect_replay () {\n-- \n1.5.5.1.580.g40a12.dirty\n"},{"id":"77955","messageId":"7vmymauzqg.fsf@gitster.siamese.dyndns.org","threadId":"13709","inReplyTo":"20080528185702.4dcd2955.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: use \"$GIT_DIR/BISECT_START\" to check if we are bisecting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-28T17:04:23Z","receivedAt":"2008-05-28T17:04:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> It seems simpler and safer to use the BISECT_START file everywhere\n> to decide if we are bisecting or not, instead of using it in some\n> places and BISECT_NAMES in other places.\n>\n> In commit 6459c7c6786aa9bda0c7a095c9db66c36da0e5f0 (Nov 18 2007,\n> Bisect: use \"$GIT_DIR/BISECT_NAMES\" to check if we are bisecting.),\n> we decided to use BISECT_NAMES but code changed a lot and we now\n> have to check BISECT_START first in the \"bisect_start\" function\n> anyway.\n>\n> This patch also makes things a little bit safer by creating\n> the BISECT_START file first and deleting it last, and also by\n> adding checks in \"bisect_clean_state\".\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n\nWhat's the breakage scenario that this patch fixes?\n\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 4bcbace..991b2ef 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -44,7 +44,7 @@ sq() {\n>  }\n>  \n>  bisect_autostart() {\n> -\ttest -f \"$GIT_DIR/BISECT_NAMES\" || {\n> +\ttest -s \"$GIT_DIR/BISECT_START\" || {\n\nThe reason you ignore an existing but empty BISECT_START file is...?\n"},{"id":"78011","messageId":"200805290601.19067.chriscool@tuxfamily.org","threadId":"13709","inReplyTo":"7vmymauzqg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bisect: use \"$GIT_DIR/BISECT_START\" to check if we are bisecting","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-05-29T04:01:18Z","receivedAt":"2008-05-29T04:01:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le mercredi 28 mai 2008, Junio C Hamano a écrit :\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> > It seems simpler and safer to use the BISECT_START file everywhere\n> > to decide if we are bisecting or not, instead of using it in some\n> > places and BISECT_NAMES in other places.\n> >\n> > In commit 6459c7c6786aa9bda0c7a095c9db66c36da0e5f0 (Nov 18 2007,\n> > Bisect: use \"$GIT_DIR/BISECT_NAMES\" to check if we are bisecting.),\n> > we decided to use BISECT_NAMES but code changed a lot and we now\n> > have to check BISECT_START first in the \"bisect_start\" function\n> > anyway.\n> >\n> > This patch also makes things a little bit safer by creating\n> > the BISECT_START file first and deleting it last, and also by\n> > adding checks in \"bisect_clean_state\".\n> >\n> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n>\n> What's the breakage scenario that this patch fixes?\n\nBefore this patch, in \"bisect_clean_state\" we removed the BISECT_START file \nbefore the other files, so for example if the process is killed after \nhaving removed this file but not the others, then we are in an inconsistent \nstate.\n\nIn this inconsistent state, if \"git bisect reset\" is called (perhaps again), \nthen it would checkout the \"master\" branch (because the BISECT_START file \ndoes not exists, but the BISECT_NAMES file still exists).\n\n> > diff --git a/git-bisect.sh b/git-bisect.sh\n> > index 4bcbace..991b2ef 100755\n> > --- a/git-bisect.sh\n> > +++ b/git-bisect.sh\n> > @@ -44,7 +44,7 @@ sq() {\n> >  }\n> >\n> >  bisect_autostart() {\n> > -\ttest -f \"$GIT_DIR/BISECT_NAMES\" || {\n> > +\ttest -s \"$GIT_DIR/BISECT_START\" || {\n>\n> The reason you ignore an existing but empty BISECT_START file is...?\n\n... that it should not happen, because this file is only written \nin \"bisect_start\" and there its content comes either from the current HEAD \nor from a previous not empty BISECT_START file.\n\nWe might add a check for an empty BISECT_START file and warn in this case \nthat the file may have been corrupted, but that may be for another patch.\n\nThanks,\nChristian.\n"},{"id":"78014","messageId":"7vd4n5raxj.fsf@gitster.siamese.dyndns.org","threadId":"13709","inReplyTo":"200805290601.19067.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: use \"$GIT_DIR/BISECT_START\" to check if we are bisecting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-29T04:28:24Z","receivedAt":"2008-05-29T04:28:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n>> The reason you ignore an existing but empty BISECT_START file is...?\n>\n> ... that it should not happen, because this file is only written \n> in \"bisect_start\" and there its content comes either from the current HEAD \n> or from a previous not empty BISECT_START file.\n>\n> We might add a check for an empty BISECT_START file and warn in this case \n> that the file may have been corrupted, but that may be for another patch.\n\nMy question was more about \"why are you acting as if distinction between\nempty and nonempty BISECT_START matters?\"\n\nBecause you \"echo $start_head >$GIT_DIR/BISECT_START\", this file will\nalways at least have a terminating LF even when the head is detached.\nIOW, if it exists, it will never be empty, I think.\n"}]}