[PATCH] t1402: check to delete broken refs

[PATCH] t1402: check to delete broken refs

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 
 This was also part of the ongoing series from Ronnie.
 But I think the patch in this form is rather independant, 
 documenting the current state of "git branch -d", so it's 
 fine to have it in now.

 t/t1402-check-ref-format.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index e5dc62e..08af156 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -197,4 +197,12 @@ invalid_ref_normalized 'heads///foo.lock'
 invalid_ref_normalized 'foo.lock/bar'
 invalid_ref_normalized 'foo.lock///bar'
 
+test_expect_failure 'git branch -d can delete ref with broken sha1' '
+	echo "pointing nowhere" > .git/refs/heads/brokensha1 &&
+	test_when_finished "rm -f .git/refs/heads/brokensha1" &&
+	git branch -d brokensha1 &&
+	git branch >output &&
+	! grep -e "brokensha1" output
+'
+
 test_done
-- 
2.2.0.rc3

Re: [PATCH] t1402: check to delete broken refs

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> This was also part of the ongoing series from Ronnie.
> But I think the patch in this form is rather independant,
> documenting the current state of "git branch -d", so it's
> fine to have it in now.

Is there a patch adding the feature this patch describes that this
could be squashed into?

>  t/t1402-check-ref-format.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)

This doesn't have anything to do with check-ref-format --- it's about
how easy it is to recover from a repository with corrupt files in it.
Would it fit somewhere like t3200-branch.sh?

Re: [PATCH] t1402: check to delete broken refs

[Stefan Beller]

On Tue, Nov 25, 2014 at 4:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Stefan Beller wrote:
>
>> This was also part of the ongoing series from Ronnie.
>> But I think the patch in this form is rather independant,
>> documenting the current state of "git branch -d", so it's
>> fine to have it in now.
>
> Is there a patch adding the feature this patch describes that this
> could be squashed into?

I have extracted it actually from a patch, which it was part of.
I want to send as much uncontroversial stuff to the list, before I try another
round of the larger reflog transactions series.

For now this test just documents, how git branch is behaving differently
from its desired  behavior. So it is also a subtle proposal how the desired
behavior should look like. (Deleting broken branches without further
questioning, flags or other magic incantations)


>
>>  t/t1402-check-ref-format.sh | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>
> This doesn't have anything to do with check-ref-format --- it's about
> how easy it is to recover from a repository with corrupt files in it.
> Would it fit somewhere like t3200-branch.sh?

I'll look into placing this test in a more appropriate spot.

[PATCHv2] branch -d: test if we can delete broken refs

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Changes v1->v2
 * relocated the test from t1402 to t3200
 * reword the commit message title to fit in with similar commits touching 
   t/t3200-branch.sh 

 t/t3200-branch.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 432921b..fa7d7bd 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -163,6 +163,14 @@ test_expect_success 'git branch --list -d t should fail' '
 	test_path_is_missing .git/refs/heads/t
 '
 
+test_expect_failure 'git branch -d can delete ref with broken sha1' '
+	echo "pointing nowhere" > .git/refs/heads/brokensha1 &&
+	test_when_finished "rm -f .git/refs/heads/brokensha1" &&
+	git branch -d brokensha1 &&
+	git branch >output &&
+	! grep -e "brokensha1" output
+'
+
 test_expect_success 'git branch --column' '
 	COLUMNS=81 git branch --column=column >actual &&
 	cat >expected <<\EOF &&
-- 
2.2.0.rc3

Re: [PATCH] t1402: check to delete broken refs

[Torsten Bögershausen]

On 11/25/2014 11:56 PM, Stefan Beller wrote:
> From: Ronnie Sahlberg <sahlberg@google.com>
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>   
>   This was also part of the ongoing series from Ronnie.
>   But I think the patch in this form is rather independant,
>   documenting the current state of "git branch -d", so it's
>   fine to have it in now.
>
>   t/t1402-check-ref-format.sh | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
> index e5dc62e..08af156 100755
> --- a/t/t1402-check-ref-format.sh
> +++ b/t/t1402-check-ref-format.sh
> @@ -197,4 +197,12 @@ invalid_ref_normalized 'heads///foo.lock'
>   invalid_ref_normalized 'foo.lock/bar'
>   invalid_ref_normalized 'foo.lock///bar'
>   
> +test_expect_failure 'git branch -d can delete ref with broken sha1' '
> +	echo "pointing nowhere" > .git/refs/heads/brokensha1 &&
> +	test_when_finished "rm -f .git/refs/heads/brokensha1" &&
> +	git branch -d brokensha1 &&
> +	git branch >output &&
> +	! grep -e "brokensha1" output
> +'
Do we need grep -e here ?
It does not give us anything in the pattern we are using.

   -e PATTERN, --regexp=PATTERN
               Use  PATTERN  as  the pattern.  This can be used to 
