Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Junio C Hamano]

Stefan Beller  writes:

> So I guess we should test a bit more extensively, maybe
>
> git status >expect
> git submodule embedgitdirs
> git status >actual
> test_cmp expect actual
> # further testing via
> test -f ..
> test -d ..

Something along that line.  "status should succeed" does not tell
the readers what kind of breakage the test is expecting to protect
us from.  If we are expecting a breakage in embed-git-dirs would
somehow corrupt an existing submodule, which would lead to "status"
that is run in the superproject report the submodule differently,
then comparing output before and after the operation may be a
reasonable test.  Going there to the submodule working tree and
checking the health of the repository (of the submodule) may be
another sensible test.

>>  In the
>> extreme, if the failed "git submodule" command did
>>
>> rm -fr .git ?* && git init
>>
>> wouldn't "git status" still succeed?
>
> In that particular case you'd get
> $ git status
> fatal: Not a git repository (or any parent up to mount point )

Even with "&& git init"?  Or you forgot that part?

Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller  wrote:
>> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano  wrote:
>>>
>>> Can the effect of this change demonstrated in a new test?  There
>>> must be a scenario where the current behaviour is broken and this
>>> change fixes an incorrect computation of relative path, no?
>
> I do not think the current usage exposes this bug in
> connect_work_tree_and_git_dir. It is only used in builtin/mv.c,
> which fills the second parameter `git_dir` via a call to read_gitfile,
> which itself produces an absolute path.

OK.  Fixing a potential bug as a preparatory step is good.

>   The current caller of connect_work_tree_and_git_dir passes
>   an absolute path for the `git_dir` parameter. In the future patch
>   we will also pass in relative path for `git_dir`. Extend the functionality
>   of connect_work_tree_and_git_dir to take relative paths for parameters.
>
>   We could work around this in the future patch by computing the absolute
>   path for the git_dir in the calling site, however accepting relative
>   paths for either parameter makes the API for this function easier
>   to use.

Yup, sounds sensible.  Thanks.

DEAR FRIEND.

[Mr Daouda Ali]

Dear Friend,
   I am Mr.Daouda Ali the head of file department of Bank of
Africa(B.O.A) here in Burkina Faso / Ouagadougou. In my department we
discover an abandoned sum of (US$18 million US Dollars) in an account
that belongs to one of our foreign customer who died along with his
family in plane crash. It is therefore upon this discovery that I now
decided to make this business proposal to you and release the money to
you as the next of kin or relation to the deceased for the safety and
subsequent disbursement since nobody is coming for it. I agree that
40% of this money will be for you, while 60% would be for me. Then
after the money is been transferred into your account, I will visit
your country for an investment under your kind control.

You have to contact my Bank directly as the real next of kin of this
deceased account with next of kin application form. You have to send
me those your information below to enable me use it and get you next
of kin application form from bank, so that you will contact Bank for
the transfer of this money into your account.

Your Full Name___
Your Home Address
Your Age ___
Your Handset Number
Your Occupation ___

I am waiting for your urgent respond to enable us proceed further for
the transfer through my private e-mail(daoudaal...@gmail.com)

Yours faithfully,
Mr.Daouda Ali

Re: [PATCHv3 3/3] submodule-config: clarify parsing of null_sha1 element

[Junio C Hamano]

Stefan Beller  writes:

> +Whenever a submodule configuration is parsed in 
> `parse_submodule_config_option`
> +via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.

It will overwrite?  It will be overwritten?  I guess it is the latter?

> +So in the normal case, when HEAD:.gitmodules is parsed first and then 
> overlayed
> +with the repository configuration, the null_sha1 entry contains the local
> +configuration of a submodule (e.g. consolidated values from local git
>  configuration and the .gitmodules file in the worktree).
>  
>  For an example usage see test-submodule-config.c.

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Junio C Hamano]

Jonathan Nieder  writes:

> This trips reproducibly for
>
>   git ls-remote https://kernel.googlesource.com/pub/scm/git/git
>
> when run outside of a git repository.
>
> Backtrace:
>
>   #0  setup_git_env () at environment.c:172
>   #1  get_git_dir () at environment.c:214
>   #2  get_helper at transport-helper.c:127
>   #3  get_refs_list (for_push=0) at transport-helper.c:1038
>   #4  transport_get_remote_refs at transport.c:1076
>   #5  cmd_ls_remote at builtin/ls-remote.c:97
>
> transport-helper.c:127 is
>
>   argv_array_pushf(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
>get_git_dir());
>
> That code is pretty clearly wrong.  Should it be made conditional
> on have_git_dir(), like this?

Looks good and I agree with Peff's analysis.  Care to wrap it in a
patch with a log message?

Thanks.

>
> Thanks,
> Jonathan
>
>  transport-helper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport 
> *transport)
>   helper->git_cmd = 0;
>   helper->silent_exec_failure = 1;
>  
> - argv_array_pushf(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> -  get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(>env_array, "%s=%s",
> +  GIT_DIR_ENVIRONMENT, get_git_dir());
>  
>   code = start_command(helper);
>   if (code < 0 && errno == ENOENT)

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

[Junio C Hamano]

Stefan Beller  writes:

> It is also possible to pass in a tree hash to lookup a submodule config.
> Make it clear by naming the variables accrodingly. Looking up a submodule
> config by tree hash will come in handy in a later patch.
>
> Signed-off-by: Stefan Beller 
> ---

Yeah, I noticed that while reading the previous round, too, but...

> -`const struct submodule *submodule_from_name(const unsigned char 
> *commit_sha1, const char *name)`::
> +`const struct submodule *submodule_from_name(const unsigned char 
> *commit_or_tree, const char *name)`::
>  
>   The same as above but lookup by name.

Doesn't (the) "above", aka submodule_from_path() want the same
treatment?  

Also the explanation of "above" has room for improvement.  Namely it
says:

Lookup values for one submodule by its commit_sha1 and path.

I do not think the commit-sha1 (or commit-or-tree-object-name) is
"ITS" object name for the submodule.  The name belongs to the
containing superproject commit (or tree), no?

Given a tree-ish in the superproject and a path, return the
submodule that is bound at the path in the named tree.

is what I would write for that one.  Thinking about it a bit
further, "treeish_name" would be a much better name for the variable
than "commit_or_tree", as "treeish" is an established short and
sweet word that means "commit_or_tree", and having a "name"
somewhere in the variable name makes it clear that we are not
passing the pointer to an in-core object (e.g. "struct commit *").

> +test_expect_success 'using tree sha1 works' '
> + (
> + cd super &&
> + tree=$(git rev-parse HEAD^{tree}) &&
> + commit=$(git rev-parse HEAD^{commit}) &&
> + test-submodule-config $commit b >expect &&
> + test-submodule-config $tree b >actual &&
> + test_cmp expect actual
> + )
> +'

Perhaps also test a tag that points at the commit?

Re: [PATCHv2 1/3] submodule config: inline config_from_{name, path}

[Junio C Hamano]

Stefan Beller  writes:

> There is no other user of config_from_{name, path}, such that there is no
> reason for the existence of these one liner functions. Just inline these
> to increase readability.
>
> Signed-off-by: Stefan Beller 
> Signed-off-by: Junio C Hamano 
> ---

Makes sense.

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Jeff King]

On Mon, Nov 21, 2016 at 04:44:21PM -0800, Jonathan Nieder wrote:

> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > -   if (!git_dir)
> > +   if (!git_dir) {
> > +   if (!startup_info->have_repository)
> > +   die("BUG: setup_git_env called without repository");
> > git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> > +   }
> 
> This trips reproducibly for
> 
>   git ls-remote https://kernel.googlesource.com/pub/scm/git/git
> 
> when run outside of a git repository.
> 
> Backtrace:
> 
>   #0  setup_git_env () at environment.c:172
>   #1  get_git_dir () at environment.c:214
>   #2  get_helper at transport-helper.c:127
>   #3  get_refs_list (for_push=0) at transport-helper.c:1038
>   #4  transport_get_remote_refs at transport.c:1076
>   #5  cmd_ls_remote at builtin/ls-remote.c:97
> 
> transport-helper.c:127 is
> 
>   argv_array_pushf(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
>get_git_dir());
> 
> That code is pretty clearly wrong.  Should it be made conditional
> on have_git_dir(), like this?

Yeah, I think making it conditional makes the most sense. Just trying to
think of cases that might not be covered by your patch:

  - if we are not in a repository, we shouldn't ever need to override an
existing $GIT_DIR from the environment. Because if $GIT_DIR is
present, then we _would_ be in a repo if it's valid, and die if it
isn't.

  - not setting $GIT_DIR may cause a helper to do the usual discovery
walk to find the repository. But we know we're not in one, or we
would have found it ourselves. So worst case it may expend a little
effort to try to find a repository and fail (I think remote-curl
would do this to try to find repo-level config).

Both of those points assume that we've already called
setup_git_directory_gently(), but I think that's a reasonable
assumption. And certainly is true for ls-remote, and should be for any
git command that invokes the transport code.

> diff --git a/transport-helper.c b/transport-helper.c
> index 91aed35..e4fd982 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport 
> *transport)
>   helper->git_cmd = 0;
>   helper->silent_exec_failure = 1;
>  
> - argv_array_pushf(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
> -  get_git_dir());
> + if (have_git_dir())
> + argv_array_pushf(>env_array, "%s=%s",
> +  GIT_DIR_ENVIRONMENT, get_git_dir());

So yeah, I think this is the extent of the change needed.

Thanks.

-Peff

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Stefan Beller]

On Mon, Nov 21, 2016 at 1:14 PM, Junio C Hamano  wrote:
>
> Does this format correctly?
>
> I somehow thought that second and subsequent paragraphs continued
> with "+" want no indentation before them.  See for example the
> Values section in config.txt and see how entries for boolean:: and
> color:: use multiple '+' paragraphs.
>
> If we do not have to refrain from indenting the second and
> subsequent paragraphs, that would be great for readability, but I
> take the existing practice as telling me that we cannot do that X-<.

Will fix and test in a resend.


>
>> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>> + git init sub2 &&
>> + test_commit -C sub2 first &&
>> + git add sub2 &&
>> + git commit -m superproject
>> +'
>> +
>> +test_expect_success 'intern the git dir fails for incomplete submodules' '
>> + test_must_fail git submodule interngitdirs &&
>> + # check that we did not break the repository:
>> + git status
>> +'
>
> It is not clear what the last "git status" wants to test.

Any errors that I ran into when manually truing to embed a submodules
git dir, were fatal with `git status` already, e.g. missing or wrong
call of connect_work_tree_and_git_dir were my main failure points.

So I guess we should test a bit more extensively, maybe

git status >expect
git submodule embedgitdirs
git status >actual
test_cmp expect actual
# further testing via
test -f ..
test -d ..

>  In the
> extreme, if the failed "git submodule" command did
>
> rm -fr .git ?* && git init
>
> wouldn't "git status" still succeed?

In that particular case you'd get
$ git status
fatal: Not a git repository (or any parent up to mount point )
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
$ echo $?
128

but I get the idea, which is why I propose the double check via status.
That would detect any logical change for the repository, e.g. a
change to the .gitmodules file.

>
> What are the minimum things that we expect from "did not break" to
> see?  sub2/.git is still a directory and is a valid repository?  The
> contents of the .git/modules/* before and after the "git submodule"
> does not change?  Some other things?

I thought about making up a name for such a repo and creating that engry
in .gitmodules. But I refrained from doing so, because it seems too much
for this command.

I dunno, but I would suspect the double status is fine here, too?

[PATCHv3 0/3] submodule-config: clarify/cleanup docs and header

[Stefan Beller]

replacing sb/submodule-config-cleanup

v3:
diff to current origin/sb/submodule-config-cleanup:
diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 1df7a827ff..e06a3fd2de 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -49,17 +49,17 @@ Functions
 
 `const struct submodule *submodule_from_path(const unsigned char 
*commit_or_tree, const char *path)`::
 
-   Lookup values for one submodule by its commit_sha1 and path.
+   Lookup values for one submodule by its commit_or_tree and path.
 
 `const struct submodule *submodule_from_name(const unsigned char 
*commit_or_tree, const char *name)`::
 
The same as above but lookup by name.
 
 Whenever a submodule configuration is parsed in `parse_submodule_config_option`
-via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1
-zeroed out.  So in the normal case, when HEAD:.gitmodules is parsed first and
-then overlayed with the repository configuration, the null_sha1 entry contains
-the local configuration of a submodule (e.g. consolidated values from local git
+via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
+So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
+with the repository configuration, the null_sha1 entry contains the local
+configuration of a submodule (e.g. consolidated values from local git
 configuration and the .gitmodules file in the worktree).
 
 For an example usage see test-submodule-config.c.
diff --git a/submodule-config.c b/submodule-config.c
index 4c5f5d074b..d88a746c56 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -448,7 +448,6 @@ static const struct submodule *config_from(struct 
submodule_cache *cache,
 
/* fill the submodule config into the cache */
parameter.cache = cache;
-   // todo: get the actual tree here:
parameter.commit_or_tree = commit_or_tree;
parameter.gitmodules_sha1 = sha1;
parameter.overwrite = 0;

v2:
addressed Jacobs concerns in patch2, fixing all occurrences of commit_sha1.

Thanks,
Stefan

interdiff to v1:
diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 1df7a827ff..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -49,7 +49,7 @@ Functions

 `const struct submodule *submodule_from_path(const unsigned char 
*commit_or_tree, const char *path)`::

-   Lookup values for one submodule by its commit_sha1 and path.
+   Lookup values for one submodule by its commit_or_tree and path.

 `const struct submodule *submodule_from_name(const unsigned char 
*commit_or_tree, const char *name)`::


v1:
A small series that would have helped me understand the submodule config
once again.

Thanks,
Stefan

Stefan Beller (3):
  submodule config: inline config_from_{name, path}
  submodule-config: rename commit_sha1 to commit_or_tree
  submodule-config: clarify parsing of null_sha1 element

 Documentation/technical/api-submodule-config.txt | 13 --
 submodule-config.c   | 58 ++--
 submodule-config.h   |  4 +-
 t/t7411-submodule-config.sh  | 11 +
 4 files changed, 44 insertions(+), 42 deletions(-)

-- 
2.11.0.rc2.18.g0126045.dirty

[PATCHv3 3/3] submodule-config: clarify parsing of null_sha1 element

[Stefan Beller]

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/technical/api-submodule-config.txt | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 768458580f..e06a3fd2de 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -55,8 +55,11 @@ Functions
 
