Add "git-update-ref" to update the HEAD (or other) ref

Add "git-update-ref" to update the HEAD (or other) ref

[Linus Torvalds]

This is a careful version of the script stuff that currently just
blindly writes HEAD with a new value.

You can use

	git-update-ref HEAD <newhead>

or

	git-update-ref HEAD <newhead> <oldhead>

where the latter version verifies that the old value of HEAD matches
oldhead.

It basically allows a "ref" file to be a symbolic pointer to another ref
file by starting with the four-byte header sequence of "ref:".

More importantly, it allows the update of a ref file to follow these 
symbolic pointers, whether they are symlinks or these "regular file 
symbolic refs".

NOTE! It follows _real_ symlinks only if they start with "refs/": 
otherwise it will just try to read them and update them as a regular file 
(ie it will allow the filesystem to follow them, but will overwrite such a 
symlink to somewhere else with a regular filename).

In general, using

	git-update-ref HEAD "$head"

should be a _lot_ safer than doing

	echo "$head" > "$GIT_DIR/HEAD"

both from a symlink following standpoint _and_ an error checking
standpoint.  The "refs/" rule for symlinks means that symlinks that point
to "outside" the tree are safe: they'll be followed for reading but not 
for writing (so we'll never write through a ref symlink to some other 
tree, if you have copied a whole archive by creating a symlink tree).

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

This is independent of my previous patch, but in the same basic series. It 
is useful regardless of whether you use the new symbolic refs or not 
because of the much improved error checking.

diff --git a/Makefile b/Makefile
--- a/Makefile
+++ b/Makefile
@@ -114,6 +114,7 @@ PROGRAMS = \
 	git-ssh-upload git-tar-tree git-unpack-file \
 	git-unpack-objects git-update-index git-update-server-info \
 	git-upload-pack git-verify-pack git-write-tree \
+	git-update-ref \
 	$(SIMPLE_PROGRAMS)
 
 # Backward compatibility -- to be removed in 0.99.8
diff --git a/update-ref.c b/update-ref.c
new file mode 100644
--- /dev/null
+++ b/update-ref.c
@@ -0,0 +1,124 @@
+#include "cache.h"
+#include "refs.h"
+
+static const char git_update_ref_usage[] = "git-update-ref <refname> <value> [<oldval>]";
+
+#define MAXDEPTH 5
+
+const char *resolve_ref(const char *path, unsigned char *sha1)
+{
+	int depth = MAXDEPTH, len;
+	char buffer[256];
+
+	for (;;) {
+		struct stat st;
+		char *buf;
+		int fd;
+
+		if (--depth < 0)
+			return NULL;
+
+		/* Special case: non-existing file */
+		if (lstat(path, &st) < 0) {
+			if (errno != ENOENT)
+				return NULL;
+			memset(sha1, 0, 20);
+			return path;
+		}
+
+		/* Follow "normalized" - ie "refs/.." symlinks by hand */
+		if (S_ISLNK(st.st_mode)) {
+			len = readlink(path, buffer, sizeof(buffer)-1);
+			if (len >= 5 && !memcmp("refs/", buffer, 5)) {
+				path = git_path("%.*s", len, buffer);
+				continue;
+			}
+		}
+
+		/*
+		 * Anything else, just open it and try to use it as
+		 * a ref
+		 */
+		fd = open(path, O_RDONLY);
+		if (fd < 0)
+			return NULL;
+		len = read(fd, buffer, sizeof(buffer)-1);
+		close(fd);
+
+		/*
+		 * Is it a symbolic ref?
+		 */
+		if (len < 4 || memcmp("ref:", buffer, 4))
+			break;
+		buf = buffer + 4;
+		len -= 4;
+		while (len && isspace(*buf))
+			buf++, len--;
+		while (len && isspace(buf[len-1]))
+			buf[--len] = 0;
+		path = git_path("%.*s", len, buf);
+	}
+	if (len < 40 || get_sha1_hex(buffer, sha1))
+		return NULL;
+	return path;
+}
+
+int main(int argc, char **argv)
+{
+	char *hex;
+	const char *refname, *value, *oldval, *path, *lockpath;
+	unsigned char sha1[20], oldsha1[20], currsha1[20];
+	int fd, written;
+
+	setup_git_directory();
+	if (argc < 3 || argc > 4)
+		usage(git_update_ref_usage);
+
+	refname = argv[1];
+	value = argv[2];
+	oldval = argv[3];
+	if (get_sha1(value, sha1) < 0)
+		die("%s: not a valid SHA1", value);
+	memset(oldsha1, 0, 20);
+	if (oldval && get_sha1(oldval, oldsha1) < 0)
+		die("%s: not a valid old SHA1", oldval);
+
+	path = resolve_ref(git_path("%s", refname), currsha1);
+	if (!path)
+		die("No such ref: %s", refname);
+
+	if (oldval) {
+		if (memcmp(currsha1, oldsha1, 20))
+			die("Ref %s changed to %s", refname, sha1_to_hex(currsha1));
+		/* Nothing to do? */
+		if (!memcmp(oldsha1, sha1, 20))
+			exit(0);
+	}
+	path = strdup(path);
+	lockpath = mkpath("%s.lock", path);
+
+	fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
+	if (fd < 0)
+		die("Unable to create %s", lockpath);
+	hex = sha1_to_hex(sha1);
+	hex[40] = '\n';
+	written = write(fd, hex, 41);
+	close(fd);
+	if (written != 41) {
+		unlink(lockpath);
+		die("Unable to write to %s", lockpath);
+	}
+		
+	/*
+	 * FIXME!
+	 *
+	 * We should re-read the old ref here, and re-verify that it
+	 * matches "oldsha1". Otherwise there's a small race.
+	 */
+
+	if (rename(lockpath, path) < 0) {
+		unlink(lockpath);
+		die("Unable to create %s", path);
+	}
+	return 0;
+}

