Re: [PATCH 14/14] Build in merge

> Miklos Vajna <vmiklos@frugalware.org> writes:
> 
> > +static int read_tree_trivial(unsigned char *common, unsigned char *head,
> > +	unsigned char *one)
> > +{
> > +	int i, nr_trees = 0;
> > +	struct tree *trees[MAX_UNPACK_TREES];
> > +	struct tree_desc t[MAX_UNPACK_TREES];
> > +	struct unpack_trees_options opts;
> > +
> > +	memset(&opts, 0, sizeof(opts));
> > +	opts.head_idx = 2;
> 
> Do you still need this assignment here?

> > +int cmd_merge(int argc, const char **argv, const char *prefix)
> > +{
> > +	unsigned char result_tree[20];
> > +	struct strbuf buf;
> > +	const char *head_arg;
> > +	int flag, head_invalid = 0, i;
> > +	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
> > +	struct commit_list *common = NULL;
> > +	struct path_list_item *best_strategy = NULL, *wt_strategy = NULL;
> > +	struct commit_list **remotes = &remoteheads;
> > +
> > +	setup_work_tree();
> > +	if (unmerged_cache())
> > +		die("You are in the middle of a conflicted merge.");
> > +
> > +	/*
> > +	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
> > +	 * current branch.
> > +	 */
> > +	branch = resolve_ref("HEAD", head, 0, &flag);
> > +	if (branch && flag & REF_ISSYMREF) {
> > +		const char *ptr = skip_prefix(branch, "refs/heads/");
> > +		if (ptr)
> > +			branch = ptr;
> > +	} else
> > +		head_invalid = 1;
> 
> Wait a minute...  Are you calling a detached HEAD as head_invalid here?  I
> am not too much worried about variable naming, but you later do ...
> 
> > +	if (!have_message && is_old_style_invocation(argc, argv)) {
> > ...
> > +	} else if (head_invalid) {
> > +		struct object *remote_head;
> > +		/*
> > +		 * If the merged head is a valid one there is no reason
> > +		 * to forbid "git merge" into a branch yet to be born.
> > +		 * We do the same for "git pull".
> > +		 */
> > +		if (argc != 1)
> > +			die("Can merge only exactly one commit into "
> > +				"empty head");
> 
> Which is about HEAD pointing at a branch that isn't born yet.  They are
> two very different concepts.
> 
> Either the above "else if (head_invalid)" is wrong, or more likely the
> setting of head_invalid we saw earlier is wrong.
> 
> Probably what you meant was:
> 
> 	- "char *branch" points at either "HEAD" (when detached) or
>           the name of the branch (e.g. "master" when "refs/heads/master");
> 
> 	- "unsigned char head[]" stores the commit object name of the
>           current HEAD (or 0{40} if the current branch is unborn);
> 
>         - set head_invalid to true only when the current branch is unborn.
> 
> So perhaps...
> 
> 	branch = resolve_ref("HEAD", head, 0, &flag);
>         if (branch && (flag & REF_ISSYMREF) && !prefixcmp(branch, "refs/heads/"))
> 		branch += 11;
>         head_invalid = is_null_sha1(head);
> 
> And probably we can even drop (flag & REF_ISSYMREF) from the above safely.
> Do we even care if the head is detached in this program?  I doubt it.

> > +		strbuf_init(&msg, 0);
> > +		strbuf_addstr(&msg, "Fast forward");
> > +		if (have_message)
> > +			strbuf_addstr(&msg,
> > +				" (no commit created; -m option ignored)");
> > +		o = peel_to_type(sha1_to_hex(remoteheads->item->object.sha1),
> > +			0, NULL, OBJ_COMMIT);
> > +		if (!o)
> > +			return 1;
> > +
> > +		if (checkout_fast_forward(head, remoteheads->item->object.sha1))
> > +			return 1;
> 
> Not exiting with 0 status from around here upon error is an improvement,
> but does the user see sensible error messages in addition to the exit
> status?