Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:05
Possibly related (same subject, not in this thread)
- 2016-06-15 · What's cooking in git.git (Jan 2010, #07; Fri, 22) · Junio C Hamano <hidden>
- 2016-06-15 · Re: What's cooking in git.git (Jan 2010, #07; Fri, 22) · Jakub Narebski <hidden>
- 2016-06-15 · Re: What's cooking in git.git (Jan 2010, #07; Fri, 22) · Jens Lehmann <hidden>
- 2016-06-15 · Re: What's cooking in git.git (Jan 2010, #07; Fri, 22) · Junio C Hamano <hidden>
- 2016-06-15 · Re: What's cooking in git.git (Jan 2010, #07; Fri, 22) · Junio C Hamano <hidden>
Jens Lehmann [off-list ref] writes:
A patch that teaches "git diff --submodule" to display if the submodule contains new untracked and/or modified files is also almost ready.
How does "git submodule summary" show them? If it doesn't, then I don't think we would want to show them, either, as my understanding is that a longer-term plan is to use "diff --submodule" in git-gui to replace it.
Would you consider it for inclusion into 1.7.0 too or shall i wait until after the release?
If a feature is not in 'master' already, I think it is too late to be included in 1.7.0, if that is what you are asking. But if you start the usual cycle of working on, asking others to review and polishing it before the release, it would give us better designed and more tested version in 1.7.1, no?
quoted hunk
diff --git a/diff-lib.c b/diff-lib.c index ec2e2ac..e896b9c 100644 --- a/diff-lib.c +++ b/diff-lib.c@@ -161,7 +161,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option) continue; } - if ((ce_uptodate(ce) && !S_ISGITLINK(ce->ce_mode)) || ce_skip_worktree(ce)) + if ((ce_uptodate(ce) + && (!S_ISGITLINK(ce->ce_mode) + || DIFF_OPT_TST(&revs->diffopt, IGNORE_SUBMODULES))) + || ce_skip_worktree(ce)) continue;
I think this is sensible; the frontend knows that it doesn't care about submodules.
quoted hunk
@@ -179,6 +182,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option) } changed = ce_match_stat(ce, &st, ce_option); if (S_ISGITLINK(ce->ce_mode) + && !DIFF_OPT_TST(&revs->diffopt, IGNORE_SUBMODULES) && (!changed || (revs->diffopt.output_format & DIFF_FORMAT_PATCH)) && is_submodule_modified(ce->name)) { changed = 1;
Likewise, but with one "hmph". This codepath deals with a path that is a submodule in the index (the work tree may still have submodule, removed it, or replaced it with a regular file). However, in the codepath that follows this up to the call to diff_change(), is_submodule_modified() is not called if the work tree has a submodule in a path that used to be something else. Do we want one?
quoted hunk
@@ -220,7 +224,7 @@ static int get_stat_data(struct cache_entry *ce, const unsigned char **sha1p, unsigned int *modep, int cached, int match_missing, - unsigned *dirty_submodule, int output_format) + unsigned *dirty_submodule, struct diff_options *diffopt) { const unsigned char *sha1 = ce->sha1; unsigned int mode = ce->ce_mode;
Below the context of this hunk, we seem to do this:
if (!cached && !ce_uptodate(ce)) {
...
if gitlink then call is_submodule_modified()
}
But isn't it inconsistent with hunk at the beginning of this patch (ll 161-170)
that essentially says "entries that is ce_uptodate() is Ok, but if it is a
gitlink we need to look deeper"? Why isn't this function looking deeper
even when we see that the submodule entry is ce_uptodate()?
Side note: the lack of ce_skip_worktree() check is Ok. The callers of
get_stat_data() are show_new_file() and show_mododified() but they are
never called from their sole caller, do_oneway_diff(), on a skipped
worktree entry.
quoted hunk
@@ -241,7 +245,8 @@ static int get_stat_data(struct cache_entry *ce, } changed = ce_match_stat(ce, &st, 0); if (S_ISGITLINK(ce->ce_mode) - && (!changed || (output_format & DIFF_FORMAT_PATCH)) + && !DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES) + && (!changed || (diffopt->output_format & DIFF_FORMAT_PATCH)) && is_submodule_modified(ce->name)) { changed = 1; *dirty_submodule = 1;
This hunk by itself is Ok, but I am still puzzled about a case where you have "seemingly clean because ce_uptodate() says so, but submodule work tree or index might be dirty" case, in which this code won't trigger.