Hi Junio,
On Mon, 22 Jan 2018, Junio C Hamano wrote:
> 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.
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/<label>. It kind of goes without saying that
those need to be valid ref names, then?
> > + 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.
I agree that write_message() is not a good name, but write_file() was
already taken. I do not think wrapper.c:write_file() is more appropriate,
as it has *also* misnamed: it *always completes the line*. In other words,
it is not write_file(), it is write_line_to_file(). And not even that, as
it takes a printf()-type format.
No, write_message(), even if not named appropriately, is the functionality
I want.
> 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.
write_file() does different things than write_message(). I think this
suggestion to congeal them into one is quite overzealous.
And would also lead us astray.
> > + 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?
Yes. You explained the reason yourself: this is `git merge`'s behavior. To
recreate it in the sequencer, the list of common ancestors has to be
reversed here, too.
Ciao,
Dscho
