From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Alex Henrie <alexhenrie24@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, newren@gmail.com
Subject: Re: [PATCH RFC] rebase: respect --ff-only option
Date: Mon, 05 Jul 2021 11:27:16 +0200 [thread overview]
Message-ID: <87a6n16r1l.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20210705044505.666977-1-alexhenrie24@gmail.com>
On Sun, Jul 04 2021, Alex Henrie wrote:
> I'm sending this patch as an RFC because I'm sure someone will find at
> least one thing that needs to be changed before it's committed.
FWIW that's true of most non-RFC patches :)
> +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.
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...
> [...]
> 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.
> +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.
I saw Phillip Wood had some comments on the rest, I didn't test this,
just skimmed it, but generally LGTM from a glance, not getting down to
the nuts of more deeply inspecting the logic.
next prev parent reply other threads:[~2021-07-05 9:36 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-05 4:45 [PATCH RFC] rebase: respect --ff-only option Alex Henrie
2021-07-05 8:53 ` Phillip Wood
2021-07-05 9:58 ` Junio C Hamano
2021-07-05 12:09 ` Felipe Contreras
2021-07-05 13:54 ` Phillip Wood
2021-07-07 0:30 ` Felipe Contreras
2021-07-05 15:29 ` Phillip Wood
2021-07-05 16:50 ` Junio C Hamano
2021-07-05 19:23 ` Junio C Hamano
2021-07-05 19:48 ` Alex Henrie
2021-07-06 13:52 ` Phillip Wood
2021-07-06 14:43 ` Ævar Arnfjörð Bjarmason
2021-07-07 1:13 ` Felipe Contreras
2021-07-05 9:27 ` Ævar Arnfjörð Bjarmason [this message]
2021-07-05 12:00 ` Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a6n16r1l.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=alexhenrie24@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).