Jens Lehmann <Jens.Lehmann@xxxxxx> 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?
> 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.
> @@ -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?
> @@ -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.
> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html