Re: [PATCH v7] submodule merge: update conflict error message
From: Calvin Wan <hidden>
Date: 2022-08-01 22:24:51
quoted
4. Submodule not checked out: Still returns early, but added a NEEDSWORK bit since current error message does not reflect the correct resolutions/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.
quoted
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.