* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Andreas Schwab @ 2013-01-16 19:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael J Gruber, Jardel Weyrich, Sascha Cunz,
git@vger.kernel.org
In-Reply-To: <7v622xyvnd.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I actually think my earlier "it shouldn't be the same (push)" is not
> needed and probably is actively wrong. Just like you can tell
> between
>
> (only one .url) (both .url and .pushurl)
>
> origin there (fetch/push) origin there (fetch)
> origin there (push)
What should happen when you have a .pushinsteadof configured that
modifies .url for pushing?
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: Question re. git remote repository
From: Konstantin Khomoutov @ 2013-01-16 19:37 UTC (permalink / raw)
To: Jeff King
Cc: Konstantin Khomoutov, Lang, David, 'git@vger.kernel.org'
In-Reply-To: <20130116182156.GB4426@sigill.intra.peff.net>
On Wed, 16 Jan 2013 10:21:56 -0800
Jeff King <peff@peff.net> wrote:
Thanks for elaborating on the "origin" -- I intended to write up on its
special status but got distracted and sent my message missing that
bit ;-)
[...]
> > > 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).
>
> I agree that performance is not ideal (although if you are on a fast
> LAN, it probably would not matter much), but I do not recall any
> specific bugs in that area. Can you elaborate?
This one [1] for instance. I also recall seing people having other
"mystical" problems with setups like this so I somehow developed an idea
than having a repository on a networked drive is asking for troubles.
Of course, if there are happy users of such setups, I would be glad to
hear as my precautions might well be unfounded for the recent versions
of Git.
1. http://code.google.com/p/msysgit/issues/detail?id=130
^ permalink raw reply
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Junio C Hamano @ 2013-01-16 20:01 UTC (permalink / raw)
To: Andreas Schwab
Cc: Michael J Gruber, Jardel Weyrich, Sascha Cunz,
git@vger.kernel.org
In-Reply-To: <m2ip6x0vtk.fsf@igel.home>
Andreas Schwab <schwab@linux-m68k.org> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I actually think my earlier "it shouldn't be the same (push)" is not
>> needed and probably is actively wrong. Just like you can tell
>> between
>>
>> (only one .url) (both .url and .pushurl)
>>
>> origin there (fetch/push) origin there (fetch)
>> origin there (push)
>
> What should happen when you have a .pushinsteadof configured that
> modifies .url for pushing?
I think push should work like this:
* the user gives us a nickname;
* we look at remote.$nickname.pushurl (and if there isn't,
remote.$nickname.url) to decide the logical URLs to push to;
* for each logical URL we decided to push, we look at
url.*.pushInsteadOf to see if there is one that match the $URL
(and if there isn't url.*.insteadOf), and map the logical URL to
the final destination.
So that we can instruct "push" to push, when pushing into a
repository that logically resides at git://git.k.org/pub/,
to instead push into the repository via git-over-ssh, e.g.
[remote "korg"]
url = git://git.k.org/pub/scm/git/git.git/
[url "git.k.org:/pub/"]
pushInsteadOf = git://git.k.org/pub/
without affecting the fetching side.
As I said in a separate message, the above "fetch/push" vs "fetch"
and "push" distinction is not descriptive enough to express the post
rewriting that is done with insteadOf; it only helps debugging
misconfiguration between .url vs .pushurl, which may be better than
the status quo but is not ideal.
^ permalink raw reply
* Re: Question re. git remote repository
From: David Lang @ 2013-01-16 20:07 UTC (permalink / raw)
To: Lang, David; +Cc: 'git@vger.kernel.org'
In-Reply-To: <201301161749.r0GHnGV6007806@smtpb02.one-mail.on.ca>
Hi David, now we are going to have some confusion here, two David Langs on the
list :-)
On Wed, 16 Jan 2013, Lang, David 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.
>
> 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?
Git is peer-to-peer, the 'origin' is just the default to pull from. It can be
hosted on any machine that you have access to.
A typical case is that you designate one person (or a small group of people)
to oversee your master repository, and that person decides when and what to pull
there from the developers.
This gives you a chance to insert code review, tests, etc between what the
developers produce in their local repository and what you then bless as the
authoritative 'released' version of the code.
However, this master repository is just a matter of convention, it is possible
to use any repository as the 'origin', changing it is just a config change away.
David Lang
^ permalink raw reply
* Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields
From: Ramsay Jones @ 2013-01-16 20:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Robin Rosenberg, git, j sixt, Shawn Pearce
In-Reply-To: <7va9sa6f0h.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> That configurability is a slipperly slope to drag us into giving users
> more complexity that does not help them very much, I suspect.
>
> Earlier somebody mentioned "size and mtime is often enough", so I
> think a single option core.looseStatInfo (substitute "loose" with
> short, minimum or whatever adjective that is more appropriate---I am
> not good at picking phrases, it sounds to me a way to more loosely
> define stat info cleanliness than we usually do) that makes us
> ignore all fields (regardless of their zero-ness) other than those
> two fields might not be a bad way to go.
At one point, I used to build (and test) the MSVC version of git on
cygwin, which leads to exactly the same problem. So, this is not just
an EGit/JGit vs c-git issue, although there can't be many people that
will have this problem. (Mixing the MinGW and cygwin versions on the
same repo will also have this problem).
I had a patch which, essentially, did what you suggest above; ie ignore
everything other than size and mtime, *including* ignoring the zero-ness
in the index. (I just don't understand why you would think of doing
otherwise!! ;-) ). As part of that patch, I also suppressed the "empty diff"
output that used to be shown for stat-dirty files (that's been fixed now
right?), otherwise using gitk was a pain.
[BTW, given the "schizophrenic stat" functions on cygwin, you can have
this problem with the cygwin version of git - all on it's lonesome!]
I can't help with naming, BTW, since I called the config variable
"core.ramsay-stat". :-P
>
> I do not offhand know if such a loose mode is too simple and make it
> excessively risky, though.
I suspect it would be fine ... *however*, I never sent my patch because
I didn't think there would be many idiots^H^H^H^H^H^H pioneers like me! :-D
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset
From: Ben Walton @ 2013-01-16 20:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: esr, git
In-Reply-To: <7vehhlyw90.fsf@alter.siamese.dyndns.org>
On Wed, Jan 16, 2013 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ben Walton <bdwalton@gmail.com> writes:
>
>> +sub get_tz_offset {
>> + # some systmes don't handle or mishandle %z, so be creative.
>
> Hmph. I wonder if we can use %z if it is handled correctly and fall
> back to this code only on platforms that are broken?
That would be perfectly acceptable to me. The reason I set it up to
always run through this function here is that when I originally added
this function for git-svn, I'd made it conditional and Eric Wong
preferred that the function be used exclusively[1]. I opted to take
the same approach here to keep things congrous.
If it were to be conditional, I think I'd add a variable to the build
system and have the code leverage that at runtime instead of the
try/except approach I attempted in 2009.
Thanks
-Ben
[1] http://lists-archives.com/git/683572-git-svn-fix-for-systems-without-strftime-z.html
--
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself. Much more happiness,
truth, beauty and wisdom will come to you that way.
-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------
^ permalink raw reply
* Re: [PATCH v2] Allow custom "comment char"
From: Junio C Hamano @ 2013-01-16 20:30 UTC (permalink / raw)
To: Ralf Thielow; +Cc: jrnieder, git
In-Reply-To: <1358363928-16729-1-git-send-email-ralf.thielow@gmail.com>
Ralf Thielow <ralf.thielow@gmail.com> writes:
> From: Junio C Hamano <gitster@pobox.com>
>
> Some users do want to write a line that begin with a pound sign, #,
> in their commit log message. Many tracking system recognise
> a token of #<bugid> form, for example.
>
> The support we offer these use cases is not very friendly to the end
> users. They have a choice between
>
> - Don't do it. Avoid such a line by rewrapping or indenting; and
>
> - Use --cleanup=whitespace but remove all the hint lines we add.
>
> Give them a way to set a custom comment char, e.g.
>
> $ git -c core.commentchar="%" commit
>
> so that they do not have to do either of the two workarounds.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> ---
> Junio, thanks for the code in your reply to the
> first version. It works very well and looks nice.
> I was also unhappy about this "\n%c\n" thing and
> pretty unsure with the code in "git-submodule.sh".
> But with this, it looks good to me. Thanks.
>
> Changes in v2:
> - extend "git stripspace" with an option to make
> it's input being converted to commented lines
> - teach git-submodule.sh using this
> - rename strbuf_commented_addstr to strbuf_add_commented_lines
> and improve it's design
Oh, I love it when something like this happens. Throw a "perhaps
along these lines" patch and then a finished product that fills the
gaps I didn't bother to fill magically appears, even with tests and
updates to comments and documentation.
What good things did I do recently to deserve such a luck? ;-)
> @@ -66,21 +67,52 @@ void stripspace(struct strbuf *sb, int skip_comments)
> strbuf_setlen(sb, j);
> }
>
> +static void comment_lines(struct strbuf *buf)
> +{
> + char *msg;
> + size_t len;
> +
> + msg = strbuf_detach(buf, &len);
> + strbuf_add_commented_lines(buf, msg, len);
> +}
This leaks msg (inherited from my "perhaps along these lines"
patch). I think I can just add free(msg) at the end.
> + if (strip_comments || mode == COMMENT_LINES)
> + git_config(git_default_config, NULL);
Nice spotting. The "along these lines" patch broke "stripspace -s"
under custom comment line char; this fixes it.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset
From: Junio C Hamano @ 2013-01-16 20:36 UTC (permalink / raw)
To: Ben Walton; +Cc: esr, git
In-Reply-To: <CAP30j164UD9gNRbZ=uCQjgpDODWnGtYmHcWES2P=YPryL=FbZA@mail.gmail.com>
Ben Walton <bdwalton@gmail.com> writes:
> On Wed, Jan 16, 2013 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ben Walton <bdwalton@gmail.com> writes:
>>
>>> +sub get_tz_offset {
>>> + # some systmes don't handle or mishandle %z, so be creative.
>>
>> Hmph. I wonder if we can use %z if it is handled correctly and fall
>> back to this code only on platforms that are broken?
>
> That would be perfectly acceptable to me. The reason I set it up to
> always run through this function here is that when I originally added
> this function for git-svn, I'd made it conditional and Eric Wong
> preferred that the function be used exclusively[1]. I opted to take
> the same approach here to keep things congrous.
>
> If it were to be conditional, I think I'd add a variable to the build
> system and have the code leverage that at runtime instead of the
> try/except approach I attempted in 2009.
If the code was originally unconditional for a reason (and I think
being bug-to-bug compatible across platforms is actually a good
thing in a tool like importers), I would not object to it. Thanks
for the back-story.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 21:02 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: <20130116174325.GA27525@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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 "+").
Yeah, actually in that example, I meant refs/tested/ would have
pointers to bare tree objects. I often rebuild 'pu' and another
private integration branch for testing, reordering the series that
are still not in 'next' and also after rewriting log messages of
some commits. It is not unusual to end up the updated 'pu' having
the identical tree as 'pu' before update, and I want to skip testing
the result (tree equality matters while commit equality does not in
such a use case).
> In my mind, the object-type checking is just making that fast-forward
> check more thorough (i.e., extending it to non-commit objects).
Yes, I agree with that point of view.
Thanks.
Here is what I am planning to queue (the patch is the same, but the
message is different on the third point).
-- >8 --
Subject: [PATCH] push: fix "refs/tags/ hierarchy cannot be updated without --force"
When pushing to update a branch with a commit that is not a
descendant of the commit at the tip, a wrong message "already
exists" was given, instead of the correct "non-fast-forward", if we
do not have the object sitting in the destination repository at the
tip of the ref we are updating.
The primary cause of the bug is that the check in a new helper
function is_forwardable() assumed both old and new objects are
available and can be checked, which is not always the case.
The way the caller uses the result of this function is also wrong.
If the helper says "we do not want to let this push go through", the
caller unconditionally translates it into "we blocked it because the
destination already exists", which is not true at all in this case.
Fix this by doing these three things:
* Remove unnecessary not_forwardable from "struct ref"; it is only
used inside set_ref_status_for_push();
* Make "refs/tags/" 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.
The policy last one tried to implement may later be resurrected and
extended to ensure fast-forwardness (defined as "not losing
objects", extending from the traditional "not losing commits from
the resulting history") when objects that are not commit are
involved (e.g. an annotated tag in hierarchies outside refs/tags),
but such a logic belongs to "is this a fast-forward?" check that is
done by ref_newer(); is_forwardable(), which is now removed, was not
the right place to do so.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 1 -
remote.c | 43 +++++++------------------------------------
t/t5516-fetch-push.sh | 21 ---------------------
3 files changed, 7 insertions(+), 58 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..d3a1ca2 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)
{
@@ -1320,32 +1300,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
}
/*
- * The below logic determines whether an individual
- * refspec A:B can be pushed. The push will succeed
- * if any of the following are true:
+ * Decide whether an individual refspec A:B can be
+ * pushed. The push will succeed if any of the
+ * following are true:
*
* (1) the remote reference B does not exist
*
* (2) the remote reference B is being removed (i.e.,
* pushing :B where no source is specified)
*
- * (3) the update meets all fast-forwarding criteria:
- *
- * (a) the destination is not under refs/tags/
- * (b) the old is a commit
- * (c) the new is a descendant of the old
- *
- * NOTE: We must actually have the old object in
- * order to overwrite it in the remote reference,
- * and the new object must be commit-ish. These are
- * implied by (b) and (c) respectively.
+ * (3) the destination is not under refs/tags/, and
+ * if the old and new value is a commit, the new
+ * is a descendant of the old.
*
* (4) it is forced using the +A:B notation, or by
* passing the --force argument
*/
- ref->not_forwardable = !is_forwardable(ref);
-
ref->update =
!ref->deletion &&
!is_null_sha1(ref->old_sha1);
@@ -1355,7 +1326,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" &&
--
1.8.1.1.426.g616047d
^ permalink raw reply related
* Re: [PATCH v2] Allow custom "comment char"
From: Jens Lehmann @ 2013-01-16 21:02 UTC (permalink / raw)
To: Ralf Thielow; +Cc: gitster, jrnieder, git
In-Reply-To: <1358363928-16729-1-git-send-email-ralf.thielow@gmail.com>
Am 16.01.2013 20:18, schrieb Ralf Thielow:
> From: Junio C Hamano <gitster@pobox.com>
>
> Some users do want to write a line that begin with a pound sign, #,
> in their commit log message. Many tracking system recognise
> a token of #<bugid> form, for example.
>
> The support we offer these use cases is not very friendly to the end
> users. They have a choice between
>
> - Don't do it. Avoid such a line by rewrapping or indenting; and
>
> - Use --cleanup=whitespace but remove all the hint lines we add.
>
> Give them a way to set a custom comment char, e.g.
>
> $ git -c core.commentchar="%" commit
>
> so that they do not have to do either of the two workarounds.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> ---
> Junio, thanks for the code in your reply to the
> first version. It works very well and looks nice.
> I was also unhappy about this "\n%c\n" thing and
> pretty unsure with the code in "git-submodule.sh".
I can't see anything wrong with it (but didn't have the time to
test it). On my todo list (but *way* down) is the task to replace
the call to "git submodule summary --for-status ..." in
wt_status_print_submodule_summary() with a call to "git diff
--submodule" (and - at least in the long term - rip out the
--for-status option from the submodule script). Maybe now is a
good time for someone else to tackle that? (especially as the new
strbuf_commented_add*() functions should make that rather easy)
^ permalink raw reply
* Re: Question re. git remote repository
From: Matt Seitz (matseitz) @ 2013-01-16 21:07 UTC (permalink / raw)
To: git@vger.kernel.org
"Konstantin Khomoutov" <kostix+git@007spb.ru> wrote in message news:<20130116233744.7d0775eaec98ce154a9de180@domain007.com>...
> On Wed, 16 Jan 2013 10:21:56 -0800
> Jeff King <peff@peff.net> wrote:
> >
> > I agree that performance is not ideal (although if you are on a fast
> > LAN, it probably would not matter much), but I do not recall any
> > specific bugs in that area. Can you elaborate?
>
> Of course, if there are happy users of such setups, I would be glad to
> hear as my precautions might well be unfounded for the recent versions
> of Git.
I'm a happy user of git on network file systems (NFS and CIFS/SMB), although not a heavy user.
> 1. http://code.google.com/p/msysgit/issues/detail?id=130
I wouldn't be surprised if there are some subtle POSIX-Win32 compatibility issues here between msysgit and Samba (POSIX GIT, ported to use Win32 file system functions, sending those Win32 requests to a Samba server, and the Samba server translating those Win32 requests back into POSIX functions).
^ permalink raw reply
* Re: [PATCH v3 2/3] config: Introduce diff.algorithm variable
From: Junio C Hamano @ 2013-01-16 21:59 UTC (permalink / raw)
To: Michal Privoznik; +Cc: git, trast, peff
In-Reply-To: <7vbocpxbwp.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Will replace the one in 'pu' with this round. Looking good.
>
> Thanks.
By the way, wouldn't we want some tests to protect this feature from
future breakages?
^ permalink raw reply
* Re: [PATCH] clean.c, ls-files.c: respect encapsulation of exclude_list_groups
From: Junio C Hamano @ 2013-01-16 22:20 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].
>
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
> builtin/clean.c | 6 +++---
> builtin/ls-files.c | 15 ++++++++++-----
> 2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index b098288..b9cb7ad 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -45,6 +45,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> static const char **pathspec;
> struct strbuf buf = STRBUF_INIT;
> struct string_list exclude_list = STRING_LIST_INIT_NODUP;
> + struct exclude_list *el;
When a type "exclude_list" exists and used in the same function,
having a local variable of the same name but of a different type
becomes a bit awkward.
builtin/ls-files.c shares the same structure. Does the file-scope
"exclude_args" variable need to be a file-scope static over there?
It seems that it is closely tied to the elements of the string list,
so it may make sense to:
* remove the file-scope static "exclude_args";
* rename "exclude_list" string list variable to "exclude_args";
and
* replace "--exclude_args" in the loop that iterates over
exclude_list (now exclude_args) with "-(i+1)" or something,
just like you do in "builtin/clean.c" below.
> - add_exclude_list(&dir, EXC_CMDL, "--exclude option");
> + el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
> for (i = 0; i < exclude_list.nr; i++)
> - add_exclude(exclude_list.items[i].string, "", 0,
> - &dir.exclude_list_group[EXC_CMDL].el[0], -(i+1));
> + add_exclude(exclude_list.items[i].string, "", 0, el, -(i+1));
We may want to use for_each_string_list_item() here and in the
corresponding loop in builtin/ls-files.c, but because we do need to
give the -(i + 1) label to each element, I think the code is OK
as-is.
^ permalink raw reply
* [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
From: Antoine Pelisse @ 2013-01-16 22:47 UTC (permalink / raw)
To: John Keeping, Max Horn, Junio C Hamano
Cc: git, Johannes Sixt, Antoine Pelisse
In-Reply-To: <1358376443-7404-1-git-send-email-apelisse@gmail.com>
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 related
* [PATCH 1/2] fix clang -Wconstant-conversion with bit fields
From: Antoine Pelisse @ 2013-01-16 22:47 UTC (permalink / raw)
To: John Keeping, Max Horn, Junio C Hamano
Cc: git, Johannes Sixt, Antoine Pelisse
In-Reply-To: <20130116182449.GA4881@sigill.intra.peff.net>
clang incorrectly reports a constant conversion warning (implicit
truncation to bit field) when using the "flag &= ~FLAG" form, because
~FLAG needs to be truncated.
Convert this form to "flag = flag & ~FLAG" fixes the issue as
the right operand now fits into the bit field.
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
I'm sorry about this fix, it really seems bad, yet it's one step closer
to warning-free clang compilation.
It seems quite clear to me that it's a bug in clang.
bisect.c | 2 +-
builtin/checkout.c | 2 +-
builtin/reflog.c | 4 ++--
commit.c | 4 ++--
revision.c | 8 ++++----
upload-pack.c | 4 ++--
6 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/bisect.c b/bisect.c
index bd1b7b5..34ac01d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -63,7 +63,7 @@ static void clear_distance(struct commit_list *list)
{
while (list) {
struct commit *commit = list->item;
- commit->object.flags &= ~COUNTED;
+ commit->object.flags = commit->object.flags & ~COUNTED;
list = list->next;
}
}
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a9c1b5a..2c83234 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -717,7 +717,7 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)
init_revisions(&revs, NULL);
setup_revisions(0, NULL, &revs, NULL);
- object->flags &= ~UNINTERESTING;
+ object->flags = object->flags & ~UNINTERESTING;
add_pending_object(&revs, object, sha1_to_hex(object->sha1));
for_each_ref(add_pending_uninteresting_ref, &revs);
diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..3079c81 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -170,7 +170,7 @@ static int commit_is_complete(struct commit *commit)
}
/* clear flags from the objects we traversed */
for (i = 0; i < found.nr; i++)
- found.objects[i].item->flags &= ~STUDYING;
+ found.objects[i].item->flags = found.objects[i].item->flags& ~STUDYING;
if (is_incomplete)
commit->object.flags |= INCOMPLETE;
else {
@@ -229,7 +229,7 @@ static void mark_reachable(struct expire_reflog_cb *cb)
struct commit_list *leftover = NULL;
for (pending = cb->mark_list; pending; pending = pending->next)
- pending->item->object.flags &= ~REACHABLE;
+ pending->item->object.flags = pending->item->object.flags & ~REACHABLE;
pending = cb->mark_list;
while (pending) {
diff --git a/commit.c b/commit.c
index e8eb0ae..800779d 100644
--- a/commit.c
+++ b/commit.c
@@ -883,7 +883,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)
/* Uniquify */
for (p = heads; p; p = p->next)
- p->item->object.flags &= ~STALE;
+ p->item->object.flags = p->item->object.flags & ~STALE;
for (p = heads, num_head = 0; p; p = p->next) {
if (p->item->object.flags & STALE)
continue;
@@ -894,7 +894,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)
for (p = heads, i = 0; p; p = p->next) {
if (p->item->object.flags & STALE) {
array[i++] = p->item;
- p->item->object.flags &= ~STALE;
+ p->item->object.flags = p->item->object.flags & ~STALE;
}
}
num_head = remove_redundant(array, num_head);
diff --git a/revision.c b/revision.c
index d7562ee..ed1c16d 100644
--- a/revision.c
+++ b/revision.c
@@ -787,9 +787,9 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li
/* We are done with the TMP_MARK */
for (p = list; p; p = p->next)
- p->item->object.flags &= ~TMP_MARK;
+ p->item->object.flags = p->item->object.flags & ~TMP_MARK;
for (p = bottom; p; p = p->next)
- p->item->object.flags &= ~TMP_MARK;
+ p->item->object.flags = p->item->object.flags & ~TMP_MARK;
free_commit_list(rlist);
}
@@ -1948,7 +1948,7 @@ static int remove_duplicate_parents(struct commit *commit)
/* count them while clearing the temporary mark */
surviving_parents = 0;
for (p = commit->parents; p; p = p->next) {
- p->item->object.flags &= ~TMP_MARK;
+ p->item->object.flags = p->item->object.flags & ~TMP_MARK;
surviving_parents++;
}
return surviving_parents;
@@ -2378,7 +2378,7 @@ static struct commit *get_revision_1(struct rev_info *revs)
if (revs->reflog_info) {
fake_reflog_parent(revs->reflog_info, commit);
- commit->object.flags &= ~(ADDED | SEEN | SHOWN);
+ commit->object.flags = commit->object.flags & ~(ADDED | SEEN | SHOWN);
}
/*
diff --git a/upload-pack.c b/upload-pack.c
index 7c05b15..74d8f0e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -113,7 +113,7 @@ static int do_rev_list(int in, int out, void *user_data)
for (i = 0; i < want_obj.nr; i++) {
struct object *o = want_obj.objects[i].item;
/* why??? */
- o->flags &= ~UNINTERESTING;
+ o->flags = o->flags & ~UNINTERESTING;
add_pending_object(&revs, o, NULL);
}
for (i = 0; i < have_obj.nr; i++) {
@@ -700,7 +700,7 @@ static void receive_needs(void)
struct commit_list *parents;
packet_write(1, "unshallow %s",
sha1_to_hex(object->sha1));
- object->flags &= ~CLIENT_SHALLOW;
+ object->flags = object->flags & ~CLIENT_SHALLOW;
/* make sure the real parents are parsed */
unregister_shallow(object->sha1);
object->parsed = 0;
--
1.8.1.1.435.g20d29be.dirty
^ permalink raw reply related
* Re: Question re. git remote repository
From: Stephen Smith @ 2013-01-16 22:59 UTC (permalink / raw)
To: Konstantin Khomoutov
Cc: Jeff King, Konstantin Khomoutov, git@vger.kernel.org, Lang, David
In-Reply-To: <20130116233744.7d0775eaec98ce154a9de180@domain007.com>
>>>> 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).
>>
>> I agree that performance is not ideal (although if you are on a fast
>> LAN, it probably would not matter much), but I do not recall any
>> specific bugs in that area. Can you elaborate?
>
> This one [1] for instance. I also recall seing people having other
> "mystical" problems with setups like this so I somehow developed an idea
> than having a repository on a networked drive is asking for troubles.
> Of course, if there are happy users of such setups, I would be glad to
> hear as my precautions might well be unfounded for the recent versions
> of Git.
>
> 1. http://code.google.com/p/msysgit/issues/detail?id=130
A group I was with used a master repository on a windows share for quite some time without a database corruption being seen.
^ permalink raw reply
* Re: Question re. git remote repository
From: David Lang @ 2013-01-16 23:00 UTC (permalink / raw)
To: Stephen Smith
Cc: Konstantin Khomoutov, Jeff King, git@vger.kernel.org, Lang, David
In-Reply-To: <0630A778-9AC8-4023-889C-4FC58ABAB683@gmail.com>
On Wed, 16 Jan 2013, Stephen Smith wrote:
>>>>> 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).
>>>
>>> I agree that performance is not ideal (although if you are on a fast
>>> LAN, it probably would not matter much), but I do not recall any
>>> specific bugs in that area. Can you elaborate?
>>
>> This one [1] for instance. I also recall seing people having other
>> "mystical" problems with setups like this so I somehow developed an idea
>> than having a repository on a networked drive is asking for troubles.
>> Of course, if there are happy users of such setups, I would be glad to
>> hear as my precautions might well be unfounded for the recent versions
>> of Git.
>>
>> 1. http://code.google.com/p/msysgit/issues/detail?id=130
>
> A group I was with used a master repository on a windows share for quite some time without a database corruption being seen. --
I think the risk is that if you have multiple people doing actions on the shared
filesystem you can run into trouble.
As long as only one copy of git is ever running against the repository, I don't
see any reason for there to be a problem.
But if you try to have one filesystem, with multiple people running git on their
machines against that shared filesystem, I would expect you to have all sorts of
problems.
David Lang
^ permalink raw reply
* Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields
From: John Keeping @ 2013-01-16 23:08 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Max Horn, Junio C Hamano, git, Johannes Sixt
In-Reply-To: <1358376443-7404-1-git-send-email-apelisse@gmail.com>
On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:
> clang incorrectly reports a constant conversion warning (implicit
> truncation to bit field) when using the "flag &= ~FLAG" form, because
> ~FLAG needs to be truncated.
>
> Convert this form to "flag = flag & ~FLAG" fixes the issue as
> the right operand now fits into the bit field.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> I'm sorry about this fix, it really seems bad, yet it's one step closer
> to warning-free clang compilation.
>
> It seems quite clear to me that it's a bug in clang.
Which version of clang did you see this with? I don't get these
warnings with clang 3.2.
John
^ permalink raw reply
* Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields
From: Antoine Pelisse @ 2013-01-16 23:09 UTC (permalink / raw)
To: John Keeping; +Cc: Max Horn, Junio C Hamano, git, Johannes Sixt
In-Reply-To: <20130116230800.GB4574@serenity.lan>
On Thu, Jan 17, 2013 at 12:08 AM, John Keeping <john@keeping.me.uk> wrote:
> On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:
>> clang incorrectly reports a constant conversion warning (implicit
>> truncation to bit field) when using the "flag &= ~FLAG" form, because
>> ~FLAG needs to be truncated.
> Which version of clang did you see this with? I don't get these
> warnings with clang 3.2.
Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final) (based on LLVM 3.0)
It's good to know it's been fixed !
^ permalink raw reply
* Re: [PATCH 2/2] fix clang -Wtautological-compare with unsigned enum
From: Antoine Pelisse @ 2013-01-16 23:10 UTC (permalink / raw)
To: John Keeping, Max Horn, Junio C Hamano
Cc: git, Johannes Sixt, Antoine Pelisse
In-Reply-To: <1358376443-7404-2-git-send-email-apelisse@gmail.com>
> With these two patches and the patch from Max Horne,
I'm deeply sorry for this typo Max
^ permalink raw reply
* Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields
From: Antoine Pelisse @ 2013-01-16 23:15 UTC (permalink / raw)
To: John Keeping; +Cc: Max Horn, Junio C Hamano, git, Johannes Sixt
In-Reply-To: <CALWbr2ypYUvuE4pWfcVvVcnJkRvCNrM1gVHp_UXeke9gbgoE3A@mail.gmail.com>
So I guess we should drop this patch, it's probably not worth it,
especially if it's been fixed already by clang.
On Thu, Jan 17, 2013 at 12:09 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
> On Thu, Jan 17, 2013 at 12:08 AM, John Keeping <john@keeping.me.uk> wrote:
>> On Wed, Jan 16, 2013 at 11:47:22PM +0100, Antoine Pelisse wrote:
>>> clang incorrectly reports a constant conversion warning (implicit
>>> truncation to bit field) when using the "flag &= ~FLAG" form, because
>>> ~FLAG needs to be truncated.
>> Which version of clang did you see this with? I don't get these
>> warnings with clang 3.2.
>
> Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final) (based on LLVM 3.0)
>
> It's good to know it's been fixed !
^ permalink raw reply
* Re: Question re. git remote repository
From: Matt Seitz (matseitz) @ 2013-01-16 23:32 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: ishchis2@gmail.com, david@lang.hm
"David Lang" <david@lang.hm> wrote in message news:<alpine.DEB.2.02.1301161459060.21503@nftneq.ynat.uz>...
> But if you try to have one filesystem, with multiple people running git on their
> machines against that shared filesystem, I would expect you to have all sorts of
> problems.
What leads you to think you will have problems?
Why would there be more of a problem on a network file system as opposed to local file system that can be accessed by multiple users?
Linus seemed to think it should work:
http://permalink.gmane.org/gmane.comp.version-control.git/122670
And "git init" specifically has a "shared" option:
--shared[=(false|true|umask|group|all|world|everybody|0xxx)]
Specify that the git repository is to be shared amongst several users. This allows users belonging to the same group to push into that repository. When specified, the config variable "core.sharedRepository" is set so that files and directories under $GIT_DIR are created with the requested permissions. When not specified, git will use permissions reported by umask(2).
^ permalink raw reply
* Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields
From: Junio C Hamano @ 2013-01-16 23:43 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: John Keeping, Max Horn, git, Johannes Sixt
In-Reply-To: <1358376443-7404-1-git-send-email-apelisse@gmail.com>
Antoine Pelisse <apelisse@gmail.com> writes:
> clang incorrectly reports a constant conversion warning (implicit
> truncation to bit field) when using the "flag &= ~FLAG" form, because
> ~FLAG needs to be truncated.
>
> Convert this form to "flag = flag & ~FLAG" fixes the issue as
> the right operand now fits into the bit field.
If the "clang incorrectly reports" is already recognised by clang
folks as a bug to be fixed in clang, I'd rather not to take this
patch.
I do not think it is reasonable to expect people to remember that
they have to write "flags &= ~TO_DROP" in a longhand whenever they
are adding new code that needs to do bit-fields, so even if this
patch makes clang silent for the _current_ code, it will not stay
that way. Something like
#define FLIP_BIT_CLR(fld,bit) do { \
typeof(fld) *x = &(fld); \
*x = *x & (~(bit)); \
} while (0)
may be more palapable but not by a large margin.
Yuck.
^ permalink raw reply
* Re: [PATCH 1/2] fix clang -Wconstant-conversion with bit fields
From: Junio C Hamano @ 2013-01-16 23:46 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: John Keeping, Max Horn, git, Johannes Sixt
In-Reply-To: <7vpq14vglu.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> clang incorrectly reports a constant conversion warning (implicit
>> truncation to bit field) when using the "flag &= ~FLAG" form, because
>> ~FLAG needs to be truncated.
>>
>> Convert this form to "flag = flag & ~FLAG" fixes the issue as
>> the right operand now fits into the bit field.
>
> If the "clang incorrectly reports" is already recognised by clang
> folks as a bug to be fixed in clang, I'd rather not to take this
> patch.
>
> I do not think it is reasonable to expect people to remember that
> they have to write "flags &= ~TO_DROP" in a longhand whenever they
> are adding new code that needs to do bit-fields, so even if this
> patch makes clang silent for the _current_ code, it will not stay
> that way. Something like
>
> #define FLIP_BIT_CLR(fld,bit) do { \
> typeof(fld) *x = &(fld); \
> *x = *x & (~(bit)); \
> } while (0)
>
> may be more palapable but not by a large margin.
>
> Yuck.
Double yuck. I meant palatable.
In any case, I see somebody reports that more recent clang does not
have this bug in the near-by message, so let's forget about this
issue.
Thanks.
^ permalink raw reply
* Re: Question re. git remote repository
From: David Lang @ 2013-01-17 0:21 UTC (permalink / raw)
To: Matt Seitz (matseitz); +Cc: git@vger.kernel.org, ishchis2@gmail.com
In-Reply-To: <A0DB01D693D8EF439496BC8B037A0AEF322098A4@xmb-rcd-x15.cisco.com>
On Wed, 16 Jan 2013, Matt Seitz (matseitz) wrote:
> "David Lang" <david@lang.hm> wrote in message news:<alpine.DEB.2.02.1301161459060.21503@nftneq.ynat.uz>...
>> But if you try to have one filesystem, with multiple people running git on their
>> machines against that shared filesystem, I would expect you to have all sorts of
>> problems.
>
> What leads you to think you will have problems?
>
> Why would there be more of a problem on a network file system as opposed to
> local file system that can be accessed by multiple users?
There are safety checks and synchronization primitives that work between
mulitiple users on one machine (where you can see what other processes are
running for example) that don't work with separate machines using a filesystem
> Linus seemed to think it should work:
>
> http://permalink.gmane.org/gmane.comp.version-control.git/122670
well, he knows git better than I do, but using git over NFS/CIFS is not the same
as saying that you have multiple users on different systems making changes.
In the link you point at, he says that you can have problems with some types of
actions. He points out things like git prune, but I would also say that there
are probably race conditions if you have two git processes that try to change
the HEAD to different things at the same time.
> And "git init" specifically has a "shared" option:
>
> --shared[=(false|true|umask|group|all|world|everybody|0xxx)]
>
> Specify that the git repository is to be shared amongst several users. This
> allows users belonging to the same group to push into that repository. When
> specified, the config variable "core.sharedRepository" is set so that files
> and directories under $GIT_DIR are created with the requested permissions.
> When not specified, git will use permissions reported by umask(2).
>
I think this is dealing with multiple users _reading_ a repository, not making
updates to it at the same time.
David Lang
^ 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