[PATCHv4 0/5] git-rm absorbs submodule git directory before deletion

[PATCHv4 0/5] git-rm absorbs submodule git directory before deletion

[Stefan Beller]

v4:
* reworded commit messages of the last 2 patches
* introduced a new patch introducing {run,start,finish}_command_or_die
* found an existing function in dir.h to use to remove a directory
  which deals gracefully with the cornercases, that Junio pointed out.

v3:
* removed the patch to enhance ok_to_remove_submodule to absorb the submodule
  if needed
* Removed all the error reporting from git-rm that was related to submodule
  git directories not absorbed.
* instead just absorb the git repositories or let the absorb function die
  with an appropriate error message.

v2:
* new base where to apply the patch:
  sb/submodule-embed-gitdir merged with sb/t3600-cleanup.
  I got merge conflicts and resolved them this way:
#@@@ -709,9 -687,10 +687,9 @@@ test_expect_success 'checking out a com
#          git commit -m "submodule removal" submod &&
#          git checkout HEAD^ &&
#          git submodule update &&
#-         git checkout -q HEAD^ 2>actual &&
#+         git checkout -q HEAD^ &&
#          git checkout -q master 2>actual &&
# -        echo "warning: unable to rmdir submod: Directory not empty" >expected &&
# -        test_i18ncmp expected actual &&
# +        test_i18ngrep "^warning: unable to rmdir submod:" actual &&
#          git status -s submod >actual &&
#          echo "?? submod/" >expected &&
#          test_cmp expected actual &&
#

* improved commit message in "ok_to_remove_submodule: absorb the submodule git dir"
  (David Turner offered me some advice on how to write better English off list)
* simplified code in last patch:
  -> dropped wrong comment for fallthrough
  -> moved redundant code out of both bodies of an if-clause.
* Fixed last patchs commit message to have "or_die" instead of or_dir.

v1:
The "checkout --recurse-submodules" series got too large to comfortably send
it out for review, so I had to break it up into smaller series'; this is the
first subseries, but it makes sense on its own.

This series teaches git-rm to absorb the git directory of a submodule instead
of failing and complaining about the git directory preventing deletion.

It applies on origin/sb/submodule-embed-gitdir.

Any feedback welcome!

Thanks,
Stefan

Stefan Beller (5):
  submodule.h: add extern keyword to functions
  submodule: modernize ok_to_remove_submodule to use argv_array
  run-command: add {run,start,finish}_command_or_die
  submodule: add flags to ok_to_remove_submodule
  rm: absorb a submodules git dir before deletion

 builtin/rm.c  | 82 +++++++++++++++--------------------------------------------
 run-command.c | 28 ++++++++++++++++++++
 run-command.h |  4 +++
 submodule.c   | 27 ++++++++++----------
 submodule.h   | 58 ++++++++++++++++++++++++------------------
 t/t3600-rm.sh | 39 +++++++++++-----------------
 6 files changed, 114 insertions(+), 124 deletions(-)

-- 
2.11.0.rc2.53.gb7b3fba.dirty

[PATCHv4 1/5] submodule.h: add extern keyword to functions

[Stefan Beller]

As the upcoming series will add a lot of functions to the submodule
header, let's first make the header consistent to the rest of the project
by adding the extern keyword to functions.

As per the CodingGuidelines we try to stay below 80 characters per line,
so adapt all those functions to stay below 80 characters that are already
using more than one line.  Those function using just one line are better
kept in one line than breaking them up into multiple lines just for the
goal of staying below the character limit as it makes grepping
for functions easier if they are one liners.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.h | 55 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/submodule.h b/submodule.h
index 6229054b99..61fb610749 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,50 +29,55 @@ struct submodule_update_strategy {
 };
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
-int is_staging_gitmodules_ok(void);
-int update_path_in_gitmodules(const char *oldpath, const char *newpath);
-int remove_path_from_gitmodules(const char *path);
-void stage_updated_gitmodules(void);
-void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
+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);
+extern void stage_updated_gitmodules(void);
+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
 		const char *path);
