GIT 1.6.0-rc1

GIT 1.6.0-rc1

[Junio C Hamano]

Ok, so I tagged and pushed it out.  There still is the "git mv" bugfix
from Pasky that is parked in 'next' but other than that I think this is
pretty much "it" for 1.6.0 feature-wise.

Major parts of the changes since 1.6.0-rc0 consists of documentation and
tests portability; there is nothing surprising nor scary.

Test tarballs and RPMs are also there.  Please give it a good beating.

----------------------------------------------------------------

Changes since v1.6.0-rc0 are as follows:

Abhijit Menon-Sen (2):
      git-gui: Look for gitk in $PATH, not $LIBEXEC/git-core
      Clarify that "git log x.c y.h" lists commits that touch either file

Alex Riesen (1):
      Allow pager of diff command be enabled/disabled

Alexander Gavrilov (4):
      Fix pre-commit hooks under MinGW/MSYS
      Add options to control the search for copies in blame.
      Kill the blame back-end on window close.
      Add a menu item to invoke full copy detection in blame.

Anders Melchiorsen (1):
      Documentation: fix diff.external example

Björn Steinbrink (2):
      index-pack.c: correctly initialize appended objects
      rev-parse: Add support for the ^! and ^@ syntax

Brad King (1):
      git-svn: teach dcommit about svn auto-props

Brandon Casey (7):
      t/: Replace diff [-u|-U0] with test_cmp to allow compilation with old diff
      t4116-apply-reverse.sh: use $TAR rather than tar
      t3200,t7201: replace '!' with test_must_fail
      t7502-commit.sh: rearrange test to make more portable
      t/t4202-log.sh: add newline at end of file
      Teach fsck and prune about the new location of temporary objects
      perl/Makefile: update NO_PERL_MAKEMAKER section

Cesar Eduardo Barros (1):
      Documentation/git-submodule.txt: fix doubled word

Daniel Barkalow (1):
      In perforce, RCS keywords are case-sensitive

Jakub Narebski (1):
      gitweb: More about how gitweb gets 'owner' of repository

Johannes Schindelin (9):
      Rename .git/rebase to .git/rebase-apply
      Rename path_list to string_list
      Fix two leftovers from path_list->string_list
      Ignore dirty submodule states in "git pull --rebase"
      Add test to show that show-branch misses out the 8th column
      sort_in_topological_order(): avoid setting a commit flag
      builtin-commit: Two trivial style-cleanups
      git daemon: avoid waking up too often
      Avoid chdir() in list_commands_in_dir()

Johannes Sixt (12):
      rebase -i: When an 'edit' stops, mention the commit
      Makefile: Do not install a copy of 'git' in $(gitexecdir)
      Makefile: Normalize $(bindir) and $(gitexecdir) before comparing
      Record the command invocation path early
      Fix relative built-in paths to be relative to the command invocation
      Allow the built-in exec path to be relative to the command invocation path
      Allow add_path() to add non-existent directories to the path
      Windows: Make $(gitexecdir) relative
      Windows: Make sure argv[0] has a path
      Windows: Do not compile git-shell
      git-gui: Fix "Stage/Unstage Line" with one line of context.
      git-gui: "Stage Line": Treat independent changes in adjacent lines better

Jonathan Nieder (3):
      git-diff(1): "--c" -> "--cc" typo fix
      document that git-tag can tag more than heads
      t6030 (bisect): work around Mac OS X "ls"

Junio C Hamano (13):
      Update my e-mail address
      Revert "make git-status use a pager"
      tests: do not rely on external "patch"
      stash save: fix parameter handling
      builtin-branch.c: remove unused code in append_ref() callback function
      builtin-branch.c: optimize --merged and --no-merged
      Documentation: clarify diff --cc
      ignore non-existent refs in dwim_log()
      tests: propagate $(TAR) down from the toplevel Makefile
      Makefile: fix shell quoting
      Documentation: clarify how to disable elements in core.whitespace
      make sure parsed wildcard refspec ends with slash
      GIT 1.6.0-rc1

Lars Hjemli (3):
      builtin-branch: remove duplicated code
      builtin-branch: factor out merge_filter matching
      builtin-branch: fix -v for --[no-]merged

Lee Marlow (2):
      bash completion: Add long options for 'git rm'
      bash completion: Add completion for 'git help'

Miklos Vajna (2):
      builtin-merge: give a proper error message for invalid strategies in config
      t7601: extend the 'merge picks up the best result' test

Nikolaj Schumacher (1):
      Don't cut off last character of commit descriptions.

Olivier Marin (4):
      git-am: remove dash from help message
      parse-options: fix segmentation fault when a required value is missing
      git am --skip: clean the index while preserving local changes
      update test case to protect am --skip behaviour

P. Christeas (1):
      svnimport: newer libsvn wants us to ask for the root with "", not "/"

Peter Valdemar Mørch (1):
      send-email: find body-encoding correctly

Petr Baudis (4):
      git-filter-branch.sh: Allow running in bare repositories
      Documentation/git-filter-branch: teach "rm" instead of "update-index --remove"
      git-mv: Remove dead code branch
      git-mv: Keep moved index entries inact

Philippe Bruhat (1):
      mailinfo: better parse email adresses containg parentheses

Pierre Habouzit (4):
      builtin-merge: add missing structure initialization
      git-submodule: move ill placed shift.
      git-checkout: fix command line parsing.
      git-checkout: improve error messages, detect ambiguities.

René Scharfe (5):
      archive: add write_archive()
      archive: move parameter parsing code to archive.c
      archive: define MAX_ARGS where it's needed
      archive: declare struct archiver where it's needed
      archive: allow --exec and --remote without equal sign

SZEDER Gábor (2):
      checkout: mention '--' in the docs
      bash: offer only paths after '--' for 'git checkout'

Shawn O. Pearce (2):
      git-gui: Correct 'Visualize Branches' on Mac OS X to start gitk
      fsck: Don't require tmp_obj_ file names are 14 bytes in length

Stephan Beyer (7):
      git-am: Add colon before the subject that is printed out as being applied
      am --abort: Add to bash-completion and mention in git-rerere documentation
      Make non-static functions, that may be static, static
      Move launch_editor() from builtin-tag.c to editor.c
      editor.c: Libify launch_editor()
      git-am: Mention --abort in usage string part of OPTIONS_SPEC
      git-reset: Let -q hush "locally modified" messages

Steve Haslam (2):
      Propagate -u/--upload-pack option of "git clone" to transport.
      Remove references to git-fetch-pack from "git clone" documentation.

Thomas Rast (2):
      git-completion.bash: provide completion for 'show-branch'
      bash completion: Add long options for 'git describe'

Re: GIT 1.6.0-rc1

[Petr Baudis]

  Hi,

On Sun, Jul 27, 2008 at 05:09:38PM -0700, Junio C Hamano wrote:
> Ok, so I tagged and pushed it out.  There still is the "git mv" bugfix
> from Pasky that is parked in 'next' but other than that I think this is
> pretty much "it" for 1.6.0 feature-wise.

  BTW, it seems that in the end, the bugfix made it to -rc1's master.
(Which is cool. :-)

				Petr "Pasky" Baudis

Re: GIT 1.6.0-rc1

[Alex Riesen]

Junio C Hamano, Mon, Jul 28, 2008 02:09:38 +0200:
> Ok, so I tagged and pushed it out.  There still is the "git mv" bugfix
> from Pasky that is parked in 'next' but other than that I think this is
> pretty much "it" for 1.6.0 feature-wise.
> 
> Major parts of the changes since 1.6.0-rc0 consists of documentation and
> tests portability; there is nothing surprising nor scary.

t2103-update-index-ignore-missing.sh still broken on Windows because
of stat.st_size being 0 for directories there.

    http://thread.gmane.org/gmane.comp.version-control.git/89370

Re: GIT 1.6.0-rc1

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> t2103-update-index-ignore-missing.sh still broken on Windows because
> of stat.st_size being 0 for directories there.

I recall we did not reach a useful conclusion of that discussion.

Re: GIT 1.6.0-rc1

[Alex Riesen]

