* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Michael J Gruber @ 2013-01-16 16:24 UTC (permalink / raw)
To: Phil Hord
Cc: Junio C Hamano, Jardel Weyrich, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <CABURp0rR_wB6vcjrZajQU_=AVVvBq-aTGpggh5XxdCMYis3-ag@mail.gmail.com>
Phil Hord venit, vidit, dixit 16.01.2013 17:15:
> On Tue, Jan 15, 2013 at 10:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>
>>> That being said, I don't mind changing the behaviour of set-url.
>>
>> I do not think we want to change the behaviour of set-url.
>
> I agree with Michael that changing the set-url behavior would be
> appropriate here. If I say "--add" this pushUrl, don't I mean to
> create an additional url which is pushed to?
I said I wouldn't mind, I didn't vote for it.
> I agree that it makes the config situation messy; this is currently a
> "clean" sequence, in that it leaves the config unchanged after both
> steps are completed:
>
> git remote set-url --add --push origin /tmp/foo
> git remote set-url --delete --push origin /tmp/foo
>
> If the behavior is changed like Michael suggested, it would not leave
> the config clean (unless heroic steps were taken to keep track). But
> I'm not sure that's such a bad thing. In simple command sequences,
> the results would be clean and the only behavior change is that the
> initial "--add" really acts like "add" and not "replace". But more
> complex sequences could be devised which were affected by this change.
>
> I'm curious, Junio. Do you think the set-url behavior is correct
> as-is, or that changing it will cause breakage for some workflows, or
> that it complicates the operation too much for people who are already
> used to the config layout?
For "set url --add --push" on top of a push url only being defaulted
from a fetch url, both behaviours (replace or add, i.e. current or new)
make sense to me. So the questions are:
- Is it worth and possible changing?
- How to best describe it in "remote -v" and "remote show" output?
My patch answered to "no" to the first question and answers the second
one in cases where (push)insteadof is not used to transform one fetch
config into two different urls for fetch and push. I think :)
Michael
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 16:36 UTC (permalink / raw)
To: Max Horn
Cc: Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Jeff King, Felipe Contreras
In-Reply-To: <DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de>
Max Horn <max@quendi.de> writes:
> But with next, I get this:
>
>
> ! [rejected] master -> master (already exists)
> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'
> hint: Updates were rejected because the destination reference already exists
> hint: in the remote.
>
> This looks like a regression to me.
It is an outright bug. The new helper function is_forwrdable() is
bogus to assume that both original and updated objects can be
locally inspected, but you do not necessarily have the original
locally.
remote.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/remote.c b/remote.c
index aa6b719..4a253ef 100644
--- a/remote.c
+++ b/remote.c
@@ -1286,9 +1286,12 @@ static inline int is_forwardable(struct ref* ref)
if (!prefixcmp(ref->name, "refs/tags/"))
return 0;
- /* old object must be a commit */
+ /*
+ * old object must be a commit, but we may be forcing
+ * without having it in the first place!
+ */
o = parse_object(ref->old_sha1);
- if (!o || o->type != OBJ_COMMIT)
+ if (o && o->type != OBJ_COMMIT)
return 0;
/* new object must be commit-ish */
^ permalink raw reply related
* Re: [PATCH] fix some clang warnings
From: Jeff King @ 2013-01-16 16:04 UTC (permalink / raw)
To: Max Horn; +Cc: git, Johannes Sixt
In-Reply-To: <1358348003-11130-1-git-send-email-max@quendi.de>
On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:
> -#ifdef __GNUC__
> +#if defined(__GNUC__) && ! defined(__clang__)
> #define config_error_nonbool(s) (config_error_nonbool(s), -1)
> #endif
You don't say what the warning is, but I'm guessing it's complaining
about throwing away the return value from config_error_nonbool?
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 16:48 UTC (permalink / raw)
To: Max Horn
Cc: Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Jeff King, Felipe Contreras
In-Reply-To: <7vsj61xez2.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Max Horn <max@quendi.de> writes:
>
>> But with next, I get this:
>>
>> ! [rejected] master -> master (already exists)
>> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'
>> hint: Updates were rejected because the destination reference already exists
>> hint: in the remote.
>>
>> This looks like a regression to me.
>
> It is an outright bug. The new helper function is_forwrdable() is
> bogus to assume that both original and updated objects can be
> locally inspected, but you do not necessarily have the original
> locally.
The way the caller uses the result of this function is equally
questionable. If this function says "we do not want to let this
push go through", it translates that unconditionally into "we
blocked it because the destination already exists".
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.
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: Junio C Hamano @ 2013-01-16 16:53 UTC (permalink / raw)
To: Jeff King; +Cc: Max Horn, git, Johannes Sixt
In-Reply-To: <20130116160410.GC22400@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:
>
>> -#ifdef __GNUC__
>> +#if defined(__GNUC__) && ! defined(__clang__)
>> #define config_error_nonbool(s) (config_error_nonbool(s), -1)
>> #endif
>
> You don't say what the warning is, but I'm guessing it's complaining
> about throwing away the return value from config_error_nonbool?
Yeah, I was wondering about the same thing. The other one looks
similar, ignoring the return value of error().
Also, is this "some versions of clang do not like this"? Or are all
versions of clang affected?
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Jeff King @ 2013-01-16 16:01 UTC (permalink / raw)
To: Max Horn
Cc: Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras, Junio C Hamano
In-Reply-To: <DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de>
On Wed, Jan 16, 2013 at 02:32:03PM +0100, Max Horn wrote:
> With git 1.8.1, I get this message:
>
> ! [rejected] master -> master (non-fast-forward)
> [...]
> But with next, I get this:
>
> ! [rejected] master -> master (already exists)
Thanks for the detailed report. I was able to reproduce easily here.
The problem is the logic in is_forwardable:
static inline int is_forwardable(struct ref* ref)
{
struct object *o;
if (!prefixcmp(ref->name, "refs/tags/"))
return 0;
/* old object must be a commit */
o = parse_object(ref->old_sha1);
if (!o || o->type != OBJ_COMMIT)
return 0;
/* new object must be commit-ish */
o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
if (!o || o->type != OBJ_COMMIT)
return 0;
return 1;
}
The intent is to allow fast-forward only between objects that both point
to commits eventually. But we are doing this check on the client, which
does not necessarily have the object for ref->old_sha1 at all. So it
cannot know the type, and cannot enforce this condition accurately.
I.e., we trigger the "!o" branch after the parse_object in your example.
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 16:00 UTC (permalink / raw)
To: Max Horn
Cc: Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Jeff King, Felipe Contreras
In-Reply-To: <DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de>
Max Horn <max@quendi.de> writes:
> But with next, I get this:
>
> ! [rejected] master -> master (already exists)
> error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
> hint: Updates were rejected because the destination reference already exists
> hint: in the remote.
>
> This looks like a regression to me.
It is in master now X-<, and this looks like a bug to me.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 17:10 UTC (permalink / raw)
To: Jeff King
Cc: Max Horn, Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <20130116160131.GB22400@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I.e., we trigger the "!o" branch after the parse_object in your example.
Heh, I didn't see this message until now (gmane seems to be lagging
a bit).
I am very tempted to do this.
* Remove unnecessary not_forwardable from "struct ref"; it is only
used inside set_ref_status_for_push();
* "refs/tags/" is the only hierarchy that cannot be replaced
without --force;
* Remove the misguided attempt to force that everything that
updates an existing ref has to be a commit outside "refs/tags/"
hierarchy. This code does not know what kind of objects the user
wants to place in "refs/frotz/" hierarchy it knows nothing about.
I feel moderately strongly about the last point. Defining special
semantics for one hierarchy (e.g. "refs/tags/") and implementing a
policy for enforcement is one thing, but a random policy that
depends on object type that applies globally is simply insane. The
user may want to do "refs/tested/" hierarchy that is meant to hold
references to commit, with one annotated tag "refs/tested/latest"
that points at the "latest tested version" with some commentary, and
maintain the latter by keep pushing to it. If that is the semantics
the user wanted to ahve in the "refs/tested/" hierarchy, it is not
reasonable to require --force for such a workflow. The user knows
better than Git in such a case.
cache.h | 1 -
remote.c | 24 +-----------------------
t/t5516-fetch-push.sh | 21 ---------------------
3 files changed, 1 insertion(+), 45 deletions(-)
diff --git a/cache.h b/cache.h
index a32a0ea..a942bbd 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,7 +1004,6 @@ struct ref {
requires_force:1,
merge:1,
nonfastforward:1,
- not_forwardable:1,
update:1,
deletion:1;
enum {
diff --git a/remote.c b/remote.c
index aa6b719..2c747c4 100644
--- a/remote.c
+++ b/remote.c
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
return 0;
}
-static inline int is_forwardable(struct ref* ref)
-{
- struct object *o;
-
- if (!prefixcmp(ref->name, "refs/tags/"))
- return 0;
-
- /* old object must be a commit */
- o = parse_object(ref->old_sha1);
- if (!o || o->type != OBJ_COMMIT)
- return 0;
-
- /* new object must be commit-ish */
- o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
- if (!o || o->type != OBJ_COMMIT)
- return 0;
-
- return 1;
-}
-
void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
int force_update)
{
@@ -1344,8 +1324,6 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* passing the --force argument
*/
- ref->not_forwardable = !is_forwardable(ref);
-
ref->update =
!ref->deletion &&
!is_null_sha1(ref->old_sha1);
@@ -1355,7 +1333,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
!has_sha1_file(ref->old_sha1)
|| !ref_newer(ref->new_sha1, ref->old_sha1);
- if (ref->not_forwardable) {
+ if (!prefixcmp(ref->name, "refs/tags/")) {
ref->requires_force = 1;
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6009372..8f024a0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -950,27 +950,6 @@ test_expect_success 'push requires --force to update lightweight tag' '
)
'
-test_expect_success 'push requires --force to update annotated tag' '
- mk_test heads/master &&
- mk_child child1 &&
- mk_child child2 &&
- (
- cd child1 &&
- git tag -a -m "message 1" Tag &&
- git push ../child2 Tag:refs/tmp/Tag &&
- git push ../child2 Tag:refs/tmp/Tag &&
- >file1 &&
- git add file1 &&
- git commit -m "file1" &&
- git tag -f -a -m "message 2" Tag &&
- test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
- git push --force ../child2 Tag:refs/tmp/Tag &&
- git tag -f -a -m "message 3" Tag HEAD~ &&
- test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
- git push --force ../child2 Tag:refs/tmp/Tag
- )
-'
-
test_expect_success 'push --porcelain' '
mk_empty &&
echo >.git/foo "To testrepo" &&
^ permalink raw reply related
* Re: [PATCH] fix some clang warnings
From: Antoine Pelisse @ 2013-01-16 17:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Max Horn, git, Johannes Sixt
In-Reply-To: <7vk3rdxe5y.fsf@alter.siamese.dyndns.org>
FWIW, I also happen to have the warning:
advice.c:69:2: warning: expression result unused [-Wunused-value]
error("'%s' is not possible because you have unmerged files.", me);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./git-compat-util.h:314:55: note: expanded from:
#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
^~
with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
(based on LLVM 3.0)
I can't say about other versions.
On Wed, Jan 16, 2013 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:
>>
>>> -#ifdef __GNUC__
>>> +#if defined(__GNUC__) && ! defined(__clang__)
>>> #define config_error_nonbool(s) (config_error_nonbool(s), -1)
>>> #endif
>>
>> You don't say what the warning is, but I'm guessing it's complaining
>> about throwing away the return value from config_error_nonbool?
>
> Yeah, I was wondering about the same thing. The other one looks
> similar, ignoring the return value of error().
>
> Also, is this "some versions of clang do not like this"? Or are all
> versions of clang affected?
>
> --
> 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] fix some clang warnings
From: John Keeping @ 2013-01-16 17:18 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Junio C Hamano, Jeff King, Max Horn, git, Johannes Sixt
In-Reply-To: <CALWbr2z4TiynwOR3Lk4005dbZaLtcHK3J01ZF73wp8Q7Rm6YBA@mail.gmail.com>
On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
> FWIW, I also happen to have the warning:
>
> advice.c:69:2: warning: expression result unused [-Wunused-value]
> error("'%s' is not possible because you have unmerged files.", me);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./git-compat-util.h:314:55: note: expanded from:
> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
> ^~
>
> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
> (based on LLVM 3.0)
I have the same output with:
clang version 3.2 (tags/RELEASE_32/final)
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: Max Horn @ 2013-01-16 17:26 UTC (permalink / raw)
To: John Keeping
Cc: Antoine Pelisse, Junio C Hamano, Jeff King, git, Johannes Sixt
In-Reply-To: <20130116171809.GA2476@farnsworth.metanate.com>
On 16.01.2013, at 18:18, John Keeping wrote:
> On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
>> FWIW, I also happen to have the warning:
>>
>> advice.c:69:2: warning: expression result unused [-Wunused-value]
>> error("'%s' is not possible because you have unmerged files.", me);
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ./git-compat-util.h:314:55: note: expanded from:
>> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
>> ^~
>>
>> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
>> (based on LLVM 3.0)
>
> I have the same output with:
>
> clang version 3.2 (tags/RELEASE_32/final)
Sorry for not being more specific in my message. I have this with
Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)
Max
^ permalink raw reply
* Re: [PATCH] Add Auto-Submitted header to post-receive-email
From: Chris Hiestand @ 2013-01-16 17:29 UTC (permalink / raw)
To: Andy Parkins, Junio C Hamano; +Cc: git
In-Reply-To: <7v392b8fv3.fsf@alter.siamese.dyndns.org>
Andy, do you have any thoughts on adding this email header to
contrib/hooks/post-receive-email? This patch shouldn't cause problems for anyone
with a sanely configured mail delivery agent, and the additional header is very
useful in toggling auto responses.
This conforms to RFC3834 and is useful in preventing eg
vacation auto-responders from replying by default
Signed-off-by: Chris Hiestand <chiestand@salk.edu>
---
contrib/hooks/post-receive-email | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index b2171a0..0e5b72d 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -237,6 +237,7 @@ generate_email_header()
X-Git-Reftype: $refname_type
X-Git-Oldrev: $oldrev
X-Git-Newrev: $newrev
+ Auto-Submitted: auto-generated
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
--
1.7.10.4
On Sep 21, 2012, at 10:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Hiestand <chiestand@salk.edu> writes:
>
>> My email in April went unanswered so I'm resending it. An Auto-Submitted header
>> would be an improvement to the standard [git] post receive email.
>>
>> Thanks,
>> Chris
>>
>>
>> Begin forwarded message:
>>
>>> From: Chris Hiestand <chiestand@salk.edu>
>>> Subject: [PATCH] Add Auto-Submitted header to post-receive-email
>>> Date: April 14, 2012 6:15:10 PM PDT
>>> To: git@vger.kernel.org, gitster@pobox.com
>>>
>>> Hi,
>>>
>>> I think the Auto-Submitted header is a useful hook mail header to include by default.
>>>
>>> This conforms to RFC3834 and is useful in preventing e.g. vacation auto-responders
>>> from replying by default.
>>>
>>> Perhaps you have already considered this and decided not to include it, but I found
>>> no record of such a conversation on this list.
>
> I think the lack of response is generally lack of interest, and the
> primary reason for that was because the To/Cc list did not contain
> anybody who touched this particular file in the past (and no, I am
> not among them; as contrib/README says, I am often the wrong person
> to ask if a patch to contrib/ material makes sense).
>
>>> From 358fc3ae1ebfd7723d54e4033d3e9a9a0322c873 Mon Sep 17 00:00:00 2001
>>> From: Chris Hiestand <chiestand@salk.edu>
>>> Date: Sat, 14 Apr 2012 17:58:39 -0700
>>> Subject: [PATCH] Add Auto-Submitted header to post-receive-email
>
> These four lines should not be in the body of the e-mail message
> (see Documentation/SubmittingPatches).
>
>>> Adds Auto-Submitted: auto-generated to post-receive-email header
>>> This conforms to RFC3834 and is useful in preventing eg
>>> vacation auto-responders from replying by default
>>> ---
>>> contrib/hooks/post-receive-email | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> Even for contrib/ material, please always sign-off your patch (see
> Documentation/SubmittingPatches).
>
>>>
>>> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
>>> index 01af9df..282507c 100755
>>> --- a/contrib/hooks/post-receive-email
>>> +++ b/contrib/hooks/post-receive-email
>>> @@ -237,6 +237,7 @@ generate_email_header()
>>> X-Git-Reftype: $refname_type
>>> X-Git-Oldrev: $oldrev
>>> X-Git-Newrev: $newrev
>>> + Auto-Submitted: auto-generated
>>>
>>> This is an automated email from the git hooks/post-receive script. It was
>>> generated because a ref change was pushed to the repository containing
>
> I think the choice of "auto-generated" is a sensible one, as
> responding to a 'push' is like triggered by 'cron'.
>
> I'd however appreciate comments from people who either worked on
> this code or list regulars who actually use this code in the
> production.
>
> Thanks.
> --
> 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 related
* Re: [PATCH v3 2/3] config: Introduce diff.algorithm variable
From: Junio C Hamano @ 2013-01-16 17:42 UTC (permalink / raw)
To: Michal Privoznik; +Cc: git, trast, peff
In-Reply-To: <4e2aacd5bbf005f0e372589bf423a8cbd776bc6d.1358322212.git.mprivozn@redhat.com>
Will replace the one in 'pu' with this round. Looking good.
Thanks.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Jeff King @ 2013-01-16 17:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: Max Horn, Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <7vfw21xde5.fsf@alter.siamese.dyndns.org>
On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I.e., we trigger the "!o" branch after the parse_object in your example.
>
> Heh, I didn't see this message until now (gmane seems to be lagging
> a bit).
I think it is vger lagging, actually.
> I am very tempted to do this.
>
> * Remove unnecessary not_forwardable from "struct ref"; it is only
> used inside set_ref_status_for_push();
>
> * "refs/tags/" is the only hierarchy that cannot be replaced
> without --force;
Agreed.
> * Remove the misguided attempt to force that everything that
> updates an existing ref has to be a commit outside "refs/tags/"
> hierarchy. This code does not know what kind of objects the user
> wants to place in "refs/frotz/" hierarchy it knows nothing about.
I agree with what your patch does, but my thinking is a bit different.
My original suggestion with respect to object types was that the rule
for --force should be "do not ever lose any objects without --force". So
a fast-forward is OK, as the new objects reference the old. A non-fast
forward is not, because objects become unreferenced. Replacing a tag
object is not OK, even if it points to the same commit, as you are
losing the old tag object (replacing an object with a tag that points to
the original object or its descendent is OK in theory, though I doubt it
is common enough to worry about).
I think that is a reasonable rule that could be applied across all parts
of the namespace hierarchy. And it could be applied by the client,
because all you need to know is whether ref->old_sha1 is reachable from
ref->new_sha1.
But it is somewhat orthogonal to the "already exists" idea, and checking
refs/tags/. Those ideas are about enforcing sane rules on the tag
hierarchy. My rule is a safety valve that is meant to extend the idea of
"is fast-forwardable" to non-commit object types. If we do it at all, it
should be part of the fast-forward check (e.g., as part of ref_newer).
The current code conflates the two under the "already exists" condition,
which is just wrong. I think the best thing at this point is to split
the two ideas apart, keep the refs/tags check (and translate it to
"already exists" in the UI, as we do), and table the safety valve. I am
not even sure if it is something that is useful, and it can come later
if we decide it is.
> I feel moderately strongly about the last point. Defining special
> semantics for one hierarchy (e.g. "refs/tags/") and implementing a
> policy for enforcement is one thing, but a random policy that
> depends on object type that applies globally is simply insane. The
> user may want to do "refs/tested/" hierarchy that is meant to hold
> references to commit, with one annotated tag "refs/tested/latest"
> that points at the "latest tested version" with some commentary, and
> maintain the latter by keep pushing to it. If that is the semantics
> the user wanted to ahve in the "refs/tested/" hierarchy, it is not
> reasonable to require --force for such a workflow. The user knows
> better than Git in such a case.
I see what you are saying, but I think the ship has already sailed to
some degree. We already implement the non-fast-forward check everywhere,
and I cannot have a "refs/tested" hierarchy that pushes arbitrary
commits without regard to their history. If I have such a hierarchy, I
have to use "--force" (or more likely, mark the refspec with "+").
In my mind, the object-type checking is just making that fast-forward
check more thorough (i.e., extending it to non-commit objects).
> cache.h | 1 -
> remote.c | 24 +-----------------------
> t/t5516-fetch-push.sh | 21 ---------------------
> 3 files changed, 1 insertion(+), 45 deletions(-)
The patch itself looks fine to me. Whether we agree on the fast-forward
object-type checking or not, it is the correct first step to take in
either case.
-Peff
^ permalink raw reply
* Re: [PATCH] clean.c, ls-files.c: respect encapsulation of exclude_list_groups
From: Junio C Hamano @ 2013-01-16 17:43 UTC (permalink / raw)
To: Adam Spiers; +Cc: git list
In-Reply-To: <1358342758-30503-1-git-send-email-git@adamspiers.org>
Adam Spiers <git@adamspiers.org> writes:
> Consumers of the dir.c traversal API should avoid assuming knowledge
> of the internal implementation of exclude_list_groups. Therefore
> when adding items to an exclude list, it should be accessed via the
> pointer returned from add_exclude_list(), rather than by referencing
> a location within dir.exclude_list_groups[EXC_CMDL].
Sounds sensible.
^ permalink raw reply
* Question re. git remote repository
From: Lang, David @ 2013-01-16 17:49 UTC (permalink / raw)
To: 'git@vger.kernel.org'
Hello,
We're just in the process of investigating a versioning tool and are very interesting in git. We have one question we're hoping someone can answer. In regards to the repositories, I think I understand correctly that each developer will have a local repository that they will work from, and that there will also be a remote repository (origin) that will hold the original version of the project.
It appears from the limited reading I've done that the remote repository must be hosted at github.com. Is this the case? Ideally we'd prefer to simply create our remote repository on a drive of one of our local network servers. Is this possible?
Thanks in advance for the help.
David Lang | Application Developer | Tel: 416-340-4800 x.5277
Cardiac IT Dept - Toronto General Hospital
The University Health Network
200 Elizabeth St.
Toronto, ON M5G 2C4
This e-mail may contain confidential and/or privileged information for the sole use of the intended recipient.
Any review or distribution by anyone other than the person for whom it was originally intended is strictly prohibited.
If you have received this e-mail in error, please contact the sender and delete all copies.
Opinions, conclusions or other information contained in this e-mail may not be that of the organization.
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: Jeff King @ 2013-01-16 17:50 UTC (permalink / raw)
To: Max Horn
Cc: John Keeping, Antoine Pelisse, Junio C Hamano, git, Johannes Sixt
In-Reply-To: <7FDA1B56-731E-4BA2-8FE5-196B965FFFDB@quendi.de>
On Wed, Jan 16, 2013 at 06:26:35PM +0100, Max Horn wrote:
> > On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
> >> FWIW, I also happen to have the warning:
> >>
> >> advice.c:69:2: warning: expression result unused [-Wunused-value]
> >> error("'%s' is not possible because you have unmerged files.", me);
> >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> ./git-compat-util.h:314:55: note: expanded from:
> >> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
> >> ^~
> >>
> >> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
> >> (based on LLVM 3.0)
> >
> > I have the same output with:
> >
> > clang version 3.2 (tags/RELEASE_32/final)
>
> Sorry for not being more specific in my message. I have this with
>
> Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)
So it seems pretty common, and is just that clang is more concerned
about this than gcc. I think your patch is a reasonable workaround. It
seems a little weird to me that clang defines __GNUC__, but I assume
there are good reasons for it. The commit message should probably be
along the lines of:
Commit a469a10 wraps some error calls in macros to give the compiler a
chance to do more static analysis on their constant -1 return value.
We limit the use of these macros to __GNUC__, since gcc is the primary
beneficiary of the new information, and because we use GNU features
for handling variadic macros.
However, clang also defines __GNUC__, but generates warnings (due to
throwing away the return value from the first half of the macro). We
can squelch the warning by turning off these macros when clang is in
use.
I'm confused, though, why your patch does not have a matching update to
the opterror macro in parse-options.h. It uses exactly the same
technique. Does it not generate a warning?
-Peff
^ permalink raw reply
* [PATCH v2 17/19] fixup! reset $sha1 $pathspec: require $sha1 only to be treeish
From: Martin von Zweigbergk @ 2013-01-16 18:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358228871-7142-18-git-send-email-martinvonz@gmail.com>
---
Sorry, I forgot the documentation updates. I hope this looks ok. Can
you squash this in, Junio? Thanks.
I don't think any documentation update is necessary for the "reset on
unborn branch" patch. Let me know if you think differently.
Documentation/git-reset.txt | 18 +++++++++---------
builtin/reset.c | 4 ++--
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 978d8da..a404b47 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -8,20 +8,20 @@ git-reset - Reset current HEAD to the specified state
SYNOPSIS
--------
[verse]
-'git reset' [-q] [<commit>] [--] <paths>...
-'git reset' (--patch | -p) [<commit>] [--] [<paths>...]
+'git reset' [-q] [<tree-ish>] [--] <paths>...
+'git reset' (--patch | -p) [<tree-sh>] [--] [<paths>...]
'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [<commit>]
DESCRIPTION
-----------
-In the first and second form, copy entries from <commit> to the index.
+In the first and second form, copy entries from <tree-ish> to the index.
In the third form, set the current branch head (HEAD) to <commit>, optionally
-modifying index and working tree to match. The <commit> defaults to HEAD
-in all forms.
+modifying index and working tree to match. The <tree-ish>/<commit> defaults
+to HEAD in all forms.
-'git reset' [-q] [<commit>] [--] <paths>...::
+'git reset' [-q] [<tree-ish>] [--] <paths>...::
This form resets the index entries for all <paths> to their
- state at <commit>. (It does not affect the working tree, nor
+ state at <tree-ish>. (It does not affect the working tree, nor
the current branch.)
+
This means that `git reset <paths>` is the opposite of `git add
@@ -34,9 +34,9 @@ Alternatively, using linkgit:git-checkout[1] and specifying a commit, you
can copy the contents of a path out of a commit to the index and to the
working tree in one go.
-'git reset' (--patch | -p) [<commit>] [--] [<paths>...]::
+'git reset' (--patch | -p) [<tree-ish>] [--] [<paths>...]::
Interactively select hunks in the difference between the index
- and <commit> (defaults to HEAD). The chosen hunks are applied
+ and <tree-ish> (defaults to HEAD). The chosen hunks are applied
in reverse to the index.
+
This means that `git reset -p` is the opposite of `git add -p`, i.e.
diff --git a/builtin/reset.c b/builtin/reset.c
index b776867..cb84f1b 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -23,8 +23,8 @@
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
- N_("git reset [-q] <commit> [--] <paths>..."),
- N_("git reset --patch [<commit>] [--] [<paths>...]"),
+ N_("git reset [-q] <tree-ish> [--] <paths>..."),
+ N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
NULL
};
--
1.8.1.1.454.gce43f05
^ permalink raw reply related
* Re: [PATCH] fix some clang warnings
From: Jeff King @ 2013-01-16 18:00 UTC (permalink / raw)
To: Max Horn
Cc: John Keeping, Antoine Pelisse, Junio C Hamano, git, Johannes Sixt
In-Reply-To: <20130116175057.GB27525@sigill.intra.peff.net>
On Wed, Jan 16, 2013 at 09:50:57AM -0800, Jeff King wrote:
> I'm confused, though, why your patch does not have a matching update to
> the opterror macro in parse-options.h. It uses exactly the same
> technique. Does it not generate a warning?
Ah, I think I see why not.
It is not about the macro itself, but rather the callsites that do not
return error, but call it for its printing side effect. It seems that
clang -Wunused-value is OK with unused values from functions being
discarded, but not with constants. So:
int foo();
void bar()
{
foo(); /* ok */
1; /* not ok */
(foo(), 1); /* not ok */
}
The first one is OK (I think it would fall under -Wunused-result under
either compiler). The middle one is an obvious error, and caught by both
compilers. The last one is OK by gcc, but clang complains.
So opterror does not happen to generate any warnings, because we do not
ever use it in a void context. It should probably be marked the same
way, though, as future-proofing.
> The commit message should probably be along the lines of:
> [...]
> However, clang also defines __GNUC__, but generates warnings (due to
> throwing away the return value from the first half of the macro). We
> can squelch the warning by turning off these macros when clang is in
> use.
So a more accurate description would be:
However, clang also defines __GNUC__, but generates warnings with
-Wunused-value when these macros are used in a void context, because
the constant "-1" ends up being useless. Gcc does not complain about
this case (though it is unclear if it is because it is smart enough to
see what we are doing, or too dumb to realize that the -1 is unused).
We can squelch the warning by just disabling these macros when clang
is in use.
-Peff
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: Tomas Carnecky @ 2013-01-16 18:03 UTC (permalink / raw)
To: Jeff King, Max Horn
Cc: John Keeping, Antoine Pelisse, Junio C Hamano, git, Johannes Sixt
In-Reply-To: <20130116175057.GB27525@sigill.intra.peff.net>
On Wed, 16 Jan 2013 09:50:57 -0800, Jeff King <peff@peff.net> wrote:
> However, clang also defines __GNUC__, [...]
http://sourceforge.net/p/predef/wiki/Compilers/
Notice that the meaning of the __GNUC__ macro has changed subtly over the
years, from identifying the GNU C/C++ compiler to identifying any compiler
that implements the GNU compiler extensions (...). For example, the Intel
C++ on Linux also defines these macros from version 8.1 (...).
^ permalink raw reply
* Re: Question re. git remote repository
From: Konstantin Khomoutov @ 2013-01-16 18:06 UTC (permalink / raw)
To: Lang, David; +Cc: 'git@vger.kernel.org'
In-Reply-To: <201301161749.r0GHnGV6007806@smtpb02.one-mail.on.ca>
On Wed, 16 Jan 2013 17:49:09 +0000
"Lang, David" <David.Lang@uhn.ca> wrote:
> We're just in the process of investigating a versioning tool and are
> very interesting in git. We have one question we're hoping someone
> can answer. In regards to the repositories, I think I understand
> correctly that each developer will have a local repository that they
> will work from, and that there will also be a remote repository
> (origin) that will hold the original version of the project.
The name "origin" is purely arbitrary: any local repository might have
> It appears from the limited reading I've done that the remote
> repository must be hosted at github.com. Is this the case?
Of course not. github is just a Git hosting provider. There are
plenty of them -- both commercial and not-for-profit (a well-known
service bitbucket.org is one example).
> Ideally we'd prefer to simply create our remote repository on a drive
> of one of our local network servers. Is this possible?
Yes, this is possible, but it's not advised to keep such a "reference"
repository on an exported networked drive for a number of reasons (both
performance and bug-free operation).
Instead, the canonical way to host "reference" repositories is to make
them accessible via SSH or via HTTP[S]. To do this, a server running
some POSIX OS (Linux- or *BSD-based) is the best bet. Both kinds of
access require Git itself installed on the server. Obviously, SSH
access requires an SSH server software (such as OpenSSH) as well and
HTTP[S] access requires a web server (such as Apache). Of course,
everything mentioned is available on any sensible OS you might install
on your server. Read-only access might be provided by a special tool
named "Git daemon" which is a part of Git.
If you have more than a couple of developers you might want to install
certain front-end Git software on the server which provides for
"virtualized" Git users and fine-grained control over who can do what.
Using gitolite [3] for this is the current trend.
Web-browsing for your repositories, if needed, is usually provided by
the tool named gitweb [4].
Everything I've just summarised is well explained in [5] and [6] (as an
addendum).
Another approach is to set up a "turn-key" solution such as GitLab [1]
or gitblit [2].
1. http://gitlabhq.com/
2. http://gitblit.com/
3. https://github.com/sitaramc/gitolite
4. https://git.wiki.kernel.org/index.php/Gitweb
5. http://git-scm.com/book/en/Git-on-the-Server
6. http://git-scm.com/2010/03/04/smart-http.html
^ permalink raw reply
* Re: [PATCH v2 17/19] fixup! reset $sha1 $pathspec: require $sha1 only to be treeish
From: Martin von Zweigbergk @ 2013-01-16 18:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Matt Kraai, Ramsay Jones, Duy Nguyen, Jeff King,
Martin von Zweigbergk
In-Reply-To: <1358359235-10213-1-git-send-email-martinvonz@gmail.com>
On Wed, Jan 16, 2013 at 10:00 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> ---
>
> Sorry, I forgot the documentation updates. I hope this looks ok. Can
> you squash this in, Junio? Thanks.
I see the series just entered 'next', so I guess it would have to go
on top then. Perhaps with a commit message like as simple as the
following. Let me know if you prefer it to be resent as a proper
patch. Sorry about the noise.
reset: update documentation to require only tree-ish with paths
When resetting with paths, we no longer require a commit argument, but
only a tree-ish. Update the documentation and synopsis accordingly.
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: Jeff King @ 2013-01-16 18:09 UTC (permalink / raw)
To: Max Horn
Cc: John Keeping, Antoine Pelisse, Junio C Hamano, git, Johannes Sixt
In-Reply-To: <20130116180041.GC27525@sigill.intra.peff.net>
On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:
> So opterror does not happen to generate any warnings, because we do not
> ever use it in a void context. It should probably be marked the same
> way, though, as future-proofing.
> [...]
> So a more accurate description would be:
Here it is all together:
-- >8 --
From: Max Horn <max@quendi.de>
Subject: [PATCH] fix clang -Wunused-value warnings for error functions
Commit a469a10 wraps some error calls in macros to give the
compiler a chance to do more static analysis on their
constant -1 return value. We limit the use of these macros
to __GNUC__, since gcc is the primary beneficiary of the new
information, and because we use GNU features for handling
variadic macros.
However, clang also defines __GNUC__, but generates warnings
with -Wunused-value when these macros are used in a void
context, because the constant "-1" ends up being useless.
Gcc does not complain about this case (though it is unclear
if it is because it is smart enough to see what we are
doing, or too dumb to realize that the -1 is unused). We
can squelch the warning by just disabling these macros when
clang is in use.
Signed-off-by: Max Horn <max@quendi.de>
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 2 +-
git-compat-util.h | 2 +-
parse-options.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index c257953..5c8440b 100644
--- a/cache.h
+++ b/cache.h
@@ -1148,7 +1148,7 @@ extern int config_error_nonbool(const char *);
extern int git_env_bool(const char *, int);
extern int git_config_system(void);
extern int config_error_nonbool(const char *);
-#ifdef __GNUC__
+#if defined(__GNUC__) && ! defined(__clang__)
#define config_error_nonbool(s) (config_error_nonbool(s), -1)
#endif
extern const char *get_log_output_encoding(void);
diff --git a/git-compat-util.h b/git-compat-util.h
index 2cecf56..2596280 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -297,7 +297,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
* behavior. But since we're only trying to help gcc, anyway, it's OK; other
* compilers will fall back to using the function as usual.
*/
-#ifdef __GNUC__
+#if defined(__GNUC__) && ! defined(__clang__)
#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
#endif
diff --git a/parse-options.h b/parse-options.h
index e703853..1c8bd8d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -177,7 +177,7 @@ extern int opterror(const struct option *opt, const char *reason, int flags);
extern int optbug(const struct option *opt, const char *reason);
extern int opterror(const struct option *opt, const char *reason, int flags);
-#ifdef __GNUC__
+#if defined(__GNUC__) && ! defined(clang)
#define opterror(o,r,f) (opterror((o),(r),(f)), -1)
#endif
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* Re: [PATCH] fix some clang warnings
From: John Keeping @ 2013-01-16 18:12 UTC (permalink / raw)
To: Jeff King
Cc: Max Horn, John Keeping, Antoine Pelisse, Junio C Hamano, git,
Johannes Sixt
In-Reply-To: <20130116180041.GC27525@sigill.intra.peff.net>
On Wed, Jan 16, 2013 at 10:00:42AM -0800, Jeff King wrote:
> It is not about the macro itself, but rather the callsites that do not
> return error, but call it for its printing side effect. It seems that
> clang -Wunused-value is OK with unused values from functions being
> discarded, but not with constants. So:
>
> int foo();
> void bar()
> {
> foo(); /* ok */
> 1; /* not ok */
> (foo(), 1); /* not ok */
> }
>
> The first one is OK (I think it would fall under -Wunused-result under
> either compiler). The middle one is an obvious error, and caught by both
> compilers. The last one is OK by gcc, but clang complains.
I wonder if this would be changed in clang - the change in [1] is
superficially similar.
[1] http://llvm.org/bugs/show_bug.cgi?id=13747
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: Matthieu Moy @ 2013-01-16 18:12 UTC (permalink / raw)
To: Jeff King
Cc: Max Horn, John Keeping, Antoine Pelisse, Junio C Hamano, git,
Johannes Sixt
In-Reply-To: <20130116175057.GB27525@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> It seems a little weird to me that clang defines __GNUC__, but I
> assume there are good reasons for it.
The reason is essentially that clang targets compatibility with GCC
(implementing the same extensions & cie), in the sense "drop in
replacement that should be able to compile legacy code possibly relying
on __GNUC__ and GCC extensions."
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ 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