* Re: git status: small difference between stating whole repository and small subdirectory
From: Nguyen Thai Ngoc Duy @ 2012-02-22 12:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <7v7gzfefw1.fsf@alter.siamese.dyndns.org>
On Wed, Feb 22, 2012 at 9:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> That makes me think if "diff --cached" can take advantage of
>> cache-tree to avoid walking down valid cached trees and do tree-tree
>> diff in those cases instead. Not sure if it gains us anything but code
>> complexity.
>
> Why do I have this funny feeling that we saw that comment in this thread
> already?
Simple. You wrote it and I missed it.
On Sat, Feb 18, 2012 at 5:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> That being said, we do have an index extension to store the tree sha1 of
>> whole directories (i.e., we populate it when we write a whole tree or
>> subtree into the index from the object db, and it becomes invalidated
>> when a file becomes modified). This optimization is used by things like
>> "git commit" to avoid having to recreate the same sub-trees over and
>> over when creating tree objects from the index. But we could also use it
>> here to avoid having to even read the sub-tree objects from the object
>> db.
>
> Like b65982b (Optimize "diff-index --cached" using cache-tree, 2009-05-20)
> perhaps?
This optimizes the case when a cached tree matches entirely.I wonder
whether it's faster if we switch to tree-tree diff whenever we find
valid cached trees. If cache-tree is fully valid, "git diff --cached
foo" would be equivalent to "git diff HEAD foo".
I tried "git diff --raw HEAD HEAD~100" (where HEAD was
v3.1-rc1-272-g73e0881 on linux-2.6) and "git diff --cached --raw
HEAD~100" with no cache-tree. The former is a little bit faster than
the latter (177ms vs 275ms). On gentoo-x86, 70k worktree files, it's
4.33s vs 4.45s. But in tree-tree diff we pay high in cold cache case
for loading trees from "HEAD". So no, probably not worth more code
changes. Your optimization is good enough.
--
Duy
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Thomas Rast @ 2012-02-22 13:17 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Junio C Hamano, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <CACsJy8BZyokLSpbA=C6YEO1JajyemT9vLG=C9W4E4coA5OfszQ@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On Sat, Feb 18, 2012 at 5:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> That being said, we do have an index extension to store the tree sha1 of
>>> whole directories (i.e., we populate it when we write a whole tree or
>>> subtree into the index from the object db, and it becomes invalidated
>>> when a file becomes modified). This optimization is used by things like
>>> "git commit" to avoid having to recreate the same sub-trees over and
>>> over when creating tree objects from the index. But we could also use it
>>> here to avoid having to even read the sub-tree objects from the object
>>> db.
>>
>> Like b65982b (Optimize "diff-index --cached" using cache-tree, 2009-05-20)
>> perhaps?
>
> This optimizes the case when a cached tree matches entirely.I wonder
> whether it's faster if we switch to tree-tree diff whenever we find
> valid cached trees. If cache-tree is fully valid, "git diff --cached
> foo" would be equivalent to "git diff HEAD foo".
Not necessarily; the cache-tree is valid if it faithfully represents
what is in the index. It does not have any direct relation to HEAD.
> I tried "git diff --raw HEAD HEAD~100" (where HEAD was
> v3.1-rc1-272-g73e0881 on linux-2.6) and "git diff --cached --raw
> HEAD~100" with no cache-tree. The former is a little bit faster than
> the latter (177ms vs 275ms). On gentoo-x86, 70k worktree files, it's
> 4.33s vs 4.45s. But in tree-tree diff we pay high in cold cache case
> for loading trees from "HEAD". So no, probably not worth more code
> changes. Your optimization is good enough.
I'm still wondering about using mincore() to good effect. I tried it
for git-grep, but it ended up slowing things down. However, it irks me
that in some cases a clueful use of one form over the other can really
make a huge performance difference, e.g.,
git grep stuff
git grep HEAD stuff
If I am in a big repository that I haven't used in a while, the HEAD
form will be much faster as the worktree search would fault many files.
OTOH if I am in a heavily-used repository (and perhaps just said 'make'
minutes ago) the worktree version will avoid the pack decompression
effort.
Sadly this also has the problem that we must first determine whether
substituting HEAD for the worktree (or vice versa) is valid at all. For
grep perhaps there could be a "just do a fast search somewhere" option
since usually you are looking for something that hasn't changed in ages.
Ok, that was almost completely beside the point of this thread.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* [PATCH] Improve http proxy support
From: Nelson Benítez León @ 2012-02-22 14:04 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 884 bytes --]
Hi, my initial motivation for this patch was to add NTLM proxy
authentication so I could 'git clone' from inside my employers
network, but apart from doing that, I also added two more features,
so, this patch adds the following improvements to the http proxy
support:
- Support NTLM proxy authentication (as well as other authentication
methods) by setting CURLOPT_PROXYAUTH[1] to CURLAUTH_ANY.
- Look up environment vars http_proxy and HTTP_PROXY in case git
http.proxy config option is not set. This supports system wide proxy
support in terminals.
- Support proxy urls with username but without a password, in which
case we interactively ask for the password (as it's already done in
http auth code). This makes possible to not have the password written
down in git config files or in env vars.
Thanks!
[1] http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTPROXYAUTH
[-- Attachment #2: gitproxyauthv2.patch --]
[-- Type: application/octet-stream, Size: 1701 bytes --]
--- http.c 2012-01-19 01:19:22.000000000 +0100
+++ http.mod2.c 2012-02-22 14:44:11.727773038 +0100
@@ -299,8 +299,54 @@
if (curl_ftp_no_epsv)
curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
- if (curl_http_proxy)
- curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+ if (!curl_http_proxy) {
+ const char *env_proxy;
+ env_proxy = getenv("HTTP_PROXY");
+ if (!env_proxy) {
+ env_proxy = getenv("http_proxy");
+ }
+ if (env_proxy) {
+ curl_http_proxy = xstrdup(env_proxy);
+ }
+ }
+ if (curl_http_proxy) {
+ char *at, *colon, *proxyuser;
+ const char *cp;
+ cp = strstr(curl_http_proxy, "://");
+ if (cp == NULL) {
+ cp = curl_http_proxy;
+ } else {
+ cp += 3;
+ }
+ at = strchr(cp, '@');
+ colon = strchr(cp, ':');
+ if (at && (!colon || at < colon)) {
+ /* proxy string has username but no password, ask for password */
+ char *ask_str, *proxyuser, *proxypass;
+ int len;
+ struct strbuf pbuf = STRBUF_INIT;
+ len = at - cp;
+ proxyuser = xmalloc(len + 1);
+ memcpy(proxyuser, cp, len);
+ proxyuser[len] = '\0';
+
+ strbuf_addf(&pbuf, "Enter password for proxy %s...", at+1);
+ ask_str = strbuf_detach(&pbuf, NULL);
+ proxypass = xstrdup(git_getpass(ask_str));
+
+ strbuf_insert(&pbuf, 0, curl_http_proxy, cp - curl_http_proxy);
+ strbuf_addf(&pbuf, "%s:%s", proxyuser, proxypass);
+ strbuf_add(&pbuf, at, strlen(at));
+ curl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&pbuf, NULL));
+
+ free(ask_str);
+ free(proxyuser);
+ free(proxypass);
+ } else {
+ curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+ }
+ curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+ }
return result;
}
^ permalink raw reply
* Re: [PATCH] Improve http proxy support
From: Matthieu Moy @ 2012-02-22 14:13 UTC (permalink / raw)
To: Nelson Benítez León; +Cc: git
In-Reply-To: <CAAUd640GaLz4TGs_Lz6KbSFK0VcEVxGfO6PpSCdhch+fYwVovw@mail.gmail.com>
Nelson Benítez León <nbenitezl@gmail.com> writes:
> Hi, my initial motivation for this patch was to add NTLM proxy
> authentication [...]
That sounds interesting, but please read Documentation/SubmittingPatches
in Git's tree. The formatting of your email is wrong (giving more work
for your maintainer) and you need to sign-off your patch to allow your
code to be legally included.
Thanks,
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Ambiguous reference weirdness
From: Phil Hord @ 2012-02-22 14:48 UTC (permalink / raw)
To: Jeff King; +Cc: git, Ramkumar Ramachandra, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20120222070034.GA17015@sigill.intra.peff.net>
On Wed, Feb 22, 2012 at 2:00 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 21, 2012 at 08:46:24PM -0500, Phil Hord wrote:
>
>> I accidentally ran into this today:
>> $ git cherry-pick 1147
>> fatal: BUG: expected exactly one commit from walk
>>
>> git log shows no output:
>> $ git log 1147
>
> What is 1147? Is it supposed to be a partial sha1, or is it a ref you
> have?
1147 was a typo. It was a Gerritt changeset ID I forgot to expand.
When I type "git cherry-pick 1147<TAB>", autocompletion expands this
for me to "git cherry-pick origin/changes/47/1147/1". Except in this
case I misfired the TAB and got the weird "BUG:" report. But I didn't
see the same problem with other invalid refs, so I went searching for
the variants and to see what caused it.
> Have you looked at the object that it resolves to? I suspect it is the
> partial sha1 of a non-commit object. E.g.:
All of these examples were run in current git.git, so you can try them
yourself if needed. But I did figure out that 1147 resolves to a
blob, and that's apparently the difference between these three:
$ git cherry-pick 1147
fatal: BUG: expected exactly one commit from walk
$ git cherry-pick 1146
error: short SHA1 1146 is ambiguous.
error: short SHA1 1146 is ambiguous.
fatal: ambiguous argument '1146': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions
$ git cherry-pick 114333
fatal: ambiguous argument '114333': unknown revision or path not in
the working tree.
Use '--' to separate paths from revisions
I consider the first two responses to be UI bugs. The second one is
minor (the twice-reported error message), and the first one is pretty
rude. I would expect all three to report the same conclusion,
"fatal: ambiguous argument 'XXXXX': unknown revision or path not in
the working tree." But the first one doesn't.
> $ git cat-file -t HEAD^{tree}
> tree
> $ git cherry-pick HEAD^{tree}
> fatal: BUG: expected exactly one commit from walk
> $ git log HEAD^{tree} | wc -l
> 0
Thanks. I'm not familiar with the {tree} syntax -- in fact I'd like
to find a dictionary for all the reference spelling variants -- but
this is elucidating.
> In the cherry-pick case, the code is checking the right thing, but the
> message is horrible. It is not a bug, but merely unexpected input, and
> it should provide a usage message.
Bug is too strong a word in one sense, but from the user perspective I
consider this "horrible message" a bug.
> In the log case, we totally ignore any pending non-revision arguments.
> So it is correct to produce no output (there is nothing to show, which
> is not unusual in itself; many queries end up producing empty output).
> But we should probably notice that there are pending objects left over
> and produce some kind of diagnostic.
I think you're right about this. I tried log as an afterthought and
was surprised by the varying results.
> I've reordered some of your example commands below to fit the flow of my
> explanation better.
>
>> $git log 114
>> fatal: ambiguous argument '114': unknown revision or path not in the
>> working tree.
>> Use '--' to separate paths from revisions
>
> Right. I think we require at least 4 characters in a partial sha1, so we
> don't treat that as a possibility. So we are left guessing whether you
> mean to do:
>
> git log 114 --
>
> or
>
> git log HEAD -- 114
>
> since it exists as neither a revision nor a path, and the error message
> reflects that (the first one is an error, as there is no such revision.
> The second is a correct query, though one that does not produce any
> results).
>
>> $ git checkout 114
>> error: pathspec '114' did not match any file(s) known to git.
>
> I think checkout has the same "is this a path or a revision" ambiguity
> to resolve. But rather than be explicit that you might have meant "114"
> as a tree, the error message assumes you meant a path. That might be
> worth improving, similar to the above example.
>
> Again, you can disambiguate with:
>
> $ git checkout -- 1147
> error: pathspec '1147' did not match any file(s) known to git.
>
> $ git checkout 1147 --
> fatal: reference is not a tree: 1147
>
>> $ git checkout 1147
>> fatal: reference is not a tree: 1147
Yes, I understand this. This was a typo and it was ambiguous. But
shouldn't we tell the user the same thing when encountering the same
failure?
> In this case the name does resolve to an object, so we try to use it as
> such (even though we later find that it is useless for the operation).
> We _could_ realize that it is not a tree and disambiguate to:
>
> $ git checkout -- 1147
>
> but the current rule is at least consistent and simple.
>
>> $ git checkout 1146
>> error: short SHA1 1146 is ambiguous.
>> error: pathspec '1146' did not match any file(s) known to git.
>
> The sha1 is ambiguous, and therefore it does not resolve to anything. So
> you get the same case as "git checkout 1147", but with the extra
> ambiguity warning.
This one I like. I only included it to demonstrate the different
error messages git produces.
>> $ git merge 114
>> fatal: '114' does not point to a commit
>
> It might be nice for this error message to be split into two cases:
>
> 1. the name does not resolve _at all_ (i.e., you made a typo)
>
> 2. the name does resolve to something, but it is not a commit
>
> In the latter case, we actually do get an extra error message from
> elsewhere in the code:
>
>> $ git merge 1147
>> error: 1147: expected commit type, but the object dereferences to blob type
>> fatal: '1147' does not point to a commit
>
> But in case 1, it's not clear which is which (maybe even rewording it as
> "114 cannot be resolved as a commit" would be less confusing).
>
>> $ git cherry-pick 114
>> fatal: ambiguous argument '114': unknown revision or path not in the
>> working tree.
>> Use '--' to separate paths from revisions
>> [...]
>> $ git cherry-pick 1147
>> fatal: BUG: expected exactly one commit from walk
>
> This is the "does not resolve" versus "is not actually a commit". In the
> first case, though, I wonder if the error message is accurate. I'm not
> sure if you can do "git cherry-pick <rev> -- <paths>", so the error
> message is misleading
Good point. I missed that one completely.
> (if anything, I would expect it to limit the
> revision walk, but trying "git cherry-pick HEAD -- 114" seems to still
> complain about the absence of 114).
>
>> [more examples]
>
> These are all variants that hopefully make sense in light of the
> explanations above.
>
>> I can understand some of the inconsistent error reporting (checkout
>> may expect filenames, but cherry-pick and merge do not). But this
>> seems too varied to me.
>>
>> And the first two look like bugs.
>>
>> Any comments or suggestions?
>
> I think the outcomes are all working as intended, but the error messages
> could stand to be improved.
Yes, I agree. I only meant to complain about the error messages, not
the results. Thanks for the discussion. I'll try to look for where
these come from and see if they can be improved within reason.
Phil
^ permalink raw reply
* Re: [PATCH] remote-curl: Fix push status report when all branches fail
From: Shawn Pearce @ 2012-02-22 15:22 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20120222101302.GA11606@sigill.intra.peff.net>
On Wed, Feb 22, 2012 at 02:13, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 20, 2012 at 09:03:31AM -0800, Shawn O. Pearce wrote:
> This hunk is causing intermittent failures of t5541 for me, especially
> when the system is under heavy load (e.g., make -j32 test).
...
> @@ -220,15 +221,21 @@ static struct child_process *get_helper(struct transport *transport)
> static int disconnect_helper(struct transport *transport)
> {
> struct helper_data *data = transport->data;
> - struct strbuf buf = STRBUF_INIT;
> int res = 0;
>
> if (data->helper) {
> if (debug)
> fprintf(stderr, "Debug: Disconnecting.\n");
> if (!data->no_disconnect_req) {
> - strbuf_addf(&buf, "\n");
> - sendline(data, &buf);
> + /*
> + * Ignore write errors; there's nothing we can do,
> + * since we're about to close the pipe anyway. And the
> + * most likely error is EPIPE due to the helper dying
> + * to report an error itself.
> + */
> + sigchain_push(SIGPIPE, SIG_IGN);
> + xwrite(data->helper->in, "\n", 1);
> + sigchain_pop(SIGPIPE);
> }
> close(data->helper->in);
> close(data->helper->out);
>
> which makes the t5541 failures go away for me. What do you think?
This sounds right to me. Its unfortunate that we missed the error
status output when we built the remote helper protocol, but your patch
above might be the best we can do now.
Eh, well, actually we could have the helper advertise a new capability
that can be enabled to return exit status. That is a much bigger
change, and even if we do it for remote-curl (since that is in tree
and easy to update) we still need your patch for the same race
condition for out of tree helpers (which Google actually has so I care
about out of tree helpers too).
^ permalink raw reply
* Re: how do you review auto-resolved files
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-22 15:24 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Neal Kreitzinger
In-Reply-To: <7vhayjga0a.fsf@alter.siamese.dyndns.org>
On 02/21/2012 10:19 PM, Junio C Hamano wrote:
> Imagine that you are the maintainer of the mainline and are reviewing the
> work made on a side branch that you just merged, but pretend that the
> contribution came as a patch instead. How would you assess the damage to
> your mainline?
>
> You would use "git show --first-parent $commit" for that.
Hi,
it seems that git show --first-parent is not documented in the man page.
This option is only documented for rev-list and log. I think that
- this example should land in Examples in git-show.txt
- --first-parent should be documented in git-show.txt because it (at
least to me, but I guess that for other people also) it isn't
immediately obvious that it means to _diff_ with the first parent.
--
Zbyszek
^ permalink raw reply
* Re: git-svn show-externals and svn version
From: Nikolaus Demmel @ 2012-02-22 15:27 UTC (permalink / raw)
To: git
In-Reply-To: <994D6101-DD43-4CD9-BB96-34807E3087C4@nikolaus-demmel.de>
Hi,
I feel a bit like I am talking to myself, but I see from the high
traffic on this list that people are busy doing great things :-). I will
write anyway in case someone interested in git-svn listens.
Is there an appropriate place to file these kinds of feature/enhancement
requests?
So I've investigated the matter a bit further. Turns out in the
subversion SWIG language bindings there is an API function that parses
svn:externals definitions for you. See [1] for a recent (minimal) change
to make this function available in python. I guess supporting Perl
requires equally minimal changes. I haven't attempted it myself since I
know neither Perl nor SWIG.
How could this be used in git-svn show-externals? As layed out before, I
believe that the current output for the svn1.5 syntax is inherently
broken and we should not worry about backwards compatibility for
that. To maintain backwards compatibility with the output for the old
format and to give a canonical, easy to parse, output for any external
definition, I suggest sticking to the current format, just inserting the
parsed definition at the appropriate place with relative URLs completely
resolved to absolute ones.
The pre-svn1.5 syntax for external definitions was:
LOCAL-PATH [-r REVISION] ABSOLUTE-URL
The output for show-externals was thus (note that there is no parsing of
the external definition going on yet):
DIRECTORY-PREFIX/LOCAL-PATH [-rREVISION] ABSOLUTE-URL
The DIRECTORY-PREFIX was added because show-externals shows the external
definitions for all subdirectories recursively. With this prefix, every
line can be processed on its own. I suggest extending this output to:
DIRECTORY-PREFIX/LOCAL-PATH [-rREVISION] ABSOLUTE-URL[@PEG-REV]
Again, as mentioned above, show-externals should parse the definitions
and resolve relative URLs. Any lines that the svn API call cannot parse
should be completely ommited (e.g. commented lines and empty lines).
As I understand it show-externals is intended primarily for scripts for
further processing. With this extension existing scripts for the old
syntax should keep working also long as they don't feature
peg-revisions. With relative URLs resolved and a standard ordering old
and new syntax cannot be distinguished in terms of show-externals output
(except when there are peg-revsion are there).
Thoughts, comments, opinions?
Cheers,
Nikolaus
[1] http://thread.gmane.org/gmane.comp.version-control.subversion.user/109033/focus=109036
Am 21.02.2012 um 12:14 schrieb Nikolaus Demmel:
> Hi,
>
> as a followup just another example of when the current show-externals gives a flaky output, namely when the line in the external definition is commented.
>
>
> $ git svn show-externals
> [...]
> # /src/
> /src/#https://codex.cs.bham.ac.uk/svn/nah/cogx/code/subarchitectures/vision.sa/tags/matlab-cosy-2008 matlab_cosy_2008
> /src/#https://codex.cs.bham.ac.uk/svn/nah/cogx/code/subarchitectures/vision.sa/tags/matlab-review-2009 matlab_review_2009
>
>
> Regards,
> Nikolaus
>
>
> Am 19.02.2012 um 19:53 schrieb Nikolaus Demmel:
>
>> Hi,
>>
>> I am currently investigating getting support for svn externals in git-svn (you might have noticed my other mails).
>>
>> It turns out that there are quite a few scripts floating around that use the output of show-externals and then try to pull these externals with git-svn into independent repositories and add the folders as submodules to the root repository.
>>
>> However, none of them work for me, and the primary reason AFAICT is that they were written for the pre svn1.5 format of svn:externals. From 1.5 svn supports a new format of svn:externals, which changes the order of revision, repository-url, and local folder, and also adds the posibility to add relative urls, peg-revisions, etc [1].
>>
>> On top that it seems to me that the output of show-externals was purely designed for the old format. For example, if you compare the output of "git svn show-externals" with "git svn propget svn:externals" in an example repository using the new format [2], you find that the in the show-externals output the prepended "/" and "/instantiations/" at the beginning of each line does not make sense. If the target url (all relative with the ^ syntax in this case) and the sub-folder were swapped in order, as of pre 1.5 svn, it would make much more sense. Also apparently the code for show-externals was added in 2007 and not changed since, whereas svn 1.5 was released in 2008.
>>
>> What I am not completely clear about is, whether svn 1.5 and later enforces the new syntax, or whether it just adds the new syntax and still has to support the old syntax (which could be distinguished, I guess, by checking of the last part on an entry is an absolute URL instead of a subfolder). Also, I'm not sure if the format depends on the version of the svn-server or the client. I would assume you can check out a repository hosted with svn 1.4 with a 1.5 client. Does the client process the svn:externals and present it in the new format, or is this the text string just taken from the server unaltered (I have not much knowledge of how svn actually works internally)?
>>
>> Another question is whether the perl svn bindings present the svn:externals in some parsed, standard format, or do they just give you the raw text string?
>>
>> In order to make show-externals more useful with the svn 1.5 and later syntax, one would maybe need to check the underlying svn version. I guess it is also quite important to retain backwards compatibility, such that users of externals with the old syntax would still get the same output as before.
>>
>> I would suggest that the show-externals output should be as close as possible to the svn:externals syntax, possibly adapting the subfolder path for nested folders. However here the recursive display of externals for subfolders becomes a bit more tricky, since the URL can also be relative to the subfolder as of the new syntax. Maybe the easiest way to deal with the new syntax in show-externals would be to have each line like it is in the svn-properties, but add a space separated relative path to the corresponding subfolder at the beginning. A tool that uses this is then responsible for making sure the relative URLs are resolved correctly.
>>
>> To sum up, given that all the questions I have are answered like I think is most likely, it would boil down to changing the output of show-externals for svn 1.5 and later just slightly, namely by inserting an additional space between the prepended subfolder and the actual svn:externals definition in each line.
>>
>> Any thoughts and/or answers?
>>
>> Cheers,
>> Nikolaus
>>
>>
>> [1] http://svnbook.red-bean.com/en/1.7/svn.advanced.externals.html
>> [2] http://paste.lisp.org/display/127858--
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [FYI] very large text files and their problems.
From: Ian Kumlien @ 2012-02-22 15:49 UTC (permalink / raw)
To: git
Hi,
We just saw a interesting issue, git compressed a ~3.4 gb project to ~57
mb. But when we tried to clone it on a big machine we got:
fatal: Out of memory, malloc failed (tried to allocate
18446744072724798634 bytes)
This is already fixed in the 1.7.10 mainline - but it also seems like
git needs to have atleast the same ammount of memory as the largest
file free... Couldn't this be worked around?
On a (32 bit) machine with 4GB memory - results in:
fatal: Out of memory, malloc failed (tried to allocate 3310214313 bytes)
(and i see how this could be a problem, but couldn't it be mitigated? or
is it bydesign and intended behaviour?)
I'm not subscribed to please keep me in CC.
/Ian Kumlien
^ permalink raw reply
* Re: [FYI] very large text files and their problems.
From: Nguyen Thai Ngoc Duy @ 2012-02-22 16:18 UTC (permalink / raw)
To: Ian Kumlien; +Cc: git
In-Reply-To: <20120222154926.GC11202@pomac.netswarm.net>
On Wed, Feb 22, 2012 at 10:49 PM, Ian Kumlien <pomac@vapor.com> wrote:
> Hi,
>
> We just saw a interesting issue, git compressed a ~3.4 gb project to ~57 mb.
How big are those files? How many of them? How often do they change?
> But when we tried to clone it on a big machine we got:
>
> fatal: Out of memory, malloc failed (tried to allocate
> 18446744072724798634 bytes)
>
> This is already fixed in the 1.7.10 mainline - but it also seems like
Does 1.7.9 have this problem?
> git needs to have atleast the same ammount of memory as the largest
> file free... Couldn't this be worked around?
>
> On a (32 bit) machine with 4GB memory - results in:
> fatal: Out of memory, malloc failed (tried to allocate 3310214313 bytes)
>
> (and i see how this could be a problem, but couldn't it be mitigated? or
> is it bydesign and intended behaviour?)
I think that it's delta resolving that hogs all your memory. If your
files are smaller than 512M, try lower core.bigFileThreshold. The
topic jc/split-blob, which stores a big file are several smaller
pieces, might solve your problem. Unfortunately the topic is not
complete yet.
--
Duy
^ permalink raw reply
* Re: [PATCH] Improve http proxy support
From: Thomas Rast @ 2012-02-22 16:43 UTC (permalink / raw)
To: Nelson Benítez León; +Cc: Matthieu Moy, git
In-Reply-To: <vpqd397x8fc.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Nelson Benítez León <nbenitezl@gmail.com> writes:
>
>> Hi, my initial motivation for this patch was to add NTLM proxy
>> authentication [...]
>
> That sounds interesting, but please read Documentation/SubmittingPatches
> in Git's tree. The formatting of your email is wrong (giving more work
> for your maintainer) and you need to sign-off your patch to allow your
> code to be legally included.
Judging from the message, it also conflates three changes into one
patch. Don't do that.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Problems with unrecognized headers in git bundles
From: Jannis Pohlmann @ 2012-02-22 16:05 UTC (permalink / raw)
To: git
Hi,
creating bundles from some repositories seems to lead to bundles with
incorrectly formatted headers, at least with git >= 1.7.2. When cloning
from such bundles, git prints the following error/warning:
$ git clone perl-clone.bundle perl-clone
Cloning into 'perl-clone'...
warning: unrecognized header: --work around mangled archname on...
This can be reproduced easily with git from any version >= 1.7.2 or from
master, using the following steps:
git clone git://perl5.git.perl.org/perl.git perl
GIT_DIR=perl/.git git bundle create perl-clone.bundle --all
git clone perl-clone.bundle perl-clone
The content of the bundle is:
# v2 git bundle
-- work around mangled archname on win32 while finding...
39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/heads/blead
39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/remotes/origin/HEAD
...
The "--work around mangled archname..." line is rather long, so I've
omitted most of it. What it contains is a series of commit messages all
combined into a single line. It appears that this line is the problem
because it's neither a comment (like '#v2 git bundle') nor a SHA1
followed by a ref name.
Note that this problem does not occur with all repositories. A bundle
created from a test repository with a single text file and just one
commit does not have this problem.
Note also that "git clone <bundle> <dest>" does not fail with corrupted
bundles if the git version is something like 1.7.2 or 1.7.7.6. Those
version print the above warning but still succeed in cloning. "git
clone" from master however treats this as an error and fails.
Without any knowledge of how git works internally, I would assume that
this is either a bug in how bundles are created, or a hint at a slightly
broken perl repository (other's have the same thing though, perhaps it
is because of a conversion from another SCM software to git in the
past). Does this sound reasonable?
Is there a way to work around this or fix it properly? I'm not sure what
lead to the decision to no longer ignore unrecognized headers in git
master, would it be sensible to revert this change if nothing can be
done to solve this during bundle creation?
- Jannis
^ permalink raw reply
* Re: [PATCH 4/5] xdiff: introduce XDF_IGNORE_CASE
From: Jakub Narebski @ 2012-02-22 18:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1329704188-9955-5-git-send-email-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> Teach the hash function and per-line comparison logic to compare lines
> while ignoring the differences in case. It is not an ignore-whitespace
> option but still needs to trigger the inexact match logic, and that is
> why the previous step introduced XDF_INEXACT_MATCH mask.
Nb. how it compares with ignore case in filesystem paths?
> Assign the 7th bit for this option, and move the bits to select diff
> algorithms out of the way in order to leave room for a few bits to add
> more variants of ignore-whitespace, such as --ignore-tab-expansion, if
> somebody else is inclined to do so later.
Or do a proper Unicode sorting / collation algorithm, with different
levels
(4.3 Form a sort key for each string, UTS #10.):
Level 1: alphabetic ordering
Level 2: diacritic ordering
Level 3: case ordering
Level 4: tie-breaking (e.g. in the case when variable is 'shifted')
> We would still need to teach the front-end to flip this bit, for this
> change to be any useful.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> +static inline int match_a_byte(char ch1, char ch2, long flags)
> +{
> + if (ch1 == ch2)
> + return 1;
> + if (!(flags & XDF_IGNORE_CASE) || ((ch1 | ch2) & 0x80))
> + return 0;
> + if (isupper(ch1))
> + ch1 = tolower(ch1);
> + if (isupper(ch2))
> + ch2 = tolower(ch2);
> + return (ch1 == ch2);
> +}
<del>
Wouldn't a better solution be a collate algorithm rather than changing
a sorting function? Or is it a performance hack on typical body of
text under version control (mainly lowercase)?
</del>
"(libc.info)Collation Fuctions" says:
The functions `strcoll' and `wcscoll' perform this translation
implicitly, in order to do one comparison. By contrast, `strxfrm' and
`wcsxfrm' perform the mapping explicitly. If you are making multiple
comparisons using the same string or set of strings, it is likely to be
more efficient to use `strxfrm' or `wcsxfrm' to transform all the
strings just once, and subsequently compare the transformed strings
with `strcmp' or `wcscmp'.
The function match_a_byte (memcoll?) defined here is similar to strcoll;
do we compare single line with more than one other line?
> +static inline unsigned long hash_a_byte(const char ch_, long flags)
> +{
> + unsigned long ch = ch_ & 0xFF;
> + if ((flags & XDF_IGNORE_CASE) && !(ch & 0x80) && isupper(ch))
> + ch = tolower(ch);
> + return ch;
> +}
> +
Hmmm... hash_a_byte (memxfrm?) is similar to strxfrm, so you do use
one or the other...
--
Jakub Narebski
^ permalink raw reply
* Re: [FYI] very large text files and their problems.
From: Ian Kumlien @ 2012-02-22 18:39 UTC (permalink / raw)
To: git; +Cc: pclouds
[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]
Seems like i ruined my dovecot config in a recent upgrade - which also
affected my mail... =/
Anyway, it's all fixed now.
from: Nguyen Thai Ngoc Duy <pclouds () gmail ! com>
> On Wed, Feb 22, 2012 at 10:49 PM, Ian Kumlien <pomac@vapor.com> wrote:
> > Hi,
> >
> > We just saw a interesting issue, git compressed a ~3.4 gb project to
> ~57 mb.
>
> How big are those files? How many of them? How often do they change?
This is the initial check in, one of the files is a 3.3 gb text file.
> > But when we tried to clone it on a big machine we got:
> >
> > fatal: Out of memory, malloc failed (tried to allocate
> > 18446744072724798634 bytes)
> >
> > This is already fixed in the 1.7.10 mainline - but it also seems
> like
>
> Does 1.7.9 have this problem?
I've tested with 1.7.9.1, haven't downgraded to test with 1.7.9...
> > git needs to have atleast the same ammount of memory as the largest
> > file free... Couldn't this be worked around?
> >
> > On a (32 bit) machine with 4GB memory - results in:
> > fatal: Out of memory, malloc failed (tried to allocate 3310214313
> bytes)
> >
> > (and i see how this could be a problem, but couldn't it be
> mitigated? or
> > is it bydesign and intended behaviour?)
>
> I think that it's delta resolving that hogs all your memory. If your
> files are smaller than 512M, try lower core.bigFileThreshold. The
> topic jc/split-blob, which stores a big file are several smaller
> pieces, might solve your problem. Unfortunately the topic is not
> complete yet.
the problem here is that there is one file that is exactly: 3310214313
bytes, so it should all be one "blob".
split-blob would be really interesting for several reasons though =)
> --
> Duy
> --
--
Ian Kumlien -- http://demius.net || http://pomac.netswarm.net
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCHv4] git-p4: add initial support for RCS keywords
From: Junio C Hamano @ 2012-02-22 19:29 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Eric Scouten, Pete Wyckoff
In-Reply-To: <20120222125327.GA2292@padd.com>
Pete Wyckoff <pw@padd.com> writes:
>> Improved-by: Pete Wyckoff <pw@padd.com>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>
> Looks brilliant. Ack. Thanks for suffering through N rounds of
> review. :)
Well, I hate to say that I need to ask another round, to redo this patch
on top of ld/git-p4-expanded-keywords topic that has already been in
'next'; a patch that replaces what is in 'next' will lose fix-ups for
issues I pointed out in the first round that you forgot to follow and were
fixed up locally by me when I queued the existing one.
When working on an improvement to what you have sent out, please make it a
habit of comparing your result with what are already queued, even when the
earlier patches are still in 'pu'. They often are polished with trivial
improvements (both to the patch and the log message) based on review
comments from people when they are queued, which you do not want to lose.
Thanks.
^ permalink raw reply
* [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Thomas Rast @ 2012-02-22 19:34 UTC (permalink / raw)
To: git; +Cc: Jannis Pohlmann
In-Reply-To: <a795f6dca5e7c3fc5f9212becda4a46116c502b7.1329939233.git.trast@student.ethz.ch>
The first part of the bundle header contains the boundary commits, and
could be approximated by
# v2 git bundle
$(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)
git-bundle actually spawns exactly this rev-list invocation, and does
the grepping internally.
There was a subtle bug in the latter step: it used fgets() with a
1024-byte buffer. If the user has sufficiently long subjects (e.g.,
by not adhering to the git oneline-subject convention in the first
place), the 'oneline' format can easily overflow the buffer. fgets()
then returns the rest of the line in the next call(s). If one of
these remaining parts started with '-', git-bundle would mistakenly
insert it into the bundle thinking it was a boundary commit.
Fix it by using strbuf_getwholeline() instead, which handles arbitrary
line lengths correctly.
Note that on the receiving side in parse_bundle_header() we were
already using strbuf_getwholeline_fd(), so that part is safe.
Reported-by: Jannis Pohlmann <jannis.pohlmann@codethink.co.uk>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
bundle.c | 14 +++++++-------
t/t5704-bundle.sh | 17 +++++++++++++++++
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/bundle.c b/bundle.c
index 313de42..0dbd174 100644
--- a/bundle.c
+++ b/bundle.c
@@ -234,7 +234,7 @@ int create_bundle(struct bundle_header *header, const char *path,
const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
const char **argv_pack = xmalloc(6 * sizeof(const char *));
int i, ref_count = 0;
- char buffer[1024];
+ struct strbuf buf = STRBUF_INIT;
struct rev_info revs;
struct child_process rls;
FILE *rls_fout;
@@ -266,16 +266,16 @@ int create_bundle(struct bundle_header *header, const char *path,
if (start_command(&rls))
return -1;
rls_fout = xfdopen(rls.out, "r");
- while (fgets(buffer, sizeof(buffer), rls_fout)) {
+ while (strbuf_getwholeline(&buf, rls_fout, '\n') != EOF) {
unsigned char sha1[20];
- if (buffer[0] == '-') {
- write_or_die(bundle_fd, buffer, strlen(buffer));
- if (!get_sha1_hex(buffer + 1, sha1)) {
+ if (buf.len > 0 && buf.buf[0] == '-') {
+ write_or_die(bundle_fd, buf.buf, buf.len);
+ if (!get_sha1_hex(buf.buf + 1, sha1)) {
struct object *object = parse_object(sha1);
object->flags |= UNINTERESTING;
- add_pending_object(&revs, object, buffer);
+ add_pending_object(&revs, object, buf.buf);
}
- } else if (!get_sha1_hex(buffer, sha1)) {
+ } else if (!get_sha1_hex(buf.buf, sha1)) {
struct object *object = parse_object(sha1);
object->flags |= SHOWN;
}
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 4ae127d..7c2f307 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -59,4 +59,21 @@ test_expect_success 'empty bundle file is rejected' '
'
+# If "ridiculous" is at least 1004 chars, this traps a bug in old
+# versions where the resulting 1025-char line (with --pretty=oneline)
+# was longer than a 1024-char buffer
+test_expect_success 'ridiculously long subject in boundary' '
+
+ : > file4 &&
+ test_tick &&
+ git add file4 &&
+ printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - &&
+ test_commit fifth &&
+ git bundle create long-subject-bundle.bdl HEAD^..HEAD &&
+ git fetch long-subject-bundle.bdl &&
+ sed -n "/^-/{p;q}" long-subject-bundle.bdl > boundary &&
+ grep "^-$_x40 " boundary
+
+'
+
test_done
--
1.7.9.1.430.g4998543
^ permalink raw reply related
* [PATCH 1/2] bundle: put strbuf_readline_fd in strbuf.c with adjustments
From: Thomas Rast @ 2012-02-22 19:34 UTC (permalink / raw)
To: git; +Cc: Jannis Pohlmann
In-Reply-To: <4F451259.7010304@codethink.co.uk>
The comment even said that it should eventually go there. While at
it, match the calling convention and name of the function to the
strbuf_get*line family. So it now is strbuf_getwholeline_fd.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
I was left looking for strbuf_readline_fd() for a while, then tried to
look for the readline() from the libc that it would presumably match.
Hence this cleanup.
bundle.c | 21 ++-------------------
strbuf.c | 16 ++++++++++++++++
strbuf.h | 1 +
3 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/bundle.c b/bundle.c
index b8acf3c..313de42 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,23 +23,6 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
list->nr++;
}
-/* Eventually this should go to strbuf.[ch] */
-static int strbuf_readline_fd(struct strbuf *sb, int fd)
-{
- strbuf_reset(sb);
-
- while (1) {
- char ch;
- ssize_t len = xread(fd, &ch, 1);
- if (len <= 0)
- return len;
- strbuf_addch(sb, ch);
- if (ch == '\n')
- break;
- }
- return 0;
-}
-
static int parse_bundle_header(int fd, struct bundle_header *header,
const char *report_path)
{
@@ -47,7 +30,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
int status = 0;
/* The bundle header begins with the signature */
- if (strbuf_readline_fd(&buf, fd) ||
+ if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
strcmp(buf.buf, bundle_signature)) {
if (report_path)
error("'%s' does not look like a v2 bundle file",
@@ -57,7 +40,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
}
/* The bundle header ends with an empty line */
- while (!strbuf_readline_fd(&buf, fd) &&
+ while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
buf.len && buf.buf[0] != '\n') {
unsigned char sha1[20];
int is_prereq = 0;
diff --git a/strbuf.c b/strbuf.c
index ff0b96b..5135d59 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -383,6 +383,22 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
return 0;
}
+int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)
+{
+ strbuf_reset(sb);
+
+ while (1) {
+ char ch;
+ ssize_t len = xread(fd, &ch, 1);
+ if (len <= 0)
+ return EOF;
+ strbuf_addch(sb, ch);
+ if (ch == term)
+ break;
+ }
+ return 0;
+}
+
int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
{
int fd, len;
diff --git a/strbuf.h b/strbuf.h
index fbf059f..3effaa8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -116,6 +116,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
extern int strbuf_getline(struct strbuf *, FILE *, int);
+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
extern void stripspace(struct strbuf *buf, int skip_comments);
extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
--
1.7.9.1.430.g4998543
^ permalink raw reply related
* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Junio C Hamano @ 2012-02-22 19:41 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Michael J Gruber, pclouds, j.sixt
In-Reply-To: <4F44D084.7030308@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> by "scales monotonically with the change count" I meant with two
> different commits.
Because of the scaling, the same 300-line change to the same file will be
shown with different number of +- depending on the amount of change to
other files in the same commit. It is not something we should be worried
about to begin with, unless you are going to read all commits in advance
and find necessary length of the name part, and also maxchange across
these commits to align things across commits, which of course would not
mesh well with our desire to stream output whenever possible.
> [7.3/8] limit graph part to 40 columns
Read the problem you are trying to solve again (from your 3/8):
>> If commits changing a lot of lines are displayed in a wide terminal
>> window (200 or more columns), and the +- graph would use the full
>> width, the output would look bad. Messages wrapped to about 80 columns
>> would be interspersed with very long +- lines.
If your name part needs only 10-20 columns and you are used to seeing
graph bars that are 60-70 columns long on your standard 80-column
terminal, you may see it ugly if the same short names are followed by
graph bars that are 180-190 columns (i.e. "very long +-"), when the
messages still fit 80 columns on the standard terminal, which is a width
that can comfortably be scanned with horizontal movement of human eyes. I
would understand that concern, although I do not personally think it is a
big deal.
Imagine if your name part were 125 columns wide (200 * 5/8), and also
imagine you drew the same graph bars as before, i.e. using 60-70 columns.
That line will fill almost the full terminal width, and will extend beyond
the right edge of the message block, but you are not showing "very long +-
lines" anymore. The length of the line comes mostly from overly long
names, carrying more information than before (because your patch lets us
show larger parts of such a long name on wider terminal), and without
losing much information from the graph part.
Now imagine if your patch only needed 10-20 columns for the names, and you
are showing the same change on an 80-column terminal with your 40-column
cap. You will introduce a new problem with "very short +-" lines instead,
as the "diff --stat" block will be much shorter than the surrounding text.
You fixed a bug in the original partitioning with 1/8, which was caused by
a code that capped the length of the graph part too early before taking
the width necessary to show the name into account; the bug resulted in a
graph that did not fill the allotted space fully to the right, even though
the user wanted to give full width to the graph.
The ugliness avoidance against "very long +-" becomes an issue only on
wide terminals; doing the same 40-column cap on non-wide terminals is to
re-introduce the same "cap the graph width too early, ending up with a
graph that is narrower than desired" bug you fixed. Why do you keep
insisting on that broken approach?
It might be just the matter of raising the artificial cap to much higher
than 40-column (say, 80-column). A possible alternative may be to declare
that the perceived ugliness is the user's problem of having overly wide
terminal in the first place and do without any such cap.
Either is fine, but regressing output on 80-column terminal when showing a
patch with short filenames and large changes is unacceptable, not because
I personally use 80-col terminal myself (I don't---mine is a bit wider but
not 200), but because it changes behaviour from the old code without any
good justification to do so.
^ permalink raw reply
* Re: contrib/git-move-refs.py not exists in cvs2svn-2.3.0
From: Michael Haggerty @ 2012-02-22 20:05 UTC (permalink / raw)
To: supadhyay; +Cc: git
In-Reply-To: <1329909939769-7308023.post@n2.nabble.com>
On 02/22/2012 12:25 PM, supadhyay wrote:
> I have run the cvs2git tool to migrate my CVS repository to git. I am able
> to finish successfully. Now as the last recommended steps from
> http://cvs2svn.tigris.org/cvs2git.html , I found to run
> "contrib/git-move-refs.py" but in my cvs2svn-2.3.0 I can not see any file
> name like "git-move-refs.ph", I have one file "git-move-tags.py". Is this
> is the same file ?
This program was added in the trunk version of cvs2svn; it is not in
release 2.3.0. I recommend using the trunk version for your conversion
anyway, as it includes a number of improvements (some git-relevant) and
it would be released already except for a few loose ends that I haven't
gotten back to.
cvs2svn/cvs2git/cvs2etc is a project separate from git and it has its
own mailing list to which your conversion questions should probably be
directed:
users@cvs2svn.tigris.org
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] contrib: added git-diffall
From: Tim Henigan @ 2012-02-22 20:14 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <CAJDDKr72rPqp1z-12Ht3Q2UaeUVFutKwoOgkD1G+SbhsBs+p1A@mail.gmail.com>
On Wed, Feb 22, 2012 at 4:15 AM, David Aguilar <davvid@gmail.com> wrote:
> On Tue, Feb 21, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Tim Henigan <tim.henigan@gmail.com> writes:
>>
>> David, any idea on this?
>
> I don't see any bash-isms there myself, either. We should keep this
> stuff without bash-isms.
> I haven't had time to read these patches in depth yet but will try to
> read the re-roll.
>
> Can we ask the github user to elaborate on what exactly was erroring out?
> Does dash not handle || inside $()? We can only make wild guesses
> without their help.
>
> The only hint from the pull request is "silent exit with no results".
> Do we do that?
> There are a few code paths where we do "exit 1" but that's only under
> error conditions.
>
> We haven't had any reports about git-mergetool/difftool, which use
> these functions...
> are we certain the problem was not some other bash-isms in the script?
I have not heard back from the user, but I tested on Ubuntu earlier
today. I found that when using an older version of the script
(fbedb7a in the GitHub repo), I could repeat the error. It is the
'git-diffall' script that fails silently, I believe due to a bash-ism
that was fixed in a subsequent commit.
In my latest local version of the script, I have switched back to
#!/bin/sh. It runs successfully, so I will squash the change into v2
of the patch.
For what it is worth, since that bug was originally reported, I have
been running "checkbashisms" [1] on the git-diffall script. That
utility reports that the script is clean.
-Tim
[1]: http://sourceforge.net/projects/checkbaskisms/
^ permalink raw reply
* Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()
From: Felipe Contreras @ 2012-02-22 20:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vvcn5qecl.fsf@alter.siamese.dyndns.org>
On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> There's a lot of code that can be consolidated there, and will be useful
>> for next patches.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>> remote.c | 59 ++++++++++++++++++++++++++++++-----------------------------
>> 1 files changed, 30 insertions(+), 29 deletions(-)
>>
>> diff --git a/remote.c b/remote.c
>> index 55d68d1..019aafc 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
>> return errs;
>> }
>>
>> -static const struct refspec *check_pattern_match(const struct refspec *rs,
>> - int rs_nr,
>> - const struct ref *src)
>> +static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
>> + int send_mirror, const struct refspec **ret_pat)
>> {
>
> For a change that not just adds parameters but removes an existing one,
> this is way under-described with neither in-code comment nor log message.
But it doesn't. src is renamed to ref.
>> + const struct refspec *pat;
>> + char *name;
>> int i;
>> int matching_refs = -1;
>> for (i = 0; i < rs_nr; i++) {
>> @@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
>> continue;
>> }
>>
>> - if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,
>> - NULL, NULL))
>> - return rs + i;
>> + if (rs[i].pattern) {
>> + const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
>> + if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
>> + matching_refs = i;
>> + break;
>
> We used to discard what match_name_with_pattern() finds out by matching a
> wildcard refspec against the ref by passing two NULLs. This updates the
> code to capture what destination ref ref->name is mapped to, by using the
> same logic as the original and only caller, i.e. 'foo' without destination
> maps to the same 'foo' destination, 'foo:bar' maps to the named 'bar'.
>
> This function is not used by fetching side of the codepath, so we do not
> have to worry about its need to use different dst_side selection logic
> (i.e. 'foo' without destination maps to "do not store anywhere other than
> FETCH_HEAD"). Good.
I actually didn't parse a lot of that.
>> + }
>> + }
>> }
>> -...
>> + if (matching_refs == -1)
>> return NULL;
>> +
>> + pat = rs + matching_refs;
>> + if (pat->matching) {
>> + /*
>> + * "matching refs"; traditionally we pushed everything
>> + * including refs outside refs/heads/ hierarchy, but
>> + * that does not make much sense these days.
>> + */
>> + if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
>> + return NULL;
>> + name = xstrdup(ref->name);
>> + }
>
> So you are moving some code from what the sole caller of this function
> does after calling us, and that is where the new parameters come from.
> And by doing so, you do not have to run the same match_name_with_pattern()
> again. OK.
Indeed.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Junio C Hamano @ 2012-02-22 20:22 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jannis Pohlmann
In-Reply-To: <fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> The first part of the bundle header contains the boundary commits, and
> could be approximated by
>
> # v2 git bundle
> $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)
>
> git-bundle actually spawns exactly this rev-list invocation, and does
> the grepping internally.
>
> There was a subtle bug in the latter step: it used fgets() with a
> 1024-byte buffer. If the user has sufficiently long subjects (e.g.,
> by not adhering to the git oneline-subject convention in the first
> place), the 'oneline' format can easily overflow the buffer. fgets()
> then returns the rest of the line in the next call(s). If one of
> these remaining parts started with '-', git-bundle would mistakenly
> insert it into the bundle thinking it was a boundary commit.
>
> Fix it by using strbuf_getwholeline() instead, which handles arbitrary
> line lengths correctly.
>
> Note that on the receiving side in parse_bundle_header() we were
> already using strbuf_getwholeline_fd(), so that part is safe.
Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline
to begin with, except for debugging purposes.
Do we ever use the subject string read from the rev-list output in any
way?
In other words, I am wondering if the right patch to minimally fix the
issue starting from older releases is something along this line instead:
diff --git a/bundle.c b/bundle.c
index b8acf3c..339dbb0 100644
--- a/bundle.c
+++ b/bundle.c
@@ -248,7 +248,7 @@ int create_bundle(struct bundle_header *header, const char *path,
static struct lock_file lock;
int bundle_fd = -1;
int bundle_to_stdout;
- const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
+ const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));
const char **argv_pack = xmalloc(6 * sizeof(const char *));
int i, ref_count = 0;
char buffer[1024];
@@ -271,11 +271,10 @@ int create_bundle(struct bundle_header *header, const char *path,
init_revisions(&revs, NULL);
/* write prerequisites */
- memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
+ memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
argv_boundary[0] = "rev-list";
argv_boundary[1] = "--boundary";
- argv_boundary[2] = "--pretty=oneline";
- argv_boundary[argc + 2] = NULL;
+ argv_boundary[argc + 1] = NULL;
memset(&rls, 0, sizeof(rls));
rls.argv = argv_boundary;
rls.out = -1;
^ permalink raw reply related
* Re: Problems with unrecognized headers in git bundles
From: Øyvind A. Holm @ 2012-02-22 20:25 UTC (permalink / raw)
To: Jannis Pohlmann; +Cc: git
In-Reply-To: <4F451259.7010304@codethink.co.uk>
On 22 February 2012 17:05, Jannis Pohlmann wrote:
> Hi,
>
> creating bundles from some repositories seems to lead to bundles with
> incorrectly formatted headers, at least with git >= 1.7.2. When
> cloning from such bundles, git prints the following error/warning:
>
> $ git clone perl-clone.bundle perl-clone
> Cloning into 'perl-clone'...
> warning: unrecognized header: --work around mangled archname on...
>
> This can be reproduced easily with git from any version >= 1.7.2 or
> from master, using the following steps:
>
> git clone git://perl5.git.perl.org/perl.git perl
> GIT_DIR=perl/.git git bundle create perl-clone.bundle --all
> git clone perl-clone.bundle perl-clone
>
> The content of the bundle is:
>
> # v2 git bundle
> -- work around mangled archname on win32 while finding...
> 39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/heads/blead
> 39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/remotes/origin/HEAD
Have researched this a bit, and I've found that all git versions back to
when git-bundle was introduced (around v1.5.4) produces the same invalid
line. The culprit is commit 3e8148feadabd0d0b1869fcc4d218a6475a5b0bc in
perl.git, branch 'maint-5.005'. The log message of that commit contains
email headers, maybe that's the reason git bundle gets confused?
Øyvind
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Thomas Rast @ 2012-02-22 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Rast, git, Jannis Pohlmann
In-Reply-To: <7vlinuaaab.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> # v2 git bundle
>> $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)
>>
>> git-bundle actually spawns exactly this rev-list invocation, and does
>> the grepping internally.
>>
>> There was a subtle bug in the latter step: it used fgets() with a
>> 1024-byte buffer. If the user has sufficiently long subjects (e.g.,
>> by not adhering to the git oneline-subject convention in the first
>> place), the 'oneline' format can easily overflow the buffer.
>
> Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline
> to begin with, except for debugging purposes.
>
> Do we ever use the subject string read from the rev-list output in any
> way?
>
> In other words, I am wondering if the right patch to minimally fix the
> issue starting from older releases is something along this line instead:
Not sure. The only use I could think of would be to google for the
subjects, in the hope of finding some repository that has the commit you
are looking for. Other than that...
In any case the --pretty=oneline is very deliberate, as we can see from
the commit below. It just doesn't give a reason :-)
commit 239296770dae75e21c179733785731ec6ffae1f5
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: Fri Feb 23 03:17:51 2007 +0100
git-bundle: record commit summary in the prerequisite data
diff --git a/builtin-bundle.c b/builtin-bundle.c
index 191ec55..d74afaa 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -267,7 +267,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv)
{
int bundle_fd = -1;
- const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));
+ const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
const char **argv_pack = xmalloc(4 * sizeof(const char *));
int pid, in, out, i, status;
char buffer[1024];
@@ -282,10 +282,11 @@ static int create_bundle(struct bundle_header *header, const char *path,
write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
/* write prerequisites */
- memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
+ memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
argv_boundary[0] = "rev-list";
argv_boundary[1] = "--boundary";
- argv_boundary[argc + 1] = NULL;
+ argv_boundary[2] = "--pretty=oneline";
+ argv_boundary[argc + 2] = NULL;
out = -1;
pid = fork_with_pipe(argv_boundary, NULL, &out);
if (pid < 0)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply related
* Re: Ambiguous reference weirdness
From: Jeff King @ 2012-02-22 20:38 UTC (permalink / raw)
To: Phil Hord; +Cc: git, Ramkumar Ramachandra, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CABURp0r+3gOQ3rq_ubv=uEoU=XmtYs=etfT4W2Lb9M0LBrikWg@mail.gmail.com>
On Wed, Feb 22, 2012 at 09:48:23AM -0500, Phil Hord wrote:
> > What is 1147? Is it supposed to be a partial sha1, or is it a ref you
> > have?
>
> 1147 was a typo. It was a Gerritt changeset ID I forgot to expand.
> When I type "git cherry-pick 1147<TAB>", autocompletion expands this
> for me to "git cherry-pick origin/changes/47/1147/1". Except in this
> case I misfired the TAB and got the weird "BUG:" report. But I didn't
> see the same problem with other invalid refs, so I went searching for
> the variants and to see what caused it.
Ah. It's a little annoying that Gerrit names refs that look kind of
like partial sha1s. But I guess most of the time it's not that big a
deal (it is only because you were using the partial Gerrit ref with tab
completion). And I don't think we're about to change how Gerrit names
things. :)
> > Have you looked at the object that it resolves to? I suspect it is the
> > partial sha1 of a non-commit object. E.g.:
>
> All of these examples were run in current git.git, so you can try them
> yourself if needed. But I did figure out that 1147 resolves to a
> blob, and that's apparently the difference between these three:
Yeah. I figured that after reading more, but didn't go back and revise
the first part of my message. I was able to easily get the same results
as you (actually, in my git.git, 1147 is ambiguous because I happen to
have some extra refs, but it was easy to replicate with a fresh clone).
> $ git cherry-pick 1147
> fatal: BUG: expected exactly one commit from walk
>
> $ git cherry-pick 1146
> error: short SHA1 1146 is ambiguous.
> error: short SHA1 1146 is ambiguous.
> fatal: ambiguous argument '1146': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions
>
> $ git cherry-pick 114333
> fatal: ambiguous argument '114333': unknown revision or path not in
> the working tree.
> Use '--' to separate paths from revisions
>
> I consider the first two responses to be UI bugs. The second one is
> minor (the twice-reported error message), and the first one is pretty
> rude. I would expect all three to report the same conclusion,
> "fatal: ambiguous argument 'XXXXX': unknown revision or path not in
> the working tree." But the first one doesn't.
Right. Because cherry-pick's logic is:
if the arguments do not resolve to any objects at all
complain of unknown revision
if the resolved object is not a single commit
die of BUG
And I think you just want to collapse those two conditions into a single
conditional, which seems reasonable (the error message does say "unknown
revision", not "unknown object". It's a little more complicated than
that, because you are crossing a boundary between reusable library code
and cherry-pick specific code.
As for the doubled "ambiguous" warning, I'm not sure what the cause is
for that. I suspect it is because cherry-pick tries to parse one way (as
a revision walk specifier, like "a..b"), and then parses again (as a
single commit) when that fails, due to historical reasons. Obviously
it would be nice to see that improved, but I suspect it will be annoying
to do so; the error message is emitted by a low-level, and cherry-pick
has no idea that it has happened (it only sees "this didn't result in
parsing anything").
> Thanks. I'm not familiar with the {tree} syntax -- in fact I'd like
> to find a dictionary for all the reference spelling variants -- but
> this is elucidating.
See "git help rev-parse", section "Specifying Revisions".
> > In the cherry-pick case, the code is checking the right thing, but the
> > message is horrible. It is not a bug, but merely unexpected input, and
> > it should provide a usage message.
>
> Bug is too strong a word in one sense, but from the user perspective I
> consider this "horrible message" a bug.
Sorry, I meant git is wrong to use the word "BUG". For us, die("BUG:
...") is the equivalent of an assert. It means something totally
unexpected happened that should never happen, and therefore there is a
bug in git. But this is not a bug in git (well, it is, but not that
kind). It is a totally reasonable and expected malformed input that git
should diagnose and report correctly.
So it is a bug that git says "BUG". It should say "you gave me objects,
but they are not revisions" or something similar.
> > I think checkout has the same "is this a path or a revision" ambiguity
> > to resolve. But rather than be explicit that you might have meant "114"
> > as a tree, the error message assumes you meant a path. That might be
> > worth improving, similar to the above example.
> >
> > Again, you can disambiguate with:
> >
> > $ git checkout -- 1147
> > error: pathspec '1147' did not match any file(s) known to git.
> >
> > $ git checkout 1147 --
> > fatal: reference is not a tree: 1147
> >
> >> $ git checkout 1147
> >> fatal: reference is not a tree: 1147
>
> Yes, I understand this. This was a typo and it was ambiguous. But
> shouldn't we tell the user the same thing when encountering the same
> failure?
No, because there are really three cases here:
1. The user told us 1147 is a path.
2. The user told us 1147 is a tree.
3. The user did not tell us which, and we must guess.
We guess (1) if the name does not resolve at all, and (2) otherwise. And
the error message only indicates the particular guess we made.
So while I don't think they should all have the same error message,
there are two improvements I can see:
a. If we are guessing, and 1147 resolves but is _not_ a tree, we
could guess path instead.
b. When our guess results in an error, it would be better to be
explicit about the fact that we guessed (i.e., say "this is neither
a tree nor a path that git knows about", similar to git-log).
> > I think the outcomes are all working as intended, but the error messages
> > could stand to be improved.
>
> Yes, I agree. I only meant to complain about the error messages, not
> the results. Thanks for the discussion. I'll try to look for where
> these come from and see if they can be improved within reason.
Great. I look forward to it.
-Peff
^ 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;
as well as URLs for NNTP newsgroup(s).