Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Junio C Hamano]

Santiago Torres  writes:

> ... It seems Debian oldstable and other variants still ship gpg1,
> which doesn't have it. Would it make sense to have a fallthrough branch
> on the switch statement for gpg2.1 instead? something like the attached patch.

If the problem of leftover agent is limited to a narrow versions of
GPG2, alternatively we could unconditionally attempt to use gpgconf
and ignore the failure ;-), but as long as we know all problematic
ones identify themselves as "gpg (GnuPG) 2.1*", then your patch
(with s/;&/;;/ of course ;-)) sounds very sensible.

> Thanks,

Thank *you* for working on this.

> From 07ab87c1ddb31197a3a5c124ad5a2462a460d4e3 Mon Sep 17 00:00:00 2001
> From: Santiago Torres 
> Date: Tue, 18 Jul 2017 13:16:11 -0400
> Subject: [RFC/PATCH] t: lib-gpg: flush gpg agent on startup

Perhaps it is about time we lost RFC/ mark from here.

>
> When running gpg-relevant tests, a gpg-daemon is spawned for each
> GNUPGHOME used. This daemon may stay running after the test and cache
> file descriptors for the trash directories, even after the trash
> directory is removed. This leads to ENOENT errors when attempting to
> create files if tests are run multiple times.
>
> Add a cleanup script to force flushing the gpg-agent for that GNUPGHOME
> (if any) before setting up the GPG relevant-environment.
>
> Helped-by: Junio C Hamano 
> Signed-off-by: Santiago Torres 
> ---
>  t/lib-gpg.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index ec2aa8f68..ffb20a438 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -10,6 +10,8 @@ then
>   'gpg (GnuPG) 1.0.6'*)
>   say "Your version of gpg (1.0.6) is too buggy for testing"
>   ;;
> + 'gpg (GnuPG) 2.1'*)
> + GNUPGHOME="$(pwd)/gpghome" gpgconf --kill all ;&
>   *)
>   # Available key info:
>   # * Type DSA and Elgamal, size 2048 bits, no expiration date,

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Santiago Torres]

> Oh, wait, I can run "gpg" just fine, but I do not seem to have
> gpgconf.
> 
>   $ type gpgconf
>   bash: type: gpgconf: not found
> 
> The patch may need a bit more cross-version work, it seems.

Right, sorry about that. 

I was testing against Debian Stretch/Arch, who do ship gpg2 with
gpgconf. It seems Debian oldstable and other variants still ship gpg1,
which doesn't have it. Would it make sense to have a fallthrough branch
on the switch statement for gpg2.1 instead? something like the attached patch.

Thanks,
-Santiago.
From 07ab87c1ddb31197a3a5c124ad5a2462a460d4e3 Mon Sep 17 00:00:00 2001
From: Santiago Torres 
Date: Tue, 18 Jul 2017 13:16:11 -0400
Subject: [RFC/PATCH] t: lib-gpg: flush gpg agent on startup

When running gpg-relevant tests, a gpg-daemon is spawned for each
GNUPGHOME used. This daemon may stay running after the test and cache
file descriptors for the trash directories, even after the trash
directory is removed. This leads to ENOENT errors when attempting to
create files if tests are run multiple times.

Add a cleanup script to force flushing the gpg-agent for that GNUPGHOME
(if any) before setting up the GPG relevant-environment.

Helped-by: Junio C Hamano 
Signed-off-by: Santiago Torres 
---
 t/lib-gpg.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index ec2aa8f68..ffb20a438 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -10,6 +10,8 @@ then
 	'gpg (GnuPG) 1.0.6'*)
 		say "Your version of gpg (1.0.6) is too buggy for testing"
 		;;
+	'gpg (GnuPG) 2.1'*)
+		GNUPGHOME="$(pwd)/gpghome" gpgconf --kill all ;&
 	*)
 		# Available key info:
 		# * Type DSA and Elgamal, size 2048 bits, no expiration date,
-- 
2.13.3



signature.asc
Description: PGP signature

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Junio C Hamano]

Santiago Torres  writes:

