Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jul 31, 2015, 21:36 UTC
- Message-ID
- <CAPig+cRZ4PicP7J0Hdwb1+E9JbE=wsfqEaVX6X4ERST2_nJrxw@mail.gmail.com>
- In-Reply-To
- <1438377312.4735.10.camel@twopensource.com>
On Fri, Jul 31, 2015 at 5:15 PM, David Turner <dturner@twopensource.com> wrote:
Show 23 quoted lines
> On Fri, 2015-07-31 at 15:35 -0400, Eric Sunshine wrote: >> On Fri, Jul 31, 2015 at 3:01 PM, David Turner <dturner@twopensource.com> wrote: >> > Add a new function, die_if_shared_symref, which works like >> > die_if_checked_out, but for all references. Refactor >> > die_if_checked_out to work in terms of die_if_shared_symref. >> > >> > Soon, we will use die_if_shared_symref to protect notes merges in >> > worktrees. >> >> I wonder if the diagnostic: >> >> 'blorp' is already checked out at '/path/name/' >> >> emitted by check_linked_checkout() needs to be adjusted for this >> change. It still makes sense for die_if_checked_out(), but sounds odd >> for die_if_shared_symref(). > > How about: > > 'refs/notes/y' is already referenced from 'NOTES_MERGE_REF' in > '/home/dturner/git/t/trash directory.t3320-notes-merge-worktrees/' > > Does that make sense?
That might be the best we can do for the generic case of die_if_shared_symref(), but I wonder how easily the typical user would understand it when trying to checkout a branch already checked out elsewhere. One concern is that that message almost comes across as an internal Git error (thus inscrutable), whereas:
% git checkout blorp
'blorp' is already checked out at '/some/path/'seems (to me) pretty clearly a user error, thus more easily understood.
Show 5 quoted lines
> It's not the only place in the error messages that mentions > NOTES_MERGE_REF, so I think we expect users to understand > NOTES_MERGE_REF. The alternative is to move the error handling to an > even higher level so we can give a notes-specific message. I don't > think that's necessary, but I'll do it if others do.
Given such precedence, the generic error message might indeed be fine for the notes merge case, however, in general, you'd probably get better and more understandable error messages by tailoring them at the call sites. (That's a positive vote from me for for lifting error handling to a higher level.)