Re: Git build fails on `make`, undefined references in libcrypto.a.
On Sun, Mar 17, 2013 at 10:03:37PM -0600, zero modulo wrote: > $ LDFLAGS="-L/sandbox/builds/lib" CPPFLAGS="-I/sandbox/builds/include" > ./configure --prefix=$PREFIX > > $ make > […] > /sandbox/builds/lib/libcrypto.a(dso_dlfcn.o): In function > `dlfcn_globallookup': > dso_dlfcn.c:(.text+0x1b): undefined reference to `dlopen' [...] > make: *** [git-imap-send] Error 1 FYI, I've already tried to answer this exact question [1] with no comments from the OP. 1. http://serverfault.com/a/488604/118848 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Make GIT_USE_LOOKUP default?
[+cc Ingo and Jonathan, as this revisits the "open-code hashcmp" thread referenced below] On Sun, Mar 17, 2013 at 01:13:56PM -0700, Junio C Hamano wrote: > Duy Nguyen writes: > > > This env comes from jc/sha1-lookup in 2008 (merge commit e9f9d4f), 5 > > years ago. I wonder if it's good enough to turn on by default and keep > > improving from there, or is it still experimental? > > The algorithm has been used in production in other codepaths like > patch-ids and replace-object, so correctness-wise it should be fine > to turn it on. I think nobody has bothered to benchmark with and > without the environment to see if it is really worth the complexity. > > It may be a good idea to try doing so, now you have noticed it ;-). The only benchmarking I could find in the list archive (besides the ones in the commit itself, showing little change, but fewer page faults) is: http://article.gmane.org/gmane.comp.version-control.git/123832 which actually indicates that GIT_USE_LOOKUP is slower (despite having fewer page faults). By the way, looking at that made me think for a few minutes about hashcmp, and I was surprised to find that we use an open-coded comparison loop. That dates back to this thread by Ingo: http://article.gmane.org/gmane.comp.version-control.git/172286 I could not replicate his benchmarks at all. In fact, my measurements showed a slight slowdown with 1a812f3 (hashcmp(): inline memcmp() by hand to optimize, 2011-04-28). Here are my best-of-five numbers for running "git rev-list --objects --all >/dev/null" on linux-2.6.git: [current master, compiled with -O2] real0m45.612s user0m45.140s sys 0m0.300s [current master, compiled with -O3 for comparison] real0m45.588s user0m45.088s sys 0m0.312s [revert 1a812f3 (i.e., go back to memcmp), -O2] real0m44.358s user0m43.876s sys 0m0.316s [open-code first byte, fall back to memcmp, -O2] real0m43.963s user0m43.568s sys 0m0.284s I wonder why we get such different numbers. Ingo said his tests are on a Nehalem CPU, as are mine (mine is an i7-840QM). I wonder if we should be wrapping the optimization in an #ifdef, but I'm not sure which flag we should be checking. Note that I didn't run all of my measurements using "git gc" as Ingo did, which I think conflates a lot of unrelated performance issues (like writing out a packfile). The interesting bits for hashcmp in "gc" are the "Counting objects" phase of pack-objects, and "git prune" determining reachability. Those are both basically the same as "rev-list --objects --all". I did do a quick check of `git gc`, though, and it showed results that matched my rev-lists above (i.e., a very slight speedup by going back to memcmp). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Proposal: sharing .git/config
Jeff King wrote: > I don't think you can avoid the 3-step problem and retain the safety in > the general case. Forgetting implementation details for a minute, you > have either a 1-step system: > > 1. Fetch and start using config from the remote. > > which is subject to fetching and executing malicious config, or: > > 1. Fetch config from remote. > 2. Inspect it. > 3. Integrate it into the current config. I don't understand your emphasis on step 2. Isn't the configuration written by me? Why would it be malicious? I've just started thinking about how to design something that will allow us to share configuration elegantly [1]. Essentially, the metadata repository will consist of *.layout files, one for each repository to clone, containing the .git/config to write after cloning that repository. So, a git.layout might look like: [layout] directory = git [remote "origin"] url = git://github.com/git/git [remote "ram"] url = g...@github.com:artagnon/git [remote "junio"] url = git://github.com/gitster/git As you can see the [layout] is a special section which will tell our fetcher where to place the repository. Everything else is meant to be inserted into the repository's .git/config. However, I can foresee a problem in scaling: when I ask a specific directory like a/b/c to be populated (equivalent of repo sync `a/b/c`), it'll have to parse the layout.directory variable of all the .layout files, and this can be slow. So, maybe we should have a special _manifest.layout listing all the paths? Further, I see this as a way to work with projects that would otherwise require nested submodules like the Android project. What do you think? [1]: https://github.com/artagnon/src.layout -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] Introduce remote.pushdefault
Jeff King wrote: > So the push lookup list is (in order of precedence): > > 1. branch.*.pushremote > 2. remote.pushdefault > 3. branch.*.remote > 4. remote.default > 5. origin > > and it solves Junio's issue because the way to say "override my > remote.pushdefault for this branch" is not to set "branch.*.remote", but > to set "branch.*.pushremote". Right, thanks for clearing that up Jeff. I'll re-roll with remote.pushdefault overriding branch..remote. Ram -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On 3/18/13 12:35 AM, Junio C Hamano wrote:
> Rob Hoelz writes:
>
>> git push currently doesn't consider pushInsteadOf when
>> using pushurl; this tests and fixes that.
>>
>> If you use pushurl with an alias that has a pushInsteadOf configuration
>> value, Git does not take advantage of it. For example:
>>
>> [url "git://github.com/"]
>> insteadOf = github:
>> [url "git://github.com/myuser/"]
>> insteadOf = mygithub:
>> [url "g...@github.com:myuser/"]
>> pushInsteadOf = mygithub:
>> [remote "origin"]
>> url = github:organization/project
>> pushurl = mygithub:project
> Incomplete sentence? For example [this is an example configuration]
> and then what happens? Something like "with the sample
> configuration, 'git push origin' should follow pushurl and then turn
> it into X, but instead it ends up accessing Y".
>
> If there is no pushInsteadOf, does it still follow insteadOf? Is it
> tested already?
>
> Wouldn't you want to cover all the combinations to negative cases
> (i.e. making sure the codepath to support a new case does not affect
> behaviour of the code outside the new case)? A remote with and
> without pushurl (two combinations) and a pseudo URL scheme with and
> without pushInsteadOf (again, two combinations) will give you four
> cases.
>
>
> Thanks.
Sorry; that's a good point. With the above configuration, the following
fails:
$ git push origin master
With the following message:
fatal: remote error:
You can't push to git://github.com/myuser/project.git
Use g...@github.com:myuser/project.git
So you can see that pushurl is being followed (it's not attempting to
push to git://github.com/organization/project), but insteadOf values are
being used as opposed to pushInsteadOf values for expanding the pushurl
alias.
I haven't tried without pushInsteadOf; I will add a test for it later
today. I assume that if pushInsteadOf isn't found, insteadOf should be
used? I will also consider the other test cases you described.
Thanks again for the feedback!
>
>> Signed-off-by: Rob Hoelz
>> ---
>> remote.c | 2 +-
>> t/t5516-fetch-push.sh | 20
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/remote.c b/remote.c
>> index ca1f8f2..de7a915 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -465,7 +465,7 @@ static void alias_all_urls(void)
>> if (!remotes[i])
>> continue;
>> for (j = 0; j < remotes[i]->pushurl_nr; j++) {
>> -remotes[i]->pushurl[j] =
>> alias_url(remotes[i]->pushurl[j], &rewrites);
>> +remotes[i]->pushurl[j] =
>> alias_url(remotes[i]->pushurl[j], &rewrites_push);
>> }
>> add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
>> for (j = 0; j < remotes[i]->url_nr; j++) {
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index b5417cc..272e225 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and
>> explicit pushurl (pushInsteadOf
>> )
>> '
>>
>> +test_expect_success 'push with pushInsteadOf and explicit pushurl
>> (pushInsteadOf does rewrite in this case)' '
>> +mk_empty &&
>> +TRASH="$(pwd)/" &&
>> +mkdir ro &&
>> +mkdir rw &&
>> +git init --bare rw/testrepo &&
>> +git config "url.file://$TRASH/ro/.insteadOf" ro: &&
>> +git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
>> +git config remote.r.url ro:wrong &&
>> +git config remote.r.pushurl rw:testrepo &&
>> +git push r refs/heads/master:refs/remotes/origin/master &&
>> +(
>> +cd rw/testrepo &&
>> +r=$(git show-ref -s --verify refs/remotes/origin/master) &&
>> +test "z$r" = "z$the_commit" &&
>> +
>> +test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
>> +)
>> +'
>> +
>> test_expect_success 'push with matching heads' '
>>
>> mk_test heads/master &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] Documentation/technical/api-fswatch.txt: start with outline
Ramkumar Ramachandra writes:
> Junio C Hamano wrote:
>> Yes, and you would need one inotify per directory but you do not
>> have an infinite supply of outstanding inotify watch (wasn't the
>> limit like 8k per a single uid or something?), so the daemon must be
>> prepared to say "I'll watch this, that and that directories, but the
>> consumers should check other directories themselves."
>>
>> FWIW, I share your suspicion that an effort in the direction this
>> thread suggests may end up duplicating what the caching vfs layer
>> already does, and doing so poorly.
>
> Thomas Rast wrote:
>> $ cat /proc/sys/fs/inotify/max_user_watches
>> 65536
>> $ cat /proc/sys/fs/inotify/max_user_instancest
>> 128
>
> From Junio's and Thomas' observations, I'm inclined to think that
> inotify is ill-suited for the problem we are trying to solve. It is
> designed as a per-directory watch, because VFS can quickly supply the
> inodes for a directory entry. As such, I think the ideal usecase for
> inotify is to execute something immediately when a change takes place
> in a directory: it's well-suited for solutions like Dropbox (which I
> think is poorly designed to begin with, but that's offtopic). It
> doesn't substitute of augment VFS caching. I suspect the VFS cache
> works by caching the inodes in a frequently used directory entry, thus
> optimizing calls like lstat() on them.
I have three objections to changing the kernel to fit us, as opposed to
just using inotify:
* inotify works. I can watch most of my $HOME with the hack I linked
earlier[1]. Yes, it's a lot of coding around the problem that it is
nonrecursive, but we already have a lot of code around the problem
that we can't ask the VFS for diffs between points in time (namely,
the whole business with an index and lstat() loops).
* inotify is here today. Even if you got a hypothetical notifier into
the kernel today, you'd have to wait months/years until it is
available in distros, and years until everyone has it.
* I'll bet you a beer that the kernel folks already had the same
discussion when they made inotify. There has to be a reason why it's
better than providing for recursive watches.
[1] https://github.com/trast/watch
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] http_init: only initialize SSL for https
On Sun, Mar 17, 2013 at 11:27 PM, Junio C Hamano wrote: > Daniel Stenberg writes: > >> On Sun, 17 Mar 2013, Antoine Pelisse wrote: >> With redirects taken into account, I can't think of any really good way around avoiding this init... >>> >>> Is there any way for curl to initialize SSL on-demand ? >> >> Yes, but not without drawbacks. >> >> If you don't call curl_global_init() at all, libcurl will notice that >> on first use and then libcurl will call global_init by itself with a >> default bitmask. >> >> That automatic call of course will prevent the application from being >> able to set its own bitmask choice, and also the global_init function >> is not (necessarily) thread safe while all other libcurl functions are >> so the internal call to global_init from an otherwise thread-safe >> function is unfortunate. > > So in short, unless you are writing a custom application to talk to > servers that you know will never redirect you to HTTPS, passing > custom masks such as ALL&~SSL to global-init is not going to be a > valid optimization. > > I think that is a reasonable API; your custom application may want > to go around your intranet servers all of which serve their status > over plain HTTP, and it is a valid optimization to initialize the > library with ALL&~SSL. It is just that such an optimization does > not apply to us---we let our users go to random hosts we have no > control over, and they may redirect us in ways we cannot anticipate. > I wonder. Our libcurl is build with "-winssl" (USE_WINDOWS_SSPI=1), it seems. Perhaps switching to openssl (which we already have libraries for) would make the init-time better? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] pack-refs: add fully-peeled trait
On Sun, Mar 17, 2013 at 01:01:44PM -0700, Junio C Hamano wrote:
> > + * All references in the file that can be peeled are peeled.
> > + * Inversely (and this is more important, any references in the
>
> A missing closing paren after "more important". Also the e-mail
> quote reveals there is some inconsistent indentation (HTs vs runs of
> SPs) here.
Thanks, will fix (the whitespace damage is due to me cutting and pasting
from Michael's commit).
>
> > + * file for which no peeled value is recorded is not peelable. This
> > + * trait should typically be written alongside "fully-peeled" for
>
> Alongside "peeled", no?
Urgh, yes. Will fix.
> [...]
> I am not sure why you find this any more readable.
I was trying to avoid the "set PEELED globally, but sometimes unset it
for specific refs" pattern which I think is confusing to the reader. But
I think what you wrote is even better. I used an enum rather than two
variables to make it clear that only ones takes effect. I had wanted to
use a switch, also, but you end up either repeating yourself, or doing
this gross fall-through:
switch (peeled) {
case PEELED_TAGS:
if (prefixcmp(refname, "refs/tags/"))
break;
/* fall-through */
case PEELED_FULLY:
last->ref |= REF_KNOWS_PEELED;
break;
default:
/* we know nothing */
}
So I just stuck with the conditional.
Here's the re-roll.
-- >8 --
From: Michael Haggerty
Subject: [PATCH] pack-refs: add fully-peeled trait
Older versions of pack-refs did not write peel lines for
refs outside of refs/tags. This meant that on reading the
pack-refs file, we might set the REF_KNOWS_PEELED flag for
such a ref, even though we do not know anything about its
peeled value.
The previous commit updated the writer to always peel, no
matter what the ref is. That means that packed-refs files
written by newer versions of git are fine to be read by both
old and new versions of git. However, we still have the
problem of reading packed-refs files written by older
versions of git, or by other implementations which have not
yet learned the same trick.
The simplest fix would be to always unset the
REF_KNOWS_PEELED flag for refs outside of refs/tags that do
not have a peel line (if it has a peel line, we know it is
valid, but we cannot assume a missing peel line means
anything). But that loses an important optimization, as
upload-pack should not need to load the object pointed to by
refs/heads/foo to determine that it is not a tag.
Instead, we add a "fully-peeled" trait to the packed-refs
file. If it is set, we know that we can trust a missing peel
line to mean that a ref cannot be peeled. Otherwise, we fall
back to assuming nothing.
[commit message and tests by Jeff King ]
Signed-off-by: Michael Haggerty
Signed-off-by: Jeff King
---
pack-refs.c | 2 +-
refs.c | 49 -
t/t3211-peel-ref.sh | 22 ++
3 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/pack-refs.c b/pack-refs.c
index ebde785..4461f71 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -128,7 +128,7 @@ int pack_refs(unsigned int flags)
die_errno("unable to create ref-pack file structure");
/* perhaps other traits later as well */
- fprintf(cbdata.refs_file, "# pack-refs with: peeled \n");
+ fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
for_each_ref(handle_one_ref, &cbdata);
if (ferror(cbdata.refs_file))
diff --git a/refs.c b/refs.c
index 175b9fc..e2b760d 100644
--- a/refs.c
+++ b/refs.c
@@ -803,11 +803,38 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
return line;
}
+/*
+ * Read f, which is a packed-refs file, into dir.
+ *
+ * A comment line of the form "# pack-refs with: " may contain zero or
+ * more traits. We interpret the traits as follows:
+ *
+ * No traits:
+ *
+ * Probably no references are peeled. But if the file contains a
+ * peeled value for a reference, we will use it.
+ *
+ * peeled:
+ *
+ * References under "refs/tags/", if they *can* be peeled, *are*
+ * peeled in this file. References outside of "refs/tags/" are
+ * probably not peeled even if they could have been, but if we find
+ * a peeled value for such a reference we will use it.
+ *
+ * fully-peeled:
+ *
+ * All references in the file that can be peeled are peeled.
+ * Inversely (and this is more important), any references in the
+ * file for which no peeled value is recorded is not peelable. This
+ * trait should typically be written alongside "peeled" for
+ * compatibility with older clients, but we do not require it
+ * (i.e., "peeled" is a no-op if "fully-peeled" is set).
+ */
static void read_packed_refs(FILE *f, struct ref_dir *dir)
{
struct ref_entry *last = NULL;
char refline[PATH_MAX];
- int flag = REF_ISPA
Re: Proposal: sharing .git/config
On Mon, Mar 18, 2013 at 02:30:23PM +0530, Ramkumar Ramachandra wrote: > Jeff King wrote: > > I don't think you can avoid the 3-step problem and retain the safety in > > the general case. Forgetting implementation details for a minute, you > > have either a 1-step system: > > > > 1. Fetch and start using config from the remote. > > > > which is subject to fetching and executing malicious config, or: > > > > 1. Fetch config from remote. > > 2. Inspect it. > > 3. Integrate it into the current config. > > I don't understand your emphasis on step 2. Isn't the configuration > written by me? Why would it be malicious? Maybe I am misunderstanding the use case, but when people talk about share config, they are often talking about pushing project-wide config out to developers. So the config is not necessarily written by you, but by somebody who had write access to the upstream repository. The obvious counterpoint is that people usually run "make" right after fetching, so they are trusting what they fetched already. And the counter-counterpoint is that yes, that's true, but at least with the "make" case they can use git to inspect the differences before running them. You may be able to tell that this is not the first time this discussion has happened. :) Personally, I do not think it is the end of the world for people to opt into the "automatically fetch and respect config" method for certain repositories (and that's why I wrote include.ref support a while ago). It's a security tradeoff that the user may want to make. But I also respect the argument that we should not be endorsing risky behavior by advertising such a feature (especially when the risk is quite subtle, as many users may not realize that git config can execute arbitrary code). > I've just started thinking about how to design something that will > allow us to share configuration elegantly [1]. Essentially, the > metadata repository will consist of *.layout files, one for each > repository to clone, containing the .git/config to write after cloning > that repository. So, a git.layout might look like: > > [layout] > directory = git > [remote "origin"] > url = git://github.com/git/git > [remote "ram"] > url = g...@github.com:artagnon/git > [remote "junio"] > url = git://github.com/gitster/git > > As you can see the [layout] is a special section which will tell our > fetcher where to place the repository. Everything else is meant to be > inserted into the repository's .git/config. However, I can foresee a > problem in scaling: when I ask a specific directory like a/b/c to be > populated (equivalent of repo sync `a/b/c`), it'll have to parse the > layout.directory variable of all the .layout files, and this can be > slow. So, maybe we should have a special _manifest.layout listing all > the paths? > > Further, I see this as a way to work with projects that would > otherwise require nested submodules like the Android project. What do > you think? Yeah, reading your layout description, this is less about git config in particular, and more about managing hierarchies of repos. Which I think is a fine thing to do, and is a sensible place to put config management (since you are probably executing arbitrary code as part of the layout tool anyway). But I don't have a real opinion on the design of such a tool. I have used repo only once or twice to deal with Android. For my own menagerie of small repos, I have a hacky custom tool that is mostly about deciding when there are items to be committed, pushed, or fetched in each repo; I never found the need to handle git config at all. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
random server hacks on top of git
[Re-titled, as we are off-topic from the original patch series] On Sun, Mar 17, 2013 at 05:38:59PM +0100, René Scharfe wrote: > Am 17.03.2013 06:40, schrieb Jeff King: > >We do have the capability to roll out to one or a few of our servers > >(the granularity is not 0.2%, but it is still small). I'm going to try > >to keep us more in sync with upstream git, but I don't know if I will > >get to the point of ever deploying "master" or "next", even for a small > >portion of the population. We are accumulating more hacks[1] on top of > >git, so it is not just "run master for an hour on this server"; I have > >to actually merge our fork. > > Did you perhaps intend to list these hacks in a footnote or link to a > repository containing them? (I can't find the counterpart of that > [1].) I was actually just going to say "some of which are gross hacks that will never see the light of day, some of which have already gone upstream, and some of which I am planning on submitting upstream". But since I happened to be cataloguing them recently, here is the list of things that have not yet gone upstream. If anybody is interested in a particular topic, I'm happy to discuss and/or prioritize moving it forward. - blame-tree; re-rolled from my submission last year to build on top of the revision machinery, handle merges sanely, etc. Mostly this needs documentation and a clean-up of the output format (which is very utilitarian, but probably should share output with git-blame). - diff --max-depth; this is a requirement to do blame-tree efficiently if you want to do GitHub-style listings (you must recurse to find the history of some/subdir, but you do not want to recurse past that for efficiency reasons). This is hung up on two things: 1. It does not integrate with the pathspec max-depth code, because we do not use struct pathspec in the tree diff (but I think Duy's patches are changing that). 2. My definition of --max-depth is subtly different from that of "git grep". But I think mine is more useful, and I haven't decided how to reconcile it. - share ref selection code between "git branch", "git tag", and "git for-each-ref". This includes cleaning up the "tag --contains" code to be safer for general use (so that "branch --contains" can benefit from the speedup), and then getting the same options for all three commands (tag doesn't know about --merged, and for-each-ref doesn't know about --contains or --merged). - receive.maxsize; index-pack will happily spool data to disk forever, and you never even get a chance to make a policy decision like "hey, this is too big". This patch lets index-pack cut off the client after a certain number of bytes. It's not elegant because the cutoff transfer is not resumable, but we use it is as a last-ditch for DoS protection (the client can reconnect and send more, of course, but at that point we have the opportunity to make external policy decisions like locking their account). Not sure if other sites would want this or not. - receive.advertisealternates; basically turn off ".have" advertisement. Some of our alternates networks are so huge that the cost of collecting all of the alternate refs is very high (even though it can save some transfer bandwidth). Not sure if other sites want this or not (and I think it would be more elegant to have a small static set of common refs that people build off of, and advertise those. e.g., if you fork rails/rails, then we should advertise rails/rails/refs/heads/master as a ".have", but not anybody else's fork). - receive.hiderefs; this is going to become redundant with Junio's implementation - an audit reflog; we keep a reflog for all refs at the root of the repository. It differs from a regular reflog in that: 1. It never expires. 2. It is not part of reachability analysis. 3. It includes the refname for each entry, so you can see deletions. It's mostly useful for forensics when somebody has screwed up their repository (or we're chasing down a git bug; it helped me find the pack-refs race recently). Probably too GitHub-specific for other people to want it (especially because it grows without bound). - statistics instrumentation; we keep counters for various things in code (e.g., which phase of protocol upload-pack is in, how many bytes sent, etc) and expose them in a few ways. One is over a socket to run a "top"-like interface. Another is to tweak the argv array of the process so that "ps" shows the process state. I think it would be useful to other people running git servers, but the code is currently quite nasty and invasive. I have a work-in-progress to clean it up, but it's got a ways to go. - hacks to set niceness and
Re: [PATCH/RFC] http_init: only initialize SSL for https
On Mon, Mar 18, 2013 at 11:38 AM, Erik Faye-Lund wrote: > On Sun, Mar 17, 2013 at 11:27 PM, Junio C Hamano wrote: >> Daniel Stenberg writes: >> >>> On Sun, 17 Mar 2013, Antoine Pelisse wrote: >>> > With redirects taken into account, I can't think of any really good way > around avoiding this init... Is there any way for curl to initialize SSL on-demand ? >>> >>> Yes, but not without drawbacks. >>> >>> If you don't call curl_global_init() at all, libcurl will notice that >>> on first use and then libcurl will call global_init by itself with a >>> default bitmask. >>> >>> That automatic call of course will prevent the application from being >>> able to set its own bitmask choice, and also the global_init function >>> is not (necessarily) thread safe while all other libcurl functions are >>> so the internal call to global_init from an otherwise thread-safe >>> function is unfortunate. >> >> So in short, unless you are writing a custom application to talk to >> servers that you know will never redirect you to HTTPS, passing >> custom masks such as ALL&~SSL to global-init is not going to be a >> valid optimization. >> >> I think that is a reasonable API; your custom application may want >> to go around your intranet servers all of which serve their status >> over plain HTTP, and it is a valid optimization to initialize the >> library with ALL&~SSL. It is just that such an optimization does >> not apply to us---we let our users go to random hosts we have no >> control over, and they may redirect us in ways we cannot anticipate. >> > > I wonder. Our libcurl is build with "-winssl" (USE_WINDOWS_SSPI=1), it > seems. Perhaps switching to openssl (which we already have libraries > for) would make the init-time better? It does indeed. So this is probably a better solution, and is something we're considering doing in Git for Windows anyway (for a different reason). Thanks for all the feed-back! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] introduce a commit metapack
On Sun, Mar 17, 2013 at 08:21:13PM +0700, Nguyen Thai Ngoc Duy wrote: > On Thu, Jan 31, 2013 at 6:06 PM, Duy Nguyen wrote: > > On Wed, Jan 30, 2013 at 09:16:29PM +0700, Duy Nguyen wrote: > >> Perhaps we could store abbrev sha-1 instead of full sha-1. Nice > >> space/time trade-off. > > > > Following the on-disk format experiment yesterday, I changed the > > format to: > > > > - a list a _short_ SHA-1 of cached commits > > .. > > > > The length of SHA-1 is chosen to be able to unambiguously identify any > > cached commits. Full SHA-1 check is done after to catch false > > positives. For linux-2.6, SHA-1 length is 6 bytes, git and many > > moderate-sized projects are 4 bytes. > > And if we are going to create index v3, the same trick could be used > for the sha-1 table in the index. We use the short sha-1 table for > binary search and put the rest of sha-1 in a following table (just > like file offset table). The advantage is a denser search space, about > 1/4-1/3 the size of full sha-1 table. You can make it even smaller at some (potential) run-time cost. Keep in mind you are just repeating information that is in the full sha1 list in the index. So you could store a fixed-size offset into that list (e.g., 32-bit), and then instead of comparing sha1s during a binary search, you would dereference the offset to the real sha1s and compare those. The run-time cost is not any worse in a big-O sense, but your cache locality is much worse (you hit a second random page for each sha1 comparison), which might be noticeable. You'd have to benchmark to see how big an impact. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: building git ; need suggestion
I'm closer to my requirement. I have found gitweb simply provide a GUI for history check and code comparison. And the git itself is good enough to do the ACL stuff with hooks. I already have the following code to deploy the push into its work-tree === #!/bin/bash while read oldrev newrev ref do branch=`echo $ref | cut -d/ -f3` if [ "master" == "$branch" ]; then git --work-tree=/path/under/root/dir/live-site/ checkout -f $branch echo 'Changes pushed live.' fi if [ "dev" == "$branch" ]; then git --work-tree=/path/under/root/dir/dev-site/ checkout -f $branch echo 'Changes pushed to dev.' fi done = This code can be extended for as many branches as you have. I now need a mechanism to restrict the user to it's own branch so that user can't push into any other branch in mistake. Say I have master branch -> only admin user can push here. dev branch -> only user dev1 , dev2 and master can push here. testing branch -> only user test1 and test2 can push here. I think this can also be done with pre-receive hook. Any suggestion on the hook design is welcome. Also this can be implemented on the above hook or in a separate hook. A separate hook is better due to maintainability and then I need to call multiple pre-receive hook. Please suggest. Thanks On 18-Mar-2013, at 11:14 AM, Joydeep Bakshi wrote: > > On 15-Mar-2013, at 6:44 PM, Magnus Bäck wrote: >>> >> >> Right, but that's R/W permissions. Almost any piece of Git hosting >> software supports restriction of pushes. Discriminating *read* access >> between developers and maintenance people sounds like a disaster if it's >> the same organisation. > > Just restriction on push access is what required. > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] read-cache: avoid memcpy in expand_name_field in index v4
perf reports memcpy at the the 6th position [1] in "git status -uno"
using index v4, and strbuf_remove() in expand_name_field() accounts
for 25% of that. What we need here is a simple string cut and a
cheaper strbuf_setlen() should be enough. After this change, memcpy
drops down to the 13th position [2] and is dominated by
read_index_from.
[1] before
+ 15.74% git git[.] blk_SHA1_Block
+ 13.22% git [kernel.kallsyms] [k] link_path_walk
+ 10.91% git [kernel.kallsyms] [k] __d_lookup
+ 8.17% git [kernel.kallsyms] [k] strncpy_from_user
+ 4.75% git [kernel.kallsyms] [k] memcmp
+ 2.42% git libc-2.11.2.so [.] memcpy
[2] after
+ 16.30% git git[.] blk_SHA1_Block
+ 13.43% git [kernel.kallsyms] [k] link_path_walk
+ 11.45% git [kernel.kallsyms] [k] __d_lookup
+ 8.73% git [kernel.kallsyms] [k] strncpy_from_user
+ 5.14% git [kernel.kallsyms] [k] memcmp
+ 2.29% git [kernel.kallsyms] [k] do_lookup
+ 2.21% git libc-2.11.2.so [.] 0x6daf6
+ 1.98% git [kernel.kallsyms] [k] _atomic_dec_and_lock
+ 1.98% git [kernel.kallsyms] [k] _raw_spin_lock
+ 1.86% git [kernel.kallsyms] [k] acl_permission_check
+ 1.61% git [kernel.kallsyms] [k] kmem_cache_free
+ 1.59% git git[.] unpack_trees
+ 1.47% git libc-2.11.2.so [.] memcpy
Signed-off-by: Nguyễn Thái Ngọc Duy
---
I was after something else when I noticed this. Seems like a simple
and safe change.
read-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index 827ae55..8c443aa 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1354,7 +1354,7 @@ static unsigned long expand_name_field(struct strbuf
*name, const char *cp_)
if (name->len < len)
die("malformed name field in the index");
- strbuf_remove(name, name->len - len, len);
+ strbuf_setlen(name, name->len - len);
for (ep = cp; *ep; ep++)
; /* find the end */
strbuf_add(name, cp, ep - cp);
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] Support triangular workflows
Hi, This series follows up a previous discussion with Junio and Jeff [1]. It attempts to support the triangular workflow, where the remote you're fetching from is not the same as the remote you're pushing to. `remote..pushurl` has already been discussed, and deemed as a poor solution to the problem [2]. [1/4] is a minor cleanup patch to make other patches consistent with the existing style. [2/4] introduces the infrastructure needed to allow [3/4] and [4/4] to be simple configuration-adding patches. [3/4] and [4/4] add the proposed configuration options. They're very simple patches, but the documentation is not so simple: I've documented all the side-effects of the other configuration option in each configuration option, to give the reader a comprehensive picture when reading one configuration option. I've put off implementing remote.default corresponding to remote.pushdefault, as Jeff suggested in [1], because it's currently not an itch; apart from the obvious symmetry, I don't know what purpose it serves: why would anyone want to fetch from a remote other than origin by default? Why wouldn't they simply swap that remote's name with "origin"? However, it's a nice thing to have for symmetry, and it should be trivial to implement: any interested person is welcome to pick it up. The series works as expected, and all tests pass. Thanks for reading. [1]: http://thread.gmane.org/gmane.comp.version-control.git/215763 [2]: http://thread.gmane.org/gmane.comp.version-control.git/215702/focus=215717 Ramkumar Ramachandra (4): remote.c: simply a bit of code using git_config_string() remote.c: introduce a way to have different remotes for fetch/ push remote.c: introduce remote.pushdefault remote.c: introduce branch..pushremote Documentation/config.txt | 23 --- builtin/push.c | 2 +- remote.c | 60 +++- remote.h | 1 + 4 files changed, 66 insertions(+), 20 deletions(-) -- 1.8.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] remote.c: simply a bit of code using git_config_string()
A small segment where handle_config() parses the branch.remote
configuration variable can be simplified using git_config_string().
Signed-off-by: Ramkumar Ramachandra
---
remote.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index e53a6eb..45b69d6 100644
--- a/remote.c
+++ b/remote.c
@@ -356,9 +356,7 @@ static int handle_config(const char *key, const char
*value, void *cb)
return 0;
branch = make_branch(name, subkey - name);
if (!strcmp(subkey, ".remote")) {
- if (!value)
- return config_error_nonbool(key);
- branch->remote_name = xstrdup(value);
+ git_config_string(&branch->remote_name, key, value);
if (branch == current_branch) {
default_remote_name = branch->remote_name;
explicit_default_remote_name = 1;
--
1.8.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] remote.c: introduce remote.pushdefault
This new configuration variable defines the default remote to push to,
and overrides `branch..remote` for all branches. It is useful
in the typical triangular-workflow setup, where the remote you're
fetching from is different from the remote you're pushing to.
Signed-off-by: Ramkumar Ramachandra
---
Documentation/config.txt | 13 ++---
remote.c | 4
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index bbba728..8ddd0fd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -723,9 +723,12 @@ branch.autosetuprebase::
This option defaults to never.
branch..remote::
- When in branch , it tells 'git fetch' and 'git push' which
- remote to fetch from/push to. It defaults to `origin` if no remote is
- configured. `origin` is also used if you are not on any branch.
+ When on branch , it tells 'git fetch' and 'git push'
+ which remote to fetch from/push to. The remote to push to
+ may be overriden with `remote.pushdefault` (for all branches).
+ If no remote is configured, or if you are not on any branch,
+ it defaults to `origin` for fetching and `remote.pushdefault`
+ for pushing.
branch..merge::
Defines, together with branch..remote, the upstream branch
@@ -1894,6 +1897,10 @@ receive.updateserverinfo::
If set to true, git-receive-pack will run git-update-server-info
after receiving data from git-push and updating refs.
+remote.pushdefault::
+ The remote to push to by default. Overrides
+ `branch..remote` for all branches.
+
remote..url::
The URL of a remote repository. See linkgit:git-fetch[1] or
linkgit:git-push[1].
diff --git a/remote.c b/remote.c
index 4704404..987edc4 100644
--- a/remote.c
+++ b/remote.c
@@ -350,6 +350,10 @@ static int handle_config(const char *key, const char
*value, void *cb)
const char *subkey;
struct remote *remote;
struct branch *branch;
+ if (!prefixcmp(key, "remote.")) {
+ if (!strcmp(key + 7, "pushdefault"))
+ git_config_string(&pushremote_name, key, value);
+ }
if (!prefixcmp(key, "branch.")) {
name = key + 7;
subkey = strrchr(name, '.');
--
1.8.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] remote.c: introduce branch..pushremote
This new configuration variable overrides `remote.pushdefault` and
`branch..remote` for pushes. In a typical triangular-workflow
setup, you would want to set `remote.pushdefault` to specify the
remote to push to for all branches, and use this option to override it
for a specific branch.
Signed-off-by: Ramkumar Ramachandra
---
Documentation/config.txt | 18 ++
remote.c | 3 +++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8ddd0fd..d0e36e9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -726,9 +726,18 @@ branch..remote::
When on branch , it tells 'git fetch' and 'git push'
which remote to fetch from/push to. The remote to push to
may be overriden with `remote.pushdefault` (for all branches).
- If no remote is configured, or if you are not on any branch,
- it defaults to `origin` for fetching and `remote.pushdefault`
- for pushing.
+ The remote to push to, for the current branch, may be further
+ overriden by `branch..pushremote`. If no remote is
+ configured, or if you are not on any branch, it defaults to
+ `origin` for fetching and `remote.pushdefault` for pushing.
+
+branch..pushremote::
+ When on branch , it overrides `branch..remote`
+ when pushing. It also overrides `remote.pushdefault` when
+ pushing from branch . In a typical triangular-workflow
+ setup, you would want to set `remote.pushdefault` to specify
+ the remote to push to for all branches, and use this option to
+ override it for a specific branch.
branch..merge::
Defines, together with branch..remote, the upstream branch
@@ -1899,7 +1908,8 @@ receive.updateserverinfo::
remote.pushdefault::
The remote to push to by default. Overrides
- `branch..remote` for all branches.
+ `branch..remote` for all branches, and is overriden by
+ `branch..pushremote` for specific branches.
remote..url::
The URL of a remote repository. See linkgit:git-fetch[1] or
diff --git a/remote.c b/remote.c
index 987edc4..a4d3d22 100644
--- a/remote.c
+++ b/remote.c
@@ -366,6 +366,9 @@ static int handle_config(const char *key, const char
*value, void *cb)
default_remote_name = branch->remote_name;
explicit_default_remote_name = 1;
}
+ } else if (!strcmp(subkey, ".pushremote")) {
+ if (branch == current_branch)
+ git_config_string(&pushremote_name, key, value);
} else if (!strcmp(subkey, ".merge")) {
if (!value)
return config_error_nonbool(key);
--
1.8.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push
Currently, do_push() in push.c calls remote_get(), which gets the
configured remote for fetching and pushing. Replace this call with a
call to pushremote_get() instead, a new function that will return the
remote configured specifically for pushing. This function tries to
work with the string pushremote_name, before falling back to the
codepath of remote_get(). This patch has no visible impact, but
serves to enable future patches to introduce configuration variables
to set this variable.
Signed-off-by: Ramkumar Ramachandra
---
builtin/push.c | 2 +-
remote.c | 49 -
remote.h | 1 +
3 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 42b129d..d447a80 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport,
int flags)
static int do_push(const char *repo, int flags)
{
int i, errs;
- struct remote *remote = remote_get(repo);
+ struct remote *remote = pushremote_get(repo);
const char **url;
int url_nr;
diff --git a/remote.c b/remote.c
index 45b69d6..4704404 100644
--- a/remote.c
+++ b/remote.c
@@ -48,6 +48,7 @@ static int branches_nr;
static struct branch *current_branch;
static const char *default_remote_name;
+static const char *pushremote_name;
static int explicit_default_remote_name;
static struct rewrites rewrites;
@@ -669,20 +670,9 @@ static int valid_remote_nick(const char *name)
return !strchr(name, '/'); /* no slash */
}
-struct remote *remote_get(const char *name)
+static struct remote *remote_get_1(const char *name, int name_given)
{
- struct remote *ret;
- int name_given = 0;
-
- read_config();
- if (name)
- name_given = 1;
- else {
- name = default_remote_name;
- name_given = explicit_default_remote_name;
- }
-
- ret = make_remote(name, 0);
+ struct remote *ret = make_remote(name, 0);
if (valid_remote_nick(name)) {
if (!valid_remote(ret))
read_remotes_file(ret);
@@ -698,6 +688,39 @@ struct remote *remote_get(const char *name)
return ret;
}
+struct remote *remote_get(const char *name)
+{
+ int name_given = 0;
+
+ read_config();
+ if (name)
+ name_given = 1;
+ else {
+ name = default_remote_name;
+ name_given = explicit_default_remote_name;
+ }
+ return remote_get_1(name, name_given);
+}
+
+struct remote *pushremote_get(const char *name)
+{
+ int name_given = 0;
+
+ read_config();
+ if (name)
+ name_given = 1;
+ else {
+ if (pushremote_name) {
+ name = pushremote_name;
+ name_given = 1;
+ } else {
+ name = default_remote_name;
+ name_given = explicit_default_remote_name;
+ }
+ }
+ return remote_get_1(name, name_given);
+}
+
int remote_is_configured(const char *name)
{
int i;
diff --git a/remote.h b/remote.h
index 251d8fd..99a437f 100644
--- a/remote.h
+++ b/remote.h
@@ -51,6 +51,7 @@ struct remote {
};
struct remote *remote_get(const char *name);
+struct remote *pushremote_get(const char *name);
int remote_is_configured(const char *name);
typedef int each_remote_fn(struct remote *remote, void *priv);
--
1.8.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-p4: support exclusively locked files
Interesting. 'Implementing sitewide pessimistic locking with p4 typemap',
http://www.perforce.com/perforce/doc.current/manuals/p4sag/03_superuser.htm
l seems to suggest this is all that's needed. I believe we're using the
default configuration, the exclusive lock on all files behaviour was
researching the exclusive locking problem,
http://ericlathrop.com/2012/12/how-to-set-up-git-p4-in-windows/, so I
thought I'd mention it.
You might be onto something w/ fstat, it doesn't include the exclusive
indicator:
... type binary+l
Latest P4 client, and fairly recent server build:
P4/DARWIN90X86_64/2012.2/536738 (2012/10/16)
P4D/LINUX26X86_64/2012.2/538478 (2012/10/16)
Danny
On 17/03/2013 20:04, "Pete Wyckoff" wrote:
>danny.tho...@blackboard.com wrote on Wed, 13 Mar 2013 13:51 -0400:
>> By default, newly added binary files are exclusively locked by Perforce:
>>
>> 'add default change (binary+l) *exclusive*'
>>
>> This results in a 'Could not determine file type' error as the regex
>> expects
>> the line to end after the file type matching group. Some repositories
>>are
>> also configured to always require exclusive locks, so may be a problem
>>for
>> all revisions in some cases.
>
>Can you explain how to configure p4d to default everything to
>binary+l? Also, what version are you using ("p4 info")? I'm
>trying to write a test case for this.
>
>I did find a way to play with typemap to get +l, as:
>
>( p4 typemap -o ; printf "\tbinary+l\t//.../bash*" ) | p4 typemap -i
>
>With this, the 2011.1 here just says:
>0
>tic-git-test$ p4 opened bash
>//depot/bash#1 - add default change (binary+l)
>
>I've not been able to make it say " *exclusive*" too.
>
>> result = p4_read_pipe(["opened", wildcard_encode(file)])
>> -match = re.match(".*\((.+)\)\r?$", result)
>> +match = re.match(".*\((.+)\)(?:.+)?\r?$", result)
>
>I think this whole function would be less brittle
>using p4's "-G" output, like:
>
>d = p4Cmd(["fstat", "-T", "type", wildcard_encode(file)])
># some error checking
>return d['type']
>
>But I'm curious if your p4d includes " *exclusive*" in the
>type there too, in which case we'll have to strip it.
>
>Thanks for starting the patch on this. It's good if we can keep
>others from running into the same problem.
>
> -- Pete
This email and any attachments may contain confidential and proprietary
information of Blackboard that is for the sole use of the intended recipient.
If you are not the intended recipient, disclosure, copying, re-distribution or
other use of any of this information is strictly prohibited. Please immediately
notify the sender and delete this transmission if you received this email in
error.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH v2 4/4] teach config parsing to read from strbuf
Hi Peff, On Thu, Mar 14, 2013 at 03:39:14AM -0400, Jeff King wrote: On Thu, Mar 14, 2013 at 03:10:46AM -0400, Jeff King wrote: > I looked into this a little. The first sticking point is that > git_config_with_options needs to support the alternate source. Here's a > sketch of what I think the solution should look like, on top of your > patches. Here it is again, with two changes: 1. Rather than handling the blob lookup inline in git_config_with_options, it adds direct functions for reading config from blob sha1s and blob references. I think this should make it easier to reuse when you are trying to read .gitmodules from C code. 2. It adds some basic tests. I'll leave it here for tonight. The next step would be to rebase it on your modified series (in particular, I think git_config_from_strbuf should become git_config_from_buf, which will impact this). Feel free to use, pick apart, rewrite, or discard as you see fit for your series. Sorry for the late reply, I was not online during the last days. Thanks a lot for this. I will use it in the next iteration of the series. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Support triangular workflows
On Mon, Mar 18, 2013 at 06:46:11PM +0530, Ramkumar Ramachandra wrote: > I've put off implementing remote.default corresponding to > remote.pushdefault, as Jeff suggested in [1], because it's currently > not an itch; apart from the obvious symmetry, I don't know what > purpose it serves: why would anyone want to fetch from a remote other > than origin by default? Why wouldn't they simply swap that remote's > name with "origin"? However, it's a nice thing to have for symmetry, > and it should be trivial to implement: any interested person is > welcome to pick it up. Yeah, I agree that it does not have much point, aside from people who have an unreasonable aversion to using the word "origin". There was a series posted last summer to add remote.default: http://article.gmane.org/gmane.comp.version-control.git/201065 It ended up stalled and never got merged. I think the main impetus was that "git clone -o foo" should leave "foo" in remote.default (of course, that still leaves unanswered why anyone would really want to use "-o foo" in the first place). I think the symmetry makes some sense, but I also think it can come later if somebody wants it. > Documentation/config.txt | 23 --- > builtin/push.c | 2 +- > remote.c | 60 > +++- > remote.h | 1 + > 4 files changed, 66 insertions(+), 20 deletions(-) No new tests? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Support triangular workflows
Jeff King wrote: > On Mon, Mar 18, 2013 at 06:46:11PM +0530, Ramkumar Ramachandra wrote: >> Documentation/config.txt | 23 --- >> builtin/push.c | 2 +- >> remote.c | 60 >> +++- >> remote.h | 1 + >> 4 files changed, 66 insertions(+), 20 deletions(-) > > No new tests? Honestly, it slipped my mind. Will write it now. Thanks for the reminder. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push
On Mon, Mar 18, 2013 at 06:46:13PM +0530, Ramkumar Ramachandra wrote:
> +struct remote *remote_get(const char *name)
> +{
> + int name_given = 0;
> +
> + read_config();
> + if (name)
> + name_given = 1;
> + else {
> + name = default_remote_name;
> + name_given = explicit_default_remote_name;
> + }
> + return remote_get_1(name, name_given);
> +}
> +
> +struct remote *pushremote_get(const char *name)
> +{
> + int name_given = 0;
> +
> + read_config();
> + if (name)
> + name_given = 1;
> + else {
> + if (pushremote_name) {
> + name = pushremote_name;
> + name_given = 1;
> + } else {
> + name = default_remote_name;
> + name_given = explicit_default_remote_name;
> + }
> + }
> + return remote_get_1(name, name_given);
> +}
Can we get rid of this duplication by having remote_get_1 take a
service-specific default argument? And then each service calls it like:
struct remote *remote_get(const char *name)
{
read_config();
return remote_get_1(name, NULL);
}
struct remote *pushremote_get(const char *name)
{
read_config();
return remote_get_1(name, pushremote_name);
}
and all of the name_given junk can stay in remote_get_1. And adding
"remote.default" would just be a matter of changing that NULL in
remote_get.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Support triangular workflows
On Mon, Mar 18, 2013 at 07:58:23PM +0530, Ramkumar Ramachandra wrote: > Jeff King wrote: > > On Mon, Mar 18, 2013 at 06:46:11PM +0530, Ramkumar Ramachandra wrote: > >> Documentation/config.txt | 23 --- > >> builtin/push.c | 2 +- > >> remote.c | 60 > >> +++- > >> remote.h | 1 + > >> 4 files changed, 66 insertions(+), 20 deletions(-) > > > > No new tests? > > Honestly, it slipped my mind. Will write it now. > > Thanks for the reminder. Thanks. Other than the suggestion I made on 2/4, I do not see anything wrong with the series. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push
Jeff King wrote:
> On Mon, Mar 18, 2013 at 06:46:13PM +0530, Ramkumar Ramachandra wrote:
>
>> +struct remote *remote_get(const char *name)
>> +{
>> + int name_given = 0;
>> +
>> + read_config();
>> + if (name)
>> + name_given = 1;
>> + else {
>> + name = default_remote_name;
>> + name_given = explicit_default_remote_name;
>> + }
>> + return remote_get_1(name, name_given);
>> +}
>> +
>> +struct remote *pushremote_get(const char *name)
>> +{
>> + int name_given = 0;
>> +
>> + read_config();
>> + if (name)
>> + name_given = 1;
>> + else {
>> + if (pushremote_name) {
>> + name = pushremote_name;
>> + name_given = 1;
>> + } else {
>> + name = default_remote_name;
>> + name_given = explicit_default_remote_name;
>> + }
>> + }
>> + return remote_get_1(name, name_given);
>> +}
>
> Can we get rid of this duplication by having remote_get_1 take a
> service-specific default argument? And then each service calls it like:
>
> struct remote *remote_get(const char *name)
> {
> read_config();
> return remote_get_1(name, NULL);
> }
>
> struct remote *pushremote_get(const char *name)
> {
> read_config();
> return remote_get_1(name, pushremote_name);
> }
Thanks for the dose of sanity. While at it, why not move
read_config() to remote_get_1() as well?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push
On Mon, Mar 18, 2013 at 08:26:46PM +0530, Ramkumar Ramachandra wrote:
> > Can we get rid of this duplication by having remote_get_1 take a
> > service-specific default argument? And then each service calls it like:
> >
> > struct remote *remote_get(const char *name)
> > {
> > read_config();
> > return remote_get_1(name, NULL);
> > }
> >
> > struct remote *pushremote_get(const char *name)
> > {
> > read_config();
> > return remote_get_1(name, pushremote_name);
> > }
>
> Thanks for the dose of sanity. While at it, why not move
> read_config() to remote_get_1() as well?
Because it sets pushremote_name, and therefore you would just be passing
NULL if read_config has not been run yet. But if you made it:
return remote_get_1(name, &pushremote_name);
that would work.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
Michael Haggerty writes: > Signed-off-by: Michael Haggerty > > and ACK for the whole series, once Junio's points are addressed. > > Regarding Junio's readability suggestion: I agree that his versions are > a bit more readable, albeit at the expense of having to evaluate a bit > more logic for each reference rather than just once when the header line > is handled. So I don't have a preference either way. The way the conditional is written, in the longer term we will almost always compare "peeled == PEELED_FULLY", and otherwise we will do the same !prefixcmp(refs/tags/), so I do not think there is "more logic" that matters compared to the original. Thanks, both; will replace what was queued with "SQUASH???". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git build fails on `make`, undefined references in libcrypto.a.
On Mon, Mar 18, 2013 at 1:29 AM, Konstantin Khomoutov wrote: > FYI, I've already tried to answer this exact question [1] with no > comments from the OP. > > 1. http://serverfault.com/a/488604/118848 It is I who posted that question. :P I haven't made any comments yet because this issue is still a work in progress. I re-compiled OpenSSL taking into consideration that libdl.a probably wasn't linked properly to libcrypto.a, and have also tried re-compiling Git afterward, but with the same errors. I have also created a .conf file for `ld.so`, and ran `sudo ldconfig` which solved some other issues I was having. Running `ldd msgfmt` revealed that run-time libraries were not being found, and after running `sudo ldconfig`, `ldd msgfmt` then showed the libgettext .so was linked. So, I solved some of my issues, and if there was an issue with statically linked libraries not being found, as in this case, then, I believe re-compiling OpenSSL with the proper LDFLAGS and CPPFLAGS would have solved the issue, but they have not. I'm currently attempting to install GCC 4.7.2, which is having some other issues with texinfo 5.1. I can't find the link, but I someone said it could be the compiler version... since everything else that seems like might be the issue isn't fixing it, I'm going to try re-compiling OpenSSL with GCC 4.7.2 and see how that goes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Support triangular workflows
On 13-03-18 10:25 AM, Jeff King wrote: > On Mon, Mar 18, 2013 at 06:46:11PM +0530, Ramkumar Ramachandra wrote: > >> I've put off implementing remote.default corresponding to >> remote.pushdefault, as Jeff suggested in [1], because it's currently >> not an itch; apart from the obvious symmetry, I don't know what >> purpose it serves: why would anyone want to fetch from a remote other >> than origin by default? Why wouldn't they simply swap that remote's >> name with "origin"? However, it's a nice thing to have for symmetry, >> and it should be trivial to implement: any interested person is >> welcome to pick it up. > > Yeah, I agree that it does not have much point, aside from people who > have an unreasonable aversion to using the word "origin". There was a > series posted last summer to add remote.default: > > http://article.gmane.org/gmane.comp.version-control.git/201065 > > It ended up stalled and never got merged. I think the main impetus was > that "git clone -o foo" should leave "foo" in remote.default (of course, > that still leaves unanswered why anyone would really want to use "-o > foo" in the first place). I'm the guy who dropped the ball on that series. I still intend to pick it up (honest!) but I just haven't had the time. The impetus was originally getting relative submodule paths to work on detached HEADs [1]. My patch for that doesn't work when someone does "clone -o", because various parts of git assume there's a remote named "origin". The discussion led to the idea of using the remote name specified during the initial clone, and implementing that as a remote.default config value. As for why "clone -o" exists, it was added in v1.1.0: commit e6c310fd0d7384973efc6b1d5999a5e8a5b2f3bd Author: Johannes Schindelin Date: Thu Dec 22 23:37:24 2005 +0100 git-clone: Support changing the origin branch with -o Earlier, git-clone stored upstream's master in the branch named 'origin', possibly overwriting an existing such branch. Now you can change it by calling git-clone with '-o '. [jc: added ref format check, subdirectory safety, documentation and usage string.] Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano It sounds like the original need for the -o option is no longer pertinent. OTOH, for folks who deal with several remotes it's nice to name them, and "origin" isn't necessarily a useful or intuitive name. M. [1] http://thread.gmane.org/gmane.comp.version-control.git/200145 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] pack-refs: add fully-peeled trait
Jeff King writes: > Here's the re-roll. Above reasoning (elided) look sensible. Thanks; will replace. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Make GIT_USE_LOOKUP default?
Jeff King writes: > By the way, looking at that made me think for a few minutes about > hashcmp, and I was surprised to find that we use an open-coded > comparison loop. That dates back to this thread by Ingo: > > http://article.gmane.org/gmane.comp.version-control.git/172286 > > I could not replicate his benchmarks at all. In fact, my measurements > showed a slight slowdown with 1a812f3 (hashcmp(): inline memcmp() by > hand to optimize, 2011-04-28). > > Here are my best-of-five numbers for running "git rev-list --objects > --all >/dev/null" on linux-2.6.git: > > [current master, compiled with -O2] > real0m45.612s > user0m45.140s > sys 0m0.300s > ... > [revert 1a812f3 (i.e., go back to memcmp), -O2] > real0m44.358s > user0m43.876s > sys 0m0.316s > ... > I wonder why we get such different numbers. Ingo said his tests are on a > Nehalem CPU, as are mine (mine is an i7-840QM). I wonder if we should be > wrapping the optimization in an #ifdef, but I'm not sure which flag we > should be checking. FWIW, I am getting something like this on my $ grep 'model name' /proc/cpuinfo | uniq -c 4 model name : Intel(R) Core(TM)2 Quad CPU Q9450 @ 2.66GHz The same "rev-list --objects --all >/dev/null", best of five: [current master, compiled with -O2] real0m39.956s user0m39.562s sys 0m0.396s [revert 1a812f3 (i.e., go back to memcmp), -O2] real0m42.161s user0m41.879s sys 0m0.284s It could be that the difference may be how well memcmp() is optimized, no? My box runs Debian with libc6 2.11.3-4 and gcc 4.4.5. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Make GIT_USE_LOOKUP default?
On Mon, Mar 18, 2013 at 09:44:20AM -0700, Junio C Hamano wrote: > FWIW, I am getting something like this on my > > $ grep 'model name' /proc/cpuinfo | uniq -c > 4 model name : Intel(R) Core(TM)2 Quad CPU Q9450 @ 2.66GHz > > The same "rev-list --objects --all >/dev/null", best of five: > > [current master, compiled with -O2] > real0m39.956s > user0m39.562s > sys 0m0.396s > > [revert 1a812f3 (i.e., go back to memcmp), -O2] > real0m42.161s > user0m41.879s > sys 0m0.284s > > It could be that the difference may be how well memcmp() is > optimized, no? My box runs Debian with libc6 2.11.3-4 and gcc > 4.4.5. Yeah, that would make sense. I have libc6 2.13-38 and gcc 4.7.2 (debian unstable). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ITCH] Specify refspec without remote
On Mon, Mar 18, 2013 at 10:28:59PM +0530, Ramkumar Ramachandra wrote: > This has irritated me for a long time. I often end up doing: > > $ git push master:master +pu:pu Me too. > Is there a reason for the remote not being optional, or are we just > waiting for a patch? The only problem I can foresee is very minor: > there is a ref with the same name as a remote; in this case, we'd have > to specify both the remote and the ref. I think the ambiguity is a little more complex than that, because we cannot enumerate the universe of all remotes. Keep in mind that we can take either a configured remote or a URL (or ssh host). So what does: git push foo:bar mean? Is it pushing "refs/heads/foo" to "refs/heads/bar" on "origin"? Or is it using the default refspecs to push to the "bar" repo on the host "foo" over ssh? So you would need some heuristics based on whether something was a valid refspec, or could be a valid remote name or URL. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Make GIT_USE_LOOKUP default?
Jeff King writes: > On Mon, Mar 18, 2013 at 09:44:20AM -0700, Junio C Hamano wrote: > >> FWIW, I am getting something like this on my >> >> $ grep 'model name' /proc/cpuinfo | uniq -c >> 4 model name : Intel(R) Core(TM)2 Quad CPU Q9450 @ 2.66GHz >> >> The same "rev-list --objects --all >/dev/null", best of five: >> >> [current master, compiled with -O2] >> real0m39.956s >> user0m39.562s >> sys 0m0.396s >> >> [revert 1a812f3 (i.e., go back to memcmp), -O2] >> real0m42.161s >> user0m41.879s >> sys 0m0.284s >> >> It could be that the difference may be how well memcmp() is >> optimized, no? My box runs Debian with libc6 2.11.3-4 and gcc >> 4.4.5. > > Yeah, that would make sense. I have libc6 2.13-38 and gcc 4.7.2 (debian > unstable). Just for fun, here is a totally unrelated comparison, both with current master, compiled with -O2 and running on the same box. [without GIT_USE_LOOKUP] real0m39.906s real0m40.020s real0m40.022s real0m40.027s real0m40.146s [with GIT_USE_LOOKUP] real0m40.336s real0m40.376s real0m40.452s real0m40.503s real0m40.572s Maybe the Newton-Raphson is right as a concept (it does seem to result in fewer minor-faults) but the current code is implemented poorly and has a huge room for improvement? [without GIT_USE_LOOKUP] 0inputs+0outputs (0major+182895minor)pagefaults 0swaps 0inputs+0outputs (0major+182920minor)pagefaults 0swaps 0inputs+0outputs (0major+183004minor)pagefaults 0swaps 0inputs+0outputs (0major+183006minor)pagefaults 0swaps 0inputs+0outputs (0major+183036minor)pagefaults 0swaps [with GIT_USE_LOOKUP] 0inputs+0outputs (0major+182803minor)pagefaults 0swaps 0inputs+0outputs (0major+182886minor)pagefaults 0swaps 0inputs+0outputs (0major+182967minor)pagefaults 0swaps 0inputs+0outputs (0major+182997minor)pagefaults 0swaps 0inputs+0outputs (0major+182998minor)pagefaults 0swaps -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Make GIT_USE_LOOKUP default?
On Mon, Mar 18, 2013 at 10:08:11AM -0700, Junio C Hamano wrote:
> Just for fun, here is a totally unrelated comparison, both with
> current master, compiled with -O2 and running on the same box.
>
> [without GIT_USE_LOOKUP]
> real0m39.906s
> real0m40.020s
> real0m40.022s
> real0m40.027s
> real0m40.146s
>
> [with GIT_USE_LOOKUP]
> real0m40.336s
> real0m40.376s
> real0m40.452s
> real0m40.503s
> real0m40.572s
>
> Maybe the Newton-Raphson is right as a concept (it does seem to
> result in fewer minor-faults) but the current code is implemented
> poorly and has a huge room for improvement?
I do not see anything obviously wrong in it, though I did not walk
through all of the ofs calculation to look for any clever speedups.
However, I think it is clear from the other timings and Ingo's thread
that glibc 2.11's memcmp does not perform very well on many short reads.
And sha1_entry_pos will do memcmps even smaller than 20 bytes.
What happens if you do this?
diff --git a/sha1-lookup.c b/sha1-lookup.c
index c4dc55d..4ea03bd 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -102,6 +102,17 @@ int sha1_pos(const unsigned char *sha1, void *table,
size_t nr,
return -lo-1;
}
+static int short_memcmp(const unsigned char *a,
+ const unsigned char *b,
+ int len)
+{
+ int i;
+ for (i = 0; i < len; i++, a++, b++)
+ if (*a != *b)
+ return *a - *b;
+ return 0;
+}
+
/*
* Conventional binary search loop looks like this:
*
@@ -257,7 +268,7 @@ int sha1_entry_pos(const void *table,
lo, mi, hi, sha1_to_hex(key));
mi_key = base + elem_size * mi + key_offset;
- cmp = memcmp(mi_key + ofs_0, key + ofs_0, 20 - ofs_0);
+ cmp = short_memcmp(mi_key + ofs_0, key + ofs_0, 20 - ofs_0);
if (!cmp)
return mi;
if (cmp > 0) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] read-cache: avoid memcpy in expand_name_field in index v4
Nguyễn Thái Ngọc Duy writes:
> perf reports memcpy at the the 6th position [1] in "git status -uno"
> using index v4, and strbuf_remove() in expand_name_field() accounts
> for 25% of that. What we need here is a simple string cut and a
> cheaper strbuf_setlen() should be enough.
While it is true that strbuf_remove(&sb, sb.len - trim, trim) is
equivalent to strbuf_setlen(&sb, sb.len - trim), I wonder why we see
any memcpy() in the first place.
strbuf_remove(&sb, sb.len - trim, trim) is turned into
strbuf_splice(&sb, sb.len - trim, trim, NULL, 0) and then in turn it
does these two:
memmove(sb.buf + (sb.len - trim) + 0,
sb.buf + sb.len, 0);
memcpy(sb.buf + (sb.len - trim), NULL, 0);
both of which should be a no-op, no?
There also is this call that has the same "trim at the right end":
pretty.c: strbuf_remove(sb, sb->len - trimlen, trimlen);
It almost makes me suggest that it may be a better solution to make
strbuf_remove() more intelligent about such a call pattern _if_
these empty memmove/memcpy are so expensive, perhaps like the
attached. It could be that strbuf_splice() should be the one that
ought to be more intelligent, but I'll leave it up to you to
benchmark to find out where the best place to optimize is.
strbuf.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/strbuf.c b/strbuf.c
index 05d0693..12db700 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -179,7 +179,10 @@ void strbuf_insert(struct strbuf *sb, size_t pos, const
void *data, size_t len)
void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
{
- strbuf_splice(sb, pos, len, NULL, 0);
+ if (pos + len == sb->len)
+ strbuf_setlen(sb, pos);
+ else
+ strbuf_splice(sb, pos, len, NULL, 0);
}
void strbuf_add(struct strbuf *sb, const void *data, size_t len)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: random server hacks on top of git
Jeff King writes: > - blame-tree; re-rolled from my submission last year to build on top > > - diff --max-depth; this is a requirement to do blame-tree efficiently Both look mildly interesting, especially after the magic pathspec settles the latter may be a good addition. > - share ref selection code between "git branch", "git tag", and "git > for-each-ref". Nice. >- receive.maxsize; index-pack will happily spool data to disk Again nice. >- receive.advertisealternates; basically turn off ".have" > advertisement. I think this is a sane thing to do, especially with some hints on the most common reference tree everybody is expected to know about. > - receive.hiderefs; this is going to become redundant with Junio's > implementation Yup. > - an audit reflog; we keep a reflog for all refs at the root of the > repository. It differs from a regular reflog in that: > > 1. It never expires. > > 2. It is not part of reachability analysis. > > 3. It includes the refname for each entry, so you can see >deletions. Interesting. > - ignore some fsck warnings under transfer.fsckobjects; some of them > are annoyingly common when people pull old history from an > existing project and try to push it back up. Depending on the implementation, this may be very much valuable. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 27/45] Convert run_add_interactive to use struct pathspec
On Fri, Mar 15, 2013 at 01:06:42PM +0700, Nguyễn Thái Ngọc Duy wrote:
> This passes the pathspec, more or less unmodified, to
> git-add--interactive. The command itself does not process pathspec. It
> simply passes the pathspec to other builtin commands. So if all those
> commands support pathspec, we're good.
This breaks "git reset --keep" in a subdirectory for me.
I ran "git reset --keep " in a subdirectory and got:
fatal: BUG: parse_pathspec cannot take no argument in this case
Bisecting points to this commit.
The simplest test case is:
( cd t && ../bin-wrappers/git reset --keep HEAD )
which works on master but not pu.
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/add.c | 26 ++
> builtin/checkout.c | 9 -
> builtin/reset.c| 8
> commit.h | 2 +-
> 4 files changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index ec6fbe3..2b20d7d 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -139,16 +139,12 @@ static void refresh(int verbose, const char **pathspec)
> }
>
> int run_add_interactive(const char *revision, const char *patch_mode,
> - const char **pathspec)
> + const struct pathspec *pathspec)
> {
> - int status, ac, pc = 0;
> + int status, ac, i;
> const char **args;
>
> - if (pathspec)
> - while (pathspec[pc])
> - pc++;
> -
> - args = xcalloc(sizeof(const char *), (pc + 5));
> + args = xcalloc(sizeof(const char *), (pathspec->nr + 6));
> ac = 0;
> args[ac++] = "add--interactive";
> if (patch_mode)
> @@ -156,11 +152,9 @@ int run_add_interactive(const char *revision, const char
> *patch_mode,
> if (revision)
> args[ac++] = revision;
> args[ac++] = "--";
> - if (pc) {
> - memcpy(&(args[ac]), pathspec, sizeof(const char *) * pc);
> - ac += pc;
> - }
> - args[ac] = NULL;
> + for (i = 0; i < pathspec->nr; i++)
> + /* pass original pathspec, to be re-parsed */
> + args[ac++] = pathspec->items[i].original;
>
> status = run_command_v_opt(args, RUN_GIT_CMD);
> free(args);
> @@ -175,17 +169,17 @@ int interactive_add(int argc, const char **argv, const
> char *prefix, int patch)
>* git-add--interactive itself does not parse pathspec. It
>* simply passes the pathspec to other builtin commands. Let's
>* hope all of them support all magic, or we'll need to limit
> - * the magic here. There is still a problem with prefix. But
> - * that'll be worked on later on.
> + * the magic here.
>*/
> parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,
> PATHSPEC_PREFER_FULL |
> -PATHSPEC_SYMLINK_LEADING_PATH,
> +PATHSPEC_SYMLINK_LEADING_PATH |
> +PATHSPEC_PREFIX_ORIGIN,
> prefix, argv);
>
> return run_add_interactive(NULL,
> patch ? "--patch" : NULL,
> -pathspec.raw);
> +&pathspec);
> }
>
> static int edit_patch(int argc, const char **argv, const char *prefix)
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3c19cb4..2ddff95 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -256,7 +256,7 @@ static int checkout_paths(const struct checkout_opts
> *opts,
>
> if (opts->patch_mode)
> return run_add_interactive(revision, "--patch=checkout",
> -opts->pathspec.raw);
> +&opts->pathspec);
>
> lock_file = xcalloc(1, sizeof(struct lock_file));
>
> @@ -1115,10 +1115,9 @@ int cmd_checkout(int argc, const char **argv, const
> char *prefix)
>* cannot handle. Magic mask is pretty safe to be
>* lifted for new magic when opts.patch_mode == 0.
>*/
> - parse_pathspec(&opts.pathspec,
> -opts.patch_mode == 0 ? 0 :
> -(PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP),
> -0, prefix, argv);
> + parse_pathspec(&opts.pathspec, 0,
> +opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,
> +prefix, argv);
>
> if (!opts.pathspec.nr)
> die(_("invalid path specification"));
> diff --git a/builtin/reset.c b/builtin/reset.c
> index da7282e..7c6e8b6 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -216,9 +216,9 @@ static void parse_args(struct pathspec *pathspec,
> }
> }
> *rev_ret = rev;
> - parse_pathspec(pathspec,
> -patch_mode ? PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP : 0,
> -PATHSPE
Re: Make GIT_USE_LOOKUP default?
Jeff King writes: > I do not see anything obviously wrong in it, though I did not walk > through all of the ofs calculation to look for any clever speedups. > However, I think it is clear from the other timings and Ingo's thread > that glibc 2.11's memcmp does not perform very well on many short reads. > And sha1_entry_pos will do memcmps even smaller than 20 bytes. > > What happens if you do this? The overall trend is the same. [without GIT_USE_LOOKUP] real0m40.044s real0m40.054s real0m40.072s real0m40.097s real0m40.159s [with GIT_USE_LOOKUP] real0m40.257s real0m40.281s real0m40.311s real0m40.366s real0m40.407s I suspect that after the first few rounds the range shrinks small enough that the difference between a simple "mi = (hi + lo)/2" and convoluted ofs computation becomes dominant. Perhaps we should only do N-R for the initial midpoint selection and then fall back to a stupid binary search? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ITCH] pull.default
Ramkumar Ramachandra writes: > I usually use `git fetch`, inspect state, and then merge/ rebase > accordingly. Unfortunately, this is very sub-optimal as I can > automate this 80% of the time. I want to be able to decide what to do > on a repository-specific basis, and hence propose a pull.default which > can be set to the following: > 1. fetch. Just fetch. (I will set this as my default in ~/.gitconfig) > 2. fast-forward. Fetch. If the FETCH_HEAD is directly ahead of HEAD, > `stash`, merge, and stash apply. Otherwise, do nothing. > 3. rebase. Fetch. stash, rebase, stash apply. `git pull n` will do > rebase --onto master HEAD~n instead of rebase. > 4. reset. Fetch, stash, reset --hard FETCH_HEAD, stash apply. > > Ofcourse, it should print a message saying what it did at the end. > > What do you think? Many other possibilities are missing. "fetch and merge", for example. You seem to be generalizing the "--rebase" and "--ff-only" options of "git pull" with 2 and 3, which may (or may not) make sense. I think you can shrink and enhance the above repertoire at the same time by separating "do I want to have stash and stash pop around" bit into an orthogonal axis. The other orthogonal axes are "Under what condition do I integrate the work from the upstream?" (e.g. "only when I do not have anything, aka, ff-only") and "How would I integrate the work from the upstream?" (e.g. "rebase my work" and "discard anything I did aka reset --hard"). By the way, I do not think you should start your design from a configuration (this is a general principle, not limited to this case). Think about what the smallest number of independent options you need to add to express various workflows, and then turn them into configuration variables that can set the default, all of which have to be overridable from the command line. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ITCH] pull.default
Junio C Hamano wrote: > Ramkumar Ramachandra writes: > >> I usually use `git fetch`, inspect state, and then merge/ rebase >> accordingly. Unfortunately, this is very sub-optimal as I can >> automate this 80% of the time. I want to be able to decide what to do >> on a repository-specific basis, and hence propose a pull.default which >> can be set to the following: >> 1. fetch. Just fetch. (I will set this as my default in ~/.gitconfig) >> 2. fast-forward. Fetch. If the FETCH_HEAD is directly ahead of HEAD, >> `stash`, merge, and stash apply. Otherwise, do nothing. >> 3. rebase. Fetch. stash, rebase, stash apply. `git pull n` will do >> rebase --onto master HEAD~n instead of rebase. >> 4. reset. Fetch, stash, reset --hard FETCH_HEAD, stash apply. >> >> Ofcourse, it should print a message saying what it did at the end. >> >> What do you think? > > Many other possibilities are missing. "fetch and merge", for > example. > > You seem to be generalizing the "--rebase" and "--ff-only" options > of "git pull" with 2 and 3, which may (or may not) make sense. > > I think you can shrink and enhance the above repertoire at the same > time by separating "do I want to have stash and stash pop around" > bit into an orthogonal axis. The other orthogonal axes are "Under > what condition do I integrate the work from the upstream?" (e.g. > "only when I do not have anything, aka, ff-only") and "How would I > integrate the work from the upstream?" (e.g. "rebase my work" and > "discard anything I did aka reset --hard"). Excellent I was hoping to start a discussion like this. My initial thought was designing a custom script that git-pull will execute like a hook; we should, however, be able to get away with designing enough configuration orthogonal configuration variables. For anything more complex, just do it by hand! > By the way, I do not think you should start your design from a > configuration (this is a general principle, not limited to this > case). Think about what the smallest number of independent options > you need to add to express various workflows, and then turn them > into configuration variables that can set the default, all of which > have to be overridable from the command line. Will do. Expect a draft soon. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/8] Improve git-status --ignored
This patch series addresses several bugs and performance issues in .gitignore processing that came up in the inotify discussion. Also available here: https://github.com/kblees/git/tree/kb/improve-git-status-ignored git pull git://github.com/kblees/git.git kb/improve-git-status-ignored Patches 1 - 4 fix bugs in 'git-status --ignored' and add appropriate test cases. Patches 5 - 7 eliminate the is_path_excluded API, in favor of a slightly improved and faster is_excluded. This speeds up 'git-ls-files --cached --ignored' by factor 5 - 6. Patch 8 finally skips excluded checks for tracked files. With the bugs and is_path_excluded out of the way, it should be obvious that this can safely be done unconditionally without introducing regressions. Speeds up 'git-status [--ignored]' by factor 1.4 - 2. I still believe that 'git-status --ignored' shouldn't list "ignored tracked" directories, to be consistent with the listing of untracked directories, and because "ignored tracked" contradicts the very definition of ignored content in gitignore(5). Cheers, Karsten Karsten Blees (8): dir.c: git-status --ignored: don't drop ignored directories dir.c: git-status --ignored: don't list files in ignored directories dir.c: git-status --ignored: don't list empty ignored directories dir.c: git-status --ignored: don't list empty directories as ignored dir.c: move prep_exclude and factor out parts of last_exclude_matching dir.c: unify is_excluded and is_path_excluded APIs dir.c: replace is_path_excluded with now equivalent is_excluded API dir.c: git-status: avoid is_excluded checks for tracked files builtin/add.c | 5 +- builtin/check-ignore.c | 6 +- builtin/ls-files.c | 15 +- dir.c | 351 + dir.h | 22 +-- t/t7061-wtstatus-ignore.sh | 104 +- unpack-trees.c | 10 +- unpack-trees.h | 1 - 8 files changed, 243 insertions(+), 271 deletions(-) -- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] dir.c: git-status --ignored: don't drop ignored directories
'git-status --ignored' drops ignored directories if they contain untracked
files in an untracked sub directory.
Fix it by getting exact (recursive) excluded status in treat_directory.
Signed-off-by: Karsten Blees
---
dir.c | 9 +
t/t7061-wtstatus-ignore.sh | 27 +++
2 files changed, 36 insertions(+)
diff --git a/dir.c b/dir.c
index 57394e4..ec4eebf 100644
--- a/dir.c
+++ b/dir.c
@@ -1060,6 +1060,15 @@ static enum directory_treatment treat_directory(struct
dir_struct *dir,
/* This is the "show_other_directories" case */
+ /* might be a sub directory in an excluded directory */
+ if (!exclude) {
+ struct path_exclude_check check;
+ int dt = DT_DIR;
+ path_exclude_check_init(&check, dir);
+ exclude = is_path_excluded(&check, dirname, len, &dt);
+ path_exclude_check_clear(&check);
+ }
+
/*
* We are looking for ignored files and our directory is not ignored,
* check if it contains only ignored files
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 0da1214..0f1034e 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -143,4 +143,31 @@ test_expect_success 'status ignored tracked directory and
uncommitted file with
test_cmp expected actual
'
+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
+!! tracked/
+EOF
+
+test_expect_success 'status ignored tracked directory with uncommitted file in
untracked subdir with --ignore' '
+ rm -rf tracked/uncommitted &&
+ mkdir tracked/ignored &&
+ : >tracked/ignored/uncommitted &&
+ git status --porcelain --ignored >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
+!! tracked/ignored/uncommitted
+EOF
+
+test_expect_success 'status ignored tracked directory with uncommitted file in
untracked subdir with --ignore -u' '
+ git status --porcelain --ignored -u >actual &&
+ test_cmp expected actual
+'
+
test_done
--
1.8.1.2.8021.g7e51819
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/8] dir.c: git-status --ignored: don't list files in ignored directories
'git-status --ignored' lists both the ignored directory and the ignored
files if the files are in a tracked sub directory.
When recursing into sub directories in read_directory_recursive, pass on
the check_only parameter so that we don't accidentally add the files.
Signed-off-by: Karsten Blees
---
dir.c | 4 +---
t/t7061-wtstatus-ignore.sh | 27 +++
2 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/dir.c b/dir.c
index ec4eebf..7c9bc9c 100644
--- a/dir.c
+++ b/dir.c
@@ -1273,7 +1273,6 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
return path_ignored;
case DT_DIR:
strbuf_addch(path, '/');
-
switch (treat_directory(dir, path->buf, path->len, exclude,
simplify)) {
case show_directory:
break;
@@ -1343,8 +1342,7 @@ static int read_directory_recursive(struct dir_struct
*dir,
switch (treat_path(dir, de, &path, baselen, simplify)) {
case path_recurse:
contents += read_directory_recursive(dir, path.buf,
-path.len, 0,
-simplify);
+ path.len, check_only, simplify);
continue;
case path_ignored:
continue;
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 0f1034e..4ece129 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -170,4 +170,31 @@ test_expect_success 'status ignored tracked directory with
uncommitted file in u
test_cmp expected actual
'
+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
+!! tracked/
+EOF
+
+test_expect_success 'status ignored tracked directory with uncommitted file in
tracked subdir with --ignore' '
+ : >tracked/ignored/committed &&
+ git add -f tracked/ignored/committed &&
+ git commit -m. &&
+ git status --porcelain --ignored >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
+!! tracked/ignored/uncommitted
+EOF
+
+test_expect_success 'status ignored tracked directory with uncommitted file in
tracked subdir with --ignore -u' '
+ git status --porcelain --ignored -u >actual &&
+ test_cmp expected actual
+'
+
test_done
--
1.8.1.2.8021.g7e51819
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/8] dir.c: git-status --ignored: don't list empty ignored directories
'git-status --ignored' lists ignored tracked directories without any
ignored files if a tracked file happens to match an exclude pattern.
Always exclude tracked files.
Signed-off-by: Karsten Blees
---
dir.c | 11 ---
t/t7061-wtstatus-ignore.sh | 24
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/dir.c b/dir.c
index 7c9bc9c..fd1f088 100644
--- a/dir.c
+++ b/dir.c
@@ -1109,16 +1109,13 @@ static int treat_file(struct dir_struct *dir, struct
strbuf *path, int exclude,
struct path_exclude_check check;
int exclude_file = 0;
+ /* Always exclude indexed files */
+ if (index_name_exists(&the_index, path->buf, path->len, ignore_case))
+ return 1;
+
if (exclude)
exclude_file = !(dir->flags & DIR_SHOW_IGNORED);
else if (dir->flags & DIR_SHOW_IGNORED) {
- /* Always exclude indexed files */
- struct cache_entry *ce = index_name_exists(&the_index,
- path->buf, path->len, ignore_case);
-
- if (ce)
- return 1;
-
path_exclude_check_init(&check, dir);
if (!is_path_excluded(&check, path->buf, path->len, dtype))
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 4ece129..28b7d95 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -122,10 +122,34 @@ cat >expected <<\EOF
?? .gitignore
?? actual
?? expected
+EOF
+
+test_expect_success 'status ignored tracked directory and ignored file with
--ignore' '
+ echo "committed" >>.gitignore &&
+ git status --porcelain --ignored >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
+EOF
+
+test_expect_success 'status ignored tracked directory and ignored file with
--ignore -u' '
+ git status --porcelain --ignored -u >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
!! tracked/
EOF
test_expect_success 'status ignored tracked directory and uncommitted file
with --ignore' '
+ echo "tracked" >.gitignore &&
: >tracked/uncommitted &&
git status --porcelain --ignored >actual &&
test_cmp expected actual
--
1.8.1.2.8021.g7e51819
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/8] dir.c: move prep_exclude and factor out parts of last_exclude_matching
Move code around in preparation for the next patch.
Signed-off-by: Karsten Blees
---
dir.c | 181 ++
1 file changed, 94 insertions(+), 87 deletions(-)
diff --git a/dir.c b/dir.c
index 64457db..417feaa 100644
--- a/dir.c
+++ b/dir.c
@@ -549,78 +549,6 @@ void add_excludes_from_file(struct dir_struct *dir, const
char *fname)
die("cannot use %s as an exclude file", fname);
}
-/*
- * Loads the per-directory exclude list for the substring of base
- * which has a char length of baselen.
- */
-static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
-{
- struct exclude_list_group *group;
- struct exclude_list *el;
- struct exclude_stack *stk = NULL;
- int current;
-
- if ((!dir->exclude_per_dir) ||
- (baselen + strlen(dir->exclude_per_dir) >= PATH_MAX))
- return; /* too long a path -- ignore */
-
- group = &dir->exclude_list_group[EXC_DIRS];
-
- /* Pop the exclude lists from the EXCL_DIRS exclude_list_group
-* which originate from directories not in the prefix of the
-* path being checked. */
- while ((stk = dir->exclude_stack) != NULL) {
- if (stk->baselen <= baselen &&
- !strncmp(dir->basebuf, base, stk->baselen))
- break;
- el = &group->el[dir->exclude_stack->exclude_ix];
- dir->exclude_stack = stk->prev;
- free((char *)el->src); /* see strdup() below */
- clear_exclude_list(el);
- free(stk);
- group->nr--;
- }
-
- /* Read from the parent directories and push them down. */
- current = stk ? stk->baselen : -1;
- while (current < baselen) {
- struct exclude_stack *stk = xcalloc(1, sizeof(*stk));
- const char *cp;
-
- if (current < 0) {
- cp = base;
- current = 0;
- }
- else {
- cp = strchr(base + current + 1, '/');
- if (!cp)
- die("oops in prep_exclude");
- cp++;
- }
- stk->prev = dir->exclude_stack;
- stk->baselen = cp - base;
- memcpy(dir->basebuf + current, base + current,
- stk->baselen - current);
- strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
- /*
-* dir->basebuf gets reused by the traversal, but we
-* need fname to remain unchanged to ensure the src
-* member of each struct exclude correctly
-* back-references its source file. Other invocations
-* of add_exclude_list provide stable strings, so we
-* strdup() and free() here in the caller.
-*/
- el = add_exclude_list(dir, EXC_DIRS, strdup(dir->basebuf));
- stk->exclude_ix = group->nr - 1;
- add_excludes_from_file_to_list(dir->basebuf,
- dir->basebuf, stk->baselen,
- el, 1);
- dir->exclude_stack = stk;
- current = stk->baselen;
- }
- dir->basebuf[baselen] = '\0';
-}
-
int match_basename(const char *basename, int basenamelen,
const char *pattern, int prefix, int patternlen,
int flags)
@@ -751,25 +679,13 @@ int is_excluded_from_list(const char *pathname,
return -1; /* undecided */
}
-/*
- * Loads the exclude lists for the directory containing pathname, then
- * scans all exclude lists to determine whether pathname is excluded.
- * Returns the exclude_list element which matched, or NULL for
- * undecided.
- */
-static struct exclude *last_exclude_matching(struct dir_struct *dir,
-const char *pathname,
-int *dtype_p)
+static struct exclude *last_exclude_matching_from_lists(struct dir_struct *dir,
+ const char *pathname, int pathlen, const char *basename,
+ int *dtype_p)
{
- int pathlen = strlen(pathname);
int i, j;
struct exclude_list_group *group;
struct exclude *exclude;
- const char *basename = strrchr(pathname, '/');
- basename = (basename) ? basename+1 : pathname;
-
- prep_exclude(dir, pathname, basename-pathname);
-
for (i = EXC_CMDL; i <= EXC_FILE; i++) {
group = &dir->exclude_list_group[i];
for (j = group->nr - 1; j >= 0; j--) {
@@ -781,6 +697,97 @@ static struct exclude *last_exclude_matching(struct
dir_struct *dir,
}
}
return NULL;
+};
+
+/*
+ * Loads the per-directory exclude list for the substring of base
+ * w
[PATCH 6/8] dir.c: unify is_excluded and is_path_excluded APIs
The is_excluded and is_path_excluded APIs are very similar, except for a
few noteworthy differences:
is_excluded doesn't handle ignored directories, results for paths within
ignored directories are incorrect. This is probably based on the premise
that recursive directory scans should stop at ignored directories, which
is no longer true (in certain cases, read_directory_recursive currently
calls is_excluded *and* is_path_excluded to get correct ignored state).
is_excluded caches parsed .gitignore files of the last directory in struct
dir_struct. If the directory changes, it finds a common parent directory
and is very careful to drop only as much state as necessary. On the other
hand, is_excluded will also read and parse .gitignore files in already
ignored directories, which are completely irrelevant.
is_path_excluded correctly handles ignored directories by checking if any
component in the path is excluded. As it uses is_excluded internally, this
unfortunately forces is_excluded to drop and re-read all .gitignore files,
as there is no common parent directory for the root dir.
is_path_excluded tracks state in a separate struct path_exclude_check,
which is essentially a wrapper of dir_struct with two more fields. However,
as is_path_excluded also modifies dir_struct, it is not possible to e.g.
use multiple path_exclude_check structures with the same dir_struct in
parallel. The additional structure just unnecessarily complicates the API.
Teach is_excluded / prep_exclude about ignored directories: whenever
entering a new directory, first check if the entire directory is excluded.
Remember the excluded state in dir_struct. Don't traverse into already
ignored directories (i.e. don't read irrelevant .gitignore files).
Directories could also be excluded by exclude patterns specified on the
command line or .git/info/exclude, so we cannot simply skip prep_exclude
entirely if there's no .gitignore file name (dir_struct.exclude_per_dir).
Move this check to just before acutally reading the file.
is_path_excluded is now equivalent to is_excluded, so we can simply
redirect to it (the public API is cleaned up in the next patch).
The performance impact of the additional ignored check per directory is
hardly noticeable when reading directories recursively (e.g. 'git status').
However, performance of git commands using the is_path_excluded API (e.g.
'git ls-files --cached --ignored --exclude-standard') is greatly improved
as this no longer re-reads .gitignore files on each call.
Here's some performance data from the linux and WebKit repos (best of 10
runs on a Debian Linux on SSD, core.preloadIndex=true):
| ls-files -ci |status | status --ignored
| linux | WebKit | linux | WebKit | linux | WebKit
---+---++---++---+-
before | 0.506 | 6.539 | 0.212 | 1.555 | 0.323 | 2.541
after | 0.080 | 1.191 | 0.218 | 1.583 | 0.321 | 2.579
gain | 6.325 | 5.490 | 0.972 | 0.982 | 1.006 | 0.985
Signed-off-by: Karsten Blees
---
dir.c | 106 ++
dir.h | 6 ++--
2 files changed, 45 insertions(+), 67 deletions(-)
diff --git a/dir.c b/dir.c
index 417feaa..16fee2c 100644
--- a/dir.c
+++ b/dir.c
@@ -710,10 +710,6 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
struct exclude_stack *stk = NULL;
int current;
- if ((!dir->exclude_per_dir) ||
- (baselen + strlen(dir->exclude_per_dir) >= PATH_MAX))
- return; /* too long a path -- ignore */
-
group = &dir->exclude_list_group[EXC_DIRS];
/* Pop the exclude lists from the EXCL_DIRS exclude_list_group
@@ -725,12 +721,17 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
break;
el = &group->el[dir->exclude_stack->exclude_ix];
dir->exclude_stack = stk->prev;
+ dir->exclude = NULL;
free((char *)el->src); /* see strdup() below */
clear_exclude_list(el);
free(stk);
group->nr--;
}
+ /* Skip traversing into sub directories if the parent is excluded */
+ if (dir->exclude)
+ return;
+
/* Read from the parent directories and push them down. */
current = stk ? stk->baselen : -1;
while (current < baselen) {
@@ -749,22 +750,43 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
}
stk->prev = dir->exclude_stack;
stk->baselen = cp - base;
+ stk->exclude_ix = group->nr;
+ el = add_exclude_list(dir, EXC_DIRS, NULL);
memcpy(dir->basebuf + current, base + current,
stk->baselen - current);
- strcpy(dir->basebuf + stk->baselen, dir->exclude_per_dir);
- /*
-* dir->basebu
[PATCH 7/8] dir.c: replace is_path_excluded with now equivalent is_excluded API
Signed-off-by: Karsten Blees
---
builtin/add.c | 5 +---
builtin/check-ignore.c | 6 +---
builtin/ls-files.c | 15 +++---
dir.c | 79 --
dir.h | 16 ++
unpack-trees.c | 10 +--
unpack-trees.h | 1 -
7 files changed, 16 insertions(+), 116 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8..06f365d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -444,9 +444,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (pathspec) {
int i;
- struct path_exclude_check check;
- path_exclude_check_init(&check, &dir);
if (!seen)
seen = find_pathspecs_matching_against_index(pathspec);
for (i = 0; pathspec[i]; i++) {
@@ -454,7 +452,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
&& !file_exists(pathspec[i])) {
if (ignore_missing) {
int dtype = DT_UNKNOWN;
- if (is_path_excluded(&check,
pathspec[i], -1, &dtype))
+ if (is_excluded(&dir, pathspec[i],
&dtype))
dir_add_ignored(&dir,
pathspec[i], strlen(pathspec[i]));
} else
die(_("pathspec '%s' did not match any
files"),
@@ -462,7 +460,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
}
free(seen);
- path_exclude_check_clear(&check);
}
plug_bulk_checkin();
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 0240f99..7388346 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -59,7 +59,6 @@ static int check_ignore(const char *prefix, const char
**pathspec)
const char *path, *full_path;
char *seen;
int num_ignored = 0, dtype = DT_UNKNOWN, i;
- struct path_exclude_check check;
struct exclude *exclude;
/* read_cache() is only necessary so we can watch out for submodules. */
@@ -76,7 +75,6 @@ static int check_ignore(const char *prefix, const char
**pathspec)
return 0;
}
- path_exclude_check_init(&check, &dir);
/*
* look for pathspecs matching entries in the index, since these
* should not be ignored, in order to be consistent with
@@ -90,8 +88,7 @@ static int check_ignore(const char *prefix, const char
**pathspec)
full_path = check_path_for_gitlink(full_path);
die_if_path_beyond_symlink(full_path, prefix);
if (!seen[i]) {
- exclude = last_exclude_matching_path(&check, full_path,
--1, &dtype);
+ exclude = last_exclude_matching(&dir, full_path,
&dtype);
if (exclude) {
if (!quiet)
output_exclude(path, exclude);
@@ -101,7 +98,6 @@ static int check_ignore(const char *prefix, const char
**pathspec)
}
free(seen);
clear_directory(&dir);
- path_exclude_check_clear(&check);
return num_ignored;
}
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 175e6e3..2202072 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -201,19 +201,15 @@ static void show_ru_info(void)
}
}
-static int ce_excluded(struct path_exclude_check *check, struct cache_entry
*ce)
+static int ce_excluded(struct dir_struct *dir, struct cache_entry *ce)
{
int dtype = ce_to_dtype(ce);
- return is_path_excluded(check, ce->name, ce_namelen(ce), &dtype);
+ return is_excluded(dir, ce->name, &dtype);
}
static void show_files(struct dir_struct *dir)
{
int i;
- struct path_exclude_check check;
-
- if ((dir->flags & DIR_SHOW_IGNORED))
- path_exclude_check_init(&check, dir);
/* For cached/deleted files we don't need to even do the readdir */
if (show_others || show_killed) {
@@ -227,7 +223,7 @@ static void show_files(struct dir_struct *dir)
for (i = 0; i < active_nr; i++) {
struct cache_entry *ce = active_cache[i];
if ((dir->flags & DIR_SHOW_IGNORED) &&
- !ce_excluded(&check, ce))
+ !ce_excluded(dir, ce))
continue;
if (show_unmerged && !ce_stage(ce))
continue;
@@ -243,7 +239,7 @@ static void show_files(struct dir_struct *dir)
struct stat st;
int err;
[PATCH 8/8] dir.c: git-status: avoid is_excluded checks for tracked files
Checking if a file is in the index is much faster (hashtable lookup) than
checking if the file is excluded (linear search over exclude patterns).
Skip is_excluded checks for files: move the cache_name_exists check from
treat_file to treat_one_path and return early if the file is tracked.
This can safely be done as all other code paths also return path_ignored
for tracked files, and dir_add_ignored skips tracked files as well.
There's just one line left in treat_file, so move this to treat_one_path
as well.
Here's some performance data for git-status from the linux and WebKit
repos (best of 10 runs on a Debian Linux on SSD, core.preloadIndex=true):
|status | status --ignored
| linux | WebKit | linux | WebKit
---+---++---+-
before | 0.218 | 1.583 | 0.321 | 2.579
after | 0.156 | 0.988 | 0.202 | 1.279
gain | 1.397 | 1.602 | 1.589 | 2.016
Signed-off-by: Karsten Blees
---
dir.c | 38 +++---
1 file changed, 11 insertions(+), 27 deletions(-)
diff --git a/dir.c b/dir.c
index 086a169..c159000 100644
--- a/dir.c
+++ b/dir.c
@@ -1026,28 +1026,6 @@ static enum directory_treatment treat_directory(struct
dir_struct *dir,
}
/*
- * Decide what to do when we find a file while traversing the
- * filesystem. Mostly two cases:
- *
- * 1. We are looking for ignored files
- * (a) File is ignored, include it
- * (b) File is in ignored path, include it
- * (c) File is not ignored, exclude it
- *
- * 2. Other scenarios, include the file if not excluded
- *
- * Return 1 for exclude, 0 for include.
- */
-static int treat_file(struct dir_struct *dir, struct strbuf *path, int exclude)
-{
- /* Always exclude indexed files */
- if (index_name_exists(&the_index, path->buf, path->len, ignore_case))
- return 1;
-
- return exclude == !(dir->flags & DIR_SHOW_IGNORED);
-}
-
-/*
* This is an inexact early pruning of any recursive directory
* reading - if the path cannot possibly be in the pathspec,
* return true, and we'll skip it early.
@@ -1170,7 +1148,16 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
const struct path_simplify *simplify,
int dtype, struct dirent *de)
{
- int exclude = is_excluded(dir, path->buf, &dtype);
+ int exclude;
+ if (dtype == DT_UNKNOWN)
+ dtype = get_dtype(de, path->buf, path->len);
+
+ /* Always exclude indexed files */
+ if (dtype != DT_DIR &&
+ cache_name_exists(path->buf, path->len, ignore_case))
+ return path_ignored;
+
+ exclude = is_excluded(dir, path->buf, &dtype);
if (exclude && (dir->flags & DIR_COLLECT_IGNORED)
&& exclude_matches_pathspec(path->buf, path->len, simplify))
dir_add_ignored(dir, path->buf, path->len);
@@ -1182,9 +1169,6 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
if (exclude && !(dir->flags & DIR_SHOW_IGNORED))
return path_ignored;
- if (dtype == DT_UNKNOWN)
- dtype = get_dtype(de, path->buf, path->len);
-
switch (dtype) {
default:
return path_ignored;
@@ -1201,7 +1185,7 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
break;
case DT_REG:
case DT_LNK:
- if (treat_file(dir, path, exclude))
+ if (exclude == !(dir->flags & DIR_SHOW_IGNORED))
return path_ignored;
break;
}
--
1.8.1.2.8021.g7e51819
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/8] dir.c: git-status --ignored: don't list empty directories as ignored
'git-status --ignored' lists empty untracked directories as ignored, even
though they don't have any ignored files.
When checking if a directory is already listed as untracked (i.e. shouldn't
be listed as ignored as well), don't assume that the dirctory has only
ignored files if it doesn't have untracked files, as the directory may be
empty.
Signed-off-by: Karsten Blees
---
dir.c | 17 -
t/t7061-wtstatus-ignore.sh | 26 --
2 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/dir.c b/dir.c
index fd1f088..64457db 100644
--- a/dir.c
+++ b/dir.c
@@ -1071,17 +1071,16 @@ static enum directory_treatment treat_directory(struct
dir_struct *dir,
/*
* We are looking for ignored files and our directory is not ignored,
-* check if it contains only ignored files
+* check if it contains untracked files (i.e. is listed as untracked)
*/
if ((dir->flags & DIR_SHOW_IGNORED) && !exclude) {
- int ignored;
- dir->flags &= ~DIR_SHOW_IGNORED;
- dir->flags |= DIR_HIDE_EMPTY_DIRECTORIES;
- ignored = read_directory_recursive(dir, dirname, len, 1,
simplify);
- dir->flags &= ~DIR_HIDE_EMPTY_DIRECTORIES;
- dir->flags |= DIR_SHOW_IGNORED;
-
- return ignored ? ignore_directory : show_directory;
+ int untracked;
+ dir->flags &= ~(DIR_SHOW_IGNORED|DIR_SHOW_OTHER_DIRECTORIES);
+ untracked = read_directory_recursive(dir, dirname, len, 1,
simplify);
+ dir->flags |= DIR_SHOW_IGNORED|DIR_SHOW_OTHER_DIRECTORIES;
+
+ if (untracked)
+ return ignore_directory;
}
if (!(dir->flags & DIR_SHOW_IGNORED) &&
!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 28b7d95..6171a49 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -64,13 +64,35 @@ cat >expected <<\EOF
?? .gitignore
?? actual
?? expected
-!! untracked-ignored/
EOF
-test_expect_success 'status untracked directory with ignored files with
--ignore' '
+test_expect_success 'status empty untracked directory with --ignore' '
rm -rf ignored &&
mkdir untracked-ignored &&
mkdir untracked-ignored/test &&
+ git status --porcelain --ignored >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
+EOF
+
+test_expect_success 'status empty untracked directory with --ignore -u' '
+ git status --porcelain --ignored -u >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<\EOF
+?? .gitignore
+?? actual
+?? expected
+!! untracked-ignored/
+EOF
+
+test_expect_success 'status untracked directory with ignored files with
--ignore' '
: >untracked-ignored/ignored &&
: >untracked-ignored/test/ignored &&
git status --porcelain --ignored >actual &&
--
1.8.1.2.8021.g7e51819
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] submodule: add 'deinit' command
Am 12.03.2013 17:22, schrieb Junio C Hamano: > Phil Hord writes: > >> I think this would be clearer if 'git deinit' said >> >> rm 'submodule/*' >> >> or maybe >> >> Removed workdir for 'submodule' >> >> Is it just me? > > The latter may probably be better. Hmm, it doesn't really remove the directory but only empties it (it recreates it a few lines after removing it together with its contents). So what about Cleared directory 'submodule' The attached interdiff suppresses the "rm 'submodule'" message and issues the "Cleared ..." message after it successfully removed the work tree. (But please note that it also prints this message even if the submodule work tree is already empty, e.g. when you deinit a submodule the second time) 8<- diff --git a/git-submodule.sh b/git-submodule.sh index 204bc78..d003e8a 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -601,10 +601,12 @@ cmd_deinit() if test -z "$force" then - git rm -n "$sm_path" || + git rm -qn "$sm_path" || die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")" fi - rm -rf "$sm_path" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")" + rm -rf "$sm_path" && + say "$(eval_gettext "Cleared directory '\$sm_path'")" || + say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")" fi mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")" -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Sun, 17 Mar 2013 16:35:59 -0700
Junio C Hamano wrote:
> Rob Hoelz writes:
>
> > git push currently doesn't consider pushInsteadOf when
> > using pushurl; this tests and fixes that.
> >
> > If you use pushurl with an alias that has a pushInsteadOf
> > configuration value, Git does not take advantage of it. For
> > example:
> >
> > [url "git://github.com/"]
> > insteadOf = github:
> > [url "git://github.com/myuser/"]
> > insteadOf = mygithub:
> > [url "g...@github.com:myuser/"]
> > pushInsteadOf = mygithub:
> > [remote "origin"]
> > url = github:organization/project
> > pushurl = mygithub:project
>
> Incomplete sentence? For example [this is an example configuration]
> and then what happens? Something like "with the sample
> configuration, 'git push origin' should follow pushurl and then turn
> it into X, but instead it ends up accessing Y".
>
> If there is no pushInsteadOf, does it still follow insteadOf? Is it
> tested already?
>
> Wouldn't you want to cover all the combinations to negative cases
> (i.e. making sure the codepath to support a new case does not affect
> behaviour of the code outside the new case)? A remote with and
> without pushurl (two combinations) and a pseudo URL scheme with and
> without pushInsteadOf (again, two combinations) will give you four
> cases.
>
>
> Thanks.
I've taken your advice, and an amended patch follows.
>
> >
> > Signed-off-by: Rob Hoelz
> > ---
> > remote.c | 2 +-
> > t/t5516-fetch-push.sh | 20
> > 2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/remote.c b/remote.c
> > index ca1f8f2..de7a915 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -465,7 +465,7 @@ static void alias_all_urls(void)
> > if (!remotes[i])
> > continue;
> > for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> > - remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites);
> > + remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites_push); }
> > add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
> > for (j = 0; j < remotes[i]->url_nr; j++) {
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index b5417cc..272e225 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf
> > and explicit pushurl (pushInsteadOf )
> > '
> >
> > +test_expect_success 'push with pushInsteadOf and explicit pushurl
> > (pushInsteadOf does rewrite in this case)' '
> > + mk_empty &&
> > + TRASH="$(pwd)/" &&
> > + mkdir ro &&
> > + mkdir rw &&
> > + git init --bare rw/testrepo &&
> > + git config "url.file://$TRASH/ro/.insteadOf" ro: &&
> > + git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> > + git config remote.r.url ro:wrong &&
> > + git config remote.r.pushurl rw:testrepo &&
> > + git push r refs/heads/master:refs/remotes/origin/master &&
> > + (
> > + cd rw/testrepo &&
> > + r=$(git show-ref -s --verify
> > refs/remotes/origin/master) &&
> > + test "z$r" = "z$the_commit" &&
> > +
> > + test 1 = $(git for-each-ref refs/remotes/origin |
> > wc -l)
> > + )
> > +'
> > +
> > test_expect_success 'push with matching heads' '
> >
> > mk_test heads/master &&
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] push: Alias pushurl from push rewrites
git push currently doesn't consider pushInsteadOf when
using pushurl; this test tests that.
If you use pushurl with an alias that has a pushInsteadOf configuration
value, Git does not take advantage of it. For example:
[url "git://github.com/"]
insteadOf = github:
[url "git://github.com/myuser/"]
insteadOf = mygithub:
[url "g...@github.com:myuser/"]
pushInsteadOf = mygithub:
[remote "origin"]
url = github:organization/project
pushurl = mygithub:project
With the above configuration, the following occurs:
$ git push origin master
fatal: remote error:
You can't push to git://github.com/myuser/project.git
Use g...@github.com:myuser/project.git
So you can see that pushurl is being followed (it's not attempting to
push to git://github.com/organization/project), but insteadOf values are
being used as opposed to pushInsteadOf values for expanding the pushurl
alias.
This commit fixes that.
Signed-off-by: Rob Hoelz
---
remote.c | 2 +-
t/t5516-fetch-push.sh | 81 +++
2 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/remote.c b/remote.c
index ca1f8f2..de7a915 100644
--- a/remote.c
+++ b/remote.c
@@ -465,7 +465,7 @@ static void alias_all_urls(void)
if (!remotes[i])
continue;
for (j = 0; j < remotes[i]->pushurl_nr; j++) {
- remotes[i]->pushurl[j] =
alias_url(remotes[i]->pushurl[j], &rewrites);
+ remotes[i]->pushurl[j] =
alias_url(remotes[i]->pushurl[j], &rewrites_push);
}
add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
for (j = 0; j < remotes[i]->url_nr; j++) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..709f506 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -244,6 +244,87 @@ test_expect_success 'push with pushInsteadOf and explicit
pushurl (pushInsteadOf
)
'
+test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl +
pushInsteadOf does rewrite in this case)' '
+ mk_empty &&
+ rm -rf ro rw &&
+ TRASH="$(pwd)/" &&
+ mkdir ro &&
+ mkdir rw &&
+ git init --bare rw/testrepo &&
+ git config "url.file://$TRASH/ro/.insteadOf" ro: &&
+ git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+ git config remote.r.url ro:wrong &&
+ git config remote.r.pushurl rw:testrepo &&
+ git push r refs/heads/master:refs/remotes/origin/master &&
+ (
+ cd rw/testrepo &&
+ r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+ test "z$r" = "z$the_commit" &&
+
+ test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ )
+'
+
+test_expect_success 'push without pushInsteadOf and explicit pushurl (pushurl
+ insteadOf is used for rewrite)' '
+ mk_empty &&
+ rm -rf ro rw &&
+ TRASH="$(pwd)/" &&
+ mkdir ro &&
+ mkdir rw &&
+ git init --bare rw/testrepo &&
+ git config "url.file://$TRASH/ro/.insteadOf" ro: &&
+ git config "url.file://$TRASH/rw/.insteadOf" rw: &&
+ git config remote.r.url ro:wrong &&
+ git config remote.r.pushurl rw:testrepo &&
+ git push r refs/heads/master:refs/remotes/origin/master &&
+ (
+ cd rw/testrepo &&
+ r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+ test "z$r" = "z$the_commit" &&
+
+ test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ )
+'
+
+test_expect_success 'push with pushInsteadOf but without explicit pushurl (url
+ pushInsteadOf is used for rewrite)' '
+ mk_empty &&
+ rm -rf ro rw &&
+ TRASH="$(pwd)/" &&
+ mkdir ro &&
+ mkdir rw &&
+ git init --bare rw/testrepo &&
+ git config "url.file://$TRASH/ro/.insteadOf" rw: &&
+ git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+ git config remote.r.url rw:testrepo &&
+ git push r refs/heads/master:refs/remotes/origin/master &&
+ (
+ cd rw/testrepo &&
+ r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+ test "z$r" = "z$the_commit" &&
+
+ test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ )
+'
+
+test_expect_success 'push without pushInsteadOf and without explicit pushurl
(url + insteadOf is used for rewrite)' '
+ mk_empty &&
+ rm -rf ro rw &&
+ TRASH="$(pwd)/" &&
+ mkdir ro &&
+ mkdir rw &&
+ git init --bare rw/testrepo &&
+ git config "url.file://$TRASH/ro/.insteadOf" rw: &&
+ git config remote.r.url rw:testrepo &&
+ git push r refs/heads/master:refs/remotes/origin/master &&
+ (
+ cd rw/testrepo &&
+ r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+ test "z$r" = "z$the_commit" &&
Re: [PATCH/RFC] Changing submodule foreach --recursive to be depth-first, --parent option to execute command in supermodule as well
Am 12.03.2013 17:01, schrieb Phil Hord:
> On Sat, Mar 9, 2013 at 1:18 PM, Jens Lehmann wrote:
>> Am 05.03.2013 22:17, schrieb Phil Hord:
>>> On Tue, Mar 5, 2013 at 3:51 PM, Jens Lehmann wrote:
Am 05.03.2013 19:34, schrieb Junio C Hamano:
> Eric Cousineau writes:
>> ...
> I am not entirely convinced we would want --include-super in the
> first place, though. It does not belong to "submodule foreach";
> it is doing something _outside_ the submoudules.
I totally agree with that. First, adding --include-super does not
belong into the --post-order patch at all, as that is a different
topic (even though it belongs to the same use case Eric has). Also
the reason why we are thinking about adding the --post-order option
IMO cuts the other way for --include-super: It is so easy to do
that yourself I'm not convinced we should add an extra option to
foreach for that, especially as it has nothing to do with submodules.
So I think we should just drop --include-super.
>>>
>>> I agree it should not be part of this commit, but I've often found
>>> myself in need of an --include-super switch. To me,
>>> git-submodule-foreach means "visit all my .git repos in this project
>>> and execute $cmd". It's a pity that the super-project is considered a
>>> second-class citizen in this regard.
>>
>> Hmm, for me the super-project is a very natural second-class citizen
>> to "git *submodule* foreach". But also I understand that sometimes the
>> user wants to apply a command to superproject and submodules alike (I
>> just recently did exactly that with "git gc" on our build server).
>>
>>> I have to do this sometimes:
>>>
>>>${cmd} && git submodule foreach --recursive '${cmd}'
>>>
>>> I often forget the first part in scripts, though, and I've seen others
>>> do it too. I usually create a function for it in git-heavy scripts.
>>>
>>> In a shell, it usually goes like this:
>>>
>>>git submodule foreach --recursive '${cmd}'
>>>{30-ish}
>>>
>>> It'd be easier if I could just include a switch for this, and maybe
>>> even create an alias for it. But maybe this is different command
>>> altogether.
>>
>> Are you sure you wouldn't forget to provide such a switch too? ;-)
>
> No. However, when I remember to add the switch, my shell history will
> remember it for me. This does not happen naturally for me in the
> "{30-ish}..." workflow.
I started to use '&&' in my daily shell work for exactly that reason:
that the bash history remembers groups of two or more commands for me.
> I also hope this switch grows up into a configuration option someday.
> Or maybe a completely different command, like I said before; because I
> actually think it could be dangerous as a configuration option since
> it would have drastic consequences for users executing scripts or
> commands in other users' environments.
I agree on the possible problems a configuration option introduces.
>> I'm still not convinced we should add a new switch, as it can easily
>> be achieved by adding "${cmd} &&" to your scripts. And on the command
>> line you could use an alias like this one to achieve that:
>>
>> [alias]
>> recurse = !sh -c \"$@ && git submodule foreach --recursive $@\"
>
> Yes, making the feature itself a 2nd-class citizen. :-)
>
> But this alias also denies me the benefit of the --post-order option.
> For 'git recurse git push', for example, I wouldn't want the
> superproject push to occur first; I would want it to occur last after
> the submodules have been successfully pushed.
[alias]
recurse-post = !sh -c \"git submodule foreach --recursive --post-order
$@ && $@\"
;-)
> I agree this should go in some other commit, but I do not think it is
> so trivial it should never be considered as a feature for git. That's
> all I'm trying to say.
I am not against adding such a functionality to Git, I'm just not
convinced "git submodule foreach" is the right command for that. I
suspect the "git for-each-repo" Lars proposed earlier this year might
be a better choice, as that could also recurse into other repos which
aren't registered as submodules. And a "for-each-repo" to me looks
like a command which could include the superproject too (at least when
told to do so with an option).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Mar 2013, #05; Mon, 18)
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.
We have been scheduling each cycle to last 8-10 weeks, but at the
end of 1.8.2 cycle we already have quite a many topics in flight. I
am wondering we should cut this cycle a bit short by tagging -rc0
soon after we have those we already have been cooking graduated to
'master' (we already have 200 non-merge commits on 'next').
This issue of "What's cooking" classifies the topics in flight into
three categories: Trivially Safe, Safe and Risky. "Risky" does not
mean the topic is with known breakages, but just that my gut feeling
tells it would not be surprising if there were unintended
consequences, either these topics touch fairly deep part of the
codepath, their design may not match some obscure but still valid
use cases, or for some other unforeseen reason. Feeding this
message to "cook -w" (found in 'todo' branch) script may make the
overall picture easier to review.
The tip of 'next' hasn't been rewound yet, but it soon will.
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--
[New Topics]
* ap/combine-diff-ignore-whitespace (2013-03-14) 1 commit
(merged to 'next' on 2013-03-18 at 32435a6)
+ Allow combined diff to ignore white-spaces
Teach "diff --cc" output to honor options to ignore various forms
of whitespace changes.
Will merge to 'master' in the 4th batch (Safe).
* jk/checkout-attribute-lookup (2013-03-14) 2 commits
(merged to 'next' on 2013-03-15 at 28bd515)
+ entry: fix filter lookup
+ t2003: modernize style
Codepath to stream blob object contents directly from the object
store to filesystem did not use the correct path to find conversion
filters when writing to temporary files.
Will merge to 'master' in the 4th batch (Safe).
* jk/difftool-dir-diff-edit-fix (2013-03-14) 3 commits
(merged to 'next' on 2013-03-15 at add7b77)
+ difftool --dir-diff: symlink all files matching the working tree
+ difftool: avoid double slashes in symlink targets
+ git-difftool(1): fix formatting of --symlink description
"git difftool --dir-diff" made symlinks to working tree files when
preparing a temporary directory structure, so that accidental edits
of these files in the difftool are reflected back to the working
tree, but the logic to decide when to do so was not quite right.
Will merge to 'master' in the 4th batch (Safe).
* lf/setup-prefix-pathspec (2013-03-14) 2 commits
(merged to 'next' on 2013-03-14 at 7552204)
+ setup.c: check that the pathspec magic ends with ")"
+ setup.c: stop prefix_pathspec() from looping past the end of string
Rerolls aw/setup-prefix-pathspec.
Will merge to 'master' in the 3rd batch (Safe).
* tb/document-status-u-tradeoff (2013-03-16) 2 commits
(merged to 'next' on 2013-03-18 at 2fc53b0)
+ status: advise to consider use of -u when read_directory takes too long
+ git status: document trade-offs in choosing parameters to the -u option
Suggest users to look into using--untracked=no option when "git
status" takes too long.
Will merge to 'master' in the 2nd batch (Trivially Safe).
* jk/fully-peeled-packed-ref (2013-03-18) 4 commits
(merged to 'next' on 2013-03-18 at cbcfa32)
+ pack-refs: add fully-peeled trait
+ pack-refs: write peeled entry for non-tags
+ use parse_object_or_die instead of die("bad object")
+ avoid segfaults on parse_object failure
Not that we do not actively encourage having annotated tags outside
refs/tags/ hierarchy, but they were not advertised correctly to the
ls-remote and fetch with recent version of Git.
Will merge to 'master' in the 3rd batch (Safe).
* jk/peel-ref (2013-03-16) 3 commits
(merged to 'next' on 2013-03-18 at 77f0e4e)
+ upload-pack: load non-tip "want" objects from disk
+ upload-pack: make sure "want" objects are parsed
+ upload-pack: drop lookup-before-parse optimization
Recent optimization broke shallow clones.
Will merge to 'master' in the 3rd batch (Safe).
* nd/index-pack-l10n-buf-overflow (2013-03-16) 1 commit
(merged to 'next' on 2013-03-18 at b7a4a8e)
+ index-pack: fix buffer overflow caused by translations
Will merge to 'master' in the 2nd batch (Trivially Safe).
* nd/magic-pathspecs (2013-03-15) 45 commits
- Rename field "raw" to "_raw" in struct pathspec
- pathspec: support :(glob) syntax
- pathspec: make --literal-pathspecs disable pathspec magic
- pathspec: support :(literal) syntax for noglob pathspec
- Kill limit_pathspec_to_literal() as it's only used by parse_pathspec()
- parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN
- parse_pathspec: make sure the prefix part is wildcard-free
- tree-diff: remove the use of pathspec's raw[] in follow-rename codepath
- Remove match_pathspec() in favor of match_pathspec_dep
FW: Windows. Git, and Dedupe
Windows probably isn’t the most popular platform for Git developers ☺, but here
goes…
On Windows with an NTFS volume with Deduplication enabled, Git believes that
deduplicated files are symlinks. It then fails to be able to do anything with
the file. This can be repro-ed by creating an NTFS volume with dedup, creating
some duplicate files, verifying that a few files are deduped, and trying to add
and commit the files via git.
Jmr
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
Re: [PATCH/RFC] Changing submodule foreach --recursive to be depth-first, --parent option to execute command in supermodule as well
Thanks, just a quick review before I find some time do take a
deeper look.
Am 14.03.2013 07:30, schrieb Eric Cousineau:
> From 59fb432e17a1aae9de26bbaaca7f09cc7f03b471 Mon Sep 17 00:00:00 2001
> From: Eric Cousineau
> Date: Thu, 14 Mar 2013 01:19:53 -0500
> Subject: [PATCH] submodule-foreach: Added in --post-order= per Jens
> Lehmann's suggestion
>
> Signed-off-by: Eric Cousineau
> ---
> Made the scope of the patch only relate to --post-order.
> Would we want to rename this to just --post= ?
Hmm, while having no strong preference on that, "post order"
looks more like the correct term describing what we do here.
> Anywho, here it is running in a test setup, where the structure is:
> a
> - b
> - - d
> - c
>
> $ git submodule foreach --recursive --post-order 'echo Post $name' 'echo Pre
> $path'
> Entering 'b'
> Pre b
> Entering 'b/d'
> Pre d
> Entering 'b/d'
> Post d
> Entering 'b'
> Post b
> Entering 'c'
> Pre c
> Entering 'c'
> Post c
Looking good.
> An interesting note is that it fails with 'git submodule foreach
> --post-order', but not 'git submodule foreach --post-order=', since it simply
> interprets that as an empty command.
> If that is important, I could add in a check for $# when parsing the argument
> for --post-order=*.
>
> git-submodule.sh | 39 ++-
> 1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 004c034..9b70bc2 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -10,7 +10,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name
> ] [--reference or: $dashless [--quiet] init [--] [...]
> or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
> [-f|--force] [--rebase] [--reference ] [--merge] [--recursive]
> [--] [...]
> or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ]
> [commit] [--] [...]
> - or: $dashless [--quiet] foreach [--recursive]
> + or: $dashless [--quiet] foreach [--recursive] [--post-order=]
>
Maybe drop the '=' from the description (see --reference for an example)?
> or: $dashless [--quiet] sync [--recursive] [--] [...]"
> OPTIONS_SPEC=
> . git-sh-setup
> @@ -434,6 +434,8 @@ Use -f if you really want to add it." >&2
> cmd_foreach()
> {
> # parse $args after "submodule ... foreach".
> +# Gratuitous (empty) local's to prevent recursive bleeding
> +local recursive= post_order=
Wouldn't it be sufficient to add "post_order=" to the top of the
file where "recursive" is already initialized? Or am I missing
something here?
> while test $# -ne 0
> do
> case "$1" in
> @@ -443,6 +445,15 @@ cmd_foreach()
> --recursive)
> recursive=1
> ;;
> +--post-order)
> +test "$#" = "1" && usage
> +post_order="$2"
> +shift
> +;;
> +--post-order=*)
> +# Will skip empty commands
> +post_order=${1#*=}
> +;;
> -*)
> usage
> ;;
> @@ -453,7 +464,7 @@ cmd_foreach()
> shift
> done
>
> -toplevel=$(pwd)
> +local toplevel=$(pwd)
Why do you have to add the "local" keyword here?
> # dup stdin so that it can be restored when running the external
> # command in the subshell (and a recursive call to this function)
> @@ -465,18 +476,36 @@ cmd_foreach()
> die_if_unmatched "$mode"
> if test -e "$sm_path"/.git
> then
> -say "$(eval_gettext "Entering '\$prefix\$sm_path'")"
> +local name prefix path message epitaph
Same here?
> +message="$(eval_gettext "Entering '\$prefix\$sm_path'")"
> +epitaph="$(eval_gettext "Stopping at '\$sm_path'; script
> returned non-zero status.")"
> name=$(module_name "$sm_path")
> (
> prefix="$prefix$sm_path/"
> clear_local_git_env
> # we make $path available to scripts ...
> path=$sm_path
> +
> +sm_eval() {
> +say "$message"
> +eval "$@" || die "$epitaph"
> +}
> +
> cd "$sm_path" &&
> -eval "$@" &&
> +sm_eval "$@" &&
> if test -n "$recursive"
> then
> -cmd_foreach "--recursive" "$@"
> +if test -n "$post_order"
> +then
> +# Tried keeping flags as a variable, but was having
> difficulty
Maybe because you set the "post_order" variable to empty at the
beginning of this function? If I read that right moving that
initialization to the top of the file could get rid of the if
here?
> +cmd_foreach --recursive --post-order "$post_order"
> "$@"
> +else
> +cmd_foreach --recursive "$@"
> +fi
> +
Re: [PATCH 4/8] dir.c: git-status --ignored: don't list empty directories as ignored
On Mon, Mar 18, 2013 at 4:28 PM, Karsten Blees wrote: > When checking if a directory is already listed as untracked (i.e. shouldn't > be listed as ignored as well), don't assume that the dirctory has only s/dirctory/directory/ > ignored files if it doesn't have untracked files, as the directory may be > empty. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/8] dir.c: unify is_excluded and is_path_excluded APIs
On Mon, Mar 18, 2013 at 4:29 PM, Karsten Blees wrote: > Directories could also be excluded by exclude patterns specified on the > command line or .git/info/exclude, so we cannot simply skip prep_exclude > entirely if there's no .gitignore file name (dir_struct.exclude_per_dir). > Move this check to just before acutally reading the file. s/acutally/actually/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] remote.c: simply a bit of code using git_config_string()
On Mon, Mar 18, 2013 at 9:16 AM, Ramkumar Ramachandra wrote: > remote.c: simply a bit of code using git_config_string() s/simply/simplify/ > A small segment where handle_config() parses the branch.remote > configuration variable can be simplified using git_config_string(). > > Signed-off-by: Ramkumar Ramachandra -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] remote.c: introduce a way to have different remotes for fetch/ push
On Mon, Mar 18, 2013 at 9:16 AM, Ramkumar Ramachandra wrote: > remote.c: introduce a way to have different remotes for fetch/ push s/ push/push/ > Currently, do_push() in push.c calls remote_get(), which gets the > configured remote for fetching and pushing. Replace this call with a > call to pushremote_get() instead, a new function that will return the > remote configured specifically for pushing. This function tries to > work with the string pushremote_name, before falling back to the > codepath of remote_get(). This patch has no visible impact, but > serves to enable future patches to introduce configuration variables > to set this variable. > > Signed-off-by: Ramkumar Ramachandra -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] remote.c: introduce remote.pushdefault
On Mon, Mar 18, 2013 at 9:16 AM, Ramkumar Ramachandra wrote: > branch..remote:: > - When in branch , it tells 'git fetch' and 'git push' which > - remote to fetch from/push to. It defaults to `origin` if no remote is > - configured. `origin` is also used if you are not on any branch. > + When on branch , it tells 'git fetch' and 'git push' > + which remote to fetch from/push to. The remote to push to > + may be overriden with `remote.pushdefault` (for all branches). s/overriden/overridden/ > + If no remote is configured, or if you are not on any branch, > + it defaults to `origin` for fetching and `remote.pushdefault` > + for pushing. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] remote.c: introduce branch..pushremote
On Mon, Mar 18, 2013 at 9:16 AM, Ramkumar Ramachandra wrote: > - If no remote is configured, or if you are not on any branch, > - it defaults to `origin` for fetching and `remote.pushdefault` > - for pushing. > + The remote to push to, for the current branch, may be further > + overriden by `branch..pushremote`. If no remote is s/overriden/overridden/ > + configured, or if you are not on any branch, it defaults to > + `origin` for fetching and `remote.pushdefault` for pushing. > + > remote.pushdefault:: > The remote to push to by default. Overrides > - `branch..remote` for all branches. > + `branch..remote` for all branches, and is overriden by Ditto: s/overriden/overridden/ > + `branch..pushremote` for specific branches. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] push test: use test_config when appropriate
Configuration from test_config does not last beyond the end of the current test assertion, making each test easier to think about in isolation. This changes the meaning of some of the tests. For example, currently "push with insteadOf" passes even if the line setting "url.$TRASH.pushInsteadOf" is dropped because an url.$TRASH.insteadOf setting leaks in from a previous test. Signed-off-by: Jonathan Nieder --- t/t5516-fetch-push.sh | 27 ++- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index c31e5c1c..5b89c111 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -203,7 +203,7 @@ test_expect_success 'push with wildcard' ' test_expect_success 'push with insteadOf' ' mk_empty && TRASH="$(pwd)/" && - git config "url.$TRASH.insteadOf" trash/ && + test_config "url.$TRASH.insteadOf" trash/ && git push trash/testrepo refs/heads/master:refs/remotes/origin/master && ( cd testrepo && @@ -217,7 +217,7 @@ test_expect_success 'push with insteadOf' ' test_expect_success 'push with pushInsteadOf' ' mk_empty && TRASH="$(pwd)/" && - git config "url.$TRASH.pushInsteadOf" trash/ && + test_config "url.$TRASH.pushInsteadOf" trash/ && git push trash/testrepo refs/heads/master:refs/remotes/origin/master && ( cd testrepo && @@ -231,9 +231,9 @@ test_expect_success 'push with pushInsteadOf' ' test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty && TRASH="$(pwd)/" && - git config "url.trash2/.pushInsteadOf" trash/ && - git config remote.r.url trash/wrong && - git config remote.r.pushurl "$TRASH/testrepo" && + test_config "url.trash2/.pushInsteadOf" trash/ && + test_config remote.r.url trash/wrong && + test_config remote.r.pushurl "$TRASH/testrepo" && git push r refs/heads/master:refs/remotes/origin/master && ( cd testrepo && @@ -489,31 +489,24 @@ test_expect_success 'push with config remote.*.push = HEAD' ' git checkout local && git reset --hard $the_first_commit ) && - git config remote.there.url testrepo && - git config remote.there.push HEAD && - git config branch.master.remote there && + test_config remote.there.url testrepo && + test_config remote.there.push HEAD && + test_config branch.master.remote there && git push && check_push_result $the_commit heads/master && check_push_result $the_first_commit heads/local ' -# clean up the cruft left with the previous one -git config --remove-section remote.there -git config --remove-section branch.master - test_expect_success 'push with config remote.*.pushurl' ' mk_test heads/master && git checkout master && - git config remote.there.url test2repo && - git config remote.there.pushurl testrepo && + test_config remote.there.url test2repo && + test_config remote.there.pushurl testrepo && git push there && check_push_result $the_commit heads/master ' -# clean up the cruft left with the previous one -git config --remove-section remote.there - test_expect_success 'push with dry-run' ' mk_test heads/master && -- 1.8.2.rc3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] push test: simplify check of push result
This test checks each ref with code like the following:
r=$(git show-ref -s --verify refs/$ref) &&
test "z$r" = "z$the_first_commit"
Afterward it counts refs:
test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
Simpler to test the number and values of relevant refs in for-each-ref
output at the same time using test_cmp. This makes the test more
readable and provides more helpful "./t5516-push-push.sh -v" output
when the test fails.
Signed-off-by: Jonathan Nieder
---
t/t5516-fetch-push.sh | 114 ++
1 file changed, 51 insertions(+), 63 deletions(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 5b89c111..2f1255d4 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -30,11 +30,10 @@ mk_test () {
cd testrepo &&
for ref in "$@"
do
- r=$(git show-ref -s --verify refs/$ref) &&
- test "z$r" = "z$the_first_commit" || {
- echo "Oops, refs/$ref is wrong"
- exit 1
- }
+ echo "$the_first_commit" >expect &&
+ git show-ref -s --verify refs/$ref >actual &&
+ test_cmp expect actual ||
+ exit
done &&
git fsck --full
)
@@ -82,15 +81,13 @@ mk_child() {
check_push_result () {
(
cd testrepo &&
- it="$1" &&
- shift
+ echo "$1" >expect &&
+ shift &&
for ref in "$@"
do
- r=$(git show-ref -s --verify refs/$ref) &&
- test "z$r" = "z$it" || {
- echo "Oops, refs/$ref is wrong"
- exit 1
- }
+ git show-ref -s --verify refs/$ref >actual &&
+ test_cmp expect actual ||
+ exit
done &&
git fsck --full
)
@@ -118,10 +115,9 @@ test_expect_success 'fetch without wildcard' '
cd testrepo &&
git fetch .. refs/heads/master:refs/remotes/origin/master &&
- r=$(git show-ref -s --verify refs/remotes/origin/master) &&
- test "z$r" = "z$the_commit" &&
-
- test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ echo "$the_commit commitrefs/remotes/origin/master"
>expect &&
+ git for-each-ref refs/remotes/origin >actual &&
+ test_cmp expect actual
)
'
@@ -133,10 +129,9 @@ test_expect_success 'fetch with wildcard' '
git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*"
&&
git fetch up &&
- r=$(git show-ref -s --verify refs/remotes/origin/master) &&
- test "z$r" = "z$the_commit" &&
-
- test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ echo "$the_commit commitrefs/remotes/origin/master"
>expect &&
+ git for-each-ref refs/remotes/origin >actual &&
+ test_cmp expect actual
)
'
@@ -150,10 +145,9 @@ test_expect_success 'fetch with insteadOf' '
git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*"
&&
git fetch up &&
- r=$(git show-ref -s --verify refs/remotes/origin/master) &&
- test "z$r" = "z$the_commit" &&
-
- test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ echo "$the_commit commitrefs/remotes/origin/master"
>expect &&
+ git for-each-ref refs/remotes/origin >actual &&
+ test_cmp expect actual
)
'
@@ -167,10 +161,9 @@ test_expect_success 'fetch with pushInsteadOf (should not
rewrite)' '
git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*"
&&
git fetch up &&
- r=$(git show-ref -s --verify refs/remotes/origin/master) &&
- test "z$r" = "z$the_commit" &&
-
- test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ echo "$the_commit commitrefs/remotes/origin/master"
>expect &&
+ git for-each-ref refs/remotes/origin >actual &&
+ test_cmp expect actual
)
'
@@ -180,10 +173,9 @@ test_expect_success 'push without wildcard' '
git push testrepo refs/heads/master:refs/remotes/origin/master &&
(
cd testrepo &&
- r=$(git show-ref -s --verify refs/remotes/origin/master) &&
- test "z$r" = "z$the_commit" &&
-
- test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ echo "$the_commit commitrefs/remotes/origin/master"
>expect &&
+
[PATCH 3/3] push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi'
When it is unclear which command from a test has failed, usual
practice these days is to debug by running the test again with "sh -x"
instead of relying on debugging 'echo' statements.
Signed-off-by: Jonathan Nieder
---
t/t5516-fetch-push.sh | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2f1255d4..0695d570 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -22,10 +22,8 @@ mk_test () {
(
for ref in "$@"
do
- git push testrepo $the_first_commit:refs/$ref || {
- echo "Oops, push refs/$ref failure"
- exit 1
- }
+ git push testrepo $the_first_commit:refs/$ref ||
+ exit
done &&
cd testrepo &&
for ref in "$@"
@@ -328,13 +326,8 @@ test_expect_success 'push with weak ambiguity (2)' '
test_expect_success 'push with ambiguity' '
mk_test heads/frotz tags/frotz &&
- if git push testrepo master:frotz
- then
- echo "Oops, should have failed"
- false
- else
- check_push_result $the_first_commit heads/frotz tags/frotz
- fi
+ test_must_fail git push testrepo master:frotz &&
+ check_push_result $the_first_commit heads/frotz tags/frotz
'
--
1.8.2.rc3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Hi, Rob Hoelz wrote: > [url "git://github.com/"] > insteadOf = github: > [url "git://github.com/myuser/"] > insteadOf = mygithub: > [url "g...@github.com:myuser/"] > pushInsteadOf = mygithub: > [remote "origin"] > url = github:organization/project > pushurl = mygithub:project > > With the above configuration, the following occurs: > > $ git push origin master > fatal: remote error: > You can't push to git://github.com/myuser/project.git > Use g...@github.com:myuser/project.git > > So you can see that pushurl is being followed (it's not attempting to > push to git://github.com/organization/project), but insteadOf values are > being used as opposed to pushInsteadOf values for expanding the pushurl > alias. At first glance it is not always obvious how overlapping settings like these should interact. Thanks for an instructive example that makes the right behavior obvious. Test nits: [...] > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -244,6 +244,87 @@ test_expect_success 'push with pushInsteadOf and > explicit pushurl (pushInsteadOf > ) > ' > > +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + > pushInsteadOf does rewrite in this case)' ' > + mk_empty && > + rm -rf ro rw && > + TRASH="$(pwd)/" && > + mkdir ro && > + mkdir rw && > + git init --bare rw/testrepo && > + git config "url.file://$TRASH/ro/.insteadOf" ro: && > + git config "url.file://$TRASH/rw/.pushInsteadOf" rw: && The surrounding tests don't do this, but I wonder if it would make sense to use test_config instead of 'git config' here. That way, the test's settings wouldn't affect other tests, and in particular if someone later decides to refactor the file by reordering tests, she could be confident that that would not break anything. In most of the surrounding tests it does not matter because 'git config' is run in a subdirectory that is not reused later. Patches fixing the exceptions below. > + git config remote.r.url ro:wrong && > + git config remote.r.pushurl rw:testrepo && > + git push r refs/heads/master:refs/remotes/origin/master && > + ( > + cd rw/testrepo && > + r=$(git show-ref -s --verify refs/remotes/origin/master) && > + test "z$r" = "z$the_commit" && > + > + test 1 = $(git for-each-ref refs/remotes/origin | wc -l) > + ) To produce more useful "./t5516-fetch-push.sh -v -i" output when the comparison fails: echo "$the_commit commit refs/remotes/origin/master" >expect && ( cd rw/testrepo && git for-each-ref refs/remotes/origin ) >actual && test_cmp expect actual Hope that helps, Jonathan Nieder (3): push test: use test_config where appropriate push test: simplify check of push result push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' t/t5516-fetch-push.sh | 156 +- 1 file changed, 65 insertions(+), 91 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] read-cache: avoid memcpy in expand_name_field in index v4
On Tue, Mar 19, 2013 at 12:50 AM, Junio C Hamano wrote: > While it is true that strbuf_remove(&sb, sb.len - trim, trim) is > equivalent to strbuf_setlen(&sb, sb.len - trim), I wonder why we see > any memcpy() in the first place. > > strbuf_remove(&sb, sb.len - trim, trim) is turned into > strbuf_splice(&sb, sb.len - trim, trim, NULL, 0) and then in turn it > does these two: > > memmove(sb.buf + (sb.len - trim) + 0, > sb.buf + sb.len, 0); > memcpy(sb.buf + (sb.len - trim), NULL, 0); > > both of which should be a no-op, no? Apparently my memcpy does not bail out early when the third arg is zero (glibc 2.11.2 on gentoo, x86). It cares more about memory alignment. This is the beginning of memcpy: mov%edi,%eax mov0x4(%esp),%edi mov%esi,%edx mov0x8(%esp),%esi mov%edi,%ecx xor%esi,%ecx and$0x3,%ecx mov0xc(%esp),%ecx cld jne75946 cmp$0x3,%ecx jbe75946 > There also is this call that has the same "trim at the right end": > > pretty.c: strbuf_remove(sb, sb->len - trimlen, trimlen); > > It almost makes me suggest that it may be a better solution to make > strbuf_remove() more intelligent about such a call pattern _if_ > these empty memmove/memcpy are so expensive, perhaps like the > attached. It could be that strbuf_splice() should be the one that > ought to be more intelligent, but I'll leave it up to you to > benchmark to find out where the best place to optimize is. memcpy is not expensive per-se, but this is (again) webkit, where expand_name_field (and memcpy) is called ~200k times. At that quantity I still prefer fixing in the "hot" call site expand_name_field(), and because strbuf_setlen is an inline function, we make no extra calls. Making strbuf_remove/strbuf_splice more intelligent may be good (or may make it harder to read), I don't know. But I think it could be a separate topic. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] submodule: add 'deinit' command
Jens Lehmann writes: > Am 12.03.2013 17:22, schrieb Junio C Hamano: >> Phil Hord writes: >> >>> I think this would be clearer if 'git deinit' said >>> >>> rm 'submodule/*' >>> >>> or maybe >>> >>> Removed workdir for 'submodule' >>> >>> Is it just me? >> >> The latter may probably be better. > > Hmm, it doesn't really remove the directory but only empties it > (it recreates it a few lines after removing it together with its > contents). So what about > > Cleared directory 'submodule' Sounds the cleanest among the suggested phrasing so far. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Jonathan Nieder writes: > Test nits: > ... > Hope that helps, > > Jonathan Nieder (3): > push test: use test_config where appropriate > push test: simplify check of push result > push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' Are these supposed to be follow-up patches? Preparatory steps that Rob can reroll on top? Something else? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Junio C Hamano wrote: > Jonathan Nieder writes: >> Test nits: >> ... >> Hope that helps, >> >> Jonathan Nieder (3): >> push test: use test_config where appropriate >> push test: simplify check of push result >> push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' > > Are these supposed to be follow-up patches? Preparatory steps that > Rob can reroll on top? Something else? Preparatory steps. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 27/45] Convert run_add_interactive to use struct pathspec
On Tue, Mar 19, 2013 at 1:26 AM, John Keeping wrote: > On Fri, Mar 15, 2013 at 01:06:42PM +0700, Nguyễn Thái Ngọc Duy wrote: >> This passes the pathspec, more or less unmodified, to >> git-add--interactive. The command itself does not process pathspec. It >> simply passes the pathspec to other builtin commands. So if all those >> commands support pathspec, we're good. > > This breaks "git reset --keep" in a subdirectory for me. > > I ran "git reset --keep " in a subdirectory and got: > > fatal: BUG: parse_pathspec cannot take no argument in this case > > Bisecting points to this commit. > > The simplest test case is: > > ( cd t && ../bin-wrappers/git reset --keep HEAD ) > > which works on master but not pu. Beautiful. I got messed up with C operator precedence. This should fix it. I'll check the rest of parse_pathspec calls later. diff --git a/builtin/reset.c b/builtin/reset.c index ab3917d..b665218 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -219,7 +219,7 @@ static void parse_args(struct pathspec *pathspec, *rev_ret = rev; parse_pathspec(pathspec, 0, PATHSPEC_PREFER_FULL | - patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0, + (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0), prefix, argv); } -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: building git ; need suggestion
On Mon, Mar 18, 2013 at 5:24 AM, Joydeep Bakshi wrote: > I'm closer to my requirement. I have found gitweb simply provide a GUI for > history check > and code comparison. And the git itself is good enough to do the ACL stuff > with hooks. > > I already have the following code to deploy the push into its work-tree You should try gitolite. It has very flexible rules, and it's already been implemented for you ;-) https://github.com/sitaramc/gitolite > === > #!/bin/bash > > while read oldrev newrev ref > do > branch=`echo $ref | cut -d/ -f3` > > if [ "master" == "$branch" ]; then > git --work-tree=/path/under/root/dir/live-site/ checkout -f $branch > echo 'Changes pushed live.' > fi > > if [ "dev" == "$branch" ]; then > git --work-tree=/path/under/root/dir/dev-site/ checkout -f $branch > echo 'Changes pushed to dev.' > fi > done > = > > This code can be extended for as many branches as you have. > > I now need a mechanism to restrict the user to it's own branch so that user > can't push into > any other branch in mistake. > > Say I have > > master branch -> only admin user can push here. > dev branch -> only user dev1 , dev2 and master can push here. > testing branch -> only user test1 and test2 can push here. > > I think this can also be done with pre-receive hook. Any suggestion on the > hook design is > welcome. Also this can be implemented on the above hook or in a separate hook. > A separate hook is better due to maintainability and then I need to call > multiple > pre-receive hook. Please suggest. > > Thanks > > > > On 18-Mar-2013, at 11:14 AM, Joydeep Bakshi > wrote: > >> >> On 15-Mar-2013, at 6:44 PM, Magnus Bäck wrote: >>> >>> Right, but that's R/W permissions. Almost any piece of Git hosting >>> software supports restriction of pushes. Discriminating *read* access >>> between developers and maintenance people sounds like a disaster if it's >>> the same organisation. >> >> Just restriction on push access is what required. >> >> -- >> To unsubscribe from this list: send the line "unsubscribe git" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] make pathless 'add [-u|-A]' warning less noisy
Hi, Jeff King wrote: > The > config option added by this patch gives them such an option. I suspect the need for this config option is a sign that the warning is too eager. After all, the whole idea of the change being safe is that it shouldn't make a difference the way people usually use git, no? In other words, how about the following patches? With them applied, hopefully no one would mind even if the warning becomes a fatal error. Looking forward to your thoughts, Jonathan Nieder (4): add: make pathless 'add [-u|-A]' warning a file-global function add: make warn_pathless_add() a no-op after first call add -u: only show pathless 'add -u' warning when changes exist outside cwd add -A: only show pathless 'add -A' warning when changes exist outside cwd builtin/add.c | 142 -- 1 file changed, 99 insertions(+), 43 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] add: make pathless 'add [-u|-A]' warning a file-global function
Currently warn_pathless_add() is only called directly by cmd_add(),
but that is about to change. Move its definition higher in the file
and pass the "--update" or "--all" option name used in its message
through globals instead of function arguments to make it easier to
call without passing values that will not change through the call
chain.
Signed-off-by: Jonathan Nieder
---
builtin/add.c | 69 +++
1 file changed, 36 insertions(+), 33 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8f..a4028eea 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -28,6 +28,41 @@ struct update_callback_data {
int add_errors;
};
+static const char *option_with_implicit_dot;
+static const char *short_option_with_implicit_dot;
+
+static void warn_pathless_add(void)
+{
+ assert(option_with_implicit_dot && short_option_with_implicit_dot);
+
+ /*
+* To be consistent with "git add -p" and most Git
+* commands, we should default to being tree-wide, but
+* this is not the original behavior and can't be
+* changed until users trained themselves not to type
+* "git add -u" or "git add -A". For now, we warn and
+* keep the old behavior. Later, the behavior can be changed
+* to tree-wide, keeping the warning for a while, and
+* eventually we can drop the warning.
+*/
+ warning(_("The behavior of 'git add %s (or %s)' with no path argument
from a\n"
+ "subdirectory of the tree will change in Git 2.0 and should
not be used anymore.\n"
+ "To add content for the whole tree, run:\n"
+ "\n"
+ " git add %s :/\n"
+ " (or git add %s :/)\n"
+ "\n"
+ "To restrict the command to the current directory, run:\n"
+ "\n"
+ " git add %s .\n"
+ " (or git add %s .)\n"
+ "\n"
+ "With the current Git version, the command is restricted to
the current directory."),
+ option_with_implicit_dot, short_option_with_implicit_dot,
+ option_with_implicit_dot, short_option_with_implicit_dot,
+ option_with_implicit_dot, short_option_with_implicit_dot);
+}
+
static int fix_unmerged_status(struct diff_filepair *p,
struct update_callback_data *data)
{
@@ -321,35 +356,6 @@ static int add_files(struct dir_struct *dir, int flags)
return exit_status;
}
-static void warn_pathless_add(const char *option_name, const char *short_name)
{
- /*
-* To be consistent with "git add -p" and most Git
-* commands, we should default to being tree-wide, but
-* this is not the original behavior and can't be
-* changed until users trained themselves not to type
-* "git add -u" or "git add -A". For now, we warn and
-* keep the old behavior. Later, the behavior can be changed
-* to tree-wide, keeping the warning for a while, and
-* eventually we can drop the warning.
-*/
- warning(_("The behavior of 'git add %s (or %s)' with no path argument
from a\n"
- "subdirectory of the tree will change in Git 2.0 and should
not be used anymore.\n"
- "To add content for the whole tree, run:\n"
- "\n"
- " git add %s :/\n"
- " (or git add %s :/)\n"
- "\n"
- "To restrict the command to the current directory, run:\n"
- "\n"
- " git add %s .\n"
- " (or git add %s .)\n"
- "\n"
- "With the current Git version, the command is restricted to
the current directory."),
- option_name, short_name,
- option_name, short_name,
- option_name, short_name);
-}
-
int cmd_add(int argc, const char **argv, const char *prefix)
{
int exit_status = 0;
@@ -360,8 +366,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
- const char *option_with_implicit_dot = NULL;
- const char *short_option_with_implicit_dot = NULL;
git_config(add_config, NULL);
@@ -392,8 +396,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
if (prefix)
- warn_pathless_add(option_with_implicit_dot,
- short_option_with_implicit_dot);
+ warn_pathless_add();
argc = 1;
argv = here;
}
--
1.8.2.rc3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kern
[PATCH 2/4] add: make warn_pathless_add() a no-op after first call
Make warn_pathless_add() print its warning the first time it is called
and do nothing if called again. This will make it easier to show the
warning on the fly when a relevant condition is detected without
risking showing it multiple times when multiple such conditions hold.
Signed-off-by: Jonathan Nieder
---
builtin/add.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/builtin/add.c b/builtin/add.c
index a4028eea..a424e69d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -33,8 +33,13 @@ static const char *short_option_with_implicit_dot;
static void warn_pathless_add(void)
{
+ static int shown;
assert(option_with_implicit_dot && short_option_with_implicit_dot);
+ if (shown)
+ return;
+ shown = 1;
+
/*
* To be consistent with "git add -p" and most Git
* commands, we should default to being tree-wide, but
--
1.8.2.rc3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
A common workflow in large projects is to chdir into a subdirectory of
interest and only do work there:
cd src
vi foo.c
make test
git add -u
git commit
The upcoming change to 'git add -u' behavior would not affect such a
workflow: when the only changes present are in the current directory,
'git add -u' will add all changes, and whether that happens via an
implicit "." or implicit ":/" parameter is an unimportant
implementation detail.
The warning about use of 'git add -u' with no pathspec is annoying
because it serves no purpose in this case. So suppress the warning
unless there are changes outside the cwd that are not being added.
Signed-off-by: Jonathan Nieder
---
builtin/add.c | 51 ---
1 file changed, 44 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index a424e69d..f05ec1c1 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -89,6 +89,22 @@ static int fix_unmerged_status(struct diff_filepair *p,
return DIFF_STATUS_MODIFIED;
}
+static void warn_if_outside_pathspec(struct diff_queue_struct *q,
+struct diff_options *opt, void *cbdata)
+{
+ int i;
+ const char **pathspec = cbdata;
+
+ for (i = 0; i < q->nr; i++) {
+ const char *path = q->queue[i]->one->path;
+
+ if (!match_pathspec(pathspec, path, strlen(path), 0, NULL)) {
+ warn_pathless_add();
+ return;
+ }
+ }
+}
+
static void update_callback(struct diff_queue_struct *q,
struct diff_options *opt, void *cbdata)
{
@@ -121,20 +137,26 @@ static void update_callback(struct diff_queue_struct *q,
}
}
-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+static void diff_files_with_callback(const char *prefix, const char **pathspec,
+diff_format_fn_t callback, void *data)
{
- struct update_callback_data data;
struct rev_info rev;
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
init_pathspec(&rev.prune_data, pathspec);
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
- rev.diffopt.format_callback = update_callback;
- data.flags = flags;
- data.add_errors = 0;
- rev.diffopt.format_callback_data = &data;
+ rev.diffopt.format_callback = callback;
+ rev.diffopt.format_callback_data = data;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+}
+
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+{
+ struct update_callback_data data;
+ data.flags = flags;
+ data.add_errors = 0;
+ diff_files_with_callback(prefix, pathspec, update_callback, &data);
return !!data.add_errors;
}
@@ -371,6 +393,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+ int implicit_dot = 0;
git_config(add_config, NULL);
@@ -400,10 +423,11 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
}
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
- if (prefix)
+ if (prefix && addremove)
warn_pathless_add();
argc = 1;
argv = here;
+ implicit_dot = 1;
}
add_new_files = !take_worktree_changes && !refresh_only;
@@ -450,6 +474,19 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
goto finish;
}
+ /*
+* Check if "git add -A" or "git add -u" was run from a
+* subdirectory with a modified file outside that directory,
+* and warn if so.
+*
+* "git add -u" will behave like "git add -u :/" instead of
+* "git add -u ." in the future. This warning prepares for
+* that change.
+*/
+ if (implicit_dot)
+ diff_files_with_callback(prefix, NULL,
+ warn_if_outside_pathspec, pathspec);
+
if (pathspec) {
int i;
struct path_exclude_check check;
--
1.8.2.rc3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] add -A: only show pathless 'add -A' warning when changes exist outside cwd
In the spirit of the recent similar change for 'git add -u', avoid
pestering users that restrict their attention to a subdirectory and
will not be affected by the coming change in the behavior of pathless
'git add -A'.
Signed-off-by: Jonathan Nieder
---
builtin/add.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index f05ec1c1..56ac4519 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -160,7 +160,9 @@ int add_files_to_cache(const char *prefix, const char
**pathspec, int flags)
return !!data.add_errors;
}
-static char *prune_directory(struct dir_struct *dir, const char **pathspec,
int prefix)
+#define WARN_IMPLICIT_DOT (1u << 0)
+static char *prune_directory(struct dir_struct *dir, const char **pathspec,
+int prefix, unsigned flag)
{
char *seen;
int i, specs;
@@ -177,6 +179,16 @@ static char *prune_directory(struct dir_struct *dir, const
char **pathspec, int
if (match_pathspec(pathspec, entry->name, entry->len,
prefix, seen))
*dst++ = entry;
+ else if (flag & WARN_IMPLICIT_DOT)
+ /*
+* "git add -A" was run from a subdirectory with a
+* new file outside that directory.
+*
+* "git add -A" will behave like "git add -A :/"
+* instead of "git add -A ." in the future.
+* Warn about the coming behavior change.
+*/
+ warn_pathless_add();
}
dir->nr = dst - dir->entries;
add_pathspec_matches_against_index(pathspec, seen, specs);
@@ -423,8 +435,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
}
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
- if (prefix && addremove)
- warn_pathless_add();
argc = 1;
argv = here;
implicit_dot = 1;
@@ -464,9 +474,10 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
}
/* This picks up the paths that are not tracked */
- baselen = fill_directory(&dir, pathspec);
+ baselen = fill_directory(&dir, implicit_dot ? NULL : pathspec);
if (pathspec)
- seen = prune_directory(&dir, pathspec, baselen);
+ seen = prune_directory(&dir, pathspec, baselen,
+ implicit_dot ? WARN_IMPLICIT_DOT : 0);
}
if (refresh_only) {
--
1.8.2.rc3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/8] Improve git-status --ignored
Karsten Blees writes: > This patch series addresses several bugs and performance issues in > .gitignore processing that came up in the inotify discussion. Thanks. How does this interact with the nd/read-directory-recursive-optim topic that has been cooking for a while? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] make pathless 'add [-u|-A]' warning less noisy
On Mon, Mar 18, 2013 at 08:44:15PM -0700, Jonathan Nieder wrote: > > The > > config option added by this patch gives them such an option. > > I suspect the need for this config option is a sign that the warning > is too eager. After all, the whole idea of the change being safe is > that it shouldn't make a difference the way people usually use git, > no? > > In other words, how about the following patches? With them applied, > hopefully no one would mind even if the warning becomes a fatal error. Clever. I think it would help in my case. I sometimes follow the workflow you describe in patch 3 (i.e., just working in a subdir), and sometimes do something more like: $ vi foo.c $ cd t $ vi t-foo.sh $ ./t-foo.sh $ git add -u With your patches, we would continue to warn about the second case, but I think that is a good thing; git is not doing what I want. But by reducing the false positives from the first case, I would start to actually pay attention to the warning more. > Jonathan Nieder (4): > add: make pathless 'add [-u|-A]' warning a file-global function > add: make warn_pathless_add() a no-op after first call > add -u: only show pathless 'add -u' warning when changes exist outside cwd > add -A: only show pathless 'add -A' warning when changes exist outside cwd I don't see anything obviously wrong with the patches themselves. I wonder if we would want to change the warning to be more explicit that yes, there really were files that were impacted by this. And possibly even list them. I suspect I would not even mind that becoming the final behavior. I.e., going to: $ cd subdir && git add -u warning: Using 'git add -u' without a pathspec operates only on the current subdirectory. Updates from the following files were NOT staged: file1 file2 other-subdir/file3 now, and then eventually converting the warning into a fatal error (and demanding that the user use ":/" or "." as appropriate). But in the long run, I guess defaulting to ":/" will be more convenient, so there is no point in complaining about the ambiguity forever. And in that case, since the warning is just a placeholder, I don't know that it's worth much effort to make it fancier. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
Jonathan Nieder writes: > A common workflow in large projects is to chdir into a subdirectory of > interest and only do work there: > > cd src > vi foo.c > make test > git add -u > git commit > > The upcoming change to 'git add -u' behavior would not affect such a > workflow: when the only changes present are in the current directory, > 'git add -u' will add all changes, and whether that happens via an > implicit "." or implicit ":/" parameter is an unimportant > implementation detail. > > The warning about use of 'git add -u' with no pathspec is annoying > because it serves no purpose in this case. So suppress the warning > unless there are changes outside the cwd that are not being added. That is a logical extension of the reason why we do not emit warnings when run at the top level. A user who has known about and is very much accustomed to the "current directory only" behaviour may run "git add -u/-A" always from the top in the current project she happens to be working on until Git 2.0 happens, and will not get any warnings. We are already robbing the chance to learn about and prepare for the upcoming change from her. And this patch makes it even more so. It does not make things fundamentally worse, but it makes it more likely to hurt such a user. The implemenation appears to run an extra diff_files() in addition to the one we already have to run in order to implement the "add -u" to collect modified/deleted paths. Is that the best we can do? I am wondering if we can special case the no-pathspec case to have add_files_to_cache() call underlying run_diff_files() without any pathspec, inspect the paths that are modified and/or deleted in the update_callback, add ones that are under the $prefix while noticing the ones outside as warning worthy events. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/8] Improve git-status --ignored
On Tue, Mar 19, 2013 at 11:08 AM, Junio C Hamano wrote: > Karsten Blees writes: > >> This patch series addresses several bugs and performance issues in >> .gitignore processing that came up in the inotify discussion. > > Thanks. > > How does this interact with the nd/read-directory-recursive-optim > topic that has been cooking for a while? I think 8/8 is another version of nd/read-directory-recursive-optim -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
Junio C Hamano wrote:
> The implemenation appears to run an extra diff_files() in addition
> to the one we already have to run in order to implement the "add -u"
> to collect modified/deleted paths. Is that the best we can do?
>
> I am wondering if we can special case the no-pathspec case to have
> add_files_to_cache() call underlying run_diff_files() without any
> pathspec, inspect the paths that are modified and/or deleted in the
> update_callback, add ones that are under the $prefix while noticing
> the ones outside as warning worthy events.
Yes, that can work, for example like this (replacing the patch you're
replying to).
-- >8 --
Subject: add -u: only show pathless 'add -u' warning when changes exist outside
cwd
A common workflow in large projects is to chdir into a subdirectory of
interest and only do work there:
cd src
vi foo.c
make test
git add -u
git commit
The upcoming change to 'git add -u' behavior would not affect such a
workflow: when the only changes present are in the current directory,
'git add -u' will add all changes, and whether that happens via an
implicit "." or implicit ":/" parameter is an unimportant
implementation detail.
The warning about use of 'git add -u' with no pathspec is annoying
because it serves no purpose in this case. So suppress the warning
unless there are changes outside the cwd that are not being added.
Signed-off-by: Jonathan Nieder
---
builtin/add.c | 41 +
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index a424e69d..e3ed5d93 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,7 @@ static int take_worktree_changes;
struct update_callback_data {
int flags;
int add_errors;
+ const char **implicit_dot;
};
static const char *option_with_implicit_dot;
@@ -94,10 +95,26 @@ static void update_callback(struct diff_queue_struct *q,
{
int i;
struct update_callback_data *data = cbdata;
+ const char **implicit_dot = data->implicit_dot;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
const char *path = p->one->path;
+
+ /*
+* Check if "git add -A" or "git add -u" was run from a
+* subdirectory with a modified file outside that directory,
+* and warn if so.
+*
+* "git add -u" will behave like "git add -u :/" instead of
+* "git add -u ." in the future. This warning prepares for
+* that change.
+*/
+ if (implicit_dot &&
+ !match_pathspec(implicit_dot, path, strlen(path), 0, NULL))
{
+ warn_pathless_add();
+ continue;
+ }
switch (fix_unmerged_status(p, data)) {
default:
die(_("unexpected diff status %c"), p->status);
@@ -121,17 +138,30 @@ static void update_callback(struct diff_queue_struct *q,
}
}
+#define ADD_CACHE_IMPLICIT_DOT 32
int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
{
struct update_callback_data data;
struct rev_info rev;
+
+ data.flags = flags & ~ADD_CACHE_IMPLICIT_DOT;
+ data.add_errors = 0;
+ data.implicit_dot = NULL;
+ if (flags & ADD_CACHE_IMPLICIT_DOT) {
+ /*
+* Check for modified files throughout the worktree so
+* update_callback has a chance to warn about changes
+* outside the cwd.
+*/
+ data.implicit_dot = pathspec;
+ pathspec = NULL;
+ }
+
init_revisions(&rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
init_pathspec(&rev.prune_data, pathspec);
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
- data.flags = flags;
- data.add_errors = 0;
rev.diffopt.format_callback_data = &data;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
@@ -371,6 +401,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+ int implicit_dot = 0;
git_config(add_config, NULL);
@@ -400,10 +431,11 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
}
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
- if (prefix)
+ if (prefix && addremove)
warn_pathless_add();
argc = 1;
argv = here;
+ implicit_dot = 1;
}
add_new_files = !take_worktree_changes && !refresh_only;
Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
Junio C Hamano wrote: > The implemenation appears to run an extra diff_files() in addition > to the one we already have to run in order to implement the "add -u" > to collect modified/deleted paths. Is that the best we can do? At least the following should be squashed in to avoid wasted effort in the easy case (cwd at top of worktree). Thanks for catching it. diff --git i/builtin/add.c w/builtin/add.c index f05ec1c1..8e4cdfae 100644 --- i/builtin/add.c +++ w/builtin/add.c @@ -483,7 +483,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) * "git add -u ." in the future. This warning prepares for * that change. */ - if (implicit_dot) + if (prefix && implicit_dot) diff_files_with_callback(prefix, NULL, warn_if_outside_pathspec, pathspec); -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
On Tue, Mar 19, 2013 at 11:25 AM, Junio C Hamano wrote: > I am wondering if we can special case the no-pathspec case to have > add_files_to_cache() call underlying run_diff_files() without any > pathspec, inspect the paths that are modified and/or deleted in the > update_callback, add ones that are under the $prefix while noticing > the ones outside as warning worthy events. My concern is run full-tree diff can be expensive on large repos (one of the reasons the user may choose to work from within a subdirectory). We can exit as soon as we find a difference outside $prefix. But in case we find nothing, we need to diff the whole worktree. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
Duy Nguyen wrote:
> My concern is run full-tree diff can be expensive on large repos (one
> of the reasons the user may choose to work from within a
> subdirectory). We can exit as soon as we find a difference outside
> $prefix. But in case we find nothing, we need to diff the whole
> worktree.
Yes. This is an argument for the single-diff approach that Junio
suggested, since at least it's not significantly slower than the
future default behavior ("git add -u :/").
Sysadmins and others helping to train users will need to make sure
people working on large repos understand that they can use "git add -u ."
to avoid a speed penalty.
Thanks for looking it over,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
Jonathan Nieder writes: > The warning about use of 'git add -u' with no pathspec is annoying > because it serves no purpose in this case. So suppress the warning > unless there are changes outside the cwd that are not being added. No time to review the code now. I thought about implementing something like that, but did not do it because I didn't want the change in the code to be too big. At some point, we'll have to remove the warning and it's easier with my version than with yours. But the "damage" to the code do not seem too big, so that's probably OK and will actually reduce the pain for some users. Discussions showed that many people were already using "git add -u" without knowing whether it was full tree or not, so for these people the change is somehow harmless anyway ;-). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