The same as above but lookup by name.
 
-If given the null_sha1 as commit_or_tree the local configuration of a
-submodule will be returned (e.g. consolidated values from local git
+Whenever a submodule configuration is parsed in `parse_submodule_config_option`
+via e.g. `gitmodules_config()`, it will be overwrite the null_sha1 entry.
+So in the normal case, when HEAD:.gitmodules is parsed first and then overlayed
+with the repository configuration, the null_sha1 entry contains the local
+configuration of a submodule (e.g. consolidated values from local git
 configuration and the .gitmodules file in the worktree).
 
 For an example usage see test-submodule-config.c.
-- 
2.11.0.rc2.18.g0126045.dirty

[PATCHv3 2/3] submodule-config: rename commit_sha1 to commit_or_tree

[Stefan Beller]

It is also possible to pass in a tree hash to lookup a submodule config.
Make it clear by naming the variables accrodingly. Looking up a submodule
config by tree hash will come in handy in a later patch.

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-submodule-config.txt |  8 ++---
 submodule-config.c   | 46 
 submodule-config.h   |  4 +--
 t/t7411-submodule-config.sh  | 11 ++
 4 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..768458580f 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -47,15 +47,15 @@ Functions
Can be passed to the config parsing infrastructure to parse
local (worktree) submodule configurations.
 
-`const struct submodule *submodule_from_path(const unsigned char *commit_sha1, 
const char *path)`::
+`const struct submodule *submodule_from_path(const unsigned char 
*commit_or_tree, const char *path)`::
 
-   Lookup values for one submodule by its commit_sha1 and path.
+   Lookup values for one submodule by its commit_or_tree and path.
 
-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, 
const char *name)`::
+`const struct submodule *submodule_from_name(const unsigned char 
*commit_or_tree, const char *name)`::
 
The same as above but lookup by name.
 
-If given the null_sha1 as commit_sha1 the local configuration of a
+If given the null_sha1 as commit_or_tree the local configuration of a
 submodule will be returned (e.g. consolidated values from local git
 configuration and the .gitmodules file in the worktree).
 
diff --git a/submodule-config.c b/submodule-config.c
index 15ffab6af4..d88a746c56 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -263,12 +263,12 @@ int parse_push_recurse_submodules_arg(const char *opt, 
const char *arg)
return parse_push_recurse(opt, arg, 1);
 }
 
-static void warn_multiple_config(const unsigned char *commit_sha1,
+static void warn_multiple_config(const unsigned char *commit_or_tree,
 const char *name, const char *option)
 {
const char *commit_string = "WORKTREE";
-   if (commit_sha1)
-   commit_string = sha1_to_hex(commit_sha1);
+   if (commit_or_tree)
+   commit_string = sha1_to_hex(commit_or_tree);
warning("%s:.gitmodules, multiple configurations found for "
"'submodule.%s.%s'. Skipping second one!",
commit_string, name, option);
@@ -276,7 +276,7 @@ static void warn_multiple_config(const unsigned char 
*commit_sha1,
 
 struct parse_config_parameter {
struct submodule_cache *cache;
-   const unsigned char *commit_sha1;
+   const unsigned char *commit_or_tree;
const unsigned char *gitmodules_sha1;
int overwrite;
 };
@@ -300,7 +300,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->path)
-   warn_multiple_config(me->commit_sha1, submodule->name,
+   warn_multiple_config(me->commit_or_tree, 
submodule->name,
"path");
else {
if (submodule->path)
@@ -314,7 +314,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
int die_on_error = is_null_sha1(me->gitmodules_sha1);
if (!me->overwrite &&
submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)
-   warn_multiple_config(me->commit_sha1, submodule->name,
+   warn_multiple_config(me->commit_or_tree, 
submodule->name,
"fetchrecursesubmodules");
else
submodule->fetch_recurse = parse_fetch_recurse(
@@ -324,7 +324,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->ignore)
-   warn_multiple_config(me->commit_sha1, submodule->name,
+   warn_multiple_config(me->commit_or_tree, 
submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
 strcmp(value, "dirty") &&
@@ -340,7 +340,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
if (!value) {
ret = config_error_nonbool(var);
} else if (!me->overwrite && 

[PATCHv3 1/3] submodule config: inline config_from_{name, path}

[Stefan Beller]

There is no other user of config_from_{name, path}, such that there is no
reason for the existence of these one liner functions. Just inline these
to increase readability.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 submodule-config.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 098085be69..15ffab6af4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -471,18 +471,6 @@ static const struct submodule *config_from(struct 
submodule_cache *cache,
return submodule;
 }
 
-static const struct submodule *config_from_path(struct submodule_cache *cache,
-   const unsigned char *commit_sha1, const char *path)
-{
-   return config_from(cache, commit_sha1, path, lookup_path);
-}
-
-static const struct submodule *config_from_name(struct submodule_cache *cache,
-   const unsigned char *commit_sha1, const char *name)
-{
-   return config_from(cache, commit_sha1, name, lookup_name);
-}
-
 static void ensure_cache_init(void)
 {
if (is_cache_init)
@@ -508,14 +496,14 @@ const struct submodule *submodule_from_name(const 
unsigned char *commit_sha1,
const char *name)
 {
ensure_cache_init();
-   return config_from_name(_submodule_cache, commit_sha1, name);
+   return config_from(_submodule_cache, commit_sha1, name, 
lookup_name);
 }
 
 const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path)
 {
ensure_cache_init();
-   return config_from_path(_submodule_cache, commit_sha1, path);
+   return config_from(_submodule_cache, commit_sha1, path, 
lookup_path);
 }
 
 void submodule_free(void)
-- 
2.11.0.rc2.18.g0126045.dirty

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

[Stefan Beller]

On Mon, Nov 21, 2016 at 4:16 PM, Brandon Williams  wrote:
> On 11/21, Stefan Beller wrote:
>> On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams  wrote:
>> > On 11/21, Stefan Beller wrote:
>> >>
>> >>   switch (lookup_type) {
>> >> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct 
>> >> submodule_cache *cache,
>> >>
>> >>   /* fill the submodule config into the cache */
>> >>   parameter.cache = cache;
>> >> - parameter.commit_sha1 = commit_sha1;
>> >> + // todo: get the actual tree here:
>> >
>> > s/todo/TODO
>> >
>> > Makes it more clear that this is a TODO
>> >
>>
>> The // is more annoying here. I'll review these changes closely
>> before sending out v3.
>
> Well I prefer // to /* */ but that's not the style we use :)

background: Initially I assume we would need to do work here as that part
is exposed to the user in error messages, such that we maybe want to
go the reverse direction and not state a tree but instead the commit containing
it. Since then I decided that it is not worth to optimize for some
hypothetical future
and hence I did not go with the internal todo note I put there. Then I
forgot about
it and it just showed up in the patch here.

Having looked through the patch again, the rest looks clean to me.

Stefan

>
> --
> Brandon Williams

Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C

[Stephan Beyer]

Okay Pranit,

this is the last patch for me to review in this series.

Now that I have a coarse overview of what you did, I have the general
remark that imho the "terms" variable should simply be global instead of
being passed around all the time.

I also had some other remarks but I forgot them... maybe they come to my
mind again when I see patch series v16.

I also want to remark again that I am not a Git developer and only
reviewed this because of my interest in git-bisect. So some of my
suggestions might conflict with other beliefs here. For example, I
consider it very bad style to leak memory... but Git is rather written
as a scripting tool than a genuine library, so perhaps many people here
do not care about it as long as it works...

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c18ca07..b367d8d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int 
> no_checkout,
>   terms->term_good = arg;
>   } else if (!strcmp(arg, "--term-bad") ||
>!strcmp(arg, "--term-new")) {
> - const char *next_arg;

This should already have been removed in patch 15/27, not here.

> @@ -875,6 +875,117 @@ static int bisect_log(void)
>   return status;
>  }
>  
> +static int get_next_word(const char *line, int pos, struct strbuf *word)
> +{
> + int i, len = strlen(line), begin = 0;
> + strbuf_reset(word);
> + for (i = pos; i < len; i++) {
> + if (line[i] == ' ' && begin)
> + return i + 1;
> +
> + if (!begin)
> + begin = 1;
> + strbuf_addch(word, line[i]);
> + }
> +
> + return i;
> +}
> +
> +static int bisect_replay(struct bisect_terms *terms, const char *filename)
> +{
> + struct strbuf line = STRBUF_INIT;
> + struct strbuf word = STRBUF_INIT;
> + FILE *fp = NULL;

(The initialization is not necessary here.)

> + int res = 0;
> +
> + if (is_empty_or_missing_file(filename)) {
> + error(_("no such file with name '%s' exists"), filename);

The error message is misleading if the file exists but is empty.
Maybe something like "cannot read file '%s' for replaying"?

> + res = -1;
> + goto finish;

goto fail;
:D

> + }
> +
> + if (bisect_reset(NULL)) {
> + res = -1;
> + goto finish;

goto fail;

> + }
> +
> + fp = fopen(filename, "r");
> + if (!fp) {
> + res = -1;
> + goto finish;

goto fail;

> + }
> +
> + while (strbuf_getline(, fp) != EOF) {
> + int pos = 0;
> + while (pos < line.len) {
> + pos = get_next_word(line.buf, pos, );
> +
> + if (!strcmp(word.buf, "git")) {
> + continue;
> + } else if (!strcmp(word.buf, "git-bisect")) {
> + continue;
> + } else if (!strcmp(word.buf, "bisect")) {
> + continue;
> + } else if (!strcmp(word.buf, "#")) {
> + break;

Maybe it is more robust to check whether word.buf begins with #

> + }
> +
> + get_terms(terms);
> + if (check_and_set_terms(terms, word.buf)) {
> + res = -1;
> + goto finish;

goto fail;

> + }
> +
> + if (!strcmp(word.buf, "start")) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + sq_dequote_to_argv_array(line.buf+pos, );
> + if (bisect_start(terms, 0, argv.argv, 
> argv.argc)) {
> + argv_array_clear();
> + res = -1;
> + goto finish;

goto fail;

> + }
> + argv_array_clear();
> + break;
> + }
> +
> + if (one_of(word.buf, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + if (bisect_write(word.buf, line.buf+pos, terms, 
> 0)) {
> + res = -1;
> + goto finish;

goto fail;

> + }
> + break;
> + }
> +
> + if (!strcmp(word.buf, "terms")) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + 

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Jonathan Nieder]

Hi,

Jeff King wrote:

> This passes the test suite (after the adjustments in the
> previous patches), but there's a risk of regression for any
> cases where the fallback usually works fine but the code
> isn't exercised by the test suite.  So by itself, this
> commit is a potential step backward, but lets us take two
> steps forward once we've identified and fixed any such
> instances.
>
> Signed-off-by: Jeff King 
> ---
>  environment.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/environment.c b/environment.c
> index cd5aa57..b1743e6 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -164,8 +164,11 @@ static void setup_git_env(void)
>   const char *replace_ref_base;
>  
>   git_dir = getenv(GIT_DIR_ENVIRONMENT);
> - if (!git_dir)
> + if (!git_dir) {
> + if (!startup_info->have_repository)
> + die("BUG: setup_git_env called without repository");
>   git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> + }

This trips reproducibly for

  git ls-remote https://kernel.googlesource.com/pub/scm/git/git

when run outside of a git repository.

Backtrace:

  #0  setup_git_env () at environment.c:172
  #1  get_git_dir () at environment.c:214
  #2  get_helper at transport-helper.c:127
  #3  get_refs_list (for_push=0) at transport-helper.c:1038
  #4  transport_get_remote_refs at transport.c:1076
  #5  cmd_ls_remote at builtin/ls-remote.c:97

transport-helper.c:127 is

argv_array_pushf(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
 get_git_dir());

That code is pretty clearly wrong.  Should it be made conditional
on have_git_dir(), like this?

Thanks,
Jonathan

 transport-helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 91aed35..e4fd982 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport 
*transport)
helper->git_cmd = 0;
helper->silent_exec_failure = 1;
 
-   argv_array_pushf(>env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
-get_git_dir());
+   if (have_git_dir())
+   argv_array_pushf(>env_array, "%s=%s",
+GIT_DIR_ENVIRONMENT, get_git_dir());
 
code = start_command(helper);
if (code < 0 && errno == ENOENT)
-- 

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

[Brandon Williams]

On 11/21, Stefan Beller wrote:
> On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams  wrote:
> > On 11/21, Stefan Beller wrote:
> >>
> >>   switch (lookup_type) {
> >> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct 
> >> submodule_cache *cache,
> >>
> >>   /* fill the submodule config into the cache */
> >>   parameter.cache = cache;
> >> - parameter.commit_sha1 = commit_sha1;
> >> + // todo: get the actual tree here:
> >
> > s/todo/TODO
> >
> > Makes it more clear that this is a TODO
> >
> 
> The // is more annoying here. I'll review these changes closely
> before sending out v3.

Well I prefer // to /* */ but that's not the style we use :)

-- 
Brandon Williams

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

[Stefan Beller]

On Mon, Nov 21, 2016 at 4:11 PM, Brandon Williams  wrote:
> On 11/21, Stefan Beller wrote:
>>
>>   switch (lookup_type) {
>> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct 
>> submodule_cache *cache,
>>
>>   /* fill the submodule config into the cache */
>>   parameter.cache = cache;
>> - parameter.commit_sha1 = commit_sha1;
>> + // todo: get the actual tree here:
>
> s/todo/TODO
>
> Makes it more clear that this is a TODO
>

The // is more annoying here. I'll review these changes closely
before sending out v3.

Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

[Stephan Beyer]

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> Reimplement the `bisect_state` shell function in C and also add a
> subcommand `--bisect-state` to `git-bisect--helper` to call it from
> git-bisect.sh .
> 
> Using `--bisect-state` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
> 
> `bisect_head` is called from `bisect_state` thus its not required to
> introduce another subcommand.

Missing comma before "thus", and "it is" (or "it's") instead of "its" :)

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1767916..1481c6d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
>   return 0;
>  }
>  
> +static char *bisect_head(void)
> +{
> + if (is_empty_or_missing_file(git_path_bisect_head()))
> + return "HEAD";
> + else
> + return "BISECT_HEAD";
> +}

This is very shellish.
In C I'd expect something like

static int bisect_head_sha1(unsigned char *sha)
{
int res;
if (is_empty_or_missing_file(git_path_bisect_head()))
res = get_sha1("HEAD", sha);
else
res = get_sha1("BISECT_HEAD", sha);

if (res)
return error(_("Could not find BISECT_HEAD or HEAD."));

return 0;
}

> +
> +static int bisect_state(struct bisect_terms *terms, const char **argv,
> + int argc)
> +{
> + const char *state = argv[0];
> +
> + get_terms(terms);
> + if (check_and_set_terms(terms, state))
> + return -1;
> +
> + if (!argc)
> + die(_("Please call `--bisect-state` with at least one 
> argument"));

I think this check should move to cmd_bisect__helper. There are also the
other argument number checks.

> +
> + if (argc == 1 && one_of(state, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + const char *bisected_head = xstrdup(bisect_head());
> + const char *hex[1];

Maybe:
const char *hex;

> + unsigned char sha1[20];
> +
> + if (get_sha1(bisected_head, sha1))
> + die(_("Bad rev input: %s"), bisected_head);

And instead of...

> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
> + return -1;
> +
> + *hex = xstrdup(sha1_to_hex(sha1));
> + if (check_expected_revs(hex, 1))
> + return -1;

... simply:

hex = xstrdup(sha1_to_hex(sha1));
if (set_state(terms, state, hex)) {
free(hex);
return -1;
}
free(hex);

where:

static int set_state(struct bisect_terms *terms, const char *state,
 const char *hex)
{
if (bisect_write(state, hex, terms, 0))
return -1;
if (check_expected_revs(, 1))
return -1;
return 0;
}

> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
> + one_of(state, terms->term_good, "skip", NULL)) {
> + int i;
> + struct string_list hex = STRING_LIST_INIT_DUP;
> +
> + for (i = 1; i < argc; i++) {
> + unsigned char sha1[20];
> +
> + if (get_sha1(argv[i], sha1)) {
> + string_list_clear(, 0);
> + die(_("Bad rev input: %s"), argv[i]);
> + }
> + string_list_append(, sha1_to_hex(sha1));
> + }
> + for (i = 0; i < hex.nr; i++) {

... And replace this:

> + const char **hex_string = (const char **) 
> [i].string;
> + if(bisect_write(state, *hex_string, terms, 0)) {
> + string_list_clear(, 0);
> + return -1;
> + }
> + if (check_expected_revs(hex_string, 1)) {
> + string_list_clear(, 0);
> + return -1;
> + }

by:

const char *hex_str = hex.items[i].string;
if (set_state(terms, state, hex_string)) {
string_list_clear(, 0);
return -1;
}

> + }
> + string_list_clear(, 0);
> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if (!strcmp(state, terms->term_bad))
> + die(_("'git bisect %s' can take only one argument."),
> +   terms->term_bad);
> +
> + return -1;
> +}
> +
>  int 

Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

[Brandon Williams]

On 11/21, Stefan Beller wrote:
>  
>   switch (lookup_type) {
> @@ -448,7 +448,8 @@ static const struct submodule *config_from(struct 
> submodule_cache *cache,
>  
>   /* fill the submodule config into the cache */
>   parameter.cache = cache;
> - parameter.commit_sha1 = commit_sha1;
> + // todo: get the actual tree here:

s/todo/TODO

Makes it more clear that this is a TODO


-- 
Brandon Williams

Re: [PATCHv2 3/3] submodule-config: clarify parsing of null_sha1 element

[Brandon Williams]

On 11/21, Stefan Beller wrote:
> Signed-off-by: Stefan Beller 
> Signed-off-by: Junio C Hamano 
> ---
>  Documentation/technical/api-submodule-config.txt | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/technical/api-submodule-config.txt 
> b/Documentation/technical/api-submodule-config.txt
> index 768458580f..a91c1f085e 100644
> --- a/Documentation/technical/api-submodule-config.txt
> +++ b/Documentation/technical/api-submodule-config.txt
> @@ -55,8 +55,11 @@ Functions
>  
>   The same as above but lookup by name.
>  
> -If given the null_sha1 as commit_or_tree the local configuration of a
> -submodule will be returned (e.g. consolidated values from local git
> +Whenever a submodule configuration is parsed in 
> `parse_submodule_config_option`
> +via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1

s/will be overwrite/will overwrite

> +zeroed out.  So in the normal case, when HEAD:.gitmodules is parsed first and
> +then overlayed with the repository configuration, the null_sha1 entry 
> contains
> +the local configuration of a submodule (e.g. consolidated values from local 
> git
>  configuration and the .gitmodules file in the worktree).
>  
>  For an example usage see test-submodule-config.c.
> -- 
> 2.11.0.rc2.18.g0126045.dirty
> 

-- 
Brandon Williams

Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller  wrote:
> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano  wrote:
>>
>> Can the effect of this change demonstrated in a new test?  There
>> must be a scenario where the current behaviour is broken and this
>> change fixes an incorrect computation of relative path, no?

I do not think the current usage exposes this bug in
connect_work_tree_and_git_dir. It is only used in builtin/mv.c,
which fills the second parameter `git_dir` via a call to read_gitfile,
which itself produces an absolute path.

The latest patch of this series however passes in relative path for the
respective git directories.

So the commit message of this patch is misleading at least.
Maybe:

  The current caller of connect_work_tree_and_git_dir passes
  an absolute path for the `git_dir` parameter. In the future patch
  we will also pass in relative path for `git_dir`. Extend the functionality
  of connect_work_tree_and_git_dir to take relative paths for parameters.

  We could work around this in the future patch by computing the absolute
  path for the git_dir in the calling site, however accepting relative
  paths for either parameter makes the API for this function easier
  to use.

  While at it, change `real_work_tree` to be non const as we own
  the memory.


>
> I found the latest patch of this series broken without this patch.
> I'll try to find existing broken code, though.

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Jeff King]

On Mon, Nov 21, 2016 at 11:12:39AM -0800, Junio C Hamano wrote:

> > Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
> > this case, but called out the fact that "cd subdir && git stripspace"
> > would never have worked. So one step back, 2 steps forward, and Dscho's
> > patch is the right step forward.
> 
> Yes, absolutely.
> 
> I sent out a set of proposed amends, and the one for this step 1/3
> runs the command inside a subdirectory to force it not to find the
> .git/config file relative to its pwd, which can reveal the existing
> breakage without help by b9605bc4f2 ;-) hence can be forked for
> older maintenance tracks.

Makes sense, and your amended patch looks good.

-Peff

[PATCHv2 1/3] submodule config: inline config_from_{name, path}

[Stefan Beller]

There is no other user of config_from_{name, path}, such that there is no
reason for the existence of these one liner functions. Just inline these
to increase readability.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 submodule-config.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 098085be69..15ffab6af4 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -471,18 +471,6 @@ static const struct submodule *config_from(struct 
submodule_cache *cache,
return submodule;
 }
 
-static const struct submodule *config_from_path(struct submodule_cache *cache,
-   const unsigned char *commit_sha1, const char *path)
-{
-   return config_from(cache, commit_sha1, path, lookup_path);
-}
-
-static const struct submodule *config_from_name(struct submodule_cache *cache,
-   const unsigned char *commit_sha1, const char *name)
-{
-   return config_from(cache, commit_sha1, name, lookup_name);
-}
-
 static void ensure_cache_init(void)
 {
if (is_cache_init)
@@ -508,14 +496,14 @@ const struct submodule *submodule_from_name(const 
unsigned char *commit_sha1,
const char *name)
 {
ensure_cache_init();
-   return config_from_name(_submodule_cache, commit_sha1, name);
+   return config_from(_submodule_cache, commit_sha1, name, 
lookup_name);
 }
 
 const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path)
 {
ensure_cache_init();
-   return config_from_path(_submodule_cache, commit_sha1, path);
+   return config_from(_submodule_cache, commit_sha1, path, 
lookup_path);
 }
 
 void submodule_free(void)
-- 
2.11.0.rc2.18.g0126045.dirty

[PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

[Stefan Beller]

It is also possible to pass in a tree hash to lookup a submodule config.
Make it clear by naming the variables accrodingly. Looking up a submodule
config by tree hash will come in handy in a later patch.

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-submodule-config.txt |  8 ++--
 submodule-config.c   | 47 
 submodule-config.h   |  4 +-
 t/t7411-submodule-config.sh  | 11 ++
 4 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..768458580f 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -47,15 +47,15 @@ Functions
Can be passed to the config parsing infrastructure to parse
local (worktree) submodule configurations.
 
-`const struct submodule *submodule_from_path(const unsigned char *commit_sha1, 
const char *path)`::
+`const struct submodule *submodule_from_path(const unsigned char 
*commit_or_tree, const char *path)`::
 
-   Lookup values for one submodule by its commit_sha1 and path.
+   Lookup values for one submodule by its commit_or_tree and path.
 
-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, 
const char *name)`::
+`const struct submodule *submodule_from_name(const unsigned char 
*commit_or_tree, const char *name)`::
 
