Re: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Johannes Sixt]

Am 26.04.2016 um 23:37 schrieb Junio C Hamano:

  * The necessary update to avoid end-user mistake would look like
this.  I am not queuing this or further working on it myself,
as I am not sure if it is all that useful.


Whoever picks up this patch, be warned that the i18n coding should be 
corrected:



+static void no_commit_impossible(const char *message)
+{
+   if (!option_commit) {
+   warning("%s\n%s", _(message),


The i18n call around message is not required, because...


+   _("--no-commit is impossible"));
+   warning(_("In future versions of Git, this will become an 
error."));
+   }
+}
+
  int cmd_merge(int argc, const char **argv, const char *prefix)
  {
unsigned char result_tree[20];
@@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * If head can reach all the merge then we are up to date.
 * but first the most common case of merging one remote.
 */
+   no_commit_impossible(_("Already up-to-date"));


... the call sites takes care of it.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Jeff King]

On Tue, Apr 26, 2016 at 08:59:22PM -0700, Stefan Beller wrote:

> > Maybe we can do a mix of 2 and 4:
> >
> >1) HTTP grows more extensions; other protocols stagnate for now.
> >2) Come up with a backwards-incompatible protocol v2, foccussed on
> >capabilities negotiation phase, hitting alternative end points
> >(non http only, or rather a subset of protocols only)
> > 3) if HTTP sees the benefits of the native protocol v2, we may switch
> > HTTP, too
> >
> > (in time order of execution. Each point is decoupled from the others and may
> > be done by different people at different times.)
> >
> 
> Today I rebased protocol-v2[1] and it was fewer conflicts than expected.
> I am surprised by myself that there is even a test case for v2 already,
> so I think it is more progressed that I had in mind. Maybe we can do 1)
> for now and hope that the non http catches up eventually?

If the plan is something like:

  1. v2 exists, but client/server don't know when they should use it.

  2. smart-http grows an extra parameter for the client to say "hey, I
 know v2"

  3. Other protocols get some manual v2 support (e.g., client asks for
 "upload-pack2" if instructed by the user, server either speaks v2
 then or says "eh, what?").

I like that very much. It lets us "cheat" on the hard part of the
problem for http, which is what David's series is doing, but it provides
a clear path forward for the protocols eventually reaching feature
parity (namely that eventually all servers speak v2, and the client can
start asking for v2 by default).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Stefan Beller]

On Mon, Apr 25, 2016 at 3:10 PM, Stefan Beller  wrote:
> On Mon, Apr 25, 2016 at 9:44 AM, David Turner  
> wrote:
>> On Wed, 2016-04-20 at 16:57 -0400, Jeff King wrote:
>>> On Wed, Apr 20, 2016 at 04:46:55PM -0400, David Turner wrote:
>>>
>>> > As you note, it appears that git-daemon does sort-of have support
>>> > for
>>> > extra args -- see parse_host_arg.  So it wouldn't be hard to add
>>> > something here. Unfortunately, current versions of git die on
>>> > unknown
>>> > args.  So this change would not be backwards-compatible.  We could
>>> > put
>>> > a decider on it so that clients would only try it when explicitly
>>> > enabled.  Or we could have clients try it with, and in the event of
>>> > an
>>> > error, retry without.  Neither is ideal, but both are possible.
>>>
>>> Right. This ends up being the same difficulty that the v2 protocol
>>> encountered; how do you figure out what you can speak without
>>> resorting
>>> to expensive fallbacks, when do you flip the switch, do you remember
>>> the
>>> protocol you used last time with this server, etc.
>>
>> Right.
>>
>> [moved]
>>> > If I read this code correctly, git-over-ssh will pass through
>>> > arbitrary
>>> > arguments.  So this should be trivial.
>>>
>>> It does if you are ssh-ing to a real shell-level account on the
>>> server,
>>> but if you are using git-shell or some other wrapper to restrict
>>> clients
>>> from running arbitrary commands, it will likely reject it.
>>
>> Oh, I see how I was mis-reading shell.c.  Oops.
>> [/moved]
>>
>>
>>> Which isn't to say it's necessarily a bad thing. Maybe the path
>>> forward
>>> instead of v2 is to shoe-horn this data into the pre-protocol
>>> conversation, and go from there. The protocol accepts that "somehow"
>>> it
>>> got some extra data from the transport layer, and acts on its
>>> uniformly.
>>
>> OK, so it seems like only HTTP (and non-git-shell-git://) allow backwar
>> ds-compatible optional pre-protocol messages.  So we don't have good
>> options; we only have bad ones.  We have to decide which particular
>> kind of badness we're willing to accept, and to what degree we care
>> about extensibility.  As I see it, the badness options are (in no
>> particular order):
>>
>> 1. Nothing changes.
>> 2. HTTP grows more extensions; other protocols stagnate.
>> 3. HTTP grows extensions; other protocols get extensions but:
>>a. only use them on explicit client configuration or
>>b. try/fail/remember per-remote
>> 4. A backwards-incompatible protocol v2 is introduced, which
>>hits alternate endpoints (with the same a/b as above).  This is
>>different from 3. in that protocol v2 is explicitly designed around
>>a capabilities negotiation phase rather than unilateral client-side
>>decisions.
>> 5. Think of another way to make fetch performant with many refs, and
>> defer the extension decision.
>
> I'd prefer 2,3,4 over 1,5.
>
> Speaking about 2,3,4:
>
> Maybe we can do a mix of 2 and 4:
>
>1) HTTP grows more extensions; other protocols stagnate for now.
>2) Come up with a backwards-incompatible protocol v2, foccussed on
>capabilities negotiation phase, hitting alternative end points
>(non http only, or rather a subset of protocols only)
> 3) if HTTP sees the benefits of the native protocol v2, we may switch
> HTTP, too
>
> (in time order of execution. Each point is decoupled from the others and may
> be done by different people at different times.)
>

Today I rebased protocol-v2[1] and it was fewer conflicts than expected.
I am surprised by myself that there is even a test case for v2 already,
so I think it is more progressed that I had in mind. Maybe we can do 1)
for now and hope that the non http catches up eventually?


[1] https://github.com/stefanbeller/git/tree/protocol-v2
--
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 03/15] submodule add: label submodules if asked to

[Jacob Keller]

On Tue, Apr 26, 2016 at 4:19 PM, Stefan Beller  wrote:
> On Tue, Apr 26, 2016 at 3:50 PM, Jacob Keller  wrote:
>> On Tue, Apr 26, 2016 at 1:50 PM, Stefan Beller  wrote:
>>> When adding new submodules, you can specify the
>>> label(s) the submodule belongs to by giving one or more
>>> --label arguments. This will record each label in the
>>> .gitmodules file as a value of the key
>>> "submodule.$NAME.label".
>>>
>>
>> Ok so labels will be in the .gitmodules file. This means that if we go
>> back in history using git bisect or something similar, we'll
>> potentially change what the default submodules are for example?
>
> Good point! Yes that's exactly what we want.
>

Ok. That makes sense.

> Ideally (read: in a later patch series), checkout will automatically
> checkout submodules for you if you have configured 
> `submodule.updateOnCheckout`,
> such that switching old and new revisions will add or delete
> submodules automatically.
>

This is something I definitely would like to be able to tell people.

>>
>> This is sort of why having some submodule data appear in the
>> .git/config file is useful since it helps keep things like the remote
>> url safe from being updated when doing this sort of thing.
>
> (Unrelated, but my thoughts on this)
> The remote url mechanism is broken with the .gitmodules file in some use 
> cases:
> Consider there is an upstream "kernel.org" which hosts a repository with
> submodules. Now you want to mirror that superproject to "kernel.mymirror.org"
> and you start with
>
> git clone --mirror git://kernel.org/superproject
>
> When the superproject uses relative URLs for the submodules, this will
> break your mirror,
> if you did not mirror them exactly with the same relative path. Then
> cloning from
> your mirror will result in broken submodule URLs.
>
> So you would want an additional mechanism for URLs. Jonathan came up
> with the idea of having another configuration file in a refs/submodules/config
> branch which essentially allows to annotate/enhance the .gitmodules file.
>

I like the idea of refs/submodules/config.

> So in such a configuration you could say:
>
> defaultRelativeURLBase = kernel.org
> [submodule."foo"]
> relativeURLBase = kernel.mymirror.org
> protocol = ssh
> # ssh as opposed to git:// which was inherited from the superproject
>
> which allows finer controls of submodule/repository mirroring.
> As the refs/submodule/configuration is not part of the history of the
> superproject,
> mirroring can be done without changing history, but still having URLs changed 
> to
> the internal mirror.
>
>>
>> I am not sure if labels will be that important in this case?
>
> I am not sure. If it turns out to be a problem, maybe we can
> introduce a hook, that will be called on adding new submodules via lables?
>
> Thanks,
> Stefan
>
>>
>> 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: [PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Eric Sunshine]

On Tue, Apr 26, 2016 at 5:37 PM, Junio C Hamano  wrote:
> A user who uses "--no-commit" does so with the intention to record a
> resulting merge after amending the merge result in the working tree.
> But there is nothing to amend and record, if the same "git merge"
> without "--no-commit" wouldn't have created a merge commit (there
> are two cases: (1) the other branch is a descendant of the current
> branch, (2) the other branch is an ancestor of the current branch).
>
> The user would want to know that before doing further damange to his

s/damange/damage/

> history.  When "merge --no-commit" fast-forwarded or succeeded with
> "already up-to-date" or "fast-forward", give a warning message.
>
> We may want to turn this into a die() after a transition period.
>
> Signed-off-by: Junio C Hamano 
--
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] Move test-* to t/helper/ subdirectory

[Duy Nguyen]

On Wed, Apr 27, 2016 at 5:07 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> This keeps top dir a bit less crowded. And because these programs are
>> for testing purposes, it makes sense that they stay somewhere in t/
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  This patch will break any patches that add new test programs.
>>  Luckily, none in 'next' or 'pu' does that. I know lmdb backend adds
>>  test-lmdb-backend, so a manual move and some .gitignore fixup is
>>  required there.
>
> Can you (or somebody else) double check that the resulting Makefile
> gets the build dependencies and exec path right?
>
> I am seeing occasional failure from t0040 when checking out between
> master and pu, and between the branches test-parse-options do change
> behaviour which explains the breakage.

I looked at "make -p" and saw nothing wrong with build rules. A bit
concerned about fa8fe28 (Makefile: do not allow gnu make to remove
test-*.o files - 2007-08-30) because make info pages say it could
leave probably broken .o files when make is interrupted. But that's
probably not it.

There is a problem moving between master and next/pu though.
bin-wrappers is not regenerated after the move so it could point to
the old binaries (in the other place). Not sure how to fix that
cleanly, will think of something, maybe, in about 8 hours.
-- 
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 03/15] submodule add: label submodules if asked to

[Stefan Beller]

On Tue, Apr 26, 2016 at 3:50 PM, Jacob Keller  wrote:
> On Tue, Apr 26, 2016 at 1:50 PM, Stefan Beller  wrote:
>> When adding new submodules, you can specify the
>> label(s) the submodule belongs to by giving one or more
>> --label arguments. This will record each label in the
>> .gitmodules file as a value of the key
>> "submodule.$NAME.label".
>>
>
> Ok so labels will be in the .gitmodules file. This means that if we go
> back in history using git bisect or something similar, we'll
> potentially change what the default submodules are for example?

Good point! Yes that's exactly what we want.

Consider this:
We have:
libA
libB

Now libB grows a lot and someone decides to split it up into
2 libraries, so we'll have:
libA
libB-small
libC-was-part-of-B

As libB (the big one repo containing all the code) was no open source,
we can deprecate it fast and just keep developing in libB-small and
libC-was-part-of-B. As we do only small changes at a time, we'll keep
those 2 in the same label set, so the users can pick up the changes via

git submodule update
#  libB-small and  libC-was-part-of-B are in both the same group,
e.g. default
# so they will be checked out.

When a bug is found in the future, you'd use `git bisect` to find it.
In a repository with no
submodules (analogy: think of splitting a file in two. Checking out
older revisions
will give the one old file, checking out newer revisions will give 2 new files.)
that works great. With submodules we want to have the same properties.

git checkout tag-before-libB-split
git submodule update
# get libB checked out

git checkout tag-after-libB-split
git submodule update
# get libB-small and libC-was-part-of-B checked out

Ideally (read: in a later patch series), checkout will automatically
checkout submodules for you if you have configured `submodule.updateOnCheckout`,
such that switching old and new revisions will add or delete
submodules automatically.

>
> This is sort of why having some submodule data appear in the
> .git/config file is useful since it helps keep things like the remote
> url safe from being updated when doing this sort of thing.

(Unrelated, but my thoughts on this)
The remote url mechanism is broken with the .gitmodules file in some use cases:
Consider there is an upstream "kernel.org" which hosts a repository with
submodules. Now you want to mirror that superproject to "kernel.mymirror.org"
and you start with

git clone --mirror git://kernel.org/superproject

When the superproject uses relative URLs for the submodules, this will
break your mirror,
if you did not mirror them exactly with the same relative path. Then
cloning from
your mirror will result in broken submodule URLs.

So you would want an additional mechanism for URLs. Jonathan came up
with the idea of having another configuration file in a refs/submodules/config
branch which essentially allows to annotate/enhance the .gitmodules file.

So in such a configuration you could say:

defaultRelativeURLBase = kernel.org
[submodule."foo"]
relativeURLBase = kernel.mymirror.org
protocol = ssh
# ssh as opposed to git:// which was inherited from the superproject

which allows finer controls of submodule/repository mirroring.
As the refs/submodule/configuration is not part of the history of the
superproject,
mirroring can be done without changing history, but still having URLs changed to
the internal mirror.

>
> I am not sure if labels will be that important in this case?

I am not sure. If it turns out to be a problem, maybe we can
introduce a hook, that will be called on adding new submodules via lables?

Thanks,
Stefan

>
> 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: [PATCH 05/15] submodule-config: check if submodule a submodule is in a group

[Junio C Hamano]

Junio C Hamano  writes:

> I see room for bikeshedding here, but the material to bikeshed
> around is not even documented yet ;-)
>
>  * a token prefixed with '*' is a label.
>  * a token prefixed with './' is a path.
>  * a token prefixed with ':' is a name.
>
> Hopefully I will see some description like that in later patches.
> I'll read on.

Extending this on a bit, I would suggest tweaking the above slightly
and make the rule more like this:

  * a token prefixed with '*' is a label.

  * a token prefixed with ':' is a name.

  * everything else is a path, but "./" at the front is skipped,
which can be used to disambiguate an unfortunate path that
begins with ':' or '*'.

A bigger thing I am wondering is if it is bettter to do _without_
adding a new --group=X option everywhere.  I am assuming that most
if not all submodule subcommands already use "module_list" aka
"submodule--helper list" that takes paths, and to them, extending
that interface to also understand the groups and names would be a
more natural way to extend the UI, no?  e.g.

$ git submodule update -- 'path1' 'path2'
$ git submodule update -- '*default'
$ git submodule update -- ':named'

instead of

$ git submodule update -- 'path1 'path2'
$ git submodule update --group='*default' --
$ git submodule update --group=':named' --

which special-cases the way to specify a set of submodules by
listing their paths.
--
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 05/15] submodule-config: check if submodule a submodule is in a group

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b6d4f27..23d7224 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -814,6 +814,46 @@ static int update_clone(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
> +int in_group(int argc, const char **argv, const char *prefix)

It is inconceivable that "submodule group" will be the only user of
the concept whose name is "group".  Please do not give such a
generic name to a helper function that is specific to "submodule
group" and make it global.  Naming a file-scope static helper
function as in_group() is perfectly fine; it is clear that such a
function in submodule--helper.c is about submodule group.

> + if (!group)
> + list = git_config_get_value_multi("submodule.defaultGroup");
> + else {
> + string_list_split(_list, group, ',', -1);
> + list = _list;

Hmm, where did this syntax to use comma-separated things come from?
Did I miss it in 02/15?

> + if (sub->labels) {
> + struct string_list_item *item;
> + for_each_string_list_item(item, sub->labels) {
> + strbuf_reset();
> + strbuf_addf(, "*%s", item->string);
> + if (string_list_has_string(group, sb.buf)) {
> + matched = 1;
> + break;
> + }
> + }
> + }
> + if (sub->path) {
> + /*
> +  * NEEDSWORK: This currently works only for
> +  * exact paths, but we want to enable
> +  * inexact matches such wildcards.
> +  */
> + strbuf_reset();
> + strbuf_addf(, "./%s", sub->path);
> + if (string_list_has_string(group, sb.buf))
> + matched = 1;
> + }
> + if (sub->name) {
> + /*
> +  * NEEDSWORK: Same as with path. Do we want to
> +  * support wildcards or such?
> +  */
> + strbuf_reset();
> + strbuf_addf(, ":%s", sub->name);
> + if (string_list_has_string(group, sb.buf))
> + matched = 1;
> + }
> + strbuf_release();

I see room for bikeshedding here, but the material to bikeshed
around is not even documented yet ;-)

 * a token prefixed with '*' is a label.
 * a token prefixed with './' is a path.
 * a token prefixed with ':' is a name.

Hopefully I will see some description like that in later patches.
I'll read on.

--
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 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Tue, Apr 26, 2016 at 11:20 AM, Stefan Beller  wrote:
> On Tue, Apr 26, 2016 at 11:05 AM, Junio C Hamano  wrote:
>> I think the way for you to indicate that desire expected by this
>> series is to use "git branch" to set upstream of new-shiny-feature
>> branch to origin/master.  Shouldn't that work, or is that too much
>> work?
>
> I can totally do that for longer series which require some back and forth.
>

So the submodule groups series is an example with some back and forth,
so I'll try to take that workflow with setting an upstream there for now.
As the groups stuff is based on origin/sb/submodule-init I set that as the
remote upstream branch. Upon checking that out I get:

Switched to branch 'submodule-groups'
Your branch is ahead of 'origin/sb/submodule-init' by 15 commits.
  (use "git push" to publish your local commits)

The first 2 lines are correct, the third however is not correct. (I cannot push
to your repository, but only email patches)

So I wonder if
 * I configured the wrong upstream branch
 * the upstream branch concept is extended to more/other use cases by the
   format.useAutoBase option. (In an email based workflow you would use the
   a remote branch to a remote, which is not owned by yourself, so the push
   advice is invalid from now on and we patch that message)
 * using an explicit upstream branch is the wrong approach here and the
   base should be implicit, i.e. Take the base sha1 and see if there is
   (one/any) remote branch matching that sha1.
   If there is, use the sha1 just fine.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/15] submodule add: label submodules if asked to

[Jacob Keller]

On Tue, Apr 26, 2016 at 1:50 PM, Stefan Beller  wrote:
> When adding new submodules, you can specify the
> label(s) the submodule belongs to by giving one or more
> --label arguments. This will record each label in the
> .gitmodules file as a value of the key
> "submodule.$NAME.label".
>

Ok so labels will be in the .gitmodules file. This means that if we go
back in history using git bisect or something similar, we'll
potentially change what the default submodules are for example?

This is sort of why having some submodule data appear in the
.git/config file is useful since it helps keep things like the remote
url safe from being updated when doing this sort of thing.

I am not sure if labels will be that important in this case?

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: [PATCH 03/15] submodule add: label submodules if asked to

[Junio C Hamano]

Stefan Beller  writes:

> When adding new submodules, you can specify the
> label(s) the submodule belongs to by giving one or more
> --label arguments. This will record each label in the
> .gitmodules file as a value of the key
> "submodule.$NAME.label".
>
> Signed-off-by: Stefan Beller 
> ---
>  Documentation/git-submodule.txt |  5 -
>  git-submodule.sh| 14 +-
>  t/t7400-submodule-basic.sh  | 32 
>  3 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 8c4bbe2..349ead8 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
>  SYNOPSIS
>  
>  [verse]
> -'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
> +'git submodule' [--quiet] add [-b ] [-f|--force] [-l|--label ]
> [--reference ] [--depth ] [--]  
> []
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
>  'git submodule' [--quiet] init [--] [...]
> @@ -109,6 +109,9 @@ is the superproject and submodule repositories will be 
> kept
>  together in the same relative location, and only the
>  superproject's URL needs to be provided: git-submodule will correctly
>  locate the submodule using the relative URL in .gitmodules.
> ++
> +If at least one label argument was given, all labels are recorded in the
> +.gitmodules file in the label fields.

I think this is better without "If ... given,".

I am not sure if it is sensible to make "label" namespace always
global to be shared with the project by updating .gitmodules,
though (it can cut both ways, so this is not an objection).

> @@ -165,6 +166,13 @@ cmd_add()
>   --depth=*)
>   depth=$1
>   ;;
> + -l|--label)
> + labels="${labels} $2"
> + shift
> + ;;
> + --label=*)
> + labels="${labels} ${1#--label=}"
> + ;;
>   --)
>   shift
>   break
> @@ -292,6 +300,10 @@ Use -f if you really want to add it." >&2
>  
>   git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
>   git config -f .gitmodules submodule."$sm_name".url "$repo" &&
> + for label in $labels
> + do
> + git config --add -f .gitmodules submodule."$sm_name".label 
> "${label}"
> + done &&

Is the acceptable syntax for "label" defined and documented
somewhere?

I didn't see it in the documentation patch.  I am seeing that we do
not allow $IFS whitespaces in them, but are there other restrictions
we want to enforce?  Remember, starting with narrow and widening as
we discover the need is the right way to design things.  Once we
start allowing random strings, it is very hard to later reject some
to carve out namespace for ourselves.

The above implementation happens to allow users to say

git submodule add -l "labelA labelB" -- $there $path

and give two labels to the module, and that will be something you
end up needing to support forever, unless you restrict early.
--
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/15] submodule doc: write down what we want to achieve in this series

[Junio C Hamano]

Stefan Beller  writes:

> +When operating on submodules you can either give paths to specify the
> +desired submodules or give no paths at all to apply the command to the
> +default group of submodules.

So, "git submodule foo path1 path2" would act on path1 and path2 but
would omit path3.  If you have path1 and path3 but not path2 in the
default group, and do not give any path, i.e. "git submodule foo",
it would act on path1 and path3 but would omit path2.

OK so far.

> +By default all submodules are included in
> +the default group.

So if you do not do anything special, i.e. do not define any group,
"git submodule foo" would act on path1, path2 and path3.

I think that is in line with the way how module_list aka
"submodule--helper list" works.

