* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Chris Rorvick @ 2013-01-17 3:42 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <20130117031100.GA7264@sigill.intra.peff.net>
On Wed, Jan 16, 2013 at 9:11 PM, Jeff King <peff@peff.net> wrote:
>> is_forwardable() did solve a UI issue. Previously all instances where
>> old is not reachable by new were assumed to be addressable with a
>> merge. is_forwardable() attempted to determine if the concept of
>> forwarding made sense given the inputs. For example, if old is a blob
>> it is useless to suggest merging it.
>
> I think it makes sense to mark such a case as different from a regular
> non-fast-forward (because "git pull" is not the right advice), but:
>
> 1. is_forwardable should assume a missing object is a commit not to
> regress the common case; otherwise we do not show the pull advice
> when we probably should, and most of the time it is going to be a
> commit
Yes, obviously this was a bug, thus the use of "attempted" above. It
would have been better to assume a missing 'old' was potentially
forwardable to present the user with the most helpful advice.
> 2. When we know that we are not working with commits, I am not sure
> that "already exists" is the right advice to give for such a case.
> It is neither "this tag already exists, so we do not update it",
> nor is it strictly "cannot fast forward this commit", but rather
> something else.
But the reference already existing in the remote is a substantial
reason for not allowing the push in all of these cases. You can break
this out further if you like to explain why the specific reference
shouldn't be moved on the remote, but this is even more complicated a
simple "is old reachable from new?" test.
Chris
^ permalink raw reply
* Re: [PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list()
From: Michael Haggerty @ 2013-01-17 4:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matt Kraai, git, Jonathan Nieder
In-Reply-To: <7vip6xywdf.fsf@alter.siamese.dyndns.org>
On 01/16/2013 04:34 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> On 01/15/2013 07:51 PM, Matt Kraai wrote:
>>> On Tue, Jan 15, 2013 at 09:06:25AM +0100, Michael Haggerty wrote:
>>>> -static struct imap_list *parse_imap_list(struct imap *imap, char **sp)
>>>> +static struct imap_list *parse_list(char **sp)
>>>
>>> The commit subject refers to imap_parse_list and imap_list whereas the
>>> code refers to parse_imap_list and parse_list.
>>
>> Yes, you're right. Thanks.
>
> I think I've fixed this (and some other minor points in other
> patches in the series) while queuing; please check master..3691031c
> after fetching from me.
Looks good. Thanks.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: Question re. git remote repository
From: Matt Seitz @ 2013-01-17 5:20 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: david@lang.hm, 'matseitz@cisco.com'
"David Lang" <david@lang.hm> wrote in message news:<alpine.DEB.2.02.1301161843390.21503@nftneq.ynat.uz>...
> >>
> >> On Thu, 17 Jan 2013, Matt Seitz (matseitz) wrote:
> >>
> >>> 2. a repository where only one user does "git add" and "git commit",
> >> while other users will do "git pull", the peer-to-peer model (you pull changes
> >> from me, I pull changes from you).
> >>
>
> you may _be_ safe, and if others who really know the internals speak up, take
> their word on it.
Well, we already have Linus's article. I guess the question is whether the use case I describe above falls under:
A. "maintenance operations"
B. "normal git workflow"
Personally, I would consider this use case to be a "normal git workflow".
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Chris Rorvick @ 2013-01-17 6:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Max Horn, git, Angelo Borsotti, Drew Northup, Michael Haggerty,
Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
Felipe Contreras
In-Reply-To: <7vobgpxeel.fsf@alter.siamese.dyndns.org>
On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> It is fine when pushing into "refs/tags/" hierarchy. It is *NOT*
> OK if the type check does not satisfy this function. In that case,
> we do not actually see the existence of the destination as a
> problem, but it is reported as such. We are blocking because we do
> not like the type of the new object or the type of the old object.
> If the destination points at a commit, the push can succeed if the
> user changes what object to push, so saying "you cannot push because
> the destination already exists" is just wrong in such a case.
So the solution is to revert back to recommending a merge?
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-17 6:59 UTC (permalink / raw)
To: Chris Rorvick
Cc: Max Horn, git, Angelo Borsotti, Drew Northup, Michael Haggerty,
Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
Felipe Contreras
In-Reply-To: <CAEUsAPb0Zg0x78e+12NqXA4PRBkOUO89KTgxtwxujS1KOx9NYg@mail.gmail.com>
Chris Rorvick <chris@rorvick.com> writes:
> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> It is fine when pushing into "refs/tags/" hierarchy. It is *NOT*
>> OK if the type check does not satisfy this function. In that case,
>> we do not actually see the existence of the destination as a
>> problem, but it is reported as such. We are blocking because we do
>> not like the type of the new object or the type of the old object.
>> If the destination points at a commit, the push can succeed if the
>> user changes what object to push, so saying "you cannot push because
>> the destination already exists" is just wrong in such a case.
>
> So the solution is to revert back to recommending a merge?
Of course not, because at that point you may not even have what you
were attempting to overwrite. Nobody says it is even something you
could merge.
The recommended solution certainly will involve a "fetch" (not
"pull" or "pull --rebase"). You fetch from over there to check what
you were about to overwrite, examine the situation to decide what
the appropriate action is.
The point is that Git in general, and the codepath that was touched
by the patch in particular, does not have enough information to
decide what the appropriate action is for the user, especially when
the ref is outside the ones we know what the conventional uses of
them are. We can make policy decisions like "tags are meant to be
unmoving anchor points, so it is unusual to overwrite any old with
any new", "heads are meant to be branch tips, and because rewinding
them while more than one repositories are working with them will
cause issues to other repositories, it is unusual to push a
non-fast-forward" and enforcement mechanism for such policy
decisions will help users, but that is only because we know what
their uses are.
The immediate action we should take is to get closer to the original
behaviour of not complaining with "ref already exists", which is
nonsensical. That does not mean that we will forbid improving the
codepath by giving different advices depending on the case.
One of the new advices could tell them to "fetch it and inspect the
situation", if old is not something we do not even have (hence we
cannot check its type, let alone the ancestry relationship of it
with new), for example.
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: John Keeping @ 2013-01-17 10:24 UTC (permalink / raw)
To: Jeff King; +Cc: Max Horn, Antoine Pelisse, Junio C Hamano, git, Johannes Sixt
In-Reply-To: <20130116190137.GD2476@farnsworth.metanate.com>
On Wed, Jan 16, 2013 at 07:01:37PM +0000, John Keeping wrote:
> On Wed, Jan 16, 2013 at 10:24:49AM -0800, Jeff King wrote:
> > On Wed, Jan 16, 2013 at 06:22:40PM +0000, John Keeping wrote:
> >
> > Thanks for checking. I'd rather squelch the warning completely (as in my
> > re-post of Max's patch from a few minutes ago), and we can loosen it
> > (possibly with a version check) later when a fix is widely disseminated.
>
> I checked again with a trunk build of clang and the warning's still
> there, so I've created a clang bug [1] to see if they will change the
> behaviour.
>
> [1] http://llvm.org/bugs/show_bug.cgi?id=14968
Well, that was quick! This warning is now gone when using a fresh trunk
build of clang.
>From [2], it looks like this will become version 3.3 (in about 5
months). So should we change the condition to:
#if defined(__GNUC__) && (!defined(__clang__) ||
__clang_major__ > 3 || \
(__clang__major == 3 && __clang_minor__ >= 3)
[2] http://llvm.org/docs/HowToReleaseLLVM.html
John
^ permalink raw reply
* Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
From: Antoine Pelisse @ 2013-01-17 10:32 UTC (permalink / raw)
To: John Keeping, Max Horn
Cc: git, Johannes Sixt, Junio C Hamano, Antoine Pelisse
In-Reply-To: <1358376443-7404-2-git-send-email-apelisse@gmail.com>
John, could you confirm that you trigger the -Wtautological-compare
warning with your version of clang ?
And that this patch makes clang compilation warning-free (with the
very latest clang) ?
Cheers,
On Wed, Jan 16, 2013 at 11:47 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
> sane and silent the clang warning.
>
> Clang warning happens because the enum is unsigned (this is
> implementation-defined, and there is no negative fields) and the check
> is then tautological.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> I tried to consider discussion [1] and this [2] discussion on clang's list
>
> With these two patches and the patch from Max Horne, I'm finally able to
> compile with CC=clang CFLAGS=-Werror.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/184908
> [2]: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html
>
> grep.c | 3 ++-
> grep.h | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 4bd1b8b..bb548ca 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
> for (p = opt->header_list; p; p = p->next) {
> if (p->token != GREP_PATTERN_HEAD)
> die("bug: a non-header pattern in grep header list.");
> - if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
> + if (p->field < GREP_HEADER_FIELD_MIN ||
> + GREP_HEADER_FIELD_MAX <= p->field)
> die("bug: unknown header field %d", p->field);
> compile_regexp(p, opt);
> }
> diff --git a/grep.h b/grep.h
> index 8fc854f..e4a1df5 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -28,7 +28,8 @@ enum grep_context {
> };
>
> enum grep_header_field {
> - GREP_HEADER_AUTHOR = 0,
> + GREP_HEADER_FIELD_MIN = 0,
> + GREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,
> GREP_HEADER_COMMITTER,
> GREP_HEADER_REFLOG,
>
> --
> 1.8.1.1.435.g20d29be.dirty
>
^ permalink raw reply
* Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
From: John Keeping @ 2013-01-17 11:00 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Max Horn, git, Johannes Sixt, Junio C Hamano
In-Reply-To: <CALWbr2wk+78zxGKCo-hCOwMuMOzdGspYvMu7PA6o0OYM3Y3m4A@mail.gmail.com>
On Thu, Jan 17, 2013 at 11:32:39AM +0100, Antoine Pelisse wrote:
> John, could you confirm that you trigger the -Wtautological-compare
> warning with your version of clang ?
Yes, the warning is still there with both 3.2 and a recent trunk build
but this patch squelches it.
> And that this patch makes clang compilation warning-free (with the
> very latest clang) ?
There is one remaining warning on pu which hasn't been discussed in this
thread as far as I can see. I'll send a patch shortly.
There's also a warning that triggers with clang 3.2 but not clang trunk, which
I think is a legitimate warning - perhaps someone who understands integer type
promotion better than me can explain why the code is OK (patch->score is
declared as 'int'):
builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
with expression of type 'int' is always false
[-Wtautological-constant-out-of-range-compare]
if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
> On Wed, Jan 16, 2013 at 11:47 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> > Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
> > sane and silent the clang warning.
> >
> > Clang warning happens because the enum is unsigned (this is
> > implementation-defined, and there is no negative fields) and the check
> > is then tautological.
> >
> > Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> > ---
> > I tried to consider discussion [1] and this [2] discussion on clang's list
> >
> > With these two patches and the patch from Max Horne, I'm finally able to
> > compile with CC=clang CFLAGS=-Werror.
> >
> > [1]: http://thread.gmane.org/gmane.comp.version-control.git/184908
> > [2]: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html
> >
> > grep.c | 3 ++-
> > grep.h | 3 ++-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/grep.c b/grep.c
> > index 4bd1b8b..bb548ca 100644
> > --- a/grep.c
> > +++ b/grep.c
> > @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
> > for (p = opt->header_list; p; p = p->next) {
> > if (p->token != GREP_PATTERN_HEAD)
> > die("bug: a non-header pattern in grep header list.");
> > - if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
> > + if (p->field < GREP_HEADER_FIELD_MIN ||
> > + GREP_HEADER_FIELD_MAX <= p->field)
> > die("bug: unknown header field %d", p->field);
> > compile_regexp(p, opt);
> > }
> > diff --git a/grep.h b/grep.h
> > index 8fc854f..e4a1df5 100644
> > --- a/grep.h
> > +++ b/grep.h
> > @@ -28,7 +28,8 @@ enum grep_context {
> > };
> >
> > enum grep_header_field {
> > - GREP_HEADER_AUTHOR = 0,
> > + GREP_HEADER_FIELD_MIN = 0,
> > + GREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,
> > GREP_HEADER_COMMITTER,
> > GREP_HEADER_REFLOG,
> >
> > --
> > 1.8.1.1.435.g20d29be.dirty
> >
^ permalink raw reply
* [PATCH] combine-diff: suppress a clang warning
From: John Keeping @ 2013-01-17 11:23 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Max Horn, git, Johannes Sixt, Junio C Hamano
In-Reply-To: <20130117110008.GD4574@serenity.lan>
When compiling combine-diff.c, clang 3.2 says:
combine-diff.c:1006:19: warning: adding 'int' to a string does not
append to the string [-Wstring-plus-int]
prefix = COLONS + offset;
~~~~~~~^~~~~~~~
combine-diff.c:1006:19: note: use array indexing to silence this warning
prefix = COLONS + offset;
^
& [ ]
Suppress this by making the suggested change.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Thu, Jan 17, 2013 at 11:00:08AM +0000, John Keeping wrote:
> On Thu, Jan 17, 2013 at 11:32:39AM +0100, Antoine Pelisse wrote:
> There is one remaining warning on pu which hasn't been discussed in this
> thread as far as I can see. I'll send a patch shortly.
... and here it is.
combine-diff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..dba4748 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
offset = strlen(COLONS) - num_parent;
if (offset < 0)
offset = 0;
- prefix = COLONS + offset;
+ prefix = &COLONS[offset];
/* Show the modes */
for (i = 0; i < num_parent; i++) {
--
1.8.1
^ permalink raw reply related
* Re: GIT get corrupted on lustre
From: Eric Chamberland @ 2013-01-17 13:07 UTC (permalink / raw)
To: git; +Cc: Maxime Boissonneault, Sébastien Boisvert
In-Reply-To: <50EDDF12.3080800@giref.ulaval.ca>
Hi!
I still have the corruption problems....
We just compiled a git without threads to try... (by the way,
--without-pthreads doesn't work, you have to do a --disable-pthreads
instead).
And to remove the warnings about threads at "git gc" execution, I did a:
git config --local pack.threads 1
and cloned a repository and started to do:
git gc
once every hour.
Then this night (at 05:35:02 exactly), the same error as usual occurred:
error: index file
.git/objects/pack/pack-bf0748cee64a1964be0a1061c82aca51c993b825.idx is
too small
error: refs/heads/master does not point to a valid object!
So now I am convinced that it is not a thread problem....
I am kind of discouraged, we like to use git, but in this case we have
this error which seems unsolvable!
Anyone has a new idea?
Thanks,
Eric
On 01/09/2013 04:20 PM, Eric Chamberland wrote:
> Hi Brian,
>
> On 01/08/2013 11:11 AM, Eric Chamberland wrote:
>> On 12/24/2012 10:11 AM, Brian J. Murrell wrote:
>>> Have you tried adding a "-q" to the git command line to quiet down git's
>>> "feedback" messages?
>>>
>>
>
> I moved to git 1.8.1 and added the "-q" to the command "git gc" but it
> occured to return an error, so the "-q" option is not avoiding the
> problem here... :-/
>
> command in crontab:
>
> cd /rap/jsf-051-aa/ericc/tests_git_clones/GIREF && for i in seq 10; do
> /software/apps/git/1.8.1/bin/git gc -q || true;done
>
> results:
> error: index file
> .git/objects/pack/pack-1f09879c88cd71a15dcc891713cf038d249830ad.idx is
> too small
> error: refs/remotes/origin/BIB_Branche_1_4_x does not point to a valid
> object!
>
> and this clone was a "clean" clone in which only "git qc -q" has been
> run on....
>
> I still have a doubt on threads....
>
> Eric
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Chris Rorvick @ 2013-01-17 13:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Max Horn, git, Angelo Borsotti, Drew Northup, Michael Haggerty,
Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
Felipe Contreras
In-Reply-To: <7vehhkuwg5.fsf@alter.siamese.dyndns.org>
On Thu, Jan 17, 2013 at 12:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Rorvick <chris@rorvick.com> writes:
>
>> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> It is fine when pushing into "refs/tags/" hierarchy. It is *NOT*
>>> OK if the type check does not satisfy this function. In that case,
>>> we do not actually see the existence of the destination as a
>>> problem, but it is reported as such. We are blocking because we do
>>> not like the type of the new object or the type of the old object.
>>> If the destination points at a commit, the push can succeed if the
>>> user changes what object to push, so saying "you cannot push because
>>> the destination already exists" is just wrong in such a case.
>>
>> So the solution is to revert back to recommending a merge?
>
> Of course not, because at that point you may not even have what you
> were attempting to overwrite. Nobody says it is even something you
> could merge.
I was referring to your concern about rejecting based on type. A push
causing a reference to move (for example) from a commit to a blob is
rejected as "already exists" with this patch. You emphatically state
this is not OK and your solution is to revert back to behavior that
advises a merge.
Clearly the bug regarding an 'old' unknown to the client should be
fixed. This is a obvious test case I should have covered and it's
unfortunate it made it into master. But I don't understand why
is_forwardable() was misguided (maybe poorly named) nor why
ref_newer() is a better place to solve the issues it was addressing.
Chris
^ permalink raw reply
* merge vs. rebase question
From: Dennis Putnam @ 2013-01-17 14:14 UTC (permalink / raw)
To: git@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 955 bytes --]
As a git noob I am having trouble understanding when to use which
commands. I have a repository (bare) on my Linux server. I also created
a build directory as a local repository. In my build script I do a 'git
pull' to make sure the build directory is up to date. No changes are
made to my source so this repository never does an 'add' or 'commit'.
When I run my script with 'pull', the output indicates that changes were
found and seems to have pulled them into the local directory. However,
when I look at the resulting source, none of the expected changes show
up. I then tried a 'fetch' and 'rebase'. That worked but I don't
understand why. I thought 'pull' did a 'fetch' and a 'merge' so I don't
understand why a 'fetch' and 'rebase' worked but 'fetch' and 'merge' did
not. Unless my understanding of what 'pull' does is wrong. In my case,
what should I be using in my script to assure that the build directory
is current?
Thanks.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]
^ permalink raw reply
* Re: GIT get corrupted on lustre
From: Philippe Vaucher @ 2013-01-17 14:23 UTC (permalink / raw)
To: Eric Chamberland
Cc: git@vger.kernel.org, Maxime Boissonneault,
Sébastien Boisvert
In-Reply-To: <50F7F793.80507@giref.ulaval.ca>
> Anyone has a new idea?
Did you try Jeff King's code to confirm his idea?
Philippe
^ permalink raw reply
* Re: GIT get corrupted on lustre
From: Eric Chamberland @ 2013-01-17 16:30 UTC (permalink / raw)
To: Philippe Vaucher
Cc: git@vger.kernel.org, Maxime Boissonneault,
Sébastien Boisvert
In-Reply-To: <CAGK7Mr4R=OwfWt4Kat75C8YDi3iLTavMLxeoLxkf1-gKhxrucg@mail.gmail.com>
On 01/17/2013 09:23 AM, Philippe Vaucher wrote:
>> Anyone has a new idea?
>
> Did you try Jeff King's code to confirm his idea?
>
> Philippe
>
Yes I did, but it was running without any problem....
I find that my test case is "simple" (fresh git clone then "git gc" in a
crontab), I bet anyone who has access to a Lustre filesystem can
reproduce the problem... The problem is to have such a filesystem to do
the tests....
But I am available to do it...
Thanks,
Eric
^ permalink raw reply
* RE: GIT get corrupted on lustre
From: Pyeron, Jason J CTR (US) @ 2013-01-17 16:40 UTC (permalink / raw)
To: Eric Chamberland, Philippe Vaucher
Cc: git@vger.kernel.org, Maxime Boissonneault,
Sébastien Boisvert
In-Reply-To: <50F8273E.5050803@giref.ulaval.ca>
[-- Attachment #1: Type: text/plain, Size: 728 bytes --]
> -----Original Message-----
> From: Eric Chamberland
> Sent: Thursday, January 17, 2013 11:31 AM
>
> On 01/17/2013 09:23 AM, Philippe Vaucher wrote:
> >> Anyone has a new idea?
> >
> > Did you try Jeff King's code to confirm his idea?
> >
> > Philippe
> >
>
> Yes I did, but it was running without any problem....
>
> I find that my test case is "simple" (fresh git clone then "git gc" in
> a
> crontab), I bet anyone who has access to a Lustre filesystem can
> reproduce the problem... The problem is to have such a filesystem to
> do
> the tests....
Stabbing in the dark, but can you log the details with ProcessMon?
http://technet.microsoft.com/en-us/sysinternals/bb896645
>
> But I am available to do it...
-Jason
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5615 bytes --]
^ permalink raw reply
* Re: GIT get corrupted on lustre
From: Maxime Boissonneault @ 2013-01-17 16:41 UTC (permalink / raw)
To: Pyeron, Jason J CTR (US)
Cc: Eric Chamberland, Philippe Vaucher, git@vger.kernel.org,
Sébastien Boisvert
In-Reply-To: <871B6C10EBEFE342A772D1159D1320853A042AD7@umechphj.easf.csd.disa.mil>
I don't know of any lustre filesystem that is used on Windows. Barely
anybody uses Windows in the HPC industry.
This is a Linux cluster.
Maxime Boissonneault
Le 2013-01-17 11:40, Pyeron, Jason J CTR (US) a écrit :
>> -----Original Message-----
>> From: Eric Chamberland
>> Sent: Thursday, January 17, 2013 11:31 AM
>>
>> On 01/17/2013 09:23 AM, Philippe Vaucher wrote:
>>>> Anyone has a new idea?
>>> Did you try Jeff King's code to confirm his idea?
>>>
>>> Philippe
>>>
>> Yes I did, but it was running without any problem....
>>
>> I find that my test case is "simple" (fresh git clone then "git gc" in
>> a
>> crontab), I bet anyone who has access to a Lustre filesystem can
>> reproduce the problem... The problem is to have such a filesystem to
>> do
>> the tests....
> Stabbing in the dark, but can you log the details with ProcessMon?
>
> http://technet.microsoft.com/en-us/sysinternals/bb896645
>
>> But I am available to do it...
> -Jason
--
---------------------------------
Maxime Boissonneault
Analyste de calcul - Calcul Québec, Université Laval
Ph. D. en physique
^ permalink raw reply
* Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
From: Linus Torvalds @ 2013-01-17 16:44 UTC (permalink / raw)
To: John Keeping
Cc: Antoine Pelisse, Max Horn, git, Johannes Sixt, Junio C Hamano
In-Reply-To: <20130117110008.GD4574@serenity.lan>
On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@keeping.me.uk> wrote:
>
> There's also a warning that triggers with clang 3.2 but not clang trunk, which
> I think is a legitimate warning - perhaps someone who understands integer type
> promotion better than me can explain why the code is OK (patch->score is
> declared as 'int'):
>
> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
> with expression of type 'int' is always false
> [-Wtautological-constant-out-of-range-compare]
> if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
The warning seems to be very very wrong, and implies that clang has
some nasty bug in it.
Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the
conversion rules for the comparison is that the int result from the
assignment is cast to unsigned long. And if you cast (int)-1 to
unsigned long, you *do* get ULONG_MAX. That's true regardless of
whether "long" has the same number of bits as "int" or is bigger. The
implicit cast will be done as a sign-extension (unsigned long is not
signed, but the source type of 'int' *is* signed, and that is what
determines the sign extension on casting).
So the "is always false" is pure and utter crap. clang is wrong, and
it is wrong in a way that implies that it actually generates incorrect
code. It may well be worth making a clang bug report about this.
That said, clang is certainly understandably confused. The code
depends on subtle conversion rules and bit patterns, and is clearly
very confusingly written.
So it would probably be good to rewrite it as
unsigned long val = strtoul(line, NULL, 10);
if (val == ULONG_MAX) ..
patch->score = val;
instead. At which point you might as well make the comparison be ">=
INT_MAX" instead, since anything bigger than that is going to be
bogus.
So the git code is probably worth cleaning up, but for git it would be
a cleanup. For clang, this implies a major bug and bad code
generation.
Linus
Linus
^ permalink raw reply
* Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
From: Antoine Pelisse @ 2013-01-17 16:56 UTC (permalink / raw)
To: John Keeping, Linus Torvalds; +Cc: Max Horn, git, Johannes Sixt, Junio C Hamano
In-Reply-To: <CA+55aFxYSX2iYPSafKdCDSfWSMfQxP3R3Hqh8GuiiR6EbWfk3w@mail.gmail.com>
BTW, I think it has been addressed [1] by clang already and that would
explain why you don't have the warning when using clang trunk version.
[1]: http://llvm-reviews.chandlerc.com/D113
Antoine,
On Thu, Jan 17, 2013 at 5:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@keeping.me.uk> wrote:
>>
>> There's also a warning that triggers with clang 3.2 but not clang trunk, which
>> I think is a legitimate warning - perhaps someone who understands integer type
>> promotion better than me can explain why the code is OK (patch->score is
>> declared as 'int'):
>>
>> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
>> with expression of type 'int' is always false
>> [-Wtautological-constant-out-of-range-compare]
>> if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> The warning seems to be very very wrong, and implies that clang has
> some nasty bug in it.
>
> Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the
> conversion rules for the comparison is that the int result from the
> assignment is cast to unsigned long. And if you cast (int)-1 to
> unsigned long, you *do* get ULONG_MAX. That's true regardless of
> whether "long" has the same number of bits as "int" or is bigger. The
> implicit cast will be done as a sign-extension (unsigned long is not
> signed, but the source type of 'int' *is* signed, and that is what
> determines the sign extension on casting).
>
> So the "is always false" is pure and utter crap. clang is wrong, and
> it is wrong in a way that implies that it actually generates incorrect
> code. It may well be worth making a clang bug report about this.
>
> That said, clang is certainly understandably confused. The code
> depends on subtle conversion rules and bit patterns, and is clearly
> very confusingly written.
>
> So it would probably be good to rewrite it as
>
> unsigned long val = strtoul(line, NULL, 10);
> if (val == ULONG_MAX) ..
> patch->score = val;
>
> instead. At which point you might as well make the comparison be ">=
> INT_MAX" instead, since anything bigger than that is going to be
> bogus.
>
> So the git code is probably worth cleaning up, but for git it would be
> a cleanup. For clang, this implies a major bug and bad code
> generation.
>
> Linus
> Linus
^ permalink raw reply
* Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
From: John Keeping @ 2013-01-17 17:02 UTC (permalink / raw)
To: Linus Torvalds
Cc: Antoine Pelisse, Max Horn, git, Johannes Sixt, Junio C Hamano
In-Reply-To: <CA+55aFxYSX2iYPSafKdCDSfWSMfQxP3R3Hqh8GuiiR6EbWfk3w@mail.gmail.com>
On Thu, Jan 17, 2013 at 08:44:20AM -0800, Linus Torvalds wrote:
> On Thu, Jan 17, 2013 at 3:00 AM, John Keeping <john@keeping.me.uk> wrote:
>>
>> There's also a warning that triggers with clang 3.2 but not clang trunk, which
>> I think is a legitimate warning - perhaps someone who understands integer type
>> promotion better than me can explain why the code is OK (patch->score is
>> declared as 'int'):
>>
>> builtin/apply.c:1044:47: warning: comparison of constant 18446744073709551615
>> with expression of type 'int' is always false
>> [-Wtautological-constant-out-of-range-compare]
>> if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> The warning seems to be very very wrong, and implies that clang has
> some nasty bug in it.
>
> Since patch->score is 'int', and UNLONG_MAX is 'unsigned long', the
> conversion rules for the comparison is that the int result from the
> assignment is cast to unsigned long. And if you cast (int)-1 to
> unsigned long, you *do* get ULONG_MAX. That's true regardless of
> whether "long" has the same number of bits as "int" or is bigger. The
> implicit cast will be done as a sign-extension (unsigned long is not
> signed, but the source type of 'int' *is* signed, and that is what
> determines the sign extension on casting).
>
> So the "is always false" is pure and utter crap. clang is wrong, and
> it is wrong in a way that implies that it actually generates incorrect
> code. It may well be worth making a clang bug report about this.
The warning doesn't occur with a build from their trunk so it looks like
it's already fixed - it just won't make into into a release for about 5
months going by their timeline.
Thanks for the clear explanation.
^ permalink raw reply
* RE: GIT get corrupted on lustre
From: Pyeron, Jason J CTR (US) @ 2013-01-17 17:17 UTC (permalink / raw)
To: Maxime Boissonneault
Cc: Eric Chamberland, Philippe Vaucher, git@vger.kernel.org,
Sébastien Boisvert
In-Reply-To: <50F829A9.7090606@calculquebec.ca>
[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]
Sorry, I am in cygwin mode, and I had crossed wires in my head. s/ProcessMon/strace/
> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Maxime Boissonneault
> Sent: Thursday, January 17, 2013 11:41 AM
> To: Pyeron, Jason J CTR (US)
> Cc: Eric Chamberland; Philippe Vaucher; git@vger.kernel.org; Sébastien
> Boisvert
> Subject: Re: GIT get corrupted on lustre
>
> I don't know of any lustre filesystem that is used on Windows. Barely
> anybody uses Windows in the HPC industry.
> This is a Linux cluster.
>
> Maxime Boissonneault
>
> Le 2013-01-17 11:40, Pyeron, Jason J CTR (US) a écrit :
> >> -----Original Message-----
> >> From: Eric Chamberland
> >> Sent: Thursday, January 17, 2013 11:31 AM
> >>
> >> On 01/17/2013 09:23 AM, Philippe Vaucher wrote:
> >>>> Anyone has a new idea?
> >>> Did you try Jeff King's code to confirm his idea?
> >>>
> >>> Philippe
> >>>
> >> Yes I did, but it was running without any problem....
> >>
> >> I find that my test case is "simple" (fresh git clone then "git gc"
> in
> >> a
> >> crontab), I bet anyone who has access to a Lustre filesystem can
> >> reproduce the problem... The problem is to have such a filesystem
> to
> >> do
> >> the tests....
> > Stabbing in the dark, but can you log the details with ProcessMon?
> >
> > http://technet.microsoft.com/en-us/sysinternals/bb896645
> >
> >> But I am available to do it...
> > -Jason
>
>
> --
> ---------------------------------
> Maxime Boissonneault
> Analyste de calcul - Calcul Québec, Université Laval
> Ph. D. en physique
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5615 bytes --]
^ permalink raw reply
* [PATCH v2 0/8] Initial Python 3 support
From: John Keeping @ 2013-01-17 18:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, Michael Haggerty, git, Pete Wyckoff
In-Reply-To: <cover.1358018078.git.john@keeping.me.uk>
This series does enough so that everything except git-p4 runs under
Python 3.
As discussed with Pete, it may not make sense to change git-p4 to
support Python 3 until Perforce's Python output mode is changed. So
does it make sense to merge this now and say "use Python 2 if you want
git-p4"?
Changes since v1:
* rebased on master after fc/remote-testgit-feature-done was merged,
leading to an extra change in patch 8 (git-remote-testpy: call print
as a function)
* changed patch 2 (git_remote_helpers: fix input when running under
Python 3) to treat ref names as byte strings
John Keeping (8):
git_remote_helpers: Allow building with Python 3
git_remote_helpers: fix input when running under Python 3
git_remote_helpers: Force rebuild if python version changes
git_remote_helpers: Use 2to3 if building with Python 3
svn-fe: allow svnrdump_sim.py to run with Python 3
git-remote-testpy: hash bytes explicitly
git-remote-testpy: don't do unbuffered text I/O
git-remote-testpy: call print as a function
contrib/svn-fe/svnrdump_sim.py | 4 ++--
git-remote-testpy.py | 42 +++++++++++++++++++-------------------
git_remote_helpers/.gitignore | 1 +
git_remote_helpers/Makefile | 10 +++++++--
git_remote_helpers/git/importer.py | 9 +++++---
git_remote_helpers/setup.py | 10 +++++++++
6 files changed, 48 insertions(+), 28 deletions(-)
--
1.8.1.1.260.g99b33f4.dirty
^ permalink raw reply
* [PATCH v2 1/8] git_remote_helpers: allow building with Python 3
From: John Keeping @ 2013-01-17 18:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, Michael Haggerty, git, Pete Wyckoff
In-Reply-To: <cover.1358448207.git.john@keeping.me.uk>
Change inline Python to call "print" as a function not a statement.
This is harmless because Python 2 will see the parentheses as redundant
grouping but they are necessary to run this code with Python 3.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git_remote_helpers/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git_remote_helpers/Makefile b/git_remote_helpers/Makefile
index 74b05dc..f65f064 100644
--- a/git_remote_helpers/Makefile
+++ b/git_remote_helpers/Makefile
@@ -23,7 +23,7 @@ endif
PYLIBDIR=$(shell $(PYTHON_PATH) -c \
"import sys; \
- print 'lib/python%i.%i/site-packages' % sys.version_info[:2]")
+ print('lib/python%i.%i/site-packages' % sys.version_info[:2])")
all: $(pysetupfile)
$(QUIET)$(PYTHON_PATH) $(pysetupfile) $(QUIETSETUP) build
--
1.8.1.1.260.g99b33f4.dirty
^ permalink raw reply related
* [PATCH v2 2/8] git_remote_helpers: fix input when running under Python 3
From: John Keeping @ 2013-01-17 18:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, Michael Haggerty, git, Pete Wyckoff
In-Reply-To: <cover.1358448207.git.john@keeping.me.uk>
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>
---
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..d3f90e1 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('ascii'))
refs = {}
for line in lines:
- value, name = line.split(' ')
- name = name.strip('commit\t')
+ value, name = line.split(' '.encode('ascii'))
+ name = name.strip('commit\t'.encode('ascii'))
refs[name] = value
return refs
--
1.8.1.1.260.g99b33f4.dirty
^ permalink raw reply related
* [PATCH v2 3/8] git_remote_helpers: force rebuild if python version changes
From: John Keeping @ 2013-01-17 18:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, Michael Haggerty, git, Pete Wyckoff
In-Reply-To: <cover.1358448207.git.john@keeping.me.uk>
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.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git_remote_helpers/.gitignore | 1 +
git_remote_helpers/Makefile | 8 +++++++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/git_remote_helpers/.gitignore b/git_remote_helpers/.gitignore
index 2247d5f..06c664f 100644
--- a/git_remote_helpers/.gitignore
+++ b/git_remote_helpers/.gitignore
@@ -1,2 +1,3 @@
+/GIT-PYTHON_VERSION
/build
/dist
diff --git a/git_remote_helpers/Makefile b/git_remote_helpers/Makefile
index f65f064..91f458f 100644
--- a/git_remote_helpers/Makefile
+++ b/git_remote_helpers/Makefile
@@ -25,8 +25,14 @@ PYLIBDIR=$(shell $(PYTHON_PATH) -c \
"import sys; \
print('lib/python%i.%i/site-packages' % sys.version_info[:2])")
+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
install: $(pysetupfile)
$(PYTHON_PATH) $(pysetupfile) install --prefix $(DESTDIR_SQ)$(prefix)
--
1.8.1.1.260.g99b33f4.dirty
^ permalink raw reply related
* [PATCH v2 4/8] git_remote_helpers: use 2to3 if building with Python 3
From: John Keeping @ 2013-01-17 18:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, Michael Haggerty, git, Pete Wyckoff
In-Reply-To: <cover.1358448207.git.john@keeping.me.uk>
Using the approach detailed on the Python wiki[1], run 2to3 on the code
as part of the build if building with Python 3.
The code itself requires no changes to convert cleanly.
[1] http://wiki.python.org/moin/PortingPythonToPy3k
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git_remote_helpers/setup.py | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/git_remote_helpers/setup.py b/git_remote_helpers/setup.py
index 4d434b6..6de41de 100644
--- a/git_remote_helpers/setup.py
+++ b/git_remote_helpers/setup.py
@@ -4,6 +4,15 @@
from distutils.core import setup
+# 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
+# Python3.
+try:
+ from distutils.command.build_py import build_py_2to3 as build_py
+except ImportError:
+ # 2.x
+ from distutils.command.build_py import build_py
+
setup(
name = 'git_remote_helpers',
version = '0.1.0',
@@ -14,4 +23,5 @@ setup(
url = 'http://www.git-scm.com/',
package_dir = {'git_remote_helpers': ''},
packages = ['git_remote_helpers', 'git_remote_helpers.git'],
+ cmdclass = {'build_py': build_py},
)
--
1.8.1.1.260.g99b33f4.dirty
^ permalink raw reply related
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