* Re: [PATCH v2 4/6] clone: --branch=<branch> always means refs/heads/<branch>
From: Junio C Hamano @ 2012-01-10 0:33 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King
In-Reply-To: <1326023188-15559-4-git-send-email-pclouds@gmail.com>
Up to this point I find the series makes sense.
^ permalink raw reply
* Re: [PATCH v2 6/6] clone: print advice on checking out detached HEAD
From: Junio C Hamano @ 2012-01-10 0:36 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King
In-Reply-To: <1326023188-15559-6-git-send-email-pclouds@gmail.com>
This patch makes 100% sense _if_ we let clone result in a repository with
a detached HEAD, which I am not sure if it is a good idea, or if it is
better to fail the attempt to clone to give incentive to the owner of the
remote repository to fix it.
^ permalink raw reply
* What's cooking in git.git (Jan 2012, #02; Mon, 9)
From: Junio C Hamano @ 2012-01-10 0:50 UTC (permalink / raw)
To: git
Here are the topics that have been cooking. Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in
'next'.
Here are the repositories that have my integration branches:
With maint, master, next, pu, todo:
git://git.kernel.org/pub/scm/git/git.git
git://repo.or.cz/alt-git.git
https://code.google.com/p/git-core/
https://github.com/git/git
With only maint and master:
git://git.sourceforge.jp/gitroot/git-core/git.git
git://git-core.git.sourceforge.net/gitroot/git-core/git-core
With all the topics and integration branches:
https://github.com/gitster/git
The preformatted documentation in HTML and man format are found in:
git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
git://repo.or.cz/git-{htmldocs,manpages}.git/
https://code.google.com/p/git-{htmldocs,manpages}.git/
https://github.com/gitster/git-{htmldocs,manpages}.git/
--------------------------------------------------
[New Topics]
* rs/diff-postimage-in-context (2012-01-06) 1 commit
(merged to 'next' on 2012-01-09 at 9635032)
+ xdiff: print post-image for common records instead of pre-image
Looked reasonable.
Not urgent.
* cb/push-quiet (2012-01-08) 3 commits
- t5541: avoid TAP test miscounting
- fix push --quiet: add 'quiet' capability to receive-pack
- server_supports(): parse feature list more carefully
Looked reasonable.
Not urgent.
* nd/clone-detached (2012-01-08) 6 commits
- clone: print advice on checking out detached HEAD
- clone: allow --branch to take a tag
- clone: --branch=<branch> always means refs/heads/<branch>
- clone: factor out checkout code
- clone: write detached HEAD in bare repositories
- t5601: add missing && cascade
I am not sure what the benefit of this is.
* nd/clone-single-branch (2012-01-08) 1 commit
(merged to 'next' on 2012-01-09 at 6c3c759)
+ clone: add --single-branch to fetch only one branch
Looked reasonable.
Not urgent.
* jn/gitweb-unspecified-action (2012-01-09) 1 commit
- gitweb: Fix actionless dispatch for non-existent objects
--------------------------------------------------
[Graduated to "master"]
* jc/show-sig (2012-01-05) 6 commits
(merged to 'next' on 2012-01-05 at 5da3ae2)
+ log --show-signature: reword the common two-head merge case
+ log-tree: show mergetag in log --show-signature output
+ log-tree.c: small refactor in show_signature()
+ commit --amend -S: strip existing gpgsig headers
+ verify_signed_buffer: fix stale comment
+ Merge branch 'jc/signed-commit' and 'jc/pull-signed-tag'
(this branch uses jc/signed-commit.)
Finishing touches to the already graduated "pull signed tags" topic.
* jc/signed-commit (2011-11-29) 5 commits
(merged to 'next' on 2011-12-21 at 8fcbf00)
+ gpg-interface: allow use of a custom GPG binary
+ pretty: %G[?GS] placeholders
+ test "commit -S" and "log --show-signature"
+ log: --show-signature
+ commit: teach --gpg-sign option
(this branch is used by jc/show-sig.)
* jh/fetch-head-update (2012-01-03) 1 commit
(merged to 'next' on 2012-01-04 at b5778e1)
+ write first for-merge ref to FETCH_HEAD first
* jk/credentials (2012-01-08) 1 commit
(merged to 'next' on 2012-01-08 at 48766c9)
+ credentials: unable to connect to cache daemon
* jm/stash-diff-disambiguate (2012-01-01) 1 commit
(merged to 'next' on 2012-01-05 at 75a283b)
+ stash: Don't fail if work dir contains file named 'HEAD'
* mh/ref-api-less-extra-refs (2012-01-06) 3 commits
(merged to 'next' on 2012-01-06 at 3105696)
+ write_head_info(): handle "extra refs" locally
+ show_ref(): remove unused "flag" and "cb_data" arguments
+ receive-pack: move more work into write_head_info()
* mm/maint-gitweb-project-maxdepth (2012-01-04) 1 commit
(merged to 'next' on 2012-01-06 at bcf3818)
+ gitweb: accept trailing "/" in $project_list
Looked quite sensible.
* pw/p4-view-updates (2012-01-03) 6 commits
(merged to 'next' on 2012-01-03 at c3b5872)
+ git-p4: view spec documentation
+ git-p4: rewrite view handling
+ git-p4: support single file p4 client view maps
+ git-p4: sort client views by reverse View number
+ git-p4: fix test for unsupported P4 Client Views
+ git-p4: test client view handling
--------------------------------------------------
[Stalled]
* jc/advise-push-default (2011-12-18) 1 commit
- push: hint to use push.default=upstream when appropriate
Peff had a good suggestion outlining an updated code structure so that
somebody new can try to dip his or her toes in the development. Any
takers?
Waiting for a reroll.
* jc/split-blob (2011-12-01) 6 commits
. WIP (streaming chunked)
- chunked-object: fallback checkout codepaths
- bulk-checkin: support chunked-object encoding
- bulk-checkin: allow the same data to be multiply hashed
- new representation types in the packstream
- varint-in-pack: refactor varint encoding/decoding
Not ready.
At least pack-objects and fsck need to learn the new encoding for the
series to be usable locally, and then index-pack/unpack-objects needs to
learn it to be used remotely.
* jc/advise-i18n (2011-12-22) 1 commit
- i18n of multi-line advice messages
Allow localization of advice messages that tend to be longer and
multi-line formatted. For now this is deliberately limited to advise()
interface and not vreportf() in general as touching the latter has
interactions with error() that has plumbing callers whose prefix "error: "
should never be translated.
--------------------------------------------------
[Cooking]
* nd/index-pack-no-recurse (2012-01-09) 3 commits
- index-pack: eliminate unlimited recursion in get_delta_base()
- index-pack: eliminate recursion in find_unresolved_deltas
- Eliminate recursion in setting/clearing marks in commit list
The first one looked sensible; I am not sure if the second and third ones
take the right approach.
* bw/maint-t8006-sed-incomplete-line (2012-01-03) 1 commit
- Work around sed portability issue in t8006-blame-textconv
Waiting for a clarification of the reasoning in the log message.
* mh/ref-api-rest (2011-12-12) 35 commits
- repack_without_ref(): call clear_packed_ref_cache()
- read_packed_refs(): keep track of the directory being worked in
- is_refname_available(): query only possibly-conflicting references
- refs: read loose references lazily
- read_loose_refs(): take a (ref_entry *) as argument
- struct ref_dir: store a reference to the enclosing ref_cache
- sort_ref_dir(): take (ref_entry *) instead of (ref_dir *)
- do_for_each_ref_in_dir*(): take (ref_entry *) instead of (ref_dir *)
- add_entry(): take (ref_entry *) instead of (ref_dir *)
- search_ref_dir(): take (ref_entry *) instead of (ref_dir *)
- find_containing_direntry(): use (ref_entry *) instead of (ref_dir *)
- add_ref(): take (ref_entry *) instead of (ref_dir *)
- read_packed_refs(): take (ref_entry *) instead of (ref_dir *)
- find_ref(): take (ref_entry *) instead of (ref_dir *)
- is_refname_available(): take (ref_entry *) instead of (ref_dir *)
- get_loose_refs(): return (ref_entry *) instead of (ref_dir *)
- get_packed_refs(): return (ref_entry *) instead of (ref_dir *)
- refs: wrap top-level ref_dirs in ref_entries
- get_ref_dir(): keep track of the current ref_dir
- do_for_each_ref(): only iterate over the subtree that was requested
- refs: sort ref_dirs lazily
- sort_ref_dir(): do not sort if already sorted
- refs: store references hierarchically
- refs.c: rename ref_array -> ref_dir
- struct ref_entry: nest the value part in a union
- check_refname_component(): return 0 for zero-length components
- free_ref_entry(): new function
- refs.c: reorder definitions more logically
- is_refname_available(): reimplement using do_for_each_ref_in_array()
- names_conflict(): simplify implementation
- names_conflict(): new function, extracted from is_refname_available()
- repack_without_ref(): reimplement using do_for_each_ref_in_array()
- do_for_each_ref_in_arrays(): new function
- do_for_each_ref_in_array(): new function
- do_for_each_ref(): correctly terminate while processesing extra_refs
The API for extra anchoring points may require rethought first; that would
hopefully make the "ref" part a lot simpler. And that is happening in
another topic (which has graduated to 'master').
Will defer till the next cycle.
* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits
- fixup! 15eaaf4
- git-svn, perl/Git.pm: extend Git::prompt helper for querying users
(merged to 'next' on 2012-01-05 at 954f125)
+ perl/Git.pm: "prompt" helper to honor GIT_ASKPASS and SSH_ASKPASS
The bottom one has been replaced with a rewrite based on comments from
Ævar. The second one needs more work, both in perl/Git.pm and prompt.c, to
give precedence to tty over SSH_ASKPASS when terminal is available.
Will defer till the next cycle.
* cb/git-daemon-tests (2012-01-08) 5 commits
(merged to 'next' on 2012-01-08 at 1db8351)
+ git-daemon tests: wait until daemon is ready
+ git-daemon: produce output when ready
+ git-daemon: add tests
+ dashed externals: kill children on exit
+ run-command: optionally kill children on exit
Will defer till the next cycle.
* jk/parse-object-cached (2012-01-06) 3 commits
(merged to 'next' on 2012-01-08 at 8c6fa4a)
+ upload-pack: avoid parsing tag destinations
+ upload-pack: avoid parsing objects during ref advertisement
+ parse_object: try internal cache before reading object db
These are a bit scary changes, but I do think they are worth doing.
Will defer till the next cycle.
* jn/maint-gitweb-grep-fix (2012-01-05) 2 commits
- gitweb: Harden "grep" search against filenames with ':'
- gitweb: Fix file links in "grep" search
Waiting for a confirmation from bug reporter.
^ permalink raw reply
* Re: leaky cherry-pick
From: Nguyen Thai Ngoc Duy @ 2012-01-10 1:38 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, Ramkumar Ramachandra
In-Reply-To: <20120109223737.GA1589@padd.com>
On Tue, Jan 10, 2012 at 5:37 AM, Pete Wyckoff <pw@padd.com> wrote:
> I've got a big tree, and rebased 200 commits today using
> cherry-pick. It ran out of memory. Both 1.7.4 and recent
> master (eac2d83 (Git 1.7.9-rc0, 2012-01-06)) behave similarly.
>
> Here's a valgrind dump for rebasing just 9 commits, if someone is
> interested to track this down. This was invoked as:
>
> valgrind --log-file=/tmp/vg.out --leak-check=full \
> /home/pw/src/git/git cherry-pick 8d535b6^..2cf53a0
>
> I can re-test if you like, or provide more detail if this seems
> unusual.
>
> Scroll to the end to see the biggest leaks.
Index entries are leaked since probably forever because we allocated
entries in bulk and could not release them individually, until debed2a
(read-cache.c: allocate index entries individually). Putting
discard_index() to read_and_refresh_cache() might help, I think.
--
Duy
^ permalink raw reply
* Re: [PATCHv3 10/13] credentials: add "cache" helper
From: Jonathan Nieder @ 2012-01-10 1:50 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111210103407.GJ16529@sigill.intra.peff.net>
Hi,
Jeff King wrote:
> This patch introduces a credential helper that will cache
> passwords in memory for a short period of time.
>
> Signed-off-by: Jeff King <peff@peff.net>
[...]
> t/t0301-credential-cache.sh | 18 ++
This test is failing for me:
expecting success:
check fill $HELPER <<-\EOF
protocol=https
host=example.com
--
username=askpass-username
password=askpass-password
--
askpass: Username for 'https://example.com':
askpass: Password for 'https://askpass-username@example.com':
EOF
--- expect-stderr 2012-01-10 00:07:02.398365248 +0000
+++ stderr 2012-01-10 00:07:02.418364996 +0000
@@ -1,2 +1,3 @@
+fatal: socket path is too long to fit in sockaddr
askpass: Username for 'https://example.com':
askpass: Password for 'https://askpass-username@example.com':
not ok - 1 helper (cache) has no existing data
I didn't notice until recently because typically the cwd is short.
The sun_path[] array on this machine is 108 bytes; POSIX informs me
that some platforms make it as small as 96 bytes and there's no
guaranteed lower limit. The machines[*] triggering it were running
tests in a chroot, hence the long path.
So you should be able to reproduce this with
longpath=foo/bar; # > 6 chars
longpath=$longpath/$longpath/$longpath/$longpath; # > 24
longpath=$longpath/$longpath/$longpath/$longpath; # > 96
longpath=/tmp/$longpath/$longpath
mkdir -p $longpath
sh t0301-credential-cache.sh --root=$longpath
Ideas?
---
[*] https://buildd.debian.org/status/logs.php?pkg=git&ver=1%3A1.7.9~rc0-1
credential-cache.c | 18 +++++++++++++-----
1 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/credential-cache.c b/credential-cache.c
index dc98372e..fd9304e6 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -82,7 +82,7 @@ static void do_cache(const char *socket, const char *action, int timeout,
int main(int argc, const char **argv)
{
- char *socket_path = NULL;
+ const char *socket_path = NULL;
int timeout = 900;
const char *op;
const char * const usage[] = {
@@ -102,10 +102,18 @@ int main(int argc, const char **argv)
usage_with_options(usage, options);
op = argv[0];
- if (!socket_path)
- socket_path = expand_user_path("~/.git-credential-cache/socket");
- if (!socket_path)
- die("unable to find a suitable socket path; use --socket");
+ if (!socket_path) {
+ char *home = expand_user_path("~");
+ if (!home)
+ die("unable to find a suitable socket path; use --socket");
+
+ if (!chdir(home))
+ socket_path = ".git-credential-cache/socket";
+ else if (errno == ENOENT && !strcmp(op, "exit"))
+ return 0;
+ else
+ die("cannot enter home directory");
+ }
if (!strcmp(op, "exit"))
do_cache(socket_path, op, timeout, 0);
--
1.7.8.2
^ permalink raw reply related
* Re: [PATCH v2 6/6] clone: print advice on checking out detached HEAD
From: Nguyen Thai Ngoc Duy @ 2012-01-10 1:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7v4nw4cs1x.fsf@alter.siamese.dyndns.org>
2012/1/10 Junio C Hamano <gitster@pobox.com>:
> This patch makes 100% sense _if_ we let clone result in a repository with
> a detached HEAD, which I am not sure if it is a good idea, or if it is
> better to fail the attempt to clone to give incentive to the owner of the
> remote repository to fix it.
Then a hostile remote can stop users from cloning his repository by
detaching HEAD? That's not nice. On the other hand, if specifying
--branch=<wrong-branch> leads to detached case, then we should
probably refuse to clone. But that should happen before transferring
the pack.
--
Duy
^ permalink raw reply
* Re: [PATCH v2 3/6] clone: factor out checkout code
From: Nguyen Thai Ngoc Duy @ 2012-01-10 2:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vd3ascs85.fsf@alter.siamese.dyndns.org>
2012/1/10 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> Read HEAD from disk instead of relying on local variable
>> our_head_points_at, so that if earlier code fails to make HEAD
>> properly, it'll be detected.
>
> The end result might be more or less the same with your patch from the
> end-user's point of view, but "if earlier code fails", shouldn't you
> detect and diagnose it right there?
Sure, but another fence does not harm. There's also one thing I missed
in the commit message that it makes update head code and checkout code
more independent. Update head code does not need to maintain
our_head_points_at at the end for checkout anymore.
> If you observe lack of "HEAD" in checkout(), you cannot tell if that was
> because the remote did not have anything usable in the first place, or
> because we knew where it should point at (and may have even attempted to
> create it) but somehow failed to make it point at it.
The lack of HEAD probably won't happen because HEAD is created by
default in init-db. This is mainly to catch invalid HEAD (like putting
"refs/tags/something" in HEAD).
--
Duy
^ permalink raw reply
* [PATCH 0/1] Re-roll of the test fix for sed on solaris
From: Ben Walton @ 2012-01-10 2:47 UTC (permalink / raw)
To: git, gitster
In-Reply-To: <7vd3b0vc6h.fsf@alter.siamese.dyndns.org>
Hi Junio,
It seems that you were correct in that it's the exit status from sed
that ultimately causes the breakage. I've updated the commit message
to reflect this.
Thanks
-Ben
^ permalink raw reply
* [PATCH] Use perl instead of sed for t8006-blame-textconv test
From: Ben Walton @ 2012-01-10 2:47 UTC (permalink / raw)
To: git, gitster; +Cc: Ben Walton
In-Reply-To: <1326163653-26565-1-git-send-email-bwalton@artsci.utoronto.ca>
In test 'blame --textconv with local changes' of t8006-blame-textconv,
using /usr/xpg4/bin/sed (as set by SANE_TOOL_PATH), an additional
newline was added to the output from the 'helper' script.
This was noted by sed with a message such as:
sed: Missing newline at end of file zero.bin.
Sed then exits with status 2 causing the helper script to also exit
with status 2.
In turn, this was triggering a fatal error from git blame:
fatal: unable to read files to diff
To work around this difference in sed behaviour, use perl -p instead
of sed -e as it exits cleanly and does not insert the additional
newline.
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
t/t8006-blame-textconv.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index 4ee42f1..c3c22f7 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -10,7 +10,7 @@ find_blame() {
cat >helper <<'EOF'
#!/bin/sh
grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
-sed 's/^bin: /converted: /' "$1"
+perl -p -e 's/^bin: /converted: /' "$1"
EOF
chmod +x helper
--
1.7.8.2
^ permalink raw reply related
* Re: [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Ramkumar Ramachandra @ 2012-01-10 3:40 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108215510.GS1942@burratino>
Hi Jonathan,
Jonathan Nieder wrote:
> On second thought, do you have a link to the last submitted version,
> and do you remember if there were any important changes since then?
> The base for that one should be closer to "master", I think.
There you go: http://thread.gmane.org/gmane.comp.version-control.git/186638/focus=186644
-- Ram
^ permalink raw reply
* Re: [PATCHv3 10/13] credentials: add "cache" helper
From: Jeff King @ 2012-01-10 4:44 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git
In-Reply-To: <20120110015038.GA17754@burratino>
On Mon, Jan 09, 2012 at 07:50:38PM -0600, Jonathan Nieder wrote:
> --- expect-stderr 2012-01-10 00:07:02.398365248 +0000
> +++ stderr 2012-01-10 00:07:02.418364996 +0000
> @@ -1,2 +1,3 @@
> +fatal: socket path is too long to fit in sockaddr
> askpass: Username for 'https://example.com':
> askpass: Password for 'https://askpass-username@example.com':
> not ok - 1 helper (cache) has no existing data
>
> I didn't notice until recently because typically the cwd is short.
> The sun_path[] array on this machine is 108 bytes; POSIX informs me
> that some platforms make it as small as 96 bytes and there's no
> guaranteed lower limit. The machines[*] triggering it were running
> tests in a chroot, hence the long path.
Ugh. Some days I really love working on Unix systems. And then some days
you run across junk like this.
Presumably Linux has kept to 108 to avoid breaking older programs. So
it's not as if we can assume it will be changed soon, or just write this
off as a limitation for old crappy systems.
Googling around, I've seen some indication that you can simply
over-allocate the sockaddr_un and pass a longer length to connect.
However that just seems to yield EINVAL on Linux (and even if it did
work on Linux, I'd be surprised if it worked everywhere).
Which really leaves us with only one option: chdir and bind to a
relative path, as you did below.
> - if (!socket_path)
> - socket_path = expand_user_path("~/.git-credential-cache/socket");
> - if (!socket_path)
> - die("unable to find a suitable socket path; use --socket");
> + if (!socket_path) {
> + char *home = expand_user_path("~");
> + if (!home)
> + die("unable to find a suitable socket path; use --socket");
> +
> + if (!chdir(home))
> + socket_path = ".git-credential-cache/socket";
> + else if (errno == ENOENT && !strcmp(op, "exit"))
> + return 0;
> + else
> + die("cannot enter home directory");
> + }
I think this is the right approach, but the wrong place to do it. Your
patch only helps people using the default path (so passing
--socket=$longpath would still be broken). And we'd need a similar fix
for the binding side in credential-cache--daemon.
Here's a patch which passes your $longpath test.
-- >8 --
Subject: [PATCH] unix-socket: handle long socket pathnames
On many systems, the sockaddr_un.sun_path field is quite
small. Even on Linux, it is only 108 characters. A user of
the credential-cache daemon can easily surpass this,
especially if their home directory is in a deep directory
tree (since the default location expands ~/.git-credentials).
We can hack around this in the unix-socket.[ch] code by
doing a chdir() to the enclosing directory, feeding the
relative basename to the socket functions, and then
restoring the working directory.
This introduces several new possible error cases for
creating a socket, including an irrecoverable one in the
case that we can't restore the working directory. In the
case of the credential-cache code, we could perhaps get away
with simply chdir()-ing to the socket directory and never
coming back. However, I'd rather do it at the lower level
for a few reasons:
1. It keeps the hackery behind an opaque interface instead
of polluting the main program logic.
2. A hack in credential-cache won't help any unix-socket
users who come along later.
3. The chdir trickery isn't that likely to fail (basically
it's only a problem if your cwd is missing or goes away
while you're running). And because we only enable the
hack when we get a too-long name, it can only fail in
cases that would have failed under the previous code
anyway.
Signed-off-by: Jeff King <peff@peff.net>
---
unix-socket.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/unix-socket.c b/unix-socket.c
index 84b1509..664782a 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -9,27 +9,86 @@ static int unix_stream_socket(void)
return fd;
}
-static void unix_sockaddr_init(struct sockaddr_un *sa, const char *path)
+static int chdir_len(const char *orig, int len)
+{
+ char *path = xmemdupz(orig, len);
+ int r = chdir(path);
+ free(path);
+ return r;
+}
+
+struct unix_sockaddr_context {
+ char orig_dir[PATH_MAX];
+};
+
+static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
+{
+ if (!ctx->orig_dir[0])
+ return;
+ /*
+ * If we fail, we can't just return an error, since we have
+ * moved the cwd of the whole process, which could confuse calling
+ * code. We are better off to just die.
+ */
+ if (chdir(ctx->orig_dir) < 0)
+ die("unable to restore original working directory");
+}
+
+static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
+ struct unix_sockaddr_context *ctx)
{
int size = strlen(path) + 1;
- if (size > sizeof(sa->sun_path))
- die("socket path is too long to fit in sockaddr");
+
+ ctx->orig_dir[0] = '\0';
+ if (size > sizeof(sa->sun_path)) {
+ const char *slash = find_last_dir_sep(path);
+ const char *dir;
+ int dir_len;
+
+ if (!slash) {
+ errno = ENAMETOOLONG;
+ return -1;
+ }
+
+ dir = path;
+ dir_len = slash - path;
+
+ path = slash + 1;
+ size = strlen(path) + 1;
+ if (size > sizeof(sa->sun_path)) {
+ errno = ENAMETOOLONG;
+ return -1;
+ }
+
+ if (!getcwd(ctx->orig_dir, sizeof(ctx->orig_dir))) {
+ errno = ENAMETOOLONG;
+ return -1;
+ }
+ if (chdir_len(dir, dir_len) < 0)
+ return -1;
+ }
+
memset(sa, 0, sizeof(*sa));
sa->sun_family = AF_UNIX;
memcpy(sa->sun_path, path, size);
+ return 0;
}
int unix_stream_connect(const char *path)
{
int fd;
struct sockaddr_un sa;
+ struct unix_sockaddr_context ctx;
- unix_sockaddr_init(&sa, path);
+ if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+ return -1;
fd = unix_stream_socket();
if (connect(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+ unix_sockaddr_cleanup(&ctx);
close(fd);
return -1;
}
+ unix_sockaddr_cleanup(&ctx);
return fd;
}
@@ -37,20 +96,25 @@ int unix_stream_listen(const char *path)
{
int fd;
struct sockaddr_un sa;
+ struct unix_sockaddr_context ctx;
- unix_sockaddr_init(&sa, path);
+ if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+ return -1;
fd = unix_stream_socket();
unlink(path);
if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
+ unix_sockaddr_cleanup(&ctx);
close(fd);
return -1;
}
if (listen(fd, 5) < 0) {
+ unix_sockaddr_cleanup(&ctx);
close(fd);
return -1;
}
+ unix_sockaddr_cleanup(&ctx);
return fd;
}
--
1.7.9.rc0.33.g15ced5
^ permalink raw reply related
* Re: [PATCH 0/1] Re-roll of the test fix for sed on solaris
From: Junio C Hamano @ 2012-01-10 4:46 UTC (permalink / raw)
To: Ben Walton; +Cc: git
In-Reply-To: <1326163653-26565-1-git-send-email-bwalton@artsci.utoronto.ca>
Ben Walton <bwalton@artsci.utoronto.ca> writes:
> It seems that you were correct in that it's the exit status from sed
> that ultimately causes the breakage. I've updated the commit message
> to reflect this.
Thanks for being thorough. Very much appreciated.
^ permalink raw reply
* Re: [PATCH v2 6/6] clone: print advice on checking out detached HEAD
From: Junio C Hamano @ 2012-01-10 4:49 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jeff King
In-Reply-To: <CACsJy8CuYkzFVrEG6T2HUAwJGnjit2xWt3VSN-9USt7h+B_CBw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 2012/1/10 Junio C Hamano <gitster@pobox.com>:
>> This patch makes 100% sense _if_ we let clone result in a repository with
>> a detached HEAD, which I am not sure if it is a good idea, or if it is
>> better to fail the attempt to clone to give incentive to the owner of the
>> remote repository to fix it.
>
> Then a hostile remote can stop users from cloning his repository by
> detaching HEAD? That's not nice.
That's crazy talk. Why does anybody from a hostile remote to begin with?
> On the other hand, if specifying --branch=<wrong-branch> leads to
> detached case, then we should probably refuse to clone.
If you mean "nonexistent" by "wrong", yeah, I agree, as we do not know
what the user asked us to pull down in that case.
^ permalink raw reply
* Re: [PATCHv3 10/13] credentials: add "cache" helper
From: Jeff King @ 2012-01-10 4:57 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git
In-Reply-To: <20120110044430.GA23619@sigill.intra.peff.net>
On Mon, Jan 09, 2012 at 11:44:30PM -0500, Jeff King wrote:
> Subject: [PATCH] unix-socket: handle long socket pathnames
And I think this should go on top. You were lucky enough that I used a
die() in the original code for your condition (because I thought it was
a "this could never happen, right?" condition). Had it simply returned
an error, the cache-daemon would have silently failed to do anything,
which would have been much more confusing for you. :)
We probably should have done this as part of Clemens' 98c2924, but I
didn't think of it then (but the same reasoning applies to both
patches).
-- >8 --
Subject: [PATCH] credential-cache: report more daemon connection errors
Originally, this code remained relatively silent when we
failed to connect to the cache. The idea was that it was
simply a cache, and we didn't want to bother the user with
temporary failures (the worst case is that we would simply
ask their password again).
However, if you have a configuration failure or other
problem, it is helpful for the daemon to report those
problems. Git will happily ignore the failed error code, but
the extra information to stderr can help the user diagnose
the problem.
Signed-off-by: Jeff King <peff@peff.net>
---
credential-cache.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/credential-cache.c b/credential-cache.c
index b15a9a7..1933018 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -71,10 +71,14 @@ static void do_cache(const char *socket, const char *action, int timeout,
die_errno("unable to relay credential");
}
- if (send_request(socket, &buf) < 0 && (flags & FLAG_SPAWN)) {
- spawn_daemon(socket);
- if (send_request(socket, &buf) < 0)
+ if (send_request(socket, &buf) < 0) {
+ if (errno != ENOENT)
die_errno("unable to connect to cache daemon");
+ if (flags & FLAG_SPAWN) {
+ spawn_daemon(socket);
+ if (send_request(socket, &buf) < 0)
+ die_errno("unable to connect to cache daemon");
+ }
}
strbuf_release(&buf);
}
--
1.7.9.rc0.33.g15ced5
^ permalink raw reply related
* Re: [PATCH v2 3/6] clone: factor out checkout code
From: Junio C Hamano @ 2012-01-10 4:59 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jeff King
In-Reply-To: <CACsJy8DZpA0sQ6ZYjgrp8PsRTsYm0nOfSXcDOEhB2TRjqwbM0Q@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 2012/1/10 Junio C Hamano <gitster@pobox.com>:
>>
>>> Read HEAD from disk instead of relying on local variable
>>> our_head_points_at, so that if earlier code fails to make HEAD
>>> properly, it'll be detected.
>>
>> The end result might be more or less the same with your patch from the
>> end-user's point of view, but "if earlier code fails", shouldn't you
>> detect and diagnose it right there?
>
> Sure, but another fence does not harm.
But that is not "another" fence but is the _only_ fence, as you do not
check after running update_ref of "HEAD".
> There's also one thing I missed in the commit message that it makes
> update head code and checkout code more independent. Update head code
> does not need to maintain our_head_points_at at the end for checkout
> anymore.
I like that reasoning in general. The logic ought to be:
- Learn what the remote has;
- Combine it with --branch parameter, determine what local branch our
head _should_ point at;
- Make our head point at it, and check it out.
I wonder if we can somehow make the above logic more clear in the
code. Perhaps the first two could be made into a single helper function
"decide_local_branch()", and the third would be the "checkout()" function
in your patch, updated to take "const char *" parameter or something?
> The lack of HEAD probably won't happen because HEAD is created by
> default in init-db. This is mainly to catch invalid HEAD (like putting
> "refs/tags/something" in HEAD).
Sorry; what I meant by "lack" was "... if earlier code fails to make HEAD
properly" case.
^ permalink raw reply
* Re: leaky cherry-pick
From: Ramkumar Ramachandra @ 2012-01-10 5:28 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20120109223737.GA1589@padd.com>
Hi,
Pete Wyckoff wrote:
> ==18789== HEAP SUMMARY:
> ==18789== in use at exit: 511,786,999 bytes in 3,954,180 blocks
> ==18789== total heap usage: 6,352,564 allocs, 2,398,384 frees, 2,610,529,433 bytes allocated
This is disturbing, to say the least. Let me try to chomp through the
Valgrind output to see what I understand.
[Re-ordering for convenience]
> [...]
Ignoring small losses from strdup(), parse_args() and other unrelated
things for the moment.
> ==18789== 16 bytes in 1 blocks are definitely lost in loss record 14 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x49578D: commit_list_insert (commit.c:356)
> ==18789== by 0x495886: commit_list_insert_by_date (commit.c:390)
> ==18789== by 0x4F51BC: commit_list_insert_by_date_cached (revision.c:508)
> ==18789== by 0x4F5344: add_parents_to_list (revision.c:550)
> ==18789== by 0x4F5C62: limit_list (revision.c:836)
> ==18789== by 0x4FA49B: prepare_revision_walk (revision.c:2068)
> ==18789== by 0x47522F: prepare_revs (revert.c:650)
> ==18789== by 0x475B6D: walk_revs_populate_todo (revert.c:855)
> ==18789== by 0x4766BF: pick_revisions (revert.c:1123)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789==
> ==18789== 144 (16 direct, 128 indirect) bytes in 1 blocks are definitely lost in loss record 41 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x47536C: commit_list_append (revert.c:692)
> ==18789== by 0x475B8A: walk_revs_populate_todo (revert.c:859)
> ==18789== by 0x4766BF: pick_revisions (revert.c:1123)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789== by 0x40558E: run_argv (git.c:513)
> ==18789== by 0x40571B: main (git.c:588)
Ugh, I never free the commit_list I build up in commit_list_append().
Rough sketch of fix (caution: untested):
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
diff --git a/builtin/revert.c b/builtin/revert.c
index 0d8020c..76be0e3 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -770,8 +770,11 @@ static int parse_insn_buffer(char *buf, struct
commit_list **todo_list,
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
commit = parse_insn_line(p, eol, opts);
- if (!commit)
+ if (!commit) {
+ if (*todo_list)
+ free_commit_list(*todo_list);
return error(_("Could not parse line %d."), i);
+ }
next = commit_list_append(commit, next);
p = *eol ? eol + 1 : eol;
}
@@ -1020,14 +1023,17 @@ static int pick_commits(struct commit_list
*todo_list, struct replay_opts *opts)
for (cur = todo_list; cur; cur = cur->next) {
save_todo(cur, opts);
res = do_pick_commit(cur->item, opts);
- if (res)
+ if (res) {
+ free_commit_list(todo_list);
return res;
+ }
}
/*
* Sequence of picks finished successfully; cleanup by
* removing the .git/sequencer directory
*/
+ free_commit_list(todo_list);
remove_sequencer_state();
return 0;
}
--
> ==18789== 1,728 bytes in 9 blocks are definitely lost in loss record 67 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x4C27927: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FFCD: xrealloc (wrapper.c:82)
> ==18789== by 0x488D5B: handle_attr_line (attr.c:325)
> ==18789== by 0x488DDD: read_attr_from_array (attr.c:340)
> ==18789== by 0x48924E: bootstrap_attr_stack (attr.c:501)
> ==18789== by 0x48940F: prepare_attr_stack (attr.c:568)
> ==18789== by 0x489A27: collect_all_attrs (attr.c:725)
> ==18789== by 0x489AED: git_check_attr (attr.c:739)
> ==18789== by 0x49E314: convert_attrs (convert.c:730)
> ==18789== by 0x49F230: get_stream_filter (convert.c:1247)
> ==18789== by 0x4B69B1: write_entry (entry.c:191)
> ==18789==
> ==18789== 7,726 (4,320 direct, 3,406 indirect) bytes in 9 blocks are definitely lost in loss record 77 of 97
> ==18789== at 0x4C27882: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FFCD: xrealloc (wrapper.c:82)
> ==18789== by 0x488D5B: handle_attr_line (attr.c:325)
> ==18789== by 0x488E86: read_attr_from_file (attr.c:358)
> ==18789== by 0x489130: read_attr (attr.c:428)
> ==18789== by 0x489335: bootstrap_attr_stack (attr.c:525)
> ==18789== by 0x48940F: prepare_attr_stack (attr.c:568)
> ==18789== by 0x489A27: collect_all_attrs (attr.c:725)
> ==18789== by 0x489AED: git_check_attr (attr.c:739)
> ==18789== by 0x49E314: convert_attrs (convert.c:730)
> ==18789== by 0x49F230: get_stream_filter (convert.c:1247)
> ==18789== by 0x4B69B1: write_entry (entry.c:191)
Interesting- I wonder where .gitattributes parsing code fits into all
this. The purpose of bootstrap _attr_stack() is to populate
attr_stack for its callers. Lots of memory allocation happening in
handle_attr_line() -- that information is returned to
bootstrap_attr_stack(). We have to keep backtracking until that
information is provably useless and free it. Hm, convert_attrs() (in
convert.c) is a common caller in both cases, but the populated
attr_stack is local to attr.c; I wonder if this is the problem. If my
hunch is right, it should be a trivial fix (caution: untested):
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
diff --git a/attr.c b/attr.c
index 76b079f..12e3824 100644
--- a/attr.c
+++ b/attr.c
@@ -745,6 +745,7 @@ int git_check_attr(const char *path, int num,
struct git_attr_check *check)
check[i].value = value;
}
+ drop_attr_stack();
return 0;
}
--
> ==18789== 1,107 bytes in 9 blocks are definitely lost in loss record 62 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x519DCD: setup_unpack_trees_porcelain (unpack-trees.c:79)
> ==18789== by 0x4C8160: git_merge_trees (merge-recursive.c:235)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789== by 0x4747B8: do_recursive_merge (revert.c:425)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789==
> ==18789== 1,143 bytes in 9 blocks are definitely lost in loss record 63 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x519E8F: setup_unpack_trees_porcelain (unpack-trees.c:82)
> ==18789== by 0x4C8160: git_merge_trees (merge-recursive.c:235)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789== by 0x4747B8: do_recursive_merge (revert.c:425)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789==
> ==18789== 1,269 bytes in 9 blocks are definitely lost in loss record 64 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x519CE0: setup_unpack_trees_porcelain (unpack-trees.c:66)
> ==18789== by 0x4C8160: git_merge_trees (merge-recursive.c:235)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789== by 0x4747B8: do_recursive_merge (revert.c:425)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789==
> ==18789== 704 bytes in 8 blocks are definitely lost in loss record 58 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51C1F5: unpack_trees (unpack-trees.c:1063)
> ==18789== by 0x4A46BD: diff_cache (diff-lib.c:474)
> ==18789== by 0x4A46FC: run_diff_index (diff-lib.c:482)
> ==18789== by 0x4A48DA: index_differs_from (diff-lib.c:517)
> ==18789== by 0x474AC2: do_pick_commit (revert.c:503)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789==
> ==18789== 1,316 bytes in 10 blocks are possibly lost in loss record 65 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789==
> ==18789== 2,376 bytes in 27 blocks are definitely lost in loss record 72 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51C1F5: unpack_trees (unpack-trees.c:1063)
> ==18789== by 0x4C81C2: git_merge_trees (merge-recursive.c:241)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789== by 0x4747B8: do_recursive_merge (revert.c:425)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789==
> ==18789== 744,353 bytes in 7,493 blocks are definitely lost in loss record 88 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51C1F5: unpack_trees (unpack-trees.c:1063)
> ==18789== by 0x4A46BD: diff_cache (diff-lib.c:474)
> ==18789== by 0x4A46FC: run_diff_index (diff-lib.c:482)
> ==18789==
> ==18789== 1,865,981 bytes in 18,806 blocks are definitely lost in loss record 90 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51C1F5: unpack_trees (unpack-trees.c:1063)
> ==18789== by 0x4C81C2: git_merge_trees (merge-recursive.c:241)
> ==18789== by 0x4CC907: merge_trees (merge-recursive.c:1817)
> ==18789==
> ==18789== 313,333,787 bytes in 2,509,382 blocks are definitely lost in loss record 97 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x51AFD1: create_ce_entry (unpack-trees.c:532)
> ==18789== by 0x51B1AE: unpack_nondirectories (unpack-trees.c:582)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
So many issues with unpack-trees! Let's try to look for some common
patterns, and see where the problems are occurring. I can see two
distinct patterns: backtraces that include
setup_unpack_trees_porcelain() and those that include
create_ce_entry(). Let's go with create_ce_entry() first: it
allocates quite a bit of memory according to cache.h:247. And it
returns the memory back to unpack_nondirectories(). What's worse?
unpack_nondirectories() calls create_ce_entry() in a loop and stuffs
all this new memory into a cache_entry provided by unpack_callback().
In the end, all the calls boil down to git_merge_trees(): I don't see
the ton of memory allocated higher in the callstack being passed down
in any way. I don't understand- why doesn't unpack_callback() clean
up the memory in "struct cache_entry *src" after it's done?
> ==18789== 281 bytes in 2 blocks are possibly lost in loss record 51 of 97
> ==18789== at 0x4C2779D: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FE4E: xmalloc (wrapper.c:35)
> ==18789== by 0x4E483E: create_from_disk (read-cache.c:1255)
> ==18789== by 0x4E4B7F: read_index_from (read-cache.c:1328)
> ==18789== by 0x4E4758: read_index (read-cache.c:1224)
> ==18789== by 0x47469F: do_recursive_merge (revert.c:411)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789==
> ==18789== 8,386,453 (1,031,576 direct, 7,354,877 indirect) bytes in 1 blocks are definitely lost in loss record 91 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x4E4B38: read_index_from (read-cache.c:1319)
> ==18789== by 0x4E4758: read_index (read-cache.c:1224)
> ==18789== by 0x4DB50F: read_index_preload (preload-index.c:105)
> ==18789== by 0x4752A1: read_and_refresh_cache (revert.c:661)
> ==18789== by 0x47657B: pick_revisions (revert.c:1081)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789== by 0x40558E: run_argv (git.c:513)
> ==18789== by 0x40571B: main (git.c:588)
> ==18789==
> ==18789== 69,782,106 (6,793,984 direct, 62,988,122 indirect) bytes in 8 blocks are definitely lost in loss record 94 of 97
> ==18789== at 0x4C27882: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x51FFCD: xrealloc (wrapper.c:82)
> ==18789== by 0x4E3D7C: add_index_entry (read-cache.c:976)
> ==18789== by 0x519FFE: add_entry (unpack-trees.c:119)
> ==18789== by 0x51D0A6: merged_entry (unpack-trees.c:1501)
> ==18789== by 0x51D444: threeway_merge (unpack-trees.c:1615)
> ==18789== by 0x51A645: call_unpack_fn (unpack-trees.c:289)
> ==18789== by 0x51B1E2: unpack_nondirectories (unpack-trees.c:586)
> ==18789== by 0x51B843: unpack_callback (unpack-trees.c:772)
> ==18789== by 0x5190F8: traverse_trees (tree-walk.c:407)
> ==18789== by 0x51AD8D: traverse_trees_recursive (unpack-trees.c:460)
> ==18789== by 0x51B973: unpack_callback (unpack-trees.c:809)
> ==18789==
> ==18789== 105,619,300 (9,284,184 direct, 96,335,116 indirect) bytes in 9 blocks are definitely lost in loss record 96 of 97
> ==18789== at 0x4C25E84: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==18789== by 0x520084: xcalloc (wrapper.c:98)
> ==18789== by 0x4E4B38: read_index_from (read-cache.c:1319)
> ==18789== by 0x4E4758: read_index (read-cache.c:1224)
> ==18789== by 0x47469F: do_recursive_merge (revert.c:411)
> ==18789== by 0x474FA5: do_pick_commit (revert.c:598)
> ==18789== by 0x47635D: pick_commits (revert.c:1022)
> ==18789== by 0x476756: pick_revisions (revert.c:1133)
> ==18789== by 0x47685A: cmd_cherry_pick (revert.c:1162)
> ==18789== by 0x4052E1: run_builtin (git.c:308)
> ==18789== by 0x405474: handle_internal_command (git.c:467)
> ==18789== by 0x40558E: run_argv (git.c:513)
Will investigate read-cache the context of Duy's comment- it looks
like I have to read through some more history to understand what's
going on.
Thanks.
-- Ram
^ permalink raw reply related
* Re: [PATCH] gitignore: warn about pointless syntax
From: Jan Engelhardt @ 2012-01-10 5:42 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, trast
In-Reply-To: <20120109223358.GA9902@sigill.intra.peff.net>
On Monday 2012-01-09 23:33, Jeff King wrote:
>On Mon, Jan 09, 2012 at 11:43:21AM -0800, Junio C Hamano wrote:
>
>>>>+static inline void check_bogus_wildcard(const char *file, const char *p)
>>>>+{
>>>>+ if (strstr(p, "**") == NULL)
>>>>+ return;
>>>>+ warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
>>>>+ "have a special meaning and is interpreted just like a single "
>>>>+ "asterisk.\n"), file, p);
>
>You only have to implement proper backslash decoding, so I think it is
>not as hard as reimplementing fnmatch:
>[...]
>
>That being said, if this is such a commonly-requested feature
Was it actually requested, or did you mean "commonly attempted use"?
As I see it, foo/**/*.o for example is equal to placing "*.o" in
foo/.gitignore, so the feature is already implemented, just not
through the syntax people falsely assume it is. And that is the
reason for wanting to output a warning. If it was me, I'd even make
it use error(), because that is the only way to educate people (and
it works), but alas, some on the list might consider that too harsh.
^ permalink raw reply
* Re: [PATCH v2 6/6] clone: print advice on checking out detached HEAD
From: Nguyen Thai Ngoc Duy @ 2012-01-10 5:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vlipgb1r1.fsf@alter.siamese.dyndns.org>
On Tue, Jan 10, 2012 at 11:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> 2012/1/10 Junio C Hamano <gitster@pobox.com>:
>>> This patch makes 100% sense _if_ we let clone result in a repository with
>>> a detached HEAD, which I am not sure if it is a good idea, or if it is
>>> better to fail the attempt to clone to give incentive to the owner of the
>>> remote repository to fix it.
>>
>> Then a hostile remote can stop users from cloning his repository by
>> detaching HEAD? That's not nice.
>
> That's crazy talk. Why does anybody from a hostile remote to begin with?
The point is, why punish client for remote's problems? If I have to
talk to upstream and wait for them to fix their repository, I might as
well give up cloning and move on. It's OK to annoy users to the point
that they ask upstream for a fix, but we should not disallow clone in
that case.
--
Duy
^ permalink raw reply
* Re: git grep doesn't follow symbolic link
From: Ramkumar Ramachandra @ 2012-01-10 5:56 UTC (permalink / raw)
To: Bertrand BENOIT; +Cc: git
In-Reply-To: <CAPRVejc7xND_8Y=Pb5rYGEcaKYUaX7_WkSro-_EL8tTGxkfY3Q@mail.gmail.com>
Hi Bertrand,
Bertrand BENOIT wrote:
> When using git grep, symbolic links are not followed.
> Is it a wanted behavior ?
I'd imagine so: symbolic links are not portable across different file
systems; Git's internal representation of a symbolic link is a file
containing the path of the file to be linked to.
> I've not found information about that in documentation, so I do a report.
Hm, the description says:
Look for specified patterns in the tracked files in the work tree, blobs
registered in the index file, or blobs in given tree objects.
Hm, "tracked files in the work tree" is definitely sub-optimal: "blobs
corresponding to the tracked files in the work tree" is probably
better. Then again, I think the description is too cryptic for an
end-user: do you have any suggestions? Have we mentioned how Git
handles symbolic links anywhere in the documentation? If not, where
should this information go?
-- Ram
^ permalink raw reply
* Re: [PATCH v2 3/6] clone: factor out checkout code
From: Nguyen Thai Ngoc Duy @ 2012-01-10 5:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vhb04b1bc.fsf@alter.siamese.dyndns.org>
On Tue, Jan 10, 2012 at 11:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> There's also one thing I missed in the commit message that it makes
>> update head code and checkout code more independent. Update head code
>> does not need to maintain our_head_points_at at the end for checkout
>> anymore.
>
> I like that reasoning in general. The logic ought to be:
>
> - Learn what the remote has;
>
> - Combine it with --branch parameter, determine what local branch our
> head _should_ point at;
>
> - Make our head point at it, and check it out.
>
> I wonder if we can somehow make the above logic more clear in the
> code. Perhaps the first two could be made into a single helper function
> "decide_local_branch()", and the third would be the "checkout()" function
> in your patch, updated to take "const char *" parameter or something?
yeah, I split the first two into update_head() but dropped it for some
reasons I don't remember. That would make the main function easier to
follow. I'll look at it again.
--
Duy
^ permalink raw reply
* Re: [PATCH] gitignore: warn about pointless syntax
From: Junio C Hamano @ 2012-01-10 6:01 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: Jeff King, git, trast
In-Reply-To: <alpine.LNX.2.01.1201100639340.11534@frira.zrqbmnf.qr>
Jan Engelhardt <jengelh@medozas.de> writes:
> On Monday 2012-01-09 23:33, Jeff King wrote:
>>On Mon, Jan 09, 2012 at 11:43:21AM -0800, Junio C Hamano wrote:
>>
>>>>>+static inline void check_bogus_wildcard(const char *file, const char *p)
>>>>>+{
>>>>>+ if (strstr(p, "**") == NULL)
>>>>>+ return;
>>>>>+ warning(_("Pattern \"%s\" from file \"%s\": Double asterisk does not "
>>>>>+ "have a special meaning and is interpreted just like a single "
>>>>>+ "asterisk.\n"), file, p);
>>
>>You only have to implement proper backslash decoding, so I think it is
>>not as hard as reimplementing fnmatch:
>>[...]
>>
>>That being said, if this is such a commonly-requested feature
>
> Was it actually requested, or did you mean "commonly attempted use"?
>
> As I see it, foo/**/*.o for example is equal to placing "*.o" in
> foo/.gitignore, so the feature is already implemented, just not
> through the syntax people falsely assume it is.
You can either adjust the people, i.e. teach that their "false" assumption
is wrong and the feature they expect is available but not in a way that
they expect.
Or you can adjust the tool to match their expectation.
The point that Peff correctly read between my lines is that in real life,
people are harder to train than tools and often the latter is a better
approach, especially if it does not amount to too much more work than
doing the former.
^ permalink raw reply
* Re: [PATCH] gitignore: warn about pointless syntax
From: Jan Engelhardt @ 2012-01-10 7:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, trast
In-Reply-To: <7vd3asayfx.fsf@alter.siamese.dyndns.org>
On Tuesday 2012-01-10 07:01, Junio C Hamano wrote:
>>>That being said, if this is such a commonly-requested feature
>>
>>Was it actually requested, or did you mean "commonly attempted use"?
>>As I see it, foo/**/*.o for example is equal to placing "*.o" in
>>foo/.gitignore, so the feature is already implemented, just not
>>through the syntax people falsely assume it is.
>
>You can either adjust the people, i.e. teach that their "false" assumption
>is wrong and the feature they expect is available but not in a way that
>they expect. Or you can adjust the tool to match their expectation.
>[...]in real life, people are harder to train than tools[...]
Though, having one more way to do things leads to a certain mess at a
later point, if such mess is not already present. I am thinking here of
the precedent iptables option parser set by removing support for
exclamation marks in odd positions, as it was redundant ("more than 1
way"), was only supported by ~45% of all options and had to be
explicitly invoked at every callsite - so in fact was harder on users
than git would be for **. There were a few mails by people who could not
seem to read error messages, but overall, within 6-9 months, everything
was quiet again. So, that's the empiric result of what teaching-the-tool
would do.
^ permalink raw reply
* Re: [PATCH] gitignore: warn about pointless syntax
From: Jan Engelhardt @ 2012-01-10 7:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, trast
In-Reply-To: <alpine.LNX.2.01.1201100744340.17336@frira.zrqbmnf.qr>
On Tuesday 2012-01-10 08:01, Jan Engelhardt wrote:
>>
>>You can either adjust the people, i.e. teach that their "false" assumption
>>is wrong and the feature they expect is available but not in a way that
>>they expect. Or you can adjust the tool to match their expectation.
>>[...]in real life, people are harder to train than tools[...]
>
>Though, having one more way to do things leads to a certain mess at a
>later point, if such mess is not already present. I am thinking here of
>the precedent iptables option parser set by removing support for
>exclamation marks in odd positions, as it was redundant ("more than 1
>way"), was only supported by ~45% of all options and had to be
>explicitly invoked at every callsite - so in fact was harder on users
>than git would be for **. There were a few mails by people who could not
>seem to read error messages, but overall, within 6-9 months, everything
>was quiet again. So, that's the empiric result of what teaching-the-tool
>would do.
~ teaching-the-user would entail - it's factually problemfree.
^ permalink raw reply
* [BUG] gitattribute macro expansion oddity
From: Jeff King @ 2012-01-10 7:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Henrik Grubbström, git-dev
I'm seeing some very odd behavior with git's attribute expansion for
diffs. You can see it with this repository:
git clone git://github.com/libgit2/libgit2sharp.git
Try a diff of a non-binary file:
$ git show 2a0f4bf7 LibGit2Sharp/Configuration.cs
...
diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs
index 83cc9d6..9ab0b60 100644
--- a/LibGit2Sharp/Configuration.cs
+++ b/LibGit2Sharp/Configuration.cs
Looks OK. Now try a diff that also has a binary file (that is marked
such via gitattributes):
$ git show 2a0f4bf7 Lib/NativeBinaries/x86/git2.dll \
LibGit2Sharp/Configuration.cs
...
diff --git a/Lib/NativeBinaries/x86/git2.dll b/Lib/NativeBinaries/x86/git2.dll
index dab0d04..8de18ab 100644
Binary files a/Lib/NativeBinaries/x86/git2.dll and b/Lib/NativeBinaries/x86/git2.dll differ
diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs
index 83cc9d6..9ab0b60 100644
Binary files a/LibGit2Sharp/Configuration.cs and b/LibGit2Sharp/Configuration.cs differ
Now the Configuration.cs blobs appear binary!
It has nothing to do with pathspecs; if you do a non-limited diff of
2a0f4bf7, you'll see many of the files appear as binary. Running it
through the debugger, it looks like we are getting wrong diff attribute
values for later paths, as if the earlier lookups are somehow polluting
the attribute stack.
The gitattributes in this repository look reasonably sane, but even if
they were not, nothing should make a file have different attributes
based on other files that were diffed.
Bisection points to ec775c4 (attr: Expand macros immediately when
encountered., 2010-04-06), but it's too late for me to dig further
tonight. Cc'ing Junio as the author of the attr code and Henrik as the
author of ec775c4.
-Peff
^ permalink raw reply
* Re: [linux.conf.au] VCS Interoperability
From: Ramkumar Ramachandra @ 2012-01-10 8:48 UTC (permalink / raw)
To: David Michael Barr; +Cc: git, Junio C Hamano, Jonathan Nieder, Dmitry Ivankov
In-Reply-To: <CAFfmPPMH2643JMMZdVbOQJL7DB-DiRYQS8x0TqEaSbGmhMdBNw@mail.gmail.com>
Hi David,
David Michael Barr wrote:
> Next week, I'll be presenting a summary of the past 2 years work
> on improving svn interoperability for git.
> I'm requesting feedback from anyone who cares with regard to
> what they'd like to hear about.
Nice. As a lay person attending the conference, here are a few things
I think I'd like to hear:
- Why this project is so challenging compared to say, a git-hg bridge
or a git-darcs bridge. What makes Subversion especially hard to deal
with?
- What is the biggest motivation for developing the svnrdump/ svnrload
combination? Are there any other usecases for the tools?
- How has this project contributed to the development of the
fast-import infrastructure? Can these changes be used to improve
other/ future remote helpers?
- You've spoken about exporting Subversion history to Git so far, but
what about the reverse? Which parts of the picture are still missing?
Thanks.
-- Ram
^ 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