[RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Chris Wedgwood]

Sometimes (often actually) I do:

          cp -Rl tree1 tree2    # new tree with implied CoW semantics
          cd tree2
          cg-update             # or similar

the latter well frob .git/HEAD or similar by doing echo foo > bar
which obviously breaks the intended CoW semantics.

How would people feel about something like the patch below be?  (RFC
purposes only, please eyeball this before blidnly applying anything.
I may have missed spots or goofed, you never know).



diff --git a/cg-Xlib b/cg-Xlib
--- a/cg-Xlib
+++ b/cg-Xlib
@@ -21,6 +21,12 @@ usage() {
 	die "usage: $USAGE"
 }
 
+echo_to_file() {
+    TMPFILE=`mktemp "$2.tmp-XXXXXXX"`
+    echo "$1" > "$TMPFILE"
+    mv "$TMPFILE" "$2"
+}
+
 pager () {
 	local line
 	# Invoke pager only if there's any actual output
@@ -101,7 +107,7 @@ tree_timewarp () {
 	fi
 
 	git-read-tree -m "$branch" || die "$branch: bad commit"
-	[ "$no_head_update" ] || echo "$branch" > $_git/HEAD
+	[ "$no_head_update" ] || echo_to_file "$branch" $_git/HEAD
 
 	# Kill gone files
 	git-diff-tree -z -r $base $branch | xargs -0 bash -c '
diff --git a/cg-branch-add b/cg-branch-add
--- a/cg-branch-add
+++ b/cg-branch-add
@@ -50,4 +50,4 @@ mkdir -p $_git/branches
 [ -s "$_git/branches/$name" ] && die "branch already exists"
 [ -s "$_git/refs/heads/$name" ] && echo "warning: I already have head of this branch" >&2
 
-echo "$location" >$_git/branches/$name
+echo_to_file "$location" $_git/branches/$name
diff --git a/cg-branch-chg b/cg-branch-chg
--- a/cg-branch-chg
+++ b/cg-branch-chg
@@ -14,4 +14,4 @@ location=$2
 ([ "$name" ] && [ "$location" ]) || usage
 
 [ -s "$_git/branches/$name" ] || die "branch does not exist"
-echo "$location" >$_git/branches/$name
+echo_to_file "$location" $_git/branches/$name
diff --git a/cg-commit b/cg-commit
--- a/cg-commit
+++ b/cg-commit
@@ -331,7 +331,7 @@ fi
 
 if [ "$newhead" ]; then
 	echo "Committed as $newhead."
-	echo $newhead >$_git/HEAD
+	echo_to_file $newhead $_git/HEAD
 	[ "$merging" ] && rm $_git/merging $_git/merging-sym $_git/merge-base
 
 	# Trigger the postcommit hook
diff --git a/cg-init b/cg-init
--- a/cg-init
+++ b/cg-init
@@ -44,7 +44,7 @@ mkdir $_git/branches
 touch $_git/refs/heads/master
 
 if [ "$uri" ]; then
-	echo "$uri" >$_git/branches/origin
+	echo_to_file "$uri" $_git/branches/origin
 	cg-pull origin || die "pull failed"
 
 	cp $_git/refs/heads/origin $_git/refs/heads/master
diff --git a/cg-seek b/cg-seek
--- a/cg-seek
+++ b/cg-seek
@@ -44,8 +44,8 @@ fi
 
 if [ "$seek_mode" = "away" ]; then
 	rm $_git/HEAD
-	echo "$dstcommit" >$_git/HEAD
-	[ -s $_git/blocked ] || echo "seeked from $_git_head" >$_git/blocked
+	echo_to_file "$dstcommit" $_git/HEAD
+	[ -s $_git/blocked ] || echo_to_file "seeked from $_git_head" $_git/blocked
 else
 	rm $_git/HEAD
 	ln -s "refs/heads/$_git_head" $_git/HEAD
diff --git a/cg-tag b/cg-tag
--- a/cg-tag
+++ b/cg-tag
@@ -79,5 +79,5 @@ SIGEND
 
 	rm -rf "$tagdir"
 else
-	echo "$id" >$_git/refs/tags/$name
+	echo_to_file "$id" $_git/refs/tags/$name
 fi
diff --git a/git-branch-script b/git-branch-script
--- a/git-branch-script
+++ b/git-branch-script
@@ -8,4 +8,4 @@ rev=$(git-rev-parse --verify --default H
 [ -z "$branchname" ] && die "git branch: I want a branch name"
 [ -e "$GIT_DIR/refs/heads/$branchname" ] && die "$branchname already exists"
 
-echo $rev > "$GIT_DIR/refs/heads/$branchname"
+echo_to_file $rev "$GIT_DIR/refs/heads/$branchname"
diff --git a/git-checkout-script b/git-checkout-script
--- a/git-checkout-script
+++ b/git-checkout-script
@@ -67,7 +67,7 @@ fi
 #
 if [ "$?" -eq 0 ]; then
 	if [ "$newbranch" ]; then
-		echo $new > "$GIT_DIR/refs/heads/$newbranch"
+		echo_to_file $new "$GIT_DIR/refs/heads/$newbranch"
 		branch="$newbranch"
 	fi
 	[ "$branch" ] && ln -sf "refs/heads/$branch" "$GIT_DIR/HEAD"
diff --git a/git-commit-script b/git-commit-script
--- a/git-commit-script
+++ b/git-commit-script
@@ -97,7 +97,7 @@ grep -v '^#' < .editmsg | git-stripspace
 [ -s .cmitmsg ] && 
 	tree=$(git-write-tree) &&
 	commit=$(cat .cmitmsg | git-commit-tree $tree $PARENTS) &&
-	echo $commit > "$GIT_DIR/HEAD" &&
+	echo_to_file $commit "$GIT_DIR/HEAD" &&
 	rm -f -- "$GIT_DIR/MERGE_HEAD"
 ret="$?"
 rm -f .cmitmsg .editmsg
diff --git a/git-fetch-script b/git-fetch-script
--- a/git-fetch-script
+++ b/git-fetch-script
@@ -30,4 +30,4 @@ rsync://*)
 	;;
 esac || exit 1
 git-rev-parse --verify "$head" > /dev/null || exit 1
-echo "$head" > "$GIT_DIR/$destination"
+echo_to_file "$head" "$GIT_DIR/$destination"
diff --git a/git-rebase-script b/git-rebase-script
--- a/git-rebase-script
+++ b/git-rebase-script
@@ -21,7 +21,7 @@ case "$#" in
 esac
 
 git-read-tree -m -u $junio $linus &&
-echo "$linus" >"$GIT_DIR/HEAD" || exit
+echo_to_file "$linus" "$GIT_DIR/HEAD" || exit
 
 tmp=.rebase-tmp$$
 fail=$tmp-fail
diff --git a/git-resolve-script b/git-resolve-script
--- a/git-resolve-script
+++ b/git-resolve-script
@@ -24,8 +24,8 @@ if [ -z "$head" -o -z "$merge" -o -z "$m
 fi
 
 dropheads
-echo $head > "$GIT_DIR"/ORIG_HEAD
-echo $merge > "$GIT_DIR"/LAST_MERGE
+echo_to_file $head "$GIT_DIR"/ORIG_HEAD
+echo_to_file $merge "$GIT_DIR"/LAST_MERGE
 
 common=$(git-merge-base $head $merge)
 if [ -z "$common" ]; then
@@ -40,7 +40,7 @@ fi
 if [ "$common" == "$head" ]; then
 	echo "Updating from $head to $merge."
 	git-read-tree -u -m $head $merge || exit 1
-	echo $merge > "$GIT_DIR"/HEAD
+	echo_to_file $merge "$GIT_DIR"/HEAD
 	git-diff-tree -p $head $merge | git-apply --stat
 	dropheads
 	exit 0
@@ -52,13 +52,13 @@ if [ $? -ne 0 ]; then
 	echo "Simple merge failed, trying Automatic merge"
 	git-merge-cache -o git-merge-one-file-script -a
 	if [ $? -ne 0 ]; then
-		echo $merge > "$GIT_DIR"/MERGE_HEAD
+		echo_to_file $merge "$GIT_DIR"/MERGE_HEAD
 		die "Automatic merge failed, fix up by hand"
 	fi
 	result_tree=$(git-write-tree) || exit 1
 fi
 result_commit=$(echo "$merge_msg" | git-commit-tree $result_tree -p $head -p $merge)
 echo "Committed merge $result_commit"
-echo $result_commit > "$GIT_DIR"/HEAD
+echo_to_file $result_commit "$GIT_DIR"/HEAD
 git-diff-tree -p $head $result_commit | git-apply --stat
 dropheads

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Junio C Hamano]

Chris Wedgwood <cw <at> f00f.org> writes:

>  if [ "$newhead" ]; then
>  	echo "Committed as $newhead."
> -	echo $newhead >$_git/HEAD
> +	echo_to_file $newhead $_git/HEAD
>  	[ "$merging" ] && rm $_git/merging $_git/merging-sym $_git/merge-base

Good intentions, but wouldn't the above clobber symlinked HEAD?

Not a fundamental flaw, though.  You need to see if it is a symlink,
readlink it (repeatedly until you get a regular file or dangling symlink
target that does not exist --- immediately after git-init-db has such a
HEAD) and run your echo_to_file on the link target.

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Chris Wedgwood]

On Tue, Jul 12, 2005 at 09:37:00PM +0000, Junio C Hamano wrote:

> >  if [ "$newhead" ]; then
> >  	echo "Committed as $newhead."
> > -	echo $newhead >$_git/HEAD
> > +	echo_to_file $newhead $_git/HEAD
> >  	[ "$merging" ] && rm $_git/merging $_git/merging-sym $_git/merge-base
>
> Good intentions, but wouldn't the above clobber symlinked HEAD?

Yeah, but I would argue that is the right thing to do.

Symlink'd trees don't really make sense to me (they seem fragile and
somewhat pointless) but perhaps I'm missing something?

> Not a fundamental flaw, though.  You need to see if it is a symlink,
> readlink it (repeatedly until you get a regular file or dangling
> symlink target that does not exist --- immediately after git-init-db
> has such a HEAD) and run your echo_to_file on the link target.

I don't see how that's helpful.  The idea of echo_to_file is to make
the metadata self-contained to the subtree --- so we don't want to
have external links (possibly) pointing outside of it.

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Matthias Urlichs]

Hi, Chris Wedgwood wrote:

> Symlink'd trees don't really make sense to me (they seem fragile and
> somewhat pointless) but perhaps I'm missing something?

You are ;-)  the tree itsels is no symlinked, but HEAD points to
refs/heads/<branch> by default.

Don't clobber that, please.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
Though many hands make light work, too many cooks spoil the broth.

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Chris Wedgwood]

On Wed, Jul 13, 2005 at 09:03:26AM +0200, Matthias Urlichs wrote:

> You are ;-) the tree itsels is no symlinked, but HEAD points to
> refs/heads/<branch> by default.

Thanks for pointing that out.  I honestly never noticed that.

How about the following?

    echo_to_file() {
	local DEST="$2"
	local count=1
	local RET

	# follow symlinks until we run out or end up with something
	# dangling
	while [ -L "$DEST" ] ; do
	    local NLINK=`readlink "$DEST"`

	    if [ ! -e "$NLINK" ] ; then
		# dangling link, just poke as-is
		echo "$1" > "$DEST"
		return $?
	    fi

	    # follow link
	    if [ -L "$NLINK" ] ; then
		count=$(("$count" + 1))
		if [ "$count" -gt 5 ] ; then
		    echo "Too many symlinks when resolving $2" 1>&2
		    return 1
		fi
		DEST="$NLINK"
	    fi
	done

	# end up with a regular file
	local TMPFILE=`mktemp "$DEST.tmp-XXXXXXX"`
	echo "$1" > "$TMPFILE"
	RET="$?"
	mv -f "$TMPFILE" "$DEST"
	return "$RET"
    }


