fix git tests for git 1.8.5

fix git tests for git 1.8.5



https://code.launchpad.net/~natefinch/juju-core/030-fix-git-tests/+merge/201162

(do not edit description out of merge proposal)

[natefinch, 2014-01-10 11:11:40]

Please take a look.

[rog, 2014-01-10 11:27:27]

https://codereview.appspot.com/49470047/diff/1/worker/uniter/charm/git_test.go
File worker/uniter/charm/git_test.go (left):

https://codereview.appspot.com/49470047/diff/1/worker/uniter/charm/git_test.g...
worker/uniter/charm/git_test.go:165: err = repo.AddAll()
Why remove this?

https://codereview.appspot.com/49470047/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/49470047/diff/1/worker/uniter/uniter_test.go#n...
worker/uniter/uniter_test.go:1592: c.Assert(string(out), cmp, ".*?On branch
master\nnothing to commit.*?\n")
.*? is the same as .*

but I'd prefer:

(# )?

[natefinch, 2014-01-10 11:37:37]

On 2014/01/10 11:27:27, rog wrote:
> https://codereview.appspot.com/49470047/diff/1/worker/uniter/charm/git_test.go
> File worker/uniter/charm/git_test.go (left):
> 
>
https://codereview.appspot.com/49470047/diff/1/worker/uniter/charm/git_test.g...
> worker/uniter/charm/git_test.go:165: err = repo.AddAll()
> Why remove this?

1.) It doesn't actually do anything.  It's doing an add all immediately after a
clone, so we know there can't be anything to add in the directory by definition.
 I don't know why it was there in the tests, it seems like just some extraneous
lines in the test, possibly leftover from copy & paste from another test.

2.) It fails on git 1.8.5, precisely because there's nothing to add.  In earlier
versions of git, if you did git add -A ., it would just do nothing if there were
no files to add, but in 1.8.5 it gives you an error instead about pathspec . not
matching any files.


> https://codereview.appspot.com/49470047/diff/1/worker/uniter/uniter_test.go
> File worker/uniter/uniter_test.go (right):
> 
>
https://codereview.appspot.com/49470047/diff/1/worker/uniter/uniter_test.go#n...
> worker/uniter/uniter_test.go:1592: c.Assert(string(out), cmp, ".*?On branch
> master\nnothing to commit.*?\n")
> .*? is the same as .*
> 
> but I'd prefer:
> 
> (# )?

In this case, you're right that .* and .*? are the functionally same (the second
question mark at the end was actually left over from previous changes I had
backed out).  But in general .* and .*? are not the same.  .* is greedy, and
matches as much as possible, .*? matches as little as possible.  


Honestly, I intentionally made the opening of the regex as broad as possible,
because it seems unlikely that there would be text "On branch master nothing to
commit" in the middle that would *not* indicate the result we were looking for,
and this way if they decide to twiddle with the format in the front of the error
message again, we don't have to come back and change the regex *again*.

[rog, 2014-01-10 12:49:52]

LGTM with one possible reservation below.

On 10 January 2014 11:37,  <nate.finch@gmail.com> wrote:
> On 2014/01/10 11:27:27, rog wrote:
>
>
https://codereview.appspot.com/49470047/diff/1/worker/uniter/charm/git_test.go
>>
>> File worker/uniter/charm/git_test.go (left):
>
>
>
>
https://codereview.appspot.com/49470047/diff/1/worker/uniter/charm/git_test.g...
>>
>> worker/uniter/charm/git_test.go:165: err = repo.AddAll()
>> Why remove this?
>
>
> 1.) It doesn't actually do anything.  It's doing an add all immediately
> after a clone, so we know there can't be anything to add in the
> directory by definition.  I don't know why it was there in the tests, it
> seems like just some extraneous lines in the test, possibly leftover
> from copy & paste from another test.

Ah, that's good to know. Is this something we could fall foul of
in real code - for example if we're upgrading a charm but hasn't
actually made any changes?