-int submodule_config(const char *var, const char *value, void *cb);
-void gitmodules_config(void);
-int parse_submodule_update_strategy(const char *value,
+extern int submodule_config(const char *var, const char *value, void *cb);
+extern void gitmodules_config(void);
+extern int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
-const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
-void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
-void show_submodule_summary(FILE *f, const char *path,
+extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
+extern void handle_ignore_submodules_arg(struct diff_options *, const char *);
+extern void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
 		struct object_id *one, struct object_id *two,
 		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset);
-void show_submodule_inline_diff(FILE *f, const char *path,
+extern void show_submodule_inline_diff(FILE *f, const char *path,
 		const char *line_prefix,
 		struct object_id *one, struct object_id *two,
 		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset,
 		const struct diff_options *opt);
-void set_config_fetch_recurse_submodules(int value);
-void check_for_new_submodule_commits(unsigned char new_sha1[20]);
-int fetch_populated_submodules(const struct argv_array *options,
+extern void set_config_fetch_recurse_submodules(int value);
+extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
+extern int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
 			       int quiet, int max_parallel_jobs);
-unsigned is_submodule_modified(const char *path, int ignore_untracked);
-int submodule_uses_gitfile(const char *path);
-int ok_to_remove_submodule(const char *path);
-int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
-		    const unsigned char a[20], const unsigned char b[20], int search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
-		struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
-int parallel_submodules(void);
+extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
+extern int submodule_uses_gitfile(const char *path);
+extern int ok_to_remove_submodule(const char *path);
+extern int merge_submodule(unsigned char result[20], const char *path,
+			   const unsigned char base[20],
+			   const unsigned char a[20],
+			   const unsigned char b[20], int search);
+extern int find_unpushed_submodules(unsigned char new_sha1[20],
+				    const char *remotes_name,
+				    struct string_list *needs_pushing);
+extern int push_unpushed_submodules(unsigned char new_sha1[20],
+				    const char *remotes_name);
+extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+extern int parallel_submodules(void);
 
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
  * a submodule by clearing any repo-specific envirionment variables, but
  * retaining any config in the environment.
  */
-void prepare_submodule_repo_env(struct argv_array *out);
+extern void prepare_submodule_repo_env(struct argv_array *out);
 
 #define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
 extern void absorb_git_dir_into_superproject(const char *prefix,
-- 
2.11.0.rc2.53.gb7b3fba.dirty

[PATCHv4 2/5] submodule: modernize ok_to_remove_submodule to use argv_array

[Stefan Beller]

Instead of constructing the NULL terminated array ourselves, we
should make use of the argv_array infrastructure.

While at it, adapt the error messages to reflect the actual invocation.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/submodule.c b/submodule.c
index 45ccfb7ab4..9f0b544ebe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1023,13 +1023,6 @@ int ok_to_remove_submodule(const char *path)
 {
 	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *argv[] = {
-		"status",
-		"--porcelain",
-		"-u",
-		"--ignore-submodules=none",
-		NULL,
-	};
 	struct strbuf buf = STRBUF_INIT;
 	int ok_to_remove = 1;
 
@@ -1039,14 +1032,15 @@ int ok_to_remove_submodule(const char *path)
 	if (!submodule_uses_gitfile(path))
 		return 0;
 
-	cp.argv = argv;
+	argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+				   "--ignore-submodules=none", NULL);
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain -uall --ignore-submodules=none' in submodule %s", path);
+		die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
 
 	len = strbuf_read(&buf, cp.out, 1024);
 	if (len > 2)
@@ -1054,7 +1048,7 @@ int ok_to_remove_submodule(const char *path)
 	close(cp.out);
 
 	if (finish_command(&cp))
-		die("'git status --porcelain -uall --ignore-submodules=none' failed in submodule %s", path);
+		die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
 
 	strbuf_release(&buf);
 	return ok_to_remove;
-- 
2.11.0.rc2.53.gb7b3fba.dirty

[PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Stefan Beller]

In a later patch we want to report the exact command that we run in the
error message. Add a convenient way to the run command API that runs the
given command or reports the exact command as failure.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 run-command.c | 28 ++++++++++++++++++++++++++++
 run-command.h |  4 ++++
 2 files changed, 32 insertions(+)

diff --git a/run-command.c b/run-command.c
index ca905a9e80..a0587db334 100644
--- a/run-command.c
+++ b/run-command.c
@@ -279,6 +279,17 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 	return code;
 }
 
+static void report_and_die(struct child_process *cmd, const char *action)
+{
+	int i;
+	struct strbuf err = STRBUF_INIT;
+	if (cmd->git_cmd)
+		strbuf_addstr(&err, "git ");
+	for (i = 0; cmd->argv[i]; )
+		strbuf_addf(&err, "'%s'", cmd->argv[i]);
+	die(_("could not %s %s"), action, err.buf);
+}
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
@@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)
 	return 0;
 }
 
+void start_command_or_die(struct child_process *cmd)
+{
+	if (start_command(cmd))
+		report_and_die(cmd, "start");
+}
+
 int finish_command(struct child_process *cmd)
 {
 	int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
@@ -558,6 +575,11 @@ int finish_command_in_signal(struct child_process *cmd)
 	return wait_or_whine(cmd->pid, cmd->argv[0], 1);
 }
 