> +You can change the default group by configuring
> +submodule.defaultGroup. Once the default group is configured any
> +submodule operation without a specified set of submodules will use
> +the default group as the set to operate on.
> +
>  Submodules are composed from a so-called `gitlink` tree entry
>  in the main repository that refers to a particular commit object
>  within the inner repository that is completely separate.
--
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 01/15] string_list: add string_list_duplicate

[Junio C Hamano]

Stefan Beller  writes:

> The result of git_config_get_value_multi do not seem to be stable and
> may get overwritten. Have an easy way to preserve the results of that
> query.
>
> Signed-off-by: Stefan Beller 
> ---

This morning I reviewed a patch that adds something whose name ends
with _copy(), and this may want to follow suit.

Should strdup_strings() be a separate parameter, or should it follow
what src->strdup_strings has?

"Do not seem to be stable" does not build confidence in the code,
making it sound as if the developer is basing his work on guess not
analysis, and makes it hard to tell if this is a wrong workaround to
a valid issue (e.g. it could be "making the result stable" is what
we want in the longer term) or a valid solution to a problem that
would be common across callers of that API.

>  string-list.c | 18 ++
>  string-list.h |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/string-list.c b/string-list.c
> index 2a32a3f..f981c8a 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -7,6 +7,24 @@ void string_list_init(struct string_list *list, int 
> strdup_strings)
>   list->strdup_strings = strdup_strings;
>  }
>  
> +struct string_list *string_list_duplicate(const struct string_list *src,
> +   int strdup_strings)
> +{
> + struct string_list *list;
> + struct string_list_item *item;
> +
> + if (!src)
> + return NULL;
> +
> + list = xmalloc(sizeof(*list));
> + string_list_init(list, strdup_strings);
> + for_each_string_list_item(item, src)
> + string_list_append(list, item->string);
> +
> + return list;
> +}
> +
> +
>  /* if there is no exact match, point to the index where the entry could be
>   * inserted */
>  static int get_entry_index(const struct string_list *list, const char 
> *string,
> diff --git a/string-list.h b/string-list.h
> index d3809a1..1a5612f 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -19,6 +19,8 @@ struct string_list {
>  #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>  
>  void string_list_init(struct string_list *list, int strdup_strings);
> +struct string_list *string_list_duplicate(const struct string_list *src,
> +   int strdup_strings);
>  
>  void print_string_list(const struct string_list *p, const char *text);
>  void string_list_clear(struct string_list *list, int free_util);
--
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/15] submodule groups (once again)

[Junio C Hamano]

Stefan Beller  writes:

> What is this series about?
> ==
>
> If you have lots of submodules, you probably don't need all of them at once, 
> but you have functional units. Some submodules are absolutely required, 
> some are optional and only for very specific purposes. 
>
> This patch series adds labels to submodules in the .gitmodules file. 
>
> So you could have a .gitmodules file such as: 
>
> [submodule "gcc"] 
> path = gcc 
> url = git://... 
> label = default
> label = devel 
> [submodule "linux"] 
> path = linux 
> url = git://... 
> label = default 
> [submodule "nethack"] 
> path = nethack 
> url = git://... 
> label = optional
> label = games 
>
> and by this series you can work on an arbitrary group of these submodules
> composed by the labels, names or paths of the submodules.
>
> git clone --recurse-submodules --init-submodule=label 
> --init-submodule=label2   git://... 
> # will clone the superproject and recursively 
> # checkout any submodule being labeled label or label2
> 
> git submodule add --label  git://... ..
> # record a label while adding a submodule
> 
> git config submodule.defaultGroups default
> git config --add submodule.defaultGroups devel
> # configure which submodules you are interested in.
> 
> git submodule update
> # update only the submodules in the default group if that is configured.
> 
> git status
> git diff
> git submodule summary
> # show only changes to submodules which are in the default group.

Nicely designed.
--
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/15] submodule groups (once again)

[Junio C Hamano]

Stefan Beller  writes:

> git diff is supposed to view the differences between "what would I
> get after checkout" (i.e. what is in the index run through smudge filters)
> compared to the actual worktree.

I do not think it affects your conclusion, but the above is wrong.
"git diff" is a preview of what you would add (i.e. what will be in
the index after passing working tree contents via the clean filter)
relative to what is actually in the index.
--
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] Move test-* to t/helper/ subdirectory

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> This keeps top dir a bit less crowded. And because these programs are
> for testing purposes, it makes sense that they stay somewhere in t/
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  This patch will break any patches that add new test programs.
>  Luckily, none in 'next' or 'pu' does that. I know lmdb backend adds
>  test-lmdb-backend, so a manual move and some .gitignore fixup is
>  required there.

Can you (or somebody else) double check that the resulting Makefile
gets the build dependencies and exec path right?

I am seeing occasional failure from t0040 when checking out between
master and pu, and between the branches test-parse-options do change
behaviour which explains the breakage.

--
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] merge: warn --no-commit merge when no new commit is created

[Junio C Hamano]

Stefan Beller  writes:

> and later
>
> if (!option_commit)
> no_commit_impossible(_("Already up-to-date"));

It would be more legible, but because there are so few callsites in
an already shallow callchain, I do not think it makes that much of a
difference in this codepath either way.

>> +
>>  int cmd_merge(int argc, const char **argv, const char *prefix)
>>  {
>> unsigned char result_tree[20];
>> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char 
>> *prefix)
>>  * If head can reach all the merge then we are up to date.
>>  * but first the most common case of merging one remote.
>>  */
>> +   no_commit_impossible(_("Already up-to-date"));
>> finish_up_to_date("Already up-to-date.");
>
> Coming back to this patch, in case of -v given, we'll
> see ("Already up-to-date") twice?

One that explains why --no-commit is impossible in warning, and the
other is the final report of what happened, so yes.

> If --quiet is given, do we want to suppress output
> in no_commit_impossible?

While we are using warning(), we probably do want to.  When we
switch to die() at a major version boundary, we don't.
--
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] merge: warn --no-commit merge when no new commit is created

[Stefan Beller]

