Re: [PATCH v2 00/34] Teach the sequencer to act as rebase -i's backend
Am 13.12.2016 um 16:29 schrieb Johannes Schindelin: base-commit: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2 Published-As: https://github.com/dscho/git/releases/tag/sequencer-i-v2 Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-i-v2 Thank you so much! I would appreciate if you could publish a branch that contains the end game so that I can test it, too. Currently I am still using git://github.com/dscho/git interactive-rebase (fca871a3cf4d) -- Hannes
Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode
Junio C Hamano writes:
> Johannes Schindelin writes:
> ...
>> +if (opts->verbose) {
>> +const char *argv[] = {
>> +"diff-tree", "--stat", NULL, NULL
>> +};
>> + ...
>> +run_command_v_opt(argv, RUN_GIT_CMD);
>> +strbuf_reset(&buf);
>> +}
>> +strbuf_release(&buf);
>> }
>
> It's a bit curious that the previous step avoided running a separate
> process and instead did "diff-tree -p" all in C, but this one does not.
>
> I think it is because this one is outside the loop?
Nah, this guess of mine "The patch file generation done in 03/34
avoids spawn because it is in a loop" is off the mark. It is done
before "edit" gives control back to the end user and it is not like
we write one patch file per iteration of the loop we want to get
maximum speed out of.
Re: Creating remote git repository?
On Wed, 14 Dec 2016 09:00:42 +0300 essam Ganadily wrote: > given that git is an old and mature product i wounder why there is no > command line (git.exe in windows) way of creating a remote git > repository? > > "git remote create repo myreponame" > > frankly speaking i know that our friends in the linux kernel project > never felt the need to create remote repository because they probably > rarely need that but i guess their merciful hearts could have some > feeling for the rest of us? Also asked and answered (by me) over there at git-users [1]. 1. https://groups.google.com/d/msg/git-users/twgkOYV6kX4/FED559TPDQAJ
Creating remote git repository?
given that git is an old and mature product i wounder why there is no command line (git.exe in windows) way of creating a remote git repository? "git remote create repo myreponame" frankly speaking i know that our friends in the linux kernel project never felt the need to create remote repository because they probably rarely need that but i guess their merciful hearts could have some feeling for the rest of us?
[PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
Add the from_user parameter to the 'is_transport_allowed' function.
This allows callers to query if a transport protocol is allowed, given
that the caller knows that the protocol is coming from the user (1) or
not from the user (0) such as redirects in libcurl. If unknown a -1
should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
to determine if the protocol came from the user.
Signed-off-by: Brandon Williams
---
http.c | 14 +++---
transport.c | 8 +---
transport.h | 13 ++---
3 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/http.c b/http.c
index f7c488a..2208269 100644
--- a/http.c
+++ b/http.c
@@ -489,17 +489,17 @@ static void set_curl_keepalive(CURL *c)
}
#endif
-static long get_curl_allowed_protocols(void)
+static long get_curl_allowed_protocols(int from_user)
{
long allowed_protocols = 0;
- if (is_transport_allowed("http"))
+ if (is_transport_allowed("http", from_user))
allowed_protocols |= CURLPROTO_HTTP;
- if (is_transport_allowed("https"))
+ if (is_transport_allowed("https", from_user))
allowed_protocols |= CURLPROTO_HTTPS;
- if (is_transport_allowed("ftp"))
+ if (is_transport_allowed("ftp", from_user))
allowed_protocols |= CURLPROTO_FTP;
- if (is_transport_allowed("ftps"))
+ if (is_transport_allowed("ftps", from_user))
allowed_protocols |= CURLPROTO_FTPS;
return allowed_protocols;
@@ -588,9 +588,9 @@ static CURL *get_curl_handle(void)
#endif
#if LIBCURL_VERSION_NUM >= 0x071304
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
-get_curl_allowed_protocols());
+get_curl_allowed_protocols(0));
curl_easy_setopt(result, CURLOPT_PROTOCOLS,
-get_curl_allowed_protocols());
+get_curl_allowed_protocols(-1));
#else
warning("protocol restrictions not applied to curl redirects because\n"
"your curl version is too old (>= 7.19.4)");
diff --git a/transport.c b/transport.c
index fbd799d..f50c31a 100644
--- a/transport.c
+++ b/transport.c
@@ -676,7 +676,7 @@ static enum protocol_allow_config get_protocol_config(const
char *type)
return PROTOCOL_ALLOW_USER_ONLY;
}
-int is_transport_allowed(const char *type)
+int is_transport_allowed(const char *type, int from_user)
{
const struct string_list *whitelist = protocol_whitelist();
if (whitelist)
@@ -688,7 +688,9 @@ int is_transport_allowed(const char *type)
case PROTOCOL_ALLOW_NEVER:
return 0;
case PROTOCOL_ALLOW_USER_ONLY:
- return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ if (from_user < 0)
+ from_user = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ return from_user;
}
die("BUG: invalid protocol_allow_config type");
@@ -696,7 +698,7 @@ int is_transport_allowed(const char *type)
void transport_check_allowed(const char *type)
{
- if (!is_transport_allowed(type))
+ if (!is_transport_allowed(type, -1))
die("transport '%s' not allowed", type);
}
diff --git a/transport.h b/transport.h
index 3396e1d..4f1c801 100644
--- a/transport.h
+++ b/transport.h
@@ -142,10 +142,17 @@ struct transport {
struct transport *transport_get(struct remote *, const char *);
/*
- * Check whether a transport is allowed by the environment. Type should
- * generally be the URL scheme, as described in Documentation/git.txt
+ * Check whether a transport is allowed by the environment.
+ *
+ * Type should generally be the URL scheme, as described in
+ * Documentation/git.txt
+ *
+ * from_user specifies if the transport was given by the user. If unknown pass
+ * a -1 to read from the environment to determine if the transport was given by
+ * the user.
+ *
*/
-int is_transport_allowed(const char *type);
+int is_transport_allowed(const char *type, int from_user);
/*
* Check whether a transport is allowed by the environment,
--
2.8.0.rc3.226.g39d4020
[PATCH v9 4/5] http: create function to get curl allowed protocols
Move the creation of an allowed protocols whitelist to a helper
function.
Signed-off-by: Brandon Williams
---
http.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/http.c b/http.c
index 034426e..f7c488a 100644
--- a/http.c
+++ b/http.c
@@ -489,10 +489,25 @@ static void set_curl_keepalive(CURL *c)
}
#endif
+static long get_curl_allowed_protocols(void)
+{
+ long allowed_protocols = 0;
+
+ if (is_transport_allowed("http"))
+ allowed_protocols |= CURLPROTO_HTTP;
+ if (is_transport_allowed("https"))
+ allowed_protocols |= CURLPROTO_HTTPS;
+ if (is_transport_allowed("ftp"))
+ allowed_protocols |= CURLPROTO_FTP;
+ if (is_transport_allowed("ftps"))
+ allowed_protocols |= CURLPROTO_FTPS;
+
+ return allowed_protocols;
+}
+
static CURL *get_curl_handle(void)
{
CURL *result = curl_easy_init();
- long allowed_protocols = 0;
if (!result)
die("curl_easy_init failed");
@@ -572,16 +587,10 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_POST301, 1);
#endif
#if LIBCURL_VERSION_NUM >= 0x071304
- if (is_transport_allowed("http"))
- allowed_protocols |= CURLPROTO_HTTP;
- if (is_transport_allowed("https"))
- allowed_protocols |= CURLPROTO_HTTPS;
- if (is_transport_allowed("ftp"))
- allowed_protocols |= CURLPROTO_FTP;
- if (is_transport_allowed("ftps"))
- allowed_protocols |= CURLPROTO_FTPS;
- curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
- curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
+ curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
+get_curl_allowed_protocols());
+ curl_easy_setopt(result, CURLOPT_PROTOCOLS,
+get_curl_allowed_protocols());
#else
warning("protocol restrictions not applied to curl redirects because\n"
"your curl version is too old (>= 7.19.4)");
--
2.8.0.rc3.226.g39d4020
[PATCH v9 3/5] http: always warn if libcurl version is too old
Now that there are default "known-good" and "known-bad" protocols which
are allowed/disallowed by 'is_transport_allowed' we should always warn
the user that older versions of libcurl can't respect the allowed
protocols for redirects.
Signed-off-by: Brandon Williams
---
http.c | 5 ++---
transport.c | 5 -
transport.h | 6 --
3 files changed, 2 insertions(+), 14 deletions(-)
diff --git a/http.c b/http.c
index 5cd3ffd..034426e 100644
--- a/http.c
+++ b/http.c
@@ -583,9 +583,8 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
#else
- if (transport_restrict_protocols())
- warning("protocol restrictions not applied to curl redirects
because\n"
- "your curl version is too old (>= 7.19.4)");
+ warning("protocol restrictions not applied to curl redirects because\n"
+ "your curl version is too old (>= 7.19.4)");
#endif
if (getenv("GIT_CURL_VERBOSE"))
diff --git a/transport.c b/transport.c
index e1ba78b..fbd799d 100644
--- a/transport.c
+++ b/transport.c
@@ -700,11 +700,6 @@ void transport_check_allowed(const char *type)
die("transport '%s' not allowed", type);
}
-int transport_restrict_protocols(void)
-{
- return !!protocol_whitelist();
-}
-
struct transport *transport_get(struct remote *remote, const char *url)
{
const char *helper;
diff --git a/transport.h b/transport.h
index c681408..3396e1d 100644
--- a/transport.h
+++ b/transport.h
@@ -153,12 +153,6 @@ int is_transport_allowed(const char *type);
*/
void transport_check_allowed(const char *type);
-/*
- * Returns true if the user has attempted to turn on protocol
- * restrictions at all.
- */
-int transport_restrict_protocols(void);
-
/* Transport options which apply to git:// and scp-style URLs */
/* The program to use on the remote side to send a pack */
--
2.8.0.rc3.226.g39d4020
[PATCH v9 2/5] transport: add protocol policy config option
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to specify a whitelist of protocols to be used in clone/fetch/push commands. This patch introduces new configuration options for more fine-grained control for allowing/disallowing protocols. This also has the added benefit of allowing easier construction of a protocol whitelist on systems where setting an environment variable is non-trivial. Now users can specify a policy to be used for each type of protocol via the 'protocol..allow' config option. A default policy for all unconfigured protocols can be set with the 'protocol.allow' config option. If no user configured default is made git will allow known-safe protocols (http, https, git, ssh, file), disallow known-dangerous protocols (ext), and have a default policy of `user` for all other protocols. The supported policies are `always`, `never`, and `user`. The `user` policy can be used to configure a protocol to be usable when explicitly used by a user, while disallowing it for commands which run clone/fetch/push commands without direct user intervention (e.g. recursive initialization of submodules). Commands which can potentially clone/fetch/push from untrusted repositories without user intervention can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent protocols configured to the `user` policy from being used. Fix remote-ext tests to use the new config to allow the ext protocol to be tested. Based on a patch by Jeff King Signed-off-by: Brandon Williams --- Documentation/config.txt | 46 ++ Documentation/git.txt| 38 +--- git-submodule.sh | 12 ++-- t/lib-proto-disable.sh | 130 +-- t/t5509-fetch-push-namespaces.sh | 1 + t/t5802-connect-helper.sh| 1 + transport.c | 75 +- 7 files changed, 264 insertions(+), 39 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 8153336..50d3d06 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2260,6 +2260,52 @@ pretty.:: Note that an alias with the same name as a built-in format will be silently ignored. +protocol.allow:: + If set, provide a user defined default policy for all protocols which + don't explicitly have a policy (`protocol..allow`). By default, + if unset, known-safe protocols (http, https, git, ssh, file) have a + default policy of `always`, known-dangerous protocols (ext) have a + default policy of `never`, and all other protocols have a default + policy of `user`. Supported policies: ++ +-- + +* `always` - protocol is always able to be used. + +* `never` - protocol is never able to be used. + +* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is + either unset or has a value of 1. This policy should be used when you want a + protocol to be directly usable by the user but don't want it used by commands which + execute clone/fetch/push commands without user input, e.g. recursive + submodule initialization. + +-- + +protocol..allow:: + Set a policy to be used by protocol `` with clone/fetch/push + commands. See `protocol.allow` above for the available policies. ++ +The protocol names currently used by git are: ++ +-- + - `file`: any local file-based path (including `file://` URLs, +or local paths) + + - `git`: the anonymous git protocol over a direct TCP +connection (or proxy, if configured) + + - `ssh`: git over ssh (including `host:path` syntax, +`ssh://`, etc). + + - `http`: git over http, both "smart http" and "dumb http". +Note that this does _not_ include `https`; if you want to configure +both, you must do so individually. + + - any external helpers are named by their protocol (e.g., use +`hg` to allow the `git-remote-hg` helper) +-- + pull.ff:: By default, Git does not create an extra merge commit when merging a commit that is a descendant of the current commit. Instead, the diff --git a/Documentation/git.txt b/Documentation/git.txt index 923aa49..d9fb937 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -1129,30 +1129,20 @@ of clones and fetches. cloning a repository to make a backup). `GIT_ALLOW_PROTOCOL`:: - If set, provide a colon-separated list of protocols which are - allowed to be used with fetch/push/clone. This is useful to - restrict recursive submodule initialization from an untrusted - repository. Any protocol not mentioned will be disallowed (i.e., - this is a whitelist, not a blacklist). If the variable is not - set at all, all protocols are enabled. The protocol names - currently used by git are: - - - `file`: any local file-based path (including `file://` URLs, - or local paths) - - - `git`: the anonymous git protocol over a direct TCP -
[PATCH v9 0/5] transport protocol policy configuration
Only difference between v8 and v9 is that v9 has been rebased ontop of Jeff's http-walker-limit-redirect series 'jk/http-walker-limit-redirect'. Brandon Williams (5): lib-proto-disable: variable name fix transport: add protocol policy config option http: always warn if libcurl version is too old http: create function to get curl allowed protocols transport: add from_user parameter to is_transport_allowed Documentation/config.txt | 46 + Documentation/git.txt| 38 --- git-submodule.sh | 12 ++-- http.c | 36 ++ t/lib-proto-disable.sh | 142 --- t/t5509-fetch-push-namespaces.sh | 1 + t/t5802-connect-helper.sh| 1 + transport.c | 84 --- transport.h | 19 +++--- 9 files changed, 305 insertions(+), 74 deletions(-) -- 2.8.0.rc3.226.g39d4020
[PATCH v9 1/5] lib-proto-disable: variable name fix
The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.
Signed-off-by: Brandon Williams
---
t/lib-proto-disable.sh | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index b0917d9..be88e9a 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -9,7 +9,7 @@ test_proto () {
proto=$2
url=$3
- test_expect_success "clone $1 (enabled)" '
+ test_expect_success "clone $desc (enabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=$proto &&
@@ -18,7 +18,7 @@ test_proto () {
)
'
- test_expect_success "fetch $1 (enabled)" '
+ test_expect_success "fetch $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -27,7 +27,7 @@ test_proto () {
)
'
- test_expect_success "push $1 (enabled)" '
+ test_expect_success "push $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -36,7 +36,7 @@ test_proto () {
)
'
- test_expect_success "push $1 (disabled)" '
+ test_expect_success "push $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -45,7 +45,7 @@ test_proto () {
)
'
- test_expect_success "fetch $1 (disabled)" '
+ test_expect_success "fetch $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -54,7 +54,7 @@ test_proto () {
)
'
- test_expect_success "clone $1 (disabled)" '
+ test_expect_success "clone $desc (disabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=none &&
--
2.8.0.rc3.226.g39d4020
Re: What's cooking in git.git (Dec 2016, #03; Tue, 13)
> * sb/submodule-embed-gitdir (2016-12-12) 6 commits > - submodule: add absorb-git-dir function > - move connect_work_tree_and_git_dir to dir.h > - worktree: check if a submodule uses worktrees > - test-lib-functions.sh: teach test_commit -C > - submodule helper: support super prefix > - submodule: use absolute path for computing relative path connecting > (this branch uses nd/worktree-list-fixup; is tangled with nd/worktree-move.) > > A new submodule helper "git submodule embedgitdirs" to make it > easier to move embedded .git/ directory for submodules in a > superproject to .git/modules/ (and point the latter with the former > that is turned into a "gitdir:" file) has been added. > > Is this now pretty much done and ready for 'next'? I consider it good and it had a fair amount of reviews already, so I would think it is ready for next.
Re: What's cooking in git.git (Dec 2016, #03; Tue, 13)
On 12/13, Junio C Hamano wrote: > * bw/realpath-wo-chdir (2016-12-12) 4 commits > - real_path: have callers use real_pathdup and strbuf_realpath > - real_path: create real_pathdup > - real_path: convert real_path_internal to strbuf_realpath > - real_path: resolve symlinks by hand > > The implementation of "real_path()" was to go there with chdir(2) > and call getcwd(3), but this obviously wouldn't be usable in a > threaded environment. Rewrite it to manually resolve relative > paths including symbolic links in path components. > > What's the donness of the topic? Is this ready for 'next'? Unless others have any additional comments I think it should be ready for next. > * bw/transport-protocol-policy (2016-12-05) 5 commits > - transport: add from_user parameter to is_transport_allowed > - http: create function to get curl allowed protocols > - http: always warn if libcurl version is too old > - transport: add protocol policy config option > - lib-proto-disable: variable name fix > > Finer-grained control of what protocols are allowed for transports > during clone/fetch/push have been enabled via a new configuration > mechanism. > > What's the doneness of this topic? Did we agree that it should be > rebased on top of Peff's http-walker-limit-redirect series? Yes I believe we agreed it should be rebased on Peff's series. I can do that and send out another version. -- Brandon Williams
What's cooking in git.git (Dec 2016, #03; Tue, 13)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. A few topics were merged to 'master' and are meant for 'maint' to be in the first maintenance release for 2.11.x series. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ew/svn-fixes (2016-12-12) 2 commits (merged to 'next' on 2016-12-12 at 91ed5b3cf3) + git-svn: document useLogAuthor and addAuthorFrom config keys + git-svn: allow "0" in SVN path components Meant eventually for 'maint'. * js/mingw-isatty (2016-12-11) 1 commit (merged to 'next' on 2016-12-12 at 60c1da6676) + mingw: intercept isatty() to handle /dev/null as Git expects it We often decide if a session is interactive by checking if the standard I/O streams are connected to a TTY, but isatty() that comes with Windows incorrectly returned true if it is used on NUL (i.e. an equivalent to /dev/null). This has been fixed. Meant eventually for 'maint'. -- [New Topics] * bw/realpath-wo-chdir (2016-12-12) 4 commits - real_path: have callers use real_pathdup and strbuf_realpath - real_path: create real_pathdup - real_path: convert real_path_internal to strbuf_realpath - real_path: resolve symlinks by hand The implementation of "real_path()" was to go there with chdir(2) and call getcwd(3), but this obviously wouldn't be usable in a threaded environment. Rewrite it to manually resolve relative paths including symbolic links in path components. What's the donness of the topic? Is this ready for 'next'? * jk/quote-env-path-list-component (2016-12-13) 4 commits - t5547-push-quarantine: run the path separator test on Windows, too - tmp-objdir: quote paths we add to alternates - alternates: accept double-quoted paths - Merge branch 'jk/alt-odb-cleanup' into jk/quote-env-path-list-component A recent update to receive-pack to make it easier to drop garbage objects made it clear that GIT_ALTERNATE_OBJECT_DIRECTORIES cannot have a pathname with a colon in it (no surprise!), and this in turn made it impossible to push into a repository at such a path. This has been fixed by introducing a quoting mechanism used when appending such a path to the colon-separated list. Will merge to 'next'. * js/normalize-path-copy-ceil (2016-12-13) 1 commit - normalize_path_copy(): fix pushing to //server/share/dir paths on Windows A pathname that begins with "//" or "\\" on Windows is special but path normalization logic was unaware of it. Will merge to 'next', but I'd appreciate a second set of eyes on this. -- [Stalled] * jc/retire-compaction-heuristics (2016-11-02) 3 commits - SQUASH??? - SQUASH??? - diff: retire the original experimental "compaction" heuristics Waiting for a reroll. * jc/abbrev-autoscale-config (2016-11-01) 1 commit - config.abbrev: document the new default that auto-scales Waiting for a reroll. * jk/nofollow-attr-ignore (2016-11-02) 5 commits - exclude: do not respect symlinks for in-tree .gitignore - attr: do not respect symlinks for in-tree .gitattributes - exclude: convert "check_index" into a flags field - attr: convert "macro_ok" into a flags field - add open_nofollow() helper As we do not follow symbolic links when reading control files like .gitignore and .gitattributes from the index, match the behaviour and not follow symbolic links when reading them from the working tree. This also tightens security a bit by not leaking contents of an unrelated file in the error messages when it is pointed at by one of these files that is a symbolic link. Perhaps we want to cover .gitmodules too with the same mechanism? * nd/worktree-move (2016-11-28) 11 commits . worktree remove: new command . worktree move: refuse to move worktrees with submodules . worktree move: accept destination as directory . worktree move: new command . worktree.c: add update_worktree_location() . worktree.c: add validate_worktree() . copy.c: convert copy_file() to copy_dir_recursively() . copy.c: style fix . copy.c: convert bb_(p)error_msg to error(_errno) . copy.c: delete unused code in copy_file() . copy.c: import copy_file() from busybox (this branch uses nd/worktree-list-fixup; is tangled with sb/submodule-embed-gitdir.) "git worktree" learned move and remove subcommands. Reported to break builds on Windows. * jc/bundle (2016-03-03) 6 commits - index-pack: --clone-bundle option - Merge branch 'jc/index-pack' into jc/bundle - bundle v3: the beginning - bundle: keep a copy of bundle file name in the in-core bundle header - bundle: plu
Re: [PATCH v2 0/6] unicode_width.h: update the width tables to Unicode 9.0
Thanks. Very much appreciated.
Re: Proposal for an increased `gitk` cohesion with `git stash`.
Dearest all,
am sorry my previous message did not enter the list (cross my fingers this
will). I won't be pasting it verbatim because shame on me it leaked zombie
processes (but that part got silently dropped out by kind Paul).
In case anyone could be interested in the topic, and because a thorough reply
will take me some time, my most recent edit of this is hosted at
https://gist.githubusercontent.com/uprego/d8c3c059c56ebb911974bb905157a81e/raw/6a08d9e0ce9c2b1decd4ed92acc924961c7f7769/gitk%2520multi%2520stash%2520patch.
All problems shown I still think is a nice start (of course p.o.c / pre alpha)
if anyone ever wanted to get this working or even fix the current problems it
has.
As Paul recommend I'll be reworking and giving a patch against a rev of his
upstream.
I'm going to try his code tips to improve non obvious design choices, and (even
he doesn’t commented it and seems to me most important) really put an extra
effort in not changing the behaviour of `gitk` (i.e. started without '--all').
Then some testing against large repos, github.com/cartodb/cartodb then
github.com/odoo/oodo finally Linux; will be done.
The performance issue Paul points to, I don't think is impacting me, but now I
reckon (just as one example) there are people who develop using IDEs that leave
garbaged unuseful stashes, and that has to be taken into account as scenario.
And the large repos. But this file event handlers thing is something that will
make me lag to fix it, even surprised me because the remaining of the
subroutines that I patched are just doing the same I typed, I just replicated
near source (general revs processing) because I have no idea Tcl, even do not
give a shit, but have to say Tcl is fun C: and an interesting discovery though.
I hope that was not a trick to get me into improving the performance of the
near loop that process ALL involved revs (and the similar for refs)! I'm old
and tired to get into performance hacking.
I guess you know, the underworked grep must be an easy solve, probably
excluding ' refs/stash' because a branch named 'refs/stash' is allowed but not
a branch named ' refs/stash' (IDK which version I was trying but I will try
both 1.x.y and 2.x.y in time).
Finally... if you don't leverage stashing too much, what is the practice?
committing ephemeral to later reset and recommit? I hope I don't just needed a
lesson on git-reset instead of all this.
Please pardon my potentially mangler mail client. Yours sincerely, regards and
thanks for your time,
> On 12 Dec 2016, at 10:36, Paul Mackerras wrote:
>
> Hi Uxio,
>
> On Thu, Sep 08, 2016 at 03:41:29PM +0200, Uxío Prego wrote:
>> Hello, please forgive me for not introducing me.
>>
>> +---+
>> |Description|
>> +---+
>>
>> Patch for showing all stashes in `gitk`.
>>
>> +---+
>> |The problem|
>> +---+
>>
>> Being `gitk` one of the best tools for navigating and understanding graphs
>> of git repo revs, I got used to have it for home use, some years ago, soon
>> for office use too.
>>
>> At some point I got used to invoke always `gitk --all` in order to keep
>> tracking of tags when using the program for building, when possible, stable
>> versions of any GNU/Linux software I would want to use.
>>
>> It seems `gitk --all` uses `git show-ref` for showing remotes information.
>> A side effect of this is that the most recent stash gets shown in `gitk
>> --all`. After learning what the stash was, I got used to it.
>>
>> Later, when at jobs, I got used to have a stash on all working branch tips.
>> So I often would have several stashes in `git stash list` but only the last
>> one in `gitk --all`. That annoyed me for about a year, finally I got
>> working in `gitk` to show a stash status more similar to what `git stash
>> list` shows.
>>
>> +---+
>> |The feature|
>> +---+
>>
>> Show all stashes in `gitk` instead of only the last one.
>
> This seems like a good feature, although I don't use stashes myself.
>
>> +--+
>> |Why it's desirable|
>> +--+
>>
>> In order to have better visual control when working on repos that have
>> several active branches and there are needed quick context changes between
>> them with the features that `git stash [apply [STASHNAME]| list | pop
>> [STASHNAME]| push | save | show [[-p] STASHNAME]]`.
>>
>> In order to have a better cohesion between the mentioned features and `gitk
>> --all`.
>>
>> ++
>> |Caveats and side effects|
>> ++
>>
>> With this patch several side effects happen:
>>
>> * Stashes are shown even invoking `gitk`, not only when running `gitk
>> --all`. If this is a problem, I can keep working in the patch to avoid this.
>>
>> * The most recent stash, which was previously shown as 'stash' because its
>> corresponding `git show-ref` output line reads 'refs/stash', gets shown as
>> 'stash@{0}'. Not removing lines with 'stash' when calling `gi
[PATCHv3] git-p4 worktree support
Support for git-p4 worktrees.
Adds test cases for using git from a worktree, and
specifying the git directory either with the --git-dir
command-line option, or with $ENV{GIT_DIR}.
Luke Diamand (1):
git-p4: support git worktrees
git-p4.py | 17 +
t/t9800-git-p4-basic.sh | 20
t/t9806-git-p4-options.sh | 32
3 files changed, 65 insertions(+), 4 deletions(-)
--
2.8.2.703.g78b384c.dirty
Re: [PATCHv3] git-p4: support git worktrees
Luke Diamand writes:
> git-p4 would attempt to find the git directory using
> its own specific code, which did not know about git
> worktrees.
>
> Rework it to use "git rev-parse --git-dir" instead.
>
> Add test cases for worktree usage and specifying
> git directory via --git-dir and $GIT_DIR.
>
> Signed-off-by: Luke Diamand
> ---
> git-p4.py | 17 +
> t/t9800-git-p4-basic.sh | 20
> t/t9806-git-p4-options.sh | 32
> 3 files changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..6a1f65f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -85,6 +85,16 @@ def p4_build_cmd(cmd):
> real_cmd += cmd
> return real_cmd
>
> +def git_dir(path):
> +""" Return TRUE if the given path is a git directory (/path/to/dir/.git).
> +This won't automatically add ".git" to a directory.
> +"""
> +d = read_pipe(["git", "--git-dir", path, "rev-parse", "--git-dir"],
> True).strip()
> +if not d or len(d) == 0:
> +return None
> +else:
> +return d
> +
> def chdir(path, is_client_path=False):
> """Do chdir to the given path, and set the PWD environment
> variable for use by P4. It does not look at getcwd() output.
> @@ -563,10 +573,7 @@ def currentGitBranch():
> return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
>
> def isValidGitDir(path):
> -if (os.path.exists(path + "/HEAD")
> -and os.path.exists(path + "/refs") and os.path.exists(path +
> "/objects")):
> -return True;
> -return False
> +return git_dir(path) != None
>
> def parseRevision(ref):
> return read_pipe("git rev-parse %s" % ref).strip()
> @@ -3682,6 +3689,7 @@ def main():
> if cmd.gitdir == None:
> cmd.gitdir = os.path.abspath(".git")
> if not isValidGitDir(cmd.gitdir):
> +# "rev-parse --git-dir" without arguments will try $PWD/.git
> cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
> if os.path.exists(cmd.gitdir):
> cdup = read_pipe("git rev-parse --show-cdup").strip()
> @@ -3694,6 +3702,7 @@ def main():
> else:
> die("fatal: cannot locate git repository at %s" % cmd.gitdir)
>
> +# so git commands invoked from the P4 workspace will succeed
> os.environ["GIT_DIR"] = cmd.gitdir
The real fix has become surprisingly short and "feels right".
Will queue. Thanks.
[PATCH v2 5/6] update_unicode.sh: remove the plane filter
The uniset upstream has accepted my patches that eliminate the Unicode
plane offsets from the output in '--32' mode.
Remove the corresponding filter in update_unicode.sh.
This also fixes the issue that the plane offsets were not removed from
the second uniset call.
Signed-off-by: Beat Bolli
---
contrib/update-unicode/update_unicode.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/contrib/update-unicode/update_unicode.sh
b/contrib/update-unicode/update_unicode.sh
index 56871a1..e05db92 100755
--- a/contrib/update-unicode/update_unicode.sh
+++ b/contrib/update-unicode/update_unicode.sh
@@ -25,8 +25,7 @@ fi &&
UNICODE_DIR=. && export UNICODE_DIR &&
cat >$UNICODEWIDTH_H <<-EOF
static const struct interval zero_width[] = {
- $(uniset/uniset --32 cat:Me,Mn,Cf + U+1160..U+11FF - U+00AD |
- grep -v plane)
+ $(uniset/uniset --32 cat:Me,Mn,Cf + U+1160..U+11FF - U+00AD)
};
static const struct interval double_width[] = {
$(uniset/uniset --32 eaw:F,W)
--
2.7.2
[PATCH v2 3/6] update_unicode.sh: pin the uniset repo to a known good commit
The uniset upstream has added more commits that for example change the hexadecimal output in '--32' mode to decimal. Let's pin the repo to a commit that still outputs the width tables in the format we want. Signed-off-by: Beat Bolli --- contrib/update-unicode/update_unicode.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/update-unicode/update_unicode.sh b/contrib/update-unicode/update_unicode.sh index ff664ec..9f1bf31 100755 --- a/contrib/update-unicode/update_unicode.sh +++ b/contrib/update-unicode/update_unicode.sh @@ -15,7 +15,8 @@ if ! test -f EastAsianWidth.txt; then wget http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt fi && if ! test -d uniset; then - git clone https://github.com/depp/uniset.git + git clone https://github.com/depp/uniset.git && + ( cd uniset && git checkout 4b186196dd ) fi && ( cd uniset && -- 2.7.2
[PATCH v2 1/6] update_unicode.sh: move it into contrib/update-unicode
As it's used only by a tiny minority of the Git developer population,
this script does not belong into the main Git source directory.
Move it into contrib/ and adjust the paths to account for the new
location.
Signed-off-by: Beat Bolli
---
.gitignore | 1 -
contrib/update-unicode/.gitignore| 3 +++
contrib/update-unicode/README| 20
contrib/update-unicode/update_unicode.sh | 38 ++
update_unicode.sh| 40
5 files changed, 61 insertions(+), 41 deletions(-)
create mode 100644 contrib/update-unicode/.gitignore
create mode 100644 contrib/update-unicode/README
create mode 100755 contrib/update-unicode/update_unicode.sh
delete mode 100755 update_unicode.sh
diff --git a/.gitignore b/.gitignore
index f96e50e..ae0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -204,7 +204,6 @@
/config.mak.autogen
/config.mak.append
/configure
-/unicode
/tags
/TAGS
/cscope*
diff --git a/contrib/update-unicode/.gitignore
b/contrib/update-unicode/.gitignore
new file mode 100644
index 000..b0ebc6a
--- /dev/null
+++ b/contrib/update-unicode/.gitignore
@@ -0,0 +1,3 @@
+uniset/
+UnicodeData.txt
+EastAsianWidth.txt
diff --git a/contrib/update-unicode/README b/contrib/update-unicode/README
new file mode 100644
index 000..b9e2fc8
--- /dev/null
+++ b/contrib/update-unicode/README
@@ -0,0 +1,20 @@
+TL;DR: Run update_unicode.sh after the publication of a new Unicode
+standard and commit the resulting unicode_widths.h file.
+
+The long version
+
+
+The Git source code ships the file unicode_widths.h which contains
+tables of zero and double width Unicode code points, respectively.
+These tables are generated using update_unicode.sh in this directory.
+update_unicode.sh itself uses a third-party tool, uniset, to query two
+Unicode data files for the interesting code points.
+
+On first run, update_unicode.sh clones uniset from Github and builds it.
+This requires a current-ish version of autoconf (2.69 works per December
+2016).
+
+On each run, update_unicode.sh checks whether more recent Unicode data
+files are available from the Unicode consortium, and rebuilds the header
+unicode_widths.h with the new data. The new header can then be
+committed.
diff --git a/contrib/update-unicode/update_unicode.sh
b/contrib/update-unicode/update_unicode.sh
new file mode 100755
index 000..7b90126
--- /dev/null
+++ b/contrib/update-unicode/update_unicode.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+#See http://www.unicode.org/reports/tr44/
+#
+#Me Enclosing_Mark an enclosing combining mark
+#Mn Nonspacing_Mark a nonspacing combining mark (zero advance width)
+#Cf Format a format control character
+#
+cd "$(dirname "$0")"
+UNICODEWIDTH_H=$(git rev-parse --show-toplevel)/unicode_width.h
+(
+ if ! test -f UnicodeData.txt; then
+ wget
http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
+ fi &&
+ if ! test -f EastAsianWidth.txt; then
+ wget
http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt
+ fi &&
+ if ! test -d uniset; then
+ git clone https://github.com/depp/uniset.git
+ fi &&
+ (
+ cd uniset &&
+ if ! test -x uniset; then
+ autoreconf -i &&
+ ./configure --enable-warnings=-Werror CFLAGS='-O0 -ggdb'
+ fi &&
+ make
+ ) &&
+ UNICODE_DIR=. && export UNICODE_DIR &&
+ cat >$UNICODEWIDTH_H <<-EOF
+ static const struct interval zero_width[] = {
+ $(uniset/uniset --32 cat:Me,Mn,Cf + U+1160..U+11FF - U+00AD |
+ grep -v plane)
+ };
+ static const struct interval double_width[] = {
+ $(uniset/uniset --32 eaw:F,W)
+ };
+ EOF
+)
diff --git a/update_unicode.sh b/update_unicode.sh
deleted file mode 100755
index 27af77c..000
--- a/update_unicode.sh
+++ /dev/null
@@ -1,40 +0,0 @@
-#!/bin/sh
-#See http://www.unicode.org/reports/tr44/
-#
-#Me Enclosing_Mark an enclosing combining mark
-#Mn Nonspacing_Mark a nonspacing combining mark (zero advance width)
-#Cf Format a format control character
-#
-UNICODEWIDTH_H=../unicode_width.h
-if ! test -d unicode; then
- mkdir unicode
-fi &&
-( cd unicode &&
- if ! test -f UnicodeData.txt; then
- wget
http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
- fi &&
- if ! test -f EastAsianWidth.txt; then
- wget
http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt
- fi &&
- if ! test -d uniset; then
- git clone https://github.com/depp/uniset.git
- fi &&
- (
- cd uniset &&
- if ! test -x uniset; then
- autoreconf -i &&
- ./configure --enable-warnings=-Werror CFLAGS='-O0
[PATCH v2 4/6] update-unicode.sh: automatically download newer definition files
Checking just for the unicode data files' existence is not sufficient; we should also download them if a newer version exists on the Unicode consortium's servers. Option -N of wget does this nicely for us. Reviewed-by: Torsten Bögershausen Signed-off-by: Beat Bolli --- contrib/update-unicode/update_unicode.sh | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/contrib/update-unicode/update_unicode.sh b/contrib/update-unicode/update_unicode.sh index 9f1bf31..56871a1 100755 --- a/contrib/update-unicode/update_unicode.sh +++ b/contrib/update-unicode/update_unicode.sh @@ -8,12 +8,8 @@ cd "$(dirname "$0")" UNICODEWIDTH_H=$(git rev-parse --show-toplevel)/unicode_width.h -if ! test -f UnicodeData.txt; then - wget http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt -fi && -if ! test -f EastAsianWidth.txt; then - wget http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt -fi && +wget -N http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt \ + http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt && if ! test -d uniset; then git clone https://github.com/depp/uniset.git && ( cd uniset && git checkout 4b186196dd ) -- 2.7.2
[PATCH v2 2/6] update_unicode.sh: remove an unnecessary subshell level
After the move into contrib/update-unicode, we no longer create the
unicode directory to have a clean working folder. Instead, the directory
of the script is used. This means that the subshell can be removed.
Signed-off-by: Beat Bolli
---
contrib/update-unicode/update_unicode.sh | 53
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/contrib/update-unicode/update_unicode.sh
b/contrib/update-unicode/update_unicode.sh
index 7b90126..ff664ec 100755
--- a/contrib/update-unicode/update_unicode.sh
+++ b/contrib/update-unicode/update_unicode.sh
@@ -7,32 +7,31 @@
#
cd "$(dirname "$0")"
UNICODEWIDTH_H=$(git rev-parse --show-toplevel)/unicode_width.h
+
+if ! test -f UnicodeData.txt; then
+ wget http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
+fi &&
+if ! test -f EastAsianWidth.txt; then
+ wget http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt
+fi &&
+if ! test -d uniset; then
+ git clone https://github.com/depp/uniset.git
+fi &&
(
- if ! test -f UnicodeData.txt; then
- wget
http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
+ cd uniset &&
+ if ! test -x uniset; then
+ autoreconf -i &&
+ ./configure --enable-warnings=-Werror CFLAGS='-O0 -ggdb'
fi &&
- if ! test -f EastAsianWidth.txt; then
- wget
http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt
- fi &&
- if ! test -d uniset; then
- git clone https://github.com/depp/uniset.git
- fi &&
- (
- cd uniset &&
- if ! test -x uniset; then
- autoreconf -i &&
- ./configure --enable-warnings=-Werror CFLAGS='-O0 -ggdb'
- fi &&
- make
- ) &&
- UNICODE_DIR=. && export UNICODE_DIR &&
- cat >$UNICODEWIDTH_H <<-EOF
- static const struct interval zero_width[] = {
- $(uniset/uniset --32 cat:Me,Mn,Cf + U+1160..U+11FF - U+00AD |
- grep -v plane)
- };
- static const struct interval double_width[] = {
- $(uniset/uniset --32 eaw:F,W)
- };
- EOF
-)
+ make
+) &&
+UNICODE_DIR=. && export UNICODE_DIR &&
+cat >$UNICODEWIDTH_H <<-EOF
+static const struct interval zero_width[] = {
+ $(uniset/uniset --32 cat:Me,Mn,Cf + U+1160..U+11FF - U+00AD |
+ grep -v plane)
+};
+static const struct interval double_width[] = {
+ $(uniset/uniset --32 eaw:F,W)
+};
+EOF
--
2.7.2
[PATCH v2 0/6] unicode_width.h: update the width tables to Unicode 9.0
This is v2 of my Unicode 9.0 series. After a short discussion [1], we decided to move the generator script into contrib. This is what this series now does first. The script is then updated in contrib. Diff to v1: - complete commit reordering - fix nits in the commit messages .gitignore | 1 - contrib/update-unicode/.gitignore| 3 ++ contrib/update-unicode/README| 20 +++ contrib/update-unicode/update_unicode.sh | 33 ++ unicode_width.h | 131 ++- update_unicode.sh| 40 -- 6 files changed, 163 insertions(+), 65 deletions(-) [1] http://public-inbox.org/git/xmqqr35dm203@gitster.mtv.corp.google.com/
[PATCH v2 6/6] unicode_width.h: update the width tables to Unicode 9.0
Rerunning update-unicode.sh that we fixed in the previous commits
produces these new tables.
Signed-off-by: Beat Bolli
---
unicode_width.h | 131 +---
1 file changed, 107 insertions(+), 24 deletions(-)
diff --git a/unicode_width.h b/unicode_width.h
index 47cdd23..02207be 100644
--- a/unicode_width.h
+++ b/unicode_width.h
@@ -25,7 +25,7 @@ static const struct interval zero_width[] = {
{ 0x0825, 0x0827 },
{ 0x0829, 0x082D },
{ 0x0859, 0x085B },
-{ 0x08E4, 0x0902 },
+{ 0x08D4, 0x0902 },
{ 0x093A, 0x093A },
{ 0x093C, 0x093C },
{ 0x0941, 0x0948 },
@@ -120,6 +120,7 @@ static const struct interval zero_width[] = {
{ 0x17C9, 0x17D3 },
{ 0x17DD, 0x17DD },
{ 0x180B, 0x180E },
+{ 0x1885, 0x1886 },
{ 0x18A9, 0x18A9 },
{ 0x1920, 0x1922 },
{ 0x1927, 0x1928 },
@@ -158,7 +159,7 @@ static const struct interval zero_width[] = {
{ 0x1CF4, 0x1CF4 },
{ 0x1CF8, 0x1CF9 },
{ 0x1DC0, 0x1DF5 },
-{ 0x1DFC, 0x1DFF },
+{ 0x1DFB, 0x1DFF },
{ 0x200B, 0x200F },
{ 0x202A, 0x202E },
{ 0x2060, 0x2064 },
@@ -171,13 +172,13 @@ static const struct interval zero_width[] = {
{ 0x3099, 0x309A },
{ 0xA66F, 0xA672 },
{ 0xA674, 0xA67D },
-{ 0xA69F, 0xA69F },
+{ 0xA69E, 0xA69F },
{ 0xA6F0, 0xA6F1 },
{ 0xA802, 0xA802 },
{ 0xA806, 0xA806 },
{ 0xA80B, 0xA80B },
{ 0xA825, 0xA826 },
-{ 0xA8C4, 0xA8C4 },
+{ 0xA8C4, 0xA8C5 },
{ 0xA8E0, 0xA8F1 },
{ 0xA926, 0xA92D },
{ 0xA947, 0xA951 },
@@ -204,7 +205,7 @@ static const struct interval zero_width[] = {
{ 0xABED, 0xABED },
{ 0xFB1E, 0xFB1E },
{ 0xFE00, 0xFE0F },
-{ 0xFE20, 0xFE2D },
+{ 0xFE20, 0xFE2F },
{ 0xFEFF, 0xFEFF },
{ 0xFFF9, 0xFFFB },
{ 0x101FD, 0x101FD },
@@ -228,16 +229,21 @@ static const struct interval zero_width[] = {
{ 0x11173, 0x11173 },
{ 0x11180, 0x11181 },
{ 0x111B6, 0x111BE },
+{ 0x111CA, 0x111CC },
{ 0x1122F, 0x11231 },
{ 0x11234, 0x11234 },
{ 0x11236, 0x11237 },
+{ 0x1123E, 0x1123E },
{ 0x112DF, 0x112DF },
{ 0x112E3, 0x112EA },
-{ 0x11301, 0x11301 },
+{ 0x11300, 0x11301 },
{ 0x1133C, 0x1133C },
{ 0x11340, 0x11340 },
{ 0x11366, 0x1136C },
{ 0x11370, 0x11374 },
+{ 0x11438, 0x1143F },
+{ 0x11442, 0x11444 },
+{ 0x11446, 0x11446 },
{ 0x114B3, 0x114B8 },
{ 0x114BA, 0x114BA },
{ 0x114BF, 0x114C0 },
@@ -245,6 +251,7 @@ static const struct interval zero_width[] = {
{ 0x115B2, 0x115B5 },
{ 0x115BC, 0x115BD },
{ 0x115BF, 0x115C0 },
+{ 0x115DC, 0x115DD },
{ 0x11633, 0x1163A },
{ 0x1163D, 0x1163D },
{ 0x1163F, 0x11640 },
@@ -252,6 +259,16 @@ static const struct interval zero_width[] = {
{ 0x116AD, 0x116AD },
{ 0x116B0, 0x116B5 },
{ 0x116B7, 0x116B7 },
+{ 0x1171D, 0x1171F },
+{ 0x11722, 0x11725 },
+{ 0x11727, 0x1172B },
+{ 0x11C30, 0x11C36 },
+{ 0x11C38, 0x11C3D },
+{ 0x11C3F, 0x11C3F },
+{ 0x11C92, 0x11CA7 },
+{ 0x11CAA, 0x11CB0 },
+{ 0x11CB2, 0x11CB3 },
+{ 0x11CB5, 0x11CB6 },
{ 0x16AF0, 0x16AF4 },
{ 0x16B30, 0x16B36 },
{ 0x16F8F, 0x16F92 },
@@ -262,31 +279,59 @@ static const struct interval zero_width[] = {
{ 0x1D185, 0x1D18B },
{ 0x1D1AA, 0x1D1AD },
{ 0x1D242, 0x1D244 },
+{ 0x1DA00, 0x1DA36 },
+{ 0x1DA3B, 0x1DA6C },
+{ 0x1DA75, 0x1DA75 },
+{ 0x1DA84, 0x1DA84 },
+{ 0x1DA9B, 0x1DA9F },
+{ 0x1DAA1, 0x1DAAF },
+{ 0x1E000, 0x1E006 },
+{ 0x1E008, 0x1E018 },
+{ 0x1E01B, 0x1E021 },
+{ 0x1E023, 0x1E024 },
+{ 0x1E026, 0x1E02A },
{ 0x1E8D0, 0x1E8D6 },
+{ 0x1E944, 0x1E94A },
{ 0xE0001, 0xE0001 },
{ 0xE0020, 0xE007F },
{ 0xE0100, 0xE01EF }
};
static const struct interval double_width[] = {
-{ /* plane */ 0x0, 0x1C },
-{ /* plane */ 0x1C, 0x21 },
-{ /* plane */ 0x21, 0x22 },
-{ /* plane */ 0x22, 0x23 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
-{ /* plane */ 0x0, 0x0 },
{ 0x1100, 0x115F },
+{ 0x231A, 0x231B },
{ 0x2329, 0x232A },
+{ 0x23E9, 0x23EC },
+{ 0x23F0, 0x23F0 },
+{ 0x23F3, 0x23F3 },
+{ 0x25FD, 0x25FE },
+{ 0x2614, 0x2615 },
+{ 0x2648, 0x2653 },
+{ 0x267F, 0x267F },
+{ 0x2693, 0x2693 },
+{ 0x26A1, 0x26A1 },
+{ 0x26AA, 0x26AB },
+{ 0x26BD, 0x26BE },
+{ 0x26C4, 0x26C5 },
+{ 0x26CE, 0x26CE },
+{ 0x26D4, 0x26D4 },
+{ 0x26EA, 0x26EA },
+{ 0x26F2, 0x26F3 },
+{ 0x26F5, 0x26F5 },
+{ 0x26FA, 0x26FA },
+{ 0x26FD, 0x26FD },
+{ 0x2705, 0x2705 },
+{ 0x270A, 0x270B },
+{ 0x2728, 0x2728 },
+{ 0x274C, 0x274C },
+{ 0x274E, 0x274E },
+{ 0x2753, 0x2755 },
+{ 0x2757, 0x2757 },
+{ 0x2795, 0x2797 },
+{ 0x27B0, 0x27B0 },
+{ 0x27BF, 0x27BF },
+{ 0x2B1B, 0x2B1C },
+{ 0x2B50, 0x2B50 },
+{ 0x2B55, 0x2B55 },
{ 0x2E80, 0x2E99 },
{ 0x2E9B, 0x2EF3 },
{ 0x2F00, 0x2FD5 },
@@ -313,11 +358,49 @@ static const struct interval double_width[] = {
{ 0xFE68, 0xFE6B },
{ 0xFF01, 0xFF60 },
{ 0xFFE0, 0xFFE6 },
+{ 0x16FE0, 0x16FE0 },
+{ 0x17000, 0x187EC },
+{ 0x18800, 0x18AF2 },
{ 0x1B000, 0x1B001 },
+{ 0x1F004, 0x1F004 },
+{ 0x1F0CF, 0x1F0
[PATCH v3 10/16] pathspec: factor global magic into its own function
Create helper functions to read the global magic environment variables
in additon to factoring out the global magic gathering logic into its
own function.
Signed-off-by: Brandon Williams
---
pathspec.c | 127 +
1 file changed, 78 insertions(+), 49 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index d44f4b6..10ce9c1 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -87,6 +87,75 @@ static void prefix_magic(struct strbuf *sb, int prefixlen,
unsigned magic)
strbuf_addf(sb, ",prefix:%d)", prefixlen);
}
+static inline int get_literal_global(void)
+{
+ static int literal = -1;
+
+ if (literal < 0)
+ literal = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
+
+ return literal;
+}
+
+static inline int get_glob_global(void)
+{
+ static int glob = -1;
+
+ if (glob < 0)
+ glob = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
+
+ return glob;
+}
+
+static inline int get_noglob_global(void)
+{
+ static int noglob = -1;
+
+ if (noglob < 0)
+ noglob = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT, 0);
+
+ return noglob;
+}
+
+static inline int get_icase_global(void)
+{
+ static int icase = -1;
+
+ if (icase < 0)
+ icase = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
+
+ return icase;
+}
+
+static int get_global_magic(int element_magic)
+{
+ int global_magic = 0;
+
+ if (get_literal_global())
+ global_magic |= PATHSPEC_LITERAL;
+
+ /* --glob-pathspec is overridden by :(literal) */
+ if (get_glob_global() && !(element_magic & PATHSPEC_LITERAL))
+ global_magic |= PATHSPEC_GLOB;
+
+ if (get_glob_global() && get_noglob_global())
+ die(_("global 'glob' and 'noglob' pathspec settings are
incompatible"));
+
+ if (get_icase_global())
+ global_magic |= PATHSPEC_ICASE;
+
+ if ((global_magic & PATHSPEC_LITERAL) &&
+ (global_magic & ~PATHSPEC_LITERAL))
+ die(_("global 'literal' pathspec setting is incompatible "
+ "with all other global pathspec settings"));
+
+ /* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
+ if (get_noglob_global() && !(element_magic & PATHSPEC_GLOB))
+ global_magic |= PATHSPEC_LITERAL;
+
+ return global_magic;
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -104,46 +173,12 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
- static int literal_global = -1;
- static int glob_global = -1;
- static int noglob_global = -1;
- static int icase_global = -1;
- unsigned magic = 0, element_magic = 0, global_magic = 0;
+ unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
- if (literal_global < 0)
- literal_global =
git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0);
- if (literal_global)
- global_magic |= PATHSPEC_LITERAL;
-
- if (glob_global < 0)
- glob_global = git_env_bool(GIT_GLOB_PATHSPECS_ENVIRONMENT, 0);
- if (glob_global)
- global_magic |= PATHSPEC_GLOB;
-
- if (noglob_global < 0)
- noglob_global = git_env_bool(GIT_NOGLOB_PATHSPECS_ENVIRONMENT,
0);
-
- if (glob_global && noglob_global)
- die(_("global 'glob' and 'noglob' pathspec settings are
incompatible"));
-
-
- if (icase_global < 0)
- icase_global = git_env_bool(GIT_ICASE_PATHSPECS_ENVIRONMENT, 0);
- if (icase_global)
- global_magic |= PATHSPEC_ICASE;
-
- if ((global_magic & PATHSPEC_LITERAL) &&
- (global_magic & ~PATHSPEC_LITERAL))
- die(_("global 'literal' pathspec setting is incompatible "
- "with all other global pathspec settings"));
-
- if (flags & PATHSPEC_LITERAL_PATH)
- global_magic = 0;
-
- if (elt[0] != ':' || literal_global ||
+ if (elt[0] != ':' || get_literal_global() ||
(flags & PATHSPEC_LITERAL_PATH)) {
; /* nothing to do */
} else if (elt[1] == '(') {
@@ -207,15 +242,11 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
magic |= element_magic;
- /* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
- if (noglob_global && !(magic & PATHSPEC_GLOB))
- global_magic |= PATHSPEC_LITERAL;
-
- /* --glob-pathspec is overridden by :(literal) */
- if ((global_magic & PATHSPEC_GLOB) && (magic & PATHSPEC_LITERAL))
- globa
[PATCH v3 11/16] pathspec: create parse_short_magic function
Factor out the logic responsible for parsing short magic into its own
function.
Signed-off-by: Brandon Williams
---
pathspec.c | 54 --
1 file changed, 36 insertions(+), 18 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 10ce9c1..94ec201 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -157,6 +157,41 @@ static int get_global_magic(int element_magic)
}
/*
+ * Parse the pathspec element looking for short magic
+ *
+ * saves all magic in 'magic'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_short_magic(unsigned *magic, const char *elem)
+{
+ const char *pos;
+
+ for (pos = elem + 1; *pos && *pos != ':'; pos++) {
+ char ch = *pos;
+ int i;
+
+ if (!is_pathspec_magic(ch))
+ break;
+
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if (pathspec_magic[i].mnemonic == ch) {
+ *magic |= pathspec_magic[i].bit;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(pathspec_magic) <= i)
+ die(_("Unimplemented pathspec magic '%c' in '%s'"),
+ ch, elem);
+ }
+
+ if (*pos == ':')
+ pos++;
+
+ return pos;
+}
+
+/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
*
@@ -220,24 +255,7 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
copyfrom++;
} else {
/* shorthand */
- for (copyfrom = elt + 1;
-*copyfrom && *copyfrom != ':';
-copyfrom++) {
- char ch = *copyfrom;
-
- if (!is_pathspec_magic(ch))
- break;
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (pathspec_magic[i].mnemonic == ch) {
- element_magic |= pathspec_magic[i].bit;
- break;
- }
- if (ARRAY_SIZE(pathspec_magic) <= i)
- die(_("Unimplemented pathspec magic '%c' in
'%s'"),
- ch, elt);
- }
- if (*copyfrom == ':')
- copyfrom++;
+ copyfrom = parse_short_magic(&element_magic, elt);
}
magic |= element_magic;
--
2.8.0.rc3.226.g39d4020
[PATCH v3 16/16] pathspec: rename prefix_pathspec to init_pathspec_item
Give a more relevant name to the prefix_pathspec function as it does
more than just prefix a pathspec element.
Signed-off-by: Brandon Williams
---
pathspec.c | 24 +++-
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 4ce2016..d4efcf6 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -297,21 +297,11 @@ static void strip_submodule_slash_expensive(struct
pathspec_item *item)
}
/*
- * Take an element of a pathspec and check for magic signatures.
- * Append the result to the prefix. Return the magic bitmap.
- *
- * For now, we only parse the syntax and throw out anything other than
- * "top" magic.
- *
- * NEEDSWORK: This needs to be rewritten when we start migrating
- * get_pathspec() users to use the "struct pathspec" interface. For
- * example, a pathspec element may be marked as case-insensitive, but
- * the prefix part must always match literally, and a single stupid
- * string cannot express such a case.
+ * Perform the initialization of a pathspec_item based on a pathspec element.
*/
-static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
- const char *prefix, int prefixlen,
- const char *elt)
+static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
+ const char *prefix, int prefixlen,
+ const char *elt)
{
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
@@ -329,6 +319,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
magic |= get_global_magic(element_magic);
}
+ item->magic = magic;
+
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
die("BUG: 'prefix' magic is supposed to be used at worktree's
root");
@@ -401,7 +393,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
/* sanity checks, pathspec matchers assume these are sane */
assert(item->nowildcard_len <= item->len &&
item->prefix <= item->len);
- return magic;
}
static int pathspec_item_cmp(const void *a_, const void *b_)
@@ -500,8 +491,7 @@ void parse_pathspec(struct pathspec *pathspec,
for (i = 0; i < n; i++) {
entry = argv[i];
- item[i].magic = prefix_pathspec(item + i, flags,
- prefix, prefixlen, entry);
+ init_pathspec_item(item + i, flags, prefix, prefixlen, entry);
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
--
2.8.0.rc3.226.g39d4020
[PATCH v3 14/16] pathspec: create strip submodule slash helpers
Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.
Signed-off-by: Brandon Williams
---
pathspec.c | 68 ++
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index a0fec49..6fd4b8e 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic,
int *prefix_len,
return parse_short_magic(magic, elem);
}
+static void strip_submodule_slash_cheap(struct pathspec_item *item)
+{
+ if (item->len >= 1 && item->match[item->len - 1] == '/') {
+ int i = cache_name_pos(item->match, item->len - 1);
+
+ if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
+ item->len--;
+ item->match[item->len] = '\0';
+ }
+ }
+}
+
+static void strip_submodule_slash_expensive(struct pathspec_item *item)
+{
+ int i;
+
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *ce = active_cache[i];
+ int ce_len = ce_namelen(ce);
+
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
+
+ if (item->len <= ce_len || item->match[ce_len] != '/' ||
+ memcmp(ce->name, item->match, ce_len))
+ continue;
+
+ if (item->len == ce_len + 1) {
+ /* strip trailing slash */
+ item->len--;
+ item->match[item->len] = '\0';
+ } else {
+ die(_("Pathspec '%s' is in submodule '%.*s'"),
+ item->original, ce_len, ce->name);
+ }
+ }
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
- int i, pathspec_prefix = -1;
+ int pathspec_prefix = -1;
/* PATHSPEC_LITERAL_PATH ignores magic */
if (flags & PATHSPEC_LITERAL_PATH) {
@@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
item->len = strlen(item->match);
item->prefix = prefixlen;
- if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
- (item->len >= 1 && item->match[item->len - 1] == '/') &&
- (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
- S_ISGITLINK(active_cache[i]->ce_mode)) {
- item->len--;
- match[item->len] = '\0';
- }
+ if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
+ strip_submodule_slash_cheap(item);
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
- for (i = 0; i < active_nr; i++) {
- struct cache_entry *ce = active_cache[i];
- int ce_len = ce_namelen(ce);
-
- if (!S_ISGITLINK(ce->ce_mode))
- continue;
-
- if (item->len <= ce_len || match[ce_len] != '/' ||
- memcmp(ce->name, match, ce_len))
- continue;
- if (item->len == ce_len + 1) {
- /* strip trailing slash */
- item->len--;
- match[item->len] = '\0';
- } else
- die (_("Pathspec '%s' is in submodule '%.*s'"),
-elt, ce_len, ce->name);
- }
+ strip_submodule_slash_expensive(item);
if (magic & PATHSPEC_LITERAL)
item->nowildcard_len = item->len;
--
2.8.0.rc3.226.g39d4020
[PATCH v3 12/16] pathspec: create parse_long_magic function
Factor out the logic responsible for parsing long magic into its own
function. As well as hoist the prefix check logic outside of the inner
loop as there isn't anything that needs to be done after matching
"prefix:".
Signed-off-by: Brandon Williams
---
pathspec.c | 92 ++
1 file changed, 57 insertions(+), 35 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 94ec201..c77be17 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -157,6 +157,60 @@ static int get_global_magic(int element_magic)
}
/*
+ * Parse the pathspec element looking for long magic
+ *
+ * saves all magic in 'magic'
+ * if prefix magic is used, save the prefix length in 'prefix_len'
+ * returns the position in 'elem' after all magic has been parsed
+ */
+static const char *parse_long_magic(unsigned *magic, int *prefix_len,
+ const char *elem)
+{
+ const char *pos;
+ const char *nextat;
+
+ for (pos = elem + 2; *pos && *pos != ')'; pos = nextat) {
+ size_t len = strcspn(pos, ",)");
+ int i;
+
+ if (pos[len] == ',')
+ nextat = pos + len + 1; /* handle ',' */
+ else
+ nextat = pos + len; /* handle ')' and '\0' */
+
+ if (!len)
+ continue;
+
+ if (starts_with(pos, "prefix:")) {
+ char *endptr;
+ *prefix_len = strtol(pos + 7, &endptr, 10);
+ if (endptr - pos != len)
+ die(_("invalid parameter for pathspec magic
'prefix'"));
+ continue;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ if (strlen(pathspec_magic[i].name) == len &&
+ !strncmp(pathspec_magic[i].name, pos, len)) {
+ *magic |= pathspec_magic[i].bit;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(pathspec_magic) <= i)
+ die(_("Invalid pathspec magic '%.*s' in '%s'"),
+ (int) len, pos, elem);
+ }
+
+ if (*pos != ')')
+ die(_("Missing ')' at the end of pathspec magic in '%s'"),
+ elem);
+ pos++;
+
+ return pos;
+}
+
+/*
* Parse the pathspec element looking for short magic
*
* saves all magic in 'magic'
@@ -218,41 +272,9 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
; /* nothing to do */
} else if (elt[1] == '(') {
/* longhand */
- const char *nextat;
- for (copyfrom = elt + 2;
-*copyfrom && *copyfrom != ')';
-copyfrom = nextat) {
- size_t len = strcspn(copyfrom, ",)");
- if (copyfrom[len] == ',')
- nextat = copyfrom + len + 1;
- else
- /* handle ')' and '\0' */
- nextat = copyfrom + len;
- if (!len)
- continue;
- for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
- if (strlen(pathspec_magic[i].name) == len &&
- !strncmp(pathspec_magic[i].name, copyfrom,
len)) {
- element_magic |= pathspec_magic[i].bit;
- break;
- }
- if (starts_with(copyfrom, "prefix:")) {
- char *endptr;
- pathspec_prefix = strtol(copyfrom + 7,
-&endptr, 10);
- if (endptr - copyfrom != len)
- die(_("invalid parameter for
pathspec magic 'prefix'"));
- /* "i" would be wrong, but it does not
matter */
- break;
- }
- }
- if (ARRAY_SIZE(pathspec_magic) <= i)
- die(_("Invalid pathspec magic '%.*s' in '%s'"),
- (int) len, copyfrom, elt);
- }
- if (*copyfrom != ')')
- die(_("Missing ')' at the end of pathspec magic in
'%s'"), elt);
- copyfrom++;
+ copyfrom = parse_long_magic(&element_magic,
+ &pathspec_prefix,
+ elt);
} else {
/* shorthand */
copyfrom = parse_short_
[PATCH v3 15/16] pathspec: small readability changes
A few small changes to improve readability. This is done by grouping related
assignments, adding blank lines, ensuring lines are <80 characters, and
adding additional comments.
Signed-off-by: Brandon Williams
---
pathspec.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 6fd4b8e..4ce2016 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -67,11 +67,11 @@ static struct pathspec_magic {
char mnemonic; /* this cannot be ':'! */
const char *name;
} pathspec_magic[] = {
- { PATHSPEC_FROMTOP, '/', "top" },
- { PATHSPEC_LITERAL, 0, "literal" },
- { PATHSPEC_GLOB, '\0', "glob" },
- { PATHSPEC_ICASE, '\0', "icase" },
- { PATHSPEC_EXCLUDE, '!', "exclude" },
+ { PATHSPEC_FROMTOP, '/', "top" },
+ { PATHSPEC_LITERAL, '\0', "literal" },
+ { PATHSPEC_GLOB,'\0', "glob" },
+ { PATHSPEC_ICASE, '\0', "icase" },
+ { PATHSPEC_EXCLUDE, '!', "exclude" },
};
static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
@@ -336,6 +336,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
die(_("%s: 'literal' and 'glob' are incompatible"), elt);
+ /* Create match string which will be used for pathspec matching */
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
@@ -343,11 +344,16 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
- match = prefix_path_gently(prefix, prefixlen, &prefixlen,
copyfrom);
+ match = prefix_path_gently(prefix, prefixlen,
+ &prefixlen, copyfrom);
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
+
item->match = match;
+ item->len = strlen(item->match);
+ item->prefix = prefixlen;
+
/*
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
@@ -364,8 +370,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
} else {
item->original = xstrdup(elt);
}
- item->len = strlen(item->match);
- item->prefix = prefixlen;
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
strip_submodule_slash_cheap(item);
@@ -373,13 +377,14 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
strip_submodule_slash_expensive(item);
- if (magic & PATHSPEC_LITERAL)
+ if (magic & PATHSPEC_LITERAL) {
item->nowildcard_len = item->len;
- else {
+ } else {
item->nowildcard_len = simple_length(item->match);
if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;
}
+
item->flags = 0;
if (magic & PATHSPEC_GLOB) {
/*
--
2.8.0.rc3.226.g39d4020
[PATCH v3 13/16] pathspec: create parse_element_magic helper
Factor out the logic responsible for the magic in a pathspec element
into its own function.
Also avoid calling into the parsing functions when
`PATHSPEC_LITERAL_PATH` is specified since it causes magic to be
ignored and all paths to be treated as literals.
Signed-off-by: Brandon Williams
---
pathspec.c | 37 -
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index c77be17..a0fec49 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -245,6 +245,19 @@ static const char *parse_short_magic(unsigned *magic,
const char *elem)
return pos;
}
+static const char *parse_element_magic(unsigned *magic, int *prefix_len,
+ const char *elem)
+{
+ if (elem[0] != ':' || get_literal_global())
+ return elem; /* nothing to do */
+ else if (elem[1] == '(')
+ /* longhand */
+ return parse_long_magic(magic, prefix_len, elem);
+ else
+ /* shorthand */
+ return parse_short_magic(magic, elem);
+}
+
/*
* Take an element of a pathspec and check for magic signatures.
* Append the result to the prefix. Return the magic bitmap.
@@ -267,26 +280,16 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
char *match;
int i, pathspec_prefix = -1;
- if (elt[0] != ':' || get_literal_global() ||
- (flags & PATHSPEC_LITERAL_PATH)) {
- ; /* nothing to do */
- } else if (elt[1] == '(') {
- /* longhand */
- copyfrom = parse_long_magic(&element_magic,
- &pathspec_prefix,
- elt);
- } else {
- /* shorthand */
- copyfrom = parse_short_magic(&element_magic, elt);
- }
-
- magic |= element_magic;
-
/* PATHSPEC_LITERAL_PATH ignores magic */
- if (flags & PATHSPEC_LITERAL_PATH)
+ if (flags & PATHSPEC_LITERAL_PATH) {
magic = PATHSPEC_LITERAL;
- else
+ } else {
+ copyfrom = parse_element_magic(&element_magic,
+ &pathspec_prefix,
+ elt);
+ magic |= element_magic;
magic |= get_global_magic(element_magic);
+ }
if (pathspec_prefix >= 0 &&
(prefixlen || (prefix && *prefix)))
--
2.8.0.rc3.226.g39d4020
[PATCH v3 01/16] mv: remove use of deprecated 'get_pathspec()'
Convert the 'internal_copy_pathspec()' function to 'prefix_path()'
instead of using the deprecated 'get_pathspec()' interface. Also,
rename 'internal_copy_pathspec()' to 'internal_prefix_pathspec()' to be
more descriptive of what the funciton is actually doing.
In addition to this, fix a memory leak caused by only duplicating some
of the pathspec elements. Instead always duplicate all of the the
pathspec elements as an intermediate step (with modificationed based on
the passed in flags). This way the intermediate strings can then be
freed after getting the result from 'prefix_path()'.
Signed-off-by: Brandon Williams
---
builtin/mv.c | 50 +++---
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index 2f43877..4e86dc5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -4,6 +4,7 @@
* Copyright (C) 2006 Johannes Schindelin
*/
#include "builtin.h"
+#include "pathspec.h"
#include "lockfile.h"
#include "dir.h"
#include "cache-tree.h"
@@ -19,31 +20,42 @@ static const char * const builtin_mv_usage[] = {
#define DUP_BASENAME 1
#define KEEP_TRAILING_SLASH 2
-static const char **internal_copy_pathspec(const char *prefix,
- const char **pathspec,
- int count, unsigned flags)
+static const char **internal_prefix_pathspec(const char *prefix,
+const char **pathspec,
+int count, unsigned flags)
{
int i;
const char **result;
+ int prefixlen = prefix ? strlen(prefix) : 0;
ALLOC_ARRAY(result, count + 1);
- COPY_ARRAY(result, pathspec, count);
- result[count] = NULL;
+
+ /* Create an intermediate copy of the pathspec based on the flags */
for (i = 0; i < count; i++) {
- int length = strlen(result[i]);
+ int length = strlen(pathspec[i]);
int to_copy = length;
+ char *it;
while (!(flags & KEEP_TRAILING_SLASH) &&
- to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
+ to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
to_copy--;
- if (to_copy != length || flags & DUP_BASENAME) {
- char *it = xmemdupz(result[i], to_copy);
- if (flags & DUP_BASENAME) {
- result[i] = xstrdup(basename(it));
- free(it);
- } else
- result[i] = it;
+
+ it = xmemdupz(pathspec[i], to_copy);
+ if (flags & DUP_BASENAME) {
+ result[i] = xstrdup(basename(it));
+ free(it);
+ } else {
+ result[i] = it;
}
}
- return get_pathspec(prefix, result);
+ result[count] = NULL;
+
+ /* Prefix the pathspec and free the old intermediate strings */
+ for (i = 0; i < count; i++) {
+ const char *match = prefix_path(prefix, prefixlen, result[i]);
+ free((char *) result[i]);
+ result[i] = match;
+ }
+
+ return result;
}
static const char *add_slash(const char *path)
@@ -130,7 +142,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (read_cache() < 0)
die(_("index file corrupt"));
- source = internal_copy_pathspec(prefix, argv, argc, 0);
+ source = internal_prefix_pathspec(prefix, argv, argc, 0);
modes = xcalloc(argc, sizeof(enum update_mode));
/*
* Keep trailing slash, needed to let
@@ -140,16 +152,16 @@ int cmd_mv(int argc, const char **argv, const char
*prefix)
flags = KEEP_TRAILING_SLASH;
if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
flags = 0;
- dest_path = internal_copy_pathspec(prefix, argv + argc, 1, flags);
+ dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
submodule_gitfile = xcalloc(argc, sizeof(char *));
if (dest_path[0][0] == '\0')
/* special case: "." was normalized to "" */
- destination = internal_copy_pathspec(dest_path[0], argv, argc,
DUP_BASENAME);
+ destination = internal_prefix_pathspec(dest_path[0], argv,
argc, DUP_BASENAME);
else if (!lstat(dest_path[0], &st) &&
S_ISDIR(st.st_mode)) {
dest_path[0] = add_slash(dest_path[0]);
- destination = internal_copy_pathspec(dest_path[0], argv, argc,
DUP_BASENAME);
+ destination = internal_prefix_pathspec(dest_path[0], argv,
argc, DUP_BASENAME);
} else {
if (argc != 1)
die(_("destination '%s' is not a directory"),
dest_path[0]);
--
[PATCH v3 06/16] pathspec: copy and free owned memory
The 'original' string entry in a pathspec_item is only duplicated some
of the time, instead always make a copy of the original and take
ownership of the memory.
Since both 'match' and 'original' string entries in a pathspec_item are
owned by the pathspec struct, they need to be freed when clearing the
pathspec struct (in 'clear_pathspec()') and duplicated when copying the
pathspec struct (in 'copy_pathspec()').
Also change the type of 'match' and 'original' to 'char *' in order to
more explicitly show the ownership of the memory.
Signed-off-by: Brandon Williams
---
pathspec.c | 22 ++
pathspec.h | 4 ++--
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 1f918cb..8f367f0 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -259,8 +259,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL);
- } else
- item->original = elt;
+ } else {
+ item->original = xstrdup(elt);
+ }
item->len = strlen(item->match);
item->prefix = prefixlen;
@@ -388,8 +389,8 @@ void parse_pathspec(struct pathspec *pathspec,
die("BUG: PATHSPEC_PREFER_CWD requires arguments");
pathspec->items = item = xcalloc(1, sizeof(*item));
- item->match = prefix;
- item->original = prefix;
+ item->match = xstrdup(prefix);
+ item->original = xstrdup(prefix);
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
pathspec->nr = 1;
@@ -453,13 +454,26 @@ void parse_pathspec(struct pathspec *pathspec,
void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
+ int i;
+
*dst = *src;
ALLOC_ARRAY(dst->items, dst->nr);
COPY_ARRAY(dst->items, src->items, dst->nr);
+
+ for (i = 0; i < dst->nr; i++) {
+ dst->items[i].match = xstrdup(src->items[i].match);
+ dst->items[i].original = xstrdup(src->items[i].original);
+ }
}
void clear_pathspec(struct pathspec *pathspec)
{
+ int i;
+
+ for (i = 0; i < pathspec->nr; i++) {
+ free(pathspec->items[i].match);
+ free(pathspec->items[i].original);
+ }
free(pathspec->items);
pathspec->items = NULL;
}
diff --git a/pathspec.h b/pathspec.h
index 70a592e..49fd823 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -25,8 +25,8 @@ struct pathspec {
unsigned magic;
int max_depth;
struct pathspec_item {
- const char *match;
- const char *original;
+ char *match;
+ char *original;
unsigned magic;
int len, prefix;
int nowildcard_len;
--
2.8.0.rc3.226.g39d4020
[PATCH v3 07/16] pathspec: remove unused variable from unsupported_magic
Removed unused variable 'n' from the 'unsupported_magic()' function.
Signed-off-by: Brandon Williams
---
pathspec.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 8f367f0..ec0d590 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -333,8 +333,8 @@ static void NORETURN unsupported_magic(const char *pattern,
unsigned short_magic)
{
struct strbuf sb = STRBUF_INIT;
- int i, n;
- for (n = i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
const struct pathspec_magic *m = pathspec_magic + i;
if (!(magic & m->bit))
continue;
@@ -344,7 +344,6 @@ static void NORETURN unsupported_magic(const char *pattern,
strbuf_addf(&sb, "'%c'", m->mnemonic);
else
strbuf_addf(&sb, "'%s'", m->name);
- n++;
}
/*
* We may want to substitute "this command" with a command
--
2.8.0.rc3.226.g39d4020
[PATCH v3 05/16] pathspec: remove the deprecated get_pathspec function
Now that all callers of the old 'get_pathspec' interface have been
migrated to use the new pathspec struct interface it can be removed
from the codebase.
Since there are no more users of the '_raw' field in the pathspec struct
it can also be removed. This patch also removes the old functionality
of modifying the const char **argv array that was passed into
parse_pathspec. Instead the constructed 'match' string (which is a
pathspec element with the prefix prepended) is only stored in its
corresponding pathspec_item entry.
Signed-off-by: Brandon Williams
---
Documentation/technical/api-setup.txt | 2 --
cache.h | 1 -
pathspec.c| 42 +++
pathspec.h| 1 -
4 files changed, 3 insertions(+), 43 deletions(-)
diff --git a/Documentation/technical/api-setup.txt
b/Documentation/technical/api-setup.txt
index 540e455..eb1fa98 100644
--- a/Documentation/technical/api-setup.txt
+++ b/Documentation/technical/api-setup.txt
@@ -27,8 +27,6 @@ parse_pathspec(). This function takes several arguments:
- prefix and args come from cmd_* functions
-get_pathspec() is obsolete and should never be used in new code.
-
parse_pathspec() helps catch unsupported features and reject them
politely. At a lower level, different pathspec-related functions may
not support the same set of features. Such pathspec-sensitive
diff --git a/cache.h b/cache.h
index a50a61a..0f80e01 100644
--- a/cache.h
+++ b/cache.h
@@ -514,7 +514,6 @@ extern void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
-extern const char **get_pathspec(const char *prefix, const char **pathspec);
extern void setup_work_tree(void);
extern const char *setup_git_directory_gently(int *);
extern const char *setup_git_directory(void);
diff --git a/pathspec.c b/pathspec.c
index 22ca74a..1f918cb 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -103,7 +103,7 @@ static void prefix_short_magic(struct strbuf *sb, int
prefixlen,
*/
static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned *p_short_magic,
- const char **raw, unsigned flags,
+ unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
@@ -240,7 +240,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
if (!match)
die(_("%s: '%s' is outside repository"), elt, copyfrom);
}
- *raw = item->match = match;
+ item->match = match;
/*
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
@@ -381,8 +381,6 @@ void parse_pathspec(struct pathspec *pathspec,
/* No arguments with prefix -> prefix pathspec */
if (!entry) {
- static const char *raw[2];
-
if (flags & PATHSPEC_PREFER_FULL)
return;
@@ -394,10 +392,7 @@ void parse_pathspec(struct pathspec *pathspec,
item->original = prefix;
item->nowildcard_len = item->len = strlen(prefix);
item->prefix = item->len;
- raw[0] = prefix;
- raw[1] = NULL;
pathspec->nr = 1;
- pathspec->_raw = raw;
return;
}
@@ -415,7 +410,6 @@ void parse_pathspec(struct pathspec *pathspec,
pathspec->nr = n;
ALLOC_ARRAY(pathspec->items, n);
item = pathspec->items;
- pathspec->_raw = argv;
prefixlen = prefix ? strlen(prefix) : 0;
for (i = 0; i < n; i++) {
@@ -423,7 +417,7 @@ void parse_pathspec(struct pathspec *pathspec,
entry = argv[i];
item[i].magic = prefix_pathspec(item + i, &short_magic,
- argv + i, flags,
+ flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -457,36 +451,6 @@ void parse_pathspec(struct pathspec *pathspec,
}
}
-/*
- * N.B. get_pathspec() is deprecated in favor of the "struct pathspec"
- * based interface - see pathspec.c:parse_pathspec().
- *
- * Arguments:
- * - prefix - a path relative to the root of the working tree
- * - pathspec - a list of paths underneath the prefix path
- *
- * Iterates over pathspec, prepending each path with prefix,
- * and return the resulting list.
- *
- * If pathspec is empty, return a singleton list containing prefix.
- *
- * If pathspec and prefix are both empty, return an empty list.
- *
- * This is typically used by built-in commands such as add.c, in order
- * to normalize argv arguments provided to the built-in into a li
[PATCH v3 08/16] pathspec: always show mnemonic and name in unsupported_magic
For better clarity, always show the mnemonic and name of the unsupported
magic being used. This lets users have a more clear understanding of
what magic feature isn't supported. And if they supplied a mnemonic,
the user will be told what its corresponding name is which will allow
them to more easily search the man pages for that magic type.
This also avoids passing an extra parameter around the pathspec
initialization code.
Signed-off-by: Brandon Williams
---
pathspec.c | 21 +++--
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index ec0d590..609c58f 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -101,9 +101,7 @@ static void prefix_short_magic(struct strbuf *sb, int
prefixlen,
* the prefix part must always match literally, and a single stupid
* string cannot express such a case.
*/
-static unsigned prefix_pathspec(struct pathspec_item *item,
- unsigned *p_short_magic,
- unsigned flags,
+static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
const char *prefix, int prefixlen,
const char *elt)
{
@@ -210,7 +208,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
magic |= short_magic;
- *p_short_magic = short_magic;
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -329,8 +326,7 @@ static int pathspec_item_cmp(const void *a_, const void *b_)
}
static void NORETURN unsupported_magic(const char *pattern,
- unsigned magic,
- unsigned short_magic)
+ unsigned magic)
{
struct strbuf sb = STRBUF_INIT;
int i;
@@ -340,8 +336,9 @@ static void NORETURN unsupported_magic(const char *pattern,
continue;
if (sb.len)
strbuf_addch(&sb, ' ');
- if (short_magic & m->bit)
- strbuf_addf(&sb, "'%c'", m->mnemonic);
+
+ if (m->mnemonic)
+ strbuf_addf(&sb, "'(%c)%s'", m->mnemonic, m->name);
else
strbuf_addf(&sb, "'%s'", m->name);
}
@@ -413,11 +410,9 @@ void parse_pathspec(struct pathspec *pathspec,
prefixlen = prefix ? strlen(prefix) : 0;
for (i = 0; i < n; i++) {
- unsigned short_magic;
entry = argv[i];
- item[i].magic = prefix_pathspec(item + i, &short_magic,
- flags,
+ item[i].magic = prefix_pathspec(item + i, flags,
prefix, prefixlen, entry);
if ((flags & PATHSPEC_LITERAL_PATH) &&
!(magic_mask & PATHSPEC_LITERAL))
@@ -425,9 +420,7 @@ void parse_pathspec(struct pathspec *pathspec,
if (item[i].magic & PATHSPEC_EXCLUDE)
nr_exclude++;
if (item[i].magic & magic_mask)
- unsupported_magic(entry,
- item[i].magic & magic_mask,
- short_magic);
+ unsupported_magic(entry, item[i].magic & magic_mask);
if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
has_symlink_leading_path(item[i].match, item[i].len)) {
--
2.8.0.rc3.226.g39d4020
[PATCH v3 09/16] pathspec: simpler logic to prefix original pathspec elements
The logic used to prefix an original pathspec element with 'prefix'
magic is more general purpose and can be used for more than just short
magic. Remove the extra code paths and rename 'prefix_short_magic' to
'prefix_magic' to better indicate that it can be used in more general
situations.
Also, slightly change the logic which decides when to prefix the
original element in order to prevent a pathspec of "." from getting
converted to "" (empty string).
Signed-off-by: Brandon Williams
---
pathspec.c | 33 +
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index 609c58f..d44f4b6 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -74,13 +74,12 @@ static struct pathspec_magic {
{ PATHSPEC_EXCLUDE, '!', "exclude" },
};
-static void prefix_short_magic(struct strbuf *sb, int prefixlen,
- unsigned short_magic)
+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
{
int i;
strbuf_addstr(sb, ":(");
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
- if (short_magic & pathspec_magic[i].bit) {
+ if (magic & pathspec_magic[i].bit) {
if (sb->buf[sb->len - 1] != '(')
strbuf_addch(sb, ',');
strbuf_addstr(sb, pathspec_magic[i].name);
@@ -109,8 +108,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
static int glob_global = -1;
static int noglob_global = -1;
static int icase_global = -1;
- unsigned magic = 0, short_magic = 0, global_magic = 0;
- const char *copyfrom = elt, *long_magic_end = NULL;
+ unsigned magic = 0, element_magic = 0, global_magic = 0;
+ const char *copyfrom = elt;
char *match;
int i, pathspec_prefix = -1;
@@ -164,7 +163,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
if (strlen(pathspec_magic[i].name) == len &&
!strncmp(pathspec_magic[i].name, copyfrom,
len)) {
- magic |= pathspec_magic[i].bit;
+ element_magic |= pathspec_magic[i].bit;
break;
}
if (starts_with(copyfrom, "prefix:")) {
@@ -183,7 +182,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
}
if (*copyfrom != ')')
die(_("Missing ')' at the end of pathspec magic in
'%s'"), elt);
- long_magic_end = copyfrom;
copyfrom++;
} else {
/* shorthand */
@@ -196,7 +194,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
break;
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
if (pathspec_magic[i].mnemonic == ch) {
- short_magic |= pathspec_magic[i].bit;
+ element_magic |= pathspec_magic[i].bit;
break;
}
if (ARRAY_SIZE(pathspec_magic) <= i)
@@ -207,7 +205,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned flags,
copyfrom++;
}
- magic |= short_magic;
+ magic |= element_magic;
/* --noglob-pathspec adds :(literal) _unless_ :(glob) is specified */
if (noglob_global && !(magic & PATHSPEC_GLOB))
@@ -242,18 +240,13 @@ static unsigned prefix_pathspec(struct pathspec_item
*item, unsigned flags,
* Prefix the pathspec (keep all magic) and assign to
* original. Useful for passing to another command.
*/
- if (flags & PATHSPEC_PREFIX_ORIGIN) {
+ if ((flags & PATHSPEC_PREFIX_ORIGIN) &&
+ prefixlen && !literal_global) {
struct strbuf sb = STRBUF_INIT;
- if (prefixlen && !literal_global) {
- /* Preserve the actual prefix length of each pattern */
- if (short_magic)
- prefix_short_magic(&sb, prefixlen, short_magic);
- else if (long_magic_end) {
- strbuf_add(&sb, elt, long_magic_end - elt);
- strbuf_addf(&sb, ",prefix:%d)", prefixlen);
- } else
- strbuf_addf(&sb, ":(prefix:%d)", prefixlen);
- }
+
+ /* Preserve the actual prefix length of each pattern */
+ prefix_magic(&sb, prefixlen, element_magic);
+
strbuf_addstr(&sb, match
[PATCH v3 03/16] dir: convert fill_directory to use the pathspec struct interface
Convert 'fill_directory()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams
---
dir.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index aadf073..15f7c99 100644
--- a/dir.c
+++ b/dir.c
@@ -174,17 +174,21 @@ char *common_prefix(const struct pathspec *pathspec)
int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
{
- size_t len;
+ char *prefix;
+ size_t prefix_len;
/*
* Calculate common prefix for the pathspec, and
* use that to optimize the directory walk
*/
- len = common_prefix_len(pathspec);
+ prefix = common_prefix(pathspec);
+ prefix_len = prefix ? strlen(prefix) : 0;
/* Read the directory and prune it */
- read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len,
pathspec);
- return len;
+ read_directory(dir, prefix, prefix_len, pathspec);
+
+ free(prefix);
+ return prefix_len;
}
int within_depth(const char *name, int namelen,
--
2.8.0.rc3.226.g39d4020
[PATCH v3 02/16] dir: remove struct path_simplify
Teach simplify_away() and exclude_matches_pathspec() to handle struct
pathspec directly, eliminating the need for the struct path_simplify.
Also renamed the len parameter to pathlen in exclude_matches_pathspec()
to match the parameter names used in simplify_away().
Signed-off-by: Brandon Williams
---
dir.c | 154 ++
1 file changed, 60 insertions(+), 94 deletions(-)
diff --git a/dir.c b/dir.c
index bfa8c8a..aadf073 100644
--- a/dir.c
+++ b/dir.c
@@ -16,11 +16,6 @@
#include "varint.h"
#include "ewah/ewok.h"
-struct path_simplify {
- int len;
- const char *path;
-};
-
/*
* Tells read_directory_recursive how a file or directory should be treated.
* Values are ordered by significance, e.g. if a directory contains both
@@ -50,7 +45,7 @@ struct cached_dir {
static enum path_treatment read_directory_recursive(struct dir_struct *dir,
const char *path, int len, struct untracked_cache_dir *untracked,
- int check_only, const struct path_simplify *simplify);
+ int check_only, const struct pathspec *pathspec);
static int get_dtype(struct dirent *de, const char *path, int len);
int fspathcmp(const char *a, const char *b)
@@ -1312,7 +1307,7 @@ static enum exist_status directory_exists_in_index(const
char *dirname, int len)
static enum path_treatment treat_directory(struct dir_struct *dir,
struct untracked_cache_dir *untracked,
const char *dirname, int len, int baselen, int exclude,
- const struct path_simplify *simplify)
+ const struct pathspec *pathspec)
{
/* The "len-1" is to strip the final '/' */
switch (directory_exists_in_index(dirname, len-1)) {
@@ -1341,7 +1336,7 @@ static enum path_treatment treat_directory(struct
dir_struct *dir,
untracked = lookup_untracked(dir->untracked, untracked,
dirname + baselen, len - baselen);
return read_directory_recursive(dir, dirname, len,
- untracked, 1, simplify);
+ untracked, 1, pathspec);
}
/*
@@ -1349,24 +1344,25 @@ static enum path_treatment treat_directory(struct
dir_struct *dir,
* reading - if the path cannot possibly be in the pathspec,
* return true, and we'll skip it early.
*/
-static int simplify_away(const char *path, int pathlen, const struct
path_simplify *simplify)
+static int simplify_away(const char *path, int pathlen,
+const struct pathspec *pathspec)
{
- if (simplify) {
- for (;;) {
- const char *match = simplify->path;
- int len = simplify->len;
+ int i;
- if (!match)
- break;
- if (len > pathlen)
- len = pathlen;
- if (!memcmp(path, match, len))
- return 0;
- simplify++;
- }
- return 1;
+ if (!pathspec || !pathspec->nr)
+ return 0;
+
+ for (i = 0; i < pathspec->nr; i++) {
+ const struct pathspec_item *item = &pathspec->items[i];
+ int len = item->nowildcard_len;
+
+ if (len > pathlen)
+ len = pathlen;
+ if (!ps_strncmp(item, item->match, path, len))
+ return 0;
}
- return 0;
+
+ return 1;
}
/*
@@ -1380,19 +1376,25 @@ static int simplify_away(const char *path, int pathlen,
const struct path_simpli
* 2. the path is a directory prefix of some element in the
* pathspec
*/
-static int exclude_matches_pathspec(const char *path, int len,
- const struct path_simplify *simplify)
-{
- if (simplify) {
- for (; simplify->path; simplify++) {
- if (len == simplify->len
- && !memcmp(path, simplify->path, len))
- return 1;
- if (len < simplify->len
- && simplify->path[len] == '/'
- && !memcmp(path, simplify->path, len))
- return 1;
- }
+static int exclude_matches_pathspec(const char *path, int pathlen,
+ const struct pathspec *pathspec)
+{
+ int i;
+
+ if (!pathspec || !pathspec->nr)
+ return 0;
+
+ for (i = 0; i < pathspec->nr; i++) {
+ const struct pathspec_item *item = &pathspec->items[i];
+ int len = item->nowildcard_len;
+
+ if (len == pathlen &&
+ !ps_strncmp(item, item->match, path, pathlen))
+ return 1;
+ if (len > pathlen &&
+ item->match[pathlen] == '/' &&
+ !ps_strncmp(item, item->match,
[PATCH v3 00/16] pathspec cleanup
Differences in v3:
* more readable strip submodule slash helper function which conforms to git's
style guide. [14/16]
* instead of having create_simply() use struct pathspec directly, remove the
struct path_simplify entirely and use struct pathspec directly in both
simplify_away() and exclude_matches_pathspec(). [02/16]
* small style issues corrected from v2. [15/16]
Brandon Williams (16):
mv: remove use of deprecated 'get_pathspec()'
dir: remove struct path_simplify
dir: convert fill_directory to use the pathspec struct interface
ls-tree: convert show_recursive to use the pathspec struct interface
pathspec: remove the deprecated get_pathspec function
pathspec: copy and free owned memory
pathspec: remove unused variable from unsupported_magic
pathspec: always show mnemonic and name in unsupported_magic
pathspec: simpler logic to prefix original pathspec elements
pathspec: factor global magic into its own function
pathspec: create parse_short_magic function
pathspec: create parse_long_magic function
pathspec: create parse_element_magic helper
pathspec: create strip submodule slash helpers
pathspec: small readability changes
pathspec: rename prefix_pathspec to init_pathspec_item
Documentation/technical/api-setup.txt | 2 -
builtin/ls-tree.c | 16 +-
builtin/mv.c | 50 ++--
cache.h | 1 -
dir.c | 166 +---
pathspec.c| 476 +++---
pathspec.h| 5 +-
7 files changed, 369 insertions(+), 347 deletions(-)
--- interdiff on 'origin/bw/pathspec-cleanup'
diff --git a/dir.c b/dir.c
index a50b6f0..15f7c99 100644
--- a/dir.c
+++ b/dir.c
@@ -16,11 +16,6 @@
#include "varint.h"
#include "ewah/ewok.h"
-struct path_simplify {
- int len;
- const char *path;
-};
-
/*
* Tells read_directory_recursive how a file or directory should be treated.
* Values are ordered by significance, e.g. if a directory contains both
@@ -50,7 +45,7 @@ struct cached_dir {
static enum path_treatment read_directory_recursive(struct dir_struct *dir,
const char *path, int len, struct untracked_cache_dir *untracked,
- int check_only, const struct path_simplify *simplify);
+ int check_only, const struct pathspec *pathspec);
static int get_dtype(struct dirent *de, const char *path, int len);
int fspathcmp(const char *a, const char *b)
@@ -1316,7 +1311,7 @@ static enum exist_status directory_exists_in_index(const
char *dirname, int len)
static enum path_treatment treat_directory(struct dir_struct *dir,
struct untracked_cache_dir *untracked,
const char *dirname, int len, int baselen, int exclude,
- const struct path_simplify *simplify)
+ const struct pathspec *pathspec)
{
/* The "len-1" is to strip the final '/' */
switch (directory_exists_in_index(dirname, len-1)) {
@@ -1345,7 +1340,7 @@ static enum path_treatment treat_directory(struct
dir_struct *dir,
untracked = lookup_untracked(dir->untracked, untracked,
dirname + baselen, len - baselen);
return read_directory_recursive(dir, dirname, len,
- untracked, 1, simplify);
+ untracked, 1, pathspec);
}
/*
@@ -1353,24 +1348,25 @@ static enum path_treatment treat_directory(struct
dir_struct *dir,
* reading - if the path cannot possibly be in the pathspec,
* return true, and we'll skip it early.
*/
-static int simplify_away(const char *path, int pathlen, const struct
path_simplify *simplify)
+static int simplify_away(const char *path, int pathlen,
+const struct pathspec *pathspec)
{
- if (simplify) {
- for (;;) {
- const char *match = simplify->path;
- int len = simplify->len;
+ int i;
- if (!match)
- break;
- if (len > pathlen)
- len = pathlen;
- if (!memcmp(path, match, len))
- return 0;
- simplify++;
- }
- return 1;
+ if (!pathspec || !pathspec->nr)
+ return 0;
+
+ for (i = 0; i < pathspec->nr; i++) {
+ const struct pathspec_item *item = &pathspec->items[i];
+ int len = item->nowildcard_len;
+
+ if (len > pathlen)
+ len = pathlen;
+ if (!ps_strncmp(item, item->match, path, len))
+ return 0;
}
- return 0;
+
+ return 1;
}
/*
@@ -1384,19 +1380,25 @@ static int simplify_away(const char *path, int pathlen,
const struct path_simpli
* 2. the path is a directory prefix of some element in the
*
[PATCH v3 04/16] ls-tree: convert show_recursive to use the pathspec struct interface
Convert 'show_recursive()' to use the pathspec struct interface from
using the '_raw' entry in the pathspec struct.
Signed-off-by: Brandon Williams
---
builtin/ls-tree.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 0e30d86..d7ebeb4 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -31,21 +31,18 @@ static const char * const ls_tree_usage[] = {
static int show_recursive(const char *base, int baselen, const char *pathname)
{
- const char **s;
+ int i;
if (ls_options & LS_RECURSIVE)
return 1;
- s = pathspec._raw;
- if (!s)
+ if (!pathspec.nr)
return 0;
- for (;;) {
- const char *spec = *s++;
+ for (i = 0; i < pathspec.nr; i++) {
+ const char *spec = pathspec.items[i].match;
int len, speclen;
- if (!spec)
- return 0;
if (strncmp(base, spec, baselen))
continue;
len = strlen(pathname);
@@ -59,6 +56,7 @@ static int show_recursive(const char *base, int baselen,
const char *pathname)
continue;
return 1;
}
+ return 0;
}
static int show_tree(const unsigned char *sha1, struct strbuf *base,
@@ -175,8 +173,8 @@ int cmd_ls_tree(int argc, const char **argv, const char
*prefix)
* cannot be lifted until it is converted to use
* match_pathspec() or tree_entry_interesting()
*/
- parse_pathspec(&pathspec, PATHSPEC_GLOB | PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE,
+ parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
PATHSPEC_PREFER_CWD,
prefix, argv + 1);
for (i = 0; i < pathspec.nr; i++)
--
2.8.0.rc3.226.g39d4020
[PATCHv3] git-p4: support git worktrees
git-p4 would attempt to find the git directory using
its own specific code, which did not know about git
worktrees.
Rework it to use "git rev-parse --git-dir" instead.
Add test cases for worktree usage and specifying
git directory via --git-dir and $GIT_DIR.
Signed-off-by: Luke Diamand
---
git-p4.py | 17 +
t/t9800-git-p4-basic.sh | 20
t/t9806-git-p4-options.sh | 32
3 files changed, 65 insertions(+), 4 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52..6a1f65f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -85,6 +85,16 @@ def p4_build_cmd(cmd):
real_cmd += cmd
return real_cmd
+def git_dir(path):
+""" Return TRUE if the given path is a git directory (/path/to/dir/.git).
+This won't automatically add ".git" to a directory.
+"""
+d = read_pipe(["git", "--git-dir", path, "rev-parse", "--git-dir"],
True).strip()
+if not d or len(d) == 0:
+return None
+else:
+return d
+
def chdir(path, is_client_path=False):
"""Do chdir to the given path, and set the PWD environment
variable for use by P4. It does not look at getcwd() output.
@@ -563,10 +573,7 @@ def currentGitBranch():
return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
def isValidGitDir(path):
-if (os.path.exists(path + "/HEAD")
-and os.path.exists(path + "/refs") and os.path.exists(path +
"/objects")):
-return True;
-return False
+return git_dir(path) != None
def parseRevision(ref):
return read_pipe("git rev-parse %s" % ref).strip()
@@ -3682,6 +3689,7 @@ def main():
if cmd.gitdir == None:
cmd.gitdir = os.path.abspath(".git")
if not isValidGitDir(cmd.gitdir):
+# "rev-parse --git-dir" without arguments will try $PWD/.git
cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
if os.path.exists(cmd.gitdir):
cdup = read_pipe("git rev-parse --show-cdup").strip()
@@ -3694,6 +3702,7 @@ def main():
else:
die("fatal: cannot locate git repository at %s" % cmd.gitdir)
+# so git commands invoked from the P4 workspace will succeed
os.environ["GIT_DIR"] = cmd.gitdir
if not cmd.run(args):
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 0730f18..093e9bd 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
)
'
+test_expect_success 'submit from worktree' '
+ test_when_finished cleanup_git &&
+ git p4 clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git worktree add ../worktree-test
+ ) &&
+ (
+ cd "$git/../worktree-test" &&
+ test_commit "worktree-commit" &&
+ git config git-p4.skipSubmitEdit true &&
+ git p4 submit
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ test_path_is_file worktree-commit.t
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
diff --git a/t/t9806-git-p4-options.sh b/t/t9806-git-p4-options.sh
index 254d428..1ab76c4 100755
--- a/t/t9806-git-p4-options.sh
+++ b/t/t9806-git-p4-options.sh
@@ -269,6 +269,38 @@ test_expect_success 'submit works with two branches' '
)
'
+test_expect_success 'use --git-dir option and GIT_DIR' '
+ test_when_finished cleanup_git &&
+ git p4 clone //depot --destination="$git" &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ test_commit first-change &&
+ git p4 submit --git-dir "$git"
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ test_path_is_file first-change.t &&
+ echo "cli_file" >cli_file.t &&
+ p4 add cli_file.t &&
+ p4 submit -d "cli change"
+ ) &&
+ (git --git-dir "$git" p4 sync) &&
+ (cd "$git" && git checkout -q p4/master) &&
+ test_path_is_file "$git"/cli_file.t &&
+ (
+ cd "$cli" &&
+ echo "cli_file2" >cli_file2.t &&
+ p4 add cli_file2.t &&
+ p4 submit -d "cli change2"
+ ) &&
+ (GIT_DIR="$git" git p4 sync) &&
+ (cd "$git" && git checkout -q p4/master) &&
+ test_path_is_file "$git"/cli_file2.t
+'
+
+
test_expect_success 'kill p4d' '
kill_p4d
'
--
2.8.2.703.g78b384c.dirty
Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
On 12/09, Brandon Williams wrote: > On 12/09, Duy Nguyen wrote: > > On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams wrote: > > > On 12/08, Duy Nguyen wrote: > > >> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams > > >> wrote: > > >> > On 12/07, Duy Nguyen wrote: > > >> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams > > >> >> wrote: > > >> >> > Convert 'create_simplify()' to use the pathspec struct interface > > >> >> > from > > >> >> > using the '_raw' entry in the pathspec. > > >> >> > > >> >> It would be even better to kill this create_simplify() and let > > >> >> simplify_away() handle struct pathspec directly. > > >> >> > > >> >> There is a bug in this code, that might have been found if we > > >> >> simpify_away() handled pathspec directly: the memcmp() in > > >> >> simplify_away() will not play well with :(icase) magic. My bad. If > > >> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on > > >> >> maybe we can teach simplify_away() to do strncasecmp instead. We could > > >> >> ignore exclude patterns there too (although not excluding is not a > > >> >> bug). > > >> > > > >> > So are you implying that the simplify struct needs to be killed? That > > >> > way the pathspec struct itself is being passed around instead? > > >> > > >> Yes. simplify struct was a thing when pathspec was an array of char *. > > >> At this point I think it can retire (when we have time to retire it) > > > > > > Alright, then for now I can leave this change as is and have a follow up > > > series that kills the simplify struct. > > > > Do let me know if you decide to drop it, so I can put it back in my backlog. > > K will do > This actually turned out to be more straight forward than I thought. I'll reroll this series again (with a few other changes) and include killing the simplify struct. -- Brandon Williams
Re: [PATCH] fix pushing to //server/share/dir paths on Windows
Johannes Sixt writes:
> There is a change in behavior: \\server\share is not transformed
> into //server/share anymore, but all subsequent directory separators
> are rewritten to '/'. This should not make a difference; Windows can
> handle the mix.
I saw Dscho had a similar "windows can handle the mix" change in an
earlier development cycle, I think, and this is being consistent.
> Another long-standing bug uncovered by the quarantine series.
>
> Dscho, it looks like this could fix the original report at
> https://github.com/git-for-windows/git/issues/979
>
> This patch should cook well because of the change in behavior.
> I would not be surprised if there is some fall-out.
>
> The other bug I'm alluding to, I still have to investigate. I do
> not think that it can be counted as fall-out.
>
> path.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
Thanks.
> diff --git a/path.c b/path.c
> index 52d889c88e..02dc70fb92 100644
> --- a/path.c
> +++ b/path.c
> @@ -991,7 +991,7 @@ const char *remove_leading_path(const char *in, const
> char *prefix)
> *
> * Performs the following normalizations on src, storing the result in dst:
> * - Ensures that components are separated by '/' (Windows only)
> - * - Squashes sequences of '/'.
> + * - Squashes sequences of '/' except "//server/share" on Windows
"on windows" because offset_1st_component() does the magic only
there? Makes sense.
> * - Removes "." components.
> * - Removes ".." components, and the components the precede them.
> * Returns failure (non-zero) if a ".." component appears as first path
> @@ -1014,17 +1014,23 @@ const char *remove_leading_path(const char *in, const
> char *prefix)
> int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
> {
> char *dst0;
> - int i;
> -
> - for (i = has_dos_drive_prefix(src); i > 0; i--)
> - *dst++ = *src++;
> - dst0 = dst;
> + int offset;
>
> - if (is_dir_sep(*src)) {
> + /*
> + * Handle initial part of absolute path: "/", "C:/", "\\server\share/".
> + */
> + offset = offset_1st_component(src);
> + if (offset) {
> + /* Convert the trailing separator to '/' on Windows. */
> + memcpy(dst, src, offset - 1);
> + dst += offset - 1;
> *dst++ = '/';
> - while (is_dir_sep(*src))
> - src++;
> + src += offset;
> }
> + dst0 = dst;
By resetting dst0 here, we ensure that up_one that is triggered by
seeing "../" will not escape the \\server\share\ part, which makes
sense to me.
> + while (is_dir_sep(*src))
> + src++;
>
> for (;;) {
> char c = *src;
Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode
Johannes Schindelin writes:
> @@ -1493,9 +1498,26 @@ static int pick_commits(struct todo_list *todo_list,
> struct replay_opts *opts)
> }
>
> if (is_rebase_i(opts)) {
> + struct strbuf buf = STRBUF_INIT;
> +
> /* Stopped in the middle, as planned? */
> if (todo_list->current < todo_list->nr)
> return 0;
> +
> + if (opts->verbose) {
> + const char *argv[] = {
> + "diff-tree", "--stat", NULL, NULL
> + };
> +
> + if (!read_oneliner(&buf, rebase_path_orig_head(), 0))
> + return error(_("could not read '%s'"),
> + rebase_path_orig_head());
> + strbuf_addstr(&buf, "..HEAD");
> + argv[2] = buf.buf;
> + run_command_v_opt(argv, RUN_GIT_CMD);
> + strbuf_reset(&buf);
> + }
> + strbuf_release(&buf);
> }
It's a bit curious that the previous step avoided running a separate
process and instead did "diff-tree -p" all in C, but this one does not.
I think it is because this one is outside the loop? The original,
being a scripted Porcelain, formulates a lazy and loose command
line, but you may want to tighten it up a bit if you spawn a
process. If your user happens to have a file whose name is
$orig_head..HEAD, the command line you are creating (which is
identical to the scripted version) will barf with "ambiguous
argument".
One good thing about a complete C rewrite is that it won't have an
issue like this one because you'd be working with in-core objects.
> diff --git a/sequencer.h b/sequencer.h
> index cb21cfddee..f885b68395 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -24,6 +24,7 @@ struct replay_opts {
> int allow_empty;
> int allow_empty_message;
> int keep_redundant_commits;
> + int verbose;
>
> int mainline;
Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
Linus Torvalds writes:
> On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano wrote:
>> Johannes Schindelin writes:
>>
>>> +/*
>>> + * Note that ordering matters in this enum. Not only must it match the
>>> mapping
>>> + * below, it is also divided into several sections that matter. When
>>> adding
>>> + * new commands, make sure you add it in the right section.
>>> + */
>>
>> Good thinking. Makes me wish C were a better language, though ;-)
>
> Do this:
>
> static const char *todo_command_strings[] = {
> [TODO_PICK] = "pick",
> [TODO_REVERT] = "revert",
> [TODO_NOOP] = "noop:,
> };
>
> which makes the array be order-independent. You still need to make
> sure you fill in all the entries, of course, but it tends to avoid at
> least one gotcha, and it makes it more obvious how the two are tied
> together.
>
> Linus
Yes, I know. But I do not think the variant of C we stick to is not
new enough to have that.
Re: [PATCH v2 06/34] sequencer (rebase -i): write the 'done' file
Johannes Schindelin writes: > In the interactive rebase, commands that were successfully processed are > not simply discarded, but appended to the 'done' file instead. This is > used e.g. to display the current state to the user in the output of > `git status` or the progress. > > Signed-off-by: Johannes Schindelin > --- Looks good. I'd stop at this point for now, and start working on other things for the rest of the day. I might find time to come back to it later tonight. So far, looking mostly good.
Re: [PATCH 1/4] doc: add articles (grammar)
Junio C Hamano writes: > I was planning to merge all four from you as-is to 'next' today, > though. Should I wait? I'll definitely defer to whatever you think is best. I guess it depends on whether you are interested in Philip Oakley's suggestions. I sent those emails to inform about what I intended to do in the next round, if it got to that point, since I haven't tried to contribute to such an organised project before. So I was informing about my assumptions about how to deal with "looks good to me"-kinds of feedback. So for my part, I'm happy with iterating on this (perhaps just adding the two "acks", or also replying to the suggestions), or just merging it as-is. -- Kristoffer Haugsbakk
Re: [PATCH v2 04/34] sequencer (rebase -i): implement the 'exec' command
Johannes Schindelin writes:
> +static int do_exec(const char *command_line)
> +{
> + const char *child_argv[] = { NULL, NULL };
> + int dirty, status;
> +
> + fprintf(stderr, "Executing: %s\n", command_line);
> + child_argv[0] = command_line;
> + status = run_command_v_opt(child_argv, RUN_USING_SHELL);
> +
> + /* force re-reading of the cache */
> + if (discard_cache() < 0 || read_cache() < 0)
> + return error(_("could not read index"));
> +
> + dirty = require_clean_work_tree("rebase", NULL, 1, 1);
> +
> + if (status) {
> + warning(_("execution failed: %s\n%s"
> + "You can fix the problem, and then run\n"
> + "\n"
> + " git rebase --continue\n"
> + "\n"),
> + command_line,
> + dirty ? N_("and made changes to the index and/or the "
> + "working tree\n") : "");
> + if (status == 127)
> + /* command not found */
> + status = 1;
> + }
> + else if (dirty) {
> + warning(_("execution succeeded: %s\nbut "
> + "left changes to the index and/or the working tree\n"
> + "Commit or stash your changes, and then run\n"
> + "\n"
> + " git rebase --continue\n"
> + "\n"), command_line);
> + status = 1;
> + }
> +
> + return status;
> +}
OK, this looks like a faithful reproduction of what the scripted
version does inside do_next() helper function.
Please have "else if" on the same line as "}" that closes the
"if (...) {" in the same if/else if/else cascade.
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
On 12/13, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > ok 13 - grep tree and more pathspecs
> >
> > expecting success:
> > git init parent &&
> > test_when_finished "rm -rf parent" &&
> > echo "foobar" >"parent/fi:le" &&
> > git -C parent add "fi:le" &&
> > git -C parent commit -m "add fi:le" &&
> > ...
> > test_cmp expect actual
> >
> > ++ git init parent
> > Initialized empty Git repository in C:/git-sdk-64/usr/src/git/wip3/t/trash
> > directory.t7814-grep-recurse-submodules/parent/.git/
> > ++ test_when_finished 'rm -rf parent'
> > ++ test 0 = 0
> > ++ test_cleanup='{ rm -rf parent
> > } && (exit "$eval_ret"); eval_ret=$?; :'
> > ++ echo foobar
> > ++ git -C parent add fi:le
> > fatal: pathspec 'fi:le' did not match any files
>
> I think !MINGW prereq is missing?
Yes the !MINGW prereq is missing on this one test since the test uses a
filename with a ":" which isn't supported on windows. I have that
change made in my local grep branch but I haven't sent out a reroll of
the series yet due to the underlying race-condition that existed (due to
the way realpath was being calculated). I'll send out a reroll of the
series once the discussion on bw/realpath-wo-chdir has concluded (as
the grep series is now dependent on it).
--
Brandon Williams
[PATCH] fix pushing to //server/share/dir paths on Windows
normalize_path_copy() is not prepared to keep the double-slash of a
//server/share/dir kind of path, but treats it like a regular POSIX
style path and transforms it to /server/share/dir.
The bug manifests when 'git push //server/share/dir master' is run,
because tmp_objdir_add_as_alternate() uses the path in normalized
form when it registers the quarantine object database via
link_alt_odb_entries(). Needless to say that the directory cannot be
accessed using the wrongly normalized path.
Fix it by skipping all of the root part, not just a potential drive
prefix. offset_1st_component takes care of this, see the
implementation in compat/mingw.c::mingw_offset_1st_component().
There is a change in behavior: \\server\share is not transformed
into //server/share anymore, but all subsequent directory separators
are rewritten to '/'. This should not make a difference; Windows can
handle the mix. In the context of 'git push' this cannot be verified,
though, as there seems to be an independent bug that transforms the
double '\\' to a single '\' on the way.
Signed-off-by: Johannes Sixt
---
Another long-standing bug uncovered by the quarantine series.
Dscho, it looks like this could fix the original report at
https://github.com/git-for-windows/git/issues/979
This patch should cook well because of the change in behavior.
I would not be surprised if there is some fall-out.
The other bug I'm alluding to, I still have to investigate. I do
not think that it can be counted as fall-out.
path.c | 24 +++-
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/path.c b/path.c
index 52d889c88e..02dc70fb92 100644
--- a/path.c
+++ b/path.c
@@ -991,7 +991,7 @@ const char *remove_leading_path(const char *in, const char
*prefix)
*
* Performs the following normalizations on src, storing the result in dst:
* - Ensures that components are separated by '/' (Windows only)
- * - Squashes sequences of '/'.
+ * - Squashes sequences of '/' except "//server/share" on Windows
* - Removes "." components.
* - Removes ".." components, and the components the precede them.
* Returns failure (non-zero) if a ".." component appears as first path
@@ -1014,17 +1014,23 @@ const char *remove_leading_path(const char *in, const
char *prefix)
int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
{
char *dst0;
- int i;
-
- for (i = has_dos_drive_prefix(src); i > 0; i--)
- *dst++ = *src++;
- dst0 = dst;
+ int offset;
- if (is_dir_sep(*src)) {
+ /*
+* Handle initial part of absolute path: "/", "C:/", "\\server\share/".
+*/
+ offset = offset_1st_component(src);
+ if (offset) {
+ /* Convert the trailing separator to '/' on Windows. */
+ memcpy(dst, src, offset - 1);
+ dst += offset - 1;
*dst++ = '/';
- while (is_dir_sep(*src))
- src++;
+ src += offset;
}
+ dst0 = dst;
+
+ while (is_dir_sep(*src))
+ src++;
for (;;) {
char c = *src;
--
2.11.0.79.g263f27a
Re: [PATCH v2 03/34] sequencer (rebase -i): implement the 'edit' command
Johannes Schindelin writes:
> @@ -43,6 +44,20 @@ static GIT_PATH_FUNC(rebase_path_todo,
> "rebase-merge/git-rebase-todo")
> */
> static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
> /*
It is minor, but please have a blank line to separate the logical
blocks. If you have "comment for thing A" before "thing A", then
having a blank after that before "comment for thing B" and "thing B"
that follow would help. Otherwise "thing A" immediately followed by
"comment for thing B" are (mis)read together, leading to nonsense.
> + * When an "edit" rebase command is being processed, the SHA1 of the
> + * commit to be edited is recorded in this file. When "git rebase
> + * --continue" is executed, if there are any staged changes then they
> + * will be amended to the HEAD commit, but only provided the HEAD
> + * commit is still the commit to be edited. When any other rebase
> + * command is processed, this file is deleted.
> + */
> +static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
> +/*
> + * When we stop at a given patch via the "edit" command, this file contains
> + * the long commit name of the corresponding patch.
If you abbreviate an object name to 38-hex that is still long but
that is not full; if you meant full 40-hex, better spell it out as
"full"---that conveys useful information to programmers (e.g. they
can just use get_sha1_hex()).
But I think you are writing short_commit_name() to it? So perhaps
"an abbreviated commit object name"?
> @@ -1301,9 +1318,87 @@ static int save_opts(struct replay_opts *opts)
> return res;
> }
>
> +static int make_patch(struct commit *commit, struct replay_opts *opts)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct rev_info log_tree_opt;
> + const char *commit_buffer = get_commit_buffer(commit, NULL), *subject,
> *p;
> + int res = 0;
> +
> + p = short_commit_name(commit);
> + if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
> + return -1;
> +
> + strbuf_addf(&buf, "%s/patch", get_dir(opts));
> + memset(&log_tree_opt, 0, sizeof(log_tree_opt));
> + init_revisions(&log_tree_opt, NULL);
> + log_tree_opt.abbrev = 0;
> + log_tree_opt.diff = 1;
> + log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
> + log_tree_opt.disable_stdin = 1;
> + log_tree_opt.no_commit_id = 1;
> + log_tree_opt.diffopt.file = fopen(buf.buf, "w");
> + log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
> + if (!log_tree_opt.diffopt.file)
> + res |= error_errno(_("could not open '%s'"), buf.buf);
> + else {
> + res |= log_tree_commit(&log_tree_opt, commit);
> + fclose(log_tree_opt.diffopt.file);
> + }
> + strbuf_reset(&buf);
> + strbuf_addf(&buf, "%s/message", get_dir(opts));
> + if (!file_exists(buf.buf)) {
> + find_commit_subject(commit_buffer, &subject);
> + res |= write_message(subject, strlen(subject), buf.buf, 1);
> + unuse_commit_buffer(commit, commit_buffer);
> + }
> + strbuf_release(&buf);
> +
> + return res;
> +}
OK. This seems to match what scripted make_patch does in a handful
of lines. We probably should have given you a helper to reduce
boilerplate that sets up log_tree_opt so that this function does not
have to be this long, but that is a separate topic.
Does it matter output_format is set to FORMAT_PATCH here, though?
With --no-commit-id set, I suspect there is no log message or
authorship information given to the output.
As you are only interested in seeing the patch/diff in this file and
the log is stored in a separate "message" file, as long as "patch"
file gets the patch correctly, it is not a problem, but it just
looked strange to see FORMAT_PATCH there.
Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
>> +/*
>> + * Note that ordering matters in this enum. Not only must it match the
>> mapping
>> + * below, it is also divided into several sections that matter. When adding
>> + * new commands, make sure you add it in the right section.
>> + */
>
> Good thinking. Makes me wish C were a better language, though ;-)
Do this:
static const char *todo_command_strings[] = {
[TODO_PICK] = "pick",
[TODO_REVERT] = "revert",
[TODO_NOOP] = "noop:,
};
which makes the array be order-independent. You still need to make
sure you fill in all the entries, of course, but it tends to avoid at
least one gotcha, and it makes it more obvious how the two are tied
together.
Linus
[PATCHv2 3/5] submodule: add flags to ok_to_remove_submodule
In different contexts the question whether deleting a submodule is ok
to remove may be answered differently.
In 293ab15eea (submodule: teach rm to remove submodules unless they
contain a git directory, 2012-09-26) a case was made that we can safely
ignore ignored untracked files for removal as we explicitely ask for the
removal of the submodule.
In a later patch we want to remove submodules even when the user doesn't
explicitly ask for it (e.g. checking out a tree-ish in which the submodule
doesn't exist). In that case we want to be more careful when it comes
to deletion of untracked files. As of this patch it is unclear how this
will be implemented exactly, so we'll offer flags in which the caller
can specify how the different untracked files ought to be handled.
Signed-off-by: Stefan Beller
---
builtin/rm.c | 3 ++-
submodule.c | 23 +++
submodule.h | 5 -
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..fdd7183f61 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -187,7 +187,8 @@ static int check_local_mod(struct object_id *head, int
index_only)
*/
if (ce_match_stat(ce, &st, 0) ||
(S_ISGITLINK(ce->ce_mode) &&
-!ok_to_remove_submodule(ce->name)))
+!ok_to_remove_submodule(ce->name,
+ SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
local_changes = 1;
/*
diff --git a/submodule.c b/submodule.c
index 9f0b544ebe..2d13744b06 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1019,7 +1019,7 @@ int submodule_uses_gitfile(const char *path)
return 1;
}
-int ok_to_remove_submodule(const char *path)
+int ok_to_remove_submodule(const char *path, unsigned flags)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1032,15 +1032,27 @@ int ok_to_remove_submodule(const char *path)
if (!submodule_uses_gitfile(path))
return 0;
- argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+ argv_array_pushl(&cp.args, "status", "--porcelain",
"--ignore-submodules=none", NULL);
+
+ if (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED)
+ argv_array_push(&cp.args, "-uno");
+ else
+ argv_array_push(&cp.args, "-uall");
+
+ if (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED))
+ argv_array_push(&cp.args, "--ignored");
+
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die(_("could not run 'git status --porcelain -u
--ignore-submodules=none' in submodule %s"), path);
+ die(_("could not run 'git status --porcelain
--ignore-submodules=none %s %s' in submodule '%s'"),
+ (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED) ? "-uno" :
"-uall",
+ (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED))
? "--ignored" : "",
+ path);
len = strbuf_read(&buf, cp.out, 1024);
if (len > 2)
@@ -1048,7 +1060,10 @@ int ok_to_remove_submodule(const char *path)
close(cp.out);
if (finish_command(&cp))
- die(_("'git status --porcelain -u --ignore-submodules=none'
failed in submodule %s"), path);
+ die(_("'git status --porcelain --ignore-submodules=none %s %s'
failed in submodule '%s'"),
+ (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED) ? "-uno" :
"-uall",
+ (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED))
? "--ignored" : "",
+ path);
strbuf_release(&buf);
return ok_to_remove;
diff --git a/submodule.h b/submodule.h
index 61fb610749..3ed3aa479a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -59,7 +59,10 @@ extern int fetch_populated_submodules(const struct
argv_array *options,
int quiet, int max_parallel_jobs);
extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
extern int submodule_uses_gitfile(const char *path);
-extern int ok_to_remove_submodule(const char *path);
+
+#define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<0)
+#define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<1)
+extern int ok_to_remove_submodule(const char *path, unsigned flags);
extern int merge_submodule(unsigned char result[20], const char *path,
const unsigned char base[20],
const unsigned char a[20],
--
2.11.0.rc2.35.g26e18c9
[PATCHv2 5/5] rm: add absorb a submodules git dir before deletion
When deleting a submodule, we need to keep the actual git directory around,
such that we do not lose local changes in there and at a later checkout
of the submodule we don't need to clone it again.
Implement `depopulate_submodule`, that migrates the git directory before
deletion of a submodule and afterwards the equivalent of "rm -rf", which
is already found in entry.c, so expose that and for clarity add a suffix
"_or_die" to it.
Signed-off-by: Stefan Beller
---
builtin/rm.c | 30 --
cache.h | 2 ++
entry.c | 5 +
submodule.c | 31 +++
submodule.h | 6 ++
t/t3600-rm.sh | 39 +++
6 files changed, 67 insertions(+), 46 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index fdd7183f61..8c9c535a88 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -392,28 +392,14 @@ int cmd_rm(int argc, const char **argv, const char
*prefix)
for (i = 0; i < list.nr; i++) {
const char *path = list.entry[i].name;
if (list.entry[i].is_submodule) {
- if (is_empty_dir(path)) {
- if (!rmdir(path)) {
- removed = 1;
- if
(!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- continue;
- }
- } else {
- strbuf_reset(&buf);
- strbuf_addstr(&buf, path);
- if (!remove_dir_recursively(&buf, 0)) {
- removed = 1;
- if
(!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- strbuf_release(&buf);
- continue;
- } else if (!file_exists(path))
- /* Submodule was removed by
user */
- if
(!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- /* Fallthrough and let remove_path()
fail. */
- }
+ if (is_empty_dir(path) && rmdir(path))
+ die_errno("git rm: '%s'", path);
+ if (file_exists(path))
+ depopulate_submodule(path);
+ removed = 1;
+ if (!remove_path_from_gitmodules(path))
+ gitmodules_modified = 1;
+ continue;
}
if (!remove_path(path)) {
removed = 1;
diff --git a/cache.h b/cache.h
index a50a61a197..b645ca2f9a 100644
--- a/cache.h
+++ b/cache.h
@@ -2018,4 +2018,6 @@ void sleep_millisec(int millisec);
*/
void safe_create_dir(const char *dir, int share);
+extern void remove_directory_or_die(struct strbuf *path);
+
#endif /* CACHE_H */
diff --git a/entry.c b/entry.c
index c6eea240b6..02c4ac9f22 100644
--- a/entry.c
+++ b/entry.c
@@ -73,6 +73,11 @@ static void remove_subtree(struct strbuf *path)
die_errno("cannot rmdir '%s'", path->buf);
}
+void remove_directory_or_die(struct strbuf *path)
+{
+ remove_subtree(path);
+}
+
static int create_file(const char *path, unsigned int mode)
{
mode = (mode & 0100) ? 0777 : 0666;
diff --git a/submodule.c b/submodule.c
index e42efa2337..3770ecb7b9 100644
--- a/submodule.c
+++ b/submodule.c
@@ -308,6 +308,37 @@ static void print_submodule_summary(struct rev_info *rev,
FILE *f,
strbuf_release(&sb);
}
+void depopulate_submodule(const char *path)
+{
+ struct strbuf pathbuf = STRBUF_INIT;
+ char *dot_git = xstrfmt("%s/.git", path);
+
+ /* Is it populated? */
+ if (!resolve_gitdir(dot_git))
+ goto out;
+
+ /* Does it have a .git directory? */
+ if (!submodule_uses_gitfile(path)) {
+ absorb_git_dir_into_superproject("", path,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
+
+ if (!submodule_uses_gitfile(path)) {
+ /*
+* We should be using a gitfile by now. Let's double
+* check as losing the git dir would be fatal.
+*/
+ die("BUG: could not absorb git directory for '%s'",
path);
+
[PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir
It is a major reason to say no, when deciding if a submodule can be
deleted, if the git directory of the submodule being contained in the
submodule's working directory.
Migrate the git directory into the superproject instead of failing,
and proceed with the other checks.
Signed-off-by: Stefan Beller
---
submodule.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2d13744b06..e42efa2337 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1026,11 +1026,22 @@ int ok_to_remove_submodule(const char *path, unsigned
flags)
struct strbuf buf = STRBUF_INIT;
int ok_to_remove = 1;
+ /* Is it there? */
if (!file_exists(path) || is_empty_dir(path))
return 1;
- if (!submodule_uses_gitfile(path))
- return 0;
+ /* Does it have a .git directory? */
+ if (!submodule_uses_gitfile(path)) {
+ absorb_git_dir_into_superproject("", path,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
+
+ /*
+* We should be using a gitfile by now. Let's double
+* check as losing the git dir would be fatal.
+*/
+ if (!submodule_uses_gitfile(path))
+ return 0;
+ }
argv_array_pushl(&cp.args, "status", "--porcelain",
"--ignore-submodules=none", NULL);
--
2.11.0.rc2.35.g26e18c9
[PATCHv2 2/5] submodule: modernize ok_to_remove_submodule to use argv_array
Instead of constructing the NULL terminated array ourselves, we
should make use of the argv_array infrastructure.
While at it, adapt the error messages to reflect the actual invocation.
Signed-off-by: Stefan Beller
---
submodule.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/submodule.c b/submodule.c
index 45ccfb7ab4..9f0b544ebe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1023,13 +1023,6 @@ int ok_to_remove_submodule(const char *path)
{
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {
- "status",
- "--porcelain",
- "-u",
- "--ignore-submodules=none",
- NULL,
- };
struct strbuf buf = STRBUF_INIT;
int ok_to_remove = 1;
@@ -1039,14 +1032,15 @@ int ok_to_remove_submodule(const char *path)
if (!submodule_uses_gitfile(path))
return 0;
- cp.argv = argv;
+ argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+ "--ignore-submodules=none", NULL);
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command(&cp))
- die("Could not run 'git status --porcelain -uall
--ignore-submodules=none' in submodule %s", path);
+ die(_("could not run 'git status --porcelain -u
--ignore-submodules=none' in submodule %s"), path);
len = strbuf_read(&buf, cp.out, 1024);
if (len > 2)
@@ -1054,7 +1048,7 @@ int ok_to_remove_submodule(const char *path)
close(cp.out);
if (finish_command(&cp))
- die("'git status --porcelain -uall --ignore-submodules=none'
failed in submodule %s", path);
+ die(_("'git status --porcelain -u --ignore-submodules=none'
failed in submodule %s"), path);
strbuf_release(&buf);
return ok_to_remove;
--
2.11.0.rc2.35.g26e18c9
[PATCHv2 0/5] git-rm absorbs submodule git directory before deletion
v2: * new base where to apply the patch: sb/submodule-embed-gitdir merged with sb/t3600-cleanup. I got merge conflicts and resolved them this way: #@@@ -709,9 -687,10 +687,9 @@@ test_expect_success 'checking out a com # git commit -m "submodule removal" submod && # git checkout HEAD^ && # git submodule update && #- git checkout -q HEAD^ 2>actual && #+ git checkout -q HEAD^ && # git checkout -q master 2>actual && # -echo "warning: unable to rmdir submod: Directory not empty" >expected && # -test_i18ncmp expected actual && # +test_i18ngrep "^warning: unable to rmdir submod:" actual && # git status -s submod >actual && # echo "?? submod/" >expected && # test_cmp expected actual && # * improved commit message in "ok_to_remove_submodule: absorb the submodule git dir" (David Turner offered me some advice on how to write better English off list) * simplified code in last patch: -> dropped wrong comment for fallthrough -> moved redundant code out of both bodies of an if-clause. * Fixed last patchs commit message to have "or_die" instead of or_dir. v1: The "checkout --recurse-submodules" series got too large to comfortably send it out for review, so I had to break it up into smaller series'; this is the first subseries, but it makes sense on its own. This series teaches git-rm to absorb the git directory of a submodule instead of failing and complaining about the git directory preventing deletion. It applies on origin/sb/submodule-embed-gitdir. Any feedback welcome! Thanks, Stefan Stefan Beller (5): submodule.h: add extern keyword to functions submodule: modernize ok_to_remove_submodule to use argv_array submodule: add flags to ok_to_remove_submodule ok_to_remove_submodule: absorb the submodule git dir rm: add absorb a submodules git dir before deletion builtin/rm.c | 33 - cache.h | 2 ++ entry.c | 5 submodule.c | 77 +-- submodule.h | 64 ++--- t/t3600-rm.sh | 39 -- 6 files changed, 135 insertions(+), 85 deletions(-) -- 2.11.0.rc2.35.g26e18c9
[PATCHv2 1/5] submodule.h: add extern keyword to functions
As the upcoming series will add a lot of functions to the submodule
header, let's first make the header consistent to the rest of the project
by adding the extern keyword to functions.
As per the CodingGuidelines we try to stay below 80 characters per line,
so adapt all those functions to stay below 80 characters that are already
using more than one line. Those function using just one line are better
kept in one line than breaking them up into multiple lines just for the
goal of staying below the character limit as it makes grepping
for functions easier if they are one liners.
Signed-off-by: Stefan Beller
---
submodule.h | 55 ++-
1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/submodule.h b/submodule.h
index 6229054b99..61fb610749 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,50 +29,55 @@ struct submodule_update_strategy {
};
#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
-int is_staging_gitmodules_ok(void);
-int update_path_in_gitmodules(const char *oldpath, const char *newpath);
-int remove_path_from_gitmodules(const char *path);
-void stage_updated_gitmodules(void);
-void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
+extern int is_staging_gitmodules_ok(void);
+extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
+extern int remove_path_from_gitmodules(const char *path);
+extern void stage_updated_gitmodules(void);
+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
const char *path);
-int submodule_config(const char *var, const char *value, void *cb);
-void gitmodules_config(void);
-int parse_submodule_update_strategy(const char *value,
+extern int submodule_config(const char *var, const char *value, void *cb);
+extern void gitmodules_config(void);
+extern int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
-const char *submodule_strategy_to_string(const struct
submodule_update_strategy *s);
-void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
-void show_submodule_summary(FILE *f, const char *path,
+extern const char *submodule_strategy_to_string(const struct
submodule_update_strategy *s);
+extern void handle_ignore_submodules_arg(struct diff_options *, const char *);
+extern void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
-void show_submodule_inline_diff(FILE *f, const char *path,
+extern void show_submodule_inline_diff(FILE *f, const char *path,
const char *line_prefix,
struct object_id *one, struct object_id *two,
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset,
const struct diff_options *opt);
-void set_config_fetch_recurse_submodules(int value);
-void check_for_new_submodule_commits(unsigned char new_sha1[20]);
-int fetch_populated_submodules(const struct argv_array *options,
+extern void set_config_fetch_recurse_submodules(int value);
+extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
+extern int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
int quiet, int max_parallel_jobs);
-unsigned is_submodule_modified(const char *path, int ignore_untracked);
-int submodule_uses_gitfile(const char *path);
-int ok_to_remove_submodule(const char *path);
-int merge_submodule(unsigned char result[20], const char *path, const unsigned
char base[20],
- const unsigned char a[20], const unsigned char b[20], int
search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char
*remotes_name,
- struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char
*remotes_name);
-int parallel_submodules(void);
+extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
+extern int submodule_uses_gitfile(const char *path);
+extern int ok_to_remove_submodule(const char *path);
+extern int merge_submodule(unsigned char result[20], const char *path,
+ const unsigned char base[20],
+ const unsigned char a[20],
+ const unsigned char b[20], int search);
+extern int find_unpushed_submodules(unsigned char new_sha1[20],
+ const char *remotes_name,
+ struct string_list *needs_pushing);
+extern int push_unpushed_submodules(unsigned char new_sha1[20],
+ const char *remotes_na
Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
Johannes Schindelin writes:
> static inline int is_rebase_i(const struct replay_opts *opts)
> {
> - return 0;
> + return opts->action == REPLAY_INTERACTIVE_REBASE;
> }
>
> static const char *get_dir(const struct replay_opts *opts)
> {
> + if (is_rebase_i(opts))
> + return rebase_path();
> return git_path_seq_dir();
> }
>
> static const char *get_todo_path(const struct replay_opts *opts)
> {
> + if (is_rebase_i(opts))
> + return rebase_path_todo();
> return git_path_todo_file();
> }
>
> @@ -168,7 +179,15 @@ int sequencer_remove_state(struct replay_opts *opts)
>
> static const char *action_name(const struct replay_opts *opts)
> {
> - return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick");
> + switch (opts->action) {
> + case REPLAY_REVERT:
> + return N_("revert");
> + case REPLAY_PICK:
> + return N_("cherry-pick");
> + case REPLAY_INTERACTIVE_REBASE:
> + return N_("rebase -i");
> + }
> + die(_("Unknown action: %d"), opts->action);
> }
This case statement which looks perfectly sensible---it says that
there are three equal modes the subsystem operates in.
This is just a mental note and not a suggestion to change anything
immediately, but it makes me wonder if git_dir/get_todo_path would
also want to do so, moving towards retiring is_rebase_i() which is
"everything else vs one oddball which is rebase-i" mindset.
> @@ -395,7 +414,10 @@ static int do_recursive_merge(struct commit *base,
> struct commit *next,
>
> if (active_cache_changed &&
> write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> - /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> + /*
> + * TRANSLATORS: %s will be "revert", "cherry-pick" or
> + * "rebase -i".
> + */
IIRC, the "TRANSLATORS:" comment has to deviate from our coding
style due to tool limitation and has to be done like this:
> + /* TRANSLATORS: %s will be "revert", "cherry-pick" or
> + * "rebase -i".
> + */
> @@ -1204,6 +1226,9 @@ static int save_todo(struct todo_list *todo_list,
> struct replay_opts *opts)
> const char *todo_path = get_todo_path(opts);
> int next = todo_list->current, offset, fd;
>
> + if (is_rebase_i(opts))
> + next++;
> +
This is because...? Everybody else counts 0-based while rebase-i
counts from 1 or something?
> fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
> if (fd < 0)
> return error_errno(_("could not lock '%s'"), todo_path);
Everything else in the patch is understandable. This bit isn't
without explanation, at least to me.
Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
Johannes Schindelin writes:
> +/*
> + * Note that ordering matters in this enum. Not only must it match the
> mapping
> + * below, it is also divided into several sections that matter. When adding
> + * new commands, make sure you add it in the right section.
> + */
Good thinking. Makes me wish C were a better language, though ;-)
> enum todo_command {
> + /* commands that handle commits */
> TODO_PICK = 0,
> - TODO_REVERT
> + TODO_REVERT,
> + /* commands that do nothing but are counted for reporting progress */
> + TODO_NOOP
> };
>
> static const char *todo_command_strings[] = {
> "pick",
> - "revert"
> + "revert",
> + "noop"
> };
> @@ -1292,7 +1316,12 @@ static int pick_commits(struct todo_list *todo_list,
> struct replay_opts *opts)
> struct todo_item *item = todo_list->items + todo_list->current;
> if (save_todo(todo_list, opts))
> return -1;
> - res = do_pick_commit(item->command, item->commit, opts);
> + if (item->command <= TODO_REVERT)
> + res = do_pick_commit(item->command, item->commit,
> + opts);
> + else if (item->command != TODO_NOOP)
> + return error(_("unknown command %d"), item->command);
I wonder if making this a switch() statement is easier to read in
the longer run. The only thing at this point we are gaining by "not
only mapping and enum must match, the orders matter" is so that this
codepath can do the same thing for PICK and REVERT, but these two
would become more and more minority as we learn more words.
> todo_list->current++;
> if (res)
> return res;
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 12:09 PM, Stefan Beller wrote: > > So I will reroll it with "absorb" fixing some nits pointed out by David? I got confused there, Davids nits are for this series, the absorb series itself doesn't seem to have nits. So I'll just reroll this series on top of the currently sb/submodule-embed-gitdir (which you originally noted to be better renamed to submodule-absorb-gitdir) merged with t3600-cleanup. Thanks, Stefan
Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
Jeff King writes:
> On Tue, Dec 13, 2016 at 10:10:04AM -0800, Junio C Hamano wrote:
>
>> > - git clone --bare . xxx:yyy.git &&
>> > + git clone --bare . xxx${path_sep}yyy.git &&
>>
>> Don't you want to dq the whole thing to prevent the shell from
>> splitting this into two commands at ';'? The other one below is OK.
>
> After expansion, I don't think the shell will do any further processing
> except for whitespace splitting. E.g.:
Ah, my mistake. Staring at too many `eval`s does it to me.
Thanks.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 11:47 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> The desired standard for submodules is to have the git dir inside the
>> superprojects git dir (since 501770e, Aug 2011, Move git-dir for
>> submodules), which is why I think an "embedded submodule git dir"
>> is inside the superproject already.
>
> Think how you start a new submodule. What are the steps you take
> before you say "git submodule add"? And where does .git for the
> submodule live at that point?
Well there is no way to directly start a submodule
(e.g. "git submodule create"), such that there has to be one repo
that actually has the git dir inside the working tree.
If the submodule exists already somewhere, there are 2 ways to do it
("git submodule add " or "git clone && git submodule add")
which lead to different outcomes, where the .git resides.
> With the current system, you as the submodule originator need to do
> something different to make your working tree of the superproject
> match what the others who clone from your public repository.
>
> And comparing the two layout, the one originally held by the
> submodule originator has .git embedded in the working tree, no?
When starting a new submodule repo, yes, the git dir is inside
the working tree.
> All of the above is coming from "submodule" centric mindset. It
> just is not centric to those who follow what others originated.
ok.
> Another reason why I personally see a .git in each submodule working
> tree is "embedded" has nothing to do with Git. It is an analogy I
> feel (perhaps it is just me) with the word "embedded reporters in
> warzone". These people are spread around, assigned to units to
> report from places closer to the front line and being closer to the
> leaf of the hierarchy, as opposed to be assigned to a more central
> place like HQ to do their reporting.
I talked to people in the office and got a heavy rejection on the
the work "embedded" here for another reason:
"Does it put the submodule on an embedded device?
What does embedded even mean?
The end user is super confused"
So I think we should not use embed or un-embed one way or the other.
Instead we need to have another name.
I think absorb is ok-ish, as "git submodule absorb" hints that the
superproject absorbs something from the submodule.
So I will reroll it with "absorb" fixing some nits pointed out by David?
Re: [RFC/PATCH v3 00/16] Add initial experimental external ODB support
Christian Couder writes: > In general I think that having a lot of refs is really a big problem > right now in Git as many big organizations using Git are facing this > problem in one form or another. > So I think that support for a big number of refs is a separate and > important problem that should and hopefully will be solved. But you do not have to make it worse. Is "refs" a good match for the problem you are solving? Or is it merely an expedient thing to use? I think it is the latter, judging by your mentioning RefTree. Whatever mechanism we choose, that will be carved into stone in users' repositories and you'd end up having to support it, and devise the migration path out of it if the initial selection is too problematic. That is why people (not just me) pointed out upfront that using refs for this purose would not scale.
Re: git add -p with unmerged files
Stephan Beyer writes: > While we're on the topic that "git add -p" should behave like the > "normal" "git add" (not "git add -u"): what about unmerged changes? > > > When I have merge conflicts, I almost always use my aliases > "edit-unmerged" and "add-unmerged": > > $ git config --global --list | grep unmerged > alias.list-unmerged=diff --name-only --diff-filter=U > alias.edit-unmerged=!vim `git list-unmerged` > alias.add-unmerged=!git add `git list-unmerged` > alias.reset-unmerged=!uf=`git list-unmerged`; git reset HEAD $uf; git > checkout -- $uf > > The "add-unmerged" alias is always a little scary because I'd rather > like to check the changes with the "git add -p" workflow I am used to. > > Opinions? For this, you would NEVER want to use "add -p" to pick and choose. By definition, while you are in conflicted merge, the path that had conflicts before you started the merge-y operation (be it "pull", "am -3", or "cherry-pick") did not have any change since HEAD, and "pick this hunk, drop that hunk" cannot be correct for the conflict resolution. "git diff" while conflicted will highlight what conflicted by showing the three-way diff (similar to "diff --cc" on a merge result) and after conflict is resolved you can view "diff HEAD" on the path to see what the merge brought in.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
Stefan Beller writes: > I guess the latter is the case, so embedding is actually inside > the working tree and un-embedding is the relocation to the > superproject. Another reason why I personally see a .git in each submodule working tree is "embedded" has nothing to do with Git. It is an analogy I feel (perhaps it is just me) with the word "embedded reporters in warzone". These people are spread around, assigned to units to report from places closer to the front line and being closer to the leaf of the hierarchy, as opposed to be assigned to a more central place like HQ to do their reporting.
Re: git add -p with unmerged files (was: git add -p with new file)
On Tue, Dec 13, 2016 at 08:21:59PM +0100, Stephan Beyer wrote:
> While we're on the topic that "git add -p" should behave like the
> "normal" "git add" (not "git add -u"): what about unmerged changes?
I agree that's a related part of the workflow, though the implementation
is a bit harder.
> When I have merge conflicts, I almost always use my aliases
> "edit-unmerged" and "add-unmerged":
>
> $ git config --global --list | grep unmerged
> alias.list-unmerged=diff --name-only --diff-filter=U
> alias.edit-unmerged=!vim `git list-unmerged`
You might like contrib/git-jump for that, which makes it easier to go to
the specific spots within files.
When "git jump merge" produces no hits, I know I've dealt with all of
the conflicts (textual ones, anyway). I do often want to run "git add
-p" then to review the changes before staging.
I think what is most helpful there is probably "git diff HEAD" to see
what the merge is bringing in (or the cherry-pick, or "am", or
whatever). If I wanted "add -p" to do anything, I think it would be to
act as if stage 2 ("ours", which should be the same as what is in HEAD)
was already in the index. I.e., show the diff against that, apply any
hunks we select, and store the whole thing as stage 0, losing the
unmerged bit.
When you select all hunks, this is equivalent to "git add unmerge-file".
If you choose only a subset of hunks, it leaves the unselected ones for
you to examine later via "git diff". And if you choose none, it should
probably leave unmerged.
That's just a scheme I thought up, though. I've never actually tried it
in practice.
-Peff
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
Stefan Beller writes: > The desired standard for submodules is to have the git dir inside the > superprojects git dir (since 501770e, Aug 2011, Move git-dir for > submodules), which is why I think an "embedded submodule git dir" > is inside the superproject already. Think how you start a new submodule. What are the steps you take before you say "git submodule add"? And where does .git for the submodule live at that point? With the current system, you as the submodule originator need to do something different to make your working tree of the superproject match what the others who clone from your public repository. And comparing the two layout, the one originally held by the submodule originator has .git embedded in the working tree, no? All of the above is coming from "submodule" centric mindset. It just is not centric to those who follow what others originated.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 11:11 AM, Junio C Hamano wrote: > Stefan Beller writes: > >>> I do not think there is no dispute about what embedding means. >> >> double negative: You think we have a slight dispute here. > > Sorry, I do not think there is any dispute on that. > >>> A >>> submodule whose .git is inside its working tree has its repository >>> embedded. >>> >>> What we had trouble settling on was what to call the operation to >>> undo the embedding, unentangling its repository out of the working >>> tree. I'd still vote for unembed if you want a name to be nominated. >> >> So I can redo the series with two commands "git submodule [un]embed". >> >> For me "unembed" == "absorb", such that we could also go with >> absorb into superproject <-> embed into worktree > > With us agreeing that "embed" is about something is _IN_ submodule > working tree, unembed would naturally be something becomes OUTSIDE > the same thing (i.e. "submodule working tree"). However, if you > introduce "absorb", we suddenly need to talk about a different > thing, i.e. "superproject's .git/modules", that is doing the > absorption. That is why I suggest "unembed" over "absorb". ok, I will take unembed then. We could also go with more command line options such as "embed --reverse" or such, but that is not as nice I'd think.
Re: [PATCH v2 00/34] Teach the sequencer to act as rebase -i's backend
Johannes Schindelin writes: > This marks the count down to '3': two more patch series after this > (really tiny ones) and we have a faster rebase -i. Nice. > Apart from mostly cosmetic patches (and the occasional odd bug that I > fixed promptly), I used these patches since mid May to perform all of my > interactive rebases. In mid June, I had the idea to teach rebase -i to > run *both* scripted rebase and rebase--helper and to cross-validate the > results. This slowed down all my interactive rebases since, but helped > me catch three rather obscure bugs (e.g. that git commit --fixup unfolds > long onelines and rebase -i still finds the correct original commit). > ... > Please note that the interdiff vs v1 is only of limited use: too many > things changed in the meantime, in particular the prepare-sequencer > branch that went through a couple of iterations before it found its way > into git.git's master branch. So please take the interdiff with a > mountain range of salt. > ... > Changes since v1: > ... > - removed the beautiful ordinal logic (to print out "1st", "2nd", "3rd" > etc) and made things consistent with the current `rebase -i`. It was removed because it was too Anglo-centric and unusable in i18n context, no? Judging from the list above, interdiff are pretty much all cosmetic and that is why you say it is only of limited use, I guess. ... goes and reads the remainder and finds that these were ... all minor changes, mostly cosmetic, with a helper function ... refactored out or two and things of that nature. It is actually a good thing. We do not want to see it deviate too drastically from what you have been testing for some months. Thanks.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 11:13 AM, Stefan Beller wrote: > On Tue, Dec 13, 2016 at 11:11 AM, Junio C Hamano wrote: >> Stefan Beller writes: >> I do not think there is no dispute about what embedding means. >>> >>> double negative: You think we have a slight dispute here. >> >> Sorry, I do not think there is any dispute on that. >> A submodule whose .git is inside its working tree has its repository embedded. What we had trouble settling on was what to call the operation to undo the embedding, unentangling its repository out of the working tree. I'd still vote for unembed if you want a name to be nominated. >>> >>> So I can redo the series with two commands "git submodule [un]embed". >>> >>> For me "unembed" == "absorb", such that we could also go with >>> absorb into superproject <-> embed into worktree >> >> With us agreeing that "embed" is about something is _IN_ submodule >> working tree, unembed would naturally be something becomes OUTSIDE >> the same thing (i.e. "submodule working tree"). I do not agree, yet. So I thought about this for a while. The standard in Git is to have the .git directory inside the working tree, which is why you are convinced that embedded means the .git is in the working tree, because you approach this discussion as the Git maintainer, spending only little time on submodule related stuff. The desired standard for submodules is to have the git dir inside the superprojects git dir (since 501770e, Aug 2011, Move git-dir for submodules), which is why I think an "embedded submodule git dir" is inside the superproject already. I think both views are legit, and we would want to choose the one that users find most intuitive (and I think there will be users that find either viewpoint intuitive). So when you have typed "git submodule ", I wonder if a user would assume a submodule-centric mindset of how submodules ought to work or if they still look at a submodule as its own git repo that just happens to be embedded into the superproject. I guess the latter is the case, so embedding is actually inside the working tree and un-embedding is the relocation to the superproject.
git add -p with unmerged files (was: git add -p with new file)
Hi, While we're on the topic that "git add -p" should behave like the "normal" "git add" (not "git add -u"): what about unmerged changes? When I have merge conflicts, I almost always use my aliases "edit-unmerged" and "add-unmerged": $ git config --global --list | grep unmerged alias.list-unmerged=diff --name-only --diff-filter=U alias.edit-unmerged=!vim `git list-unmerged` alias.add-unmerged=!git add `git list-unmerged` alias.reset-unmerged=!uf=`git list-unmerged`; git reset HEAD $uf; git checkout -- $uf The "add-unmerged" alias is always a little scary because I'd rather like to check the changes with the "git add -p" workflow I am used to. Opinions? Best Stephan
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
Johannes Schindelin writes:
> ok 13 - grep tree and more pathspecs
>
> expecting success:
> git init parent &&
> test_when_finished "rm -rf parent" &&
> echo "foobar" >"parent/fi:le" &&
> git -C parent add "fi:le" &&
> git -C parent commit -m "add fi:le" &&
> ...
> test_cmp expect actual
>
> ++ git init parent
> Initialized empty Git repository in C:/git-sdk-64/usr/src/git/wip3/t/trash
> directory.t7814-grep-recurse-submodules/parent/.git/
> ++ test_when_finished 'rm -rf parent'
> ++ test 0 = 0
> ++ test_cleanup='{ rm -rf parent
> } && (exit "$eval_ret"); eval_ret=$?; :'
> ++ echo foobar
> ++ git -C parent add fi:le
> fatal: pathspec 'fi:le' did not match any files
I think !MINGW prereq is missing?
Re: [PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too
Johannes Sixt writes:
> To perform the test case on Windows in a way that corresponds to the
> POSIX version, inject the semicolon in a directory name.
>
> Typically, an absolute POSIX style path, such as the one in $PWD, is
> translated into a Windows style path by bash when it invokes git.exe.
> However, the presence of the semicolon suppresses this translation;
> but the untranslated POSIX style path is useless for git.exe.
> Therefore, instead of $PWD pass the Windows style path that $(pwd)
> produces.
>
> Signed-off-by: Johannes Sixt
> ---
> Am 12.12.2016 um 20:53 schrieb Jeff King:
>> Johannes, please let me know if I am wrong about skipping the test on
>> !MINGW. The appropriate check there would be ";" anyway, but I am not
>> sure _that_ is allowed in paths, either.
>
> Here is a version for Windows. I'd prefer this patch on top instead
> of squashing it into yours to keep the $PWD vs. $(pwd) explanation.
>
> The result is the same as yours in all practical matters; but this
> version I have already tested.
Will queue (I would wait for peff@ to say "OK", but I suspect he
would be OK in this case).
Thanks.
> t/t5547-push-quarantine.sh | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
> index 6275ec807b..af9fcd833a 100755
> --- a/t/t5547-push-quarantine.sh
> +++ b/t/t5547-push-quarantine.sh
> @@ -33,8 +33,7 @@ test_expect_success 'rejected objects are removed' '
> test_cmp expect actual
> '
>
> -# MINGW does not allow colons in pathnames in the first place
> -test_expect_success !MINGW 'push to repo path with colon' '
> +test_expect_success 'push to repo path with path separator (colon)' '
> # The interesting failure case here is when the
> # receiving end cannot access its original object directory,
> # so make it likely for us to generate a delta by having
> @@ -43,13 +42,20 @@ test_expect_success !MINGW 'push to repo path with colon'
> '
> test-genrandom foo 4096 >file.bin &&
> git add file.bin &&
> git commit -m bin &&
> - git clone --bare . xxx:yyy.git &&
> +
> + if test_have_prereq MINGW
> + then
> + pathsep=";"
> + else
> + pathsep=":"
> + fi &&
> + git clone --bare . "xxx${pathsep}yyy.git" &&
>
> echo change >>file.bin &&
> git commit -am change &&
> # Note that we have to use the full path here, or it gets confused
> # with the ssh host:path syntax.
> - git push "$PWD/xxx:yyy.git" HEAD
> + git push "$(pwd)/xxx${pathsep}yyy.git" HEAD
> '
>
> test_done
Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
Hi Junio, On Mon, 12 Dec 2016, Junio C Hamano wrote: > * bw/grep-recurse-submodules (2016-11-22) 6 commits > - grep: search history of moved submodules > - grep: enable recurse-submodules to work on objects > - grep: optionally recurse into submodules > - grep: add submodules as a grep source type > - submodules: load gitmodules file from commit sha1 > - submodules: add helper functions to determine presence of submodules > > "git grep" learns to optionally recurse into submodules > > Has anybody else seen t7814 being flakey with this series? It is not flakey for me, it fails consistently on Windows. This is the output with -i -v -x (sorry, I won't have time this week to do anything about it, but maybe it helps identify the root cause): -- snipsnap -- Initialized empty Git repository in C:/git-sdk-64/usr/src/git/wip3/t/trash directory.t7814-grep-recurse-submodules/.git/ expecting success: echo "foobar" >a && mkdir b && echo "bar" >b/b && git add a b && git commit -m "add a and b" && git init submodule && echo "foobar" >submodule/a && git -C submodule add a && git -C submodule commit -m "add a" && git submodule add ./submodule && git commit -m "added submodule" ++ echo foobar ++ mkdir b ++ echo bar ++ git add a b ++ git commit -m 'add a and b' [master (root-commit) 6a17548] add a and b Author: A U Thor 2 files changed, 2 insertions(+) create mode 100644 a create mode 100644 b/b ++ git init submodule Initialized empty Git repository in C:/git-sdk-64/usr/src/git/wip3/t/trash directory.t7814-grep-recurse-submodules/submodule/.git/ ++ echo foobar ++ git -C submodule add a ++ git -C submodule commit -m 'add a' [master (root-commit) 081a998] add a Author: A U Thor 1 file changed, 1 insertion(+) create mode 100644 a ++ git submodule add ./submodule Adding existing repo at 'submodule' to the index ++ git commit -m 'added submodule' [master 0c0fdd0] added submodule Author: A U Thor 2 files changed, 4 insertions(+) create mode 100644 .gitmodules create mode 16 submodule + test_eval_ret_=0 + want_trace + test t = t + test t = t + set +x ok 1 - setup directory structure and submodule expecting success: cat >expect <<-\EOF && a:foobar b/b:bar submodule/a:foobar EOF git grep -e "bar" --recurse-submodules >actual && test_cmp expect actual ++ cat ++ git grep -e bar --recurse-submodules ++ test_cmp expect actual ++ mingw_test_cmp expect actual ++ local test_cmp_a= test_cmp_b= ++ local stdin_for_diff= ++ test -s expect ++ test -s actual ++ mingw_read_file_strip_cr_ test_cmp_a ++ local line ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='a:foobar ' ++ eval 'test_cmp_a=$test_cmp_a$line' +++ test_cmp_a='a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='b/b:bar ' ++ eval 'test_cmp_a=$test_cmp_a$line' +++ test_cmp_a='a:foobar b/b:bar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='submodule/a:foobar ' ++ eval 'test_cmp_a=$test_cmp_a$line' +++ test_cmp_a='a:foobar b/b:bar submodule/a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ test -z '' ++ break ++ mingw_read_file_strip_cr_ test_cmp_b ++ local line ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='a:foobar ' ++ eval 'test_cmp_b=$test_cmp_b$line' +++ test_cmp_b='a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='b/b:bar ' ++ eval 'test_cmp_b=$test_cmp_b$line' +++ test_cmp_b='a:foobar b/b:bar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='submodule/a:foobar ' ++ eval 'test_cmp_b=$test_cmp_b$line' +++ test_cmp_b='a:foobar b/b:bar submodule/a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ test -z '' ++ break ++ test -n 'a:foobar b/b:bar submodule/a:foobar ' ++ test -n 'a:foobar b/b:bar submodule/a:foobar ' ++ test 'a:foobar b/b:bar submodule/a:foobar ' = 'a:foobar b/b:bar submodule/a:foobar ' + test_eval_ret_=0 + want_trace + test t = t + test t = t + set +x ok 2 - grep correctly finds patterns in a submodule expecting success: cat >expect <<-\EOF && submodule/a:foobar EOF git grep -e. --recurse-submodules -- submodule >actual && test_cmp expect actual ++ cat ++ git grep -e. --recurse-submodules -- submodule ++ test_cmp expect actual ++ mingw_test_cmp expect actual ++ local test_cmp_a= test_cmp_b= ++ local stdin_for_diff= ++ test -s expect ++ test -s actual ++ mingw_read_file_strip_cr_ test_cmp_a ++ local line ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='submodule/a:foobar ' ++ eval 'test_cmp_a=$test_cmp_a$line' +++ test_cmp_a='submodule/a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ test -z '' ++ break ++ mingw_read_file_strip_cr_ test_cmp_b ++ local line ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ line='submodule/a:foobar ' ++ eval 'test_cmp_b=$test_cmp_b$line' +++ test_cmp_b='submodule/a:foobar ' ++ : ++ IFS=$'\r' ++ read -r -d ' ' line ++ test -z '' ++ break ++ test -n 'submodule/a:foobar ' ++ test
Re: [PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too
On Tue, Dec 13, 2016 at 08:09:31PM +0100, Johannes Sixt wrote: > Am 12.12.2016 um 20:53 schrieb Jeff King: > > Johannes, please let me know if I am wrong about skipping the test on > > !MINGW. The appropriate check there would be ";" anyway, but I am not > > sure _that_ is allowed in paths, either. > > Here is a version for Windows. I'd prefer this patch on top instead > of squashing it into yours to keep the $PWD vs. $(pwd) explanation. > > The result is the same as yours in all practical matters; but this > version I have already tested. Yeah, I'm happy to have this on top. The patch itself looks obviously correct. Thanks! -Peff
Re: git add -p with new file
Jeff King writes: >> Perhaps the latter is not advertised well enough? "add -p" does not >> even page so it is not very useful way to check what is being added >> if you are adding a new file (unless you are doing a toy example to >> add a 7-line file). > > I use "add -p" routinely for my final add-and-sanity-check,... > ... To me they are all tools in the toolbox, and I can pick the one that > works best in any given situation, or that I just feel like using that > day. Oh, there is no question about that. I was just pointing out that "add -p" is not the "one that works best" when dealing with a path that is not yet even in the index.
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
Stefan Beller writes: >> I do not think there is no dispute about what embedding means. > > double negative: You think we have a slight dispute here. Sorry, I do not think there is any dispute on that. >> A >> submodule whose .git is inside its working tree has its repository >> embedded. >> >> What we had trouble settling on was what to call the operation to >> undo the embedding, unentangling its repository out of the working >> tree. I'd still vote for unembed if you want a name to be nominated. > > So I can redo the series with two commands "git submodule [un]embed". > > For me "unembed" == "absorb", such that we could also go with > absorb into superproject <-> embed into worktree With us agreeing that "embed" is about something is _IN_ submodule working tree, unembed would naturally be something becomes OUTSIDE the same thing (i.e. "submodule working tree"). However, if you introduce "absorb", we suddenly need to talk about a different thing, i.e. "superproject's .git/modules", that is doing the absorption. That is why I suggest "unembed" over "absorb".
Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf
On Tue, Dec 13, 2016 at 11:03:53AM -0800, Junio C Hamano wrote:
> >> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const
> >> char *v, void *cb)
> >> static int run_rewrite_hook(const unsigned char *oldsha1,
> >>const unsigned char *newsha1)
> >> {
> >> - /* oldsha1 SP newsha1 LF NUL */
> >> - static char buf[2*40 + 3];
> >> + char *buf;
> >>struct child_process proc = CHILD_PROCESS_INIT;
> >>const char *argv[3];
> >>int code;
> >> - size_t n;
> >>
> >>argv[0] = find_hook("post-rewrite");
> >>if (!argv[0])
> >> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char
> >> *oldsha1,
> >>code = start_command(&proc);
> >>if (code)
> >>return code;
> >> - n = snprintf(buf, sizeof(buf), "%s %s\n",
> >> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> >> + buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> >>sigchain_push(SIGPIPE, SIG_IGN);
> >> - write_in_full(proc.in, buf, n);
> >> + write_in_full(proc.in, buf, strlen(buf));
> >>close(proc.in);
> >> + free(buf);
> >
> > Any time you care about the length of the result, I'd generally use an
> > actual strbuf instead of xstrfmt. The extra strlen isn't a big deal
> > here, but it somehow seems simpler to me. It probably doesn't matter
> > much either way, though.
>
> Your justification for this extra allocation was that it is a
> heavy-weight operation. While I agree that the runtime cost of
> allocation and deallocation does not matter, I would be a bit
> worried about extra cognitive burden to programmers. They did not
> have to worry about leaking because they are writing a fixed length
> string. Now they do, whether they use xstrfmt() or struct strbuf.
> When they need to update what they write, they do have to remember
> to adjust the size of the "fixed string", and the original is not
> free from the "programmers' cognitive cost" point of view, of
> course. Probably use of strbuf/xstrfmt is an overall win.
So I think you are agreeing, but I have a minor nit to pick. :)
The fact that the extra allocation will not hurt performance is
_necessary_, but not _sufficient_. So it's not a justification in
itself, only something we have to check before proceeding.
The only justification here is that magic numbers like "2*40 + 3" are
confusing and a potential maintenance burden. And that's why I suggested
splitting this one out from the other two (whose justification is
"PATH_MAX is sometimes too small").
I agree with you that it's a tradeoff between "magic numbers" versus
"having to free resources". In my opinion it's a net improvement, but I
think it would also be reasonable to switch to xsnprintf() here. Then
the programmer has an automatic check that the buffer size is
sufficient.
-Peff
[PATCH 3/2] t5547-push-quarantine: run the path separator test on Windows, too
To perform the test case on Windows in a way that corresponds to the
POSIX version, inject the semicolon in a directory name.
Typically, an absolute POSIX style path, such as the one in $PWD, is
translated into a Windows style path by bash when it invokes git.exe.
However, the presence of the semicolon suppresses this translation;
but the untranslated POSIX style path is useless for git.exe.
Therefore, instead of $PWD pass the Windows style path that $(pwd)
produces.
Signed-off-by: Johannes Sixt
---
Am 12.12.2016 um 20:53 schrieb Jeff King:
> Johannes, please let me know if I am wrong about skipping the test on
> !MINGW. The appropriate check there would be ";" anyway, but I am not
> sure _that_ is allowed in paths, either.
Here is a version for Windows. I'd prefer this patch on top instead
of squashing it into yours to keep the $PWD vs. $(pwd) explanation.
The result is the same as yours in all practical matters; but this
version I have already tested.
t/t5547-push-quarantine.sh | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 6275ec807b..af9fcd833a 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -33,8 +33,7 @@ test_expect_success 'rejected objects are removed' '
test_cmp expect actual
'
-# MINGW does not allow colons in pathnames in the first place
-test_expect_success !MINGW 'push to repo path with colon' '
+test_expect_success 'push to repo path with path separator (colon)' '
# The interesting failure case here is when the
# receiving end cannot access its original object directory,
# so make it likely for us to generate a delta by having
@@ -43,13 +42,20 @@ test_expect_success !MINGW 'push to repo path with colon' '
test-genrandom foo 4096 >file.bin &&
git add file.bin &&
git commit -m bin &&
- git clone --bare . xxx:yyy.git &&
+
+ if test_have_prereq MINGW
+ then
+ pathsep=";"
+ else
+ pathsep=":"
+ fi &&
+ git clone --bare . "xxx${pathsep}yyy.git" &&
echo change >>file.bin &&
git commit -am change &&
# Note that we have to use the full path here, or it gets confused
# with the ssh host:path syntax.
- git push "$PWD/xxx:yyy.git" HEAD
+ git push "$(pwd)/xxx${pathsep}yyy.git" HEAD
'
test_done
--
2.11.0.55.g6a4dbb1
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Tue, Dec 13, 2016 at 10:53 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Mon, Dec 12, 2016 at 11:28 PM, Junio C Hamano wrote: >>> Stefan Beller writes: >>> The "checkout --recurse-submodules" series got too large to comfortably send it out for review, so I had to break it up into smaller series'; this is the first subseries, but it makes sense on its own. This series teaches git-rm to absorb the git directory of a submodule instead of failing and complaining about the git directory preventing deletion. It applies on origin/sb/submodule-embed-gitdir. >>> >>> Thanks. I probably should rename the topic again with s/embed/absorb/; >> >> I mostly renamed it in the hope to settle our dispute what embedding means. >> ;) > > I do not think there is no dispute about what embedding means. double negative: You think we have a slight dispute here. > A > submodule whose .git is inside its working tree has its repository > embedded. > > What we had trouble settling on was what to call the operation to > undo the embedding, unentangling its repository out of the working > tree. I'd still vote for unembed if you want a name to be nominated. So I can redo the series with two commands "git submodule [un]embed". For me "unembed" == "absorb", such that we could also go with absorb into superproject <-> embed into worktree > >> Note that sb/t3600-cleanup is part of this series now, >> (The first commit of that series is in patch 6/6 of this series, and patch 5 >> is >> the modernization effort.) > > Well, "t3600: remove useless redirect" has been in 'next' already; > do you mean that you want me to queue this series on top of that > topic? > I need to reroll this series any way; the reroll will be on top of that. Thanks, Stefan
Re: [PATCH] builtin/commit.c: convert trivial snprintf calls to xsnprintf
Jeff King writes:
>> +argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
>> +if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
>> fprintf(stderr,
>> _("Please supply the message using either -m or -F
>> option.\n"));
>> +argv_array_clear(&env);
>> exit(1);
>> }
>> +argv_array_clear(&env);
>
> I'd generally skip the clear() right before exiting, though I saw
> somebody disagree with me recently on that. I wonder if we should
> decide as a project on whether it is worth doing or not.
I'd say it is a responsibility of the person who turns exit(1) to
return -1 to ensure the code does not leak.
>> @@ -1525,12 +1526,10 @@ static int git_commit_config(const char *k, const
>> char *v, void *cb)
>> static int run_rewrite_hook(const unsigned char *oldsha1,
>> const unsigned char *newsha1)
>> {
>> -/* oldsha1 SP newsha1 LF NUL */
>> -static char buf[2*40 + 3];
>> +char *buf;
>> struct child_process proc = CHILD_PROCESS_INIT;
>> const char *argv[3];
>> int code;
>> -size_t n;
>>
>> argv[0] = find_hook("post-rewrite");
>> if (!argv[0])
>> @@ -1546,34 +1545,33 @@ static int run_rewrite_hook(const unsigned char
>> *oldsha1,
>> code = start_command(&proc);
>> if (code)
>> return code;
>> -n = snprintf(buf, sizeof(buf), "%s %s\n",
>> - sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> +buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> sigchain_push(SIGPIPE, SIG_IGN);
>> -write_in_full(proc.in, buf, n);
>> +write_in_full(proc.in, buf, strlen(buf));
>> close(proc.in);
>> +free(buf);
>
> Any time you care about the length of the result, I'd generally use an
> actual strbuf instead of xstrfmt. The extra strlen isn't a big deal
> here, but it somehow seems simpler to me. It probably doesn't matter
> much either way, though.
Your justification for this extra allocation was that it is a
heavy-weight operation. While I agree that the runtime cost of
allocation and deallocation does not matter, I would be a bit
worried about extra cognitive burden to programmers. They did not
have to worry about leaking because they are writing a fixed length
string. Now they do, whether they use xstrfmt() or struct strbuf.
When they need to update what they write, they do have to remember
to adjust the size of the "fixed string", and the original is not
free from the "programmers' cognitive cost" point of view, of
course. Probably use of strbuf/xstrfmt is an overall win.
And of course, I think strbuf is more appropriate if you have to do
strlen().
Re: git add -p with new file
On Tue, Dec 13, 2016 at 10:48:07AM -0800, Junio C Hamano wrote: > > I think the problem is just that "add -p" does not give the whole story > > of what you might want to do before making a commit. > > The same is shared by "git diff [HEAD]", by the way. It is beyond > me why people use "add -p", not "git diff [HEAD]", for the final > sanity check before committing. > > Perhaps the latter is not advertised well enough? "add -p" does not > even page so it is not very useful way to check what is being added > if you are adding a new file (unless you are doing a toy example to > add a 7-line file). I use "add -p" routinely for my final add-and-sanity-check, and it is certainly not because I don't know about "git diff". I think it's just nice to break it into bite-size chunks and sort them into "yes, OK" or "no, not yet" bins. The lack of paging isn't usually a problem, because this "add -p" is useful precisely when you have a lot of little changes spread across the code base. I'd probably also run "git diff" if I wanted to look at something bigger. And I routinely use "git status", too, to see the full state of my tree. To me they are all tools in the toolbox, and I can pick the one that works best in any given situation, or that I just feel like using that day. > >> Hm, "interactive.showUntracked" is a confusing name because "git add -i" > >> (interactive) already handles untracked files. > > > > Sure, that was just meant for illustration. I agree there's probably a > > better name. > > "interactive.*" is not a sensible hierarchy to use, because things > other than "add" can go interactive. > > addPatch.showUntracked? Hmm, I wonder if interactive.diffFilter was mis-named then. The description is written in such a way as to cover other possible interactive commands showing a diff. It might be possible to do the same here: come up with a general option that _could_ cover new situations, but right now just applies here. Or maybe it would be too confusing. TBH, I wasn't all that concerned with the name yet. Whoever writes the patch can figure it out, and _then_ we can all bikeshed it. :) -Peff
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
Stefan Beller writes: > On Mon, Dec 12, 2016 at 11:28 PM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> The "checkout --recurse-submodules" series got too large to comfortably send >>> it out for review, so I had to break it up into smaller series'; this is the >>> first subseries, but it makes sense on its own. >>> >>> This series teaches git-rm to absorb the git directory of a submodule >>> instead >>> of failing and complaining about the git directory preventing deletion. >>> >>> It applies on origin/sb/submodule-embed-gitdir. >> >> Thanks. I probably should rename the topic again with s/embed/absorb/; > > I mostly renamed it in the hope to settle our dispute what embedding means. ;) I do not think there is no dispute about what embedding means. A submodule whose .git is inside its working tree has its repository embedded. What we had trouble settling on was what to call the operation to undo the embedding, unentangling its repository out of the working tree. I'd still vote for unembed if you want a name to be nominated. > Note that sb/t3600-cleanup is part of this series now, > (The first commit of that series is in patch 6/6 of this series, and patch 5 > is > the modernization effort.) Well, "t3600: remove useless redirect" has been in 'next' already; do you mean that you want me to queue this series on top of that topic?
Re: git add -p with new file
Jeff King writes: >> I am also not really sure what problem this feature is trying to solve. >> If the "problem"(?) is that it should act more like "git add" instead of >> "git add -u", for whatever reason, this may be fine (but the >> configuration option is a must-have then). > > I think the problem is just that "add -p" does not give the whole story > of what you might want to do before making a commit. The same is shared by "git diff [HEAD]", by the way. It is beyond me why people use "add -p", not "git diff [HEAD]", for the final sanity check before committing. Perhaps the latter is not advertised well enough? "add -p" does not even page so it is not very useful way to check what is being added if you are adding a new file (unless you are doing a toy example to add a 7-line file). >> > I'd also probably add interactive.showUntracked to make the whole thing >> > optional (but I think it would be OK to default it to on). >> Hm, "interactive.showUntracked" is a confusing name because "git add -i" >> (interactive) already handles untracked files. > > Sure, that was just meant for illustration. I agree there's probably a > better name. "interactive.*" is not a sensible hierarchy to use, because things other than "add" can go interactive. addPatch.showUntracked?
Re: [PATCH 0/2] handling alternates paths with colons
On Tue, Dec 13, 2016 at 10:42:39AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > Right, but we also support relative paths via environment variables. I > > don't think that changes the conclusion though. I'm not convinced > > relative paths via the environment aren't slightly insane in the first > > place, > > Sorry, a triple negation is above my head. "relative paths in env > aren't insane" == "using relative paths is OK" and you are not > convinced that it is the case, so you are saying that you think > relative paths in env is not something that is sensible? I think relative paths in env have very flaky semantics which makes them inadvisable to use in general. That being said, when we broke even those flaky semantics somebody complained. So I consider a feature we _do_ support, but not one I would recommend to people. -Peff
Re: [PATCH 0/2] handling alternates paths with colons
Jeff King writes: > Right, but we also support relative paths via environment variables. I > don't think that changes the conclusion though. I'm not convinced > relative paths via the environment aren't slightly insane in the first > place, Sorry, a triple negation is above my head. "relative paths in env aren't insane" == "using relative paths is OK" and you are not convinced that it is the case, so you are saying that you think relative paths in env is not something that is sensible? > given the discussion in 37a95862c (alternates: re-allow relative > paths from environment, 2016-11-07). And they'd probably start with > "../" as well. Yeah. In any case, there is a potential regression but the chance is miniscule---the user has to have been using a relative path that begins with a double-quote in the environment or in alternates file.
Re: [PATCH 0/2] handling alternates paths with colons
On Tue, Dec 13, 2016 at 10:10:54AM -0800, Junio C Hamano wrote: > > We do support non-absolute paths, both in alternates files and > > environment variables. It's a nice feature if you want to have a > > relocatable family of shared repositories. I'd imagine that most cases > > start with "../", though. > > Yes. I was only talking about the environment variable, as you can > tell from my mention of "colon" there. Right, but we also support relative paths via environment variables. I don't think that changes the conclusion though. I'm not convinced relative paths via the environment aren't slightly insane in the first place, given the discussion in 37a95862c (alternates: re-allow relative paths from environment, 2016-11-07). And they'd probably start with "../" as well. -Peff
Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
On Tue, Dec 13, 2016 at 10:10:04AM -0800, Junio C Hamano wrote:
> > - git clone --bare . xxx:yyy.git &&
> > + git clone --bare . xxx${path_sep}yyy.git &&
>
> Don't you want to dq the whole thing to prevent the shell from
> splitting this into two commands at ';'? The other one below is OK.
After expansion, I don't think the shell will do any further processing
except for whitespace splitting. E.g.:
$ debug() { for i in "$@"; do echo "got $i"; done; }
$ sep=';'
$ space=' '
$ debug foo${sep}bar
got foo;bar
$ debug foo${space}bar
got foo
got bar
I don't mind quoting it to make it more obvious, though.
-Peff
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Vasco Almeida writes: >> We only update comment_line_char from the default "#" when the >> configured value is a single-byte character and we ignore incorrect >> values in the configuration file. So I think the patch you sent is >> correct after all. > > I am still not sure what version do we prefer. > > Check whether core.commentchar is a single character. If not, use '#' > as the $comment_line_char. This, plus special casing "auto". Picking the first byte is inconsistent with the current practice (the paragraph you quoted above), I think.
Re: [PATCH 1/4] doc: add articles (grammar)
Kristoffer Haugsbakk writes: > Thank you for reviewing this series, Philip. > > Philip Oakley writes: > >> This looks good to me. > > I'll add this header: > > Acked-by: Philip Oakley > > To the commit message of this patch in the next review round (version 2 > of the patch series). > > Let me know if I should add another header, or do something different > than this. I was planning to merge all four from you as-is to 'next' today, though. Should I wait?
Re: [PATCH 0/2] handling alternates paths with colons
Jeff King writes: > On Mon, Dec 12, 2016 at 02:37:08PM -0800, Junio C Hamano wrote: > >> Jeff King writes: >> >> > So here are patches that do that. It kicks in only when the first >> > character of a path is a double-quote, and then expects the usual >> > C-style quoting. >> >> The quote being per delimited component is what makes this fifth >> approach a huge win. >> >> All sane components on a list-valued environment are still honored >> and an insane component that has either a colon in the middle or >> begins with a double-quote gets quoted. As long as nobody used a >> path that begins with a double-quote as an element in such a >> list-valued environment (and they cannot be, as using a non-absolute >> path as an element does not make much sense), this will be safe, and >> a path with a colon didn't work with Git unaware of the new quoting >> rule anyway. Nice. > > We do support non-absolute paths, both in alternates files and > environment variables. It's a nice feature if you want to have a > relocatable family of shared repositories. I'd imagine that most cases > start with "../", though. Yes. I was only talking about the environment variable, as you can tell from my mention of "colon" there.
Re: [PATCH 2/2] tmp-objdir: quote paths we add to alternates
Jeff King writes:
> The naive conversion is just:
> ...
> -# MINGW does not allow colons in pathnames in the first place
> -test_expect_success !MINGW 'push to repo path with colon' '
> +if test_have_prereq MINGW
> +then
> + path_sep=';'
> +else
> + path_sep=':'
> +fi
> ...
> - git clone --bare . xxx:yyy.git &&
> + git clone --bare . xxx${path_sep}yyy.git &&
Don't you want to dq the whole thing to prevent the shell from
splitting this into two commands at ';'? The other one below is OK.
>
> echo change >>file.bin &&
> git commit -am change &&
> # Note that we have to use the full path here, or it gets confused
> # with the ssh host:path syntax.
> - git push "$PWD/xxx:yyy.git" HEAD
> + git push "$PWD/xxx${path_sep}yyy.git" HEAD
> '
>
> test_done
>
> Does that work?
>
> -Peff
Re: [PATCH 1/2] alternates: accept double-quoted paths
Duy Nguyen writes: > At least attr has the same problem and is going the same direction > [1]. Cool. (I actually thought the patch was in and evidence that this > kind of backward compatibility breaking was ok, turns out the patch > has stayed around for years) > > [1] > http://public-inbox.org/git/%3c20161110203428.30512-18-sbel...@google.com%3E/ Seeing that again, I see this in the proposed log message: Full pattern must be quoted. So 'pat"t"ern attr' will give exactly 'pat"t"ern', not 'pattern'. I cannot tell what it is trying to say. The code suggests something quite different, i.e. a line like this "pat\"t\"ern" attr would tell us that a path that matches the pattern pat"t"ern is given 'attr'. The log message may need to be cleaned up. The update by that commit to the documentation looks alright, thoguh.
RE: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
> -Original Message-
> From: Stefan Beller [mailto:sbel...@google.com]
> Sent: Monday, December 12, 2016 8:41 PM
> To: gits...@pobox.com
> Cc: git@vger.kernel.org; David Turner; bmw...@google.com; Stefan Beller
> Subject: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
>
> When deleting a submodule we need to keep the actual git directory around,
Nit: comma after submodule.
> - strbuf_reset(&buf);
> - strbuf_addstr(&buf, path);
> - if (!remove_dir_recursively(&buf, 0)) {
> - removed = 1;
> - if
> (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> - strbuf_release(&buf);
> - continue;
> - } else if (!file_exists(path))
> - /* Submodule was removed by
> user */
> - if
> (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> + if (file_exists(path))
> + depopulate_submodule(path);
> + removed = 1;
> + if (!remove_path_from_gitmodules(path))
> + gitmodules_modified = 1;
> + continue;
> /* Fallthrough and let remove_path()
> fail.
> */
It seems odd to have a continue right before a comment that says "Fallthrough".
Re: [PATCHv2 1/2] merge: Add '--continue' option as a synonym for 'git commit'
Chris Packham writes:
> +'git merge' --continue
>
> DESCRIPTION
> ---
> @@ -61,6 +62,9 @@ reconstruct the original (pre-merge) changes. Therefore:
> discouraged: while possible, it may leave you in a state that is hard to
> back out of in the case of a conflict.
>
> +The fourth syntax ("`git merge --continue`") can only be run after the
> +merge has resulted in conflicts.
OK. I can see that the code refuses if there is no MERGE_HEAD, so
"can only be run" is ensured correctly.
> 'git merge --continue' will take the
> +currently staged changes and complete the merge.
For Git-savvy folks, this may be sufficient to tell that they are
expected to resolve conflicts in the working tree and register the
resolusion by doing "git add" before running "git merge --continue",
but I wonder if that is clear enough for new readers.
The same comment applies to the option description below. I suspect
that it is better to remove the last sentence above, leaving "4th
one can be run only with MERGE_HEAD" here, and enhance the
explanation in the option description (see below).
> OPTIONS
> ---
> @@ -99,6 +103,12 @@ commit or stash your changes before running 'git merge'.
> 'git merge --abort' is equivalent to 'git reset --merge' when
> `MERGE_HEAD` is present.
>
> +--continue::
> + Take the currently staged changes and complete the merge.
> ++
> +'git merge --continue' is equivalent to 'git commit' when
> +`MERGE_HEAD` is present.
> +
These two sentences are even more technical and unfriendly to new
readers, I am afraid. How about giving a hint by referring to an
existing section, like this?
--continue::
After a "git merge" stops due to conflicts you can conclude
the merge by running "git merge --continue" (see "How to
resolve conflicts" section below).
> @@ -277,7 +287,8 @@ After seeing a conflict, you can do two things:
>
> * Resolve the conflicts. Git will mark the conflicts in
> the working tree. Edit the files into shape and
> - 'git add' them to the index. Use 'git commit' to seal the deal.
> + 'git add' them to the index. Use 'git merge --continue' to seal the
> + deal.
Why do we want to make it harder to discover "git commit" here?
I would understand:
... Use 'git commit' to conclude (you can also say 'git
merge --continue').
though. After all, we are merely introducing a synonym for those
who want to type more. There is no plan to deprecate the use of
'git commit', which is a perfectly reasonable way to conclude an
interrupted merge, that has worked well for us for the past 10 years
and still works.
> @@ -65,6 +66,7 @@ static int option_renormalize;
> static int verbosity;
> static int allow_rerere_auto;
> static int abort_current_merge;
> +static int continue_current_merge;
> static int allow_unrelated_histories;
> static int show_progress = -1;
> static int default_to_upstream = 1;
> @@ -223,6 +225,8 @@ static struct option builtin_merge_options[] = {
> OPT__VERBOSITY(&verbosity),
> OPT_BOOL(0, "abort", &abort_current_merge,
> N_("abort the current in-progress merge")),
> + OPT_BOOL(0, "continue", &continue_current_merge,
> + N_("continue the current in-progress merge")),
> OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
>N_("allow merging unrelated histories")),
> OPT_SET_INT(0, "progress", &show_progress, N_("force progress
> reporting"), 1),
> @@ -739,7 +743,7 @@ static void abort_commit(struct commit_list *remoteheads,
> const char *err_msg)
> if (err_msg)
> error("%s", err_msg);
> fprintf(stderr,
> - _("Not committing merge; use 'git commit' to complete the
> merge.\n"));
> + _("Not committing merge; use 'git merge --continue' to complete
> the merge.\n"));
Likewise. I do not see a need to change this one at all.
> @@ -1166,6 +1170,22 @@ int cmd_merge(int argc, const char **argv, const char
> *prefix)
> goto done;
> }
>
> + if (continue_current_merge) {
> + int nargc = 1;
> + const char *nargv[] = {"commit", NULL};
> +
> + if (argc)
> + usage_msg_opt("--continue expects no arguments",
> + builtin_merge_usage, builtin_merge_options);
Peff already commented on "what about other options?", and I think
his "check the number of args before parse-options ran to ensure
that the '--abort' or '--continue' was the only thing" is probably
a workable hack.
The "right" way to fix it would be way too involved to be worth for
just this single change (and also fixing "--abort"). Just thinking
aloud:
* Update parse-options API to:
- extend "struct option" with one field that holds what "command
modes" the option the "struct option" describes is incompatible
with.
- make parse_options() to keep track of the set of command modes
that are
Re: [PATCH 0/6] git-rm absorbs submodule git directory before deletion
On Mon, Dec 12, 2016 at 11:28 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> The "checkout --recurse-submodules" series got too large to comfortably send >> it out for review, so I had to break it up into smaller series'; this is the >> first subseries, but it makes sense on its own. >> >> This series teaches git-rm to absorb the git directory of a submodule instead >> of failing and complaining about the git directory preventing deletion. >> >> It applies on origin/sb/submodule-embed-gitdir. > > Thanks. I probably should rename the topic again with s/embed/absorb/; I mostly renamed it in the hope to settle our dispute what embedding means. ;) So in case we want to further discuss on the name of the function, we should do that before doing actual work such as renaming. Note that sb/t3600-cleanup is part of this series now, (The first commit of that series is in patch 6/6 of this series, and patch 5 is the modernization effort.) Thanks, Stefan
Re: [PATCH 6/6] rm: add absorb a submodules git dir before deletion
On Mon, Dec 12, 2016 at 7:28 PM, brian m. carlson wrote: > On Mon, Dec 12, 2016 at 05:40:55PM -0800, Stefan Beller wrote: >> When deleting a submodule we need to keep the actual git directory around, >> such that we do not lose local changes in there and at a later checkout >> of the submodule we don't need to clone it again. >> >> Implement `depopulate_submodule`, that migrates the git directory before >> deletion of a submodule and afterwards the equivalent of "rm -rf", which >> is already found in entry.c, so expose that and for clarity add a suffix >> "_or_dir" to it. > > I think you might have meant "_or_die" here. indeed, will fix in a reroll. Thanks for the review!
Re: git add -p with new file
On Mon, Dec 12, 2016 at 09:31:03PM +0100, Stephan Beyer wrote: > I am also a "git add -p"-only user (except for new files and merges). > However, I usually keep a lot of untracked files in my repositories. > Files that I do not (git)ignore because I want to see them when I type > "git status". > > Hence, the imagination only that "git add -p" starts to ask me for each > untracked file feels like a lot of annoying "n" presses. I could imagine > that it is okay-ish when it asks about the untracked files *after* all > tracked paths have been processed (I guess this has been proposed > before), so that I can safely quit. Yeah, this is the "some people might be annoyed" that I mentioned originally. If your workflow leaves a lot of untracked files that you don't care about it, then I think you'd want this feature disabled entirely via a config option (or vice versa, that it would only be enabled by config option). > I am also not really sure what problem this feature is trying to solve. > If the "problem"(?) is that it should act more like "git add" instead of > "git add -u", for whatever reason, this may be fine (but the > configuration option is a must-have then). I think the problem is just that "add -p" does not give the whole story of what you might want to do before making a commit. > > I'd also probably add interactive.showUntracked to make the whole thing > > optional (but I think it would be OK to default it to on). > Hm, "interactive.showUntracked" is a confusing name because "git add -i" > (interactive) already handles untracked files. Sure, that was just meant for illustration. I agree there's probably a better name. -Peff
