patcht1402: check to delete broken refs

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(+)

Hi,

Stefan Beller wrote:

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

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?

On Tue, Nov 25, 2014 at 4:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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)

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

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(+)

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:

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

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

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.

On 11/25/2014 11:56 PM, Stefan Beller wrote:

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

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 

 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(+)