Jeff King [off-list ref] writes:
Thanks for reporting, this is an interesting case. I agree that it
probably ought to continue to be a noop. There is nothing to pull, and
so the question of ff-versus-merge should not even enter into it.
Probably something along this line? Hasn't been tested beyond
compiling and passing
$ git checkout master && ./git pull --ff-only -v . maint
but that should be sufficient, I hope.
builtin/pull.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git c/builtin/pull.c w/builtin/pull.c
index ae9f5bd7cc..d925999543 100644
--- c/builtin/pull.c
+++ w/builtin/pull.c
@@ -931,6 +931,33 @@ static int get_can_ff(struct object_id *orig_head,
return ret;
}
+/*
+ * Is orig_head is a descendant of _all_ merge_heads?
+ * Unfortunately is_descendant_of() cannot be used as it
+ * asks if orig_head is a descendant of at least one of them.
+ */
+static int already_up_to_date(struct object_id *orig_head,
+ struct oid_array *merge_heads)
+{
+ int i;
+ struct commit *ours;
+
+ ours = lookup_commit_reference(the_repository, orig_head);
+ for (i = 0; i < merge_heads->nr; i++) {
+ struct commit_list *list = NULL;
+ struct commit *theirs;
+ int ok;
+
+ theirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);
+ commit_list_insert(theirs, &list);
+ ok = repo_is_descendant_of(the_repository, ours, list);
+ free_commit_list(list);
+ if (!ok)
+ return 0;
+ }
+ return 1;
+}
+
static void show_advice_pull_non_ff(void)
{
advise(_("You have divergent branches and need to specify how to reconcile them.\n"@@ -1072,7 +1099,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
/* ff-only takes precedence over rebase */
if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
- if (!can_ff)
+ if (!can_ff && !already_up_to_date(&orig_head, &merge_heads))
die_ff_impossible();
opt_rebase = REBASE_FALSE;
}On Wed, Oct 20, 2021 at 09:28:08AM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
Thanks for reporting, this is an interesting case. I agree that it
probably ought to continue to be a noop. There is nothing to pull, and
so the question of ff-versus-merge should not even enter into it.
Probably something along this line? Hasn't been tested beyond
compiling and passing
$ git checkout master && ./git pull --ff-only -v . maint
but that should be sufficient, I hope.
Yeah, this direction makes sense to me. Just looking over the patch...
+/*
+ * Is orig_head is a descendant of _all_ merge_heads?
+ * Unfortunately is_descendant_of() cannot be used as it
+ * asks if orig_head is a descendant of at least one of them.
+ */
+static int already_up_to_date(struct object_id *orig_head,
+ struct oid_array *merge_heads)
+{
+ int i;
+ struct commit *ours;
+
+ ours = lookup_commit_reference(the_repository, orig_head);
I think orig_head can be the null oid if we're on an unborn HEAD. I
guess you'd want to return "1" in that case (but I could be wrong; it
looks like get_can_ff() assumes it's valid, so perhaps that case is
handled earlier).
I'd expect that merge_heads can never be empty here, or we'd bail
earlier in the command, but I didn't check (though again, get_can_ff()
seems to assume there's at least one).
+ for (i = 0; i < merge_heads->nr; i++) {
+ struct commit_list *list = NULL;
+ struct commit *theirs;
+ int ok;
+
+ theirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);
+ commit_list_insert(theirs, &list);
+ ok = repo_is_descendant_of(the_repository, ours, list);
+ free_commit_list(list);
+ if (!ok)
+ return 0;
+ }
Running a sequence of traversals like this can be slow, because we may
walk over the same history again and again. But I think in the usual
non-octopus cases we'd only have one entry, so we'd only be adding a
single extra merge-base traversal in most cases.
It does feel like this could be combined with get_can_ff() somehow so
that we're not adding even that single traversal. But I expect that may
be hard to do because of the multiple heads (e.g., we cannot use the
usual ahead/behind code).
-Peff
Earlier, we made sure that "git pull --ff-only" (and "git -c
pull.ff=only pull") errors out when our current HEAD is not an
ancestor of the tip of the history we are merging, but the condition
to trigger the error was implemented incorrectly.
Imagine you forked from a remote branch, built your history on top
of it, and then attempted to pull from them again. If they have not
made any update in the meantime, our current HEAD is obviously not
their ancestor, and this new error triggers.
Without the --ff-only option, we just report that there is no need
to pull; we did the same historically with --ff-only, too.
Make sure we do not fail with the recently added check to restore
the historycal behaviour.
Reported-by: Kenneth Arnold <redacted>
Helped-by: Jeff King [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
* With tests and a proposed log message.
builtin/pull.c | 29 ++++++++++++++++++++++++++++-
t/t7601-merge-pull-config.sh | 16 +++++++++++++++-
2 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index b311ea6b9d..05f1f0e446 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -933,6 +933,33 @@ static int get_can_ff(struct object_id *orig_head,
return ret;
}
+/*
+ * Is orig_head is a descendant of _all_ merge_heads?
+ * unfortunately is_descendant_of() cannot be used as it is to
+ * ask if orig_head is a descendant of at least one of them.
+ */
+static int already_up_to_date(struct object_id *orig_head,
+ struct oid_array *merge_heads)
+{
+ int i;
+ struct commit *ours;
+
+ ours = lookup_commit_reference(the_repository, orig_head);
+ for (i = 0; i < merge_heads->nr; i++) {
+ struct commit_list *list = NULL;
+ struct commit *theirs;
+ int ok;
+
+ theirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);
+ commit_list_insert(theirs, &list);
+ ok = repo_is_descendant_of(the_repository, ours, list);
+ free_commit_list(list);
+ if (!ok)
+ return 0;
+ }
+ return 1;
+}
+
static void show_advice_pull_non_ff(void)
{
advise(_("You have divergent branches and need to specify how to reconcile them.\n"@@ -1074,7 +1101,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
/* ff-only takes precedence over rebase */
if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
- if (!can_ff)
+ if (!can_ff && !already_up_to_date(&orig_head, &merge_heads))
die_ff_impossible();
opt_rebase = REBASE_FALSE;
}diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 1f652f433e..6275641b9c 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -2,7 +2,7 @@
test_description='git merge
-Testing pull.* configuration parsing.'
+Testing pull.* configuration parsing and other things.'
. ./test-lib.sh
@@ -387,6 +387,20 @@ test_expect_success 'pull prevents non-fast-forward with "only" in pull.ff' '
test_must_fail git pull . c3
'
+test_expect_success 'already-up-to-date pull succeeds with "only" in pull.ff' '
+ git reset --hard c1 &&
+ test_config pull.ff only &&
+ git pull . c0 &&
+ test "$(git rev-parse HEAD)" = "$(git rev-parse c1)"
+'
+
+test_expect_success 'already-up-to-date pull/rebase succeeds with "only" in pull.ff' '
+ git reset --hard c1 &&
+ test_config pull.ff only &&
+ git -c pull.rebase=true pull . c0 &&
+ test "$(git rev-parse HEAD)" = "$(git rev-parse c1)"
+'
+
test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
git reset --hard c1 &&
git config pull.twohead ours &&
--
2.33.1-904-gfba30156dd
On Wed, Oct 20, 2021 at 12:02:09PM -0700, Junio C Hamano wrote:
Earlier, we made sure that "git pull --ff-only" (and "git -c
pull.ff=only pull") errors out when our current HEAD is not an
ancestor of the tip of the history we are merging, but the condition
to trigger the error was implemented incorrectly.
Imagine you forked from a remote branch, built your history on top
of it, and then attempted to pull from them again. If they have not
made any update in the meantime, our current HEAD is obviously not
their ancestor, and this new error triggers.
Without the --ff-only option, we just report that there is no need
to pull; we did the same historically with --ff-only, too.
Thanks, this looks good to me overall, and I agree this is a regression
we should try to fix promptly (so thank you for jumping on it).
Make sure we do not fail with the recently added check to restore
the historycal behaviour.
Not sure if "historycal" is a typo or some clever pun. :)
+/*
+ * Is orig_head is a descendant of _all_ merge_heads?
s/is a/a/
+static int already_up_to_date(struct object_id *orig_head,
+ struct oid_array *merge_heads)
+{
+ int i;
+ struct commit *ours;
+
+ ours = lookup_commit_reference(the_repository, orig_head);
+ for (i = 0; i < merge_heads->nr; i++) {
+ struct commit_list *list = NULL;
+ struct commit *theirs;
+ int ok;
+
+ theirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);
+ commit_list_insert(theirs, &list);
+ ok = repo_is_descendant_of(the_repository, ours, list);
+ free_commit_list(list);
+ if (!ok)
+ return 0;
+ }
+ return 1;
+}
You answered all of my "what about..." questions from before elsewhere
in the thread, so this looks correct.
+test_expect_success 'already-up-to-date pull succeeds with "only" in pull.ff' '
+ git reset --hard c1 &&
+ test_config pull.ff only &&
+ git pull . c0 &&
+ test "$(git rev-parse HEAD)" = "$(git rev-parse c1)"
+'
+
+test_expect_success 'already-up-to-date pull/rebase succeeds with "only" in pull.ff' '
+ git reset --hard c1 &&
+ test_config pull.ff only &&
+ git -c pull.rebase=true pull . c0 &&
+ test "$(git rev-parse HEAD)" = "$(git rev-parse c1)"
+'
And these tests cover the cases I'd expect. The use of "test" with
process substitution looks a bit funny to me these days, but it does
match the surrounding code (and losing the exit codes isn't a big deal
here, as we are not testing rev-parse).
The combo of "test_config" and "git -c" is unusual, but I don't see
anything wrong with it.
-Peff
On Wed, Oct 20, 2021 at 2:45 PM Jeff King [off-list ref] wrote:
On Wed, Oct 20, 2021 at 12:02:09PM -0700, Junio C Hamano wrote:
quoted
Earlier, we made sure that "git pull --ff-only" (and "git -c
pull.ff=only pull") errors out when our current HEAD is not an
ancestor of the tip of the history we are merging, but the condition
to trigger the error was implemented incorrectly.
Imagine you forked from a remote branch, built your history on top
of it, and then attempted to pull from them again. If they have not
made any update in the meantime, our current HEAD is obviously not
their ancestor, and this new error triggers.
Without the --ff-only option, we just report that there is no need
to pull; we did the same historically with --ff-only, too.
Thanks, this looks good to me overall, and I agree this is a regression
we should try to fix promptly (so thank you for jumping on it).
It looks like you guys got this under control in no time. Thank you so much!
-Alex