Re: [PATCH 9/9] Build in checkout

> 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.

> > On Mon, 4 Feb 2008, Daniel Barkalow wrote:
> > 
> > > 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.

> > > +	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.

> > > +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.

> > > +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*

> > > +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).