Re: [PATCH] submodule.c: use GIT_DIR_ENVIRONMENT consistently

[René Scharfe]

Am 30.12.2016 um 01:47 schrieb Stefan Beller:

diff --git a/submodule.c b/submodule.c
index ece17315d6..973b9f3f96 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,5 +1333,6 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
-   argv_array_push(out, "GIT_DIR=.git");
+   argv_array_push(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+   DEFAULT_GIT_DIR_ENVIRONMENT);


argv_array_pushf (with added "f") instead?

And indent continued lines to align them with the left parenthesis, like 
this:


fn(arg1,
   arg2);


 }

Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR

[Jeff King]

On Thu, Dec 29, 2016 at 04:37:30PM -0800, Stefan Beller wrote:

> > +   mkdir lsremote-root &&
> > +   (
> > +   GIT_CEILING_DIRECTORIES=$(pwd) &&
> > +   export GIT_CEILING_DIRECTORIES &&
> > +   cd lsremote-root &&
> > +   git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> > +   ) &&
> 
> We could avoid the subshell via
> 
> GIT_CEILING_DIRECTORIES=$(pwd) \
> git -C lsremote-root lsremote ... >actual
> 
> Not sure if it is worth to trade off a block of code (and an extra shell
> at run time) for an overly long line.
> 
> The rest looks good to me.

I mentioned elsewhere that we now have a "nongit" function to do this as
a one-liner. It might be worth applying your optimization to that
function, so it would take effect in may places.

-Peff

[PATCH] submodule.c: use GIT_DIR_ENVIRONMENT consistently

[Stefan Beller]

In C code we have the luxury of having constants for all the important
things that are hard coded. This is the only place in C, that hard codes
the git directory environment variable, so fix it.

Signed-off-by: Stefan Beller 
---
  Signed-off-by-the-format-patch-config ;)
  
  This is the only occurrence for "GIT_DIR=" in C, but what about ".git"
  git grep "\.git\"" *.c finds some places, which we may want to convert
  to DEFAULT_GIT_DIR_ENVIRONMENT?
  (mainly things that are newer if I can judge the places correctly
  lots of submodules, worktrees and the no data in ".git" bug AFAICT)
  
  Thanks,
  Stefan

 submodule.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index ece17315d6..973b9f3f96 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,5 +1333,6 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
-   argv_array_push(out, "GIT_DIR=.git");
+   argv_array_push(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+   DEFAULT_GIT_DIR_ENVIRONMENT);
 }
-- 
2.11.0.259.ga95e92af08.dirty

Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR

[Jeff King]

On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:

> Thanks.  Here's the patch again, now with commit messages and a test.
> Thanks for the analysis and sorry for the slow turnaround.

Thanks for following up. While working on a similar one recently, I had
the nagging feeling that there was a case we had found that was still to
be dealt with, and I think this was it. :)

The patch to the C code looks good. I have a few comments on the tests:

> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index aeb3a63f7c..a86fca3e6c 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -34,6 +34,21 @@ test_expect_success 'clone http repository' '
>   test_cmp file clone/file
>  '
>  
> +test_expect_success 'list refs from outside any repository' '
> + cat >expect <<-EOF &&
> + $(git rev-parse master) HEAD
> + $(git rev-parse master) refs/heads/master
> + EOF
> + mkdir lsremote-root &&
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd lsremote-root &&
> + git ls-remote "$HTTPD_URL/dumb/repo.git" >../actual
> + ) &&
> + test_cmp expect actual
> +'

Since my recent de95302a4c (t5000: extract nongit function to
test-lib-functions.sh, 2016-12-15), this can be shortened to:

  cat >expect <<-EOF &&
  ...
  EOF
  nongit git ls-remote "$HTTPD_URL/dumb/repo.git" >actual &&
  test_cmp expect actual

I think that commit is in 'master' now.

Without my patch to die() in setup_git_env(), I think this would pass
with or without your patch, right?  I think the current behavior _is_
buggy, but a setup to show off the improvement would be so arcane that I
don't think it is worth it. E.g., something like:

  echo '[http]extraHeader = "Foo: Bar" >nongit/.git/config

would probably trigger the use of that config when it shouldn't. But
that's really stretching.

> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 6e5b9e42fb..7ba894f0f4 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -163,6 +163,21 @@ test_expect_success 'redirects send auth to new 
> location' '
>   expect_askpass both user@host auth/smart/repo.git
>  '
>  
> +test_expect_success 'list refs from outside any repository' '
> + cat >expect <<-EOF &&
> + $(git rev-parse master) HEAD
> + $(git rev-parse master) refs/heads/master
> + EOF
> + mkdir lsremote-root &&
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd lsremote-root &&
> + git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> + ) &&
> + test_cmp expect actual
> +'

Is this really testing anything that the dumb one isn't? The interesting
bit is in invoking git-remote-http, not what it does under the hood. I
don't mind too much being thorough, but if we are going to go that route
we should probably come up with a standard battery of tests for dumb
and smart HTTP, and then invoke them for each case without having to
write each one twice.

> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index 225a022e8a..4573d98e6c 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -35,6 +35,21 @@ test_expect_success 'clone git repository' '
>   test_cmp file clone/file
>  '
>  
> +test_expect_success 'list refs from outside any repository' '
> + cat >expect <<-EOF &&
> + $(git rev-parse master) HEAD
> + $(git rev-parse master) refs/heads/master
> + EOF
> + mkdir lsremote-root &&
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd lsremote-root &&
> + git ls-remote "$GIT_DAEMON_URL/repo.git" >../actual
> + ) &&
> + test_cmp expect actual
> +'

This one isn't actually broken now, right? The test is just there to
catch future regressions?

If we are being thorough, then would we also care about file-local
repos, git-over-ssh, etc?

> diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
> index c6c2661878..7232032cd2 100755
> --- a/t/t5802-connect-helper.sh
> +++ b/t/t5802-connect-helper.sh

