Git development
 help / color / mirror / Atom feed
* [PATCH 2/3] setup_revisions(): swap if/else bodies to make the next step more readable
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
  To: git; +Cc: Siddharth Kannan
In-Reply-To: <20170212184132.12375-1-gitster@pobox.com>

Swap the condition and bodies of an "if (A) do_A else do_B" in
setup_revisions() to "if (!A) do_B else do A", to make the change in
the the next step easier to read.  

No behaviour change is intended in this step.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 4f46b8ba81..eccf1ab695 100644
--- a/revision.c
+++ b/revision.c
@@ -2237,8 +2237,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			continue;
 		}
 
-
-		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+		if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
+			got_rev_arg = 1;
+		} else {
 			int j;
 			if (seen_dashdash || *arg == '^')
 				die("bad revision '%s'", arg);
@@ -2255,8 +2256,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			append_prune_data(&prune_data, argv + i);
 			break;
 		}
-		else
-			got_rev_arg = 1;
 	}
 
 	if (prune_data.nr) {
-- 
2.12.0-rc1-212-ga9adfb24fa


^ permalink raw reply related

* [PATCH 3/3] setup_revisions(): allow a rev that begins with a dash
From: Junio C Hamano @ 2017-02-12 18:41 UTC (permalink / raw)
  To: git; +Cc: Siddharth Kannan
In-Reply-To: <20170212184132.12375-1-gitster@pobox.com>

Now all the preparatory pieces are in place, it is a matter of
handling a truly unknown option _after_ handle_revision_arg()
decides that arg is not a rev.  

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 We _could_ do without a new variable maybe_opt and instead check if
 arg begins with a dash one more time, but it is cleaner to do it
 the way this patch does to avoid writing the same check twice.  We
 may be hit with a desire similar to but an opposite of the current
 topic (which wants to allow a rev that begins with a dash), to
 start allowing an option that does not begin with a dash someday.

 revision.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index eccf1ab695..0f772ba73d 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,6 +2203,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		int maybe_opt = 0;
+
 		if (*arg == '-') {
 			int opts;
 
@@ -2232,13 +2234,20 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 			if (opts < 0)
 				exit(128);
-			/* arg looks like an opt but something we do not recognise. */
-			argv[left++] = arg;
-			continue;
+			/*
+			 * arg looks like an opt but something we do not recognise.
+			 * It may be a rev that begins with a dash; fall through to
+			 * let handle_revision_arg() have a say in this.
+			 */
+			maybe_opt = 1;
 		}
 
 		if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
 			got_rev_arg = 1;
+		} else if (maybe_opt) {
+			/* it turns out that it is not a rev after all */
+			argv[left++] = arg;
+			continue;
 		} else {
 			int j;
 			if (seen_dashdash || *arg == '^')
-- 
2.12.0-rc1-212-ga9adfb24fa


^ permalink raw reply related

* Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Junio C Hamano @ 2017-02-12 18:56 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <20170212123630.GA20872@ubuntu-512mb-blr1-01.localdomain>

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> This "changing the order" gave me the idea to change the flow. I tried to
> implement the above steps without touching the function handle_revision_opt. By
> inserting the handle_revision_arg call just before calling handle_revision_opt.

Changing the order is changing the order of the function calls,
i.e. changing the flow.  So at the idea level we are on the same
page.

I was shooting for not having to duplicate calls to
handle_revision_arg().  

>> But I think the resulting code flow is much closer to the
>> above ideal.
>
> (about Junio's version of the patch): Yes, I agree with you on this. It's like
> the ideal, but the argv has already been populated, so the only remaining step
> is "left++".
>> 
>> Such a change to handle_revision_opt() unfortunately affects other
>> callers of the function, so it may not be worth it.

See the 3-patch series I just sent out.  I didn't think it through
very carefully (especially the error message the other caller
produces), but the whole thing _smells_ correct to me.

^ permalink raw reply

* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-12 19:30 UTC (permalink / raw)
  To: Jeff King
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski,
	Matthieu Moy
In-Reply-To: <20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net>

[+cc Matthieu Moy as author of a patch mentioned below]

On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:37PM +0000, Thomas Gummerer wrote:
> 
> > Thanks Junio for the review in the previous round.
> > 
> > Changes since v2:
> > 
> > - $IFS should now be supported by using "$@" everywhere instead of using
> >   a $files variable.
> > - Added a new patch showing the old behaviour of git stash create is
> >   preserved.
> > - Rephrased the documentation
> > - Simplified the option parsing in save_stash, by leaving the
> >   actual parsing to push_stash instead.
> 
> Overall, I like the direction this is heading. I raised a few issues,
> the most major of which is whether we want to allow the minor regression
> in "git stash create -m foo".
> 
> This also makes "git stash push -p <pathspec...>" work, which is good. I
> don't think "git stash -p <pathspec...>" does, though. I _think_ it
> would be trivial to do on top, since we already consider that case an
> error. That's somewhat outside the scope of your series, so I won't
> complain (too much :) ) if you don't dig into it, but it might just be
> trivial on top.

Hmm good idea, I think it would indeed be nice to add that.  Theres a
few things to consider though.  First, we need to switch git stash
without arguments over to use git stash push internally.  This does
introduce a slight regression as we currently allow git stash save --
-fmessage, (only messages starting with - are allowed).  I think that
regression would be acceptable however.

The other question is when we should allow the pathspec argument.
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options") made sure that all option arguments are treated the same.  I
think we could use the usual disambiguation of -- to allow pathspecs
after that, so stash -p wouldn't be treated specially, otherwise the
rules could get a bit confusing again.

The other question is how we would deal with the -m flag for
specifying a stash message.  I think we could special case it, but
that would allow users to do things such as git stash -m apply, which
would make the interface a bit more error prone.  So I'm leaning
towards disallowing that for now.

> A few other random bits I noticed while playing with the new code:
> 
>   $ git init
>   $ echo content >file && git add file && git commit -m file
>   $ echo change >file
> 
>   $ git stash push -p not-file
>   No changes.
>   No changes selected
> 
> Probably only one of those is necessary. :)
> 
> Let's keep trying a few things:
> 
>   $ git stash push not-file
>   Saved working directory and index state WIP on master: 5d5f951 file
>   Unstaged changes after reset:
>   M	file
>   M	file
> 
> The unstaged change is mentioned twice, which is weird. But weirder
> still is that we created a stash at all, as it's empty. Why didn't we
> hit the "no changes selected" path?
> 
> And one more:
> 
>   $ echo foo >untracked
>   $ git stash push untracked
>   Saved working directory and index state WIP on master: 5d5f951 file
>   Unstaged changes after reset:
>   M	file
>   M	file
>   Removing untracked
> 
> We removed the untracked file, even though it wasn't actually stashed! I
> thought at first this was because it was mentioned as a pathspec, but it
> seems to happen even with a different name:
> 
>   $ echo foo >untracked
>   $ git stash push does-not-exist
>   ...
>   Removing untracked
> 
> That seems dangerous.

Ouch, yeah this is clearly not good.  I'll fix this, and add tests for
these cases.

> -Peff

^ permalink raw reply

* [git-gui] Amending doesn't preserve timestamp
From: Juraj @ 2017-02-12 20:50 UTC (permalink / raw)
  To: git

