Disallow git commands from within unpopulated submodules

PATCH In review Prathamesh Chavan 2 participants 4 messages Apr 6, 2017

lore

Share & tools

lore

Prathamesh Chavan

author

2017-04-06 06:03 UTC #0 lore raw

PATCH 1/1 Disallow git commands from within unpopulated submodules

The main motivations for disallowing git commands within an unpopulated submodule are:

Whenever we run "git -C status" within an unpopulated submodule, it falls back to the superproject. This occurs since there is no .git file in the submodule directory. So superproject's status gets displayed. Also, the user's intention is not clear behind running the command in an unpopulated submodule. Hence we prefer to error out.

When we run the command "git -C sub add ." within a submodule, the results observed are:

In the case of the populated submodule, it acts like running “git add .“ inside the submodule. This is uncontroversial and runs as expected.

In the case of the unpopulated submodule, the user's intention behind entering the above command is unclear. He may have intended to add the submodule to the superproject or to add all files inside the sub/ directory to the submodule or superproject. Hence we’ll prefer to error out in these case.

Eventually, we use a check_prefix_inside_submodule to see check if the path is inside an unpopulated submodule. If it is, then we report the user about the unpopulated submodule.

Signed-off-by Prathamesh Chavan <[email protected]>

---

Since this patch effectively uses RUN_SETUP, builtin commands like 'diff' and other non-builtin commands are not filtered. For such cases, I think, we need to handle them separately.

Also since currently, git-submodule is not a builtin command, the command for initializing and updating the submodule doesn't return an error message, but once it is converted to builtin, we need to handle its case explicitly.

The build report of this patch is available on:
https://travis-ci.org/pratham-pc/git/builds/219030999

Also, the above patch was initially my GSoC project topic, but I changed it later on and added these bug fixes to my wishlist of the proposal.

 builtin/submodule--helper.c      |  4 ----
 git.c                            |  3 +++
 submodule.c                      | 45 ++++++++++++++++++++++++++++++++++++++++
 submodule.h                      |  6 ++++++
 t/t6134-pathspec-in-submodule.sh |  2 +-
 5 files changed, 55 insertions(+), 5 deletions(-)

builtin/submodule--helper.c +0 -4

index 15a5430c0..4f7c7d7b8 100644
@@ -219,10 +219,6 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-struct module_list {
-	const struct cache_entry **entries;
-	int alloc, nr;
-};
 #define MODULE_LIST_INIT { NULL, 0, 0 }
 
 static int module_list_compute(int argc, const char **argv,

git.c +3 -0

index 33f52acbc..eefe3fb01 100644
@@ -2,6 +2,7 @@
 #include "exec_cmd.h"
 #include "help.h"
 #include "run-command.h"
+#include "submodule.h"
 
 const char git_usage_string[] =
 	"git [--version] [--help] [-C <path>] [-c name=value]\n"

@@ -364,6 +365,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 		if (prefix)
 			die("can't use --super-prefix from a subdirectory");
 	}
+	if (prefix)
+		check_prefix_inside_submodule(prefix);
 
 	if (!help && p->option & NEED_WORK_TREE)
 		setup_work_tree();

submodule.c +45 -0

51 lines not on this page · raw message

submodule.h +6 -0

index 05ab674f0..5e41e5afc 100644
@@ -31,6 +31,12 @@ struct submodule_update_strategy {
 };
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
+struct module_list {
+	const struct cache_entry **entries;
+	int alloc, nr;
+};
+
+extern void check_prefix_inside_submodule(const char *prefix);
 extern int is_staging_gitmodules_ok(void);
 extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 extern int remove_path_from_gitmodules(const char *path);

t/t6134-pathspec-in-submodule.sh +1 -1

index fd401ca60..086cc4c47 100755
@@ -25,7 +25,7 @@ test_expect_success 'error message for path inside submodule' '
 '
 
 cat <<EOF >expect
-fatal: Pathspec '.' is in submodule 'sub'
+fatal: command from inside unpopulated submodule 'sub' not supported.
 EOF
 
 test_expect_success 'error message for path inside submodule from within submodule' '

Signature · 1 line

2.11.0

Prathamesh Chavan

author

2017-04-06 06:13 UTC #1 lore raw

PATCH 1/1 Disallow git commands from within unpopulated submodules

The main motivations for disallowing git commands within an unpopulated submodule are:

When we run the command "git -C sub add ." within a submodule, the results observed are:

In the case of the populated submodule, it acts like running “git add .“ inside the submodule. This is uncontroversial and runs as expected.

