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

[Keller, Jacob E]

On Fri, 2016-08-26 at 11:19 -0700, Junio C Hamano wrote:
> Jacob Keller  writes:
> 
> > 
> > Currently, do_submodule_path will attempt locating the .git
> > directory by
> > using read_gitfile on /.git. If this fails it just assumes
> > the
> > /.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.

> > 
> > Unfortunately this fails if the  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.

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

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

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

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

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

[Junio C Hamano]

Jacob Keller  writes:

> Currently, do_submodule_path will attempt locating the .git directory by
> using read_gitfile on /.git. If this fails it just assumes the
> /.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  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html