> > 4. Submodule not checked out: Still returns early, but added a
> > NEEDSWORK bit since current error message does not reflect the
> > correct resolution
>
> s/does not reflect the correct resolution/also deserves a more
> detailed explanation of how to resolve/ ?
ack
> Thanks. Including a range-diff in the cover letter would be really
> helpful, for future reference.
will do
> Um, repo_submodule_init() returns 0 on success, non-zero upon failure.
> So !sub_initialized means "submodule IS initialized", though it
> appears to read to mean the opposite. Can you consider renaming your
> variable (maybe just to "sub_not_initialized")?
ack
> Technically, we just did generate an error message ("Failed to merge
> submodule %s (not checked out)").
>
> Maybe replace "immediately rather than generating an error message"
> with "immediately. It would be better to "goto cleanup" here, after
> setting some flag requesting a more detailed message be saved for
> print_submodule_conflict_suggetion()". Or something like that.
Sounds like a better place to put the NEEDSWORK bit.
>
> > return 0;
> > }
> >
> > + if (is_null_oid(o)) {
> > + path_msg(opt, CONFLICT_SUBMODULE_NULL_MERGE_BASE, 0,
> > + path, NULL, NULL, NULL,
> > + _("Failed to merge submodule %s (no merge base)"),
> > + path);
> > + goto cleanup;
> > + }
>
> Does this need to be moved after initializing the submodule? I
> thought that was the point of introducing the sub_initialized
> variable, and we're clearly not going to use it, so it would seem to
> make more sense to not initialize it for this case.
The submodule needs to be initialized to generate part of the error
message. See the following in cleanup:
repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV)
> Yuck. I'm not a translator, so maybe what you are doing is preferred.
> But wouldn't translators find it annoying to have to translate " - %s"
> and " %s" in all these places (and wouldn't there need to be a
> TRANSLATORS comment before each and every one)?
As per Avar's suggestion, I made a helper function so translators
only have to translate " - %s" and " %s" once. This change does
end up lining up the `git add ...` command, but I think that's fine
- come back to superproject and run:
git add sub3 sub2 sub
to record the above merge or update
> I also find the big block of code somewhat painful to read. Could we
> instead do something like (note, I have both a tmp and tmp2):
>
> strbuf_add_separated_string_list(&tmp2, " ", csub);
>
> for_each_string_list_item(item, csub) {
> const char *abbrev= item->util;
> /*
> * TRANSLATORS: This is a line of advice to resolve a merge conflict
> * in a submodule. The second argument is the abbreviated id of the
> * commit that needs to be merged.
> * E.g. - go to submodule (sub), and either merge commit abc1234
> */
> strbuf_addf(&tmp, _(" - go to submodule %s, and either merge
> commit %s\n"
> " or update to an existing commit which
> has merged those changes\n"),
> item->string, abbrev);
> }
>
> strbuf_addf(&msg,
> _("Recursive merging with submodules currently only
> supports trivial cases.\n"
> "Please manually handle the merging of each
> conflicted submodule.\n"
> "This can be accomplished with the following steps:\n"
> "%s"
> " - come back to superproject and run:\n\n"
> " git add %s\n\n"
> " to record the above merge or update\n"
> " - resolve any other conflicts in the superproject\n"
> " - commit the resulting index in the superproject\n"),
> tmp.buf, tmp2.buf);
>
> This will give translators precisely two messages to translate (and we
> can't drop it to one since one of the two is repeated a variable
> number of times), and provide more built-in context about how to
> translate since the whole message is involved. If one of the messages
> translates into something especially long, they can even add line
> breaks and reflow the paragraph in ways that make sense for them,
> which your current version just doesn't permit.
I think Avar's suggestion makes translating the formatting easier than
your suggestion, at the cost of having a big block of code to setup it
all up.