The same as above but lookup by name.
 
-If given the null_sha1 as commit_sha1 the local configuration of a
+If given the null_sha1 as commit_or_tree the local configuration of a
 submodule will be returned (e.g. consolidated values from local git
 configuration and the .gitmodules file in the worktree).
 
diff --git a/submodule-config.c b/submodule-config.c
index 15ffab6af4..4c5f5d074b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -263,12 +263,12 @@ int parse_push_recurse_submodules_arg(const char *opt, 
const char *arg)
return parse_push_recurse(opt, arg, 1);
 }
 
-static void warn_multiple_config(const unsigned char *commit_sha1,
+static void warn_multiple_config(const unsigned char *commit_or_tree,
 const char *name, const char *option)
 {
const char *commit_string = "WORKTREE";
-   if (commit_sha1)
-   commit_string = sha1_to_hex(commit_sha1);
+   if (commit_or_tree)
+   commit_string = sha1_to_hex(commit_or_tree);
warning("%s:.gitmodules, multiple configurations found for "
"'submodule.%s.%s'. Skipping second one!",
commit_string, name, option);
@@ -276,7 +276,7 @@ static void warn_multiple_config(const unsigned char 
*commit_sha1,
 
 struct parse_config_parameter {
struct submodule_cache *cache;
-   const unsigned char *commit_sha1;
+   const unsigned char *commit_or_tree;
const unsigned char *gitmodules_sha1;
int overwrite;
 };
@@ -300,7 +300,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->path)
-   warn_multiple_config(me->commit_sha1, submodule->name,
+   warn_multiple_config(me->commit_or_tree, 
submodule->name,
"path");
else {
if (submodule->path)
@@ -314,7 +314,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
int die_on_error = is_null_sha1(me->gitmodules_sha1);
if (!me->overwrite &&
submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)
-   warn_multiple_config(me->commit_sha1, submodule->name,
+   warn_multiple_config(me->commit_or_tree, 
submodule->name,
"fetchrecursesubmodules");
else
submodule->fetch_recurse = parse_fetch_recurse(
@@ -324,7 +324,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->ignore)
-   warn_multiple_config(me->commit_sha1, submodule->name,
+   warn_multiple_config(me->commit_or_tree, 
submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
 strcmp(value, "dirty") &&
@@ -340,7 +340,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
if (!value) {
ret = config_error_nonbool(var);
} else if (!me->overwrite && 

[PATCHv2 0/3] submodule-config: clarify/cleanup docs and header

[Stefan Beller]

replacing sb/submodule-config-cleanup

v2:
addressed Jacobs concerns in patch2, fixing all occurrences of commit_sha1.

Thanks,
Stefan

interdiff to v1:
diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 1df7a827ff..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -49,7 +49,7 @@ Functions
 
 `const struct submodule *submodule_from_path(const unsigned char 
*commit_or_tree, const char *path)`::
 
-   Lookup values for one submodule by its commit_sha1 and path.
+   Lookup values for one submodule by its commit_or_tree and path.
 
 `const struct submodule *submodule_from_name(const unsigned char 
*commit_or_tree, const char *name)`::


v1:
A small series that would have helped me understand the submodule config
once again.

Thanks,
Stefan

Stefan Beller (3):
  submodule config: inline config_from_{name, path}
  submodule-config: rename commit_sha1 to commit_or_tree
  submodule-config: clarify parsing of null_sha1 element

 Documentation/technical/api-submodule-config.txt | 13 --
 submodule-config.c   | 59 ++--
 submodule-config.h   |  4 +-
 t/t7411-submodule-config.sh  | 11 +
 4 files changed, 45 insertions(+), 42 deletions(-)

-- 
2.11.0.rc2.18.g0126045.dirty

[PATCHv2 3/3] submodule-config: clarify parsing of null_sha1 element

[Stefan Beller]

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/technical/api-submodule-config.txt | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 768458580f..a91c1f085e 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -55,8 +55,11 @@ Functions
 
The same as above but lookup by name.
 
-If given the null_sha1 as commit_or_tree the local configuration of a
-submodule will be returned (e.g. consolidated values from local git
+Whenever a submodule configuration is parsed in `parse_submodule_config_option`
+via e.g. `gitmodules_config()`, it will be overwrite the entry with the sha1
+zeroed out.  So in the normal case, when HEAD:.gitmodules is parsed first and
+then overlayed with the repository configuration, the null_sha1 entry contains
+the local configuration of a submodule (e.g. consolidated values from local git
 configuration and the .gitmodules file in the worktree).
 
 For an example usage see test-submodule-config.c.
-- 
2.11.0.rc2.18.g0126045.dirty

Re: What's cooking in git.git (Nov 2016, #04; Mon, 21)

[Stefan Beller]

On Mon, Nov 21, 2016 at 3:14 PM, Junio C Hamano  wrote:
>>
>> I did review both off and on list and I think the latest version is good.
>
> I thought that there were strange mixups of two enumeration types
> that are incompatible, at least.  Is there an update that I didn't
> see, or you didn't read problems pointed out on list?

Oh right; there was no resolution to that one IIRC.

>
>>> * jt/use-trailer-api-in-commands (2016-11-02) 6 commits
..
>> From a cursory read (with the SQUASH applied)
>> this seems to be done to me.
>
> We are not all that in a hurry to move non-fix to 'next' only with a
> cursory read at this point in the cycle ;-).

But the cycle only applies to patches going to master, but when
asking for doneness I assumed you were asking for opinions on
the transition pu -> next, which I would support.

Thanks,
Stefan

Re: What's cooking in git.git (Nov 2016, #04; Mon, 21)

[Junio C Hamano]

Stefan Beller  writes:

>> * bw/grep-recurse-submodules (2016-11-18) 6 commits
>>  - grep: search history of moved submodules
>>  - grep: enable recurse-submodules to work on  objects
>>  - grep: optionally recurse into submodules
>>  - grep: add submodules as a grep source type
>>  - submodules: load gitmodules file from commit sha1
>>  - submodules: add helper functions to determine presence of submodules
>>
>>  "git grep" learns to optionally recurse into submodules
>>
>>  Waiting for review.
>
> I did review both off and on list and I think the latest version is good.

I thought that there were strange mixups of two enumeration types
that are incompatible, at least.  Is there an update that I didn't
see, or you didn't read problems pointed out on list?

>> * jt/use-trailer-api-in-commands (2016-11-02) 6 commits
>>  - sequencer: use trailer's trailer layout
>>  - trailer: have function to describe trailer layout
>>  - trailer: avoid unnecessary splitting on lines
>>  - commit: make ignore_non_trailer take buf/len
>>  - SQUASH???
>>  - trailer: be stricter in parsing separators
>>
>>  Commands that operate on a log message and add lines to the trailer
>>  blocks, such as "format-patch -s", "cherry-pick (-x|-s)", and
>>  "commit -s", have been taught to use the logic of and share the
>>  code with "git interpret-trailer".
>>
>>  What's the doneness of this topic?
>
> From a cursory read (with the SQUASH applied)
> this seems to be done to me.

We are not all that in a hurry to move non-fix to 'next' only with a
cursory read at this point in the cycle ;-).

>> * sb/submodule-config-cleanup (2016-11-02) 3 commits
>> - submodule-config: clarify parsing of null_sha1 element
>> - submodule-config: rename commit_sha1 to commit_or_tree
>> - submodule config: inline config_from_{name, path}
>>
>>  What's the doneness of this topic?
>
> Jake Keller reviewed this and it turns out I was not careful in patch 2/3.
>
> Will resend.

OK.  Thanks.

Re: What's cooking in git.git (Nov 2016, #04; Mon, 21)

[Junio C Hamano]

Stefan Beller  writes:

>> * sb/push-make-submodule-check-the-default (2016-10-10) 2 commits
>>  - push: change submodule default to check when submodules exist
>>  - submodule add: extend force flag to add existing repos
>>
>>  Turn the default of "push.recurseSubmodules" to "check" when
>>  submodules seem to be in use.
>>
>>  Will hold to wait for hv/submodule-not-yet-pushed-fix
>
> Which is cooking in next, so we'd want to include this into next as well?

Not really.  One step at a time.

Re: What's cooking in git.git (Nov 2016, #04; Mon, 21)

[Stefan Beller]

> * sb/push-make-submodule-check-the-default (2016-10-10) 2 commits
>  - push: change submodule default to check when submodules exist
>  - submodule add: extend force flag to add existing repos
>
>  Turn the default of "push.recurseSubmodules" to "check" when
>  submodules seem to be in use.
>
>  Will hold to wait for hv/submodule-not-yet-pushed-fix

Which is cooking in next, so we'd want to include this into next as well?

When including this series, we get 2 benefits:
* the cooking of hv/submodule-not-yet-pushed-fix is greatly enhanced as
  more submodule users will make use of it (as it would be the default).
* for non submodule users we would see if the approximated estimation
  if the user cares about submodules produces false positives:

if (has_submodules_configured || file_exists(git_path("modules")) ||
   (!is_bare_repository() && file_exists(".gitmodules")))
recurse_submodules = RECURSE_SUBMODULES_CHECK;
else
recurse_submodules = RECURSE_SUBMODULES_OFF;

This heuristic was introduced after we got burned and called out by Linus,
so I would expect this series to not stress non submodule users any more.

>
> * dt/empty-submodule-in-merge (2016-11-17) 1 commit
>  - submodules: allow empty working-tree dirs in merge/cherry-pick
>
>  An empty directory in a working tree that can simply be nuked used
>  to interfere while merging or cherry-picking a change to create a
>  submodule directory there, which has been fixed..
>
>  Waiting for review.

I thought I had reviewed it, will do again and comment.

> * bw/grep-recurse-submodules (2016-11-18) 6 commits
>  - grep: search history of moved submodules
>  - grep: enable recurse-submodules to work on  objects
>  - grep: optionally recurse into submodules
>  - grep: add submodules as a grep source type
>  - submodules: load gitmodules file from commit sha1
>  - submodules: add helper functions to determine presence of submodules
>
>  "git grep" learns to optionally recurse into submodules
>
>  Waiting for review.

I did review both off and on list and I think the latest version is good.


> * hv/submodule-not-yet-pushed-fix (2016-11-16) 4 commits
>   (merged to 'next' on 2016-11-21 at 1a599af962)
>  + submodule_needs_pushing(): explain the behaviour when we cannot answer
>  + batch check whether submodule needs pushing into one call
>  + serialize collection of refs that contain submodule changes
>  + serialize collection of changed submodules
>
>  The code in "git push" to compute if any commit being pushed in the
>  superproject binds a commit in a submodule that hasn't been pushed
>  out was overly inefficient, making it unusable even for a small
>  project that does not have any submodule but have a reasonable
>  number of refs.
>
>  Will cook in 'next'.

Thanks!

>
> * jt/use-trailer-api-in-commands (2016-11-02) 6 commits
>  - sequencer: use trailer's trailer layout
>  - trailer: have function to describe trailer layout
>  - trailer: avoid unnecessary splitting on lines
>  - commit: make ignore_non_trailer take buf/len
>  - SQUASH???
>  - trailer: be stricter in parsing separators
>
>  Commands that operate on a log message and add lines to the trailer
>  blocks, such as "format-patch -s", "cherry-pick (-x|-s)", and
>  "commit -s", have been taught to use the logic of and share the
>  code with "git interpret-trailer".
>
>  What's the doneness of this topic?

>From a cursory read (with the SQUASH applied)
this seems to be done to me.

> * sb/submodule-config-cleanup (2016-11-02) 3 commits
> - submodule-config: clarify parsing of null_sha1 element
> - submodule-config: rename commit_sha1 to commit_or_tree
> - submodule config: inline config_from_{name, path}
>
>  What's the doneness of this topic?

Jake Keller reviewed this and it turns out I was not careful in patch 2/3.

Will resend.

Thanks,
Stefan

What's cooking in git.git (Nov 2016, #04; Mon, 21)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* jc/for-each-ref-head-segfault-fix (2016-11-18) 1 commit
  (merged to 'next' on 2016-11-21 at 3c1f352316)
 + for-each-ref: do not segv with %(HEAD) on an unborn branch

 Using a %(HEAD) placeholder in "for-each-ref --format=" option
 caused the command to segfault when on an unborn branch.

 Will cook in 'next'.


* js/prepare-sequencer (2016-11-21) 1 commit
  (merged to 'next' on 2016-11-21 at 7cdf4ca421)
 + i18n: fix unmatched single quote in error message

 Fix for an error message string.
 Will merge to 'master'.


* js/rebase-i-commentchar-fix (2016-11-21) 3 commits
 - rebase -i: handle core.commentChar=auto
 - stripspace: respect repository config
 - rebase -i: highlight problems with core.commentchar

 "git rebase -i" did not work well with core.commentchar
 configuration variable for two reasons, both of which have been
 fixed.

 Waiting for an ack for updates.
 Hopefully we can merge this before 2.11 final, as one of the
 breakages is a recent regression.


* jc/cache-tree-wip (2016-11-18) 4 commits
 - cache-tree: freshen the tree object at the top level
 - cache-tree: retire cache_tree_fully_valid() API function
 - commit: remove redundant check for active_cache_changed
 - freshen_object(): factor out a helper function


* jk/trailers-placeholder-in-pretty (2016-11-21) 2 commits
 - ref-filter: add support to display trailers as part of contents
 - pretty: add %(trailers) format for displaying trailers of a commit message


* jt/trailer-with-cruft (2016-11-21) 1 commit
 - doc: mention user-configured trailers


* sb/submodule-intern-gitdir (2016-11-21) 3 commits
 - submodule--helper: add intern-git-dir function
 - test-lib-functions.sh: teach test_commit -C 
 - submodule: use absolute path for computing relative path connecting

--
[Graduated to "master"]

* jk/create-branch-remove-unused-param (2016-11-09) 1 commit
  (merged to 'next' on 2016-11-16 at 621254c832)
 + create_branch: drop unused "head" parameter

 Code clean-up.


* nd/worktree-lock (2016-11-13) 1 commit
  (merged to 'next' on 2016-11-16 at 67b731de07)
 + git-worktree.txt: fix typo "to"/"two", and add comma

 Typofix.


* tk/diffcore-delta-remove-unused (2016-11-14) 1 commit
  (merged to 'next' on 2016-11-16 at 51e66c2fa7)
 + diffcore-delta: remove unused parameter to diffcore_count_changes()

 Code cleanup.

--
[Stalled]

* sb/push-make-submodule-check-the-default (2016-10-10) 2 commits
 - push: change submodule default to check when submodules exist
 - submodule add: extend force flag to add existing repos

 Turn the default of "push.recurseSubmodules" to "check" when
 submodules seem to be in use.

 Will hold to wait for hv/submodule-not-yet-pushed-fix


* jc/bundle (2016-03-03) 6 commits
 - index-pack: --clone-bundle option
 - Merge branch 'jc/index-pack' into jc/bundle
 - bundle v3: the beginning
 - bundle: keep a copy of bundle file name in the in-core bundle header
 - bundle: plug resource leak
 - bundle doc: 'verify' is not about verifying the bundle

 The beginning of "split bundle", which could be one of the
 ingredients to allow "git clone" traffic off of the core server
 network to CDN.

 While I think it would make it easier for people to experiment and
 build on if the topic is merged to 'next', I am at the same time a
 bit reluctant to merge an unproven new topic that introduces a new
 file format, which we may end up having to support til the end of
 time.  It is likely that to support a "prime clone from CDN", it
 would need a lot more than just "these are the heads and the pack
 data is over there", so this may not be sufficient.

 Will discard.


* mh/connect (2016-06-06) 10 commits
 - connect: [host:port] is legacy for ssh
 - connect: move ssh command line preparation to a separate function
 - connect: actively reject git:// urls with a user part
 - connect: change the --diag-url output to separate user and host
 - connect: make parse_connect_url() return the user part of the url as a 
separate value
 - connect: group CONNECT_DIAG_URL handling code
 - connect: make parse_connect_url() return separated host and port
 - connect: re-derive a host:port string from the separate host and port 
variables
 - connect: call get_host_and_port() earlier
 - connect: document why we sometimes call get_port after get_host_and_port

 Rewrite Git-URL parsing routine (hopefully) without changing any
 behaviour.

 It has been two months without any 

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Brandon Williams]

On 11/21, Stefan Beller wrote:
> diff --git a/t/t7412-submodule-interngitdirs.sh 
> b/t/t7412-submodule-interngitdirs.sh
> new file mode 100755
> index 00..8938a4c8b7
> --- /dev/null
> +++ b/t/t7412-submodule-interngitdirs.sh
> @@ -0,0 +1,41 @@
> +#!/bin/sh
> +
> +test_description='Test submodule interngitdirs
> +
> +This test verifies that `git submodue interngitdirs` moves a submodules git
> +directory into the superproject.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a real submodule' '
> + git init sub1 &&
> + test_commit -C sub1 first &&
> + git submodule add ./sub1 &&
> + test_tick &&
> + git commit -m superproject
> +'
> +
> +test_expect_success 'intern the git dir' '
> + git submodule interngitdirs &&
> + test -f sub1/.git &&
> + test -d .git/modules/sub1 &&
> + # check that we did not break the repository:
> + git status
> +'
> +
> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
> + git init sub2 &&
> + test_commit -C sub2 first &&
> + git add sub2 &&
> + git commit -m superproject
> +'
> +
> +test_expect_success 'intern the git dir fails for incomplete submodules' '
> + test_must_fail git submodule interngitdirs &&
> + # check that we did not break the repository:
> + git status
> +'
> +
> +test_done
> +

Could we add a test which has nested submodules that need to be
migrated?  Hopfully its just as easy as adding the test :)

-- 
Brandon Williams

Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C

[Stephan Beyer]

Hi Pranit,

in this mail I review the "second part" of your patch: the transition of
bisect_next and bisect_auto_next to C.

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1d3e17f..fcd7574 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -408,6 +411,136 @@ static int bisect_terms(struct bisect_terms *terms, 
> const char **argv, int argc)
>   return 0;
>  }
>  
> +static int register_good_ref(const char *refname,
> +  const struct object_id *oid, int flags,
> +  void *cb_data)
> +{
> + struct string_list *good_refs = cb_data;
> + string_list_append(good_refs, oid_to_hex(oid));
> + return 0;
> +}
> +
> +static int bisect_next(struct bisect_terms *terms, const char *prefix)
> +{
> + int res, no_checkout;
> +
> + /*
> +  * In case of mistaken revs or checkout error, or signals received,
> +  * "bisect_auto_next" below may exit or misbehave.
> +  * We have to trap this to be able to clean up using
> +  * "bisect_clean_state".
> +  */

The comment above makes no sense here, or does it?

> + if (bisect_next_check(terms, terms->term_good))
> + return -1;
> +
> + no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
> +
> + /* Perform all bisection computation, display and checkout */
> + res = bisect_next_all(prefix , no_checkout);

Style: there is a space left of the comma.

> +
> + if (res == 10) {
> + FILE *fp = NULL;
> + unsigned char sha1[20];
> + struct commit *commit;
> + struct pretty_print_context pp = {0};
> + struct strbuf commit_name = STRBUF_INIT;
> + char *bad_ref = xstrfmt("refs/bisect/%s",
> +   terms->term_bad);
> + int retval = 0;
> +
> + read_ref(bad_ref, sha1);
> + commit = lookup_commit_reference(sha1);
> + format_commit_message(commit, "%s", _name, );
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp) {
> + retval = -1;
> + goto finish_10;
> + }
> + if (fprintf(fp, "# first %s commit: [%s] %s\n",
> + terms->term_bad, sha1_to_hex(sha1),
> + commit_name.buf) < 1){
> + retval = -1;
> + goto finish_10;
> + }
> + goto finish_10;
> + finish_10:
> + if (fp)
> + fclose(fp);
> + strbuf_release(_name);
> + free(bad_ref);
> + return retval;
> + }
> + else if (res == 2) {
> + FILE *fp = NULL;
> + struct rev_info revs;
> + struct argv_array rev_argv = ARGV_ARRAY_INIT;
> + struct string_list good_revs = STRING_LIST_INIT_DUP;
> + struct pretty_print_context pp = {0};
> + struct commit *commit;
> + char *term_good = xstrfmt("%s-*", terms->term_good);
> + int i, retval = 0;
> +
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp) {
> + retval = -1;
> + goto finish_2;
> + }
> + if (fprintf(fp, "# only skipped commits left to test\n") < 1) {
> + retval = -1;
> + goto finish_2;
> + }
> + for_each_glob_ref_in(register_good_ref, term_good,
> +  "refs/bisect/", (void *) _revs);
> +
> + argv_array_pushl(_argv, "skipped_commits", 
> "refs/bisect/bad", "--not", NULL);
> + for (i = 0; i < good_revs.nr; i++)
> + argv_array_push(_argv, good_revs.items[i].string);
> +
> + /* It is important to reset the flags used by revision walks
> +  * as the previous call to bisect_next_all() in turn
> +  * setups a revision walk.
> +  */
> + reset_revision_walk();
> + init_revisions(, NULL);
> + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, 
> , NULL);
> + argv_array_clear(_argv);
> + string_list_clear(_revs, 0);
> + if (prepare_revision_walk())
> + die(_("revision walk setup failed\n"));
> +
> + while ((commit = get_revision()) != NULL) {
> + struct strbuf commit_name = STRBUF_INIT;
> + format_commit_message(commit, "%s",
> +   _name, );
> + fprintf(fp, "# possible first %s commit: "
> + "[%s] %s\n", terms->term_bad,
> + oid_to_hex(>object.oid),
> + commit_name.buf);
> +  

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Junio C Hamano]

Stefan Beller  writes:

> +interngitdirs::
> + Move the git directory of submodules into its superprojects
> + `$GIT_DIR/modules` path and then connect the git directory and
> + its working directory by setting the `core.worktree` and adding
> + a .git file pointing to the git directory interned into the
> + superproject.
> ++
> + A repository that was cloned independently and later added
> + as a submodule or old setups have the submodules git directory
> + inside the submodule instead of the
> ++
> + This command is recursive by default.

Does this format correctly?

I somehow thought that second and subsequent paragraphs continued
with "+" want no indentation before them.  See for example the
Values section in config.txt and see how entries for boolean:: and
color:: use multiple '+' paragraphs.

If we do not have to refrain from indenting the second and
subsequent paragraphs, that would be great for readability, but I
take the existing practice as telling me that we cannot do that X-<.

> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
> + git init sub2 &&
> + test_commit -C sub2 first &&
> + git add sub2 &&
> + git commit -m superproject
> +'
> +
> +test_expect_success 'intern the git dir fails for incomplete submodules' '
> + test_must_fail git submodule interngitdirs &&
> + # check that we did not break the repository:
> + git status
> +'

It is not clear what the last "git status" wants to test.  In the
extreme, if the failed "git submodule" command did

rm -fr .git ?* && git init

wouldn't "git status" still succeed?  

What are the minimum things that we expect from "did not break" to
see?  sub2/.git is still a directory and is a valid repository?  The
contents of the .git/modules/* before and after the "git submodule"
does not change?  Some other things?

Re: [PATCH] doc: mention user-configured trailers

[Junio C Hamano]

Jonathan Tan  writes:

> In commit 1462450 ("trailer: allow non-trailers in trailer block",
> 2016-10-21), functionality was added (and tested [1]) to allow
> non-trailer lines in trailer blocks, as long as those blocks contain at
> least one Git-generated or user-configured trailer, and consists of at
> least 25% trailers. The documentation was updated to mention this new
> functionality, but did not mention "user-configured trailer".
>
> Further update the documentation to also mention "user-configured
> trailer".
>
> [1] "with non-trailer lines mixed with a configured trailer" in
> t/t7513-interpret-trailers.sh
>
> Signed-off-by: Jonathan Tan 
> ---
>
> Yes, mentioning a trailer in a Git config will cause interpret-trailers
> to treat it similarly to a Git-generated trailer (in that its presence
> causes a block partially consisting of trailers to be considered a
> trailer block). See the commit message above for a test case that
> verifies that.
>
> I took a look at the documentation, and it wasn't completely documented,
> so here is a patch to correct that.

Looks sensible. Thanks.

>  Documentation/git-interpret-trailers.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-interpret-trailers.txt 
> b/Documentation/git-interpret-trailers.txt
> index e99bda6..09074c7 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -49,7 +49,8 @@ will be added before the new trailer.
>  
>  Existing trailers are extracted from the input message by looking for
>  a group of one or more lines that (i) are all trailers, or (ii) contains at
> -least one Git-generated trailer and consists of at least 25% trailers.
> +least one Git-generated or user-configured trailer and consists of at
> +least 25% trailers.
>  The group must be preceded by one or more empty (or whitespace-only) lines.
>  The group must either be at the end of the message or be the last
>  non-whitespace lines before a line that starts with '---'. Such three

Re: [PATCH 2/3] test-lib-functions.sh: teach test_commit -C

[Junio C Hamano]

Stefan Beller  writes:

> Specifically when setting up submodule tests, it comes in handy if
> we can create commits in repositories that are not at the root of
> the tested trash dir. Add "-C " similar to gits -C parameter
> that will perform the operation in the given directory.
>
> Signed-off-by: Stefan Beller 
> ---

Looks useful.

>  t/test-lib-functions.sh | 20 +++-
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index fdaeb3a96b..579e812506 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -157,16 +157,21 @@ debug () {
>GIT_TEST_GDB=1 "$@"
>  }
>  
> -# Call test_commit with the arguments " [ [ 
> []]]"
> +# Call test_commit with the arguments
> +# [-C ]  [ [ []]]"
>  #
>  # This will commit a file with the given contents and the given commit
>  # message, and tag the resulting commit with the given tag name.
>  #
>  # , , and  all default to .
> +#
> +# If the first argument is "-C", the second argument is used as a path for
> +# the git invocations.
>  
>  test_commit () {
>   notick= &&
>   signoff= &&
> + indir= &&
>   while test $# != 0
>   do
>   case "$1" in
> @@ -176,21 +181,26 @@ test_commit () {
>   --signoff)
>   signoff="$1"
>   ;;
> + -C)
> + indir="$2"
> + shift
> + ;;
>   *)
>   break
>   ;;
>   esac
>   shift
>   done &&
> + indir=${indir:+"$indir"/} &&
>   file=${2:-"$1.t"} &&
> - echo "${3-$1}" > "$file" &&
> - git add "$file" &&
> + echo "${3-$1}" > "$indir$file" &&
> + git ${indir:+ -C "$indir"} add "$file" &&
>   if test -z "$notick"
>   then
>   test_tick
>   fi &&
> - git commit $signoff -m "$1" &&
> - git tag "${4:-$1}"
> + git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
> + git ${indir:+ -C "$indir"} tag "${4:-$1}"
>  }
>  
>  # Call test_merge with the arguments " ", where 

Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Subject: Re: [PATCH 1/3] submodule: use absolute path for computing relative 
>> path connecting
>
> connecting what?  IOW, has the subject line somehow truncated?
>
>> This addresses a similar concern as in f8eaa0ba98b (submodule--helper,
>> module_clone: always operate on absolute paths, 2016-03-31)
>>
>> When computing the relative path from one to another location, we
>> need to provide both locations as absolute paths to make sure the
>> computation of the relative path is correct.
>
> Can the effect of this change demonstrated in a new test?  There
> must be a scenario where the current behaviour is broken and this
> change fixes an incorrect computation of relative path, no?
>

I found the latest patch of this series broken without this patch.
I'll try to find existing broken code, though.

Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Junio C Hamano]

Stefan Beller  writes:

> Subject: Re: [PATCH 1/3] submodule: use absolute path for computing relative 
> path connecting

connecting what?  IOW, has the subject line somehow truncated?

> This addresses a similar concern as in f8eaa0ba98b (submodule--helper,
> module_clone: always operate on absolute paths, 2016-03-31)
>
> When computing the relative path from one to another location, we
> need to provide both locations as absolute paths to make sure the
> computation of the relative path is correct.

Can the effect of this change demonstrated in a new test?  There
must be a scenario where the current behaviour is broken and this
change fixes an incorrect computation of relative path, no?

>
> While at it, change `real_work_tree` to be non const as we own
> the memory.
>
> Signed-off-by: Stefan Beller 
> ---

>  submodule.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6f7d883de9..66c5ce5a24 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char 
> *work_tree, const char *git_dir)
>  {
>   struct strbuf file_name = STRBUF_INIT;
>   struct strbuf rel_path = STRBUF_INIT;
> - const char *real_work_tree = xstrdup(real_path(work_tree));
> + char *real_git_dir = xstrdup(real_path(git_dir));
> + char *real_work_tree = xstrdup(real_path(work_tree));
>  
>   /* Update gitfile */
>   strbuf_addf(_name, "%s/.git", work_tree);
>   write_file(file_name.buf, "gitdir: %s",
> -relative_path(git_dir, real_work_tree, _path));
> +relative_path(real_git_dir, real_work_tree, _path));
>  
>   /* Update core.worktree setting */
>   strbuf_reset(_name);
> - strbuf_addf(_name, "%s/config", git_dir);
> + strbuf_addf(_name, "%s/config", real_git_dir);
>   git_config_set_in_file(file_name.buf, "core.worktree",
> -relative_path(real_work_tree, git_dir,
> +relative_path(real_work_tree, real_git_dir,
>_path));
>  
>   strbuf_release(_name);
>   strbuf_release(_path);
> - free((void *)real_work_tree);
> + free(real_work_tree);
> + free(real_git_dir);
>  }
>  
>  int parallel_submodules(void)

Re: [PATCH 0/3] Introduce `submodule interngitdirs`

[Stefan Beller]

On Mon, Nov 21, 2016 at 12:48 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> The discussion of the submodule checkout series revealed to me
>> that a command is needed to move the git directory from the
>> submodules working tree to be embedded into the superprojects git
>> directory.
>
> You used "move" here, and "migrate" in the function name in 3/3,
> both of which make sense.
>
> But "intern" sounds funny.  Who is confined as a prisoner here?
> North American English uses that verb as "serve as an intern", but
> that does not apply here.  The verb also is used in Lisp-ish
> languages to mean the act of turning a string into a symbol, but
> that does not apply here, either.

I was inspired by the latter as we ask Git to make the submodule
"properly embedded" into the superproject, which is what I imagined
is similar to the lisp interning.

So I guess my imagination went to far and we rather want to invoke it via
"git submodule migrategitdirs" ?

But there you would ask "where are we migrating the git dirs to?", so
it would be reasonable to expect 2 parameters (just like the mv
command).

So maybe "git submodule embedgitdirs" ?

Re: [PATCH 0/3] Introduce `submodule interngitdirs`

[Junio C Hamano]

Stefan Beller  writes:

> The discussion of the submodule checkout series revealed to me
> that a command is needed to move the git directory from the
> submodules working tree to be embedded into the superprojects git
> directory.

You used "move" here, and "migrate" in the function name in 3/3,
both of which make sense.  

But "intern" sounds funny.  Who is confined as a prisoner here?
North American English uses that verb as "serve as an intern", but
that does not apply here.  The verb also is used in Lisp-ish
languages to mean the act of turning a string into a symbol, but
that does not apply here, either.

[PATCH] doc: mention user-configured trailers

[Jonathan Tan]

In commit 1462450 ("trailer: allow non-trailers in trailer block",
2016-10-21), functionality was added (and tested [1]) to allow
non-trailer lines in trailer blocks, as long as those blocks contain at
least one Git-generated or user-configured trailer, and consists of at
least 25% trailers. The documentation was updated to mention this new
functionality, but did not mention "user-configured trailer".

Further update the documentation to also mention "user-configured
trailer".

[1] "with non-trailer lines mixed with a configured trailer" in
t/t7513-interpret-trailers.sh

Signed-off-by: Jonathan Tan 
---

Yes, mentioning a trailer in a Git config will cause interpret-trailers
to treat it similarly to a Git-generated trailer (in that its presence
causes a block partially consisting of trailers to be considered a
trailer block). See the commit message above for a test case that
verifies that.

I took a look at the documentation, and it wasn't completely documented,
so here is a patch to correct that.

 Documentation/git-interpret-trailers.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index e99bda6..09074c7 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -49,7 +49,8 @@ will be added before the new trailer.
 
 Existing trailers are extracted from the input message by looking for
 a group of one or more lines that (i) are all trailers, or (ii) contains at
-least one Git-generated trailer and consists of at least 25% trailers.
+least one Git-generated or user-configured trailer and consists of at
+least 25% trailers.
 The group must be preceded by one or more empty (or whitespace-only) lines.
 The group must either be at the end of the message or be the last
 non-whitespace lines before a line that starts with '---'. Such three
-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

This addresses a similar concern as in f8eaa0ba98b (submodule--helper,
module_clone: always operate on absolute paths, 2016-03-31)

When computing the relative path from one to another location, we
need to provide both locations as absolute paths to make sure the
computation of the relative path is correct.

While at it, change `real_work_tree` to be non const as we own
the memory.

Signed-off-by: Stefan Beller 
---
 submodule.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6f7d883de9..66c5ce5a24 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char 
*work_tree, const char *git_dir)
 {
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
-   const char *real_work_tree = xstrdup(real_path(work_tree));
+   char *real_git_dir = xstrdup(real_path(git_dir));
+   char *real_work_tree = xstrdup(real_path(work_tree));
 
/* Update gitfile */
strbuf_addf(_name, "%s/.git", work_tree);
write_file(file_name.buf, "gitdir: %s",
-  relative_path(git_dir, real_work_tree, _path));
+  relative_path(real_git_dir, real_work_tree, _path));
 
/* Update core.worktree setting */
strbuf_reset(_name);
-   strbuf_addf(_name, "%s/config", git_dir);
+   strbuf_addf(_name, "%s/config", real_git_dir);
git_config_set_in_file(file_name.buf, "core.worktree",
-  relative_path(real_work_tree, git_dir,
+  relative_path(real_work_tree, real_git_dir,
 _path));
 
strbuf_release(_name);
strbuf_release(_path);
-   free((void *)real_work_tree);
+   free(real_work_tree);
+   free(real_git_dir);
 }
 
 int parallel_submodules(void)
-- 
2.11.0.rc2.18.g0126045.dirty

[PATCH 0/3] Introduce `submodule interngitdirs`

[Stefan Beller]

The discussion of the submodule checkout series revealed to me that a command
is needed to move the git directory from the submodules working tree to be
embedded into the superprojects git directory. 

So I wrote the code to intern the submodules git dir into the superproject,
but whilst writing the code I realized this could be valueable for our use
in testing too. So I exposed it via the submodule--helper. But as the
submodule helper ought to be just an internal API, we could also
offer it via the proper submodule command.

The command as it is has little value to the end user for now, but
breaking it out of the submodule checkout series hopefully makes review easier.

Thanks,
Stefan

Stefan Beller (3):
  submodule: use absolute path for computing relative path connecting
  test-lib-functions.sh: teach test_commit -C 
  submodule--helper: add intern-git-dir function

 Documentation/git-submodule.txt| 15 ++-
 builtin/submodule--helper.c| 33 ++-
 git-submodule.sh   |  7 -
 submodule.c| 55 ++
 submodule.h|  1 +
 t/t7412-submodule-interngitdirs.sh | 41 
 t/test-lib-functions.sh| 20 ++
 7 files changed, 159 insertions(+), 13 deletions(-)
 create mode 100755 t/t7412-submodule-interngitdirs.sh

-- 
2.11.0.rc2.18.g0126045.dirty

[PATCH 3/3] submodule--helper: add intern-git-dir function

[Stefan Beller]

When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.

Add functionality to migrate the git directory to be embedded
into the superprojects git directory.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt| 15 -
 builtin/submodule--helper.c| 33 -
 git-submodule.sh   |  7 ++-
 submodule.c| 43 ++
 submodule.h|  1 +
 t/t7412-submodule-interngitdirs.sh | 41 
 6 files changed, 137 insertions(+), 3 deletions(-)
 create mode 100755 t/t7412-submodule-interngitdirs.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..80d55350eb 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,7 +22,7 @@ SYNOPSIS
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
 'git submodule' [--quiet] sync [--recursive] [--] [...]
-
+'git submodule' [--quiet] interngitdirs [--] [...]
 
 DESCRIPTION
 ---
@@ -245,6 +245,19 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
+interngitdirs::
+   Move the git directory of submodules into its superprojects
+   `$GIT_DIR/modules` path and then connect the git directory and
+   its working directory by setting the `core.worktree` and adding
+   a .git file pointing to the git directory interned into the
+   superproject.
++
+   A repository that was cloned independently and later added
+   as a submodule or old setups have the submodules git directory
+   inside the submodule instead of the
++
+   This command is recursive by default.
+
 OPTIONS
 ---
 -q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..256f8e9439 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,36 @@ static int resolve_remote_submodule_branch(int argc, 
const char **argv,
return 0;
 }
 
+static int intern_git_dir(int argc, const char **argv, const char *prefix)
+{
+   int i;
+   struct pathspec pathspec;
+   struct module_list list = MODULE_LIST_INIT;
+
+   struct option intern_gitdir_options[] = {
+   OPT_END()
+   };
+
+   const char *const git_submodule_helper_usage[] = {
+   N_("git submodule--helper intern-git-dir [...]"),
+   NULL
+   };
+
+   argc = parse_options(argc, argv, prefix, intern_gitdir_options,
+git_submodule_helper_usage, 0);
+
+   gitmodules_config();
+   git_config(submodule_config, NULL);
+
+   if (module_list_compute(argc, argv, prefix, , ) < 0)
+   return 1;
+
+   for (i = 0; i < list.nr; i++)
+   migrate_submodule_gitdir(list.entries[i]->name);
+
+   return 0;
+}
+
 struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
@@ -1090,7 +1120,8 @@ static struct cmd_struct commands[] = {
{"resolve-relative-url", resolve_relative_url},
{"resolve-relative-url-test", resolve_relative_url_test},
{"init", module_init},
-   {"remote-branch", resolve_remote_submodule_branch}
+   {"remote-branch", resolve_remote_submodule_branch},
+   {"intern-git-dir", intern_git_dir}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..747e934df2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1131,6 +1131,11 @@ cmd_sync()
done
 }
 
+cmd_interngitdirs()
+{
+   git submodule--helper intern-git-dir "$@"
+}
+
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific
 # options are primarily done by the subcommand implementations.
@@ -1140,7 +1145,7 @@ cmd_sync()
 while test $# != 0 && test -z "$command"
 do
case "$1" in
-   add | foreach | init | deinit | update | status | summary | sync)
+   add | foreach | init | deinit | update | status | summary | sync | 
interngitdirs)
command=$1
;;
-q|--quiet)
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..99befdba85 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1263,3 +1263,46 @@ void prepare_submodule_repo_env(struct argv_array *out)
}
argv_array_push(out, "GIT_DIR=.git");
 }
+
+/*
+ * Migrate the given submodule (and all its submodules recursively) from
+ * having its git directory within the working tree to the git dir nested
+ * in its superprojects git dir under modules/.
+ */
+void migrate_submodule_gitdir(const char *path)
+{
+   char *old_git_dir;
+   

[PATCH 2/3] test-lib-functions.sh: teach test_commit -C

[Stefan Beller]

Specifically when setting up submodule tests, it comes in handy if
we can create commits in repositories that are not at the root of
the tested trash dir. Add "-C " similar to gits -C parameter
that will perform the operation in the given directory.

Signed-off-by: Stefan Beller 
---
 t/test-lib-functions.sh | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..579e812506 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -157,16 +157,21 @@ debug () {
 GIT_TEST_GDB=1 "$@"
 }
 
-# Call test_commit with the arguments " [ [ []]]"
+# Call test_commit with the arguments
+# [-C ]  [ [ []]]"
 #
 # This will commit a file with the given contents and the given commit
 # message, and tag the resulting commit with the given tag name.
 #
 # , , and  all default to .
+#
+# If the first argument is "-C", the second argument is used as a path for
+# the git invocations.
 
 test_commit () {
notick= &&
signoff= &&
+   indir= &&
while test $# != 0
do
case "$1" in
@@ -176,21 +181,26 @@ test_commit () {
--signoff)
signoff="$1"
;;
+   -C)
+   indir="$2"
+   shift
+   ;;
*)
break
;;
esac
shift
done &&
+   indir=${indir:+"$indir"/} &&
file=${2:-"$1.t"} &&
-   echo "${3-$1}" > "$file" &&
-   git add "$file" &&
+   echo "${3-$1}" > "$indir$file" &&
+   git ${indir:+ -C "$indir"} add "$file" &&
if test -z "$notick"
then
test_tick
fi &&
-   git commit $signoff -m "$1" &&
-   git tag "${4:-$1}"
+   git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
+   git ${indir:+ -C "$indir"} tag "${4:-$1}"
 }
 
 # Call test_merge with the arguments " ", where 
-- 
2.11.0.rc2.18.g0126045.dirty

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Junio C Hamano]

Junio C Hamano  writes:

> @@ -93,8 +93,17 @@ eval '
>  GIT_CHERRY_PICK_HELP="$resolvemsg"
>  export GIT_CHERRY_PICK_HELP
>  
> -comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> -: ${comment_char:=#}
> +comment_char=$(git config --get core.commentchar 2>/dev/null)
> +case "$comment_char" in
> +'' | auto)
> + comment_char="#"
> + ;;
> +?)
> + ;;
> +*)
> + comment_char=$(echo "$comment_char" | cut -c1)
> + ;;
> +esac

Amended in is a fix for a typo the other Johannes noticed.

Thanks.

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d941f0a69f..5d0a7dca9d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -983,7 +983,7 @@ test_expect_success 'rebase -i respects core.commentchar' 
> '
>   test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> -test_expect_failure 'rebase -i respects core.commentchar=auto' '
> +test_expect_success 'rebase -i respects core.commentchar=auto' '
>   test_config core.commentchar auto &&
>   write_script copy-edit-script.sh <<-\EOF &&
>   cp "$1" edit-script

Re: [PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

Junio C Hamano  writes:

> From: Johannes Schindelin 
>
> The way "git stripspace" reads the configuration was not quite
> correct, in that it forgot to probe for a possibly existing
> repository (note: stripspace is designed to be usable outside the
> repository as well) before doing so.  Due to this, .git/config was
> read only when the command was run from the top-level of the working
> tree.  
>
> A recent change b9605bc4f2 ("config: only read .git/config from
> configured repos", 2016-09-12) stopped reading the repository-local
> configuration file ".git/config" unless the repository discovery
> process is done, and ".git/config" is no longer read even when run
> from the top-level, which exposed the bug even more.

The above two paragraphs are rewritten from the original to explain
how this seemed to work (by accident) and its breakage surfaced in
"rebase -i" after b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) better.  The use of stripspace in
"rebase-i" was done after cd_to_toplevel and it happened to work
before that commit.

Otherwise there is no change from the original.

> When rebasing interactively with a commentChar defined in the
> current repository's config, the help text at the bottom of the edit
> script potentially used an incorrect comment character. This was not
> only funny-looking, but also resulted in tons of warnings like this
> one:
>
>   Warning: the command isn't recognized in the following line
>- #
>
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Junio C Hamano 
> ---
>  builtin/stripspace.c  | 4 +++-
>  t/t0030-stripspace.sh | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 15e716ef43..1e62a008cb 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char 
> *prefix)
>   if (argc)
>   usage_with_options(stripspace_usage, options);
>  
> - if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
> + if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
> + setup_git_directory_gently(NULL);
>   git_config(git_default_config, NULL);
> + }
>  
>   if (strbuf_read(, 0, 1024) < 0)
>   die_errno("could not read the input");
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index c1f6411eb2..bbf3e39e3d 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
>   test_cmp expect actual
>  '
>  
> -test_expect_failure '-c with comment char defined in .git/config' '
> +test_expect_success '-c with comment char defined in .git/config' '
>   test_config core.commentchar = &&
>   printf "= foo\n" >expect &&
>   printf "foo" | (

Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar

[Junio C Hamano]

Junio C Hamano  writes:

> From: Johannes Schindelin 
>
> The interactive rebase does not currently play well with
> core.commentchar. Let's add some tests to highlight those problems
> that will be fixed in the remainder of the series.
>
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Junio C Hamano 
> ---

Sorry, I should have commented here after --- line what changes were
proposed by this set of amends.

>  t/t0030-stripspace.sh |  9 +
>  t/t3404-rebase-interactive.sh | 11 +++
>  2 files changed, 20 insertions(+)
>
> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 29e91d861c..c1f6411eb2 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
>   test_cmp expect actual
>  '
>  
> +test_expect_failure '-c with comment char defined in .git/config' '
> + test_config core.commentchar = &&
> + printf "= foo\n" >expect &&
> + printf "foo" | (
> + mkdir sub && cd sub && git stripspace -c
> + ) >actual &&
> + test_cmp expect actual
> +'

I noticed that the lack of "\n" at the end was merely copied from the
previous existing test (i.e. "-c with with changed comment char"),
which was already wrong the same way, so I kept that part as-is.

Running "stripspace" in a subdirectory is to avoid the ".git/config
was read without repository discovery as long as the command runs at
the top-level of the working tree" accident so that this highlights
the breakage with or without Peff's b9605bc4f2 ("config: only read
.git/config from configured repos", 2016-09-12).


> +
>  test_expect_success 'avoid SP-HT sequence in commented line' '
>   printf "#\tone\n#\n# two\n" >expect &&
>   printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d6d65a3a94..d941f0a69f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -983,6 +983,17 @@ test_expect_success 'rebase -i respects 
> core.commentchar' '
>   test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_failure 'rebase -i respects core.commentchar=auto' '
> + test_config core.commentchar auto &&
> + write_script copy-edit-script.sh <<-\EOF &&
> + cp "$1" edit-script
> + EOF
> + test_set_editor "$(pwd)/copy-edit-script.sh" &&
> + test_when_finished "git rebase --abort || :" &&
> + git rebase -i HEAD^ &&
> + test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
> +'
> +

Removed from here is a leftover debugging bit (grep "^#"
edit-script).

Dear Friend,

[Miss Aisha Aisha]

Dear Friend,

I am soliciting your partnership to relocation $9 Million to your country for 
investment on my behalf and you will be entitled to 30% of the sum once the 
transaction is successful made, please indicate your interest if you are 
capable so that i will send you details of the transaction.

Thanks with my best regards. Aisha

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Sixt]

Am 21.11.2016 um 20:07 schrieb Junio C Hamano:

Setting comment-char to multi-letter sequence is not supported at
all, and this code is protecting itself from that nonsense---it does
not need to be fast, and Dscho already gives a fast path for a
single letter case in his patch.


Fair enough, no problem.

-- Hannes

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Junio C Hamano]

Jeff King  writes:

> On Mon, Nov 21, 2016 at 10:15:43AM -0800, Junio C Hamano wrote:
>
>> > +  test_cmp expect actual
>> > +'
>> > +
>> 
>> Is this a recent regression?  When applied on top of 'maint' or
>> older, it seems to pass just fine.
>> 
>> ... Goes and looks ...
>> 
>> Interesting.  Peff's b9605bc4f2 ("config: only read .git/config from
>> configured repos", 2016-09-12) is where this starts failing, which
>> is understandable given the code change to builtin/stripspace.c in
>> [2/3]. 
>> 
>> The analysis of the log message in [2/3] is wrong and needs
>> updating, though.  In the old world order it worked by accident to
>> call git_config() without calling setup_git_directory(); after
>> b9605bc4f2, that no longer is valid and is exposed as a bug.  
>
> Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
> this case, but called out the fact that "cd subdir && git stripspace"
> would never have worked. So one step back, 2 steps forward, and Dscho's
> patch is the right step forward.

Yes, absolutely.

I sent out a set of proposed amends, and the one for this step 1/3
runs the command inside a subdirectory to force it not to find the
.git/config file relative to its pwd, which can reveal the existing
breakage without help by b9605bc4f2 ;-) hence can be forked for
older maintenance tracks.

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Junio C Hamano]

Johannes Sixt  writes:

> Am 21.11.2016 um 19:40 schrieb Junio C Hamano:
>> Johannes Sixt  writes:
>>> It could be written without forking a process:
>>>
>>> comment_char=${comment_char%${comment_char#?}}
>>>
>>> (aka "remove from the end what remains after removing the first character")
>>
>> Hopefully nobody would include any glob metacharacters in there,
>> e.g. "core.commentchar='=*'", which would break that?
>
> Heh. I tested a few variations, but not this one. Make it
>
>   comment_char=${comment_char%"${comment_char#?}"}

I tried a few implementations of shells, and decided that this is
not worth the portability hassle.

Setting comment-char to multi-letter sequence is not supported at
all, and this code is protecting itself from that nonsense---it does
not need to be fast, and Dscho already gives a fast path for a
single letter case in his patch.

[PATCH 2/3] stripspace: respect repository config

[Junio C Hamano]

From: Johannes Schindelin 

The way "git stripspace" reads the configuration was not quite
correct, in that it forgot to probe for a possibly existing
repository (note: stripspace is designed to be usable outside the
repository as well) before doing so.  Due to this, .git/config was
read only when the command was run from the top-level of the working
tree.  

A recent change b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) stopped reading the repository-local
configuration file ".git/config" unless the repository discovery
process is done, and ".git/config" is no longer read even when run
from the top-level, which exposed the bug even more.

When rebasing interactively with a commentChar defined in the
current repository's config, the help text at the bottom of the edit
script potentially used an incorrect comment character. This was not
only funny-looking, but also resulted in tons of warnings like this
one:

Warning: the command isn't recognized in the following line
 - #

Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 
---
 builtin/stripspace.c  | 4 +++-
 t/t0030-stripspace.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 15e716ef43..1e62a008cb 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char 
*prefix)
if (argc)
usage_with_options(stripspace_usage, options);
 
