What's cooking in git.git (Jan 2010, #07; Fri, 22)

What's cooking in git.git (Jan 2010, #07; Fri, 22)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' while commits prefixed with '+' are in 'next'.  The ones
marked with '.' do not appear in any of the integration branches, but I am
still holding onto them.

--------------------------------------------------
[Graduated to "master"]

* jc/conflict-marker-size (2010-01-16) 8 commits
  (merged to 'next' on 2010-01-18 at f1f6023)
 + rerere: honor conflict-marker-size attribute
 + rerere: prepare for customizable conflict marker length
 + conflict-marker-size: new attribute
 + rerere: use ll_merge() instead of using xdl_merge()
 + merge-tree: use ll_merge() not xdl_merge()
 + xdl_merge(): allow passing down marker_size in xmparam_t
 + xdl_merge(): introduce xmparam_t for merge specific parameters
 + git_attr(): fix function signature

* ag/maint-apply-too-large-p (2010-01-17) 1 commit
  (merged to 'next' on 2010-01-18 at 8bd106a)
 + builtin-apply.c: Skip filenames without enough components

* ag/patch-header-verify (2010-01-18) 1 commit
  (merged to 'next' on 2010-01-18 at 2cd0ddc)
 + builtin-apply.c: fix the --- and +++ header filename consistency check

* bw/cvsimport (2010-01-19) 3 commits
  (merged to 'next' on 2010-01-19 at 63f4c8d)
 + cvsimport: standarize system() calls to external git tools
 + cvsimport: standarize open() calls to external git tools
 + cvsimport: modernize callouts to git subcommands

* jc/checkout-merge-base (2010-01-19) 1 commit
  (merged to 'next' on 2010-01-19 at 3665110)
 + Fix "checkout A..." synonym for "checkout A...HEAD" on Windows

* jc/maint-refresh-index-is-optional-for-status (2010-01-19) 1 commit
 + status: don't require the repository to be writable

* nd/status-partial-refresh (2010-01-17) 2 commits
  (merged to 'next' on 2010-01-19 at 64f0c0b)
 + rm: only refresh entries that we may touch
  (merged to 'next' on 2010-01-16 at f77bc8f)
 + status: only touch path we may need to check

* ap/merge-backend-opts (2008-07-18) 7 commits
  (merged to 'next' on 2010-01-18 at cb1f6b7)
 + Document that merge strategies can now take their own options
 + Extend merge-subtree tests to test -Xsubtree=dir.
 + Make "subtree" part more orthogonal to the rest of merge-recursive.
 + pull: Fix parsing of -X<option>
 + Teach git-pull to pass -X<option> to git-merge
 + git merge -X<option>
 + git-merge-file --ours, --theirs

* jc/maint-limit-note-output (2010-01-21) 2 commits
  (merged to 'next' on 2010-01-21 at bcb80b9)
 + Fix "log --oneline" not to show notes
  (merged to 'next' on 2010-01-20 at 526bfcc)
 + Fix "log" family not to be too agressive about showing notes

* nd/ls-files-sparse-fix (2010-01-20) 1 commit
  (merged to 'next' on 2010-01-20 at 0f61dbc)
 + Fix memory corruption when .gitignore does not end by \n

* il/branch-set-upstream (2010-01-18) 2 commits
  (merged to 'next' on 2010-01-18 at b9b0993)
 + branch: warn and refuse to set a branch as a tracking branch of itself.
 + Add branch --set-upstream

* il/remote-updates (2010-01-18) 1 commit
  (merged to 'next' on 2010-01-18 at 5c3e805)
 + Add git remote set-url

* il/rev-glob (2010-01-22) 3 commits
  (merged to 'next' on 2010-01-21 at 453a21c)
 + Documentation: improve description of --glob=pattern and friends
  (merged to 'next' on 2010-01-20 at 928ba0a)
 + rev-parse --branches/--tags/--remotes=pattern
 + rev-parse --glob

This is a re-rolled "--namespace=" one.

* jl/submodule-diff (2010-01-18) 4 commits
  (merged to 'next' on 2010-01-20 at 95cb513)
 + Performance optimization for detection of modified submodules
  (merged to 'next' on 2010-01-17 at 525075b)
 + git status: Show uncommitted submodule changes too when enabled
  (merged to 'next' on 2010-01-16 at 0a99e3c)
 + Teach diff that modified submodule directory is dirty
 + Show submodules as modified when they contain a dirty work tree

* js/refer-upstream (2010-01-19) 3 commits
  (merged to 'next' on 2010-01-20 at 5a5547a)
 + Teach @{upstream} syntax to strbuf_branchanme()
 + t1506: more test for @{upstream} syntax
 + Introduce <branch>@{upstream} notation

