Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches

[Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>]

Ævar, please let me know if your concern is resolved?

On 27.11.2019 17:09, Alexandr Miloslavskiy wrote:
> 
> 
> On 27.11.2019 16:43, Ævar Arnfjörð Bjarmason wrote:
>> I'll reserve judgement on whether we really should do this for now, my
>> current opinion on the matter is undefined as I haven't re-paged this
>> behavior of checkout into my brain.
>>
>> But a giant red flag here for me is that you say "I understand that this
>> was never intended".
>>
>> Just from a cursory look at this that's not true, for better or worse it
>> *is* intended behavior. Most of the code you're moving around here is
>> what I added in ad8d5104b4 ("checkout: add advice for ambiguous
>> "checkout <branch>"", 2018-06-05), and the very start of that commit
>> message refers to the checkout documentation we have that explicitly
>> documents this edge case.
>>
>> Digging a bit further reveals that we've had this behavior (again,
>> intended, not emergent) since 70c9ac2f19 ("DWIM "git checkout frotz" to
>> "git checkout -b frotz origin/frotz"", 2009-10-18), and had it
>> documented since 00bb4378c7 ("Documentation/git-checkout.txt: document
>> 70c9ac2 behavior", 2012-12-17).
>>
>> So at the very least I'd say you need a v2 where you amend the relevant
>> docs & commit message to make a case to the effect of "we've had this
>> since 2009, but it was never really all that important etc.".
>>
> 
> I'm sorry, can you please clarify?
> 
> My patch addresses the situation where there are *multiple* remote 
> candidates *and* a file with the same name.
> 
> My feeling is that in this case, reverting a file is an unintended 
> surprise.
> 
> I understand previous patches this way:
> ad8d5104 - patch series is mostly for "if there are *multiple* remotes,
>             disambiguate via checkout.defaultRemote". This essentially
>             converts the case of multiple remotes into a single remote.
>             However, this also semi-documents what I'm now preventing.
>             Was that really intended?
> 70c9ac2f - if there is *one* remote, DWIM it.
>             This isn't what I'm changing.
> be4908f1 - if there is *one* remote *and* file, die().
>             This is what I'm extending further.
> 
> If the prevented behavior is documented, could you please quote it 
> explicitly?
> 
>> Such a change should also be changing the docs etc. added in 8d7b558bae
>> ("checkout & worktree: introduce checkout.defaultRemote",
>> 2018-06-05). With this series our docs don't make a lot of sense anymore
>> & don't describe the behavior with the patches applied.
> 
> To my understanding, my patch doesn't affect those pieces of docs? 
> Please correct me if I'm wrong.