Eventually, we use a check_prefix_inside_submodule to see check if the path is inside an unpopulated submodule. If it is, then we report the user about the unpopulated submodule.

Signed-off-by Prathamesh Chavan <[email protected]>

---

Since this patch effectively uses RUN_SETUP, builtin commands like 'diff' and other non-builtin commands are not filtered. For such cases, I think, we need to handle them separately.

The build report of this patch is available on:
https://travis-ci.org/pratham-pc/git/builds/219030999

Also, the above patch was initially my GSoC project topic, but I changed it later on and added these bug fixes to my wishlist of the proposal.

(Had to send the patch again since a typo occurred while sending previous the mail)

 builtin/submodule--helper.c      |  4 ----
 git.c                            |  3 +++
 submodule.c                      | 45 ++++++++++++++++++++++++++++++++++++++++
 submodule.h                      |  6 ++++++
 t/t6134-pathspec-in-submodule.sh |  2 +-
 5 files changed, 55 insertions(+), 5 deletions(-)

builtin/submodule--helper.c +0 -4

index 15a5430c0..4f7c7d7b8 100644
@@ -219,10 +219,6 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
 	return 0;
 }
 
-struct module_list {
-	const struct cache_entry **entries;
-	int alloc, nr;
-};
 #define MODULE_LIST_INIT { NULL, 0, 0 }
 
 static int module_list_compute(int argc, const char **argv,

git.c +3 -0

index 33f52acbc..eefe3fb01 100644
@@ -2,6 +2,7 @@
 #include "exec_cmd.h"
 #include "help.h"
 #include "run-command.h"
+#include "submodule.h"
 
 const char git_usage_string[] =
 	"git [--version] [--help] [-C <path>] [-c name=value]\n"

@@ -364,6 +365,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 		if (prefix)
 			die("can't use --super-prefix from a subdirectory");
 	}
+	if (prefix)
+		check_prefix_inside_submodule(prefix);
 
 	if (!help && p->option & NEED_WORK_TREE)
 		setup_work_tree();

submodule.c +45 -0

51 lines not on this page · raw message

submodule.h +6 -0

index 05ab674f0..5e41e5afc 100644
@@ -31,6 +31,12 @@ struct submodule_update_strategy {
 };
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
+struct module_list {
+	const struct cache_entry **entries;
+	int alloc, nr;
+};
+
+extern void check_prefix_inside_submodule(const char *prefix);
 extern int is_staging_gitmodules_ok(void);
 extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 extern int remove_path_from_gitmodules(const char *path);

t/t6134-pathspec-in-submodule.sh +1 -1

index fd401ca60..086cc4c47 100755
@@ -25,7 +25,7 @@ test_expect_success 'error message for path inside submodule' '
 '
 
 cat <<EOF >expect
-fatal: Pathspec '.' is in submodule 'sub'
+fatal: command from inside unpopulated submodule 'sub' not supported.
 EOF
 
 test_expect_success 'error message for path inside submodule from within submodule' '

Signature · 1 line

2.11.0

Stefan Beller

2017-04-06 18:15 UTC #2 lore raw

In reply to Prathamesh Chavan

Quoted 37 lines · Prathamesh Chavan

The main motivations for disallowing git commands within an unpopulated submodule are:
Whenever we run "git -C status" within an unpopulated submodule, it falls back to the superproject. This occurs since there is no .git file in the submodule directory. So superproject's status gets displayed. Also, the user's intention is not clear behind running the command in an unpopulated submodule. Hence we prefer to error out.
When we run the command "git -C sub add ." within a submodule, the results observed are:
In the case of the populated submodule, it acts like running “git add .“ inside the submodule. This is uncontroversial and runs as expected.
In the case of the unpopulated submodule, the user's intention behind entering the above command is unclear. He may have intended to add the submodule to the superproject or to add all files inside the sub/ directory to the submodule or superproject. Hence we’ll prefer to error out in these case.
Eventually, we use a check_prefix_inside_submodule to see check if the path is inside an unpopulated submodule. If it is, then we report the user about the unpopulated submodule.
Signed-off-by Prathamesh Chavan <[email protected]>
---
Since this patch effectively uses RUN_SETUP, builtin commands like 'diff' and other non-builtin commands are not filtered. For such cases, I think, we need to handle them separately.
Also since currently, git-submodule is not a builtin command, the command for initializing and updating the submodule doesn't return an error message, but once it is converted to builtin, we need to handle its case explicitly.

Quoted 5 lines

