possible to checkout same branch in different worktree

[Eric Sunshine]

On Tue, May 10, 2016 at 10:15 AM, Nguyễn Thái Ngọc Duy
<[hidden email]> wrote:
> strbuf is a bit overkill for this function. What we need is call

s/is call/is to call/

> absolute_path() twice and make sure the second call does not destroy the
> result of the first. One buffer allocation is enough.

The patch itself looks good.

> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -153,21 +153,19 @@ done:
>  static void mark_current_worktree(struct worktree **worktrees)
>  {
> -       struct strbuf git_dir = STRBUF_INIT;
> -       struct strbuf path = STRBUF_INIT;
> +       char *git_dir = xstrdup(absolute_path(get_git_dir()));
>         int i;
>
> -       strbuf_addstr(&git_dir, absolute_path(get_git_dir()));
>         for (i = 0; worktrees[i]; i++) {
>                 struct worktree *wt = worktrees[i];
> -               strbuf_addstr(&path, absolute_path(get_worktree_git_dir(wt)));
> -               wt->is_current = !fspathcmp(git_dir.buf, path.buf);
> -               strbuf_reset(&path);
> -               if (wt->is_current)
> +               const char *wt_git_dir = get_worktree_git_dir(wt);
> +
> +               if (!fspathcmp(git_dir, absolute_path(wt_git_dir))) {
> +                       wt->is_current = 1;
>                         break;
> +               }
>         }
> -       strbuf_release(&git_dir);
> -       strbuf_release(&path);
> +       free(git_dir);
>  }

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Sunshine]

On Tue, May 10, 2016 at 10:15 AM, Nguyễn Thái Ngọc Duy
<[hidden email]> wrote:
> The use case is keep some worktree and discard the rest of the worktree
> list.

So, you're saying that rather than a client freeing the entire
worktree list like this:

    free_worktrees(worktrees);

it might instead want to keep some element ('n') and free all others
plus the list itself, like this:

    struct worktree *keep = worktrees[n];
    struct worktree **p = worktrees;
    for (; *p; p++)
        if (*p != keep)
            clear_worktree(*p);
    free(worktrees);

Is that correct?

If so, then doesn't this require the client to have far too intimate
knowledge of the internals of free_worktrees(). Or, was your idea that
the client would simply leak the 'worktrees' array after freeing the
unwanted elements?

In either case, a cleaner approach might be to provide a function for
copying a worktree element. Perhaps:

    struct worktree *copy_worktree(const struct worktree *);

or something?

> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/worktree.c b/worktree.c
> index f4a4f38..335c58f 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -5,14 +5,22 @@
>  #include "dir.h"
>  #include "wt-status.h"
>
> +void clear_worktree(struct worktree *wt)
> +{
> +       if (!wt)
> +               return;
> +       free(wt->path);
> +       free(wt->id);
> +       free(wt->head_ref);
> +       memset(wt, 0, sizeof(*wt));
> +}
> +
>  void free_worktrees(struct worktree **worktrees)
>  {
>         int i = 0;
>
>         for (i = 0; worktrees[i]; i++) {
> -               free(worktrees[i]->path);
> -               free(worktrees[i]->id);
> -               free(worktrees[i]->head_ref);
> +               clear_worktree(worktrees[i]);
>                 free(worktrees[i]);
>         }
>         free (worktrees);
> diff --git a/worktree.h b/worktree.h
> index 1394909..7430a4e 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -29,6 +29,11 @@ extern struct worktree **get_worktrees(void);
>   */
>  extern const char *get_worktree_git_dir(const struct worktree *wt);
>
> +/*
> + * Free up the memory for worktree
> + */
> +extern void clear_worktree(struct worktree *);
> +
>  /*
>   * Free up the memory for worktree(s)
>   */
> --
> 2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Sunshine]

On Wed, May 11, 2016 at 2:36 AM, Eric Sunshine <[hidden email]> wrote:

> On Tue, May 10, 2016 at 10:15 AM, Nguyễn Thái Ngọc Duy
> <[hidden email]> wrote:
>> The use case is keep some worktree and discard the rest of the worktree
>> list.
>
> So, you're saying that rather than a client freeing the entire
> worktree list like this:
>
>     free_worktrees(worktrees);
>
> it might instead want to keep some element ('n') and free all others
> plus the list itself, like this:
>
>     struct worktree *keep = worktrees[n];
>     struct worktree **p = worktrees;
>     for (; *p; p++)
>         if (*p != keep)
>             clear_worktree(*p);
>     free(worktrees);
>
> Is that correct?
>
> If so, then doesn't this require the client to have far too intimate
> knowledge of the internals of free_worktrees(). Or, was your idea that
> the client would simply leak the 'worktrees' array after freeing the
> unwanted elements?
>
> In either case, a cleaner approach might be to provide a function for
> copying a worktree element. Perhaps:
>
>     struct worktree *copy_worktree(const struct worktree *);

Of course, you'd still need a clear_worktree() or free_worktree()
function to dispose of the copy.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Sunshine]

On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy
<[hidden email]> wrote:

> So far we haven't needed to identify an existing worktree from command
> line. Future commands such as lock or move will need it. There are of
> course other options for identifying a worktree, for example by branch
> or even by internal id. They may be added later if proved useful.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -222,6 +222,22 @@ const char *get_worktree_git_dir(const struct worktree *wt)
> +struct worktree *find_worktree_by_path(struct worktree **list,
> +                                      const char *path_)
> +{
> +       char *path = xstrdup(real_path(path_));
> +       struct worktree *wt = NULL;
> +
> +       while (*list) {
> +               wt = *list++;
> +               if (!fspathcmp(path, real_path(wt->path)))
> +                       break;
> +               wt = NULL;
> +       }
> +       free(path);
> +       return wt;
> +}

Very slightly shorter and perhaps idiomatic (but not at all worth a re-roll):

    for (; *list; list++)
        if (!fspathcmp(path, real_path((*list)->path)))
            break;
    free(path);
    return *list;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Sunshine]

On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy
<[hidden email]> wrote:

> Main worktree _is_ different. You can lock a linked worktree but not the
> main one, for example. Provide an API for checking that.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -238,6 +238,11 @@ struct worktree *find_worktree_by_path(struct worktree **list,
> +int is_main_worktree(const struct worktree *wt)
> +{
> +       return wt && !wt->id;
> +}

Is or will there ever be a case when it is valid for 'wt' to be NULL?
If not, perhaps assert() or die() if given NULL?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Sunshine]

On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy
<[hidden email]> wrote:

> This provides an API for checking if a worktree is locked. We need to
> check this to avoid double locking a worktree, or try to unlock one when
> it's not even locked.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -243,6 +243,24 @@ int is_main_worktree(const struct worktree *wt)
> +const char *is_worktree_locked(const struct worktree *wt)
> +{
> +       static struct strbuf sb = STRBUF_INIT;
> +
> +       if (!file_exists(git_common_path("worktrees/%s/locked", wt->id)))
> +               return NULL;

The git_common_path(...) invocation is textually lengthy and repeated
three times in this function. If you instead assign the result to a
variable (possibly xstrdup'ing it if needed), then the below
strbuf_read_file() would likely fit on one line, thus be easier to
read.

> +
> +       strbuf_reset(&sb);
> +       if (strbuf_read_file(&sb,
> +                            git_common_path("worktrees/%s/locked", wt->id),
> +                            0) < 0)

It's too bad that strbuf_read_file() doesn't guarantee anything about
'errno', such as indicating that the file did not exist, in which case
the !file_exists() check would not be needed, and a bit of raciness
eliminated, but that's outside the scope of this series.

> +               die_errno(_("failed to read '%s'"),
> +                         git_common_path("worktrees/%s/locked", wt->id));
> +
> +       strbuf_rtrim(&sb);

Since this file is presumably human-editable (historically and at this
point in the series) in order to specify the lock reason, should this
be doing a full trim() rather than only rtrim()?

> +       return sb.buf;
> +}
> diff --git a/worktree.h b/worktree.h
> @@ -40,6 +40,12 @@ extern struct worktree *find_worktree_by_path(struct worktree **list,
> +/*
> + * Return the reason string if the given worktree is locked. Return
> + * NULL otherwise.
> + */

Does this need to mention that the returned "locked reason" string is
only guaranteed valid until the next invocation?

Actually, I recall that when I suggested the idea of 'struct worktree'
and get_worktrees() to Mike that it would be natural for the structure
to have a 'locked' (or 'locked_reason') field, in which case the
reason could be stored there instead of in this static strbuf. In
fact, my idea at the time was that get_worktrees() would populate that
field automatically, rather than having to do so via a separate
on-demand function call as in this patch.

> +extern const char *is_worktree_locked(const struct worktree *wt);

I was wondering if builtin/worktree.c:prune_worktree() should be
updated to invoke this new function instead of consulting
"worktrees/<id>/locked" manually, but I see that the entire "prune
worktrees" functionality in builting/worktree.c first needs to be
updated to the get_worktrees() API for that to happen.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Sunshine]

On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy
<[hidden email]> wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
>  [verse]
>  'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
>  'git worktree list' [--porcelain]
> +'git worktree lock' [--reason <string>] <path>
>  'git worktree prune' [-n] [-v] [--expire <expire>]
>
>  DESCRIPTION

This forgets to update the paragraph in DESCRIPTION which currently
talks about manually creating the 'locked' file. Perhaps it can be
rewritten like this:

    If a linked working tree is stored on a portable device or
    network share which is not always mounted, you can prevent its
    administrative files from being pruned by issuing the `git
    worktree lock` command, optionally specifying `--reason` to
    explain why the working tree is locked.

The DETAILS section might also need a small update. It currently says:

    ...add a file named 'locked'...

but perhaps it should instead say:

    ...use the `git worktree lock` command, which adds a
    file named `locked`,...

> @@ -61,6 +62,12 @@ each of the linked worktrees.  The output details include if the worktree is
> +lock::
> +
> +When a worktree is locked, it cannot be pruned, moved or deleted. For
> +example, if the worktree is on portable device that is not available
> +when "git worktree <command>" is executed.

The really important reason for locking a worktree is to prevent its
administrative files from being pruned *automatically*. This
description doesn't properly emphasize that point.

Also, bc48328 (Documentation/git-worktree: consistently use term
"linked working tree", 2015-07-20) comprehensively replaced the term
"worktree" with "working tree" and, although some new instances
slipped in with bb9c03b (worktree: add 'list' command, 2015-10-08), we
should probably avoid adding more.

Perhaps the description of "lock" can be rewritten like this:

    If a working tree is on a portable device or network share which
    is not always mounted, lock it to prevent its administrative
    files from being pruned automatically. This also prevents it from
    being moved or deleted. Optionally, specify a reason for the lock
    with `--reason`.

I guess preventing it from being deleted when locked is a safety measure?

> @@ -110,6 +117,9 @@ OPTIONS
>  --expire <time>::
>         With `prune`, only expire unused working trees older than <time>.
>
> +--reason <string>:
> +       An explanation why the worktree is locked.

For consistency with other option descriptions, perhaps:

    With `lock`, an explanation ...

though I don't feel strongly about it.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -459,6 +460,44 @@ static int list(int ac, const char **av, const char *prefix)
> +static int lock_worktree(int ac, const char **av, const char *prefix)
> +{
> +       const char *reason = "", *old_reason;
> +       struct option options[] = {
> +               OPT_STRING(0, "reason", &reason, N_("string"),
> +                          N_("reason for locking")),
> +               OPT_END()
> +       };
> +       struct worktree **worktrees, *wt;
> +       struct strbuf dst = STRBUF_INIT;
> +
> +       ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +       if (ac != 1)
> +               usage_with_options(worktree_usage, options);
> +
> +       strbuf_addstr(&dst, prefix_filename(prefix,
> +                                           strlen(prefix),
> +                                           av[0]));
> +
> +       worktrees = get_worktrees();
> +       wt = find_worktree_by_path(worktrees, dst.buf);
> +       if (!wt)
> +               die(_("'%s' is not a working directory"), av[0]);
> +       if (is_main_worktree(wt))
> +               die(_("'%s' is a main working directory"), av[0]);
> +
> +       old_reason = is_worktree_locked(wt);
> +       if (old_reason) {
> +               if (*old_reason)
> +                       die(_("already locked, reason: %s"), old_reason);
> +               die(_("already locked, no reason"));
> +       }

As a convenience, would it make sense to allow a worktree to be
re-locked with a different reason rather than erroring out?

Also, the "no reason" in the error message might be confusing. Perhaps
it would be better to say merely "already locked" in this case.

> +       write_file(git_common_path("worktrees/%s/locked", wt->id),
> +                  "%s", reason);
> +       return 0;
> +}

This is a tangent, but it would be nice for the "git worktree list"
command to show whether a worktree is locked, and the reason (if
available), both in pretty and porcelain formats. (That was another
reason why I suggested to Mike, back when he was adding the "list"
command, that 'struct worktree' should have a 'locked' field and
get_worktrees() should populate it automatically.)

> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='test git worktree move, remove, lock and unlock'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +       test_commit init &&
> +       git worktree add source &&
> +       git worktree list --porcelain | grep "^worktree" >actual &&
> +       cat <<-EOF >expected &&
> +       worktree $TRASH_DIRECTORY
> +       worktree $TRASH_DIRECTORY/source
> +       EOF
> +       test_cmp expected actual
> +'
> +
> +test_expect_success 'lock main worktree' '
> +       test_must_fail git worktree lock .
> +'
> +
> +test_expect_success 'lock linked worktree' '
> +       git worktree lock --reason hahaha source &&
> +       echo hahaha >expected &&
> +       test_cmp expected .git/worktrees/source/locked
> +'

Would it make sense to also add tests of locking from within the
worktree being locked, and from within a worktree other than the one
being locked (and other than the main one)?

> +test_expect_success 'lock worktree twice' '
> +       test_must_fail git worktree lock source &&
> +       echo hahaha >expected &&
> +       test_cmp expected .git/worktrees/source/locked
> +'
> +
> +test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Sunshine]

On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy
<[hidden email]> wrote:

> +static int lock_worktree(int ac, const char **av, const char *prefix)
> +{
> +       const char *reason = "", *old_reason;
> +       struct option options[] = {
> +               OPT_STRING(0, "reason", &reason, N_("string"),
> +                          N_("reason for locking")),
> +               OPT_END()
> +       };
> +       struct worktree **worktrees, *wt;
> +       struct strbuf dst = STRBUF_INIT;
> +
> +       ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +       if (ac != 1)
> +               usage_with_options(worktree_usage, options);
> +
> +       strbuf_addstr(&dst, prefix_filename(prefix,
> +                                           strlen(prefix),
> +                                           av[0]));
> +
> +       worktrees = get_worktrees();
> +       wt = find_worktree_by_path(worktrees, dst.buf);

Oh, I forgot to mention in my previous email that this is leaking
'strbuf dst'. It could be released right here at its point of final
use.

> +       if (!wt)
> +               die(_("'%s' is not a working directory"), av[0]);
> +       if (is_main_worktree(wt))
> +               die(_("'%s' is a main working directory"), av[0]);
> +
> +       old_reason = is_worktree_locked(wt);
> +       if (old_reason) {
> +               if (*old_reason)
> +                       die(_("already locked, reason: %s"), old_reason);
> +               die(_("already locked, no reason"));
> +       }
> +
> +       write_file(git_common_path("worktrees/%s/locked", wt->id),
> +                  "%s", reason);
> +       return 0;
> +}

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Eric Sunshine]