Obvious changes from last time are:

  - mark things local

  - follow links, bailing out if it takes too long

  - if the link is dangling, just use as-is

  - when doing the 'replace' at the end use -f because it's quite
    possible someone will have done cp -Rl ~otheruser/tree here and mv
    *might* bitch if the UID is different?

  - return (some) errors (just because, it's not like cogito checks
    for these)

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Linus Torvalds]

On Wed, 13 Jul 2005, Chris Wedgwood wrote:

> On Wed, Jul 13, 2005 at 09:03:26AM +0200, Matthias Urlichs wrote:
> 
> > You are ;-) the tree itsels is no symlinked, but HEAD points to
> > refs/heads/<branch> by default.
> 
> Thanks for pointing that out.  I honestly never noticed that.
> 
> How about the following?

This is really complicated, for no good reason.

The _object_ directories should be linked, but it's really wrong to link 
the "refs/" directories and expect them to have COW behaviour.

I've tried to make most of the git tools write the refs objects "safely" 
too, ie things like "git-receive-pack" (the receiving end of a 
"git-send-pack") will write the new ref to a lock-file and then do a 
"rename()" to set it. That is COW-safe, but the thing is, it's incredibly 
painful for many other operations, and I won't guarantee that git in 
general is always going to be COW-safe wrt all the git files.

For example, the "git clone -l" behaviour will _only_ link the objects 
subdirectory. The rest is copied. Unless there's a bug there somewhere.

		Linus

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Chris Wedgwood]

On Wed, Jul 13, 2005 at 01:05:02PM -0700, Linus Torvalds wrote:

> This is really complicated, for no good reason.

What should I be doing?

> The _object_ directories should be linked, but it's really wrong to link 
> the "refs/" directories and expect them to have COW behaviour.