The build report of this patch is available on:
https://travis-ci.org/pratham-pc/git/builds/219030999
Also, the above patch was initially my GSoC project topic, but I changed it later on and added these bug fixes to my wishlist of the proposal.

Thanks for picking this topic up. :)

A couple of weeks back I floated a similar proposal of a patch[1], but as far as I remember Peff hinted that it is a bad UI to do it on such a generic early level[2]. And you also mention here that we'd not affect git-diff or other commands that do not have RUN_SETUP set.

[1] https://public-inbox.org/git/[email protected]/
[2] from the same thread as [1]:
https://public-inbox.org/git/[email protected]/

And I agree with Peff here that this high level catching is not the best way to go. Rather we'd have to go through each command, e.g. in git-status I could imagine it could look like: (white space mangled):

    diff --git a/builtin/commit.c b/builtin/commit.c
    index 4e288bc513..e3c44d4ac4 100644
    --- a/builtin/commit.c
    +++ b/builtin/commit.c
    @@ -1328,6 +1328,23 @@ static int git_status_config(const char *k,
const char *v, void *cb)
         return git_diff_ui_config(k, v, NULL);
     }

    +static void print_warning_inside_submodule(int status_format,

const char *prefix)
+{
+ switch (status_format) {
+ case STATUS_FORMAT_UNSPECIFIED: /* fall through */
+ case STATUS_FORMAT_NONE: /* fall through */
+ case STATUS_FORMAT_LONG:
+ printf(_("\n\nWARNING: \n\nIn uninitialized submodule
'%s'\n\n\n"), prefix);
+ case STATUS_FORMAT_SHORT:
+ printf(_("WARNING: In uninitialized submodule '%s'\n"), prefix);
+ case STATUS_FORMAT_PORCELAIN:
+ /* cannot encode the warning in porcelain v1. */
+ break;
+ case STATUS_FORMAT_PORCELAIN_V2:
+ printf("# WARNING prefix in submodule\n");
+ }
+}
+
int cmd_status(int argc, const char **argv, const char *prefix) {
static struct wt_status s;
@@ -1380,6 +1397,9 @@ int cmd_status(int argc, const char **argv,
const char *prefix)

         read_cache_preload(&s.pathspec);
         refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,

&s.pathspec, NULL, NULL);

    +    if (prefix && check_prefix_inside_submodule(prefix))
    +        print_warning_inside_submodule(status_format, prefix);
    +
         fd = hold_locked_index(&index_lock, 0);

         s.is_initial = get_sha1(s.reference, oid.hash) ? 1 : 0;

Quoted 2 lines

+#define MODULE_LIST_INIT { NULL, 0, 0 }

I'd keep this initializer macro near the definition, i.e. in the header (compare to STRBUF_INIT or STRING_LIST_INIT_* for example), as this can then be used where ever we can use the data structure.

Quoted 1 line

+void check_prefix_inside_submodule(const char *prefix)

I think we'll need this function in returning way, i.e.

    int check_prefix_inside_submodule(const char *prefix)
    {
        ... do check ...
        return result;
    }

Quoted 13 lines

submodule.c +13 -0

+{
+       struct module_list list = MODULE_LIST_INIT;
+       int i;
+
+       if (read_cache() < 0)
+               die(_("index file corrupt"));
+
+       for (i = 0; i < active_nr; i++) {
+               const struct cache_entry *ce = active_cache[i];
+
+               if (!S_ISGITLINK(ce->ce_mode))
+                               continue;
+

submodule.c void set_config_fetch_recurse_submodules(int value) · line 573

Quoted 10 lines

submodule.c +10 -0

+               ALLOC_GROW(list.entries, list.nr + 1, list.alloc);
+               list.entries[list.nr++] = ce;
+               while (i + 1 < active_nr &&
+                       !strcmp(ce->name, active_cache[i + 1]->name))
+                        /*
+                         * Skip entries with the same name in different stages
+                         * to make sure an entry is returned only once.
+                         */
+                       i++;
+       }

The code up to here seems to be partially duplicate of submodule--helper.c#module_list_compute().

At first I tried coming up with a nice code deduplication (i.e. put the essential parts as a function somewhere), but I think this doesn't make sense from an algorithmic point of view, because this runs in O(n) of active_cache.

active_cache is sorted by (1st) alphabet and (2nd) the index stage. The second sorting is why we have the while loop with the comment in the code.

The problem we are trying to solve here, is "Does the active_index contain prefix?", which can be done in O(log n) with a binary search, because active_index is sorted by alphabet.

So I am not sure how much code we can reuse here. nevertheless:

Quoted 1 line

+ for(i = 0; i < list.nr; i++) {

Style nit: We prefer a whitespace between a control statement (for, if, while) and the opening parens, i.e. "for (i = .."

Quoted 1 line

+ if(strlen((*list.entries[i]).name) == strlen(prefix)) {

(*list.entries[i]).name

can be simplified. The dereference *, combined with an access of the struct member can be done as ->.
(*foo).bar is equal to foo->bar.

Review

Quoted 2 lines

(quoted diff) +2 -0

+               }
+               else if(strlen((*list.entries[i]).name) ==  strlen(prefix)-1) {

The Git coding style prefers to have the closing brace and the else on the same line:

        ..
    } else if (..) {
        ..

submodule.c void set_config_fetch_recurse_submodules(int value) · line 585

Quoted 3 lines

submodule.c +3 -0

+                       const char *out = NULL;
+                       if(skip_prefix(prefix, (*list.entries[i]).name, &out)) {
+                               if(strlen(out) == 1 && out[0] == '/')

The strlen operation can take quite long potentially (O(n) with n the length of out).
So you could put the cheaper operation first, or the following would check the same
without having to compute the length:

    if (out && out[0] == '/' && !out + 1)
        ..

Quoted 1 line

+ die(_("command from inside unpopulated submodule '%s' not supported."), (*list.entries[i]).name);

Once we have this function inside each command, we can be more precise the error message. :)

Thanks,
Stefan

Prathamesh Chavan

author

2017-04-06 20:48 UTC #3 lore raw

Quoted 48 lines

A couple of weeks back I floated a similar proposal of a patch[1], but as far as I remember Peff hinted that it is a bad UI to do it on such a generic early level[2]. And you also mention here that we'd not affect git-diff or other commands that do not have RUN_SETUP set.
[1] https://public-inbox.org/git/[email protected]/
[2] from the same thread as [1]:
https://public-inbox.org/git/[email protected]/
And I agree with Peff here that this high level catching is not the best way to go. Rather we'd have to go through each command, e.g. in git-status I could imagine it could look like: (white space mangled):
    diff --git a/builtin/commit.c b/builtin/commit.c
    index 4e288bc513..e3c44d4ac4 100644
    --- a/builtin/commit.c
    +++ b/builtin/commit.c
    @@ -1328,6 +1328,23 @@ static int git_status_config(const char *k,
const char *v, void *cb)
         return git_diff_ui_config(k, v, NULL);
     }

    +static void print_warning_inside_submodule(int status_format,
const char *prefix)
+{
+ switch (status_format) {
+ case STATUS_FORMAT_UNSPECIFIED: /* fall through */
+ case STATUS_FORMAT_NONE: /* fall through */
+ case STATUS_FORMAT_LONG:
+ printf(_("\n\nWARNING: \n\nIn uninitialized submodule
'%s'\n\n\n"), prefix);
+ case STATUS_FORMAT_SHORT:
+ printf(_("WARNING: In uninitialized submodule '%s'\n"), prefix);
+ case STATUS_FORMAT_PORCELAIN:
+ /* cannot encode the warning in porcelain v1. */
+ break;
+ case STATUS_FORMAT_PORCELAIN_V2:
+ printf("# WARNING prefix in submodule\n");
+ }
+}
+
int cmd_status(int argc, const char **argv, const char *prefix) {
static struct wt_status s;
@@ -1380,6 +1397,9 @@ int cmd_status(int argc, const char **argv,
const char *prefix)
         read_cache_preload(&s.pathspec);
         refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,

Quoted 9 lines

&s.pathspec, NULL, NULL);

    +    if (prefix && check_prefix_inside_submodule(prefix))
    +        print_warning_inside_submodule(status_format, prefix);
    +
         fd = hold_locked_index(&index_lock, 0);

         s.is_initial = get_sha1(s.reference, oid.hash) ? 1 : 0;

Yes, even after finishing my patch and finding out that it doesn't work for various commands, high level catching doesn't seems correct. Also after you mention in the status command itself, as we don't expect it to error out in every case, hence making the change in every command individually will be an appropriate choice. Also in this way, we can also include the case of git-diff and also for the non-bultin commands.

Quoted 7 lines

Quoted 2 lines
+#define MODULE_LIST_INIT { NULL, 0, 0 }
I'd keep this initializer macro near the definition, i.e. in the header (compare to STRBUF_INIT or STRING_LIST_INIT_* for example), as this can then be used where ever we can use the data structure.

I was a little confused about its location too. But I agree with you that as it can be used where ever we can later, keeping it in header will be more appropriate.

Quoted 37 lines

Quoted 1 line

+void check_prefix_inside_submodule(const char *prefix)

I think we'll need this function in returning way, i.e.

    int check_prefix_inside_submodule(const char *prefix)
    {
        ... do check ...
        return result;
    }

Quoted 13 lines

submodule.c +13 -0

+{
+       struct module_list list = MODULE_LIST_INIT;
+       int i;
+
+       if (read_cache() < 0)
+               die(_("index file corrupt"));
+
+       for (i = 0; i < active_nr; i++) {
+               const struct cache_entry *ce = active_cache[i];
+
+               if (!S_ISGITLINK(ce->ce_mode))
+                               continue;
+

submodule.c void set_config_fetch_recurse_submodules(int value) · line 573

Quoted 10 lines

submodule.c +10 -0

+               ALLOC_GROW(list.entries, list.nr + 1, list.alloc);
+               list.entries[list.nr++] = ce;
+               while (i + 1 < active_nr &&
+                       !strcmp(ce->name, active_cache[i + 1]->name))
+                        /*
+                         * Skip entries with the same name in different stages
+                         * to make sure an entry is returned only once.
+                         */
+                       i++;
+       }

The code up to here seems to be partially duplicate of submodule--helper.c#module_list_compute().

Quoted 8 lines

At first I tried coming up with a nice code deduplication (i.e. put the essential parts as a function somewhere), but I think this doesn't make sense from an algorithmic point of view, because this runs in O(n) of active_cache.
active_cache is sorted by (1st) alphabet and (2nd) the index stage. The second sorting is why we have the while loop with the comment in the code.

Sorry, I was unaware about the active_cache being sorted, hence went for linear search.

Quoted 23 lines

The problem we are trying to solve here, is "Does the active_index contain prefix?", which can be done in O(log n) with a binary search, because active_index is sorted by alphabet.
So I am not sure how much code we can reuse here. nevertheless:
Quoted 1 line
+ for(i = 0; i < list.nr; i++) {
Style nit: We prefer a whitespace between a control statement (for, if, while) and the opening parens, i.e. "for (i = .."
Quoted 1 line
+ if(strlen((*list.entries[i]).name) == strlen(prefix)) {
(*list.entries[i]).name
can be simplified. The dereference *, combined with an access of the struct member can be done as ->.
(*foo).bar is equal to foo->bar.
Review
Quoted 2 lines
(quoted diff) +2 -0
+               }
+               else if(strlen((*list.entries[i]).name) ==  strlen(prefix)-1) {

Quoted 7 lines

The Git coding style prefers to have the closing brace and the else on the same line:
        ..
    } else if (..) {
        ..

Sorry for being sloppy with the coding style. In all my future patches I'll make sure I learn from all the mistakes I made so far and also be more careful.

Quoted 11 lines

submodule.c void set_config_fetch_recurse_submodules(int value) · line 585
Quoted 4 lines
submodule.c +3 -0
+                       const char *out = NULL;
+                       if(skip_prefix(prefix, (*list.entries[i]).name, &out)) {
+                               if(strlen(out) == 1 && out[0] == '/')
The strlen operation can take quite long potentially (O(n) with n the length of out).
So you could put the cheaper operation first, or the following would check the same
without having to compute the length:

Quoted 7 lines

    if (out && out[0] == '/' && !out + 1)
        ..
Quoted 1 line
+ die(_("command from inside unpopulated submodule '%s' not supported."), (*list.entries[i]).name);
Once we have this function inside each command, we can be more precise the error message. :)

Thanks for all the advices on this patch. I'll implement the above changes in a new patch series.
The approach that I'll take will be as follows:
1. Create function check_prefix_inside_submodule which returns value accordingly. Also this function is implemented by checking the active_cache list and checking if it prefix is present in active_cache[i]->name, using binary search.
Also is there some other way which is more quicker than searching for the prefix in the cache?
2. Individually call this in each command after the git environment is set and options are parsed.
3. Apply this change for appropriate options only, as suggested above for the status command.
This will ensure more accuracy.
I have also mentioned to work on this BUG in my git proposal as well, but I kept it in my wishlist section. Hence, I'll continue to work on this as my time permits. Currently, I'm also working on converting the git-submodule subcommand 'foreach' from script to builtin, by first converting it to a function in submodule--helper.c and then later converting it to builtin.

Thanks,
Prathamesh Chavan