[PATCH v2] Bisect reset: remove bisect refs that may have been packed.

[PATCH v2] Bisect reset: remove bisect refs that may have been packed.

[Christian Couder]

If refs were ever packed in the middle of bisection, the bisect
refs were not removed from the "packed-refs" file.

This patch fixes this problem by using "git update-ref -d $ref $hash"
in "bisect_clean_state".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-bisect.sh               |   11 ++++++++++-
 t/t6030-bisect-porcelain.sh |   12 ++++++++++++
 2 files changed, 22 insertions(+), 1 deletions(-)

	Ooops, there was a problem with the previous patch
	if "git bisect reset" was used when not bisecting.

	Sorry.

diff --git a/git-bisect.sh b/git-bisect.sh
index 1ed44e5..584906f 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -351,7 +351,16 @@ bisect_reset() {
 
 bisect_clean_state() {
 	rm -fr "$GIT_DIR/refs/bisect"
-	rm -f "$GIT_DIR/refs/heads/bisect"
+
+	# There may be some refs packed during bisection.
+	git for-each-ref --format='%(refname) %(objectname)' \
+		"refs/bisect/*" | while read ref hash
+	do
+		git update-ref -d $ref $hash
+	done
+
+	hash=$(git show-ref --hash refs/heads/bisect)
+	test -n "$hash" && git update-ref -d refs/heads/bisect $hash
 	rm -f "$GIT_DIR/BISECT_LOG"
 	rm -f "$GIT_DIR/BISECT_NAMES"
 	rm -f "$GIT_DIR/BISECT_RUN"
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 53956c0..f09db62 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -71,6 +71,18 @@ test_expect_success 'bisect start with one bad and good' '
 	git bisect next
 '
 
+test_expect_success 'bisect reset removes packed refs' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH3 &&
+	git pack-refs --all --prune &&
+	git bisect next &&
+	git bisect reset &&
+	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test -z "$(git for-each-ref "refs/heads/bisect")"
+'
+
 # $HASH1 is good, $HASH4 is bad, we skip $HASH3
 # but $HASH2 is bad,
 # so we should find $HASH2 as the first bad commit
-- 
1.5.3.5.722.g789fd-dirty

Re: [PATCH v2] Bisect reset: remove bisect refs that may have been packed.

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> If refs were ever packed in the middle of bisection, the bisect
> refs were not removed from the "packed-refs" file.
>
> This patch fixes this problem by using "git update-ref -d $ref $hash"
> in "bisect_clean_state".
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

Thanks.  Just a few nits.

On top of your patch...

 - You forgot to remove one "removal of filesystem refs";
 - for-each-ref takes more than one patterns.

 git-bisect.sh |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 584906f..21ed02f 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -350,17 +350,12 @@ bisect_reset() {
 }
 
 bisect_clean_state() {
-	rm -fr "$GIT_DIR/refs/bisect"
-
 	# There may be some refs packed during bisection.
-	git for-each-ref --format='%(refname) %(objectname)' \
-		"refs/bisect/*" | while read ref hash
+	git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* refs/heads/bisect |
+	while read ref hash
 	do
 		git update-ref -d $ref $hash
 	done
-
-	hash=$(git show-ref --hash refs/heads/bisect)
-	test -n "$hash" && git update-ref -d refs/heads/bisect $hash
 	rm -f "$GIT_DIR/BISECT_LOG"
 	rm -f "$GIT_DIR/BISECT_NAMES"
 	rm -f "$GIT_DIR/BISECT_RUN"

Re: [PATCH v2] Bisect reset: remove bisect refs that may have been packed.

[Johannes Sixt]

Junio C Hamano schrieb:
> +	git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* refs/heads/bisect |
> +	while read ref hash

If you also swap %(refname) and %(objectname), then this is also not prone 
to whitespace in refnames. (Yes, I know, there shouldn't be such, but...)

-- Hannes

Re: [PATCH v2] Bisect reset: remove bisect refs that may have been packed.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> If refs were ever packed in the middle of bisection, the bisect
>> refs were not removed from the "packed-refs" file.
>>
>> This patch fixes this problem by using "git update-ref -d $ref $hash"
>> in "bisect_clean_state".
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>
> Thanks.  Just a few nits.
>
> On top of your patch...
>
>  - You forgot to remove one "removal of filesystem refs";

Ooops.  'test -d "$GIT_DIR/refs/bisect"' is used as a signal
that we are bisecting for the rest of the code, so we cannot
lose that rm -fr there.

I think a longer term clean-up would be not to treat "bisect" as
a reserved branch name but use detached HEAD while bisecting.
But that is a larger topic.

Re: [PATCH v2] Bisect reset: remove bisect refs that may have been packed.

[Shawn O. Pearce]

Johannes Sixt <j.sixt@viscovery.net> wrote:
> Junio C Hamano schrieb:
> >+	git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* 
> >refs/heads/bisect |
> >+	while read ref hash
> 
> If you also swap %(refname) and %(objectname), then this is also not prone 
> to whitespace in refnames. (Yes, I know, there shouldn't be such, but...)

What's this obsession with whitespace in refnames?  Twice in like
two days people are talking about whitespace in refnames.

WHITESPACE IS NOT PERMITTED IN REFNAMES.

Do we need to apply the following patch, to keep people from creating
refs by hand with whitespace in them?  Is this really that common?

	git rev-parse HEAD >'.git/refs/heads/..i have spaces hah!'


diff --git a/refs.c b/refs.c
index aff02cd..b95bf83 100644
--- a/refs.c
+++ b/refs.c
@@ -246,6 +246,7 @@ static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
 			struct stat st;
 			int flag;
 			int namelen;
+			int check;
 
 			if (de->d_name[0] == '.')
 				continue;
@@ -261,6 +262,9 @@ static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
 				list = get_ref_dir(ref, list);
 				continue;
 			}
+			check = check_ref_format(ref);
+			if (check != 0 && check != -2)
+				continue;
 			if (!resolve_ref(ref, sha1, 1, &flag)) {
 				error("%s points nowhere!", ref);
 				continue;

-- 
Shawn.

Re: [PATCH v2] Bisect reset: remove bisect refs that may have been packed.

[Jeff King]

On Thu, Nov 15, 2007 at 03:49:34AM -0500, Shawn O. Pearce wrote:

> What's this obsession with whitespace in refnames?  Twice in like
> two days people are talking about whitespace in refnames.

I even had Junio convinced at one point!

I am not actually creating such refs, but I think my brain was still
fried from the URL encoding discussion, and I was overly paranoid about
spaces.

-Peff

[PATCH 1/3] git-bisect: war on "sed"

[Junio C Hamano]

We do not need to pipe "echo" to "sed" only to strip refs/heads/
from the beginning.  We are assuming not-so-ancient shells these
days.

Also there is no need to avoid assuming \012 is the LF; we do
not run on EBCDIC, sorry.  Other parts of the script already
uses tr to convert separator to LF that way.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-bisect.sh |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 46a7b8d..3a21033 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -71,7 +71,7 @@ bisect_start() {
 		;;
 	refs/heads/*)
 		[ -s "$GIT_DIR/head-name" ] && die "won't bisect on seeked tree"
-		echo "$head" | sed 's#^refs/heads/##' >"$GIT_DIR/head-name"
+		echo "${head#refs/heads/}" >"$GIT_DIR/head-name"
 		;;
 	*)
 		die "Bad HEAD - strange symbolic ref"
@@ -275,8 +275,7 @@ exit_if_skipped_commits () {
 	if expr "$_tried" : ".*[|].*" > /dev/null ; then
 		echo "There are only 'skip'ped commit left to test."
 		echo "The first bad commit could be any of:"
-		echo "$_tried" | sed -e 's/[|]/\
-/g'
+		echo "$_tried" | tr '[|]' '[\012]'
 		echo "We cannot bisect more!"
 		exit 2
 	fi
-- 
1.5.3.5.1780.gca2b

[PATCH 2/3] git-bisect: use update-ref to mark good/bad commits

[Junio C Hamano]

This removes the last instance of making a ref by hand with
"echo SHA1 >.git/refs/$refname" from the script and replaces it
with update-ref.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-bisect.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 3a21033..4b74a7b 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -130,7 +130,7 @@ bisect_write() {
 		good|skip)	tag="$state"-"$rev" ;;
 		*)		die "Bad bisect_write argument: $state" ;;
 	esac
-	echo "$rev" >"$GIT_DIR/refs/bisect/$tag"
+	git update-ref "refs/bisect/$tag" "$rev"
 	echo "# $state: "$(git show-branch $rev) >>"$GIT_DIR/BISECT_LOG"
 	test -z "$nolog" && echo "git-bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
 }
-- 
1.5.3.5.1780.gca2b

[PATCH 3/3] git-bisect: modernize branch shuffling hack

[Junio C Hamano]

When switching to a new rev, we first made "new-bisect" branch to
point at the chosen commit, attempt to switch to it, and then
finally renamed the new-bisect branch to bisect by hand when
successful.  This is so that we can catch checkout failure (your
local modification may interfere with switching to the chosen
version) without losing information on which commit the next
attempt should be made.

Rewrite it using a more modern form but without breaking the
safety.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-bisect.sh |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 4b74a7b..dae8a8e 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -316,10 +316,9 @@ bisect_next() {
 	exit_if_skipped_commits "$bisect_rev"
 
 	echo "Bisecting: $bisect_nr revisions left to test after this"
-	echo "$bisect_rev" >"$GIT_DIR/refs/heads/new-bisect"
+	git branch -f new-bisect "$bisect_rev"
 	git checkout -q new-bisect || exit
-	mv "$GIT_DIR/refs/heads/new-bisect" "$GIT_DIR/refs/heads/bisect" &&
-	GIT_DIR="$GIT_DIR" git symbolic-ref HEAD refs/heads/bisect
+	git branch -M new-bisect bisect
 	git show-branch "$bisect_rev"
 }
 
-- 
1.5.3.5.1780.gca2b

Re: [PATCH 1/3] git-bisect: war on "sed"

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
> We do not need to pipe "echo" to "sed" only to strip refs/heads/
> from the beginning.  We are assuming not-so-ancient shells these
> days.

What's wrong with sed?

-Miles

-- 
[|nurgle|]  ddt- demonic? so quake will have an evil kinda setting? one that
            will  make every christian in the world foamm at the mouth?
[iddt]      nurg, that's the goal

Re: [PATCH 1/3] git-bisect: war on "sed"

[Wincent Colaiuta]

El 15/11/2007, a las 10:29, Miles Bader escribió:

> Junio C Hamano <gitster@pobox.com> writes:
>> We do not need to pipe "echo" to "sed" only to strip refs/heads/
>> from the beginning.  We are assuming not-so-ancient shells these
>> days.
>
> What's wrong with sed?

Nothing, but using it means forking a new process unnecessarily, and  
the shorter form without sed is arguably more readable:

-		echo "$head" | sed 's#^refs/heads/##' >"$GIT_DIR/head-name"
+		echo "${head#refs/heads/}" >"$GIT_DIR/head-name"

Cheers,
Wincent

Re: [PATCH 1/3] git-bisect: war on "sed"

[Miles Bader]

Wincent Colaiuta <win@wincent.com> writes:
> the shorter form without sed is arguably more readable:
>
> -		echo "$head" | sed 's#^refs/heads/##' >"$GIT_DIR/head-name"
> +		echo "${head#refs/heads/}" >"$GIT_DIR/head-name"

Er, I suppose -- if you are acquainted with that particular shell
variable syntax (I suspect knowledge of sed is far more widespread).

[personally, I know that syntax has something to do with replacing
something with something else, but really haven't much clue other than
that, and I always _thought_ it was bash-specific and so avoided using
any of that stuff.]

-miles
-- 
People who are more than casually interested in computers should have at
least some idea of what the underlying hardware is like.  Otherwise the
programs they write will be pretty weird.  -- Donald Knuth

Re: [PATCH 1/3] git-bisect: war on "sed"

[Andreas Ericsson]

Miles Bader wrote:
> Wincent Colaiuta <win@wincent.com> writes:
>> the shorter form without sed is arguably more readable:
>>
>> -		echo "$head" | sed 's#^refs/heads/##' >"$GIT_DIR/head-name"
>> +		echo "${head#refs/heads/}" >"$GIT_DIR/head-name"
> 
> Er, I suppose -- if you are acquainted with that particular shell
> variable syntax (I suspect knowledge of sed is far more widespread).
> 
> [personally, I know that syntax has something to do with replacing
> something with something else, but really haven't much clue other than
> that, and I always _thought_ it was bash-specific and so avoided using
> any of that stuff.]
> 

It says "remove refs/heads/ from the beginning of the string pointed to
by $head".

It's not a bashism. Some extensions to that syntax are though (I think).
If you want to be sure of portability, use sed instead. git uses this
syntax often enough that it's worth using everywhere, but usually only
in porcelain commands which one can relatively safely assume are run on
at least decently up-to-date developer workstations.

You'll note that stuff that absolutely *has* to reside server-side are
entirely in C.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 1/3] git-bisect: war on "sed"

[David Kastrup]

Andreas Ericsson <ae@op5.se> writes:

>> Wincent Colaiuta <win@wincent.com> writes:
>>> +		echo "${head#refs/heads/}" >"$GIT_DIR/head-name"
>
> It says "remove refs/heads/ from the beginning of the string pointed
> to by $head".
>
> It's not a bashism. Some extensions to that syntax are though (I
> think).

General replace anywhere.

> If you want to be sure of portability, use sed instead.

Guffaw.

> git uses this syntax often enough that it's worth using everywhere,
> but usually only in porcelain commands which one can relatively
> safely assume are run on at least decently up-to-date developer
> workstations.

Huh?  It is used throughout.  That's why "make install" will install
xxx.sh scripts as xxx after possibly replacing the initial #!/bin/sh
line with a shell known to be reasonably conformant on a particular
system.

-- 
David Kastrup

Re: [PATCH 1/3] git-bisect: war on "sed"

[Miles Bader]

On Nov 15, 2007 7:09 PM, David Kastrup <dak@gnu.org> wrote:
> > Er, I suppose -- if you are acquainted with that particular shell
> > variable syntax (I suspect knowledge of sed is far more widespread).
>
> sed is rather bad with regard to portability.  If you take a look at
> the respective portability section in the autoconf file, it is almost
> longer than that of Bourne shells.

I don't think that's really a fair characterization -- almost all of
the autoconf sed portability caveats concern weird corner cases in the
dusty corners of the language (and some are "common" issues with other
tools, i.e., avoiding ^ and $ inside of regexp groups).

For typical straightforward usage, there seems little problem.

[The main portability problems I've actually _noticed_ with sed are
the "-e" versus ";" issue and what happens with "\n" in various
contexts...]

-Miles

-- 
Do not taunt Happy Fun Ball.

Re: [PATCH 1/3] git-bisect: war on "sed"

[David Kastrup]

"Miles Bader" <miles@gnu.org> writes:

> On Nov 15, 2007 7:09 PM, David Kastrup <dak@gnu.org> wrote:
>> > Er, I suppose -- if you are acquainted with that particular shell
>> > variable syntax (I suspect knowledge of sed is far more widespread).
>>
>> sed is rather bad with regard to portability.  If you take a look at
>> the respective portability section in the autoconf file, it is almost
>> longer than that of Bourne shells.
>
> I don't think that's really a fair characterization -- almost all of
> the autoconf sed portability caveats concern weird corner cases in the
> dusty corners of the language (and some are "common" issues with other
> tools, i.e., avoiding ^ and $ inside of regexp groups).
>
> For typical straightforward usage, there seems little problem.
>
> [The main portability problems I've actually _noticed_ with sed are
> the "-e" versus ";" issue and what happens with "\n" in various
> contexts...]

What about character classes containing the pattern delimiter, \+, \?,
\|, nested grouping, anchors in groups, * after groups?  That's all
rather straightforward usage.

-- 
David Kastrup

Re: [PATCH v2] Bisect reset: remove bisect refs that may have been packed.

[Johannes Schindelin]

Hi,

On Thu, 15 Nov 2007, Jeff King wrote:

> On Thu, Nov 15, 2007 at 03:49:34AM -0500, Shawn O. Pearce wrote:
> 
> > What's this obsession with whitespace in refnames?  Twice in like
> > two days people are talking about whitespace in refnames.
> 
> I even had Junio convinced at one point!
> 
> I am not actually creating such refs, but I think my brain was still 
> fried from the URL encoding discussion, and I was overly paranoid about 
> spaces.

Yeah, I'm really sorry I started that.  It was meant to do good, but did 
bad.  Oh, well.  The road to hell is paved with good intentions.

Ciao,
Dscho

Re: [PATCH 1/3] git-bisect: war on "sed"

[Miles Bader]

David Kastrup <dak@gnu.org> writes:
>> For typical straightforward usage, there seems little problem.
>>
>> [The main portability problems I've actually _noticed_ with sed are
>> the "-e" versus ";" issue and what happens with "\n" in various
>> contexts...]
>
> What about character classes containing the pattern delimiter, \+, \?,
> \|, nested grouping, anchors in groups, * after groups?  That's all
> rather straightforward usage.

You're right that the regexp stuff is not really "dusty corners", but
none of those affect typical sed usage I think -- most sed usage being
really rather simple (and the old "regexps differ between traditional
unix tools" issue tends to dampen enthusiasm for really complex regexps
with those tools).

I looked over the various random uses of sed I have locally (a couple of
hundred instances), and the only thing which would potentially affect
any of them would be the SVR3 thing about no nested groups (does anybody
actually care about SVR3 though?!?).

-Miles

-- 
Yo mama's so fat when she gets on an elevator it HAS to go down.