* [PATCH] http-push: refactor request url creation
From: Tay Ray Chuan @ 2009-01-28 22:19 UTC (permalink / raw)
To: git, Johannes Schindelin, Junio C Hamano
Currently, functions that deal with objects on the remote repository have to allocate and do strcpys to generate the URL.
This patch saves them this trouble, by providing two functions, "append_remote_object_url" and "get_remote_object_url".
Both generate a URL, with either the object's 2-digit hex directory (eg. /objects/a1/), or the complete object location (eg. /objects/a1/b2).
However, they differ in that "append_remote_object_url" appends this URL to a strbuf, while "get_remote_object_url" wraps around the former and returns the URL directly in char*. Users usually would use "get_remote_object_url", but may find "append_remote_object_url" useful if they require further string operations on the URL.
PS. In "start_fetch_loose", the variable "url" which is passed to "curl_easy_setopt" is removed, and in its place "request->url" is used. This is safe, since curl strdup's it.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Acked-by: Junio C Hamano <gitster@pobox.com>
---
* renamed only_two_digit_postfix in original patch to only_two_digit_prefix
* rebased and generated on master (5dc1308)
* updated with Junio's changes (if (...) and fix memory leak)
* updated with Junio's double interface
* rebased and generated on master (297f6a5)
http-push.c | 64 +++++++++++++++++++++++-----------------------------------
1 files changed, 25 insertions(+), 39 deletions(-)
diff --git a/http-push.c b/http-push.c
index 59037df..6511d19 100644
--- a/http-push.c
+++ b/http-push.c
@@ -209,6 +209,20 @@ static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum d
return dav_headers;
}
+static void append_remote_object_url(struct strbuf *buf, const char *url, const char *hex, int only_two_digit_prefix)
+{
+ strbuf_addf(buf, "%sobjects/%.*s/", url, 2, hex);
+ if (!only_two_digit_prefix)
+ strbuf_addf(buf, "%s", hex+2);
+}
+
+static char *get_remote_object_url(const char *url, const char *hex, int only_two_digit_prefix)
+{
+ struct strbuf buf = STRBUF_INIT;
+ append_remote_object_url(&buf, url, hex, only_two_digit_prefix);
+ return strbuf_detach(&buf, NULL);
+}
+
static void finish_request(struct transfer_request *request);
static void release_request(struct transfer_request *request);
@@ -254,8 +268,6 @@ static void start_fetch_loose(struct transfer_request *request)
char *hex = sha1_to_hex(request->obj->sha1);
char *filename;
char prevfile[PATH_MAX];
- char *url;
- char *posn;
int prevlocal;
unsigned char prev_buf[PREV_BUF_SIZE];
ssize_t prev_read = 0;
@@ -305,17 +317,7 @@ static void start_fetch_loose(struct transfer_request *request)
git_SHA1_Init(&request->c);
- url = xmalloc(strlen(remote->url) + 50);
- request->url = xmalloc(strlen(remote->url) + 50);
- strcpy(url, remote->url);
- posn = url + strlen(remote->url);
- strcpy(posn, "objects/");
- posn += 8;
- memcpy(posn, hex, 2);
- posn += 2;
- *(posn++) = '/';
- strcpy(posn, hex + 2);
- strcpy(request->url, url);
+ request->url = get_remote_object_url(remote->url, hex, 0);
/* If a previous temp file is present, process what was already
fetched. */
@@ -359,7 +361,7 @@ static void start_fetch_loose(struct transfer_request *request)
curl_easy_setopt(slot->curl, CURLOPT_FILE, request);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
curl_easy_setopt(slot->curl, CURLOPT_ERRORBUFFER, request->errorstr);
- curl_easy_setopt(slot->curl, CURLOPT_URL, url);
+ curl_easy_setopt(slot->curl, CURLOPT_URL, request->url);
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);
/* If we have successfully processed data from a previous fetch
@@ -388,16 +390,8 @@ static void start_mkcol(struct transfer_request *request)
{
char *hex = sha1_to_hex(request->obj->sha1);
struct active_request_slot *slot;
- char *posn;
- request->url = xmalloc(strlen(remote->url) + 13);
- strcpy(request->url, remote->url);
- posn = request->url + strlen(remote->url);
- strcpy(posn, "objects/");
- posn += 8;
- memcpy(posn, hex, 2);
- posn += 2;
- strcpy(posn, "/");
+ request->url = get_remote_object_url(remote->url, hex, 1);
slot = get_active_slot();
slot->callback_func = process_response;
@@ -512,7 +506,7 @@ static void start_put(struct transfer_request *request)
{
char *hex = sha1_to_hex(request->obj->sha1);
struct active_request_slot *slot;
- char *posn;
+ struct strbuf buf = STRBUF_INIT;
enum object_type type;
char hdr[50];
void *unpacked;
@@ -551,21 +545,13 @@ static void start_put(struct transfer_request *request)
request->buffer.buf.len = stream.total_out;
- request->url = xmalloc(strlen(remote->url) +
- strlen(request->lock->token) + 51);
- strcpy(request->url, remote->url);
- posn = request->url + strlen(remote->url);
- strcpy(posn, "objects/");
- posn += 8;
- memcpy(posn, hex, 2);
- posn += 2;
- *(posn++) = '/';
- strcpy(posn, hex + 2);
- request->dest = xmalloc(strlen(request->url) + 14);
- sprintf(request->dest, "Destination: %s", request->url);
- posn += 38;
- *(posn++) = '_';
- strcpy(posn, request->lock->token);
+ strbuf_addstr(&buf, "Destination: ");
+ append_remote_object_url(&buf, remote->url, hex, 0);
+ request->dest = strbuf_detach(&buf, NULL);
+
+ append_remote_object_url(&buf, remote->url, hex, 0);
+ strbuf_addstr(&buf, request->lock->token);
+ request->url = strbuf_detach(&buf, NULL);
slot = get_active_slot();
slot->callback_func = process_response;
--
1.6.1.1.251.g0dfa1
^ permalink raw reply related
* Re: "malloc failed"
From: Pau Garcia i Quiles @ 2009-01-28 22:16 UTC (permalink / raw)
To: Jeff King; +Cc: David Abrahams, git
In-Reply-To: <20090128050225.GA18546@coredump.intra.peff.net>
On Wed, Jan 28, 2009 at 6:02 AM, Jeff King <peff@peff.net> wrote:
> How big is the repository? How big are the biggest files? I have a
> 3.5G repo with files ranging from a few bytes to about 180M. I've never
> run into malloc problems or gone into swap on my measly 1G box.
> How does your dataset compare?
I also have malloc problems but only on Windows, on Linux it works fine.
My case: I have a 500 MB repository with a 1GB working tree, with
binary files ranging from 100KB to 50MB and a few thousand source
files.
I have two branches ('master' and 'cmake') and the latter has suffered
a huge hierarchy reorganization.
When I merge 'master' in 'cmake', if I use the 'subtree' strategy, it
works fine. If I use any other strategy, after a couple of minutes I
receive a "malloc failed" and the tree is all messed up. As I said, on
Linux it works fine, so maybe it's a Windows-specific problem.
--
Pau Garcia i Quiles
http://www.elpauer.org
(Due to my workload, I may need 10 days to answer)
^ permalink raw reply
* Re: (beginner) git rm
From: Björn Steinbrink @ 2009-01-28 22:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Zabre, git
In-Reply-To: <7vab9bm85b.fsf@gitster.siamese.dyndns.org>
[Zabre, please keep the Cc: list when replying]
On 2009.01.28 13:29:36 -0800, Junio C Hamano wrote:
> Zabre <427@free.fr> writes:
> > Björn Steinbrink wrote:
> >>
> >> The "git checkout -- d.txt" is also a valid command, but that restores
> >> the file from the index.
> >>
> >> git checkout -- paths
> >> ==> Copy "paths" from the index to the working tree
> >>
> >> git checkout <tree-ish> -- paths
> >> ==> Copy "paths" from the tree-ish to the index and working tree
> >>
> >> So, for "rm d.txt", a plain "git checkout -- d.txt" would also do the
> >> trick, as d.txt is still in the index. But your "git rm d.txt" also
> >> removed the file from the index, and thus that checkout does nothing.
> >> But "git checkout HEAD -- d.txt" works, as it gets the file from HEAD
> >> and puts it into the index and working tree.
> >
> > This is enlightening, thank you very much!
> > (I knew I would love git more and more)
> >
> > Oh just one (probably stupid) thing : <tree-ish> does represent a directory
> > being the root of a tree of folders (which has been added to the index),
> > does it?
>
> Yeah, it typically is a commit object.
>
> Björn said "Copy", but the operation really is like checking out a book
> from a library and "checkout" is a good word for it. "I do not like what
> I have in my work tree, and I'd like to replace it with a fresh one taken
> out of the index (or, out of that commit)".
With "checkout", I'm still a bit unsure about which term to use, because
of the behaviour you get with, for example, "git checkout HEAD --
directory". It always just adds or replaces files, but never removes
them. So it's not really like taking the old directory out of the repo
and using that instead. For example:
git rm dir/old_file
echo 123 > dir/new_file
git add dir/new_file
git checkout HEAD -- dir
That won't remove dir/new_file from the index (and of course it won't
drop it from the working tree). That's the one thing where "git checkout
HEAD -- dir" differs from "git reset HEAD -- dir && git checkout --
dir". IIRC we've talked about that on #git a few months ago, but I don't
recall what conclusions we came up with.
It would probably be better to say that checkout only works with the
blobs (because the index doesn't have entries for trees, right?) that
exist in the given tree-ish. And thus it doesn't remove entries from the
index. But that feels a bit convoluted. :-/
Björn
^ permalink raw reply
* Re: "malloc failed"
From: David Abrahams @ 2009-01-28 21:53 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20090128050225.GA18546@coredump.intra.peff.net>
On Wed, 28 Jan 2009 00:02:25 -0500, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 27, 2009 at 10:04:42AM -0500, David Abrahams wrote:
>
>> I've been abusing Git for a purpose it wasn't intended to serve:
>> archiving a large number of files with many duplicates and
>> near-duplicates. Every once in a while, when trying to do something
>> really big, it tells me "malloc failed" and bails out (I think it's
>> during "git add" but because of the way I issued the commands I can't
>> tell: it could have been a commit or a gc). This is on a 64-bit linux
>> machine with 8G of ram and plenty of swap space, so I'm surprised.
>>
>> Git is doing an amazing job at archiving and compressing all this stuff
>> I'm putting in it, but I have to do it a wee bit at a time or it craps
>> out. Bug?
>
> How big is the repository? How big are the biggest files? I have a
> 3.5G repo with files ranging from a few bytes to about 180M. I've never
> run into malloc problems or gone into swap on my measly 1G box.
> How does your dataset compare?
I'll try to do some research. Gotta go pick up my boy now...
> As others have mentioned, git wasn't really designed specifically for
> those sorts of numbers, but in the interests of performance, I find git
> is usually pretty careful about not keeping too much useless stuff in
> memory at one time. And the fact that you can perform the same
> operation a little bit at a time and achieve success implies to me there
> might be a leak or some silly behavior that can be fixed.
>
> It would help a lot if we knew the operation that was causing the
> problem. Can you try to isolate the failed command next time it happens?
root@recovery:/olympic/deuce/review# ulimit -v
unlimited
root@recovery:/olympic/deuce/review# git add hydra.bak/home-dave
fatal: Out of memory, malloc failed
The process never even gets close to my total installed RAM size, much less
my whole VM space size.
--
David Abrahams
Boostpro Computing
http://www.boostpro.com
^ permalink raw reply
* Re: [PATCHv2 0/6] gitweb: feed metadata enhancements
From: Junio C Hamano @ 2009-01-28 22:03 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Jakub Narebski
In-Reply-To: <cb7bb73a0901281357w52aa275i1a734b171396dd58@mail.gmail.com>
Ok, let's apply it directly to 'master', then.
Thanks.
^ permalink raw reply
* Re: [PATCHv2 0/6] gitweb: feed metadata enhancements
From: Giuseppe Bilotta @ 2009-01-28 21:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narebski
In-Reply-To: <7vskn3m9l1.fsf@gitster.siamese.dyndns.org>
On Wed, Jan 28, 2009 at 9:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This is independent from your other topic on PATH_INFO, right? I am aware
> that other one is being polished between you and Jakub, but is anobody
> doing anything on this one, or is it already polished enough?
Yes it's a totally independent topic. For what it's worth, this series
is running on ruby-rbot.org gitweb and it fixed a bunch of rss polling
related issues we were having with the bot we use to watch it.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [PATCHv2 0/6] gitweb: feed metadata enhancements
From: Jakub Narebski @ 2009-01-28 21:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Giuseppe Bilotta, git
In-Reply-To: <7vskn3m9l1.fsf@gitster.siamese.dyndns.org>
On Wed, 28 Jan 2009, Junio C Hamano wrote:
> This is independent from your other topic on PATH_INFO, right?
Yes, it is independent on adding BASE for PATH_INFO.
> I am aware that other one is being polished between you and Jakub,
The code (in BASE for PATH_INFO) looks now good, only commit
message might be further improved.
> but is anobody doing anything on this one, or is it already polished enough?
I think it is good, but I do not use web feeds (Atom and RSS)
from gitweb myself, therefore I don't feel competent to tell
if it is good enough. With exception of last one there are
quite simple and feel good; the last one is a bit more involved,
but I think also good (it would be nice to say in commit
message that HTTP::Date can be found in perl-libwww-perl).
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: Emacs git-mode feature request: support fill-paragraph correctly
From: Junio C Hamano @ 2009-01-28 21:32 UTC (permalink / raw)
To: Alexandre Julliard; +Cc: git, Peter Simons
In-Reply-To: <878wow7sgx.fsf@write-only.cryp.to>
Peter Simons <simons@cryp.to> writes:
> Hi Alexandre,
>
> > diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
> > index 3c37d0d..e314c44 100644
> > --- a/contrib/emacs/git.el
> > +++ b/contrib/emacs/git.el
> > @@ -1331,6 +1331,7 @@ Return the list of files that haven't been handled."
> > (log-edit-diff-function . git-log-edit-diff)) buffer)
> > (log-edit 'git-do-commit nil 'git-log-edit-files buffer))
> > (setq font-lock-keywords (font-lock-compile-keywords git-log-edit-font-lock-keywords))
> > + (setq paragraph-separate (concat (regexp-quote git-log-msg-separator) "$\\|Author: \\|Date: \\|Merge: \\|Signed-off-by: \\|\f\\|[ ]*$"))
> > (setq buffer-file-coding-system coding-system)
> > (re-search-forward (regexp-quote (concat git-log-msg-separator "\n")) nil t))))
>
> that patch has the desired effect (tested with GNU Emacs 22.3.1 and
> GIT 1.6.1). Thank you very much.
>
> Now, I'd be hugely in favor of applying that change to the repository
> so that future versions of GIT have it.
Alexandre?
^ permalink raw reply
* Re: [PATCH] diff.c: output correct index lines for a split diff
From: Junio C Hamano @ 2009-01-28 21:32 UTC (permalink / raw)
To: Jeff King; +Cc: Pixel, git
In-Reply-To: <20090126090712.GA12648@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Jan 26, 2009 at 12:33:56AM -0800, Junio C Hamano wrote:
>
>> This moves the code to generate metainfo lines around, so that two
>> independent sets of metainfo lines are used for the split halves.
>
> The patch and the generated output look correct to me, so
>
> Acked-by: Jeff King <peff@peff.net>
>
>> I did not include your new test script here; perhaps we can add it to an
>> existing typechange diff/apply test, like t4114?
>
> I think it makes sense to add to t4114. Please squash in the test below.
Thanks.
^ permalink raw reply
* Re: [PATCH/RFC v1 3/6] cleanup of write_entry() in entry.c
From: Junio C Hamano @ 2009-01-28 21:31 UTC (permalink / raw)
To: Kjetil Barvik; +Cc: git
In-Reply-To: <1233004637-15112-4-git-send-email-barvik@broadpark.no>
Kjetil Barvik <barvik@broadpark.no> writes:
> The switch-cases for S_IFREG and S_IFLNK was so similar that it will
> be better to do some cleanup and use the common parts of it.
>
> Also fold the longest lines such that no line is longer then 80 chars
> or so.
>
> And the entry.c file should now be clean for 'gcc -Wextra' warnings.
>
> Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
> ---
>
> If people do not like this approach I can be willing to drop it from
> this patch-series, but then I get some source code duplication from
> the next patch (4/6).
Merging of the two similar codepaths looked good from a cursory reading,
but if you have doubts about one patch, it usually is easier for other
people to work with you if the series is reordered to have it near the
end, iow, undisputably good ones first.
> - return error("git checkout-index: unable to read sha1 file of %s (%s)",
> - path, sha1_to_hex(ce->sha1));
> + return error("git checkout-index: "\
> + "unable to read sha1 file of %s (%s)",
> + path, sha1_to_hex(ce->sha1));
You lost greppability when somebody calls for help saying "I am getting an
error message that says 'git checkout-index: unable to read sha1 file'".
^ permalink raw reply
* Re: [PATCH v2] Makefile: Make 'configure --with-expat=path' actually work
From: Junio C Hamano @ 2009-01-28 21:30 UTC (permalink / raw)
To: Serge van den Boom; +Cc: git
In-Reply-To: <alpine.BSF.2.00.0901282120240.74552@toad.stack.nl>
Thanks.
^ permalink raw reply
* Re: (beginner) git rm
From: Junio C Hamano @ 2009-01-28 21:29 UTC (permalink / raw)
To: Zabre; +Cc: git
In-Reply-To: <1233175322729-2234796.post@n2.nabble.com>
Zabre <427@free.fr> writes:
> Björn Steinbrink wrote:
>>
>> The "git checkout -- d.txt" is also a valid command, but that restores
>> the file from the index.
>>
>> git checkout -- paths
>> ==> Copy "paths" from the index to the working tree
>>
>> git checkout <tree-ish> -- paths
>> ==> Copy "paths" from the tree-ish to the index and working tree
>>
>> So, for "rm d.txt", a plain "git checkout -- d.txt" would also do the
>> trick, as d.txt is still in the index. But your "git rm d.txt" also
>> removed the file from the index, and thus that checkout does nothing.
>> But "git checkout HEAD -- d.txt" works, as it gets the file from HEAD
>> and puts it into the index and working tree.
>
> This is enlightening, thank you very much!
> (I knew I would love git more and more)
>
> Oh just one (probably stupid) thing : <tree-ish> does represent a directory
> being the root of a tree of folders (which has been added to the index),
> does it?
Yeah, it typically is a commit object.
Björn said "Copy", but the operation really is like checking out a book
from a library and "checkout" is a good word for it. "I do not like what
I have in my work tree, and I'd like to replace it with a fresh one taken
out of the index (or, out of that commit)".
^ permalink raw reply
* Re: [PATCH] Add --ff-only flag to git-merge
From: Junio C Hamano @ 2009-01-28 21:12 UTC (permalink / raw)
To: Yuval Kogman; +Cc: git
In-Reply-To: <1233147238-30082-1-git-send-email-nothingmuch@woobling.org>
Yuval Kogman <nothingmuch@woobling.org> writes:
> This patch adds an --ff-only flag to git-merge. When specified git-merge
> will exit with an error whenver a merge will not be a simple fast
> forward, similar to the default behavior of git push.
> ---
Lacks a sign-off.
> @@ -167,6 +168,8 @@ static struct option builtin_merge_options[] = {
> "perform a commit if the merge succeeds (default)"),
> OPT_BOOLEAN(0, "ff", &allow_fast_forward,
> "allow fast forward (default)"),
> + OPT_BOOLEAN(0, "ff-only", &only_fast_forward,
> + "allow only fast forward"),
> OPT_CALLBACK('s', "strategy", &use_strategies, "strategy",
> "merge strategy to use", option_parse_strategy),
> OPT_CALLBACK('m', "message", &merge_msg, "message",
After parse_options() returns, there are a few existing tests to reject
nonsensical combination of options. I think there are some combinations
you would want to reject (for example, "--no-ff --ff-only" is an obvious
such nonsensical combination, but there may be others, such as giving a
strategy explicitly).
> @@ -1012,6 +1015,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> finish(o->sha1, msg.buf);
> drop_save();
> return 0;
> + } else if ( only_fast_forward ) {
> + printf("Merge is non fast forward, aborting.\n");
> + return 1;
> } else if (!remoteheads->next && common->next)
> ;
This is wrong for at least three reasons:
* Style of "if (expression) {", somebody else already pointed out.
* Only this case exits with 1, while others die() that exits with 128.
* The placement of this misses the case where a merge of two unrelated
histories is attempted. You would need to also have a check at "No
common ancestors found. We need a real merge." part. The octopus
codepath could also end up with a fast forward or up-to-date.
^ permalink raw reply
* Re: (beginner) git rm
From: Zabre @ 2009-01-28 21:05 UTC (permalink / raw)
To: git
In-Reply-To: <1233175322729-2234796.post@n2.nabble.com>
Zabre wrote:
>
> Oh just one (probably stupid) thing : <tree-ish> does represent a
> directory being the root of a tree of folders (which has been added to the
> index), does it?
> This is the way I understand it at the moment. It must be a convention I
> don't know just yet. (I need to investigate on this)
>
Ok I found in the doc (in the "The Object Database" section) what trees are
about, seems a little bit more obscure, so I guess I'm not ready for this
right now. (I'm a beginner trying to use git for a simple regular workflow,
managing code and other documents)
http://www.kernel.org/pub/software/scm/git/docs/user-manual.html#the-object-database
I guess <tree-ish> refers to such an object, and you use it in order to
revert it from the Object Database.
I'll keep this idea in mind for when I'll try to restore data from tricky
last-chance "stores", there is a section about such things in the doc :
http://www.kernel.org/pub/software/scm/git/docs/user-manual.html#fixing-mistakes
I think that in a normal everyday use, the small index modification I can do
using the previous advices can do the job (or am I wrong?)
--
View this message in context: http://n2.nabble.com/%28beginner%29-git-rm-tp2231416p2234918.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCHv2 0/6] gitweb: feed metadata enhancements
From: Junio C Hamano @ 2009-01-28 20:58 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Jakub Narebski
In-Reply-To: <1232970616-21167-1-git-send-email-giuseppe.bilotta@gmail.com>
This is independent from your other topic on PATH_INFO, right? I am aware
that other one is being polished between you and Jakub, but is anobody
doing anything on this one, or is it already polished enough?
^ permalink raw reply
* [PATCH v2] Makefile: Make 'configure --with-expat=path' actually work
From: Serge van den Boom @ 2009-01-28 20:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vocxrqm57.fsf@gitster.siamese.dyndns.org>
While the configure script sets the EXPATDIR environment variable to
whatever value was passed to its option --with-expat as the prefix of
the location of the expat library and headers, the Makefile ignored it.
This patch fixes this bug.
Signed-off-by: Serge van den Boom <svdb@stack.nl>
---
On Wed, 28 Jan 2009, Junio C Hamano wrote:
> Serge van den Boom <svdb@stack.nl> writes:
> > The prefix specified with the --with-expat option of configure was not
> > actually used.
>
> I see configure.ac already has support for autodetection but I realized it
> only after running "git grep EXPATDIR". "Even though the configure script
> knows how to autodetect presence of the expat library and set EXPATDIR to
> the prefix of the location it was found, the Makefile ignored it and only
> honoured NO_EXPAT" might have been a better way to describe the breakage
> the patch fixes.
That's not entirely right, unless I'm missing something. The configure script
does not try to detect expat itself, though it passes on the argument
to --with-expat to the Makefile, via the EXPATDIR environment variable.
> If you look at the Makefile, you will notice a sequence of comments like
> this:
>
> # Define NO_CURL if you do not have libcurl installed. git-http-pull and
> # git-http-push are not built, and you cannot use http:// and https://
> # transports.
> #
> # Define CURLDIR=/foo/bar if your curl header and library files are in
> # /foo/bar/include and /foo/bar/lib directories.
> #
>
> Please add one for EXPATDIR, just after "Define NO_EXPAT if ...". People
> who do not run ./configure but add their own customizations in config.mak
> should benefit from your patch as well.
Ok.
diff --git a/Makefile b/Makefile
index 9d451cf..a7310f2 100644
--- a/Makefile
+++ b/Makefile
@@ -23,6 +23,9 @@ all::
# Define NO_EXPAT if you do not have expat installed. git-http-push is
# not built, and you cannot push using http:// and https:// transports.
#
+# Define EXPATDIR=/foo/bar if your expat header and library files are in
+# /foo/bar/include and /foo/bar/lib directories.
+#
# Define NO_D_INO_IN_DIRENT if you don't have d_ino in your struct dirent.
#
# Define NO_D_TYPE_IN_DIRENT if your platform defines DT_UNKNOWN but lacks
@@ -850,7 +853,12 @@ else
endif
endif
ifndef NO_EXPAT
- EXPAT_LIBEXPAT = -lexpat
+ ifdef EXPATDIR
+ BASIC_CFLAGS += -I$(EXPATDIR)/include
+ EXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib) $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat
+ else
+ EXPAT_LIBEXPAT = -lexpat
+ endif
endif
endif
^ permalink raw reply related
* Re: (beginner) git rm
From: Zabre @ 2009-01-28 20:42 UTC (permalink / raw)
To: git
In-Reply-To: <20090128201727.GD7503@atjola.homenet>
Björn Steinbrink wrote:
>
> The "git checkout -- d.txt" is also a valid command, but that restores
> the file from the index.
>
> git checkout -- paths
> ==> Copy "paths" from the index to the working tree
>
> git checkout <tree-ish> -- paths
> ==> Copy "paths" from the tree-ish to the index and working tree
>
> So, for "rm d.txt", a plain "git checkout -- d.txt" would also do the
> trick, as d.txt is still in the index. But your "git rm d.txt" also
> removed the file from the index, and thus that checkout does nothing.
> But "git checkout HEAD -- d.txt" works, as it gets the file from HEAD
> and puts it into the index and working tree.
>
This is enlightening, thank you very much!
(I knew I would love git more and more)
Oh just one (probably stupid) thing : <tree-ish> does represent a directory
being the root of a tree of folders (which has been added to the index),
does it?
This is the way I understand it at the moment. It must be a convention I
don't know just yet. (I need to investigate on this)
--
View this message in context: http://n2.nabble.com/%28beginner%29-git-rm-tp2231416p2234796.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCH/RFC v1 5/6] combine-diff.c: remove a call to fstat() inside show_patch_diff()
From: Junio C Hamano @ 2009-01-28 20:37 UTC (permalink / raw)
To: Kjetil Barvik; +Cc: git
In-Reply-To: <1233004637-15112-6-git-send-email-barvik@broadpark.no>
Kjetil Barvik <barvik@broadpark.no> writes:
> Currently inside show_patch_diff() we have and fstat() call after an
> ok lstat() call. Since we before the call to fstat() have already
> test for the link case with S_ISLNK() the fstat() can be removed.
Good eyes. Thanks.
^ permalink raw reply
* Re: [PATCH/RFC v1 2/6] remove some memcpy() and strchr() calls inside create_directories()
From: Junio C Hamano @ 2009-01-28 20:36 UTC (permalink / raw)
To: Kjetil Barvik; +Cc: git
In-Reply-To: <1233004637-15112-3-git-send-email-barvik@broadpark.no>
Kjetil Barvik <barvik@broadpark.no> writes:
> OK, maybe I instead should have tried to merge the function
> create_directories() with the safe_create_leading_directories() and
> *_const() functions? What do pepople think?
Strictly speaking, the safe_create_leading_* functions are meant to work
on paths inside $GIT_DIR and are not meant for paths inside the work tree,
which is this function is about. Their semantics are different with
respect to adjust_shared_perm().
Which means that you would either have two variants (one for work tree
paths, and another for paths inside $GIT_DIR), or a unified function that
has an argument to specify whether to run adjust_shared_perm().
HOWEVER.
That is only "strictly speaking".
A non-bare repository that is shared feels like an oximoron, but perhaps
there is a valid "shared work area" workflow that may benefit from such a
setup.
I see existing (mis)uses of the safe_create_leading_* in builtin-apply.c,
builtin-clone.c (the one that creates the work_tree, the other one is Ok),
merge-recursive.c (both call sites) that are used to touch the work tree,
but all places that create regular files in the work tree do not run
adjust_shared_perm() but simply honor the user's umask.
If we _were_ to support a "shared work area" workflow, having a unified
"create leading directory" function that always calls adjust_shared_perm()
may make sense (note that adjust_shared_perm() is a no-op in a non-shared
repository). We then need to also call adjust_shared_perm() for codepaths
that create regular files as well, though (e.g. write_entry() in entry.c,
but there are many others).
> diff --git a/entry.c b/entry.c
> index 05aa58d..c2404ea 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -2,15 +2,19 @@
> #include "blob.h"
> #include "dir.h"
>
> -static void create_directories(const char *path, const struct checkout *state)
> +static void
> +create_directories(int path_len, const char *path, const struct checkout *state)
Please do not split the line like this.
The existing sources are not laid out to allow "grep ^funcname(", nor are
we going to reformat all the files to support such a use case.
When we pass <string, length> pair to functions, I think we pass them in
the order I just wrote in all the other functions.
The micro-optimization itself makes sense to me, though.
^ permalink raw reply
* Re: [PATCH 2/3] Make has_commit non-static
From: Junio C Hamano @ 2009-01-28 20:36 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jake Goulding, git
In-Reply-To: <alpine.DEB.1.00.0901261637300.25749@intel-tinevez-2-302>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Mon, 26 Jan 2009, Jake Goulding wrote:
>
>> Moving has_commit from branch to a common location in preparation for
>> using it in tag. Renaming it to commit_has_any_in_commit_list to be more
>> unique.
>
> I feel like bike-shedding for a change, and I'd also like to prove that
> not all Germans like long names:
>
> is_ancestor_of_any()
I'll queue this after munging it to "is_descendant_of()".
Thanks.
^ permalink raw reply
* ??? Re: [PATCH/RFC v1 1/6] symlinks.c: small cleanup and optimisation
From: Junio C Hamano @ 2009-01-28 20:36 UTC (permalink / raw)
To: Kjetil Barvik; +Cc: git
In-Reply-To: <1233004637-15112-2-git-send-email-barvik@broadpark.no>
Kjetil Barvik <barvik@broadpark.no> writes:
> Simplify the if-else test in longest_match_lstat_cache() such that we
> only have one simple if test. Instead of testing for 'i == cache.len'
> or 'i == len', we transform this to a common test for 'i == max_len'.
>
> And to further optimise we use 'i >= max_len' instead of 'i ==
> max_len', the reason is that it is now the exact opposite of one part
> inside the while-loop termination expression 'i < max_len && name[i]
> == cache.path[i]', and then the compiler can hopefully/probably reuse
> a test-result from it.
This is *dense*, and made me wonder if is this really worth doing.
> + /*
> + * Is the cached path string a substring of 'name', is 'name'
> + * a substring of the cached path string, or is 'name' and the
> + * cached path string the exact same string?
> + */
> + if (i >= max_len && ((i < len && name[i] == '/') ||
> + (i < cache.len && cache.path[i] == '/') ||
> + (len == cache.len))) {
As you described in your commit log message, what this really wants to
check is (i == max_len). By saying (i >= max_len) you are losing
readability, optimizing for compilers (that do not notice this) and
pessimizing for human readers (like me, who had to spend a few dozens of
seconds to realize what you are doing, to speculate why you might have
thought this would be a good idea, and writing this paragraph). I do not
know if it is a good trade-off.
> @@ -91,7 +92,7 @@ static int lstat_cache(int len, const char *name,
> match_len = last_slash =
> longest_match_lstat_cache(len, name, &previous_slash);
> match_flags = cache.flags & track_flags & (FL_NOENT|FL_SYMLINK);
> - if (match_flags && match_len == cache.len)
> + if (match_flags && match_len >= cache.len)
> return match_flags;
> /*
> * If we now have match_len > 0, we would know that
Likewise.
> @@ -102,7 +103,7 @@ static int lstat_cache(int len, const char *name,
> * we can return immediately.
> */
> match_flags = track_flags & FL_DIR;
> - if (match_flags && len == match_len)
> + if (match_flags && match_len >= len)
> return match_flags;
> }
>
Likewise.
> @@ -184,7 +185,7 @@ void invalidate_lstat_cache(int len, const char *name)
> int match_len, previous_slash;
>
> match_len = longest_match_lstat_cache(len, name, &previous_slash);
> - if (len == match_len) {
> + if (match_len >= len) {
> if ((cache.track_flags & FL_DIR) && previous_slash > 0) {
> cache.path[previous_slash] = '\0';
> cache.len = previous_slash;
Likewise.
People who would want to fix potential bugs in this code 3 months down the
road may not have a ready access to your commit log message (they may be
looking at an extract from a release tarball, scratching their heads
wondering why you are not checking for equality). Perhaps the inline
function can have comments that says something like "when comparing the
return value from this function to see if it is equal to cache.len or len,
checking if the returned value is >= might help your compiler optimize it
better" to help the readers, but still, I do not know if it is a good
trade-off.
> @@ -153,7 +154,7 @@ static int lstat_cache(int len, const char *name,
> cache.path[last_slash] = '\0';
> cache.len = last_slash;
> cache.flags = save_flags;
> - } else if (track_flags & FL_DIR &&
> + } else if ((track_flags & FL_DIR) &&
> last_slash_dir > 0 && last_slash_dir <= PATH_MAX) {
> /*
> * We have a separate test for the directory case,
Good.
^ permalink raw reply
* Re: (beginner) git rm
From: Björn Steinbrink @ 2009-01-28 20:17 UTC (permalink / raw)
To: Zabre; +Cc: git
In-Reply-To: <1233166992184-2233892.post@n2.nabble.com>
On 2009.01.28 10:23:12 -0800, Zabre wrote:
> Tomas Carnecky wrote:
> >
> > Oops, sorry. git checkout HEAD -- d.txt
> > You have to tell which version of d.txt you want. In your case the
> > version in HEAD.
>
> Thank you for this precision, it makes me understand this command better.
> (Sorry for my late answer I've been unable to check my computer for a few
> hours)
The "git checkout -- d.txt" is also a valid command, but that restores
the file from the index.
git checkout -- paths
==> Copy "paths" from the index to the working tree
git checkout <tree-ish> -- paths
==> Copy "paths" from the tree-ish to the index and working tree
So, for "rm d.txt", a plain "git checkout -- d.txt" would also do the
trick, as d.txt is still in the index. But your "git rm d.txt" also
removed the file from the index, and thus that checkout does nothing.
But "git checkout HEAD -- d.txt" works, as it gets the file from HEAD
and puts it into the index and working tree.
Björn
^ permalink raw reply
* Re: [JGIT PATCH] Add getTaggerIdent, getShortMessage, getFullMessage to RevTag
From: Robin Rosenberg @ 2009-01-28 20:10 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <1233117316-3291-1-git-send-email-spearce@spearce.org>
onsdag 28 januari 2009 05:35:16 skrev Shawn O. Pearce:
> These methods make the RevTag API more like the RevCommit API, such
> that it is more consistent for applications to access the "fields"
> of a tag object in the same way that they access the fields of any
> commit object.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>
> Actually, this has also gone through Gerrit2. If you want to see
> what the same patch looks like (just for fun):
The reviewer is biased :) Where are the unit tests?
I'm kind of surprised you did not optimized skipping past the object header,
like the parents are skipped when parsing commits.
-- robin
^ permalink raw reply
* Re: [PATCH] Makefile: Make 'configure --with-expat=path' actually work
From: Junio C Hamano @ 2009-01-28 19:13 UTC (permalink / raw)
To: Serge van den Boom; +Cc: git
In-Reply-To: <alpine.BSF.2.00.0901251938120.97940@toad.stack.nl>
Serge van den Boom <svdb@stack.nl> writes:
> The prefix specified with the --with-expat option of configure was not
> actually used.
I see configure.ac already has support for autodetection but I realized it
only after running "git grep EXPATDIR". "Even though the configure script
knows how to autodetect presence of the expat library and set EXPATDIR to
the prefix of the location it was found, the Makefile ignored it and only
honoured NO_EXPAT" might have been a better way to describe the breakage
the patch fixes.
If you look at the Makefile, you will notice a sequence of comments like
this:
# Define NO_CURL if you do not have libcurl installed. git-http-pull and
# git-http-push are not built, and you cannot use http:// and https://
# transports.
#
# Define CURLDIR=/foo/bar if your curl header and library files are in
# /foo/bar/include and /foo/bar/lib directories.
#
Please add one for EXPATDIR, just after "Define NO_EXPAT if ...". People
who do not run ./configure but add their own customizations in config.mak
should benefit from your patch as well.
You might want to add a logic to drop NO_EXPAT when EXPATDIR is specified
to the Makefile as well, but I didn't check.
Please do *not* send a patch in 'text/plain; format="flowed"' content-type.
You will get a whitespace mangled patch that I have to fix up by hand.
Thanks.
> Signed-off-by: Serge van den Boom <svdb@stack.nl>
> ---
> diff --git a/Makefile b/Makefile
> index b4d9cb4..e7218cb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -849,7 +849,12 @@ else
> endif
> endif
> ifndef NO_EXPAT
> - EXPAT_LIBEXPAT = -lexpat
> + ifdef EXPATDIR
> + BASIC_CFLAGS += -I$(EXPATDIR)/include
> + EXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib) $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat
> + else
> + EXPAT_LIBEXPAT = -lexpat
> + endif
> endif
> endif
^ permalink raw reply
* Re: Bad objects error since upgrading GitHub servers to 1.6.1
From: Junio C Hamano @ 2009-01-28 19:00 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Shawn O. Pearce, PJ Hyett, Johannes Schindelin, git
In-Reply-To: <alpine.LFD.2.00.0901280738430.3123@localhost.localdomain>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Tue, 27 Jan 2009, Junio C Hamano wrote:
>>
>> - When digging deeper into the ancestry chain of a commit that is already
>> painted as UNINTERESTING, in order to paint its parents UNINTERESTING,
>> we barfed if parse_parent() for a parent commit object failed. We can
>> ignore such a parent commit object.
>
> Wouldn't it be better to still mark it UNINTERESTING too?
>
>> @@ -480,7 +483,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
>> struct commit *p = parent->item;
>> parent = parent->next;
>> if (parse_commit(p) < 0)
>> - return -1;
>> + continue;
>> p->object.flags |= UNINTERESTING;
>> if (p->parents)
>> mark_parents_uninteresting(p);
>
> IOW, move that
>
> p->object.flags |= UNINTERESTING;
>
> to before parse_commit(). That's assuming 'parent' is never NULL, of
> course.
Ok, makes sense. Will do.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox