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

["Ping Yin" <pkufranky@gmail.com>]

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