Unresolved issues

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

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
[email protected]

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.

---
As promised...

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

index b4ef6bc..54d943a 100755
@@ -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

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
[email protected]

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.

---
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(-)

index 84520e7..8e12f81 100755
@@ -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
[email protected]

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.

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
[email protected]

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

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

Nicolas

You just leave From: header in the body of message...

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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.

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

index 8e12f81..f4b8bd2 100755
@@ -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

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
[email protected]

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

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

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

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.

---

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

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

index ec506d9..cfa1511 100755
@@ -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

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.

---
 builtin-update-index.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

index 1ac613a..3fbdc67 100644
@@ -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

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)

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

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.

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

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

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

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

Hi,

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

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

hoi :)

I don't think so.

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

Hi,

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

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

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.

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

onsdag 21 februari 2007 01:56 skrev Linus Torvalds:

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

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

Hi,

Yes, that's a nice side effect.

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

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

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

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

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!

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.

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

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

Julian

 ---
  This report is filled with omissions.

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

Hi,

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