stash: handle pathspec magic again

From: Johannes Schindelin <[email protected]>

When this developer backported support for --quiet to the scripted version of git stash in 80590055ea (stash: optionally use the scripted version again, 2018-12-20), it looked like a sane choice to use eval to execute the command line passed in via the parameter list of maybe_quiet.

However, that is not what we should have done, as that command-line was already in the correct shape.

This can be seen very clearly when passing arguments with special characters, like

	git stash -- ':(glob)**/*.txt'

Since this is exactly what we want to test in the next commit (where we fix this very incantation with the built-in stash), let's fix the legacy scripted version of git stash first.

---
 git-legacy-stash.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 8a8c4a9270..f60e9b3e87 100755
@@ -86,17 +86,17 @@ maybe_quiet () {
 		shift
 		if test -n "$GIT_QUIET"
 		then
-			eval "$@" 2>/dev/null
+			"$@" 2>/dev/null
 		else
-			eval "$@"
+			"$@"
 		fi
 		;;
 	*)
 		if test -n "$GIT_QUIET"
 		then
-			eval "$@" >/dev/null 2>&1
+			"$@" >/dev/null 2>&1
 		else
-			eval "$@"
+			"$@"
 		fi
 		;;
 	esac

gitgitgadget

From: Johannes Schindelin <[email protected]>

When passing a list of pathspecs to, say, git add, we need to be careful to use the original form, not the parsed form of the pathspecs.

This makes a difference e.g. when calling

	git stash -- ':(glob)**/*.txt'

where the original form includes the :(glob) prefix while the parsed form does not.

However, in the built-in git stash, we passed the parsed (i.e. incorrect) form, and git add would fail with the error message:

fatal: pathspec '**/*.txt' did not match any files

at the stage where git stash drops the changes from the worktree, even if refs/stash has been actually updated successfully.

This fixes https://github.com/git-for-windows/git/issues/2037

---
 builtin/stash.c                    | 5 +++--
 t/t3905-stash-include-untracked.sh | 6 ++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

index 1bfa24030c..2f29d037c8 100644
@@ -830,7 +830,7 @@ static void add_pathspecs(struct argv_array *args,
 	int i;
 
 	for (i = 0; i < ps.nr; i++)
-		argv_array_push(args, ps.items[i].match);
+		argv_array_push(args, ps.items[i].original);
 }
 
 /*

@@ -1466,7 +1466,8 @@ static int push_stash(int argc, const char **argv, const char *prefix)
 				     git_stash_push_usage,
 				     0);
 
-	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL, prefix, argv);
+	parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
+		       prefix, argv);
 	return do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode,
 			     include_untracked);
 }

index cc1c8a7bb2..29ca76f2fb 100755
@@ -283,4 +283,10 @@ test_expect_success 'stash -u -- <non-existant> shows no changes when there are
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'stash -u with globs' '
+	>untracked.txt &&
+	git stash -u -- ":(glob)**/*.txt" &&
+	test_path_is_missing untracked.txt
+'
+
 test_done

gitgitgadget

It was reported in https://github.com/git-for-windows/git/issues/2037 that git stash -- ':(glob)**/*.testextension is broken. The problem is not even the stash operation itself, it happens when the git add part of dropping the local changes is spawned: we simply passed the parsed pathspec instead of the unparsed one.

While verifying the fix, I also realized that the escape hatch was seriously broken by my "backport of the -q option": It introduced four bogus eval statements, which really need to be dropped.

Johannes Schindelin (2):
legacy stash: fix "rudimentary backport of -q"
built-in stash: handle :(glob) pathspecs again

 builtin/stash.c                    | 5 +++--
 git-legacy-stash.sh                | 8 ++++----
 t/t3905-stash-include-untracked.sh | 6 ++++++
 3 files changed, 13 insertions(+), 6 deletions(-)

base-commit: 7906af0cb84c8e65656347909abd4e22b04d1c1e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-159%2Fdscho%2Fstash-with-globs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-159/dscho/stash-with-globs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/159

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Thanks.

We seem to be piling many "oops" on top, even after the topic hits 'next'. But fixes are better late than never ;-).

Will queue.

Hi Junio,

Yes, we do. At least now I do not have the impression that I have to impose on Paul to integrate whatever diffs I came up with, I can now just submit small patch series that are easily reviewed.

If you care deeply about the commit history, I hereby offer to you to clean up the built-in stash patches when you say you're ready to advance them to master; I will then squash the fixes into the proper places to make it a nicer read, with the promise that the tree will be the same in the end as with the oops-upon-oops patch history that we're piling up in next. I would do that for you.