I'm confused.

This function in only used to update various files, one of which is
.git/HEAD

The complexity I added was to deal with a situation where we have

  tree1/
	.git/HEAD -> refs/head/master

and I do "cp -Rl tree1 tree2" giving me:

  tree2/
	.git/HEAD -> refs/head/master

  and .git/refs/head/master is hardlinked between both trees.

Cogito will do "echo <foo> > .git/HEAD" which update *both* tree1 and
tree2 which is not what I want.  So the idea is to readlink and do the
update using echo + rename to get the CoW semantics I desire.


Yes, weird things happen if you have out-of-treel links but I suspect
if you do that with cogito you've probably lost anyhow.

> I've tried to make most of the git tools write the refs objects
> "safely" too, ie things like "git-receive-pack" (the receiving end
> of a "git-send-pack") will write the new ref to a lock-file and then
> do a "rename()" to set it.

Yes, git is probably fine.  Cogito isn't.

> That is COW-safe, but the thing is, it's incredibly painful for many
> other operations, and I won't guarantee that git in general is
> always going to be COW-safe wrt all the git files.

What operations is this painful for?

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Linus Torvalds]

On Wed, 13 Jul 2005, Chris Wedgwood wrote:
> 
> The complexity I added was to deal with a situation where we have
> 
>   tree1/
> 	.git/HEAD -> refs/head/master
> 
> and I do "cp -Rl tree1 tree2" giving me:
> 
>   tree2/
> 	.git/HEAD -> refs/head/master
> 
>   and .git/refs/head/master is hardlinked between both trees.

AND THAT IS WRONG.

You shouldn't hardlink whole trees. Git will not guarantee that it breaks 
the links.

Right now, if you go a "git commit", and a number of other core git 
operations, it will use ">" to overwrite the HEAD. You're screwed.

> > That is COW-safe, but the thing is, it's incredibly painful for many
> > other operations, and I won't guarantee that git in general is
> > always going to be COW-safe wrt all the git files.
> 
> What operations is this painful for?

Your script was tens of lines of code, and it was BUGGY.

And now you ask what it's painful for?

Guys, if you want to mess up code, the #1 rule is:
 - make complex code to handle insane situations

You're well on your way to do that.

Don't hardlink git trees. It's a bug to do so. It will eventually change 
the other git tree.

Hardlink git _object_ directories. That's a totally different thing.

		Linus

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Chris Wedgwood]

On Wed, Jul 13, 2005 at 02:05:35PM -0700, Linus Torvalds wrote:

> >   and .git/refs/head/master is hardlinked between both trees.
>
> AND THAT IS WRONG.

OK, I was more-or-less assuming that 'cp -Rl tree1 tree2' was always a
valid thing to do.

Clearly if it's not then all of this is somewhat moot.

> You shouldn't hardlink whole trees. Git will not guarantee that it
> breaks the links.

I can live with that, until now though I wasn't aware that hardlinked
trees were a bad-thing.

> Your script was tens of lines of code, and it was BUGGY.

Yeah well, I'm a retard.

> Hardlink git _object_ directories. That's a totally different thing.

I was under the misconception that hardlink entire trees was
permissible.

I would however like to be able to hardlink more than just the
_object_ directory --- hardlinking the source is quite nice too.
Might that be considered safe?  (I'm of course assuming that editors
do write + rename when saving their buffers).

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Linus Torvalds]

On Wed, 13 Jul 2005, Chris Wedgwood wrote:
> 
> I would however like to be able to hardlink more than just the
> _object_ directory --- hardlinking the source is quite nice too.
> Might that be considered safe?  (I'm of course assuming that editors
> do write + rename when saving their buffers).

Hmm.. I don't think it's necessarily wrong, although as you say, your
editor had better DTRT. That said, even if your editor doesn't, at least
you won't corrupt your git archive, although you might have surprising
changes creep into the other side..

But I've not verified that every git program does the right thing...

Search-search-search.. Indeed: at least "git-apply" seems to modify the
file in place. That may be the only one, though - and should be easily
fixed by adding an "unlink()" (and make it use O_EXCL just to make it 
easy to grep to verify that we always use O_EXCL with O_WR.*).

entry.c should also add O_EXCL (it seems to already do the unlink).

Anybody want to go through and verify it? I think we should support 
hardlinked source trees if we can do so easily, and it _looks_ like we're 
pretty close..

		Linus

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Chris Wedgwood]

On Wed, Jul 13, 2005 at 02:33:43PM -0700, Linus Torvalds wrote:

> Hmm.. I don't think it's necessarily wrong, although as you say,
> your editor had better DTRT.

