All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git List <git@vger.kernel.org>, Duy Nguyen <pclouds@gmail.com>,
	Mark Levedahl <mlevedahl@gmail.com>,
	Mikael Magnusson <mikachu@gmail.com>
Subject: Re: [PATCH v3 18/23] checkout: retire --to option
Date: Tue, 7 Jul 2015 03:08:36 -0400	[thread overview]
Message-ID: <CAPig+cSBp-U_jC3fcPXkZQ6kEPh7TRs2bAwKYQGGTtoGR3UYeg@mail.gmail.com> (raw)
In-Reply-To: <xmqqh9physyu.fsf@gitster.dls.corp.google.com>

On Mon, Jul 6, 2015 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> Now that "git worktree add" has achieved user-facing feature-parity with
>> "git checkout --to", retire the latter.
>> [...]
>> This effectively reverts changes to checkout.c by 529fef2 (checkout:
>> support checking out into a new working directory, 2014-11-30) with the
>> exception of merge_working_tree() and switch_branches() which still
>> require specialized knowledge that a the checkout is occurring in a
>> newly-created linked worktree (signaled to them by the private
>> GIT_CHECKOUT_NEW_WORKTREE environment variable).
>
> I do not quite understand why we still need the hidden environment
> variable.  Is this a sign that the implementation is shared too much
> between unrelated codepaths (or to put it another way, perhaps API
> functions that are not best fit are being used)?

Duy had responded[1][2] to my inquiry about these two remaining bits
of checkout.c code with intimate knowledge of the initial
linked-worktree checkout (merge_working_tree & switch_branches), but I
don't know the checkout code well enough (yet) to fully understand his
responses or to answer your question satisfactorily. This is also why
I was afraid to rip out those final two bits of code, even though in
my own testing, having ripped them out locally, I was unable to
trigger any sort of "bad" behavior, as far as I could tell. Thus, I
wasn't sure if those two bits of code were needed, and was hoping
someone more qualified (Duy, for instance) would be able to provide a
more authoritative answer.

Having now re-read and finally digested Duy's response about
switch_branches(), when I rip out the `new_worktree_mode` check
locally, I find that I can trigger the misleading warning about an
orphaned commit, so that bit of code still serves a practical purpose.
This doesn't justify that such ugly intimacy between git-worktree and
git-checkout is desirable; only that it still seems to be needed until
"git reset --hard" can be swapped in to replace "git checkout".

[1]: http://article.gmane.org/gmane.comp.version-control.git/273225
[2]: http://article.gmane.org/gmane.comp.version-control.git/273226

> Stepping back a bit, with or without the new "linked worktree"
> feature, when you came across a repository whose working tree does
> not have any file (i.e. somebody ran "git ls-files | xargs rm"), you
> do not know and care what is in .git/index right now, you do not
> know and care what branch its .git/HEAD points at, but you *do* know
> what branch you want to be on (or where you want its HEAD detached
> at), what would be the command you would use?
>
> The state immediately after a new worktree is constructed by
> populating /path/main/.git/worktrees/test-next/ and pointing it from
> /path/other/test-next/.git but before the index or the files under
> /path/other/test-next/ are populated is exactly that situation, no?
> Wouldn't "symbolic-ref HEAD the-branch-i-want" (or "update-ref HEAD
> the-commit-i-want" in the detached case) followed by "reset --hard"
> the more natural thing to use, instead of merge-working-tree and
> switch-branches that are implementation details of "checkout"?

It seems so to me. I didn't repeat it in the v3 cover letter, but the
v2 cover letter said this (which still holds true for v3):

    v2 does not attempt either of the suggestions by Junio[*3*] or
    Duy[*4*] for eliminating git-checkout from the equation, which
    would allow us to remove the final couple bits of code in
    git-checkout which require intimate knowledge that the checkout
    is occurring in a newly created linked worktree. This series is
    already too long, and I didn't want it to grow further by
    implementing either of those ideas. Instead, this series leaves
    git-worktree at a state where one or the other of those
    suggestions can be done as follow-on patches touching only the
    underlying machinery, without affecting the user-facing
    interface.

    [*3*]: via private email which suggested using "git-reset --hard"
           rather than "git checkout" to populate the new linked
           worktree.
    [*4*]: http://thread.gmane.org/gmane.comp.version-control.git/273032/focus=273226

In order to use "git reset --hard" in place of "git checkout",
git-worktree will need to handle -b/-B, --detach, --force (and
possibly --track and --orphan) itself. I'm still in the process of
familiarizing myself with the code needed to perform those actions, as
well as whatever else is needed to make "git reset --hard" a reality,
so I am not yet in a position to say now much time or work will be
required to swap out "git checkout" for "git reset --hard". As such, I
didn't want to hold up the series for an unknown amount of time while
studying the issue; and, as the series stands, the remaining ugly
intimate knowledge between git-worktree and git-checkout is
behind-the-scenes and localized (not affecting the user experience).

  reply	other threads:[~2015-07-07  7:08 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-06 17:30 [PATCH v3 00/23] replace "checkout --to" with "worktree add" Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 01/23] Documentation/git-checkout: fix incorrect worktree prune command Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 02/23] Documentation/git-worktree: associate options with commands Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 03/23] Documentation: move linked worktree description from checkout to worktree Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 04/23] Documentation/git-worktree: add BUGS section Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 05/23] Documentation/git-worktree: split technical info from general description Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 06/23] Documentation/git-worktree: add high-level 'lock' overview Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 07/23] Documentation/git-worktree: add EXAMPLES section Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 08/23] checkout: fix bug with --to and relative HEAD Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 09/23] checkout: relocate --to's "no branch specified" check Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 10/23] checkout: prepare_linked_checkout: drop now-unused 'new' argument Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 11/23] checkout: make --to unconditionally verbose Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 12/23] checkout: drop 'checkout_opts' dependency from prepare_linked_checkout Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 13/23] worktree: introduce "add" command Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 14/23] worktree: add --force option Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 15/23] worktree: add --detach option Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 16/23] worktree: add -b/-B options Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 17/23] tests: worktree: retrofit "checkout --to" tests for "worktree add" Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 18/23] checkout: retire --to option Eric Sunshine
2015-07-06 19:41   ` Junio C Hamano
2015-07-07  7:08     ` Eric Sunshine [this message]
2015-07-08 16:58       ` Junio C Hamano
2015-07-06 17:30 ` [PATCH v3 19/23] checkout: require worktree unconditionally Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 20/23] worktree: extract basename computation to new function Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 21/23] worktree: add: make -b/-B default to HEAD when <branch> is omitted Eric Sunshine
2015-07-06 17:30 ` [PATCH v3 22/23] worktree: add: auto-vivify new branch " Eric Sunshine
2015-07-06 19:19   ` Junio C Hamano
2015-07-07  1:33     ` Eric Sunshine
2015-07-07 16:10       ` Junio C Hamano
2015-07-06 17:31 ` [PATCH v3 23/23] checkout: retire --ignore-other-worktrees in favor of --force Eric Sunshine
2015-07-06 19:40   ` Junio C Hamano
2015-07-07  8:24     ` Eric Sunshine
2015-07-07  9:41       ` Eric Sunshine
2015-07-07 16:20         ` Junio C Hamano
2015-07-07 23:10           ` Eric Sunshine
2015-07-08  0:43     ` Mark Levedahl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPig+cSBp-U_jC3fcPXkZQ6kEPh7TRs2bAwKYQGGTtoGR3UYeg@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mikachu@gmail.com \
    --cc=mlevedahl@gmail.com \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.