> On Mon, Jul 17, 2017 at 03:09:44PM -0700, Junio C Hamano wrote:
>> I am not sure if it is merely "if it's even necessary"; if there are
>> two tests running in parallel, with their own separate
>> $TRASH_DIRECTORY, and one of them say "kill the agent" at the
>> beginning, would it affect the other test, depending on the timing?
>
> This shouldn't happen, provided their respective GNUPGHOME env vars
> aren't the same. A gpg-agent process is spawned per GNUPGHOME on the
> versions that I've tested.
>
>> 
>> I would imagine that the sockets are kept per GNUPGHOME and they are
>> not going to interfere, so if that is the case, I do not think we
>> mind helping folks with a buggy versions of GnuPG by having a "let's
>> be cautious and kill a leftover agent before starting to test"
>> patch, as long as the reason why we do so is clearly understood and
>> documented.
>
> I double checked the patch/solutions/causes just to be sure I'm not
> doing anything crazy. Here's a v2 of the patch that kills the agent upon
> cleanup rather than startup.

Thanks.  

The workaround of killing the lingering agent makes sense to me, but
is test_done the right place to do so?  When "sh t7004-tag.sh -i"
dies in the middle, I do not think the control even reaches the
invocation of that shell function at the end.

The httpd tests seem to do this in start_httpd shell function:

start_httpd () {
prepare_httpd 
trap 'code=$?; stop_httpd: (exit $code); die' EXIT
...

and I can see how this would avoid the "'-i' option can stop the
test script in the middle" issue.  Because the GPG tests do not have
a clear "start agent" step, we'd need to arrange the "trap" in the
case arm in t/lib-gpg.sh where we set GPG prerequisite if we were to
use the same approach.  Or killing a possibly lingering one like your
first patch did may be simpler (we need to make sure &&-chain works
well with "gpgconf --kill all").

Oh, wait, I can run "gpg" just fine, but I do not seem to have
gpgconf.

$ type gpgconf
bash: type: gpgconf: not found

The patch may need a bit more cross-version work, it seems.


-- >8 --
From: Santiago Torres 
Date: Tue, 18 Jul 2017 13:16:11 -0400
Subject: [RFC/PATCH] t: test-lib: flush gpg agent on cleanup

When running gpg-relevant tests, a gpg-daemon is spawned for each
GNUPGHOME used. This daemon may stay running after the testand cache ile
descriptors for the trash directories, even after the trash directory is
removed. This leads to ENOENT errors when attempting to create files if
tests are run multiple times.

Add a cleanup script to force flushing the gpg-agent when before
removing the trash directory if the test is GPG-relevant.

Helped-by: Junio C Hamano 
Signed-off-by: Santiago Torres 
---
 t/test-lib.sh | 5 +
 1 file changed, 5 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1b6e53f78..ed8796d7a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -732,6 +732,11 @@ test_done () {
EOF
fi
 
+   if test_have_prereq GPG
+   then
+   GNUPGHOME="$TRASH_DIRECTORY/gpghome" gpgconf --kill all
+   fi
+
if test "$test_fixed" != 0
then
say_color error "# $test_fixed known breakage(s) vanished; 
please update test(s)"
-- 
2.13.3

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Santiago Torres]

On Mon, Jul 17, 2017 at 03:09:44PM -0700, Junio C Hamano wrote:
> I am not sure if it is merely "if it's even necessary"; if there are
> two tests running in parallel, with their own separate
> $TRASH_DIRECTORY, and one of them say "kill the agent" at the
> beginning, would it affect the other test, depending on the timing?

This shouldn't happen, provided their respective GNUPGHOME env vars
aren't the same. A gpg-agent process is spawned per GNUPGHOME on the
versions that I've tested.

> 
> I would imagine that the sockets are kept per GNUPGHOME and they are
> not going to interfere, so if that is the case, I do not think we
> mind helping folks with a buggy versions of GnuPG by having a "let's
> be cautious and kill a leftover agent before starting to test"
> patch, as long as the reason why we do so is clearly understood and
> documented.

I double checked the patch/solutions/causes just to be sure I'm not
doing anything crazy. Here's a v2 of the patch that kills the agent upon
cleanup rather than startup.

Thanks!
-Santiago.
From 20491890b804d13f9edb0205c1cc21d080beffe2 Mon Sep 17 00:00:00 2001
From: Santiago Torres 
Date: Tue, 18 Jul 2017 13:16:11 -0400
Subject: [RFC/PATCH] t: test-lib: flush gpg agent on cleanup

When running gpg-relevant tests, a gpg-daemon is spawned for each
GNUPGHOME used. This daemon may stay running after the testand cache ile
descriptors for the trash directories, even after the trash directory is
removed. This leads to ENOENT errors when attempting to create files if
tests are run multiple times.

Add a cleanup script to force flushing the gpg-agent when before
removing the trash directory if the test is GPG-relevant.

Helped-by: Junio C Hamano 
Signed-off-by: Santiago Torres 
---
 t/test-lib.sh | 5 +
 1 file changed, 5 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1b6e53f78..ed8796d7a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -732,6 +732,11 @@ test_done () {
 		EOF
 	fi
 
+	if test_have_prereq GPG
+	then
+		GNUPGHOME="$TRASH_DIRECTORY/gpghome" gpgconf --kill all
+	fi
+
 	if test "$test_fixed" != 0
 	then
 		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
-- 
2.13.3



signature.asc
Description: PGP signature

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Junio C Hamano]

Santiago Torres  writes:

> Other projects such as notmuch opted for a solution that's simlar to
> what I had suggested[1], but I wonder if it's even necessary to do.
> There is already a fix on the master branch of gnupg[2], which I imagine
> will show up to the next version of gpg2.
>
> I don't think it would make sense to fix anything on our side, unless we
> want to be extra sure the test suite is not leaking agents for all gpg
> versions (including these minor versions). 

I am not sure if it is merely "if it's even necessary"; if there are
two tests running in parallel, with their own separate
$TRASH_DIRECTORY, and one of them say "kill the agent" at the
beginning, would it affect the other test, depending on the timing?

I would imagine that the sockets are kept per GNUPGHOME and they are
not going to interfere, so if that is the case, I do not think we
mind helping folks with a buggy versions of GnuPG by having a "let's
be cautious and kill a leftover agent before starting to test"
patch, as long as the reason why we do so is clearly understood and
documented.

Thanks for digging it to the root cause.

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Santiago Torres]