+void finish_command_or_die(struct child_process *cmd)
+{
+	if (finish_command(cmd))
+		report_and_die(cmd, "finish");
+}
 
 int run_command(struct child_process *cmd)
 {
@@ -592,6 +614,12 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
 	return run_command(&cmd);
 }
 
+void run_command_or_die(struct child_process *cmd)
+{
+	if (finish_command(cmd))
+		report_and_die(cmd, "run");
+}
+
 #ifndef NO_PTHREADS
 static pthread_t main_thread;
 static int main_thread_set;
diff --git a/run-command.h b/run-command.h
index dd1c78c28d..e4585885c5 100644
--- a/run-command.h
+++ b/run-command.h
@@ -56,6 +56,10 @@ int finish_command(struct child_process *);
 int finish_command_in_signal(struct child_process *);
 int run_command(struct child_process *);
 
+void start_command_or_die(struct child_process *);
+void finish_command_or_die(struct child_process *);
+void run_command_or_die(struct child_process *);
+
 /*
  * Returns the path to the hook file, or NULL if the hook is missing
  * or disabled. Note that this points to static storage that will be
-- 
2.11.0.rc2.53.gb7b3fba.dirty

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Brandon Williams]

On 12/19, Stefan Beller wrote:
> In a later patch we want to report the exact command that we run in the
> error message. Add a convenient way to the run command API that runs the
> given command or reports the exact command as failure.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  run-command.c | 28 ++++++++++++++++++++++++++++
>  run-command.h |  4 ++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/run-command.c b/run-command.c
> index ca905a9e80..a0587db334 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -279,6 +279,17 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
>  	return code;
>  }
>  
> +static void report_and_die(struct child_process *cmd, const char *action)
> +{
> +	int i;
> +	struct strbuf err = STRBUF_INIT;
> +	if (cmd->git_cmd)
> +		strbuf_addstr(&err, "git ");
> +	for (i = 0; cmd->argv[i]; )

Missing the increment of i here.

> +		strbuf_addf(&err, "'%s'", cmd->argv[i]);
> +	die(_("could not %s %s"), action, err.buf);
> +}
> +
>  int start_command(struct child_process *cmd)
>  {
>  	int need_in, need_out, need_err;
> @@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)
>  	return 0;
>  }
>  
> +void start_command_or_die(struct child_process *cmd)
> +{
> +	if (start_command(cmd))
> +		report_and_die(cmd, "start");
> +}
> +
>  int finish_command(struct child_process *cmd)
>  {
>  	int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
> @@ -558,6 +575,11 @@ int finish_command_in_signal(struct child_process *cmd)
>  	return wait_or_whine(cmd->pid, cmd->argv[0], 1);
>  }
>  
> +void finish_command_or_die(struct child_process *cmd)
> +{
> +	if (finish_command(cmd))
> +		report_and_die(cmd, "finish");
> +}
>  
>  int run_command(struct child_process *cmd)
>  {
> @@ -592,6 +614,12 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
>  	return run_command(&cmd);
>  }
>  
> +void run_command_or_die(struct child_process *cmd)
> +{
> +	if (finish_command(cmd))
> +		report_and_die(cmd, "run");
> +}
> +
>  #ifndef NO_PTHREADS
>  static pthread_t main_thread;
>  static int main_thread_set;
> diff --git a/run-command.h b/run-command.h
> index dd1c78c28d..e4585885c5 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -56,6 +56,10 @@ int finish_command(struct child_process *);
>  int finish_command_in_signal(struct child_process *);
>  int run_command(struct child_process *);
>  
> +void start_command_or_die(struct child_process *);
> +void finish_command_or_die(struct child_process *);
> +void run_command_or_die(struct child_process *);
> +
>  /*
>   * Returns the path to the hook file, or NULL if the hook is missing
>   * or disabled. Note that this points to static storage that will be
> -- 
> 2.11.0.rc2.53.gb7b3fba.dirty
> 

-- 
Brandon Williams

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Stefan Beller]

On Mon, Dec 19, 2016 at 3:32 PM, Brandon Williams <bmwill@google.com> wrote:

>> +             strbuf_addstr(&err, "git ");
>> +     for (i = 0; cmd->argv[i]; )
>
> Missing the increment of i here.

Doh :(

When writing the code I was undecided between using
an incremental loop "for (i = 0; cmd->argv[i]; i++)" and
a pointer arithmetic thing as "for(;;) {... cmd->argv++;",
and ended up with a broken middle ground.

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Johannes Sixt]

Am 20.12.2016 um 00:28 schrieb Stefan Beller:
> +static void report_and_die(struct child_process *cmd, const char *action)
> +{
> +	int i;
> +	struct strbuf err = STRBUF_INIT;
> +	if (cmd->git_cmd)
> +		strbuf_addstr(&err, "git ");
> +	for (i = 0; cmd->argv[i]; )
> +		strbuf_addf(&err, "'%s'", cmd->argv[i]);

Take note that cmd is accessed here.

> +	die(_("could not %s %s"), action, err.buf);

Should lego sentences not be avoided? They are not exactly translator 
friendly.

Given that a lot of effort is spent elsewhere to actually *avoid* dying 
in library code, this new die() is not very welcome, I must say. 
Granted, you just add convenience functions here, and callers have 
alternatives that do not die, but still...

> +}
> +
>  int start_command(struct child_process *cmd)
>  {
>  	int need_in, need_out, need_err;
> @@ -546,6 +557,12 @@ int start_command(struct child_process *cmd)
>  	return 0;
>  }
>
> +void start_command_or_die(struct child_process *cmd)
> +{
> +	if (start_command(cmd))
> +		report_and_die(cmd, "start");

But cmd has been cleaned up at this point of call of report_and_die. The 
access noted above goes to freed memory, I think.

> +}
> ...
> +void finish_command_or_die(struct child_process *cmd)
> +{
> +	if (finish_command(cmd))
> +		report_and_die(cmd, "finish");

Same here.

> +}
> ...
> +void run_command_or_die(struct child_process *cmd)
> +{
> +	if (finish_command(cmd))
> +		report_and_die(cmd, "run");

And here as well.

> +}

-- Hannes

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Johannes Sixt]

Am 20.12.2016 um 19:33 schrieb Johannes Sixt:
> Am 20.12.2016 um 00:28 schrieb Stefan Beller:
>> +void run_command_or_die(struct child_process *cmd)
>> +{
>> +    if (finish_command(cmd))
>> +        report_and_die(cmd, "run");
>
> And here as well.

Oh, and BTW, this one wraps only finish_command, not run_command.

-- Hannes

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Stefan Beller]

On Tue, Dec 20, 2016 at 10:33 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.12.2016 um 00:28 schrieb Stefan Beller:
>>
>> +static void report_and_die(struct child_process *cmd, const char *action)
>> +{
>> +       int i;
>> +       struct strbuf err = STRBUF_INIT;
>> +       if (cmd->git_cmd)
>> +               strbuf_addstr(&err, "git ");
>> +       for (i = 0; cmd->argv[i]; )
>> +               strbuf_addf(&err, "'%s'", cmd->argv[i]);
>
>
> Take note that cmd is accessed here.
>
>> +       die(_("could not %s %s"), action, err.buf);
>
>
> Should lego sentences not be avoided? They are not exactly translator
> friendly.
>
> Given that a lot of effort is spent elsewhere to actually *avoid* dying in
> library code, this new die() is not very welcome, I must say.

I agree on the sentiment. In a reroll I'll drop this patch and instead introduce
a function `char *get_child_command_line(struct child_process*);`, which
a caller can call before calling finish_command and then use the
resulting string
to assemble an error message without lego.

Thanks for the thorough review!
Stefan

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Johannes Sixt]

Am 20.12.2016 um 20:23 schrieb Stefan Beller:
> In a reroll I'll drop this patch and instead introduce
> a function `char *get_child_command_line(struct child_process*);`, which
> a caller can call before calling finish_command and then use the
> resulting string
> to assemble an error message without lego.

That sounds a lot better, thank you. Note though, that the function 
would have to be called before start_command(), because when it fails, 
it would be too late.

I have to ask, though: Is it so much necessary to report the exact 
command line in an error message? If someone is interested in which 
command failed, it would be "sufficient" to run the command under 
GIT_TRACE=2.

-- Hannes

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Stefan Beller]

On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.12.2016 um 20:23 schrieb Stefan Beller:
>>
>> In a reroll I'll drop this patch and instead introduce
>> a function `char *get_child_command_line(struct child_process*);`, which
>> a caller can call before calling finish_command and then use the
>> resulting string
>> to assemble an error message without lego.
>
>
> That sounds a lot better, thank you. Note though, that the function would
> have to be called before start_command(), because when it fails, it would be
> too late.

Yes, the pattern to use it would be

    // first assemble the child process struct with conditions

    char *cmdline = get_child_command_line(&child)

    if (start_command(&child))
        // use cmdline here for error reporting.


>
> I have to ask, though: Is it so much necessary to report the exact command
> line in an error message?

Have a look at https://github.com/git/git/blob/master/submodule.c#L1122

    die("Could not run 'git status --porcelain -uall \
        --ignore-submodules=none' in submodule %s", path);

There are 2 things:
1) The error message is not very informative, as your question hints at.
    I consider changing it to add more meaning. I think the end user
    would also be interested in "Why did we run this command?".
2) We really want to report the correct command line here.
    Currently that is the case, but if you look at the prior patch that extends
    the arguments conditionally (and then also uses the same condition
    to format the error message... that hints at hard to maintain error
    messages.)

So the new proposed function really only addresses 2) here.

> If someone is interested in which command failed,
> it would be "sufficient" to run the command under GIT_TRACE=2.
>

Yes, while at it, I may just move up the error reporting to the builtin
command giving a higher level message.

Thanks,
Stefan

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Johannes Sixt]

Am 20.12.2016 um 21:49 schrieb Stefan Beller:
> On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> I have to ask, though: Is it so much necessary to report the exact command
>> line in an error message?
>
> Have a look at https://github.com/git/git/blob/master/submodule.c#L1122
>
>     die("Could not run 'git status --porcelain -uall \
>         --ignore-submodules=none' in submodule %s", path);
>
> There are 2 things:
> 1) The error message is not very informative, as your question hints at.
>     I consider changing it to add more meaning. I think the end user
>     would also be interested in "Why did we run this command?".

You don't have to. start_command() and finish_command() report any 
low-level errors (exec failed, signal received...). If the exit code of 
the program is non-zero, finish_command() reports nothing because the 
command *itself* will have written some error message ("working 
directory dirty", "object database corrupt", "xyz: no such file or 
directory"...). At this level, it only makes sense to leave a trace in 
which submodule an error occured. So I think it would be sufficient to  just

     die("could not run 'git status' in submodule %s", path);
or
     die("'git status' in submodule %s failed", path);

> 2) We really want to report the correct command line here.

Why? We do not do this anywhere else.

>     Currently that is the case, but if you look at the prior patch that extends
>     the arguments conditionally (and then also uses the same condition
>     to format the error message... that hints at hard to maintain error
>     messages.)

This does not explain why the *complete and detailed* invocation must be 
reported. I haven't followed this topic at all, so I may be missing some 
cruical detail. (If you say "it must happen" one more time, then I will 
believe you, because for me that's simpler than to plough through a 
flock of submodule topics. ;-)

-- Hannes

Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

[Stefan Beller]

On Tue, Dec 20, 2016 at 1:47 PM, Johannes Sixt <j6t@kdbg.org> wrote:

> This does not explain why the *complete and detailed* invocation must be
> reported.

eh. I tried to say that a report that looks like a *complete and
detailed* invocation
should be such, and not be misleading (and e.g. miss one out of 5
arguments printed).

 I haven't followed this topic at all, so I may be missing some
> cruical detail. (If you say "it must happen" one more time, then I will
> believe you, because for me that's simpler than to plough through a flock of
> submodule topics. ;-)

It doesn't have to happen; I am trying to come up with a better message.

>
> -- Hannes
>

[PATCHv4 4/5] submodule: add flags to ok_to_remove_submodule

[Stefan Beller]

In different contexts the question "Is it ok to delete a submodule?"
may be answered differently.

In 293ab15eea (submodule: teach rm to remove submodules unless they
contain a git directory, 2012-09-26) a case was made that we can safely
ignore ignored untracked files for removal as we explicitely ask for the
removal of the submodule.

In a later patch we want to remove submodules even when the user doesn't
explicitly ask for it (e.g. checking out a tree-ish in which the submodule
doesn't exist).  In that case we want to be more careful when it comes
to deletion of untracked files. As of this patch it is unclear how this
will be implemented exactly, so we'll offer flags in which the caller
can specify how the different untracked files ought to be handled.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/rm.c |  3 ++-
 submodule.c  | 19 +++++++++++++------
 submodule.h  |  5 ++++-
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..fdd7183f61 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -187,7 +187,8 @@ static int check_local_mod(struct object_id *head, int index_only)
 		 */
 		if (ce_match_stat(ce, &st, 0) ||
 		    (S_ISGITLINK(ce->ce_mode) &&
-		     !ok_to_remove_submodule(ce->name)))
+		     !ok_to_remove_submodule(ce->name,
+				SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
 			local_changes = 1;
 
 		/*
diff --git a/submodule.c b/submodule.c
index 9f0b544ebe..9aecb8930c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1019,7 +1019,7 @@ int submodule_uses_gitfile(const char *path)
 	return 1;
 }
 
-int ok_to_remove_submodule(const char *path)
+int ok_to_remove_submodule(const char *path, unsigned flags)
 {
 	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
@@ -1032,23 +1032,30 @@ int ok_to_remove_submodule(const char *path)
 	if (!submodule_uses_gitfile(path))
 		return 0;
 
-	argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+	argv_array_pushl(&cp.args, "status", "--porcelain",
 				   "--ignore-submodules=none", NULL);
+
+	if (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED)
+		argv_array_push(&cp.args, "-uno");
+	else
+		argv_array_push(&cp.args, "-uall");
+
+	if (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED))
+		argv_array_push(&cp.args, "--ignored");
+
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
 	cp.dir = path;
-	if (start_command(&cp))
-		die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
+	start_command_or_die(&cp);
 
 	len = strbuf_read(&buf, cp.out, 1024);
 	if (len > 2)
 		ok_to_remove = 0;
 	close(cp.out);
 
-	if (finish_command(&cp))
-		die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
+	finish_command_or_die(&cp);
 
 	strbuf_release(&buf);
 	return ok_to_remove;
diff --git a/submodule.h b/submodule.h
index 61fb610749..3ed3aa479a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -59,7 +59,10 @@ extern int fetch_populated_submodules(const struct argv_array *options,
 			       int quiet, int max_parallel_jobs);
 extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
 extern int submodule_uses_gitfile(const char *path);
-extern int ok_to_remove_submodule(const char *path);
+
+#define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<0)
+#define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<1)
+extern int ok_to_remove_submodule(const char *path, unsigned flags);
 extern int merge_submodule(unsigned char result[20], const char *path,
 			   const unsigned char base[20],
 			   const unsigned char a[20],
-- 
2.11.0.rc2.53.gb7b3fba.dirty

[PATCHv4 5/5] rm: absorb a submodules git dir before deletion

[Stefan Beller]

When deleting a submodule, we need to keep the actual git directory around,
such that we do not lose local changes in there and at a later checkout
of the submodule we don't need to clone it again.

Now that the functionality is available to absorb the git directory of a
submodule, rewrite the checking in git-rm to not complain, but rather
relocate the git directories inside the superproject.

An alternative solution was discussed to have a function
`depopulate_submodule`. That would couple the check for its git directory
and possible relocation before the the removal, such that it is less
likely to miss the check in the future.  But the indirection with such
a function added seemed also complex. The reason for that was that this
possible move of the git directory was also implemented in
`ok_to_remove_submodule`, such that this function could truthfully
answer whether it is ok to remove the submodule.

The solution proposed here defers all these checks to the caller.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/rm.c  | 79 ++++++++++++++---------------------------------------------
 t/t3600-rm.sh | 39 ++++++++++++-----------------
 2 files changed, 33 insertions(+), 85 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index fdd7183f61..ed758f7144 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -59,27 +59,9 @@ static void print_error_files(struct string_list *files_list,
 	}
 }
 
-static void error_removing_concrete_submodules(struct string_list *files, int *errs)
-{
-	print_error_files(files,
-			  Q_("the following submodule (or one of its nested "
-			     "submodules)\n"
-			     "uses a .git directory:",
-			     "the following submodules (or one of their nested "
-			     "submodules)\n"
-			     "use a .git directory:", files->nr),
-			  _("\n(use 'rm -rf' if you really want to remove "
-			    "it including all of its history)"),
-			  errs);
-	string_list_clear(files, 0);
-}
-
-static int check_submodules_use_gitfiles(void)
+static void submodules_absorb_gitdir_if_needed(const char *prefix)
 {
 	int i;
-	int errs = 0;
-	struct string_list files = STRING_LIST_INIT_NODUP;
-
 	for (i = 0; i < list.nr; i++) {
 		const char *name = list.entry[i].name;
 		int pos;
@@ -99,12 +81,9 @@ static int check_submodules_use_gitfiles(void)
 			continue;
 
 		if (!submodule_uses_gitfile(name))
-			string_list_append(&files, name);
+			absorb_git_dir_into_superproject(prefix, name,
+				ABSORB_GITDIR_RECURSE_SUBMODULES);
 	}
-
-	error_removing_concrete_submodules(&files, &errs);
-
-	return errs;
 }
 
 static int check_local_mod(struct object_id *head, int index_only)
@@ -120,7 +99,6 @@ static int check_local_mod(struct object_id *head, int index_only)
 	int errs = 0;
 	struct string_list files_staged = STRING_LIST_INIT_NODUP;
 	struct string_list files_cached = STRING_LIST_INIT_NODUP;
-	struct string_list files_submodule = STRING_LIST_INIT_NODUP;
 	struct string_list files_local = STRING_LIST_INIT_NODUP;
 
 	no_head = is_null_oid(head);
@@ -218,13 +196,8 @@ static int check_local_mod(struct object_id *head, int index_only)
 		else if (!index_only) {
 			if (staged_changes)
 				string_list_append(&files_cached, name);
-			if (local_changes) {
-				if (S_ISGITLINK(ce->ce_mode) &&
-				    !submodule_uses_gitfile(name))
-					string_list_append(&files_submodule, name);
-				else
-					string_list_append(&files_local, name);
-			}
+			if (local_changes)
+				string_list_append(&files_local, name);
 		}
 	}
 	print_error_files(&files_staged,
@@ -246,8 +219,6 @@ static int check_local_mod(struct object_id *head, int index_only)
 			  &errs);
 	string_list_clear(&files_cached, 0);
 
-	error_removing_concrete_submodules(&files_submodule, &errs);
-
 	print_error_files(&files_local,
 			  Q_("the following file has local modifications:",
 			     "the following files have local modifications:",
@@ -341,6 +312,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			exit(0);
 	}
 
+	submodules_absorb_gitdir_if_needed(prefix);
+
 	/*
 	 * If not forced, the file, the index and the HEAD (if exists)
 	 * must match; but the file can already been removed, since
@@ -357,9 +330,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			oidclr(&oid);
 		if (check_local_mod(&oid, index_only))
 			exit(1);
-	} else if (!index_only) {
-		if (check_submodules_use_gitfiles())
-			exit(1);
 	}
 
 	/*
@@ -388,32 +358,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 */
 	if (!index_only) {
 		int removed = 0, gitmodules_modified = 0;
-		struct strbuf buf = STRBUF_INIT;
 		for (i = 0; i < list.nr; i++) {
 			const char *path = list.entry[i].name;
 			if (list.entry[i].is_submodule) {
-				if (is_empty_dir(path)) {
-					if (!rmdir(path)) {
-						removed = 1;
-						if (!remove_path_from_gitmodules(path))
-							gitmodules_modified = 1;
-						continue;
-					}
-				} else {
-					strbuf_reset(&buf);
-					strbuf_addstr(&buf, path);
-					if (!remove_dir_recursively(&buf, 0)) {
-						removed = 1;
-						if (!remove_path_from_gitmodules(path))
-							gitmodules_modified = 1;
-						strbuf_release(&buf);
-						continue;
-					} else if (!file_exists(path))
-						/* Submodule was removed by user */
-						if (!remove_path_from_gitmodules(path))
-							gitmodules_modified = 1;
-					/* Fallthrough and let remove_path() fail. */
-				}
+				struct strbuf buf = STRBUF_INIT;
+
+				strbuf_addstr(&buf, path);
+				if (remove_dir_recursively(&buf, 0))
+					die(_("could not remove '%s'"), path);
+				strbuf_release(&buf);
+
+				removed = 1;
+				if (!remove_path_from_gitmodules(path))
+					gitmodules_modified = 1;
+				continue;
 			}
 			if (!remove_path(path)) {
 				removed = 1;
@@ -422,7 +380,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			if (!removed)
 				die_errno("git rm: '%s'", path);
 		}
-		strbuf_release(&buf);
 		if (gitmodules_modified)
 			stage_updated_gitmodules();
 	}
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index bcbb680651..5aa6db584c 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -569,26 +569,22 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rm of a populated submodule with a .git directory fails even when forced' '
+test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '
 	git checkout -f master &&
 	git reset --hard &&
 	git submodule update &&
 	(cd submod &&
 		rm .git &&
 		cp -R ../.git/modules/sub .git &&
-		GIT_WORK_TREE=. git config --unset core.worktree
+		GIT_WORK_TREE=. git config --unset core.worktree &&
+		rm -r ../.git/modules/sub
 	) &&
-	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
-	git status -s -uno --ignore-submodules=none >actual &&
-	! test -s actual &&
-	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	git rm submod 2>output.err &&
+	! test -d submod &&
+	! test -d submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	! test -s actual &&
-	rm -rf submod
+	test -s actual &&
+	test_i18ngrep Migrating output.err
 '
 
 cat >expect.deepmodified <<EOF
@@ -667,24 +663,19 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc
 	git submodule update --recursive &&
 	(cd submod/subsubmod &&
 		rm .git &&
-		cp -R ../../.git/modules/sub/modules/sub .git &&
+		mv ../../.git/modules/sub/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
-	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/subsubmod/.git &&
-	git status -s -uno --ignore-submodules=none >actual &&
-	! test -s actual &&
-	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/subsubmod/.git &&
+	git rm submod 2>output.err &&
+	! test -d submod &&
+	! test -d submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	! test -s actual &&
-	rm -rf submod
+	test -s actual &&
+	test_i18ngrep Migrating output.err
 '
 
 test_expect_success 'checking out a commit after submodule removal needs manual updates' '
-	git commit -m "submodule removal" submod &&
+	git commit -m "submodule removal" submod .gitmodules &&
 	git checkout HEAD^ &&
 	git submodule update &&
 	git checkout -q HEAD^ &&
-- 
2.11.0.rc2.53.gb7b3fba.dirty

RE: [PATCHv4 0/5] git-rm absorbs submodule git directory before deletion

[David Turner]

Other than Brandon's issue on 3/5 (which I noticed myself but wanted to double-check that my mailer wasn't playing tricks on me and so lost the race to report), this version looks good.

> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Monday, December 19, 2016 6:28 PM
> To: gitster@pobox.com
> Cc: git@vger.kernel.org; bmwill@google.com; sandals@crustytoothpaste.net;
> David Turner; Stefan Beller
> Subject: [PATCHv4 0/5] git-rm absorbs submodule git directory before
> deletion
> 
> v4:
> * reworded commit messages of the last 2 patches
> * introduced a new patch introducing {run,start,finish}_command_or_die
> * found an existing function in dir.h to use to remove a directory
>   which deals gracefully with the cornercases, that Junio pointed out.
> 
> v3:
> * removed the patch to enhance ok_to_remove_submodule to absorb the
> submodule
>   if needed
> * Removed all the error reporting from git-rm that was related to
> submodule
>   git directories not absorbed.
> * instead just absorb the git repositories or let the absorb function die
>   with an appropriate error message.
> 
> v2:
> * new base where to apply the patch:
>   sb/submodule-embed-gitdir merged with sb/t3600-cleanup.
>   I got merge conflicts and resolved them this way:
> #@@@ -709,9 -687,10 +687,9 @@@ test_expect_success 'checking out a com
> #          git commit -m "submodule removal" submod &&
> #          git checkout HEAD^ &&
> #          git submodule update &&
> #-         git checkout -q HEAD^ 2>actual &&
> #+         git checkout -q HEAD^ &&
> #          git checkout -q master 2>actual &&
> # -        echo "warning: unable to rmdir submod: Directory not empty"
> >expected &&
> # -        test_i18ncmp expected actual &&
> # +        test_i18ngrep "^warning: unable to rmdir submod:" actual &&
> #          git status -s submod >actual &&
> #          echo "?? submod/" >expected &&
> #          test_cmp expected actual &&
> #
> 
> * improved commit message in "ok_to_remove_submodule: absorb the submodule
> git dir"
>   (David Turner offered me some advice on how to write better English off
> list)
> * simplified code in last patch:
>   -> dropped wrong comment for fallthrough
>   -> moved redundant code out of both bodies of an if-clause.
> * Fixed last patchs commit message to have "or_die" instead of or_dir.
> 
> v1:
> The "checkout --recurse-submodules" series got too large to comfortably
> send it out for review, so I had to break it up into smaller series'; this
> is the first subseries, but it makes sense on its own.
> 
> This series teaches git-rm to absorb the git directory of a submodule
> instead of failing and complaining about the git directory preventing
> deletion.
> 
> It applies on origin/sb/submodule-embed-gitdir.
> 
> Any feedback welcome!
> 
> Thanks,
> Stefan
> 
> Stefan Beller (5):
>   submodule.h: add extern keyword to functions
>   submodule: modernize ok_to_remove_submodule to use argv_array
>   run-command: add {run,start,finish}_command_or_die
>   submodule: add flags to ok_to_remove_submodule
>   rm: absorb a submodules git dir before deletion
> 
>  builtin/rm.c  | 82 +++++++++++++++---------------------------------------
> -----
>  run-command.c | 28 ++++++++++++++++++++  run-command.h |  4 +++
>  submodule.c   | 27 ++++++++++----------
>  submodule.h   | 58 ++++++++++++++++++++++++------------------
>  t/t3600-rm.sh | 39 +++++++++++-----------------
>  6 files changed, 114 insertions(+), 124 deletions(-)
> 
> --
> 2.11.0.rc2.53.gb7b3fba.dirty