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:
>
>> Paul-Sebastian Ungureanu 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
>> > ---
>> > .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.
>> 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.
>>
Re: [PATCH v11 20/22] stash: convert `stash--helper.c` into `stash.c`
Hi Junio & Paul,
On Mon, 26 Nov 2018, Junio C Hamano wrote:
> Paul-Sebastian Ungureanu 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
> > ---
> > .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.
Ciao,
Dscho
> 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.
>
Re: [PATCH v11 20/22] stash: convert `stash--helper.c` into `stash.c`
Paul-Sebastian Ungureanu 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
> ---
> .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.
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.
[PATCH v11 20/22] stash: convert `stash--helper.c` into `stash.c`
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
---
.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
diff --git a/.gitignore b/.gitignore
index 6ecab90ab2..0d77ea5894 100644
--- a/.gitignore
+++ b/.gitignore
@@ -162,7 +162,6 @@
/git-show-ref
/git-stage
/git-stash
-/git-stash--helper
/git-status
/git-stripspace
/git-submodule
diff --git a/Makefile b/Makefile
index aa83545e94..450936fcaf 100644
--- a/Makefile
+++ b/Makefile
@@ -619,7 +619,6 @@ SCRIPT_SH += git-quiltimport.sh
SCRIPT_SH += git-legacy-rebase.sh
SCRIPT_SH += git-remote-testgit.sh
SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
SCRIPT_SH += git-submodule.sh
SCRIPT_SH += git-web--browse.sh
@@ -1115,7 +1114,7 @@ BUILTIN_OBJS += builtin/shortlog.o
BUILTIN_OBJS += builtin/show-branch.o
BUILTIN_OBJS += builtin/show-index.o
BUILTIN_OBJS += builtin/show-ref.o
-BUILTIN_OBJS += builtin/stash--helper.o
+BUILTIN_OBJS += builtin/stash.o
BUILTIN_OBJS += builtin/stripspace.o
BUILTIN_OBJS += builtin/submodule--helper.o
BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index ff4460aff7..b78ab6e30b 100644
--- a/builtin.h
+++ b/builtin.h
@@ -225,7 +225,7 @@ extern int cmd_show(int argc, const char **argv, const char
*prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
extern int cmd_show_index(int argc, const char **argv, const char *prefix);
extern int cmd_status(int argc, const char **argv, const char *prefix);
-extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
extern int cmd_submodule__helper(int argc, const char **argv, const char
*prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash.c
similarity index 91%
rename from builtin/stash--helper.c
rename to builtin/stash.c
index 47a0ab6669..c76a1936d5 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash.c
@@ -16,75 +16,70 @@
#define INCLUDE_ALL_FILES 2
-static const char * const git_stash_helper_usage[] = {
- N_("git stash--helper list []"),
- N_("git stash--helper show [] []"),
- N_("git stash--helper drop [-q|--quiet] []"),
- N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
- N_("git stash--helper branch []"),
- N_("git stash--helper clear"),
- N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+static const char * const git_stash_usage[] = {
+ N_("git stash list []"),
+ N_("git stash show [] []"),
+ N_("git stash drop [-q|--quiet] []"),
+ N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+ N_("git stash branch []"),
+ N_("git stash clear"),
+ N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
" [--] [...]]"),
- N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] []"),
NULL
};
-static const char * const git_stash_helper_list_usage[] = {
- N_("git stash--helper list []"),
+static const char * const git_stash_list_usage[] = {
+ N_("git stash list []"),
NULL
};
-static const char * const git_stash_helper_show_usage[] = {
- N_("git stash--helper show [] []"),
+static const char * const git_stash_show_usage[] = {
+ N_("git stash show [] []"),
NULL
};
-static const char * const git_stash_helper_drop_usage[] = {
- N_("git stash--helper drop [-q|--quiet] []"),
+static const char * const git_stash_drop_usage[] = {
+ N_("git stash drop [-q|--quiet] []"),
NULL
};
-static const char * const git_stash_helper_pop_usage[] = {
- N_("git stash--helper pop [--index]
