Re: [PATCH 5/5] builtin/rebase: support running "git rebase <upstream>"
- From
Stefan Beller <sbeller@google.com>
- Date
- Jun 28, 2018, 21:58 UTC
- Message-ID
- <CAGZ79kZpf3AMqZPraCYp+KNKPo9xex3OBJLAz_foGiSPVswYHw@mail.gmail.com>
- In-Reply-To
- <20180628074655.5756-6-predatoramigo@gmail.com>
On Thu, Jun 28, 2018 at 12:48 AM Pratik Karki <predatoramigo@gmail.com> wrote:
Show 5 quoted lines
> > This patch gives life to the skeleton added in the previous patch. > This patch makes real operation happen i.e. by using > `git -c rebase.usebuiltin=true rebase <upstream>`. > With this patch, the basic operation of rebase can be done.
Would it make sense to add this config option to some basic test in the test suite to show off one case in there? (Otherwise it is hard to keep this code correct for the future (even if it is just a few days/weeks) as other series on the list may collide with it in subtle ways, so a test would be fast signal to catch these subtleties).
Maybe setting this in one of the early tests in t3400 would be good?
> These backends use Unix shell functions defined both by git-sh-setup.sh > and git-rebase.sh (we move the latter's into git-rebase--common.sh to
s/move/moved in a previous patch/ ? But then again we already know about the earlier patch, I am on the fence whether this is worth mentioning. But it sure is fine to leave it here.
Show 5 quoted lines
> accommodate for that), so we not only have to source the backend file > before calling the respective Unix shell script function, but we have > to source git-sh-setup and git-rebase--common before that. > And since this is all done in a Unix shell script snippet, all of this > is in argv[0]. There never will be a non-NULL argv[1].
No double negatives are never harder to read than simple forms. ;) So you are saying, there are no further arguments to that shell invocation?
Show 48 quoted lines
> This patch does the *bare* minimum to get `git rebase <upstream>` to
> work: there is still no option parsing, and only the bare minimum set
> of environment variables are set (in particular, the current revision
> would be susceptible to bugs where e.g. `rebase_root` could be set by
> mistake before running `git rebase` and the `git-rebase--am` backend
> would pick up that variable and use it).
>
> It still calls original `git-legacy-rebase.sh` unless the config
> setting rebase.useBuiltin is set to true. This patch uses the
> detach_head_to() function from checkout.c introduced by a previous
> commit to perform initial checkout.
>
> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
> builtin/rebase.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 229 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1152b7229..2f90389c2 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -9,6 +9,19 @@
> #include "exec-cmd.h"
> #include "argv-array.h"
> #include "dir.h"
> +#include "packfile.h"
> +#include "checkout.h"
> +#include "refs.h"
> +
> +static GIT_PATH_FUNC(apply_dir, "rebase-apply");
> +static GIT_PATH_FUNC(merge_dir, "rebase-merge");
> +
> +enum rebase_type {
> + REBASE_AM,
> + REBASE_MERGE,
> + REBASE_INTERACTIVE,
> + REBASE_PRESERVE_MERGES
> +};
>
> static int use_builtin_rebase(void)
> {
> @@ -28,8 +41,129 @@ static int use_builtin_rebase(void)
> return ret;
> }
>
> +static int apply_autostash(void)
> +{
> + warning("TODO");This comes up unconditionally here, so the automated testing idea from above might not be as good as I thought after all.
> +static struct commit *peel_committish(const char *name)
The -ish suffix is to indicate that a wide range of notations that describe commits are accepted. Another way of naming this function would be by its output, i.e. peel_to_commit, the name similar to peel_to_type. But I guess emphasizing the input to be anything that describes a commit is also important here, as we pass in the arguments eventually provided by users (e.g. "master^^") so this name sounds fine; I cannot think of a better suggestion for now.