[v12,00/26] Convert "git stash" to C builtin

Message

Paul-Sebastian Ungureanu Dec. 20, 2018, 7:44 p.m. UTC

Hello,

This is a new iteration of git-stash which also takes
sd/stash-wo-user-name into account. I cherry-picked
some of dscho's commits (from [1]) to keep the scripted
version of `git stash` as `git-legacy-stash`.

Thank you for your suggestions!

Best,
Paul

[1]
https://github.com/dscho/git

Joel Teichroeb (5):
  stash: improve option parsing test coverage
  stash: convert apply to builtin
  stash: convert drop and clear to builtin
  stash: convert branch to builtin
  stash: convert pop to builtin

Johannes Schindelin (4):
  ident: add the ability to provide a "fallback identity"
  stash: add back the original, scripted `git stash`
  stash: optionally use the scripted version again
  tests: add a special setup where stash.useBuiltin is off

Paul-Sebastian Ungureanu (17):
  sha1-name.c: add `get_oidf()` which acts like `get_oid()`
  strbuf.c: add `strbuf_join_argv()`
  strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`
  t3903: modernize style
  stash: rename test cases to be more descriptive
  stash: add tests for `git stash show` config
  stash: mention options in `show` synopsis
  stash: convert list to builtin
  stash: convert show to builtin
  stash: convert store to builtin
  stash: convert create to builtin
  stash: convert push to builtin
  stash: make push -q quiet
  stash: convert save to builtin
  stash: optimize `get_untracked_files()` and `check_changes()`
  stash: replace all `write-tree` child processes with API calls
  stash: convert `stash--helper.c` into `stash.c`

 .gitignore                          |    1 +
 Documentation/git-stash.txt         |    4 +-
 Makefile                            |    3 +-
 builtin.h                           |    1 +
 builtin/stash.c                     | 1636 +++++++++++++++++++++++++++
 cache.h                             |    2 +
 git-stash.sh => git-legacy-stash.sh |   34 +-
 git-sh-setup.sh                     |    1 +
 git.c                               |    6 +
 ident.c                             |   20 +
 sha1-name.c                         |   19 +
 strbuf.c                            |   51 +
 strbuf.h                            |   16 +
 t/README                            |    4 +
 t/t3903-stash.sh                    |  192 ++--
 t/t3907-stash-show-config.sh        |   83 ++
 16 files changed, 2001 insertions(+), 72 deletions(-)
 create mode 100644 builtin/stash.c
 rename git-stash.sh => git-legacy-stash.sh (97%)
 create mode 100755 t/t3907-stash-show-config.sh

Comments

Junio C Hamano Jan. 3, 2019, 11:39 p.m. UTC | #1

Paul-Sebastian Ungureanu <[e-mail]> writes:

> This is a new iteration of git-stash which also takes
> sd/stash-wo-user-name into account. I cherry-picked
> some of dscho's commits (from [1]) to keep the scripted
> version of `git stash` as `git-legacy-stash`.

I took a brief look and left a comment on 04/26 last year.  I had
some time blocked for this topic today to take another look at the
whole series again.  Thanks for working on this.

It seems that the last three or so steps are new, relative to the
previous round.  I made sure that what is added back at step 24
exactly matches the result of merging sd/stash-wo-user-name into the
current 'master', but such a manual validation is error prone.  Is
it possible to avoid "remove the scripted one prematurely at step
23, and then add it back as 'oops, that was wrong' fix at step 24"?
That would have been much more robust approach.

Thanks.

Johannes Schindelin Jan. 18, 2019, 12:06 p.m. UTC | #2

Hi Junio,

On Thu, 3 Jan 2019, Junio C Hamano wrote:

> Paul-Sebastian Ungureanu <[e-mail]> writes:
> 
> > This is a new iteration of git-stash which also takes
> > sd/stash-wo-user-name into account. I cherry-picked
> > some of dscho's commits (from [1]) to keep the scripted
> > version of `git stash` as `git-legacy-stash`.
> 
> I took a brief look and left a comment on 04/26 last year.  I had
> some time blocked for this topic today to take another look at the
> whole series again.  Thanks for working on this.
> 
> It seems that the last three or so steps are new, relative to the
> previous round.  I made sure that what is added back at step 24
> exactly matches the result of merging sd/stash-wo-user-name into the
> current 'master', but such a manual validation is error prone.  Is
> it possible to avoid "remove the scripted one prematurely at step
> 23, and then add it back as 'oops, that was wrong' fix at step 24"?
> That would have been much more robust approach.

Sorry, I should have thought of that. My mistake.

As it is, Thomas verified that they are identical, so should we go forward
with ps/stash-in-c as-is? I'd prefer that...

Ciao,
Dscho

Junio C Hamano Jan. 18, 2019, 5:49 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

> Hi Junio,
>
> On Thu, 3 Jan 2019, Junio C Hamano wrote:
>
>> Paul-Sebastian Ungureanu <[e-mail]> writes:
>> 
>> > This is a new iteration of git-stash which also takes
>> > sd/stash-wo-user-name into account. I cherry-picked
>> > some of dscho's commits (from [1]) to keep the scripted
>> > version of `git stash` as `git-legacy-stash`.
>> 
>> I took a brief look and left a comment on 04/26 last year.  I had
>> some time blocked for this topic today to take another look at the
>> whole series again.  Thanks for working on this.
>> 
>> It seems that the last three or so steps are new, relative to the
>> previous round.  I made sure that what is added back at step 24
>> exactly matches the result of merging sd/stash-wo-user-name into the
>> current 'master', but such a manual validation is error prone.  Is
>> it possible to avoid "remove the scripted one prematurely at step
>> 23, and then add it back as 'oops, that was wrong' fix at step 24"?
>> That would have been much more robust approach.
>
> Sorry, I should have thought of that. My mistake.
>
> As it is, Thomas verified that they are identical, so should we go forward
> with ps/stash-in-c as-is? I'd prefer that...

Yes, before sending the message you are responding to, I made sure
the scripted version added back is identical to the current one, and
also there is no in-flight updates/fixes to the scripted one.

The benefit that would come from a possible reroll to start the
series from the last three patches would be fairly limited.

Such a reorganized series would have allowed investigation of
regressions and bugs during the development comparing the original
and rewritten implementations slightly easier, but experience from
seeing the evolution of these "reimplement in C" topics tells us
that we see major part of the regression fallouts after the series
is declared "feature complete" anyway, so in the long run, the
less-than-ideal organization of the topic does not matter much in
practice.