{"thread":{"id":"30125","subject":"Bug: bad errno report from lock_ref_sha1_basic() when contents bad","startedAt":"2012-04-02T10:04:29Z","lastAt":"2012-04-02T18:12:05Z","messageCount":3,"participants":["Thomas Rast","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"188302","messageId":"878viephzm.fsf@thomas.inf.ethz.ch","threadId":"30125","inReplyTo":null,"subject":"Bug: bad errno report from lock_ref_sha1_basic() when contents bad","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-04-02T10:04:29Z","receivedAt":"2012-04-02T10:04:29Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Hi Junio,\n\nUser \"mbrochh\" on IRC reported a result along the lines of\n\n  error: unable to resolve reference refs/remotes/origin/master: Success\n  ! [new branch] master -> origin/master (unable to update local ref)\n\nfrom 'git fetch' (or pull actually, but clearly it's the fetch part\nfailing).  This error message is from lock_ref_sha1_basic(), and\nsuggests a confused error exit taken with errno=0.\n\nI can reproduce the problem by writing a bit of garbage to a remote ref\nand then fetching, though I usually get some other error in the message\n(usually ENOENT, which is equally wrong).\n\nAs far as I can tell it's caused by resolve_ref_unsafe() returning NULL\nwhen the content checks fail:\n\n\tif (get_sha1_hex(buffer, sha1) || (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n\t\tif (flag)\n\t\t\t*flag |= REF_ISBROKEN;\n\t\treturn NULL;\n\t}\n\nThis used to warn before your 5595635 (resolve_ref(): report breakage to\nthe caller without warning, 2011-10-19), but otherwise suffered from the\nsame problem.\n\nAs far as I can see lock_ref_sha1_basic() needs a similar check for\nREF_ISBROKEN (and presumably the only fix is to nuke it so maybe it\nshould even suggest that to the user).  But in general, the \"errno may\nbe crucial or worthless\" interface of lock_ref_sha1_basic() seems a bit\nbroken...\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"188334","messageId":"7vhax2ghy1.fsf@alter.siamese.dyndns.org","threadId":"30125","inReplyTo":"878viephzm.fsf@thomas.inf.ethz.ch","subject":"Re: Bug: bad errno report from lock_ref_sha1_basic() when contents bad","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-02T17:30:14Z","receivedAt":"2012-04-02T17:30:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@inf.ethz.ch> writes:\n\n> As far as I can see lock_ref_sha1_basic() needs a similar check for\n> REF_ISBROKEN (and presumably the only fix is to nuke it so maybe it\n> should even suggest that to the user).\n\nProbably.\n\n> But in general, the \"errno may\n> be crucial or worthless\" interface of lock_ref_sha1_basic() seems a bit\n> broken...\n\nSorry, but I am not sure what you mean by this.  The function is fairly\ncareful using errno to base its decision and even saves errno when it\ncalls helpers that may overwrite it, so the \"interface\" does not want \"may\nbe crucial or worthless\".  The implementation may have been broken over\ntime by many patches, but that is a different issue.  Which codepaths\nclobber errno, and is it something you can easily fix with a patch or two?\n"},{"id":"188335","messageId":"87bonam2a2.fsf@thomas.inf.ethz.ch","threadId":"30125","inReplyTo":"7vhax2ghy1.fsf@alter.siamese.dyndns.org","subject":"Re: Bug: bad errno report from lock_ref_sha1_basic() when contents bad","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-04-02T18:12:05Z","receivedAt":"2012-04-02T18:12:05Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thomas Rast <trast@inf.ethz.ch> writes:\n>\n>> But in general, the \"errno may\n>> be crucial or worthless\" interface of lock_ref_sha1_basic() seems a bit\n>> broken...\n>\n> Sorry, but I am not sure what you mean by this.  The function is fairly\n> careful using errno to base its decision and even saves errno when it\n> calls helpers that may overwrite it, so the \"interface\" does not want \"may\n> be crucial or worthless\".  The implementation may have been broken over\n> time by many patches, but that is a different issue.  Which codepaths\n> clobber errno, and is it something you can easily fix with a patch or two?\n\nI meant something else, sorry for being so unclear and confused.  In\nparticular my comment was supposed to be about resolve_ref_unsafe().\n\nIt has two different failure modes:\n\n* Some FS syscall went wrong, and errno is crucial for the user to debug\n  it (e.g., ENOENT).\n\n* Reading went fine, but we failed to parse the resulting buffer.  errno\n  has some unknown value.\n\nThe user of the function needs to be aware of this distinction, and\nhandle it accordingly.  Otherwise it risks emitting outright wrong error\nmessages, e.g., in my case I got\n\n  thomas@thomas:~/g(next u+107)$ echo garbage >.git/refs/remotes/origin/master\n  thomas@thomas:~/g(next u+107)$ g fetch\n  error: unable to resolve reference refs/remotes/origin/master: No such file or directory\n  From git://git.kernel.org/pub/scm/git/git\n   ! [new branch]      master     -> origin/master  (unable to update local ref)\n\n(The IRC user who asked about this was lucky because \"Success\"\nimmediately told us that it was an error path not related to syscalls.)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}