From: Calvin Wan <calvinwan@google.com>
To: Elijah Newren <newren@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
"Junio C Hamano" <gitster@pobox.com>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"Ævar Arnfjörð" <avarab@gmail.com>
Subject: Re: [PATCH v7] submodule merge: update conflict error message
Date: Mon, 1 Aug 2022 15:24:33 -0700 [thread overview]
Message-ID: <CAFySSZBdTvxn8QjOujvu2orkf_tA4Y1nufdjaNJHLsUTWsoo-A@mail.gmail.com> (raw)
In-Reply-To: <CABPp-BE9hdzn_8fqSa-JYXS14xbXDEvKi=yupvWDFAUnALhD9A@mail.gmail.com>
> > 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.
next prev parent reply other threads:[~2022-08-01 22:25 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-06 23:54 [PATCH] submodule merge: update conflict error message Calvin Wan
2022-06-07 0:48 ` Junio C Hamano
2022-06-08 17:19 ` Calvin Wan
2022-06-08 17:34 ` Glen Choo
2022-06-08 18:01 ` Calvin Wan
2022-06-08 19:13 ` Junio C Hamano
2022-06-10 23:11 ` [PATCH v2] " Calvin Wan
2022-06-11 4:53 ` Elijah Newren
2022-06-11 17:08 ` Philippe Blain
2022-06-12 6:56 ` Elijah Newren
2022-06-13 2:03 ` Calvin Wan
2022-06-12 23:30 ` Junio C Hamano
2022-06-13 1:54 ` Calvin Wan
2022-06-29 22:40 ` [PATCH v3] " Calvin Wan
2022-06-30 2:40 ` Elijah Newren
2022-06-30 19:48 ` Calvin Wan
2022-07-01 4:27 ` Elijah Newren
2022-06-30 20:35 ` Glen Choo
2022-06-30 20:45 ` Glen Choo
2022-06-30 21:08 ` Calvin Wan
2022-07-12 23:19 ` [PATCH v4] " Calvin Wan
2022-07-13 18:11 ` Junio C Hamano
2022-07-17 2:46 ` Elijah Newren
2022-07-15 12:57 ` Johannes Schindelin
2022-07-16 6:22 ` Junio C Hamano
2022-07-17 2:44 ` Elijah Newren
2022-07-18 17:03 ` Calvin Wan
2022-07-18 21:43 ` [PATCH v5] " Calvin Wan
2022-07-19 6:39 ` Junio C Hamano
2022-07-19 19:30 ` Calvin Wan
2022-07-19 20:16 ` Junio C Hamano
2022-07-19 7:13 ` Junio C Hamano
2022-07-19 19:07 ` Calvin Wan
2022-07-19 20:30 ` Junio C Hamano
2022-07-25 6:05 ` Ævar Arnfjörð Bjarmason
2022-07-25 12:11 ` Ævar Arnfjörð Bjarmason
2022-07-25 22:03 ` Calvin Wan
2022-07-25 12:31 ` Ævar Arnfjörð Bjarmason
2022-07-25 21:27 ` Calvin Wan
2022-07-26 21:00 ` [PATCH v6] " Calvin Wan
2022-07-27 1:13 ` Elijah Newren
2022-07-27 22:00 ` Calvin Wan
2022-07-28 0:41 ` Elijah Newren
2022-07-28 19:06 ` Calvin Wan
2022-07-27 9:20 ` Ævar Arnfjörð Bjarmason
2022-07-28 21:12 ` [PATCH v7] " Calvin Wan
2022-07-28 23:22 ` Ævar Arnfjörð Bjarmason
2022-07-29 0:24 ` Elijah Newren
2022-08-01 22:24 ` Calvin Wan [this message]
2022-08-01 12:06 ` Ævar Arnfjörð Bjarmason
2022-08-02 0:50 ` Junio C Hamano
2022-08-02 21:03 ` Calvin Wan
2022-08-02 21:11 ` Junio C Hamano
2022-08-02 21:55 ` Calvin Wan
2022-08-02 22:22 ` Junio C Hamano
2022-08-04 19:51 ` [PATCH v8] " Calvin Wan
2022-08-16 15:58 ` Junio C Hamano
2022-08-16 18:58 ` Junio C Hamano
2022-08-16 19:34 ` Calvin Wan
2022-08-16 19:39 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFySSZBdTvxn8QjOujvu2orkf_tA4Y1nufdjaNJHLsUTWsoo-A@mail.gmail.com \
--to=calvinwan@google.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).