Re: Add "git-update-ref" to update the HEAD (or other) ref

[Linus Torvalds]

On Sun, 25 Sep 2005, Linus Torvalds wrote:
>
> This is a careful version of the script stuff that currently just
> blindly writes HEAD with a new value.

Btw, in case it wasn't clear from the patch, this only introduced 
mechanism, it didn't actually use it in any script.

But the point of the exercise is to move towards using git-update-ref in 
places like git-fetch.sh, which currently are doing some of it by hand 
(and not handling symlinks etc).

So you should be able to do something like

	..
	old=$(git-rev-parse --verify "$refname^0") >& /dev/null
	if [ "$old" ]; then
		mb=$(git-merge-base $new $old)
		if [ "$mb" != "$old" ]; then
			echo "$new is not a fast-forward of $old"
			[ "$force" ] || exit 1
			old=
		fi
	fi
	git-update-ref "$refname" $new $old

and the actual update phase will re-verify that "old" is still valid (if 
it exists at all).

Doing it by hand works, of course, but for example, if we have two
symlinks pointing to the same ref, the current locking in git-fetch.sh is
broken - it may lock the _symlink_, but since the other one _also_ points 
to the same thing, there's no locking of the _target_.

git-update-ref should do things like that right. Famous last words ;)

			Linus

Re: Add "git-update-ref" to update the HEAD (or other) ref

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Btw, in case it wasn't clear from the patch, this only introduced 
> mechanism, it didn't actually use it in any script.

No I understood it perfectly well thanks -- I am just slow
because I am somewhat sick today.

Re: Add "git-update-ref" to update the HEAD (or other) ref

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> +	 * FIXME!

Is something like the one at the end acceptable?

I'd like to take these patches in two stages (I am not asking
you for a resend):

 - Drop the emulated symlink part from the update-ref.c; have it
   graduate to "master" branch and use it in existing scripts.

 - Take the read_ref() change, along with a patch to re-add the
   emulated symlink part to update-ref.c (after making its
   interpretation to match that of read_ref() -- which requires
   the prefix to be exactly "ref: " five bytes); keep it in "pu"
   branch a bit longer.


---
diff --git a/update-ref.c b/update-ref.c
--- a/update-ref.c
+++ b/update-ref.c
@@ -97,11 +97,13 @@ int main(int argc, char **argv)
 	}
 
 	/*
-	 * FIXME!
-	 *
-	 * We should re-read the old ref here, and re-verify that it
+	 * We re-read the old ref here, and re-verify that it
 	 * matches "oldsha1". Otherwise there's a small race.
 	 */
+	if (!resolve_ref(git_path("%s", refname), oldsha1))
+		die("Cannot verify ref: %s", refname); 
+	if (memcmp(oldsha1, currsha1, 20))
+		die("Ref %s changed to %s", refname, sha1_to_hex(oldsha1));
 
 	if (rename(lockpath, path) < 0) {
 		unlink(lockpath);

Re: Add "git-update-ref" to update the HEAD (or other) ref

[Linus Torvalds]

On Sun, 25 Sep 2005, Junio C Hamano wrote:
> 
> I'd like to take these patches in two stages (I am not asking
> you for a resend):
> 
>  - Drop the emulated symlink part from the update-ref.c; have it
>    graduate to "master" branch and use it in existing scripts.

