Re: [PATCH] pathspec: remove check_path_for_gitlink
On Thu, May 5, 2016 at 11:21 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> When deep/in/ is an unrelated repository, and running either >> >> git add deep/in/the >> git add deep/in/the/tree >> >> would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did >> >> git add deep/in >> >> I'd lose that and suddenly everything there turns into a submodule. > > Also, I recall that you floated an idea to declare that > > git add deep/in/the/tree/is-a-leaf.txt > > must always behave as if this is done instead: > > git -C deep/in/the/tree/ add is-a-leaf.txt In a different thread, yeah. I am not sure about that any more. > > Even though I am not a huge fan of an operation that crosses module > boundaries, I think that is a sensible semantics of a "cross module > boundary operation" (the actual implementation should not be > iterating over pathspecs and chdir(2)ing around for each and every > one of them, though), if we need "cross module boundary operation" > in order to support end users working on a project with one or more > submodules at the same time. I agree. > > But treating the bug under discussion as a "feature" will destroy > that future. The bug under discussion was blogged about in 2010 and still persists. I'll try to find out if people actually use it. If that bug was fixed, but I still wanted to enjoy the upsides of it, I could to that with pointing core.worktree into deep/in/tree. e.g. I have git.git and gitk-git as separate repositories, then I could still do a GIT_DIR=gitk-git/.git git -C git.git/gitk-git git checkout This looks more complicated than this bug/feature though. There are 2 fundamental cases though. (1) The bug we're talking about (as explained in that blog), refers to 2 independent repositories, whose work trees are nested (2) You seemed to bring in the notion that the nested repo is considered a submodule of the outer repo, i.e. they have a relationship. I don't mind (1). It's a neat hack as these 2 repos are totally unrelated (except for the working tree in the file system being the same files). You could also achieve a similar handling by hardlinking gitk-git/gitk and git.git/gitk-git/gitk. In (2), we have a gitlink, which by definition takes up the whole directory. So any file in that directory in the file system which represents the root of the submodule should belong to the submodule. -- 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 v16 7/7] commit: add a commit.verbose config variable
On Fri, May 6, 2016 at 10:35 AM, Pranit Bauva wrote: > I checked out your branch gitster/pb/commit-verbose-config and tests > from t0040 seem to be failing. Don't worry I will handle those, I will > squash your patch in mine and re-roll it again. I am still unsure how > those tests broke. I will figure it out. > > Thanks for your help! :) False alarm. I had a dirty build. The test suite passes perfectly. Feel free to squash your patch in locally. -- 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] pathspec: remove check_path_for_gitlink
Junio C Hamano writes: > When deep/in/ is an unrelated repository, and running either > > git add deep/in/the > git add deep/in/the/tree > > would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did > > git add deep/in > > I'd lose that and suddenly everything there turns into a submodule. Also, I recall that you floated an idea to declare that git add deep/in/the/tree/is-a-leaf.txt must always behave as if this is done instead: git -C deep/in/the/tree/ add is-a-leaf.txt Even though I am not a huge fan of an operation that crosses module boundaries, I think that is a sensible semantics of a "cross module boundary operation" (the actual implementation should not be iterating over pathspecs and chdir(2)ing around for each and every one of them, though), if we need "cross module boundary operation" in order to support end users working on a project with one or more submodules at the same time. But treating the bug under discussion as a "feature" will destroy that 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: [git-for-windows] [PATCH] exec_cmd.c, sideband.c, Makefile: avoid multiple PREFIX definitions
Am 05.05.2016 um 23:28 schrieb Philip Oakley: The short and sweet PREFIX can be confused when used in many places. Rename both usages to better describe their purpose. Noticed when compiling Git for Windows using MSVC/Visual Studio which reports the conflict beteeen the command line definition and the definition in sideband.c You should describe the circumstances better under which you notice a conflict, because there is no conflict when Git is built with the Makefile and 'make': diff --git a/Makefile b/Makefile index 33b0f76..bcdd3ec 100644 --- a/Makefile +++ b/Makefile @@ -1973,7 +1973,7 @@ exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \ '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \ '-DBINDIR="$(bindir_relative_SQ)"' \ - '-DPREFIX="$(prefix_SQ)"' + '-DEXEC_PREFIX="$(prefix_SQ)"' Notice that PREFIX is set only for a small subset of .c files. sideband.c is not among them. -- Hannes -- 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 11/15] diff: ignore submodules excluded by groups
Junio C Hamano writes: > The set of submodules you "init" to the working tree are the ones > that are interesting to you. So once the tree is populated, you do > not ever have to look at the "defaultGroup" configuration. You just > need to look at the working tree. > ... I forgot to prefix the first few paragraphs of that message with "Here is how my version of the world should work." I did not mean to say "Here is how you must make your work work, or I won't talk to you." I was just hurried as I had to tend to other topics. I actually do not care too deeply (except for the "automatically remove" part, which I do not think we should do), as I do not think there is a big fundamental difference between the two views. To make sure we are on the same page, let me rephrase the two views I have in mind. The difference is what should happen when the user does not give any pathspec, *label, or :name to limit the set of submodules to act on, which, traditionally meant to work on everything, and we are trying to change that to some "default". (1) The default set is what the configuration file says is the default group. The working tree state is ignored. (2) The default set is what the configuration file says is the default group, plus those the user showed interest by doing "submodule init". Suppose that the user has a mostly satisfactory default configured, i.e. the set of submodules the configuration file says is the default is both necessary and sufficient to carry out her daily task. Then there is no difference between the two. Further suppose that the user needs to view a submodule outside the default group temporarily (imagine: for a day or two), while carrying out some specific task. Perhaps she is working on the documentation submodule, which is her primary task hence her configuration file specifies it as the default, but needs to see the submodule that houses the implementation to describe the behaviour. So she does "init code-module/"; this has explicit pathspec, so there is no difference between the two. Now, while reading the code module, she finds a typo in a comment, and wants to fix it. We will start to see differences. * When she wants to get a bird's eye view of everything she cares about at the moment, i.e. wants to view the state of her usual modules plus the code-module she is visiting, (1) is more cumbersome. With (1), "diff --recursive" will not step outside of her configured default, so she says "diff --recursive \*default code-module/" to say "I want to see both my default submodule(s) and the one I checked out by hand". With (2), she does not have to do anything special, as manually checked out code-module/ will be acted upon, in addition to the configured default. * When she wants to see her usual modules ignoring the one-off checkout, (1) is easier. With (1), she can say "diff --recursive" and done. With (2), she needs to say "diff --recursive \*default" to explicitly state "I may have checkouts of other submodules, but this time I want to view only the usual default of mine". The difference is not that big either case. Whichever way we choose to make the default behaviour, the user needs to type a bit extra when asking a behaviour that is different from the default behaviour. The amount of "extra" in the first use case necessary for (1) is greater than the amount of "extra" in the second use case necessary for (2), though. In addition, in the second use case, (1) makes it easier for the user to miss important changes she made outside the area of her usual attention, while (2) forces her to make a conscious effort to exclude them. These are the reasons why I have a slight preference for (2) over (1). -- 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] git-stash: Don't GPG sign when stashing changes
On 3 May 2016 at 11:21, Junio C Hamano wrote: > On Mon, May 2, 2016 at 4:56 PM, Daurnimator wrote: >> On 3 May 2016 at 07:57, Junio C Hamano wrote: >> >> I agree quiltimport should not, but I think filter-branch possibly >> should... at least for your *own* commits. >> I often think of filter-branch as an "advanced" `git commit --amend` > > But it does not do the gpgsign thing only for your own commits right now, and > even if it did, you would not necessarily want it to always use the configured > setting. That means that the command, if it wants to work well with the signed > commits, must learn to honor command line option to enable or disable passing > --gpgsign option to underlying commit-tree *anyway*. > > So I think it is a right thing to fix commit-tree to ignore > commit.gpgsign at least > by default. Okay, that makes sense. Will you work on a patch? -- 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/3] test-parse-options: --expect= option to simplify tests
On Thu, May 5, 2016 at 7:57 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> instead of filtering afterwards, i.e. each strbuf_add is guarded by
>> an
>>
>> if (is_interesting_output(...))
>> strbuf_add(...)
>
> That's a good approach.
>
> The implementation gets a bit trickier than the previous one, but it
> would look like this. Discard 2/3 and 3/3 and replace them with
> this one.
>
> The external interface on the input side is no different, but on the
> output side, this version has "expected '%s', got '%s'" error, in
> the same spirit as the output from "test_cmp", added in.
>
> Instead of checking the entire output line-by-line for each expected
> output (in case you did not notice, you can give --expect='quiet: 3'
> --expect='abbrev: 7' and both must match), this one will check each
> output line against each expected pattern. We wouldn't have too
> many entries in the variable dump and we wouldn't be taking too many
> --expect options, so the matching performance would not matter,
> though.
>
>
> t/t0040-parse-options.sh | 1 +
> test-parse-options.c | 88
>
> 2 files changed, 75 insertions(+), 14 deletions(-)
>
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index dbaee55..d678fbf 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -45,6 +45,7 @@ Standard options
> -v, --verbose be verbose
> -n, --dry-run dry run
> -q, --quiet be quiet
> +--expect expected output in the variable dump
>
> EOF
>
> diff --git a/test-parse-options.c b/test-parse-options.c
> index b5f4e90..e3f25df 100644
> --- a/test-parse-options.c
> +++ b/test-parse-options.c
> @@ -39,6 +39,61 @@ static int number_callback(const struct option *opt, const
> char *arg, int unset)
> return 0;
> }
>
> +static int collect_expect(const struct option *opt, const char *arg, int
> unset)
> +{
> + struct string_list *expect;
> + struct string_list_item *item;
> + struct strbuf label = STRBUF_INIT;
> + const char *colon;
> +
> + if (!arg || unset)
> + die("malformed --expect option");
> +
> + expect = (struct string_list *)opt->value;
> + colon = strchr(arg, ':');
> + if (!colon)
> + die("malformed --expect option, lacking a colon");
> + strbuf_add(&label, arg, colon - arg);
> + item = string_list_insert(expect, strbuf_detach(&label, NULL));
> + if (item->util)
> + die("malformed --expect option, duplicate %s", label.buf);
> + item->util = (void *)arg;
> + return 0;
> +}
> +
> +__attribute__((format (printf,3,4)))
> +static void show(struct string_list *expect, int *status, const char *fmt,
> ...)
> +{
> + struct string_list_item *item;
> + struct strbuf buf = STRBUF_INIT;
> + va_list args;
> +
> + va_start(args, fmt);
> + strbuf_vaddf(&buf, fmt, args);
> + va_end(args);
> +
> + if (!expect->nr)
> + printf("%s\n", buf.buf);
> + else {
> + char *colon = strchr(buf.buf, ':');
> + if (!colon)
> + die("malformed output format, output lacking colon:
> %s", fmt);
> + *colon = '\0';
> + item = string_list_lookup(expect, buf.buf);
> + *colon = ':';
I have been staring at this for a good couple of minutes and wondered if this
low level string manipulation is really the best way to do it.
(It feels very C idiomatic, not using a lot of Gits own data
structures. I would have
expected some sort of skip_prefix just with partial regular expression or a
string_list_split_in_place for the splitting. But this "set and reset *colon"
seems to be optimal here)
> + if (!item)
> + ; /* not among entries being checked */
> + else {
> + if (strcmp((const char *)item->util, buf.buf)) {
> + printf("expected '%s', got '%s'\n",
> + (char *)item->util, buf.buf);
> + *status = 1;
> + }
> + }
> + }
> + strbuf_reset(&buf);
strbuf_release ?
> +}
> +
> int main(int argc, char **argv)
> {
> const char *prefix = "prefix/";
> @@ -46,6 +101,7 @@ int main(int argc, char **argv)
> "test-parse-options ",
> NULL
> };
> + struct string_list expect = STRING_LIST_INIT_NODUP;
> struct option options[] = {
> OPT_BOOL(0, "yes", &boolean, "get a boolean"),
> OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
> @@ -86,34 +142,38 @@ int main(int argc, char **argv)
> OPT__VERBOSE(&verbose, "be verbose"),
> OPT__DRY_RUN(&dry_run, "dry run"),
> OPT__QUIET(&quiet, "be quiet"),
>
Re: [PATCH v16 0/7] config commit verbose
On Thu, May 5, 2016 at 3:21 PM, Junio C Hamano wrote: > Pranit Bauva writes: >> This series of patches add a configuration variable for verbose in >> git-commit. >> >> Changes wrt v15: >> * Remove the previous patch 7/7 and split the tests. Include one in >>initial patch 6/7. The other one is introduced in a separate commit >>after 4/7. >> * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose, >>--no-verbose with -v as suggested by Eric Sunshine > > Thanks for a pleasant read. Modulo minor readability nits I sent > separately on 7/7, this looked good. Agreed, this version was a more pleasant and coherent read than previous ones. Considering that this series is already at v16 and the 7/7 review comments were very minor, I'd be fine seeing this series land as-is, rather than expecting v17. -- 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 v16 7/7] commit: add a commit.verbose config variable
On Thu, May 5, 2016 at 3:14 PM, Junio C Hamano wrote:
> Pranit Bauva writes:
>> +static int config_verbose = -1; /* unspecified */
>
> The name does not make it clear that config_verbose is only for
> "commit" and not relevant to "status".
>
>> @@ -1364,6 +1366,8 @@ int cmd_status(int argc, const char **argv, const char
>> *prefix)
>>builtin_status_usage, 0);
>> finalize_colopts(&s.colopts, -1);
>> finalize_deferred_config(&s);
>> + if (verbose == -1)
>> + verbose = 0;
>
> Mental note: cmd_status() does not use git_commit_config() but uses
> git_status_config(), hence config_verbose is not affected. But
> because verbose is initialised to -1, the code needs to turn it off
> like this.
>
>> @@ -1664,6 +1673,9 @@ int cmd_commit(int argc, const char **argv, const char
>> *prefix)
>> argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>> builtin_commit_usage,
>> prefix, current_head, &s);
>> + if (verbose == -1)
>> + verbose = (config_verbose < 0) ? 0 : config_verbose;
>> +
>
> cmd_commit() does use git_commit_config(), and verbose is
> initialised -1, so without command line option, we fall back to
> config_verbose if it is set from the configuration.
>
> I wonder if the attached patch squashed into this commit makes
> things easier to understand, though. The points are:
>
> - We rename the configuration to make it clear that it is about
>"commit" and does not apply to "status".
>
> - We initialize verbose to 0 as before. The only thing "git
>status" cares about is if "--verbose" was given. Giving it
>"--no-verbose" or nothing should not make any difference.
>
> - But we do need to stuff -1 to verbose in "git commit" before
>handling the command line options, because the distinction
>between having "--no-verbose" and not having any matter there,
>and we do so in cmd_commit(), i.e. only place where it matters.
Hmm... if someday someone wants git-status to support a status.verbose
config variable, with Pranit's current implementation, it's a pretty
simple change: Just add to git_status_config():
if (!strcmp(k, "status.verbose")) {
int is_bool;
config_verbose = git_config_bool_or_int(k, v, &is_bool);
return 0;
}
and in cmd_status() change:
if (verbose == -1)
verbose = 0;
to:
if (verbose == -1)
verbose = (config_verbose < 0) ? 0 : config_verbose;
It wouldn't be too hard with your proposal either: Either add a
'config_status_verbose' variable or rename 'config_commit_verbose'
back to 'config_verbose', initialize the global 'verbose' to -1, drop
the explicit 'verbose = -1' from cmd_commit(), and make the same
changes shown for Pranit's version. The diff would be a bit noisier.
I do like that your proposal makes it more difficult for
commit.verbose to break git-status, but otherwise don't feel that it
is significantly better. So, I dunno...
--
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 v16 7/7] commit: add a commit.verbose config variable
On Fri, May 6, 2016 at 12:44 AM, Junio C Hamano wrote:
> Pranit Bauva writes:
>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 391126e..114ffc9 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -113,7 +113,9 @@ static char *edit_message, *use_message;
>> static char *fixup_message, *squash_message;
>> static int all, also, interactive, patch_interactive, only, amend, signoff;
>> static int edit_flag = -1; /* unspecified */
>> -static int quiet, verbose, no_verify, allow_empty, dry_run,
>> renew_authorship;
>> +static int config_verbose = -1; /* unspecified */
>> +static int verbose = -1; /* unspecified */
>> +static int quiet, no_verify, allow_empty, dry_run, renew_authorship;
>
> The name does not make it clear that config_verbose is only for
> "commit" and not relevant to "status".
True.
>> @@ -1364,6 +1366,8 @@ int cmd_status(int argc, const char **argv, const char
>> *prefix)
>>builtin_status_usage, 0);
>> finalize_colopts(&s.colopts, -1);
>> finalize_deferred_config(&s);
>> + if (verbose == -1)
>> + verbose = 0;
>
> Mental note: cmd_status() does not use git_commit_config() but uses
> git_status_config(), hence config_verbose is not affected. But
> because verbose is initialised to -1, the code needs to turn it off
> like this.
Yes
>> @@ -1664,6 +1673,9 @@ int cmd_commit(int argc, const char **argv, const char
>> *prefix)
>> argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>> builtin_commit_usage,
>> prefix, current_head, &s);
>> + if (verbose == -1)
>> + verbose = (config_verbose < 0) ? 0 : config_verbose;
>> +
>
> cmd_commit() does use git_commit_config(), and verbose is
> initialised -1, so without command line option, we fall back to
> config_verbose if it is set from the configuration.
>
> I wonder if the attached patch squashed into this commit makes
> things easier to understand, though. The points are:
>
> - We rename the configuration to make it clear that it is about
>"commit" and does not apply to "status".
>
> - We initialize verbose to 0 as before. The only thing "git
>status" cares about is if "--verbose" was given. Giving it
>"--no-verbose" or nothing should not make any difference.
>
> - But we do need to stuff -1 to verbose in "git commit" before
>handling the command line options, because the distinction
>between having "--no-verbose" and not having any matter there,
>and we do so in cmd_commit(), i.e. only place where it matters.
Awesome work by addressing these points. I hadn't thought of these earlier.
> builtin/commit.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 583d1e3..a486620 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -113,9 +113,8 @@ static char *edit_message, *use_message;
> static char *fixup_message, *squash_message;
> static int all, also, interactive, patch_interactive, only, amend, signoff;
> static int edit_flag = -1; /* unspecified */
> -static int config_verbose = -1; /* unspecified */
> -static int verbose = -1; /* unspecified */
> -static int quiet, no_verify, allow_empty, dry_run, renew_authorship;
> +static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
> +static int config_commit_verbose = -1; /* unspecified */
> static int no_post_rewrite, allow_empty_message;
> static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
> static char *sign_commit;
> @@ -1366,8 +1365,6 @@ int cmd_status(int argc, const char **argv, const char
> *prefix)
> builtin_status_usage, 0);
> finalize_colopts(&s.colopts, -1);
> finalize_deferred_config(&s);
> - if (verbose == -1)
> - verbose = 0;
>
> handle_untracked_files_arg(&s);
> if (show_ignored_in_status)
> @@ -1521,7 +1518,7 @@ static int git_commit_config(const char *k, const char
> *v, void *cb)
> }
> if (!strcmp(k, "commit.verbose")) {
> int is_bool;
> - config_verbose = git_config_bool_or_int(k, v, &is_bool);
> + config_commit_verbose = git_config_bool_or_int(k, v,
> &is_bool);
> return 0;
> }
>
> @@ -1670,11 +1667,12 @@ int cmd_commit(int argc, const char **argv, const
> char *prefix)
> if (parse_commit(current_head))
> die(_("could not parse HEAD commit"));
> }
> + verbose = -1; /* unspecified */
> argc = parse_and_validate_options(argc, argv, builtin_commit_options,
> builtin_commit_usage,
> prefix, current_head, &s);
> if (verbose == -1)
> - verbose = (config_verbose < 0) ? 0 : config_verbose;
> +
Re: [PATCH v4 01/11] add fetch-pack --diag-url tests for some corner cases
On 05.05.16 23:52, Mike Hommey wrote: > On Wed, May 04, 2016 at 07:48:30AM +0900, Mike Hommey wrote: >> On Tue, May 03, 2016 at 09:07:41AM -0700, Junio C Hamano wrote: >>> Mike Hommey writes: >>> t5603-clone-dirname uses url patterns that are not tested with fetch-pack --diag-url, and it would be useful if they were. Interestingly, some of those tests, involving both a port and a user:password pair, don't currently pass. Note that even if a user:password pair is actually not supported by git, the values used could be valid user names (user names can actually contain colons and at signs), and are still worth testing the url parser for. >>> >>> I am not sure about the last part of this (and the tests in the >>> patch for them). When you are constrained by the Common Internet >>> Scheme Syntax, i.e. >>> >>> ://:@:/ >>> >>> you cannot have arbitrary characters in these parts; within the user >>> and password field, any ":", "@", or "/" must be encoded. >>> >>> Which maens that for the purpose of the parser you are modifying, >>> you can rely on these three special characters to parse things out >>> (decoding after the code determines which part is user and which >>> part is password is a separate issue). >> >> t5603-clone-dirname contains a test for e.g. ssh://user:passw@rd@host:1234/ >> That's the basis for these additions. Whether that should work or not is >> besides what I was interested in, which was to have a single test file to >> run to test my changes, instead of several. >> >> Strictly speaking, this patch is not necessary, because it only covers >> things that I found while breaking other tests. >> >> So, there are multiple possible ways forward here: >> - Completely remove this patch for v5 of the series. >> - Remove the user:passw@rd cases because of the @. >> - Remove the user:password cases because we do nothing with the password >> anyways. >> - A combination of both of the above. > > Any opinions on this? ssh itself does not use a password: SSH(1)BSD General Commands Manual SSH(1) NAME ssh -- OpenSSH SSH client (remote login program) SYNOPSIS ssh [-1246AaCfgKkMNnqsTtVvXxYy] [-b bind_address] [-c cipher_spec] [-D [bind_address:]port] [-e escape_char] [-F configfile] [-I pkcs11] [-i identity_file] [-L [bind_address:]port:host:hostport] [-l login_name] [-m mac_spec] [-O ctl_cmd] [-o option] [-p port] [-R [bind_address:]port:host:hostport] [-S ctl_path] [-W host:port] [-w local_tun[:remote_tun]] [user@]hostname [command] Neither does Git. A user name must not include a ':' The user:password came in here: Commit 92722efec01f67a54b clone: do not use port number as dir name Actually, looking back, it may have been better to say git clone ssh://:@host:/path is illegal and simply die() out. Back to your question and looking at the offered alternatives. I would vote for "Completely remove this patch for v5 of the series." -- 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] remote-ext: fix typo
Fix a typo in builtin/remote-ext.c.
Signed-off-by: Li Peng
---
builtin/remote-ext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 7457c74..88eb8f9 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -168,7 +168,7 @@ static int command_loop(const char *child)
size_t i;
if (!fgets(buffer, MAXCOMMAND - 1, stdin)) {
if (ferror(stdin))
- die("Comammand input error");
+ die("Command input error");
exit(0);
}
/* Strip end of line characters. */
--
1.8.3.1
--
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/3] test-parse-options: --expect= option to simplify tests
Stefan Beller writes:
> instead of filtering afterwards, i.e. each strbuf_add is guarded by
> an
>
> if (is_interesting_output(...))
> strbuf_add(...)
That's a good approach.
The implementation gets a bit trickier than the previous one, but it
would look like this. Discard 2/3 and 3/3 and replace them with
this one.
The external interface on the input side is no different, but on the
output side, this version has "expected '%s', got '%s'" error, in
the same spirit as the output from "test_cmp", added in.
Instead of checking the entire output line-by-line for each expected
output (in case you did not notice, you can give --expect='quiet: 3'
--expect='abbrev: 7' and both must match), this one will check each
output line against each expected pattern. We wouldn't have too
many entries in the variable dump and we wouldn't be taking too many
--expect options, so the matching performance would not matter,
though.
t/t0040-parse-options.sh | 1 +
test-parse-options.c | 88
2 files changed, 75 insertions(+), 14 deletions(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index dbaee55..d678fbf 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -45,6 +45,7 @@ Standard options
-v, --verbose be verbose
-n, --dry-run dry run
-q, --quiet be quiet
+--expect expected output in the variable dump
EOF
diff --git a/test-parse-options.c b/test-parse-options.c
index b5f4e90..e3f25df 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -39,6 +39,61 @@ static int number_callback(const struct option *opt, const
char *arg, int unset)
return 0;
}
+static int collect_expect(const struct option *opt, const char *arg, int unset)
+{
+ struct string_list *expect;
+ struct string_list_item *item;
+ struct strbuf label = STRBUF_INIT;
+ const char *colon;
+
+ if (!arg || unset)
+ die("malformed --expect option");
+
+ expect = (struct string_list *)opt->value;
+ colon = strchr(arg, ':');
+ if (!colon)
+ die("malformed --expect option, lacking a colon");
+ strbuf_add(&label, arg, colon - arg);
+ item = string_list_insert(expect, strbuf_detach(&label, NULL));
+ if (item->util)
+ die("malformed --expect option, duplicate %s", label.buf);
+ item->util = (void *)arg;
+ return 0;
+}
+
+__attribute__((format (printf,3,4)))
+static void show(struct string_list *expect, int *status, const char *fmt, ...)
+{
+ struct string_list_item *item;
+ struct strbuf buf = STRBUF_INIT;
+ va_list args;
+
+ va_start(args, fmt);
+ strbuf_vaddf(&buf, fmt, args);
+ va_end(args);
+
+ if (!expect->nr)
+ printf("%s\n", buf.buf);
+ else {
+ char *colon = strchr(buf.buf, ':');
+ if (!colon)
+ die("malformed output format, output lacking colon:
%s", fmt);
+ *colon = '\0';
+ item = string_list_lookup(expect, buf.buf);
+ *colon = ':';
+ if (!item)
+ ; /* not among entries being checked */
+ else {
+ if (strcmp((const char *)item->util, buf.buf)) {
+ printf("expected '%s', got '%s'\n",
+ (char *)item->util, buf.buf);
+ *status = 1;
+ }
+ }
+ }
+ strbuf_reset(&buf);
+}
+
int main(int argc, char **argv)
{
const char *prefix = "prefix/";
@@ -46,6 +101,7 @@ int main(int argc, char **argv)
"test-parse-options ",
NULL
};
+ struct string_list expect = STRING_LIST_INIT_NODUP;
struct option options[] = {
OPT_BOOL(0, "yes", &boolean, "get a boolean"),
OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
@@ -86,34 +142,38 @@ int main(int argc, char **argv)
OPT__VERBOSE(&verbose, "be verbose"),
OPT__DRY_RUN(&dry_run, "dry run"),
OPT__QUIET(&quiet, "be quiet"),
+ OPT_CALLBACK(0, "expect", &expect, "string",
+"expected output in the variable dump",
+collect_expect),
OPT_END(),
};
int i;
+ int ret = 0;
argc = parse_options(argc, (const char **)argv, prefix, options, usage,
0);
if (length_cb.called) {
const char *arg = length_cb.arg;
int unset = length_cb.unset;
- printf("Callback: \"%s\", %d\n",
- (arg ? arg : "not set"), unset);
+ show(&expect, &ret, "Callback: \"%s\", %d",
+(arg ? arg : "not set"), unset);
}
- printf("boolean: %d
Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests
On Thu, May 5, 2016 at 8:41 PM, Stefan Beller wrote:
> On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano wrote:
>> [...]
>> But the only thing this test cares about is if "quiet: 3" is in the
>> output. We should be able to write the above 18 lines with just
>> four lines, like this:
>>
>> test_expect_success 'multiple quiet levels' '
>> test-parse-options --expect="quiet: 3" -q -q -q
>> '
>>
>> Teach the new --expect= option to test-parse-options helper.
>>
>> Signed-off-by: Junio C Hamano
>> ---
>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
>> +/*
>> + * See if expect->string ("label: value") has a line in output that
>> + * begins with "label:", and if the line in output matches it.
>> + */
>> +static int match_line(struct string_list_item *expect, struct strbuf
>> *output)
>> +{
>> + [...]
>> + const char *scan = output->buf;
>> + [...]
>> + while (scan < output->buf + output->len) {
>> + const char *next;
>> + scan = memmem(scan, output->buf + output->len - scan,
>> + label, label_len);
>> + if (!scan)
>> + return 0;
>> + if (scan == output->buf || scan[-1] == '\n')
>
> Does scan[-1] work for the first line?
Take note of the short-circuiting '||' operator.
> On a philosophical level this patch series is adding a
> trailing "|grep $X" for the test-parse-options.
> I think such a grep pattern is a good thing because it is
> cheap to implement in unix like environments.
>
> This however is a lot of C code for finding specific subsets
> in the output, so it is not quite cheap. Then we could also go
> the non-wasteful way and instead check what to add to the strbuf
> instead of filtering afterwards, i.e. each strbuf_add is guarded by
> an
>
> if (is_interesting_output(...))
> strbuf_add(...)
I agree that this is adds far more complexity than I had expected upon
reading Junio's suggestion about simplifying the t0040 tests. Patch 1
aside (which seems a desirable change), rather than patches 2 and 3, I
had expected to see only introduction of a minor helper function in
t0040; perhaps something like this:
options_expect () {
expect="$1" &&
shift &&
test-parse-options "$@" >actual &&
grep "$expect" actual
}
and tests updated like this:
options_expect "quiet: 3" -q -q -q
--
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: stop sanitizing config options
On Thu, May 05, 2016 at 05:23:51PM -0700, Stefan Beller wrote: > >> -/* > >> - * This function is intended as a callback for use with > >> - * git_config_from_parameters(). It ignores any config options which > >> - * are not suitable for passing along to a submodule, and accumulates the > >> rest > >> - * in "data", which must be a pointer to a strbuf. The end result can > >> - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process. > >> - */ > >> -int sanitize_submodule_config(const char *var, const char *value, void > >> *data); > >> - > >> /* > >> * Prepare the "env_array" parameter of a "struct child_process" for > >> executing > >> * a submodule by clearing any repo-specific envirionment variables, but > >> - * retaining any config approved by sanitize_submodule_config(). > >> + * retaining any config in the environment. > >> */ > >> void prepare_submodule_repo_env(struct argv_array *out); > >> > >> > >> -Peff > > > > Hmph, Stefan, do you want to keep this (if you want to resurrect the > > function in some future, for example)? > > IMHO it is easier to revert or rewrite than to carry unused code? > Unused code is not tested and untested code is broken code. > And relying on broken code in the future will guarantee bugs. > (Sure new code may also have bugs, but I just think that bugs > in newly written code can be fixed easier) > > I would prefer to get rid of it, i.e. taking that patch. Keep in mind this squash is just dropping the _declaration_. The code itself was dropped in the earlier patch. (And I agree there isn't a good reason to keep it when we can easily "git revert" later). -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/3] test-parse-options: --expect= option to simplify tests
On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano wrote:
> Existing tests in t0040 follow a rather verbose pattern:
>
> cat >expect <<\EOF
> boolean: 0
> integer: 0
> magnitude: 0
> timestamp: 0
> string: (not set)
> abbrev: 7
> verbose: 0
> quiet: 3
> dry run: no
> file: (not set)
> EOF
>
> test_expect_success 'multiple quiet levels' '
> test-parse-options -q -q -q >output 2>output.err &&
> test_must_be_empty output.err &&
> test_cmp expect output
> '
>
> But the only thing this test cares about is if "quiet: 3" is in the
> output. We should be able to write the above 18 lines with just
> four lines, like this:
>
> test_expect_success 'multiple quiet levels' '
> test-parse-options --expect="quiet: 3" -q -q -q
> '
>
> Teach the new --expect= option to test-parse-options helper.
>
> Signed-off-by: Junio C Hamano
> ---
> t/t0040-parse-options.sh | 1 +
> test-parse-options.c | 68
> +---
> 2 files changed, 66 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index dbaee55..d678fbf 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -45,6 +45,7 @@ Standard options
> -v, --verbose be verbose
> -n, --dry-run dry run
> -q, --quiet be quiet
> +--expect expected output in the variable dump
>
> EOF
>
> diff --git a/test-parse-options.c b/test-parse-options.c
> index 3db4332..010f3b2 100644
> --- a/test-parse-options.c
> +++ b/test-parse-options.c
> @@ -14,6 +14,7 @@ static char *string = NULL;
> static char *file = NULL;
> static int ambiguous;
> static struct string_list list;
> +static struct string_list expect;
>
> static struct {
> int called;
> @@ -40,6 +41,62 @@ static int number_callback(const struct option *opt, const
> char *arg, int unset)
> return 0;
> }
>
> +/*
> + * See if expect->string ("label: value") has a line in output that
> + * begins with "label:", and if the line in output matches it.
> + */
> +static int match_line(struct string_list_item *expect, struct strbuf *output)
> +{
> + const char *label = expect->string;
> + const char *colon = strchr(label, ':');
> + const char *scan = output->buf;
> + size_t label_len, expect_len;
> +
> + if (!colon)
> + die("Malformed --expect value: %s", label);
> + label_len = colon - label;
> +
> + while (scan < output->buf + output->len) {
> + const char *next;
> + scan = memmem(scan, output->buf + output->len - scan,
> + label, label_len);
> + if (!scan)
> + return 0;
> + if (scan == output->buf || scan[-1] == '\n')
Does scan[-1] work for the first line?
> + break;
> + next = strchr(scan + label_len, '\n');
> + if (!next)
> + return 0;
> + scan = next + 1;
> + }
> +
> + /*
> +* scan points at a line that begins with the label we are
> +* looking for. Does it match?
> +*/
> + expect_len = strlen(expect->string);
> +
> + if (output->buf + output->len <= scan + expect_len)
> + return 0; /* value not long enough */
> + if (memcmp(scan, expect->string, expect_len))
> + return 0; /* does not match */
> +
> + return (scan + expect_len < output->buf + output->len &&
> + scan[expect_len] == '\n');
> +}
> +
> +static int show_expected(struct string_list *list, struct strbuf *output)
> +{
> + struct string_list_item *expect;
> + int found_mismatch = 0;
> +
> + for_each_string_list_item(expect, list) {
> + if (!match_line(expect, output))
> + found_mismatch = 1;
> + }
> + return found_mismatch;
> +}
> +
> int main(int argc, char **argv)
> {
> const char *prefix = "prefix/";
> @@ -87,6 +144,8 @@ int main(int argc, char **argv)
> OPT__VERBOSE(&verbose, "be verbose"),
> OPT__DRY_RUN(&dry_run, "dry run"),
> OPT__QUIET(&quiet, "be quiet"),
> + OPT_STRING_LIST(0, "expect", &expect, "string",
> + "expected output in the variable dump"),
> OPT_END(),
> };
> int i;
> @@ -117,7 +176,10 @@ int main(int argc, char **argv)
> for (i = 0; i < argc; i++)
> strbuf_addf(&output, "arg %02d: %s\n", i, argv[i]);
>
> - printf("%s", output.buf);
> -
> - return 0;
> + if (expect.nr)
> + return show_expected(&expect, &output);
On a philosophical level this patch series is adding a
trailing "|grep $X" for the t
Re: [PATCH] pathspec: remove check_path_for_gitlink
On Thu, May 5, 2016 at 4:54 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> That was my first reaction as well. However after a while of thought I >> actually >> like that bug. Consider the possibilities how gitk/git-gui or other >> subsystems >> can be developed. When accepting a patch for that you can either apply the >> patch in the outer or inner repository, depending on what the sender used. >> >> I am not so sure if it is a bug plain and simple, but devolved into a >> "feature" now. > > I'd freely admit that I have not considered its possible upsides at > all. When deep/in/ is an unrelated repository, and running either > > git add deep/in/the > git add deep/in/the/tree I think that doesn't work (I did not test), but the crucial part is to add a trailing '/'. E.g. `git add deep/in/the/` adds the 'the/**' tree of the nested repository. > > would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did > > git add deep/in > > I'd lose that and suddenly everything there turns into a submodule. Yes. git add deep/in # adds a submodule however: git add deep/in/ # adds all files of the sub-"repo" as indpendent files git commit -a -m "new files" git -C deep/in reset --hard HEAD^ git diff # shows a difference in deep/in/the/tree/is-a-leaf.txt > > And that is enough for me to declare that it is not worth my time to > consider possible upside of that hole. Can you tell offhand what > would happen if you do "git add deep" (before adding deep/in as a > submodule) without experimenting? > Not really. My expectation is to add everything *but* the deep/in/ repo as this is not exercising the bug/feature. -- 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: stop sanitizing config options
On Thu, May 5, 2016 at 4:33 PM, Junio C Hamano wrote: > Jeff King writes: > >> I had originally thought after the final one that we could further clean >> up by turning prepare_submodule_repo_env() into a static function. But >> we can't; it gets called in one spot from submodule--helper. However, >> while looking at it, I did notice that we probably want to squash this >> into the final patch (since sanitize_submodule_config went away >> completely): >> >> diff --git a/submodule.h b/submodule.h >> index 48690b1..869d259 100644 >> --- a/submodule.h >> +++ b/submodule.h >> @@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], >> const char *remotes_nam >> int push_unpushed_submodules(unsigned char new_sha1[20], const char >> *remotes_name); >> void connect_work_tree_and_git_dir(const char *work_tree, const char >> *git_dir); >> >> -/* >> - * This function is intended as a callback for use with >> - * git_config_from_parameters(). It ignores any config options which >> - * are not suitable for passing along to a submodule, and accumulates the >> rest >> - * in "data", which must be a pointer to a strbuf. The end result can >> - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process. >> - */ >> -int sanitize_submodule_config(const char *var, const char *value, void >> *data); >> - >> /* >> * Prepare the "env_array" parameter of a "struct child_process" for >> executing >> * a submodule by clearing any repo-specific envirionment variables, but >> - * retaining any config approved by sanitize_submodule_config(). >> + * retaining any config in the environment. >> */ >> void prepare_submodule_repo_env(struct argv_array *out); >> >> >> -Peff > > Hmph, Stefan, do you want to keep this (if you want to resurrect the > function in some future, for example)? IMHO it is easier to revert or rewrite than to carry unused code? Unused code is not tested and untested code is broken code. And relying on broken code in the future will guarantee bugs. (Sure new code may also have bugs, but I just think that bugs in newly written code can be fixed easier) I would prefer to get rid of it, i.e. taking that patch. Thanks, Stefan -- 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] pathspec: remove check_path_for_gitlink
Stefan Beller writes: > That was my first reaction as well. However after a while of thought I > actually > like that bug. Consider the possibilities how gitk/git-gui or other subsystems > can be developed. When accepting a patch for that you can either apply the > patch in the outer or inner repository, depending on what the sender used. > > I am not so sure if it is a bug plain and simple, but devolved into a > "feature" now. I'd freely admit that I have not considered its possible upsides at all. When deep/in/ is an unrelated repository, and running either git add deep/in/the git add deep/in/the/tree would add deep/in/the/tree/is-a-leaf.txt to my index, but if I did git add deep/in I'd lose that and suddenly everything there turns into a submodule. And that is enough for me to declare that it is not worth my time to consider possible upside of that hole. Can you tell offhand what would happen if you do "git add deep" (before adding deep/in as a submodule) without experimenting? -- 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] pathspec: remove check_path_for_gitlink
On Thu, May 5, 2016 at 4:27 PM, Junio C Hamano wrote: > Stefan Beller writes: > >>> I wonder if the patches mentioned have something to do with the "git >>> add deep/in/the/tree" that fails to notice deep/in/ is an unrelated >>> repository in some way? >> >> Which is considered a feature now. Maybe we should add tests for that? >> >> http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb > > That is a bug, plain and simple. Duy any ideas where we went wrong? That was my first reaction as well. However after a while of thought I actually like that bug. Consider the possibilities how gitk/git-gui or other subsystems can be developed. When accepting a patch for that you can either apply the patch in the outer or inner repository, depending on what the sender used. I am not so sure if it is a bug plain and simple, but devolved into a "feature" now. > > I think we already have code to avoid adding beyond symlinks. > "git add deep/in/the/tree" should refuse if deep/in is a symbolic > link (and happens to point at a directory that has the/tree in it). > We used not to catch that long time ago, but I think we fixed it. > > The logic and the places to do the checks for "no, that thing may be > a directory but is an unrelated repository" should be the same. -- 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: stop sanitizing config options
Jeff King writes: > I had originally thought after the final one that we could further clean > up by turning prepare_submodule_repo_env() into a static function. But > we can't; it gets called in one spot from submodule--helper. However, > while looking at it, I did notice that we probably want to squash this > into the final patch (since sanitize_submodule_config went away > completely): > > diff --git a/submodule.h b/submodule.h > index 48690b1..869d259 100644 > --- a/submodule.h > +++ b/submodule.h > @@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], > const char *remotes_nam > int push_unpushed_submodules(unsigned char new_sha1[20], const char > *remotes_name); > void connect_work_tree_and_git_dir(const char *work_tree, const char > *git_dir); > > -/* > - * This function is intended as a callback for use with > - * git_config_from_parameters(). It ignores any config options which > - * are not suitable for passing along to a submodule, and accumulates the > rest > - * in "data", which must be a pointer to a strbuf. The end result can > - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process. > - */ > -int sanitize_submodule_config(const char *var, const char *value, void > *data); > - > /* > * Prepare the "env_array" parameter of a "struct child_process" for > executing > * a submodule by clearing any repo-specific envirionment variables, but > - * retaining any config approved by sanitize_submodule_config(). > + * retaining any config in the environment. > */ > void prepare_submodule_repo_env(struct argv_array *out); > > > -Peff Hmph, Stefan, do you want to keep this (if you want to resurrect the function in some future, for example)? -- 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] pathspec: remove check_path_for_gitlink
Stefan Beller writes: >> I wonder if the patches mentioned have something to do with the "git >> add deep/in/the/tree" that fails to notice deep/in/ is an unrelated >> repository in some way? > > Which is considered a feature now. Maybe we should add tests for that? > > http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb That is a bug, plain and simple. Duy any ideas where we went wrong? I think we already have code to avoid adding beyond symlinks. "git add deep/in/the/tree" should refuse if deep/in is a symbolic link (and happens to point at a directory that has the/tree in it). We used not to catch that long time ago, but I think we fixed it. The logic and the places to do the checks for "no, that thing may be a directory but is an unrelated repository" should be the same. -- 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] pathspec: get non matching arguments without reporting.
Junio C Hamano writes: > Stefan Beller writes: > >> When giving more than just pathspec arguments (e.g. submodule labels), >> we need a way to check all non-matching arguments for the pathspec parsing >> if these are the submodule labels or typos. >> >> This patch prepares for that use case by splitting up `report_path_error` >> into a new checking function `unmatched_pathspec_items` and a >> reporting callback. > > I seem to recall that there is a longstanding plan to move the > "seen[]" array that is separate and outside from the pathspec > structure into the pathspec structure. Wouldn't that be a more > sensible approach to solve the same thing? Even with such a refactoring, the need to split the "preprocessing" (mainly "dedup") still remains if you want to silence this codepath, so I think this is OK from pathspec/dir.c point of view. But I do not agree with the first paragraph at all. You'd be using separate syntax like '*label', ':name', etc. for the purpose of enumerating submodules, so instead of blindly passing argv[] to pathspec code, I expected you to do a preprocessing of argv[] upfront, sifting them into three bins (labels, names and paths), and giving only the last ones to the pathspec machinery. And if you did that, report_path_error() would never have to worry about "ah, this thing does not exist in the working tree or in the index but that is natural because it is a submodule label" at all, no? -- 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] pathspec: remove check_path_for_gitlink
On Thu, May 5, 2016 at 4:09 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> `check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06, >> add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse) >> but the implementation was removed in 5a76aff1a6 (2013-07-14, add: >> convert to use parse_pathspec). >> >> Remove the declaration from the header as well. >> >> Signed-off-by: Stefan Beller >> --- >> pathspec.h | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/pathspec.h b/pathspec.h >> index 0c11262..b596aed 100644 >> --- a/pathspec.h >> +++ b/pathspec.h >> @@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item >> *item, >> >> extern char *find_pathspecs_matching_against_index(const struct pathspec >> *pathspec); >> extern void add_pathspec_matches_against_index(const struct pathspec >> *pathspec, char *seen); >> -extern const char *check_path_for_gitlink(const char *path); >> extern void die_if_path_beyond_symlink(const char *path, const char >> *prefix); >> >> #endif /* PATHSPEC_H */ > > Interesting. > > I wonder if the patches mentioned have something to do with the "git > add deep/in/the/tree" that fails to notice deep/in/ is an unrelated > repository in some way? Which is considered a feature now. Maybe we should add tests for that? http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb -- 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] pathspec: get non matching arguments without reporting.
Stefan Beller writes: > When giving more than just pathspec arguments (e.g. submodule labels), > we need a way to check all non-matching arguments for the pathspec parsing > if these are the submodule labels or typos. > > This patch prepares for that use case by splitting up `report_path_error` > into a new checking function `unmatched_pathspec_items` and a > reporting callback. I seem to recall that there is a longstanding plan to move the "seen[]" array that is separate and outside from the pathspec structure into the pathspec structure. Wouldn't that be a more sensible approach to solve the same thing? -- 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] pathspec: remove check_path_for_gitlink
Stefan Beller writes: > `check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06, > add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse) > but the implementation was removed in 5a76aff1a6 (2013-07-14, add: > convert to use parse_pathspec). > > Remove the declaration from the header as well. > > Signed-off-by: Stefan Beller > --- > pathspec.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/pathspec.h b/pathspec.h > index 0c11262..b596aed 100644 > --- a/pathspec.h > +++ b/pathspec.h > @@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item > *item, > > extern char *find_pathspecs_matching_against_index(const struct pathspec > *pathspec); > extern void add_pathspec_matches_against_index(const struct pathspec > *pathspec, char *seen); > -extern const char *check_path_for_gitlink(const char *path); > extern void die_if_path_beyond_symlink(const char *path, const char *prefix); > > #endif /* PATHSPEC_H */ Interesting. I wonder if the patches mentioned have something to do with the "git add deep/in/the/tree" that fails to notice deep/in/ is an unrelated repository in some way? -- 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 11/15] diff: ignore submodules excluded by groups
Stefan Beller writes: > I am aware that other operations such as a build system would be glad to > have the contents of the submodules there. But those would not use a > restrictive default group? The set of submodules you "init" to the working tree are the ones that are interesting to you. So once the tree is populated, you do not ever have to look at the "defaultGroup" configuration. You just need to look at the working tree. But there is a chicken-and-egg problem. What should happen after the initial clone, or you switched to a different branch in the superproject. The concept of "default" would help by limiting these "checkout to reflect my interest to my working tree" step. So "if the user inititializes a submodule and we detect that it is not in the default, add it to the default configuration" pretty much feels like a tail wagging a dog arrangement to me. Then there is another interesting issue: what should happen when the project added a submodule when you decided what your "default" set should be and wrote it in your configuration already. I suspect that an idea similar to "the elaborate thing proposed (by whom I no longer rememvber) in the ancient days" I mentioned earlier might leads us to a nice solution. When you define a default group, you remember what the set of available submodules were, and tie your choice to that "available set". E.g. there may be submodules A, B and C in .gitmodules, and you chose to record a "default" that contains only A and B. The exact way you chose does not matter; it could have been using labels, or you may have explicitly named it. When you record, you remember that the decision of using A and B was made when A, B and C were the available submodules. Next time when you see .gitmodules talks about A, B and D but no longer C, then instead of using the previous "default" choice blindly, you ask. If the user says it still is the right "default" to use A and B, then you _also_ remember the set. So that when the user switches between the state of the project with submodules A, B and C (original) and A, B and D (updated), the user does not have to answer the same question twice. -- 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] pathspec: get non matching arguments without reporting.
When giving more than just pathspec arguments (e.g. submodule labels),
we need a way to check all non-matching arguments for the pathspec parsing
if these are the submodule labels or typos.
This patch prepares for that use case by splitting up `report_path_error`
into a new checking function `unmatched_pathspec_items` and a
reporting callback.
Signed-off-by: Stefan Beller
---
So I imagine the unmatched_pathspec_items to be used later similar as
in report_path_error, maybe just like this:
void submodule_list_fn(const struct pathspec *pathspec,
int pathspec_index,
void *data_cb)
{
if (submodule_label_exist(pathspec->items[pathspec_index].original))
// ok, record in data_cb
else
error(" '%s' is neither recognized as matching pathspec nor did
it
match any submodule label",
pathspec->items[pathspec_index].original);
}
submodule_list(const char *ps_matched,
const struct pathspec *pathspec,
const char *prefix)
{
struct string_list detected_labels;
unmatched_pathspec_items(ps_matched, pathspec, prefix,
submodule_list_fn, &detected_labels);
foreach (submodule s) {
if matches_pathspec(s, pathspec) or label_match(s,
detected_labels)
// do a thing
else
continue
}
}
What do you think of such a design? Is it worth carrying and polishing or
would there be another way to do it which matches the pathspec mechanism better?
Thanks,
Stefan
dir.c | 36 ++--
dir.h | 8
2 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/dir.c b/dir.c
index a4a9d9f..bc8b199 100644
--- a/dir.c
+++ b/dir.c
@@ -394,14 +394,13 @@ int match_pathspec(const struct pathspec *ps,
return negative ? 0 : positive;
}
-int report_path_error(const char *ps_matched,
- const struct pathspec *pathspec,
- const char *prefix)
+void unmatched_pathspec_items(const char *ps_matched,
+const struct pathspec *pathspec,
+const char *prefix,
+unmatched_pathspec_items_fn fn,
+void *data_cb)
{
- /*
-* Make sure all pathspec matched; otherwise it is an error.
-*/
- int num, errors = 0;
+ int num;
for (num = 0; num < pathspec->nr; num++) {
int other, found_dup;
@@ -428,10 +427,27 @@ int report_path_error(const char *ps_matched,
if (found_dup)
continue;
- error("pathspec '%s' did not match any file(s) known to git.",
- pathspec->items[num].original);
- errors++;
+ fn(pathspec, num, data_cb);
}
+}
+
+void report_path_error_fn(const struct pathspec *pathspec,
+ int pathspec_index,
+ void *data_cb)
+{
+ int *errors = data_cb;
+ error("pathspec '%s' did not match any file(s) known to git.",
+ pathspec->items[pathspec_index].original);
+ (*errors)++;
+}
+
+int report_path_error(const char *ps_matched,
+ const struct pathspec *pathspec,
+ const char *prefix)
+{
+ int errors = 0;
+ unmatched_pathspec_items(ps_matched, pathspec, prefix,
+ report_path_error_fn, &errors);
return errors;
}
diff --git a/dir.h b/dir.h
index cd46f30..ea222eb 100644
--- a/dir.h
+++ b/dir.h
@@ -211,6 +211,14 @@ extern char *common_prefix(const struct pathspec
*pathspec);
extern int match_pathspec(const struct pathspec *pathspec,
const char *name, int namelen,
int prefix, char *seen, int is_dir);
+typedef void (*unmatched_pathspec_items_fn)(const struct pathspec *pathspec,
+ int pathspec_index,
+ void *data_cb);
+void unmatched_pathspec_items(const char *ps_matched,
+const struct pathspec *pathspec,
+const char *prefix,
+unmatched_pathspec_items_fn fn,
+void *data_cb);
extern int report_path_error(const char *ps_matched, const struct pathspec
*pathspec, const char *prefix);
extern int within_depth(const char *name, int namelen, int depth, int
max_depth);
--
2.8.0.1.g3af9c03
--
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 v8 19/19] untracked-cache: config option
David Turner writes:
> + if (!git_config_get_bool("index.adduntrackedcache", &untracked) &&
> + untracked &&
> + !istate->untracked)
> + add_untracked_cache(&the_index);
The body is indented with HT + 4 spaces, replace it with two HTs.
--
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 v8 10/19] index-helper: use watchman to avoid refreshing index with lstat()
David Turner writes:
> +extern void write_watchman_ext(struct strbuf *sb, struct index_state*
> istate);
Asterisk sticks to the variable, not to the type.
> + if (c == sb.buf) {
> + strbuf_setlen(&sb, 0);
> + }
Unnecessary braces {}.
--
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 11/15] diff: ignore submodules excluded by groups
On Thu, May 5, 2016 at 3:20 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> The first option is giving nothing: >> >> git config submodule.defaultGroup "*SomeLabel" >> git -C submodule-not-labeled reset --hard HEAD^ >> git status >> # all good, no report about submodule-not-labeled >> # because it is not in the default group. >> # (This is implemented in the series) >> >> The "second option" is some sort of reporting. Either what I or you proposed. > > OK, although I didn't propose anything ;-). > >> >>> > If we want to go with the second option, the design described in 0/15 > is broken. Going one step further: > > ... > # But what about subC which is not in the default group? It was > changed as well. >>> >>> So why not show it? Is there anything controversial? >> >> The user made clear to not be interested in subC by setting >> up the default group. > > I wonder if that is a valid argument. Git's position has always > been very clear after doing this: > > git add -f foo.bin && echo \*.bin >>.gitignore > > What the user might have said in the "configuration" is the default > suggestion, that is much weaker than what has been done to the > repository data. I think "the path is recorded in the index" in the > ignore/exclude situation is similar to "the submodule is initialized > in the working tree" in this context. > >> Well it can get out of sync by not touching it as well, because others >> changed the submodule pointer who are interested in that though. > > Which "others" are mucking with your working tree? (don't respond: > I'll come to the answer a few lines below). > >> >> # in the superproject >> git checkout new-version >> git submodule update >> # checks out submodules to their version >> >> git checkout some-ancient-version I think here is when the conceptual bug happens. We would want a git checkout --keep-submodules-to-pristine-default-group (intentionally long bad name) that option would init new submodules and deinit old submodules which were clean before. Here we can compare it to a file, i.e. after checkout some files are newly there, some are deleted. (And that's totally expected) If we had this `checkout --treat-submodules-as-files-for-defaultGroup`, then the following command `git submodule update` would not be required. I think long term this is a far better approach. I just wonder what the `checkout --recurse-submodules` should do when there is no defaultGroup configured. (i.e. should that delete submodules which were there before and delete them if they were clean? Just like files.) >> git submodule update >> # this would only update the submodules in the defaultGroup, >> # not those which are initialized but "uninteresting" >> # the labeling may have changed between the different versions > > I see. I think that is where the conceptual bug lies. Thanks for > an illustration. Yes. > > If we take an approach similar to ignore/exclude, then yes the > "default" action should be done to "defaultGroup" specified plus > what the user instantiated in the working tree already. And that > is not limited to "update" operation. > > Just like "git diff" is not the only thing that would show > difference in foo.bin in the working tree even when *.bin is > ignored, but we consistently treat foo.bin as tracked. > >> The state of a submodule (un-initialized, initialized, checked out) >> doesn't change the index or anything. Only the working tree is affected. >> >> And by flipping between the initialized and checked-out state we do not >> lose any information (such as user configured remote urls) nor does it >> affect the "state" (index, recorded tree, history) of the repository. > > Users want to initialize a module and keep it checked out even if > they do intend to keep it pristine and not making any changes > themselves, only so that other parts of the tree that depends on the > module can be built. The `submodule init` command could learn to just add that path of the extra submodule to the defaultGroup, such that it persists between different checkouts. > So "removing a tracked and unmodified thing > from the working tree does not hurt users" is a nonsense argument, > isn't it? I would be very unhappy if "git status" removed pristine > paths from the working tree and toggle assume-unchanged bit in my > index automatically. No, `checkout` did it for you. Assume we had a "defaultGroup for files", (others call it "narrow clones") # in git.git git set-file-default-group Documentation/RelNotes git checkout v2.6.0 ls Documentation/RelNotes |grep v2.7 # no files! But this is uncontroversial as it's current behavior # even with fictional "files default group" set to "all files ever" ls builtin/ # does it exist? No because set-file-default-group # did restrict out interest here. > > I am afraid you are focused too much on "version cont
Re: [PATCH v8 18/19] Add tracing to measure where most of the time is spent
David Turner writes: > From: Nguyễn Thái Ngọc Duy > Subject: Re: [PATCH v8 18/19] Add tracing to measure where most of the time > is spent trace: measure where the time is spent in the index-heavy operations -- 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 v8 09/19] Add watchman support to reduce index refresh cost
David Turner writes:
> From: Nguyễn Thái Ngọc Duy
> Subject: Re: [PATCH v8 09/19] Add watchman support to reduce index refresh
> cost
Subject: watchman: support watchman to reduce index refresh cost
> +static struct watchman_query_result* query_watchman(
> + struct index_state *istate, struct watchman_connection *connection,
> + const char *fs_path, const char *last_update)
> +{
static struct watchman_query_result *query_watchman(
Asterisk sticks to the variable, not to the type.
--
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 v8 08/19] read-cache: add watchman 'WAMA' extension
David Turner writes: > +void write_watchman_ext(struct strbuf *sb, struct index_state* istate) void write_watchman_ext(struct strbuf *sb, struct index_state *istate) Asterisk sticks to the variable, not to the type. -- 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] pathspec: remove check_path_for_gitlink
`check_path_for_gitlink` was introduced in 9d67b61f739a (2013-01-06, add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse) but the implementation was removed in 5a76aff1a6 (2013-07-14, add: convert to use parse_pathspec). Remove the declaration from the header as well. Signed-off-by: Stefan Beller --- pathspec.h | 1 - 1 file changed, 1 deletion(-) diff --git a/pathspec.h b/pathspec.h index 0c11262..b596aed 100644 --- a/pathspec.h +++ b/pathspec.h @@ -96,7 +96,6 @@ static inline int ps_strcmp(const struct pathspec_item *item, extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec); extern void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen); -extern const char *check_path_for_gitlink(const char *path); extern void die_if_path_beyond_symlink(const char *path, const char *prefix); #endif /* PATHSPEC_H */ -- 2.8.0.1.g3af9c03 -- 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 11/15] diff: ignore submodules excluded by groups
Stefan Beller writes: > The first option is giving nothing: > > git config submodule.defaultGroup "*SomeLabel" > git -C submodule-not-labeled reset --hard HEAD^ > git status > # all good, no report about submodule-not-labeled > # because it is not in the default group. > # (This is implemented in the series) > > The "second option" is some sort of reporting. Either what I or you proposed. OK, although I didn't propose anything ;-). > >> If we want to go with the second option, the design described in 0/15 is broken. Going one step further: ... # But what about subC which is not in the default group? It was changed as well. >> >> So why not show it? Is there anything controversial? > > The user made clear to not be interested in subC by setting > up the default group. I wonder if that is a valid argument. Git's position has always been very clear after doing this: git add -f foo.bin && echo \*.bin >>.gitignore What the user might have said in the "configuration" is the default suggestion, that is much weaker than what has been done to the repository data. I think "the path is recorded in the index" in the ignore/exclude situation is similar to "the submodule is initialized in the working tree" in this context. > Well it can get out of sync by not touching it as well, because others > changed the submodule pointer who are interested in that though. Which "others" are mucking with your working tree? (don't respond: I'll come to the answer a few lines below). > > # in the superproject > git checkout new-version > git submodule update > # checks out submodules to their version > > git checkout some-ancient-version > git submodule update > # this would only update the submodules in the defaultGroup, > # not those which are initialized but "uninteresting" > # the labeling may have changed between the different versions I see. I think that is where the conceptual bug lies. Thanks for an illustration. If we take an approach similar to ignore/exclude, then yes the "default" action should be done to "defaultGroup" specified plus what the user instantiated in the working tree already. And that is not limited to "update" operation. Just like "git diff" is not the only thing that would show difference in foo.bin in the working tree even when *.bin is ignored, but we consistently treat foo.bin as tracked. > The state of a submodule (un-initialized, initialized, checked out) > doesn't change the index or anything. Only the working tree is affected. > > And by flipping between the initialized and checked-out state we do not > lose any information (such as user configured remote urls) nor does it > affect the "state" (index, recorded tree, history) of the repository. Users want to initialize a module and keep it checked out even if they do intend to keep it pristine and not making any changes themselves, only so that other parts of the tree that depends on the module can be built. So "removing a tracked and unmodified thing from the working tree does not hurt users" is a nonsense argument, isn't it? I would be very unhappy if "git status" removed pristine paths from the working tree and toggle assume-unchanged bit in my index automatically. I am afraid you are focused too much on "version control" and losing sight of those who use the data stored in "version control", perhaps because you worked in submodule area too long and too hard? -- 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] exec_cmd.c, sideband.c, Makefile: avoid multiple PREFIX definitions
Philip Oakley writes:
> The short and sweet PREFIX can be confused when used in many places.
>
> Rename both usages to better describe their purpose.
>
> Noticed when compiling Git for Windows using MSVC/Visual Studio which
> reports the conflict beteeen the command line definition and the
> definition in sideband.c
Good eyes.
>
> Signed-off-by: Philip Oakley
> ---
> Makefile | 2 +-
> exec_cmd.c | 4 ++--
> sideband.c | 6 +++---
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 33b0f76..bcdd3ec 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1973,7 +1973,7 @@ exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
> exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
> '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
> '-DBINDIR="$(bindir_relative_SQ)"' \
> - '-DPREFIX="$(prefix_SQ)"'
> + '-DEXEC_PREFIX="$(prefix_SQ)"'
I am not entirely happy with this name as the name can be easily
confused with GIT_EXEC_PATH.
This is a fallback used under RUNTIME_PREFIX option, if I am reading
the code correctly, so the name should hint the linkage between the
"runtime prefix" mechanism and this variable.
Perhaps RUNTIME_PREFIX_FALLBACK? Used at only two places, that
should not be an overlong name.
DISPLAY_PREFIX is OK, as it is an entirely a local thing to
sideband.c, but with the externally visible PREFIX fixed to be a
more appropriate name that hints its relation with the "runtime
prefix" mechanism, it may be better not to even touch it.
> builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
> builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 9d5703a..2a79781 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -12,7 +12,7 @@ char *system_path(const char *path)
> #ifdef RUNTIME_PREFIX
> static const char *prefix;
> #else
> - static const char *prefix = PREFIX;
> + static const char *prefix = EXEC_PREFIX;
> #endif
> struct strbuf d = STRBUF_INIT;
>
> @@ -27,7 +27,7 @@ char *system_path(const char *path)
> !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&
> !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&
> !(prefix = strip_path_suffix(argv0_path, "git"))) {
> - prefix = PREFIX;
> + prefix = EXEC_PREFIX;
> trace_printf("RUNTIME_PREFIX requested, "
> "but prefix computation failed. "
> "Using static fallback '%s'.\n", prefix);
--
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 01/11] add fetch-pack --diag-url tests for some corner cases
On Wed, May 04, 2016 at 07:48:30AM +0900, Mike Hommey wrote: > On Tue, May 03, 2016 at 09:07:41AM -0700, Junio C Hamano wrote: > > Mike Hommey writes: > > > > > t5603-clone-dirname uses url patterns that are not tested with > > > fetch-pack --diag-url, and it would be useful if they were. > > > > > > Interestingly, some of those tests, involving both a port and a > > > user:password pair, don't currently pass. Note that even if a > > > user:password pair is actually not supported by git, the values used > > > could be valid user names (user names can actually contain colons > > > and at signs), and are still worth testing the url parser for. > > > > I am not sure about the last part of this (and the tests in the > > patch for them). When you are constrained by the Common Internet > > Scheme Syntax, i.e. > > > > ://:@:/ > > > > you cannot have arbitrary characters in these parts; within the user > > and password field, any ":", "@", or "/" must be encoded. > > > > Which maens that for the purpose of the parser you are modifying, > > you can rely on these three special characters to parse things out > > (decoding after the code determines which part is user and which > > part is password is a separate issue). > > t5603-clone-dirname contains a test for e.g. ssh://user:passw@rd@host:1234/ > That's the basis for these additions. Whether that should work or not is > besides what I was interested in, which was to have a single test file to > run to test my changes, instead of several. > > Strictly speaking, this patch is not necessary, because it only covers > things that I found while breaking other tests. > > So, there are multiple possible ways forward here: > - Completely remove this patch for v5 of the series. > - Remove the user:passw@rd cases because of the @. > - Remove the user:password cases because we do nothing with the password > anyways. > - A combination of both of the above. Any opinions on this? Mike -- 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/3] test-parse-options: fix output when callback option fails
When test-parse-options detects an error on the command line, it
gives the usage string just like any parse-options API users do,
without showing any "variable dump". An exception is the callback
test, where a "variable dump" for the option is done before the
command line options are fully parsed.
Do not expose this implementation detail by separating the handling
of callback test into two phases, one to capture the fact that an
option was given during the option parsing phase, and the other to
show that fact as a part of normal "variable dump".
The effect of this fix is seen in the patch to t/t0040 where it
tried "test-parse-options --no-length" where "--length" is a callback
that does not take a negative form.
Signed-off-by: Junio C Hamano
---
t/t0040-parse-options.sh | 4 +---
test-parse-options.c | 18 --
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index fec3fef..dbaee55 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -356,9 +356,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
test_cmp expect output
'
-cat >expect <<\EOF
-Callback: "not set", 1
-EOF
+>expect
test_expect_success 'OPT_CALLBACK() and callback errors work' '
test_must_fail test-parse-options --no-length >output 2>output.err &&
diff --git a/test-parse-options.c b/test-parse-options.c
index f02c275..b5f4e90 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -14,10 +14,18 @@ static char *file = NULL;
static int ambiguous;
static struct string_list list;
+static struct {
+ int called;
+ const char *arg;
+ int unset;
+} length_cb;
+
static int length_callback(const struct option *opt, const char *arg, int
unset)
{
- printf("Callback: \"%s\", %d\n",
- (arg ? arg : "not set"), unset);
+ length_cb.called = 1;
+ length_cb.arg = arg;
+ length_cb.unset = unset;
+
if (unset)
return 1; /* do not support unset */
@@ -84,6 +92,12 @@ int main(int argc, char **argv)
argc = parse_options(argc, (const char **)argv, prefix, options, usage,
0);
+ if (length_cb.called) {
+ const char *arg = length_cb.arg;
+ int unset = length_cb.unset;
+ printf("Callback: \"%s\", %d\n",
+ (arg ? arg : "not set"), unset);
+ }
printf("boolean: %d\n", boolean);
printf("integer: %d\n", integer);
printf("magnitude: %lu\n", magnitude);
--
2.8.2-505-gdbd0e1d
--
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/3] test-parse-options update
During the review of Pranit's "commit.verbose" series, I noticed an overly verbose input and output used to drive test-parse-options helper in t0040. Here is a patch to teach the program to allow us to write the test in a more concise way. I'll leave it as an exercise to the readers to actually use this to convert tests in t0040. That needs to wait until Pranit's series is merged and the dust settles. Junio C Hamano (3): test-parse-options: fix output when callback option fails test-parse-options: hold output in a strbuf test-parse-options: --expect= option to simplify tests t/t0040-parse-options.sh | 5 +-- test-parse-options.c | 110 --- 2 files changed, 97 insertions(+), 18 deletions(-) -- 2.8.2-505-gdbd0e1d -- 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/3] test-parse-options: --expect= option to simplify tests
Existing tests in t0040 follow a rather verbose pattern:
cat >expect <<\EOF
boolean: 0
integer: 0
magnitude: 0
timestamp: 0
string: (not set)
abbrev: 7
verbose: 0
quiet: 3
dry run: no
file: (not set)
EOF
test_expect_success 'multiple quiet levels' '
test-parse-options -q -q -q >output 2>output.err &&
test_must_be_empty output.err &&
test_cmp expect output
'
But the only thing this test cares about is if "quiet: 3" is in the
output. We should be able to write the above 18 lines with just
four lines, like this:
test_expect_success 'multiple quiet levels' '
test-parse-options --expect="quiet: 3" -q -q -q
'
Teach the new --expect= option to test-parse-options helper.
Signed-off-by: Junio C Hamano
---
t/t0040-parse-options.sh | 1 +
test-parse-options.c | 68 +---
2 files changed, 66 insertions(+), 3 deletions(-)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index dbaee55..d678fbf 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -45,6 +45,7 @@ Standard options
-v, --verbose be verbose
-n, --dry-run dry run
-q, --quiet be quiet
+--expect expected output in the variable dump
EOF
diff --git a/test-parse-options.c b/test-parse-options.c
index 3db4332..010f3b2 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -14,6 +14,7 @@ static char *string = NULL;
static char *file = NULL;
static int ambiguous;
static struct string_list list;
+static struct string_list expect;
static struct {
int called;
@@ -40,6 +41,62 @@ static int number_callback(const struct option *opt, const
char *arg, int unset)
return 0;
}
+/*
+ * See if expect->string ("label: value") has a line in output that
+ * begins with "label:", and if the line in output matches it.
+ */
+static int match_line(struct string_list_item *expect, struct strbuf *output)
+{
+ const char *label = expect->string;
+ const char *colon = strchr(label, ':');
+ const char *scan = output->buf;
+ size_t label_len, expect_len;
+
+ if (!colon)
+ die("Malformed --expect value: %s", label);
+ label_len = colon - label;
+
+ while (scan < output->buf + output->len) {
+ const char *next;
+ scan = memmem(scan, output->buf + output->len - scan,
+ label, label_len);
+ if (!scan)
+ return 0;
+ if (scan == output->buf || scan[-1] == '\n')
+ break;
+ next = strchr(scan + label_len, '\n');
+ if (!next)
+ return 0;
+ scan = next + 1;
+ }
+
+ /*
+* scan points at a line that begins with the label we are
+* looking for. Does it match?
+*/
+ expect_len = strlen(expect->string);
+
+ if (output->buf + output->len <= scan + expect_len)
+ return 0; /* value not long enough */
+ if (memcmp(scan, expect->string, expect_len))
+ return 0; /* does not match */
+
+ return (scan + expect_len < output->buf + output->len &&
+ scan[expect_len] == '\n');
+}
+
+static int show_expected(struct string_list *list, struct strbuf *output)
+{
+ struct string_list_item *expect;
+ int found_mismatch = 0;
+
+ for_each_string_list_item(expect, list) {
+ if (!match_line(expect, output))
+ found_mismatch = 1;
+ }
+ return found_mismatch;
+}
+
int main(int argc, char **argv)
{
const char *prefix = "prefix/";
@@ -87,6 +144,8 @@ int main(int argc, char **argv)
OPT__VERBOSE(&verbose, "be verbose"),
OPT__DRY_RUN(&dry_run, "dry run"),
OPT__QUIET(&quiet, "be quiet"),
+ OPT_STRING_LIST(0, "expect", &expect, "string",
+ "expected output in the variable dump"),
OPT_END(),
};
int i;
@@ -117,7 +176,10 @@ int main(int argc, char **argv)
for (i = 0; i < argc; i++)
strbuf_addf(&output, "arg %02d: %s\n", i, argv[i]);
- printf("%s", output.buf);
-
- return 0;
+ if (expect.nr)
+ return show_expected(&expect, &output);
+ else {
+ printf("%s", output.buf);
+ return 0;
+ }
}
--
2.8.2-505-gdbd0e1d
--
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/3] test-parse-options: hold output in a strbuf
In this step, all the output is held in a strbuf and unconditionally
dumped at the end, so there is no behaviour change (other than that
the processing may be a bit slower, now we do the buffering stdio
has been doing for us).
Signed-off-by: Junio C Hamano
---
test-parse-options.c | 30 +-
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/test-parse-options.c b/test-parse-options.c
index b5f4e90..3db4332 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -1,6 +1,7 @@
#include "cache.h"
#include "parse-options.h"
#include "string-list.h"
+#include "strbuf.h"
static int boolean = 0;
static int integer = 0;
@@ -89,31 +90,34 @@ int main(int argc, char **argv)
OPT_END(),
};
int i;
+ struct strbuf output = STRBUF_INIT;
argc = parse_options(argc, (const char **)argv, prefix, options, usage,
0);
if (length_cb.called) {
const char *arg = length_cb.arg;
int unset = length_cb.unset;
- printf("Callback: \"%s\", %d\n",
+ strbuf_addf(&output, "Callback: \"%s\", %d\n",
(arg ? arg : "not set"), unset);
}
- printf("boolean: %d\n", boolean);
- printf("integer: %d\n", integer);
- printf("magnitude: %lu\n", magnitude);
- printf("timestamp: %lu\n", timestamp);
- printf("string: %s\n", string ? string : "(not set)");
- printf("abbrev: %d\n", abbrev);
- printf("verbose: %d\n", verbose);
- printf("quiet: %d\n", quiet);
- printf("dry run: %s\n", dry_run ? "yes" : "no");
- printf("file: %s\n", file ? file : "(not set)");
+ strbuf_addf(&output, "boolean: %d\n", boolean);
+ strbuf_addf(&output, "integer: %d\n", integer);
+ strbuf_addf(&output, "magnitude: %lu\n", magnitude);
+ strbuf_addf(&output, "timestamp: %lu\n", timestamp);
+ strbuf_addf(&output, "string: %s\n", string ? string : "(not set)");
+ strbuf_addf(&output, "abbrev: %d\n", abbrev);
+ strbuf_addf(&output, "verbose: %d\n", verbose);
+ strbuf_addf(&output, "quiet: %d\n", quiet);
+ strbuf_addf(&output, "dry run: %s\n", dry_run ? "yes" : "no");
+ strbuf_addf(&output, "file: %s\n", file ? file : "(not set)");
for (i = 0; i < list.nr; i++)
- printf("list: %s\n", list.items[i].string);
+ strbuf_addf(&output, "list: %s\n", list.items[i].string);
for (i = 0; i < argc; i++)
- printf("arg %02d: %s\n", i, argv[i]);
+ strbuf_addf(&output, "arg %02d: %s\n", i, argv[i]);
+
+ printf("%s", output.buf);
return 0;
}
--
2.8.2-505-gdbd0e1d
--
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 v8 16/19] index-helper: autorun mode
Soon, we'll want to automatically start index-helper, so we need
a mode that silently exits if it can't start up (either because
it's not in a git dir, or because another one is already running).
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 4
index-helper.c | 29 +++--
t/t7900-index-helper.sh| 8
3 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index addf694..0466296 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -43,6 +43,10 @@ OPTIONS
--kill::
Kill any running index-helper and clean up the socket
+--autorun::
+ If index-helper is not already running, start it. Else, do
+ nothing.
+
NOTES
-
diff --git a/index-helper.c b/index-helper.c
index b275f6e..9743481 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -427,8 +427,9 @@ static void request_kill(void)
int main(int argc, char **argv)
{
const char *prefix;
- int idle_in_seconds = 600, detach = 0, kill = 0;
+ int idle_in_seconds = 600, detach = 0, kill = 0, autorun = 0;
int fd;
+ int nongit;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
OPT_INTEGER(0, "exit-after", &idle_in_seconds,
@@ -437,6 +438,7 @@ int main(int argc, char **argv)
"verify shared memory after creating"),
OPT_BOOL(0, "detach", &detach, "detach the process"),
OPT_BOOL(0, "kill", &kill, "request that existing index helper
processes exit"),
+ OPT_BOOL(0, "autorun", &autorun, "this is an automatic run of
git index-helper, so certain errors can be solved by silently exiting"),
OPT_END()
};
@@ -446,7 +448,14 @@ int main(int argc, char **argv)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(usage_text, options);
- prefix = setup_git_directory();
+ prefix = setup_git_directory_gently(&nongit);
+ if (nongit) {
+ if (autorun)
+ exit(0);
+ else
+ die(_("not a git repository"));
+ }
+
if (parse_options(argc, (const char **)argv, prefix,
options, usage_text, 0))
die(_("too many arguments"));
@@ -460,10 +469,18 @@ int main(int argc, char **argv)
/* check that no other copy is running */
fd = unix_stream_connect(git_path("index-helper.sock"));
- if (fd > 0)
- die(_("Already running"));
- if (errno != ECONNREFUSED && errno != ENOENT)
- die_errno(_("Unexpected error checking socket"));
+ if (fd > 0) {
+ if (autorun)
+ exit(0);
+ else
+ die(_("Already running"));
+ }
+ if (errno != ECONNREFUSED && errno != ENOENT) {
+ if (autorun)
+ return 0;
+ else
+ die_errno(_("Unexpected error checking socket"));
+ }
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index 7159971..aa6e5fc 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -38,4 +38,12 @@ test_expect_success 'index-helper will not start if already
running' '
grep "Already running" err
'
+test_expect_success 'index-helper is quiet with --autorun' '
+ test_when_finished "git index-helper --kill" &&
+ git index-helper --kill &&
+ git index-helper --detach &&
+ test -S .git/index-helper.sock &&
+ git index-helper --autorun
+'
+
test_done
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 04/19] index-helper: add --strict
From: Nguyễn Thái Ngọc Duy
There are "holes" in the index-helper approach because the shared
memory is not verified again by git. If $USER is compromised, shared
memory could be modified. But anyone who could do this could already
modify $GIT_DIR/index. A more realistic risk is some bugs in
index-helper that produce corrupt shared memory. --strict is added to
avoid that.
Strictly speaking there's still a very small gap where corrupt shared
memory could still be read by git: after we write the trailing SHA-1 in
the shared memory (thus signaling "this shm is ready") and before
verify_shm() detects an error.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 9 +++
cache.h| 1 +
index-helper.c | 48 ++
read-cache.c | 9 ---
4 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index f892184..1f92c89 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -25,6 +25,15 @@ OPTIONS
Exit if the cached index is not accessed for ``
seconds. Specify 0 to wait forever. Default is 600.
+--strict::
+--no-strict::
+ Strict mode makes index-helper verify the shared memory after
+ it's created. If the result does not match what's read from
+ $GIT_DIR/index, the shared memory is destroyed. This makes
+ index-helper take more than double the amount of time required
+ for reading an index, but because it will happen in the
+ background, it's not noticable. `--strict` is enabled by default.
+
NOTES
-
diff --git a/cache.h b/cache.h
index 2d7af6f..6cb0d02 100644
--- a/cache.h
+++ b/cache.h
@@ -345,6 +345,7 @@ struct index_state {
* on it.
*/
to_shm : 1,
+always_verify_trailing_sha1 : 1,
initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
diff --git a/index-helper.c b/index-helper.c
index b9443f4..bc7e110 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -17,6 +17,7 @@ struct shm {
static struct shm shm_index;
static struct shm shm_base_index;
+static int to_verify = 1;
static void release_index_shm(struct shm *is)
{
@@ -114,11 +115,56 @@ static void share_index(struct index_state *istate,
struct shm *is)
hashcpy((unsigned char *)new_mmap + istate->mmap_size - 20, is->sha1);
}
+static int verify_shm(void)
+{
+ int i;
+ struct index_state istate;
+ memset(&istate, 0, sizeof(istate));
+ istate.always_verify_trailing_sha1 = 1;
+ istate.to_shm = 1;
+ i = read_index(&istate);
+ if (i != the_index.cache_nr)
+ goto done;
+ for (; i < the_index.cache_nr; i++) {
+ struct cache_entry *base, *ce;
+ /* namelen is checked separately */
+ const unsigned int ondisk_flags =
+ CE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;
+ unsigned int ce_flags, base_flags, ret;
+ base = the_index.cache[i];
+ ce = istate.cache[i];
+ if (ce->ce_namelen != base->ce_namelen ||
+ strcmp(ce->name, base->name)) {
+ warning("mismatch at entry %d", i);
+ break;
+ }
+ ce_flags = ce->ce_flags;
+ base_flags = base->ce_flags;
+ /* only on-disk flags matter */
+ ce->ce_flags &= ondisk_flags;
+ base->ce_flags &= ondisk_flags;
+ ret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,
+offsetof(struct cache_entry, name) -
+offsetof(struct cache_entry, ce_stat_data));
+ ce->ce_flags = ce_flags;
+ base->ce_flags = base_flags;
+ if (ret) {
+ warning("mismatch at entry %d", i);
+ break;
+ }
+ }
+done:
+ discard_index(&istate);
+ return i == the_index.cache_nr;
+}
+
static void share_the_index(void)
{
if (the_index.split_index && the_index.split_index->base)
share_index(the_index.split_index->base, &shm_base_index);
share_index(&the_index, &shm_index);
+ if (to_verify && !verify_shm())
+ cleanup_shm();
discard_index(&the_index);
}
@@ -247,6 +293,8 @@ int main(int argc, char **argv)
struct option options[] = {
OPT_INTEGER(0, "exit-after", &idle_in_seconds,
N_("exit if not used after some seconds")),
+ OPT_BOOL(0, "strict", &to_verify,
+"verify shared memory after creating"),
OPT_END()
};
diff --git a/read-cache.c
[PATCH v8 05/19] index-helper: log warnings
Instead of writing warnings to stderr, write them to a log. Later, we'll
probably be daemonized, so writing to stderr will be pointless.
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 3 +++
index-helper.c | 32
2 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index 1f92c89..9c2f5eb 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -54,6 +54,9 @@ command. The following commands are used to control the
daemon:
All commands and replies are terminated by a NUL byte.
+In the event of an error, messages may be written to
+$GIT_DIR/index-helper.log.
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/index-helper.c b/index-helper.c
index bc7e110..21fb431 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -19,6 +19,29 @@ static struct shm shm_index;
static struct shm shm_base_index;
static int to_verify = 1;
+static void log_warning(const char *warning, ...)
+{
+ va_list ap;
+ FILE *fp;
+
+ fp = fopen(git_path("index-helper.log"), "a");
+ if (!fp)
+ /*
+* Probably nobody will see this, but it's the best
+* we can do.
+*/
+ die("Failed to open log for warnings");
+
+ fprintf(fp, "%"PRIuMAX"\t", (uintmax_t)time(NULL));
+
+ va_start(ap, warning);
+ vfprintf(fp, warning, ap);
+ va_end(ap);
+
+ fputc('\n', fp);
+ fclose(fp);
+}
+
static void release_index_shm(struct shm *is)
{
if (!is->shm)
@@ -93,7 +116,8 @@ static void share_index(struct index_state *istate, struct
shm *is)
if (shared_mmap_create(istate->mmap_size, &new_mmap,
git_path("shm-index-%s",
sha1_to_hex(istate->sha1))) < 0) {
- die("Failed to create shm-index file");
+ log_warning("Failed to create shm-index file");
+ exit(1);
}
@@ -135,7 +159,7 @@ static int verify_shm(void)
ce = istate.cache[i];
if (ce->ce_namelen != base->ce_namelen ||
strcmp(ce->name, base->name)) {
- warning("mismatch at entry %d", i);
+ log_warning("mismatch at entry %d", i);
break;
}
ce_flags = ce->ce_flags;
@@ -149,7 +173,7 @@ static int verify_shm(void)
ce->ce_flags = ce_flags;
base->ce_flags = base_flags;
if (ret) {
- warning("mismatch at entry %d", i);
+ log_warning("mismatch at entry %d", i);
break;
}
}
@@ -255,7 +279,7 @@ static void loop(int fd, int idle_in_seconds)
* alive, nothing to do.
*/
} else {
- warning("BUG: Bogus command %s", buf);
+ log_warning("BUG: Bogus command %s", buf);
}
} else {
/*
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 06/19] daemonize(): set a flag before exiting the main process
From: Nguyễn Thái Ngọc Duy
This allows signal handlers and atexit functions to realize this
situation and not clean up.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
builtin/gc.c | 2 +-
cache.h | 2 +-
daemon.c | 2 +-
setup.c | 4 +++-
4 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad..37180de 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -385,7 +385,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
* failure to daemonize is ok, we'll continue
* in foreground
*/
- daemonized = !daemonize();
+ daemonized = !daemonize(NULL);
}
} else
add_repack_all_option();
diff --git a/cache.h b/cache.h
index 6cb0d02..4c1529a 100644
--- a/cache.h
+++ b/cache.h
@@ -539,7 +539,7 @@ extern int set_git_dir_init(const char *git_dir, const char
*real_git_dir, int);
extern int init_db(const char *template_dir, unsigned int flags);
extern void sanitize_stdfds(void);
-extern int daemonize(void);
+extern int daemonize(int *);
#define alloc_nr(x) (((x)+16)*3/2)
diff --git a/daemon.c b/daemon.c
index 8d45c33..a5cf954 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1365,7 +1365,7 @@ int main(int argc, char **argv)
return execute();
if (detach) {
- if (daemonize())
+ if (daemonize(NULL))
die("--detach not supported on this platform");
} else
sanitize_stdfds();
diff --git a/setup.c b/setup.c
index de1a2a7..9adf13f 100644
--- a/setup.c
+++ b/setup.c
@@ -1017,7 +1017,7 @@ void sanitize_stdfds(void)
close(fd);
}
-int daemonize(void)
+int daemonize(int *daemonized)
{
#ifdef NO_POSIX_GOODIES
errno = ENOSYS;
@@ -1029,6 +1029,8 @@ int daemonize(void)
case -1:
die_errno("fork failed");
default:
+ if (daemonized)
+ *daemonized = 1;
exit(0);
}
if (setsid() == -1)
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 15/19] index-helper: don't run if already running
Signed-off-by: David Turner
---
index-helper.c | 7 +++
t/t7900-index-helper.sh | 9 +
2 files changed, 16 insertions(+)
diff --git a/index-helper.c b/index-helper.c
index 4ed1610..b275f6e 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -458,6 +458,13 @@ int main(int argc, char **argv)
return 0;
}
+ /* check that no other copy is running */
+ fd = unix_stream_connect(git_path("index-helper.sock"));
+ if (fd > 0)
+ die(_("Already running"));
+ if (errno != ECONNREFUSED && errno != ENOENT)
+ die_errno(_("Unexpected error checking socket"));
+
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index e71b5af..7159971 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -29,4 +29,13 @@ test_expect_success 'index-helper creates usable path file
and can be killed' '
test_path_is_missing .git/index-helper.sock
'
+test_expect_success 'index-helper will not start if already running' '
+ test_when_finished "git index-helper --kill" &&
+ git index-helper --detach &&
+ test -S .git/index-helper.sock &&
+ test_must_fail git index-helper 2>err &&
+ test -S .git/index-helper.sock &&
+ grep "Already running" err
+'
+
test_done
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 08/19] read-cache: add watchman 'WAMA' extension
From: Nguyễn Thái Ngọc Duy
The extension contains a bitmap, one bit for each entry in the
index. If the n-th bit is zero, the n-th entry is considered
unchanged, we can ce_mark_uptodate() it without refreshing. If the bit
is non-zero and we found out the corresponding file is clean after
refresh, we can clear the bit.
In addition, there's a list of directories in the untracked-cache
to invalidate (because they have new or modified entries).
The 'skipping refresh' bit is not in this patch yet as we would need
watchman. More details in later patches.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/technical/index-format.txt | 22 ++
cache.h | 4 ++
dir.h| 3 +
read-cache.c | 117 ++-
4 files changed, 144 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/index-format.txt
b/Documentation/technical/index-format.txt
index ade0b0c..86ed3a6 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -295,3 +295,25 @@ The remaining data of each directory block is grouped by
type:
in the previous ewah bitmap.
- One NUL.
+
+== Watchman cache
+
+ The watchman cache tracks files for which watchman has told us about
+ changes. The signature for this extension is { 'W', 'A', 'M', 'A' }.
+
+ The extension starts with
+
+ - A NUL-terminated string: the watchman vector clock at the last
+time we heard from watchman.
+
+ - 32-bit bitmap size: the size of the CE_WATCHMAN_DIRTY bitmap
+
+ - 32-bit untracked cache entry count: the number of dirty untracked
+cache entries
+
+ - An ewah bitmap, the n-th bit indicates whether the n-th index entry
+is CE_WATCHMAN_DIRTY.
+
+ - a list of N NUL-terminated strings. Each is a directory that should
+be marked dirty in the untracked cache because watchman has told us
+about an update to a file in it.
diff --git a/cache.h b/cache.h
index 4c1529a..f10992d 100644
--- a/cache.h
+++ b/cache.h
@@ -182,6 +182,8 @@ struct cache_entry {
#define CE_VALID (0x8000)
#define CE_STAGESHIFT 12
+#define CE_WATCHMAN_DIRTY (0x0001)
+
/*
* Range 0x0FFF in ce_flags is divided into
* two parts: in-memory flags and on-disk ones.
@@ -320,6 +322,7 @@ static inline unsigned int canon_mode(unsigned int mode)
#define CACHE_TREE_CHANGED (1 << 5)
#define SPLIT_INDEX_ORDERED(1 << 6)
#define UNTRACKED_CHANGED (1 << 7)
+#define WATCHMAN_CHANGED (1 << 8)
struct split_index;
struct untracked_cache;
@@ -353,6 +356,7 @@ struct index_state {
struct untracked_cache *untracked;
void *mmap;
size_t mmap_size;
+ char *last_update;
};
extern struct index_state the_index;
diff --git a/dir.h b/dir.h
index 3ec3fb0..3d540de 100644
--- a/dir.h
+++ b/dir.h
@@ -142,6 +142,9 @@ struct untracked_cache {
int gitignore_invalidated;
int dir_invalidated;
int dir_opened;
+ /* watchman invalidation data */
+ unsigned int use_watchman : 1;
+ struct string_list invalid_untracked;
};
struct dir_struct {
diff --git a/read-cache.c b/read-cache.c
index d7849d6..e640098 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -21,6 +21,7 @@
#include "unix-socket.h"
#include "pkt-line.h"
#include "sigchain.h"
+#include "ewah/ewok.h"
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
unsigned int options);
@@ -43,11 +44,13 @@ static struct cache_entry *refresh_cache_entry(struct
cache_entry *ce,
#define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* "REUC" */
#define CACHE_EXT_LINK 0x6c696e6b/* "link" */
#define CACHE_EXT_UNTRACKED 0x554E5452 /* "UNTR" */
+#define CACHE_EXT_WATCHMAN 0x57414D41/* "WAMA" */
/* changes that can be kept in $GIT_DIR/index (basically all extensions) */
#define EXTMASK (RESOLVE_UNDO_CHANGED | CACHE_TREE_CHANGED | \
CE_ENTRY_ADDED | CE_ENTRY_REMOVED | CE_ENTRY_CHANGED | \
-SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED)
+SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED | \
+WATCHMAN_CHANGED)
struct index_state the_index;
static const char *alternate_index_output;
@@ -1222,8 +1225,13 @@ int refresh_index(struct index_state *istate, unsigned
int flags,
continue;
new = refresh_cache_ent(istate, ce, options, &cache_errno,
&changed);
- if (new == ce)
+ if (new == ce) {
+ if (ce->ce_flags & CE_WATCHMAN_DIRTY) {
+ ce->ce_flags &= ~CE_WATCHMAN_DIRTY;
+ istate->cache_changed |= WATCHMAN_CHANGED;
+ }
continue;
+ }
if (!new) {
const char *fmt;
@@ -1367,6 +13
[PATCH v8 02/19] read-cache: allow to keep mmap'd memory after reading
From: Nguyễn Thái Ngọc Duy
Later, we will introduce git index-helper to share this memory with
other git processes.
We only unmap it when we discard the index (although the kernel may of
course choose to page it out).
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
cache.h | 3 +++
read-cache.c | 13 -
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/cache.h b/cache.h
index b829410..4180e2b 100644
--- a/cache.h
+++ b/cache.h
@@ -333,11 +333,14 @@ struct index_state {
struct split_index *split_index;
struct cache_time timestamp;
unsigned name_hash_initialized : 1,
+keep_mmap : 1,
initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
unsigned char sha1[20];
struct untracked_cache *untracked;
+ void *mmap;
+ size_t mmap_size;
};
extern struct index_state the_index;
diff --git a/read-cache.c b/read-cache.c
index 16cc487..3cb0ec3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1574,6 +1574,10 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist)
mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (mmap == MAP_FAILED)
die_errno("unable to map index file");
+ if (istate->keep_mmap) {
+ istate->mmap = mmap;
+ istate->mmap_size = mmap_size;
+ }
close(fd);
hdr = mmap;
@@ -1626,11 +1630,13 @@ int do_read_index(struct index_state *istate, const
char *path, int must_exist)
src_offset += 8;
src_offset += extsize;
}
- munmap(mmap, mmap_size);
+ if (!istate->keep_mmap)
+ munmap(mmap, mmap_size);
return istate->cache_nr;
unmap:
munmap(mmap, mmap_size);
+ istate->mmap = NULL;
die("index file corrupt");
}
@@ -1655,6 +1661,7 @@ int read_index_from(struct index_state *istate, const
char *path)
discard_index(split_index->base);
else
split_index->base = xcalloc(1, sizeof(*split_index->base));
+ split_index->base->keep_mmap = istate->keep_mmap;
ret = do_read_index(split_index->base,
git_path("sharedindex.%s",
sha1_to_hex(split_index->base_sha1)), 1);
@@ -1698,6 +1705,10 @@ int discard_index(struct index_state *istate)
free(istate->cache);
istate->cache = NULL;
istate->cache_alloc = 0;
+ if (istate->keep_mmap && istate->mmap) {
+ munmap(istate->mmap, istate->mmap_size);
+ istate->mmap = NULL;
+ }
discard_split_index(istate);
free_untracked_cache(istate->untracked);
istate->untracked = NULL;
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 18/19] Add tracing to measure where most of the time is spent
From: Nguyễn Thái Ngọc Duy
All the known heavy code blocks are measured (except object database
access). This should help identify if an optimization is effective or
not. An unoptimized git-status would give something like below (92% of
time is accounted). To sum up the effort of making git scale better:
- read cache line is being addressed by index-helper
- preload/refresh index by watchman
- read packed refs by lmdb backend
- diff-index by rebuilding cache-tree
- read directory by untracked cache and watchman
- write index by split index
- name hash potientially by index-helper
read-cache.c:2075 performance: 0.004058570 s: read cache .../index
preload-index.c:104 performance: 0.004419864 s: preload index
read-cache.c:1265 performance: 0.000185224 s: refresh index
refs/files-backend.c:1100 performance: 0.001935788 s: read packed refs
diff-lib.c:240performance: 0.000144132 s: diff-files
diff-lib.c:506performance: 0.013592000 s: diff-index
name-hash.c:128 performance: 0.000614177 s: initialize name hash
dir.c:2030performance: 0.015814103 s: read directory
read-cache.c:2565 performance: 0.004052343 s: write index, changed mask
= 2
trace.c:420 performance: 0.048365509 s: git command: './git'
'status'
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
diff-lib.c | 4
dir.c| 2 ++
name-hash.c | 2 ++
preload-index.c | 2 ++
read-cache.c | 11 +++
refs/files-backend.c | 2 ++
6 files changed, 23 insertions(+)
diff --git a/diff-lib.c b/diff-lib.c
index bc49c70..7af7f9a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -90,6 +90,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
int diff_unmerged_stage = revs->max_count;
unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
? CE_MATCH_RACY_IS_DIRTY : 0);
+ uint64_t start = getnanotime();
diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
@@ -236,6 +237,7 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
}
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
+ trace_performance_since(start, "diff-files");
return 0;
}
@@ -491,6 +493,7 @@ static int diff_cache(struct rev_info *revs,
int run_diff_index(struct rev_info *revs, int cached)
{
struct object_array_entry *ent;
+ uint64_t start = getnanotime();
ent = revs->pending.objects;
if (diff_cache(revs, ent->item->oid.hash, ent->name, cached))
@@ -500,6 +503,7 @@ int run_diff_index(struct rev_info *revs, int cached)
diffcore_fix_diff_index(&revs->diffopt);
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
+ trace_performance_since(start, "diff-index");
return 0;
}
diff --git a/dir.c b/dir.c
index 5058b29..c56a8b9 100644
--- a/dir.c
+++ b/dir.c
@@ -2183,6 +2183,7 @@ int read_directory(struct dir_struct *dir, const char
*path, int len, const stru
{
struct path_simplify *simplify;
struct untracked_cache_dir *untracked;
+ uint64_t start = getnanotime();
/*
* Check out create_simplify()
@@ -2224,6 +2225,7 @@ int read_directory(struct dir_struct *dir, const char
*path, int len, const stru
free_simplify(simplify);
qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *),
cmp_name);
+ trace_performance_since(start, "read directory %.*s", len, path);
if (dir->untracked) {
static struct trace_key trace_untracked_stats =
TRACE_KEY_INIT(UNTRACKED_STATS);
trace_printf_key(&trace_untracked_stats,
diff --git a/name-hash.c b/name-hash.c
index 6d9f23e..b3966d8 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -115,6 +115,7 @@ static int cache_entry_cmp(const struct cache_entry *ce1,
static void lazy_init_name_hash(struct index_state *istate)
{
int nr;
+ uint64_t start = getnanotime();
if (istate->name_hash_initialized)
return;
@@ -124,6 +125,7 @@ static void lazy_init_name_hash(struct index_state *istate)
for (nr = 0; nr < istate->cache_nr; nr++)
hash_index_entry(istate, istate->cache[nr]);
istate->name_hash_initialized = 1;
+ trace_performance_since(start, "initialize name hash");
}
void add_name_hash(struct index_state *istate, struct cache_entry *ce)
diff --git a/preload-index.c b/preload-index.c
index c1fe3a3..7bb4809 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -72,6 +72,7 @@ static void preload_index(struct index_state *index,
{
int threads, i, work, offset;
struct thread_data data[MAX_PARALLEL];
+ uint64_t start = getnanotime();
if (!core_preload_index)
return;
@@ -100,6 +101,7 @@ static void p
[PATCH v8 13/19] watchman: add a config option to enable the extension
For installations that have centrally-managed configuration, it's
easier to set a config once than to run update-index on every
repository.
Signed-off-by: David Turner
---
.gitignore| 1 +
Documentation/config.txt | 4
Makefile | 1 +
read-cache.c | 6 ++
t/t1701-watchman-extension.sh | 37 +
test-dump-watchman.c | 16
6 files changed, 65 insertions(+)
create mode 100755 t/t1701-watchman-extension.sh
create mode 100644 test-dump-watchman.c
diff --git a/.gitignore b/.gitignore
index b92f122..e6a5b2c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -188,6 +188,7 @@
/test-dump-cache-tree
/test-dump-split-index
/test-dump-untracked-cache
+/test-dump-watchman
/test-fake-ssh
/test-scrap-cache-tree
/test-genrandom
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..15001ce 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,6 +1848,10 @@ imap::
The configuration variables in the 'imap' section are described
in linkgit:git-imap-send[1].
+index.addwatchmanextension::
+ Automatically add the watchman extension to the index whenever
+ it is written.
+
index.version::
Specify the version with which new index files should be
initialized. This does not affect existing repositories.
diff --git a/Makefile b/Makefile
index 65ab0f4..5f0a954 100644
--- a/Makefile
+++ b/Makefile
@@ -599,6 +599,7 @@ TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
TEST_PROGRAMS_NEED_X += test-dump-split-index
TEST_PROGRAMS_NEED_X += test-dump-untracked-cache
+TEST_PROGRAMS_NEED_X += test-dump-watchman
TEST_PROGRAMS_NEED_X += test-fake-ssh
TEST_PROGRAMS_NEED_X += test-genrandom
TEST_PROGRAMS_NEED_X += test-hashmap
diff --git a/read-cache.c b/read-cache.c
index 4ad2c19..b00919a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2429,6 +2429,7 @@ static int do_write_index(struct index_state *istate, int
newfd,
int entries = istate->cache_nr;
struct stat st;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
+ int watchman = 0;
for (i = removed = extended = 0; i < entries; i++) {
if (cache[i]->ce_flags & CE_REMOVE)
@@ -2452,6 +2453,11 @@ static int do_write_index(struct index_state *istate,
int newfd,
if (istate->version == 3 || istate->version == 2)
istate->version = extended ? 3 : 2;
+ if (!git_config_get_bool("index.addwatchmanextension", &watchman) &&
+ watchman &&
+ !the_index.last_update)
+ the_index.last_update = xstrdup("");
+
hdr_version = istate->version;
hdr.hdr_signature = htonl(CACHE_SIGNATURE);
diff --git a/t/t1701-watchman-extension.sh b/t/t1701-watchman-extension.sh
new file mode 100755
index 000..71f1d46
--- /dev/null
+++ b/t/t1701-watchman-extension.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='watchman extension smoke tests'
+
+# These don't actually test watchman interaction -- just the
+# index extension
+
+. ./test-lib.sh
+
+test_expect_success 'enable watchman' '
+ test_commit a &&
+ test-dump-watchman .git/index >actual &&
+ echo "last_update: (null)" >expect &&
+ test_cmp expect actual &&
+ git update-index --watchman &&
+ test-dump-watchman .git/index >actual &&
+ echo "last_update: " >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'disable watchman' '
+ git update-index --no-watchman &&
+ test-dump-watchman .git/index >actual &&
+ echo "last_update: (null)" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'auto-enable watchman' '
+ test_config index.addwatchmanextension true &&
+ test_commit c &&
+ test-dump-watchman .git/index >actual &&
+ echo "last_update: " >expect &&
+ test_cmp expect actual
+'
+
+
+test_done
diff --git a/test-dump-watchman.c b/test-dump-watchman.c
new file mode 100644
index 000..0314fa5
--- /dev/null
+++ b/test-dump-watchman.c
@@ -0,0 +1,16 @@
+#include "cache.h"
+#include "ewah/ewok.h"
+
+int main(int argc, char **argv)
+{
+ do_read_index(&the_index, argv[1], 1);
+ printf("last_update: %s\n", the_index.last_update ?
+ the_index.last_update : "(null)");
+
+ /*
+* For now, we just dump last_update, since it is not reasonable
+* to populate the extension itself in tests.
+*/
+
+ return 0;
+}
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 12/19] unpack-trees: preserve index extensions
Make git checkout (and other unpack_tree operations) preserve the
untracked cache and watchman status. This is valuable for two reasons:
1. Often, an unpack_tree operation will not touch large parts of the
working tree, and thus most of the untracked cache will continue to be
valid.
2. Even if the untracked cache were entirely invalidated by such an
operation, the user has signaled their intention to have such a cache,
and we don't want to throw it away.
The same logic applies to the watchman state.
Signed-off-by: David Turner
---
cache.h | 1 +
read-cache.c | 8
t/t7063-status-untracked-cache.sh | 22 ++
t/test-lib-functions.sh | 4
unpack-trees.c| 1 +
5 files changed, 36 insertions(+)
diff --git a/cache.h b/cache.h
index a167920..e65234d 100644
--- a/cache.h
+++ b/cache.h
@@ -580,6 +580,7 @@ extern void write_watchman_ext(struct strbuf *sb, struct
index_state* istate);
#define CLOSE_LOCK (1 << 1)
extern int write_locked_index(struct index_state *, struct lock_file *lock,
unsigned flags);
extern int discard_index(struct index_state *);
+extern void move_index_extensions(struct index_state *dst, struct index_state
*src);
extern int unmerged_index(const struct index_state *);
extern int verify_path(const char *path);
extern int index_dir_exists(struct index_state *istate, const char *name, int
namelen);
diff --git a/read-cache.c b/read-cache.c
index b44b18b..4ad2c19 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2770,3 +2770,11 @@ void stat_validity_update(struct stat_validity *sv, int
fd)
fill_stat_data(sv->sd, &st);
}
}
+
+void move_index_extensions(struct index_state *dst, struct index_state *src)
+{
+ dst->untracked = src->untracked;
+ src->untracked = NULL;
+ dst->last_update = src->last_update;
+ src->last_update = NULL;
+}
diff --git a/t/t7063-status-untracked-cache.sh
b/t/t7063-status-untracked-cache.sh
index a971884..083516d 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -646,4 +646,26 @@ test_expect_success 'test ident field is working' '
test_cmp ../expect ../err
'
+test_expect_success 'untracked cache survives a checkout' '
+ git commit --allow-empty -m empty &&
+ test-dump-untracked-cache >../before &&
+ test_when_finished "git checkout master" &&
+ git checkout -b other_branch &&
+ test-dump-untracked-cache >../after &&
+ test_cmp ../before ../after &&
+ test_commit test &&
+ test-dump-untracked-cache >../before &&
+ git checkout master &&
+ test-dump-untracked-cache >../after &&
+ test_cmp ../before ../after
+'
+
+test_expect_success 'untracked cache survives a commit' '
+ test-dump-untracked-cache >../before &&
+ git add done/two &&
+ git commit -m commit &&
+ test-dump-untracked-cache >../after &&
+ test_cmp ../before ../after
+'
+
test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8d99eb3..e974b5b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -186,6 +186,10 @@ test_commit () {
test_tick
fi &&
git commit $signoff -m "$1" &&
+ if [ "$(git config core.bare)" = false ]
+ then
+ git update-index --force-untracked-cache
+ fi
git tag "${4:-$1}"
}
diff --git a/unpack-trees.c b/unpack-trees.c
index 9f55cc2..fc90eb3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1215,6 +1215,7 @@ int unpack_trees(unsigned len, struct tree_desc *t,
struct unpack_trees_options
WRITE_TREE_SILENT |
WRITE_TREE_REPAIR);
}
+ move_index_extensions(&o->result, o->dst_index);
discard_index(o->dst_index);
*o->dst_index = o->result;
} else {
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 11/19] update-index: enable/disable watchman support
From: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 3 +++
Documentation/git-update-index.txt | 6 ++
builtin/update-index.c | 16
3 files changed, 25 insertions(+)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index cce00cb..55a8a5a 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -18,6 +18,9 @@ each with a submodule, you might need four index-helpers.
(In practice,
this is only worthwhile for large indexes, so only use it if you notice
that git status is slow).
+If you want the index-helper to accelerate untracked file checking,
+run git update-index --watchman before using it.
+
OPTIONS
---
diff --git a/Documentation/git-update-index.txt
b/Documentation/git-update-index.txt
index c6cbed1..6736487 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -19,6 +19,7 @@ SYNOPSIS
[--ignore-submodules]
[--[no-]split-index]
[--[no-|test-|force-]untracked-cache]
+[--[no-]watchman]
[--really-refresh] [--unresolve] [--again | -g]
[--info-only] [--index-info]
[-z] [--stdin] [--index-version ]
@@ -176,6 +177,11 @@ may not support it yet.
--no-untracked-cache::
Enable or disable untracked cache feature. Please use
`--test-untracked-cache` before enabling it.
+
+--watchman::
+--no-watchman::
+ Enable or disable watchman support. This is, at present,
+ only useful with git index-helper.
+
These options take effect whatever the value of the `core.untrackedCache`
configuration variable (see linkgit:git-config[1]). But a warning is
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 1c94ca5..a3b4b5d 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -914,6 +914,7 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
{
int newfd, entries, has_errors = 0, nul_term_line = 0;
enum uc_mode untracked_cache = UC_UNSPECIFIED;
+ int use_watchman = -1;
int read_from_stdin = 0;
int prefix_length = prefix ? strlen(prefix) : 0;
int preferred_index_format = 0;
@@ -1012,6 +1013,8 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
N_("test if the filesystem supports untracked
cache"), UC_TEST),
OPT_SET_INT(0, "force-untracked-cache", &untracked_cache,
N_("enable untracked cache without testing the
filesystem"), UC_FORCE),
+ OPT_BOOL(0, "watchman", &use_watchman,
+ N_("use or not use watchman to reduce refresh cost")),
OPT_END()
};
@@ -1149,6 +1152,19 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
die("Bug: bad untracked_cache value: %d", untracked_cache);
}
+ if (use_watchman > 0) {
+ the_index.last_update= xstrdup("");
+ the_index.cache_changed |= WATCHMAN_CHANGED;
+#ifndef USE_WATCHMAN
+ warning("git was built without watchman support -- I'm "
+ "adding the extension here, but it probably won't "
+ "do you any good.");
+#endif
+ } else if (!use_watchman) {
+ the_index.last_update= NULL;
+ the_index.cache_changed |= WATCHMAN_CHANGED;
+ }
+
if (active_cache_changed) {
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 07/19] index-helper: add --detach
From: Nguyễn Thái Ngọc Duy
We detach after creating and opening the socket, because otherwise
we might return control to the shell before index-helper is ready to
accept commands. This might lead to flaky tests.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 3 +++
index-helper.c | 15 +--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index 9c2f5eb..e144752 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -34,6 +34,9 @@ OPTIONS
for reading an index, but because it will happen in the
background, it's not noticable. `--strict` is enabled by default.
+--detach::
+ Detach from the shell.
+
NOTES
-
diff --git a/index-helper.c b/index-helper.c
index 21fb431..7df7a97 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -17,7 +17,7 @@ struct shm {
static struct shm shm_index;
static struct shm shm_base_index;
-static int to_verify = 1;
+static int daemonized, to_verify = 1;
static void log_warning(const char *warning, ...)
{
@@ -59,6 +59,8 @@ static void cleanup_shm(void)
static void cleanup(void)
{
+ if (daemonized)
+ return;
unlink(git_path("index-helper.sock"));
cleanup_shm();
}
@@ -311,7 +313,7 @@ static const char * const usage_text[] = {
int main(int argc, char **argv)
{
const char *prefix;
- int idle_in_seconds = 600;
+ int idle_in_seconds = 600, detach = 0;
int fd;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
@@ -319,6 +321,7 @@ int main(int argc, char **argv)
N_("exit if not used after some seconds")),
OPT_BOOL(0, "strict", &to_verify,
"verify shared memory after creating"),
+ OPT_BOOL(0, "detach", &detach, "detach the process"),
OPT_END()
};
@@ -342,6 +345,14 @@ int main(int argc, char **argv)
if (fd < 0)
die_errno(_("could not set up index-helper socket"));
+ if (!freopen("/dev/null", "w", stderr))
+ die_errno("failed to redirect stderr to /dev/null");
+
+ if (!freopen("/dev/null", "w", stdout))
+ die_errno("failed to redirect stdout to /dev/null");
+
+ if (detach && daemonize(&daemonized))
+ die_errno(_("unable to detach"));
loop(fd, idle_in_seconds);
close(fd);
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 19/19] untracked-cache: config option
Add a config option to populate the untracked cache.
For installations that have centrally-managed configuration, it's
easier to set a config once than to run update-index on every
repository.
Signed-off-by: David Turner
---
Documentation/config.txt | 4
read-cache.c | 7 ++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 385ea66..c7b76ef 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,6 +1848,10 @@ imap::
The configuration variables in the 'imap' section are described
in linkgit:git-imap-send[1].
+index.adduntrackedcache::
+ Automatically populate the untracked cache whenever the index
+ is written.
+
index.addwatchmanextension::
Automatically add the watchman extension to the index whenever
it is written.
diff --git a/read-cache.c b/read-cache.c
index a32bd54..ee1d4c0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2471,7 +2471,7 @@ static int do_write_index(struct index_state *istate, int
newfd,
int entries = istate->cache_nr;
struct stat st;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
- int watchman = 0;
+ int watchman = 0, untracked = 0;
uint64_t start = getnanotime();
for (i = removed = extended = 0; i < entries; i++) {
@@ -2501,6 +2501,11 @@ static int do_write_index(struct index_state *istate,
int newfd,
!the_index.last_update)
the_index.last_update = xstrdup("");
+ if (!git_config_get_bool("index.adduntrackedcache", &untracked) &&
+ untracked &&
+ !istate->untracked)
+ add_untracked_cache(&the_index);
+
hdr_version = istate->version;
hdr.hdr_signature = htonl(CACHE_SIGNATURE);
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 09/19] Add watchman support to reduce index refresh cost
From: Nguyễn Thái Ngọc Duy
The previous patch has the logic to clear bits in 'WAMA' bitmap. This
patch has logic to set bits as told by watchman. The missing bit,
_using_ these bits, are not here yet.
A lot of this code is written by David Turner originally, mostly from
[1]. I'm just copying and polishing it a bit.
[1] http://article.gmane.org/gmane.comp.version-control.git/248006
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Makefile | 12 +
cache.h| 1 +
config.c | 5 ++
configure.ac | 8
environment.c | 3 ++
watchman-support.c | 135 +
watchman-support.h | 7 +++
7 files changed, 171 insertions(+)
create mode 100644 watchman-support.c
create mode 100644 watchman-support.h
diff --git a/Makefile b/Makefile
index c8be0e7..65ab0f4 100644
--- a/Makefile
+++ b/Makefile
@@ -451,6 +451,7 @@ MSGFMT = msgfmt
CURL_CONFIG = curl-config
PTHREAD_LIBS = -lpthread
PTHREAD_CFLAGS =
+WATCHMAN_LIBS =
GCOV = gcov
export TCL_PATH TCLTK_PATH
@@ -1416,6 +1417,13 @@ else
LIB_OBJS += thread-utils.o
endif
+ifdef USE_WATCHMAN
+ LIB_H += watchman-support.h
+ LIB_OBJS += watchman-support.o
+ WATCHMAN_LIBS = -lwatchman
+ BASIC_CFLAGS += -DUSE_WATCHMAN
+endif
+
ifdef HAVE_PATHS_H
BASIC_CFLAGS += -DHAVE_PATHS_H
endif
@@ -2025,6 +2033,9 @@ git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS
$(GITLIBS) $(VCSSVN_LIB)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)
$(LIBS) \
$(VCSSVN_LIB)
+git-index-helper$X: index-helper.o GIT-LDFLAGS $(GITLIBS)
+ $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)
$(LIBS) $(WATCHMAN_LIBS)
+
$(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
$(QUIET_LNCP)$(RM) $@ && \
ln $< $@ 2>/dev/null || \
@@ -2164,6 +2175,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
@echo NO_MMAP=\''$(subst ','\'',$(subst ','\'',$(NO_MMAP)))'\' >>$@+
+ @echo USE_WATCHMAN=\''$(subst ','\'',$(subst
','\'',$(USE_WATCHMAN)))'\' >>$@+
ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
endif
diff --git a/cache.h b/cache.h
index f10992d..452aea2 100644
--- a/cache.h
+++ b/cache.h
@@ -696,6 +696,7 @@ extern char *git_replace_ref_base;
extern int fsync_object_files;
extern int core_preload_index;
+extern int core_watchman_sync_timeout;
extern int core_apply_sparse_checkout;
extern int precomposed_unicode;
extern int protect_hfs;
diff --git a/config.c b/config.c
index 9ba40bc..e6dc141 100644
--- a/config.c
+++ b/config.c
@@ -882,6 +882,11 @@ static int git_default_core_config(const char *var, const
char *value)
return 0;
}
+ if (!strcmp(var, "core.watchmansynctimeout")) {
+ core_watchman_sync_timeout = git_config_int(var, value);
+ return 0;
+ }
+
if (!strcmp(var, "core.createobject")) {
if (!strcmp(value, "rename"))
object_creation_mode = OBJECT_CREATION_USES_RENAMES;
diff --git a/configure.ac b/configure.ac
index 0cd9f46..334d63b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1099,6 +1099,14 @@ AC_COMPILE_IFELSE([BSD_SYSCTL_SRC],
HAVE_BSD_SYSCTL=])
GIT_CONF_SUBST([HAVE_BSD_SYSCTL])
+#
+# Check for watchman client library
+
+AC_CHECK_LIB([watchman], [watchman_connect],
+ [USE_WATCHMAN=YesPlease],
+ [USE_WATCHMAN=])
+GIT_CONF_SUBST([USE_WATCHMAN])
+
## Other checks.
# Define USE_PIC if you need the main git objects to be built with -fPIC
# in order to build and link perl/Git.so. x86-64 seems to need this.
diff --git a/environment.c b/environment.c
index 6dec9d0..35e03c7 100644
--- a/environment.c
+++ b/environment.c
@@ -94,6 +94,9 @@ int core_preload_index = 1;
*/
int ignore_untracked_cache_config;
+int core_watchman_sync_timeout = 300;
+
+
/* This is set by setup_git_dir_gently() and/or git_default_config() */
char *git_work_tree_cfg;
static char *work_tree;
diff --git a/watchman-support.c b/watchman-support.c
new file mode 100644
index 000..b168e88
--- /dev/null
+++ b/watchman-support.c
@@ -0,0 +1,135 @@
+#include "cache.h"
+#include "watchman-support.h"
+#include "strbuf.h"
+#include "dir.h"
+#include
+
+static struct watchman_query *make_query(const char *last_update)
+{
+ struct watchman_query *query = watchman_query();
+ watchman_query_set_fields(query, WATCHMAN_FIELD_NAME |
+WATCHMAN_FIELD_EXISTS |
+WATCHMAN_FIELD_NEWER);
+ watchman_query_set_empty_on_fresh(query, 1);
+ query->sync_timeout = core_watchman_sync_timeout;
+ if (*last_upda
[PATCH v8 03/19] index-helper: new daemon for caching index and related stuff
From: Nguyễn Thái Ngọc Duy Instead of reading the index from disk and worrying about disk corruption, the index is cached in memory (memory bit-flips happen too, but hopefully less often). The result is faster read. Read time is reduced by 70%. The biggest gain is not having to verify the trailing SHA-1, which takes lots of time especially on large index files. But this also opens doors for further optimizations: - we could create an in-memory format that's essentially the memory dump of the index to eliminate most of parsing/allocation overhead. The mmap'd memory can be used straight away. Experiment [1] shows we could reduce read time by 88%. - we could cache non-index info such as name hash Shared memory is done by storing files in a per-repository temporary directory. This is more portable than shm (which requires posix-realtime and has various quirks on OS X). It might even work on Windows, although this has not been tested. The shared memory file's name follows the template "shm--" where is the trailing SHA-1 of the index file. is "index" for cached index files (and might later be "name-hash" for name-hash cache). If such shared memory exists, it contains the same index content as on disk. The content is already validated by the daemon and git won't validate it again (except comparing the trailing SHA-1s). We also add some bits to the index (to_shm and from_shm) to track when an index came from shared memory or is going to shared memory. We keep this daemon's logic as thin as possible. The "brain" stays in git. So the daemon can read and validate stuff, but that's all it's allowed to do. It does not add/create new information. It doesn't even accept direct updates from git. Git can poke the daemon via unix domain sockets to tell it to refresh the index cache, or to keep it alive some more minutes. It can't give any real index data directly to the daemon. Real data goes to disk first, then the daemon reads and verifies it from there. The daemon only handles $GIT_DIR/index, not temporary index files; it only gets poked for the former. $GIT_DIR/index-helper.sock is the socket for the daemon process. The daemon reads from the socket and executes commands. Named pipes were considered for portability reasons, but then commands that need replies from the daemon would have to open their own pipes, since a named pipe should only have one reader. Unix domain sockets don't have this problem. On webkit.git with index format v2, duplicating 8 times to 1.5m entries and 236MB in size: (vanilla) 0.50 s: read_index_from .git/index (index-helper) 0.18 s: read_index_from .git/index Interestingly with index v4, we get less out of index-helper. It makes sense as v4 requires more processing after loading the index: (vanilla) 0.37 s: read_index_from .git/index (index-helper) 0.22 s: read_index_from .git/index [1] http://thread.gmane.org/gmane.comp.version-control.git/247268/focus=248771 Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: David Turner Signed-off-by: Ramsay Jones --- .gitignore | 1 + Documentation/git-index-helper.txt | 50 +++ Makefile | 5 + cache.h| 11 ++ git-compat-util.h | 1 + index-helper.c | 277 + read-cache.c | 125 +++-- t/t7900-index-helper.sh| 23 +++ 8 files changed, 484 insertions(+), 9 deletions(-) create mode 100644 Documentation/git-index-helper.txt create mode 100644 index-helper.c create mode 100755 t/t7900-index-helper.sh diff --git a/.gitignore b/.gitignore index 5087ce1..b92f122 100644 --- a/.gitignore +++ b/.gitignore @@ -71,6 +71,7 @@ /git-http-fetch /git-http-push /git-imap-send +/git-index-helper /git-index-pack /git-init /git-init-db diff --git a/Documentation/git-index-helper.txt b/Documentation/git-index-helper.txt new file mode 100644 index 000..f892184 --- /dev/null +++ b/Documentation/git-index-helper.txt @@ -0,0 +1,50 @@ +git-index-helper(1) +=== + +NAME + +git-index-helper - A simple cache daemon for speeding up index file access + +SYNOPSIS + +[verse] +'git index-helper' [options] + +DESCRIPTION +--- +Keep the index file in memory for faster access. This daemon is per +repository and per working tree. So if you have two working trees +each with a submodule, you might need four index-helpers. (In practice, +this is only worthwhile for large indexes, so only use it if you notice +that git status is slow). + +OPTIONS +--- + +--exit-after=:: + Exit if the cached index is not accessed for `` + seconds. Specify 0 to wait forever. Default is 600. + +NOTES +- + +$GIT_DIR/index-helper.sock a Unix domain socket that the daemon reads +commands from. The directory will also contain files named +"shm-index-". These are used as backing stores for shared +memor
[PATCH v8 14/19] index-helper: kill mode
Add a new command (and command-line arg) to allow index-helpers to
exit cleanly.
This is mainly useful for tests.
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 3 +++
index-helper.c | 31 ++-
t/t7900-index-helper.sh| 9 +
3 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index 55a8a5a..addf694 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -40,6 +40,9 @@ OPTIONS
--detach::
Detach from the shell.
+--kill::
+ Kill any running index-helper and clean up the socket
+
NOTES
-
diff --git a/index-helper.c b/index-helper.c
index 71c4f48..4ed1610 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -373,6 +373,8 @@ static void loop(int fd, int idle_in_seconds)
* alive, nothing to do.
*/
}
+ } else if (!strcmp(buf, "die")) {
+ break;
} else {
log_warning("BUG: Bogus command %s", buf);
}
@@ -403,10 +405,29 @@ static const char * const usage_text[] = {
NULL
};
+static void request_kill(void)
+{
+ int fd = unix_stream_connect(git_path("index-helper.sock"));
+
+ if (fd >= 0) {
+ write_in_full(fd, "die", 4);
+ close(fd);
+ }
+
+ /*
+* The child will try to do this anyway, but we want to be
+* ready to launch a new daemon immediately after this command
+* returns.
+*/
+
+ unlink(git_path("index-helper.sock"));
+ return;
+}
+
int main(int argc, char **argv)
{
const char *prefix;
- int idle_in_seconds = 600, detach = 0;
+ int idle_in_seconds = 600, detach = 0, kill = 0;
int fd;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
@@ -415,6 +436,7 @@ int main(int argc, char **argv)
OPT_BOOL(0, "strict", &to_verify,
"verify shared memory after creating"),
OPT_BOOL(0, "detach", &detach, "detach the process"),
+ OPT_BOOL(0, "kill", &kill, "request that existing index helper
processes exit"),
OPT_END()
};
@@ -429,6 +451,13 @@ int main(int argc, char **argv)
options, usage_text, 0))
die(_("too many arguments"));
+ if (kill) {
+ if (detach)
+ die(_("--kill doesn't want any other options"));
+ request_kill();
+ return 0;
+ }
+
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index 114c112..e71b5af 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -20,4 +20,13 @@ test_expect_success 'index-helper smoke test' '
test_path_is_missing .git/index-helper.sock
'
+test_expect_success 'index-helper creates usable path file and can be killed' '
+ test_when_finished "git index-helper --kill" &&
+ test_path_is_missing .git/index-helper.sock &&
+ git index-helper --detach &&
+ test -S .git/index-helper.sock &&
+ git index-helper --kill &&
+ test_path_is_missing .git/index-helper.sock
+'
+
test_done
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 17/19] index-helper: optionally automatically run
Introduce a new config option, indexhelper.autorun, to automatically
run git index-helper before starting up a builtin git command. This
enables users to keep index-helper running without manual
intervention.
Signed-off-by: David Turner
---
Documentation/config.txt | 4
read-cache.c | 37 +++--
t/t7900-index-helper.sh | 20
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 15001ce..385ea66 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1856,6 +1856,10 @@ index.version::
Specify the version with which new index files should be
initialized. This does not affect existing repositories.
+indexhelper.autorun::
+ Automatically run git index-helper when any builtin git
+ command is run inside a repository.
+
init.templateDir::
Specify the directory from which templates will be copied.
(See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
diff --git a/read-cache.c b/read-cache.c
index b00919a..d91742c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -22,6 +22,7 @@
#include "pkt-line.h"
#include "sigchain.h"
#include "ewah/ewok.h"
+#include "run-command.h"
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
unsigned int options);
@@ -1724,6 +1725,33 @@ static void post_read_index_from(struct index_state
*istate)
tweak_untracked_cache(istate);
}
+static int want_auto_index_helper(void)
+{
+ int want = -1;
+ const char *value = NULL;
+ const char *key = "indexhelper.autorun";
+
+ if (git_config_key_is_valid(key) &&
+ !git_config_get_value(key, &value)) {
+ int b = git_config_maybe_bool(key, value);
+ want = b >= 0 ? b : 0;
+ }
+ return want;
+}
+
+static void autorun_index_helper(void)
+{
+ const char *argv[] = {"git-index-helper", "--detach", "--autorun",
NULL};
+ if (want_auto_index_helper() <= 0)
+ return;
+
+ trace_argv_printf(argv, "trace: auto index-helper:");
+
+ if (run_command_v_opt(argv,
+ RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT))
+ warning(_("You specified indexhelper.autorun, but running
git-index-helper failed."));
+}
+
/* in ms */
#define WATCHMAN_TIMEOUT 1000
@@ -1797,6 +1825,7 @@ static int poke_daemon(struct index_state *istate,
if (fd < 0) {
warning("Failed to connect to index-helper socket");
unlink(git_path("index-helper.sock"));
+ autorun_index_helper();
return -1;
}
sigchain_push(SIGPIPE, SIG_IGN);
@@ -1836,9 +1865,13 @@ static int try_shm(struct index_state *istate)
int fd = -1;
if (!is_main_index(istate) ||
- old_size <= 20 ||
- stat(git_path("index-helper.sock"), &st))
+ old_size <= 20)
return -1;
+
+ if (stat(git_path("index-helper.sock"), &st)) {
+ autorun_index_helper();
+ return -1;
+ }
if (poke_daemon(istate, &st, 0))
return -1;
sha1 = (unsigned char *)istate->mmap + old_size - 20;
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index aa6e5fc..3cfdf63 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -16,6 +16,9 @@ test -n "$NO_MMAP" && {
}
test_expect_success 'index-helper smoke test' '
+ # We need an existing commit so that the index exists (otherwise,
+ # the index-helper will not be autostarted)
+ test_commit x &&
git index-helper --exit-after 1 &&
test_path_is_missing .git/index-helper.sock
'
@@ -46,4 +49,21 @@ test_expect_success 'index-helper is quiet with --autorun' '
git index-helper --autorun
'
+test_expect_success 'index-helper autorun works' '
+ test_when_finished "git index-helper --kill" &&
+ rm -f .git/index-helper.sock &&
+ git status &&
+ test_path_is_missing .git/index-helper.sock &&
+ test_config indexhelper.autorun true &&
+ git status &&
+ test -S .git/index-helper.sock &&
+ git status 2>err &&
+ test -S .git/index-helper.sock &&
+ test_must_be_empty err &&
+ git index-helper --kill &&
+ test_config indexhelper.autorun false &&
+ git status &&
+ test_path_is_missing .git/index-helper.sock
+'
+
test_done
--
2.4.2.767.g62658d5-twtrsrc
--
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 v8 10/19] index-helper: use watchman to avoid refreshing index with lstat()
From: Nguyễn Thái Ngọc Duy
Watchman is hidden behind index-helper. Before git tries to read the
index from shm, it notifies index-helper through the socket and waits
for index-helper to prepare a file for sharing memory (with
MAP_SHARED). index-helper then contacts watchman, updates 'WAMA'
extension and put it in a separate file and wakes git up with a reply
to git's socket.
Git uses this extension to not lstat unchanged entries. Git only
trusts the 'WAMA' extension when it's received from the separate file,
not from disk. Unmarked entries are "clean". Marked entries are dirty
from watchman point of view. If it finds out some entries are
'watchman-dirty', but are really unchanged (e.g. the file was changed,
then reverted back), then Git will clear the marking in 'WAMA' before
writing it down.
Hiding watchman behind index-helper means you need both daemons. You
can't run watchman alone. Not so good. But on the other hand, 'git'
binary is not linked to watchman/json libraries, which is good for
packaging. Core git package will run fine without watchman-related
packages. If they need watchman, they can install git-index-helper and
dependencies.
This also lets us trust anything in the untracked cache that we haven't
marked invalid, saving those stat() calls.
Another reason for tying watchman to index-helper is, when used with
untracked cache, we need to keep track of $GIT_WORK_TREE file
listing. That kind of list can be kept in index-helper.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 6 +
cache.h| 2 +
dir.c | 23 +++-
dir.h | 3 +
index-helper.c | 107 ++--
read-cache.c | 244 ++---
6 files changed, 358 insertions(+), 27 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index e144752..cce00cb 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -55,6 +55,12 @@ command. The following commands are used to control the
daemon:
Let the daemon know the index is to be read. It keeps the
daemon alive longer, unless `--exit-after=0` is used.
+"poke ":
+ Like "poke", but replies with "OK". If the index has the
+ watchman extension, index-helper queries watchman, then
+ prepares a shared memory object with the watchman index
+ extension before replying.
+
All commands and replies are terminated by a NUL byte.
In the event of an error, messages may be written to
diff --git a/cache.h b/cache.h
index 452aea2..a167920 100644
--- a/cache.h
+++ b/cache.h
@@ -567,6 +567,7 @@ extern int daemonize(int *);
/* Initialize and use the cache information */
struct lock_file;
+extern int verify_index(const struct index_state *);
extern int read_index(struct index_state *);
extern int read_index_preload(struct index_state *, const struct pathspec
*pathspec);
extern int do_read_index(struct index_state *istate, const char *path,
@@ -574,6 +575,7 @@ extern int do_read_index(struct index_state *istate, const
char *path,
extern int read_index_from(struct index_state *, const char *path);
extern int is_index_unborn(struct index_state *);
extern int read_index_unmerged(struct index_state *);
+extern void write_watchman_ext(struct strbuf *sb, struct index_state* istate);
#define COMMIT_LOCK(1 << 0)
#define CLOSE_LOCK (1 << 1)
extern int write_locked_index(struct index_state *, struct lock_file *lock,
unsigned flags);
diff --git a/dir.c b/dir.c
index 69e0be6..5058b29 100644
--- a/dir.c
+++ b/dir.c
@@ -597,9 +597,9 @@ static void trim_trailing_spaces(char *buf)
*
* If "name" has the trailing slash, it'll be excluded in the search.
*/
-static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
- struct untracked_cache_dir
*dir,
- const char *name, int len)
+struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
+struct untracked_cache_dir *dir,
+const char *name, int len)
{
int first, last;
struct untracked_cache_dir *d;
@@ -1726,6 +1726,17 @@ static int valid_cached_dir(struct dir_struct *dir,
if (!untracked)
return 0;
+ if (dir->untracked->use_watchman) {
+ /*
+* With watchman, we can trust the untracked cache's
+* valid field.
+*/
+ if (untracked->valid)
+ goto skip_stat;
+ else
+ invalidate_directory(dir->untracked, untracked);
+ }
+
if (stat(path->len ? path->buf : ".", &st)) {
inval
[PATCH v8 00/19] index-helper/watchman
This version corrects defects noticed by Ramsay Jones: index-helper now works when USE_WATCHMAN is not set sigpipe is handled post-test cleanup is improved a test now uses a more idiomatic check restored an index verification check from a previous version of the series David Turner (8): index-helper: log warnings unpack-trees: preserve index extensions watchman: add a config option to enable the extension index-helper: kill mode index-helper: don't run if already running index-helper: autorun mode index-helper: optionally automatically run untracked-cache: config option Nguyễn Thái Ngọc Duy (11): read-cache.c: fix constness of verify_hdr() read-cache: allow to keep mmap'd memory after reading index-helper: new daemon for caching index and related stuff index-helper: add --strict daemonize(): set a flag before exiting the main process index-helper: add --detach read-cache: add watchman 'WAMA' extension Add watchman support to reduce index refresh cost index-helper: use watchman to avoid refreshing index with lstat() update-index: enable/disable watchman support Add tracing to measure where most of the time is spent .gitignore | 2 + Documentation/config.txt | 12 + Documentation/git-index-helper.txt | 81 + Documentation/git-update-index.txt | 6 + Documentation/technical/index-format.txt | 22 ++ Makefile | 18 ++ builtin/gc.c | 2 +- builtin/update-index.c | 16 + cache.h | 25 +- config.c | 5 + configure.ac | 8 + daemon.c | 2 +- diff-lib.c | 4 + dir.c| 25 +- dir.h| 6 + environment.c| 3 + git-compat-util.h| 1 + index-helper.c | 506 + name-hash.c | 2 + preload-index.c | 2 + read-cache.c | 535 ++- refs/files-backend.c | 2 + setup.c | 4 +- t/t1701-watchman-extension.sh| 37 +++ t/t7063-status-untracked-cache.sh| 22 ++ t/t7900-index-helper.sh | 69 t/test-lib-functions.sh | 4 + test-dump-watchman.c | 16 + unpack-trees.c | 1 + watchman-support.c | 135 watchman-support.h | 7 + 31 files changed, 1559 insertions(+), 21 deletions(-) create mode 100644 Documentation/git-index-helper.txt create mode 100644 index-helper.c create mode 100755 t/t1701-watchman-extension.sh create mode 100755 t/t7900-index-helper.sh create mode 100644 test-dump-watchman.c create mode 100644 watchman-support.c create mode 100644 watchman-support.h -- 2.4.2.767.g62658d5-twtrsrc -- 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 v8 01/19] read-cache.c: fix constness of verify_hdr()
From: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
read-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index d9fb78b..16cc487 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1345,7 +1345,7 @@ struct ondisk_cache_entry_extended {
ondisk_cache_entry_extended_size(ce_namelen(ce)) : \
ondisk_cache_entry_size(ce_namelen(ce)))
-static int verify_hdr(struct cache_header *hdr, unsigned long size)
+static int verify_hdr(const struct cache_header *hdr, unsigned long size)
{
git_SHA_CTX c;
unsigned char sha1[20];
--
2.4.2.767.g62658d5-twtrsrc
--
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: /* compiler workaround */ - what was the issue?
"Philip Oakley" writes: > int saved_namelen = saved_namelen; /* compiler workaround */ > > Which then becomes an MSVC compile warning C4700: uninitialized local > variable. > > I'm wondering what was the compiler workaround being referred to? i.e. why > does it need that tweak? There's no mention of the reason in the commit > message. That was a fairly well-known workaround for GCC that issues a false warning that variable is used before initialized. I thought we stopped using it several years ago in new code after doing a bulk sanitizing (I think the new recommended workaround was to initialise such a variable to the nil value like '0' for integers). -- 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: unary minus operator applied to unsigned type, result still unsigned?
From: "Junio C Hamano" "Philip Oakley" writes: I'm working on building Git on Visual Studio as part of the Git for Windows capability. The MSVC compiler is reporting: 1>..\sha1-lookup.c(100) : warning C4146: unary minus operator applied to unsigned type, result still unsigned 1>..\sha1-lookup.c(316) : warning C4146: unary minus operator applied to unsigned type, result still unsigned the two lines of code are the same, and the message suggests a bad return value: #100: return -lo-1; #316: return -lo-1; Should these be protected by an appropriate brackets/calculation (-1-lo) ? Or does gcc use an alternate assumption about unary minus conversion for functions returning int? I think -lo-1 (which is carried out as unsigned) is further casted back to signed int as part of the return from the function whose type is signed int. This may be portable in practice, but changing type of lo to signed int may be prudent. sha1_pos() uses size_t, but returns "int", which is inconsistent (size_t could be much larger). Either using int for hi/lo/mi, or using ssize_t and returning ssize_t would make sense. I'm going to hold off on patching this at the moment, as there are a few more signed/unsigned mismatches that are reported but I've been ignoring (mainly comparisons), so I'll try and catch them all at the same time, especially as I now understand the standards issues. This thread has a summary of the standards http://stackoverflow.com/questions/8026694/c-unary-minus-operator-behavior-with-unsigned-operands (includes C++ >> C) while https://msdn.microsoft.com/en-us/library/4kh09110(v=vs.90).aspx explains what is being reported. i.e. unary minus on unsigned ints produces the 2's complement + 1 value, except for 0x8000h, where that would be out of range. It's an oddball of partially defined behaviours. -- Philip -- 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: t7900-*.sh tesr #5 failure
On Tue, 2016-05-03 at 22:34 +0100, Ramsay Jones wrote: > Hi David, > > Test t7900.5 fails for me, thus: > > $ ./t7900-index-helper.sh -i -v -x -d > > ... > > + test -S .git/index-helper.sock > + git status > On branch master > Untracked files: > (use "git add ..." to include in what will be committed) > > err > > nothing added to commit but untracked files present (use "git add" > to track) > + test -S .git/index-helper.sock > + grep -q . err > error: last command exited with $?=1 > not ok 5 - index-helper autorun works > # > # rm -f .git/index-helper.sock && > # git status && > # test_path_is_missing .git/index-helper.sock && > # test_config indexhelper.autorun true && > # git status && > # test -S .git/index-helper.sock && > # git status 2>err && > # test -S .git/index-helper.sock && > # ! grep -q . err && > # git index-helper --kill && > # test_config indexhelper.autorun false && > # git status && > # test_path_is_missing .git/index-helper.sock > # > $ cd trash\ directory.t7900-index-helper/ > $ ls > err x.t > $ cat err > warning: We requested watchman support from index-helper, but it > doesn't support it. Please use a version of git index-helper with > watchman support. > $ > > [Yes, that is one long line in err!] OK, the failure here is due to a bad check in the code -- it's intended that index-helper work without watchman. Will re-roll with a fix. > [At least, this is one of the failures, I have also seen git status > failing > with a SIGPIPE.] I'll add in a sig_ign on that. > Note that I do not have the watchman libraries etc., so USE_WATCHMAN > is > not defined. Note also, that I had an instance of git-index-helper > still > running after the test failure (which I kill-ed). I missed a test_when_finished (note that in the case of test failures in debug mode, test_when_finished statements are not executed on failing tests, so you would still see an index-helper in that case; since test failures are not expected, that shouldn't be a problem). Will fix, thanks. > I haven't spent any time debuging this, but some questions spring to > mind: > > - can index-helper be used without watchman support? Yes (once I re-roll with the bug fix). > - why is index-helper requesting watchman support, when it was > built without USE_WATCHMAN being defined? It's not; the code that checks for that request is mistakenly detecting that it is. > - why is read-cache.o exporting hte verify_index and > write_watchman_ext symbols? verify_index should be used in index-helper but that somehow got lost in v3. Will fix on reroll. write_watchman_ext is exported because index-helper currently uses it. > - is index-helper any use/help without watchman support? Yes for large indexes -- see the measurements in patch v3 (which is earlier in the series than watchman is introduced). > - is '! grep -q . err' meant to determine if the err file is > empty (ie git status did not issue an error message)? > [if yes, maybe 'test_must_be_empty err &&' would read better!] Will fix, thanks. -- 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] exec_cmd.c, sideband.c, Makefile: avoid multiple PREFIX definitions
The short and sweet PREFIX can be confused when used in many places.
Rename both usages to better describe their purpose.
Noticed when compiling Git for Windows using MSVC/Visual Studio which
reports the conflict beteeen the command line definition and the
definition in sideband.c
Signed-off-by: Philip Oakley
---
Makefile | 2 +-
exec_cmd.c | 4 ++--
sideband.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/Makefile b/Makefile
index 33b0f76..bcdd3ec 100644
--- a/Makefile
+++ b/Makefile
@@ -1973,7 +1973,7 @@ exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
'-DBINDIR="$(bindir_relative_SQ)"' \
- '-DPREFIX="$(prefix_SQ)"'
+ '-DEXEC_PREFIX="$(prefix_SQ)"'
builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
diff --git a/exec_cmd.c b/exec_cmd.c
index 9d5703a..2a79781 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -12,7 +12,7 @@ char *system_path(const char *path)
#ifdef RUNTIME_PREFIX
static const char *prefix;
#else
- static const char *prefix = PREFIX;
+ static const char *prefix = EXEC_PREFIX;
#endif
struct strbuf d = STRBUF_INIT;
@@ -27,7 +27,7 @@ char *system_path(const char *path)
!(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&
!(prefix = strip_path_suffix(argv0_path, BINDIR)) &&
!(prefix = strip_path_suffix(argv0_path, "git"))) {
- prefix = PREFIX;
+ prefix = EXEC_PREFIX;
trace_printf("RUNTIME_PREFIX requested, "
"but prefix computation failed. "
"Using static fallback '%s'.\n", prefix);
diff --git a/sideband.c b/sideband.c
index fde8adc..d448ba7 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,7 +13,7 @@
* the remote died unexpectedly. A flush() concludes the stream.
*/
-#define PREFIX "remote:"
+#define DISPLAY_PREFIX "remote:"
#define ANSI_SUFFIX "\033[K"
#define DUMB_SUFFIX ""
@@ -22,13 +22,13 @@
int recv_sideband(const char *me, int in_stream, int out)
{
- unsigned pf = strlen(PREFIX);
+ unsigned pf = strlen(DISPLAY_PREFIX);
unsigned sf;
char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
char *suffix, *term;
int skip_pf = 0;
- memcpy(buf, PREFIX, pf);
+ memcpy(buf, DISPLAY_PREFIX, pf);
term = getenv("TERM");
if (isatty(2) && term && strcmp(term, "dumb"))
suffix = ANSI_SUFFIX;
--
2.8.1.windows.1
--
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] Conflicting PREFIX usage
While trying to resurrect the MSVC/Visual Studio capability for Git for Windows I noticed this little nuance. The PREFIX in Windows was introduced in 35fb0e8 (Compute prefix at runtime if RUNTIME_PREFIX is set, 2009-01-18) and in sideband.c at ebe8fa7 (fix display overlap between remote and local progress, 2007-11-04) though clash has not been noticed before. The attached patch simply gives the two PREFIXs more purposeful names of EXEC_PREFIX and DISPLAY_PREFIX. Philip Oakley (1): exec_cmd.c, sideband.c, Makefile: avoid multiple PREFIX definitions Makefile | 2 +- exec_cmd.c | 4 ++-- sideband.c | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) -- 2.8.1.windows.1 -- 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 11/15] diff: ignore submodules excluded by groups
On Thu, May 5, 2016 at 1:19 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> Any thoughts on my thoughts below? >> >>> So here is a thought experiment: >>> >>> # get all submodules into the work tree >>> git submodule update --recursive --init >>> >>> # The selected default group will not include all submodules >>> git config submodule.defaultGroup "*SomeLabel" >>> >>> git status >>> # What do we expect now? >>> # either a "nothing to commit, working directory clean" >>> # or rather what was described in 0/15: >>> >>> More than 2 submodules (123 actually) including >>> 'path/foo' >>> 'lib/baz' >>> are checked out, but not part of the default group. >>> You can add these submodules via >>> git config --add submodule.defaultGroup ./path/foo >>> git config --add submodule.defaultGroup ./lib/baz > > That may be an interesting thing to know, but I am not sure if it > adds value to the user. The user wanted the defaultGroup to be > the set of submodules labeled with SomeLabel, and an alternative > valid suggestion could be > > 'path/foo' and other submodules are not part of what you are > interested in; if you want to deinitialize them, do > > git submodule deinit !defaultGroup > > Both look equally valid to me, but offering both would be way too > much. I'd say you should give that only with "status -v" or > something, perhaps? > >>> If we want to go with the second option, > > You already lost me here, as it is not clear what two "options" you > are comparing. The first option is giving nothing: git config submodule.defaultGroup "*SomeLabel" git -C submodule-not-labeled reset --hard HEAD^ git status # all good, no report about submodule-not-labeled # because it is not in the default group. # (This is implemented in the series) The "second option" is some sort of reporting. Either what I or you proposed. > >>> If we want to go with the second option, the design described in 0/15 >>> is broken. Going one step further: >>> >>> ... >>> # But what about subC which is not in the default group? It was >>> changed as well. > > So why not show it? Is there anything controversial? The user made clear to not be interested in subC by setting up the default group. > > If you are truly not interested in it by excluding it from the > default group, you wouldn't have touched it in the first place. Well it can get out of sync by not touching it as well, because others changed the submodule pointer who are interested in that though. # in the superproject git checkout new-version git submodule update # checks out submodules to their version git checkout some-ancient-version git submodule update # this would only update the submodules in the defaultGroup, # not those which are initialized but "uninteresting" # the labeling may have changed between the different versions git status # I don't want to see any submodule changes here. # but there may be a submodule which is not at the right version # as `git submodule update` only paid attention to the default group. > If > you did touch it, then you are showing some special interest that > overrides what you said in the default mechanism. > > In short, I think I understood what you wanted with your analogy to > the ignore/exclude mechanism in 0/15. Default is a handy way to say > "I do not want to bother specifying everything from the command line > every time" but we can say that it is nothing more than that. That > is exactly how the ignore/exclude mechanism is used--"git add" by > default will not add those that are ignored when discovering paths > by recursively descending, but once added, that is part of what the > user told Git that she cares about. > >>> In case we report these submodules which are checked out but not in >>> the default group, we probably want to adapt "git submodule update" to >>> un-checkout the work tree of the submodules in case they are clean. > > Why? > > Letting them know that they have such an option, and giving them a > way to tell which submodules fall into that category, are both good > things. But why is it a good thing to automatically clean what the > user has checked out (which I expect that she expects to remain > until she explicitly tells us otherwise)? > > We do not automatically "git rm" a clean tracked path that happens > to match .gitignore pattern and I do not think it is a good thing to > do so. git rm changes the index (which may show up in the next commit) The state of a submodule (un-initialized, initialized, checked out) doesn't change the index or anything. Only the working tree is affected. And by flipping between the initialized and checked-out state we do not lose any information (such as user configured remote urls) nor does it affect the "state" (index, recorded tree, history) of the r
Re: [PATCHv5] submodule deinit: require '--all' instead of '.' for all submodules
On Thu, May 5, 2016 at 1:37 PM, Jonathan Nieder wrote: > Stefan Beller wrote: > >>>$ rungit v2.6.6 submodule deinit . >>>error: pathspec '.' did not match any file(s) known to git. >>>Did you forget to 'git add'? > [...] >> So instead of a pathspec add the '--all' option to deinit all submodules >> and add a test to check for the corner case of an empty repository. > [...] >> Signed-off-by: Stefan Beller > > Reviewed-by: Jonathan Nieder Thanks, both. Will queue. -- 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
/* compiler workaround */ - what was the issue?
Duy, In https://github.com/git-for-windows/git/commit/b3c96fb158f05152336f167076f5d81d23c3a5e5 (split-index: strip pathname of on-disk replaced entries, 13 Jun 2014) Nguyễn Thái Ngọc Duy you have read-cache.c#L1790 (in that commit, now ~L1873) int saved_namelen = saved_namelen; /* compiler workaround */ Which then becomes an MSVC compile warning C4700: uninitialized local variable. I'm wondering what was the compiler workaround being referred to? i.e. why does it need that tweak? There's no mention of the reason in the commit message. There are a few other occurences of this same initialisation issue, so I was wondering if it's still neded? Or should I just get MSVC to ignore it, which may hide other issues.. -- Philip -- 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: [PATCHv5] submodule deinit: require '--all' instead of '.' for all submodules
Stefan Beller wrote: >>$ rungit v2.6.6 submodule deinit . >>error: pathspec '.' did not match any file(s) known to git. >>Did you forget to 'git add'? [...] > So instead of a pathspec add the '--all' option to deinit all submodules > and add a test to check for the corner case of an empty repository. [...] > Signed-off-by: Stefan Beller Reviewed-by: Jonathan Nieder -- 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 11/15] diff: ignore submodules excluded by groups
Stefan Beller writes: > Any thoughts on my thoughts below? > >> So here is a thought experiment: >> >> # get all submodules into the work tree >> git submodule update --recursive --init >> >> # The selected default group will not include all submodules >> git config submodule.defaultGroup "*SomeLabel" >> >> git status >> # What do we expect now? >> # either a "nothing to commit, working directory clean" >> # or rather what was described in 0/15: >> >> More than 2 submodules (123 actually) including >> 'path/foo' >> 'lib/baz' >> are checked out, but not part of the default group. >> You can add these submodules via >> git config --add submodule.defaultGroup ./path/foo >> git config --add submodule.defaultGroup ./lib/baz That may be an interesting thing to know, but I am not sure if it adds value to the user. The user wanted the defaultGroup to be the set of submodules labeled with SomeLabel, and an alternative valid suggestion could be 'path/foo' and other submodules are not part of what you are interested in; if you want to deinitialize them, do git submodule deinit !defaultGroup Both look equally valid to me, but offering both would be way too much. I'd say you should give that only with "status -v" or something, perhaps? >> If we want to go with the second option, You already lost me here, as it is not clear what two "options" you are comparing. >> If we want to go with the second option, the design described in 0/15 >> is broken. Going one step further: >> >> ... >> # But what about subC which is not in the default group? It was >> changed as well. So why not show it? Is there anything controversial? If you are truly not interested in it by excluding it from the default group, you wouldn't have touched it in the first place. If you did touch it, then you are showing some special interest that overrides what you said in the default mechanism. In short, I think I understood what you wanted with your analogy to the ignore/exclude mechanism in 0/15. Default is a handy way to say "I do not want to bother specifying everything from the command line every time" but we can say that it is nothing more than that. That is exactly how the ignore/exclude mechanism is used--"git add" by default will not add those that are ignored when discovering paths by recursively descending, but once added, that is part of what the user told Git that she cares about. >> In case we report these submodules which are checked out but not in >> the default group, we probably want to adapt "git submodule update" to >> un-checkout the work tree of the submodules in case they are clean. Why? Letting them know that they have such an option, and giving them a way to tell which submodules fall into that category, are both good things. But why is it a good thing to automatically clean what the user has checked out (which I expect that she expects to remain until she explicitly tells us otherwise)? We do not automatically "git rm" a clean tracked path that happens to match .gitignore pattern and I do not think it is a good thing to do so. Puzzled. -- 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: stop sanitizing config options
On Thu, May 05, 2016 at 09:59:15AM -0700, Junio C Hamano wrote: > Sorry for the trouble. > > I queued jk/submodule-c-credential to be mergeable to 'maint', as it > could be argued two ways. We can say that not propagating -c down > is a bug and the series is a bugfix. Or it is merely a known > limitation and the series is a new feature. I was leaning towards > the former, but I was also willing to declare that is a known bug > that will left unfixed in the maintenance track. Ah, OK, that makes perfect sense. > It probably is a good time to merge jk/submodule-config-sanitize-fix > into jk/submodule-c-credential (i.e. a mere fast-forward), remove > that "-fix" branch, and apply this patch directly on top of the > resulting jk/submodule-c-credential. That will make the whole thing > a 13-patch series, consisting of: > > 7 patches up to the current jk/submodule-c-credential > d1f8849 git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS > 14111fc git: submodule honor -c credential.* from command line > e70986d quote: implement sq_quotef() > 7dad263 submodule: fix segmentation fault in submodule--helper clone > 717416c submodule: fix submodule--helper clone usage > 08e0970 submodule: check argc count for git submodule--helper clone > d10e3b4 submodule: don't pass empty string arguments to submodule--helper > clone > > 5 patches up to the current jk/submodule-config-sanitize-fix > c12e865 submodule: use prepare_submodule_repo_env consistently > 4638728 submodule--helper: move config-sanitizing to submodule.c > 860cba6 submodule: export sanitized GIT_CONFIG_PARAMETERS > 455d22c t5550: break submodule config test into multiple sub-tests > 1149ee2 t5550: fix typo in $HTTPD_URL > > 1 patch (this one) > 4e6706a submodule: stop sanitizing config options That sounds reasonable. Note that the later patches drop the only caller of the newly-introduced sq_quotef(). So we could revert e70986d (omitting it from the series doesn't make sense, as it would leave a broken state in the middle). I am also fine with leaving it. It seems like a potentially useful addition. I had originally thought after the final one that we could further clean up by turning prepare_submodule_repo_env() into a static function. But we can't; it gets called in one spot from submodule--helper. However, while looking at it, I did notice that we probably want to squash this into the final patch (since sanitize_submodule_config went away completely): diff --git a/submodule.h b/submodule.h index 48690b1..869d259 100644 --- a/submodule.h +++ b/submodule.h @@ -43,19 +43,10 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name); void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir); -/* - * This function is intended as a callback for use with - * git_config_from_parameters(). It ignores any config options which - * are not suitable for passing along to a submodule, and accumulates the rest - * in "data", which must be a pointer to a strbuf. The end result can - * be put into $GIT_CONFIG_PARAMETERS for passing to a sub-process. - */ -int sanitize_submodule_config(const char *var, const char *value, void *data); - /* * Prepare the "env_array" parameter of a "struct child_process" for executing * a submodule by clearing any repo-specific envirionment variables, but - * retaining any config approved by sanitize_submodule_config(). + * retaining any config in the environment. */ void prepare_submodule_repo_env(struct argv_array *out); -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 80/83] run-command: make dup_devnull() non static
Am 05.05.2016 um 11:50 schrieb Christian Couder:
On Mon, Apr 25, 2016 at 5:05 PM, Johannes Schindelin
wrote:
Hi Chris,
On Sun, 24 Apr 2016, Christian Couder wrote:
diff --git a/run-command.c b/run-command.c
index 8c7115a..29d2bda 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
}
#ifndef GIT_WINDOWS_NATIVE
-static inline void dup_devnull(int to)
+void dup_devnull(int to)
{
The #ifndef GIT_WINDOWS_NATIVE rings very, very loud alarm bells.
Yeah, but I must say that I don't know what I should do about this.
Do you have a suggestion? Should I try to implement the same function
for Windows?
No, just remove the #ifndef brackets. There is already code in
compat/mingw.c that treats the file name "/dev/null" specially.
-- Hannes
--
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 v6 1/2] http: support sending custom HTTP headers
On Thu, May 05, 2016 at 09:10:21PM +0200, Lars Schneider wrote:
> > +
> > +
> > + Require expr %{HTTP:x-magic-one} == 'abra'
> > + Require expr %{HTTP:x-magic-two} == 'cadabra'
> > +
>
> I think "" depends on mod_authz_core which is only
> available in Apache HTTPD 2.3 or later [1].
>
> Right now the test only checks if Apache version greater 2
> is installed. Should we guard this test with a special version
> check? Or do you see a way to check the magic values without
> ""?
I think you can get rid of RequireAll with:
Require expr %{HTTP:x-magic-one} == 'abra' && %{HTTP:x-magic-two} == 'cadabra'
But I am also not sure that "expr" existed in Apache 2.2.
I think the older way of checking headers was to do some trickery with
RewriteCond; I tried briefly to make that work when I wrote the test,
but never did (but I'm far from an expert in Apache; the only reason I
have touched it in the last 15 years is for Git's test suite).
The nuclear option would be to put a shell script between Apache and
git-http-backend that checks for those headers (I suspect we'd still
need some magic in the Apache config to pass the headers out in the
environment, though).
-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 11/15] diff: ignore submodules excluded by groups
Any thoughts on my thoughts below? On Fri, Apr 29, 2016 at 12:17 PM, Stefan Beller wrote: > On Fri, Apr 29, 2016 at 11:37 AM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> We do not need to do anything special to initialize the `submodule_groups` >>> pointer as the diff options setup will fill in 0 by default. >>> >>> Signed-off-by: Stefan Beller >>> --- >>> diff.c | 3 +++ >>> diff.h | 1 + >>> 2 files changed, 4 insertions(+) >> >> Isn't this going in the opposite way from what you described in 0/15 >> with analogy to how "ignore" mechanism works? Just like a path is >> tracked once it is tracked, whether it matches an ignore pattern, >> shouldn't we be getting a summary for a submodule for any submodule >> once submodule/.git/HEAD is there (i.e. we can give a comparison), >> whether it is specified by a separate mechanism that acts from >> sideways (e.g. the "default group"). > > That is why I started in 0/15 with >> One pain point I am still aware of: > as I did not do the right thing in these patches? > These patches do however: > >>git status >>git diff >>git submodule summary >># show only changes to submodules which are in the default group. > > which seems to be the wrong thing then. > > So here is a thought experiment: > > # get all submodules into the work tree > git submodule update --recursive --init > > # The selected default group will not include all submodules > git config submodule.defaultGroup "*SomeLabel" > > git status > # What do we expect now? > # either a "nothing to commit, working directory clean" > # or rather what was described in 0/15: > > More than 2 submodules (123 actually) including > 'path/foo' > 'lib/baz' > are checked out, but not part of the default group. > You can add these submodules via > git config --add submodule.defaultGroup ./path/foo > git config --add submodule.defaultGroup ./lib/baz > > If we want to go with the second option, the design described in 0/15 > is broken. Going one step further: > > # in all submodules including the excluded via groups, > # by resetting the groups for this command > git -c submodule.defaultGroup= submodule foreach git reset --hard HEAD^ > > git status > # Reporting the changes from submodules in the default Group is > uncontroversial: > On branch master > Your branch is up-to-date with 'origin/master'. > Changes not staged for commit: > (use "git add ..." to update what will be committed) > (use "git checkout -- ..." to discard changes in working > directory) > > modified: subA (new commits) > modified: subB (new commits) > > no changes added to commit (use "git add" and/or "git commit -a") > > # But what about subC which is not in the default group? It was > changed as well. > # one thing I could imagine, is similar to above: > > More than 2 submodules (123 actually) including > modified:'subC' (new commits) > modified: 'lib/baz' (new commits) > are checked out, but not part of the default group. > You can add these submodules via > git config --add submodule.defaultGroup ./path/foo > git config --add submodule.defaultGroup ./lib/baz > > # and the remaining 121 submodules are not reported in git status > > git diff > # report them all below: > diff --git a/subA b/subA > index e4e79a2..6689f08 16 > --- a/subA > +++ b/subA > @@ -1 +1 @@ > -Subproject commit e4e79a217576d24ef4d73b620766f62b155bcd98 > +Subproject commit 6689f08735d08a057f8d6f91af98b04960afa517 > ... > # goes on including 123 submodule not in the default group. > > > In case we report these submodules which are checked out but not in > the default group, we probably want to adapt "git submodule update" to > un-checkout the work tree of the submodules in case they are clean. > > Thanks, > Stefan -- 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
[PATCHv5] submodule deinit: require '--all' instead of '.' for all submodules
The discussion in [1] pointed out that '.' is a faulty suggestion as
there is a corner case where it fails:
> "submodule deinit ." may have "worked" in the sense that you would
> have at least one path in your tree and avoided this "nothing
> matches" most of the time. It would have still failed with the
> exactly same error if run in an empty repository, i.e.
>
>$ E=/var/tmp/x/empty && rm -fr "$E" && mkdir -p "$E" && cd "$E"
>$ git init
>$ rungit v2.6.6 submodule deinit .
>error: pathspec '.' did not match any file(s) known to git.
>Did you forget to 'git add'?
>$ >file && git add file
>$ rungit v2.6.6 submodule deinit .
>$ echo $?
>0
So instead of a pathspec add the '--all' option to deinit all submodules
and add a test to check for the corner case of an empty repository.
The code only needs to learn about the '--all' option and doesn't
require further changes as `git submodule--helper list "$@"` will list
all submodules when "$@" is empty.
[1] http://news.gmane.org/gmane.comp.version-control.git/289535
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
---
* dropped the short -a option
* dropped the "In version 2.8 and before.." part of the documentation
* be concise and give the usage. This doesn't use the new "die_with_usage"
or "error_with_usage" though.
* fixed the test
Documentation/git-submodule.txt | 19 +--
git-submodule.sh| 15 ---
t/t7400-submodule-basic.sh | 24 +++-
3 files changed, 48 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..ad85183 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[--reference ] [--depth ] [--]
[]
'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
'git submodule' [--quiet] init [--] [...]
-'git submodule' [--quiet] deinit [-f|--force] [--] ...
+'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase|--merge] [--reference ]
[--depth ] [--recursive] [--] [...]
@@ -140,12 +140,15 @@ deinit::
tree. Further calls to `git submodule update`, `git submodule foreach`
and `git submodule sync` will skip any unregistered submodules until
they are initialized again, so use this command if you don't want to
- have a local checkout of the submodule in your work tree anymore. If
+ have a local checkout of the submodule in your working tree anymore. If
you really want to remove a submodule from the repository and commit
that use linkgit:git-rm[1] instead.
+
-If `--force` is specified, the submodule's work tree will be removed even if
-it contains local modifications.
+When the command is run without pathspec, it errors out,
+instead of deinit-ing everything, to prevent mistakes.
++
+If `--force` is specified, the submodule's working tree will
+be removed even if it contains local modifications.
update::
+
@@ -247,6 +250,10 @@ OPTIONS
--quiet::
Only print error messages.
+--all::
+ This option is only valid for the deinit command. Unregister all
+ submodules in the working tree.
+
-b::
--branch::
Branch of repository to add as submodule.
@@ -257,8 +264,8 @@ OPTIONS
--force::
This option is only valid for add, deinit and update commands.
When running add, allow adding an otherwise ignored submodule path.
- When running deinit the submodule work trees will be removed even if
- they contain local changes.
+ When running deinit the submodule working trees will be removed even
+ if they contain local changes.
When running update (only effective with the checkout procedure),
throw away local changes in submodules when switching to a
different commit; and always run a checkout operation in the
diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..5e9b5c0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference
] [--] []
or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
or: $dashless [--quiet] init [--] [...]
- or: $dashless [--quiet] deinit [-f|--force] [--] ...
+ or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--checkout|--merge|--rebase] [--reference ]
[--recursive] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ]
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive]
@@ -521,6 +521,7 @@ cmd_init()
cmd_deinit()
{
# parse $args after "submodule ... deinit".
Re: [PATCH] git-gui: l10n: add Portuguese translation
Às 11:23 de 05-05-2016, Vasco Almeida escreveu: > Add Portuguese glossary. > > Signed-off-by: Vasco Almeida > --- > po/glossary/pt_pt.po | 177 > po/pt_pt.po | 2574 ++ > 2 files changed, 2751 insertions(+) > create mode 100644 po/glossary/pt_pt.po > create mode 100644 po/pt_pt.po > > diff --git a/po/glossary/pt_pt.po b/po/glossary/pt_pt.po > new file mode 100644 > index 000..239b965 > --- /dev/null > +++ b/po/glossary/pt_pt.po > @@ -0,0 +1,177 @@ > +# Portuguese translations for git-gui glossary. > +# Copyright (C) 2016 Shawn Pearce, et al. > +# This file is distributed under the same license as the git package. > +# Vasco Almeida , 2016. > +msgid "" > +msgstr "" > +"Project-Id-Version: git-gui glossary\n" > +"POT-Creation-Date: 2008-01-07 21:20+0100\n" > +"PO-Revision-Date: 2016-05-04 18:44+\n" Oops, I didn't realize I used and outdated pot template. Don't apply this yet, please. I'm updating some translations and will send a re-roll. -- 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 v6 1/2] http: support sending custom HTTP headers
Lars Schneider writes:
[administrivia: next time please snip the 224 lines you are not
responding to when commenting on only five lines].
>> +
>> +
>> +Require expr %{HTTP:x-magic-one} == 'abra'
>> +Require expr %{HTTP:x-magic-two} == 'cadabra'
>> +
>
> I think "" depends on mod_authz_core which is only
> available in Apache HTTPD 2.3 or later [1].
>
> Right now the test only checks if Apache version greater 2
> is installed. Should we guard this test with a special version
> check? Or do you see a way to check the magic values without
> ""?
>
> I only noticed this because I enabled these tests on Travis-CI
> and the Travis Linux box comes with Apache 2.2.22 installed...
>
> - Lars
>
> [1] https://httpd.apache.org/docs/trunk/mod/mod_authz_core.html
2.2 is 12 years old, but the last update is last summer, which is
still fairly recent. It would be really nice if we can accomodate
2.2.x series.
It is only test, so it probably is OK to skip it in the worst case,
but on the other hand, the point of having CI is so that we can
catch bugs that would manifest only in tests that normal people
would not run regularly, so...
Thanks. Real-world value of having Travis canary is seen here.
--
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: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules
On Thu, May 5, 2016 at 12:20 PM, Jonathan Nieder wrote:
> Junio C Hamano wrote:
>> Jonathan Nieder writes:
>
>>> I mean low level as in implementation detail. The human user would
>>> wonder "what is incompatible about them? Why are you stopping me from
>>> what I am trying to do?"
>>
>> Maybe s/incompatible/inconsistent/ is what you are after? Why are
>> you stopping me from what I am trying to do? Because you are asking
>> to do two contradicting things. Do you want to nuke everything, or
>> do you want to keep everything outside what you specified?
>>
>> After being saved countless times from a stupid mistake
>>
>> $ git commit -a -s foo.c
>>
>> that was caused by habitually typing "-a", when I do want to limit
>> the changes to record to a specific path (or two) with similar
>> safety, I do not think "what is incompatible about them" is a valid
>> question.
>
> Yes, 'inconsistent' works better than 'incompatible'. Stepping back,
> what I meant is that when I pass an invalid set of command line
> arguments, it's difficult to give advice back because it's not obvious
> what I intended to do.
>
> When I say "git submodule deinit --all -- foo/", I'm most likely trying
> to deinit all submodules within the foo subdirectory. That's a
> perfectly consistent thing to want --- it just doesn't match what the
> command is expecting. It is more helpful for the command to tell me
> what it is expecting me to do instead instead of just telling me that what
> I gave it is wrong.
>
> The ideal would be something like "git check-attr"'s error_with_usage.
> It tells in a targetted way where my error was and also gives a guide
> of what to do instead.
>
> $ git submodule deinit --all lib/
> error: paths with --all do not make sense
> usage: git submodule deinit [-f | --force] (--all | [--] ...)
>
> Thanks,
> Jonathan
>
> -- >8 --
> Subject: git-sh-setup: add error_with_usage helper
>
> When given an impossible set of options, this allows a command to
> print a targetted message followed by a short usage string that tells
> the user both (1) what part of their command wasn't supported (what
> went wrong) and (2) what correct usage looks like (what to do
> instead).
>
> Signed-off-by: Jonathan Nieder
Thanks for this patch, I can take that as a preparation for the patch
under discussion.
> ---
> git-sh-setup.sh | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index c48139a..2b56636 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -63,6 +63,11 @@ say () {
> fi
> }
>
> +error_with_usage () {
As usage is the last thing (it's die(...) essentially),
I'd rename it to die_with_usage ?
> + printf >&2 'error: %s\n' "$*"
> + usage
> +}
> +
> if test -n "$OPTIONS_SPEC"; then
> usage() {
> "$0" -h
> --
--
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: Submodule's .git file contains absolute path when created using 'git clone --recursive'
On Thu, May 5, 2016 at 12:20 PM, Loet Avramson wrote: > It happened on 2.8.1, also reproducible on 2.8.2. > Haven't had the time to dive deeper into the code but my guess is that > relative_path() returns different results in those 2 cases or maybe > the way git-submodule.sh handles it. > Then you found a new bug, congratulations. ;) Thanks for reporting. The shell script uses relative_path() only for displaying paths, not for writing them to the .git file. it really boils down to different environments "git submodule update --init --recursive" is called from (either manually or from `git clone`). Apart from that there are no immediate bells ringing, are you doing any weird stuff with the file system (soft/hard links) ? Thanks, Stefan -- 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 v16 0/7] config commit verbose
Pranit Bauva writes: > This series of patches add a configuration variable for verbose in > git-commit. > > Link to v15: > http://thread.gmane.org/gmane.comp.version-control.git/293127 > > Changes wrt v15: > * Remove the previous patch 7/7 and split the tests. Include one in >initial patch 6/7. The other one is introduced in a separate commit >after 4/7. > * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose, >--no-verbose with -v as suggested by Eric Sunshine Thanks for a pleasant read. Modulo minor readability nits I sent separately on 7/7, this looked good. A tangent that we may want to think about after this series lands and dust settles is to make test-parse-options simpler to use. I see many instances of this repeated: cat >expect <<\EOF boolean: 0 integer: 0 magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 verbose: 0 quiet: 3 dry run: no file: (not set) EOF test_expect_success 'multiple quiet levels' ' test-parse-options -q -q -q >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' But the only thing this test cares about is if "quiet: 3" is in the output. I think we should be able to write the above 18 lines with just four lines, like this: test_expect_success 'multiple quiet levels' ' test-parse-options --expect="quiet: 3" -q -q -q ' There may be a handful of tests that care about more than one variable, and the current output format must be used when the new --expect option is not given, but I suspect that the majority of tests would want the concise form. -- 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: Submodule's .git file contains absolute path when created using 'git clone --recursive'
It happened on 2.8.1, also reproducible on 2.8.2. Haven't had the time to dive deeper into the code but my guess is that relative_path() returns different results in those 2 cases or maybe the way git-submodule.sh handles it. On Thu, May 5, 2016 at 8:22 PM, Stefan Beller wrote: > On Thu, May 5, 2016 at 5:51 AM, Loet Avramson wrote: >> Hi, >> >> According to git-clone man page - running 'git clone --recursive' "...is >> equivalent to running 'git submodule update --init --recursive' immediately >> after the clone is finished...", though I found a little difference between >> the two regarding the submodule's .git file: >> >> 1. Running 'git clone' and 'git submodule update --init --recursive' >> separately will create the .git file in each submodule containing a relative >> path to the superproject's .git directory as expected. >> >> 2. Running 'git clone --recursive' will create the .git file containing an >> *absolute* path to the superproject's .git directory. (as it was expected >> using git versions 1.7.8 - 1.7.10 as far as I understand) >> >> Not sure if that's a bug but it got stuff behaving really weird in a specific >> usecase on one of our environments. It would be highly appreciated to update >> the docs at least. > > Which version of Git are you using? > > See[1] for how clone handles submodules. (It's a call to submodule > update --init --recursive) > > There was a bug with recursive submodules in the 2.7 time frame and > that got fixed in [2]. > > So could you make sure your version of Git contains these fixes? > > > > [1] > https://kernel.googlesource.com/pub/scm/git/git/+/master/builtin/clone.c#734 > [2] > https://kernel.googlesource.com/pub/scm/git/git/+/7307dd898988c79fc687051e783b3cac8488a559 > specially > https://kernel.googlesource.com/pub/scm/git/git/+/f8eaa0ba98b3bd9cb9035eba184a2d9806d30b27 > > > >> >> Thanks. >> >> -Loet >> >> -- >> 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 -- 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: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules
Junio C Hamano wrote:
> Jonathan Nieder writes:
>> I mean low level as in implementation detail. The human user would
>> wonder "what is incompatible about them? Why are you stopping me from
>> what I am trying to do?"
>
> Maybe s/incompatible/inconsistent/ is what you are after? Why are
> you stopping me from what I am trying to do? Because you are asking
> to do two contradicting things. Do you want to nuke everything, or
> do you want to keep everything outside what you specified?
>
> After being saved countless times from a stupid mistake
>
> $ git commit -a -s foo.c
>
> that was caused by habitually typing "-a", when I do want to limit
> the changes to record to a specific path (or two) with similar
> safety, I do not think "what is incompatible about them" is a valid
> question.
Yes, 'inconsistent' works better than 'incompatible'. Stepping back,
what I meant is that when I pass an invalid set of command line
arguments, it's difficult to give advice back because it's not obvious
what I intended to do.
When I say "git submodule deinit --all -- foo/", I'm most likely trying
to deinit all submodules within the foo subdirectory. That's a
perfectly consistent thing to want --- it just doesn't match what the
command is expecting. It is more helpful for the command to tell me
what it is expecting me to do instead instead of just telling me that what
I gave it is wrong.
The ideal would be something like "git check-attr"'s error_with_usage.
It tells in a targetted way where my error was and also gives a guide
of what to do instead.
$ git submodule deinit --all lib/
error: paths with --all do not make sense
usage: git submodule deinit [-f | --force] (--all | [--] ...)
Thanks,
Jonathan
-- >8 --
Subject: git-sh-setup: add error_with_usage helper
When given an impossible set of options, this allows a command to
print a targetted message followed by a short usage string that tells
the user both (1) what part of their command wasn't supported (what
went wrong) and (2) what correct usage looks like (what to do
instead).
Signed-off-by: Jonathan Nieder
---
git-sh-setup.sh | 5 +
1 file changed, 5 insertions(+)
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c48139a..2b56636 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -63,6 +63,11 @@ say () {
fi
}
+error_with_usage () {
+ printf >&2 'error: %s\n' "$*"
+ usage
+}
+
if test -n "$OPTIONS_SPEC"; then
usage() {
"$0" -h
--
--
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 v16 7/7] commit: add a commit.verbose config variable
Pranit Bauva writes:
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 391126e..114ffc9 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -113,7 +113,9 @@ static char *edit_message, *use_message;
> static char *fixup_message, *squash_message;
> static int all, also, interactive, patch_interactive, only, amend, signoff;
> static int edit_flag = -1; /* unspecified */
> -static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
> +static int config_verbose = -1; /* unspecified */
> +static int verbose = -1; /* unspecified */
> +static int quiet, no_verify, allow_empty, dry_run, renew_authorship;
The name does not make it clear that config_verbose is only for
"commit" and not relevant to "status".
> @@ -1364,6 +1366,8 @@ int cmd_status(int argc, const char **argv, const char
> *prefix)
>builtin_status_usage, 0);
> finalize_colopts(&s.colopts, -1);
> finalize_deferred_config(&s);
> + if (verbose == -1)
> + verbose = 0;
Mental note: cmd_status() does not use git_commit_config() but uses
git_status_config(), hence config_verbose is not affected. But
because verbose is initialised to -1, the code needs to turn it off
like this.
> @@ -1664,6 +1673,9 @@ int cmd_commit(int argc, const char **argv, const char
> *prefix)
> argc = parse_and_validate_options(argc, argv, builtin_commit_options,
> builtin_commit_usage,
> prefix, current_head, &s);
> + if (verbose == -1)
> + verbose = (config_verbose < 0) ? 0 : config_verbose;
> +
cmd_commit() does use git_commit_config(), and verbose is
initialised -1, so without command line option, we fall back to
config_verbose if it is set from the configuration.
I wonder if the attached patch squashed into this commit makes
things easier to understand, though. The points are:
- We rename the configuration to make it clear that it is about
"commit" and does not apply to "status".
- We initialize verbose to 0 as before. The only thing "git
status" cares about is if "--verbose" was given. Giving it
"--no-verbose" or nothing should not make any difference.
- But we do need to stuff -1 to verbose in "git commit" before
handling the command line options, because the distinction
between having "--no-verbose" and not having any matter there,
and we do so in cmd_commit(), i.e. only place where it matters.
builtin/commit.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 583d1e3..a486620 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -113,9 +113,8 @@ static char *edit_message, *use_message;
static char *fixup_message, *squash_message;
static int all, also, interactive, patch_interactive, only, amend, signoff;
static int edit_flag = -1; /* unspecified */
-static int config_verbose = -1; /* unspecified */
-static int verbose = -1; /* unspecified */
-static int quiet, no_verify, allow_empty, dry_run, renew_authorship;
+static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
+static int config_commit_verbose = -1; /* unspecified */
static int no_post_rewrite, allow_empty_message;
static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
static char *sign_commit;
@@ -1366,8 +1365,6 @@ int cmd_status(int argc, const char **argv, const char
*prefix)
builtin_status_usage, 0);
finalize_colopts(&s.colopts, -1);
finalize_deferred_config(&s);
- if (verbose == -1)
- verbose = 0;
handle_untracked_files_arg(&s);
if (show_ignored_in_status)
@@ -1521,7 +1518,7 @@ static int git_commit_config(const char *k, const char
*v, void *cb)
}
if (!strcmp(k, "commit.verbose")) {
int is_bool;
- config_verbose = git_config_bool_or_int(k, v, &is_bool);
+ config_commit_verbose = git_config_bool_or_int(k, v, &is_bool);
return 0;
}
@@ -1670,11 +1667,12 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
if (parse_commit(current_head))
die(_("could not parse HEAD commit"));
}
+ verbose = -1; /* unspecified */
argc = parse_and_validate_options(argc, argv, builtin_commit_options,
builtin_commit_usage,
prefix, current_head, &s);
if (verbose == -1)
- verbose = (config_verbose < 0) ? 0 : config_verbose;
+ verbose = (config_commit_verbose < 0) ? 0 :
config_commit_verbose;
if (dry_run)
return dry_run_commit(argc, argv, prefix, current_head, &s);
--
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 ht
Re: [PATCH v6 1/2] http: support sending custom HTTP headers
On 04 May 2016, at 08:14, Johannes Schindelin
wrote:
> We introduce a way to send custom HTTP headers with all requests.
>
> This allows us, for example, to send an extra token from build agents
> for temporary access to private repositories. (This is the use case that
> triggered this patch.)
>
> This feature can be used like this:
>
> git -c http.extraheader='Secret: sssh!' fetch $URL $REF
>
> Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
> a single list, overriding any previous call. This means we have to
> collect _all_ of the headers we want to use into a single list, and
> feed it to cURL in one shot. Since we already unconditionally set a
> "pragma" header when initializing the curl handles, we can add our new
> headers to that list.
>
> For callers which override the default header list (like probe_rpc),
> we provide `http_copy_default_headers()` so they can do the same
> trick.
>
> Big thanks to Jeff King and Junio Hamano for their outstanding help and
> patient reviews.
>
> Signed-off-by: Johannes Schindelin
> ---
> Documentation/config.txt| 6 ++
> http-push.c | 10 +-
> http.c | 35 ---
> http.h | 1 +
> remote-curl.c | 4 ++--
> t/lib-httpd/apache.conf | 8
> t/t5551-http-fetch-smart.sh | 7 +++
> 7 files changed, 61 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 42d2b50..c7bbe98 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1655,6 +1655,12 @@ http.emptyAuth::
> a username in the URL, as libcurl normally requires a username for
> authentication.
>
> +http.extraHeader::
> + Pass an additional HTTP header when communicating with a server. If
> + more than one such entry exists, all of them are added as extra
> + headers. To allow overriding the settings inherited from the system
> + config, an empty value will reset the extra headers to the empty list.
> +
> http.cookieFile::
> File containing previously stored cookie lines which should be used
> in the Git http session, if they match the server. The file format
> diff --git a/http-push.c b/http-push.c
> index bd60668..ae2b7f1 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
> static struct curl_slist *get_dav_token_headers(struct remote_lock *lock,
> enum dav_header_flag options)
> {
> struct strbuf buf = STRBUF_INIT;
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_copy_default_headers();
>
> if (options & DAV_HEADER_IF) {
> strbuf_addf(&buf, "If: (<%s>)", lock->token);
> @@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
> static void start_move(struct transfer_request *request)
> {
> struct active_request_slot *slot;
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_copy_default_headers();
>
> slot = get_active_slot();
> slot->callback_func = process_response;
> @@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path,
> long timeout)
> char *ep;
> char timeout_header[25];
> struct remote_lock *lock = NULL;
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_copy_default_headers();
> struct xml_ctx ctx;
> char *escaped;
>
> @@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
> struct slot_results results;
> struct strbuf in_buffer = STRBUF_INIT;
> struct buffer out_buffer = { STRBUF_INIT, 0 };
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_copy_default_headers();
> struct xml_ctx ctx;
> struct remote_ls_ctx ls;
>
> @@ -1204,7 +1204,7 @@ static int locking_available(void)
> struct slot_results results;
> struct strbuf in_buffer = STRBUF_INIT;
> struct buffer out_buffer = { STRBUF_INIT, 0 };
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_copy_default_headers();
> struct xml_ctx ctx;
> int lock_flags = 0;
> char *escaped;
> diff --git a/http.c b/http.c
> index 4304b80..985b995 100644
> --- a/http.c
> +++ b/http.c
> @@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
>
> static struct curl_slist *pragma_header;
> static struct curl_slist *no_pragma_header;
> +static struct curl_slist *extra_http_headers;
>
> static struct active_request_slot *active_queue_head;
>
> @@ -323,6 +324,19 @@ static int http_options(const char *var, const char
> *value, void *cb)
> #endif
> }
>
> + if (!strcmp("http.extraheader", var)) {
> + if (!value) {
> + return config_error_nonbool(var);
Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules
On Wed, May 4, 2016 at 4:59 PM, Jonathan Nieder wrote: >>> >>> Another option would be to call 'usage' and be done. >> >> I had that idea as well, but I think pointing out the low level is better >> than giving the high level again, so the user immediately sees what's wrong. > > I mean low level as in implementation detail. The human user would > wonder "what is incompatible about them? Why are you stopping me from > what I am trying to do?" Most likely the user was trying to do > something other than specify a path, since they also passed --all. If > I run something like > > $ git submodule deinit force --all error: unknown option `all' usage: git submodule--helper list [--prefix=] [...] --prefixalternative anchor for relative paths `force` is seen as the first pathspec, so "force --all" is given to the `submodule--helper list`, which gives a less than optimal error message $ git submodule deinit --all force gettext: unrecognized option '--all and pathspec are incompatible' Try 'gettext --help' for more information. envsubst: unrecognized option '--all and pathspec are incompatible' Try 'envsubst --help' for more information. envsubst: unrecognized option '--all and pathspec are incompatible' Try 'envsubst --help' for more information. We should not put --all as the first thing in the error message. $ git submodule deinit --all force pathspec and --all are incompatible With the next patch this is the error message. I think the missing dashes for force are quite visible as force is after --all. > > and the output tells me that --all and pathspec are incompatible then > I just scratch my head more. > > We can do > > USAGE="$dashless [--quiet] deinit [-f|--force] (--all | [--] > ...)" > usage > > to print the subcommand's usage. git commandline tools don't > translate any usage strings today, so not getting translation here > wouldn't feel out of place. We can have both? I'd prefer not rewriting the USAGE string here as it would easily be out of sync in the future? I tried to just grep the deinit line from the USAGe though, but that doesn't look right as it would need some post processing. (remove the "or:") and processing the USAGE string also doesn't sound future proof. -- 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: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules
On Thu, May 5, 2016 at 10:59 AM, Junio C Hamano wrote: > Stefan Beller writes: > +When the command is run without pathspec, it errors out, +instead of deinit-ing everything, to prevent mistakes. In +version 2.8 and before the command gave a suggestion to use +'.' to unregister all submodules when it was invoked without +any argument, but this suggestion did not work and gave a +wrong message if you followed it in pathological cases and is +no longer recommended. >>> >>> Why tell the user what happened in 2.8 and earlier? It's not clear what >>> the reader would do with that information. >> >> Because people may wonder what happened to '.' ? > > I am to blame on that final text, but I think Jonathan is right. > "In version 2.8 and earlier..." can just go. Users may need to > understand why no-arg form is not a silent no-op but an error, > and they need to know how to de-init everything with the version > of Git they have (i.e. with "--all"). Compared to these two, > "Your fingers may have been trained to say '.', but it was found > not to work in pathological cases" is of much lessor importance, > especially because with or without this patch, the definition of > "pathological" cases does not change. So we'll only give When the command is run without pathspec, it errors out, instead of deinit-ing everything, to prevent mistakes. > >>> I think this paragraph could be removed. --all is explained lower >>> down and the error message points it out to users who need it. >> >> When we want to keep supporting '.' forever, I would remove this section. > > I am not sure what you mean by "keep supporting '.'". If your > repository has any tracked path, "deinit ." would deinit all > submodules, with or without this change. > > Are you worried about the future change you are planning to that > involves reverting 84ba959b (submodule: fix regression for deinit > without submodules, 2016-03-22), after which a pathspec that does > not match any submodule would become a "possible typo" error? When redoing the groups series, I may or may not go that route. It sounds compelling to me. > > It is true that '.' would error out if there is no submodule in the > repository, as opposed to erroring out only when there is no tracked > path, which is what you get with today's version (and the version > with this fix in the patch under discussion). But '.' is not > special with respect to that change. 'README' would also error out > if there is no submodule whose path matches that pathspec in that > future version, as opposed to erroring out only if 'README' is not > tracked at all in today's version. > > Or are you thinking that it may be better to give '.' a special > meaning, iow, not treating it as just a regular pathspec? Perhaps > make '.' to mean "everything but it is not an error if there is none > to begin with"? I fear that going in that direction would deform > the mental model the users would form from seeing how commands > behave when given a pathspec. The "." would still look like any > other pathspec elements, and I am sure you will not special case "." > in the usage string but will claim that it is covered by the mention > of at the end of the command line in the usage string, > so you are making them expect that "." used as a pathspec would > behave like that for all other places that we take pathspec, when > in reality, only "submodule deinit" make it behave differently. > > Which I do not think is particularly a good idea. I did not think special casing '.'. (I did in the very first patch, but I understand that it's a bad idea now, so I do not think of it again) > >>> Not about this patch: the organization of options is a little strange. >>> A separate section with options for each subcommand would be easier to >>> read. >> >> I agree. > > I agree. > >>> Do we want to claim the short-and-sweet option -a? (I don't mind but it >>> doesn't seem necessary.) >> >> We do. > > I don't, but I do not care too deeaply. Me neither, so I'll remove the short option. > > @@ -257,8 +270,8 @@ OPTIONS --force:: This option is only valid for add, deinit and update commands. When running add, allow adding an otherwise ignored submodule path. - When running deinit the submodule work trees will be removed even if - they contain local changes. + When running deinit the submodule working trees will be removed even + if they contain local changes. >>> >>> Unrelated change? >> >> It's close enough for deinit to squash it in here, no? > > More importantly, the patch adds a new instance of "working tree" to > the documentation elsewhere; fixing this existing instance of "work > tree" is relevant from consistency's point of view. > @@ -544,9 +548,13 @@ cmd_deinit() shift done - if test $# = 0 + if test -n "$deinit_all" && test "$#" -ne 0 + then
Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules
Jonathan Nieder writes: > I mean low level as in implementation detail. The human user would > wonder "what is incompatible about them? Why are you stopping me from > what I am trying to do?" Maybe s/incompatible/inconsistent/ is what you are after? Why are you stopping me from what I am trying to do? Because you are asking to do two contradicting things. Do you want to nuke everything, or do you want to keep everything outside what you specified? After being saved countless times from a stupid mistake $ git commit -a -s foo.c that was caused by habitually typing "-a", when I do want to limit the changes to record to a specific path (or two) with similar safety, I do not think "what is incompatible about them" is a valid question. -- 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 v2] gitk: Add Portuguese translation
Signed-off-by: Vasco Almeida --- po/pt_pt.po | 1376 +++ 1 file changed, 1376 insertions(+) create mode 100644 po/pt_pt.po diff --git a/po/pt_pt.po b/po/pt_pt.po new file mode 100644 index 000..c181acc --- /dev/null +++ b/po/pt_pt.po @@ -0,0 +1,1376 @@ +# Portuguese translations for gitk package. +# Copyright (C) 2016 Paul Mackerras +# This file is distributed under the same license as the gitk package. +# Vasco Almeida , 2016. +msgid "" +msgstr "" +"Project-Id-Version: gitk\n" +"Report-Msgid-Bugs-To: \n" +"POT-Creation-Date: 2016-04-15 16:52+\n" +"PO-Revision-Date: 2016-05-05 16:54+\n" +"Last-Translator: Vasco Almeida \n" +"Language-Team: Portuguese\n" +"Language: pt\n" +"MIME-Version: 1.0\n" +"Content-Type: text/plain; charset=UTF-8\n" +"Content-Transfer-Encoding: 8bit\n" +"Plural-Forms: nplurals=2; plural=(n != 1);\n" +"X-Generator: Virtaal 0.7.1\n" + +#: gitk:140 +msgid "Couldn't get list of unmerged files:" +msgstr "Não foi possível obter lista de ficheiros não integrados:" + +#: gitk:212 gitk:2399 +msgid "Color words" +msgstr "Colorir palavras" + +#: gitk:217 gitk:2399 gitk:8239 gitk:8272 +msgid "Markup words" +msgstr "Marcar palavras" + +#: gitk:324 +msgid "Error parsing revisions:" +msgstr "Erro ao analisar revisões:" + +#: gitk:380 +msgid "Error executing --argscmd command:" +msgstr "Erro ao executar o comando de --argscmd:" + +#: gitk:393 +msgid "No files selected: --merge specified but no files are unmerged." +msgstr "" +"Nenhum ficheiro selecionado: --merge especificado mas não há ficheiros por " +"integrar." + +#: gitk:396 +msgid "" +"No files selected: --merge specified but no unmerged files are within file " +"limit." +msgstr "" +"Nenhum ficheiro selecionado: --merge especificado mas não há ficheiros por " +"integrar ao nível de ficheiro." + +#: gitk:418 gitk:566 +msgid "Error executing git log:" +msgstr "Erro ao executar git log:" + +#: gitk:436 gitk:582 +msgid "Reading" +msgstr "A ler" + +#: gitk:496 gitk:4544 +msgid "Reading commits..." +msgstr "A ler commits..." + +#: gitk:499 gitk:1637 gitk:4547 +msgid "No commits selected" +msgstr "Nenhum commit selecionado" + +#: gitk:1445 gitk:4064 gitk:12469 +msgid "Command line" +msgstr "Linha de comandos" + +#: gitk:1511 +msgid "Can't parse git log output:" +msgstr "Não é possível analisar a saída de git log:" + +#: gitk:1740 +msgid "No commit information available" +msgstr "Não há informação disponível sobre o commit" + +#: gitk:1903 gitk:1932 gitk:4334 gitk:9702 gitk:11274 gitk:11554 +msgid "OK" +msgstr "OK" + +#: gitk:1934 gitk:4336 gitk:9215 gitk:9294 gitk:9424 gitk:9473 gitk:9704 +#: gitk:11275 gitk:11555 +msgid "Cancel" +msgstr "Cancelar" + +#: gitk:2083 +msgid "&Update" +msgstr "At&ualizar" + +#: gitk:2084 +msgid "&Reload" +msgstr "&Recarregar" + +#: gitk:2085 +msgid "Reread re&ferences" +msgstr "Reler re&ferências" + +#: gitk:2086 +msgid "&List references" +msgstr "&Listar referências" + +#: gitk:2088 +msgid "Start git &gui" +msgstr "Iniciar git &gui" + +#: gitk:2090 +msgid "&Quit" +msgstr "&Sair" + +#: gitk:2082 +msgid "&File" +msgstr "&Ficheiro" + +#: gitk:2094 +msgid "&Preferences" +msgstr "&Preferências" + +#: gitk:2093 +msgid "&Edit" +msgstr "&Editar" + +#: gitk:2098 +msgid "&New view..." +msgstr "&Nova vista..." + +#: gitk:2099 +msgid "&Edit view..." +msgstr "&Editar vista..." + +#: gitk:2100 +msgid "&Delete view" +msgstr "Elimina&r vista" + +#: gitk:2102 +msgid "&All files" +msgstr "&Todos os ficheiros" + +#: gitk:2097 +msgid "&View" +msgstr "&Ver" + +#: gitk:2107 gitk:2117 +msgid "&About gitk" +msgstr "&Sobre o gitk" + +#: gitk:2108 gitk:2122 +msgid "&Key bindings" +msgstr "&Atalhos" + +#: gitk:2106 gitk:2121 +msgid "&Help" +msgstr "&Ajuda" + +#: gitk:2199 gitk:8671 +msgid "SHA1 ID:" +msgstr "ID SHA1:" + +#: gitk:2243 +msgid "Row" +msgstr "Linha" + +#: gitk:2281 +msgid "Find" +msgstr "Procurar" + +#: gitk:2309 +msgid "commit" +msgstr "commit" + +#: gitk:2313 gitk:2315 gitk:4706 gitk:4729 gitk:4753 gitk:6774 gitk:6846 +#: gitk:6931 +msgid "containing:" +msgstr "contendo:" + +#: gitk:2316 gitk:3545 gitk:3550 gitk:4782 +msgid "touching paths:" +msgstr "altera os caminhos:" + +#: gitk:2317 gitk:4796 +msgid "adding/removing string:" +msgstr "adiciona/remove a cadeia:" + +#: gitk:2318 gitk:4798 +msgid "changing lines matching:" +msgstr "altera linhas com:" + +#: gitk:2327 gitk:2329 gitk:4785 +msgid "Exact" +msgstr "Exato" + +#: gitk:2329 gitk:4873 gitk:6742 +msgid "IgnCase" +msgstr "IgnMaiúsculas" + +#: gitk:2329 gitk:4755 gitk:4871 gitk:6738 +msgid "Regexp" +msgstr "Regexp" + +#: gitk:2331 gitk:2332 gitk:4893 gitk:4923 gitk:4930 gitk:6867 gitk:6935 +msgid "All fields" +msgstr "Todos os campos" + +#: gitk:2332 gitk:4890 gitk:4923 gitk:6805 +msgid "Headline" +msgstr "Cabeçalho" + +#: gitk:2333 gitk:4890 gitk:6805 gitk:6935 gitk:7408 +msgid "Comments" +msgstr "Comentários" + +#: gitk:2333 gitk:4890 gitk:4895 gitk:4930 gitk:6805 gitk:7343 gitk:8849 +#: gitk:8864 +msgid
Re: [PATCHv4] submodule deinit: require '--all' instead of '.' for all submodules
Stefan Beller writes: >>> +When the command is run without pathspec, it errors out, >>> +instead of deinit-ing everything, to prevent mistakes. In >>> +version 2.8 and before the command gave a suggestion to use >>> +'.' to unregister all submodules when it was invoked without >>> +any argument, but this suggestion did not work and gave a >>> +wrong message if you followed it in pathological cases and is >>> +no longer recommended. >> >> Why tell the user what happened in 2.8 and earlier? It's not clear what >> the reader would do with that information. > > Because people may wonder what happened to '.' ? I am to blame on that final text, but I think Jonathan is right. "In version 2.8 and earlier..." can just go. Users may need to understand why no-arg form is not a silent no-op but an error, and they need to know how to de-init everything with the version of Git they have (i.e. with "--all"). Compared to these two, "Your fingers may have been trained to say '.', but it was found not to work in pathological cases" is of much lessor importance, especially because with or without this patch, the definition of "pathological" cases does not change. >> I think this paragraph could be removed. --all is explained lower >> down and the error message points it out to users who need it. > > When we want to keep supporting '.' forever, I would remove this section. I am not sure what you mean by "keep supporting '.'". If your repository has any tracked path, "deinit ." would deinit all submodules, with or without this change. Are you worried about the future change you are planning to that involves reverting 84ba959b (submodule: fix regression for deinit without submodules, 2016-03-22), after which a pathspec that does not match any submodule would become a "possible typo" error? It is true that '.' would error out if there is no submodule in the repository, as opposed to erroring out only when there is no tracked path, which is what you get with today's version (and the version with this fix in the patch under discussion). But '.' is not special with respect to that change. 'README' would also error out if there is no submodule whose path matches that pathspec in that future version, as opposed to erroring out only if 'README' is not tracked at all in today's version. Or are you thinking that it may be better to give '.' a special meaning, iow, not treating it as just a regular pathspec? Perhaps make '.' to mean "everything but it is not an error if there is none to begin with"? I fear that going in that direction would deform the mental model the users would form from seeing how commands behave when given a pathspec. The "." would still look like any other pathspec elements, and I am sure you will not special case "." in the usage string but will claim that it is covered by the mention of at the end of the command line in the usage string, so you are making them expect that "." used as a pathspec would behave like that for all other places that we take pathspec, when in reality, only "submodule deinit" make it behave differently. Which I do not think is particularly a good idea. >> Not about this patch: the organization of options is a little strange. >> A separate section with options for each subcommand would be easier to >> read. > > I agree. I agree. >> Do we want to claim the short-and-sweet option -a? (I don't mind but it >> doesn't seem necessary.) > > We do. I don't, but I do not care too deeaply. >>> @@ -257,8 +270,8 @@ OPTIONS >>> --force:: >>> This option is only valid for add, deinit and update commands. >>> When running add, allow adding an otherwise ignored submodule path. >>> - When running deinit the submodule work trees will be removed even if >>> - they contain local changes. >>> + When running deinit the submodule working trees will be removed even >>> + if they contain local changes. >> >> Unrelated change? > > It's close enough for deinit to squash it in here, no? More importantly, the patch adds a new instance of "working tree" to the documentation elsewhere; fixing this existing instance of "work tree" is relevant from consistency's point of view. >>> @@ -544,9 +548,13 @@ cmd_deinit() >>> shift >>> done >>> >>> - if test $# = 0 >>> + if test -n "$deinit_all" && test "$#" -ne 0 >>> + then >>> + die "$(eval_gettext "--all and pathspec are incompatible")" >> >> This message still feels too low-level to me, but I might be swimming >> uphill here. >> >> Another option would be to call 'usage' and be done. > > I had that idea as well, but I think pointing out the low level is better > than giving the high level again, so the user immediately sees what's wrong. I do not particularly see the message low-level. Jonathan, what do you have against pointing out the exact problem? After seeing the usage string that also talks about --quite, --force, etc., I have to somehow realize that these are irrele
[PATCH] gitk: Makefile: create install bin directory
Force creation of destination bin directory. Before this commit, gitk would fail to install if this directory didn't already exist. Signed-off-by: Vasco Almeida --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 5acdc90..5bdd52a 100644 --- a/Makefile +++ b/Makefile @@ -50,6 +50,7 @@ endif all:: gitk-wish $(ALL_MSGFILES) install:: all + $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)' $(INSTALL) -m 755 gitk-wish '$(DESTDIR_SQ)$(bindir_SQ)'/gitk $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(msgsdir_SQ)' $(foreach p,$(ALL_MSGFILES), $(INSTALL) -m 644 $p '$(DESTDIR_SQ)$(msgsdir_SQ)' &&) true -- 2.7.3 -- 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] git-gui: Do not reset author details on amend
Pat, we haven't heard from you for a long time. Are you still
around and interested in helping us by maintaining git-gui?
Otherwise we may have to start recruiting a volunteer or two to take
this over.
Thanks.
Orgad Shaneh writes:
> git commit --amend preserves the author details unless --reset-author is
> given.
>
> git-gui discards the author details on amend.
>
> Fix by reading the author details along with the commit message, and
> setting the appropriate environment variables required for preserving
> them.
>
> Reported long ago in the mailing list[1].
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/243921
>
> Signed-off-by: Orgad Shaneh
> ---
> git-gui/lib/commit.tcl | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
> index 864b687..60edf99 100644
> --- a/git-gui/lib/commit.tcl
> +++ b/git-gui/lib/commit.tcl
> @@ -1,8 +1,13 @@
> # git-gui misc. commit reading/writing support
> # Copyright (C) 2006, 2007 Shawn Pearce
>
> +set author_name ""
> +set author_email ""
> +set author_date ""
> +
> proc load_last_commit {} {
> global HEAD PARENT MERGE_HEAD commit_type ui_comm
> + global author_name author_email author_date
> global repo_config
>
> if {[llength $PARENT] == 0} {
> @@ -34,6 +39,10 @@ You are currently in the middle of a merge that has not
> been fully completed. Y
> lappend parents [string range $line 7
> end]
> } elseif {[string match {encoding *} $line]} {
> set enc [string tolower [string range
> $line 9 end]]
> + } elseif {[regexp "author
> (.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} {
> + set author_name $name
> + set author_email $email
> + set author_date $time
> }
> }
> set msg [read $fd]
> @@ -107,8 +116,12 @@ proc do_signoff {} {
>
> proc create_new_commit {} {
> global commit_type ui_comm
> + global author_name author_email author_date
>
> set commit_type normal
> + set author_name ""
> + set author_email ""
> + set author_date ""
> $ui_comm delete 0.0 end
> $ui_comm edit reset
> $ui_comm edit modified false
> @@ -327,6 +340,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {
> global ui_comm selected_commit_type
> global file_states selected_paths rescan_active
> global repo_config
> + global env author_name author_email author_date
>
> gets $fd_wt tree_id
> if {[catch {close $fd_wt} err]} {
> @@ -366,6 +380,11 @@ A rescan will be automatically started now.
> }
> }
>
> + if {$author_name ne ""} {
> + set env(GIT_AUTHOR_NAME) $author_name
> + set env(GIT_AUTHOR_EMAIL) $author_email
> + set env(GIT_AUTHOR_DATE) $author_date
> + }
> # -- Create the commit.
> #
> set cmd [list commit-tree $tree_id]
--
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: Submodule's .git file contains absolute path when created using 'git clone --recursive'
On Thu, May 5, 2016 at 5:51 AM, Loet Avramson wrote: > Hi, > > According to git-clone man page - running 'git clone --recursive' "...is > equivalent to running 'git submodule update --init --recursive' immediately > after the clone is finished...", though I found a little difference between > the two regarding the submodule's .git file: > > 1. Running 'git clone' and 'git submodule update --init --recursive' > separately will create the .git file in each submodule containing a relative > path to the superproject's .git directory as expected. > > 2. Running 'git clone --recursive' will create the .git file containing an > *absolute* path to the superproject's .git directory. (as it was expected > using git versions 1.7.8 - 1.7.10 as far as I understand) > > Not sure if that's a bug but it got stuff behaving really weird in a specific > usecase on one of our environments. It would be highly appreciated to update > the docs at least. Which version of Git are you using? See[1] for how clone handles submodules. (It's a call to submodule update --init --recursive) There was a bug with recursive submodules in the 2.7 time frame and that got fixed in [2]. So could you make sure your version of Git contains these fixes? [1] https://kernel.googlesource.com/pub/scm/git/git/+/master/builtin/clone.c#734 [2] https://kernel.googlesource.com/pub/scm/git/git/+/7307dd898988c79fc687051e783b3cac8488a559 specially https://kernel.googlesource.com/pub/scm/git/git/+/f8eaa0ba98b3bd9cb9035eba184a2d9806d30b27 > > Thanks. > > -Loet > > -- > 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 -- 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: Submodule's .git file contains absolute path when created using 'git clone --recursive'
Loet Avramson writes: > According to git-clone man page - running 'git clone --recursive' "...is > equivalent to running 'git submodule update --init --recursive' immediately > after the clone is finished...", though I found a little difference between > the two regarding the submodule's .git file: > > 1. Running 'git clone' and 'git submodule update --init --recursive' > separately will create the .git file in each submodule containing a relative > path to the superproject's .git directory as expected. > > 2. Running 'git clone --recursive' will create the .git file containing an > *absolute* path to the superproject's .git directory. (as it was expected > using git versions 1.7.8 - 1.7.10 as far as I understand) > > Not sure if that's a bug but it got stuff behaving really weird in a specific > usecase on one of our environments. It would be highly appreciated to update > the docs at least. If the documentation already says "equivalent" and does not say "identical", I am not sure if there is anything to update. In any case, I thought some people are already working to make the result consistently use relative paths (or was it absolute?), so this may soon become a non-issue. -- 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: stop sanitizing config options
Jeff King writes:
> Here's a version of my patch that should apply for you (no
> semantic changes, just differences in the surrounding context):
Sorry for the trouble.
I queued jk/submodule-c-credential to be mergeable to 'maint', as it
could be argued two ways. We can say that not propagating -c down
is a bug and the series is a bugfix. Or it is merely a known
limitation and the series is a new feature. I was leaning towards
the former, but I was also willing to declare that is a known bug
that will left unfixed in the maintenance track.
If I knew the 5 patches on jk/submodule-config-sanitize-fix was what
we would eventually agree to be the right change, I would have
applied them directly on top of jk/submodule-c-credential instead of
forking a separate branch for them. That way, either the "bugfix"
would all go to 'maint', or the "feature" wouldn't, and we do not
have to worry about making a mistake to merge only half-done state
(i.e. jk/submodule-c-credential that passes only the credential.*
configuration) that nobody would want to 'maint'.
But when I picked up jk/submodule-config-sanitize-fix, I didn't have
enough time to think things through or carefully read the discussion
to convince myself that we already had a list concensus, so I queued
it separately only to make sure I won't lose track of it--I can come
back to it later that way.
It probably is a good time to merge jk/submodule-config-sanitize-fix
into jk/submodule-c-credential (i.e. a mere fast-forward), remove
that "-fix" branch, and apply this patch directly on top of the
resulting jk/submodule-c-credential. That will make the whole thing
a 13-patch series, consisting of:
7 patches up to the current jk/submodule-c-credential
d1f8849 git_config_push_parameter: handle empty GIT_CONFIG_PARAMETERS
14111fc git: submodule honor -c credential.* from command line
e70986d quote: implement sq_quotef()
7dad263 submodule: fix segmentation fault in submodule--helper clone
717416c submodule: fix submodule--helper clone usage
08e0970 submodule: check argc count for git submodule--helper clone
d10e3b4 submodule: don't pass empty string arguments to submodule--helper
clone
5 patches up to the current jk/submodule-config-sanitize-fix
c12e865 submodule: use prepare_submodule_repo_env consistently
4638728 submodule--helper: move config-sanitizing to submodule.c
860cba6 submodule: export sanitized GIT_CONFIG_PARAMETERS
455d22c t5550: break submodule config test into multiple sub-tests
1149ee2 t5550: fix typo in $HTTPD_URL
1 patch (this one)
4e6706a submodule: stop sanitizing config options
whose early 7 patches have already been merged to 'master' (and none
has been merged to 'maint').
> -- >8 --
> Subject: [PATCH] submodule: stop sanitizing config options
>
> The point of having a whitelist of command-line config
> options to pass to submodules was two-fold:
>
> 1. It prevented obvious nonsense like using core.worktree
> for multiple repos.
>
> 2. It could prevent surprise when the user did not mean
> for the options to leak to the submodules (e.g.,
> http.sslverify=false).
>
> For case 1, the answer is mostly "if it hurts, don't do
> that". For case 2, we can note that any such example has a
> matching inverted surprise (e.g., a user who meant
> http.sslverify=true to apply everywhere, but it didn't).
>
> So this whitelist is probably not giving us any benefit, and
> is already creating a hassle as people propose things to put
> on it. Let's just drop it entirely.
>
> Note that we still need to keep a special code path for
> "prepare the submodule environment", because we still have
> to take care to pass through $GIT_CONFIG_PARAMETERS (and
> block the rest of the repo-specific environment variables).
>
> We can do this easily from within the submodule shell
> script, which lets us drop the submodule--helper option
> entirely (and it's OK to do so because as a "--" program, it
> is entirely a private implementation detail).
>
> Signed-off-by: Jeff King
> ---
> builtin/submodule--helper.c | 17 -
> git-submodule.sh | 4 ++--
> submodule.c | 39 +--
> t/t7412-submodule--helper.sh | 26 --
> 4 files changed, 3 insertions(+), 83 deletions(-)
> delete mode 100755 t/t7412-submodule--helper.sh
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 16d6432..89250f0 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -260,22 +260,6 @@ static int module_clone(int argc, const char **argv,
> const char *prefix)
> return 0;
> }
>
> -static int module_sanitize_config(int argc, const char **argv, const char
> *prefix)
> -{
> - struct strbuf sanitized_config = STRBUF_INIT;
> -
> - if (argc > 1)
> - usage(_("git submodule--helper sanitize-config"));
> -
> - git_config_from_parameters(sanitize_submodule_config,
Re: [RFC PATCH] gc --auto: don't lie about running in background on Windows
SZEDER Gábor writes:
> Arguably this helper function could be just a simple variable. I
> opted for a function because:
>
> - I preferred a single '#ifdef NO_POSIX_GOODIES', and putting a
> static variable so near to EOF felt just wrong. (And this is why
> it's not an inline-able function defined in a header file.)
>
> - currently we know already at compile time that Windows can't
> daemonize, but in the future we might want to extend this helper
> function to perform some runtime checks, too. But this is perhaps
> like preparing for crossing a bridge where we'll never get to.
Alternatively, the implementation of daemonize() and can_daemonize()
can live in compat/ and have the #ifdef switch in git-compat-util.h,
e.g. something along the lines of these:
<< git-compat-util.h >>
... after conditional inclusion of compat/mingw.h ...
#ifndef can_daemonize
#define can_daemonize() 1
#endif
<< compat/mingw.h >>
#define can_daemonize() 0
#define daemonize() mingw_daemonize()
<< setup.c >>
...
#ifndef NO_POSIX_GOODIES
int daemonize(void)
{
... no ifdef around here ...
}
#endif
We can be even more purist and move the daemonize() implementation
for POSIX to compat/posix.c to keep the generic part even more
platform agnostic, which would remove the only #ifdef in setup.c,
but that might be going a bit too far.
These possible implementation choices aside, the goal of this patch
is a worthwhile thing to do, I would think.
Thanks.
> builtin/gc.c | 1 +
> cache.h | 1 +
> setup.c | 17 +++--
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index c583aad6ec28..79a0f6dc1126 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -368,6 +368,7 @@ int cmd_gc(int argc, const char **argv, const char
> *prefix)
>*/
> if (!need_to_gc())
> return 0;
> + detach_auto &= can_daemonize();
> if (!quiet) {
> if (detach_auto)
> fprintf(stderr, _("Auto packing the repository
> in background for optimum performance.\n"));
> diff --git a/cache.h b/cache.h
> index fd728f079320..3662e5aabb98 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -524,6 +524,7 @@ extern int set_git_dir_init(const char *git_dir, const
> char *real_git_dir, int);
> extern int init_db(const char *template_dir, unsigned int flags);
>
> extern void sanitize_stdfds(void);
> +extern int can_daemonize(void);
> extern int daemonize(void);
>
> #define alloc_nr(x) (((x)+16)*3/2)
> diff --git a/setup.c b/setup.c
> index c86bf5c9fabe..6187a4ad9c47 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1033,12 +1033,25 @@ void sanitize_stdfds(void)
> close(fd);
> }
>
> +#ifdef NO_POSIX_GOODIES
> +int can_daemonize(void)
> +{
> + return 0;
> +}
> +
> int daemonize(void)
> {
> -#ifdef NO_POSIX_GOODIES
> errno = ENOSYS;
> return -1;
> +}
> #else
> +int can_daemonize(void)
> +{
> + return 1;
> +}
> +
> +int daemonize(void)
> +{
> switch (fork()) {
> case 0:
> break;
> @@ -1054,5 +1067,5 @@ int daemonize(void)
> close(2);
> sanitize_stdfds();
> return 0;
> -#endif
> }
> +#endif /* #ifdef NO_POSIX_GOODIES */
--
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] gc --auto: don't lie about running in background on Windows
When 'git gc --auto' is invoked it tells the user whether it is about
to auto pack the repository in the background or in the foreground
depending on 'gc.autoDetach' (enabled by default). However, going to
the background is not supported at all on Windows, yet 'git gc --auto'
by default claims that it will do so.
Add a helper function that can tell whether the platform supports
going to the background and use it in the 'git gc --auto' codepath to
print an honest message.
Signed-off-by: SZEDER Gábor
---
I built and tested with 'NO_POSIX_GOODIES=UnfortunatelyYes': it
worked. However, I did not test it on Windows due to lack of
resources, hence the RFC out of caution.
Arguably this helper function could be just a simple variable. I
opted for a function because:
- I preferred a single '#ifdef NO_POSIX_GOODIES', and putting a
static variable so near to EOF felt just wrong. (And this is why
it's not an inline-able function defined in a header file.)
- currently we know already at compile time that Windows can't
daemonize, but in the future we might want to extend this helper
function to perform some runtime checks, too. But this is perhaps
like preparing for crossing a bridge where we'll never get to.
builtin/gc.c | 1 +
cache.h | 1 +
setup.c | 17 +++--
3 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad6ec28..79a0f6dc1126 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -368,6 +368,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
*/
if (!need_to_gc())
return 0;
+ detach_auto &= can_daemonize();
if (!quiet) {
if (detach_auto)
fprintf(stderr, _("Auto packing the repository
in background for optimum performance.\n"));
diff --git a/cache.h b/cache.h
index fd728f079320..3662e5aabb98 100644
--- a/cache.h
+++ b/cache.h
@@ -524,6 +524,7 @@ extern int set_git_dir_init(const char *git_dir, const char
*real_git_dir, int);
extern int init_db(const char *template_dir, unsigned int flags);
extern void sanitize_stdfds(void);
+extern int can_daemonize(void);
extern int daemonize(void);
#define alloc_nr(x) (((x)+16)*3/2)
diff --git a/setup.c b/setup.c
index c86bf5c9fabe..6187a4ad9c47 100644
--- a/setup.c
+++ b/setup.c
@@ -1033,12 +1033,25 @@ void sanitize_stdfds(void)
close(fd);
}
+#ifdef NO_POSIX_GOODIES
+int can_daemonize(void)
+{
+ return 0;
+}
+
int daemonize(void)
{
-#ifdef NO_POSIX_GOODIES
errno = ENOSYS;
return -1;
+}
#else
+int can_daemonize(void)
+{
+ return 1;
+}
+
+int daemonize(void)
+{
switch (fork()) {
case 0:
break;
@@ -1054,5 +1067,5 @@ int daemonize(void)
close(2);
sanitize_stdfds();
return 0;
-#endif
}
+#endif /* #ifdef NO_POSIX_GOODIES */
--
2.8.2.356.ge684b1d
--
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] t5510: run auto-gc in the foreground
Quoting Johannes Schindelin :
Hi Gábor,
On Tue, 3 May 2016, SZEDER Gábor wrote:
Quoting SZEDER Gábor :
>Quoting Johannes Schindelin :
>
> >Hi Gábor,
> >
> >On Sun, 1 May 2016, SZEDER Gábor wrote:
> >
> > >diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> > >index 38321d19efbe..454d896390c0 100755
> > >--- a/t/t5510-fetch.sh
> > >+++ b/t/t5510-fetch.sh
> > >@@ -682,6 +682,7 @@ test_expect_success 'fetching with auto-gc does
> > >not lock up' '
> > > (
> > > cd auto-gc &&
> > > git config gc.autoPackLimit 1 &&
> > >+ git config gc.autoDetach false &&
> > > GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
> > > ! grep "Should I try again" fetch.out
> > > )
> >
> >Sounds good to me.
>
>There is something still bothering me, though.
>
>I take this was a Windows-specific issue; deleting open files on Linux is
>no brainer. According to a comment on the original Git for Windows issue
>at github[1], 'git gc' runs in the background by default on
Windows as well.
Ok, having slept on it, it was a false alarm.
Though 'git gc --auto' claims "Auto packing the repository in background for
optimum performance." on Windows, it does in fact runs in the foreground.
Thanks for checking. I ran out of time yesterday.
'git gc --auto' first prints that message, unless gc.autoDetach is disabled,
and then calls daemonize() to go to the background. However, daemonize() is
basically a no-op on Windows, thus 'git gc' will remain in the
foreground and
the sequence I described below is impossible. Good.
Oh, right. I think this will take a lot of energy to fix: daemonize()'s
functionality is not really available, indeed. What *is* available is a
spawn() that detaches the new process.
Perhaps it would be worth updating 'git gc' to not lie about going to the
background when we can already know in advance that it won't.
Hmm. https://github.com/git/git/blob/master/builtin/gc.c#L372-L373
already looks correct (should it really know that we cannot daemonize()?
if (detach_auto)
fprintf(stderr, _("Auto packing the repository in background for
optimum performance.\n"));
else
fprintf(stderr, _("Auto packing the repository for optimum
performance.\n"));
It is only correct as far as the value of the 'gc.autoDetach' config
variable is concerned, which is enabled by default, even on Windows, where
it can't go to the background.
(should it really know that we cannot daemonize()?
What about other code paths using that function?):
daemonize() is currently only used in the 'git gc --auto' code path and in
'git daemon', which doesn't announce that it is about to go in the
background. Then there is the WIP index-helper, which will use daemonize()
as well, but it won't announce that either.
Being the sole callsite that makes such an announcement, I think it should
know or at least should check whether "daemonizing" is possible. (as
opposed to e.g. teaching daemonize() to print such messages)
Gábor
--
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] git-gui: Do not reset author details on amend
git commit --amend preserves the author details unless --reset-author is
given.
git-gui discards the author details on amend.
Fix by reading the author details along with the commit message, and
setting the appropriate environment variables required for preserving
them.
Reported long ago in the mailing list[1].
[1] http://article.gmane.org/gmane.comp.version-control.git/243921
Signed-off-by: Orgad Shaneh
---
git-gui/lib/commit.tcl | 19 +++
1 file changed, 19 insertions(+)
diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 864b687..60edf99 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -1,8 +1,13 @@
# git-gui misc. commit reading/writing support
# Copyright (C) 2006, 2007 Shawn Pearce
+set author_name ""
+set author_email ""
+set author_date ""
+
proc load_last_commit {} {
global HEAD PARENT MERGE_HEAD commit_type ui_comm
+ global author_name author_email author_date
global repo_config
if {[llength $PARENT] == 0} {
@@ -34,6 +39,10 @@ You are currently in the middle of a merge that has not been
fully completed. Y
lappend parents [string range $line 7
end]
} elseif {[string match {encoding *} $line]} {
set enc [string tolower [string range
$line 9 end]]
+ } elseif {[regexp "author
(.*)\\s<(.*)>\\s(\\d.*$)" $line all name email time]} {
+ set author_name $name
+ set author_email $email
+ set author_date $time
}
}
set msg [read $fd]
@@ -107,8 +116,12 @@ proc do_signoff {} {
proc create_new_commit {} {
global commit_type ui_comm
+ global author_name author_email author_date
set commit_type normal
+ set author_name ""
+ set author_email ""
+ set author_date ""
$ui_comm delete 0.0 end
$ui_comm edit reset
$ui_comm edit modified false
@@ -327,6 +340,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {
global ui_comm selected_commit_type
global file_states selected_paths rescan_active
global repo_config
+ global env author_name author_email author_date
gets $fd_wt tree_id
if {[catch {close $fd_wt} err]} {
@@ -366,6 +380,11 @@ A rescan will be automatically started now.
}
}
+ if {$author_name ne ""} {
+ set env(GIT_AUTHOR_NAME) $author_name
+ set env(GIT_AUTHOR_EMAIL) $author_email
+ set env(GIT_AUTHOR_DATE) $author_date
+ }
# -- Create the commit.
#
set cmd [list commit-tree $tree_id]
--
2.8.1
--
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
