* Re: [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport
From: Chris Rorvick @ 2013-01-16 1:53 UTC (permalink / raw)
To: Ben Walton; +Cc: gitster, esr, git
In-Reply-To: <1358291405-10173-4-git-send-email-bdwalton@gmail.com>
On Tue, Jan 15, 2013 at 5:10 PM, Ben Walton <bdwalton@gmail.com> wrote:
> 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.
Out of curiosity, which platforms are affected? Assuming DST is a 1
hour shift (patch 2/3) is not necessarily portable either, though this
currently appears to only affect a small island off of the coast of
Australia. :-)
Chris
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2013, #06; Mon, 14)
From: Adam Spiers @ 2013-01-16 1:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vehhn8kub.fsf@alter.siamese.dyndns.org>
On Mon, Jan 14, 2013 at 10:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> * as/check-ignore (2013-01-10) 12 commits
> (merged to 'next' on 2013-01-14 at 9df2afc)
> + t0008: avoid brace expansion
> + add git-check-ignore sub-command
> + setup.c: document get_pathspec()
> + add.c: extract new die_if_path_beyond_symlink() for reuse
> + add.c: extract check_path_for_gitlink() from treat_gitlinks() for reuse
> + pathspec.c: rename newly public functions for clarity
> + add.c: move pathspec matchers into new pathspec.c for reuse
> + add.c: remove unused argument from validate_pathspec()
> + dir.c: improve docs for match_pathspec() and match_pathspec_depth()
> + dir.c: provide clear_directory() for reclaiming dir_struct memory
> + dir.c: keep track of where patterns came from
> + dir.c: use a single struct exclude_list per source of excludes
>
> Add a new command "git check-ignore" for debugging .gitignore
> files.
The above is v4 plus the "t0008: avoid brace expansion" fix. v4 is
slightly outdated and not quite the right version to merge to 'next'.
I'll post a v5 re-roll as per:
http://thread.gmane.org/gmane.comp.version-control.git/212184/focus=212856
in the next 24 hours or so.
I think the "t0008: avoid brace expansion" fix at the tip should
probably be squashed into its parent. I've amended the commit message
accordingly in my github fork.
Thanks,
Adam
^ permalink raw reply
* Re: [PATCH] attr: fix off-by-one directory component length calculation
From: Duy Nguyen @ 2013-01-16 1:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7vwqve2qk3.fsf@alter.siamese.dyndns.org>
On Wed, Jan 16, 2013 at 2:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "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
Actually I'd like to remove that function. The function is called twice:
- collect_all_attrs
+ prepare_attr_stack
* find_basename
+ find_basename
which could be reordered to
- collect_all_attrs
+ find_basename
+ prepare_attr_stack (modified to take dirlen from caller)
and because that'll be the only place find_basename is used, we could
just inline the code there.
--
Duy
^ permalink raw reply
* Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
From: René Scharfe @ 2013-01-16 1:08 UTC (permalink / raw)
To: Andreas Schwab
Cc: Junio C Hamano, git, Jeff King, Carlos Martín Nieto,
Johannes Schindelin
In-Reply-To: <m2libunqdj.fsf@igel.home>
Am 15.01.2013 21:27, schrieb Andreas Schwab:
> 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.
Thanks for catching this!
-- >8 --
Subject: [PATCH] refs: use strncmp() instead of strlen() and memcmp()
Simplify ref_entry_cmp_sslice() by using strncmp() to compare the
length-limited key and a NUL-terminated entry. While we're at it,
retain the const attribute of the input pointers.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
refs.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 541fec2..5129da0 100644
--- a/refs.c
+++ b/refs.c
@@ -333,14 +333,12 @@ struct string_slice {
static int ref_entry_cmp_sslice(const void *key_, const void *ent_)
{
- struct string_slice *key = (struct string_slice *)key_;
- struct ref_entry *ent = *(struct ref_entry **)ent_;
- int entlen = strlen(ent->name);
- int cmplen = key->len < entlen ? key->len : entlen;
- int cmp = memcmp(key->str, ent->name, cmplen);
+ const struct string_slice *key = key_;
+ const struct ref_entry *ent = *(const struct ref_entry * const *)ent_;
+ int cmp = strncmp(key->str, ent->name, key->len);
if (cmp)
return cmp;
- return key->len - entlen;
+ return '\0' - (unsigned char)ent->name[key->len];
}
/*
--
1.8.0
^ permalink raw reply related
* Re: [PATCH] attr: fix off-by-one directory component length calculation
From: Duy Nguyen @ 2013-01-16 1:03 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git
In-Reply-To: <201301152014.28433.avila.jn@gmail.com>
On Wed, Jan 16, 2013 at 2:14 AM, Jean-Noël AVILA <avila.jn@gmail.com> wrote:
> I did not monitor the system calls when writing that patch.
> Where is the perf framework?
It's in t/perf. I think you can do:
./run HEAD .
to run and compare performance of HEAD and working directory (assume
you haven't commit yet). Check out the README file.
--
Duy
^ permalink raw reply
* Re: real git pull grief
From: Jay Vee @ 2013-01-16 0:50 UTC (permalink / raw)
To: git
In-Reply-To: <20130116004534.GG12524@google.com>
git diff shows a list of files that I have made some changes to and
the diff for files that I have not changed.
The files I have made changes to (config files), I never want to
commit or push and sometimes I may want them overwritten. for now I
simply want to leave them there (changes to .sql files that are
specific to my environment).
What I want to know is why git is reporting that changes will be
overwritten to files that I have not touched or changed.
>> $git diff spews a lot of information.
>
> Just a representative first 20 lines or so would be fine. :)
>
> Jonathan
^ permalink raw reply
* Re: real git pull grief
From: Jonathan Nieder @ 2013-01-16 0:20 UTC (permalink / raw)
To: Jay Vee; +Cc: git
In-Reply-To: <CADq_mb8LwzbjvaXGCR-6TZbTShf2nzw5wtNZ66_XmOM00-=xzQ@mail.gmail.com>
Hi Jay,
Jay Vee wrote:
> I HAVE NOT CHANGED THIS FILE. It is telling me that my local
> changes.... huh? I have not changed or modified the file.
>
> ----
> When I do a $git status I see a lot of other files that begin with:
>
> # modified: ....
> <many of these>
Two questions:
* what does "git diff" say?
* do you use Eclipse's git integration?
Curious,
Jonathan
^ permalink raw reply
* real git pull grief
From: Jay Vee @ 2013-01-16 0:17 UTC (permalink / raw)
To: git
I have not changed any code and just tried to do a git pull and get
the following message:
Updating 527f1ee..18cf73e
error: Your local changes to the following files would be overwritten by merge:
java/..../Info.java
Please, commit your changes or stash them before you can merge.
Aborting
----------
I HAVE NOT CHANGED THIS FILE. It is telling me that my local
changes.... huh? I have not changed or modified the file.
----
When I do a $git status I see a lot of other files that begin with:
# modified: ....
<many of these>
How do I get out of this mess? I do not want to reset head. I want
to get back to the state where it does not think I have modified a
specific set of files... namely the files that I did not change.
How did I get into this state? How do I prevent from getting into
this state in the future? How do I get out of this state now?
thanks for all the help.
J.V.
^ permalink raw reply
* Re: [RFC/PATCH 2/8 v3] git_remote_helpers: fix input when running under Python 3
From: Pete Wyckoff @ 2013-01-16 0:03 UTC (permalink / raw)
To: John Keeping
Cc: Junio C Hamano, Michael Haggerty, git, Eric S. Raymond,
Felipe Contreras, Sverre Rabbelier
In-Reply-To: <20130115224049.GZ4574@serenity.lan>
john@keeping.me.uk wrote on Tue, 15 Jan 2013 22:40 +0000:
> This is what keeping the refs as byte strings looks like.
As John knows, it is not possible to interpret text from a byte
string without talking about the character encoding.
Git is (largely) a C program and uses the character set defined
in the C standard, which is a subset of ASCII. But git does
"math" on strings, like this snippet that takes something from
argv[] and prepends "refs/heads/":
strcpy(refname, "refs/heads/");
strcpy(refname + strlen("refs/heads/"), ret->name);
The result doesn't talk about what character set it is using,
but because it combines a prefix from ASCII with its input,
git makes the assumption that the input is ASCII-compatible.
If you feed a UTF-16 string in argv, e.g.
$ echo master | iconv -f ascii -t utf16 | xargs git branch
xargs: Warning: a NUL character occurred in the input. It cannot be passed through in the argument list. Did you mean to use the --null option?
fatal: Not a valid object name: ''.
you get an error about NUL, and not the branch you hoped for.
Git assumes that the input character set contains roughly ASCII
in byte positions 0..127.
That's one small reason why the useful character encodings put
ASCII in the 0..127 range, including utf-8, big5 and shift-jis.
ASCII is indeed special due to its legacy, and both C and Python
recognize this.
> diff --git 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
I'd suggest for this Python conundrum using byte-string literals, e.g.:
lines = check_output(args).strip().split(b'\n')
value, name = line.split(b' ')
name = name.strip(b'commit\t')
Essentially identical to what you have, but avoids naming "utf-8" as
the encoding. It instead relies on Python's interpretation of
ASCII characters in string context, which is exactly what C does.
-- Pete
^ permalink raw reply
* Re: [PATCH] remote-hg: fix handling of file perms when pushing
From: Junio C Hamano @ 2013-01-15 23:51 UTC (permalink / raw)
To: Max Horn; +Cc: git, Felipe Contreras
In-Reply-To: <1E49829A-0675-40D2-97C6-FD62982A0923@quendi.de>
Max Horn <max@quendi.de> writes:
> On 15.01.2013, at 14:02, Max Horn wrote:
>
>> Previously, when changing and committing an executable file, the file
>> would loose its executable bit on the hg side. Likewise, symlinks ended
>> up as "normal" files". This was not immediately apparent on the git side
>> unless one did a fresh clone.
>
> Sorry, forgot to sign off, please add:
>
> Signed-off-by: Max Horn <max@quendi.de>
>
> Max
Thanks; merged together with the other patch from Felipe to 'next'.
Unfortunately I noticed the "loose" typo (I think you meant "lose")
after I pushed out the results X-<.
^ permalink raw reply
* Re: --simplify-merges returns too many references
From: Junio C Hamano @ 2013-01-15 23:48 UTC (permalink / raw)
To: Phil Hord; +Cc: git@vger.kernel.org
In-Reply-To: <CABURp0q0nhka+ivrs3+m+3C1N+FfTq2zJ=1rWp34tfGNQ3b30g@mail.gmail.com>
Phil Hord <phil.hord@gmail.com> writes:
> 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
After indenting away what should be shown, I notice all these extra
are merges without any common ancestors.
Just an observation; I didn't think things through.
^ permalink raw reply
* Re: [PATCH 0/3] Fix a portability issue with git-cvsimport
From: Junio C Hamano @ 2013-01-15 23:43 UTC (permalink / raw)
To: Ben Walton; +Cc: esr, git
In-Reply-To: <1358291405-10173-1-git-send-email-bdwalton@gmail.com>
Ben Walton <bdwalton@gmail.com> writes:
> 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?)
There is my reroll of Eric's patch [*1*], that is in 'pu'. The topic
ends at 12b3541 (t9600: adjust for new cvsimport, 2013-01-13).
I think the folks on the traditional Git side prefer the approach
taken by it to keep the old one under cvsimport-2 while adding
Eric's as cvsimport-3 and have a separate version switcher wrapper
[*2*, *3*]. Also Chris Rorvick, a contributor to cvsps-3 & new
cvsimport combo, who already has patches to Eric's version, agrees
that it is a foundation we can build on together [*4*].
Eric hasn't spoken on the topic yet, but I think what the rest of us
agreed may be a reasonable starting point.
I think I can apply your patches on top of 12b3541 with "am -3" and
have it automatically update git-cvsimport-2.perl ;-)
[References]
*1* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213460
*2* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213432
*3* http://thread.gmane.org/gmane.comp.version-control.git/213170/focus=213466
*4* http://thread.gmane.org/gmane.comp.version-control.git/213537/focus=213595
^ 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
* --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
* [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
* [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 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 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
* 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
* 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
* [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 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
* 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: [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: [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
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