Re: [PATCH 9/9] Build in checkout

Hi,

On Mon, 4 Feb 2008, Daniel Barkalow wrote:

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

+static int cache_unmerged(void)
+{
+	int i;
+	for (i = 0; i < active_nr; i++) {
+		if (ce_stage(active_cache[i]))
+			return 1;
+	}
+	return 0;
+}

AFAICT we have this function in merge-recursive.c (unmerged_index()) and 
builtin-reset.c (unmerged_files()) already.  Maybe it is time to 
consolidate them into one function in read-cache.c?

+
+static int update_some(const unsigned char *sha1, const char *base, int baselen,

Heh, nice function name ;-)

+		       const char *pathname, unsigned mode, int stage)
+{
+	if (S_ISGITLINK(mode)) {
+		return 0;
+	} else if (S_ISDIR(mode)) {
+		return READ_TREE_RECURSIVE;
+	}

You could lose the curly brackets and the "else", for readability.

+	int len = baselen + strlen(pathname);
+	struct cache_entry *ce = xcalloc(1, cache_entry_size(len));

These variables need to be declared before the code (the if()s) starts.

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

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

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

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

+	init_revisions(&rev, NULL);
+	rev.abbrev = 0;
+	rev.diffopt.output_format |= DIFF_FORMAT_NAME_STATUS;
+	add_pending_object(&rev, head, NULL);
+	run_diff_index(&rev, 0);
+}
+
+static void describe_detached_head(char *msg, struct commit *commit)
+{
+	struct strbuf sb;
+	strbuf_init(&sb, 0);
+	parse_commit(commit);
+	pretty_print_commit(CMIT_FMT_ONELINE, commit, &sb, 0, "", "", 0, 0);
+	fprintf(stderr, "%s %s... %s\n", msg,
+		find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV), sb.buf);

I am tempted to suggest

	pretty_print_commit(get_commit_format("format:%h... %s"), <...>);

here...

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

+struct checkout_opts {
+	int quiet;
+	int merge;
+	int force;
+
+	char *new_branch;
+	int new_branch_log;
+	int track;
+	int no_track;
+};
+
+struct branch_info {
+	const char *name; /* The short name used */
+	const char *path; /* The full name of a real branch */
+	struct commit *commit; /* The named commit */
+};
+
+static void setup_branch_path(struct branch_info *branch)
+{
+	char *path = xmalloc(strlen(branch->name) +
+			     strlen("refs/heads/") + 1);
+	sprintf(path, "refs/heads/%s", branch->name);
+	branch->path = path;

Looks like a perfect candidate for strbuf to me.

+static void update_refs_for_switch(struct checkout_opts *opts,
+				   struct branch_info *old,
+				   struct branch_info *new)
+{
+	char *msg;
+	const char *old_desc;
+	if (opts->new_branch) {
+		create_branch(opts->new_branch, new->name, 0,
+			      opts->new_branch_log, opts->track);
+		new->name = opts->new_branch;
+		setup_branch_path(new);
+	}
+
+	msg = malloc(1024);
+	old_desc = old->name;
+	if (!old_desc)
+		old_desc = sha1_to_hex(old->commit->object.sha1);
+	snprintf(msg, 1024, "checkout: moving from %s to %s",
+		 old_desc, new->name);

Again, a perfect candidate for strbuf.

Unfortunately, I am a bit too tired to go on (especially for finding 
common ground between builtin-reset and read_tree_some(), 
checkout_paths(), reset_to_new(), and reset_clean_to_new()).  Hopefully 
I'll find time to look at it tomorrow...

Ciao,
Dscho