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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 27, 2018, 23:40 UTC
Message-ID: <87ftvmytqj.fsf@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1811271438310.41@tvgsbejvaqbjf.bet>

On Tue, Nov 27 2018, Johannes Schindelin wrote:

Show 38 quoted lines

> Hi Junio & Paul,
>
> On Mon, 26 Nov 2018, Junio C Hamano wrote:
>
>> Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:
>>
>> > 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 <ungureanupaulsebastian@gmail.com>
>> > ---
>> >  .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.

Show 11 quoted lines

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