Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Michael Haggerty]

On 10/07/2017 06:36 AM, Michael Haggerty wrote:
> On 10/06/2017 07:16 PM, Jeff King wrote:
>> On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:
>>
>>> I do have one twinge of uneasiness at a deeper level, that I haven't had
>>> time to check...
>>>
>>> Does this patch make it easier to *set* HEAD to an unborn branch that
>>> d/f conflicts with an existing reference? If so, that might be a
>>> slightly worse UI for users. I'd rather learn about such a problem when
>>> setting HEAD (when I am thinking about the new branch name and am in the
>>> frame of mind to solve the problem) rather than later, when I try to
>>> commit to the new branch.
>>
>> Good question. The answer is no, it's allowed both before and after my
>> patch. At least via git-symbolic-ref.
>>
>> I agree it would be nice to know earlier for such a case. For
>> symbolic-ref, we probably should allow it, because it's plumbing that
>> may be used for tricky things. For things like "checkout -b", you'd
>> generally get a timely warning as we try to create the ref.
>>
>> The odd man out is "checkout --orphan", which leaves the branch unborn.
>> It might be nice if it did a manual check that the ref is available (and
>> also that it's syntactically acceptable, though I think we may do that
>> already).
>>
>> But all of that is orthogonal to this fix, I think.
> 
> Thanks for checking. Yes, I totally agree that this is orthogonal.

I also just checked but there don't seem to be any docstrings that need
updating.

Reviewed-by: Michael Haggerty 

(both patches in this series).

Michael

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Michael Haggerty]

On 10/06/2017 07:16 PM, Jeff King wrote:
> On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:
> 
>> I do have one twinge of uneasiness at a deeper level, that I haven't had
>> time to check...
>>
>> Does this patch make it easier to *set* HEAD to an unborn branch that
>> d/f conflicts with an existing reference? If so, that might be a
>> slightly worse UI for users. I'd rather learn about such a problem when
>> setting HEAD (when I am thinking about the new branch name and am in the
>> frame of mind to solve the problem) rather than later, when I try to
>> commit to the new branch.
> 
> Good question. The answer is no, it's allowed both before and after my
> patch. At least via git-symbolic-ref.
> 
> I agree it would be nice to know earlier for such a case. For
> symbolic-ref, we probably should allow it, because it's plumbing that
> may be used for tricky things. For things like "checkout -b", you'd
> generally get a timely warning as we try to create the ref.
> 
> The odd man out is "checkout --orphan", which leaves the branch unborn.
> It might be nice if it did a manual check that the ref is available (and
> also that it's syntactically acceptable, though I think we may do that
> already).
> 
> But all of that is orthogonal to this fix, I think.

Thanks for checking. Yes, I totally agree that this is orthogonal.

Michael

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Jeff King]

On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:

> I do have one twinge of uneasiness at a deeper level, that I haven't had
> time to check...
> 
> Does this patch make it easier to *set* HEAD to an unborn branch that
> d/f conflicts with an existing reference? If so, that might be a
> slightly worse UI for users. I'd rather learn about such a problem when
> setting HEAD (when I am thinking about the new branch name and am in the
> frame of mind to solve the problem) rather than later, when I try to
> commit to the new branch.

Good question. The answer is no, it's allowed both before and after my
patch. At least via git-symbolic-ref.

I agree it would be nice to know earlier for such a case. For
symbolic-ref, we probably should allow it, because it's plumbing that
may be used for tricky things. For things like "checkout -b", you'd
generally get a timely warning as we try to create the ref.

The odd man out is "checkout --orphan", which leaves the branch unborn.
It might be nice if it did a manual check that the ref is available (and
also that it's syntactically acceptable, though I think we may do that
already).

But all of that is orthogonal to this fix, I think.

-Peff

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Michael Haggerty]

On 10/06/2017 04:42 PM, Jeff King wrote:
> If our call to refs_read_raw_ref() fails, we check errno to
> see if the ref is simply missing, or if we encountered a
> more serious error. If it's just missing, then in "write"
> mode (i.e., when RESOLVE_REFS_READING is not set), this is
> perfectly fine.
> 
> However, checking for ENOENT isn't sufficient to catch all
> missing-ref cases. In the filesystem backend, we may also
> see EISDIR when we try to resolve "a" and "a/b" exists.
> Likewise, we may see ENOTDIR if we try to resolve "a/b" and
> "a" exists. In both of those cases, we know that our
> resolved ref doesn't exist, but we return an error (rather
> than reporting the refname and returning a null sha1).
> 
> This has been broken for a long time, but nobody really
> noticed because the next step after resolving without the
> READING flag is usually to lock the ref and write it. But in
> both of those cases, the write will fail with the same
> errno due to the directory/file conflict.
> 
> There are two cases where we can notice this, though:
> 
>   1. If we try to write "a" and there's a leftover directory
>  already at "a", even though there is no ref "a/b". The
>  actual write is smart enough to move the empty "a" out
>  of the way.
> 
>  This is reasonably rare, if only because the writing
>  code has to do an independent resolution before trying
>  its write (because the actual update_ref() code handles
>  this case fine). The notes-merge code does this, and
>  before the fix in the prior commit t3308 erroneously
>  expected this case to fail.
> 
>   2. When resolving symbolic refs, we typically do not use
>  the READING flag because we want to resolve even
>  symrefs that point to unborn refs. Even if those unborn
>  refs could not actually be written because of d/f
>  conflicts with existing refs.
> 
>  You can see this by asking "git symbolic-ref" to report
>  the target of a symref pointing past a d/f conflict.
> 
> We can fix the problem by recognizing the other "missing"
> errnos and treating them like ENOENT. This should be safe to
> do even for callers who are then going to actually write the
> ref, because the actual writing process will fail if the d/f
> conflict is a real one (and t1404 checks these cases).
> 
> Arguably this should be the responsibility of the
> files-backend to normalize all "missing ref" errors into
> ENOENT (since something like EISDIR may not be meaningful at
> all to a database backend). However other callers of
> refs_read_raw_ref() may actually care about the distinction;
> putting this into resolve_ref() is the minimal fix for now.
> 
> The new tests in t1401 use git-symbolic-ref, which is the
> most direct way to check the resolution by itself.
> Interestingly we actually had a test that setup this case
> already, but we only used it to verify that the funny state
> could be overwritten, not that it could be resolved.
> 
> We also add a new test in t3200, as "branch -m" was the
> original motivation for looking into this. What happens is
> this:
> 
>   0. HEAD is pointing to branch "a"
> 
>   1. The user asks to rename "a" to "a/b".
> 
>   2. We create "a/b" and delete "a".
> 
>   3. We then try to update any worktree HEADs that point to
>  the renamed ref (including the main repo HEAD). To do
>  that, we have to resolve each HEAD. But now our HEAD is
>  pointing at "a", and we get EISDIR due to the loose
>  "a/b". As a result, we think there is no HEAD, and we
>  do not update it. It now points to the bogus "a".
> 
> Interestingly this case used to work, but only accidentally.
> Before 31824d180d (branch: fix branch renaming not updating
> HEADs correctly, 2017-08-24), we'd update any HEAD which we
> couldn't resolve. That was wrong, but it papered over the
> fact that we were incorrectly failing to resolve HEAD.
> 
> So while the bug demonstrated by the git-symbolic-ref is
> quite old, the regression to "branch -m" is recent.

Thanks for your detailed investigation and analysis and for the new tests.

This makes sense to me at the level of fixing the bug.

I do have one twinge of uneasiness at a deeper level, that I haven't had
time to check...

Does this patch make it easier to *set* HEAD to an unborn branch that
d/f conflicts with an existing reference? If so, that might be a
slightly worse UI for users. I'd rather learn about such a problem when
setting HEAD (when I am thinking about the new branch name and am in the
frame of mind to solve the problem) rather than later, when I try to
commit to the new branch.

Even if so, that wouldn't be a problem with this patch per se, but
rather a possible accidental side-effect of fixing the bug.

Michael

> [...]