| Message ID | a6692eef2ca7c9d9e4701f087269d537248a4941.1545331726.git.ungureanupaulsebastian@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert "git stash" to C builtin | expand |
On Thu, Dec 20, 2018 at 09:44:34PM +0200, Paul-Sebastian Ungureanu wrote: > Add stash push to the helper. > > Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> This patch causes rare failures in 't3903-stash.sh', I've seen it break for the first time today in a Travis CI build: +echo bar3 +echo bar4 +git add file2 +git stash -k Saved working directory and index state WIP on stashbranch: d3a23d9 alternate second +cat file +cat file2 +test bar,bar4 = bar,bar2 error: last command exited with $?=1 not ok 20 - stash -k Steps to reproduce: $ git checkout -f fa38428f76 HEAD is now at fa38428f76 stash: convert push to builtin # fb7d1e3ac8 (test-lib: add the '--stress' option to run a test # repeatedly under load, 2019-01-05) $ git merge --no-commit fb7d1e3ac8 Automatic merge went well; stopped before committing as requested $ make && cd t <snip> $ ./t3903-stash.sh --stress -r 1,13,14,20 # wait, it tends to fail in <30 repetitions I run stress testing on its parent for over 800 repetitions, no sign of failure yet. > --- > builtin/stash--helper.c | 245 +++++++++++++++++++++++++++++++++++++++- > git-stash.sh | 6 +- > 2 files changed, 245 insertions(+), 6 deletions(-) > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > index 080c2f7aa6..c77f62c895 100644 > --- a/builtin/stash--helper.c > +++ b/builtin/stash--helper.c > @@ -23,6 +23,9 @@ static const char * const git_stash_helper_usage[] = { > N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] [<stash>]"), > N_("git stash--helper branch <branchname> [<stash>]"), > N_("git stash--helper clear"), > + N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" > + " [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n" > + " [--] [<pathspec>...]]"), > NULL > }; > > @@ -71,6 +74,13 @@ static const char * const git_stash_helper_create_usage[] = { > NULL > }; > > +static const char * const git_stash_helper_push_usage[] = { > + N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" > + " [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n" > + " [--] [<pathspec>...]]"), > + NULL > +}; > + > static const char *ref_stash = "refs/stash"; > static struct strbuf stash_index_path = STRBUF_INIT; > > @@ -1092,7 +1102,7 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps) > > static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf, > int include_untracked, int patch_mode, > - struct stash_info *info) > + struct stash_info *info, struct strbuf *patch) > { > int ret = 0; > int flags = 0; > @@ -1105,7 +1115,6 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf, > struct strbuf msg = STRBUF_INIT; > struct strbuf commit_tree_label = STRBUF_INIT; > struct strbuf untracked_files = STRBUF_INIT; > - struct strbuf patch = STRBUF_INIT; > > prepare_fallback_ident("git stash", "git@stash"); > > @@ -1154,7 +1163,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf, > untracked_commit_option = 1; > } > if (patch_mode) { > - ret = stash_patch(info, ps, &patch); > + ret = stash_patch(info, ps, patch); > if (ret < 0) { > fprintf_ln(stderr, _("Cannot save the current " > "worktree state")); > @@ -1225,7 +1234,8 @@ static int create_stash(int argc, const char **argv, const char *prefix) > > memset(&ps, 0, sizeof(ps)); > strbuf_addstr(&stash_msg_buf, stash_msg); > - ret = do_create_stash(ps, &stash_msg_buf, include_untracked, 0, &info); > + ret = do_create_stash(ps, &stash_msg_buf, include_untracked, 0, &info, > + NULL); > > if (!ret) > printf_ln("%s", oid_to_hex(&info.w_commit)); > @@ -1239,6 +1249,231 @@ static int create_stash(int argc, const char **argv, const char *prefix) > return ret < 0; > } > > +static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet, > + int keep_index, int patch_mode, int include_untracked) > +{ > + int ret = 0; > + struct stash_info info; > + struct strbuf patch = STRBUF_INIT; > + struct strbuf stash_msg_buf = STRBUF_INIT; > + > + if (patch_mode && keep_index == -1) > + keep_index = 1; > + > + if (patch_mode && include_untracked) { > + fprintf_ln(stderr, _("Can't use --patch and --include-untracked" > + " or --all at the same time")); > + ret = -1; > + goto done; > + } > + > + read_cache_preload(NULL); > + if (!include_untracked && ps.nr) { > + int i; > + char *ps_matched = xcalloc(ps.nr, 1); > + > + for (i = 0; i < active_nr; i++) > + ce_path_match(&the_index, active_cache[i], &ps, > + ps_matched); > + > + if (report_path_error(ps_matched, &ps, NULL)) { > + fprintf_ln(stderr, _("Did you forget to 'git add'?")); > + ret = -1; > + free(ps_matched); > + goto done; > + } > + free(ps_matched); > + } > + > + if (refresh_cache(REFRESH_QUIET)) { > + ret = -1; > + goto done; > + } > + > + if (!check_changes(ps, include_untracked)) { > + if (!quiet) > + printf_ln(_("No local changes to save")); > + goto done; > + } > + > + if (!reflog_exists(ref_stash) && do_clear_stash()) { > + ret = -1; > + fprintf_ln(stderr, _("Cannot initialize stash")); > + goto done; > + } > + > + if (stash_msg) > + strbuf_addstr(&stash_msg_buf, stash_msg); > + if (do_create_stash(ps, &stash_msg_buf, include_untracked, patch_mode, > + &info, &patch)) { > + ret = -1; > + goto done; > + } > + > + if (do_store_stash(&info.w_commit, stash_msg_buf.buf, 1)) { > + ret = -1; > + fprintf_ln(stderr, _("Cannot save the current status")); > + goto done; > + } > + > + printf_ln(_("Saved working directory and index state %s"), > + stash_msg_buf.buf); > + > + if (!patch_mode) { > + if (include_untracked && !ps.nr) { > + struct child_process cp = CHILD_PROCESS_INIT; > + > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "clean", "--force", > + "--quiet", "-d", NULL); > + if (include_untracked == INCLUDE_ALL_FILES) > + argv_array_push(&cp.args, "-x"); > + if (run_command(&cp)) { > + ret = -1; > + goto done; > + } > + } > + if (ps.nr) { > + struct child_process cp_add = CHILD_PROCESS_INIT; > + struct child_process cp_diff = CHILD_PROCESS_INIT; > + struct child_process cp_apply = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + > + cp_add.git_cmd = 1; > + argv_array_push(&cp_add.args, "add"); > + if (!include_untracked) > + argv_array_push(&cp_add.args, "-u"); > + if (include_untracked == INCLUDE_ALL_FILES) > + argv_array_push(&cp_add.args, "--force"); > + argv_array_push(&cp_add.args, "--"); > + add_pathspecs(&cp_add.args, ps); > + if (run_command(&cp_add)) { > + ret = -1; > + goto done; > + } > + > + cp_diff.git_cmd = 1; > + argv_array_pushl(&cp_diff.args, "diff-index", "-p", > + "--cached", "--binary", "HEAD", "--", > + NULL); > + add_pathspecs(&cp_diff.args, ps); > + if (pipe_command(&cp_diff, NULL, 0, &out, 0, NULL, 0)) { > + ret = -1; > + goto done; > + } > + > + cp_apply.git_cmd = 1; > + argv_array_pushl(&cp_apply.args, "apply", "--index", > + "-R", NULL); > + if (pipe_command(&cp_apply, out.buf, out.len, NULL, 0, > + NULL, 0)) { > + ret = -1; > + goto done; > + } > + } else { > + struct child_process cp = CHILD_PROCESS_INIT; > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "reset", "--hard", "-q", > + NULL); > + if (run_command(&cp)) { > + ret = -1; > + goto done; > + } > + } > + > + if (keep_index == 1 && !is_null_oid(&info.i_tree)) { > + struct child_process cp_ls = CHILD_PROCESS_INIT; > + struct child_process cp_checkout = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + > + if (reset_tree(&info.i_tree, 0, 1)) { > + ret = -1; > + goto done; > + } > + > + cp_ls.git_cmd = 1; > + argv_array_pushl(&cp_ls.args, "ls-files", "-z", > + "--modified", "--", NULL); > + > + add_pathspecs(&cp_ls.args, ps); > + if (pipe_command(&cp_ls, NULL, 0, &out, 0, NULL, 0)) { > + ret = -1; > + goto done; > + } > + > + cp_checkout.git_cmd = 1; > + argv_array_pushl(&cp_checkout.args, "checkout-index", > + "-z", "--force", "--stdin", NULL); > + if (pipe_command(&cp_checkout, out.buf, out.len, NULL, > + 0, NULL, 0)) { > + ret = -1; > + goto done; > + } > + } > + goto done; > + } else { > + struct child_process cp = CHILD_PROCESS_INIT; > + > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "apply", "-R", NULL); > + > + if (pipe_command(&cp, patch.buf, patch.len, NULL, 0, NULL, 0)) { > + fprintf_ln(stderr, _("Cannot remove worktree changes")); > + ret = -1; > + goto done; > + } > + > + if (keep_index < 1) { > + struct child_process cp = CHILD_PROCESS_INIT; > + > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "reset", "-q", "--", NULL); > + add_pathspecs(&cp.args, ps); > + if (run_command(&cp)) { > + ret = -1; > + goto done; > + } > + } > + goto done; > + } > + > +done: > + strbuf_release(&stash_msg_buf); > + return ret; > +} > + > +static int push_stash(int argc, const char **argv, const char *prefix) > +{ > + int keep_index = -1; > + int patch_mode = 0; > + int include_untracked = 0; > + int quiet = 0; > + const char *stash_msg = NULL; > + struct pathspec ps; > + struct option options[] = { > + OPT_BOOL('k', "keep-index", &keep_index, > + N_("keep index")), > + OPT_BOOL('p', "patch", &patch_mode, > + N_("stash in patch mode")), > + OPT__QUIET(&quiet, N_("quiet mode")), > + OPT_BOOL('u', "include-untracked", &include_untracked, > + N_("include untracked files in stash")), > + OPT_SET_INT('a', "all", &include_untracked, > + N_("include ignore files"), 2), > + OPT_STRING('m', "message", &stash_msg, N_("message"), > + N_("stash message")), > + OPT_END() > + }; > + > + argc = parse_options(argc, argv, prefix, options, > + git_stash_helper_push_usage, > + 0); > + > + parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL, prefix, argv); > + return do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode, > + include_untracked); > +} > + > int cmd_stash__helper(int argc, const char **argv, const char *prefix) > { > pid_t pid = getpid(); > @@ -1277,6 +1512,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix) > return !!store_stash(argc, argv, prefix); > else if (!strcmp(argv[0], "create")) > return !!create_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "push")) > + return !!push_stash(argc, argv, prefix); > > usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]), > git_stash_helper_usage, options); > diff --git a/git-stash.sh b/git-stash.sh > index a9b3064ff0..51d7a06601 100755 > --- a/git-stash.sh > +++ b/git-stash.sh > @@ -429,7 +429,8 @@ save) > ;; > push) > shift > - push_stash "$@" > + cd "$START_DIR" > + git stash--helper push "$@" > ;; > apply) > shift > @@ -465,7 +466,8 @@ branch) > *) > case $# in > 0) > - push_stash && > + cd "$START_DIR" > + git stash--helper push && > say "$(gettext "(To restore them type \"git stash apply\")")" > ;; > *) > -- > 2.20.1.441.g764a526393 >
On 02/08, SZEDER Gábor wrote: > On Thu, Dec 20, 2018 at 09:44:34PM +0200, Paul-Sebastian Ungureanu wrote: > > Add stash push to the helper. > > > > Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> > > This patch causes rare failures in 't3903-stash.sh', I've seen it > break for the first time today in a Travis CI build: Thanks for reporting this. I was going to take a look at it, but unfortunately I can't seem to reproduce the issue even with --stress (I let it run for ~1000 repetitions and then aborted it). Which platform did you see/test this on, and which compile options did you use? I went through a failures on https://travis-ci.org/git/git/builds, but couldn't find this particular one. Could you point me at the failed run? > +echo bar3 > +echo bar4 > +git add file2 > +git stash -k > Saved working directory and index state WIP on stashbranch: d3a23d9 alternate second > +cat file > +cat file2 > +test bar,bar4 = bar,bar2 > error: last command exited with $?=1 > not ok 20 - stash -k > > Steps to reproduce: > > $ git checkout -f fa38428f76 > HEAD is now at fa38428f76 stash: convert push to builtin > > # fb7d1e3ac8 (test-lib: add the '--stress' option to run a test > # repeatedly under load, 2019-01-05) > $ git merge --no-commit fb7d1e3ac8 > Automatic merge went well; stopped before committing as requested > $ make && cd t > <snip> > $ ./t3903-stash.sh --stress -r 1,13,14,20 > # wait, it tends to fail in <30 repetitions > > I run stress testing on its parent for over 800 repetitions, no sign > of failure yet.
On Sun, Feb 10, 2019 at 10:17:12PM +0000, Thomas Gummerer wrote: > On 02/08, SZEDER Gábor wrote: > > On Thu, Dec 20, 2018 at 09:44:34PM +0200, Paul-Sebastian Ungureanu wrote: > > > Add stash push to the helper. > > > > > > Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> > > > > This patch causes rare failures in 't3903-stash.sh', I've seen it > > break for the first time today in a Travis CI build: > > Thanks for reporting this. I was going to take a look at it, but > unfortunately I can't seem to reproduce the issue even with --stress > (I let it run for ~1000 repetitions and then aborted it). > > Which platform did you see/test this on, and which compile options did > you use? I went through a failures on > https://travis-ci.org/git/git/builds, but couldn't find this > particular one. Could you point me at the failed run? It was in a Linux build job in one of my custom CI builds [1]: https://travis-ci.org/szeder/git-cooking-topics-for-travis-ci/jobs/490420713#L3401 and I tested it locally on Linux as well. I don't think there are any unusual compiler options. I run it with --stress on Travis CI's macOS as well, but couldn't trigger a failure there. [1] That build log doesn't include the trash dir of the failed test in the log, most likely because of a faulty merge conflict resolution on my part, but you can find a failed trash dir embedded here: https://travis-ci.org/szeder/git/jobs/491401882#L2309
On 02/11, SZEDER Gábor wrote: > On Sun, Feb 10, 2019 at 10:17:12PM +0000, Thomas Gummerer wrote: > > On 02/08, SZEDER Gábor wrote: > > > On Thu, Dec 20, 2018 at 09:44:34PM +0200, Paul-Sebastian Ungureanu wrote: > > > > Add stash push to the helper. > > > > > > > > Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> > > > > > > This patch causes rare failures in 't3903-stash.sh', I've seen it > > > break for the first time today in a Travis CI build: > > > > Thanks for reporting this. I was going to take a look at it, but > > unfortunately I can't seem to reproduce the issue even with --stress > > (I let it run for ~1000 repetitions and then aborted it). > > > > Which platform did you see/test this on, and which compile options did > > you use? I went through a failures on > > https://travis-ci.org/git/git/builds, but couldn't find this > > particular one. Could you point me at the failed run? > > It was in a Linux build job in one of my custom CI builds [1]: > > https://travis-ci.org/szeder/git-cooking-topics-for-travis-ci/jobs/490420713#L3401 > > and I tested it locally on Linux as well. I don't think there are any > unusual compiler options. I run it with --stress on Travis CI's macOS > as well, but couldn't trigger a failure there. Thanks. I still didn't manage to reproduce it locally, but I was now able to test it on Travis CI. The diff below fixes the issue, but I still need to spend some time to better understand why it does. I'll hopefully be in a position to send a patch with a proper log message why this is the right fix in the next couple of days. diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c index c77f62c895..3dab488bd6 100644 --- a/builtin/stash--helper.c +++ b/builtin/stash--helper.c @@ -231,6 +231,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) struct tree *tree; struct lock_file lock_file = LOCK_INIT; + discard_cache(); read_cache_preload(NULL); if (refresh_cache(REFRESH_QUIET)) return -1; > [1] That build log doesn't include the trash dir of the failed test > in the log, most likely because of a faulty merge conflict > resolution on my part, but you can find a failed trash dir > embedded here: > > https://travis-ci.org/szeder/git/jobs/491401882#L2309 >
On Tue, Feb 12, 2019 at 11:18:37PM +0000, Thomas Gummerer wrote: > Thanks. I still didn't manage to reproduce it locally, but I was now > able to test it on Travis CI. > > The diff below fixes the issue, but I still need to spend some time to > better understand why it does. There is nothing like a fix that works, but you have no idea why :) FWIW, I'm at a couple of thousands of '--stress' repetitions with your patch below, and not a single failure yet. > I'll hopefully be in a position to > send a patch with a proper log message why this is the right fix in > the next couple of days. > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > index c77f62c895..3dab488bd6 100644 > --- a/builtin/stash--helper.c > +++ b/builtin/stash--helper.c > @@ -231,6 +231,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) > struct tree *tree; > struct lock_file lock_file = LOCK_INIT; > > + discard_cache(); > read_cache_preload(NULL); > if (refresh_cache(REFRESH_QUIET)) > return -1; > >
Hi Gábor & Thomas, On Tue, 19 Feb 2019, SZEDER Gábor wrote: > On Tue, Feb 12, 2019 at 11:18:37PM +0000, Thomas Gummerer wrote: > > Thanks. I still didn't manage to reproduce it locally, but I was now > > able to test it on Travis CI. > > > > The diff below fixes the issue, but I still need to spend some time to > > better understand why it does. > > There is nothing like a fix that works, but you have no idea why :) I know why. Now. See below for the analysis. > FWIW, I'm at a couple of thousands of '--stress' repetitions with your > patch below, and not a single failure yet. Good, and yes, there is a problem. > > I'll hopefully be in a position to > > send a patch with a proper log message why this is the right fix in > > the next couple of days. > > > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > > index c77f62c895..3dab488bd6 100644 > > --- a/builtin/stash--helper.c > > +++ b/builtin/stash--helper.c > > @@ -231,6 +231,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) > > struct tree *tree; > > struct lock_file lock_file = LOCK_INIT; > > > > + discard_cache(); > > read_cache_preload(NULL); > > if (refresh_cache(REFRESH_QUIET)) > > return -1; > > So this is working, but it is not the correct spot for that `discard_cache()`, as it forces unnecessary cycles on code paths calling `reset_tree()` (which corresponds to `git read-tree`, admittedly a bit confusing) with a fully up to date index. The real fix, I believe, is this: -- snip -- diff --git a/builtin/stash.c b/builtin/stash.c index 2d6dfce883..516dee0fa4 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1372,6 +1372,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet, } } else { struct child_process cp = CHILD_PROCESS_INIT; + discard_cache(); cp.git_cmd = 1; argv_array_pushl(&cp.args, "reset", "--hard", "-q", NULL); -- snap -- And the reason this is needed: we spawn a `git reset --hard` here, which will change the index, but outside of the current process. So the in-process copy is stale. And when the index' mtime does not help us detect that, we run into that test breakage. Now, I seriously believe that we missed the best time to move ps/stash-in-c into `next` for cooking. The best time would have been just after Paul submitted the latest patch series: we know for a fact that he is too busy to really take care of this patch series, so keeping it in `pu` puts everybody into that awkward spot where nobody wants to step on Paul's toes messing with his patch series, but where Paul also lacks the time to push it further, so everything is stuck in a limbo and is *so very much* not cooking at all. You might say that it has turned bad because we failed to stoke the fire appropriately. Since it is now way too late in the v2.21.0 process, this problem is only exacerbated, because it won't even enter `next` "better late than never". To address this unfortunate situation, my current plan is to take over from Paul (we had been chatting about this privately in the past, and he is okay with this because of University eating all his time). I will open the whole bag again, most likely squashing the late fixups into the patches that introduced the problems, re-review with a much finer comb than the patch series has enjoyed on the Git mailing list (even just a quick look at `do_apply_stash()` revealed an unnecessary `reset_tree()` call that *no* reviewer spotted, even I myself, but then, I am hardly solely responsible for that review), and most likely I'll even take my sweet little time changing the code to avoid more spawned Git processes. It will take a long time, and the `stash` project that has been discussed recently to be given to GSoC students is no longer available, as I will take care of it before GSoC even starts, and I won't spend much time reviewing other people's code in the meantime. I will start that only after v2.21.0 final is out, obviously. Once I submit a new iteration, it will look quite a bit different from before, and reviewers will have to re-review *everything*, wasting everybody's time even more. It will have to be re-reviewed in its entirety anyway because it has been *such* a long time since the latest review, and that's just the price we all have to pay for missing the right moment to advance this to `next`. Thomas, I will ask you to review, and Gábor, I will expect you to review that iteration, too, as you are now a bit familiar with the code, and I will really need your help here. Anyway, that's my plan for now. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c >> > index c77f62c895..3dab488bd6 100644 >> > --- a/builtin/stash--helper.c >> > +++ b/builtin/stash--helper.c >> > @@ -231,6 +231,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) >> > struct tree *tree; >> > struct lock_file lock_file = LOCK_INIT; >> > >> > + discard_cache(); >> > read_cache_preload(NULL); >> > if (refresh_cache(REFRESH_QUIET)) >> > return -1; >> > > > So this is working, but it is not the correct spot for that > `discard_cache()`, as it forces unnecessary cycles on code paths calling > `reset_tree()` (which corresponds to `git read-tree`, admittedly a bit > confusing) with a fully up to date index. > > The real fix, I believe, is this: > > -- snip -- > diff --git a/builtin/stash.c b/builtin/stash.c > index 2d6dfce883..516dee0fa4 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -1372,6 +1372,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet, > } > } else { > struct child_process cp = CHILD_PROCESS_INIT; > + discard_cache(); > cp.git_cmd = 1; > argv_array_pushl(&cp.args, "reset", "--hard", "-q", > NULL); > -- snap -- > > And the reason this is needed: we spawn a `git reset --hard` here, which > will change the index, but outside of the current process. So the > in-process copy is stale. And when the index' mtime does not help us > detect that, we run into that test breakage. In non-patch mode with pathspec, there is an invocation of "apply --index -R" of a patch that takes the contents of the HEAD to what is in the index, updating the on-disk index and making our in-core copy stale. Wouldn't we need to do the same? Otherwise, the same "reset_tree()" you are tryhing to protect with this discard_cache() will call read_cache_preload(), no? Among the calls to reset_tree() in this file, I think the one that follows the "reset --hard" (your fix above) and "apply --index -R" (the other side of the same if/else) is the only one that wants to read from the result of an external command we just spawned from the on-disk index, so perhaps moving discard_cache() to just before that call may be a better fix.
On 02/19, Johannes Schindelin wrote: > Hi Gábor & Thomas, > > On Tue, 19 Feb 2019, SZEDER Gábor wrote: > > > > I'll hopefully be in a position to > > > send a patch with a proper log message why this is the right fix in > > > the next couple of days. > > > > > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > > > index c77f62c895..3dab488bd6 100644 > > > --- a/builtin/stash--helper.c > > > +++ b/builtin/stash--helper.c > > > @@ -231,6 +231,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) > > > struct tree *tree; > > > struct lock_file lock_file = LOCK_INIT; > > > > > > + discard_cache(); > > > read_cache_preload(NULL); > > > if (refresh_cache(REFRESH_QUIET)) > > > return -1; > > > > > So this is working, but it is not the correct spot for that > `discard_cache()`, as it forces unnecessary cycles on code paths calling > `reset_tree()` (which corresponds to `git read-tree`, admittedly a bit > confusing) with a fully up to date index. > > The real fix, I believe, is this: > > -- snip -- > diff --git a/builtin/stash.c b/builtin/stash.c > index 2d6dfce883..516dee0fa4 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -1372,6 +1372,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet, > } > } else { > struct child_process cp = CHILD_PROCESS_INIT; > + discard_cache(); > cp.git_cmd = 1; > argv_array_pushl(&cp.args, "reset", "--hard", "-q", > NULL); > -- snap -- > > And the reason this is needed: we spawn a `git reset --hard` here, which > will change the index, but outside of the current process. So the > in-process copy is stale. And when the index' mtime does not help us > detect that, we run into that test breakage. Right, I agree with this assessment, and also agree that this is a better place to discard the cache, rather than doing it in 'reset_tree()'. > Now, I seriously believe that we missed the best time to move > ps/stash-in-c into `next` for cooking. The best time would have been just > after Paul submitted the latest patch series: we know for a fact that he > is too busy to really take care of this patch series, so keeping it in > `pu` puts everybody into that awkward spot where nobody wants to step on > Paul's toes messing with his patch series, but where Paul also lacks the > time to push it further, so everything is stuck in a limbo and is *so very > much* not cooking at all. You might say that it has turned bad because we > failed to stoke the fire appropriately. > > Since it is now way too late in the v2.21.0 process, this problem is only > exacerbated, because it won't even enter `next` "better late than never". > > To address this unfortunate situation, my current plan is to take over > from Paul (we had been chatting about this privately in the past, and he > is okay with this because of University eating all his time). > > I will open the whole bag again, most likely squashing the late fixups > into the patches that introduced the problems, re-review with a much finer > comb than the patch series has enjoyed on the Git mailing list (even just > a quick look at `do_apply_stash()` revealed an unnecessary `reset_tree()` > call that *no* reviewer spotted, even I myself, but then, I am hardly > solely responsible for that review), and most likely I'll even take my > sweet little time changing the code to avoid more spawned Git processes. > > It will take a long time, and the `stash` project that has been discussed > recently to be given to GSoC students is no longer available, as I will > take care of it before GSoC even starts, and I won't spend much time > reviewing other people's code in the meantime. > > I will start that only after v2.21.0 final is out, obviously. > > Once I submit a new iteration, it will look quite a bit different from > before, and reviewers will have to re-review *everything*, wasting > everybody's time even more. It will have to be re-reviewed in its entirety > anyway because it has been *such* a long time since the latest review, and > that's just the price we all have to pay for missing the right moment to > advance this to `next`. Thomas, I will ask you to review, and Gábor, I > will expect you to review that iteration, too, as you are now a bit > familiar with the code, and I will really need your help here. > > Anyway, that's my plan for now. I must say I am not very happy about this plan. The series has been marked as "Will merge to 'next'" in previous iterations, but then we found some issues that prevented that. However I thought we were fine fixing those on top at this point, rather than starting with a new iteration again. I was always under the impression that once the problem that was discovered here was fixed we'd advance the series to 'next' with the patch that comes out of this discussion on top. Whether it's in next shortly before 2.21 or not doesn't seem to make much of a difference to me, as this wasn't going to make the 2.21 release anyway. My hope was that we could get it into 'next' shortly after 2.21 is released to get the series some further exposure (which may well turn up some other issues that we are not aware of yet, but such is the life of software). Sure there are some things that no reviewer spotted, but I'd think so will there be next time and the time after that. I don't believe that code review can eliminate all issues, but I think we all did our best to avoid a good chunk of them and unfortunately missed some in the process. Reviewing this series again in a slightly new form is not something I am personally looking forward to. I'd be more than happy to review patches on top of this series, but after reading it many times over, in slightly different versions, it gets harder and harder to review and to find the motivation to review this. Now if we really think this series has to be completely overhauled, so be it, but I don't think we would end up with a better end result than if we were to continue to work on top of this. > Ciao, > Dscho
Thomas Gummerer <t.gummerer@gmail.com> writes: >> Now, I seriously believe that we missed the best time to move >> ps/stash-in-c into `next` for cooking. The best time would have been just >> ... >> Anyway, that's my plan for now. > > I must say I am not very happy about this plan. The series has been > marked as "Will merge to 'next'" in previous iterations, but then we > found some issues that prevented that. However I thought we were fine > fixing those on top at this point, rather than starting with a new > iteration again. First before going into anything else, let me thank, and let me invite readers of this thread to join me thanking, Paul (Sebi) for sticking with this topic for this long. It is above and beyond what GSoC calls for. Having said that. I too was somehow led to believe that the topic was in a good enough shape, with some room for clean-up by reordering the patches to make them into a more logical progression and squashing an existing and recently figured out "oops, that was wrong" fixes into the patches where the breakages originate. And that was where the "Will merge to" originally came from. Thanks to tools like range-diff, a topic that goes through such reordering and squashing of patches should not have to "waste" a lot of review cycles out of those who have seen the previous round. It however is a totally different matter if the topic was so unsalvageable that it needs a total rewrite---that would need another round of careful review, of course, and it would be irresponsive to merge a topic in such a messy state to 'next'. But my impression was that the topic was not _that_ bad, so Dscho's message and the plan were something that was totally unexpected to me, too.. > I was always under the impression that once the problem that was > discovered here was fixed we'd advance the series to 'next' with the > patch that comes out of this discussion on top. Whether it's in next > shortly before 2.21 or not doesn't seem to make much of a difference > to me, as this wasn't going to make the 2.21 release anyway. My hope > was that we could get it into 'next' shortly after 2.21 is released to > get the series some further exposure (which may well turn up some > other issues that we are not aware of yet, but such is the life of > software). I was hoping similar, but also was hoping that people would use the time wisely while waiting for the next cycle to polish the topic with reordering and squashing, so that it can hit 'next' early once the tree opens. Anyway. I actually have a different issue with this topic, though. It is wonderful to see a GSoC student's continued involvement in the project, but it is not healthy that we need so much work on top of what was marked "done" at the end of the GSoC period. Especially the impression I am getting for the post GSoC work of this topic is not "we are already done converting to built-in during GSoC, and now we are extending the command", but "we ran out of time during GSoC; here is what we would have seen at the end of GSoC in an ideal world." I wonder if this is an unfortunate indication that our expectation is unrealistically high when we accept students' applications. Being overly ambitious is *not* students' fault, but those of us on the list, especially those who mentor, have far deeper experience with how our code and project is structured than any students do. We should be able to, and should not hesitate to, say things like "that's overly ambitious---for such and such, you'd need to even invent an internal API---can we reduce the scope and still produce a useful end result?" One suggestion I have is to have success criteria (e.g. "gets merged to 'master' before GSoC ends" [*1*]) clearly spelled out in the application. Something like that would help managing the expectation and biting way too much for a summer, I'd hope. Side note *1*. Of course, depending on the alignment of the stars ^W our ~10-12 week development cycle and the end of GSoC, getting merged to 'master' might become impossible if it coincides with the pre-release freeze period. But we on the list and the mentors know how the project works, and can help stating a more realistic success criterion if the development cycle and other quirks specific to this project gets in the way. Thanks.
Hi Junio, On Tue, 19 Feb 2019, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > >> > index c77f62c895..3dab488bd6 100644 > >> > --- a/builtin/stash--helper.c > >> > +++ b/builtin/stash--helper.c > >> > @@ -231,6 +231,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset) > >> > struct tree *tree; > >> > struct lock_file lock_file = LOCK_INIT; > >> > > >> > + discard_cache(); > >> > read_cache_preload(NULL); > >> > if (refresh_cache(REFRESH_QUIET)) > >> > return -1; > >> > > > > > So this is working, but it is not the correct spot for that > > `discard_cache()`, as it forces unnecessary cycles on code paths calling > > `reset_tree()` (which corresponds to `git read-tree`, admittedly a bit > > confusing) with a fully up to date index. > > > > The real fix, I believe, is this: > > > > -- snip -- > > diff --git a/builtin/stash.c b/builtin/stash.c > > index 2d6dfce883..516dee0fa4 100644 > > --- a/builtin/stash.c > > +++ b/builtin/stash.c > > @@ -1372,6 +1372,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet, > > } > > } else { > > struct child_process cp = CHILD_PROCESS_INIT; > > + discard_cache(); > > cp.git_cmd = 1; > > argv_array_pushl(&cp.args, "reset", "--hard", "-q", > > NULL); > > -- snap -- > > > > And the reason this is needed: we spawn a `git reset --hard` here, which > > will change the index, but outside of the current process. So the > > in-process copy is stale. And when the index' mtime does not help us > > detect that, we run into that test breakage. > > In non-patch mode with pathspec, there is an invocation of "apply > --index -R" of a patch that takes the contents of the HEAD to what > is in the index, updating the on-disk index and making our in-core > copy stale. Wouldn't we need to do the same? Otherwise, the same > "reset_tree()" you are tryhing to protect with this discard_cache() > will call read_cache_preload(), no? > > Among the calls to reset_tree() in this file, I think the one that > follows the "reset --hard" (your fix above) and "apply --index -R" > (the other side of the same if/else) is the only one that wants to > read from the result of an external command we just spawned from the > on-disk index, so perhaps moving discard_cache() to just before that > call may be a better fix. Good catch! Dscho
Hi Junio, On Tue, 19 Feb 2019, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > >> Now, I seriously believe that we missed the best time to move > >> ps/stash-in-c into `next` for cooking. The best time would have been just > >> ... > >> Anyway, that's my plan for now. > > > > I must say I am not very happy about this plan. The series has been > > marked as "Will merge to 'next'" in previous iterations, but then we > > found some issues that prevented that. However I thought we were fine > > fixing those on top at this point, rather than starting with a new > > iteration again. > > First before going into anything else, let me thank, and let me > invite readers of this thread to join me thanking, Paul (Sebi) for > sticking with this topic for this long. It is above and beyond what > GSoC calls for. Indeed. He put in quite a few dozen hours of work *after* GSoC. > Having said that. > > I too was somehow led to believe that the topic was in a good enough > shape, with some room for clean-up by reordering the patches to make > them into a more logical progression and squashing an existing and > recently figured out "oops, that was wrong" fixes into the patches > where the breakages originate. > > And that was where the "Will merge to" originally came from. Thanks > to tools like range-diff, a topic that goes through such reordering > and squashing of patches should not have to "waste" a lot of review > cycles out of those who have seen the previous round. > > It however is a totally different matter if the topic was so > unsalvageable that it needs a total rewrite---that would need > another round of careful review, of course, and it would be > irresponsive to merge a topic in such a messy state to 'next'. But > my impression was that the topic was not _that_ bad, so Dscho's > message and the plan were something that was totally unexpected to > me, too.. My throwing hands into the air and giving up on advancing it in the current form to `next` was based on its rotting away in `pu`... If it had been in `next` while Hannes, others and I found and fixed bugs, then it would truly be cooking, but in `pu`? It kind of stopped pretty much all the development around it. And that's the only reason why I came up with that plan (that will require at least 50 hours from my side, I know that): because I thought that your not advancing the patches to `next` meant that this extra work was exactly what you were expecting. > > I was always under the impression that once the problem that was > > discovered here was fixed we'd advance the series to 'next' with the > > patch that comes out of this discussion on top. Whether it's in next > > shortly before 2.21 or not doesn't seem to make much of a difference > > to me, as this wasn't going to make the 2.21 release anyway. My hope > > was that we could get it into 'next' shortly after 2.21 is released to > > get the series some further exposure (which may well turn up some > > other issues that we are not aware of yet, but such is the life of > > software). > > I was hoping similar, but also was hoping that people would use the > time wisely while waiting for the next cycle to polish the topic with > reordering and squashing, so that it can hit 'next' early once the > tree opens. > > Anyway. > > I actually have a different issue with this topic, though. It is > wonderful to see a GSoC student's continued involvement in the > project, but it is not healthy that we need so much work on top of > what was marked "done" at the end of the GSoC period. Especially > the impression I am getting for the post GSoC work of this topic is > not "we are already done converting to built-in during GSoC, and now > we are extending the command", but "we ran out of time during GSoC; > here is what we would have seen at the end of GSoC in an ideal > world." > > I wonder if this is an unfortunate indication that our expectation > is unrealistically high when we accept students' applications. Yes, you are right. This is on me. Guilty as charged. And I don't mean this flippantly. I thought long and hard about this, and I've come to the conclusion that I will not offer to mentor this round of GSoC as a consequence. Ciao, Dscho > Being overly ambitious is *not* students' fault, but those of us on > the list, especially those who mentor, have far deeper experience > with how our code and project is structured than any students do. > We should be able to, and should not hesitate to, say things like > "that's overly ambitious---for such and such, you'd need to even > invent an internal API---can we reduce the scope and still produce a > useful end result?" > > One suggestion I have is to have success criteria (e.g. "gets merged > to 'master' before GSoC ends" [*1*]) clearly spelled out in the > application. Something like that would help managing the > expectation and biting way too much for a summer, I'd hope. > > Side note *1*. Of course, depending on the alignment of the > stars ^W our ~10-12 week development cycle and the end of GSoC, > getting merged to 'master' might become impossible if it > coincides with the pre-release freeze period. But we on the > list and the mentors know how the project works, and can help > stating a more realistic success criterion if the development > cycle and other quirks specific to this project gets in the way. > > Thanks. >
On 02/19, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > >> Now, I seriously believe that we missed the best time to move > >> ps/stash-in-c into `next` for cooking. The best time would have been just > >> ... > >> Anyway, that's my plan for now. > > > > I must say I am not very happy about this plan. The series has been > > marked as "Will merge to 'next'" in previous iterations, but then we > > found some issues that prevented that. However I thought we were fine > > fixing those on top at this point, rather than starting with a new > > iteration again. > > First before going into anything else, let me thank, and let me > invite readers of this thread to join me thanking, Paul (Sebi) for > sticking with this topic for this long. It is above and beyond what > GSoC calls for. Indeed, thanks for all your work on this Paul-Sebastian! > Having said that. > > I too was somehow led to believe that the topic was in a good enough > shape, with some room for clean-up by reordering the patches to make > them into a more logical progression and squashing an existing and > recently figured out "oops, that was wrong" fixes into the patches > where the breakages originate. > > And that was where the "Will merge to" originally came from. Thanks > to tools like range-diff, a topic that goes through such reordering > and squashing of patches should not have to "waste" a lot of review > cycles out of those who have seen the previous round. Right, I had the impression that we were okay with doing the cleanups on top of what is already in 'pu'. Especially since the topic with Johannes Sixt's patch on top was marked as "Will merge to 'next'" at one point if I remember correctly. I didn't think that it being in 'pu' vs. it being in 'next' would make too much of a difference there, and that it's just a by-product of where in the development cycle we are rather than an indication of which way we are taking the branch. > It however is a totally different matter if the topic was so > unsalvageable that it needs a total rewrite---that would need > another round of careful review, of course, and it would be > irresponsive to merge a topic in such a messy state to 'next'. But > my impression was that the topic was not _that_ bad, so Dscho's > message and the plan were something that was totally unexpected to > me, too.. Indeed, the topic did not get any worse over the time it was in 'pu', indeed it got a couple of fixes on top. And my impression was that Dscho still would have wanted to get the topic merged to next much earlier, so I don't quite understand what changed since then, other than getting a few fixes on top. > > I was always under the impression that once the problem that was > > discovered here was fixed we'd advance the series to 'next' with the > > patch that comes out of this discussion on top. Whether it's in next > > shortly before 2.21 or not doesn't seem to make much of a difference > > to me, as this wasn't going to make the 2.21 release anyway. My hope > > was that we could get it into 'next' shortly after 2.21 is released to > > get the series some further exposure (which may well turn up some > > other issues that we are not aware of yet, but such is the life of > > software). > > I was hoping similar, but also was hoping that people would use the > time wisely while waiting for the next cycle to polish the topic with > reordering and squashing, so that it can hit 'next' early once the > tree opens. I'd be happy to do this myself, but as mentioned above I thought we were okay with just having the patches on top, and leave the work Paul-Sebastian sent until now as it was. If that impression was wrong I'm happy to put in some work to help with the cleanup. > Anyway. > > I actually have a different issue with this topic, though. It is > wonderful to see a GSoC student's continued involvement in the > project, but it is not healthy that we need so much work on top of > what was marked "done" at the end of the GSoC period. Especially > the impression I am getting for the post GSoC work of this topic is > not "we are already done converting to built-in during GSoC, and now > we are extending the command", but "we ran out of time during GSoC; > here is what we would have seen at the end of GSoC in an ideal > world." > > I wonder if this is an unfortunate indication that our expectation > is unrealistically high when we accept students' applications. > Being overly ambitious is *not* students' fault, but those of us on > the list, especially those who mentor, have far deeper experience > with how our code and project is structured than any students do. > We should be able to, and should not hesitate to, say things like > "that's overly ambitious---for such and such, you'd need to even > invent an internal API---can we reduce the scope and still produce a > useful end result?" > > One suggestion I have is to have success criteria (e.g. "gets merged > to 'master' before GSoC ends" [*1*]) clearly spelled out in the > application. Something like that would help managing the > expectation and biting way too much for a summer, I'd hope. [Adding Christian and Olga to cc here as this discussion should be interesting to them as well as GSoC mentors] I think one thing we underestimated at least here is how long it takes from "everything that we intended to do is done" to "this is reviewed and ready to merge into 'next' and 'master'". This is partly my fault as well because it took me quite a while to review the series on the list, which certainly didn't help in moving things along. While having set criteria is a good idea, I think we should still give some leeway to the mentors in terms of the actual success/fail rating in the program. Not getting things merged is definitely not a good experience, but it would be much worse to not get it merged, and also fail GSoC and not getting paid for the efforts over the summer. We shouldn't punish students for the failure of the mentors to estimate projects correctly. One thing we should do I think is to say the project should be "complete" at least a month before the end of GSoC, and that the last month should only be dedicated to polishing the patches. I don't know how to make sure the students still have enough work to do during that last period while they are just waiting for reviewers. An other alternative (just thinking out loud here) is to make sure the project has a few deliverables that can and will be merged individually, even if they later build on top of eachother. This would mean getting students to send multiple series, when the patches may normally go better in a single series, but I think it would help moving the review cycle along faster. Dunno, maybe there are other alternatives that I'm missing as well. > Side note *1*. Of course, depending on the alignment of the > stars ^W our ~10-12 week development cycle and the end of GSoC, > getting merged to 'master' might become impossible if it > coincides with the pre-release freeze period. But we on the > list and the mentors know how the project works, and can help > stating a more realistic success criterion if the development > cycle and other quirks specific to this project gets in the way. > > Thanks.
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c index 080c2f7aa6..c77f62c895 100644 --- a/builtin/stash--helper.c +++ b/builtin/stash--helper.c @@ -23,6 +23,9 @@ static const char * const git_stash_helper_usage[] = { N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] [<stash>]"), N_("git stash--helper branch <branchname> [<stash>]"), N_("git stash--helper clear"), + N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" + " [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n" + " [--] [<pathspec>...]]"), NULL }; @@ -71,6 +74,13 @@ static const char * const git_stash_helper_create_usage[] = { NULL }; +static const char * const git_stash_helper_push_usage[] = { + N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" + " [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n" + " [--] [<pathspec>...]]"), + NULL +}; + static const char *ref_stash = "refs/stash"; static struct strbuf stash_index_path = STRBUF_INIT; @@ -1092,7 +1102,7 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps) static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf, int include_untracked, int patch_mode, - struct stash_info *info) + struct stash_info *info, struct strbuf *patch) { int ret = 0; int flags = 0; @@ -1105,7 +1115,6 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf, struct strbuf msg = STRBUF_INIT; struct strbuf commit_tree_label = STRBUF_INIT; struct strbuf untracked_files = STRBUF_INIT; - struct strbuf patch = STRBUF_INIT; prepare_fallback_ident("git stash", "git@stash"); @@ -1154,7 +1163,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf, untracked_commit_option = 1; } if (patch_mode) { - ret = stash_patch(info, ps, &patch); + ret = stash_patch(info, ps, patch); if (ret < 0) { fprintf_ln(stderr, _("Cannot save the current " "worktree state")); @@ -1225,7 +1234,8 @@ static int create_stash(int argc, const char **argv, const char *prefix) memset(&ps, 0, sizeof(ps)); strbuf_addstr(&stash_msg_buf, stash_msg); - ret = do_create_stash(ps, &stash_msg_buf, include_untracked, 0, &info); + ret = do_create_stash(ps, &stash_msg_buf, include_untracked, 0, &info, + NULL); if (!ret) printf_ln("%s", oid_to_hex(&info.w_commit)); @@ -1239,6 +1249,231 @@ static int create_stash(int argc, const char **argv, const char *prefix) return ret < 0; } +static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet, + int keep_index, int patch_mode, int include_untracked) +{ + int ret = 0; + struct stash_info info; + struct strbuf patch = STRBUF_INIT; + struct strbuf stash_msg_buf = STRBUF_INIT; + + if (patch_mode && keep_index == -1) + keep_index = 1; + + if (patch_mode && include_untracked) { + fprintf_ln(stderr, _("Can't use --patch and --include-untracked" + " or --all at the same time")); + ret = -1; + goto done; + } + + read_cache_preload(NULL); + if (!include_untracked && ps.nr) { + int i; + char *ps_matched = xcalloc(ps.nr, 1); + + for (i = 0; i < active_nr; i++) + ce_path_match(&the_index, active_cache[i], &ps, + ps_matched); + + if (report_path_error(ps_matched, &ps, NULL)) { + fprintf_ln(stderr, _("Did you forget to 'git add'?")); + ret = -1; + free(ps_matched); + goto done; + } + free(ps_matched); + } + + if (refresh_cache(REFRESH_QUIET)) { + ret = -1; + goto done; + } + + if (!check_changes(ps, include_untracked)) { + if (!quiet) + printf_ln(_("No local changes to save")); + goto done; + } + + if (!reflog_exists(ref_stash) && do_clear_stash()) { + ret = -1; + fprintf_ln(stderr, _("Cannot initialize stash")); + goto done; + } + + if (stash_msg) + strbuf_addstr(&stash_msg_buf, stash_msg); + if (do_create_stash(ps, &stash_msg_buf, include_untracked, patch_mode, + &info, &patch)) { + ret = -1; + goto done; + } + + if (do_store_stash(&info.w_commit, stash_msg_buf.buf, 1)) { + ret = -1; + fprintf_ln(stderr, _("Cannot save the current status")); + goto done; + } + + printf_ln(_("Saved working directory and index state %s"), + stash_msg_buf.buf); + + if (!patch_mode) { + if (include_untracked && !ps.nr) { + struct child_process cp = CHILD_PROCESS_INIT; + + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "clean", "--force", + "--quiet", "-d", NULL); + if (include_untracked == INCLUDE_ALL_FILES) + argv_array_push(&cp.args, "-x"); + if (run_command(&cp)) { + ret = -1; + goto done; + } + } + if (ps.nr) { + struct child_process cp_add = CHILD_PROCESS_INIT; + struct child_process cp_diff = CHILD_PROCESS_INIT; + struct child_process cp_apply = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + + cp_add.git_cmd = 1; + argv_array_push(&cp_add.args, "add"); + if (!include_untracked) + argv_array_push(&cp_add.args, "-u"); + if (include_untracked == INCLUDE_ALL_FILES) + argv_array_push(&cp_add.args, "--force"); + argv_array_push(&cp_add.args, "--"); + add_pathspecs(&cp_add.args, ps); + if (run_command(&cp_add)) { + ret = -1; + goto done; + } + + cp_diff.git_cmd = 1; + argv_array_pushl(&cp_diff.args, "diff-index", "-p", + "--cached", "--binary", "HEAD", "--", + NULL); + add_pathspecs(&cp_diff.args, ps); + if (pipe_command(&cp_diff, NULL, 0, &out, 0, NULL, 0)) { + ret = -1; + goto done; + } + + cp_apply.git_cmd = 1; + argv_array_pushl(&cp_apply.args, "apply", "--index", + "-R", NULL); + if (pipe_command(&cp_apply, out.buf, out.len, NULL, 0, + NULL, 0)) { + ret = -1; + goto done; + } + } else { + struct child_process cp = CHILD_PROCESS_INIT; + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "reset", "--hard", "-q", + NULL); + if (run_command(&cp)) { + ret = -1; + goto done; + } + } + + if (keep_index == 1 && !is_null_oid(&info.i_tree)) { + struct child_process cp_ls = CHILD_PROCESS_INIT; + struct child_process cp_checkout = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + + if (reset_tree(&info.i_tree, 0, 1)) { + ret = -1; + goto done; + } + + cp_ls.git_cmd = 1; + argv_array_pushl(&cp_ls.args, "ls-files", "-z", + "--modified", "--", NULL); + + add_pathspecs(&cp_ls.args, ps); + if (pipe_command(&cp_ls, NULL, 0, &out, 0, NULL, 0)) { + ret = -1; + goto done; + } + + cp_checkout.git_cmd = 1; + argv_array_pushl(&cp_checkout.args, "checkout-index", + "-z", "--force", "--stdin", NULL); + if (pipe_command(&cp_checkout, out.buf, out.len, NULL, + 0, NULL, 0)) { + ret = -1; + goto done; + } + } + goto done; + } else { + struct child_process cp = CHILD_PROCESS_INIT; + + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "apply", "-R", NULL); + + if (pipe_command(&cp, patch.buf, patch.len, NULL, 0, NULL, 0)) { + fprintf_ln(stderr, _("Cannot remove worktree changes")); + ret = -1; + goto done; + } + + if (keep_index < 1) { + struct child_process cp = CHILD_PROCESS_INIT; + + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "reset", "-q", "--", NULL); + add_pathspecs(&cp.args, ps); + if (run_command(&cp)) { + ret = -1; + goto done; + } + } + goto done; + } + +done: + strbuf_release(&stash_msg_buf); + return ret; +} + +static int push_stash(int argc, const char **argv, const char *prefix) +{ + int keep_index = -1; + int patch_mode = 0; + int include_untracked = 0; + int quiet = 0; + const char *stash_msg = NULL; + struct pathspec ps; + struct option options[] = { + OPT_BOOL('k', "keep-index", &keep_index, + N_("keep index")), + OPT_BOOL('p', "patch", &patch_mode, + N_("stash in patch mode")), + OPT__QUIET(&quiet, N_("quiet mode")), + OPT_BOOL('u', "include-untracked", &include_untracked, + N_("include untracked files in stash")), + OPT_SET_INT('a', "all", &include_untracked, + N_("include ignore files"), 2), + OPT_STRING('m', "message", &stash_msg, N_("message"), + N_("stash message")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_helper_push_usage, + 0); + + parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL, prefix, argv); + return do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode, + include_untracked); +} + int cmd_stash__helper(int argc, const char **argv, const char *prefix) { pid_t pid = getpid(); @@ -1277,6 +1512,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix) return !!store_stash(argc, argv, prefix); else if (!strcmp(argv[0], "create")) return !!create_stash(argc, argv, prefix); + else if (!strcmp(argv[0], "push")) + return !!push_stash(argc, argv, prefix); usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]), git_stash_helper_usage, options); diff --git a/git-stash.sh b/git-stash.sh index a9b3064ff0..51d7a06601 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -429,7 +429,8 @@ save) ;; push) shift - push_stash "$@" + cd "$START_DIR" + git stash--helper push "$@" ;; apply) shift @@ -465,7 +466,8 @@ branch) *) case $# in 0) - push_stash && + cd "$START_DIR" + git stash--helper push && say "$(gettext "(To restore them type \"git stash apply\")")" ;; *)
Add stash push to the helper. Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> --- builtin/stash--helper.c | 245 +++++++++++++++++++++++++++++++++++++++- git-stash.sh | 6 +- 2 files changed, 245 insertions(+), 6 deletions(-)