I've just noticed that amending a commit from git-gui uses the time of
amending as the new timestamp of the commit, whereas git commit
--amend preserves the original timestamp. Maybe the two should work
the same, whatever it is decided to be the standard behavior.

Juraj

^ permalink raw reply

* Re: [PATCH] fetch: print an error when declining to request an unadvertised object
From: Matt McCutchen @ 2017-02-12 21:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwpcxlwpv.fsf@gitster.mtv.corp.google.com>

On Fri, 2017-02-10 at 10:36 -0800, Junio C Hamano wrote:
> > 
> There is this piece of code near the end of builtin/fetch-pack.c:
> 
> [...]
> 
> that happens before the command shows the list of fetched refs, and
> this code is prepared to inspect what happend to the requests it (in
> response to the user request) made to the underlying fetch
> machinery, and issue the error message.
> If you change your command line to "git fetch-pack REMOTE SHA1", you
> do see an error from the above.

Yes, "error: no such remote ref NNNN", which at least makes clear that
the operation didn't work, though it would be nice to give a more
specific error message.

> This all happens in transport.c::fetch_refs_via_pack().
> I think that function is a much better place to error or die than
> filter_refs().

I confirmed that checking the sought refs there works.  However, in
filter_refs, it's easy to give a more specific error message that the
server doesn't allow requests for unadvertised objects, and that code
works for "git fetch-pack" too.  To do the same in fetch_refs_via_pack,
we'd have to duplicate a few lines of code from filter_refs and expose
the allow_unadvertised_object_request variable, or just set a flag on
the "struct ref" in filter_refs and check it in fetch_refs_via_pack.

What do you think?  Do you not care about having a more specific error,
in which case I can copy the code from builtin/fetch-pack.c to
fetch_refs_via_pack?  Or shall I add code to filter_refs to set a flag
and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check
the flag?  Or what?

Matt

^ permalink raw reply

* Re: [git-gui] Amending doesn't preserve timestamp
From: Igor Djordjevic BugA @ 2017-02-12 21:33 UTC (permalink / raw)
  To: Juraj, git
In-Reply-To: <CAEPqvoxM9_Ku-1YgwNiqearUBaaYbAJcehuSwZYNNfJLQNH1_g@mail.gmail.com>

On 12/02/2017 21:50, Juraj wrote:
> I've just noticed that amending a commit from git-gui uses the time of
> amending as the new timestamp of the commit, whereas git commit
> --amend preserves the original timestamp. Maybe the two should work
> the same, whatever it is decided to be the standard behavior.
> 
> Juraj
> 

Hi Juraj,

Just to report that it seems to work as expected on my end...? Amending
through both command line and git-gui preserves author date and updates
commiter date.

git version 2.11.1.windows.1
git-gui version 0.21.GITGUI
Tcl/Tk version 8.6.6

Regards, BugA

^ permalink raw reply

* Re: [git-gui] Amending doesn't preserve timestamp
From: Juraj @ 2017-02-12 21:40 UTC (permalink / raw)
  To: Igor Djordjevic BugA; +Cc: git@vger.kernel.org
In-Reply-To: <8a1179dfbb7743b6b8c23570306120b7@MAIL.fer.hr>

Hi Igor,

I forgot to write the version I'm using. It's on Ubuntu 16.04, git-gui
package version 1:2.7.4-0ubuntu1 (--version: git-gui version 0.20.0),
git version 2.7.4, tcl and tk 8.6.0+9. Perhaps it got fixed in a newer
version, in that case, my bad for not checking before posting.

Thanks,
Juraj

^ permalink raw reply

* [PATCH v4 0/7] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-12 21:54 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski, Thomas Gummerer
In-Reply-To: <mailto:20170205202642.14216-1-t.gummerer@gmail.com>

Thanks Peff and Junio for the review of the last round.

Changes since v3:

- Instead of using ${1-...} to check for missing arguments and dying
  show the usage for stash instead, being more consistent with how the
  rest of stash behaves
- Let push_stash handle the --help argument from save_stash as well,
  passing it through directly.

- Fix the tests to not pipe the output to something, and thereby
  swallowing the error codes

- Update the commit message in 4/7 to acknowledge that there is a
  small regression between the two versions

- Correctly detect when there are no changes, and only show the
  message once
- Fix the accidental deletion of untracked files when a pathspec is
  used.  Also added test cases for that.
- Add documentation for git stash push to the usage text
- Respect the -q flag and the GIT_QUIET environment variable

- This also adds two new patches, one using push_stash instead of
  save_stash for git stash without any verb.  The other allows using a
  pathspec in the verb-less option of git stash.

Interdiff below:
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index be55e456fa..b0825f4aca 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,15 +13,15 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
-	     [-u|--include-untracked] [-a|--all] [<message>]]
-'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+	     [-u|--include-untracked] [-a|--all] [<message>]
+'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 	     [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
-	     [--] [<pathspec>...]
+	     [--] [<pathspec>...]]
 'git stash' clear
 'git stash' create [<message>]
 'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
-	     [-- <paths>...]
+	     [-- <pathspec>...]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 
 DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index 46367d40aa..a184b1e274 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,8 +7,11 @@ USAGE="list [<options>]
    or: $dashless drop [-q|--quiet] [<stash>]
    or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
    or: $dashless branch <branchname> [<stash>]
-   or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
-		       [-u|--include-untracked] [-a|--all] [<message>]]
+   or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+		      [-u|--include-untracked] [-a|--all] [<message>]
+   or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+		       [-u|--include-untracked] [-a|--all] [-m <message>]
+		       [-- <pathspec>...]]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -33,8 +36,8 @@ else
 fi
 
 no_changes () {
-	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
-	git diff-files --quiet --ignore-submodules &&
+	git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
+	git diff-files --quiet --ignore-submodules -- "$@" &&
 	(test -z "$untracked" || test -z "$(untracked_files)")
 }
 
@@ -64,11 +67,13 @@ create_stash () {
 		case "$1" in
 		-m|--message)
 			shift
-			stash_msg=${1?"-m needs an argument"}
+			test -z ${1+x} && usage
+			stash_msg="$1"
 			new_style=t
 			;;
 		-u|--include-untracked)
 			shift
+			test -z ${1+x} && usage
 			untracked="$1"
 			new_style=t
 			;;
@@ -104,14 +109,11 @@ create_stash () {
 	if test -z "$new_style"
 	then
 		stash_msg="$*"
-		while test $# != 0
-		do
-			shift
-		done
+		set --
 	fi
 
 	git update-index -q --refresh
-	if no_changes
+	if no_changes "$@"
 	then
 		exit 0
 	fi
@@ -270,7 +272,8 @@ push_stash () {
 			;;
 		-m|--message)
 			shift
-			stash_msg=${1?"-m needs an argument"}
+			test -z ${1+x} && usage
+			stash_msg=$1
 			;;
 		--help)
 			show_help
@@ -308,7 +311,7 @@ push_stash () {
 	fi
 
 	git update-index -q --refresh
-	if no_changes
+	if no_changes "$@"
 	then
 		say "$(gettext "No local changes to save")"
 		exit 0
@@ -325,9 +328,23 @@ push_stash () {
 	then
 		if test $# != 0
 		then
-			git ls-files -z -- "$@" | xargs -0 git reset --
-			git ls-files -z --modified -- "$@" | xargs -0 git checkout HEAD --
-			git ls-files -z --others -- "$@" | xargs -0 git clean --force --
+			saved_untracked=
+			if test -n "$(git ls-files --others -- "$@")"
+			then
+				saved_untracked=$(
+					git ls-files -z --others -- "$@" |
+					    xargs -0 git stash create -u all --)
+			fi
+			git ls-files -z -- "$@" | xargs -0 git reset ${GIT_QUIET:+-q} --
+			git ls-files -z --modified -- "$@" | xargs -0 git checkout ${GIT_QUIET:+-q} HEAD --
+			if test -n "$(git ls-files -z --others -- "$@")"
+			then
+				git ls-files -z --others -- "$@" | xargs -0 git clean --force -d ${GIT_QUIET:+-q} --
+			fi
+			if test -n "$saved_untracked"
+			then
+				git stash pop -q $saved_untracked
+			fi
 		else
 			git reset --hard ${GIT_QUIET:+-q}
 		fi
@@ -357,9 +374,6 @@ save_stash () {
 	while test $# != 0
 	do
 		case "$1" in
-		--help)
-			show_help
-			;;
 		--)
 			shift
 			break
@@ -685,17 +699,18 @@ apply_to_branch () {
 }
 
 PARSE_CACHE='--not-parsed'
-# The default command is "save" if nothing but options are given
+# The default command is "push" if nothing but options are given
 seen_non_option=
 for opt
 do
 	case "$opt" in
+	--) break ;;
 	-*) ;;
 	*) seen_non_option=t; break ;;
 	esac
 done
 
-test -n "$seen_non_option" || set "save" "$@"
+test -n "$seen_non_option" || set "push" "$@"
 
 # Main command set
 case "$1" in
@@ -746,7 +761,7 @@ branch)
 *)
 	case $# in
 	0)
-		save_stash &&
+		push_stash &&
 		say "$(gettext "(To restore them type \"git stash apply\")")"
 		;;
 	*)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 461fe46045..22ac75377b 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -274,9 +274,7 @@ test_expect_success 'stash --invalid-option' '
 	git add file2 &&
 	test_must_fail git stash --invalid-option &&
 	test_must_fail git stash save --invalid-option &&
-	test bar5,bar6 = $(cat file),$(cat file2) &&
-	git stash -- -message-starting-with-dash &&
-	test bar,bar2 = $(cat file),$(cat file2)
+	test bar5,bar6 = $(cat file),$(cat file2)
 '
 
 test_expect_success 'stash an added file' '
@@ -780,7 +778,7 @@ test_expect_success 'push -m shows right message' '
 	git add foo &&
 	git stash push -m "test message" &&
 	echo "stash@{0}: On master: test message" >expect &&
-	git stash list | head -n 1 >actual &&
+	git stash list -1 >actual &&
 	test_cmp expect actual
 '
 
@@ -789,7 +787,7 @@ test_expect_success 'create stores correct message' '
 	git add foo &&
 	STASH_ID=$(git stash create "create test message") &&
 	echo "On master: create test message" >expect &&
-	git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
 	test_cmp expect actual
 '
 
@@ -798,7 +796,7 @@ test_expect_success 'create with multiple arguments for the message' '
 	git add foo &&
 	STASH_ID=$(git stash create test untracked) &&
 	echo "On master: test untracked" >expect &&
-	git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
 	test_cmp expect actual
 '
 
@@ -807,7 +805,7 @@ test_expect_success 'new style stash create stores correct message' '
 	git add foo &&
 	STASH_ID=$(git stash create -m "create test message new style") &&
 	echo "On master: create test message new style" >expect &&
-	git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
 	test_cmp expect actual
 '
 
@@ -839,18 +837,63 @@ test_expect_success 'stash with multiple filename arguments' '
 '
 
 test_expect_success 'stash with file including $IFS character' '
-	>"foo	bar" &&
+	>"foo bar" &&
 	>foo &&
-	>untracked &&
+	>bar &&
 	git add foo* &&
-	git stash push -- foo* &&
-	test_path_is_missing "foo	bar" &&
-	test_path_is_missing foo &&
-	test_path_is_file untracked &&
+	git stash push -- "foo b*" &&
+	test_path_is_missing "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar &&
 	git stash pop &&
-	test_path_is_file "foo	bar" &&
+	test_path_is_file "foo bar" &&
 	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
+test_expect_success 'stash push -p with pathspec shows no changes only onece' '
+	>file &&
+	git add file &&
+	git stash push -p not-file >actual &&
+	echo "No local changes to save" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'stash push with pathspec shows no changes when there are none' '
+	>file &&
+	git add file &&
+	git stash push not-file >actual &&
+	echo "No local changes to save" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'untracked file is not removed when using pathspecs' '
+	>untracked &&
+	git stash push untracked &&
+	test_path_is_file untracked
+'
+
+test_expect_success 'untracked files are left in place when -u is not given' '
+	>file &&
+	git add file &&
+	>untracked &&
+	git stash push file &&
 	test_path_is_file untracked
 '
 
+test_expect_success 'stash without verb with pathspec' '
+	>"foo bar" &&
+	>foo &&
+	>bar &&
+	git add foo* &&
+	git stash -- "foo b*" &&
+	test_path_is_missing "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar &&
+	git stash pop &&
+	test_path_is_file "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
 test_done
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index 9a98e31a3e..193adc7b68 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -197,16 +197,16 @@ test_expect_success 'stash push --include-untracked with pathspec' '
 '
 
 test_expect_success 'stash push with $IFS character' '
-	>"foo	bar" &&
+	>"foo bar" &&
 	>foo &&
 	>bar &&
 	git add foo* &&
-	git stash push --include-untracked -- foo* &&
-	test_path_is_missing "foo	bar" &&
-	test_path_is_missing foo &&
+	git stash push --include-untracked -- "foo b*" &&
+	test_path_is_missing "foo bar" &&
+	test_path_is_file foo &&
 	test_path_is_file bar &&
 	git stash pop &&
-	test_path_is_file "foo	bar" &&
+	test_path_is_file "foo bar" &&
 	test_path_is_file foo &&
 	test_path_is_file bar
 '

Thomas Gummerer (7):
  Documentation/stash: remove mention of git reset --hard
  stash: introduce push verb
  stash: add test for the create command line arguments
  stash: introduce new format create
  stash: teach 'push' (and 'create') to honor pathspec
  stash: use stash_push for no verb form
  stash: allow pathspecs in the no verb form

 Documentation/git-stash.txt        |  21 ++++-
 git-stash.sh                       | 157 +++++++++++++++++++++++++++++++------
 t/t3903-stash.sh                   | 127 +++++++++++++++++++++++++++++-
 t/t3905-stash-include-untracked.sh |  26 ++++++
 4 files changed, 302 insertions(+), 29 deletions(-)

-- 
2.11.0.301.g86e6ecc671.dirty


^ permalink raw reply related

* [PATCH v4 1/7] Documentation/stash: remove mention of git reset --hard
From: Thomas Gummerer @ 2017-02-12 21:54 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski, Thomas Gummerer
In-Reply-To: <20170212215420.16701-1-t.gummerer@gmail.com>

Don't mention git reset --hard in the documentation for git stash save.
It's an implementation detail that doesn't matter to the end user and
thus shouldn't be exposed to them.  In addition it's not quite true for
git stash -p, and will not be true when a filename argument to limit the
stash to a few files is introduced.

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

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e6..2e9e344cd7 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -47,8 +47,9 @@ OPTIONS
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
 
-	Save your local modifications to a new 'stash', and run `git reset
-	--hard` to revert them.  The <message> part is optional and gives
+	Save your local modifications to a new 'stash' and roll them
+	back to HEAD (in the working tree and in the index).
+	The <message> part is optional and gives
 	the description along with the stashed state.  For quickly making
 	a snapshot, you can omit _both_ "save" and <message>, but giving
 	only <message> does not trigger this action to prevent a misspelled
-- 
2.11.0.301.g86e6ecc671.dirty


^ permalink raw reply related

* [PATCH v4 2/7] stash: introduce push verb
From: Thomas Gummerer @ 2017-02-12 21:54 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski, Thomas Gummerer
In-Reply-To: <20170212215420.16701-1-t.gummerer@gmail.com>

Introduce a new git stash push verb in addition to git stash save.  The
push verb is used to transition from the current command line arguments
to a more conventional way, in which the message is given as an argument
to the -m option.

This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say which
subset of paths to stash (and leave others behind).

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh     | 46 +++++++++++++++++++++++++++++++++++++++++++---
 t/t3903-stash.sh |  9 +++++++++
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..8365ebba2a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -9,6 +9,8 @@ USAGE="list [<options>]
    or: $dashless branch <branchname> [<stash>]
    or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
 		       [-u|--include-untracked] [-a|--all] [<message>]]
+   or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+		      [-u|--include-untracked] [-a|--all] [-m <message>]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -189,10 +191,11 @@ store_stash () {
 	return $ret
 }
 
-save_stash () {
+push_stash () {
 	keep_index=
 	patch_mode=
 	untracked=
+	stash_msg=
 	while test $# != 0
 	do
 		case "$1" in
@@ -216,6 +219,11 @@ save_stash () {
 		-a|--all)
 			untracked=all
 			;;
+		-m|--message)
+			shift
+			test -z ${1+x} && usage
+			stash_msg=$1
+			;;
 		--help)
 			show_help
 			;;
@@ -251,8 +259,6 @@ save_stash () {
 		die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
 	fi
 
-	stash_msg="$*"
-
 	git update-index -q --refresh
 	if no_changes
 	then
@@ -291,6 +297,36 @@ save_stash () {
 	fi
 }
 
+save_stash () {
+	push_options=
+	while test $# != 0
+	do
+		case "$1" in
+		--)
+			shift
+			break
+			;;
+		-*)
+			# pass all options through to push_stash
+			push_options="$push_options $1"
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
+	stash_msg="$*"
+
+	if test -z "$stash_msg"
+	then
+		push_stash $push_options
+	else
+		push_stash $push_options -m "$stash_msg"
+	fi
+}
+
 have_stash () {
 	git rev-parse --verify --quiet $ref_stash >/dev/null
 }
@@ -617,6 +653,10 @@ save)
 	shift
 	save_stash "$@"
 	;;
+push)
+	shift
+	push_stash "$@"
+	;;
 apply)
 	shift
 	apply_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..3577115807 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial renames' '
 	test_path_is_missing file
 '
 
+test_expect_success 'push -m shows right message' '
+	>foo &&
+	git add foo &&
+	git stash push -m "test message" &&
+	echo "stash@{0}: On master: test message" >expect &&
+	git stash list -1 >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty


^ permalink raw reply related

* [PATCH v4 6/7] stash: use stash_push for no verb form
From: Thomas Gummerer @ 2017-02-12 21:54 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski, Thomas Gummerer
In-Reply-To: <20170212215420.16701-1-t.gummerer@gmail.com>

Now that we have stash_push, which accepts pathspec arguments, use
it instead of stash_save in git stash without any additional verbs.

This does introduce a small regression.  Previously we allowed
git stash -- -message, with a hyphen in front of the message.  However
git stash -- message without the hyphen was not allowed.  After this
change adding a message to the stash, with or without hyphen is no
longer allowed.

While this now allows specifying a message with the -m flag, it also
disallows messages without a hyphen.  This is to prevent user errors,
where a user tries to stash something with a message, but changes their
mind, and now would like to apply a stash, but forgets to remove the -m
flag.  E.g. git stash -m mes^H^H^Happly would result in a stash with the
message apply, while the user might have intended to apply a previous
stash.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---

If this kind of regression introduced here is not acceptable, I think
we could hide this change between some kind of config option, and
transition users over later after a warning period.

 Documentation/git-stash.txt |  8 ++++----
 git-stash.sh                | 16 ++++++++--------
 t/t3903-stash.sh            |  4 +---
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index f462f393b0..b0825f4aca 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,11 +13,11 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
-	     [-u|--include-untracked] [-a|--all] [<message>]]
-'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+	     [-u|--include-untracked] [-a|--all] [<message>]
+'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 	     [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
-	     [--] [<pathspec>...]
+	     [--] [<pathspec>...]]
 'git stash' clear
 'git stash' create [<message>]
 'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
diff --git a/git-stash.sh b/git-stash.sh
index db56cf2c66..769cee9fd8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,11 +7,11 @@ USAGE="list [<options>]
    or: $dashless drop [-q|--quiet] [<stash>]
    or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
    or: $dashless branch <branchname> [<stash>]
-   or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
-		       [-u|--include-untracked] [-a|--all] [<message>]]
-   or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
-		      [-u|--include-untracked] [-a|--all] [-m <message>]
-		      [-- <pathspec>...]
+   or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+		      [-u|--include-untracked] [-a|--all] [<message>]
+   or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+		       [-u|--include-untracked] [-a|--all] [-m <message>]
+		       [-- <pathspec>...]]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -699,7 +699,7 @@ apply_to_branch () {
 }
 
 PARSE_CACHE='--not-parsed'
-# The default command is "save" if nothing but options are given
+# The default command is "push" if nothing but options are given
 seen_non_option=
 for opt
 do
@@ -709,7 +709,7 @@ do
 	esac
 done
 
-test -n "$seen_non_option" || set "save" "$@"
+test -n "$seen_non_option" || set "push" "$@"
 
 # Main command set
 case "$1" in
@@ -760,7 +760,7 @@ branch)
 *)
 	case $# in
 	0)
-		save_stash &&
+		push_stash &&
 		say "$(gettext "(To restore them type \"git stash apply\")")"
 		;;
 	*)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 8b372c35fb..d568799da9 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -274,9 +274,7 @@ test_expect_success 'stash --invalid-option' '
 	git add file2 &&
 	test_must_fail git stash --invalid-option &&
 	test_must_fail git stash save --invalid-option &&
-	test bar5,bar6 = $(cat file),$(cat file2) &&
-	git stash -- -message-starting-with-dash &&
-	test bar,bar2 = $(cat file),$(cat file2)
+	test bar5,bar6 = $(cat file),$(cat file2)
 '
 
 test_expect_success 'stash an added file' '
-- 
2.11.0.301.g86e6ecc671.dirty


