git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git List <git@vger.kernel.org>,
	Nguyễn Thái Ngọc Duy <pclouds@gmail.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
Date: Sun, 8 Apr 2018 15:24:17 +0100
Message-ID: <20180408142417.GJ2629@hank> (raw)
In-Reply-To: <CAPig+cQ8VzDycUMo-QOexNDBgQGEGj2BPmPa-Y0vhGCt_brbhg@mail.gmail.com>

On 04/08, Eric Sunshine wrote:
> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > This round should fix all the UI issues Eric found in the last round.
> > The changes I made in a bit more detail:
> >
> > - added a new commit introducing a new hidden --show-new-head-line
> >   flag in 'git reset'.  This is used to suppress the "HEAD is now at
> >   ..."  line that 'git reset --hard' usually prints, so we can replace
> >   it with our own "New worktree HEAD is now at ..." line instead,
> >   while keeping the progress indicator for larger repositories.
> 
> As with Junio, I'm fine with this hidden option (for now), however, I
> think you can take this a step further. Rather than having a (hidden)
> git-reset option which suppresses "HEAD is now at...", instead have a
> (hidden) option which augments the message. For example,
> --new-head-desc="New worktree" would make it output "New worktree HEAD
> is now at...". Changes to builtin/reset.c to support this would hardly
> be larger than the changes you already made.
> 
> The major benefit is that patch 3/6 no longer has to duplicate the
> code from builtin/reset.c:print_new_head_line() just to print its own
> "New worktree HEAD is now at..." message. (As for the argument that
> "git worktree add" must duplicate that code because it wants the
> message on stderr, whereas git-reset prints it to stdout, I don't see
> why git-worktree puts those messages to stderr in the first place. As
> far as I can tell, it would be equally valid to print them to stdout.)

I didn't think of that, but I think that's nicer indeed.  Will change.
This will also be nicer when we're in a position to remove the hidden
option and do all of this with internal functions.  Then we can just
re-use the new function that also takes a prefix at that point (after
moving the function to 'libgit.a' of course.

> > Some examples of the new UI behaviour here for reference:
> >
> >  - guess-remote mode
> >
> >     $ git worktree add --guess-remote ../next
> >     Creating branch 'next'
> >     Branch 'next' set up to track remote branch 'next' from 'origin'.
> >     New worktree HEAD is now at caa68db14 Merge branch 'sb/packfiles-in-repository' into next
> >
> >  - original dwim (create a branch based on the current HEAD)
> >
> >     $ git worktree add ../test
> >     Creating branch 'test'
> >     New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> >
> >  - new dwim (check out existing branch)
> >
> >     $ git worktree add ../test
> >     Checking out branch 'test'
> >     New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> >
> >  - no new branch created
> >
> >     $ git worktree add ../test2 origin/master
> >     New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
> 
> I like the "creating" or "checking out" messages we now get for all
> the DWIM cases. I wonder if it would make sense to print "Checkout out
> blah..." for this case too. It's certainly not necessary since the
> user specified <commit-ish> explicitly, but it would make the UI even
> more consistent, and address your subsequent comment about missing
> context above the "Checking out files: ...%" line for this case.
> Thoughts?

Let me think through some of the cases here, of 'git worktre add
<path> <commit-ish>' with various flags and what the UI would be with
that added:

  - no flags:

    $ git worktree add ../test origin/master
    Checking out 'origin/master'
    Checking out files: ...%
    New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

  - -b branch:

    $ git worktree add -b test ../test origin/master
    Creating branch 'test'
    Checking out 'origin/master'
    Checking out files: ...%
    New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

    Would we want to omit the "Checking out ..." here?  I'm leaning
    towards yes, but dunno?

  - --no-checkout

    $ git worktree add --no-checkout test ../test origin/master
    New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

  - Original dwim with --detach flag

    $ git worktree add --detach ../test
    Checking out 'c2a499e6c'
    Checking out files: ...%
    New worktree HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

Looking at this, I'm not sure what's best here.  I'm not sure I'm a
fan of the duplicate "Checking out " message (I assume that's what you
meant above, or did you mean just "Checkout ..."?)

I als don't think it gives too much context compared to just "Checking
out files: ...%".  I think it gives a bit more context when that
message is not displayed at all, as it shows whether files are checked
out or not, but if we do that, when we create a new branch, the amount
of output we'd display is getting a bit long, to the point where I
suspect users would just not read it anymore.

So I personally don't feel like this is worth it, even though it may
give some context in some cases.  But I'm also far from an expert in
UI design, so if you (or others) feel this would be nicer I'm happy to
implement it :)

