[PATCH] "checkout-cache -m" writes unmerged contents for each stage.

[PATCH] "checkout-cache -m" writes unmerged contents for each stage.

[Junio C Hamano]

This is the alternative strategy I mentioned in my previous
message.  A new -m option to checkout-cache causes it to store
contents of unmerged paths in path~1~, path~2~, and path~3~.

To be applied on top of the previous patch I re-sent:

    [PATCH] checkout-cache -a should not extract unmerged stages.

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

 checkout-cache.c |   93 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 76 insertions(+), 17 deletions(-)

checkout-cache.c: 596471a2f98b80a622488cf04edf9e95ce8666b1
--- checkout-cache.c
+++ checkout-cache.c	2005-04-17 15:49:15.000000000 -0700
@@ -34,7 +34,7 @@
  */
 #include "cache.h"
 
-static int force = 0, quiet = 0;
+static int force = 0, merge = 0, quiet = 0;
 
 static void create_directories(const char *path)
 {
@@ -65,6 +65,31 @@ static int create_file(const char *path,
 	return fd;
 }
 
+/* Returns the pathname itself for a merged entry
+ * and pathname~N~ for an unmerged one.
+ * Do not free the value you get from this function.
+ */
+static char *ce_name_with_stage(struct cache_entry *ce)
+{
+	/* (CE_NAMEMASK+1) is the max length of a name.
+	 * We are adding 3 bytes for ~N~ and we need a terminating NUL
+	 * hence +5.
+	 */
+	static char name[CE_NAMEMASK + 5];
+	int stage = ce_stage(ce);
+	if (! stage)
+		return ce->name;
+	else {
+		int pos = ce_namelen(ce);
+		strcpy(name, ce->name);
+		name[pos++] = '~';
+		name[pos++] = '0' + stage;
+		name[pos++] = '~';
+		name[pos] = 0;
+		return name;
+	}
+}
+
 static int write_entry(struct cache_entry *ce)
 {
 	int fd;
@@ -72,13 +97,20 @@ static int write_entry(struct cache_entr
 	unsigned long size;
 	long wrote;
 	char type[20];
+	char *name;
 
 	new = read_sha1_file(ce->sha1, type, &size);
 	if (!new || strcmp(type, "blob")) {
 		return error("checkout-cache: unable to read sha1 file of %s (%s)",
 			ce->name, sha1_to_hex(ce->sha1));
 	}
-	fd = create_file(ce->name, ntohl(ce->ce_mode));
+	name = ce_name_with_stage(ce);
+	if (!quiet && name != ce->name)
+		fprintf(stderr,
+			"checkout-cache: storing stage %d of %s in %s\n",
+			ce_stage(ce), ce->name, name);
+
+	fd = create_file(name, ntohl(ce->ce_mode));
 	if (fd < 0) {
 		free(new);
 		return error("checkout-cache: unable to create %s (%s)",
@@ -117,19 +149,39 @@ static int checkout_entry(struct cache_e
 	return write_entry(ce);
 }
 
+static int checkout_unmerged(int pos)
+{
+	int i, err;
+	struct cache_entry *ce = active_cache[pos];
+
+	for (err = 0, i = pos;
+	     (i < active_nr &&
+	      !strcmp(active_cache[i]->name, ce->name));
+	     i++)
+		err |= checkout_entry(active_cache[i]);
+	return err;
+}
+
 static int checkout_file(const char *name)
 {
 	int pos = cache_name_pos(name, strlen(name));
 	if (pos < 0) {
-		if (!quiet) {
-			pos = -pos - 1;
-			fprintf(stderr,
-				"checkout-cache: %s is %s.\n",
-				name,
-				(pos < active_nr &&
-				 !strcmp(active_cache[pos]->name, name)) ?
-				"unmerged" : "not in the cache");
+		pos = -pos - 1;
+		if (pos < active_nr &&
+		    !strcmp(active_cache[pos]->name, name)) {
+			if (merge)
+				return checkout_unmerged(pos);
+			else if (! quiet) {
+				fprintf(stderr,
+					"checkout-cache: %s is unmerged.\n",
+					name);
+				return -1;
+			}
 		}
+		else if (! quiet)
+			fprintf(stderr,
+				"checkout-cache: %s is not in the cache.\n",
+				name);
 		return -1;
 	}
 	return checkout_entry(active_cache[pos]);
@@ -137,22 +189,25 @@ static int checkout_file(const char *nam
 
 static int checkout_all(void)
 {
-	struct cache_entry *unmerge_skipping = NULL;
 	int i;
 
 	for (i = 0; i < active_nr ; i++) {
 		struct cache_entry *ce = active_cache[i];
 		if (ce_stage(ce)) {
-			if (!unmerge_skipping ||
-			    strcmp(unmerge_skipping->name, ce->name))
+			if (!merge)
 				fprintf(stderr,
 					"checkout-cache: needs merge %s\n",
 					ce->name);
-			unmerge_skipping = ce;
-			continue;
+			while (i < active_nr &&
+			       !strcmp(ce->name, active_cache[i]->name)) {
+				if (merge) {
+					checkout_entry(active_cache[i]);
+				}
+				i++;
+			}
+			i--;
 		}
-		unmerge_skipping = NULL;
-		if (checkout_entry(ce) < 0)
+		else if (checkout_entry(ce) < 0)
 			return -1;
 	}
 	return 0;
@@ -181,6 +236,10 @@ int main(int argc, char **argv)
 				force = 1;
 				continue;
 			}
+			if (!strcmp(arg, "-m")) {
+				merge = 1;
+				continue;
+			}
 			if (!strcmp(arg, "-q")) {
 				quiet = 1;
 				continue;

Re: [PATCH] "checkout-cache -m" writes unmerged contents for each stage.

[Linus Torvalds]

On Sun, 17 Apr 2005, Junio C Hamano wrote:
>
> This is the alternative strategy I mentioned in my previous
> message.  A new -m option to checkout-cache causes it to store
> contents of unmerged paths in path~1~, path~2~, and path~3~.

I don't think this is wrong, but it all seems a bit hacky. The reason I 
didn't apply your other patch was that I just don't feel quite comfortable 
with what is the RightThing(tm) to do yet.

I'm actually fairly certain that the right thing to do is to silently 
ignore all unmerged entries. They just aren't "interesting" from a 
checkout-cache perspective, least of all when given the "-a" flag. So even 
warning about them seems to be pointless.

The other issue is that obviously we _do_ need to have some way to merge
them, but as far as I can tell, we've really got that already in
"show-files --unmerged"  together with "cat-file blob xxxx". I do believe 
that it's a damn ugly script, and that maybe we could create some helper 
plumbing to make it more obvious, but I haven't quite decided what the 
right helper interface would be.

Writing files to the current working directory I don't like, especially as
we don't even necessarily write all three files (so with your example
code, if there was a left-over xxx~1~ file from a previous merge, your
merger would really get confused now since it thinks it has an original
file that didn't actually really exist). So I'd much rather write them to
a /tmp directory instead, let the user merge them some way.

I'm actually thinking that maybe the _right_ interface is to do something 
like this:

	merge-cache <program> <filename>

and what that does is to look up the <filename> in the cache, and if it 
has any merge entries, unpack all of them (which may be just one file, of 
course) into up to three separate files (mkstemp()), and then execve the 
supplied program name with those three files as arguments 1,2,3 (empty 
argument if no file), and "filename" as argument 4.

So now the program could be a simple script:

	#!/bin/sh
	#
	# This is the git merge script, called with
	#
	#   $1 - original file (or empty string)
	#   $2 - file in branch1 (or empty string)
	#   $3 - file in branch2 (or empty string)
	#   $4 - pathname in repository
	#
	#
	# Case 1: file removed in both
	#
	if [ -z "$2" && -z "$3" ]; then
		rm -- "$4"
		update-cache --remove -- "$4"
		exit 0
	fi
	#
	# Case 2: file exists in just one
	#
	if [ -z "$2" || -z "$3" ] then
		cat "$2""$3" > "$4"
		update-cache --add -- "$4"
		exit 0
	fi
	#
	# Case 3: file exists in both
	#
	src="$1"
	if [ -z "$1" ]; then
		src=/den/null
	fi	
	merge "$3" "$src" "$2" && cp "$3" "$4" && update-cache --add -- "$4"

or you could so something fancier.

This would _seem_ to be a very simple way to generate a nice merge (the 
merge script can obviously be a lot smarter than the above thing, 
including doing nice graphical merge interfaces).

An extension of this might be to make "merge-cache" take several 
filenames, and merge them one at a time (stopping if the merge program 
returns a failure code). And obviously this also expands trivially to the 
normal "-a" case, which would do the same, except it would just call the 
merge program for any file it finds that needs merging)

What do you think? I can whip up a "merge-cache" program like that in five 
minutes, and it _seems_ like the right interface..

		Linus

Re: [PATCH] "checkout-cache -m" writes unmerged contents for each stage.

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> On Sun, 17 Apr 2005, Junio C Hamano wrote:
>> 
>> This is the alternative strategy I mentioned in my previous
>> message.  A new -m option to checkout-cache causes it to store
>> contents of unmerged paths in path~1~, path~2~, and path~3~.

LT> I don't think this is wrong, but it all seems a bit hacky. The reason I 
LT> didn't apply your other patch was that I just don't feel quite comfortable 
LT> with what is the RightThing(tm) to do yet.

I do not personally like that '-m' thing either so please feel
free to drop it.  I was just toying with the idea.  I knew you
would not use "-m" yourself (and I would not myself either).
The patch was primarily meant for Pasky to raise his opinion.
If having something like this helps Cogito then he can keep it.
If Cogito will be doing the "show-files --unmerged" with
"cat-file blob" itself and this "checkout-cache -m" does not
help it that much then there is no point having this hack.

LT> I'm actually fairly certain that the right thing to do is to silently 
LT> ignore all unmerged entries. They just aren't "interesting" from a 
LT> checkout-cache perspective, least of all when given the "-a" flag. So even 
LT> warning about them seems to be pointless.

I do not mind your dropping the "warning" bit, but please keep
the skipping bit for "-a".  Dropping that TT from stage 1 into
the working directory and then complaining that TT exists when
it then tries to extract stage 2 and stage 3 is simply madness.

LT> What do you think? I can whip up a "merge-cache" program like that in five 
LT> minutes, and it _seems_ like the right interface..

Yes.  I think that is the right thing to do.  In fact the idea
is quite similar to what I've been working on, which is a
rewrite of that perl thing to use "read-tree -m O A B".

Re: [PATCH] "checkout-cache -m" writes unmerged contents for each stage.

[Linus Torvalds]

On Sun, 17 Apr 2005, Junio C Hamano wrote:
> 
> LT> What do you think? I can whip up a "merge-cache" program like that in five 
> LT> minutes, and it _seems_ like the right interface..
> 
> Yes.  I think that is the right thing to do.  In fact the idea
> is quite similar to what I've been working on, which is a
> rewrite of that perl thing to use "read-tree -m O A B".

I pushed the thing out. It was indeed pretty trivial.

Just as an example, on your merge-test, I can then do

	merge-cache echo -a

and the output is

	 .merge_file_Raanu4 .merge_file_q7ZoLY AA
	 .merge_file_zMXLAW .merge_file_SOugrU AN
	.merge_file_tWf5zS   DD
	.merge_file_YmGzgR   DM
	.merge_file_N1M8oO   DN
	.merge_file_eAC5WL .merge_file_ROg7mM  MD
	.merge_file_G1AWSM .merge_file_LMCruN .merge_file_ucVx7N MM
	  .merge_file_3YxD2O NA
	.merge_file_2SJa6P .merge_file_3V5g6Q  ND
	.merge_file_SIo4nQ   S/DD
	.merge_file_dO8AOP   S/DM
	.merge_file_wOO6iP   S/DN
	.merge_file_RrgnYR .merge_file_KQxqHU  S/MD
	.merge_file_F0sQRX .merge_file_E4DC00 .merge_file_HQkyn2 S/MM
	.merge_file_od0mf4 .merge_file_Xvv035  S/ND
	.merge_file_oEtq17 .merge_file_TwkT5c .merge_file_aqx58h Trivial

(Note that the spaces signify an empty argument in those places, since the
file in question didn't always exist). It executed the "echo" thing 16 
times (once for each object that your script-from-hell had caused a clash 
with).

In all cases you have $1 being "original", $2 being "branch 1", $3 being
"branch 2", and $4 being "name in cache".

ALERT ALERT ALERT! The git "merge object order" is different from the 
"merge" program merge object order. In the above ordering, the original is 
first. But the argument order to the 3-way merge program "merge" is to 
have the original in the middle. Don't ask me why.

Anyway, another example:

	torvalds@ppc970:~/merge-test> merge-cache cat MM
	This is MM from the original tree.			# original
	This is modified MM in the branch A.			# merge1
	This is modified MM in the branch B.			# merge2
	This is modified MM in the branch B.			# current contents

or 

	torvalds@ppc970:~/merge-test> merge-cache cat AA MM
	cat: : No such file or directory
	This is added AA in the branch A.
	This is added AA in the branch B.
	This is added AA in the branch B.
	fatal: merge program failed

where the latter example shows how "merge-cache" will stop trying to merge 
once anything has returned an error (ie "cat" returned an error for the AA 
file, because it didn't exist in the original, and thus "merge-cache" 
didn't even try to merge the MM thing).

		Linus

Re: [PATCH] "checkout-cache -m" writes unmerged contents for each stage.

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> I'm actually thinking that maybe the _right_ interface is to do something 
LT> like this:

LT> 	merge-cache <program> <filename>

LT> and what that does is to look up the <filename> in the cache, and if it 
LT> has any merge entries, unpack all of them (which may be just one file, of 
LT> course) into up to three separate files (mkstemp()), and then execve the 
LT> supplied program name with those three files as arguments 1,2,3 (empty 
LT> argument if no file), and "filename" as argument 4.

LT> What do you think? I can whip up a "merge-cache" program like that in five 
LT> minutes, and it _seems_ like the right interface..

One small detail.  What about the "-x" bit?

In case 2 and 3 in your sample merge script, the "merge script"
needs to know what the preferred mode bits are before running
"update-cache --add".

Yes, it can figure that out by running "show-files --unmerged"
and grepping for "$4" by itself, but then it can figure out the
information in "$1" through "$3" by itself as well, so that
makes having merge-cache wrapper less useful to begin with.

I do not think it is realistic for these three related trees to
have files that differ in -x bit, so it would not be that useful
to give the "merge script" the flexibility to pick -x bit value
among three parents --- it would be fine for the merge-cache
wrapper to dictate the value of the -x bit for the resulting
file.  So I'd suggest to add an extra parameter to the "merge
script" when merge-cache wrapper calls it.

Re: [PATCH] "checkout-cache -m" writes unmerged contents for each stage.

[Linus Torvalds]

On Sun, 17 Apr 2005, Junio C Hamano wrote:
> 
> One small detail.  What about the "-x" bit?

You'll need to merge those kinds of changes separately for now. We'll have 
to resolve that too, right now I think the -x bit gets lost on a merge 
simply because we just write the temp-files, something merges them, and 
does an "update-cache" on the merged result, and we'll pick up the -x bit 
(or rather, lack of one) at that point.

		Linus