Pratik Karki <[email protected]> writes:
> This commit imitates the strategy that was used to convert the
> difftool to a builtin, see be8a90e (difftool: add a skeleton for the
> upcoming builtin, 2017-01-17) for details: This commit renames the
> shell script `git-rebase.sh` to `git-legacy-rebase.sh` and hands off to
> it by default.
I'd appreciate it if mentors can teach a bit more log message
writing to students, thanks.
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> new file mode 100644
> index 000000000..c0c9d6cd2
> --- /dev/null
> +++ b/builtin/rebase.c
> @@ -0,0 +1,55 @@
> +/*
> + * "git rebase" builtin command
Nice ;-)
> + * Copyright (c) 2018 Pratik Karki
> + */
> +
> +#include "builtin.h"
> +#include "run-command.h"
> +#include "exec-cmd.h"
> +#include "argv-array.h"
> +#include "dir.h"
> +
> +static int use_builtin_rebase(void)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf out = STRBUF_INIT;
> + int ret;
> +
> + argv_array_pushl(&cp.args,
> + "config", "--bool", "rebase.usebuiltin", NULL);
> + cp.git_cmd = 1;
> + if (capture_command(&cp, &out, 6))
> + return 0;
> +
> + strbuf_trim(&out);
> + ret = !strcmp("true", out.buf);
> + strbuf_release(&out);
> + return ret;
> +}
OK. As we give "--bool", it is perfectly OK to check with "true"
and no other forms of positive setting.
> +int cmd_rebase(int argc, const char **argv, const char *prefix)
> +{
> + /*
> + * NEEDSWORK: Once the builtin rebase has been tested enough
> + * and git-legacy-rebase.sh is retired to contrib/, this preamble
Align these asterisks, i.e.
/*
* NEEDSWORK: ...
*/
> + if (!use_builtin_rebase()) {
> + const char *path = mkpath("%s/git-legacy-rebase",
> + git_exec_path());
> +
> + if (sane_execvp(path, (char **)argv) < 0)
> + die_errno("could not exec %s", path);
> +
> + return 0;
Hmph, I know this was inherited from the old commit you modelled
this patch after, but can sane_execvp() ever give us control back
with non-negative value returned? IOW I am wondering if this should
be more like
if (sane_execvp(...) < 0)
die_errno(...);
else
die("sane_execvp() returned???");
Not worth a reroll, but something to think about.
> diff --git a/git-rebase.sh b/git-legacy-rebase.sh
> similarity index 100%
> rename from git-rebase.sh
> rename to git-legacy-rebase.sh
> diff --git a/git.c b/git.c
> index 9dbe6ffaa..bacfefb2d 100644
> --- a/git.c
> +++ b/git.c
> @@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
> { "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
> { "push", cmd_push, RUN_SETUP },
> { "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
> + /*
> + *NEEDSWORK: Until the rebase is independent and needs no redirection
> + to rebase shell script this is kept as is, then should be changed to
> + RUN_SETUP | NEED_WORK_TREE
> + */
Likewise.
> + { "rebase", cmd_rebase },
> { "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
> { "receive-pack", cmd_receive_pack },
> { "reflog", cmd_reflog, RUN_SETUP },
