[PATCH 1/2] submodule.c: convert submodule_move_head new argument to object id

[PATCH 1/2] submodule.c: convert submodule_move_head new argument to object id

[Stefan Beller]

All callers use oid_to_hex to convert the desired oid to a string before
calling submodule_move_head. Defer the conversion to the
submodule_move_head as it will turn out to be useful in a bit.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This is also part of the other series sent out yesterday, 
https://public-inbox.org/git/20180904230149.180332-5-sbeller@google.com/

 entry.c        |  6 +++---
 submodule.c    | 12 ++++++------
 submodule.h    |  2 +-
 unpack-trees.c | 13 +++++--------
 4 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/entry.c b/entry.c
index 2a2ab6c8394..0b676f997b2 100644
--- a/entry.c
+++ b/entry.c
@@ -358,7 +358,7 @@ static int write_entry(struct cache_entry *ce,
 		sub = submodule_from_ce(ce);
 		if (sub)
 			return submodule_move_head(ce->name,
-				NULL, oid_to_hex(&ce->oid),
+				NULL, &ce->oid,
 				state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);
 		break;
 
@@ -439,10 +439,10 @@ int checkout_entry(struct cache_entry *ce,
 					unlink_or_warn(ce->name);
 
 				return submodule_move_head(ce->name,
-					NULL, oid_to_hex(&ce->oid), 0);
+					NULL, &ce->oid, 0);
 			} else
 				return submodule_move_head(ce->name,
-					"HEAD", oid_to_hex(&ce->oid),
+					"HEAD", &ce->oid,
 					state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);
 		}
 
diff --git a/submodule.c b/submodule.c
index 50cbf5f13ed..da2ed8696f5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1597,9 +1597,9 @@ static void submodule_reset_index(const char *path)
  * pass NULL for old or new respectively.
  */
 int submodule_move_head(const char *path,
-			 const char *old_head,
-			 const char *new_head,
-			 unsigned flags)
+			const char *old_head,
+			const struct object_id *new_oid,
+			unsigned flags)
 {
 	int ret = 0;
 	struct child_process cp = CHILD_PROCESS_INIT;
@@ -1679,7 +1679,7 @@ int submodule_move_head(const char *path,
 	if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))
 		argv_array_push(&cp.args, old_head ? old_head : empty_tree_oid_hex());
 