> > Compare this to the old UI (new dwim omitted, as there's no old
> > version of that):
> 
> Thanks for contrasting the new with the old. The new output is nicer
> and more helpful.
> 
> > The one thing we are loosing is a context line before "Checking out
> > files:", if no new branch is created.  Personally I feel like that's
> > acceptable, as the user just used the 'git worktree add' command, so
> > it should be intuitive where those files are being checked out.

  reply index

Thread overview: 111+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-21 12:02 [PATCH] " Thomas Gummerer
2018-01-21 12:02 ` Robert P. J. Day
2018-01-22 11:18 ` Duy Nguyen
2018-01-22 20:17   ` Thomas Gummerer
2018-02-04 22:13 ` [PATCH v2 0/3] " Thomas Gummerer
2018-02-04 22:13   ` [PATCH v2 1/3] worktree: improve message when creating a new worktree Thomas Gummerer
2018-02-05  2:12     ` Duy Nguyen
2018-02-05 20:13       ` Thomas Gummerer
2018-02-05 20:15       ` Junio C Hamano
2018-02-07  8:51       ` Eric Sunshine
2018-02-09 11:27         ` Thomas Gummerer
2018-02-09 12:08           ` Duy Nguyen
2018-02-10 11:20             ` Duy Nguyen
2018-02-04 22:13   ` [PATCH v2 2/3] worktree: be clearer when "add" dwim-ery kicks in Thomas Gummerer
2018-02-04 22:13   ` [PATCH v2 3/3] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-02-05  2:18     ` Duy Nguyen
2018-02-05 20:20       ` Junio C Hamano
2018-02-05 20:23       ` Thomas Gummerer
2018-02-06 11:53         ` Duy Nguyen
2018-02-09 11:04           ` Thomas Gummerer
2018-03-17 22:08   ` [PATCH v3 0/4] " Thomas Gummerer
2018-03-17 22:08     ` [PATCH v3 1/4] worktree: improve message when creating a new worktree Thomas Gummerer
2018-03-17 22:08     ` [PATCH v3 2/4] worktree: be clearer when "add" dwim-ery kicks in Thomas Gummerer
2018-03-17 22:08     ` [PATCH v3 3/4] worktree: factor out dwim_branch function Thomas Gummerer
2018-03-17 22:08     ` [PATCH v3 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-03-17 22:22     ` [PATCH v4 0/4] " Thomas Gummerer
2018-03-17 22:22       ` [PATCH v4 1/4] worktree: improve message when creating a new worktree Thomas Gummerer
2018-03-19 17:11         ` Duy Nguyen
2018-03-19 18:09           ` Junio C Hamano
2018-03-20  6:37         ` Eric Sunshine
2018-03-24 20:34           ` Thomas Gummerer
2018-03-17 22:22       ` [PATCH v4 2/4] worktree: be clearer when "add" dwim-ery kicks in Thomas Gummerer
2018-03-20  6:40         ` Eric Sunshine
2018-03-20  7:26         ` Eric Sunshine
2018-03-20  7:32           ` Eric Sunshine
2018-03-24 20:35             ` Thomas Gummerer
2018-03-17 22:22       ` [PATCH v4 3/4] worktree: factor out dwim_branch function Thomas Gummerer
2018-03-17 22:22       ` [PATCH v4 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-03-20  8:02         ` Eric Sunshine
2018-03-24 21:00           ` Thomas Gummerer
2018-03-25 13:49       ` [PATCH v5 0/6] " Thomas Gummerer
2018-03-25 13:49         ` [PATCH v5 1/6] worktree: improve message when creating a new worktree Thomas Gummerer
2018-03-25 13:49         ` [PATCH v5 2/6] worktree: be clearer when "add" dwim-ery kicks in Thomas Gummerer
2018-03-27  8:59           ` Eric Sunshine
2018-03-30 13:53             ` Thomas Gummerer
2018-03-25 13:49         ` [PATCH v5 3/6] worktree: remove force_new_branch from struct add_opts Thomas Gummerer
2018-03-27  9:00           ` Eric Sunshine
2018-03-30 13:55             ` Thomas Gummerer
2018-03-25 13:49         ` [PATCH v5 4/6] worktree: factor out dwim_branch function Thomas Gummerer
2018-03-27  9:01           ` Eric Sunshine
2018-03-25 13:49         ` [PATCH v5 5/6] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-03-27  9:04           ` Eric Sunshine
2018-03-30 14:04             ` Thomas Gummerer
2018-03-25 13:49         ` [PATCH v5 6/6] t2025: rename now outdated branch name Thomas Gummerer
2018-03-27  8:58         ` [PATCH v5 0/6] worktree: teach "add" to check out existing branches Eric Sunshine
2018-03-30 14:08           ` Thomas Gummerer
2018-03-31 15:17         ` [PATCH v6 " Thomas Gummerer
2018-03-31 15:17           ` [PATCH v6 1/6] worktree: remove extra members from struct add_opts Thomas Gummerer
2018-03-31 15:18           ` [PATCH v6 2/6] reset: introduce show-new-head-line option Thomas Gummerer
2018-04-02 20:29             ` Junio C Hamano
2018-04-02 22:07               ` Thomas Gummerer
2018-04-02 22:20               ` Thomas Gummerer
2018-04-02 20:34             ` Junio C Hamano
2018-04-02 22:09               ` Thomas Gummerer
2018-03-31 15:18           ` [PATCH v6 3/6] worktree: improve message when creating a new worktree Thomas Gummerer
2018-04-08  9:27             ` Eric Sunshine
2018-03-31 15:18           ` [PATCH v6 4/6] worktree: be clearer when "add" dwim-ery kicks in Thomas Gummerer
2018-03-31 15:18           ` [PATCH v6 5/6] worktree: factor out dwim_branch function Thomas Gummerer
2018-03-31 15:18           ` [PATCH v6 6/6] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-04-01 13:11             ` [PATCH v6 6.5/6] fixup! " Thomas Gummerer
2018-04-09  0:23               ` Eric Sunshine
2018-04-09 19:44                 ` Thomas Gummerer
2018-04-09 21:35                   ` Eric Sunshine
2018-04-08 10:09             ` [PATCH v6 6/6] " Eric Sunshine
2018-04-08 14:30               ` Thomas Gummerer
2018-04-08  9:08           ` [PATCH v6 0/6] " Eric Sunshine
2018-04-08 14:24             ` Thomas Gummerer [this message]
2018-04-09  0:38               ` Eric Sunshine
2018-04-09 19:47                 ` Thomas Gummerer
2018-04-09 19:30             ` Thomas Gummerer
2018-04-09 22:06               ` Eric Sunshine
2018-04-11 20:09                 ` Thomas Gummerer
2018-04-11 20:48                   ` Eric Sunshine
2018-04-11 20:50                   ` Thomas Gummerer
2018-04-11 21:14                     ` Eric Sunshine
2018-04-15 20:29           ` [PATCH v7 0/4] " Thomas Gummerer
2018-04-15 20:29             ` [PATCH v7 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
2018-04-15 20:29             ` [PATCH v7 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
2018-04-16  2:09               ` Junio C Hamano
2018-04-23 18:55                 ` Thomas Gummerer
2018-04-23  4:27               ` Eric Sunshine
2018-04-23 18:50                 ` Thomas Gummerer
2018-04-15 20:29             ` [PATCH v7 3/4] worktree: factor out dwim_branch function Thomas Gummerer
2018-04-15 20:29             ` [PATCH v7 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-04-23  4:52             ` [PATCH v7 0/4] " Eric Sunshine
2018-04-23 19:38             ` [PATCH v8 " Thomas Gummerer
2018-04-23 19:38               ` [PATCH v8 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
2018-04-24  3:26                 ` Eric Sunshine
2018-04-23 19:38               ` [PATCH v8 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
2018-04-24  3:58                 ` Eric Sunshine
2018-04-23 19:38               ` [PATCH v8 3/4] worktree: factor out dwim_branch function Thomas Gummerer
2018-04-23 19:38               ` [PATCH v8 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-04-24  4:25                 ` Eric Sunshine
2018-04-24 21:56               ` [PATCH v9 0/4] " Thomas Gummerer
2018-04-24 21:56                 ` [PATCH v9 1/4] worktree: remove extra members from struct add_opts Thomas Gummerer
2018-04-24 21:56                 ` [PATCH v9 2/4] worktree: improve message when creating a new worktree Thomas Gummerer
2018-04-24 21:56                 ` [PATCH v9 3/4] worktree: factor out dwim_branch function Thomas Gummerer
2018-04-24 21:56                 ` [PATCH v9 4/4] worktree: teach "add" to check out existing branches Thomas Gummerer
2018-04-27  7:36                 ` [PATCH v9 0/4] " Eric Sunshine
2018-04-28 16:09                   ` Thomas Gummerer
2018-04-30  0:07                     ` Junio C Hamano
2018-03-18  0:24     ` [PATCH v3 " Junio C Hamano

Reply instructions:

You may reply publically 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20180408142417.GJ2629@hank \
    --to=t.gummerer@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=sunshine@sunshineco.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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox