New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rebase: minimal skeleton builtin #497

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@prertik
Copy link
Contributor

prertik commented May 23, 2018

To rewrite git-rebase.sh into a C builtin, a minimal implement of skeleton or
framework which holds the basic usage and creates a base for further
modification. This allows us to keep on depending upon the git-rebase.sh until
builtin/rebase2.c is ready.

This skeletal will be ultimately be re-written to further accommodate the
crucial rebase features and with the development of this builtin the work of
rebase--helper gets obsolete with every commit.

Signed-off-by: Pratik Karki predatoramigo@gmail.com

@prertik

This comment has been minimized.

Copy link
Contributor Author

prertik commented May 23, 2018

Hi @chriscool. What is your opinion on the follow-up mail by @dscho? You're my direct mentor and if you are opinionated on rebase--helper2 then I will gladly change rebase2 to rebase--helper2. I know in my proposal I had mentioned that I will work on --helper then gradually develop it. But I followed this approach because I found it feasible. @stefanbeller, could you please take some time off from your busy schedule to follow the mail thread and give your opinion too?

@dscho

This comment has been minimized.

Copy link
Member

dscho commented May 24, 2018

It does not really make sense to call it rebase--helper2: first of all, the *--helper commands are not end-user facing, while rebase2 is. Second, they are all called *--helper, and none of them is *--helper2.

After sleeping over this, however, I am even convinced that we have to not only go the non--helper route, but actually imitate the way I did it with the difftool in be8a90e:

  • rename git-rebase.sh to git-legacy-rebase.sh,
  • adjust all the files referencing git-rebase, such as .gitignore, Makefile etc,
  • add the minimal draft of the rebase builtin that for now calls use_builtin_rebase(void) to determine whether to hand off to git legacy-rebase or not.

This has the rather dramatic advantage that you can switch the default from legacy to non-legacy and very quickly encounter bugs by running the test suite.

@prertik

This comment has been minimized.

Copy link
Contributor Author

prertik commented May 25, 2018

This is my second calling to @stefanbeller to give his opinion on all this. @chriscool and @dscho, can we come to a generally accepted technique so I can progress further? I'm getting really really confused. Both of your techniques are good for me.

My goal is to get builtin rebase running in this GSoC cycle and on the newer Git release I want this to be the default.

@dscho

This comment has been minimized.

Copy link
Member

dscho commented May 25, 2018

I am sorry for the confusion!

Re: how to proceed: I have a rather strong opinion about going the same route as difftool now, because you basically face the same problems as I did when I converted that command: you are converting from top-down, as your main project is to convert the option parsing. And it would not make sense at all to move that to a helper, as you would have to call that helper from the script (helpers are only called from scripts, they are implementation details that the user never should need to care about), transferring all relevant information to the helper, then somehow parsing the output of the helper back into shell data structures. And all of that would be done completely in vain because at the end of the project, this code would become obsolete again.

Besides, changing the --helper would necessarily cause tons of conflicts with the fellow GSoC project by @agrn. We should very much avoid that.

So I am rather strongly suggesting that @chriscool should agree with me that the --helper approach is not appropriate here.

Instead, let's imitate be8a90e here: rename git-rebase.sh to git-legacy-rebase.sh, and introduce a builtin builtin/rebase.c that inspects a config option (opt-in) to hand off to the legacy rebase. When that option is turned off explicitly, we continue with the builtin rebase.

Then we can build up the interactive rebase incrementally: the first step could even be to implement essentially these shell commands: . git-rebase--am && git_rebase_am. Then, we would implement enough of the command-line parsing to support git rebase <base>. Then, we should pass in enough of the environment variables to the shell script snippet to make this work.

In general, that would be wrong, of course, but would it not be satisfying to see the first draft of the builtin rebase to perform its duties correctly?

And from there, we can add all of the functionality, one by one, until it all works. And we can even have a running statistic how many of the test cases in Git's test suite still fail with the builtin rebase, to gage how far along we are.

@chriscool

This comment has been minimized.

Copy link
Contributor

chriscool commented May 25, 2018

So I am rather strongly suggesting that @chriscool should agree with me that the --helper approach is not appropriate here.

Ok I agree with you now.

@prertik

This comment has been minimized.

Copy link
Contributor Author

prertik commented May 25, 2018

@dscho & @chriscool, glad we came to an understanding. I'm very happy now and I believe the way @dscho guided is the best way. Adding @stefanbeller to fill up his notifications on this pull request. 😉
This will perfectly demonstrate how glad I am.

@prertik

This comment has been minimized.

Copy link
Contributor Author

prertik commented May 25, 2018

@sea51930 I don't understand... 😕

@stefanbeller

This comment has been minimized.

Copy link
Contributor

stefanbeller commented May 25, 2018

@prertik
Sorry for chiming in late here, I was AFK for a while and then had so many loose ends to look at. I should have announced that prior to that.

@dscho wrote:

Instead, let's imitate be8a90e here: rename git-rebase.sh to git-legacy-rebase.sh, and introduce a builtin builtin/rebase.c that inspects a config option (opt-in) to hand off to the legacy rebase. When that option is turned off explicitly, we continue with the builtin rebase.

I think that is a good way to go, as it has proven to be successful. Another (although slightly different) example for the "skeleton + add features on top" is found in the series that was merged in 7aa2da6, so the skeleton starts at 73c2779, and then eventually 783d7e8 removed the shell code. (That series was also a GSoC student)

@dscho

This comment has been minimized.

Copy link
Member

dscho commented May 28, 2018

@stefanbeller Yep, that's essentially what I imitated in difftool. I chose to go for a config setting instead of an environment variable, though, as it made it easier for me to let Git for Windows' users opt-in to use the builtin difftool early (and report bugs and make it robust).