-	argv_array_push(&cp.args, new_head ? new_head : empty_tree_oid_hex());
+	argv_array_push(&cp.args, new_oid ? oid_to_hex(new_oid) : empty_tree_oid_hex());
 
 	if (run_command(&cp)) {
 		ret = error(_("Submodule '%s' could not be updated."), path);
@@ -1687,7 +1687,7 @@ int submodule_move_head(const char *path,
 	}
 
 	if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
-		if (new_head) {
+		if (new_oid) {
 			child_process_init(&cp);
 			/* also set the HEAD accordingly */
 			cp.git_cmd = 1;
@@ -1696,7 +1696,7 @@ int submodule_move_head(const char *path,
 
 			prepare_submodule_repo_env(&cp.env_array);
 			argv_array_pushl(&cp.args, "update-ref", "HEAD",
-					 "--no-deref", new_head, NULL);
+					 "--no-deref", oid_to_hex(new_oid), NULL);
 
 			if (run_command(&cp)) {
 				ret = -1;
diff --git a/submodule.h b/submodule.h
index 7d476cefa7e..ef34d5a7ca8 100644
--- a/submodule.h
+++ b/submodule.h
@@ -124,7 +124,7 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule);
 #define SUBMODULE_MOVE_HEAD_FORCE   (1<<1)
 int submodule_move_head(const char *path,
 			const char *old,
-			const char *new_head,
+			const struct object_id *new_oid,
 			unsigned flags);
 
 void submodule_unset_core_worktree(const struct submodule *sub);
diff --git a/unpack-trees.c b/unpack-trees.c
index f25089b878a..75d1b294ade 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -256,7 +256,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
 
 static int check_submodule_move_head(const struct cache_entry *ce,
 				     const char *old_id,
-				     const char *new_id,
+				     const struct object_id *new_id,
 				     struct unpack_trees_options *o)
 {
 	unsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;
@@ -1517,7 +1517,7 @@ static int verify_uptodate_1(const struct cache_entry *ce,
 
 		if (submodule_from_ce(ce)) {
 			int r = check_submodule_move_head(ce,
-				"HEAD", oid_to_hex(&ce->oid), o);
+				"HEAD", &ce->oid, o);
 			if (r)
 				return o->gently ? -1 :
 					add_rejected_path(o, error_type, ce->name);
@@ -1591,8 +1591,7 @@ static int verify_clean_submodule(const char *old_sha1,
 	if (!submodule_from_ce(ce))
 		return 0;
 
-	return check_submodule_move_head(ce, old_sha1,
-					 oid_to_hex(&ce->oid), o);
+	return check_submodule_move_head(ce, old_sha1, &ce->oid, o);
 }
 
 static int verify_clean_subdirectory(const struct cache_entry *ce,
@@ -1836,8 +1835,7 @@ static int merged_entry(const struct cache_entry *ce,
 
 		if (submodule_from_ce(ce)) {
 			int ret = check_submodule_move_head(ce, NULL,
-							    oid_to_hex(&ce->oid),
-							    o);
+							    &ce->oid, o);
 			if (ret)
 				return ret;
 		}
@@ -1865,8 +1863,7 @@ static int merged_entry(const struct cache_entry *ce,
 
 		if (submodule_from_ce(ce)) {
 			int ret = check_submodule_move_head(ce, oid_to_hex(&old->oid),
-							    oid_to_hex(&ce->oid),
-							    o);
+							    &ce->oid, o);
 			if (ret)
 				return ret;
 		}
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

[PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions

[Stefan Beller]

By checking if a submodule commit exists before attempting the update
we can improve the error message from the
    error(_("Submodule '%s' could not be updated."), path);
to the new and more specific
    error(_("Submodule '%s' doesn't have commit '%s'"),
          path, oid_to_hex(new_oid));

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

diff --git a/submodule.c b/submodule.c
index da2ed8696f5..56104af1c7c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1605,6 +1605,7 @@ int submodule_move_head(const char *path,
 	struct child_process cp = CHILD_PROCESS_INIT;
 	const struct submodule *sub;
 	int *error_code_ptr, error_code;
+	struct repository subrepo;
 
 	if (!is_submodule_active(the_repository, path))
 		return 0;
@@ -1627,6 +1628,13 @@ int submodule_move_head(const char *path,
 	if (!sub)
 		BUG("could not get submodule information for '%s'", path);
 
+	if (repo_submodule_init(&subrepo, the_repository, path) < 0)
+		warning(_("Could not get submodule repository for submodule 's'"), path);
+	else if (new_oid && !lookup_commit(subrepo, new_oid)) {
+		return error(_("Submodule '%s' doesn't have commit '%s'"),
+			     path, oid_to_hex(new_oid));
+	}
+
 	if (old_head && !(flags & SUBMODULE_MOVE_HEAD_FORCE)) {
 		/* Check if the submodule has a dirty index. */
 		if (submodule_has_dirty_index(sub))
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Subject: submodule.c: warn about missing submodule commit in recursive actions

Nit: the diff already tells me what file the change is in.  What I'd
be more interested in is the subsystem or what commands this affects.
Does this affect all --recurse-submodules commands, or just some?

Here, I think it's about common submodule code, so I guess
'submodule:' would be a fine prefix.

> By checking if a submodule commit exists before attempting the update
> we can improve the error message from the
>     error(_("Submodule '%s' could not be updated."), path);
> to the new and more specific
>     error(_("Submodule '%s' doesn't have commit '%s'"),
>           path, oid_to_hex(new_oid));

Maybe it's just me, but I find this formatting where I cannot
distinguish between a line that was wrapped early and the start of a
callout hard to read.  Some extra line breaks would help:

  By checking if a submodule commit exists before attempting the update
  we can improve the error message from the

      error(_("Submodule '%s' could not be updated."), path);

  to the new and more specific

      error(_("Submodule '%s' doesn't have commit '%s'"),
            path, oid_to_hex(new_oid));

Beyond that, I still don't know what this change does.  Can you give
an example?  For example, what command would I run before and what bad
result would I get, and what result does this patch produce instead?

Thanks,
Jonathan

Re: [PATCH 2/2] submodule.c: warn about missing submodule commit in recursive actions

[Martin Ågren]

> +       if (repo_submodule_init(&subrepo, the_repository, path) < 0)
> +               warning(_("Could not get submodule repository for submodule 's'"), path);

Missing "%" in format specifier, so `path` will never be used. Also,
s/C/c/ at the start of the warning.

Thanks for marking with _().

Martin

Re: [PATCH 1/2] submodule.c: convert submodule_move_head new argument to object id

[Jonathan Nieder]

Stefan Beller wrote:

> Subject: submodule.c: convert submodule_move_head new argument to object id

Same nit as in https://public-inbox.org/git/20180905233203.GE120842@aiede.svl.corp.google.com/
applies about wondering which subsystem this is in.

[...]
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1597,9 +1597,9 @@ static void submodule_reset_index(const char *path)
>   * pass NULL for old or new respectively.
>   */
>  int submodule_move_head(const char *path,
> -			 const char *old_head,
> -			 const char *new_head,
> -			 unsigned flags)
> +			const char *old_head,
> +			const struct object_id *new_oid,
> +			unsigned flags)

This seems oddly asymmetrical.  Should the old value be passed as an
object_id as well?

Curious,
Jonathan

Re: [PATCH 1/2] submodule.c: convert submodule_move_head new argument to object id

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> All callers use oid_to_hex to convert the desired oid to a string before
> calling submodule_move_head. Defer the conversion to the
> submodule_move_head as it will turn out to be useful in a bit.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> This is also part of the other series sent out yesterday, 
> https://public-inbox.org/git/20180904230149.180332-5-sbeller@google.com/

And what is your intention wrt the other one?

Do you want two separate copies of essentially the same patch queued
as part of two separate topics?  Did you change your mind since
yesterday and decided to retract these patches, and instead
concentrate on these two patches that address an issue with tigher
focus before moving on to other things?

Exactly the same comment applies to this copy.  It is somewhat
disturbing to see that new and old, which correspond to each other
and ought to be symmetrical, take the same "commit object" in
different shapes.