changed submodules
cover changed submodules
A little bit ago I was looking for a function in our code base which would be able to give me a list of all the submodules which changed across a number of commits. I stumbled upon some of that logic in both our recurisve fetch and pull code paths. Problem is both of these code paths were performing almost identical tasks, without sharing any code.
This series is meant as a cleanup to submodule.c in order to unify these two code paths as well as make it easier for future callers to be able to query for submodules which have chagned across a number of commits.
Brandon Williams (6):
submodule: rename add_sha1_to_array
submodule: rename free_submodules_sha1s
submodule: remove add_oid_to_argv
submodule: change string_list changed_submodule_paths
submodule: improve submodule_has_commits
submodule: refactor logic to determine changed submodules
submodule.c | 295 ++++++++++++++++++++++++++++-------------------------------- 1 file changed, 139 insertions(+), 156 deletions(-)
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
PATCH 1/6 submodule: rename add_sha1_to_array
Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more accuratly describe what the function does since it handles 'struct object_id' and not sha1 character arrays.
--- submodule.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
submodule.c +5 -4
@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q, } } -static int add_sha1_to_array (const char *ref,const struct object_id *oid, -int flags,void *data) +static int append_oid_to_array (const char *ref,const struct object_id *oid, +int flags,void *data) { -oid_array_append (data, oid); +struct oid_array *array = data; +oid_array_append (array, oid);return 0 ; }void check_for_new_submodule_commits (struct object_id *oid) {if (!initialized_fetch_ref_tips) { -for_each_ref (add_sha1_to_array, &ref_tips_before_fetch); +for_each_ref (append_oid_to_array, &ref_tips_before_fetch); initialized_fetch_ref_tips =1 ; }
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
Re: [PATCH 1/6] submodule: rename add_sha1_to_array
Quoted 2 lines · Brandon Williams
Rename '
add_sha1_to_array()' to 'append_oid_to_array()' to more accuratly describe what the function does since it handles 'struct
accurately. Will fix while queuing.
Makes sense. Thanks.
Quoted 33 lines
object_id' and not sha1 character arrays.Signed-off-by Brandon Williams <[email protected]>--- submodule.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)submodule.c +5
-4@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q, } } -static int add_sha1_to_array (const char *ref,const struct object_id *oid, -int flags,void *data) +static int append_oid_to_array (const char *ref,const struct object_id *oid, +int flags,void *data) { -oid_array_append (data, oid); +struct oid_array *array = data; +oid_array_append (array, oid);return 0 ; }void check_for_new_submodule_commits (struct object_id *oid) {if (!initialized_fetch_ref_tips) { -for_each_ref (add_sha1_to_array, &ref_tips_before_fetch); +for_each_ref (append_oid_to_array, &ref_tips_before_fetch); initialized_fetch_ref_tips =1 ; }
PATCH 2/6 submodule: rename free_submodules_sha1s
In reply to Brandon WilliamsRename 'free_submodules_sha1s()' to 'free_submodules_oids()' since the function frees a 'struct string_list' which has a 'struct oid_array' stored in the 'util' field.
--- submodule.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
submodule.c +3 -2
@@ -738,7 +738,7 @@ static void find_unpushed_submodule_commits(struct commit *commit,diff_tree_combined_merge (commit,1 , &rev); } -static void free_submodules_sha1s (struct string_list *submodules) +static void free_submodules_oids (struct string_list *submodules) {struct string_list_item *item;for_each_string_list_item (item, submodules)
@@ -779,7 +779,8 @@ int find_unpushed_submodules(struct oid_array *commits,if (submodule_needs_pushing (submodule->string, commits))string_list_insert (needs_pushing, submodule->string); } -free_submodules_sha1s (&submodules); + +free_submodules_oids (&submodules);return needs_pushing->nr; }
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
PATCH 3/6 submodule: remove add_oid_to_argv
In reply to Brandon WilliamsThe function 'add_oid_to_argv()' provides the same functionality as 'append_oid_to_argv()'. Remove this duplicate function and instead use 'append_oid_to_argv()' where 'add_oid_to_argv()' was previously used.
--- submodule.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
submodule.c +2 -8
@@ -970,12 +970,6 @@ void check_for_new_submodule_commits(struct object_id *oid)oid_array_append (&ref_tips_after_fetch, oid); } -static int add_oid_to_argv (const struct object_id *oid,void *data) -{ -argv_array_push (data,oid_to_hex (oid)); -return 0 ; -} -static void calculate_changed_submodule_paths (void ) {struct rev_info rev;
@@ -989,10 +983,10 @@ static void calculate_changed_submodule_paths(void)init_revisions (&rev,NULL );argv_array_push (&argv,"--" );/* argv[0] program name */ oid_array_for_each_unique (&ref_tips_after_fetch, - add_oid_to_argv, &argv); + append_oid_to_argv, &argv);argv_array_push (&argv,"--not" );oid_array_for_each_unique (&ref_tips_before_fetch, - add_oid_to_argv, &argv); + append_oid_to_argv, &argv);setup_revisions (argv.argc, argv.argv, &rev,NULL );if (prepare_revision_walk (&rev))die ("revision walk setup failed" );
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
PATCH 4/6 submodule: change string_list changed_submodule_paths
In reply to Brandon WilliamsEliminate a call to 'xstrdup()' by changing the string_list 'changed_submodule_paths' to duplicated strings added to it.
--- submodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
submodule.c +2 -2
@@ -20,7 +20,7 @@static int config_fetch_recurse_submodules =RECURSE_SUBMODULES_ON_DEMAND ;static int config_update_recurse_submodules =RECURSE_SUBMODULES_DEFAULT ;static int parallel_jobs =1 ; -static struct string_list changed_submodule_paths =STRING_LIST_INIT_NODUP ; +static struct string_list changed_submodule_paths =STRING_LIST_INIT_DUP ;static int initialized_fetch_ref_tips;static struct oid_array ref_tips_before_fetch;static struct oid_array ref_tips_after_fetch;
@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,struct string_list_item *path; path =unsorted_string_list_lookup (&changed_submodule_paths, p->two->path);if (!path && !is_submodule_commit_present (p->two->path, p->two->oid.hash)) -string_list_append (&changed_submodule_paths,xstrdup (p->two->path)); +string_list_append (&changed_submodule_paths, p->two->path); }else {/* Submodule is new or was moved here */ /* NEEDSWORK: When the .git directories of submodules
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
Re: [PATCH 4/6] submodule: change string_list changed_submodule_paths
Quoted 22 lines · Brandon Williams
Eliminate a call to '
xstrdup()' by changing thestring_list'changed_submodule_paths' to duplicated strings added to it.Signed-off-by Brandon Williams <[email protected]>--- submodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)submodule.c +1
-1@@ -20,7 +20,7 @@static int config_fetch_recurse_submodules =RECURSE_SUBMODULES_ON_DEMAND ;static int config_update_recurse_submodules =RECURSE_SUBMODULES_DEFAULT ;static int parallel_jobs =1 ; -static struct string_list changed_submodule_paths =STRING_LIST_INIT_NODUP ; +static struct string_list changed_submodule_paths =STRING_LIST_INIT_DUP ;static int initialized_fetch_ref_tips;static struct oid_array ref_tips_before_fetch;static struct oid_array ref_tips_after_fetch;
submodule.c static void submodule_collect_changed_cb(struct diff_queue_struct *q, · line 942
Quoted 5 lines
submodule.c +1
-1@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,struct string_list_item *path; path =unsorted_string_list_lookup (&changed_submodule_paths, p->two->path);if (!path && !is_submodule_commit_present (p->two->path, p->two->oid.hash)) -string_list_append (&changed_submodule_paths,xstrdup (p->two->path)); +string_list_append (&changed_submodule_paths, p->two->path);
I notice that "path" is not used at all, and other users of this string list do not even bother using a variable, i.e.
if (!unsorted_string_list_lookup(&changed_submodule_paths, ...))
In fact, it might be even better to use a hashmap for this instead?
The call to string_list_clear() onthis list tells it to free the util field, and I went to see what we are storing in the util field, but it seems that it is freeing NULLs, which is somewhat misleading and time-wasting on the code readers. Using hashmap may also clear this up.
But all of the above are not within the scope of this topic ;-)
Re: [PATCH 4/6] submodule: change string_list changed_submodule_paths
Quoted 37 lines · Junio C Hamano
Quoted 22 lines · Brandon Williams
Eliminate a call to '
xstrdup()' by changing thestring_list'changed_submodule_paths' to duplicated strings added to it.Signed-off-by Brandon Williams <[email protected]>--- submodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)submodule.c +1
-1@@ -20,7 +20,7 @@static int config_fetch_recurse_submodules =RECURSE_SUBMODULES_ON_DEMAND ;static int config_update_recurse_submodules =RECURSE_SUBMODULES_DEFAULT ;static int parallel_jobs =1 ; -static struct string_list changed_submodule_paths =STRING_LIST_INIT_NODUP ; +static struct string_list changed_submodule_paths =STRING_LIST_INIT_DUP ;static int initialized_fetch_ref_tips;static struct oid_array ref_tips_before_fetch;static struct oid_array ref_tips_after_fetch;submodule.c static void submodule_collect_changed_cb(struct diff_queue_struct *q, · line 942
Quoted 5 lines
submodule.c +1
-1@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,struct string_list_item *path; path =unsorted_string_list_lookup (&changed_submodule_paths, p->two->path);if (!path && !is_submodule_commit_present (p->two->path, p->two->oid.hash)) -string_list_append (&changed_submodule_paths,xstrdup (p->two->path)); +string_list_append (&changed_submodule_paths, p->two->path);I notice that "path" is not used at all, and other users of this string list do not even bother using a variable, i.e.
if (!unsorted_string_list_lookup(&changed_submodule_paths, ...))
In fact, it might be even better to use a hashmap for this instead?
Quoted 7 lines
The call to
string_list_clear()onthis list tells it to free the util field, and I went to see what we are storing in the util field, but it seems that it is freeing NULLs, which is somewhat misleading and time-wasting on the code readers. Using hashmap may also clear this up.But all of the above are not within the scope of this topic ;-)
All good good points which hopefully are resolved in a later patch. This patch exists mainly to factor out the change to make the string_list duplicate the strings added to it as opposed to have that change seem random and out of place in a later patch.
Most of this logic itself is removed entirely later once it is unified with the other code path which checked for changed submodules. This may be out of context on this particular patch, but eventually an oid_array is stored in the 'util' field of the string list items. This oid_array holds all of the changes to the submodule pointers. This allows us to then batch check for the existence of all the submodule commits instead of checking each change individually (which is what this code path currently does).
Signature · 1 line
Brandon Williams
PATCH 5/6 submodule: improve submodule_has_commits
In reply to Brandon WilliamsTeach 'submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.
This is a prepritory step prior to merging the logic which checkes for changed submodules when fetching or pushing.
--- submodule.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
submodule.c +24 -0
@@ -648,6 +648,30 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)return 0 ;oid_array_for_each_unique (commits, check_has_commit, &has_commit); + +if (has_commit) { +/* +* Even if the submodule is checked out and the commit is +* present, make sure it is reachable from a ref. +*/ +struct child_process cp =CHILD_PROCESS_INIT ; +struct strbuf out =STRBUF_INIT ; + +argv_array_pushl (&cp.args,"rev-list" ,"-n" ,"1" ,NULL ); +oid_array_for_each_unique (commits, append_oid_to_argv, &cp.args); +argv_array_pushl (&cp.args,"--not" ,"--all" ,NULL ); + +prepare_submodule_repo_env (&cp.env_array); + cp.git_cmd =1 ; + cp.no_stdin =1 ; + cp.dir = path; + +if (capture_command (&cp, &out,1024 ) || out.len) + has_commit =0 ; + +strbuf_release (&out); + } +return has_commit; }
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
Re: [PATCH 5/6] submodule: improve submodule_has_commits
Quoted 4 lines · Brandon Williams
Teach '
submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.This is a prepritory step prior to merging the logic which checkes for
s/prepritory/preparatory/
s/checkes/checks/
This is the first commit in the series that changes user observable behavior, I guess there will be tests in a later patch? Can you elaborate in this commit message more about why it is useful (or at least harmless for performing this check in the case of fetch/push)?
Quoted 18 lines
submodule.c +10
-0@@ -648,6 +648,30 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)return 0 ;oid_array_for_each_unique (commits, check_has_commit, &has_commit); + +if (has_commit) { +/* +* Even if the submodule is checked out and the commit is +* present, make sure it is reachable from a ref. +*/ +struct child_process cp =CHILD_PROCESS_INIT ; +struct strbuf out =STRBUF_INIT ; + +argv_array_pushl (&cp.args,"rev-list" ,"-n" ,"1" ,NULL );
submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 670
Quoted 10 lines
submodule.c +10
-0+oid_array_for_each_unique (commits, append_oid_to_argv, &cp.args); +argv_array_pushl (&cp.args,"--not" ,"--all" ,NULL ); + +prepare_submodule_repo_env (&cp.env_array); + cp.git_cmd =1 ; + cp.no_stdin =1 ; + cp.dir = path; + +if (capture_command (&cp, &out,1024 ) || out.len) + has_commit =0 ;
So if we fail to launch capture_command, we assume we do not have the commits?
capture_command can fail for reasons that are hard to track down or even spurious (OOM due to excessive output, disk failure, corrupt repo, error in executing the process, getting a signal and so on), some of them are ok to ignore, others should never be ignored.
So I'd rather die on capture_command, and inspect out.len only in case of successful capturing.
In addition to that we're only interested if there is any output, such that we can optimize further:
c.f. http://public-inbox.org/git/[email protected]/
if (start_command(&cp))
die("cannot start git-rev-list in submodule'%s', sub->path);
/* read only one character, if any */
if (xread(cp.out, &tmp, 1);
has_commit = 0;
/*
* close cp.out, such that the child may get SIGPIPE, upon which
* it dies (silently, maybe we need to suppress cp.err ?)
*/
close(cp.out);
/*
* Though we need to be nitpicky about finish_command IIUC by now:
* TODO(sbeller): if this turns out to be true, fixup is_submodule_modified
*/
int code = finish_command(&cp);
if (!code && code != 128 + SIGPIPE)
die("git rev-list failed in submodule'%s'", sub->path);Upon rereading the patch, I notice the '-n 1', which would make the optimized code above useless, so just consider it food for thought.
Thanks,
Stefan
Re: [PATCH 5/6] submodule: improve submodule_has_commits
Quoted 6 lines · Stefan Beller
Quoted 4 lines · Brandon Williams
Teach '
submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.This is a prepritory step prior to merging the logic which checkes for
Quoted 7 lines
s/prepritory/preparatory/
s/checkes/checks/This is the first commit in the series that changes user observable behavior, I guess there will be tests in a later patch? Can you elaborate in this commit message more about why it is useful (or at least harmless for performing this check in the case of fetch/push)?
This hunk of logic is essentially a copy and paste from elsewhere in the file. Essentially both code paths were essentially doing the same thing (checking if a submodule has a commit) but one of the code paths included this logic to ensure that it was reachable from a ref in the submodule. In order to merge the two code paths, I included that logic here.
Maybe it would make sense to squash this into the next patch as the place where this was copied from is removed?
Signature · 1 line
Brandon Williams
Re: [PATCH 5/6] submodule: improve submodule_has_commits
Quoted 5 lines · Brandon Williams
This hunk of logic is essentially a copy and paste from elsewhere in the file. Essentially both code paths were essentially doing the same thing (checking if a submodule has a commit) but one of the code paths included this logic to ensure that it was reachable from a ref in the submodule. In order to merge the two code paths, I included that logic here.
yes, I get that. The question is whether the other code path omitted the check intentionally or just missed it (Is it a bug or feature to have this check in that code
path now)?
Re: [PATCH 5/6] submodule: improve submodule_has_commits
Quoted 8 lines · Stefan Beller
Quoted 5 lines · Brandon Williams
This hunk of logic is essentially a copy and paste from elsewhere in the file. Essentially both code paths were essentially doing the same thing (checking if a submodule has a commit) but one of the code paths included this logic to ensure that it was reachable from a ref in the submodule. In order to merge the two code paths, I included that logic here.
Quoted 4 lines
yes, I get that. The question is whether the other code path omitted the check intentionally or just missed it (Is it a bug or feature to have this check in that code
path now)?
I'd take a look at Junio's response. This check more than likely needs to be included.
Signature · 1 line
Brandon Williams
Re: [PATCH 5/6] submodule: improve submodule_has_commits
In reply to Brandon WilliamsQuoted 17 lines · Brandon Williams
submodule.c +16
-0oid_array_for_each_unique (commits, check_has_commit, &has_commit); + +if (has_commit) { +/* +* Even if the submodule is checked out and the commit is +* present, make sure it is reachable from a ref. +*/ +struct child_process cp =CHILD_PROCESS_INIT ; +struct strbuf out =STRBUF_INIT ; + +argv_array_pushl (&cp.args,"rev-list" ,"-n" ,"1" ,NULL ); +oid_array_for_each_unique (commits, append_oid_to_argv, &cp.args); +argv_array_pushl (&cp.args,"--not" ,"--all" ,NULL ); + +prepare_submodule_repo_env (&cp.env_array); + cp.git_cmd =1 ; + cp.no_stdin =1 ;
submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 676
Quoted 10 lines
submodule.c +8
-0+ cp.dir = path; + +if (capture_command (&cp, &out,1024 ) || out.len) + has_commit =0 ; + +strbuf_release (&out); + } +return has_commit; }
The "check-has-commit" we see in the pre-context is "we contaminated our in-core object store by tentatively borrowing from submodule's object store---now do we see these commits in our in-core view?" Which is a wrong thing to do from two separate point of view. Even though the commit in question may be visible in our contaminated view, there is no guarantee that the commit exists in the object store of the correct submodule. And of course the commit may exist but may not be anchored by any ref.
This patch fixes the latter, and if we remove that check-has-commit call before it, we can fix the former at the same time.
There is value in leaving the check-has-commit code if we anticipate that we would very often have to say "no, the submodule does not have these commits"---a cheap but wrong check it does can be used as an optimization. If we do not have the commit object anywhere, there is no chance we have it in the object store of the correct submodule and have it reachable from a ref, so we can fail without spawning rev-list which is expensive.
Re: [PATCH 5/6] submodule: improve submodule_has_commits
Quoted 39 lines · Junio C Hamano
Quoted 17 lines · Brandon Williams
submodule.c +16
-0oid_array_for_each_unique (commits, check_has_commit, &has_commit); + +if (has_commit) { +/* +* Even if the submodule is checked out and the commit is +* present, make sure it is reachable from a ref. +*/ +struct child_process cp =CHILD_PROCESS_INIT ; +struct strbuf out =STRBUF_INIT ; + +argv_array_pushl (&cp.args,"rev-list" ,"-n" ,"1" ,NULL ); +oid_array_for_each_unique (commits, append_oid_to_argv, &cp.args); +argv_array_pushl (&cp.args,"--not" ,"--all" ,NULL ); + +prepare_submodule_repo_env (&cp.env_array); + cp.git_cmd =1 ; + cp.no_stdin =1 ;submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 676
Quoted 10 lines
submodule.c +8
-0+ cp.dir = path; + +if (capture_command (&cp, &out,1024 ) || out.len) + has_commit =0 ; + +strbuf_release (&out); + } +return has_commit; }The "check-has-commit" we see in the pre-context is "we contaminated our in-core object store by tentatively borrowing from submodule's object store---now do we see these commits in our in-core view?" Which is a wrong thing to do from two separate point of view. Even though the commit in question may be visible in our contaminated view, there is no guarantee that the commit exists in the object store of the correct submodule. And of course the commit may exist but may not be anchored by any ref.
Quoted 2 lines
This patch fixes the latter, and if we remove that check-has-commit call before it, we can fix the former at the same time.
I noticed this when cleaning up this code but was unsure if I should drop the "check-has-commit" bit.
Quoted 8 lines
There is value in leaving the check-has-commit code if we anticipate that we would very often have to say "no, the submodule does not have these commits"---a cheap but wrong check it does can be used as an optimization. If we do not have the commit object anywhere, there is no chance we have it in the object store of the correct submodule and have it reachable from a ref, so we can fail without spawning rev-list which is expensive.
Mostly because it gave the code a way to fail quickly, of course I'm making the assumption that polluting the object store than then checking it is quicker than launching a child process (though I guess most things are cheaper than launching a process ;)
Signature · 1 line
Brandon Williams
PATCH 6/6 submodule: refactor logic to determine changed submodules
In reply to Brandon WilliamsThere are currently two instances (fetch and push) where we want to determine if submodules have changed given some revision specification. These two instances don't use the same logic to generate a list of changed submodules and as a result there is a fair amount of code duplication.
This patch refactors these two code paths such that they both use the same logic to generate a list of changed submodules. This also makes it easier for future callers to be able to reuse this logic as they only need to create an argv_array with the revision specification to be using during the revision walk.
--- submodule.c | 247 ++++++++++++++++++++++++++---------------------------------- 1 file changed, 105 insertions(+), 142 deletions(-)
submodule.c +105 -142
@@ -617,6 +617,94 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)return submodule_from_path (null_sha1, ce->name); } +static struct oid_array *submodule_commits (struct string_list *submodules, +const char *path) +{ +struct string_list_item *item; + + item =string_list_insert (submodules, path); +if (item->util) +return (struct oid_array *) item->util; + +/* NEEDSWORK: should we have oid_array_init()? */ + item->util =xcalloc (1 ,sizeof (struct oid_array )); +return (struct oid_array *) item->util; +} + +static void collect_changed_submodules_cb (struct diff_queue_struct *q, +struct diff_options *options, +void *data) +{ +int i; +struct string_list *changed = data; + +for (i =0 ; i < q->nr; i++) { +struct diff_filepair *p = q->queue[i]; +struct oid_array *commits; +if (!S_ISGITLINK (p->two->mode)) +continue ; + +if (S_ISGITLINK (p->one->mode)) { +/* +* NEEDSWORK: We should honor the name configured in +* the .gitmodules file of the commit we are examining +* here to be able to correctly follow submodules +* being moved around. +*/ + commits =submodule_commits (changed, p->two->path); +oid_array_append (commits, &p->two->oid); + }else { +/* Submodule is new or was moved here */ +/* +* NEEDSWORK: When the .git directories of submodules +* live inside the superprojects .git directory some +* day we should fetch new submodules directly into +* that location too when config or options request +* that so they can be checked out from there. +*/ +continue ; + } + } +} + +/* +* Collect the paths of submodules in 'changed' which have changed based on +* the revisions as specified in 'argv'. Each entry in 'changed' will also +* have a corresponding 'struct oid_array' (in the 'util' field) which lists +* what the submodule pointers were updated to during the change. +*/ +static void collect_changed_submodules (struct string_list *changed, +struct argv_array *argv) +{ +struct rev_info rev; +const struct commit *commit; + +init_revisions (&rev,NULL ); +setup_revisions (argv->argc, argv->argv, &rev,NULL ); +if (prepare_revision_walk (&rev)) +die ("revision walk setup failed" ); + +while ((commit =get_revision (&rev))) { +struct rev_info diff_rev; + +init_revisions (&diff_rev,NULL ); + diff_rev.diffopt.output_format |=DIFF_FORMAT_CALLBACK ; + diff_rev.diffopt.format_callback = collect_changed_submodules_cb; + diff_rev.diffopt.format_callback_data = changed; +diff_tree_combined_merge (commit,1 , &diff_rev); + } + +reset_revision_walk (); +} + +static void free_submodules_oids (struct string_list *submodules) +{ +struct string_list_item *item; +for_each_string_list_item (item, submodules) +oid_array_clear ((struct oid_array *) item->util); +string_list_clear (submodules,1 ); +} +static int has_remote (const char *refname,const struct object_id *oid,int flags,void *cb_data) {
@@ -719,92 +807,31 @@ static int submodule_needs_pushing(const char *path, struct oid_array *commits)return 0 ; } -static struct oid_array *submodule_commits (struct string_list *submodules, -const char *path) -{ -struct string_list_item *item; - - item =string_list_insert (submodules, path); -if (item->util) -return (struct oid_array *) item->util; - -/* NEEDSWORK: should we have oid_array_init()? */ - item->util =xcalloc (1 ,sizeof (struct oid_array )); -return (struct oid_array *) item->util; -} - -static void collect_submodules_from_diff (struct diff_queue_struct *q, -struct diff_options *options, -void *data) -{ -int i; -struct string_list *submodules = data; - -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -struct oid_array *commits; -if (!S_ISGITLINK (p->two->mode)) -continue ; - commits =submodule_commits (submodules, p->two->path); -oid_array_append (commits, &p->two->oid); - } -} - -static void find_unpushed_submodule_commits (struct commit *commit, -struct string_list *needs_pushing) -{ -struct rev_info rev; - -init_revisions (&rev,NULL ); - rev.diffopt.output_format |=DIFF_FORMAT_CALLBACK ; - rev.diffopt.format_callback = collect_submodules_from_diff; - rev.diffopt.format_callback_data = needs_pushing; -diff_tree_combined_merge (commit,1 , &rev); -} - -static void free_submodules_oids (struct string_list *submodules) -{ -struct string_list_item *item; -for_each_string_list_item (item, submodules) -oid_array_clear ((struct oid_array *) item->util); -string_list_clear (submodules,1 ); -} -int find_unpushed_submodules (struct oid_array *commits,const char *remotes_name,struct string_list *needs_pushing) { -struct rev_info rev; -struct commit *commit;struct string_list submodules =STRING_LIST_INIT_DUP ;struct string_list_item *submodule;struct argv_array argv =ARGV_ARRAY_INIT ; -init_revisions (&rev,NULL ); -/* argv.argv[0] will be ignored by setup_revisions */ argv_array_push (&argv,"find_unpushed_submodules" );oid_array_for_each_unique (commits, append_oid_to_argv, &argv);argv_array_push (&argv,"--not" );argv_array_pushf (&argv,"--remotes=%s" , remotes_name); -setup_revisions (argv.argc, argv.argv, &rev,NULL ); -if (prepare_revision_walk (&rev)) -die ("revision walk setup failed" ); - -while ((commit =get_revision (&rev)) !=NULL ) -find_unpushed_submodule_commits (commit, &submodules); - -reset_revision_walk (); -argv_array_clear (&argv); +collect_changed_submodules (&submodules, &argv);for_each_string_list_item (submodule, &submodules) { -struct oid_array *commits = (struct oid_array *) submodule->util; +struct oid_array *commits = submodule->util; +const char *path = submodule->string; -if (submodule_needs_pushing (submodule->string, commits)) -string_list_insert (needs_pushing, submodule->string); +if (submodule_needs_pushing (path, commits)) +string_list_insert (needs_pushing, path); }free_submodules_oids (&submodules); +argv_array_clear (&argv);return needs_pushing->nr; }
@@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,return ret; } -static int is_submodule_commit_present (const char *path,unsigned char sha1[20 ]) -{ -int is_present =0 ; -if (!add_submodule_odb (path) &&lookup_commit_reference (sha1)) { -/* Even if the submodule is checked out and the commit is -* present, make sure it is reachable from a ref. */ -struct child_process cp =CHILD_PROCESS_INIT ; -const char *argv[] = {"rev-list" ,"-n" ,"1" ,NULL ,"--not" ,"--all" ,NULL }; -struct strbuf buf =STRBUF_INIT ; - - argv[3 ] =sha1_to_hex (sha1); - cp.argv = argv; -prepare_submodule_repo_env (&cp.env_array); - cp.git_cmd =1 ; - cp.no_stdin =1 ; - cp.dir = path; -if (!capture_command (&cp, &buf,1024 ) && !buf.len) - is_present =1 ; - -strbuf_release (&buf); - } -return is_present; -} - -static void submodule_collect_changed_cb (struct diff_queue_struct *q, -struct diff_options *options, -void *data) -{ -int i; -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (!S_ISGITLINK (p->two->mode)) -continue ; - -if (S_ISGITLINK (p->one->mode)) { -/* NEEDSWORK: We should honor the name configured in -* the .gitmodules file of the commit we are examining -* here to be able to correctly follow submodules -* being moved around. */ -struct string_list_item *path; - path =unsorted_string_list_lookup (&changed_submodule_paths, p->two->path); -if (!path && !is_submodule_commit_present (p->two->path, p->two->oid.hash)) -string_list_append (&changed_submodule_paths, p->two->path); - }else { -/* Submodule is new or was moved here */ -/* NEEDSWORK: When the .git directories of submodules -* live inside the superprojects .git directory some -* day we should fetch new submodules directly into -* that location too when config or options request -* that so they can be checked out from there. */ -continue ; - } - } -} -static int append_oid_to_array (const char *ref,const struct object_id *oid,int flags,void *data) {
@@ -996,45 +968,36 @@ void check_for_new_submodule_commits(struct object_id *oid)static void calculate_changed_submodule_paths (void ) { -struct rev_info rev; -struct commit *commit;struct argv_array argv =ARGV_ARRAY_INIT ; +struct string_list changed_submodules =STRING_LIST_INIT_DUP ; +const struct string_list_item *item;/* No need to check if there are no submodules configured */ if (!submodule_from_path (NULL ,NULL ))return ; -init_revisions (&rev,NULL );argv_array_push (&argv,"--" );/* argv[0] program name */ oid_array_for_each_unique (&ref_tips_after_fetch, append_oid_to_argv, &argv);argv_array_push (&argv,"--not" );oid_array_for_each_unique (&ref_tips_before_fetch, append_oid_to_argv, &argv); -setup_revisions (argv.argc, argv.argv, &rev,NULL ); -if (prepare_revision_walk (&rev)) -die ("revision walk setup failed" );/* * Collect all submodules (whether checked out or not) for which new * commits have been recorded upstream in "changed_submodule_paths". */ -while ((commit =get_revision (&rev))) { -struct commit_list *parent = commit->parents; -while (parent) { -struct diff_options diff_opts; -diff_setup (&diff_opts); -DIFF_OPT_SET (&diff_opts,RECURSIVE ); - diff_opts.output_format |=DIFF_FORMAT_CALLBACK ; - diff_opts.format_callback = submodule_collect_changed_cb; -diff_setup_done (&diff_opts); -diff_tree_sha1 (parent->item->object.oid.hash, commit->object.oid.hash,"" , &diff_opts); -diffcore_std (&diff_opts); -diff_flush (&diff_opts); - parent = parent->next; - } +collect_changed_submodules (&changed_submodules, &argv); + +for_each_string_list_item (item, &changed_submodules) { +struct oid_array *commits = item->util; +const char *path = item->string; + +if (!submodule_has_commits (path, commits)) +string_list_append (&changed_submodule_paths, path); } +free_submodules_oids (&changed_submodules);argv_array_clear (&argv);oid_array_clear (&ref_tips_before_fetch);oid_array_clear (&ref_tips_after_fetch);
Signature · 1 line
2.13.0.rc0.306.g87b477812d-goog
Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules
+ Heiko, who touched the pushing code end of last year.
Quoted 11 lines · Brandon Williams
There are currently two instances (fetch and push) where we want to determine if submodules have changed given some revision specification. These two instances don't use the same logic to generate a list of changed submodules and as a result there is a fair amount of code duplication.
This patch refactors these two code paths such that they both use the same logic to generate a list of changed submodules. This also makes it easier for future callers to be able to reuse this logic as they only need to create an
argv_arraywith the revision specification to be using during the revision walk.
Thanks for doing some refactoring. :)
submodule.c static int submodule_needs_pushing(const char *path, struct oid_array *commits)
Quoted 7 lines
submodule.c +0
-4-static struct oid_array *submodule_commits (struct string_list *submodules, -const char *path) -static void free_submodules_oids(struct string_list *submodules) -{
These are just moved north, no change in code.
If you want to be extra nice to reviewers this could have been an extra patch, as it makes reviewing easier if you just have to look at the lines of code with one goal ("shuffling code around, no change intended" vs "make a change to improve things with no bad side effects")
submodule.c const struct submodule *submodule_from_ce(const struct cache_entry *ce) · line 637
Quoted 5 lines
submodule.c +5
-0+ +static void collect_changed_submodules_cb (struct diff_queue_struct *q, +struct diff_options *options, +void *data) +{
This one combines both collect_submodules_from_diff and submodule_collect_changed_cb ?
Quoted 12 lines
submodule.c +0
-8@@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,return ret; } -static int is_submodule_commit_present (const char *path,unsigned char sha1[20 ]) -{ -int is_present =0 ; -if (!add_submodule_odb (path) &&lookup_commit_reference (sha1)) { -/* Even if the submodule is checked out and the commit is -* present, make sure it is reachable from a ref. */ -struct child_process cp =CHILD_PROCESS_INIT ; -const char *argv[] = {"rev-list" ,"-n" ,"1" ,NULL ,"--not" ,"--all" ,NULL };
submodule.c int push_unpushed_submodules(struct oid_array *commits, · line 941
Quoted 10 lines
submodule.c +0
-10-struct strbuf buf =STRBUF_INIT ; - - argv[3 ] =sha1_to_hex (sha1); - cp.argv = argv; -prepare_submodule_repo_env (&cp.env_array); - cp.git_cmd =1 ; - cp.no_stdin =1 ; - cp.dir = path; -if (!capture_command (&cp, &buf,1024 ) && !buf.len) - is_present =1 ;
Oh, I see where the nit in patch 5/6 is coming from. Another note on that: The hint is way off. The hint should be on the order of GIT_SHA1_HEXSZ
submodule.c static int submodule_needs_pushing(const char *path, struct oid_array *commits)
Quoted 6 lines
submodule.c +0
-0int find_unpushed_submodules(struct oid_array *commits, const char *remotes_name, struct string_list *needs_pushing) static void calculate_changed_submodule_paths(void)
These are both nicely clean now.
Thanks,
Stefan
Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules
Quoted 21 lines · Stefan Beller
+ Heiko, who touched the pushing code end of last year.
Quoted 11 lines · Brandon Williams
There are currently two instances (fetch and push) where we want to determine if submodules have changed given some revision specification. These two instances don't use the same logic to generate a list of changed submodules and as a result there is a fair amount of code duplication.
This patch refactors these two code paths such that they both use the same logic to generate a list of changed submodules. This also makes it easier for future callers to be able to reuse this logic as they only need to create an
argv_arraywith the revision specification to be using during the revision walk.Thanks for doing some refactoring. :)
Quoted 3 lines
submodule.c +0
-2-static struct oid_array *submodule_commits (struct string_list *submodules, -const char *path)
Quoted 9 lines
submodule.c static int submodule_needs_pushing(const char *path, struct oid_array *commits) · line 766
Quoted 3 lines
submodule.c +0
-2-static void free_submodules_oids (struct string_list *submodules) -{These are just moved north, no change in code.
If you want to be extra nice to reviewers this could have been an extra patch, as it makes reviewing easier if you just have to look at the lines of code with one goal ("shuffling code around, no change intended" vs "make a change to improve things with no bad side effects")
Yeah I suppose, I was having a difficult time breaking this largest patch into multiple.
Quoted 9 lines
submodule.c const struct submodule *submodule_from_ce(const struct cache_entry *ce) · line 637
Quoted 6 lines
submodule.c +5
-0+ +static void collect_changed_submodules_cb (struct diff_queue_struct *q, +struct diff_options *options, +void *data) +{This one combines both
collect_submodules_from_diffandsubmodule_collect_changed_cb?
Yep.
Quoted 19 lines
Quoted 19 lines
submodule.c +0
-14@@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,return ret; } -static int is_submodule_commit_present (const char *path,unsigned char sha1[20 ]) -{ -int is_present =0 ; -if (!add_submodule_odb (path) &&lookup_commit_reference (sha1)) { -/* Even if the submodule is checked out and the commit is -* present, make sure it is reachable from a ref. */ -struct child_process cp =CHILD_PROCESS_INIT ; -const char *argv[] = {"rev-list" ,"-n" ,"1" ,NULL ,"--not" ,"--all" ,NULL }; -struct strbuf buf =STRBUF_INIT ; - - argv[3 ] =sha1_to_hex (sha1); - cp.argv = argv; -prepare_submodule_repo_env (&cp.env_array); - cp.git_cmd =1 ;
Quoted 8 lines
submodule.c int push_unpushed_submodules(struct oid_array *commits, · line 941
Quoted 4 lines
submodule.c +0
-4- cp.no_stdin =1 ; - cp.dir = path; -if (!capture_command (&cp, &buf,1024 ) && !buf.len) - is_present =1 ;Oh, I see where the nit in patch 5/6 is coming from. Another note on that: The hint is way off. The hint should be on the order of
GIT_SHA1_HEXSZ
Yeah agreed. I make this change in the earlier patch.
Quoted 12 lines
submodule.c static int submodule_needs_pushing(const char *path, struct oid_array *commits)
Quoted 7 lines
submodule.c +0
-0int find_unpushed_submodules(struct oid_array *commits, const char *remotes_name, struct string_list *needs_pushing) static void calculate_changed_submodule_paths(void)These are both nicely clean now.
Thanks,
Stefan
Signature · 1 line
Brandon Williams
Quoted 9 lines · Brandon Williams
A little bit ago I was looking for a function in our code base which would be able to give me a list of all the submodules which changed across a number of commits. I stumbled upon some of that logic in both our recurisve fetch and pull code paths. Problem is both of these code paths were performing almost identical tasks, without sharing any code.
This series is meant as a cleanup to
submodule.cin order to unify these two code paths as well as make it easier for future callers to be able to query for submodules which have chagned across a number of commits.
Yay. Nice to see code reduction.
Quoted 11 lines
Brandon Williams (6):
submodule: renameadd_sha1_to_array
submodule: renamefree_submodules_sha1s
submodule: removeadd_oid_to_argv
submodule: changestring_listchanged_submodule_paths
submodule: improvesubmodule_has_commits
submodule: refactor logic to determine changed submodulessubmodule.c | 295 ++++++++++++++++++++++++++++-------------------------------- 1 file changed, 139 insertions(+), 156 deletions(-)
v2 cover changed submodules
In reply to Brandon WilliamsChanges in v2:
- few tweaks in the commit messages of a couple of the commits
- use 'GIT_MAX_HEXSZ + 1' as the hint size in [5/6]
- added a comment in [5/6] better explaining the rational for having a quick, incorrect check for the existence of a commit in a submodule.
Brandon Williams (6):
submodule: rename add_sha1_to_array
submodule: rename free_submodules_sha1s
submodule: remove add_oid_to_argv
submodule: change string_list changed_submodule_paths
submodule: improve submodule_has_commits
submodule: refactor logic to determine changed submodules
submodule.c | 305 +++++++++++++++++++++++++++++------------------------------- 1 file changed, 149 insertions(+), 156 deletions(-)
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
PATCH v2 1/6 submodule: rename add_sha1_to_array
Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more accurately describe what the function does since it handles 'struct object_id' and not sha1 character arrays.
--- submodule.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
submodule.c +5 -4
@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q, } } -static int add_sha1_to_array (const char *ref,const struct object_id *oid, -int flags,void *data) +static int append_oid_to_array (const char *ref,const struct object_id *oid, +int flags,void *data) { -oid_array_append (data, oid); +struct oid_array *array = data; +oid_array_append (array, oid);return 0 ; }void check_for_new_submodule_commits (struct object_id *oid) {if (!initialized_fetch_ref_tips) { -for_each_ref (add_sha1_to_array, &ref_tips_before_fetch); +for_each_ref (append_oid_to_array, &ref_tips_before_fetch); initialized_fetch_ref_tips =1 ; }
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
Re: [PATCH v2 1/6] submodule: rename add_sha1_to_array
Quoted 1 line · Brandon Williams
Change-Id: Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf
Uh?
Maybe another side project for the long todo list: get git-trailers into shape, such that it can be configured to yell at you upon formatting the patch or sending email when a given trailer occurs.
Thanks,
Stefan
Re: [PATCH v2 1/6] submodule: rename add_sha1_to_array
Quoted 5 lines · Stefan Beller
Quoted 1 line · Brandon Williams
Change-Id: Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf
Uh?
Oops! I forgot to run it through my scrubbing script...
Quoted 7 lines
Maybe another side project for the long todo list: get git-trailers into shape, such that it can be configured to yell at you upon formatting the patch or sending email when a given trailer occurs.
Thanks,
Stefan
Signature · 1 line
Brandon Williams
PATCH v2 2/6 submodule: rename free_submodules_sha1s
In reply to Brandon WilliamsRename 'free_submodules_sha1s()' to 'free_submodules_oids()' since the function frees a 'struct string_list' which has a 'struct oid_array' stored in the 'util' field.
--- submodule.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
submodule.c +3 -2
@@ -738,7 +738,7 @@ static void find_unpushed_submodule_commits(struct commit *commit,diff_tree_combined_merge (commit,1 , &rev); } -static void free_submodules_sha1s (struct string_list *submodules) +static void free_submodules_oids (struct string_list *submodules) {struct string_list_item *item;for_each_string_list_item (item, submodules)
@@ -779,7 +779,8 @@ int find_unpushed_submodules(struct oid_array *commits,if (submodule_needs_pushing (submodule->string, commits))string_list_insert (needs_pushing, submodule->string); } -free_submodules_sha1s (&submodules); + +free_submodules_oids (&submodules);return needs_pushing->nr; }
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
PATCH v2 3/6 submodule: remove add_oid_to_argv
In reply to Brandon WilliamsThe function 'add_oid_to_argv()' provides the same functionality as 'append_oid_to_argv()'. Remove this duplicate function and instead use 'append_oid_to_argv()' where 'add_oid_to_argv()' was previously used.
--- submodule.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
submodule.c +2 -8
@@ -970,12 +970,6 @@ void check_for_new_submodule_commits(struct object_id *oid)oid_array_append (&ref_tips_after_fetch, oid); } -static int add_oid_to_argv (const struct object_id *oid,void *data) -{ -argv_array_push (data,oid_to_hex (oid)); -return 0 ; -} -static void calculate_changed_submodule_paths (void ) {struct rev_info rev;
@@ -989,10 +983,10 @@ static void calculate_changed_submodule_paths(void)init_revisions (&rev,NULL );argv_array_push (&argv,"--" );/* argv[0] program name */ oid_array_for_each_unique (&ref_tips_after_fetch, - add_oid_to_argv, &argv); + append_oid_to_argv, &argv);argv_array_push (&argv,"--not" );oid_array_for_each_unique (&ref_tips_before_fetch, - add_oid_to_argv, &argv); + append_oid_to_argv, &argv);setup_revisions (argv.argc, argv.argv, &rev,NULL );if (prepare_revision_walk (&rev))die ("revision walk setup failed" );
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
PATCH v2 4/6 submodule: change string_list changed_submodule_paths
In reply to Brandon WilliamsEliminate a call to 'xstrdup()' by changing the string_list 'changed_submodule_paths' to duplicated strings added to it.
--- submodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
submodule.c +2 -2
@@ -20,7 +20,7 @@static int config_fetch_recurse_submodules =RECURSE_SUBMODULES_ON_DEMAND ;static int config_update_recurse_submodules =RECURSE_SUBMODULES_DEFAULT ;static int parallel_jobs =1 ; -static struct string_list changed_submodule_paths =STRING_LIST_INIT_NODUP ; +static struct string_list changed_submodule_paths =STRING_LIST_INIT_DUP ;static int initialized_fetch_ref_tips;static struct oid_array ref_tips_before_fetch;static struct oid_array ref_tips_after_fetch;
@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,struct string_list_item *path; path =unsorted_string_list_lookup (&changed_submodule_paths, p->two->path);if (!path && !is_submodule_commit_present (p->two->path, p->two->oid.hash)) -string_list_append (&changed_submodule_paths,xstrdup (p->two->path)); +string_list_append (&changed_submodule_paths, p->two->path); }else {/* Submodule is new or was moved here */ /* NEEDSWORK: When the .git directories of submodules
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
PATCH v2 5/6 submodule: improve submodule_has_commits
In reply to Brandon WilliamsTeach 'submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.
This is a preparatory step prior to merging the logic which checks for changed submodules when fetching or pushing.
--- submodule.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
submodule.c +34 -0
@@ -644,10 +644,44 @@ static int submodule_has_commits(const char *path, struct oid_array *commits) {int has_commit =1 ; +/* +* Perform a cheap, but incorrect check for the existance of 'commits'. +* This is done by adding the submodule's object store to the in-core +* object store, and then querying for each commit's existance. If we +* do not have the commit object anywhere, there is no chance we have +* it in the object store of the correct submodule and have it +* reachable from a ref, so we can fail early without spawning rev-list +* which is expensive. +*/ if (add_submodule_odb (path))return 0 ;oid_array_for_each_unique (commits, check_has_commit, &has_commit); + +if (has_commit) { +/* +* Even if the submodule is checked out and the commit is +* present, make sure it exists in the submodule's object store +* and that it is reachable from a ref. +*/ +struct child_process cp =CHILD_PROCESS_INIT ; +struct strbuf out =STRBUF_INIT ; + +argv_array_pushl (&cp.args,"rev-list" ,"-n" ,"1" ,NULL ); +oid_array_for_each_unique (commits, append_oid_to_argv, &cp.args); +argv_array_pushl (&cp.args,"--not" ,"--all" ,NULL ); + +prepare_submodule_repo_env (&cp.env_array); + cp.git_cmd =1 ; + cp.no_stdin =1 ; + cp.dir = path; + +if (capture_command (&cp, &out,GIT_MAX_HEXSZ +1 ) || out.len) + has_commit =0 ; + +strbuf_release (&out); + } +return has_commit; }
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
Quoted 21 lines · Brandon Williams
Teach '
submodule_has_commits()' to ensure that if a commit exists in a submodule, that it is also reachable from a ref.This is a preparatory step prior to merging the logic which checks for changed submodules when fetching or pushing.
Change-Id I4fed2acfa7e69a5fbbca534df165671e77a90f22Signed-off-by Brandon Williams <[email protected]>--- submodule.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)submodule.c +1
-0@@ -644,10 +644,44 @@ static int submodule_has_commits(const char *path, struct oid_array *commits) {int has_commit =1 ; +/*
submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 657
Quoted 10 lines
submodule.c +8
-0+* Perform a cheap, but incorrect check for the existance of 'commits'. +* This is done by adding the submodule's object store to the in-core +* object store, and then querying for each commit's existance. If we +* do not have the commit object anywhere, there is no chance we have +* it in the object store of the correct submodule and have it +* reachable from a ref, so we can fail early without spawning rev-list +* which is expensive. +*/ if (add_submodule_odb (path))return 0 ;
Thanks for the comment!
Quoted 12 lines
submodule.c +10
-0oid_array_for_each_unique (commits, check_has_commit, &has_commit); + +if (has_commit) { +/* +* Even if the submodule is checked out and the commit is +* present, make sure it exists in the submodule's object store +* and that it is reachable from a ref. +*/ +struct child_process cp =CHILD_PROCESS_INIT ; +struct strbuf out =STRBUF_INIT ; +
submodule.c static int submodule_has_commits(const char *path, struct oid_array *commits) · line 679
Quoted 10 lines
submodule.c +10
-0+argv_array_pushl (&cp.args,"rev-list" ,"-n" ,"1" ,NULL ); +oid_array_for_each_unique (commits, append_oid_to_argv, &cp.args); +argv_array_pushl (&cp.args,"--not" ,"--all" ,NULL ); + +prepare_submodule_repo_env (&cp.env_array); + cp.git_cmd =1 ; + cp.no_stdin =1 ; + cp.dir = path; + +if (capture_command (&cp, &out,GIT_MAX_HEXSZ +1 ) || out.len)
eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:
if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);
if (out.len)
has_commit = 0;instead as that does not have a silent error. (though it errs on the safe side, so maybe it is not to bad.)
I could understand if the callers do not want to have submodule_has_commits die()-ing on them, so maybe
if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) {
warning("cannot capture git-rev-list in submodule '%s', sub->path);
has_commit = -1;
/* this would require auditing all callers and handling -1 though */
}
if (out.len)
has_commit = 0;As the comment eludes, we'd then have
0 -> has no commits
1 -> has commits
-1 -> error
So to group (error || has_no_commits), we could write
if (submodule_has_commits(..) <= 0)
which is awkward. So maybe we can rename the function to misses_submodule_commits instead, as then we could flip the return value as well and have
0 -> has commits
1 -> has no commits
-1 -> error
and the lazy invoker could just go with
if (!misses_submodule_commits(..))
proceed();
else
die("missing submodule commits or errors; I don't care");whereas the careful invoker could go with
switch (misses_submodule_commits(..)) {
case 0:
proceed(); break;
case 1:
pull_magic_trick(); break;
case -1:
make_errors_go_away_and_retry(); break;
}---
On the longer term plan:
As you wrote about costs. Maybe instead of invoking rev-list, we could try to have this in-core as a first try-out for "classified-repos", looking at refs.h there is e.g.
int for_each_ref_submodule(const char *submodule_path,
each_ref_fn fn, void *cb_data);
which we could use to obtain all submodule refs and then use the revision walking machinery to find out ourselves if we have or do not have the commits. (As we loaded the odb of the submodule, this would *just work*, building one kludgy hack upon the next.)
Thanks,
Stefan
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
Quoted 9 lines · Stefan Beller
Review
Quoted 2 lines · Brandon Williams
(quoted diff) +2
-0+ + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:
if (
capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);
This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.
Quoted 45 lines
if (out.len) has_commit = 0;instead as that does not have a silent error. (though it errs on the safe side, so maybe it is not to bad.)
I could understand if the callers do not want to have
submodule_has_commitsdie()-ing on them, so maybeif (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) { warning("cannot capture git-rev-list in submodule '%s', sub->path); has_commit = -1; /* this would require auditing all callers and handling -1 though */ } if (out.len) has_commit = 0;As the comment eludes, we'd then have
0 -> has no commits
1 -> has commits
-1 -> errorSo to group (error ||
has_no_commits), we could writeif (submodule_has_commits(..) <= 0)which is awkward. So maybe we can rename the function to
misses_submodule_commitsinstead, as then we could flip the return value as well and have0 -> has commits
1 -> has no commits
-1 -> errorand the lazy invoker could just go with
if (!misses_submodule_commits(..)) proceed(); else die("missing submodule commits or errors; I don't care");whereas the careful invoker could go with
Quoted 8 lines
switch (misses_submodule_commits(..)) {
case 0:
proceed(); break;
case 1:
pull_magic_trick(); break;
case -1:
make_errors_go_away_and_retry(); break;
}I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.
Quoted 17 lines
---
On the longer term plan:
As you wrote about costs. Maybe instead of invoking rev-list, we could try to have this in-core as a first try-out for "classified-repos", looking atrefs.hthere is e.g.int for_each_ref_submodule(const char
*submodule_path,
each_ref_fnfn, void*cb_data);which we could use to obtain all submodule refs and then use the revision walking machinery to find out ourselves if we have or do not have the commits. (As we loaded the odb of the submodule, this would *just work*, building one kludgy hack upon the next.)
Thanks,
Stefan
Signature · 1 line
Brandon Williams
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
Quoted 8 lines · Brandon Williams
Quoted 7 lines · Stefan Beller
Review
Quoted 2 lines · Brandon Williams
(quoted diff) +2
-0+ + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:
Quoted 6 lines
Quoted 2 lines
if (
capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.
Oh. In that case, why do we even check for its stdout?
(from rev-lists man page)
--quiet
Don’t print anything to standard output. This form is primarily meant to allow the caller to test the exit status to see if a range of objects is fully connected (or not). It is faster than redirecting stdout to /dev/null as the output does not have to be formatted.
Quoted 4 lines
I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.
ok. I was just giving my thoughts on how I would approach it.
Thanks,
Stefan
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
Quoted 16 lines · Stefan Beller
Quoted 8 lines · Brandon Williams
Quoted 7 lines · Stefan Beller
Review
Quoted 2 lines · Brandon Williams
(quoted diff) +2
-0+ + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:
Quoted 6 lines
Quoted 2 lines
if (
capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.
Quoted 9 lines
Oh. In that case, why do we even check for its stdout?
(from rev-lists man page)
--quiet
Don’t print anything to standard output. This form is primarily meant to allow the caller to test the exit status to see if a range of objects is fully connected (or not). It is faster than redirecting stdout to/dev/nullas the output does not have to be formatted.
Sounds good, Let me take a look at using --quiet
Quoted 9 lines
Quoted 4 lines
I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.
ok. I was just giving my thoughts on how I would approach it.
Thanks,
Stefan
Signature · 1 line
Brandon Williams
Re: [PATCH v2 5/6] submodule: improve submodule_has_commits
Quoted 27 lines · Brandon Williams
Quoted 16 lines · Stefan Beller
Quoted 8 lines · Brandon Williams
Quoted 7 lines · Stefan Beller
Review
Quoted 2 lines · Brandon Williams
(quoted diff) +2
-0+ + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)eh, I gave too much and self-contradicting feedback here earlier, ideally I'd like to review this to be similar as:
Quoted 6 lines
Quoted 2 lines
if (
capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)
die("cannot capture git-rev-list in submodule '%s', sub->path);This wouldn't really work because if you provide a SHA1 to rev-list which it isn't able to find then it returns a non-zero exit code which would cause this to die, which isn't the desired behavior.
Quoted 9 lines
Oh. In that case, why do we even check for its stdout?
(from rev-lists man page)
--quiet
Don’t print anything to standard output. This form is primarily meant to allow the caller to test the exit status to see if a range of objects is fully connected (or not). It is faster than redirecting stdout to/dev/nullas the output does not have to be formatted.
Quoted 1 line
Sounds good, Let me take a look at using --quiet
From our offline discussion --quiet doesn't do what we want here. We are checking (1) if the commit exists and (2) if it is reachable. Using --quiet would only satisfy (1)
Quoted 13 lines
Quoted 10 lines
Quoted 5 lines
I feel like you're making this a little too complicated, as all I'm doing is shuffling around already existing logic. I understand the want to make things more robust but this seems unnecessarily complex.
ok. I was just giving my thoughts on how I would approach it.
Thanks,
StefanSignature · 1 line
Brandon Williams
Signature · 1 line
Brandon Williams
PATCH v2 6/6 submodule: refactor logic to determine changed submodules
In reply to Brandon WilliamsThere are currently two instances (fetch and push) where we want to determine if submodules have changed given some revision specification. These two instances don't use the same logic to generate a list of changed submodules and as a result there is a fair amount of code duplication.
This patch refactors these two code paths such that they both use the same logic to generate a list of changed submodules. This also makes it easier for future callers to be able to reuse this logic as they only need to create an argv_array with the revision specification to be using during the revision walk.
--- submodule.c | 247 ++++++++++++++++++++++++++---------------------------------- 1 file changed, 105 insertions(+), 142 deletions(-)
submodule.c +105 -142
@@ -617,6 +617,94 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)return submodule_from_path (null_sha1, ce->name); } +static struct oid_array *submodule_commits (struct string_list *submodules, +const char *path) +{ +struct string_list_item *item; + + item =string_list_insert (submodules, path); +if (item->util) +return (struct oid_array *) item->util; + +/* NEEDSWORK: should we have oid_array_init()? */ + item->util =xcalloc (1 ,sizeof (struct oid_array )); +return (struct oid_array *) item->util; +} + +static void collect_changed_submodules_cb (struct diff_queue_struct *q, +struct diff_options *options, +void *data) +{ +int i; +struct string_list *changed = data; + +for (i =0 ; i < q->nr; i++) { +struct diff_filepair *p = q->queue[i]; +struct oid_array *commits; +if (!S_ISGITLINK (p->two->mode)) +continue ; + +if (S_ISGITLINK (p->one->mode)) { +/* +* NEEDSWORK: We should honor the name configured in +* the .gitmodules file of the commit we are examining +* here to be able to correctly follow submodules +* being moved around. +*/ + commits =submodule_commits (changed, p->two->path); +oid_array_append (commits, &p->two->oid); + }else { +/* Submodule is new or was moved here */ +/* +* NEEDSWORK: When the .git directories of submodules +* live inside the superprojects .git directory some +* day we should fetch new submodules directly into +* that location too when config or options request +* that so they can be checked out from there. +*/ +continue ; + } + } +} + +/* +* Collect the paths of submodules in 'changed' which have changed based on +* the revisions as specified in 'argv'. Each entry in 'changed' will also +* have a corresponding 'struct oid_array' (in the 'util' field) which lists +* what the submodule pointers were updated to during the change. +*/ +static void collect_changed_submodules (struct string_list *changed, +struct argv_array *argv) +{ +struct rev_info rev; +const struct commit *commit; + +init_revisions (&rev,NULL ); +setup_revisions (argv->argc, argv->argv, &rev,NULL ); +if (prepare_revision_walk (&rev)) +die ("revision walk setup failed" ); + +while ((commit =get_revision (&rev))) { +struct rev_info diff_rev; + +init_revisions (&diff_rev,NULL ); + diff_rev.diffopt.output_format |=DIFF_FORMAT_CALLBACK ; + diff_rev.diffopt.format_callback = collect_changed_submodules_cb; + diff_rev.diffopt.format_callback_data = changed; +diff_tree_combined_merge (commit,1 , &diff_rev); + } + +reset_revision_walk (); +} + +static void free_submodules_oids (struct string_list *submodules) +{ +struct string_list_item *item; +for_each_string_list_item (item, submodules) +oid_array_clear ((struct oid_array *) item->util); +string_list_clear (submodules,1 ); +} +static int has_remote (const char *refname,const struct object_id *oid,int flags,void *cb_data) {
@@ -729,92 +817,31 @@ static int submodule_needs_pushing(const char *path, struct oid_array *commits)return 0 ; } -static struct oid_array *submodule_commits (struct string_list *submodules, -const char *path) -{ -struct string_list_item *item; - - item =string_list_insert (submodules, path); -if (item->util) -return (struct oid_array *) item->util; - -/* NEEDSWORK: should we have oid_array_init()? */ - item->util =xcalloc (1 ,sizeof (struct oid_array )); -return (struct oid_array *) item->util; -} - -static void collect_submodules_from_diff (struct diff_queue_struct *q, -struct diff_options *options, -void *data) -{ -int i; -struct string_list *submodules = data; - -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -struct oid_array *commits; -if (!S_ISGITLINK (p->two->mode)) -continue ; - commits =submodule_commits (submodules, p->two->path); -oid_array_append (commits, &p->two->oid); - } -} - -static void find_unpushed_submodule_commits (struct commit *commit, -struct string_list *needs_pushing) -{ -struct rev_info rev; - -init_revisions (&rev,NULL ); - rev.diffopt.output_format |=DIFF_FORMAT_CALLBACK ; - rev.diffopt.format_callback = collect_submodules_from_diff; - rev.diffopt.format_callback_data = needs_pushing; -diff_tree_combined_merge (commit,1 , &rev); -} - -static void free_submodules_oids (struct string_list *submodules) -{ -struct string_list_item *item; -for_each_string_list_item (item, submodules) -oid_array_clear ((struct oid_array *) item->util); -string_list_clear (submodules,1 ); -} -int find_unpushed_submodules (struct oid_array *commits,const char *remotes_name,struct string_list *needs_pushing) { -struct rev_info rev; -struct commit *commit;struct string_list submodules =STRING_LIST_INIT_DUP ;struct string_list_item *submodule;struct argv_array argv =ARGV_ARRAY_INIT ; -init_revisions (&rev,NULL ); -/* argv.argv[0] will be ignored by setup_revisions */ argv_array_push (&argv,"find_unpushed_submodules" );oid_array_for_each_unique (commits, append_oid_to_argv, &argv);argv_array_push (&argv,"--not" );argv_array_pushf (&argv,"--remotes=%s" , remotes_name); -setup_revisions (argv.argc, argv.argv, &rev,NULL ); -if (prepare_revision_walk (&rev)) -die ("revision walk setup failed" ); - -while ((commit =get_revision (&rev)) !=NULL ) -find_unpushed_submodule_commits (commit, &submodules); - -reset_revision_walk (); -argv_array_clear (&argv); +collect_changed_submodules (&submodules, &argv);for_each_string_list_item (submodule, &submodules) { -struct oid_array *commits = (struct oid_array *) submodule->util; +struct oid_array *commits = submodule->util; +const char *path = submodule->string; -if (submodule_needs_pushing (submodule->string, commits)) -string_list_insert (needs_pushing, submodule->string); +if (submodule_needs_pushing (path, commits)) +string_list_insert (needs_pushing, path); }free_submodules_oids (&submodules); +argv_array_clear (&argv);return needs_pushing->nr; }
@@ -931,61 +958,6 @@ int push_unpushed_submodules(struct oid_array *commits,return ret; } -static int is_submodule_commit_present (const char *path,unsigned char sha1[20 ]) -{ -int is_present =0 ; -if (!add_submodule_odb (path) &&lookup_commit_reference (sha1)) { -/* Even if the submodule is checked out and the commit is -* present, make sure it is reachable from a ref. */ -struct child_process cp =CHILD_PROCESS_INIT ; -const char *argv[] = {"rev-list" ,"-n" ,"1" ,NULL ,"--not" ,"--all" ,NULL }; -struct strbuf buf =STRBUF_INIT ; - - argv[3 ] =sha1_to_hex (sha1); - cp.argv = argv; -prepare_submodule_repo_env (&cp.env_array); - cp.git_cmd =1 ; - cp.no_stdin =1 ; - cp.dir = path; -if (!capture_command (&cp, &buf,1024 ) && !buf.len) - is_present =1 ; - -strbuf_release (&buf); - } -return is_present; -} - -static void submodule_collect_changed_cb (struct diff_queue_struct *q, -struct diff_options *options, -void *data) -{ -int i; -for (i =0 ; i < q->nr; i++) { -struct diff_filepair *p = q->queue[i]; -if (!S_ISGITLINK (p->two->mode)) -continue ; - -if (S_ISGITLINK (p->one->mode)) { -/* NEEDSWORK: We should honor the name configured in -* the .gitmodules file of the commit we are examining -* here to be able to correctly follow submodules -* being moved around. */ -struct string_list_item *path; - path =unsorted_string_list_lookup (&changed_submodule_paths, p->two->path); -if (!path && !is_submodule_commit_present (p->two->path, p->two->oid.hash)) -string_list_append (&changed_submodule_paths, p->two->path); - }else { -/* Submodule is new or was moved here */ -/* NEEDSWORK: When the .git directories of submodules -* live inside the superprojects .git directory some -* day we should fetch new submodules directly into -* that location too when config or options request -* that so they can be checked out from there. */ -continue ; - } - } -} -static int append_oid_to_array (const char *ref,const struct object_id *oid,int flags,void *data) {
@@ -1006,45 +978,36 @@ void check_for_new_submodule_commits(struct object_id *oid)static void calculate_changed_submodule_paths (void ) { -struct rev_info rev; -struct commit *commit;struct argv_array argv =ARGV_ARRAY_INIT ; +struct string_list changed_submodules =STRING_LIST_INIT_DUP ; +const struct string_list_item *item;/* No need to check if there are no submodules configured */ if (!submodule_from_path (NULL ,NULL ))return ; -init_revisions (&rev,NULL );argv_array_push (&argv,"--" );/* argv[0] program name */ oid_array_for_each_unique (&ref_tips_after_fetch, append_oid_to_argv, &argv);argv_array_push (&argv,"--not" );oid_array_for_each_unique (&ref_tips_before_fetch, append_oid_to_argv, &argv); -setup_revisions (argv.argc, argv.argv, &rev,NULL ); -if (prepare_revision_walk (&rev)) -die ("revision walk setup failed" );/* * Collect all submodules (whether checked out or not) for which new * commits have been recorded upstream in "changed_submodule_paths". */ -while ((commit =get_revision (&rev))) { -struct commit_list *parent = commit->parents; -while (parent) { -struct diff_options diff_opts; -diff_setup (&diff_opts); -DIFF_OPT_SET (&diff_opts,RECURSIVE ); - diff_opts.output_format |=DIFF_FORMAT_CALLBACK ; - diff_opts.format_callback = submodule_collect_changed_cb; -diff_setup_done (&diff_opts); -diff_tree_sha1 (parent->item->object.oid.hash, commit->object.oid.hash,"" , &diff_opts); -diffcore_std (&diff_opts); -diff_flush (&diff_opts); - parent = parent->next; - } +collect_changed_submodules (&changed_submodules, &argv); + +for_each_string_list_item (item, &changed_submodules) { +struct oid_array *commits = item->util; +const char *path = item->string; + +if (!submodule_has_commits (path, commits)) +string_list_append (&changed_submodule_paths, path); } +free_submodules_oids (&changed_submodules);argv_array_clear (&argv);oid_array_clear (&ref_tips_before_fetch);oid_array_clear (&ref_tips_after_fetch);
Signature · 1 line
2.13.0.rc1.294.g07d810a77f-goog