This one is quite a big addition. I know this falls under the "while
we're at it" line at the end of your commit message, but maybe it's
worth pulling the GIT_EXT_SERVICE bits out into their own patch (and
explaining briefly what's going on; I had to go look up what
GIT_EXT_SERVICE_NOPREFIX even was).

-Peff

Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR

[Stefan Beller]

> +   mkdir lsremote-root &&
> +   (
> +   GIT_CEILING_DIRECTORIES=$(pwd) &&
> +   export GIT_CEILING_DIRECTORIES &&
> +   cd lsremote-root &&
> +   git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> +   ) &&

We could avoid the subshell via

GIT_CEILING_DIRECTORIES=$(pwd) \
git -C lsremote-root lsremote ... >actual

Not sure if it is worth to trade off a block of code (and an extra shell
at run time) for an overly long line.

The rest looks good to me.

Thanks,
Stefan

[PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR

[Jonathan Nieder]

To push from or fetch to the current repository, remote helpers need
to know what repository that is.  Accordingly, Git sets the GIT_DIR
environment variable to the path to the current repository when
invoking remote helpers.

There is a special case it does not handle: "git ls-remote" and "git
archive --remote" can be run to inspect a remote repository without
being run from any local repository.  GIT_DIR is not useful in this
scenario:

- if we are not in a repository, we don't need to set GIT_DIR to
  override an existing GIT_DIR value from the environment.  If GIT_DIR
  is present then we would be in a repository if it were valid and
  would have called die() if it weren't.

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

So leave GIT_DIR unset in this case.  This makes GIT_DIR easier to
understand for remote helper authors and makes transport code less of
a special case for repository discovery.

Noticed using b1ef400e (setup_git_env: avoid blind fall-back to
".git", 2016-10-20) from 'next':

 $ cd /tmp
 $ git ls-remote https://kernel.googlesource.com/pub/scm/git/git
 fatal: BUG: setup_git_env called without repository

While at it, add some tests for other variables in the environment of
commands run by git-remote-ext.

Helped-by: Jeff King 
Signed-off-by: Jonathan Nieder 
---
Jeff King wrote:

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

Thanks.  Here's the patch again, now with commit messages and a test.
Thanks for the analysis and sorry for the slow turnaround.

Sincerely,
Jonathan

 t/t5550-http-fetch-dumb.sh  | 15 
 t/t5551-http-fetch-smart.sh | 15 
 t/t5570-git-daemon.sh   | 15 
 t/t5802-connect-helper.sh   | 84 -
 transport-helper.c  |  5 +--
 5 files changed, 131 insertions(+), 3 deletions(-)

diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index aeb3a63f7c..a86fca3e6c 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -34,6 +34,21 @@ test_expect_success 'clone http repository' '
test_cmp file clone/file
 '
 
+test_expect_success 'list refs from outside any repository' '
+   cat >expect <<-EOF &&
+   $(git rev-parse master) HEAD
+   $(git rev-parse master) refs/heads/master
+   EOF
+   mkdir lsremote-root &&
+   (
+   GIT_CEILING_DIRECTORIES=$(pwd) &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd lsremote-root &&
+   git ls-remote "$HTTPD_URL/dumb/repo.git" >../actual
+   ) &&
+   test_cmp expect actual
+'
+
 test_expect_success 'create password-protected repository' '
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6e5b9e42fb..7ba894f0f4 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -163,6 +163,21 @@ test_expect_success 'redirects send auth to new location' '
expect_askpass both user@host auth/smart/repo.git
 '
 
+test_expect_success 'list refs from outside any repository' '
+   cat >expect <<-EOF &&
+   $(git rev-parse master) HEAD
+   $(git rev-parse master) refs/heads/master
+   EOF
+   mkdir lsremote-root &&
+   (
+   GIT_CEILING_DIRECTORIES=$(pwd) &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd lsremote-root &&
+   git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
+   ) &&
+   test_cmp expect actual
+'
+
 test_expect_success 'disable dumb http on server' '
git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
config http.getanyfile false
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 225a022e8a..4573d98e6c 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -35,6 +35,21 @@ test_expect_success 'clone git repository' '
test_cmp file clone/file
 '
 
+test_expect_success 'list refs from outside any repository' '
+   cat >expect <<-EOF &&
+   $(git rev-parse master) HEAD
+   $(git rev-parse master) refs/heads/master
+   EOF
+   mkdir lsremote-root &&
+   (
+   GIT_CEILING_DIRECTORIES=$(pwd) &&
+   export GIT_CEILING_DIRECTORIES &&
+   cd lsremote-root &&
+   git ls-remote "$GIT_DAEMON_URL/repo.git" >../actual
+   ) &&
+   test_cmp expect actual
+'
+
 test_expect_success 'fetch changes via git protocol' '
echo content >>file &&
git commit -a -m two &&
diff --git a/t/t5802-connect-helper.sh 

Re: Bug: Assertion failed: function prefix_pathspec, file pathspec.c, line 317.

[Stefan Beller]

On Thu, Dec 29, 2016 at 3:37 PM, Rafal W  wrote:
> The following error happens when I'm running "git add ." in the submodule dir:
>

Thanks for reporting!

Please see the patch that I sent out earlier today:
https://public-inbox.org/git/20161229192908.32633-1-sbel...@google.com/

I wonder if the error message is sufficient, i.e. would you liked to have
more help reported by git?

Bug: Assertion failed: function prefix_pathspec, file pathspec.c, line 317.

[Rafal W]

The following error happens when I'm running "git add ." in the submodule dir:

$ GIT_TRACE=1 git add .
23:25:18.313575 git.c:350   trace: built-in: git 'add' '.'
Assertion failed: (item->nowildcard_len <= item->len && item->prefix
<= item->len), function prefix_pathspec, file pathspec.c, line 317.
Abort trap: 6

Then git crashes, so I've to remove index.lock manually
(.git/modules/Foo/index.lock).

This is what I did before it start doing this:
1. I've renamed GitHub repository to different name.
2. Created new repository in place of the old one (which consist other
submodules).
3. In previously cloned submodule dir (which was pointing to previous
repo before rename) I did: git fetch origin
4. Then I did: git reset origin/master --hard
5. 'git status' looks ok
6. Moved some of the files into submodule dir of the current submodule
and entered that dir.
7. From now on, the git add crashes with trap 6:  git add .

Probably there are some easier steps to reproduce the issue, but this
is what I did.

OS: macOS Sierra
git version 2.8.4

(lldb) bt
* thread #1: tid = 0x2338e, 0x7fffbe6a1dda
libsystem_kernel.dylib`__pthread_kill + 10, queue =
'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x7fffbe6a1dda libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x7fffbe78c797 libsystem_pthread.dylib`pthread_kill + 90
frame #2: 0x7fffbe607440 libsystem_c.dylib`abort + 129
frame #3: 0x7fffbe5ce8b3 libsystem_c.dylib`__assert_rtn + 320
frame #4: 0x0001000d83f9 git`parse_pathspec + 2917
frame #5: 0x00012ded git`cmd_add + 991
frame #6: 0x00011e64 git`handle_builtin + 357
frame #7: 0x00011877 git`main + 288

Other people could reproduce the same thing, also on Mac, see:
http://stackoverflow.com/q/23205961

Kind regards,
kenorb

Re: [PATCH] am: add am.signoff add config variable

[Stefan Beller]

On Thu, Dec 29, 2016 at 2:43 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> IANAL either, but we have been striving to keep output of
>>>
>>>$ git grep '\.signoff' Documentation
>>
>>>
>>> empty to keep Sign-off meaningful.
>>
>> Try again with -i ;)
>> and you'll find format.signOff
>
> Mistakes happen.  Finding an old mistake is not an excuse for you to
> make the same one again.
> It is an opportunity to come up with a way
> to correct it without hurting existing users by designing a smooth
> transition path.
>