^ permalink raw reply related

* [PATCH v4 5/7] stash: teach 'push' (and 'create') to honor pathspec
From: Thomas Gummerer @ 2017-02-12 21:54 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski, Thomas Gummerer
In-Reply-To: <20170212215420.16701-1-t.gummerer@gmail.com>

While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone.  Unfortunately
git currently offers no such option.  git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.

Allow 'git stash push' to take pathspec to specify which paths to stash.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-stash.txt        | 11 ++++++
 git-stash.sh                       | 50 ++++++++++++++++++++------
 t/t3903-stash.sh                   | 72 ++++++++++++++++++++++++++++++++++++++
 t/t3905-stash-include-untracked.sh | 26 ++++++++++++++
 4 files changed, 148 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index a138ed6a24..f462f393b0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,9 +15,13 @@ SYNOPSIS
 'git stash' branch <branchname> [<stash>]
 'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 	     [-u|--include-untracked] [-a|--all] [<message>]]
+'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+	     [-u|--include-untracked] [-a|--all] [-m|--message <message>]]
+	     [--] [<pathspec>...]
 'git stash' clear
 'git stash' create [<message>]
 'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
+	     [-- <pathspec>...]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 
 DESCRIPTION
@@ -47,6 +51,7 @@ OPTIONS
 -------
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
 
 	Save your local modifications to a new 'stash' and roll them
 	back to HEAD (in the working tree and in the index).
@@ -56,6 +61,12 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
 	only <message> does not trigger this action to prevent a misspelled
 	subcommand from making an unwanted stash.
 +
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec.  The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
++
 If the `--keep-index` option is used, all changes already added to the
 index are left intact.
 +
diff --git a/git-stash.sh b/git-stash.sh
index 6d629fc43f..db56cf2c66 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -11,6 +11,7 @@ USAGE="list [<options>]
 		       [-u|--include-untracked] [-a|--all] [<message>]]
    or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
 		      [-u|--include-untracked] [-a|--all] [-m <message>]
+		      [-- <pathspec>...]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -35,15 +36,15 @@ else
 fi
 
 no_changes () {
-	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
-	git diff-files --quiet --ignore-submodules &&
+	git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
+	git diff-files --quiet --ignore-submodules -- "$@" &&
 	(test -z "$untracked" || test -z "$(untracked_files)")
 }
 
 untracked_files () {
 	excl_opt=--exclude-standard
 	test "$untracked" = "all" && excl_opt=
-	git ls-files -o -z $excl_opt
+	git ls-files -o -z $excl_opt -- "$@"
 }
 
 clear_stash () {
@@ -76,6 +77,11 @@ create_stash () {
 			untracked="$1"
 			new_style=t
 			;;
+		--)
+			shift
+			new_style=t
+			break
+			;;
 		*)
 			if test -n "$new_style"
 			then
@@ -103,10 +109,11 @@ create_stash () {
 	if test -z "$new_style"
 	then
 		stash_msg="$*"
+		set --
 	fi
 
 	git update-index -q --refresh
-	if no_changes
+	if no_changes "$@"
 	then
 		exit 0
 	fi
@@ -138,7 +145,7 @@ create_stash () {
 		# Untracked files are stored by themselves in a parentless commit, for
 		# ease of unpacking later.
 		u_commit=$(
-			untracked_files | (
+			untracked_files "$@" | (
 				GIT_INDEX_FILE="$TMPindex" &&
 				export GIT_INDEX_FILE &&
 				rm -f "$TMPindex" &&
@@ -161,7 +168,7 @@ create_stash () {
 			git read-tree --index-output="$TMPindex" -m $i_tree &&
 			GIT_INDEX_FILE="$TMPindex" &&
 			export GIT_INDEX_FILE &&
-			git diff-index --name-only -z HEAD -- >"$TMP-stagenames" &&
+			git diff-index --name-only -z HEAD -- "$@" >"$TMP-stagenames" &&
 			git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
 			git write-tree &&
 			rm -f "$TMPindex"
@@ -175,7 +182,7 @@ create_stash () {
 
 		# find out what the user wants
 		GIT_INDEX_FILE="$TMP-index" \
-			git add--interactive --patch=stash -- &&
+			git add--interactive --patch=stash -- "$@" &&
 
 		# state of the working tree
 		w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) ||
@@ -304,7 +311,7 @@ push_stash () {
 	fi
 
 	git update-index -q --refresh
-	if no_changes
+	if no_changes "$@"
 	then
 		say "$(gettext "No local changes to save")"
 		exit 0
@@ -312,18 +319,39 @@ push_stash () {
 	git reflog exists $ref_stash ||
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
-	create_stash -m "$stash_msg" -u "$untracked"
+	create_stash -m "$stash_msg" -u "$untracked" -- "$@"
 	store_stash -m "$stash_msg" -q $w_commit ||
 	die "$(gettext "Cannot save the current status")"
 	say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
 
 	if test -z "$patch_mode"
 	then
-		git reset --hard ${GIT_QUIET:+-q}
+		if test $# != 0
+		then
+			saved_untracked=
+			if test -n "$(git ls-files --others -- "$@")"
+			then
+				saved_untracked=$(
+					git ls-files -z --others -- "$@" |
+					    xargs -0 git stash create -u all --)
+			fi
+			git ls-files -z -- "$@" | xargs -0 git reset ${GIT_QUIET:+-q} --
+			git ls-files -z --modified -- "$@" | xargs -0 git checkout ${GIT_QUIET:+-q} HEAD --
+			if test -n "$(git ls-files -z --others -- "$@")"
+			then
+				git ls-files -z --others -- "$@" | xargs -0 git clean --force -d ${GIT_QUIET:+-q} --
+			fi
+			if test -n "$saved_untracked"
+			then
+				git stash pop -q $saved_untracked
+			fi
+		else
+			git reset --hard ${GIT_QUIET:+-q}
+		fi
 		test "$untracked" = "all" && CLEAN_X_OPTION=-x || CLEAN_X_OPTION=
 		if test -n "$untracked"
 		then
-			git clean --force --quiet -d $CLEAN_X_OPTION
+			git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
 		fi
 
 		if test "$keep_index" = "t" && test -n "$i_tree"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 812d0f7a40..8b372c35fb 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -811,4 +811,76 @@ test_expect_success 'new style stash create stores correct message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'stash -- <filename> stashes and restores the file' '
+	>foo &&
+	>bar &&
+	git add foo bar &&
+	git stash push -- foo &&
+	test_path_is_file bar &&
+	test_path_is_missing foo &&
+	git stash pop &&
+	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
+test_expect_success 'stash with multiple filename arguments' '
+	>foo &&
+	>bar &&
+	>extra &&
+	git add foo bar extra &&
+	git stash push -- foo bar &&
+	test_path_is_missing bar &&
+	test_path_is_missing foo &&
+	test_path_is_file extra &&
+	git stash pop &&
+	test_path_is_file foo &&
+	test_path_is_file bar &&
+	test_path_is_file extra
+'
+
+test_expect_success 'stash with file including $IFS character' '
+	>"foo bar" &&
+	>foo &&
+	>bar &&
+	git add foo* &&
+	git stash push -- "foo b*" &&
+	test_path_is_missing "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar &&
+	git stash pop &&
+	test_path_is_file "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
+test_expect_success 'stash push -p with pathspec shows no changes only onece' '
+	>file &&
+	git add file &&
+	git stash push -p not-file >actual &&
+	echo "No local changes to save" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'stash push with pathspec shows no changes when there are none' '
+	>file &&
+	git add file &&
+	git stash push not-file >actual &&
+	echo "No local changes to save" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'untracked file is not removed when using pathspecs' '
+	>untracked &&
+	git stash push untracked &&
+	test_path_is_file untracked
+'
+
+test_expect_success 'untracked files are left in place when -u is not given' '
+	>file &&
+	git add file &&
+	>untracked &&
+	git stash push file &&
+	test_path_is_file untracked
+'
+
 test_done
diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
index f372fc8ca8..193adc7b68 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -185,4 +185,30 @@ test_expect_success 'stash save --all is stash poppable' '
 	test -s .gitignore
 '
 
+test_expect_success 'stash push --include-untracked with pathspec' '
+	>foo &&
+	>bar &&
+	git stash push --include-untracked -- foo &&
+	test_path_is_file bar &&
+	test_path_is_missing foo &&
+	git stash pop &&
+	test_path_is_file bar &&
+	test_path_is_file foo
+'
+
+test_expect_success 'stash push with $IFS character' '
+	>"foo bar" &&
+	>foo &&
+	>bar &&
+	git add foo* &&
+	git stash push --include-untracked -- "foo b*" &&
+	test_path_is_missing "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar &&
+	git stash pop &&
+	test_path_is_file "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty


^ permalink raw reply related

* [PATCH v4 4/7] stash: introduce new format create
From: Thomas Gummerer @ 2017-02-12 21:54 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski, Thomas Gummerer
In-Reply-To: <20170212215420.16701-1-t.gummerer@gmail.com>

git stash create currently supports a positional argument for adding a
message.  This is not quite in line with how git commands usually take
comments (using a -m flag).

Add a new syntax for adding a message to git stash create using a -m
flag.  This is with the goal of deprecating the old style git stash
create with positional arguments.

This also adds a -u argument, for untracked files.  This is already used
internally as another positional argument, but can now be used from the
command line.

This introduces a slight regression, when git stash create -m works is
used.  Before this change, it created a stash with the message
"-m works", but now it creates a stash with the message "-m".

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-stash.txt |  1 +
 git-stash.sh                | 52 +++++++++++++++++++++++++++++++++++++++++----
 t/t3903-stash.sh            |  9 ++++++++
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9e344cd7..a138ed6a24 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 	     [-u|--include-untracked] [-a|--all] [<message>]]
 'git stash' clear
 'git stash' create [<message>]
+'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
 
 DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index 8365ebba2a..6d629fc43f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -58,8 +58,52 @@ clear_stash () {
 }
 
 create_stash () {
-	stash_msg="$1"
-	untracked="$2"
+	stash_msg=
+	untracked=
+	new_style=
+	while test $# != 0
+	do
+		case "$1" in
+		-m|--message)
+			shift
+			test -z ${1+x} && usage
+			stash_msg="$1"
+			new_style=t
+			;;
+		-u|--include-untracked)
+			shift
+			test -z ${1+x} && usage
+			untracked="$1"
+			new_style=t
+			;;
+		*)
+			if test -n "$new_style"
+			then
+				echo "invalid argument"
+				option="$1"
+				# TRANSLATORS: $option is an invalid option, like
+				# `--blah-blah'. The 7 spaces at the beginning of the
+				# second line correspond to "error: ". So you should line
+				# up the second line with however many characters the
+				# translation of "error: " takes in your language. E.g. in
+				# English this is:
+				#
+				#    $ git stash save --blah-blah 2>&1 | head -n 2
+				#    error: unknown option for 'stash save': --blah-blah
+				#           To provide a message, use git stash save -- '--blah-blah'
+				eval_gettextln "error: unknown option for 'stash create': \$option"
+				usage
+			fi
+			break
+			;;
+		esac
+		shift
+	done
+
+	if test -z "$new_style"
+	then
+		stash_msg="$*"
+	fi
 
 	git update-index -q --refresh
 	if no_changes
@@ -268,7 +312,7 @@ push_stash () {
 	git reflog exists $ref_stash ||
 		clear_stash || die "$(gettext "Cannot initialize stash")"
 
-	create_stash "$stash_msg" $untracked
+	create_stash -m "$stash_msg" -u "$untracked"
 	store_stash -m "$stash_msg" -q $w_commit ||
 	die "$(gettext "Cannot save the current status")"
 	say "$(eval_gettext "Saved working directory and index state \$stash_msg")"
@@ -667,7 +711,7 @@ clear)
 	;;
 create)
 	shift
-	create_stash "$*" && echo "$w_commit"
+	create_stash "$@" && echo "$w_commit"
 	;;
 store)
 	shift
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ffe3549ea5..812d0f7a40 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -802,4 +802,13 @@ test_expect_success 'create with multiple arguments for the message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'new style stash create stores correct message' '
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create -m "create test message new style") &&
+	echo "On master: create test message new style" >expect &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty


^ permalink raw reply related

* [PATCH v4 3/7] stash: add test for the create command line arguments
From: Thomas Gummerer @ 2017-02-12 21:54 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski, Thomas Gummerer
In-Reply-To: <20170212215420.16701-1-t.gummerer@gmail.com>

Currently there is no test showing the expected behaviour of git stash
create's command line arguments.  Add a test for that to show the
current expected behaviour and to make sure future refactorings don't
break those expectations.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 t/t3903-stash.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3577115807..ffe3549ea5 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create stores correct message' '
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create "create test message") &&
+	echo "On master: create test message" >expect &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'create with multiple arguments for the message' '
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create test untracked) &&
+	echo "On master: test untracked" >expect &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty


^ permalink raw reply related

* [PATCH v4 7/7] stash: allow pathspecs in the no verb form
From: Thomas Gummerer @ 2017-02-12 21:54 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski, Thomas Gummerer
In-Reply-To: <20170212215420.16701-1-t.gummerer@gmail.com>

Now that stash_push is used in the no verb form of stash, allow
specifying the command line for this form as well.  Always use -- to
disambiguate pathspecs from other non-option arguments.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 git-stash.sh     |  1 +
 t/t3903-stash.sh | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/git-stash.sh b/git-stash.sh
index 769cee9fd8..a184b1e274 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -704,6 +704,7 @@ seen_non_option=
 for opt
 do
 	case "$opt" in
+	--) break ;;
 	-*) ;;
 	*) seen_non_option=t; break ;;
 	esac
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index d568799da9..22ac75377b 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -881,4 +881,19 @@ test_expect_success 'untracked files are left in place when -u is not given' '
 	test_path_is_file untracked
 '
 
+test_expect_success 'stash without verb with pathspec' '
+	>"foo bar" &&
+	>foo &&
+	>bar &&
+	git add foo* &&
+	git stash -- "foo b*" &&
+	test_path_is_missing "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar &&
+	git stash pop &&
+	test_path_is_file "foo bar" &&
+	test_path_is_file foo &&
+	test_path_is_file bar
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty


^ permalink raw reply related

