Johannes Schindelin <johannes.schindelin@xxxxxx> 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.
This hunk may have been the most expedient way to coax "-" into the
location where a commit object name is expected; it looks ugly, but
for the limited purpose of this series it should do.
> @@ -2069,6 +2077,132 @@ static int do_reset(const char *name, int len)
> return ret;
> }
>
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> + struct replay_opts *opts)
> +{
> + int merge_arg_len;
> + struct strbuf ref_name = STRBUF_INIT;
> + struct commit *head_commit, *merge_commit, *i;
> + struct commit_list *common, *j, *reversed = NULL;
> + struct merge_options o;
> + int ret;
> + static struct lock_file lock;
> +
> + for (merge_arg_len = 0; merge_arg_len < arg_len; merge_arg_len++)
> + if (isspace(arg[merge_arg_len]))
> + break;
Mental note: this scans for a whitespace, and tab is accepted
instead of SP, which presumably is to allow human typed string.
> + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> + return -1;
> +
> + if (commit) {
> + const char *message = get_commit_buffer(commit, NULL);
> + const char *body;
> + int len;
> +
> + if (!message) {
> + rollback_lock_file(&lock);
> + return error(_("could not get commit message of '%s'"),
> + oid_to_hex(&commit->object.oid));
> + }
> + write_author_script(message);
> + find_commit_subject(message, &body);
> + len = strlen(body);
> + if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
> + error_errno(_("Could not write '%s'"),
> + git_path_merge_msg());
> + unuse_commit_buffer(commit, message);
> + rollback_lock_file(&lock);
> + return -1;
> + }
> + unuse_commit_buffer(commit, message);
> + } else {
> + const char *p = arg + merge_arg_len;
> + struct strbuf buf = STRBUF_INIT;
> + int len;
> +
> + strbuf_addf(&buf, "author %s", git_author_info(0));
> + write_author_script(buf.buf);
> + strbuf_reset(&buf);
> +
> + p += strspn(p, " \t");
... and this matches the above mental note. It allows consecutive
whitespaces as a separator, which is sensible behaviour.
> + 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.
Hopefully allowed names in "label" would be documented clearly in
later steps (I am guessing that "a name that can be used as a branch
name can be used as a label name and vice versa" or something like
that).
> + 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.
Alternatively, perhaps write_message() can be dropped and its
callers can call wrapper.c::write_file() instead? Such a clean-up
may require teaching the append-eol thing that write_message() wants
to wrapper.c::write_file(), but it shouldn't be a rocket science.
> + 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?
> + read_cache();
> + init_merge_options(&o);
> + o.branch1 = "HEAD";
> + o.branch2 = ref_name.buf;
> + o.buffer_output = 2;
> +
> + ret = merge_recursive(&o, head_commit, merge_commit, reversed, &i);
> + if (ret <= 0)
> + fputs(o.obuf.buf, stdout);
> +...
Other than these minor nits, looks quite promising. Nicely done.