Junio C Hamano, Mon, Jul 28, 2008 08:44:52 +0200:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > t2103-update-index-ignore-missing.sh still broken on Windows because
> > of stat.st_size being 0 for directories there.
> 
> I recall we did not reach a useful conclusion of that discussion.

Why isn't the proposed patch useful? (and would it be possible to make
the answer out of plain, small and short words?)

Re: GIT 1.6.0-rc1

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Mon, Jul 28, 2008 08:44:52 +0200:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>> 
>> > t2103-update-index-ignore-missing.sh still broken on Windows because
>> > of stat.st_size being 0 for directories there.
>> 
>> I recall we did not reach a useful conclusion of that discussion.
>
> Why isn't the proposed patch useful? (and would it be possible to make
> the answer out of plain, small and short words?)

Can you answer out of plain, small and short words why the proposed patch
is correct without unwanted side effects, not just "this seems to solve
the particular issue for me but I don't know if it has unintended side
effects"?

Re: GIT 1.6.0-rc1

[Junio C Hamano]

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

> Alex Riesen <raa.lkml@gmail.com> writes:
>
>> Junio C Hamano, Mon, Jul 28, 2008 08:44:52 +0200:
>>> Alex Riesen <raa.lkml@gmail.com> writes:
>>> 
>>> > t2103-update-index-ignore-missing.sh still broken on Windows because
>>> > of stat.st_size being 0 for directories there.
>>> 
>>> I recall we did not reach a useful conclusion of that discussion.
>>
>> Why isn't the proposed patch useful? (and would it be possible to make
>> the answer out of plain, small and short words?)
>
> Can you answer out of plain, small and short words why the proposed patch
> is correct without unwanted side effects, not just "this seems to solve
> the particular issue for me but I don't know if it has unintended side
> effects"?

Ok, I took a deeper look at the codepaths involved.  Although it does work
around the issue, I do not think your patch alone is the "correct" one in
the longer term.

It needs a bit of explanation, and the explanation won't be exactly
"plain, small and short", unfortunately.

-- >8 --
[PATCH] Teach gitlinks to ie_modified() and ce_modified_check_fs()

The ie_modified() function is the workhorse for refresh_cache_entry(),
i.e. checking if an index entry that is stat-dirty actually has changes.

After running quicker check to compare cached stat information with
results from the latest lstat(2) to answer "has modification" early, the
code goes on to check if there really is a change by comparing the staged
data with what is on the filesystem by asking ce_modified_check_fs().
However, this function always said "no change" for any gitlinks that has a
directory at the corresponding path.  This made ie_modified() to miss
actual changes in the subproject.

The patch fixes this first by modifying an existing short-circuit logic
before calling the ce_modified_check_fs() function.  It knows that for any
filesystem entity to which ie_match_stat() says its data has changed, if
its cached size is nonzero then the contents cannot match, which is a
correct optimization only for blob objects.  We teach gitlink objects to
this special case, as we already know that any gitlink that
ie_match_stat() says is modified is indeed modified at this point in the
codepath.

With the above change, we could leave ce_modified_check_fs() broken, but
it also futureproofs the code by teaching it to use ce_compare_gitlink(),
instead of assuming (incorrectly) that any directory is unchanged.

Originally noticed by Alex Riesen on Cygwin.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c |   27 ++++++++++++++++++++++-----
 1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index b5916b0..a940f8d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -152,7 +152,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
 		break;
 	case S_IFDIR:
 		if (S_ISGITLINK(ce->ce_mode))
-			return 0;
+			return ce_compare_gitlink(ce) ? DATA_CHANGED : 0;
 	default:
 		return TYPE_CHANGED;
 	}
@@ -192,6 +192,7 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 			changed |= TYPE_CHANGED;
 		break;
 	case S_IFGITLINK:
+		/* We ignore most of the st_xxx fields for gitlinks */
 		if (!S_ISDIR(st->st_mode))
 			changed |= TYPE_CHANGED;
 		else if (ce_compare_gitlink(ce))
@@ -298,11 +299,22 @@ int ie_modified(const struct index_state *istate,
 	if (changed & (MODE_CHANGED | TYPE_CHANGED))
 		return changed;
 
-	/* Immediately after read-tree or update-index --cacheinfo,
-	 * the length field is zero.  For other cases the ce_size
-	 * should match the SHA1 recorded in the index entry.
+	/*
+	 * Immediately after read-tree or update-index --cacheinfo,
+	 * the length field is zero, as we have never even read the
+	 * lstat(2) information once, and we cannot trust DATA_CHANGED
+	 * returned by ie_match_stat() which in turn was returned by
+	 * ce_match_stat_basic() to signal that the filesize of the
+	 * blob changed.  We have to actually go to the filesystem to
+	 * see if the contents match, and if so, should answer "unchanged".
+	 *
+	 * The logic does not apply to gitlinks, as ce_match_stat_basic()
+	 * already has checked the actual HEAD from the filesystem in the
+	 * subproject.  If ie_match_stat() already said it is different,
+	 * then we know it is.
 	 */
-	if ((changed & DATA_CHANGED) && ce->ce_size != 0)
+	if ((changed & DATA_CHANGED) &&
+	    (S_ISGITLINK(ce->ce_mode) || ce->ce_size != 0))
 		return changed;
 
 	changed_fs = ce_modified_check_fs(ce, st);
@@ -1331,6 +1343,11 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
 	 * falsely clean entry due to touch-update-touch race, so we leave
 	 * everything else as they are.  We are called for entries whose
 	 * ce_mtime match the index file mtime.
+	 *
+	 * Note that this actually does not do much for gitlinks, for
+	 * which ce_match_stat_basic() always goes to the actual
+	 * contents.  The caller checks with is_racy_timestamp() which
+	 * always says "no" for gitlinks, so we are not called for them ;-)
 	 */
 	struct stat st;
 

Re: GIT 1.6.0-rc1

[Junio C Hamano]

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

>>> Junio C Hamano, Mon, Jul 28, 2008 08:44:52 +0200:
>>>> Alex Riesen <raa.lkml@gmail.com> writes:
>>>> 
>>>> > t2103-update-index-ignore-missing.sh still broken on Windows because
>>>> > of stat.st_size being 0 for directories there.
>>>> 
>>>> I recall we did not reach a useful conclusion of that discussion.
>>>
>>> Why isn't the proposed patch useful? (and would it be possible to make
>>> the answer out of plain, small and short words?)
>>
>> Can you answer out of plain, small and short words why the proposed patch
>> is correct without unwanted side effects, not just "this seems to solve
>> the particular issue for me but I don't know if it has unintended side
>> effects"?
>
> Ok, I took a deeper look at the codepaths involved.  Although it does work
> around the issue, I do not think your patch alone is the "correct" one in
> the longer term.
>
> It needs a bit of explanation, and the explanation won't be exactly
> "plain, small and short", unfortunately.

Alex, I ran the full test with this, but only on Linux boxes; obviously
not on any flavor of Windows.  I think it is correct, and the "first line
of defence" fix is the same as your patch, so I'd assume it would work for
you as well.  But extra eyeballs are always appreciated.

Re: GIT 1.6.0-rc1

[Alex Riesen]

Junio C Hamano, Tue, Jul 29, 2008 10:36:19 +0200:
> Junio C Hamano <gitster@pobox.com> writes:
> >
> > Ok, I took a deeper look at the codepaths involved.  Although it does work
> > around the issue, I do not think your patch alone is the "correct" one in
> > the longer term.

Thought so. I just didn't know the code around

> > It needs a bit of explanation, and the explanation won't be exactly
> > "plain, small and short", unfortunately.
> 
> Alex, I ran the full test with this, but only on Linux boxes; obviously
> not on any flavor of Windows.  I think it is correct, and the "first line
> of defence" fix is the same as your patch, so I'd assume it would work for
> you as well.  But extra eyeballs are always appreciated.

Well, it works on Cygwin too. And I had my eyeballs on the code
(wondered first if it will cause more fs accesses than before: it
will, in the racy check. Which is correct, AFAICT)

Re: GIT 1.6.0-rc1

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Tue, Jul 29, 2008 10:36:19 +0200:
>> Junio C Hamano <gitster@pobox.com> writes:
>> >
>> > Ok, I took a deeper look at the codepaths involved.  Although it does work
>> > around the issue, I do not think your patch alone is the "correct" one in
>> > the longer term.
>
> Thought so. I just didn't know the code around
>
>> > It needs a bit of explanation, and the explanation won't be exactly
>> > "plain, small and short", unfortunately.
>> 
>> Alex, I ran the full test with this, but only on Linux boxes; obviously
>> not on any flavor of Windows.  I think it is correct, and the "first line
>> of defence" fix is the same as your patch, so I'd assume it would work for
>> you as well.  But extra eyeballs are always appreciated.
>
> Well, it works on Cygwin too. And I had my eyeballs on the code
> (wondered first if it will cause more fs accesses than before: it
> will, in the racy check. Which is correct, AFAICT)

I thought racy check won't even trigger for gitlinks, no?

ce_modified_check_fs() has 3 call sites:

 - the call site in ie_match_stat() is protected with is_racy_timestamp()
   that is always false for gitlinks;

 - the call site in ie_modified() we just took care of in the current
   thread;

 - the other call site is in ce_smudge_racily_clean_entry(), which is
   called from write_index() but it also is protected with
   is_racy_timestamp() that is always false for gitlinks.

 

Re: GIT 1.6.0-rc1

[Alex Riesen]

Junio C Hamano, Wed, Jul 30, 2008 00:03:44 +0200:
> Alex Riesen <raa.lkml@gmail.com> writes:
> >> Alex, I ran the full test with this, but only on Linux boxes; obviously
> >> not on any flavor of Windows.  I think it is correct, and the "first line
> >> of defence" fix is the same as your patch, so I'd assume it would work for
> >> you as well.  But extra eyeballs are always appreciated.
> >
> > Well, it works on Cygwin too. And I had my eyeballs on the code
> > (wondered first if it will cause more fs accesses than before: it
> > will, in the racy check. Which is correct, AFAICT)
> 
> I thought racy check won't even trigger for gitlinks, no?
> 

I didn't know. But now, come to think of it, there wouldn't be much
point - the gitlinks are always compared by content, aren't they?

> ce_modified_check_fs() has 3 call sites:
> 
>  - the call site in ie_match_stat() is protected with is_racy_timestamp()
>    that is always false for gitlinks;
> 
>  - the call site in ie_modified() we just took care of in the current
>    thread;
> 
>  - the other call site is in ce_smudge_racily_clean_entry(), which is
>    called from write_index() but it also is protected with
>    is_racy_timestamp() that is always false for gitlinks.

So, the change in ce_modified_check_fs is not really needed, because
the gitlink case is never executed?

Re: GIT 1.6.0-rc1

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Wed, Jul 30, 2008 00:03:44 +0200:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>> >> Alex, I ran the full test with this, but only on Linux boxes; obviously
>> >> not on any flavor of Windows.  I think it is correct, and the "first line
>> >> of defence" fix is the same as your patch, so I'd assume it would work for
>> >> you as well.  But extra eyeballs are always appreciated.
>> >
>> > Well, it works on Cygwin too. And I had my eyeballs on the code
>> > (wondered first if it will cause more fs accesses than before: it
>> > will, in the racy check. Which is correct, AFAICT)
>> 
>> I thought racy check won't even trigger for gitlinks, no?
>> 
>
> I didn't know. But now, come to think of it, there wouldn't be much
> point - the gitlinks are always compared by content, aren't they?
>
>> ce_modified_check_fs() has 3 call sites:
>> 
>>  - the call site in ie_match_stat() is protected with is_racy_timestamp()
>>    that is always false for gitlinks;
>> 
>>  - the call site in ie_modified() we just took care of in the current
>>    thread;
>> 
>>  - the other call site is in ce_smudge_racily_clean_entry(), which is
>>    called from write_index() but it also is protected with
>>    is_racy_timestamp() that is always false for gitlinks.
>
> So, the change in ce_modified_check_fs is not really needed, because
> the gitlink case is never executed?

That's what I meant by "futureproofing".