From: Johannes Schindelin Date: Mon, 29 Jan 2018 22:15:50 GMT Subject: Re: [PATCH 2/8] sequencer: introduce the `merge` command Message-ID: In-Reply-To: Hi Junio, On Mon, 22 Jan 2018, Junio C Hamano wrote: > Johannes Schindelin 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. True. This was a sloppily resolved merge conflict in one of the many rewrites, I guess. > > + 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. Precisely. > > + 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. Fixed. > > + 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. Yes. And auto-generated labels are guaranteed not to be full hex hashes for that reason. > 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). Well, I thought that it would suffice to say that these labels are available as refs/rewritten/