* Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites
From: Junio C Hamano @ 2013-01-05 1:17 UTC (permalink / raw)
To: David Michael; +Cc: git@vger.kernel.org
In-Reply-To: <CAEvUa7niTJVfp8_kuWs50kvhfZ59F-yAuAmeOXEduHXOq-tRFA@mail.gmail.com>
David Michael <fedora.dm0@gmail.com> writes:
> I have encountered an issue with consecutive calls to getenv
> overwriting earlier values. Most notably, it prevents a plain "git
> clone" from working.
>
> Long story short: This value of GIT_DIR gets passed around setup.c
> until it reaches check_repository_format_gently. This function calls
> git_config_early, which eventually runs getenv("HOME"). When it
> returns back to check_repository_format_gently, the gitdir variable
> contains my home directory path. The end result is that I wind up
> with ~/objects/ etc. and a failed repository clone. (Simply adding a
> bare getenv("GIT_DIR") afterwards to reset the pointer also corrects
> the problem.)
>
> Since other platforms are apparently working, yet this getenv behavior
> is supported by the standards, I am left wondering if this could be a
> symptom of something else being broken on my platform (z/OS).
The execve(2) function
int execve(const char *filename, char *const argv[],
char *const envp[]);
takes a NULL terminated array of NUL terminated strings of form
"VAR=VAL" in envp[], and this is kept in:
extern char **environ;
of the new image that runs.
The most naive and straight-forward way to implement getenv(3) is to
iterate over this environ[] array to look for an element that begins
with "GIT_DIR=", and return the pointer pointing at the location one
byte past that equal sign. So even if the standard allowed the
returned value to be volatile across calls to getenv(3), it will
take *more* work for implementations if they want to break our use
pattern. They have to deliberately return a string that they will
overwrite in subsequent calls to getenv(3).
Also the natural way to implement putenv(3) and setenv(3) is to
replace the pointer in the environ[] array (not overwrite the
existing string in environ[] that holds the "VAR=VAL" string that
represents the current value, which might be shorter than the new
value of the enviornment variable), hence even calling these
functions is unlikely to invalidate the result you previously
received from getenv(3).
I am not at all surprised that nobody from other platforms has seen
this breakage.
In fact,
http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html
says that only setenv(), unsetenv() and putenv() may invalidate
previous return values. Note that getenv() is not listed as a
function that is allowed to break return values from a previous call
to getenv().
So in short, your platform's getenv(3) emulation may be broken. We
have other calls to getenv() where we rely on the result of it being
stable, and you might discover these places also break.
Having said that, we do have codepaths to update a handful of
environment variables ourselves (GIT_DIR is among them), so I think
your patch is a good safety measure in general.
> setup.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index f108c4b..64fb160 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -675,8 +675,12 @@ static const char
> *setup_git_directory_gently_1(int *nongit_ok)
> * validation.
> */
> gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
> - if (gitdirenv)
> - return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
> + if (gitdirenv) {
> + gitdirenv = xstrdup(gitdirenv);
> + ret = setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
> + free(gitdirenv);
> + return ret;
> + }
>
> if (env_ceiling_dirs) {
> string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
> --
> 1.7.11.7
^ permalink raw reply
* Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites
From: David Michael @ 2013-01-05 2:15 UTC (permalink / raw)
To: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <7vsj6gqvhc.fsf@alter.siamese.dyndns.org>
Hi,
On Fri, Jan 4, 2013 at 8:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> In fact,
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html
>
> says that only setenv(), unsetenv() and putenv() may invalidate
> previous return values. Note that getenv() is not listed as a
> function that is allowed to break return values from a previous call
> to getenv().
Before I sent the e-mail, I checked that very page to be sure I wasn't
entirely insane. Specifically, the second paragraph begins with:
> The string pointed to may be overwritten by a subsequent call to getenv(), [...]
I read that line as confirmation that this is indeed acceptably
standard behavior. Even the getenv man page on my Fedora workstation
says:
> The string pointed to by the return value of getenv() may be statically allocated, and can be modified by a subsequent call to getenv(), putenv(3), setenv(3), or unsetenv(3).
Am I misinterpreting these statements?
Thanks.
David
^ permalink raw reply
* Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites
From: Duy Nguyen @ 2013-01-05 2:45 UTC (permalink / raw)
To: David Michael; +Cc: git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CAEvUa7niTJVfp8_kuWs50kvhfZ59F-yAuAmeOXEduHXOq-tRFA@mail.gmail.com>
On Sat, Jan 5, 2013 at 7:35 AM, David Michael <fedora.dm0@gmail.com> wrote:
> - if (gitdirenv)
> - return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
> + if (gitdirenv) {
> + gitdirenv = xstrdup(gitdirenv);
> + ret = setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
> + free(gitdirenv);
> + return ret;
> + }
Maybe we could all this into a wrapper? If getenv() here has a
problem, many other places may have the same problem too. This
simplifies the change. But one has to check that getenv() must not be
used in threaded code.
char *git_getenv(const char *env)
{
static int bufno;
static char *buf[4];
bufno = (bufno + 1) % 4;
free(buf[bufno]);
buf[bufno] = xstrdup(getenv(env));
return buf[bufno];
}
#define getenv(x) git_getenv(x)
--
Duy
^ permalink raw reply
* Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites
From: Junio C Hamano @ 2013-01-05 4:32 UTC (permalink / raw)
To: David Michael; +Cc: git@vger.kernel.org
In-Reply-To: <7vsj6gqvhc.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> ... So even if the standard allowed the
> returned value to be volatile across calls to getenv(3),...
> ...
> In fact,
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html
>
> says that only ...
Apparently I wasn't even reading what I was quoting carefully
enough. The above does include getenv() as one of the functions
that are allowed to invalidate earlier return values.
Sorry about that. I'll go back to bed (I am a bit under the weather
and OOO today). The conclusion in my original message is still
valid.
> Having said that, we do have codepaths to update a handful of
> environment variables ourselves (GIT_DIR is among them), so I think
> your patch is a good safety measure in general.
^ permalink raw reply
* Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites
From: Junio C Hamano @ 2013-01-05 4:38 UTC (permalink / raw)
To: Duy Nguyen; +Cc: David Michael, git@vger.kernel.org
In-Reply-To: <CACsJy8BeuV8esGTWsQiT_G9pZE28s5KJxH6+dzdhioLgmSiNVg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> Maybe we could all this into a wrapper? If getenv() here has a
> problem, many other places may have the same problem too. This
> simplifies the change. But one has to check that getenv() must not be
> used in threaded code.
That needs to be done regardless, if we care; POSIX explicitly says
getenv() need not be thread-safe.
I personally do not think a wrapper with limited slots is a healthy
direction to go. Most places we use getenv() do not let the return
value live across their scope, and those that do should explicitly
copy the value away. It's between validating that there is _no_ *env()
calls in the codepath between a getenv() call and the use of its
return value, and validating that there is at most 4 such calls there.
The former is much easier to verify and maintain, I think.
^ permalink raw reply
* Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites
From: Duy Nguyen @ 2013-01-05 6:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Michael, git@vger.kernel.org
In-Reply-To: <7vk3rsqm6u.fsf@alter.siamese.dyndns.org>
On Sat, Jan 5, 2013 at 11:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I personally do not think a wrapper with limited slots is a healthy
> direction to go. Most places we use getenv() do not let the return
> value live across their scope, and those that do should explicitly
> copy the value away. It's between validating that there is _no_ *env()
> calls in the codepath between a getenv() call and the use of its
> return value, and validating that there is at most 4 such calls there.
> The former is much easier to verify and maintain, I think.
I did not look carefully and was scared of 143 getenv calls. But with
about 4 calls, yes it's best to do without the wrapper.
--
Duy
^ permalink raw reply
* Re: [PATCH v4] git-completion.bash: add support for path completion
From: Junio C Hamano @ 2013-01-05 6:27 UTC (permalink / raw)
To: Marc Khouzam, Manlio Perillo; +Cc: git, szeder, felipe.contreras
In-Reply-To: <7vobh4sffw.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Marc Khouzam <marc.khouzam@ericsson.com> writes:
>
>> I've been playing with it but I'm not getting the expected
>> behavior when I cd to a sub-directory.
>
> Thanks for testing. Manlio?
Can you try the attached patch?
As I am not familiar with the completion machinery, take this with a
large grain of salt. Here is my explanation of what is going on in
this "how about this" fixup:
* Giving --git-dir from the command line (or GIT_DIR environment)
without specifying GIT_WORK_TREE is to signal Git that you are at
the top of the working tree. "git ls-files" will then show the
full tree even outside the real $cwd because you are lying to
Git.
* "git diff-index" could be told to show only the $cwd and its
subdirectory with the "--relative" option, but it alone is not
sufficient if you throw --git-dir at it; again, you end up lying
that you are at the top.
* As far as I can tell, there is no reason you would want to pass
"--git-dir" to these invocations of ls-files and diff-index. If
the previous call to "__gitdir" could figure out where it is, the
command should be able to figure it out the same way.
There seem to be millions of other existing "git --git-dir=$there"
in this script. As I already said, I am not familiar with the
completion machinery, and I do not know what they are for in the
first place. Perhaps people put them there for a reason, but I do
not know what that reason is.
I think the ones for "for-each-ref", "config" and "stash" should be
harmless. They are commands that do not care about the working
tree.
There is one given to "ls-tree" used in __git_complete_revlist_file;
I do not know if this was intended, what it is for, and if that is
working as intended, though.
I've been CC'ing two people who touched this script rather heavily,
are expected to know the completion machinery, and should be able to
help polishing this topic and moving it forward. Perhaps one of
them can shed light on this.
-- >8 --
Subject: completion: do not pass harmful "--git-dir=$there"
The recently added __git_index_files and __git_diff_index_files
helper functions invoke "ls-files" and "diff_index" while explicitly
passing "--git-dir=$there", to tell them that the invocation is done
at the top of the working tree, which may not be the case when the
user is in a subdirectory. Remove the harmful use of this option,
Tell "diff-index" to show only the paths in the $cwd and show them
relative to the $cwd by passing "--relative". The "ls-files" does
not need this, as that is already its default mode of operation.
---
contrib/completion/git-completion.bash | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c8c6464..f4bd548 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -267,9 +267,9 @@ __git_index_files ()
if [ -d "$dir" ]; then
# NOTE: $1 is not quoted in order to support multiple options
- git --git-dir="$dir" ls-files --exclude-standard $1 ${2+"$2"} 2>/dev/null |
- __git_index_file_list_filter ${2+"$2"} |
- uniq
+ git ls-files --exclude-standard $1 ${2+"$2"} 2>/dev/null |
+ __git_index_file_list_filter ${2+"$2"} |
+ uniq
fi
}
@@ -284,9 +284,9 @@ __git_diff_index_files ()
local dir="$(__gitdir)"
if [ -d "$dir" ]; then
- git --git-dir="$dir" diff-index --name-only "$1" 2>/dev/null |
- __git_index_file_list_filter ${2+"$2"} |
- uniq
+ git diff-index --name-only --relative "$1" 2>/dev/null |
+ __git_index_file_list_filter ${2+"$2"} |
+ uniq
fi
}
^ permalink raw reply related
* Re: [BUG/PATCH] setup: Copy an environment variable to avoid overwrites
From: Junio C Hamano @ 2013-01-05 6:47 UTC (permalink / raw)
To: Duy Nguyen; +Cc: David Michael, git@vger.kernel.org
In-Reply-To: <CACsJy8CZe=qyzmG_1vdLYp07OvkDAU4wYc8MN3et7WBVmMhJOQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Sat, Jan 5, 2013 at 11:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I personally do not think a wrapper with limited slots is a healthy
>> direction to go. Most places we use getenv() do not let the return
>> value live across their scope, and those that do should explicitly
>> copy the value away. It's between validating that there is _no_ *env()
>> calls in the codepath between a getenv() call and the use of its
>> return value, and validating that there is at most 4 such calls there.
>> The former is much easier to verify and maintain, I think.
>
> I did not look carefully and was scared of 143 getenv calls. But with
> about 4 calls, yes it's best to do without the wrapper.
Just to make sure you did not misunderstand, the 4 (four) in my
message is not about "4 calls among 143 are unsafe".
It was referring to the number of rotating slots your patch defined,
which means
val = getenv("FOO");
... random other code ...
use(val)
is safe only if random other code makes less than 4 getenv() calls.
I didn't verify all of the call sites. It needs to be done with or
without your wrapper patch. Without your wrapper, the validation
needs to make sure "random other code" above does not make any getenv()
call. With your wrapper, the validation needs to make sure "random
other code" above does not make more than 4 such calls. My point
was that the effort needed for both are about the same, so your
wrapper does not buy us much.
^ permalink raw reply
* [PATCH 00/10] push: switch default from "matching" to "simple"
From: Junio C Hamano @ 2013-01-05 6:52 UTC (permalink / raw)
To: git
We promised to change the behaviour of lazy "git push [there]" that
does not say what to push on the command line from "matching" to
"simple" in Git 2.0. Even though the top-level RelNotes symbolic
link points at 1.8.2, we can change our mind to tag 2.0 at the end
of this cycle.
This carries out the threat ;-)
Mmany tests have assumed that the long established default will
never change and relied on the convenience of the "matching"
behaviour. They need to be updated to either specify what they want
from the command line explicitly, or configure the default behaviour
they would expect. And then finally we can flip the default.
Junio C Hamano (10):
t5404: do not assume the "matching" push is the default
t5505: do not assume the "matching" push is the default
t5516: do not assume the "matching" push is the default
t5517: do not assume the "matching" push is the default
t5519: do not assume the "matching" push is the default
t5531: do not assume the "matching" push is the default
t7406: do not assume the "matching" push is the default
t9400: do not assume the "matching" push is the default
t9401: do not assume the "matching" push is the default
push: switch default from "matching" to "simple"
Documentation/config.txt | 6 +++---
builtin/push.c | 31 +++++++------------------------
t/t5404-tracking-branches.sh | 2 +-
t/t5505-remote.sh | 2 +-
t/t5516-fetch-push.sh | 10 +++++-----
t/t5517-push-mirror.sh | 2 +-
t/t5519-push-alternates.sh | 12 ++++++------
t/t5531-deep-submodule-push.sh | 1 +
t/t7406-submodule-update.sh | 4 ++--
t/t9400-git-cvsserver-server.sh | 1 +
t/t9401-git-cvsserver-crlf.sh | 1 +
11 files changed, 29 insertions(+), 43 deletions(-)
--
1.8.1.299.gc73b41f
^ permalink raw reply
* [PATCH 01/10] t5404: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:52 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t5404-tracking-branches.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index c240035..2b8c0ba 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -36,7 +36,7 @@ test_expect_success 'prepare pushable branches' '
'
test_expect_success 'mixed-success push returns error' '
- test_must_fail git push
+ test_must_fail git push origin :
'
test_expect_success 'check tracking branches updated correctly after push' '
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 02/10] t5505: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t5505-remote.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index ccc55eb..6579a86 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -345,7 +345,7 @@ test_expect_success 'fetch mirrors do not act as mirrors during push' '
) &&
(cd mirror-fetch/child &&
git branch -m renamed renamed2 &&
- git push parent
+ git push parent :
) &&
(cd mirror-fetch/parent &&
git rev-parse --verify renamed &&
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 04/10] t5517: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t5517-push-mirror.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index e2ad260..12a5dfb 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -256,7 +256,7 @@ test_expect_success 'remote.foo.mirror=no has no effect' '
git branch keep master &&
git push --mirror up &&
git branch -D keep &&
- git push up
+ git push up :
) &&
(
cd mirror &&
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 03/10] t5516: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t5516-fetch-push.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..1a8753d 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -247,7 +247,7 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
test_expect_success 'push with matching heads' '
mk_test heads/master &&
- git push testrepo &&
+ git push testrepo : &&
check_push_result $the_commit heads/master
'
@@ -276,7 +276,7 @@ test_expect_success 'push --force with matching heads' '
mk_test heads/master &&
git push testrepo : &&
git commit --amend -massaged &&
- git push --force testrepo &&
+ git push --force testrepo : &&
! check_push_result $the_commit heads/master &&
git reset --hard $the_commit
@@ -507,7 +507,7 @@ test_expect_success 'push with config remote.*.pushurl' '
git checkout master &&
git config remote.there.url test2repo &&
git config remote.there.pushurl testrepo &&
- git push there &&
+ git push there : &&
check_push_result $the_commit heads/master
'
@@ -521,7 +521,7 @@ test_expect_success 'push with dry-run' '
cd testrepo &&
old_commit=$(git show-ref -s --verify refs/heads/master)
) &&
- git push --dry-run testrepo &&
+ git push --dry-run testrepo : &&
check_push_result $old_commit heads/master
'
@@ -981,7 +981,7 @@ test_expect_success 'push --porcelain --dry-run rejected' '
test_expect_success 'push --prune' '
mk_test heads/master heads/second heads/foo heads/bar &&
- git push --prune testrepo &&
+ git push --prune testrepo : &&
check_push_result $the_commit heads/master &&
check_push_result $the_first_commit heads/second &&
! check_push_result $the_first_commit heads/foo heads/bar
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 05/10] t5519: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t5519-push-alternates.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh
index c00c9b0..11fcd37 100755
--- a/t/t5519-push-alternates.sh
+++ b/t/t5519-push-alternates.sh
@@ -40,7 +40,7 @@ test_expect_success 'alice works and pushes' '
cd alice-work &&
echo more >file &&
git commit -a -m second &&
- git push ../alice-pub
+ git push ../alice-pub :
)
'
@@ -57,7 +57,7 @@ test_expect_success 'bob fetches from alice, works and pushes' '
git pull ../alice-pub master &&
echo more bob >file &&
git commit -a -m third &&
- git push ../bob-pub
+ git push ../bob-pub :
) &&
# Check that the second commit by Alice is not sent
@@ -86,7 +86,7 @@ test_expect_success 'alice works and pushes again' '
cd alice-work &&
echo more alice >file &&
git commit -a -m fourth &&
- git push ../alice-pub
+ git push ../alice-pub :
)
'
@@ -99,7 +99,7 @@ test_expect_success 'bob works and pushes' '
cd bob-work &&
echo yet more bob >file &&
git commit -a -m fifth &&
- git push ../bob-pub
+ git push ../bob-pub :
)
'
@@ -115,7 +115,7 @@ test_expect_success 'alice works and pushes yet again' '
git commit -a -m sixth.2 &&
echo more and more alice >>file &&
git commit -a -m sixth.3 &&
- git push ../alice-pub
+ git push ../alice-pub :
)
'
@@ -136,7 +136,7 @@ test_expect_success 'bob works and pushes again' '
git hash-object -t commit -w commit &&
echo even more bob >file &&
git commit -a -m seventh &&
- git push ../bob-pub
+ git push ../bob-pub :
)
'
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 06/10] t5531: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t5531-deep-submodule-push.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 1947c28..8c16e04 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -16,6 +16,7 @@ test_expect_success setup '
(
cd gar/bage &&
git init &&
+ git config push.default matching &&
>junk &&
git add junk &&
git commit -m "Initial junk"
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 07/10] t7406: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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 feaec6c..c675ce6 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -565,14 +565,14 @@ test_expect_success 'submodule add places git-dir in superprojects git-dir recur
git log > ../../../expected
) &&
git commit -m "added subsubmodule" &&
- git push
+ git push origin :
) &&
(cd .git/modules/deeper/submodule/modules/subsubmodule &&
git log > ../../../../../actual
) &&
git add deeper/submodule &&
git commit -m "update submodule" &&
- git push &&
+ git push origin : &&
test_cmp actual expected
)
'
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 09/10] t9401: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t9401-git-cvsserver-crlf.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index 1c5bc84..8c3db76 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh
@@ -84,6 +84,7 @@ export CVSROOT CVS_SERVER
rm -rf "$CVSWORK" "$SERVERDIR"
test_expect_success 'setup' '
+ git config push.default matching &&
echo "Simple text file" >textfile.c &&
echo "File with embedded NUL: Q <- there" | q_to_nul > binfile.bin &&
mkdir subdir &&
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 10/10] push: switch default from "matching" to "simple"
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
We promised to change the behaviour of lazy "git push [there]" that
does not say what to push on the command line from "matching" to
"simple" in Git 2.0.
This finally flips that bit.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/config.txt | 6 +++---
builtin/push.c | 31 +++++++------------------------
2 files changed, 10 insertions(+), 27 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index bf8f911..770eefe 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1750,15 +1750,15 @@ push.default::
since locally stalled branches will attempt a non-fast forward push
if other users updated the branch.
+
- This is currently the default, but Git 2.0 will change the default
- to `simple`.
+ This used to be the default, and stale web sites may still say so,
+ but Git 2.0 has changed the default to `simple`.
* `upstream` - push the current branch to its upstream branch.
With this, `git push` will update the same remote ref as the one which
is merged by `git pull`, making `push` and `pull` symmetrical.
See "branch.<name>.merge" for how to configure the upstream branch.
* `simple` - like `upstream`, but refuses to push if the upstream
branch's name is different from the local one. This is the safest
- option and is well-suited for beginners. It will become the default
+ option and is well-suited for beginners. It has become the default
in Git 2.0.
* `current` - push the current branch to a branch of the same name.
--
diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..9f7c252 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -24,7 +24,6 @@ static int progress = -1;
static const char **refspec;
static int refspec_nr;
static int refspec_alloc;
-static int default_matching_used;
static void add_refspec(const char *ref)
{
@@ -148,9 +147,9 @@ static void setup_push_upstream(struct remote *remote, int simple)
}
static char warn_unspecified_push_default_msg[] =
-N_("push.default is unset; its implicit value is changing in\n"
+N_("push.default is unset; its implicit value has changed in\n"
"Git 2.0 from 'matching' to 'simple'. To squelch this message\n"
- "and maintain the current behavior after the default changes, use:\n"
+ "and maintain the traditional behavior, use:\n"
"\n"
" git config --global push.default matching\n"
"\n"
@@ -175,14 +174,14 @@ static void setup_default_push_refspecs(struct remote *remote)
{
switch (push_default) {
default:
- case PUSH_DEFAULT_UNSPECIFIED:
- default_matching_used = 1;
- warn_unspecified_push_default_configuration();
- /* fallthru */
case PUSH_DEFAULT_MATCHING:
add_refspec(":");
break;
+ case PUSH_DEFAULT_UNSPECIFIED:
+ warn_unspecified_push_default_configuration();
+ /* fallthru */
+
case PUSH_DEFAULT_SIMPLE:
setup_push_upstream(remote, 1);
break;
@@ -208,12 +207,6 @@ static const char message_advice_pull_before_push[] =
"before pushing again.\n"
"See the 'Note about fast-forwards' in 'git push --help' for details.");
-static const char message_advice_use_upstream[] =
- N_("Updates were rejected because a pushed branch tip is behind its remote\n"
- "counterpart. If you did not intend to push that branch, you may want to\n"
- "specify branches to push or set the 'push.default' configuration variable\n"
- "to 'simple', 'current' or 'upstream' to push only the current branch.");
-
static const char message_advice_checkout_pull_push[] =
N_("Updates were rejected because a pushed branch tip is behind its remote\n"
"counterpart. Check out this branch and merge the remote changes\n"
@@ -227,13 +220,6 @@ static void advise_pull_before_push(void)
advise(_(message_advice_pull_before_push));
}
-static void advise_use_upstream(void)
-{
- if (!advice_push_non_ff_default || !advice_push_nonfastforward)
- return;
- advise(_(message_advice_use_upstream));
-}
-
static void advise_checkout_pull_push(void)
{
if (!advice_push_non_ff_matching || !advice_push_nonfastforward)
@@ -272,10 +258,7 @@ static int push_with_options(struct transport *transport, int flags)
advise_pull_before_push();
break;
case NON_FF_OTHER:
- if (default_matching_used)
- advise_use_upstream();
- else
- advise_checkout_pull_push();
+ advise_checkout_pull_push();
break;
}
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* [PATCH 08/10] t9400: do not assume the "matching" push is the default
From: Junio C Hamano @ 2013-01-05 6:53 UTC (permalink / raw)
To: git
In-Reply-To: <1357368788-28035-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t9400-git-cvsserver-server.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index 9502f24..0431386 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -36,6 +36,7 @@ export CVSROOT CVS_SERVER
rm -rf "$CVSWORK" "$SERVERDIR"
test_expect_success 'setup' '
+ git config push.default matching &&
echo >empty &&
git add empty &&
git commit -q -m "First Commit" &&
--
1.8.1.299.gc73b41f
^ permalink raw reply related
* Re: [PATCH v3 11/19] dir.c: use a single struct exclude_list per source of excludes
From: Junio C Hamano @ 2013-01-05 7:54 UTC (permalink / raw)
To: Adam Spiers; +Cc: git list
In-Reply-To: <7v1ue0veww.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Adam Spiers <git@adamspiers.org> writes:
>
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index 0c7b3d0..bd18b88 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -97,9 +97,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>> if (!ignored)
>> setup_standard_excludes(&dir);
>>
>> + add_exclude_list(&dir, EXC_CMDL);
>> for (i = 0; i < exclude_list.nr; i++)
>> add_exclude(exclude_list.items[i].string, "", 0,
>> - &dir.exclude_list[EXC_CMDL]);
>> + &dir.exclude_list_groups[EXC_CMDL].ary[0]);
>
> This looks somewhat ugly for two reasons.
>
> * The abstraction add_exclude() offers to its callers is just to
> let them add one pattern to the list of patterns for the kind
> (here, EXC_CMDL); why should they care about .ary[0] part? Are
> there cases any sane caller (not the implementation of the
> exclude_list_group machinery e.g. add_excludes_from_... function)
> may want to call it with .ary[1]? I do not think of any.
> Shouldn't the public API function add_exclude() take a pointer to
> the list group itself?
>
> * When naming an array of things, we tend to prefer naming it
>
> type thing[count]
>
> so that the second element can be called "thing[2]" and not
> "things[2]". dir.exclude_list_group[EXC_CMDL] reads better.
Also, "ary[]" is a bad name, even as an implementation detail, for
two reasons: it is naming it after its type (being an "array") not
after what it is (if it holds the patterns from the same information
source, e.g. file, togeter, "src" might be a better name), and it
uses rather unusual abbreviation (I haven't seen "array" shortened
to "ary" anywhere else).
>
>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
>> index ef7f99a..c448e06 100644
>> --- a/builtin/ls-files.c
>> +++ b/builtin/ls-files.c
>> @@ -420,10 +420,10 @@ static int option_parse_z(const struct option *opt,
>> static int option_parse_exclude(const struct option *opt,
>> const char *arg, int unset)
>> {
>> - struct exclude_list *list = opt->value;
>> + struct exclude_list_group *group = opt->value;
>>
>> exc_given = 1;
>> - add_exclude(arg, "", 0, list);
>> + add_exclude(arg, "", 0, &group->ary[0]);
>
> This is another example where the caller would wish to be able to say
>
> add_exclude(arg, "", 0, group);
>
> instead.
^ permalink raw reply
* Re: cvsps, parsecvs, svn2git and the CVS exporter mess
From: Max Horn @ 2013-01-05 8:27 UTC (permalink / raw)
To: esr
Cc: Michael Haggerty, Yann Dirson, Heiko Voigt, Antoine Pelisse,
Bart Massey, Keith Packard, David Mansfield, git
In-Reply-To: <20130103205301.GD26201@thyrsus.com>
On 03.01.2013, at 21:53, Eric S. Raymond wrote:
> Michael Haggerty <mhagger@alum.mit.edu>:
>> There are two good reasons that the output is written to two separate files:
>
> Those are good reasons to write to a pair of tempfiles, and I was able
> to deduce in advance most of what your explanation would be from the
> bare fact that you did it that way.
>
> They are *not* good reasons for having an interface that exposes this
> implementation detail to the caller - that choice I consider a failure
> of interface-design judgment. But I know how to fix this in a simple and
> backward-compatible way, and will do so when I have time to write you
> a patch. Next week or the week after, most likely.
>
> Also, the cvs2git manual page is still rather half-baked and careless,
> with several fossil references to cvs2svn that shouldn't be there and
> obviously incomplete feature coverage. Fixing these bugs is also on my
> to-do list for sometime this month.
>
> I'd be willing to put in this work anyway, but it still in the back of
> my mind that if cvs2git wins the test-suite competition I might
> officially end-of-life both cvsps and parsecvs. One of the features
> of the new git-cvsimport is direct support for using cvs2git as a
> conversion engine.
>
>> A potentially bigger problem is that if you want to handle such
>> blob/dump output, you have to deal with git-fast-import format's "blob"
>> command as opposed to only handling inline blobs.
>
> Not a problem. All of the main potential consumers for this output,
> including reposurgeon, handle the blob command just fine.
Hm, you snipped this part of Michael's mail:
>> However, if that is a
>> problem, it is possible to configure cvs2git to write the blobs inline
>> with the rest of the dumpfile (this mode is supported because "hg
>> fast-import" doesn't support detached blobs).
I would call "hg fast-import" a main potential customer, given that there "cvs2hg" is another part of the cvs2svn suite. So I can't quite see how you can come to your conclusion above...
Cheers,
Max
^ permalink raw reply
* [PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD
From: Nguyễn Thái Ngọc Duy @ 2013-01-05 8:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Michael,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CAEvUa7niTJVfp8_kuWs50kvhfZ59F-yAuAmeOXEduHXOq-tRFA@mail.gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Perhaps this will help the getenv bug hunting (I assume we do the
hunting on Linux platform only). So far it catches this and is stuck
at getenv in git_pager().
diff --git a/exec_cmd.c b/exec_cmd.c
index 125fa6f..d8be5ce 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -97,7 +97,7 @@ static void add_path(struct strbuf *out, const char *path)
void setup_path(void)
{
- const char *old_path = getenv("PATH");
+ char *old_path = xstrdup(getenv("PATH"));
struct strbuf new_path = STRBUF_INIT;
add_path(&new_path, git_exec_path());
@@ -110,6 +110,7 @@ void setup_path(void)
setenv("PATH", new_path.buf, 1);
+ free(old_path);
strbuf_release(&new_path);
}
contrib/getenv/Makefile | 2 ++
contrib/getenv/getenv.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+)
create mode 100644 contrib/getenv/Makefile
create mode 100644 contrib/getenv/getenv.c
diff --git a/contrib/getenv/Makefile b/contrib/getenv/Makefile
new file mode 100644
index 0000000..4881b85
--- /dev/null
+++ b/contrib/getenv/Makefile
@@ -0,0 +1,2 @@
+getenv.so: getenv.c
+ $(CC) -g -shared -fPIC -ldl -o $@ $<
diff --git a/contrib/getenv/getenv.c b/contrib/getenv/getenv.c
new file mode 100644
index 0000000..e351e10
--- /dev/null
+++ b/contrib/getenv/getenv.c
@@ -0,0 +1,67 @@
+#include <gnu/lib-names.h>
+#include <sys/mman.h>
+#include <dlfcn.h>
+#include <execinfo.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdio.h>
+
+/* Global symbols for easy access from gdb */
+static char *getenv_current;
+static char *getenv_prev;
+
+/*
+ * Intercept standard getenv() via LD_PRELOAD. The return value is
+ * made inaccessible by the next getenv() call. This helps catch
+ * places that ignore the statement "The string pointed to may be
+ * overwritten by a subsequent call to getenv()" [1].
+ *
+ * The backtrace is appended after the env string, which may be
+ * helpful to identify where this getenv() is called in a core dump.
+ *
+ * [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getenv.html
+ */
+char *getenv(const char *name)
+{
+ static char *(*libc_getenv)(const char*);
+ char *value;
+
+ if (!libc_getenv) {
+ void *libc = dlopen(LIBC_SO, RTLD_LAZY);
+ libc_getenv = dlsym(libc, "getenv");
+ }
+ if (getenv_current) {
+ mprotect(getenv_current, strlen(getenv_current) + 1, PROT_NONE);
+ getenv_prev = getenv_current;
+ getenv_current = NULL;
+ }
+
+ value = libc_getenv(name);
+ if (value) {
+ int len = strlen(value) + 1;
+ int backtrace_len = 0;
+ void *buffer[100];
+ char **symbols;
+ int i, n;
+
+ n = backtrace(buffer, 100);
+ symbols = backtrace_symbols(buffer, n);
+ if (symbols) {
+ for (i = 0;i < n; i++)
+ backtrace_len += strlen(symbols[i]) + 1; /* \n */
+ backtrace_len++; /* NULL */
+ }
+
+ getenv_current = mmap(NULL, len + backtrace_len, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+ memcpy(getenv_current, value, len);
+ value = getenv_current;
+
+ if (symbols) {
+ char *p = getenv_current + len;
+ for (i = 0; i < n; i++)
+ p += sprintf(p, "%s\n", symbols[i]);
+ }
+ }
+ return value;
+}
--
1.8.0.rc2.23.g1fb49df
^ permalink raw reply related
* Re: [PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD
From: Matt Kraai @ 2013-01-05 10:39 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, David Michael
In-Reply-To: <1357376146-7155-1-git-send-email-pclouds@gmail.com>
On Sat, Jan 05, 2013 at 03:55:46PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Perhaps this will help the getenv bug hunting (I assume we do the
> hunting on Linux platform only). So far it catches this and is stuck
> at getenv in git_pager().
It seems like a static analysis tool might be able to detect these
problems. Is there a way to do so using sparse?
> + n = backtrace(buffer, 100);
> + symbols = backtrace_symbols(buffer, n);
> + if (symbols) {
> + for (i = 0;i < n; i++)
s/;i/; i/
--
Matt Kraai
https://ftbfs.org/kraai
^ permalink raw reply
* t7061: comments and one failure
From: Torsten Bögershausen @ 2013-01-05 11:07 UTC (permalink / raw)
To: apelisse, Git Mailing List; +Cc: Torsten Bögershausen
Hej,
TC 9 is failing (Mac OS X 10.6),
==========================
expecting success:
>tracked/uncommitted &&
git status --porcelain --ignored >actual &&
test_cmp expected actual
--- expected 2013-01-05 11:01:00.000000000 +0000
+++ actual 2013-01-05 11:01:00.000000000 +0000
@@ -1,4 +1,3 @@
?? .gitignore
?? actual
?? expected
-!! tracked/
not ok 9 - status ignored tracked directory and uncommitted file with --ignore
#
# >tracked/uncommitted &&
# git status --porcelain --ignored >actual &&
# test_cmp expected actual
#
=======================
I haven't been able to dig further into this,
(I can volonteer to do some more debugging).
Looking into the code, there are 2 questions:
1) echo "ignored" >.gitignore &&
We don't need the quoting of a simple string which does not have space in it.
2) : >untracked/ignored &&
Do we need the colon here?
Would it make sence to do the following:
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 0da1214..761a2e7 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -12,10 +12,10 @@ cat >expected <<\EOF
EOF
test_expect_success 'status untracked directory with --ignored' '
- echo "ignored" >.gitignore &&
+ echo ignored >.gitignore &&
mkdir untracked &&
- : >untracked/ignored &&
- : >untracked/uncommitted &&
+ >untracked/ignored &&
+ >untracked/uncommitted &&
git status --porcelain --ignored >actual &&
test_cmp expected actual
'
@@ -43,7 +43,7 @@ EOF
test_expect_success 'status ignored directory with --ignore' '
rm -rf untracked &&
mkdir ignored &&
- : >ignored/uncommitted &&
+ >ignored/uncommitted &&
git status --porcelain --ignored >actual &&
test_cmp expected actual
'
@@ -71,8 +71,8 @@ test_expect_success 'status untracked directory with ignored files with --ignore
rm -rf ignored &&
mkdir untracked-ignored &&
mkdir untracked-ignored/test &&
- : >untracked-ignored/ignored &&
- : >untracked-ignored/test/ignored &&
+ >untracked-ignored/ignored &&
+ >untracked-ignored/test/ignored &&
git status --porcelain --ignored >actual &&
test_cmp expected actual
'
@@ -99,10 +99,10 @@ EOF
test_expect_success 'status ignored tracked directory with --ignore' '
rm -rf untracked-ignored &&
mkdir tracked &&
- : >tracked/committed &&
+ >tracked/committed &&
git add tracked/committed &&
git commit -m. &&
- echo "tracked" >.gitignore &&
+ echo tracked >.gitignore &&
git status --porcelain --ignored >actual &&
test_cmp expected actual
'
@@ -126,7 +126,7 @@ cat >expected <<\EOF
EOF
test_expect_success 'status ignored tracked directory and uncommitted file with --ignore' '
- : >tracked/uncommitted &&
+ >tracked/uncommitted &&
git status --porcelain --ignored >actual &&
test_cmp expected actual
'
/Torsten
^ permalink raw reply related
* Re: t7061: comments and one failure
From: Jeff King @ 2013-01-05 11:24 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: apelisse, Git Mailing List
In-Reply-To: <50E8096C.7000501@web.de>
On Sat, Jan 05, 2013 at 12:07:24PM +0100, Torsten Bögershausen wrote:
> TC 9 is failing (Mac OS X 10.6),
Yeah, I can reproduce the problem here on my OS X test box. It seems to
be related to core.ignorecase. If you put
git config core.ignorecase false
at the top of t7061, it passes on OS X. Likewise, if you set it to true,
it will start failing on Linux.
So it looks like a real bug, not a test-portability issue.
> Looking into the code, there are 2 questions:
>
> 1) echo "ignored" >.gitignore &&
> We don't need the quoting of a simple string which does not have space in it.
No, but it does not hurt anything.
> 2) : >untracked/ignored &&
> Do we need the colon here?
No, but it does not hurt anything.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox