[PATCH v6 0/6] worktree: teach "add" to check out existing branches
Thanks Eric for the review of the last round.
Previous rounds are at <20180121120208.12760-1-t.gumme...@gmail.com>,
<20180204221305.28300-1-t.gumme...@gmail.com>,
<20180317220830.30963-1-t.gumme...@gmail.com>,
<2018031719.4940-1-t.gumme...@gmail.com> and
20180325134947.25828-1-t.gumme...@gmail.com.
This round should fix all the UI issues Eric found in the last round.
The changes I made in a bit more detail:
- in addition to removing the 'force_new_branch' flag from 'struct
add_opts', also remove the 'new_branch' member, which is local to
the 'add()' function. The other four members are needed in the
'worktree_add()' function, so I left them there. This patch is now
the first patch in the series.
- 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.
I guess this may be the most controversial bit at this point, as
we'd be adding an option for internal use only. Not sure how we
feel about that. But short of going back to the old output format, I
don't see a good option to make this work otherwise. I tried
re-using internal functions for this, but until we have 'struct
repository' everywhere, that's going to be quite hard.
- Print the "Creating branch ..." and "Checking out branch ..."
messages earlier in the process. This is mainly to avoid the out of
order "Branch '...' now set up to track ..." message.
- Various commit message and style cleanups
Note that these fixes are quite differently executed than I had
imagined them in the reply to the review comments, as things didn't
work as I had imagined. The UI problems should be fixed now
nonetheless :)
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'
- large repository (with original dwim)
$ g worktree add ../test
Creating branch 'test'
Checking out files: 100% (62915/62915), done.
New worktree HEAD is now at c2a9838452a4 Merge tag 'for-4.16/dm-fixes-4' of
git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm
Compare this to the old UI (new dwim omitted, as there's no old
version of that):
- guess-remote mode
$ git worktree add --guess-remote ../next
Branch 'next' set up to track remote branch 'next' from 'origin'.
Preparing ../next (identifier next)
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
Preparing ../test (identifier test)
HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
- no new branch created
$ git worktree add ../test2 origin/master
Preparing ../test2 (identifier test2)
HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'
- large repository
$ git worktree add ../test
Preparing ../test (identifier test)
Checking out files: 100% (62915/62915), done.
HEAD is now at c2a9838452a4 Merge tag 'for-4.16/dm-fixes-4' of
git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm
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.
We could also print "Preparing worktree " as a line in the
beginning (without mentioning the identifier, so we can print it in
the 'add()' function), but I don't feel like that's worth spending the
extra screen estate. I don't feel strongly about that though, so if
someone has a moderately strong preference for that line being there,
I'm happy to add it.
Interdiff below:
diff --git a/builtin/reset.c b/builtin/reset.c
index e15f595799..54b2576449 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -288,6 +288,7 @@ static int git_reset_config(const char *var, const char
*value, void *cb)
int cmd_reset(int argc, const char **argv, const char *prefix)
{
int reset_type = N
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer 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.) > 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 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? > 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.
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On 04/08, Eric Sunshine wrote: > On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer > 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 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 ' 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 (o
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On Sun, Apr 8, 2018 at 10:24 AM, Thomas Gummerer wrote: > On 04/08, Eric Sunshine wrote: >> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer >> wrote: > Let me think through some of the cases here, of 'git worktre add > ' 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' Did you mean "Checking out 'test'"? > 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? To which "Checking out" message do you refer, the one showing the branch name or the one showing the checkout progress? I'd probably agree that showing both "Creating" and "Checkout out" is overkill. However, see my response[1] to your "fixup!" patch in which I explore the idea that unifying "Checking out 'branch' and "Creating branch" messages may be a good idea and get us out of some UI jams which seem to be cropping up. [1]: https://public-inbox.org/git/20180325134947.25828-1-t.gumme...@gmail.com/T/#m5d38b0c6427609e8c36aa6af83d518791c1e1581 > - 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 ..."?) Taking [1] into account, this might become something like: $ git worktree add --detach ../test Preparing worktree (detached HEAD c2a499e6c) Checking out files: ...% New worktree HEAD is now at c2a499e6c Gobbledygook > 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. Fair enough observation. The idea suggested in [1] may keep output to a reasonable amount.
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On 04/08, Eric Sunshine wrote: > On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer > 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. Something else I just noticed that may make this a worse solution is that this breaks the sentence in two pieces for translators. I guess we could somehow get the "New worktree" part of the option translated, but that still means that if some language would require to move parts of the sentence around that would be less than ideal for translation. Duy pointed this out to me in an earlier patch series, and I think we should probably not make life harder (or impossible) for translators if we can avoid it. Would factoring out what we have in 'print_new_head_line()' into some common code, maybe in 'pretty.c', and still doing the printing from here be a reasonable tradeoff? I think this could potentially even be re-used in other places, although again I'd like to keep that for a followup series to avoid scope creep in this one. > 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.) > > > 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 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? > > > 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.
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On 04/08, Eric Sunshine wrote: > On Sun, Apr 8, 2018 at 10:24 AM, Thomas Gummerer wrote: > > On 04/08, Eric Sunshine wrote: > >> On Sat, Mar 31, 2018 at 11:17 AM, Thomas Gummerer > >> wrote: > > Let me think through some of the cases here, of 'git worktre add > > ' 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' > > Did you mean "Checking out 'test'"? > > > 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? > > To which "Checking out" message do you refer, the one showing the > branch name or the one showing the checkout progress? > > I'd probably agree that showing both "Creating" and "Checkout out" is > overkill. However, see my response[1] to your "fixup!" patch in which > I explore the idea that unifying "Checking out 'branch' and "Creating > branch" messages may be a good idea and get us out of some UI jams > which seem to be cropping up. > > [1]: > https://public-inbox.org/git/20180325134947.25828-1-t.gumme...@gmail.com/T/#m5d38b0c6427609e8c36aa6af83d518791c1e1581 > > > - 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 ..."?) > > Taking [1] into account, this might become something like: > > $ git worktree add --detach ../test > Preparing worktree (detached HEAD c2a499e6c) > Checking out files: ...% > New worktree HEAD is now at c2a499e6c Gobbledygook The more I look at this solution, the more I like it. I'll try to implement this to see if there's anything I'm not thinking of right now, but I think I'll take the suggestion and send a re-roll with it implemented. > > 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. > > Fair enough observation. The idea suggested in [1] may keep output to > a reasonable amount.
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On Mon, Apr 9, 2018 at 3:30 PM, Thomas Gummerer wrote: > On 04/08, Eric Sunshine wrote: >> 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. > > Something else I just noticed that may make this a worse solution is > that this breaks the sentence in two pieces for translators. I guess > we could somehow get the "New worktree" part of the option translated, > but that still means that if some language would require to move parts > of the sentence around that would be less than ideal for translation. Good point. One solution would be to have the new hidden option replace the string entirely: --new-head-msg="New worktree HEAD is now at %s", which would allow translators to deal with the entire sentence. Even clearer would be to drop "now", as in "New worktree HEAD is at %s". (Default in reset.c would still be "HEAD is now at %s", of course.) Another solution would be not to augment the "HEAD is now at..." message at all. I realize that that augmentation was one of the original motivations for this patch series, but with the upcoming restoration of the "Preparing worktree" message: Preparing worktree (_branch disposition_) HEAD is now at ... it seems clear enough (at least to me) from the context introduced by the "Preparing..." message that "HEAD is now at..." refers to HEAD in the worktree. (But that's just my opinion.) > Would factoring out what we have in 'print_new_head_line()' into some > common code, maybe in 'pretty.c', and still doing the printing from > here be a reasonable tradeoff? Isn't that getting uglier again? Not only would you have to publish that function, but you'd still need the hidden git-reset --show-new-head-line option. Also, you'd end up calling that function from within low-level worker worktree.c:add_worktree(), thus making it take on UI duties, which would be nice to avoid if possible. (Unfortunately, the alternate idea of having worktree.c:add() handle this UI task doesn't quite work since add_worktree() doesn't return sufficient information for add() to know whether or not it should print "HEAD is now at..."; however, add_worktree() could be augmented to return more information.) So, I don't presently have a good answer. I'm partial to the idea of not augmenting "HEAD is now at...", partly because context makes it relatively clear that it applies to the new worktree and partly because it's simpler (less implementation work for you) to leave it as is. If that choice isn't desirable, then next best would be for --new-head-msg= to replace the entire "HEAD is now at..." string rather than augmenting it.
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On 04/09, Eric Sunshine wrote: > On Mon, Apr 9, 2018 at 3:30 PM, Thomas Gummerer wrote: > > On 04/08, Eric Sunshine wrote: > >> 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. > > > > Something else I just noticed that may make this a worse solution is > > that this breaks the sentence in two pieces for translators. I guess > > we could somehow get the "New worktree" part of the option translated, > > but that still means that if some language would require to move parts > > of the sentence around that would be less than ideal for translation. > > Good point. > > One solution would be to have the new hidden option replace the string > entirely: --new-head-msg="New worktree HEAD is now at %s", which would > allow translators to deal with the entire sentence. Even clearer would > be to drop "now", as in "New worktree HEAD is at %s". (Default in > reset.c would still be "HEAD is now at %s", of course.) > > Another solution would be not to augment the "HEAD is now at..." > message at all. I realize that that augmentation was one of the > original motivations for this patch series, but with the upcoming > restoration of the "Preparing worktree" message: My original motivation of the series was to just make the new dwim work :) Because that's adding some magic, the secondary motivation became improving the UI, to help users see which dwim was used. I felt like this was going to be one of those improvements, especially after we get rid of the "Preparing ..." line. I do however like your suggestion of the "Preparing worktree (_branch disposition_)", as that doesn't add more lines to the output, while still giving a good indication of what exactly is happening. At that point just showing "HEAD is now at ..." is fine by me, and doesn't require adding the hidden flag to 'git reset'. So I'm happy just dropping the change in the message here, which will simplify things. Thanks for the suggestion! > Preparing worktree (_branch disposition_) > HEAD is now at ... > > it seems clear enough (at least to me) from the context introduced by > the "Preparing..." message that "HEAD is now at..." refers to HEAD in > the worktree. (But that's just my opinion.) > > > Would factoring out what we have in 'print_new_head_line()' into some > > common code, maybe in 'pretty.c', and still doing the printing from > > here be a reasonable tradeoff? > > Isn't that getting uglier again? Not only would you have to publish > that function, but you'd still need the hidden git-reset > --show-new-head-line option. > > Also, you'd end up calling that function from within low-level worker > worktree.c:add_worktree(), thus making it take on UI duties, which > would be nice to avoid if possible. (Unfortunately, the alternate idea > of having worktree.c:add() handle this UI task doesn't quite work > since add_worktree() doesn't return sufficient information for add() > to know whether or not it should print "HEAD is now at..."; however, > add_worktree() could be augmented to return more information.) > > So, I don't presently have a good answer. I'm partial to the idea of > not augmenting "HEAD is now at...", partly because context makes it > relatively clear that it applies to the new worktree and partly > because it's simpler (less implementation work for you) to leave it as > is. If that choice isn't desirable, then next best would be for > --new-head-msg= to replace the entire "HEAD is now at..." string > rather than augmenting it.
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On Wed, Apr 11, 2018 at 4:09 PM, Thomas Gummerer wrote: > On 04/09, Eric Sunshine wrote: >> Another solution would be not to augment the "HEAD is now at..." >> message at all. I realize that that augmentation was one of the >> original motivations for this patch series, but with the upcoming >> restoration of the "Preparing worktree" message: > > My original motivation of the series was to just make the new dwim > work :) Because that's adding some magic, the secondary motivation > became improving the UI, to help users see which dwim was used. I > felt like this was going to be one of those improvements, especially > after we get rid of the "Preparing ..." line. > > I do however like your suggestion of the "Preparing worktree (_branch > disposition_)", as that doesn't add more lines to the output, while > still giving a good indication of what exactly is happening. At that > point just showing "HEAD is now at ..." is fine by me, and doesn't > require adding the hidden flag to 'git reset'. So I'm happy just > dropping the change in the message here, which will simplify things. Nice. This sounds like a good plan for moving forward.
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On 04/11, Thomas Gummerer wrote: > On 04/09, Eric Sunshine wrote: > > On Mon, Apr 9, 2018 at 3:30 PM, Thomas Gummerer > > wrote: > > > On 04/08, Eric Sunshine wrote: > > >> 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. > > > > > > Something else I just noticed that may make this a worse solution is > > > that this breaks the sentence in two pieces for translators. I guess > > > we could somehow get the "New worktree" part of the option translated, > > > but that still means that if some language would require to move parts > > > of the sentence around that would be less than ideal for translation. > > > > Good point. > > > > One solution would be to have the new hidden option replace the string > > entirely: --new-head-msg="New worktree HEAD is now at %s", which would > > allow translators to deal with the entire sentence. Even clearer would > > be to drop "now", as in "New worktree HEAD is at %s". (Default in > > reset.c would still be "HEAD is now at %s", of course.) > > > > Another solution would be not to augment the "HEAD is now at..." > > message at all. I realize that that augmentation was one of the > > original motivations for this patch series, but with the upcoming > > restoration of the "Preparing worktree" message: > > My original motivation of the series was to just make the new dwim > work :) Because that's adding some magic, the secondary motivation > became improving the UI, to help users see which dwim was used. I > felt like this was going to be one of those improvements, especially > after we get rid of the "Preparing ..." line. > > I do however like your suggestion of the "Preparing worktree (_branch > disposition_)", as that doesn't add more lines to the output, while > still giving a good indication of what exactly is happening. At that > point just showing "HEAD is now at ..." is fine by me, and doesn't > require adding the hidden flag to 'git reset'. So I'm happy just > dropping the change in the message here, which will simplify things. And just as I'm re-reading my commit messages, I guess there was one more motivation for printing the "HEAD is now at ..." line ourselves: If the '--no-checkout' flag is given, the output of 'git worktree add' is just: Preparing foo (identifier foo) even though the HEAD is set to a commit, which is just not checked out. I think I can live with that for now, I personally don't use '--no-checkout', and we could fix this at some point in the future if we desire to do so. I think we can consider that out of scope for this patch series, as its main goal is to introduce the new dwim. > Thanks for the suggestion!
Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches
On Wed, Apr 11, 2018 at 4:50 PM, Thomas Gummerer wrote: > And just as I'm re-reading my commit messages, I guess there was one > more motivation for printing the "HEAD is now at ..." line ourselves: > > If the '--no-checkout' flag is given, the output of 'git worktree add' > is just: > > Preparing foo (identifier foo) > > even though the HEAD is set to a commit, which is just not checked out. > > I think I can live with that for now, I personally don't use > '--no-checkout', and we could fix this at some point in the future if > we desire to do so. I think we can consider that out of scope for > this patch series, as its main goal is to introduce the new dwim. Sounds reasonable, especially since the proposed "Preparing worktree (_branch description_)" provides similarly useful feedback.