Updated to teach the new syntax to commands like "checkout" and "merge"
that want to behave better when they know what were given was a branch
name, not a random SHA-1.

* jc/branch-d (2009-12-29) 1 commit
  (merged to 'next' on 2010-01-10 at 61a14b7)
 + branch -d: base the "already-merged" safety on the branch it merges with

--------------------------------------------------
[Will merge to 'master' after a bit more cooking in 'next']

* jc/fix-tree-walk (2009-09-14) 7 commits
  (merged to 'next' on 2010-01-13 at 1c01b87)
 + read-tree --debug-unpack
 + unpack-trees.c: look ahead in the index
 + unpack-trees.c: prepare for looking ahead in the index
 + Aggressive three-way merge: fix D/F case
 + traverse_trees(): handle D/F conflict case sanely
 + more D/F conflict tests
 + tests: move convenience regexp to match object names to test-lib.sh

Resurrected from "Ejected" category.  This is fix for a tricky codepath
and testing and improving before it hits 'master' is greatly appreciated.
(I have been using this in my private build for some time).

--------------------------------------------------
[Cooking]

* jh/notes (2010-01-17) 20 commits
 . builtin-gc: Teach the new --notes option to garbage-collect notes
 . Notes API: gc_notes(): Prune notes that belong to non-existing objects
 . t3305: Verify that removing notes triggers automatic fanout consolidation
 . builtin-notes: Teach -d option for deleting existing notes
 . Teach builtin-notes to remove empty notes
 . Teach notes code to properly preserve non-notes in the notes tree
 . t3305: Verify that adding many notes with git-notes triggers increased fanout
 . t3301: Verify successful annotation of non-commits
 . Builtin-ify git-notes
 . Refactor notes concatenation into a flexible interface for combining notes
 . Notes API: Allow multiple concurrent notes trees with new struct notes_tree
 . Notes API: write_notes_tree(): Store the notes tree in the database
 . Notes API: for_each_note(): Traverse the entire notes tree with a callback
 . Notes API: get_note(): Return the note annotating the given object
 . Notes API: remove_note(): Remove note objects from the notes tree structure
 . Notes API: add_note(): Add note objects to the internal notes tree structure
 . Notes API: init_notes(): Initialize the notes tree from the given notes ref
 . Add tests for checking correct handling of $GIT_NOTES_REF and core.notesRef
 . Notes API: get_commit_notes() -> format_note() + remove the commit restriction
 . Minor non-functional fixes to notes.c

Tentatively ejected, as its tests conflict with tests in a higher priority
fix.

* jh/gitweb-cached (2010-01-13) 9 commits
 - gitweb: File based caching layer (from git.kernel.org)
 - gitweb: Convert output to using indirect file handle
 - gitweb: cleanup error message produced by undefined $site_header
 - gitweb: add a get function to compliment print_sort_th
 - gitweb: add a get function to compliment print_local_time
 - gitweb: Makefile improvements
 - gitweb: Add option to force version match
 - gitweb: change die_error to take "extra" argument for extended die information
 - gitweb: Load checking

Replaced with a re-roll.  Update to t9500 is probably needed.

* jc/grep-author-all-match-implicit (2010-01-17) 1 commit
 - "log --author=me --grep=it" should find intersection, not union

Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:

> * jh/gitweb-cached (2010-01-13) 9 commits
>  - gitweb: File based caching layer (from git.kernel.org)
>  - gitweb: Convert output to using indirect file handle
>  - gitweb: cleanup error message produced by undefined $site_header
>  - gitweb: add a get function to compliment print_sort_th
>  - gitweb: add a get function to compliment print_local_time
>  - gitweb: Makefile improvements
>  - gitweb: Add option to force version match
>  - gitweb: change die_error to take "extra" argument for extended die information
>  - gitweb: Load checking
> 
> Replaced with a re-roll.  Update to t9500 is probably needed.

I have sent proof of concept replacements for the last two
patches... and those do include update to t9500 (although caching is
tested only rudimentally).

I don't know if J.H. is working on replacement patches; I am planning
on re-rolling cleaned up version of split file based caching layer on
top of his changes / his re-roll.

-- 
Jakub Narebski
Poland

Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)

[Jens Lehmann]

Am 23.01.2010 04:28, schrieb Junio C Hamano:
> * jl/submodule-diff (2010-01-18) 4 commits
>   (merged to 'next' on 2010-01-20 at 95cb513)
>  + Performance optimization for detection of modified submodules
>   (merged to 'next' on 2010-01-17 at 525075b)
>  + git status: Show uncommitted submodule changes too when enabled
>   (merged to 'next' on 2010-01-16 at 0a99e3c)
>  + Teach diff that modified submodule directory is dirty
>  + Show submodules as modified when they contain a dirty work tree

What about adding the following patch to this series?
Without it i see a performance regression for people who use
"git diff* --ignore-submodules".

A patch that teaches "git diff --submodule" to display if the submodule
contains new untracked and/or modified files is also almost ready. Would
you consider it for inclusion into 1.7.0 too or shall i wait until after
the release?

------8<-----
Subject: [PATCH] git diff: Don't test submodule dirtiness with --ignore-submodules

The diff family suppresses the output of submodule changes when
requested but checks them nonetheless. But since recently submodules
get examined for their dirtiness, which is rather expensive. There is
no need to do that when the --ignore-submodules option is used, as
the gathered information is never used anyway.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 diff-lib.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

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;

 		/* If CE_VALID is set, don't look at workdir for file removal */
@@ -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;
@@ -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;
@@ -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;
@@ -270,7 +275,7 @@ static void show_new_file(struct rev_info *revs,
 	 * the working copy.
 	 */
 	if (get_stat_data(new, &sha1, &mode, cached, match_missing,
-	    &dirty_submodule, revs->diffopt.output_format) < 0)
+	    &dirty_submodule, &revs->diffopt) < 0)
 		return;

 	diff_index_show_file(revs, "+", new, sha1, mode, dirty_submodule);
@@ -287,7 +292,7 @@ static int show_modified(struct rev_info *revs,
 	unsigned dirty_submodule = 0;

 	if (get_stat_data(new, &sha1, &mode, cached, match_missing,
-			  &dirty_submodule, revs->diffopt.output_format) < 0) {
+			  &dirty_submodule, &revs->diffopt) < 0) {
 		if (report_missing)
 			diff_index_show_file(revs, "-", old,
 					     old->sha1, old->ce_mode, 0);
-- 
1.6.6.1.558.g5c480.dirty

Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)

[Junio C Hamano]

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?


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

Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> ...
>> @@ -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.

I think we need to clarify the rule for ce_uptodate(ce).

The rule has always been that an entry that is ce_uptodate(ce) is _known_
to be clean, and nobody should have to dig deeper to double check.  We
should keep it that way.

We at least need to fix preload_index() not to mark any gitlink entries
with ce_mark_uptodate(ce), as your series changes the definition of a
dirty submodule.  It used to be that if a submodule is checked out and its
HEAD matches what is recorded in the index of the superproject, then the
submodule is clean.  The checks made in preload_thread() is consistent
with this definition.

With the update, we consider that having local changes in the submodule
(either to the index or to the work tree files) makes it modified (which
by the way is a right definition, and prevents people from forgetting to
commit in the submodule and updating the superproject index with the new
submodule commit, before commiting the state in the superproject).

Because the checks in preload_thread() does not cover this kind of change
that is_submodule_modified() reports, it shouldn't mark a gitlink entry as
ce_uptodate(ce).

Another possibility is to run the is_submodule_modified() check inside
ie_match_stat(), but (1) I don't know if that function is thread-safe, and
(2) I don't think we would want to do it in preload-index time (it is
rather expensive).

The reason preload_index() passes CE_MATCH_RACY_IS_DIRTY to ie_match_stat() 
is to avoid doing a rather expensive ce_modified_check_fs() --- it avoids
the overhead and leaves the expensive check to the true callers that care
if the entry is really clean.  In the same sense, even if we were to run
is_submodule_modified() there, we would want to avoid doing so when we are
running ie_match_stat() from preload codepath.

We need to also see if there other codepaths that call
ce_mark_uptodate(ce) on a gitlink that hasn't been checked with
is_submodule_modified(), and eliminate them.

Then we can fix the "even though ce_uptodate(ce) says this entry is clean,
if it is gitlink, we need to double check" insanity around ll 164 of
diff-lib.c.  We should be able to trust ce_uptodate(ce) even for gitlinks.

What do you think?

Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think we need to clarify the rule for ce_uptodate(ce).

A first step to do so may look like this.

-- >8 --
Subject: Make ce_uptodate() trustworthy again

The rule has always been that a cache entry that is ce_uptodate(ce)
means that we already have checked the work tree entity and we know
there is no change in the work tree compared to the index, and nobody
should have to double check.  Note that false ce_uptodate(ce) does not
mean it is known to be dirty---it only means we don't know if it is
clean.

There are a few codepaths (refresh-index and preload-index are among
them) that mark a cache entry as up-to-date based solely on the return
value from ie_match_stat(); this function uses lstat() to see if the
work tree entity has been touched, and for a submodule entry, if its
HEAD points at the same commit as the commit recorded in the index of
the superproject (a submodule that is not even cloned is considered
clean).

A submodule is no longer considered unmodified merely because its HEAD
matches the index of the superproject these days, in order to prevent
people from forgetting to commit in the submodule and updating the
superproject index with the new submodule commit, before commiting the
state in the superproject.  However, the patch to do so didn't update
the codepath that marks cache entries up-to-date based on the updated
definition and instead worked it around by saying "we don't trust the
return value of ce_uptodate() for submodules."

This makes ce_uptodate() trustworthy again by not marking submodule
entries up-to-date.

The next step _could_ be to introduce a few "in-core" flag bits to
cache_entry structure to record "this entry is _known_ to be dirty",
call is_submodule_modified() from ie_match_stat(), and use these new
bits to avoid running this rather expensive check more than once, but
that can be a separate patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-lib.c      |    2 +-
 preload-index.c |    2 ++
 read-cache.c    |    6 ++++--
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 23e180e..c6c425e 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -161,7 +161,7 @@ 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) || ce_skip_worktree(ce))
 			continue;
 
 		/* If CE_VALID is set, don't look at workdir for file removal */
diff --git a/preload-index.c b/preload-index.c
index 9289933..e3d0bda 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -47,6 +47,8 @@ static void *preload_thread(void *_data)
 
 		if (ce_stage(ce))
 			continue;
+		if (S_ISGITLINK(ce->ce_mode))
+			continue;
 		if (ce_uptodate(ce))
 			continue;
 		if (!ce_path_match(ce, p->pathspec))
diff --git a/read-cache.c b/read-cache.c
index 79938bf..309b77a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -612,7 +612,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 	if (alias && !ce_stage(alias) && !ie_match_stat(istate, alias, st, ce_option)) {
 		/* Nothing changed, really */
 		free(ce);
-		ce_mark_uptodate(alias);
+		if (!S_ISGITLINK(alias->ce_mode))
+			ce_mark_uptodate(alias);
 		alias->ce_flags |= CE_ADDED;
 		return 0;
 	}
@@ -1050,7 +1051,8 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 			 * because CE_UPTODATE flag is in-core only;
 			 * we are not going to write this change out.
 			 */
-			ce_mark_uptodate(ce);
+			if (!S_ISGITLINK(ce->ce_mode))
+				ce_mark_uptodate(ce);
 			return ce;
 		}
 	}

Re: What's cooking in git.git (Jan 2010, #07; Fri, 22)

[Jens Lehmann]

Am 23.01.2010 21:03, schrieb Junio C Hamano:
> 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.

