Re: [PATCH 9/9] Build in checkout

Hi,

On Tue, 5 Feb 2008, Daniel Barkalow wrote:

On Tue, 5 Feb 2008, Johannes Schindelin wrote:

So, as an introductory comment, I was working quite strictly from 
git-checkout.sh, using the implementations of the operations as given 
there, and not looking at what other code has the same goal and may be 
doing it differently, so I've almost certainly missed a number of 
chances for sharing code (beyond calling functions directly instead of 
putting together command lines and having them parsed). In particular, I 
hadn't looked at all at builtin-reset, which obviously does a bunch of 
the same things.

Well, it seems like I won't be able to review any more til this weekend.  
Sorry.

quoted

On Mon, 4 Feb 2008, Daniel Barkalow wrote:

quoted

diff --git a/builtin-checkout.c b/builtin-checkout.c
new file mode 100644
index 0000000..2950d5c
--- /dev/null
+++ b/builtin-checkout.c

@@ -0,0 +1,478 @@
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "commit.h"
+#include "tree.h"
+#include "tree-walk.h"
+#include "unpack-trees.h"
+#include "dir.h"
+#include "run-command.h"
+#include "merge-recursive.h"
+#include "diff.h"
+#include "revision.h"
+
+static const char * const checkout_usage[] = {
+	"git checkout [options] <branch>",
+	"git checkout [options] [<branch>] -- <file>...",
+	NULL,
+};
+
+static int post_checkout_hook(struct commit *old, struct commit *new,
+			      int changed)
+{
+	struct child_process proc;
+	const char *name = git_path("hooks/post-checkout");
+	const char *argv[5];
+
+	if (access(name, X_OK) < 0)
+		return 0;
+
+	memset(&proc, 0, sizeof(proc));
+	argv[0] = name;
+	argv[1] = xstrdup(sha1_to_hex(old->object.sha1));
+	argv[2] = xstrdup(sha1_to_hex(new->object.sha1));
+	argv[3] = changed ? "1" : "0";
+	argv[4] = NULL;
+	proc.argv = argv;
+	proc.no_stdin = 1;
+	proc.stdout_to_stderr = 1;
+	return run_command(&proc);
+}

Would this not be helped by the patch of Paolo in 
http://article.gmane.org/gmane.comp.version-control.git/72495?

We would only need to move the function into run-command.[ch]...

Probably, but that hasn't been merged yet as of origin/next, which is 
what I'm working from. Good candidate for a follow-up when both series 
are in, or if I do a revision of this series after that one is in.

Concur.  I briefly looked at moving run_hook(), and it is not as nice as I 
thought: builtin-commit's version of run_hook() wants to set the 
environment variable GIT_INDEX_FILE, and therefore it has a parameter 
index_file which most other hooks probably do not need.

quoted

quoted

+	hashcpy(ce->sha1, sha1);
+	memcpy(ce->name, base, baselen);
+	memcpy(ce->name + baselen, pathname, len - baselen);
+	ce->ce_flags = create_ce_flags(len, 0);
+	ce->ce_mode = create_ce_mode(mode);
+	add_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
+	return 0;
+}
+
+static int read_tree_some(struct tree *tree, const char **pathspec)

I have a hunch that you could share code with builtin-reset.c, namely 
call function read_from_tree() here.

This version matches the "ls-tree | update-index" implementation in 
git-checkout.sh; I'm not terribly fond of the diff-index-based version, 
since it messed up the index, rereads it from disk, and then applies 
changes. On the other hand, it would probably be good to update the 
cache entries with a function that would keep the stat info if nothing 
else changed.

Exactly.  In fact, that is the only reason I suggested that: to prevent 
the index from needing an update.

This is quite important if you have a large working tree, and switch 
branches from A to B.  For example, "make" will punish you where it hurts.

quoted

quoted

+static int remove_merge_head(void)
+{
+	unlink(git_path("MERGE_HEAD"));
+	return 0;
+}

Please make this either void, or return what the unlink() returns.

I think, actually, that I should steal the end of "builtin-reset", which 
discards a few other files as well that won't be appropriate after the 
switch.

Good.

quoted

quoted

+static void show_local_changes(struct object *head)
+{
+	struct rev_info rev;
+	// I think we want full paths, even if we're in a subdirectory.

Please avoid C++-style comments.

Yup. Can we make git build by default with --std=c89, so I get errors for 
this sort of thing? *sigh*

Heh.  You can do that in config.mak.  For the Makefile, I am afraid this 
is no option, as we try to support other compilers than gcc, too.

quoted

quoted

+static int reset_to_new(struct tree *tree, int quiet)

Again, I think that this would benefit from code sharing with 
builtin-reset.  It can be a bit tricky to avoid writing files when they 
are already up-to-date...

Reset actually execs "read-tree", which does a bunch of parsing to set up 
the arguments like this function does (except that it doesn't keep the 
stat info, I think), and then uses the same actual code that this function 
calls directly (which has done the only-write-changes thing correctly 
since June 2005 or before).

Okay, I'll just wait and see (as I do not have the time to dive into the 
code).

Thanks,
Dscho