Thanks,
Dscho

What's the goal of such a rebase? To rebuild the topic as a sensible sequence of commits that logically builds on top of previous steps to ease later bisection and understanding?

Thanks for an offer out of good intentions,, but let's move on and polish the tree shape at the tip of this topic. The history behind it may be messier than other segments of our history, and future developers may have harder time learning the intention of the topic when making changes on top, but this one was supposed to create a bug-to-bug reimplementation of the scripted version. What matters more would be our future changes on top of this code, which improves what we used to have as scripted Porcelain. They will genuinely be novel efforts, need to be built in logical order and explainable steps to help future developers. Compared to that, so the history of our stumbling along the way to reach today's tip of the topic has much lower value.

Besides I think it is way too late for the current topic. We established before the topic hit 'next' that reviewers' eyes all lost freshness and patience to review another round of this series adequately.

We at least know that the ordering and organization of the iteration we see in 'next' is crappy, because some reviewers did look at them. The rewrite will see no reviews, if any, far fewer and shallower reviews than the iteration we have; nobody would be able to say with confidence that the rewritten series achieves its goal of leaving a sensible history. Doing so just before it hits 'master' makes it a sure thing.

Let's just we all admit that we did a poor job when we decided to push this topic to 'next' before it was ready, and learn the lesson to avoid haste making waste for the future topics.

Thanks.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Thanks. I agree that these eval are evaluating one iteration too much.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Hi Junio,

To appease you enough that you stop complaining about the current, or previous, state of ps/stash-in-c.

I am genuinely interested in making this all more pleasant for you, even if my efforts to that end show no fruit.

I would be prepared to do that, but I am constantly reminded of the unfortunate way we handled ps/stash-in-c, where you thought it was way too early to move to next, and I am convinced that we simply were way too late to start cooking in next.

So I keep offering to do work so that you would be happier, but none of my suggestions seem to work.

Right. But we moved right past that, and continued enhancing git stash, (like the --quiet thing) and were now stuck with the unfortunate situation that we had to do it in both built-in and scripted version.

Fine. But in that case, I would appreciate not being reminded of the messiness. Not unless you let me do something about it. Don't put me between a rock and a hard place, please.

Quite honestly, I am at a loss what you are suggesting here. The original contributor (Paul) got unexpectedly busy with university, so he was not able to take care of any updates.

I would have made those updates (I promised, after all), but at some stage it felt more logical to explain in add-on topics what breakages were introduced by the built-in rewrite and fix them: squashing the fixes in would have made it less obvious why certain changes had to be done that way (after all, I missed in the original dozens of reviews, pre-submission and post-submission, e.g. the ORIG_HEAD problems).

But you did not complain about me adding on top back then, so I do not understand why you complain about it now...

I am more than willing to move on, but if we keep repeating how messy the current state is and do not even come up with a way how we could handle this better in the future, then I do not really feel that we *are* moving on after all.

Ciao,
Dscho

Hi Junio,

I agree. How can I help?

Ciao,
Dscho

I just sent an updated version of my patch, based on top of this at [*1*]. Would be great if you could review that :)

*1*: https://public-inbox.org/git/[email protected]/

First of all, you do not have to appease me. What happened in the past has happened already, and whether I complain or not, the fact that the history we came up with before pushing the topic to 'next' was suboptimal. Nothing short of kicking it out of 'next' and redoing as if it were a fresh topic would fix that, but we all agreed that it is not the best way to spend our developer and reviewer resources.

You had been given plenty of chance to do something about it after you added "oh, it was wrong not to have a legacy fallback, and here is a patch on the top". This is not the time to revisit the issue.

Gagging me won't change the fact that the history we ended up is messy. Without getting reminded of our past mistake(s) ourselves, what else encourages us to do better the next time?

The lesson I personally learned is that yielding to the wish to hastily push things that are not ready to 'next' will leave us mess. I hope the lesson submitters and mentors have learned is not that by bombarding reviewers with too many iterations that do not address an issue, a topic can be pushed through with the issue unresolved with reviewer fatigue.

Perhaps your attitude is coming from a fundamental misunderstanding of the process. Don't be unnecessarily defensive, as nobody is being hostile or attacking you.

It's not like this is a competition to see if your taste as a developer is better or worse than others (including me). It is a cooperative process and everybody involved is "at fault" if we made a mistake. Ending up with a suboptimal history in 'next' is not creditable solely to you. I am as much to blame, so are others who advocated to merge it to 'next' even it were not ready. Or those who did not speak up before it was too late, for that matter.