knee jerk reaction:

1) Document why format.signoff is bad (even after a long
  office discussion I am not fully convinced it is bad. That
  may be because I am biased as I find format.signoff *very*
  useful. The cumbersome contribution process as laid out
  by SubmittingPatches just got easier for me as I have one
  step less to worry about. I haven't made a mistake so far
  sending out crap where I'll throw a temper tantrum if you
  apply it.

  So I would expect a maintainer of a project that uses
  email based workflow to write that documentation giving
  reasons. That person is currently consuming the automatically
  signed off patches, so I'd want to know their line of thinking.

2) If the config option is set, but no explicit sign off is given,
  put a different footer, e.g.
git config format.signoff true &&
git format-patch HEAD^
  may produce Auto-Signed-Off-By: ...
  whereas
git -c format.signoff format-patch
  behaves the same as
git format-patch --signoff
  that gives the Signed-Off-By as we know it.

  It is up to the upstream project to accept these new sign offs.

3) (later) warn about the option if it is set, giving the text from 1)

4) (a long time later) remove the option.

--
For 2) I am not sure what we want there, because this
has to happen in collaboration with all the upstream projects that
use sign offs.

We could be subtle, i.e. just use all lowercase / all uppercase
letters for this differentiation. Then automated tools that check for signoff
are easily adjusted. e.g. The eclipse foundation disallows pushing for
review if any patch is missing a signoff; Gerrit can check for that.
Gerrit is not case sensitive when checking for a footer.

I am not sure if there are any other tools out there that automatically
check for that, but I would assume they are also case insensitive in
such a case, as it is unclear to me how to properly capitalize the sign off.

This is an easy way forward for upstream projects., though confusing in
court later on.
--
We could also be non-subtle, very explicit, and each tool that
can add sign offs currently, needs to be explicit about itself:

Configured-Formatpatch-Signed-Off: (for git format.signoff with config)
# and others:
Explicit-Formatpatch-Signed-Off:
Git-Gui-Button-Clicked-Signed-Off:
Git-Gui-Button-Shortcut-Signed-Off:

Once we have that we could add much more of these:
Configured-Commit-Signed-Off:
etc.

You can continue to sign off via just typing it, or by
I-typed-it-signed-Off,
--
One of the problems highlighted to me was that you could have accidentally
configured format.signoff globally, but you're only allowed/desire to sign off
in a particular repository, such that

Repolocal-Configured-Formatpatch-Signed-Off:
Global-Configured-Formatpatch-Signed-Off:

may be worth discussing.
--

Re: [PATCH] am: add am.signoff add config variable

[Junio C Hamano]

Stefan Beller  writes:

>> IANAL either, but we have been striving to keep output of
>>
>>$ git grep '\.signoff' Documentation
>
>>
>> empty to keep Sign-off meaningful.
>
> Try again with -i ;)
> and you'll find format.signOff

Mistakes happen.  Finding an old mistake is not an excuse for you to
make the same one again.  It is an opportunity to come up with a way
to correct it without hurting existing users by designing a smooth
transition path.

Re: [PATCH] am: add am.signoff add config variable

[Stefan Beller]

On Thu, Dec 29, 2016 at 1:42 PM, Junio C Hamano  wrote:
> Eric Wong  writes:
>
>> Eduardo Habkost  wrote:
>>> git-am has options to enable --message-id and --3way by default,
>>> but no option to enable --signoff by default. Add a "am.signoff"
>>> config option.
>>
>> I'm not sure this is a good idea.  IANAL, but a sign-off
>> has some sort of legal meaning for this project (DCO)
>> and that would be better decided on a patch-by-patch basis
>> rather than a blanket statement.
>
> IANAL either, but we have been striving to keep output of
>
>$ git grep '\.signoff' Documentation

