Re: [PATCHv9 1/6] submodule-config: keep update strategy around

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>> +else {
>> +submodule->update_command = NULL;
>> +if (!strcmp(value, "none"))
>> +submodule->update = SM_UPDATE_NONE;
>> +else if (!strcmp(value, "checkout"))
>> +submodule->update = SM_UPDATE_CHECKOUT;
>> +else if (!strcmp(value, "rebase"))
>> +submodule->update = SM_UPDATE_REBASE;
>> +else if (!strcmp(value, "merge"))
>> +submodule->update = SM_UPDATE_MERGE;
>> +else if (!skip_prefix(value, "!", 
>> >update_command))
>> +die(_("invalid value for %s"), var);
>> +}
>
> I think this "string to enum" parser can become a separate helper
> function in this patch, so that later patch can use it to parse the
> "--update" option in the update_clone() function.
>
> That would solve the slight inconsistency we have seen
>
> + if ((pp->update && !strcmp(pp->update, "none")) ||
> + (!pp->update && sub->update == SM_UPDATE_NONE)) {
>
> in that patch.

In addition, you would eventually want the reverse translation,
i.e. SM_UPDATE_ENUM to string, when you rebase the second patch of
your sb/submodule-init series (I just tried it myself and realized
that a patch to refactor the above part would be the best place to
add such a helper).

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

Re: [PATCHv9 1/6] submodule-config: keep update strategy around

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Feb 9, 2016 at 1:08 PM, Junio C Hamano  wrote:
>>> + } else if (!strcmp(item.buf, "update")) {
>>> + if (!value)
>>> + ret = config_error_nonbool(var);
>>> + else if (!me->overwrite &&
>>> + submodule->update != SM_UPDATE_UNSPECIFIED)
>>
>> Funny indentation here (locally fixable).
>
> I looked through the code base and reread our CodingGuidelines
> to find out what is considered correct. (I assumed we had a gnu-ish
> coding style w.r.t. breaking overly long lines in conditions, which is
> having the next line be indented with 4 spaces.)
>
> So I assume by funny you mean "the next line doesn't start below the
> opening parenthesis"?

I think we typically do one of the two:

if (A &&
B && C && ...)

or

if (A &&
B && C && ...)

That is, the second line may align just inside the open paren on the
first line, or even deeper, but never shallower.
--
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

Re: [PATCHv9 1/6] submodule-config: keep update strategy around

[Junio C Hamano]

Stefan Beller  writes:

> Currently submodule..update is only handled by git-submodule.sh.
> C code will start to need to make use of that value as more of the
> functionality of git-submodule.sh moves into library code in C.
>
> Add the update field to 'struct submodule' and populate it so it can
> be read as sm->update or from sm->update_command.
>
> Signed-off-by: Stefan Beller 
> ---
>  submodule-config.c | 22 ++
>  submodule-config.h | 11 +++
>  2 files changed, 33 insertions(+)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index afe0ea8..a1af5de 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -194,6 +194,8 @@ static struct submodule *lookup_or_create_by_name(struct 
> submodule_cache *cache,
>  
>   submodule->path = NULL;
>   submodule->url = NULL;
> + submodule->update = SM_UPDATE_UNSPECIFIED;
> + submodule->update_command = NULL;
>   submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
>   submodule->ignore = NULL;
>  
> @@ -311,6 +313,26 @@ static int parse_config(const char *var, const char 
> *value, void *data)
>   free((void *) submodule->url);
>   submodule->url = xstrdup(value);
>   }
> + } else if (!strcmp(item.buf, "update")) {
> + if (!value)
> + ret = config_error_nonbool(var);
> + else if (!me->overwrite &&
> + submodule->update != SM_UPDATE_UNSPECIFIED)

Funny indentation here (locally fixable).

Thanks to this change, we do not need "'!= NULL' removal" for this
location in 2/6.

> + warn_multiple_config(me->commit_sha1, submodule->name,
> +  "update");
> + else {
> + submodule->update_command = NULL;
> + if (!strcmp(value, "none"))
> + submodule->update = SM_UPDATE_NONE;
> + else if (!strcmp(value, "checkout"))
> + submodule->update = SM_UPDATE_CHECKOUT;
> + else if (!strcmp(value, "rebase"))
> + submodule->update = SM_UPDATE_REBASE;
> + else if (!strcmp(value, "merge"))
> + submodule->update = SM_UPDATE_MERGE;
> + else if (!skip_prefix(value, "!", 
> >update_command))
> + die(_("invalid value for %s"), var);
> + }
>   }
>  
>   strbuf_release();
> diff --git a/submodule-config.h b/submodule-config.h
> index 9061e4e..e3bd56e 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -4,6 +4,15 @@
>  #include "hashmap.h"
>  #include "strbuf.h"
>  
> +enum submodule_update_type {
> + SM_UPDATE_CHECKOUT,
> + SM_UPDATE_REBASE,
> + SM_UPDATE_MERGE,
> + SM_UPDATE_NONE,
> + SM_UPDATE_COMMAND,
> + SM_UPDATE_UNSPECIFIED
> +};
> +
>  /*
>   * Submodule entry containing the information about a certain submodule
>   * in a certain revision.
> @@ -14,6 +23,8 @@ struct submodule {
>   const char *url;
>   int fetch_recurse;
>   const char *ignore;
> + enum submodule_update_type update;
> + const char *update_command;
>   /* the sha1 blob id of the responsible .gitmodules file */
>   unsigned char gitmodules_sha1[20];
>  };
--
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

Re: [PATCHv9 1/6] submodule-config: keep update strategy around

[Stefan Beller]

On Tue, Feb 9, 2016 at 1:08 PM, Junio C Hamano  wrote:
>> + } else if (!strcmp(item.buf, "update")) {
>> + if (!value)
>> + ret = config_error_nonbool(var);
>> + else if (!me->overwrite &&
>> + submodule->update != SM_UPDATE_UNSPECIFIED)
>
> Funny indentation here (locally fixable).

I looked through the code base and reread our CodingGuidelines
to find out what is considered correct. (I assumed we had a gnu-ish
coding style w.r.t. breaking overly long lines in conditions, which is
having the next line be indented with 4 spaces.)

So I assume by funny you mean "the next line doesn't start below the
opening parenthesis"?

That would seem to be consistent as it fits both the 4 space indentation
which is always found below "if (", but we have more than 4 spaces in
other places such as overly long return statements, i.e. refs.c, l 610
(@origin/master)
return starts_with(refname, "refs/heads/") ||
starts_with(refname, "refs/remotes/") ||
starts_with(refname, "refs/notes/") ||
!strcmp(refname, "HEAD");

which also doesn't seem to align perfectly to me.

Looking for places, which have the pattern "else if (..." with line
break, I found several different styles.

builtin/mv.c (@origin/master) has two occurrences of

else if (...
<2 additional tabs> condition continues here

and another of

else if (...
<1 tab + 1 SP> condition nearly aligned to statement below

Looking at remote.c, I can find 1 tab, plus 3 spaces.
...
Giving up to come up with an idea for space based rule other than
"visually align it below the original statement".

Puzzled,
Stefan
--
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

Re: [PATCHv9 1/6] submodule-config: keep update strategy around

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Signed-off-by: Stefan Beller 
> ---
>  submodule-config.c | 22 ++
>  submodule-config.h | 11 +++
>  2 files changed, 33 insertions(+)

Yay, this is much clearer.  Thanks for indulging.

[...]
> +++ b/submodule-config.c
[...]
> @@ -311,6 +313,26 @@ static int parse_config(const char *var, const char 
> *value, void *data)
> + else if (!skip_prefix(value, "!", 
> >update_command))
> + die(_("invalid value for %s"), var);

Should this say
else if (skip_prefix(value, "!", 
>update_command))
submodule->update = SM_UPDATE_COMMAND;
else
die(...);

?

[...]
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -4,6 +4,15 @@
>  #include "hashmap.h"
>  #include "strbuf.h"
>  
> +enum submodule_update_type {
> + SM_UPDATE_CHECKOUT,
> + SM_UPDATE_REBASE,
> + SM_UPDATE_MERGE,
> + SM_UPDATE_NONE,
> + SM_UPDATE_COMMAND,
> + SM_UPDATE_UNSPECIFIED
> +};

If _UNSPECIFIED is equal to 0, that would futureproof this better
against zero-initialized submodule structs without ->update explicitly
set.

After those two changes and the whitespace change Junio suggested,
Reviewed-by: Jonathan Nieder 
--
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

Re: [PATCHv9 1/6] submodule-config: keep update strategy around

[Junio C Hamano]

Stefan Beller  writes:

> + else {
> + submodule->update_command = NULL;
> + if (!strcmp(value, "none"))
> + submodule->update = SM_UPDATE_NONE;
> + else if (!strcmp(value, "checkout"))
> + submodule->update = SM_UPDATE_CHECKOUT;
> + else if (!strcmp(value, "rebase"))
> + submodule->update = SM_UPDATE_REBASE;
> + else if (!strcmp(value, "merge"))
> + submodule->update = SM_UPDATE_MERGE;
> + else if (!skip_prefix(value, "!", 
> >update_command))
> + die(_("invalid value for %s"), var);
> + }

I think this "string to enum" parser can become a separate helper
function in this patch, so that later patch can use it to parse the
"--update" option in the update_clone() function.

That would solve the slight inconsistency we have seen

+   if ((pp->update && !strcmp(pp->update, "none")) ||
+   (!pp->update && sub->update == SM_UPDATE_NONE)) {

in that patch.
--
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