It does.  I assume probably everything does but I never really
checked.

> That said, even if your editor doesn't, at least you won't corrupt
> your git archive, although you might have surprising changes creep
> into the other side..

Well, with hard-linked trees and the nastyness I posted I would test
with

	cp -Rl tree1 tree2
	cd tree2
	<hack hack hack>
	find ../tree1 -mmin -<n> ... # make sure we didn't mess up original tree

So far this seems to work for me.

> Search-search-search.. Indeed: at least "git-apply" seems to modify
> the file in place.

I probably just luckily never hit this.

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Matthias Urlichs]

Hi,

Chris Wedgwood:
> How about the following?
> 
Ummm...

> 	    local NLINK=`readlink "$DEST"`
> 
> 	    if [ ! -e "$NLINK" ] ; then

You lose if the link is relative and the symlink is not in the current
directory. You also lose on systems where the empty filename is
synonymous with the current directory.

You'd need to do something along the lines of

	if [ -n "$NLINK" ] ; then
		case "$NLINK" in
		/*) ;;
		*) NLINK="$(dirname "$DEST")/$NLINK" ;;
		esac
	fi

first.

> 		# dangling link, just poke as-is
> 		echo "$1" > "$DEST"

You should remove "DEST first. Otherwise, under Linux, you'll magically
create the file the symlink points to, which may not be what you want to
do.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
Custom does often reason overrule And only serves for reason to the fool.
					-- John Wilmot, Earl of Rochester

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Chris Wedgwood]

On Wed, Jul 13, 2005 at 10:07:24PM +0200, Matthias Urlichs wrote:

> You lose if the link is relative and the symlink is not in the
> current directory.

Cogito doesn't create such links in my (limited_ experience.  Why
would anyone else do that?

> You also lose on systems where the empty filename is synonymous with
> the current directory.

Well, return code from readlink should be tested there.


[...]

> You should remove "DEST first. Otherwise, under Linux, you'll
> magically create the file the symlink points to, which may not be
> what you want to do.

That behavior *is* desirable in this case.


Anyhow, the fact this is so complicated and getting more so makes me
think it's misdirected :-(

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Linus Torvalds]

On Wed, 13 Jul 2005, Chris Wedgwood wrote:
> 
> How about the following?
> 
>     echo_to_file() {
> 	local DEST="$2"
> 	local count=1
> 	local RET
> 
> 	# follow symlinks until we run out or end up with something
> 	# dangling
> 	while [ -L "$DEST" ] ; do

If you really want to do something like this, then you should do it like 
this:

	update_head() {
		head="$GIT_DIR/HEAD"
		newvalue=$(git-rev-parse --verify "$1"^0) || exit
		if [ -L "$head" ]; then
			head=$(readlink "$head")
			case "$head" in
			refs/heads/*)
				head="$GIT_DIR"/"$head"
				;;
			*)
				exit 1
				;;
			esac
		fi
		echo $newvalue > "$head.lock" && rename "$head.lock" "$head"
	}

which is at least slightly simpler, and might even work thanks to that.

Your was buggy for several reasons:

 - following multiple links is _wrong_, since the next-level link could 
   actually be a symlink to another tree entirely (if somebody is crazy 
   enough to use "cp -Rl" to copy trees, then why not "cp -Rs"?)

   Ergo: you should only follow a symlink if it points to refs/heads, and 
   anything else would be a BUG.

 - you got relative links wrong.

Now, admittedly the above is totally untested too, so I'll probably have 
typos etc in there. But the basic point stands: don't go for complexity. 
Go for one _particular_ case.

		Linus

Re: [RFC PATCH] cogito --- don't overwrite metadata files in place (breaks CoW use)

[Chris Wedgwood]

On Wed, Jul 13, 2005 at 02:23:18PM -0700, Linus Torvalds wrote:

> If you really want to do something like this, then you should do it like
> this:
>
> 	update_head() {

except there are a number of places I need to frob things in Cogito
and they are not all for the head.  If the head is special and the
only candidate sure, that works for me.

>  - following multiple links is _wrong_, since the next-level link could
>    actually be a symlink to another tree entirely (if somebody is crazy
>    enough to use "cp -Rl" to copy trees, then why not "cp -Rs"?)

I already stated that wasn't supposed to work.  If you have symlinks
outside of your tree with cogito I think there is a *lot* more code
that probably needs to be checked.

Remember I used cogito here, not git directly.  That probably means
things are invoked very differently for me?

>  - you got relative links wrong.

Possibly, I was more for RFC/concept reasons I posted the patch.  I'm
clueless when it comes to writing decent or reliable bash sh/bash
scripts.