Skip to content

checkout/reset/read-tree: fix --recurse-submodules in linked worktree #523

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.

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

Conversation

phil-blain
Copy link

@phil-blain phil-blain commented Jan 13, 2020

Changes since v1:

  • revert the addition of a dash in "file1 updated" by correctly using the 'tag' argument of test_commit
  • improve the commit message for the 3rd patch, as suggested by Eric
  • fix spacing when redirecting into 'expect' and 'actual'
  • harden the tests by echoing expected values into expect, as suggested by Stolee
    (I did that in both tests)
  • remove test_might_fail and use test_expect_code
  • add the 'git log' test suggested by Stolee

v1:
This series fixes the behaviour of git checkout/reset/read-tree --recurse-submodules when they are called in a linked worktree (created by git worktree add).

Although submodules are cloned in $GIT_COMMON_DIR/worktrees/<name>/modules upon issuing git submodule update in the linked worktree, any invocation of git checkout/reset/read-tree --recurse-submodules that changes the state of the submodule(s) will incorrectly operate on the repositories of the submodules in the main worktree, i.e. the ones at $GIT_COMMON_DIR/modules/.

The fourth patch fixes this behaviour by using get_git_dir() instead of git_common_dir() in submodule.c::submodule_move_head and submodule.c::submodule_unset_core_worktree to construct the path to the submodule repository.

The first 3 patches are clean-up patches on t7410-submodule-checkout-to.sh (renamed to t2405-worktree-submodule.sh) to bring it up to date.

Cc:Max Kirillov [email protected] Brandon Williams [email protected] Jonathan Tan [email protected] Stefan Beller [email protected] Nguyễn Thái Ngọc Duy [email protected] Eric Sunshine [email protected] Derrick Stolee [email protected]

@phil-blain phil-blain force-pushed the checkout-recurse-in-linked-worktree branch from bef3333 to 72cdb2f Compare January 13, 2020 12:18
@phil-blain
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 17, 2020

Submitted as [email protected]

@@ -1811,7 +1811,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
void submodule_unset_core_worktree(const struct submodule *sub)
Copy link

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, Derrick Stolee wrote (reply to this):

On 1/17/2020 7:23 AM, Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <[email protected]>
> 
> Ever since df56607dff (git-common-dir: make "modules/"
> per-working-directory directory, 2014-11-30), submodules in linked worktrees
> are cloned to $GIT_DIR/modules, i.e. $GIT_COMMON_DIR/worktrees/<name>/modules.
> 
> However, this convention was not followed when the worktree updater commands
> checkout, reset and read-tree learned to recurse into submodules. Specifically,
> submodule.c::submodule_move_head, introduced in 6e3c1595c6 (update submodules:
> add submodule_move_head, 2017-03-14) and submodule.c::submodule_unset_core_worktree,
> (re)introduced in 898c2e65b7 (submodule: unset core.worktree if no working tree
> is present, 2018-12-14) use get_git_common_dir() instead of get_git_dir()
> to get the path of the submodule repository.
> 
> This means that, for example, 'git checkout --recurse-submodules <branch>'
> in a linked worktree will correctly checkout <branch>, detach the submodule's HEAD
> at the commit recorded in <branch> and update the submodule working tree, but the
> submodule HEAD that will be moved is the one in $GIT_COMMON_DIR/modules/<name>/,
> i.e. the submodule repository of the main superproject working tree.
> It will also rewrite the gitfile in the submodule working tree of the linked worktree
> to point to $GIT_COMMON_DIR/modules/<name>/.
> This leads to an incorrect (and confusing!) state in the submodule working tree
> of the main superproject worktree.
> 
> Additionnally, if switching to a commit where the submodule is not present,

s/Additionnally/Additionally

