2.2.0-rc behavior changes (1/2)

2014-11-10 Thread Bryan Turner
I've been running a test suite we use to verify Git behaviors across
versions, and the 2.2.0 RCs (0 and 1 both) have a couple of small
behavioral differences. I'm sending them in separate e-mails just to
make the contents easier to grok.

Important: It's entirely possible neither of these is a _bug_; they
may both be intentional changes in behavior.

First change: git update-ref -d /refs/heads/nonexistent
some-valid-sha1 now produces an error about ref locking that it
didn't produce before

Git 2.1.x and prior produced this output:
error: unable to resolve reference refs/heads/nonexistent: No such
file or directory

Now, in the 2.2.0 RCs, it says:
error: unable to resolve reference refs/heads/nonexistent: No such
file or directory
error: Cannot lock the ref 'refs/heads/nonexistent'.

This one feels more like a bug, but again may not be. I say it feels
like a bug because of the order of the messages: If git has decided
the ref doesn't exist, why is it still trying to lock it?

This change bisects to:

bturner@felurian:~/Development/git/git$ git bisect bad
7521cc4611a783f4a8174bd0fcec5f4a47357ac1 is the first bad commit
commit 7521cc4611a783f4a8174bd0fcec5f4a47357ac1
Author: Ronnie Sahlberg sahlb...@google.com
Date:   Wed Apr 30 09:22:45 2014 -0700

refs.c: make delete_ref use a transaction

Best regards,
Bryan Turner
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: 2.2.0-rc behavior changes (1/2)

2014-11-10 Thread Jeff King
On Mon, Nov 10, 2014 at 07:47:32PM +1100, Bryan Turner wrote:

 First change: git update-ref -d /refs/heads/nonexistent
 some-valid-sha1 now produces an error about ref locking that it
 didn't produce before
 
 Git 2.1.x and prior produced this output:
 error: unable to resolve reference refs/heads/nonexistent: No such
 file or directory
 
 Now, in the 2.2.0 RCs, it says:
 error: unable to resolve reference refs/heads/nonexistent: No such
 file or directory
 error: Cannot lock the ref 'refs/heads/nonexistent'.
 
 This one feels more like a bug, but again may not be. I say it feels
 like a bug because of the order of the messages: If git has decided
 the ref doesn't exist, why is it still trying to lock it?

I don't think this is a bug. The order you see is because the code goes
something like this:

  1. the parent function calls a sub-function to lock

  2. the sub-function generates the error no such file or directory
 and returns failure to the caller

  3. the caller reports that acquiring the lock failed

The only thing that has changed between the two is step (3), but it is
not an extra lock action after the error. It is just a more verbose
report of the same error.

That being said, the sub-function (lock_ref_sha1_basic) gives a much
more useful message. So it would be a nice enhancement to make sure that
it prints something useful in every return case, and then drop the
message from the caller.

As an aside, I'm also slightly confused by your output. Are you feeding
/refs/heads/nonexistent (with a leading slash), or
refs/heads/nonexistent (no leading slash)? If the latter, then that
should silently succeed (and seems to in my tests). If the former, then
the real problem is not ENOENT, but rather EINVAL; that name is not a
valid refname.

Older versions of git would produce:

  error: unable to resolve reference /refs/heads/nonexistent: No such file or 
directory

which is like the error you showed, but note that the refname is
reported with the leading slash. In v2.2.0-rc1, this is:

  error: unable to resolve reference /refs/heads/nonexistent: Invalid argument
  error: Cannot lock the ref '/refs/heads/nonexistent'.

which is more accurate. I could explain the differences in our output
from some simple transcription errors when writing your email, but I
wanted to make sure I am not missing something.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: 2.2.0-rc behavior changes (1/2)

2014-11-10 Thread Bryan Turner
On Mon, Nov 10, 2014 at 8:22 PM, Jeff King p...@peff.net wrote:
 On Mon, Nov 10, 2014 at 07:47:32PM +1100, Bryan Turner wrote:

 First change: git update-ref -d /refs/heads/nonexistent
 some-valid-sha1 now produces an error about ref locking that it
 didn't produce before

 Git 2.1.x and prior produced this output:
 error: unable to resolve reference refs/heads/nonexistent: No such
 file or directory

 Now, in the 2.2.0 RCs, it says:
 error: unable to resolve reference refs/heads/nonexistent: No such
 file or directory
 error: Cannot lock the ref 'refs/heads/nonexistent'.

 This one feels more like a bug, but again may not be. I say it feels
 like a bug because of the order of the messages: If git has decided
 the ref doesn't exist, why is it still trying to lock it?

 I don't think this is a bug. The order you see is because the code goes
 something like this:

   1. the parent function calls a sub-function to lock

   2. the sub-function generates the error no such file or directory
  and returns failure to the caller

   3. the caller reports that acquiring the lock failed

 The only thing that has changed between the two is step (3), but it is
 not an extra lock action after the error. It is just a more verbose
 report of the same error.

 That being said, the sub-function (lock_ref_sha1_basic) gives a much
 more useful message. So it would be a nice enhancement to make sure that
 it prints something useful in every return case, and then drop the
 message from the caller.

 As an aside, I'm also slightly confused by your output. Are you feeding
 /refs/heads/nonexistent (with a leading slash), or
 refs/heads/nonexistent (no leading slash)? If the latter, then that
 should silently succeed (and seems to in my tests). If the former, then
 the real problem is not ENOENT, but rather EINVAL; that name is not a
 valid refname.

 Older versions of git would produce:

   error: unable to resolve reference /refs/heads/nonexistent: No such file or 
 directory

 which is like the error you showed, but note that the refname is
 reported with the leading slash. In v2.2.0-rc1, this is:

   error: unable to resolve reference /refs/heads/nonexistent: Invalid argument
   error: Cannot lock the ref '/refs/heads/nonexistent'.

 which is more accurate. I could explain the differences in our output
 from some simple transcription errors when writing your email, but I
 wanted to make sure I am not missing something.

Sorry, no, you're not missing anything. That is indeed a transcription
error from my e-mail. The test in question is using
refs/heads/nonexistent.

Thanks for the quick response, Jeff. With the sub-function the
ordering of the messages makes perfect sense.


 -Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html