Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 23, 2010, 20:03 UTC
- Message-ID
- <7v7hr8d1xg.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4B5B25C6.70604@web.de>
Jens Lehmann <Jens.Lehmann@web.de> 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?
Show 14 quoted lines
> 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.
Show 8 quoted lines
> @@ -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?
Show 9 quoted lines
> @@ -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.Show 10 quoted lines
> @@ -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.