Re-implement git add -i as built-in

[dscho]

This is an outline for a feature, rather than a bug report.

One of the relatively few remaining scripted parts (after turning rebase, rebase -i and stash into built-in commands, which are still experimental but should replace the scripted versions before long), the next one in line seems to be git-add--interactive.perl.

Here is a head-start: https://public-inbox.org/git/1494907234-28903-1-git-send-email-bnmvco%40gmail.com/t

I shall fill in more details as I get around to analyze the source code and come up with a good project plan.

[dscho]

Okay, here goes a somewhat unstructured analysis toward coming up with a timeline.

First things first: the most successful strategy to convert a script to a built-in appears to introduce a built-in helper first, extracting functionality from the script and then calling the helper from the script.

A good example to follow is 3d5ec65. It introduces a new helper, written in C, and we should do the same, following the same naming convention git-stash--helper: we should call our helper git-add--helper. I would even recommend introducing the helper without any functionality first, just to get those changes out of the way: new file builtin/add--helper.c, then the associated changes in Makefile, builtin.h, git.c and .gitignore.

Now let's look at https://github.com/git/git/blob/v2.19.1/git-add--interactive.perl. There is already some logical code flow structuring by grace of having functions (or "subroutines", as Perl calls them, memorialized via the keyword sub). Let's see whether those functions give us a good structure for the conversion to follow.

The first function is called run_cmd_pipe and is clearly too low-level to be moved to a built-in. Same for refresh. And maybe also for list_untracked.

is_initial_commit is used more than once, but let's see whether the first user, get_diff_reference is a good candidate for a first code move to the built-in helper. It clearly needs get_empty_tree, which is also used in diff_cmd. But yes, it looks as if that would make for a fine start.

Since it is the first real code move, I would probably give this 1-3 days to get right.

list_modified looks like a logical next task. This is a bit complex a function, and it does a lot of text processing of the output of diff-index and diff-files. That would have to be done differently in a built-in, of course, as it would have to call the underlying APIs instead and produce different output than diff-index and diff-files.

I think I would give this a good 2 weeks to get right.

The find_unique function looks like it operates on Perl data structures, which would be hard to convert to C individually, as you would have to de-serialize the data structures into a command-line (or into a stream that is fed to the stdin of the built-in command). So I think it has to be done as part of moving the code of the only user, list_and_choose.

Same goes, unfortunately, for find_unique_prefixes.

And as update_trie is only used by find_unique_prefixes, the same goes for that function.

list_and_choose is also the only caller of highlight_prefixes, which in turn is the only user of is_valid_prefix. And it is a function that outputs color, i.e. it has to have a notion whether this command really is run connected to an interactive terminal (as opposed to connected to another process via pipes).

So list_and_choose and its callees are the next task, and it is big. 3-4 weeks, I would guesstimate, maybe even more.

I think this is the big one, though. There are a couple of *_cmd commands after that, which seem to basically rely on list_and_choose and then do only minimal things on top. Those functions would be the next tasks. Probably a couple of days, each, revert_cmd maybe a bit more (judging by its size).

Then there are a lot of patch parsing/reassembling functions up until (and including) reassemble_patch, and they seem to pass around Perl arrays. I think that this is basically all needed for edit_hunk_manually, but I have not analyzed it all that well yet.

prompt_single_character will be tricky; I do not think there is a POSIX way to do that, so we will have to figure something out that is not too un-portable.

This concludes the timeline so far, as far as my best guess of a realistic one goes, and we'll have to take it from there, when we reach that point.

[dscho]

Oh, and regression tests! There is t/t3701-add-interactive.sh already. As we convert one thing after another, we should probably make sure that t3701 covers what we convert, and if it does not, introduce a test case first, verify that it works with the Perl script version of git add -i and only then convert it, because that way, we can debug more or less efficiently when our conversion introduced a regression.

[dscho]

This was started by @slavicaDj

[dscho]

Progress can be seen at gitgitgadget#103

[dscho]

Actually, progress can be seen at gitgitgadget#170, gitgitgadget#171, gitgitgadget#172, gitgitgadget#173, gitgitgadget#174 and , gitgitgadget#175. Let's continue there.