threads / patch / 46984

patchstatus: do not get confused by submodules in excluded directories

Subject: [PATCH] status: do not get confused by submodules in excluded directories

## tl;dr

12 messages between Oct 17, 2017 and Oct 26, 2017. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Johannes Schindelin· Oct 17, 2017, 13:10 UTC · lore

We meticulously pass the `exclude` flag to the `treat_directory()` function so that we can indicate that files in it are excluded rather than untracked when recursing.

But we did not yet treat submodules the same way.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/submodule-in-excluded-v1
Fetch-It-Via: git fetch https://github.com/dscho/git submodule-in-excluded-v1
 dir.c                      |  2 +-
 t/t7061-wtstatus-ignore.sh | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
Show changes to 2 files +15 −1

dir.c, t/t7061-wtstatus-ignore.sh

diff --git a/dir.c b/dir.c
index 1d17b800cf3..9987011da57 100644
--- a/dir.c
+++ b/dir.c
@@ -1392,7 +1392,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 		if (!(dir->flags & DIR_NO_GITLINKS)) {
 			unsigned char sha1[20];
 			if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
-				return path_untracked;
+				return exclude ? path_excluded : path_untracked;
 		}
 		return path_recurse;
 	}
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index fc6013ba3c8..8c849a4cd2f 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -272,4 +272,18 @@ test_expect_success 'status ignored tracked directory with uncommitted file in t
 	test_cmp expected actual
 '
 
+cat >expected <<\EOF
+!! tracked/submodule/
+EOF
+
+test_expect_success 'status ignores submodule in excluded directory' '
+	git init tracked/submodule &&
+	(
+		cd tracked/submodule &&
+		test_commit initial
+	) &&
+	git status --porcelain --ignored -u tracked/submodule >actual &&
+	test_cmp expected actual
+'
+
 test_done

base-commit: 111ef79afe185f8731920569450f6a65320f5d5f

-- 
2.14.2.windows.3
Junio C Hamano· Oct 24, 2017, 05:18 UTC · re: Johannes Schindelin · lore

Re: [PATCH] status: do not get confused by submodules in excluded directories

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 5 quoted lines
> We meticulously pass the `exclude` flag to the `treat_directory()`
> function so that we can indicate that files in it are excluded rather
> than untracked when recursing.
>
> But we did not yet treat submodules the same way.

... "because of that, we ended up showing <<what incorrect result in what situation>>" would be a nice thing to have here, so that it can be copied to the release notes for the bugfix.

How far back a release do we want to make this fix applicable? It seems that it applies cleanly to maint-2.13 without breaking from my quick test, so that is probably where I'll queue this, even though we may no longer issue further maintenance releases on that track.

Any comment from submodule folks?

Sorry that I didn't notice this was left unattended by anybody til now. Will queue while waiting for those who are into submodules to respond.

Thanks.
Heiko Voigt· Oct 24, 2017, 12:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] status: do not get confused by submodules in excluded directories

On Tue, Oct 24, 2017 at 02:18:49PM +0900, Junio C Hamano wrote:
Show 11 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > We meticulously pass the `exclude` flag to the `treat_directory()`
> > function so that we can indicate that files in it are excluded rather
> > than untracked when recursing.
> >
> > But we did not yet treat submodules the same way.
> 
> ... "because of that, we ended up showing <<what incorrect result in
> what situation>>" would be a nice thing to have here, so that it can
> be copied to the release notes for the bugfix.

Yes I agree that would be nice here. It was not immediately obvious that this only applies when using both flags: -u and --ignored.

Seems to be a corner that not many people are using. At first I thought a plain 'git status' would show that behavior...

Show 10 quoted lines
> How far back a release do we want to make this fix applicable?  It
> seems that it applies cleanly to maint-2.13 without breaking from my
> quick test, so that is probably where I'll queue this, even though
> we may no longer issue further maintenance releases on that track.
> 
> Any comment from submodule folks?
> 
> Sorry that I didn't notice this was left unattended by anybody til
> now.  Will queue while waiting for those who are into submodules to
> respond.
Looks good to me.
Cheers Heiko
Stefan Beller· Oct 24, 2017, 15:34 UTC · re: Heiko Voigt · lore

Re: [PATCH] status: do not get confused by submodules in excluded directories

On Tue, Oct 24, 2017 at 5:15 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> Looks good to me.
Same here,

Thanks, Stefan

Junio C Hamano· Oct 25, 2017, 01:28 UTC · re: Heiko Voigt · lore

Re: [PATCH] status: do not get confused by submodules in excluded directories

Heiko Voigt <hvoigt@hvoigt.net> writes:
Show 15 quoted lines
> On Tue, Oct 24, 2017 at 02:18:49PM +0900, Junio C Hamano wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > We meticulously pass the `exclude` flag to the `treat_directory()`
>> > function so that we can indicate that files in it are excluded rather
>> > than untracked when recursing.
>> >
>> > But we did not yet treat submodules the same way.
>> 
>> ... "because of that, we ended up showing <<what incorrect result in
>> what situation>>" would be a nice thing to have here, so that it can
>> be copied to the release notes for the bugfix.  
>
> Yes I agree that would be nice here. It was not immediately obvious that
> this only applies when using both flags: -u and --ignored.
Does any of you care to fill in the <<blanks above>> then? ;-)
> Looks good to me.
>
> Cheers Heiko
Heiko Voigt· Oct 25, 2017, 14:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] status: do not get confused by submodules in excluded directories

On Wed, Oct 25, 2017 at 10:28:25AM +0900, Junio C Hamano wrote:
Show 19 quoted lines
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
> > On Tue, Oct 24, 2017 at 02:18:49PM +0900, Junio C Hamano wrote:
> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >> 
> >> > We meticulously pass the `exclude` flag to the `treat_directory()`
> >> > function so that we can indicate that files in it are excluded rather
> >> > than untracked when recursing.
> >> >
> >> > But we did not yet treat submodules the same way.
> >> 
> >> ... "because of that, we ended up showing <<what incorrect result in
> >> what situation>>" would be a nice thing to have here, so that it can
> >> be copied to the release notes for the bugfix.  
> >
> > Yes I agree that would be nice here. It was not immediately obvious that
> > this only applies when using both flags: -u and --ignored.
> 
> Does any of you care to fill in the <<blanks above>> then? ;-)
How about:

Because of that, we ended up showing the submodule as untracked and its content as ignored files when using the --ignored and -u flags with git status.

? But maybe Dscho also has some more information to add about his situation?

Cheers Heiko
Johannes Schindelin· Oct 25, 2017, 20:39 UTC · re: Heiko Voigt · lore

Re: [PATCH] status: do not get confused by submodules in excluded directories

Hi,
On Wed, 25 Oct 2017, Heiko Voigt wrote:
Show 29 quoted lines
> On Wed, Oct 25, 2017 at 10:28:25AM +0900, Junio C Hamano wrote:
> > Heiko Voigt <hvoigt@hvoigt.net> writes:
> > 
> > > On Tue, Oct 24, 2017 at 02:18:49PM +0900, Junio C Hamano wrote:
> > >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > >> 
> > >> > We meticulously pass the `exclude` flag to the `treat_directory()`
> > >> > function so that we can indicate that files in it are excluded rather
> > >> > than untracked when recursing.
> > >> >
> > >> > But we did not yet treat submodules the same way.
> > >> 
> > >> ... "because of that, we ended up showing <<what incorrect result in
> > >> what situation>>" would be a nice thing to have here, so that it can
> > >> be copied to the release notes for the bugfix.  
> > >
> > > Yes I agree that would be nice here. It was not immediately obvious that
> > > this only applies when using both flags: -u and --ignored.
> > 
> > Does any of you care to fill in the <<blanks above>> then? ;-)
> 
> How about:
> 
> Because of that, we ended up showing the submodule as untracked and its
> content as ignored files when using the --ignored and -u flags with git
> status.
> 
> ? But maybe Dscho also has some more information to add about his
> situation?

He has... as part of v2, a substantially more detailed commit message will reach your inbox Real Soon Now.

Ciao, Dscho

Kevin Daudt· Oct 24, 2017, 08:20 UTC · re: Johannes Schindelin · lore

Re: [PATCH] status: do not get confused by submodules in excluded directories

On Tue, Oct 17, 2017 at 03:10:11PM +0200, Johannes Schindelin wrote:
Show 45 quoted lines
> We meticulously pass the `exclude` flag to the `treat_directory()`
> function so that we can indicate that files in it are excluded rather
> than untracked when recursing.
> 
> But we did not yet treat submodules the same way.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/submodule-in-excluded-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git submodule-in-excluded-v1
>  dir.c                      |  2 +-
>  t/t7061-wtstatus-ignore.sh | 14 ++++++++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/dir.c b/dir.c
> index 1d17b800cf3..9987011da57 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1392,7 +1392,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>  		if (!(dir->flags & DIR_NO_GITLINKS)) {
>  			unsigned char sha1[20];
>  			if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
> -				return path_untracked;
> +				return exclude ? path_excluded : path_untracked;
>  		}
>  		return path_recurse;
>  	}
> diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
> index fc6013ba3c8..8c849a4cd2f 100755
> --- a/t/t7061-wtstatus-ignore.sh
> +++ b/t/t7061-wtstatus-ignore.sh
> @@ -272,4 +272,18 @@ test_expect_success 'status ignored tracked directory with uncommitted file in t
>  	test_cmp expected actual
>  '
>  
> +cat >expected <<\EOF
> +!! tracked/submodule/
> +EOF
> +
> +test_expect_success 'status ignores submodule in excluded directory' '
> +	git init tracked/submodule &&
> +	(
> +		cd tracked/submodule &&
> +		test_commit initial
> +	) &&
Could this use test_commit -C tracked/submodule initial?
Show 9 quoted lines
> +	git status --porcelain --ignored -u tracked/submodule >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_done
> 
> base-commit: 111ef79afe185f8731920569450f6a65320f5d5f
> -- 
> 2.14.2.windows.3
Johannes Schindelin· Oct 25, 2017, 13:26 UTC · re: Kevin Daudt · lore

Re: [PATCH] status: do not get confused by submodules in excluded directories

Hi Kevin,
On Tue, 24 Oct 2017, Kevin Daudt wrote:
Show 21 quoted lines
> On Tue, Oct 17, 2017 at 03:10:11PM +0200, Johannes Schindelin wrote:
> > diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
> > index fc6013ba3c8..8c849a4cd2f 100755
> > --- a/t/t7061-wtstatus-ignore.sh
> > +++ b/t/t7061-wtstatus-ignore.sh
> > @@ -272,4 +272,18 @@ test_expect_success 'status ignored tracked directory with uncommitted file in t
> >  	test_cmp expected actual
> >  '
> >  
> > +cat >expected <<\EOF
> > +!! tracked/submodule/
> > +EOF
> > +
> > +test_expect_success 'status ignores submodule in excluded directory' '
> > +	git init tracked/submodule &&
> > +	(
> > +		cd tracked/submodule &&
> > +		test_commit initial
> > +	) &&
> 
> Could this use test_commit -C tracked/submodule initial?

Yes! Thanks. For some reason, I did not even think that test_commit would accept the -C option.

Ciao, Dscho

Johannes Schindelin· Oct 25, 2017, 20:40 UTC · re: Johannes Schindelin · lore

[PATCH v2 0/1] Do not handle submodules in excluded directories as untracked

Anything in an excluded directory should be ignored, not only files and directories but also submodules.

Changes since v1:
- simplified the test case, as suggested by Kevin
- added explicit output to the commit message to demonstrate what is fixed
Johannes Schindelin (1):
  status: do not get confused by submodules in excluded directories
 dir.c                      |  2 +-
 t/t7061-wtstatus-ignore.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)
base-commit: ba78f398be65e941b93276680f68a81075716472
Published-As: https://github.com/dscho/git/releases/tag/submodule-in-excluded-v2
Fetch-It-Via: git fetch https://github.com/dscho/git submodule-in-excluded-v2
Interdiff vs v1:
 diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
 index 8c849a4cd2f..0c394cf995c 100755
 --- a/t/t7061-wtstatus-ignore.sh
 +++ b/t/t7061-wtstatus-ignore.sh
 @@ -278,10 +278,7 @@ EOF
  
  test_expect_success 'status ignores submodule in excluded directory' '
  	git init tracked/submodule &&
 -	(
 -		cd tracked/submodule &&
 -		test_commit initial
 -	) &&
 +	test_commit -C tracked/submodule initial &&
  	git status --porcelain --ignored -u tracked/submodule >actual &&
  	test_cmp expected actual
  '
-- 
2.14.3.windows.1
Johannes Schindelin· Oct 25, 2017, 20:40 UTC · re: Johannes Schindelin · lore

[PATCH v2 1/1] status: do not get confused by submodules in excluded directories

We meticulously pass the `exclude` flag to the `treat_directory()` function so that we can indicate that files in it are excluded rather than untracked when recursing.

But we did not yet treat submodules the same way.

Because of that, `git status --ignored --untracked` with a submodule `submodule` in a gitignored `tracked/` would show the submodule in the "Untracked files" section, e.g.

	On branch master
	Untracked files:
	  (use "git add <file>..." to include in what will be committed)
		tracked/submodule/
	Ignored files:
	  (use "git add -f <file>..." to include in what will be committed)
		tracked/submodule/initial.t

Instead, we would want it to show the submodule in the "Ignored files" section:

	On branch master
	Ignored files:
	  (use "git add -f <file>..." to include in what will be committed)
		tracked/submodule/
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 dir.c                      |  2 +-
 t/t7061-wtstatus-ignore.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)
Show changes to 2 files +12 −1

dir.c, t/t7061-wtstatus-ignore.sh

diff --git a/dir.c b/dir.c
index 1d17b800cf3..9987011da57 100644
--- a/dir.c
+++ b/dir.c
@@ -1392,7 +1392,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
 		if (!(dir->flags & DIR_NO_GITLINKS)) {
 			unsigned char sha1[20];
 			if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
-				return path_untracked;
+				return exclude ? path_excluded : path_untracked;
 		}
 		return path_recurse;
 	}
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index fc6013ba3c8..0c394cf995c 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -272,4 +272,15 @@ test_expect_success 'status ignored tracked directory with uncommitted file in t
 	test_cmp expected actual
 '
 
+cat >expected <<\EOF
+!! tracked/submodule/
+EOF
+
+test_expect_success 'status ignores submodule in excluded directory' '
+	git init tracked/submodule &&
+	test_commit -C tracked/submodule initial &&
+	git status --porcelain --ignored -u tracked/submodule >actual &&
+	test_cmp expected actual
+'
+
 test_done

-- 
2.14.3.windows.1
Junio C Hamano· Oct 26, 2017, 02:28 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 1/1] status: do not get confused by submodules in excluded directories

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 23 quoted lines
> We meticulously pass the `exclude` flag to the `treat_directory()`
> function so that we can indicate that files in it are excluded rather
> than untracked when recursing.
>
> But we did not yet treat submodules the same way.
>
> Because of that, `git status --ignored --untracked` with a submodule
> `submodule` in a gitignored `tracked/` would show the submodule in the
> "Untracked files" section, e.g.
>
> 	On branch master
> 	Untracked files:
> 	  (use "git add <file>..." to include in what will be committed)
>
> 		tracked/submodule/
>
> 	Ignored files:
> 	  (use "git add -f <file>..." to include in what will be committed)
>
> 		tracked/submodule/initial.t
>
> Instead, we would want it to show the submodule in the "Ignored files"
> section:

Makes sense. Also listing the paths in the embedded working tree like initial.t as if it were part of our project is utterly wrong, especially because we are not doing any --recurse-submodules thing.

Both the change and the updated description looks good.  Thanks.

← back to recent threads