Re: [PATCH v7] submodule merge: update conflict error message

From: Calvin Wan <calvinwan@google.com>
Date: Aug 1, 2022, 22:24 UTC
Message-ID: <CAFySSZBdTvxn8QjOujvu2orkf_tA4Y1nufdjaNJHLsUTWsoo-A@mail.gmail.com>
In-Reply-To: <CABPp-BE9hdzn_8fqSa-JYXS14xbXDEvKi=yupvWDFAUnALhD9A@mail.gmail.com>

Show 6 quoted lines

> > 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

Show 7 quoted lines

> 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.

Show 16 quoted lines

>
> >                 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

Show 41 quoted lines

> 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.