* Re: [PATCH v2] test-lib.sh: unfilter GIT_PERF_*
From: Junio C Hamano @ 2013-01-15 19:31 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Thomas Rast
In-Reply-To: <1358257856-3274-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> These variables are user parameters to control how to run the perf
> tests. Allow users to do so.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
I think the Subject makes more sense, so I'd suggest replacing the
current one with "PERF_", not with "PERF".
> t/test-lib.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index f50f834..e1c8c85 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -85,7 +85,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
> .*_TEST
> PROVE
> VALGRIND
> - PERF_AGGREGATING_LATER
> + PERF
> ));
> my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
> print join("\n", @vars);
^ permalink raw reply
* [RFC/PATCH 2/8 v2] git_remote_helpers: fix input when running under Python 3
From: John Keeping @ 2013-01-15 19:48 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Eric S. Raymond, Felipe Contreras, Sverre Rabbelier
In-Reply-To: <20130114094721.GQ4574@serenity.lan>
Although 2to3 will fix most issues in Python 2 code to make it run under
Python 3, it does not handle the new strict separation between byte
strings and unicode strings. There is one instance in
git_remote_helpers where we are caught by this, which is when reading
refs from "git for-each-ref".
While we could fix this by explicitly handling refs as byte strings,
this is merely punting the problem to users of the library since the
same problem will be encountered as soon you want to display the ref
name to a user.
Instead of doing this, explicit decode the incoming byte string into a
unicode string. Following the lead of pygit2 (the Python bindings for
libgit2 - see [1] and [2]), use the filesystem encoding by default,
providing a way for callers to override this if necessary.
[1] https://github.com/libgit2/pygit2/blob/e34911b63e5d2266f9f72a4e3f32e27b13190feb/src/pygit2/reference.c#L261
[2] https://github.com/libgit2/pygit2/blob/e34911b63e5d2266f9f72a4e3f32e27b13190feb/include/pygit2/utils.h#L55
Signed-off-by: John Keeping <john@keeping.me.uk>
---
I think this is in fact the best way to handle this, and I hope the
above description clarified why I don't think we want to treat refs as
byte strings in Python 3.
My only remaining question is whether it would be better to set the
error mode when decoding to "replace" instead of "strict" (the default).
"strict" will cause a UnicodeError if the string cannot be decoded
whereas "replace" will use U+FFFD (the replacement character). [3]
I think it's better to use "strict" and let the user know that
something has gone wrong rather than silently change the string, but I'd
welcome other opinions.
[3] http://docs.python.org/2/library/codecs.html#codec-base-classes
git_remote_helpers/git/importer.py | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
index e28cc8f..5bc16a4 100644
--- a/git_remote_helpers/git/importer.py
+++ b/git_remote_helpers/git/importer.py
@@ -1,5 +1,6 @@
import os
import subprocess
+import sys
from git_remote_helpers.util import check_call, check_output
@@ -10,17 +11,26 @@ class GitImporter(object):
This importer simply delegates to git fast-import.
"""
- def __init__(self, repo):
+ def __init__(self, repo, ref_encoding=None):
"""Creates a new importer for the specified repo.
+
+ If ref_encoding is specified that refs are decoded using that
+ encoding. Otherwise the system filesystem encoding is used.
"""
self.repo = repo
+ self.ref_encoding = ref_encoding
def get_refs(self, gitdir):
"""Returns a dictionary with refs.
"""
args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
- lines = check_output(args).strip().split('\n')
+ encoding = self.ref_encoding
+ if encoding is None:
+ encoding = sys.getfilesystemencoding()
+ if encoding is None:
+ encoding = sys.getdefaultencoding()
+ lines = check_output(args).decode(encoding).strip().split('\n')
refs = {}
for line in lines:
value, name = line.split(' ')
--
1.8.1
^ permalink raw reply related
* Re: [PATCH] attr: fix off-by-one directory component length calculation
From: Jean-Noël AVILA @ 2013-01-15 19:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwqve2qk3.fsf@alter.siamese.dyndns.org>
Le mardi 15 janvier 2013 20:29:16, Junio C Hamano a écrit :
> "Jean-Noël AVILA" <avila.jn@gmail.com> writes:
> > Thank you for the explanation.
> >
> > I did not monitor the system calls when writing that patch.
> > Where is the perf framework?
> >
> > As the mistake is located in the "find_basename" function, I would
> > propose a fix directly into it so that the output fits what the other
> > functions expect.
>
> Isn't that a crazy semantics for the function, though? I would
> expect find_basename("/a/path/to/file") to return "file", not
> "/file".
>
Yes, it is. I was wrong on the meaning of basename.
Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known
issue?
^ permalink raw reply
* Re: [PATCH] remote-hg: store converted URL
From: Junio C Hamano @ 2013-01-15 20:10 UTC (permalink / raw)
To: Max Horn; +Cc: git, Felipe Contreras
In-Reply-To: <B35B3EA6-F01B-46D8-AC3D-0F7C8A45A06B@quendi.de>
Max Horn <max@quendi.de> writes:
> So far, all I look at do not deal with this at all. Any attempts
> to deal with it should be pretty easy to recognize: The
> remote-$backend would have to store something into the git config,
> or else, verify the opaque token and refuse to work with it under
> certain conditions (e.g. when it contains a relative path). But
> they don't. E.g. git-remote-testgit has the exact same problem.
Thanks for confirming what I suspected. I think the way Felipe's
patch makes remote-hg take responsibility of how $opaqueToken should
look like for future invocations is the simplest and makes the most
sense. We could try to go fancier and end up over-engineering, but
I'd rather have a simple fix in the tree first.
Thanks.
^ permalink raw reply
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Torsten Bögershausen @ 2013-01-15 20:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Torsten Bögershausen, git, kraai
In-Reply-To: <7v4nikiu81.fsf@alter.siamese.dyndns.org>
On 13.01.13 23:38, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Hi,
>>
>> Torsten Bögershausen wrote:
>>
>>> - /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
>>> + /^\s*[^#]\s*which\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';
>>
>> Hmm. Neither the old version nor the new one matches what seem to
>> be typical uses of 'which', based on a quick code search:
>>
>> if which sl >/dev/null 2>&1
>> then
>> sl -l
>> ...
>> fi
>>
>> or
>>
>> if test -x "$(which sl 2>/dev/null)"
>> then
>> sl -l
>> ...
>> fi
>
> Yes, these two misuses are what we want it to trigger on, so the
> test is very easy to trigger and produce a false positive, but does
> not trigger on what we really want to catch.
>
> That does not sound like a good benefit/cost ratio to me.
>
Thanks for comments, I think writing a regexp for which is difficult.
What do we think about something like this for fishing for which:
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -644,6 +644,10 @@ yes () {
:
done
}
+which () {
+ echo >&2 "which is not portable (please use type)"
+ exit 1
+}
This will happen in runtime, which might be good enough ?
@Matt:
>The "[^#]" appears to ensure that there's at least one character
>before the which and that it's not a pound sign. Why is this done?
This is simply wrong.
^ permalink raw reply
* Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
From: Andreas Schwab @ 2013-01-15 20:27 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, git, Jeff King, Carlos Martín Nieto,
Johannes Schindelin
In-Reply-To: <50F57BDF.1050400@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> + return '\0' - ent->name[key->len];
You need to cast to unsigned char first to make it consistent with
memcmp and strcmp.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store
From: Jonathan Nieder @ 2013-01-15 20:32 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, Jeff King
In-Reply-To: <1358237193-8887-7-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty wrote:
> - else if ((arg1 = next_arg(&cmd))) {
> - if (!strcmp("EXISTS", arg1))
> - ctx->gen.count = atoi(arg);
> - else if (!strcmp("RECENT", arg1))
> - ctx->gen.recent = atoi(arg);
> + } else if ((arg1 = next_arg(&cmd))) {
> + /* unused */
The above is just the right thing to do to ensure no behavior change.
Let's take a look at the resulting code, though:
if (... various reasonable things ...) {
...
} else if ((arg1 = next_arg(&cmd))) {
/* unused */
} else {
fprintf(stderr, "IMAP error: unable to parse untagged response\n");
return RESP_BAD;
Anyone forced by some bug to examine this "/* unused */" case is going
to have no clue what's going on. In that respect, the old code was
much better, since it at least made it clear that one case where this
code gets hit is handling "<num> EXISTS" and "<num> RECENT" untagged
responses.
I suspect that original code did not have an implicit and intended
missing
else
; /* negligible response; ignore it */
but the intent was rather
else {
fprintf(stderr, "IMAP error: I can't parse this\n");
return RESP_BAD;
}
Since actually fixing that is probably too aggressive for this patch,
how about a FIXME comment like the following?
/*
* Unhandled response-data with at least two words.
* Ignore it.
*
* NEEDSWORK: Previously this case handled '<num> EXISTS'
* and '<num> RECENT' but as a probably-unintended side
* effect it ignores other unrecognized two-word
* responses. imap-send doesn't ever try to read
* messages or mailboxes these days, so consider
* eliminating this case.
*/
^ permalink raw reply
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Junio C Hamano @ 2013-01-15 20:38 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Jonathan Nieder, git, kraai
In-Reply-To: <50F5B83E.9060800@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> What do we think about something like this for fishing for which:
>
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -644,6 +644,10 @@ yes () {
> :
> done
> }
> +which () {
> + echo >&2 "which is not portable (please use type)"
> + exit 1
> +}
>
>
> This will happen in runtime, which might be good enough ?
if (
which frotz &&
test $(frobonitz --version" -le 2.0
)
then
test_set_prereq FROTZ_FROBONITZ
else
echo >&2 "suitable Frotz/Frobonitz combo not available;"
echo >&2 "some tests may be skipped"
fi
I somehow think this is a lost cause.
^ permalink raw reply
* Re: [PATCH] attr: fix off-by-one directory component length calculation
From: Junio C Hamano @ 2013-01-15 20:49 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git, Nguyễn Thái Ngọc Duy
In-Reply-To: <201301152053.58561.avila.jn@gmail.com>
"Jean-Noël AVILA" <avila.jn@gmail.com> writes:
> Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known
> issue?
Which branch?
If you mean "'master' with the patch in this discussion applied", I
didn't even have a chance to start today's integration cycle, so I
don't know (it is not known to me).
^ permalink raw reply
* Re: [PATCH v2 00/14] Remove unused code from imap-send.c
From: Jonathan Nieder @ 2013-01-15 20:49 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, Jeff King
In-Reply-To: <1358237193-8887-1-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty wrote:
> imap-send.c | 308 +++++++++++-------------------------------------------------
> 1 file changed, 55 insertions(+), 253 deletions(-)
Patch 14 is lovely. Except for patch 6, for what it's worth these are
all
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Nicely done.
^ permalink raw reply
* Re: [RFC/PATCH 2/8 v2] git_remote_helpers: fix input when running under Python 3
From: Junio C Hamano @ 2013-01-15 20:51 UTC (permalink / raw)
To: John Keeping
Cc: Michael Haggerty, git, Eric S. Raymond, Felipe Contreras,
Sverre Rabbelier
In-Reply-To: <20130115194809.GU4574@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> Although 2to3 will fix most issues in Python 2 code to make it run under
> Python 3, it does not handle the new strict separation between byte
> strings and unicode strings. There is one instance in
> git_remote_helpers where we are caught by this, which is when reading
> refs from "git for-each-ref".
>
> While we could fix this by explicitly handling refs as byte strings,
> this is merely punting the problem to users of the library since the
> same problem will be encountered as soon you want to display the ref
> name to a user.
>
> Instead of doing this, explicit decode the incoming byte string into a
> unicode string.
That really feels wrong. Displaying is a separate issue and it is
the _right_ thing to punt the problem at the lower-level machinery
level.
> Following the lead of pygit2 (the Python bindings for
> libgit2 - see [1] and [2]),...
I do not think other people getting it wrong is not an excuse to
repeat the same mistake.
Is it really so cumbersome to handle byte strings as byte strings in
Python?
^ permalink raw reply
* [PATCH] config.txt: Document help.htmlpath config parameter
From: Sebastian Staudt @ 2013-01-15 20:56 UTC (permalink / raw)
To: git
Signed-off-by: Sebastian Staudt <koraktor@gmail.com>
---
Documentation/config.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index bf8f911..e452ff8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1351,6 +1351,12 @@ help.autocorrect::
value is 0 - the command will be just shown but not executed.
This is the default.
+help.htmlpath::
+ Specify the path where the HTML documentation resides. File system paths
+ and URLs are supported. HTML pages will be prefixed with this path when
+ help is displayed in the 'web' format. This defaults to the documentation
+ path of your Git installation.
+
http.proxy::
Override the HTTP proxy, normally configured using the 'http_proxy',
'https_proxy', and 'all_proxy' environment variables (see
--
1.8.1.1
^ permalink raw reply related
* Re: [PATCH] config.txt: Document help.htmlpath config parameter
From: Junio C Hamano @ 2013-01-15 21:51 UTC (permalink / raw)
To: Sebastian Staudt; +Cc: git
In-Reply-To: <20130115205621.GB49671@mormegil.local>
Thanks.
This will eventually go to the maintenance track as well.
^ permalink raw reply
* Re: [RFC/PATCH 2/8 v2] git_remote_helpers: fix input when running under Python 3
From: John Keeping @ 2013-01-15 21:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, git, Eric S. Raymond, Felipe Contreras,
Sverre Rabbelier
In-Reply-To: <7vbocq2mri.fsf@alter.siamese.dyndns.org>
On Tue, Jan 15, 2013 at 12:51:13PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>> Although 2to3 will fix most issues in Python 2 code to make it run under
>> Python 3, it does not handle the new strict separation between byte
>> strings and unicode strings. There is one instance in
>> git_remote_helpers where we are caught by this, which is when reading
>> refs from "git for-each-ref".
>>
>> While we could fix this by explicitly handling refs as byte strings,
>> this is merely punting the problem to users of the library since the
>> same problem will be encountered as soon you want to display the ref
>> name to a user.
>>
>> Instead of doing this, explicit decode the incoming byte string into a
>> unicode string.
>
> That really feels wrong. Displaying is a separate issue and it is
> the _right_ thing to punt the problem at the lower-level machinery
> level.
But the display will require decoding the ref name to a Unicode string,
which depends on the encoding of the underlying ref name, so it feels
like it should be decoded where it's read (see [1]).
>> Following the lead of pygit2 (the Python bindings for
>> libgit2 - see [1] and [2]),...
>
> I do not think other people getting it wrong is not an excuse to
> repeat the same mistake.
>
> Is it really so cumbersome to handle byte strings as byte strings in
> Python?
As [1] says, there is a potential for bugs whenever people attempt to
combine Unicode and byte strings. I think it also violates the
principle of least surprise if a ref name (a string) doesn't behave like
a normal string.
[1] http://docs.python.org/3.3/howto/unicode.html#tips-for-writing-unicode-aware-programs
John
^ permalink raw reply
* Re: [RFC/PATCH 2/8 v2] git_remote_helpers: fix input when running under Python 3
From: Junio C Hamano @ 2013-01-15 22:04 UTC (permalink / raw)
To: John Keeping
Cc: Michael Haggerty, git, Eric S. Raymond, Felipe Contreras,
Sverre Rabbelier
In-Reply-To: <20130115215412.GX4574@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
>> That really feels wrong. Displaying is a separate issue and it is
>> the _right_ thing to punt the problem at the lower-level machinery
>> level.
>
> But the display will require decoding the ref name to a Unicode string,
> which depends on the encoding of the underlying ref name, so it feels
> like it should be decoded where it's read (see [1]).
If you botch the decoding in a way you cannot recover the original
byte string, you cannot create a ref whose name is the original byte
string, no? Keeping the original byte string internally (this
includes where you use it to create new refs or update existing
refs), and attempting to convert it to Unicode when you choose to
show that string as a part of a message to the user (and falling
back to replacing some bytes to '?' if you cannot, but do so only in
the message), you won't have that problem.
^ permalink raw reply
* Re: [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store
From: Junio C Hamano @ 2013-01-15 22:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Michael Haggerty, git, Jeff King
In-Reply-To: <20130115203204.GA12524@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Michael Haggerty wrote:
>
>> - else if ((arg1 = next_arg(&cmd))) {
>> - if (!strcmp("EXISTS", arg1))
>> - ctx->gen.count = atoi(arg);
>> - else if (!strcmp("RECENT", arg1))
>> - ctx->gen.recent = atoi(arg);
>> + } else if ((arg1 = next_arg(&cmd))) {
>> + /* unused */
>
> The above is just the right thing to do to ensure no behavior change.
> Let's take a look at the resulting code, though:
>
> if (... various reasonable things ...) {
> ...
> } else if ((arg1 = next_arg(&cmd))) {
> /* unused */
> } else {
> fprintf(stderr, "IMAP error: unable to parse untagged response\n");
> return RESP_BAD;
>
> Anyone forced by some bug to examine this "/* unused */" case is going
> to have no clue what's going on. In that respect, the old code was
> much better, since it at least made it clear that one case where this
> code gets hit is handling "<num> EXISTS" and "<num> RECENT" untagged
> responses.
>
> I suspect that original code did not have an implicit and intended
> missing
>
> else
> ; /* negligible response; ignore it */
>
> but the intent was rather
>
> else {
> fprintf(stderr, "IMAP error: I can't parse this\n");
> return RESP_BAD;
> }
>
> Since actually fixing that is probably too aggressive for this patch,
> how about a FIXME comment like the following?
>
> /*
> * Unhandled response-data with at least two words.
> * Ignore it.
> *
> * NEEDSWORK: Previously this case handled '<num> EXISTS'
> * and '<num> RECENT' but as a probably-unintended side
> * effect it ignores other unrecognized two-word
> * responses. imap-send doesn't ever try to read
> * messages or mailboxes these days, so consider
> * eliminating this case.
> */
Hmph; it seems that it is not worth rerolling the whole thing only
for this, so let me squash this in, replacing the /* unused */ with
the large comment, and then merge the result to 'next'.
^ permalink raw reply
* [RFC/PATCH 2/8 v3] git_remote_helpers: fix input when running under Python 3
From: John Keeping @ 2013-01-15 22:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, git, Eric S. Raymond, Felipe Contreras,
Sverre Rabbelier
In-Reply-To: <7vy5fu14sy.fsf@alter.siamese.dyndns.org>
Although 2to3 will fix most issues in Python 2 code to make it run under
Python 3, it does not handle the new strict separation between byte
strings and unicode strings. There is one instance in
git_remote_helpers where we are caught by this, which is when reading
refs from "git for-each-ref".
Fix this by operating on the returned string as a byte string rather
than a unicode string. As this method is currently only used internally
by the class this does not affect code anywhere else.
Note that we cannot use byte strings in the source as the 'b' prefix is
not supported before Python 2.7 so in order to maintain compatibility
with the maximum range of Python versions we use an explicit call to
encode().
Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Tue, Jan 15, 2013 at 02:04:29PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>>> That really feels wrong. Displaying is a separate issue and it is
>>> the _right_ thing to punt the problem at the lower-level machinery
>>> level.
>>
>> But the display will require decoding the ref name to a Unicode string,
>> which depends on the encoding of the underlying ref name, so it feels
>> like it should be decoded where it's read (see [1]).
>
> If you botch the decoding in a way you cannot recover the original
> byte string, you cannot create a ref whose name is the original byte
> string, no? Keeping the original byte string internally (this
> includes where you use it to create new refs or update existing
> refs), and attempting to convert it to Unicode when you choose to
> show that string as a part of a message to the user (and falling
> back to replacing some bytes to '?' if you cannot, but do so only in
> the message), you won't have that problem.
Actually, this method is currently only used internally so I don't think
my argument holds.
This is what keeping the refs as byte strings looks like.
git_remote_helpers/git/importer.py | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
index e28cc8f..c54846c 100644
--- a/git_remote_helpers/git/importer.py
+++ b/git_remote_helpers/git/importer.py
@@ -18,13 +18,16 @@ class GitImporter(object):
def get_refs(self, gitdir):
"""Returns a dictionary with refs.
+
+ Note that the keys in the returned dictionary are byte strings as
+ read from git.
"""
args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
- lines = check_output(args).strip().split('\n')
+ lines = check_output(args).strip().split('\n'.encode('utf-8'))
refs = {}
for line in lines:
- value, name = line.split(' ')
- name = name.strip('commit\t')
+ value, name = line.split(' '.encode('utf-8'))
+ name = name.strip('commit\t'.encode('utf-8'))
refs[name] = value
return refs
--
1.8.1
^ permalink raw reply related
* Re: [PATCH 3/8] git_remote_helpers: Force rebuild if python version changes
From: John Keeping @ 2013-01-15 22:58 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, Eric S. Raymond, Felipe Contreras, Sverre Rabbelier
In-Reply-To: <20130113175238.GO4574@serenity.lan>
On Sun, Jan 13, 2013 at 05:52:38PM +0000, John Keeping wrote:
> On Sun, Jan 13, 2013 at 12:14:02PM -0500, Pete Wyckoff wrote:
>> john@keeping.me.uk wrote on Sun, 13 Jan 2013 16:26 +0000:
>>> On Sat, Jan 12, 2013 at 06:30:44PM -0500, Pete Wyckoff wrote:
>>> > john@keeping.me.uk wrote on Sat, 12 Jan 2013 19:23 +0000:
>>> >> When different version of python are used to build via distutils, the
>>> >> behaviour can change. Detect changes in version and pass --force in
>>> >> this case.
>>> >[..]
>>> >> diff --git a/git_remote_helpers/Makefile b/git_remote_helpers/Makefile
>>> >[..]
>>> >> +py_version=$(shell $(PYTHON_PATH) -c \
>>> >> + 'import sys; print("%i.%i" % sys.version_info[:2])')
>>> >> +
>>> >> all: $(pysetupfile)
>>> >> - $(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
>>> >> + $(QUIET)test "$$(cat GIT-PYTHON_VERSION 2>/dev/null)" = "$(py_version)" || \
>>> >> + flags=--force; \
>>> >> + $(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build $$flags
>>> >> + $(QUIET)echo "$(py_version)" >GIT-PYTHON_VERSION
>>> >
>>> > Can you depend on ../GIT-PYTHON-VARS instead? It comes from
>>> > 96a4647 (Makefile: detect when PYTHON_PATH changes, 2012-12-18).
>>> > It doesn't check version, just path, but hopefully that's good
>>> > enough. I'm imagining a rule that would do "clean" if
>>> > ../GIT-PYTHON-VARS changed, then build without --force.
>>>
>>> I was trying to keep the git_remote_helpers directory self contained. I
>>> can't see how to depend on ../GIT-PYTHON-VARS in a way that is as simple
>>> as this and keeps "make -C git_remote_helpers" working in a clean tree.
>>>
>>> Am I missing something obvious here?
>>
>> Not if it wants to stay self-contained; you're right.
>>
>> I'm not thrilled with how git_remote_helpers/Makefile always
>> runs setup.py, and always generates PYLIBDIR, and now always
>> invokes python a third time to see if its version changed.
>
> I don't think PYLIBDIR will be calculated unless it's used ('=' not
> ':=' means its a deferred variable).
>
> I wonder if the version check should move into setup.py - it would be
> just as easy to check the file there and massage sys.args, although
> possibly not as neat.
For reference, putting the version check in setup.py looks like this:
-- >8 --
diff --git a/git_remote_helpers/setup.py b/git_remote_helpers/setup.py
index 6de41de..2c21eb5 100644
--- a/git_remote_helpers/setup.py
+++ b/git_remote_helpers/setup.py
@@ -3,6 +3,7 @@
"""Distutils build/install script for the git_remote_helpers package."""
from distutils.core import setup
+import sys
# If building under Python3 we need to run 2to3 on the code, do this by
# trying to import distutils' 2to3 builder, which is only available in
@@ -13,6 +14,24 @@ except ImportError:
# 2.x
from distutils.command.build_py import build_py
+
+current_version = '%d.%d' % sys.version_info[:2]
+try:
+ f = open('GIT-PYTHON_VERSION', 'r')
+ latest_version = f.read().strip()
+ f.close()
+
+ if latest_version != current_version:
+ if not '--force' in sys.argv:
+ sys.argv.insert(0, '--force')
+except IOError:
+ pass
+
+f = open('GIT-PYTHON_VERSION', 'w')
+f.write(current_version)
+f.close()
+
+
setup(
name = 'git_remote_helpers',
version = '0.1.0',
^ permalink raw reply related
* Re: [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store
From: Jonathan Nieder @ 2013-01-15 22:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git, Jeff King
In-Reply-To: <7vtxqi13ks.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Since actually fixing that is probably too aggressive for this patch,
>> how about a FIXME comment like the following?
[...]
> Hmph; it seems that it is not worth rerolling the whole thing only
> for this, so let me squash this in, replacing the /* unused */ with
> the large comment, and then merge the result to 'next'.
Sounds good to me. Next time, I'll include a 'fixup!' patch instead of
making you do the copy/pasting.
Thanks, both.
Jonathan
^ permalink raw reply
* [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset
From: Ben Walton @ 2013-01-15 23:10 UTC (permalink / raw)
To: gitster; +Cc: esr, git, Ben Walton
In-Reply-To: <1358291405-10173-1-git-send-email-bdwalton@gmail.com>
This function has utility outside of the SVN module for any routine
that needs the equivalent of GNU strftime's %z formatting option.
Move it to the top-level Git.pm so that non-SVN modules don't need to
import the SVN module to use it.
The rename makes the purpose of the function clearer.
Signed-off-by: Ben Walton <bdwalton@gmail.com>
---
perl/Git.pm | 23 +++++++++++++++++++++++
perl/Git/SVN.pm | 12 ++----------
perl/Git/SVN/Log.pm | 8 ++++++--
3 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 931047c..5649bcc 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -59,6 +59,7 @@ require Exporter;
command_bidi_pipe command_close_bidi_pipe
version exec_path html_path hash_object git_cmd_try
remote_refs prompt
+ get_tz_offset
temp_acquire temp_release temp_reset temp_path);
@@ -102,6 +103,7 @@ use Error qw(:try);
use Cwd qw(abs_path cwd);
use IPC::Open2 qw(open2);
use Fcntl qw(SEEK_SET SEEK_CUR);
+use Time::Local qw(timelocal);
}
@@ -511,6 +513,27 @@ C<git --html-path>). Useful mostly only internally.
sub html_path { command_oneline('--html-path') }
+
+=item get_tz_offset ( TIME )
+
+Return the time zone offset from GMT in the form +/-HHMM where HH is
+the number of hours from GMT and MM is the number of minutes. This is
+the equivalent of what strftime("%z", ...) would provide on a GNU
+platform.
+
+If TIME is not supplied, the current local time is used.
+
+=cut
+
+sub get_tz_offset {
+ # some systmes don't handle or mishandle %z, so be creative.
+ my $t = shift || time;
+ my $gm = timelocal(gmtime($t));
+ my $sign = qw( + + - )[ $t <=> $gm ];
+ return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
+}
+
+
=item prompt ( PROMPT , ISPASSWORD )
Query user C<PROMPT> and return answer from user.
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 59215fa..8c84560 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -11,7 +11,6 @@ use Carp qw/croak/;
use File::Path qw/mkpath/;
use File::Copy qw/copy/;
use IPC::Open3;
-use Time::Local;
use Memoize; # core since 5.8.0, Jul 2002
use Memoize::Storable;
use POSIX qw(:signal_h);
@@ -22,6 +21,7 @@ use Git qw(
command_noisy
command_output_pipe
command_close_pipe
+ get_tz_offset
);
use Git::SVN::Utils qw(
fatal
@@ -1311,14 +1311,6 @@ sub get_untracked {
\@out;
}
-sub get_tz {
- # some systmes don't handle or mishandle %z, so be creative.
- my $t = shift || time;
- my $gm = timelocal(gmtime($t));
- my $sign = qw( + + - )[ $t <=> $gm ];
- return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
-}
-
# parse_svn_date(DATE)
# --------------------
# Given a date (in UTC) from Subversion, return a string in the format
@@ -1351,7 +1343,7 @@ sub parse_svn_date {
delete $ENV{TZ};
}
- my $our_TZ = get_tz();
+ my $our_TZ = get_tz_offset();
# This converts $epoch_in_UTC into our local timezone.
my ($sec, $min, $hour, $mday, $mon, $year,
diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
index 3cc1c6f..3f8350a 100644
--- a/perl/Git/SVN/Log.pm
+++ b/perl/Git/SVN/Log.pm
@@ -2,7 +2,11 @@ package Git::SVN::Log;
use strict;
use warnings;
use Git::SVN::Utils qw(fatal);
-use Git qw(command command_oneline command_output_pipe command_close_pipe);
+use Git qw(command
+ command_oneline
+ command_output_pipe
+ command_close_pipe
+ get_tz_offset);
use POSIX qw/strftime/;
use constant commit_log_separator => ('-' x 72) . "\n";
use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline
@@ -119,7 +123,7 @@ sub run_pager {
sub format_svn_date {
my $t = shift || time;
require Git::SVN;
- my $gmoff = Git::SVN::get_tz($t);
+ my $gmoff = get_tz_offset($t);
return strftime("%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y)", localtime($t));
}
--
1.7.10.4
^ permalink raw reply related
* [PATCH 0/3] Fix a portability issue with git-cvsimport
From: Ben Walton @ 2013-01-15 23:10 UTC (permalink / raw)
To: gitster; +Cc: esr, git, Ben Walton
This patch series started as a quick fix for the use of %s and %z in
git-cvsimport but grew slightly when I realized that the get_tz
(get_tz_offset after this series) function used by Git::SVN didn't
properly handle DST boundary conditions.
I realize that Eric Raymond is working to deprecate the current
iteration of git-cvsimport so this series may be only partially
worthwhile. (If the cvsps 2 vs 3 issue does require a fallback
git-cvsimport script then maybe the whole series is still valid?) I'm
not attached to the current git-cvsimport so if the third patch isn't
useful then maybe the only the second patch is worthwhile (modified to
correct the function in its current location).
Currently, the DST boundary functionality is exercised by the
git-cvsimport tests. If those go away as part of Eric's work then
another test that monitors this condition should be added. I can do
that as part of this series if it seems the right way to go.
Ben Walton (3):
Move Git::SVN::get_tz to Git::get_tz_offset
Allow Git::get_tz_offset to properly handle DST boundary times
Avoid non-portable strftime format specifiers in git-cvsimport
git-cvsimport.perl | 5 ++++-
perl/Git.pm | 43 +++++++++++++++++++++++++++++++++++++++++++
perl/Git/SVN.pm | 12 ++----------
perl/Git/SVN/Log.pm | 8 ++++++--
4 files changed, 55 insertions(+), 13 deletions(-)
--
1.7.10.4
^ permalink raw reply
* [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport
From: Ben Walton @ 2013-01-15 23:10 UTC (permalink / raw)
To: gitster; +Cc: esr, git, Ben Walton
In-Reply-To: <1358291405-10173-1-git-send-email-bdwalton@gmail.com>
Neither %s or %z are portable strftime format specifiers. There is no
need for %s in git-cvsimport as the supplied time is already in
seconds since the epoch. For %z, use the function get_tz_offset
provided by Git.pm instead.
Signed-off-by: Ben Walton <bdwalton@gmail.com>
---
git-cvsimport.perl | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 0a31ebd..344f120 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -26,6 +26,7 @@ use IO::Socket;
use IO::Pipe;
use POSIX qw(strftime tzset dup2 ENOENT);
use IPC::Open2;
+use Git qw(get_tz_offset);
$SIG{'PIPE'}="IGNORE";
set_timezone('UTC');
@@ -864,7 +865,9 @@ sub commit {
}
set_timezone($author_tz);
- my $commit_date = strftime("%s %z", localtime($date));
+ # $date is in the seconds since epoch format
+ my $tz_offset = get_tz_offset($date);
+ my $commit_date = "$date $tz_offset";
set_timezone('UTC');
$ENV{GIT_AUTHOR_NAME} = $author_name;
$ENV{GIT_AUTHOR_EMAIL} = $author_email;
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/3] Allow Git::get_tz_offset to properly handle DST boundary times
From: Ben Walton @ 2013-01-15 23:10 UTC (permalink / raw)
To: gitster; +Cc: esr, git, Ben Walton
In-Reply-To: <1358291405-10173-1-git-send-email-bdwalton@gmail.com>
The Git::get_tz_offset is meant to provide a workalike replacement for
the GNU strftime %z format specifier. The algorithm used failed to
properly handle DST boundary cases.
For example, the unix time 1162105199 in CST6CDT saw set_tz_offset
improperly return -0600 instead of -0500.
TZ=CST6CDT date -d @1162105199 +"%c %z"
Sun 29 Oct 2006 01:59:59 AM CDT -0500
$ zdump -v /usr/share/zoneinfo/CST6CDT | grep 2006
/usr/share/zoneinfo/CST6CDT Sun Apr 2 07:59:59 2006 UTC = Sun Apr 2
01:59:59 2006 CST isdst=0 gmtoff=-21600
/usr/share/zoneinfo/CST6CDT Sun Apr 2 08:00:00 2006 UTC = Sun Apr 2
03:00:00 2006 CDT isdst=1 gmtoff=-18000
/usr/share/zoneinfo/CST6CDT Sun Oct 29 06:59:59 2006 UTC = Sun Oct 29
01:59:59 2006 CDT isdst=1 gmtoff=-18000
/usr/share/zoneinfo/CST6CDT Sun Oct 29 07:00:00 2006 UTC = Sun Oct 29
01:00:00 2006 CST isdst=0 gmtoff=-21600
To determine how many hours/minutes away from GMT a particular time
was, we calculated the gmtime() of the requested time value and then
used Time::Local's timelocal() function to turn the GMT-based time
back into a scalar value representing seconds from the epoch. Because
GMT has no daylight savings time, timelocal() cannot handle the
ambiguous times that occur at DST boundaries since there are two
possible correct results.
To work around the ambiguity at these boundaries, we must take into
account the pre and post conversion values for is_dst as provided by
both the original time value and the value that has been run through
timelocal(). If the is_dst field of the two times disagree then we
must modify the value returned from timelocal() by an hour in the
correct direction.
Signed-off-by: Ben Walton <bdwalton@gmail.com>
---
perl/Git.pm | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/perl/Git.pm b/perl/Git.pm
index 5649bcc..788b9b4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -528,7 +528,27 @@ If TIME is not supplied, the current local time is used.
sub get_tz_offset {
# some systmes don't handle or mishandle %z, so be creative.
my $t = shift || time;
+ # timelocal() has a problem when it comes to DST ambiguity so
+ # times that are on a DST boundary cannot be properly converted
+ # using it. we will possibly adjust its result depending on whehter
+ # pre and post conversions agree on DST
my $gm = timelocal(gmtime($t));
+
+ # we need to know whether we were originally in DST or not
+ my $orig_dst = (localtime($t))[8];
+ # and also whether timelocal thinks we're in DST
+ my $conv_dst = (localtime($gm))[8];
+
+ # re-adjust $gm based on the DST value for the two times we're
+ # handling.
+ if ($orig_dst != $conv_dst) {
+ if ($orig_dst == 1) {
+ $gm -= 3600;
+ } else {
+ $gm += 3600;
+ }
+ }
+
my $sign = qw( + + - )[ $t <=> $gm ];
return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
}
--
1.7.10.4
^ permalink raw reply related
* --simplify-merges returns too many references
From: Phil Hord @ 2013-01-15 23:12 UTC (permalink / raw)
To: git@vger.kernel.org
I thought I understood the intent of the various history
simplification switches, but maybe I am still confused.
In git.git, I see three commits which touch stripspace.c:
$ git log --oneline -- builtin/stripspace.c
497215d Update documentation for stripspace
c2857fb stripspace: fix outdated comment
81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory
With --full-history and also with --dense, I see the same three commits:
$ git log --full-history --oneline -- builtin/stripspace.c
497215d Update documentation for stripspace
c2857fb stripspace: fix outdated comment
81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory
$ git log --dense --oneline -- builtin/stripspace.c
497215d Update documentation for stripspace
c2857fb stripspace: fix outdated comment
81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory
But with --simplify-merges, I see _more_ commits.
$ git log --simplify-merges --oneline -- builtin/stripspace.c
634392b Add 'contrib/subtree/' from commit ...
497215d Update documentation for stripspace
c2857fb stripspace: fix outdated comment
81b50f3 Move 'builtin-*' into a 'builtin/' subdirectory
610f043 Import branch 'git-p4' of git://repo.or.cz/fast-export
b4d2b04 Merge git-gui
0a8f4f0 Merge git://git.kernel.org/pub/scm/git/gitweb
98e031f Merge git-tools repository under "tools" subdirectory
5569bf9 Do a cross-project merge of Paul Mackerras' gitk visualizer
None of the "new" commits touches this file. The man page suggests
that simplify-merges should result in fewer commits than full-history.
" --simplify-merges
Additional option to --full-history to remove some needless merges from
the resulting history, as there are no selected commits contributing to
this merge."
Am I confused or is git?
Phil
^ permalink raw reply
* Re: [PATCH] attr: fix off-by-one directory component length calculation
From: Jeff King @ 2013-01-15 23:24 UTC (permalink / raw)
To: Jean-Noël AVILA
Cc: Junio C Hamano, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <7vfw222mv2.fsf@alter.siamese.dyndns.org>
On Tue, Jan 15, 2013 at 12:49:05PM -0800, Junio C Hamano wrote:
> "Jean-Noël AVILA" <avila.jn@gmail.com> writes:
>
> > Btw, the test 10 to t9902 is failing on my Debian testing. Is it a known
> > issue?
>
> Which branch?
t9902.10 is overly sensitive to extra git commands in your PATH, as well
as cruft in your build dir (especially if you have been building 'pu',
which has git-check-ignore). Try "make clean && make test".
-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