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

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

[Johannes Schindelin]

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
+	) &&
+	git status --porcelain --ignored -u tracked/submodule >actual &&
+	test_cmp expected actual
+'
+
 test_done

base-commit: 111ef79afe185f8731920569450f6a65320f5d5f
-- 
2.14.2.windows.3

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

[Junio C Hamano]

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.  

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.

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

[Kevin Daudt]

On Tue, Oct 17, 2017 at 03:10:11PM +0200, Johannes Schindelin wrote:
> 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?

> +	git status --porcelain --ignored -u tracked/submodule >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_done
> 
> base-commit: 111ef79afe185f8731920569450f6a65320f5d5f
> -- 
> 2.14.2.windows.3

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

[Heiko Voigt]

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.

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

> 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

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

[Stefan Beller]

On Tue, Oct 24, 2017 at 5:15 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:

> Looks good to me.

Same here,

Thanks,
Stefan

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

[Junio C Hamano]

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? ;-)

> Looks good to me.
>
> Cheers Heiko

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

[Johannes Schindelin]

Hi Kevin,

On Tue, 24 Oct 2017, Kevin Daudt wrote:

> 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

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

[Heiko Voigt]

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?

Cheers Heiko

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

[Johannes Schindelin]

Hi,

On Wed, 25 Oct 2017, Heiko Voigt wrote:

> 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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

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

[Junio C Hamano]

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, `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.