Re: [GSoC][PATCH v4 15/20] rebase -i: rewrite write_basic_state() in C
On Mon, Jul 30, 2018 at 08:25:16PM +0200, SZEDER Gábor wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index 1c035ceec7..d257903db0 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
>
> > +int write_basic_state(struct replay_opts *opts, const char *head_name,
> > + const char *onto, const char *orig_head)
> > +{
> > + const char *quiet = getenv("GIT_QUIET");
> > +
> > + if (head_name)
> > + write_file(rebase_path_head_name(), "%s\n", head_name);
> > + if (onto)
> > + write_file(rebase_path_onto(), "%s\n", onto);
> > + if (orig_head)
> > + write_file(rebase_path_orig_head(), "%s\n", orig_head);
> > +
> > + if (quiet)
> > + write_file(rebase_path_quiet(), "%s\n", quiet);
> > + else
> > + write_file(rebase_path_quiet(), "");
>
> This is not a faithful conversion of the original. git-rebase.sh writes
> this 'quiet' file with:
>
> echo "$GIT_QUIET" > "$state_dir"/quiet
>
> which means that a single newline character was written even when
> $GIT_QUIET was unset/empty.
write_file() will call strbuf_complete_line(), so even passing "" will
result in a file with a newline in it (I didn't dig, but this comes from
e7ffa38c in 2015, so it may well have been a response to the breakage
you were thinking of).
So actually all of the "%s\n" here can be just "%s".
But there _is_ a reason not to use "", which is that it triggers
-Wformat-zero-length (which is part of -Wall unless you explicitly turn
it off, which our DEVELOPER=1 setup does). For a long time you _had_ to
do that, but these days we're actually clean with respect to that
warning.
So using "\n" here is better, and likewise here:
> > + if (opts->verbose)
> > + write_file(rebase_path_verbose(), "");
Unless we really do want a zero-length file, in which case:
write_file_buf(path, "", 0);
is the right option. That matches the shell version, but I'd be
surprised if it mattered, since it's clearly meant to be "if this file
exists, we're verbose".
-Peff
Re: [GSoC][PATCH v4 15/20] rebase -i: rewrite write_basic_state() in C
Hi,
Le 30/07/2018 à 20:25, SZEDER Gábor a écrit :
>> diff --git a/sequencer.c b/sequencer.c
>> index 1c035ceec7..d257903db0 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>
>> +int write_basic_state(struct replay_opts *opts, const char *head_name,
>> + const char *onto, const char *orig_head)
>> +{
>> +const char *quiet = getenv("GIT_QUIET");
>> +
>> +if (head_name)
>> +write_file(rebase_path_head_name(), "%s\n", head_name);
>> +if (onto)
>> +write_file(rebase_path_onto(), "%s\n", onto);
>> +if (orig_head)
>> +write_file(rebase_path_orig_head(), "%s\n", orig_head);
>> +
>> +if (quiet)
>> +write_file(rebase_path_quiet(), "%s\n", quiet);
>> +else
>> +write_file(rebase_path_quiet(), "");
>
> This is not a faithful conversion of the original. git-rebase.sh writes
> this 'quiet' file with:
>
> echo "$GIT_QUIET" > "$state_dir"/quiet
>
> which means that a single newline character was written even when
> $GIT_QUIET was unset/empty.
>
> I seem to recall a case in the past, when a shell-to-C conversion
> accidentally dropped a newline from a similar state-file, which then
> caused some issues later on. But I don't remember the specifics and a
> quick search didn't turn up anything relevant either...
>
I don’t think it’s a problem here, but we’re better safe than sorry.
I’ll send a fix soon.
Cheers,
Alban
Re: [GSoC][PATCH v4 15/20] rebase -i: rewrite write_basic_state() in C
> diff --git a/sequencer.c b/sequencer.c
> index 1c035ceec7..d257903db0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> +int write_basic_state(struct replay_opts *opts, const char *head_name,
> + const char *onto, const char *orig_head)
> +{
> + const char *quiet = getenv("GIT_QUIET");
> +
> + if (head_name)
> + write_file(rebase_path_head_name(), "%s\n", head_name);
> + if (onto)
> + write_file(rebase_path_onto(), "%s\n", onto);
> + if (orig_head)
> + write_file(rebase_path_orig_head(), "%s\n", orig_head);
> +
> + if (quiet)
> + write_file(rebase_path_quiet(), "%s\n", quiet);
> + else
> + write_file(rebase_path_quiet(), "");
This is not a faithful conversion of the original. git-rebase.sh writes
this 'quiet' file with:
echo "$GIT_QUIET" > "$state_dir"/quiet
which means that a single newline character was written even when
$GIT_QUIET was unset/empty.
I seem to recall a case in the past, when a shell-to-C conversion
accidentally dropped a newline from a similar state-file, which then
caused some issues later on. But I don't remember the specifics and a
quick search didn't turn up anything relevant either...
> +
> + if (opts->verbose)
> + write_file(rebase_path_verbose(), "");
> + if (opts->strategy)
> + write_file(rebase_path_strategy(), "%s\n", opts->strategy);
> + if (opts->xopts_nr > 0)
> + write_strategy_opts(opts);
> +
> + if (opts->allow_rerere_auto == RERERE_AUTOUPDATE)
> + write_file(rebase_path_allow_rerere_autoupdate(),
> "--rerere-autoupdate\n");
> + else if (opts->allow_rerere_auto == RERERE_NOAUTOUPDATE)
> + write_file(rebase_path_allow_rerere_autoupdate(),
> "--no-rerere-autoupdate\n");
> +
> + if (opts->gpg_sign)
> + write_file(rebase_path_gpg_sign_opt(), "-S%s\n",
> opts->gpg_sign);
> + if (opts->signoff)
> + write_file(rebase_path_signoff(), "--signoff\n");
> +
> + return 0;
> +}
> +
> static int walk_revs_populate_todo(struct todo_list *todo_list,
> struct replay_opts *opts)
> {
[GSoC][PATCH v4 15/20] rebase -i: rewrite write_basic_state() in C
This rewrites write_basic_state() from git-rebase.sh in C. This is the
first step in the conversion of init_basic_state(), hence the mode in
rebase--helper.c is called INIT_BASIC_STATE. init_basic_state() will be
converted in the next commit.
The part of read_strategy_opts() that parses the stategy options is
moved to a new function to allow its use in rebase--helper.c.
Finally, the call to write_basic_state() is removed from
git-rebase--interactive.sh, replaced by a call to `--init-basic-state`.
Signed-off-by: Alban Gruin
---
All patches from this one are new.
builtin/rebase--helper.c | 28 +-
git-rebase--interactive.sh | 7 +++-
sequencer.c| 77 --
sequencer.h| 4 ++
4 files changed, 102 insertions(+), 14 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index c4a4c5cfbb..06fe3c018b 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -5,6 +5,8 @@
#include "sequencer.h"
#include "rebase-interactive.h"
#include "argv-array.h"
+#include "rerere.h"
+#include "alias.h"
static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
@@ -53,11 +55,12 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
int abbreviate_commands = 0, rebase_cousins = -1, ret;
const char *head_hash = NULL, *onto = NULL, *restrict_revision = NULL,
- *squash_onto = NULL, *upstream = NULL;
+ *squash_onto = NULL, *upstream = NULL, *head_name = NULL;
+ char *raw_strategies = NULL;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC, EDIT_TODO,
PREPARE_BRANCH,
- COMPLETE_ACTION
+ COMPLETE_ACTION, INIT_BASIC_STATE
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -69,6 +72,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("keep original branch points of cousins")),
OPT_BOOL(0, "autosquash", ,
N_("move commits thas begin with squash!/fixup!")),
+ OPT_BOOL(0, "signoff", , N_("sign commits")),
OPT__VERBOSE(, N_("be verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
@@ -93,6 +97,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("prepare the branch to be rebased"),
PREPARE_BRANCH),
OPT_CMDMODE(0, "complete-action", ,
N_("complete the action"), COMPLETE_ACTION),
+ OPT_CMDMODE(0, "init-basic-state", ,
+ N_("initialise the rebase state"),
INIT_BASIC_STATE),
OPT_STRING(0, "onto", , N_("onto"), N_("onto")),
OPT_STRING(0, "restrict-revision", _revision,
N_("restrict-revision"), N_("restrict revision")),
@@ -100,6 +106,14 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("squash onto")),
OPT_STRING(0, "upstream", , N_("upstream"),
N_("the upstream commit")),
+ OPT_STRING(0, "head-name", _name, N_("head-name"),
N_("head name")),
+ OPT_STRING('S', "gpg-sign", _sign, N_("gpg-sign"),
+ N_("GPG-sign commits")),
+ OPT_STRING(0, "strategy", , N_("strategy"),
+ N_("rebase strategy")),
+ OPT_STRING(0, "strategy-opts", _strategies,
N_("strategy-opts"),
+ N_("strategy options")),
+ OPT_RERERE_AUTOUPDATE(_rerere_auto),
OPT_END()
};
@@ -176,6 +190,16 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
free(shortrevisions);
return !!ret;
}
+ if (command == INIT_BASIC_STATE) {
+ if (raw_strategies)
+ parse_strategy_opts(, raw_strategies);
+
+ ret = get_revision_ranges(upstream, onto, _hash, NULL,
NULL);
+ if (ret)
+ return ret;
+
+ return !!write_basic_state(, head_name, onto, head_hash);
+ }
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 08e9a21c2f..6367da66e2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -57,7 +57,6 @@ init_basic_state () {
rm -f "$(git rev-parse --git-path REBASE_HEAD)"
: > "$state_dir"/interactive || die "$(gettext "Could not mark as
interactive")"
-
