Re: [PATCH v11 5/8] allow do_submodule_path to work even if submodule isn't checked out

Jacob Keller [off-list ref] writes:

Currently, do_submodule_path will attempt locating the .git directory by
using read_gitfile on <path>/.git. If this fails it just assumes the
<path>/.git is actually a git directory.

This is good because it allows for handling submodules which were cloned
in a regular manner first before being added to the parent project.

s/parent project/superproject/;

Unfortunately this fails if the <path> is not actually checked out any
longer, such as by removing the directory.

Fix this by checking if the directory we found is actually a gitdir. In
the case it is not, attempt to lookup the submodule configuration and
find the name of where it is stored in the .git/modules/ folder of the
parent project.

As you consistently say "directory" in the earlier part of the log
message,

s/folder of the parent project/directory of the superproject/;

is desired.

If we can't locate the submodule configuration this might occur because

I added s/configuration/&,/ to make it a bit easier to read.

for example a submodule gitlink was added but the corresponding
.gitmodules file was not properly updated. A die() here would not be
pleasant to the users of submodule diff formats, so instead, modify
do_submodule_path to return an error code. For git_pathdup_submodule,
just return NULL when we fail to find a path. For strbuf_git_path_submodule
propagate the error code to the caller.

Somehow I had to read the latter half of this paragraph twice,
before I realized that the last two sentence talks about how these
two functions exactly do "to return an error code".  Tentatively
what I queued has:

    ... so instead, modify do_submodule_path() to return an error code:

     - git_pathdup_submodule() returns NULL when we fail to find a path.
     - strbuf_git_path_submodule() propagates the error code to the
       caller.

instead, hoping that would be easier to understand.

-static void do_submodule_path(struct strbuf *buf, const char *path,
-			      const char *fmt, va_list args)
+/* Returns 0 on success, non-zero on failure. */

s/non-zero/negative/;

+#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
+static int do_submodule_path(struct strbuf *buf, const char *path,
+			     const char *fmt, va_list args)
 {

This 5/8 is the only changed one in the entire series from the
previous round, I think.  I'll replace what was in 'pu' and wait for
responses.

Thanks.

On Fri, 2016-08-26 at 11:19 -0700, Junio C Hamano wrote:

Jacob Keller [off-list ref] writes:

quoted

Currently, do_submodule_path will attempt locating the .git
directory by
using read_gitfile on <path>/.git. If this fails it just assumes
the
<path>/.git is actually a git directory.

This is good because it allows for handling submodules which were
cloned
in a regular manner first before being added to the parent project.

s/parent project/superproject/;

Yep.

quoted

Unfortunately this fails if the <path> is not actually checked out
any
longer, such as by removing the directory.

Fix this by checking if the directory we found is actually a
gitdir. In
the case it is not, attempt to lookup the submodule configuration
and
find the name of where it is stored in the .git/modules/ folder of
the
parent project.

As you consistently say "directory" in the earlier part of the log
message,

s/folder of the parent project/directory of the superproject/;

is desired.

Yep.

quoted

If we can't locate the submodule configuration this might occur
because

I added s/configuration/&,/ to make it a bit easier to read.

Makes sense.

quoted

for example a submodule gitlink was added but the corresponding
.gitmodules file was not properly updated. A die() here would not
be
pleasant to the users of submodule diff formats, so instead, modify
do_submodule_path to return an error code. For
git_pathdup_submodule,
just return NULL when we fail to find a path. For
strbuf_git_path_submodule
propagate the error code to the caller.

Somehow I had to read the latter half of this paragraph twice,
before I realized that the last two sentence talks about how these
two functions exactly do "to return an error code".  Tentatively
what I queued has:

    ... so instead, modify do_submodule_path() to return an error
code:

     - git_pathdup_submodule() returns NULL when we fail to find a
path.
     - strbuf_git_path_submodule() propagates the error code to the
       caller.

instead, hoping that would be easier to understand.

That's much better and more clear. Thanks!

quoted

-static void do_submodule_path(struct strbuf *buf, const char
*path,
-			      const char *fmt, va_list args)
+/* Returns 0 on success, non-zero on failure. */

s/non-zero/negative/;

True.

quoted

+#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
+static int do_submodule_path(struct strbuf *buf, const char *path,
+			     const char *fmt, va_list args)
 {

This 5/8 is the only changed one in the entire series from the
previous round, I think.  I'll replace what was in 'pu' and wait for
responses.

Thanks.

Yep, that's correct.

Regards,
Jake