Skip to content

[GSoC] receive.denyCurrentBranch: respect all worktrees #535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
harry-hov wants to merge 3 commits into from
Closed

[GSoC] receive.denyCurrentBranch: respect all worktrees #535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8718fac
get_main_worktree(): allow it to be called in the Git directory
harry-hov Jan 30, 2020
ae74931
t5509: use a bare repository for test push target
harry-hov Feb 4, 2020
d21a590
receive.denyCurrentBranch: respect all worktrees
harry-hov Feb 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 20 additions & 17 deletions builtin/receive-pack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "object-store.h"
Copy link

@gitgitgadget gitgitgadget bot Feb 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
> index c89483fdba2..6270fb7b576 100755
> --- a/t/t5509-fetch-push-namespaces.sh
> +++ b/t/t5509-fetch-push-namespaces.sh
> @@ -152,7 +152,7 @@ test_expect_success 'clone chooses correct HEAD (v2)' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '
> +test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
>  	cd original &&
>  	git init unborn &&
>  	git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index c81ca360ac4..49982b0fd90 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
>  	)
>  '
>  
> +test_expect_success 'denyCurrentBranch and worktrees' '
> +	git worktree add new-wt &&
> +	git clone . cloned &&
> +	test_commit -C cloned first &&
> +	test_config receive.denyCurrentBranch refuse &&
> +	test_must_fail git -C cloned push origin HEAD:new-wt &&
> +	test_config receive.denyCurrentBranch updateInstead &&
> +	git -C cloned push origin HEAD:new-wt &&
> +	test_must_fail git -C cloned push --delete origin new-wt
> +'
> +
>  test_done

This adds one new test and also flips a test that was added in a
separate step that expected a failure to expect success, which looks
a bit strange.

For a series this small, having a test that demonstrates that the
updated code works as expected together with the fix to the code in
a single patch is easier to manage.  After applying a single
test+fix patch, you can easily apply the same patch except for the
test part in reverse on top, if you need to see in what way the code
without the change breaks by running the test.

On a truly large fix, sometimes it may make sense to add a failing
test and nothing else and then a separate step that changes the code
and flips the expectation of the test from failure->success, but I
think a change this size is easier to handle without such an artificial
split.

Thanks.

#include "protocol.h"
#include "commit-reach.h"
#include "worktree.h"

static const char * const receive_pack_usage[] = {
N_("git receive-pack <git-dir>"),
Expand Down Expand Up @@ -816,16 +817,6 @@ static int run_update_hook(struct command *cmd)
return finish_command(&proc);
}

static int is_ref_checked_out(const char *ref)
{
if (is_bare_repository())
return 0;

if (!head_name)
return 0;
return !strcmp(head_name, ref);
}

static char *refuse_unconfigured_deny_msg =
N_("By default, updating the current branch in a non-bare repository\n"
"is denied, because it will make the index and work tree inconsistent\n"
Expand Down Expand Up @@ -997,16 +988,27 @@ static const char *push_to_checkout(unsigned char *hash,
return NULL;
}

static const char *update_worktree(unsigned char *sha1)
static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
{
const char *retval;
const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : "..";
const char *retval, *work_tree, *git_dir = NULL;
struct argv_array env = ARGV_ARRAY_INIT;

if (worktree && worktree->path)
work_tree = worktree->path;
else if (git_work_tree_cfg)
work_tree = git_work_tree_cfg;
else
work_tree = "..";

if (is_bare_repository())
return "denyCurrentBranch = updateInstead needs a worktree";

if (worktree)
git_dir = get_worktree_git_dir(worktree);
if (!git_dir)
git_dir = get_git_dir();

argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));

if (!find_hook(push_to_checkout_hook))
retval = push_to_deploy(sha1, &env, work_tree);
Expand All @@ -1026,6 +1028,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
struct object_id *old_oid = &cmd->old_oid;
struct object_id *new_oid = &cmd->new_oid;
int do_update_worktree = 0;
const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name);

/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
Expand All @@ -1037,7 +1040,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
free(namespaced_name);
namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);