On Tue, Apr 26, 2016 at 2:37 PM, Junio C Hamano  wrote:
> +static void no_commit_impossible(const char *message)
> +{
> +   if (!option_commit) {
> +   warning("%s\n%s", _(message),
> +   _("--no-commit is impossible"));
> +   warning(_("In future versions of Git, this will become an 
> error."));
> +   }
> +}

During discussion of the parallel process framework
(sb/submodule-parallel-fetch~3),
you seemed very inclined on not having major decisions made deep
inside the helper
function, but rather at the main function to easier see the program flow IIRC.

This looks very similar to me as we'll have the no_commit_impossible function
which is a helper of cmd_merge. Following your advice there, I would
have expected to
have

static void no_commit_impossible(const char *message)
{
warning("%s\n%s", _(message), _("--no-commit is impossible"));
warning(_("In future versions of Git, this will become an error."));
}

and later

if (!option_commit)
no_commit_impossible(_("Already up-to-date"));


> +
>  int cmd_merge(int argc, const char **argv, const char *prefix)
>  {
> unsigned char result_tree[20];
> @@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char 
> *prefix)
>  * If head can reach all the merge then we are up to date.
>  * but first the most common case of merging one remote.
>  */
> +   no_commit_impossible(_("Already up-to-date"));
> finish_up_to_date("Already up-to-date.");

Coming back to this patch, in case of -v given, we'll
see ("Already up-to-date") twice?

If --quiet is given, do we want to suppress output
in no_commit_impossible?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> I think it's fair enough to say that if we had this facility this
> would be good enough:
>
>  * Your hooks are executed in glob() order, local .git first, then 
> /etc/git/...
>
>  * If it's a hook like pre-commit that can reject something the first
> hook to fail short-circuits. I.e. none of the rest get executed.
>
>  * If it's not a hook like that e.g. post-commit all of the hooks will
> get executed.
>
>  * If you need anything more complex you can just wrap your hooks in
> your own shellscript.
>
> I.e. it takes care of the common case where:
>
>  * You just want to execute N hooks and don't want to write a wrapper.
>
>  * For pre-* hooks the common case is it doesn't matter /what/
> rejected things, just that it gets rejected, e.g. for pre-receive.
> Also if you care about performance you can order them in
> cheapest-first order.

Stop using the word "common" to describe what is not demonstratably
"common".

The above only covers a very limited subset of the use cases, which
is the two bullet points above (one of them i.e. "I do not bother to
write a wrapper" is not even a valid use case).  That may be a good
starting point, but it is so simple that can be done with a wrapper
with several lines at most.  So I am not sympathetic to that line of
reasoning at all.

I can buy "It is too cumbersome to require everybody to reinvent and
script the cascading logic, and the core side should help by giving
a standard interface that is flexible enough to suit people's need",
though.

And I have to say that a sequential execution that always
short-circuits at the first failure is below that threshold.

One reason I care about allowing the users to specify "do not
shortcut" is that I anticipate that people would want to have a
logging of the result at the end of the chain.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] merge: warn --no-commit merge when no new commit is created

[Junio C Hamano]

A user who uses "--no-commit" does so with the intention to record a
resulting merge after amending the merge result in the working tree.
But there is nothing to amend and record, if the same "git merge"
without "--no-commit" wouldn't have created a merge commit (there
are two cases: (1) the other branch is a descendant of the current
branch, (2) the other branch is an ancestor of the current branch).

The user would want to know that before doing further damange to his
history.  When "merge --no-commit" fast-forwarded or succeeded with
"already up-to-date" or "fast-forward", give a warning message.

We may want to turn this into a die() after a transition period.

Signed-off-by: Junio C Hamano 
---

 * The necessary update to avoid end-user mistake would look like
   this.  I am not queuing this or further working on it myself,
   as I am not sure if it is all that useful.

 builtin/merge.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index f641751..66c536d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1157,6 +1157,15 @@ static struct commit_list *collect_parents(struct commit 
*head_commit,
return remoteheads;
 }
 
+static void no_commit_impossible(const char *message)
+{
+   if (!option_commit) {
+   warning("%s\n%s", _(message),
+   _("--no-commit is impossible"));
+   warning(_("In future versions of Git, this will become an 
error."));
+   }
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
unsigned char result_tree[20];
@@ -1403,6 +1412,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * If head can reach all the merge then we are up to date.
 * but first the most common case of merging one remote.
 */
+   no_commit_impossible(_("Already up-to-date"));
finish_up_to_date("Already up-to-date.");
goto done;
} else if (fast_forward != FF_NO && !remoteheads->next &&
@@ -1412,6 +1422,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
struct strbuf msg = STRBUF_INIT;
struct commit *commit;
 
+   no_commit_impossible(_("Fast-forward"));
if (verbosity >= 0) {
char from[GIT_SHA1_HEXSZ + 1], to[GIT_SHA1_HEXSZ + 1];
find_unique_abbrev_r(from, head_commit->object.oid.hash,
@@ -1488,6 +1499,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
}
}
if (up_to_date) {
+   no_commit_impossible(_("Already up-to-date"));
finish_up_to_date("Already up-to-date. Yeeah!");
goto done;
}
-- 
2.8.1-491-g88b9e4a

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] merge: do not contaminate option_commit with --squash

[Junio C Hamano]

It is true that we do not make a commit when we are asked to do
"merge --squash", and the code does so by setting option_commit
variable to zero when seeing the squash option.  But this made it
impossible to see from the value of option_commit if --no-commit was
given from the command line, or --squash turned it off.

We check for the value of option_commit at only two places.  Check
for the value of squash at them, too.

Signed-off-by: Junio C Hamano 
---

 * Just a preliminary clean-up for the next one which is on topic.

 builtin/merge.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index bf2f261..de9d1b6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1237,11 +1237,8 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
if (verbosity < 0)
show_diffstat = 0;
 
-   if (squash) {
-   if (fast_forward == FF_NO)
-   die(_("You cannot combine --squash with --no-ff."));
-   option_commit = 0;
-   }
+   if (squash && fast_forward == FF_NO)
+   die(_("You cannot combine --squash with --no-ff."));
 
if (!argc) {
if (default_to_upstream)
@@ -1449,10 +1446,10 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
 * We are not doing octopus and not fast-forward.  Need
 * a real merge.
 */
-   else if (!remoteheads->next && !common->next && option_commit) {
+   else if (!remoteheads->next && !common->next && option_commit && 
!squash)
/*
 * We are not doing octopus, not fast-forward, and have
-* only one common.
+* only one common.  And we do want to create a new commit.
 */
refresh_cache(REFRESH_QUIET);
if (allow_trivial && fast_forward != FF_ONLY) {
@@ -1535,7 +1532,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
ret = try_merge_strategy(use_strategies[i]->name,
 common, remoteheads,
 head_commit, head_arg);
-   if (!option_commit && !ret) {
+   if ((!option_commit || squash) && !ret) {
merge_was_ok = 1;
/*
 * This is necessary here just to avoid writing
-- 
2.8.1-491-g88b9e4a

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Marc Branchaud]

On 2016-04-26 12:09 PM, Ævar Arnfjörð Bjarmason wrote:


Basically you can look at patches to a project on a spectrum of:

  1. You hacked something up locally
  2. It's in someone's *.git repo as a POC
  3. It's a third-party patch series used by a bunch of people
  4. In an official release but documented as experimental
  5. In an official release as a first-rate feature

Something like an experimental.WHATEVER=bool flag provides #4.


But the git project already does #4 without needing a special 
configuration tree.  In fact, you ignored the git project's "pu" branch 
entirely.  Once a feature gets onto the "next" branch, it's already much 
less experimental.  If it needs to put the word "experimental" in its 
config settings, then maybe shouldn't leave the "pu" branch in the first 
place.



I think aside from this feature just leaving these things undocumented
really provides the worst of both worlds.


Yes, I apologize.  I did not mean that things should remain 
undocumented, only that if you're afraid of users harming themselves 
then you're better off not documenting settings than labelling them as 
experimental.



Now you have some feature that's undocumented *because* you're not
sure about it, but you can't ever be sure about it unless people
actually use it, and if it's not documented at all effectively it's as
visible as some third-party patch series. I.e. only people really
involved in the project will ever use it.


Slapping "experimental" on the configuration only serves to muddy the 
waters.  Either the feature is good enough to be tried by normal users, 
or it isn't.  If it isn't ready for normal users, let it cook in pu (or 
next) for a while longer.


Git has got on just fine without having some special designation for 
not-ready-for-prime-time features, mostly because the development 
process lends itself naturally to gradually exposing things as they 
mature: topics move from the list to pu to next to master.  (The string 
"experiment" only appears 16 times in all the release notes, which I 
think is something the git project can be proud of.)


To me, designating part of the config tree as "experimental" enables 
sloppier practices, where a feature can be released with a bit less 
effort than might otherwise be acceptable, because it's clearly marked 
as experimental, and so anyone trying it out surely has the requisite 
bulletproof footwear.  (I don't mean to imply that you or any other git 
contributor might slack off on any work you do for the project. It's 
more that the ability to easily designate something as experimental 
lowers the bar a bit, and makes it more tempting to release 
not-quite-ready features.)


Far better to instead work on the feature until it's as ready as can be, 
and release it properly.


In this particular case, for example, I think your proposed approach is 
perfectly fine and does not need to be designated as "experimental" in 
any way.  With a reasonable "hooks.something" config variable to turn on 
directory support, what you've described sounds like a great feature. 
Sure, it may have bugs, it may have unintended consequences, it may not 
fulfill some odd corner cases.  But that's true for almost everything. 
As with every other git feature, it'll be developed to the best of the 
project's abilities and released.  Future patches are welcome.


M.

--
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 09/15] submodule--helper init: respect submodule groups

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c  | 19 +--
 t/t7413-submodule--helper.sh | 15 +++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index adb6188..29a345e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -405,6 +405,7 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
 {
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
+   struct string_list *group = NULL;
int quiet = 0;
int i;
 
@@ -427,8 +428,22 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
if (module_list_compute(argc, argv, prefix, , ) < 0)
return 1;
 
-   for (i = 0; i < list.nr; i++)
-   init_submodule(list.entries[i]->name, prefix, quiet);
+   if (!pathspec.nr)
+   group = string_list_duplicate(
+   git_config_get_value_multi("submodule.defaultGroup"), 
1);
+   if (group) {
+   gitmodules_config();
+   for (i = 0; i < list.nr; i++) {
+   const struct submodule *sub =
+   submodule_from_path(null_sha1,
+   list.entries[i]->name);
+   if (submodule_in_group(group, sub))
+   init_submodule(list.entries[i]->name, prefix, 
quiet);
+   }
+   string_list_clear(group, 1);
+   } else
+   for (i = 0; i < list.nr; i++)
+   init_submodule(list.entries[i]->name, prefix, quiet);
 
return 0;
 }
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index 1b5d135..ef12c63 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -175,4 +175,19 @@ test_expect_success 'submodule sync respects groups' '
)
 '
 
+test_expect_success 'submodule--helper init respects groups' '
+   (
+   cd super_clone &&
+   git submodule deinit . &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git submodule init &&
+   git config --unset-all submodule.defaultGroup &&
+   test "$(git config submodule.sub0.url)" = "$suburl" &&
+   test "$(git config submodule.sub1.url)" = "$suburl" &&
+   test_must_fail git config submodule.sub2.url &&
+   test "$(git config submodule.sub3.url)" = "$suburl"
+   )
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

--
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 15/15] clone: allow specification of submodules to be cloned

[Stefan Beller]

This is in line with clone being the contraction of
mkdir  && cd 
git init
git config
git fetch
git submodule update

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt |  6 +++
 builtin/clone.c | 40 +--
 t/t7400-submodule-basic.sh  | 96 +
 3 files changed, 139 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 45d74be..38b1948 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -212,6 +212,12 @@ objects from the source repository into a pack in the 
cloned repository.
repository does not have a worktree/checkout (i.e. if any of
`--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
 
+--init-submodule::
+   After the repository is cloned, specified submodules are cloned.
+   It is possible to give multiple specifications by repeating the
+   argument. This option will be recorded in the repository config
+   as `submodule.defaultGroup`.
+
 --separate-git-dir=::
Instead of placing the cloned repository where it is supposed
to be, place the cloned repository at the specified directory,
diff --git a/builtin/clone.c b/builtin/clone.c
index 6576ecf..8371bc2 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -52,6 +52,22 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 static int max_jobs = -1;
+static struct string_list init_submodules;
+
+static int init_submodules_cb(const struct option *opt, const char *arg, int 
unset)
+{
+   struct string_list_item *item;
+   struct string_list sl = STRING_LIST_INIT_DUP;
+
+   if (unset)
+   return -1;
+
+   string_list_split(, arg, ',', -1);
+   for_each_string_list_item(item, )
+   string_list_append((struct string_list *)opt->value, 
item->string);
+
+   return 0;
+}
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
@@ -100,6 +116,8 @@ static struct option builtin_clone_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+   OPT_CALLBACK(0, "init-submodule", _submodules, N_("string"),
+   N_("clone specific submodules"), init_submodules_cb),
OPT_END()
 };
 
@@ -731,17 +749,24 @@ 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) {
+   if (err)
+   goto out;
+
+   if (option_recursive || init_submodules.nr > 0) {
struct argv_array args = ARGV_ARRAY_INIT;
-   argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
+   argv_array_pushl(, "submodule", "update", NULL);
 
+   if (option_recursive) {
+   argv_array_pushf(, "--init");
+   argv_array_pushf(, "--recursive");
+   }
if (max_jobs != -1)
argv_array_pushf(, "--jobs=%d", max_jobs);
 
err = run_command_v_opt(args.argv, RUN_GIT_CMD);
argv_array_clear();
}
-
+out:
return err;
 }
 
@@ -876,6 +901,15 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
option_no_checkout = 1;
}
 
+   if (init_submodules.nr > 0) {
+   struct string_list_item *item;
+   struct strbuf sb = STRBUF_INIT;
+   for_each_string_list_item(item, _submodules) {
+   strbuf_addf(, "submodule.defaultGroup=%s", 
item->string);
+   string_list_append(_config, strbuf_detach(, 
0));
+   }
+   }
+
if (!option_origin)
option_origin = "origin";
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ac477b2..1fd313b 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1110,4 +1110,100 @@ test_expect_success 'submodule add records multiple 
labels' '
test_cmp expected actual
 '
 
+cat < expected
+submodule
+EOF
+
+test_expect_success 'clone --init-submodule works' '
+   test_when_finished "rm -rf super super_clone" &&
+   mkdir super &&
+   pwd=$(pwd) &&
+   (
+   cd super &&
+   git init &&
+   git submodule add --label labelA file://"$pwd"/example2 
submodule &&
+   git submodule add file://"$pwd"/example2 submodule1 &&
+   git commit -a -m "create repository with 2 submodules, one is 
in a group"
+   ) &&
+   git clone --recurse-submodules --init-submodule \*labelA super 
super_clone &&
+   (
+   cd super_clone &&
+  

[PATCH 12/15] git submodule summary respects groups

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 git-submodule.sh |  5 +
 t/t7413-submodule--helper.sh | 26 ++
 2 files changed, 31 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 253ad07..f065b1f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -833,6 +833,11 @@ cmd_summary() {
sane_egrep '^:([0-7]* )?16' |
while read mod_src mod_dst sha1_src sha1_dst status sm_path
do
+   # ignore modules not in group
+   if ! git submodule--helper in-group $sm_path
+   then
+   continue
+   fi
# Always show modules deleted or type-changed 
(blob<->module)
if test "$status" = D || test "$status" = T
then
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index 39e469f..d01cdc6 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -226,4 +226,30 @@ test_expect_success 'git submodule update respects groups' 
'
test_cmp expect actual
 '
 
+range_back="$(echo $submodule_sha1|cut -c1-7)...$(echo $sub_priorsha1|cut 
-c1-7)"
+cat >expect <<-EOF
+* sub0 $range_back (1):
+  < test2
+
+* sub1 $range_back (1):
+  < test2
+
+* sub3 $range_back (1):
+  < test2
+
+EOF
+
+test_expect_success 'git submodule summary respects groups' '
+   (
+   cd super_clone &&
+   git submodule update --init &&
+   git submodule foreach "git checkout HEAD^" &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git submodule summary >../actual &&
+   git config --unset-all submodule.defaultGroup
+   ) &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

--
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 13/15] cmd_status: respect submodule groups

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/commit.c |  3 +++
 t/t7413-submodule--helper.sh | 15 +++
 wt-status.c  |  2 ++
 wt-status.h  |  1 +
 4 files changed, 21 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..d29134d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1369,6 +1369,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
s.ignore_submodule_arg = ignore_submodule_arg;
+   s.submodule_groups = string_list_duplicate(
+   git_config_get_value_multi("submodule.defaultGroup"), 1);
+
wt_status_collect();
 
if (0 <= fd)
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index d01cdc6..a3dbfea 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -252,4 +252,19 @@ test_expect_success 'git submodule summary respects 
groups' '
test_cmp expect actual
 '
 
+test_expect_success 'git status respects groups' '
+   # use setup from previous test
+   (
+   cd super_clone &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git status >../actual
+   git config --unset-all submodule.defaultGroup
+   ) &&
+   test_i18ngrep "modified:   sub0" actual &&
+   test_i18ngrep "modified:   sub1" actual &&
+   test_i18ngrep ! "modified:   sub2" actual &&
+   test_i18ngrep "modified:   sub3" actual
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index ef74864..0d494ac 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -502,6 +502,7 @@ static void wt_status_collect_changes_worktree(struct 
wt_status *s)
DIFF_OPT_SET(, OVERRIDE_SUBMODULE_CONFIG);
handle_ignore_submodules_arg(, 
s->ignore_submodule_arg);
}
+   rev.diffopt.submodule_groups = s->submodule_groups;
rev.diffopt.format_callback = wt_status_collect_changed_cb;
rev.diffopt.format_callback_data = s;
copy_pathspec(_data, >pathspec);
@@ -532,6 +533,7 @@ static void wt_status_collect_changes_index(struct 
wt_status *s)
 */
handle_ignore_submodules_arg(, "dirty");
}
+   rev.diffopt.submodule_groups = s->submodule_groups;
 
rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = wt_status_collect_updated_cb;
diff --git a/wt-status.h b/wt-status.h
index c9b3b74..d66a2b5 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -73,6 +73,7 @@ struct wt_status {
struct string_list change;
struct string_list untracked;
struct string_list ignored;
+   struct string_list *submodule_groups;
uint32_t untracked_in_ms;
 };
 
-- 
2.8.0.41.g8d9aeb3

--
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 14/15] cmd_diff: respect submodule groups

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/diff.c   |  2 ++
 t/t7413-submodule--helper.sh | 15 +++
 2 files changed, 17 insertions(+)

diff --git a/builtin/diff.c b/builtin/diff.c
index 52c98a9..1c6abd5 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -366,6 +366,8 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
 
setup_diff_pager();
 
+   rev.diffopt.submodule_groups = string_list_duplicate(
+   git_config_get_value_multi("submodule.defaultGroup"), 1);
/*
 * Do we have --cached and not have a pending object, then
 * default to HEAD by hand.  Eek.
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index a3dbfea..1ca7878 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -267,4 +267,19 @@ test_expect_success 'git status respects groups' '
test_i18ngrep "modified:   sub3" actual
 '
 
+test_expect_success 'git diff respects groups' '
+   # use setup from previous test
+   (
+   cd super_clone &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git diff >../actual
+   git config --unset-all submodule.defaultGroup
+   ) &&
+   test_i18ngrep "diff --git a/sub0 b/sub0" actual &&
+   test_i18ngrep "diff --git a/sub1 b/sub1" actual &&
+   test_i18ngrep ! "diff --git a/sub2 b/sub2" actual &&
+   test_i18ngrep "diff --git a/sub3 b/sub3" actual
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

--
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 08/15] submodule--helper list: respect submodule groups

[Stefan Beller]

As submodule--helper list is the building block for some submodule
commands (foreach, deinit, status, sync), also add tests for those.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c  | 13 ++
 t/t7413-submodule--helper.sh | 97 
 2 files changed, 110 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7b9a4d7..adb6188 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -271,6 +271,7 @@ static int module_list(int argc, const char **argv, const 
char *prefix)
int i;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
+   const struct string_list *group = NULL;
 
struct option module_list_options[] = {
OPT_STRING(0, "prefix", ,
@@ -292,9 +293,21 @@ static int module_list(int argc, const char **argv, const 
char *prefix)
return 1;
}
 
+   if (!pathspec.nr)
+   group = git_config_get_value_multi("submodule.defaultGroup");
+   if (group)
+   gitmodules_config();
+
for (i = 0; i < list.nr; i++) {
const struct cache_entry *ce = list.entries[i];
 
+   if (group) {
+   const struct submodule *sub =
+   submodule_from_path(null_sha1, ce->name);
+   if (!submodule_in_group(group, sub))
+   continue;
+   }
+
if (ce_stage(ce))
printf("%06o %s U\t", ce->ce_mode, 
sha1_to_hex(null_sha1));
else
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index c6939ab..1b5d135 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -78,4 +78,101 @@ test_expect_success 'in-group' '
)
 '
 
+submodule_sha1=$(git -C sub rev-parse HEAD)
+
+cat >expect <<-EOF
+16 $submodule_sha1 0   sub0
+16 $submodule_sha1 0   sub1
+16 $submodule_sha1 0   sub3
+EOF
+
+test_expect_success 'submodule--helper list respects groups' '
+   (
+   cd super &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git submodule--helper list >../actual
+   ) &&
+   test_cmp expect actual
+'
+
+cat >expect <<-EOF
+Entering 'sub0'
+$submodule_sha1 sub0
+Entering 'sub1'
+$submodule_sha1 sub1
+Entering 'sub3'
+$submodule_sha1 sub3
+EOF
+
+test_expect_success 'submodule foreach respects groups' '
+   (
+   cd super &&
+   git submodule foreach "echo \$sha1 \$name" >../actual
+   ) &&
+   test_cmp expect actual
+'
+
+sub_priorsha1=$(git -C sub rev-parse HEAD^)
+
+cat >expect <<-EOF
++$sub_priorsha1 sub0 (test)
++$sub_priorsha1 sub1 (test)
++$sub_priorsha1 sub3 (test)
+EOF
+
+test_expect_success 'submodule status respects groups' '
+   git clone --recurse-submodules super super_clone &&
+   (
+   cd super_clone &&
+   git -C sub0 checkout HEAD^ &&
+   git -C sub1 checkout HEAD^ &&
+   git -C sub2 checkout HEAD^ &&
+   git -C sub3 checkout HEAD^ &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git submodule status >../actual &&
+   git config --unset-all submodule.defaultGroup &&
+   git submodule update
+   ) &&
+   test_cmp expect actual
+'
+
+test_expect_success 'submodule deinit respects groups' '
+   suburl=$(pwd)/sub &&
+   (
+   cd super_clone &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git submodule deinit &&
+   test_must_fail git config submodule.sub0.url &&
+   test_must_fail git config submodule.sub1.url &&
+   test "$(git config submodule.sub2.url)" = "$suburl" &&
+   test_must_fail git config submodule.sub3.url &&
+   git config --unset-all submodule.defaultGroup &&
+   git submodule init
+   )
+'
+
+test_expect_success 'submodule sync respects groups' '
+   suburl=$(pwd)/sub &&
+   (
+   cd super_clone &&
+   git config submodule.sub0.url nonsense &&
+   git config submodule.sub1.url nonsense &&
+   git config submodule.sub2.url nonsense &&
+   git config submodule.sub3.url nonsense &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git submodule sync &&
+   git config --unset-all submodule.defaultGroup &&
+   test "$(git config submodule.sub0.url)" = "$suburl" &&
+   

[PATCH 04/15] submodule-config: keep labels around

[Stefan Beller]

We need the submodule labels in a later patch.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 16 
 submodule-config.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index b82d1fb..0cdb47e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -60,6 +60,10 @@ 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);
+   if (entry->config->labels) {
+   string_list_clear(entry->config->labels, 0);
+   free(entry->config->labels);
+   }
free(entry->config);
 }
 
@@ -199,6 +203,7 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
submodule->update_strategy.command = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
+   submodule->labels = NULL;
 
hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
@@ -353,6 +358,17 @@ static int parse_config(const char *var, const char 
*value, void *data)
else if (parse_submodule_update_strategy(value,
 >update_strategy) < 0)
die(_("invalid value for %s"), var);
+   } else if (!strcmp(item.buf, "label")) {
+   if (!value)
+   ret = config_error_nonbool(var);
+   else {
+   if (!submodule->labels) {
+   submodule->labels =
+   xmalloc(sizeof(*submodule->labels));
+   string_list_init(submodule->labels, 1);
+   }
+   string_list_insert(submodule->labels, value);
+   }
}
 
strbuf_release();
diff --git a/submodule-config.h b/submodule-config.h
index e4857f5..d57da59 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -18,6 +18,8 @@ struct submodule {
struct submodule_update_strategy update_strategy;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
+   /* sorted, not as on disk */
+   struct string_list *labels;
 };
 
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-- 
2.8.0.41.g8d9aeb3

--
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 10/15] submodule--helper update_clone: respect submodule groups

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c  | 16 
 t/t7413-submodule--helper.sh | 36 
 2 files changed, 52 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 29a345e..aa838c5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -610,6 +610,8 @@ struct submodule_update_clone {
 
/* Machine-readable status lines to be consumed by git-submodule.sh */
struct string_list projectlines;
+   /* The group specification we'll be processing. */
+   struct string_list *group;
 
/* If we want to stop as fast as possible and return an error */
unsigned quickstop : 1;
@@ -646,6 +648,9 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
 
sub = submodule_from_path(null_sha1, ce->name);
 
+   if (!submodule_in_group(suc->group, sub))
+   goto cleanup;
+
if (suc->recursive_prefix)
displaypath = relative_path(suc->recursive_prefix,
ce->name, _sb);
@@ -771,6 +776,7 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
struct string_list_item *item;
struct pathspec pathspec;
struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
+   struct string_list actual_group = STRING_LIST_INIT_DUP;
 
struct option module_update_clone_options[] = {
OPT_STRING(0, "prefix", ,
@@ -810,6 +816,16 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
if (module_list_compute(argc, argv, prefix, , ) < 0)
return 1;
 
+   if (!pathspec.nr) {
+   const struct string_list *group =
+   group = 
git_config_get_value_multi("submodule.defaultGroup");
+   if (group) {
+   for_each_string_list_item(item, group)
+   string_list_append(_group, item->string);
+   suc.group = _group;
+   }
+   }
+
if (pathspec.nr)
suc.warn_if_uninitialized = 1;
 
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
index ef12c63..39e469f 100755
--- a/t/t7413-submodule--helper.sh
+++ b/t/t7413-submodule--helper.sh
@@ -190,4 +190,40 @@ test_expect_success 'submodule--helper init respects 
groups' '
)
 '
 
+cat >expect <<-EOF
+16 $submodule_sha1 0 1 sub0
+16 $submodule_sha1 0 1 sub1
+16 $submodule_sha1 0 1 sub3
+EOF
+
+test_expect_success 'submodule--helper update-clone respects groups' '
+   (
+   cd super_clone &&
+   git submodule init &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git submodule--helper update-clone >../actual &&
+   git config --unset-all submodule.defaultGroup
+   ) &&
+   test_cmp expect actual
+'
+
+cat >expect <<-EOF
+Submodule path 'sub0': checked out '$submodule_sha1'
+Submodule path 'sub1': checked out '$submodule_sha1'
+Submodule path 'sub3': checked out '$submodule_sha1'
+EOF
+
+test_expect_success 'git submodule update respects groups' '
+   (
+   cd super_clone &&
+   git submodule deinit -f . &&
+   git config --add submodule.defaultGroup *bit1 &&
+   git config --add submodule.defaultGroup ./sub0 &&
+   git submodule update --init >../actual &&
+   git config --unset-all submodule.defaultGroup
+   ) &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.8.0.41.g8d9aeb3

--
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 11/15] diff: ignore submodules excluded by groups

[Stefan Beller]

We do not need to do anything special to initialize the `submodule_groups`
pointer as the diff options setup will fill in 0 by default.

Signed-off-by: Stefan Beller 
---
 diff.c | 3 +++
 diff.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/diff.c b/diff.c
index 059123c..5808d8a 100644
--- a/diff.c
+++ b/diff.c
@@ -4921,10 +4921,13 @@ static int is_submodule_ignored(const char *path, 
struct diff_options *options)
 {
int ignored = 0;
unsigned orig_flags = options->flags;
+   const struct submodule *sub = submodule_from_path(null_sha1, path);
if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))
set_diffopt_flags_from_submodule_config(options, path);
if (DIFF_OPT_TST(options, IGNORE_SUBMODULES))
ignored = 1;
+   if (!submodule_in_group(options->submodule_groups, sub))
+   ignored = 1;
options->flags = orig_flags;
return ignored;
 }
diff --git a/diff.h b/diff.h
index e7d68ed..7d499fb 100644
--- a/diff.h
+++ b/diff.h
@@ -178,6 +178,7 @@ struct diff_options {
void *output_prefix_data;
 
int diff_path_counter;
+   struct string_list *submodule_groups;
 };
 
 enum color_diff {
-- 
2.8.0.41.g8d9aeb3

--
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 05/15] submodule-config: check if submodule a submodule is in a group

[Stefan Beller]

In later patches we need to tell if a submodule is in a group,
which is defined by name, path or labels.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c  | 43 ++-
 submodule-config.c   | 50 +++
 submodule-config.h   |  3 ++
 t/t7413-submodule--helper.sh | 81 
 4 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100755 t/t7413-submodule--helper.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b6d4f27..23d7224 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -814,6 +814,46 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+int in_group(int argc, const char **argv, const char *prefix)
+{
+   const struct string_list *list;
+   struct string_list actual_list = STRING_LIST_INIT_DUP;
+   const struct submodule *sub;
+   const char *group = NULL;
+
+   struct option default_group_options[] = {
+   OPT_STRING('g', "group", , N_("group"),
+   N_("group specifier for submodules")),
+   OPT_END()
+   };
+
+   const char *const git_submodule_helper_usage[] = {
+   N_("git submodule--helper in-group "),
+   NULL
+   };
+
+   argc = parse_options(argc, argv, prefix, default_group_options,
+git_submodule_helper_usage, 0);
+
+   /* Overlay the parsed .gitmodules file with .git/config */
+   gitmodules_config();
+   git_config(submodule_config, NULL);
+
+   if (argc != 1)
+   usage(git_submodule_helper_usage[0]);
+
+   sub = submodule_from_path(null_sha1, argv[0]);
+
+   if (!group)
+   list = git_config_get_value_multi("submodule.defaultGroup");
+   else {
+   string_list_split(_list, group, ',', -1);
+   list = _list;
+   }
+
+   return !submodule_in_group(list, sub);
+}
+
 struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
@@ -826,7 +866,8 @@ static struct cmd_struct commands[] = {
{"update-clone", update_clone},
{"resolve-relative-url", resolve_relative_url},
{"resolve-relative-url-test", resolve_relative_url_test},
-   {"init", module_init}
+   {"init", module_init},
+   {"in-group", in_group}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/submodule-config.c b/submodule-config.c
index 0cdb47e..ebed0f2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -522,3 +522,53 @@ void submodule_free(void)
cache_free();
is_cache_init = 0;
 }
+
+int submodule_in_group(const struct string_list *group,
+  const struct submodule *sub)
+{
+   int matched = 0;
+   struct strbuf sb = STRBUF_INIT;
+
+   if (!group)
+   /*
+* If no group is specified all, all submodules match to
+* keep traditional behavior
+*/
+   return 1;
+
+   if (sub->labels) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, sub->labels) {
+   strbuf_reset();
+   strbuf_addf(, "*%s", item->string);
+   if (string_list_has_string(group, sb.buf)) {
+   matched = 1;
+   break;
+   }
+   }
+   }
+   if (sub->path) {
+   /*
+* NEEDSWORK: This currently works only for
+* exact paths, but we want to enable
+* inexact matches such wildcards.
+*/
+   strbuf_reset();
+   strbuf_addf(, "./%s", sub->path);
+   if (string_list_has_string(group, sb.buf))
+   matched = 1;
+   }
+   if (sub->name) {
+   /*
+* NEEDSWORK: Same as with path. Do we want to
+* support wildcards or such?
+*/
+   strbuf_reset();
+   strbuf_addf(, ":%s", sub->name);
+   if (string_list_has_string(group, sb.buf))
+   matched = 1;
+   }
+   strbuf_release();
+
+   return matched;
+}
diff --git a/submodule-config.h b/submodule-config.h
index d57da59..4c696cc 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -31,4 +31,7 @@ const struct submodule *submodule_from_path(const unsigned 
char *commit_sha1,
const char *path);
 void submodule_free(void);
 
+int submodule_in_group(const struct string_list *group,
+  const struct submodule *sub);
+
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/t/t7413-submodule--helper.sh b/t/t7413-submodule--helper.sh
new file mode 100755

[PATCH 07/15] submodule deinit: loose requirement for giving '.'

[Stefan Beller]

This is needed later to make a distinction between 'all specified'
and the default group of submodules.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh   | 5 -
 t/t7400-submodule-basic.sh | 1 -
 2 files changed, 6 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index d7a5e1a..253ad07 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -440,11 +440,6 @@ cmd_deinit()
shift
done
 
-   if test $# = 0
-   then
-   die "$(eval_gettext "Use '.' if you really want to deinitialize 
all submodules")"
-   fi
-
git submodule--helper list --prefix "$wt_prefix" "$@" |
while read mode sha1 stage sm_path
do
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index e9d1d58..ac477b2 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -932,7 +932,6 @@ test_expect_success 'submodule deinit . deinits all 
initialized submodules' '
git submodule update --init &&
git config submodule.example.foo bar &&
git config submodule.example2.frotz nitfol &&
-   test_must_fail git submodule deinit &&
git submodule deinit . >actual &&
test -z "$(git config --get-regexp "submodule\.example\.")" &&
test -z "$(git config --get-regexp "submodule\.example2\.")" &&
-- 
2.8.0.41.g8d9aeb3

--
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 01/15] string_list: add string_list_duplicate

[Stefan Beller]

The result of git_config_get_value_multi do not seem to be stable and
may get overwritten. Have an easy way to preserve the results of that
query.

Signed-off-by: Stefan Beller 
---
 string-list.c | 18 ++
 string-list.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/string-list.c b/string-list.c
index 2a32a3f..f981c8a 100644
--- a/string-list.c
+++ b/string-list.c
@@ -7,6 +7,24 @@ void string_list_init(struct string_list *list, int 
strdup_strings)
list->strdup_strings = strdup_strings;
 }
 
+struct string_list *string_list_duplicate(const struct string_list *src,
+ int strdup_strings)
+{
+   struct string_list *list;
+   struct string_list_item *item;
+
+   if (!src)
+   return NULL;
+
+   list = xmalloc(sizeof(*list));
+   string_list_init(list, strdup_strings);
+   for_each_string_list_item(item, src)
+   string_list_append(list, item->string);
+
+   return list;
+}
+
+
 /* if there is no exact match, point to the index where the entry could be
  * inserted */
 static int get_entry_index(const struct string_list *list, const char *string,
diff --git a/string-list.h b/string-list.h
index d3809a1..1a5612f 100644
--- a/string-list.h
+++ b/string-list.h
@@ -19,6 +19,8 @@ struct string_list {
 #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
 
 void string_list_init(struct string_list *list, int strdup_strings);
+struct string_list *string_list_duplicate(const struct string_list *src,
+ int strdup_strings);
 
 void print_string_list(const struct string_list *p, const char *text);
 void string_list_clear(struct string_list *list, int free_util);
-- 
2.8.0.41.g8d9aeb3

--
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 03/15] submodule add: label submodules if asked to

[Stefan Beller]

When adding new submodules, you can specify the
label(s) the submodule belongs to by giving one or more
--label arguments. This will record each label in the
.gitmodules file as a value of the key
"submodule.$NAME.label".

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt |  5 -
 git-submodule.sh| 14 +-
 t/t7400-submodule-basic.sh  | 32 
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 8c4bbe2..349ead8 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
 SYNOPSIS
 
 [verse]
-'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
+'git submodule' [--quiet] add [-b ] [-f|--force] [-l|--label ]
  [--reference ] [--depth ] [--]  
[]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
 'git submodule' [--quiet] init [--] [...]
@@ -109,6 +109,9 @@ is the superproject and submodule repositories will be kept
 together in the same relative location, and only the
 superproject's URL needs to be provided: git-submodule will correctly
 locate the submodule using the relative URL in .gitmodules.
++
+If at least one label argument was given, all labels are recorded in the
+.gitmodules file in the label fields.
 
 status::
Show the status of the submodules. This will print the SHA-1 of the
diff --git a/git-submodule.sh b/git-submodule.sh
index 82e95a9..d7a5e1a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,7 +5,7 @@
 # Copyright (c) 2007 Lars Hjemli
 
 dashless=$(basename "$0" | sed -e 's/-/ /')
-USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
] [--]  []
+USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
] [-l|--label ][--]  []
or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] [--] ...
@@ -130,6 +130,7 @@ cmd_add()
 {
# parse $args after "submodule ... add".
reference_path=
+   labels=
while test $# -ne 0
do
case "$1" in
@@ -165,6 +166,13 @@ cmd_add()
--depth=*)
depth=$1
;;
+   -l|--label)
+   labels="${labels} $2"
+   shift
+   ;;
+   --label=*)
+   labels="${labels} ${1#--label=}"
+   ;;
--)
shift
break
@@ -292,6 +300,10 @@ Use -f if you really want to add it." >&2
 
git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
git config -f .gitmodules submodule."$sm_name".url "$repo" &&
+   for label in $labels
+   do
+   git config --add -f .gitmodules submodule."$sm_name".label 
"${label}"
+   done &&
if test -n "$branch"
then
git config -f .gitmodules submodule."$sm_name".branch "$branch"
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index f99f674..e9d1d58 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1040,6 +1040,7 @@ test_expect_success 'submodule with UTF-8 name' '
 '
 
 test_expect_success 'submodule add clone shallow submodule' '
+   test_when_finished "rm -rf super" &&
mkdir super &&
pwd=$(pwd) &&
(
@@ -1078,5 +1079,36 @@ test_expect_success 'submodule helper list is not 
confused by common prefixes' '
test_cmp expect actual
 '
 
+test_expect_success 'submodule add records a label' '
+   test_when_finished "rm -rf super" &&
+   mkdir super &&
+   pwd=$(pwd) &&
+   (
+   cd super &&
+   git init &&
+   git submodule add --label labelA file://"$pwd"/example2 
submodule &&
+   git config -f .gitmodules submodule."submodule".label >actual &&
+   echo labelA >expected &&
+   test_cmp expected actual
+   )
+'
+
+cat >expected <<-EOF
+labelA
+labelB
+EOF
+
+test_expect_success 'submodule add records multiple labels' '
+   test_when_finished "rm -rf super" &&
+   mkdir super &&
+   pwd=$(pwd) &&
+   (
+   cd super &&
+   git init &&
+   git submodule add --label=labelA -l labelB 
file://"$pwd"/example2 submodule &&
+   git config --get-all -f .gitmodules submodule."submodule".label 
>../actual
+   ) &&
+   test_cmp expected actual
+'
 
 test_done
-- 
2.8.0.41.g8d9aeb3

--
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 02/15] submodule doc: write down what we want to achieve in this series

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt| 4 
 Documentation/git-submodule.txt | 8 
 2 files changed, 12 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 59d7046..c5b6a4e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2735,6 +2735,10 @@ submodule.fetchJobs::
in parallel. A value of 0 will give some reasonable default.
If unset, it defaults to 1.
 
+submodule.defaultGroup::
+   Specifies the group of submodules to be operated on
+   in a command if no submodules were specified in the command.
+
 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/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 13adebf..8c4bbe2 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -37,6 +37,14 @@ these will not be checked out by default; the 'init' and 
'update'
 subcommands will maintain submodules checked out and at
 appropriate revision in your working tree.
 
+When operating on submodules you can either give paths to specify the
+desired submodules or give no paths at all to apply the command to the
+default group of submodules. By default all submodules are included in
+the default group. You can change the default group by configuring
+submodule.defaultGroup. Once the default group is configured any
+submodule operation without a specified set of submodules will use
+the default group as the set to operate on.
+
 Submodules are composed from a so-called `gitlink` tree entry
 in the main repository that refers to a particular commit object
 within the inner repository that is completely separate.
-- 
2.8.0.41.g8d9aeb3

--
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 06/15] submodule init: redirect stdout to stderr

[Stefan Beller]

Reroute the output of stdout to stderr as it is just informative
messages, not to be consumed by machines.

We want to init submodules from the helper for `submodule update`
in a later patch and the stdout output of said helper is consumed
by the parts of `submodule update` which are still written in shell.
So we have to be careful which messages are on stdout.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c |  3 ++-
 t/t7406-submodule-update.sh | 24 ++--
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 23d7224..7b9a4d7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -362,7 +362,8 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
die(_("Failed to register url for submodule path '%s'"),
displaypath);
if (!quiet)
-   printf(_("Submodule '%s' (%s) registered for path 
'%s'\n"),
+   fprintf(stderr,
+   _("Submodule '%s' (%s) registered for path 
'%s'\n"),
sub->name, url, displaypath);
}
 
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index fd741f5..5f27879 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -108,24 +108,36 @@ pwd=$(pwd)
 
 cat ../../actual
+git submodule update --init --recursive ../super >../../actual 
2>../../actual2
) &&
-   test_cmp expect actual
+   test_cmp expect actual &&
+   test_cmp expect2 actual2
 '
 
 apos="'";
-- 
2.8.0.41.g8d9aeb3

--
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 00/15] submodule groups (once again)

[Stefan Beller]

New in this series: git status, git diff and all remaining git submodule 
subcommands.

One pain point I am still aware of:
`git diff` and `git status` completely ignore submodules which are not in the
default group. I am not sure if that is a reasonable default.

A poor analogy could be the .gitignore file configuration:
If an entry exists in .gitignore, the corresponding file is ignored if
(and only if) the file is not part of the repository, i.e changes to 
a tracked (but ignored) file are still shown. Another way of saying it:
The ignore mechanism doesn't influence the diff machinery.

git diff is supposed to view the differences between "what would I
get after checkout" (i.e. what is in the index run through smudge filters)
compared to the actual worktree.
With the submodule default group set, we would expect to not see some
submodules checked out. But if such a submodule is in the worktree,
we may want to show a message instead:

$ git status
... # normal git status stuff
More than 2 submodules (123 actually) including 
'path/foo'
'lib/baz'
# have a reasonable maximum for the number of submodules shown
are checked out, but not part of the default group.
You can add these submodules via
git config --add submodule.defaultGroup ./path/foo
git config --add submodule.defaultGroup ./lib/baz

Once we have such a message, we would need to train `git checkout` to checkout
and drop the right submodules for switching branches.

It has been a while since last posting this series and it is substantially
different in scope (and I have rewritten most of the patches), so I'll not
provide an intra-diff or a version number for this series.

What is this series about?
==

If you have lots of submodules, you probably don't need all of them at once, 
but you have functional units. Some submodules are absolutely required, 
some are optional and only for very specific purposes. 

This patch series adds labels to submodules in the .gitmodules file. 

So you could have a .gitmodules file such as: 

[submodule "gcc"] 
path = gcc 
url = git://... 
label = default
label = devel 
[submodule "linux"] 
path = linux 
url = git://... 
label = default 
[submodule "nethack"] 
path = nethack 
url = git://... 
label = optional
label = games 

and by this series you can work on an arbitrary group of these submodules
composed by the labels, names or paths of the submodules.

git clone --recurse-submodules --init-submodule=label 
--init-submodule=label2   git://... 
# will clone the superproject and recursively 
# checkout any submodule being labeled label or label2

git submodule add --label  git://... ..
# record a label while adding a submodule

git config submodule.defaultGroups default
git config --add submodule.defaultGroups devel
# configure which submodules you are interested in.

git submodule update
# update only the submodules in the default group if that is configured.

git status
git diff
git submodule summary
# show only changes to submodules which are in the default group.

Any feedback welcome, specially on the design level! 
(Do we want to have it stored in the .gitmodules file? Do we want to have 
the groups configured in .git/config as "submodule.groups", any other way 
to make it future proof and extend the groups syntax?) 

Thanks, 
Stefan 

Stefan Beller (15):
  string_list: add string_list_duplicate
  submodule doc: write down what we want to achieve in this series
  submodule add: label submodules if asked to
  submodule-config: keep labels around
  submodule-config: check if submodule a submodule is in a group
  submodule init: redirect stdout to stderr
  submodule deinit: loose requirement for giving '.'
  submodule--helper list: respect submodule groups
  submodule--helper init: respect submodule groups
  submodule--helper update_clone: respect submodule groups
  diff: ignore submodules excluded by groups
  git submodule summary respects groups
  cmd_status: respect submodule groups
  cmd_diff: respect submodule groups
  clone: allow specification of submodules to be cloned

 Documentation/config.txt|   4 +
 Documentation/git-clone.txt |   6 +
 Documentation/git-submodule.txt |  13 +-
 builtin/clone.c |  40 +-
 builtin/commit.c|   3 +
 builtin/diff.c  |   2 +
 builtin/submodule--helper.c |  94 -
 diff.c  |   3 +
 diff.h  |   1 +
 git-submodule.sh|  24 +++-
 string-list.c   |  18 +++
 string-list.h   |   2 +
 submodule-config.c  |  66 ++
 submodule-config.h  |   5 +
 t/t7400-submodule-basic.sh  | 129 +-
 

Re: [PATCH 10/83] builtin/apply: move 'check_index' global into 'struct apply_state'

[Junio C Hamano]

Christian Couder  writes:

> Signed-off-by: Christian Couder 
> ---
>  builtin/apply.c | 69 
> +
>  1 file changed, 40 insertions(+), 29 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 6c628f6..3f8671c 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -30,6 +30,10 @@ struct apply_state {
>*files that are being modified, but doesn't apply the patch
>*/
>   int check;
> +
> + /* --index updates the cache as well. */
> + int check_index;
> +
>   int unidiff_zero;
>  };
>  
> @@ -37,14 +41,12 @@ struct apply_state {
>   *  --stat does just a diffstat, and doesn't actually apply
>   *  --numstat does numeric diffstat, and doesn't actually apply
>   *  --index-info shows the old and new index info for paths if available.
> - *  --index updates the cache as well.
>   *  --cached updates only the cache without ever touching the working tree.
>   */
>  static int newfd = -1;
>  
>  static int state_p_value = 1;
>  static int p_value_known;
> -static int check_index;
>  static int update_index;
>  static int cached;

I like the way this series moves only a few variables at a time to
limit the scope of each step.  I would have expected check-index and
cached to touch pretty much the same codepaths (the latter would
involve a subset of the codepaths involved for the former), but
doing them separately is fine.
--
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 39/83] builtin/apply: move 'ws_error_action' into 'struct apply_state'

[Junio C Hamano]

Junio C Hamano  writes:

> As I do not expect that cmd_apply() which is a moral equivalent of
> main() will stay to be the only one who wants to see a reasonably
> initialized apply_state(), I think the patch that introduced the
> very first version of "struct apply_state" should also introduce a
> helper function to initialize it, i.e.
>
>   static void init_apply_state(struct apply_state *s,
>const char *prefix)
> {
>   memset(s, '\0', sizeof(*s));
> s->prefix = prefix;
> s->prefix_length = s->prefix ? strlen(s->prefix) : 0;
>   }
>
> in [PATCH 7/xx].

Just to avoid misunderstanding, I do not mean to say that the
init-apply-state helper that should have been introduced in 07/xx
would gain a new caller-supplied parameter ws_error_action.  This
step would have a patch to the function that does something like:

 static void init_apply_state(struct apply_state *s,
 const char *prefix)
 {
 memset(s, '\0', sizeof(*s));
 s->prefix = prefix;
 s->prefix_length = s->prefix ? strlen(s->prefix) : 0;
 ...
+s->ws_error_action = warn_on_ws_error;
 }

without having the caller supply what error_action should the state
be initialized with.
--
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 39/83] builtin/apply: move 'ws_error_action' into 'struct apply_state'

[Junio C Hamano]

Christian Couder  writes:

> +enum ws_error_action {
> + nowarn_ws_error,
> + warn_on_ws_error,
> + die_on_ws_error,
> + correct_ws_error
> +};
> +
>  struct apply_state {
>   const char *prefix;
>   int prefix_length;
> @@ -80,6 +87,8 @@ struct apply_state {
>   int whitespace_error;
>   int squelch_whitespace_errors;
>   int applied_after_fixing_ws;
> +
> + enum ws_error_action ws_error_action;
>  };
>  
>  static int newfd = -1;
> @@ -89,12 +98,6 @@ static const char * const apply_usage[] = {
>   NULL
>  };
>  
> -static enum ws_error_action {
> - nowarn_ws_error,
> - warn_on_ws_error,
> - die_on_ws_error,
> - correct_ws_error
> -} ws_error_action = warn_on_ws_error;

This is a good example of a variable that needs initialization,
which is turned into a field in apply_state that needs
initialization.  It is done here:

> @@ -4738,6 +4743,7 @@ int cmd_apply(int argc, const char **argv, const char 
> *prefix_)
>   state.p_value = 1;
>   state.p_context = UINT_MAX;
>   state.squelch_whitespace_errors = 5;
> + state.ws_error_action = warn_on_ws_error;
>   strbuf_init(, 0);

and we already have these random initial values described here.

As I do not expect that cmd_apply() which is a moral equivalent of
main() will stay to be the only one who wants to see a reasonably
initialized apply_state(), I think the patch that introduced the
very first version of "struct apply_state" should also introduce a
helper function to initialize it, i.e.

static void init_apply_state(struct apply_state *s,
 const char *prefix)
{
memset(s, '\0', sizeof(*s));
s->prefix = prefix;
s->prefix_length = s->prefix ? strlen(s->prefix) : 0;
}

in [PATCH 7/xx].
--
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 22/83] builtin/apply: move 'unsafe_paths' global into 'struct apply_state'

[Junio C Hamano]

Christian Couder  writes:

> Signed-off-by: Christian Couder 
> ---
>  builtin/apply.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 506357c..c45e481 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -57,6 +57,8 @@ struct apply_state {
>   int unidiff_zero;
>  
>   int update_index;
> +
> + int unsafe_paths;
>  };

Having said 

I like the way this series moves only a few variables at a time to
limit the scope of each step.

it gets irritating to see all these unnecessary blank lines in the
structure definition added by each step, which would mean all of
these patches need to fix them in the next reroll.
--
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 27/83] builtin/apply: move 'read_stdin' global into cmd_apply()

[Junio C Hamano]

Christian Couder  writes:

> Signed-off-by: Christian Couder 
> ---
>  builtin/apply.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Makes sense but do so immediately next to 06/83, "options" thing?

> diff --git a/builtin/apply.c b/builtin/apply.c
> index 699cabf..be237d1 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -96,7 +96,6 @@ static enum ws_ignore {
>  
>  static const char *patch_input_file;
>  static struct strbuf root = STRBUF_INIT;
> -static int read_stdin = 1;
>  
>  static void parse_whitespace_option(const char *option)
>  {
> @@ -4586,6 +4585,7 @@ int cmd_apply(int argc, const char **argv, const char 
> *prefix_)
>   int is_not_gitdir = !startup_info->have_repository;
>   int force_apply = 0;
>   int options = 0;
> + int read_stdin = 1;
>   struct apply_state state;
>  
>   const char *whitespace_option = NULL;
--
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] remote.c: spell __attribute__ correctly

[Philip Oakley]

Thnx,
From: "Ramsay Jones" 

On 25/04/16 22:50, Philip Oakley wrote:

From: "Jeff King" 

On Mon, Apr 25, 2016 at 05:10:30PM -0400, Jeff King wrote:

It should be handled in git-compat-util.h, which is included by 
cache.h,

which is included by remote.c.

There we have:

  #ifndef __GNUC__
  #ifndef __attribute__
  #define __attribute__(x)
  #endif
  #endif

which should make it a noop on compilers which don't know about it. Is
VS (or another file) setting __GNUC__?


Of course it helps if we spell the name right...


Indeed! ;-)

Not that it matters, but the above #define in git-compat-util.h is not
the relevant definition - msvc will not see it.


Ah, I see that that block is further guarded with other if/elif/else clauses 
so that it's not seen if _MSC_VER is defined.


git-compat-util.h#L400-411


However, it does see
the #define on line 12 of compat/msvc.h. :-D

ATB,
Ramsay Jones



--
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 05/83] builtin/apply: extract line_by_line_fuzzy_match() from match_fragment()

[Junio C Hamano]

Christian Couder  writes:

>> It's not clear why we need to declare buf here? Oh wait it is. It's just
>> moved from the start of the function. But why do it in this patch?
>> It seems unrelated to the general intent of the patch. No need to reroll
>> for this nit alone, it just took me a while to figure out it was an unrelated
>> thing.
>
> Yeah, I agree it's a bit unrelated. But rather than add another patch
> to an already long series just for this,...

Isn't not doing the unrelated move at all a more sensible
alternative, if that change does not deserve its own place in the
series after all?
--
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 33/83] builtin/apply: move 'root' global into 'struct apply_state'

[Christian Couder]

On Mon, Apr 25, 2016 at 11:50 PM, Stefan Beller  wrote:
> On Sun, Apr 24, 2016 at 6:33 AM, Christian Couder
>  wrote:
>
>> @@ -4630,9 +4644,10 @@ static int option_parse_whitespace(const struct 
>> option *opt,
>>  static int option_parse_directory(const struct option *opt,
>>   const char *arg, int unset)
>>  {
>> -   strbuf_reset();
>> -   strbuf_addstr(, arg);
>> -   strbuf_complete(, '/');
>> +   struct apply_state *state = opt->value;
>
> Or even
>
> struct strbuf root = ((state*)opt->value)->root;
>
> and then keep the next lines as is?

I found it more coherent to have all the option_parse_*() functions do:

struct apply_state *state = opt->value;

as their first instruction.
--
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: [PATCHv2 2/3] git-p4 tests: work with python3 as well as python2

[Luke Diamand]

On 26 April 2016 at 18:48, Junio C Hamano  wrote:
> Luke Diamand  writes:
>
>> Update the git-p4 tests so that they work with both
>> Python2 and Python3.
>>
>> We have to be explicit about the difference between
>> Unicode text strings (Python3 default) and raw binary
>> strings which will be exchanged with Perforce.
>>
>> Additionally, print always takes braces in Python3.
>
> s/braces/parentheses/, I think (can locally tweak if you say so--in
> which case no need to resend).

Please do so, thanks!

Luke
--
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 4/4] hooks: Add ability to specify where the hook directory is

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> +core.hooksPath::
> + By default Git will look for your hooks in the
> + '$GIT_DIR/hooks' directory. Set this to different path,
> + e.g. '/etc/git/hooks', and Git will try to find your hooks in
> + that directory, e.g. '/etc/git/hooks/pre-receive' instead of
> + in '$GIT_DIR/hooks/pre-receive'.
> ++
> +The path can either be absolute or relative. When specifying a
> +relative path see the discussion in the "DESCRIPTION" section of
> +linkgit:githooks[5] for what the relative relative path will be
> +relative to.

The path can be either absolute or relative.  A relative path is
taken as relative to the directory where the hooks are run (see
linkgit:githooks[5]).

perhaps?

> +This configuration is useful in cases where you'd like to
> +e.g. centrally configure your Git hooks instead of configuring them on

s/e.g. //;

I thought "you can do X (if you want to do X)" were getting
rewritten to "you can do X", not "you can do e.g. X".

> diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
> new file mode 100755
> index 000..f2f0aa9
> --- /dev/null
> +++ b/t/t1350-config-hooks-path.sh
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +
> +test_description='Test the core.hooksPath configuration variable'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'set up a pre-commit hook in core.hooksPath' '
> + mkdir -p .git/custom-hooks .git/hooks &&
> + write_script .git/custom-hooks/pre-commit <<-\EOF &&
> +printf "%s" "CUST" >>.git/PRE-COMMIT-HOOK-WAS-CALLED
> +EOF

write_script .git/custom-hooks/pre-commit <<-\EOF &&
printf "%s" "CUST" >>.git/PRE-COMMIT-HOOK-WAS-CALLED
EOF

I however wonder why this is not "echo CUST" (or even "echo
CUSTOM").

> + write_script .git/hooks/pre-commit <<-\EOF
> +printf "%s" "NORM" >>.git/PRE-COMMIT-HOOK-WAS-CALLED
> +EOF
> +'

Likewise.

> +test_expect_success 'Check that various forms of specifying core.hooksPath 
> work' '
> + test_commit no_custom_hook &&
> + git config core.hooksPath .git/custom-hooks &&
> + test_commit have_custom_hook &&
> + git config core.hooksPath .git/custom-hooks/ &&
> + test_commit have_custom_hook_trailing_slash &&
> + git config core.hooksPath "$PWD/.git/custom-hooks" &&
> + test_commit have_custom_hook_abs_path &&
> + git config core.hooksPath "$PWD/.git/custom-hooks/" &&
> + test_commit have_custom_hook_abs_path_trailing_slash &&
> + printf "%s" "NORMCUSTCUSTCUSTCUST" 
> >.git/PRE-COMMIT-HOOK-WAS-CALLED.expect &&

And this will become

test_write_lines NORM CUST CUST CUST CUST >expect &&

> + test_cmp .git/PRE-COMMIT-HOOK-WAS-CALLED.expect 
> .git/PRE-COMMIT-HOOK-WAS-CALLED

If you did one-line-at-a-time with 'echo', test_cmp would show
line-wise diff, which may make spotting the difference easier.

> +'
> +
> +test_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

Re: [PATCH v4 2/4] githooks.txt: Amend dangerous advice about 'update' hook ACL

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Any ACL you implement via an 'update' hook isn't actual access control
> if the user has login access to the machine running git, because they
> can trivially just built their own git version which doesn't run the

s/built their own git version/build their own version of git/;

> hook.
>
> Change the documentation to take this dangerous edge case into account,
> and remove the mention of the advice originating on the mailing list,
> the users reading this don't care where the idea came up.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  Documentation/githooks.txt | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 7660b95..9051584 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -275,9 +275,11 @@ does not know the entire set of branches, so it would 
> end up
>  firing one e-mail per ref when used naively, though.  The
>  <> hook is more suited to that.
>  
> -Another use suggested on the mailing list is to use this hook to
> -implement access control which is finer grained than the one
> -based on filesystem group.
> +In an environment that restricts the users' access only to git
> +commands over the wire, this hook can be used to implement access
> +control without relying on filesystem ownership and group
> +membership. See linkgit:git-shell[1] for how you might use the login
> +shell to restrict the user's access to only git commands.
>  
>  Both standard output and standard error output are forwarded to
>  'git send-pack' on the other end, so you can simply `echo` messages
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] githooks.txt: Improve the intro section

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

>  * We note what happens with chdir() before a hook is called, nothing
>documented this explicitly, but the current behavior is
>predictable. It helps a lot to know what directory these hooks will
>be executed from.

I _think_ this item is what you meant by "changed the wording for
all of this to hopefully be more clear in my v4.", but I do not
think it addresses "find it somehow and then chdir()" confusion.

It does say "it is run at", meaning that chdir() happens before it
is run.  That is not sufficient to let the reader guess taht chdir()
happens before the hooks directory is checked relative to that
directory to find the hook to run.

> +When 'git init' is run it may, depending on its configuration, copy
> +hooks to the new repository, see the the "TEMPLATE DIRECTORY" section

"... is run, it may, depending on its configuration, copy hooks..."

If that is too much sentence fragments, you could reorder them, e.g.

'git init' may copy hooks to the new repository, depending
on its configuration.  See the "TEMPLATE DIRECTORY" section

That would fix "the the" as a side effect ;-).

> +in linkgit:git-init[1] for details. When the rest of this document
> +refers to "default hooks" it's talking about the default template
> +shipped with Git.
> +
> +The currently supported hooks are described below.
>  
>  HOOKS
>  -
--
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] config doc: improve exit code listing

[Ramsay Jones]

On 26/04/16 20:18, Stefan Beller wrote:
> On Tue, Apr 26, 2016 at 12:11 PM, Ramsay Jones
>  wrote:
>>
>>
>> On 26/04/16 19:10, Stefan Beller wrote:
>>> The possible reasons for exiting are now ordered by the exit code value.
>>> While at it, rewrite the `can not write to the config file` to
>>> `the config file cannot be written` to be grammatically correct and a
>>> proper sentence.
>>>
>>> Signed-off-by: Stefan Beller 
>>> ---
>>>  Documentation/git-config.txt | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
>>> index 6fc08e3..6843114 100644
>>> --- a/Documentation/git-config.txt
>>> +++ b/Documentation/git-config.txt
>>> @@ -58,10 +58,10 @@ that location (you can say '--local' but that is the 
>>> default).
>>>  This command will fail with non-zero status upon error.  Some exit
>>>  codes are:
>>>
>>> -- The config file is invalid (ret=3),
>>  ^
>> I don't see why this is capitalised, so ...
> 
> Because the whole listing is a bunch of sentences,
> stringed together with commas at the end of each line.
> Note that there is a ',' at the end of each line, except for
> the last, where you see a '.'.

Heh, I hadn't noticed the commas, no - I assumed periods.

> I thought about breaking that
> up into a list and make all of the bullet points either a sentence
> (all capitalised and ending in dot) or part sentences (lower
> case for each bullet point, not clear about the ending)

That's probably what I would have done.

> I kept it as is in a long sentence as I expected to see
> lowest resistance there. ;)

'One long sentence' split into a bullet/numbered list is err ...
Hmm, I'm lost for words. ;-)

>> Only a minor point.
> 
> If the current state bothers you too much,
> please send a patch with correct lists. :)
> (Feel free to squash this patch into that or
> just on top of this)

Having said all that, (since I have very few documentation
skills), I will leave this for others to improve, if
necessary.

ATB,
Ramsay Jones


--
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] hooks: Add ability to specify where the hook directory is

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 26, 2016 at 9:16 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
 +The path can either be absolute or relative. In the latter case see
 +the discussion in the "DESCRIPTION" section of linkgit:githooks[5]
 +about what the relative path will be relative to.
>>>
>>> ... which does not seem to appear there, it seems?
>>
>> I think it does. Read on...
>
> I actually read the result of applying the patch before sending the
> review above.
>
  DESCRIPTION
  ---

 -Hooks are programs you can place in the `$GIT_DIR/hooks` directory to
 -trigger action at certain points. Hooks that don't have the executable
 -bit set are ignored.
 +Hooks are programs you can place in a hooks directory to trigger action
 +at certain points. Hooks that don't have the executable bit set are
 +ignored.
 +
 +By default the hooks directory is `$GIT_DIR/hooks`, but that can be
 +changed via the `core.hooksPath` configuration variable (see
 +linkgit:git-config[1]).
>>>
>>> The section talks about what the cwd of the process that runs the
>>> hook is, but it is not clear at all from these three lines in
>>> core.hooksPath description above how the cwd of the process is
>>> related with the directory the relative path will be relative to.
>>
>> I think the documentation mostly makes sense, but that the context of
>> this patch is confusing.
>>
>> I.e. when I say:
>>
>>> The path can either be absolute or relative. In the latter case see
>>> the discussion in the "DESCRIPTION" section of linkgit:githooks[5]
>>> about what the relative path will be relative to.
>>
>> In config.txt, I'm not talking about the patch to githooks.txt I'm
>> adding in this commit, but the first patch in the githooks.txt series,
>> i.e. this section:
>>
>>> When a hook is called in a non-bare repository the working directory
>>> is guaranteed to be the root of the working tree, in a bare repository
>>> the working directory will be the path to the repository. I.e. hooks
>>> don't need to worry about the user's current working directory.
>>
>> I.e. I'm not talking about the "by default the hooks directory [blah
>> blah]" part I'm adding here.
>
> I know.  What it boils down to I think is this.
>
> If somebody said:
>
> The path to the hooks directory can be specified relative, and
> it is relative to something described elsewhere.
>
> Hooks are run either at the root of the working tree or in
> GIT_DIR, and they are not affected where the user's current
> directory is (they cannot even know where it is).
>
> you interpret, with the knowledge that "we first determine in which
> directory to run a hook with a given name, go there, and then look
> for the named hook", the directory hooks are run in is NATURALLY the
> directory relative paths the hooks are found are relative to.
>
> My problem was that it is only natural if you have that knowledge.
>
> A reader who starts with a mindset "Git first finds the hook to run,
> and then goes to the directory to run it in", it is not naturally
> clear.  The latter is specified by two rules, one for a bare and the
> other for a non-bare repository, and it is very clear.  The former
> is specified nowhere, unless you give a hint to fix the mindset of
> such a reader.

Right. I changed the wording for all of this to hopefully be more
clear in my v4.
--
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] config doc: improve exit code listing

[Stefan Beller]

On Tue, Apr 26, 2016 at 12:11 PM, Ramsay Jones
 wrote:
>
>
> On 26/04/16 19:10, Stefan Beller wrote:
>> The possible reasons for exiting are now ordered by the exit code value.
>> While at it, rewrite the `can not write to the config file` to
>> `the config file cannot be written` to be grammatically correct and a
>> proper sentence.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>  Documentation/git-config.txt | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
>> index 6fc08e3..6843114 100644
>> --- a/Documentation/git-config.txt
>> +++ b/Documentation/git-config.txt
>> @@ -58,10 +58,10 @@ that location (you can say '--local' but that is the 
>> default).
>>  This command will fail with non-zero status upon error.  Some exit
>>  codes are:
>>
>> -- The config file is invalid (ret=3),
>  ^
> I don't see why this is capitalised, so ...

Because the whole listing is a bunch of sentences,
stringed together with commas at the end of each line.
Note that there is a ',' at the end of each line, except for
the last, where you see a '.'. I thought about breaking that
up into a list and make all of the bullet points either a sentence
(all capitalised and ending in dot) or part sentences (lower
case for each bullet point, not clear about the ending)

I kept it as is in a long sentence as I expected to see
lowest resistance there. ;)

>
>> -- can not write to the config file (ret=4),
>> +- The section or key is invalid (ret=1),
>  ^
> I don't think you should transfer the capital here. ;-)

It's the first bullet point starting the long sentence.

>
>>  - no section or name was provided (ret=2),
>> -- the section or key is invalid (ret=1),
>> +- the config file is invalid (ret=3),
>> +- the config file cannot be written (ret=4),
>>  - you try to unset an option which does not exist (ret=5),
>>  - you try to unset/set an option for which multiple lines match (ret=5), or
>>  - you try to use an invalid regexp (ret=6).
>>
>
> Only a minor point.

If the current state bothers you too much,
please send a patch with correct lists. :)
(Feel free to squash this patch into that or
just on top of this)

Thanks,
Stefan

>
> ATB,
> Ramsay Jones
>
>
--
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] hooks: Add ability to specify where the hook directory is

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>>> +The path can either be absolute or relative. In the latter case see
>>> +the discussion in the "DESCRIPTION" section of linkgit:githooks[5]
>>> +about what the relative path will be relative to.
>>
>> ... which does not seem to appear there, it seems?
>
> I think it does. Read on...

I actually read the result of applying the patch before sending the
review above.

>>>  DESCRIPTION
>>>  ---
>>>
>>> -Hooks are programs you can place in the `$GIT_DIR/hooks` directory to
>>> -trigger action at certain points. Hooks that don't have the executable
>>> -bit set are ignored.
>>> +Hooks are programs you can place in a hooks directory to trigger action
>>> +at certain points. Hooks that don't have the executable bit set are
>>> +ignored.
>>> +
>>> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be
>>> +changed via the `core.hooksPath` configuration variable (see
>>> +linkgit:git-config[1]).
>>
>> The section talks about what the cwd of the process that runs the
>> hook is, but it is not clear at all from these three lines in
>> core.hooksPath description above how the cwd of the process is
>> related with the directory the relative path will be relative to.
>
> I think the documentation mostly makes sense, but that the context of
> this patch is confusing.
>
> I.e. when I say:
>
>> The path can either be absolute or relative. In the latter case see
>> the discussion in the "DESCRIPTION" section of linkgit:githooks[5]
>> about what the relative path will be relative to.
>
> In config.txt, I'm not talking about the patch to githooks.txt I'm
> adding in this commit, but the first patch in the githooks.txt series,
> i.e. this section:
>
>> When a hook is called in a non-bare repository the working directory
>> is guaranteed to be the root of the working tree, in a bare repository
>> the working directory will be the path to the repository. I.e. hooks
>> don't need to worry about the user's current working directory.
>
> I.e. I'm not talking about the "by default the hooks directory [blah
> blah]" part I'm adding here.

I know.  What it boils down to I think is this.

If somebody said:

The path to the hooks directory can be specified relative, and
it is relative to something described elsewhere.

Hooks are run either at the root of the working tree or in
GIT_DIR, and they are not affected where the user's current
directory is (they cannot even know where it is).

you interpret, with the knowledge that "we first determine in which
directory to run a hook with a given name, go there, and then look
for the named hook", the directory hooks are run in is NATURALLY the
directory relative paths the hooks are found are relative to.

My problem was that it is only natural if you have that knowledge.

A reader who starts with a mindset "Git first finds the hook to run,
and then goes to the directory to run it in", it is not naturally
clear.  The latter is specified by two rules, one for a bare and the
other for a non-bare repository, and it is very clear.  The former
is specified nowhere, unless you give a hint to fix the mindset of
such a reader.
--
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] config doc: improve exit code listing

[Ramsay Jones]

On 26/04/16 19:10, Stefan Beller wrote:
> The possible reasons for exiting are now ordered by the exit code value.
> While at it, rewrite the `can not write to the config file` to
> `the config file cannot be written` to be grammatically correct and a
> proper sentence.
> 
> Signed-off-by: Stefan Beller 
> ---
>  Documentation/git-config.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 6fc08e3..6843114 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -58,10 +58,10 @@ that location (you can say '--local' but that is the 
> default).
>  This command will fail with non-zero status upon error.  Some exit
>  codes are:
>  
> -- The config file is invalid (ret=3),
 ^
I don't see why this is capitalised, so ...

> -- can not write to the config file (ret=4),
> +- The section or key is invalid (ret=1),
 ^
I don't think you should transfer the capital here. ;-)

>  - no section or name was provided (ret=2),
> -- the section or key is invalid (ret=1),
> +- the config file is invalid (ret=3),
> +- the config file cannot be written (ret=4),
>  - you try to unset an option which does not exist (ret=5),
>  - you try to unset/set an option for which multiple lines match (ret=5), or
>  - you try to use an invalid regexp (ret=6).
> 

Only a minor point.

ATB,
Ramsay Jones


--
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] http: support sending custom HTTP headers

[Junio C Hamano]

Johannes Schindelin  writes:

>  diff --git a/Documentation/config.txt b/Documentation/config.txt
>  index 42d2b50..37b9af7 100644
>  --- a/Documentation/config.txt
>  +++ b/Documentation/config.txt
>  @@ -1655,6 +1655,12 @@ http.emptyAuth::
>   a username in the URL, as libcurl normally requires a username for
>   authentication.
>   
>  +http.extraHeader::
>  +Pass an additional HTTP header when communicating with a server.  If
>  +more than one such entry exists, all of them are added as extra headers.
>  +This feature is useful e.g. to increase security, or to allow
>  +time-limited access based on expiring tokens.

I am unsure about the last two lines.  Is it necessary to have them?
--
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 0/4] Add --base option to git-format-patch to record base tree info

[Junio C Hamano]

Stefan Beller  writes:

>> So from where are you proposing Git to grab that information if you
>> do not tell it?  "If the HEAD is detached, assume that the base is
>> where it was detached from" or something?
>
> That would also work for me. In my first mail I was proposing to take
> the information from the format-patch argument, such that a one off fix
> would be:
>
> (1) git checkout origin/master
> (2) EDIT
> (3) git commit -a -m "fix"
> (4) git format-patch origin/master..  # <- This is the information.
>
> However you read it as taking the information from the first line,
> which is also fine with me, as then the (4) can become
>
> (4a) git format-patch HEAD^

Either would work, but reading from (4) feels a lot less black magic
to me.

>> If you are doing "format-patch master..my-branch", what do you
>> propose to set your base to?  master@{u}, perhaps?
>
> Yes. (I usually use that command with |s|master|origin/master|, so the
> argument is the upstream already. A local master branch does not exist for 
> me.)

Let's hear from folks at Intel ;-) Both of the above sounds like
sensible enhancements to me.


--
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/3] mmap(win32): avoid copy-on-write when it is unnecessary

[Johannes Sixt]

Am 22.04.2016 um 16:31 schrieb Johannes Schindelin:

Often we are mmap()ing read-only. In those cases, it is wasteful to map in
copy-on-write mode. Even worse: it can cause errors where we run out of
space in the page file.

So let's be extra careful to map files in read-only mode whenever
possible.

Signed-off-by: Johannes Schindelin 
---
  compat/win32mmap.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/compat/win32mmap.c b/compat/win32mmap.c
index 3a39f0f..b836169 100644
--- a/compat/win32mmap.c
+++ b/compat/win32mmap.c
@@ -22,14 +22,15 @@ void *git_mmap(void *start, size_t length, int prot, int 
flags, int fd, off_t of
die("Invalid usage of mmap when built with USE_WIN32_MMAP");

hmap = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
-   PAGE_WRITECOPY, 0, 0, NULL);
+   prot == PROT_READ ? PAGE_READONLY : PAGE_WRITECOPY, 0, 0, NULL);


As long as we use this implementation with MAP_PRIVATE, PAGE_WRITECOPY 
is the right setting. Should we insert a check for MAP_PRIVATE to catch 
unexpected use-cases (think of the index-helper daemon effort)?




if (!hmap) {
errno = EINVAL;
return MAP_FAILED;
}

-   temp = MapViewOfFileEx(hmap, FILE_MAP_COPY, h, l, length, start);
+   temp = MapViewOfFileEx(hmap, prot == PROT_READ ?
+   FILE_MAP_READ : FILE_MAP_COPY, h, l, length, start);


Same here: FILE_MAP_COPY is the right choice for MAP_SHARED mmaps.



if (!CloseHandle(hmap))
warning("unable to close file mapping handle");



Except for these mental notes, I've no comments on this series. Looks good.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Tue, Apr 26, 2016 at 11:30 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> I can totally do that for longer series which require some back and forth.
>>
>> But one-offs, such as typo fixes or other small things[1], for which I do
>> even have a local branch (i.e. checkout origin/master && fix &&
>> commit && send-email) this is another step that potentially bothers me.
>
> So from where are you proposing Git to grab that information if you
> do not tell it?  "If the HEAD is detached, assume that the base is
> where it was detached from" or something?

That would also work for me. In my first mail I was proposing to take
the information from the format-patch argument, such that a one off fix
would be:

(1) git checkout origin/master
(2) EDIT
(3) git commit -a -m "fix"
(4) git format-patch origin/master..  # <- This is the information.

However you read it as taking the information from the first line,
which is also fine with me, as then the (4) can become

(4a) git format-patch HEAD^

Another thought:

  Most workflows do not have different remotes per branch, e.g.
  when `master` maps to  `origin/master` as its upstream it is likely that
  `topic-foo` maps to its equivalent at `origin/..` as well.

  Branches come and go in a topic based workflow, so configuring them
  for each new branch is cumbersome, so let's have a default `remote` for
  repository.

If we have a default remote per repository, the base finding algorithm
in format-patch could check if the base(s) of the patch series is a head
in one of the default remote branches, i.e. check all origin/* branches for
a match?


>
>> From a UI perspective it seems logical to also check if the base
>> can be obtained from the patch range specifier.
>
> If you are doing "format-patch master..my-branch", what do you
> propose to set your base to?  master@{u}, perhaps?

Yes. (I usually use that command with |s|master|origin/master|, so the
argument is the upstream already. A local master branch does not exist for me.)

>
--
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] config doc: improve exit code listing

[Junio C Hamano]

Stefan Beller  writes:

> The possible reasons for exiting are now ordered by the exit code value.

Which makes sense.  Will queue.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Junio C Hamano]

Stefan Beller  writes:

> I can totally do that for longer series which require some back and forth.
>
> But one-offs, such as typo fixes or other small things[1], for which I do
> even have a local branch (i.e. checkout origin/master && fix &&
> commit && send-email) this is another step that potentially bothers me.

So from where are you proposing Git to grab that information if you
do not tell it?  "If the HEAD is detached, assume that the base is
where it was detached from" or something?

> From a UI perspective it seems logical to also check if the base
> can be obtained from the patch range specifier.

If you are doing "format-patch master..my-branch", what do you
propose to set your base to?  master@{u}, perhaps?

--
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 19/21] bisect: use a bottom-up traversal to find relevant weights

[Junio C Hamano]

Stephan Beyer  writes:

>  struct commit_list *find_bisection(struct commit_list *list,
> @@ -470,8 +541,11 @@ struct commit_list *find_bisection(struct commit_list 
> *list,
>   compute_all_weights(list, weights);
>   best = best_bisection_sorted(list);
>   } else {
> + int i;
>   compute_relevant_weights(list, weights);
>   best = best_bisection(list);
> + for (i = 0; i < on_list; i++) /* cleanup */
> + free_commit_list(weights[i].children);
>   }
>   assert(best);
>   *reaches = node_data(best->item)->weight;

One thing I forgot to mention is that we now may want to reconsider
what the first loop in this function does.  It used to be that the
purpose of the loop is to "count the number of total and
tree-changing items on the list while reversing the list" as the
comment says.  While it is still necessary to count the items (by
the way, with 16/21 you made these two numbers identical, i.e. there
no longer is a separate 'total' but your 'total' now actually means
the number of tree-changing items), I do not know if the "reverse"
would still be a good fit for the performance characteristic of the
new algorithm.

The list-reversal there was done as an optimization to make sure
that older ones are processed early to avoid looping too much just
to follow the list to find a single-parent commit whose parent's
weight is already known, as the only meaningful optimization in the
original algorithm was the "we can increment one without doing the
costly graph re-traversal for single-strand-of-pearls".  That
optimization may no longer relevant (or it could even be harmful)
as you traverse the graph in reverse.





--
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 v5b 00/17] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Tue, Apr 26, 2016 at 3:17 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> This is part of unification of the commands 'git tag -l, git branch -l
>> and git for-each-ref'. This ports over branch.c to use ref-filter's
>> printing options.
>>
>> Initially posted here: $(gmane/279226). It was decided that this series
>> would follow up after refactoring ref-filter parsing mechanism, which
>> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>>
>> v1 can be found here: $(gmane/288342)
>> v2 can be found here: $(gmane/288863)
>> v3 can be found here: $(gmane/290299)
>> v4 can be found here: $(gmane/291106)
>>
>> Changes in this version (v5b):
>> 1. Added the first patch of the series which was missing in v5.
>
> 2. Rebased on top of 'master', which includes
>jk/branch-shortening-funny-symrefs.
>
>> Interdiff:
>>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index c9a2e5b..6847ac3 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -288,9 +288,11 @@ static int calc_maxwidth(struct ref_array *refs, int 
>> remote_bonus)
>>
>> skip_prefix(it->refname, "refs/heads/", );
>> skip_prefix(it->refname, "refs/remotes/", );
>> -   if (it->kind == FILTER_REFS_DETACHED_HEAD)
>> -   w = strlen(get_head_description());
>> -   else
>> +   if (it->kind == FILTER_REFS_DETACHED_HEAD) {
>> +   char *head_desc = get_head_description();
>> +   w = strlen(head_desc);
>> +   free(head_desc);
>> +   } else
>> w = utf8_strwidth(desc);
>
> Presumably w is computed here to be used later for some kind of
> alignment?  It is curious why we can assume that head_desc does not
> need utf8_strwidth() here.

Seems to be a bug, `get_head_description()`  is susceptible to be changed
due to translation and hence `utf8_strwidth()` would be a better candidate here
rather than `strlen()`.

Thanks.

-- 
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 v6 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Tue, Apr 26, 2016 at 11:05 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> git checkout origin/master
>> # toy around, do stuff
>> git checkout -b new-shiny-feature
>> git format-patch origin-master..
>>
>> Now I have set the format.useautobase option and then the `git format-patch`
>> fails with
>>
>> fatal: Failed to get upstream, if you want to record base commit
>> automatically,
>> please use git branch --set-upstream-to to track a remote branch.
>> Or you could specify base commit by --base= manually.
>>
>> but as I indicated I want patches from origin/master onwards,
>> Could we make use of that information?
>
> As you indicated where other than in this e-mail?
>
> I think the way for you to indicate that desire expected by this
> series is to use "git branch" to set upstream of new-shiny-feature
> branch to origin/master.  Shouldn't that work, or is that too much
> work?

I can totally do that for longer series which require some back and forth.

But one-offs, such as typo fixes or other small things[1], for which I do
even have a local branch (i.e. checkout origin/master && fix &&
commit && send-email) this is another step that potentially bothers me.
Maybe I'll get used to it.

>From a UI perspective it seems logical to also check if the base
can be obtained from the patch range specifier. the message of
patch 2 focuses on the advantages for the maintainer and 3rd party
people. So I was just testing it as an individual contributor to ensure
it can be used easily. (People only use this once the benefits outweigh
the disadvantages. And as we do not have any advantage of it in Git,
the negatives need to be kept low?)

Thanks,
Stefan

[1] http://thread.gmane.org/gmane.comp.version-control.git/292634
--
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 v4 0/4] githooks.txt improvements + core.hooksDirectory

[Ævar Arnfjörð Bjarmason]

In this version of the series I've hopefully addressed all the
comments that came up on the list after the last one, and a few fixes
I noticed myself, e.g. a couple of grammar errors and a broken
asciidoc syntax.

I've combined both the githooks.txt documentation improvements and the
core.hooksDirectory patch into one series. Although they're logically
different things I think it makes more sense to combine them for ease
of reading, since the core.hooksDirectory documentation refers to some
documentation I fixed earlier in the series.

Ævar Arnfjörð Bjarmason (4):
  githooks.txt: Improve the intro section
  githooks.txt: Amend dangerous advice about 'update' hook ACL
  githooks.txt: Minor improvements to the grammar & phrasing
  hooks: Add ability to specify where the hook directory is

 Documentation/config.txt | 18 +++
 Documentation/git-init.txt   |  7 -
 Documentation/githooks.txt   | 74 ++--
 cache.h  |  1 +
 config.c |  3 ++
 environment.c|  1 +
 run-command.c|  5 ++-
 t/t1350-config-hooks-path.sh | 31 +++
 8 files changed, 108 insertions(+), 32 deletions(-)
 create mode 100755 t/t1350-config-hooks-path.sh

-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 4/4] hooks: Add ability to specify where the hook directory is

[Ævar Arnfjörð Bjarmason]

