RE: git svn and SVN property svn:original-date

[Craig McQueen]

Eric Wong wrote:
> Craig McQueen  wrote:
> > When doing "git svn dcommit", the SVN revision just has the date/time
> stamp of the time of the dcommit.
> 
> Yeah, that's sometimes annoying to me, too.
> 
> > Apparently SVN revisions can have an "svn:original-date" property, which
> would be good to set on dcommit, to preserve the timestamp from the git
> repository.
> >
> >
> https://subversion.apache.org/docs/api/1.7/group__svn__props__revision
> > __props.html#ga8f17351dd056149da9cb490f1daf4018
> 
> Any idea if which versions of SVN it's supported in and how recent the
> feature is?

I see discussion about it in 2003, so I guess it's been there right from 1.0.0.

> Perhaps we can enable it everywhere, and maybe only old clients won't
> understand it, but won't fail; and we could start using it as the author date
> with "git svn fetch".

Using it for author date sounds sensible.

> OTOH, that would break the (perhaps unofficial) independently-created-git-
> svn-mirrors-should-have-same-oids-by-default
> rule when people run different versions of git, so maybe it could be an
> option...

Hmm, good question. Maybe it should be an option, though I hope it would be 
enabled by default (since the feature would be more metadata-preserving, which 
is a good thing), with an option to disable it to allow backwards compatibility 
with people running an older version of git. That's my opinion anyway, and I 
realise my opinion is not necessarily well-informed regarding all 
considerations.

Note, I'm unclear as to whether Subversion is willing to store timezone 
information in svn:original-date. But I guess having git author date correspond 
to svn:original-date is an improvement for preserving more metadata, even if 
the timezone is lost in the process.

-- 
Craig McQueen

Re: git svn propset and multi-line svn:externals property

[Eric Wong]

Craig McQueen  wrote:
> Is it possible to set multi-line SVN properties somehow? Or
> could this be a future enhancement?

I'm not sure.  I don't use this feature, but it seems tied
to gitattributes(5), and I'm not sure if gitattributes supports
multi-line values, either...

Re: git svn and SVN property svn:original-date

[Eric Wong]

Craig McQueen  wrote:
> When doing "git svn dcommit", the SVN revision just has the date/time stamp 
> of the time of the dcommit.

Yeah, that's sometimes annoying to me, too.

> Apparently SVN revisions can have an "svn:original-date" property, which 
> would be good to set on dcommit, to preserve the timestamp from the git 
> repository.
> 
> https://subversion.apache.org/docs/api/1.7/group__svn__props__revision__props.html#ga8f17351dd056149da9cb490f1daf4018

Any idea if which versions of SVN it's supported in and how
recent the feature is?

Perhaps we can enable it everywhere, and maybe only old clients
won't understand it, but won't fail; and we could start using
it as the author date with "git svn fetch".

OTOH, that would break the (perhaps unofficial)
independently-created-git-svn-mirrors-should-have-same-oids-by-default
rule when people run different versions of git, so maybe it
could be an option...

[PATCH] tests: lint for run-away here-doc

[Junio C Hamano]

We found a few run-away here documents that are started with an
end-of-here-doc marker that is incorrectly spelled, e.g.

git some command >actual &&
cat 
---

 * This catches all the cases detected in the recent discussion, I think.

 t/test-lib.sh | 4 
 1 file changed, 4 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 86d77c16dd..97bdc91f54 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -627,6 +627,10 @@ test_run_ () {
test_eval_ "(exit 117) && $1"
if test "$?" != 117; then
error "bug in the test script: broken &&-chain: $1"
+   elif ! OK=$(test_eval_ "false && $1${LF}${LF}echo OK" 
2>/dev/null) ||
+  test OK != "$OK"
+   then
+   error "bug in the test script: possibly unterminated 
HERE-DOC"
fi
trace=$trace_tmp
fi

Re: [PATCH 3/3] short status: improve reporting for submodule changes

[Jonathan Nieder]

Stefan Beller wrote:

>  Documentation/git-status.txt |  9 +
>  t/t3600-rm.sh| 18 +-
>  t/t7506-status-submodule.sh  | 24 
>  wt-status.c  | 13 +++--
>  4 files changed, 57 insertions(+), 7 deletions(-)

Very nice.

[...]
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -181,6 +181,13 @@ in which case `XY` are `!!`.

The documentation is clear.

[...]
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh

The updated behavior shown in this test makes sense.

[...]
> --- a/t/t7506-status-submodule.sh
> +++ b/t/t7506-status-submodule.sh

This test has good coverage and describes a good behavior.

[...]
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -431,10 +431,19 @@ static void wt_status_collect_changed_cb(struct 
> diff_queue_struct *q,
>   }
>   if (!d->worktree_status)
>   d->worktree_status = p->status;
> - d->dirty_submodule = p->two->dirty_submodule;
> - if (S_ISGITLINK(p->two->mode))
> + if (S_ISGITLINK(p->two->mode)) {
> + d->dirty_submodule = p->two->dirty_submodule;
>   d->new_submodule_commits = !!oidcmp(>one->oid,
>   >two->oid);
> + if (s->status_format == STATUS_FORMAT_SHORT) {
> + if (d->new_submodule_commits)
> + d->worktree_status = 'M';

Can these be new DIFF_STATUS_* letters in diff.h?

I hadn't realized before that the worktree_status usually is taken
verbatim from diff_filepair.  Now I understand better what you were
talking about offline earlier today (sorry for the confusion).

We probably don't want the diff machinery to have to be aware of the
various status_format modes, so doing this here seems sensible to me.
Unfortunately it's also a reminder that "git diff --name-status" and
--diff-filter could benefit from a similar change.  (Unfortunate
because that's a harder change to make without breaking scripts.)

> + else if (d->dirty_submodule & 
> DIRTY_SUBMODULE_MODIFIED)
> + d->worktree_status = 'm';
> + else if (d->dirty_submodule & 
> DIRTY_SUBMODULE_UNTRACKED)
> + d->worktree_status = '?';
> + }
> + }

Given the above, this implementation looks good. So for what it's worth,
this patch is
Reviewed-by: Jonathan Nieder 

Patch 1/3 is the one that gives me pause, since I hadn't been able to
chase down all callers.  Would it be feasible to split that into two:
one patch to switch to --porcelain=2 but not change behavior, and a
separate patch to improve what is stored in the dirty_submodule flags?

Thanks,
Jonathan

Re: [PATCH 2/3] submodule.c, is_submodule_modified: stricter checking for submodules

[Jonathan Nieder]

Stefan Beller wrote:

> By having a stricter check in the superproject we catch errors earlier,
> instead of spawning a child process to tell us.

Plus the error message is clearer.

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

Reviewed-by: Jonathan Nieder 

Re: [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2

[Jonathan Nieder]

Stefan Beller wrote:

> Migrate 'is_submodule_modified' to the new porcelain format of
> git-status.
>
> As the old porcelain only reported ' M' for submodules, no
> matter what happened inside the submodule (untracked files,
> changes to tracked files or move of HEAD), the new API
> properly reports the different scenarios.
[...]
>  submodule.c | 53 -
>  1 file changed, 24 insertions(+), 29 deletions(-)

Neat.  Is this something that could be covered in tests, or should I
be patient and rely on patch 3/3 for that?

I think this would be easier to understand if it were two patches: one
that switched to --porcelain=2 with no change in behavior, and another
that took advantage of --porcelain=2 to return richer information.  As
is, I had trouble verifying that this isn't going to break anything
--- there's not enough local information here and in submodule.h to
tell what callers may rely on and I didn't audit callers.

Thanks and hope that helps,
Jonathan

Re: [PATCH v2 10/16] tag: change misleading --list documentation

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Junio would you be fine with just this on top:
>
> diff --git a/t/README b/t/README
> index 4982d1c521..9e079a360a 100644
> --- a/t/README
> +++ b/t/README
> @@ -379,2 +379,5 @@ Do:
>
> + - Include tests which assert that the desired & recommended behavior
> +   of commands is preserved.
> +
>   - Put all code inside test_expect_success and other assertions.
> @@ -424,2 +427,17 @@ Don't:
>
> + - Include tests which exhaustively test for various edge cases or
> +   unintended emergent behavior which we're not interested in
> +   supporting in the future.
> +
> +   An exception to this is are cases where we don't care about
> +   different behaviors X and Y, but we need to check that it does one
> +   of them, and not Z.
> +
> +   Another exception are cases where our documentation might
> +   unintentionally stated or implied that something was supported or
> +   recommended, but we'd like to discourage its use going forward.
> +
> +   In both of the above cases please prominently comment the test
> +   indicating that you're testing for one of these two cases.
> +
>   - exit() within a 

[PATCH 3/3] short status: improve reporting for submodule changes

[Stefan Beller]

If I add an untracked file to a submodule or modify a tracked file,
currently "git status --short" treats the change in the same way as
changes to the current HEAD of the submodule:

$ git clone --quiet --recurse-submodules 
https://gerrit.googlesource.com/gerrit
$ echo hello >gerrit/plugins/replication/stray-file
$ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
$ git -C gerrit status --short
 M plugins/replication

This is by analogy with ordinary files, where "M" represents a change
that has not been added yet to the index.  But this change cannot be
added to the index without entering the submodule, "git add"-ing it,
and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this.  These are similar
to the existing "??" and " M" but mean that the submodule (not the
parent project) has new untracked files and modified files, respectively.
The user can use "git add" and "git commit" from within the submodule to
add them.

Changes to the submodule's HEAD commit can be recorded in the index with
a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for
the submodule.  They represent increasing levels of change --- the last
one that applies is shown (e.g., " m" if there are both modified files
and untracked files in the submodule, or " M" if the submodule's HEAD
has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain
level 1, which shares code with "status --short".  We only change
"git status --short".

Non-short "git status" and "git status --porcelain=2" already handle
these cases by showing more detail:

$ git -C gerrit status --porcelain=2
1 .M S.MU 16 16 16 305c864db28eb0c77c8499bc04c87de3f849cf3c 
305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
$ git -C gerrit status
[...]
modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

Helped-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 Documentation/git-status.txt |  9 +
 t/t3600-rm.sh| 18 +-
 t/t7506-status-submodule.sh  | 24 
 wt-status.c  | 13 +++--
 4 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
 !   !ignored
 -
 
+Submodules have more state and instead report
+   Mthe submodule has a different HEAD than
+recorded in the index
+   mthe submodule has modified content
+   ?the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
 ## branchname tracking info
@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames 
containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single 
`?`.
+
 Porcelain Format Version 2
 ~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -268,6 +268,14 @@ cat >expect.modified actual &&
@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with 
untracked files fails unle
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified actual &&
+   test_cmp expect.modified_untracked actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule 
with different nested HE
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified actual &&
+   test_cmp expect.modified_inside actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule 
with nested modification
test -d submod &&
test -f submod/.git &&
git 

[PATCH 2/3] submodule.c, is_submodule_modified: stricter checking for submodules

[Stefan Beller]

By having a stricter check in the superproject we catch errors earlier,
instead of spawning a child process to tell us.

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

diff --git a/submodule.c b/submodule.c
index d355ddb46b..66335159ae 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1050,11 +1050,12 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
git_dir = read_gitfile(buf.buf);
if (!git_dir)
git_dir = buf.buf;
-   if (!is_directory(git_dir)) {
+   if (!is_git_directory(git_dir)) {
+   if (is_directory(git_dir))
+   die(_("'%s' not recognized as a git repository"), 
git_dir);
strbuf_release();
/* The submodule is not checked out, so it is not modified */
return 0;
-
}
strbuf_reset();
 
-- 
2.12.1.432.gfe308fe33c.dirty

[PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

Migrate 'is_submodule_modified' to the new porcelain format of
git-status.

As the old porcelain only reported ' M' for submodules, no
matter what happened inside the submodule (untracked files,
changes to tracked files or move of HEAD), the new API
properly reports the different scenarios.

In a followup patch we will make use of these finer grained
reporting for git-status.

Signed-off-by: Stefan Beller 
---
 submodule.c | 53 -
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..d355ddb46b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1041,17 +1041,9 @@ int fetch_populated_submodules(const struct argv_array 
*options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-   ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
-   const char *argv[] = {
-   "status",
-   "--porcelain",
-   NULL,
-   NULL,
-   };
struct strbuf buf = STRBUF_INIT;
unsigned dirty_submodule = 0;
-   const char *line, *next_line;
const char *git_dir;
 
strbuf_addf(, "%s/.git", path);
@@ -1066,42 +1058,45 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
}
strbuf_reset();
 
+   argv_array_pushl(, "status", "--porcelain=2", NULL);
if (ignore_untracked)
-   argv[2] = "-uno";
+   argv_array_push(, "-uno");
 
-   cp.argv = argv;
prepare_submodule_repo_env(_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
cp.out = -1;
cp.dir = path;
if (start_command())
-   die("Could not run 'git status --porcelain' in submodule %s", 
path);
+   die("Could not run 'git status --porcelain=2' in submodule %s", 
path);
 
-   len = strbuf_read(, cp.out, 1024);
-   line = buf.buf;
-   while (len > 2) {
-   if ((line[0] == '?') && (line[1] == '?')) {
+   while (strbuf_getwholeline_fd(, cp.out, '\n') != EOF) {
+   /* regular untracked files */
+   if (buf.buf[0] == '?')
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-   if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-   break;
-   } else {
-   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-   if (ignore_untracked ||
-   (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-   break;
+
+   /* regular unmerged and renamed files */
+   if (buf.buf[0] == 'u' ||
+   buf.buf[0] == '1' ||
+   buf.buf[0] == '2') {
+   if (buf.buf[5] == 'S') {
+   /* nested submodule handling */
+   if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
+   dirty_submodule |= 
DIRTY_SUBMODULE_MODIFIED;
+   if (buf.buf[8] == 'U')
+   dirty_submodule |= 
DIRTY_SUBMODULE_UNTRACKED;
+   } else
+   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
}
-   next_line = strchr(line, '\n');
-   if (!next_line)
-   break;
-   next_line++;
-   len -= (next_line - line);
-   line = next_line;
+
+   if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&
+   dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+   break;
}
close(cp.out);
 
if (finish_command())
-   die("'git status --porcelain' failed in submodule %s", path);
+   die("'git status --porcelain=2' failed in submodule %s", path);
 
strbuf_release();
return dirty_submodule;
-- 
2.12.1.432.gfe308fe33c.dirty

[PATCHv3 0/3] short status: improve reporting for submodule changes

[Stefan Beller]

This comes as a series; first I'd like to refactor is_submodule_modified
to take advantage of the new porcelain=2 plumbing switch to check for changes
in the submodule.

On top of the refactoring comes the actual change, which moved the
rewriting of the submodule change indicator letter to the collection part.

Thanks,
Stefan


Stefan Beller (3):
  submodule.c: port is_submodule_modified to use porcelain 2
  submodule.c, is_submodule_modified: stricter checking for submodules
  short status: improve reporting for submodule changes

 Documentation/git-status.txt |  9 +++
 submodule.c  | 58 +---
 t/t3600-rm.sh| 18 ++
 t/t7506-status-submodule.sh  | 24 ++
 wt-status.c  | 13 --
 5 files changed, 84 insertions(+), 38 deletions(-)

-- 
2.12.1.432.gfe308fe33c.dirty

blame --line-porcelain is providing me with funny output

[Edmundo Carmona Antoranz]

Hi, everybody!

As part of my improvements to difflame I want to use revision
information as provided by blame --line-porcelain so that I can avoid
some git calls to cat-file -p to get revision information hoping that
information would be a match. However I'm not finding that to be the
case.

$ git blame --no-progress -w --line-porcelain -L 72,72
4e59582ff70d299f5a88449891e78d15b4b3fabe -- t/lib-submodule-update.sh

3290fe6dd2a7e2bb35ac760443335dec58802ff1 72 72 1
author Stefan Beller
author-mail 
author-time 1484160452
author-tz -0800
committer Junio C Hamano
committer-mail 
committer-time 1484337771
committer-tz -0800
summary lib-submodule-update.sh: reduce use of subshell by using "git -C"
previous d7dffce1cebde29a0c4b309a79e4345450bf352a t/lib-submodule-update.sh
filename t/lib-submodule-update.sh
   git -C sub1 checkout modifications &&



If we then take a look at the information on that revision using
cat-file -p on the revision:

$ git cat-file -p 3290fe6dd2a7e2bb35ac760443335dec58802ff1


tree 7df89dad28ec8b08875395265a3f2e13ba180174
parent d7dffce1cebde29a0c4b309a79e4345450bf352a
author Stefan Beller  1484160452 -0800
committer Junio C Hamano  1484337771 -0800

(which, just in case, resembles the information provided by git
show... but is simpler to parse with cat-file).

Committer mails are matching, however author mail does not match
between line-porcelain and cat-file. Is there a reason for that?


Thanks in advance.

Re: How do I copy a branch & its config to a new name?

[Jeff King]

On Thu, Mar 23, 2017 at 12:54:48AM +0100, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Mar 22, 2017 at 11:58 PM, Jeff King  wrote:
> > On Wed, Mar 22, 2017 at 11:46:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >
> >> But both of these are really just a limited special case for what I'd
> >> really like, which is given branch "foo", copy it and all its
> >> configuration to a new name "bar". I.e. both of the hacks above only
> >> set up the correct tracking info, but none of the other branch.*
> >> variables I may have set.
> >
> > I thought that's what "git branch -m" was for, though I don't know how
> > thorough it is about finding config that refers _to_ your branch (I know
> > it handles config _for_ your branch).
> >
> > There might be room for improvement there.
> 
> -m can only rename "foo" -> "bar", but I'd like to end up with a new
> "bar" with the same config as "foo"

Ah, I see. You really want "copy". In that case, yeah, I think you'd
need a new command. It might make sense for it to share implementation
with rename (and just do the "add new config" part, skipping the "drop
old config" half).

-Peff

Re: How do I copy a branch & its config to a new name?

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 22, 2017 at 11:58 PM, Jeff King  wrote:
> On Wed, Mar 22, 2017 at 11:46:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> But both of these are really just a limited special case for what I'd
>> really like, which is given branch "foo", copy it and all its
>> configuration to a new name "bar". I.e. both of the hacks above only
>> set up the correct tracking info, but none of the other branch.*
>> variables I may have set.
>
> I thought that's what "git branch -m" was for, though I don't know how
> thorough it is about finding config that refers _to_ your branch (I know
> it handles config _for_ your branch).
>
> There might be room for improvement there.

-m can only rename "foo" -> "bar", but I'd like to end up with a new
"bar" with the same config as "foo"

Re: [PATCH v2 10/16] tag: change misleading --list documentation

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 22, 2017 at 11:36 PM, Jeff King  wrote:
> On Wed, Mar 22, 2017 at 03:26:21PM -0700, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  writes:
>>
>> > of things you think we should be putting in the test suite. I.e.
>> > should the tests be:
>> >
>> > a) Only be a collection of invocations of git we'd be comfortable
>> > showing to someone as "this works, and this is how you should do it",
>> > or things that explicitly fail marked with test_must_fail.
>> >
>> > b) or a) && also various surprising combinations of things we don't
>> > necessarily want to encourage or even support in the future, but which
>> > are in there so if we change them, we at least know our change changed
>> > something that worked before.
>>
>> I am strongly inclined to (a).  If we cannot decide when we designed
>> the feature, and we anticipate that we may want to change it later,
>> then documenting the choice in a test or two may be a way to remind
>> the choice we happened to have made, but in general I do not think
>> we want to promise (to ourselves) more than what we are willing to
>> commit to.
>
> I've occasionally[1] added tests that are "what we happen to produce
> now", but I almost always mark them with a comment either in the test
> script or in the commit message.  What I'm _most_ concerned about is a
> developer later breaking the test, but being unsure if they were
> breaking some real-world case (and not being able to find clues in the
> history).
>
> A secondary concern would be people using the test snippets as guidance
> on what is normal or encouraged.
>
> So I could live with these patches, but I'd prefer to see a comment
> somewhere. And I think I'd have a slight inclination to just stick to
> (a) in the first place, unless there is a really good reason to cover
> the test (like that we do not care between behaviors X and Y, but we
> need to check that it does one of them, and not Z).

Right, or in this case something where we're testing for behavior we
documented for a long time, but never really intended to support.
Junio would you be fine with just this on top:

diff --git a/t/README b/t/README
index 4982d1c521..9e079a360a 100644
--- a/t/README
+++ b/t/README
@@ -379,2 +379,5 @@ Do:

+ - Include tests which assert that the desired & recommended behavior
+   of commands is preserved.
+
  - Put all code inside test_expect_success and other assertions.
@@ -424,2 +427,17 @@ Don't:

+ - Include tests which exhaustively test for various edge cases or
+   unintended emergent behavior which we're not interested in
+   supporting in the future.
+
+   An exception to this is are cases where we don't care about
+   different behaviors X and Y, but we need to check that it does one
+   of them, and not Z.
+
+   Another exception are cases where our documentation might
+   unintentionally stated or implied that something was supported or
+   recommended, but we'd like to discourage its use going forward.
+
+   In both of the above cases please prominently comment the test
+   indicating that you're testing for one of these two cases.
+
  - exit() within a 

git svn propset and multi-line svn:externals property

[Craig McQueen]

I'd like to be able to set the svn:externals property using "git svn propset". 
However, that property can be multi-line. I've not had any success in setting 
this property because git svn doesn't seem to handle the newlines.

Is it possible to set multi-line SVN properties somehow? Or could this be a 
future enhancement?

-- 
Craig McQueen

git svn and SVN property svn:original-date

[Craig McQueen]

When doing "git svn dcommit", the SVN revision just has the date/time stamp of 
the time of the dcommit.

Apparently SVN revisions can have an "svn:original-date" property, which would 
be good to set on dcommit, to preserve the timestamp from the git repository.

https://subversion.apache.org/docs/api/1.7/group__svn__props__revision__props.html#ga8f17351dd056149da9cb490f1daf4018
"The svn:date property must be monotonically increasing, along with the 
revision number. In certain scenarios, this may pose a problem when the 
revision represents a commit that occurred at a time which does not fit within 
the sequencing required for svn:date. This can happen, for instance, when the 
revision represents a commit to a foreign version control system, or possibly 
when two Subversion repositories are combined. This property 
[svn:original-date] can be used to record the TRUE, original date of the 
commit."

-- 
Craig McQueen

Re: How do I copy a branch & its config to a new name?

[Jeff King]

On Wed, Mar 22, 2017 at 11:46:14PM +0100, Ævar Arnfjörð Bjarmason wrote:

> But both of these are really just a limited special case for what I'd
> really like, which is given branch "foo", copy it and all its
> configuration to a new name "bar". I.e. both of the hacks above only
> set up the correct tracking info, but none of the other branch.*
> variables I may have set.

I thought that's what "git branch -m" was for, though I don't know how
thorough it is about finding config that refers _to_ your branch (I know
it handles config _for_ your branch).

There might be room for improvement there.

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

On Wed, Mar 22, 2017 at 06:41:24PM -0400, Jeff King wrote:
> > In that case, something like this would be closer to the desired
> > behavior?
> 
> Yes, though you can spell:
> 
>   cat >expect <<-\EOF
>   EOF
> 
> as just:
> 
>   >expect

Ah, that sounds like a better way to fix this with a smaller diff.
> 
> > I'm also unsure on what would be the right thing to put on the commit
> > message.
> 
> I think the argument is:
> 
>   1. It's safer not to expound on tags that have failed verification (so
>  that the caller cannot accidentally use them). Especially since the
>  --format cannot tell anything about the GPG status.
> 
>  That means that
> 
>tag=$(git verify-tag --format='%(tag)' foo)
> 
>  can use a non-blank $tag without having to wonder whether it is
>  valid or not.
> 
> and
> 
>   2. That's what we've done since the feature was released.
> 
> The only thing that would give me pause is if were to later add
> %G-like formatters, and then:
> 
>   xargs git verify-tag --format='%(gpg:status) %(tag)' |
>   while read status tag
>   do
>  ...
>   done
> 
> would become useful, but we'd be tied to the behavior that we omit the
> tag when the gpg verification failed (for backwards compatibility).
> OTOH, we could perhaps make the rule "ignored unless %(gpg) formatters
> are used". Which would be backwards-compatible and safe for old formats,
> and work correctly for new ones.

This sounds like a helpful addition to implement. We could update/add
tests for compliance on this once the feature is addded and fix the
ambiguous behavior in the tests now.

Thanks,
-Santiago.

---
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f..0581053a0 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -896,17 +896,15 @@ test_expect_success GPG 'verifying a forged tag should 
fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : signed-tag
-   EOF &&
+   EOF
git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
-   tagname : forged-tag
-   EOF &&
+test_expect_success 'verifying a forged tag with --format should fail 
silently' '
+   >expect &&
test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" 
>actual &&
test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98..173a88e89 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,15 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : fourth-signed
-   EOF &&
+   EOF
git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
-   tagname : 7th forged-signed
-   EOF &&
+test_expect_success 'verifying a forged tag with --format should fail 
silently' '
+   >expect &&
test_must_fail git verify-tag --format="tagname : %(tag)" $(cat 
forged1.tag) >actual-forged &&
test_cmp expect actual-forged
 '


signature.asc
Description: PGP signature

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King  writes:

> OTOH, we could perhaps make the rule "ignored unless %(gpg) formatters
> are used". Which would be backwards-compatible and safe for old formats,
> and work correctly for new ones.

Yup, that is a very sensible escape hatch that we can use later.

Thanks.

How do I copy a branch & its config to a new name?

[Ævar Arnfjörð Bjarmason]

When I start work on a new series I have e.g. avar/various-tag-2 with
its upstream info set to origin/master.

When I start avar/various-tag-3 just:

git checkcout -b avar/various-tag-3 avar/various-tag-2

I don't get the correct tracking info. As far as I can tell there's no
way to do this in a single command, either you do:

git checkout -b avar/various-tag-4 avar/various-tag-3 && git
branch --set-upstream-to origin/master

Or:

git checkout -b avar/various-tag-3 -t origin/master && git reset
--hard avar/various-tag-2

But both of these are really just a limited special case for what I'd
really like, which is given branch "foo", copy it and all its
configuration to a new name "bar". I.e. both of the hacks above only
set up the correct tracking info, but none of the other branch.*
variables I may have set.

So I have this relative horror-show as an alias:

$ git config alias.cp-branch
!f() { git checkout -b $2 $1 && for line in $(git config --list
--file .git/config | grep "^branch\.$1"); do key=$(echo $line | cut
-d= -f1); newkey=$(echo $key | sed "s!$1!$2!"); value=$(echo $line |
cut -d= -f2); git config $newkey $value; done; }; f

Turned into a more readable tiny shell-script you can call git-cp-branch that's:

#!/bin/sh -e
git checkout -b $2 $1
for line in $(git config --list --file $(git rev-parse
--git-dir)/config | grep "^branch\.$1");
do
key=$(echo $line | cut -d= -f1)
newkey=$(echo $key | sed "s!$1!$2!")
value=$(echo $line | cut -d= -f2)
git config $newkey $value
done

I couldn't find any previous list discussion about this, but if not I
think something like:

git [checkout|branch] --copy src dest

Would make sense as an interface for this.

Re: [PATCH 0/3] fix "here-doc" syntax errors

[Jan Palus]

On 22.03.2017 13:08, Junio C Hamano wrote:
> Please respond to the list, saying it is OK to add your "sign-off"
> (see Documentation/SubmittingPatches) to these patches.

Please feel free to do so and thanks for handling the issue.


Regards
Jan

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 06:34:43PM -0400, Santiago Torres wrote:

> > I worked up the patch to do that (see below), but I got stumped trying
> > to write the commit message. Perhaps that is what the test intended, but
> > I don't think tag's --format understands "%G" codes at all. So you
> > cannot tell from the output if a tag was valid or not; you have to check
> > the exit code separately.
> > 
> > And if you do something like:
> > 
> >   git tag -v --format='%(tag)' foo bar |
> >   while read tag
> >   do
> >  ...
> >   done
> > 
> > you cannot tell at all which ones are bogus. Whereas with the current
> > behavior, the bogus ones are quietly omitted. Which can also be
> > confusing, but I'd think would generally err on the side of caution.
> 
> In that case, something like this would be closer to the desired
> behavior?

Yes, though you can spell:

  cat >expect <<-\EOF
  EOF

as just:

  >expect

> I'm also unsure on what would be the right thing to put on the commit
> message.

I think the argument is:

  1. It's safer not to expound on tags that have failed verification (so
 that the caller cannot accidentally use them). Especially since the
 --format cannot tell anything about the GPG status.

 That means that

   tag=$(git verify-tag --format='%(tag)' foo)

 can use a non-blank $tag without having to wonder whether it is
 valid or not.

and

  2. That's what we've done since the feature was released.

The only thing that would give me pause is if were to later add
%G-like formatters, and then:

  xargs git verify-tag --format='%(gpg:status) %(tag)' |
  while read status tag
  do
 ...
  done

would become useful, but we'd be tied to the behavior that we omit the
tag when the gpg verification failed (for backwards compatibility).
OTOH, we could perhaps make the rule "ignored unless %(gpg) formatters
are used". Which would be backwards-compatible and safe for old formats,
and work correctly for new ones.

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King  writes:

> I worked up the patch to do that (see below), but I got stumped trying
> to write the commit message. Perhaps that is what the test intended, but
> I don't think tag's --format understands "%G" codes at all.
> So you cannot tell from the output if a tag was valid or not; you have to 
> check
> the exit code separately.

Thanks for digging; that was an unexpected show-stopper to me X-<.

> you cannot tell at all which ones are bogus. Whereas with the current
> behavior, the bogus ones are quietly omitted. Which can also be
> confusing, but I'd think would generally err on the side of caution.

OK.

Re: Warn user about known Git Rebase bug?!

[Jonathan Nieder]

Lars Schneider wrote:

> we rebased a branch using "--preserve-merges --interactive" and were 
> surprised that the operation reported success although it silently 
> omitted commits (Git 2.12 on Windows). A little search revealed that 
> these are likely known bugs [1]. Would it make sense to print an 
> appropriate warning message if a user runs rebase with 
> "--preserve-merges --interactive"?

Sounds like a good idea.  Care to suggest a patch?

Thanks,
Jonathan

> [1] https://git-scm.com/docs/git-rebase#_bugs

Re: [PATCH v2 10/16] tag: change misleading --list documentation

[Jeff King]

On Wed, Mar 22, 2017 at 03:26:21PM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  writes:
> 
> > of things you think we should be putting in the test suite. I.e.
> > should the tests be:
> >
> > a) Only be a collection of invocations of git we'd be comfortable
> > showing to someone as "this works, and this is how you should do it",
> > or things that explicitly fail marked with test_must_fail.
> >
> > b) or a) && also various surprising combinations of things we don't
> > necessarily want to encourage or even support in the future, but which
> > are in there so if we change them, we at least know our change changed
> > something that worked before.
> 
> I am strongly inclined to (a).  If we cannot decide when we designed
> the feature, and we anticipate that we may want to change it later,
> then documenting the choice in a test or two may be a way to remind
> the choice we happened to have made, but in general I do not think
> we want to promise (to ourselves) more than what we are willing to
> commit to.

I've occasionally[1] added tests that are "what we happen to produce
now", but I almost always mark them with a comment either in the test
script or in the commit message.  What I'm _most_ concerned about is a
developer later breaking the test, but being unsure if they were
breaking some real-world case (and not being able to find clues in the
history).

A secondary concern would be people using the test snippets as guidance
on what is normal or encouraged.

So I could live with these patches, but I'd prefer to see a comment
somewhere. And I think I'd have a slight inclination to just stick to
(a) in the first place, unless there is a really good reason to cover
the test (like that we do not care between behaviors X and Y, but we
need to check that it does one of them, and not Z).

-Peff

[1] E.g., see the comment in t3204 from a356e8e2a (t3204: test
git-branch @-expansion corner cases, 2017-03-02).

Re: [PATCH 1/3] t/README: link to metacpan.org, not search.cpan.org

[Junio C Hamano]

Thanks.

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

> I worked up the patch to do that (see below), but I got stumped trying
> to write the commit message. Perhaps that is what the test intended, but
> I don't think tag's --format understands "%G" codes at all. So you
> cannot tell from the output if a tag was valid or not; you have to check
> the exit code separately.
> 
> And if you do something like:
> 
>   git tag -v --format='%(tag)' foo bar |
>   while read tag
>   do
>  ...
>   done
> 
> you cannot tell at all which ones are bogus. Whereas with the current
> behavior, the bogus ones are quietly omitted. Which can also be
> confusing, but I'd think would generally err on the side of caution.

In that case, something like this would be closer to the desired
behavior?

I'm also unsure on what would be the right thing to put on the commit
message.

-Santiago.

---
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f..70f6d4646 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -896,19 +896,18 @@ test_expect_success GPG 'verifying a forged tag should 
fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : signed-tag
-   EOF &&
+   EOF
git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
+test_expect_success 'verifying a forged tag with --format should fail 
silently' '
+   cat >expect <<-\EOF &&
tagname : forged-tag
-   EOF &&
-   test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" 
>actual &&
-   test_cmp expect actual
+   EOF
+   test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag"
 '
 
 # blank and empty messages for signed tags:
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98..ba0aafa1f 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,19 +126,18 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : fourth-signed
-   EOF &&
+   EOF
git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
+test_expect_success 'verifying a forged tag with --format should fail 
silently' '
+   cat >expect <<-\EOF &&
tagname : 7th forged-signed
-   EOF &&
-   test_must_fail git verify-tag --format="tagname : %(tag)" $(cat 
forged1.tag) >actual-forged &&
-   test_cmp expect actual-forged
+   EOF
+   test_must_fail git verify-tag --format="tagname : %(tag)" $(cat 
forged1.tag)
 '
 
 test_done


signature.asc
Description: PGP signature

Re: [PATCH v2 0/2] push options across http

[Jonathan Nieder]

Brandon Williams wrote:

> v2 addresses Jonathan's comments from v1.
>   * Fix a test
>   * Add some documentation
>   * remove short option from --push-option in git send-pack
> 
> Brandon Williams (2):
>   send-pack: send push options correctly in stateless-rpc case
>   remote-curl: allow push options
> 
>  Documentation/git-send-pack.txt |  6 ++
>  builtin/send-pack.c |  5 +
>  remote-curl.c   |  8 
>  send-pack.c | 20 
>  t/t5545-push-options.sh | 33 +++--
>  5 files changed, 58 insertions(+), 14 deletions(-)

Reviewed-by: Jonathan Nieder 
Tested-by: Jonathan Nieder 

Thanks.

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Stefan Beller]

On Wed, Mar 22, 2017 at 3:24 PM, Jeff King  wrote:
> On Wed, Mar 22, 2017 at 03:12:07PM -0700, Junio C Hamano wrote:
>
>> >   sq=\'
>> >   test_expect_success '...' '
>> > cat >expect <<-EOF
>> > Execution of ${sq}false $submodulesha1${sq} ...
>> >   '
>> >
>> > but I'm not sure if that is any more readable.
>>
>> Yup, my eyes have long learned to coast over '\'' as an idiomatic
>> symbol, but I agree that it is harder to see until you get used to
>> it (and I do not think it is particularly useful skill to be able to
>> spot '\'' as a logical unit, either).  ${sq} thing may make it easier
>> to read but I think the one you did in the first quoted part in this
>> reply is good enough.
>
> Sounds good.
>
>> -- >8 --
>> Subject: t7406: correct test case for submodule-update initial population
>>
>> There are three issues with the test:
>>
>> * The syntax of the here-doc was wrong, such that the entire test was
>>   sucked into the here-doc, which is why the test succeeded successfully.
>
> This version looks fine except for the repetitious repetition. :)

When I wrote it, It sounded more convincing, but we can drop the
"successfully".

Thanks,
Stefan

Re: [PATCH v2 10/16] tag: change misleading --list documentation

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> of things you think we should be putting in the test suite. I.e.
> should the tests be:
>
> a) Only be a collection of invocations of git we'd be comfortable
> showing to someone as "this works, and this is how you should do it",
> or things that explicitly fail marked with test_must_fail.
>
> b) or a) && also various surprising combinations of things we don't
> necessarily want to encourage or even support in the future, but which
> are in there so if we change them, we at least know our change changed
> something that worked before.

I am strongly inclined to (a).  If we cannot decide when we designed
the feature, and we anticipate that we may want to change it later,
then documenting the choice in a test or two may be a way to remind
the choice we happened to have made, but in general I do not think
we want to promise (to ourselves) more than what we are willing to
commit to.

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Jeff King]

On Wed, Mar 22, 2017 at 03:12:07PM -0700, Junio C Hamano wrote:

> >   sq=\'
> >   test_expect_success '...' '
> > cat >expect <<-EOF
> > Execution of ${sq}false $submodulesha1${sq} ...
> >   '
> >
> > but I'm not sure if that is any more readable.
> 
> Yup, my eyes have long learned to coast over '\'' as an idiomatic
> symbol, but I agree that it is harder to see until you get used to
> it (and I do not think it is particularly useful skill to be able to
> spot '\'' as a logical unit, either).  ${sq} thing may make it easier
> to read but I think the one you did in the first quoted part in this
> reply is good enough.

Sounds good.

> -- >8 --
> Subject: t7406: correct test case for submodule-update initial population
> 
> There are three issues with the test:
> 
> * The syntax of the here-doc was wrong, such that the entire test was
>   sucked into the here-doc, which is why the test succeeded successfully.

This version looks fine except for the repetitious repetition. :)

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 06:15:57PM -0400, Santiago Torres wrote:

> > > Like 2/3, this one also produces test failures for me. It looks like
> > > "verify-tag" does not show a tag which has been forged. I'm not sure if
> > > that's intentional (and the test is wrong) or a bug.  +cc Santiago
> > 
> > It appears that the test expected a broken one to be shown, and my
> > reading of its log message is that the change expected --format= to
> > be used with %G? so that scripts can tell between pass and fail?  
> > 
> > So if I have to judge, the code becoming silent for a tag that does
> > not pass verification is not doing what the commit wanted it to do.
> > 
> 
> Yes, considering the test name is:
> 
> "verifying a forged tag with --format fail and format accordingly" 
> 
> It feels as if the behavior of verify-tag/tag -v is not the one
> intended. I could add two patches on top of those two commits.

I worked up the patch to do that (see below), but I got stumped trying
to write the commit message. Perhaps that is what the test intended, but
I don't think tag's --format understands "%G" codes at all. So you
cannot tell from the output if a tag was valid or not; you have to check
the exit code separately.

And if you do something like:

  git tag -v --format='%(tag)' foo bar |
  while read tag
  do
 ...
  done

you cannot tell at all which ones are bogus. Whereas with the current
behavior, the bogus ones are quietly omitted. Which can also be
confusing, but I'd think would generally err on the side of caution.

-Peff

---
diff --git a/builtin/tag.c b/builtin/tag.c
index fbb85ba3d..37e768db4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -107,19 +107,19 @@ static int verify_tag(const char *name, const char *ref,
  const unsigned char *sha1, const void *cb_data)
 {
int flags;
+   int ret;
const char *fmt_pretty = cb_data;
flags = GPG_VERIFY_VERBOSE;
 
if (fmt_pretty)
flags = GPG_VERIFY_OMIT_STATUS;
 
-   if (gpg_verify_tag(sha1, name, flags))
-   return -1;
+   ret = gpg_verify_tag(sha1, name, flags);
 
if (fmt_pretty)
pretty_print_ref(name, sha1, fmt_pretty);
 
-   return 0;
+   return ret;
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 5199553d9..cb1024ef4 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -62,10 +62,8 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
continue;
}
 
-   if (gpg_verify_tag(sha1, name, flags)) {
+   if (gpg_verify_tag(sha1, name, flags))
had_error = 1;
-   continue;
-   }
 
if (fmt_pretty)
pretty_print_ref(name, sha1, fmt_pretty);
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b53a2e5e4..633b08956 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -848,17 +848,17 @@ test_expect_success GPG 'verifying a forged tag should 
fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : signed-tag
-   EOF &&
+   EOF
git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : forged-tag
-   EOF &&
+   EOF
test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" 
>actual &&
test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98..ce37fd986 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,17 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : fourth-signed
-   EOF &&
+   EOF
git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : 7th forged-signed
-   EOF &&
+   EOF
test_must_fail git verify-tag --format="tagname : %(tag)" $(cat 
forged1.tag) >actual-forged &&
test_cmp expect actual-forged
 '

[PATCH v2 2/2] remote-curl: allow push options

[Brandon Williams]

Teach remote-curl to understand push options and to be able to convey
them across HTTP.

Signed-off-by: Brandon Williams 
---
 Documentation/git-send-pack.txt |  6 ++
 builtin/send-pack.c |  5 +
 remote-curl.c   |  8 
 t/t5545-push-options.sh | 33 +++--
 4 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index a831dd028..966abb0df 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a flush 
packet.
will also fail if the actual call to `gpg --sign` fails.  See
linkgit:git-receive-pack[1] for the details on the receiving end.
 
+--push-option=::
+   Pass the specified string as a push option for consumption by
+   hooks on the server side.  If the server doesn't support push
+   options, error out.  See linkgit:git-push[1] and
+   linkgit:githooks[5] for details.
+
 ::
A remote host to house the repository.  When this
part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 1ff5a6753..832fd7ed0 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
unsigned force_update = 0;
unsigned quiet = 0;
int push_cert = 0;
+   struct string_list push_options = STRING_LIST_INIT_NODUP;
unsigned use_thin_pack = 0;
unsigned atomic = 0;
unsigned stateless_rpc = 0;
@@ -165,6 +166,9 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
{ OPTION_CALLBACK,
  0, "signed", _cert, "yes|no|if-asked", N_("GPG sign the 
push"),
  PARSE_OPT_OPTARG, option_parse_push_signed },
+   OPT_STRING_LIST(0, "push-option", _options,
+   N_("server-specific"),
+   N_("option to transmit")),
OPT_BOOL(0, "progress", , N_("force progress 
reporting")),
OPT_BOOL(0, "thin", _thin_pack, N_("use thin pack")),
OPT_BOOL(0, "atomic", , N_("request atomic transaction 
on remote side")),
@@ -199,6 +203,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
args.use_thin_pack = use_thin_pack;
args.atomic = atomic;
args.stateless_rpc = stateless_rpc;
+   args.push_options = push_options.nr ? _options : NULL;
 
if (from_stdin) {
struct argv_array all_refspecs = ARGV_ARRAY_INIT;
diff --git a/remote-curl.c b/remote-curl.c
index 34a97e732..e953d06f6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -22,6 +22,7 @@ struct options {
unsigned long depth;
char *deepen_since;
struct string_list deepen_not;
+   struct string_list push_options;
unsigned progress : 1,
check_self_contained_and_connected : 1,
cloning : 1,
@@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)
else
return -1;
return 0;
+   } else if (!strcmp(name, "push-option")) {
+   string_list_append(_options, value);
+   return 0;
 
 #if LIBCURL_VERSION_NUM >= 0x070a08
} else if (!strcmp(name, "family")) {
@@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, 
char **specs)
argv_array_push(, "--quiet");
else if (options.verbosity > 1)
argv_array_push(, "--verbose");
+   for (i = 0; i < options.push_options.nr; i++)
+   argv_array_pushf(, "--push-option=%s",
+options.push_options.items[i].string);
argv_array_push(, options.progress ? "--progress" : 
"--no-progress");
for_each_string_list_item(cas_option, _options)
argv_array_push(, cas_option->string);
@@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)
options.progress = !!isatty(2);
options.thin = 1;
string_list_init(_not, 1);
+   string_list_init(_options, 1);
 
remote = remote_get(argv[1]);
 
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 9a57a7d8f..97065e62b 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -102,17 +102,46 @@ test_expect_success 'two push options work' '
test_cmp expect upstream/.git/hooks/post-receive.push_options
 '
 
-test_expect_success 'push option denied properly by http remote helper' '\
+test_expect_success 'push option denied properly by http server' '
+   test_when_finished "rm -rf test_http_clone" &&
+   test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" 
&&
mk_repo_pair &&
git -C upstream 

[PATCH v2 1/2] send-pack: send push options correctly in stateless-rpc case

[Brandon Williams]

"git send-pack --stateless-rpc" puts each request in a sequence of pkt-lines
followed by a flush-pkt. The push option code forgot about this and sends push
options and their terminating delimiter as ordinary pkt-lines that get their
length header stripped off by remote-curl before being sent to the server.

The result is multiple malformed requests, which the server rejects.

Fortunately send-pack --stateless-rpc already is aware of this "pkt-line within
pkt-line" framing for the update commands that precede push options. Handle
push options the same way.

Helped-by: Jonathan Nieder 
Signed-off-by: Brandon Williams 
---
 send-pack.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index d2d2a49a0..66e652f7e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -532,6 +532,14 @@ int send_pack(struct send_pack_args *args,
}
}
 
+   if (use_push_options) {
+   struct string_list_item *item;
+
+   packet_buf_flush(_buf);
+   for_each_string_list_item(item, args->push_options)
+   packet_buf_write(_buf, "%s", item->string);
+   }
+
if (args->stateless_rpc) {
if (!args->dry_run && (cmds_sent || is_repository_shallow())) {
packet_buf_flush(_buf);
@@ -544,18 +552,6 @@ int send_pack(struct send_pack_args *args,
strbuf_release(_buf);
strbuf_release(_buf);
 
-   if (use_push_options) {
-   struct string_list_item *item;
-   struct strbuf sb = STRBUF_INIT;
-
-   for_each_string_list_item(item, args->push_options)
-   packet_buf_write(, "%s", item->string);
-
-   write_or_die(out, sb.buf, sb.len);
-   packet_flush(out);
-   strbuf_release();
-   }
-
if (use_sideband && cmds_sent) {
memset(, 0, sizeof(demux));
demux.proc = sideband_demux;
-- 
2.12.1.500.gab5fba24ee-goog

[PATCH v2 0/2] push options across http

[Brandon Williams]

v2 addresses Jonathan's comments from v1.
  * Fix a test
  * Add some documentation
  * remove short option from --push-option in git send-pack

Brandon Williams (2):
  send-pack: send push options correctly in stateless-rpc case
  remote-curl: allow push options

 Documentation/git-send-pack.txt |  6 ++
 builtin/send-pack.c |  5 +
 remote-curl.c   |  8 
 send-pack.c | 20 
 t/t5545-push-options.sh | 33 +++--
 5 files changed, 58 insertions(+), 14 deletions(-)

-- 
2.12.1.500.gab5fba24ee-goog

Re: [PATCH 1/2] send-pack: send push options correctly in stateless-rpc case

[Brandon Williams]

On 03/22, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> > "git send-pack --stateless-rpc" puts each request in a sequence of pkt-lines
> > followed by a flush-pkt. The push option code forgot about this and sends 
> > push
> > options and their terminating delimiter as ordinary pkt-lines that get their
> > length header stripped off by remote-curl before being sent to the server.
> >
> > The result is multiple malformed requests, which the server rejects.
> >
> > Fortunately send-pack --stateless-rpc already is aware of this "pkt-line 
> > within
> > pkt-line" framing for the update commands that precede push options. Handle
> > push options the same way.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  send-pack.c | 20 
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> This is only a hypothetical issue until the next patch though, right?

Correct, Its in preparation for the next patch.

> 
> For what it's worth,
> 
> Tested-by: Jonathan Nieder 
> Reviewed-by: Jonathan Nieder 
> 
> Thanks.

-- 
Brandon Williams

[PATCH 2/3] t/README: change "Inside part" to "Inside the part"

[Ævar Arnfjörð Bjarmason]

Change the mention of "Inside the 

[PATCH 3/3] t/README: clarify the test_have_prereq documentation

[Ævar Arnfjörð Bjarmason]

Clarify the test_have_prereq documentation so that it's clear in the
reader's mind when the text says "most common use of this directly"
what the answer to "as opposed to what?" is.

Usually this function isn't used in lieu of using the prerequisite
support built into test_expect_*, mention that explicitly.

This changes documentation that I added in commit
9a897893a7 ("t/README: Document the prereq functions, and 3-arg
test_*", 2010-07-02).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/README | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/README b/t/README
index 89512e23d4..36813cef94 100644
--- a/t/README
+++ b/t/README
@@ -612,8 +612,10 @@ library for your script to use.
  - test_have_prereq 
 
Check if we have a prerequisite previously set with
-   test_set_prereq. The most common use of this directly is to skip
-   all the tests if we don't have some essential prerequisite:
+   test_set_prereq. The most common use-case for using this directly,
+   as opposed to as an argument to test_expect_*, is to skip all the
+   tests at the start of the test script if we don't have some
+   essential prerequisite:
 
if ! test_have_prereq PERL
then
-- 
2.11.0

[PATCH 1/3] t/README: link to metacpan.org, not search.cpan.org

[Ævar Arnfjörð Bjarmason]

Change a link to the web version of the TAP::Parser::Grammar
documentation to link to metacpan.org instead of search.cpan.org.

This is something I added back in commit 20873f45e7 ("t/README:
Document the do's and don'ts of tests", 2010-07-02), at the time
search.cpan.org was the more actively maintained CPAN web-interface,
nowadays that's metacpan.org.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/README | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 4982d1c521..a50290c789 100644
--- a/t/README
+++ b/t/README
@@ -471,7 +471,7 @@ Don't:
their output.
 
You can glean some further possible issues from the TAP grammar
-   (see http://search.cpan.org/perldoc?TAP::Parser::Grammar#TAP_Grammar)
+   (see https://metacpan.org/pod/TAP::Parser::Grammar#TAP-GRAMMAR)
but the best indication is to just run the tests with prove(1),
it'll complain if anything is amiss.
 
-- 
2.11.0

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

On Wed, Mar 22, 2017 at 03:04:32PM -0700, Junio C Hamano wrote:
> Jeff King  writes:
> 
> > On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:
> >
> >> From: Jan Palus 
> >> 
> >> These all came as part of an earlier st/verify-tag topic that was
> >> merged to 2.12.
> >> 
> >> Signed-off-by: Junio C Hamano 
> >> ---
> >> 
> >>  * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
> >>--format specifier tests", 2017-01-17)
> >> 
> >>  t/t7004-tag.sh| 8 
> >>  t/t7030-verify-tag.sh | 8 
> >>  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > Like 2/3, this one also produces test failures for me. It looks like
> > "verify-tag" does not show a tag which has been forged. I'm not sure if
> > that's intentional (and the test is wrong) or a bug.  +cc Santiago
> 
> It appears that the test expected a broken one to be shown, and my
> reading of its log message is that the change expected --format= to
> be used with %G? so that scripts can tell between pass and fail?  
> 
> So if I have to judge, the code becoming silent for a tag that does
> not pass verification is not doing what the commit wanted it to do.
> 

Yes, considering the test name is:

"verifying a forged tag with --format fail and format accordingly" 

It feels as if the behavior of verify-tag/tag -v is not the one
intended. I could add two patches on top of those two commits.

Would this be enough?
-Santiago.


signature.asc
Description: PGP signature

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Jonathan Nieder]

Stefan Beller wrote:
> On Wed, Mar 22, 2017 at 2:59 PM, Jeff King  wrote:

>>   sq=\'
>>   test_expect_success '...' '
>> cat >expect <<-EOF
>> Execution of ${sq}false $submodulesha1${sq} ...
>>   '
>>
>> but I'm not sure if that is any more readable.
>
> If I recall correctly, I made a big fuss about single quotes used correctly 
> when
> writing that patch (which is why I may have lost track of the actual work 
> there)
> to be told the one and only blessed way to use single quotes in our test 
> suite.
>
> Your proposal to use ${sq} sounds good to me, though we did not
> follow through with it for some other reason. I can reroll with that, though.

I don't know what you're referring to by only and blessed way, but it
might be the following.  If you want to use single-quotes on a command
line using $sq, you would have to use eval:

eval "some_command ${sq}single-quoted argument${sq}"

which is generally more complicated than doing the '\'' dance:

some_command '\''single-quoted argument'\''

But the example in this thread is different. In the here-document you
are in an interpolated context so the ${sq} method or '\'' would work
equally well.

Hope that helps,
Jonathan

Re: [PATCH 2/2] remote-curl: allow push options

[Brandon Williams]

On 03/22, Jonathan Nieder wrote:

I agree with most of these changes,  I'll make them locally and send out
a reroll.

> Brandon Williams wrote:
> 
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const 
> > char *prefix)
> > int progress = -1;
> > int from_stdin = 0;
> > struct push_cas_option cas = {0};
> > +   struct string_list push_options = STRING_LIST_INIT_DUP;
> 
> It's safe for this to be NODUP, since the strings added to it live in
> argv.
> 
> >  
> > struct option options[] = {
> > OPT__VERBOSITY(),
> > @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const 
> > char *prefix)
> > OPT_BOOL(0, "stateless-rpc", _rpc, N_("use stateless 
> > RPC protocol")),
> > OPT_BOOL(0, "stdin", _stdin, N_("read refs from stdin")),
> > OPT_BOOL(0, "helper-status", _status, N_("print status 
> > from remote helper")),
> > +   OPT_STRING_LIST('o', "push-option", _options,
> > +   N_("server-specific"),
> > +   N_("option to transmit")),
> 
> Does this need the short-and-sweet option name 'o'?  For this command,
> I think I'd prefer giving it no short name.
> 
> Should this option be documented in the manpage, too?
> 
> [...]
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -22,6 +22,7 @@ struct options {
> > unsigned long depth;
> > char *deepen_since;
> > struct string_list deepen_not;
> > +   struct string_list push_options;
> > unsigned progress : 1,
> > check_self_contained_and_connected : 1,
> > cloning : 1,
> > @@ -139,6 +140,9 @@ static int set_option(const char *name, const char 
> > *value)
> > else
> > return -1;
> > return 0;
> > +   } else if (!strcmp(name, "push-option")) {
> > +   string_list_append(_options, value);
> > +   return 0;
> 
> push_options has strdup_strings enabled so this takes ownership of a
> copy of value.  Good.
> 
> [...]
> > --- a/t/t5545-push-options.sh
> > +++ b/t/t5545-push-options.sh
> > @@ -102,7 +102,9 @@ test_expect_success 'two push options work' '
> > test_cmp expect upstream/.git/hooks/post-receive.push_options
> >  '
> >  
> > -test_expect_success 'push option denied properly by http remote helper' '\
> > +test_expect_success 'push option denied properly by http server' '
> 
> Should this test use test_i18ngrep to check that the error message
> diagnoses the problem correctly instead of hitting an unrelated error
> condition?
> 
> [...]
> > @@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by 
> > http remote helper' '\
> > git -C test_http_clone push origin master
> >  '
> >  
> > +test_expect_success 'push options work properly across http' '
> 
> Nice.
> 
> Tested-by: Jonathan Nieder 
> 
> With whatever subset of the following changes seems sensible to you
> squashed in,
> Reviewed-by: Jonathan Nieder 
> 
> Thanks.
> 
> diff --git i/Documentation/git-send-pack.txt w/Documentation/git-send-pack.txt
> index a831dd0288..966abb0df8 100644
> --- i/Documentation/git-send-pack.txt
> +++ w/Documentation/git-send-pack.txt
> @@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a 
> flush packet.
>   will also fail if the actual call to `gpg --sign` fails.  See
>   linkgit:git-receive-pack[1] for the details on the receiving end.
>  
> +--push-option=::
> + Pass the specified string as a push option for consumption by
> + hooks on the server side.  If the server doesn't support push
> + options, error out.  See linkgit:git-push[1] and
> + linkgit:githooks[5] for details.
> +
>  ::
>   A remote host to house the repository.  When this
>   part is specified, 'git-receive-pack' is invoked via
> diff --git i/builtin/send-pack.c w/builtin/send-pack.c
> index 6796f33687..832fd7ed0a 100644
> --- i/builtin/send-pack.c
> +++ w/builtin/send-pack.c
> @@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>   unsigned force_update = 0;
>   unsigned quiet = 0;
>   int push_cert = 0;
> + struct string_list push_options = STRING_LIST_INIT_NODUP;
>   unsigned use_thin_pack = 0;
>   unsigned atomic = 0;
>   unsigned stateless_rpc = 0;
> @@ -152,7 +153,6 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>   int progress = -1;
>   int from_stdin = 0;
>   struct push_cas_option cas = {0};
> - struct string_list push_options = STRING_LIST_INIT_DUP;
>  
>   struct option options[] = {
>   OPT__VERBOSITY(),
> @@ -166,15 +166,15 @@ int cmd_send_pack(int argc, const char **argv, const 
> char *prefix)
>   { OPTION_CALLBACK,
> 0, "signed", _cert, "yes|no|if-asked", N_("GPG sign the 
> push"),
> 

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Junio C Hamano]

Jeff King  writes:

> Neither of those is true, but I think:
>
>   cat >expect <<-EOF &&
>   Execution of '\''false $submodulesha1'\'' failed in ...
>   EOF
>
> is safer and less surprising. The single-quote handling is unfortunate and
> ugly, but necessary to get them into the shell snippet in the first
> place. I notice the others tests in this script set up the expect file
> outside of a block. You could also do something like:
>
>   sq=\'
>   test_expect_success '...' '
>   cat >expect <<-EOF
>   Execution of ${sq}false $submodulesha1${sq} ...
>   '
>
> but I'm not sure if that is any more readable.

Yup, my eyes have long learned to coast over '\'' as an idiomatic
symbol, but I agree that it is harder to see until you get used to
it (and I do not think it is particularly useful skill to be able to
spot '\'' as a logical unit, either).  ${sq} thing may make it easier
to read but I think the one you did in the first quoted part in this
reply is good enough.

-- >8 --
Subject: t7406: correct test case for submodule-update initial population

There are three issues with the test:

* The syntax of the here-doc was wrong, such that the entire test was
  sucked into the here-doc, which is why the test succeeded successfully.

* The variable $submodulesha1 was not expanded as it was inside a quoted
  here text.  We do not want to quote EOF marker for this.

* The redirection from the git command to the output file for comparison
  was wrong as the -C operator from git doesn't apply to the redirect path.
  Also we're interested in stderr of that command.

Noticed-by: Jan Palus 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---

 t/t7406-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 8c086a429b..a70fe96ad6 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -425,11 +425,11 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
 '
 
 test_expect_success 'submodule update - command run for initial population of 
submodule' '
-   cat <<-\ EOF >expect
+   cat >expect <<-EOF &&
Execution of '\''false $submodulesha1'\'' failed in submodule path 
'\''submodule'\''
-   EOF &&
+   EOF
rm -rf super/submodule &&
-   test_must_fail git -C super submodule update >../actual &&
+   test_must_fail git -C super submodule update 2>actual &&
test_cmp expect actual &&
git -C super submodule update --checkout
 '

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Jeff King]

On Wed, Mar 22, 2017 at 03:07:45PM -0700, Stefan Beller wrote:

> > Neither of those is true, but I think:
> >
> >   cat >expect <<-EOF &&
> >   Execution of '\''false $submodulesha1'\'' failed in ...
> >   EOF
> >
> > is safer and less surprising. The single-quote handling is unfortunate and
> > ugly, but necessary to get them into the shell snippet in the first
> > place. I notice the others tests in this script set up the expect file
> > outside of a block. You could also do something like:
> >
> >   sq=\'
> >   test_expect_success '...' '
> > cat >expect <<-EOF
> > Execution of ${sq}false $submodulesha1${sq} ...
> >   '
> >
> > but I'm not sure if that is any more readable.
> >
> 
> If I recall correctly, I made a big fuss about single quotes used correctly 
> when
> writing that patch (which is why I may have lost track of the actual work 
> there)
> to be told the one and only blessed way to use single quotes in our test 
> suite.
> 
> Your proposal to use ${sq} sounds good to me, though we did not
> follow through with it for some other reason. I can reroll with that, though.

I'm not sure if it's a proposal. I ended it with "I'm not sure if that
is any more readable". :)

I mainly care about making sure the interpolation happens inside the
test block (i.e., at the top of the quoted section above).

-Peff

Re: [PATCH v2 10/16] tag: change misleading --list documentation

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 22, 2017 at 10:09 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
>>> Please drop "interleaved", as we are not encouraging GNUism.  We
>>> just tolerate it but do not guarantee to keep them working.
>>
>> This brings up the same point you made in
>> , I replied to in
>> 

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Stefan Beller]

On Wed, Mar 22, 2017 at 2:59 PM, Jeff King  wrote:
> On Wed, Mar 22, 2017 at 02:49:48PM -0700, Stefan Beller wrote:
>
>> * The syntax of the here-doc was wrong, such that the entire test was
>>   sucked into the here-doc, which is why the test succeeded successfully.
>
> As opposed to succeeding unsuccessfully? :)
>
>> * The variable $submodulesha1 was not expanded as it was inside a single
>>   quoted string. Use double quote to expand the variable.
>
> Hmm. Sort of. It was inside a non-interpolating here-doc inside a
> single-quoted string which was being eval'd. The second half is fine
> (the eval adds an extra layer of evaluation).
>
> Your fix:
>
>> + cat >expect <<-\EOF &&
>> + Execution of '\'"false $submodulesha1"\'' failed in submodule path 
>> '\''submodule'\''
>> + EOF
>
> _does_ work, but it does so because it's evaluating $submodulesha1 in
> the shell snippet and handing the result off to test_expect_success to
> eval. So it would have problems if:
>
>   - that variable contained "\nEOF\n" itself ;)
>
>   - the variable was modified inside the shell snippet.
>
> Neither of those is true, but I think:
>
>   cat >expect <<-EOF &&
>   Execution of '\''false $submodulesha1'\'' failed in ...
>   EOF
>
> is safer and less surprising. The single-quote handling is unfortunate and
> ugly, but necessary to get them into the shell snippet in the first
> place. I notice the others tests in this script set up the expect file
> outside of a block. You could also do something like:
>
>   sq=\'
>   test_expect_success '...' '
> cat >expect <<-EOF
> Execution of ${sq}false $submodulesha1${sq} ...
>   '
>
> but I'm not sure if that is any more readable.
>

If I recall correctly, I made a big fuss about single quotes used correctly when
writing that patch (which is why I may have lost track of the actual work there)
to be told the one and only blessed way to use single quotes in our test suite.

Your proposal to use ${sq} sounds good to me, though we did not
follow through with it for some other reason. I can reroll with that, though.

Thanks,
Stefan

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King  writes:

> On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:
>
>> From: Jan Palus 
>> 
>> These all came as part of an earlier st/verify-tag topic that was
>> merged to 2.12.
>> 
>> Signed-off-by: Junio C Hamano 
>> ---
>> 
>>  * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
>>--format specifier tests", 2017-01-17)
>> 
>>  t/t7004-tag.sh| 8 
>>  t/t7030-verify-tag.sh | 8 
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> Like 2/3, this one also produces test failures for me. It looks like
> "verify-tag" does not show a tag which has been forged. I'm not sure if
> that's intentional (and the test is wrong) or a bug.  +cc Santiago

It appears that the test expected a broken one to be shown, and my
reading of its log message is that the change expected --format= to
be used with %G? so that scripts can tell between pass and fail?  

So if I have to judge, the code becoming silent for a tag that does
not pass verification is not doing what the commit wanted it to do.

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 05:43:57PM -0400, Santiago Torres wrote:

> > Like 2/3, this one also produces test failures for me. It looks like
> > "verify-tag" does not show a tag which has been forged. I'm not sure if
> > that's intentional (and the test is wrong) or a bug. 
> 
> I see that offending code would be [1]. Changing this behavior should be
> trivial (dropping the continue), although I'm not sure if this is what
> we want?

I could see arguments for either behavior. The fact that v2.12 was
released with the skip-gpg-failures behavior makes me inclined to just
keep that and fix the test. But I don't feel strongly.

If we do change it, I think builtin/tag.c:verify_tag() would need a
similar fix (to avoid the early return).

-Peff

Re: USE_SHA1DC is broken in pu

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:

> As to the default of seriously slowing down all SHA-1 computations: since
> you made that the default, at compile time, with no way to turn on the
> faster computation, this will have a major, negative impact. Are you
> really, really sure you want to do that?
>
> I thought that it was obvious that we would have at least a runtime option
> to lessen the load.

It's not obvious to me.  I agree that the DC_SHA1 case can be sped up,
e.g. by turning off the collision detection for sha1 calculations that
are not part of fetching, receiving a push, or running fsck.

To be clear, are you saying that this is a bad compile-time default
because distributors are going to leave it and end-users will end up
with a bad experience?  Or are you saying distributors have no good
alternative to choose at compile time?  Or something else?

Is there some particular downstream that this breaks?

Thanks and hope that helps,
Jonathan

Re: [PATCH] t7406: correct test case for submodule-update initial population

[Jeff King]

On Wed, Mar 22, 2017 at 02:49:48PM -0700, Stefan Beller wrote:

> * The syntax of the here-doc was wrong, such that the entire test was
>   sucked into the here-doc, which is why the test succeeded successfully.

As opposed to succeeding unsuccessfully? :)

> * The variable $submodulesha1 was not expanded as it was inside a single
>   quoted string. Use double quote to expand the variable.

Hmm. Sort of. It was inside a non-interpolating here-doc inside a
single-quoted string which was being eval'd. The second half is fine
(the eval adds an extra layer of evaluation).

Your fix:

> + cat >expect <<-\EOF &&
> + Execution of '\'"false $submodulesha1"\'' failed in submodule path 
> '\''submodule'\''
> + EOF

_does_ work, but it does so because it's evaluating $submodulesha1 in
the shell snippet and handing the result off to test_expect_success to
eval. So it would have problems if:

  - that variable contained "\nEOF\n" itself ;)

  - the variable was modified inside the shell snippet.

Neither of those is true, but I think:

  cat >expect <<-EOF &&
  Execution of '\''false $submodulesha1'\'' failed in ...
  EOF

is safer and less surprising. The single-quote handling is unfortunate and
ugly, but necessary to get them into the shell snippet in the first
place. I notice the others tests in this script set up the expect file
outside of a block. You could also do something like:

  sq=\'
  test_expect_success '...' '
cat >expect <<-EOF
Execution of ${sq}false $submodulesha1${sq} ...
  '

but I'm not sure if that is any more readable.

-Peff

Re: [PATCH v2] builtin/describe: introduce --broken flag

[Jakub Narębski]

W dniu 21.03.2017 o 23:57, Stefan Beller pisze:

> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -30,9 +30,14 @@ OPTIONS
>   Commit-ish object names to describe.  Defaults to HEAD if omitted.
>  
>  --dirty[=]::
> - Describe the working tree.
> - It means describe HEAD and appends  (`-dirty` by
> - default) if the working tree is dirty.
> +--broken[=]::
> + Describe the state of the working tree.  When the working
> + tree matches HEAD, the output is the same as "git describe
> + HEAD".  If the working tree has local modification "-dirty"
> + is appended to it.  If a repository is corrupt and Git
> + cannot determine if there is local modification, Git will
> + error out, unless `--broken' is given, which appends
> + the suffix "-broken" instead.

The common description reads better... but unfortunately it lost
information about the optional parameter, namely .  The
'-dirty' is just the default for , and '-broken' is
the default for .

Maybe /the suffix "-broken"/ suffix ('-broken' by default)/
and similarly for "-dirty"?

Best,
-- 
Jakub Narębski

[PATCH] t7406: correct test case for submodule-update initial population

[Stefan Beller]

There are three issues with the test:

* The syntax of the here-doc was wrong, such that the entire test was
  sucked into the here-doc, which is why the test succeeded successfully.

* The variable $submodulesha1 was not expanded as it was inside a single
  quoted string. Use double quote to expand the variable.

* The redirection from the git command to the output file for comparison
  was wrong as the -C operator from git doesn't apply to the redirect path.
  Also we're interested in stderr of that command.

Noticed-by: Jan Palus 
Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---

This is just one patch (for bisectability)
it applies on e7b37caf4fe.

Thanks,
Stefan

 t/t7406-submodule-update.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 8c086a4..c327eb6 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -425,11 +425,11 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
 '
 
 test_expect_success 'submodule update - command run for initial population of 
submodule' '
-   cat <<-\ EOF >expect
-   Execution of '\''false $submodulesha1'\'' failed in submodule path 
'\''submodule'\''
-   EOF &&
+   cat >expect <<-\EOF &&
+   Execution of '\'"false $submodulesha1"\'' failed in submodule path 
'\''submodule'\''
+   EOF
rm -rf super/submodule &&
-   test_must_fail git -C super submodule update >../actual &&
+   test_must_fail git -C super submodule update 2>actual &&
test_cmp expect actual &&
git -C super submodule update --checkout
 '
-- 
2.10.2.50.g9d09a6e.dirty

Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec

[Brandon Williams]

On 03/21, Duy Nguyen wrote:
> On Sat, Mar 18, 2017 at 12:22 AM, Brandon Williams  wrote:
> > With these two pieces of information a child process can correctly
> > interpret the pathspecs provided by the user as well as being able to
> > properly format its output relative to the directory the user invoked
> > the original command from.
> 
> This part can stand alone as a separate patch right? It would help
> focus on the pathspec thingy first.

I guess it could probably be factored out, though it is necessary for it
to work.  The issue I was running into was that when no pathspecs were
given it would create a 'prefix' pathspec.  So if we were in directory
'dir/' the pathspec that would get created would be:

ps.match = "dir/"
ps.original = "dir/"

Since it also set the original field it messed up when the child tried
to interpret the pathspecs again.  I solved this by just passing the raw
pathspec through to the child.

> 
> > @@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const 
> > char *prefix)
> >  }
> >
> >  static void compile_submodule_options(const struct grep_opt *opt,
> > - const struct pathspec *pathspec,
> > + const char **argv,
> >   int cached, int untracked,
> >   int opt_exclude, int use_index,
> >   int pattern_type_arg)
> >  {
> > struct grep_pat *pattern;
> > -   int i;
> >
> > if (recurse_submodules)
> > argv_array_push(_options, "--recurse-submodules");
> 
> Side note. It would be awesome if you could make parse_options() (or a
> new function) do the reverse process: given a 'struct option' with
> valid data, spit out argv_array. Less worrying about git-grep having
> new option but not passed to subgrep by accident. You can have a new
> flag to tell it to ignore certain options if you don't want to pass
> all.

I thought about this for a second but didn't pursue it very far.  Mostly
because of how you would handle options with callback routines.  Maybe
if you want the option to be reversible you need to have an additional
callback routine to do the conversion?  I agree that having this sort of
functionality would be nice as it does save you from forgetting about
passing on options to a child process.

-- 
Brandon Williams

Re: [PATCH 2/2] remote-curl: allow push options

[Jonathan Nieder]

Brandon Williams wrote:

> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>   int progress = -1;
>   int from_stdin = 0;
>   struct push_cas_option cas = {0};
> + struct string_list push_options = STRING_LIST_INIT_DUP;

It's safe for this to be NODUP, since the strings added to it live in
argv.

>  
>   struct option options[] = {
>   OPT__VERBOSITY(),
> @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>   OPT_BOOL(0, "stateless-rpc", _rpc, N_("use stateless 
> RPC protocol")),
>   OPT_BOOL(0, "stdin", _stdin, N_("read refs from stdin")),
>   OPT_BOOL(0, "helper-status", _status, N_("print status 
> from remote helper")),
> + OPT_STRING_LIST('o', "push-option", _options,
> + N_("server-specific"),
> + N_("option to transmit")),

Does this need the short-and-sweet option name 'o'?  For this command,
I think I'd prefer giving it no short name.

Should this option be documented in the manpage, too?

[...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -22,6 +22,7 @@ struct options {
>   unsigned long depth;
>   char *deepen_since;
>   struct string_list deepen_not;
> + struct string_list push_options;
>   unsigned progress : 1,
>   check_self_contained_and_connected : 1,
>   cloning : 1,
> @@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)
>   else
>   return -1;
>   return 0;
> + } else if (!strcmp(name, "push-option")) {
> + string_list_append(_options, value);
> + return 0;

push_options has strdup_strings enabled so this takes ownership of a
copy of value.  Good.

[...]
> --- a/t/t5545-push-options.sh
> +++ b/t/t5545-push-options.sh
> @@ -102,7 +102,9 @@ test_expect_success 'two push options work' '
>   test_cmp expect upstream/.git/hooks/post-receive.push_options
>  '
>  
> -test_expect_success 'push option denied properly by http remote helper' '\
> +test_expect_success 'push option denied properly by http server' '

Should this test use test_i18ngrep to check that the error message
diagnoses the problem correctly instead of hitting an unrelated error
condition?

[...]
> @@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http 
> remote helper' '\
>   git -C test_http_clone push origin master
>  '
>  
> +test_expect_success 'push options work properly across http' '

Nice.

Tested-by: Jonathan Nieder 

With whatever subset of the following changes seems sensible to you
squashed in,
Reviewed-by: Jonathan Nieder 

Thanks.

diff --git i/Documentation/git-send-pack.txt w/Documentation/git-send-pack.txt
index a831dd0288..966abb0df8 100644
--- i/Documentation/git-send-pack.txt
+++ w/Documentation/git-send-pack.txt
@@ -81,6 +81,12 @@ be in a separate packet, and the list must end with a flush 
packet.
will also fail if the actual call to `gpg --sign` fails.  See
linkgit:git-receive-pack[1] for the details on the receiving end.
 
+--push-option=::
+   Pass the specified string as a push option for consumption by
+   hooks on the server side.  If the server doesn't support push
+   options, error out.  See linkgit:git-push[1] and
+   linkgit:githooks[5] for details.
+
 ::
A remote host to house the repository.  When this
part is specified, 'git-receive-pack' is invoked via
diff --git i/builtin/send-pack.c w/builtin/send-pack.c
index 6796f33687..832fd7ed0a 100644
--- i/builtin/send-pack.c
+++ w/builtin/send-pack.c
@@ -144,6 +144,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
unsigned force_update = 0;
unsigned quiet = 0;
int push_cert = 0;
+   struct string_list push_options = STRING_LIST_INIT_NODUP;
unsigned use_thin_pack = 0;
unsigned atomic = 0;
unsigned stateless_rpc = 0;
@@ -152,7 +153,6 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
int progress = -1;
int from_stdin = 0;
struct push_cas_option cas = {0};
-   struct string_list push_options = STRING_LIST_INIT_DUP;
 
struct option options[] = {
OPT__VERBOSITY(),
@@ -166,15 +166,15 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
{ OPTION_CALLBACK,
  0, "signed", _cert, "yes|no|if-asked", N_("GPG sign the 
push"),
  PARSE_OPT_OPTARG, option_parse_push_signed },
+   OPT_STRING_LIST(0, "push-option", _options,
+   N_("server-specific"),
+   N_("option to transmit")),
OPT_BOOL(0, "progress", , N_("force progress 
reporting")),
 

Re: [PATCH v2 3/3] stash: keep untracked files intact in stash -k

[Thomas Gummerer]

On 03/21, Jeff King wrote:
> On Tue, Mar 21, 2017 at 10:12:19PM +, Thomas Gummerer wrote:
> 
> > Currently when there are untracked changes in a file "one" and in a file
> > "two" in the repository and the user uses:
> > 
> > git stash push -k one
> > 
> > all changes in "two" are wiped out completely.  That is clearly not the
> > intended result.  Make sure that only the files given in the pathspec
> > are changed when git stash push -k  is used.
> 
> Good description.

I basically just tweaked your report here :)  thanks for that!

> > diff --git a/git-stash.sh b/git-stash.sh
> > index 13711764a9..2fb651b2b8 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -314,7 +314,9 @@ push_stash () {
> >  
> > if test "$keep_index" = "t" && test -n "$i_tree"
> > then
> > -   git read-tree --reset -u $i_tree
> > +   git read-tree --reset $i_tree
> > +   git ls-files -z --modified -- "$@" |
> > +   git checkout-index -z --force --stdin
> > fi
> 
> I briefly wondered if this needed "-q" to match the earlier commit, but
> "checkout-index" isn't really chatty, so I don't think so (and the
> earlier checkout-index doesn't have it either).

Yep, I had the same thoughts here.

> I also wondered if this could be done in a single command as:
> 
>   git reset -q --hard $i_tree -- "$@"
> 
> But "git reset" can't handle pathspecs with "--hard" (which is why the
> similar case a few lines above uses the same commands).

Yeah unfortunately, that would have made the previous patch series
quite a bit easier :)  But here it wouldn't help anyway, as git reset
without pathspecs can't handle a tree-ish either, right?  And we also
want to handle the case where no pathspecs are given to git stash.

> 
> So this looks good to me.
> 
> > +test_expect_success 'stash -k --  leaves unstaged files intact' '
> > +   git reset &&
> > +   >foo &&
> > +   >bar &&
> > +   git add foo bar &&
> > +   git commit -m "test" &&
> > +   echo "foo" >foo &&
> > +   echo "bar" >bar &&
> > +   git stash -k -- foo &&
> > +   test "",bar = $(cat foo),$(cat bar) &&
> > +   git stash pop &&
> > +   test foo,bar = $(cat foo),$(cat bar)
> > +'
> 
> I always get nervous when I see test arguments without quotes, but I
> think this is fine (and I couldn't see a shorter way of doing it with
> test_cmp).

Hmm yeah I basically copied this from a different test and tweaked it
a bit to fit here.

> -Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Santiago Torres]

On Wed, Mar 22, 2017 at 05:10:03PM -0400, Jeff King wrote:
> On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:
> 
> > From: Jan Palus 
> > 
> > These all came as part of an earlier st/verify-tag topic that was
> > merged to 2.12.
> > 
> > Signed-off-by: Junio C Hamano 
> > ---
> > 
> >  * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
> >--format specifier tests", 2017-01-17)
> > 
> >  t/t7004-tag.sh| 8 
> >  t/t7030-verify-tag.sh | 8 
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> 

On my side, these patches make tests fail. I'm wondering if this is an
issue with the underlying shell (probably the version)? Let me try to
figure out what is exactly going on here.

> Like 2/3, this one also produces test failures for me. It looks like
> "verify-tag" does not show a tag which has been forged. I'm not sure if
> that's intentional (and the test is wrong) or a bug. 

I see that offending code would be [1]. Changing this behavior should be
trivial (dropping the continue), although I'm not sure if this is what
we want?

> 
> -Peff

Thanks,
-Santiago.

[1] 
https://github.com/git/git/blob/master/builtin/verify-tag.c#L6://github.com/git/git/blob/master/builtin/verify-tag.c#L67


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] stash: don't show internal implementation details

[Jeff King]

On Wed, Mar 22, 2017 at 09:33:47PM +, Thomas Gummerer wrote:

> > > - sed -e 1,2d >actual && # drop "Saved..." and "HEAD is now..."
> > > + sed -e 1,1d >actual && # drop "Saved..."
> > >   test_i18ncmp expect actual
> > >  '
> > 
> > This too, though I think "1d" would be the more usual way to say it.
> 
> Right thanks, I'll keep that in mind for another time. (I guess just
> changing this doesn't warrant a re-roll?)

Nah, I don't think it's important enough (and I give even odds that
Junio may just tweak it while applying).

-Peff

Re: [PATCH 2/3] t7406: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 02:32:22PM -0700, Stefan Beller wrote:

> > But the submodule path seems wrong, too. I'm not sure if
> > the expectation is wrong, or if there's a bug. +cc Stefan
> 
> Yes the actual output is wrong, too.
> But that is not because Git is wrong, but the test suite is.
> 
> test_must_fail git -C super submodule update >../actual &&
> 
> contains 2 errors:
> 
> * we are interested in stderr, so make it 2>
> * the -C switch doesn't apply to redirection. (obviously!)
>   So if you have run the test suite in a normal setup, you may have
>   a file "t/actual" which is empty. This is scary as it managed to break
>   out of the test suite and overwrote a potential file in your git.git.

Oh yeah, I didn't even look at the test itself, but I see I now have
t/actual in my repository. Yikes. That didn't happen before because it
simply sucked the entire command into the here document.

> I'll send a patch on top of the one under discussion momentarily.

I'd prefer if they come in a single patch so that we don't break bisect.

-Peff

Re: [PATCH 2/2] remote-curl: allow push options

[Brandon Williams]

On 03/22, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > Teach remote-curl to understand push options and to be able to convey
> > them across HTTP.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> 
> An earlier 438fc684 ("push options: pass push options to the
> transport helper", 2017-02-08) said:
> 
> ...
> Fix this by propagating the push options to the transport helper.
> 
> This is only addressing the first issue of
>(1) the helper protocol does not propagate push-option
>(2) the http helper is not prepared to handle push-option
> 
> Once we fix (2), the http transport helper can make use of push options
> as well, but that happens as a follow up. (1) is a bug fix, whereas (2)
> is a feature, which is why we only do (1) here.
> 
> and this is the step (2) to complete what it started?

Yep!  As far as I understand from looking at that commit, push options
were being passed to remote-curl and remote-curl was silently ignoring
them.  So that patch made it complain when a remote-helper didn't
support push options.

This patch teaches remote-curl to actually support push options, fixing
(2).

-- 
Brandon Williams

Re: [PATCH 2/3] t7406: fix here-doc syntax errors

[Junio C Hamano]

Jeff King  writes:

> After applying this, I get a failure:
>
>   --- expect  2017-03-22 21:01:53.350721155 +
>   +++ actual  2017-03-22 21:01:53.346721155 +
>   @@ -1 +1 @@
>   -Execution of 'false $submodulesha1' failed in submodule path 'submodule'
>   +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in 
> submodule path '../submodule'
>
> At the very least, we need to drop the "\" from EOF to expand
> $submodulesha1.

Right.

> But the submodule path seems wrong, too. I'm not sure if
> the expectation is wrong, or if there's a bug. +cc Stefan

Re: [PATCH v2 1/3] stash: don't show internal implementation details

[Thomas Gummerer]

On 03/21, Jeff King wrote:
> On Tue, Mar 21, 2017 at 10:12:17PM +, Thomas Gummerer wrote:
> 
> > git stash push uses other git commands internally.  Currently it only
> > passes the -q flag to those if the -q flag is passed to git stash.  when
> > using 'git stash push -p -q --no-keep-index', it doesn't even pass the
> > flag on to the internal reset at all.
> > 
> > It really is enough for the user to know that the stash is created,
> > without bothering them with the internal details of what's happening.
> > Always pass the -q flag to the internal git clean and git reset
> > commands, to avoid unnecessary and potentially confusing output.
> > 
> > Reported-by: Jeff King 
> > Signed-off-by: Thomas Gummerer 
> 
> I think combining these is fine. The incorrect output with pathspecs
> isn't mentioned anymore, but I think that's OK.

Yeah, they felt so similar now that splitting this up in different
patches would be a bit too noisy.

> [...]
> 
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 89877e4b52..ea8e5c7818 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -663,7 +663,7 @@ test_expect_success 'stash apply shows status same as 
> > git status (relative to cu
> > sane_unset GIT_MERGE_VERBOSITY &&
> > git stash apply
> > ) |
> > -   sed -e 1,2d >actual && # drop "Saved..." and "HEAD is now..."
> > +   sed -e 1,1d >actual && # drop "Saved..."
> > test_i18ncmp expect actual
> >  '
> 
> This too, though I think "1d" would be the more usual way to say it.

Right thanks, I'll keep that in mind for another time. (I guess just
changing this doesn't warrant a re-roll?)

> -peff

Re: [PATCH 2/3] t7406: fix here-doc syntax errors

[Stefan Beller]

On Wed, Mar 22, 2017 at 2:07 PM, Jeff King  wrote:
> On Wed, Mar 22, 2017 at 01:08:04PM -0700, Junio C Hamano wrote:
>
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index 347857fa7c..a20df9420a 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in 
>> .git/config catches failure -
>>  '
>>
>>  test_expect_success 'submodule update - command run for initial population 
>> of submodule' '
>> - cat <<-\ EOF >expect
>> + cat >expect <<-\EOF &&
>>   Execution of '\''false $submodulesha1'\'' failed in submodule path 
>> '\''submodule'\''
>> - EOF &&
>> + EOF
>
> After applying this, I get a failure:
>
>   --- expect2017-03-22 21:01:53.350721155 +
>   +++ actual2017-03-22 21:01:53.346721155 +
>   @@ -1 +1 @@
>   -Execution of 'false $submodulesha1' failed in submodule path 'submodule'
>   +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in 
> submodule path '../submodule'
>
> At the very least, we need to drop the "\" from EOF to expand
> $submodulesha1.

yes.

> But the submodule path seems wrong, too. I'm not sure if
> the expectation is wrong, or if there's a bug. +cc Stefan

Yes the actual output is wrong, too.
But that is not because Git is wrong, but the test suite is.

test_must_fail git -C super submodule update >../actual &&

contains 2 errors:

* we are interested in stderr, so make it 2>
* the -C switch doesn't apply to redirection. (obviously!)
  So if you have run the test suite in a normal setup, you may have
  a file "t/actual" which is empty. This is scary as it managed to break
  out of the test suite and overwrote a potential file in your git.git.

I'll send a patch on top of the one under discussion momentarily.

Thanks,
Stefan

Re: [PATCH 0/6] thread lazy_init_name_hash

[Junio C Hamano]

Jeff Hostetler  writes:

> On 3/22/2017 4:54 PM, Junio C Hamano wrote:
>>>
>>> Thank you for an update.
>>
>> One notable difference I noticed since the previous round is that
>> this no longer adds precomputed hash to "struct cache_entry".  As
>> you are aiming to manage an index with a large number of entries,
>> this is a welcome change that makes sense.
>
> Yes, this completely isolates the changes inside the name-hash.c
> code.  And it eliminates the need to update/invalidate the precomputed
> hash values as entries are changed.

Thanks.  Having a summary like that for differences since the last
round in the cover letter would help during the review, but this is
an example that lack of such a summary would be one way to make sure
that reviewer(s) are paying attention ;-)

Re: [PATCH 2/2] remote-curl: allow push options

[Junio C Hamano]

Brandon Williams  writes:

> Teach remote-curl to understand push options and to be able to convey
> them across HTTP.
>
> Signed-off-by: Brandon Williams 
> ---

An earlier 438fc684 ("push options: pass push options to the
transport helper", 2017-02-08) said:

...
Fix this by propagating the push options to the transport helper.

This is only addressing the first issue of
   (1) the helper protocol does not propagate push-option
   (2) the http helper is not prepared to handle push-option

Once we fix (2), the http transport helper can make use of push options
as well, but that happens as a follow up. (1) is a bug fix, whereas (2)
is a feature, which is why we only do (1) here.

and this is the step (2) to complete what it started?

>  builtin/send-pack.c |  5 +
>  remote-curl.c   |  8 
>  t/t5545-push-options.sh | 30 +-
>  3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 1ff5a6753..6796f3368 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>   int progress = -1;
>   int from_stdin = 0;
>   struct push_cas_option cas = {0};
> + struct string_list push_options = STRING_LIST_INIT_DUP;
>  
>   struct option options[] = {
>   OPT__VERBOSITY(),
> @@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>   OPT_BOOL(0, "stateless-rpc", _rpc, N_("use stateless 
> RPC protocol")),
>   OPT_BOOL(0, "stdin", _stdin, N_("read refs from stdin")),
>   OPT_BOOL(0, "helper-status", _status, N_("print status 
> from remote helper")),
> + OPT_STRING_LIST('o', "push-option", _options,
> + N_("server-specific"),
> + N_("option to transmit")),
>   { OPTION_CALLBACK,
> 0, CAS_OPT_NAME, , N_("refname>: N_("require old value of ref to be at this value"),
> @@ -199,6 +203,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>   args.use_thin_pack = use_thin_pack;
>   args.atomic = atomic;
>   args.stateless_rpc = stateless_rpc;
> + args.push_options = push_options.nr ? _options : NULL;
>  
>   if (from_stdin) {
>   struct argv_array all_refspecs = ARGV_ARRAY_INIT;
> diff --git a/remote-curl.c b/remote-curl.c
> index 34a97e732..e953d06f6 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -22,6 +22,7 @@ struct options {
>   unsigned long depth;
>   char *deepen_since;
>   struct string_list deepen_not;
> + struct string_list push_options;
>   unsigned progress : 1,
>   check_self_contained_and_connected : 1,
>   cloning : 1,
> @@ -139,6 +140,9 @@ static int set_option(const char *name, const char *value)
>   else
>   return -1;
>   return 0;
> + } else if (!strcmp(name, "push-option")) {
> + string_list_append(_options, value);
> + return 0;
>  
>  #if LIBCURL_VERSION_NUM >= 0x070a08
>   } else if (!strcmp(name, "family")) {
> @@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, 
> char **specs)
>   argv_array_push(, "--quiet");
>   else if (options.verbosity > 1)
>   argv_array_push(, "--verbose");
> + for (i = 0; i < options.push_options.nr; i++)
> + argv_array_pushf(, "--push-option=%s",
> +  options.push_options.items[i].string);
>   argv_array_push(, options.progress ? "--progress" : 
> "--no-progress");
>   for_each_string_list_item(cas_option, _options)
>   argv_array_push(, cas_option->string);
> @@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)
>   options.progress = !!isatty(2);
>   options.thin = 1;
>   string_list_init(_not, 1);
> + string_list_init(_options, 1);
>  
>   remote = remote_get(argv[1]);
>  
> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
> index 9a57a7d8f..ac62083e9 100755
> --- a/t/t5545-push-options.sh
> +++ b/t/t5545-push-options.sh
> @@ -102,7 +102,9 @@ test_expect_success 'two push options work' '
>   test_cmp expect upstream/.git/hooks/post-receive.push_options
>  '
>  
> -test_expect_success 'push option denied properly by http remote helper' '\
> +test_expect_success 'push option denied properly by http server' '
> + test_when_finished "rm -rf test_http_clone" &&
> + test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" 
> &&
>   mk_repo_pair &&
>   git -C upstream config receive.advertisePushOptions false &&
>   git -C upstream config http.receivepack true &&
> @@ -113,6 +115,32 @@ test_expect_success 

Re: [PATCH 0/2] push options across http

[Stefan Beller]

On Wed, Mar 22, 2017 at 2:17 PM, Junio C Hamano  wrote:
> Thanks.  Why does the topic name sound familiar to me?  Did we have
> a recent attempt to do the same that didn't work well or something?

'sb/push-options-via-transport' sounds similar. Before that we silently
ignored push options in http, with that series we got an error indicating
http doesn't support push options. this series actually adds support for it.

Thanks,
Stefan

Re: [PATCH 0/2] push options across http

[Junio C Hamano]

Brandon Williams  writes:

> This series enables push options to be sent across http using remote-curl
>
> Thanks to Jonathan Nieder for helping troubleshoot.
>
> Brandon Williams (2):
>   send-pack: send push options correctly in stateless-rpc case
>   remote-curl: allow push options

Thanks.  Why does the topic name sound familiar to me?  Did we have
a recent attempt to do the same that didn't work well or something?

>
>  builtin/send-pack.c |  5 +
>  remote-curl.c   |  8 
>  send-pack.c | 20 
>  t/t5545-push-options.sh | 30 +-
>  4 files changed, 50 insertions(+), 13 deletions(-)

Re: [PATCH v2 10/16] tag: change misleading --list documentation

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Yes, all of this is correct, but not relevant to what I'm describing
> in the commit message, because I'm making a documentation change and
> describing how you *would* expect git to work if you read the
> *documentation*, not if you read the code.

OK.

>>> +-l::
>>> +--list::
>>> + Activate the list mode. `git tag ` would try to create a
>>
>> Dont say  on this line.  It is `git tag `.
>
> Makes sense, but this is something I copied as-is from git-branch.txt,
> which then has the same issue, so v3 will have yet another related
> patch...

I think you'd rather want to make it a single patch to be applied
and merged independently, as a fix to a documentation bug we somehow
noticed that is unrelated to the main theme of what we were working
to perfect ;-)

>> The "-l/-d/-v" options follow the last-one-wins rule, no?  Perhaps
>> also show how this one works in this test (while retitling it)?
>>
>> git tag -d -v -l
>
> This will fail as tested for in "tag: add more incompatibles mode
> tests". We weren't testing "-d" with "-l", or this combination, I'll
> add both to the tests.

Thanks.

Re: [PATCH 1/2] send-pack: send push options correctly in stateless-rpc case

[Jonathan Nieder]

Brandon Williams wrote:

> "git send-pack --stateless-rpc" puts each request in a sequence of pkt-lines
> followed by a flush-pkt. The push option code forgot about this and sends push
> options and their terminating delimiter as ordinary pkt-lines that get their
> length header stripped off by remote-curl before being sent to the server.
>
> The result is multiple malformed requests, which the server rejects.
>
> Fortunately send-pack --stateless-rpc already is aware of this "pkt-line 
> within
> pkt-line" framing for the update commands that precede push options. Handle
> push options the same way.
>
> Signed-off-by: Brandon Williams 
> ---
>  send-pack.c | 20 
>  1 file changed, 8 insertions(+), 12 deletions(-)

This is only a hypothetical issue until the next patch though, right?

For what it's worth,

Tested-by: Jonathan Nieder 
Reviewed-by: Jonathan Nieder 

Thanks.

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 01:08:05PM -0700, Junio C Hamano wrote:

> From: Jan Palus 
> 
> These all came as part of an earlier st/verify-tag topic that was
> merged to 2.12.
> 
> Signed-off-by: Junio C Hamano 
> ---
> 
>  * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
>--format specifier tests", 2017-01-17)
> 
>  t/t7004-tag.sh| 8 
>  t/t7030-verify-tag.sh | 8 
>  2 files changed, 8 insertions(+), 8 deletions(-)

Like 2/3, this one also produces test failures for me. It looks like
"verify-tag" does not show a tag which has been forged. I'm not sure if
that's intentional (and the test is wrong) or a bug.  +cc Santiago

-Peff

Re: [PATCH v2 10/16] tag: change misleading --list documentation

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> Please drop "interleaved", as we are not encouraging GNUism.  We
>> just tolerate it but do not guarantee to keep them working.
>
> This brings up the same point you made in
> , I replied to in
> 

Re: [PATCH 2/3] t7406: fix here-doc syntax errors

[Jeff King]

On Wed, Mar 22, 2017 at 01:08:04PM -0700, Junio C Hamano wrote:

> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 347857fa7c..a20df9420a 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in 
> .git/config catches failure -
>  '
>  
>  test_expect_success 'submodule update - command run for initial population 
> of submodule' '
> - cat <<-\ EOF >expect
> + cat >expect <<-\EOF &&
>   Execution of '\''false $submodulesha1'\'' failed in submodule path 
> '\''submodule'\''
> - EOF &&
> + EOF

After applying this, I get a failure:

  --- expect2017-03-22 21:01:53.350721155 +
  +++ actual2017-03-22 21:01:53.346721155 +
  @@ -1 +1 @@
  -Execution of 'false $submodulesha1' failed in submodule path 'submodule'
  +Execution of 'false 4301fd3e4110d3b6212c19aed3094150392545b9' failed in 
submodule path '../submodule'

At the very least, we need to drop the "\" from EOF to expand
$submodulesha1. But the submodule path seems wrong, too. I'm not sure if
the expectation is wrong, or if there's a bug. +cc Stefan

-Peff

Re: [PATCH 0/6] thread lazy_init_name_hash

[Jeff Hostetler]

On 3/22/2017 4:54 PM, Junio C Hamano wrote:

Junio C Hamano  writes:


This patch series replaces my earlier
 * jh/memihash-opt (2017-02-17) 5 commits
patch series.


Ahh.  I was scratching my head trying to remember why some of these
look so familiar.  [PATCH v2 ...] would have helped.

Thank you for an update.


One notable difference I noticed since the previous round is that
this no longer adds precomputed hash to "struct cache_entry".  As
you are aiming to manage an index with a large number of entries,
this is a welcome change that makes sense.


Yes, this completely isolates the changes inside the name-hash.c
code.  And it eliminates the need to update/invalidate the precomputed
hash values as entries are changed.



$ make NO_PTHREADS=NoThanks name-hash.o
CC name-hash.o
name-hash.c: In function 'lazy_init_name_hash':
...
still has to be addressed.  Perhaps squash pieces of these into
appropriate patches in the series?
...


Will do. I wasn't sure how you specified non-threaded builds.

Thanks,
Jeff

Re: [PATCH 1/3] t5615: fix a here-doc syntax error

[Jeff King]

On Wed, Mar 22, 2017 at 01:08:03PM -0700, Junio C Hamano wrote:

> From: Jan Palus 
> 
> This came as part of jk/quote-env-path-list-component and was merged
> to 2.11.1 and later.
> 
> Signed-off-by: Junio C Hamano 
> ---
> 
>  * This should be applied on top of 5e74824f ("t5615-alternate-env:
>  double-quotes in file names do not work on Windows", 2016-12-21)
> 
>  t/t5615-alternate-env.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
> index 26ebb0375d..d2d883f3a1 100755
> --- a/t/t5615-alternate-env.sh
> +++ b/t/t5615-alternate-env.sh
> @@ -77,6 +77,7 @@ test_expect_success 'mix of quoted and unquoted alternates' 
> '
>   check_obj "$quoted:$unquoted" <<-EOF
>   $one blob
>   $two blob
> + EOF
>  '

Thanks, this one is my fault, and the solution is obviously correct.

-Peff

Re: [PATCH 0/6] thread lazy_init_name_hash

[Junio C Hamano]

Junio C Hamano  writes:

>> This patch series replaces my earlier
>>  * jh/memihash-opt (2017-02-17) 5 commits
>> patch series.
>
> Ahh.  I was scratching my head trying to remember why some of these
> look so familiar.  [PATCH v2 ...] would have helped.
>
> Thank you for an update.

One notable difference I noticed since the previous round is that
this no longer adds precomputed hash to "struct cache_entry".  As
you are aiming to manage an index with a large number of entries,
this is a welcome change that makes sense.

$ make NO_PTHREADS=NoThanks name-hash.o
CC name-hash.o
name-hash.c: In function 'lazy_init_name_hash':
name-hash.c:573:3: error: implicit declaration of function 
'threaded_lazy_init_name_hash' [-Werror=implicit-function-declaration]
   threaded_lazy_init_name_hash(istate);
   ^
name-hash.c: In function 'test_lazy_init_name_hash':
name-hash.c:595:2: error: 'lazy_nr_dir_threads' undeclared (first use in this 
function)
  lazy_nr_dir_threads = 0;
  ^
name-hash.c:595:2: note: each undeclared identifier is reported only once for 
each function it appears in
name-hash.c:596:2: error: 'lazy_try_threaded' undeclared (first use in this 
function)
  lazy_try_threaded = try_threaded;
  ^
name-hash.c:601:1: error: control reaches end of non-void function 
[-Werror=return-type]
 }
 ^
cc1: all warnings being treated as errors
make: *** [name-hash.o] Error 1

still has to be addressed.  Perhaps squash pieces of these into
appropriate patches in the series?

 name-hash.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/name-hash.c b/name-hash.c
index a5826505e0..725ac22886 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -118,9 +118,16 @@ static int cache_entry_cmp(const struct cache_entry *ce1,
return remove ? !(ce1 == ce2) : 0;
 }
 
+static int lazy_try_threaded = 1;
+static int lazy_nr_dir_threads = 0;
 
 #ifdef NO_PTHREADS
 
+static void threaded_lazy_init_name_hash(struct index_state *istate)
+{
+   ; /* nothing */
+}
+
 static int lookup_lazy_params(struct index_state *istate)
 {
return 0;
@@ -153,8 +160,6 @@ static int lookup_lazy_params(struct index_state *istate)
  */
 #define LAZY_MAX_MUTEX   (32)
 
-static int lazy_try_threaded = 1;
-static int lazy_nr_dir_threads = 0;
 static pthread_mutex_t *lazy_dir_mutex_array;
 
 /*

Re: [PATCH 0/6] thread lazy_init_name_hash

[Junio C Hamano]

Just FYI before I start reading each patch carefully...

Subject: [PATCH 2/6] hashmap: allow memihash computation to be continued
ERROR: trailing whitespace
#28: FILE: hashmap.c:56:
+ */ $

total: 1 errors, 0 warnings, 30 lines checked

Subject: [PATCH 4/6] name-hash: perf improvement for lazy_init_name_hash
ERROR: space required after that ',' (ctx:VxV)
#57: FILE: name-hash.c:38:
+   return find_dir_entry__hash(istate, name, namelen, 
memihash(name,namelen));
^

ERROR: do not initialise statics to 0 or NULL
#105: FILE: name-hash.c:157:
+static int lazy_nr_dir_threads = 0;

total: 2 errors, 0 warnings, 516 lines checked

Subject: [PATCH 5/6] name-hash: add test-lazy-init-name-hash
ERROR: do not initialise statics to 0 or NULL
#64: FILE: t/helper/test-lazy-init-name-hash.c:4:
+static int single = 0;

ERROR: do not initialise statics to 0 or NULL
#65: FILE: t/helper/test-lazy-init-name-hash.c:5:
+static int multi = 0;

ERROR: do not initialise statics to 0 or NULL
#67: FILE: t/helper/test-lazy-init-name-hash.c:7:
+static int dump = 0;

ERROR: do not initialise statics to 0 or NULL
#68: FILE: t/helper/test-lazy-init-name-hash.c:8:
+static int perf = 0;

ERROR: do not initialise statics to 0 or NULL
#69: FILE: t/helper/test-lazy-init-name-hash.c:9:
+static int analyze = 0;

ERROR: do not initialise statics to 0 or NULL
#70: FILE: t/helper/test-lazy-init-name-hash.c:10:
+static int analyze_step = 0;

ERROR: trailing whitespace
#215: FILE: t/helper/test-lazy-init-name-hash.c:155:
+^I^I^Ielse $

total: 7 errors, 0 warnings, 278 lines checked

Re: [PATCH v1 1/3] pkt-line: add packet_write_list_gently()

[Junio C Hamano]

Ben Peart  writes:

> Add packet_write_list_gently() which writes multiple lines in a single
> call and then calls packet_flush_gently(). This is used later in this
> patch series.

I can see how it would be convenient to have a function like this.
I'd name it without _gently(), though.  We call something _gently()
when we initially only have a function that dies hard on error and
later want to have a variant that returns an error for the caller to
handle.  You are starting without a dying variant (which is probably
a preferable way to structure the API).

Also I am hesitant to take a function that does not take any "list"
type argument and yet calls itself "write_list".  IOW, I'd expect
something like these

write_list(int fd, const char **lines);
write_list(int fd, struct string_list *lines);

given that name, and not "varargs, each of which is a line".  I am
tempted to suggest

packet_writel(int fd, const char *line, ...);

noticing similarity with execl(), but perhaps others may be able to
come up with better names.

> Signed-off-by: Ben Peart 
> ---
>  pkt-line.c | 19 +++
>  pkt-line.h |  1 +
>  2 files changed, 20 insertions(+)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index d4b6bfe076..fccdac1352 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
>   return status;
>  }
>  
> +int packet_write_list_gently(int fd, const char *line, ...)
> +{
> + va_list args;
> + int err;
> + va_start(args, line);
> + for (;;) {
> + if (!line)
> + break;
> + if (strlen(line) > LARGE_PACKET_DATA_MAX)
> + return -1;
> + err = packet_write_fmt_gently(fd, "%s\n", line);
> + if (err)
> + return err;
> + line = va_arg(args, const char*);
> + }
> + va_end(args);
> + return packet_flush_gently(fd);
> +}
> +
>  static int packet_write_gently(const int fd_out, const char *buf, size_t 
> size)
>  {
>   static char packet_write_buffer[LARGE_PACKET_MAX];
> diff --git a/pkt-line.h b/pkt-line.h
> index 18eac64830..3674d04509 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf);
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
> __attribute__((format (printf, 2, 3)));
>  int packet_flush_gently(int fd);
>  int packet_write_fmt_gently(int fd, const char *fmt, ...) 
> __attribute__((format (printf, 2, 3)));
> +int packet_write_list_gently(int fd, const char *line, ...);
>  int write_packetized_from_fd(int fd_in, int fd_out);
>  int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);

[PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

From: Jan Palus 

These all came as part of an earlier st/verify-tag topic that was
merged to 2.12.

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

 * This should be applied on top of 4fea72f4 ("t/t7004-tag: Add
   --format specifier tests", 2017-01-17)

 t/t7004-tag.sh| 8 
 t/t7030-verify-tag.sh | 8 
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b4698ab5f5..efc6dde7b8 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -896,17 +896,17 @@ test_expect_success GPG 'verifying a forged tag should 
fail' '
 '
 
 test_expect_success 'verifying a proper tag with --format pass and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : signed-tag
-   EOF &&
+   EOF
git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : forged-tag
-   EOF &&
+   EOF
test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" 
>actual &&
test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98e..ce37fd9864 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -126,17 +126,17 @@ test_expect_success GPG 'verify multiple tags' '
 '
 
 test_expect_success 'verifying tag with --format' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : fourth-signed
-   EOF &&
+   EOF
git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : 7th forged-signed
-   EOF &&
+   EOF
test_must_fail git verify-tag --format="tagname : %(tag)" $(cat 
forged1.tag) >actual-forged &&
test_cmp expect actual-forged
 '
-- 
2.12.1-430-gafd6726309

[PATCH 1/3] t5615: fix a here-doc syntax error

[Junio C Hamano]

From: Jan Palus 

This came as part of jk/quote-env-path-list-component and was merged
to 2.11.1 and later.

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

 * This should be applied on top of 5e74824f ("t5615-alternate-env:
 double-quotes in file names do not work on Windows", 2016-12-21)

 t/t5615-alternate-env.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 26ebb0375d..d2d883f3a1 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -77,6 +77,7 @@ test_expect_success 'mix of quoted and unquoted alternates' '
check_obj "$quoted:$unquoted" <<-EOF
$one blob
$two blob
+   EOF
 '
 
 test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
-- 
2.12.1-430-gafd6726309

[PATCH 0/3] fix "here-doc" syntax errors

[Junio C Hamano]

Because I'd prefer to be able to queue fixes on individual topics
that introduced the breakages, I splitted the fixes in your two
messages into three patches.

Please respond to the list, saying it is OK to add your "sign-off"
(see Documentation/SubmittingPatches) to these patches.

Thanks.

Jan Palus (3):
  t5615: fix a here-doc syntax error
  t7406: fix here-doc syntax errors
  t7004, t7030: fix here-doc syntax errors

 t/t5615-alternate-env.sh| 1 +
 t/t7004-tag.sh  | 8 
 t/t7030-verify-tag.sh   | 8 
 t/t7406-submodule-update.sh | 4 ++--
 4 files changed, 11 insertions(+), 10 deletions(-)

-- 
2.12.1-430-gafd6726309

[PATCH 2/3] t7406: fix here-doc syntax errors

[Junio C Hamano]

From: Jan Palus 

The came in an earlier sb/submodule-update-initial-runs-custom-script
topic that was merged to 2.12.

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

 * This should be applied on top of e7b37caf ("submodule update: run
   custom update script for initial populating as well", 2017-01-25)

 t/t7406-submodule-update.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 347857fa7c..a20df9420a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -442,9 +442,9 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
 '
 
 test_expect_success 'submodule update - command run for initial population of 
submodule' '
-   cat <<-\ EOF >expect
+   cat >expect <<-\EOF &&
Execution of '\''false $submodulesha1'\'' failed in submodule path 
'\''submodule'\''
-   EOF &&
+   EOF
rm -rf super/submodule &&
test_must_fail git -C super submodule update >../actual &&
test_cmp expect actual &&
-- 
2.12.1-430-gafd6726309

[PATCH 0/2] push options across http

[Brandon Williams]

This series enables push options to be sent across http using remote-curl

Thanks to Jonathan Nieder for helping troubleshoot.

Brandon Williams (2):
  send-pack: send push options correctly in stateless-rpc case
  remote-curl: allow push options

 builtin/send-pack.c |  5 +
 remote-curl.c   |  8 
 send-pack.c | 20 
 t/t5545-push-options.sh | 30 +-
 4 files changed, 50 insertions(+), 13 deletions(-)

-- 
2.12.1.500.gab5fba24ee-goog

[PATCH 1/2] send-pack: send push options correctly in stateless-rpc case

[Brandon Williams]

"git send-pack --stateless-rpc" puts each request in a sequence of pkt-lines
followed by a flush-pkt. The push option code forgot about this and sends push
options and their terminating delimiter as ordinary pkt-lines that get their
length header stripped off by remote-curl before being sent to the server.

The result is multiple malformed requests, which the server rejects.

Fortunately send-pack --stateless-rpc already is aware of this "pkt-line within
pkt-line" framing for the update commands that precede push options. Handle
push options the same way.

Helped-by: Jonathan Nieder 
Signed-off-by: Brandon Williams 
---
 send-pack.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index d2d2a49a0..66e652f7e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -532,6 +532,14 @@ int send_pack(struct send_pack_args *args,
}
}
 
+   if (use_push_options) {
+   struct string_list_item *item;
+
+   packet_buf_flush(_buf);
+   for_each_string_list_item(item, args->push_options)
+   packet_buf_write(_buf, "%s", item->string);
+   }
+
if (args->stateless_rpc) {
if (!args->dry_run && (cmds_sent || is_repository_shallow())) {
packet_buf_flush(_buf);
@@ -544,18 +552,6 @@ int send_pack(struct send_pack_args *args,
strbuf_release(_buf);
strbuf_release(_buf);
 
-   if (use_push_options) {
-   struct string_list_item *item;
-   struct strbuf sb = STRBUF_INIT;
-
-   for_each_string_list_item(item, args->push_options)
-   packet_buf_write(, "%s", item->string);
-
-   write_or_die(out, sb.buf, sb.len);
-   packet_flush(out);
-   strbuf_release();
-   }
-
if (use_sideband && cmds_sent) {
memset(, 0, sizeof(demux));
demux.proc = sideband_demux;
-- 
2.12.1.500.gab5fba24ee-goog

[PATCH 2/2] remote-curl: allow push options

[Brandon Williams]

Teach remote-curl to understand push options and to be able to convey
them across HTTP.

Signed-off-by: Brandon Williams 
---
 builtin/send-pack.c |  5 +
 remote-curl.c   |  8 
 t/t5545-push-options.sh | 30 +-
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 1ff5a6753..6796f3368 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -152,6 +152,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
int progress = -1;
int from_stdin = 0;
struct push_cas_option cas = {0};
+   struct string_list push_options = STRING_LIST_INIT_DUP;
 
struct option options[] = {
OPT__VERBOSITY(),
@@ -171,6 +172,9 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
OPT_BOOL(0, "stateless-rpc", _rpc, N_("use stateless 
RPC protocol")),
OPT_BOOL(0, "stdin", _stdin, N_("read refs from stdin")),
OPT_BOOL(0, "helper-status", _status, N_("print status 
from remote helper")),
+   OPT_STRING_LIST('o', "push-option", _options,
+   N_("server-specific"),
+   N_("option to transmit")),
{ OPTION_CALLBACK,
  0, CAS_OPT_NAME, , N_("refname>:= 0x070a08
} else if (!strcmp(name, "family")) {
@@ -943,6 +947,9 @@ static int push_git(struct discovery *heads, int nr_spec, 
char **specs)
argv_array_push(, "--quiet");
else if (options.verbosity > 1)
argv_array_push(, "--verbose");
+   for (i = 0; i < options.push_options.nr; i++)
+   argv_array_pushf(, "--push-option=%s",
+options.push_options.items[i].string);
argv_array_push(, options.progress ? "--progress" : 
"--no-progress");
for_each_string_list_item(cas_option, _options)
argv_array_push(, cas_option->string);
@@ -1028,6 +1035,7 @@ int cmd_main(int argc, const char **argv)
options.progress = !!isatty(2);
options.thin = 1;
string_list_init(_not, 1);
+   string_list_init(_options, 1);
 
remote = remote_get(argv[1]);
 
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 9a57a7d8f..ac62083e9 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -102,7 +102,9 @@ test_expect_success 'two push options work' '
test_cmp expect upstream/.git/hooks/post-receive.push_options
 '
 
-test_expect_success 'push option denied properly by http remote helper' '\
+test_expect_success 'push option denied properly by http server' '
+   test_when_finished "rm -rf test_http_clone" &&
+   test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" 
&&
mk_repo_pair &&
git -C upstream config receive.advertisePushOptions false &&
git -C upstream config http.receivepack true &&
@@ -113,6 +115,32 @@ test_expect_success 'push option denied properly by http 
remote helper' '\
git -C test_http_clone push origin master
 '
 
+test_expect_success 'push options work properly across http' '
+   test_when_finished "rm -rf test_http_clone" &&
+   test_when_finished "rm -rf \"$HTTPD_DOCUMENT_ROOT_PATH\"/upstream.git" 
&&
+   mk_repo_pair &&
+   git -C upstream config receive.advertisePushOptions true &&
+   git -C upstream config http.receivepack true &&
+   cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
+   git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
+
+   test_commit -C test_http_clone one &&
+   git -C test_http_clone push origin master &&
+   git -C "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git 

Re: Question: xdiff and "pretty" (human readable) diff output

[matthew]

* Stefan Beller  [2017-03-22 12:43:41 -0700]:


On Wed, Mar 22, 2017 at 12:23 PM,   wrote:

Good day,

I have a question with respect to how git generates "pretty" (ie: human
readable) unified diffs. It's to my understanding that git uses its own
(simplified/minimized) fork of libxdiff, simply referred to as "xdiff"
[1].


correct.


Which tool/library is used to take the xdiff output and generate
the human-readable equivalent that is rendered to the console?


Git itself. Have a look at diff.c (it's only 5k lines of code ;)

Maybe start with fn_out_consume which is a callback (for xdiff)
that is responsible for pretty printing the output, i.e. transforming the
output of xdiff according to Gits configuration.


I have a
program that I'm maintaining that currently tracks changes to a couple
of "sandboxed" files, and I wanted to add a simple console UI that
periodically shows the changes to the files over time and/or dumps the
"pretty diff" to syslog.



over time


So you could use cron for that?


 "pretty diff" to syslog.


So you want to modify the "pretty diff"?
Maybe that can be archived by a custom format
(see the git *log* man page) and doesn't require hacking.

Thanks,
Stefan


Thank you Stefan. This provides me with everything I need.

Cheers!

--
Matthew Giassa
e: matt...@giassa.net

Re: Question: xdiff and "pretty" (human readable) diff output

[Stefan Beller]

On Wed, Mar 22, 2017 at 12:23 PM,   wrote:
> Good day,
>
> I have a question with respect to how git generates "pretty" (ie: human
> readable) unified diffs. It's to my understanding that git uses its own
> (simplified/minimized) fork of libxdiff, simply referred to as "xdiff"
> [1].

correct.

> Which tool/library is used to take the xdiff output and generate
> the human-readable equivalent that is rendered to the console?

Git itself. Have a look at diff.c (it's only 5k lines of code ;)

Maybe start with fn_out_consume which is a callback (for xdiff)
that is responsible for pretty printing the output, i.e. transforming the
output of xdiff according to Gits configuration.

> I have a
> program that I'm maintaining that currently tracks changes to a couple
> of "sandboxed" files, and I wanted to add a simple console UI that
> periodically shows the changes to the files over time and/or dumps the
> "pretty diff" to syslog.

> over time

So you could use cron for that?

>  "pretty diff" to syslog.

So you want to modify the "pretty diff"?
Maybe that can be archived by a custom format
(see the git *log* man page) and doesn't require hacking.

Thanks,
Stefan

Re: EOF test fixes (t5615/t7004)

[Junio C Hamano]

Jan Palus  writes:

> diff -ur git-2.12.0.orig/t/t7004-tag.sh git-2.12.0/t/t7004-tag.sh
> --- git-2.12.0.orig/t/t7004-tag.sh2017-02-25 14:10:53.059367179 +0100
> +++ git-2.12.0/t/t7004-tag.sh 2017-02-25 14:11:45.105827700 +0100
> @@ -880,17 +880,17 @@
>  '
>  
>  test_expect_success 'verifying a proper tag with --format pass and format 
> accordingly' '
> - cat >expect <<-\EOF
> + cat >expect <<-\EOF &&
>   tagname : signed-tag
> - EOF &&
> + EOF
>   git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
>   test_cmp expect actual
>  '
>  
>  test_expect_success 'verifying a forged tag with --format fail and format 
> accordingly' '
> - cat >expect <<-\EOF
> + cat >expect <<-\EOF &&
>   tagname : forged-tag
> - EOF &&
> + EOF
>   test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" 
> >actual &&
>   test_cmp expect actual
>  '

Ouch.  These shouldn't even have passed our review.

Thanks for spotting.

Re: [PATCH 0/6] thread lazy_init_name_hash

[Junio C Hamano]

g...@jeffhostetler.com writes:

> From: Jeff Hostetler 
>
> This patch series is a performance optimization for
> lazy_init_name_hash() in name-hash.c on very large
> repositories.
>
> This change allows lazy_init_name_hash() to optionally
> use multiple threads when building the the_index.dir_hash
> and the_index.name_hash hashmaps.  The original code path
> has been preserved and is used when the repo is small or
> the system does not have sufficient CPUs.
>
> A helper command (t/helper/test-lazy-init-name-hash) was
> created to demonstrate performance differences and validate
> output.  For example, use the '-p' option to compare both
> code paths on a large repo.
>
> During our testing on the Windows source tree (3.1M
> files, 500K folders, 450MB index), this change reduced
> the runtime of lazy_init_name_hash() from 1.4 to 0.27
> seconds.
>
> This patch series replaces my earlier
>  * jh/memihash-opt (2017-02-17) 5 commits
> patch series.

Ahh.  I was scratching my head trying to remember why some of these
look so familiar.  [PATCH v2 ...] would have helped.

Thank you for an update.

>
> Jeff Hostetler (6):
>   name-hash: specify initial size for istate.dir_hash table
>   hashmap: allow memihash computation to be continued
>   hashmap: Add disallow_rehash setting
>   name-hash: perf improvement for lazy_init_name_hash
>   name-hash: add test-lazy-init-name-hash
>   name-hash: add perf test for lazy_init_name_hash
>
>  Makefile|   1 +
>  cache.h |   1 +
>  hashmap.c   |  29 ++-
>  hashmap.h   |  25 ++
>  name-hash.c | 490 
> +++-
>  t/helper/test-lazy-init-name-hash.c | 264 +++
>  t/perf/p0004-lazy-init-name-hash.sh |  19 ++
>  7 files changed, 820 insertions(+), 9 deletions(-)
>  create mode 100644 t/helper/test-lazy-init-name-hash.c
>  create mode 100644 t/perf/p0004-lazy-init-name-hash.sh

Re: [PATCH v2 10/16] tag: change misleading --list documentation

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 21, 2017 at 7:45 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason   writes:
>
>> And after Jeff's change, one that took multiple patterns:
>>
>> git tag --list 'v*rc*' --list '*2.8*'
>
> I do not think this is a correct depiction of the evolution, and is
> a confusing description.  It should say
>
> git tag --list 'v*rc*' '*2.8*'
>
> instead.

Changing this from "--list  --list " to "--list
 " wouldn't make any sense, because it's in the
middle of a sentence explaining not how the tooling worked, but before
this change, how it was documented to work. I.e. read up a few lines
to  "One would expect an option that was documented like that..." for
the context.

> When the logic was still in scripted Porcelain,  _was_ an
> optional argument to the "-l" option (see b867c7c2 ("git-tag: -l to
> list tags (usability).", 2006-02-17) you quoted earlier).  But way
> before Jeff's change, when the command was reimplemented in C and
> started using parse_options(),  stopped being an argument
> to the "-l" option.  What Jeff's change did was that the original
> code that only took
>
> git tag -l 'v*rc*'
>
> to also take
>
> git tag -l 'v*rc*' '*2.8*'
>
> i.e. multiple patterns.  Yes, thanks to parse_options, you could
> have -l twice on the command line, but "multiple patterns" does not
> have anything to do with having to (or allowing to) use '-l'
> multiple times.

Yes, all of this is correct, but not relevant to what I'm describing
in the commit message, because I'm making a documentation change and
describing how you *would* expect git to work if you read the
*documentation*, not if you read the code.

If you read the documentation after Jeff's 588d0e834b, since -l was
still documented as taking a  you'd expect "tag -l 'v*rc*' -l
'*2.8*'" to be how it worked, and for "tag -l 'v*rc*' '*2.8*'" to be
an error.

>> But since it's actually a toggle all of these work as well, and
>> produce identical output to the last example above:
>>
>> git tag --list 'v*rc*' '*2.8*'
>> git tag --list 'v*rc*' '*2.8*' --list --list --list
>> git tag --list 'v*rc*' '*2.8*' --list -l --list -l --list
>
> So citing Jeff's change is irrelevant to explain these.  I doubt you
> even need to show these variations.

Jeff's change isn't being cited to explain these, everything before
the "But since it's actually" is describing how the documentation was
phrased to give you the impression that it worked, including after
Jeff's change where we started accepting multiple patterns. But at
this point I'm starting to describe how the code actually parsed the
--list option.

Regardless of that, I'll try to rephrase this somehow to make it
clearer, but I'd like to keep the general gist of "before this change,
if you read the docs, here's how you'd expect the -l option to work,
but it actually worked like so-and-so, and now that's what we document
instead".

>> Now the documentation is more in tune with how the "branch" command
>> describes its --list option since commit cddd127b9a ("branch:
>> introduce --list option", 2011-08-28).
>>
>> Change the test suite to assert that these invocations work for the
>> cases that weren't already being tested for.
>
> These two paragraphs are relevant.
>
>> --l ::
>> ---list ::
>> - List tags with names that match the given pattern (or all if no
>> - pattern is given).  Running "git tag" without arguments also
>> - lists all tags. The pattern is a shell wildcard (i.e., matched
>> - using fnmatch(3)).  Multiple patterns may be given; if any of
>> - them matches, the tag is shown.
>> +-l::
>> +--list::
>> + Activate the list mode. `git tag ` would try to create a
>
> Dont say  on this line.  It is `git tag `.

Makes sense, but this is something I copied as-is from git-branch.txt,
which then has the same issue, so v3 will have yet another related
patch...

>> + tag, use `git tag --list ` to list matching branches, (or
>
> Perhaps ... instead to signal that you can give multiple
> patterns?

Makes sense.

>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> index 958c77ab86..1de7185be0 100755
>> --- a/t/t7004-tag.sh
>> +++ b/t/t7004-tag.sh
>> @@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists 
>> should succeed' '
>>   git tag
>>  '
>>
>> +cat >expect <> +mytag
>> +EOF
>> +test_expect_success 'Multiple -l or --list options are equivalent to one -l 
>> option' '
>> + git tag -l -l >actual &&
>> + test_cmp expect actual &&
>> + git tag --list --list >actual &&
>> + test_cmp expect actual &&
>> + git tag --list -l --list >actual &&
>> + test_cmp expect actual
>> +'
>
> The "-l/-d/-v" options follow the last-one-wins rule, no?  Perhaps
> also show how this one works in this test (while retitling it)?
>
> git tag -d -v -l

This will fail as tested for in "tag: add more incompatibles mode
tests". We weren't testing "-d" with "-l", or this combination, I'll
add both 

Question: xdiff and "pretty" (human readable) diff output

[matthew]

Good day,

I have a question with respect to how git generates "pretty" (ie: human
readable) unified diffs. It's to my understanding that git uses its own
(simplified/minimized) fork of libxdiff, simply referred to as "xdiff"
[1]. Which tool/library is used to take the xdiff output and generate
the human-readable equivalent that is rendered to the console? I have a
program that I'm maintaining that currently tracks changes to a couple
of "sandboxed" files, and I wanted to add a simple console UI that
periodically shows the changes to the files over time and/or dumps the
"pretty diff" to syslog.

Thank you.

1. "Use a *real* built-in diff generator · git/git@3443546",
  ,
  Accessed 2017-03-22

--Matthew

Re: [PATCH v1] travis-ci: build and test Git on Windows

[Junio C Hamano]

Lars Schneider  writes:

> Therefore, we did the following:
> * Johannes Schindelin set up a Visual Studio Team Services build
>   sponsored by Microsoft and made it accessible via an Azure Function
>   that speaks a super-simple API. We made TravisCI use this API to
>   trigger a build, wait until its completion, and print the build and
>   test results.
> * A Windows build and test run takes up to 3h and TravisCI has a timeout
>   after 50min for Open Source projects. Since the TravisCI job does not
>   use heavy CPU/memory/etc. resources, the friendly TravisCI folks
>   extended the job timeout for git/git to 3h.

The benefit is that Windows CI does not have to subscribe to the
GitHub repository to get notified (instead uses Travis as a relay
for update notification) and the result can be seen at the same
place as results on other platforms Travis natively support are
shown?  

Very nice.

> Things, that would need to be done:
> * Someone with write access to https://travis-ci.org/git/git would need
>   to add the secret token as "GFW_CI_TOKEN" variable in the TravisCI
>   repository setting [1]. Afterwards the build should just work.

We need to make sure this does not leak to the execution log of
Travis.

For example, in https://travis-ci.org/git/git/jobs/213616973, which
is a log of the documentation build for #1335.6 ran for the 'master'
branch, you can see "ci/test-documentation.sh" string appearing in
the log twice.  This comes from "script:" part, which is the same
mechanism this patch uses to invoke the new script with sekrit on
the command line.

I am expecting that no expansion of "$GFW_CI_TOKEN" will be shown in
the output, but I've seen an incident where an unsuspecting 'set -x'
or '$cmd -v' revealed something that shouldn't have been made
public.  I want to make sure we are sure that the command line this
patch adds does not get echoed with expansion to the log.

Is GFW_CI_TOKEN known to be safe without double-quote around it, by
the way?

> Things, that might need to be done:
> * The Windows box can only process a single build at a time. A second
>   Windows build would need to wait until the first finishes.

Perhaps instead of accumulating pending requests, perhaps we can
arrange so that Travis skips a build/test request that is not even
started yet for the same branch?  For branches that are never
rewound, a breakage in an earlier pushout would either show in a
later pushout of the same branch (if breakage is not fixed yet), or
doesn't (if the later pushout was to fix that breakage), and in
either case, it is not useful to test the earlier pushout when a
newer one is already available for testing.  For branches that are
constantly rewound, again, it is not useful to test the earlier
pushout when a newer one is already available for testing.

> diff --git a/ci/run-windows-build.sh b/ci/run-windows-build.sh
> new file mode 100755
> index 00..324a9ea4e6
> --- /dev/null
> +++ b/ci/run-windows-build.sh
> @@ -0,0 +1,55 @@
> +#!/usr/bin/env bash

I know this is not a usual scripted Porcelain that must be usable by
all users, but I do not see anything that requires bash-ism in the
script.  Can we just do "#!/bin/sh", avoid bash-isms, and follow the
usual coding guidelines in general?

> +[ $# -eq 3 ] || (echo "Unexpected number of parameters" && exit 1)

i.e.e.g "test $# = 3" || ...

> +# Check if the $BUILD_ID contains a number
> +case $BUILD_ID in
> + ''|*[!0-9]*) echo $BUILD_ID && exit 1
> +esac

Too deep an indent of a case arm, i.e. align them with case/esac, like

case $BUILD_ID in
''|*[!0-9]*) echo $BUILD_ID && exit 1
esac

Thanks.

Re: [PATCH 0/6] thread lazy_init_name_hash

[Jeff Hostetler]

On 3/22/2017 2:02 PM, Stefan Beller wrote:

On Wed, Mar 22, 2017 at 10:14 AM,   wrote:


During our testing on the Windows source tree (3.1M
files, 500K folders, 450MB index), this change reduced
the runtime of lazy_init_name_hash() from 1.4 to 0.27
seconds.


This sounds promising. :)
A fast skim over the code makes me like the code.


I forgot to mention that this was on an 8 (logical) core machine.




 hashmap.c   |  29 ++-
 hashmap.h   |  25 ++


Could you add some documentation to
Documentation/technical/api-hashmap.txt ?


Will do. Thanks.


(Bonus points for migrating the documentation inline,
c.f. discussion surrounding [1])

[1] https://public-inbox.org/git/20141212212800.ga27...@peff.net/


I'd like to save this for a separate effort.




 name-hash.c | 490 +++-


AFAICT the new threading is all implicit in name-hash and we do not expose
its functionality or tuning knobs outside the testing helper, such that we do
not need API documentation here, but only enough code comments to
understand the code for maintainability?


Yes, my goal was to just make it transparent to all callers.
Especially since the main lazy_init_name_hash() function is static
and not visible anyway.

Jeff

Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

[Stefan Beller]

On Wed, Mar 22, 2017 at 11:56 AM, Junio C Hamano  wrote:
>> So we'd want to be able to say:
>>   "get a tarball including all submodules except the superproject"
>>   (This would produce the "optional language pack tarball")
>
> You do not need that.  Just go to the gitman-l10n project and grab a
> tarball out of it.

Oh, I misunderstood your proposal.
You said: We have *one* submodule for all languages, but I understood
we'd have a submodule for *each* language.

Thanks,
Stefan

Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

[Junio C Hamano]

Stefan Beller  writes:

> I wonder if we could have partial functionality for these "clone and checkout"
> fake submodules, by having e.g. the .gitmodules file telling you the URL
> and path, but no recorded gitlink in the tree.

You can have such a comment in any file including .gitmodules but
would that even be a feature?  

A comment in the INSTALL file was what I had in mind, at least while
we are getting more familiar with the proposed two project structure
and before we commit to use the submodule mechanism to bind them
together.

> So we'd want to be able to say:
>   "get a tarball including all submodules except the superproject"
>   (This would produce the "optional language pack tarball")

You do not need that.  Just go to the gitman-l10n project and grab a
tarball out of it.

>   "get a tarball including the superproject and only one submodule"
>   (This would produce the "I can distribute this in locally as everyone
>   speaks the same language in the organisation" tarball)

We don't need that, either, even though some other project would.
"git archive --recurse-submodules" with properly implemented
pathspec support will solve that.

Re: EOF test fixes (t5615/t7004)

[Stefan Beller]

On Wed, Mar 22, 2017 at 10:35 AM, Jan Palus  wrote:
> Hi,
>
> attached patch fixes 2 out of 3 tests failing in my env -- missing EOF,
> incorrectly placed "&&" after EOF. One test seems to be incorrect as it
> fails even after fixing. I suspect the main difference between my env and
> others is in shell used -- mksh in my case and probably bash in others.
> Looks like bash has a nasty behavior when it encounters syntax error:
>
> $ cat test.sh
> cat >/dev/null <<-\EOF
> tagname : forged-tag
> EOF &&
> echo foo
>
> $ bash test.sh && echo success || echo failed
> test.sh: line 4: warning: here-document at line 1 delimited by
> end-of-file (wanted `EOF')
> success
>
> # notice no "foo" printed
>
> $ mksh test.sh && echo success || echo failed
> test.sh[5]: here document 'EOF' unclosed
> failed
>
> Test that fails even after fixing EOFs:
> t7004-tag.sh:verifying a forged tag with --format fail and format accordingly
>
> Note that I'm not subscribed to mailing list so in case of any questions
> please mail me directly.

Thanks for catching these bugs!

Please have a look at Documentation/SubmittingPatches.
(the most important thing is the "Sign-off-by" line indicating you
are legally permitted to send such a patch;
for one-off patches the format can be negotiated, but it is easier
for maintainers to take the proper format.)

This email conveys the actual problem very well, so only a little
change is needed to make it a proper commit message.
(c.f. git clone git://git.kernel.org/pub/scm/git/git.git/ &&
git -C git log)

Thanks,
Stefan

Re: Stable GnuPG interface, git should use GPGME

[Peter Lebbing]

On 22/03/17 18:15, Werner Koch wrote:
> It actually does.  For the tasks git uses gpg you should not notice a
> difference in gpgme between any of these versions.

Bernhard wrote "interoperability problems between [...] key stores". I'm
under the impression you are actually answering the question "can GPGME
be used in the same way regardless of the GnuPG version" instead?

> Interoperability with 1.4 is a bit cumbersome if you often add new keys.
> However, "gpg --export | gpg1 --import" is not too complicated. 

This presumes that

1) Keys are only updated on the 2.1 side
2) Keys are not deleted
3) Secret keys are never changed

right?

1) is trivially solvable. 2) is trickier, but can be done.

3) is because GnuPG 1.4 cannot update a secret key at all. Adding a new
subkey fails with:

gpg: key DCDFDFA4: already in secret keyring
gpg: Total number processed: 1
gpg:   secret keys read: 1
gpg:  secret keys unchanged: 1

You could delete before re-importing, but what if the key on the 1.4
side is actually the newer one? You'd lose all changes. Worst case:
updates of a single key on both sides. But that is perhaps pushing the
limit of sane use. Perhaps not, perhaps the user likes to use two
different frontends which use different GnuPG versions as their backend.

Luckily, expiration extensions are picked up by just transferring the
public key part of a secret key, so that does work.

Peter.

-- 
I use the GNU Privacy Guard (GnuPG) in combination with Enigmail.
You can send me encrypted mail if you want some privacy.
My key is available at 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 3/3] sequencer: allow the commit-msg hooks to run during a `reword`

[Junio C Hamano]

Johannes Schindelin  writes:

> The `reword` command used to call `git commit` in a manner that asks for
> the prepare-commit-msg and commit-msg hooks to do their thing.
>
> Converting that part of the interactive rebase to C code introduced the
> regression where those hooks were no longer run.
>
> Let's fix this.
>
> Note: the flag is called `VERIFY_MSG` instead of the more intuitive
> `RUN_COMMIT_MSG_HOOKS` to indicate that the flag suppresses the
> `--no-verify` flag (which may do other things in the future in addition
> to suppressing the commit message hooks, too).

Yup, this is a sound reasoning.  I would have made it not a "Note"
but just a regular description of the design decision, e.g.

Call the flag bit "VERIFY_MSG", because this is to suppress the
"--no-verify" flag (do not call it RUN_COMMIT_MSG_HOOKS, as the
purpose of the bit does not have to stay only to run the hooks).

or somesuch, but I can go either way.

> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c| 12 +---
>  t/t7504-commit-msg-hook.sh |  2 +-
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 1abe559fe86..377af91c475 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -606,6 +606,7 @@ N_("you have staged changes in your working tree\n"
>  #define EDIT_MSG(1<<1)
>  #define AMEND_MSG   (1<<2)
>  #define CLEANUP_MSG (1<<3)
> +#define VERIFY_MSG  (1<<4)
>  
>  /*
>   * If we are cherry-pick, and if the merge did not result in
> @@ -642,8 +643,9 @@ static int run_git_commit(const char *defmsg, struct 
> replay_opts *opts,
>   }
>  
>   argv_array_push(, "commit");
> - argv_array_push(, "-n");
>  
> + if (!(flags & VERIFY_MSG))
> + argv_array_push(, "-n");

OK, so by default we pass "--no-verify" but selected callers can
set the bit in the flags word to disable it.  That sounds sensible,
especially given that the original callers, cherry-pick and revert, 
did want "--no-verify".  "reword" in "rebase -i" is currently the
only one we want to supress "--no-verify" and the place that does so
are all shown in this patch.

Just to see if there are other callers that may want to do the same
suppressing of "--no-verify" as a follow-up, I looked at the whole
file after applying the patch, and I think everything looked good
as-is.

>   if ((flags & AMEND_MSG))
>   argv_array_push(, "--amend");
>   if (opts->gpg_sign)
> @@ -993,7 +995,11 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   write_author_script(msg.message);
>   res = fast_forward_to(commit->object.oid.hash, head, unborn,
>   opts);
> - if (res || command != TODO_REWORD)
> + if (res)
> + goto leave;
> + else if (command == TODO_REWORD)
> + flags |= VERIFY_MSG;
> + else
>   goto leave;

Both before and after are your code, but wouldn't this:

if (res || command != TODO_REWORD)
goto leave;
+   if (command == TODO_REWORD)
+   flags |= VERIFY_MSG

result in smaller changes relative to the original and still be much
easier to understand than the above?

Thanks.

Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

[Stefan Beller]

On Wed, Mar 22, 2017 at 11:02 AM, Junio C Hamano  wrote:
> Jean-Noël Avila  writes:
>
>>> I am wondering if Documentation/po part should be a separate
>>> repository, with a dedicated i18n/l10n coordinator.  Would it make
>>> it easier for (1) those who write code and doc without knowing other
>>> languages, (2) those who update .pot and coordinate the l10n effort
>>> for the documentation and (3) those who translate them if we keep
>>> them in a single repository?
>>
>> This is one of the points raised in the first RFC mail. Splitting this
>> part would help a lot manage the translations with their own workflow,
>> would not clutter the main repo with files not really needed for
>> packaging and would simplify dealing with the interaction with crowd
>> translation websites which can directly push translation content to a
>> git repo.
>
> As I was in favor of splitting it out, I was trying to gauge what
> the downside of doing so would be, especially for those who are
> doing the translation work (it is obvious that it would help
> developers who are not translators, as nothing will change for them
> if we keep this new thing as a separate project).
>
> We may still want to fill in the details (and by doing so we may
> discover it is not as easy as I make it sound to be here), but a
> rough outline of what I think we could do is:
>
>  * What you added to Documentation/po/ in these two patches becomes
>a separate project (let's call it "gitman-l10n") and they will be
>at the root level of the project, i.e. documentation.pot and
>documentation.$LANG.po will sit at the top level of the working
>tree of that project, without Documentation/po/ prefix.
>
>The idea is for some of us to have a checkout of "gitman-l10n"
>project inside Documentation/po of the checkout of git.git
>project and achieve a layout similar to what these two patches
>from you create, but keep that optional.
>
>  * In git.git, teach Documentation/Makefile to enable "make
>doc-l10n" and "make install-l10n" targets in "Documentation/" if
>and only if Documentation/po/Makefile exists, and delegate these
>two targets to it, i.e. something like:
>
>(in Documentation/Makefile)
>ifeq ($(wildcard po/Makefile),po/Makefile)
>doc-l10n install-l10n::
> $(MAKE) -C po $@
>endif
>
>Certain Makefile macros Documentation/Makefile knows aboute
>(e.g. location to install, list of pages in the man1 section) may
>have to be exported down to Documentation/po/Makefile.
>
>  * Some other Makefile targets to help i18n coordinator, e.g.
>updating Documentation/po/documentation.pot by using the latest
>set of Documentation/*.txt files, may also have to be added to
>Documentation/Makefile and conditionally enabled the same way
>(i.e. keying off of the presence of po/Makefile).
>
>  * Those who work on the documentation translation and those who
>want to build and install localized documentation will have a
>checkout of the "gitman-l10n" project at "Documentation/po".
>This will _eventually_ be done by making "gitman-l10n" a
>submodule that git.git project uses, but it can start as a manual
>"clone and checkout" without making it a submodule.  Those who do
>not deal with localized manpages can just work with git.git
>proper without even knowing anything about the gitman-l10n
>project.

I wonder if we could have partial functionality for these "clone and checkout"
fake submodules, by having e.g. the .gitmodules file telling you the URL
and path, but no recorded gitlink in the tree.

> I'd prefer to start with the "optional gitman-l10n repository is
> checked out at Documentation/po only by convention" approach, before
> committing to bind it as a submodule.  Once we got comfortable with
> cooperating between these two projects, we do want to bind them
> using the submodule mechanism, but not before.

Yay, submodules in git.git!  Another aspect besides the git-archive
command supporting submodules is the URL management.

The URL for the canonical git.git ought to not change in the near future,
but for proper support we'd need to have a mechanism to have the URL
default and configuration outside the tracked content. The configuration
is already outside the tracked content, but the default is not.

IIUC Jonathan Nieder has a proposal for that (though not in written
form, easy to point at), which consists of having an additional" .gitmodules"
file stored in another ref, e.g. "refs/superproject/master" (Maybe we do not
need it to be branch specific? then a refs/meta/superproject would do).

That branch can be changed
* over time
  This is useful when the canonical URL where to obtain the submodules
  changes. Also when going back in history and then getting the submodules
  would be covered with this as the new ref would not go back in history.
* across people
  Some people prefer the submodules to be fetched 

Re: EOF test fixes (t7030/t7406)

[Jan Palus]

It appears more tests are affected:

$ grep -r '^[[:space:]]*EOF &&' .   
./t7406-submodule-update.sh:EOF &&
./t7030-verify-tag.sh:  EOF &&
./t7030-verify-tag.sh:  EOF &&
./t7004-tag.sh: EOF &&
./t7004-tag.sh: EOF &&

attaching patch for t7406 and t7030 which both fail even after fix is
applied.
diff -ur git-2.12.1.orig/t/t7030-verify-tag.sh git-2.12.1/t/t7030-verify-tag.sh
--- git-2.12.1.orig/t/t7030-verify-tag.sh   2017-03-22 19:20:49.614759549 
+0100
+++ git-2.12.1/t/t7030-verify-tag.sh2017-03-22 19:26:27.608511234 +0100
@@ -126,17 +126,17 @@
 '
 
 test_expect_success 'verifying tag with --format' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : fourth-signed
-   EOF &&
+   EOF
git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
test_cmp expect actual
 '
 
 test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
+   cat >expect <<-\EOF &&
tagname : 7th forged-signed
-   EOF &&
+   EOF
test_must_fail git verify-tag --format="tagname : %(tag)" $(cat 
forged1.tag) >actual-forged &&
test_cmp expect actual-forged
 '
diff -ur git-2.12.1.orig/t/t7406-submodule-update.sh 
git-2.12.1/t/t7406-submodule-update.sh
--- git-2.12.1.orig/t/t7406-submodule-update.sh 2017-03-22 19:20:49.614759549 
+0100
+++ git-2.12.1/t/t7406-submodule-update.sh  2017-03-22 19:25:34.105528379 
+0100
@@ -442,9 +442,9 @@
 '
 
 test_expect_success 'submodule update - command run for initial population of 
submodule' '
-   cat <<-\ EOF >expect
+   cat >expect <<-\EOF &&
Execution of '\''false $submodulesha1'\'' failed in submodule path 
'\''submodule'\''
-   EOF &&
+   EOF
rm -rf super/submodule &&
test_must_fail git -C super submodule update >../actual &&
test_cmp expect actual &&