* Re: [PATCH] i18n: fix small typos
From: Eric Sunshine @ 2018-11-28 21:51 UTC (permalink / raw)
To: Jean-Noel Avila; +Cc: Git List
In-Reply-To: <20181128214309.23523-1-jn.avila@free.fr>
On Wed, Nov 28, 2018 at 4:43 PM Jean-Noël Avila <jn.avila@free.fr> wrote:
> Translating the new strings introduced for v2.20 showed some typos.
Hard to spot by eyeball when looking at the diff, but both fixes make
sense. Thanks.
> Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
^ permalink raw reply
* [PATCH 0/5] Add a new "sparse" tree walk algorithm
From: Derrick Stolee via GitGitGadget @ 2018-11-28 21:52 UTC (permalink / raw)
To: git; +Cc: peff, avarab, jrnieder, Junio C Hamano
One of the biggest remaining pain points for users of very large
repositories is the time it takes to run 'git push'. We inspected some slow
pushes by our developers and found that the "Enumerating Objects" phase of a
push was very slow. This is unsurprising, because this is why reachability
bitmaps exist. However, reachability bitmaps are not available to us because
of the single pack-file requirement. The bitmap approach is intended for
servers anyway, and clients have a much different behavior pattern.
Specifically, clients are normally pushing a very small number of objects
compared to the entire working directory. A typical user changes only a
small cone of the working directory, so let's use that to our benefit.
Create a new "sparse" mode for 'git pack-objects' that uses the paths that
introduce new objects to direct our search into the reachable trees. By
collecting trees at each path, we can then recurse into a path only when
there are uninteresting and interesting trees at that path. This gains a
significant performance boost for small topics while presenting a
possibility of packing extra objects.
The main algorithm change is in patch 4, but is set up a little bit in
patches 1 and 2.
As demonstrated in the included test script, we see that the existing
algorithm can send extra objects due to the way we specify the "frontier".
But we can send even more objects if a user copies objects from one folder
to another. I say "copy" because a rename would (usually) change the
original folder and trigger a walk into that path, discovering the objects.
In order to benefit from this approach, the user can opt-in using the
pack.useSparse config setting. This setting can be overridden using the
'--no-sparse' option.
Derrick Stolee (5):
revision: add mark_tree_uninteresting_sparse
list-objects: consume sparse tree walk
pack-objects: add --sparse option
revision: implement sparse algorithm
pack-objects: create pack.useSparse setting
Documentation/git-pack-objects.txt | 9 +-
bisect.c | 2 +-
builtin/pack-objects.c | 9 +-
builtin/rev-list.c | 2 +-
http-push.c | 2 +-
list-objects.c | 51 ++++++++++-
list-objects.h | 4 +-
revision.c | 113 +++++++++++++++++++++++
revision.h | 2 +
t/t5322-pack-objects-sparse.sh | 139 +++++++++++++++++++++++++++++
10 files changed, 323 insertions(+), 10 deletions(-)
create mode 100755 t/t5322-pack-objects-sparse.sh
base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-89%2Fderrickstolee%2Fpush%2Fsparse-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-89/derrickstolee/push/sparse-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/89
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/5] revision: add mark_tree_uninteresting_sparse
From: Derrick Stolee via GitGitGadget @ 2018-11-28 21:52 UTC (permalink / raw)
To: git; +Cc: peff, avarab, jrnieder, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.89.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
In preparation for a new algorithm that walks fewer trees when
creating a pack from a set of revisions, create a method that
takes an oidset of tree oids and marks reachable objects as
UNINTERESTING.
The current implementation uses the existing
mark_tree_uninteresting to recursively walk the trees and blobs.
This will walk the same number of trees as the old mechanism.
There is one new assumption in this approach: we are also given
the oids of the interesting trees. This implementation does not
use those trees at the moment, but we will use them in a later
rewrite of this method.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
revision.c | 22 ++++++++++++++++++++++
revision.h | 2 ++
2 files changed, 24 insertions(+)
diff --git a/revision.c b/revision.c
index 13e0519c02..3a62c7c187 100644
--- a/revision.c
+++ b/revision.c
@@ -99,6 +99,28 @@ void mark_tree_uninteresting(struct repository *r, struct tree *tree)
mark_tree_contents_uninteresting(r, tree);
}
+void mark_trees_uninteresting_sparse(struct repository *r,
+ struct oidset *set)
+{
+ struct object_id *oid;
+ struct oidset_iter iter;
+
+ oidset_iter_init(set, &iter);
+ while ((oid = oidset_iter_next(&iter))) {
+ struct tree *tree = lookup_tree(r, oid);
+
+ if (tree->object.flags & UNINTERESTING) {
+ /*
+ * Remove the flag so the next call
+ * is not a no-op. The flag is added
+ * in mark_tree_unintersting().
+ */
+ tree->object.flags ^= UNINTERESTING;
+ mark_tree_uninteresting(r, tree);
+ }
+ }
+}
+
struct commit_stack {
struct commit **items;
size_t nr, alloc;
diff --git a/revision.h b/revision.h
index 7987bfcd2e..f828e91ae9 100644
--- a/revision.h
+++ b/revision.h
@@ -67,6 +67,7 @@ struct rev_cmdline_info {
#define REVISION_WALK_NO_WALK_SORTED 1
#define REVISION_WALK_NO_WALK_UNSORTED 2
+struct oidset;
struct topo_walk_info;
struct rev_info {
@@ -327,6 +328,7 @@ void put_revision_mark(const struct rev_info *revs,
void mark_parents_uninteresting(struct commit *commit);
void mark_tree_uninteresting(struct repository *r, struct tree *tree);
+void mark_trees_uninteresting_sparse(struct repository *r, struct oidset *set);
void show_object_with_name(FILE *, struct object *, const char *);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/5] list-objects: consume sparse tree walk
From: Derrick Stolee via GitGitGadget @ 2018-11-28 21:52 UTC (permalink / raw)
To: git; +Cc: peff, avarab, jrnieder, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.89.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
When creating a pack-file using 'git pack-objects --revs' we provide
a list of interesting and uninteresting commits. For example, a push
operation would make the local topic branch be interesting and the
known remote refs as uninteresting. We want to discover the set of
new objects to send to the server as a thin pack.
We walk these commits until we discover a frontier of commits such
that every commit walk starting at interesting commits ends in a root
commit or unintersting commit. We then need to discover which
non-commit objects are reachable from uninteresting commits.
The mark_edges_unintersting() method in list-objects.c iterates on
the commit list and does the following:
* If the commit is UNINTERSTING, then mark its root tree and every
object it can reach as UNINTERESTING.
* If the commit is interesting, then mark the root tree of every
UNINTERSTING parent (and all objects that tree can reach) as
UNINTERSTING.
At the very end, we repeat the process on every commit directly
given to the revision walk from stdin. This helps ensure we properly
cover shallow commits that otherwise were not included in the
frontier.
The logic to recursively follow trees is in the
mark_tree_uninteresting() method in revision.c. The algorithm avoids
duplicate work by not recursing into trees that are already marked
UNINTERSTING.
Add a new 'sparse' option to the mark_edges_uninteresting() method
that performs this logic in a slightly new way. As we iterate over
the commits, we add all of the root trees to an oidset. Then, call
mark_trees_uninteresting_sparse() on that oidset. Note that we
include interesting trees in this process. The current implementation
of mark_trees_unintersting_sparse() will walk the same trees as
the old logic, but this will be replaced in a later change.
The sparse option is not used by any callers at the moment, but
will be wired to 'git pack-objects' in the next change.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
bisect.c | 2 +-
builtin/pack-objects.c | 2 +-
builtin/rev-list.c | 2 +-
http-push.c | 2 +-
list-objects.c | 51 ++++++++++++++++++++++++++++++++++++++----
list-objects.h | 4 +++-
6 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/bisect.c b/bisect.c
index 487675c672..842f8b4b8f 100644
--- a/bisect.c
+++ b/bisect.c
@@ -656,7 +656,7 @@ static void bisect_common(struct rev_info *revs)
if (prepare_revision_walk(revs))
die("revision walk setup failed");
if (revs->tree_objects)
- mark_edges_uninteresting(revs, NULL);
+ mark_edges_uninteresting(revs, NULL, 0);
}
static void exit_if_skipped_commits(struct commit_list *tried,
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 411aefd687..5f70d840a7 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3135,7 +3135,7 @@ static void get_object_list(int ac, const char **av)
if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
- mark_edges_uninteresting(&revs, show_edge);
+ mark_edges_uninteresting(&revs, show_edge, 0);
if (!fn_show_object)
fn_show_object = show_object;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 2880ed37e3..9663cbfae0 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -543,7 +543,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if (prepare_revision_walk(&revs))
die("revision walk setup failed");
if (revs.tree_objects)
- mark_edges_uninteresting(&revs, show_edge);
+ mark_edges_uninteresting(&revs, show_edge, 0);
if (bisect_list) {
int reaches, all;
diff --git a/http-push.c b/http-push.c
index cd48590912..ea52d6f9f6 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1933,7 +1933,7 @@ int cmd_main(int argc, const char **argv)
pushing = 0;
if (prepare_revision_walk(&revs))
die("revision walk setup failed");
- mark_edges_uninteresting(&revs, NULL);
+ mark_edges_uninteresting(&revs, NULL, 0);
objects_to_send = get_delta(&revs, ref_lock);
finish_all_active_slots();
diff --git a/list-objects.c b/list-objects.c
index c41cc80db5..9bb93d1640 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -222,25 +222,68 @@ static void mark_edge_parents_uninteresting(struct commit *commit,
}
}
-void mark_edges_uninteresting(struct rev_info *revs, show_edge_fn show_edge)
+static void add_edge_parents(struct commit *commit,
+ struct rev_info *revs,
+ show_edge_fn show_edge,
+ struct oidset *set)
+{
+ struct commit_list *parents;
+
+ for (parents = commit->parents; parents; parents = parents->next) {
+ struct commit *parent = parents->item;
+ struct tree *tree = get_commit_tree(parent);
+ oidset_insert(set, &tree->object.oid);
+
+ if (!(parent->object.flags & UNINTERESTING))
+ continue;
+ tree->object.flags |= UNINTERESTING;
+
+ if (revs->edge_hint && !(parent->object.flags & SHOWN)) {
+ parent->object.flags |= SHOWN;
+ show_edge(parent);
+ }
+ }
+}
+
+void mark_edges_uninteresting(struct rev_info *revs,
+ show_edge_fn show_edge,
+ int sparse)
{
struct commit_list *list;
+ struct oidset set;
int i;
+ if (sparse)
+ oidset_init(&set, 16);
+
for (list = revs->commits; list; list = list->next) {
struct commit *commit = list->item;
+
+ if (sparse) {
+ struct tree *tree = get_commit_tree(commit);
+
+ if (commit->object.flags & UNINTERESTING)
+ tree->object.flags |= UNINTERESTING;
- if (commit->object.flags & UNINTERESTING) {
+ oidset_insert(&set, &tree->object.oid);
+ add_edge_parents(commit, revs, show_edge, &set);
+ } else if (commit->object.flags & UNINTERESTING) {
mark_tree_uninteresting(revs->repo,
get_commit_tree(commit));
if (revs->edge_hint_aggressive && !(commit->object.flags & SHOWN)) {
commit->object.flags |= SHOWN;
show_edge(commit);
}
- continue;
+ } else {
+ mark_edge_parents_uninteresting(commit, revs, show_edge);
}
- mark_edge_parents_uninteresting(commit, revs, show_edge);
}
+
+ if (sparse) {
+ mark_trees_uninteresting_sparse(revs->repo, &set);
+ oidset_clear(&set);
+ }
+
if (revs->edge_hint_aggressive) {
for (i = 0; i < revs->cmdline.nr; i++) {
struct object *obj = revs->cmdline.rev[i].item;
diff --git a/list-objects.h b/list-objects.h
index ad40762926..a952680e46 100644
--- a/list-objects.h
+++ b/list-objects.h
@@ -10,7 +10,9 @@ typedef void (*show_object_fn)(struct object *, const char *, void *);
void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);
typedef void (*show_edge_fn)(struct commit *);
-void mark_edges_uninteresting(struct rev_info *, show_edge_fn);
+void mark_edges_uninteresting(struct rev_info *revs,
+ show_edge_fn show_edge,
+ int sparse);
struct oidset;
struct list_objects_filter_options;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/5] pack-objects: add --sparse option
From: Derrick Stolee via GitGitGadget @ 2018-11-28 21:52 UTC (permalink / raw)
To: git; +Cc: peff, avarab, jrnieder, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.89.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
Add a '--sparse' option flag to the pack-objects builtin. This
allows the user to specify that they want to use the new logic
for walking trees. This logic currently does not differ from the
existing output, but will in a later change.
Create a new test script, t5322-pack-objects-sparse.sh, to ensure
the object list that is selected matches what we expect. When we
update the logic to walk in a sparse fashion, the final test will
be updated to show the extra objects that are added.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/git-pack-objects.txt | 9 ++-
builtin/pack-objects.c | 5 +-
t/t5322-pack-objects-sparse.sh | 115 +++++++++++++++++++++++++++++
3 files changed, 127 insertions(+), 2 deletions(-)
create mode 100755 t/t5322-pack-objects-sparse.sh
diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 40c825c381..ced2630eb3 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -14,7 +14,7 @@ SYNOPSIS
[--local] [--incremental] [--window=<n>] [--depth=<n>]
[--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
[--stdout [--filter=<filter-spec>] | base-name]
- [--shallow] [--keep-true-parents] < object-list
+ [--shallow] [--keep-true-parents] [--sparse] < object-list
DESCRIPTION
@@ -196,6 +196,13 @@ depth is 4095.
Add --no-reuse-object if you want to force a uniform compression
level on all data no matter the source.
+--sparse::
+ Use the "sparse" algorithm to determine which objects to include in
+ the pack. This can have significant performance benefits when computing
+ a pack to send a small change. However, it is possible that extra
+ objects are added to the pack-file if the included commits contain
+ certain types of direct renames.
+
--thin::
Create a "thin" pack by omitting the common objects between a
sender and a receiver in order to reduce network transfer. This
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 5f70d840a7..7d5b0735e3 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -84,6 +84,7 @@ static unsigned long pack_size_limit;
static int depth = 50;
static int delta_search_threads;
static int pack_to_stdout;
+static int sparse;
static int thin;
static int num_preferred_base;
static struct progress *progress_state;
@@ -3135,7 +3136,7 @@ static void get_object_list(int ac, const char **av)
if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
- mark_edges_uninteresting(&revs, show_edge, 0);
+ mark_edges_uninteresting(&revs, show_edge, sparse);
if (!fn_show_object)
fn_show_object = show_object;
@@ -3292,6 +3293,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
{ OPTION_CALLBACK, 0, "unpack-unreachable", NULL, N_("time"),
N_("unpack unreachable objects newer than <time>"),
PARSE_OPT_OPTARG, option_parse_unpack_unreachable },
+ OPT_BOOL(0, "sparse", &sparse,
+ N_("use the sparse reachability algorithm")),
OPT_BOOL(0, "thin", &thin,
N_("create thin packs")),
OPT_BOOL(0, "shallow", &shallow,
diff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh
new file mode 100755
index 0000000000..81f6805bc3
--- /dev/null
+++ b/t/t5322-pack-objects-sparse.sh
@@ -0,0 +1,115 @@
+#!/bin/sh
+
+test_description='pack-objects object selection using sparse algorithm'
+. ./test-lib.sh
+
+test_expect_success 'setup repo' '
+ test_commit initial &&
+ for i in $(test_seq 1 3)
+ do
+ mkdir f$i &&
+ for j in $(test_seq 1 3)
+ do
+ mkdir f$i/f$j &&
+ echo $j >f$i/f$j/data.txt
+ done
+ done &&
+ git add . &&
+ git commit -m "Initialized trees" &&
+ for i in $(test_seq 1 3)
+ do
+ git checkout -b topic$i master &&
+ echo change-$i >f$i/f$i/data.txt &&
+ git commit -a -m "Changed f$i/f$i/data.txt"
+ done &&
+ cat >packinput.txt <<-EOF &&
+ topic1
+ ^topic2
+ ^topic3
+ EOF
+ git rev-parse \
+ topic1 \
+ topic1^{tree} \
+ topic1:f1 \
+ topic1:f1/f1 \
+ topic1:f1/f1/data.txt | sort >expect_objects.txt
+'
+
+test_expect_success 'non-sparse pack-objects' '
+ git pack-objects --stdout --revs <packinput.txt >nonsparse.pack &&
+ git index-pack -o nonsparse.idx nonsparse.pack &&
+ git show-index <nonsparse.idx | awk "{print \$2}" >nonsparse_objects.txt &&
+ test_cmp expect_objects.txt nonsparse_objects.txt
+'
+
+test_expect_success 'sparse pack-objects' '
+ git pack-objects --stdout --revs --sparse <packinput.txt >sparse.pack &&
+ git index-pack -o sparse.idx sparse.pack &&
+ git show-index <sparse.idx | awk "{print \$2}" >sparse_objects.txt &&
+ test_cmp expect_objects.txt sparse_objects.txt
+'
+
+# Demonstrate that both algorithms send "extra" objects because
+# they are not in the frontier.
+
+test_expect_success 'duplicate a folder from f3 and commit to topic1' '
+ git checkout topic1 &&
+ echo change-3 >f3/f3/data.txt &&
+ git commit -a -m "Changed f3/f3/data.txt" &&
+ git rev-parse \
+ topic1~1 \
+ topic1~1^{tree} \
+ topic1^{tree} \
+ topic1 \
+ topic1:f1 \
+ topic1:f1/f1 \
+ topic1:f1/f1/data.txt \
+ topic1:f3 \
+ topic1:f3/f3 \
+ topic1:f3/f3/data.txt | sort >expect_objects.txt
+'
+
+test_expect_success 'non-sparse pack-objects' '
+ git pack-objects --stdout --revs <packinput.txt >nonsparse.pack &&
+ git index-pack -o nonsparse.idx nonsparse.pack &&
+ git show-index <nonsparse.idx | awk "{print \$2}" >nonsparse_objects.txt &&
+ test_cmp expect_objects.txt nonsparse_objects.txt
+'
+
+test_expect_success 'sparse pack-objects' '
+ git pack-objects --stdout --revs --sparse <packinput.txt >sparse.pack &&
+ git index-pack -o sparse.idx sparse.pack &&
+ git show-index <sparse.idx | awk "{print \$2}" >sparse_objects.txt &&
+ test_cmp expect_objects.txt sparse_objects.txt
+'
+
+test_expect_success 'duplicate a folder from f1 into f3' '
+ mkdir f3/f4 &&
+ cp -r f1/f1/* f3/f4 &&
+ git add f3/f4 &&
+ git commit -m "Copied f1/f1 to f3/f4" &&
+ cat >packinput.txt <<-EOF &&
+ topic1
+ ^topic1~1
+ EOF
+ git rev-parse \
+ topic1 \
+ topic1^{tree} \
+ topic1:f3 | sort >expect_objects.txt
+'
+
+test_expect_success 'non-sparse pack-objects' '
+ git pack-objects --stdout --revs <packinput.txt >nonsparse.pack &&
+ git index-pack -o nonsparse.idx nonsparse.pack &&
+ git show-index <nonsparse.idx | awk "{print \$2}" >nonsparse_objects.txt &&
+ test_cmp expect_objects.txt nonsparse_objects.txt
+'
+
+test_expect_success 'sparse pack-objects' '
+ git pack-objects --stdout --revs --sparse <packinput.txt >sparse.pack &&
+ git index-pack -o sparse.idx sparse.pack &&
+ git show-index <sparse.idx | awk "{print \$2}" >sparse_objects.txt &&
+ test_cmp expect_objects.txt sparse_objects.txt
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 5/5] pack-objects: create pack.useSparse setting
From: Derrick Stolee via GitGitGadget @ 2018-11-28 21:52 UTC (permalink / raw)
To: git; +Cc: peff, avarab, jrnieder, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.89.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
The '--sparse' flag in 'git pack-objects' changes the algorithm
used to enumerate objects to one that is faster for individual
users pushing new objects that change only a small cone of the
working directory. The sparse algorithm is not recommended for a
server, which likely sends new objects that appear across the
entire working directory.
Create a 'pack.useSparse' setting that enables this new algorithm.
This allows 'git push' to use this algorithm without passing a
'--sparse' flag all the way through four levels of run_command()
calls.
If the '--no-sparse' flag is set, then this config setting is
overridden.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
builtin/pack-objects.c | 4 ++++
t/t5322-pack-objects-sparse.sh | 15 +++++++++++++++
2 files changed, 19 insertions(+)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 7d5b0735e3..124b1bafc4 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2711,6 +2711,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)
use_bitmap_index_default = git_config_bool(k, v);
return 0;
}
+ if (!strcmp(k, "pack.usesparse")) {
+ sparse = git_config_bool(k, v);
+ return 0;
+ }
if (!strcmp(k, "pack.threads")) {
delta_search_threads = git_config_int(k, v);
if (delta_search_threads < 0)
diff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh
index 45dba6e014..8f5699bd91 100755
--- a/t/t5322-pack-objects-sparse.sh
+++ b/t/t5322-pack-objects-sparse.sh
@@ -121,4 +121,19 @@ test_expect_success 'sparse pack-objects' '
test_cmp expect_sparse_objects.txt sparse_objects.txt
'
+test_expect_success 'pack.useSparse enables algorithm' '
+ git config pack.useSparse true &&
+ git pack-objects --stdout --revs <packinput.txt >sparse.pack &&
+ git index-pack -o sparse.idx sparse.pack &&
+ git show-index <sparse.idx | awk "{print \$2}" >sparse_objects.txt &&
+ test_cmp expect_sparse_objects.txt sparse_objects.txt
+'
+
+test_expect_success 'pack.useSparse overridden' '
+ git pack-objects --stdout --revs --no-sparse <packinput.txt >sparse.pack &&
+ git index-pack -o sparse.idx sparse.pack &&
+ git show-index <sparse.idx | awk "{print \$2}" >sparse_objects.txt &&
+ test_cmp expect_objects.txt sparse_objects.txt
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 4/5] revision: implement sparse algorithm
From: Derrick Stolee via GitGitGadget @ 2018-11-28 21:52 UTC (permalink / raw)
To: git; +Cc: peff, avarab, jrnieder, Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.89.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
When enumerating objects to place in a pack-file during 'git
pack-objects --revs', we discover the "frontier" of commits
that we care about and the boundary with commit we find
uninteresting. From that point, we walk trees to discover which
trees and blobs are uninteresting. Finally, we walk trees to find
the interesting trees.
This commit introduces a new, "sparse" way to discover the
uninteresting trees. We use the perspective of a single user trying
to push their topic to a large repository. That user likely changed
a very small fraction of the paths in their working directory, but
we spend a lot of time walking all reachable trees.
The way to switch the logic to work in this sparse way is to start
caring about which paths introduce new trees. While it is not
possible to generate a diff between the frontier boundary and all
of the interesting commits, we can simulate that behavior by
inspecting all of the root trees as a whole, then recursing down
to the set of trees at each path.
We already had taken the first step by passing an oidset to
mark_trees_uninteresting_sparse(). We now create a dictionary
whose keys are paths and values are oidsets. We consider the set
of trees that appear at each path. While we inspect a tree, we
add its subtrees to the oidsets corresponding to the tree entry's
path. We also mark trees as UNINTERESTING if the tree we are
parsing is UNINTERESTING.
To actually improve the peformance, we need to terminate our
recursion unless the oidset contains some intersting trees and
some uninteresting trees. Technically, we only need one interesting
tree for this to speed up in most cases, but we also will not mark
anything UNINTERESTING if there are no uninteresting trees, so
that would be wasted effort.
There are a few ways that this is not a universally better option.
First, we can pack extra objects. If someone copies a subtree
from one tree to another, the first tree will appear UNINTERESTING
and we will not recurse to see that the subtree should also be
UNINTERESTING. We will walk the new tree and see the subtree as
a "new" object and add it to the pack. We add a test case that
demonstrates this as a way to prove that the --sparse option is
actually working.
Second, we can have extra memory pressure. If instead of being a
single user pushing a small topic we are a server sending new
objects from across the entire working directory, then we will
gain very little (the recursion will rarely terminate early) but
will spend extra time maintaining the path-oidset dictionaries.
Despite these potential drawbacks, the benefits of the algorithm
are clear. By adding a counter to 'add_children_by_path' and
'mark_tree_contents_uninteresting', I measured the number of
parsed trees for the two algorithms in a variety of repos.
For git.git, I used the following input:
v2.19.0
^v2.19.0~10
Objects to pack: 550
Walked (old alg): 282
Walked (new alg): 130
For the Linux repo, I used the following input:
v4.18
^v4.18~10
Objects to pack: 518
Walked (old alg): 4,836
Walked (new alg): 188
The two repos above are rather "wide and flat" compared to
other repos that I have used in the past. As a comparison,
I tested an old topic branch in the Azure DevOps repo, which
has a much deeper folder structure than the Linux repo.
Objects to pack: 220
Walked (old alg): 22,804
Walked (new alg): 129
I used the number of walked trees the main metric above because
it is consistent across multiple runs. When I ran my tests, the
performance of the pack-objects command with the same options
could change the end-to-end time by 10x depending on the file
system being warm. However, by repeating the same test on repeat
I could get more consistent timing results. The git.git and
Linux tests were too fast overall (less than 0.5s) to measure
an end-to-end difference. The Azure DevOps case was slow enough
to see the time improve from 15s to 1s in the warm case. The
cold case was 90s to 9s in my testing.
These improvements will have even larger benefits in the super-
large Windows repository. In our experiments, we see the
"Enumerate objects" phase of pack-objects taking 60-80% of the
end-to-end time of non-trivial pushes, taking longer than the
network time to send the pack and the server time to verify the
pack.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
revision.c | 111 ++++++++++++++++++++++++++++++---
t/t5322-pack-objects-sparse.sh | 21 +++++--
2 files changed, 116 insertions(+), 16 deletions(-)
diff --git a/revision.c b/revision.c
index 3a62c7c187..7e4bfe621a 100644
--- a/revision.c
+++ b/revision.c
@@ -99,26 +99,117 @@ void mark_tree_uninteresting(struct repository *r, struct tree *tree)
mark_tree_contents_uninteresting(r, tree);
}
+struct paths_and_oids {
+ struct string_list list;
+};
+
+static void paths_and_oids_init(struct paths_and_oids *po)
+{
+ string_list_init(&po->list, 1);
+}
+
+static void paths_and_oids_clear(struct paths_and_oids *po)
+{
+ int i;
+ for (i = 0; i < po->list.nr; i++) {
+ oidset_clear(po->list.items[i].util);
+ free(po->list.items[i].util);
+ }
+
+ string_list_clear(&po->list, 0);
+}
+
+static void paths_and_oids_insert(struct paths_and_oids *po,
+ const char *path,
+ const struct object_id *oid)
+{
+ struct string_list_item *item = string_list_insert(&po->list, path);
+ struct oidset *set;
+
+ if (!item->util) {
+ set = xcalloc(1, sizeof(struct oidset));
+ oidset_init(set, 16);
+ item->util = set;
+ } else {
+ set = item->util;
+ }
+
+ oidset_insert(set, oid);
+}
+
+static void add_children_by_path(struct repository *r,
+ struct tree *tree,
+ struct paths_and_oids *po)
+{
+ struct tree_desc desc;
+ struct name_entry entry;
+
+ if (parse_tree_gently(tree, 1) < 0)
+ return;
+
+ init_tree_desc(&desc, tree->buffer, tree->size);
+ while (tree_entry(&desc, &entry)) {
+ switch (object_type(entry.mode)) {
+ case OBJ_TREE:
+ paths_and_oids_insert(po, entry.path, entry.oid);
+
+ if (tree->object.flags & UNINTERESTING) {
+ struct tree *child = lookup_tree(r, entry.oid);
+ child->object.flags |= UNINTERESTING;
+ }
+ break;
+ case OBJ_BLOB:
+ if (tree->object.flags & UNINTERESTING) {
+ struct blob *child = lookup_blob(r, entry.oid);
+ child->object.flags |= UNINTERESTING;
+ }
+ break;
+ default:
+ /* Subproject commit - not in this repository */
+ break;
+ }
+ }
+
+ free_tree_buffer(tree);
+}
+
void mark_trees_uninteresting_sparse(struct repository *r,
struct oidset *set)
{
+ int i;
+ unsigned has_interesting = 0, has_uninteresting = 0;
+ struct paths_and_oids po;
struct object_id *oid;
struct oidset_iter iter;
oidset_iter_init(set, &iter);
- while ((oid = oidset_iter_next(&iter))) {
+ while ((!has_interesting || !has_uninteresting) &&
+ (oid = oidset_iter_next(&iter))) {
struct tree *tree = lookup_tree(r, oid);
- if (tree->object.flags & UNINTERESTING) {
- /*
- * Remove the flag so the next call
- * is not a no-op. The flag is added
- * in mark_tree_unintersting().
- */
- tree->object.flags ^= UNINTERESTING;
- mark_tree_uninteresting(r, tree);
- }
+ if (tree->object.flags & UNINTERESTING)
+ has_uninteresting = 1;
+ else
+ has_interesting = 1;
}
+
+ /* Do not walk unless we have both types of trees. */
+ if (!has_uninteresting || !has_interesting)
+ return;
+
+ paths_and_oids_init(&po);
+
+ oidset_iter_init(set, &iter);
+ while ((oid = oidset_iter_next(&iter))) {
+ struct tree *tree = lookup_tree(r, oid);
+ add_children_by_path(r, tree, &po);
+ }
+
+ for (i = 0; i < po.list.nr; i++)
+ mark_trees_uninteresting_sparse(
+ r, (struct oidset *)po.list.items[i].util);
+
+ paths_and_oids_clear(&po);
}
struct commit_stack {
diff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh
index 81f6805bc3..45dba6e014 100755
--- a/t/t5322-pack-objects-sparse.sh
+++ b/t/t5322-pack-objects-sparse.sh
@@ -83,22 +83,25 @@ test_expect_success 'sparse pack-objects' '
test_cmp expect_objects.txt sparse_objects.txt
'
+# Demonstrate that the algorithms differ when we copy a tree wholesale
+# from one folder to another.
+
test_expect_success 'duplicate a folder from f1 into f3' '
mkdir f3/f4 &&
cp -r f1/f1/* f3/f4 &&
git add f3/f4 &&
git commit -m "Copied f1/f1 to f3/f4" &&
- cat >packinput.txt <<-EOF &&
+ cat >packinput.txt <<-EOF
topic1
^topic1~1
EOF
- git rev-parse \
- topic1 \
- topic1^{tree} \
- topic1:f3 | sort >expect_objects.txt
'
test_expect_success 'non-sparse pack-objects' '
+ git rev-parse \
+ topic1 \
+ topic1^{tree} \
+ topic1:f3 | sort >expect_objects.txt &&
git pack-objects --stdout --revs <packinput.txt >nonsparse.pack &&
git index-pack -o nonsparse.idx nonsparse.pack &&
git show-index <nonsparse.idx | awk "{print \$2}" >nonsparse_objects.txt &&
@@ -106,10 +109,16 @@ test_expect_success 'non-sparse pack-objects' '
'
test_expect_success 'sparse pack-objects' '
+ git rev-parse \
+ topic1 \
+ topic1^{tree} \
+ topic1:f3 \
+ topic1:f3/f4 \
+ topic1:f3/f4/data.txt | sort >expect_sparse_objects.txt &&
git pack-objects --stdout --revs --sparse <packinput.txt >sparse.pack &&
git index-pack -o sparse.idx sparse.pack &&
git show-index <sparse.idx | awk "{print \$2}" >sparse_objects.txt &&
- test_cmp expect_objects.txt sparse_objects.txt
+ test_cmp expect_sparse_objects.txt sparse_objects.txt
'
test_done
--
gitgitgadget
^ permalink raw reply related
* Re: [RFC PATCH] Introduce "precious" file concept
From: Ævar Arnfjörð Bjarmason @ 2018-11-28 21:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Per Lundberg, brian m. carlson, git@vger.kernel.org, Steffen Jost,
Joshua Jensen, Matthieu Moy, Clemens Buchacher, Holger Hellmuth,
Kevin Ballard, Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqa7ltua2s.fsf@gitster-ct.c.googlers.com>
On Wed, Nov 28 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> What do you think about some patch like that which retains the plumbing
>> behavior for things like read-tree, doesn't introduce "precious" or
>> "trashable", and just makes you specify "[checkout|merge|...] --force"
>> in cases where we'd have clobbering?
>
> Whether you like it or not, don't people's automation use tons of
> invocations of "git merge", "git checkout", etc.? You'd be breaking
> them by such a change.
I'm so sympathetic to this argument that I tried to convince you of
something like this around a year and a half ago:
https://public-inbox.org/git/CACBZZX59KXPOEjiUKtZLN6zjO_xpiWve7Xga6q-53J2LwvfZyw@mail.gmail.com/
:)
I was probing for what your current stance on this sort of thing is,
because discussions like this tend to get bogged down in the irrelevant
distraction of whether something is plumbing or porcelain, which almost
none of our users care about, and we've effectively stopped caring about
ourselves.
But we must have some viable way to repair warts in the tools, and
losing user data is a *big* wart.
I don't think something like the endgame you've described in
https://public-inbox.org/git/xmqqzhtwuhpc.fsf@gitster-ct.c.googlers.com/
is ever going to work. Novice git users (the vast majority) are not
going to diligently update both .gitignore and some .gitattribute
mechanism in lockstep. I'd bet most git users haven't read more than a
few paragraphs of our entire documentation at best.
So what's the way forward? I think ultimately we must move to something
where we effectively version the entire CLI UI similar to stable API
versions. I.e. for things like this that would break some things (or
Duy's new "split checkout") introduce them as flags first, then bundle
up all such flags and cut a major release "Git 3, 4, ...", and
eventually remove old functionality.
> Other than that, if we never had Git before and do not have to worry
> about existing users, I'd think it would be a lot closer to the ideal
> than today's system if "checkout <tree> foo.o" rejected overwriting
> "foo.o" that is not tracked in the current index but matches an ignore
> pattern, and required a "--force" option to overwrite it.
>
> A user, during a conflict resolution, may say "I want this 'git
> checkout foo/' to ignore conflicted paths in that directory, so I
> would give "--force" option to it, but now "--force" also implies
> that I am willing to clobber ignored paths, which means I cannot use
> it".
>
> I would think that a proper automation needs per-path hint from the
> user and/or the project, not just a single-size-fits-all --force
> option, and "unlike all the *.o ignored files that are expendable,
> this vendor-supplied-object.o is not" is one way to give such a
> per-path hint.
>
>> This would give scripts which relied on our stable plumbing consistent
>> behavior, while helping users who're using our main porcelain not to
>> lose data. I could then add a --force option to the likes of read-tree
>> (on by default), so you could get porcelain-like behavior with
>> --no-force.
>
> At that low level, I suspect that a single size fits all "--force"
> would work even less well.
Yeah I don't think the one-size-fits-all way out of this is a single
--force flag.
^ permalink raw reply
* Re: [PATCH 3/5] pack-objects: add --sparse option
From: Stefan Beller @ 2018-11-28 22:11 UTC (permalink / raw)
To: gitgitgadget
Cc: git, Jeff King, Ævar Arnfjörð Bjarmason,
Jonathan Nieder, Junio C Hamano, Derrick Stolee
In-Reply-To: <9d6b8f6d0602e85652b2a748c58eeed4cbf4359e.1543441960.git.gitgitgadget@gmail.com>
> +--sparse::
> + Use the "sparse" algorithm to determine which objects to include in
> + the pack. This can have significant performance benefits when computing
> + a pack to send a small change. However, it is possible that extra
> + objects are added to the pack-file if the included commits contain
> + certain types of direct renames.
As a user, where do I find a discussion of these walking algorithms?
(i.e. how can I tell if I can really expect to gain performance benefits,
what are the tradeoffs? "If it were strictly better than the original,
it would be on by default" might be a thought of a user)
^ permalink raw reply
* Re: [PATCH 0/5] Add a new "sparse" tree walk algorithm
From: Ævar Arnfjörð Bjarmason @ 2018-11-28 22:18 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget; +Cc: git, peff, jrnieder, Junio C Hamano
In-Reply-To: <pull.89.git.gitgitgadget@gmail.com>
On Wed, Nov 28 2018, Derrick Stolee via GitGitGadget wrote:
> One of the biggest remaining pain points for users of very large
> repositories is the time it takes to run 'git push'. We inspected some slow
> pushes by our developers and found that the "Enumerating Objects" phase of a
> push was very slow. This is unsurprising, because this is why reachability
> bitmaps exist. However, reachability bitmaps are not available to us because
> of the single pack-file requirement. The bitmap approach is intended for
> servers anyway, and clients have a much different behavior pattern.
>
> Specifically, clients are normally pushing a very small number of objects
> compared to the entire working directory. A typical user changes only a
> small cone of the working directory, so let's use that to our benefit.
>
> Create a new "sparse" mode for 'git pack-objects' that uses the paths that
> introduce new objects to direct our search into the reachable trees. By
> collecting trees at each path, we can then recurse into a path only when
> there are uninteresting and interesting trees at that path. This gains a
> significant performance boost for small topics while presenting a
> possibility of packing extra objects.
>
> The main algorithm change is in patch 4, but is set up a little bit in
> patches 1 and 2.
>
> As demonstrated in the included test script, we see that the existing
> algorithm can send extra objects due to the way we specify the "frontier".
> But we can send even more objects if a user copies objects from one folder
> to another. I say "copy" because a rename would (usually) change the
> original folder and trigger a walk into that path, discovering the objects.
>
> In order to benefit from this approach, the user can opt-in using the
> pack.useSparse config setting. This setting can be overridden using the
> '--no-sparse' option.
This is really interesting. I tested this with:
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 124b1bafc4..5c7615f06c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3143 +3143 @@ static void get_object_list(int ac, const char **av)
- mark_edges_uninteresting(&revs, show_edge, sparse);
+ mark_edges_uninteresting(&revs, show_edge, 1);
To emulate having a GIT_TEST_* mode for this, which seems like a good
idea since it turned up a lot of segfaults in pack-objects. I wasn't
able to get a backtrace for that since it always happens indirectly, and
I didn't dig enough to see how to manually invoke it the right way.
^ permalink raw reply
* Re: [PATCH/RFC v2 0/7] Introduce new commands switch-branch and checkout-files
From: Stefan Xenos @ 2018-11-28 22:53 UTC (permalink / raw)
To: pclouds
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Stefan Beller, t.gummerer
In-Reply-To: <CAPL8Ziuj7Ffmdvz6NZWSJ+vzAtxFQhO1cfY2wmXm16J_8sY5fw@mail.gmail.com>
I think users have problems with detached heads for several reasons:
1. Users often enter the detached head state unexpectedly (for
example, by mistyping a "checkout" command or not understanding its
multipurpose nature, or as a side-effect of running a submodule
command). The change described here will go in the right direction
towards addressing this, since you won't be able to accidentally
detach your head by mistyping checkout. If detaching was always an
intentional action by the user, it would be much less intimidating.
2. Git shows a lot of scary error messages when running in a detached
head state. They tell you you're doing something dangerous, which
dissuades users from experimenting with the feature. IMO, it would be
better to replace the scary warning messages with instructions on
where to get more information about detached heads (and those
instructions should frontload info on how to reattach to a branch and
how to recover commits from the reflog).
3. When in a detached head state, a lot of commands add extra
confirmation prompts, which are unnecessary and intimidating.
Basically, the current user interface implies to the user that a
detached head is an error condition they'll need to recover from
rather than the normal and useful mode of working that it is.
(I'm resending this email without html - sorry if you received it twice).
So - IMO - detaching should always be an explicit action. Some options
that occur to me:
git switch-branch --detach
git detach
git switch-branch HEAD
- Stefan
On Wed, Nov 28, 2018 at 2:48 PM Stefan Xenos <sxenos@google.com> wrote:
>
> I think users have problems with detached heads for several reasons:
>
> 1. Users often enter the detached head state unexpectedly (for example, by mistyping a "checkout" command or not understanding its multipurpose nature, or as a side-effect of running a submodule command). The change described here will go in the right direction towards addressing this, since you won't be able to accidentally detach your head by mistyping checkout. If detaching was always an intentional action by the user, it would be much less intimidating.
>
> 2. Git shows a lot of scary error messages when running in a detached head state. They tell you you're doing something dangerous, which dissuades users from experimenting with the feature. IMO, it would be better to replace the scary warning messages with instructions on where to get more information about detached heads (and those instructions should frontload info on how to reattach to a branch and how to recover commits from the reflog).
>
> 3. When in a detached head state, a lot of commands add extra confirmation prompts, which are unnecessary and intimidating.
>
> Basically, the current user interface implies to the user that a detached head is an error condition they'll need to recover from rather than the normal and useful mode of working that it is.
>
> - Stefan
>
> On Wed, Nov 28, 2018 at 12:01 PM Duy Nguyen <pclouds@gmail.com> wrote:
>>
>> On Tue, Nov 27, 2018 at 5:53 PM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> >
>> > v2 is just a bit better to look at than v1. This is by no means final.
>> > If you think the command name is bad, the default behavior should
>> > change, or something else, speak up. It's still very "RFC".
>> >
>> > v2 breaks down the giant patch in v1 and starts adding some changes in
>> > these new commands:
>> >
>> > - restore-paths is renamed to checkout-paths. I wrote I didn't like
>> > "checkout" because of completion conflict. But who am I kidding,
>> > I'll use aliases anyway. "-files" instead of "-paths" because we
>> > already have ls-files.
>> > - both commands will not accept no arguments. There is no "git
>> > checkout" equivalent.
>> > - ambiguation rules are now aware that "switch-branch" for example
>> > can't take pathspec...
>>
>> Another thing. Since we start with a clean slate, should we do
>> something about detached HEAD in this switch-branch command (or
>> whatever its name will be)?
>>
>> This is usually a confusing concept to new users and I've seen some
>> users have a hard time getting out of it. I'm too comfortable with
>> detached HEAD to see this from new user's perspective and a better way
>> to deal with it.
>> --
>> Duy
^ permalink raw reply
* Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files
From: Stefan Xenos @ 2018-11-28 23:22 UTC (permalink / raw)
To: pclouds
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Stefan Beller, t.gummerer
In-Reply-To: <CACsJy8Bzs=FYKrR6h1cqVH32eEt2t8rUMtE2yFNvt+W55u=sDA@mail.gmail.com>
> Since the other one is already "checkout-files", maybe this one could just be "checkout-branch".
I rather like switch-branch and dislike the word "checkout" since it
has been overloaded in git for so long (does it mean moving HEAD or
copying files to my working tree?)
> nobody will become "sick of" the single "checkout" command that can
I have to admit I'm already sick of the checkout command. :-p I can
see myself using these two new commands 100% of the time and never
missing the old one.
Some behaviors I'd expect to see from these commands (I haven't yet
checked to see if you've already done this):
git checkout-files <tree-ish>
should reset all the files in the repository regardless of the current
directory - it should produce the same effect as "git reset --hard
<tree-ish> && git reset HEAD@{1}". It should also delete
locally-created files that aren't present in <tree-ish>, such that the
final working tree is exactly identical to what was committed in that
tree-ish.
git checkout-files foo -- myfile.txt
should delete myfile.txt if it is present locally but not present in foo.
git checkout-files foo -- .
should recursively checkout all files in the current folder and all
subfolders, and delete any locally-created files if they're not
present in foo.
git checkout-files should never move HEAD in any circumstance.
Suggestion:
If git checkout-files overwrites or deletes any locally-modified files
from the workspace or index, those files could be auto-stashed. That
would make it easy to restore them in the event of a mistyped command.
Auto-stashing could be suppressed with a command-line argument (with
alternate behaviors being fail-if-modified or always-overwrite).
Idea:
If git checkout-files modifies the submodules file, it could also
auto-update the submodules. (For example, with something like "git
submodule update --init --recursive --progress").
- Stefan
On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> >
> > > The good old "git checkout" command is still here and will be until
> > > all (or most of users) are sick of it.
> >
> > Two comments on the goal (the implementation looked reasonable
> > assuming the reader agrees with the gaol).
> >
> > At least to me, the verb "switch" needs two things to switch
> > between, i.e. "switch A and B", unless it is "switch to X".
> > Either "switch-to-branch" or simply "switch-to", perhaps?
> >
> > As I already hinted in my response to Stefan (?) about
> > checkout-from-tree vs checkout-from-index, a command with multiple
> > modes of operation is not confusing to people with the right mental
> > model, and I suspect that having two separate commands for "checking
> > out a branch" and "checking out paths" that is done by this step
> > would help users to form the right mental model.
>
> Since the other one is already "checkout-files", maybe this one could
> just be "checkout-branch".
>
> > So I tend to think
> > these two are "training wheels", and suspect that once they got it,
> > nobody will become "sick of" the single "checkout" command that can
> > be used to do either. It's just the matter of being aware what can
> > be done (which requires the right mental model) and how to tell Git
> > what the user wants it do (two separate commands, operating mode
> > option, or just the implied command line syntax---once the user
> > knows what s/he is doing, these do not make that much a difference).
>
> I would hope this becomes better defaults and being used 90% of time.
> Even though I know "git checkout" quite well, it still bites me from
> time to time. Having the right mental model is one thing. Having to
> think a bit every time to write "git checkout" with the right syntax,
> and whether you need "--" (that ambiguation problem can still bite you
> from time to time), is frankly something I'd rather avoid.
> --
> Duy
^ permalink raw reply
* Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files
From: Stefan Xenos @ 2018-11-28 23:26 UTC (permalink / raw)
To: pclouds
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Stefan Beller, t.gummerer
In-Reply-To: <CAPL8ZiuaEW5tp8ZMOZtZcb5oi3L-pDF6ajcA7b5wnH3=7Ls7Tg@mail.gmail.com>
Although I have no problem with "switch-branch" as a command name,
some alternative names we might consider for switch-branch might be:
chbranch
swbranch
switch
branch change (as a subcommand for the "branch" command)
I've personally been using "chbranch" as an alias for this
functionality for some time.
- Stefan
On Wed, Nov 28, 2018 at 3:22 PM Stefan Xenos <sxenos@google.com> wrote:
>
> > Since the other one is already "checkout-files", maybe this one could just be "checkout-branch".
>
> I rather like switch-branch and dislike the word "checkout" since it
> has been overloaded in git for so long (does it mean moving HEAD or
> copying files to my working tree?)
>
> > nobody will become "sick of" the single "checkout" command that can
>
> I have to admit I'm already sick of the checkout command. :-p I can
> see myself using these two new commands 100% of the time and never
> missing the old one.
>
> Some behaviors I'd expect to see from these commands (I haven't yet
> checked to see if you've already done this):
>
> git checkout-files <tree-ish>
> should reset all the files in the repository regardless of the current
> directory - it should produce the same effect as "git reset --hard
> <tree-ish> && git reset HEAD@{1}". It should also delete
> locally-created files that aren't present in <tree-ish>, such that the
> final working tree is exactly identical to what was committed in that
> tree-ish.
>
> git checkout-files foo -- myfile.txt
> should delete myfile.txt if it is present locally but not present in foo.
>
> git checkout-files foo -- .
> should recursively checkout all files in the current folder and all
> subfolders, and delete any locally-created files if they're not
> present in foo.
>
> git checkout-files should never move HEAD in any circumstance.
>
> Suggestion:
> If git checkout-files overwrites or deletes any locally-modified files
> from the workspace or index, those files could be auto-stashed. That
> would make it easy to restore them in the event of a mistyped command.
> Auto-stashing could be suppressed with a command-line argument (with
> alternate behaviors being fail-if-modified or always-overwrite).
>
> Idea:
> If git checkout-files modifies the submodules file, it could also
> auto-update the submodules. (For example, with something like "git
> submodule update --init --recursive --progress").
>
> - Stefan
> On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> > >
> > > > The good old "git checkout" command is still here and will be until
> > > > all (or most of users) are sick of it.
> > >
> > > Two comments on the goal (the implementation looked reasonable
> > > assuming the reader agrees with the gaol).
> > >
> > > At least to me, the verb "switch" needs two things to switch
> > > between, i.e. "switch A and B", unless it is "switch to X".
> > > Either "switch-to-branch" or simply "switch-to", perhaps?
> > >
> > > As I already hinted in my response to Stefan (?) about
> > > checkout-from-tree vs checkout-from-index, a command with multiple
> > > modes of operation is not confusing to people with the right mental
> > > model, and I suspect that having two separate commands for "checking
> > > out a branch" and "checking out paths" that is done by this step
> > > would help users to form the right mental model.
> >
> > Since the other one is already "checkout-files", maybe this one could
> > just be "checkout-branch".
> >
> > > So I tend to think
> > > these two are "training wheels", and suspect that once they got it,
> > > nobody will become "sick of" the single "checkout" command that can
> > > be used to do either. It's just the matter of being aware what can
> > > be done (which requires the right mental model) and how to tell Git
> > > what the user wants it do (two separate commands, operating mode
> > > option, or just the implied command line syntax---once the user
> > > knows what s/he is doing, these do not make that much a difference).
> >
> > I would hope this becomes better defaults and being used 90% of time.
> > Even though I know "git checkout" quite well, it still bites me from
> > time to time. Having the right mental model is one thing. Having to
> > think a bit every time to write "git checkout" with the right syntax,
> > and whether you need "--" (that ambiguation problem can still bite you
> > from time to time), is frankly something I'd rather avoid.
> > --
> > Duy
^ permalink raw reply
* Re: [PATCH v2 6/7] checkout: split into switch-branch and checkout-files
From: Stefan Xenos @ 2018-11-28 23:37 UTC (permalink / raw)
To: pclouds
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, git,
Stefan Beller, t.gummerer
In-Reply-To: <CAPL8ZivJ+=Y=8pxvs3sJrdxVtkn9xfTA63GeHcr=J0Y2JscOMQ@mail.gmail.com>
More thoughts:
git switch-branch should never detach HEAD unless asked to do so
explicitly. That also means that "git switch-branch" shouldn't accept
any of the non-branch tree-ish arguments that would have caused "git
checkout" to do so.
On Wed, Nov 28, 2018 at 3:26 PM Stefan Xenos <sxenos@google.com> wrote:
>
> Although I have no problem with "switch-branch" as a command name,
> some alternative names we might consider for switch-branch might be:
>
> chbranch
> swbranch
> switch
> branch change (as a subcommand for the "branch" command)
>
> I've personally been using "chbranch" as an alias for this
> functionality for some time.
>
> - Stefan
> On Wed, Nov 28, 2018 at 3:22 PM Stefan Xenos <sxenos@google.com> wrote:
> >
> > > Since the other one is already "checkout-files", maybe this one could just be "checkout-branch".
> >
> > I rather like switch-branch and dislike the word "checkout" since it
> > has been overloaded in git for so long (does it mean moving HEAD or
> > copying files to my working tree?)
> >
> > > nobody will become "sick of" the single "checkout" command that can
> >
> > I have to admit I'm already sick of the checkout command. :-p I can
> > see myself using these two new commands 100% of the time and never
> > missing the old one.
> >
> > Some behaviors I'd expect to see from these commands (I haven't yet
> > checked to see if you've already done this):
> >
> > git checkout-files <tree-ish>
> > should reset all the files in the repository regardless of the current
> > directory - it should produce the same effect as "git reset --hard
> > <tree-ish> && git reset HEAD@{1}". It should also delete
> > locally-created files that aren't present in <tree-ish>, such that the
> > final working tree is exactly identical to what was committed in that
> > tree-ish.
> >
> > git checkout-files foo -- myfile.txt
> > should delete myfile.txt if it is present locally but not present in foo.
> >
> > git checkout-files foo -- .
> > should recursively checkout all files in the current folder and all
> > subfolders, and delete any locally-created files if they're not
> > present in foo.
> >
> > git checkout-files should never move HEAD in any circumstance.
> >
> > Suggestion:
> > If git checkout-files overwrites or deletes any locally-modified files
> > from the workspace or index, those files could be auto-stashed. That
> > would make it easy to restore them in the event of a mistyped command.
> > Auto-stashing could be suppressed with a command-line argument (with
> > alternate behaviors being fail-if-modified or always-overwrite).
> >
> > Idea:
> > If git checkout-files modifies the submodules file, it could also
> > auto-update the submodules. (For example, with something like "git
> > submodule update --init --recursive --progress").
> >
> > - Stefan
> > On Wed, Nov 28, 2018 at 7:31 AM Duy Nguyen <pclouds@gmail.com> wrote:
> > >
> > > On Wed, Nov 28, 2018 at 7:03 AM Junio C Hamano <gitster@pobox.com> wrote:
> > > >
> > > > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> > > >
> > > > > The good old "git checkout" command is still here and will be until
> > > > > all (or most of users) are sick of it.
> > > >
> > > > Two comments on the goal (the implementation looked reasonable
> > > > assuming the reader agrees with the gaol).
> > > >
> > > > At least to me, the verb "switch" needs two things to switch
> > > > between, i.e. "switch A and B", unless it is "switch to X".
> > > > Either "switch-to-branch" or simply "switch-to", perhaps?
> > > >
> > > > As I already hinted in my response to Stefan (?) about
> > > > checkout-from-tree vs checkout-from-index, a command with multiple
> > > > modes of operation is not confusing to people with the right mental
> > > > model, and I suspect that having two separate commands for "checking
> > > > out a branch" and "checking out paths" that is done by this step
> > > > would help users to form the right mental model.
> > >
> > > Since the other one is already "checkout-files", maybe this one could
> > > just be "checkout-branch".
> > >
> > > > So I tend to think
> > > > these two are "training wheels", and suspect that once they got it,
> > > > nobody will become "sick of" the single "checkout" command that can
> > > > be used to do either. It's just the matter of being aware what can
> > > > be done (which requires the right mental model) and how to tell Git
> > > > what the user wants it do (two separate commands, operating mode
> > > > option, or just the implied command line syntax---once the user
> > > > knows what s/he is doing, these do not make that much a difference).
> > >
> > > I would hope this becomes better defaults and being used 90% of time.
> > > Even though I know "git checkout" quite well, it still bites me from
> > > time to time. Having the right mental model is one thing. Having to
> > > think a bit every time to write "git checkout" with the right syntax,
> > > and whether you need "--" (that ambiguation problem can still bite you
> > > from time to time), is frankly something I'd rather avoid.
> > > --
> > > Duy
^ permalink raw reply
* [PATCHv2 0/9] Resending sb/submodule-recursive-fetch-gets-the-tip
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller
This is a resend of sb/submodule-recursive-fetch-gets-the-tip,
with all feedback addressed. As it took some time, I'll send it
without range-diff, but would ask for full review.
I plan on resending after the next release as this got delayed quite a bit,
which is why I also rebased it to master.
Thanks,
Stefan
Previous round:
https://public-inbox.org/git/20181016181327.107186-1-sbeller@google.com/
Stefan Beller (9):
sha1-array: provide oid_array_filter
submodule.c: fix indentation
submodule.c: sort changed_submodule_names before searching it
submodule.c: tighten scope of changed_submodule_names struct
submodule: store OIDs in changed_submodule_names
repository: repo_submodule_init to take a submodule struct
submodule: migrate get_next_submodule to use repository structs
submodule.c: fetch in submodules git directory instead of in worktree
fetch: try fetching submodules if needed objects were not fetched
Documentation/technical/api-oid-array.txt | 5 +
builtin/fetch.c | 11 +-
builtin/grep.c | 17 +-
builtin/ls-files.c | 12 +-
builtin/submodule--helper.c | 2 +-
repository.c | 27 +-
repository.h | 12 +-
sha1-array.c | 17 ++
sha1-array.h | 3 +
submodule.c | 284 ++++++++++++++++---
t/helper/test-submodule-nested-repo-config.c | 8 +-
t/t5526-fetch-submodules.sh | 86 ++++++
12 files changed, 395 insertions(+), 89 deletions(-)
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply
* [PATCH 1/9] sha1-array: provide oid_array_filter
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller, Junio C Hamano
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/technical/api-oid-array.txt | 5 +++++
sha1-array.c | 17 +++++++++++++++++
sha1-array.h | 3 +++
3 files changed, 25 insertions(+)
diff --git a/Documentation/technical/api-oid-array.txt b/Documentation/technical/api-oid-array.txt
index 9febfb1d52..c97428c2c3 100644
--- a/Documentation/technical/api-oid-array.txt
+++ b/Documentation/technical/api-oid-array.txt
@@ -48,6 +48,11 @@ Functions
is not sorted, this function has the side effect of sorting
it.
+`oid_array_filter`::
+ Apply the callback function `want` to each entry in the array,
+ retaining only the entries for which the function returns true.
+ Preserve the order of the entries that are retained.
+
Examples
--------
diff --git a/sha1-array.c b/sha1-array.c
index b94e0ec0f5..d922e94e3f 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,
}
return 0;
}
+
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cb_data)
+{
+ unsigned nr = array->nr, src, dst;
+ struct object_id *oids = array->oid;
+
+ for (src = dst = 0; src < nr; src++) {
+ if (want(&oids[src], cb_data)) {
+ if (src != dst)
+ oidcpy(&oids[dst], &oids[src]);
+ dst++;
+ }
+ }
+ array->nr = dst;
+}
diff --git a/sha1-array.h b/sha1-array.h
index 232bf95017..55d016c4bf 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,
int oid_array_for_each_unique(struct oid_array *array,
for_each_oid_fn fn,
void *data);
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cbdata);
#endif /* SHA1_ARRAY_H */
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* [PATCH 2/9] submodule.c: fix indentation
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller, Junio C Hamano
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
The submodule subsystem is really bad at staying within 80 characters.
Fix it while we are here.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
submodule.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/submodule.c b/submodule.c
index 6415cc5580..bc48ea3b68 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1271,7 +1271,8 @@ static int get_next_submodule(struct child_process *cp,
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
- default_submodule.path = default_submodule.name = name;
+ default_submodule.path = name;
+ default_submodule.name = name;
submodule = &default_submodule;
}
}
@@ -1281,8 +1282,10 @@ static int get_next_submodule(struct child_process *cp,
default:
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
- if (!submodule || !unsorted_string_list_lookup(&changed_submodule_names,
- submodule->name))
+ if (!submodule ||
+ !unsorted_string_list_lookup(
+ &changed_submodule_names,
+ submodule->name))
continue;
default_argv = "on-demand";
break;
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* [PATCH 3/9] submodule.c: sort changed_submodule_names before searching it
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller, Junio C Hamano
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
We can string_list_insert() to maintain sorted-ness of the
list as we find new items, or we can string_list_append() to
build an unsorted list and sort it at the end just once.
As we do not rely on the sortedness while building the
list, we pick the "append and sort at the end" as it
has better worst case execution times.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
submodule.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/submodule.c b/submodule.c
index bc48ea3b68..3c388f85cc 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1283,7 +1283,7 @@ static int get_next_submodule(struct child_process *cp,
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
if (!submodule ||
- !unsorted_string_list_lookup(
+ !string_list_lookup(
&changed_submodule_names,
submodule->name))
continue;
@@ -1377,6 +1377,7 @@ int fetch_populated_submodules(struct repository *r,
/* default value, "--submodule-prefix" and its value are added later */
calculate_changed_submodule_paths(r);
+ string_list_sort(&changed_submodule_names);
run_processes_parallel(max_parallel_jobs,
get_next_submodule,
fetch_start_failure,
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* [PATCH 4/9] submodule.c: tighten scope of changed_submodule_names struct
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
The `changed_submodule_names` are only used for fetching, so let's make it
part of the struct that is passed around for fetching submodules.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/submodule.c b/submodule.c
index 3c388f85cc..f93f0aff82 100644
--- a/submodule.c
+++ b/submodule.c
@@ -25,7 +25,6 @@
#include "commit-reach.h"
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = 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;
@@ -1136,7 +1135,8 @@ void check_for_new_submodule_commits(struct object_id *oid)
oid_array_append(&ref_tips_after_fetch, oid);
}
-static void calculate_changed_submodule_paths(struct repository *r)
+static void calculate_changed_submodule_paths(struct repository *r,
+ struct string_list *changed_submodule_names)
{
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -1174,7 +1174,8 @@ static void calculate_changed_submodule_paths(struct repository *r)
continue;
if (!submodule_has_commits(r, path, commits))
- string_list_append(&changed_submodule_names, name->string);
+ string_list_append(changed_submodule_names,
+ name->string);
}
free_submodules_oids(&changed_submodules);
@@ -1221,8 +1222,10 @@ struct submodule_parallel_fetch {
int default_option;
int quiet;
int result;
+
+ struct string_list changed_submodule_names;
};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }
static int get_fetch_recurse_config(const struct submodule *submodule,
struct submodule_parallel_fetch *spf)
@@ -1284,7 +1287,7 @@ static int get_next_submodule(struct child_process *cp,
case RECURSE_SUBMODULES_ON_DEMAND:
if (!submodule ||
!string_list_lookup(
- &changed_submodule_names,
+ &spf->changed_submodule_names,
submodule->name))
continue;
default_argv = "on-demand";
@@ -1376,8 +1379,8 @@ int fetch_populated_submodules(struct repository *r,
argv_array_push(&spf.args, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
- calculate_changed_submodule_paths(r);
- string_list_sort(&changed_submodule_names);
+ calculate_changed_submodule_paths(r, &spf.changed_submodule_names);
+ string_list_sort(&spf.changed_submodule_names);
run_processes_parallel(max_parallel_jobs,
get_next_submodule,
fetch_start_failure,
@@ -1386,7 +1389,7 @@ int fetch_populated_submodules(struct repository *r,
argv_array_clear(&spf.args);
out:
- string_list_clear(&changed_submodule_names, 1);
+ string_list_clear(&spf.changed_submodule_names, 1);
return spf.result;
}
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* [PATCH 5/9] submodule: store OIDs in changed_submodule_names
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
'calculate_changed_submodule_paths' uses a local list to compute the
changed submodules, and then produces the result by copying appropriate
items into the result list.
Instead use the result list directly and prune items afterwards
using string_list_remove_empty_items.
By doing so we'll have access to the util pointer for longer that
contains the commits that we need to fetch, which will be
useful in a later patch.
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
---
submodule.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/submodule.c b/submodule.c
index f93f0aff82..0c81aca6f2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1139,8 +1139,7 @@ static void calculate_changed_submodule_paths(struct repository *r,
struct string_list *changed_submodule_names)
{
struct argv_array argv = ARGV_ARRAY_INIT;
- struct string_list changed_submodules = STRING_LIST_INIT_DUP;
- const struct string_list_item *name;
+ struct string_list_item *name;
/* No need to check if there are no submodules configured */
if (!submodule_from_path(r, NULL, NULL))
@@ -1157,9 +1156,9 @@ static void calculate_changed_submodule_paths(struct repository *r,
* Collect all submodules (whether checked out or not) for which new
* commits have been recorded upstream in "changed_submodule_names".
*/
- collect_changed_submodules(r, &changed_submodules, &argv);
+ collect_changed_submodules(r, changed_submodule_names, &argv);
- for_each_string_list_item(name, &changed_submodules) {
+ for_each_string_list_item(name, changed_submodule_names) {
struct oid_array *commits = name->util;
const struct submodule *submodule;
const char *path = NULL;
@@ -1173,12 +1172,14 @@ static void calculate_changed_submodule_paths(struct repository *r,
if (!path)
continue;
- if (!submodule_has_commits(r, path, commits))
- string_list_append(changed_submodule_names,
- name->string);
+ if (submodule_has_commits(r, path, commits)) {
+ oid_array_clear(commits);
+ *name->string = '\0';
+ }
}
- free_submodules_oids(&changed_submodules);
+ string_list_remove_empty_items(changed_submodule_names, 1);
+
argv_array_clear(&argv);
oid_array_clear(&ref_tips_before_fetch);
oid_array_clear(&ref_tips_after_fetch);
@@ -1389,7 +1390,7 @@ int fetch_populated_submodules(struct repository *r,
argv_array_clear(&spf.args);
out:
- string_list_clear(&spf.changed_submodule_names, 1);
+ free_submodules_oids(&spf.changed_submodule_names);
return spf.result;
}
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* [PATCH 6/9] repository: repo_submodule_init to take a submodule struct
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
When constructing a struct repository for a submodule for some revision
of the superproject where the submodule is not contained in the index,
it may not be present in the working tree currently either. In that
situation giving a 'path' argument is not useful. Upgrade the
repo_submodule_init function to take a struct submodule instead.
The submodule struct can be obtained via submodule_from_{path, name} or
an artificial submodule struct can be passed in.
While we are at it, rename the repository struct in the repo_submodule_init
function, which is to be initialized, to a name that is not confused with
the struct submodule as easily. Perform such renames in similar functions
as well.
Also move its documentation into the header file.
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/grep.c | 17 +++++++-----
builtin/ls-files.c | 12 +++++----
builtin/submodule--helper.c | 2 +-
repository.c | 27 ++++++++------------
repository.h | 12 +++++++--
t/helper/test-submodule-nested-repo-config.c | 8 +++---
6 files changed, 43 insertions(+), 35 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 71df52a333..d6bd887b2d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -404,7 +404,10 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
const struct object_id *oid,
const char *filename, const char *path)
{
- struct repository submodule;
+ struct repository subrepo;
+ const struct submodule *sub = submodule_from_path(superproject,
+ &null_oid, path);
+
int hit;
/*
@@ -420,12 +423,12 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
return 0;
}
- if (repo_submodule_init(&submodule, superproject, path)) {
+ if (repo_submodule_init(&subrepo, superproject, sub)) {
grep_read_unlock();
return 0;
}
- repo_read_gitmodules(&submodule);
+ repo_read_gitmodules(&subrepo);
/*
* NEEDSWORK: This adds the submodule's object directory to the list of
@@ -437,7 +440,7 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
* store is no longer global and instead is a member of the repository
* object.
*/
- add_to_alternates_memory(submodule.objects->objectdir);
+ add_to_alternates_memory(subrepo.objects->objectdir);
grep_read_unlock();
if (oid) {
@@ -462,14 +465,14 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
init_tree_desc(&tree, data, size);
hit = grep_tree(opt, pathspec, &tree, &base, base.len,
- object->type == OBJ_COMMIT, &submodule);
+ object->type == OBJ_COMMIT, &subrepo);
strbuf_release(&base);
free(data);
} else {
- hit = grep_cache(opt, &submodule, pathspec, 1);
+ hit = grep_cache(opt, &subrepo, pathspec, 1);
}
- repo_clear(&submodule);
+ repo_clear(&subrepo);
return hit;
}
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c70a9c7158..583a0e1ca2 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -206,17 +206,19 @@ static void show_files(struct repository *repo, struct dir_struct *dir);
static void show_submodule(struct repository *superproject,
struct dir_struct *dir, const char *path)
{
- struct repository submodule;
+ struct repository subrepo;
+ const struct submodule *sub = submodule_from_path(superproject,
+ &null_oid, path);
- if (repo_submodule_init(&submodule, superproject, path))
+ if (repo_submodule_init(&subrepo, superproject, sub))
return;
- if (repo_read_index(&submodule) < 0)
+ if (repo_read_index(&subrepo) < 0)
die("index file corrupt");
- show_files(&submodule, dir);
+ show_files(&subrepo, dir);
- repo_clear(&submodule);
+ repo_clear(&subrepo);
}
static void show_ce(struct repository *repo, struct dir_struct *dir,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d38113a31a..4eceb8f040 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2053,7 +2053,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
if (!sub)
BUG("We could get the submodule handle before?");
- if (repo_submodule_init(&subrepo, the_repository, path))
+ if (repo_submodule_init(&subrepo, the_repository, sub))
die(_("could not get a repository handle for submodule '%s'"), path);
if (!repo_config_get_string(&subrepo, "core.worktree", &cw)) {
diff --git a/repository.c b/repository.c
index 5dd1486718..aabe64ee5d 100644
--- a/repository.c
+++ b/repository.c
@@ -166,30 +166,23 @@ int repo_init(struct repository *repo,
return -1;
}
-/*
- * Initialize 'submodule' as the submodule given by 'path' in parent repository
- * 'superproject'.
- * Return 0 upon success and a non-zero value upon failure.
- */
-int repo_submodule_init(struct repository *submodule,
+int repo_submodule_init(struct repository *subrepo,
struct repository *superproject,
- const char *path)
+ const struct submodule *sub)
{
- const struct submodule *sub;
struct strbuf gitdir = STRBUF_INIT;
struct strbuf worktree = STRBUF_INIT;
int ret = 0;
- sub = submodule_from_path(superproject, &null_oid, path);
if (!sub) {
ret = -1;
goto out;
}
- strbuf_repo_worktree_path(&gitdir, superproject, "%s/.git", path);
- strbuf_repo_worktree_path(&worktree, superproject, "%s", path);
+ strbuf_repo_worktree_path(&gitdir, superproject, "%s/.git", sub->path);
+ strbuf_repo_worktree_path(&worktree, superproject, "%s", sub->path);
- if (repo_init(submodule, gitdir.buf, worktree.buf)) {
+ if (repo_init(subrepo, gitdir.buf, worktree.buf)) {
/*
* If initilization fails then it may be due to the submodule
* not being populated in the superproject's worktree. Instead
@@ -201,16 +194,16 @@ int repo_submodule_init(struct repository *submodule,
strbuf_repo_git_path(&gitdir, superproject,
"modules/%s", sub->name);
- if (repo_init(submodule, gitdir.buf, NULL)) {
+ if (repo_init(subrepo, gitdir.buf, NULL)) {
ret = -1;
goto out;
}
}
- submodule->submodule_prefix = xstrfmt("%s%s/",
- superproject->submodule_prefix ?
- superproject->submodule_prefix :
- "", path);
+ subrepo->submodule_prefix = xstrfmt("%s%s/",
+ superproject->submodule_prefix ?
+ superproject->submodule_prefix :
+ "", sub->path);
out:
strbuf_release(&gitdir);
diff --git a/repository.h b/repository.h
index 9f16c42c1e..0e482b7d49 100644
--- a/repository.h
+++ b/repository.h
@@ -116,9 +116,17 @@ void repo_set_worktree(struct repository *repo, const char *path);
void repo_set_hash_algo(struct repository *repo, int algo);
void initialize_the_repository(void);
int repo_init(struct repository *r, const char *gitdir, const char *worktree);
-int repo_submodule_init(struct repository *submodule,
+
+/*
+ * Initialize the repository 'subrepo' as the submodule given by the
+ * struct submodule 'sub' in parent repository 'superproject'.
+ * Return 0 upon success and a non-zero value upon failure, which may happen
+ * if the submodule is not found, or 'sub' is NULL.
+ */
+struct submodule;
+int repo_submodule_init(struct repository *subrepo,
struct repository *superproject,
- const char *path);
+ const struct submodule *sub);
void repo_clear(struct repository *repo);
/*
diff --git a/t/helper/test-submodule-nested-repo-config.c b/t/helper/test-submodule-nested-repo-config.c
index a31e2a9bea..bc97929bbc 100644
--- a/t/helper/test-submodule-nested-repo-config.c
+++ b/t/helper/test-submodule-nested-repo-config.c
@@ -10,19 +10,21 @@ static void die_usage(int argc, const char **argv, const char *msg)
int cmd__submodule_nested_repo_config(int argc, const char **argv)
{
- struct repository submodule;
+ struct repository subrepo;
+ const struct submodule *sub;
if (argc < 3)
die_usage(argc, argv, "Wrong number of arguments.");
setup_git_directory();
- if (repo_submodule_init(&submodule, the_repository, argv[1])) {
+ sub = submodule_from_path(the_repository, &null_oid, argv[1]);
+ if (repo_submodule_init(&subrepo, the_repository, sub)) {
die_usage(argc, argv, "Submodule not found.");
}
/* Read the config of _child_ submodules. */
- print_config_from_gitmodules(&submodule, argv[2]);
+ print_config_from_gitmodules(&subrepo, argv[2]);
submodule_free(the_repository);
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* [PATCH 7/9] submodule: migrate get_next_submodule to use repository structs
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
We used to recurse into submodules, even if they were broken having
only an objects directory. The child process executed in the submodule
would fail though if the submodule was broken. This is tested via
"fetching submodule into a broken repository" in t5526.
This patch tightens the check upfront, such that we do not need
to spawn a child process to find out if the submodule is broken.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 56 +++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 44 insertions(+), 12 deletions(-)
diff --git a/submodule.c b/submodule.c
index 0c81aca6f2..77ace5e784 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1253,6 +1253,30 @@ static int get_fetch_recurse_config(const struct submodule *submodule,
return spf->default_option;
}
+static struct repository *get_submodule_repo_for(struct repository *r,
+ const struct submodule *sub)
+{
+ struct repository *ret = xmalloc(sizeof(*ret));
+
+ if (repo_submodule_init(ret, r, sub)) {
+ /*
+ * No entry in .gitmodules? Technically not a submodule,
+ * but historically we supported repositories that happen to be
+ * in-place where a gitlink is. Keep supporting them.
+ */
+ struct strbuf gitdir = STRBUF_INIT;
+ strbuf_repo_worktree_path(&gitdir, r, "%s/.git", sub->path);
+ if (repo_init(ret, gitdir.buf, NULL)) {
+ strbuf_release(&gitdir);
+ free(ret);
+ return NULL;
+ }
+ strbuf_release(&gitdir);
+ }
+
+ return ret;
+}
+
static int get_next_submodule(struct child_process *cp,
struct strbuf *err, void *data, void **task_cb)
{
@@ -1260,12 +1284,11 @@ static int get_next_submodule(struct child_process *cp,
struct submodule_parallel_fetch *spf = data;
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
- struct strbuf submodule_path = STRBUF_INIT;
- struct strbuf submodule_git_dir = STRBUF_INIT;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
- const char *git_dir, *default_argv;
+ const char *default_argv;
const struct submodule *submodule;
+ struct repository *repo;
struct submodule default_submodule = SUBMODULE_INIT;
if (!S_ISGITLINK(ce->ce_mode))
@@ -1300,15 +1323,11 @@ static int get_next_submodule(struct child_process *cp,
continue;
}
- strbuf_repo_worktree_path(&submodule_path, spf->r, "%s", ce->name);
- strbuf_addf(&submodule_git_dir, "%s/.git", submodule_path.buf);
strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, ce->name);
- git_dir = read_gitfile(submodule_git_dir.buf);
- if (!git_dir)
- git_dir = submodule_git_dir.buf;
- if (is_directory(git_dir)) {
+ repo = get_submodule_repo_for(spf->r, submodule);
+ if (repo) {
child_process_init(cp);
- cp->dir = strbuf_detach(&submodule_path, NULL);
+ cp->dir = xstrdup(repo->worktree);
prepare_submodule_repo_env(&cp->env_array);
cp->git_cmd = 1;
if (!spf->quiet)
@@ -1319,10 +1338,23 @@ static int get_next_submodule(struct child_process *cp,
argv_array_push(&cp->args, default_argv);
argv_array_push(&cp->args, "--submodule-prefix");
argv_array_push(&cp->args, submodule_prefix.buf);
+
+ repo_clear(repo);
+ free(repo);
ret = 1;
+ } else {
+ /*
+ * An empty directory is normal,
+ * the submodule is not initialized
+ */
+ if (S_ISGITLINK(ce->ce_mode) &&
+ !is_empty_dir(ce->name)) {
+ spf->result = 1;
+ strbuf_addf(err,
+ _("Could not access submodule '%s'"),
+ ce->name);
+ }
}
- strbuf_release(&submodule_path);
- strbuf_release(&submodule_git_dir);
strbuf_release(&submodule_prefix);
if (ret) {
spf->count++;
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* [PATCH 8/9] submodule.c: fetch in submodules git directory instead of in worktree
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
Keep the properties introduced in 10f5c52656 (submodule: avoid
auto-discovery in prepare_submodule_repo_env(), 2016-09-01), by fixating
the git directory of the submodule.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index 77ace5e784..d1b6646f42 100644
--- a/submodule.c
+++ b/submodule.c
@@ -494,6 +494,12 @@ void prepare_submodule_repo_env(struct argv_array *out)
DEFAULT_GIT_DIR_ENVIRONMENT);
}
+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
+{
+ prepare_submodule_repo_env_no_git_dir(out);
+ argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
+}
+
/* Helper function to display the submodule header line prior to the full
* summary output. If it can locate the submodule objects directory it will
* attempt to lookup both the left and right commits and put them into the
@@ -1327,8 +1333,8 @@ static int get_next_submodule(struct child_process *cp,
repo = get_submodule_repo_for(spf->r, submodule);
if (repo) {
child_process_init(cp);
- cp->dir = xstrdup(repo->worktree);
- prepare_submodule_repo_env(&cp->env_array);
+ cp->dir = xstrdup(repo->gitdir);
+ prepare_submodule_repo_env_in_gitdir(&cp->env_array);
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* [PATCH 9/9] fetch: try fetching submodules if needed objects were not fetched
From: Stefan Beller @ 2018-11-29 0:27 UTC (permalink / raw)
To: git; +Cc: jonathantanmy, Stefan Beller
In-Reply-To: <20181129002756.167615-1-sbeller@google.com>
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C <submodule-dir>" (with some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.
But this default fetch is not sufficient, as a newly fetched commit in
the superproject could point to a commit in the submodule that is not
in the default refspec. This is common in workflows like Gerrit's.
When fetching a Gerrit change under review (from refs/changes/??), the
commits in that change likely point to submodule commits that have not
been merged to a branch yet.
Try fetching a submodule by object id if the object id that the
superproject points to, cannot be found.
builtin/fetch used to only inspect submodules when they were fetched
"on-demand", as in either on/off case it was clear whether the submodule
needs to be fetched. However to know whether we need to try fetching the
object ids, we need to identify the object names, which is done in this
function check_for_new_submodule_commits(), so we'll also run that code
in case the submodule recursion is set to "on".
The submodule checks were done only when a ref in the superproject
changed, these checks were extended to also be performed when fetching
into FETCH_HEAD for completeness, and add a test for that too.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/fetch.c | 11 +-
submodule.c | 206 +++++++++++++++++++++++++++++++-----
t/t5526-fetch-submodules.sh | 86 +++++++++++++++
3 files changed, 265 insertions(+), 38 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e0140327aa..91f9b7d9c8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -763,9 +763,6 @@ static int update_local_ref(struct ref *ref,
what = _("[new ref]");
}
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
- check_for_new_submodule_commits(&ref->new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
r ? _("unable to update local ref") : NULL,
@@ -779,9 +776,6 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);
strbuf_addstr(&quickref, "..");
strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
- check_for_new_submodule_commits(&ref->new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
r ? _("unable to update local ref") : NULL,
@@ -794,9 +788,6 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV);
strbuf_addstr(&quickref, "...");
strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
- check_for_new_submodule_commits(&ref->new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
r ? _("unable to update local ref") : _("forced update"),
@@ -892,6 +883,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
ref->force = rm->peer_ref->force;
}
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
+ check_for_new_submodule_commits(&rm->old_oid);
if (!strcmp(rm->name, "HEAD")) {
kind = "";
diff --git a/submodule.c b/submodule.c
index d1b6646f42..1ce944a737 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1231,8 +1231,14 @@ struct submodule_parallel_fetch {
int result;
struct string_list changed_submodule_names;
+
+ /* The submodules to fetch in */
+ struct fetch_task **oid_fetch_tasks;
+ int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
+ STRING_LIST_INIT_DUP, \
+ NULL, 0, 0}
static int get_fetch_recurse_config(const struct submodule *submodule,
struct submodule_parallel_fetch *spf)
@@ -1259,6 +1265,73 @@ static int get_fetch_recurse_config(const struct submodule *submodule,
return spf->default_option;
}
+struct fetch_task {
+ struct repository *repo;
+ const struct submodule *sub;
+ unsigned free_sub : 1; /* Do we need to free the submodule? */
+
+ /* fetch specific oids if set, otherwise fetch default refspec */
+ struct oid_array *commits;
+};
+
+/**
+ * When a submodule is not defined in .gitmodules, we cannot access it
+ * via the regular submodule-config. Create a fake submodule, which we can
+ * work on.
+ */
+static const struct submodule *get_non_gitmodules_submodule(const char *path)
+{
+ struct submodule *ret = NULL;
+ const char *name = default_name_or_path(path);
+
+ if (!name)
+ return NULL;
+
+ ret = xmalloc(sizeof(*ret));
+ memset(ret, 0, sizeof(*ret));
+ ret->path = name;
+ ret->name = name;
+
+ return (const struct submodule *) ret;
+}
+
+static struct fetch_task *fetch_task_create(struct repository *r,
+ const char *path)
+{
+ struct fetch_task *task = xmalloc(sizeof(*task));
+ memset(task, 0, sizeof(*task));
+
+ task->sub = submodule_from_path(r, &null_oid, path);
+ if (!task->sub) {
+ /*
+ * No entry in .gitmodules? Technically not a submodule,
+ * but historically we supported repositories that happen to be
+ * in-place where a gitlink is. Keep supporting them.
+ */
+ task->sub = get_non_gitmodules_submodule(path);
+ if (!task->sub) {
+ free(task);
+ return NULL;
+ }
+
+ task->free_sub = 1;
+ }
+
+ return task;
+}
+
+static void fetch_task_release(struct fetch_task *p)
+{
+ if (p->free_sub)
+ free((void*)p->sub);
+ p->free_sub = 0;
+ p->sub = NULL;
+
+ if (p->repo)
+ repo_clear(p->repo);
+ FREE_AND_NULL(p->repo);
+}
+
static struct repository *get_submodule_repo_for(struct repository *r,
const struct submodule *sub)
{
@@ -1286,39 +1359,32 @@ static struct repository *get_submodule_repo_for(struct repository *r,
static int get_next_submodule(struct child_process *cp,
struct strbuf *err, void *data, void **task_cb)
{
- int ret = 0;
struct submodule_parallel_fetch *spf = data;
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
- struct strbuf submodule_prefix = STRBUF_INIT;
+ int recurse_config;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
const char *default_argv;
- const struct submodule *submodule;
- struct repository *repo;
- struct submodule default_submodule = SUBMODULE_INIT;
+ struct fetch_task *task;
if (!S_ISGITLINK(ce->ce_mode))
continue;
- submodule = submodule_from_path(spf->r, &null_oid, ce->name);
- if (!submodule) {
- const char *name = default_name_or_path(ce->name);
- if (name) {
- default_submodule.path = name;
- default_submodule.name = name;
- submodule = &default_submodule;
- }
- }
+ task = fetch_task_create(spf->r, ce->name);
+ if (!task)
+ continue;
+
+ recurse_config = get_fetch_recurse_config(task->sub, spf);
- switch (get_fetch_recurse_config(submodule, spf))
+ switch (recurse_config)
{
default:
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
- if (!submodule ||
+ if (!task->sub ||
!string_list_lookup(
&spf->changed_submodule_names,
- submodule->name))
+ task->sub->name))
continue;
default_argv = "on-demand";
break;
@@ -1329,11 +1395,11 @@ static int get_next_submodule(struct child_process *cp,
continue;
}
- strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, ce->name);
- repo = get_submodule_repo_for(spf->r, submodule);
- if (repo) {
+ task->repo = get_submodule_repo_for(spf->r, task->sub);
+ if (task->repo) {
+ struct strbuf submodule_prefix = STRBUF_INIT;
child_process_init(cp);
- cp->dir = xstrdup(repo->gitdir);
+ cp->dir = task->repo->gitdir;
prepare_submodule_repo_env_in_gitdir(&cp->env_array);
cp->git_cmd = 1;
if (!spf->quiet)
@@ -1343,12 +1409,22 @@ static int get_next_submodule(struct child_process *cp,
argv_array_pushv(&cp->args, spf->args.argv);
argv_array_push(&cp->args, default_argv);
argv_array_push(&cp->args, "--submodule-prefix");
+
+ strbuf_addf(&submodule_prefix, "%s%s/",
+ spf->prefix,
+ task->sub->path);
argv_array_push(&cp->args, submodule_prefix.buf);
- repo_clear(repo);
- free(repo);
- ret = 1;
+ spf->count++;
+ *task_cb = task;
+
+ strbuf_release(&submodule_prefix);
+ return 1;
} else {
+
+ fetch_task_release(task);
+ free(task);
+
/*
* An empty directory is normal,
* the submodule is not initialized
@@ -1361,12 +1437,38 @@ static int get_next_submodule(struct child_process *cp,
ce->name);
}
}
+ }
+
+ if (spf->oid_fetch_tasks_nr) {
+ struct fetch_task *task =
+ spf->oid_fetch_tasks[spf->oid_fetch_tasks_nr - 1];
+ struct strbuf submodule_prefix = STRBUF_INIT;
+ spf->oid_fetch_tasks_nr--;
+
+ strbuf_addf(&submodule_prefix, "%s%s/",
+ spf->prefix, task->sub->path);
+
+ child_process_init(cp);
+ prepare_submodule_repo_env_in_gitdir(&cp->env_array);
+ cp->git_cmd = 1;
+ cp->dir = task->repo->gitdir;
+
+ argv_array_init(&cp->args);
+ argv_array_pushv(&cp->args, spf->args.argv);
+ argv_array_push(&cp->args, "on-demand");
+ argv_array_push(&cp->args, "--submodule-prefix");
+ argv_array_push(&cp->args, submodule_prefix.buf);
+
+ /* NEEDSWORK: have get_default_remote from submodule--helper */
+ argv_array_push(&cp->args, "origin");
+ oid_array_for_each_unique(task->commits,
+ append_oid_to_argv, &cp->args);
+
+ *task_cb = task;
strbuf_release(&submodule_prefix);
- if (ret) {
- spf->count++;
- return 1;
- }
+ return 1;
}
+
return 0;
}
@@ -1374,20 +1476,66 @@ static int fetch_start_failure(struct strbuf *err,
void *cb, void *task_cb)
{
struct submodule_parallel_fetch *spf = cb;
+ struct fetch_task *task = task_cb;
spf->result = 1;
+ fetch_task_release(task);
return 0;
}
+static int commit_exists_in_sub(const struct object_id *oid, void *data)
+{
+ struct repository *subrepo = data;
+
+ enum object_type type = oid_object_info(subrepo, oid, NULL);
+
+ return type != OBJ_COMMIT;
+}
+
static int fetch_finish(int retvalue, struct strbuf *err,
void *cb, void *task_cb)
{
struct submodule_parallel_fetch *spf = cb;
+ struct fetch_task *task = task_cb;
+
+ struct string_list_item *it;
+ struct oid_array *commits;
if (retvalue)
spf->result = 1;
+ if (!task || !task->sub)
+ BUG("callback cookie bogus");
+
+ /* Is this the second time we process this submodule? */
+ if (task->commits)
+ return 0;
+
+ it = string_list_lookup(&spf->changed_submodule_names, task->sub->name);
+ if (!it)
+ /* Could be an unchanged submodule, not contained in the list */
+ goto out;
+
+ commits = it->util;
+ oid_array_filter(commits,
+ commit_exists_in_sub,
+ task->repo);
+
+ /* Are there commits we want, but do not exist? */
+ if (commits->nr) {
+ task->commits = commits;
+ ALLOC_GROW(spf->oid_fetch_tasks,
+ spf->oid_fetch_tasks_nr + 1,
+ spf->oid_fetch_tasks_alloc);
+ spf->oid_fetch_tasks[spf->oid_fetch_tasks_nr] = task;
+ spf->oid_fetch_tasks_nr++;
+ return 0;
+ }
+
+out:
+ fetch_task_release(task);
+
return 0;
}
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 6c2f9b2ba2..8a016272bc 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -600,4 +600,90 @@ test_expect_success "fetch new commits when submodule got renamed" '
test_cmp expect actual
'
+test_expect_success "fetch new submodule commits on-demand outside standard refspec" '
+ # add a second submodule and ensure it is around in downstream first
+ git clone submodule sub1 &&
+ git submodule add ./sub1 &&
+ git commit -m "adding a second submodule" &&
+ git -C downstream pull &&
+ git -C downstream submodule update --init --recursive &&
+
+ git checkout --detach &&
+
+ C=$(git -C submodule commit-tree -m "new change outside refs/heads" HEAD^{tree}) &&
+ git -C submodule update-ref refs/changes/1 $C &&
+ git update-index --cacheinfo 160000 $C submodule &&
+ test_tick &&
+
+ D=$(git -C sub1 commit-tree -m "new change outside refs/heads" HEAD^{tree}) &&
+ git -C sub1 update-ref refs/changes/2 $D &&
+ git update-index --cacheinfo 160000 $D sub1 &&
+
+ git commit -m "updated submodules outside of refs/heads" &&
+ E=$(git rev-parse HEAD) &&
+ git update-ref refs/changes/3 $E &&
+ (
+ cd downstream &&
+ git fetch --recurse-submodules origin refs/changes/3:refs/heads/my_branch &&
+ git -C submodule cat-file -t $C &&
+ git -C sub1 cat-file -t $D &&
+ git checkout --recurse-submodules FETCH_HEAD
+ )
+'
+
+test_expect_success 'fetch new submodule commits on-demand in FETCH_HEAD' '
+ # depends on the previous test for setup
+
+ C=$(git -C submodule commit-tree -m "another change outside refs/heads" HEAD^{tree}) &&
+ git -C submodule update-ref refs/changes/4 $C &&
+ git update-index --cacheinfo 160000 $C submodule &&
+ test_tick &&
+
+ D=$(git -C sub1 commit-tree -m "another change outside refs/heads" HEAD^{tree}) &&
+ git -C sub1 update-ref refs/changes/5 $D &&
+ git update-index --cacheinfo 160000 $D sub1 &&
+
+ git commit -m "updated submodules outside of refs/heads" &&
+ E=$(git rev-parse HEAD) &&
+ git update-ref refs/changes/6 $E &&
+ (
+ cd downstream &&
+ git fetch --recurse-submodules origin refs/changes/6 &&
+ git -C submodule cat-file -t $C &&
+ git -C sub1 cat-file -t $D &&
+ git checkout --recurse-submodules FETCH_HEAD
+ )
+'
+
+test_expect_success 'fetch new submodule commits on-demand without .gitmodules entry' '
+ # depends on the previous test for setup
+
+ git config -f .gitmodules --remove-section submodule.sub1 &&
+ git add .gitmodules &&
+ git commit -m "delete gitmodules file" &&
+ git checkout -B master &&
+ git -C downstream fetch &&
+ git -C downstream checkout origin/master &&
+
+ C=$(git -C submodule commit-tree -m "yet another change outside refs/heads" HEAD^{tree}) &&
+ git -C submodule update-ref refs/changes/7 $C &&
+ git update-index --cacheinfo 160000 $C submodule &&
+ test_tick &&
+
+ D=$(git -C sub1 commit-tree -m "yet another change outside refs/heads" HEAD^{tree}) &&
+ git -C sub1 update-ref refs/changes/8 $D &&
+ git update-index --cacheinfo 160000 $D sub1 &&
+
+ git commit -m "updated submodules outside of refs/heads" &&
+ E=$(git rev-parse HEAD) &&
+ git update-ref refs/changes/9 $E &&
+ (
+ cd downstream &&
+ git fetch --recurse-submodules origin refs/changes/9 &&
+ git -C submodule cat-file -t $C &&
+ git -C sub1 cat-file -t $D &&
+ git checkout --recurse-submodules FETCH_HEAD
+ )
+'
+
test_done
--
2.20.0.rc1.387.gf8505762e3-goog
^ permalink raw reply related
* Re: [PATCH 09/10] fetch: try fetching submodules if needed objects were not fetched
From: Stefan Beller @ 2018-11-29 0:30 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <20181026204106.132296-1-jonathantanmy@google.com>
On Fri, Oct 26, 2018 at 1:41 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > But this default fetch is not sufficient, as a newly fetched commit in
> > the superproject could point to a commit in the submodule that is not
> > in the default refspec. This is common in workflows like Gerrit's.
> > When fetching a Gerrit change under review (from refs/changes/??), the
> > commits in that change likely point to submodule commits that have not
> > been merged to a branch yet.
> >
> > Try fetching a submodule by object id if the object id that the
> > superproject points to, cannot be found.
>
> I see that these suggestions of mine (from [1]) was implemented, but not
> others. If you disagree, that's fine, but I think they should be
> discussed.
ok.
>
> > - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> > - (recurse_submodules != RECURSE_SUBMODULES_ON))
> > + if (recurse_submodules != RECURSE_SUBMODULES_OFF)
>
> I think the next patch should be squashed into this patch. Then you can
> say that these are now redundant and can be removed.
ok.
>
> > @@ -1218,8 +1218,12 @@ struct submodule_parallel_fetch {
> > int result;
> >
> > struct string_list changed_submodule_names;
> > + struct get_next_submodule_task **fetch_specific_oids;
> > + int fetch_specific_oids_nr, fetch_specific_oids_alloc;
> > };
>
> Add documentation for fetch_specific_oids. Also, it might be better to
> call these oid_fetch_tasks and the struct, "struct fetch_task".
ok.
> Here, struct get_next_submodule_task is used for 2 different things:
> (1) After the first round fetch, fetch_finish() uses it to determine if
> a second round is needed.
> (2) In submodule_parallel_fetch.fetch_specific_oids, to tell the
> parallel runner (through get_next_submodule_task()) to do this
> fetch.
>
> I think that it's better to have 2 different structs for these 2
> different uses. (Note that task_cb can be NULL for the second round.
> Unless we plan to recheck the OIDs? Currently we recheck them, but we
> don't do anything either way.)
I think it is easier to only have one struct until we have substantially
more to communicate. (1) is a boolean answer, for which (non-)NULL
is sufficient.
> I think that this is best described as the submodule that has no entry
> in .gitmodules? Maybe call it "get_non_gitmodules_submodule" and
> document it with a similar comment as in get_submodule_repo_for().
done.
>
> > +
> > +static struct get_next_submodule_task *get_next_submodule_task_create(
> > + struct repository *r, const char *path)
> > +{
> > + struct get_next_submodule_task *task = xmalloc(sizeof(*task));
> > + memset(task, 0, sizeof(*task));
> > +
> > + task->sub = submodule_from_path(r, &null_oid, path);
> > + if (!task->sub) {
> > + task->sub = get_default_submodule(path);
> > + task->free_sub = 1;
> > + }
> > +
> > + return task;
> > +}
>
> Clearer to me would be to make get_next_submodule_task_create() return
> NULL if submodule_from_path() returns NULL.
I doubled down on this one and return NULL when get_default_submodule
(now renamed to get_non_gitmodules_submodule) returns NULL, as then we
can move the free() from get_next_submodule here and there we'll just have
task = fetch_task_create(spf->r, ce->name);
if (!task)
continue;
which helps get_next_submodule to stay readable.
> Same comment about "on-demand" as in my previous e-mail.
I'd want to push back on refactoring and defer that to a later series.
> Break lines to 80.
[...]
> Same comment about "s--h" as in my previous e-mail.
done
> > + /* Are there commits that do not exist? */
> > + if (commits->nr) {
> > + /* We already tried fetching them, do not try again. */
> > + if (task->commits)
> > + return 0;
>
> Same comment about "task->commits" as in my previous e-mail.
Good call. I reordered the function read easier and added a comment
on any early return how it could happen.
>
> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> > index 6c2f9b2ba2..5a75b57852 100755
>
> One more thing to test is the case where a submodule doesn't have a
> .gitmodules entry.
added a test.
I just resent the series.
Stefan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox