Add --temp and --stage=all options to checkout-index.

PATCH In review Shawn Pearce 2 participants 6 messages Jun 15, 2016 Documentation

Share & tools

Shawn Pearce

author

2016-06-15 22:42 UTC #0 lore raw

PATCH 1/1 Add --temp and --stage=all options to checkout-index.

Sometimes it is convient for a Porcelain to be able to checkout all unmerged files in all stages so that an external merge tool can be executed by the Porcelain or the end-user. Using git-unpack-file on each stage individually incurs a rather high penalty due to the need to fork for each file version obtained. git-checkout-index -a --stage=all will now do the same thing, but faster.

Signed-off-by Shawn O. Pearce <[email protected]>

---
This spawned out of the discussion of my prior --suffix patch and replaces it entirely.

The output format is Junio's idea, which I happened to like a lot. I think I have faithfully implemented it here, but added a case where you can obtain temporary files for only a specific stage without needing to read the 'wider' 3-stage format.

Unfortunately this change lead me down a path which changed the core checkout code also used by apply and read-tree. So those commands could now checkout to temporary files if they wanted to. :-)

It currently passes all test cases, including the new one provided by this patch.

 Documentation/git-checkout-index.txt |   51 ++++++++
 apply.c                              |    3 
 cache.h                              |    5 -
 checkout-index.c                     |   84 ++++++++++++-
 entry.c                              |   92 +++++++++------
 read-tree.c                          |    3 
 t/t2004-checkout-cache-temp.sh       |  212 ++++++++++++++++++++++++++++++++++
 7 files changed, 396 insertions(+), 54 deletions(-)
 create mode 100755 t/t2004-checkout-cache-temp.sh

base c6b84bc40310f8e0cf2fe290b5f291e31cf688ef
last 022659f9b239fd5513a883e09ac0e0688b69dbe1

Documentation/git-checkout-index.txt +49 -2

index b0b6588..09bd6a5 100644
@@ -10,7 +10,8 @@ SYNOPSIS
 --------
 [verse]
 'git-checkout-index' [-u] [-q] [-a] [-f] [-n] [--prefix=<string>]
-		   [--stage=<number>]
+		   [--stage=<number>|all]
+		   [--temp]
 		   [-z] [--stdin]
 		   [--] [<file>]\*

@@ -43,9 +44,15 @@ OPTIONS
 	When creating files, prepend <string> (usually a directory
 	including a trailing /)
 
---stage=<number>::
+--stage=<number>|all::
 	Instead of checking out unmerged entries, copy out the
 	files from named stage.  <number> must be between 1 and 3.
+	Note: --stage=all automatically implies --temp.
+
+--temp::
+	Instead of copying the files to the working directory
+	write the content to temporary files.  The temporary name
+	associations will be written to stdout.
 
 --stdin::
 	Instead of taking list of paths from the command line,

@@ -87,6 +94,46 @@ it will prevent problems with a filename
 Using `--` is probably a good policy in scripts.
 
 
+Using --temp or --stage=all
+---------------------------
+When `--temp` is used (or implied by `--stage=all`)
+`git-checkout-index` will create a temporary file for each index
+entry being checked out.  The index will not be updated with stat
+information.  These options can be useful if the caller needs all
+stages of all unmerged entries so that the unmerged files can be
+processed by an external merge tool.
+
+A listing will be written to stdout providing the association of
+temporary file names to tracked path names.  The listing format
+has two variations:
+
+    . tempname TAB path RS
++
+The first format is what gets used when `--stage` is omitted or
+is not `--stage=all`. The field tempname is the temporary file
+name holding the file content and path is the tracked path name in
+the index.  Only the requested entries are output.
+
+    . stage1temp SP stage2temp SP stage3tmp TAB path RS
++
+The second format is what gets used when `--stage=all`.  The three
+stage temporary fields (stage1temp, stage2temp, stage3temp) list the
+name of the temporary file if there is a stage entry in the index
+or `.` if there is no stage entry.  Paths which only have a stage 0
+entry will always be omitted from the output.
+
+In both formats RS (the record separator) is newline by default
+but will be the null byte if -z was passed on the command line.
+The temporary file names are always safe strings; they will never
+contain directory separators or whitespace characters.  The path
+field is always relative to the current directory and the temporary
+file names are always relative to the top level directory.
+
+If the object being copied out to a temporary file is a symbolic
+link the content of the link will be written to a normal file.  It is
+up to the end-user or the Porcelain to make use of this information.
+
+
 EXAMPLES
 --------
 To update and refresh only the files already checked out::

apply.c +2 -1

index c369966..5583a80 100644
@@ -1376,6 +1376,7 @@ static int apply_data(struct patch *patc
 static int check_patch(struct patch *patch)
 {
 	struct stat st;
+	static char topath[MAXPATHLEN+1];
 	const char *old_name = patch->old_name;
 	const char *new_name = patch->new_name;
 	const char *name = old_name ? old_name : new_name;

@@ -1402,7 +1403,7 @@ static int check_patch(struct patch *pat
 				costate.not_new = 0;
 				costate.refresh_cache = 1;
 				if (checkout_entry(active_cache[pos],
-						   &costate) ||
+						   &costate, topath) ||
 				    lstat(old_name, &st))
 					return -1;
 			}

cache.h +3 -2

index 0d3b244..f87744c 100644
@@ -259,10 +259,11 @@ struct checkout {
 	unsigned force:1,
 		 quiet:1,
 		 not_new:1,
-		 refresh_cache:1;
+		 refresh_cache:1,
+		 to_tempfile:1;
 };
 
-extern int checkout_entry(struct cache_entry *ce, struct checkout *state);
+extern int checkout_entry(struct cache_entry *ce, struct checkout *state, char *topath);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;

checkout-index.c +72 ~~-12~~

159 lines not on this page · raw message

entry.c +56 ~~-36~~

123 lines not on this page · raw message

read-tree.c +2 -1

index be29b3f..e88d69e 100644
@@ -279,6 +279,7 @@ static void progress_interval(int signum
 
 static void check_updates(struct cache_entry **src, int nr)
 {
+	static char topath[MAXPATHLEN+1];
 	static struct checkout state = {
 		.base_dir = "",
 		.force = 1,

@@ -337,7 +338,7 @@ static void check_updates(struct cache_e
 		if (ce->ce_flags & mask) {
 			ce->ce_flags &= ~mask;
 			if (update)
-				checkout_entry(ce, &state);
+				checkout_entry(ce, &state, topath);
 		}
 	}
 	if (total) {

t/t2004-checkout-cache-temp.sh new file +212 -0

212 lines not on this page · raw message

Signature · 1 line

1.2.3.g5f0e

Junio C Hamano

2016-06-15 22:42 UTC #1 lore raw

Quoted 2 lines · Shawn Pearce

Unfortunately this change lead me down a path which changed the core checkout code also used by apply and read-tree.

... which makes it much harder to swallow without careful inspection X-<.

I think the patch only appears much bigger than it actually is, because of reindentation effect coming from "if (to-temp-file)". However, I am too tired to carefully examine them tonight, so I hope you do not mind my postponing this for now.

Shawn Pearce

author

2016-06-15 22:42 UTC #2 lore raw

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Shawn Pearce
Unfortunately this change lead me down a path which changed the core checkout code also used by apply and read-tree.

Quoted 7 lines

... which makes it much harder to swallow without careful inspection X-<.
I think the patch only appears much bigger than it actually is, because of reindentation effect coming from "if (to-temp-file)". However, I am too tired to carefully examine them tonight, so I hope you do not mind my postponing this for now.

Not at all. I read over the diff before sending it and thought it made the patch look much worse than it really is, simply because of the reindentation. :-|

Sometimes good 'ole diff ain't that great for conveying a change.

Signature · 1 line

Shawn.

Junio C Hamano

2016-06-15 22:42 UTC #3 lore raw

Quoted 6 lines · Shawn Pearce

Quoted 5 lines · Junio C Hamano
Quoted 2 lines · Shawn Pearce
Unfortunately this change lead me down a path which changed the core checkout code also used by apply and read-tree.

I would have preferred not to add to_tempfile member to the checkout struct. Instead, if checkout_entry has non NULL topath, check out to a temporary location and return the path; otherwise behave the way it did before.

That way everybody else would not need to declare useless topath[] array on the stack and pass it to checkout_entry (instead they would just pass NULL because they are not interested in checking out to temporary files).

I am unsure about what the tempfile option should do when asked to checkout a symbolic link. Creating a temporary regular file that has the readlink result does not sound very useful to me.

BTW, in any case, I think there is one breakage that needs to be fixed with something like this...

---

apply.c +1 -0

index 5a2cf54..727d63d 100644
@@ -1311,6 +1311,7 @@ static int check_patch(struct patch *pat
 				costate.force = 0;
 				costate.quiet = 0;
 				costate.not_new = 0;
+				costate.to_tempfile = 0;
 				costate.refresh_cache = 1;
 				if (checkout_entry(active_cache[pos],
 						   &costate, topath) ||

Shawn Pearce

author

2016-06-15 22:42 UTC #4 lore raw

Signed-off-by Shawn O. Pearce <[email protected]>

---
This replaces my prior patch. It looks a heck of a lot cleaner too due to less indentation changes. :-)

Quoted 5 lines · Junio C Hamano

Quoted 3 lines · Shawn Pearce
Quoted 2 lines · Junio C Hamano
Shawn Pearce <[email protected]> writes:

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
Unfortunately this change lead me down a path which changed the core checkout code also used by apply and read-tree.
I would have preferred not to add to_tempfile member to the checkout struct. Instead, if checkout_entry has non NULL topath, check out to a temporary location and return the path; otherwise behave the way it did before.

Good idea.

Quoted 3 lines

I am unsure about what the tempfile option should do when asked to checkout a symbolic link. Creating a temporary regular file that has the readlink result does not sound very useful to me.

I couldn't decide either here; but there's no `mkstemplink' or some-such function to generate a temporary symbolic link. With the target in a temp file a Porcelain can at least generate a true symbolic link with the correct target or present it the different versions to the user for resolution. Symlinks are definately difficult.

Quoted 2 lines

BTW, in any case, I think there is one breakage that needs to be fixed with something like this...

That should be fixed with this version of the patch. Good catch.

 Documentation/git-checkout-index.txt |   51 ++++++++
 apply.c                              |    3 
 cache.h                              |    2 
 checkout-index.c                     |   86 ++++++++++++--
 entry.c                              |   40 +++++-
 read-tree.c                          |    2 
 t/t2004-checkout-cache-temp.sh       |  212 ++++++++++++++++++++++++++++++++++
 7 files changed, 370 insertions(+), 26 deletions(-)
 create mode 100755 t/t2004-checkout-cache-temp.sh

base 1c7fee5d0857d809fe5d93a1d56968e44296ce55
last aa1305c3181ac1a3a493765bf41c2d59f8b8ce3e

Documentation/git-checkout-index.txt +49 -2

index b0b6588..09bd6a5 100644
@@ -10,7 +10,8 @@ SYNOPSIS
 --------
 [verse]
 'git-checkout-index' [-u] [-q] [-a] [-f] [-n] [--prefix=<string>]
-		   [--stage=<number>]
+		   [--stage=<number>|all]
+		   [--temp]
 		   [-z] [--stdin]
 		   [--] [<file>]\*

@@ -43,9 +44,15 @@ OPTIONS
 	When creating files, prepend <string> (usually a directory
 	including a trailing /)
 
---stage=<number>::
+--stage=<number>|all::
 	Instead of checking out unmerged entries, copy out the
 	files from named stage.  <number> must be between 1 and 3.
+	Note: --stage=all automatically implies --temp.
+
+--temp::
+	Instead of copying the files to the working directory
+	write the content to temporary files.  The temporary name
+	associations will be written to stdout.
 
 --stdin::
 	Instead of taking list of paths from the command line,

@@ -87,6 +94,46 @@ it will prevent problems with a filename
 Using `--` is probably a good policy in scripts.
 
 
+Using --temp or --stage=all
+---------------------------
+When `--temp` is used (or implied by `--stage=all`)
+`git-checkout-index` will create a temporary file for each index
+entry being checked out.  The index will not be updated with stat
+information.  These options can be useful if the caller needs all
+stages of all unmerged entries so that the unmerged files can be
+processed by an external merge tool.
+
+A listing will be written to stdout providing the association of
+temporary file names to tracked path names.  The listing format
+has two variations:
+
+    . tempname TAB path RS
++
+The first format is what gets used when `--stage` is omitted or
+is not `--stage=all`. The field tempname is the temporary file
+name holding the file content and path is the tracked path name in
+the index.  Only the requested entries are output.
+
+    . stage1temp SP stage2temp SP stage3tmp TAB path RS
++
+The second format is what gets used when `--stage=all`.  The three
+stage temporary fields (stage1temp, stage2temp, stage3temp) list the
+name of the temporary file if there is a stage entry in the index
+or `.` if there is no stage entry.  Paths which only have a stage 0
+entry will always be omitted from the output.
+
+In both formats RS (the record separator) is newline by default
+but will be the null byte if -z was passed on the command line.
+The temporary file names are always safe strings; they will never
+contain directory separators or whitespace characters.  The path
+field is always relative to the current directory and the temporary
+file names are always relative to the top level directory.
+
+If the object being copied out to a temporary file is a symbolic
+link the content of the link will be written to a normal file.  It is
+up to the end-user or the Porcelain to make use of this information.
+
+
 EXAMPLES
 --------
 To update and refresh only the files already checked out::

apply.c +2 -1

index c369966..849a8b4 100644
@@ -1402,7 +1402,8 @@ static int check_patch(struct patch *pat
 				costate.not_new = 0;
 				costate.refresh_cache = 1;
 				if (checkout_entry(active_cache[pos],
-						   &costate) ||
+						   &costate,
+						   NULL) ||
 				    lstat(old_name, &st))
 					return -1;
 			}

cache.h +1 -1

index 8dc1de1..1f96280 100644
@@ -262,7 +262,7 @@ struct checkout {
 		 refresh_cache:1;
 };
 
-extern int checkout_entry(struct cache_entry *ce, struct checkout *state);
+extern int checkout_entry(struct cache_entry *ce, struct checkout *state, char *topath);
 
 extern struct alternate_object_database {
 	struct alternate_object_database *next;

checkout-index.c +74 ~~-12~~

159 lines not on this page · raw message

entry.c +31 -9

index 8fb99bc..38b5241 100644
@@ -63,7 +63,7 @@ static int create_file(const char *path,
 	return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
 }
 
-static int write_entry(struct cache_entry *ce, const char *path, struct checkout *state)
+static int write_entry(struct cache_entry *ce, char *path, struct checkout *state, int to_tempfile)
 {
 	int fd;
 	void *new;

@@ -80,7 +80,11 @@ static int write_entry(struct cache_entr
 	}
 	switch (ntohl(ce->ce_mode) & S_IFMT) {
 	case S_IFREG:
-		fd = create_file(path, ntohl(ce->ce_mode));
+		if (to_tempfile) {
+			strcpy(path, ".merge_file_XXXXXX");
+			fd = mkstemp(path);
+		} else 
+			fd = create_file(path, ntohl(ce->ce_mode));
 		if (fd < 0) {
 			free(new);
 			return error("git-checkout-index: unable to create file %s (%s)",

@@ -93,12 +97,27 @@ static int write_entry(struct cache_entr
 			return error("git-checkout-index: unable to write file %s", path);
 		break;
 	case S_IFLNK:
-		if (symlink(new, path)) {
+		if (to_tempfile) {
+			strcpy(path, ".merge_link_XXXXXX");
+			fd = mkstemp(path);
+			if (fd < 0) {
+				free(new);
+				return error("git-checkout-index: unable to create "
+						 "file %s (%s)", path, strerror(errno));
+			}
+			wrote = write(fd, new, size);
+			close(fd);
 			free(new);
-			return error("git-checkout-index: unable to create "
-				     "symlink %s (%s)", path, strerror(errno));
+			if (wrote != size)
+				return error("git-checkout-index: unable to write file %s",
+					path);
+		} else {
+			wrote = symlink(new, path);
+			free(new);
+			if (wrote)
+				return error("git-checkout-index: unable to create "
+						 "symlink %s (%s)", path, strerror(errno));
 		}
-		free(new);
 		break;
 	default:
 		free(new);

@@ -113,12 +132,15 @@ static int write_entry(struct cache_entr
 	return 0;
 }
 
-int checkout_entry(struct cache_entry *ce, struct checkout *state)
+int checkout_entry(struct cache_entry *ce, struct checkout *state, char *topath)
 {
-	struct stat st;
 	static char path[MAXPATHLEN+1];
+	struct stat st;
 	int len = state->base_dir_len;
 
+	if (topath)
+		return write_entry(ce, topath, state, 1);
+
 	memcpy(path, state->base_dir, len);
 	strcpy(path + len, ce->name);

@@ -147,7 +169,7 @@ int checkout_entry(struct cache_entry *c
 	} else if (state->not_new) 
 		return 0;
 	create_directories(path, state);
-	return write_entry(ce, path, state);
+	return write_entry(ce, path, state, 0);
 }

read-tree.c +1 -1

index be29b3f..1c3b09b 100644
@@ -337,7 +337,7 @@ static void check_updates(struct cache_e
 		if (ce->ce_flags & mask) {
 			ce->ce_flags &= ~mask;
 			if (update)
-				checkout_entry(ce, &state);
+				checkout_entry(ce, &state, NULL);
 		}
 	}
 	if (total) {

t/t2004-checkout-cache-temp.sh new file +212 -0

212 lines not on this page · raw message

Signature · 1 line

1.2.4.g0226

Junio C Hamano

2016-06-15 22:42 UTC #5 lore raw

Quoted 2 lines · Shawn Pearce

This replaces my prior patch. It looks a heck of a lot cleaner too due to less indentation changes. :-)

Much nicer. Will put in "next" for wider exposure.