Sure.

>  - Take the read_ref() change, along with a patch to re-add the
>    emulated symlink part to update-ref.c (after making its
>    interpretation to match that of read_ref() -- which requires
>    the prefix to be exactly "ref: " five bytes); keep it in "pu"
>    branch a bit longer.

I was actually thinking of maybe entirely replacing "read_ref()" with the
more powerful "resolve_ref()" - moving resolve_ref() into refs.c.

That way there's only one place that knows about the "ref:" thing.

But yes, forcing the format to be "ref: " instead of "ref:<whitespace>*" 
sounds fine.

		Linus

Re: Add "git-update-ref" to update the HEAD (or other) ref

[Linus Torvalds]

On Sun, 25 Sep 2005, Junio C Hamano wrote:
> 
> Is something like the one at the end acceptable?

Looking at the patch closer, no, that's incorrect.

"oldsha" doesn't necessarily exist, since there has to be some way to 
force the new one. So if "oldval" is NULL, we shouldn't re-verify 
anything.

Also, independently of that your patch is buggy because calling
"resolve_ref()" again will overwrite the lockpath, since it's re-used by
the static buffer in git_path(). That's why the "strdup()" is there.

Yeah, yeah, static buffers are evil, but they are also simple and 
efficient. 

But something like this (on top of my original one) might work.

		Linus
----
diff --git a/update-ref.c b/update-ref.c
--- a/update-ref.c
+++ b/update-ref.c
@@ -63,6 +63,19 @@ const char *resolve_ref(const char *path
 	return path;
 }
 
+static int re_verify(const char *path, unsigned char *oldsha1, unsigned char *currsha1)
+{
+	char buf[40];
+	int fd = open(path, O_RDONLY), nr;
+	if (fd < 0)
+		return -1;
+	nr = read(fd, buf, 40);
+	close(fd);
+	if (nr != 40 || get_sha1_hex(buf, currsha1) < 0)
+		return -1;
+	return memcmp(oldsha1, currsha1, 20) ? -1 : 0;
+}
+
 int main(int argc, char **argv)
 {
 	char *hex;
@@ -108,14 +121,18 @@ int main(int argc, char **argv)
 		unlink(lockpath);
 		die("Unable to write to %s", lockpath);
 	}
-		
+
 	/*
-	 * FIXME!
-	 *
-	 * We should re-read the old ref here, and re-verify that it
-	 * matches "oldsha1". Otherwise there's a small race.
+	 * Re-read the ref after getting the lock to verify
 	 */
+	if (oldval && re_verify(path, oldsha1, currsha1) < 0) {
+		unlink(lockpath);
+		die("Ref lock failed");
+	}
 
+	/*
+	 * Finally, replace the old ref with the new one
+	 */
 	if (rename(lockpath, path) < 0) {
 		unlink(lockpath);
 		die("Unable to create %s", path);

Re: Add "git-update-ref" to update the HEAD (or other) ref

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> I was actually thinking of maybe entirely replacing "read_ref()" with the
> more powerful "resolve_ref()" - moving resolve_ref() into refs.c.
>
> That way there's only one place that knows about the "ref:" thing.

That would make sense.  But I am feeling a bit too weak tonight
and am going to crash now.

[PATCH] Use git-update-ref in scripts.

[Junio C Hamano]

This uses the git-update-ref command in scripts for safer updates.
Also places where we used to read HEAD ref by using "cat" were fixed
to use git-rev-parse.  This will matter when we start using symbolic
references.

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

---

 * Requesting extra sets of eyeballs from the list for stupid
   mistakes.

 git-applypatch.sh |    5 +++--
 git-commit.sh     |    4 +++-
 git-fetch.sh      |   34 ++++++++++++++++++----------------
 git-merge.sh      |    9 +++++----
 git-octopus.sh    |    2 +-
 git-pull.sh       |    6 +++---
 git-rebase.sh     |    5 +++--
 git-reset.sh      |    2 +-
 git-resolve.sh    |    4 ++--
 9 files changed, 39 insertions(+), 32 deletions(-)

7bae83d5754a1afb8e64f9de17f1dc34d0022f0a
diff --git a/git-applypatch.sh b/git-applypatch.sh
--- a/git-applypatch.sh
+++ b/git-applypatch.sh
@@ -108,9 +108,10 @@ fi
 
 tree=$(git-write-tree) || exit 1
 echo Wrote tree $tree
-commit=$(git-commit-tree $tree -p $(cat "$GIT_DIR"/HEAD) < "$final") || exit 1
+parent=$(git-rev-parse --verify HEAD) &&
+commit=$(git-commit-tree $tree -p $parent <"$final") || exit 1
 echo Committed: $commit
-echo $commit > "$GIT_DIR"/HEAD
+git-update-ref HEAD $commit $parent || exit
 
 if test -x "$GIT_DIR"/hooks/post-applypatch
 then
diff --git a/git-commit.sh b/git-commit.sh
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -159,7 +159,9 @@ if [ ! -r "$GIT_DIR/HEAD" ]; then
 		exit 1
 	fi
 	PARENTS=""
+	current=
 else
+	current=$(git-rev-parse --verify HEAD)
 	if [ -f "$GIT_DIR/MERGE_HEAD" ]; then
 		PARENTS="-p HEAD "`sed -e 's/^/-p /' "$GIT_DIR/MERGE_HEAD"`
 	fi
@@ -220,7 +222,7 @@ if test -s .cmitchk
 then
 	tree=$(git-write-tree) &&
 	commit=$(cat .cmitmsg | git-commit-tree $tree $PARENTS) &&
-	echo $commit > "$GIT_DIR/HEAD" &&
+	git-update-ref HEAD $commit $current &&
 	rm -f -- "$GIT_DIR/MERGE_HEAD"
 else
 	echo >&2 "* no commit message?  aborting commit."
diff --git a/git-fetch.sh b/git-fetch.sh
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -105,14 +105,16 @@ fast_forward_local () {
 	else
 		echo >&2 "* $1: storing $3"
 	fi
-	echo "$2" >"$GIT_DIR/$1" ;;
+	git-update-ref "$1" "$2" 
+	;;
 
     refs/heads/*)
-	# NEEDSWORK: use the same cmpxchg protocol here.
-	echo "$2" >"$GIT_DIR/$1.lock"
-	if test -f "$GIT_DIR/$1"
+	# $1 is the ref being updated.
+	# $2 is the new value for the ref.
+	local=$(git-rev-parse --verify "$1^0" 2>/dev/null)
+	if test "$local"
 	then
-	    local=$(git-rev-parse --verify "$1^0") &&
+	    # Require fast-forward.
 	    mb=$(git-merge-base "$local" "$2") &&
 	    case "$2,$mb" in
 	    $local,*)
@@ -120,34 +122,34 @@ fast_forward_local () {
 		;;
 	    *,$local)
 		echo >&2 "* $1: fast forward to $3"
+		git-update-ref "$1" "$2" "$local"
 		;;
 	    *)
 		false
 		;;
 	    esac || {
 		echo >&2 "* $1: does not fast forward to $3;"
-		case "$force,$single_force" in
-		t,* | *,t)
+		case ",$force,$single_force," in
+		*,t,*)
 			echo >&2 "  forcing update."
+			git-update-ref "$1" "$2" "$local"
 			;;
 		*)
-			mv "$GIT_DIR/$1.lock" "$GIT_DIR/$1.remote"
-			echo >&2 "  leaving it in '$1.remote'"
+			echo >&2 "  not updating."
 			;;
 		esac
 	    }
 	else
-		echo >&2 "* $1: storing $3"
+	    echo >&2 "* $1: storing $3"
+	    git-update-ref "$1" "$2"
 	fi
-	test -f "$GIT_DIR/$1.lock" &&
-	    mv "$GIT_DIR/$1.lock" "$GIT_DIR/$1"
 	;;
     esac
 }
 
 case "$update_head_ok" in
 '')
-	orig_head=$(cat "$GIT_DIR/HEAD" 2>/dev/null)
+	orig_head=$(git-rev-parse --verify HEAD 2>/dev/null)
 	;;
 esac
 
@@ -184,7 +186,7 @@ do
     rsync://*)
 	TMP_HEAD="$GIT_DIR/TMP_HEAD"
 	rsync -L -q "$remote/$remote_name" "$TMP_HEAD" || exit 1
-	head=$(git-rev-parse TMP_HEAD)
+	head=$(git-rev-parse --verify TMP_HEAD)
 	rm -f "$TMP_HEAD"
 	test "$rsync_slurped_objects" || {
 	    rsync -av --ignore-existing --exclude info \
@@ -261,10 +263,10 @@ case ",$update_head_ok,$orig_head," in
 *,, | t,* )
 	;;
 *)
-	curr_head=$(cat "$GIT_DIR/HEAD" 2>/dev/null)
+	curr_head=$(git-rev-parse --verify HEAD 2>/dev/null)
 	if test "$curr_head" != "$orig_head"
 	then
-		echo "$orig_head" >$GIT_DIR/HEAD
+	    	git-update-ref HEAD "$orig_head"
 		die "Cannot fetch into the current branch."
 	fi
 	;;
diff --git a/git-merge.sh b/git-merge.sh
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -114,8 +114,9 @@ case "$#,$common" in
 	# Again the most common case of merging one remote.
 	echo "Updating from $head to $1."
 	git-update-index --refresh 2>/dev/null
-	git-read-tree -u -m $head "$1" || exit 1
-	git-rev-parse --verify "$1^0" > "$GIT_DIR/HEAD"
+	git-read-tree -u -m $head "$1" &&
+	new_head=$(git-rev-parse --verify "$1^0") &&
+	git-update-ref HEAD "$new_head" "$head" || exit 1
 	summary "$1"
 	dropsave
 	exit 0
@@ -218,9 +219,9 @@ then
     do
         parents="$parents -p $remote"
     done
-    result_commit=$(echo "$merge_msg" | git-commit-tree $result_tree $parents)
+    result_commit=$(echo "$merge_msg" | git-commit-tree $result_tree $parents) || exit
     echo "Committed merge $result_commit, made by $wt_strategy."
-    echo $result_commit >"$GIT_DIR/HEAD"
+    git-update-ref HEAD $result_commit $head
     summary $result_commit
     dropsave
     exit 0
diff --git a/git-octopus.sh b/git-octopus.sh
--- a/git-octopus.sh
+++ b/git-octopus.sh
@@ -86,5 +86,5 @@ esac
 result_commit=$(git-fmt-merge-msg <"$GIT_DIR/FETCH_HEAD" |
 		git-commit-tree $MRT $PARENT)
 echo "Committed merge $result_commit"
-echo $result_commit >"$GIT_DIR"/HEAD
+git-update-ref HEAD $result_commit $head
 git-diff-tree -p $head $result_commit | git-apply --stat
diff --git a/git-pull.sh b/git-pull.sh
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -6,10 +6,10 @@
 
 . git-sh-setup || die "Not a git archive"
 
-orig_head=$(cat "$GIT_DIR/HEAD") || die "Pulling into a black hole?"
+orig_head=$(git-rev-parse --verify HEAD) || die "Pulling into a black hole?"
 git-fetch --update-head-ok "$@" || exit 1
 
-curr_head=$(cat "$GIT_DIR/HEAD")
+curr_head=$(git-rev-parse --verify HEAD)
 if test "$curr_head" != "$orig_head"
 then
 	# The fetch involved updating the current branch.
@@ -38,4 +38,4 @@ case "$merge_head" in
 esac
 
 merge_name=$(git-fmt-merge-msg <"$GIT_DIR/FETCH_HEAD")
-git-resolve "$(cat "$GIT_DIR"/HEAD)" $merge_head "$merge_name"
+git-resolve "$curr_head" $merge_head "$merge_name"
diff --git a/git-rebase.sh b/git-rebase.sh
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -33,7 +33,8 @@ test "$different1$different2" = "" ||
 die "Your working tree does not match $ours_symbolic."
 
 git-read-tree -m -u $ours $upstream &&
-git-rev-parse --verify "$upstream^0" >"$GIT_DIR/HEAD" || exit
+new_head=$(git-rev-parse --verify "$upstream^0") &&
+git-update-ref HEAD "$new_head" || exit
 
 tmp=.rebase-tmp$$
 fail=$tmp-fail
@@ -50,7 +51,7 @@ do
 		continue ;;
 	esac
 	echo >&2 "* Applying: $msg"
-	S=`cat "$GIT_DIR/HEAD"` &&
+	S=$(git-rev-parse --verify HEAD) &&
 	git-cherry-pick --replay $commit || {
 		echo >&2 "* Not applying the patch and continuing."
 		echo $commit >>$fail
diff --git a/git-reset.sh b/git-reset.sh
--- a/git-reset.sh
+++ b/git-reset.sh
@@ -60,7 +60,7 @@ then
 else
 	rm -f "$GIT_DIR/ORIG_HEAD"
 fi
-echo "$rev" >"$GIT_DIR/HEAD"
+git-update-ref HEAD "$rev"
 
 case "$reset_type" in
 --hard )
diff --git a/git-resolve.sh b/git-resolve.sh
--- a/git-resolve.sh
+++ b/git-resolve.sh
@@ -45,7 +45,7 @@ case "$common" in
 "$head")
 	echo "Updating from $head to $merge."
 	git-read-tree -u -m $head $merge || exit 1
-	echo $merge > "$GIT_DIR"/HEAD
+	git-update-ref HEAD "$merge"
 	git-diff-tree -p $head $merge | git-apply --stat
 	dropheads
 	exit 0
@@ -99,6 +99,6 @@ if [ $? -ne 0 ]; then
 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
+git-update-ref HEAD "$result_commit"
 git-diff-tree -p $head $result_commit | git-apply --stat
 dropheads

Re: [PATCH] Use git-update-ref in scripts.

[Linus Torvalds]

On Tue, 27 Sep 2005, Junio C Hamano wrote:
>
> This uses the git-update-ref command in scripts for safer updates.

Looks good.

git-resolve might want to verify the old head. On the other hand, it looks 
like it's being phased out, so maybe nobody cares?

		Linus

Re: [PATCH] Use git-update-ref in scripts.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> git-resolve might want to verify the old head. On the other hand, it looks 
> like it's being phased out, so maybe nobody cares?

It was my mistake -- git-merge does it, and I should do the
same in git-resolve.  Thanks for pointing it out.

Have you had a chance to look at the git-merge change to remove
the stupid clean-tree requirements?  I have been planning to
inflict the 'use git-merge instead of git-resolve' change on you
sometime soonish (like today ;-).  Having said that, I myself
would vote against phasing out 'git-resolve' -- being able to
say 'git resolve master hold fast' to fast forward the master
head to topic branch head of 'hold' (my topic branches are often
rebased to allow this) is quite useful.

I have one unrelated request.

Could I have a copy of .git/{branches,remotes,refs}/* from the
primary repository you do your kernel work please?

Re: [PATCH] Use git-update-ref in scripts.

[Linus Torvalds]

On Wed, 28 Sep 2005, Junio C Hamano wrote:
> 
> Have you had a chance to look at the git-merge change to remove
> the stupid clean-tree requirements?  I have been planning to
> inflict the 'use git-merge instead of git-resolve' change on you
> sometime soonish (like today ;-).

I don't like doing the diff before-hand, but it looked like the default 
was to try just one strategy, and avoid the diff in that case.

Actually, my preference would be to have a unconditional simple case
first. If there's only one possible base, and the trivial merge succeeds
(ie no three-way merges needed at all, just a single git-read-tree), do
that part unconditionally.

That actually matches 90% of all merges I do, and I'd be much happier with 
git-merge if it did that first and if it then does something more complex 
(including diffs etc) afterwards, I'm much less likely to worry.

> Could I have a copy of .git/{branches,remotes,refs}/* from the
> primary repository you do your kernel work please?

Heh. My kernel has none of that. Well, it obviously has refs, but even 
there it literally has just one head: "master". The rest are the standard 
tags you see in public.

So if you clone the public kernel,. you'll actually have a superset of 
what I have, since you'll have the "origin" thing ;)

			Linus

Re: [PATCH] Use git-update-ref in scripts.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Heh. My kernel has none of that. Well, it obviously has refs, but even 
> there it literally has just one head: "master". The rest are the standard 
> tags you see in public.

I was wondering the URL shorthand you mentioned is being used in
practice.  In theory, you do not "have to write out in full"
anymore ;-).

Message-ID: <Pine.LNX.4.58.0507151529590.19183@g5.osdl.org>
From: Linus Torvalds <torvalds@osdl.org>
Date: Fri, 15 Jul 2005 15:42:42 -0700 (PDT)

And it's not necessarily just the branch handling, but more of a generic
shorthand: I'd love to be able to mix something like

	git pull jgarzik/misc-2.6 upstream

and "jgarzik" would be expanded (through something like .git/branches) to 
"master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/"), resulting in the 
_full_ path being expanded to

	master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6 upstream

which I have to write out in full (or, more commonly, cut-and-paste) right
now.

Re: [PATCH] Use git-update-ref in scripts.

[Linus Torvalds]

On Wed, 28 Sep 2005, Junio C Hamano wrote:
>
> I was wondering the URL shorthand you mentioned is being used in
> practice.  In theory, you do not "have to write out in full"
> anymore ;-).

Almost always, I end up cut-and-pasting the thing from an email.

In many ways, the most irritating part for me about that is that an email 
that wants to give a publically accessible part has to be something like

   "Please pull from

      rsync://rsync.kernel.org/path

    to get the xyz updates"

and then I cut-and-paste it but have to delete the "rsync://", and replace 
the "rsync.kernel.org/" with "master.kernel.org:/". 

Sad.

So I've actually considered a totally Linus-only hack that does that 
automatically, ie something like this untested patch.. (same goes for http 
too, for that matter)

		Linus

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -32,7 +32,7 @@ get_remote_url () {
 	data_source=$(get_data_source "$1")
 	case "$data_source" in
 	'')
-		echo "$1" ;;
+		echo "$1" | sed 's/rsync:[^/]*kernel.org/master.kernel.org:/ ;;
 	remotes)
 		sed -ne '/^URL: */{
 			s///p

Re: [PATCH] Use git-update-ref in scripts.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Wed, 28 Sep 2005, Junio C Hamano wrote:
>> 
> I don't like doing the diff before-hand, but it looked like the default 
> was to try just one strategy, and avoid the diff in that case.

By 'diff before-hand' I take it to mean the savestate for later
rounds to keep the pre-merge state.  You are correct that it is
not done in a single strategy case, and 'git pull' by default
would use only one of Daniel's git-merge-resolve or in the
multi-remote case git-merge-octopus, depending on the number of
heads being merged.  BTW, I decided not to use diff, just in
case somebody has binary blob we cannot reproduce with diff and
patch.

> Actually, my preference would be to have a unconditional simple case
> first. If there's only one possible base, and the trivial merge succeeds
> (ie no three-way merges needed at all, just a single git-read-tree), do
> that part unconditionally.
>
> That actually matches 90% of all merges I do, and I'd be much happier with 
> git-merge if it did that first and if it then does something more complex 
> (including diffs etc) afterwards, I'm much less likely to worry.

Hmph.  That sort of makes sense but to make the unconditional
simple case really fast it should use read-tree -m -u which
_would_ smudge if things do not go well, which implies you need
savestate before that which would make it slower -- wouldn't it?

I think using Daniel's git-merge-resolve and nothing else by
default would be equivalent of having that unconditional simple
case upfront.

Re: [PATCH] Use git-update-ref in scripts.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> In many ways, the most irritating part for me about that is that an email 
> that wants to give a publically accessible part has to be something like
>
>    "Please pull from
>
>       rsync://rsync.kernel.org/path
>
>     to get the xyz updates"
>
> and then I cut-and-paste it but have to delete the "rsync://", and replace 
> the "rsync.kernel.org/" with "master.kernel.org:/". 
>
> Sad.

Hopefully that would be rectified when git-daemon goes on-line,
now I've merged updates from HPA ;-).

Re: [PATCH] Use git-update-ref in scripts.

[Junio C Hamano]

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

> Linus Torvalds <torvalds@osdl.org> writes:
>
>> On Wed, 28 Sep 2005, Junio C Hamano wrote:
>>> 
>> I don't like doing the diff before-hand, but it looked like the default 
>> was to try just one strategy, and avoid the diff in that case.
>
> By 'diff before-hand' I take it to mean the savestate for later
> rounds to keep the pre-merge state.  You are correct that it is
> not done in a single strategy case, and 'git pull' by default
> would use only one of Daniel's git-merge-resolve or in the
> multi-remote case git-merge-octopus, depending on the number of
> heads being merged.  BTW, I decided not to use diff, just in
> case somebody has binary blob we cannot reproduce with diff and
> patch.

I see two more diffs that turns out to be problematic in
git-merge.sh code.

 (1) As a safety measure I have a check to make sure the index
     is in sync with $head.  This visibly hurts; on my slow disk
     and CPU with a couple of locally modified paths in the
     working tree, this check takes about a second in the kernel
     tree with hot cache.

     git-merge-resolve uses "git-read-tree -u -m O A B" form, so
     this is totally unnecessary.  I am not so sure about
     Fredrik's git-merge-recursive (I haven't looked at it for a
     while).

 (2) savestate uses "git diff $head" to find out the list of
     paths that have local modifications, but the current code
     calls it after the check described above, so "git-ls-files
     -m" is enough.  On a kernel tree with hot cache and index
     in sync with HEAD, "git-ls-files -m" is about 3-4 times as
     fast with a couple of locally modified files.

Keeping (1) and using 'git-ls-files -m' in (2) is optimizing for
a wrong path.  On the other hand, (1) is a safety measure, and
if it is kept (2) becomes a quite cheap operation.

I am inclined to just remove the check in (1), and make it the
responsibility of merge strategies to make sure it does not
commit unrelated changes.

Comments?

Re: [PATCH] Use git-update-ref in scripts.

[Fredrik Kuivinen]

On Wed, Sep 28, 2005 at 12:47:12PM -0700, Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
> 
> > Linus Torvalds <torvalds@osdl.org> writes:
> >
> >> On Wed, 28 Sep 2005, Junio C Hamano wrote:
> >>> 
> >> I don't like doing the diff before-hand, but it looked like the default 
> >> was to try just one strategy, and avoid the diff in that case.
> >
> > By 'diff before-hand' I take it to mean the savestate for later
> > rounds to keep the pre-merge state.  You are correct that it is
> > not done in a single strategy case, and 'git pull' by default
> > would use only one of Daniel's git-merge-resolve or in the
> > multi-remote case git-merge-octopus, depending on the number of
> > heads being merged.  BTW, I decided not to use diff, just in
> > case somebody has binary blob we cannot reproduce with diff and
> > patch.
> 
> I see two more diffs that turns out to be problematic in
> git-merge.sh code.
> 
>  (1) As a safety measure I have a check to make sure the index
>      is in sync with $head.  This visibly hurts; on my slow disk
>      and CPU with a couple of locally modified paths in the
>      working tree, this check takes about a second in the kernel
>      tree with hot cache.
> 
>      git-merge-resolve uses "git-read-tree -u -m O A B" form, so
>      this is totally unnecessary.  I am not so sure about
>      Fredrik's git-merge-recursive (I haven't looked at it for a
>      while).
> 

git-merge-recursive also uses 'git-read-tree -u -m O A B' so it
shouldn't have any problems with this change.


>  (2) savestate uses "git diff $head" to find out the list of
>      paths that have local modifications, but the current code
>      calls it after the check described above, so "git-ls-files
>      -m" is enough.  On a kernel tree with hot cache and index
>      in sync with HEAD, "git-ls-files -m" is about 3-4 times as
>      fast with a couple of locally modified files.
> 
> Keeping (1) and using 'git-ls-files -m' in (2) is optimizing for
> a wrong path.  On the other hand, (1) is a safety measure, and
> if it is kept (2) becomes a quite cheap operation.
> 
> I am inclined to just remove the check in (1), and make it the
> responsibility of merge strategies to make sure it does not
> commit unrelated changes.
> 
> Comments?
> 

Looks good to me.

- Fredrik

Re: [PATCH] Use git-update-ref in scripts.

[Linus Torvalds]

On Wed, 28 Sep 2005, Junio C Hamano wrote:
> 
> Hmph.  That sort of makes sense but to make the unconditional
> simple case really fast it should use read-tree -m -u which
> _would_ smudge if things do not go well, which implies you need
> savestate before that which would make it slower -- wouldn't it?

Yeah, we'd have to do something like this.. (untested, surprise, surprise)

NOTE! Even if you don't take this, I noticed what looks like a missing 
"continue" in the "--head" case. That just can't work without it, afaik. 

Not that I know what "--head" is supposed to do.. 

		Linus
---
diff --git a/read-tree.c b/read-tree.c
--- a/read-tree.c
+++ b/read-tree.c
@@ -13,6 +13,8 @@
 static int merge = 0;
 static int update = 0;
 static int index_only = 0;
+static int nontrivial_merge = 0;
+static int trivial_merges_only = 0;
 
 static int head_idx = -1;
 static int merge_size = 0;
@@ -275,6 +277,9 @@ static int unpack_trees(merge_fn_t fn)
 	if (unpack_trees_rec(posns, len, "", fn, &indpos))
 		return -1;
 
+	if (trivial_merges_only && nontrivial_merge)
+		die("Merge requires file-level merging");
+
 	check_updates(active_cache, active_nr);
 	return 0;
 }
@@ -460,6 +465,8 @@ static int threeway_merge(struct cache_e
 		verify_uptodate(index);
 	}
 
+	nontrivial_merge = 1;
+
 	/* #2, #3, #4, #6, #7, #9, #11. */
 	count = 0;
 	if (!head_match || !remote_match) {
@@ -629,9 +636,15 @@ int main(int argc, char **argv)
 			continue;
 		}
 
+		if (!strcmp(arg, "--trivial")) {
+			trivial_merges_only = 1;
+			continue;
+		}
+
 		if (!strcmp(arg, "--head")) {
 			head_idx = stage - 1;
 			fn = threeway_merge;
+			continue;
 		}
 
 		/* "-m" stands for "merge", meaning we start in stage 1 */

Re: [PATCH] Use git-update-ref in scripts.

[Linus Torvalds]

On Wed, 28 Sep 2005, Junio C Hamano wrote:
> 
> Hopefully that would be rectified when git-daemon goes on-line,
> now I've merged updates from HPA ;-).

Yes, the git-daemon will help. However, it won't fix the lag from 
master.kernel.org to the slaves, so I'd probably still have to re-write 
things if there are mirror delays..

		Linus