Re: [PATCH 9/9] Build in checkout

On Tue, 5 Feb 2008, Johannes Schindelin wrote:

Hi,

On Tue, 5 Feb 2008, Daniel Barkalow wrote:

quoted

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.

It's okay, I've got other things to work on. I'll probably send out a new 
version of the series now, in any case, so that you can see if I handled 
your comments correctly thus far.

quoted

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.

This isn't used for switching branches; this is used for checking out 
paths. If you do "git checkout <not-head> -- <every single path>", make 
will punish you, but why would you do that? I'd guess that people are 
unlikely to have a significant number of non-changes in this piece of 
code, just because they wouldn't list things that they didn't think had 
changes.

quoted

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.

I'd say only for the case where CC is initially unset, at which point the 
compiler is going to be "gcc" because we set it to that. And people who 
symlink their non-gcc-like system compiler to gcc have other problems, 
most likely.

quoted

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

Sure. I'll probably actually change builtin-reset to do the non-fork 
thing (in a separate series; it's really unrelated). I doubt there's any 
point to sharing the code for setting all of the options, though, which is 
best to have inlined by the compiler and be easy to find.

	-Daniel
*This .sig left intentionally blank*