git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: dev@kipras.org,
	Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>,
	git@vger.kernel.org
Subject: Re: [FR] Allow `git stash create` to include untracked changes
Date: Tue, 29 Oct 2019 08:29:39 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1910290806310.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqa79lbpte.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Mon, 28 Oct 2019, Junio C Hamano wrote:

> dev@kipras.org writes:
>
> > Would it be possible for `git stash create` to include untracked
> > changes (probably the same way `git stash push --include-untracked`
> > does)?
>
> Doesn't the subcommand have -u/--include-untracked option?
>
> ... goes and looks git-stash.sh ...
>
> create_stash () {

Careful, this is a false friend. It _is_ work horse of `git stash
create`, but not _quite_ in the way you think:

-- snap --
create)
	shift
	create_stash -m "$*" && echo "$w_commit"
	;;
-- snap --

So what is the reason for the existence of this command-line parsing:

>
> 	prepare_fallback_ident
>
> 	stash_msg=
> 	untracked=
> 	while test $# != 0
> 	do
> 		case "$1" in
> 		-m|--message)
> 			...
> 			;;
> 		-u|--include-untracked)
> 			shift
> 			untracked=${1?"BUG: create_stash () -u requires an argument"}
> 			;;
> 		...
> 	done
> 	...

Why, `push_stash` calls `create_stash` ;-)

-- snip --
push_stash () {
	[...]
        untracked=
	[...]
                -u|--include-untracked)
			untracked=untracked
			;;
	[...]
        create_stash -m "$stash_msg" -u "$untracked" -- "$@"
	[...]
-- snap --

So the reason that `git stash create -u ...` does not work is that it
never was designed to work ;-)

> It is entirely possible that with recent push to rewrite scripted
> Porcelain commands to builtin/, we may have lost a feature or two
> by accident.
>
>   ... goes and checks ...
>
> And it does appear that builtin/stash.c::create_stash() lacks
> support for command line arguments since d4788af8 ("stash: convert
> create to builtin", 2019-02-25).
>
> Would doing
>
> 	export GIT_TEST_STASH_USE_BUILTIN=no
>
> or
>
> 	git config --global stash.usebuiltin no
>
> help in the meantime???

Well, given this code in `builtin/stash.c`, I would imagine that `git
stash push -u ...` works both in the legacy and the built-in version of
`git stash`:

-- snip --
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()
	};

	if (argc)
		argc = parse_options(argc, argv, prefix, options,
				     git_stash_push_usage,
				     0);

	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);
}
-- snap --

So why does `git stash create -u ...` not work? Because the original
design of `git stash create` never intended to take any command-line
arguments other than the stash message. And that design can obviously
not be fixed without breaking backwards compatibility.

What we _could_ do is to add a new command-line option to `git stash
push` that would make it behave like the `create` subsubcommand: _not_
update the `refs/stash` ref but instead print the commit hash.

I am not quite sure how to call this option (`--ephemeral` came to my
mind, as the created commit is not reachable from anywhere and is hence
subject to garbage collection).

A completely different approach would be to allow overriding the ref to
store the stash in, with an empty value triggering the mode where the
ref is not updated but the commit hash would be printed instead. I am
thinking of something like `git stash -r '' push ...`, starting with
this patch:

-- snip --
diff --git a/builtin/stash.c b/builtin/stash.c
index bb4f6d8d762..43b0a155b1d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1553,6 +1553,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
 	struct argv_array args = ARGV_ARRAY_INIT;

 	struct option options[] = {
+		OPT_STRING('r', "ref", &ref_stash, N_("ref"),
+			   N_("override `refs/stash`")),
 		OPT_END()
 	};

-- snap --

The biggest trick will be to make all the code paths safe that assume
that `ref_stash` refers to a valid ref ;-)

Ciao,
Dscho

  parent reply	other threads:[~2019-10-29  7:30 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-26 14:38 dev
2019-10-28  1:51 ` Junio C Hamano
2019-10-28 16:03   ` Kipras Melnikovas
2019-10-29  7:29   ` Johannes Schindelin [this message]
2019-10-31 13:40     ` 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=nycvar.QRO.7.76.6.1910290806310.46@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=dev@kipras.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ungureanupaulsebastian@gmail.com \
    --subject='Re: [FR] Allow `git stash create` to include untracked changes' \
    /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).