Re: [PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 5, 2018, 23:32 UTC
Message-ID: <20180905233203.GE120842@aiede.svl.corp.google.com>
In-Reply-To: <20180905231952.28145-2-sbeller@google.com>

Hi,

Stefan Beller wrote:

> Subject: submodule.c: warn about missing submodule commit in recursive actions

Nit: the diff already tells me what file the change is in.  What I'd
be more interested in is the subsystem or what commands this affects.
Does this affect all --recurse-submodules commands, or just some?

Here, I think it's about common submodule code, so I guess 'submodule:' would be a fine prefix.

Show 6 quoted lines

> By checking if a submodule commit exists before attempting the update
> we can improve the error message from the
>     error(_("Submodule '%s' could not be updated."), path);
> to the new and more specific
>     error(_("Submodule '%s' doesn't have commit '%s'"),
>           path, oid_to_hex(new_oid));

Maybe it's just me, but I find this formatting where I cannot distinguish between a line that was wrapped early and the start of a callout hard to read. Some extra line breaks would help:

  By checking if a submodule commit exists before attempting the update
  we can improve the error message from the

      error(_("Submodule '%s' could not be updated."), path);

  to the new and more specific

      error(_("Submodule '%s' doesn't have commit '%s'"),
            path, oid_to_hex(new_oid));

Beyond that, I still don't know what this change does. Can you give an example? For example, what command would I run before and what bad result would I get, and what result does this patch produce instead?

Thanks, Jonathan