Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > On Tue, Nov 21, 2017 at 2:12 PM, Kaartic Sivaraam > <kaartic.sivaraam@xxxxxxxxx> wrote: >> On Wednesday 22 November 2017 12:08 AM, Eric Sunshine wrote: >>> The original code unconditionally uses "+ 11", which says that the >>> prefix is _always_ present. This commit message muddies the waters [...] >> >> That muddiness of that statement is a consequence of my recent encounter[1] >> in which the assumption (that the prefix(refs/heads/ always exists) of that >> code failed. I had a little suspicion, when I wrote that commit message, >> that there might be other cases in which assumption might fail. The issue >> has been resolved only in 3/4 of jc/branch-name-sanity but that was only >> after I wrote the commit message initially. So, it does make sense to >> remove that muddiness now. Thanks for noting that. >> >> [1]: Note the 'warning: ' message in the following mail. That ugly '|�?' was >> a consequence of the assumption that the 'prefix' always existed! >> https://public-inbox.org/git/1509209933.2256.4.camel@xxxxxxxxx/ > > Thanks for the explanation and history reference. I have a suspicion that it wasn't the case. The ugly uninitialized byte sequence came from an error codepath of a function that is asked to fill a strbuf with "refs/heads/$something" returning early when it found an error in the input, without realizing that the caller still looks at the strbuf even when it got an error. That caller-callee pair was updated. It is just a bug and +11 is always correct; passing the data that does not begin with "refs/heads/" there, including the case where an uninitialized buffer is passed, is simply a bug. In other words, skip_prefix() is a good change, but if we are to use it, we should also check the result and error out if we do not see "refs/heads/" there--you already bothered to spend extra cycles for that.
