Re: [PATCH v9 01/11] ref-filter: print output to strbuf for formatting
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
Introduce a strbuf `output` which will act as a substitute rather than
printing directly to stdout. This will be used for formatting
eventually.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/ref-filter.c b/ref-filter.c
index 46963a5..91482c9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format, int qu
if (color_parse(reset, color) 0)
die(BUG: couldn't parse 'reset' as a color);
resetv.s = color;
- print_value(resetv, quote_style);
+ print_value(resetv, quote_style, output);
}
+ for (i = 0; i output.len; i++)
+ printf(%c, output.buf[i]);
Everything up to this point seems straightforward, however, it's not
clear why you need to emit 'output' one character at a time. Is it
because it might contain a NUL '\0' character and therefore you can't
use the simpler printf(%s, output.buf)?
If that's the case, then why not just use fwrite() to emit it all at once?
fwrite(output.buf, output.len, 1, stdout);
putchar('\n');
+ strbuf_release(output);
}
/* If no sorting option is given, use refname to sort as default */
--
2.5.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule: implement `module_name` as a builtin helper
Am 05.08.2015 um 23:08 schrieb Stefan Beller:
This implements the helper `module_name` in C instead of shell,
yielding a nice performance boost.
Before this patch, I measured a time (best out of three):
$ time ./t7400-submodule-basic.sh /dev/null
real 0m11.066s
user 0m3.348s
sys0m8.534s
With this patch applied I measured (also best out of three)
$ time ./t7400-submodule-basic.sh /dev/null
real 0m10.063s
user 0m3.044s
sys0m7.487s
Signed-off-by: Stefan Beller sbel...@google.com
---
Is this what you had in mind, Jens?
Yup, thanks!
Just some small nits, please see below (and also in the fixup! commit
I pushed on the submodule--helper branch in my Github repo).
Jonathan pointed me to https://github.com/jlehmann/git-submod-enhancements/wiki
Does it reflect reality (i.e. as time passes code changes)?
I also noticed that you have made quite some changes to submodules on different
branches which are not upstream. Soem changes look familiar (as in I believe
this is upstream alreaday? while others look new and exciting to me).
I could not quite get the order yet, though.
I think the Wiki should be pretty much up to date, but I'll try to check
that and the state of the branches on the weekend to see if it needs an
update. If you see some branches you believe are already upstream, it'd
be great if you could mention them so I can double check.
builtin/submodule--helper.c | 23 +++
git-submodule.sh| 32 +++-
submodule.c | 18 +-
submodule.h | 1 +
4 files changed, 44 insertions(+), 30 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index cb18ddf..3713c4c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,8 @@
#include pathspec.h
#include dir.h
#include utf8.h
+#include submodule.h
+#include string-list.h
static char *ps_matched;
static const struct cache_entry **ce_entries;
@@ -98,6 +100,24 @@ static int module_list(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_name(int argc, const char **argv, const char *prefix)
+{
+ const char *name;
+
+ if (argc != 1)
+ usage(git submodule--helper module_name path\n);
+
+ gitmodules_config();
+ name = submodule_name_for_path(argv[0]);
+
+ if (name)
+ printf(%s\n, name);
+ else
+ die(No submodule mapping found in .gitmodules for path '%s',
argv[0]);
Hmm, I prefer the pattern to bail out inside if() and continue with the
expected case without else:
+ if (!name)
+ die(No submodule mapping found in .gitmodules for path '%s',
argv[0]);
+
+ printf(%s\n, name);
But maybe that's just me.
+ return 0;
+}
+
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
{
if (argc 2)
@@ -106,6 +126,9 @@ int cmd_submodule__helper(int argc, const char **argv,
const char *prefix)
if (!strcmp(argv[1], module_list))
return module_list(argc - 1, argv + 1, prefix);
+ if (!strcmp(argv[1], module_name))
+ return module_name(argc - 2, argv + 2, prefix);
+
usage:
usage(git submodule--helper module_list\n);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index af9ecef..e6ff38d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -178,24 +178,6 @@ get_submodule_config () {
printf '%s' ${value:-$default}
}
-
-#
-# Map submodule path to submodule name
-#
-# $1 = path
-#
-module_name()
-{
- # Do we have submodule.something.path = $1 defined in .gitmodules
file?
- sm_path=$1
- re=$(printf '%s\n' $1 | sed -e 's/[].[^$\\*]/\\/g')
- name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
- sed -n -e 's|^submodule\.\(.*\)\.path '$re'$|\1|p' )
- test -z $name
- die $(eval_gettext No submodule mapping found in .gitmodules for path
'\$sm_path')
- printf '%s\n' $name
-}
-
#
# Clone a submodule
#
@@ -498,7 +480,7 @@ cmd_foreach()
then
displaypath=$(relative_path $sm_path)
say $(eval_gettext Entering '\$prefix\$displaypath')
- name=$(module_name $sm_path)
+ name=$(git submodule--helper module_name $sm_path)
(
prefix=$prefix$sm_path/
clear_local_git_env
@@ -554,7 +536,7 @@ cmd_init()
while read mode sha1 stage sm_path
do
die_if_unmatched $mode
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
displaypath=$(relative_path $sm_path)
@@ -636,7 +618,7 @@ cmd_deinit()
Re: [RFC/PATCH 0/4] parallel fetch for submodules
Am 06.08.2015 um 19:35 schrieb Stefan Beller: When I was looking at the branches of Jens for work done on submodules not yet upstream I found a commit WIP threaded submodule fetching[1], and I was side tracked wanting to present a different approach to that. Cool. I didn't follow that route further than building a proof of concept because I ran into a nasty DNS-timeout on my router at home and at work we host all repos on a not-so-beefy server making parallel fetch rather pointless. But I suspect this approach will bring down fetch times for some users. Maybe we could also re-use parallel fetch for multiple upstreams in the superproject when doing a git fetch --all without too much extra work? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/4] parallel fetch for submodules
On Thu, Aug 6, 2015 at 1:08 PM, Jens Lehmann jens.lehm...@web.de wrote: Am 06.08.2015 um 19:35 schrieb Stefan Beller: When I was looking at the branches of Jens for work done on submodules not yet upstream I found a commit WIP threaded submodule fetching[1], and I was side tracked wanting to present a different approach to that. Cool. I didn't follow that route further than building a proof of concept because I ran into a nasty DNS-timeout on my router at home and at work we host all repos on a not-so-beefy server making parallel fetch rather pointless. But I suspect this approach will bring down fetch times for some users. The difference between your proof of concept and mine is to have the number of threads easier configurable. (Think of git fetch --recurse-submodules=yes -j4, similar to make -j4 splitting work up to 4 different programs at the same time. And that thing would be possible to add in this series) If you fetch lots of submodules, both the client and server load should come in an ping-pong on-off pattern as the client waits for the server to prepare stuff and get it sent to it and then the client needs time to resolve deltas and write to disk. Depending on the duty cycle of each, a different number of parallel threads make sense (I would expected that they shift their phases against each other by pure randomness, i.e. one thread is currently resolving deltas while the other thread is telling the server to get some work done, so both the client and server get utilized at the same time). Maybe we could also re-use parallel fetch for multiple upstreams in the superproject when doing a git fetch --all without too much extra work? That's why I tried advertising RFC patch 2/4 as I believe it could be easily reused for such things like git fetch --all, but maybe other people see problems with it (over/under engineered, wrong things added, but other critical things missing) so I'd like to hear opinions on that. :) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty
On Tue, Aug 4, 2015 at 10:08 AM, Paul Tan pyoka...@gmail.com wrote: When resuming, git-am detects if we are trying to feed it patches or not by checking if stdin is a TTY. However, the test library redirects stdin to /dev/null. This makes it difficult, for instance, to test the behavior of git am -3 when resuming, as git-am will think we are trying to feed it patches and error out. Support this use case by extending test-terminal.perl to create a pseudo-tty for the child process' standard input as well. An alternative would be to have git-am detect that it is being tested and pretend that isatty() returns true. There is some precedent for having core functionality recognize that it is being tested. See, for instance, environment variable TEST_DATE_NOW, and rev-list --test-bitmap. Doing so would allow the tests work on non-Unix platforms, as well. Note that due to the way the code is structured, the child's stdin pseudo-tty will be closed when we finish reading from our stdin. This means that in the common case, where our stdin is attached to /dev/null, the child's stdin pseudo-tty will be closed immediately. Some operations like isatty(), which git-am uses, require the file descriptor to be open, and hence if the success of the command depends on such functions, test_terminal's stdin should be redirected to a source with large amount of data to ensure that the child's stdin is not closed, e.g. test_terminal git am --3way /dev/zero Signed-off-by: Paul Tan pyoka...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] submodule: implement `module_name` as a builtin helper
Am 06.08.2015 um 19:35 schrieb Stefan Beller:
This implements the helper `module_name` in C instead of shell,
yielding a nice performance boost.
Before this patch, I measured a time (best out of three):
$ time ./t7400-submodule-basic.sh /dev/null
real 0m11.066s
user 0m3.348s
sys0m8.534s
With this patch applied I measured (also best out of three)
$ time ./t7400-submodule-basic.sh /dev/null
real 0m10.063s
user 0m3.044s
sys0m7.487s
Signed-off-by: Stefan Beller sbel...@google.com
---
Please see my comments in the other thread concerning this patch.
And wouldn't it make more sense to keep this patch together with
the submodule: implement `module_list` as a builtin helper in
its own submodule-helper series and have the following three
patches in a separate parallel fetch for submodules series?
builtin/submodule--helper.c | 23 +++
git-submodule.sh| 32 +++-
submodule.c | 18 +-
submodule.h | 1 +
4 files changed, 44 insertions(+), 30 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index cb18ddf..3713c4c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,8 @@
#include pathspec.h
#include dir.h
#include utf8.h
+#include submodule.h
+#include string-list.h
static char *ps_matched;
static const struct cache_entry **ce_entries;
@@ -98,6 +100,24 @@ static int module_list(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_name(int argc, const char **argv, const char *prefix)
+{
+ const char *name;
+
+ if (argc != 1)
+ usage(git submodule--helper module_name path\n);
+
+ gitmodules_config();
+ name = submodule_name_for_path(argv[0]);
+
+ if (name)
+ printf(%s\n, name);
+ else
+ die(No submodule mapping found in .gitmodules for path '%s',
argv[0]);
+
+ return 0;
+}
+
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
{
if (argc 2)
@@ -106,6 +126,9 @@ int cmd_submodule__helper(int argc, const char **argv,
const char *prefix)
if (!strcmp(argv[1], module_list))
return module_list(argc - 1, argv + 1, prefix);
+ if (!strcmp(argv[1], module_name))
+ return module_name(argc - 2, argv + 2, prefix);
+
usage:
usage(git submodule--helper module_list\n);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index af9ecef..e6ff38d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -178,24 +178,6 @@ get_submodule_config () {
printf '%s' ${value:-$default}
}
-
-#
-# Map submodule path to submodule name
-#
-# $1 = path
-#
-module_name()
-{
- # Do we have submodule.something.path = $1 defined in .gitmodules
file?
- sm_path=$1
- re=$(printf '%s\n' $1 | sed -e 's/[].[^$\\*]/\\/g')
- name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
- sed -n -e 's|^submodule\.\(.*\)\.path '$re'$|\1|p' )
- test -z $name
- die $(eval_gettext No submodule mapping found in .gitmodules for path
'\$sm_path')
- printf '%s\n' $name
-}
-
#
# Clone a submodule
#
@@ -498,7 +480,7 @@ cmd_foreach()
then
displaypath=$(relative_path $sm_path)
say $(eval_gettext Entering '\$prefix\$displaypath')
- name=$(module_name $sm_path)
+ name=$(git submodule--helper module_name $sm_path)
(
prefix=$prefix$sm_path/
clear_local_git_env
@@ -554,7 +536,7 @@ cmd_init()
while read mode sha1 stage sm_path
do
die_if_unmatched $mode
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
displaypath=$(relative_path $sm_path)
@@ -636,7 +618,7 @@ cmd_deinit()
while read mode sha1 stage sm_path
do
die_if_unmatched $mode
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
displaypath=$(relative_path $sm_path)
@@ -758,7 +740,7 @@ cmd_update()
echo 2 Skipping unmerged submodule $prefix$sm_path
continue
fi
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
url=$(git config submodule.$name.url)
branch=$(get_submodule_config $name branch master)
if ! test -z $update
@@ -1022,7 +1004,7 @@ cmd_summary() {
# Respect the ignore setting for --for-status.
Re: Draft of Git Rev News edition 6
On Mon, Aug 3, 2015 at 10:49 PM, Thomas Ferris Nicolaisen tfn...@gmail.com wrote: I hope we can attract more contributors in the future, so the weight of this doesn't lie too much on his shoulders. Perhaps we should send out the draft earlier next time, and beckon for more contributions from the list. Just to follow up on this point: The draft for the next edition is available here (still empty): https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-7.md Suggestions and comments can go here: https://github.com/git/git.github.io/issues/94 We could also add a call-for-help at the bottom of the respective section, asking people who are trawling the mailing list to contribute. You may have spotted that I added this in the recently published edition, but I think it can bear repeating here for git@vger: Before we move on to the link section of this edition, we want to make a call-to-arms for people reading the Git mailing list. We didn't get enough material in this edition on what was really discussed on the list the last month. In order to to be the window into the Git community that we set out to be, we need your help. So please, the next time you read through an interesting discussion on the mailing list, note down a few lines about it and share them with us. So, thanks in advance for the contributions we get for #7 and later editions! -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] builtin/mv: remove get_pathspec()
On Thu, Aug 6, 2015 at 1:18 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Thu, Aug 6, 2015 at 2:58 PM, Stefan Beller sbel...@google.com wrote: On Thu, Aug 6, 2015 at 11:46 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller sbel...@google.com wrote: `get_pathspec` is deprecated and builtin/mv.c is its last caller, so reimplement `get_pathspec` literally in builtin/mv.c Curious. Since this is just moving code around, rather than doing the actual work to complete the final step as stated by the NEEDSWORK comment, isn't it just moving the problem from one location to another? Is it worth the code churn? Yeah it is moving around the problem a bit. And the code churn is unfortunate. Though when I was reading the documentation on pathspecs, literally the first sentence was Do not use get_pathspec, it is out dated. And that was a sad taste for reading documentation. By loudly warning you about deprecation and, more importantly, pointing you at the accepted alternative, this documentation saves you from wasting time (both time spent reading and time spent going down a dead-end path). It would be a sad taste if it warned you quietly only at the end of the documentation or not at all. It's ok to have such warnings in the docs, but as the first sentence as if there was nothing more important than avoiding the out dated stuff? From a documentation standpoint, there is nothing more important than warning you to avoid it since it is outdated and likely to go away in the future. I mean I want to understand the actual code and how I can use it, right? No. It's deprecated and not meant for your use. I need to be more precise: I mean I want to learn how to deal with pathspec things in code. Assume I know nothing of the history of that code and its deprecated earlier attempts. So I would expect a documentation like: foo-bar(int arg): bars the foos with the specific thing equal `arg` foobaz(int a, int b, int c): Note: this is outdated, bars the foos with ... in that order, so if I were to start reading from top to bottom, I would find the current version first and could jump off starting writing code based on foo-bar. If I was reading code and wonder: What does foobaz exactly do? Oh I know! I'll consult the documentation, then I would find the note about it being out dated and preferably its suggested new replacement. However I perceived the documentation I read more like this: some words on foo-bar, a bit too shallow (a good overview, but I'd like to know more details, so keep reading) note on foobaz being outdated more words on foo-bar It's a different matter if you want to understand what the function does because you've encountered a call in existing code, but that case is covered by the existing documentation still being intact (that is, it wasn't removed when the deprecation notice was added). And there are different approaches to solving the problem. I could have just reworded or even just rearranged the documentation. The documentation seems fine as-is. The approach I take here includes a bit of code churn, but it moves the problematic pieces all in one spot. Indeed, I had the localizing the problem to one spot argument in mind, and even wrote it as an answer to my own question, but deleted it before hitting Send. The counterargument (aside from code churn) is, that by leaving it alone, it serves as a good reminder of the problem and is more likely to get noticed and (perhaps) fixed by someone than if it is hidden away in builtin/mv.c. Having code just in one ugly spot however helps people in the other areas, where they can see the shiny new thing in action and don't have the mental burden of seeing a NEEDSWORK comment and thinking about that instead of the feature they want to add there. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file.c: fix a declaration-after-statement
On 06/08/15 10:53, Ramsay Jones wrote:
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
---
Hi Junio,
Sorry for this hit-n-run patch, but I'm in a hurry ... :-D
Could you please squash this (or something like it) into
the relevant patch; Thanks!
Ah, I've just read your 'What's Cooking' email and I see that
you are already aware of this.
Sorry for the noise!
ATB,
Ramsay Jones
[I noticed this simply because I have '-Wdeclaration-after-statement'
and '-Werror' (among others) set in CFLAGS (via config.mak).]
HTH
ATB,
Ramsay Jones
sha1_file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sha1_file.c b/sha1_file.c
index 43386a8..a0af574 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1462,8 +1462,9 @@ int git_open_noatime(const char *name)
static int sha1_file_open_flag = O_NOATIME;
for (;;) {
+ int fd;
errno = 0;
- int fd = open(name, O_RDONLY | sha1_file_open_flag);
+ fd = open(name, O_RDONLY | sha1_file_open_flag);
if (fd = 0)
return fd;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git
Hi Junio, On 2015-08-06 00:55, Junio C Hamano wrote: * sb/submodule-helper (2015-08-05) 1 commit - submodule: implement `module_list` as a builtin helper The beginning of git submodule rewritten in C. I am really looking forward to that, with my Windows performance hat firmly on my head. * tb/complete-rebase-i-edit-todo (2015-08-05) 1 commit - completion: offer '--edit-todo' during interactive rebase Comments? The problem fixed by this one has bugged me plenty of times. I often need to edit the TODO e.g. when I managed to get the reordering wrong and want to insert a `pick commit` later in the `git-rebase-todo` and then call `git rebase --skip`. Read: I am very much in favor of fast-tracking this topic. Thanks, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] clone: do not include authentication data in guessed dir
On Wed, Aug 05, 2015 at 12:41:27PM -0700, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
For completeness, here is what I think the end result (together with
Peff's series) of the test should look like.
...
Note that ssh://user:passw@rd@host:1234/ and user:passw@rd@host:/
tests fail for the same reason (finding @ should be greedy, I think).
And I think this should make it pass. Just remember the last
occurrence of '@' by moving the 'start' every time we see an '@'
sign.
builtin/clone.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index cae288f..5d86439 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -160,13 +160,12 @@ static char *guess_dir_name(const char *repo, int
is_bundle, int is_bare)
start += 3;
/*
- * Skip authentication data.
+ * Skip authentication data, if exists.
*/
- ptr = start;
- while (ptr end !is_dir_sep(*ptr) *ptr != '@')
- ptr++;
- if (*ptr == '@')
- start = ptr + 1;
+ for (ptr = start; ptr end !is_dir_sep(*ptr); ptr++) {
+ if (*ptr == '@')
+ start = ptr + 1;
+ }
/*
* Strip trailing spaces, slashes and /.git
I guess it makes sense to skip over @-signs greedily. Is there
anything I need to do here or will you squash those changes in?
signature.asc
Description: Digital signature
[PATCH] sha1_file.c: fix a declaration-after-statement
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
---
Hi Junio,
Sorry for this hit-n-run patch, but I'm in a hurry ... :-D
Could you please squash this (or something like it) into
the relevant patch; Thanks!
[I noticed this simply because I have '-Wdeclaration-after-statement'
and '-Werror' (among others) set in CFLAGS (via config.mak).]
HTH
ATB,
Ramsay Jones
sha1_file.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sha1_file.c b/sha1_file.c
index 43386a8..a0af574 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1462,8 +1462,9 @@ int git_open_noatime(const char *name)
static int sha1_file_open_flag = O_NOATIME;
for (;;) {
+ int fd;
errno = 0;
- int fd = open(name, O_RDONLY | sha1_file_open_flag);
+ fd = open(name, O_RDONLY | sha1_file_open_flag);
if (fd = 0)
return fd;
--
2.5.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] submodule: implement `module_name` as a builtin helper
This implements the helper `module_name` in C instead of shell,
yielding a nice performance boost.
Before this patch, I measured a time (best out of three):
$ time ./t7400-submodule-basic.sh /dev/null
real0m11.066s
user0m3.348s
sys 0m8.534s
With this patch applied I measured (also best out of three)
$ time ./t7400-submodule-basic.sh /dev/null
real0m10.063s
user0m3.044s
sys 0m7.487s
Helped-by: Jens Lehmann jens.lehm...@web.de
Signed-off-by: Stefan Beller sbel...@google.com
---
This incorporates the changes from Jens fixup! commit
(which addresses all issues he pointed out).
I agree this looks much cleaner. :)
This patch advances origin/sb/submodule-helper (d2c6c09ac8,
submodule: implement `module_list` as a builtin helper)
builtin/submodule--helper.c | 22 ++
git-submodule.sh| 32 +++-
submodule.c | 19 ++-
submodule.h | 1 +
4 files changed, 44 insertions(+), 30 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index cb18ddf..bc37b74 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,8 @@
#include pathspec.h
#include dir.h
#include utf8.h
+#include submodule.h
+#include string-list.h
static char *ps_matched;
static const struct cache_entry **ce_entries;
@@ -98,6 +100,23 @@ static int module_list(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_name(int argc, const char **argv, const char *prefix)
+{
+ const char *name;
+
+ if (argc != 1)
+ usage(git submodule--helper module_name path\n);
+
+ gitmodules_config();
+ name = submodule_name_for_path(argv[0]);
+
+ if (!name)
+ die(No submodule mapping found in .gitmodules for path '%s',
argv[0]);
+
+ printf(%s\n, name);
+ return 0;
+}
+
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
{
if (argc 2)
@@ -106,6 +125,9 @@ int cmd_submodule__helper(int argc, const char **argv,
const char *prefix)
if (!strcmp(argv[1], module_list))
return module_list(argc - 1, argv + 1, prefix);
+ if (!strcmp(argv[1], module_name))
+ return module_name(argc - 2, argv + 2, prefix);
+
usage:
usage(git submodule--helper module_list\n);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index af9ecef..e6ff38d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -178,24 +178,6 @@ get_submodule_config () {
printf '%s' ${value:-$default}
}
-
-#
-# Map submodule path to submodule name
-#
-# $1 = path
-#
-module_name()
-{
- # Do we have submodule.something.path = $1 defined in .gitmodules
file?
- sm_path=$1
- re=$(printf '%s\n' $1 | sed -e 's/[].[^$\\*]/\\/g')
- name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
- sed -n -e 's|^submodule\.\(.*\)\.path '$re'$|\1|p' )
- test -z $name
- die $(eval_gettext No submodule mapping found in .gitmodules for path
'\$sm_path')
- printf '%s\n' $name
-}
-
#
# Clone a submodule
#
@@ -498,7 +480,7 @@ cmd_foreach()
then
displaypath=$(relative_path $sm_path)
say $(eval_gettext Entering '\$prefix\$displaypath')
- name=$(module_name $sm_path)
+ name=$(git submodule--helper module_name $sm_path)
(
prefix=$prefix$sm_path/
clear_local_git_env
@@ -554,7 +536,7 @@ cmd_init()
while read mode sha1 stage sm_path
do
die_if_unmatched $mode
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
displaypath=$(relative_path $sm_path)
@@ -636,7 +618,7 @@ cmd_deinit()
while read mode sha1 stage sm_path
do
die_if_unmatched $mode
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
displaypath=$(relative_path $sm_path)
@@ -758,7 +740,7 @@ cmd_update()
echo 2 Skipping unmerged submodule $prefix$sm_path
continue
fi
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
url=$(git config submodule.$name.url)
branch=$(get_submodule_config $name branch master)
if ! test -z $update
@@ -1022,7 +1004,7 @@ cmd_summary() {
# Respect the ignore setting for --for-status.
if test -n $for_status
then
-
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
I guess the idea here is that you intend state-output to be re-used
and it is convenient to clear it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
it seems like a smarter way to around this without much overhead But it
was more of to release it as its no longer required unless another modifier
atom
is encountered. Is it worth keeping hoping for another modifier atom
eventually,
and release it at the end like you suggested below?
If I understand your question correctly, it sounds like you're asking
about a memory micro-optimization. From an architectural standpoint,
it's cleaner for the entity which allocates a resource to also release
it. In this case, show_ref_array_item() allocates the strbuf, thus it
should be the one to release it.
And, although we shouldn't be worrying about micro-optimizations at
this point, if it were to be an issue, resetting the strbuf via
strbuf_reset(), which doesn't involve slow memory
deallocation/reallocation, is likely to be a winner over repeated
strbuf_release().
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
This looks neater, thanks. It'll go along with the previous patch.
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
Yea the memset is needed for bit fields evnetually added in the future.
Perhaps move the memset() to the first patch which actually requires
it, where it won't be (effectively) dead code, as it becomes here once
you make the above change.
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
What is this change about?
To remove the warning about atomv being unassigned before usage.
Hmm, where were you seeing that warning? The first use of 'atomv'
following its declaration is in the get_ref_atom_value() below, and
(as far as the compiler knows) that should be setting its value.
ep = strchr(sp, ')');
- if (cp sp)
- emit(cp, sp, output);
+ if (cp sp) {
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- print_value(atomv, quote_style, output);
+ process_formatting_state(atomv, state);
+ print_value(atomv, state);
+ apply_formatting_state(state, final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, output);
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
perhaps set_formatting_state()?
I don't know. I don't have a proper high-level overview of the
functionality yet to say if that is a good name or not (which is one
reason I didn't suggest an alternative).
print_value() and emit() both imply outputting something, but neither
does so anymore.
I think I'll append a to_state to each of them.
Meh. print_value() might be better named format_value(). emit() might
become append_literal() or append_non_atom() or something.
apply_formatting_state() seems to be more about finalizing the
already-formatted output.
perform_state_formatting()? perhaps.
Dunno.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More
Re: [PATCH v9 01/11] ref-filter: print output to strbuf for formatting
On Fri, Aug 7, 2015 at 3:51 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote: Introduce a strbuf `output` which will act as a substitute rather than printing directly to stdout. This will be used for formatting eventually. Signed-off-by: Karthik Nayak karthik@gmail.com --- diff --git a/ref-filter.c b/ref-filter.c index 46963a5..91482c9 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu if (color_parse(reset, color) 0) die(BUG: couldn't parse 'reset' as a color); resetv.s = color; - print_value(resetv, quote_style); + print_value(resetv, quote_style, output); } + for (i = 0; i output.len; i++) + printf(%c, output.buf[i]); Everything up to this point seems straightforward, however, it's not clear why you need to emit 'output' one character at a time. Is it because it might contain a NUL '\0' character and therefore you can't use the simpler printf(%s, output.buf)? If that's the case, then why not just use fwrite() to emit it all at once? fwrite(output.buf, output.len, 1, stdout); It was to avoid the printing to stop at '\0' as you mentioned. I've never come across such a situation before, so I looked for similar implementations online, and found the individual character printing. Thanks `fwrite` seems neater. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
Introduce a ref_formatting_state which will eventually hold the values
of modifier atoms. Implement this within ref-filter.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/ref-filter.c b/ref-filter.c
index 91482c9..2c074a1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1238,35 +1238,58 @@ static void emit(const char *cp, const char *ep,
struct strbuf *output)
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
I forgot to mention this in my earlier review of this patch:
s/evetually/eventually/
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
Introduce a ref_formatting_state which will eventually hold the values
of modifier atoms. Implement this within ref-filter.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
I guess the idea here is that you intend state-output to be re-used
and it is convenient to clear it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
[1]: http://article.gmane.org/gmane.comp.version-control.git/273094
it seems like a smarter way to around this without much overhead But it
was more of to release it as its no longer required unless another modifier atom
is encountered. Is it worth keeping hoping for another modifier atom eventually,
and release it at the end like you suggested below?
+}
+
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
- struct strbuf output = STRBUF_INIT;
+ struct strbuf value = STRBUF_INIT;
+ struct strbuf final_buf = STRBUF_INIT;
+ struct ref_formatting_state state;
int i;
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
This looks neater, thanks. It'll go along with the previous patch.
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
Yea the memset is needed for bit fields evnetually added in the future.
This still allows re-use via strbuf_reset() mentioned above.
And, of course, you'd want to strbuf_release() it at the end of this
function where you're already releasing final_buf.
Addressed this above.
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
What is this change about?
To remove the warning about atomv being unassigned before usage.
ep = strchr(sp, ')');
- if (cp sp)
- emit(cp, sp, output);
+ if (cp sp) {
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- print_value(atomv, quote_style, output);
+ process_formatting_state(atomv, state);
+ print_value(atomv, state);
+ apply_formatting_state(state, final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, output);
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
perhaps set_formatting_state()?
print_value() and emit() both imply outputting something, but neither
does so anymore.
I think I'll append a to_state to each of them.
apply_formatting_state() seems to be more about finalizing the
already-formatted output.
perform_state_formatting()? perhaps.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote: Implement an `align` atom which will act as a modifier atom and align any string with or without an %(atom) appearing before a %(end) atom to the right, left or middle. It is followed by `:type,paddinglength`, where the `type` is either left, right or middle and `paddinglength` is the total length of the padding to be performed. If the atom length is more than the padding length then no padding is performed. e.g. to pad a succeeding atom to the middle with a total padding size of 40 we can do a --format=%(align:middle,40).. Add documentation and tests for the same. I forgot to mention in my earlier review of this patch that you should explain in the commit message, and probably the documentation, this this implementation (assuming I'm understanding the code) does not correctly support nested %(foo)...%(end) constructs, where %(foo) might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or some as yet unimagined modifier. Supporting nesting of these constructs will require pushing the formatting states onto a stack (or invoking the parser recursively). Signed-off-by: Karthik Nayak karthik@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote:
Implement an `align` atom which will act as a modifier atom and align
any string with or without an %(atom) appearing before a %(end) atom
to the right, left or middle.
For someone not familiar with the evolution of this patch series,
align any string with or without an %(atom) is confusing and
distracting and seems to imply that something extra mysterious is
going on behind the scenes. Keeping it simple makes it easier to
understand:
Add an `align` atom which left-, middle-, or right-aligns the
content between %(align:...) and %(end).
It is followed by `:type,paddinglength`, where the `type` is
'type' may not be the best name. Perhaps 'style' or 'position' or
something else would be better?
either left, right or middle and `paddinglength` is the total length
'paddinglength' is misleading. You're really describing the container
width in which alignment takes places, so perhaps call it 'width' or
'fieldwidth' or something.
of the padding to be performed. If the atom length is more than the
padding length then no padding is performed. e.g. to pad a succeeding
atom to the middle with a total padding size of 40 we can do a
It's odd to have alignment described in terms of padding and
padding length, especially in the case of center alignment. It
might be better to rephrase the discussion in terms of field width or
such.
--format=%(align:middle,40)..
I may have missed the discussion, but why was comma chosen as a
separator here, rather than, say, colon? For instance:
%(align:middle:40)
Add documentation and tests for the same.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index e49d578..d865f98 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,14 @@ color::
+align::
+ Align any string with or without %(atom) before the %(end)
+ atom to the right, left or middle. Followed by
Ditto regarding any string with or without %(atom) being confusing
to someone not familiar with the evolution of this patch series.
Instead, perhaps:
Left-, middle-, or right-align content between %(align:...)
and %(end).
+ `:type,paddinglength`, where the `type` is either left,
+ right or middle and `paddinglength` is the total length of
+ the padding to be performed. If the string length is more than
+ the padding length then no padding is performed.
Ditto regarding above observations about 'type', 'paddinglength', and padding.
diff --git a/ref-filter.c b/ref-filter.c
index 2c074a1..d123299 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
const char *name = used_atom[i];
struct atom_value *v = ref-value[i];
int deref = 0;
- const char *refname;
+ const char *refname = NULL;
What is this change about?
const char *formatp;
struct branch *branch = NULL;
@@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
else
v-s = ;
continue;
+ } else if (starts_with(name, align:)) {
+ const char *valp = NULL;
Unnecessary NULL assignment.
+ struct align *align = xmalloc(sizeof(struct align));
+
+ skip_prefix(name, align:, valp);
You could simplify the code by combining this skip_prefix() with the
earlier starts_with() in the conditional:
} else if (skip_prefix(name, align:, valp)) {
struct align *align = xmalloc(sizeof(struct align));
...
+ if (skip_prefix(valp, left,, valp))
+ align-align_type = ALIGN_LEFT;
+ else if (skip_prefix(valp, right,, valp))
+ align-align_type = ALIGN_RIGHT;
+ else if (skip_prefix(valp, middle,, valp))
+ align-align_type = ALIGN_MIDDLE;
+ else
+ die(_(align: improper format));
You could do a better job of helping the user track down the offending
improper format by actually including it in the error message.
+ if (strtoul_ui(valp, 10, align-align_value))
+ die(_(align: positive value expected));
Ditto.
+ v-align = align;
+ v-modifier_atom = 1;
+ continue;
+ } else if (starts_with(name, end)) {
+ v-end = 1;
+ v-modifier_atom = 1;
+ continue;
} else
Re: Draft of Git Rev News edition 6
On Fri, Aug 7, 2015 at 12:18 AM, Thomas Ferris Nicolaisen tfn...@gmail.com wrote: On Mon, Aug 3, 2015 at 10:49 PM, Thomas Ferris Nicolaisen tfn...@gmail.com wrote: I hope we can attract more contributors in the future, so the weight of this doesn't lie too much on his shoulders. Perhaps we should send out the draft earlier next time, and beckon for more contributions from the list. Just to follow up on this point: The draft for the next edition is available here (still empty): https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-7.md Suggestions and comments can go here: https://github.com/git/git.github.io/issues/94 We could also add a call-for-help at the bottom of the respective section, asking people who are trawling the mailing list to contribute. You may have spotted that I added this in the recently published edition, but I think it can bear repeating here for git@vger: It is surprisingly difficult to get to the actual post of edition 6 from this thread. The link in the original post is just a 404, and to get to it from the link in this mail, which you might not have sent at all, I had to click like 5 things, and a few of those turned out to be dead ends. I suppose once you know where they are published, it is easy to find, but I did not :). -- Mikael Magnusson -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] notes: add notes.merge option to select default strategy
On Sun, Aug 2, 2015 at 6:10 AM, Jacob Keller jacob.e.kel...@intel.com wrote:
Teach git-notes about a new configuration option notes.merge for
selecting the default notes merge strategy. Document the option in
config.txt and git-notes.txt
Add tests for use of the configuration option. Include a test to ensure
that --strategy correctly overrides the configured setting.
Signed-off-by: Jacob Keller jacob.kel...@gmail.com
---
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 674682b34b83..9c4f8536182f 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -101,7 +101,7 @@ merge::
If conflicts arise and a strategy for automatically resolving
-conflicting notes (see the -s/--strategy option) is not given,
+conflicting notes (see the NOTES MERGE STRATEGIES section) is not given,
the manual resolver is used. This resolver checks out the
conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
and instructs the user to manually resolve the conflicts there.
@@ -183,6 +183,7 @@ OPTIONS
When merging notes, resolve notes conflicts using the given
strategy. The following strategies are recognized: manual
(default), ours, theirs, union and cat_sort_uniq.
+ This option overrides the notes.merge configuration setting.
See the NOTES MERGE STRATEGIES section below for more
information on each notes merge strategy.
These two documentation updates are much easier to digest than the
noisy-diff versions of the previous attempt; and the patch overall is
a more pleasant read than v1.
diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc55439..de0caa00df1b 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -945,6 +955,20 @@ static int get_ref(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int git_notes_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, notes.merge)) {
+ if (!value)
+ return config_error_nonbool(var);
+ if (parse_notes_strategy(value, merge_strategy))
+ return error(Unknown notes merge strategy: %s,
value);
+ else
+ return 0;
A purely subjective stylistic suggestion, which you can freely ignore
if your preference differs:
if (!value)
return ...;
if (parse_notes_strategy(...))
return ...;
return 0;
+ }
+
+ return git_default_config(var, value, cb);
+}
+
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Draft of Git Rev News edition 6
On Fri, Aug 7, 2015 at 1:44 AM, Mikael Magnusson mika...@gmail.com wrote: It is surprisingly difficult to get to the actual post of edition 6 from this thread. The link in the original post is just a 404, and to get to it from the link in this mail, which you might not have sent at all, I had to click like 5 things, and a few of those turned out to be dead ends. I suppose once you know where they are published, it is easy to find, but I did not :). Sorry about this. It's because at the time of publishing, we move the draft into the CMS' (Jekyll) blogging directory, and as we have no nice and easy way of making redirects in that system, the link ends up dead. The current live edition 6 is here: http://git.github.io/rev_news/2015/08/05/edition-6/ The source page is now here: https://github.com/git/git.github.io/blob/master/_posts/2015-08-05-edition-6.markdown You can always find the latest published edition, as well as an archive here: http://git.github.io/rev_news/ We'll make sure to include fresh links in our emails in the future. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
Introduce a ref_formatting_state which will eventually hold the values
of modifier atoms. Implement this within ref-filter.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
I guess the idea here is that you intend state-output to be re-used
and it is convenient to clear it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
[1]: http://article.gmane.org/gmane.comp.version-control.git/273094
+}
+
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
- struct strbuf output = STRBUF_INIT;
+ struct strbuf value = STRBUF_INIT;
+ struct strbuf final_buf = STRBUF_INIT;
+ struct ref_formatting_state state;
int i;
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
This still allows re-use via strbuf_reset() mentioned above.
And, of course, you'd want to strbuf_release() it at the end of this
function where you're already releasing final_buf.
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
What is this change about?
ep = strchr(sp, ')');
- if (cp sp)
- emit(cp, sp, output);
+ if (cp sp) {
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- print_value(atomv, quote_style, output);
+ process_formatting_state(atomv, state);
+ print_value(atomv, state);
+ apply_formatting_state(state, final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, output);
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
print_value() and emit() both imply outputting something, but neither
does so anymore.
apply_formatting_state() seems to be more about finalizing the
already-formatted output.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
wishlist: make it possible to amend commit messages after push to remote
Not for the first time, and probably not for the last, I pushed a commit upstream without adding a link for the bug report as I was meaning to. Or it could have been... - Simple typos. - Broken URLs. - The impossibility of two consecutive commits referring to each other because the older one cannot know what the newer one will be called. - The following morning / 5 minutes / 5 second later thinking of an additional factoid that would've been great to have in the commit message. In general, I find the fact that once a commit has left the building, it goes into your permanent record, and cannot be changed, ever, to be very, very annoying. I get the cryptographic sealing with all the preceding changes, but... Not that I've thought this through... but couldn't there be a bunch of aliases (new SHAs) for a commit? The original one being the master, but as/if the commit message is changed, it could get new SHAs. Sort of separating the real data of the commit, and the metadata? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 2/4] Add a workdispatcher to get work done in parallel
This adds infrastructure code to work a set of tasks from a thread pool.
The whole life cycle of such a thread pool would look like
struct workdispatcher *wd;
struct return_values *rv;
wd = create_workdispatcher(command_for_task, max_parallel_jobs);
for (...) {
prepare(pointer_to_task);
add_task(wd, pointer_to_task);
}
rv = wait_workdispatcher(wd);
Signed-off-by: Stefan Beller sbel...@google.com
---
Makefile | 1 +
workdispatcher.c | 184 +++
workdispatcher.h | 29 +
3 files changed, 214 insertions(+)
create mode 100644 workdispatcher.c
create mode 100644 workdispatcher.h
diff --git a/Makefile b/Makefile
index 6fb7484..2d8803c 100644
--- a/Makefile
+++ b/Makefile
@@ -805,6 +805,7 @@ LIB_OBJS += version.o
LIB_OBJS += versioncmp.o
LIB_OBJS += walker.o
LIB_OBJS += wildmatch.o
+LIB_OBJS += workdispatcher.o
LIB_OBJS += wrapper.o
LIB_OBJS += write_or_die.o
LIB_OBJS += ws.o
diff --git a/workdispatcher.c b/workdispatcher.c
new file mode 100644
index 000..adfedd9
--- /dev/null
+++ b/workdispatcher.c
@@ -0,0 +1,184 @@
+#include cache.h
+#include workdispatcher.h
+
+#ifndef NO_PTHREADS
+#include pthread.h
+#include semaphore.h
+#include stdio.h
+#include unistd.h
+
+#include git-compat-util.h
+struct job_list {
+ void *item;
+ struct job_list *next;
+};
+#endif
+
+struct workdispatcher {
+#ifndef NO_PTHREADS
+ /*
+* To avoid deadlocks always aquire the semaphores with lowest priority
+* first, priorites are in descending order as listed.
+*
+* The `mutex` is a general purpose lock for modifying data in the work
+* dispatcher, such as adding a new task or adding a return value from
+* an already run task.
+*
+* `workingcount` and `freecount` are opposing semaphores, the sum of
+* their values should equal `max_threads` at any time while the `mutex`
+* is available.
+*/
+ sem_t mutex;
+ sem_t workingcount;
+ sem_t freecount;
+
+ pthread_t *threads;
+ unsigned max_threads;
+
+ struct job_list *first;
+ struct job_list *last;
+#endif
+ void *(*function)(void*);
+ struct return_values *ret;
+};
+
+#ifndef NO_PTHREADS
+static unsigned number_cores(void)
+{
+ int count = sysconf(_SC_NPROCESSORS_ONLN);
+ if (count 1) {
+ fprintf(stderr, Number of CPUs online reported %d.
+ Using one core.\n, count);
+ count = 1;
+ }
+ return count;
+}
+
+void *get_task(struct workdispatcher *wd)
+{
+ void *ret;
+ struct job_list *job;
+
+ sem_wait(wd-workingcount);
+ sem_wait(wd-mutex);
+
+ if (!wd-first)
+ die(BUG: internal error with dequeuing jobs for threads);
+ job = wd-first;
+ ret = job-item;
+ wd-first = job-next;
+ if (!wd-first)
+ wd-last = NULL;
+
+ sem_post(wd-freecount);
+ sem_post(wd-mutex);
+
+ free(job);
+ return ret;
+}
+
+void* dispatcher(void *args)
+{
+ struct workdispatcher *wd = args;
+ void *job = get_task(wd);
+ while (job) {
+ void *retvalue = wd-function(job);
+
+ sem_wait(wd-mutex);
+ struct return_values *rv = wd-ret;
+ ALLOC_GROW(rv-ret, rv-count + 1, rv-alloc);
+ wd-ret-ret[rv-count++] = retvalue;
+ sem_post(wd-mutex);
+
+ job = get_task(wd);
+ }
+
+ pthread_exit(0);
+}
+#endif
+
+struct workdispatcher *create_workdispatcher(void *function(void*),
+unsigned max_threads)
+{
+ struct workdispatcher *wd = xmalloc(sizeof(*wd));
+
+#ifndef NO_PTHREADS
+ int i;
+ if (!max_threads)
+ wd-max_threads = number_cores();
+ else
+ wd-max_threads = max_threads;
+
+ sem_init(wd-mutex, 0, 1);
+ sem_init(wd-workingcount, 0, 0);
+ sem_init(wd-freecount, 0, wd-max_threads);
+ wd-threads = xmalloc(wd-max_threads * sizeof(pthread_t));
+
+ for (i = 0; i wd-max_threads; i++)
+ pthread_create(wd-threads[i], 0, dispatcher, wd);
+
+ wd-first = NULL;
+ wd-last = NULL;
+#endif
+ wd-function = function;
+ wd-ret = xmalloc(sizeof(*wd-ret));
+ wd-ret-ret = NULL;
+ wd-ret-count = 0;
+ wd-ret-alloc = 0;
+
+ return wd;
+}
+
+void add_task(struct workdispatcher *wd, void *job)
+{
+#ifndef NO_PTHREADS
+ struct job_list *job_list;
+
+ job_list = xmalloc(sizeof(*job_list));
+ job_list-item = job;
+ job_list-next = NULL;
+
+ sem_wait(wd-freecount);
+ sem_wait(wd-mutex);
+
+ if (!wd-last) {
+ wd-last = job_list;
+ wd-first = wd-last;
+ } else {
+ wd-last-next = job_list;
+
Re: fetching from an hg remote fails with bare git repositories
On Tue, Aug 4, 2015 at 7:03 PM, Mike Hommey m...@glandium.org wrote: Another missing detail is what you're using for mercurial support in git. I would guess https://github.com/felipec/git-remote-hg. Yes. I was going off some outdated information on the web that told me the felipec/git-remote-hg had moved to mainline git. I now see that has been undone. Shameless plug, you may want to give a try to https://github.com/glandium/git-cinnabar. Thanks, I'll keep an eye on the project for support for pushing merge commits. That missing feature is a show-stopper for me right now. Since git-cinnabar is not doing a full hg clone under the hood, is there a chance that it will support shallow clones? (even before the native hg client has this feature...) Anyways, your error looks like what I fixed in 33cae54, which git describe tells me made it to git 2.3.2. Yep, grabbing the latest version of git (2.5.0) solved the problem. Sorry for the bother. Taylor -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] builtin/mv: remove get_pathspec()
On Thu, Aug 6, 2015 at 11:46 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller sbel...@google.com wrote:
builtin/mv: remove get_pathspec()
Misleading. Perhaps rephrase as:
mv: drop dependency upon deprecated get_pathspec
`get_pathspec` is deprecated and builtin/mv.c is its last caller, so
reimplement `get_pathspec` literally in builtin/mv.c
Curious. Since this is just moving code around, rather than doing the
actual work to complete the final step as stated by the NEEDSWORK
comment, isn't it just moving the problem from one location to
another? Is it worth the code churn?
Yeah it is moving around the problem a bit. And the code churn is
unfortunate. Though when I was reading the documentation on
pathspecs, literally the first sentence was Do not use get_pathspec,
it is out dated. And that was a sad taste for reading documentation.
It's ok to have such warnings in the docs, but as the first sentence as if
there was nothing more important than avoiding the out dated stuff? I
mean I want to understand the actual code and how I can use it, right?
And there are different approaches to solving the problem.
I could have just reworded or even just rearranged the documentation.
The approach I take here includes a bit of code churn, but it moves the
problematic pieces all in one spot.
Signed-off-by: Stefan Beller sbel...@google.com
---
diff --git a/builtin/mv.c b/builtin/mv.c
index d1d4316..99e9b3c 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -10,6 +10,7 @@
#include string-list.h
#include parse-options.h
#include submodule.h
+#include pathspec.h
static const char * const builtin_mv_usage[] = {
N_(git mv [options] source... destination),
@@ -20,13 +21,16 @@ static const char * const builtin_mv_usage[] = {
#define KEEP_TRAILING_SLASH 2
static const char **internal_copy_pathspec(const char *prefix,
- const char **pathspec,
+ const char **argv,
What is this change about? It doesn't seem to be related to anything
else in the patch or to its stated purpose, and makes the argument's
purpose less clear, so it's not obvious why it is a good change.
int count, unsigned flags)
{
int i;
+ struct pathspec ps;
const char **result = xmalloc((count + 1) * sizeof(const char *));
- memcpy(result, pathspec, count * sizeof(const char *));
+ memcpy(result, argv, count * sizeof(const char *));
result[count] = NULL;
+
+ /* NEEDSWORK: Move these preprocessing steps into parse_pathspec */
for (i = 0; i count; i++) {
int length = strlen(result[i]);
int to_copy = length;
@@ -42,7 +46,13 @@ static const char **internal_copy_pathspec(const char
*prefix,
result[i] = it;
}
}
- return get_pathspec(prefix, result);
+
+ parse_pathspec(ps,
+ PATHSPEC_ALL_MAGIC
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
+ PATHSPEC_PREFER_CWD,
+ prefix, result);
+ return ps._raw;
}
static const char *add_slash(const char *path)
--
2.5.0.239.g9728e1d.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] argv_array: add argv_array_copy
On Thu, Aug 06, 2015 at 02:18:26PM -0400, Eric Sunshine wrote: However, that begs the question: Why do you need argv_array_copy() at all? Isn't the same functionality already provided by argv_array_pushv()? To wit, a caller which wants to copy from 'src' to 'dst' can already do: struct argv_array src = ...; struct argv_array dst = ARGV_ARRAY_INIT; argv_array_pushv(dst, src-argv); Thanks for reviewing this, Eric. Everything you said is exactly what I was thinking, too, especially this last part. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] argv_array: add argv_array_copy
On Thu, Aug 6, 2015 at 1:35 PM, Stefan Beller sbel...@google.com wrote:
The copied argv array shall be an identical deep copy except for
the internal allocation value.
Signed-off-by: Stefan Beller sbel...@google.com
---
diff --git a/argv-array.c b/argv-array.c
index 256741d..6d9c1dd 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -68,3 +68,16 @@ void argv_array_clear(struct argv_array *array)
+void argv_array_copy(struct argv_array *src, struct argv_array *dst)
'src' should be 'const'.
Typical Unix argument order has 'dst' first and 'src' second, i.e
strcpy(dst, src). Is it worth deviating from that precedent?
+{
+ int i;
+
+ dst-argv = xmalloc((src-argc + 1) * sizeof(*dst-argv));
What happens if 'dst' already has content? Isn't that being leaked
here? At the very least, don't you want to argv_array_clear(dst)?
+ dst-argc = src-argc;
+ dst-alloc = src-argc;
This is wrong, of course. The number allocated is actually argc+1, not argc.
+ for (i = 0; i dst-argc ; i++)
While it's not wrong per se to use dst-argc as the terminating
condition, it is potentially misleading and confusing. Instead using
src-argc as the terminating condition will better telegraph that the
copy process is indeed predicated upon 'src'.
+ dst-argv[i] = xstrdup(src-argv[i]);
+ dst-argv[dst-argc] = NULL;
It's not clear why you want to hand-code the low-level functionality
again (such as array allocation and string duplication), risking (and
indeed making) errors in the process, when you could instead re-use
existing argv_array code. I would have expected to see
argv_array_copy() implemented as:
argv_array_clear(dst);
for (i = 0; i src-argc; i++)
argv_array_push(dst, src-argv[i]);
which provides far fewer opportunities for errors to creep in.
Moreover, this function might be too special-purpose. That is, why
does it need to overwrite 'dst'? Can't you achieve the same
functionality by merely appending to 'dst', and leave it up to the
caller to decide whether 'dst' should be cleared beforehand or not? If
so, then you can drop the argv_array_clear(dst) from the above.
However, that begs the question: Why do you need argv_array_copy() at
all? Isn't the same functionality already provided by
argv_array_pushv()? To wit, a caller which wants to copy from 'src' to
'dst' can already do:
struct argv_array src = ...;
struct argv_array dst = ARGV_ARRAY_INIT;
argv_array_pushv(dst, src-argv);
+}
+
diff --git a/argv-array.h b/argv-array.h
index c65e6e8..247627da 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -19,5 +19,6 @@ LAST_ARG_MUST_BE_NULL
void argv_array_pushl(struct argv_array *, ...);
void argv_array_pop(struct argv_array *);
void argv_array_clear(struct argv_array *);
+void argv_array_copy(struct argv_array *src, struct argv_array *dst);
#endif /* ARGV_ARRAY_H */
--
2.5.0.239.g9728e1d.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 4/4] submodule: add infrastructure to fetch submodules in parallel
This makes use of the new workdispatcher to fetch a number
of submodules at the same time.
Still todo: sort the output of the fetch commands. I am unsure
if this should be hooked into the workdispatcher as the problem
of sorted output will appear likely again, so a general solution
would not hurt.
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/fetch.c | 3 ++-
submodule.c | 74 -
submodule.h | 2 +-
3 files changed, 60 insertions(+), 19 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8d5b2db..9053e8b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1207,7 +1207,8 @@ int cmd_fetch(int argc, const char **argv, const char
*prefix)
result = fetch_populated_submodules(options,
submodule_prefix,
recurse_submodules,
- verbosity 0);
+ verbosity 0,
+ 1);
argv_array_clear(options);
}
diff --git a/submodule.c b/submodule.c
index 872967f..0b2842b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -11,6 +11,7 @@
#include sha1-array.h
#include argv-array.h
#include blob.h
+#include workdispatcher.h
static struct string_list config_name_for_path;
static struct string_list config_fetch_recurse_submodules_for_name;
@@ -696,13 +697,49 @@ const char* submodule_name_for_path(const char* path)
return NULL;
}
+struct submodule_parallel_fetch {
+ struct child_process cp;
+ struct argv_array argv;
+ struct strbuf sb;
+ int quiet;
+};
+
+void submodule_parallel_fetch_init(struct submodule_parallel_fetch *spf)
+{
+ child_process_init(spf-cp);
+ argv_array_init(spf-argv);
+ strbuf_init(spf-sb, 0);
+ spf-quiet = 0;
+}
+
+void *run_command_and_cleanup(void *arg)
+{
+ struct submodule_parallel_fetch *spf = arg;
+ void *ret = NULL;
+
+ if (!spf-quiet)
+ puts(spf-sb.buf);
+
+ spf-cp.argv = spf-argv.argv;
+
+ if (run_command(spf-cp))
+ ret = (void *)1;
+
+ strbuf_release(spf-cp);
+ argv_array_clear(spf-argv);
+ free(spf);
+ return ret;
+}
+
int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
- int quiet)
+ int quiet, int max_parallel_jobs)
{
int i, result = 0;
- struct child_process cp = CHILD_PROCESS_INIT;
+ struct workdispatcher *wd;
+ struct return_values *rv;
struct argv_array argv = ARGV_ARRAY_INIT;
+ struct submodule_parallel_fetch *spf;
const char *name_for_path;
const char *work_tree = get_git_work_tree();
if (!work_tree)
@@ -717,12 +754,9 @@ int fetch_populated_submodules(const struct argv_array
*options,
argv_array_push(argv, --recurse-submodules-default);
/* default value, --submodule-prefix and its value are added later */
- cp.env = local_repo_env;
- cp.git_cmd = 1;
- cp.no_stdin = 1;
-
calculate_changed_submodule_paths();
+ wd = create_workdispatcher(run_command_and_cleanup, max_parallel_jobs);
for (i = 0; i active_nr; i++) {
struct strbuf submodule_path = STRBUF_INIT;
struct strbuf submodule_git_dir = STRBUF_INIT;
@@ -771,24 +805,30 @@ int fetch_populated_submodules(const struct argv_array
*options,
if (!git_dir)
git_dir = submodule_git_dir.buf;
if (is_directory(git_dir)) {
+ spf = xmalloc(sizeof(*spf));
+ submodule_parallel_fetch_init(spf);
+ spf-cp.env = local_repo_env;
+ spf-cp.git_cmd = 1;
+ spf-cp.no_stdin = 1;
+ spf-cp.dir = strbuf_detach(submodule_path, NULL);
+ spf-quiet = quiet;
if (!quiet)
- printf(Fetching submodule %s%s\n, prefix,
ce-name);
- cp.dir = submodule_path.buf;
- argv_array_push(argv, default_argv);
- argv_array_push(argv, --submodule-prefix);
- argv_array_push(argv, submodule_prefix.buf);
- cp.argv = argv.argv;
- if (run_command(cp))
- result = 1;
- argv_array_pop(argv);
- argv_array_pop(argv);
- argv_array_pop(argv);
+ strbuf_addf(spf-sb, Fetching submodule
%s%s, prefix, ce-name);
+ argv_array_copy(argv, spf-argv);
[RFC/PATCH 0/4] parallel fetch for submodules
When I was looking at the branches of Jens for work done on submodules not yet upstream I found a commit WIP threaded submodule fetching[1], and I was side tracked wanting to present a different approach to that. The first patch is a bit unrelated as it relates to the rewrite of git-submodule.sh but also has code in submodule.c and the following patches modify code just around that, so I did not remove that patch from this series. It is the same I sent yesterday. The next patch 2/4 presents a framework for parallel threaded work. It allows to setup a worker pool of n threads and then have a queue of tasks which are worked on by the threads. The patch is a the one which I'd request most comments on as I think that can be reused in a variety of situations (parallel checkout of files, parallel fetch of different remotes, or such). I consider the third patch farely boring as it adds argv_array_copy, so I would not expect much discussion there. The last patch 4/4 presents the new workdispatcher from 2/4 in use with just one unsolved problem of how to handle the output of the parallel commands to stdout and stderr. It may be useful to put handling of parallel outputs into the work dispatcher. [1] https://github.com/jlehmann/git-submod-enhancements/commit/47597753206d40e234a47392e258065c9489e2b3 This series applies on top of origin/sb/submodule-helper (d2c6c09ac819, submodule: implement `module_list` as a builtin helper) and can also be found at https://github.com/stefanbeller/git/tree/parallel-submodule-fetch Stefan Beller (4): submodule: implement `module_name` as a builtin helper Add a workdispatcher to get work done in parallel argv_array: add argv_array_clone to clone an existing argv array submodule: add infrastructure to fetch submodules in parallel Makefile| 1 + argv-array.c| 13 argv-array.h| 1 + builtin/fetch.c | 3 +- builtin/submodule--helper.c | 23 ++ git-submodule.sh| 32 ++-- submodule.c | 92 -- submodule.h | 3 +- workdispatcher.c| 184 workdispatcher.h| 29 +++ 10 files changed, 332 insertions(+), 49 deletions(-) create mode 100644 workdispatcher.c create mode 100644 workdispatcher.h -- 2.5.0.239.g9728e1d.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] argv_array: add argv_array_copy
The copied argv array shall be an identical deep copy except for
the internal allocation value.
CC: Jeff King p...@peff.net
Signed-off-by: Stefan Beller sbel...@google.com
---
argv-array.c | 13 +
argv-array.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/argv-array.c b/argv-array.c
index 256741d..6d9c1dd 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -68,3 +68,16 @@ void argv_array_clear(struct argv_array *array)
}
argv_array_init(array);
}
+
+void argv_array_copy(struct argv_array *src, struct argv_array *dst)
+{
+ int i;
+
+ dst-argv = xmalloc((src-argc + 1) * sizeof(*dst-argv));
+ dst-argc = src-argc;
+ dst-alloc = src-argc;
+ for (i = 0; i dst-argc ; i++)
+ dst-argv[i] = xstrdup(src-argv[i]);
+ dst-argv[dst-argc] = NULL;
+}
+
diff --git a/argv-array.h b/argv-array.h
index c65e6e8..247627da 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -19,5 +19,6 @@ LAST_ARG_MUST_BE_NULL
void argv_array_pushl(struct argv_array *, ...);
void argv_array_pop(struct argv_array *);
void argv_array_clear(struct argv_array *);
+void argv_array_copy(struct argv_array *src, struct argv_array *dst);
#endif /* ARGV_ARRAY_H */
--
2.5.0.239.g9728e1d.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] submodule: implement `module_name` as a builtin helper
This implements the helper `module_name` in C instead of shell,
yielding a nice performance boost.
Before this patch, I measured a time (best out of three):
$ time ./t7400-submodule-basic.sh /dev/null
real0m11.066s
user0m3.348s
sys 0m8.534s
With this patch applied I measured (also best out of three)
$ time ./t7400-submodule-basic.sh /dev/null
real0m10.063s
user0m3.044s
sys 0m7.487s
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/submodule--helper.c | 23 +++
git-submodule.sh| 32 +++-
submodule.c | 18 +-
submodule.h | 1 +
4 files changed, 44 insertions(+), 30 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index cb18ddf..3713c4c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,8 @@
#include pathspec.h
#include dir.h
#include utf8.h
+#include submodule.h
+#include string-list.h
static char *ps_matched;
static const struct cache_entry **ce_entries;
@@ -98,6 +100,24 @@ static int module_list(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_name(int argc, const char **argv, const char *prefix)
+{
+ const char *name;
+
+ if (argc != 1)
+ usage(git submodule--helper module_name path\n);
+
+ gitmodules_config();
+ name = submodule_name_for_path(argv[0]);
+
+ if (name)
+ printf(%s\n, name);
+ else
+ die(No submodule mapping found in .gitmodules for path '%s',
argv[0]);
+
+ return 0;
+}
+
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
{
if (argc 2)
@@ -106,6 +126,9 @@ int cmd_submodule__helper(int argc, const char **argv,
const char *prefix)
if (!strcmp(argv[1], module_list))
return module_list(argc - 1, argv + 1, prefix);
+ if (!strcmp(argv[1], module_name))
+ return module_name(argc - 2, argv + 2, prefix);
+
usage:
usage(git submodule--helper module_list\n);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index af9ecef..e6ff38d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -178,24 +178,6 @@ get_submodule_config () {
printf '%s' ${value:-$default}
}
-
-#
-# Map submodule path to submodule name
-#
-# $1 = path
-#
-module_name()
-{
- # Do we have submodule.something.path = $1 defined in .gitmodules
file?
- sm_path=$1
- re=$(printf '%s\n' $1 | sed -e 's/[].[^$\\*]/\\/g')
- name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
- sed -n -e 's|^submodule\.\(.*\)\.path '$re'$|\1|p' )
- test -z $name
- die $(eval_gettext No submodule mapping found in .gitmodules for path
'\$sm_path')
- printf '%s\n' $name
-}
-
#
# Clone a submodule
#
@@ -498,7 +480,7 @@ cmd_foreach()
then
displaypath=$(relative_path $sm_path)
say $(eval_gettext Entering '\$prefix\$displaypath')
- name=$(module_name $sm_path)
+ name=$(git submodule--helper module_name $sm_path)
(
prefix=$prefix$sm_path/
clear_local_git_env
@@ -554,7 +536,7 @@ cmd_init()
while read mode sha1 stage sm_path
do
die_if_unmatched $mode
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
displaypath=$(relative_path $sm_path)
@@ -636,7 +618,7 @@ cmd_deinit()
while read mode sha1 stage sm_path
do
die_if_unmatched $mode
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
displaypath=$(relative_path $sm_path)
@@ -758,7 +740,7 @@ cmd_update()
echo 2 Skipping unmerged submodule $prefix$sm_path
continue
fi
- name=$(module_name $sm_path) || exit
+ name=$(git submodule--helper module_name $sm_path) || exit
url=$(git config submodule.$name.url)
branch=$(get_submodule_config $name branch master)
if ! test -z $update
@@ -1022,7 +1004,7 @@ cmd_summary() {
# Respect the ignore setting for --for-status.
if test -n $for_status
then
- name=$(module_name $sm_path)
+ name=$(git submodule--helper module_name
$sm_path)
ignore_config=$(get_submodule_config $name
ignore none)
test $status != A test $ignore_config = all
continue
Re: [PATCH 1/2] builtin/mv: remove get_pathspec()
On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller sbel...@google.com wrote:
builtin/mv: remove get_pathspec()
Misleading. Perhaps rephrase as:
mv: drop dependency upon deprecated get_pathspec
`get_pathspec` is deprecated and builtin/mv.c is its last caller, so
reimplement `get_pathspec` literally in builtin/mv.c
Curious. Since this is just moving code around, rather than doing the
actual work to complete the final step as stated by the NEEDSWORK
comment, isn't it just moving the problem from one location to
another? Is it worth the code churn?
Signed-off-by: Stefan Beller sbel...@google.com
---
diff --git a/builtin/mv.c b/builtin/mv.c
index d1d4316..99e9b3c 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -10,6 +10,7 @@
#include string-list.h
#include parse-options.h
#include submodule.h
+#include pathspec.h
static const char * const builtin_mv_usage[] = {
N_(git mv [options] source... destination),
@@ -20,13 +21,16 @@ static const char * const builtin_mv_usage[] = {
#define KEEP_TRAILING_SLASH 2
static const char **internal_copy_pathspec(const char *prefix,
- const char **pathspec,
+ const char **argv,
What is this change about? It doesn't seem to be related to anything
else in the patch or to its stated purpose, and makes the argument's
purpose less clear, so it's not obvious why it is a good change.
int count, unsigned flags)
{
int i;
+ struct pathspec ps;
const char **result = xmalloc((count + 1) * sizeof(const char *));
- memcpy(result, pathspec, count * sizeof(const char *));
+ memcpy(result, argv, count * sizeof(const char *));
result[count] = NULL;
+
+ /* NEEDSWORK: Move these preprocessing steps into parse_pathspec */
for (i = 0; i count; i++) {
int length = strlen(result[i]);
int to_copy = length;
@@ -42,7 +46,13 @@ static const char **internal_copy_pathspec(const char
*prefix,
result[i] = it;
}
}
- return get_pathspec(prefix, result);
+
+ parse_pathspec(ps,
+ PATHSPEC_ALL_MAGIC
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
+ PATHSPEC_PREFER_CWD,
+ prefix, result);
+ return ps._raw;
}
static const char *add_slash(const char *path)
--
2.5.0.239.g9728e1d.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] pathspec: remove deprecated get_pathspec
The function `get_pathspec` is no longer used, so remove it.
The NEEDSWORK comment in pathspec.c is outdated as that happened in
(fadf96aba, 2013-09-09, Merge branch 'nd/magic-pathspec')
Signed-off-by: Stefan Beller sbel...@google.com
---
Documentation/technical/api-setup.txt | 2 --
cache.h | 1 -
pathspec.c| 36 ---
pathspec.h| 2 +-
4 files changed, 1 insertion(+), 40 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 4f55466..d4e22e2 100644
--- a/cache.h
+++ b/cache.h
@@ -452,7 +452,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 9304ee3..abaec0a 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -94,12 +94,6 @@ static void prefix_short_magic(struct strbuf *sb, int
prefixlen,
*
* 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.
*/
static unsigned prefix_pathspec(struct pathspec_item *item,
unsigned *p_short_magic,
@@ -450,36 +444,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 list of
- * paths to process, all relative to the root of the working tree.
- */
-const char **get_pathspec(const char *prefix, const char **pathspec)
-{
- struct pathspec ps;
- parse_pathspec(ps,
- PATHSPEC_ALL_MAGIC
- ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
- PATHSPEC_PREFER_CWD,
- prefix, pathspec);
- return ps._raw;
-}
-
void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
{
*dst = *src;
diff --git a/pathspec.h b/pathspec.h
index 0c11262..b345df6 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -19,7 +19,7 @@
#define PATHSPEC_ONESTAR 1 /* the pathspec pattern satisfies GFNM_ONESTAR
*/
struct pathspec {
- const char **_raw; /* get_pathspec() result, not freed by
free_pathspec() */
+ const char **_raw; /* not freed by free_pathspec() */
int nr;
unsigned int has_wildcard:1;
unsigned int recursive:1;
--
2.5.0.239.g9728e1d.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Remove get_pathspec finally
Remove the last caller of the get_pathspec function and the get_pathspec function itself. I stumbled into this as I was reading the documentation on pathspec, and the first sentence get_pathspec() is obsolete and should never be used in new code. made me wonder. This replaces sb/remove-get-pathspec Stefan Beller (2): builtin/mv: remove get_pathspec() pathspec: remove deprecated get_pathspec Documentation/technical/api-setup.txt | 2 -- builtin/mv.c | 16 +--- cache.h | 1 - pathspec.c| 36 --- pathspec.h| 2 +- 5 files changed, 14 insertions(+), 43 deletions(-) -- 2.5.0.239.g9728e1d.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] builtin/mv: remove get_pathspec()
`get_pathspec` is deprecated and builtin/mv.c is its last caller, so
reimplement `get_pathspec` literally in builtin/mv.c
Signed-off-by: Stefan Beller sbel...@google.com
---
builtin/mv.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index d1d4316..99e9b3c 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -10,6 +10,7 @@
#include string-list.h
#include parse-options.h
#include submodule.h
+#include pathspec.h
static const char * const builtin_mv_usage[] = {
N_(git mv [options] source... destination),
@@ -20,13 +21,16 @@ static const char * const builtin_mv_usage[] = {
#define KEEP_TRAILING_SLASH 2
static const char **internal_copy_pathspec(const char *prefix,
- const char **pathspec,
+ const char **argv,
int count, unsigned flags)
{
int i;
+ struct pathspec ps;
const char **result = xmalloc((count + 1) * sizeof(const char *));
- memcpy(result, pathspec, count * sizeof(const char *));
+ memcpy(result, argv, count * sizeof(const char *));
result[count] = NULL;
+
+ /* NEEDSWORK: Move these preprocessing steps into parse_pathspec */
for (i = 0; i count; i++) {
int length = strlen(result[i]);
int to_copy = length;
@@ -42,7 +46,13 @@ static const char **internal_copy_pathspec(const char
*prefix,
result[i] = it;
}
}
- return get_pathspec(prefix, result);
+
+ parse_pathspec(ps,
+ PATHSPEC_ALL_MAGIC
+ ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
+ PATHSPEC_PREFER_CWD,
+ prefix, result);
+ return ps._raw;
}
static const char *add_slash(const char *path)
--
2.5.0.239.g9728e1d.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] gitweb: Don't pass --full-history to git-log(1)
Junio C Hamano gits...@pobox.com writes: Heh, in 2008 we already had more than a few dozen. I think (1) It is perfectly OK to add an UI option to let the web visitor choose between simplified and full history at runtime, optionally with a new gitweb.conf option to let the project owner choose which one is the default; (2) It is also OK to add gitweb.conf option to let the project/site choose between the two, optionally allowing the web visitor to override it with something like (1). Anything else would not give the same out-of-the-box experience and would probably not fly well. Just to make sure, would not fly well is not an outright rejection (given enough thrust, even a pig could fly). An alternative with a bit more friction may be to do a variant of (2), without UI but only with gitweb.conf tweakability, _and_ flipped default. That will break the out-of-the-box experience but I suspect that many people would not notice (because their history is linear), some people who do notice would like the change, and the remainder can tweak their installation back to the full-history version, as long the change of the default is prominently advertised. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Error when cloning with weird local directory
Junio C Hamano gits...@pobox.com writes: Torsten Bögershausen tbo...@web.de writes: It looks as if static char *get_repo_path(const char *repo, int *is_bundle) in built/clone.c checks if there is a local directory structure looking like a .git directory. This is wrong. It is as designed, though, to allow cloning from a local directory with any name. There should be a check for the scheme first. That will be wrong. It matters mostly when dealing with scp-like syntax, word:path. I _think_ taking notice of word:// (with doubled slashes) and treating it specially will not introduce any new issue; while it is still OK for users to have a local directory called word:, if they meant a subdirectory of it, they wouldn't have typed double-slashes there. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Error when cloning with weird local directory
Torsten Bögershausen tbo...@web.de writes: It looks as if static char *get_repo_path(const char *repo, int *is_bundle) in built/clone.c checks if there is a local directory structure looking like a .git directory. This is wrong. It is as designed, though, to allow cloning from a local directory with any name. There should be a check for the scheme first. That will be wrong. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/6] fix repo name when cloning a server's root
On 2015-08-05 23.19, Jeff King wrote: On Wed, Aug 05, 2015 at 10:34:34AM -0700, Junio C Hamano wrote: As you can see, there is a lot of complexity in there and I'm not convinced this is better than just exposing 'parse_connect_url()', which already handles everything for us. I try expose and use parse_connect_url(): It handles the scp-like syntax host:/path, literall IPV6 addresses, port numbers, ':' without a port number and all other Git specific parsing, which is inside and outside the RFC 3986. (I should know, because I managed to break the parser twice, and fix it) I added a diagnostics to connect.c, and if you run the a simply test, we can see that the colon slash logic is often unsufficient: tb@mypc:~/projects/git/tb.150731_connect ./git fetch-pack --diag-url ssh://host/ Diag: url=ssh://host/ Diag: protocol=ssh Diag: userandhost=host Diag: port=NONE Diag: path=/ Diag: guesseddir=host/ tb@macce:~/projects/git/tb.150731_connect ./git fetch-pack --diag-url ssh://host:/ Diag: url=ssh://host:/ Diag: protocol=ssh Diag: userandhost=host Diag: port=NONE Diag: path=/ Diag: guesseddir=/ On top of that, you can easily write test cases in t5601, as many as you want. The (minor) drawback is that it doesn't handle http:// or https://, but that is easy to add in the parser, and doesn't break existing code. The major which remains is to search for '@' in userandhost, and strip that off. (Or when there is a '@', search for a ':' before the '@', and strip that off) After that, all non-printable characters should be %-escaped. If we replace ':' as non-printable as well, we can make Windows users 1% more happy. If the function handles everything for us, that's fine, but the primary reason I am hesitant is because parse_connect_url() was designed specifically not to have to worry about some protocols (e.g. I think feeding it a http://; would fail, and more importantly, its current callers want such a call to fail). Also it is meant to handle some non-protocols (e.g. scp style host:path that does not follow scheme://...). True, but the transport code _is_ handling that at some point. It makes me wonder if it would be possible to push the call to transport_get further up inside cmd_clone(), and then provide some way to query the remote path and hostname from the transport code. Then guess_dir_name could just go away entirely, in favor of something like: dir_name = transport_get_path(transport); if (!*dir_name) dir_name = transport_get_host(transport); That may be overly simplistic or unworkable, though. I haven't dug into the code. Also does it handle the case above? I do not think parse_connect_url() even calls get_host_and_port() to be able to tell what means in these examples. Speaking of which, has anyone tested whether the old or new code handles external remote helpers? Certainly: foo::https://host/repo.git should still use repo.git. But technically the string handed to git-remote-foo does not have to look anything like a URL. In those cases neither guess_dir_name nor the transport code have any idea what anything to the right of the :: means; we probably have to resort to blind guessing based on characters like colon and slash. It is easy to strip the foo:: part of the url, assume that the remote helper uses a RFC 3986 similar url syntax, so that we can feed the reminding https://host/repo.git into the parser (see above). If the remote helper doesn't do this, we can't guess anything, can we ? So error out and tell the user seems the right thing to do. In the hope that this is useful, pushed my prototype branch to https://github.com/tboegi/git/tree/150731_connect_diag_guess_name -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/6] fix repo name when cloning a server's root
Torsten Bögershausen tbo...@web.de writes: It is easy to strip the foo:: part of the url, assume that the remote helper uses a RFC 3986 similar url syntax, so that we can feed the reminding https://host/repo.git into the parser (see above). The thing that worries me is that foo:: syntax and external helper interface was invented by Daniel Barkalow primarily because he wanted to allow things that do *not* fit in that URL-like scheme, for example see: http://thread.gmane.org/gmane.comp.version-control.git/125374/focus=125405 If the remote helper doesn't do this, we can't guess anything, can we ? So error out and tell the user seems the right thing to do. Yes. A blind guess that fails spectacularly is far worse than an outright rejection that is cautious. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] contrib: teach completion about git-worktree options and arguments
On Thu, Jul 23, 2015 at 4:49 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
Complete subcommands 'add' and 'prune', as well as their respective
options --force, --detach, --dry-run, --verbose, and --expire. Also
complete 'refname' in git worktree add [-b newbranch] path
refname.
Ping[1]?
[1]: http://article.gmane.org/gmane.comp.version-control.git/274526
Signed-off-by: Eric Sunshine sunsh...@sunshineco.com
---
This is RFC since this is my first foray into the Git completion script,
and there are likely better and more idiomatic ways to accomplish this.
contrib/completion/git-completion.bash | 32
1 file changed, 32 insertions(+)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index c97c648..07c34ef 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2564,6 +2564,38 @@ _git_whatchanged ()
_git_log
}
+_git_worktree ()
+{
+ local subcommands='add prune'
+ local subcommand=$(__git_find_on_cmdline $subcommands)
+ local c=2 n=0 refpos=2
+ if [ -z $subcommand ]; then
+ __gitcomp $subcommands
+ else
+ case $subcommand,$cur in
+ add,--*)
+ __gitcomp --force --detach
+ ;;
+ add,*)
+ while [ $c -lt $cword ]; do
+ case ${words[c]} in
+ --*) ;;
+ -[bB]) ((refpos++)) ;;
+ *) ((n++)) ;;
+ esac
+ ((c++))
+ done
+ if [ $n -eq $refpos ]; then
+ __gitcomp_nl $(__git_refs)
+ fi
+ ;;
+ prune,--*)
+ __gitcomp --dry-run --verbose --expire
+ ;;
+ esac
+ fi
+}
+
__git_main ()
{
local i c=1 command __git_dir
--
2.5.0.rc3.407.g68aafd0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
