Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

2018-10-08 Thread Antonio Ospite
On Sun, 07 Oct 2018 08:44:20 +0900
Junio C Hamano  wrote:

> Stefan Beller  writes:
> 
> >>  static int module_config(int argc, const char **argv, const char *prefix)
> >>  {
> >> +   enum {
> >> +   CHECK_WRITEABLE = 1
> >> +   } command = 0;
> >
> > Can we have the default named? Then we would only use states
> > from within the enum?
> 
> Why?  Do we use a half-intelligent "switch () { case ...: ... }"
> checker that would otherwise complain if we handled "case 0" in such
> a switch statement, or something like that?
> 
> Are we going to gain a lot more enum members, by the way?  At this
> point, this looks more like a
> 
>   unsigned check_writable = 0; /* default is not to check */
> 
> to me.

Hi,

the CHECK_WRITEABLE operation is alternative to the get/set ones, not
an addition, so I can see the rationale behind Stefan's suggestion:
either have named enums members for all command "modes" or for none of
them; however other users of enum+OPT_CMDMODE seems to think like the
enum is for commands passed as *options* and the unnamed default is for
actions derived from *arguments*. I don't have a strong opinion on this
matter, tho, so just tell me what you prefer and I'll do it for v7.

Using an enum was to have a more explicit syntax in case other commands
were going to be added in the future (I imagine "--stage" or
"--list-all" as possible additions), and does not affect the generated
code, so I though it was worth it.

Anyways, these are really details, let's concentrate on patches 9 and
10 which deserve much more attention. :)

Thanks you,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?


Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

2018-10-06 Thread Junio C Hamano
Stefan Beller  writes:

>>  static int module_config(int argc, const char **argv, const char *prefix)
>>  {
>> +   enum {
>> +   CHECK_WRITEABLE = 1
>> +   } command = 0;
>
> Can we have the default named? Then we would only use states
> from within the enum?

Why?  Do we use a half-intelligent "switch () { case ...: ... }"
checker that would otherwise complain if we handled "case 0" in such
a switch statement, or something like that?

Are we going to gain a lot more enum members, by the way?  At this
point, this looks more like a

unsigned check_writable = 0; /* default is not to check */


to me.


Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

2018-10-06 Thread Antonio Ospite
On Fri, 5 Oct 2018 16:50:10 -0700
Stefan Beller  wrote:

> >  static int module_config(int argc, const char **argv, const char *prefix)
> >  {
> > +   enum {
> > +   CHECK_WRITEABLE = 1
> > +   } command = 0;
> 
> Can we have the default named? Then we would only use states
> from within the enum?

The default would mean:

  "no command passed as a CLI *option*"

I copied this style from builtin/bisect--helper.c::cmd_bisect__helper()
and it's also used in builtin/rebase--helper.c

I can add a name for the default enum value but I am not sure what it
should be: NO_COMMAND_OPTION, COMMAND_DEFAULT, MODE_DEFAULT?

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?


Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

2018-10-05 Thread Stefan Beller
>  static int module_config(int argc, const char **argv, const char *prefix)
>  {
> +   enum {
> +   CHECK_WRITEABLE = 1
> +   } command = 0;

Can we have the default named? Then we would only use states
from within the enum?


[PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules

2018-10-05 Thread Antonio Ospite
Introduce a helper function named is_writing_gitmodules_ok() to verify
that the .gitmodules file is safe to write.

The function name follows the scheme of is_staging_gitmodules_ok().

The two symbolic constants GITMODULES_INDEX and GITMODULES_HEAD are used
to get help from the C preprocessor in preventing typos, especially for
future users.

This is in preparation for a future change which teaches git how to read
.gitmodules from the index or from the current branch if the file is not
available in the working tree.

The rationale behind the check is that writing to .gitmodules requires
the file to be present in the working tree, unless a brand new
.gitmodules is being created (in which case the .gitmodules file would
not exist at all: neither in the working tree nor in the index or in the
current branch).

Expose the functionality also via a "submodule-helper config
--check-writeable" command, as git scripts may want to perform the check
before modifying submodules configuration.

Signed-off-by: Antonio Ospite 
---
 builtin/submodule--helper.c | 24 +++-
 cache.h |  2 ++
 submodule.c | 18 ++
 submodule.h |  1 +
 t/t7411-submodule-config.sh | 31 +++
 5 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e1bdca8f0b..28f3ccca6d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2127,6 +2127,28 @@ static int check_name(int argc, const char **argv, const 
char *prefix)
 
 static int module_config(int argc, const char **argv, const char *prefix)
 {
+   enum {
+   CHECK_WRITEABLE = 1
+   } command = 0;
+
+   struct option module_config_options[] = {
+   OPT_CMDMODE(0, "check-writeable", ,
+   N_("check if it is safe to write to the .gitmodules 
file"),
+   CHECK_WRITEABLE),
+   OPT_END()
+   };
+   const char *const git_submodule_helper_usage[] = {
+   N_("git submodule--helper config name [value]"),
+   N_("git submodule--helper config --check-writeable"),
+   NULL
+   };
+
+   argc = parse_options(argc, argv, prefix, module_config_options,
+git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+   if (argc == 1 && command == CHECK_WRITEABLE)
+   return is_writing_gitmodules_ok() ? 0 : -1;
+
/* Equivalent to ACTION_GET in builtin/config.c */
if (argc == 2)
return print_config_from_gitmodules(the_repository, argv[1]);
@@ -2135,7 +2157,7 @@ static int module_config(int argc, const char **argv, 
const char *prefix)
if (argc == 3)
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
 
-   die("submodule--helper config takes 1 or 2 arguments: name [value]");
+   usage_with_options(git_submodule_helper_usage, module_config_options);
 }
 
 #define SUPPORT_SUPER_PREFIX (1<<0)
diff --git a/cache.h b/cache.h
index d508f3d4f8..2d495fc800 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,8 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
 #define GITMODULES_FILE ".gitmodules"
+#define GITMODULES_INDEX ":.gitmodules"
+#define GITMODULES_HEAD "HEAD:.gitmodules"
 #define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
 #define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
 #define GIT_NOTES_DISPLAY_REF_ENVIRONMENT "GIT_NOTES_DISPLAY_REF"
diff --git a/submodule.c b/submodule.c
index 3b23e76e55..bd2506c5ba 100644
--- a/submodule.c
+++ b/submodule.c
@@ -51,6 +51,24 @@ int is_gitmodules_unmerged(const struct index_state *istate)
return 0;
 }
 
+/*
+ * Check if the .gitmodules file is safe to write.
+ *
+ * Writing to the .gitmodules file requires that the file exists in the
+ * working tree or, if it doesn't, that a brand new .gitmodules file is going
+ * to be created (i.e. it's neither in the index nor in the current branch).
+ *
+ * It is not safe to write to .gitmodules if it's not in the working tree but
+ * it is in the index or in the current branch, because writing new values
+ * (and staging them) would blindly overwrite ALL the old content.
+ */
+int is_writing_gitmodules_ok(void)
+{
+   struct object_id oid;
+   return file_exists(GITMODULES_FILE) ||
+   (get_oid(GITMODULES_INDEX, ) < 0 && 
get_oid(GITMODULES_HEAD, ) < 0);
+}
+
 /*
  * Check if the .gitmodules file has unstaged modifications.  This must be
  * checked before allowing modifications to the .gitmodules file with the
diff --git a/submodule.h b/submodule.h
index e452919aa4..7a22f71cb9 100644
--- a/submodule.h
+++ b/submodule.h
@@ -40,6 +40,7 @@ struct submodule_update_strategy {
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
 int