Did You Get My Message This Time?

[Friedrich Mayrhofer]

Friedrich MayrhoferThis is the second time i am sending you this mail.I, 
Friedrich Mayrhofer Donate $ 1,000,000.00 to You, Email Me personally for more 
details.

Regards.
Friedrich Mayrhofer

dangling commits in worktree

[Van Oostenryck Luc]

Hi,

More or less by error I used the fsck command in a worktree and I had
the surprised to see that it reported a lot of dangling commits while it was
not supposed to have one.
I quickly realized that it was the case only in the worktree, in the main dir
things were OK. While experimenting a bit I also saw that git gc had not the
same effect in a worktree than in the main tree (the pack was smaller, more
files were left in objects/xx/ dirs), which is even more odd and a bit
scary when thinking to the pruning.

This seems like a bug to me and googling about it didn't returned anything.

Luc Van Oostenryck

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

[Jeff King]

On Tue, Nov 22, 2016 at 01:55:07PM -0800, Junio C Hamano wrote:

> > And this test makes sense. Even without "sub", it would show the
> > regression, but it's a good idea to test the sub-directory case to cover
> > the path-munging.
> 
> Yup.  Obviously during my initial attempt I was scratching my head
> wondering where these two files went--they were later found inside
> t/ directory which was really bad ;-)

Heh. Yeah, it's nice when the failure mode doesn't escape the trash
directory, but it's probably not worth worrying about too much.

> > In the "archive --remote" test I added, we may want to do the same to
> > show that "--output" points at the correct path.
> 
> Perhaps something like this.  By going down one level, we make sure
> that it is not sufficient to accidentally read from .git/config to
> find out what 'foo' is, and also ../b5-nick.tar that is relative to
> the prefix (aka 'a/') ends up at the top-level.

Yeah, your modification looks good.

-Peff

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

[Brandon Williams]

On 11/22, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> >> > So this change may have an impact on "git ls-tree -r" with pathspec;
> >> > I offhand do not know if that impact is undesirable or not.  A test
> >> > or two may be in order to illustrate what happens?  With a submodule
> >> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> >> > something like that, perhaps?
> >> 
> >> Maybe unrelated, but it looks like wildcard characters are overridden in
> >> ls-tree.c per '170260ae'.  As such wildmatching just doesn't work with
> >> ls-tree.  so `git ls-tree -r HEAD -- "*"` results in no hits.
> >
> > Wrong commit.  Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
> > that disabled wildmatching since it is 'plumbing'
> 
> OK.  Things that share tree-walk other than "ls-tree -r" are still
> affected, no?

Yeah potentially, though I'm having a difficult time finding a case that
would actually be affected.

-- 
Brandon Williams

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

[Junio C Hamano]

Brandon Williams  writes:

>> > So this change may have an impact on "git ls-tree -r" with pathspec;
>> > I offhand do not know if that impact is undesirable or not.  A test
>> > or two may be in order to illustrate what happens?  With a submodule
>> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
>> > something like that, perhaps?
>> 
>> Maybe unrelated, but it looks like wildcard characters are overridden in
>> ls-tree.c per '170260ae'.  As such wildmatching just doesn't work with
>> ls-tree.  so `git ls-tree -r HEAD -- "*"` results in no hits.
>
> Wrong commit.  Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
> that disabled wildmatching since it is 'plumbing'

OK.  Things that share tree-walk other than "ls-tree -r" are still
affected, no?

Announce: delaying 2.11 final by one week

[Junio C Hamano]

We'd like to cook and merge a few hot-fix topics to 'master' before
the upcoming release, in order to fix minor regressions in
"stripspace" (which affects "rebase -i"), "archive" and "mailinfo"
that were introduced during this cycle.  So I'll be cutting -rc3
soon this week, and hopefully tagging the final by the end of this
month.

Thanks.

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

[Brandon Williams]

On 11/22, Brandon Williams wrote:
> On 11/22, Junio C Hamano wrote:
> > Brandon Williams  writes:
> > 
> > > diff --git a/tree-walk.c b/tree-walk.c
> > > index 828f435..ff77605 100644
> > > --- a/tree-walk.c
> > > +++ b/tree-walk.c
> > > @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct 
> > > name_entry *entry,
> > >*/
> > >   if (ps->recursive && S_ISDIR(entry->mode))
> > >   return entry_interesting;
> > > +
> > > + /*
> > > +  * When matching against submodules with
> > > +  * wildcard characters, ensure that the entry
> > > +  * at least matches up to the first wild
> > > +  * character.  More accurate matching can then
> > > +  * be performed in the submodule itself.
> > > +  */
> > > + if (ps->recursive && S_ISGITLINK(entry->mode) &&
> > > + !ps_strncmp(item, match + baselen,
> > > + entry->path,
> > > + item->nowildcard_len - baselen))
> > > + return entry_interesting;
> > >   }
> > 
> > This one (and the other hunk) feels more correct than the previous
> > round.  One thing to keep in mind however is that ps->recursive is
> > about "do we show a tree as a tree aka 04, or do we descend into
> > it to show its contents?", not about "do we recurse into submodules?",
> > AFAICT.
> > 
> > So this change may have an impact on "git ls-tree -r" with pathspec;
> > I offhand do not know if that impact is undesirable or not.  A test
> > or two may be in order to illustrate what happens?  With a submodule
> > at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> > something like that, perhaps?
> 
> Maybe unrelated, but it looks like wildcard characters are overridden in
> ls-tree.c per '170260ae'.  As such wildmatching just doesn't work with
> ls-tree.  so `git ls-tree -r HEAD -- "*"` results in no hits.

Wrong commit.  Its this one (f0096c06bcdeb7aa6ae8a749ddc9d6d4a2c381d1)
that disabled wildmatching since it is 'plumbing'

-- 
Brandon Williams

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

[Brandon Williams]

On 11/22, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > diff --git a/tree-walk.c b/tree-walk.c
> > index 828f435..ff77605 100644
> > --- a/tree-walk.c
> > +++ b/tree-walk.c
> > @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct 
> > name_entry *entry,
> >  */
> > if (ps->recursive && S_ISDIR(entry->mode))
> > return entry_interesting;
> > +
> > +   /*
> > +* When matching against submodules with
> > +* wildcard characters, ensure that the entry
> > +* at least matches up to the first wild
> > +* character.  More accurate matching can then
> > +* be performed in the submodule itself.
> > +*/
> > +   if (ps->recursive && S_ISGITLINK(entry->mode) &&
> > +   !ps_strncmp(item, match + baselen,
> > +   entry->path,
> > +   item->nowildcard_len - baselen))
> > +   return entry_interesting;
> > }
> 
> This one (and the other hunk) feels more correct than the previous
> round.  One thing to keep in mind however is that ps->recursive is
> about "do we show a tree as a tree aka 04, or do we descend into
> it to show its contents?", not about "do we recurse into submodules?",
> AFAICT.
> 
> So this change may have an impact on "git ls-tree -r" with pathspec;
> I offhand do not know if that impact is undesirable or not.  A test
> or two may be in order to illustrate what happens?  With a submodule
> at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
> something like that, perhaps?

Maybe unrelated, but it looks like wildcard characters are overridden in
ls-tree.c per '170260ae'.  As such wildmatching just doesn't work with
ls-tree.  so `git ls-tree -r HEAD -- "*"` results in no hits.

-- 
Brandon Williams

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

[Junio C Hamano]

Brandon Williams  writes:

> diff --git a/tree-walk.c b/tree-walk.c
> index 828f435..ff77605 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct 
> name_entry *entry,
>*/
>   if (ps->recursive && S_ISDIR(entry->mode))
>   return entry_interesting;
> +
> + /*
> +  * When matching against submodules with
> +  * wildcard characters, ensure that the entry
> +  * at least matches up to the first wild
> +  * character.  More accurate matching can then
> +  * be performed in the submodule itself.
> +  */
> + if (ps->recursive && S_ISGITLINK(entry->mode) &&
> + !ps_strncmp(item, match + baselen,
> + entry->path,
> + item->nowildcard_len - baselen))
> + return entry_interesting;
>   }

This one (and the other hunk) feels more correct than the previous
round.  One thing to keep in mind however is that ps->recursive is
about "do we show a tree as a tree aka 04, or do we descend into
it to show its contents?", not about "do we recurse into submodules?",
AFAICT.

So this change may have an impact on "git ls-tree -r" with pathspec;
I offhand do not know if that impact is undesirable or not.  A test
or two may be in order to illustrate what happens?  With a submodule
at "sub/module", running "git ls-tree -r HEAD -- sub/module/*" or
something like that, perhaps?

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

[Brandon Williams]

Series looks good to me. At least the issues I raised are fixed.

-- 
Brandon Williams

Re: [GIT PULL] l10n updates for 2.11.0 round 2

[Junio C Hamano]

Jiang Xin  writes:

> Git 2.11.0-rc2 introduced a very small l10n update.  Because the
> update windows will be
> closed tomorrow, I made a batch updates. See commit:
>
> * https://github.com/git-l10n/git-po/commit/275588f93
>
> I have a reduced diff for this commit using a custom diff driver, see:
>
> * https://gist.github.com/jiangxin/6384b1e865249228e00385fab84ef3f3
>
>
> 2016-11-22 22:59 GMT+08:00 Jiang Xin :
>> Hi Junio,
>>
>> The following changes since commit 1310affe024fba407bff55dbe65cd6d670c8a32d:
>>
>>   Git 2.11-rc2 (2016-11-17 13:47:36 -0800)
>>
>> are available in the git repository at:
>>
>>   git://github.com/git-l10n/git-po tags/l10n-2.11.0-rnd2

Thanks, pulled.

FYI, we seem to need a bit more time on the code front so we'll have
an unscheduled 2.11-rc3 tomorrow.  The final one will be near the
end of the month, tenatively scheduled on Nov 29th.

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

[Junio C Hamano]

Jeff King  writes:

>> +test_expect_success 'mailinfo with mailinfo.scissors config' '
>> +test_config mailinfo.scissors true &&
>> +(
>> +mkdir sub &&
>> +cd sub &&
>> +git mailinfo ../msg0014.sc ../patch0014.sc <../0014 
>> >../info0014.sc
>> +) &&
>> +test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
>> +test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
>> +test_cmp "$DATA/info0014--scissors" info0014.sc
>> +'
>
> And this test makes sense. Even without "sub", it would show the
> regression, but it's a good idea to test the sub-directory case to cover
> the path-munging.

Yup.  Obviously during my initial attempt I was scratching my head
wondering where these two files went--they were later found inside
t/ directory which was really bad ;-)

> In the "archive --remote" test I added, we may want to do the same to
> show that "--output" points at the correct path.

Perhaps something like this.  By going down one level, we make sure
that it is not sufficient to accidentally read from .git/config to
find out what 'foo' is, and also ../b5-nick.tar that is relative to
the prefix (aka 'a/') ends up at the top-level.

 t/t5000-tar-tree.sh | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 09df7f0458..830bf2a2f6 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -195,7 +195,10 @@ test_expect_success 'git archive --remote' \
 
 test_expect_success 'git archive --remote with configured remote' '
git config remote.foo.url . &&
-   git archive --remote=foo HEAD >b5-nick.tar &&
+   (
+   cd a &&
+   git archive --remote=foo --output=../b5-nick.tar HEAD
+   ) &&
test_cmp_bin b.tar b5-nick.tar
 '
 

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

[Jeff King]

On Tue, Nov 22, 2016 at 01:22:24PM -0800, Junio C Hamano wrote:

> OK.  The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
> as it takes paths from the command line that needs to be adjusted
> for the prefix.

I wondered at first if our lack of parse-options here was holding us
back from using OPT_FILENAME. Though even if that was the case, it is
probably better to do the minimal fix in this instance, rather than
converting the option parsing.

However, the paths which need munged are not even options, they are
arguments. So we wouldn't be able to rely on OPT_FILENAME anyway. I'm
surprised we haven't had to deal with this elsewhere, but I guess most
commands don't take arbitrary filenames (things like `add` take
pathspecs, and then the pathspec machinery takes care of the details).

The only other case I could think of is git-apply, and it does use
prefix_filename() correctly.

> +static char *prefix_copy(const char *prefix, const char *filename)
> +{
> + if (!prefix || is_absolute_path(filename))
> + return xstrdup(filename);
> + return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
> +}

So this is more or less a copy of parse-option's fix_filename(), which
makes sense.

I noticed that git-apply does not check is_absolute_path(), but that's
OK, as prefix_filename() does. So I think it would be correct to drop it
here, but it doesn't matter much either way.

>  int cmd_mailinfo(int argc, const char **argv, const char *prefix)
>  {
>   const char *def_charset;
>   struct mailinfo mi;
>   int status;
> + char *msgfile, *patchfile;
>  
> - /* NEEDSWORK: might want to do the optional .git/ directory
> -  * discovery
> -  */
>   setup_mailinfo();

This part looks straightforward and correct. Dropping the NEEDSWORK is a
nice bonus.

> +test_expect_success 'mailinfo with mailinfo.scissors config' '
> + test_config mailinfo.scissors true &&
> + (
> + mkdir sub &&
> + cd sub &&
> + git mailinfo ../msg0014.sc ../patch0014.sc <../0014 
> >../info0014.sc
> + ) &&
> + test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
> + test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
> + test_cmp "$DATA/info0014--scissors" info0014.sc
> +'

And this test makes sense. Even without "sub", it would show the
regression, but it's a good idea to test the sub-directory case to cover
the path-munging.

In the "archive --remote" test I added, we may want to do the same to
show that "--output" points at the correct path.

-Peff

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

[Jeff King]

On Tue, Nov 22, 2016 at 04:19:20PM -0500, Jeff King wrote:

> > > Do you want to do another round of -rc3? Ship with the
> > > minor regressions and fix them up in v2.11.1?
> > 
> > I am leaning towards the former (though we may also end up doing the
> > latter).
> 
> I think I'd lead towards -rc3, as well. Our schedule is somewhat

I meant s/lead/lean/, of course.

> Something like this, which does all but the last (and that should
> probably happen separately post-release).

As usual, I'm utterly confused by the $(pwd) versus $PWD thing. So let
me review what I did to see if it makes sense. :)

> +# run "$@" inside a non-git directory
> +nongit () {
> + test -d non-repo ||
> + mkdir non-repo ||
> + return 1
> +
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd non-repo &&
> + "$@"
> + )
> +}

I copied this bit from t1515, which sets up a similar scenario. I think
it _probably_ works either way because of the environment-variable
conversion magic that Windows bash does.