>> master\nnothing to commit.*?\n")
>> .*? is the same as .*
>
>
>> but I'd prefer:
>
>
>> (# )?
>
>
> In this case, you're right that .* and .*? are the functionally same
> (the second question mark at the end was actually left over from
> previous changes I had backed out).  But in general .* and .*? are not
> the same.  .* is greedy, and matches as much as possible, .*? matches as
> little as possible.

OK, fair enough. And I'd forgotten about the non-greedy regexp extensions.
Please lose the unnecessary non-greedy operators, though - they
just leave the reader wondering if they're necessary for some reason.

[natefinch, 2014-01-10 15:42:38]

On 2014/01/10 12:49:52, rog wrote:
> LGTM with one possible reservation below.
> 
>
https://codereview.appspot.com/49470047/diff/1/worker/uniter/charm/git_test.g...
> >>
> >> worker/uniter/charm/git_test.go:165: err = repo.AddAll()
> >> Why remove this?
> >
> >
> > 1.) It doesn't actually do anything.  It's doing an add all immediately
> > after a clone, so we know there can't be anything to add in the
> > directory by definition.  I don't know why it was there in the tests, it
> > seems like just some extraneous lines in the test, possibly leftover
> > from copy & paste from another test.
> 
> Ah, that's good to know. Is this something we could fall foul of
> in real code - for example if we're upgrading a charm but hasn't
> actually made any changes?

In theory, yes.  If we were relying on this not to fail in that instance.  I'm
not sure if we can determine whether or not we do that.  In theory we could
write code to ignore the error in that case... but it's a little more
complicated than just checking the error that is returned - we'd have to check
ErrOut for the message, which is currently getting munged into CombinedOutput
with the stdout info.... really kinda of a pain in the butt, which is why I
decided to just remove the spurious AddAll.  Let me know if you want me to put
in the code to actively ignore the error, and I'll do so.

[rog, 2014-01-10 16:09:03]

On 10 January 2014 15:42,  <nate.finch@gmail.com> wrote:
> On 2014/01/10 12:49:52, rog wrote:
>>
>> LGTM with one possible reservation below.
>
>
>
>
https://codereview.appspot.com/49470047/diff/1/worker/uniter/charm/git_test.g...
>>
>> >>
>> >> worker/uniter/charm/git_test.go:165: err = repo.AddAll()
>> >> Why remove this?
>> >
>> >
>> > 1.) It doesn't actually do anything.  It's doing an add all
>
> immediately
>>
>> > after a clone, so we know there can't be anything to add in the
>> > directory by definition.  I don't know why it was there in the
>
> tests, it
>>
>> > seems like just some extraneous lines in the test, possibly leftover
>> > from copy & paste from another test.
>
>
>> Ah, that's good to know. Is this something we could fall foul of
>> in real code - for example if we're upgrading a charm but hasn't
>> actually made any changes?
>
>
> In theory, yes.  If we were relying on this not to fail in that
> instance.  I'm not sure if we can determine whether or not we do that.
> In theory we could write code to ignore the error in that case... but
> it's a little more complicated than just checking the error that is
> returned - we'd have to check ErrOut for the message, which is currently
> getting munged into CombinedOutput with the stdout info.... really kinda
> of a pain in the butt, which is why I decided to just remove the
> spurious AddAll.  Let me know if you want me to put in the code to
> actively ignore the error, and I'll do so.

I don't know - it seems like the test perhaps was not *entirely* spurious after
all. I realise that it might be a hassle to test for, but perhaps this
is a genuine
regression.

William, what do you think?

[natefinch, 2014-01-13 13:35:25]

Please take a look.

[fwereade, 2014-01-13 13:47:25]

LGTM, but I'd prefer a tighter match in uniter_test

https://codereview.appspot.com/49470047/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/49470047/diff/1/worker/uniter/uniter_test.go#n...
worker/uniter/uniter_test.go:1592: c.Assert(string(out), cmp, ".*?On branch
master\nnothing to commit.*?\n")
On 2014/01/10 11:27:28, rog wrote:
> .*? is the same as .*
> 
> but I'd prefer:
> 
> (# )?

+1000