Re: [PATCH v11 20/22] stash: convert `stash--helper.c` into `stash.c`

On Tue, Nov 27 2018, Johannes Schindelin wrote:

Hi Junio & Paul,

On Mon, 26 Nov 2018, Junio C Hamano wrote:

quoted

Paul-Sebastian Ungureanu [off-list ref] writes:

quoted

The old shell script `git-stash.sh`  was removed and replaced
entirely by `builtin/stash.c`. In order to do that, `create` and
`push` were adapted to work without `stash.sh`. For example, before
this commit, `git stash create` called `git stash--helper create
--message "$*"`. If it called `git stash--helper create "$@"`, then
some of these changes wouldn't have been necessary.

This commit also removes the word `helper` since now stash is
called directly and not by a shell script.

Signed-off-by: Paul-Sebastian Ungureanu <redacted>
---
 .gitignore                           |   1 -
 Makefile                             |   3 +-
 builtin.h                            |   2 +-
 builtin/{stash--helper.c => stash.c} | 157 +++++++++++++++------------
 git-stash.sh                         | 153 --------------------------
 git.c                                |   2 +-
 6 files changed, 92 insertions(+), 226 deletions(-)
 rename builtin/{stash--helper.c => stash.c} (91%)
 delete mode 100755 git-stash.sh

Seeing the recent trouble in "rebase in C" and how keeping the
scripted version as "git legacy-rebase" helped us postpone the
rewritten version without ripping the whole thing out, I wonder if
we can do the same here.

Feel very free to cherry-pick
https://github.com/git-for-windows/git/commit/004da7e7faa36c872868ae938e06594ea1c2f01c
and
https://github.com/git-for-windows/git/commit/cedfcd39f5a4e4beb33e16fa67c4659fd4bdabf6
which is what we carry in Git for Windows.

...and then something similar to 62c23938fa ("tests: add a special setup
where rebase.useBuiltin is off", 2018-11-14) so those of us who're
smoking next for bugs can test both and report if some of the test
setups (odd OS's etc) show a difference in behavior.

I did some of this the last time around, but then I had to e.g. smoke
next against pu, and look at the general fallout there and see what was
due to stash-in-C, it would be much better to have a
GIT_TEST_STASH_USE_BUILTIN.

quoted

Also, the remaining two patches should be done _before_ this step, I
would think.  I can understand it if the reason you have those two
after this step is because you found the opportunity for these
improvements after you wrote this step, but in the larger picture
seen by the end users of the "stash in C" and those developers who
follow the evolution of the code, the logical place for this "now we
have everything in C, we retire the scripted version" step to happen
is at the very end.

Thanks.