[regression?] "git status -a" reports modified for empty submodule directory

[regression?] "git status -a" reports modified for empty submodule directory

[Ping Yin]

# create a super project super
$ mkdir super && cd super && git init
$ touch foo && git add foo && git commit -m "add foo"

# create a sub project sub
$ mkdir sub && cd sub && git init
$ touch bar && git add bar && git commit -m "add bar"

# add sub project to super project
$ cd ..
$ git add sub && git commit -m 'add sub'

# remote contents of subproject
$ rm -rf sub/* sub/.git

# git status -a regression
$ git status
# On branch master
nothing to commit (working directory clean)
$ git status -a
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       deleted:    sub
#


-- 
Ping Yin

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Ping Yin]

On Tue, Apr 22, 2008 at 7:01 PM, Ping Yin <pkufranky@gmail.com> wrote:
> # create a super project super
>  $ mkdir super && cd super && git init
>  $ touch foo && git add foo && git commit -m "add foo"
>
>  # create a sub project sub
>  $ mkdir sub && cd sub && git init
>  $ touch bar && git add bar && git commit -m "add bar"
>
>  # add sub project to super project
>  $ cd ..
>  $ git add sub && git commit -m 'add sub'
>
>  # remote contents of subproject

s/remote/remove/

>  --
>  Ping Yin
>



-- 
Ping Yin

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Johannes Sixt]

Ping Yin schrieb:
> # create a super project super
> $ mkdir super && cd super && git init
> $ touch foo && git add foo && git commit -m "add foo"
> 
> # create a sub project sub
> $ mkdir sub && cd sub && git init
> $ touch bar && git add bar && git commit -m "add bar"
> 
> # add sub project to super project
> $ cd ..
> $ git add sub && git commit -m 'add sub'
> 
> # remote contents of subproject
> $ rm -rf sub/* sub/.git
> 
> # git status -a regression
> $ git status
> # On branch master
> nothing to commit (working directory clean)

This should have reported:

# On branch master
# Changed but not updated:
#   (use "git add/rm <file>..." to update what will be committed)
#
#       deleted:    sub
no changes added to commit (use "git add" and/or "git commit -a")

Right?

> $ git status -a
> # On branch master
> # Changes to be committed:
> #   (use "git reset HEAD <file>..." to unstage)
> #
> #       deleted:    sub
> #

There's nothing wrong with this.

-- Hannes

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Ping Yin]

On Tue, Apr 22, 2008 at 8:15 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Ping Yin schrieb:
>
> > # create a super project super
>  > $ mkdir super && cd super && git init
>  > $ touch foo && git add foo && git commit -m "add foo"
>  >
>  > # create a sub project sub
>  > $ mkdir sub && cd sub && git init
>  > $ touch bar && git add bar && git commit -m "add bar"
>  >
>  > # add sub project to super project
>  > $ cd ..
>  > $ git add sub && git commit -m 'add sub'
>  >
>  > # remote contents of subproject
>  > $ rm -rf sub/* sub/.git
>  >
>  > # git status -a regression
>  > $ git status
>  > # On branch master
>  > nothing to commit (working directory clean)
>
>  This should have reported:
>
>  # On branch master
>  # Changed but not updated:
>  #   (use "git add/rm <file>..." to update what will be committed)
>  #
>  #       deleted:    sub
>  no changes added to commit (use "git add" and/or "git commit -a")
>
>  Right?
>
>
>  > $ git status -a
>  > # On branch master
>  > # Changes to be committed:
>  > #   (use "git reset HEAD <file>..." to unstage)
>  > #
>  > #       deleted:    sub
>  > #
>
>  There's nothing wrong with this.
>
>  -- Hannes

It seems that in 1.5.4, both 'git status' and 'git status -a' report
"no changes added to commit". And i think this is the right behaviour.
Because when a super project is cloned, all submodule directories are
empty in the beginning. In this case 'git status' and 'git status -a'
should report " no changes added to commit".



-- 
Ping Yin

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Johannes Sixt]

Ping Yin schrieb:
> It seems that in 1.5.4, both 'git status' and 'git status -a' report
> "no changes added to commit". And i think this is the right behaviour.
> Because when a super project is cloned, all submodule directories are
> empty in the beginning. In this case 'git status' and 'git status -a'
> should report " no changes added to commit".

That makes sense.

-- Hannes

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Roman Shaposhnik]

On Tue, 2008-04-22 at 20:39 +0800, Ping Yin wrote:
> >  > $ git status -a
> >  > # On branch master
> >  > # Changes to be committed:
> >  > #   (use "git reset HEAD <file>..." to unstage)
> >  > #
> >  > #       deleted:    sub
> >  > #
> >
> >  There's nothing wrong with this.
> >
> >  -- Hannes
> 
> It seems that in 1.5.4, both 'git status' and 'git status -a' report
> "no changes added to commit". And i think this is the right behaviour.
> Because when a super project is cloned, all submodule directories are
> empty in the beginning. In this case 'git status' and 'git status -a'
> should report " no changes added to commit".

Agreed 100%

Thanks,
Roman.

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Ping Yin]

On Tue, Apr 22, 2008 at 7:01 PM, Ping Yin <pkufranky@gmail.com> wrote:
> # create a super project super
>  $ mkdir super && cd super && git init
>  $ touch foo && git add foo && git commit -m "add foo"
>
>  # create a sub project sub
>  $ mkdir sub && cd sub && git init
>  $ touch bar && git add bar && git commit -m "add bar"
>
>  # add sub project to super project
>  $ cd ..
>  $ git add sub && git commit -m 'add sub'
>
>  # remote contents of subproject
>  $ rm -rf sub/* sub/.git
>
>  # git status -a regression
>  $ git status
>  # On branch master
>  nothing to commit (working directory clean)
>  $ git status -a
>  # On branch master
>  # Changes to be committed:
>  #   (use "git reset HEAD <file>..." to unstage)
>  #
>  #       deleted:    sub
>  #
>

Another regression following

In the super project super with empty submodule directory sub
$ git diff
diff --git a/sub b/sub
deleted file mode 160000
index f2c0d45..0000000
--- a/sub
+++ /dev/null
@@ -1 +0,0 @@
-Subproject commit f2c0d4509a3178c...


-- 
Ping Yin

[PATCH 0/2] Add tests for submodule with empty directory

[Ping Yin]

For regression about empty submodule directory

* "git status -a" reports modified
* "git diff" generates diff

[PATCH 1/2] t4027: test diff for submodule with empty directory

[Ping Yin]

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 t/t4027-diff-submodule.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 1fd3fb7..ba6679c 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -50,4 +50,11 @@ test_expect_success 'git diff-files --raw' '
 	test_cmp expect actual.files
 '
 
+test_expect_success 'git diff (empty submodule dir)' '
+	: >empty &&
+	rm -rf sub/* sub/.git &&
+	git diff > actual.empty &&
+	test_cmp empty actual.empty
+'
+
 test_done
-- 
1.5.5.1.96.gef4a.dirty

[PATCH 2/2] Add t7506 to test submodule related functions for git-status

[Ping Yin]

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 t/t7506-status-submodule.sh |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100755 t/t7506-status-submodule.sh

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
new file mode 100755
index 0000000..a75130c
--- /dev/null
+++ b/t/t7506-status-submodule.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='git-status for submodule'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_create_repo sub
+	cd sub &&
+	: >bar &&
+	git add bar &&
+	git commit -m " Add bar" &&
+	cd .. &&
+	git add sub &&
+	git commit -m "Add submodule sub"
+'
+
+test_expect_success 'status clean' '
+	git status |
+	grep "nothing to commit"
+'
+test_expect_success 'status -a clean' '
+	git status -a |
+	grep "nothing to commit"
+'
+test_expect_success 'rm submodule contents' '
+	rm -rf sub/* sub/.git
+'
+test_expect_success 'status clean (empty submodule dir)' '
+	git status |
+	grep "nothing to commit"
+'
+test_expect_success 'status -a clean (empty submodule dir)' '
+	git status -a |
+	grep "nothing to commit"
+'
+
+test_done
-- 
1.5.5.1.96.gef4a.dirty

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Junio C Hamano]

"Ping Yin" <pkufranky@gmail.com> writes:

> In the super project super with empty submodule directory sub
> $ git diff
> diff --git a/sub b/sub
> deleted file mode 160000
> index f2c0d45..0000000
> --- a/sub
> +++ /dev/null
> @@ -1 +0,0 @@
> -Subproject commit f2c0d4509a3178c...

The repository used to have a subproject and now it doesn't.  Why
shouldn't it report the removal?

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Johannes Sixt]

Junio C Hamano schrieb:
> "Ping Yin" <pkufranky@gmail.com> writes:
> 
>> In the super project super with empty submodule directory sub
>> $ git diff
>> diff --git a/sub b/sub
>> deleted file mode 160000
>> index f2c0d45..0000000
>> --- a/sub
>> +++ /dev/null
>> @@ -1 +0,0 @@
>> -Subproject commit f2c0d4509a3178c...
> 
> The repository used to have a subproject and now it doesn't.  Why
> shouldn't it report the removal?

Because you are not required to have a subproject checked out?

-- Hannes

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
>> "Ping Yin" <pkufranky@gmail.com> writes:
>> 
>>> In the super project super with empty submodule directory sub
>>> $ git diff
>>> diff --git a/sub b/sub
>>> deleted file mode 160000
>>> index f2c0d45..0000000
>>> --- a/sub
>>> +++ /dev/null
>>> @@ -1 +0,0 @@
>>> -Subproject commit f2c0d4509a3178c...
>> 
>> The repository used to have a subproject and now it doesn't.  Why
>> shouldn't it report the removal?
>
> Because you are not required to have a subproject checked out?

Yes, but.

This is a policy issue, which is not very well enforced currently.

I have been scratching my head to figure out where the right balance we
should strike at for the past week and a half.

For example, it has long been known ever since submodules were introduced
that if you work inside a sparsely checked out supermodule you have to use
"commit -a" with care, because the command notices missing submodule, and
there is no way for it to differenciate between the case you _want to_
remove it and the case you did not care about it, so you will end up
removing unchecked out submodules.

That quirk was something people could live with while submodule support
was merely a newly invented curiosity.  But I do not think a command at
high level (iow Porcelain) such as commit and status should be left
unaware of the Policy that equate missing submodule and unmodified one
forever.  We should actively enforce the policy, so that unless you
explicitly ask nicely, the command should consider a missing submodule
just as unmodified, e.g. "commit -a" should not remove unchecked out
submodules.

But then you would need a way to ask nicely.  How?  Perhaps using "git rm",
and low level "update-index --remove".  Do we even need "commit -A"?  I
doubt it --- you do not remove submodules every day.

We'd like to keep the lowest-level unaware of the Policy, which means that
"diff-files" and "diff-index" should report unchecked out submodules.
Otherwise script writers will be left with no way to differenciate missing
and removed submodules.

Once we start doing this, I think "git diff" Porcelain should fall into
Policy-aware category.

Re: [regression?] "git status -a" reports modified for empty submodule directory

[Ping Yin]

On Wed, Apr 30, 2008 at 3:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>
>  For example, it has long been known ever since submodules were introduced
>  that if you work inside a sparsely checked out supermodule you have to use
>  "commit -a" with care, because the command notices missing submodule, and
>  there is no way for it to differenciate between the case you _want to_
>  remove it and the case you did not care about it,

If i am not misunderstood, there is one way now (although broken now,
it used this kind of behaviour before git-status became built-in):
empty directory to denote that the submodule is not checked out and
unaware, and deleted directory to denote that the submodule is
deleted.

Actually, i don't like the empty directory trick, since it will leave
so many empty directories in a repository with many submodules
unchecked out.

>
>  That quirk was something people could live with while submodule support
>  was merely a newly invented curiosity.  But I do not think a command at
>  high level (iow Porcelain) such as commit and status should be left
>  unaware of the Policy that equate missing submodule and unmodified one
>  forever.  We should actively enforce the policy, so that unless you
>  explicitly ask nicely, the command should consider a missing submodule
>  just as unmodified, e.g. "commit -a" should not remove unchecked out
>  submodules.
>
>  But then you would need a way to ask nicely.  How?  Perhaps using "git rm",
>  and low level "update-index --remove".  Do we even need "commit -A"?  I
>  doubt it --- you do not remove submodules every day.
>
>  We'd like to keep the lowest-level unaware of the Policy, which means that
>  "diff-files" and "diff-index" should report unchecked out submodules.
>  Otherwise script writers will be left with no way to differenciate missing
>  and removed submodules.

Good point.

>
>  Once we start doing this, I think "git diff" Porcelain should fall into
>  Policy-aware category.
>

Agree this pilicy.

If this change needs a long way. Should we fix the regression first?
Anyway, 'git status' and 'git status -a' should behave the same for
submodules unchecked out. I have tried but i failed. I just found this
regression was introduced on the first day of built-in status

-- 
Ping Yin

[PATCH 0/4] Fix regression for unchecked out submodules

[Ping Yin]

> If this change needs a long way. Should we fix the regression first?
> Anyway, 'git status' and 'git status -a' should behave the same for
> submodules unchecked out. I have tried but i failed. I just found this
> regression was introduced on the first day of built-in status

Ok, this is my try to fix the regression.

[PATCH 1/4] t4027: test diff for submodule with empty directory

[Ping Yin]

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 t/t4027-diff-submodule.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 1fd3fb7..ba6679c 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -50,4 +50,11 @@ test_expect_success 'git diff-files --raw' '
 	test_cmp expect actual.files
 '
 
+test_expect_success 'git diff (empty submodule dir)' '
+	: >empty &&
+	rm -rf sub/* sub/.git &&
+	git diff > actual.empty &&
+	test_cmp empty actual.empty
+'
+
 test_done
-- 
1.5.5.1.116.ge4b9c.dirty

[PATCH 2/4] Add t7506 to test submodule related functions for git-status

[Ping Yin]

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 t/t7506-status-submodule.sh |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100755 t/t7506-status-submodule.sh

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
new file mode 100755
index 0000000..a75130c
--- /dev/null
+++ b/t/t7506-status-submodule.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='git-status for submodule'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_create_repo sub
+	cd sub &&
+	: >bar &&
+	git add bar &&
+	git commit -m " Add bar" &&
+	cd .. &&
+	git add sub &&
+	git commit -m "Add submodule sub"
+'
+
+test_expect_success 'status clean' '
+	git status |
+	grep "nothing to commit"
+'
+test_expect_success 'status -a clean' '
+	git status -a |
+	grep "nothing to commit"
+'
+test_expect_success 'rm submodule contents' '
+	rm -rf sub/* sub/.git
+'
+test_expect_success 'status clean (empty submodule dir)' '
+	git status |
+	grep "nothing to commit"
+'
+test_expect_success 'status -a clean (empty submodule dir)' '
+	git status -a |
+	grep "nothing to commit"
+'
+
+test_done
-- 
1.5.5.1.116.ge4b9c.dirty

[PATCH 3/4] Fix diff regression for submodules not checked out

[Ping Yin]

This regression is introduced by f58dbf23c3, which calls
check_work_tree_entity in run_diff_files. While check_work_tree_entity
treats submodule not checked out as non stagable which causes that
diff-files shows these submodules as deleted.

check_work_tree_entity considers a worktree entity having two statuses:
stagable and inexistent. Actually, there is a 3rd status: a submodule
entity can be existent but not stagable (for example, empty directory for
non-checked-out submodule)

This patch redesigns the return value of check_work_tree_entity to
consider both the 3 statuses.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 diff-lib.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index cfd629d..72c2a7b 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -337,25 +337,29 @@ int run_diff_files_cmd(struct rev_info *revs, int argc, const char **argv)
 	}
 	return run_diff_files(revs, options);
 }
+
+#define ENT_STAGABLE 1
+#define ENT_INEXISTENT 2
+#define ENT_NOTGITDIR 3		/* Existent but not stagable (not a git dir) */
 /*
- * See if work tree has an entity that can be staged.  Return 0 if so,
- * return 1 if not and return -1 if error.
+ * Check the status of a work tree entity
+ * Return ENT_{STAGABLE,INEXISTENT,NOTGITDIR} or -1 if error
  */
 static int check_work_tree_entity(const struct cache_entry *ce, struct stat *st, char *symcache)
 {
 	if (lstat(ce->name, st) < 0) {
 		if (errno != ENOENT && errno != ENOTDIR)
 			return -1;
-		return 1;
+		return ENT_INEXISTENT;
 	}
 	if (has_symlink_leading_path(ce->name, symcache))
-		return 1;
+		return ENT_INEXISTENT;
 	if (S_ISDIR(st->st_mode)) {
 		unsigned char sub[20];
 		if (resolve_gitlink_ref(ce->name, "HEAD", sub))
-			return 1;
+			return ENT_NOTGITDIR;
 	}
-	return 0;
+	return ENT_STAGABLE;
 }
 
 int run_diff_files(struct rev_info *revs, unsigned int option)
@@ -403,7 +407,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			       sizeof(struct combine_diff_parent)*5);
 
 			changed = check_work_tree_entity(ce, &st, symcache);
-			if (!changed)
+			if (changed != ENT_INEXISTENT)
 				dpath->mode = ce_mode_from_stat(ce, st.st_mode);
 			else {
 				if (changed < 0) {
@@ -467,7 +471,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			continue;
 
 		changed = check_work_tree_entity(ce, &st, symcache);
-		if (changed) {
+		if (changed == ENT_INEXISTENT) {
 			if (changed < 0) {
 				perror(ce->name);
 				continue;
@@ -527,7 +531,7 @@ static int get_stat_data(struct cache_entry *ce,
 		changed = check_work_tree_entity(ce, &st, cbdata->symcache);
 		if (changed < 0)
 			return -1;
-		else if (changed) {
+		else if (changed == ENT_INEXISTENT) {
 			if (match_missing) {
 				*sha1p = sha1;
 				*modep = mode;
-- 
1.5.5.1.116.ge4b9c.dirty

[PATCH 4/4] Fix ie_match_stat for non-checked-out submodule

[Ping Yin]

For submodules non checked out, ie_match_stat should always return 0.
So in this case avoid calling is_racy_timestamp.

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 read-cache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index a92b25b..8c9c8e2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -294,7 +294,7 @@ int ie_match_stat(const struct index_state *istate,
 	 * whose mtime are the same as the index file timestamp more
 	 * carefully than others.
 	 */
-	if (!changed && is_racy_timestamp(istate, ce)) {
+	if (!changed && !S_ISGITLINK(ce->ce_mode) && is_racy_timestamp(istate, ce)) {
 		if (assume_racy_is_modified)
 			changed |= DATA_CHANGED;
 		else
-- 
1.5.5.1.116.ge4b9c.dirty

Re: [PATCH 4/4] Fix ie_match_stat for non-checked-out submodule

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> For submodules non checked out, ie_match_stat should always return 0.
> So in this case avoid calling is_racy_timestamp.

It is conceptually wrong to have this check in that function, I think.

Look at what ce_match_stat_basic() does.  For S_IFGITLINK entries, we do
not even compare the timestamps, so is_racy_timestamp() check should not
even care and should return Ok for them.

Perhaps this patch would be a better, in that it covers the other caller
on the write_index() callpath as well.

---
diff --git a/read-cache.c b/read-cache.c
index a92b25b..0a0ea3b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -258,6 +258,7 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 static int is_racy_timestamp(const struct index_state *istate, struct cache_entry *ce)
 {
 	return (istate->timestamp &&
+		!S_ISGITLINK(ce->ce_mode) &&
 		((unsigned int)istate->timestamp) <= ce->ce_mtime);
 }
 

Re: [PATCH 3/4] Fix diff regression for submodules not checked out

[Junio C Hamano]

Ping Yin <pkufranky@gmail.com> writes:

> This regression is introduced by f58dbf23c3, which calls
> check_work_tree_entity in run_diff_files.  While check_work_tree_entity
> treats submodule not checked out as non stagable which causes that
> diff-files shows these submodules as deleted.

>  int run_diff_files(struct rev_info *revs, unsigned int option)
> @@ -403,7 +407,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			       sizeof(struct combine_diff_parent)*5);
>  
>  			changed = check_work_tree_entity(ce, &st, symcache);
> -			if (!changed)
> +			if (changed != ENT_INEXISTENT)
>  				dpath->mode = ce_mode_from_stat(ce, st.st_mode);

>  			else {
>  				if (changed < 0) {
> @@ -467,7 +471,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			continue;
>  
>  		changed = check_work_tree_entity(ce, &st, symcache);
> -		if (changed) {
> +		if (changed == ENT_INEXISTENT) {
>  			if (changed < 0) {
>  				perror(ce->name);
>  				continue;
> @@ -527,7 +531,7 @@ static int get_stat_data(struct cache_entry *ce,
>  		changed = check_work_tree_entity(ce, &st, cbdata->symcache);
>  		if (changed < 0)
>  			return -1;
> -		else if (changed) {
> +		else if (changed == ENT_INEXISTENT) {
>  			if (match_missing) {
>  				*sha1p = sha1;
>  				*modep = mode;

Earier I said we may have to teach the Porcelain layer (status, diff) to
equate a submodule that is not checked out and a submodule that is not
modified while keeping low-level plumbing (diff-files and diff-index)
still aware that the submodule is missing from the work tree, but that was
because I incorrectly thought there are only two cases (either the
submodule is fully checked out or the submodule directory itself does not
even exist) and treating the latter the same as an unmodified case would
mean there won't be an easy way to remove the submodule from the
superproject for Porcelains that are written in terms of diff-files and
diff-index.

But that was a faulty thinking on my part (heh, why didn't anybody correct
me?).  Actually, there are three cases:

 - submodule directory exists and it is a full fledged repository.  It may
   or may not be modified, but we can tell by looking at its .git/HEAD.

 - submodule directory exists but there is nothing there (no "init" nor
   "update" was done, just an empty directory checked out).  This is how a
   superproject with a submodule is checked out by default.

 - submodule directory itself does not even exist.

The second case is "not checked out -- treat me as unmodified", and the
third case is "the user does not want the submodule there", and the latter
is still reported as "removed".  That is exactly what your patch does.

I like it.

By the way, "inexistent" is a word, but somehow it sounds quite awkward.
Perhaps one of NONEXISTENT (more common), REMOVED (run_diff_files() takes
a SILENT_ON_REMOVED option) or or MISSING (update-index --refresh takes an
IGNORE_MISSING option) is better?  I dunno.

Re: [PATCH 4/4] Fix ie_match_stat for non-checked-out submodule

[Ping Yin]

On Sat, May 3, 2008 at 5:57 AM, Junio C Hamano <gitster@pobox.com> wrote:

>
> --- a/read-cache.c
>  +++ b/read-cache.c
>  @@ -258,6 +258,7 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
>   static int is_racy_timestamp(const struct index_state *istate, struct cache_entry *ce)
>   {
>         return (istate->timestamp &&
>  +               !S_ISGITLINK(ce->ce_mode) &&
>                 ((unsigned int)istate->timestamp) <= ce->ce_mtime);
>   }
>

fine



-- 
Ping Yin

Re: [PATCH 3/4] Fix diff regression for submodules not checked out

[Ping Yin]

On Sat, May 3, 2008 at 6:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ping Yin <pkufranky@gmail.com> writes:
>
>  > This regression is introduced by f58dbf23c3, which calls
>  > check_work_tree_entity in run_diff_files.  While check_work_tree_entity
>  > treats submodule not checked out as non stagable which causes that
>  > diff-files shows these submodules as deleted.
>
>
>
> >  int run_diff_files(struct rev_info *revs, unsigned int option)
>  > @@ -403,7 +407,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  >                              sizeof(struct combine_diff_parent)*5);
>  >
>  >                       changed = check_work_tree_entity(ce, &st, symcache);
>  > -                     if (!changed)
>  > +                     if (changed != ENT_INEXISTENT)
>  >                               dpath->mode = ce_mode_from_stat(ce, st.st_mode);
>
>  >                       else {
>  >                               if (changed < 0) {
>  > @@ -467,7 +471,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  >                       continue;
>  >
>  >               changed = check_work_tree_entity(ce, &st, symcache);
>  > -             if (changed) {
>  > +             if (changed == ENT_INEXISTENT) {
>  >                       if (changed < 0) {
>  >                               perror(ce->name);
>  >                               continue;
>  > @@ -527,7 +531,7 @@ static int get_stat_data(struct cache_entry *ce,
>  >               changed = check_work_tree_entity(ce, &st, cbdata->symcache);
>  >               if (changed < 0)
>  >                       return -1;
>  > -             else if (changed) {
>  > +             else if (changed == ENT_INEXISTENT) {
>  >                       if (match_missing) {
>  >                               *sha1p = sha1;
>  >                               *modep = mode;
>
>  Earier I said we may have to teach the Porcelain layer (status, diff) to
>  equate a submodule that is not checked out and a submodule that is not
>  modified while keeping low-level plumbing (diff-files and diff-index)
>  still aware that the submodule is missing from the work tree, but that was
>  because I incorrectly thought there are only two cases (either the
>  submodule is fully checked out or the submodule directory itself does not
>  even exist) and treating the latter the same as an unmodified case would
>  mean there won't be an easy way to remove the submodule from the
>  superproject for Porcelains that are written in terms of diff-files and
>  diff-index.
>
>  But that was a faulty thinking on my part (heh, why didn't anybody correct
>  me?).  Actually, there are three cases:

Actually, i pointed out in early discussion that we use empty
directory (or directory without .git subdirectory) to represent the
3rd case. However, i don't like the trick. And i don't think you are
thinking faultily because I prefer your idea: we treat nonexistent
directory and directory without .git as the same for 'git diff'. "git
diff" will show "no modified" for both case.

One point i don't agree with you is that i think diff-files should
also show "no modified" for both case. By doing this, we can avoid the
empty directory for nonchecked out submodules. For a project with
hundreds of submodules, it's really really annoying to see so many
unused empty directories.

So how we diffrentiate removed and unchecked out? For submodule, we
don't differentiate it. If you want to remove a submodule, use "git
rm" or "git update-index --removed" instead of "rm && git commit -a".

>
>   - submodule directory exists and it is a full fledged repository.  It may
>    or may not be modified, but we can tell by looking at its .git/HEAD.
>
>   - submodule directory exists but there is nothing there (no "init" nor
>    "update" was done, just an empty directory checked out).  This is how a
>    superproject with a submodule is checked out by default.
>
>   - submodule directory itself does not even exist.
>
>  The second case is "not checked out -- treat me as unmodified", and the
>  third case is "the user does not want the submodule there", and the latter
>  is still reported as "removed".  That is exactly what your patch does.

Great, you make the 3 cases more clear.



-- 
Ping Yin

Re: [PATCH] Rename ENT_INEXISTENT to ENT_NONEXISTENT

[Ping Yin]

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
> By the way, "inexistent" is a word, but somehow it sounds quite awkward.
> Perhaps one of NONEXISTENT (more common), REMOVED (run_diff_files() takes
> a SILENT_ON_REMOVED option) or or MISSING (update-index --refresh takes an
> IGNORE_MISSING option) is better? 

I prefer nonexistent because removed or missing has the meaning that the
user has removed it. However, it may be not this case (althogh it is at
current time).


 diff-lib.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 72c2a7b..61a1b7c 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -339,7 +339,7 @@ int run_diff_files_cmd(struct rev_info *revs, int argc, const char **argv)
 }
 
 #define ENT_STAGABLE 1
-#define ENT_INEXISTENT 2
+#define ENT_NONEXISTENT 2
 #define ENT_NOTGITDIR 3		/* Existent but not stagable (not a git dir) */
 /*
  * Check the status of a work tree entity
@@ -350,10 +350,10 @@ static int check_work_tree_entity(const struct cache_entry *ce, struct stat *st,
 	if (lstat(ce->name, st) < 0) {
 		if (errno != ENOENT && errno != ENOTDIR)
 			return -1;
-		return ENT_INEXISTENT;
+		return ENT_NONEXISTENT;
 	}
 	if (has_symlink_leading_path(ce->name, symcache))
-		return ENT_INEXISTENT;
+		return ENT_NONEXISTENT;
 	if (S_ISDIR(st->st_mode)) {
 		unsigned char sub[20];
 		if (resolve_gitlink_ref(ce->name, "HEAD", sub))
@@ -407,7 +407,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			       sizeof(struct combine_diff_parent)*5);
 
 			changed = check_work_tree_entity(ce, &st, symcache);
-			if (changed != ENT_INEXISTENT)
+			if (changed != ENT_NONEXISTENT)
 				dpath->mode = ce_mode_from_stat(ce, st.st_mode);
 			else {
 				if (changed < 0) {
@@ -471,7 +471,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			continue;
 
 		changed = check_work_tree_entity(ce, &st, symcache);
-		if (changed == ENT_INEXISTENT) {
+		if (changed == ENT_NONEXISTENT) {
 			if (changed < 0) {
 				perror(ce->name);
 				continue;
@@ -531,7 +531,7 @@ static int get_stat_data(struct cache_entry *ce,
 		changed = check_work_tree_entity(ce, &st, cbdata->symcache);
 		if (changed < 0)
 			return -1;
-		else if (changed == ENT_INEXISTENT) {
+		else if (changed == ENT_NONEXISTENT) {
 			if (match_missing) {
 				*sha1p = sha1;
 				*modep = mode;
-- 
1.5.5.1.117.g73010

Re: [PATCH 3/4] Fix diff regression for submodules not checked out

[Johannes Schindelin]

Hi,

On Fri, 2 May 2008, Junio C Hamano wrote:

> By the way, "inexistent" is a word, but somehow it sounds quite awkward.

In the same vein, I had to think about "stagable" for some time.  I think 
the correct term is "stageable", but then, I am not sure if there is no 
better word anyway.

Ciao,
Dscho

Re: [PATCH 3/4] Fix diff regression for submodules not checked out

[Junio C Hamano]

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

> In the same vein, I had to think about "stagable" for some time.  I think 
> the correct term is "stageable", but then, I am not sure if there is no 
> better word anyway.

Oh, I did not even mention that because it quite obviously is an typo of a
non-word ;-)

Re: [PATCH 3/4] Fix diff regression for submodules not checked out

[Junio C Hamano]

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

> The second case is "not checked out -- treat me as unmodified", and the
> third case is "the user does not want the submodule there", and the latter
> is still reported as "removed".  That is exactly what your patch does.

Having looked at the code a bit more, I do not think we need the
three-kind distinction for this part.

The attached patch would be both sufficient and cleaner.  The real change
is a single-liner, and everything else is additional comment ;-)  I'd
follow it up with s/check_work_tree_entity/check_removed/ for
clarification.

A cleaned up series is queued near the tip of 'pu' for tonight, but 'pu'
itself has some uncompiable crap in it (not your series) and needs to be
rebuilt soon.

---
[PATCH] diff: a submodule not checked out is not modified

948dd34 (diff-index: careful when inspecting work tree items, 2008-03-30)
made the work tree check careful not to be fooled by a new directory that
exists at a place the index expects a blob.  For such a change to be a
typechange from blob to submodule, the new directory has to be a
repository.

However, if the index expects a submodule there, we should not insist the
work tree entity to be a repository --- a simple directory that is not a
full fledged repository (even an empty directory would do) should be
considered an unmodified subproject, because that is how a superproject
with a submodule is checked out sparsely by default.

This makes the function check_work_tree_entity() even more careful not to
report a submodule that is not checked out as removed.  It fixes the
recently added test in t4027.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-lib.c                |   25 ++++++++++++++++++++++---
 t/t4027-diff-submodule.sh |    2 +-
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index cfd629d..e0ebcdc 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -337,9 +337,15 @@ int run_diff_files_cmd(struct rev_info *revs, int argc, const char **argv)
 	}
 	return run_diff_files(revs, options);
 }
+
 /*
- * See if work tree has an entity that can be staged.  Return 0 if so,
- * return 1 if not and return -1 if error.
+ * Has the work tree entity been removed?
+ *
+ * Return 1 if it was removed from the work tree, 0 if an entity to be
+ * compared with the cache entry ce still exists (the latter includes
+ * the case where a directory that is not a submodule repository
+ * exists for ce that is a submodule -- it is a submodule that is not
+ * checked out).  Return negative for an error.
  */
 static int check_work_tree_entity(const struct cache_entry *ce, struct stat *st, char *symcache)
 {
@@ -352,7 +358,20 @@ static int check_work_tree_entity(const struct cache_entry *ce, struct stat *st,
 		return 1;
 	if (S_ISDIR(st->st_mode)) {
 		unsigned char sub[20];
-		if (resolve_gitlink_ref(ce->name, "HEAD", sub))
+
+		/*
+		 * If ce is already a gitlink, we can have a plain
+		 * directory (i.e. the submodule is not checked out),
+		 * or a checked out submodule.  Either case this is not
+		 * a case where something was removed from the work tree,
+		 * so we will return 0.
+		 *
+		 * Otherwise, if the directory is not a submodule
+		 * repository, that means ce which was a blob turned into
+		 * a directory --- the blob was removed!
+		 */
+		if (!S_ISGITLINK(ce->ce_mode) &&
+		    resolve_gitlink_ref(ce->name, "HEAD", sub))
 			return 1;
 	}
 	return 0;
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 61caad0..ba6679c 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -50,7 +50,7 @@ test_expect_success 'git diff-files --raw' '
 	test_cmp expect actual.files
 '
 
-test_expect_failure 'git diff (empty submodule dir)' '
+test_expect_success 'git diff (empty submodule dir)' '
 	: >empty &&
 	rm -rf sub/* sub/.git &&
 	git diff > actual.empty &&
-- 
1.5.5.1.219.gb8f92

Re: [PATCH 3/4] Fix diff regression for submodules not checked out

[Ping Yin]

On Sun, May 4, 2008 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>  > The second case is "not checked out -- treat me as unmodified", and the
>  > third case is "the user does not want the submodule there", and the latter
>  > is still reported as "removed".  That is exactly what your patch does.
>
>  Having looked at the code a bit more, I do not think we need the
>  three-kind distinction for this part.
>
>  The attached patch would be both sufficient and cleaner.  The real change
>  is a single-liner, and everything else is additional comment ;-)  I'd
>  follow it up with s/check_work_tree_entity/check_removed/ for
>  clarification.
>

Fine, it's cleaner.

-- 
Ping Yin