* Re: [PATCH v4 0/7] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-12 22:07 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Johannes Schindelin,
	Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170212215420.16701-1-t.gummerer@gmail.com>

On 02/12, Thomas Gummerer wrote:
> Thanks Peff and Junio for the review of the last round.
>

Sorry it seems like I messed up the In-Reply-To header.  Previous rounds were at:
v1: http://public-inbox.org/git/20170121200804.19009-1-t.gummerer@gmail.com/
v2: http://public-inbox.org/git/20170129201604.30445-1-t.gummerer@gmail.com/
v3: http://public-inbox.org/git/20170205202642.14216-1-t.gummerer@gmail.com/

^ permalink raw reply

* Re: What's cooking in git.git (Feb 2017, #03; Fri, 10)
From: René Scharfe @ 2017-02-12 23:01 UTC (permalink / raw)
  To: Vegard Nossum, Junio C Hamano; +Cc: git
In-Reply-To: <36fdf8c9-f82d-4996-09ef-7d718ab3ac41@oracle.com>

Am 12.02.2017 um 19:32 schrieb Vegard Nossum:
> I said I would resubmit the patches with more config options and more
> command-line arguments (to avoid potentially breaking backwards
> compatibility), but IIRC the response seemed to be "preceding blank line
> heuristic is good enough" and "why bother", so I ended up not not
> resubmitting anything.

I was (and still am) looking forward to your patches.  The current 
heuristic is simplistic, the patches you already sent improve the output 
in certain scenarios, my proposed changes on top aimed to limit 
drawbacks in other scenarios, but together they still have shortcomings.

Avoiding new switches would be nice, though (if possible).  I feel we 
need a lot more tests to nail down our expectations.

René

^ permalink raw reply

* Re: [git-gui] Amending doesn't preserve timestamp
From: Igor Djordjevic BugA @ 2017-02-12 23:06 UTC (permalink / raw)
  To: Juraj; +Cc: git@vger.kernel.org
In-Reply-To: <CAEPqvozCvFqOTNPw0c1ErKoOd+Mn1WCyJr9hj6CXiWWtL93Tqw@mail.gmail.com>

On 12/02/2017 22:40, Juraj wrote:
> Hi Igor,
> 
> I forgot to write the version I'm using. It's on Ubuntu 16.04, git-gui
> package version 1:2.7.4-0ubuntu1 (--version: git-gui version 0.20.0),
> git version 2.7.4, tcl and tk 8.6.0+9. Perhaps it got fixed in a newer
> version, in that case, my bad for not checking before posting.
> 
> Thanks,
> Juraj

Hi Juraj,

