Re: [PATCH 10/15] update submodules: add submodule_go_from_to

[Stefan Beller]

On Thu, Feb 16, 2017 at 1:15 PM, Junio C Hamano  wrote:
> [administrivia: I've been seeing "unlisted-recipients:; (no To-header on 
> input)"
> for all of your recent patches.  Can it be corrected on your end, please?]

I cc'd everyone and had no to field, actually. Maybe git-send-email should
complain more loudly when I am holding it wrong.

>> + cp.git_cmd = 1;
>> + argv_array_pushl(, "diff-index", "--cached", "HEAD", NULL);
>
> We'd want to use the QUICK optimization here, I suspect.  This
> caller does not need to (or want to) learn which exact paths are
> modified, right?

ok

>> + if (run_command())
>> + die("could not clean submodule index");
>> +}
>
> Do s/clean/reset/ everywhere above.

ok

>
>> +/**
>> + * Moves a submodule at a given path from a given head to another new head.
>> + * For edge cases (a submodule coming into existence or removing a 
>> submodule)
>> + * pass NULL for old or new respectively.
>> + *
>> + * TODO: move dryrun and forced to flags.
>
> The reason why this seeingly trivial thing is left as TODO is...???

will do. The reason was to first get the grand design right before
spending more time in the details.

>
>> + */
>> +int submodule_go_from_to(const char *path,
>> +  const char *old,
>> +  const char *new,
>> +  int dry_run,
>> +  int force)
>> +{
>
> go-from-to does not tell me what it does, but my cursory read of the
> body of the function tells me that this is doing a checkout of a
> branch in the submodule?  The operation in builtin/checkout.c that
> conceptually correspond to this is called switch_branches(), I
> think, so perhaps submodule_switch_branches() is a better name?

Well as of now all submodule operations (submodule update mostly)
end up with detached HEADs in the submodule. So it is rather
going from one state (sha1) to another given sha1.

I would rather compare it to checkout_entry/write_entry in entry.c
except that there are more things to go wrong. A single file has no
notion of its own index or dirtiness.

>
>> + int ret = 0;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + const struct submodule *sub;
>> +
>> + sub = submodule_from_path(null_sha1, path);
>> +
>> + if (!sub)
>> + die("BUG: could not get submodule information for '%s'", path);
>> +
>> + if (!dry_run) {
>> + if (old) {
>> + if (!submodule_uses_gitfile(path))
>> + absorb_git_dir_into_superproject("", path,
>> + ABSORB_GITDIR_RECURSE_SUBMODULES);
>> + } else {
>> + struct strbuf sb = STRBUF_INIT;
>> + strbuf_addf(, "%s/modules/%s",
>> + get_git_common_dir(), sub->name);
>> + connect_work_tree_and_git_dir(path, sb.buf);
>> + strbuf_release();
>> +
>> + /* make sure the index is clean as well */
>> + submodule_clean_index(path);
>> + }
>> + }
>> +
>> + if (old && !force) {
>> + /* Check if the submodule has a dirty index. */
>> + if (submodule_has_dirty_index(sub)) {
>> + /* print a thing here? */
>> + return -1;
>> + }
>
> Isn't it too late to do this here?  You already reset the index
> in the submodule, no?

Yes this is confusing.
We run this function first as a dry_run, and in a second pass
as a real run. So the order inside the function is confusing
as we would run this first in the dry run.

>
> Is the idea that changes that are only in the submodule's working
> tree are noticed by later "read-tree -u -m" down there?  Not
> complaining but trying to understand.

I think (as a first step) we only want to allow a clean index in
submodules, as then we have to implement less cases at first.

Re: [PATCH 10/15] update submodules: add submodule_go_from_to

[Junio C Hamano]

Stefan Beller  writes:

[administrivia: I've been seeing "unlisted-recipients:; (no To-header on input)"
for all of your recent patches.  Can it be corrected on your end, please?]

> In later patches we introduce the options and flag for commands
> that modify the working directory, e.g. git-checkout.
>
> This piece of code will be used universally for
> all these working tree modifications as it
> * supports dry run to answer the question:
>   "Is it safe to change the submodule to this new state?"
>   e.g. is it overwriting untracked files or are there local
>   changes that would be overwritten?
> * supports a force flag that can be used for resetting
>   the tree.
>
> Signed-off-by: Stefan Beller 
> ---
>  submodule.c | 151 
> 
>  submodule.h |   5 ++
>  2 files changed, 156 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index b262c5b0ad..84cc62f3bb 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1250,6 +1250,157 @@ int bad_to_remove_submodule(const char *path, 
> unsigned flags)
>   return ret;
>  }
>  
> +static int submodule_has_dirty_index(const struct submodule *sub)
> +{
> + ssize_t len;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf buf = STRBUF_INIT;
> + int ret = 0;
> +
> + prepare_submodule_repo_env_no_git_dir(_array);
> +
> + cp.git_cmd = 1;
> + argv_array_pushl(, "diff-index", "--cached", "HEAD", NULL);

We'd want to use the QUICK optimization here, I suspect.  This
caller does not need to (or want to) learn which exact paths are
modified, right?

> +void submodule_clean_index(const char *path)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + prepare_submodule_repo_env_no_git_dir(_array);
> +
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> +
> + argv_array_pushf(, "--super-prefix=%s/", path);
> + argv_array_pushl(, "read-tree", "-u", "--reset", NULL);
> +
> + argv_array_push(, EMPTY_TREE_SHA1_HEX);
> +
> + if (run_command())
> + die("could not clean submodule index");
> +}

Do s/clean/reset/ everywhere above.

> +/**
> + * Moves a submodule at a given path from a given head to another new head.
> + * For edge cases (a submodule coming into existence or removing a submodule)
> + * pass NULL for old or new respectively.
> + *
> + * TODO: move dryrun and forced to flags.

The reason why this seeingly trivial thing is left as TODO is...???

> + */
> +int submodule_go_from_to(const char *path,
> +  const char *old,
> +  const char *new,
> +  int dry_run,
> +  int force)
> +{

go-from-to does not tell me what it does, but my cursory read of the
body of the function tells me that this is doing a checkout of a
branch in the submodule?  The operation in builtin/checkout.c that
conceptually correspond to this is called switch_branches(), I
think, so perhaps submodule_switch_branches() is a better name?

> + int ret = 0;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + const struct submodule *sub;
> +
> + sub = submodule_from_path(null_sha1, path);
> +
> + if (!sub)
> + die("BUG: could not get submodule information for '%s'", path);
> +
> + if (!dry_run) {
> + if (old) {
> + if (!submodule_uses_gitfile(path))
> + absorb_git_dir_into_superproject("", path,
> + ABSORB_GITDIR_RECURSE_SUBMODULES);
> + } else {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_addf(, "%s/modules/%s",
> + get_git_common_dir(), sub->name);
> + connect_work_tree_and_git_dir(path, sb.buf);
> + strbuf_release();
> +
> + /* make sure the index is clean as well */
> + submodule_clean_index(path);
> + }
> + }
> +
> + if (old && !force) {
> + /* Check if the submodule has a dirty index. */
> + if (submodule_has_dirty_index(sub)) {
> + /* print a thing here? */
> + return -1;
> + }

Isn't it too late to do this here?  You already reset the index
in the submodule, no?

Is the idea that changes that are only in the submodule's working
tree are noticed by later "read-tree -u -m" down there?  Not
complaining but trying to understand.

> + }
> +
> + prepare_submodule_repo_env_no_git_dir(_array);
> +
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> +
> + argv_array_pushf(, "--super-prefix=%s/", path);
> + argv_array_pushl(, "read-tree", NULL);
> +
> + if (dry_run)
> + argv_array_push(, "-n");
> + else
> + argv_array_push(, "-u");
> +
> + if (force)
> + 

[PATCH 10/15] update submodules: add submodule_go_from_to

[Stefan Beller]

In later patches we introduce the options and flag for commands
that modify the working directory, e.g. git-checkout.

This piece of code will be used universally for
all these working tree modifications as it
* supports dry run to answer the question:
  "Is it safe to change the submodule to this new state?"
  e.g. is it overwriting untracked files or are there local
  changes that would be overwritten?
* supports a force flag that can be used for resetting
  the tree.

Signed-off-by: Stefan Beller 
---
 submodule.c | 151 
 submodule.h |   5 ++
 2 files changed, 156 insertions(+)

diff --git a/submodule.c b/submodule.c
index b262c5b0ad..84cc62f3bb 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1250,6 +1250,157 @@ int bad_to_remove_submodule(const char *path, unsigned 
flags)
return ret;
 }
 
+static int submodule_has_dirty_index(const struct submodule *sub)
+{
+   ssize_t len;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct strbuf buf = STRBUF_INIT;
+   int ret = 0;
+
+   prepare_submodule_repo_env_no_git_dir(_array);
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "diff-index", "--cached", "HEAD", NULL);
+   cp.no_stdin = 1;
+   cp.out = -1;
+   cp.dir = sub->path;
+   if (start_command())
+   die("could not recurse into submodule %s", sub->path);
+
+   len = strbuf_read(, cp.out, 1024);
+   if (len > 2)
+   ret = 1;
+
+   close(cp.out);
+   if (finish_command())
+   die("could not recurse into submodule %s", sub->path);
+
+   strbuf_release();
+   return ret;
+}
+
+void submodule_clean_index(const char *path)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+   prepare_submodule_repo_env_no_git_dir(_array);
+
+   cp.git_cmd = 1;
+   cp.no_stdin = 1;
+   cp.dir = path;
+
+   argv_array_pushf(, "--super-prefix=%s/", path);
+   argv_array_pushl(, "read-tree", "-u", "--reset", NULL);
+
+   argv_array_push(, EMPTY_TREE_SHA1_HEX);
+
+   if (run_command())
+   die("could not clean submodule index");
+}
+
+/**
+ * Moves a submodule at a given path from a given head to another new head.
+ * For edge cases (a submodule coming into existence or removing a submodule)
+ * pass NULL for old or new respectively.
+ *
+ * TODO: move dryrun and forced to flags.
+ */
+int submodule_go_from_to(const char *path,
+const char *old,
+const char *new,
+int dry_run,
+int force)
+{
+   int ret = 0;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   const struct submodule *sub;
+
+   sub = submodule_from_path(null_sha1, path);
+
+   if (!sub)
+   die("BUG: could not get submodule information for '%s'", path);
+
+   if (!dry_run) {
+   if (old) {
+   if (!submodule_uses_gitfile(path))
+   absorb_git_dir_into_superproject("", path,
+   ABSORB_GITDIR_RECURSE_SUBMODULES);
+   } else {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_addf(, "%s/modules/%s",
+   get_git_common_dir(), sub->name);
+   connect_work_tree_and_git_dir(path, sb.buf);
+   strbuf_release();
+
+   /* make sure the index is clean as well */
+   submodule_clean_index(path);
+   }
+   }
+
+   if (old && !force) {
+   /* Check if the submodule has a dirty index. */
+   if (submodule_has_dirty_index(sub)) {
+   /* print a thing here? */
+   return -1;
+   }
+   }
+
+   prepare_submodule_repo_env_no_git_dir(_array);
+
+   cp.git_cmd = 1;
+   cp.no_stdin = 1;
+   cp.dir = path;
+
+   argv_array_pushf(, "--super-prefix=%s/", path);
+   argv_array_pushl(, "read-tree", NULL);
+
+   if (dry_run)
+   argv_array_push(, "-n");
+   else
+   argv_array_push(, "-u");
+
+   if (force)
+   argv_array_push(, "--reset");
+   else
+   argv_array_push(, "-m");
+
+   argv_array_push(, old ? old : EMPTY_TREE_SHA1_HEX);
+   argv_array_push(, new ? new : EMPTY_TREE_SHA1_HEX);
+
+   if (run_command()) {
+   ret = -1;
+   goto out;
+   }
+
+   if (!dry_run) {
+   if (new) {
+   struct child_process cp1 = CHILD_PROCESS_INIT;
+   /* also set the HEAD accordingly */
+   cp1.git_cmd = 1;
+   cp1.no_stdin = 1;
+   cp1.dir = path;
+
+   argv_array_pushl(, "update-ref", "HEAD",
+