> > I'll dig into this. This sounds a way more reasonable approach.
> 
> Thanks.  Another thing that may help, if it turns out that we do
> want to let agent run when it wants to

I did some digging on the reason as to why this was happening. It turns
out there is a bug on gnupg. As of gpg 2.1.21, all operations require an
agent running (BTW --no-use-agent is effectively a no-op now). 


Given that on versions 2.19-21, they changed the way sockets are
created to be stored on /run/user/UID/gnupg/... deleting the $GNUPG_HOME
directory wouldn't flush the agent socket, and would leave an agent
instance per test running, possibly forever. E.g., make test would
result in the following:

santiago at ~ ✔ pgrep -a gpg-agent
632 gpg-agent --homedir /git/t/trash directory.t6050-replace/gpghome 
--use-standard-socket --daemon
1192 /usr/bin/gpg-agent --supervised
2939 gpg-agent --homedir /git/t/trash 
directory.t5801-remote-helpers/gpghome --use-standard-socket --daemon
4656 gpg-agent --homedir /git/t/trash directory.t6300-for-each-ref/gpghome 
--use-standard-socket --daemon
5427 gpg-agent --homedir /git/t/trash directory.t7510-signed-commit/gpghome 
--use-standard-socket --daemon
5898 gpg-agent --homedir /git/t/trash 
directory.t6302-for-each-ref-filter/gpghome --use-standard-socket --daemon
7747 gpg-agent --homedir /git/t/trash directory.t7003-filter-branch/gpghome 
--use-standard-socket --daemon
12922 gpg-agent --homedir /git/t/trash directory.t7600-merge/gpghome 
--use-standard-socket --daemon
13572 gpg-agent --homedir /git/t/trash directory.t7004-tag/gpghome 
--use-standard-socket --daemon
14521 gpg-agent --homedir /git/t/trash directory.t5534-push-signed/gpghome 
--use-standard-socket --daemon
16563 gpg-agent --homedir /git/t/trash 
directory.t5541-http-push-smart/gpghome --use-standard-socket --daemon
17853 gpg-agent --homedir /git/t/trash directory.t7030-verify-tag/gpghome 
--use-standard-socket --daemon
29858 gpg-agent --homedir /git/t/trash 
directory.t7612-merge-verify-signatures/gpghome --use-standard-socket --daemon
31100 gpg-agent --homedir /git/t/trash directory.t4202-log/gpghome 
--use-standard-socket --daemon

