Unresolved issues

Unresolved issues

[Junio C Hamano]

Here are some issues recently raised on the list, some of them
with patches, that have not been resolved satisfactory.

 [gmane=http://thread.gmane.org/gmane.comp.version-control.git]

* "git status" is not a read-only operation.

  It needs to do enough lstat(2) to run "update-index --refresh" to come
  up with the information it needs to give.  We could do so internally
  without writing out the result to the index (there is a patch to do
  this) even if a repository is not writable.

	$gmane/39205
        $gmane/39206

  However, a big downside of this approach is that doing so
  unconditionally would mean the expensive lstat(2) is wasted
  afterwards.

	$gmane/39246

  Currently an workaround is to run git-runstatus and live with the fact
  that otherwise unmodified but stat-dirty paths to show up in the
  output.  I think (iff somebody feels strongly about it) a possible
  compromise would be to see if we can update the index, and do what the
  current code does if we can, and otherwise fall back on the new code
  that does the internal "update-index --refresh".

* "git -p status" does not honor color.

  The problem is that when git-runstatus is run, it already runs as an
  upstream process of a pipe and git_config_colorbool() would say "oh,
  our output is not a terminal, and we did not start pager ourselves".

	$gmane/39919

  A patch was proposed to propagate pager_in_use as an environment down
  to subprocesses,

	$gmane/39936

  but I think this would have unintended side effects when scripts want
  to run commands and redirect their output internally for their own use
  (they will get colorized output from lower level in their temporary
  files or v=$(cmd) redirect).

* "git log -r --raw -z -- path | grep -z SHA-1" is not very useful.  

  We would need a separate -Z option that means "output records are
  separated with NUL, but output fields are not".

	$gmane/39207

  This would help to solve "someone mails me a blob, git please tell me
  what it is" problem.

	$gmane/39925

* "git fetch" between repositories with hundreds of refs.

	$gmane/39330

  There are partial rewrite of the most expensive parts of git-fetch in
  C parked in 'pu'.  It might be good enough for public consumption
  without going the whole nine yards.  I dunno.  I am not very keen on
  rewriting all of "git fetch" in C right now, as people seem to be
  still interested in touching it (including "git bundle" topic).

* core.autocrlf

  Linus and I laid out most of the infrastructure and the basic things
  already seem to work.  We even added some tests ;-)

        commit 6716027108f426c83038b05baf3f20ceefe6fbd1
        commit 634ede32ae7d4c76e96e88f9cd5c1b3a70ea08ac
        commit d7f4633405acf3dc09798a759463c616c7c49dfd
        commit 6c510bee2013022fbce52f4b0ec0cc593fc0cc48

  What's still missing is support for .gitignore like "these files are
  text" information.

  One thing that might be tricky is what should be done while making a
  merge or checking out from a tree.  Ideally, the information should be
  read from the tree that is being extracted, but that would make the
  code structure a little bit, eh, "interesting".

* Use update-ref in cvsserver.

  It currently does it by hand, which is racy and does not leave traces
  in reflog.

	$gmane/39541

* Dissociating a repository from its alternates

  I sent out a rather elaborate changes in the binary, but what Johannes
  suggests is much easier to implement.

	$gmane/39834

  Volunteers?

* User-wide ignore list

	$gmane/39809

  I am not really sure if this is even a desirable feature, but assuming
  it is, one possible solution would be to do this:

	$gmane/39820

  But I am not going to do this myself; I suspect that it would be
  fairly simple and straightforward that some git-hacker-hopeful should
  be able to.

* git-diff2

  It somehow feels a tad ugly that this is a separate command from
  git-diff, but I do not feel strongly enough to fix it myself.
  Currently parked in 'pu'.

Re: Unresolved issues

[Andy Parkins]

On Tuesday 2007 February 20 07:28, Junio C Hamano wrote:

> * Use update-ref in cvsserver.
>
>   It currently does it by hand, which is racy and does not leave traces
>   in reflog.
>
> 	$gmane/39541

I've got a patch for this - I thought I'd sent it and it left my mind - I'll 
send when I get home.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE
andyparkins@gmail.com

Re: Unresolved issues

[Linus Torvalds]

On Mon, 19 Feb 2007, Junio C Hamano wrote:
> 
> * core.autocrlf
> 
>   What's still missing is support for .gitignore like "these files are
>   text" information.

Well, we could actually just do that in stages.

We might start off with just saying "unlike .gitignore", we only support 
one top-level ".gitattributes" file. That makes the problem space much 
simpler, and then we can have code like

	enum file_type {
		FILE_AUTO,
		FILE_BINARY,
		FILE_TEXT
	};

	static enum file_type get_file_type(const char *pathname)
	{
		static int has_initialized = 0;

		if (!has_initialized) {
			has_initialized = 1;
			read_file_attributes_file();
		}
		... check the filename against our attribute rules ..
	}

which would be fairly straightforward, and efficient.

It gets more complicated with per-directory attributes files, because then 
you need to either open those files *all* the time (stupid and expensive 
if you have thousands of files and hundreds of directories), or you need 
to have some way to cache just the ones you need.

(In fact, it might be perfectly fine to have just a *single* cache, which 
is keyed on the dirname of the pathname: if the dirname changes, just 
throw the cache away, and read it in from all the subdirectories leading 
to that directory - you'd still re-read stuff, but all the common cases 
will walk the directory structure in a nice pattern, so you'd have a very 
simple cache that actually gets good cache hit behaviour)

>   One thing that might be tricky is what should be done while making a
>   merge or checking out from a tree.  Ideally, the information should be
>   read from the tree that is being extracted, but that would make the
>   code structure a little bit, eh, "interesting".

No, that would be pretty horrid. So just tell everybody that it's based on 
the working tree. I don't think it's likely to be a problem in practice.

		Linus

[PATCH] Use git-update-ref to update a ref during commit in git-cvsserver

[Andy Parkins]

Nicholas Pitre mentioned that updating a reference should be done with
git-update-ref.

This patch does that and includes the -m option to have the reflog
updated as a bonus.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
As promised...

 git-cvsserver.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index b4ef6bc..54d943a 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1216,9 +1216,9 @@ sub req_ci
     }
 
     close LOCKFILE;
-    my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}";
-    unlink($reffile);
-    rename($lockfile, $reffile);
+    my $reffile = "refs/heads/$state->{module}";
+	`git-update-ref -m "git-cvsserver commit" $reffile $commithash $parenthash`;
+	unlink($lockfile);
     chdir "/";
 
     print "ok\n";
-- 
1.5.0.rc4.gb4d2

Re: Unresolved issues

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> (In fact, it might be perfectly fine to have just a *single* cache, which 
> is keyed on the dirname of the pathname: if the dirname changes, just 
> throw the cache away, and read it in from all the subdirectories leading 
> to that directory - you'd still re-read stuff, but all the common cases 
> will walk the directory structure in a nice pattern, so you'd have a very 
> simple cache that actually gets good cache hit behaviour)

I'd agree that we can start with just a single one at the
toplevel and if somebody wants to extend it we can do so later.

>>   One thing that might be tricky is what should be done while making a
>>   merge or checking out from a tree.  Ideally, the information should be
>>   read from the tree that is being extracted, but that would make the
>>   code structure a little bit, eh, "interesting".
>
> No, that would be pretty horrid. So just tell everybody that it's based on 
> the working tree. I don't think it's likely to be a problem in practice.

Except for the initial checkout...

Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver

[Nicolas Pitre]

On Tue, 20 Feb 2007, Andy Parkins wrote:

