git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Son Luong Ngoc <son.luong@booking.com>,
	git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Subject: [PATCH 1/2] stash: get git_stash_config at the top level
Date: Mon, 2 Mar 2020 18:18:32 +0000	[thread overview]
Message-ID: <20200302181832.GA1571684@cat> (raw)
In-Reply-To: <xmqq36ayob9a.fsf@gitster-ct.c.googlers.com>

On 02/25, Junio C Hamano wrote:
> 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


  parent reply	other threads:[~2020-03-02 18:18 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-24  8:33 Git Rebase: test failing with GIT_TEST_STASH_USE_BUILTIN=false Son Luong Ngoc
2020-02-25 16:57 ` Junio C Hamano
     [not found]   ` <710DB9BA-D134-48E7-8CAB-B8816FED8AB8@booking.com>
2020-03-01  9:59     ` [External] " Son Luong Ngoc
2020-03-01 10:40       ` Son Luong Ngoc
2020-03-02 18:18   ` Thomas Gummerer [this message]
2020-03-02 18:19     ` [PATCH 2/2] stash: remove the stash.useBuiltin setting Thomas Gummerer
2020-03-02 21:30       ` Junio C Hamano
2020-03-02 22:21       ` non-regression bug in 'git merge' (really, xdiff/xmerge, I think) Junio C Hamano
2020-03-06 15:48         ` Elijah Newren
2020-03-03 13:33       ` [PATCH 2/2] stash: remove the stash.useBuiltin setting Johannes Schindelin
2020-03-03 16:47         ` Junio C Hamano
2020-03-03 17:36         ` Thomas Gummerer
2020-03-04 16:41           ` Johannes Schindelin
     [not found]       ` <CA+P7+xqVyMnG6FiyL4hngPfEEMi5CKz1qN5Cy1TMMpLg3CmsBw@mail.gmail.com>
2020-03-06 14:11         ` Johannes Schindelin
2020-03-06 17:29         ` Thomas Gummerer
2020-03-03 17:46     ` [PATCH v2 1/2] stash: get git_stash_config at the top level Thomas Gummerer
2020-03-03 17:46       ` [PATCH v2 2/2] stash: remove the stash.useBuiltin setting Thomas Gummerer
2020-03-03 18:45         ` Junio C Hamano
2020-03-04 11:49           ` Thomas Gummerer
2020-03-04 16:25             ` Junio C Hamano
2020-03-05 17:04               ` Thomas Gummerer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200302181832.GA1571684@cat \
    --to=t.gummerer@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=son.luong@booking.com \
    --cc=ungureanupaulsebastian@gmail.com \
    --subject='Re: [PATCH 1/2] stash: get git_stash_config at the top level' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).