Indeed, if I`m reading it correctly, it seems to be addressed in git-gui
version 0.21.0[1], introduced in git version 2.11.0[2] on 2016-11-29
("git-gui: Do not reset author details on amend", 2016-04-11[3],
referencing an old bug report[4]).

Regards,
BugA

[1] https://public-inbox.org/git/878ttji701.fsf@red.patthoyts.tk/
[2]
https://public-inbox.org/git/xmqqmvgidlsg.fsf@gitster.mtv.corp.google.com/
[3]
https://public-inbox.org/git/1462458182-4488-1-git-send-email-orgads@gmail.com/
[4]
https://public-inbox.org/git/CAGHpTB+35j0njmCZ0uCgBVroe=Ma7HLnn6fDty8yebKWgEmECg@mail.gmail.com/

^ permalink raw reply

* Re: [PATCH] fetch: print an error when declining to request an unadvertised object
From: Junio C Hamano @ 2017-02-12 23:49 UTC (permalink / raw)
  To: Matt McCutchen; +Cc: git
In-Reply-To: <1486934007.8517.10.camel@mattmccutchen.net>

Matt McCutchen <matt@mattmccutchen.net> writes:

> What do you think?  Do you not care about having a more specific error,
> in which case I can copy the code from builtin/fetch-pack.c to
> fetch_refs_via_pack?  Or shall I add code to filter_refs to set a flag
> and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check
> the flag?  Or what?

The fact that we have the above two choices tells me that a two-step
approach may be an appropriate approach.

The first step is to teach fetch_refs_via_pack() that it should not
ignore the information returned in sought[].  It would add new code
similar to what cmd_fetch_pack() uses to notice and report errors
[*1*] to the function.  It would be a sensible first step, but would
not let the user know which of multiple causes of "not matched" we
noticed.

By "a more specific error", I think you are envisioning that the
current boolean "matched" is made into an enum that allows the
caller to tell how each request did not match [*2*].  That can be
the topic of the second patch and would have to touch filter_refs()
[*3*], cmd_fetch_pack() and fetch_refs_via_pack().

I do not have strong preference myself offhand between stopping at
the first step or completing both.

Even if you did only the first step, as long as the second step can
be done without reverting what the first step did [*4*] by somebody
who cares the "specific error" deeply enough, I am OK with that.  Of
course if you did both steps, that is fine by me as well ;-)


[Footnote]

*1* While I know that it is not right to die() in filter_refs(), and
    fetch_refs_via_pack() is a better place to notice errors, I do
    not offhand know if it is the right place to report errors, or a
    caller higher in the callchain may want the callee to be silent
    and wants to show its own error message (in which case the error
    may have to percolate upwards in the callchain).

*2* e.g. "was it a ref but they did not advertise?  Did it request
    an explicit object name and they did not allow it?"  We may want
    to support other "more specific" errors that can be detected in
    the future.

*3* The current code flips the sought[i]->matched bit on for matched
    ones (relying on the initial state of the bit being false), but
    it now needs to stuff different kind of "not matched" to the
    field to allow the caller to act on it.

*4* IOW, I am OK with an initial "small" improvement, but I'd want
    to make sure that such an initial step does not make future
    enhancements by others harder.

^ permalink raw reply

* [PATCH] completion: teach to complete git-subtree
From: cornelius.weig @ 2017-02-13  1:07 UTC (permalink / raw)
  To: git
  Cc: bitte.keine.werbung.einwerfen, Cornelius Weig, szeder.dev, s6t,
	greened, davvid

From: Cornelius Weig <cornelius.weig@tngtech.com>

Git-subtree is a contrib-command without completion, making it
cumbersome to use.

Teach bash completion to complete the subcommands of subtree (add,
merge, pull, push, split) and options thereof. For subcommands
supporting it (add, push, pull) also complete remote names and refspec.

Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
 contrib/completion/git-completion.bash | 35 ++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6c6e1c7..430bfed 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -535,9 +535,9 @@ __git_complete_remote_or_refspec ()
 {
 	local cur_="$cur" cmd="${words[1]}"
 	local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
-	if [ "$cmd" = "remote" ]; then
-		((c++))
-	fi
+	case "$cmd" in
+	remote|subtree) ((c++)) ;;
+	esac
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
 		case "$i" in
@@ -586,7 +586,7 @@ __git_complete_remote_or_refspec ()
 			__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
 		fi
 		;;
-	pull|remote)
+	pull|remote|subtree)
 		if [ $lhs = 1 ]; then
 			__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
 		else
@@ -2569,6 +2569,33 @@ _git_submodule ()
 	fi
 }
 
+_git_subtree ()
+{
+	local subcommands="add merge pull push split"
+	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	if [ -z "$subcommand" ]; then
+		__gitcomp "$subcommands"
+		return
+	fi
+	case "$subcommand,$cur" in
+	add,--*|merge,--*|pull,--*)
+		__gitcomp "--prefix= --squash --message="
+		;;
+	push,--*)
+		__gitcomp "--prefix="
+		;;
+	split,--*)
+		__gitcomp "
+			--annotate= --branch= --ignore-joins
+			--onto= --prefix= --rejoin
+			"
+		;;
+	add,*|push,*|pull,*)
+		__git_complete_remote_or_refspec
+		;;
+	esac
+}
+
 _git_svn ()
 {
 	local subcommands="
-- 
2.10.2


^ permalink raw reply related

* Re: [PATCH 0/5] Store submodules in a hash, not a linked list
From: Duy Nguyen @ 2017-02-13  2:39 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael Haggerty, Junio C Hamano, Stefan Beller,
	Johannes Schindelin, David Turner, git
In-Reply-To: <20170210004033.cgqmovhvoylad5cf@sigill.intra.peff.net>

On Thu, Feb 09, 2017 at 07:40:33PM -0500, Jeff King wrote:
> On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:
> 
> > >> So push the submodule attribute down to the `files_ref_store` class
> > >> (but continue to let the `ref_store`s be looked up by submodule).
> > > 
> > > I'm not sure I understand all of the ramifications here. It _sounds_ like
> > > pushing this down into the files-backend code would make it harder to
> > > have mixed ref-backends for different submodules. Or is this just
> > > pushing down an implementation detail of the files backend, and future
> > > code is free to have as many different ref_stores as it likes?
> > 
> > I don't understand how this would make it harder, aside from the fact
> > that a new backend class might also need a path member and have to
> > maintain its own copy rather than using one that the base class provides.
> 
> Probably the answer is "I'm really confused".
> 
> But here's how my line of reasoning went:
> 
>   Right now we have a main ref-store that points to the submodule
>   ref-stores. I don't know the current state of it, but in theory those
>   could all use different backends.
> 
>   This seems like it's pushing that submodule linkage down into the
>   backend.
> 
> But I think from your response that the answer is no, the thing that is
> being pushed down is not the right way for the main ref store and the
> submodules to be linked.

I think it's more about "pushing out" than "pushing down". Once files
backend takes a path to .git directory, we could have a submodule
ref_store that resolves submodule path to that .git directory,
files-backend will not need to know anything about submodules.

I imagine in future lookup_ref_store() will take a .git path instead
of a submodule path, then iterate through all backends and call the
backend-specific "probe" function to let the backend figure out if
it's the right backend and whatever parameters it needs (e.g. IP
address or path). There would be submodule_lookup_ref_store() wrapper
that converts submodule path to .git path for lookup_ref_store() to
consume.
--
Duy

^ permalink raw reply

* Incorrect pipe for git log graph when using --name-status option
From: hIpPy @ 2017-02-13  2:49 UTC (permalink / raw)
  To: git

The `git log` command with `graph` and pretty format works correctly
as expected.

$ git log --graph --pretty=format:'%h' -2
* 714a14e
* 87dce5f


However, with `--name-status` option added, there is a pipe
incorrectly placed after the commit hash (example below).

$ git log --graph --pretty=format:'%h' -2 --name-status
* 714a14e|
| M README.md
| A rm.Extensions/BitSet.cs
| M rm.Extensions/Properties/AssemblyInfo.cs
| M rm.Extensions/rm.Extensions.csproj
| A rm.ExtensionsTest/BitSetTest.cs
| M rm.ExtensionsTest/rm.ExtensionsTest.csproj

* 87dce5f|
| M rm.Extensions/GraphExtension.cs
| M rm.Extensions/Wrapped.cs
| M rm.Extensions/WrappedExtension.cs
| M rm.Extensions/rm.Extensions.csproj


IMHO, I think this is a bug. I think the correct output should be
below.

$ git log --graph --pretty=format:'%h' -2 --name-status
* 714a14e
| M README.md
| A rm.Extensions/BitSet.cs
| M rm.Extensions/Properties/AssemblyInfo.cs
| M rm.Extensions/rm.Extensions.csproj

| A rm.ExtensionsTest/BitSetTest.cs
| M rm.ExtensionsTest/rm.ExtensionsTest.csproj
|
* 87dce5f
| M rm.Extensions/GraphExtension.cs
| M rm.Extensions/Wrapped.cs
| M rm.Extensions/WrappedExtension.cs

| M rm.Extensions/rm.Extensions.csproj

I'm using this:

git version 2.11.0.windows.1
GNU bash, version 4.3.46(2)-release (x86_64-pc-msys)
Windows 8.1 64-bit


Thanks,
RM

^ permalink raw reply

* Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list
From: David Turner @ 2017-02-13  3:09 UTC (permalink / raw)
  To: Michael Haggerty, Junio C Hamano
  Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
	Johannes Schindelin, Jeff King, git
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>



On Fri, 2017-02-10 at 12:16 +0100, Michael Haggerty wrote:
> This is v2 of the patch series, considerably reorganized but not that
> different codewise. Thanks to Stefan, Junio, and Peff for their
> feedback about v1 [1]. I think I have addressed all of your comments.

LGTM.


^ permalink raw reply

* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Arif Khokar @ 2017-02-13  5:52 UTC (permalink / raw)
  To: Johannes Schindelin, Arif Khokar
  Cc: Jakub Narębski, Jeff King, Stefan Beller,
	meta@public-inbox.org, git@vger.kernel.org, Eric Wong
In-Reply-To: <alpine.DEB.2.20.1702101707060.3496@virtualbox>

On 02/10/2017 11:10 AM, Johannes Schindelin wrote:
> Hi Arif,
>
> On Wed, 24 Aug 2016, Johannes Schindelin wrote:

>> I recently adapted an old script I had to apply an entire patch series
>> given the GMane link to its cover letter:
>>
>> https://github.com/git-for-windows/build-extra/blob/master/apply-from-gmane.sh
>>
>> Maybe you find it in you to adapt that to work with public-inbox.org?
>
> Oh well. That would have been too easy a task, right?
>
> As it happens, I needed this functionality myself (when reworking my
> git-path-in-subdir patch to include Mike Rappazzo's previous patch series
> that tried to fix the same bug).
>
> I copy-edited the script to work with public-inbox.org, it accepts a
> Message-ID or URL or GMane URL and will try to apply the patch (or patch
> series) on top of the current revision:
>
> https://github.com/git-for-windows/build-extra/blob/2268850552c7/apply-from-public-inbox.sh

Thanks for the link.  One thing that comes to mind that is that it may 
be better to just download the patches and then manually apply them 
afterwords rather than doing it in the script itself.  Or at least add 
an option to the script to not automatically invoke git am.

Getting back to the point I made when this thread was still active, I 
still think it would be better to be able to list the message-id values 
in the header or body of the cover letter message of a patch series 
(preferably the former) in order to facilitate downloading the patches 
via NNTP from gmane or public-inbox.org.  That would make it easier 
compared to the different, ad-hoc, methods that exist for each email client.

Alternatively, or perhaps in addition to the list of message-ids, a list 
of URLs to public-inbox.org or gmane messages could also be provided for 
those who prefer to download patches via HTTP.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox