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.

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

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

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