Re: [PATCH v2] checkout: optimize "git checkout -b <new_branch>"

> But if you still want to push it further, this is something I have in
> mind. It probably has bugs, but at least preliminary test shows me
> that it could skip 99% work inside unpack_trees() and not need to
> write the index.
> 
> The main check to determine "checkout -b" is basically the new
> oidcmp() in merge_working_tree(). Not sure if I miss any condition in
> that check, I didn't look very closely at checkout code.
> 

> -- 8< --
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 28627650cd..912e565acc 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -478,6 +478,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>   {
>   	int ret;
>   	struct lock_file lock_file = LOCK_INIT;
> +	int skip_cache_tree_update = 0;
>   
>   	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
>   	if (read_cache_preload(NULL) < 0)
> @@ -493,6 +494,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
>   		struct tree_desc trees[2];
>   		struct tree *tree;
>   		struct unpack_trees_options topts;
> +		int nr_trees;
>   
>   		memset(&topts, 0, sizeof(topts));
>   		topts.head_idx = -1;
> @@ -514,20 +516,45 @@ static int merge_working_tree(const struct checkout_opts *opts,
>   		topts.merge = 1;
>   		topts.gently = opts->merge && old_branch_info->commit;
>   		topts.verbose_update = opts->show_progress;
> -		topts.fn = twoway_merge;
> +		/*
> +		 * We repair cache-tree explicitly in this function,
> +		 * no point in doing it again inside unpack_trees()
> +		 */
> +		topts.skip_cache_tree_update = 1;
>   		if (opts->overwrite_ignore) {
>   			topts.dir = xcalloc(1, sizeof(*topts.dir));
>   			topts.dir->flags |= DIR_SHOW_IGNORED;
>   			setup_standard_excludes(topts.dir);
>   		}
> -		tree = parse_tree_indirect(old_branch_info->commit ?
> -					   &old_branch_info->commit->object.oid :
> -					   the_hash_algo->empty_tree);
> -		init_tree_desc(&trees[0], tree->buffer, tree->size);
> -		tree = parse_tree_indirect(&new_branch_info->commit->object.oid);
> -		init_tree_desc(&trees[1], tree->buffer, tree->size);
> -
> -		ret = unpack_trees(2, trees, &topts);
> +		if (!oidcmp(&old_branch_info->commit->object.oid,
> +			    &new_branch_info->commit->object.oid)) {
> +			/*
> +			 * Skip 2-way merge because we know the end
> +			 * result would be the same anyway (to double
> +			 * check).
> +			 */
> +			topts.fn = noop_merge;
> +			nr_trees = 0;
> +			/*
> +			 * Repairing cache-tree is an optional optimization
> +			 * and could be safely skipped. Maybe have some
> +			 * heuristics based on the_index->cache_nr to
> +			 * determine if updating cache-tree could be too
> +			 * costly (including index writing cost) and skip it
> +			 * instead.
> +			 */
> +			skip_cache_tree_update = 1;
> +		} else {
> +			topts.fn = twoway_merge;
> +			tree = parse_tree_indirect(old_branch_info->commit ?
> +						   &old_branch_info->commit->object.oid :
> +						   the_hash_algo->empty_tree);
> +			init_tree_desc(&trees[0], tree->buffer, tree->size);
> +			tree = parse_tree_indirect(&new_branch_info->commit->object.oid);
> +			init_tree_desc(&trees[1], tree->buffer, tree->size);
> +			nr_trees = 2;
> +		}
> +		ret = unpack_trees(nr_trees, trees, &topts);
>   		clear_unpack_trees_porcelain(&topts);
>   		if (ret == -1) {
>   			/*
> @@ -597,13 +624,15 @@ static int merge_working_tree(const struct checkout_opts *opts,
>   		}
>   	}
>   
> -	if (!active_cache_tree)
> -		active_cache_tree = cache_tree();
> +	if (!skip_cache_tree_update) {
> +		if (!active_cache_tree)
> +			active_cache_tree = cache_tree();
>   
> -	if (!cache_tree_fully_valid(active_cache_tree))
> -		cache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);
> +		if (!cache_tree_fully_valid(active_cache_tree))
> +			cache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);
> +	}
>   
> -	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> +	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK | SKIP_IF_UNCHANGED))
>   		die(_("unable to write new index file"));
>   
>   	if (!opts->force && !opts->quiet)
> diff --git a/unpack-trees.c b/unpack-trees.c
> index cd0680f11e..007d6662f2 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1275,6 +1275,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>   	int i, ret;
>   	static struct cache_entry *dfc;
>   	struct exclude_list el;
> +	int result_is_actually_src = 0;
>   
>   	if (len > MAX_UNPACK_TREES)
>   		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
> @@ -1353,6 +1354,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>   
>   		if (traverse_trees(len, t, &info) < 0)
>   			goto return_failed;
> +	} else if (o->fn == noop_merge) {
> +		o->result = *o->src_index;
> +		o->cache_bottom = o->src_index->cache_nr;
> +		result_is_actually_src = 1;
>   	}
>   
>   	/* Any left-over entries in the index? */
> @@ -1426,7 +1431,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>   
>   	ret = check_updates(o) ? (-2) : 0;
>   	if (o->dst_index) {
> -		if (!ret) {
> +		if (!ret &&!o->skip_cache_tree_update) {
>   			if (!o->result.cache_tree)
>   				o->result.cache_tree = cache_tree();
>   			if (!cache_tree_fully_valid(o->result.cache_tree))
> @@ -1435,7 +1440,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>   						  WRITE_TREE_REPAIR);
>   		}
>   		move_index_extensions(&o->result, o->src_index);
> -		discard_index(o->dst_index);
> +		if (result_is_actually_src && o->dst_index == o->src_index)
> +			; /* no discarding */
> +		else
> +			discard_index(o->dst_index);
>   		*o->dst_index = o->result;
>   	} else {
>   		discard_index(&o->result);
> @@ -2193,3 +2201,9 @@ int oneway_merge(const struct cache_entry * const *src,
>   	}
>   	return merged_entry(a, old, o);
>   }
> +
> +int noop_merge(const struct cache_entry * const *src,
> +	       struct unpack_trees_options *o)
> +{
> +	BUG("This is basically a flag and not supposed to be called");
> +}
> diff --git a/unpack-trees.h b/unpack-trees.h
> index c2b434c606..3e5bce19a1 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -52,6 +52,7 @@ struct unpack_trees_options {
>   		     diff_index_cached,
>   		     debug_unpack,
>   		     skip_sparse_checkout,
> +		     skip_cache_tree_update,
>   		     gently,
>   		     exiting_early,
>   		     show_all_errors,
> @@ -96,5 +97,7 @@ int bind_merge(const struct cache_entry * const *src,
>   	       struct unpack_trees_options *o);
>   int oneway_merge(const struct cache_entry * const *src,
>   		 struct unpack_trees_options *o);
> +int noop_merge(const struct cache_entry * const *src,
> +	       struct unpack_trees_options *o);
>   
>   #endif
> -- 8< --
> 
> --
> Duy
>