Try again with -i ;)
and you'll find format.signOff

>
> empty to keep Sign-off meaningful.
>
> Adding more publicized ways to add SoB without thinking will make it
> harder to argue against one who tells the court "that log message
> ends with a SoB by person X but it is very plausible that it was
> done by inertia without person X really intending to certify what
> DCO says, and the SoB is meaningless".

I think we should be symmetrical, am is the opposite of
format-patch in the Git <-> email conversion.

If I were to follow your arguments here, we should revert
1d1876e930 (Add configuration variable for sign-off to format-patch)

On the other hand, I would argue that thinking and typing things is
orthogonal (the more you type, doesn't imply that you think harder
or even at all).

--
However I think there is a use case for such an option
(in the short term?) as e.g. you as the Git maintainer being employed
has the legal rights to sign off on pretty much any patch in Git.
For other projects this may be different.

Which is why a per-repository configurable thing is useful to
setup a default for your environment.

IIUC long term we rather want to have easily configurable trailers
for am/format-patch/commit, such that you could configure to
remove any "ChangeId:" footers before sending out, or adding
a "Tested-by" footer by a CI system or such?

>
>> I don't add my SoB to patches (either my own or received) until
>> I'm comfortable with it;

comfortable is orthogonal to legal, specifically on the receiving side.
I can understand using format-patch to send out unsigned patches
(e.g. for heavy WIP things, "please to not apply literally")

>> and I'd rather err on the side of
>> forgetting and being prodded to resubmit rather than putting
>> an SoB on the wrong patch.
>

Re: [PATCH] am: add am.signoff add config variable

[Eduardo Habkost]

On Thu, Dec 29, 2016 at 01:42:13PM -0800, Junio C Hamano wrote:
> Eric Wong  writes:
> 
> > Eduardo Habkost  wrote:
> >> git-am has options to enable --message-id and --3way by default,
> >> but no option to enable --signoff by default. Add a "am.signoff"
> >> config option.
> >
> > I'm not sure this is a good idea.  IANAL, but a sign-off
> > has some sort of legal meaning for this project (DCO)
> > and that would be better decided on a patch-by-patch basis
> > rather than a blanket statement.
> 
> IANAL either, but we have been striving to keep output of
> 
>$ git grep '\.signoff' Documentation
> 
> empty to keep Sign-off meaningful. 
> 
> Adding more publicized ways to add SoB without thinking will make it
> harder to argue against one who tells the court "that log message
> ends with a SoB by person X but it is very plausible that it was
> done by inertia without person X really intending to certify what
> DCO says, and the SoB is meaningless".

This sounds completely reasonable to me. I now see that the
config option was already proposed in 2011 and the same arguments
were discussed. Sorry for the noise.

> 
> > I don't add my SoB to patches (either my own or received) until
> > I'm comfortable with it; and I'd rather err on the side of
> > forgetting and being prodded to resubmit rather than putting
> > an SoB on the wrong patch.
> 

-- 
Eduardo

Re: [PATCH] am: add am.signoff add config variable

[Junio C Hamano]

Eric Wong  writes:

> Eduardo Habkost  wrote:
>> git-am has options to enable --message-id and --3way by default,
>> but no option to enable --signoff by default. Add a "am.signoff"
>> config option.
>
> I'm not sure this is a good idea.  IANAL, but a sign-off
> has some sort of legal meaning for this project (DCO)
> and that would be better decided on a patch-by-patch basis
> rather than a blanket statement.

IANAL either, but we have been striving to keep output of

   $ git grep '\.signoff' Documentation

empty to keep Sign-off meaningful. 

Adding more publicized ways to add SoB without thinking will make it
harder to argue against one who tells the court "that log message
ends with a SoB by person X but it is very plausible that it was
done by inertia without person X really intending to certify what
DCO says, and the SoB is meaningless".

> I don't add my SoB to patches (either my own or received) until
> I'm comfortable with it; and I'd rather err on the side of
> forgetting and being prodded to resubmit rather than putting
> an SoB on the wrong patch.

[PATCHv2] unpack-trees: move checkout state into check_updates

[Stefan Beller]

The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.

The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.

Signed-off-by: Stefan Beller 
---

Rene wrote:
> we could use "index" instead of ">result".  Not sure if it's worth a 
> patch, though.

done in this iteration of the patch.
I also reordered the variables at the beginning of the function for readability.

Thanks,
Stefan

 unpack-trees.c | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 7a6df99d10..eb44f50e65 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -218,14 +218,18 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
 }
 
-static int check_updates(struct unpack_trees_options *o,
-const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
 {
unsigned cnt = 0, total = 0;
struct progress *progress = NULL;
struct index_state *index = >result;
-   int i;
-   int errs = 0;
+   struct checkout state = CHECKOUT_INIT;
+   int i, errs = 0;
+
+   state.force = 1;
+   state.quiet = 1;
+   state.refresh_cache = 1;
+   state.istate = index;
 
if (o->update && o->verbose_update) {
for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -240,7 +244,7 @@ static int check_updates(struct unpack_trees_options *o,
}
 
if (o->update)
-   git_attr_set_direction(GIT_ATTR_CHECKOUT, >result);
+   git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
 
@@ -251,7 +255,7 @@ static int check_updates(struct unpack_trees_options *o,
continue;
}
}
-   remove_marked_cache_entries(>result);
+   remove_marked_cache_entries(index);
remove_scheduled_dirs();
 
for (i = 0; i < index->cache_nr; i++) {
@@ -264,7 +268,7 @@ static int check_updates(struct unpack_trees_options *o,
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
-   errs |= checkout_entry(ce, state, NULL);
+   errs |= checkout_entry(ce, , NULL);
}
}
}
@@ -1094,14 +1098,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
int i, ret;
static struct cache_entry *dfc;
struct exclude_list el;
-   struct checkout state = CHECKOUT_INIT;
 
if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
-   state.force = 1;
-   state.quiet = 1;
-   state.refresh_cache = 1;
-   state.istate = >result;
 
memset(, 0, sizeof(el));
if (!core_apply_sparse_checkout || !o->update)
@@ -1238,7 +1237,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
}
 
o->src_index = NULL;
-   ret = check_updates(o, ) ? (-2) : 0;
+   ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
if (!o->result.cache_tree)
-- 
2.11.0.259.ga95e92af08.dirty

[PATCH] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

Every once in a while someone complains to the mailing list to have
run into this weird assertion[1].

The usual response from the mailing list is link to old discussions[2],
and acknowledging the problem stating it is known.

For now just improve the user visible error message.

[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] 
http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html

Signed-off-by: Stefan Beller 
---

 * a more defensive check and message
 * with tests!
 
 pathspec.c   | 19 +--
 t/t6134-pathspec-in-submodule.sh | 33 +
 2 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..b446d79615 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,23 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
 
/* sanity checks, pathspec matchers assume these are sane */
-   assert(item->nowildcard_len <= item->len &&
-  item->prefix <= item->len);
+   if (item->nowildcard_len > item->len ||
+   item->prefix > item->len) {
+   /* Historically this always was a submodule issue */
+   for (i = 0; i < active_nr; i++) {
+   struct cache_entry *ce = active_cache[i];
+   int ce_len = ce_namelen(ce);
+   int len = ce_len < item->len ? ce_len : item->len;
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+   if (!memcmp(ce->name, item->match, len))
+   die (_("Pathspec '%s' is in submodule '%.*s'"),
+   item->original, ce_len, ce->name);
+   }
+   /* The error is a new unknown bug */
+   die ("BUG: item->nowildcard_len > item->len || item->prefix > 
item->len)");
+   }
+
return magic;
 }
 
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 00..e62dbb7327
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+   test_commit 1 &&
+   git submodule add ./ sub &&
+   git commit -a -m "add submodule" &&
+   git submodule deinit --all
+'
+
+cat sub/a &&
+   test_must_fail git add sub/a 2>actual &&
+   test_cmp expect actual
+'
+
+cat actual &&
+   test_cmp expect actual
+'
+
+test_done
-- 
2.11.0.259.ga95e92af08.dirty

Re: [PATCH] am: add am.signoff add config variable

[Jacob Keller]

On December 29, 2016 12:47:01 AM PST, Eric Wong  wrote:
>Eduardo Habkost  wrote:
>> git-am has options to enable --message-id and --3way by default,
>> but no option to enable --signoff by default. Add a "am.signoff"
>> config option.
>
>I'm not sure this is a good idea.  IANAL, but a sign-off
>has some sort of legal meaning for this project (DCO)
>and that would be better decided on a patch-by-patch basis
>rather than a blanket statement.
>
>I don't add my SoB to patches (either my own or received) until
>I'm comfortable with it; and I'd rather err on the side of
>forgetting and being prodded to resubmit rather than putting
>an SoB on the wrong patch.
>
>
>(I'm barely online today, no rush needed in responding)

I don't know what is true for all projects, but I can't believe making it 
configurable would cause problems If you don't want it you don't have to 
enable it.

I suppose your argument is that we shouldn't ever make it automatically but... 
I know that many people who receive email patches, apply them, then forward 
those to another group add their sign off as a mark of "yes I certify that this 
is legally OK" so I think this would be useful.

Thanks
Jake

Re: [PATCH v3] am: add am.signoff add config variable

[Eduardo Habkost]

On Thu, Dec 29, 2016 at 08:58:36AM +0100, Andreas Schwab wrote:
> On Dez 28 2016, Eduardo Habkost  wrote:
> 
> > @@ -32,10 +32,12 @@ OPTIONS
> > If you supply directories, they will be treated as Maildirs.
> >  
> >  -s::
> > ---signoff::
> > +--[no-]-signoff::
> 
> That's one dash too much.

Oops. I can fix it in v4, but I will first wait to see what
others think about the legal implications of setting it by
default (see point raised by Eric Wong on v1).

-- 
Eduardo

Re: [PATCH] am: add am.signoff add config variable

[Eduardo Habkost]

On Thu, Dec 29, 2016 at 08:47:01AM +, Eric Wong wrote:
> Eduardo Habkost  wrote:
> > git-am has options to enable --message-id and --3way by default,
> > but no option to enable --signoff by default. Add a "am.signoff"
> > config option.
> 
> I'm not sure this is a good idea.  IANAL, but a sign-off
> has some sort of legal meaning for this project (DCO)
> and that would be better decided on a patch-by-patch basis
> rather than a blanket statement.
> 
> I don't add my SoB to patches (either my own or received) until
> I'm comfortable with it; and I'd rather err on the side of
> forgetting and being prodded to resubmit rather than putting
> an SoB on the wrong patch.

This is an interesting point. I am not competent to argue about
it, so I will let the Git developers decide.

-- 
Eduardo

Re: [PATCH v2] am: add am.signoff add config variable

[Eduardo Habkost]

On Thu, Dec 29, 2016 at 01:29:33PM +0530, Pranit Bauva wrote:
> Hey Eduardo,
> 
> On Thu, Dec 29, 2016 at 12:49 AM, Eduardo Habkost  wrote:
> >> test_expect_success '--no-signoff overrides am.signoff' '
> >>   rm -fr .git/rebase-apply &&
> >>   git reset --hard first &&
> >>   test_config am.signoff true &&
> >>   git am --no-signoff  >>   printf "%s\n" "$signoff" >expected &&
> >>   git cat-file commit HEAD^ | grep "Signed-off-by:" >actual &&
> >>   test_cmp expected actual &&
> >>   git cat-file commit HEAD | grep "Signed-off-by:" >actual &&
> >>   test_must_be_empty actual
> >> '
> >>
> >> The test fails because the second "grep" command returns a
> >> non-zero exit code. Any suggestions to avoid that problem in a
> >> more idiomatic way?
> >
> > I just found out that "test_must_fail grep ..." is a common
> > idiom, so what about:
> 
> Is there any particular reason to use "grep" instead of "test_cmp"? To
> check for non-zero error code, you can always use "! test_cmp".

The test code is checking only the "Signed-off-by" lines, not the
whole commit message. "test_cmp" would require recovering the
entire contents of the original commit message, which would add
complexity to the test code.

-- 
Eduardo

Re: [PATCH] string-list: make compare function compatible with qsort(3)

[René Scharfe]

Am 21.12.2016 um 17:12 schrieb Jeff King:

On Wed, Dec 21, 2016 at 10:36:41AM +0100, René Scharfe wrote:


One shortcoming is that the comparison function is restricted to working
with the string members of items; util is inaccessible to it.  Another
one is that the value of cmp is passed in a global variable to
cmp_items(), making string_list_sort() non-reentrant.


I think this approach is OK for string_list, but it doesn't help the
general case that wants qsort_s() to actually access global data. I
don't know how common that is in our codebase, though.

So I'm fine with it, but I think we might eventually need to revisit the
qsort_s() thing anyway.


I have to admit I didn't even consider the possibility that the pattern 
of accessing global variables in qsort(1) compare functions could have 
spread.


And indeed, at least ref-filter.c::compare_refs() and 
worktree.c::compare_worktree() so that as well.  The latter uses 
fspathcmp(), which is OK as ignore_case is only set once when reading 
the config, but the first one looks, well, interesting.  Perhaps a 
single ref filter per program is enough?


Anyway, that's a good enough argument for me for adding that newfangled 
C11 function after all..


Btw.: Found with

  git grep 'QSORT.*;$' |
  sed 's/.* /int /; s/);//' |
  sort -u |
  git grep -Ww -f-

and

  git grep -A1 'QSORT.*,$'


Remove the intermediate layer, i.e. cmp_items(), make the comparison
functions compatible with qsort(3) and pass them pointers to full items.
This allows comparisons to also take the util member into account, and
avoids the need to pass the real comparison function to an intermediate
function, removing the need for a global function.


I'm not sure if access to the util field is really of any value, after
looking at it in:

  
http://public-inbox.org/git/20161125171546.fa3zpapbjngjc...@sigill.intra.peff.net/

Though note that if we do take this patch, there are probably one or two
spots that could switch from QSORT() to string_list_sort().


Yes, but as you noted in that thread there is not much point in doing 
that; the only net win is that we can pass a list as a single pointer 
instead of as base pointer and element count -- the special compare 
function needs to be specified anyway (once), somehow.


René

Re: [PATCH] unpack-trees: move checkout state into check_updates

[René Scharfe]

Am 29.12.2016 um 00:26 schrieb Stefan Beller:

The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.

The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.


Thanks for catching this, the result looks nicer.


Signed-off-by: Stefan Beller 
---
 unpack-trees.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index ea799d37c5..78703af135 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -224,14 +224,18 @@ static void unlink_entry(const struct cache_entry *ce)
schedule_dir_for_removal(ce->name, ce_namelen(ce));
 }

-static int check_updates(struct unpack_trees_options *o,
-const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
 {
unsigned cnt = 0, total = 0;
struct progress *progress = NULL;
struct index_state *index = >result;
int i;
int errs = 0;
+   struct checkout state = CHECKOUT_INIT;
+   state.force = 1;
+   state.quiet = 1;
+   state.refresh_cache = 1;
+   state.istate = >result;


And here (as well as in two more places in this function) we could use 
"index" instead of ">result".  Not sure if it's worth a patch, though.




if (o->update && o->verbose_update) {
for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -270,7 +274,7 @@ static int check_updates(struct unpack_trees_options *o,
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
-   errs |= checkout_entry(ce, state, NULL);
+   errs |= checkout_entry(ce, , NULL);
}
}
}
@@ -1100,14 +1104,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
int i, ret;
static struct cache_entry *dfc;
struct exclude_list el;
-   struct checkout state = CHECKOUT_INIT;

if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
-   state.force = 1;
-   state.quiet = 1;
-   state.refresh_cache = 1;
-   state.istate = >result;

memset(, 0, sizeof(el));
if (!core_apply_sparse_checkout || !o->update)
@@ -1244,7 +1243,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
}

o->src_index = NULL;
-   ret = check_updates(o, ) ? (-2) : 0;
+   ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
if (!o->result.cache_tree)

Re: HowTo distribute a hook with the reposity.

[Lars Schneider]

> On 28 Dec 2016, at 19:53, Jacob Keller  wrote:
> 
> On Tue, Dec 27, 2016 at 10:08 PM, Jeff King  wrote:
>> 
>>   https://github.com/Autodesk/enterprise-config-for-git
>> 
>> (with the disclaimer that I've never used it myself, so I have no
>> idea how good it is).
>> 
>> I think you probably know all that, Jake, but I am laying it out for the
>> benefit of the OP and the list. :)
>> 
> 
> Heh, well I didn't know about this last part so it's still useful to
> me! I knew of the larger description for why not but wasn't exactly
> clear how to word it so I am glad you filled that in. Thanks!

Author of the "enterprise-config-for-git" here. The version in the public
Git repo is a stripped down version of the one we use at my day job for
our engineering workforce. I tried to extract the "generally useful bits".
Unfortunately it cannot be used "as-is". Don't hesitate to ping me if you
want to learn more about it and/or if you have an idea how to make it
better suited for open source collaboration.

Cheers,
Lars

[PATCH v2] git-p4: do not pass '-r 0' to p4 commands

[Igor Kushnir]

git-p4 crashes when used with a very old p4 client version
that does not support the '-r ' option in its commands.

Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.

Alternatively git-p4.retries could be made opt-in.
But since only very old, barely maintained p4 versions don't support
the '-r' option, the setting-retries-to-0 workaround would do.

The "-r retries" option is present in Perforce 2012.2 Command Reference,
but absent from Perforce 2012.1 Command Reference.

Signed-off-by: Igor Kushnir 
---
 Documentation/git-p4.txt | 2 ++
 git-p4.py| 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index bae862ddc..7436c64a9 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -479,6 +479,8 @@ git-p4.client::
 git-p4.retries::
Specifies the number of times to retry a p4 command (notably,
'p4 sync') if the network times out. The default value is 3.
+   Set the value to 0 to disable retries or if your p4 version
+   does not support retries (pre 2012.2).
 
 Clone and sync variables
 
diff --git a/git-p4.py b/git-p4.py
index 22e3f57e7..7bda915bd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
 if retries is None:
 # Perform 3 retries by default
 retries = 3
-real_cmd += ["-r", str(retries)]
+if retries > 0:
+# Provide a way to not pass this option by setting git-p4.retries to 0
+real_cmd += ["-r", str(retries)]
 
 if isinstance(cmd,basestring):
 real_cmd = ' '.join(real_cmd) + ' ' + cmd
-- 
2.11.0

Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)

[Lars Schneider]

> On 28 Dec 2016, at 00:11, Junio C Hamano  wrote:
> 
> 
> * bw/realpath-wo-chdir (2016-12-22) 5 commits
>  (merged to 'next' on 2016-12-22 at fea8fa870f)
> + real_path: canonicalize directory separators in root parts
> + real_path: have callers use real_pathdup and strbuf_realpath
> + real_path: create real_pathdup
> + real_path: convert real_path_internal to strbuf_realpath
> + real_path: resolve symlinks by hand
> (this branch is used by bw/grep-recurse-submodules.)
> 
> The implementation of "real_path()" was to go there with chdir(2)
> and call getcwd(3), but this obviously wouldn't be usable in a
> threaded environment.  Rewrite it to manually resolve relative
> paths including symbolic links in path components.

"real_path: resolve symlinks by hand" (05b458c) introduces
"MAXSYMLINKS" which is already defined on macOS in

/usr/include/sys/param.h:197:9:

 * .., MAXSYMLINKS defines the
 * maximum number of symbolic links that may be expanded in a path name.
 * It should be set high enough to allow all legitimate uses, but halt
 * infinite loops reasonably quickly.
 */


Log with JS: https://travis-ci.org/git/git/jobs/187092215
Log without JS: 
https://s3.amazonaws.com/archive.travis-ci.org/jobs/187092215/log.txt

- Lars

Re: What's cooking in git.git (Dec 2016, #08; Tue, 27)

[Duy Nguyen]

On Thu, Dec 29, 2016 at 1:18 AM, Brandon Williams  wrote:
> On 12/27, Junio C Hamano wrote:
>> * bw/pathspec-cleanup (2016-12-14) 16 commits
>>  - pathspec: rename prefix_pathspec to init_pathspec_item
>>  - pathspec: small readability changes
>>  - pathspec: create strip submodule slash helpers
>>  - pathspec: create parse_element_magic helper
>>  - pathspec: create parse_long_magic function
>>  - pathspec: create parse_short_magic function
>>  - pathspec: factor global magic into its own function
>>  - pathspec: simpler logic to prefix original pathspec elements
>>  - pathspec: always show mnemonic and name in unsupported_magic
>>  - pathspec: remove unused variable from unsupported_magic
>>  - pathspec: copy and free owned memory
>>  - pathspec: remove the deprecated get_pathspec function
>>  - ls-tree: convert show_recursive to use the pathspec struct interface
>>  - dir: convert fill_directory to use the pathspec struct interface
>>  - dir: remove struct path_simplify
>>  - mv: remove use of deprecated 'get_pathspec()'
>>
>>  Code clean-up in the pathspec API.
>>
>>  Waiting for the (hopefully) final round of review before 'next'.
>
> What more needs to be reviewed for this series?

I wanted to have a look at it but I successfully managed to put if off
so far. Will do soonish.
-- 
Duy

Re: [PATCH] git-p4: do not pass '-r 0' to p4 commands

[Lars Schneider]

> On 29 Dec 2016, at 10:05, Igor Kushnir  wrote:
> 
> git-p4 crashes when used with a very old p4 client version
> that does not support the '-r ' option in its commands.
> 
> Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.
> 
> Alternatively git-p4.retries could be made opt-in.
> But since only very old, barely maintained p4 versions don't support
> the '-r' option, the setting-retries-to-0 workaround would do.
> 
> The "-r retries" option is present in Perforce 2012.2 Command Reference,
> but absent from Perforce 2012.1 Command Reference.

Thanks for this workaround!


> Signed-off-by: Igor Kushnir 
> ---
> Documentation/git-p4.txt | 1 +
> git-p4.py| 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index bae862ddc..f4f1be5be 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -479,6 +479,7 @@ git-p4.client::
> git-p4.retries::
>   Specifies the number of times to retry a p4 command (notably,
>   'p4 sync') if the network times out. The default value is 3.
> + '-r 0' is not passed to p4 commands if this option is set to 0.

At this point in the docs we have never talked about the "-r" flag and I
would argue it is an "implementation detail". Therefore, I would prefer
something like: "Set the value to 0 if want to disable retries or if 
your p4 version does not support retries (pre 2012.2)."


> 
> Clone and sync variables
> 
> diff --git a/git-p4.py b/git-p4.py
> index 22e3f57e7..e5a9e1cce 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
> if retries is None:
> # Perform 3 retries by default
> retries = 3
> -real_cmd += ["-r", str(retries)]
> +if retries != 0:

How about "retries > 0"?


> +# Provide a way to not pass this option by setting git-p4.retries to > 0
> +real_cmd += ["-r", str(retries)]
> 
> if isinstance(cmd,basestring):
> real_cmd = ' '.join(real_cmd) + ' ' + cmd
> -- 
> 2.11.0
> 

Re: How can I make a best dissertation paper?

[neiljakson]

I am very glad to learn a lot from you this meaningful knowledge. From an
article describing your unique way , we can see that you are an approachable
, humorous person. Not only that, your article is rich with a lot of useful
knowledge and helpful information.
Dissertation writing services
 
 



--
View this message in context: 
http://git.661346.n2.nabble.com/How-can-I-make-a-best-dissertation-paper-tp7657453p7657460.html
Sent from the git mailing list archive at Nabble.com.

Re: [PATCH] git-p4: do not pass '-r 0' to p4 commands

[Luke Diamand]

On 29 December 2016 at 09:05, Igor Kushnir  wrote:
> git-p4 crashes when used with a very old p4 client version
> that does not support the '-r ' option in its commands.
>
> Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.
>
> Alternatively git-p4.retries could be made opt-in.
> But since only very old, barely maintained p4 versions don't support
> the '-r' option, the setting-retries-to-0 workaround would do.
>
> The "-r retries" option is present in Perforce 2012.2 Command Reference,
> but absent from Perforce 2012.1 Command Reference.

That looks like a good fix, thanks.

I feel sad for anyone still using 2012.1.

Luke


>
> Signed-off-by: Igor Kushnir 
> ---
>  Documentation/git-p4.txt | 1 +
>  git-p4.py| 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index bae862ddc..f4f1be5be 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -479,6 +479,7 @@ git-p4.client::
>  git-p4.retries::
> Specifies the number of times to retry a p4 command (notably,
> 'p4 sync') if the network times out. The default value is 3.
> +   '-r 0' is not passed to p4 commands if this option is set to 0.
>
>  Clone and sync variables
>  
> diff --git a/git-p4.py b/git-p4.py
> index 22e3f57e7..e5a9e1cce 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
>  if retries is None:
>  # Perform 3 retries by default
>  retries = 3
> -real_cmd += ["-r", str(retries)]
> +if retries != 0:
> +# Provide a way to not pass this option by setting git-p4.retries to > 0
> +real_cmd += ["-r", str(retries)]
>
>  if isinstance(cmd,basestring):
>  real_cmd = ' '.join(real_cmd) + ' ' + cmd
> --
> 2.11.0
>

[PATCH] git-p4: do not pass '-r 0' to p4 commands

[Igor Kushnir]

git-p4 crashes when used with a very old p4 client version
that does not support the '-r ' option in its commands.

Allow making git-p4 work with old p4 clients by setting git-p4.retries to 0.

Alternatively git-p4.retries could be made opt-in.
But since only very old, barely maintained p4 versions don't support
the '-r' option, the setting-retries-to-0 workaround would do.

The "-r retries" option is present in Perforce 2012.2 Command Reference,
but absent from Perforce 2012.1 Command Reference.

Signed-off-by: Igor Kushnir 
---
 Documentation/git-p4.txt | 1 +
 git-p4.py| 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index bae862ddc..f4f1be5be 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -479,6 +479,7 @@ git-p4.client::
 git-p4.retries::
Specifies the number of times to retry a p4 command (notably,
'p4 sync') if the network times out. The default value is 3.
+   '-r 0' is not passed to p4 commands if this option is set to 0.
 
 Clone and sync variables
 
diff --git a/git-p4.py b/git-p4.py
index 22e3f57e7..e5a9e1cce 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -83,7 +83,9 @@ def p4_build_cmd(cmd):
 if retries is None:
 # Perform 3 retries by default
 retries = 3
-real_cmd += ["-r", str(retries)]
+if retries != 0:
+# Provide a way to not pass this option by setting git-p4.retries to 0
+real_cmd += ["-r", str(retries)]
 
 if isinstance(cmd,basestring):
 real_cmd = ' '.join(real_cmd) + ' ' + cmd
-- 
2.11.0

Re: [PATCH] am: add am.signoff add config variable

[Eric Wong]

Eduardo Habkost  wrote:
> git-am has options to enable --message-id and --3way by default,
> but no option to enable --signoff by default. Add a "am.signoff"
> config option.

I'm not sure this is a good idea.  IANAL, but a sign-off
has some sort of legal meaning for this project (DCO)
and that would be better decided on a patch-by-patch basis
rather than a blanket statement.

I don't add my SoB to patches (either my own or received) until
I'm comfortable with it; and I'd rather err on the side of
forgetting and being prodded to resubmit rather than putting
an SoB on the wrong patch.


(I'm barely online today, no rush needed in responding)

Re: [PATCH v2] am: add am.signoff add config variable

[Pranit Bauva]

Hey Eduardo,

On Thu, Dec 29, 2016 at 12:49 AM, Eduardo Habkost  wrote:
>> test_expect_success '--no-signoff overrides am.signoff' '
>>   rm -fr .git/rebase-apply &&
>>   git reset --hard first &&
>>   test_config am.signoff true &&
>>   git am --no-signoff >   printf "%s\n" "$signoff" >expected &&
>>   git cat-file commit HEAD^ | grep "Signed-off-by:" >actual &&
>>   test_cmp expected actual &&
>>   git cat-file commit HEAD | grep "Signed-off-by:" >actual &&
>>   test_must_be_empty actual
>> '
>>
>> The test fails because the second "grep" command returns a
>> non-zero exit code. Any suggestions to avoid that problem in a
>> more idiomatic way?
>
> I just found out that "test_must_fail grep ..." is a common
> idiom, so what about:

Is there any particular reason to use "grep" instead of "test_cmp"? To
check for non-zero error code, you can always use "! test_cmp".

Regards,
Pranit Bauva