-   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
+   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
+   setup_git_directory_gently(NULL);
git_config(git_default_config, NULL);
+   }
 
if (strbuf_read(, 0, 1024) < 0)
die_errno("could not read the input");
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index c1f6411eb2..bbf3e39e3d 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
 '
 
-test_expect_failure '-c with comment char defined in .git/config' '
+test_expect_success '-c with comment char defined in .git/config' '
test_config core.commentchar = &&
printf "= foo\n" >expect &&
printf "foo" | (
-- 
2.11.0-rc2-154-g95ba452916

[PATCH 3/3] rebase -i: handle core.commentChar=auto

[Junio C Hamano]

From: Johannes Schindelin 

When 84c9dc2 (commit: allow core.commentChar=auto for character auto
selection, 2014-05-17) extended the core.commentChar functionality to
allow for the value 'auto', it forgot that rebase -i was already taught to
handle core.commentChar, and in turn forgot to let rebase -i handle that
new value gracefully.

Reported by Taufiq Hoven.

Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 
---
 git-rebase--interactive.sh| 13 +++--
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 655ebaa471..c167bc36b3 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -93,8 +93,17 @@ eval '
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
-comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
-: ${comment_char:=#}
+comment_char=$(git config --get core.commentchar 2>/dev/null)
+case "$comment_char" in
+'' | auto)
+   comment_char="#"
+   ;;
+?)
+   ;;
+*)
+   comment_char=$(echo "$comment_char" | cut -c1)
+   ;;
+esac
 
 warn () {
printf '%s\n' "$*" >&2
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d941f0a69f..5d0a7dca9d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -983,7 +983,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_failure 'rebase -i respects core.commentchar=auto' '
+test_expect_success 'rebase -i respects core.commentchar=auto' '
test_config core.commentchar auto &&
write_script copy-edit-script.sh <<-\EOF &&
cp "$1" edit-script
-- 
2.11.0-rc2-154-g95ba452916

[PATCH 1/3] rebase -i: highlight problems with core.commentchar

[Junio C Hamano]

From: Johannes Schindelin 

The interactive rebase does not currently play well with
core.commentchar. Let's add some tests to highlight those problems
that will be fixed in the remainder of the series.

Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 
---
 t/t0030-stripspace.sh |  9 +
 t/t3404-rebase-interactive.sh | 11 +++
 2 files changed, 20 insertions(+)

diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d861c..c1f6411eb2 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
 '
 
+test_expect_failure '-c with comment char defined in .git/config' '
+   test_config core.commentchar = &&
+   printf "= foo\n" >expect &&
+   printf "foo" | (
+   mkdir sub && cd sub && git stripspace -c
+   ) >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'avoid SP-HT sequence in commented line' '
printf "#\tone\n#\n# two\n" >expect &&
printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d6d65a3a94..d941f0a69f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -983,6 +983,17 @@ test_expect_success 'rebase -i respects core.commentchar' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_failure 'rebase -i respects core.commentchar=auto' '
+   test_config core.commentchar auto &&
+   write_script copy-edit-script.sh <<-\EOF &&
+   cp "$1" edit-script
+   EOF
+   test_set_editor "$(pwd)/copy-edit-script.sh" &&
+   test_when_finished "git rebase --abort || :" &&
+   git rebase -i HEAD^ &&
+   test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
+'
+
 test_expect_success 'rebase -i, with  and  specified as 
:/quuxery' '
test_when_finished "git branch -D torebase" &&
git checkout -b torebase branch1 &&
-- 
2.11.0-rc2-154-g95ba452916

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Sixt]

Am 21.11.2016 um 19:40 schrieb Junio C Hamano:

Johannes Sixt  writes:

It could be written without forking a process:

