Re: [PATCH 13/16] test-reach: test can_all_from_reach_with_flags
On 7/16/2018 5:54 PM, Stefan Beller wrote: On Mon, Jul 16, 2018 at 6:00 AM Derrick Stolee via GitGitGadget wrote: From: Derrick Stolee The can_all_from_reach_with_flags method is used by ok_to_give_up in upload-pack.c to see if we have done enough negotiation during a fetch. This method is intentionally created to preserve state between calls to assist with stateful negotiation, such as over SSH. To make this method testable, add a new can_all_from_reach method that does the initial setup and final tear-down. Call the method from 'test-tool reach'. Since this is a many-to-many reachability query, add a new type of input to the 'test-tool reach' input format. Lines "Y:" create a list of commits to be the reachability targets from the commits in the 'X' list. In the context of fetch negotiation, the 'X' commits are the 'want' commits and the 'Y' commits are the 'have' commits. Makes sense. I shortly wondered if we want to s/Y/Z/ as I find X and Z more distinguishable than X/Y for reading/skimming. Thanks, Stefan +++ b/commit-reach.c @@ -593,3 +593,50 @@ int can_all_from_reach_with_flag(struct object_array *from, } return 1; } + +int can_all_from_reach(struct commit_list *from, struct commit_list *to, + int cutoff_by_min_date) We'll put this method (that is only used by tests so far) here to not clutter the test tool code too much, or do we see more benefits from the code here? If so, docs would be nice. We will use it later as we reduce duplicate walk implementations, but I can hint at that in the message. +++ b/t/t6600-test-reach.sh +test_expect_success 'can_all_from_reach:hit' ' [...] + Y:commit-7-3 + Y:commit-8-1 +test_expect_success 'can_all_from_reach:miss' ' [...] + Y:commit-8-5 It would be nice if the difference in the list could be easier to spot as a reader. (There is a lot of repetition). Maybe we can teach "test-tool reach" to ignore input lines starting with '#' such that we can annotate the last line in the miss case? Why do we omit 7-3 in the miss case? (might be nice for symmetry to keep around) The X-commit that fails to reach a Y-commit in this second case is commit-8-3. That commit can reach both commit-7-3 and commit-8-1, so both need to be removed. Thanks, -Stolee
Re: [PATCH 13/16] test-reach: test can_all_from_reach_with_flags
The subject should be can_all_from_reach_with_flag (without the "s" at the end). Likewise in the commit message. > To make this method testable, add a new can_all_from_reach method that > does the initial setup and final tear-down. Call the method from > 'test-tool reach'. This description leads me to believe that can_all_from_reach() is (1) trivial, and (2) will not be used in production code. But (1) the function itself is non-trivial and the function signature contains a "cutoff_by_min_date" parameter not found in can_all_from_reach_with_flag(): > +int can_all_from_reach(struct commit_list *from, struct commit_list *to, > +int cutoff_by_min_date) and (2) this function will be used in production code subsequently in the "commit-reach: use can_all_from_reach" commit. It would be clearer, maybe, if there were some rearrangement - maybe a commit introducing this function, especially documenting what cutoff_by_min_date does, and then a test (which just tests can_all_from_reach), and then the "commit-reach: use can_all_from_reach" commit.
Re: [PATCH 13/16] test-reach: test can_all_from_reach_with_flags
On Mon, Jul 16, 2018 at 6:00 AM Derrick Stolee via GitGitGadget wrote: > > From: Derrick Stolee > > The can_all_from_reach_with_flags method is used by ok_to_give_up in > upload-pack.c to see if we have done enough negotiation during a fetch. > This method is intentionally created to preserve state between calls to > assist with stateful negotiation, such as over SSH. > > To make this method testable, add a new can_all_from_reach method that > does the initial setup and final tear-down. Call the method from > 'test-tool reach'. > > Since this is a many-to-many reachability query, add a new type of input > to the 'test-tool reach' input format. Lines "Y:" create a > list of commits to be the reachability targets from the commits in the > 'X' list. In the context of fetch negotiation, the 'X' commits are the > 'want' commits and the 'Y' commits are the 'have' commits. Makes sense. I shortly wondered if we want to s/Y/Z/ as I find X and Z more distinguishable than X/Y for reading/skimming. Thanks, Stefan > +++ b/commit-reach.c > @@ -593,3 +593,50 @@ int can_all_from_reach_with_flag(struct object_array > *from, > } > return 1; > } > + > +int can_all_from_reach(struct commit_list *from, struct commit_list *to, > + int cutoff_by_min_date) We'll put this method (that is only used by tests so far) here to not clutter the test tool code too much, or do we see more benefits from the code here? If so, docs would be nice. > +++ b/t/t6600-test-reach.sh > +test_expect_success 'can_all_from_reach:hit' ' [...] > + Y:commit-7-3 > + Y:commit-8-1 > +test_expect_success 'can_all_from_reach:miss' ' [...] > + Y:commit-8-5 It would be nice if the difference in the list could be easier to spot as a reader. (There is a lot of repetition). Maybe we can teach "test-tool reach" to ignore input lines starting with '#' such that we can annotate the last line in the miss case? Why do we omit 7-3 in the miss case? (might be nice for symmetry to keep around) Thanks, Stefan
[PATCH 13/16] test-reach: test can_all_from_reach_with_flags
From: Derrick Stolee
The can_all_from_reach_with_flags method is used by ok_to_give_up in
upload-pack.c to see if we have done enough negotiation during a fetch.
This method is intentionally created to preserve state between calls to
assist with stateful negotiation, such as over SSH.
To make this method testable, add a new can_all_from_reach method that
does the initial setup and final tear-down. Call the method from
'test-tool reach'.
Since this is a many-to-many reachability query, add a new type of input
to the 'test-tool reach' input format. Lines "Y:" create a
list of commits to be the reachability targets from the commits in the
'X' list. In the context of fetch negotiation, the 'X' commits are the
'want' commits and the 'Y' commits are the 'have' commits.
Signed-off-by: Derrick Stolee
---
commit-reach.c| 47 +++
commit-reach.h| 2 ++
t/helper/test-reach.c | 10 +++--
t/t6600-test-reach.sh | 45 +
4 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index e07ff993f..c5e89a2de 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -593,3 +593,50 @@ int can_all_from_reach_with_flag(struct object_array *from,
}
return 1;
}
+
+int can_all_from_reach(struct commit_list *from, struct commit_list *to,
+ int cutoff_by_min_date)
+{
+ struct object_array from_objs = OBJECT_ARRAY_INIT;
+ time_t min_commit_date = cutoff_by_min_date ? from->item->date : 0;
+ struct commit_list *from_iter = from, *to_iter = to;
+ int result;
+
+ while (from_iter) {
+ add_object_array(_iter->item->object, NULL, _objs);
+
+ if (!parse_commit(from_iter->item)) {
+ if (from_iter->item->date < min_commit_date)
+ min_commit_date = from_iter->item->date;
+ }
+
+ from_iter = from_iter->next;
+ }
+
+ while (to_iter) {
+ if (!parse_commit(to_iter->item)) {
+ if (to_iter->item->date < min_commit_date)
+ min_commit_date = to_iter->item->date;
+ }
+
+ to_iter->item->object.flags |= PARENT2;
+
+ to_iter = to_iter->next;
+ }
+
+ result = can_all_from_reach_with_flag(_objs, PARENT2, PARENT1,
+ min_commit_date);
+
+ while (from) {
+ clear_commit_marks(from->item, PARENT1);
+ from = from->next;
+ }
+
+ while (to) {
+ clear_commit_marks(to->item, PARENT2);
+ to = to->next;
+ }
+
+ object_array_clear(_objs);
+ return result;
+}
diff --git a/commit-reach.h b/commit-reach.h
index ce3b963e6..58de0df56 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -68,5 +68,7 @@ int reachable(struct commit *from, int with_flag, int
assign_flag,
int can_all_from_reach_with_flag(struct object_array *from,
int with_flag, int assign_flag,
time_t min_commit_date);
+int can_all_from_reach(struct commit_list *from, struct commit_list *to,
+ int commit_date_cutoff);
#endif
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index 73cb55208..dc9710099 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -9,7 +9,7 @@ int cmd__reach(int ac, const char **av)
{
struct object_id oid_A, oid_B;
struct commit *A, *B;
- struct commit_list *X;
+ struct commit_list *X, *Y;
struct commit **X_array;
int X_nr, X_alloc;
struct strbuf buf = STRBUF_INIT;
@@ -21,7 +21,7 @@ int cmd__reach(int ac, const char **av)
exit(1);
A = B = NULL;
- X = NULL;
+ X = Y = NULL;
X_nr = 0;
X_alloc = 16;
ALLOC_ARRAY(X_array, X_alloc);
@@ -66,6 +66,10 @@ int cmd__reach(int ac, const char **av)
X_array[X_nr++] = c;
break;
+ case 'Y':
+ commit_list_insert(c, );
+ break;
+
default:
die("unexpected start of line: %c", buf.buf[0]);
}
@@ -92,6 +96,8 @@ int cmd__reach(int ac, const char **av)
printf("%s\n", oid_to_hex(>item->object.oid));
list = list->next;
}
+ } else if (!strcmp(av[1], "can_all_from_reach")) {
+ printf("%s(X,Y):%d\n", av[1], can_all_from_reach(X, Y, 1));
}
exit(0);
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index e365a4ea6..cb07c6431 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -160,4 +160,49 @@ test_expect_success 'reduce_heads' '