@prertik prertik force-pushed the prertik:builtin-rebase branch from f8d3791 to 41531d0 May 28, 2018

@dscho
Copy link
Member

dscho left a comment

Already pretty nice!

* "git rebase" builtin command
*
* Copyright (c) 2018 Pratik Karki
*

This comment has been minimized.

@dscho

dscho May 28, 2018

Member

Please remove this (almost) empty line.

static struct option rebase_options[] = {
OPT_END()
};
*/

This comment has been minimized.

@dscho

dscho May 28, 2018

Member

Please remove these lines from this commit, and add them as a stand-alone commit (which should then grow into the first commit adding support for a first option).

OPT_END()
};
*/
static int use_builtin_rebase(void) {

This comment has been minimized.

@dscho

dscho May 28, 2018

Member

I am really sorry that we do not have automated code formatter rules that would make this here comment unnecessary. You can blame those who think that truly elegant code requires tons of time touching up the formatting manually.

Anyway. In Git's source code, function bodies' starting curly bracket is always on its own line (that is different to the convention to start code blocks on the same line as the for, if, while, etc).


return 0;
}
prefix = setup_git_directory();

This comment has been minimized.

@dscho

dscho May 28, 2018

Member

I vaguely remember that I also put a /* NEEDSWORK */ comment above this part (which will be obsolete once we can adjust the rebase entry in git.c to have the RUN_SETUP | NEED_WORK_TREE flags).

trace_repo_setup(prefix);
setup_work_tree();

die("NEEDS WORK");

This comment has been minimized.

@dscho

dscho May 28, 2018

Member

That's okay, but I think it would be better to use either "TODO" or "TBD" (for "To Be Done") here.

Now, let's plan for the next steps because we are really close to the exciting part! I am fairly certain that you can already implement enough code to run git rebase <base> in a relatively small second commit: As I mentioned a couple of times via mail and IRC (i.e. too far from the code to be useful), what we need to do here is essentially emulate https://github.com/prertik/git/blob/41531d0ba88cfe20e870d57ee437d0fcd7d05064/git-legacy-rebase.sh#L203-L222 which kind of boils down to

. git-rebase--$type
git_rebase__$type${preserve_merges:+__preserve_merges}

This is very simple shell scripting that we can call ourselves very easily by imitating what rebase -i's exec command does: https://github.com/prertik/git/blob/41531d0ba88cfe20e870d57ee437d0fcd7d05064/sequencer.c#L2501-L2509

Note: I chose that exec example because it not only demonstrates how to execute a Unix shell script snippet, but also how to pass environment variables to it (which you will need to do later, quite a few variables even, because the git-rebase--<type> backends have to consume pretty much all of the parsed command-line options in the form of shell variables.

Please do take the time, though, to implement this environment variable passing using argv_array_push() and argv_array_pushf(), as demonstrated e.g. here: https://github.com/prertik/git/blob/41531d0ba88cfe20e870d57ee437d0fcd7d05064/column.c#L377-L394

By the way, it's not quite the truth that we can get away with implementing code to call just those two Unix shell commands: there is a function finish_rebase() which also needs to be implemented. However, in the first run, it does not need to handle the autostash. And the rm -f command corresponds to the POSIX function unlink(), the rm -rf to remove_dir_recursively(), and the most concise example how to run git gc --auto is in builtin/merge.c.

To support git -c rebase.useBuiltin=true rebase <base>, I think that a very simple "option parsing" would be enough: if (argc != 2) die("Usage: %s <base>", argv[0]);.

The autostash part can be handled in a later patch, and should definitely leverage the apply_autostash() function that was introduced to support finishing interactive rebases in the sequencer; to make that work with the type=am mode, you will want to use get_dir() and teach it about the rebase-apply directory, and you will want to make apply_autostash() non-static (but keep get_dir() static).

I really think that we are very close to having something that is both exciting ("It can run a (super-simple) rebase already!") as well as helpful, as it leads the way to using the t34*.sh suite of tests to assess how far along this project is. You could, for example, append the line git config rebase.useBuiltin true to the body of the test_create_repo function in t/test-lib-functions.sh, and then run make -C t t3400-rebase.sh to see whether any test case succeeds ;-)

argv_array_pushl(&cp.args,
"config", "--bool", "rebase.usebuiltin", NULL);
cp.git_cmd = 1;
if (capture_command(&cp, &out, 28))

This comment has been minimized.

@chriscool

chriscool May 30, 2018

Contributor

I wonder if outshould be released before returning in case capture_command() failed.

Also I wonder why 28 is used.

This comment has been minimized.

@prertik

prertik May 30, 2018

Author Contributor

Oh no... At first, I was trying to hardcode options provided by rebase into the builtin. There were about 28 options provided in rebase. But, that was not the approach to do it and I failed to do so. I think I mistakenly left 28 there.

rebase: add a skeleton for the upcoming builtin
To rewrite git-rebase.sh into a C builtin, a minimal implement of skeleton or
framework which currently, falls back to the legacy rebase version which has
been renamed to `legacy-rebase`.

This approach follows the idea behind builtin difftool done by
Johannes Schindelin. Currently, the working way to be to set git config to
use the config variable rebase.useBuiltin set to true.

The flag will be used further to test builtin features until rebase can be
free from legacy-rebase.

Signed-off-by: Pratik Karki <predatoramigo@gmail.com>

@prertik prertik force-pushed the prertik:builtin-rebase branch from 41531d0 to 8ffd77e Jun 2, 2018

@sea51930

This comment has been minimized.

Copy link

sea51930 commented Dec 1, 2018

@prertik prertik closed this Dec 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment