From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 05 Sep 2018 23:32:03 GMT
Subject: Re: [PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions
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.

> 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