Skip to content
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

Reimplement git subtree as a built-in #1410

Open
dscho opened this issue Nov 8, 2022 · 14 comments
Open

Reimplement git subtree as a built-in #1410

dscho opened this issue Nov 8, 2022 · 14 comments

Comments

@dscho
Copy link
Member

dscho commented Nov 8, 2022

The git subtree command is currently implemented as a shell script and it lives inside contrib/subtree/. There was previously an attempt to move it out of contrib/, but there was the concern of adding yet another scripted command (with all the problems that brings with it).

Using the OPT_SUBCOMMAND infrastructure, this project is about reimplementing the individual subcommands one by one.

In contrast to earlier projects that converted shell scripts to pure C versions, we want to be mindful to avoid too-literal translations. For example, instead of using the --grep= argument of the revision walking machinery, we will want to implement a precise search through the commit messages via strstr() and avoid spawning a new process. And instead of near-duplicating the logic of find_latest_squash and find_existing_splits, there are probably better ways to do this in C, ways that avoid code duplication.

However, just like previous conversions, this project could start by renaming git-subtree.sh to git-subtree--helper.sh while moving it to the top-level, moving the test script to t/ and implementing a builtin/subtree.c that simply shells out to the helper. That way, subcommands can be converted incrementally (and even independently).

@phil-blain
Copy link

I'm a little bit unsure about that idea, given the general lack of interest from mailing list regulars for any bug report in git subtree...

Moving it out of contrib does imply (at least to me) some sort of "support", which I'm not sure the project would be interested in providing.

Just my 2 cents...

@dscho
Copy link
Member Author

dscho commented Nov 9, 2022

@phil-blain the lack of interest might very well be explained by git subtree living in contrib/.

FWIW Git for Windows ships with git subtree for I don't know how long, so there is kind of some support already.

@dscho
Copy link
Member Author

dscho commented Nov 20, 2022

Besides, there was https://lore.kernel.org/git/20180430095044.28492-1-avarab@gmail.com/, so there is interest in getting this command into mainline.

@vinayakdsci
Copy link

Hey! If possible, I would be very happy to work on this issue.

@dscho
Copy link
Member Author

dscho commented Jan 13, 2023

@vinayakdsci excellent! Are you already a little bit familiar with Git's source code? Do you have a preferred development environment? In other words: how can I help?

@vinayakdsci
Copy link

Thanks a lot for replying, @dscho! I have read the Git documentation and developer guide, and the source code thoroughly. Should I introduce myself on the mailing list, too?

@dscho
Copy link
Member Author

dscho commented Jan 13, 2023

Should I introduce myself on the mailing list, too?

You can do that, but it's no requirement.

@dscho
Copy link
Member Author

dscho commented Jan 14, 2023

I have read the Git documentation and developer guide, and the source code thoroughly.

Even more important, I think, would be to look at the commit history where new builtins were added. git log --diff-filter=A builtin/ should be a good start.

@vinayakdsci
Copy link

vinayakdsci commented Jan 14, 2023 via email

@vinayakdsci
Copy link

vinayakdsci commented Jan 17, 2023

Hey @dscho! I have been trying to follow your advice to make progress, but it looks like I am stuck in a mess.
Could you help? I am unable to understand where to place the shell file for the subtree command.
(Once that is done, I'll try to call it through C code placed in builtin/)

@dscho
Copy link
Member Author

dscho commented Jan 18, 2023

I am unable to understand where to place the shell file for the subtree command.
(Once that is done, I'll try to call it through C code placed in builtin/)

The idea is not to move the shell file. The idea is to re-implement what the shell script does, in C...

@dscho
Copy link
Member Author

dscho commented Jan 25, 2023

@vinayakdsci here are a couple of commits that demonstrate what will need to be done for this ticket: git/git@main...dscho:git:turn-subtree-into-a-built-in^

Make no mistake, this is not an easy project. It will require effort and care, it's not something you can wing by merely copying files around and then adding a tiny bit of glue.

@vinayakdsci
Copy link

@dscho okay! I am sorry for the late reply.
The commits are a great starting point. I understand that it is not easy, what I meant was to move the sh file to the top level, and then implement C code incrementally. But these commits are really a good base to start from. Thanks!

@dscho
Copy link
Member Author

dscho commented Jan 29, 2023

In the past, we indeed tried this incremental approach, but it encourages too literal a translation from shell to C, which results in suboptimal code...

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

No branches or pull requests

3 participants