* Re: [PATCH 1/2] apply: guard against renames of non-existant empty files
From: Philip Oakley @ 2017-02-25 12:47 UTC (permalink / raw)
To: Junio C Hamano, git, Vegard Nossum; +Cc: Christian Couder, Michal Zalewski
In-Reply-To: <0cdd4304-7b71-c38d-21ab-b4e997242bd4@oracle.com>
From: "Vegard Nossum" <vegard.nossum@oracle.com>
> On 25/02/2017 12:59, Philip Oakley wrote:
>> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>>> If we have a patch like the one in the new test-case, then we will
>>
>> "the one in the new test-case" needs a clearer reference to the
>> particular case so that future readers will know what it refers to.
>> Noticed while browsing the commit message..
>
> There is only one testcase added by this patch, so how is it possibly
> unclear? In what situation would you read a commit message and not even
> think to glance at the patch for more details?
>
On initial reading of a commit message, the expectation is that the commit
will be about a change from some previous state, so I immediately asked
myself, where is that new (recent) test case from.
You could say "This patch presents a new test case" which would straight
away set the expectation that one should read on to see what its about. It
was just that as a reader of the log message I didn't pick up the sense you
wanted to convey. It's easy to see with hindsight or fore-knowledge.
I, personally, think that bringing the AFL discovery to the fore would help
in explaining why/how the patch appeared in the first place.
Hope that helps explain why I responded.
regards
Philip
^ permalink raw reply
* Re: [PATCH v6 0/6] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-25 15:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <xmqqd1eck5x5.fsf@gitster.mtv.corp.google.com>
On 02/20, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > @@ -55,10 +53,13 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
> >
> > Save your local modifications to a new 'stash', and run `git reset
> > --hard` to revert them. The <message> part is optional and gives
>
> I didn't notice this during v5 review, but the above seems to be
> based on the codebase before your documentation update (which used
> to be part of the series in older iteration). I had to tweak the
> series to apply on top of your 20a7e06172 ("Documentation/stash:
> remove mention of git reset --hard", 2017-02-12) while queuing, so
> please double check the result when it is pushed out to 'pu'.
Sorry about that. What you queued looks good to me, I will however
send a re-roll based on your comments on 4/6. (And I'll make sure to
base it on 20a7e06172)
^ permalink raw reply
* Re: git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Mike Crowe @ 2017-02-25 15:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqlgt0imhe.fsf@gitster.mtv.corp.google.com>
On Monday 20 February 2017 at 13:25:01 -0800, Junio C Hamano wrote:
> This almost makes me suspect that the place that checks lengths of
> one and two in order to refrain from running more expensive content
> comparison you found earlier need to ask would_convert_to_git()
> before taking the short-cut, something along the lines of this (for
> illustration purposes only, not even compile-tested). The "almost"
> comes to me because I do not offhand know the performance implications
> of making calls to would_convert_to_git() here.
>
> diff.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 051761be40..094d5913da 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
> * differences.
> *
> * 2. At this point, the file is known to be modified,
> - * with the same mode and size, and the object
> - * name of one side is unknown. Need to inspect
> - * the identical contents.
> + * with the same mode and size, the object
> + * name of one side is unknown, or size comparison
> + * cannot be depended upon. Need to inspect the
> + * contents.
> */
> if (!DIFF_FILE_VALID(p->one) || /* (1) */
> !DIFF_FILE_VALID(p->two) ||
> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
> (p->one->mode != p->two->mode) ||
> diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
> diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
> - (p->one->size != p->two->size) ||
> +
> + /*
> + * only if eol and other conversions are not involved,
> + * we can say that two contents of different sizes
> + * cannot be the same without checking their contents.
> + */
> + (!would_convert_to_git(p->one->path) &&
> + !would_convert_to_git(p->two->path) &&
> + (p->one->size != p->two->size)) ||
> +
> !diff_filespec_is_identical(p->one, p->two)) /* (2) */
> p->skip_stat_unmatch_result = 1;
> return p->skip_stat_unmatch_result;
>
>
Thanks for investigating this. I think you are correct that I was misguided
in my previous "fix". However, your change above does fix the problem for
me.
It looks like the main cost of convert_to_git is in convert_attrs which
ends up doing various path operations in attr.c. After that, both
apply_filter and crlf_to_git return straight away if there's nothing to do.
I experimented several times with running "git diff -quiet" after touching
every file in Git's own worktree and any difference in total time was lost
in the noise.
I've further improved my test case. Tests 3 and 4 fail without the above
change but pass with it. Unfortunately I'm still unable to get those tests
to fail without the above fix unless the sleeps are present. I've tried
using the "touch -r .datetime" technique from racy-git.txt but it doesn't
help. It seems that I'm unable to stop Git from using its cache without
sleeping. :(
diff --git a/t/t4063-diff-converted.sh b/t/t4063-diff-converted.sh
new file mode 100755
index 0000000..31a730d
--- /dev/null
+++ b/t/t4063-diff-converted.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+#
+# Copyright (c) 2017 Mike Crowe
+#
+# These tests ensure that files changing line endings in the presence
+# of .gitattributes to indicate that line endings should be ignored
+# don't cause 'git diff' or 'git diff --quiet' to think that they have
+# been changed.
+#
+# The sleeps are necessary to reproduce the problem for reasons that I
+# don't understand.
+
+test_description='git diff with files that require CRLF conversion'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ echo "* text=auto" > .gitattributes &&
+ printf "Hello\r\nWorld\r\n" > crlf.txt &&
+ printf "Hello\nWorld\n" > lf.txt &&
+ git add .gitattributes crlf.txt lf.txt &&
+ git commit -m "initial" && echo three
+'
+test_expect_success 'noisy diff works on file that requires CRLF conversion' '
+ git status >/dev/null &&
+ git diff >/dev/null &&
+ sleep 1 &&
+ touch crlf.txt lf.txt &&
+ git diff >/dev/null
+'
+test_expect_success 'quiet diff works on file that requires CRLF conversion with no changes' '
+ git status &&
+ git diff --quiet &&
+ sleep 1 &&
+ touch crlf.txt lf.txt &&
+ git diff --quiet
+'
+
+test_expect_success 'quiet diff works on file with line-ending change that has no effect on repository' '
+ printf "Hello\nWorld\n" > crlf.txt &&
+ printf "Hello\r\nWorld\r\n" > lf.txt &&
+ git diff --quiet
+'
+test_done
Mike.
^ permalink raw reply related
* [PATCH] strbuf: add strbuf_add_real_path()
From: René Scharfe @ 2017-02-25 16:00 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Brandon Williams
Add a function for appending the canonized absolute pathname of a given
path to a strbuf. It keeps the existing contents intact, as expected of
a function of the strbuf_add() family, while avoiding copying the result
if the given strbuf is empty. It's more consistent with the rest of the
strbuf API than strbuf_realpath(), which it's wrapping.
Also add a semantic patch demonstrating its intended usage and apply it
to the current tree. Using strbuf_add_real_path() instead of calling
strbuf_addstr() and real_path() avoids an extra copy to a static buffer.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/strbuf.cocci | 6 ++++++
setup.c | 2 +-
strbuf.c | 11 +++++++++++
strbuf.h | 14 ++++++++++++++
4 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 63995f22ff..1d580e49b0 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -38,3 +38,9 @@ expression E1, E2, E3;
@@
- strbuf_addstr(E1, find_unique_abbrev(E2, E3));
+ strbuf_add_unique_abbrev(E1, E2, E3);
+
+@@
+expression E1, E2;
+@@
+- strbuf_addstr(E1, real_path(E2));
++ strbuf_add_real_path(E1, E2);
diff --git a/setup.c b/setup.c
index 967f289f1e..f14cbcd338 100644
--- a/setup.c
+++ b/setup.c
@@ -254,7 +254,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
if (!is_absolute_path(data.buf))
strbuf_addf(&path, "%s/", gitdir);
strbuf_addbuf(&path, &data);
- strbuf_addstr(sb, real_path(path.buf));
+ strbuf_add_real_path(sb, path.buf);
ret = 1;
} else {
strbuf_addstr(sb, gitdir);
diff --git a/strbuf.c b/strbuf.cq
index 8fec6579f7..ace58e7367 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -707,6 +707,17 @@ void strbuf_add_absolute_path(struct strbuf *sb, const char *path)
strbuf_addstr(sb, path);
}
+void strbuf_add_real_path(struct strbuf *sb, const char *path)
+{
+ if (sb->len) {
+ struct strbuf resolved = STRBUF_INIT;
+ strbuf_realpath(&resolved, path, 1);
+ strbuf_addbuf(sb, &resolved);
+ strbuf_release(&resolved);
+ } else
+ strbuf_realpath(sb, path, 1);
+}
+
int printf_ln(const char *fmt, ...)
{
int ret;
diff --git a/strbuf.h b/strbuf.h
index cf1b5409e7..cf8e4bf532 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -441,6 +441,20 @@ extern int strbuf_getcwd(struct strbuf *sb);
*/
extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
+/**
+ * Canonize `path` (make it absolute, resolve symlinks, remove extra
+ * slashes) and append it to `sb`. Die with an informative error
+ * message if there is a problem.
+ *
+ * The directory part of `path` (i.e., everything up to the last
+ * dir_sep) must denote a valid, existing directory, but the last
+ * component need not exist.
+ *
+ * Callers that don't mind links should use the more lightweight
+ * strbuf_add_absolute_path() instead.
+ */
+extern void strbuf_add_real_path(struct strbuf *sb, const char *path);
+
/**
* Normalize in-place the path contained in the strbuf. See
--
2.12.0
^ permalink raw reply related
* [ANNOUNCE] Git for Windows 2.12.0
From: Johannes Schindelin @ 2017-02-25 16:55 UTC (permalink / raw)
To: git-for-windows, git; +Cc: Johannes Schindelin
MIME-Version: 1.0
Fcc: Sent
Dear Git users,
It is my pleasure to announce that Git for Windows 2.12.0 is available from:
https://git-for-windows.github.io/
Changes since Git for Windows v2.11.1 (February 3rd 2017)
New Features
• Comes with Git v2.12.0.
• The builtin difftool is no longer opt-in, as it graduated to be
officially adopted by the Git project.
• Comes with v2.7.0 of the POSIX emulation layer based on the Cygwin
runtime.
• Includes cURL 7.53.1.
• The Portable Git now defaults to using the included Git Credential
Manager.
Bug Fixes
• The stderr output is unbuffered again, i.e. errors are displayed
immediately (this was reported on the Git mailing list as well as
issues #1064, #1064, #1068).
• Git can clone again from paths containing non-ASCII characters.
• We no longer ship two different versions of curl.exe.
• Hitting Ctrl+T in Git GUI even after all files have been (un)staged
no longer throws an exception.
• A couple of Git GUI bugs regarding the list of recent repositories
have been fixed.
• The git-bash.exe helper now waits again for the terminal to be
closed before returning.
• Git for Windows no longer attempts to send empty credentials to
HTTP(S) servers that handle only Basic and/or Digest authentication
.
Filename | SHA-256
-------- | -------
Git-2.12.0-64-bit.exe | 0224c1cf4ff48535fdfc2555175be9a06c6d8b67fbf208b1c524f01252f8b13b
Git-2.12.0-32-bit.exe | de7f69bd6313bf7b427b02687a0ee930012a32d40aed2bb96f428699c936180d
PortableGit-2.12.0-64-bit.7z.exe | 5bebd0ee21e5cf3976bc71826a28b2663c7a0c9b5c98f4ab46ff03c3c0d3556f
PortableGit-2.12.0-32-bit.7z.exe | 0375ba0a05f9cd501cc8089b9af6f2adf8904a5efb1e5b9421e6561bd9f8c817
MinGit-2.12.0-64-bit.zip | 6238f65c4d8412b993cb092efde4954f8cb7da4def54d0c1533835f00e83fdad
MinGit-2.12.0-32-bit.zip | 5a118ff8a8f859866d6874261fc8ec685848a2ccf9fa0858417c98e21f5c0ec3
Git-2.12.0-64-bit.tar.bz2 | b512fb28ceeddb6a6cdf15e6c936aea15fd2b1b3c8154f72101f8c9060549f90
Git-2.12.0-32-bit.tar.bz2 | a6c0b5b36c19a70f2c9ffd8fbfeb57bedbb7a9a2207672ac38c5bc5a38ae320a
Ciao,
Johannes
^ permalink raw reply
* Re: SHA1 collisions found
From: brian m. carlson @ 2017-02-25 18:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ian Jackson, Joey Hess, git
In-Reply-To: <xmqq60jz5wbm.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]
On Fri, Feb 24, 2017 at 09:32:13AM -0800, Junio C Hamano wrote:
> Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
>
> > I have been thinking about how to do a transition from SHA1 to another
> > hash function.
>
> Good. I think many of us have also been, too, not necessarily just
> in the past few days in response to shattered, but over the last 10
> years, yet without coming to a consensus design ;-)
>
> > I have concluded that:
> >
> > * We can should avoid expecting everyone to rewrite all their
> > history.
>
> Yes.
There are security implications for old objects if we mix hashes, but I
suppose people who want better security will just rewrite history
anyway.
> As long as the reader can tell from the format of object names
> stored in the "new object format" object from what era is being
> referred to in some way [*1*], we can name new objects with only new
> hash, I would think. "new refers only to new" that stratifies
> objects into older and newer may make things simpler, but I am not
> convinced yet that it would give our users a smooth enough
> transition path (but I am open to be educated and pursuaded the
> other way).
I would simply use multihash[0] for this purpose. New-style objects
serialize data in multihash format, so it's immediately obvious what
hash we're referring to. That makes future transitions less
problematic.
[0] https://github.com/multiformats/multihash
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: SHA1 collisions found
From: brian m. carlson @ 2017-02-25 19:04 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Joey Hess, Git Mailing List
In-Reply-To: <CACsJy8AtQG8YXQ+YfSFifUxqtd==THj5weJK5jooyiRN0yamiQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]
On Fri, Feb 24, 2017 at 04:42:38PM +0700, Duy Nguyen wrote:
> On Thu, Feb 23, 2017 at 11:43 PM, Joey Hess <id@joeyh.name> wrote:
> > IIRC someone has been working on parameterizing git's SHA1 assumptions
> > so a repository could eventually use a more secure hash. How far has
> > that gotten? There are still many "40" constants in git.git HEAD.
>
> Michael asked Brian (that "someone") the other day and he replied [1]
>
> >> I'm curious; what fraction of the overall convert-to-object_id campaign
> >> do you estimate is done so far? Are you getting close to the promised
> >> land yet?
> >
> > So I think that the current scope left is best estimated by the
> > following command:
> >
> > git grep -P 'unsigned char\s+(\*|.*20)' | grep -v '^Documentation'
> >
> > So there are approximately 1200 call sites left, which is quite a bit of
> > work. I estimate between the work I've done and other people's
> > refactoring work (such as the refs backend refactor), we're about 40%
> > done.
As a note, I've been working on this pretty much nonstop since the
collision announcement was made. After another 27 commits, I've got it
down from 1244 to 1119.
I plan to send another series out sometime after the existing series has
hit next. People who are interested can follow the object-id-part*
branches at https://github.com/bk2204/git.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* git-clone --config order & fetching extra refs during initial clone
From: Robin H. Johnson @ 2017-02-25 19:12 UTC (permalink / raw)
To: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
TL;DR: git-clone ignores any fetch specs passed via --config.
The documentation for git-clone --config says:
| Set a configuration variable in the newly-created repository; this takes
| effect immediately __AFTER__ the repository is initialized, but __BEFORE__
| the remote history is fetched or any files checked out. [...]
(emphasis added)
However, this doesn't seem be be true, right after the clone, the refs are NOT
present, and the next fetch seems to pull the extra refs. This seems to be
because the refspec building for the initial clone doesn't take into account
any fetch lines added to the config.
Testcase to reproduce (confirmed in v2.11.1, not tested 2.12.0 quite yet):
# export REPOURI=https://github.com/openstack-dev/sandbox.git DIR=test
# git clone \
-c remote.origin.fetch=+refs/notes/*:refs/notes/* \
-c remote.origin.fetch=+refs/changes/*:refs/remotes/origin/changes/* \
$REPOURI $DIR \
&& cd $DIR \
&& git fetch
--
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Trustee & Treasurer
E-Mail : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 1083 bytes --]
^ permalink raw reply
* [PATCH] http: add an "auto" mode for http.emptyauth
From: Jeff King @ 2017-02-25 19:18 UTC (permalink / raw)
To: Johannes Schindelin
Cc: David Turner, Junio C Hamano, git@vger.kernel.org,
sandals@crustytoothpaste.net, Eric Sunshine
In-Reply-To: <20170225191506.4it7pdsi6ijanfft@sigill.intra.peff.net>
This variable needs to be specified to make some types of
non-basic authentication work, but ideally this would just
work out of the box for everyone.
However, simply setting it to "1" by default introduces an
extra round-trip for cases where it _isn't_ useful. We end
up sending a bogus empty credential that the server rejects.
Instead, let's introduce an automatic mode, that works like
this:
1. We won't try to send the bogus credential on the first
request. We'll wait to get an HTTP 401, as usual.
2. After seeing an HTTP 401, the empty-auth hack will kick
in only when we know there is an auth method available
that might make use of it (i.e., something besides
"Basic" or "Digest").
That should make it work out of the box, without incurring
any extra round-trips for people hitting Basic-only servers.
This _does_ incur an extra round-trip if you really want to
use "Basic" but your server advertises other methods (the
emptyauth hack will kick in but fail, and then Git will
actually ask for a password).
The auto mode may incur an extra round-trip over setting
http.emptyauth=true, because part of the emptyauth hack is
to feed this blank password to curl even before we've made a
single request.
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
---
And here's the full patch. It is meant to go on top of the
already-queued 1/2, though I suspect it could apply separately.
Test reports welcome from people who actually have NTLM or Kerberos
servers. The changes from the previous are fairly minimal, but this kind
of bit-mangling is exactly the kind of thing where I tend to
accidentally invert the logic. ;)
http.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 45 insertions(+), 5 deletions(-)
diff --git a/http.c b/http.c
index a05609766..dd637d031 100644
--- a/http.c
+++ b/http.c
@@ -109,7 +109,7 @@ static int curl_save_cookies;
struct credential http_auth = CREDENTIAL_INIT;
static int http_proactive_auth;
static const char *user_agent;
-static int curl_empty_auth;
+static int curl_empty_auth = -1;
enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
@@ -125,6 +125,14 @@ static struct credential cert_auth = CREDENTIAL_INIT;
static int ssl_cert_password_required;
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
static unsigned long http_auth_methods = CURLAUTH_ANY;
+static int http_auth_methods_restricted;
+/* Modes for which empty_auth cannot actually help us. */
+static unsigned long empty_auth_useless =
+ CURLAUTH_BASIC
+#ifdef CURLAUTH_DIGEST_IE
+ | CURLAUTH_DIGEST_IE
+#endif
+ | CURLAUTH_DIGEST;
#endif
static struct curl_slist *pragma_header;
@@ -333,7 +341,10 @@ static int http_options(const char *var, const char *value, void *cb)
return git_config_string(&user_agent, var, value);
if (!strcmp("http.emptyauth", var)) {
- curl_empty_auth = git_config_bool(var, value);
+ if (value && !strcmp("auto", value))
+ curl_empty_auth = -1;
+ else
+ curl_empty_auth = git_config_bool(var, value);
return 0;
}
@@ -382,10 +393,37 @@ static int http_options(const char *var, const char *value, void *cb)
return git_default_config(var, value, cb);
}
+static int curl_empty_auth_enabled(void)
+{
+ if (curl_empty_auth >= 0)
+ return curl_empty_auth;
+
+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
+ /*
+ * Our libcurl is too old to do AUTH_ANY in the first place;
+ * just default to turning the feature off.
+ */
+#else
+ /*
+ * In the automatic case, kick in the empty-auth
+ * hack as long as we would potentially try some
+ * method more exotic than "Basic" or "Digest".
+ *
+ * But only do this when this is our second or
+ * subsequent * request, as by then we know what
+ * methods are available.
+ */
+ if (http_auth_methods_restricted &&
+ (http_auth_methods & ~empty_auth_useless))
+ return 1;
+#endif
+ return 0;
+}
+
static void init_curl_http_auth(CURL *result)
{
if (!http_auth.username || !*http_auth.username) {
- if (curl_empty_auth)
+ if (curl_empty_auth_enabled())
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
return;
}
@@ -1079,7 +1117,7 @@ struct active_request_slot *get_active_slot(void)
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
#endif
- if (http_auth.password || curl_empty_auth)
+ if (http_auth.password || curl_empty_auth_enabled())
init_curl_http_auth(slot->curl);
return slot;
@@ -1347,8 +1385,10 @@ static int handle_curl_result(struct slot_results *results)
} else {
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
- if (results->auth_avail)
+ if (results->auth_avail) {
http_auth_methods &= results->auth_avail;
+ http_auth_methods_restricted = 1;
+ }
#endif
return HTTP_REAUTH;
}
--
2.12.0.616.g5f622f3b1
^ permalink raw reply related
* Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth
From: Jeff King @ 2017-02-25 19:15 UTC (permalink / raw)
To: Johannes Schindelin
Cc: David Turner, Junio C Hamano, git@vger.kernel.org,
sandals@crustytoothpaste.net, Eric Sunshine
In-Reply-To: <alpine.DEB.2.20.1702251243390.3767@virtualbox>
On Sat, Feb 25, 2017 at 12:48:54PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 22 Feb 2017, Jeff King wrote:
>
> > [two beautiful patches]
>
> I applied them and verified that the reported issue is fixed. Thank you!
>
> Hopefully you do not mind that I cherry-picked them in preparation for
> Git for Windows v2.12.0?
No, I don't mind. I'm happy that more people with a non-Basic setup are
verifying that they work. :)
Of the changes:
> diff --git a/http.c b/http.c
> index f8eb0f23d6c..fb94c444c80 100644
> --- a/http.c
> +++ b/http.c
> @@ -334,7 +334,10 @@ static int http_options(const char *var, const char *value, void *cb)
> return git_config_string(&user_agent, var, value);
>
> if (!strcmp("http.emptyauth", var)) {
> - curl_empty_auth = git_config_bool(var, value);
> + if (value && !strcmp("auto", value))
> + curl_empty_auth = -1;
> + else
> + curl_empty_auth = git_config_bool(var, value);
> return 0;
> }
Obviously good, I should have included this in the original.
> +#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
> + /*
> + * Our libcurl is too old to do AUTH_ANY in the first place;
> + * just default to turning the feature off.
> + */
> #else
> - /*
> - * Our libcurl is too old to do AUTH_ANY in the first place;
> - * just default to turning the feature off.
> - */
The ifdef reordering here is good.
> + /*
> + * In the automatic case, kick in the empty-auth
> + * hack as long as we would potentially try some
> + * method more exotic than "Basic".
> + *
> + * But only do this when this is our second or
> + * subsequent * request, as by then we know what
> + * methods are available.
> + */
> + if (http_auth_methods_restricted)
> + switch (http_auth_methods) {
> + case CURLAUTH_BASIC:
> + case CURLAUTH_DIGEST:
> +#ifdef CURLAUTH_DIGEST_IE
> + case CURLAUTH_DIGEST_IE:
> #endif
> [...]
> + return 0;
> + default:
> + return 1;
> + }
This is an improvement over my basic-only, but I think you actually want
to bitmask here. A server which advertises only BASIC|DIGEST should not
do empty-auth, but wouldn't match your switch statement.
Patch below.
> Now, how to get this into upstream Git, too? Jeff, do you want to submit a
> v2? In that case, would you please consider the fixup! I mentioned above?
> Otherwise I'd be happy to take it from here.
I don't mind doing a v2. I'm unsure of whether we want to default to
"auto" or not upstream. It seems from your releases that you think it is
safe enough to do in Windows. And I guess nobody outside of that is
really doing NTLM. So it's OK, I guess?
<shrug> I don't have enough information to make an intelligent opinion,
so I'm happy to defer.
I'll send my v2 in a minute. Here's the interdiff/fixup if you need to
apply it separately:
diff --git a/http.c b/http.c
index 523c43cf9..dd637d031 100644
--- a/http.c
+++ b/http.c
@@ -126,6 +126,13 @@ static int ssl_cert_password_required;
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
static unsigned long http_auth_methods = CURLAUTH_ANY;
static int http_auth_methods_restricted;
+/* Modes for which empty_auth cannot actually help us. */
+static unsigned long empty_auth_useless =
+ CURLAUTH_BASIC
+#ifdef CURLAUTH_DIGEST_IE
+ | CURLAUTH_DIGEST_IE
+#endif
+ | CURLAUTH_DIGEST;
#endif
static struct curl_slist *pragma_header;
@@ -400,23 +407,15 @@ static int curl_empty_auth_enabled(void)
/*
* In the automatic case, kick in the empty-auth
* hack as long as we would potentially try some
- * method more exotic than "Basic".
+ * method more exotic than "Basic" or "Digest".
*
* But only do this when this is our second or
* subsequent * request, as by then we know what
* methods are available.
*/
- if (http_auth_methods_restricted)
- switch (http_auth_methods) {
- case CURLAUTH_BASIC:
- case CURLAUTH_DIGEST:
-#ifdef CURLAUTH_DIGEST_IE
- case CURLAUTH_DIGEST_IE:
-#endif
- return 0;
- default:
- return 1;
- }
+ if (http_auth_methods_restricted &&
+ (http_auth_methods & ~empty_auth_useless))
+ return 1;
#endif
return 0;
}
^ permalink raw reply related
* Re: [PATCH v6 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Thomas Gummerer @ 2017-02-25 20:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Johannes Schindelin, sunny, Jakub Narębski,
Matthieu Moy
In-Reply-To: <xmqqmvdfh4az.fsf@gitster.mtv.corp.google.com>
On 02/21, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > - git reset --hard ${GIT_QUIET:+-q}
>
> This hunk is probably the most important one to review in the whole
> series, in the sense that these are entirely new code that didn't
> exist in the original.
>
> > + if test $# != 0
> > + then
> > + saved_untracked=
> > + if test -n "$(git ls-files --others -- "$@")"
>
> I notice that "ls-files -o" used in the code before this series are
> almost always used with --exclude-standard but we do not set up the
> standard exclude pattern here. Is there a good reason to use (or
> not to use) it here as well?
We probably should use it, not adding it was an oversight.
> > + then
> > + saved_untracked=$(
> > + git ls-files -z --others -- "$@" |
> > + xargs -0 git stash create -u all --)
> > + fi
>
> Running the same ls-files twice look somewhat wasteful.
>
> I suspect that we avoid "xargs -0" in our code from portability
> concern (isn't it a GNU invention?)
>
> > + git ls-files -z -- "$@" | xargs -0 git reset ${GIT_QUIET:+-q} --
>
> Hmm, am I being naive to suspect that the above is a roundabout way
> to say:
>
> git reset ${GIT_QUIET:+-q} -- "$@"
>
> or is an effect quite different from that intended here?
>
> > + git ls-files -z --modified -- "$@" | xargs -0 git checkout ${GIT_QUIET:+-q} HEAD --
>
> Likewise. Wouldn't the above be equivalent to:
>
> git checkout ${GIT_QUIET:+-q} HEAD -- "$@"
>
> Or is this trying to preserve paths modified in the working tree and
> fully added to the index?.
No, it's not trying to do that (all the paths we're touching here
would have been "reset" earlier, so it wouldn't change anything.
However what this code tried to do is to allow "stash push -- path
pathspec-not-in-repo", where "pathspec-not-in-repo" would end up
tripping up "checkout" which does not accept pathspecs that are not in
the index.
I think we should disallow such pathspecs in stash as well, except in
the --include-untracked case, where it still makes sense.
This means we can't get rid of the "ls-files --modified" here, but we
can in all other places, and get rid of the "stash_create"
"stash_apply" dance to store the untracked files, simplifying this
part quite a bit.
Will re-roll.
>
> > + if test -n "$(git ls-files -z --others -- "$@")"
> > + then
> > + git ls-files -z --others -- "$@" | xargs -0 git clean --force -d ${GIT_QUIET:+-q} --
>
> Likewise. "ls-files --others" being the major part of what "clean"
> is about, I suspect the "ls-files piped to clean" is redundant. Do
> you even need a test? IOW, doesn't "git clean" with a pathspec that
> does not match anything silently succeed without doing anything
> harmful?
>
> > + fi
> > + if test -n "$saved_untracked"
> > + then
> > + git stash pop -q $saved_untracked
>
> I see this thing was "created", and the whole point of "create" is
> to be different from "save/push" that automatically adds the result
> to the stash reflog. Should we be "pop"ing it, or did you mean to
> just call apply_stash on it?
>
> > + fi
> > + else
> > + git reset --hard ${GIT_QUIET:+-q}
> > + fi
>
^ permalink raw reply
* [PATCH 1/2] commit: be more precise when searching for headers
From: René Scharfe @ 2017-02-25 19:21 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Search for a space character only within the current line in
read_commit_extra_header_lines() instead of searching in the whole
buffer (and possibly beyond, if it's not NUL-terminated) and then
discarding any results after the end of the current line.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
commit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 2cf85158b4..173c6d3818 100644
--- a/commit.c
+++ b/commit.c
@@ -1354,8 +1354,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
strbuf_reset(&buf);
it = NULL;
- eof = strchr(line, ' ');
- if (next <= eof)
+ eof = memchr(line, ' ', next - line);
+ if (!eof)
eof = next;
if (standard_header_field(line, eof - line) ||
--
2.12.0
^ permalink raw reply related
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-25 19:26 UTC (permalink / raw)
To: brian m. carlson, Junio C Hamano, Ian Jackson, Joey Hess, git
In-Reply-To: <20170225185050.t6e5txrppofgelsf@genre.crustytoothpaste.net>
On Sat, Feb 25, 2017 at 06:50:50PM +0000, brian m. carlson wrote:
> > As long as the reader can tell from the format of object names
> > stored in the "new object format" object from what era is being
> > referred to in some way [*1*], we can name new objects with only new
> > hash, I would think. "new refers only to new" that stratifies
> > objects into older and newer may make things simpler, but I am not
> > convinced yet that it would give our users a smooth enough
> > transition path (but I am open to be educated and pursuaded the
> > other way).
>
> I would simply use multihash[0] for this purpose. New-style objects
> serialize data in multihash format, so it's immediately obvious what
> hash we're referring to. That makes future transitions less
> problematic.
>
> [0] https://github.com/multiformats/multihash
I looked at that earlier, because I think it's a reasonable idea for
future-proofing. The first byte is a "varint", but I couldn't find where
they defined that format.
The closest I could find is:
https://github.com/multiformats/unsigned-varint
whose README says:
This unsigned varint (VARiable INTeger) format is for the use in all
the multiformats.
- We have not yet decided on a format yet. When we do, this readme
will be updated.
- We have time. All multiformats are far from requiring this varint.
which is not exactly confidence inspiring. They also put the length at
the front of the hash. That's probably convenient if you're parsing an
unknown set of hashes, but I'm not sure it's helpful inside Git objects.
And there's an incentive to minimize header data at the front of a hash,
because every byte is one more byte that every single hash will collide
over, and people will have to type when passing hashes to "git show",
etc.
I'd almost rather use something _really_ verbose like
sha256:1234abcd...
in all of the objects. And then when we get an unadorned hash from the
user, we guess it's sha256 (or whatever), and fallback to treating it as
a sha1.
Using a syntactically-obvious name like that also solves one other
problem: there are sha1 hashes whose first bytes will encode as a "this
is sha256" multihash, creating some ambiguity.
-Peff
^ permalink raw reply
* [PATCH 2/2] commit: don't check for space twice when looking for header
From: René Scharfe @ 2017-02-25 19:27 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
In-Reply-To: <23989e76-24ba-90a4-91a9-9f66bfccb7c9@web.de>
Both standard_header_field() and excluded_header_field() check if
there's a space after the buffer that's handed to them. We already
check in the caller if that space is present. Don't bother calling
the functions if it's missing, as they are guaranteed to return 0 in
that case, and remove the now redundant checks from them.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
commit.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/commit.c b/commit.c
index 173c6d3818..fab8269731 100644
--- a/commit.c
+++ b/commit.c
@@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
static inline int standard_header_field(const char *field, size_t len)
{
- return ((len == 4 && !memcmp(field, "tree ", 5)) ||
- (len == 6 && !memcmp(field, "parent ", 7)) ||
- (len == 6 && !memcmp(field, "author ", 7)) ||
- (len == 9 && !memcmp(field, "committer ", 10)) ||
- (len == 8 && !memcmp(field, "encoding ", 9)));
+ return ((len == 4 && !memcmp(field, "tree", 4)) ||
+ (len == 6 && !memcmp(field, "parent", 6)) ||
+ (len == 6 && !memcmp(field, "author", 6)) ||
+ (len == 9 && !memcmp(field, "committer", 9)) ||
+ (len == 8 && !memcmp(field, "encoding", 8)));
}
static int excluded_header_field(const char *field, size_t len, const char **exclude)
@@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc
while (*exclude) {
size_t xlen = strlen(*exclude);
- if (len == xlen &&
- !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
+ if (len == xlen && !memcmp(field, *exclude, xlen))
return 1;
exclude++;
}
@@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
eof = memchr(line, ' ', next - line);
if (!eof)
eof = next;
-
- if (standard_header_field(line, eof - line) ||
- excluded_header_field(line, eof - line, exclude))
+ else if (standard_header_field(line, eof - line) ||
+ excluded_header_field(line, eof - line, exclude))
continue;
it = xcalloc(1, sizeof(*it));
--
2.12.0
^ permalink raw reply related
* Re: [PATCH 1/2] commit: be more precise when searching for headers
From: Jeff King @ 2017-02-25 20:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <23989e76-24ba-90a4-91a9-9f66bfccb7c9@web.de>
On Sat, Feb 25, 2017 at 08:21:52PM +0100, René Scharfe wrote:
> Search for a space character only within the current line in
> read_commit_extra_header_lines() instead of searching in the whole
> buffer (and possibly beyond, if it's not NUL-terminated) and then
> discarding any results after the end of the current line.
> [...]
> - eof = strchr(line, ' ');
> - if (next <= eof)
> + eof = memchr(line, ' ', next - line);
> + if (!eof)
> eof = next;
Nice. More efficient, and I think the intent is more clear.
-Peff
^ permalink raw reply
* Re: [PATCH] strbuf: add strbuf_add_real_path()
From: Jeff King @ 2017-02-25 20:11 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, Brandon Williams
In-Reply-To: <4d191b86-d36c-e3ec-99c6-d15baa6b659a@web.de>
On Sat, Feb 25, 2017 at 05:00:33PM +0100, René Scharfe wrote:
> Add a function for appending the canonized absolute pathname of a given
> path to a strbuf. It keeps the existing contents intact, as expected of
> a function of the strbuf_add() family, while avoiding copying the result
> if the given strbuf is empty. It's more consistent with the rest of the
> strbuf API than strbuf_realpath(), which it's wrapping.
>
> Also add a semantic patch demonstrating its intended usage and apply it
> to the current tree. Using strbuf_add_real_path() instead of calling
> strbuf_addstr() and real_path() avoids an extra copy to a static buffer.
It's also re-entrant, which real_path() is not.
> +void strbuf_add_real_path(struct strbuf *sb, const char *path)
> +{
> + if (sb->len) {
> + struct strbuf resolved = STRBUF_INIT;
> + strbuf_realpath(&resolved, path, 1);
> + strbuf_addbuf(sb, &resolved);
> + strbuf_release(&resolved);
> + } else
> + strbuf_realpath(sb, path, 1);
> +}
The wrapping here seems a little backwards. If strbuf_add_real_path()
were the inner one, then we would not need this extra allocation. I know
that the reasons are historical, but I don't think it would be
impossible to teach the realpath code to do it.
OTOH, it may not be worth the effort. It's not like strbuf_realpath()
doesn't allocate secondary strbufs for its work already, so dropping one
more is probably not that exciting. And certainly think your patch is an
incremental improvement.
Out of curiosity, I took a stab at the patch, which is below. Only
lightly tested by me, and it does make the logic a bit more complicated
to read, as you have to adjust for the original "base" in several
places.
diff --git a/abspath.c b/abspath.c
index 2f0c26e0e..286072f48 100644
--- a/abspath.c
+++ b/abspath.c
@@ -12,9 +12,9 @@ int is_directory(const char *path)
}
/* removes the last path component from 'path' except if 'path' is root */
-static void strip_last_component(struct strbuf *path)
+static void strip_last_component(struct strbuf *path, size_t base)
{
- size_t offset = offset_1st_component(path->buf);
+ size_t offset = base + offset_1st_component(path->buf + base);
size_t len = path->len;
/* Find start of the last component */
@@ -49,14 +49,15 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
}
/* copies root part from remaining to resolved, canonicalizing it on the way */
-static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
+static void get_root_part(struct strbuf *resolved, struct strbuf *remaining,
+ size_t base)
{
int offset = offset_1st_component(remaining->buf);
- strbuf_reset(resolved);
+ strbuf_setlen(resolved, base);
strbuf_add(resolved, remaining->buf, offset);
#ifdef GIT_WINDOWS_NATIVE
- convert_slashes(resolved->buf);
+ convert_slashes(resolved->buf + base);
#endif
strbuf_remove(remaining, 0, offset);
}
@@ -78,8 +79,8 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
* informative error message if there is a problem. Otherwise, return
* NULL on errors (without generating any output).
*/
-char *strbuf_realpath(struct strbuf *resolved, const char *path,
- int die_on_error)
+char *strbuf_add_real_path(struct strbuf *resolved, const char *path,
+ int die_on_error)
{
struct strbuf remaining = STRBUF_INIT;
struct strbuf next = STRBUF_INIT;
@@ -87,6 +88,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
char *retval = NULL;
int num_symlinks = 0;
struct stat st;
+ size_t base = resolved->len;
if (!*path) {
if (die_on_error)
@@ -96,9 +98,9 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
}
strbuf_addstr(&remaining, path);
- get_root_part(resolved, &remaining);
+ get_root_part(resolved, &remaining, base);
- if (!resolved->len) {
+ if (resolved->len == base) {
/* relative path; can use CWD as the initial resolved path */
if (strbuf_getcwd(resolved)) {
if (die_on_error)
@@ -118,7 +120,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
continue; /* '.' component */
} else if (next.len == 2 && !strcmp(next.buf, "..")) {
/* '..' component; strip the last path component */
- strip_last_component(resolved);
+ strip_last_component(resolved, base);
continue;
}
@@ -127,12 +129,12 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
strbuf_addch(resolved, '/');
strbuf_addbuf(resolved, &next);
- if (lstat(resolved->buf, &st)) {
+ if (lstat(resolved->buf + base, &st)) {
/* error out unless this was the last component */
if (errno != ENOENT || remaining.len) {
if (die_on_error)
die_errno("Invalid path '%s'",
- resolved->buf);
+ resolved->buf + base);
else
goto error_out;
}
@@ -150,7 +152,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
goto error_out;
}
- len = strbuf_readlink(&symlink, resolved->buf,
+ len = strbuf_readlink(&symlink, resolved->buf + base,
st.st_size);
if (len < 0) {
if (die_on_error)
@@ -162,14 +164,14 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
if (is_absolute_path(symlink.buf)) {
/* absolute symlink; set resolved to root */
- get_root_part(resolved, &symlink);
+ get_root_part(resolved, &symlink, base);
} else {
/*
* relative symlink
* strip off the last component since it will
* be replaced with the contents of the symlink
*/
- strip_last_component(resolved);
+ strip_last_component(resolved, base);
}
/*
@@ -202,6 +204,12 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
return retval;
}
+char *strbuf_realpath(struct strbuf *sb, const char *path, int die_on_error)
+{
+ strbuf_reset(sb);
+ return strbuf_add_real_path(sb, path, die_on_error);
+}
+
const char *real_path(const char *path)
{
static struct strbuf realpath = STRBUF_INIT;
^ permalink raw reply related
* Re: [PATCH 2/2] commit: don't check for space twice when looking for header
From: Jeff King @ 2017-02-25 20:15 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <b1d5c882-38b8-dd2d-2e5f-aafb8dfada81@web.de>
On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:
> Both standard_header_field() and excluded_header_field() check if
> there's a space after the buffer that's handed to them. We already
> check in the caller if that space is present. Don't bother calling
> the functions if it's missing, as they are guaranteed to return 0 in
> that case, and remove the now redundant checks from them.
Makes sense, and I couldn't spot any errors in your logic or in the
code.
> static inline int standard_header_field(const char *field, size_t len)
> {
> - return ((len == 4 && !memcmp(field, "tree ", 5)) ||
> - (len == 6 && !memcmp(field, "parent ", 7)) ||
> - (len == 6 && !memcmp(field, "author ", 7)) ||
> - (len == 9 && !memcmp(field, "committer ", 10)) ||
> - (len == 8 && !memcmp(field, "encoding ", 9)));
> + return ((len == 4 && !memcmp(field, "tree", 4)) ||
> + (len == 6 && !memcmp(field, "parent", 6)) ||
> + (len == 6 && !memcmp(field, "author", 6)) ||
> + (len == 9 && !memcmp(field, "committer", 9)) ||
> + (len == 8 && !memcmp(field, "encoding", 8)));
Unrelated, but this could probably be spelled with a macro and strlen()
to avoid the magic numbers. It would probably be measurably slower for a
compiler which doesn't pre-compute strlen() on a string literal, though.
-Peff
^ permalink raw reply
* Re: git-clone --config order & fetching extra refs during initial clone
From: Jeff King @ 2017-02-25 20:21 UTC (permalink / raw)
To: Robin H. Johnson; +Cc: SZEDER Gábor, Git Mailing List
In-Reply-To: <robbat2-20170225T185056-448272755Z@orbis-terrarum.net>
On Sat, Feb 25, 2017 at 07:12:38PM +0000, Robin H. Johnson wrote:
> TL;DR: git-clone ignores any fetch specs passed via --config.
I agree that this is a bug. There's some previous discussion and an RFC
patch from lat March (author cc'd):
http://public-inbox.org/git/1457313062-10073-1-git-send-email-szeder@ira.uka.de/
That discussion veered off into alternatives, but I think the v2 posted
in that thread is taking a sane approach.
-Peff
^ permalink raw reply
* [PATCH v7 2/6] stash: add test for the create command line arguments
From: Thomas Gummerer @ 2017-02-25 21:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, sunny,
Jakub Narębski, Matthieu Moy, Thomas Gummerer
In-Reply-To: <20170225213306.2410-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.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 3/6] stash: refactor stash_create
From: Thomas Gummerer @ 2017-02-25 21:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, sunny,
Jakub Narębski, Matthieu Moy, Thomas Gummerer
In-Reply-To: <20170225213306.2410-1-t.gummerer@gmail.com>
Refactor the internal stash_create function to use a -m flag for
specifying the message and -u flag to indicate whether untracked files
should be added to the stash.
This makes it easier to pass a pathspec argument to stash_create in the
next patch.
The user interface for git stash create stays the same.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
git-stash.sh | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 8365ebba2a..ef5d1b45be 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -58,8 +58,22 @@ clear_stash () {
}
create_stash () {
- stash_msg="$1"
- untracked="$2"
+ stash_msg=
+ untracked=
+ while test $# != 0
+ do
+ case "$1" in
+ -m|--message)
+ shift
+ stash_msg=${1?"BUG: create_stash () -m requires an argument"}
+ ;;
+ -u|--include-untracked)
+ shift
+ untracked=${1?"BUG: create_stash () -u requires an argument"}
+ ;;
+ esac
+ shift
+ done
git update-index -q --refresh
if no_changes
@@ -268,7 +282,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 +681,7 @@ clear)
;;
create)
shift
- create_stash "$*" && echo "$w_commit"
+ create_stash -m "$*" && echo "$w_commit"
;;
store)
shift
--
2.11.0.301.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 6/6] stash: allow pathspecs in the no verb form
From: Thomas Gummerer @ 2017-02-25 21:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, sunny,
Jakub Narębski, Matthieu Moy, Thomas Gummerer
In-Reply-To: <20170225213306.2410-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.
Also make git stash -p an alias for git stash push -p. This allows
users to use git stash -p <pathspec>.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
Documentation/git-stash.txt | 11 +++++++----
git-stash.sh | 3 +++
t/t3903-stash.sh | 15 +++++++++++++++
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 4d8d30f179..70191d06b6 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -54,10 +54,13 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
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
- subcommand from making an unwanted stash.
+ the description along with the stashed state.
++
+For quickly making a snapshot, you can omit "push". In this mode,
+non-option arguments are not allowed to prevent a misspelled
+subcommand from making an unwanted stash. The two exceptions to this
+are `stash -p` which acts as alias for `stash push -p` and pathspecs,
+which are allowed after a double hyphen `--` for disambiguation.
+
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
diff --git a/git-stash.sh b/git-stash.sh
index 2d7b30ec5e..28d0624c75 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -655,12 +655,15 @@ apply_to_branch () {
}
}
+test "$1" = "-p" && set "push" "$@"
+
PARSE_CACHE='--not-parsed'
# 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
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2f5888df0d..f7733b4dd4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -876,4 +876,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.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 0/6] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-25 21:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, sunny,
Jakub Narębski, Matthieu Moy, Thomas Gummerer
In-Reply-To: <20170219110313.24070-1-t.gummerer@gmail.com>
Thanks Junio for more comments on the last round, and Peff for reading
through it as well.
Changes since v6:
- If no --include-untracked option is given to git stash push, and a
pathspec is not in the repository, error out instead of ignoring
it. This brings it in line with things like checkout, and also
allows us to simplify the code internally.
- Simplify the code for rolling back the changes from the working
tree. This is enabled by the changes to the pathspec handling.
- There's no more "xargs -0", so there should be no more portability
concerns.
- Adjust the tests and improve some of the titles a bit
Interdiff:
diff --git a/git-stash.sh b/git-stash.sh
index 18aba1346f..28d0624c75 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -278,12 +278,15 @@ push_stash () {
die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
fi
+ test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
+
git update-index -q --refresh
if no_changes "$@"
then
say "$(gettext "No local changes to save")"
exit 0
fi
+
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
@@ -296,23 +299,9 @@ push_stash () {
then
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
+ git reset ${GIT_QUIET:+-q} -- "$@"
+ git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z --modified "$@")
+ git clean --force ${GIT_QUIET:+-q} -d -- "$@"
else
git reset --hard ${GIT_QUIET:+-q}
fi
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index c0ae41e724..f7733b4dd4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -800,7 +800,7 @@ test_expect_success 'create with multiple arguments for the message' '
test_cmp expect actual
'
-test_expect_success 'stash -- <filename> stashes and restores the file' '
+test_expect_success 'stash -- <pathspec> stashes and restores the file' '
>foo &&
>bar &&
git add foo bar &&
@@ -812,7 +812,7 @@ test_expect_success 'stash -- <filename> stashes and restores the file' '
test_path_is_file bar
'
-test_expect_success 'stash with multiple filename arguments' '
+test_expect_success 'stash with multiple pathspec arguments' '
>foo &&
>bar &&
>extra &&
@@ -842,25 +842,29 @@ test_expect_success 'stash with file including $IFS character' '
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 &&
+test_expect_success 'stash push -p with pathspec shows no changes only once' '
+ >foo &&
+ git add foo &&
+ git commit -m "tmp" &&
+ git stash push -p foo >actual &&
echo "No local changes to save" >expect &&
+ git reset --hard HEAD~ &&
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 &&
+ >foo &&
+ git add foo &&
+ git commit -m "tmp" &&
+ git stash push foo >actual &&
echo "No local changes to save" >expect &&
+ git reset --hard HEAD~ &&
test_cmp expect actual
'
-test_expect_success 'untracked file is not removed when using pathspecs' '
+test_expect_success 'stash push with pathspec not in the repository errors out' '
>untracked &&
- git stash push untracked &&
+ test_must_fail git stash push untracked &&
test_path_is_file untracked
'
Thomas Gummerer (6):
stash: introduce push verb
stash: add test for the create command line arguments
stash: refactor stash_create
stash: teach 'push' (and 'create_stash') to honor pathspec
stash: use stash_push for no verb form
stash: allow pathspecs in the no verb form
Documentation/git-stash.txt | 25 ++++++--
git-stash.sh | 114 +++++++++++++++++++++++++++-------
t/t3903-stash.sh | 122 ++++++++++++++++++++++++++++++++++++-
t/t3905-stash-include-untracked.sh | 26 ++++++++
4 files changed, 257 insertions(+), 30 deletions(-)
--
2.11.0.301.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 1/6] stash: introduce push verb
From: Thomas Gummerer @ 2017-02-25 21:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, sunny,
Jakub Narębski, Matthieu Moy, Thomas Gummerer
In-Reply-To: <20170225213306.2410-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>
---
Documentation/git-stash.txt | 3 +++
git-stash.sh | 46 ++++++++++++++++++++++++++++++++++++++++++---
t/t3903-stash.sh | 9 +++++++++
3 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9e344cd7..d240df4af7 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,6 +15,8 @@ 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>]]
'git stash' clear
'git stash' create [<message>]
'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -46,6 +48,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>]::
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
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.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
From: Thomas Gummerer @ 2017-02-25 21:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, sunny,
Jakub Narębski, Matthieu Moy, Thomas Gummerer
In-Reply-To: <20170225213306.2410-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 | 9 ++++-
git-stash.sh | 37 +++++++++++++------
t/t3903-stash.sh | 76 ++++++++++++++++++++++++++++++++++++++
t/t3905-stash-include-untracked.sh | 26 +++++++++++++
4 files changed, 136 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index d240df4af7..88369ed8b6 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
[-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' store [-m|--message <message>] [-q|--quiet] <commit>
@@ -48,7 +49,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>]::
+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).
@@ -58,6 +59,12 @@ push [-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 ef5d1b45be..57828f926d 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 () {
@@ -71,12 +72,16 @@ create_stash () {
shift
untracked=${1?"BUG: create_stash () -u requires an argument"}
;;
+ --)
+ shift
+ break
+ ;;
esac
shift
done
git update-index -q --refresh
- if no_changes
+ if no_changes "$@"
then
exit 0
fi
@@ -108,7 +113,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" &&
@@ -131,7 +136,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"
@@ -145,7 +150,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) ||
@@ -273,27 +278,37 @@ push_stash () {
die "$(gettext "Can't use --patch and --include-untracked or --all at the same time")"
fi
+ test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
+
git update-index -q --refresh
- if no_changes
+ if no_changes "$@"
then
say "$(gettext "No local changes to save")"
exit 0
fi
+
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
+ git reset ${GIT_QUIET:+-q} -- "$@"
+ git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z --modified "$@")
+ git clean --force ${GIT_QUIET:+-q} -d -- "$@"
+ 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 ffe3549ea5..4d8a096773 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -802,4 +802,80 @@ test_expect_success 'create with multiple arguments for the message' '
test_cmp expect actual
'
+test_expect_success 'stash -- <pathspec> 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 pathspec 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 once' '
+ >foo &&
+ git add foo &&
+ git commit -m "tmp" &&
+ git stash push -p foo >actual &&
+ echo "No local changes to save" >expect &&
+ git reset --hard HEAD~ &&
+ test_cmp expect actual
+'
+
+test_expect_success 'stash push with pathspec shows no changes when there are none' '
+ >foo &&
+ git add foo &&
+ git commit -m "tmp" &&
+ git stash push foo >actual &&
+ echo "No local changes to save" >expect &&
+ git reset --hard HEAD~ &&
+ test_cmp expect actual
+'
+
+test_expect_success 'stash push with pathspec not in the repository errors out' '
+ >untracked &&
+ test_must_fail 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.g275aeb250c.dirty
^ permalink raw reply related
* [PATCH v7 5/6] stash: use stash_push for no verb form
From: Thomas Gummerer @ 2017-02-25 21:33 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin, sunny,
Jakub Narębski, Matthieu Moy, Thomas Gummerer
In-Reply-To: <20170225213306.2410-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.
Previously we allowed git stash -- -message, which is no longer allowed
after this patch. Messages starting with a hyphen was allowed since
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options"). However it was never the intent to allow that, but rather it
was allowed accidentally.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
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 88369ed8b6..4d8d30f179 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' store [-m|--message <message>] [-q|--quiet] <commit>
diff --git a/git-stash.sh b/git-stash.sh
index 57828f926d..2d7b30ec5e 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
@@ -656,7 +656,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
@@ -666,7 +666,7 @@ do
esac
done
-test -n "$seen_non_option" || set "save" "$@"
+test -n "$seen_non_option" || set "push" "$@"
# Main command set
case "$1" in
@@ -717,7 +717,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 4d8a096773..2f5888df0d 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.g275aeb250c.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox