* [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
* Re: [BUG] gitattribute macro expansion oddity
From: Michael Haggerty @ 2012-01-10 9:01 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Henrik Grubbström, git-dev
In-Reply-To: <20120110070300.GA17086@sigill.intra.peff.net>
On 01/10/2012 08:03 AM, Jeff King wrote:
> 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: [...]
The problem has nothing with diffing; simply interrogating the attribute
values gives different results depending on the order of the files:
$ git check-attr --all Lib/NativeBinaries/x86/git2.dll
LibGit2Sharp/Configuration.cs
Lib/NativeBinaries/x86/git2.dll: binary: set
Lib/NativeBinaries/x86/git2.dll: diff: unset
Lib/NativeBinaries/x86/git2.dll: text: unset
LibGit2Sharp/Configuration.cs: binary: set
LibGit2Sharp/Configuration.cs: diff: unset
LibGit2Sharp/Configuration.cs: text: unset
LibGit2Sharp/Configuration.cs: crlf: set
$ git check-attr --all LibGit2Sharp/Configuration.cs
Lib/NativeBinaries/x86/git2.dll
LibGit2Sharp/Configuration.cs: diff: csharp
LibGit2Sharp/Configuration.cs: crlf: set
Lib/NativeBinaries/x86/git2.dll: binary: set
Lib/NativeBinaries/x86/git2.dll: diff: unset
Lib/NativeBinaries/x86/git2.dll: text: unset
It also doesn't depend on the fact that Lib/.gitattributes uses CRLF as
its EOL, nor does it depend on the use of the "binary" macro. However,
it does depend on the fact that the directory name "Lib" matches the
first part of the directory name "LibGit2Sharp". Here is a simplified
demonstration of the problem:
a=LibA/a.txt
b=Lib/b.bin
rm -rf foo
git init foo
cd foo
mkdir $(dirname $a) $(dirname $b)
touch $a $b
echo '*.txt foo' >.gitattributes
echo '* bar' >$(dirname $b)/.gitattributes
git add .
git commit -am 'Demonstrate problem'
echo '================================================='
git check-attr --all $b $a
echo '================================================='
git check-attr --all $a $b
echo '================================================='
The attributes of $a are different depending on what order $a and $b
appear in the "git check-attr" command line.
Changing the example to "a=foo/a.txt" makes the problem go away.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] gitignore: warn about pointless syntax
From: Thomas Rast @ 2012-01-10 9:44 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: Jeff King, Junio C Hamano, git
In-Reply-To: <alpine.LNX.2.01.1201100639340.11534@frira.zrqbmnf.qr>
Jan Engelhardt <jengelh@medozas.de> writes:
>
> 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(),
No, please don't even think about it. Having an error() there would
mean that git would be essentially useless in repositories that have
this mistake, and the user may not be in a position to fix the
repository!
(He could drag along a local change to that effect, which is highly
annoying if the repository is otherwise read-only for him.)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: git-send-email: bug with sendemail.multiedit
From: Thomas Rast @ 2012-01-10 9:56 UTC (permalink / raw)
To: Jean-Francois Dagenais; +Cc: Pierre Habouzit, pierre.habouzit, git
In-Reply-To: <1AC16B4B-8376-4A50-A900-BB8E704FAB82@gmail.com>
Jean-Francois Dagenais <jeff.dagenais@gmail.com> writes:
> Bonjour Pierre! ... and all git developers!
>
> I think there is a bug with git-send-email.perl's evaluation of the sendemail.multiedit config variable.
>
> I was only able to make the "do_edit()" function detect it as false by setting the variable to "0" instead
> of "false", like so:
>
> git config --global sendemail.multiedit 0
>
> otherwise do_edit evaluates it as true and invokes the editor with all files as argument.
The patch below looks like the obvious fix. Perhaps you can test it?
diff --git i/git-send-email.perl w/git-send-email.perl
index d491db9..7ac0a7d 100755
--- i/git-send-email.perl
+++ w/git-send-email.perl
@@ -210,6 +210,7 @@ sub do_edit {
"signedoffbycc" => [\$signed_off_by_cc, undef],
"signedoffcc" => [\$signed_off_by_cc, undef], # Deprecated
"validate" => [\$validate, 1],
+ "multiedit" => [\$multiedit, undef],
);
my %config_settings = (
@@ -227,7 +228,6 @@ sub do_edit {
"bcc" => \@bcclist,
"suppresscc" => \@suppress_cc,
"envelopesender" => \$envelope_sender,
- "multiedit" => \$multiedit,
"confirm" => \$confirm,
"from" => \$sender,
"assume8bitencoding" => \$auto_8bit_encoding,
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply related
* [PATCH v3 00/10] nd/clone-detached
From: Nguyễn Thái Ngọc Duy @ 2012-01-10 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <1326023188-15559-1-git-send-email-pclouds@gmail.com>
Compare to v2, this round does more refactoring, which makes
cmd_clone() looks easier to follow in the end, in my opinion.
There's also 7/10 that refuses --branch=<nonexistent>. I don't know if
I react too strong. The current behavior is fall back to remote's HEAD
(and detached HEAD if remote's HEAD is detached). Maybe we should only
refuse it when it leads to detached HEAD and let it fall back to
remote's HEAD otherwise.
The last two patches remain debatable. If we disallow detached HEAD
from new clones, perhaps we could put <tag>^{commit} to
refs/heads/master then drop the last patch. t3501.6, t5527.2, t5707.5,
t7406.29 likes to have detached HEAD, but those can be fixed.
Nguyễn Thái Ngọc Duy (10):
t5601: add missing && cascade
clone: write detached HEAD in bare repositories
clone: factor out checkout code
clone: factor out HEAD update code
clone: factor out remote ref writing
clone: delay cloning until after remote HEAD checking
clone: --branch=<branch> always means refs/heads/<branch>
clone: refuse to clone if --branch points to bogus ref
clone: allow --branch to take a tag
clone: print advice on checking out detached HEAD
Documentation/git-clone.txt | 5 +-
advice.c | 14 +++
advice.h | 1 +
builtin/checkout.c | 16 +---
builtin/clone.c | 252 +++++++++++++++++++++++++------------------
t/t5601-clone.sh | 40 ++++++-
t/t5706-clone-branch.sh | 8 +-
transport.c | 5 +-
8 files changed, 207 insertions(+), 134 deletions(-)
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply
* [PATCH v3 01/10] t5601: add missing && cascade
From: Nguyễn Thái Ngọc Duy @ 2012-01-10 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <1326023188-15559-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
t/t5601-clone.sh | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 87ee016..49821eb 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -9,9 +9,9 @@ test_expect_success setup '
rm -fr .git &&
test_create_repo src &&
(
- cd src
- >file
- git add file
+ cd src &&
+ >file &&
+ git add file &&
git commit -m initial
)
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH v3 02/10] clone: write detached HEAD in bare repositories
From: Nguyễn Thái Ngọc Duy @ 2012-01-10 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <1326023188-15559-1-git-send-email-pclouds@gmail.com>
If we don't write, HEAD is still at refs/heads/master as initialized
by init-db, which may or may not match remote's HEAD.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/clone.c | 9 +++------
t/t5601-clone.sh | 25 ++++++++++++++++++++++++-
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 86db954..8dde1ea 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -720,12 +720,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
}
} else if (remote_head) {
/* Source had detached HEAD pointing somewhere. */
- if (!option_bare) {
- update_ref(reflog_msg.buf, "HEAD",
- remote_head->old_sha1,
- NULL, REF_NODEREF, DIE_ON_ERR);
- our_head_points_at = remote_head;
- }
+ update_ref(reflog_msg.buf, "HEAD", remote_head->old_sha1,
+ NULL, REF_NODEREF, DIE_ON_ERR);
+ our_head_points_at = remote_head;
} else {
/* Nothing to checkout out */
if (!option_no_checkout)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 49821eb..e0b8db6 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -12,7 +12,10 @@ test_expect_success setup '
cd src &&
>file &&
git add file &&
- git commit -m initial
+ git commit -m initial &&
+ echo 1 >file &&
+ git add file &&
+ git commit -m updated
)
'
@@ -88,6 +91,26 @@ test_expect_success 'clone --mirror' '
'
+test_expect_success 'clone --mirror with detached HEAD' '
+
+ ( cd src && git checkout HEAD^ && git rev-parse HEAD >../expected ) &&
+ git clone --mirror src mirror.detached &&
+ ( cd src && git checkout - ) &&
+ GIT_DIR=mirror.detached git rev-parse HEAD >actual &&
+ test_cmp expected actual
+
+'
+
+test_expect_success 'clone --bare with detached HEAD' '
+
+ ( cd src && git checkout HEAD^ && git rev-parse HEAD >../expected ) &&
+ git clone --bare src bare.detached &&
+ ( cd src && git checkout - ) &&
+ GIT_DIR=bare.detached git rev-parse HEAD >actual &&
+ test_cmp expected actual
+
+'
+
test_expect_success 'clone --bare names the local repository <name>.git' '
git clone --bare src &&
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH v3 03/10] clone: factor out checkout code
From: Nguyễn Thái Ngọc Duy @ 2012-01-10 9:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <1326023188-15559-1-git-send-email-pclouds@gmail.com>
Read HEAD from disk instead of relying on local variable
our_head_points_at. This reduces complexity of cmd_clone() a little
bit.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/clone.c | 101 +++++++++++++++++++++++++++++++-----------------------
1 files changed, 58 insertions(+), 43 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 8dde1ea..100056d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -448,6 +448,63 @@ static void write_remote_refs(const struct ref *local_refs)
clear_extra_refs();
}
+static int checkout(void)
+{
+ unsigned char sha1[20];
+ char *head;
+ struct lock_file *lock_file;
+ struct unpack_trees_options opts;
+ struct tree *tree;
+ struct tree_desc t;
+ int err = 0, fd;
+
+ if (option_no_checkout)
+ return 0;
+
+ head = resolve_refdup("HEAD", sha1, 1, NULL);
+ if (!head) {
+ warning(_("remote HEAD refers to nonexistent ref, "
+ "unable to checkout.\n"));
+ return 0;
+ }
+ if (strcmp(head, "HEAD")) {
+ if (prefixcmp(head, "refs/heads/"))
+ die(_("HEAD not found below refs/heads!"));
+ }
+ free(head);
+
+ /* We need to be in the new work tree for the checkout */
+ setup_work_tree();
+
+ lock_file = xcalloc(1, sizeof(struct lock_file));
+ fd = hold_locked_index(lock_file, 1);
+
+ memset(&opts, 0, sizeof opts);
+ opts.update = 1;
+ opts.merge = 1;
+ opts.fn = oneway_merge;
+ opts.verbose_update = (option_verbosity > 0);
+ opts.src_index = &the_index;
+ opts.dst_index = &the_index;
+
+ tree = parse_tree_indirect(sha1);
+ parse_tree(tree);
+ init_tree_desc(&t, tree->buffer, tree->size);
+ unpack_trees(1, &t, &opts);
+
+ if (write_cache(fd, active_cache, active_nr) ||
+ commit_locked_index(lock_file))
+ die(_("unable to write new index file"));
+
+ err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
+ sha1_to_hex(sha1), "1", NULL);
+
+ if (!err && option_recursive)
+ err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+
+ return err;
+}
+
static int write_one_config(const char *key, const char *value, void *data)
{
return git_config_set_multivar(key, value ? value : "true", "^$", 0);
@@ -722,13 +779,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
/* Source had detached HEAD pointing somewhere. */
update_ref(reflog_msg.buf, "HEAD", remote_head->old_sha1,
NULL, REF_NODEREF, DIE_ON_ERR);
- our_head_points_at = remote_head;
- } else {
- /* Nothing to checkout out */
- if (!option_no_checkout)
- warning(_("remote HEAD refers to nonexistent ref, "
- "unable to checkout.\n"));
- option_no_checkout = 1;
}
if (transport) {
@@ -736,42 +786,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
transport_disconnect(transport);
}
- if (!option_no_checkout) {
- struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
- struct unpack_trees_options opts;
- struct tree *tree;
- struct tree_desc t;
- int fd;
-
- /* We need to be in the new work tree for the checkout */
- setup_work_tree();
-
- fd = hold_locked_index(lock_file, 1);
-
- memset(&opts, 0, sizeof opts);
- opts.update = 1;
- opts.merge = 1;
- opts.fn = oneway_merge;
- opts.verbose_update = (option_verbosity > 0);
- opts.src_index = &the_index;
- opts.dst_index = &the_index;
-
- tree = parse_tree_indirect(our_head_points_at->old_sha1);
- parse_tree(tree);
- init_tree_desc(&t, tree->buffer, tree->size);
- unpack_trees(1, &t, &opts);
-
- if (write_cache(fd, active_cache, active_nr) ||
- commit_locked_index(lock_file))
- die(_("unable to write new index file"));
-
- err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
- sha1_to_hex(our_head_points_at->old_sha1), "1",
- NULL);
-
- if (!err && option_recursive)
- err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
- }
+ err = checkout();
strbuf_release(&reflog_msg);
strbuf_release(&branch_top);
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
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