[PATCH 1/2] stash: get git_stash_config at the top level

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Mar 2, 2020, 18:18 UTC
Message-ID: <20200302181832.GA1571684@cat>
In-Reply-To: <xmqq36ayob9a.fsf@gitster-ct.c.googlers.com>

On 02/25, Junio C Hamano wrote:

Show 21 quoted lines

> Son Luong Ngoc <son.luong@booking.com> writes:
> 
> > I have been trying to build git from source and noticing that some
> > tests have been failing since 2.25 with the flag
> > "GIT_TEST_STASH_USE_BUILTIN=false"
> >
> > I think in 2.25 t3903.103 started to fail (rebase related) and
> > current master t3904 may be failing also.
> >
> > Is "GIT_TEST_STASH_USE_BUILTIN=false" is still being tested with
> > or are we totally deprecating this flag?
> 
> In the longer term, when "git stash" gains new features that did not
> exist in the original scripted version, tests that observe how these
> features work would start failing when using the scripted version.
> 
> I picked some people from "git shortlog --no-merges builtin/stash.c"
> and placed them on the CC line---perhaps they may know more.  It
> happens that Johannes is also familiar with "rebase", which you
> said is involved in the test failure, so I'd imagine he would be the
> best person to ask.

Thanks for the report Son, and sorry for taking so long. I'm a little behing on reading my emails right now.

I think it is time to get rid of legacy stash. Nobody seems to have noticed this test failure until now, but according to your bisection it looks like this test has been failing since 2.23, which I confirmed locally. In addition, the last bugfix that was related to the rewrite was in 2.25, though that was really a fix for another fix in 2.24 that we failed to catch earlier.

I think 2.26 should still ship with the option, but after that we can probably get rid of it. So here's a couple of patches to do just that, for merging after 2.26 ships.

--- >8 --- In the next commit we're adding another config variable to be read from 'git_stash_config', that is valid for the top level command instead of just a subset. Move the 'git_config' invocation for 'git_stash_config' to the top-level to prepare for that.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 879fc5f368..f371db270c 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -712,7 +712,7 @@ static int git_stash_config(const char *var, const char *value, void *cb)
 		show_patch = git_config_bool(var, value);
 		return 0;
 	}
-	return git_default_config(var, value, cb);
+	return git_diff_basic_config(var, value, cb);
 }
 
 static int show_stash(int argc, const char **argv, const char *prefix)
@@ -749,7 +749,6 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 	 * any options.
 	 */
 	if (revision_args.argc == 1) {
-		git_config(git_stash_config, NULL);
 		if (show_stat)
 			rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT;
 
@@ -1573,7 +1572,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 	trace_repo_setup(prefix);
 	setup_work_tree();
 
-	git_config(git_diff_basic_config, NULL);
+	git_config(git_stash_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, options, git_stash_usage,
 			     PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH);

-- 
2.25.1.377.g2d2118b814