Junio C Hamano [off-list ref] writes:
Phillip Wood [off-list ref] writes:
quoted
Looking at origin/seen:builtin/pull.c we already check if we can
fast-forward and unconditionally merge in that case irrespective of
any '--rebase' option or pull.rebase config. It should be simple for
pull to error out if '--ff-only' is given and we cannot fast-forward.
Excellent.
Even though teaching even more special case on the "git pull" side
makes me feel somewhat dirty, but I think it would be a small price
to pay, and the end result would save an useless fork whose sole
purpose is to make the integration step after fetch fail when "pull"
can easily tell, as you said, that it ought to fail, so overall it
would probably be a net win.
A tangent after thinking a bit more.
I do not think "pull.rebase=interactive" affects the "ff logic" at
all. Rebasing integration rebuilds _your_ commits on the current
branch on top of the tip of _their_ history, and if their history is
a descendant of your history (i.e. the history fast-forwards), by
definition, you do not have anything you need to rebuild on top of
theirs, whether with the opportunity to make tweaks via "rebase -i"
or without.
This observation does not change the conclusion at all, but I should
have spoken after thinking X-<.
Thanks.
On Mon, Jul 5, 2021 at 2:53 AM Phillip Wood [off-list ref] wrote:
On 05/07/2021 05:45, Alex Henrie wrote:
quoted
index 12f093121d..b88f0cbcca 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1345,12 +1349,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
N_("ignore changes in whitespace")),
OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
N_("action"), N_("passed to 'git apply'"), 0),
- OPT_BIT('f', "force-rebase", &options.flags,
- N_("cherry-pick all commits, even if unchanged"),
- REBASE_FORCE),
- OPT_BIT(0, "no-ff", &options.flags,
- N_("cherry-pick all commits, even if unchanged"),
- REBASE_FORCE),
+ OPT_SET_INT_F('f', "force-rebase", &options.fast_forward,
+ N_("cherry-pick all commits, even if unchanged"),
+ FF_NO, PARSE_OPT_NONEG),
Why does this change rebase to start rejecting --no-force-rebase? This
is the sort of behavior change that deserves a mention in the commit
message.
That was accidental, sorry. I copied and pasted option code from
another place without paying attention to the PARSE_OPT_NONEG.
quoted
+ OPT_SET_INT(0, "ff", &options.fast_forward, N_("allow fast-forward"),
+ FF_ALLOW),
The useful option when rebasing is '--no-ff', now that will no longer
appear in the output of 'git rebase -h'
Yeah, that's a good point.
On Mon, Jul 5, 2021 at 3:36 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Sun, Jul 04 2021, Alex Henrie wrote:
quoted
+int error_ff_impossible(void)
+{
+ error(_("Not possible to fast-forward, aborting."));
+ return -1;
+}
Here's one, the idiom is just "return error", i.e it itself returns -1.
That would indeed be simpler; thanks for pointing that out.
FWIW I'd consider doing:
/* earlier, static scope */
static const char error_ff_impossible = N_("Not possible...");
/* later, function scope */
return error(error_ff_impossible);
It's a small difference, but for translators who use the jump-to-source
while translating not having the indirection helps, and in this case
it's just used 3 times...
Wouldn't jump-to-source take the user to the English text in advice.c
either way? How does putting it in a static variable help?
quoted
[...]
if (parent && parse_commit(parent) < 0)
/* TRANSLATORS: The first %s will be a "todo" command like@@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
else if (!strcmp(key, "options.record-origin"))
opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
else if (!strcmp(key, "options.allow-ff"))
- opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
+ opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO;
Since we're on nits, we try to wrap at 80 characters.
Thanks, I didn't know what the exact cutoff was.
quoted
+test_expect_success "rebase: impossible fast-forward rebase" '
+ test_config rebase.autostash true &&
+ git reset --hard &&
+ echo dirty >>file1 &&
+ (git rebase --ff-only unrelated-onto-branch || true) &&
Never "||" git commands, better as test_might_fail. We don't want to
hide segfaults.
Also thanks for the advice here.
On Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano [off-list ref] wrote:
Phillip Wood [off-list ref] writes:
quoted
Looking at origin/seen:builtin/pull.c we already check if we can
fast-forward and unconditionally merge in that case irrespective of
any '--rebase' option or pull.rebase config. It should be simple for
pull to error out if '--ff-only' is given and we cannot fast-forward.
Excellent.
Even though teaching even more special case on the "git pull" side
makes me feel somewhat dirty, but I think it would be a small price
to pay, and the end result would save an useless fork whose sole
purpose is to make the integration step after fetch fail when "pull"
can easily tell, as you said, that it ought to fail, so overall it
would probably be a net win.
Okay, so it sounds like I should just scrap this patch and try again,
after "pull: cleanup autostash check" is in master, with a patch that
only modifies `git pull` and not `git rebase`. (Unless someone more
experienced wants to take over, which would be fine by me.)
Thanks anyway to Phillip and Ævar: Your feedback will help me write
better patches in the future.
-Alex
On Mon, Jul 05 2021, Alex Henrie wrote:
On Mon, Jul 5, 2021 at 3:36 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
On Sun, Jul 04 2021, Alex Henrie wrote:
quoted
+int error_ff_impossible(void)
+{
+ error(_("Not possible to fast-forward, aborting."));
+ return -1;
+}
Here's one, the idiom is just "return error", i.e it itself returns -1.
That would indeed be simpler; thanks for pointing that out.
quoted
FWIW I'd consider doing:
/* earlier, static scope */
static const char error_ff_impossible = N_("Not possible...");
/* later, function scope */
return error(error_ff_impossible);
It's a small difference, but for translators who use the jump-to-source
while translating not having the indirection helps, and in this case
it's just used 3 times...
Wouldn't jump-to-source take the user to the English text in advice.c
either way? How does putting it in a static variable help?
Yes, sorry. I don't know what I was thinking there. What I said would
only apply if _() was used inline, nevermind.
quoted
quoted
[...]
if (parent && parse_commit(parent) < 0)
/* TRANSLATORS: The first %s will be a "todo" command like@@ -2764,7 +2769,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
else if (!strcmp(key, "options.record-origin"))
opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
else if (!strcmp(key, "options.allow-ff"))
- opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
+ opts->fast_forward = git_config_bool_or_int(key, value, &error_flag) ? FF_ALLOW : FF_NO;
Since we're on nits, we try to wrap at 80 characters.
Thanks, I didn't know what the exact cutoff was.
It's not strictly adhered to, and depending on the code we'll have it be
longer (sometimes much so) if it helps the general readability,
e.g. long repetitive declarations or something. For this sort of thing
it's generally preferred.
On Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano [off-list ref] wrote:
quoted
Phillip Wood [off-list ref] writes:
quoted
Looking at origin/seen:builtin/pull.c we already check if we can
fast-forward and unconditionally merge in that case irrespective of
any '--rebase' option or pull.rebase config. It should be simple for
pull to error out if '--ff-only' is given and we cannot fast-forward.
Excellent.
Even though teaching even more special case on the "git pull" side
makes me feel somewhat dirty, but I think it would be a small price
to pay, and the end result would save an useless fork whose sole
purpose is to make the integration step after fetch fail when "pull"
can easily tell, as you said, that it ought to fail, so overall it
would probably be a net win.
Okay, so it sounds like I should just scrap this patch and try again,
after "pull: cleanup autostash check" is in master, with a patch that
only modifies `git pull` and not `git rebase`. (Unless someone more
experienced wants to take over, which would be fine by me.)
I've just skimmed that whole part of the larger "what should we do"
discussion here, and don't really have an opinion, but I see Phillip
Wood replied to this downthread.
You can grab the branches Junio mentions in What's Cooking from
https://github.com/gitster/git.git, if you'd like to rebase on top of
Felipe's (or from the ML).
And if you fetch from it with:
fetch = +refs/notes/amlog:refs/notes/amlog
And set:
[notes]
displayRef = refs/notes/amlog
You'll get mappings from the mails to the commits Junio applies.
(Note that they're not always 1=1 the same, even after ignoring Junio's
Signed-off-by header, he'll sometimes fix them up as he queues them,
might use a different base for "master" than you did etc.)
Hi Alex
On 05/07/2021 20:48, Alex Henrie wrote:
On Mon, Jul 5, 2021 at 10:50 AM Junio C Hamano [off-list ref] wrote:
quoted
Phillip Wood [off-list ref] writes:
quoted
Looking at origin/seen:builtin/pull.c we already check if we can
fast-forward and unconditionally merge in that case irrespective of
any '--rebase' option or pull.rebase config. It should be simple for
pull to error out if '--ff-only' is given and we cannot fast-forward.
Excellent.
Even though teaching even more special case on the "git pull" side
makes me feel somewhat dirty, but I think it would be a small price
to pay, and the end result would save an useless fork whose sole
purpose is to make the integration step after fetch fail when "pull"
can easily tell, as you said, that it ought to fail, so overall it
would probably be a net win.
Okay, so it sounds like I should just scrap this patch and try again,
after "pull: cleanup autostash check" is in master, with a patch that
only modifies `git pull` and not `git rebase`. (Unless someone more
experienced wants to take over, which would be fine by me.)
I don't think you need to wait, if necessary you could base your patches
on top of that branch. Junio's latest what's cooking email said he was
going to merge that branch into next so it is unlikely to change. In any
case looking at the 'git diff origin/master origin/seen builtin/pull.c'
I suspect the changes required won't conflict with that branch.
Thanks anyway to Phillip and Ævar: Your feedback will help me write
better patches in the future.
You're welcome, I look forward to reading your future patches
Best wishes
Phillip
-Alex