Re: [PATCH v8 2/4] worktree: improve message when creating a new worktree
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Apr 24, 2018, 03:58 UTC
- Message-ID
- <CAPig+cSOhdfyn+yJcsM9h3r7Atf4pQCX7pgPOB2hv+OHJ3r1bg@mail.gmail.com>
- In-Reply-To
- <20180423193848.5159-3-t.gummerer@gmail.com>
On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 21 quoted lines
> Currently 'git worktree add' produces output like the following: > > Preparing ../foo (identifier foo) > HEAD is now at 26da330922 <title> > > The '../foo' is the path where the worktree is created, which the user > has just given on the command line. The identifier is an internal > implementation detail, which is not particularly relevant for the user > and indeed isn't mentioned explicitly anywhere in the man page. > > Instead of this message, print a message that gives the user a bit more > detail of what exactly 'git worktree' is doing. There are various dwim > modes which perform some magic under the hood, which should be > helpful to users. Just from the output of the command it is not always > visible to users what exactly has happened. > > Help the users a bit more by modifying the "Preparing ..." message and > adding some additional information of what 'git worktree add' did under > the hood, while not displaying the identifier anymore. > > Currently this ends up in three different cases:
You now enumerate four cases, not three. Perhaps: "There are several different cases:"
Show 16 quoted lines
> - 'git worktree add -b ...' or 'git worktree add <path>', both of > which create a new branch, either through the user explicitly > requesting it, or through 'git worktree add' implicitly creating > it. This will end up with the following output: > > Preparing worktree (new branch '<branch>') > HEAD is now at 26da330922 <title> > > - 'git worktree add -B ...', which may either create a new branch if > the branch with the given name does not exist yet, or resets an > existing branch to the current HEAD, or the commit-ish given. > Depending on which action is taken, we'll end up with the following > output: > > Preparing worktree (resetting branch 'next'; was at caa68db14) > HEAD is now at 26da330922 <title>
For consistency with the other examples: s/next/<branch>/
Show 14 quoted lines
> or: > > Preparing worktree (new branch '<branch>') > HEAD is now at 26da330922 <title> > > - 'git worktree add --detach' or 'git worktree add <path> > <commit-ish>', both of which create a new worktree with a detached > HEAD, for which we will print the following output: > > Preparing worktree (detached HEAD 26da330922) > HEAD is now at 26da330922 <title> > > - 'git worktree add <path> <local-branch>', which checks out the > branch prints the following output:
s/prints/and prints/
> Preparing worktree (checking out '<branch>') > HEAD is now at 47007d5 <title>
s/<branch>/<local-branch>/ would probably be clearer since you use <local-branch> in the example command.
Show 13 quoted lines
> [...]
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -355,6 +353,40 @@ static int add_worktree(const char *path, const char *refname,
> +static void print_preparing_worktree_line(int detach,
> + const char *branch,
> + const char *new_branch,
> + const char *new_branch_force)
> +{
> + if (new_branch_force) {
> + struct commit *commit = lookup_commit_reference_by_name(new_branch_force);Nit: By the time this function is called, 'new_branch' contains the same value as 'new_branch_force' if "-B" was used. 'new_branch' _is_ the name of the new branch, forced or not, and it's easier to reason about 'new_branch_force' as a mere boolean indicating whether "force" is in effect. This suggests that the final argument to this function should be a boolean (int) named either 'force' or 'force_new_branch', and that 'new_branch' should be used everywhere the code needs to reference the new branch name (forced or not).
Not worth a re-roll.
Show 23 quoted lines
> + if (!commit)
> + printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
> + else
> + printf_ln(_("Preparing worktree (resetting branch '%s'; was at %s)"),
> + new_branch_force,
> + find_unique_abbrev(commit->object.oid.hash,
> + DEFAULT_ABBREV));
> + } else if (new_branch) {
> + printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
> + } else {
> + struct strbuf s = STRBUF_INIT;
> + if (!detach && !strbuf_check_branch_ref(&s, branch) &&
> + ref_exists(s.buf))
> + printf_ln(_("Preparing worktree (checking out '%s')"),
> + branch);
> + else {
> + struct commit *commit = lookup_commit_reference_by_name(branch);
> + if (!commit)
> + die(_("invalid reference: %s"), branch);
> + printf_ln(_("Preparing worktree (detached HEAD %s)"),
> + find_unique_abbrev(commit->object.oid.hash,
> + DEFAULT_ABBREV));
> + }It's too bad this final case duplicates so much code from add_worktree(). Perhaps some future refactoring patch (not part of this series) can consolidate the code.
Show 8 quoted lines
> + strbuf_release(&s);
> + }
> +}
> @@ -434,6 +466,8 @@ static int add(int ac, const char **av, const char *prefix)
> + print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force);
> +
> if (new_branch) {
> struct child_process cp = CHILD_PROCESS_INIT;