Other projects such as notmuch opted for a solution that's simlar to
what I had suggested[1], but I wonder if it's even necessary to do.
There is already a fix on the master branch of gnupg[2], which I imagine
will show up to the next version of gpg2.

I don't think it would make sense to fix anything on our side, unless we
want to be extra sure the test suite is not leaking agents for all gpg
versions (including these minor versions). 

What is your take?

Thanks!
-Santiago.

[1] https://paste.debian.net/976970/
[2] https://dev.gnupg.org/T3218


signature.asc
Description: PGP signature

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Let's see what *nix does:
>
> $ rm -rf /tmp/{master,backup}; mkdir /tmp/master && cd /tmp/master && mv 
> /tmp/{master,backup} ; file /tmp/{master,backup}
>
> Similarly to that, when you're on "master" "git branch --move backup"
> could have left you on an orphan branch, but it doesn't, it's the
> equivalent of "mv && cd" in *nix terms.

And with the same analogy,

mkdir /tmp/master
cd /tmp/master
cp -R /tmp/master /tmp/backup
pwd

would show you that you are still in /tmp/master.  Which is quite
different from what happens with "mv".  So what's your point?

In any case, you already realize that it is a crazy and convoluted
example nobody does in real life to go in to one directory *and*
rename it while you are inside it, don't you?

The reason why I do not think it is wise to move to the backup
branch when you did

git checkout master
git branch --copy master backup

is not because of that crazy analogy, though.  It is primarily that
I think one of the major reasons people want to do a "copy" to a
backup is to keep the pre-modification state of a thing that they
may be able to later come back to when it turns out to be needed.
For that workflow, with the above "branch --copy", your user is
planning to make a change that is not usually kept track of by Git
to the "master" branch (perhaps branch.*.description is changed,
perhaps the remote-tracking branch it is set to integrate with is
changed, etc.), but the user is unsuare, and preparing a backup that
can be used with "gir branch -M backup master" to come back to.  

You can do all that even if your "branch --copy" initially takes you
to the "backup" branch you just created, but then the user has to
immediately come back to "master" to start doing the major surgery
on "master", for which the "copy" was a preparation to recover.

> So since our --move is really --move-and-checkout I think it would be
> confusing to introduce a --copy sister option that has the semantics of
> --copy-no-checkout instead of a corresponding --copy-and-checkout.

Our --move is *NOT* --move-and-checkout.

git chekckout master
git branch naster
git branch -m naster aster

will *not* move "naster" to "aster" and check it out.  During the
whole exercise, you will stay on "master".  

When you rename the current branch so that the name of the current
branch will no longer exist in the system, you _could_ detach the
HEAD, or you _could_ move the current branch to the new name.  There
is no sensible alternative other than these two, but either way, you
need to have a special case for renaming the current branch.  It's
just the latter is more useful in practice and that is the only
reason why it moves _you_ along with the current branch if you
happen to be on that branch being renamed.

I do not see much reason why such a special case has to apply to
"copy".  The source of the copy is not going away, and in the
"backup" scenario, moving away from the thing that is backed up, in
preparation for further work that may have to be reverted, is
actively counter-productive.

There however _is_ an opposite use case.  You may want to start
working on a _new_ branch that is more similarly set up to what your
current branch is, _and_ you want the new branch to start at your
current branch.  But I think that should be done by adding an
enhanced mode of "checkout -b".  IOW, I think

git checkout master
git checkout -b --with-configuration naster [master]

is a very sensible thing to desire; if "master" is set to integrate
with refs/remotes/origin/master, the new "naster" branch may want to
integrate with refs/remotes/origin/naster (instead of the local
"master", which is what the traditional "checkout -b" would do), for
example.  Or you could do the same thing with

git branch --copy master naster
git checkout naster

if your "branch --copy" does not switch out of the current branch.

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 13 2017, Junio C. Hamano jotted:

> Here are the topics that have been cooking.  Commits prefixed with
> [...]
>
> * jc/allow-lazy-cas (2017-07-06) 1 commit
>  - push: disable lazy --force-with-lease by default
>
>  Because "git push --force-with-lease[=]" that relies on the
>  stability of remote-tracking branches is unsafe when something
>  fetches into the repository behind user's back, it is now disabled
>  by default.  A new configuration variable can be used to enable it
>  by users who know what they are doing.  This would pave the way to
>  possibly turn `--force` into `--force-with-lease`.
>
>  Will wait for feedback, then merge to and cook in 'next'.

Aside from the feedback I just sent now 87k23a2d28@gmail.com,
there's my outstanding comment on the config variable name in
877ezkbn6x@gmail.com which is waiting on your feedback.

> * sd/branch-copy (2017-06-18) 3 commits
>  - branch: add a --copy (-c) option to go with --move (-m)
>  - branch: add test for -m renaming multiple config sections
>  - config: create a function to format section headers
>
>  "git branch" learned "-c/-C" to create and switch to a new branch
>  by copying an existing one.
>
>  Undecided.
>
>  I personally do not think "branch --copy master backup" while on
>  "master" that switches to "backup" is a good UI, and I *will* say
>  "I told you so" when users complain after we merge this down to
>  'next' and eventually to 'master'.

For anyone following along with this. The reason Sahil & I hacked it up
like that is because that's analogous to what the --move option does.

Let's see what *nix does:

$ rm -rf /tmp/{master,backup}; mkdir /tmp/master && cd /tmp/master && mv 
/tmp/{master,backup} ; file /tmp/{master,backup}
/tmp/master: cannot open `/tmp/master' (No such file or directory)
/tmp/backup: directory

Similarly to that, when you're on "master" "git branch --move backup"
could have left you on an orphan branch, but it doesn't, it's the
equivalent of "mv && cd" in *nix terms.

So since our --move is really --move-and-checkout I think it would be
confusing to introduce a --copy sister option that has the semantics of
--copy-no-checkout instead of a corresponding --copy-and-checkout.

I think it's easier to explain & use an option that's "like --move
except the old branch doesn't get deleted", which is what this does,
instead of "actually not analogous to --move at all".

I happen to want to use this for something where the semantics on that
topic work better for me, but I recognize that that's just a matter of
taste, if we were green-fielding this I wouldn't mind either way.

But since we're not, and especially with all the confusion around
checkout/branch (some of the hairiest UX in git) I think our --move &
--copy should be symmetric in the same way that mv(1) an cp(1) are
symmetric. Let's not add yet another special case to whether a ref is
created/checked out when being manipulated by some mode of
checkout/branch.

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Junio C Hamano]

Santiago Torres  writes:

>> Combined with the unknown-ness of the root cause of the issue, I can
>> only say that the patch may be raising an issue worth addressing,
>> but it is too sketchy to tell if it is a right solution or what the
>> exact problem being solved is.
>
> I'll dig into this. This sounds a way more reasonable approach.

Thanks.  Another thing that may help, if it turns out that we do
want to let agent run when it wants to, may be to study and mimick
the way our webserver tests arrange how the servers are killed at
the end of the test.

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Santiago Torres]

Hi, Junio. Thanks for replying.

> I postponed it when I saw it the first time to see if anybody
> comments on it, and then it turns out nobody was interested, and it
> remained uninteresting to the list to this day.  
> 

True, that's what I was afraid of, but I wanted to give it some closure.

> Now, after looking at the message again, from the patch description,
> I would believe you that you experienced _some_ trouble when the
> gpg-agent that is auto-spawned by gpg gets left behind (as I do not
> see any hits from "git grep gpg-agent t/", we are not deliberately
> using the agent). 

True, this is what I could gather from asking people over at #gnupg. The
agent spawns a socket for a GNUPGHOME and leaves it open outside of the
home folder (and it caches the inode for the olddir or so).

> However, I could not convince myself that the
> solution is credible.  

I think you're right on this. I'd rather have more people reproduce the
issue (some of my colleagues were able to do so, but we all were running
the latest GPG, vanilla conf) and find the root of the issue too.

> but the current directory of this part is the $TRASH_DIRECTORY,
> which is always created anew from the beginning in a test.  What
> agent process could possibly be running there immedately after
> creating ./gpghome (which happens with "mkdir gpghome &&" without
> "-p" just before the context of this hunk---if the agent was running
> already, the directory would have been there, and mkdir would have
> failed, which would have caused "test_set_prereq GPG" at the end of
> the "&&" cascade to be skipped.  In other words, it is unclear to
> me, and your log message does not sufficiently explain, why this is
> the right solution (or what the exact cause of the symptom is, for
> that matter).

I see. What is causing this (as far as my current understanding goes)
is:

1) First iteration of the tests is run, the .gpghome is created and a
unix socket is created too. The keychain is imported etc. Tests run
normally.

2) The test ends, and the trash directory (along with the .gpghome) are
flushed, but the agent is not aware of this. The socket is still
open.

3) The second iteration of the tests is run, the new .gpghome is
created, but when the keychain fails to import and the agent errors
out with ENOENT. The and-chain fails and test_set_preqreq GPG is
skipped (as you pointed out).

This last bit is apparently a result of the agent trying to be clever
with the paths. 

> 
> Or perhaps the gpg-agent socket is created somewhere outside the
> GNUPGHOME and stays behind even after a previous run of the same
> test finishes and $TRASH_DIRECTORY gets cleared (I am guessing the
> "what the exact cause is" part, as the log message did not explain
> it)?  If that is the case, it makes me wonder if either of the two
> alternative may be a more robust solution: (1) running gpg tests
> telling gpg never to use agent, or (2) telling gpg and gpg-agent to
> use a socket inside GNUPGHOME.

I agree. In hindsight this solution seems rather naive. I'll dig into
the root cause, as well as to try to isolate the issue from a
gpg-version and gpg-config viewpoint.

> After all, "kill"ing agent blindly like the above patch would mean
> you do not know what other party is relying on the proper operation
> of the thing you are killing.  That sounds more like a workaround
> that a solution (unless it is explained with a solid reason why that
> is the right way to run more than one instances of GPG).

I agree. It is probably better to seek the solutions that you suggested
above.

> 
> Perhaps everybody else is running these gpg tests without having to
> worry about gpg-agent already because their environment is more
> vanilla, but you have some configuration or environment that cause
> gpg to use agent, and that is the reason why nobody is interested
> (because they have never seen the symptom)?  It is possible that the
> part of t/test-lib.sh that tries to cleanse environment variables
> and other "external influence" to give us a predictable and
> repeatable test is unaware of such a knob that only some developers
> (including you) have and the rest of us were merely lucky.  Perhaps
> we need to throw GPG_AGENT_INFO SSH_AUTH_SOCK etc. into the list of
> envirionment variables to nuke there?
> 
> Combined with the unknown-ness of the root cause of the issue, I can
> only say that the patch may be raising an issue worth addressing,
> but it is too sketchy to tell if it is a right solution or what the
> exact problem being solved is.

I'll dig into this. This sounds a way more reasonable approach.

Thanks for the feedback!
-Santiago.


signature.asc
Description: PGP signature

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Junio C Hamano]

Santiago Torres  writes:

>> Here are the topics that have been cooking.  
>
> I sent (a patch almost a week ago) that would probably[1] be labeled
> as "uninteresting" (as per the notes from the maintainer), but I wanted
> to make sure it wasn't lost in the noise -- I see that theres a lot of
> active development lately. I checked the latest iterations of "what's
> cooking" to see if it was going to be discarded or so, but I see no
> mention of it.

I postponed it when I saw it the first time to see if anybody
comments on it, and then it turns out nobody was interested, and it
remained uninteresting to the list to this day.  

Now, after looking at the message again, from the patch description,
I would believe you that you experienced _some_ trouble when the
gpg-agent that is auto-spawned by gpg gets left behind (as I do not
see any hits from "git grep gpg-agent t/", we are not deliberately
using the agent).  However, I could not convince myself that the
solution is credible.  Here is an excerpt from the patch:

> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index ec2aa8f68..22ef2fa87 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -31,6 +31,7 @@ then
>   chmod 0700 ./gpghome &&
>   GNUPGHOME="$(pwd)/gpghome" &&
>   export GNUPGHOME &&
> + gpgconf --kill gpg-agent &&
>   gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
>   "$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
>   gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
> -- 

but the current directory of this part is the $TRASH_DIRECTORY,
which is always created anew from the beginning in a test.  What
agent process could possibly be running there immedately after
creating ./gpghome (which happens with "mkdir gpghome &&" without
"-p" just before the context of this hunk---if the agent was running
already, the directory would have been there, and mkdir would have
failed, which would have caused "test_set_prereq GPG" at the end of
the "&&" cascade to be skipped.  In other words, it is unclear to
me, and your log message does not sufficiently explain, why this is
the right solution (or what the exact cause of the symptom is, for
that matter).

Or perhaps the gpg-agent socket is created somewhere outside the
GNUPGHOME and stays behind even after a previous run of the same
test finishes and $TRASH_DIRECTORY gets cleared (I am guessing the
"what the exact cause is" part, as the log message did not explain
it)?  If that is the case, it makes me wonder if either of the two
alternative may be a more robust solution: (1) running gpg tests
telling gpg never to use agent, or (2) telling gpg and gpg-agent to
use a socket inside GNUPGHOME.

After all, "kill"ing agent blindly like the above patch would mean
you do not know what other party is relying on the proper operation
of the thing you are killing.  That sounds more like a workaround
that a solution (unless it is explained with a solid reason why that
is the right way to run more than one instances of GPG).

Perhaps everybody else is running these gpg tests without having to
worry about gpg-agent already because their environment is more
vanilla, but you have some configuration or environment that cause
gpg to use agent, and that is the reason why nobody is interested
(because they have never seen the symptom)?  It is possible that the
part of t/test-lib.sh that tries to cleanse environment variables
and other "external influence" to give us a predictable and
repeatable test is unaware of such a knob that only some developers
(including you) have and the rest of us were merely lucky.  Perhaps
we need to throw GPG_AGENT_INFO SSH_AUTH_SOCK etc. into the list of
envirionment variables to nuke there?

Combined with the unknown-ness of the root cause of the issue, I can
only say that the patch may be raising an issue worth addressing,
but it is too sketchy to tell if it is a right solution or what the
exact problem being solved is.

Re: What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Santiago Torres]

> Here are the topics that have been cooking.  

Hi, 

I sent (a patch almost a week ago) that would probably[1] be labeled
as "uninteresting" (as per the notes from the maintainer), but I wanted
to make sure it wasn't lost in the noise -- I see that theres a lot of
active development lately. I checked the latest iterations of "what's
cooking" to see if it was going to be discarded or so, but I see no
mention of it.

Thanks!
-Santiago

[1] 
https://public-inbox.org/git/20170707220729.a3xrsju3rf4guyzs@LykOS.localdomain/T/#t


signature.asc
Description: PGP signature

What's cooking in git.git (Jul 2017, #04; Thu, 13)

[Junio C Hamano]

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

A maintenance release for 2.13.x series, and the first preview for
2.14 series -rc0, have been tagged.  Let's close the 'master' except
for obvious fixes and clean-ups and concentrate on regressions from
now on.

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

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

--
[Graduated to "master"]

* ab/grep-lose-opt-regflags (2017-06-30) 6 commits
  (merged to 'next' on 2017-07-05 at 375c0b92ea)
 + grep: remove redundant REG_NEWLINE when compiling fixed regex
 + grep: remove regflags from the public grep_opt API
 + grep: remove redundant and verbose re-assignments to 0
 + grep: remove redundant "fixed" field re-assignment to 0
 + grep: adjust a redundant grep pattern type assignment
 + grep: remove redundant double assignment to 0

 Code cleanup.


* jk/build-with-asan (2017-07-10) 5 commits
  (merged to 'next' on 2017-07-10 at 49757e1119)
 + Makefile: disable unaligned loads with UBSan
 + Makefile: turn off -fomit-frame-pointer with sanitizers
 + Makefile: add helper for compiling with -fsanitize
 + test-lib: turn on ASan abort_on_error by default
 + test-lib: set ASAN_OPTIONS variable before we run git

 The build procedure has been improved to allow building and testing
 Git with address sanitizer more easily.


* kn/ref-filter-branch-list (2017-07-10) 4 commits
  (merged to 'next' on 2017-07-10 at 35fd25c0dd)
 + ref-filter.c: drop return from void function
 + branch: set remote color in ref-filter branch immediately
 + branch: use BRANCH_COLOR_LOCAL in ref-filter format
 + branch: only perform HEAD check for local branches

 The rewrite of "git branch --list" using for-each-ref's internals
 that happened in v2.13 regressed its handling of color.branch.local;
 this has been fixed.


* ks/fix-rebase-doc-picture (2017-07-10) 1 commit
  (merged to 'next' on 2017-07-10 at 3acb856b17)
 + doc: correct a mistake in an illustration

 Doc update.


* rs/apply-avoid-over-reading (2017-07-09) 1 commit
  (merged to 'next' on 2017-07-10 at 2d8191ec3f)
 + apply: use strcmp(3) for comparing strings in gitdiff_verify_name()

 Code cleanup.


* rs/urlmatch-cleanup (2017-07-09) 1 commit
  (merged to 'next' on 2017-07-10 at 2dd3e7cab0)
 + urlmatch: use hex2chr() in append_normalized_escapes()

 Code cleanup.


* rs/use-div-round-up (2017-07-10) 1 commit
  (merged to 'next' on 2017-07-10 at accb7919da)
 + use DIV_ROUND_UP

 Code cleanup.


* rs/wt-status-cleanup (2017-07-10) 1 commit
  (merged to 'next' on 2017-07-10 at d8939f683a)
 + wt-status: use separate variable for result of shorten_unambiguous_ref

 Code cleanup.


* sb/hashmap-customize-comparison (2017-06-30) 3 commits
  (merged to 'next' on 2017-07-06 at cc420805f3)
 + hashmap: migrate documentation from Documentation/technical into header
 + patch-ids.c: use hashmap correctly
 + hashmap.h: compare function has access to a data field
 (this branch is used by sb/diff-color-move and sb/hashmap-cleanup.)

 Update the hashmap API so that data to customize the behaviour of
 the comparison function can be specified at the time a hashmap is
 initialized.


* sb/pull-rebase-submodule (2017-06-27) 4 commits
  (merged to 'next' on 2017-07-09 at 48d2c3a51c)
 + builtin/fetch cleanup: always set default value for submodule recursing
 + pull: optionally rebase submodules (remote submodule changes only)
 + builtin/fetch: parse recurse-submodules-default at default options parsing
 + builtin/fetch: factor submodule recurse parsing out to submodule config

 "git pull --rebase --recurse-submodules" learns to rebase the
 branch in the submodules to an updated base.


* sb/submodule-doc (2017-06-22) 1 commit
  (merged to 'next' on 2017-07-09 at fda0ceec31)
 + submodules: overhaul documentation

 Doc update.

--
[New Topics]

* jn/hooks-pre-rebase-sample-fix (2017-07-11) 1 commit
  (merged to 'next' on 2017-07-12 at ed86887454)
 + pre-rebase hook: capture documentation in a