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

From: Calvin Wan <calvinwan@google.com>
Date: Aug 2, 2022, 21:03 UTC
Message-ID: <CAFySSZCEWX22h1FBX65=eofNm+WU97DtL7P+11WFie72DQLDkg@mail.gmail.com>
In-Reply-To: <xmqqpmhjjwo9.fsf@gitster.g>

Show 12 quoted lines

> > @@ -570,6 +574,8 @@ static const char *type_short_descriptions[] = {
> >               "CONFLICT (submodule history not available)",
> >       [CONFLICT_SUBMODULE_MAY_HAVE_REWINDS] =
> >               "CONFLICT (submodule may have rewinds)",
>
> The other ones are semi sentences ...
>
> > +     [CONFLICT_SUBMODULE_NULL_MERGE_BASE] =
> > +             "CONFLICT (submodule no merge base)"
>
> ... and this wants to become one, too, e.g. "submodule lacks merge
> base", perhaps?

lacks/missing both SGTM

>         if (is_null_oid(a) || is_null_oid(b))
>                 BUG(...);
>
> may be easier to read, perhaps?

ack

> I wonder where this overly deep indentation come from?  Can the
> .editorconfig file we ship with the project help?

It comes from me not properly indenting the line. I'll take a look at the .editorconfig file.

> When we say "either merge commit %s" (where %s is 'b'---what they
> have as we saw earlier), do we need to mention what the value of 'a'
> is to the user, or is it redundant because we are absolutely sure
> that 'a' is what is checked out in the submodule?

We are absolutely sure that 'a' is the GITLINK to the submodule, but if the user changes their local submodule without updating the reference, it can result in errors like "not checked out" and "commits not present" during the merge.

Show 5 quoted lines

> > +                     strbuf_addf(&tmp, _("or update to an existing commit which has merged those changes"));
>
> "those changes" means "a..b"?  Again, I just want to make sure that
> the user in this situation knows "the other end" of the range when
> they are told only about 'b'.

Correct. Based on what I said above, it might be worthwhile to mention 'a' as well since that information could also help the user in those cases. I'm thinking something like this:

"go to submodule ('sub' : 'a'), and either merge commit 'b'\n" "go to submodule ('sub', 'a'), and either merge commit 'b'\n" "go to submodule 'sub', commit 'a', and either merge commit 'b'\n"