* Re: rerere with modified/deleted conflicts
From: Nicolas Pitre @ 2011-09-26 23:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v62ke293f.fsf@alter.siamese.dyndns.org>
On Mon, 26 Sep 2011, Junio C Hamano wrote:
> Nicolas Pitre <nico@fluxnic.net> writes:
>
> > Is this supported already?
>
> I do not think so, as rerere is about applying previous change using 3-way
> merge. A modify/delete conflict would mean you have only two stages, and
> while I can see somebody might want to say "I want to ignore further
> modifications to this dead path ignored and resolve in favor of deletion",
> it felt a bit too aggressive to my taste when I last worked on rerere.
Well, that's exactly the use case we have here this would be nice for.
But I wouldn't design it as "ignore any further changes" but rather
"ignore _this_ particular change to the file I want you to delete".
Nicolas
^ permalink raw reply
* Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links
From: Jakub Narebski @ 2011-09-26 23:19 UTC (permalink / raw)
To: Peter Stuge; +Cc: git
In-Reply-To: <1317060642-25488-1-git-send-email-peter@stuge.se>
Peter Stuge <peter@stuge.se> writes:
It really needs a proper commit message. Perhaps something like this:
The fixLinks() function in javascript-detection.js is supposed to
add 'js' query parameter with a value of 1 to each link that does
not have 'js' query parameter already.
However it didn't take into account the fact that URI can have
'fragment' part. It meant that:
1. URIs with fragment and 'js' query parameter, like e.g.
...foo?js=0#l199
were not recognized as having 'js' query parameter already.
2. The 'js' query parameter, in the form of either '?js=1' or ';js=1'
was appended at the end of URI, even if it included a fragment
(had a hash part). This lead to the incorrect links like this
...foo#l199?js=1
instead of adding query parameter as last part of query, but
before the fragment part, i.e.
...foo?js=1#l199
> Signed-off-by: Peter Stuge <peter@stuge.se>
For what it is worth it
Acked-by: Jakub Narebski <jnareb@gmail.com>
> ---
> gitweb/static/js/javascript-detection.js | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/static/js/javascript-detection.js b/gitweb/static/js/javascript-detection.js
> index 93dd2bd..003acd1 100644
> --- a/gitweb/static/js/javascript-detection.js
> +++ b/gitweb/static/js/javascript-detection.js
> @@ -16,7 +16,7 @@
> * and other reasons to not add 'js=1' param at the end of link
> * @constant
> */
> -var jsExceptionsRe = /[;?]js=[01]$/;
> +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;
>
> /**
> * Add '?js=1' or ';js=1' to the end of every link in the document
> @@ -33,9 +33,9 @@ function fixLinks() {
> var allLinks = document.getElementsByTagName("a") || document.links;
> for (var i = 0, len = allLinks.length; i < len; i++) {
> var link = allLinks[i];
> - if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;
> - link.href +=
> - (link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';
> + if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;
> + link.href = link.href.replace(/(#|$)/,
> + (link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1$1');
> }
> }
> }
> --
> 1.7.4.1.343.ga91df.dirty
>
--
Jakub Narębski mailto:jnareb@fuw.edu.pl
ZTHiL IFT UW http://info.fuw.edu.pl/~jnareb/
^ permalink raw reply
* Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links
From: Peter Stuge @ 2011-09-26 23:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vaa9q29ag.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> > var link = allLinks[i];
> > - if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;
> > - link.href +=
> > - (link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';
> > + if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;
>
> Let's not repeat the regexp in the comment (badness you inherited
> from the original).
I agree. I chose to follow style.
> Regarding the "we already have the js=0 or js=1 in the URL" check here...
>
> > +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;
>
> ... I am wondering who guarantees that this js=[01] is the last parameter
> before the fragment identifier. The answer obviously is "the way the
> current code is written using replace() method on link.href", but that is
> somewhat disturbing, because it is not clear what should happen, short of
> total rewrite of the code around this, when somebody needs to include
> another variable, say xx=[01], just like the js=[01] you are fixing here,
> in the resulting URL. In other words, this fixLinks() logic does not seem
> to scale and also looks brittle.
At first glance indeed. But I think it might be fine, because the
purpose of this function is only to use javascript in order to
indicate to the server that the client supports javascript. This is
somewhat an odd case, the link rewriting is pretty much needed only
for this one value so it doesn't really have to scale. But I have
nothing against say removing (#.*)?$ and thus only matching
[?;]js=[01] in link.href.
> The patch itself looks correct as a short-term fix, though.
Thanks, I figured if it was OK before then at least it would be
better now.
//Peter
^ permalink raw reply
* Re: [BUG?] git fetch -p -t prunes all non-tag refs
From: Junio C Hamano @ 2011-09-26 23:16 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: mathstuf, git
In-Reply-To: <1317078667.5579.13.camel@centaur.lab.cmartin.tk>
Carlos Martín Nieto <cmn@elego.de> writes:
> On Mon, 2011-09-26 at 15:30 -0700, Junio C Hamano wrote:
>> Ben Boeckel <mathstuf@gmail.com> writes:
>>
>> > When the --prune and --tags options are given to git fetch together, all
>> > non-tag refs are pruned because only tags are looked at and when pruning
>> > it appears as if the branches have disappeared and are therefore deleted
>> > locally.
>>
>> I would call that a bug, and it is not limited to the use of "--tags". For
>> example, I suspect that
>>
>> $ git fetch --prune origin refs/heads/master:refs/remotes/origin/master
>>
>> would prune remote tracking branches for "origin" other than "master".
>
> This should fix it (in a way). Let's agree that it's a bad idea and
> complain to the user.
That might be a reasonable short-term safety measure, but in the longer
term I think we should fix it properly. We are already learning "what are
the refs the remote side currently has" from the transport and the right
fix ought to be to use that original information, not the version filtered
for the use of the primary objective of fetch, which is to only fetch what
the user asked for.
^ permalink raw reply
* Re: [BUG?] git fetch -p -t prunes all non-tag refs
From: Carlos Martín Nieto @ 2011-09-26 23:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: mathstuf, git
In-Reply-To: <7vehz30wdy.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]
On Mon, 2011-09-26 at 15:30 -0700, Junio C Hamano wrote:
> Ben Boeckel <mathstuf@gmail.com> writes:
>
> > When the --prune and --tags options are given to git fetch together, all
> > non-tag refs are pruned because only tags are looked at and when pruning
> > it appears as if the branches have disappeared and are therefore deleted
> > locally.
>
> I would call that a bug, and it is not limited to the use of "--tags". For
> example, I suspect that
>
> $ git fetch --prune origin refs/heads/master:refs/remotes/origin/master
>
> would prune remote tracking branches for "origin" other than "master".
This should fix it (in a way). Let's agree that it's a bad idea and
complain to the user. Looking at the surrounding code, I can't find
anything obvious that would break, and `git fetch --prune --multiple a
b` is allowed.
Patch on top of maint.
--- 8< ---
Subject: [PATCH] fetch: disallow --prune in combination with tags or refspecs
Pruning shouldn't be used when other options limit the references that
should be taken into account. For example
git fetch --prune --tags origin
git fetch --prune origin refs/heads/*:refs/remotes/*
Both these commands would remove references which do still exist in
the remote.
Print an error and exit if prune is selected at the same time as
either tags are selected or a refspec is given.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
builtin/fetch.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e422ced..158b20a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -967,6 +967,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
if (!add_remote_or_group(argv[i], &list))
die(_("No such remote or remote group: %s"), argv[i]);
result = fetch_multiple(&list);
+ } else if (prune && (argc == 2 || tags != TAGS_DEFAULT)) {
+ /* The if (multiple) is above so we don't report multiple remotes as an error */
+ die(_("Pruning and limiting refs is not compatible"));
} else {
/* Single remote or group */
(void) add_remote_or_group(argv[0], &list);
--
1.7.5.2.354.g349bf
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* Re: rerere with modified/deleted conflicts
From: Junio C Hamano @ 2011-09-26 23:10 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: git
In-Reply-To: <alpine.LFD.2.00.1109261904340.2718@xanadu.home>
Nicolas Pitre <nico@fluxnic.net> writes:
> Is this supported already?
I do not think so, as rerere is about applying previous change using 3-way
merge. A modify/delete conflict would mean you have only two stages, and
while I can see somebody might want to say "I want to ignore further
modifications to this dead path ignored and resolve in favor of deletion",
it felt a bit too aggressive to my taste when I last worked on rerere.
^ permalink raw reply
* Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links
From: Junio C Hamano @ 2011-09-26 23:06 UTC (permalink / raw)
To: Peter Stuge; +Cc: git
In-Reply-To: <1317060642-25488-1-git-send-email-peter@stuge.se>
Peter Stuge <peter@stuge.se> writes:
> -var jsExceptionsRe = /[;?]js=[01]$/;
> +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;
>
> /**
> * Add '?js=1' or ';js=1' to the end of every link in the document
> @@ -33,9 +33,9 @@ function fixLinks() {
> var allLinks = document.getElementsByTagName("a") || document.links;
> for (var i = 0, len = allLinks.length; i < len; i++) {
> var link = allLinks[i];
> - if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01]$/;
> - link.href +=
> - (link.href.indexOf('?') === -1 ? '?' : ';') + 'js=1';
> + if (!jsExceptionsRe.test(link)) { // =~ /[;?]js=[01](#.*)?$/;
Let's not repeat the regexp in the comment (badness you inherited from the
original).
Regarding the "we already have the js=0 or js=1 in the URL" check here...
> +var jsExceptionsRe = /[;?]js=[01](#.*)?$/;
... I am wondering who guarantees that this js=[01] is the last parameter
before the fragment identifier. The answer obviously is "the way the
current code is written using replace() method on link.href", but that is
somewhat disturbing, because it is not clear what should happen, short of
total rewrite of the code around this, when somebody needs to include
another variable, say xx=[01], just like the js=[01] you are fixing here,
in the resulting URL. In other words, this fixLinks() logic does not seem
to scale and also looks brittle.
The patch itself looks correct as a short-term fix, though.
Thanks.
^ permalink raw reply
* rerere with modified/deleted conflicts
From: Nicolas Pitre @ 2011-09-26 23:06 UTC (permalink / raw)
To: git
Is this supported already?
Nicolas
^ permalink raw reply
* Re: [BUG?] git fetch -p -t prunes all non-tag refs
From: Ben Boeckel @ 2011-09-26 22:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vehz30wdy.fsf@alter.siamese.dyndns.org>
On Mon, Sep 26, 2011 at 15:30:33 -0700, Junio C Hamano wrote:
> I would call that a bug, and it is not limited to the use of "--tags". For
> example, I suspect that
>
> $ git fetch --prune origin refs/heads/master:refs/remotes/origin/master
>
> would prune remote tracking branches for "origin" other than "master".
Indeed it does. Given that, should --prune be an error when a refspec is
given or when in combination with --tags?
--Ben
^ permalink raw reply
* Re: Git is not scalable with too many refs/*
From: Julian Phillips @ 2011-09-26 22:30 UTC (permalink / raw)
To: Martin Fick; +Cc: git
In-Reply-To: <201109261539.33437.mfick@codeaurora.org>
On Mon, 26 Sep 2011 15:39:33 -0600, Martin Fick wrote:
> On Monday, September 26, 2011 02:28:53 pm Julian Phillips
> wrote:
>> On Mon, 26 Sep 2011 14:01:38 -0600, Martin Fick wrote:
>> -- snip --
>>
>> > So, maybe you are correct, maybe my repo is the corner
>> > case? Is a repo which needs to be gced considered a
>> > corner case? Should git be able to detect that the
>> > repo is so in desperate need of gcing? Is it normal
>> > for git to need to gc right after a clone and then
>> > fetching ~100K refs?
>>
>> Were you 100k refs packed before the gc? If not, perhaps
>> your refs are causing a lot of trouble for the merge
>> sort? They will be written out sorted to the
>> packed-refs file, so the merge sort won't have to do any
>> real work when loading them after that...
>
> I am not sure how to determine that (?), but I think they
> were packed. Under .git/objects/pack there were 2 large
> files, both close to 500MB. Those 2 files constituted most
> of the space in the repo (I was wrong about the repo sizes,
> that included the working dir, so think about half the
> quoted sizes for all of .git). So does that mean it is
> mostly packed? Aside from the pack and idx files, there was
> nothing else under the objects dir. After gcing, it is down
> to just one ~500MB pack file.
If refs are listed under .git/refs/... they are unpacked, if they are
listed in .git/packed-refs they are packed.
They can be in both if updated since the last pack.
>> > I am not sure what is right here, if this patch makes a
>> > repo which needs gcing degrade 5 to 10 times worse
>> > than the benefit of this patch, it still seems
>> > questionable to me.
>>
>> Well - it does this _for your repo_, that doesn't
>> automatically mean that it does generally, or
>> frequently.
>
> Oh, def agreed! I just didn't want to discount it so quickly
> as being a corner case.
>
>
>> For instance, none of my normal repos that
>> have a lot of refs are Gerrit ones, and I wouldn't be
>> surprised if they benefitted from the merge sort
>> (assuming that I am right that the merge sort is taking
>> a long time on your gerrit refs).
>>
>> Besides, you would be better off running gc, and thus
>> getting the benefit too.
>
> Agreed, which is why I was asking if git should have noticed
> my "degenerate" case and auto gced? But hopefully, there is
> an actual bug here somewhere and we both will get to eat our
> cake. :)
I think automatic gc is currently only triggered by unpacked objects,
not unpacked refs ... perhaps the auto-gc should cover refs too?
>> >> Random thought. What happens to the with compression
>> >> case if you leave the commit in, but add a sleep(15)
>> >> to the end of sort_refs_list?
>> >
>> > Why, what are you thinking? Hmm, I am trying this on
>> > the non gced repo and it doesn't seem to be completing
>> > (no cpu usage)! It appears that perhaps it is being
>> > called many times (the sleeping would explain no cpu
>> > usage)?!? This could be a real problem, this should
>> > only get called once right?
>>
>> I was just wondering if the time taken to get the refs
>> was changing the interaction with something else. Not
>> very likely, but ...
>>
>> I added a print statement, and it was called four times
>> when I had unpacked refs, and once with packed. So,
>> maybe you are hitting some nasty case with unpacked
>> refs. If you use a print statement instead of a sleep,
>> how many times does sort_refs_lists get called in your
>> unpacked case? It may well also be worth calculating
>> the time taken to do the sort.
>
> In my case it was called 18785 times! Any other tests I
> should run?
That's a lot of sorts. I really can't see why there would need to be
more than one ...
I've created a new test repo, using a more complicated method to
construct the 100k refs, and it took ~40m to run "git branch" instead of
the 1.2s for the previous repo. So, I think the ref naming pattern used
by Gerrit is definitely triggering something odd. However, progress is
a bit slow - now that it takes over 1/2 an hour to try things out ...
--
Julian
^ permalink raw reply
* Re: [BUG?] git fetch -p -t prunes all non-tag refs
From: Junio C Hamano @ 2011-09-26 22:30 UTC (permalink / raw)
To: mathstuf; +Cc: git
In-Reply-To: <20110926184739.GA11745@erythro.kitwarein.com>
Ben Boeckel <mathstuf@gmail.com> writes:
> When the --prune and --tags options are given to git fetch together, all
> non-tag refs are pruned because only tags are looked at and when pruning
> it appears as if the branches have disappeared and are therefore deleted
> locally.
I would call that a bug, and it is not limited to the use of "--tags". For
example, I suspect that
$ git fetch --prune origin refs/heads/master:refs/remotes/origin/master
would prune remote tracking branches for "origin" other than "master".
^ permalink raw reply
* Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links
From: Peter Stuge @ 2011-09-26 22:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vipof0zx0.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> > I thought subject together with change would be clear enough. :)
> >
> >
> >> Explanation of what you are fixing is totally lacking.
> >
> > The subject sums it up, if briefly.
>
> ... Sorry, that is not what I meant.
>
> You don't have to explain these to *me* specifically as a response
> to this thread. What I meant was that your patch should have these
> necessary descriptions in its proposed commit log message.
IMO not so neccessary if one knows a little web and javascript, which
is probably likely for a gitweb change..
It's a simple fix of links broken by manual URI manipulation that
didn't consider fragments. Is the subject description really not
enough?
//Peter
^ permalink raw reply
* Re: Thoughts on gitk's memory footprint over linux-2.6.git
From: Martin Langhoff @ 2011-09-26 22:07 UTC (permalink / raw)
To: Elijah Newren; +Cc: Git Mailing List
In-Reply-To: <CABPp-BHo5qKQVFRzFpoyt6dZZ3=UqVAtSjVy0uRDWnA+ASsBPw@mail.gmail.com>
On Mon, Sep 26, 2011 at 6:02 PM, Elijah Newren <newren@gmail.com> wrote:
> If you only want to look at a couple commits, you could tell gitk that:
> gitk -N
I know I can -- I've done my bit of git hacking but it was long ago so
if you use your -N parameter you won't see it ;-)
I argue that -n 10000 should be the default -- most of the time you
open git to see recent commits, not that 2.6.12-rc3 commit. Of course,
it cannot actually be the default, because currently there's no way to
scroll past that limiter.
It's a hard limit, and that's not useful.
Now, git internally is very smart about using sliding windows over
large datasets. The porcelain tools don't try to be so smart, but
we're at a point where, as a user, I really wish gitk had some of that
magic.
cheers,
m
--
martin.langhoff@gmail.com
martin@laptop.org -- Software Architect - OLPC
- ask interesting questions
- don't get distracted with shiny stuff - working code first
- http://wiki.laptop.org/go/User:Martinlanghoff
^ permalink raw reply
* Re: Thoughts on gitk's memory footprint over linux-2.6.git
From: Elijah Newren @ 2011-09-26 22:02 UTC (permalink / raw)
To: Martin Langhoff; +Cc: Git Mailing List
In-Reply-To: <CACPiFC+T1EZ1CSakQxsYZhsnHc-ZsN1-=tpoi-NaQSdpU5Yxkg@mail.gmail.com>
On Mon, Sep 26, 2011 at 1:38 PM, Martin Langhoff
<martin.langhoff@gmail.com> wrote:
> In an odd turn of affairs, I am managing a bit of kernel development,
> and even writing the odd patch myself. As usual, gitk is excellent to
> visualize what's new in the tree when developers in the team commit
> and push new stuff.
>
> However, I find it extremely annoying over the kernel tree, due to its
> memory footprint. It is not the only thing I am running, (Chrome
> Browser, Gnome3, Firefox, many gnome Terminal windows, emacs), and
> given that I am looking at "just a couple of commits" I don't feel
> opening a few gitk instances should be problematic... except that it
> is.
If you only want to look at a couple commits, you could tell gitk that:
gitk -N
where N is some small integer -- or even 10000, as you suggest. Or
use other rev-list arguments, such as
gitk v2.6.38..master
gitk and 'git log' accept basically the same arguments, so look at the
help for git log if you want to find other example ways to slice and
dice what pieces of history to view.
Hope that helps,
Elijah
^ permalink raw reply
* [PATCH] git-submodule: a small fix
From: Roy Liu @ 2011-09-26 22:00 UTC (permalink / raw)
To: git
In git-submodule.sh, the "url" variable may contain a stale value from
the previous loop iteration, so clear it.
--- git-submodule.sh.orig 2011-09-26 17:50:45.000000000 -0400
+++ git-submodule.sh 2011-09-26 17:51:18.000000000 -0400
@@ -370,6 +370,8 @@
esac
git config submodule."$name".url "$url" ||
die "Failed to register url for submodule path '$path'"
+ else
+ url=""
fi
# Copy "update" setting when it is not set yet
^ permalink raw reply
* Re: Git is not scalable with too many refs/*
From: Martin Fick @ 2011-09-26 21:52 UTC (permalink / raw)
To: Julian Phillips; +Cc: git
In-Reply-To: <201109261539.33437.mfick@codeaurora.org>
On Monday, September 26, 2011 03:39:33 pm Martin Fick wrote:
> On Monday, September 26, 2011 02:28:53 pm Julian Phillips
> wrote:
> > >> Random thought. What happens to the with
> > >> compression case if you leave the commit in, but
> > >> add a sleep(15) to the end of sort_refs_list?
> > >
> > > Why, what are you thinking? Hmm, I am trying this on
> > > the non gced repo and it doesn't seem to be
> > > completing (no cpu usage)! It appears that perhaps
> > > it is being called many times (the sleeping would
> > > explain no cpu usage)?!? This could be a real
> > > problem, this should only get called once right?
> >
> > I was just wondering if the time taken to get the refs
> > was changing the interaction with something else. Not
> > very likely, but ...
> >
> > I added a print statement, and it was called four times
> > when I had unpacked refs, and once with packed. So,
> > maybe you are hitting some nasty case with unpacked
> > refs. If you use a print statement instead of a sleep,
> > how many times does sort_refs_lists get called in your
> > unpacked case? It may well also be worth calculating
> > the time taken to do the sort.
>
> In my case it was called 18785 times! Any other tests I
> should run?
Gerrit stores the changes in directories under refs/changes
named after the last 2 digits of the change. Then under
each change it stores each patchset. So it looks like this:
refs/changes/dd/change_num/ps_num
I noticed that:
ls refs/changes/* | wc -l
-> 18876
somewhat close, but not super close to 18785, I am not sure
if that is a clue. It's almost like each change is causing
a re-sort,
-Martin
--
Employee of Qualcomm Innovation Center, Inc. which is a
member of Code Aurora Forum
^ permalink raw reply
* Re: [PATCH/RFCv1] git-p4: handle files with shell metacharacters
From: Pete Wyckoff @ 2011-09-26 21:47 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, vitor.hda
In-Reply-To: <1317072555-23438-2-git-send-email-luke@diamand.org>
luke@diamand.org wrote on Mon, 26 Sep 2011 22:29 +0100:
> git-p4 used to simply pass strings into system() and popen(), and
> relied on the shell doing the necessary expansion. This though meant
> that shell metacharacters in file names would be corrupted - for
> example files with $ or space in them.
>
> Switch to using subprocess.Popen() and friends, and pass in explicit
> arrays in the places where it matters. This then avoids needing shell
> expansion.
>
> Add trivial helper functions for some common perforce operations. Add
> test case.
This is great. Some code comments below. Could you resend an
RFCv2? I'll fixup your test case along with mine later, and send
you the patch just for that.
-- Pete
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
> contrib/fast-import/git-p4 | 174 +++++++++++++++++++++++++---------------
> t/t9800-git-p4.sh | 2 +-
> t/t9803-git-shell-metachars.sh | 70 ++++++++++++++++
> 3 files changed, 179 insertions(+), 67 deletions(-)
> create mode 100755 t/t9803-git-shell-metachars.sh
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 782b891..be67d30 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -14,7 +14,6 @@ import re
>
> verbose = False
>
> -
> def p4_build_cmd(cmd):
> """Build a suitable p4 command line.
>
> @@ -22,36 +21,40 @@ def p4_build_cmd(cmd):
> location. It means that hooking into the environment, or other configuration
> can be done more easily.
> """
> - real_cmd = "%s " % "p4"
> + real_cmd = ["p4"]
>
> user = gitConfig("git-p4.user")
> if len(user) > 0:
> - real_cmd += "-u %s " % user
> + real_cmd += ["-u",user]
>
> password = gitConfig("git-p4.password")
> if len(password) > 0:
> - real_cmd += "-P %s " % password
> + real_cmd += ["-P", password]
>
> port = gitConfig("git-p4.port")
> if len(port) > 0:
> - real_cmd += "-p %s " % port
> + real_cmd += ["-p", port]
>
> host = gitConfig("git-p4.host")
> if len(host) > 0:
> - real_cmd += "-h %s " % host
> + real_cmd += ["-h", host]
>
> client = gitConfig("git-p4.client")
> if len(client) > 0:
> - real_cmd += "-c %s " % client
> + real_cmd += ["-c", client]
> +
>
> - real_cmd += "%s" % (cmd)
> + if isinstance(cmd,basestring):
> + real_cmd = ' '.join(real_cmd) + ' ' + cmd
> + else:
> + real_cmd += cmd
> if verbose:
> - print real_cmd
> + print ' '.join(real_cmd)
> return real_cmd
Okay, so it returns a string if given a string, only. Would be
nice if we could kill off the string usages eventually?
The verbose print ' '.join(real_cmd) will look quite funny if
it real_cmd is a string.
> def chdir(dir):
> - if os.name == 'nt':
> - os.environ['PWD']=dir
> + # P4 uses the PWD environment variable rather than getcwd(), it would appear!
> + os.environ['PWD']=dir
> os.chdir(dir)
Interesting. Separate unrelated fix? Both "nt" and other OSes?
> def die(msg):
> @@ -65,9 +68,12 @@ def write_pipe(c, str):
> if verbose:
> sys.stderr.write('Writing pipe: %s\n' % c)
Needs str(c) like in read_pipe()?
> - pipe = os.popen(c, 'w')
> + p = subprocess.Popen(c, shell=isinstance(c,basestring),
> + stdin=subprocess.PIPE)
> + pipe = p.stdin
> val = pipe.write(str)
> - if pipe.close():
> + pipe.close();
Stray semicolon.
> + if p.wait():
> die('Command failed: %s' % c)
> return val
> @@ -78,11 +84,13 @@ def p4_write_pipe(c, str):
>
> def read_pipe(c, ignore_error=False):
> if verbose:
> - sys.stderr.write('Reading pipe: %s\n' % c)
> + sys.stderr.write('Reading pipe: %s\n' % str(c))
>
> - pipe = os.popen(c, 'rb')
> + expand = isinstance(c,basestring)
> + p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
> + pipe = p.stdout
> val = pipe.read()
> - if pipe.close() and not ignore_error:
> + if p.wait() and not ignore_error:
> die('Command failed: %s' % c)
Can this be made more symmetric with read_pipe()? "expand",
order of args.
> return val
> @@ -93,11 +101,13 @@ def p4_read_pipe(c, ignore_error=False):
>
> def read_pipe_lines(c):
> if verbose:
> - sys.stderr.write('Reading pipe: %s\n' % c)
> - ## todo: check return status
> - pipe = os.popen(c, 'rb')
> + sys.stderr.write('Reading pipe: %s\n' % str(c))
> +
> + expand = isinstance(c, basestring)
> + p = subprocess.Popen(c, shell=expand, stdout=subprocess.PIPE)
> + pipe = p.stdout
> val = pipe.readlines()
> - if pipe.close():
> + if pipe.close() or p.wait():
> die('Command failed: %s' % c)
>
> return val
> @@ -108,15 +118,37 @@ def p4_read_pipe_lines(c):
> return read_pipe_lines(real_cmd)
>
> def system(cmd):
> + expand = isinstance(cmd,basestring)
> if verbose:
> - sys.stderr.write("executing %s\n" % cmd)
> - if os.system(cmd) != 0:
> - die("command failed: %s" % cmd)
> + sys.stderr.write("executing %s\n" % str(cmd))
> + subprocess.check_call(cmd, shell=expand)
>
> def p4_system(cmd):
> """Specifically invoke p4 as the system command. """
> real_cmd = p4_build_cmd(cmd)
> - return system(real_cmd)
> + expand = isinstance(real_cmd, basestring)
> + subprocess.check_call(real_cmd, shell=expand)
> +
> +def p4_integrate(src, dest):
> + p4_system(["integrate", "-Dt", src, dest])
> +
> +def p4_sync(path):
> + p4_system(["sync", path])
> +
> +def p4_add(f):
> + p4_system(["add", f])
> +
> +def p4_delete(f):
> + p4_system(["delete", f])
> +
> +def p4_edit(f):
> + p4_system(["edit", f])
> +
> +def p4_revert(f):
> + p4_system(["revert", f])
> +
> +def p4_reopen(type, file):
> + p4_system(["reopen", "-t", type, file])
These look cleaner. We almost need a class for this
p4 interface stuff. Maybe a job for a later refactoring.
> @@ -1365,9 +1408,9 @@ class P4Sync(Command, P4UserMap):
> def streamP4FilesCbSelf(entry):
> self.streamP4FilesCb(entry)
>
> - p4CmdList("-x - print",
> - '\n'.join(['%s#%s' % (f['path'], f['rev'])
> - for f in filesToRead]),
> + fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
> +
> + p4CmdList(["print"] + fileArgs,
> cb=streamP4FilesCbSelf)
I think this used "-x -" because when there are too many
fileArgs, the kernel command line couldn't hold them all. So
this trick feeds them on stdin instead.
> @@ -1429,7 +1472,7 @@ class P4Sync(Command, P4UserMap):
> if self.verbose:
> print "Change %s is labelled %s" % (change, labelDetails)
>
> - files = p4CmdList("files " + ' '.join (["%s...@%s" % (p, change)
> + files = p4CmdList(["files"] + (["%s...@%s" % (p, change)
> for p in branchPrefixes]))
Spurious parens around the list comprehension; confusing.
> @@ -1478,9 +1521,9 @@ class P4Sync(Command, P4UserMap):
> newestChange = 0
> if self.verbose:
> print "Querying files for label %s" % label
> - for file in p4CmdList("files "
> - + ' '.join (["%s...@%s" % (p, label)
> - for p in self.depotPaths])):
> + for file in p4CmdList(["files"] +
> + (["%s...@%s" % (p, label)
> + for p in self.depotPaths])):
Here too.
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index bb89b63..12f374f 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -150,7 +150,7 @@ test_expect_success 'preserve users' '
> git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
> git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
> git config git-p4.skipSubmitEditCheck true &&
> - P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
> + P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --verbose --preserve-user &&
> p4_check_commit_author file1 alice &&
> p4_check_commit_author file2 bob
> '
Debugging change?
> diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
> new file mode 100755
> index 0000000..1143491
> --- /dev/null
> +++ b/t/t9803-git-shell-metachars.sh
> @@ -0,0 +1,70 @@
> +#!/bin/sh
> +
> +test_description='git-p4 transparency to shell metachars in filenames'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> + kill_p4d || : &&
> + start_p4d &&
> + cd "$TRASH_DIRECTORY"
> +'
Excellent, a test case. I'm still waiting 1.7.7 to resubmit my
series, but part of it includes fixing the test cases. I'll mail
a separate patch that does similar transformations to this.
Easier just to do it all at once.
> +test_expect_success 'init depot' '
> + (
> + cd "$cli" &&
> + echo file1 >file1 &&
> + p4 add file1 &&
> + p4 submit -d "file1"
> + )
> +'
> +
> +test_expect_success 'shell metachars in filenames' '
> + "$GITP4" clone --dest="$git" //depot &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git" &&
> + git config git-p4.skipSubmitEditCheck true &&
> + echo f1 >foo\$bar &&
> + git add foo\$bar &&
> + echo f2 >"file with spaces" &&
> + git add "file with spaces" &&
> + P4EDITOR=touch git commit -m "add files" &&
> + "$GITP4" submit --verbose &&
> + cd "$cli" &&
> + p4 sync ... &&
> + ls -l "file with spaces" &&
> + ls -l "foo\$bar"
> + )
> +'
> +
> +check_missing() {
> + for i in $*; do
> + if [ -f $i ]; then
> + echo $i found but should be missing 1>&2
> + exit 1
> + fi
> + done
> +}
> +
> +test_expect_success 'deleting with shell metachars' '
> + "$GITP4" clone --dest="$git" //depot &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git" &&
> + git config git-p4.skipSubmitEditCheck true &&
> + git rm foo\$bar &&
> + git rm file\ with\ spaces &&
> + P4EDITOR=touch git commit -m "remove files" &&
> + "$GITP4" submit --verbose
> + cd "$cli" &&
> + p4 sync ... &&
> + check_missing "file with spaces" foo\$bar
> + )
> +'
> +
> +test_expect_success 'kill p4d' '
> + kill_p4d
> +'
> +
> +test_done
> --
> 1.7.6.347.g4db0d
>
^ permalink raw reply
* Re: Can a git changeset be created with no parent
From: Carlos Martín Nieto @ 2011-09-26 21:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: vra5107, git
In-Reply-To: <7vaa9r2jii.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]
On Mon, 2011-09-26 at 12:25 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
>
> > On Sun, 2011-09-25 at 07:15 -0700, vra5107 wrote:
> >> Hi
> >>
> >> I am currently in the process of converting a large hg repository.
> >> One of the changesets has no parents assigned. So to mirror that is it
> >> possible to create a git changeset that doesnot have a parent ?
> >
> > They're called commits in git, and yes it's possible. They are called
> > orphan commits and it's what you get when you do the first commit in the
>
> Just to set the terminology straight, s/orphan/root/;
Ah, quite.
>
> > repository.
> >
> > You can do this with 'git checkout --orphan somebranch'. Notice that the
>
> The orphan here refers to the fact that the next commit will not be a
> child of the current commit. The resulting one is a "root" commit.
The manpage mentions that an orphan branch is created. I guess that's
where I got my "orphan commit" name.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: Git is not scalable with too many refs/*
From: Martin Fick @ 2011-09-26 21:39 UTC (permalink / raw)
To: Julian Phillips; +Cc: git
In-Reply-To: <9ae990f15489d7b51a172d08e63ca458@quantumfyre.co.uk>
On Monday, September 26, 2011 02:28:53 pm Julian Phillips
wrote:
> On Mon, 26 Sep 2011 14:01:38 -0600, Martin Fick wrote:
> -- snip --
>
> > So, maybe you are correct, maybe my repo is the corner
> > case? Is a repo which needs to be gced considered a
> > corner case? Should git be able to detect that the
> > repo is so in desperate need of gcing? Is it normal
> > for git to need to gc right after a clone and then
> > fetching ~100K refs?
>
> Were you 100k refs packed before the gc? If not, perhaps
> your refs are causing a lot of trouble for the merge
> sort? They will be written out sorted to the
> packed-refs file, so the merge sort won't have to do any
> real work when loading them after that...
I am not sure how to determine that (?), but I think they
were packed. Under .git/objects/pack there were 2 large
files, both close to 500MB. Those 2 files constituted most
of the space in the repo (I was wrong about the repo sizes,
that included the working dir, so think about half the
quoted sizes for all of .git). So does that mean it is
mostly packed? Aside from the pack and idx files, there was
nothing else under the objects dir. After gcing, it is down
to just one ~500MB pack file.
> > I am not sure what is right here, if this patch makes a
> > repo which needs gcing degrade 5 to 10 times worse
> > than the benefit of this patch, it still seems
> > questionable to me.
>
> Well - it does this _for your repo_, that doesn't
> automatically mean that it does generally, or
> frequently.
Oh, def agreed! I just didn't want to discount it so quickly
as being a corner case.
> For instance, none of my normal repos that
> have a lot of refs are Gerrit ones, and I wouldn't be
> surprised if they benefitted from the merge sort
> (assuming that I am right that the merge sort is taking
> a long time on your gerrit refs).
>
> Besides, you would be better off running gc, and thus
> getting the benefit too.
Agreed, which is why I was asking if git should have noticed
my "degenerate" case and auto gced? But hopefully, there is
an actual bug here somewhere and we both will get to eat our
cake. :)
> >> Random thought. What happens to the with compression
> >> case if you leave the commit in, but add a sleep(15)
> >> to the end of sort_refs_list?
> >
> > Why, what are you thinking? Hmm, I am trying this on
> > the non gced repo and it doesn't seem to be completing
> > (no cpu usage)! It appears that perhaps it is being
> > called many times (the sleeping would explain no cpu
> > usage)?!? This could be a real problem, this should
> > only get called once right?
>
> I was just wondering if the time taken to get the refs
> was changing the interaction with something else. Not
> very likely, but ...
>
> I added a print statement, and it was called four times
> when I had unpacked refs, and once with packed. So,
> maybe you are hitting some nasty case with unpacked
> refs. If you use a print statement instead of a sleep,
> how many times does sort_refs_lists get called in your
> unpacked case? It may well also be worth calculating
> the time taken to do the sort.
In my case it was called 18785 times! Any other tests I
should run?
-Martin
--
Employee of Qualcomm Innovation Center, Inc. which is a
member of Code Aurora Forum
^ permalink raw reply
* Re: [PATCH/RFC 1/2] is_url: Remove redundant assignment
From: Jeff King @ 2011-09-26 21:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ilari Liusvaara, Tay Ray Chuan, Pang Yan Han, git,
Shawn O. Pearce, Sitaram Chamarty, Johannes Schindelin
In-Reply-To: <7vsjnj455l.fsf@alter.siamese.dyndns.org>
[+cc Ilari, who wrote the code originally]
On Mon, Sep 26, 2011 at 09:52:54AM -0700, Junio C Hamano wrote:
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
> > On Sun, Sep 25, 2011 at 1:06 PM, Pang Yan Han <pangyanhan@gmail.com> wrote:
> >> Signed-off-by: Pang Yan Han <pangyanhan@gmail.com>
> >> ---
> >> url.c | 1 -
> >> 1 files changed, 0 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/url.c b/url.c
> >> index 3e06fd3..d2e17e6 100644
> >> --- a/url.c
> >> +++ b/url.c
> >> @@ -22,7 +22,6 @@ int is_url(const char *url)
> >>
> >> if (!url)
> >> return 0;
> >> - url2 = url;
> >> first_slash = strchr(url, '/');
> >>
> >> /* Input with no slash at all or slash first can't be URL. */
> >
> > Looks correct. Perhaps you could mention in the patch message that
> >
> > There are no operations on url2 until another assignment to it later
> > at line 41.
>
> The looks correct, so I'll queue it, but it looks like that the function
> is implemented in an overly complicated way.
>
> Why aren't we checking from left to right in a single pass, perhaps like
> this?
>
> /* Make sure it is of form "scheme://something" */
> int is_url(const char *url)
> {
> /* Is "scheme" part reasonable? */
> if (!url || !is_urlschemechar(1, *url++))
> return 0;
> while (*url && *url != ':') {
> if (!is_urlschemechar(0, *url++))
> return 0;
> }
> /* We've seen "scheme"; we want colon-slash-slash */
> return (url[0] == ':' && url[1] == '/' && url[2] == '/');
> }
That looks right to me, and is way more readable.
-Peff
^ permalink raw reply
* [PATCH/RFCv1] git-p4: handle files with shell metacharacters
From: Luke Diamand @ 2011-09-26 21:29 UTC (permalink / raw)
To: git; +Cc: pw, vitor.hda, Luke Diamand
In-Reply-To: <1317072555-23438-1-git-send-email-luke@diamand.org>
git-p4 used to simply pass strings into system() and popen(), and
relied on the shell doing the necessary expansion. This though meant
that shell metacharacters in file names would be corrupted - for
example files with $ or space in them.
Switch to using subprocess.Popen() and friends, and pass in explicit
arrays in the places where it matters. This then avoids needing shell
expansion.
Add trivial helper functions for some common perforce operations. Add
test case.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
contrib/fast-import/git-p4 | 174 +++++++++++++++++++++++++---------------
t/t9800-git-p4.sh | 2 +-
t/t9803-git-shell-metachars.sh | 70 ++++++++++++++++
3 files changed, 179 insertions(+), 67 deletions(-)
create mode 100755 t/t9803-git-shell-metachars.sh
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 782b891..be67d30 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -14,7 +14,6 @@ import re
verbose = False
-
def p4_build_cmd(cmd):
"""Build a suitable p4 command line.
@@ -22,36 +21,40 @@ def p4_build_cmd(cmd):
location. It means that hooking into the environment, or other configuration
can be done more easily.
"""
- real_cmd = "%s " % "p4"
+ real_cmd = ["p4"]
user = gitConfig("git-p4.user")
if len(user) > 0:
- real_cmd += "-u %s " % user
+ real_cmd += ["-u",user]
password = gitConfig("git-p4.password")
if len(password) > 0:
- real_cmd += "-P %s " % password
+ real_cmd += ["-P", password]
port = gitConfig("git-p4.port")
if len(port) > 0:
- real_cmd += "-p %s " % port
+ real_cmd += ["-p", port]
host = gitConfig("git-p4.host")
if len(host) > 0:
- real_cmd += "-h %s " % host
+ real_cmd += ["-h", host]
client = gitConfig("git-p4.client")
if len(client) > 0:
- real_cmd += "-c %s " % client
+ real_cmd += ["-c", client]
+
- real_cmd += "%s" % (cmd)
+ if isinstance(cmd,basestring):
+ real_cmd = ' '.join(real_cmd) + ' ' + cmd
+ else:
+ real_cmd += cmd
if verbose:
- print real_cmd
+ print ' '.join(real_cmd)
return real_cmd
def chdir(dir):
- if os.name == 'nt':
- os.environ['PWD']=dir
+ # P4 uses the PWD environment variable rather than getcwd(), it would appear!
+ os.environ['PWD']=dir
os.chdir(dir)
def die(msg):
@@ -65,9 +68,12 @@ def write_pipe(c, str):
if verbose:
sys.stderr.write('Writing pipe: %s\n' % c)
- pipe = os.popen(c, 'w')
+ p = subprocess.Popen(c, shell=isinstance(c,basestring),
+ stdin=subprocess.PIPE)
+ pipe = p.stdin
val = pipe.write(str)
- if pipe.close():
+ pipe.close();
+ if p.wait():
die('Command failed: %s' % c)
return val
@@ -78,11 +84,13 @@ def p4_write_pipe(c, str):
def read_pipe(c, ignore_error=False):
if verbose:
- sys.stderr.write('Reading pipe: %s\n' % c)
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
- pipe = os.popen(c, 'rb')
+ expand = isinstance(c,basestring)
+ p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
+ pipe = p.stdout
val = pipe.read()
- if pipe.close() and not ignore_error:
+ if p.wait() and not ignore_error:
die('Command failed: %s' % c)
return val
@@ -93,11 +101,13 @@ def p4_read_pipe(c, ignore_error=False):
def read_pipe_lines(c):
if verbose:
- sys.stderr.write('Reading pipe: %s\n' % c)
- ## todo: check return status
- pipe = os.popen(c, 'rb')
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
+
+ expand = isinstance(c, basestring)
+ p = subprocess.Popen(c, shell=expand, stdout=subprocess.PIPE)
+ pipe = p.stdout
val = pipe.readlines()
- if pipe.close():
+ if pipe.close() or p.wait():
die('Command failed: %s' % c)
return val
@@ -108,15 +118,37 @@ def p4_read_pipe_lines(c):
return read_pipe_lines(real_cmd)
def system(cmd):
+ expand = isinstance(cmd,basestring)
if verbose:
- sys.stderr.write("executing %s\n" % cmd)
- if os.system(cmd) != 0:
- die("command failed: %s" % cmd)
+ sys.stderr.write("executing %s\n" % str(cmd))
+ subprocess.check_call(cmd, shell=expand)
def p4_system(cmd):
"""Specifically invoke p4 as the system command. """
real_cmd = p4_build_cmd(cmd)
- return system(real_cmd)
+ expand = isinstance(real_cmd, basestring)
+ subprocess.check_call(real_cmd, shell=expand)
+
+def p4_integrate(src, dest):
+ p4_system(["integrate", "-Dt", src, dest])
+
+def p4_sync(path):
+ p4_system(["sync", path])
+
+def p4_add(f):
+ p4_system(["add", f])
+
+def p4_delete(f):
+ p4_system(["delete", f])
+
+def p4_edit(f):
+ p4_system(["edit", f])
+
+def p4_revert(f):
+ p4_system(["revert", f])
+
+def p4_reopen(type, file):
+ p4_system(["reopen", "-t", type, file])
#
# Canonicalize the p4 type and return a tuple of the
@@ -167,12 +199,12 @@ def setP4ExecBit(file, mode):
if p4Type[-1] == "+":
p4Type = p4Type[0:-1]
- p4_system("reopen -t %s %s" % (p4Type, file))
+ p4_reopen(p4Type, file)
def getP4OpenedType(file):
# Returns the perforce file type for the given file.
- result = p4_read_pipe("opened %s" % file)
+ result = p4_read_pipe(["opened", file])
match = re.match(".*\((.+)\)\r?$", result)
if match:
return match.group(1)
@@ -228,9 +260,17 @@ def isModeExecChanged(src_mode, dst_mode):
return isModeExec(src_mode) != isModeExec(dst_mode)
def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
- cmd = p4_build_cmd("-G %s" % (cmd))
+
+ if isinstance(cmd,basestring):
+ cmd = "-G " + cmd
+ expand = True
+ else:
+ cmd = ["-G"] + cmd
+ expand = False
+
+ cmd = p4_build_cmd(cmd)
if verbose:
- sys.stderr.write("Opening pipe: %s\n" % cmd)
+ sys.stderr.write("Opening pipe: %s\n" % str(cmd))
# Use a temporary file to avoid deadlocks without
# subprocess.communicate(), which would put another copy
@@ -242,7 +282,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
stdin_file.flush()
stdin_file.seek(0)
- p4 = subprocess.Popen(cmd, shell=True,
+ print cmd
+ p4 = subprocess.Popen(cmd,
+ shell=expand,
stdin=stdin_file,
stdout=subprocess.PIPE)
@@ -275,7 +317,7 @@ def p4Where(depotPath):
if not depotPath.endswith("/"):
depotPath += "/"
depotPath = depotPath + "..."
- outputList = p4CmdList("where %s" % depotPath)
+ outputList = p4CmdList(["where", depotPath])
output = None
for entry in outputList:
if "depotFile" in entry:
@@ -477,8 +519,10 @@ def originP4BranchesExist():
def p4ChangesForPaths(depotPaths, changeRange):
assert depotPaths
- output = p4_read_pipe_lines("changes " + ' '.join (["%s...%s" % (p, changeRange)
- for p in depotPaths]))
+ cmd = ['changes']
+ for p in depotPaths:
+ cmd += ["%s...%s" % (p, changeRange)]
+ output = p4_read_pipe_lines(cmd)
changes = {}
for line in output:
@@ -561,7 +605,7 @@ class P4Debug(Command):
def run(self, args):
j = 0
- for output in p4CmdList(" ".join(args)):
+ for output in p4CmdList(args):
print 'Element: %d' % j
j += 1
print output
@@ -715,7 +759,7 @@ class P4Submit(Command, P4UserMap):
break
if not client:
die("could not get client spec")
- results = p4CmdList("changes -c %s -m 1" % client)
+ results = p4CmdList(["changes", "-c", client, "-m", "1"])
for r in results:
if r.has_key('change'):
return r['change']
@@ -778,7 +822,7 @@ class P4Submit(Command, P4UserMap):
# remove lines in the Files section that show changes to files outside the depot path we're committing into
template = ""
inFilesSection = False
- for line in p4_read_pipe_lines("change -o"):
+ for line in p4_read_pipe_lines(['change', '-o']):
if line.endswith("\r\n"):
line = line[:-2] + "\n"
if inFilesSection:
@@ -835,7 +879,7 @@ class P4Submit(Command, P4UserMap):
modifier = diff['status']
path = diff['src']
if modifier == "M":
- p4_system("edit \"%s\"" % path)
+ p4_edit(path)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
filesToChangeExecBit[path] = diff['dst_mode']
editedFiles.add(path)
@@ -850,21 +894,21 @@ class P4Submit(Command, P4UserMap):
filesToAdd.remove(path)
elif modifier == "C":
src, dest = diff['src'], diff['dst']
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+ p4_integrate(src, dest)
if diff['src_sha1'] != diff['dst_sha1']:
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
os.unlink(dest)
editedFiles.add(dest)
elif modifier == "R":
src, dest = diff['src'], diff['dst']
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+ p4_integrate(src, dest)
if diff['src_sha1'] != diff['dst_sha1']:
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
os.unlink(dest)
editedFiles.add(dest)
@@ -887,9 +931,9 @@ class P4Submit(Command, P4UserMap):
if response == "s":
print "Skipping! Good luck with the next patches..."
for f in editedFiles:
- p4_system("revert \"%s\"" % f);
+ p4_revert(f)
for f in filesToAdd:
- system("rm %s" %f)
+ os.remove(f)
return
elif response == "a":
os.system(applyPatchCmd)
@@ -910,10 +954,10 @@ class P4Submit(Command, P4UserMap):
system(applyPatchCmd)
for f in filesToAdd:
- p4_system("add \"%s\"" % f)
+ p4_add(f)
for f in filesToDelete:
- p4_system("revert \"%s\"" % f)
- p4_system("delete \"%s\"" % f)
+ p4_revert(f)
+ p4_delete(f)
# Set/clear executable bits
for f in filesToChangeExecBit.keys():
@@ -935,7 +979,7 @@ class P4Submit(Command, P4UserMap):
del(os.environ["P4DIFF"])
diff = ""
for editedFile in editedFiles:
- diff += p4_read_pipe("diff -du %r" % editedFile)
+ diff += p4_read_pipe(['diff', '-du', editedFile])
newdiff = ""
for newFile in filesToAdd:
@@ -987,7 +1031,7 @@ class P4Submit(Command, P4UserMap):
submitTemplate = message[:message.index(separatorLine)]
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
- p4_write_pipe("submit -i", submitTemplate)
+ p4_write_pipe(['submit', '-i'], submitTemplate)
if self.preserveUser:
if p4User:
@@ -998,10 +1042,10 @@ class P4Submit(Command, P4UserMap):
else:
for f in editedFiles:
- p4_system("revert \"%s\"" % f);
+ p4_revert(f)
for f in filesToAdd:
- p4_system("revert \"%s\"" % f);
- system("rm %s" %f)
+ p4_revert(f)
+ os.remove(f)
os.remove(fileName)
else:
@@ -1054,8 +1098,7 @@ class P4Submit(Command, P4UserMap):
chdir(self.clientPath)
print "Synchronizing p4 checkout..."
- p4_system("sync ...")
-
+ p4_sync("...")
self.check()
commits = []
@@ -1269,7 +1312,7 @@ class P4Sync(Command, P4UserMap):
# operations. utf16 is converted to ascii or utf8, perhaps.
# But ascii text saved as -t utf16 is completely mangled.
# Invoke print -o to get the real contents.
- text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
+ text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
contents = [ text ]
# Perhaps windows wants unicode, utf16 newlines translated too;
@@ -1365,9 +1408,9 @@ class P4Sync(Command, P4UserMap):
def streamP4FilesCbSelf(entry):
self.streamP4FilesCb(entry)
- p4CmdList("-x - print",
- '\n'.join(['%s#%s' % (f['path'], f['rev'])
- for f in filesToRead]),
+ fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
+
+ p4CmdList(["print"] + fileArgs,
cb=streamP4FilesCbSelf)
# do the last chunk
@@ -1429,7 +1472,7 @@ class P4Sync(Command, P4UserMap):
if self.verbose:
print "Change %s is labelled %s" % (change, labelDetails)
- files = p4CmdList("files " + ' '.join (["%s...@%s" % (p, change)
+ files = p4CmdList(["files"] + (["%s...@%s" % (p, change)
for p in branchPrefixes]))
if len(files) == len(labelRevisions):
@@ -1478,9 +1521,9 @@ class P4Sync(Command, P4UserMap):
newestChange = 0
if self.verbose:
print "Querying files for label %s" % label
- for file in p4CmdList("files "
- + ' '.join (["%s...@%s" % (p, label)
- for p in self.depotPaths])):
+ for file in p4CmdList(["files"] +
+ (["%s...@%s" % (p, label)
+ for p in self.depotPaths])):
revisions[file["depotFile"]] = file["rev"]
change = int(file["change"])
if change > newestChange:
@@ -1735,10 +1778,9 @@ class P4Sync(Command, P4UserMap):
newestRevision = 0
fileCnt = 0
- for info in p4CmdList("files "
- + ' '.join(["%s...%s"
- % (p, revision)
- for p in self.depotPaths])):
+ fileArgs = ["%s...%s" % (p,revision) for p in self.depotPaths]
+
+ for info in p4CmdList(["files"] + fileArgs):
if 'code' in info and info['code'] == 'error':
sys.stderr.write("p4 returned an error: %s\n"
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index bb89b63..12f374f 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -150,7 +150,7 @@ test_expect_success 'preserve users' '
git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
git config git-p4.skipSubmitEditCheck true &&
- P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
+ P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --verbose --preserve-user &&
p4_check_commit_author file1 alice &&
p4_check_commit_author file2 bob
'
diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
new file mode 100755
index 0000000..1143491
--- /dev/null
+++ b/t/t9803-git-shell-metachars.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='git-p4 transparency to shell metachars in filenames'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ kill_p4d || : &&
+ start_p4d &&
+ cd "$TRASH_DIRECTORY"
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "file1"
+ )
+'
+
+test_expect_success 'shell metachars in filenames' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ echo f1 >foo\$bar &&
+ git add foo\$bar &&
+ echo f2 >"file with spaces" &&
+ git add "file with spaces" &&
+ P4EDITOR=touch git commit -m "add files" &&
+ "$GITP4" submit --verbose &&
+ cd "$cli" &&
+ p4 sync ... &&
+ ls -l "file with spaces" &&
+ ls -l "foo\$bar"
+ )
+'
+
+check_missing() {
+ for i in $*; do
+ if [ -f $i ]; then
+ echo $i found but should be missing 1>&2
+ exit 1
+ fi
+ done
+}
+
+test_expect_success 'deleting with shell metachars' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ git rm foo\$bar &&
+ git rm file\ with\ spaces &&
+ P4EDITOR=touch git commit -m "remove files" &&
+ "$GITP4" submit --verbose
+ cd "$cli" &&
+ p4 sync ... &&
+ check_missing "file with spaces" foo\$bar
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.6.347.g4db0d
^ permalink raw reply related
* [PATCH/RFCv1] git-p4: handle files with shell metacharacters
From: Luke Diamand @ 2011-09-26 21:29 UTC (permalink / raw)
To: git; +Cc: pw, vitor.hda, Luke Diamand
git-p4 uses the shell to execute perforce and git. This leads to problems
where files contain shell metacharacters or spaces. I first hit this
when someone checked in files with dollars ($) in their name, but in theory
you could cause complete havoc with other characters: backticks in a
filename would be especially entertaining.
Make git-p4 use subprocess.Popen() and subprocess.call() instead, and
pass in argv[] style arrays instead, at least for cases where filenames
are involved. Add test cases.
Notes:
This patch is based on Pete Wyckoff's recent patch series for refactoring
the git-p4 test harness, so it won't apply to the current next or master
branches.
I tried testing it on Cygwin as well, but the test harness appears to be
very broken on that platform as it is unable to start p4d.
Luke Diamand (1):
git-p4: handle files with shell metacharacters
contrib/fast-import/git-p4 | 174 +++++++++++++++++++++++++---------------
t/t9800-git-p4.sh | 2 +-
t/t9803-git-shell-metachars.sh | 70 ++++++++++++++++
3 files changed, 179 insertions(+), 67 deletions(-)
create mode 100755 t/t9803-git-shell-metachars.sh
--
1.7.6.347.g4db0d
^ permalink raw reply
* Re: [PATCH] gitweb: Add js=1 before an URI fragment to fix line number links
From: Junio C Hamano @ 2011-09-26 21:14 UTC (permalink / raw)
To: Peter Stuge; +Cc: git
In-Reply-To: <20110926194639.25339.qmail@stuge.se>
Peter Stuge <peter@stuge.se> writes:
> Okey. I thought subject together with change would be clear enough. :)
>
>
>> Explanation of what you are fixing is totally lacking.
>
> The subject sums it up, if briefly.
... Sorry, that is not what I meant.
You don't have to explain these to *me* specifically as a response to this
thread. What I meant was that your patch should have these necessary
descriptions in its proposed commit log message.
^ permalink raw reply
* Re: config-file includes
From: Junio C Hamano @ 2011-09-26 21:12 UTC (permalink / raw)
To: Jeff King
Cc: David Aguilar, Nguyen Thai Ngoc Duy, git, Michael Haggerty,
Jay Soffian, Jakub Narebski
In-Reply-To: <20110926200553.GA492@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We could allow arbitrary shell code like:
>
> [include-if "test `uname -s` -eq Darwin"]
>
> Very flexible, though it makes me think we are getting a little
> overboard. And it's an extra shell invocation whenever we read the
> config, which is ugly.
Let's not go there. It is not even like we only ever call git_config()
once in the lifetime of a process.
^ permalink raw reply
* Re: subversion-perl missing
From: Andreas Schwab @ 2011-09-26 21:11 UTC (permalink / raw)
To: Georg-Johann Lay; +Cc: Michael J Gruber, git
In-Reply-To: <4E80811B.4030309@gjlay.de>
Georg-Johann Lay <avr@gjlay.de> writes:
> svn identifies itself as
>
> > svn --version
>
> svn, version 1.6.2 (r37639)
> compiled Jun 19 2009, 12:21:15
openSUSE 11.1 contained subversion 1.5.2 (and 1.5.7 as an update), so
this is definitely not vanilla.
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
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