From: Alexandr Miloslavskiy Date: Thu, 05 Dec 2019 15:34:17 GMT Subject: Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches Message-ID: <065c02a7-8ef9-192b-097e-57913db05520@syntevo.com> In-Reply-To: <08b8acc1-6925-ad40-faa2-88a16b8d135f@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 "", 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.