> +test_expect_success 'git archive --remote outside of a git repo' '
> + git archive HEAD >expect.tar &&
> + nongit git archive --remote="$PWD" HEAD >actual.tar &&
> + test_cmp_bin expect.tar actual.tar
> +'

I'm not sure if it matters here. I almost wrote $TRASH_DIRECTORY, but
that I think is in the same form as $PWD. Either would be fine.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:
>
>> > Do you want to do another round of -rc3? Ship with the
>> > minor regressions and fix them up in v2.11.1?
>> 
>> I am leaning towards the former (though we may also end up doing the
>> latter).
>
> I think I'd lead towards -rc3, as well. Our schedule is somewhat
> arbitrary anyway, and one week is less important than somebody picking
> up the new release and trying to puzzle out a regression that we already
> have a fix for.

OK.  The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
as it takes paths from the command line that needs to be adjusted
for the prefix.

-- >8 --
From: Junio C Hamano 
Date: Tue, 22 Nov 2016 13:13:16 -0800
Subject: [PATCH] mailinfo: read local configuration

Since b9605bc4f2 ("config: only read .git/config from configured
repos", 2016-09-12), we do not read from ".git/config" unless we
know we are in a repository.  "git mailinfo" however didn't do the
repository discovery and instead relied on the old behaviour.  This
was mostly OK because it was merely run as a helper program by other
porcelain scripts that first chdir's up to the root of the working
tree.

Teach the command to run a "gentle" version of repository discovery
so that local configuration variables like mailinfo.scissors are
honoured.

Signed-off-by: Junio C Hamano 
---
 builtin/mailinfo.c  | 19 +++
 git.c   |  2 +-
 t/t5100-mailinfo.sh | 13 +
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index f6df274111..e3b62f2fc7 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -11,15 +11,20 @@
 static const char mailinfo_usage[] =
"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding= 
| -n] [--scissors | --no-scissors]   < mail >info";
 
+static char *prefix_copy(const char *prefix, const char *filename)
+{
+   if (!prefix || is_absolute_path(filename))
+   return xstrdup(filename);
+   return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
+}
+
 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 {
const char *def_charset;
struct mailinfo mi;
int status;
+   char *msgfile, *patchfile;
 
-   /* NEEDSWORK: might want to do the optional .git/ directory
-* discovery
-*/
setup_mailinfo();
 
def_charset = get_commit_output_encoding();
@@ -54,8 +59,14 @@ int cmd_mailinfo(int argc, const char **argv, const char 
*prefix)
 
mi.input = stdin;
mi.output = stdout;
-   status = !!mailinfo(, argv[1], argv[2]);
+
+   msgfile = prefix_copy(prefix, argv[1]);
+   patchfile = prefix_copy(prefix, argv[2]);
+
+   status = !!mailinfo(, msgfile, patchfile);
clear_mailinfo();
 
+   free(msgfile);
+   free(patchfile);
return status;
 }
diff --git a/git.c b/git.c
index efa1059fe0..fd3abf85d2 100644
--- a/git.c
+++ b/git.c
@@ -445,7 +445,7 @@ static struct cmd_struct commands[] = {
{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-   { "mailinfo", cmd_mailinfo },
+   { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY },
{ "mailsplit", cmd_mailsplit },
{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
{ "merge-base", cmd_merge_base, RUN_SETUP },
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index e6b995161e..7171f67539 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -158,4 +158,17 @@ test_expect_success 'mailinfo handles rfc2822 comment' '
test_cmp "$DATA/comment.expect" comment/info
 '
 
+test_expect_success 'mailinfo with mailinfo.scissors config' '
+   test_config mailinfo.scissors true &&
+   (
+   mkdir sub &&
+   cd sub &&
+   git mailinfo ../msg0014.sc ../patch0014.sc <../0014 
>../info0014.sc
+   ) &&
+   test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
+   test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
+   test_cmp "$DATA/info0014--scissors" info0014.sc
+'
+
+
 test_done
-- 
2.11.0-rc2-170-g4c384f318f

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

[Jeff King]

On Tue, Nov 22, 2016 at 12:24:15PM -0800, Junio C Hamano wrote:

> > Do you want to do another round of -rc3? Ship with the
> > minor regressions and fix them up in v2.11.1?
> 
> I am leaning towards the former (though we may also end up doing the
> latter).

I think I'd lead towards -rc3, as well. Our schedule is somewhat
arbitrary anyway, and one week is less important than somebody picking
up the new release and trying to puzzle out a regression that we already
have a fix for.

> Here is an initial attempt.  It turns out that write_archive() must
> be callable outside a repository to respond to "git archive --list",
> which happens in parse_archive_args(), and the "have-repository?"
> check cannot be moved before that to the caller.

Right, that makes sense. t5000 already tests git-archive outside of a
repo, although it does it in a funny way (it sets GIT_DIR rather than
relying on GIT_CEILING_DIRECTORIES). We should cover that and make sure
that --remote works outside of a repo. Those work now, but we want to
make sure we don't regress them.

And then we want to test that --remote can read a configured remote
name.

And then post-v2.11, we'd want to check that --remote=foo outside of a
repo produces a sane error instead of looking for a bogus
$GIT_DIR/remotes/foo.

Something like this, which does all but the last (and that should
probably happen separately post-release).

---
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 80b238734..09df7f045 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -94,6 +94,20 @@ check_tar() {
'
 }
 
+# run "$@" inside a non-git directory
+nongit () {
+   test -d non-repo ||
+   mkdir non-repo ||
+   return 1
+
+   (
+   GIT_CEILING_DIRECTORIES=$(pwd) &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd non-repo &&
+   "$@"
+   )
+}
+
 test_expect_success \
 'populate workdir' \
 'mkdir a &&
@@ -179,6 +193,12 @@ test_expect_success 'git archive --remote' \
 'git archive --remote=. HEAD >b5.tar &&
 test_cmp_bin b.tar b5.tar'
 
+test_expect_success 'git archive --remote with configured remote' '
+   git config remote.foo.url . &&
+   git archive --remote=foo HEAD >b5-nick.tar &&
+   test_cmp_bin b.tar b5-nick.tar
+'
+
 test_expect_success \
 'validate file modification time' \
 'mkdir extract &&
@@ -197,9 +217,15 @@ test_expect_success 'git archive with --output, override 
inferred format' '
test_cmp_bin b.tar d4.zip
 '
 
-test_expect_success \
-'git archive --list outside of a git repo' \
-'GIT_DIR=some/non-existing/directory git archive --list'
+test_expect_success 'git archive --list outside of a git repo' '
+   nongit git archive --list
+'
+
+test_expect_success 'git archive --remote outside of a git repo' '
+   git archive HEAD >expect.tar &&
+   nongit git archive --remote="$PWD" HEAD >actual.tar &&
+   test_cmp_bin expect.tar actual.tar
+'
 
 test_expect_success 'clients cannot access unreachable commits' '
test_commit unreachable &&

Re: [PATCH] git-gui: pass the branch name to git merge

[Johannes Sixt]

Am 22.11.2016 um 20:16 schrieb Junio C Hamano:

Can't this be handled on the "git merge FETCH_HEAD" codepath
instead?


Absolutely. Any takers? ;)

-- Hannes

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

[Junio C Hamano]

Jeff King  writes:

> So what do you want to do for v2.11? I think there are minor regressions
> in stripspace, archive, and mailinfo with respect to reading
> .git/config. The right solution in each case is to do a gentle repo
> setup. We have patches for stripspace already, but not yet for the other
> two. They _should_ be fairly trivial, but we are getting awfully close
> to the release. 

Right.

> Do you want to do another round of -rc3? Ship with the
> minor regressions and fix them up in v2.11.1?

I am leaning towards the former (though we may also end up doing the
latter).

Here is an initial attempt.  It turns out that write_archive() must
be callable outside a repository to respond to "git archive --list",
which happens in parse_archive_args(), and the "have-repository?"
check cannot be moved before that to the caller.

No tests yet.  Help appreciated ;-)


 archive.c| 9 ++---
 archive.h| 2 +-
 builtin/archive.c| 6 +++---
 builtin/upload-archive.c | 2 +-
 git.c| 4 ++--
 5 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/archive.c b/archive.c
index dde1ab4c79..901b10264f 100644
--- a/archive.c
+++ b/archive.c
@@ -504,15 +504,11 @@ static int parse_archive_args(int argc, const char **argv,
 }
 
 int write_archive(int argc, const char **argv, const char *prefix,
- int setup_prefix, const char *name_hint, int remote)
+ const char *name_hint, int remote)
 {
-   int nongit = 0;
const struct archiver *ar = NULL;
struct archiver_args args;
 
-   if (setup_prefix && prefix == NULL)
-   prefix = setup_git_directory_gently();
-
git_config_get_bool("uploadarchive.allowunreachable", 
_allow_unreachable);
git_config(git_default_config, NULL);
 
@@ -520,7 +516,7 @@ int write_archive(int argc, const char **argv, const char 
*prefix,
init_zip_archiver();
 
argc = parse_archive_args(argc, argv, , , name_hint, remote);
-   if (nongit) {
+   if (!startup_info->have_repository) {
/*
 * We know this will die() with an error, so we could just
 * die ourselves; but its error message will be more specific
@@ -528,7 +524,6 @@ int write_archive(int argc, const char **argv, const char 
*prefix,
 */
setup_git_directory();
}
-
parse_treeish_arg(argv, , prefix, remote);
parse_pathspec_arg(argv + 1, );
 
diff --git a/archive.h b/archive.h
index 4a791e1fed..415e0152e2 100644
--- a/archive.h
+++ b/archive.h
@@ -36,7 +36,7 @@ typedef int (*write_archive_entry_fn_t)(struct archiver_args 
*args,
unsigned int mode);
 
 extern int write_archive_entries(struct archiver_args *args, 
write_archive_entry_fn_t write_entry);
-extern int write_archive(int argc, const char **argv, const char *prefix, int 
setup_prefix, const char *name_hint, int remote);
+extern int write_archive(int argc, const char **argv, const char *prefix, 
const char *name_hint, int remote);
 
 const char *archive_format_from_filename(const char *filename);
 extern void *sha1_file_to_archive(const struct archiver_args *args,
diff --git a/builtin/archive.c b/builtin/archive.c
index 49f491413a..f863465a0f 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -85,8 +85,8 @@ int cmd_archive(int argc, const char **argv, const char 
*prefix)
const char *output = NULL;
const char *remote = NULL;
struct option local_opts[] = {
-   OPT_STRING('o', "output", , N_("file"),
-   N_("write the archive to this file")),
+   OPT_FILENAME('o', "output", ,
+N_("write the archive to this file")),
OPT_STRING(0, "remote", , N_("repo"),
N_("retrieve the archive from remote repository 
")),
OPT_STRING(0, "exec", , N_("command"),
@@ -105,5 +105,5 @@ int cmd_archive(int argc, const char **argv, const char 
*prefix)
 
setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-   return write_archive(argc, argv, prefix, 1, output, 0);
+   return write_archive(argc, argv, prefix, output, 0);
 }
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index dc872f6a94..cde06977b7 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -43,7 +43,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, 
const char *prefix)
}
 
/* parse all options sent by the client */
-   return write_archive(sent_argv.argc, sent_argv.argv, prefix, 0, NULL, 
1);
+   return write_archive(sent_argv.argc, sent_argv.argv, prefix, NULL, 1);
 }
 
 __attribute__((format (printf, 1, 2)))
diff --git a/git.c b/git.c
index efa1059fe0..e8b2baf2d1 100644
--- a/git.c
+++ b/git.c
@@ -396,7 +396,7 @@ static struct cmd_struct commands[] = {
{ "am", cmd_am, RUN_SETUP | 

[PATCHv4 2/3] submodule-config: rename commit_sha1 to treeish_name

[Stefan Beller]

It is also possible to pass in any treeish name to lookup a submodule
config. Make it clear by naming the variables accordingly. 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 |  9 ++---
 submodule-config.c   | 46 
 submodule-config.h   |  4 +--
 t/t7411-submodule-config.sh  | 14 
 4 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/Documentation/technical/api-submodule-config.txt 
b/Documentation/technical/api-submodule-config.txt
index 941fa178dd..8285bcc605 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -47,15 +47,16 @@ 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 
*treeish_name, const char *path)`::
 
-   Lookup values for one submodule by its commit_sha1 and path.
+   Given a tree-ish in the superproject and a path, return the
+   submodule that is bound at the path in the named tree.
 
-`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, 
const char *name)`::
+`const struct submodule *submodule_from_name(const unsigned char 
*treeish_name, 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 treeish_name 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..ec13ab5a3d 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 *treeish_name,
 const char *name, const char *option)
 {
const char *commit_string = "WORKTREE";
-   if (commit_sha1)
-   commit_string = sha1_to_hex(commit_sha1);
+   if (treeish_name)
+   commit_string = sha1_to_hex(treeish_name);
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 *treeish_name;
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->treeish_name, 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->treeish_name, 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->treeish_name, 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);
  

[PATCHv4 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 8285bcc605..2f2eda377f 100644
--- a/Documentation/technical/api-submodule-config.txt
+++ b/Documentation/technical/api-submodule-config.txt
@@ -56,8 +56,11 @@ Functions
 
The same as above but lookup by name.
 
-If given the null_sha1 as treeish_name 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 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.4.g3396b6f.dirty

[PATCHv4 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.4.g3396b6f.dirty

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

[Stefan Beller]

replacing sb/submodule-config-cleanup

v4:
* renamed commit_or_tree to treeish_name
* added a test with a tag
* "it will overwrite" (removed the spurious "be").

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 treeish_name
  submodule-config: clarify parsing of null_sha1 element

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

-- 
2.11.0.rc2.4.g3396b6f.dirty

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

[Stefan Beller]

On Mon, Nov 21, 2016 at 7:37 PM, Junio C Hamano  wrote:

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

fixed in a resend.

> 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.

I'll take this.

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

ok, so treeish_name it is.

>> +'
>
> Perhaps also test a tag that points at the commit?

will fix in a resend.

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

[Jeff King]

On Tue, Nov 22, 2016 at 11:10:25AM -0800, Junio C Hamano wrote:

> Archive & Upload-archive:
> 
> "cd Documentation && git archive --remote=origin" immediately hits
> "BUG: setup_git_env called without repository" if your Git is built
> with b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
> 2016-10-20), which will not be part of the upcoming release.  And
> 'origin' will probably not be understood from the local config.

Yeah, I think "cd Documentation && git archive --remote" has always been
broken. It doesn't barf until b1ef400eec, but yes, v2.11-rc2 breaks
finding "origin" in the local config.

I agree that doing the "gently" setup will fix the config problem, but
there's more to do to make it work with b1ef400eec outside of a repo.
It looks like read_remotes_file() and read_branches_file() in remote.c
need to learn to silently return when we are not in a git repository (or
alternatively, remote_get_1() should learn not to call them in such a
case).

I doubt this is a critical case (it only kicks in if you are outside a
repo _and_ your name looks like a remote name and not a URL, _and_ it is
not defined by your config, and the result is that we die("BUG") rather
than reporting "you don't have such a remote"). But any time it is
possible to hit a die("BUG"), we should be fixing it.

> I think we can do the "gently" thing there, as we may be retrieving
> a remote archive outside a local repository.  We'd need to tweak
> "output" with prefix to compensate for the case in which the command
> is run from a subdirectory, and probably we need to futz with the
> setup_prefix parameter to write_archive(), as a local caller now
> will know if we are in nongit situation.

Makes sense. I think OPT_FILENAME() would handle "output", but yeah, it
looks like write_archive() already tries to be clever about the setup.

> On the upload-archive side that serves "--remote" request, there is
> enter_repo() so we should be covered OK.

Right, that has to run in a repo, and enter_repo() should handle it.

> Mailinfo:
> 
> We may want a "gently" thing there to pick up local configuration.
> i18n.commitencoding and mailinfo.scissors in local repository would
> be ignored otherwise.

Yeah, agreed.

> Verify-pack:
> 
> This calls git_config() but these days farms out its operation to
> "index-pack", so we should be OK.  We may even want to lose the call
> to git_config() which does not affect anything.

I'd be slightly worried that there is a core.* config option that
affects us in a subtle way (e.g., some portability flag for how we do
run_command()). I don't think there is one now, but it seems like a
potential maintenance pitfall. IOW, I think I'd generally prefer to err
on the side of having everything do a gentle setup and load at least the
default config, to avoid surprises.

I don't think it's a high priority, though.

So what do you want to do for v2.11? I think there are minor regressions
in stripspace, archive, and mailinfo with respect to reading
.git/config. The right solution in each case is to do a gentle repo
setup. We have patches for stripspace already, but not yet for the other
two. They _should_ be fairly trivial, but we are getting awfully close
to the release. Do you want to do another round of -rc3? Ship with the
minor regressions and fix them up in v2.11.1? Last-minute revert the
original config topic that introduced the regressions (I'm biased
against that as the author, but I also think it's dangerous; it's a big
enough topic that it might introduce new problems)?

-Peff

[PATCH] SQUASH

[Stefan Beller]

Signed-off-by: Stefan Beller 
---

On Tue, Nov 22, 2016 at 11:22 AM, Stefan Beller  wrote:
> 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.
>

I spoke too early, this needs to be squashed. :(

Stefan

 builtin/submodule--helper.c | 3 +++
 git-submodule.sh| 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e94dd68a0e..75cdbf45b8 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1083,6 +1083,9 @@ static int embed_git_dir(int argc, const char **argv, 
const char *prefix)
struct module_list list = MODULE_LIST_INIT;
 
struct option embed_gitdir_options[] = {
+   OPT_STRING(0, "prefix", ,
+  N_("path"),
+  N_("path into the working tree")),
OPT_END()
};
 
diff --git a/git-submodule.sh b/git-submodule.sh
index 2178248287..b7e124f340 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1133,7 +1133,7 @@ cmd_sync()
 
 cmd_embedgitdirs()
 {
-   git submodule--helper --prefix "$wt_prefix" embed-git-dirs "$@"
+   git submodule--helper embed-git-dirs --prefix "$wt_prefix" "$@"
 }
 
 # This loop parses the command line arguments to find the
-- 
2.11.0.rc2.4.g3396b6f.dirty

Re: Fwd: git diff with “--word-diff-regex” extremely slow compared to “--word-diff”?

[Jeff King]

On Tue, Nov 22, 2016 at 10:08:33AM -0800, Matthieu S wrote:

> You are right, I should have compared with the same regex, and indeed,
> --word-diff-regex=[^[:space:]] is also much slower than just
> --word-diff, although they do the same job. Maybe this is a hint that
> the --word-diff-regex code could be made faster?

Maybe. If most of the time is spent in the regex engine, there may not
be much we can do. But perhaps there is something in the surrounding
code that can be improved. Looking at find_word_boundaries() (and this
is the first time I've done so), it does look like we regex-match the
whole buffer, and only then find the end-of-line. Now that we have
regexec_buf(), it might be possible to constrain the regex buffer more.

> I have a small understanding of git, but is git diff computing the
> diff value for the whole file, and then showing in the terminal the 10
> first values? In some cases, it seems to be a lot of unnecessary
> computation! Is there any possibility to ask git-diff to only compare
> say the first 100 lines? Or compute only when necessary, i.e.
> when"enter" is prompted in the console?

Git always computes the diff for the whole file. The paging is done by
an external program. So no, there's no easy way to do it incrementally
as the user interacts with the pager, as the pager does not communicate
back to git in any way. However, git should generally be streaming out
results (and the pager showing them) as they're computed, so in an ideal
world you get output immediately, and then the pager buffers the rest of
it while you're reading the first page.

Git does have to look at the whole file in order to do the initial
line-by-line diff, so it would be hard to make that incremental. It
could do the word-coloring for each hunk incrementally, though. I would
have assumed that is already how it is done, though I didn't dig into
it.

-Peff

[PATCHv2 4/4] submodule: add embed-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   | 14 +++
 builtin/submodule--helper.c   | 33 -
 git-submodule.sh  |  7 +++-
 submodule.c   | 65 
 submodule.h   |  2 +
 t/t7412-submodule-embedgitdirs.sh | 78 +++
 6 files changed, 197 insertions(+), 2 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..34791cfc65 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,6 +22,7 @@ SYNOPSIS
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
 'git submodule' [--quiet] sync [--recursive] [--] [...]
+'git submodule' [--quiet] embedgitdirs [--] [...]
 
 
 DESCRIPTION
@@ -245,6 +246,19 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
+embedgitdirs::
+   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
+embedded into the superprojects git directory.
++
+This command is recursive by default.
+
 OPTIONS
 ---
 -q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 806e29ce4e..e94dd68a0e 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 embed_git_dir(int argc, const char **argv, const char *prefix)
+{
+   int i;
+   struct pathspec pathspec;
+   struct module_list list = MODULE_LIST_INIT;
+
+   struct option embed_gitdir_options[] = {
+   OPT_END()
+   };
+
+   const char *const git_submodule_helper_usage[] = {
+   N_("git submodule--helper embed-git-dir [...]"),
+   NULL
+   };
+
+   argc = parse_options(argc, argv, prefix, embed_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(prefix, list.entries[i]->name, 1);
+
+   return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1093,7 +1123,8 @@ static struct cmd_struct commands[] = {
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
{"init", module_init, 0},
-   {"remote-branch", resolve_remote_submodule_branch, 0}
+   {"remote-branch", resolve_remote_submodule_branch, 0},
+   {"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..2178248287 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1131,6 +1131,11 @@ cmd_sync()
done
 }
 
+cmd_embedgitdirs()
+{
+   git submodule--helper --prefix "$wt_prefix" embed-git-dirs "$@"
+}
+
 # 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 | 
embedgitdirs)
command=$1
;;
-q|--quiet)
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..d330b567a3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1263,3 +1263,68 @@ 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 *prefix, const char *path,
+ 

[PATCHv2 3/4] 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.4.g3396b6f.dirty

[PATCHv2 2/4] submodule helper: support super prefix

[Stefan Beller]

Just like main commands in Git, the submodule helper needs
access to the superproject prefix. Enable this in the git.c
but have its own fuse in the helper code by having a flag to
turn on the super prefix.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 31 ---
 git.c   |  2 +-
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..806e29ce4e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc, 
const char **argv,
return 0;
 }
 
+#define SUPPORT_SUPER_PREFIX (1<<0)
+
 struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
+   int option;
 };
 
 static struct cmd_struct commands[] = {
-   {"list", module_list},
-   {"name", module_name},
-   {"clone", module_clone},
-   {"update-clone", update_clone},
-   {"relative-path", resolve_relative_path},
-   {"resolve-relative-url", resolve_relative_url},
-   {"resolve-relative-url-test", resolve_relative_url_test},
-   {"init", module_init},
-   {"remote-branch", resolve_remote_submodule_branch}
+   {"list", module_list, 0},
+   {"name", module_name, 0},
+   {"clone", module_clone, 0},
+   {"update-clone", update_clone, 0},
+   {"relative-path", resolve_relative_path, 0},
+   {"resolve-relative-url", resolve_relative_url, 0},
+   {"resolve-relative-url-test", resolve_relative_url_test, 0},
+   {"init", module_init, 0},
+   {"remote-branch", resolve_remote_submodule_branch, 0}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
@@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv, 
const char *prefix)
die(_("submodule--helper subcommand must be "
  "called with a subcommand"));
 
-   for (i = 0; i < ARRAY_SIZE(commands); i++)
-   if (!strcmp(argv[1], commands[i].cmd))
+   for (i = 0; i < ARRAY_SIZE(commands); i++) {
+   if (!strcmp(argv[1], commands[i].cmd)) {
+   if (get_super_prefix() &&
+   !(commands[i].option & SUPPORT_SUPER_PREFIX))
+   die("%s doesn't support --super-prefix",
+   commands[i].cmd);
return commands[i].fn(argc - 1, argv + 1, prefix);
+   }
+   }
 
die(_("'%s' is not a valid submodule--helper "
  "subcommand"), argv[1]);
diff --git a/git.c b/git.c
index efa1059fe0..98dcf6c518 100644
--- a/git.c
+++ b/git.c
@@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
{ "stripspace", cmd_stripspace },
-   { "submodule--helper", cmd_submodule__helper, RUN_SETUP },
+   { "submodule--helper", cmd_submodule__helper, RUN_SETUP | 
SUPPORT_SUPER_PREFIX},
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
{ "tag", cmd_tag, RUN_SETUP },
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
-- 
2.11.0.rc2.4.g3396b6f.dirty

[PATCHv2 1/4] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

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 much harder
to misuse.

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.4.g3396b6f.dirty

[PATCHv2 0/4] `submodule embedgitdirs` [was: Introduce `submodule interngitdirs`]

[Stefan Beller]

v2:
* fixed commit message for patch:
 "submodule: use absolute path for computing relative path connecting"
* a new patch "submodule helper: support super prefix"
* redid the final patch with more tests and fixing bugs along the way
* "test-lib-functions.sh: teach test_commit -C " unchanged

diff to v1 below.

Stefan Beller (4):
  submodule: use absolute path for computing relative path connecting
  submodule helper: support super prefix
  test-lib-functions.sh: teach test_commit -C 
  submodule: add embed-git-dir function

 Documentation/git-submodule.txt   | 14 +++
 builtin/submodule--helper.c   | 62 +--
 git-submodule.sh  |  7 +++-
 git.c |  2 +-
 submodule.c   | 77 +++---
 submodule.h   |  2 +
 t/t7412-submodule-embedgitdirs.sh | 78 +++
 t/test-lib-functions.sh   | 20 +++---
 8 files changed, 239 insertions(+), 23 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

v1:
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

diff to v1:
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 80d55350eb..34791cfc65 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,7 +22,8 @@ SYNOPSIS
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
 'git submodule' [--quiet] sync [--recursive] [--] [...]
-'git submodule' [--quiet] interngitdirs [--] [...]
+'git submodule' [--quiet] embedgitdirs [--] [...]
+
 
 DESCRIPTION
 ---
@@ -245,18 +246,18 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
-interngitdirs::
+embedgitdirs::
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
+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
+embedded into the superprojects git directory.
 +
-   This command is recursive by default.
+This command is recursive by default.
 
 OPTIONS
 ---
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 256f8e9439..75cdbf45b8 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,22 +1076,25 @@ 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)
+static int embed_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[] = {
+   struct option embed_gitdir_options[] = {
+   OPT_STRING(0, "prefix", ,
+  N_("path"),
+  N_("path into the working tree")),
OPT_END()
};
 
const char *const git_submodule_helper_usage[] = {
-   N_("git submodule--helper intern-git-dir [...]"),
+   N_("git submodule--helper embed-git-dir [...]"),
NULL
};
 
-   argc = parse_options(argc, argv, prefix, intern_gitdir_options,
+   argc = parse_options(argc, argv, prefix, embed_gitdir_options,
 git_submodule_helper_usage, 0);
 
gitmodules_config();
@@ -1101,27 +1104,30 @@ static int intern_git_dir(int argc, const char **argv, 
const char *prefix)
return 1;
 
for (i = 0; i < list.nr; i++)
-   migrate_submodule_gitdir(list.entries[i]->name);
+   migrate_submodule_gitdir(prefix, list.entries[i]->name, 1);
 
return 0;
 }
 
+#define SUPPORT_SUPER_PREFIX (1<<0)
+
 struct cmd_struct {
const char 

Re: [PATCH] git-gui: pass the branch name to git merge

[Junio C Hamano]

Johannes Sixt  writes:

> The recent rewrite of the 'git merge' invocation in b5f325cb
> (git-gui: stop using deprecated merge syntax) replaced the
> subsidiary call of 'git fmt-merge-msg' to take advantage of
> the capability of 'git merge' that can construct a merge log
> message when the rev being merged is FETCH_HEAD.
>
> A disadvantage of this method is, though, that the conflict
> markers are augmented with a raw SHA1 instead of a symbolic
> branch name.

Can't this be handled on the "git merge FETCH_HEAD" codepath
instead?  The codepath already does enough "magic" things to
FETCH_HEAD like skipping 'not-for-merge' entries and also knows
about the original refnames the commits to be merged came from.

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

[Junio C Hamano]

Junio C Hamano  writes:

> I think we want to audit the ones without RUN_SETUP* in the command
> table in order to hunt for regression aka "a fix revealed a bug that
> was covered by .git/config accidentally getting read when run from
> the top-level of the working tree", though. We may find unreported
> breakages that we may have to fix.

So I took a brief look at the "PROGRAM_OBJS += ..." in the Makefile
for non-builtin commands, and commands[] array in git.c for builtin
commands to see how bad it looks.


Archive & Upload-archive:

"cd Documentation && git archive --remote=origin" immediately hits
"BUG: setup_git_env called without repository" if your Git is built
with b1ef400eec ("setup_git_env: avoid blind fall-back to ".git"",
2016-10-20), which will not be part of the upcoming release.  And
'origin' will probably not be understood from the local config.

I think we can do the "gently" thing there, as we may be retrieving
a remote archive outside a local repository.  We'd need to tweak
"output" with prefix to compensate for the case in which the command
is run from a subdirectory, and probably we need to futz with the
setup_prefix parameter to write_archive(), as a local caller now
will know if we are in nongit situation.

On the upload-archive side that serves "--remote" request, there is
enter_repo() so we should be covered OK.


Mailinfo:

We may want a "gently" thing there to pick up local configuration.
i18n.commitencoding and mailinfo.scissors in local repository would
be ignored otherwise.


Splitspace:

Dscho fixed this one.


Verify-pack:

This calls git_config() but these days farms out its operation to
"index-pack", so we should be OK.  We may even want to lose the call
to git_config() which does not affect anything.

[PATCH v5 4/6] grep: optionally recurse into submodules

[Brandon Williams]

Allow grep to recognize submodules and recursively search for patterns in
each submodule.  This is done by forking off a process to recursively
call grep on each submodule.  The top level --super-prefix option is
used to pass a path to the submodule which can in turn be used to
prepend to output or in pathspec matching logic.

Recursion only occurs for submodules which have been initialized and
checked out by the parent project.  If a submodule hasn't been
initialized and checked out it is simply skipped.

In order to support the existing multi-threading infrastructure in grep,
output from each child process is captured in a strbuf so that it can be
later printed to the console in an ordered fashion.

To limit the number of theads that are created, each child process has
half the number of threads as its parents (minimum of 1), otherwise we
potentailly have a fork-bomb.

Signed-off-by: Brandon Williams 
---
 Documentation/git-grep.txt |   5 +
 builtin/grep.c | 300 ++---
 git.c  |   2 +-
 t/t7814-grep-recurse-submodules.sh |  99 
 4 files changed, 386 insertions(+), 20 deletions(-)
 create mode 100755 t/t7814-grep-recurse-submodules.sh

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 0ecea6e..17aa1ba 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -26,6 +26,7 @@ SYNOPSIS
   [--threads ]
   [-f ] [-e] 
   [--and|--or|--not|(|)|-e ...]
+  [--recurse-submodules]
   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | 
...]
   [--] [...]
 
@@ -88,6 +89,10 @@ OPTIONS
mechanism.  Only useful when searching files in the current directory
with `--no-index`.
 
+--recurse-submodules::
+   Recursively search in each submodule that has been initialized and
+   checked out in the repository.
+
 -a::
 --text::
Process binary files as if they were text.
diff --git a/builtin/grep.c b/builtin/grep.c
index 8887b6a..dca0be6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -18,12 +18,20 @@
 #include "quote.h"
 #include "dir.h"
 #include "pathspec.h"
+#include "submodule.h"
 
 static char const * const grep_usage[] = {
N_("git grep [] [-e]  [...] [[--] ...]"),
NULL
 };
 
+static const char *super_prefix;
+static int recurse_submodules;
+static struct argv_array submodule_options = ARGV_ARRAY_INIT;
+
+static int grep_submodule_launch(struct grep_opt *opt,
+const struct grep_source *gs);
+
 #define GREP_NUM_THREADS_DEFAULT 8
 static int num_threads;
 
@@ -174,7 +182,10 @@ static void *run(void *arg)
break;
 
opt->output_priv = w;
-   hit |= grep_source(opt, >source);
+   if (w->source.type == GREP_SOURCE_SUBMODULE)
+   hit |= grep_submodule_launch(opt, >source);
+   else
+   hit |= grep_source(opt, >source);
grep_source_clear_data(>source);
work_done(w);
}
@@ -300,6 +311,10 @@ static int grep_sha1(struct grep_opt *opt, const unsigned 
char *sha1,
if (opt->relative && opt->prefix_length) {
quote_path_relative(filename + tree_name_len, opt->prefix, 
);
strbuf_insert(, 0, filename, tree_name_len);
+   } else if (super_prefix) {
+   strbuf_add(, filename, tree_name_len);
+   strbuf_addstr(, super_prefix);
+   strbuf_addstr(, filename + tree_name_len);
} else {
strbuf_addstr(, filename);
}
@@ -328,10 +343,13 @@ static int grep_file(struct grep_opt *opt, const char 
*filename)
 {
struct strbuf buf = STRBUF_INIT;
 
-   if (opt->relative && opt->prefix_length)
+   if (opt->relative && opt->prefix_length) {
quote_path_relative(filename, opt->prefix, );
-   else
+   } else {
+   if (super_prefix)
+   strbuf_addstr(, super_prefix);
strbuf_addstr(, filename);
+   }
 
 #ifndef NO_PTHREADS
if (num_threads) {
@@ -378,31 +396,260 @@ static void run_pager(struct grep_opt *opt, const char 
*prefix)
exit(status);
 }
 
-static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, 
int cached)
+static void compile_submodule_options(const struct grep_opt *opt,
+ const struct pathspec *pathspec,
+ int cached, int untracked,
+ int opt_exclude, int use_index,
+ int pattern_type_arg)
+{
+   struct grep_pat *pattern;
+   int i;
+
+   if (recurse_submodules)
+   argv_array_push(_options, "--recurse-submodules");
+
+   if (cached)
+   argv_array_push(_options, 

[PATCH v5 6/6] grep: search history of moved submodules

[Brandon Williams]

If a submodule was renamed at any point since it's inception then if you
were to try and grep on a commit prior to the submodule being moved, you
wouldn't be able to find a working directory for the submodule since the
path in the past is different from the current path.

This patch teaches grep to find the .git directory for a submodule in
the parents .git/modules/ directory in the event the path to the
submodule in the commit that is being searched differs from the state of
the currently checked out commit.  If found, the child process that is
spawned to grep the submodule will chdir into its gitdir instead of a
working directory.

In order to override the explicit setting of submodule child process's
gitdir environment variable (which was introduced in '10f5c526')
`GIT_DIR_ENVIORMENT` needs to be pushed onto child process's env_array.
This allows the searching of history from a submodule's gitdir, rather
than from a working directory.

Signed-off-by: Brandon Williams 
---
 builtin/grep.c | 20 +--
 t/t7814-grep-recurse-submodules.sh | 41 ++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 5918a26..2c727ef 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -547,6 +547,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
name = gs->name;
 
prepare_submodule_repo_env(_array);
+   argv_array_push(_array, GIT_DIR_ENVIRONMENT);
 
/* Add super prefix */
argv_array_pushf(, "--super-prefix=%s%s/",
@@ -615,8 +616,23 @@ static int grep_submodule(struct grep_opt *opt, const 
unsigned char *sha1,
 {
if (!is_submodule_initialized(path))
return 0;
-   if (!is_submodule_populated(path))
-   return 0;
+   if (!is_submodule_populated(path)) {
+   /*
+* If searching history, check for the presense of the
+* submodule's gitdir before skipping the submodule.
+*/
+   if (sha1) {
+   const struct submodule *sub =
+   submodule_from_path(null_sha1, path);
+   if (sub)
+   path = git_path("modules/%s", sub->name);
+
+   if (!(is_directory(path) && is_git_directory(path)))
+   return 0;
+   } else {
+   return 0;
+   }
+   }
 
 #ifndef NO_PTHREADS
if (num_threads) {
diff --git a/t/t7814-grep-recurse-submodules.sh 
b/t/t7814-grep-recurse-submodules.sh
index 9e93fe7..0507771 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -186,6 +186,47 @@ test_expect_success 'grep recurse submodule colon in name' 
'
test_cmp expect actual
 '
 
+test_expect_success 'grep history with moved submoules' '
+   git init parent &&
+   test_when_finished "rm -rf parent" &&
+   echo "foobar" >parent/file &&
+   git -C parent add file &&
+   git -C parent commit -m "add file" &&
+
+   git init sub &&
+   test_when_finished "rm -rf sub" &&
+   echo "foobar" >sub/file &&
+   git -C sub add file &&
+   git -C sub commit -m "add file" &&
+
+   git -C parent submodule add ../sub dir/sub &&
+   git -C parent commit -m "add submodule" &&
+
+   cat >expect <<-\EOF &&
+   dir/sub/file:foobar
+   file:foobar
+   EOF
+   git -C parent grep -e "foobar" --recurse-submodules >actual &&
+   test_cmp expect actual &&
+
+   git -C parent mv dir/sub sub-moved &&
+   git -C parent commit -m "moved submodule" &&
+
+   cat >expect <<-\EOF &&
+   file:foobar
+   sub-moved/file:foobar
+   EOF
+   git -C parent grep -e "foobar" --recurse-submodules >actual &&
+   test_cmp expect actual &&
+
+   cat >expect <<-\EOF &&
+   HEAD^:dir/sub/file:foobar
+   HEAD^:file:foobar
+   EOF
+   git -C parent grep -e "foobar" --recurse-submodules HEAD^ >actual &&
+   test_cmp expect actual
+'
+
 test_incompatible_with_recurse_submodules ()
 {
test_expect_success "--recurse-submodules and $1 are incompatible" "
-- 
2.8.0.rc3.226.g39d4020

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

[Brandon Williams]

Teach grep to recursively search in submodules when provided with a
 object. This allows grep to search a submodule based on the state
of the submodule that is present in a commit of the super project.

When grep is provided with a  object, the name of the object is
prefixed to all output.  In order to provide uniformity of output
between the parent and child processes the option `--parent-basename`
has been added so that the child can preface all of it's output with the
name of the parent's object instead of the name of the commit SHA1 of
the submodule. This changes output from the command
`git grep -e. -l --recurse-submodules HEAD`

from:
  HEAD:file
  :sub/file

to:
  HEAD:file
  HEAD:sub/file

Signed-off-by: Brandon Williams 
---
 Documentation/git-grep.txt |  13 -
 builtin/grep.c |  76 ---
 t/t7814-grep-recurse-submodules.sh | 103 -
 tree-walk.c|  28 ++
 4 files changed, 211 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 17aa1ba..71f32f3 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -26,7 +26,7 @@ SYNOPSIS
   [--threads ]
   [-f ] [-e] 
   [--and|--or|--not|(|)|-e ...]
-  [--recurse-submodules]
+  [--recurse-submodules] [--parent-basename ]
   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | 
...]
   [--] [...]
 
@@ -91,7 +91,16 @@ OPTIONS
 
 --recurse-submodules::
Recursively search in each submodule that has been initialized and
-   checked out in the repository.
+   checked out in the repository.  When used in combination with the
+option the prefix of all submodule output will be the name of
+   the parent project's  object.
+
+--parent-basename ::
+   For internal use only.  In order to produce uniform output with the
+   --recurse-submodules option, this option can be used to provide the
+   basename of a parent's  object to a submodule so the submodule
+   can prefix its output with the parent's name rather than the SHA1 of
+   the submodule.
 
 -a::
 --text::
diff --git a/builtin/grep.c b/builtin/grep.c
index dca0be6..5918a26 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -19,6 +19,7 @@
 #include "dir.h"
 #include "pathspec.h"
 #include "submodule.h"
+#include "submodule-config.h"
 
 static char const * const grep_usage[] = {
N_("git grep [] [-e]  [...] [[--] ...]"),
@@ -28,6 +29,7 @@ static char const * const grep_usage[] = {
 static const char *super_prefix;
 static int recurse_submodules;
 static struct argv_array submodule_options = ARGV_ARRAY_INIT;
+static const char *parent_basename;
 
 static int grep_submodule_launch(struct grep_opt *opt,
 const struct grep_source *gs);
@@ -534,19 +536,53 @@ static int grep_submodule_launch(struct grep_opt *opt,
 {
struct child_process cp = CHILD_PROCESS_INIT;
int status, i;
+   const char *end_of_base;
+   const char *name;
struct work_item *w = opt->output_priv;
 
+   end_of_base = strchr(gs->name, ':');
+   if (gs->identifier && end_of_base)
+   name = end_of_base + 1;
+   else
+   name = gs->name;
+
prepare_submodule_repo_env(_array);
 
/* Add super prefix */
argv_array_pushf(, "--super-prefix=%s%s/",
 super_prefix ? super_prefix : "",
-gs->name);
+name);
argv_array_push(, "grep");
 
+   /*
+* Add basename of parent project
+* When performing grep on a tree object the filename is prefixed
+* with the object's name: 'tree-name:filename'.  In order to
+* provide uniformity of output we want to pass the name of the
+* parent project's object name to the submodule so the submodule can
+* prefix its output with the parent's name and not its own SHA1.
+*/
+   if (gs->identifier && end_of_base)
+   argv_array_pushf(, "--parent-basename=%.*s",
+(int) (end_of_base - gs->name),
+gs->name);
+
/* Add options */
-   for (i = 0; i < submodule_options.argc; i++)
+   for (i = 0; i < submodule_options.argc; i++) {
+   /*
+* If there is a tree identifier for the submodule, add the
+* rev after adding the submodule options but before the
+* pathspecs.  To do this we listen for the '--' and insert the
+* sha1 before pushing the '--' onto the child process argv
+* array.
+*/
+   if (gs->identifier &&
+   !strcmp("--", submodule_options.argv[i])) {
+   argv_array_push(, sha1_to_hex(gs->identifier));
+  

[PATCH v5 0/6] recursively grep across submodules

[Brandon Williams]

Major change in v5 is to use tree_is_interesting api instead of the vanilla
pathspec code for submodules.  This is to fix the issue in the last seires
where I mix the two types.  More tests were also added to ensure that the
changes to the pathspec code functioned properly.

Brandon Williams (6):
  submodules: add helper functions to determine presence of submodules
  submodules: load gitmodules file from commit sha1
  grep: add submodules as a grep source type
  grep: optionally recurse into submodules
  grep: enable recurse-submodules to work on  objects
  grep: search history of moved submodules

 Documentation/git-grep.txt |  14 ++
 builtin/grep.c | 386 ++---
 cache.h|   2 +
 config.c   |   8 +-
 git.c  |   2 +-
 grep.c |  16 +-
 grep.h |   1 +
 submodule-config.c |   6 +-
 submodule-config.h |   3 +
 submodule.c|  50 +
 submodule.h|   3 +
 t/t7814-grep-recurse-submodules.sh | 241 +++
 tree-walk.c|  28 +++
 13 files changed, 729 insertions(+), 31 deletions(-)
 create mode 100755 t/t7814-grep-recurse-submodules.sh

-- interdiff based on 'bw/grep-recurse-submodules'

diff --git a/builtin/grep.c b/builtin/grep.c
index 052f605..2c727ef 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -698,6 +698,8 @@ static int grep_cache(struct grep_opt *opt, const struct 
pathspec *pathspec,
} else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
   submodule_path_match(pathspec, name.buf, NULL)) {
hit |= grep_submodule(opt, NULL, ce->name, ce->name);
+   } else {
+   continue;
}
 
if (ce_stage(ce)) {
@@ -734,17 +736,10 @@ static int grep_tree(struct grep_opt *opt, const struct 
pathspec *pathspec,
int te_len = tree_entry_len();
 
if (match != all_entries_interesting) {
-   strbuf_setlen(, name_base_len);
strbuf_addstr(, base->buf + tn_len);
-
-   if (recurse_submodules && S_ISGITLINK(entry.mode)) {
-   strbuf_addstr(, entry.path);
-   match = submodule_path_match(pathspec, name.buf,
-NULL);
-   } else {
-   match = tree_entry_interesting(, ,
-  0, pathspec);
-   }
+   match = tree_entry_interesting(, ,
+  0, pathspec);
+   strbuf_setlen(, name_base_len);
 
if (match == all_entries_not_interesting)
break;
diff --git a/t/t7814-grep-recurse-submodules.sh 
b/t/t7814-grep-recurse-submodules.sh
index 7d66716..0507771 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -127,6 +127,34 @@ test_expect_success 'grep tree and pathspecs' '
test_cmp expect actual
 '
 
+test_expect_success 'grep tree and pathspecs' '
+   cat >expect <<-\EOF &&
+   HEAD:submodule/a:foobar
+   HEAD:submodule/sub/a:foobar
+   EOF
+
+   git grep -e "bar" --recurse-submodules HEAD -- "submodule*a" >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'grep tree and more pathspecs' '
+   cat >expect <<-\EOF &&
+   HEAD:submodule/a:foobar
+   EOF
+
+   git grep -e "bar" --recurse-submodules HEAD -- "submodul?/a" >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'grep tree and more pathspecs' '
+   cat >expect <<-\EOF &&
+   HEAD:submodule/sub/a:foobar
+   EOF
+
+   git grep -e "bar" --recurse-submodules HEAD -- "submodul*/sub/a" 
>actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'grep recurse submodule colon in name' '
git init parent &&
test_when_finished "rm -rf parent" &&
diff --git a/tree-walk.c b/tree-walk.c
index 828f435..ff77605 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -1004,6 +1004,19 @@ static enum interesting do_match(const struct name_entry 
*entry,
 */
if (ps->recursive && S_ISDIR(entry->mode))
return entry_interesting;
+
+   /*
+* When matching against submodules with
+* wildcard characters, ensure that the entry
+* at least matches up to the first wild
+* character.  More accurate matching 

[PATCH v5 2/6] submodules: load gitmodules file from commit sha1

[Brandon Williams]

teach submodules to load a '.gitmodules' file from a commit sha1.  This
enables the population of the submodule_cache to be based on the state
of the '.gitmodules' file from a particular commit.

Signed-off-by: Brandon Williams 
---
 cache.h|  2 ++
 config.c   |  8 
 submodule-config.c |  6 +++---
 submodule-config.h |  3 +++
 submodule.c| 12 
 submodule.h|  1 +
 6 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 1be6526..559a461 100644
--- a/cache.h
+++ b/cache.h
@@ -1690,6 +1690,8 @@ extern int git_default_config(const char *, const char *, 
void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
 extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,
const char *name, const char *buf, 
size_t len, void *data);
+extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,
+const unsigned char *sha1, void *data);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern void git_config(config_fn_t fn, void *);
diff --git a/config.c b/config.c
index 83fdecb..4d78e72 100644
--- a/config.c
+++ b/config.c
@@ -1214,10 +1214,10 @@ int git_config_from_mem(config_fn_t fn, const enum 
config_origin_type origin_typ
return do_config_from(, fn, data);
 }
 
-static int git_config_from_blob_sha1(config_fn_t fn,
-const char *name,
-const unsigned char *sha1,
-void *data)
+int git_config_from_blob_sha1(config_fn_t fn,
+ const char *name,
+ const unsigned char *sha1,
+ void *data)
 {
enum object_type type;
char *buf;
diff --git a/submodule-config.c b/submodule-config.c
index 098085b..8b9a2ef 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -379,9 +379,9 @@ static int parse_config(const char *var, const char *value, 
void *data)
return ret;
 }
 
-static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
- unsigned char *gitmodules_sha1,
- struct strbuf *rev)
+int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
+  unsigned char *gitmodules_sha1,
+  struct strbuf *rev)
 {
int ret = 0;
 
diff --git a/submodule-config.h b/submodule-config.h
index d05c542..78584ba 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -29,6 +29,9 @@ const struct submodule *submodule_from_name(const unsigned 
char *commit_sha1,
const char *name);
 const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path);
+extern int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
+ unsigned char *gitmodules_sha1,
+ struct strbuf *rev);
 void submodule_free(void);
 
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index f5107f0..062e58b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -198,6 +198,18 @@ void gitmodules_config(void)
}
 }
 
+void gitmodules_config_sha1(const unsigned char *commit_sha1)
+{
+   struct strbuf rev = STRBUF_INIT;
+   unsigned char sha1[20];
+
+   if (gitmodule_sha1_from_commit(commit_sha1, sha1, )) {
+   git_config_from_blob_sha1(submodule_config, rev.buf,
+ sha1, NULL);
+   }
+   strbuf_release();
+}
+
 /*
  * Determine if a submodule has been initialized at a given 'path'
  */
diff --git a/submodule.h b/submodule.h
index 6ec5f2f..9203d89 100644
--- a/submodule.h
+++ b/submodule.h
@@ -37,6 +37,7 @@ void set_diffopt_flags_from_submodule_config(struct 
diff_options *diffopt,
const char *path);
 int submodule_config(const char *var, const char *value, void *cb);
 void gitmodules_config(void);
+extern void gitmodules_config_sha1(const unsigned char *commit_sha1);
 extern int is_submodule_initialized(const char *path);
 extern int is_submodule_populated(const char *path);
 int parse_submodule_update_strategy(const char *value,
-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 3/6] grep: add submodules as a grep source type

[Brandon Williams]

Add `GREP_SOURCE_SUBMODULE` as a grep_source type and cases for this new
type in the various switch statements in grep.c.

When initializing a grep_source with type `GREP_SOURCE_SUBMODULE` the
identifier can either be NULL (to indicate that the working tree will be
used) or a SHA1 (the REV of the submodule to be grep'd).  If the
identifier is a SHA1 then we want to fall through to the
`GREP_SOURCE_SHA1` case to handle the copying of the SHA1.

Signed-off-by: Brandon Williams 
---
 grep.c | 16 +++-
 grep.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index 1194d35..0dbdc1d 100644
--- a/grep.c
+++ b/grep.c
@@ -1735,12 +1735,23 @@ void grep_source_init(struct grep_source *gs, enum 
grep_source_type type,
case GREP_SOURCE_FILE:
gs->identifier = xstrdup(identifier);
break;
+   case GREP_SOURCE_SUBMODULE:
+   if (!identifier) {
+   gs->identifier = NULL;
+   break;
+   }
+   /*
+* FALL THROUGH
+* If the identifier is non-NULL (in the submodule case) it
+* will be a SHA1 that needs to be copied.
+*/
case GREP_SOURCE_SHA1:
gs->identifier = xmalloc(20);
hashcpy(gs->identifier, identifier);
break;
case GREP_SOURCE_BUF:
gs->identifier = NULL;
+   break;
}
 }
 
@@ -1760,6 +1771,7 @@ void grep_source_clear_data(struct grep_source *gs)
switch (gs->type) {
case GREP_SOURCE_FILE:
case GREP_SOURCE_SHA1:
+   case GREP_SOURCE_SUBMODULE:
free(gs->buf);
gs->buf = NULL;
gs->size = 0;
@@ -1831,8 +1843,10 @@ static int grep_source_load(struct grep_source *gs)
return grep_source_load_sha1(gs);
case GREP_SOURCE_BUF:
return gs->buf ? 0 : -1;
+   case GREP_SOURCE_SUBMODULE:
+   break;
}
-   die("BUG: invalid grep_source type");
+   die("BUG: invalid grep_source type to load");
 }
 
 void grep_source_load_driver(struct grep_source *gs)
diff --git a/grep.h b/grep.h
index 5856a23..267534c 100644
--- a/grep.h
+++ b/grep.h
@@ -161,6 +161,7 @@ struct grep_source {
GREP_SOURCE_SHA1,
GREP_SOURCE_FILE,
GREP_SOURCE_BUF,
+   GREP_SOURCE_SUBMODULE,
} type;
void *identifier;
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 1/6] submodules: add helper functions to determine presence of submodules

[Brandon Williams]

Add two helper functions to submodules.c.
`is_submodule_initialized()` checks if a submodule has been initialized
at a given path and `is_submodule_populated()` check if a submodule
has been checked out at a given path.

Signed-off-by: Brandon Williams 
---
 submodule.c | 38 ++
 submodule.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/submodule.c b/submodule.c
index 6f7d883..f5107f0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -198,6 +198,44 @@ void gitmodules_config(void)
}
 }
 
+/*
+ * Determine if a submodule has been initialized at a given 'path'
+ */
+int is_submodule_initialized(const char *path)
+{
+   int ret = 0;
+   const struct submodule *module = NULL;
+
+   module = submodule_from_path(null_sha1, path);
+
+   if (module) {
+   char *key = xstrfmt("submodule.%s.url", module->name);
+   char *value = NULL;
+
+   ret = !git_config_get_string(key, );
+
+   free(value);
+   free(key);
+   }
+
+   return ret;
+}
+
+/*
+ * Determine if a submodule has been populated at a given 'path'
+ */
+int is_submodule_populated(const char *path)
+{
+   int ret = 0;
+   char *gitdir = xstrfmt("%s/.git", path);
+
+   if (resolve_gitdir(gitdir))
+   ret = 1;
+
+   free(gitdir);
+   return ret;
+}
+
 int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst)
 {
diff --git a/submodule.h b/submodule.h
index d9e197a..6ec5f2f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -37,6 +37,8 @@ void set_diffopt_flags_from_submodule_config(struct 
diff_options *diffopt,
const char *path);
 int submodule_config(const char *var, const char *value, void *cb);
 void gitmodules_config(void);
+extern int is_submodule_initialized(const char *path);
+extern int is_submodule_populated(const char *path);
 int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
 const char *submodule_strategy_to_string(const struct 
submodule_update_strategy *s);
-- 
2.8.0.rc3.226.g39d4020

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

[Brandon Williams]

On 11/21, Junio C Hamano wrote:
> 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.

I brought this up in v2, he must have just missed it for v3.

-- 
Brandon Williams

Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms

[Karthik Nayak]

On Sun, Nov 20, 2016 at 11:02 PM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> We could have lstrip and rstrip as you suggested and perhaps make it work
>> together too. But I see this going off the scope of this series. Maybe
>> I'll follow up
>> with another series introducing these features. Since we can currently
>> make do with
>> 'strip=2' I'll drop this patch from v8 of this series and pursue this
>> idea after this.
>
> My primary point was that if we know we want to add "rstrip" later
> and still decide not to add it right now, it is OK, but we will
> regret it if we named the one we are going to add right now "strip".
> That will mean that future users, when "rstrip" is introduced, will
> end up having to choose between "strip" and "rstrip" (as opposed to
> "lstrip" and "rstrip"), wondering why left-variant is more important
> and named without left/right prefix.
>

That's a good point actually, I'll try and implement both and re-roll.

-- 
Regards,
Karthik Nayak

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

[Karthik Nayak]

On Mon, Nov 21, 2016 at 2:11 PM, Matthieu Moy
 wrote:
> 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: */
>

Will update the commit message and add the comment. Thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v7 16/17] branch: use ref-filter printing APIs

[Karthik Nayak]

On Fri, Nov 18, 2016 at 3:35 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> One worry that I have is if the strings embedded in this function to
>> the final format are safe.  As far as I can tell, the pieces of
>> strings that are literally inserted into the resulting format string
>> by this function are maxwidth, remote_prefix, and return values from
>> branch_get_color() calls.
>>
>> The maxwidth is inserted via "%d" and made into decimal constant,
>> and there is no risk for it being in the resulting format.  Are
>> the return values of branch_get_color() calls safe?  I do not think
>> they can have '%' in them, but if they do, they need to be quoted.
>> The same worry exists for remote_prefix.  Currently it can either be
>> an empty string or "remotes/", and is safe to be embedded in a
>> format string.
>
> In case it was not clear, in short, I do not think there is anything
> broken in the code, but it is a longer-term improvement to introduce
> a helper that takes a string and returns a version of the string
> that is safely quoted to be used in the for-each-ref format string
> use it like so:
>
> strbuf_addf(,
> "%s"
> "%%(align:%d,left)%s%%(refname:strip=2)%%(end)"
> ...
> "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> 
> quote_literal_for_format(branch_get_color(BRANCH_COLOR_REMOTE)),
> ...);
>
> and the implementation of the helper may look like:
>
> const char *quote_literal_for_format(const char *s)
> {
> static strbuf buf = STRBUF_INIT;
>
> strbuf_reset();
> while (*s) {
> const char *ep = strchrnul(s, '%');
> if (s < ep)
> strbuf_add(, s, ep - s);
> if (*ep == '%') {
> strbuf_addstr(, "%%");
> s = ep + 1;
> } else {
> s = ep;
> }
> }
> return buf.buf;
> }
>

Perfect. I get what you're saying, I'll add this in :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules

[Brandon Williams]

On 11/22, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > On 11/17, Stefan Beller wrote:
> >> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams  
> >> wrote:
> >> 
> >> > sha1_array_clear();
> >> > -   die("Failed to push all needed 
> >> > submodules!");
> >> > +   die ("Failed to push all needed 
> >> > submodules!");
> >> 
> >> huh? Is this a whitespace change?
> >
> > That's odd...I didn't mean to add that lone space.
> 
> Is that the only glitch in this round?  IOW, is the series OK to be
> picked up as long as I treak this out while queuing?

It looks that way.  And I did fix this in my local series.  Let me know
if you would rather I resend the series. Otherwise I think it looks
good.

I do also have a follow on series I'm planning on sending out later to
actually add in a feature which mimics what this bug does (as this
functionality could be desirable in some circumstances) but thought it
best to wait till heiko's and this series were more stable.

-- 
Brandon Williams

Re: Fwd: git diff with “--word-diff-regex” extremely slow compared to “--word-diff”?

[Matthieu S]

Thanks Jeff for the answer!

You are right, I should have compared with the same regex, and indeed,
--word-diff-regex=[^[:space:]] is also much slower than just
--word-diff, although they do the same job. Maybe this is a hint that
the --word-diff-regex code could be made faster?

I have a small understanding of git, but is git diff computing the
diff value for the whole file, and then showing in the terminal the 10
first values? In some cases, it seems to be a lot of unnecessary
computation! Is there any possibility to ask git-diff to only compare
say the first 100 lines? Or compute only when necessary, i.e.
when"enter" is prompted in the console?

Thanks!

Matthieu

2016-11-20 12:17 GMT-08:00 Jeff King :
> On Fri, Nov 18, 2016 at 03:40:22PM -0800, Matthieu S wrote:
>
>> Why is the speed so different if one uses --word-diff instead of
>> --word-diff-regex= ? Is it just because my expression is (slightly)
>> more complex than the default one (split on period instead of only
>> whitespace) ? Or is it that the default word-diff is implemented
>> differently/more efficiently? How can I overcome this speed slowdown?
>
> I think it's probably both.
>
> See diff.c:find_word_boundaries(). If there's no regex, we use a simple
> loop over isspace() to find the boundaries. I don't recall anybody
> measuring the performance before, but I'm not surprised to hear that
> matching a regex is slower.
>
> If I look at the output of "perf", though, it looks like we also spend a
> lot more time in xdl_clean_mmatch(). Which isn't surprising. Your regex
> treats commas as boundaries, which is going to generate a lot more
> matches for this particular data set (though the output is the same, I
> think, because of the nature of the change).
>
> I would have expected "--word-diff-regex=[^[:space:]]" to be faster than
> your regex, though, and it does not seem to be.
>
> -Peff

Re: v2.11 new diff heuristic?

[Jeff King]

On Tue, Nov 22, 2016 at 08:42:36AM -0600, Robert Dailey wrote:

> I dug into the git diff documentation here:
> 
> https://git-scm.com/docs/git-diff
> 
> It mentions a "--compaction-heuristic" option. Is this the new
> heuristic outlined by the release notes? If not, which is it?

No. The docs on git-scm.com are only for released versions, so that is
the "old" attempt at a similar feature from v2.9. The "new" one goes by
the name "--indent-heuristic" (and "diff.indentHeuristic" in the
config). It is not the default in v2.11, but it probably will become so
in a future version.

> Is the
> compaction heuristic compatible with the histogram diff algorithm? Is
> there a config option to turn this on all the time? For that matter,
> is this something I can keep on all the time or is it only useful in
> certain situations?
> 
> There's still so much more about this feature I would like to know.

Yes, you can keep it all the time. Yes, it's compatible with histogram
(and patience); it happens as a separate post-processing step after the
actual diff.

You can find more discussion about how it works in the commit message of
433860f3d0beb0c6f205290bd16cda413148f098.

-Peff

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

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, Nov 21, 2016 at 11:07 PM, Junio C Hamano  wrote:
>> 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.
>
> and by checking the health you mean just a status in there, or rather a
> more nuanced thing like `git rev-parse HEAD` ? I'll go with that for now.

Would fsck have caught the breakages you caused while developing it,
for example?

As the core of the "embed" operation is an non-atomic "move the .git
directory to .git/modules/$name in the superproject and then make a
gitfile pointing at it", verifying that there is no change in the
contents of .git/modules before and after the failed operation, and
verifying that the submodule working tree has the embedded .git/
directory would be good enough, I would think.

[PATCH] git-gui: pass the branch name to git merge

[Johannes Sixt]

The recent rewrite of the 'git merge' invocation in b5f325cb
(git-gui: stop using deprecated merge syntax) replaced the
subsidiary call of 'git fmt-merge-msg' to take advantage of
the capability of 'git merge' that can construct a merge log
message when the rev being merged is FETCH_HEAD.

A disadvantage of this method is, though, that the conflict
markers are augmented with a raw SHA1 instead of a symbolic
branch name. Revert to the former invocation style so that
we get both a useful commit message and a symbolic name in the
conflict markers.

Signed-off-by: Johannes Sixt 
---
 A minor regression of our effort to get rid of
 deprecated merge syntax. Even though I had done a number of
 merges since then, I noticed this only today because I
 actively looked for the branch name.

 lib/merge.tcl | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/lib/merge.tcl b/lib/merge.tcl
index 9f253db..39e3fb4 100644
--- a/lib/merge.tcl
+++ b/lib/merge.tcl
@@ -112,16 +112,9 @@ method _start {} {
close $fh
set _last_merged_branch $branch
 
-   if {[git-version >= "2.5.0"]} {
-   set cmd [list git merge --strategy=recursive FETCH_HEAD]
-   } else {
-   set cmd [list git]
-   lappend cmd merge
-   lappend cmd --strategy=recursive
-   lappend cmd [git fmt-merge-msg <[gitdir FETCH_HEAD]]
-   lappend cmd HEAD
-   lappend cmd $name
-   }
+   set cmd [list git merge --strategy=recursive --no-log -m]
+   lappend cmd [git fmt-merge-msg <[gitdir FETCH_HEAD]]
+   lappend cmd $name
 
ui_status [mc "Merging %s and %s..." $current_branch $stitle]
set cons [console::new [mc "Merge"] "merge $stitle"]
-- 
2.11.0.rc1.52.g65ffb51

Re: v2.11 new diff heuristic?

[Stefan Beller]

On Tue, Nov 22, 2016 at 6:42 AM, Robert Dailey  wrote:
> The release notes mention a new heuristic for diff:
>
> * Output from "git diff" can be made easier to read by selecting
> which lines are common and which lines are added/deleted
> intelligently when the lines before and after the changed section
> are the same. A command line option is added to help with the
> experiment to find a good heuristics.
>
> However, it lacks information on exactly how to use this new feature.
> I dug into the git diff documentation here:
>
> https://git-scm.com/docs/git-diff
>
> It mentions a "--compaction-heuristic" option. Is this the new
> heuristic outlined by the release notes?

yes.

> Is the
> compaction heuristic compatible with the histogram diff algorithm?

yes as the compaction heuristic is applied after the actual diff is performed.

> Is
> there a config option to turn this on all the time? For that matter,
> is this something I can keep on all the time or is it only useful in
> certain situations?

I think you can set diff.compactionHeuristic and it will use it by default.

>
> There's still so much more about this feature I would like to know.

The background story (and what this new compaction heuristic is doing)
is found at https://github.com/mhagger/diff-slider-tools

Re: [PATCH] merge-recursive.c: use QSORT macro

[Jeff King]

On Tue, Nov 22, 2016 at 07:30:19PM +0700, Nguyễn Thái Ngọc Duy wrote:

> This is the follow up of rs/qsort series, merged in b8688ad (Merge
> branch 'rs/qsort' - 2016-10-10), where coccinelle was used to do
> automatic transformation.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>   coccinelle missed this place, understandably, because it can't know
>   that
>   
>   sizeof(*entries->items)
>   
>   is the same as
>   
>   sizeof(*df_name_compare.items)
>   
>   without some semantic analysis.

That made me wonder why "entries" is used at all. Does it point to the
same struct? But no, df_name_compare is a string list we create with the
same list of strings.

Which is why...

> - qsort(df_sorted_entries.items, entries->nr, sizeof(*entries->items),
> + QSORT(df_sorted_entries.items, entries->nr,
> string_list_df_name_compare);

...it's OK to use entries->nr here, and not df_sorted_entries.nr. It
still seems a bit odd, though. Maybe it's worth making this:

  QSORT(df_sorted_entries.items, df_sorted_entries.nr,
string_list_df_name_compare);

while we're at it. Another possibility is:

  df_sorted_entries.cmp = string_list_df_name_compare;
  string_list_sort(_sorted_entries);

It's not any shorter, but maybe it's conceptually simpler.

-Peff

Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines

[Junio C Hamano]

Vasco Almeida  writes:

> A Sex, 11-11-2016 às 11:45 -0100, Vasco Almeida escreveu:
>> +=item comment_lines ( STRING [, STRING... ])
>> +
>> +Comments lines following core.commentchar configuration.
>> +
>> +=cut
>> +
>> +sub comment_lines {
>> +   my $comment_line_char = config("core.commentchar") || '#';
>> +   return prefix_lines("$comment_line_char ", @_);
>> +}
>> +
>
> In light of the recent "Fix problems with rebase -i when
> core.commentchar is defined" [1], I realized that this patch does not
> handle the 'auto' value of core.commentchat configuration variable.
>
> I propose to do the patch below in the next re-roll.
>
> [1] http://www.mail-archive.com/git@vger.kernel.org/msg107818.html

The incremental update below looks sensible.  We'd also want to
protect this codepath from a misconfigured two-or-more byte sequence
in core.commentchar, I would suspect, to be consistent.

> -- >8 --
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 3a6d846..8d33634 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1073,6 +1073,7 @@ sub edit_hunk_manually {
>   my $is_reverse = $patch_mode_flavour{IS_REVERSE};
>   my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', 
> '-');
>   my $comment_line_char = Git::config("core.commentchar") || '#';
> + $comment_line_char = '#' if ($comment_line_char eq 'auto');
>   print $fh Git::comment_lines sprintf(__ < $remove_plus, $comment_line_char),
>  ---
>  To remove '%s' lines, make them ' ' lines (context).
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 69cd1dd..47b5899 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1459,6 +1459,7 @@ Comments lines following core.commentchar configuration.
>  
>  sub comment_lines {
>   my $comment_line_char = config("core.commentchar") || '#';
> + $comment_line_char = '#' if ($comment_line_char eq 'auto');
>   return prefix_lines("$comment_line_char ", @_);
>  }

Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules

[Junio C Hamano]

Brandon Williams  writes:

> On 11/17, Stefan Beller wrote:
>> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams  wrote:
>> 
>> > sha1_array_clear();
>> > -   die("Failed to push all needed 
>> > submodules!");
>> > +   die ("Failed to push all needed 
>> > submodules!");
>> 
>> huh? Is this a whitespace change?
>
> That's odd...I didn't mean to add that lone space.

Is that the only glitch in this round?  IOW, is the series OK to be
picked up as long as I treak this out while queuing?

Re: [PATCH 31/35] pathspec: allow querying for attributes

[Stefan Beller]

On Tue, Nov 22, 2016 at 2:41 AM, Duy Nguyen  wrote:
> On Fri, Nov 11, 2016 at 3:34 AM, Stefan Beller  wrote:
>> @@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec 
>> *pathspec)
>>PATHSPEC_LITERAL |
>>PATHSPEC_GLOB |
>>PATHSPEC_ICASE |
>> -  PATHSPEC_EXCLUDE);
>> +  PATHSPEC_EXCLUDE |
>> +  PATHSPEC_ATTR);
>
> Hmm.. common_prefix_len() has always been a bit relaxing and can cover
> more than needed. It's for early pruning. Exact pathspec matching
> _will_ be done later anyway.
>
> Is that obvious?

Yes it is.
Not sure what your concern is, though.

Given the pathspec ":(attr:plumbing)Documentation/", the common_prefix_len
is still able to figure out that any match has a prefix of
strlen("Documentation/"),
no matter what attr stuff is involved, because the attr stuff is also
just reducing the
matching set.

Now if we have such a pathspec it is easier to claim common_prefix_len supports
attr as it is a correct thing to ignore the attrs completely, than to
rewrite the call to
common_prefix_len to be without attrs involved.

You *may* be able to improve it with knowledge of the attrs:
":(attr:internal-technical-api)Documentation/" may restrict the results to match
only "Documentation/technical/", but as you said, we don't have to be
exact here.

> I'm wondering if we need to add a line or two in the
> big comment code before this statement. I'm thinking it is and we
> probably don't need more comments...

Do I misunderstand the code completely here?

Thanks,
Stefan

> --
> Duy

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

[Junio C Hamano]

Johannes Schindelin  writes:

> On Mon, 21 Nov 2016, Junio C Hamano wrote:
>
>> 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" | (
>
> Could I ask you to sneak in a \n here?

The one before that _is_ about fixing incomplete line, but this and
the one before this one are not, so I think we should fix them at
the same time.

But does it break anything to leave it as-is?  If not, I'd prefer to
leave this (and one before this one) for a later clean-up patch post
release.

Thanks

Re: v2.11 new diff heuristic?

[Jacob Keller]

On November 22, 2016 6:42:36 AM PST, Robert Dailey  
wrote:
>The release notes mention a new heuristic for diff:
>
>* Output from "git diff" can be made easier to read by selecting
>which lines are common and which lines are added/deleted
>intelligently when the lines before and after the changed section
>are the same. A command line option is added to help with the
>experiment to find a good heuristics.
>
>However, it lacks information on exactly how to use this new feature.
>I dug into the git diff documentation here:
>
>https://git-scm.com/docs/git-diff
>
>It mentions a "--compaction-heuristic" option. Is this the new
>heuristic outlined by the release notes? If not, which is it? Is the
>compaction heuristic compatible with the histogram diff algorithm? Is
>there a config option to turn this on all the time? For that matter,
>is this something I can keep on all the time or is it only useful in
>certain situations?
>
>There's still so much more about this feature I would like to know.

Hi,

Yes for now the compaction heuristic option has an undocumented config. (I 
forget the exact name off the top of my head). Currently it is being evaluated 
and likely we want to make it default in the near future once we are certain 
that it helps and doesn't make any difference worse.

So long term you will not need any special knobs to benefit.

Thanks,
Jake

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

[Stefan Beller]

On Mon, Nov 21, 2016 at 11:07 PM, Junio C Hamano  wrote:
> 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.

and by checking the health you mean just a status in there, or rather a
more nuanced thing like `git rev-parse HEAD` ? I'll go with that for now.

>
>>>  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?

yes I did, because why would you run init after an accidental rm -rf ... ?

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> This conditional config file reading is a trap for similar bugs to
>> happen again. Is there any reason we should not just mark the command
>> RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?
>
> As I plan to slip these patches into Git for Windows v2.11.0, i.e. making
> this a last-minute hot fix, I want to err on the side of caution.

So do I.  As a hot-fix, I'd prefer the patch I queued yesterday.

I think we want to audit the ones without RUN_SETUP* in the command
table in order to hunt for regression aka "a fix revealed a bug that
was covered by .git/config accidentally getting read when run from
the top-level of the working tree", though. We may find unreported
breakages that we may have to fix.

[PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version

[Johannes Schindelin]

The popular difftool command was just converted into a builtin, for
better performance on Windows as well as to reduce the number of Perl
scripts (so that we may, in the very long run, be able to ship Git for
Windows without any Perl interpreter at all).

However, it would be sloppy practice to simply switch over from the
tried-and-tested (albeit slow) scripted version to the spanking new
builtin version that has not seen a whole lot of real-world testing.

So let's add a feature flag.

If the file `use-builtin-difftool` exists in Git's exec path, Git will
now automagically use the builtin version of the difftool, without
requiring the user to call `git builtin-difftool `. This comes in
particularly handy when the difftool command is used from within
scripts.

If the file `use-builtin-difftool` is absent from Git's exec path, which
is the default, Git will use the scripted version as before.

The original idea was to use an environment variable
GIT_USE_BUILTIN_DIFFTOOL, but the test suite resets those variables, and
we do want to use that feature flag to run the tests with, and without,
the feature flag.

Besides, the plan is to add an opt-in flag in Git for Windows'
installer. If we implemented the feature flag as an environment
variable, we would have to modify the user's environment, in order to
make the builtin difftool the default when called from Git Bash, Git CMD
or third-party tools.

Signed-off-by: Johannes Schindelin 
---
 .gitignore|  1 +
 git-difftool.perl |  7 +++
 git.c | 20 
 3 files changed, 28 insertions(+)

diff --git a/.gitignore b/.gitignore
index 4f54531..91bfd09 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,3 +1,4 @@
+/use-builtin-difftool
 /GIT-BUILD-OPTIONS
 /GIT-CFLAGS
 /GIT-LDFLAGS
diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d0..28e47d8 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -23,6 +23,13 @@ use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
+if (-e Git::exec_path() . '/use-builtin-difftool') {
+   unshift(@ARGV, "builtin-difftool");
+   unshift(@ARGV, "git");
+   exec(@ARGV);
+   die("Could not execute builtin difftool");
+}
+
 sub usage
 {
my $exitcode = shift;
diff --git a/git.c b/git.c
index eaa0f67..7a0df7a 100644
--- a/git.c
+++ b/git.c
@@ -2,6 +2,7 @@
 #include "exec_cmd.h"
 #include "help.h"
 #include "run-command.h"
+#include "dir.h"
 
 const char git_usage_string[] =
"git [--version] [--help] [-C ] [-c name=value]\n"
@@ -542,6 +543,22 @@ static void strip_extension(const char **argv)
 #define strip_extension(cmd)
 #endif
 
+static int use_builtin_difftool(void)
+{
+   static int initialized, use;
+
+   if (!initialized) {
+   struct strbuf buf = STRBUF_INIT;
+   strbuf_addf(, "%s/%s", git_exec_path(),
+   "use-builtin-difftool");
+   use = file_exists(buf.buf);
+   strbuf_release();
+   initialized = 1;
+   }
+
+   return use;
+}
+
 static void handle_builtin(int argc, const char **argv)
 {
struct argv_array args = ARGV_ARRAY_INIT;
@@ -551,6 +568,9 @@ static void handle_builtin(int argc, const char **argv)
strip_extension(argv);
cmd = argv[0];
 
+   if (!strcmp("difftool", cmd) && use_builtin_difftool())
+   cmd = "builtin-difftool";
+
/* Turn "git cmd --help" into "git help --exclude-guides cmd" */
if (argc > 1 && !strcmp(argv[1], "--help")) {
int i;
-- 
2.10.1.583.g721a9e0

[PATCH 0/2] Show Git Mailing List: a builtin difftool

[Johannes Schindelin]

I have been working on the builtin difftool for a little over a week,
for two reasons:

1. Perl is really not native on Windows. Not only is there a performance
   penalty to be paid just for running Perl scripts, we also have to deal
   with the fact that users may have different Perl installations, with
   different options, and some other Perl installation may decide to set
   PERL5LIB globally, wreaking havoc with Git for Windows' Perl (which we
   have to use because almost all other Perl distributions lack the
   Subversion bindings we need for `git svn`).

2. Perl makes for a rather large reason that Git for Windows' installer
   weighs in with >30MB. While one Perl script less does not relieve us
   of that burden, it is one step in the right direction.

This pair of patches serves two purposes: to ask for reviews, and to
show what I plan to release as part of Git for Windows v2.11.0 (which is
due this Thursday, if Git v2.11.0 is released tomorrow, as planned).

The second patch really only explains how I will make sure that the
builtin difftool will only affect users who want to opt in to testing.


Johannes Schindelin (2):
  difftool: add the builtin
  difftool: add a feature flag for the builtin vs scripted version

 .gitignore |   2 +
 Makefile   |   1 +
 builtin.h  |   1 +
 builtin/builtin-difftool.c | 680 +
 git-difftool.perl  |   7 +
 git.c  |  21 ++
 6 files changed, 712 insertions(+)
 create mode 100644 builtin/builtin-difftool.c


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

-- 
2.10.1.583.g721a9e0

[PATCH 1/2] difftool: add the builtin

[Johannes Schindelin]

This adds a builtin difftool that represents a conversion of the current
Perl script version of the difftool.

The motivation is that Perl scripts are not at all native on Windows,
and that `git difftool` therefore is pretty slow on that platform, when
there is no good reason for it to be slow.

In addition, Perl does not really have access to Git's internals. That
means that any script will always have to jump through unnecessary
hoops.

The current version of the builtin difftool does not, however, make full
use of the internals but instead chooses to spawn a couple of Git
processes, still, to make for an easier conversion. There remains a lot
of room for improvement, left for a later date.

Note: the original difftool is still called by `git difftool`. To get the
new, experimental version, call `git builtin-difftool`. The reason: this
new, experimental, builtin difftool will be shipped as part of Git for
Windows v2.11.0, to allow for easier large-scale testing.

Signed-off-by: Johannes Schindelin 
---
 .gitignore |   1 +
 Makefile   |   1 +
 builtin.h  |   1 +
 builtin/builtin-difftool.c | 680 +
 git.c  |   1 +
 5 files changed, 684 insertions(+)
 create mode 100644 builtin/builtin-difftool.c

diff --git a/.gitignore b/.gitignore
index 05cb58a..4f54531 100644
--- a/.gitignore
+++ b/.gitignore
@@ -51,6 +51,7 @@
 /git-diff-tree
 /git-difftool
 /git-difftool--helper
+/git-builtin-difftool
 /git-describe
 /git-fast-export
 /git-fast-import
diff --git a/Makefile b/Makefile
index f53fcc9..f764174 100644
--- a/Makefile
+++ b/Makefile
@@ -888,6 +888,7 @@ BUILTIN_OBJS += builtin/diff-files.o
 BUILTIN_OBJS += builtin/diff-index.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
+BUILTIN_OBJS += builtin/builtin-difftool.o
 BUILTIN_OBJS += builtin/fast-export.o
 BUILTIN_OBJS += builtin/fetch-pack.o
 BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index b9122bc..409a61e 100644
--- a/builtin.h
+++ b/builtin.h
@@ -60,6 +60,7 @@ extern int cmd_diff_files(int argc, const char **argv, const 
char *prefix);
 extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
 extern int cmd_diff(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_builtin_difftool(int argc, const char **argv, const char 
*prefix);
 extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/builtin-difftool.c b/builtin/builtin-difftool.c
new file mode 100644
index 000..9feefcd
--- /dev/null
+++ b/builtin/builtin-difftool.c
@@ -0,0 +1,680 @@
+/*
+ * "git difftool" builtin command
+ *
+ * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
+ * git-difftool--helper script.
+ *
+ * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+ * The GIT_DIFF* variables are exported for use by git-difftool--helper.
+ *
+ * Any arguments that are unknown to this script are forwarded to 'git diff'.
+ *
+ * Copyright (C) 2016 Johannes Schindelin
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "run-command.h"
+#include "argv-array.h"
+#include "strbuf.h"
+#include "lockfile.h"
+
+static char *diff_gui_tool;
+static int trust_exit_code;
+
+static const char * const builtin_difftool_usage[] = {
+   N_("git add [] [--] ..."),
+   NULL
+};
+
+static int difftool_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "diff.guitool")) {
+   diff_gui_tool = xstrdup(value);
+   return 0;
+   }
+
+   if (!strcmp(var, "difftool.trustexitcode")) {
+   trust_exit_code = git_config_bool(var, value);
+   return 0;
+   }
+
+   return git_default_config(var, value, cb);
+}
+
+static int print_tool_help(void)
+{
+   const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
+   return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int parse_index_info(char *p, int *mode1, int *mode2,
+   struct object_id *oid1, struct object_id *oid2,
+   char *status)
+{
+   if (*p != ':')
+   return error("expected ':', got '%c'", *p);
+   *mode1 = (int)strtol(p + 1, , 8);
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   *mode2 = (int)strtol(p + 1, , 8);
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   if (get_oid_hex(++p, oid1))
+   return error("expected object ID, got '%s'", p + 1);
+   p += GIT_SHA1_HEXSZ;
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+ 

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

[Johannes Schindelin]

Hi Duy,

On Tue, 22 Nov 2016, Duy Nguyen wrote:

> On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
>  wrote:
> > 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);
> > +   }
> 
> This conditional config file reading is a trap for similar bugs to
> happen again. Is there any reason we should not just mark the command
> RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?

As I plan to slip these patches into Git for Windows v2.11.0, i.e. making
this a last-minute hot fix, I want to err on the side of caution. So I'd
rather keep this conditional (it might regress on the performance front,
or something).

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Nov 2016, Junio C Hamano wrote:

> 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 

Thanks for the corrected commit message!
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Nov 2016, Junio C Hamano wrote:

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

Could I ask you to sneak in a \n here?

Thanks,
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Mon, 21 Nov 2016, Junio C Hamano wrote:

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

True.

> The analysis of the log message in [2/3] is wrong and needs
> updating, though.

Thanks for following up on that, and for fixing the commit message.

> > +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?

No, this was me trying to ensure that the comment character '#' *is* used.
As opposed to somehow testing any edit script that contains no commented
lines whatsoever.

But if you don't care, I won't, either.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Hannes,

On Mon, 21 Nov 2016, Johannes Sixt wrote:

> 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)

D'oh.

> 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")

I was considering this, actually, but it is rather unreadable. Better
rewrite it in C, to begin with.

Thanks,
Dscho

Re: [GIT PULL] l10n updates for 2.11.0 round 2

[Jiang Xin]

Git 2.11.0-rc2 introduced a very small l10n update.  Because the
update windows will be
closed tomorrow, I made a batch updates. See commit:

* https://github.com/git-l10n/git-po/commit/275588f93

I have a reduced diff for this commit using a custom diff driver, see:

* https://gist.github.com/jiangxin/6384b1e865249228e00385fab84ef3f3


2016-11-22 22:59 GMT+08:00 Jiang Xin :
> Hi Junio,
>
> The following changes since commit 1310affe024fba407bff55dbe65cd6d670c8a32d:
>
>   Git 2.11-rc2 (2016-11-17 13:47:36 -0800)
>
> are available in the git repository at:
>
>   git://github.com/git-l10n/git-po tags/l10n-2.11.0-rnd2

--
Jiang Xin

[GIT PULL] l10n updates for 2.11.0 round 2

[Jiang Xin]

Hi Junio,

The following changes since commit 1310affe024fba407bff55dbe65cd6d670c8a32d:

  Git 2.11-rc2 (2016-11-17 13:47:36 -0800)

are available in the git repository at:

  git://github.com/git-l10n/git-po tags/l10n-2.11.0-rnd2

for you to fetch changes up to 275588f93eedd8d7a556c38b75944b858e704dce:

  l10n: Fixed typo of git fetch-pack command (2016-11-22 22:24:59 +0800)


l10n-2.11.0-rnd2


Changwoo Ryu (1):
  l10n: ko.po: Update Korean translation

Dimitriy Ryazantcev (1):
  l10n: ru.po: update Russian translation

Jean-Noel Avila (1):
  l10n: fr.po v2.11.0_rnd1

Jiang Xin (8):
  l10n: git.pot: v2.11.0 round 1 (209 new, 53 removed)
  Merge branch 'russian-l10n' of https://github.com/DJm00n/git-po-ru
  Merge branch 'ko/merge-l10n' of https://github.com/changwoo/git-l10n-ko
  Merge branch 'fr_v2.11.0_rnd1' of git://github.com/jnavila/git
  l10n: zh_CN: for git v2.11.0 l10n round 1
  Merge branch 'master' of git://github.com/git-l10n/git-po
  l10n: git.pot: v2.11.0 round 2 (1 new, 1 removed)
  l10n: Fixed typo of git fetch-pack command

Peter Krefting (1):
  l10n: sv.po: Update Swedish translation (2913t0f0u)

Trần Ngọc Quân (1):
  l10n: vi.po: Updated translation to v2.11.0 (2913t)

Vasco Almeida (1):
  l10n: pt_PT: update Portuguese translation

jfbu (1):
  l10n: fr.po fix grammar mistakes

 po/fr.po| 8916 +--
 po/git.pot  | 6664 
 po/ko.po| 8729 +++--
 po/pt_PT.po | 8794 --
 po/ru.po| 4429 +
 po/sv.po| 8709 +++--
 po/vi.po| 8697 +++--
 po/zh_CN.po | 8689 +++--
 8 files changed, 35318 insertions(+), 28309 deletions(-)

--
Jiang Xin

v2.11 new diff heuristic?

[Robert Dailey]

The release notes mention a new heuristic for diff:

* Output from "git diff" can be made easier to read by selecting
which lines are common and which lines are added/deleted
intelligently when the lines before and after the changed section
are the same. A command line option is added to help with the
experiment to find a good heuristics.

However, it lacks information on exactly how to use this new feature.
I dug into the git diff documentation here:

https://git-scm.com/docs/git-diff

It mentions a "--compaction-heuristic" option. Is this the new
heuristic outlined by the release notes? If not, which is it? Is the
compaction heuristic compatible with the histogram diff algorithm? Is
there a config option to turn this on all the time? For that matter,
is this something I can keep on all the time or is it only useful in
certain situations?

There's still so much more about this feature I would like to know.

Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines

[Vasco Almeida]

A Sex, 11-11-2016 às 11:45 -0100, Vasco Almeida escreveu:
> +=item comment_lines ( STRING [, STRING... ])
> +
> +Comments lines following core.commentchar configuration.
> +
> +=cut
> +
> +sub comment_lines {
> +   my $comment_line_char = config("core.commentchar") || '#';
> +   return prefix_lines("$comment_line_char ", @_);
> +}
> +

In light of the recent "Fix problems with rebase -i when
core.commentchar is defined" [1], I realized that this patch does not
handle the 'auto' value of core.commentchat configuration variable.

I propose to do the patch below in the next re-roll.

[1] http://www.mail-archive.com/git@vger.kernel.org/msg107818.html

-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..8d33634 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1073,6 +1073,7 @@ sub edit_hunk_manually {
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', 
'-');
my $comment_line_char = Git::config("core.commentchar") || '#';
+   $comment_line_char = '#' if ($comment_line_char eq 'auto');
print $fh Git::comment_lines sprintf(__ <

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Duy Nguyen]

On Tue, Nov 22, 2016 at 8:13 PM, Christian Couder
 wrote:
> So if we now mix things up just to avoid one more configuration
> option, we could very well make things harder to develop, to
> configure, to parse and to understand later, so it is not a trade off
> worth making.

OK since we're still in the early phase of determining how to
re-split, I guess going with elaborate, precise configuration is
better, even if it's more config options. Once we know better, maybe
we can decide a good default cover-all "auto" then (or maybe not
because we find out no shoe fits all).
-- 
Duy

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Christian Couder]

On Tue, Nov 22, 2016 at 11:35 AM, Duy Nguyen  wrote:

[...]

>>> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
>>> do the right thing, just re-split the index when it makes sense", "no"
>>> is disabled of course. If the user wants to be specific, just write
>>> "10" or some other percentage.(and either 0 or 100 would mean enable
>>> split-index but do not re-split automatically, let _me_ do it when I
>>> want it)
>>
>> The meaning of a future "auto" option for "core.splitIndex" could be
>> "use the split-index feature only if the number of entries in whole
>> index is greater than 1 (by default)".
>
> Well.. with the "just re-split the index when it makes sense" part,
> the user entrusts git to do something sensible in all cases,

That's an interpretation of what "core.splitIndex=true" could mean,
but there could be users who trust Git to re-split when it makes
sense, but who do want to use the split-index on all theirs repos even
the small ones or who just don't trust Git to choose when it might be
better to use it or not.

Yeah, a typical git user would most of the time just trust Git for all
those things, but on the other hand there are companies out there that
are willing to tweak many configuration options to get the better
possible behavior for them.

In fact I am working on this for Booking.com, and if we find out later
that we would gain something significant, like performance
improvements or configuration simplification, by adding "auto" and/or
other configuration variables to tweak more split-index related
things, we might very well post patch series to do that.

So if we now mix things up just to avoid one more configuration
option, we could very well make things harder to develop, to
configure, to parse and to understand later, so it is not a trade off
worth making.

> and going
> with absolute numbers might not be the best way, I think. It's big
> responsibility :)

About going with absolute number, yeah I am not sure at all it is the
best way, but this was just part of an example to try to explain what
I am saying above.

[PATCH] merge-recursive.c: use QSORT macro

[Nguyễn Thái Ngọc Duy]

This is the follow up of rs/qsort series, merged in b8688ad (Merge
branch 'rs/qsort' - 2016-10-10), where coccinelle was used to do
automatic transformation.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
  coccinelle missed this place, understandably, because it can't know
  that
  
  sizeof(*entries->items)
  
  is the same as
  
  sizeof(*df_name_compare.items)
  
  without some semantic analysis.

 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f..2d4dca9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -451,7 +451,7 @@ static void record_df_conflict_files(struct merge_options 
*o,
string_list_append(_sorted_entries, next->string)->util =
   next->util;
}
-   qsort(df_sorted_entries.items, entries->nr, sizeof(*entries->items),
+   QSORT(df_sorted_entries.items, entries->nr,
  string_list_df_name_compare);
 
string_list_clear(>df_conflict_file_set, 1);
-- 
2.8.2.524.g6ff3d78

Re: [PATCH 31/35] pathspec: allow querying for attributes

[Duy Nguyen]

On Fri, Nov 11, 2016 at 3:34 AM, Stefan Beller  wrote:
> @@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec 
> *pathspec)
>PATHSPEC_LITERAL |
>PATHSPEC_GLOB |
>PATHSPEC_ICASE |
> -  PATHSPEC_EXCLUDE);
> +  PATHSPEC_EXCLUDE |
> +  PATHSPEC_ATTR);

Hmm.. common_prefix_len() has always been a bit relaxing and can cover
more than needed. It's for early pruning. Exact pathspec matching
_will_ be done later anyway.

Is that obvious? I'm wondering if we need to add a line or two in the
big comment code before this statement. I'm thinking it is and we
probably don't need more comments...
-- 
Duy

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Duy Nguyen]

On Fri, Nov 18, 2016 at 9:34 PM, Christian Couder
 wrote:
> On Mon, Nov 7, 2016 at 10:38 AM, Duy Nguyen  wrote:
>> (sorry I got sick in the last few weeks and could not respond to this 
>> earlier)
>
> (Yeah, I have also been sick during the last few weeks.)
>
>> On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder
>>  wrote:
>>> Le 6 nov. 2016 09:16, "Junio C Hamano"  a écrit :

 Christian Couder  writes:

 > I think it is easier for user to be able to just set core.splitIndex
 > to true to enable split-index.

 You can have that exact benefit by making core.splitIndex to
 bool-or-more.  If your default is 20%, take 'true' as if the user
 specified 20% and take 'false' as if the user specified 100% (or is
 it 0%?  I do not care about the details but you get the point).
>>
>>> Then if we ever add 'auto' and the user wants for example 10% instead of the
>>> default 20%, we will have to make it accept things like "auto,10".
>
> (Sorry for writing the above on my phone which added HTML, so that it
> didn't reach the list.)
>
>> In my opinion, "true" _is_ auto, which is a way to say "I trust you to
>> do the right thing, just re-split the index when it makes sense", "no"
>> is disabled of course. If the user wants to be specific, just write
>> "10" or some other percentage.(and either 0 or 100 would mean enable
>> split-index but do not re-split automatically, let _me_ do it when I
>> want it)
>
> The meaning of a future "auto" option for "core.splitIndex" could be
> "use the split-index feature only if the number of entries in whole
> index is greater than 1 (by default)".

Well.. with the "just re-split the index when it makes sense" part,
the user entrusts git to do something sensible in all cases, and going
with absolute numbers might not be the best way, I think. It's big
responsibility :)

> If there is no difference between "true" and "auto" then, when users
> who have "core.splitIndex=true" will migrate to the git version that
> adds the "auto" feature, their repos with under 1 entires will not
> use the split-index feature anymore. These users may then be annoyed
> that the behavior has been switched under them, and that the
> split-index feature is not always used despite having
> "core.splitIndex=true" in their config.
-- 
Duy

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

[Duy Nguyen]

On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
 wrote:
> 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,

I think this is 180bad3 (rebase -i: respect core.commentchar -
2013-02-11) and a couple followup fixes. My bad.

> and in turn forgot to let rebase -i handle that
> new value gracefully.
>
> Reported by Taufiq Hoven.

Another report in the same area: a merge conflict always writes the
"Conflicts:" section in the commit message with '#' as comment char
when core.commentChar is auto. I've known this for a while, but it has
not bitten me hard enough to do something about it,

>
> 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=#

My first thought was "this kinda defeats the purpose of auto, doesn't
it". But 'auto' here is irrelevant because we control the leading
character of all lines in the todo file. 'auto' makes little sense in
this context, falling back to any comment char would do.

So, ack (after you fix the $(comment_char other people have mentioned,
of course).
-- 
Duy

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

[Duy Nguyen]

On Mon, Nov 21, 2016 at 9:18 PM, Johannes Schindelin
 wrote:
> 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);
> +   }

This conditional config file reading is a trap for similar bugs to
happen again. Is there any reason we should not just mark the command
RUN_SETUP_GENTLY in git.c and call git_config() here unconditionally?

>
> if (strbuf_read(, 0, 1024) < 0)
> die_errno("could not read the input");
-- 
Duy

[PATCH 3/3] worktree list: keep the list sorted

[Nguyễn Thái Ngọc Duy]

It makes it easier to write tests for. But it should also be good for
the user since locating a worktree by eye would be easier once they
notice this.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/worktree.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index b835b91..80ccc51 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -434,6 +434,13 @@ static void measure_widths(struct worktree **wt, int 
*abbrev, int *maxlen)
}
 }
 
+static int compare_worktree(const void *a_, const void *b_)
+{
+   const struct worktree *const *a = a_;
+   const struct worktree *const *b = b_;
+   return fspathcmp((*a)->path, (*b)->path);
+}
+
 static int list(int ac, const char **av, const char *prefix)
 {
int porcelain = 0;
@@ -448,11 +455,20 @@ static int list(int ac, const char **av, const char 
*prefix)
usage_with_options(worktree_usage, options);
else {
struct worktree **worktrees = get_worktrees();
-   int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
+   int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i, nr;
 
if (!porcelain)
measure_widths(worktrees, , _maxlen);
 
+   for (i = nr = 0; worktrees[i]; i++)
+   nr++;
+
+   /*
+* don't sort the first item (main worktree), which will
+* always be the first
+*/
+   QSORT(worktrees + 1, nr - 1, compare_worktree);
+
for (i = 0; worktrees[i]; i++) {
if (porcelain)
show_worktree_porcelain(worktrees[i]);
-- 
2.8.2.524.g6ff3d78

[PATCH 1/3] worktree.c: zero new 'struct worktree' on allocation

[Nguyễn Thái Ngọc Duy]

This keeps things a bit simpler when we add more fields, knowing that
default values are always zero.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/worktree.c b/worktree.c
index f7869f8..f7c1b5e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -91,16 +91,11 @@ static struct worktree *get_main_worktree(void)
if (parse_ref(path.buf, _ref, _detached) < 0)
goto done;
 
-   worktree = xmalloc(sizeof(struct worktree));
+   worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(_path, NULL);
-   worktree->id = NULL;
worktree->is_bare = is_bare;
-   worktree->head_ref = NULL;
worktree->is_detached = is_detached;
-   worktree->is_current = 0;
add_head_info(_ref, worktree);
-   worktree->lock_reason = NULL;
-   worktree->lock_reason_valid = 0;
 
 done:
strbuf_release();
@@ -138,16 +133,11 @@ static struct worktree *get_linked_worktree(const char 
*id)
if (parse_ref(path.buf, _ref, _detached) < 0)
goto done;
 
-   worktree = xmalloc(sizeof(struct worktree));
+   worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(_path, NULL);
worktree->id = xstrdup(id);
-   worktree->is_bare = 0;
-   worktree->head_ref = NULL;
worktree->is_detached = is_detached;
-   worktree->is_current = 0;
add_head_info(_ref, worktree);
-   worktree->lock_reason = NULL;
-   worktree->lock_reason_valid = 0;
 
 done:
strbuf_release();
-- 
2.8.2.524.g6ff3d78

[PATCH 0/3] Minor fixes on 'git worktree'

[Nguyễn Thái Ngọc Duy]

This fixes two things:

 - make sure the first item is always the main worktree even if we
   fail to retrieve some info

 - keep 'worktree list' order stable (which in turn fixes the random
   failure on my 'worktree-move' series

Nguyễn Thái Ngọc Duy (3):
  worktree.c: zero new 'struct worktree' on allocation
  get_worktrees() must return main worktree as first item even on error
  worktree list: keep the list sorted

 builtin/worktree.c | 26 ++
 worktree.c | 20 
 2 files changed, 26 insertions(+), 20 deletions(-)

-- 
2.8.2.524.g6ff3d78

[PATCH 2/3] get_worktrees() must return main worktree as first item even on error

[Nguyễn Thái Ngọc Duy]

This is required by git-worktree.txt, stating that the main worktree is
the first line (especially in --porcelain mode when we can't just change
behavior at will).

There's only one case when get_worktrees() may skip main worktree, when
parse_ref() fails. Update the code so that we keep first item as main
worktree and return something sensible in this case:

 - In user-friendly mode, since we're not constraint by anything,
   returning "(error)" should do the job (we already show "(detached
   HEAD)" which is not machine-friendly). Actually errors should be
   printed on stderr by parse_ref() (*)

 - In plumbing mode, we do not show neither 'bare', 'detached' or
   'branch ...', which is possible by the format description if I read
   it right.

Careful readers may realize that when the local variable "head_ref" in
get_main_worktree() is emptied, add_head_info() will do nothing to
wt->head_sha1. But that's ok because head_sha1 is zero-ized in the
previous patch.

(*) Well, it does not. But it's supposed to be a stop gap
implementation until refs we can reuse code, to parse "ref: " stuff
in HEAD, from resolve_refs_unsafe(). Now may be the time since
refs refactoring is mostly done.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/worktree.c | 8 +---
 worktree.c | 6 ++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c4854d..b835b91 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
if (wt->is_detached)
printf("detached\n");
-   else
+   else if (wt->head_ref)
printf("branch %s\n", wt->head_ref);
}
printf("\n");
@@ -406,10 +406,12 @@ static void show_worktree(struct worktree *wt, int 
path_maxlen, int abbrev_len)
else {
strbuf_addf(, "%-*s ", abbrev_len,
find_unique_abbrev(wt->head_sha1, 
DEFAULT_ABBREV));
-   if (!wt->is_detached)
+   if (wt->is_detached)
+   strbuf_addstr(, "(detached HEAD)");
+   else if (wt->head_ref)
strbuf_addf(, "[%s]", 
shorten_unambiguous_ref(wt->head_ref, 0));
else
-   strbuf_addstr(, "(detached HEAD)");
+   strbuf_addstr(, "(error)");
}
printf("%s\n", sb.buf);
 
diff --git a/worktree.c b/worktree.c
index f7c1b5e..a674efa 100644
--- a/worktree.c
+++ b/worktree.c
@@ -89,7 +89,7 @@ static struct worktree *get_main_worktree(void)
strbuf_addf(, "%s/HEAD", get_git_common_dir());
 
if (parse_ref(path.buf, _ref, _detached) < 0)
-   goto done;
+   strbuf_reset(_ref);
 
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(_path, NULL);
@@ -97,7 +97,6 @@ static struct worktree *get_main_worktree(void)
worktree->is_detached = is_detached;
add_head_info(_ref, worktree);
 
-done:
strbuf_release();
strbuf_release(_path);
strbuf_release(_ref);
@@ -173,8 +172,7 @@ struct worktree **get_worktrees(void)
 
list = xmalloc(alloc * sizeof(struct worktree *));
 
-   if ((list[counter] = get_main_worktree()))
-   counter++;
+   list[counter++] = get_main_worktree();
 
strbuf_addf(, "%s/worktrees", get_git_common_dir());
dir = opendir(path.buf);
-- 
2.8.2.524.g6ff3d78