if (is_ref_checked_out(namespaced_name)) {
if (worktree) {
switch (deny_current_branch) {
case DENY_IGNORE:
break;
Expand Down Expand Up @@ -1069,7 +1072,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
return "deletion prohibited";
}

if (head_name && !strcmp(namespaced_name, head_name)) {
if (worktree || (head_name && !strcmp(namespaced_name, head_name))) {
switch (deny_delete_current) {
case DENY_IGNORE:
break;
Expand Down Expand Up @@ -1118,7 +1121,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
}

if (do_update_worktree) {
ret = update_worktree(new_oid->hash);
ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
if (ret)
return ret;
}
Expand Down
13 changes: 12 additions & 1 deletion t/t5509-fetch-push-namespaces.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ test_expect_success setup '
) &&
Copy link

@gitgitgadget gitgitgadget bot Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Hariom Verma <hariom18599@gmail.com>
>
> `receive.denyCurrentBranch` currently has a bug where it allows pushing
> into the current branch of a non-bare repository as long as it does not
> have any commits.

Can patch 3/3 be split into two, so that the fix would protect an
already populated branch that is checked out anywhere (not in the
primary worktree--which is the bug you are fixing) from getting
updated but still allow an unborn branch to be updated, and then
have patch 4/3 that forbids an update to even an unborn branch
"checked out" in any working tree?  This update to the test can then
become part of patch 4/3.

Thanks.

Copy link

@gitgitgadget gitgitgadget bot Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Hariom Verma <hariom18599@gmail.com>
>>
>> `receive.denyCurrentBranch` currently has a bug where it allows pushing
>> into the current branch of a non-bare repository as long as it does not
>> have any commits.
>
> Can patch 3/3 be split into two, so that the fix would protect an
> already populated branch that is checked out anywhere (not in the
> primary worktree--which is the bug you are fixing) from getting
> updated but still allow an unborn branch to be updated, and then
> have patch 4/3 that forbids an update to even an unborn branch
> "checked out" in any working tree?  This update to the test can then
> become part of patch 4/3.

Oh, another thing.  The patch 4/3 that starts forbidding a push into
a checked out unborn branch should also have a test that makes sure
that such an attempt fails.  IOW, making the test repository used in
the test you changed to a bare one, to allow existing test to still
test what it wants to test, like you did in this patch is OK, but we
would want to have a new test that prepares a repository with the
primary and the secondary worktrees, check out an unborn branch in
each of the worktrees, and make sure receive.denyCurrentBranch would
prevent "git push" to update these branches.

Thanks.

Copy link

@gitgitgadget gitgitgadget bot Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

On Thu, 13 Feb 2020, Junio C Hamano wrote:

> "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Hariom Verma <hariom18599@gmail.com>
> >
> > `receive.denyCurrentBranch` currently has a bug where it allows pushing
> > into the current branch of a non-bare repository as long as it does not
> > have any commits.
>
> Can patch 3/3 be split into two,

I actually don't think so. The `refs_resolve_unsafe()` function simply
requires a tip commit, so it is the wrong function to call in this
context. And the fix for it is to use a more appropriate function, which
3/3 already does (although for an unrelated reason).

In other words, a fix for one bug would be a fix for the other, and
(probably) vice versa.

> so that the fix would protect an already populated branch that is
> checked out anywhere (not in the primary worktree--which is the bug you
> are fixing) from getting updated but still allow an unborn branch to be
> updated, and then have patch 4/3 that forbids an update to even an
> unborn branch "checked out" in any working tree?  This update to the
> test can then become part of patch 4/3.

I agree that this merits a regression test.

Thanks,
Dscho

>
> Thanks.
>

Copy link

@gitgitgadget gitgitgadget bot Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Can patch 3/3 be split into two,
>
> I actually don't think so. The `refs_resolve_unsafe()` function simply
> ...
> In other words, a fix for one bug would be a fix for the other, and
> (probably) vice versa.

What mislead me was the way this step presented itself.  It sounded
as if the primary (and possibly the only) thing the series wanted to
fix was to make .denyCurrentBranch pay attention to other worktrees,
and while fixing that, it broke as collateral damage a "feature"
that denyCurrentBranch allows an unborn branch to be updated no
matter what and called it a bugfix when it was not a bug.

If the series is fixing two bugs, perhaps 2/3 can first fix it for a
primary worktree case by seeing what HEAD symref for the primary
worktree points at is the target of a push without iterating over
all the worktrees, have the test change in 2/3 (i.e. "fixing the
'unborn' case revealed a wrong expectation in an existing test"),
and a couple of new tests to see what a push from sideways would do
to an unborn branch that is checked out in the primary worktree when
.denyCurrentBranch is and isn't in effect.

Then 3/3 can use the same logic to see if one worktree is OK with
the proposed ref update by the push used in 2/3 (which no longer
uses refs_resolve_unsafe()') to check for all worktrees.  The new
tests introduced in 2/3 would be extended to see what happens when
the unborn branch getting updated by the push happens to be checked
out in a secondary worktree.

That would have avoided misleading this reader.

Thanks.

Copy link

@gitgitgadget gitgitgadget bot Feb 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Hariom verma wrote (reply to this):

On Fri, Feb 14, 2020 at 8:33 PM Junio C Hamano <gitster@pobox.com> wrote:
> If the series is fixing two bugs, perhaps 2/3 can first fix it for a
> primary worktree case by seeing what HEAD symref for the primary
> worktree points at is the target of a push without iterating over
> all the worktrees, have the test change in 2/3 (i.e. "fixing the
> 'unborn' case revealed a wrong expectation in an existing test"),
> and a couple of new tests to see what a push from sideways would do
> to an unborn branch that is checked out in the primary worktree when
> .denyCurrentBranch is and isn't in effect.
>
> Then 3/3 can use the same logic to see if one worktree is OK with
> the proposed ref update by the push used in 2/3 (which no longer
> uses refs_resolve_unsafe()') to check for all worktrees.  The new
> tests introduced in 2/3 would be extended to see what happens when
> the unborn branch getting updated by the push happens to be checked
> out in a secondary worktree.

As far as my understanding goes, what we want is:
1) fixing `.denyCurrentBranch` for unborn branches in primary worktree. (2/3)
2) writing test (expect it to fail if `unborn` & 'non-bare' case) (2/3)
3) making `.denyCurrentBranch` respect all worktrees. (3/3)
4) extending tests written in step 2 for secondary worktrees. (3/3)

Correct me if I'm wrong.
As I'm not entirely familiar with working and structure of
`.denyCurrentBranch`. So I might need more explicit explanation.

Thanks,
Hariom

Copy link

@gitgitgadget gitgitgadget bot Feb 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Hariom verma <hariom18599@gmail.com> writes:

> On Fri, Feb 14, 2020 at 8:33 PM Junio C Hamano <gitster@pobox.com> wrote:
>> If the series is fixing two bugs, perhaps 2/3 can first fix it for a
>> primary worktree case by seeing what HEAD symref for the primary
>> worktree points at is the target of a push without iterating over
>> all the worktrees, have the test change in 2/3 (i.e. "fixing the
>> 'unborn' case revealed a wrong expectation in an existing test"),
>> and a couple of new tests to see what a push from sideways would do
>> to an unborn branch that is checked out in the primary worktree when
>> .denyCurrentBranch is and isn't in effect.
>>
>> Then 3/3 can use the same logic to see if one worktree is OK with
>> the proposed ref update by the push used in 2/3 (which no longer
>> uses refs_resolve_unsafe()') to check for all worktrees.  The new
>> tests introduced in 2/3 would be extended to see what happens when
>> the unborn branch getting updated by the push happens to be checked
>> out in a secondary worktree.
>
> As far as my understanding goes, what we want is:
> 1) fixing `.denyCurrentBranch` for unborn branches in primary worktree. (2/3)
> 2) writing test (expect it to fail if `unborn` & 'non-bare' case) (2/3)
> 3) making `.denyCurrentBranch` respect all worktrees. (3/3)
> 4) extending tests written in step 2 for secondary worktrees. (3/3)
>
> Correct me if I'm wrong.

If the above is what _you_ want, then there is nothing for me to
correct ;-)