> submodule_unset_core_worktree will be called and will incorrectly remove
> 'core.wortree' from the config file of the submodule in the main superproject worktree,
> $GIT_COMMON_DIR/modules/<name>/config.
>
> Fix this by constructing the path to the submodule repository using get_git_dir()
> in both submodule_move_head and submodule_unset_core_worktree.
> 
> Signed-off-by: Philippe Blain <[email protected]>
> ---
>  submodule.c                   |  6 +++---
>  t/t2405-worktree-submodule.sh | 22 ++++++++++++++++++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 9da7181321..5d19ec48a6 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1811,7 +1811,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
>  void submodule_unset_core_worktree(const struct submodule *sub)
>  {
>  	char *config_path = xstrfmt("%s/modules/%s/config",
> -				    get_git_common_dir(), sub->name);
> +				    get_git_dir(), sub->name);
>  
>  	if (git_config_set_in_file_gently(config_path, "core.worktree", NULL))
>  		warning(_("Could not unset core.worktree setting in submodule '%s'"),
> @@ -1914,7 +1914,7 @@ int submodule_move_head(const char *path,
>  					ABSORB_GITDIR_RECURSE_SUBMODULES);
>  		} else {
>  			char *gitdir = xstrfmt("%s/modules/%s",
> -				    get_git_common_dir(), sub->name);
> +				    get_git_dir(), sub->name);
>  			connect_work_tree_and_git_dir(path, gitdir, 0);
>  			free(gitdir);
>  
> @@ -1924,7 +1924,7 @@ int submodule_move_head(const char *path,
>  
>  		if (old_head && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {
>  			char *gitdir = xstrfmt("%s/modules/%s",
> -				    get_git_common_dir(), sub->name);
> +				    get_git_dir(), sub->name);
>  			connect_work_tree_and_git_dir(path, gitdir, 1);
>  			free(gitdir);
>  		}
> diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
> index f1952c70dd..eba17d9e35 100755
> --- a/t/t2405-worktree-submodule.sh
> +++ b/t/t2405-worktree-submodule.sh
> @@ -10,6 +10,7 @@ test_expect_success 'setup: create origin repos'  '
>  	git init origin/sub &&
>  	test_commit -C origin/sub file1 &&
>  	git init origin/main &&
> +	test_commit -C origin/main first &&
>  	git -C origin/main submodule add ../sub &&
>  	git -C origin/main commit -m "add sub" &&
>  	test_commit -C origin/sub "file1-updated" file1 file1updated &&
> @@ -54,4 +55,25 @@ test_expect_success 'submodule is checked out after manually adding submodule wo
>  	grep "file1-updated" out
>  '
>  
> +test_expect_success 'checkout --recurse-submodules uses $GIT_DIR for submodules in a linked worktree' '
> +	git -C main worktree add "$base_path/checkout-recurse" --detach  &&
> +	git -C checkout-recurse submodule update --init &&
> +	cat checkout-recurse/sub/.git > expect-gitfile &&

Here, and the rest of these tests, please drop the space between the ">" and
the output file: ">expect-gitfile".

> +	git -C main/sub rev-parse HEAD > expect-head-main &&
> +	git -C checkout-recurse checkout --recurse-submodules HEAD~1 &&
> +	cat checkout-recurse/sub/.git > actual-gitfile &&
> +	git -C main/sub rev-parse HEAD > actual-head-main &&
> +	test_cmp expect-gitfile actual-gitfile &&
> +	test_cmp expect-head-main actual-head-main
> +'
> +
> +test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '
> +	git -C main/sub config --get core.worktree > expect &&
> +	git -C checkout-recurse checkout --recurse-submodules first &&
> +	test_might_fail git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree > linked-config &&

Why test_might_fail here, and below? Because the config may not exist, which
would return an error code? Should we _expect_ that failure with test_must_fail?

> +	test_must_be_empty linked-config &&
> +	test_might_fail git -C main/sub config --get core.worktree > actual &&
> +	test_cmp expect actual

This tests that core.wortkree didn't change throughout the process, but
could we instead confirm an exact value by echoing into "expect" and
comparing both config outputs against that value?

Perhaps it is worth checking the success of the command that was failing
in submodules that still had core.worktree=true before 898c2e6? For your
test, it would be:

	git -C main/.git/worktrees/checkout-recurse/modules/sub log

Thanks,
-Stolee

Copy link

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, Philippe Blain wrote (reply to this):

Hi Stolee,

> Le 17 janv. 2020 à 08:24, Derrick Stolee <[email protected]> a écrit :
> 
>> Additionnally, if switching to a commit where the submodule is not present,
> 
> s/Additionnally/Additionally
fixed.
> 
>> +	cat checkout-recurse/sub/.git > expect-gitfile &&
> 
> Here, and the rest of these tests, please drop the space between the ">" and
> the output file: ">expect-gitfile ».
fixed.
> 
>> +	git -C main/sub rev-parse HEAD > expect-head-main &&
>> +	git -C checkout-recurse checkout --recurse-submodules HEAD~1 &&
>> +	cat checkout-recurse/sub/.git > actual-gitfile &&
>> +	git -C main/sub rev-parse HEAD > actual-head-main &&
>> +	test_cmp expect-gitfile actual-gitfile &&
>> +	test_cmp expect-head-main actual-head-main
>> +'
>> +
>> +test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '
>> +	git -C main/sub config --get core.worktree > expect &&
>> +	git -C checkout-recurse checkout --recurse-submodules first &&
>> +	test_might_fail git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree > linked-config &&
> 
> Why test_might_fail here, and below? Because the config may not exist, which
> would return an error code? Should we _expect_ that failure with test_must_fail?
I expected that question and answered in the cover letter (since Gitgitgadet can�t (yet, I hope) do in-patch "timely commentaries", [1]), but here is my answer:
I used test_might_fail such that when the test is run on the current master, only the last test_cmp makes the test fail. If we want to be more strict I'll change that to :

diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh

index eba17d9e35..31d156cce7 100755

--- a/t/t2405-worktree-submodule.sh
+++ b/t/t2405-worktree-submodule.sh
@@ -70,9 +70,9 @@
test_expect_success 'checkout --recurse-submodules uses $GIT_DIR for submodules
test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '
	git -C main/sub config --get core.worktree > expect &&
	git -C checkout-recurse checkout --recurse-submodules first &&

-	test_might_fail git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree >linked-config &&
+	test_expect_code 1 git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree >linked-config &&

	test_must_be_empty linked-config &&

-	test_might_fail git -C main/sub config --get core.worktree >actual &&
+	git -C main/sub config --get core.worktree >actual &&

	test_cmp expect actual
�
[1] https://github.com/gitgitgadget/gitgitgadget/issues/173

> 
>> +	test_must_be_empty linked-config &&
>> +	test_might_fail git -C main/sub config --get core.worktree > actual &&
>> +	test_cmp expect actual
> 
> This tests that core.wortkree didn't change throughout the process, but
> could we instead confirm an exact value by echoing into "expect" and
> comparing both config outputs against that value?
Good idea, thanks. I�ll harden the test in v2.
> 
> Perhaps it is worth checking the success of the command that was failing
> in submodules that still had core.worktree=true before 898c2e6? For your
> test, it would be:
> 
> 	git -C main/.git/worktrees/checkout-recurse/modules/sub log
I�ll also add that.
> 
> Thanks,
> -Stolee
Thanks for the review!
Philippe.
p.s. Sorry for the re-send, I forgot to CC the list.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 17, 2020

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/17/2020 7:23 AM, Philippe Blain via GitGitGadget wrote:
> The first 3 patches are clean-up patches on t7410-submodule-checkout-to.sh
> (renamed to t2405-worktree-submodule.sh) to bring it up to date.

These cleanups make sense to me.

Thanks,
-Stolee

@@ -1,37 +1,21 @@
#!/bin/sh
Copy link

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 Fri, Jan 17, 2020 at 7:24 AM Philippe Blain via GitGitGadget
<[email protected]> wrote:
> The subshells used in the setup phase of this test are unnecessary.
> Remove them by using 'git -C' and 'test_commit -C'.

The subshells may not be necessary, but the code feels cleaner before
this patch is applied since all the added "-C foo/bar" noise hurts
readability. So, I'm "meh" on this patch and wouldn't complain if it
was dropped (though I don't insist upon it).

> Signed-off-by: Philippe Blain <[email protected]>
> ---
> diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
> @@ -6,32 +6,16 @@ test_description='Combination of submodules and multiple worktrees'
> -               git commit -m "file1 updated"
> +       git -C origin/main commit -m "add sub" &&
> +       test_commit -C origin/sub "file1-updated" file1 file1updated &&
> @@ -49,7 +33,7 @@ test_expect_success 'checkout main' '
> -       grep "file1 updated" out
> +       grep "file1-updated" out

Why this change? Is it because test_commit() mishandles the whitespace
in the commit message? If so, it might deserve mention in the commit
message of this patch. (Even better would be to fix test_commit(), if
that is the case.)

Copy link

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):

Eric Sunshine <[email protected]> writes:

> On Fri, Jan 17, 2020 at 7:24 AM Philippe Blain via GitGitGadget
> <[email protected]> wrote:
>> The subshells used in the setup phase of this test are unnecessary.
>> Remove them by using 'git -C' and 'test_commit -C'.
>
> The subshells may not be necessary, but the code feels cleaner before
> this patch is applied since all the added "-C foo/bar" noise hurts
> readability. So, I'm "meh" on this patch and wouldn't complain if it
> was dropped (though I don't insist upon it).

I dunno.  Each of these subshells did not do much after going into
its subdirectory, so repetition of "-C foo/bar" did not bother me
that much.

>> Signed-off-by: Philippe Blain <[email protected]>
>> ---
>> diff --git a/t/t2405-worktree-submodule.sh b/t/t2405-worktree-submodule.sh
>> @@ -6,32 +6,16 @@ test_description='Combination of submodules and multiple worktrees'
>> -               git commit -m "file1 updated"
>> +       git -C origin/main commit -m "add sub" &&
>> +       test_commit -C origin/sub "file1-updated" file1 file1updated &&
>> @@ -49,7 +33,7 @@ test_expect_success 'checkout main' '
>> -       grep "file1 updated" out
>> +       grep "file1-updated" out
>
> Why this change? Is it because test_commit() mishandles the whitespace
> in the commit message? If so, it might deserve mention in the commit
> message of this patch. (Even better would be to fix test_commit(), if
> that is the case.)

FWIW I had the same reaction on that dash in "file1 updated".

Copy link

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, Philippe Blain wrote (reply to this):

Hi Eric,

>> -       grep "file1 updated" out
>> +       grep "file1-updated" out
> 
> Why this change? Is it because test_commit() mishandles the whitespace
> in the commit message? If so, it might deserve mention in the commit
> message of this patch. (Even better would be to fix test_commit(), if
> that is the case.)

The only reason is that I didn�t notice that test_commit accepts a <tag> argument, which defaults to <message> if unset. So when I changed the test to use test_commit and ran it I got errors from �git tag� saying "file1 updated" is not a valid tag name, so I added a dash.

I�ll correct that in v2.

Thanks,
Philippe.

@@ -0,0 +1,57 @@
#!/bin/sh
Copy link

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 Fri, Jan 17, 2020 at 7:25 AM Philippe Blain via GitGitGadget
<[email protected]> wrote:
> When 'checkout --to' functionality was moved to 'worktree add', tests were adapted
> in f194b1ef6e (tests: worktree: retrofit "checkout --to" tests for "worktree add",
> 2015-07-06).
>
> The calls were changed to 'worktree add' in this test (then t7410), but the test
> descriptions were not updated, keeping 'checkout' instead of using the new
> terminology (linked worktrees).
>
> Clarify the tests by using the right terminology. While at it, remove some unnecessary
> leading directories such that all superproject worktrees are directly next to one
> another in the trash directory.

The unanswered questions which popped into my head when reading the
"While at it..." include:

    Why is it desirable for the worktrees to live in this new location
    rather than their original locations?

    Is it safe to relocate the worktrees like this without losing some
    important aspect of the test?

    Why were the worktrees located as they were originally? Was there
    some hidden or not-so-obvious reason that we're overlooking? (I
    guess this is really the same as question #2.)

Copy link

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, Philippe Blain wrote (reply to this):

Hi Eric,
> Le 17 janv. 2020 à 08:56, Eric Sunshine <[email protected]> a écrit :
> 
>> While at it, remove some unnecessary
>> leading directories such that all superproject worktrees are directly next to one
>> another in the trash directory.
> 
> The unanswered questions which popped into my head when reading the
> "While at it..." include:
> 
>    Why is it desirable for the worktrees to live in this new location
>    rather than their original locations?
I thought it was desirable because the leading directories don�t serve any purpose apart from carrying information about what they are testing, which the worktree directory itself can do instead of all of them being called "main". 
> 
>    Is it safe to relocate the worktrees like this without losing some
>    important aspect of the test?
After analyzing the test to see what was being tested, making the change and making sure the test behaved the same way, I concluded that it was.
> 
>    Why were the worktrees located as they were originally? Was there
>    some hidden or not-so-obvious reason that we're overlooking? (I
>    guess this is really the same as question #2.)
I don�t think there was a reason. The worktrees directories were structured that way since the test was introduced in df56607dff (git-common-dir: make "modules/" per-working-directory directory, 2014-11-30). Maybe one reason was for every superproject worktree to be in a directory called "main", just as every submodule is in a directory called "sub"�

I can explain more this reasoning in the commit message if necessary.

Philippe.

Copy link

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 Sat, Jan 18, 2020 at 7:21 PM Philippe Blain
<[email protected]> wrote:
> > Le 17 janv. 2020 à 08:56, Eric Sunshine <[email protected]> a écrit :
> > The unanswered questions which popped into my head when reading the
> > "While at it..." include:
> > [...]
> I can explain more this reasoning in the commit message if necessary.

Yes, please.

This test was added in df56607 (git-common-dir: make "modules/"
per-working-directory directory, 2014-11-30), back when the 'git worktree' command
did not exist and 'git checkout --to' was used to create supplementary worktrees.

Since this file contains tests for the interaction of 'git worktree' with
submodules, rename it to t2405-worktree-submodule.sh, following the naming scheme for
tests checking the behavior of various commands with submodules.

Signed-off-by: Philippe Blain <[email protected]>
The subshells used in the setup phase of this test are unnecessary.

Remove them by using 'git -C' and 'test_commit -C'.

Signed-off-by: Philippe Blain <[email protected]>
@phil-blain phil-blain force-pushed the checkout-recurse-in-linked-worktree branch from 72cdb2f to 0b38949 Compare January 19, 2020 00:00
When 'checkout --to' functionality was moved to 'worktree add', tests were adapted
in f194b1e (tests: worktree: retrofit "checkout --to" tests for "worktree add",
2015-07-06).

The calls were changed to 'worktree add' in this test (then t7410), but the test
descriptions were not updated, keeping 'checkout' instead of using the new
terminology (linked worktrees).

Also, in the test each worktree is created in
$TRASH_DIRECTORY/<leading-directory>/main, where the name of <leading-directory>
carries some information about what behavior each test verifies. This directory
structure is not mandatory for the tests; the worktrees can live next to one
another in the trash directory.

Clarify the tests by using the right terminology, and remove the unnecessary
leading directories such that all superproject worktrees are directly next to one
another in the trash directory.

Signed-off-by: Philippe Blain <[email protected]>
Ever since df56607 (git-common-dir: make "modules/"
per-working-directory directory, 2014-11-30), submodules in linked worktrees
are cloned to $GIT_DIR/modules, i.e. $GIT_COMMON_DIR/worktrees/<name>/modules.

However, this convention was not followed when the worktree updater commands
checkout, reset and read-tree learned to recurse into submodules. Specifically,
submodule.c::submodule_move_head, introduced in 6e3c159 (update submodules:
add submodule_move_head, 2017-03-14) and submodule.c::submodule_unset_core_worktree,
(re)introduced in 898c2e6 (submodule: unset core.worktree if no working tree
is present, 2018-12-14) use get_git_common_dir() instead of get_git_dir()
to get the path of the submodule repository.

This means that, for example, 'git checkout --recurse-submodules <branch>'
in a linked worktree will correctly checkout <branch>, detach the submodule's HEAD
at the commit recorded in <branch> and update the submodule working tree, but the
submodule HEAD that will be moved is the one in $GIT_COMMON_DIR/modules/<name>/,
i.e. the submodule repository of the main superproject working tree.
It will also rewrite the gitfile in the submodule working tree of the linked worktree
to point to $GIT_COMMON_DIR/modules/<name>/.
This leads to an incorrect (and confusing!) state in the submodule working tree
of the main superproject worktree.

Additionally, if switching to a commit where the submodule is not present,
submodule_unset_core_worktree will be called and will incorrectly remove
'core.wortree' from the config file of the submodule in the main superproject worktree,
$GIT_COMMON_DIR/modules/<name>/config.

Fix this by constructing the path to the submodule repository using get_git_dir()
in both submodule_move_head and submodule_unset_core_worktree.

Signed-off-by: Philippe Blain <[email protected]>
@phil-blain phil-blain force-pushed the checkout-recurse-in-linked-worktree branch from 0b38949 to 614bccd Compare January 21, 2020 12:58
@phil-blain
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 21, 2020

Submitted as [email protected]

@phil-blain
Copy link
Author

phil-blain commented Jan 22, 2020

@dscho sorry to bother you, it seems neither v1 nor v2 of this PR was sent with the CC's I added in the footer of the PR description... is it because there is no space between "Cc:" and the first name ?

What syntax is accepted for the emails in the CC footer ? I see you and Stolee seem to use Markdown with mailto links, is this essential ? I usually copy the contact from Mail on my Mac which when pasted looks like Brandon Williams <[email protected]>...

Also regarding list etiquette, what should I do now ? should I /submit again with the CC's fixed ? will this send a v3 or GGG will realize that nothing has changed and re-send v2 ?

@dscho
Copy link
Member

dscho commented Jan 22, 2020

is it because there is no space between "Cc:" and the first name ?

I think so.

I see you and Stolee seem to use Markdown with mailto links, is this essential ?

No, the GitHub renderer makes them look like it, but we really write the Cc: lines the same way as we would in the commit message.

Also regarding list etiquette, what should I do now ? should I /submit again with the CC's fixed ? will this send a v3 or GGG will realize that nothing has changed and re-send v2 ?

I would simply reply to the cover letter manually, with all those Cc:s. GitGitGadget would refuse to send a v3 because nothing changed...

@dscho
Copy link
Member

dscho commented Jan 22, 2020

is it because there is no space between "Cc:" and the first name ?

I think so.

Yep, it would appear your hunch was right: https://github.com/gitgitgadget/gitgitgadget/blob/f9f7cc94bd3e20e5e9217233c0abfed6b5b091ac/lib/patch-series.ts#L296

Maybe you want to patch this, substituting the space in the regular expression by \s*, then add or modify a test and open a PR?

@phil-blain
Copy link
Author

Thanks a lot for checking! I’ll gladly look into patching that :)

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 22, 2020

On the Git mailing list, Philippe Blain wrote (reply to this):

Hello,

Here is v2 of my series. I realized that Gitgitgadget failed to parse the Cc: footer in my PR description and thus both v1 and v2 got sent to the list without any Cc:s.

Cheers,

Philippe.

> Le 21 janv. 2020 à 10:01, Philippe Blain via GitGitGadget <[email protected]> a écrit :
> 
> Changes since v1:
> 
> * revert the addition of a dash in "file1 updated" by correctly using the
>   'tag' argument of test_commit
> * improve the commit message for the 3rd patch, as suggested by Eric
> * fix spacing when redirecting into 'expect' and 'actual'
> * harden the tests by echoing expected values into expect, as suggested by
>   Stolee (I did that in both tests)
> * remove test_might_fail and use test_expect_code
> * add the 'git log' test suggested by Stolee
> 
> v1: This series fixes the behaviour of git checkout/reset/read-tree
> --recurse-submodules when they are called in a linked worktree (created by 
> git worktree add). 
> 
> Although submodules are cloned in $GIT_COMMON_DIR/worktrees/<name>/modules 
> upon issuing git submodule update in the linked worktree, any invocation of 
> git checkout/reset/read-tree --recurse-submodules that changes the state of
> the submodule(s) will incorrectly operate on the repositories of the
> submodules in the main worktree, i.e. the ones at $GIT_COMMON_DIR/modules/. 
> 
> The fourth patch fixes this behaviour by using get_git_dir() instead of 
> git_common_dir() in submodule.c::submodule_move_head and 
> submodule.c::submodule_unset_core_worktree to construct the path to the
> submodule repository.
> 
> The first 3 patches are clean-up patches on t7410-submodule-checkout-to.sh
> (renamed to t2405-worktree-submodule.sh) to bring it up to date.
> 
> Cc:Max Kirillov [email protected] [[email protected]] Brandon Williams 
> [email protected] [[email protected]] Jonathan Tan 
> [email protected] [[email protected]] Stefan Beller 
> [email protected] [[email protected]] Nguyá»�n Thái Ngá»�c Duy 
> [email protected] [[email protected]] Eric Sunshine [email protected]
> [[email protected]] Derrick Stolee [email protected] [[email protected]]
> 
> Philippe Blain (4):
>  t7410: rename to t2405-worktree-submodule.sh
>  t2405: use git -C and test_commit -C instead of subshells
>  t2405: clarify test descriptions and simplify test
>  submodule.c: use get_git_dir() instead of get_git_common_dir()
> 
> submodule.c                      |  6 +--
> t/t2405-worktree-submodule.sh    | 90 ++++++++++++++++++++++++++++++++
> t/t7410-submodule-checkout-to.sh | 77 ---------------------------
> 3 files changed, 93 insertions(+), 80 deletions(-)
> create mode 100755 t/t2405-worktree-submodule.sh
> delete mode 100755 t/t7410-submodule-checkout-to.sh
> 
> 
> base-commit: b4615e40a8125477e18490d868f7b65954372b43
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-523%2Fphil-blain%2Fcheckout-recurse-in-linked-worktree-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-523/phil-blain/checkout-recurse-in-linked-worktree-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/523
> 
> Range-diff vs v1:
> 
> 1:  ff33339690 = 1:  1a4eae1ef5 t7410: rename to t2405-worktree-submodule.sh
> 2:  5060ce3d64 ! 2:  f06d2c4aa5 t2405: use git -C and test_commit -C instead of subshells
>     @@ -41,7 +41,7 @@
>      +	git init origin/main &&
>      +	git -C origin/main submodule add ../sub &&
>      +	git -C origin/main commit -m "add sub" &&
>     -+	test_commit -C origin/sub "file1-updated" file1 file1updated &&
>     ++	test_commit -C origin/sub "file1 updated" file1 file1updated file1updated &&
>       	git -C origin/main/sub pull &&
>      -	(
>      -		cd origin/main &&
>     @@ -53,30 +53,3 @@
>       '
> 
>       test_expect_success 'setup: clone' '
>     -@@
>     - 
>     - test_expect_failure 'can see submodule diffs just after checkout' '
>     - 	git -C default_checkout/main diff --submodule master"^!" >out &&
>     --	grep "file1 updated" out
>     -+	grep "file1-updated" out
>     - '
>     - 
>     - test_expect_success 'checkout main and initialize independent clones' '
>     -@@
>     - 
>     - test_expect_success 'can see submodule diffs after independent cloning' '
>     - 	git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
>     --	grep "file1 updated" out
>     -+	grep "file1-updated" out
>     - '
>     - 
>     - test_expect_success 'checkout sub manually' '
>     -@@
>     - 
>     - test_expect_success 'can see submodule diffs after manual checkout of linked submodule' '
>     - 	git -C linked_submodule/main diff --submodule master"^!" >out &&
>     --	grep "file1 updated" out
>     -+	grep "file1-updated" out
>     - '
>     - 
>     - test_done
> 3:  6e0e664026 ! 3:  10727275a2 t2405: clarify test descriptions and simplify test
>     @@ -10,7 +10,13 @@
>          descriptions were not updated, keeping 'checkout' instead of using the new
>          terminology (linked worktrees).
> 
>     -    Clarify the tests by using the right terminology. While at it, remove some unnecessary
>     +    Also, in the test each worktree is created in
>     +    $TRASH_DIRECTORY/<leading-directory>/main, where the name of <leading-directory>
>     +    carries some information about what behavior each test verifies. This directory
>     +    structure is not mandatory for the tests; the worktrees can live next to one
>     +    another in the trash directory.
>     +
>     +    Clarify the tests by using the right terminology, and remove the unnecessary
>          leading directories such that all superproject worktrees are directly next to one
>          another in the trash directory.
> 
>     @@ -44,7 +50,7 @@
>      -	git -C default_checkout/main diff --submodule master"^!" >out &&
>      +test_expect_failure 'submodule is checked out just after worktree add' '
>      +	git -C worktree diff --submodule master"^!" >out &&
>     - 	grep "file1-updated" out
>     + 	grep "file1 updated" out
>       '
> 
>      -test_expect_success 'checkout main and initialize independent clones' '
>     @@ -60,7 +66,7 @@
>      -	git -C fully_cloned_submodule/main diff --submodule master"^!" >out &&
>      +test_expect_success 'submodule is checked out just after submodule update in linked worktree' '
>      +	git -C worktree-submodule-update diff --submodule master"^!" >out &&
>     - 	grep "file1-updated" out
>     + 	grep "file1 updated" out
>       '
> 
>      -test_expect_success 'checkout sub manually' '
>     @@ -76,6 +82,6 @@
>      -	git -C linked_submodule/main diff --submodule master"^!" >out &&
>      +test_expect_success 'submodule is checked out after manually adding submodule worktree' '
>      +	git -C linked_submodule diff --submodule master"^!" >out &&
>     - 	grep "file1-updated" out
>     + 	grep "file1 updated" out
>       '
> 
> 4:  72cdb2f95d ! 4:  614bccd31b submodule.c: use get_git_dir() instead of get_git_common_dir()
>     @@ -24,7 +24,7 @@
>          This leads to an incorrect (and confusing!) state in the submodule working tree
>          of the main superproject worktree.
> 
>     -    Additionnally, if switching to a commit where the submodule is not present,
>     +    Additionally, if switching to a commit where the submodule is not present,
>          submodule_unset_core_worktree will be called and will incorrectly remove
>          'core.wortree' from the config file of the submodule in the main superproject worktree,
>          $GIT_COMMON_DIR/modules/<name>/config.
>     @@ -75,30 +75,41 @@
>      +	test_commit -C origin/main first &&
>       	git -C origin/main submodule add ../sub &&
>       	git -C origin/main commit -m "add sub" &&
>     - 	test_commit -C origin/sub "file1-updated" file1 file1updated &&
>     + 	test_commit -C origin/sub "file1 updated" file1 file1updated file1updated &&
>      @@
>     - 	grep "file1-updated" out
>     + 	grep "file1 updated" out
>       '
> 
>      +test_expect_success 'checkout --recurse-submodules uses $GIT_DIR for submodules in a linked worktree' '
>      +	git -C main worktree add "$base_path/checkout-recurse" --detach  &&
>      +	git -C checkout-recurse submodule update --init &&
>     -+	cat checkout-recurse/sub/.git > expect-gitfile &&
>     -+	git -C main/sub rev-parse HEAD > expect-head-main &&
>     ++	echo "gitdir: ../../main/.git/worktrees/checkout-recurse/modules/sub" >expect-gitfile &&
>     ++	cat checkout-recurse/sub/.git >actual-gitfile &&
>     ++	test_cmp expect-gitfile actual-gitfile &&
>     ++	git -C main/sub rev-parse HEAD >expect-head-main &&
>      +	git -C checkout-recurse checkout --recurse-submodules HEAD~1 &&
>     -+	cat checkout-recurse/sub/.git > actual-gitfile &&
>     -+	git -C main/sub rev-parse HEAD > actual-head-main &&
>     ++	cat checkout-recurse/sub/.git >actual-gitfile &&
>     ++	git -C main/sub rev-parse HEAD >actual-head-main &&
>      +	test_cmp expect-gitfile actual-gitfile &&
>      +	test_cmp expect-head-main actual-head-main
>      +'
>      +
>      +test_expect_success 'core.worktree is removed in $GIT_DIR/modules/<name>/config, not in $GIT_COMMON_DIR/modules/<name>/config' '
>     -+	git -C main/sub config --get core.worktree > expect &&
>     ++	echo "../../../sub" >expect-main &&
>     ++	git -C main/sub config --get core.worktree >actual-main &&
>     ++	test_cmp expect-main actual-main &&
>     ++	echo "../../../../../../checkout-recurse/sub" >expect-linked &&
>     ++	git -C checkout-recurse/sub config --get core.worktree >actual-linked &&
>     ++	test_cmp expect-linked actual-linked &&
>      +	git -C checkout-recurse checkout --recurse-submodules first &&
>     -+	test_might_fail git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree > linked-config &&
>     ++	test_expect_code 1 git -C main/.git/worktrees/checkout-recurse/modules/sub config --get core.worktree >linked-config &&
>      +	test_must_be_empty linked-config &&
>     -+	test_might_fail git -C main/sub config --get core.worktree > actual &&
>     -+	test_cmp expect actual
>     ++	git -C main/sub config --get core.worktree >actual-main &&
>     ++	test_cmp expect-main actual-main
>     ++'
>     ++
>     ++test_expect_success 'unsetting core.worktree does not prevent running commands directly against the submodule repository' '
>     ++	git -C main/.git/worktrees/checkout-recurse/modules/sub log
>      +'
>      +
>       test_done
> 
> -- 
> gitgitgadget

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 22, 2020

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

"Philippe Blain via GitGitGadget" <[email protected]> writes:

> Changes since v1:
>
>  * revert the addition of a dash in "file1 updated" by correctly using the
>    'tag' argument of test_commit
>  * improve the commit message for the 3rd patch, as suggested by Eric
>  * fix spacing when redirecting into 'expect' and 'actual'
>  * harden the tests by echoing expected values into expect, as suggested by
>    Stolee (I did that in both tests)
>  * remove test_might_fail and use test_expect_code
>  * add the 'git log' test suggested by Stolee
>
> v1: This series fixes the behaviour of git checkout/reset/read-tree
> --recurse-submodules when they are called in a linked worktree (created by 
> git worktree add). 
>
> Although submodules are cloned in $GIT_COMMON_DIR/worktrees/<name>/modules 
> upon issuing git submodule update in the linked worktree, any invocation of 
> git checkout/reset/read-tree --recurse-submodules that changes the state of
> the submodule(s) will incorrectly operate on the repositories of the
> submodules in the main worktree, i.e. the ones at $GIT_COMMON_DIR/modules/. 
>
> The fourth patch fixes this behaviour by using get_git_dir() instead of 
> git_common_dir() in submodule.c::submodule_move_head and 
> submodule.c::submodule_unset_core_worktree to construct the path to the
> submodule repository.
>
> The first 3 patches are clean-up patches on t7410-submodule-checkout-to.sh
> (renamed to t2405-worktree-submodule.sh) to bring it up to date.

I've read these four patches a few times and did not spot any
remaining issues.  I however is hardly an expert on (nor a big fan
of) the "--recurse-submodule" feature, so a proper review by those
who are more familiar with it is surely appreciated.

Thanks.

> Cc:Max Kirillov [email protected] [[email protected]] Brandon Williams 
> [email protected] [[email protected]] Jonathan Tan 
> [email protected] [[email protected]] Stefan Beller 
> [email protected] [[email protected]] Nguyá»�n Thái Ngá»�c Duy 
> [email protected] [[email protected]] Eric Sunshine [email protected]
> [[email protected]] Derrick Stolee [email protected] [[email protected]]

This is somewhat unreadable wall of names X-< Is it a funny
rendering of what is originally in some mark-up format (perhaps
HTML???)

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 22, 2020

This patch series was integrated into pu via git@93d4f04.

@gitgitgadget gitgitgadget bot added the pu label Jan 22, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Jan 22, 2020

This branch is now known as pb/recurse-submodule-in-worktree-fix.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 22, 2020

On the Git mailing list, Philippe Blain wrote (reply to this):

Hi Junio, 

> Le 22 janv. 2020 à 17:10, Junio C Hamano <[email protected]> a écrit :
> 
> "Philippe Blain via GitGitGadget" <[email protected]> writes:

>> Cc:Max Kirillov [email protected] [[email protected]] Brandon Williams 
>> [email protected] [[email protected]] Jonathan Tan 
>> [email protected] [[email protected]] Stefan Beller 
>> [email protected] [[email protected]] Nguyá»�n Thái Ngá»�c Duy 
>> [email protected] [[email protected]] Eric Sunshine [email protected]
>> [[email protected]] Derrick Stolee [email protected] [[email protected]]
> 
> This is somewhat unreadable wall of names X-< Is it a funny
> rendering of what is originally in some mark-up format (perhaps
> HTML???)

Yes, Gitgitgadget unfortunately failed to convert this wall of text into a proper CC: list because there was no space between the "Cc:" and "Max". 
I�ll try to submit a PR for that in GGG. 

Cheers,
Philippe. 

phil-blain added a commit to phil-blain/gitgitgadget that referenced this pull request Jan 24, 2020
A recent Gitgitgadget PR [1] demonstrates that GGG fails to parse
CC: footers if no space is found after the colon.

Replace the literal space " " by the more lenient "\s*" so that CC:s are
parsed correctly if any kind of whitespace, or no whitespace at all, is
detected after the colon.

[1] gitgitgadget/git#523

Helped-by: Johannes Schindelin <[email protected]>
Signed-off-by: Philippe Blain <[email protected]>
@gitgitgadget
Copy link

gitgitgadget bot commented Jan 24, 2020

This patch series was integrated into pu via git@2341c64.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 24, 2020

On the Git mailing list, Philippe Blain wrote (reply to this):


> Le 22 janv. 2020 à 17:25, Philippe Blain <[email protected]> a écrit :
> 
> Hi Junio, 
> 
>> Le 22 janv. 2020 à 17:10, Junio C Hamano <[email protected]> a écrit :
>> 
>> "Philippe Blain via GitGitGadget" <[email protected]> writes:
> 
>>> Cc:Max Kirillov [email protected] [[email protected]] Brandon Williams 
>>> [email protected] [[email protected]] Jonathan Tan 
>>> [email protected] [[email protected]] Stefan Beller 
>>> [email protected] [[email protected]] Nguyá»�n Thái Ngá»�c Duy 
>>> [email protected] [[email protected]] Eric Sunshine [email protected]
>>> [[email protected]] Derrick Stolee [email protected] [[email protected]]
>> 
>> This is somewhat unreadable wall of names X-< Is it a funny
>> rendering of what is originally in some mark-up format (perhaps
>> HTML???)
> 
> Yes, Gitgitgadget unfortunately failed to convert this wall of text into a proper CC: list because there was no space between the "Cc:" and "Max". 
> I�ll try to submit a PR for that in GGG. 
For the record, https://github.com/gitgitgadget/gitgitgadget/pull/198 has been merged so this should not happen again.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 24, 2020

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

Philippe Blain <[email protected]> writes:

>>>> Cc:Max Kirillov [email protected] [[email protected]] Brandon Williams 
>>>> [email protected] [[email protected]] Jonathan Tan 
>>>> [email protected] [[email protected]] Stefan Beller 
>>>> [email protected] [[email protected]] Nguyá»�n Thái Ngá»�c Duy 
>>>> [email protected] [[email protected]] Eric Sunshine [email protected]
>>>> [[email protected]] Derrick Stolee [email protected] [[email protected]]
>>> 
>>> This is somewhat unreadable wall of names X-< Is it a funny
>>> rendering of what is originally in some mark-up format (perhaps
>>> HTML???)
>> 
>> Yes, Gitgitgadget unfortunately failed to convert this wall of text into a proper CC: list because there was no space between the "Cc:" and "Max". 
>> I�ll try to submit a PR for that in GGG. 
> For the record, https://github.com/gitgitgadget/gitgitgadget/pull/198 has been merged so this should not happen again.

That's good to know.  As long as the resulting e-mail addresses are
sensible (e.g. if Markdown markups are not cleansed enough and the
duplicated addresses inside [] seep through, the result would still
not be good), it is OK.

Thanks.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 27, 2020

This patch series was integrated into pu via git@3aa8baa.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 27, 2020

This patch series was integrated into next via git@e4cb1e3.

@gitgitgadget gitgitgadget bot added the next label Jan 27, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Jan 30, 2020

This patch series was integrated into pu via git@10d41c1.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2020

This patch series was integrated into pu via git@ff5134b.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2020

This patch series was integrated into next via git@ff5134b.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2020

This patch series was integrated into master via git@ff5134b.

@gitgitgadget gitgitgadget bot added the master label Feb 5, 2020
@gitgitgadget gitgitgadget bot closed this Feb 5, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2020

Closed via ff5134b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants