What's in git.git, and announcing GIT 1.4.2-rc4

What's in git.git, and announcing GIT 1.4.2-rc4

[Junio C Hamano]

GIT 1.4.2-rc4

It's been a week since -rc3, so here it is.  The changes are
really small fixes and nothing else.  Let's hope I can tag the
real 1.4.2 this weekend.

* The 'master' branch has these since the last announcement;
  these are all in 1.4.2-rc4:

   Jeff King:
      git-push: allow pushing from subdirectories

   Johannes Schindelin:
      Fix crash when GIT_DIR is invalid

   Jonas Fonseca:
      Update git-init-db(1) and documentation of core.sharedRepository

   Junio C Hamano:
      Cygwin needs NO_C99_FORMAT???
      Makefile: Cygwin does not seem to need NO_STRLCPY
      Fix "grep -w"
      debugging: XMALLOC_POISON
      builtin-mv: fix use of uninitialized memory.
      GIT-VERSION-GEN: adjust for ancient git
      Documentation: git-status takes the same options as git-commit
      Fix tutorial-2.html
      check return value from diff_setup_done()
      find_unique_abbrev() with len=0 should not abbreviate
      make --find-copies-harder imply -C
      allow diff.renamelimit to be set regardless of -M/-C

   Michael Krelin:
      handle https:// protocol in git-clone

   Ramsay Jones:
      Allow config file to specify Signed-off-by identity in format-patch.
      commit walkers: setup_ident() to record correct committer in ref-log.

   Ryan Anderson:
      log-tree: show_log() should respect the setting of diffopt->line_termination
      annotate: Fix bug when parsing merges with differing real and logical parents.


* The 'next' branch, in addition, has these.

  = To graduate immediately after 1.4.2 happens:

  - Jakub Narebski's autoconf stuff acquired a bit more
    clean-ups and new detections since the last announcement.

  - A new merge strategy, merge-recur, which is a rewrite of
    merge-recursive in C, by Johannes and Alex.

  - More commands are made built-in by Matthias Kestenholz, and
    I cleaned up the build procedure for built-ins a bit.

  - Matthias Lederhofer introduced $GIT_PAGER environment
    variable that can specify a different pager from $PAGER.

  - Ramsay Jones has one header fix to add _GNU_SOURCE, which
    helps things to compile in his environment.  This was
    confirmed to fix a similar problem on an ancient version of
    one distribution.

  - Timo Hirvonen made the parameter parsing of diff family
    saner some time ago.  Remaining two minor changes will
    graduate to "master" after 1.4.2:

    * --name-only, --name-status, --check and -s are mutually exclusive

    * Remove awkward compatibility warts "-s".  Now -s means "do
      not output diff" everywhere, including git-diff-files.

  - Johannes made http-push avoid fork() by calling
    merge_bases() directly.

  - MAX_NEEDS and MAX_HAS limitation in upload-pack has been
    lifted.

  - pack-objects can copy a non-delta representation of a object
    with the new style header straight into packs.

  - Paul Mackerras has a few gitk updates.


  * Hopefully not too long after 1.4.2:

  - A big gitweb clean-up series by Jakub Narebski, with help
    from Jeff King, Matthias Lederhofer and Martin Waitz to make
    run-time and build-time configuration easier.

    Quite a lot of clean-ups and enhancements by Jakub and Luben
    Tuikov are queued, and with the proposed function renames,
    it may be stable enough to start seriously testing soon
    after 1.4.2 happens.

  - New style loose objects, which use the same header format as
    in-pack objects, can be copied straight into packs when not
    deltified.  I am hoping that we can make the new-style loose
    objects the default in 10 to 12 weeks to give everybody time
    to update to 1.4 series.

  = Graduation schedule unknown:

  - Not-universally-liked Git.pm by Pasky with help from Dennis
    Stosberg, Johannes, Pavel Roskin and others.  One drawback
    is this pretty much makes Perl scripts that use Git.pm
    unusable with ActiveState right now.  No changes since the
    last announcement.

  - Linus worries that Racy-git avoidance code leaves racily-clean
    index entries forever and hurts performance, and I did some
    tweaks.  First we need to verify performance is actually
    harmed and by how much to see if this is needed.


* The 'pu' branch, in addition, has these.

  - An update to upload-pack to prevent it from going all the
    way back when the downloader has more roots than it.  Needs
    testing and comments.

  - Johannes has a new diff option --color-words to use color to
    squash word differences into single line output.

    I do not feel much need for this stuff, and the change is
    rather intrusive, so I am tempted to drop it.

  - A new merge strategy, merge-rename, which is still a
    work-in-progress to handle renames in read-tree 3-way
    merge.  Judging from the way Johannes's merge-recur is
    making progress, I may want to drop this.

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Willy Tarreau]

Hi Junio,

On Wed, Aug 09, 2006 at 06:29:30PM -0700, Junio C Hamano wrote:
> GIT 1.4.2-rc4
> 
> It's been a week since -rc3, so here it is.  The changes are
> really small fixes and nothing else.  Let's hope I can tag the
> real 1.4.2 this weekend.

I encountered a problem in 1.4.1 and 1.4-git about 2 weeks ago
(I've not tried 1.4.2-rc4 yet). When applying a git patch which
contains a symlink, the symlink created on the filesystem sometimes
has a wrong name with some chars appended to its end, such as if its
name was not zero-terminated. I always encountered this only when
there were several symlinks in the patch, eg: you create a symlink
first, then commit, then change its dest, then commit, then git-format-patch
and try to apply both patches (git-am -k -3) to another tree. Generally,
the second link will be broken. The index will be OK however, since git-status
will complain about it.

I've tried to find the reason for this problem in the code, but
failed to do so. I remember that the function which calls symlink()
(I don't remember its name, sorry) already gets an invalid name in
path->name or something like this.

That's all I could do because I was really too short in time, and I'm
sorry I really don't have enough time to track this further right now.
However, if you have some time to reproduce it and have questions, I'm
willing to respond you.

Thanks, and sorry again for the lack of information,
Willy

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Junio C Hamano]

Willy Tarreau <w@1wt.eu> writes:

> I encountered a problem in 1.4.1 and 1.4-git about 2 weeks ago
> (I've not tried 1.4.2-rc4 yet). When applying a git patch which
> contains a symlink, the symlink created on the filesystem sometimes
> has a wrong name with some chars appended to its end.

Thanks.  I can reproduce this, and am looking into it.

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Willy Tarreau]

On Wed, Aug 09, 2006 at 10:43:57PM -0700, Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
> 
> > Willy Tarreau <w@1wt.eu> writes:
> >
> >> I encountered a problem in 1.4.1 and 1.4-git about 2 weeks ago
> >> (I've not tried 1.4.2-rc4 yet). When applying a git patch which
> >> contains a symlink, the symlink created on the filesystem sometimes
> >> has a wrong name with some chars appended to its end.
> >
> > Thanks.  I can reproduce this, and am looking into it.
> 
> Found it.  The patch application mechanism uses a counted string
> (char *buf with ulong size) to hold the result, and the code
> stupidly threw the buf to symlink(2), without making it NUL
> terminated.

Excellent ! That means that under some circumstances, it might even have
corrupted the very first link, and not necessarily only the second and
other ones. Thanks for having fixed it that fast.

Cheers,
Willy

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Willy Tarreau <w@1wt.eu> writes:
>
>> I encountered a problem in 1.4.1 and 1.4-git about 2 weeks ago
>> (I've not tried 1.4.2-rc4 yet). When applying a git patch which
>> contains a symlink, the symlink created on the filesystem sometimes
>> has a wrong name with some chars appended to its end.
>
> Thanks.  I can reproduce this, and am looking into it.

Found it.  The patch application mechanism uses a counted string
(char *buf with ulong size) to hold the result, and the code
stupidly threw the buf to symlink(2), without making it NUL
terminated.


diff --git a/builtin-apply.c b/builtin-apply.c
index f8c6763..c159873 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1698,6 +1698,14 @@ static int apply_data(struct patch *patc
 	desc.buffer = buf;
 	if (apply_fragments(&desc, patch) < 0)
 		return -1;
+
+	/* NUL terminate the result */
+	if (desc.alloc <= desc.size) {
+		desc.buffer = xrealloc(desc.buffer, desc.size + 1);
+		desc.alloc++;
+	}
+	desc.buffer[desc.size] = 0;
+
 	patch->result = desc.buffer;
 	patch->resultsize = desc.size;
 
@@ -2040,6 +2048,9 @@ static int try_create_file(const char *p
 	int fd;
 
 	if (S_ISLNK(mode))
+		/* Although buf:size is counted string, it also is NUL
+		 * terminated.
+		 */
 		return symlink(buf, path);
 	fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 0666);
 	if (fd < 0)

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Johannes Schindelin]

Hi,

On Wed, 9 Aug 2006, Junio C Hamano wrote:

> +
> +	/* NUL terminate the result */
> +	if (desc.alloc <= desc.size) {
> +		desc.buffer = xrealloc(desc.buffer, desc.size + 1);
> +		desc.alloc++;

While in this particular case, desc.alloc cannot be smaller than desc.size 
(and therefore the condition in the if() is misleading), it might be 
better to write "desc.alloc = desc.size + 1;" for others to understand...

Ciao,
Dscho

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> While in this particular case, desc.alloc cannot be smaller than desc.size 
> (and therefore the condition in the if() is misleading), it might be 
> better to write "desc.alloc = desc.size + 1;" for others to understand...

We overallocate desc.alloc in apply_one_fragment for growth, so
that if() is needed, but after this part of the code we discard
the desc hence the increment is not needed ;-).

diff --git a/builtin-apply.c b/builtin-apply.c
index c159873..be2c715 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1700,10 +1700,8 @@ static int apply_data(struct patch *patc
 		return -1;
 
 	/* NUL terminate the result */
-	if (desc.alloc <= desc.size) {
+	if (desc.alloc <= desc.size)
 		desc.buffer = xrealloc(desc.buffer, desc.size + 1);
-		desc.alloc++;
-	}
 	desc.buffer[desc.size] = 0;
 
 	patch->result = desc.buffer;

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Johannes Schindelin]

Hi,

On Wed, 9 Aug 2006, Junio C Hamano wrote:

> * The 'next' branch, in addition, has these.
>
>   = To graduate immediately after 1.4.2 happens:
>
> [...]
>
>   - pack-objects can copy a non-delta representation of a object
>     with the new style header straight into packs.
>
> [...]
>
>   * Hopefully not too long after 1.4.2:
> 
> [...]
> 
>   - New style loose objects, which use the same header format as
>     in-pack objects, can be copied straight into packs when not
>     deltified.  I am hoping that we can make the new-style loose
>     objects the default in 10 to 12 weeks to give everybody time
>     to update to 1.4 series.

These are the same, no?

> * The 'pu' branch, in addition, has these.
>
> [...]
> 
>   - Johannes has a new diff option --color-words to use color to
>     squash word differences into single line output.
> 
>     I do not feel much need for this stuff, and the change is
>     rather intrusive, so I am tempted to drop it.

I beg to differ on the "intrusive": except for needed structs and 
functions, which are totally orthogonal with the rest of git, it only 
touches fn_out_consume(), builtin_diff() and in an obvious way, 
diff_opt_parse() and struct diff_options.

So the real impact is pretty low and well contained.

Besides, I really use it often -- you should try it! Call me blind, but 
very often I cannot spot the differences (in the unified diff) when they 
are minor, especially when there was just a typo in the documentation. You 
could now say that I should not care about it, then, but if _I_ made the 
mistake, I want to learn from it.

Of course, if you really hate what it does, I will happily carry it in my 
personal repository; I _need_ it.

Ciao,
Dscho

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>>   - pack-objects can copy a non-delta representation of a object
>>     with the new style header straight into packs.
>>...
>>   * Hopefully not too long after 1.4.2:
>>   - New style loose objects, which use the same header format as
>>     in-pack objects, can be copied straight into packs when not
>>     deltified.  I am hoping that we can make the new-style loose
>>     objects the default in 10 to 12 weeks to give everybody time
>>     to update to 1.4 series.
>
> These are the same, no?

Yeah; I meant to say that we need to wait a while before making
the new style loose object the default, since people with older
git would not be able to use them.

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Besides, I really use it often -- you should try it! Call me blind, but 
> very often I cannot spot the differences (in the unified diff) when they 
> are minor, especially when there was just a typo in the documentation. You 
> could now say that I should not care about it, then, but if _I_ made the 
> mistake, I want to learn from it.

I tried it on this:

diff --git a/builtin-apply.c b/builtin-apply.c
index c159873..be2c715 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1700,10 +1700,8 @@ static int apply_data(struct patch *patc
 		return -1;
 
 	/* NUL terminate the result */
-	if (desc.alloc <= desc.size) {
+	if (desc.alloc <= desc.size)
 		desc.buffer = xrealloc(desc.buffer, desc.size + 1);
-		desc.alloc++;
-	}
 	desc.buffer[desc.size] = 0;
 
 	patch->result = desc.buffer;

which shows something like:

diff --git a/builtin-apply.c b/builtin-apply.c
index c159873..be2c715 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1700,10 +1700,8 @@ static int apply_data(struct patch *patc
                return -1;

        /* NUL terminate the result */
        if (desc.alloc <= desc.size)
{
                desc.buffer = xrealloc(desc.buffer, desc.size + 1);
                desc.alloc++;
        }
        desc.buffer[desc.size] = 0;

        patch->result = desc.buffer;

where "desc.alloc++;" and next lines and the opening brace after
if() are red.  Why does that red opening brace have to come at
the beginning of line, I wonder...

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Johannes Schindelin]

Hi,

On Thu, 10 Aug 2006, Junio C Hamano wrote:

> I tried it [--color-words] on this:
> 
> diff --git a/builtin-apply.c b/builtin-apply.c
> index c159873..be2c715 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1700,10 +1700,8 @@ static int apply_data(struct patch *patc
>  		return -1;
>  
>  	/* NUL terminate the result */
> -	if (desc.alloc <= desc.size) {
> +	if (desc.alloc <= desc.size)
>  		desc.buffer = xrealloc(desc.buffer, desc.size + 1);
> -		desc.alloc++;
> -	}
>  	desc.buffer[desc.size] = 0;
>  
>  	patch->result = desc.buffer;
> 
> which shows something like:
> 
> diff --git a/builtin-apply.c b/builtin-apply.c
> index c159873..be2c715 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1700,10 +1700,8 @@ static int apply_data(struct patch *patc
>                 return -1;
> 
>         /* NUL terminate the result */
>         if (desc.alloc <= desc.size)
> {
>                 desc.buffer = xrealloc(desc.buffer, desc.size + 1);
>                 desc.alloc++;
>         }
>         desc.buffer[desc.size] = 0;
> 
>         patch->result = desc.buffer;
> 
> where "desc.alloc++;" and next lines and the opening brace after
> if() are red.  Why does that red opening brace have to come at
> the beginning of line, I wonder...

It is an implementation detail: to determine the differing words, I turn 
all whitespace into newlines in that particular hunk, and then run another 
diff (nested diff run!). So, the space in --- and the newline in +++ 
compare equal. Since wI am mostly interested in the _new_ (gree) version, 
the newline is printed, not the space.

Hmmm. I have to think about it.

Ciao,
Dscho

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Alex Riesen]

On 8/10/06, Junio C Hamano <junkio@cox.net> wrote:
>   - A new merge strategy, merge-recur, which is a rewrite of
>     merge-recursive in C, by Johannes and Alex.

It still has problems devouring that monster merge I have (~20k entries,
with around 40 changed in the other branch, around 100 revs ago. Big
binary files involved. Renames and copies).
Perfomance is nowhere near usable: ~20min on Windows/3GHz/2Gb,
~4Min on Linux/1.4GHz/384Mb ;) I agree that Linux is much more bearable
but 4Min is still too much (especially comparing to that "stupid" resolve).

I noticed that it spends a lot of time finding renames (diffcore_std,
in particular).
Why doesn't it take that much time for "diff-tree -M -r base head1" +
"diff-tree -M -r base head2", I wonder...? (~30 sek, for that windows box).

Sorry, I can't provide the tree. I suppose Mozilla tree can be compared
to that thing, when it becomes available. Linux kernel is no good for
reproducing this problem: it's too clean and compact.

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Johannes Schindelin]

Hi,

On Thu, 10 Aug 2006, Alex Riesen wrote:

> On 8/10/06, Junio C Hamano <junkio@cox.net> wrote:
> >   - A new merge strategy, merge-recur, which is a rewrite of
> >     merge-recursive in C, by Johannes and Alex.
> 
> It still has problems devouring that monster merge I have (~20k entries,
> with around 40 changed in the other branch, around 100 revs ago. Big
> binary files involved. Renames and copies).
> Perfomance is nowhere near usable: ~20min on Windows/3GHz/2Gb,
> ~4Min on Linux/1.4GHz/384Mb ;) I agree that Linux is much more bearable
> but 4Min is still too much (especially comparing to that "stupid" resolve).
> 
> I noticed that it spends a lot of time finding renames (diffcore_std,
> in particular).

My next plans -- after making sure that merge-recursive is accurate 
(enough) -- was to use oprofile to find the expensive spots.

> Why doesn't it take that much time for "diff-tree -M -r base head1" +
> "diff-tree -M -r base head2", I wonder...? (~30 sek, for that windows box).

Could it be that it has many common ancestors? You have to do the rename 
handling twice for each merge...

> Sorry, I can't provide the tree. I suppose Mozilla tree can be compared
> to that thing, when it becomes available. Linux kernel is no good for
> reproducing this problem: it's too clean and compact.

The beauty of Open Source: since everyone can see your mess, you tend to 
be tidier...

Ciao,
Dscho

Re: What's in git.git, and announcing GIT 1.4.2-rc4

[Alex Riesen]

On 8/10/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > I noticed that it spends a lot of time finding renames (diffcore_std,
> > in particular).
>
> My next plans -- after making sure that merge-recursive is accurate
> (enough) -- was to use oprofile to find the expensive spots.

BTW, it maybe subjective, but diff-tree seem to have slowed down too.

> > Why doesn't it take that much time for "diff-tree -M -r base head1" +
> > "diff-tree -M -r base head2", I wonder...? (~30 sek, for that windows box).
>
> Could it be that it has many common ancestors? You have to do the rename
> handling twice for each merge...

No. I'd notice that.

> > Sorry, I can't provide the tree. I suppose Mozilla tree can be compared
> > to that thing, when it becomes available. Linux kernel is no good for
> > reproducing this problem: it's too clean and compact.
>
> The beauty of Open Source: since everyone can see your mess, you tend to
> be tidier...

Yep. One of the reasons to have source closed: so your customers do
not see how dirty the mess they paid for is.