git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Rafael Silva <rafaeloliveira.cs@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] worktree: teach `list` to mark locked worktree
Date: Mon, 28 Sep 2020 14:37:54 -0700	[thread overview]
Message-ID: <xmqq8sctlgzx.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200928154953.30396-2-rafaeloliveira.cs@gmail.com> (Rafael Silva's message of "Mon, 28 Sep 2020 15:49:52 +0000")

Rafael Silva <rafaeloliveira.cs@gmail.com> writes:

> The output of `worktree list` command is extended to mark a locked
> worktree with `(locked)` text. This is used to communicate to the
> user that a linked worktree is locked instead of learning only when
> attempting to remove it.
>
> This is the output of the worktree list with locked marker:
>
>   $ git worktree list
>   /repo/to/main                abc123 [master]
>   /path/to/unlocked-worktree1  456def [brancha]
>   /path/to/locked-worktree     123abc (detached HEAD) (locked)


In our log message, we tend NOT to say "This commit does X" or "X is
done", because such a statement is often insufficient to illustrate
if the commit indeed does X, and explain why it is a good thing to
do X in the first place.

Instead, we 

 - first explain that the current system does not do X (in present
   tense, so we do NOT say "previously we did not do X"), then

 - explain why doing X would be a good thing, and finally

 - give an order to the codebase to start doing X.


For this change, it might look like this:

    The "git worktree list" shows the absolute path to the working
    tree, the commit that is checked out and the name of the branch.
    It is not immediately obvious which of the worktrees, if any,
    are locked.

    "git worktree remove" refuses to remove a locked worktree with
    an error message.  If "git worktree list" told which worktrees
    are locked in its output, the user would not even attempt to
    remove such a worktree.

    Teach "git worktree list" to append "(locked)" to its output.
    The output from the command becomes like so:

          $ git worktree list
          /repo/to/main                abc123 [master]
          /path/to/unlocked-worktree1  456def [brancha]
          /path/to/locked-worktree     123abc (detached HEAD) (locked)



> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 32e8440cde..a3781dd664 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -96,8 +96,9 @@ list::
>  
>  List details of each working tree.  The main working tree is listed first,
>  followed by each of the linked working trees.  The output details include
> -whether the working tree is bare, the revision currently checked out, and the
> -branch currently checked out (or "detached HEAD" if none).
> +whether the working tree is bare, the revision currently checked out, the
> +branch currently checked out (or "detached HEAD" if none), and whether
> +the worktree is locked.

At the first glance, the above gave me an impression that you'd be
adding "(unlocked)" or "(locked)" for each working tree, but that is
not the case.  How about keeping the original sentence intact, and
adding something like "For a locked worktree, the marker (locked) is
also shown at the end"?

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 99abaeec6c..8ad2cdd2f9 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -676,8 +676,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
>  		} else
>  			strbuf_addstr(&sb, "(error)");
>  	}
> -	printf("%s\n", sb.buf);
>  
> +	if (!is_main_worktree(wt) &&
> +	    worktree_lock_reason(wt))
> +		strbuf_addstr(&sb, " (locked)");

Is this because for the primary worktree, worktree_lock_reason()
will always yield true?

    ... goes and looks ...

Ah, OK, the callers are not even allowed to ask the question on the
primary one.  That's a bit strange API but OK.

Writing that on a single line would perfectly be readable, by the
way.

	if (!is_main_worktree(wt) && worktree_lock_reason(wt))
		strbuf_addstr(&sb, " (locked)");

> +	printf("%s\n", sb.buf);
>  	strbuf_release(&sb);
>  }

  reply	other threads:[~2020-09-28 21:38 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-28 15:49 [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees Rafael Silva
2020-09-28 15:49 ` [RFC PATCH 1/2] worktree: teach `list` to mark locked worktree Rafael Silva
2020-09-28 21:37   ` Junio C Hamano [this message]
2020-09-29 21:36     ` Rafael Silva
2020-09-30  7:35     ` Eric Sunshine
2020-09-28 15:49 ` [RFC PATCH 2/2] t2402: add test to locked linked worktree marker Rafael Silva
2020-09-28 21:54   ` Junio C Hamano
2020-09-29 21:37     ` Rafael Silva
2020-09-30  8:06   ` Eric Sunshine
2020-09-28 21:19 ` [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees Junio C Hamano
2020-09-29 21:35   ` Rafael Silva
2020-09-30  7:19 ` Eric Sunshine
2020-10-02 16:28   ` Rafael Silva
2020-10-09 22:50     ` Eric Sunshine
2020-10-10 19:06       ` Rafael Silva
2020-10-10 18:55 ` [PATCH v2 0/1] Teach "worktree list" to annotate " Rafael Silva
2020-10-10 18:55   ` [PATCH v2 1/1] worktree: teach `list` to annotate locked worktree Rafael Silva
2020-10-11  6:26     ` Eric Sunshine
2020-10-11  6:34       ` Eric Sunshine
2020-10-11 10:04       ` Rafael Silva
2020-10-11 10:11   ` [PATCH v3 0/1] Teach "worktree list" to annotate locked worktrees Rafael Silva
2020-10-11 10:11     ` [PATCH v3] worktree: teach `list` to annotate locked worktree Rafael Silva
2020-10-12  2:24       ` Eric Sunshine

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=xmqq8sctlgzx.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=rafaeloliveira.cs@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).