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

From: Junio C Hamano <gitster@pobox.com>
Date: May 2, 2008, 22:23 UTC
Message-ID: <7vfxt0wdkq.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1209735336-4690-4-git-send-email-pkufranky@gmail.com>

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.

Show 8 quoted lines

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

Show 20 quoted lines

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