On Tue, May 10, 2016 at 10:17 AM, Nguyễn Thái Ngọc Duy
<[hidden email]> wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -498,6 +499,34 @@ static int lock_worktree(int ac, const char **av, const char *prefix)
> +static int unlock_worktree(int ac, const char **av, const char *prefix)
> +{
> +       struct option options[] = {
> +               OPT_END()
> +       };
> +       struct worktree **worktrees, *wt;
> +       struct strbuf dst = STRBUF_INIT;
> +
> +       ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +       if (ac != 1)
> +               usage_with_options(worktree_usage, options);
> +
> +       strbuf_addstr(&dst, prefix_filename(prefix,
> +                                           strlen(prefix),
> +                                           av[0]));
> +
> +       worktrees = get_worktrees();
> +       wt = find_worktree_by_path(worktrees, dst.buf);

strbuf_release(&dst);

> +       if (!wt)
> +               die(_("'%s' is not a working directory"), av[0]);
> +       if (is_main_worktree(wt))
> +               die(_("'%s' is a main working directory"), av[0]);
> +       if (!is_worktree_locked(wt))
> +               die(_("not locked"));
> +
> +       return unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

On Wed, May 11, 2016 at 1:36 PM, Eric Sunshine <[hidden email]> wrote:

> On Tue, May 10, 2016 at 10:15 AM, Nguyễn Thái Ngọc Duy
> <[hidden email]> wrote:
>> The use case is keep some worktree and discard the rest of the worktree
>> list.
>
> So, you're saying that rather than a client freeing the entire
> worktree list like this:
>
>     free_worktrees(worktrees);
>
> it might instead want to keep some element ('n') and free all others
> plus the list itself, like this:
>
>     struct worktree *keep = worktrees[n];
>     struct worktree **p = worktrees;
>     for (; *p; p++)
>         if (*p != keep)
>             clear_worktree(*p);
>     free(worktrees);
>
> Is that correct?
>
> If so, then doesn't this require the client to have far too intimate
> knowledge of the internals of free_worktrees(). Or, was your idea that
> the client would simply leak the 'worktrees' array after freeing the
> unwanted elements?

I looked back in my trees to see why I needed it in the first place.
It turns out my explanation was a lie. I needed it for refactoring
builtin/worktree.c:add_worktree(), where I initialized struct worktree
separately, not from get_worktrees() [1]. Because it's not clear of
the exact use case for this commit, and because that commit I
mentioned may take even longer time to be here (it adds "git worktree
move --repository", a lot more complex operation than a "git worktree
move"), I'll take this commit out of the series for now.

[1] https://github.com/pclouds/git/commit/528d81ce5609b5fbbe1ba193389f7aeecc83d992

> In either case, a cleaner approach might be to provide a function for
> copying a worktree element. Perhaps:
>
>     struct worktree *copy_worktree(const struct worktree *);
>
> or something?
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

This is mostly commit message update plus

 - dropping "--force" from the completion patch (a comment from Szeder
   Gabor in a previous attempt of worktree completion support).
 
 - the removal of clear_worktree() patch.

I keep the completion patch anyway even though two versions of it were
posted by Eric and Szeder (and didn't get discussed much) because I
needed this for new commands and because I didn't aim high. It's meant
to provide very basic completion support. Fancy stuff like ref
completion can be added later by people who are more experienced in
bash completion stuff.

Anyway, interdiff

-- 8< --
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d3ac391..951a186 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2604,7 +2604,7 @@ _git_worktree ()
  else
  case "$subcommand,$cur" in
  add,--*)
- __gitcomp "--detach --force"
+ __gitcomp "--detach"
  ;;
  list,--*)
  __gitcomp "--porcelain"
diff --git a/worktree.c b/worktree.c
index 335c58f..f4a4f38 100644
--- a/worktree.c
+++ b/worktree.c
@@ -5,22 +5,14 @@
 #include "dir.h"
 #include "wt-status.h"
 
-void clear_worktree(struct worktree *wt)
-{
- if (!wt)
- return;
- free(wt->path);
- free(wt->id);
- free(wt->head_ref);
- memset(wt, 0, sizeof(*wt));
-}
-
 void free_worktrees(struct worktree **worktrees)
 {
  int i = 0;
 
  for (i = 0; worktrees[i]; i++) {
- clear_worktree(worktrees[i]);
+ free(worktrees[i]->path);
+ free(worktrees[i]->id);
+ free(worktrees[i]->head_ref);
  free(worktrees[i]);
  }
  free (worktrees);
diff --git a/worktree.h b/worktree.h
index 7430a4e..1394909 100644
--- a/worktree.h
+++ b/worktree.h
@@ -29,11 +29,6 @@ extern struct worktree **get_worktrees(void);
  */
 extern const char *get_worktree_git_dir(const struct worktree *wt);
 
-/*
- * Free up the memory for worktree
- */
-extern void clear_worktree(struct worktree *);
-
 /*
  * Free up the memory for worktree(s)
  */
-- 8< --

Nguyễn Thái Ngọc Duy (6):
  completion: support git-worktree
  worktree.c: rewrite mark_current_worktree() to avoid strbuf
  git-worktree.txt: keep subcommand listing in alphabetical order
  worktree.c: use is_dot_or_dotdot()
  worktree: avoid 0{40}, too many zeroes, hard to read
  worktree: simplify prefixing paths

 Documentation/git-worktree.txt         | 10 +++++-----
 builtin/worktree.c                     | 10 ++++++----
 contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++
 worktree.c                             | 18 ++++++++----------
 4 files changed, 42 insertions(+), 19 deletions(-)

--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

This adds bare-bone completion support for git-worktree. More advanced
completion (e.g. ref completion in git-worktree-add) can be added later.

--force completion in "worktree add" is left out because that option
should be handled with care.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3402475..951a186 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2595,6 +2595,29 @@ _git_whatchanged ()
  _git_log
 }
 
+_git_worktree ()
+{
+ local subcommands="add list prune"
+ local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ if [ -z "$subcommand" ]; then
+ __gitcomp "$subcommands"
+ else
+ case "$subcommand,$cur" in
+ add,--*)
+ __gitcomp "--detach"
+ ;;
+ list,--*)
+ __gitcomp "--porcelain"
+ ;;
+ prune,--*)
+ __gitcomp "--dry-run --expire --verbose"
+ ;;
+ *)
+ ;;
+ esac
+ fi
+}
+
 __git_main ()
 {
  local i c=1 command __git_dir
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

strbuf is a bit overkill for this function. What we need is to call
absolute_path() twice and make sure the second call does not destroy the
result of the first. One buffer allocation is enough.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 worktree.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/worktree.c b/worktree.c
index 4817d60..6a11611 100644
--- a/worktree.c
+++ b/worktree.c
@@ -153,21 +153,19 @@ done:
 
 static void mark_current_worktree(struct worktree **worktrees)
 {
- struct strbuf git_dir = STRBUF_INIT;
- struct strbuf path = STRBUF_INIT;
+ char *git_dir = xstrdup(absolute_path(get_git_dir()));
  int i;
 
- strbuf_addstr(&git_dir, absolute_path(get_git_dir()));
  for (i = 0; worktrees[i]; i++) {
  struct worktree *wt = worktrees[i];
- strbuf_addstr(&path, absolute_path(get_worktree_git_dir(wt)));
- wt->is_current = !fspathcmp(git_dir.buf, path.buf);
- strbuf_reset(&path);
- if (wt->is_current)
+ const char *wt_git_dir = get_worktree_git_dir(wt);
+
+ if (!fspathcmp(git_dir, absolute_path(wt_git_dir))) {
+ wt->is_current = 1;
  break;
+ }
  }
- strbuf_release(&git_dir);
- strbuf_release(&path);
+ free(git_dir);
 }
 
 struct worktree **get_worktrees(void)
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

This is probably not the best order. But it makes it no-brainer to know
where to insert new commands. At some point we might want to reorder at
least the synopsis part again, grouping commonly use subcommands together.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 Documentation/git-worktree.txt | 10 +++++-----
 builtin/worktree.c             |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index c622345..27feff6 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -10,8 +10,8 @@ SYNOPSIS
 --------
 [verse]
 'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
-'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree list' [--porcelain]
+'git worktree prune' [-n] [-v] [--expire <expire>]
 
 DESCRIPTION
 -----------
@@ -54,10 +54,6 @@ If `<branch>` is omitted and neither `-b` nor `-B` nor `--detached` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
 as if `-b $(basename <path>)` was specified.
 
-prune::
-
-Prune working tree information in $GIT_DIR/worktrees.
-
 list::
 
 List details of each worktree.  The main worktree is listed first, followed by
@@ -65,6 +61,10 @@ each of the linked worktrees.  The output details include if the worktree is
 bare, the revision currently checked out, and the branch currently checked out
 (or 'detached HEAD' if none).
 
+prune::
+
+Prune working tree information in $GIT_DIR/worktrees.
+
 OPTIONS
 -------
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 12c0af7..bf80111 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -13,8 +13,8 @@
 
 static const char * const worktree_usage[] = {
  N_("git worktree add [<options>] <path> [<branch>]"),
- N_("git worktree prune [<options>]"),
  N_("git worktree list [<options>]"),
+ N_("git worktree prune [<options>]"),
  NULL
 };
 
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 builtin/worktree.c | 2 +-
 worktree.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index bf80111..aaee0e2 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -95,7 +95,7 @@ static void prune_worktrees(void)
  if (!dir)
  return;
  while ((d = readdir(dir)) != NULL) {
- if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+ if (is_dot_or_dotdot(d->d_name))
  continue;
  strbuf_reset(&reason);
  if (!prune_worktree(d->d_name, &reason))
diff --git a/worktree.c b/worktree.c
index 6a11611..f4a4f38 100644
--- a/worktree.c
+++ b/worktree.c
@@ -187,7 +187,7 @@ struct worktree **get_worktrees(void)
  if (dir) {
  while ((d = readdir(dir)) != NULL) {
  struct worktree *linked = NULL;
- if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+ if (is_dot_or_dotdot(d->d_name))
  continue;
 
  if ((linked = get_linked_worktree(d->d_name))) {
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 builtin/worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index aaee0e2..b53f802 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -262,7 +262,7 @@ static int add_worktree(const char *path, const char *refname,
  */
  strbuf_reset(&sb);
  strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
- write_file(sb.buf, "0000000000000000000000000000000000000000");
+ write_file(sb.buf, sha1_to_hex(null_sha1));
  strbuf_reset(&sb);
  strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
  write_file(sb.buf, "../..");
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

This also makes slash conversion always happen on Windows (a side effect
of prefix_filename). Which is a good thing.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 builtin/worktree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index b53f802..f9dac37 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -337,7 +337,7 @@ static int add(int ac, const char **av, const char *prefix)
  if (ac < 1 || ac > 2)
  usage_with_options(worktree_usage, options);
 
- path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
+ path = prefix_filename(prefix, strlen(prefix), av[0]);
  branch = ac < 2 ? "HEAD" : av[1];
 
  opts.force_new_branch = !!new_branch_force;
@@ -467,6 +467,8 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
 
  if (ac < 2)
  usage_with_options(worktree_usage, options);
+ if (!prefix)
+ prefix = "";
  if (!strcmp(av[1], "add"))
  return add(ac - 1, av + 1, prefix);
  if (!strcmp(av[1], "prune"))
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

On Fri, May 13, 2016 at 11:52 PM, Eric Sunshine <[hidden email]> wrote:
> Actually, I recall that when I suggested the idea of 'struct worktree'
> and get_worktrees() to Mike that it would be natural for the structure
> to have a 'locked' (or 'locked_reason') field, in which case the
> reason could be stored there instead of in this static strbuf. In
> fact, my idea at the time was that get_worktrees() would populate that
> field automatically, rather than having to do so via a separate
> on-demand function call as in this patch.

I'm keeping this as a separate function for now. I don't like
get_worktrees() doing extra work unless it has to. We soon will see
the complete picture of "git worktree" then we can merge it back to
get_worktrees() if it turns out checking "locked" is the common
operation. is_worktree_locked() then may become a thin wrapper to
access this "locked" field.

>> +extern const char *is_worktree_locked(const struct worktree *wt);
>
> I was wondering if builtin/worktree.c:prune_worktree() should be
> updated to invoke this new function instead of consulting
> "worktrees/<id>/locked" manually, but I see that the entire "prune
> worktrees" functionality in builting/worktree.c first needs to be
> updated to the get_worktrees() API for that to happen.

I thought about updating prune too. But it is in a bit special
situation where it may need to consider invalid (or partly invalid)
worktrees as well. So far worktree api is about valid worktrees only
if I'm not mistaken and we probably should keep it that way, otherwise
all callers may need to check "is this worktree valid" all over the
place.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

(the answer to rest of your questions is "yes you're right, will fix"
or something along that line so I will not quote them here)

On Mon, May 16, 2016 at 7:09 AM, Eric Sunshine <[hidden email]> wrote:
>> +       old_reason = is_worktree_locked(wt);
>> +       if (old_reason) {
>> +               if (*old_reason)
>> +                       die(_("already locked, reason: %s"), old_reason);
>> +               die(_("already locked, no reason"));
>> +       }
>
> As a convenience, would it make sense to allow a worktree to be
> re-locked with a different reason rather than erroring out?

For now I think the user can just unlock then relock, after examining
the previous reason and maybe incorporating it to the new reason. Yes
there is a race but for these operations I don't think it really
matters. If it's done often enough, we can lift this restriction.

>> +       write_file(git_common_path("worktrees/%s/locked", wt->id),
>> +                  "%s", reason);
>> +       return 0;
>> +}
>
> This is a tangent, but it would be nice for the "git worktree list"
> command to show whether a worktree is locked, and the reason (if
> available), both in pretty and porcelain formats. (That was another
> reason why I suggested to Mike, back when he was adding the "list"
> command, that 'struct worktree' should have a 'locked' field and
> get_worktrees() should populate it automatically.)

Yes. And a good reason for get_worktrees() to read lock status
automatically too. I will not do it right away though or at least
until after I'm done with move/remove and the shared config file
problem.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Duy Nguyen]

This should address all of Eric's comments (thanks!). An extra change
I made is free_worktrees() at the end of {,un}lock_worktree() to avoid
leaking. This series depends on nd/worktree-cleanup-post-head-protection.

Interdiff
-- 8< --
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9ac1129..e0f2605 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -40,9 +40,8 @@ section "DETAILS" for more information.
 
 If a linked working tree is stored on a portable device or network share
 which is not always mounted, you can prevent its administrative files from
-being pruned by creating a file named 'locked' alongside the other
-administrative files, optionally containing a plain text reason that
-pruning should be suppressed. See section "DETAILS" for more information.
+being pruned by issuing the `git worktree lock` command, optionally
+specifying `--reason` to explain why the working tree is locked.
 
 COMMANDS
 --------
@@ -65,9 +64,11 @@ bare, the revision currently checked out, and the branch currently checked out
 
 lock::
 
-When a worktree is locked, it cannot be pruned, moved or deleted. For
-example, if the worktree is on portable device that is not available
-when "git worktree <command>" is executed.
+If a working tree is on a portable device or network share which
+is not always mounted, lock it to prevent its administrative
+files from being pruned automatically. This also prevents it from
+being moved or deleted. Optionally, specify a reason for the lock
+with `--reason`.
 
 prune::
 
@@ -123,7 +124,7 @@ OPTIONS
  With `prune`, only expire unused working trees older than <time>.
 
 --reason <string>:
- An explanation why the worktree is locked.
+ With `lock`, an explanation why the worktree is locked.
 
 DETAILS
 -------
@@ -165,7 +166,8 @@ instead.
 
 To prevent a $GIT_DIR/worktrees entry from being pruned (which
 can be useful in some situations, such as when the
-entry's working tree is stored on a portable device), add a file named
+entry's working tree is stored on a portable device), use the
+`git worktree lock` comamnd, which adds a file named
 'locked' to the entry's directory. The file contains the reason in
 plain text. For example, if a linked working tree's `.git` file points
 to `/path/main/.git/worktrees/test-next` then a file named
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 53e5f5a..da9f771 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -482,6 +482,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix)
 
  worktrees = get_worktrees();
  wt = find_worktree_by_path(worktrees, dst.buf);
+ strbuf_release(&dst);
  if (!wt)
  die(_("'%s' is not a working directory"), av[0]);
  if (is_main_worktree(wt))
@@ -491,11 +492,12 @@ static int lock_worktree(int ac, const char **av, const char *prefix)
  if (old_reason) {
  if (*old_reason)
  die(_("already locked, reason: %s"), old_reason);
- die(_("already locked, no reason"));
+ die(_("already locked"));
  }
 
  write_file(git_common_path("worktrees/%s/locked", wt->id),
    "%s", reason);
+ free_worktrees(worktrees);
  return 0;
 }
 
@@ -506,6 +508,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
  };
  struct worktree **worktrees, *wt;
  struct strbuf dst = STRBUF_INIT;
+ int ret;
 
  ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
  if (ac != 1)
@@ -517,14 +520,16 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 
  worktrees = get_worktrees();
  wt = find_worktree_by_path(worktrees, dst.buf);
+ strbuf_release(&dst);
  if (!wt)
  die(_("'%s' is not a working directory"), av[0]);
  if (is_main_worktree(wt))
  die(_("'%s' is a main working directory"), av[0]);
  if (!is_worktree_locked(wt))
  die(_("not locked"));
-
- return unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
+ ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
+ free_worktrees(worktrees);
+ return ret;
 }
 
 int cmd_worktree(int ac, const char **av, const char *prefix)
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index f4b2816..1927537 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -25,12 +25,32 @@ test_expect_success 'lock linked worktree' '
  test_cmp expected .git/worktrees/source/locked
 '
 
+test_expect_success 'lock linked worktree from another worktree' '
+ rm .git/worktrees/source/locked &&
+ git worktree add elsewhere &&
+ (
+ cd elsewhere &&
+ git worktree lock --reason hahaha ../source
+ ) &&
+ echo hahaha >expected &&
+ test_cmp expected .git/worktrees/source/locked
+'
+
 test_expect_success 'lock worktree twice' '
  test_must_fail git worktree lock source &&
  echo hahaha >expected &&
  test_cmp expected .git/worktrees/source/locked
 '
 
+test_expect_success 'lock worktree twice (from the locked worktree)' '
+ (
+ cd source &&
+ test_must_fail git worktree lock .
+ ) &&
+ echo hahaha >expected &&
+ test_cmp expected .git/worktrees/source/locked
+'
+
 test_expect_success 'unlock main worktree' '
  test_must_fail git worktree unlock .
 '
diff --git a/worktree.c b/worktree.c
index a9cfbb3..cb30af3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -218,38 +218,37 @@ struct worktree *find_worktree_by_path(struct worktree **list,
        const char *path_)
 {
  char *path = xstrdup(real_path(path_));
- struct worktree *wt = NULL;
 
- while (*list) {
- wt = *list++;
- if (!fspathcmp(path, real_path(wt->path)))
+ for (; *list; list++)
+ if (!fspathcmp(path, real_path((*list)->path)))
  break;
- wt = NULL;
- }
  free(path);
- return wt;
+ return *list;
 }
 
 int is_main_worktree(const struct worktree *wt)
 {
- return wt && !wt->id;
+ return !wt->id;
 }
 
 const char *is_worktree_locked(const struct worktree *wt)
 {
  static struct strbuf sb = STRBUF_INIT;
+ struct strbuf path = STRBUF_INIT;
+
+ strbuf_git_common_path(&path, "worktrees/%s/locked", wt->id);
 
- if (!file_exists(git_common_path("worktrees/%s/locked", wt->id)))
+ if (!file_exists(path.buf)) {
+ strbuf_release(&path);
  return NULL;
+ }
 
  strbuf_reset(&sb);
- if (strbuf_read_file(&sb,
-     git_common_path("worktrees/%s/locked", wt->id),
-     0) < 0)
- die_errno(_("failed to read '%s'"),
-  git_common_path("worktrees/%s/locked", wt->id));
+ if (strbuf_read_file(&sb, path.buf, 0) < 0)
+ die_errno(_("failed to read '%s'"), path.buf);
+ strbuf_release(&path);
 
- strbuf_rtrim(&sb);
+ strbuf_trim(&sb);
  return sb.buf;
 }
 
diff --git a/worktree.h b/worktree.h
index 19a6790..3a780c2 100644
--- a/worktree.h
+++ b/worktree.h
@@ -42,7 +42,7 @@ extern int is_main_worktree(const struct worktree *wt);
 
 /*
  * Return the reason string if the given worktree is locked. Return
- * NULL otherwise.
+ * NULL otherwise. Return value is only valid until the next invocation.
  */
 extern const char *is_worktree_locked(const struct worktree *wt);
-- 8< --

Nguyễn Thái Ngọc Duy (5):
  worktree.c: add find_worktree_by_path()
  worktree.c: add is_main_worktree()
  worktree.c: add is_worktree_locked()
  worktree: add "lock" command
  worktree: add "unlock" command

 Documentation/git-worktree.txt         | 27 +++++++++---
 builtin/worktree.c                     | 77 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  5 ++-
 t/t2028-worktree-move.sh (new +x)      | 68 ++++++++++++++++++++++++++++++
 worktree.c                             | 38 +++++++++++++++++
 worktree.h                             | 17 ++++++++
 6 files changed, 225 insertions(+), 7 deletions(-)
 create mode 100755 t/t2028-worktree-move.sh

--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html