git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Pratik Karki <predatoramigo@gmail.com>
Cc: git <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Alban Gruin <alban.gruin@gmail.com>
Subject: Re: [PATCH 5/5] builtin/rebase: support running "git rebase <upstream>"
Date: Thu, 28 Jun 2018 14:58:56 -0700	[thread overview]
Message-ID: <CAGZ79kZpf3AMqZPraCYp+KNKPo9xex3OBJLAz_foGiSPVswYHw@mail.gmail.com> (raw)
In-Reply-To: <20180628074655.5756-6-predatoramigo@gmail.com>

On Thu, Jun 28, 2018 at 12:48 AM Pratik Karki <predatoramigo@gmail.com> wrote:
>
> 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.

> 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?

> 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.

  reply	other threads:[~2018-06-28 21:59 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-28  7:46 [GSoC] [PATCH 0/5] rebase: rewrite rebase in C Pratik Karki
2018-06-28  7:46 ` [PATCH 1/5] Start TODO-rebase.sh Pratik Karki
2018-06-28  9:17   ` Pratik Karki
2018-06-28  7:46 ` [PATCH 2/5] rebase: start implementing it as a builtin Pratik Karki
2018-06-28  8:08   ` Christian Couder
2018-06-28 18:49   ` Stefan Beller
2018-06-28  7:46 ` [PATCH 3/5] rebase: refactor common shell functions into their own file Pratik Karki
2018-06-28  8:02   ` Christian Couder
2018-06-28 19:17   ` Stefan Beller
2018-06-28  7:46 ` [PATCH 4/5] sequencer: refactor the code to detach HEAD to checkout.c Pratik Karki
2018-06-28  8:14   ` Christian Couder
2018-06-28 21:19   ` Stefan Beller
2018-06-28  7:46 ` [PATCH 5/5] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-06-28 21:58   ` Stefan Beller [this message]
2018-07-02  9:15 ` [GSoC] [PATCH v2 0/4] rebase: rewrite rebase in C Pratik Karki
2018-07-02  9:15   ` [PATCH v2 1/4] rebase: start implementing it as a builtin Pratik Karki
2018-07-03 21:09     ` Junio C Hamano
2018-07-02  9:15   ` [PATCH v2 2/4] rebase: refactor common shell functions into their own file Pratik Karki
2018-07-03 21:13     ` Junio C Hamano
2018-07-02  9:15   ` [PATCH v2 3/4] sequencer: refactor the code to detach HEAD to checkout.c Pratik Karki
2018-07-03 21:26     ` Junio C Hamano
2018-07-02  9:15   ` [PATCH v2 4/4] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-07-03 21:42     ` Junio C Hamano
2018-07-06 12:08 ` [GSoC] [PATCH v3 0/4] rebase: rewrite rebase in C Pratik Karki
2018-07-06 12:08   ` [PATCH v3 1/4] rebase: start implementing it as a builtin Pratik Karki
2018-07-06 21:09     ` Junio C Hamano
2018-07-06 12:08   ` [PATCH v3 2/4] rebase: refactor common shell functions into their own file Pratik Karki
2018-07-06 12:36     ` Johannes Schindelin
2018-07-06 12:08   ` [PATCH v3 3/4] sequencer: refactor the code to detach HEAD to checkout.c Pratik Karki
2018-07-06 12:08   ` [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-07-06 21:30     ` Junio C Hamano
2018-07-07  6:45     ` Christian Couder
2018-07-07 11:59       ` Johannes Schindelin
2018-07-07 16:24       ` Junio C Hamano
2018-07-17 21:49     ` Beat Bolli
2018-07-17 21:55       ` Beat Bolli
2018-07-08 18:01 ` [GSoC] [PATCH v4 0/4] rebase: rewrite rebase in C Pratik Karki
2018-07-08 18:01   ` [PATCH v4 1/4] rebase: start implementing it as a builtin Pratik Karki
2018-07-09  7:59     ` Andrei Rybak
2018-07-09  8:36       ` Eric Sunshine
2018-07-09 17:18         ` Pratik Karki
2018-07-22  9:14     ` Duy Nguyen
2018-07-08 18:01   ` [PATCH v4 2/4] rebase: refactor common shell functions into their own file Pratik Karki
2018-07-08 18:01   ` [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c Pratik Karki
2018-07-08 21:31     ` Johannes Schindelin
2018-07-09 17:23       ` Pratik Karki
2018-07-09 16:42     ` Junio C Hamano
2018-07-09 17:20       ` Pratik Karki
2018-07-08 18:01   ` [PATCH v4 4/4] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-07-08 21:44     ` Johannes Schindelin
2018-07-08 21:14   ` [GSoC] [PATCH v4 0/4] rebase: rewrite rebase in C Johannes Schindelin
2018-07-30 16:29   ` [GSoC] [PATCH v5 0/3] " Pratik Karki
2018-07-30 16:29     ` [PATCH v5 1/3] rebase: start implementing it as a builtin Pratik Karki
2018-07-30 16:29     ` [PATCH v5 2/3] rebase: refactor common shell functions into their own file Pratik Karki
2018-07-30 16:29     ` [PATCH v5 3/3] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-08-01  0:17     ` [GSoC] [PATCH v5 0/3] rebase: rewrite rebase in C Pratik Karki
2018-08-06 19:31     ` [GSoC] [PATCH v6 " Pratik Karki
2018-08-06 19:31       ` [GSoC] [PATCH v6 1/3] rebase: start implementing it as a builtin Pratik Karki
2018-08-06 19:31       ` [GSoC] [PATCH v6 2/3] rebase: refactor common shell functions into their own file Pratik Karki
2018-08-06 19:31       ` [GSoC] [PATCH v6 3/3] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-08-16 21:43         ` Junio C Hamano

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=CAGZ79kZpf3AMqZPraCYp+KNKPo9xex3OBJLAz_foGiSPVswYHw@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=alban.gruin@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=predatoramigo@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).