Re: [PATCH 2/8] sequencer: introduce the `merge` command

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> >  	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> > +	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> > +	item->arg_len = (int)(eol - item->arg);
> > +
> >  	saved = *end_of_object_name;
> > +	if (item->command == TODO_MERGE && *bol == '-' &&
> > +	    bol + 1 == end_of_object_name) {
> > +		item->commit = NULL;
> > +		return 0;
> > +	}
> > +
> >  	*end_of_object_name = '\0';
> >  	status = get_oid(bol, &commit_oid);
> >  	*end_of_object_name = saved;
> >  
> > -	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> > -	item->arg_len = (int)(eol - item->arg);
> > -
> 
> Assigning to "saved" before the added "if we are doing merge and see
> '-', do this special thing" is not only unnecessary, but makes the
> logic in the non-special case harder to read.  The four things
> "saved = *eol; *eol = 0; do_thing_using(bol); *eol = saved;" is a
> single logical unit; keep them together.

> > +		if (*p)
> > +			len = strlen(p);
> > +		else {
> > +			strbuf_addf(&buf, "Merge branch '%.*s'",
> > +				    merge_arg_len, arg);
> > +			p = buf.buf;
> > +			len = buf.len;
> > +		}
> 
> So... "arg" received by this function can be a single non-whitespace
> token, which is taken as the name of the branch being merged (in
> this else clause).  Or it can also be followed by a single liner
> message for the merge commit.  Presumably, this is for creating a
> new merge (i.e. "commit==NULL" case), and preparing a proper log
> message in the todo list is unrealistic, so this would be a
> reasonable compromise.  Those users who want to write proper log
> message could presumably follow such "merge" insn with a "x git
> commit --amend" or something, I presume, if they really wanted to.

> > +		if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> > +			error_errno(_("Could not write '%s'"),
> > +				    git_path_merge_msg());
> > +			strbuf_release(&buf);
> > +			rollback_lock_file(&lock);
> > +			return -1;
> > +		}
> > +		strbuf_release(&buf);
> > +	}
> 
> OK.  Now we have prepared the MERGE_MSG file and are ready to commit.
> 
> > +	head_commit = lookup_commit_reference_by_name("HEAD");
> > +	if (!head_commit) {
> > +		rollback_lock_file(&lock);
> > +		return error(_("Cannot merge without a current revision"));
> > +	}
> 
> Hmph, I would have expected to see this a lot earlier, before
> dealing with the log message.  Leftover MERGE_MSG file after an
> error will cause unexpected fallout to the end-user experience
> (including what is shown by the shell prompt scripts), but if we do
> this before the MERGE_MSG thing, we do not have to worry about
> error codepath having to remove it.

> > +	strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
> > +	merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> > +	if (!merge_commit) {
> > +		/* fall back to non-rewritten ref or commit */
> > +		strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0);
> > +		merge_commit = lookup_commit_reference_by_name(ref_name.buf);
> > +	}
> 
> OK, so "abc" in the example in the log message is looked up first as
> a label and then we take a fallback to interpret as an object name.

> > +	if (!merge_commit) {
> > +		error(_("could not resolve '%s'"), ref_name.buf);
> > +		strbuf_release(&ref_name);
> > +		rollback_lock_file(&lock);
> > +		return -1;
> > +	}
> > +	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
> > +		      git_path_merge_head(), 0);
> > +	write_message("no-ff", 5, git_path_merge_mode(), 0);
> 
> These two calls gave me a "Huh?" moment; write_message() sounds like
> it is allowed to be later updated with frills suitable for *_MSG
> files we place in .git/ directory (iow, it is in principle OK if
> commented out instructions common to these files are added to the
> output by the function), but these want exact bytes passed in the
> result, for which wrapper.c::write_file() is more appropriate.

> > +	common = get_merge_bases(head_commit, merge_commit);
> > +	for (j = common; j; j = j->next)
> > +		commit_list_insert(j->item, &reversed);
> > +	free_commit_list(common);
> 
> I know this is copy&pasted code from "builtin/merge.c", but is there
> a reason to reverse the common ancestor list here?