Re: git submodule should honor "-c credential.helper" command line argument
On Thu, Feb 18, 2016 at 11:46 PM, Jeff King wrote: > To trigger a credential fetch in actual use, you have to clone over > http. See the credential tests in t5550, for example. > I'll look at these. >> As for how to whitelist config to share with the submodule I am really >> not 100% sure, since we just clear GIT_CONFIG_PARAMETERS, and I think >> we'd need a specialized variant of clear_local_git_env_vars specific >> to submodule then. > > Yeah, you'll have to parse, which is pretty painful. In C, you'd do > something like: > > but right now git-submodule.sh is all in shell. You'd probably need a > special helper from git-submodule--helper, though it might simply make > sense to put this off until the submodule code is fully ported to C. > I think the best approach is probably to put this off since I am not 100% sure how we'd handle environment variables from swapping into submoduler--helper and out. That seems really finicky and likely to go away once the full port occurs so I am not sure it's worth it. > -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: git submodule should honor "-c credential.helper" command line argument
On Thu, Feb 18, 2016 at 11:29:09PM -0800, Jacob Keller wrote:
> I would prefer to either.. blacklist stuff like core.worktree, or
> whitelist a bunch of stuff that makes sense. In this case though, I
> would prefer to have an explicit test of credential.helper, but I
> don't know if any of our tests actually have a solid test case for
> "credential.helper was used in a clone. There may not be test
> infrastructure for this though, so your test might work well enough.
To trigger a credential fetch in actual use, you have to clone over
http. See the credential tests in t5550, for example.
> As for how to whitelist config to share with the submodule I am really
> not 100% sure, since we just clear GIT_CONFIG_PARAMETERS, and I think
> we'd need a specialized variant of clear_local_git_env_vars specific
> to submodule then.
Yeah, you'll have to parse, which is pretty painful. In C, you'd do
something like:
int submodule_config_ok(const char *var)
{
if (starts_with(var, "credential."))
return 1;
return 0;
}
int filter_submodule_config(const char *var, const char *value, void *data)
{
struct strbuf *out = data;
if (submodule_config_ok(var)) {
if (out->len)
strbuf_addch(out, ' ');
/* these actually probably need quoted all as * one string */
sq_quote_buf(out, var);
sq_quote_buf(out, "=");
sq_quote_buf(out, value);
}
return 0;
}
and then call it like:
struct strbuf filtered_config = STRBUF_INIT;
git_config_from_parameters(filter_submodule_config, &filtered_config);
argv_array_pushf(&child_process.env, "%s=%s",
CONFIG_DATA_ENVIRONMENT, filtered_config.buf);
but right now git-submodule.sh is all in shell. You'd probably need a
special helper from git-submodule--helper, though it might simply make
sense to put this off until the submodule code is fully ported to C.
-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: GSoC 2016: applications open, deadline = Fri, 19/2
Junio C Hamano writes: > Deciding what the 'safe' subset is must be done with a lot of > thinking by people who intimately know what implications it has to > ban each feature. I do not think it would be a good fit for a > project to give to a relatively new participant to the Git project. I have to agree with this: this would actually be very hard to get a nice proposal from a student. Students can be good technically, but we can't expect them to be experienced and giving sound advices to beginners is hard in this situation. > We have these "powerful" tools for a reason. After making a mess > experimenting with your working tree files, "reset --hard" is the > best tool to go back to the known-good state, I disagree with that. This reminds me a discussion I had with a student a few years ago: student: how do a clear all changes from my worktree? me: git reset --hard the next day: student: OK, now, how do I get my changes back? me: ...! There's almost no situation where reset --hard is the best tool. If you just want to discard local changes, then "stash" is much safer (it'll eat a bit of your disk space, but in 99% cases it's not an issue). If you messed up a merge then "merge --abort" is safer. If the goal is to move HEAD, then "reset --keep" is safer. One thing I like about Git is: when a beginner messes up his tree or repo, his Git guru friend can almost always repair it easily (at least, much easier than it was with svn). But there are still a few ways for beginners to shoot themselves in the foot in a way that the guru cannot repair. Now, another issue with the proposed core.isbeginner is compatibility with scripts. Dangerous commands are often plumbing, and a beginner may still want to use scripts or other porcelain on top of it. Typically, I think this rules out "git reset --hard" which is legitimate in scripts. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 submodule should honor "-c credential.helper" command line argument
On Thu, Feb 18, 2016 at 8:30 PM, Jeff King wrote: > On Thu, Feb 18, 2016 at 05:15:54PM -0800, Jacob Keller wrote: > >> I am looking at this more and I am stuck as to how best to provide a >> test case. >> >> I think the problem as stated above is pretty straight forward, we >> just want to stop clearing GIT_CONFIG_PARAMETERS but I can't find an >> easy way to test that we've done the right thing. There are no current >> tests for using a credential helper with submodule update right now. > > If you just want to test that GIT_CONFIG_PARAMETERS is left untouched in > the submodule, you can tweak any config setting that would impact the > newly-cloned repo. E.g. this: > > unset GIT_COMMITTER_NAME > git config --global user.name='Global Name' > git -c user.name='Command-Line Name' clone repo-with-module foo > head -1 foo/.git/logs/HEAD > head -1 foo/.git/modules/sub/logs/HEAD > > shows that the parent-level clone uses the "-c" name, but the submodule > does not. > > That being said, I am not sure this is the right solution. In the thread > I linked earlier[1], Jens indicated he would prefer not to blindly share > config with the submodules, and I think I agree. Or are you proposing to > pick and choose the keys in GIT_CONFIG_PARAMETERS, and whitelist > credential.*? > > In that case, obviously my test example would not work, though I think > that it might be fine to put "user.name" onto the whitelist (the things > we really would worry about is stuff like "core.worktree" that clearly > does not make sense to carry over into the submodule). > > -Peff > > [1] http://thread.gmane.org/gmane.comp.version-control.git/264840 > I would prefer to either.. blacklist stuff like core.worktree, or whitelist a bunch of stuff that makes sense. In this case though, I would prefer to have an explicit test of credential.helper, but I don't know if any of our tests actually have a solid test case for "credential.helper was used in a clone. There may not be test infrastructure for this though, so your test might work well enough. As for how to whitelist config to share with the submodule I am really not 100% sure, since we just clear GIT_CONFIG_PARAMETERS, and I think we'd need a specialized variant of clear_local_git_env_vars specific to submodule then. Thanks, Jake -- 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: GSoC 2016: applications open, deadline = Fri, 19/2
Duy Nguyen writes: > On Thu, Feb 18, 2016 at 1:58 AM, Matthieu Moy > wrote: >> Feel free to start writting an idea for >> http://git.github.io/SoC-2016-Ideas/. It'd be nice to have a few more >> ideas before Friday. We can polish them later if needed. > > Probably too late now, anyway.. It's still time. I'll post the application very soon (a few hours from now), but the idea list is not included in the application, but linked from it. So we can add something before reviewers follow the link, and obviously we can add more before students start picking them. > with David's multiple ref backend work, we could have a third, > no-dependency backend. We can use index format to store refs. This sounds like an interesting but ambitious project for a GSoC. There are a lot of new stuff to understand for someone potentially new to Git's codebase. And it's hard to work incrementally: the result would hardly be mergeable before being almost finished. I think it's interesting to offer the idea, but there should be a warning for the student about the difficulties. Would you be willing to (co-)mentor? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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
GSoC 2016
Hello everyone, I'm Mehul Jain. I'm looking for participating in GSoC 2016. I've started work on a Microproject" Teach git pull --rebase the --no-autostash" option. While looking at Git's source code I have made following observation: In the pull.c file 1. git_config_get_bool( , ) search in the configuration key for the value of rebase.autostash, if found true then modify autostash's value to a non-zero number and thus making a stash to encounter the problem of dirty tree. 2. Here if in command line a flag "--no-autostash" is given then we can easily set the value of autostash = 0 and thus killing the process by die_on_unclean_work_tree(prefix). Is my observation is right? I'm new to open source projects and not much experienced at it. So please correct/comment on any mistake that I made while trying to put my point. I will also appreciate any comment/suggestion/criticism on my observation. 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
RFC: Updating Git for Windows' Code of Conduct
Hi all, especially active Git for Windows contributors, I would like to ask you for (constructive ;-)) feedback on https://github.com/git-for-windows/git/pull/661 I personally find it very important to keep it fun and pleasant to contribute to Git for Windows anf hope that the updated Code of Conduct will make that even easier than the current one. So if you could spare a moment and look over that Pull Request, that would be very much appreciated. Thanks, Johannes -- 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 submodule should honor "-c credential.helper" command line argument
On Thu, Feb 18, 2016 at 05:15:54PM -0800, Jacob Keller wrote: > I am looking at this more and I am stuck as to how best to provide a > test case. > > I think the problem as stated above is pretty straight forward, we > just want to stop clearing GIT_CONFIG_PARAMETERS but I can't find an > easy way to test that we've done the right thing. There are no current > tests for using a credential helper with submodule update right now. If you just want to test that GIT_CONFIG_PARAMETERS is left untouched in the submodule, you can tweak any config setting that would impact the newly-cloned repo. E.g. this: unset GIT_COMMITTER_NAME git config --global user.name='Global Name' git -c user.name='Command-Line Name' clone repo-with-module foo head -1 foo/.git/logs/HEAD head -1 foo/.git/modules/sub/logs/HEAD shows that the parent-level clone uses the "-c" name, but the submodule does not. That being said, I am not sure this is the right solution. In the thread I linked earlier[1], Jens indicated he would prefer not to blindly share config with the submodules, and I think I agree. Or are you proposing to pick and choose the keys in GIT_CONFIG_PARAMETERS, and whitelist credential.*? In that case, obviously my test example would not work, though I think that it might be fine to put "user.name" onto the whitelist (the things we really would worry about is stuff like "core.worktree" that clearly does not make sense to carry over into the submodule). -Peff [1] http://thread.gmane.org/gmane.comp.version-control.git/264840 -- 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: GSoC 2016: applications open, deadline = Fri, 19/2
On Fri, Feb 19, 2016 at 10:20 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> Probably too late now, anyway.. with David's multiple ref backend >> work, we could have a third, no-dependency backend. We can use index >> format to store refs. Then we can avoid case-sensitivity issue with >> filesystems. > > I'd actually vote for a ref backend that is based on a tree object ;-) > > http://thread.gmane.org/gmane.comp.version-control.git/282677 For reasonably small sets of refs I think index beats trees (remember we have cache-tree, which basically gives us the tree behind the scene), but when you have so many refs, hierarchical storage may be more efficient. Either way it's nice to see a builtin, no dependency backend besides "files". -- Duy -- 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: GSoC 2016: applications open, deadline = Fri, 19/2
Duy Nguyen writes: > Probably too late now, anyway.. with David's multiple ref backend > work, we could have a third, no-dependency backend. We can use index > format to store refs. Then we can avoid case-sensitivity issue with > filesystems. I'd actually vote for a ref backend that is based on a tree object ;-) http://thread.gmane.org/gmane.comp.version-control.git/282677 -- 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: [PATCHv12 0/7] Expose submodule parallelism to the user
Stefan Beller writes:
> On Thu, Feb 18, 2016 at 3:20 PM, Junio C Hamano wrote:
>> Stefan Beller writes:
>>
>>> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:
> Unless you count "I want to write differently from what was
> suggested" is a desirable thing to do, I do not see a point in
> favouring the above that uses an extra variable and skip_prefix()
> over what I gave you as "how about" patch. But whatever.
The skip_prefix was there before, so it stuck there.
>>
>> Sorry, but I thought this "parsing update strategy" was all new
>> code.
>
> I meant previous patches or in my mind. That's why I was hesitant to
> throw out the skip_prefix.
I actually think the attached on top of your final version would be
the best. It would not make too big a difference in this codepath
that skips just one byte, the pattern naturally would apply to
prefix of any length, and this would serve as the BCP, ready to be
copied-and-pasted by others when writing new code.
And of course it does not waste an otherwise unnecessary temporary
variable ;-)
diff --git a/submodule.c b/submodule.c
index 911fa3b..8e08159 100644
--- a/submodule.c
+++ b/submodule.c
@@ -223,9 +223,9 @@ int parse_submodule_update_strategy(const char *value,
dst->type = SM_UPDATE_REBASE;
else if (!strcmp(value, "merge"))
dst->type = SM_UPDATE_MERGE;
- else if (value[0] == '!') {
+ else if (skip_prefix(value, "!", &value)) {
dst->type = SM_UPDATE_COMMAND;
- dst->command = xstrdup(value + 1);
+ dst->command = xstrdup(value);
} else
return -1;
return 0;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GSoC 2016: applications open, deadline = Fri, 19/2
On Thu, Feb 18, 2016 at 1:58 AM, Matthieu Moy wrote: > Feel free to start writting an idea for > http://git.github.io/SoC-2016-Ideas/. It'd be nice to have a few more > ideas before Friday. We can polish them later if needed. Probably too late now, anyway.. with David's multiple ref backend work, we could have a third, no-dependency backend. We can use index format to store refs. Then we can avoid case-sensitivity issue with filesystems. Split-index could make it relatively cheap for updating refs. Later on, when we can store tree objects in index (*), some (rarely used) refs could be stored as tree objects and we can reduce index file size (and loading cost). This idea is inspired by Shawn's storing refs as tree objects mail, except that I stopped at "wait, if we want to create trees we (usually) have to go through index, why not just stop at index?". (*) In have some WIP in this area, but not ready for public discussion yet. And it's out of scope for GSoC. -- Duy -- 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 v5 25/27] refs: add LMDB refs storage backend
On Fri, Feb 19, 2016 at 3:23 AM, David Turner wrote:
>> > +static int read_per_worktree_ref(const char *submodule, const char
>> > *refname,
>> > +struct MDB_val *val, int
>> > *needs_free)
>>
>> From what I read, I suspect these _per_worktree functions will be
>> identical for the next backend. Should we just hand over the job for
>> files backend? For all entry points that may deal with per-worktree
>> refs, e.g. lmdb_resolve_ref_unsafe, can we check ref_type() first
>> thing, if it's per-worktree we call
>> refs_be_files.resolve_ref_unsafe()
>> instead? It could even be done at frontend level,
>> e.g. refs.c:resolve_ref_unsafe().
>>
>> Though I may be talking rubbish here because I don't know how whether
>> it has anything to do with transactions.
>
> The reason I did it this way is that some ref chains cross backend
> boundaries (e.g. HEAD -> refs/heads/master). But if we have other
> backends later, we could generalize.
Crossing backends should go through frontend again, imo. But I don't
really know if it's efficient.
>> > +static int lmdb_create_symref(const char *ref_target,
>> > + const char *refs_heads_master,
>> > + const char *logmsg)
>> > +{
>> > +
>> ...
>> > + mdb_put_or_die(&transaction, &key, &val, 0);
>> > +
>> > + /* TODO: Don't create ref d/f conflicts */
>>
>> I'm not sure I get this comment. D/F conflicts are no longer a thing
>> for lmdb backend, right?
>
> I'm trying to avoid the lmdb backend creating a set of refs that the
> files backend can't handle. This would make collaboration with other
> versions of git more difficult.
It already is. If you create refs "foo" and "FOO" on case sensitive
file system and clone it on a case-insensitive one, you face the same
problem. We may have an optional configuration knob to prevent
incompatibilities with files backend, but I think that should be done
(and enforced if necessary) outside backends.
--
Duy
--
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 21/25] fetch: define shallow boundary with --shallow-exclude
On Mon, Feb 15, 2016 at 3:15 AM, Duy Nguyen wrote:
> On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote:
>> Yes, dropping 'const' was implied. I didn't examine it too deeply, but
>> it did not appear as if there would be any major fallout from dropping
>> 'const'. It feels a bit cleaner to keep it all self-contained than to
>> have that somewhat oddball static string_list*, but it's not such a
>> big deal that I'd insist upon a rewrite.
>
> Dropping 'const' is not a big deal. But before we do that, how about
> this instead? I think the code looks better, and the compiler can
> still catch invalid updates to deepen_not.
I like this better, too.
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 0402e27..07570be 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char
> *prefix)
> struct child_process *conn;
> struct fetch_pack_args args;
> struct sha1_array shallow = SHA1_ARRAY_INIT;
> + struct string_list deepen_not = STRING_LIST_INIT_DUP;
>
> packet_trace_identity("fetch-pack");
>
> @@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const
> char *prefix)
> args.deepen_since = xstrdup(arg);
> continue;
> }
> + if (skip_prefix(arg, "--shallow-exclude=", &arg)) {
> + string_list_append(&deepen_not, arg);
> + continue;
> + }
> if (!strcmp("--no-progress", arg)) {
> args.no_progress = 1;
> continue;
> @@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const
> char *prefix)
> }
> usage(fetch_pack_usage);
> }
> + if (deepen_not.nr)
> + args.deepen_not = &deepen_not;
>
> if (i < argc)
> dest = argv[i++];
> --
> Duy
--
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 submodule should honor "-c credential.helper" command line argument
On Sun, Feb 7, 2016 at 7:44 PM, Jacob Keller wrote: > On Sun, Feb 7, 2016 at 5:48 AM, Marc Strapetz > wrote: >> On 07.02.2016 05:41, Jacob Keller wrote: >>> >>> On Wed, Feb 3, 2016 at 3:44 PM, Jacob Keller >>> wrote: Ok so I am not sure we even really need to use "-c" option in git-clone considering that we can just use the same flow we do for setting core.worktree values. I'll propose a patch with you two Cc'ed, which I think fixes the issue. There may actually be a set of configuration we want to include though, and the main issue I see is that it won't get updated correctly whenever the parent configuration changes. Thanks, Jake >>> >>> >>> I tried adding the config as part of module_clone in >>> submodule--helper.c but it didn't pass the test I added. I haven't had >>> time to look at this in the last few days, but I am stuck as to why >>> submodule--helper.c appeared to not use module_clone as I thought. >> >> >> I've tried to just comment out clearing of environment variables in >> git-sh-setup.sh, clear_local_git_env(). I've noticed that "-c >> credentials-helper ..." is stored in $GIT_CONFIG_PARAMETERS and with >> existing code is reset there. If not clearing the environment variables, at >> least "git submodule init" is working properly. I didn't try with other >> commands nor to run tests. >> >> -Marc >> >> > > I'll have to dig more into this next week. > > Regards, > Jake I am looking at this more and I am stuck as to how best to provide a test case. I think the problem as stated above is pretty straight forward, we just want to stop clearing GIT_CONFIG_PARAMETERS but I can't find an easy way to test that we've done the right thing. There are no current tests for using a credential helper with submodule update right now. Regards, Jake -- 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
[PATCHv13 7/7] clone: allow an explicit argument for parallel submodule clones
Just pass it along to "git submodule update", which may pick reasonable
defaults if you don't specify an explicit number.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/git-clone.txt | 6 +-
builtin/clone.c | 19 +--
t/t7406-submodule-update.sh | 15 +++
3 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6bf000d..6db7b6d 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
[-o ] [-b ] [-u ] [--reference ]
[--dissociate] [--separate-git-dir ]
[--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--]
+ [--recursive | --recurse-submodules] [--jobs ] [--]
[]
DESCRIPTION
@@ -221,6 +221,10 @@ objects from the source repository into a pack in the
cloned repository.
The result is Git repository can be separated from working
tree.
+-j ::
+--jobs ::
+ The number of submodules fetched at the same time.
+ Defaults to the `submodule.fetchJobs` option.
::
The (possibly remote) repository to clone from. See the
diff --git a/builtin/clone.c b/builtin/clone.c
index a0b3cd9..b004fb4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_progress = -1;
static struct string_list option_config;
static struct string_list option_reference;
static int option_dissociate;
+static int max_jobs = -1;
static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
N_("initialize submodules in the clone")),
OPT_BOOL(0, "recurse-submodules", &option_recursive,
N_("initialize submodules in the clone")),
+ OPT_INTEGER('j', "jobs", &max_jobs,
+ N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", &option_template, N_("template-directory"),
N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
OPT_END()
};
-static const char *argv_submodule[] = {
- "submodule", "update", "--init", "--recursive", NULL
-};
-
static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
{
static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -724,8 +723,16 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
sha1_to_hex(sha1), "1", NULL);
- if (!err && option_recursive)
- err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+ if (!err && option_recursive) {
+ struct argv_array args = ARGV_ARRAY_INIT;
+ argv_array_pushl(&args, "submodule", "update", "--init",
"--recursive", NULL);
+
+ if (max_jobs != -1)
+ argv_array_pushf(&args, "--jobs=%d", max_jobs);
+
+ err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+ argv_array_clear(&args);
+ }
return err;
}
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7fd5142..090891e 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in
parallel' '
grep "9 tasks" trace.out
)
'
+
+test_expect_success 'git clone passes the parallel jobs config on to
submodules' '
+ test_when_finished "rm -rf super4" &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 .
super4 &&
+ grep "7 tasks" trace.out &&
+ rm -rf super4 &&
+ git config --global submodule.fetchJobs 8 &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+ grep "8 tasks" trace.out &&
+ rm -rf super4 &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 .
super4 &&
+ grep "9 tasks" trace.out &&
+ rm -rf super4
+'
+
test_done
--
2.7.0.rc0.34.g65aed89
--
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
[PATCHv13 2/7] submodule-config: drop check against NULL
Adhere to the common coding style of Git and not check explicitly
for NULL throughout the file. There are still other occurrences in the
code base but that is usually inside of conditions with side effects.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule-config.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index a5cd2ee..9fa2269 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -267,7 +267,7 @@ static int parse_config(const char *var, const char *value,
void *data)
if (!strcmp(item.buf, "path")) {
if (!value)
ret = config_error_nonbool(var);
- else if (!me->overwrite && submodule->path != NULL)
+ else if (!me->overwrite && submodule->path)
warn_multiple_config(me->commit_sha1, submodule->name,
"path");
else {
@@ -291,7 +291,7 @@ static int parse_config(const char *var, const char *value,
void *data)
} else if (!strcmp(item.buf, "ignore")) {
if (!value)
ret = config_error_nonbool(var);
- else if (!me->overwrite && submodule->ignore != NULL)
+ else if (!me->overwrite && submodule->ignore)
warn_multiple_config(me->commit_sha1, submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
@@ -307,7 +307,7 @@ static int parse_config(const char *var, const char *value,
void *data)
} else if (!strcmp(item.buf, "url")) {
if (!value) {
ret = config_error_nonbool(var);
- } else if (!me->overwrite && submodule->url != NULL) {
+ } else if (!me->overwrite && submodule->url) {
warn_multiple_config(me->commit_sha1, submodule->name,
"url");
} else {
--
2.7.0.rc0.34.g65aed89
--
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
[PATCHv13 1/7] submodule-config: keep update strategy around
Currently submodule..update is only handled by git-submodule.sh.
C code will start to need to make use of that value as more of the
functionality of git-submodule.sh moves into library code in C.
Add the update field to 'struct submodule' and populate it so it can
be read as sm->update or from sm->update_command.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule-config.c | 13 +
submodule-config.h | 2 ++
submodule.c| 21 +
submodule.h| 16
4 files changed, 52 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..a5cd2ee 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -59,6 +59,7 @@ static void free_one_config(struct submodule_entry *entry)
{
free((void *) entry->config->path);
free((void *) entry->config->name);
+ free((void *) entry->config->update_strategy.command);
free(entry->config);
}
@@ -194,6 +195,8 @@ static struct submodule *lookup_or_create_by_name(struct
submodule_cache *cache,
submodule->path = NULL;
submodule->url = NULL;
+ submodule->update_strategy.type = SM_UPDATE_UNSPECIFIED;
+ submodule->update_strategy.command = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
@@ -311,6 +314,16 @@ static int parse_config(const char *var, const char
*value, void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
+ } else if (!strcmp(item.buf, "update")) {
+ if (!value)
+ ret = config_error_nonbool(var);
+ else if (!me->overwrite &&
+submodule->update_strategy.type !=
SM_UPDATE_UNSPECIFIED)
+ warn_multiple_config(me->commit_sha1, submodule->name,
+"update");
+ else if (parse_submodule_update_strategy(value,
+&submodule->update_strategy) < 0)
+ die(_("invalid value for %s"), var);
}
strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..092ebfc 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -2,6 +2,7 @@
#define SUBMODULE_CONFIG_CACHE_H
#include "hashmap.h"
+#include "submodule.h"
#include "strbuf.h"
/*
@@ -14,6 +15,7 @@ struct submodule {
const char *url;
int fetch_recurse;
const char *ignore;
+ struct submodule_update_strategy update_strategy;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
};
diff --git a/submodule.c b/submodule.c
index b83939c..2891aad 100644
--- a/submodule.c
+++ b/submodule.c
@@ -210,6 +210,27 @@ void gitmodules_config(void)
}
}
+int parse_submodule_update_strategy(const char *value,
+ struct submodule_update_strategy *dst)
+{
+ free((void*)dst->command);
+ dst->command = NULL;
+ if (!strcmp(value, "none"))
+ dst->type = SM_UPDATE_NONE;
+ else if (!strcmp(value, "checkout"))
+ dst->type = SM_UPDATE_CHECKOUT;
+ else if (!strcmp(value, "rebase"))
+ dst->type = SM_UPDATE_REBASE;
+ else if (!strcmp(value, "merge"))
+ dst->type = SM_UPDATE_MERGE;
+ else if (value[0] == '!') {
+ dst->type = SM_UPDATE_COMMAND;
+ dst->command = xstrdup(value + 1);
+ } else
+ return -1;
+ return 0;
+}
+
void handle_ignore_submodules_arg(struct diff_options *diffopt,
const char *arg)
{
diff --git a/submodule.h b/submodule.h
index cbc0003..3464500 100644
--- a/submodule.h
+++ b/submodule.h
@@ -13,6 +13,20 @@ enum {
RECURSE_SUBMODULES_ON = 2
};
+enum submodule_update_type {
+ SM_UPDATE_UNSPECIFIED = 0,
+ SM_UPDATE_CHECKOUT,
+ SM_UPDATE_REBASE,
+ SM_UPDATE_MERGE,
+ SM_UPDATE_NONE,
+ SM_UPDATE_COMMAND
+};
+
+struct submodule_update_strategy {
+ enum submodule_update_type type;
+ const char *command;
+};
+
int is_staging_gitmodules_ok(void);
int update_path_in_gitmodules(const char *oldpath, const char *newpath);
int remove_path_from_gitmodules(const char *path);
@@ -21,6 +35,8 @@ void set_diffopt_flags_from_submodule_config(struct
diff_options *diffopt,
const char *path);
int submodule_config(const char *var, const char *value, void *cb);
void gitmodules_config(void);
+int parse_submodule_update_strategy(const char *value,
+ struct submodule_update_strategy *dst);
void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
--
2.7.0.rc0.34.g65aed89
--
To unsubscribe from this list: send
[PATCHv13 0/7] Expose submodule parallelism to the user
Thanks Junio, for the discussion about the last issue!
I changed to check for '!' in the code as well as addressing the
cleanup afterwards.
Thanks,
Stefan
Interdiff to v11(! not v12)
diff --git a/submodule-config.c b/submodule-config.c
index 02bcaa7..9fa2269 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -59,6 +59,7 @@ static void free_one_config(struct submodule_entry *entry)
{
free((void *) entry->config->path);
free((void *) entry->config->name);
+ free((void *) entry->config->update_strategy.command);
free(entry->config);
}
diff --git a/submodule.c b/submodule.c
index 263cb2a..c1211d7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -219,6 +219,7 @@ void gitmodules_config(void)
int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst)
{
+ free((void*)dst->command);
dst->command = NULL;
if (!strcmp(value, "none"))
dst->type = SM_UPDATE_NONE;
@@ -228,9 +229,10 @@ int parse_submodule_update_strategy(const char *value,
dst->type = SM_UPDATE_REBASE;
else if (!strcmp(value, "merge"))
dst->type = SM_UPDATE_MERGE;
- else if (skip_prefix(value, "!", &dst->command))
+ else if (value[0] == '!') {
dst->type = SM_UPDATE_COMMAND;
- else
+ dst->command = xstrdup(value + 1);
+ } else
return -1;
return 0;
}
Stefan Beller (7):
submodule-config: keep update strategy around
submodule-config: drop check against NULL
fetching submodules: respect `submodule.fetchJobs` config option
submodule update: direct error message to stderr
git submodule update: have a dedicated helper for cloning
submodule update: expose parallelism to the user
clone: allow an explicit argument for parallel submodule clones
Documentation/config.txt| 6 +
Documentation/git-clone.txt | 6 +-
Documentation/git-submodule.txt | 7 +-
builtin/clone.c | 19 +++-
builtin/fetch.c | 2 +-
builtin/submodule--helper.c | 239
git-submodule.sh| 54 -
submodule-config.c | 19 +++-
submodule-config.h | 2 +
submodule.c | 37 ++-
submodule.h | 18 +++
t/t5526-fetch-submodules.sh | 14 +++
t/t7400-submodule-basic.sh | 4 +-
t/t7406-submodule-update.sh | 27 +
14 files changed, 405 insertions(+), 49 deletions(-)
--
2.7.0.rc0.34.g65aed89
--
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
[PATCHv13 3/7] fetching submodules: respect `submodule.fetchJobs` config option
This allows to configure fetching and updating in parallel
without having the command line option.
This moved the responsibility to determine how many parallel processes
to start from builtin/fetch to submodule.c as we need a way to communicate
"The user did not specify the number of parallel processes in the command
line options" in the builtin fetch. The submodule code takes care of
the precedence (CLI > config > default).
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/config.txt| 6 ++
builtin/fetch.c | 2 +-
submodule.c | 16 +++-
submodule.h | 2 ++
t/t5526-fetch-submodules.sh | 14 ++
5 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2d06b11..3b02732 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2646,6 +2646,12 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
+submodule.fetchJobs::
+ Specifies how many submodules are fetched/cloned at the same time.
+ A positive integer allows up to that number of submodules fetched
+ in parallel. A value of 0 will give some reasonable default.
+ If unset, it defaults to 1.
+
tag.sort::
This variable controls the sort ordering of tags when displayed by
linkgit:git-tag[1]. Without the "--sort=" option provided, the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 586840d..5aa1c2d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
static int all, append, dry_run, force, keep, multiple, update_head_ok,
verbosity;
static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static int tags = TAGS_DEFAULT, unshallow, update_shallow;
-static int max_children = 1;
+static int max_children = -1;
static const char *depth;
static const char *upload_pack;
static struct strbuf default_rla = STRBUF_INIT;
diff --git a/submodule.c b/submodule.c
index 2891aad..c1211d7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -15,6 +15,7 @@
#include "thread-utils.h"
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
+static int parallel_jobs = 1;
static struct string_list changed_submodule_paths;
static int initialized_fetch_ref_tips;
static struct sha1_array ref_tips_before_fetch;
@@ -169,7 +170,12 @@ void set_diffopt_flags_from_submodule_config(struct
diff_options *diffopt,
int submodule_config(const char *var, const char *value, void *cb)
{
- if (starts_with(var, "submodule."))
+ if (!strcmp(var, "submodule.fetchjobs")) {
+ parallel_jobs = git_config_int(var, value);
+ if (parallel_jobs < 0)
+ die(_("negative values not allowed for
submodule.fetchJobs"));
+ return 0;
+ } else if (starts_with(var, "submodule."))
return parse_submodule_config_option(var, value);
else if (!strcmp(var, "fetch.recursesubmodules")) {
config_fetch_recurse_submodules =
parse_fetch_recurse_submodules_arg(var, value);
@@ -772,6 +778,9 @@ int fetch_populated_submodules(const struct argv_array
*options,
argv_array_push(&spf.args, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
+ if (max_parallel_jobs < 0)
+ max_parallel_jobs = parallel_jobs;
+
calculate_changed_submodule_paths();
run_processes_parallel(max_parallel_jobs,
get_next_submodule,
@@ -1118,3 +1127,8 @@ void connect_work_tree_and_git_dir(const char *work_tree,
const char *git_dir)
strbuf_release(&rel_path);
free((void *)real_work_tree);
}
+
+int parallel_submodules(void)
+{
+ return parallel_jobs;
+}
diff --git a/submodule.h b/submodule.h
index 3464500..3166608 100644
--- a/submodule.h
+++ b/submodule.h
@@ -26,6 +26,7 @@ struct submodule_update_strategy {
enum submodule_update_type type;
const char *command;
};
+#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
int is_staging_gitmodules_ok(void);
int update_path_in_gitmodules(const char *oldpath, const char *newpath);
@@ -57,5 +58,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_nam
struct string_list *needs_pushing);
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);
+int parallel_submodules(void);
#endif
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1241146..954d0e4 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -471,4 +471,18 @@ test_expect_success "don't fetch submodule when newly
recorded commits are alre
[PATCHv13 4/7] submodule update: direct error message to stderr
Reroute the error message for specified but initialized submodules to stderr instead of stdout. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- git-submodule.sh | 4 ++-- t/t7400-submodule-basic.sh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 9bc5c5f..9ee86d4 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -693,7 +693,7 @@ cmd_update() if test "$update_module" = "none" then - echo "Skipping submodule '$displaypath'" + echo >&2 "Skipping submodule '$displaypath'" continue fi @@ -702,7 +702,7 @@ cmd_update() # Only mention uninitialized submodules when its # path have been specified test "$#" != "0" && - say "$(eval_gettext "Submodule path '\$displaypath' not initialized + say >&2 "$(eval_gettext "Submodule path '\$displaypath' not initialized Maybe you want to use 'update --init'?")" continue fi diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 540771c..5991e3c 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -462,7 +462,7 @@ test_expect_success 'update --init' ' git config --remove-section submodule.example && test_must_fail git config submodule.example.url && - git submodule update init > update.out && + git submodule update init 2> update.out && cat update.out && test_i18ngrep "not initialized" update.out && test_must_fail git rev-parse --resolve-git-dir init/.git && @@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' ' mkdir -p sub && ( cd sub && - git submodule update ../init >update.out && + git submodule update ../init 2>update.out && cat update.out && test_i18ngrep "not initialized" update.out && test_must_fail git rev-parse --resolve-git-dir ../init/.git && -- 2.7.0.rc0.34.g65aed89 -- 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
[PATCHv13 6/7] submodule update: expose parallelism to the user
Expose possible parallelism either via the "--jobs" CLI parameter or
the "submodule.fetchJobs" setting.
By having the variable initialized to -1, we make sure 0 can be passed
into the parallel processing machine, which will then pick as many parallel
workers as there are CPUs.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/git-submodule.txt | 7 ++-
builtin/submodule--helper.c | 16
git-submodule.sh| 9 +
t/t7406-submodule-update.sh | 12
4 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..13adebf 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
'git submodule' [--quiet] deinit [-f|--force] [--] ...
'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--] [...]
+ [--depth ] [--recursive] [--jobs ] [--] [...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
[commit] [--] [...]
'git submodule' [--quiet] foreach [--recursive]
@@ -377,6 +377,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared`
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
+-j ::
+--jobs ::
+ This option is only valid for the update command.
+ Clone new submodules in parallel with as many jobs.
+ Defaults to the `submodule.fetchJobs` option.
...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7629a41..65bdc14 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -424,6 +424,7 @@ static int update_clone_task_finished(int result,
static int update_clone(int argc, const char **argv, const char *prefix)
{
const char *update = NULL;
+ int max_jobs = -1;
struct string_list_item *item;
struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
@@ -444,6 +445,8 @@ static int update_clone(int argc, const char **argv, const
char *prefix)
OPT_STRING(0, "depth", &pp.depth, "",
N_("Create a shallow clone truncated to the "
"specified number of revisions")),
+ OPT_INTEGER('j', "jobs", &max_jobs,
+ N_("parallel jobs")),
OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
OPT_END()
};
@@ -469,10 +472,15 @@ static int update_clone(int argc, const char **argv,
const char *prefix)
gitmodules_config();
/* Overlay the parsed .gitmodules file with .git/config */
git_config(submodule_config, NULL);
- run_processes_parallel(1, update_clone_get_next_task,
- update_clone_start_failure,
- update_clone_task_finished,
- &pp);
+
+ if (max_jobs < 0)
+ max_jobs = parallel_submodules();
+
+ run_processes_parallel(max_jobs,
+ update_clone_get_next_task,
+ update_clone_start_failure,
+ update_clone_task_finished,
+ &pp);
if (pp.print_unmatched) {
printf("#unmatched\n");
diff --git a/git-submodule.sh b/git-submodule.sh
index 9f554fb..10c5af9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -645,6 +645,14 @@ cmd_update()
--depth=*)
depth=$1
;;
+ -j|--jobs)
+ case "$2" in '') usage ;; esac
+ jobs="--jobs=$2"
+ shift
+ ;;
+ --jobs=*)
+ jobs=$1
+ ;;
--)
shift
break
@@ -670,6 +678,7 @@ cmd_update()
${update:+--update "$update"} \
${reference:+--reference "$reference"} \
${depth:+--depth "$depth"} \
+ ${jobs:+$jobs} \
"$@" | {
err=
while read mode sha1 stage just_cloned sm_path
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..7fd5142 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops
module name before recur
test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked
out" actual
)
'
+
+test_expect_success 'submodule update can be run in parallel' '
+ (cd super2 &&
+GIT_TRA
[PATCHv13 5/7] git submodule update: have a dedicated helper for cloning
This introduces a new helper function in git submodule--helper
which takes care of cloning all submodules, which we want to
parallelize eventually.
Some tests (such as empty URL, update_mode=none) are required in the
helper to make the decision for cloning. These checks have been
moved into the C function as well (no need to repeat them in the
shell script).
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
builtin/submodule--helper.c | 231
git-submodule.sh| 45 +++--
2 files changed, 242 insertions(+), 34 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..7629a41 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,236 @@ static int module_clone(int argc, const char **argv,
const char *prefix)
return 0;
}
+struct submodule_update_clone {
+ /* states */
+ int count;
+ int print_unmatched;
+ /* configuration */
+ int quiet;
+ const char *reference;
+ const char *depth;
+ const char *recursive_prefix;
+ const char *prefix;
+ struct module_list list;
+ struct string_list projectlines;
+ struct submodule_update_strategy update;
+ struct pathspec pathspec;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL,
MODULE_LIST_INIT, STRING_LIST_INIT_DUP, SUBMODULE_UPDATE_STRATEGY_INIT}
+
+static int update_clone_inspect_next_task(struct child_process *cp,
+ struct strbuf *err,
+ struct submodule_update_clone *pp,
+ void **pp_task_cb,
+ const struct cache_entry *ce)
+{
+ const struct submodule *sub = NULL;
+ struct strbuf displaypath_sb = STRBUF_INIT;
+ struct strbuf sb = STRBUF_INIT;
+ const char *displaypath = NULL;
+ char *url = NULL;
+ int needs_cloning = 0;
+
+ if (ce_stage(ce)) {
+ if (pp->recursive_prefix)
+ strbuf_addf(err, "Skipping unmerged submodule %s/%s\n",
+ pp->recursive_prefix, ce->name);
+ else
+ strbuf_addf(err, "Skipping unmerged submodule %s\n",
+ ce->name);
+ goto cleanup;
+ }
+
+ sub = submodule_from_path(null_sha1, ce->name);
+
+ if (pp->recursive_prefix)
+ displaypath = relative_path(pp->recursive_prefix,
+ ce->name, &displaypath_sb);
+ else
+ displaypath = ce->name;
+
+ if (pp->update.type == SM_UPDATE_NONE ||
+ (pp->update.type == SM_UPDATE_UNSPECIFIED &&
+sub->update_strategy.type == SM_UPDATE_NONE)) {
+ strbuf_addf(err, "Skipping submodule '%s'\n",
+ displaypath);
+ goto cleanup;
+ }
+
+ /*
+* Looking up the url in .git/config.
+* We must not fall back to .gitmodules as we only want
+* to process configured submodules.
+*/
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "submodule.%s.url", sub->name);
+ git_config_get_string(sb.buf, &url);
+ if (!url) {
+ /*
+* Only mention uninitialized submodules when its
+* path have been specified
+*/
+ if (pp->pathspec.nr)
+ strbuf_addf(err, _("Submodule path '%s' not
initialized\n"
+ "Maybe you want to use 'update --init'?"),
+ displaypath);
+ goto cleanup;
+ }
+
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/.git", ce->name);
+ needs_cloning = !file_exists(sb.buf);
+
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
+ sha1_to_hex(ce->sha1), ce_stage(ce),
+ needs_cloning, ce->name);
+ string_list_append(&pp->projectlines, sb.buf);
+
+ if (needs_cloning) {
+ cp->git_cmd = 1;
+ cp->no_stdin = 1;
+ cp->stdout_to_stderr = 1;
+ cp->err = -1;
+ argv_array_push(&cp->args, "submodule--helper");
+ argv_array_push(&cp->args, "clone");
+ if (pp->quiet)
+ argv_array_push(&cp->args, "--quiet");
+
+ if (pp->prefix)
+ argv_array_pushl(&cp->args, "--prefix", pp->prefix,
NULL);
+
+ argv_array_pushl(&cp->args, "--path", sub->path, NULL);
+ argv_array_pushl(&cp->args, "--name", sub->name, NULL);
+ argv_array_pushl(&cp->args, "--url", strdup(url), NULL);
+ if (pp->reference)
+ argv_array_push(&cp
Re: [PATCHv12 0/7] Expose submodule parallelism to the user
On Thu, Feb 18, 2016 at 3:20 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:
Unless you count "I want to write differently from what was
suggested" is a desirable thing to do, I do not see a point in
favouring the above that uses an extra variable and skip_prefix()
over what I gave you as "how about" patch. But whatever.
>>>
>>> The skip_prefix was there before, so it stuck there.
>
> Sorry, but I thought this "parsing update strategy" was all new
> code.
I meant previous patches or in my mind. That's why I was hesitant to
throw out the skip_prefix.
>
>>> Also it seems a bit more high level to me hence easier to read,
>>> (though I am biased). I'll use your suggestion.
>>
>> and it doesn't crash when passing in value == NULL.
>> (We don't do that currently, just a side observation)
>
> Hmph. If you pass str==NULL with prefix="!" to what we have below,
> I would think the first iteration would try to read from *str and do
> a bizarre thing.
>
> static inline int skip_prefix(const char *str, const char *prefix,
> const char **out)
> {
> do {
> if (!*prefix) {
> *out = str;
> return 1;
> }
> } while (*str++ == *prefix++);
> return 0;
> }
>
> Puzzled.
And there I was asserting properties about methods
without looking them up.
ok.
--
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: [PATCHv12 0/7] Expose submodule parallelism to the user
Stefan Beller writes:
> On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote:
>>> Unless you count "I want to write differently from what was
>>> suggested" is a desirable thing to do, I do not see a point in
>>> favouring the above that uses an extra variable and skip_prefix()
>>> over what I gave you as "how about" patch. But whatever.
>>
>> The skip_prefix was there before, so it stuck there.
Sorry, but I thought this "parsing update strategy" was all new
code.
>> Also it seems a bit more high level to me hence easier to read,
>> (though I am biased). I'll use your suggestion.
>
> and it doesn't crash when passing in value == NULL.
> (We don't do that currently, just a side observation)
Hmph. If you pass str==NULL with prefix="!" to what we have below,
I would think the first iteration would try to read from *str and do
a bizarre thing.
static inline int skip_prefix(const char *str, const char *prefix,
const char **out)
{
do {
if (!*prefix) {
*out = str;
return 1;
}
} while (*str++ == *prefix++);
return 0;
}
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 2/2] submodule: port init from shell to C
Junio C Hamano writes:
>> +strbuf_reset(&sb);
>> +strbuf_addf(&sb, "submodule.%s.url", sub->name);
>> +if (git_config_get_string(sb.buf, &url)) {
>> +url = xstrdup(sub->url);
>> +if (!url)
>> +die(_("No url found for submodule path '%s' in
>> .gitmodules"),
>> +displaypath);
>
> I am assuming that this corresponds to these lines in the original
> scripted version:
>
> url=$(git config -f .gitmodules submodule."$name".url)
> test -z "$url" &&
> die "$(eval_gettext "No url found for submodule path...
>
> but what makes git_config_get_string() to read from ".gitmodules"
> file? Doesn't it read from $GIT_DIR/config & ~/.gitconfig instead?
I am starting to suspect that reading of ".gitmodules" in the
context of "git submodule" command is a good use case for the
configset API. It wants to read variables from ".gitmodules" and
the regular configuration file, and cares about where they come
from, illustrated by this codepath. Read URL from .gitmodules, do
something to it, and update the regular configuration file. Read
Update from .gitmodules, do some verification, and selectively store
it in the regular configuration file. There may be cases where you
want to check if a variable is in the regular configuration file
(i.e. read from there), see its value in ".gitmodules", and
conditionally update the regular configuration file (i.e. write into
it). The configset API was designed to help implement this kind of
thing in a clean manner (i.e. initialize one, add ".gitmodules"
file, and then configset_get_* on it, without affecting what you
read and write with the regular config_get_*/config_set_* to the
regular configuration file).
--
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: [PATCHv12 0/7] Expose submodule parallelism to the user
On Thu, Feb 18, 2016 at 3:12 PM, Stefan Beller wrote: >> Unless you count "I want to write differently from what was >> suggested" is a desirable thing to do, I do not see a point in >> favouring the above that uses an extra variable and skip_prefix() >> over what I gave you as "how about" patch. But whatever. > > The skip_prefix was there before, so it stuck there. > Also it seems a bit more high level to me hence easier to read, > (though I am biased). I'll use your suggestion. and it doesn't crash when passing in value == NULL. (We don't do that currently, just a side observation) -- 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: [PATCHv12 0/7] Expose submodule parallelism to the user
On Thu, Feb 18, 2016 at 2:55 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> Thanks Junio for a review of v11!
>>
>> I addressed the memory issue with the interdiff (in patch 1/7) as follows:
>> Interdiff to v11:
>>
>> diff --git a/submodule.c b/submodule.c
>> index 263cb2a..45d0967 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -219,6 +219,9 @@ void gitmodules_config(void)
>> int parse_submodule_update_strategy(const char *value,
>> struct submodule_update_strategy *dst)
>> {
>> + const char *com;
>> +
>> + free((void*)dst->command);
>> dst->command = NULL;
>> if (!strcmp(value, "none"))
>> dst->type = SM_UPDATE_NONE;
>> @@ -228,9 +231,10 @@ int parse_submodule_update_strategy(const char *value,
>> dst->type = SM_UPDATE_REBASE;
>> else if (!strcmp(value, "merge"))
>> dst->type = SM_UPDATE_MERGE;
>> - else if (skip_prefix(value, "!", &dst->command))
>> + else if (skip_prefix(value, "!", &com)) {
>> dst->type = SM_UPDATE_COMMAND;
>> - else
>> + dst->command = xstrdup(com);
>> + } else
>> return -1;
>> return 0;
>> }
>
> Unless you count "I want to write differently from what was
> suggested" is a desirable thing to do, I do not see a point in
> favouring the above that uses an extra variable and skip_prefix()
> over what I gave you as "how about" patch. But whatever.
The skip_prefix was there before, so it stuck there.
Also it seems a bit more high level to me hence easier to read,
(though I am biased). I'll use your suggestion.
>
> - Is dst->command always initialized to a NULL (otherwise the
>unconditional upfront free() makes it unsafe)?
Yes, although just currently. It seems hard to maintain going forward as
the struct submodule_update_strategy is part of the struct submodule
(as defined in submodule.h) as well as the struct submodule_update_clone
(as defined in submodule--helper.c) and both places take care of initializing
it to null.
>
> - Is there a global free_something() or something_clear() function
>that are used to release the resource held by a structure that
>has submodule_update_strategy structure embedded in it? If so
>dst->command needs to be freed there as well.
Sure, I'll just reroll the series now.
>
> Thanks.
>
>> Stefan Beller (7):
>> submodule-config: keep update strategy around
>> submodule-config: drop check against NULL
>> fetching submodules: respect `submodule.fetchJobs` config option
>> submodule update: direct error message to stderr
>> git submodule update: have a dedicated helper for cloning
>> submodule update: expose parallelism to the user
>> clone: allow an explicit argument for parallel submodule clones
>>
>> Documentation/config.txt| 6 +
>> Documentation/git-clone.txt | 6 +-
>> Documentation/git-submodule.txt | 7 +-
>> builtin/clone.c | 19 +++-
>> builtin/fetch.c | 2 +-
>> builtin/submodule--helper.c | 239
>>
>> git-submodule.sh| 54 -
>> submodule-config.c | 18 ++-
>> submodule-config.h | 2 +
>> submodule.c | 39 ++-
>> submodule.h | 18 +++
>> t/t5526-fetch-submodules.sh | 14 +++
>> t/t7400-submodule-basic.sh | 4 +-
>> t/t7406-submodule-update.sh | 27 +
>> 14 files changed, 406 insertions(+), 49 deletions(-)
--
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-p4.py: Don't try to rebase on submit from bare repository
git-p4 can be successfully used from bare repository (which acts as a
bridge between Perforce repository and pure Git repositories). On submit
git-p4 performs unconditional rebase. Do rebase only on non-bare
repositories.
---
git-p4.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index c33dece..e00cd02 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2059,8 +2059,9 @@ class P4Submit(Command, P4UserMap):
sync.branch = self.branch
sync.run([])
-rebase = P4Rebase()
-rebase.rebase()
+if not gitConfigBool("core.bare"):
+rebase = P4Rebase()
+rebase.rebase()
else:
if len(applied) == 0:
--
2.7.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv12 0/7] Expose submodule parallelism to the user
Stefan Beller writes:
> Thanks Junio for a review of v11!
>
> I addressed the memory issue with the interdiff (in patch 1/7) as follows:
> Interdiff to v11:
>
> diff --git a/submodule.c b/submodule.c
> index 263cb2a..45d0967 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -219,6 +219,9 @@ void gitmodules_config(void)
> int parse_submodule_update_strategy(const char *value,
> struct submodule_update_strategy *dst)
> {
> + const char *com;
> +
> + free((void*)dst->command);
> dst->command = NULL;
> if (!strcmp(value, "none"))
> dst->type = SM_UPDATE_NONE;
> @@ -228,9 +231,10 @@ int parse_submodule_update_strategy(const char *value,
> dst->type = SM_UPDATE_REBASE;
> else if (!strcmp(value, "merge"))
> dst->type = SM_UPDATE_MERGE;
> - else if (skip_prefix(value, "!", &dst->command))
> + else if (skip_prefix(value, "!", &com)) {
> dst->type = SM_UPDATE_COMMAND;
> - else
> + dst->command = xstrdup(com);
> + } else
> return -1;
> return 0;
> }
Unless you count "I want to write differently from what was
suggested" is a desirable thing to do, I do not see a point in
favouring the above that uses an extra variable and skip_prefix()
over what I gave you as "how about" patch. But whatever.
- Is dst->command always initialized to a NULL (otherwise the
unconditional upfront free() makes it unsafe)?
- Is there a global free_something() or something_clear() function
that are used to release the resource held by a structure that
has submodule_update_strategy structure embedded in it? If so
dst->command needs to be freed there as well.
Thanks.
> Stefan Beller (7):
> submodule-config: keep update strategy around
> submodule-config: drop check against NULL
> fetching submodules: respect `submodule.fetchJobs` config option
> submodule update: direct error message to stderr
> git submodule update: have a dedicated helper for cloning
> submodule update: expose parallelism to the user
> clone: allow an explicit argument for parallel submodule clones
>
> Documentation/config.txt| 6 +
> Documentation/git-clone.txt | 6 +-
> Documentation/git-submodule.txt | 7 +-
> builtin/clone.c | 19 +++-
> builtin/fetch.c | 2 +-
> builtin/submodule--helper.c | 239
>
> git-submodule.sh| 54 -
> submodule-config.c | 18 ++-
> submodule-config.h | 2 +
> submodule.c | 39 ++-
> submodule.h | 18 +++
> t/t5526-fetch-submodules.sh | 14 +++
> t/t7400-submodule-basic.sh | 4 +-
> t/t7406-submodule-update.sh | 27 +
> 14 files changed, 406 insertions(+), 49 deletions(-)
--
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: [BUG] git-log: tracking deleted file in a repository with multiple "initial commit" histories
On Tue, Feb 16, 2016 at 05:29:42PM -0500, Jeff King wrote: > On Tue, Feb 16, 2016 at 01:24:29PM -0800, Brian Norris wrote: > > > On Tue, Feb 16, 2016 at 03:45:57PM -0500, Jeff King wrote: > > > See the section on History Simplification in git-log. But basically, > > > when you specify a pathspec, git does not traverse side branches that > > > had no effect on the given pathspec. > > > > Thanks for the pointer. Is this done primarily for performance reasons, > > or for UI simplicity (e.g., to avoid some kinds of double-counting)? > > Seems like it generates unintuitive behaviors, but if it's helping block > > other unintuitive behaviors, then maybe it can't be resolved easily. > > Both, I think. Try looking at "--full-history" and you will see that it > has a lot of cruft that is probably not that interesting. But I wasn't seeing the "cruft" at first, but now I see. It appears, BTW, that 'git log --full-history -- ' gives vastly different commits than 'git log --full-history --graph -- '. (The latter has a lot more "cruft" about unrelated merges.) That seems like a weird inconsistency... > simplifying further (e.g., with "--simplify-merges") doesn't tell much > of the whole story (or maybe it does; we do see the final deletion > there, which is the end state). git log --full-history --simplify-merges -- init/iptables.conf and git log --full-history --simplify-merges --graph --oneline -- init/iptables.conf give good results for me, showing every commit that actually modifies the file, AFAICT. > > FWIW, I quite often use git-log to look at the history of a deleted > > file. Seems like a pretty big hole if the default behavior is going to > > prune away the entire history of the file. > > It doesn't normally. That doesn't really change my statement. > What happened is that you had two parallel > histories, and the final state of the file could be explained by > following the simpler of the two histories (the one where it never > existed in the first place). I (sort of) understand what happened. But I disagree that it's a good default. It's obviously not what the user is asking for. > > > If you want to see the full history, you can with "--full-history" > > > (there are some other simplification possibilities, but I don't think > > > any of them are interesting for your particular case). > > > > --full-history gives me what I want (I'll admit, I didn't read through > > all the other "History Simplification" documentation). Can I make this > > the default somehow? > > No, but you can make an alias that includes it. Sure. Thanks for the help! Brian -- 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 12/20] log_ref_write_1(): inline function
Michael Haggerty writes: > Now log_ref_write_1() doesn't do anything, so inline it. Nice. -- 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 09/20] log_ref_setup(): pass the open file descriptor back to the caller
Michael Haggerty writes:
> This function will most often be called by log_ref_write_1(), which
> wants to append to the reflog file. In that case, it is silly to close
> the file only for the caller to reopen it immediately. So, in the case
> that the file was opened, pass the open file descriptor back to the
> caller.
Makes sense.
> } else {
> adjust_shared_perm(logfile->buf);
> - close(logfd);
> }
I briefly wondered if permission twiddling on an open fd may have
negative effect on certain platforms, but the original was doing so
already (even before step 08/20), so that shouldn't be an issue with
this change, either.
Nicely done.
--
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
[PATCHv12 6/7] submodule update: expose parallelism to the user
Expose possible parallelism either via the "--jobs" CLI parameter or
the "submodule.fetchJobs" setting.
By having the variable initialized to -1, we make sure 0 can be passed
into the parallel processing machine, which will then pick as many parallel
workers as there are CPUs.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/git-submodule.txt | 7 ++-
builtin/submodule--helper.c | 16
git-submodule.sh| 9 +
t/t7406-submodule-update.sh | 12
4 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1572f05..13adebf 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
'git submodule' [--quiet] deinit [-f|--force] [--] ...
'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--] [...]
+ [--depth ] [--recursive] [--jobs ] [--] [...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
[commit] [--] [...]
'git submodule' [--quiet] foreach [--recursive]
@@ -377,6 +377,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared`
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
+-j ::
+--jobs ::
+ This option is only valid for the update command.
+ Clone new submodules in parallel with as many jobs.
+ Defaults to the `submodule.fetchJobs` option.
...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7629a41..65bdc14 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -424,6 +424,7 @@ static int update_clone_task_finished(int result,
static int update_clone(int argc, const char **argv, const char *prefix)
{
const char *update = NULL;
+ int max_jobs = -1;
struct string_list_item *item;
struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
@@ -444,6 +445,8 @@ static int update_clone(int argc, const char **argv, const
char *prefix)
OPT_STRING(0, "depth", &pp.depth, "",
N_("Create a shallow clone truncated to the "
"specified number of revisions")),
+ OPT_INTEGER('j', "jobs", &max_jobs,
+ N_("parallel jobs")),
OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
OPT_END()
};
@@ -469,10 +472,15 @@ static int update_clone(int argc, const char **argv,
const char *prefix)
gitmodules_config();
/* Overlay the parsed .gitmodules file with .git/config */
git_config(submodule_config, NULL);
- run_processes_parallel(1, update_clone_get_next_task,
- update_clone_start_failure,
- update_clone_task_finished,
- &pp);
+
+ if (max_jobs < 0)
+ max_jobs = parallel_submodules();
+
+ run_processes_parallel(max_jobs,
+ update_clone_get_next_task,
+ update_clone_start_failure,
+ update_clone_task_finished,
+ &pp);
if (pp.print_unmatched) {
printf("#unmatched\n");
diff --git a/git-submodule.sh b/git-submodule.sh
index 9f554fb..10c5af9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -645,6 +645,14 @@ cmd_update()
--depth=*)
depth=$1
;;
+ -j|--jobs)
+ case "$2" in '') usage ;; esac
+ jobs="--jobs=$2"
+ shift
+ ;;
+ --jobs=*)
+ jobs=$1
+ ;;
--)
shift
break
@@ -670,6 +678,7 @@ cmd_update()
${update:+--update "$update"} \
${reference:+--reference "$reference"} \
${depth:+--depth "$depth"} \
+ ${jobs:+$jobs} \
"$@" | {
err=
while read mode sha1 stage just_cloned sm_path
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..7fd5142 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops
module name before recur
test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked
out" actual
)
'
+
+test_expect_success 'submodule update can be run in parallel' '
+ (cd super2 &&
+GIT_TRA
[PATCHv12 1/7] submodule-config: keep update strategy around
Currently submodule..update is only handled by git-submodule.sh.
C code will start to need to make use of that value as more of the
functionality of git-submodule.sh moves into library code in C.
Add the update field to 'struct submodule' and populate it so it can
be read as sm->update or from sm->update_command.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule-config.c | 12
submodule-config.h | 2 ++
submodule.c| 23 +++
submodule.h| 16
4 files changed, 53 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..f8d1be9 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -194,6 +194,8 @@ static struct submodule *lookup_or_create_by_name(struct
submodule_cache *cache,
submodule->path = NULL;
submodule->url = NULL;
+ submodule->update_strategy.type = SM_UPDATE_UNSPECIFIED;
+ submodule->update_strategy.command = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
@@ -311,6 +313,16 @@ static int parse_config(const char *var, const char
*value, void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
+ } else if (!strcmp(item.buf, "update")) {
+ if (!value)
+ ret = config_error_nonbool(var);
+ else if (!me->overwrite &&
+submodule->update_strategy.type !=
SM_UPDATE_UNSPECIFIED)
+ warn_multiple_config(me->commit_sha1, submodule->name,
+"update");
+ else if (parse_submodule_update_strategy(value,
+&submodule->update_strategy) < 0)
+ die(_("invalid value for %s"), var);
}
strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..092ebfc 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -2,6 +2,7 @@
#define SUBMODULE_CONFIG_CACHE_H
#include "hashmap.h"
+#include "submodule.h"
#include "strbuf.h"
/*
@@ -14,6 +15,7 @@ struct submodule {
const char *url;
int fetch_recurse;
const char *ignore;
+ struct submodule_update_strategy update_strategy;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
};
diff --git a/submodule.c b/submodule.c
index b83939c..1de465f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -210,6 +210,29 @@ void gitmodules_config(void)
}
}
+int parse_submodule_update_strategy(const char *value,
+ struct submodule_update_strategy *dst)
+{
+ const char *com;
+
+ free((void*)dst->command);
+ dst->command = NULL;
+ if (!strcmp(value, "none"))
+ dst->type = SM_UPDATE_NONE;
+ else if (!strcmp(value, "checkout"))
+ dst->type = SM_UPDATE_CHECKOUT;
+ else if (!strcmp(value, "rebase"))
+ dst->type = SM_UPDATE_REBASE;
+ else if (!strcmp(value, "merge"))
+ dst->type = SM_UPDATE_MERGE;
+ else if (skip_prefix(value, "!", &com)) {
+ dst->type = SM_UPDATE_COMMAND;
+ dst->command = xstrdup(com);
+ } else
+ return -1;
+ return 0;
+}
+
void handle_ignore_submodules_arg(struct diff_options *diffopt,
const char *arg)
{
diff --git a/submodule.h b/submodule.h
index cbc0003..3464500 100644
--- a/submodule.h
+++ b/submodule.h
@@ -13,6 +13,20 @@ enum {
RECURSE_SUBMODULES_ON = 2
};
+enum submodule_update_type {
+ SM_UPDATE_UNSPECIFIED = 0,
+ SM_UPDATE_CHECKOUT,
+ SM_UPDATE_REBASE,
+ SM_UPDATE_MERGE,
+ SM_UPDATE_NONE,
+ SM_UPDATE_COMMAND
+};
+
+struct submodule_update_strategy {
+ enum submodule_update_type type;
+ const char *command;
+};
+
int is_staging_gitmodules_ok(void);
int update_path_in_gitmodules(const char *oldpath, const char *newpath);
int remove_path_from_gitmodules(const char *path);
@@ -21,6 +35,8 @@ void set_diffopt_flags_from_submodule_config(struct
diff_options *diffopt,
const char *path);
int submodule_config(const char *var, const char *value, void *cb);
void gitmodules_config(void);
+int parse_submodule_update_strategy(const char *value,
+ struct submodule_update_strategy *dst);
void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
void show_submodule_summary(FILE *f, const char *path,
const char *line_prefix,
--
2.7.0.rc0.34.g65aed89
--
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
[PATCHv12 2/7] submodule-config: drop check against NULL
Adhere to the common coding style of Git and not check explicitly
for NULL throughout the file. There are still other occurrences in the
code base but that is usually inside of conditions with side effects.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule-config.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index f8d1be9..02bcaa7 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -266,7 +266,7 @@ static int parse_config(const char *var, const char *value,
void *data)
if (!strcmp(item.buf, "path")) {
if (!value)
ret = config_error_nonbool(var);
- else if (!me->overwrite && submodule->path != NULL)
+ else if (!me->overwrite && submodule->path)
warn_multiple_config(me->commit_sha1, submodule->name,
"path");
else {
@@ -290,7 +290,7 @@ static int parse_config(const char *var, const char *value,
void *data)
} else if (!strcmp(item.buf, "ignore")) {
if (!value)
ret = config_error_nonbool(var);
- else if (!me->overwrite && submodule->ignore != NULL)
+ else if (!me->overwrite && submodule->ignore)
warn_multiple_config(me->commit_sha1, submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
@@ -306,7 +306,7 @@ static int parse_config(const char *var, const char *value,
void *data)
} else if (!strcmp(item.buf, "url")) {
if (!value) {
ret = config_error_nonbool(var);
- } else if (!me->overwrite && submodule->url != NULL) {
+ } else if (!me->overwrite && submodule->url) {
warn_multiple_config(me->commit_sha1, submodule->name,
"url");
} else {
--
2.7.0.rc0.34.g65aed89
--
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
[PATCHv12 7/7] clone: allow an explicit argument for parallel submodule clones
Just pass it along to "git submodule update", which may pick reasonable
defaults if you don't specify an explicit number.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/git-clone.txt | 6 +-
builtin/clone.c | 19 +--
t/t7406-submodule-update.sh | 15 +++
3 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 6bf000d..6db7b6d 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
[-o ] [-b ] [-u ] [--reference ]
[--dissociate] [--separate-git-dir ]
[--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--]
+ [--recursive | --recurse-submodules] [--jobs ] [--]
[]
DESCRIPTION
@@ -221,6 +221,10 @@ objects from the source repository into a pack in the
cloned repository.
The result is Git repository can be separated from working
tree.
+-j ::
+--jobs ::
+ The number of submodules fetched at the same time.
+ Defaults to the `submodule.fetchJobs` option.
::
The (possibly remote) repository to clone from. See the
diff --git a/builtin/clone.c b/builtin/clone.c
index a0b3cd9..b004fb4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_progress = -1;
static struct string_list option_config;
static struct string_list option_reference;
static int option_dissociate;
+static int max_jobs = -1;
static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
N_("initialize submodules in the clone")),
OPT_BOOL(0, "recurse-submodules", &option_recursive,
N_("initialize submodules in the clone")),
+ OPT_INTEGER('j', "jobs", &max_jobs,
+ N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", &option_template, N_("template-directory"),
N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
OPT_END()
};
-static const char *argv_submodule[] = {
- "submodule", "update", "--init", "--recursive", NULL
-};
-
static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
{
static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -724,8 +723,16 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
sha1_to_hex(sha1), "1", NULL);
- if (!err && option_recursive)
- err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+ if (!err && option_recursive) {
+ struct argv_array args = ARGV_ARRAY_INIT;
+ argv_array_pushl(&args, "submodule", "update", "--init",
"--recursive", NULL);
+
+ if (max_jobs != -1)
+ argv_array_pushf(&args, "--jobs=%d", max_jobs);
+
+ err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+ argv_array_clear(&args);
+ }
return err;
}
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7fd5142..090891e 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in
parallel' '
grep "9 tasks" trace.out
)
'
+
+test_expect_success 'git clone passes the parallel jobs config on to
submodules' '
+ test_when_finished "rm -rf super4" &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 .
super4 &&
+ grep "7 tasks" trace.out &&
+ rm -rf super4 &&
+ git config --global submodule.fetchJobs 8 &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+ grep "8 tasks" trace.out &&
+ rm -rf super4 &&
+ GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 .
super4 &&
+ grep "9 tasks" trace.out &&
+ rm -rf super4
+'
+
test_done
--
2.7.0.rc0.34.g65aed89
--
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
[PATCHv12 5/7] git submodule update: have a dedicated helper for cloning
This introduces a new helper function in git submodule--helper
which takes care of cloning all submodules, which we want to
parallelize eventually.
Some tests (such as empty URL, update_mode=none) are required in the
helper to make the decision for cloning. These checks have been
moved into the C function as well (no need to repeat them in the
shell script).
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
builtin/submodule--helper.c | 231
git-submodule.sh| 45 +++--
2 files changed, 242 insertions(+), 34 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..7629a41 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,236 @@ static int module_clone(int argc, const char **argv,
const char *prefix)
return 0;
}
+struct submodule_update_clone {
+ /* states */
+ int count;
+ int print_unmatched;
+ /* configuration */
+ int quiet;
+ const char *reference;
+ const char *depth;
+ const char *recursive_prefix;
+ const char *prefix;
+ struct module_list list;
+ struct string_list projectlines;
+ struct submodule_update_strategy update;
+ struct pathspec pathspec;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL,
MODULE_LIST_INIT, STRING_LIST_INIT_DUP, SUBMODULE_UPDATE_STRATEGY_INIT}
+
+static int update_clone_inspect_next_task(struct child_process *cp,
+ struct strbuf *err,
+ struct submodule_update_clone *pp,
+ void **pp_task_cb,
+ const struct cache_entry *ce)
+{
+ const struct submodule *sub = NULL;
+ struct strbuf displaypath_sb = STRBUF_INIT;
+ struct strbuf sb = STRBUF_INIT;
+ const char *displaypath = NULL;
+ char *url = NULL;
+ int needs_cloning = 0;
+
+ if (ce_stage(ce)) {
+ if (pp->recursive_prefix)
+ strbuf_addf(err, "Skipping unmerged submodule %s/%s\n",
+ pp->recursive_prefix, ce->name);
+ else
+ strbuf_addf(err, "Skipping unmerged submodule %s\n",
+ ce->name);
+ goto cleanup;
+ }
+
+ sub = submodule_from_path(null_sha1, ce->name);
+
+ if (pp->recursive_prefix)
+ displaypath = relative_path(pp->recursive_prefix,
+ ce->name, &displaypath_sb);
+ else
+ displaypath = ce->name;
+
+ if (pp->update.type == SM_UPDATE_NONE ||
+ (pp->update.type == SM_UPDATE_UNSPECIFIED &&
+sub->update_strategy.type == SM_UPDATE_NONE)) {
+ strbuf_addf(err, "Skipping submodule '%s'\n",
+ displaypath);
+ goto cleanup;
+ }
+
+ /*
+* Looking up the url in .git/config.
+* We must not fall back to .gitmodules as we only want
+* to process configured submodules.
+*/
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "submodule.%s.url", sub->name);
+ git_config_get_string(sb.buf, &url);
+ if (!url) {
+ /*
+* Only mention uninitialized submodules when its
+* path have been specified
+*/
+ if (pp->pathspec.nr)
+ strbuf_addf(err, _("Submodule path '%s' not
initialized\n"
+ "Maybe you want to use 'update --init'?"),
+ displaypath);
+ goto cleanup;
+ }
+
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%s/.git", ce->name);
+ needs_cloning = !file_exists(sb.buf);
+
+ strbuf_reset(&sb);
+ strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
+ sha1_to_hex(ce->sha1), ce_stage(ce),
+ needs_cloning, ce->name);
+ string_list_append(&pp->projectlines, sb.buf);
+
+ if (needs_cloning) {
+ cp->git_cmd = 1;
+ cp->no_stdin = 1;
+ cp->stdout_to_stderr = 1;
+ cp->err = -1;
+ argv_array_push(&cp->args, "submodule--helper");
+ argv_array_push(&cp->args, "clone");
+ if (pp->quiet)
+ argv_array_push(&cp->args, "--quiet");
+
+ if (pp->prefix)
+ argv_array_pushl(&cp->args, "--prefix", pp->prefix,
NULL);
+
+ argv_array_pushl(&cp->args, "--path", sub->path, NULL);
+ argv_array_pushl(&cp->args, "--name", sub->name, NULL);
+ argv_array_pushl(&cp->args, "--url", strdup(url), NULL);
+ if (pp->reference)
+ argv_array_push(&cp
[PATCHv12 3/7] fetching submodules: respect `submodule.fetchJobs` config option
This allows to configure fetching and updating in parallel
without having the command line option.
This moved the responsibility to determine how many parallel processes
to start from builtin/fetch to submodule.c as we need a way to communicate
"The user did not specify the number of parallel processes in the command
line options" in the builtin fetch. The submodule code takes care of
the precedence (CLI > config > default).
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/config.txt| 6 ++
builtin/fetch.c | 2 +-
submodule.c | 16 +++-
submodule.h | 2 ++
t/t5526-fetch-submodules.sh | 14 ++
5 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2d06b11..3b02732 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2646,6 +2646,12 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
+submodule.fetchJobs::
+ Specifies how many submodules are fetched/cloned at the same time.
+ A positive integer allows up to that number of submodules fetched
+ in parallel. A value of 0 will give some reasonable default.
+ If unset, it defaults to 1.
+
tag.sort::
This variable controls the sort ordering of tags when displayed by
linkgit:git-tag[1]. Without the "--sort=" option provided, the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 586840d..5aa1c2d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
static int all, append, dry_run, force, keep, multiple, update_head_ok,
verbosity;
static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static int tags = TAGS_DEFAULT, unshallow, update_shallow;
-static int max_children = 1;
+static int max_children = -1;
static const char *depth;
static const char *upload_pack;
static struct strbuf default_rla = STRBUF_INIT;
diff --git a/submodule.c b/submodule.c
index 1de465f..45d0967 100644
--- a/submodule.c
+++ b/submodule.c
@@ -15,6 +15,7 @@
#include "thread-utils.h"
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
+static int parallel_jobs = 1;
static struct string_list changed_submodule_paths;
static int initialized_fetch_ref_tips;
static struct sha1_array ref_tips_before_fetch;
@@ -169,7 +170,12 @@ void set_diffopt_flags_from_submodule_config(struct
diff_options *diffopt,
int submodule_config(const char *var, const char *value, void *cb)
{
- if (starts_with(var, "submodule."))
+ if (!strcmp(var, "submodule.fetchjobs")) {
+ parallel_jobs = git_config_int(var, value);
+ if (parallel_jobs < 0)
+ die(_("negative values not allowed for
submodule.fetchJobs"));
+ return 0;
+ } else if (starts_with(var, "submodule."))
return parse_submodule_config_option(var, value);
else if (!strcmp(var, "fetch.recursesubmodules")) {
config_fetch_recurse_submodules =
parse_fetch_recurse_submodules_arg(var, value);
@@ -774,6 +780,9 @@ int fetch_populated_submodules(const struct argv_array
*options,
argv_array_push(&spf.args, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
+ if (max_parallel_jobs < 0)
+ max_parallel_jobs = parallel_jobs;
+
calculate_changed_submodule_paths();
run_processes_parallel(max_parallel_jobs,
get_next_submodule,
@@ -1120,3 +1129,8 @@ void connect_work_tree_and_git_dir(const char *work_tree,
const char *git_dir)
strbuf_release(&rel_path);
free((void *)real_work_tree);
}
+
+int parallel_submodules(void)
+{
+ return parallel_jobs;
+}
diff --git a/submodule.h b/submodule.h
index 3464500..3166608 100644
--- a/submodule.h
+++ b/submodule.h
@@ -26,6 +26,7 @@ struct submodule_update_strategy {
enum submodule_update_type type;
const char *command;
};
+#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
int is_staging_gitmodules_ok(void);
int update_path_in_gitmodules(const char *oldpath, const char *newpath);
@@ -57,5 +58,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
const char *remotes_nam
struct string_list *needs_pushing);
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);
+int parallel_submodules(void);
#endif
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1241146..954d0e4 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -471,4 +471,18 @@ test_expect_success "don't fetch submodule when newly
recorded commits are alre
[PATCHv12 4/7] submodule update: direct error message to stderr
Reroute the error message for specified but initialized submodules to stderr instead of stdout. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- git-submodule.sh | 4 ++-- t/t7400-submodule-basic.sh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 9bc5c5f..9ee86d4 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -693,7 +693,7 @@ cmd_update() if test "$update_module" = "none" then - echo "Skipping submodule '$displaypath'" + echo >&2 "Skipping submodule '$displaypath'" continue fi @@ -702,7 +702,7 @@ cmd_update() # Only mention uninitialized submodules when its # path have been specified test "$#" != "0" && - say "$(eval_gettext "Submodule path '\$displaypath' not initialized + say >&2 "$(eval_gettext "Submodule path '\$displaypath' not initialized Maybe you want to use 'update --init'?")" continue fi diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 540771c..5991e3c 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -462,7 +462,7 @@ test_expect_success 'update --init' ' git config --remove-section submodule.example && test_must_fail git config submodule.example.url && - git submodule update init > update.out && + git submodule update init 2> update.out && cat update.out && test_i18ngrep "not initialized" update.out && test_must_fail git rev-parse --resolve-git-dir init/.git && @@ -480,7 +480,7 @@ test_expect_success 'update --init from subdirectory' ' mkdir -p sub && ( cd sub && - git submodule update ../init >update.out && + git submodule update ../init 2>update.out && cat update.out && test_i18ngrep "not initialized" update.out && test_must_fail git rev-parse --resolve-git-dir ../init/.git && -- 2.7.0.rc0.34.g65aed89 -- 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
[PATCHv12 0/7] Expose submodule parallelism to the user
Thanks Junio for a review of v11!
I addressed the memory issue with the interdiff (in patch 1/7) as follows:
Interdiff to v11:
diff --git a/submodule.c b/submodule.c
index 263cb2a..45d0967 100644
--- a/submodule.c
+++ b/submodule.c
@@ -219,6 +219,9 @@ void gitmodules_config(void)
int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst)
{
+ const char *com;
+
+ free((void*)dst->command);
dst->command = NULL;
if (!strcmp(value, "none"))
dst->type = SM_UPDATE_NONE;
@@ -228,9 +231,10 @@ int parse_submodule_update_strategy(const char *value,
dst->type = SM_UPDATE_REBASE;
else if (!strcmp(value, "merge"))
dst->type = SM_UPDATE_MERGE;
- else if (skip_prefix(value, "!", &dst->command))
+ else if (skip_prefix(value, "!", &com)) {
dst->type = SM_UPDATE_COMMAND;
- else
+ dst->command = xstrdup(com);
+ } else
return -1;
return 0;
}
Stefan Beller (7):
submodule-config: keep update strategy around
submodule-config: drop check against NULL
fetching submodules: respect `submodule.fetchJobs` config option
submodule update: direct error message to stderr
git submodule update: have a dedicated helper for cloning
submodule update: expose parallelism to the user
clone: allow an explicit argument for parallel submodule clones
Documentation/config.txt| 6 +
Documentation/git-clone.txt | 6 +-
Documentation/git-submodule.txt | 7 +-
builtin/clone.c | 19 +++-
builtin/fetch.c | 2 +-
builtin/submodule--helper.c | 239
git-submodule.sh| 54 -
submodule-config.c | 18 ++-
submodule-config.h | 2 +
submodule.c | 39 ++-
submodule.h | 18 +++
t/t5526-fetch-submodules.sh | 14 +++
t/t7400-submodule-basic.sh | 4 +-
t/t7406-submodule-update.sh | 27 +
14 files changed, 406 insertions(+), 49 deletions(-)
--
2.7.0.rc0.34.g65aed89
--
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 08/20] log_ref_setup(): improve robustness against races
Michael Haggerty writes: > Change log_ref_setup() to use raceproof_create_file() to create the new > logfile. This makes it more robust against a race against another > process that might be trying to clean up empty directories while we are > trying to create a new logfile. > > This also means that it will only call create_leading_directories() if > open() fails, which should be a net win. Even in the cases where we are > willing to create a new logfile, it will usually be the case that the > logfile already exists, or if not then that the directory containing the > logfile already exists. In such cases, we will save some work that was > previously done unconditionally. Makes sense. -- 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 06/20] rename_tmp_log(): improve error reporting
Michael Haggerty writes:
> * Don't capitalize error strings
> * Report true paths of affected files
Obvious improvements. Good.
> Signed-off-by: Michael Haggerty
> ---
> Maybe these error strings should be internationalized? If so, there
> are a bunch of similar error strings in related functions that should
> be changed at the same time.
>
> refs/files-backend.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index e5f964c..4266da9 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2430,10 +2430,11 @@ static int rename_tmp_log(const char *newrefname)
> ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
> if (ret) {
> if (true_errno==EISDIR)
> - error("Directory not empty: %s", path);
> + error("directory not empty: %s", path);
> else
> - error("unable to move logfile "TMP_RENAMED_LOG" to
> logs/%s: %s",
> - newrefname, strerror(true_errno));
> + error("unable to move logfile %s to %s: %s",
> + git_path(TMP_RENAMED_LOG), path,
> + strerror(true_errno));
> }
>
> free(path);
--
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 00/22] add the FORMATPRINTF macro to declare the gcc function
Jeff King writes: > On Thu, Feb 11, 2016 at 09:59:21AM -0800, Junio C Hamano wrote: > >> Elia Pinto writes: >> >> > Add the FORMATPRINTF macro for declaring the gcc function attribute >> > 'format printf' >> > for code style consistency with similar macro that git already use for >> > other gcc >> > attributes. And use it where necessary. >> > >> > Elia Pinto (22): >> > git-compat-util.h: add the FORMATPRINTF macro >> >> Hmm. Given that we already have >> >> #ifndef __GNUC__ >> #ifndef __attribute__ >> #define __attribute__(x) >> #endif >> #endif >> >> in git-compat-util.h, it is really between: >> >> __attribute__((format (printf, 1, 2))) >> void advise(const char *advice, ...); >> >> __attribute__((format (printf,2,3))) >> extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...); >> >> and >> >> FORMATPRINTF(1,2) >> void advise(const char *advice, ...); >> >> FORMATPRINTF(2,3) >> extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...); >> >> >> Perhaps I am biased for staring at our source code for too long, but >> somehow the latter looks unnecessarily loud, spelled in all caps. >> >> I dunno. What does this really buy us? > > I had the same thought on reading this. I think the "similar macro" Elia > mentions is probably NORETURN. But in that case, we cannot rely on > __attribute__, because it is spelled so many different ways (e.g., > __declspec(noreturn)). > > This series would be helpful to us if there was a platform that > understood the format attribute, but needed to spell it differently > somehow. But short of that, I think it is a net negative. Yes, exactly. I've been carrying this in my tree for about a week, but I think this does not buy us very much. Let's drop it. -- 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
How to merge branches with git-svn without loosing svn-properties
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Hello, could you please give me some recommendations how to merge two branches in a git-svn repository, without loosing svn-properties? As the git-svn man-page says: > We ignore all SVN properties except svn:executable. Any unhandled > properties are logged to $GIT_DIR/svn//unhandled.log Therefore I expect the following: If branch A is merged by git into branch B any modified property (except svn:executable and svn:mergeinfo) is not merged by git automatically. If I know that in branch A some property was set (e.g. svn-eol-style or svn:keywords), than I could re-add the property manually by calling git svn propset But usually I do not know. In this case it would be nice to have some script which iterates over all files and compares the properties for each file in the 2 branches. Is there already such a tool? Greetings Juergen -BEGIN PGP SIGNATURE- Version: GnuPG v2 Comment: Using GnuPG with Icedove - http://www.enigmail.net/ iD8DBQFWxi/vYZbcPghIM9IRApUpAJ9TFKagQUPdSx3QQZn5Cygb40G2TwCgkrWz 7+28Llb0IAo97gfNTSYIdG4= =W2u9 -END PGP SIGNATURE- -- 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 v5 25/27] refs: add LMDB refs storage backend
David Turner writes: > On Thu, 2016-02-18 at 15:50 +0700, Duy Nguyen wrote: > > [snip] > > Thanks; applied the above Please. Your other messages did excessively quote parts of the message you are not responding to, but this will not tell anybody but you what "the above" refers to, not even to Duy if the message suggested more than one thing and you took only some but not all of them. >> This permission makes me wonder if we need adjust_shared_perm() here >> and some other places. > > So just add this after every mkdir? > > if (shared_repository) > adjust_shared_perm(db_path); That reads as if the caller is saying "if we are in a shared repository, tweak the permission bits to make it sharable." Rather, think of "adjust_shared_perm(path)" as a declaration that you know "path" is something that needs to be accessible by those who needs write access to the repository. The caller does not need "if (shared_repository)"; the callee knows to become no-op. -- 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 2/2] submodule: port init from shell to C
Stefan Beller writes:
> By having the `init` functionality in C, we can reference it easier
> from other parts in the code.
>
> Signed-off-by: Stefan Beller
> ---
> builtin/submodule--helper.c | 107
>
> git-submodule.sh| 39 +---
> submodule.c | 21 +
> submodule.h | 1 +
> 4 files changed, 130 insertions(+), 38 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d1e9118..30e623a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -214,6 +214,112 @@ static int resolve_relative_url_test(int argc, const
> char **argv, const char *pr
> return 0;
> }
>
> +static void init_submodule(const char *path, const char *prefix, int quiet)
> +{
> + const struct submodule *sub;
> + struct strbuf sb = STRBUF_INIT;
> + char *url = NULL;
> + const char *upd = NULL;
> + char *cwd = xgetcwd();
> + const char *displaypath = relative_path(cwd, prefix, &sb);
> +
> + /* Only loads from .gitmodules, no overlay with .git/config */
> + gitmodules_config();
This feeds submodule_config() function with the contents of
".gitmodules".
> + sub = submodule_from_path(null_sha1, path);
> +
> + /*
> + * Copy url setting when it is not set yet.
> + * To look up the url in .git/config, we must not fall back to
> + * .gitmodules, so look it up directly.
> + */
> + strbuf_reset(&sb);
> + strbuf_addf(&sb, "submodule.%s.url", sub->name);
> + if (git_config_get_string(sb.buf, &url)) {
> + url = xstrdup(sub->url);
> + if (!url)
> + die(_("No url found for submodule path '%s' in
> .gitmodules"),
> + displaypath);
I am assuming that this corresponds to these lines in the original
scripted version:
url=$(git config -f .gitmodules submodule."$name".url)
test -z "$url" &&
die "$(eval_gettext "No url found for submodule path...
but what makes git_config_get_string() to read from ".gitmodules"
file? Doesn't it read from $GIT_DIR/config & ~/.gitconfig instead?
> + /* Possibly a url relative to parent */
> + if (starts_with_dot_dot_slash(url) ||
> + starts_with_dot_slash(url)) {
> + char *remoteurl;
> + char *remote = get_default_remote();
> + struct strbuf remotesb = STRBUF_INIT;
> + strbuf_addf(&remotesb, "remote.%s.url", remote);
> + free(remote);
> +
> + if (git_config_get_string(remotesb.buf, &remoteurl))
> + /*
> + * The repository is its own
> + * authoritative upstream
> + */
> + remoteurl = xgetcwd();
> + url = relative_url(remoteurl, url, NULL);
> + strbuf_release(&remotesb);
> + free(remoteurl);
Does the code inside this block correspond to this single line in
the original?
url=$(git submodule--helper resolve-relative-url "$url") || exit
It seems to be doing quite a different thing, though.
> + }
> +
> + if (git_config_set(sb.buf, url))
> + die(_("Failed to register url for submodule path '%s'"),
> + displaypath);
> + if (!quiet)
> + printf(_("Submodule '%s' (%s) registered for path
> '%s'\n"),
> + sub->name, url, displaypath);
> + }
> +
> + /* Copy "update" setting when it is not set yet */
> + strbuf_reset(&sb);
> + strbuf_addf(&sb, "submodule.%s.update", sub->name);
> + if (git_config_get_string_const(sb.buf, &upd) &&
This part of the code is supposed to read from in-tree ".gitmodules"
and copy to $GIT_DIR/config (i.e. git_config_set() below), but
again, I am not sure what makes this read from ".gitmodules".
Puzzled.
> + sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
> + if (sub->update_strategy.type == SM_UPDATE_COMMAND) {
> + fprintf(stderr, _("warning: command update mode
> suggested for submodule '%s'\n"),
> + sub->name);
> + upd = "none";
> + } else
> + upd =
> submodule_strategy_to_string(&sub->update_strategy);
> +
> + if (git_config_set(sb.buf, upd))
> + die(_("Failed to register update mode for submodule
> path '%s'"), displaypath);
> + }
> + strbuf_release(&sb);
> + free(cwd);
> + free(url);
> +}
--
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://
Re: [PATCH 1/2] submodule: port resolve_relative_url from shell to C
Stefan Beller writes:
> +static int starts_with_dot_slash(const char *str)
> +{
> + return str[0] == '.' && is_dir_sep(str[1]);
> +}
> +
> +static int starts_with_dot_dot_slash(const char *str)
> +{
> + return str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);
> +}
> +
> +/*
> + * Returns 1 if it was the last chop before ':'.
> + */
> +static int chop_last_dir(char **remoteurl, int is_relative)
> +{
> + char *rfind = find_last_dir_sep(*remoteurl);
> + if (rfind) {
> + *rfind = '\0';
> + return 0;
> + }
> +
> + rfind = strrchr(*remoteurl, ':');
> + if (rfind) {
> + *rfind = '\0';
> + return 1;
> + }
> +
> + if (is_relative || !strcmp(".", *remoteurl))
> + die(_("cannot strip one component off url '%s'"),
> + *remoteurl);
> +
> + free(*remoteurl);
> + *remoteurl = xstrdup(".");
> + return 0;
> +}
> +
> +/*
> + * The `url` argument is the URL that navigates to the submodule origin
> + * repo. When relative, this URL is relative to the superproject origin
> + * URL repo. The `up_path` argument, if specified, is the relative
> + * path that navigates from the submodule working tree to the superproject
> + * working tree. Returns the origin URL of the submodule.
> + *
> + * Return either an absolute URL or filesystem path (if the superproject
> + * origin URL is an absolute URL or filesystem path, respectively) or a
> + * relative file system path (if the superproject origin URL is a relative
> + * file system path).
> + *
> + * When the output is a relative file system path, the path is either
> + * relative to the submodule working tree, if up_path is specified, or to
> + * the superproject working tree otherwise.
> + *
> + * NEEDSWORK: This works incorrectly on the domain and protocol part.
> + * remote_url url outcome correct
> + * http://a.com/b ../c http://a.com/c yes
> + * http://a.com/b ../../c http://c no (domain should be
> kept)
> + * http://a.com/b ../../../c http:/c no
> + * http://a.com/b ../../../../chttp:c no
> + * http://a.com/b ../../../../../c.:c no
> + */
Not just "no" but we should say what the expected outcome is. I
think all of them should error out?
Given how chop_last_dir() works, I think this function is broken
when a local part has a colon in its path component, too. I do not
think the scripted version did any better on such a URL; just
another thing that NEEDSWORK comment should list as a thing to be
fixed in the future.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 25/27] refs: add LMDB refs storage backend
On Thu, 2016-02-18 at 15:50 +0700, Duy Nguyen wrote:
[snip]
Thanks; applied the above
> This permission makes me wonder if we need adjust_shared_perm() here
> and some other places.
So just add this after every mkdir?
if (shared_repository)
adjust_shared_perm(db_path);
> > + if (ret)
> > + die("BUG: mdb_env_open (%s) failed: %s", path,
> > + mdb_strerror(ret));
> > +}
> > +
> > +static int lmdb_init_db(int shared, struct strbuf *err)
> > +{
> > + /*
> > +* To create a db, all we need to do is make a directory
> > for
> > +* it to live in; lmdb will do the rest.
> > +*/
> > +
> > + if (!db_path)
> > + db_path =
> > xstrdup(real_path(git_path("refs.lmdb")));
> > +
> > + if (mkdir(db_path, 0775) && errno != EEXIST) {
> > + strbuf_addf(err, "%s", strerror(errno));
>
> maybe strbuf_addstr, unless want to add something more, "mkdir
> failed"?
added more
> > +static int read_per_worktree_ref(const char *submodule, const char
> > *refname,
> > +struct MDB_val *val, int
> > *needs_free)
>
> From what I read, I suspect these _per_worktree functions will be
> identical for the next backend. Should we just hand over the job for
> files backend? For all entry points that may deal with per-worktree
> refs, e.g. lmdb_resolve_ref_unsafe, can we check ref_type() first
> thing, if it's per-worktree we call
> refs_be_files.resolve_ref_unsafe()
> instead? It could even be done at frontend level,
> e.g. refs.c:resolve_ref_unsafe().
>
> Though I may be talking rubbish here because I don't know how whether
> it has anything to do with transactions.
The reason I did it this way is that some ref chains cross backend
boundaries (e.g. HEAD -> refs/heads/master). But if we have other
backends later, we could generalize.
> > +{
> > + struct strbuf sb = STRBUF_INIT;
> > + struct strbuf path = STRBUF_INIT;
> > + struct stat st;
> > + int ret = -1;
> > +
> > + submodule_path(&path, submodule, refname);
> > +
> > +#ifndef NO_SYMLINK_HEAD
>
> It started with the compiler warns about unused "st" when this macro
> is defined. Which makes me wonder, should we do something like this
> to
> make sure this code compiles unconditionally?
>
> +#ifndef NO_SYMLINK_HEAD
> + int no_symlink_head = 0;
> +#else
> + int no_symlink_head = 1;
> +#endif
> ...
> + if (!no_symlink_head) {
> ...
OK.
> > +int lmdb_transaction_begin_flags(struct strbuf *err, unsigned int
> > flags)
>
> static?
yep
> > +static const char *parse_ref_data(struct lmdb_transaction
> > *transaction,
> > + const char *refname, const char
> > *ref_data,
> > + unsigned char *sha1, int
> > resolve_flags,
> > + int *flags, int bad_name)
> > +{
>[snip]
> This code looks a lot like near the end of resolve_ref_1(). Maybe we
> could share the code in refs/backend-common.c or something and call
> here instead?
When I wrote this, I couldn't find a straightforward way to factor out
the commonalities, but I'll try again now that I understand the refs
code better.
> > +static int show_one_reflog_ent(struct strbuf *sb,
> > each_reflog_ent_fn fn, void *cb_data)
> > +{
> > + unsigned char osha1[20], nsha1[20];
> > + char *email_end, *message;
> > + unsigned long timestamp;
> > + int tz;
> > +
> > + /* old (raw sha) new (raw sha) name SP time TAB
> > msg LF */
>
> Hmm.. since you're going with raw sha-1, this is clearly not a text
> string anymore, any reason to still keep LF at the end?
IIRC, some of the common funcs depend on this.
> > +static int lmdb_delete_reflog(const char *refname)
> > +{
> > + MDB_val key, val;
> > + char *log_path;
> > + int len;
> > + MDB_cursor *cursor;
> > + int ret = 0;
> > + int mdb_ret;
> > + struct strbuf err = STRBUF_INIT;
> > + int in_transaction;
> > +
> > + if (ref_type(refname) != REF_TYPE_NORMAL)
> > + return refs_be_files.delete_reflog(refname);
>
> Yay.. delegating work to files backend. I still think doing this in
> refs.c:delete_reflog() may be a good idea.
Yes, I agree.
> > +int lmdb_reflog_expire(const char *refname, const unsigned char
> > *sha1,
>
> static?
Yep.
> > +static int lmdb_create_symref(const char *ref_target,
> > + const char *refs_heads_master,
> > + const char *logmsg)
> > +{
> > +
> ...
> > + mdb_put_or_die(&transaction, &key, &val, 0);
> > +
> > + /* TODO: Don't create ref d/f conflicts */
>
> I'm not sure I get this comment. D/F conflicts are no longer a thing
> for lmdb backend, right?
I'm trying to avoid the lmdb backend creating a set of refs that the
files backend can't handle. This would make collaboration with other
versions of git more difficult.
> > +MDB_env *submodule_txn_begin(struct lmdb_transaction *transaction)
>
> static?
Yes.
> > +{
> > + int ret;
> > + MDB_env *submodul
Re: [PATCHv11 0/7] Expose submodule parallelism to the user
Stefan Beller writes: >> This replaces origin/sb/submodule-parallel-update >> and is based on origin/master >> >> * broke out the patch for redirecting errors to stderr in "submodule update" >> (Thanks Jonathan, Jacob) >> * use git_config_int and manually check for less than 0. >> (Thanks Junio) >> * use an enum consistently now for submodule update strategy >> (Thanks Junio) >> * fixed the funny indentation > > * removed the indirect call to submodule_config, but made it directly > (Thanks Jonathan) I just finished going through the series one more time, and other than one small "huh?", everything looked good to me. Let's seriously push this forward. 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
Re: [PATCHv11 1/7] submodule-config: keep update strategy around
Stefan Beller writes:
> @@ -340,6 +342,16 @@ static int parse_config(const char *var, const char
> *value, void *data)
> free((void *) submodule->url);
> submodule->url = xstrdup(value);
> }
> + } else if (!strcmp(item.buf, "update")) {
> + if (!value)
> + ret = config_error_nonbool(var);
> + else if (!me->overwrite &&
> + submodule->update_strategy.type !=
> SM_UPDATE_UNSPECIFIED)
> + warn_multiple_config(me->commit_sha1, submodule->name,
> + "update");
> + else if (parse_submodule_update_strategy(value,
> + &submodule->update_strategy) < 0)
> + die(_("invalid value for %s"), var);
Mental note. This takes "value" that comes from the config API; the
pre-context in this hunk treats it as a transient piece of memory
and uses xstrdup() on it before storing it away in submodule->url.
> +int parse_submodule_update_strategy(const char *value,
> + struct submodule_update_strategy *dst)
> +{
> + dst->command = NULL;
> + if (!strcmp(value, "none"))
> + dst->type = SM_UPDATE_NONE;
> + else if (!strcmp(value, "checkout"))
> + dst->type = SM_UPDATE_CHECKOUT;
> + else if (!strcmp(value, "rebase"))
> + dst->type = SM_UPDATE_REBASE;
> + else if (!strcmp(value, "merge"))
> + dst->type = SM_UPDATE_MERGE;
> + else if (skip_prefix(value, "!", &dst->command))
> + dst->type = SM_UPDATE_COMMAND;
This however uses skip_prefix() on value, making dst->command store
a pointer into that transient piece of memory.
That looks inconsistent, doesn't it? I think this part should be
else if (value[0] == '!') {
dst->type = SM_UPDATE_COMMAND;
dst->command = xstrdup(value + 1);
}
or something like that. I.e. dst->command should own the piece of
memory it points at, 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: GSoC 2016: applications open, deadline = Fri, 19/2
Stefan Beller writes: > On Thu, Feb 18, 2016 at 12:41 AM, Lars Schneider > wrote: >>> Feel free to start writting an idea for >>> http://git.github.io/SoC-2016-Ideas/. It'd be nice to have a few more >>> ideas before Friday. We can polish them later if needed. >> >> I published my ideas here: >> https://github.com/git/git.github.io/pull/125/files > > I like the idea of a beginner mode, but on the other hand that looks > inflexible to me ;) > (What if I want to use rebase, but not reset --hard?) That's simple. You say "cd .. && rm -fr repo && git clone" and start from scratch ;-). This whole "beginner should be limited to a 'safe' subset" is an unhealthy attitude. Deciding what the 'safe' subset is must be done with a lot of thinking by people who intimately know what implications it has to ban each feature. I do not think it would be a good fit for a project to give to a relatively new participant to the Git project. For example, I think banning "worktree" feature from newbies may not be a bad idea, as you can work on a project without using "worktree" at all, and use of "worktree" would only subject you to bugs that do not exist when you do not use that feature. The "shallow clone", "sparse checkout", and "untracked cache" fall into the same category for exactly the same reason. The "submodule" feature might fall into the same category for the same reason, but that is not something you as a project participant can unilaterally decide, as the project you are working on may have already decided to use the feature, so it is harder to ban from the beginners. But for the rest of really "core" part of Git, I do not think there is any such command that can be totally banned. We have these "powerful" tools for a reason. After making a mess experimenting with your working tree files, "reset --hard" is the best tool to go back to the known-good state, and robbing it from the users is not a sound approach to help them. When "powerful" becomes "too powerful" is when a "powerful" tool is misused. It is perhaps done by mistake or perhaps done by copying and pasting a solution from Interweb for a problem that does not match your situation without understanding what you are doing. What is needed to help beginners is to make the powerful tool harder to misuse. Of course, that would be a harder task, because you have to do a real thinking. You do not have to do any thinking to say that "a blanket ban that hides these powerful tools behind the beginner mode" helps beginners, but I do not think it is solving what really matters. At the same time, it just adds to the FUD, i.e. some commands are too powerful for their own good. -- 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: GSoC 2016: applications open, deadline = Fri, 19/2
On Thu, Feb 18, 2016 at 12:41 AM, Lars Schneider wrote: >> Feel free to start writting an idea for >> http://git.github.io/SoC-2016-Ideas/. It'd be nice to have a few more >> ideas before Friday. We can polish them later if needed. > > I published my ideas here: > https://github.com/git/git.github.io/pull/125/files I like the idea of a beginner mode, but on the other hand that looks inflexible to me ;) (What if I want to use rebase, but not reset --hard?) I am confused by the black white mode, did you switch allow and deny in there? > > Do you think that works as start or do we need more detailed, hands-on > instructions? > > Thanks, > Lars -- 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 14/21] refs: always handle non-normal refs in files backend
On Thu, 2016-02-18 at 13:07 +0100, Michael Haggerty wrote:
> On 02/18/2016 03:44 AM, David Turner wrote:
> > On Fri, 2016-02-12 at 16:07 +0100, Michael Haggerty wrote:
> > > On 02/05/2016 08:44 PM, David Turner wrote:
> > > > Always handle non-normal (per-worktree or pseudo) refs in the
> > > > files
> > > > backend instead of alternate backends.
> > > >
> > > > Sometimes a ref transaction will update both a per-worktree ref
> > > > and
> > > > a
> > > > normal ref. For instance, an ordinary commit might update
> > > > refs/heads/master and HEAD (or at least HEAD's reflog).
> > > >
> > > > Updates to normal refs continue to go through the chosen
> > > > backend.
> > > >
> > > > Updates to non-normal refs are moved to a separate files
> > > > backend
> > > > transaction.
> > > >
> > > > Signed-off-by: David Turner
> > > > ---
> > > > refs.c | 81
> > > > +++
> > > > +--
> > > > 1 file changed, 79 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/refs.c b/refs.c
> > > > index 227c018..18ba356 100644
> > > > --- a/refs.c
> > > > +++ b/refs.c
> > > > @@ -9,6 +9,11 @@
> > > > #include "object.h"
> > > > #include "tag.h"
> > > >
> > > > +static const char split_transaction_fail_warning[] = N_(
> > > > + "A ref transaction was split across two refs backends.
> > > > Part of the "
> > > > + "transaction succeeded, but then the update to the per
> > > > -worktree refs "
> > > > + "failed. Your repository may be in an inconsistent
> > > > state.");
> > > > +
> > > > /*
> > > > * We always have a files backend and it is the default.
> > > > */
> > > > @@ -791,6 +796,13 @@ void ref_transaction_free(struct
> > > > ref_transaction *transaction)
> > > > free(transaction);
> > > > }
> > > >
> > > > +static void add_update_obj(struct ref_transaction
> > > > *transaction,
> > > > + struct ref_update *update)
> > > > +{
> > > > + ALLOC_GROW(transaction->updates, transaction->nr + 1,
> > > > transaction->alloc);
> > > > + transaction->updates[transaction->nr++] = update;
> > > > +}
> > > > +
> > > > static struct ref_update *add_update(struct ref_transaction
> > > > *transaction,
> > > > const char *refname)
> > > > {
> > > > @@ -798,8 +810,7 @@ static struct ref_update *add_update(struct
> > > > ref_transaction *transaction,
> > > > struct ref_update *update = xcalloc(1, sizeof(*update)
> > > > +
> > > > len);
> > > >
> > > > memcpy((char *)update->refname, refname, len); /*
> > > > includes
> > > > NUL */
> > > > - ALLOC_GROW(transaction->updates, transaction->nr + 1,
> > > > transaction->alloc);
> > > > - transaction->updates[transaction->nr++] = update;
> > > > + add_update_obj(transaction, update);
> > > > return update;
> > > > }
> > > >
> > > > @@ -1217,11 +1228,38 @@ static int dereference_symrefs(struct
> > > > ref_transaction *transaction,
> > > > return 0;
> > > > }
> > > >
> > > > +/*
> > > > + * Move all non-normal ref updates into a specially-created
> > > > + * files-backend transaction
> > > > + */
> > > > +static int move_abnormal_ref_updates(struct ref_transaction
> > > > *transaction,
> > > > +struct ref_transaction
> > > > *files_transaction,
> > > > +struct strbuf *err)
> > > > +{
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < transaction->nr; i++) {
> > > > + int last;
> > > > + struct ref_update *update = transaction
> > > > ->updates[i];
> > > > +
> > > > + if (ref_type(update->refname) ==
> > > > REF_TYPE_NORMAL)
> > > > + continue;
> > > > +
> > > > + last = --transaction->nr;
> > > > + transaction->updates[i] = transaction
> > > > ->updates[last];
> > > > + add_update_obj(files_transaction, update);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > >
> > > I think this function is incorrect. The update that was
> > > previously at
> > > transaction->updates[i] never gets checked for abnormality.
> >
> > Yes it does; that's the "update" variable that we just checked.
>
> Sorry, I meant to say "the update that was previously at
> `transaction->updates[transaction->nr - 1]` never gets checked for
> abnormality". Because it gets moved to `transaction->updates[i]`,
> then
> `i` is incremented without checking it.
Oh, right.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Feb 2016, #05; Wed, 17)
Jeff King writes: > On Wed, Feb 17, 2016 at 02:34:08PM -0800, Junio C Hamano wrote: > >> * jk/tighten-alloc (2016-02-15) 18 commits >> ... > Please hold off a bit; I have a re-roll coming for this one. > >> * nd/dwim-wildcards-as-pathspecs (2016-02-10) 3 commits >> ... > Even if we drop it, I think the first two patches are an improvement. 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
Re, Helllo
Hello, I am Mr. John Smith , personal assistance to the former oil and gas resources minister. I wish to transact a business with you that will be of immense benefits to both of us. I have a deal worth US$75 Million. Kindly contact me for more information. Best Regards, John Smith, Jr -- 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 02/20] safe_create_leading_directories(): set errno on SCLD_EXISTS
On 02/17/2016 08:23 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> The exit path for SCLD_EXISTS wasn't setting errno, as expected by at
>> least one caller. Fix the problem and document that the function sets
>> errno correctly to help avoid similar regressions in the future.
>
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 568120e..a1ac646 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -135,8 +135,10 @@ enum scld_error safe_create_leading_directories(char
>> *path)
>> *slash = '\0';
>> if (!stat(path, &st)) {
>> /* path exists */
>> -if (!S_ISDIR(st.st_mode))
>> +if (!S_ISDIR(st.st_mode)) {
>> +errno = EEXIST;
>> ret = SCLD_EXISTS;
>
> Hmm, when does this trigger? There is a non-directory A/B, you are
> preparing leading directories to create A/B/C/D, and you find A/B
> exists but is not a directory so you cannot create A/B/C underneath
> it?
>
> That sounds more like ENOTDIR to me.
Yes, you're right. Thanks.
> Does the caller expect EEXIST, or both?
I don't know of any callers that branch based on errno, but several use
strerror() or die_errno() to report the error to the user. The few
callers that care about the reason for the failure base their behavior
on the return value.
But there are a lot of callers and I haven't audited each of them
carefully. Given that this error path currently doesn't necessarily set
errno *at all*, I think using a plausible value is strictly an improvement.
I'll change it to ENOTDIR in the next round.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
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 01/11] ref-filter: use string_list_split over strbuf_split
On Thu, Feb 18, 2016 at 3:49 AM, Junio C Hamano wrote: > Jeff King writes: > >> On Wed, Feb 17, 2016 at 05:11:50PM -0500, Eric Sunshine wrote: >> >>> On Wed, Feb 17, 2016 at 1:06 PM, Karthik Nayak >>> wrote: >>> > From: Jeff King >>> > >>> > We don't do any post-processing on the resulting strbufs, so it is >>> > simpler to just use string_list_split, which takes care of removing >>> > the delimiter for us. >>> > >>> > Written-by: Jeff King >>> >>> Perhaps Peff can give his sign-off... >> >> Ah, right. I usually sign-off when sending to the list, so the version >> he pulled from GitHub didn't have it. >> >> Definitely: >> >> Signed-off-by: Jeff King >> >> And I don't think "Written-by" was doing much here, anyway; I am already >> the author by the From header at the top. :) > > ;-). > > So, is everybody happy with this round? > > With another series on top for the "conditional" stuff, I guess we > are ready to do the formatting for "git branch --list", which would > be a big step forward. > > Thanks. Yes, I'll be sending that soon :D > Yes, v6 looks good to me and is: > > Reviewed-by: Eric Sunshine > I have not looked with nearly as close an eye as Eric, but I did not see > anything objectionable (and I trust the reviews that have led us up to > v6 in the first place). > > Thanks, Karthik, for your continued work on this (and to reviewers, of > course :) ). > > -Peff Thanks to everyone who helped, especially reviewers for going through such long patch series. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 15/21] init: allow alternate ref strorage to be set for new repos
On 02/17/2016 09:47 PM, David Turner wrote: > On Fri, 2016-02-12 at 16:26 +0100, Michael Haggerty wrote: >> [...] >> Is it worth testing re-initializing with the same --ref-storage? >> Perhaps >> not. > > Would be nice, but since we cannot guarantee that any alternate backend > exists, we have no way to test this. We'd have to add an entire "test" > ref backend just for tests, which seems a bit overboard. I meant re-initializing a repository that is using the "files" backend with `--ref-storage=files` to make sure that it is a NOP. But I think this functionality is unlikely to be broken so it wouldn't be a very high-value test. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 14/21] refs: always handle non-normal refs in files backend
On 02/18/2016 03:44 AM, David Turner wrote:
> On Fri, 2016-02-12 at 16:07 +0100, Michael Haggerty wrote:
>> On 02/05/2016 08:44 PM, David Turner wrote:
>>> Always handle non-normal (per-worktree or pseudo) refs in the files
>>> backend instead of alternate backends.
>>>
>>> Sometimes a ref transaction will update both a per-worktree ref and
>>> a
>>> normal ref. For instance, an ordinary commit might update
>>> refs/heads/master and HEAD (or at least HEAD's reflog).
>>>
>>> Updates to normal refs continue to go through the chosen backend.
>>>
>>> Updates to non-normal refs are moved to a separate files backend
>>> transaction.
>>>
>>> Signed-off-by: David Turner
>>> ---
>>> refs.c | 81
>>> --
>>> 1 file changed, 79 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/refs.c b/refs.c
>>> index 227c018..18ba356 100644
>>> --- a/refs.c
>>> +++ b/refs.c
>>> @@ -9,6 +9,11 @@
>>> #include "object.h"
>>> #include "tag.h"
>>>
>>> +static const char split_transaction_fail_warning[] = N_(
>>> + "A ref transaction was split across two refs backends.
>>> Part of the "
>>> + "transaction succeeded, but then the update to the per
>>> -worktree refs "
>>> + "failed. Your repository may be in an inconsistent
>>> state.");
>>> +
>>> /*
>>> * We always have a files backend and it is the default.
>>> */
>>> @@ -791,6 +796,13 @@ void ref_transaction_free(struct
>>> ref_transaction *transaction)
>>> free(transaction);
>>> }
>>>
>>> +static void add_update_obj(struct ref_transaction *transaction,
>>> + struct ref_update *update)
>>> +{
>>> + ALLOC_GROW(transaction->updates, transaction->nr + 1,
>>> transaction->alloc);
>>> + transaction->updates[transaction->nr++] = update;
>>> +}
>>> +
>>> static struct ref_update *add_update(struct ref_transaction
>>> *transaction,
>>> const char *refname)
>>> {
>>> @@ -798,8 +810,7 @@ static struct ref_update *add_update(struct
>>> ref_transaction *transaction,
>>> struct ref_update *update = xcalloc(1, sizeof(*update) +
>>> len);
>>>
>>> memcpy((char *)update->refname, refname, len); /* includes
>>> NUL */
>>> - ALLOC_GROW(transaction->updates, transaction->nr + 1,
>>> transaction->alloc);
>>> - transaction->updates[transaction->nr++] = update;
>>> + add_update_obj(transaction, update);
>>> return update;
>>> }
>>>
>>> @@ -1217,11 +1228,38 @@ static int dereference_symrefs(struct
>>> ref_transaction *transaction,
>>> return 0;
>>> }
>>>
>>> +/*
>>> + * Move all non-normal ref updates into a specially-created
>>> + * files-backend transaction
>>> + */
>>> +static int move_abnormal_ref_updates(struct ref_transaction
>>> *transaction,
>>> +struct ref_transaction
>>> *files_transaction,
>>> +struct strbuf *err)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < transaction->nr; i++) {
>>> + int last;
>>> + struct ref_update *update = transaction
>>> ->updates[i];
>>> +
>>> + if (ref_type(update->refname) == REF_TYPE_NORMAL)
>>> + continue;
>>> +
>>> + last = --transaction->nr;
>>> + transaction->updates[i] = transaction
>>> ->updates[last];
>>> + add_update_obj(files_transaction, update);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>> I think this function is incorrect. The update that was previously at
>> transaction->updates[i] never gets checked for abnormality.
>
> Yes it does; that's the "update" variable that we just checked.
Sorry, I meant to say "the update that was previously at
`transaction->updates[transaction->nr - 1]` never gets checked for
abnormality". Because it gets moved to `transaction->updates[i]`, then
`i` is incremented without checking it.
> [...]
>> Another alternative would be to set
>>
>> update->flags |= REF_ABNORMAL
>> [...]
> I'm also interested in this idea. Perhaps it would also be nice to
> report *why* they fail (e.g. the conflicting ref name). I did a
> variant of this with for the journal code, but my way of doing it
> turned out to be a bad idea (long story). But I want to stay focused
> on the simplest thing possible, for now.
Fair enough.
> [...]
>> Does initial_ref_transaction_commit() need the same treatment?
>
> We only use that for remote refs -- I'm not sure if those can be
> symrefs. Wouldn't hurt.
Hmmm, good point. I think the only symrefs that can be set during a
clone are `HEAD` and `refs/remotes/origin/HEAD`. But I guess that no
other references are updated *through* these symrefs, so it's probably
OK. I haven't checked carefully, though.
> [...]
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
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 13/21] refs: resolve symbolic refs first
On 02/18/2016 01:29 AM, David Turner wrote: > On Fri, 201-02-12 at 15:09 +0100, Michael Haggerty wrote:] >> On 02/05/2016 08:44 PM, David Turner wrote: >>> Before committing ref updates, split symbolic ref updates into two >>> parts: an update to the underlying ref, and a log-only update to >>> the >>> symbolic ref. This ensures that both references are locked >>> correctly >>> while their reflogs are updated. >>> >>> It is still possible to confuse git by concurrent updates, since >>> the >>> splitting of symbolic refs does not happen under lock. So a >>> symbolic ref >>> could be replaced by a plain ref in the middle of this operation, >>> which >>> would lead to reflog discontinuities and missed old-ref checks. >> >> This patch is doing too much at once for my little brain to follow. >> >> My first hangup is the change to setting RESOLVE_REF_NO_RECURSE >> unconditionally in lock_ref_sha1_basic(). I count five callers of >> that >> function and see no justification for why the change is OK in the >> context of each caller. Here are some thoughts: >> >> * The call from files_create_symref() sets REF_NODEREF, so it is >> unaffected by this change. > > Yes. > >> * The call from files_transaction_commit() is preceded by a call to >> dereference_symrefs(), which I assume effectively replaces the need >> for >> RESOLVE_REF_NO_RECURSE. > > Yes. > >> * There are two calls from files_rename_ref(). Why is it OK to do >> without RESOLVE_REF_NO_RECURSE there? >> >> * For the oldrefname call, I suppose the justification is the >> "(flag & >> REF_ISSYMREF)" check earlier in the function. (But does this >> introduce a >> significant TOCTOU race?) > > The refs code as a whole seems likely to have TOCTOU issues. In > general, anywhere we check/set flag & REF_ISSYMREF without holding a > lock, we have a potential problem. I haven't generally tried to handle > these cases, since they're not presently handled. I agree that we don't do so well here, though I think that most races would result in reading/writing a ref that was pointed to by the symref a moment ago, which is usually indistinguishable to the user from their update having gone through the moment before the symref was updated. So I don't think your change makes this bit of code significantly worse. > The central problem with this area of the code is that commit interacts > so intimately with the locking machinery. I understand some of why > it's done that way. In particular, your change to ref locking to not > hold lots of open files was a big win for us at Twitter. But this > means that it's hard to deal with cross-backend ref updates: you want > to hold multiple locks, and backends don't have the machinery for it. > > We could add backend hooks to specifically lock and unlock refs. Then > the backend commit code would just be handled a bundle of locked refs > and would commit them. This might be hairy, but it could fix the > TOCTOU problems. So, first lock the outer refs, then split out updates > for any which are symbolic refs, and lock those. Finally, commit all > updates (split by backend). As chance would have it, for an internal GitHub project I've implemented hooks that can be called *during* a ref transaction. The hooks can, for example, take arbitrary actions between the time that the reflocks are all acquired and the time that the updates start to be committed. I didn't submit this code upstream because I didn't think that it would benefit other users, but many it would be useful for implementing split-backend reference transaction commits. E.g., the primary reference transaction could run the secondary backend's commit while holding the locks for the primary backend references. Let me think about it. I don't think this is urgent though. The current code is not significantly racy in mainstream usage scenarios, right? > One downside of this is that right now, the backend API is relatively > close to the front-end, and this would leak what should be an > implementation detail. But maybe this is necessary to knit multiple > backends together. > > But I'm not sure that this is necessary right now, because I'm not sure > that I'm actually making TOCTOU issues much worse. Agreed. > [...] > That's a legit complaint. The problem, as you note, is that doing some > of these steps completely independently doesn't work. But I'll try > splitting out what I can. Thanks! Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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: GSoC 2016: applications open, deadline = Fri, 19/2
On Wed, 2016-02-17 at 13:33 -0800, Junio C Hamano wrote: > Jeff King writes: > > > On Wed, Feb 17, 2016 at 09:21:20PM +0100, Matthieu Moy wrote: > > > > > > I am wondering if we heard from libgit2 folks if they want us > > > > to > > > > host them (or they want to participate in GSoC at all). > > > > > > The libgit2 mention is left from previous versions of this page. > > > I left > > > a message on their IRC channel asking to join this thread if > > > people were > > > interested (I don't know the libgit2 community really well, and I > > > didn't > > > find a mailing-list to Cc here). > > > > > > I did not hear anything from them. We should probably remove the > > > mention > > > of libgit2. Or, if anyone receiving this message is interested in > > > having > > > libgit2 participate, or knows anyone who may be, speak now. > > > > I think they do a lot of their communication via GitHub issues. > > I've > > cc'd Carlos, the maintainer, who can ping the rest of the community > > as > > appropriate. > > > > I don't think we did a libgit2 project last year, and included the > > libgit2 references mainly so that we would not drop them with zero > > warning. > > Understandable. I do not mind seeing us hosting them if that is > what they want, but the candidate selection and mentor assignment > between two more-or-less independent projects would not work very > well unless there is _some_ degree of coordination ;-) We still have most of the same things open as for the 2014 list. I'll ask around to see if we have. Last year I wasn't involved in the candidate selection but IIRC we didn't do a project as none of the applications showed the candidates would be capable of doing the project they were applying for. I'll ask around to make sure people would be able to be mentors, but I think that we would still like to put forward a few projects (I can send a PR with the projects that we would still like to see to the 2016 page). Cheers, cmn -- 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 v5 25/27] refs: add LMDB refs storage backend
Caveat: I did not study how to use lmdb. I just guessed what it does
based on function names. I don't know much about refs handling either
(especially since the transaction thing is introduced)
> diff --git a/Documentation/technical/refs-lmdb-backend.txt
> b/Documentation/technical/refs-lmdb-backend.txt
> new file mode 100644
> index 000..eb81465
> --- /dev/null
> +++ b/Documentation/technical/refs-lmdb-backend.txt
> +Reflog values are in the same format as the original files-based
> +reflog, including the trailing LF. The date in the reflog value
> +matches the date in the timestamp field.
..except that SHA-1s are stored in raw values instead of hex strings.
> diff --git a/Documentation/technical/repository-version.txt
> b/Documentation/technical/repository-version.txt
> index 00ad379..fca5ecd 100644
> --- a/Documentation/technical/repository-version.txt
> +++ b/Documentation/technical/repository-version.txt
> @@ -86,3 +86,8 @@ for testing format-1 compatibility.
> When the config key `extensions.preciousObjects` is set to `true`,
> objects in the repository MUST NOT be deleted (e.g., by `git-prune` or
> `git repack -d`).
> +
> +`refStorage`
> +
> +This extension allows the use of alternate ref storage backends. The
> +only defined value is `lmdb`.
refStorage accepts empty string and `files` as well, should probably
be worth mentioning.
> diff --git a/refs/lmdb-backend.c b/refs/lmdb-backend.c
> +#include "../cache.h"
> +#include
> +#include "../object.h"
> +#include "../refs.h"
> +#include "refs-internal.h"
> +#include "../tag.h"
> +#include "../lockfile.h"
I'm quite sure we don't need "../". We have plenty of source files in
subdirs and many of them (haven't checked all) just go with
#include "cache.h".
> +struct lmdb_transaction transaction;
static?
> +
> +static int in_write_transaction(void)
> +{
> + return transaction.txn && !(transaction.flags & MDB_RDONLY);
> +}
> +
> +static void init_env(MDB_env **env, const char *path)
> +{
> + int ret;
> + if (*env)
> + return;
> +
> + assert(path);
> +
> + ret = mdb_env_create(env);
> + if (ret)
> + die("BUG: mdb_env_create failed: %s", mdb_strerror(ret));
> + ret = mdb_env_set_maxreaders(*env, 1000);
> + if (ret)
> + die("BUG: mdb_env_set_maxreaders failed: %s",
> mdb_strerror(ret));
> + ret = mdb_env_set_mapsize(*env, (1<<30));
> + if (ret)
> + die("BUG: mdb_set_mapsize failed: %s", mdb_strerror(ret));
> + ret = mdb_env_open(*env, path, 0 , 0664);
This permission makes me wonder if we need adjust_shared_perm() here
and some other places.
> + if (ret)
> + die("BUG: mdb_env_open (%s) failed: %s", path,
> + mdb_strerror(ret));
> +}
> +
> +static int lmdb_init_db(int shared, struct strbuf *err)
> +{
> + /*
> + * To create a db, all we need to do is make a directory for
> + * it to live in; lmdb will do the rest.
> + */
> +
> + if (!db_path)
> + db_path = xstrdup(real_path(git_path("refs.lmdb")));
> +
> + if (mkdir(db_path, 0775) && errno != EEXIST) {
> + strbuf_addf(err, "%s", strerror(errno));
maybe strbuf_addstr, unless want to add something more, "mkdir failed"?
> +static int read_per_worktree_ref(const char *submodule, const char *refname,
> + struct MDB_val *val, int *needs_free)
>From what I read, I suspect these _per_worktree functions will be
identical for the next backend. Should we just hand over the job for
files backend? For all entry points that may deal with per-worktree
refs, e.g. lmdb_resolve_ref_unsafe, can we check ref_type() first
thing, if it's per-worktree we call refs_be_files.resolve_ref_unsafe()
instead? It could even be done at frontend level,
e.g. refs.c:resolve_ref_unsafe().
Though I may be talking rubbish here because I don't know how whether
it has anything to do with transactions.
> +{
> + struct strbuf sb = STRBUF_INIT;
> + struct strbuf path = STRBUF_INIT;
> + struct stat st;
> + int ret = -1;
> +
> + submodule_path(&path, submodule, refname);
> +
> +#ifndef NO_SYMLINK_HEAD
It started with the compiler warns about unused "st" when this macro
is defined. Which makes me wonder, should we do something like this to
make sure this code compiles unconditionally?
+#ifndef NO_SYMLINK_HEAD
+ int no_symlink_head = 0;
+#else
+ int no_symlink_head = 1;
+#endif
...
+ if (!no_symlink_head) {
...
> +int lmdb_transaction_begin_flags(struct strbuf *err, unsigned int flags)
static?
> +#define MAXDEPTH 5
> +
> +static const char *parse_ref_data(struct lmdb_transaction *transaction,
> + const char *refname, const char *ref_data,
> + unsigned char *sha1, int resolve_flags,
> + int *flags, int bad_name)
> +{
> + int depth = MAXDEPTH;
> + const char *buf;
> + static str
Re: [PATCH v4 2/3] config: add 'type' to config_source struct that identifies config type
On 17 Feb 2016, at 14:12, Johannes Schindelin wrote: > Hi Lars, > > On Mon, 15 Feb 2016, larsxschnei...@gmail.com wrote: > >> From: Lars Schneider >> >> Use the config type to print more detailed error messages that inform >> the user about the origin of a config error (file, stdin, blob). > > Given that you settled on `--show-origin` as the command-line option, I > would have expected s/type/origin/g, too... Agreed. Junio suggested "origin_type" which I like, too. Thanks, Lars-- 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 v5 2/3] config: add 'type' to config_source struct that identifies config type
On 17 Feb 2016, at 22:31, Junio C Hamano wrote: > Junio C Hamano writes: > >> larsxschnei...@gmail.com writes: >> >>> From: Lars Schneider >>> >>> Use the config type to print more detailed error messages that inform >>> the user about the origin of a config error (file, stdin, blob). >> >> "type" is too broad a word in the context of configuration file, and >> does not help readers as a variable or a field name in a structure. >> You are not talking about "this is a binary typed variable", for >> example. >> >> If showing the origin is useful to the user, that origin should be >> called origin, not type, I would think. > > You seem to use in 3/3 "origin type", and I think that is a sensible > phrasing. Renaming 'type' to 'origin' in this patch would not be a > good idea, but renaming it to 'origin_type' would be a great thing > to do. OK, I agree. I will wait one more day for other comments and make a reroll with the change tomorrow. Thanks, Lars > > In the context of configuration parsing, 'origin' would be like > "file called .git/config", "standard input", "command line's 3rd > argument", etc., i.e. for some origin types, it would consist of > pair, while some other > origin type (e.g. "standard input") there is no need for an > "origin_value" that further qualifies the origin. > > 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
Re: GSoC 2016: applications open, deadline = Fri, 19/2
On 17 Feb 2016, at 19:58, Matthieu Moy wrote: > Lars Schneider writes: > >> Coincidentally I started working on similar thing already (1) and I have >> lots of ideas around it. > > I guess it's time to start sharing these ideas then ;-). > > I think there's a lot to do. If we want to push this idea as a GSoC > project, we need: > > * A rough plan. We can't expect students to read a vague text like > "let's make Git safer" and write a real proposal out of it. > > * A way to start this rough plan incrementally (i.e. first step should > be easy and mergeable without waiting for next steps). > > Feel free to start writting an idea for > http://git.github.io/SoC-2016-Ideas/. It'd be nice to have a few more > ideas before Friday. We can polish them later if needed. I published my ideas here: https://github.com/git/git.github.io/pull/125/files Do you think that works as start or do we need more detailed, hands-on instructions? Thanks, Lars -- 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