What I suggested was somewhat different, though.

  1) get_main_worktree() fix you have as [1/3] in the current round.

  2) fix `.denyCurrentBranch` for unborn branches in the primary
     worktree, new tests for the cases I outlined in the message you
     are responding to, and adjusting the test (i.e. what you have
     as [2/3] in the current round).

  3) fix `.denyCurrentBranch` to pay attention to HEAD of not just
     the primary worktree but of all the worktrees, and add tests.

Thanks.

Copy link

@gitgitgadget gitgitgadget bot Feb 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Hariom verma wrote (reply to this):

On Mon, Feb 17, 2020 at 5:19 AM Junio C Hamano <gitster@pobox.com> wrote:
> What I suggested was somewhat different, though.
>
>   1) get_main_worktree() fix you have as [1/3] in the current round.
>
>   2) fix `.denyCurrentBranch` for unborn branches in the primary
>      worktree, new tests for the cases I outlined in the message you
>      are responding to, and adjusting the test (i.e. what you have
>      as [2/3] in the current round).
>
>   3) fix `.denyCurrentBranch` to pay attention to HEAD of not just
>      the primary worktree but of all the worktrees, and add tests.

I doubt that it's possible to solve these 2 issues separately.
As dscho said: "a fix for one bug would be a fix for the other, and
(probably) vice versa."

As I discussed with dscho, the best possible solution for this
situation is to demonstrate the bug and fix it in succeeding commit.

I have sent this v2[1] for this patch.

Thanks,
Hariom

[1]: https://lore.kernel.org/git/pull.535.v2.git.1582410908.gitgitgadget@gmail.com/

commit0=$(cd original && git rev-parse HEAD^) &&
commit1=$(cd original && git rev-parse HEAD) &&
git init pushee &&
git init --bare pushee &&
git init puller
'

Expand Down Expand Up @@ -152,4 +152,15 @@ test_expect_success 'clone chooses correct HEAD (v2)' '
test_cmp expect actual
'

test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '
(
cd original &&
git init unborn &&
git remote add unborn-namespaced "ext::git --namespace=namespace %s unborn" &&
test_must_fail git push unborn-namespaced HEAD:master &&
git -C unborn config receive.denyCurrentBranch updateInstead &&
git push unborn-namespaced HEAD:master
)
'

test_done
11 changes: 11 additions & 0 deletions t/t5516-fetch-push.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '
)
'

test_expect_success 'denyCurrentBranch and worktrees' '
git worktree add new-wt &&
git clone . cloned &&
test_commit -C cloned first &&
test_config receive.denyCurrentBranch refuse &&
test_must_fail git -C cloned push origin HEAD:new-wt &&
test_config receive.denyCurrentBranch updateInstead &&
git -C cloned push origin HEAD:new-wt &&
test_must_fail git -C cloned push --delete origin new-wt
'

test_done
1 change: 1 addition & 0 deletions worktree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)
struct strbuf worktree_path = STRBUF_INIT;
Copy link

@gitgitgadget gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sun, Feb 23, 2020 at 1:57 PM Hariom Verma via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> get_main_worktree(): allow it to be called in the Git directory

This title is a bit too generic; it fails to explain what this patch
is really fixing. Perhaps:

    get_main_worktree: correctly normalize worktree path when in .git dir

or something.

> When called in the Git directory of a non-bare repository, this function
> would not return the directory of the main worktree, but of the Git
> directory instead.

"Git directory" is imprecise. As a reader, I can't tell if this means
the main worktree into which the project is checked out or the `.git`
directory itself. Please write it instead as "`.git` directory".

> The reason: when the Git directory is the current working directory, the
> absolute path of the common directory will be reported with a trailing
> `/.git/.`, which the code of `get_main_worktree()` does not handle
> correctly.
>
> Signed-off-by: Hariom Verma <hariom18599@gmail.com>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)
>         strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
> +       strbuf_strip_suffix(&worktree_path, "/.");
>         if (!strbuf_strip_suffix(&worktree_path, "/.git"))
>                 strbuf_strip_suffix(&worktree_path, "/.");

This change makes the code unnecessarily confusing and effectively
turns the final line into dead code. I would much rather see the three
cases spelled out explicitly, perhaps like this:

    if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
        !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
            strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */

Also, please add a test to ensure that this behavior doesn't regress
in the future. You can probably test it via the "git worktree list"
command, so perhaps add the test to t/t2402-worktree-list.sh.

Copy link

@gitgitgadget gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Hariom verma wrote (reply to this):

Hi Eric,

On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> This title is a bit too generic; it fails to explain what this patch
> is really fixing. Perhaps:
>
>     get_main_worktree: correctly normalize worktree path when in .git dir
>
> or something.
>
> "Git directory" is imprecise. As a reader, I can't tell if this means
> the main worktree into which the project is checked out or the `.git`
> directory itself. Please write it instead as "`.git` directory".
> [...]
> This change makes the code unnecessarily confusing and effectively
> turns the final line into dead code. I would much rather see the three
> cases spelled out explicitly, perhaps like this:
>
>     if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
>         !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
>             strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */

I'll implement these comments in the next revision for sure.

> Also, please add a test to ensure that this behavior doesn't regress
> in the future. You can probably test it via the "git worktree list"
> command, so perhaps add the test to t/t2402-worktree-list.sh.

There already exists tests in "t/t2402-worktree-list.sh" which lists and
verifies all worktrees. Does this make sense to write a new test that
also does kinda the same thing?

Thanks,
Hariom

Copy link

@gitgitgadget gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Feb 24, 2020 at 6:09 AM Hariom verma <hariom18599@gmail.com> wrote:
> On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >     if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
> >         !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
> >             strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */
> >
> > Also, please add a test to ensure that this behavior doesn't regress
> > in the future. You can probably test it via the "git worktree list"
> > command, so perhaps add the test to t/t2402-worktree-list.sh.
>
> There already exists tests in "t/t2402-worktree-list.sh" which lists and
> verifies all worktrees. Does this make sense to write a new test that
> also does kinda the same thing?

The change this patch is making is to correctly strip the suffix
"/.git/." from the main worktree's path since that suffix was not
getting stripped correctly by the existing code. We want a test that
verifies that the "/.git/." suffix is indeed being stripped once this
change is applied. If there is an existing test which already checks
the output of "git worktree list" when invoked from within the .git
directory, then that test should suffice, but then I would have
expected that you would have had to tweak the existing test to make it
succeed after this change. If there is no such test which verifies
that "/.git/." is being stripped, then this patch should add one. "git
worktree list" would be a possible way to implement such a test.

Copy link

@gitgitgadget gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi,

On Mon, 24 Feb 2020, Hariom verma wrote:

> On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >
> > This title is a bit too generic; it fails to explain what this patch
> > is really fixing. Perhaps:
> >
> >     get_main_worktree: correctly normalize worktree path when in .git dir
> >
> > or something.
> >
> > "Git directory" is imprecise. As a reader, I can't tell if this means
> > the main worktree into which the project is checked out or the `.git`
> > directory itself. Please write it instead as "`.git` directory".
> > [...]
> > This change makes the code unnecessarily confusing and effectively
> > turns the final line into dead code. I would much rather see the three
> > cases spelled out explicitly, perhaps like this:
> >
> >     if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
> >         !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
> >             strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */

I would be really cautious about that.

To me, the originally proposed change says: strip `/.`, if any. Then,
strip `/.git`, and if successful, strip another `/.`, if any.

That reads pretty fine to me. It makes sense.

Above-mentioned proposal, however, puts quite a few twists into my brain,
as is a "if neither X nor Y then Z", and I find the code comments outright
confusing.

> I'll implement these comments in the next revision for sure.

I'd like to suggest taking a step back and reflecting whether _you_ like
the suggested version better. It is just a suggestion, after all, and if
it was up to me, I would argue against it.

> > Also, please add a test to ensure that this behavior doesn't regress
> > in the future. You can probably test it via the "git worktree list"
> > command, so perhaps add the test to t/t2402-worktree-list.sh.
>
> There already exists tests in "t/t2402-worktree-list.sh" which lists and
> verifies all worktrees. Does this make sense to write a new test that
> also does kinda the same thing?

The scenario in which we found the buggy behavior involved calling
`find_shared_symref()`. I imagine that we could use `git branch -D` inside
the `.git` directory for the new regression test.

But yes, in my testing, `git worktree list` and `git -C .git worktree
list` do show a different top-level directory (the latter shows an
incorrect one). Such a test case would find a splendid home in t2402.

Ciao,
Dscho

Copy link

@gitgitgadget gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Hariom verma <hariom18599@gmail.com> writes:

> Hi Eric,
>
> On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>>
>> This title is a bit too generic; it fails to explain what this patch
>> is really fixing. Perhaps:
>>
>>     get_main_worktree: correctly normalize worktree path when in .git dir
>>
>> or something.
>>
>> "Git directory" is imprecise. As a reader, I can't tell if this means
>> the main worktree into which the project is checked out or the `.git`
>> directory itself. Please write it instead as "`.git` directory".
>> [...]
>> This change makes the code unnecessarily confusing and effectively
>> turns the final line into dead code. I would much rather see the three
>> cases spelled out explicitly, perhaps like this:
>>
>>     if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
>>         !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
>>             strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */
>
> I'll implement these comments in the next revision for sure.
>
>> Also, please add a test to ensure that this behavior doesn't regress
>> in the future. You can probably test it via the "git worktree list"
>> command, so perhaps add the test to t/t2402-worktree-list.sh.
>
> There already exists tests in "t/t2402-worktree-list.sh" which lists and
> verifies all worktrees. Does this make sense to write a new test that
> also does kinda the same thing?

I'd read Eric's suggestion as "please make sure we have a test to
ensure...".  If there already are tests that protects the behaviour
we care about here, there is no need to duplicate it.

Thanks for working on this topic.

Copy link

@gitgitgadget gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Feb 24, 2020 at 1:58 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> > On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > This change makes the code unnecessarily confusing and effectively
> > > turns the final line into dead code. I would much rather see the three
> > > cases spelled out explicitly, perhaps like this:
> > >
> > >     if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
> > >         !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
> > >             strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */
>
> I would be really cautious about that.
>
> To me, the originally proposed change says: strip `/.`, if any. Then,
> strip `/.git`, and if successful, strip another `/.`, if any.

That's not at all what the original said, which is reproduced here:

    if (!strbuf_strip_suffix(&worktree_path, "/.git"))
        strbuf_strip_suffix(&worktree_path, "/.");

It says "try stripping '/.git'; if that fails, try stripping '/.'".
That is, it recognizes and handles two distinct cases: (1) the path to
the .git directory of a non-bare repository, which always ends with
"/.git", and (2) the path to a bare git repository, which always ends
with "/.". So, the original code wasn't doing any sort of incremental
stripping of suffixes; it was just handling two known distinct cases.

Perhaps you missed the '!' in the conditional?

> That reads pretty fine to me. It makes sense.

To me, it doesn't make sense to update the code as done by the patch
since that just muddies the issue by making it seem as if
get_git_common_dir() is indeterminately tacking on various suffixes
rather than giving us deterministic results.

> Above-mentioned proposal, however, puts quite a few twists into my brain,
> as is a "if neither X nor Y then Z", and I find the code comments outright
> confusing.

It's just three distinct cases my proposed code is handling; there are
no twists.

> The scenario in which we found the buggy behavior involved calling
> `find_shared_symref()`. I imagine that we could use `git branch -D` inside
> the `.git` directory for the new regression test.
>
> But yes, in my testing, `git worktree list` and `git -C .git worktree
> list` do show a different top-level directory (the latter shows an
> incorrect one). Such a test case would find a splendid home in t2402.

I don't have strong feelings about how it is tested, but would like to
see some sort of test proving that it works as expected following this
change.

Copy link

@gitgitgadget gitgitgadget bot Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Eric,

On Mon, 24 Feb 2020, Eric Sunshine wrote:

> On Mon, Feb 24, 2020 at 1:58 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > > On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > > This change makes the code unnecessarily confusing and effectively
> > > > turns the final line into dead code. I would much rather see the three
> > > > cases spelled out explicitly, perhaps like this:
> > > >
> > > >     if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */
> > > >         !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */
> > > >             strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */
> >
> > I would be really cautious about that.
> >
> > To me, the originally proposed change says: strip `/.`, if any. Then,
> > strip `/.git`, and if successful, strip another `/.`, if any.
>
> That's not at all what the original said, which is reproduced here:
>
>     if (!strbuf_strip_suffix(&worktree_path, "/.git"))
>         strbuf_strip_suffix(&worktree_path, "/.");
>
> It says "try stripping '/.git'; if that fails, try stripping '/.'".
> That is, it recognizes and handles two distinct cases: (1) the path to
> the .git directory of a non-bare repository, which always ends with
> "/.git", and (2) the path to a bare git repository, which always ends
> with "/.". So, the original code wasn't doing any sort of incremental
> stripping of suffixes; it was just handling two known distinct cases.
>
> Perhaps you missed the '!' in the conditional?

I totally did. Sorry!

Ciao,
Dscho


strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
strbuf_strip_suffix(&worktree_path, "/.");
if (!strbuf_strip_suffix(&worktree_path, "/.git"))
strbuf_strip_suffix(&worktree_path, "/.");

Expand Down