specify multiple search patterns, or to protect a pattern beginning
               with a hyphen (-).  (-e is specified by POSIX.)

(And we do not need the "" either, there is no space in brokensha1)
We can simply use:
! grep brokensha1 output





> +
>   test_done

Re: [PATCHv2] branch -d: test if we can delete broken refs

[Michael Haggerty]

Aside from one tiny formatting nit (see below), the test looks good to
me. On the other hand, this is kind of an "aspirational test"; I don't
know that the tested functionality has ever worked or that anybody has
ever claimed that it works. So my feeling is that the addition of the
test would feel more natural in the patch series that implements the new
feature. But I don't feel strongly about it.

Michael

On 11/26/2014 01:59 AM, Stefan Beller wrote:
> From: Ronnie Sahlberg <sahlberg@google.com>
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> Changes v1->v2
>  * relocated the test from t1402 to t3200
>  * reword the commit message title to fit in with similar commits touching 
>    t/t3200-branch.sh 
> 
>  t/t3200-branch.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 432921b..fa7d7bd 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -163,6 +163,14 @@ test_expect_success 'git branch --list -d t should fail' '
>  	test_path_is_missing .git/refs/heads/t
>  '
>  
> +test_expect_failure 'git branch -d can delete ref with broken sha1' '
> +	echo "pointing nowhere" > .git/refs/heads/brokensha1 &&

Please no space between the ">" and the filename.

> +	test_when_finished "rm -f .git/refs/heads/brokensha1" &&
> +	git branch -d brokensha1 &&
> +	git branch >output &&
> +	! grep -e "brokensha1" output
> +'
> +
>  test_expect_success 'git branch --column' '
>  	COLUMNS=81 git branch --column=column >actual &&
>  	cat >expected <<\EOF &&
> 

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCHv3] branch -d: test if we can delete broken refs

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Changes v1->v2
 * relocated the test from t1402 to t3200
 * reword the commit message title to fit in with similar commits touching 
   t/t3200-branch.sh 

Changes v2 -> v3
 * remove space nit pointed out by Michael
 * simplified grep expression as pointed out by Torsten
 
 I don't mind applying this one later as part of the series, where we actually
 introduce the feature. Thanks for all the feedback! 

Michael writes 
> On the other hand, this is kind of an "aspirational test"; I don't
> know that the tested functionality has ever worked or that anybody has
> ever claimed that it works. So my feeling is that the addition of the
> test would feel more natural in the patch series that implements the new
> feature. But I don't feel strongly about it.

 I don't have a strong oppinion on this one either. It's just my impression, that
 small single patches are digested easier by the mailing list rather than large series.
 So as the feedback is in, I'll put this patch at the end of the series, where the actual
 feature will be introduced.
 

 t/t3200-branch.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 432921b..fa7d7bd 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -163,6 +163,14 @@ test_expect_success 'git branch --list -d t should fail' '
 	test_path_is_missing .git/refs/heads/t
 '
 
+test_expect_failure 'git branch -d can delete ref with broken sha1' '
+	echo "pointing nowhere" >.git/refs/heads/brokensha1 &&
+	test_when_finished "rm -f .git/refs/heads/brokensha1" &&
+	git branch -d brokensha1 &&
+	git branch >output &&
+	! grep brokensha1 output
+'
+
 test_expect_success 'git branch --column' '
 	COLUMNS=81 git branch --column=column >actual &&
 	cat >expected <<\EOF &&
-- 
2.2.0.rc3

Re: [PATCHv2] branch -d: test if we can delete broken refs

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> ... On the other hand, this is kind of an "aspirational test"; I don't
> know that the tested functionality has ever worked or that anybody has
> ever claimed that it works. So my feeling is that the addition of the
> test would feel more natural in the patch series that implements the new
> feature. But I don't feel strongly about it.

I share your feeling.  

In the "aspiration followed by realization" pattern, the realization
commit shows a change in t/ hierarchy that turns test_expect_failure
into test_expect_success and it is likely that what is being tested
will fall outside the context.  Unless the test title is phrased
very well, it would not be obvious from the patch text to the t/
hierarchy alone what behaviour is being corrected when looking at
the realization commit.

If aspiration and realization are in a same series, that would not
be a problem, but it is if the commits that add "aspirational test"
and "realization" are too far apart.

If it is pretty clear to everybody that another topic to realize the
aspiration will be coming in a not so distant future, I think it is
fine, though.