Re: git submodule should honor "-c credential.helper" command line argument

[Jacob Keller]

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

[Jeff King]

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, _config);
  argv_array_pushf(_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

[Matthieu Moy]

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

[Jacob Keller]

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

[Matthieu Moy]

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

[Mehul Jain]

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

[Johannes Schindelin]

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

[Jeff King]

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

[Duy Nguyen]

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

[Junio C Hamano]

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

[Junio C Hamano]

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, "!", )) {
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

[Duy Nguyen]

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

[Duy Nguyen]

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(, , , 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

[Eric Sunshine]

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=", )) {
> +   string_list_append(_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 = _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

[Jacob Keller]

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

[Stefan Beller]

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(_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
N_("initialize submodules in the clone")),
OPT_BOOL(0, "recurse-submodules", _recursive,
N_("initialize submodules in the clone")),
+   OPT_INTEGER('j', "jobs", _jobs,
+   N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", _template, N_("template-directory"),
   N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", _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(, "submodule", "update", "--init", 
"--recursive", NULL);
+
+   if (max_jobs != -1)
+   argv_array_pushf(, "--jobs=%d", max_jobs);
+
+   err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+   argv_array_clear();
+   }
 
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

[Stefan Beller]

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

[Stefan Beller]

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,
+>update_strategy) < 0)
+   die(_("invalid value for %s"), var);
}
 
strbuf_release();
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 

[PATCHv13 0/7] Expose submodule parallelism to the user

[Stefan Beller]

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, "!", >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

[Stefan Beller]

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(, "--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(_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 

[PATCHv13 4/7] submodule update: direct error message to stderr

[Stefan Beller]

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

[Stefan Beller]

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", , "",
   N_("Create a shallow clone truncated to the "
  "specified number of revisions")),
+   OPT_INTEGER('j', "jobs", _jobs,
+   N_("parallel jobs")),
OPT__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,
- );
+
+   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,
+  );
 
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 &&
+

[PATCHv13 5/7] git submodule update: have a dedicated helper for cloning

[Stefan Beller]

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, _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();
+   strbuf_addf(, "submodule.%s.url", sub->name);
+   git_config_get_string(sb.buf, );
+   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();
+   strbuf_addf(, "%s/.git", ce->name);
+   needs_cloning = !file_exists(sb.buf);
+
+   strbuf_reset();
+   strbuf_addf(, "%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(>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(>args, "submodule--helper");
+   argv_array_push(>args, "clone");
+   if (pp->quiet)
+   argv_array_push(>args, "--quiet");
+
+   if (pp->prefix)
+   argv_array_pushl(>args, "--prefix", pp->prefix, 
NULL);
+
+   argv_array_pushl(>args, "--path", sub->path, NULL);
+   argv_array_pushl(>args, "--name", sub->name, NULL);
+   argv_array_pushl(>args, "--url", strdup(url), NULL);
+   if (pp->reference)
+   argv_array_push(>args, pp->reference);
+  

Re: [PATCHv12 0/7] Expose submodule parallelism to the user

[Stefan Beller]

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

[Junio C Hamano]

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]

Junio C Hamano  writes:

>> +strbuf_reset();
>> +strbuf_addf(, "submodule.%s.url", sub->name);
>> +if (git_config_get_string(sb.buf, )) {
>> +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

[Stefan Beller]

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

[Stefan Beller]

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, "!", >command))
>> +   else if (skip_prefix(value, "!", )) {
>> 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

[Amadeusz Żołnowski]

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

[Junio C Hamano]

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, "!", >command))
> +   else if (skip_prefix(value, "!", )) {
> 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

[Brian Norris]

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

[Junio C Hamano]

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

[Junio C Hamano]

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

[Stefan Beller]

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", , "",
   N_("Create a shallow clone truncated to the "
  "specified number of revisions")),
+   OPT_INTEGER('j', "jobs", _jobs,
+   N_("parallel jobs")),
OPT__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,
- );
+
+   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,
+  );
 
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 &&
+

[PATCHv12 1/7] submodule-config: keep update strategy around

[Stefan Beller]

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,
+>update_strategy) < 0)
+   die(_("invalid value for %s"), var);
}
 
strbuf_release();
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, "!", )) {
+   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

[Stefan Beller]

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

[Stefan Beller]

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(_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
N_("initialize submodules in the clone")),
OPT_BOOL(0, "recurse-submodules", _recursive,
N_("initialize submodules in the clone")),
+   OPT_INTEGER('j', "jobs", _jobs,
+   N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", _template, N_("template-directory"),
   N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", _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(, "submodule", "update", "--init", 
"--recursive", NULL);
+
+   if (max_jobs != -1)
+   argv_array_pushf(, "--jobs=%d", max_jobs);
+
+   err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+   argv_array_clear();
+   }
 
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

[Stefan Beller]

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, _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();
+   strbuf_addf(, "submodule.%s.url", sub->name);
+   git_config_get_string(sb.buf, );
+   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();
+   strbuf_addf(, "%s/.git", ce->name);
+   needs_cloning = !file_exists(sb.buf);
+
+   strbuf_reset();
+   strbuf_addf(, "%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(>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(>args, "submodule--helper");
+   argv_array_push(>args, "clone");
+   if (pp->quiet)
+   argv_array_push(>args, "--quiet");
+
+   if (pp->prefix)
+   argv_array_pushl(>args, "--prefix", pp->prefix, 
NULL);
+
+   argv_array_pushl(>args, "--path", sub->path, NULL);
+   argv_array_pushl(>args, "--name", sub->name, NULL);
+   argv_array_pushl(>args, "--url", strdup(url), NULL);
+   if (pp->reference)
+   argv_array_push(>args, pp->reference);
+  

[PATCHv12 3/7] fetching submodules: respect `submodule.fetchJobs` config option

[Stefan Beller]

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(, "--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(_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 

[PATCHv12 4/7] submodule update: direct error message to stderr

[Stefan Beller]

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

[Stefan Beller]

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, "!", >command))
+   else if (skip_prefix(value, "!", )) {
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

[Junio C Hamano]

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

[Junio C Hamano]

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, _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

[Junio C Hamano]

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

[Juergen Kosel]

-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

[Junio C Hamano]

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

[Junio C Hamano]

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, );
> +
> + /* 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();
> + strbuf_addf(, "submodule.%s.url", sub->name);
> + if (git_config_get_string(sb.buf, )) {
> + 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(, "remote.%s.url", remote);
> + free(remote);
> +
> + if (git_config_get_string(remotesb.buf, ))
> + /*
> +  * The repository is its own
> +  * authoritative upstream
> +  */
> + remoteurl = xgetcwd();
> + url = relative_url(remoteurl, url, NULL);
> + strbuf_release();
> + 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();
> + strbuf_addf(, "submodule.%s.update", sub->name);
> + if (git_config_get_string_const(sb.buf, ) &&



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(>update_strategy);
> +
> + if (git_config_set(sb.buf, upd))
> + die(_("Failed to register update mode for submodule 
> path '%s'"), displaypath);
> + }
> + strbuf_release();
> + 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  

Re: [PATCH 1/2] submodule: port resolve_relative_url from shell to C

[Junio C Hamano]

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

[David Turner]

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(, 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(, , , 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 *submodule_env = NULL;
> > +   

Re: [PATCHv11 0/7] Expose submodule parallelism to the user

[Junio C Hamano]

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

[Junio C Hamano]

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,
> +  >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, "!", >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

[Junio C Hamano]

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

[Stefan Beller]

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

[David Turner]

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)

[Junio C Hamano]

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

[John Smith]

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

[Michael Haggerty]

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, )) {
>>  /* 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

[Karthik Nayak]

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

[Michael Haggerty]

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

[Michael Haggerty]

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

[Michael Haggerty]

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

[Carlos Martín Nieto]

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

[Duy Nguyen]

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(, 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 struct 

Re: [PATCH v4 2/3] config: add 'type' to config_source struct that identifies config type

[Lars Schneider]

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

[Lars Schneider]

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
> 

Re: GSoC 2016: applications open, deadline = Fri, 19/2

[Lars Schneider]

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