comment_char=${comment_char%${comment_char#?}}

(aka "remove from the end what remains after removing the first character")


Hopefully nobody would include any glob metacharacters in there,
e.g. "core.commentchar='=*'", which would break that?


Heh. I tested a few variations, but not this one. Make it

comment_char=${comment_char%"${comment_char#?}"}

;)

-- Hannes

Re: Feature request: Improve diff algorithm

[Jacob Keller]

On Mon, Nov 21, 2016 at 10:17 AM, Stefan Beller  wrote:
> On Mon, Nov 21, 2016 at 8:56 AM, Jacob Keller  wrote:
>> On Mon, Nov 21, 2016 at 12:11 AM, KES  wrote:
>>> Hi.
>>>
>>
>> Hi,
>>
>>> I have some question about how diff works then give proposal:
>>>
>>> it will be very useful for each "symbol" store additional meta info as 
>>> source line length. So in this case when git counter two equal sequence of 
>>> commands it will do further comparison: Adds 23 chars deletes none VS adds 
>>> 75 chars deletes 46
>>>
>>> Actually I got this:
>>>
>>> @@ -129,8 +132,9 @@ sub _preprocess_message {
>>>  sub _process_message {
>>>  my ($self, $message) = @_;
>>>
>>> -my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>>> +my $time =  [ gettimeofday ];
>>>
>>> +my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>>>  return $self->send_error(ERROR_REQUEST_INVALID)
>>>  unless defined($method);
>>>
>>> Instead of expected:
>>> @@ -129,6 +132,8 @@ sub _preprocess_message {
>>>  sub _process_message {
>>>  my ($self, $message) = @_;
>>>
>>> +my $time =  [ gettimeofday ];
>>> +
>>>  my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>>> -
>>>  return $self->send_error(ERROR_REQUEST_INVALID)
>>>
>>
>> Have you tried the various options for git to search for smaller
>> diffs? Or using the other diff algorithms such as histogram instead of
>> patience?
>>
>
> The newest version of Git comes with a flag to move around the diff
> better, based on the work at https://github.com/mhagger/diff-slider-tools

Unfortunately in this case, I'm not convinced that it will improve the
diff. It's worth a try as well though.

Thanks,
Jake

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Jeff King]

On Mon, Nov 21, 2016 at 10:15:43AM -0800, Junio C Hamano wrote:

> > +   test_cmp expect actual
> > +'
> > +
> 
> Is this a recent regression?  When applied on top of 'maint' or
> older, it seems to pass just fine.
> 
> ... Goes and looks ...
> 
> Interesting.  Peff's b9605bc4f2 ("config: only read .git/config from
> configured repos", 2016-09-12) is where this starts failing, which
> is understandable given the code change to builtin/stripspace.c in
> [2/3]. 
> 
> The analysis of the log message in [2/3] is wrong and needs
> updating, though.  In the old world order it worked by accident to
> call git_config() without calling setup_git_directory(); after
> b9605bc4f2, that no longer is valid and is exposed as a bug.  

Yeah, I noticed that while reading the patch. My b9605bc4f2 did regress
this case, but called out the fact that "cd subdir && git stripspace"
would never have worked. So one step back, 2 steps forward, and Dscho's
patch is the right step forward.

-Peff

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Junio C Hamano]

Johannes Sixt  writes:

> comment_char is a command? Did you mean
>
>   comment_char=$(echo "$comment_char" | cut -c1)

;-)

> It could be written without forking a process:
>
>   comment_char=${comment_char%${comment_char#?}}
>
> (aka "remove from the end what remains after removing the first character")

Hopefully nobody would include any glob metacharacters in there,
e.g. "core.commentchar='=*'", which would break that?

Re: [PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Sixt]

Am 21.11.2016 um 15:18 schrieb Johannes Schindelin:

-comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
-: ${comment_char:=#}
+comment_char=$(git config --get core.commentchar 2>/dev/null)
+case "$comment_char" in
+''|auto)
+   comment_char=#
+   ;;
+?)
+   ;;
+*)
+   comment_char=$(comment_char | cut -c1)


comment_char is a command? Did you mean

comment_char=$(echo "$comment_char" | cut -c1)

It could be written without forking a process:

comment_char=${comment_char%${comment_char#?}}

(aka "remove from the end what remains after removing the first character")

-- Hannes

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Junio C Hamano]

Junio C Hamano  writes:

>> +test_expect_failure '-c with comment char defined in .git/config' '
>> +test_config core.commentchar = &&
>> +printf "= foo\n" >expect &&
>> +printf "foo" | git stripspace -c >actual &&
>
> We'd want "\n" on this printf to match the one before as well, as
> this test is not about "does stripspace complete an incomplete
> line?", I think.  
>
> I could amend it while queuing, but I need to know if I am missing a
> reason why this must be an incomplete line before doing so.
>
>> +test_cmp expect actual
>> +'
>> +
>
> Is this a recent regression?  When applied on top of 'maint' or
> older, it seems to pass just fine.

I think we can force failure by running this test somewhere other
than the top level of the working tree.  A set of proposed amends
incoming ...

Re: Feature request: Improve diff algorithm

[Stefan Beller]

On Mon, Nov 21, 2016 at 8:56 AM, Jacob Keller  wrote:
> On Mon, Nov 21, 2016 at 12:11 AM, KES  wrote:
>> Hi.
>>
>
> Hi,
>
>> I have some question about how diff works then give proposal:
>>
>> it will be very useful for each "symbol" store additional meta info as 
>> source line length. So in this case when git counter two equal sequence of 
>> commands it will do further comparison: Adds 23 chars deletes none VS adds 
>> 75 chars deletes 46
>>
>> Actually I got this:
>>
>> @@ -129,8 +132,9 @@ sub _preprocess_message {
>>  sub _process_message {
>>  my ($self, $message) = @_;
>>
>> -my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>> +my $time =  [ gettimeofday ];
>>
>> +my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>>  return $self->send_error(ERROR_REQUEST_INVALID)
>>  unless defined($method);
>>
>> Instead of expected:
>> @@ -129,6 +132,8 @@ sub _preprocess_message {
>>  sub _process_message {
>>  my ($self, $message) = @_;
>>
>> +my $time =  [ gettimeofday ];
>> +
>>  my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>> -
>>  return $self->send_error(ERROR_REQUEST_INVALID)
>>
>
> Have you tried the various options for git to search for smaller
> diffs? Or using the other diff algorithms such as histogram instead of
> patience?
>

The newest version of Git comes with a flag to move around the diff
better, based on the work at https://github.com/mhagger/diff-slider-tools

Re: [PATCH 1/3] rebase -i: identify problems with core.commentchar

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> index 29e91d8..202ac07 100755
> --- a/t/t0030-stripspace.sh
> +++ b/t/t0030-stripspace.sh
> @@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '
>   test_cmp expect actual
>  '
>  
> +test_expect_failure '-c with comment char defined in .git/config' '
> + test_config core.commentchar = &&
> + printf "= foo\n" >expect &&
> + printf "foo" | git stripspace -c >actual &&

We'd want "\n" on this printf to match the one before as well, as
this test is not about "does stripspace complete an incomplete
line?", I think.  

I could amend it while queuing, but I need to know if I am missing a
reason why this must be an incomplete line before doing so.

> + test_cmp expect actual
> +'
> +

Is this a recent regression?  When applied on top of 'maint' or
older, it seems to pass just fine.

... Goes and looks ...

Interesting.  Peff's b9605bc4f2 ("config: only read .git/config from
configured repos", 2016-09-12) is where this starts failing, which
is understandable given the code change to builtin/stripspace.c in
[2/3].  

The analysis of the log message in [2/3] is wrong and needs
updating, though.  In the old world order it worked by accident to
call git_config() without calling setup_git_directory(); after
b9605bc4f2, that no longer is valid and is exposed as a bug.  

Your [2/3] is a good fix for that change.

In any case, well spotted.

>  test_expect_success 'avoid SP-HT sequence in commented line' '
>   printf "#\tone\n#\n# two\n" >expect &&
>   printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index e38e296..e080399 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -976,6 +976,18 @@ test_expect_success 'rebase -i respects 
> core.commentchar' '
>   test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_failure 'rebase -i respects core.commentchar=auto' '
> + test_config core.commentchar auto &&
> + write_script copy-edit-script.sh <<-\EOF &&
> + cp "$1" edit-script
> + EOF
> + test_set_editor "$(pwd)/copy-edit-script.sh" &&
> + test_when_finished "git rebase --abort || :" &&
> + git rebase -i HEAD^ &&
> + grep "^#" edit-script &&

This was added for debugging that was forgotten?

> + test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"

This says "There shouldn't be any line left once we remove
'#'-commented lines, empty lines and pick insns.".  OK.

The correction in [3/3] seems good.

> +'
> +
>  test_expect_success 'rebase -i, with  and  specified as 
> :/quuxery' '
>   test_when_finished "git branch -D torebase" &&
>   git checkout -b torebase branch1 &&

Re: [PATCH v4 5/6] grep: enable recurse-submodules to work on objects

[Brandon Williams]

On 11/18, Brandon Williams wrote:

> Also, in order to use the tree_entry_interesting code it looks like I'll
> either have to pipe through a flag saying 'yes i want to match against
> submodules' like I did for the other pathspec codepath.  Either that or
> add functionality to perform wildmatching against partial matches (ie
> directories and submodules) since currently the tree_entry_interesting
> code path just punts and says 'well say it matches for now and check
> again later' whenever it runs into a directory (I can't really make it
> do that for submodules without a flag of somesort as tests could break).
> Or maybe both?

Looks like my initial assumption was incorrect, I just needed to be
smarter than punting when running into a submodule.  Should be able to
just ensure that the entry matches up to at least the first wildcard
character before punting and all should be good.

-- 
Brandon Williams

RE: [PATCH 13/16] submodule: teach unpack_trees() to update submodules

[David Turner]

> -Original Message-
> From: Stefan Beller [mailto:sbel...@google.com]

> >> + if (submodule_is_interesting(old->name,
> null_sha1)
> >> + && ok_to_remove_submodule(old->name))
> >> + return 0;
> >> + }
> >
> > Do we need a return 1 in here somewhere?  Because otherwise, we fall
> through and return 0 later.
> 
> Otherwise we would fall through and run
> 
> if (errno == ENOENT)
> return 0;
> return o->gently ? -1 :
> add_rejected_path(o, error_type, ce->name);
> 
> which produces different results than 0?

Oh, I see.  I was misreading that errno check.

Re: [PATCH] i18n: Fixed unmatched single quote in error message

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi,
>
> On Sun, 20 Nov 2016, Jiang Xin wrote:
>
>> Fixed unmatched single quote introduced by commit:
>> 
>>  * f56fffef9a sequencer: teach write_message() to append an optional LF
>> 
>> Signed-off-by: Jiang Xin 
>
> ACK!
>
> Thank you,
> Dscho

Thanks, both.

Re: [PATCH 0/2] add format specifiers to display trailers

[Junio C Hamano]

Jacob Keller  writes:

>> We have %s and %b so that we can reconstruct the whole thing by
>> using both.  It is unclear how %bT fits in this picture.  I wonder
>> if we also need another placeholder that expands to the body of the
>> message without the trailer---otherwise the whole set would become
>> incoherent, no?
>
> I'm not entirely sure what to do here. I just wanted a way to easily
> format "just the trailers" of a message. We could add something that
> formats just the non-trailers, that's not too difficult. Not really
> sure what I'd call it though.

I was wondering if %(log:) was a better way to go.

%(log:title) and %(log:body) would be equivalents of traditional %s
and %b, and %(log:body) in turn would be a shorter way to write
%(log:description)%+(log:trailer), i.e. show the message body, and
if there is a trailer block, add it after adding a blank line.

Or something like that?

Re: [PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined

[Jacob Keller]

On Mon, Nov 21, 2016 at 6:18 AM, Johannes Schindelin
 wrote:
> The Git for Windows project recently got a bug report that detailed how
> `git rebase -i` fails when core.commentchar=auto:
>
> https://groups.google.com/forum/#!topic/git-for-windows/eOZKjkgyX1Q
>
> This patch series fixes rebase -i's handling of core.commentchar.
>
>
> Johannes Schindelin (3):
>   rebase -i: identify problems with core.commentchar
>   stripspace: respect repository config
>   rebase -i: handle core.commentChar=auto
>
>  builtin/stripspace.c  |  4 +++-
>  git-rebase--interactive.sh| 13 +++--
>  t/t0030-stripspace.sh |  7 +++
>  t/t3404-rebase-interactive.sh | 12 
>  4 files changed, 33 insertions(+), 3 deletions(-)
>
>
> base-commit: 1310affe024fba407bff55dbe65cd6d670c8a32d
> Published-As: 
> https://github.com/dscho/git/releases/tag/rebase-i-commentchar-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-commentchar-v1
>
> --
> 2.10.1.583.g721a9e0
>

The series makes sense to me.

Thanks,
Jake

Re: Feature request: Improve diff algorithm

[Jacob Keller]

On Mon, Nov 21, 2016 at 12:11 AM, KES  wrote:
> Hi.
>

Hi,

> I have some question about how diff works then give proposal:
>
> it will be very useful for each "symbol" store additional meta info as source 
> line length. So in this case when git counter two equal sequence of commands 
> it will do further comparison: Adds 23 chars deletes none VS adds 75 chars 
> deletes 46
>
> Actually I got this:
>
> @@ -129,8 +132,9 @@ sub _preprocess_message {
>  sub _process_message {
>  my ($self, $message) = @_;
>
> -my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
> +my $time =  [ gettimeofday ];
>
> +my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
>  return $self->send_error(ERROR_REQUEST_INVALID)
>  unless defined($method);
>
> Instead of expected:
> @@ -129,6 +132,8 @@ sub _preprocess_message {
>  sub _process_message {
>  my ($self, $message) = @_;
>
> +my $time =  [ gettimeofday ];
> +
>  my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
> -
>  return $self->send_error(ERROR_REQUEST_INVALID)
>

Have you tried the various options for git to search for smaller
diffs? Or using the other diff algorithms such as histogram instead of
patience?

Thanks,
Jake

[PATCH 2/3] stripspace: respect repository config

[Johannes Schindelin]

When eff80a9 (Allow custom "comment char", 2013-01-16) taught the
`stripspace` command to respect the config setting `core.commentChar`,
it forgot that this variable may be defined in .git/config.

So when rebasing interactively with a commentChar defined in the current
repository's config, the help text at the bottom of the edit script
potentially used an incorrect comment character. This was not only
funny-looking, but also resulted in tons of warnings like this one:

Warning: the command isn't recognized in the following line
 - #

Signed-off-by: Johannes Schindelin 
---
 builtin/stripspace.c  | 4 +++-
 t/t0030-stripspace.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 15e716e..1e62a00 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -44,8 +44,10 @@ int cmd_stripspace(int argc, const char **argv, const char 
*prefix)
if (argc)
usage_with_options(stripspace_usage, options);
 
-   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
+   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
+   setup_git_directory_gently(NULL);
git_config(git_default_config, NULL);
+   }
 
if (strbuf_read(, 0, 1024) < 0)
die_errno("could not read the input");
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 202ac07..67f77df 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,7 +432,7 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
 '
 
-test_expect_failure '-c with comment char defined in .git/config' '
+test_expect_success '-c with comment char defined in .git/config' '
test_config core.commentchar = &&
printf "= foo\n" >expect &&
printf "foo" | git stripspace -c >actual &&
-- 
2.10.1.583.g721a9e0

Re: [PATCH] i18n: Fixed unmatched single quote in error message

[Johannes Schindelin]

Hi,

On Sun, 20 Nov 2016, Jiang Xin wrote:

> Fixed unmatched single quote introduced by commit:
> 
>  * f56fffef9a sequencer: teach write_message() to append an optional LF
> 
> Signed-off-by: Jiang Xin 

ACK!

Thank you,
Dscho

[PATCH 3/3] rebase -i: handle core.commentChar=auto

[Johannes Schindelin]

When 84c9dc2 (commit: allow core.commentChar=auto for character auto
selection, 2014-05-17) extended the core.commentChar functionality to
allow for the value 'auto', it forgot that rebase -i was already taught to
handle core.commentChar, and in turn forgot to let rebase -i handle that
new value gracefully.

Reported by Taufiq Hoven.

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh| 13 +++--
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ca994c5..6bb740c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -93,8 +93,17 @@ eval '
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
-comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
-: ${comment_char:=#}
+comment_char=$(git config --get core.commentchar 2>/dev/null)
+case "$comment_char" in
+''|auto)
+   comment_char=#
+   ;;
+?)
+   ;;
+*)
+   comment_char=$(comment_char | cut -c1)
+   ;;
+esac
 
 warn () {
printf '%s\n' "$*" >&2
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e080399..0f3d177 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,7 +976,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_failure 'rebase -i respects core.commentchar=auto' '
+test_expect_success 'rebase -i respects core.commentchar=auto' '
test_config core.commentchar auto &&
write_script copy-edit-script.sh <<-\EOF &&
cp "$1" edit-script
-- 
2.10.1.583.g721a9e0

[PATCH 1/3] rebase -i: identify problems with core.commentchar

[Johannes Schindelin]

The interactive rebase does not currently play well with
core.commentchar. Let's add some tests to identify those problems.

Signed-off-by: Johannes Schindelin 
---
 t/t0030-stripspace.sh |  7 +++
 t/t3404-rebase-interactive.sh | 12 
 2 files changed, 19 insertions(+)

diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d8..202ac07 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -432,6 +432,13 @@ test_expect_success '-c with changed comment char' '
test_cmp expect actual
 '
 
+test_expect_failure '-c with comment char defined in .git/config' '
+   test_config core.commentchar = &&
+   printf "= foo\n" >expect &&
+   printf "foo" | git stripspace -c >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'avoid SP-HT sequence in commented line' '
printf "#\tone\n#\n# two\n" >expect &&
printf "\tone\n\ntwo\n" | git stripspace -c >actual &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e38e296..e080399 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,6 +976,18 @@ test_expect_success 'rebase -i respects core.commentchar' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_failure 'rebase -i respects core.commentchar=auto' '
+   test_config core.commentchar auto &&
+   write_script copy-edit-script.sh <<-\EOF &&
+   cp "$1" edit-script
+   EOF
+   test_set_editor "$(pwd)/copy-edit-script.sh" &&
+   test_when_finished "git rebase --abort || :" &&
+   git rebase -i HEAD^ &&
+   grep "^#" edit-script &&
+   test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
+'
+
 test_expect_success 'rebase -i, with  and  specified as 
:/quuxery' '
test_when_finished "git branch -D torebase" &&
git checkout -b torebase branch1 &&
-- 
2.10.1.583.g721a9e0

[PATCH 0/3] Fix problems with rebase -i when core.commentchar is defined

[Johannes Schindelin]

The Git for Windows project recently got a bug report that detailed how
`git rebase -i` fails when core.commentchar=auto:

https://groups.google.com/forum/#!topic/git-for-windows/eOZKjkgyX1Q

This patch series fixes rebase -i's handling of core.commentchar.


Johannes Schindelin (3):
  rebase -i: identify problems with core.commentchar
  stripspace: respect repository config
  rebase -i: handle core.commentChar=auto

 builtin/stripspace.c  |  4 +++-
 git-rebase--interactive.sh| 13 +++--
 t/t0030-stripspace.sh |  7 +++
 t/t3404-rebase-interactive.sh | 12 
 4 files changed, 33 insertions(+), 3 deletions(-)


base-commit: 1310affe024fba407bff55dbe65cd6d670c8a32d
Published-As: https://github.com/dscho/git/releases/tag/rebase-i-commentchar-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-commentchar-v1

-- 
2.10.1.583.g721a9e0

--root seems to cancel out --keep-empty in git-rebase

[Pierre Ossman]

As the subject says, these two commands give identical behaviour:

git rebase --interactive --keep-empty --root

and:

git rebase --interactive --root

I.e. --keep-empty has no effect when --root is specified.

Ideas for workaround welcome.

Regards
--
Pierre Ossman   Software Development
Cendio AB   https://cendio.com
Teknikringen 8  https://twitter.com/ThinLinc
583 30 Linköpinghttps://facebook.com/ThinLinc
Phone: +46-13-214600https://plus.google.com/+CendioThinLinc

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Re: [PATCH v7 14/17] ref-filter: allow porcelain to translate messages in the output

[Matthieu Moy]

Karthik Nayak  writes:

> cc'in Matthieu since he wrote the patch.
>
> On Sat, Nov 19, 2016 at 4:16 AM, Jakub Narębski  wrote:
>> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>>> From: Karthik Nayak 
>>>
>>> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
>>> the atom %(upstream:track) can be translated if needed. This is needed
>>> as we port branch.c to use ref-filter's printing API's.
>>>
>>> Written-by: Matthieu Moy 
>>> Mentored-by: Christian Couder 
>>> Mentored-by: Matthieu Moy 
>>> Signed-off-by: Karthik Nayak 
>>> ---
>>>  ref-filter.c | 28 
>>>  ref-filter.h |  2 ++
>>>  2 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ref-filter.c b/ref-filter.c
>>> index b47b900..944671a 100644
>>> --- a/ref-filter.c
>>> +++ b/ref-filter.c
>>> @@ -15,6 +15,26 @@
>>>  #include "version.h"
>>>  #include "wt-status.h"
>>>
>>> +static struct ref_msg {
>>> + const char *gone;
>>> + const char *ahead;
>>> + const char *behind;
>>> + const char *ahead_behind;
>>> +} msgs = {
>>> + "gone",
>>> + "ahead %d",
>>> + "behind %d",
>>> + "ahead %d, behind %d"
>>> +};
>>> +
>>> +void setup_ref_filter_porcelain_msg(void)
>>> +{
>>> + msgs.gone = _("gone");
>>> + msgs.ahead = _("ahead %d");
>>> + msgs.behind = _("behind %d");
>>> + msgs.ahead_behind = _("ahead %d, behind %d");
>>> +}
>>
>> Do I understand it correctly that this mechanism is here to avoid
>> repeated calls into gettext, as those messages would get repeated
>> over and over; otherwise one would use foo = N_("...") and _(foo),
>> isn't it?

That's not the primary goal. The primary goal is to keep untranslated,
and immutable messages in plumbing commands. We may decide one day that
_("gone") is not the best message for the end user and replace it with,
say, _("vanished"), but the "gone" has to remain the same forever and
regardless of the user's config for scripts using it.

We could have written

  in_porcelain ? _("gone") : "gone"

here and there in the code, but having a single place where we set all
the messages seems simpler. Call setup_ref_filter_porcelain_msg() and
get the porcelain messages, don't call it and keep the plumbing
messages.

Note that it's not the first place in the code where we do this, see
setup_unpack_trees_porcelain in unpack-trees.c for another instance.

Karthik: the commit message could be improved, for example:

Introduce setup_ref_filter_porcelain_msg() so that the messages used in
the atom %(upstream:track) can be translated if needed. By default, keep
the messages untranslated, which is the right behavior for plumbing
commands. This is needed as we port branch.c to use ref-filter's
printing API's.

Why not a comment right below "} msgs = {" saying e.g.:

/* Untranslated plumbing messages: */

>> I wonder if there is some way to avoid duplication here, but I don't
>> see anything easy and safe (e.g. against running setup_*() twice).

I think we do need duplication for the reason above.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Feature request: Improve diff algorithm

[KES]

Hi.

I have some question about how diff works then give proposal:

it will be very useful for each "symbol" store additional meta info as source 
line length. So in this case when git counter two equal sequence of commands it 
will do further comparison: Adds 23 chars deletes none VS adds 75 chars deletes 
46

Actually I got this:

@@ -129,8 +132,9 @@ sub _preprocess_message {
 sub _process_message {
 my ($self, $message) = @_;

-my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
+my $time =  [ gettimeofday ];

+my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
 return $self->send_error(ERROR_REQUEST_INVALID)
 unless defined($method);

Instead of expected:
@@ -129,6 +132,8 @@ sub _preprocess_message {
 sub _process_message {
 my ($self, $message) = @_;

+my $time =  [ gettimeofday ];
+
 my $method = ref($message) eq 'HASH' ? $message->{method} : undef;
- 
 return $self->send_error(ERROR_REQUEST_INVALID)


Details: 
http://stackoverflow.com/questions/40550751/unexpected-result-in-git-diff/40552165?noredirect=1#comment68648377_40552165