Change the hardcoded lookup for .git/hooks/* to optionally lookup in
$(git config core.hooksPath)/* instead.

This is essentially a more intrusive version of the git-init ability to
specify hooks on init time via init templates.

The difference between that facility and this feature is that this can
be set up after the fact via e.g. ~/.gitconfig or /etc/gitconfig to
apply for all your personal repositories, or all repositories on the
system.

I plan on using this on a centralized Git server where users can create
arbitrary repositories under /gitroot, but I'd like to manage all the
hooks that should be run centrally via a unified dispatch mechanism.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/config.txt | 18 ++
 Documentation/githooks.txt   | 12 
 cache.h  |  1 +
 config.c |  3 +++
 environment.c|  1 +
 run-command.c|  5 -
 t/t1350-config-hooks-path.sh | 31 +++
 7 files changed, 66 insertions(+), 5 deletions(-)
 create mode 100755 t/t1350-config-hooks-path.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..c007b12 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -618,6 +618,24 @@ core.attributesFile::
$XDG_CONFIG_HOME/git/attributes. If $XDG_CONFIG_HOME is either not
set or empty, $HOME/.config/git/attributes is used instead.
 
+core.hooksPath::
+   By default Git will look for your hooks in the
+   '$GIT_DIR/hooks' directory. Set this to different path,
+   e.g. '/etc/git/hooks', and Git will try to find your hooks in
+   that directory, e.g. '/etc/git/hooks/pre-receive' instead of
+   in '$GIT_DIR/hooks/pre-receive'.
++
+The path can either be absolute or relative. When specifying a
+relative path see the discussion in the "DESCRIPTION" section of
+linkgit:githooks[5] for what the relative relative path will be
+relative to.
++
+This configuration is useful in cases where you'd like to
+e.g. centrally configure your Git hooks instead of configuring them on
+a per-repository basis, or as a more flexible and centralized
+alterantive to having an `init.templateDir` where you've changed the
+'hooks' directory.
+
 core.editor::
Commands such as `commit` and `tag` that lets you edit
messages by launching an editor uses the value of this
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index a214284..97ae78d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -7,15 +7,19 @@ githooks - Hooks used by Git
 
 SYNOPSIS
 
-$GIT_DIR/hooks/*
+$GIT_DIR/hooks/* (or \`git config core.hooksPath`/*)
 
 
 DESCRIPTION
 ---
 
-Hooks are programs you can place in the `$GIT_DIR/hooks` directory to
-trigger actions at certain points in git's execution. Hooks that don't
-have the executable bit set are ignored.
+Hooks are programs you can place in a hooks directory to trigger
+actions at certain points in git's execution. Hooks that don't have
+the executable bit set are ignored.
+
+By default the hooks directory is `$GIT_DIR/hooks`, but that can be
+changed via the `core.hooksPath` configuration variable (see
+linkgit:git-config[1]).
 
 When a hook is invoked, it is run at the root of the working tree in a
 non-bare repository, or in the $GIT_DIR in a bare
diff --git a/cache.h b/cache.h
index 2711048..4f7d222 100644
--- a/cache.h
+++ b/cache.h
@@ -654,6 +654,7 @@ extern int warn_on_object_refname_ambiguity;
 extern const char *apply_default_whitespace;
 extern const char *apply_default_ignorewhitespace;
 extern const char *git_attributes_file;
+extern const char *git_hooks_path;
 extern int zlib_compression_level;
 extern int core_compression_level;
 extern int core_compression_seen;
diff --git a/config.c b/config.c
index 10b5c95..51f80e4 100644
--- a/config.c
+++ b/config.c
@@ -717,6 +717,9 @@ static int git_default_core_config(const char *var, const 
char *value)
if (!strcmp(var, "core.attributesfile"))
return git_config_pathname(_attributes_file, var, value);
 
+   if (!strcmp(var, "core.hookspath"))
+   return git_config_pathname(_hooks_path, var, value);
+
if (!strcmp(var, "core.bare")) {
is_bare_repository_cfg = git_config_bool(var, value);
return 0;
diff --git a/environment.c b/environment.c
index 57acb2f..2857e3f 100644
--- a/environment.c
+++ b/environment.c
@@ -31,6 +31,7 @@ const char *git_log_output_encoding;
 const char *apply_default_whitespace;
 const char *apply_default_ignorewhitespace;
 const char *git_attributes_file;
+const char *git_hooks_path;
 int zlib_compression_level = Z_BEST_SPEED;
 int core_compression_level;
 int core_compression_seen;
diff --git a/run-command.c b/run-command.c
index 8c7115a..39d7237 100644
--- a/run-command.c
+++ b/run-command.c
@@ -815,7 +815,10 @@ const char 

[PATCH v4 3/4] githooks.txt: Minor improvements to the grammar & phrasing

[Ævar Arnfjörð Bjarmason]

Change:

 * Sentences that needed "the" or "a" to either add those or change them
   so they don't need them.

 * The little tangent about "You can use this to do X (if your project
   wants to do X)" can just be shortened to "e.g. if you want to do X".

 * s/parameter/parameters/ when the plural made more sense.

Most of this goes all the way back to the initial introduction of
hooks.txt in v0.99.5-76-g6d35cc7 by Junio.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/githooks.txt | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 9051584..a214284 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -40,15 +40,15 @@ HOOKS
 applypatch-msg
 ~~
 
-This hook is invoked by 'git am' script.  It takes a single
+This hook is invoked by 'git am'.  It takes a single
 parameter, the name of the file that holds the proposed commit
-log message.  Exiting with non-zero status causes
-'git am' to abort before applying the patch.
+log message.  Exiting with a non-zero status causes 'git am' to abort
+before applying the patch.
 
 The hook is allowed to edit the message file in place, and can
 be used to normalize the message into some project standard
-format (if the project has one). It can also be used to refuse
-the commit after inspecting the message file.
+format. It can also be used to refuse the commit after inspecting
+the message file.
 
 The default 'applypatch-msg' hook, when enabled, runs the
 'commit-msg' hook, if the latter is enabled.
@@ -81,10 +81,10 @@ pre-commit
 ~~
 
 This hook is invoked by 'git commit', and can be bypassed
-with `--no-verify` option.  It takes no parameter, and is
+with the `--no-verify` option.  It takes no parameters, and is
 invoked before obtaining the proposed commit log message and
-making a commit.  Exiting with non-zero status from this script
-causes the 'git commit' to abort.
+making a commit.  Exiting with a non-zero status from this script
+causes the 'git commit' command to abort before creating a commit.
 
 The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
@@ -123,15 +123,15 @@ commit-msg
 ~~
 
 This hook is invoked by 'git commit', and can be bypassed
-with `--no-verify` option.  It takes a single parameter, the
+with the `--no-verify` option.  It takes a single parameter, the
 name of the file that holds the proposed commit log message.
-Exiting with non-zero status causes the 'git commit' to
+Exiting with a non-zero status causes the 'git commit' to
 abort.
 
-The hook is allowed to edit the message file in place, and can
-be used to normalize the message into some project standard
-format (if the project has one). It can also be used to refuse
-the commit after inspecting the message file.
+The hook is allowed to edit the message file in place, and can be used
+to normalize the message into some project standard format. It
+can also be used to refuse the commit after inspecting the message
+file.
 
 The default 'commit-msg' hook, when enabled, detects duplicate
 "Signed-off-by" lines, and aborts the commit if one is found.
@@ -139,8 +139,8 @@ The default 'commit-msg' hook, when enabled, detects 
duplicate
 post-commit
 ~~~
 
-This hook is invoked by 'git commit'.  It takes no
-parameter, and is invoked after a commit is made.
+This hook is invoked by 'git commit'. It takes no parameters, and is
+invoked after a commit is made.
 
 This hook is meant primarily for notification, and cannot affect
 the outcome of 'git commit'.
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/4] githooks.txt: Improve the intro section

[Ævar Arnfjörð Bjarmason]

Change the documentation so that:

 * We don't talk about "little scripts". Hooks can be as big as you
   want, and don't have to be scripts, just call them "programs".

 * We note what happens with chdir() before a hook is called, nothing
   documented this explicitly, but the current behavior is
   predictable. It helps a lot to know what directory these hooks will
   be executed from.

 * We don't make claims about the example hooks which may not be true
   depending on the configuration of 'init.templateDir'. Clarify that
   we're talking about the default settings of git-init in those cases,
   and move some of this documentation into git-init's documentation
   about the default templates.

 * We briefly note in the intro that hooks can get their arguments in
   various different ways, and that how exactly is described below for
   each hook.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-init.txt |  7 ++-
 Documentation/githooks.txt | 32 
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 8174d27..6364e5d 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -130,7 +130,12 @@ The template directory will be one of the following (in 
order):
  - the default template directory: `/usr/share/git-core/templates`.
 
 The default template directory includes some directory structure, suggested
-"exclude patterns" (see linkgit:gitignore[5]), and sample hook files (see 
linkgit:githooks[5]).
+"exclude patterns" (see linkgit:gitignore[5]), and sample hook files.
+
+The sample hooks are all disabled by default, To enable one of the
+sample hooks rename it by removing its `.sample` suffix.
+
+See linkgit:githooks[5] for more general info on hook execution.
 
 EXAMPLES
 
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index a2f59b1..7660b95 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -13,18 +13,26 @@ $GIT_DIR/hooks/*
 DESCRIPTION
 ---
 
-Hooks are little scripts you can place in `$GIT_DIR/hooks`
-directory to trigger action at certain points.  When
-'git init' is run, a handful of example hooks are copied into the
-`hooks` directory of the new repository, but by default they are
-all disabled.  To enable a hook, rename it by removing its `.sample`
-suffix.
-
-NOTE: It is also a requirement for a given hook to be executable.
-However - in a freshly initialized repository - the `.sample` files are
-executable by default.
-
-This document describes the currently defined hooks.
+Hooks are programs you can place in the `$GIT_DIR/hooks` directory to
+trigger actions at certain points in git's execution. Hooks that don't
+have the executable bit set are ignored.
+
+When a hook is invoked, it is run at the root of the working tree in a
+non-bare repository, or in the $GIT_DIR in a bare
+repository. I.e. hooks don't need to worry about the user's current
+working directory.
+
+Hooks can get their arguments via the environment, command-line
+arguments, and stdin. See the documentation for each hook below for
+details.
+
+When 'git init' is run it may, depending on its configuration, copy
+hooks to the new repository, see the the "TEMPLATE DIRECTORY" section
+in linkgit:git-init[1] for details. When the rest of this document
+refers to "default hooks" it's talking about the default template
+shipped with Git.
+
+The currently supported hooks are described below.
 
 HOOKS
 -
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/4] githooks.txt: Amend dangerous advice about 'update' hook ACL

[Ævar Arnfjörð Bjarmason]

Any ACL you implement via an 'update' hook isn't actual access control
if the user has login access to the machine running git, because they
can trivially just built their own git version which doesn't run the
hook.

Change the documentation to take this dangerous edge case into account,
and remove the mention of the advice originating on the mailing list,
the users reading this don't care where the idea came up.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/githooks.txt | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 7660b95..9051584 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -275,9 +275,11 @@ does not know the entire set of branches, so it would end 
up
 firing one e-mail per ref when used naively, though.  The
 <> hook is more suited to that.
 
-Another use suggested on the mailing list is to use this hook to
-implement access control which is finer grained than the one
-based on filesystem group.
+In an environment that restricts the users' access only to git
+commands over the wire, this hook can be used to implement access
+control without relying on filesystem ownership and group
+membership. See linkgit:git-shell[1] for how you might use the login
+shell to restrict the user's access to only git commands.
 
 Both standard output and standard error output are forwarded to
 'git send-pack' on the other end, so you can simply `echo` messages
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] config doc: improve exit code listing

[Stefan Beller]

The possible reasons for exiting are now ordered by the exit code value.
While at it, rewrite the `can not write to the config file` to
`the config file cannot be written` to be grammatically correct and a
proper sentence.

Signed-off-by: Stefan Beller 
---
 Documentation/git-config.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 6fc08e3..6843114 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -58,10 +58,10 @@ that location (you can say '--local' but that is the 
default).
 This command will fail with non-zero status upon error.  Some exit
 codes are:
 
-- The config file is invalid (ret=3),
-- can not write to the config file (ret=4),
+- The section or key is invalid (ret=1),
 - no section or name was provided (ret=2),
-- the section or key is invalid (ret=1),
+- the config file is invalid (ret=3),
+- the config file cannot be written (ret=4),
 - you try to unset an option which does not exist (ret=5),
 - you try to unset/set an option for which multiple lines match (ret=5), or
 - you try to use an invalid regexp (ret=6).
-- 
2.8.1.343.gb96d81e

base-commit: 3ad15fd5e17bbb73fb1161ff4e9c3ed254d5b243
--
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 0/4] Add --base option to git-format-patch to record base tree info

[Junio C Hamano]

Stefan Beller  writes:

> git checkout origin/master
> # toy around, do stuff
> git checkout -b new-shiny-feature
> git format-patch origin-master..
>
> Now I have set the format.useautobase option and then the `git format-patch`
> fails with
>
> fatal: Failed to get upstream, if you want to record base commit
> automatically,
> please use git branch --set-upstream-to to track a remote branch.
> Or you could specify base commit by --base= manually.
>
> but as I indicated I want patches from origin/master onwards,
> Could we make use of that information?

As you indicated where other than in this e-mail?  

I think the way for you to indicate that desire expected by this
series is to use "git branch" to set upstream of new-shiny-feature
branch to origin/master.  Shouldn't that work, or is that too much
work?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "gitk --author=foo" shows also parent

[Jeff King]

On Tue, Apr 26, 2016 at 06:12:48PM +0200, Nikolai Kosjar wrote:

> >I believe that this is intentional.  Notice that the parent commit's
> >circle is just outlined
> >compared to the selected authored commits are filled.  I consider this
> >the context
> >of the commits you are looking at.
> 
> Hmm, then I'm not interested in the context since it's too noisy. Is there
> any way to suppress this?

I am not a gitk user, but AFAIK, no. Gitk uses "--boundary" to ask git
for the bottom boundary of each string of commits. There is
unfortunately no "--no-boundary" option to override this (and I just
tried adding one, and it doesn't seem to work; gitk is perhaps too
aggressive in the way it passes along --boundary).

So I think it would require a patch to gitk.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] githooks.txt: Improve the intro section

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 25, 2016 at 8:23 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason   writes:
>
>> Change the documentation so that:
>>
>>  * We don't talk about "little scripts". Hooks can be as big as you
>>want, and don't have to be scripts, just call them "programs".
>>
>>  * We note what happens with chdir() before a hook is called, nothing
>>documented this explicitly, but the current behavior is
>>predictable. It helps a lot to know what directory these hooks will
>>be executed from.
>>
>>  * We don't make claims about the example hooks which may not be true
>>depending on the configuration of 'init.templateDir'. Clarify that
>>we're talking about the default settings of git-init in those cases,
>>and move some of this documentation into git-init's documentation
>>about the default templates.
>>
>>  * We briefly note in the intro that hooks can get their arguments in
>>various different ways, and that how exactly is described below for
>>each hook.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>> ---
>>  Documentation/git-init.txt |  6 +-
>>  Documentation/githooks.txt | 32 
>>  2 files changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
>> index 8174d27..cf37926 100644
>> --- a/Documentation/git-init.txt
>> +++ b/Documentation/git-init.txt
>> @@ -130,7 +130,11 @@ The template directory will be one of the following (in 
>> order):
>>   - the default template directory: `/usr/share/git-core/templates`.
>>
>>  The default template directory includes some directory structure, suggested
>> -"exclude patterns" (see linkgit:gitignore[5]), and sample hook files (see 
>> linkgit:githooks[5]).
>> +"exclude patterns" (see linkgit:gitignore[5]), and example hook files.
>> +
>> +The example are all disabled by default. To enable a hook, rename it
>
> "sample hooks are all disabled" would be better; if for some unknown
> reason you are trying to avoid "sample hooks", "examples are all
> disabled" may be acceptable.
>
>> +by removing its `.sample` suffix. See linkgit:githooks[5] for more
>> +info on hook execution.
>
> Makes a first-time reader wonder if I am allowed to ignore the
> sample and write my own from scratch, or if the only thing that is
> allowed is to enable what is shipped with .sample suffix.
>
> I wonder it would become less confusing if we placed even _less_
> stress on these sample hooks; I find that saying we ship sample
> hooks that are disabled is probably more confusing.
>
> We do not ship any hook (hence nothing is enabled by definition);
> there are a handful of sample files that you can use when adding
> your own hook by either referencing them or taking them as-is, and
> the latter can be done by removing .sample suffix, which is merely a
> special case convenience.

Will fix this confusion.

>
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index a2f59b1..2f3caf7 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -13,18 +13,26 @@ $GIT_DIR/hooks/*
>>  DESCRIPTION
>>  ---
>>
>> -Hooks are little scripts you can place in `$GIT_DIR/hooks`
>> -directory to trigger action at certain points.  When
>> -'git init' is run, a handful of example hooks are copied into the
>> -`hooks` directory of the new repository, but by default they are
>> -all disabled.  To enable a hook, rename it by removing its `.sample`
>> -suffix.
>> -
>> -NOTE: It is also a requirement for a given hook to be executable.
>> -However - in a freshly initialized repository - the `.sample` files are
>> -executable by default.
>> -
>> -This document describes the currently defined hooks.
>> +Hooks are programs you can place in the `$GIT_DIR/hooks` directory to
>> +trigger action at certain points. Hooks that don't have the executable
>> +bit set are ignored.
>
> The last sentence is POSIXPERM only, though.

So what does this do on non-POSIX systems?:

const char *find_hook(const char *name)
[...]
strbuf_git_path(, "hooks/%s", name);
if (access(path.buf, X_OK) < 0)
return NULL;

Just always return true I guess.

I'm not going to change this bit, I think that if we have special
systems that don't have +x it makes sense to just document how +x
works on those systems rather than the entirety of the rest of the git
documentation adding caveats every time the executable bit concept
comes up.

>> +When a hook is called in a non-bare repository the working directory
>> +is guaranteed to be the root of the working tree, in a bare repository
>> +the working directory will be the path to the repository. I.e. hooks
>> +don't need to worry about the user's current working directory.
>
> This sentence took me two reads until I mentally substituted "the
> working directory" with "its working diretory", to realize that you
> are 

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Christian Couder]

On Tue, Apr 26, 2016 at 6:09 PM, Ævar Arnfjörð Bjarmason
 wrote:
> On Tue, Apr 26, 2016 at 3:40 PM, Marc Branchaud  wrote:
>> On 2016-04-26 06:58 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> Makes sense to have an experimental.* config tree for git for stuff like 
>>> this.
>>
>> I disagree.
>>
>> * If the point is to express some kind of warning to users, I think the
>> community has been much better served by leaving experimental settings
>> undocumented (or documented only in unmerged topic branches).

There are features considered experimental that are documented, like
--split-index in `git update-index` and `git interpret-trailers`.

As far as I know the possible reasons they are considered experimental are:

- in the release notes they were described as "experimental",
- they are not considered complete, because of missing features,
- they might not be useful as is,
- they might be buggy in the real world.

>> It feels like
>> an experimental.* tree is a doorway to putting experimental features in
>> official releases, which seems odd considering that (IMHO) git has so far
>> done very well with the carefully-planned-out integration of all sorts of
>> features.

Some people have been happily experimenting with or even using some of
the experimental features, like the ones I am talking about above.

>> * Part of the experiment is coming up with appropriate configuration knobs,
>> including where those knobs should live.

I agree that it can be difficult to find the right knobs for any new feature.

>> Often such considerations lead to a
>> better implementation for the feature.  Dumping things into an experimental.*
>> tree would merely postpone that part of the feature's design.

I am not so sure about this. For example `git update-index
--untracked-cache` used to be considered experimental, but it
definitely had knobs that were not right, so its UI has been reworked
recently.

Maybe if it had first been created with an UI in an experimental.*
config tree, rather than only options to `git update-index` it would
have been an easier transition.

>> * Such a tree creates a flag day when the experimental feature eventually
>> becomes a "real" feature. That'll annoy any early adopters. Sure, they
>> *should* be prepared to deal with config tree bike-shedding, but still that
>> extra churn seems unnecessary.

Not sure about that. New config options outside the experimental.*
config tree could always take over config options in the
experimental.* config tree, as if they appear, it means that the user
is aware that they are the new way to configure the feature. Then the
cost of supporting both the old options in the experimental.* config
tree and the new ones outside should be very small, especially if
there are not many changes between the two set of options.

If there are a lot of changes between these two sets, it means that we
were probably right to have the old ones in the experimental.* config
tree. And in the worst case, which should hardly ever happen, we can
probably afford to just emit warnings saying "old 'experimental.'
config option is not supported anymore, please use  config option
instead" and just ignore the 'experimental.' config option.

> By "stuff like this", yeah I did mean, and I assume Junio means,
> putting "experimental" features in official releases.
>
> E.g. perl does this, if you type "perldoc experimental" on a Linux box
> you'll get the documentation.
>
> Basically you can look at patches to a project on a spectrum of:
>
>  1. You hacked something up locally
>  2. It's in someone's *.git repo as a POC
>  3. It's a third-party patch series used by a bunch of people
>  4. In an official release but documented as experimental
>  5. In an official release as a first-rate feature

Yeah, and it seems to me that Git also has:

4.5. In an official release, documented, but scarcely documented as experimental

and in fact more stuff under 4.5. than under 4.

And in Git there is also the notion of porcelain vs plumbing, where
porcelain output can more easily be changed a bit than plumbing
output.

> Something like an experimental.WHATEVER=bool flag provides #4.
>
> I think aside from this feature just leaving these things undocumented
> really provides the worst of both worlds.

Yeah I agree. Undocumented stuff in Git is already for stuff that we
don't want people to use.

> Now you have some feature that's undocumented *because* you're not
> sure about it, but you can't ever be sure about it unless people
> actually use it, and if it's not documented at all effectively it's as
> visible as some third-party patch series. I.e. only people really
> involved in the project will ever use it.
>
> Which is why perl has the "experimental" subsystem, it allows for
> playing around with features the maintainers aren't quite sure about
> in official releases, and the users know they're opting in to trying
> something unstable that may go away or 

Re: [PATCHv2] clone: add `--shallow-submodules` flag

[Stefan Beller]

On Tue, Apr 26, 2016 at 10:46 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>  I did not rebase this on 85705cfb (Merge branch 'ss/clone-depth-single-doc',
>>  2016-01-20) or later, but worked on it with the base unchanged.
>
> Thanks, will replace.
>
>> diff --git a/t/t5614-clone-submodules.sh b/t/t5614-clone-submodules.sh
>> new file mode 100755
>> index 000..62044c5
>> --- /dev/null
>> +++ b/t/t5614-clone-submodules.sh
>> @@ -0,0 +1,85 @@
>> +#!/bin/sh
>> +
>> +test_description='Test shallow cloning of repos with submodules'
>> +
>> +. ./test-lib.sh
>> +
>> +pwd=$(pwd)
>> +
>> +test_expect_success 'setup' '
>> + git checkout -b master &&
>> + test_commit commit1 &&
>> + test_commit commit2 &&
>> + mkdir sub &&
>> + (
>> + cd sub &&
>> + git init &&
>> + test_commit subcommit1 &&
>> + test_commit subcommit2 &&
>> + test_commit subcommit3
>> + ) &&
>> + git submodule add "file://$pwd/sub" sub &&
>> + git commit -m "add submodule"
>> +'
>> +
>> +test_expect_success 'nonshallow clone implies nonshallow submodule' '
>> + test_when_finished "rm -rf super_clone" &&
>> + git clone --recurse-submodules "file://$pwd/." super_clone &&
>
> All of these "$path/." made me wonder one thing.  I know these URLs
> that ends with slash-dot ought to work, but shouldn't they work
> without them, too?  The "consistency" in this test that ends
> anything that would have ended with "$pwd" with "$pwd/." somewhat
> bothered me.
>

Another case of me not thinking it through. There used to be just '.'
in the former
series with the --no-local option. And to make it a file url I just
prefixed that dot
without thinking if the dot is still needed.

In case another reroll is needed, I'll fix that up, too.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/3] git-p4 tests: work with python3 as well as python2

[Junio C Hamano]

Luke Diamand  writes:

> Update the git-p4 tests so that they work with both
> Python2 and Python3.
>
> We have to be explicit about the difference between
> Unicode text strings (Python3 default) and raw binary
> strings which will be exchanged with Perforce.
>
> Additionally, print always takes braces in Python3.

s/braces/parentheses/, I think (can locally tweak if you say so--in
which case no need to resend).

>
> Signed-off-by: Luke Diamand 
> ---
>  t/lib-git-p4.sh| 5 +++--
>  t/t9802-git-p4-filetype.sh | 6 +++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index 724bc43..7393ee2 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -198,9 +198,10 @@ marshal_dump() {
>   cat >"$TRASH_DIRECTORY/marshal-dump.py" <<-EOF &&
>   import marshal
>   import sys
> + instream = getattr(sys.stdin, 'buffer', sys.stdin)
>   for i in range($line):
> - d = marshal.load(sys.stdin)
> - print d['$what']
> + d = marshal.load(instream)
> + print(d[b'$what'].decode('utf-8'))
>   EOF
>   "$PYTHON_PATH" "$TRASH_DIRECTORY/marshal-dump.py"
>  }
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> index 66d3fc9..eb9a8ed 100755
> --- a/t/t9802-git-p4-filetype.sh
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -223,12 +223,12 @@ build_gendouble() {
>   import sys
>   import struct
>  
> - s = struct.pack(">LL18s",
> + s = struct.pack(b">LL18s",
>   0x00051607,  # AppleDouble
>   0x0002,  # version 2
> - ""   # pad to 26 bytes
> + b""  # pad to 26 bytes
>   )
> - sys.stdout.write(s)
> + getattr(sys.stdout, 'buffer', sys.stdout).write(s)
>   EOF
>  }
--
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: [PATCHv2] clone: add `--shallow-submodules` flag

[Junio C Hamano]

Stefan Beller  writes:

>  I did not rebase this on 85705cfb (Merge branch 'ss/clone-depth-single-doc',
>  2016-01-20) or later, but worked on it with the base unchanged.

Thanks, will replace.

> diff --git a/t/t5614-clone-submodules.sh b/t/t5614-clone-submodules.sh
> new file mode 100755
> index 000..62044c5
> --- /dev/null
> +++ b/t/t5614-clone-submodules.sh
> @@ -0,0 +1,85 @@
> +#!/bin/sh
> +
> +test_description='Test shallow cloning of repos with submodules'
> +
> +. ./test-lib.sh
> +
> +pwd=$(pwd)
> +
> +test_expect_success 'setup' '
> + git checkout -b master &&
> + test_commit commit1 &&
> + test_commit commit2 &&
> + mkdir sub &&
> + (
> + cd sub &&
> + git init &&
> + test_commit subcommit1 &&
> + test_commit subcommit2 &&
> + test_commit subcommit3
> + ) &&
> + git submodule add "file://$pwd/sub" sub &&
> + git commit -m "add submodule"
> +'
> +
> +test_expect_success 'nonshallow clone implies nonshallow submodule' '
> + test_when_finished "rm -rf super_clone" &&
> + git clone --recurse-submodules "file://$pwd/." super_clone &&

All of these "$path/." made me wonder one thing.  I know these URLs
that ends with slash-dot ought to work, but shouldn't they work
without them, too?  The "consistency" in this test that ends
anything that would have ended with "$pwd" with "$pwd/." somewhat
bothered me.

--
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] http: support sending custom HTTP headers

[Jeff King]

On Tue, Apr 26, 2016 at 10:20:19AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > But I think this block (even before my patch) also needs to handle the
> > case where "value" is NULL (presumably by complaining with
> > config_error_nonbool).
> 
> OK, so squashes found to be necessary so far amounts to the attached
> patch.  I still haven't figured out the best way to rephrase the "by
> default" in the proposed log message that made me stutter while
> reading it, though.

I think that part is just trying to explain the implementation hackery.
Maybe something like:

  Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
  a single list, overriding any previous call. This means we have to
  collect _all_ of the headers we want to use into a single list, and
  feed it to curl in one shot. Since we already unconditionally set a
  "pragma" header when initializing the curl handles, we can add our new
  headers to that list.

  For callers which override the default header list (like probe_rpc),
  we provide `http_copy_default_headers()` so they can do the same
  trick.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 07/10] convert: unify the "auto" handling of CRLF

[Junio C Hamano]

Torsten Bögershausen  writes:

>>> diff --git a/convert.c b/convert.c
>>> index 24ab095..3782172 100644
>>> --- a/convert.c
>>> +++ b/convert.c
>>> @@ -227,7 +227,9 @@ static enum eol output_eol(enum crlf_action crlf_action)
>>> return EOL_LF;
>>> case CRLF_UNDEFINED:
>>> case CRLF_AUTO_CRLF:
>>> +   return EOL_CRLF;
>> 
>> Hmph, do we want UNDEFINED case to return EOL_CRLF here?
>> 
>>> case CRLF_AUTO_INPUT:
>>> +   return EOL_LF;
> One of the compilers claimed that UNDEFINED was not handled in switch-case.
> A Warning may be better ? 
>>> case CRLF_TEXT:
>>> case CRLF_AUTO:
>>> /* fall through */

The original made it fall-through; a very simple "a patch that makes
auto=crlf or auto=input do something different shouldn't have any
effect on the undefined case" was what triggered my reaction.

If returning EOL_CRLF does not make any sense for UNDEFINED case, it
shouldn't.  If the codeflow should not reach without crlf_action
computed to something other than UNDEFINED, then you should have a
die("BUG") there.

Looking at a larger context, you already have a warning there:

static enum eol output_eol(enum crlf_action crlf_action)
{
switch (crlf_action) {
case CRLF_BINARY:
return EOL_UNSET;
...
case CRLF_AUTO:
/* fall through */
return text_eol_is_crlf() ? EOL_CRLF : EOL_LF;
}
warning("Illegal crlf_action %d\n", (int)crlf_action);
return core_eol;
}

which tells me that you shouldn't even have CRLF_UNDEFINED listed if
you have no intention to treat UNDEFINED as a valid input in this
codepath.  Instead just doing

switch (crlf_action) {
case ...: /* handle all valid inputs appropriately */
return ...;
...
default: /* the above should cover all valid cases */
break;
}
warning(...);
return ...;

and not listing CRLF_UNDEFINED in any of the case arms would make
your intention more clear.

>> 
> How about this as the commit message:
> --
> ...
> The 'eol' attribute had higher priority than 'text=auto'. Binary
> files may had been corrupted, and this is not what users expect to happen.
>
> Make the 'eol' attribute to work together with 'text=auto'.

The primary issue I had was that "work together" was a useful
explanation only to those who already understand what you are trying
to do with this patch.

Something like:

Having an "eol" attribute is taken as a declaration that the
path is text.  This may be logical (on a binary blob, you
wouldn't be defining an "eol" attribute in the first place)
but is not very useful if you wanted to say "I do not know
if the path is text; inspect the contents to see if it is
text, and apply this 'eol' conversion only if it is".

"text=auto" attribute combined with an "eol" attribute ought
to have meant that, but it didn't.  Make it so.

would be sufficient without any configuration example, I would think.

> With this change
>
> $ echo "* text=auto eol=crlf" >.gitattributes
> has the same effect as
> $ git config core.autocrlf true
>
> and
>
> $ echo "* text=auto eol=lf" >.gitattributes
> has the same effect as
> $ git config core.autocrlf input
--
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/3] githooks.txt: Amend dangerous advice about 'update' hook ACL

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 25, 2016 at 8:29 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>>> -Another use suggested on the mailing list is to use this hook to
>>> -implement access control which is finer grained than the one
>>> -based on filesystem group.
>>> +Another use for this hook to implement access control which is finer
>>> +grained than the one based on filesystem group. Note that if the user
>>> +pushing has a normal login shell on the machine receiving the push
>>> +implementing access control like this can be trivially bypassed by
>>> +just using not executing the hook. In those cases consider using
>>
>> "by just using not executing the hook."
>>
>> This grammar doesn't make sense. It doesn't quite match what you said
>> in the commit message either.
>>
>>> +e.g. linkgit:git-shell[1] as the login shell to restrict the user's
>>> +access.
>
> While there is nothing technically wrong in what it says, I wonder
> if it is worth to state the obvious.  If one can bypass update hook,
> one can bypass any other hook, so the information does not even
> belong here.
>
> Instead of saying "acl can be implemented on top of update hook, but
> not quite because you can bypass it", it may be more useful to say
> "in an environment that restricts the users' access only to git
> commands over the wire, this hook can be used to access control
> without relying on filesystem ownership and group membership",
> perhaps?

Will fix to use something closer to that phrasing.
--
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] http: Support sending custom HTTP headers

[Jeff King]

On Tue, Apr 26, 2016 at 05:33:33PM +0200, Johannes Schindelin wrote:

> Testing the headers? I dunno, do we have tests for that already? I thought
> we did not: it requires an HTTP server (so that the headers are actually
> sent) that we can force to check the header...
> 
> So I see we have some tests that use Apache, and one that uses our own
> http-backend. But is there already anything that logs HTTP requests? I did
> not think so, please correct me if I am wrong.

You can ask apache to check for specific headers. Like this:

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 9317ba0..de5a8fe 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,6 +102,12 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_HTTP_EXPORT_ALL
Header set Set-Cookie name=value
 
+
+   Require expr %{HTTP:x-magic-one} == 'abra'
+   Require expr %{HTTP:x-magic-two} == 'cadabra'
+   SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+   SetEnv GIT_HTTP_EXPORT_ALL
+
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 58207d8..e44fe72 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous 
ref negotiation' '
test_line_count = 10 tags
 '
 
+test_expect_success 'custom http headers' '
+   test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+   git -c http.extraheader="x-magic-one: abra" \
+   -c http.extraheader="x-magic-two: cadabra" \
+   fetch "$HTTPD_URL/smart_headers/repo.git"
+'
+
 stop_httpd
 test_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

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Tue, Apr 26, 2016 at 12:51 AM, Xiaolong Ye  wrote:
> Thanks for Junio's reviews and suggestions.
>
> This version contains the following changes since v5:
>
>  - Fix a decl-after-statement in patch 3/4.
>
>  - Improve testcases to cover more scenarios and make them more portable and
>readable.
>
> Thanks,
> Xiaolong
>

Thanks for this feature!

I am playing around with this series, and here comes a feature request:
I have a local branch with no upstream set. My usual workflow is like this

git checkout origin/master
# toy around, do stuff
git checkout -b new-shiny-feature
git format-patch origin-master..

Now I have set the format.useautobase option and then the `git format-patch`
fails with

fatal: Failed to get upstream, if you want to record base commit
automatically,
please use git branch --set-upstream-to to track a remote branch.
Or you could specify base commit by --base= manually.

but as I indicated I want patches from origin/master onwards,
Could we make use of that information? To record the base in my workflow
currently I need to do:

git format-patch origin/master.. --base=origin/master

which seems redundant to me.
(I may be holding it wrong though? Should I try to set upstream
branches for my local branches? This seems weird to me as I cannot
push/change the upstream branches directly, as Junio owns the branches)

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] http: support sending custom HTTP headers

[Junio C Hamano]

Jeff King  writes:

> But I think this block (even before my patch) also needs to handle the
> case where "value" is NULL (presumably by complaining with
> config_error_nonbool).

OK, so squashes found to be necessary so far amounts to the attached
patch.  I still haven't figured out the best way to rephrase the "by
default" in the proposed log message that made me stutter while
reading it, though.


 http.c | 13 ++---
 http.h |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 6c4d2ed..aae9944 100644
--- a/http.c
+++ b/http.c
@@ -325,8 +325,15 @@ static int http_options(const char *var, const char 
*value, void *cb)
}
 
if (!strcmp("http.extraheader", var)) {
-   extra_http_headers =
-   curl_slist_append(extra_http_headers, value);
+   if (!value) {
+   return config_error_nonbool(var);
+   } else if (!*value) {
+   curl_slist_free_all(extra_http_headers);
+   extra_http_headers = NULL;
+   } else {
+   extra_http_headers =
+   curl_slist_append(extra_http_headers, value);
+   }
return 0;
}
 
@@ -1172,7 +1179,7 @@ int run_one_slot(struct active_request_slot *slot,
return handle_curl_result(results);
 }
 
-struct curl_slist *http_copy_default_headers()
+struct curl_slist *http_copy_default_headers(void)
 {
struct curl_slist *headers = NULL, *h;
 
diff --git a/http.h b/http.h
index 5f13695..36f558b 100644
--- a/http.h
+++ b/http.h
@@ -106,7 +106,7 @@ extern void step_active_slots(void);
 extern void http_init(struct remote *remote, const char *url,
  int proactive_auth);
 extern void http_cleanup(void);
-extern struct curl_slist *http_copy_default_headers();
+extern struct curl_slist *http_copy_default_headers(void);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
--
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] http: support sending custom HTTP headers

[Jeff King]

On Tue, Apr 26, 2016 at 10:03:14AM -0700, Junio C Hamano wrote:

> > +http.extraHeader::
> > +   Pass an additional HTTP header when communicating with a server.  If
> > +   more than one such entry exists, all of them are added as extra headers.
> > +   This feature is useful e.g. to increase security, or to allow
> > +   time-limited access based on expiring tokens.
> > +
> 
> I think one-time/short-lived use case does not want to have this in
> a configuration file, and instead want to do the command line thing
> you illustrated in the proposed log message.  I however wonder if
> there are other use cases where having this in $GIT_DIR/config for
> repeated use is useful.  If there is, not being able to override a
> configured value per invocation would become a problem.
> 
> Peff, what do you think?  I vaguely recollect that you did a hack to
> one variable that declares "an empty value means discard accumulated
> values so far" or something like that, and this variable deserves a
> mechanism like that, too.

Yes, it was for credential.helper. I think the _implementation_ is a
hack (because each callback has to handle it individually), but the
user-facing part is reasonably elegant, given the constraint of not
introducing new syntax.

In this case it would be something like:

diff --git a/http.c b/http.c
index 3d662bb..a7a4be5 100644
--- a/http.c
+++ b/http.c
@@ -325,8 +325,13 @@ static int http_options(const char *var, const char 
*value, void *cb)
}
 
if (!strcmp("http.extraheader", var)) {
-   extra_http_headers =
-   curl_slist_append(extra_http_headers, value);
+   if (*value)
+   extra_http_headers =
+   curl_slist_append(extra_http_headers, value);
+   else {
+   curl_slist_free_all(extra_http_headers);
+   extra_http_headers = NULL;
+   }
return 0;
}
 

But I think this block (even before my patch) also needs to handle the
case where "value" is NULL (presumably by complaining with
config_error_nonbool).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/4] format-patch: introduce format.useAutoBase configuration

[Stefan Beller]

> +format.useAutoBase::
> +   A boolean value which lets you enable the `--base=auto` option of
> +   format-patch by default.
> +
> +

In case you resend, please use just one empty line here?
(No need to resend because of this alone)
--
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] http: support sending custom HTTP headers

[Junio C Hamano]

Johannes Schindelin  writes:

> We introduce a way to send custom HTTP headers with all requests.
>
> This allows us, for example, to send an extra token from build agents
> for temporary access to private repositories. (This is the use case that
> triggered this patch.)
>
> This feature can be used like this:
>
>   git -c http.extraheader='Secret: sssh!' fetch $URL $REF
>
> As `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` overrides previous
> calls' headers (instead of appending the headers, as this unsuspecting
> developer thought initially), we piggyback onto the `Pragma:` setting by
> default, and introduce the global helper `http_copy_default_headers()`
> to help functions that want to specify HTTP headers themselves.

My reading stuttered at "we piggyback onto the `Pragma:` setting by
default", which made me stop and wonder if a description about a
knob that changes this behaviour and makes us piggyback onto
something else follows.

I guess "by default" you meant that the majority of codepaths set
the headers using no_pragma_header or pragma_header variables, and
by [no_]pragma_header to mean more than just about "Pragma:" by
adding the extra headers to them, you did not have to touch them.
Other codepaths that do not use these two variables but start from
NULL you made them start from these extra headers.

Which makes sense.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 42d2b50..37b9af7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1655,6 +1655,12 @@ http.emptyAuth::
>   a username in the URL, as libcurl normally requires a username for
>   authentication.
>  
> +http.extraHeader::
> + Pass an additional HTTP header when communicating with a server.  If
> + more than one such entry exists, all of them are added as extra headers.
> + This feature is useful e.g. to increase security, or to allow
> + time-limited access based on expiring tokens.
> +

I think one-time/short-lived use case does not want to have this in
a configuration file, and instead want to do the command line thing
you illustrated in the proposed log message.  I however wonder if
there are other use cases where having this in $GIT_DIR/config for
repeated use is useful.  If there is, not being able to override a
configured value per invocation would become a problem.

Peff, what do you think?  I vaguely recollect that you did a hack to
one variable that declares "an empty value means discard accumulated
values so far" or something like that, and this variable deserves a
mechanism like that, too.

> diff --git a/http.c b/http.c
> index 4304b80..3d662bb 100644
> --- a/http.c
> +++ b/http.c
> @@ -1163,6 +1175,16 @@ int run_one_slot(struct active_request_slot *slot,
>   return handle_curl_result(results);
>  }
>  
> +struct curl_slist *http_copy_default_headers()

struct curl_slist *http_copy_default_headers(void)

> +{
> + struct curl_slist *headers = NULL, *h;
> +
> + for (h = extra_http_headers; h; h = h->next)
> + headers = curl_slist_append(headers, h->data);
> +
> + return headers;
> +}
> +
> diff --git a/http.h b/http.h
> index 4ef4bbd..5f13695 100644
> --- a/http.h
> +++ b/http.h
> @@ -106,6 +106,7 @@ extern void step_active_slots(void);
>  extern void http_init(struct remote *remote, const char *url,
> int proactive_auth);
>  extern void http_cleanup(void);
> +extern struct curl_slist *http_copy_default_headers();

extern struct curl_slist *http_copy_default_headers(void);
--
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] http: Support sending custom HTTP headers

[Jeff King]

On Tue, Apr 26, 2016 at 05:37:32PM +0200, Johannes Schindelin wrote:

> > I think that's really the only sane way to do it because of curl's
> > interfaces. But maybe it is worth a comment either here, or along with
> > http_get_default_headers(), or both.
> 
> I chose to rename it to http_copy_default_headers(); That should make it
> easier to understand.

Thanks, I think that makes it clearer.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] githooks.txt: Minor improvements to the grammar & phrasing

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 25, 2016 at 8:33 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason   writes:
>
>> -This hook is invoked by 'git am' script.  It takes a single
>> +This hook is invoked by 'git am'.  It takes a single
>
> Good, as it does not matter to the readers that "am" happens to be
> implemented as a script.

Yeah, no need to mention it being a script. But aside from that I was
mainly trying to fix grammar errors:

* Incorrect: "This is invoked by 'git am' script"
* Correct: "This is invoked by the 'git am' script"
* Correct: "This is invoked by a 'git am' script"

I.e. a few things in these docs were missing the definite/indefinite article.

>>  parameter, the name of the file that holds the proposed commit
>> -log message.  Exiting with non-zero status causes
>> -'git am' to abort before applying the patch.
>> +log message.  Exiting with non-zero causes 'git am' to abort
>> +before applying the patch.
>
> I am not sure why you dropped "status" from here.  The result is
> harder to grok, at least to me.  Perhaps "with a non-zero status"?

I'm not 100% sure about this actually, but I thought:

* Incorrect: "Exiting with non-zero status"
* Correct: "Exiting with non-zero"
* Correct: "Exiting with a non-zero status".

I.e. the first one is missing an "a", I thought I'd just make it more
brief, but sure, I'll make it "a non-zero status".

>>  The hook is allowed to edit the message file in place, and can
>> -be used to normalize the message into some project standard
>> -format (if the project has one). It can also be used to refuse
>> -the commit after inspecting the message file.
>> +be used to e.g. normalize the message into some project standard
>> +format. It can also be used to refuse the commit after inspecting
>> +the message file.
>
> OK.  Or we can even drop "e.g." and the result still reads perfectly
> fine.

Done, and willdo too for the other occurrence.

>>  This hook is invoked by 'git commit', and can be bypassed
>> -with `--no-verify` option.  It takes no parameter, and is
>> +with the `--no-verify` option.  It takes no parameters, and is
>>  invoked before obtaining the proposed commit log message and
>> -making a commit.  Exiting with non-zero status from this script
>> -causes the 'git commit' to abort.
>> +making a commit.  Exiting with a non-zero status from this script
>> +causes the 'git commit' command to abort before creating a commit.
>
> Good.  And you kept "status" here, which is doubly 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: [PATCH 18/83] builtin/apply: move 'numstat' global into 'struct apply_state'

[Christian Couder]

On Mon, Apr 25, 2016 at 11:40 PM, Stefan Beller  wrote:
> On Sun, Apr 24, 2016 at 6:33 AM, Christian Couder
>  wrote:
>> Signed-off-by: Christian Couder 
>> ---
>>  builtin/apply.c | 11 ++-
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index d90948a..16d78f9 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -36,6 +36,9 @@ struct apply_state {
>> /* --stat does just a diffstat, and doesn't actually apply */
>> int diffstat;
>>
>> +   /* --numstat does numeric diffstat, and doesn't actually apply */
>> +   int numstat;
>> +
>> /*
>>  *  --check turns on checking that the working tree matches the
>>  *files that are being modified, but doesn't apply the patch
>> @@ -51,14 +54,12 @@ struct apply_state {
>>  };
>>
>>  /*
>> - *  --numstat does numeric diffstat, and doesn't actually apply
>>   *  --index-info shows the old and new index info for paths if available.
>>   */
>>  static int newfd = -1;
>>
>>  static int state_p_value = 1;
>>  static int p_value_known;
>> -static int numstat;
>>  static int summary;
>>  static int apply = 1;
>>  static int no_add;
>> @@ -4500,7 +4501,7 @@ static int apply_patch(struct apply_state *state,
>> if (state->diffstat)
>> stat_patch_list(list);
>>
>> -   if (numstat)
>> +   if (state->numstat)
>> numstat_patch_list(list);
>>
>> if (summary)
>> @@ -4598,7 +4599,7 @@ int cmd_apply(int argc, const char **argv, const char 
>> *prefix_)
>> N_("instead of applying the patch, output diffstat 
>> for the input")),
>> OPT_NOOP_NOARG(0, "allow-binary-replacement"),
>> OPT_NOOP_NOARG(0, "binary"),
>> -   OPT_BOOL(0, "numstat", ,
>> +   OPT_BOOL(0, "numstat", ,
>> N_("show number of added and deleted lines in 
>> decimal notation")),
>> OPT_BOOL(0, "summary", ,
>> N_("instead of applying the patch, output a summary 
>> for the input")),
>> @@ -4675,7 +4676,7 @@ int cmd_apply(int argc, const char **argv, const char 
>> *prefix_)
>> }
>> if (state.apply_with_reject)
>> apply = state.apply_verbosely = 1;
>> -   if (!force_apply && (state.diffstat || numstat || summary || 
>> state.check || fake_ancestor))
>> +   if (!force_apply && (state.diffstat || state.numstat || summary || 
>> state.check || fake_ancestor))
>
> Mental note: This patch is just doing a mechanical conversion, so it
> is fine to check for many "state.FOOs" here.
>
> However later we may want to move this out to a static oneliner like:
>
> static int really_apply(state *s) {
>   return s->diffstat || s->numstat || ...;
> }
>
> (with a better name obviously)

Yeah, this is another cleanup that could be done.
I added it to a list and will try to take care of it later.
--
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 v7 07/10] convert: unify the "auto" handling of CRLF

[Torsten Bögershausen]

On 25.04.16 21:37, Junio C Hamano wrote:
> tbo...@web.de writes:
Thanks for review.
Some comments are inline, and a suggestion to a new commit message at the end.
[]
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 4a27ad4..9caf6ae 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -389,13 +389,13 @@ file with mixed line endings would be reported by the 
>> `core.safecrlf`
>>  mechanism.
>>  
>>  core.autocrlf::
>> -Setting this variable to "true" is almost the same as setting
>> -the `text` attribute to "auto" on all files except that text
>> -files are not guaranteed to be normalized: files that contain
>> -`CRLF` in the repository will not be touched.  Use this
>> -setting if you want to have `CRLF` line endings in your
>> -working directory even though the repository does not have
>> -normalized line endings.  This variable can be set to 'input',
>> +Setting this variable to "true" is the same as setting
>> +the `text` attribute to "auto" on all files and core.eol to "crlf".
>> +Set to true if you want to have `CRLF` line endings in your
>> +working directory and the repository has LF line endings.
>> +Text files are guaranteed not to be normalized: files that contain
>> +`CRLF` in the repository will not be touched.
> 
> This is not a new problem but the last sentence and a half look
> bad.  Telling readers "X is not guaranteed to happen" is not all
> that useful--telling them what happens is.  Also the use of colon
> there is probably ungrammatical.

> 
>   Set to true if you want to have CRLF line endings in your
>   working directory and LF line endings in the repository.
>   Text files that contain CRLF in the repository will not get
>   their end-of-line converted.
> 
> perhaps?
OK, but may be 
s/end-of-line/line endings/
--
Set to true if you want to have CRLF line endings in your
working directory and LF line endings in the repository.
Text files that contain CRLF in the repository will not get
their line endings converted.


> 
>> diff --git a/convert.h b/convert.h
>> index ccf436b..81b6cdf 100644
>> --- a/convert.h
>> +++ b/convert.h
>> @@ -7,7 +7,8 @@
>>  enum safe_crlf {
>>  SAFE_CRLF_FALSE = 0,
>>  SAFE_CRLF_FAIL = 1,
>> -SAFE_CRLF_WARN = 2
>> +SAFE_CRLF_WARN = 2,
>> +SAFE_CRLF_RENORMALIZE = 4
> 
> Are these bits in a word?  If not where did 3 go?
We use an enum, sometimes mixed with an int, so my brain automatically
changed it into a bit field.
"3" is probably better.
> 
>> diff --git a/convert.c b/convert.c
>> index 24ab095..3782172 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -227,7 +227,9 @@ static enum eol output_eol(enum crlf_action crlf_action)
>>  return EOL_LF;
>>  case CRLF_UNDEFINED:
>>  case CRLF_AUTO_CRLF:
>> +return EOL_CRLF;
> 
> Hmph, do we want UNDEFINED case to return EOL_CRLF here?
> 
>>  case CRLF_AUTO_INPUT:
>> +return EOL_LF;
One of the compilers claimed that UNDEFINED was not handled in switch-case.
A Warning may be better ? 
>>  case CRLF_TEXT:
>>  case CRLF_AUTO:
>>  /* fall through */
> 
How about this as the commit message:
--

convert: unify the "auto" handling of CRLF

From: Torsten Bögershausen 

Before this change,
$ echo "* text=auto" >.gitattributes
$ echo "* eol=crlf" >>.gitattributes
would have the same effect as
$ echo "* text" >.gitattributes
$ git config core.eol crlf

The 'eol' attribute had higher priority than 'text=auto'. Binary
files may had been corrupted, and this is not what users expect to happen.

Make the 'eol' attribute to work together with 'text=auto'.

With this change

$ echo "* text=auto eol=crlf" >.gitattributes
has the same effect as
$ git config core.autocrlf true

and

$ echo "* text=auto eol=lf" >.gitattributes
has the same effect as
$ git config core.autocrlf input



--
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] hooks: Add ability to specify where the hook directory is

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 25, 2016 at 10:33 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason   writes:
>
>> +core.hooksPath::
>> + By default Git will look for your hooks in the
>> + '$GIT_DIR/hooks' directory. Set this to different path,
>> + e.g. '/etc/git/hooks', and Git will try to find your hooks in
>> + that directory, e.g. '/etc/git/hooks/pre-receive' instead of
>> + in '$GIT_DIR/hooks/pre-receive'.
>> ++
>> +The path can either be absolute or relative. In the latter case see
>> +the discussion in the "DESCRIPTION" section of linkgit:githooks[5]
>> +about what the relative path will be relative to.
>
> ... which does not seem to appear there, it seems?

I think it does. Read on...

>>  DESCRIPTION
>>  ---
>>
>> -Hooks are programs you can place in the `$GIT_DIR/hooks` directory to
>> -trigger action at certain points. Hooks that don't have the executable
>> -bit set are ignored.
>> +Hooks are programs you can place in a hooks directory to trigger action
>> +at certain points. Hooks that don't have the executable bit set are
>> +ignored.
>> +
>> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be
>> +changed via the `core.hooksPath` configuration variable (see
>> +linkgit:git-config[1]).
>
> The section talks about what the cwd of the process that runs the
> hook is, but it is not clear at all from these three lines in
> core.hooksPath description above how the cwd of the process is
> related with the directory the relative path will be relative to.

I think the documentation mostly makes sense, but that the context of
this patch is confusing.

I.e. when I say:

> The path can either be absolute or relative. In the latter case see
> the discussion in the "DESCRIPTION" section of linkgit:githooks[5]
> about what the relative path will be relative to.

In config.txt, I'm not talking about the patch to githooks.txt I'm
adding in this commit, but the first patch in the githooks.txt series,
i.e. this section:

> When a hook is called in a non-bare repository the working directory
> is guaranteed to be the root of the working tree, in a bare repository
> the working directory will be the path to the repository. I.e. hooks
> don't need to worry about the user's current working directory.

I.e. I'm not talking about the "by default the hooks directory [blah
blah]" part I'm adding here.

> Unless the readers know that the implementation makes sure that the
> process chdir'ed to that final directory before calling find_hook(),
> that is.  And I do not think you want to assume that knowledge on
> the side of the readers.

This should not be explicitly covered in githooks.txt in my previous
series per the above..

>> diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
>> new file mode 100755
>> index 000..31461aa
>> --- /dev/null
>> +++ b/t/t1350-config-hooks-path.sh
>> @@ -0,0 +1,33 @@
>> +#!/bin/sh
>> +
>> +test_description='Test the core.hooksPath configuration variable'
>> +
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'set up a pre-commit hook in core.hooksPath' '
>> + mkdir -p .git/custom-hooks .git/hooks &&
>> + write_script .git/custom-hooks/pre-commit <> +printf "%s" "." >>.git/PRE-COMMIT-HOOK-WAS-CALLED
>> +EOF
>
> Because there is no funny interpolation going on, it would help
> readers to signal that fact by quoting the end-of-here-text marker.
> And it makes the entire test block reads nicer if you indent the
> body of hte here-text by prefixing the end-of-here-text marker with
> a dash, i.e.

Yes this and all the other stuff you and Johannes mentioned in this
thread just made no sense and was just some brainfart of mine. I'm
fixing all these issues up for the next re-send.

> write_script .git/custom-hooks/pre-commit <<-\EOF &&
> printf "%s" "." >>.git/PRE-COMMIT-HOOK-WAS-CALLED
> EOF
>
>> + cat >.git/hooks/pre-commit <> + write_script .git/hooks/pre-commit &&
>> +printf "%s" "SHOULD NOT BE CALLED" >>.git/PRE-COMMIT-HOOK-WAS-CALLED
>> +EOF
>> + chmod +x .git/custom-hooks/pre-commit
>
> You didn't want cat and chmod here?
>
>> +'
>> +
>> +test_expect_success 'Check that various forms of specifying core.hooksPath 
>> work' '
>> + test_commit no_custom_hook &&
>> + git config core.hooksPath .git/custom-hooks &&
>> + test_commit have_custom_hook &&
>> + git config core.hooksPath .git/custom-hooks/ &&
>> + test_commit have_custom_hook_trailing_slash &&
>> + git config core.hooksPath "$PWD/.git/custom-hooks" &&
>> + test_commit have_custom_hook_abs_path &&
>> + git config core.hooksPath "$PWD/.git/custom-hooks/" &&
>> + test_commit have_custom_hook_abs_path_trailing_slash &&
>> + printf "%s" "" >.git/PRE-COMMIT-HOOK-WAS-CALLED.expect &&
>> + test_cmp .git/PRE-COMMIT-HOOK-WAS-CALLED.expect 
>> .git/PRE-COMMIT-HOOK-WAS-CALLED
>> +'
>> +
>> +test_done
--
To unsubscribe from this list: send the line "unsubscribe git" 

Re: [PATCH 09/83] builtin/apply: move 'check' global into 'struct apply_state'

[Stefan Beller]

On Tue, Apr 26, 2016 at 9:26 AM, Christian Couder
 wrote:
>>
>>>  /*
>>> - *  --check turns on checking that the working tree matches the
>>> - *files that are being modified, but doesn't apply the patch
>>
>> Oh I see it was moved from here. Not sure if we want to rename
>> comments along the way or just keep it in this series.
>
> I kept the existing comments when they were still relevant.
> It could be a cleanup to change them to something like what you
> suggest, but as it is not important for this series which is already
> long, I prefer to leave it for now.

Yeah that was my conclusion as well after some thought.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/83] builtin/apply: move 'check' global into 'struct apply_state'

[Christian Couder]

On Mon, Apr 25, 2016 at 8:57 PM, Stefan Beller  wrote:
> On Sun, Apr 24, 2016 at 6:33 AM, Christian Couder
>  wrote:
>> Signed-off-by: Christian Couder 
>> ---
>>  builtin/apply.c | 16 +---
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index ad81210..6c628f6 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -25,12 +25,15 @@ struct apply_state {
>> const char *prefix;
>> int prefix_length;
>>
>> +   /*
>> +*  --check turns on checking that the working tree matches the
>> +*files that are being modified, but doesn't apply the patch
>
> This is true, but at this part of the file/code we rather want to know what
> `check` does, instead of what the command line option --check does.
> (They are 2 different things, though one leading to the other one?) How about:
>
> /*
>  * Only check the files to be modified, but do not modify the files.
>  */
>
>
>>  /*
>> - *  --check turns on checking that the working tree matches the
>> - *files that are being modified, but doesn't apply the patch
>
> Oh I see it was moved from here. Not sure if we want to rename
> comments along the way or just keep it in this series.

I kept the existing comments when they were still relevant.
It could be a cleanup to change them to something like what you
suggest, but as it is not important for this series which is already
long, I prefer to leave it for now.
--
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] http: Support sending custom HTTP headers

[Junio C Hamano]

Johannes Schindelin  writes:

> Testing the headers? I dunno, do we have tests for that already? I thought
> we did not: it requires an HTTP server (so that the headers are actually
> sent) that we can force to check the header...
>
> So I see we have some tests that use Apache, and one that uses our own
> http-backend. But is there already anything that logs HTTP requests? I did
> not think so, please correct me if I am wrong.

I suspect that no codepath in the current system has cared about
what http headers are sent; auth stuff might have but even then I
would imagine that a test for auth would observe the end result
(i.e. it would ask "did the server accept or reject us?") not the
mechanism (i.e. it would not ask "did we correctly send an
Authorization header?").

So I wouldn't be surprised if this topic is the first one that cares
exactly what headers are sent out (eh, rather, "we told Git to send
this and that header, do they appear at the server end?"), in which
case it is very likely that we do not have any existing test that
can be imitated for that purpose X-<.

In other words, the answer to "Do we already have a test so that I
can mimick it instead of thinking of a way to test this?" would
probably be "No".

Do we care about this feature deeply enough to devise a mechanism
to prevent it from getting broken by careless others 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 05/83] builtin/apply: extract line_by_line_fuzzy_match() from match_fragment()

[Christian Couder]

On Mon, Apr 25, 2016 at 8:50 PM, Stefan Beller  wrote:
>> @@ -2251,7 +2319,7 @@ static int match_fragment(struct image *img,
>>   int match_beginning, int match_end)
>>  {
>> int i;
>> -   char *fixed_buf, *buf, *orig, *target;
>> +   char *fixed_buf, *orig, *target;
>> struct strbuf fixed;
>> size_t fixed_len, postlen;
>> int preimage_limit;
>> @@ -2312,6 +2380,7 @@ static int match_fragment(struct image *img,
>>  * There must be one non-blank context line that match
>>  * a line before the end of img.
>>  */
>> +   char *buf;
>
> patches 1-4 looking good, with no comment from me. Here is the first spot to
> comment on.
>
> It's not clear why we need to declare buf here? Oh wait it is. It's just
> moved from the start of the function. But why do it in this patch?
> It seems unrelated to the general intent of the patch. No need to reroll
> for this nit alone, it just took me a while to figure out it was an unrelated
> thing.

Yeah, I agree it's a bit unrelated. But rather than add another patch
to an already long series just for this, I added the following to the
commit message:

While at it, let's reduce the scope of "char *buf" in match_fragment().
--
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: "gitk --author=foo" shows also parent

[Nikolai Kosjar]

On 04/26/2016 04:06 PM, Mike Rappazzo wrote:

On Tue, Apr 26, 2016 at 9:08 AM, Nikolai Kosjar  wrote:

Hi!

 $ gitk --author=foo

...seems to show also the parent of each author-matched commit, whereas

 $ git log --author=foo

does not. Is this intended or a bug? I've stumbled over this while
configuring a gitk view with the author field.


I believe that this is intentional.  Notice that the parent commit's
circle is just outlined
compared to the selected authored commits are filled.  I consider this
the context
of the commits you are looking at.


Hmm, then I'm not interested in the context since it's too noisy. Is 
there any way to suppress this?


Nikolai

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 26, 2016 at 3:40 PM, Marc Branchaud  wrote:
> On 2016-04-26 06:58 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> Makes sense to have an experimental.* config tree for git for stuff like 
>> this.
>
> I disagree.
>
> * If the point is to express some kind of warning to users, I think the
> community has been much better served by leaving experimental settings
> undocumented (or documented only in unmerged topic branches).  It feels like
> an experimental.* tree is a doorway to putting experimental features in
> official releases, which seems odd considering that (IMHO) git has so far
> done very well with the carefully-planned-out integration of all sorts of
> features.
>
> * Part of the experiment is coming up with appropriate configuration knobs,
> including where those knobs should live.  Often such considerations lead to a
> better implementation for the feature.  Dumping things into an experimental.*
> tree would merely postpone that part of the feature's design.
>
> * Such a tree creates a flag day when the experimental feature eventually
> becomes a "real" feature. That'll annoy any early adopters. Sure, they
> *should* be prepared to deal with config tree bike-shedding, but still that
> extra churn seems unnecessary.

By "stuff like this", yeah I did mean, and I assume Junio means,
putting "experimental" features in official releases.

E.g. perl does this, if you type "perldoc experimental" on a Linux box
you'll get the documentation.

Basically you can look at patches to a project on a spectrum of:

 1. You hacked something up locally
 2. It's in someone's *.git repo as a POC
 3. It's a third-party patch series used by a bunch of people
 4. In an official release but documented as experimental
 5. In an official release as a first-rate feature

Something like an experimental.WHATEVER=bool flag provides #4.

I think aside from this feature just leaving these things undocumented
really provides the worst of both worlds.

Now you have some feature that's undocumented *because* you're not
sure about it, but you can't ever be sure about it unless people
actually use it, and if it's not documented at all effectively it's as
visible as some third-party patch series. I.e. only people really
involved in the project will ever use it.

Which is why perl has the "experimental" subsystem, it allows for
playing around with features the maintainers aren't quite sure about
in official releases, and the users know they're opting in to trying
something unstable that may go away or have its semantics changed from
under them.
--
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] string_list: use string-list API in unsorted_string_list_lookup()

[Johannes Schindelin]

Hi Ralf,

On Mon, 25 Apr 2016, Ralf Thielow wrote:

> Using the string-list API in function unsorted_string_list_lookup()
> makes the code more readable.
> 
> Signed-off-by: Ralf Thielow 
> ---
> Changes since v1:
> - remove extra curly braces

ACK!
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] http: support sending custom HTTP headers

[Johannes Schindelin]

We introduce a way to send custom HTTP headers with all requests.

This allows us, for example, to send an extra token from build agents
for temporary access to private repositories. (This is the use case that
triggered this patch.)

This feature can be used like this:

git -c http.extraheader='Secret: sssh!' fetch $URL $REF

As `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` overrides previous
calls' headers (instead of appending the headers, as this unsuspecting
developer thought initially), we piggyback onto the `Pragma:` setting by
default, and introduce the global helper `http_copy_default_headers()`
to help functions that want to specify HTTP headers themselves.

Signed-off-by: Johannes Schindelin 
---

Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v2

 Documentation/config.txt |  6 ++
 http-push.c  | 10 +-
 http.c   | 28 +---
 http.h   |  1 +
 remote-curl.c|  4 ++--
 5 files changed, 39 insertions(+), 10 deletions(-)

Interdiff vs v1:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index 42d2b50..37b9af7 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1655,6 +1655,12 @@ http.emptyAuth::
a username in the URL, as libcurl normally requires a username for
authentication.
  
 +http.extraHeader::
 +  Pass an additional HTTP header when communicating with a server.  If
 +  more than one such entry exists, all of them are added as extra headers.
 +  This feature is useful e.g. to increase security, or to allow
 +  time-limited access based on expiring tokens.
 +
  http.cookieFile::
File containing previously stored cookie lines which should be used
in the Git http session, if they match the server. The file format
 diff --git a/http-push.c b/http-push.c
 index 04eef17..ae2b7f1 100644
 --- a/http-push.c
 +++ b/http-push.c
 @@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
  static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, 
enum dav_header_flag options)
  {
struct strbuf buf = STRBUF_INIT;
 -  struct curl_slist *dav_headers = http_get_default_headers();
 +  struct curl_slist *dav_headers = http_copy_default_headers();
  
if (options & DAV_HEADER_IF) {
strbuf_addf(, "If: (<%s>)", lock->token);
 @@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
  static void start_move(struct transfer_request *request)
  {
struct active_request_slot *slot;
 -  struct curl_slist *dav_headers = http_get_default_headers();
 +  struct curl_slist *dav_headers = http_copy_default_headers();
  
slot = get_active_slot();
slot->callback_func = process_response;
 @@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, 
long timeout)
char *ep;
char timeout_header[25];
struct remote_lock *lock = NULL;
 -  struct curl_slist *dav_headers = http_get_default_headers();
 +  struct curl_slist *dav_headers = http_copy_default_headers();
struct xml_ctx ctx;
char *escaped;
  
 @@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
struct slot_results results;
struct strbuf in_buffer = STRBUF_INIT;
struct buffer out_buffer = { STRBUF_INIT, 0 };
 -  struct curl_slist *dav_headers = http_get_default_headers();
 +  struct curl_slist *dav_headers = http_copy_default_headers();
struct xml_ctx ctx;
struct remote_ls_ctx ls;
  
 @@ -1204,7 +1204,7 @@ static int locking_available(void)
struct slot_results results;
struct strbuf in_buffer = STRBUF_INIT;
struct buffer out_buffer = { STRBUF_INIT, 0 };
 -  struct curl_slist *dav_headers = http_get_default_headers();
 +  struct curl_slist *dav_headers = http_copy_default_headers();
struct xml_ctx ctx;
int lock_flags = 0;
char *escaped;
 diff --git a/http.c b/http.c
 index 02d7147..3d662bb 100644
 --- a/http.c
 +++ b/http.c
 @@ -685,9 +685,9 @@ void http_init(struct remote *remote, const char *url, int 
proactive_auth)
if (remote)
var_override(_proxy_authmethod, 
remote->http_proxy_authmethod);
  
 -  pragma_header = curl_slist_append(http_get_default_headers(),
 +  pragma_header = curl_slist_append(http_copy_default_headers(),
"Pragma: no-cache");
 -  no_pragma_header = curl_slist_append(http_get_default_headers(),
 +  no_pragma_header = curl_slist_append(http_copy_default_headers(),
"Pragma:");
  
  #ifdef USE_CURL_MULTI
 @@ -1175,7 +1175,7 @@ int run_one_slot(struct active_request_slot *slot,
return handle_curl_result(results);
  }
  
 -struct curl_slist *http_get_default_headers()
 +struct curl_slist *http_copy_default_headers()
  {