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

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

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

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

On Tue, 20 Feb 2007, Andy Parkins wrote:

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

Andy Parkins <andyparkins@gmail.com> writes:

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

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

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

Andy

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

Andy Parkins wrote:

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

On Tue, 27 Feb 2007, Andy Parkins wrote:

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

Nicolas

Andy Parkins <andyparkins@gmail.com> writes:

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.

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

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

Andy Parkins <andyparkins@gmail.com> writes:

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

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

Andy Parkins <andyparkins@gmail.com> writes:

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

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

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

Andy

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:

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

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

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

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

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

On Tue, 20 Feb 2007, Junio C Hamano wrote:

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

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

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,

On Tue, 20 Feb 2007, Linus Torvalds 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?

Ciao, Dscho

On Wed, 21 Feb 2007, Johannes Schindelin wrote:

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

On Tue, 20 Feb 2007, Linus Torvalds wrote:

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,

On Tue, 20 Feb 2007, Linus Torvalds wrote:

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

On Tue, 20 Feb 2007, Linus Torvalds 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

etc.

Nicolas

On Tue, 20 Feb 2007, Nicolas Pitre wrote:

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

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

On Wed, Feb 21, 2007 at 01:39:48AM +0100, Johannes Schindelin wrote:

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

On Wed, Feb 21, 2007 at 01:39:48AM +0100, Johannes Schindelin wrote:

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

Hi,

On Wed, 21 Feb 2007, Martin Waitz 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.

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

On Feb 21, 2007, at 7:55 AM, Johannes Schindelin 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.

~~ Brian

Brian Gernhardt <benji@silverinsanity.com> wrote:

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

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

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

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

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

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.

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

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 Phillips <julian@quantumfyre.co.uk> writes:

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

Hi,

On Mon, 26 Feb 2007, Julian Phillips wrote:

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