Shawn applied the patch doing that yesterday, now gitk and git gui both
use "git diff --submodule" and not "git submodule summary" anymore. And
it would be really nice if both of these tools would show the dirtiness
of submodules in 1.7.0 (and after 1.7.0 i plan to teach "git submodule
summary" about dirtiness too).


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

Sure, I'm fine with having the full functionality in 1.7.1. But a part of
it looks like a fix to me, let me explain:

Right now "git diff --submodule" doesn't show the dirty status of a
submodule at all (like it does when using it without that option and
having paid the cost to get the necessary information). So IMHO something
like the patch below should go into 1.7.0 to fix that. When applied the
output looks like this:

Submodule sub 3f35670..3f35670-dirty:

which is now consistent with the output of "git diff" without that option:

diff --git a/sub b/sub
--- a/sub
+++ b/sub
@@ -1 +1 @@
-Subproject commit 3f356705649b5d566d97ff843cf193359229a453
+Subproject commit 3f356705649b5d566d97ff843cf193359229a453-dirty


This is a stripped down version of the patch i had in mind. I would post
the rest after 1.7.0 to give detailed output about the reasons a submodule
is dirty.

What do you think?


-- >8 --
Subject: Teach diff --submodule that modified submodule directory is dirty

Since commit 8e08b4 git diff does append "-dirty" to the work tree side
if the working directory of a submodule contains new or modified files.
Lets do the same when the --submodule option is used.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 diff.c                    |    2 +-
 submodule.c               |    3 ++
 submodule.h               |    1 +
 t/t4041-diff-submodule.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 1 deletions(-)

diff --git a/diff.c b/diff.c
index f130a36..381cc8d 100644
--- a/diff.c
+++ b/diff.c
@@ -1615,7 +1615,7 @@ static void builtin_diff(const char *name_a,
 		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
 		const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
 		show_submodule_summary(o->file, one ? one->path : two->path,
-				one->sha1, two->sha1,
+				one->sha1, two->sha1, two->dirty_submodule,
 				del, add, reset);
 		return;
 	}
diff --git a/submodule.c b/submodule.c
index f657bee..ca0527f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -36,6 +36,7 @@ static int add_submodule_odb(const char *path)

 void show_submodule_summary(FILE *f, const char *path,
 		unsigned char one[20], unsigned char two[20],
+		unsigned dirty_submodule,
 		const char *del, const char *add, const char *reset)
 {
 	struct rev_info rev;
@@ -85,6 +86,8 @@ void show_submodule_summary(FILE *f, const char *path,
 	if (!fast_backward && !fast_forward)
 		strbuf_addch(&sb, '.');
 	strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
+	if (dirty_submodule)
+		strbuf_add(&sb, "-dirty", 6);
 	if (message)
 		strbuf_addf(&sb, " %s\n", message);
 	else
diff --git a/submodule.h b/submodule.h
index 0773121..2336965 100644
--- a/submodule.h
+++ b/submodule.h
@@ -3,6 +3,7 @@

 void show_submodule_summary(FILE *f, const char *path,
 		unsigned char one[20], unsigned char two[20],
+		unsigned dirty_submodule,
 		const char *del, const char *add, const char *reset);
 int is_submodule_modified(const char *path);

diff --git a/t/t4041-diff-submodule.sh b/t/t4041-diff-submodule.sh
index 5bb4fed..4643054 100755
--- a/t/t4041-diff-submodule.sh
+++ b/t/t4041-diff-submodule.sh
@@ -191,6 +191,73 @@ EOF
 "

 commit_file sm1 &&
+test_expect_success 'submodule is up to date' "
+	git diff-index -p --submodule=log HEAD >actual &&
+	diff actual - <<-EOF
+EOF
+"
+
+test_expect_success 'submodule contains untracked content' "
+	echo new > sm1/new-file &&
+	git diff-index -p --submodule=log HEAD >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6..$head6-dirty:
+EOF
+"
+
+test_expect_success 'submodule contains untracked and modifed content' "
+	echo new > sm1/foo6 &&
+	git diff-index -p --submodule=log HEAD >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6..$head6-dirty:
+EOF
+"
+
+test_expect_success 'submodule contains modifed content' "
+	rm -f sm1/new-file &&
+	git diff-index -p --submodule=log HEAD >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6..$head6-dirty:
+EOF
+"
+
+(cd sm1; git commit -mchange foo6 >/dev/null) &&
+head8=$(cd sm1; git rev-parse --verify HEAD | cut -c1-7) &&
+test_expect_success 'submodule is modified' "
+	git diff-index -p --submodule=log HEAD >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6..$head8:
+  > change
+EOF
+"
+
+test_expect_success 'modified submodule contains untracked content' "
+	echo new > sm1/new-file &&
+	git diff-index -p --submodule=log HEAD >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6..$head8-dirty:
+  > change
+EOF
+"
+
+test_expect_success 'modified submodule contains untracked and modifed content' "
+	echo modification >> sm1/foo6 &&
+	git diff-index -p --submodule=log HEAD >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6..$head8-dirty:
+  > change
+EOF
+"
+
+test_expect_success 'modified submodule contains modifed content' "
+	rm -f sm1/new-file &&
+	git diff-index -p --submodule=log HEAD >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6..$head8-dirty:
+  > change
+EOF
+"
+
 rm -rf sm1
 test_expect_success 'deleted submodule' "
 	git diff-index -p --submodule=log HEAD >actual &&
-- 
1.6.6.1.559.g4914

[RFC/H] "git diff --submodule" showing submodule work tree dirtiness

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Right now "git diff --submodule" doesn't show the dirty status of a
> submodule at all (like it does when using it without that option and
> having paid the cost to get the necessary information). So IMHO something
> like the patch below should go into 1.7.0 to fix that. When applied the
> output looks like this:
>
> Submodule sub 3f35670..3f35670-dirty:
>
> which is now consistent with the output of "git diff" without that option:
>
> diff --git a/sub b/sub
> --- a/sub
> +++ b/sub
> @@ -1 +1 @@
> -Subproject commit 3f356705649b5d566d97ff843cf193359229a453
> +Subproject commit 3f356705649b5d566d97ff843cf193359229a453-dirty

I think this is sensible; I queued it to 'pu' to give chance to submodule
users comment on it, but I am inclined to say it should be part of 1.7.0
for consistency.