Re: [PATCH] packfile: Correct zlib buffer handling

[Duy Nguyen]

On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton  wrote:
> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git 
> *p,
> return NULL;
> memset(, 0, sizeof(stream));
> stream.next_out = buffer;
> -   stream.avail_out = size + 1;
> +   stream.avail_out = size;

You may want to include in your commit message a reference to
39eea7bdd9 (Fix incorrect error check while reading deflated pack data
- 2009-10-21) which adds this plus one with a fascinating story
behind.
-- 
Duy

Re: [GSoC][PATCH v3 1/4] rebase: duplicate git-rebase--interactive.sh to git-rebase--preserve-merges.sh

[Junio C Hamano]

Alban Gruin  writes:

> This duplicates git-rebase--interactive.sh to
> git-rebase--preserve-merges.sh. This is done to split -p from -i. No
> modifications are made to this file here, but any code that is not used by -p
> will be stripped in the next commit.
>
> Signed-off-by: Alban Gruin 
> ---
>  .gitignore |1 +
>  Makefile   |2 +
>  git-rebase--preserve-merges.sh | 1069 
> 

I would normally say "Yuck" to an approach like this series, because
it does not force us to make any explicit effort to share as much
code between the two codepaths.

But the codepath for "-p" is something that has been abandoned even
by its original author and would be going away, so the net effect of
this series to the project in the longer term is to remove the "dead
code" that will be made unnecessary from "git-rebase--interactive"
when support for "-p" gets discarded in a distant future, and do so
before we actually remove "-p" (and replace it with the "recreate
merges" feature).

So I won't have objection to the approach taken in this series.  I
may still raise issues on individual changes, though.

Thanks.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King  writes:

>> By the way, this is one of these times when I feel that we should
>> have a better multi-line message support in die/error/warning/info
>> functions.  Ideally, I should be able to write
>> 
>>  warning(_("the '-l' option is an alias for '--create-reflog' and\n"
>>"has no effect in list mode, This option will soon be\n"
>>"removed and you should omit it (or use '--list' instead)."));
>> 
>> and warning() would:
>> 
>>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>>message;
>> 
>>  - chomp that into lines at '\n' boundary; and
>> 
>>  - give each of these lines with _("warning: ") prefixed.
>> 
>> That way, translators can choose to make the resulting message to
>> different number of lines from the original easily.
>
> Yep, I totally agree. In past discussions I was thinking mostly of
> the pain of writing these multi-line messages. But I imagine it is
> absolute hell for translators, and we should fix it for that reason.
>
> (Also, I guess this message probably ought to be marked for
> translation).

Needless to tell you, I worked backwards when noticing that these
three warning() lines are not marked for translation ;-).

But of course, fixing this in a naïve way will involve memory
allocation during execution of die(), which may well be due to OOM,
which is why we knew the need but haven't found a good solution to
it.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Junio C Hamano  writes:

> With these two patches queued on top of jk/branch-l-0-deprecation,
> the follow-up patches jk/branch-l-1-removal that makes 'branch -l'
> to fail and then jk/branch-l-2-reincarnation that makes 'branch -l'
> a synonym to 'branch --list' needs rebasing.  Both are trivial and
> hopefully I did them correctly.
>
> -- >8 --
> From: Jeff King 
> Date: Mon, 26 Mar 2018 03:29:22 -0400
> Subject: [PATCH] branch: drop deprecated "-l" option

And this is the final "reincarnation" step.

-- >8 --
From: Jeff King 
Date: Mon, 26 Mar 2018 03:29:48 -0400
Subject: [PATCH] branch: make "-l" a synonym for "--list"

The other "mode" options of git-branch have short-option
aliases that are easy to type (e.g., "-d" and "-m"). Let's
give "--list" the same treatment.

This also makes it consistent with the similar "git tag -l"
option.

We didn't do this originally because "--create-reflog" was
squatting on the "-l" option. Now that sufficient time has
passed with that alias removed, we can finally repurpose it.

Signed-off-by: Jeff King 
Reviewed-by: Eric Sunshine 
Reviewed-by: Jacob Keller 
[jc: adjusted the new tests added earlier]
Signed-off-by: Junio C Hamano 
---
 builtin/branch.c  | 2 +-
 t/t3200-branch.sh | 8 +---
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 01b35b3c3d..1d06f5c547 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -612,7 +612,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BIT('M', NULL, , N_("move/rename a branch, even if 
target exists"), 2),
OPT_BIT('c', "copy", , N_("copy a branch and its reflog"), 
1),
OPT_BIT('C', NULL, , N_("copy a branch, even if target 
exists"), 2),
-   OPT_BOOL(0, "list", , N_("list branch names")),
+   OPT_BOOL('l', "list", , N_("list branch names")),
OPT_BOOL(0, "create-reflog", , N_("create the branch's 
reflog")),
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index eca75d3ca1..022d6a41c8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -45,12 +45,6 @@ test_expect_success 'git branch HEAD should fail' '
test_must_fail git branch HEAD
 '
 
-test_expect_success 'git branch -l no longer is --create-reflog' '
-   test_when_finished "git branch -D new-branch-with-reflog || :" &&
-   test_must_fail git branch -l new-branch-with-reflog &&
-   test_must_fail git rev-parse --verify refs/heads/new-branch-with-reflog
-'
-
 cat >expect < 1117150200 +
branch: Created from master
 EOF
@@ -294,7 +288,7 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 '
 
-test_expect_failure 'git branch -l eventually is --list' '
+test_expect_success 'git branch -l is --list' '
git branch --list >expect &&
git branch -l >actual &&
test_cmp expect actual
-- 
2.17.0-775-ge144d126d7

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Junio C Hamano  writes:

> Yup, thanks for being extra explicit.  I do imagine there are quite
> a few of us who would be puzzled without this update (but with the
> previous one to unhide it from behind the pager).

With these two patches queued on top of jk/branch-l-0-deprecation,
the follow-up patches jk/branch-l-1-removal that makes 'branch -l'
to fail and then jk/branch-l-2-reincarnation that makes 'branch -l'
a synonym to 'branch --list' needs rebasing.  Both are trivial and
hopefully I did them correctly.

-- >8 --
From: Jeff King 
Date: Mon, 26 Mar 2018 03:29:22 -0400
Subject: [PATCH] branch: drop deprecated "-l" option

We marked the "-l" option as deprecated back in . Now that sufficient time has passed, let's follow
through and get rid of it.

Signed-off-by: Jeff King 
Reviewed-by: Eric Sunshine 
Reviewed-by: Jacob Keller 
[jc: added a few tests]
Signed-off-by: Junio C Hamano 
---
 builtin/branch.c  | 14 --
 t/t3200-branch.sh | 12 
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b0b33dab94..01b35b3c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -571,14 +571,6 @@ static int edit_branch_description(const char *branch_name)
return 0;
 }
 
-static int deprecated_reflog_option_cb(const struct option *opt,
-  const char *arg, int unset)
-{
-   used_deprecated_reflog_option = 1;
-   *(int *)opt->value = !unset;
-   return 0;
-}
-
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
int delete = 0, rename = 0, copy = 0, force = 0, list = 0;
@@ -622,12 +614,6 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_BIT('C', NULL, , N_("copy a branch, even if target 
exists"), 2),
OPT_BOOL(0, "list", , N_("list branch names")),
OPT_BOOL(0, "create-reflog", , N_("create the branch's 
reflog")),
-   {
-   OPTION_CALLBACK, 'l', NULL, , NULL,
-   N_("deprecated synonym for --create-reflog"),
-   PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
-   deprecated_reflog_option_cb
-   },
OPT_BOOL(0, "edit-description", _description,
 N_("edit the description for the branch")),
OPT__FORCE(, N_("force creation, move/rename, deletion"), 
PARSE_OPT_NOCOMPLETE),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index da97b8a62b..eca75d3ca1 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -45,6 +45,12 @@ test_expect_success 'git branch HEAD should fail' '
test_must_fail git branch HEAD
 '
 
+test_expect_success 'git branch -l no longer is --create-reflog' '
+   test_when_finished "git branch -D new-branch-with-reflog || :" &&
+   test_must_fail git branch -l new-branch-with-reflog &&
+   test_must_fail git rev-parse --verify refs/heads/new-branch-with-reflog
+'
+
 cat >expect < 1117150200 +
branch: Created from master
 EOF
@@ -288,6 +294,12 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 '
 
+test_expect_failure 'git branch -l eventually is --list' '
+   git branch --list >expect &&
+   git branch -l >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'git branch --column' '
COLUMNS=81 git branch --column=column >actual &&
cat >expected <<\EOF &&
-- 
2.17.0-775-ge144d126d7

Re: [PATCH 2/5] t6036, t6042: use test_line_count instead of wc -l

[Elijah Newren]

On Thu, May 24, 2018 at 6:17 PM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>>> -test 2 = $(git ls-files -s | wc -l) &&
>>> -test 2 = $(git ls-files -u | wc -l) &&
>>> -test 2 = $(git ls-files -o | wc -l) &&
>>
>> Here 'git ls-files -o' should have listed two untracked files ...
>>
>>> +git ls-files -s >out &&
>>> +test_line_count = 2 out &&
>>> +git ls-files -u >out &&
>>> +test_line_count = 2 out &&
>>> +git ls-files -o >out &&
>>> +test_line_count = 3 out &&
>>
>> ... but now you expect it to list three.  I was about to point out the
>> typo, but then noticed that you expect it to list one more untracked
>> file than before in all subsequent tests...  now that can't be just a
>> typo, can it?
>>
>> Please mention in the commit message that when using an intermediate
>> file to store the output, 'git ls-files -o' will list that file, too,
>> that's why the number of expected untracked files had to be adjusted;
>> so future readers won't have to figure this out themselves.
>
> I'd expect that a reader of the commit who cares enough to bother to
> wonder by looking at the patch and seeing that 2 became 3 would know
> why already.  And a reader of the resulting file would not know that
> the 3 used to be 2, and won't be helped by "we used to count to 2,
> now we have 'out' also counted" that much, especially in the commit
> log message.  What would help the latter would be to name which
> three paths we expect to see in the comment (or test against the
> exact list of paths, instead of using test_line_count).
>
>> An alternative to consider would be to add a .gitignore file in the
>> initial commit to ignore 'out', then the number of untracked files
>> don't have to be adjusted.
>
> I think that is a preferred solution that we've used in ls-files and
> status tests successfully.

...except that if we add a .gitignore to each initial commit (we use
test_create_repo for nearly every test to keep them separable meaning
we'd have to do this many times), then four lines above we have to
adjust the number of expected tracked files.  And, for it to work,
we'd have to add an --exclude-standard flag to ls-files -o.

I can make that change if you both want it, but it seems like it's
actually making it harder to follow the changes rather than easier.

-- >8 --
Subject: [PATCH] fixup! t6036, t6042: use test_line_count instead of wc -l

---

and here's what it looks like which you can apply as a fixup.  I think it makes 
things worse, though.

 t/t6036-recursive-corner-cases.sh| 81 +---
 t/t6042-merge-rename-corner-cases.sh | 93 +++-
 2 files changed, 95 insertions(+), 79 deletions(-)

diff --git a/t/t6036-recursive-corner-cases.sh 
b/t/t6036-recursive-corner-cases.sh
index 3e659cff28..06944a8c0a 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -25,6 +25,7 @@ test_expect_success 'setup basic criss-cross + rename with no 
modifications' '
(
cd basic-rename &&
 
+   echo out >.gitignore &&
ten="0 1 2 3 4 5 6 7 8 9" &&
for i in $ten
do
@@ -34,7 +35,7 @@ test_expect_success 'setup basic criss-cross + rename with no 
modifications' '
do
echo line $i in another sample file
done >two &&
-   git add one two &&
+   git add .gitignore one two &&
test_tick && git commit -m initial &&
 
git branch L1 &&
@@ -66,11 +67,11 @@ test_expect_success 'merge simple rename+criss-cross with 
no modifications' '
test_must_fail git merge -s recursive R2^0 &&
 
git ls-files -s >out &&
-   test_line_count = 2 out &&
+   test_line_count = 3 out &&
git ls-files -u >out &&
test_line_count = 2 out &&
-   git ls-files -o >out &&
-   test_line_count = 3 out &&
+   git ls-files -o --exclude-standard >out &&
+   test_line_count = 2 out &&
 
test $(git rev-parse :2:three) = $(git rev-parse L2:three) &&
test $(git rev-parse :3:three) = $(git rev-parse R2:three) &&
@@ -97,6 +98,7 @@ test_expect_success 'setup criss-cross + rename merges with 
basic modification'
(
cd rename-modify &&
 
+   echo out >.gitignore &&
ten="0 1 2 3 4 5 6 7 8 9" &&
for i in $ten
do
@@ -106,7 +108,7 @@ test_expect_success 'setup criss-cross + rename merges with 
basic modification'
do
echo line $i in another sample file
done >two &&
-   git add one two &&
+   git add .gitignore one two &&
test_tick && git 

Re: [PATCH] packfile: Correct zlib buffer handling

[Todd Zullinger]

Jeremy Linton wrote:
> The buffer being passed to zlib includes a null terminator that
> git needs to keep in place. unpack_compressed_entry() attempts to
> detect the case that the source buffer hasn't been fully consumed
> by checking to see if the destination buffer has been over consumed.
> 
> This yields two problems, first a single byte overrun won't be detected
> properly because the Z_STREAM_END will then be set, but the null
> terminator will have been overwritten. The other problem is that
> more recent zlib patches have been poisoning the unconsumed portions
> of the buffers which also overwrites the null, while correctly
> returning length and status.
> 
> Lets rely on the fact that the source buffer will only be fully
> consumed when the when the destination buffer is inflated to the
> correct size. We can do this by passing zlib the correct buffer size
> and properly checking the return status. The latter check actually
> already exists if the buffer size is correct.
> 
> Signed-off-by: Jeremy Linton 
> ---

As a little background, earlier today Pavel Cahyna filed a
ticket about a regression in a recent zlib update on aarch64
in Fedora[1].

While he was doing that I was just beginning to look at why
the git test suite failed fairly badly a build last
night[2].  The aarch64 build on Fedora 28 failed, while it
succeeded on all other architectures (armv7hl, i686, ppc64,
ppc64le, s390x, x86_64).  It also passed on newer and older
Fedora releases.

The difference was that the Fedora 28 zlib build has some
aarch64 optimization patches applied[3].  (Those patches are
in rawhide/f29 as well, but due to an unrelated issue have
not yet made it into the buildroot used for the git build.)

I'm woefully unqualified to comment on the patch, but if
there are any questions about how this was found, I hope the
above background will be helpful.

A big thanks to Jeremy for dropping whatever he had planned
to do today and dig into this issue.  I can only hope it was
either more fun or less work than what he hoped to do with
his Friday. :)

Thanks also to Pavel for finding the source of the failures
and filing a detailed bug report to get things moving.

[1] https://bugzilla.redhat.com/1582555
[2] https://fedorapeople.org/~tmz/git-aarch64-make-test
[3] https://src.fedoraproject.org/rpms/zlib/c/25e9802

>  packfile.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/packfile.c b/packfile.c
> index 7c1a2519f..245eb3204 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git 
> *p,
>   return NULL;
>   memset(, 0, sizeof(stream));
>   stream.next_out = buffer;
> - stream.avail_out = size + 1;
> + stream.avail_out = size;
>  
>   git_inflate_init();
>   do {
> @@ -1424,7 +1424,7 @@ static void *unpack_compressed_entry(struct packed_git 
> *p,
>   stream.next_in = in;
>   st = git_inflate(, Z_FINISH);
>   if (!stream.avail_out)
> - break; /* the payload is larger than it should be */
> + break; /* done, st indicates if source fully consumed */
>   curpos += stream.next_in - in;
>   } while (st == Z_OK || st == Z_BUF_ERROR);
>   git_inflate_end();

-- 
Todd
~~
An election is coming. Universal peace is declared and the foxes have
a sincere interest in prolonging the lives of the poultry.
-- T.S. Eliot

Re: [PATCH 2/5] t6036, t6042: use test_line_count instead of wc -l

[Elijah Newren]

On Thu, May 24, 2018 at 6:17 PM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>>> -test 2 = $(git ls-files -s | wc -l) &&
>>> -test 2 = $(git ls-files -u | wc -l) &&
>>> -test 2 = $(git ls-files -o | wc -l) &&
>>
>> Here 'git ls-files -o' should have listed two untracked files ...
>>
>>> +git ls-files -s >out &&
>>> +test_line_count = 2 out &&
>>> +git ls-files -u >out &&
>>> +test_line_count = 2 out &&
>>> +git ls-files -o >out &&
>>> +test_line_count = 3 out &&
>>
>> ... but now you expect it to list three.  I was about to point out the
>> typo, but then noticed that you expect it to list one more untracked
>> file than before in all subsequent tests...  now that can't be just a
>> typo, can it?
>>
>> Please mention in the commit message that when using an intermediate
>> file to store the output, 'git ls-files -o' will list that file, too,
>> that's why the number of expected untracked files had to be adjusted;
>> so future readers won't have to figure this out themselves.
>
> I'd expect that a reader of the commit who cares enough to bother to
> wonder by looking at the patch and seeing that 2 became 3 would know
> why already.  And a reader of the resulting file would not know that
> the 3 used to be 2, and won't be helped by "we used to count to 2,
> now we have 'out' also counted" that much, especially in the commit
> log message.  What would help the latter would be to name which
> three paths we expect to see in the comment (or test against the
> exact list of paths, instead of using test_line_count).
>
>> An alternative to consider would be to add a .gitignore file in the
>> initial commit to ignore 'out', then the number of untracked files
>> don't have to be adjusted.
>
> I think that is a preferred solution that we've used in ls-files and
> status tests successfully.

...except that if we add a .gitignore to each initial commit (we use
test_create_repo for nearly every test to keep them separable meaning
we'd have to do this many times), then four lines above we have to
adjust the number of expected tracked files.  And, for it to work,
we'd have to add an --exclude-standard flag to ls-files -o.

I can make that change if you both want it, but it seems like it's
actually making it harder to follow the changes rather than easier.

Re: [PATCH] packfile: Correct zlib buffer handling

[Eric Sunshine]

On Fri, May 25, 2018 at 7:17 PM, Jeremy Linton  wrote:
> The buffer being passed to zlib includes a null terminator that
> git needs to keep in place. unpack_compressed_entry() attempts to
> detect the case that the source buffer hasn't been fully consumed
> by checking to see if the destination buffer has been over consumed.
>
> This yields two problems, first a single byte overrun won't be detected
> properly because the Z_STREAM_END will then be set, but the null
> terminator will have been overwritten. The other problem is that
> more recent zlib patches have been poisoning the unconsumed portions
> of the buffers which also overwrites the null, while correctly
> returning length and status.
>
> Lets rely on the fact that the source buffer will only be fully

s/Lets/Let's/

> consumed when the when the destination buffer is inflated to the

s/when the when the/when the/

> correct size. We can do this by passing zlib the correct buffer size
> and properly checking the return status. The latter check actually
> already exists if the buffer size is correct.
>
> Signed-off-by: Jeremy Linton 

[PATCH] packfile: Correct zlib buffer handling

[Jeremy Linton]

The buffer being passed to zlib includes a null terminator that
git needs to keep in place. unpack_compressed_entry() attempts to
detect the case that the source buffer hasn't been fully consumed
by checking to see if the destination buffer has been over consumed.

This yields two problems, first a single byte overrun won't be detected
properly because the Z_STREAM_END will then be set, but the null
terminator will have been overwritten. The other problem is that
more recent zlib patches have been poisoning the unconsumed portions
of the buffers which also overwrites the null, while correctly
returning length and status.

Lets rely on the fact that the source buffer will only be fully
consumed when the when the destination buffer is inflated to the
correct size. We can do this by passing zlib the correct buffer size
and properly checking the return status. The latter check actually
already exists if the buffer size is correct.

Signed-off-by: Jeremy Linton 
---
 packfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/packfile.c b/packfile.c
index 7c1a2519f..245eb3204 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,
return NULL;
memset(, 0, sizeof(stream));
stream.next_out = buffer;
-   stream.avail_out = size + 1;
+   stream.avail_out = size;
 
git_inflate_init();
do {
@@ -1424,7 +1424,7 @@ static void *unpack_compressed_entry(struct packed_git *p,
stream.next_in = in;
st = git_inflate(, Z_FINISH);
if (!stream.avail_out)
-   break; /* the payload is larger than it should be */
+   break; /* done, st indicates if source fully consumed */
curpos += stream.next_in - in;
} while (st == Z_OK || st == Z_BUF_ERROR);
git_inflate_end();
-- 
2.13.6

[PATCH] packfile: Correct zlib buffer handling

[Jeremy Linton]

The buffer being passed to zlib includes a null terminator that
git needs to keep in place. unpack_compressed_entry() attempts to
detect the case that the source buffer hasn't been fully consumed
by checking to see if the destination buffer has been over consumed.

This yields two problems, first a single byte overrun won't be detected
properly because the Z_STREAM_END will then be set, but the null
terminator will have been overwritten. The other problem is that
more recent zlib patches have been poisoning the unconsumed portions
of the buffers which also overwrites the null, while correctly
returning length and status.

Lets rely on the fact that the source buffer will only be fully
consumed when the when the destination buffer is inflated to the
correct size. We can do this by passing zlib the correct buffer size
and properly checking the return status. The latter check actually
already exists if the buffer size is correct.

Signed-off-by: Jeremy Linton 
---
 packfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/packfile.c b/packfile.c
index 7c1a2519f..245eb3204 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct packed_git *p,
return NULL;
memset(, 0, sizeof(stream));
stream.next_out = buffer;
-   stream.avail_out = size + 1;
+   stream.avail_out = size;
 
git_inflate_init();
do {
@@ -1424,7 +1424,7 @@ static void *unpack_compressed_entry(struct packed_git *p,
stream.next_in = in;
st = git_inflate(, Z_FINISH);
if (!stream.avail_out)
-   break; /* the payload is larger than it should be */
+   break; /* done, st indicates if source fully consumed */
curpos += stream.next_in - in;
} while (st == Z_OK || st == Z_BUF_ERROR);
git_inflate_end();
-- 
2.13.6

Re: [PATCH v3 1/7] read-cache: teach refresh_cache_entry() to take istate

[Stefan Beller]

On Wed, May 23, 2018 at 7:47 AM, Jameson Miller  wrote:
> Refactor refresh_cache_entry() to work on a specific index, instead of
> implicitly using the_index. This is in preparation for making the
> make_cache_entry function work on a specific index.
>
> Signed-off-by: Jameson Miller 

Reviewed-by: Stefan Beller 

Re: [PATCH v3 0/7] allocate cache entries from memory pool

[Stefan Beller]

>
> The memory pool design makes some tradeoffs. It is not meant to
> be completely replace malloc / free as a general purpose
> allocator, but rather used in scenarios where the benefit (faster
> allocations, lower bookkeeping overhead) is worth the
> tradeoffs (not able to free individual allocations).

So this is the actual stated design goal of this memory pool?
Fast allocation with little overhead for giving up individual frees?

> We debated several approaches for what to do here

it would be awesome if the list could participate in the discussion
even if only read-only.

Re: [PATCH v3 0/7] allocate cache entries from memory pool

[Stefan Beller]

On Wed, May 23, 2018 at 7:47 AM, Jameson Miller  wrote:
> Changes from V2:
>
> - Tweak logic of finding available memory block for memory
>   allocation
>
>   - Only search head block
>
> - Tweaked handling of large memory allocations.
>
>   - Large blocks now tracked in same manner as "regular"
> blocks
>
>   - Large blocks are placed at end of linked list of memory
> blocks
>
> - Cache_entry type gains notion of whether it was allocated
>   from memory pool or not
>
>   - Collapsed cache_entry discard logic into single
> function. This should make code easier to maintain
>
> - Small tweaks based on V1 feedback
>
> Base Ref: master
> Web-Diff: g...@github.com:jamill/git.git/commit/d608515f9e

Unrelated to this series, but to the tool you use to generate the cover-letter:
I think the Web-Diff only works when you give the http[s] address, git@
won't work here?

Re: [PATCH v2 02/18] Add a new builtin: branch-diff

[Stefan Beller]

Johannes,

On IRC you wrote:
 And BTW this is not bike-shedding to me. Discussing the name
of a variable, or indentation, or line wrapping, is. But improving the
user experience is important. We *suck* on that, historically, and I
do want to break with that habit.
...
 avar, _ikke_: so a colleague of mine whose opinion on naming I
respect more than all Git developers combined *also* came up with the
term `range-diff`, independently.
...
 Yes, you are looking at two ranges. But not *any* two ranges.
*That* is my point.

So I sat back and want to try again;

IIUC your dislike for "range-diff" boils down to:
(A) it doesn't diff any arbitrary range, as the output would become
very cumbersome and hard to understand,
(B) it is not a good intuitive name for users, as they would not think
of range-diff when they'd want to have this feature.

Regarding (A), I think the same can be said about input to the diff
machinery, e.g. 'git diff v2.0.0 v2.17.0' is just very much text, and
it is hardly useful (except as a patch fed to the machine).

Over time there were added tons of options that make the diff output
easier to digest, e.g. additional pathspecs to restrict to a sub tree or
ignoring certain things (white spaces mostly), such that
'git diff -w v2.0.0 v2.17.0 -- refs.h' is easier for a human to grok.

Regarding (B), I agree, but blame it on the nature of an open
source project that provides a toolbox. So the way a user is
going to discover this feature is via stackoverflow or via
asking a coworker or finding the example output somewhere.

I think that last point could be part of the feedback:
git-diff has prominently hints at its name via "diff --git ..."
in the first line of its output, so maybe the output of this feature
also wants to name itself?

Other thoughts:
We could go down the route and trying to find a best possible
technical name, for which I could offer:

  revision-walk-difference
  revwalk-diff

As that literally describes the output: two rev walks are
performed and then those outputs of the rev walks is diffed.
Based off these technicals we could get more creative:

  redo-rev-walk-spot-the-difference
  re-walk-spot
  retravel-spot
  spot-diff

But I think all these do not address the feedback (B).

"What would a user find intuitive?"; I personally thought
a bit about how I discovered cherry-pick. I just took it as
a given name, without much thought, as I discovered it
by tell tale, not looking for it in the docs. It sort of made
sense as a command that I learned earlier about,
"interactive rebase", also has had the "pick" command,
such that "picking" made sense. I think I retroactively
made sense of the "cherry" part. Now I tried to find it
in the mailing list archive and actually learn about its origin,
but no good stories are found there.

For what the user might find most useful, I just looked
at other tools in Gerrits landscape and there the expectation
seems that you upload your code first and do the diff of the different
patches serverside. I think the same holds for Github or other
branch based reviewing systems. You can force push the
branch that is pull requested and the web UI somehow makes
sense of it.

That leads me to the (weak) conclusion of branch-diff or tbdiff
to be useful most for patch based / mailing list based workflows
as there is no magic server helping you out.

Searching for "kernel +tbdiff" to find the kernel devs using tbdiff
gave me no results, so I may be mistaken there.

Trying to find "interdiffs" (for the lack of a better name) between
patches on the kernel mailing list also is not obvious to the uninitiated.

So for the various workflows, I could come up with

  change-diff
  pullrequest-diff
  patch-series-diff

but we do not look at diffs, rather we only use this tool to work on
incremental things, so maybe instead:

  change-history
  pullrequest-history
  patch-series-evolution

Note how these are 3 suggestions, one for each major workflow,
and I'd *REALLY* would want to have a tool that is agnostic to the
workflow on top (whether you use pull requests or Gerrit changes),
but now I would like to step back and remind us that this tool
is only mostly used for viewing the evolution of your new thing,
but it can also be very useful to inspect non-new things.
(backported patches to maint, or some -stable branch)

Or rather: We do not know the major use case yet. Sure
I will use it in my cover letter and that is on my mind now,
but I think there are other use cases that are not explored
yet, so we should rather make the naming decision based
off of technicals rather than anticipated use case and user
discovery methods.

I hope this is actually useful feedback on the naming discovery.

Thanks,
Stefan

Re: [PATCH v2 3/5] config doc: elaborate on what transfer.fsckObjects does

[Eric Sunshine]

On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason
 wrote:
> The existing documentation led the user to believe that all we were
> doing were basic reachability sanity checks, but that hasn't been true
> for a very long time. Update the description to match reality, and
> note the caveat that there's a quarantine for accepting pushes, but
> not for fetching.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -3341,8 +3341,16 @@ transfer.fsckObjects::
>  When set, the fetch or receive will abort in the case of a malformed
> +object or a link to a nonexistent object. In addition, various other
> +issues are checked for, including legacy issues (see `fsck.`),
> +and potential security issues like the existence of a `.GIT` directory
> +(see the release notes for v2.2.1 for details). Other sanity and
> +security checks may be added in future releases.
> ++
> +On the receiving side, failing fsckObjects will make those objects
> +unreachable, see "QUARANTINE ENVIRONMENT" in
> +linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
> +instead be left unreferenced in the repository.

This version looks better. Thanks.

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Eric Sunshine]

On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason
 wrote:
> The documentation for the fsck. and receive.fsck.
> variables was mostly duplicated in two places, with fsck.
> making no mention of the corresponding receive.fsck., and the
> same for fsck.skipList.
> [...]
> Rectify this situation by describing the feature in general terms
> under the fsck.* documentation, and make the receive.fsck.*
> documentation refer to those variables instead.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1554,23 +1554,42 @@ filter..smudge::
> +Depending on the circumstances it might be better to use
> +`fsck.skipList` instead to explicitly whitelist those objects that
> +have issues, otherwise new occurrences of the same issue will be
> +hidden going forward, although that's unlikely to happen in practice
> +unless someone is being deliberately malicious.

Is it worth mentioning buggy tools and/or other buggy Git
implementations as sources?

>  fsck.skipList::
> The path to a sorted list of object names (i.e. one SHA-1 per
> -   line) that are known to be broken in a non-fatal way and should
> -   be ignored. This feature is useful when an established project
> -   should be accepted despite early commits containing errors that
> -   can be safely ignored such as invalid committer email addresses.
> -   Note: corrupt objects cannot be skipped with this setting.
> +   line) that are known to be broken in a non-fatal way and
> +   should be ignored. This feature is useful when an established
> +   project should be accepted despite early commits containing
> +   errors that can be safely ignored such as invalid committer
> +   email addresses. Note: corrupt objects cannot be skipped with
> +   this setting.

Not sure why this paragraph got re-wrapped (without, as far as I can
see, any other changes), thus making the patch unnecessarily noisy.

> +Like `fsck.` this variable has a corresponding
> +`receive.fsck.skipList` variant.

Re: [PATCH v2 1/5] config doc: don't describe *.fetchObjects twice

[Eric Sunshine]

On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason
 wrote:
> Let's not duplicate the description of what *.fsckObjects does twice.

Nit: "duplicate" and "twice" smell redundant.

> instead let's refer to transfer.fsckObjects from both fetch.* and
> receive.*.

s/instead/Instead/

Perhaps the above paragraph can be rewritten:

Refer readers of fetch.fsckObjects and receive.fsckObjects to
transfer.fsckObjects instead of repeating the description at each
location.

(Not at all worth a re-roll.)

> I don't think this description of it makes much sense, but for now I'm
> just moving the existing documentation around. Making it better will
> be done in a later patch.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 

[RFC PATCH 3/3] usage: translate the "error: "-prefix and others

[Martin Ågren]

Translate the "error: ", "fatal: ", "usage: " and "warning: " prefixes
that we use for reporting that kind of information.

Do not translate "BUG: ". We tend to prefer the messages themselves to
be non-translated (and they're not supposed to ever appear anyway) so it
makes sense to let the prefix be nontranslated, too.

Signed-off-by: Martin Ågren 
---
This change breaks several tests under GETTEXT_POISON, e.g. t6030 which
does this:

git bisect skip > my_bisect_log.txt 2>&1 &&
grep "warning" my_bisect_log.txt

 usage.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/usage.c b/usage.c
index 6a5669922f..7709cb22e7 100644
--- a/usage.c
+++ b/usage.c
@@ -39,24 +39,24 @@ void vreportf(const char *prefix, const char *err, va_list 
params)
 
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
-   vreportf("usage: ", err, params);
+   vreportf(_("usage: "), err, params);
exit(129);
 }
 
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-   vreportf("fatal: ", err, params);
+   vreportf(_("fatal: "), err, params);
exit(128);
 }
 
 static void error_builtin(const char *err, va_list params)
 {
-   vreportf("error: ", err, params);
+   vreportf(_("error: "), err, params);
 }
 
 static void warn_builtin(const char *warn, va_list params)
 {
-   vreportf("warning: ", warn, params);
+   vreportf(_("warning: "), warn, params);
 }
 
 static int die_is_recursing_builtin(void)
-- 
2.17.0.1181.g093e983b05

[RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Martin Ågren]

advice.c contains a useful code snippet which takes a multi-line string
and prints the lines, prefixing and suffixing each line with two
constant strings. This was originally added in 23cb5bf3b3 (i18n of
multi-line advice messages, 2011-12-22) to produce such output:

hint: some multi-line advice
hint: prefixed with "hint: "

The prefix is actually colored after 960786e761 (push: colorize errors,
2018-04-21) and each line has a suffix for resetting the color.

The next commit will teach the same "prefix all the lines"-trick to the
code that produces, e.g., "warning: "-messages. In preparation for that,
extract the code for printing the individual lines and expose it through
git-compat-util.h.

Signed-off-by: Martin Ågren 
---
I'm open for suggestions on the naming of `prefix_suffix_lines()`...

 git-compat-util.h |  8 
 advice.c  | 18 --
 usage.c   | 18 ++
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f9e4c5f9bc..23445f7ab9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -415,6 +415,14 @@ static inline char *git_find_last_dir_sep(const char *path)
 struct strbuf;
 
 /* General helper functions */
+
+/*
+ * Write the message to the file, prefixing and suffixing
+ * each line with `prefix` resp. `suffix`.
+ */
+void prefix_suffix_lines(FILE *f, const char *prefix,
+const char *message, const char *suffix);
+
 extern void vreportf(const char *prefix, const char *err, va_list params);
 extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
diff --git a/advice.c b/advice.c
index 370a56d054..ffb29e7ef4 100644
--- a/advice.c
+++ b/advice.c
@@ -79,24 +79,22 @@ static struct {
 
 void advise(const char *advice, ...)
 {
+   struct strbuf prefix = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
va_list params;
-   const char *cp, *np;
+
+   strbuf_addf(, _("%shint: "),
+   advise_get_color(ADVICE_COLOR_HINT));
 
va_start(params, advice);
strbuf_vaddf(, advice, params);
va_end(params);
 
-   for (cp = buf.buf; *cp; cp = np) {
-   np = strchrnul(cp, '\n');
-   fprintf(stderr, _("%shint: %.*s%s\n"),
-   advise_get_color(ADVICE_COLOR_HINT),
-   (int)(np - cp), cp,
-   advise_get_color(ADVICE_COLOR_RESET));
-   if (*np)
-   np++;
-   }
+   prefix_suffix_lines(stderr, prefix.buf, buf.buf,
+   advise_get_color(ADVICE_COLOR_RESET));
+
strbuf_release();
+   strbuf_release();
 }
 
 int git_default_advice_config(const char *var, const char *value)
diff --git a/usage.c b/usage.c
index cdd534c9df..80f9c1d14b 100644
--- a/usage.c
+++ b/usage.c
@@ -6,6 +6,24 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
+void prefix_suffix_lines(FILE *f,
+const char *prefix,
+const char *message,
+const char *suffix)
+{
+   const char *cp, *np;
+
+   for (cp = message; *cp; cp = np) {
+   np = strchrnul(cp, '\n');
+   fprintf(f, "%s%.*s%s\n",
+   prefix,
+   (int)(np - cp), cp,
+   suffix);
+   if (*np)
+   np++;
+   }
+}
+
 void vreportf(const char *prefix, const char *err, va_list params)
 {
char msg[4096];
-- 
2.17.0.1181.g093e983b05

[RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

Teach `vreportf()` to prefix all lines with the given prefix, not only
the first line. This matches how "hint: " is being shown, and affects
"error: ", "fatal: ", "usage: ", "warning: " and "BUG: " (as well as any
out-of-tree and future users).

Note that we need to adjust quite a few tests as a result of this
change. All of these changes are because we obviously need to prefix
more lines in various "expect"-files -- except for one instance of a
trailing empty line that disappears with this commit (see t7609). This
is a very minor change, and arguably a good one.

Signed-off-by: Martin Ågren 
---
Looking back at this, I wonder if the opposite approach would be better,
that is, making `advise()` use `vreportf()` after teaching the latter
the multi-line trick.

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh   |  2 +-
 t/t1600-index.sh |  6 ++---
 t/t3600-rm.sh| 36 -
 t/t5512-ls-remote.sh |  6 ++---
 t/t7607-merge-overwrite.sh   |  6 ++---
 t/t7609-merge-co-error-msgs.sh   | 39 ++--
 usage.c  |  2 +-
 8 files changed, 51 insertions(+), 52 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh 
b/t/t1011-read-tree-sparse-checkout.sh
index 0c6f48f302..31b0702e6c 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
worktree' '
test_must_fail git checkout top 2>actual &&
cat >expected <<\EOF &&
 error: The following untracked working tree files would be overwritten by 
checkout:
-   sub/added
-   sub/addedtoo
-Please move or remove them before you switch branches.
+error: sub/added
+error: sub/addedtoo
+error: Please move or remove them before you switch branches.
 Aborting
 EOF
test_i18ncmp expected actual
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4ee009da66..80d35087b7 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -11,7 +11,7 @@ test_did_you_mean ()
sq="'" &&
cat >expected <<-EOF &&
fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
-   Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
+   fatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
EOF
test_cmp expected error
 }
diff --git a/t/t1600-index.sh b/t/t1600-index.sh
index c4422312f4..39a707c94a 100755
--- a/t/t1600-index.sh
+++ b/t/t1600-index.sh
@@ -16,7 +16,7 @@ test_expect_success 'bogus GIT_INDEX_VERSION issues warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: GIT_INDEX_VERSION set, but the value is 
invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
@@ -30,7 +30,7 @@ test_expect_success 'out of bounds GIT_INDEX_VERSION issues 
warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: GIT_INDEX_VERSION set, but the value is 
invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
@@ -54,7 +54,7 @@ test_expect_success 'out of bounds index.version issues 
warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: index.version set, but the value is invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b8fbdefcdc..cd4a10df2d 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -771,10 +771,10 @@ test_expect_success 'setup for testing rm messages' '
 test_expect_success 'rm files with different staged content' '
cat >expect <<-\EOF &&
error: the following files have staged content different from both the
-   file and the HEAD:
-   bar.txt
-   foo.txt
-   (use -f to force removal)
+   error: file and the HEAD:
+   error: bar.txt
+   error: foo.txt
+   error: (use -f to force removal)
EOF
echo content1 >foo.txt &&
echo content1 >bar.txt &&
@@ -785,9 +785,9 @@ test_expect_success 'rm files with different staged 
content' '
 test_expect_success 'rm files with different staged content without hints' '
cat >expect <<-\EOF &&
error: the following files have staged content different from both the
-   file and the HEAD:
-  

[RFC PATCH 0/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 25 May 2018 at 11:14, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>>> +warning("the '-l' option is an alias for 
>>> '--create-reflog' and");
>>> +warning("has no effect in list mode. This option will 
>>> soon be");
>>> +warning("removed and you should omit it (or use 
>>> '--list' instead).");
>
> By the way, this is one of these times when I feel that we should
> have a better multi-line message support in die/error/warning/info
> functions.  Ideally, I should be able to write
[...]
> warning(_("the '-l' option is an alias for '--create-reflog' and\n"
>   "has no effect in list mode, This option will soon be\n"
>   "removed and you should omit it (or use '--list' 
> instead)."));
[...]
> and warning() would:
>
>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>message;
>
>  - chomp that into lines at '\n' boundary; and
>
>  - give each of these lines with _("warning: ") prefixed.
>
> That way, translators can choose to make the resulting message to
> different number of lines from the original easily.

How about something like this? The first two patches implement the
above three points, except for the translation of "warning: ".

The third patch is the main reason this is marked RFC. It translates
"warning: " and similar, and breaks quite a few tests under
GETTEXT_POISON since we grep for, e.g., "warning" on stderr. I could
annotate those tests, but since I'm running out of time anyway, I
thought I'd post this as a heads-up of "I'm looking into this".

I do wonder: If our tests rely on grepping for "warning" (for pretty
good reasons), how many scripts out there do something similar (for
maybe-not-so-good reasons, but still)? Do we want to avoid breaking
them?

Also left to do is to convert any existing lego-ing users to the
single-string form that Junio wished for above.

Martin

Martin Ågren (3):
  usage: extract `prefix_suffix_lines()` from `advise()`
  usage: prefix all lines in `vreportf()`, not just the first
  usage: translate the "error: "-prefix and others

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh   |  2 +-
 t/t1600-index.sh |  6 ++---
 t/t3600-rm.sh| 36 -
 t/t5512-ls-remote.sh |  6 ++---
 t/t7607-merge-overwrite.sh   |  6 ++---
 t/t7609-merge-co-error-msgs.sh   | 39 ++--
 git-compat-util.h|  8 ++
 advice.c | 18 ++---
 usage.c  | 28 
 10 files changed, 89 insertions(+), 66 deletions(-)

-- 
2.17.0.1181.g093e983b05

Re: [PATCH v2 4/5] config doc: mention future aspirations for transfer.fsckObjects

[Christian Couder]

On Fri, May 25, 2018 at 9:28 PM, Ævar Arnfjörð Bjarmason
 wrote:
>
> Jeff King has said on at least a couple of occasions (at least one of
> Let's note that in the documentation so we don't seem to be claiming
> that this is by design. A previous version of this change called the
> current behavior a "bug", that's probably too strong a claim, but I
> don't think anyone would dislike a hypothetical local quarantine
> patch, so let's we might change this in the future.

Maybe: s/so let's we might/so let's say we might/

Re: sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Stefan Beller]

On Fri, May 25, 2018 at 5:28 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Thu, May 17 2018, Junio C Hamano wrote:
>
>> * sb/submodule-move-nested (2018-03-29) 6 commits
>>   (merged to 'next' on 2018-04-25 at 86b177433a)
>>  + submodule: fixup nested submodules after moving the submodule
>>  + submodule-config: remove submodule_from_cache
>>  + submodule-config: add repository argument to submodule_from_{name, path}
>>  + submodule-config: allow submodule_free to handle arbitrary repositories
>>  + grep: remove "repo" arg from non-supporting funcs
>>  + submodule.h: drop declaration of connect_work_tree_and_git_dir
>>
>>  Moving a submodule that itself has submodule in it with "git mv"
>>  forgot to make necessary adjustment to the nested sub-submodules;
>>  now the codepath learned to recurse into the submodules.
>
> I didn't spot this earlier because I don't test this a lot, but I've
> bisected the following breakage down to da62f786d2 ("submodule: fixup
> nested submodules after moving the submodule", 2018-03-28) (and manually
> confirmed by reverting). On Linux both Debian & CentOS I get tests 3 and
> 4 failing with:
>
>  GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
>
> -v -x output follows:
>
> expecting success:
> mkdir submodule &&
> (cd submodule &&
> git init &&
> echo a >a &&
> git add . &&
> git commit -ma
> ) &&
> mkdir super &&
> (cd super &&
> git init &&
> git submodule add ../submodule &&
> git submodule add ../submodule a &&
> git commit -m "add as submodule and as a" &&
> git mv a b &&
> git commit -m "move a to b"
> )

when you add a test_pause here and dump the
state of the setup, then it can be observed that when the fsmonitor is active
the last commit is different; without fsmonitor the moved gitlink and the change
to the .gitmodules file is part of the commit, i.e.

$ git -C super show
commit d3d90b70a01bd17d026f75a803c8b65f5903a7c0 (HEAD -> master)
Author: A U Thor 
Date:   Fri May 25 19:21:58 2018 +

move a to b

diff --git a/.gitmodules b/.gitmodules
index 3f4d474..6149210 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -2,5 +2,5 @@
  path = submodule
  url = ../submodule
 [submodule "a"]
- path = a
+ path = b
  url = ../submodule
diff --git a/a b/b
similarity index 100%
rename from a
rename to b
When running with the fsmonitor:

$ git -C super show
commit 57022a92acf46f303498c045440ec099cbc35a2d (HEAD -> master)
Author: A U Thor 
Date:   Fri May 25 19:22:52 2018 +

move a to b

diff --git a/a b/b
similarity index 100%
rename from a
rename to b
$ git -C super diff
diff --git a/.gitmodules b/.gitmodules
index 3f4d474..6149210 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -2,5 +2,5 @@
  path = submodule
  url = ../submodule
 [submodule "a"]
- path = a
+ path = b
  url = ../submodule

This hints at a problem with git commit;

I tried adding test_tick, to unconfuse the fsmonitor, but that doesn't help,
digging further, the problem is in the git mv command, which fails to
add the change in
.gitmodules to the index.

Adding the verbose flag to stage_updated_gitmodules() that is called by
git-mv very late in the game, such that

void stage_updated_gitmodules(struct index_state *istate)
{
trace_printf("staging .gitmodules files");
if (add_file_to_index(istate, GITMODULES_FILE, ADD_CACHE_VERBOSE))
die(_("staging updated .gitmodules failed"));
}

We would get a message if the .gitmodules file is staged correctly, as
add_file_to_index() that calls add_to_index that would print

if (verbose && !was_same)
printf("add '%s'\n", path);

I could not see that message, so I suspect, that there is something
racy.

Will debug further.

[PATCH v2 3/5] config doc: elaborate on what transfer.fsckObjects does

[Ævar Arnfjörð Bjarmason]

The existing documentation led the user to believe that all we were
doing were basic reachability sanity checks, but that hasn't been true
for a very long time. Update the description to match reality, and
note the caveat that there's a quarantine for accepting pushes, but
not for fetching.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/config.txt | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af7311e73f..71b3805b4e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3341,8 +3341,16 @@ transfer.fsckObjects::
Defaults to false.
 +
 When set, the fetch or receive will abort in the case of a malformed
-object or a broken link. The result of an abort are only dangling
-objects.
+object or a link to a nonexistent object. In addition, various other
+issues are checked for, including legacy issues (see `fsck.`),
+and potential security issues like the existence of a `.GIT` directory
+(see the release notes for v2.2.1 for details). Other sanity and
+security checks may be added in future releases.
++
+On the receiving side, failing fsckObjects will make those objects
+unreachable, see "QUARANTINE ENVIRONMENT" in
+linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
+instead be left unreferenced in the repository.
 
 transfer.hideRefs::
String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.17.0.290.gded63e768a

[PATCH v2 5/5] fetch: implement fetch.fsck.*

[Ævar Arnfjörð Bjarmason]

Implement support for fetch.fsck.* corresponding with the existing
receive.fsck.*. This allows for pedantically cloning repositories with
specific issues without turning off fetch.fsckObjects.

One such repository is https://github.com/robbyrussell/oh-my-zsh.git
which before this change will emit this error when cloned with
fetch.fsckObjects:

error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: 
contains zero-padded file modes
fatal: Error in object
fatal: index-pack failed

Now with fetch.fsck.zeroPaddedFilemode=warn we'll warn about that
issue, but the clone will succeed:

warning: object 2b7227859263b6aabcc28355b0b994995b7148b6: 
zeroPaddedFilemode: contains zero-padded file modes
warning: object a18c4d13c2a5fa2d4ecd5346c50e119b999b807d: 
zeroPaddedFilemode: contains zero-padded file modes
warning: object 84df066176c8da3fd59b13731a86d90f4f1e5c9d: 
zeroPaddedFilemode: contains zero-padded file modes

The motivation for this is to be able to turn on fetch.fsckObjects
globally across a fleet of computers but still be able to manually
clone various legacy repositories by either white-listing specific
issues, or better yet whitelist specific objects.

The use of --git-dir=* instead of -C in the tests could be considered
somewhat archaic, but the tests I'm adding here are duplicating the
corresponding receive.* tests with as few changes as possible.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/config.txt| 21 +++
 fetch-pack.c| 32 +--
 t/t5504-fetch-receive-strict.sh | 46 +
 3 files changed, 92 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f97f21c022..f69cd31ad0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1426,6 +1426,16 @@ fetch.fsckObjects::
checked. Defaults to false. If not set, the value of
`transfer.fsckObjects` is used instead.
 
+fetch.fsck.::
+   Acts like `fsck.`, but is used by
+   linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
+   the `fsck.` documentation for details.
+
+fetch.fsck.skipList::
+   Acts like `fsck.skipList`, but is used by
+   linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
+   the `fsck.skipList` documentation for details.
+
 fetch.unpackLimit::
If the number of objects fetched over the Git native
transfer is below this
@@ -1561,9 +1571,10 @@ fsck.::
repositories containing such data.
 +
 Setting `fsck.` will be picked up by linkgit:git-fsck[1], but
-to accept pushes of such data set `receive.fsck.` instead. The
-rest of the documentation discusses `fsck.*` for brevity, but the same
-applies for the corresponding `receive.fsck.*` variables.
+to accept pushes of such data set `receive.fsck.` instead, or
+to clone or fetch it set `fetch.fsck.`. The rest of the
+documentation discusses `fsck.*` for brevity, but the same applies for
+the corresponding `receive.fsck.*` and `fetch..*`. variables.
 +
 When `fsck.` is set, errors can be switched to warnings and
 vice versa by configuring the `fsck.` setting where the
@@ -1588,8 +1599,8 @@ fsck.skipList::
email addresses. Note: corrupt objects cannot be skipped with
this setting.
 +
-Like `fsck.` this variable has a corresponding
-`receive.fsck.skipList` variant.
+Like `fsck.` this variable has corresponding
+`receive.fsck.skipList` and `fetch.fsck.skipList` variants.
 
 gc.aggressiveDepth::
The depth parameter used in the delta compression
diff --git a/fetch-pack.c b/fetch-pack.c
index 490c38f833..9e4282788e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -19,6 +19,7 @@
 #include "sha1-array.h"
 #include "oidset.h"
 #include "packfile.h"
+#include "fsck.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -33,6 +34,7 @@ static int agent_supported;
 static int server_supports_filtering;
 static struct lock_file shallow_lock;
 static const char *alternate_shallow_file;
+static struct strbuf fsck_msg_types = STRBUF_INIT;
 
 /* Remember to update object flag allocation in object.h */
 #define COMPLETE   (1U << 0)
@@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,
 */
argv_array_push(, "--fsck-objects");
else
-   argv_array_push(, "--strict");
+   argv_array_pushf(, "--strict%s",
+fsck_msg_types.buf);
}
 
cmd.in = demux.out;
@@ -1409,6 +1412,31 @@ static struct ref *do_fetch_pack_v2(struct 
fetch_pack_args *args,
return ref;
 }
 
+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)
+{
+   if (strcmp(var, "fetch.fsck.skiplist") == 0) {
+   const char *path;
+
+   if (git_config_pathname(, var, value))
+   

[PATCH v2 0/5] fsck: doc fixes & fetch.fsck.* implementation

[Ævar Arnfjörð Bjarmason]

This should address all the comments to v1. Inter-diff:

1: a9cd795db5 ! 1: 3d61e44cb8 config doc: don't describe *.fetchObjects 
twice
@@ -1,10 +1,6 @@
 Author: Ævar Arnfjörð Bjarmason 
 
 config doc: don't describe *.fetchObjects twice
-
-Change the copy/pasted description of the fetch.fsckObjects and
-receive.fsckObjects variables to refer to transfer.fsckObjects
-instead.
 
 Let's not duplicate the description of what *.fsckObjects does 
twice.
 instead let's refer to transfer.fsckObjects from both fetch.* and
2: 637c2d4241 ! 2: 9683fd2ec6 config doc: unify the description of fsck.* 
and receive.fsck.*
@@ -64,21 +64,21 @@
 +unless someone is being deliberately malicious.
  
  fsck.skipList::
--  The path to a sorted list of object names (i.e. one SHA-1 per
+   The path to a sorted list of object names (i.e. one SHA-1 per
 -  line) that are known to be broken in a non-fatal way and should
 -  be ignored. This feature is useful when an established project
 -  should be accepted despite early commits containing errors that
 -  can be safely ignored such as invalid committer email addresses.
 -  Note: corrupt objects cannot be skipped with this setting.
-+  Like `fsck.` this variable has a corresponding
-+  `receive.fsck.skipList` variant.
++  line) that are known to be broken in a non-fatal way and
++  should be ignored. This feature is useful when an established
++  project should be accepted despite early commits containing
++  errors that can be safely ignored such as invalid committer
++  email addresses. Note: corrupt objects cannot be skipped with
++  this setting.
 ++
-+The path to a sorted list of object names (i.e. one SHA-1 per line)
-+that are known to be broken in a non-fatal way and should be
-+ignored. This feature is useful when an established project should be
-+accepted despite early commits containing errors that can be safely
-+ignored such as invalid committer email addresses. Note: corrupt
-+objects cannot be skipped with this setting.
++Like `fsck.` this variable has a corresponding
++`receive.fsck.skipList` variant.
  
  gc.aggressiveDepth::
The depth parameter used in the delta compression
3: 55dc555196 < -:  --- config doc: elaborate on what 
transfer.fsckObjects does
-:  --- > 3: 8e9646a6ce config doc: elaborate on what 
transfer.fsckObjects does
-:  --- > 4: 2b3aafdfde config doc: mention future aspirations for 
transfer.fsckObjects
4: 13f4d994c0 ! 5: be32b19696 fetch: implement fetch.fsck.*
@@ -67,16 +67,16 @@
  When `fsck.` is set, errors can be switched to warnings and
  vice versa by configuring the `fsck.` setting where the
 @@
- unless someone is being deliberately malicious.
- 
- fsck.skipList::
--  Like `fsck.` this variable has a corresponding
--  `receive.fsck.skipList` variant.
-+  Like `fsck.` this variable has corresponding
-+  `receive.fsck.skipList` and `fetch.fsck.skipList` variants.
+   email addresses. Note: corrupt objects cannot be skipped with
+   this setting.
  +
- The path to a sorted list of object names (i.e. one SHA-1 per line)
- that are known to be broken in a non-fatal way and should be
+-Like `fsck.` this variable has a corresponding
+-`receive.fsck.skipList` variant.
++Like `fsck.` this variable has corresponding
++`receive.fsck.skipList` and `fetch.fsck.skipList` variants.
+ 
+ gc.aggressiveDepth::
+   The depth parameter used in the delta compression
 
 diff --git a/fetch-pack.c b/fetch-pack.c
 --- a/fetch-pack.c

The "mention future aspirations for transfer.fsckObjects" patch is
new. I've split up the "we're probably going to quarantine fetches
too" part of this.

Ævar Arnfjörð Bjarmason (5):
  config doc: don't describe *.fetchObjects twice
  config doc: unify the description of fsck.* and receive.fsck.*
  config doc: elaborate on what transfer.fsckObjects does
  config doc: mention future aspirations for transfer.fsckObjects
  fetch: implement fetch.fsck.*

 Documentation/config.txt| 112 
 fetch-pack.c|  32 -
 t/t5504-fetch-receive-strict.sh |  46 +
 3 files changed, 148 insertions(+), 42 deletions(-)

-- 
2.17.0.290.gded63e768a

[PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Ævar Arnfjörð Bjarmason]

The documentation for the fsck. and receive.fsck.
variables was mostly duplicated in two places, with fsck.
making no mention of the corresponding receive.fsck., and the
same for fsck.skipList.

I spent quite a lot of time today wondering why setting the
fsck. variant wasn't working to clone a legacy repository (not
that that would have worked anyway, but a subsequent patch implements
fetch.fsck.).

Rectify this situation by describing the feature in general terms
under the fsck.* documentation, and make the receive.fsck.*
documentation refer to those variables instead.

This documentation was initially added in 2becf00ff7 ("fsck: support
demoting errors to warnings", 2015-06-22) and 4b55b9b479 ("fsck:
document the new receive.fsck. options", 2015-06-22).

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 623dffd198..af7311e73f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1554,23 +1554,42 @@ filter..smudge::
linkgit:gitattributes[5] for details.
 
 fsck.::
-   Allows overriding the message type (error, warn or ignore) of a
-   specific message ID such as `missingEmail`.
-+
-For convenience, fsck prefixes the error/warning with the message ID,
-e.g.  "missingEmail: invalid author/committer line - missing email" means
-that setting `fsck.missingEmail = ignore` will hide that issue.
-+
-This feature is intended to support working with legacy repositories
-which cannot be repaired without disruptive changes.
+   During fsck git may find issues with legacy data which
+   wouldn't be generated by current versions of git, and which
+   wouldn't be sent over the wire if `transfer.fsckObjects` was
+   set. This feature is intended to support working with legacy
+   repositories containing such data.
++
+Setting `fsck.` will be picked up by linkgit:git-fsck[1], but
+to accept pushes of such data set `receive.fsck.` instead. The
+rest of the documentation discusses `fsck.*` for brevity, but the same
+applies for the corresponding `receive.fsck.*` variables.
++
+When `fsck.` is set, errors can be switched to warnings and
+vice versa by configuring the `fsck.` setting where the
+`` is the fsck message ID and the value is one of `error`,
+`warn` or `ignore`. For convenience, fsck prefixes the error/warning
+with the message ID, e.g. "missingEmail: invalid author/committer line
+- missing email" means that setting `fsck.missingEmail = ignore` will
+hide that issue.
++
+Depending on the circumstances it might be better to use
+`fsck.skipList` instead to explicitly whitelist those objects that
+have issues, otherwise new occurrences of the same issue will be
+hidden going forward, although that's unlikely to happen in practice
+unless someone is being deliberately malicious.
 
 fsck.skipList::
The path to a sorted list of object names (i.e. one SHA-1 per
-   line) that are known to be broken in a non-fatal way and should
-   be ignored. This feature is useful when an established project
-   should be accepted despite early commits containing errors that
-   can be safely ignored such as invalid committer email addresses.
-   Note: corrupt objects cannot be skipped with this setting.
+   line) that are known to be broken in a non-fatal way and
+   should be ignored. This feature is useful when an established
+   project should be accepted despite early commits containing
+   errors that can be safely ignored such as invalid committer
+   email addresses. Note: corrupt objects cannot be skipped with
+   this setting.
++
+Like `fsck.` this variable has a corresponding
+`receive.fsck.skipList` variant.
 
 gc.aggressiveDepth::
The depth parameter used in the delta compression
@@ -2849,26 +2868,16 @@ receive.fsckObjects::
`transfer.fsckObjects` is used instead.
 
 receive.fsck.::
-   When `receive.fsckObjects` is set to true, errors can be switched
-   to warnings and vice versa by configuring the `receive.fsck.`
-   setting where the `` is the fsck message ID and the value
-   is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes
-   the error/warning with the message ID, e.g. "missingEmail: invalid
-   author/committer line - missing email" means that setting
-   `receive.fsck.missingEmail = ignore` will hide that issue.
-+
-This feature is intended to support working with legacy repositories
-which would not pass pushing when `receive.fsckObjects = true`, allowing
-the host to accept repositories with certain known issues but still catch
-other issues.
+   Acts like `fsck.`, but is used by
+   linkgit:git-receive-pack[1] instead of
+   linkgit:git-fsck[1]. See the `fsck.` documentation for
+   details.
 
 receive.fsck.skipList::
-

[PATCH v2 1/5] config doc: don't describe *.fetchObjects twice

[Ævar Arnfjörð Bjarmason]

Let's not duplicate the description of what *.fsckObjects does twice.
instead let's refer to transfer.fsckObjects from both fetch.* and
receive.*.

I don't think this description of it makes much sense, but for now I'm
just moving the existing documentation around. Making it better will
be done in a later patch.

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 84e2891aed..623dffd198 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1422,10 +1422,9 @@ fetch.recurseSubmodules::
 
 fetch.fsckObjects::
If it is set to true, git-fetch-pack will check all fetched
-   objects. It will abort in the case of a malformed object or a
-   broken link. The result of an abort are only dangling objects.
-   Defaults to false. If not set, the value of `transfer.fsckObjects`
-   is used instead.
+   objects. See `transfer.fsckObjects` for what's
+   checked. Defaults to false. If not set, the value of
+   `transfer.fsckObjects` is used instead.
 
 fetch.unpackLimit::
If the number of objects fetched over the Git native
@@ -2845,10 +2844,9 @@ receive.certNonceSlop::
 
 receive.fsckObjects::
If it is set to true, git-receive-pack will check all received
-   objects. It will abort in the case of a malformed object or a
-   broken link. The result of an abort are only dangling objects.
-   Defaults to false. If not set, the value of `transfer.fsckObjects`
-   is used instead.
+   objects. See `transfer.fsckObjects` for what's checked.
+   Defaults to false. If not set, the value of
+   `transfer.fsckObjects` is used instead.
 
 receive.fsck.::
When `receive.fsckObjects` is set to true, errors can be switched
@@ -3332,6 +3330,10 @@ transfer.fsckObjects::
When `fetch.fsckObjects` or `receive.fsckObjects` are
not set, the value of this variable is used instead.
Defaults to false.
++
+When set, the fetch or receive will abort in the case of a malformed
+object or a broken link. The result of an abort are only dangling
+objects.
 
 transfer.hideRefs::
String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.17.0.290.gded63e768a

[PATCH v2 4/5] config doc: mention future aspirations for transfer.fsckObjects

[Ævar Arnfjörð Bjarmason]

Jeff King has said on at least a couple of occasions (at least one of
which may have been in person over beer) that leaving corrupt objects
in the local object store after a "fetch" that fails
transfer.fsckObjects should be fixed, and we should have something
like the server-side quarantine environment on the client-side.

Let's note that in the documentation so we don't seem to be claiming
that this is by design. A previous version of this change called the
current behavior a "bug", that's probably too strong a claim, but I
don't think anyone would dislike a hypothetical local quarantine
patch, so let's we might change this in the future.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/config.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 71b3805b4e..f97f21c022 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3350,7 +3350,9 @@ security checks may be added in future releases.
 On the receiving side, failing fsckObjects will make those objects
 unreachable, see "QUARANTINE ENVIRONMENT" in
 linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
-instead be left unreferenced in the repository.
+instead be left unreferenced in the repository. That difference in
+behavior should not be relied upon. In the future, such objects may be
+quarantined for "fetch" as well.
 
 transfer.hideRefs::
String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.17.0.290.gded63e768a

Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Ævar Arnfjörð Bjarmason]

On Fri, May 25 2018, Duy Nguyen wrote:

> On Fri, May 25, 2018 at 10:12 AM, Junio C Hamano  wrote:
>>> +Currently this is used by linkgit:git-checkout[1] when 'git checkout
>>> +' will checkout the '' branch on another remote,
>>> +and by linkgit:git-worktree[1] when 'git worktree add' refers to a
>>> +remote branch. This setting might be used for other checkout-like
>>> +commands or functionality in the future.
>>
>> Hmph, that is an interesting direction.  You foresee that you'll
>> have a single repository with multiple remotes to grab and share
>> objects from different people working on the same project, and use
>> multiple worktrees to work on different branches, yet you are happy
>> to declare that each worktree is to work with one particular remote?
>>
>> We'd need a per-worktree config file to make it work, I guess, or
>> a three-level checkout.$worktree_id.defaultRemote configuration
>> variable, perhaps?
>
> I still plan to revisit per-worktree config support [1] at some point
> and finish it off. Or is it decided that we don't need a generic
> mechanism and will add a new level like this for each config var that
> is per-worktree? If defaultRemote sets a precedence, then it'll be the
> way to go from now on or we'll have another mess when some config does
> "foo.$worktree.bar" while others use per-worktree config files.

What do you think about including worktree in this at this time? I'm not
very familiar with it and just included it because I worked my way
backwards from the DWIM function, but I could exclude it if you think
it's too much trouble, i.e. if you'd like to hold out for some facility
like the per-worktree config Junio mentioned.

Re: sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Stefan Beller]

Hi Ævar,

On Fri, May 25, 2018 at 5:28 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Thu, May 17 2018, Junio C Hamano wrote:
>
>> * sb/submodule-move-nested (2018-03-29) 6 commits
>>   (merged to 'next' on 2018-04-25 at 86b177433a)
>>  + submodule: fixup nested submodules after moving the submodule
>>  + submodule-config: remove submodule_from_cache
>>  + submodule-config: add repository argument to submodule_from_{name, path}
>>  + submodule-config: allow submodule_free to handle arbitrary repositories
>>  + grep: remove "repo" arg from non-supporting funcs
>>  + submodule.h: drop declaration of connect_work_tree_and_git_dir
>>
>>  Moving a submodule that itself has submodule in it with "git mv"
>>  forgot to make necessary adjustment to the nested sub-submodules;
>>  now the codepath learned to recurse into the submodules.
>
> I didn't spot this earlier because I don't test this a lot, but I've
> bisected the following breakage down to da62f786d2 ("submodule: fixup
> nested submodules after moving the submodule", 2018-03-28) (and manually
> confirmed by reverting). On Linux both Debian & CentOS I get tests 3 and
> 4 failing with:
>
>  GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh

I can reproduce this. I'll look into it.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Fri, May 25, 2018 at 06:14:16PM +0900, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> >> -  warning("the '-l' alias for '--create-reflog' is deprecated;");
> >> -  warning("it will be removed in a future version of Git");
> >> +  if (list) {
> >> +  warning("the '-l' option is an alias for 
> >> '--create-reflog' and");
> >> +  warning("has no effect in list mode. This option will 
> >> soon be");
> >> +  warning("removed and you should omit it (or use 
> >> '--list' instead).");
> >> +  } else {
> >> +  warning("the '-l' alias for '--create-reflog' is 
> >> deprecated;");
> >> +  warning("it will be removed in a future version of 
> >> Git");
> >> +  }
> 
> By the way, this is one of these times when I feel that we should
> have a better multi-line message support in die/error/warning/info
> functions.  Ideally, I should be able to write
> 
>   warning(_("the '-l' option is an alias for '--create-reflog' and\n"
> "has no effect in list mode, This option will soon be\n"
> "removed and you should omit it (or use '--list' instead)."));
> 
> and warning() would:
> 
>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>message;
> 
>  - chomp that into lines at '\n' boundary; and
> 
>  - give each of these lines with _("warning: ") prefixed.
> 
> That way, translators can choose to make the resulting message to
> different number of lines from the original easily.

Yep, I totally agree. In past discussions I was thinking mostly of
the pain of writing these multi-line messages. But I imagine it is
absolute hell for translators, and we should fix it for that reason.

(Also, I guess this message probably ought to be marked for
translation).

-Peff

Re: [PATCH 1/2] diff: turn --ita-invisible-in-index on by default

[Duy Nguyen]

On Fri, May 25, 2018 at 6:43 PM, Jonathan Nieder  wrote:
> I think it should be reverted from 'next' because of the unintended
> change to the behavior of "git diff HEAD".

Ah. That is indeed unintended. I still don't know how this change
affects that (but that's probably why it slipped through in the first
place). While there, perhaps you could add a test in t2203 just to
make sure I will not break it again in the next attempt?

I agree that it should be reverted. Or if it's possible to eject it
from next, then it's even better since I will need to fix up the
second patch in that series anyway. Junio?

> Here is what I have applied internally.
>
> -- >8 --
> Subject: Revert "diff: turn --ita-invisible-in-index on by default"
>
> This reverts commit 992118dd95b052bc82a795fd3a8978bea0fe5c0e.  That
> change is promising, since it improves the behavior of "git diff" and
> "git diff --cached" by correctly showing an intent-to-add entry as no
> file.  Unfortunately, though, it breaks "git diff HEAD" in the
> process:
>
> echo hi >new-file
> git add -N new-file
> git diff HEAD
>
> Before: shows addition of the new file
> After: shows no change
>
> Noticed because the new --ita-invisible-in-index default broke a
> script using "git diff --name-only --diff-filter=A master" to get a
> list of added files.  Arguably that script should use diff-index
> instead, which would have sidestepped the issue, but the new behavior
> is unexpected in interactive use as well.
>
> Signed-off-by: Jonathan Nieder 
> ---
>  builtin/diff.c  |  7 ---
>  t/t2203-add-intent.sh   | 40 
>  t/t4011-diff-symlink.sh | 10 --
>  3 files changed, 12 insertions(+), 45 deletions(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index b709b6e984..bfefff3a84 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -352,13 +352,6 @@ int cmd_diff(int argc, const char **argv, const char 
> *prefix)
> rev.diffopt.flags.allow_external = 1;
> rev.diffopt.flags.allow_textconv = 1;
>
> -   /*
> -* Default to intent-to-add entries invisible in the
> -* index. This makes them show up as new files in diff-files
> -* and not at all in diff-cached.
> -*/
> -   rev.diffopt.ita_invisible_in_index = 1;
> -
> if (nongit)
> die(_("Not a git repository"));
> argc = setup_revisions(argc, argv, , NULL);
> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
> index 1d640a33f0..d9fb151d52 100755
> --- a/t/t2203-add-intent.sh
> +++ b/t/t2203-add-intent.sh
> @@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
> git add -N nitfol &&
> git commit -m second &&
> test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
> -   test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | 
> wc -l) = 1 &&
> -   test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
> -   test $(git diff --name-only -- nitfol | wc -l) = 1
> +   test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
> +   test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | 
> wc -l) = 0 &&
> +   test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc 
> -l) = 1
>  '
>
>  test_expect_success 'can commit with an unrelated i-t-a entry in index' '
> @@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
>
> : >dir/bar &&
> git add -N dir/bar &&
> -   git diff --name-only >actual &&
> +   git diff --cached --name-only >actual &&
> echo dir/bar >expect &&
> test_cmp expect actual &&
>
> git write-tree >/dev/null &&
>
> -   git diff --name-only >actual &&
> +   git diff --cached --name-only >actual &&
> echo dir/bar >expect &&
> test_cmp expect actual
>  '
> @@ -186,19 +186,7 @@ test_expect_success 'rename detection finds the right 
> names' '
> cat >expected.3 <<-EOF &&
> 2 .R N... 100644 100644 100644 $hash $hash R100 third   first
> EOF
> -   test_cmp expected.3 actual.3 &&
> -
> -   git diff --stat >actual.4 &&
> -   cat >expected.4 <<-EOF &&
> -first => third | 0
> -1 file changed, 0 insertions(+), 0 deletions(-)
> -   EOF
> -   test_cmp expected.4 actual.4 &&
> -
> -   git diff --cached --stat >actual.5 &&
> -   : >expected.5 &&
> -   test_cmp expected.5 actual.5
> -
> +   test_cmp expected.3 actual.3
> )
>  '
>
> @@ -234,27 +222,15 @@ test_expect_success 'double rename detection in status' 
> '
> )
>  '
>
> -test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '
> -   git reset --hard &&
> -   echo new >new-ita &&
> -   git add -N new-ita &&
> -   git diff 

Re: [PATCH 1/2] diff: turn --ita-invisible-in-index on by default

[Jonathan Nieder]

Hi,

Duy Nguyen wrote:

> $ echo haha > new; git add -N
> $ git diff
> diff --git a/new b/new
> index e69de29..5ad28e2 100644
> --- a/new
> +++ b/new
> @@ -0,0 +1 @@
> +haha
>
> Notice that the diff does not tell you that 'new' is a new file. The
> diff with this patch gives you this
>
> $ git diff
> diff --git a/new b/new
> new file mode 100644
> index 000..5ad28e2
> --- /dev/null
> +++ b/new
> @@ -0,0 +1 @@
> +haha
[...]
> One consequence of this is you can't apply the diff generated with ita
> entries because the diff expects empty files to be already in the
> worktree. This to me does not make sense.
>
> Of course there's other things that also go along this line. Like if
> "git commit" does not add an ita entry, why should it appear in 'git
> diff --cached', which should show you what's to be committed.

Thankds for this context.  That helps a lot.

[...]
> Since this commit is already in 'next', it's too late to update the
> commit message now.

I think it should be reverted from 'next' because of the unintended
change to the behavior of "git diff HEAD".  Here is what I have
applied internally.

-- >8 -- 
Subject: Revert "diff: turn --ita-invisible-in-index on by default"

This reverts commit 992118dd95b052bc82a795fd3a8978bea0fe5c0e.  That
change is promising, since it improves the behavior of "git diff" and
"git diff --cached" by correctly showing an intent-to-add entry as no
file.  Unfortunately, though, it breaks "git diff HEAD" in the
process:

echo hi >new-file
git add -N new-file
git diff HEAD

Before: shows addition of the new file
After: shows no change

Noticed because the new --ita-invisible-in-index default broke a
script using "git diff --name-only --diff-filter=A master" to get a
list of added files.  Arguably that script should use diff-index
instead, which would have sidestepped the issue, but the new behavior
is unexpected in interactive use as well.

Signed-off-by: Jonathan Nieder 
---
 builtin/diff.c  |  7 ---
 t/t2203-add-intent.sh   | 40 
 t/t4011-diff-symlink.sh | 10 --
 3 files changed, 12 insertions(+), 45 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index b709b6e984..bfefff3a84 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -352,13 +352,6 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
rev.diffopt.flags.allow_external = 1;
rev.diffopt.flags.allow_textconv = 1;
 
-   /*
-* Default to intent-to-add entries invisible in the
-* index. This makes them show up as new files in diff-files
-* and not at all in diff-cached.
-*/
-   rev.diffopt.ita_invisible_in_index = 1;
-
if (nongit)
die(_("Not a git repository"));
argc = setup_revisions(argc, argv, , NULL);
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 1d640a33f0..d9fb151d52 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -69,9 +69,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol &&
git commit -m second &&
test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
-   test $(git diff --name-only --ita-visible-in-index HEAD -- nitfol | wc 
-l) = 1 &&
-   test $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&
-   test $(git diff --name-only -- nitfol | wc -l) = 1
+   test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 &&
+   test $(git diff --name-only --ita-invisible-in-index HEAD -- nitfol | 
wc -l) = 0 &&
+   test $(git diff --name-only --ita-invisible-in-index -- nitfol | wc -l) 
= 1
 '
 
 test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -99,13 +99,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '
 
: >dir/bar &&
git add -N dir/bar &&
-   git diff --name-only >actual &&
+   git diff --cached --name-only >actual &&
echo dir/bar >expect &&
test_cmp expect actual &&
 
git write-tree >/dev/null &&
 
-   git diff --name-only >actual &&
+   git diff --cached --name-only >actual &&
echo dir/bar >expect &&
test_cmp expect actual
 '
@@ -186,19 +186,7 @@ test_expect_success 'rename detection finds the right 
names' '
cat >expected.3 <<-EOF &&
2 .R N... 100644 100644 100644 $hash $hash R100 third   first
EOF
-   test_cmp expected.3 actual.3 &&
-
-   git diff --stat >actual.4 &&
-   cat >expected.4 <<-EOF &&
-first => third | 0
-1 file changed, 0 insertions(+), 0 deletions(-)
-   EOF
-   test_cmp expected.4 actual.4 &&
-
-   git diff --cached --stat >actual.5 &&
-   : >expected.5 &&
-   test_cmp expected.5 actual.5
-
+   test_cmp expected.3 

Re: [PATCH v9 00/17] rebase -i: offer to recreate commit topology by rebasing merges

[Sergey Organov]

This has been sent by mistake, I'm sorry, please disregard.

Sergey Organov  writes:

> Johannes Schindelin  writes:
>
>> Junio, I think this is now ready for `next`. Thank you for your patience
>> and help with this.

[...]

Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Duy Nguyen]

On Fri, May 25, 2018 at 10:12 AM, Junio C Hamano  wrote:
>> +Currently this is used by linkgit:git-checkout[1] when 'git checkout
>> +' will checkout the '' branch on another remote,
>> +and by linkgit:git-worktree[1] when 'git worktree add' refers to a
>> +remote branch. This setting might be used for other checkout-like
>> +commands or functionality in the future.
>
> Hmph, that is an interesting direction.  You foresee that you'll
> have a single repository with multiple remotes to grab and share
> objects from different people working on the same project, and use
> multiple worktrees to work on different branches, yet you are happy
> to declare that each worktree is to work with one particular remote?
>
> We'd need a per-worktree config file to make it work, I guess, or
> a three-level checkout.$worktree_id.defaultRemote configuration
> variable, perhaps?

I still plan to revisit per-worktree config support [1] at some point
and finish it off. Or is it decided that we don't need a generic
mechanism and will add a new level like this for each config var that
is per-worktree? If defaultRemote sets a precedence, then it'll be the
way to go from now on or we'll have another mess when some config does
"foo.$worktree.bar" while others use per-worktree config files.

[1] https://public-inbox.org/git/20170110112524.12870-1-pclo...@gmail.com/
-- 
Duy

Re: [PATCH 1/2] diff: turn --ita-invisible-in-index on by default

[Duy Nguyen]

On Thu, May 24, 2018 at 08:39:42PM -0700, Jonathan Nieder wrote:
> Hi Duy,
> 
> Nguyễn Thái Ngọc Duy wrote:
> 
> > Due to the implementation detail of intent-to-add entries. The current
> > "git diff" (i.e. no treeish or --cached argument) would show the
> > changes in the i-t-a file, but it does not mark the file as new, while
> > "diff --cached" would mark the file as new while showing its content
> > as empty.
> >
> > One evidence of the current output being wrong is that, the output
> > from "git diff" (with ita entries) cannot be applied because it
> > assumes empty files exist before applying.
> >
> > Turning on --ita-invisible-in-index [1] [2] would fix this.
> 
> I'm having a lot of trouble understanding the above.  Part of my
> confusion may be grammatical: for example, the first sentence is a
> sentence fragment.  Another part is that the commit message doesn't tell
> me a story: what does the user try to do and fail that is not possible
> without this?  What is the intention or effect behind the commits
> mentioned that leads to them being cited?
>
> ...

Sorry, this i-t-a thing had been on my mind for too long I mistakenly
thought this problem was common knowledge. Reference [1] in that
commit points to a previous attempt d95d728aba (diff-lib.c: adjust
position of i-t-a entries in diff - 2015-03-16) that was reverted,
which gives more info.

Anyway this is the diff we see today

$ echo haha > new; git add -N
$ git diff
diff --git a/new b/new
index e69de29..5ad28e2 100644
--- a/new
+++ b/new
@@ -0,0 +1 @@
+haha

Notice that the diff does not tell you that 'new' is a new file. The
diff with this patch gives you this

$ git diff
diff --git a/new b/new
new file mode 100644
index 000..5ad28e2
--- /dev/null
+++ b/new
@@ -0,0 +1 @@
+haha

You may argue that an intent-to-add entry is a real entry in the index
and showing "new file mode 100644" here is wrong. I beg to differ.
That just happens to be how you mark an ita entry. If Junio chose to
record ita entries as an index extension, then they would not be
"real" and could still be used to remind users about things to add.

>From the user perspective, as a user I do not care if it's a "real
entry". I want git to _remind_ myself that I need to add that
file. That file should not truly exist in the index because I have not
actually added it. I did not tell git to add anything to the index.

One consequence of this is you can't apply the diff generated with ita
entries because the diff expects empty files to be already in the
worktree. This to me does not make sense.

Of course there's other things that also go along this line. Like if
"git commit" does not add an ita entry, why should it appear in 'git
diff --cached', which should show you what's to be committed. Right
know it shows


$ git diff --cached
diff --git a/new b/new
new file mode 100644
index 000..e69de29

which to me is ridiculous. Why would it show me a diff of a new file
with empty content? I as a user have not mentioned "empty content"
anywhere through the commands I have used.

Since this commit is already in 'next', it's too late to update the
commit message now. Maybe I can elaborate about this more in
git-add.txt if needed (and then I can add more explanation in the
commit message that updates that file).
--
Duy

Re: [PATCH v9 00/17] rebase -i: offer to recreate commit topology by rebasing merges

[Sergey Organov]

Johannes Schindelin  writes:

> Junio, I think this is now ready for `next`. Thank you for your patience
> and help with this.
>
> Once upon a time, I dreamed of an interactive rebase that would not
> linearize all patches and drop all merge commits, but instead recreate
> the commit topology faithfully.
>
> My original attempt was --preserve-merges, but that design was so
> limited that I did not even enable it in interactive mode.
>
> Subsequently, it *was* enabled in interactive mode, with the predictable
> consequences: as the --preserve-merges design does not allow for
> specifying the parents of merge commits explicitly, all the new commits'
> parents are defined *implicitly* by the previous commit history, and
> hence it is *not possible to even reorder commits*.
>
> This design flaw cannot be fixed. Not without a complete re-design, at
> least. This patch series offers such a re-design.
>
> Think of --rebase-merges as "--preserve-merges done right". It
> introduces new verbs for the todo list, `label`, `reset` and `merge`.
> For a commit topology like this:
>
> A - B - C
>   \   /
> D
>
> the generated todo list would look like this:
>
> # branch D
> pick 0123 A
> label branch-point
> pick 1234 D
> label D
>
> reset branch-point
> pick 2345 B
> merge -C 3456 D # C
>
> There are more patches in the pipeline, based on this patch series, but
> left for later in the interest of reviewable patch series: one mini
> series to use the sequencer even for `git rebase -i --root`, and another
> one to add support for octopus merges to --rebase-merges. And then one
> to allow for rebasing merge commits in a smarter way (this one will need
> a bit more work, though, as it can result in very complicated, nested
> merge conflicts *very* easily).
>
> Changes since v8:
>
> - Disentangled the patch introducing `label`/`reset` from the one
>   introducing `merge` again (this was one stupid, tired `git commit
>   --amend` too many).
>
> - Augmented the commit message of "introduce the `merge` command" to
>   describe what the `label onto` is all about.
>
> - Fixed the error message when `reset` would overwrite untracked files to
>   actually say that a "reset" failed (not a "merge").
>
> - Clarified the rationale for `label onto` in the commit message of
>   "rebase-helper --make-script: introduce a flag to rebase merges".
>
> - Edited the description of `--rebase-merges` heavily, for clarity, in
>   "rebase: introduce the --rebase-merges option".
>
> - Edited the commit message of (and the documentation introduced by) " rebase
>   -i: introduce --rebase-merges=[no-]rebase-cousins" for clarity (also
>   mentioning the `--ancestry-path` option).
>
> - When run_git_commit() fails after a successful merge, we now take pains
>   not to reschedule the `merge` command.
>
> - Rebased the patch series on top of current `master`, i.e. both
>   `pw/rebase-keep-empty-fixes` and `pw/rebase-signoff`, to resolve merge
>   conflicts myself.
>
>
> Johannes Schindelin (15):
>   sequencer: avoid using errno clobbered by rollback_lock_file()
>   sequencer: make rearrange_squash() a bit more obvious
>   sequencer: refactor how original todo list lines are accessed
>   sequencer: offer helpful advice when a command was rescheduled
>   sequencer: introduce new commands to reset the revision
>   sequencer: introduce the `merge` command
>   sequencer: fast-forward `merge` commands, if possible
>   rebase-helper --make-script: introduce a flag to rebase merges
>   rebase: introduce the --rebase-merges option
>   sequencer: make refs generated by the `label` command worktree-local
>   sequencer: handle post-rewrite for merge commands
>   rebase --rebase-merges: avoid "empty merges"
>   pull: accept --rebase=merges to recreate the branch topology
>   rebase -i: introduce --rebase-merges=[no-]rebase-cousins
>   rebase -i --rebase-merges: add a section to the man page
>
> Phillip Wood (1):
>   rebase --rebase-merges: add test for --keep-empty
>
> Stefan Beller (1):
>   git-rebase--interactive: clarify arguments
>
>  Documentation/config.txt   |   8 +
>  Documentation/git-pull.txt |   6 +-
>  Documentation/git-rebase.txt   | 163 -
>  builtin/pull.c |  14 +-
>  builtin/rebase--helper.c   |  13 +-
>  builtin/remote.c   |  18 +-
>  contrib/completion/git-completion.bash |   4 +-
>  git-rebase--interactive.sh |  22 +-
>  git-rebase.sh  |  16 +
>  refs.c |   3 +-
>  sequencer.c| 892 -
>  sequencer.h|   7 +
>  t/t3421-rebase-topology-linear.sh  |   1 +
>  t/t3430-rebase-merges.sh   | 244 +++
>  14 files changed, 1352 insertions(+), 59 

[PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Sergey Organov]

When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.

This patch allows '-m 1' for non-merge commits. Besides, as mainline is
always the only parent for a non-merge commit, it made little sense to
disable it in the first place.

Signed-off-by: Sergey Organov 
---
 sequencer.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1ce6326..2393bdf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
return error(_("commit %s does not have parent %d"),
oid_to_hex(>object.oid), 
opts->mainline);
parent = p->item;
-   } else if (0 < opts->mainline)
-   return error(_("mainline was specified but commit %s is not a 
merge."),
-   oid_to_hex(>object.oid));
+   } else if (1 < opts->mainline)
+   /* Non-first parent explicitly specified as mainline for
+* non-merge commit */
+   return error(_("commit %s does not have parent %d"),
+oid_to_hex(>object.oid), opts->mainline);
else
parent = commit->parents->item;
 
-- 
2.10.0.1.g57b01a3

js/empty-config-section-fix, was Re: What's cooking in git.git (May 2018, #03; Wed, 23)

[Johannes Schindelin]

Hi Junio,

On Thu, 24 May 2018, Junio C Hamano wrote:

> * js/empty-config-section-fix (2018-05-18) 1 commit
>  - config: a user-provided invalid section is not a BUG
> 
>  Error codepath fix.
> 
>  Will merge to 'next'.

As a hotfix, maybe we can fast-track it to master?

Thanks,
Dscho

sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Ævar Arnfjörð Bjarmason]

On Thu, May 17 2018, Junio C Hamano wrote:

> * sb/submodule-move-nested (2018-03-29) 6 commits
>   (merged to 'next' on 2018-04-25 at 86b177433a)
>  + submodule: fixup nested submodules after moving the submodule
>  + submodule-config: remove submodule_from_cache
>  + submodule-config: add repository argument to submodule_from_{name, path}
>  + submodule-config: allow submodule_free to handle arbitrary repositories
>  + grep: remove "repo" arg from non-supporting funcs
>  + submodule.h: drop declaration of connect_work_tree_and_git_dir
>
>  Moving a submodule that itself has submodule in it with "git mv"
>  forgot to make necessary adjustment to the nested sub-submodules;
>  now the codepath learned to recurse into the submodules.

I didn't spot this earlier because I don't test this a lot, but I've
bisected the following breakage down to da62f786d2 ("submodule: fixup
nested submodules after moving the submodule", 2018-03-28) (and manually
confirmed by reverting). On Linux both Debian & CentOS I get tests 3 and
4 failing with:

 GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh

-v -x output follows:

expecting success: 
mkdir submodule &&
(cd submodule &&
git init &&
echo a >a &&
git add . &&
git commit -ma
) &&
mkdir super &&
(cd super &&
git init &&
git submodule add ../submodule &&
git submodule add ../submodule a &&
git commit -m "add as submodule and as a" &&
git mv a b &&
git commit -m "move a to b"
)

+ mkdir submodule
+ cd submodule
+ git init
Initialized empty Git repository in /home/avar/g/git/t/trash 
directory.t7411-submodule-config/submodule/.git/
+ echo a
+ git add .
+ git commit -ma
[master (root-commit) 27e9f0e] a
 Author: A U Thor 
 1 file changed, 1 insertion(+)
 create mode 100644 a
+ mkdir super
+ cd super
+ git init
Initialized empty Git repository in /home/avar/g/git/t/trash 
directory.t7411-submodule-config/super/.git/
+ git submodule add ../submodule
Cloning into '/home/avar/g/git/t/trash 
directory.t7411-submodule-config/super/submodule'...
done.
+ git submodule add ../submodule a
Cloning into '/home/avar/g/git/t/trash 
directory.t7411-submodule-config/super/a'...
done.
+ git commit -m add as submodule and as a
[master (root-commit) 5a1dac1] add as submodule and as a
 Author: A U Thor 
 3 files changed, 8 insertions(+)
 create mode 100644 .gitmodules
 create mode 16 a
 create mode 16 submodule
+ git mv a b
+ git commit -m move a to b
[master ab1e9c7] move a to b
 Author: A U Thor 
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename a => b (100%)
ok 1 - submodule config cache setup

expecting success: 
test_when_finished "rm -rf repo" &&
test_create_repo repo &&
cat >repo/.gitmodules <<-\EOF &&
[submodule "s"]
path
ignore
EOF
(
cd repo &&
test_must_fail test-submodule-config "" s 2>actual &&
test_i18ngrep "bad config" actual
)

+ test_when_finished rm -rf repo
+ test 0 = 0
+ test_cleanup={ rm -rf repo
} && (exit "$eval_ret"); eval_ret=$?; :
+ test_create_repo repo
+ test 1 = 1
+ repo=repo
+ mkdir -p repo
+ cd repo
+ /home/avar/g/git/t/../git-init --template=/home/avar/g/git/t/../templates/blt/
Initialized empty Git repository in /home/avar/g/git/t/trash 
directory.t7411-submodule-config/repo/.git/
+ mv .git/hooks .git/hooks-disabled
+ cat
+ cd repo
+ test_must_fail test-submodule-config  s
+ test_i18ngrep bad config actual
+ test -n 
+ test x! = xbad config
+ grep bad config actual
fatal: bad config line 2 in file /home/avar/g/git/t/trash 
directory.t7411-submodule-config/repo/.gitmodules
+ rm -rf repo
+ exit 0
+ eval_ret=0
+ :
ok 2 - configuration parsing with error

expecting success: 
(cd super &&
test-submodule-config \
HEAD^ a \
HEAD b \
HEAD^ submodule \
HEAD submodule \
>actual &&
test_cmp expect actual
)

+ cd super
+ test-submodule-config HEAD^ a HEAD b HEAD^ submodule HEAD submodule
warning: ab1e9c76c1e6a15df51b20e75552ec5ad00708ac:.gitmodules, multiple 
configurations found for 'submodule.submodule.path'. Skipping second one!
warning: ab1e9c76c1e6a15df51b20e75552ec5ad00708ac:.gitmodules, multiple 
configurations found for 'submodule.submodule.url'. Skipping second one!
warning: ab1e9c76c1e6a15df51b20e75552ec5ad00708ac:.gitmodules, multiple 
configurations found for 'submodule.a.path'. Skipping second one!
warning: ab1e9c76c1e6a15df51b20e75552ec5ad00708ac:.gitmodules, multiple 
configurations found for 'submodule.a.url'. Skipping second one!

Re: [PATCH v2] t: make many tests depend less on the refs being files

[Christian Couder]

On Fri, May 25, 2018 at 11:05 AM, Michael Haggerty  wrote:
> On Fri, May 25, 2018 at 10:59 AM, Jeff King  wrote:
>> On Fri, May 25, 2018 at 10:48:04AM +0200, Michael Haggerty wrote:
>>
>>> >  test_expect_success "multi-fetch works off a 'clean' repository" '
>>> > -   rm -r "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" "$GIT_DIR/logs" &&
>>> > +   rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" &&
>>> > +   git reflog expire --all --expire=all &&
>>> > mkdir "$GIT_DIR/svn" &&
>>> > git svn multi-fetch
>>> > '
>>> >
>>>
>>> `rm -rf "$GIT_DIR/refs/remotes"` is not kosher. I think it can be written
>>>
>>> printf 'option no-deref\ndelete %s\n' $(git for-each-ref
>>> --format='%(refname)' refs/remotes) | git update-ref --stdin
>>>
>>> as long as the number of references doesn't exceed command-line limits.
>>> This will also take care of the reflogs. Another alternative would be to
>>> write it as a loop.
>>
>> Perhaps:
>>
>>   git for-each-ref --format="option no-deref%0adelete %(refname)" 
>> refs/remotes |
>>   git update-ref --stdin
>
> Ah yes, that's nicer. I tried with `\n`, but that's not supported
> (wouldn't it be nice if it were?). I didn't think to try `%0a` (let
> alone look in the documentation!)

Thanks both for this suggestion. I plan to use it in another patch.

Re: [PATCH v2] t: make many tests depend less on the refs being files

[Christian Couder]

On Fri, May 25, 2018 at 10:48 AM, Michael Haggerty  wrote:
> On 05/23/2018 07:25 AM, Christian Couder wrote:
>>
>> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
>> index 9e782a8122..a4ebb0b65f 100755
>> --- a/t/t1401-symbolic-ref.sh
>> +++ b/t/t1401-symbolic-ref.sh
>> @@ -65,7 +65,7 @@ reset_to_sane
>>  test_expect_success 'symbolic-ref fails to delete real ref' '
>>   echo "fatal: Cannot delete refs/heads/foo, not a symbolic ref" >expect 
>> &&
>>   test_must_fail git symbolic-ref -d refs/heads/foo >actual 2>&1 &&
>> - test_path_is_file .git/refs/heads/foo &&
>> + git rev-parse --verify refs/heads/foo &&
>>   test_cmp expect actual
>>  '
>>  reset_to_sane
>
> Should t1401 be considered a backend-agnostic test, or is it needed to
> ensure that symbolic refs are written correctly in the files backend?

I don't know. And I am ok to go either way. Another possibility would
be to split in two parts.

Re: "git grep" and "working tree" vs "working directory"

[Robert P. J. Day]

On Fri, 25 May 2018, Junio C Hamano wrote:

> Stefan Beller  writes:
>
> > There are 2 dimensions to it:
> > * (where you are)
> >   if you run git-grep from a sub directory of the repository, then the
> > "sub-working-tree"
> >   will be searched.
>
> s/the repository/the top level directory of the working tree/, perhaps?
>
> >>   also, at the bottom of that section, one reads:
> >>
> >>   ...
> >>   If given, limit the search to paths matching at least one
> >>   pattern. Both leading paths match and glob(7) patterns are supported.
> >>
> >>   For more details about the  syntax, see the pathspec
> >>   entry in gitglossary(7).
> >>
> >> but, again, what if  is *not* given? then what?
> >
> > Assume "$pwd/."
>
> This is not technically wrong per-se, but I do not think there is
> any reason to encourage it over just a simple "." dot.
>
> >>   finally, the first example has the same problem:
> >>
> >>   git grep 'time_t' -- '*.[ch]'
> >>   Looks for time_t in all tracked .c and .h files in the
> >>   working directory and its subdirectories.
> >>
> >> in "the working directory"?
> >>
> >>   what is the proper terminology to be used here?
> >
> > the working directory sounds right, not sure which aspect you want to be
> > exposed more clearly.
>
> "The part of the working tree below and including your current
> working directory", if you really want to be pedantic ;-).
>
> But almost all the examples that show how to work _with_ Git
> inspecting and manipulating tracked contents assume that your
> current working directory _is_ inside a working tree of the
> repository you are working on, so the above is equivalent to "The
> current working directory" should be clear enough for most readers,
> I would think.

  against my better judgment, i'm going to try to summarize this.
first, it appears everyone agrees that the proper way to refer to the
*entirety* of the checked out content is "working tree", and that is
the phrase to use, regardless of your current location in said working
tree.  so, given that, one might say that the command "git status"
would normally display all untracked files in the working tree
(because it does so regardless of your current working directory.)

  related to that, it would seem that the phrase "working tree" is,
far and away, the preferred way to refer to the checked out content.

  on the other end, the meaning of "current working directory" should
be self-evident.

  it's the middle ground, "working directory", that is the problem,
since lots of documentation and comments use that phrase
interchangeably with "working tree", and i doubt that confusion is
ever going away. all one needs to do is:

  $ grep -r "working directory" *

in the git code base to see its prevalence. so, what *should* it mean?
or is there any point in even trying to clarify it?

rday

p.s. i absolutely will not entertain the introduction of the weirdness
that is "sub-working-tree." it's just a subdirectory, that's all.

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

[PATCH] completion: complete remote names too

[Łukasz Stelmach]

"git remote update" accepts both groups and single remotes.

Signed-off-by: Łukasz Stelmach 
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 961a0ed76..fb05bb2f9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2632,7 +2632,7 @@ _git_remote ()
__gitcomp_builtin remote_update
;;
update,*)
-   __gitcomp "$(__git_get_config_variables "remotes")"
+   __gitcomp "$(__git_remotes) $(__git_get_config_variables 
"remotes")"
;;
set-url,--*)
__gitcomp_builtin remote_set-url
-- 
2.11.0

Re: [GSoC][PATCH v3 0/4] rebase: split rebase -p from rebase -i

[Johannes Schindelin]

Hi Alban,

On Thu, 24 May 2018, Alban Gruin wrote:

> This splits the `rebase --preserve-merges` functionnality from
> git-rebase--interactive.sh. All the dead code left by the duplication of
> git-rebase--interactive.sh is also removed. This will make it easier to 
> rewrite
> this script in C.
> 
> This patch series is based of js/sequencer-and-root-commits.

Since you got such copious feedback, I'll just stick to reviewing on
GitHub if you don't mind.

I added a couple of suggestions there.

Ciao,
Dscho

[PATCH] Clarify that a tag can refer to a non-commit object

[Robert P. J. Day]

Reword "man git-tag" to clarify that a tag can refer directly to an
arbitrary object, not just a commit object.

Signed-off-by: Robert P. J. Day 

---

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 1d17101ba..87c4288ff 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -34,8 +34,8 @@ in the tag message.
 If `-m ` or `-F ` is given and `-a`, `-s`, and `-u `
 are absent, `-a` is implied.

-Otherwise just a tag reference for the SHA-1 object name of the commit object 
is
-created (i.e. a lightweight tag).
+Otherwise, a tag reference that points directly at the given object
+(i.e., a lightweight tag) is created.

 A GnuPG signed tag object will be created when `-s` or `-u
 ` is used.  When `-u ` is not used, the

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: "man git-tag" inconsistent about whether you can tag non-commit objects

[Robert P. J. Day]

On Fri, 25 May 2018, Junio C Hamano wrote:

> "Robert P. J. Day"  writes:
>
> >   embarrassed to admit i had no idea that you could tag non-commit
> > objects, only realized that when i was reading the man page and saw:
> >
> >   SYNOPSIS
> >  git tag [-a | -s | -u ] [-f] [-m  | -F ] [-e]
> > [ | ]
> >  
> >
> > so i tried it and, sure enough, i could tag a blob object. but if you
> > read further into DESCRIPTION, about halfway through, you read:
> >
> >   "Otherwise just a tag reference for the SHA-1 object name of the
> >commit object is created (i.e. a lightweight tag)."
> >^^
> >
> > which suggests only commit objects. finally, much further down, under
> > OPTIONS:
> >
> >   ", 
> >  The object that the new tag will refer to, usually a commit.
> > 
> >
> > so to clean this up, is it sufficient to just change that middle line
> > to say "object" rather than "commit object"? or is there more in the
> > man page that needs tweaking?
>
> As that sentence talks about a lightweight tag (i.e. a reference in
> refs/tags/ hierarchy that directly points at an object of any kind),
> another possibility would be to say
>
>   Otherwise a tag reference that directly points at the given
>   object (i.e. lightweight tag) is created.

  yup, that would resolve the issue. patch coming shortly.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Junio C Hamano  writes:

>> -warning("the '-l' alias for '--create-reflog' is deprecated;");
>> -warning("it will be removed in a future version of Git");
>> +if (list) {
>> +warning("the '-l' option is an alias for 
>> '--create-reflog' and");
>> +warning("has no effect in list mode. This option will 
>> soon be");
>> +warning("removed and you should omit it (or use 
>> '--list' instead).");
>> +} else {
>> +warning("the '-l' alias for '--create-reflog' is 
>> deprecated;");
>> +warning("it will be removed in a future version of 
>> Git");
>> +}

By the way, this is one of these times when I feel that we should
have a better multi-line message support in die/error/warning/info
functions.  Ideally, I should be able to write

warning(_("the '-l' option is an alias for '--create-reflog' and\n"
  "has no effect in list mode, This option will soon be\n"
  "removed and you should omit it (or use '--list' instead)."));

and warning() would:

 - do the sprintf formatting thing as necessary to prepare a long multi-line
   message;

 - chomp that into lines at '\n' boundary; and

 - give each of these lines with _("warning: ") prefixed.

That way, translators can choose to make the resulting message to
different number of lines from the original easily.

Re: [PATCH v2] t: make many tests depend less on the refs being files

[Michael Haggerty]

On Fri, May 25, 2018 at 10:59 AM, Jeff King  wrote:
> On Fri, May 25, 2018 at 10:48:04AM +0200, Michael Haggerty wrote:
>
>> >  test_expect_success "multi-fetch works off a 'clean' repository" '
>> > -   rm -r "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" "$GIT_DIR/logs" &&
>> > +   rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" &&
>> > +   git reflog expire --all --expire=all &&
>> > mkdir "$GIT_DIR/svn" &&
>> > git svn multi-fetch
>> > '
>> >
>>
>> `rm -rf "$GIT_DIR/refs/remotes"` is not kosher. I think it can be written
>>
>> printf 'option no-deref\ndelete %s\n' $(git for-each-ref
>> --format='%(refname)' refs/remotes) | git update-ref --stdin
>>
>> as long as the number of references doesn't exceed command-line limits.
>> This will also take care of the reflogs. Another alternative would be to
>> write it as a loop.
>
> Perhaps:
>
>   git for-each-ref --format="option no-deref%0adelete %(refname)" 
> refs/remotes |
>   git update-ref --stdin

Ah yes, that's nicer. I tried with `\n`, but that's not supported
(wouldn't it be nice if it were?). I didn't think to try `%0a` (let
alone look in the documentation!)

Michael

Re: [PATCH v2] t: make many tests depend less on the refs being files

[Jeff King]

On Fri, May 25, 2018 at 10:48:04AM +0200, Michael Haggerty wrote:

> >  test_expect_success "multi-fetch works off a 'clean' repository" '
> > -   rm -r "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" "$GIT_DIR/logs" &&
> > +   rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" &&
> > +   git reflog expire --all --expire=all &&
> > mkdir "$GIT_DIR/svn" &&
> > git svn multi-fetch
> > '
> > 
> 
> `rm -rf "$GIT_DIR/refs/remotes"` is not kosher. I think it can be written
> 
> printf 'option no-deref\ndelete %s\n' $(git for-each-ref
> --format='%(refname)' refs/remotes) | git update-ref --stdin
> 
> as long as the number of references doesn't exceed command-line limits.
> This will also take care of the reflogs. Another alternative would be to
> write it as a loop.

Perhaps:

  git for-each-ref --format="option no-deref%0adelete %(refname)" refs/remotes |
  git update-ref --stdin

-Peff

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King  writes:

> People mistakenly use "git branch -l", thinking that it
> triggers list mode. It doesn't, but the lack of non-option
> arguments in that command does (and the "-l" becomes a
> silent noop).
>
> Since afc968e579 (branch: deprecate "-l" option, 2018-03-26)
> we've warned that "-l" is going away. But the warning text
> is primarily aimed at people who _meant_ to use "-l", as in
> "git branch -l foo". People who mistakenly said "git branch
> -l" may be left puzzled.

Yup, thanks for being extra explicit.  I do imagine there are quite
a few of us who would be puzzled without this update (but with the
previous one to unhide it from behind the pager).

> Let's make it clear that:
>
>   1. No, "-l" didn't do what they thought here.
>
>   2. It's going away, and what they should do instead.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/branch.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55bfacd843..b0b33dab94 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -701,8 +701,14 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
>   setup_auto_pager("branch", 1);
>  
>   if (used_deprecated_reflog_option) {
> - warning("the '-l' alias for '--create-reflog' is deprecated;");
> - warning("it will be removed in a future version of Git");
> + if (list) {
> + warning("the '-l' option is an alias for 
> '--create-reflog' and");
> + warning("has no effect in list mode. This option will 
> soon be");
> + warning("removed and you should omit it (or use 
> '--list' instead).");
> + } else {
> + warning("the '-l' alias for '--create-reflog' is 
> deprecated;");
> + warning("it will be removed in a future version of 
> Git");
> + }
>   }
>  
>   if (delete) {

Re: [PATCH v2] t: make many tests depend less on the refs being files

[Michael Haggerty]

On 05/23/2018 07:25 AM, Christian Couder wrote:
> From: David Turner 
> 
> Many tests are very focused on the file system representation of the
> loose and packed refs code. As there are plans to implement other
> ref storage systems, let's migrate these tests to a form that test
> the intent of the refs storage system instead of it internals.
> [...]
> 
> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
> index 9e782a8122..a4ebb0b65f 100755
> --- a/t/t1401-symbolic-ref.sh
> +++ b/t/t1401-symbolic-ref.sh
> @@ -65,7 +65,7 @@ reset_to_sane
>  test_expect_success 'symbolic-ref fails to delete real ref' '
>   echo "fatal: Cannot delete refs/heads/foo, not a symbolic ref" >expect 
> &&
>   test_must_fail git symbolic-ref -d refs/heads/foo >actual 2>&1 &&
> - test_path_is_file .git/refs/heads/foo &&
> + git rev-parse --verify refs/heads/foo &&
>   test_cmp expect actual
>  '
>  reset_to_sane

Should t1401 be considered a backend-agnostic test, or is it needed to
ensure that symbolic refs are written correctly in the files backend?

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index c0ef946811..222dc2c377 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -234,34 +234,34 @@ test_expect_success 'git branch -M master2 master2 
> should work when master is ch
>  
>  test_expect_success 'git branch -v -d t should work' '
>   git branch t &&
> - test_path_is_file .git/refs/heads/t &&
> + git rev-parse --verify refs/heads/t &&
>   git branch -v -d t &&
> - test_path_is_missing .git/refs/heads/t
> + test_must_fail git rev-parse --verify refs/heads/t
>  '
>  
>  test_expect_success 'git branch -v -m t s should work' '
>   git branch t &&
> - test_path_is_file .git/refs/heads/t &&
> + git rev-parse --verify refs/heads/t &&
>   git branch -v -m t s &&
> - test_path_is_missing .git/refs/heads/t &&
> - test_path_is_file .git/refs/heads/s &&
> + test_must_fail git rev-parse --verify refs/heads/t &&
> + git rev-parse --verify refs/heads/s &&
>   git branch -d s
>  '
>  
>  test_expect_success 'git branch -m -d t s should fail' '
>   git branch t &&
> - test_path_is_file .git/refs/heads/t &&
> + git rev-parse refs/heads/t &&
>   test_must_fail git branch -m -d t s &&
>   git branch -d t &&
> - test_path_is_missing .git/refs/heads/t
> + test_must_fail git rev-parse refs/heads/t
>  '
>  
>  test_expect_success 'git branch --list -d t should fail' '
>   git branch t &&
> - test_path_is_file .git/refs/heads/t &&
> + git rev-parse refs/heads/t &&
>   test_must_fail git branch --list -d t &&
>   git branch -d t &&
> - test_path_is_missing .git/refs/heads/t
> + test_must_fail git rev-parse refs/heads/t
>  '
>  
>  test_expect_success 'git branch --list -v with --abbrev' '
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aefde7b172..1f871d3cca 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -726,7 +726,7 @@ test_expect_success 'store updates stash ref and reflog' '
>   git reset --hard &&
>   ! grep quux bazzy &&
>   git stash store -m quuxery $STASH_ID &&
> - test $(cat .git/refs/stash) = $STASH_ID &&
> + test $(git rev-parse stash) = $STASH_ID &&
>   git reflog --format=%H stash| grep $STASH_ID &&
>   git stash pop &&
>   grep quux bazzy
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 0680dec808..d4f435155f 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -30,7 +30,7 @@ add () {
>   test_tick &&
>   commit=$(echo "$text" | git commit-tree $tree $parents) &&
>   eval "$name=$commit; export $name" &&
> - echo $commit > .git/refs/heads/$branch &&
> + git update-ref "refs/heads/$branch" "$commit" &&
>   eval ${branch}TIP=$commit
>  }
>  
> @@ -45,10 +45,10 @@ pull_to_client () {
>  
>   case "$heads" in
>   *A*)
> - echo $ATIP > .git/refs/heads/A;;
> + git update-ref refs/heads/A "$ATIP";;
>   esac &&
>   case "$heads" in *B*)
> - echo $BTIP > .git/refs/heads/B;;
> + git update-ref refs/heads/B "$BTIP";;
>   esac &&
>   git symbolic-ref HEAD refs/heads/$(echo $heads \
>   | sed -e "s/^\(.\).*$/\1/") &&
> @@ -92,8 +92,8 @@ test_expect_success 'setup' '
>   cur=$(($cur+1))
>   done &&
>   add B1 $A1 &&
> - echo $ATIP > .git/refs/heads/A &&
> - echo $BTIP > .git/refs/heads/B &&
> + git update-ref refs/heads/A "$ATIP" &&
> + git update-ref refs/heads/B "$BTIP" &&
>   git symbolic-ref HEAD refs/heads/B
>  '
>  
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index ae5a530a2d..e402aee6a2 100755
> --- a/t/t5510-fetch.sh
> 

Re: "man git-tag" inconsistent about whether you can tag non-commit objects

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> As that sentence talks about a lightweight tag (i.e. a reference in
>> refs/tags/ hierarchy that directly points at an object of any kind),
>> another possibility would be to say
>>
>>  Otherwise a tag reference that directly points at the given
>>  object (i.e. lightweight tag) is created.
>
> Related, this recent patch of mine:
> https://public-inbox.org/git/20180429202100.32353-6-ava...@gmail.com/#t
>
> I.e. might be worth talking about this briefly in the git-tag manpage as
> well, i.e. that you can create a lightweight "tag" to a commit, but then
> depending on where you push it it becomes either a branch or a tag,
> which may not be intuitive to users...

I am not sure if I agree.  People won't get confused, unless they
think too much and pedantically.

If you make refs/tags/$name point at a commit, it becomes a
lightweight tag, if you make refs/heads/$name point at a commit,
it becomes a local branch, if you make refs/remotes/$remote/$name
point at a commit, it behaves as a remote-tracking branch for the
named $remote.  Anywhere else, it is just a random ref that happens
to point at a commit.

And notice that I never said "push" in the above.  The verb I used
is "make" and that is deliberate.  It does not make any difference
if you make such a ref point at a commit by pushing into a
repository, fetching from elsewhere, or running "git branch", "git
tag", "git update-ref" locally.

Re: [PATCH v2] Use proper syntax for replaceables in command docs

[Junio C Hamano]

"Robert P. J. Day"  writes:

> The standard for command documentation synopses appears to be:
>
>   [...] means optional
>   <...> means replaceable
>   [<...>] means both optional and replaceable
>
> So fix a number of doc pages that use incorrect variations of the
> above.
>
> Signed-off-by: Robert P. J. Day 
>
> ---
>
> diff --git a/Documentation/git-annotate.txt b/Documentation/git-annotate.txt
> index 05fd482b7..e44a83133 100644
> --- a/Documentation/git-annotate.txt
> +++ b/Documentation/git-annotate.txt
> @@ -8,7 +8,7 @@ git-annotate - Annotate file lines with commit information
>  SYNOPSIS
>  
>  [verse]
> -'git annotate' [options] file [revision]
> +'git annotate' []  []
> ...
> -'git check-mailmap' [options] ...
> +'git check-mailmap' [] ...

A pedant in me screams s//.../ after seeing this
line, but  appears _very_ _very_ often and extremely handy,
compared to having to spell "...".  So let's standardise
the way this patch does.

Thanks.

Re: [PATCH v3] checkout & worktree: introduce checkout.implicitRemote

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> +checkout.implicitRemote::
> + When you run 'git checkout ' and only have one
> + remote, it may implicitly fall back on checking out and
> + tracking e.g. 'origin/'.

Yup, that is quite implicit.  It works without configuring anything,
only as long as you have a single remote.  That is quite implicit.


> This stops working as soon
> + as you have more than one remote with a ''
> + reference. This setting allows for setting the name of a
> + preferred remote that should always win when it comes to
> + disambiguation. The typical use-case is to set this to
> + `origin`.

So this feature and configuration feels more like an explicit one,
to be used to affect how Git works when the implicit one does not
work well.  I would have called it checkout.defaultRemote, as it
would be a nonsense name to call it checkout.explicitRemote ;-).

> +Currently this is used by linkgit:git-checkout[1] when 'git checkout
> +' will checkout the '' branch on another remote,
> +and by linkgit:git-worktree[1] when 'git worktree add' refers to a
> +remote branch. This setting might be used for other checkout-like
> +commands or functionality in the future.

Hmph, that is an interesting direction.  You foresee that you'll
have a single repository with multiple remotes to grab and share
objects from different people working on the same project, and use
multiple worktrees to work on different branches, yet you are happy
to declare that each worktree is to work with one particular remote?

We'd need a per-worktree config file to make it work, I guess, or
a three-level checkout.$worktree_id.defaultRemote configuration
variable, perhaps?

In any case I can see how this will help those with multiple remotes
(including me ;-).  Thanks for moving this topic forward.

Re: "man git-tag" inconsistent about whether you can tag non-commit objects

[Ævar Arnfjörð Bjarmason]

On Fri, May 25 2018, Junio C Hamano wrote:

> "Robert P. J. Day"  writes:
>
>>   embarrassed to admit i had no idea that you could tag non-commit
>> objects, only realized that when i was reading the man page and saw:
>>
>>   SYNOPSIS
>>  git tag [-a | -s | -u ] [-f] [-m  | -F ] [-e]
>> [ | ]
>>  
>>
>> so i tried it and, sure enough, i could tag a blob object. but if you
>> read further into DESCRIPTION, about halfway through, you read:
>>
>>   "Otherwise just a tag reference for the SHA-1 object name of the
>>commit object is created (i.e. a lightweight tag)."
>>^^
>>
>> which suggests only commit objects. finally, much further down, under
>> OPTIONS:
>>
>>   ", 
>>  The object that the new tag will refer to, usually a commit.
>> 
>>
>> so to clean this up, is it sufficient to just change that middle line
>> to say "object" rather than "commit object"? or is there more in the
>> man page that needs tweaking?
>
> As that sentence talks about a lightweight tag (i.e. a reference in
> refs/tags/ hierarchy that directly points at an object of any kind),
> another possibility would be to say
>
>   Otherwise a tag reference that directly points at the given
>   object (i.e. lightweight tag) is created.

Related, this recent patch of mine:
https://public-inbox.org/git/20180429202100.32353-6-ava...@gmail.com/#t

I.e. might be worth talking about this briefly in the git-tag manpage as
well, i.e. that you can create a lightweight "tag" to a commit, but then
depending on where you push it it becomes either a branch or a tag,
which may not be intuitive to users...

Re: [PATCH v2] Use proper syntax for replaceables in command docs

[Simon Ruderich]

On Thu, May 24, 2018 at 04:11:39PM -0400, Robert P. J. Day wrote:
> diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
> index 37b96c545..f98b7c6ed 100644
> --- a/Documentation/git-cvsserver.txt
> +++ b/Documentation/git-cvsserver.txt
> @@ -22,7 +22,7 @@ cvspserver stream tcp nowait nobody /usr/bin/git-cvsserver 
> git-cvsserver pserver
>  Usage:
>
>  [verse]
> -'git-cvsserver' [options] [pserver|server] [ ...]
> +'git-cvsserver' [] [pserver|server] [ ...]

No space in front of "..." for consistency?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: BUG: No way to set fsck. when cloning

[Ævar Arnfjörð Bjarmason]

On Thu, May 24 2018, Thomas Braun wrote:

> Am 24.05.2018 um 17:25 schrieb Ævar Arnfjörð Bjarmason:
>> When I do:
>>
>> git -c fetch.fsckObjects=true clone 
>> g...@github.com:robbyrussell/oh-my-zsh.git
>>
>> I get:
>>
>> error: object 2b7227859263b6aabcc28355b0b994995b7148b6: 
>> zeroPaddedFilemode: contains zero-padded file modes
>> fatal: Error in object
>> fatal: index-pack failed
>>
>> [...]
>
> Doing
>
> git clone --config transfer.fsckobjects=false --config
> receive.fsckobjects=false --config fetch.fsckobjects=false
> g...@github.com:robbyrussell/oh-my-zsh.git
>
> does the trick here (stolen from [1]).
>
> $ git --version
> git version 2.17.0.windows.1
>
> I don't know why though.

Yeah, because it entirely turns off fsck on fetch, and we didn't have
any way to selectively disable just some stuff until the patch series I
just sent
(https://public-inbox.org/git/20180524193516.28713-1-ava...@gmail.com/)

> [1]:
> https://github.com/michaeljones/breathe/issues/340#issuecomment-390775142

Re: unexpected "unresolved merge conflict" for a new file

[Michal Hocko]

On Thu 24-05-18 13:11:20, Jeff King wrote:
> On Thu, May 24, 2018 at 01:36:57PM +0200, Michal Hocko wrote:
> 
> > `git commit' fails on a newly added file with the following
> > *
> > * You have some suspicious patch lines:
> > *
> > * In Documentation/core-api/gfp_mask-from-fs-io.rst
> > * unresolved merge conflict (line 27)
> > Documentation/core-api/gfp_mask-from-fs-io.rst:27:===
> 
> This message isn't generated by git itself, but rather by a pre-commit
> hook. You can skip the hook by running "git commit --no-verify".
> 
> As for the false positive in the hook logic, I can't say more without
> having seen the hook source. :) Do you know where you got it from?
> 
> (Googling for "suspicious patch lines" turns up some hits, but with
> varying provenance).

Ohh, I see. I must have installed this one lng time ago. Attached
for reference. I will just drop it. Sorry about tht noise.
-- 
Michal Hocko
SUSE Labs
#!/bin/sh
#
# An example hook script to verify what is about to be committed.
# Called by git-commit with no arguments.  The hook should
# exit with non-zero status after issuing an appropriate message if
# it wants to stop the commit.
#
# To enable this hook, make this file executable.

# This is slightly modified from Andrew Morton's Perfect Patch.
# Lines you introduce should not have trailing whitespace.
# Also check for an indentation that has SP before a TAB.

if git-rev-parse --verify HEAD 2>/dev/null
then
git-diff-index -p -M --cached HEAD --
else
# NEEDSWORK: we should produce a diff with an empty tree here
# if we want to do the same verification for the initial import.
:
fi |
perl -e '
my $found_bad = 0;
my $filename;
my $reported_filename = "";
my $lineno;
sub bad_line {
my ($why, $line) = @_;
if (!$found_bad) {
print STDERR "*\n";
print STDERR "* You have some suspicious patch lines:\n";
print STDERR "*\n";
$found_bad = 1;
}
if ($reported_filename ne $filename) {
print STDERR "* In $filename\n";
$reported_filename = $filename;
}
print STDERR "* $why (line $lineno)\n";
print STDERR "$filename:$lineno:$line\n";
}
while (<>) {
if (m|^diff --git a/(.*) b/\1$|) {
$filename = $1;
next;
}
if (/^@@ -\S+ \+(\d+)/) {
$lineno = $1 - 1;
next;
}
if (/^ /) {
$lineno++;
next;
}
if (s/^\+//) {
$lineno++;
chomp;
if (/\s$/) {
bad_line("trailing whitespace", $_);
}
if (/^\s* \t/) {
bad_line("indent SP followed by a TAB", $_);
}
if (/^([<>])\1{6} |^={7}$/) {
bad_line("unresolved merge conflict", $_);
}
}
}
exit($found_bad);
'

Re: git difftool with symlink to readonly jar failed

[Etienne d'Hautefeuille]

Hi,

same problem with all program, There is a crash before the launch

> git difftool --dir-diff 4cb98b4a307ce97d9e6b8e4b03211fa5ca8af7e7
0244799661b993b1f78fa5afb621de3fe4c4a39c --
fatal: could not open '/tmp/git-difftool.g80nLO/left/jenkins.war' for
writing: Permission denied

> git difftool --dir-diff  --dir-diff --tool=kdiff3
4cb98b4a307ce97d9e6b8e4b03211fa5ca8af7e7
0244799661b993b1f78fa5afb621de3fe4c4a39c --
fatal: could not open '/tmp/git-difftool.R1NgMw/left/jenkins.war' for
writing: Permission denied

> git difftool --dir-diff  --dir-diff --tool=bc3
4cb98b4a307ce97d9e6b8e4b03211fa5ca8af7e7
0244799661b993b1f78fa5afb621de3fe4c4a39c --
fatal: could not open '/tmp/git-difftool.MNyx8b/left/jenkins.war' for
writing: Permission denied

> git difftool --dir-diff  --dir-diff --tool=kompare
4cb98b4a307ce97d9e6b8e4b03211fa5ca8af7e7
0244799661b993b1f78fa5afb621de3fe4c4a39c --
fatal: could not open '/tmp/git-difftool.lj82wR/left/jenkins.war' for
writing: Permission denied

Le ven. 25 mai 2018 à 07:40, Christian Couder 
a écrit :

> Hi,

> On Thu, May 24, 2018 at 11:11 PM, Etienne d'Hautefeuille
>  wrote:
> >
> > #try  a diff
> > git difftool --dir-diff 4cb98b4a307ce97d9e6b8e4b03211fa5ca8af7e7
> > 0244799661b993b1f78fa5afb621de3fe4c4a39c
> > fatal: impossible d'ouvrir '/tmp/git-difftool.UQ4mqo/left/jenkins.war'
en
> > écriture: Permission non accordée

> You should use LANG=C so that people can understand the error message.

> Also git difftool launches another program that will actually perform
> the diff. It looks like it is bcompare on your setup. Did you try with
> another program?

Re: commit-graph: change in "best" merge-base when ambiguous

[Michael Haggerty]

On 05/25/2018 12:08 AM, Jakub Narebski wrote:
> Derrick Stolee  writes:
>> On 5/22/2018 1:39 AM, Michael Haggerty wrote:
>>> On 05/21/2018 08:10 PM, Derrick Stolee wrote:
 [...]
>>> This may be beyond the scope of what you are working on, but there are
>>> significant advantages to selecting a "best" merge base from among the
>>> candidates. Long ago [1] I proposed that the "best" merge base is the
>>> merge base candidate that minimizes the number of non-merge commits that
>>> are in
>>>
>>>  git rev-list $candidate..$branch
>>>
>>> that are already in master:
>>>
>>>  git rev-list $master
>>>
>>> (assuming merging branch into master), which is equivalent to choosing
>>> the merge base that minimizes
>>>
>>>  git rev-list --count $candidate..$branch
> 
> Is the above correct...
> 
>>> In fact, this criterion is symmetric if you exchange branch ↔ master,
>>> which is a nice property, and indeed generalizes pretty simply to
>>> computing the merge base of more than two commits.
> 
> ...as it doesn't seem to have the described symmetry.

The first email that I referenced [1] demonstrates this in the section
"Symmetry; generalization to more than two branches". The same thing is
demonstrated in a simpler way using set notation in a later email in
that thread [2].

Michael

[1] https://public-inbox.org/git/539a25bf.4060...@alum.mit.edu/
[2] https://public-inbox.org/git/53a06264.9080...@alum.mit.edu/