On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote:
> 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:"
> - '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>/
> 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.
> [...]
> Helped-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
> Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx>
> ---
> 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.
> + 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.
> + 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;