> Nicholas Pitre mentioned that updating a reference should be done with
> git-update-ref.
> 
> This patch does that and includes the -m option to have the reflog
> updated as a bonus.
> 
> Signed-off-by: Andy Parkins <andyparkins@gmail.com>
> ---
> As promised...

What if git-update-ref fails?  This may occur if the server repo was 
updated and the client needs to "cvs up" again before commit.


Nicolas

Re: Unresolved issues

[Linus Torvalds]

On Tue, 20 Feb 2007, Junio C Hamano wrote:
> >
> > No, that would be pretty horrid. So just tell everybody that it's based on 
> > the working tree. I don't think it's likely to be a problem in practice.
> 
> Except for the initial checkout...

Yeah, that's true. That's indeed pretty nasty.

There's also a rather strange special case when you do merges: you can 
certainly always use the .gitattributes of the working tree, but it will 
cause some interesting issues if new files were added with new patterns.

However, we're a bit lucky here (or perhaps "lucky" is not the right word: 
we basically have a good design) where all these actions come down to "git 
read-tree", regardless of whether it's checking out the end result of a 
totally new clone, or a fast-forward update, or a merge. Or a "git 
checkout" or "git reset". They all boil down to one thing:

	git read-tree -u

and it should be fairly easy to add some simple logic just to 
"cmd_read_tree()" to do the right thing. It has the "main tree" to use, 
and the logic could be as simple as

	fd = open(".gitattributes", O_RDONLY);
	if (fd < 0) {
		.. try in "$tree:.gitattributes" instead ..


and it would do the right thing for all the common operations.

Again, the special case (as always) is
 - git cat-file
 - the file-level merger code (which uses the equivalent of git-cat-file)
which would need to add their own logic for this.

		Linus

Re: Unresolved issues

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, 20 Feb 2007, Junio C Hamano wrote:
>> >
>> > No, that would be pretty horrid. So just tell everybody that it's based on 
>> > the working tree. I don't think it's likely to be a problem in practice.
>> 
>> Except for the initial checkout...
>
> Yeah, that's true. That's indeed pretty nasty.
>
> There's also a rather strange special case when you do merges: you can 
> certainly always use the .gitattributes of the working tree, but it will 
> cause some interesting issues if new files were added with new patterns.

Let alone the case where you need to merge .gitattributes file
itself and the conflicting part says something conflicting about
other paths that needed to be merged and the result need to be
checked out ;-).

> However, we're a bit lucky here (or perhaps "lucky" is not the right word: 
> we basically have a good design) where all these actions come down to "git 
> read-tree", regardless of whether it's checking out the end result of a 
> totally new clone, or a fast-forward update, or a merge. Or a "git 
> checkout" or "git reset". They all boil down to one thing:
>
> 	git read-tree -u
>
> and it should be fairly easy to add some simple logic just to 
> "cmd_read_tree()" to do the right thing. It has the "main tree" to use, 
> and the logic could be as simple as
>
> 	fd = open(".gitattributes", O_RDONLY);
> 	if (fd < 0) {
> 		.. try in "$tree:.gitattributes" instead ..
>
>
> and it would do the right thing for all the common operations.

Yes.  That is what I had in mind when I said "initial checkout".

> Again, the special case (as always) is
>  - git cat-file
>  - the file-level merger code (which uses the equivalent of git-cat-file)
> which would need to add their own logic for this.
>
> 		Linus

And git-apply which is (as usual) on its own.

Re: Unresolved issues

[Johannes Schindelin]

Hi,

On Tue, 20 Feb 2007, Linus Torvalds wrote:

> 
> On Tue, 20 Feb 2007, Junio C Hamano wrote:
> > >
> > > No, that would be pretty horrid. So just tell everybody that it's based on 
> > > the working tree. I don't think it's likely to be a problem in practice.
> > 
> > Except for the initial checkout...
> 
> Yeah, that's true. That's indeed pretty nasty.

Um, I don't want to spoil the party, but was not the original idea of this 
auto-CRLF thing some sort of "emulation" of the CVS text checkout 
behaviour?

In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, 
wrong.

It's a local setup if you want auto-CRLF or not. So, why not just make it 
a local setting (if in config or $GIT_DIR/info/gitattributes, I don't 
care) which shell patterns are to be transformed on input and/or output?

Ciao,
Dscho

Re: Unresolved issues

[David Lang]

On Tue, 20 Feb 2007, Linus Torvalds wrote:

> On Wed, 21 Feb 2007, Johannes Schindelin wrote:
>>
>> Um, I don't want to spoil the party, but was not the original idea of this
>> auto-CRLF thing some sort of "emulation" of the CVS text checkout
>> behaviour?
>>
>> In that case, .gitattributes (I mean a tracked one) would be wrong, wrong,
>> wrong.
>>
>> It's a local setup if you want auto-CRLF or not. So, why not just make it
>> a local setting (if in config or $GIT_DIR/info/gitattributes, I don't
>> care) which shell patterns are to be transformed on input and/or output?
>
> That is a good point. We *could* just make it a ".git/config" issue, which
> has the nice benefit that you can just set up some user-wide rules rather
> than making it be per-repo.

some of the things that .gitattributes is being talked being used for about for 
are local, some are per-repo

per the prior discussion of how many places to check a local configuration 
should override the per-repo setting.

David Lang

>
> Of course, the config language may not be wonderful for this. But we could
> certainly have something like
>
> 	[format "crlf"]
> 		enable = true
> 		text = *.[ch]
> 		binary = *.jpg
>
> which would just override the built-in rules (where anything that doesn't
> match is just "auto-content"). And make the default built-in ones be good
> enough that in _practice_ you never even need this in the first place.
>
> 		Linus
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Unresolved issues

[Linus Torvalds]

On Wed, 21 Feb 2007, Johannes Schindelin wrote:
>
> Um, I don't want to spoil the party, but was not the original idea of this 
> auto-CRLF thing some sort of "emulation" of the CVS text checkout 
> behaviour?
> 
> In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, 
> wrong.
> 
> It's a local setup if you want auto-CRLF or not. So, why not just make it 
> a local setting (if in config or $GIT_DIR/info/gitattributes, I don't 
> care) which shell patterns are to be transformed on input and/or output?

That is a good point. We *could* just make it a ".git/config" issue, which 
has the nice benefit that you can just set up some user-wide rules rather 
than making it be per-repo.

Of course, the config language may not be wonderful for this. But we could 
certainly have something like

	[format "crlf"]
		enable = true
		text = *.[ch]
		binary = *.jpg

which would just override the built-in rules (where anything that doesn't 
match is just "auto-content"). And make the default built-in ones be good 
enough that in _practice_ you never even need this in the first place.

		Linus

Re: Unresolved issues

[Johannes Schindelin]

Hi,

On Tue, 20 Feb 2007, Linus Torvalds wrote:

> On Wed, 21 Feb 2007, Johannes Schindelin wrote:
> >
> > Um, I don't want to spoil the party, but was not the original idea of this 
> > auto-CRLF thing some sort of "emulation" of the CVS text checkout 
> > behaviour?
> > 
> > In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, 
> > wrong.
> > 
> > It's a local setup if you want auto-CRLF or not. So, why not just make it 
> > a local setting (if in config or $GIT_DIR/info/gitattributes, I don't 
> > care) which shell patterns are to be transformed on input and/or output?
> 
> That is a good point. We *could* just make it a ".git/config" issue, 
> which has the nice benefit that you can just set up some user-wide rules 
> rather than making it be per-repo.

Yes, that's a nice side effect.

> Of course, the config language may not be wonderful for this.

Like you wrote in your example, most shell patterns are no problem in the 
config.

> And make the default built-in ones be good enough that in _practice_ you 
> never even need this in the first place.

This is really, really important. We already see many users using git, 
expecting it somehow to figure out what they want without them having read 
TFM.

Anyhow, I am right now talking with Junio (on IRC; my 2nd day wasting 
time in chatspace ;-), among other things about the import/export filters. 
We should at least keep in mind that this would be nice to have, and leave 
a door open for them when we do this stuff.

Ciao,
Dscho

Re: Unresolved issues

[Theodore Tso]

On Wed, Feb 21, 2007 at 01:39:48AM +0100, Johannes Schindelin wrote:
> In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, 
> wrong.

Well, some of the uses of .gitattributes that I had propose was to
associate file types (and from the file types, a set of check-in,
check-out, diff, and pretty-print helper progams) for things like
OpenOffice files.

For those a tracked .gitattributes makes a lot of sense.

						- Ted

Re: Unresolved issues

[Nicolas Pitre]

On Tue, 20 Feb 2007, Linus Torvalds wrote:

> 
> > It's a local setup if you want auto-CRLF or not. So, why not just make it 
> > a local setting (if in config or $GIT_DIR/info/gitattributes, I don't 
> > care) which shell patterns are to be transformed on input and/or output?
> 
> That is a good point. We *could* just make it a ".git/config" issue, which 
> has the nice benefit that you can just set up some user-wide rules rather 
> than making it be per-repo.
> 
> Of course, the config language may not be wonderful for this. But we could 
> certainly have something like
> 
> 	[format "crlf"]
> 		enable = true
> 		text = *.[ch]
> 		binary = *.jpg

I think this is not generic enough.  For one thing this should not be 
used for crlf only.  There is also the binary patch generation code that 
wants to know if a file is binary or not.

What about:

	[filetype "text"]
		match=*.[ch]
		attribute=text
		crlfmangle=true

	[filetype "images"]
		match=*.jpg
		attribute=binary
		merge=special_jpg_merger

etc.


Nicolas

Re: Unresolved issues

[Linus Torvalds]

On Tue, 20 Feb 2007, Nicolas Pitre wrote:
>
> I think this is not generic enough.  For one thing this should not be 
> used for crlf only.  There is also the binary patch generation code that 
> wants to know if a file is binary or not.
> 
> What about:
> 
> 	[filetype "text"]
> 		match=*.[ch]
> 		attribute=text
> 		crlfmangle=true
> 
> 	[filetype "images"]
> 		match=*.jpg
> 		attribute=binary
> 		merge=special_jpg_merger

Yes, that's a much nicer format - both more readable, and more generic. 

Although I'd just suggest skipping the "crlfmangle". Just document the 
fact that for "attribute=text", we mangle line-endings as per the rules 
defined elsewhere (which is possibly different for input/output in 
addition for the normal unix/windows rule changes)

And then you can just have multiple "match=" rules, so that you don't need 
to make one complex one. 

		Linus

Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> As promised...
>
>  git-cvsserver.perl |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index b4ef6bc..54d943a 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -1216,9 +1216,9 @@ sub req_ci
>      }
>  
>      close LOCKFILE;
> -    my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}";
> -    unlink($reffile);
> -    rename($lockfile, $reffile);
> +    my $reffile = "refs/heads/$state->{module}";
> +	`git-update-ref -m "git-cvsserver commit" $reffile $commithash $parenthash`;
> +	unlink($lockfile);
>      chdir "/";
>  
>      print "ok\n";

Using its own lockfile to update ref by hand while running
update-ref alongside it feels _very_ wrong.  How about this one
instead?

-- >8 --
[PATCH] Make 'cvs ci' lockless

This makes "ci" codepath lockless by following the usual
"remember the tip, do your thing, then compare and swap at the
end" update pattern using update-ref.  Incidentally, by updating
the code that reads where the tip of the head is to use
show-ref, it makes it safe to use in a repository whose refs are
pack-pruned.

I noticed that other parts of the program are not yet pack-refs
safe, but tried to keep the changes to the minimum.

Now I rarely use git-cvsserver myself, so I may be completely
breaking the check-in codepath.  Buyers beware...

---

 git-cvsserver.perl |   41 ++++++++++++++++-------------------------
 1 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 9371788..471621b 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1031,36 +1031,35 @@ sub req_ci
         exit;
     }
 
-    my $lockfile = "$state->{CVSROOT}/refs/heads/$state->{module}.lock";
-    unless ( sysopen(LOCKFILE,$lockfile,O_EXCL|O_CREAT|O_WRONLY) )
-    {
-        $log->warn("lockfile '$lockfile' already exists, please try again");
-        print "error 1 Lock file '$lockfile' already exists, please try again\n";
-        exit;
-    }
-
     # Grab a handle to the SQLite db and do any necessary updates
     my $updater = GITCVS::updater->new($state->{CVSROOT}, $state->{module}, $log);
     $updater->update();
 
     my $tmpdir = tempdir ( DIR => $TEMP_DIR );
     my ( undef, $file_index ) = tempfile ( DIR => $TEMP_DIR, OPEN => 0 );
-    $log->info("Lock successful, basing commit on '$tmpdir', index file is '$file_index'");
+    $log->info("Lockless commit start, basing commit on '$tmpdir', index file is '$file_index'");
 
     $ENV{GIT_DIR} = $state->{CVSROOT} . "/";
     $ENV{GIT_INDEX_FILE} = $file_index;
 
+    # Remember where the head was at the beginning.
+    my $parenthash = `git show-ref -s refs/heads/$state->{module}`;
+    chomp $parenthash;
+    if ($parenthash !~ /^[0-9a-f]{40}$/) {
+	    print "error 1 pserver cannot find the current HEAD of module";
+	    exit;
+    }
+
     chdir $tmpdir;
 
     # populate the temporary index based
-    system("git-read-tree", $state->{module});
+    system("git-read-tree", $parenthash);
     unless ($? == 0)
     {
 	die "Error running git-read-tree $state->{module} $file_index $!";
     }
     $log->info("Created index '$file_index' with for head $state->{module} - exit status $?");
 
-
     my @committedfiles = ();
 
     # foreach file specified on the command line ...
@@ -1095,8 +1094,6 @@ sub req_ci
         {
             # fail everything if an up to date check fails
             print "error 1 Up to date check failed for $filename\n";
-            close LOCKFILE;
-            unlink($lockfile);
             chdir "/";
             exit;
         }
@@ -1139,16 +1136,12 @@ sub req_ci
     {
         print "E No files to commit\n";
         print "ok\n";
-        close LOCKFILE;
-        unlink($lockfile);
         chdir "/";
         return;
     }
 
     my $treehash = `git-write-tree`;
-    my $parenthash = `cat $ENV{GIT_DIR}refs/heads/$state->{module}`;
     chomp $treehash;
-    chomp $parenthash;
 
     $log->debug("Treehash : $treehash, Parenthash : $parenthash");
 
@@ -1165,13 +1158,16 @@ sub req_ci
     {
         $log->warn("Commit failed (Invalid commit hash)");
         print "error 1 Commit failed (unknown reason)\n";
-        close LOCKFILE;
-        unlink($lockfile);
         chdir "/";
         exit;
     }
 
-    print LOCKFILE $commithash;
+    if (system(qw(git update-ref -m), "cvsserver ci",
+	       "refs/heads/$state->{module}", $commithash, $parenthash)) {
+	    $log->warn("update-ref for $state->{module} failed.");
+	    print "error 1 Cannot commit -- update first\n";
+	    exit;
+    }
 
     $updater->update();
 
@@ -1200,12 +1196,7 @@ sub req_ci
         }
     }
 
-    close LOCKFILE;
-    my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}";
-    unlink($reffile);
-    rename($lockfile, $reffile);
     chdir "/";
-
     print "ok\n";
 }
 

Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver

[Andy Parkins]

On Wednesday 2007 February 21 05:54, Junio C Hamano wrote:

> This makes "ci" codepath lockless by following the usual
> "remember the tip, do your thing, then compare and swap at the
> end" update pattern using update-ref.  Incidentally, by updating

Looks much better than mine (obviously).  I'll run it for a few days and 
report back.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE
andyparkins@gmail.com

Re: Unresolved issues

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

hoi :)

On Wed, Feb 21, 2007 at 01:39:48AM +0100, Johannes Schindelin wrote:
> In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, 
> wrong.

I don't think so.

> It's a local setup if you want auto-CRLF or not. So, why not just make it 
> a local setting (if in config or $GIT_DIR/info/gitattributes, I don't 
> care) which shell patterns are to be transformed on input and/or output?

The file-type/whatever information about paths is per repository.
Whether you want to do crlf conversion for "text" files is a local
setting.  So I do think that a tracked .gitattributes makes sense.

Of course you also need some local settings (e.g. in .git/config) to
use that information.  Perhaps something like:

[filetype "text*"]
	AutoCRLF = yes

[filetype "text/xml"]
	Merge = xml-merge -whatever

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Unresolved issues

[Johannes Schindelin]

Hi,

On Wed, 21 Feb 2007, Martin Waitz wrote:

> On Wed, Feb 21, 2007 at 01:39:48AM +0100, Johannes Schindelin wrote:
> > In that case, .gitattributes (I mean a tracked one) would be wrong, 
> > wrong, wrong.
> 
> I don't think so.

What you conveniently "forgot" to quote was the case: if we want this to 
decide on when to use crlf<->lf transformation, we should decide that 
locally.

But you are probably right: the information if a file _is_ fair game for 
crlf munging is probably something we might want to _be able_ to have 
tracked.

BUT there are a whole lot of problems with that approach, as Junio pointed 
out, like merging attributes files, like what to do if a file is not 
changed by a commit, but its attributes are, etc.

So, why not make the autodetection really brilliant at first, and _if_ we 
hit a hard case which cannot be autodetect, _then_ add .gitattributes 
which should _only_ force settings on misdetected files?

Ciao,
Dscho

Re: Unresolved issues

[Robin Rosenberg]

onsdag 21 februari 2007 01:56 skrev Linus Torvalds:
> 
> On Wed, 21 Feb 2007, Johannes Schindelin wrote:
> >
> > Um, I don't want to spoil the party, but was not the original idea of this 
> > auto-CRLF thing some sort of "emulation" of the CVS text checkout 
> > behaviour?
> > 
> > In that case, .gitattributes (I mean a tracked one) would be wrong, wrong, 
> > wrong.
> > 
> > It's a local setup if you want auto-CRLF or not. So, why not just make it 
> > a local setting (if in config or $GIT_DIR/info/gitattributes, I don't 
> > care) which shell patterns are to be transformed on input and/or output?
> 
> That is a good point. We *could* just make it a ".git/config" issue, which 
> has the nice benefit that you can just set up some user-wide rules rather 
> than making it be per-repo.
> 
> Of course, the config language may not be wonderful for this. But we could 
> certainly have something like
> 
> 	[format "crlf"]
> 		enable = true
> 		text = *.[ch]
> 		binary = *.jpg

The decision whether to mangel at all shoule be local. Which files to mangle, if mangle is "on",
should be a per version (not like CVS' setting for all versions). Otherwise it won't
be propagated properly on push/pull, and people *will* get it wrong over and over. 

It it's .gitattributes or similar it can be merged as any other file and conflicts can be resolved like
any other file. For efficiency you can have one .gitattributes.

Hopefully it won't happen often because autodetection is soo good, but when it get's 
wrong it's important that it can be fixed and distributed properly.

-- robin

Re: Unresolved issues

[Brian Gernhardt]

On Feb 21, 2007, at 7:55 AM, Johannes Schindelin wrote:

> What you conveniently "forgot" to quote was the case: if we want  
> this to
> decide on when to use crlf<->lf transformation, we should decide that
> locally.

It seems to me that a tracked .gitattributes file should have things  
like

*.txt: text
*.gif: binary
*.[ch]: text

And the .git/config should have

[attribute "text"]
    mangle = crlf

[attribute "binary"]
    merge = none

The type of each file should be tracked, but what to do with each  
type is a local issue.  Trying to merge the two is madness.

~~ Brian

Re: Unresolved issues

[Shawn O. Pearce]

Brian Gernhardt <benji@silverinsanity.com> wrote:
> It seems to me that a tracked .gitattributes file should have things  
> like
> 
> *.txt: text
> *.gif: binary
> *.[ch]: text
> 
> And the .git/config should have
> 
> [attribute "text"]
>    mangle = crlf
> 
> [attribute "binary"]
>    merge = none
> 
> The type of each file should be tracked, but what to do with each  
> type is a local issue.  Trying to merge the two is madness.

Yes, exactly.  :-)

I would also recommend that we encourage use of standard MIME types
to define the file types, but don't enforce it.  Thus I can setup
something like:

  cat >.gitattributes <<EOF
  *.txt: text/plain
  *.java: text/java-source
  *.xml: text/xml
  *.bin: mycompany-binary
  EOF

  cat >>.git/config <<EOF
  [attribute "text/*"]
    mangle = crlf
  [attribute "text/xml"]
    merge = better-xml-mergething
  [attribute "mycompany-binary"]
    mangle = no
    merge = mycompany-binarymerge
  EOF

And have all three classes of files be mangled with CRLF, but
XML files are also merged with the external merge process, and the
special type mycompany-binary might be a local file format that comes
with its own merge tools, but is not exactly a registered MIME type.

One advantage here is we can setup attribute.text/*.mangle=crlf on
Windows platforms by default for users, as we can reasonably assume
all text content falls into this MIME type...

-- 
Shawn.

[PATCH] git-status: do not be totally useless in a read-only repository.

[Junio C Hamano]

This makes git-status work semi-decently in a read-only
repository.  Earlier, the command simply died with "cannot lock
the index file" before giving any useful information to the
user.

Because index won't be updated in a read-only repository,
stat-dirty paths appear in the "Changed but not updated" list.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

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

  >  [gmane=http://thread.gmane.org/gmane.comp.version-control.git]
  >
  > * "git status" is not a read-only operation.
  >
  >   It needs to do enough lstat(2) to run "update-index --refresh" to come
  >   up with the information it needs to give.  We could do so internally
  >   without writing out the result to the index (there is a patch to do
  >   this) even if a repository is not writable.
  >
  >     $gmane/39205
  >     $gmane/39206
  >
  >   However, a big downside of this approach is that doing so
  >   unconditionally would mean the expensive lstat(2) is wasted
  >   afterwards.
  >
  >     $gmane/39246
  >
  >   Currently an workaround is to run git-runstatus and live with the fact
  >   that otherwise unmodified but stat-dirty paths to show up in the
  >   output.  I think (iff somebody feels strongly about it) a possible
  >   compromise would be to see if we can update the index, and do what the
  >   current code does if we can, and otherwise fall back on the new code
  >   that does the internal "update-index --refresh".

  I did not feel strongly enough about it, so here is another
  approach.

 git-commit.sh |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index ec506d9..cfa1511 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -13,10 +13,10 @@ git-rev-parse --verify HEAD >/dev/null 2>&1 || initial_commit=t
 case "$0" in
 *status)
 	status_only=t
-	unmerged_ok_if_status=--unmerged ;;
+	;;
 *commit)
 	status_only=
-	unmerged_ok_if_status= ;;
+	;;
 esac
 
 refuse_partial () {
@@ -389,16 +389,17 @@ else
 	USE_INDEX="$THIS_INDEX"
 fi
 
-GIT_INDEX_FILE="$USE_INDEX" \
-	git-update-index -q $unmerged_ok_if_status --refresh || exit
-
-################################################################
-# If the request is status, just show it and exit.
-
-case "$0" in
-*status)
+case "$status_only" in
+t)
+	# This will silently fail in a read-only repository, which is
+	# what we want.
+	GIT_INDEX_FILE="$USE_INDEX" git-update-index -q --unmerged --refresh
 	run_status
 	exit $?
+	;;
+'')
+	GIT_INDEX_FILE="$USE_INDEX" git-update-index -q --refresh || exit
+	;;
 esac
 
 ################################################################
-- 
1.5.0.1.619.g04c5c

[PATCH] update-index: do not die too early in a read-only repository.

[Junio C Hamano]

This delays the error exit from hold_lock_file_for_update() in
update-index, so that "update-index --refresh" in a read-only
repository can still report what paths are stat-dirty before
exiting.

Also it makes -q to squelch the error message.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-update-index.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/builtin-update-index.c b/builtin-update-index.c
index 1ac613a..3fbdc67 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -486,6 +486,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	char set_executable_bit = 0;
 	unsigned int refresh_flags = 0;
+	int lock_error = 0;
 	struct lock_file *lock_file;
 
 	git_config(git_default_config);
@@ -493,7 +494,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
 	lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	newfd = hold_lock_file_for_update(lock_file, get_index_file(), 1);
+	newfd = hold_lock_file_for_update(lock_file, get_index_file(), 0);
+	if (newfd < 0)
+		lock_error = errno;
 
 	entries = read_cache();
 	if (entries < 0)
@@ -650,6 +653,12 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
  finish:
 	if (active_cache_changed) {
+		if (newfd < 0) {
+			if (refresh_flags & REFRESH_QUIET)
+				exit(128);
+			die("unable to create '%s.lock': %s",
+			    get_index_file(), strerror(lock_error));
+		}
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    close(newfd) || commit_lock_file(lock_file))
 			die("Unable to write new index file");
-- 
1.5.0.1.619.g04c5c

Re: Unresolved issues

[Julian Phillips]

On Mon, 19 Feb 2007, Junio C Hamano wrote:

> * "git fetch" between repositories with hundreds of refs.
>
> 	$gmane/39330
>
>  There are partial rewrite of the most expensive parts of git-fetch in
>  C parked in 'pu'.  It might be good enough for public consumption
>  without going the whole nine yards.  I dunno.  I am not very keen on
>  rewriting all of "git fetch" in C right now, as people seem to be
>  still interested in touching it (including "git bundle" topic).

The current changes in jc/fetch take things from "unusable" to "a bit 
slow", which I think could quite easily be considered a separate task from 
"a bit slow" to "something that even Linus would consider reasonable".  So 
my opinion would be to get the current improvements in so that they can be 
combined with the other good work happening in this area, and wait for 
things to settle before going the last mile (after all anyone converting 
from Subversion or CVS probably won't find 30s to be slow anyway ... ;)).

I would be happy to work on this if you would rather spend your time on 
more generally useful things ...

-- 
Julian

  ---
Stewie Griffin:  [thinks] How wonderful it will be to have mother back!
Brian Griffin:  [thinks] I heard that.
Stewie Griffin:  [thinks] Damn!

Re: Unresolved issues

[Junio C Hamano]

Julian Phillips <julian@quantumfyre.co.uk> writes:

> On Mon, 19 Feb 2007, Junio C Hamano wrote:
>
>> * "git fetch" between repositories with hundreds of refs.
>>
>> 	$gmane/39330
>>
>>  There are partial rewrite of the most expensive parts of git-fetch in
>>  C parked in 'pu'.  It might be good enough for public consumption
>>  without going the whole nine yards.  I dunno.  I am not very keen on
>>  rewriting all of "git fetch" in C right now, as people seem to be
>>  still interested in touching it (including "git bundle" topic).
>
> The current changes in jc/fetch take things from "unusable" to "a bit
> slow", which I think could quite easily be considered a separate task
> from "a bit slow" to "something that even Linus would consider
> reasonable".  So my opinion would be to get the current improvements
> in so that they can be combined with the other good work happening in
> this area, and wait for things to settle before going the last mile
> (after all anyone converting from Subversion or CVS probably won't
> find 30s to be slow anyway ... ;)).

I was kind of waiting for dust from Santi's code shuffling to
settle down, because the series moderately conflicts with it.  I
wanted to take Santi's patch first as it was supposed to be a
clean-up without any functionality changes, although it was kind
of painful to really make sure there is no regression.

If what jc/fetch topic tries to do helps real users, let's merge
it in 'next' first, as Santi's change is not supposed to bring
any improvements by itself even when it proves regression-free.

In the short term, this means we have to ask Santi to rebase his
patch instead of the other way around as I planned first, which
is a bit unfortunate.

Re: Unresolved issues

[Julian Phillips]

On Sun, 25 Feb 2007, Junio C Hamano wrote:

> Julian Phillips <julian@quantumfyre.co.uk> writes:
>
>> On Mon, 19 Feb 2007, Junio C Hamano wrote:
>>
>>> * "git fetch" between repositories with hundreds of refs.
>>>
>>> 	$gmane/39330
>>>
>>>  There are partial rewrite of the most expensive parts of git-fetch in
>>>  C parked in 'pu'.  It might be good enough for public consumption
>>>  without going the whole nine yards.  I dunno.  I am not very keen on
>>>  rewriting all of "git fetch" in C right now, as people seem to be
>>>  still interested in touching it (including "git bundle" topic).
>>
>> The current changes in jc/fetch take things from "unusable" to "a bit
>> slow", which I think could quite easily be considered a separate task
>> from "a bit slow" to "something that even Linus would consider
>> reasonable".  So my opinion would be to get the current improvements
>> in so that they can be combined with the other good work happening in
>> this area, and wait for things to settle before going the last mile
>> (after all anyone converting from Subversion or CVS probably won't
>> find 30s to be slow anyway ... ;)).
>
> I was kind of waiting for dust from Santi's code shuffling to
> settle down, because the series moderately conflicts with it.  I
> wanted to take Santi's patch first as it was supposed to be a
> clean-up without any functionality changes, although it was kind
> of painful to really make sure there is no regression.

Indeed.  I was thinking pretty much the same.  It seems unnecessary to 
make Santi rebase his patch without any evidence that the jc/fetch topic 
is actually urgently needed by anyone.

I was advocating a two step approach, but I didn't mean to give the 
impressions that I wanted the topic merged now.  I envisioned both steps 
as being after the current active work that is affecting git-fetch. 
Thanks to the power of git I have got myself a master+jc/fetch branch so 
I'm happy - so unless there is anyone else out there working with stupidly 
large numbers of refs I'm quite happy to let Santi's work go in first. 
I'll even have a go at rebasing the fetch improvements ontop of Santi's 
work ...

>
> If what jc/fetch topic tries to do helps real users, let's merge
> it in 'next' first, as Santi's change is not supposed to bring
> any improvements by itself even when it proves regression-free.
>
> In the short term, this means we have to ask Santi to rebase his
> patch instead of the other way around as I planned first, which
> is a bit unfortunate.

Unless there is someone else out there that wants jc/fetch badly, I would 
say "after you" ...

-- 
Julian

  ---
   This report is filled with omissions.

Re: Unresolved issues

[Junio C Hamano]

Julian Phillips <julian@quantumfyre.co.uk> writes:

>> I was kind of waiting for dust from Santi's code shuffling to
>> settle down, because the series moderately conflicts with it.  I
>> wanted to take Santi's patch first as it was supposed to be a
>> clean-up without any functionality changes, although it was kind
>> of painful to really make sure there is no regression.
>
> Indeed.  I was thinking pretty much the same.  It seems unnecessary to
> make Santi rebase his patch without any evidence that the jc/fetch
> topic is actually urgently needed by anyone.

Sorry, too late --- it's a done deal.  Partial C rewrite of
git-fetch is now in 'next'.

[PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref

[Andy Parkins]

This makes "ci" codepath lockless by following the usual
"remember the tip, do your thing, then compare and swap at the
end" update pattern using update-ref.  Incidentally, by updating
the code that reads where the tip of the head is to use
show-ref, it makes it safe to use in a repository whose refs are
pack-pruned.

I noticed that other parts of the program are not yet pack-refs
safe, but tried to keep the changes to the minimum.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
This patch is actually yours (with one extra removal of lock file reference
that you'd missed, and a change of shortlog), but I don't know how to send
an email that comes from me but attributes authorship to you.


 git-cvsserver.perl |   43 ++++++++++++++++---------------------------
 1 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 84520e7..8e12f81 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1031,36 +1031,35 @@ sub req_ci
         exit;
     }
 
-    my $lockfile = "$state->{CVSROOT}/refs/heads/$state->{module}.lock";
-    unless ( sysopen(LOCKFILE,$lockfile,O_EXCL|O_CREAT|O_WRONLY) )
-    {
-        $log->warn("lockfile '$lockfile' already exists, please try again");
-        print "error 1 Lock file '$lockfile' already exists, please try again\n";
-        exit;
-    }
-
     # Grab a handle to the SQLite db and do any necessary updates
     my $updater = GITCVS::updater->new($state->{CVSROOT}, $state->{module}, $log);
     $updater->update();
 
     my $tmpdir = tempdir ( DIR => $TEMP_DIR );
     my ( undef, $file_index ) = tempfile ( DIR => $TEMP_DIR, OPEN => 0 );
-    $log->info("Lock successful, basing commit on '$tmpdir', index file is '$file_index'");
+    $log->info("Lockless commit start, basing commit on '$tmpdir', index file is '$file_index'");
 
     $ENV{GIT_DIR} = $state->{CVSROOT} . "/";
     $ENV{GIT_INDEX_FILE} = $file_index;
 
+    # Remember where the head was at the beginning.
+    my $parenthash = `git show-ref -s refs/heads/$state->{module}`;
+    chomp $parenthash;
+    if ($parenthash !~ /^[0-9a-f]{40}$/) {
+	    print "error 1 pserver cannot find the current HEAD of module";
+	    exit;
+    }
+
     chdir $tmpdir;
 
     # populate the temporary index based
-    system("git-read-tree", $state->{module});
+    system("git-read-tree", $parenthash);
     unless ($? == 0)
     {
 	die "Error running git-read-tree $state->{module} $file_index $!";
     }
     $log->info("Created index '$file_index' with for head $state->{module} - exit status $?");
 
-
     my @committedfiles = ();
 
     # foreach file specified on the command line ...
@@ -1095,8 +1094,6 @@ sub req_ci
         {
             # fail everything if an up to date check fails
             print "error 1 Up to date check failed for $filename\n";
-            close LOCKFILE;
-            unlink($lockfile);
             chdir "/";
             exit;
         }
@@ -1139,16 +1136,12 @@ sub req_ci
     {
         print "E No files to commit\n";
         print "ok\n";
-        close LOCKFILE;
-        unlink($lockfile);
         chdir "/";
         return;
     }
 
     my $treehash = `git-write-tree`;
-    my $parenthash = `cat $ENV{GIT_DIR}refs/heads/$state->{module}`;
     chomp $treehash;
-    chomp $parenthash;
 
     $log->debug("Treehash : $treehash, Parenthash : $parenthash");
 
@@ -1165,8 +1158,6 @@ sub req_ci
     {
         $log->warn("Commit failed (Invalid commit hash)");
         print "error 1 Commit failed (unknown reason)\n";
-        close LOCKFILE;
-        unlink($lockfile);
         chdir "/";
         exit;
     }
@@ -1179,14 +1170,17 @@ sub req_ci
 		{
 			$log->warn("Commit failed (update hook declined to update ref)");
 			print "error 1 Commit failed (update hook declined)\n";
-			close LOCKFILE;
-			unlink($lockfile);
 			chdir "/";
 			exit;
 		}
 	}
 
-    print LOCKFILE $commithash;
+	if (system(qw(git update-ref -m), "cvsserver ci",
+			"refs/heads/$state->{module}", $commithash, $parenthash)) {
+		$log->warn("update-ref for $state->{module} failed.");
+		print "error 1 Cannot commit -- update first\n";
+		exit;
+	}
 
     $updater->update();
 
@@ -1215,12 +1209,7 @@ sub req_ci
         }
     }
 
-    close LOCKFILE;
-    my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}";
-    unlink($reffile);
-    rename($lockfile, $reffile);
     chdir "/";
-
     print "ok\n";
 }
 
-- 
1.5.0.2.778.gdcb06
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

[PATCH 2/2] cvsserver: Remove trailing "\n" from commithash in checkin function

[Andy Parkins]

The commithash for updating the ref is obtained from a call to
git-commit-tree.  However, it was returned (and stored) with the
trailing newline.  This meant that the later call to git-update-ref that
was trying to update to $commithash was including the newline in the
parameter - obviously that hash would never exist, and so git-update-ref
would always fail.

The solution is to chomp() the commithash as soon as it is returned by
git-commit-tree.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 git-cvsserver.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 8e12f81..f4b8bd2 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1152,6 +1152,7 @@ sub req_ci
     close $msg_fh;
 
     my $commithash = `git-commit-tree $treehash -p $parenthash < $msg_filename`;
+	chomp($commithash);
     $log->info("Commit hash : $commithash");
 
     unless ( $commithash =~ /[a-zA-Z0-9]{40}/ )
-- 
1.5.0.2.778.gdcb06

Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref

[Jakub Narebski]

Andy Parkins wrote:

> ---
> This patch is actually yours (with one extra removal of lock file reference
> that you'd missed, and a change of shortlog), but I don't know how to send
> an email that comes from me but attributes authorship to you.

You just leave From: header in the body of message...
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref

[Nicolas Pitre]

On Tue, 27 Feb 2007, Andy Parkins wrote:

> This patch is actually yours (with one extra removal of lock file reference
> that you'd missed, and a change of shortlog), but I don't know how to send
> an email that comes from me but attributes authorship to you.

Start your email body with a "From: " and the address of the person.


Nicolas

Re: Unresolved issues

[Johannes Schindelin]

Hi,

On Mon, 26 Feb 2007, Julian Phillips wrote:

> On Mon, 19 Feb 2007, Junio C Hamano wrote:
> 
> > * "git fetch" between repositories with hundreds of refs.
> 
> I would be happy to work on this if you would rather spend your time on 
> more generally useful things ...

I would be grateful indeed if you worked on a builtin fetch. But beware, I 
think it is quite some work...

The good thing: We have some tests involving git-fetch, so if all the 
tests pass, chances are that the builtin fetch works correctly.

(Of course, we do not have tests for HTTP, SSH and GIT fetch... Anyone?)

Ciao,
Dscho

Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver

[Martin Langhoff]

Hi Junio, Andy,

sorry for the long delay, I'm catching up with a sizable backlog at work
and in my foss projects.

I like Junio's patch -- it fixes cvsserver to work with packed refs
(which needed to be done!) and does things lockless, which is a great bonus.

The meat of the matter is

Junio C Hamano wrote:
> -    print LOCKFILE $commithash;
> +    if (system(qw(git update-ref -m), "cvsserver ci",
> +	       "refs/heads/$state->{module}", $commithash, $parenthash)) {
> +	    $log->warn("update-ref for $state->{module} failed.");
> +	    print "error 1 Cannot commit -- update first\n";
> +	    exit;
> +    }
>
>      $updater->update();

Running the commit lockless makes it a little bit more likely that we'll
fail the commit after all files have been sent. Some older CVS clients
have broken error handling in the late stages of the commit, but we
cannot really fix that - and such cvs clients are so broken that they
probably don't deserve our attention.

The other area I checked is that we don't get a nasty race condition
between the update-ref and calling $updater->update() - but it is safe.

So ack from this corner and thanks for the patch!

cheers,


martin
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
OFFICE: +64(4)916-7224  UK: 0845 868 5733 ext 7224  MOB: +64(21)364-017
      Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------

Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> This patch is actually yours (with one extra removal of lock file reference
> that you'd missed, and a change of shortlog), but I don't know how to send
> an email that comes from me but attributes authorship to you.

The extra one was introduced by a later patch to honor the
update hook since I wrote the original patch you forward ported.

Thanks.  Will apply.

Re: [PATCH 2/2] cvsserver: Remove trailing "\n" from commithash in checkin function

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> The commithash for updating the ref is obtained from a call to
> git-commit-tree.  However, it was returned (and stored) with the
> trailing newline.  This meant that the later call to git-update-ref that
> was trying to update to $commithash was including the newline in the
> parameter - obviously that hash would never exist, and so git-update-ref
> would always fail.
>
> The solution is to chomp() the commithash as soon as it is returned by
> git-commit-tree.
>
> Signed-off-by: Andy Parkins <andyparkins@gmail.com>
>      my $commithash = `git-commit-tree $treehash -p $parenthash < $msg_filename`;
> +	chomp($commithash);
>      $log->info("Commit hash : $commithash");
>  

Thanks.  Do we need to compensate with a trailing LF in the $log
line?

Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref

[Andy Parkins]

On Tuesday 2007 February 27 23:44, Junio C Hamano wrote:

> The extra one was introduced by a later patch to honor the
> update hook since I wrote the original patch you forward ported.

Apologies.  I must have applied your patch in the wrong place on my branch.  
It did seem unlikely that you would have made a mistake.


Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH 2/2] cvsserver: Remove trailing "\n" from commithash in checkin function

[Andy Parkins]

On Tuesday 2007 February 27 23:45, Junio C Hamano wrote:

> Thanks.  Do we need to compensate with a trailing LF in the $log
> line?

Interestingly, no.  I checked that, and the log has always had an unnecessary 
blank line in it - which is of course now removed.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH 1/2] Make 'cvs ci' lockless in git-cvsserver by using git-update-ref

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> On Tuesday 2007 February 27 23:44, Junio C Hamano wrote:
>
>> The extra one was introduced by a later patch to honor the
>> update hook since I wrote the original patch you forward ported.
>
> Apologies.  I must have applied your patch in the wrong place on my branch.  
> It did seem unlikely that you would have made a mistake.

Apologies if I sounded I was complaining -- not at all.
I was _thanking_ you for the forward porting of my patch so I
did not have to do that ;-).

Unresolved issues

[Junio C Hamano]

Here is an "issues" list I am keeping track of on the back of my head (and
in "TODO" file on 'todo' branch, which is not updated often enough).

The tail end of this list is probably a bit stale, as I am running out of
time tonight and did not have enough time to check the current status.
Please spot issues that have been resolved and report them.

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

Possible bug not diagnosed yet

* "git commit --interactive" allows same tree as parent to be recorded

Message-ID: <slrng059n3.nd8.joerg@alea.gnuu.de>

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

Issues that are unconcluded or need a fresh infusion of kerosene to
firestart them again.

* adding across symlinks (me)

Message-ID: <7vd4oppllw.fsf@gitster.siamese.dyndns.org>

* cvsserver improvements (Fredrik Noring)

Needs third-party confirmation and then properly applicable patches.

* reworking rebase interactive (Jörg Sommer)

Message-Id: <1208132469-26471-1-git-send-email-joerg@alea.gnuu.de>
Message-Id: <1208169584-15931-1-git-send-email-joerg@alea.gnuu.de>

* log --graph (Adam Simpkins)

Message-ID: <20080406214445.GA5822@adamsimpkins.net>

* --pretty=format:%d (Dscho)

* SP in path to $GIT_DIR breaks some scripts and many tests (Bryan Donlan)

Message-ID: <20080410084820.GA1904@shion.is.fushizen.net>

* Prepare cvsimport for fixed cvsps (David Mansfield)

Message-Id: <1207100091.10532.64.camel@gandalf.cobite.com>

* ff=only (Sverre Hvammen Johansen)
  $gmane/78250

* more help backends (Christian Couder)
  $gmane/78150

* cvsserver updates (Damien Diederen with help by Frank Lichtenheld)
  $gmane/78188

* rewriting annotated tags in filter-branch (Brandon Casey)
  $gmane/78286

* fsck --lost-found (me)
  $gmane/78267

* core.inithook (Dscho)
  $gmane/78123

* receive.localBranches = { refuse | allow } (Dscho)
  $gmane/78065

* autosetting core.ignorecase (Dmitry Potapov)
  $gmane/78176

* mailinfo extention to extract Message-ID and others (Anton Gladkov)
  $gmane/78006

* rebase -p
  $gmane/78074

  There is a quite well done series from Jörg Sommer I have to queue but
  I am getting the feeling that it did not get enough review from the
  list.  I am reading it through (slowly) myself, but I'd appreciate more
  review comments on the list (not limited to this series).

* synopsys: use {} instead of () for grouping alternatives (Jari Aalto)
  $gmane/72243

* "[alias] st = status" and "cd .git && git st" (Jeff King)
  $gmane/72327

* "git fetch" does not exit with non-zero status when it failed to update
  some refs due to non-ffness (Daniel)
  $gmane/77178

* use "assume unchanged" bit to implement narrow checkout
  $gmane/77046

* zlib abstraction (Marco)
  $gmane/72262

* git --index-file=<foo> <cmd> (Linus)
  $gmane/77332

* git lost-found vs git fsck --lost-found
  $gmane/78267

Re: Unresolved issues

[Sam Vilain]

Linus Torvalds wrote:
>>   When you run "git rev-list A..B C", and there is a commit in the chain
>>   between A and B whose timestamp is much older than its parent, sometimes
>>   we fail to mark C as reachable from A (hence not interesting) even when
>>   it actualy is.  This is very expensive to solve in general, and we are
>>   not going to introduce "generation number" field to the commit objects,
>>   so we may have to settle with a heuristic.
> Here is the already posted heuristic that fixes both t/t6009 and the 
> real-world case that triggered the whole discussion.
> It's certainly not perfect, but I think it's likely an improvement on what 
> we have now, and it should be robust in the face of the _occasional_ wrong 
> date.
> Now, if there are consistently totally bogus dates, the SLOP thing won't 
> help, but ...

Ouch - I had always supposed that topology was king, and that the commit
dates were purely informational.  In particular the Perl history that I
produced in general takes a position of blatant and wanton disregard to
such consistency.

I can't find the other thread you refer to - is there a good summary of
the issues somewhere?  The test script is not very descriptive.

If timewise out-of-order commits are bad, perhaps git-filter-branch
should warn when it is creating histories whose topology disagrees with
their chronology... and also the user manual should probably describe this.

Sam

Re: Unresolved issues

[Linus Torvalds]

On Mon, 17 Mar 2008, Junio C Hamano wrote:
> 
> * revision.c::limit_list() breakage
>   $gmane/72324
>   t/t6009
> 
>   When you run "git rev-list A..B C", and there is a commit in the chain
>   between A and B whose timestamp is much older than its parent, sometimes
>   we fail to mark C as reachable from A (hence not interesting) even when
>   it actualy is.  This is very expensive to solve in general, and we are
>   not going to introduce "generation number" field to the commit objects,
>   so we may have to settle with a heuristic.

Here is the already posted heuristic that fixes both t/t6009 and the 
real-world case that triggered the whole discussion.

It's certainly not perfect, but I think it's likely an improvement on what 
we have now, and it should be robust in the face of the _occasional_ wrong 
date.

Now, if there are consistently totally bogus dates, the SLOP thing won't 
help, but ...

		Linus
---
From: Linus Torvalds <torvalds@woody.linux-foundation.org>

Make revision limiting more robust against occasional bad commit dates

The revision limiter uses the commit date to decide when it has seen
enough commits to finalize the revision list, but that can get confused
if there are incorrect dates far in the past on some commits.

This makes the logic a bit more robust by

 - we always walk an extra SLOP commits from the source list even if we
   decide that the source list is probably all done (unless the source is
   entirely empty, of course, because then we really can't do anything at
   all)

 - we keep track of the date of the last commit we added to the
   destination list (this will *generally* be the oldest entry we've seen
   so far)

 - we compare that with the youngest entry (the first one) of the source
   list, and if the destination is older than the source, we know we want
   to look at the source.

which causes occasional date mishaps to be handled cleanly.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 revision.c |   46 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/revision.c b/revision.c
index 63bf2c5..196fedc 100644
--- a/revision.c
+++ b/revision.c
@@ -564,14 +564,39 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 	free_patch_ids(&ids);
 }
 
-static void add_to_list(struct commit_list **p, struct commit *commit, struct commit_list *n)
+/* How many extra uninteresting commits we want to see.. */
+#define SLOP 5
+
+static int still_interesting(struct commit_list *src, unsigned long date, int slop)
 {
-	p = &commit_list_insert(commit, p)->next;
-	*p = n;
+	/*
+	 * No source list at all? We're definitely done..
+	 */
+	if (!src)
+		return 0;
+
+	/*
+	 * Does the destination list contain entries with a date
+	 * before the source list? Definitely _not_ done.
+	 */
+	if (date < src->item->date)
+		return SLOP;
+
+	/*
+	 * Does the source list still have interesting commits in
+	 * it? Definitely not done..
+	 */
+	if (!everybody_uninteresting(src))
+		return SLOP;
+
+	/* Ok, we're closing in.. */
+	return slop-1;
 }
 
 static int limit_list(struct rev_info *revs)
 {
+	int slop = SLOP;
+	unsigned long date = ~0ul;
 	struct commit_list *list = revs->commits;
 	struct commit_list *newlist = NULL;
 	struct commit_list **p = &newlist;
@@ -591,16 +616,19 @@ static int limit_list(struct rev_info *revs)
 			return -1;
 		if (obj->flags & UNINTERESTING) {
 			mark_parents_uninteresting(commit);
-			if (everybody_uninteresting(list)) {
-				if (revs->show_all)
-					add_to_list(p, commit, list);
-				break;
-			}
-			if (!revs->show_all)
-				continue;
+			if (revs->show_all)
+				p = &commit_list_insert(commit, p)->next;
+			slop = still_interesting(list, date, slop);
+			if (slop)
+				continue;
+			/* If showing all, add the whole pending list to the end */
+			if (revs->show_all)
+				*p = list;
+			break;
 		}
 		if (revs->min_age != -1 && (commit->date > revs->min_age))
 			continue;
+		date = commit->date;
 		p = &commit_list_insert(commit, p)->next;
 
 		show = show_early_output;

Re: Unresolved issues

[Jeff King]

On Mon, Mar 17, 2008 at 06:12:02PM -0700, Junio C Hamano wrote:

> * "[alias] st = status" and "cd .git && git st" (Jeff King)
>   $gmane/72327
> 
>   This shows everything as deleted, I believe it hasn't resolved.  I am
>   not sure if this is worth resolving, though.

I started on a patch for this, but got mired in trying to clean up the
worktree handling in this area, which is quite confusing and fragile. I
have a suspicion that Duy's recent patch series will impact this, but I
haven't had time yet to review that carefully.

-Peff

Unresolved issues

[Junio C Hamano]

I tagged 1.5.5-rc0 Sunday night (my time, obviously).

Our contributors have been busy inventing new features and reinventing old
ones in C during the 1.5.5 cycle, and we have a fair number of known
breakages.  Here is a short list of issues I know (or I think I've heard)
about, that we would like to address (either "resolve", or "declare to
postpone") before the final release, but I am sure I missed some things.

Let's hope contributors are as responsive in fixing their own mess as they
are responsive in scratching their own itch, and we can resolve most of
them shortly.

* synopsys: use {} instead of () for grouping alternatives (Jari Aalto)
  $gmane/72243

  This was discussed during 1.5.4 pre-freeze timeframe but never
  materialized.

* "git remote" showing remotes/origin/HEAD as a candidate for pruning,
  and pruning it results in removal of what is pointed at by it.

  Pointers?  This may not be a regression but bug-to-bug compatibility
  with the older implementation, but this should better be fixed.

* fetch with "refs/*:refs/*" errors out erroneously
  $gmane/77335

  Breakage exposed by recent git allowing "mirror" layout with "git remote
  add --mirror".

* fetch with tag following uses smudged object database
  $gmane/74141

  Regression introduced by recent C-rewrite of git-fetch.

* "git fetch" does not exit with non-zero status when it failed to update
  some refs due to non-ffness
  $gmane/77178

  Regression introduced by recent C-rewrite of git-fetch.

* "git fetch" shows error when dangling symref exists at the remote
  but does not really error out
  $gmane/76658

  I am not sure what the right course of action is.  Maybe we should
  ignore dangling symrefs in upload-pack?

* D/F conflict to merge a tree with D into a tree with F
  $gmane/77352

  Needs more info.

* revision.c::limit_list() breakage
  $gmane/72324
  t/t6009

  When you run "git rev-list A..B C", and there is a commit in the chain
  between A and B whose timestamp is much older than its parent, sometimes
  we fail to mark C as reachable from A (hence not interesting) even when
  it actualy is.  This is very expensive to solve in general, and we are
  not going to introduce "generation number" field to the commit objects,
  so we may have to settle with a heuristic.

* "[alias] st = status" and "cd .git && git st" (Jeff King)
  $gmane/72327

  This shows everything as deleted, I believe it hasn't resolved.  I am
  not sure if this is worth resolving, though.