No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
On 07/09/2014 11:57 PM, Junio C Hamano wrote:
Eric Wong normalper...@yhbt.net writes:
Junio C Hamano gits...@pobox.com wrote:
Johannes Sixt j...@kdbg.org writes:
Am 08.07.2014 21:34, schrieb Jens Lehmann:
And Msysgit complains
error: fchmod on c:/xxxt/trash
directory.t7613-merge-submodule/submodule_update_repo/.git/modules/sub1/config.lock
failed: Function not implemented
I'm not sure what this is about, seems to happen during the cp -R of
the repo under .git/modules into the submodule.
No. It happens because fchmod() is not implemented in our Windows port.
Please see my band-aid patch at
http://thread.gmane.org/gmane.comp.version-control.git/248154/focus=20266
The sub-thread ended inconclusive.
We need to start somewhere, and a no-op fchmod() in your patch may
be as a good place to start as anything. At least we would then
keep the old behaviour without introducing any new failure.
Right, this likely makes the most sense for single-user systems or
systesm without a *nix-like permission system.
An alternative might be to use chmod() after we are done writing to
the config.lock in order to avoid the use of fchmod() altogether,
which I think can replace the existing two callsites of fchmod().
That approach might be a more expedient, but may turn out to be
undesirable in the longer term.
In that case, we would need to open with mode=0600 to avoid a window
where the file may be world-readable with any data in it.
Yes, of course.
To elaborate what I was alluding to at the end of the message you
are responding to a bit more, if we were to move this grab perms
from existing file (if there is any) and propagate to the new one
into the lockfile API,
- in hold_lock_file_for_update(), we would record the permission of
the original file, if any, to a new field in struct lock_file;
- open with 0600 or tighter in lock_file(), and
- either before closing the file use fchmod() or after closing and
moving the file use chmod() to propagate the permission.
If the original did not exist, we would pass 0666 to open as before
in lock_file() and do not bother chmod/fchmod at the end.
Or something like that, perhaps.
Isn't the whole problem starting here:
in config.c:
fd = hold_lock_file_for_update(lock, config_filename, 0);
In lockfile.c:
/* This should return a meaningful errno on failure */
int hold_lock_file_for_update(struct lock_file *lk, const char *path,
int flags)
{
int fd = lock_file(lk, path, flags);
which leads to
static int lock_file(struct lock_file *lk, const char *path, int flags)
[]
lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
There is no way to tell which permissions the new lockfile should have.
That is somewhat unlucky.
On the other hand, shouldn't we call
adjust_shared_perm(const char *path) from path.c on the config file?
And to all files which are fiddled through the lock_file API?
In other words, the lockfile could be created with the restrictive
permissions
600, and once the lockfile had been closed and renamed into the final name
we apply adjust_shared_perm() on it ?
Or probably directly after close() ?
I think there are 2 different things missing here:
- Be able to specify permissions to hold_lock_file_for_update(),
especially restrictive ones, like 600 and not 666.
- Adjust the permissions for shared files in a shared repo.
This is probably needed for a shared repo, when the user itself
has a umask which is too restrictive and adjust_shared_perm()
must be run to widen the permissions.
Do I miss something ?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] use strbuf_addbuf for adding strbufs
Signed-off-by: Rene Scharfe l@web.de
---
builtin/log.c | 2 +-
pretty.c | 2 +-
rerere.c | 4 ++--
sha1_name.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 27c1b65..4389722 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -861,7 +861,7 @@ static void add_branch_description(struct strbuf *buf,
const char *branch_name)
read_branch_desc(desc, branch_name);
if (desc.len) {
strbuf_addch(buf, '\n');
- strbuf_add(buf, desc.buf, desc.len);
+ strbuf_addbuf(buf, desc);
strbuf_addch(buf, '\n');
}
}
diff --git a/pretty.c b/pretty.c
index 8d201f6..6e54934 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1376,7 +1376,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /*
in UTF-8 */
case trunc_none:
break;
}
- strbuf_addstr(sb, local_sb.buf);
+ strbuf_addbuf(sb, local_sb);
} else {
int sb_len = sb-len, offset = 0;
if (c-flush_type == flush_left)
diff --git a/rerere.c b/rerere.c
index d55aa8a..04d923d 100644
--- a/rerere.c
+++ b/rerere.c
@@ -207,11 +207,11 @@ static int handle_path(unsigned char *sha1, struct
rerere_io *io, int marker_siz
strbuf_reset(one);
strbuf_reset(two);
} else if (hunk == RR_SIDE_1)
- strbuf_addstr(one, buf.buf);
+ strbuf_addbuf(one, buf);
else if (hunk == RR_ORIGINAL)
; /* discard */
else if (hunk == RR_SIDE_2)
- strbuf_addstr(two, buf.buf);
+ strbuf_addbuf(two, buf);
else
rerere_io_putstr(buf.buf, io);
continue;
diff --git a/sha1_name.c b/sha1_name.c
index 5bfa841..6ccd3a5 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -946,7 +946,7 @@ static int interpret_nth_prior_checkout(const char *name,
int namelen,
retval = 0;
if (0 for_each_reflog_ent_reverse(HEAD, grab_nth_branch_switch,
cb)) {
strbuf_reset(buf);
- strbuf_add(buf, cb.buf.buf, cb.buf.len);
+ strbuf_addbuf(buf, cb.buf);
retval = brace - name + 1;
}
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] use strbuf_addch for adding single characters
Signed-off-by: Rene Scharfe l@web.de
---
merge-recursive.c | 2 +-
pathspec.c| 2 +-
url.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index b5c3c53..fad7b2c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -171,7 +171,7 @@ static void output(struct merge_options *o, int v, const
char *fmt, ...)
strbuf_vaddf(o-obuf, fmt, ap);
va_end(ap);
- strbuf_add(o-obuf, \n, 1);
+ strbuf_addch(o-obuf, '\n');
if (!o-buffer_output)
flush_output(o);
}
diff --git a/pathspec.c b/pathspec.c
index 8043099..89f2c8f 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -338,7 +338,7 @@ static void NORETURN unsupported_magic(const char *pattern,
if (!(magic m-bit))
continue;
if (sb.len)
- strbuf_addstr(sb, );
+ strbuf_addch(sb, ' ');
if (short_magic m-bit)
strbuf_addf(sb, '%c', m-mnemonic);
else
diff --git a/url.c b/url.c
index 335d97d..7ca2a69 100644
--- a/url.c
+++ b/url.c
@@ -121,7 +121,7 @@ void end_url_with_slash(struct strbuf *buf, const char *url)
{
strbuf_addstr(buf, url);
if (buf-len buf-buf[buf-len - 1] != '/')
- strbuf_addstr(buf, /);
+ strbuf_addch(buf, '/');
}
void str_end_url_with_slash(const char *url, char **dest) {
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 06/19] rebase -i: Stop on root commits with empty log messages
Hi Junio,
Junio C Hamano writes:
Fabian Ruch baf...@gmail.com writes:
The command line used to recreate root commits specifies the
erroneous option `--allow-empty-message`. If the root commit has an
empty log message, the replay of this commit should fail and the
rebase should be interrupted like for any other commit that is on the
to-do list and has an empty commit message. Remove the option.
The option might have been introduced by copy-and-paste of the first
part of the command line which initializes the authorship of the
sentinel commit. Indeed, the sentinel commit has an empty log message
and this should not trigger a failure, which is why the option
`--allow-empty-message` is correctly specified here.
The first commit --amend uses -C $1 to give the amended result
not just the authorship but also the log message taken from $1.
If we are allowing a commit without any message to be used as $1,
I think --allow-empty-message needs to be there. If $1 requires
the option here, why doesn't the second one, that records the
updated tree with the metainformation taken from the same commit
$1 can successfully commit without the option?
(I realize now that the emptiness of the sentinel log message is
irrelevant to the success of the first commit --amend since we are
amending using -C. I'll rewrite the second paragraph of the patch
description.)
The first commit --amend requires --allow-empty-message because we do
not want to fail without the authorship or log message of $1 being in
place. It's not a matter of allowing or disallowing empty log messages yet.
git-rebase--interactive can come across an empty log message in three
different ways, which are, depicted as to-do list tasks, the following.
1) pick --ff $1
2) pick --no-ff $1
3) reword $1
This patch is concerned with consistency in the second case.
git-rebase--root does not handle the first case yet and the third case
is handled somewhere else in the script independent of the first two.
The --root option handling was added to the script as a special case
later in the revision history. It's that option handling which
introduced the inconsistency that non-fast-forwards of commits with
empty log messages succeed if they are root commits but have always
failed otherwise.
Your reply suggests that git-rebase--interactive was wrong from the
beginning and that the replay of commits without any message should be
allowed. This would reconcile the first case with the second. In fact,
since neither of them alters the changes introduced by $1 or its log
message, it might be incorrect to complain about a missing message in
the first place.
Do you want me to replace this patch with a patch
rebase -i: Always allow picking of commits with empty log messages
that makes git-rebase--interactive cherry-pick commits using
--allow-empty-message? The script would still abort an empty reword with
the new patch and the user could then still force the empty log message
with git commit --amend --allow-empty-message.
Fabian
Puzzled...
Add test.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 2 +-
t/t3412-rebase-root.sh | 39 +++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4c875d5..0af96f2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -510,7 +510,7 @@ do_pick () {
git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1
pick_one -n $1
-git commit --allow-empty --allow-empty-message \
+git commit --allow-empty \
--amend --no-post-rewrite -n -q -C $1 \
${gpg_sign_opt:+$gpg_sign_opt} ||
die_with_patch $1 Could not apply $1... $2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 02/10] replace: add --graft option
On Wed, Jul 9, 2014 at 4:59 PM, Junio C Hamano gits...@pobox.com wrote:
Christian Couder chrisc...@tuxfamily.org writes:
+static void replace_parents(struct strbuf *buf, int argc, const char **argv)
+{
+ struct strbuf new_parents = STRBUF_INIT;
+ const char *parent_start, *parent_end;
+ int i;
+
+ /* find existing parents */
+ parent_start = buf-buf;
+ parent_start += 46; /* tree + hex sha1 + \n */
+ parent_end = parent_start;
+
+ while (starts_with(parent_end, parent ))
+ parent_end += 48; /* parent + hex sha1 + \n */
+
+ /* prepare new parents */
+ for (i = 1; i argc; i++) {
It looks somewhat strange that both replace_parents() and
create_graft() take familiar-looking argc, argv pair, but one
ignores argv[0] and uses the remainder and the other uses argv[0].
Shouldn't this function consume argv[] starting from [0] for
consistency? You'd obviously need to update the caller to adjust
the arguments it gives to this function.
Ok, will do.
+static int create_graft(int argc, const char **argv, int force)
+{
+ unsigned char old[20], new[20];
+ const char *old_ref = argv[0];
+...
+
+ replace_parents(buf, argc, argv);
+
+ if (write_sha1_file(buf.buf, buf.len, commit_type, new))
+ die(_(could not write replacement commit for: '%s'),
old_ref);
+
+ strbuf_release(buf);
+
+ if (!hashcmp(old, new))
+ return error(new commit is the same as the old one: '%s',
sha1_to_hex(old));
Is this really an error? It may be a warning-worthy situation for a
user or a script to end up doing a no-op graft, e.g.
git replace --graft HEAD HEAD^
but I wonder if it is more convenient to signal an error (like this
patch does) or just ignore the request and return without adding the
replace ref.
As the user might expect that a new replace ref was created on success
(0 exit code), and as we should at least warn if we would create a
commit that is the same as an existing one, I think it is just simpler
to error out in this case.
Though maybe we could use a special exit code (for example 2) in this
case, so that the user might more easily ignore this error in a
script.
Other than these two points, looks good to me.
Thanks,
Christian.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/2] add `config_set` API for caching config-like files
Junio C Hamano gits...@pobox.com writes: After thinking about it a bit more, I think file, line support needs to be done not as a mere client of the generic, uncached git_config() API that we have had for forever, like the current iteration, but needs to know a bit more about the state the caller of the callback (namely, git_parse_source()), and we obviously do not want to expose that machinery to anybody other than the implementation of the config subsystem (to which the new facility this GSoC project adds belongs to), so in that sense you have to have your code in the same config.c file anyway. I do not buy the argument. Why would callers of the callback-style API not be allowed to give file, line errors? To me, it is a weakness of the API that file, line information is not available to callback functions, regardless of the config-cache. It is somewhat dissapointing that this initial implementation was done as a client of the traditional git_config(), by the way. I would have expected that it would be the other way around, in that the traditional callers of git_config() would behefit automatically by having the cache layer below it. I disagree, and I agree ;-). I disagree that it is disapointing to use git_config(), and I think the beauty of the current implementation is to allow this cache mechanism without changing the parsing code. I agree that the callers of git_config() could benefit from the cache mechanism, i.e. use the in-memory pre-parsed config instead of re-parsing the file each time. But we have to start from somewhere. Perhaps the round after this one can rename the exisiting implementation of git_config() to something else internal to the caching layer and give the existing callers a compatible interface that is backed by this new caching layer in a transparent way. In PATCH v4, there was a git_config_iter function that did exactly that. I didn't notice it was gone for v5, but could be rather easily resurected. I suggest, on top of the current series: PATCH 3 : (re)introduce git_config_iter, compatible with git_config (one variant with a configset argument, another working on the_config_set). PATCH 4 : rename git_config to e.g. git_config_parse, and git_config_iter to git_config. Then, check that tests still pass (PATCH 4 automatically uses the new code essentially in every place where Git deals with config, so existing tests will start to stress the code a lot more), and check with e.g. strace -e open git ... that config files are now parsed only once where they used to be parsed multiple times. I'd do that as a separate series, to let the current one finally reach pu. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] merge: simplify merge_trivial() by using commit_list_append()
Build the commit_list of parents by calling commit_list_append() twice
instead of allocating and linking the items by hand. This makes the
code shorter and simpler. Rename the commit_list from parent to parents
(plural) while at it because there are two of them.
Signed-off-by: Rene Scharfe l@web.de
---
builtin/merge.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index b49c310..f50270e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -843,16 +843,14 @@ static void prepare_to_commit(struct commit_list
*remoteheads)
static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
{
unsigned char result_tree[20], result_commit[20];
- struct commit_list *parent = xmalloc(sizeof(*parent));
+ struct commit_list *parents, **pptr = parents;
write_tree_trivial(result_tree);
printf(_(Wonderful.\n));
- parent-item = head;
- parent-next = xmalloc(sizeof(*parent-next));
- parent-next-item = remoteheads-item;
- parent-next-next = NULL;
+ pptr = commit_list_append(head, pptr);
+ pptr = commit_list_append(remoteheads-item, pptr);
prepare_to_commit(remoteheads);
- if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parent,
+ if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,
result_commit, NULL, sign_commit))
die(_(failed to write commit object));
finish(head, remoteheads, result_commit, In-index merge);
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] commit: use commit_list_append() instead of duplicating its code
Signed-off-by: Rene Scharfe l@web.de
---
commit.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/commit.c b/commit.c
index fb7897c..61d2e13 100644
--- a/commit.c
+++ b/commit.c
@@ -447,12 +447,7 @@ struct commit_list *copy_commit_list(struct commit_list
*list)
struct commit_list *head = NULL;
struct commit_list **pp = head;
while (list) {
- struct commit_list *new;
- new = xmalloc(sizeof(struct commit_list));
- new-item = list-item;
- new-next = NULL;
- *pp = new;
- pp = new-next;
+ pp = commit_list_append(list-item, pp);
list = list-next;
}
return head;
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] fsck: simplify fsck_commit_buffer() by using commit_list_count()
fsck_commit_buffer() checks that the number of items in the parents
list of a commit matches the number of parent lines in its buffer or --
if a graft is used -- the number of parents in that graft. Simplify
the code by using commit_list_count() instead of counting by hand.
Also use different variables for the number of lines and the number of
list items, making it easier to compare them.
Signed-off-by: Rene Scharfe l@web.de
---
fsck.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/fsck.c b/fsck.c
index a4e8593..56156ff 100644
--- a/fsck.c
+++ b/fsck.c
@@ -281,7 +281,7 @@ static int fsck_commit_buffer(struct commit *commit, const
char *buffer,
{
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
- int parents = 0;
+ unsigned parent_count, parent_line_count = 0;
int err;
if (!skip_prefix(buffer, tree , buffer))
@@ -293,27 +293,17 @@ static int fsck_commit_buffer(struct commit *commit,
const char *buffer,
if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n')
return error_func(commit-object, FSCK_ERROR, invalid
'parent' line format - bad sha1);
buffer += 41;
- parents++;
+ parent_line_count++;
}
graft = lookup_commit_graft(commit-object.sha1);
+ parent_count = commit_list_count(commit-parents);
if (graft) {
- struct commit_list *p = commit-parents;
- parents = 0;
- while (p) {
- p = p-next;
- parents++;
- }
- if (graft-nr_parent == -1 !parents)
+ if (graft-nr_parent == -1 !parent_count)
; /* shallow commit */
- else if (graft-nr_parent != parents)
+ else if (graft-nr_parent != parent_count)
return error_func(commit-object, FSCK_ERROR, graft
objects missing);
} else {
- struct commit_list *p = commit-parents;
- while (p parents) {
- p = p-next;
- parents--;
- }
- if (p || parents)
+ if (parent_count != parent_line_count)
return error_func(commit-object, FSCK_ERROR, parent
objects missing);
}
if (!skip_prefix(buffer, author , buffer))
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/2] add `config_set` API for caching config-like files
Tanay Abhra tanay...@gmail.com writes: Noted, still I want to add that even when GSoC finishes, I won't leave the work unfinished. I had said that I wanted to continue being part of the Git community and I mean that. :) This is a good thing, but you shouldn't rely on it for your GSoC. After the GSoC finishes, you will have much less time for Git. I have to decide on what to do next on moving the contents to config.c or not. Seeing Junio's comments on the topic seems that he wasn't convinced in the first place that we needed a new file. What should we do, as I am sending the revised patch tomorrow? The move will be trivial, just cutting and pasting the contents. Other approaches you mentioned are also doable but, after a certain amount of retooling. I am open to any method you think would be best. No strong opinion from me here. I like splitting stuff into relatively small files, and to me it makes sense to keep the parsing code and the caching code separate (although config-hash.c is no longer a good name, config-set.c or config-cache.c would be better). But I can for sure live with both in the same file. I guess you'll have to make the decision if others don't give better argument ;-). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] .mailmap: Map different names with the same email address together
Pretty much one year ago (94b410bba864, Jul 12 2013, .mailmap: Map email addresses to names) I cleaned up the output of `git shortlog -sne` of git.git by writing a .mailmap file fot the git.git project. Now I find some time again for another review. During the year Jens, Kazuki and Trần contributed to git.git using different names, but the same email address. Would you mind to acknowledge this change to the mailmap file? Signed-off-by: Stefan Beller stefanbel...@googlemail.com --- .mailmap | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.mailmap b/.mailmap index 11057cb..2edbeb5 100644 --- a/.mailmap +++ b/.mailmap @@ -85,6 +85,7 @@ Jeff King p...@peff.net p...@github.com Jeff Muizelaar jmuizel...@mozilla.com j...@infidigm.net Jens Axboe ax...@kernel.dk ax...@suse.de Jens Axboe ax...@kernel.dk jens.ax...@oracle.com +Jens Lindström j...@opera.com Jens Lindstrom j...@opera.com Jim Meyering j...@meyering.net meyer...@redhat.com Joachim Berdal Haga cjh...@fys.uio.no Johannes Schindelin johannes.schinde...@gmx.de johannes.schinde...@gmx.de @@ -113,6 +114,7 @@ Karsten Blees bl...@dcon.de karsten.bl...@dcon.de Karsten Blees bl...@dcon.de karsten.bl...@gmail.com Kay Sievers kay.siev...@vrfy.org kay.siev...@suse.de Kay Sievers kay.siev...@vrfy.org kay@mam.(none) +Kazuki Saitoh ksaitoh...@gmail.com kazuki saitoh ksaitoh...@gmail.com Keith Cascio ke...@cs.ucla.edu ke...@cs.ucla.edu Kent Engstrom k...@lysator.liu.se Kevin Leung kevin...@gmail.com @@ -229,6 +231,7 @@ Tommi Virtanen t...@debian.org t...@inoi.fi Tommy Thorn tommy-...@thorn.ws tt1...@yahoo.com Tony Luck tony.l...@intel.com Tor Arne Vestbø tora...@gmail.com taves...@trolltech.com +Trần Ngọc Quân vnwild...@gmail.com Tran Ngoc Quan vnwild...@gmail.com Trent Piepho tpie...@gmail.com tpie...@freescale.com Trent Piepho tpie...@gmail.com xy...@speakeasy.org Uwe Kleine-König u.kleine-koe...@pengutronix.de uwe.kleine-koe...@digi.com -- 2.0.1.472.g6f92e5f -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 01/19] rebase -i: Failed reword prints redundant error message
On Tue, Jul 8, 2014 at 4:31 PM, Junio C Hamano gits...@pobox.com wrote: Fabian Ruch baf...@gmail.com writes: It is true that a failed hook script might not output any diagnosis... I think the message originally came from 0becb3e4 (rebase -i: interrupt rebase when commit --amend failed during reword, 2011-11-30); it seems that the primary point of the change was to make sure it exits and the warning message may not have been well thought out, but before discarding the result of somebody else's work, it may not be a bad idea to ask just in case you may have overlooked something (Andrew Wong Cc'ed). but then the generic message is not of much help either. Since this lack of information affects the built-in git commands for commit, merge and cherry-pick first, the solution would be to keep track of the failed hooks in their output so that the user knows which of her hooks require improvement. Since git commit already prints out error messages for failing due to empty commit message, the third message is really about giving hints in the case where pre-commit fails. We could probably assume that pre-commit would also print out error messages. So I'd be fine with removing the third message. But I wonder if we need to explain that the user needs to run git rebase --continue to resume the rebase? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 01/19] rebase -i: Failed reword prints redundant error message
Hi Andrew, thanks for your review and sorry that I forgot to cc the bug fix to you. Andrew Wong writes: On Tue, Jul 8, 2014 at 4:31 PM, Junio C Hamano gits...@pobox.com wrote: Fabian Ruch baf...@gmail.com writes: It is true that a failed hook script might not output any diagnosis... I think the message originally came from 0becb3e4 (rebase -i: interrupt rebase when commit --amend failed during reword, 2011-11-30); it seems that the primary point of the change was to make sure it exits and the warning message may not have been well thought out, but before discarding the result of somebody else's work, it may not be a bad idea to ask just in case you may have overlooked something (Andrew Wong Cc'ed). but then the generic message is not of much help either. Since this lack of information affects the built-in git commands for commit, merge and cherry-pick first, the solution would be to keep track of the failed hooks in their output so that the user knows which of her hooks require improvement. Since git commit already prints out error messages for failing due to empty commit message, the third message is really about giving hints in the case where pre-commit fails. We could probably assume that pre-commit would also print out error messages. So I'd be fine with removing the third message. But I wonder if we need to explain that the user needs to run git rebase --continue to resume the rebase? That is still taken care of by exit_with_patch here. When called in the error case, it prints You can amend the commit now, with git commit --amend Once you are satisfied with your changes, run git rebase --continue to standard error. I might have overlooked this in one of the later patches though. Fabian -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/2] add `config_set` API for caching config-like files
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: After thinking about it a bit more, I think file, line support needs to be done not as a mere client of the generic, uncached git_config() API that we have had for forever, like the current iteration, but needs to know a bit more about the state the caller of the callback (namely, git_parse_source()), and we obviously do not want to expose that machinery to anybody other than the implementation of the config subsystem (to which the new facility this GSoC project adds belongs to), so in that sense you have to have your code in the same config.c file anyway. I do not buy the argument. Why would callers of the callback-style API not be allowed to give file, line errors? To me, it is a weakness of the API that file, line information is not available to callback functions, regardless of the config-cache. I agree that it is good to allow scan-all-config-items-with-callback callers to learn file, line, but that is irrelevant. Perhaps you misread what I envision as the endgame of this thing to be and that is where our differences come from. One thing I think would be good to see in the endgame will be to give the benefit of the caching layer to the callback callers without having them change a single line of their code. One possible sequence of changes to make it happen would go like this, roughly in this order: - rename the current git_config() to git_config_raw(), and make it static to the config.c. - write a thin wrapper git_config() around git_config_raw() in config.c, until caching layer learns the iterator. - write caching layer to read from git_config_raw(). - teach git_config_raw() feed its callback functions to learn the file, line information. git_configset_get_type can then start using this in their error reporting. - implement iterator in the caching layer. - rewrite git_config() that was a thin wrapper around git_config_raw() by using the iterator over the cached values. - (optional) think about ways to give file, line information to the callback style callers. Perhaps we need git_config_2(). Perhaps we can rewrite all callers of git_config(). We do not have to decide it now. Between git_parse_source() and git_config_raw() we would need to pass the line-number information, but there is no reason for us to make public (all of these will be implementation details of the config system, including the config caching). My answer to why shouldn't the callbacks have file, line information? is there is no reason why they shouldn't. It is a good addition in the long run. But the optionality of the last step in the above list makes it an irrelevant question. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 02/10] replace: add --graft option
Christian Couder christian.cou...@gmail.com writes: Is this really an error? It may be a warning-worthy situation for a user or a script to end up doing a no-op graft, e.g. git replace --graft HEAD HEAD^ but I wonder if it is more convenient to signal an error (like this patch does) or just ignore the request and return without adding the replace ref. As the user might expect that a new replace ref was created on success (0 exit code), and as we should at least warn if we would create a commit that is the same as an existing one,... Why is it an event that needs a warning? I do not buy that as we should at least at all. If you say make sure A's parent is B and then you asked the same thing again when there already is a replacement in place, that should be a no-op. Making sure A's parent is B would be an idempotent operation, no? Why not just make sure A's parent is already B and report Your wish has been granted to the user? Why would it be simpler for the user to get an error, inspect the situation and realize that his wish has been granted after all? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 06/19] rebase -i: Stop on root commits with empty log messages
Fabian Ruch baf...@gmail.com writes: ... Do you want me to replace this patch with a patch rebase -i: Always allow picking of commits with empty log messages that makes git-rebase--interactive cherry-pick commits using --allow-empty-message? I do not want any particular behaviour change. I wanted you to clarify what changed behaviour you are trying to implement and why, and that was why I was asking these questions. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 01/19] rebase -i: Failed reword prints redundant error message
On Thu, Jul 10, 2014 at 12:35 PM, Fabian Ruch baf...@gmail.com wrote: That is still taken care of by exit_with_patch here. Oh, that's right. Ok, go ahead and remove the third warning then. Thanks! -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git p4 diff-tree ambiguous argument error
$ git p4 sync --detect-branches --import-labels //main@all ... Lots of useful information elided fatal: ambiguous argument 'git-p4-tmp/8031': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git command [revision...] -- [file...]' Command failed: ['git', 'diff-tree', '6b3ef26a3e2635a5ff0170e15fdadb386672f8b9', 'git-p4-tmp/8031'] If I re-run the command, it works the second time. Of course there are 73000+ commits. This is gonna take a while. I've done some debugging. It appears there is a timing problem between git-p4 and git. The failure occurs in P4Sync.searchParent(). Even though a checkpoint is sent to git (for fast-import) just prior to the call to searchParent() in importChanges(), the file does not yet exist. I used pdb, paused the program just before the call to diff-tree and the file was missing. After the program exits due to the error the file exists (i.e. the OS flushed the file). This is why re-running continues to work, there is an old file with basically the same information laying around (dangerous). How can I get git (fast-import) to flush the file at the right time? $ git --version git version 1.7.12.4 $ python --version Python 2.6.6 OS: GNU/Linux 2.6.32-431.el6.x86_64 -- View this message in context: http://git.661346.n2.nabble.com/git-p4-diff-tree-ambiguous-argument-error-tp7614774.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 06/19] rebase -i: Stop on root commits with empty log messages
Fabian Ruch baf...@gmail.com writes: Your reply suggests that git-rebase--interactive was wrong from the beginning and that the replay of commits without any message should be allowed. This would reconcile the first case with the second. In fact, since neither of them alters the changes introduced by $1 or its log message, it might be incorrect to complain about a missing message in the first place. ... Do you want me to replace this patch with a patch Now you explained your line of thought more clearly and I have thought about it a bit more, I think I agree with the conclusion of the above. An alternative may be to teach a new option --allow-empty-message to rebase to allow people to replay/reproduce an existing history with commits without any message, and uniformly tighten to refuse replaying a commit without message by default. But I am not sure if that is a good direction to go. Doesn't rebase without -i by default replay a commit without message faithfully? It might be debatable to allow a commit that we would normally would flag as suspicious (i.e. no change relative to its parent, or no log message) when replaying with reword or edit, but when replaying with pick, rebase -i should behave just like rebase without interactive. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 02/10] replace: add --graft option
Junio C Hamano gits...@pobox.com writes: Christian Couder christian.cou...@gmail.com writes: Is this really an error? It may be a warning-worthy situation for a user or a script to end up doing a no-op graft, e.g. git replace --graft HEAD HEAD^ but I wonder if it is more convenient to signal an error (like this patch does) or just ignore the request and return without adding the replace ref. As the user might expect that a new replace ref was created on success (0 exit code), and as we should at least warn if we would create a commit that is the same as an existing one,... Why is it an event that needs a warning? I do not buy that as we should at least at all. Ehh, it came a bit differently from what I meant. Perhaps s/do not buy/do not understand/ is closer to what I think---that is, it is not like I with a strong conviction think you are wrong. It is more like I do not understand why you think it needs a warning, meaing you would need to explain it better. If you say make sure A's parent is B and then you asked the same thing again when there already is a replacement in place, that should be a no-op. Making sure A's parent is B would be an idempotent operation, no? Why not just make sure A's parent is already B and report Your wish has been granted to the user? Why would it be simpler for the user to get an error, inspect the situation and realize that his wish has been granted after all? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tag: support configuring --sort via .gitconfig
Jeff King p...@peff.net writes:
I know this is existing code you are moving, but I noticed it looks ripe
for using skip_prefix. Perhaps while we are in the area we should do the
following on top of your patch (I'd also be happy for it be squashed
in, but that may be too much in one patch).
I am tempted to suggest going the other way around, i.e. queue (an
equivalent of) this on jk/skip-prefix and merge it to 'next' and
then 'master' quickly.
I can go either way, but I tend to prefer building new things on top
of obviously correct clean-up, not the other way around.
-- 8 --
Subject: [PATCH] tag: use skip_prefix instead of magic numbers
We can make the parsing of the --sort parameter a bit more
readable by having skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King p...@peff.net
builtin/tag.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..1101c19 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,14 @@ static int parse_opt_sort(const struct option *opt, const
char *arg, int unset)
int *sort = opt-value;
int flags = 0;
- if (*arg == '-') {
+ if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
- arg++;
- }
- if (starts_with(arg, version:)) {
- *sort = VERCMP_SORT;
- arg += 8;
- } else if (starts_with(arg, v:)) {
+
+ if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
*sort = VERCMP_SORT;
- arg += 2;
- } else
+ else
*sort = STRCMP_SORT;
+
if (strcmp(arg, refname))
die(_(unsupported sort specification %s), arg);
*sort |= flags;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git p4 diff-tree ambiguous argument error
Is this using NFS, or local storage? On 10/07/14 18:30, Bill Door wrote: $ git p4 sync --detect-branches --import-labels //main@all ... Lots of useful information elided fatal: ambiguous argument 'git-p4-tmp/8031': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git command [revision...] -- [file...]' Command failed: ['git', 'diff-tree', '6b3ef26a3e2635a5ff0170e15fdadb386672f8b9', 'git-p4-tmp/8031'] If I re-run the command, it works the second time. Of course there are 73000+ commits. This is gonna take a while. I've done some debugging. It appears there is a timing problem between git-p4 and git. The failure occurs in P4Sync.searchParent(). Even though a checkpoint is sent to git (for fast-import) just prior to the call to searchParent() in importChanges(), the file does not yet exist. I used pdb, paused the program just before the call to diff-tree and the file was missing. After the program exits due to the error the file exists (i.e. the OS flushed the file). This is why re-running continues to work, there is an old file with basically the same information laying around (dangerous). How can I get git (fast-import) to flush the file at the right time? $ git --version git version 1.7.12.4 $ python --version Python 2.6.6 OS: GNU/Linux 2.6.32-431.el6.x86_64 -- View this message in context: http://git.661346.n2.nabble.com/git-p4-diff-tree-ambiguous-argument-error-tp7614774.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git p4 diff-tree ambiguous argument error
All local storage. …Duane On Jul 10, 2014, at 11:07 AM, Luke Diamand l...@diamand.org wrote: Is this using NFS, or local storage? On 10/07/14 18:30, Bill Door wrote: $ git p4 sync --detect-branches --import-labels //main@all ... Lots of useful information elided fatal: ambiguous argument 'git-p4-tmp/8031': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git command [revision...] -- [file...]' Command failed: ['git', 'diff-tree', '6b3ef26a3e2635a5ff0170e15fdadb386672f8b9', 'git-p4-tmp/8031'] If I re-run the command, it works the second time. Of course there are 73000+ commits. This is gonna take a while. I've done some debugging. It appears there is a timing problem between git-p4 and git. The failure occurs in P4Sync.searchParent(). Even though a checkpoint is sent to git (for fast-import) just prior to the call to searchParent() in importChanges(), the file does not yet exist. I used pdb, paused the program just before the call to diff-tree and the file was missing. After the program exits due to the error the file exists (i.e. the OS flushed the file). This is why re-running continues to work, there is an old file with basically the same information laying around (dangerous). How can I get git (fast-import) to flush the file at the right time? $ git --version git version 1.7.12.4 $ python --version Python 2.6.6 OS: GNU/Linux 2.6.32-431.el6.x86_64 -- View this message in context: http://git.661346.n2.nabble.com/git-p4-diff-tree-ambiguous-argument-error-tp7614774.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git p4 diff-tree ambiguous argument error
Some additional investigation. I am working in a copy of a repository that was originally used to pull the data from Perforce. As part of my experiments to figure out this problem, I deleted the contents of .git/git-p4-tmp/. I noticed that git-p4 would continue if those files were present. I have now copied the files that were in .git/git-p4-tmp/ from the other repository. git-p4 is not crashing now, but I also noticed that none of the dates on these files have changed. These files should have been touched each time that a branch is taken, but these files have not changed while the sync is running. That seems significant. I expect git-p4 to crash again on a new commit that is not in .git/git-p4-tmp/. Then I have to start the 8-12 hour process over again (did I mention 70k commits?). …Duane On Jul 10, 2014, at 11:08 AM, Duane Murphy duanemur...@mac.com wrote: All local storage. …Duane On Jul 10, 2014, at 11:07 AM, Luke Diamand l...@diamand.org wrote: Is this using NFS, or local storage? On 10/07/14 18:30, Bill Door wrote: $ git p4 sync --detect-branches --import-labels //main@all ... Lots of useful information elided fatal: ambiguous argument 'git-p4-tmp/8031': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git command [revision...] -- [file...]' Command failed: ['git', 'diff-tree', '6b3ef26a3e2635a5ff0170e15fdadb386672f8b9', 'git-p4-tmp/8031'] If I re-run the command, it works the second time. Of course there are 73000+ commits. This is gonna take a while. I've done some debugging. It appears there is a timing problem between git-p4 and git. The failure occurs in P4Sync.searchParent(). Even though a checkpoint is sent to git (for fast-import) just prior to the call to searchParent() in importChanges(), the file does not yet exist. I used pdb, paused the program just before the call to diff-tree and the file was missing. After the program exits due to the error the file exists (i.e. the OS flushed the file). This is why re-running continues to work, there is an old file with basically the same information laying around (dangerous). How can I get git (fast-import) to flush the file at the right time? $ git --version git version 1.7.12.4 $ python --version Python 2.6.6 OS: GNU/Linux 2.6.32-431.el6.x86_64 -- View this message in context: http://git.661346.n2.nabble.com/git-p4-diff-tree-ambiguous-argument-error-tp7614774.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tag: support configuring --sort via .gitconfig
On Thu, Jul 10, 2014 at 10:59:36AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I know this is existing code you are moving, but I noticed it looks ripe for using skip_prefix. Perhaps while we are in the area we should do the following on top of your patch (I'd also be happy for it be squashed in, but that may be too much in one patch). I am tempted to suggest going the other way around, i.e. queue (an equivalent of) this on jk/skip-prefix and merge it to 'next' and then 'master' quickly. I can go either way, but I tend to prefer building new things on top of obviously correct clean-up, not the other way around. Me too. I just didn't want to make more work for Jacob (in having to rebase on top of mine) or for you (in having to do a non-obvious merge a few days from now). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
Torsten Bögershausen tbo...@web.de writes:
Isn't the whole problem starting here:
in config.c:
fd = hold_lock_file_for_update(lock, config_filename, 0);
In lockfile.c:
/* This should return a meaningful errno on failure */
int hold_lock_file_for_update(struct lock_file *lk, const char
*path, int flags)
{
int fd = lock_file(lk, path, flags);
which leads to
static int lock_file(struct lock_file *lk, const char *path, int flags)
[]
lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
There is no way to tell which permissions the new lockfile should have.
We follow whategver user's umask says with this code.
That is somewhat unlucky.
On the other hand, shouldn't we call
adjust_shared_perm(const char *path) from path.c on the config file?
Good question, but I am not sure. For $GIT_DIR/config, I tend to
agree we should, but git config --global foo bar would not be a
shared file anyway, and my understanding of Eric's original
motivation is to keep $HOME/.gitconfig to be tighter than the user's
umask normally would indicate.
And to all files which are fiddled through the lock_file API?
In other words, the lockfile could be created with the restrictive
permissions
600, and once the lockfile had been closed and renamed into the final name
we apply adjust_shared_perm() on it ?
For all files that adjust-shared-perm should apply, yes, but I do
not think it is relevant to the codepath in question.
I think there are 2 different things missing here:
- Be able to specify permissions to hold_lock_file_for_update(),
especially restrictive ones, like 600 and not 666.
Yes (in the sense that yes we can add an extra parameter) and no
(in the sense that where would we get the value to pass to the
extra parameter from? would it be worth to add configurations
variables for different kinds of files?).
If we limit the case to Inherit permissions from the file we are
replacing by taking a lock on it, which is the topic of discussion
in this thread, we do not have to worry about how to configure the
value (we do not have to) and adding a new parameter to tell the
mode to hold-lock-file-for-update is unneeded (the function will
have a pathname of the original and can learn the current permission
bits itself).
- Adjust the permissions for shared files in a shared repo.
This is probably needed for a shared repo, when the user itself
has a umask which is too restrictive and adjust_shared_perm()
must be run to widen the permissions.
Don't we already do that for $GIT_DIR/config? In any case that will
not help $HOME/.gitconfig and other files that are not shared.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
745224e0 gcc-4.9 emmintrin.h build error
The changes in 745224e0 don't seem to build here with gcc-4.9 on linux x64_64. Any idea what's wrong? CC credential-store.o In file included from /usr/lib/.../xmmintrin.h:31:0, from /usr/lib/.../emmintrin.h:31, from git-compat-util.h:708, from cache.h:4, from credential-store.c:1: /usr/lib/.../mmintrin.h: In function '_mm_cvtsi32_si64': /usr/lib/.../mmintrin.h:64:3: error: can't convert between vector values of different size return (__m64) __builtin_ia32_vec_init_v2si (__i, 0); ^ /usr/lib/.../mmintrin.h: In function '_mm_cvtsi64_si32': /usr/lib/.../mmintrin.h:107:10: error: incompatible type for argument 1 of '__builtin_ia32_vec_ext_v2si' return __builtin_ia32_vec_ext_v2si ((__v2si)__i, 0); ^ [...] In file included from /usr/lib/.../emmintrin.h:31:0, from git-compat-util.h:708, from cache.h:4, from credential-store.c:1: /usr/lib/.../xmmintrin.h: In function '_mm_add_ss': /usr/lib/.../xmmintrin.h:127:19: error: incompatible type for argument 1 of '__builtin_ia32_addss' return (__m128) __builtin_ia32_addss ((__v4sf)__A, (__v4sf)__B); ^ /usr/lib/.../xmmintrin.h:127:3: note: expected '__vector(4) float' but argument is of type '__m128' return (__m128) __builtin_ia32_addss ((__v4sf)__A, (__v4sf)__B); ^ /usr/lib/.../xmmintrin.h:127:19: error: incompatible type for argument 2 of '__builtin_ia32_addss' return (__m128) __builtin_ia32_addss ((__v4sf)__A, (__v4sf)__B); ^ [...] /usr/lib/.../emmintrin.h:1455:3: error: incompatible type for argument 2 of '__builtin_ia32_movntpd' __builtin_ia32_movntpd (__A, (__v2df)__B); ^ /usr/lib/.../emmintrin.h:1455:3: note: expected '__vector(2) double' but argument is of type '__m128d' Makefile:1983: recipe for target 'credential-store.o' failed make: *** [credential-store.o] Error 1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 15/19] rebase -i: Explicitly distinguish replay commands and exec tasks
Fabian Ruch baf...@gmail.com writes:
There are two kinds of to-do list commands available. One kind
replays a commit (`pick`, `reword`, `edit`, `squash` and `fixup` that
is) and the other executes a shell command (`exec`). We will call the
first kind replay commands.
The two kinds of tasks are scheduled using different line formats.
Replay commands expect a commit hash argument following the command
name and exec concatenates all arguments to assemble a command line.
Adhere to the distinction of formats by not trying to parse the
`sha1` field unless we are dealing with a replay command. Move the
replay command handling code to a new function `do_replay` which
assumes the first argument to be a commit hash and make no more such
assumptions in `do_next`.
In do_next(), we used read the first line of the insn sheet into
three variables, assuming the common case of handling one of the
replay commands, and (somewhat wastefully) re-read the same line
into two variables when we realize that the command was exec.
After you split do_replay() out of do_next() with this patch, we
seem to do exactly the same thing.
What exactly is the problem this change wants to fix?
Puzzled...
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 42 --
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 008f3a0..9de7441 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -585,13 +585,12 @@ do_pick () {
fi
}
-do_next () {
- rm -f $msg $author_script $amend || exit
- read -r command sha1 rest $todo
+do_replay () {
+ command=$1
+ sha1=$2
+ rest=$3
+
case $command in
- $comment_char*|''|noop)
- mark_action_done
- ;;
pick|p)
comment_for_reflog pick
@@ -656,6 +655,28 @@ do_next () {
esac
record_in_rewritten $sha1
;;
+ *)
+ read -r command $todo
+ warn Unknown command: $command
+ fixtodo=Please fix this using 'git rebase --edit-todo'.
+ if git rev-parse --verify -q $sha1 /dev/null
+ then
+ die_with_patch $sha1 $fixtodo
+ else
+ die $fixtodo
+ fi
+ ;;
+ esac
+}
+
+do_next () {
+ rm -f $msg $author_script $amend || exit
+ read -r command sha1 rest $todo
+
+ case $command in
+ $comment_char*|''|noop)
+ mark_action_done
+ ;;
x|exec)
read -r command rest $todo
mark_action_done
@@ -695,14 +716,7 @@ do_next () {
fi
;;
*)
- warn Unknown command: $command $sha1 $rest
- fixtodo=Please fix this using 'git rebase --edit-todo'.
- if git rev-parse --verify -q $sha1 /dev/null
- then
- die_with_patch $sha1 $fixtodo
- else
- die $fixtodo
- fi
+ do_replay $command $sha1 $rest
;;
esac
test -s $todo return
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Topic sk/mingw-unicode-spawn-args breaks tests
It looks like I totally missed the topic sk/mingw-unicode-spawn-args. Now it's in master, and it breaks lots of test cases for me: t0050-filesystem t0110-urlmatch-normalization t4014-format-patch t4041-diff-submodule-option t4120-apply-popt t4201-shortlog t4205-log-pretty-formats t4209-log-pickaxe t4210-log-i18n (I killed the test run here) Am I doing something wrong? Does the topic depend on a particular version of MSYS (or DLL)? -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 745224e0 gcc-4.9 emmintrin.h build error
On Thu, Jul 10, 2014 at 9:59 PM, Tuncer Ayaz wrote: The changes in 745224e0 don't seem to build here with gcc-4.9 on linux x64_64. Any idea what's wrong? s/x64_64/x86_64/ Should have written amd64 to avoid the typo :). CC credential-store.o In file included from /usr/lib/.../xmmintrin.h:31:0, from /usr/lib/.../emmintrin.h:31, from git-compat-util.h:708, from cache.h:4, from credential-store.c:1: /usr/lib/.../mmintrin.h: In function '_mm_cvtsi32_si64': /usr/lib/.../mmintrin.h:64:3: error: can't convert between vector values of different size return (__m64) __builtin_ia32_vec_init_v2si (__i, 0); ^ /usr/lib/.../mmintrin.h: In function '_mm_cvtsi64_si32': /usr/lib/.../mmintrin.h:107:10: error: incompatible type for argument 1 of '__builtin_ia32_vec_ext_v2si' return __builtin_ia32_vec_ext_v2si ((__v2si)__i, 0); ^ [...] In file included from /usr/lib/.../emmintrin.h:31:0, from git-compat-util.h:708, from cache.h:4, from credential-store.c:1: /usr/lib/.../xmmintrin.h: In function '_mm_add_ss': /usr/lib/.../xmmintrin.h:127:19: error: incompatible type for argument 1 of '__builtin_ia32_addss' return (__m128) __builtin_ia32_addss ((__v4sf)__A, (__v4sf)__B); ^ /usr/lib/.../xmmintrin.h:127:3: note: expected '__vector(4) float' but argument is of type '__m128' return (__m128) __builtin_ia32_addss ((__v4sf)__A, (__v4sf)__B); ^ /usr/lib/.../xmmintrin.h:127:19: error: incompatible type for argument 2 of '__builtin_ia32_addss' return (__m128) __builtin_ia32_addss ((__v4sf)__A, (__v4sf)__B); ^ [...] /usr/lib/.../emmintrin.h:1455:3: error: incompatible type for argument 2 of '__builtin_ia32_movntpd' __builtin_ia32_movntpd (__A, (__v2df)__B); ^ /usr/lib/.../emmintrin.h:1455:3: note: expected '__vector(2) double' but argument is of type '__m128d' Makefile:1983: recipe for target 'credential-store.o' failed make: *** [credential-store.o] Error 1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] http: Add Accept-Language header if possible
On Wed, Jul 09, 2014 at 11:46:14AM +0100, Peter Krefting wrote: Jeff King: I did some digging, and I think the public API is setlocale with a NULL parameter, like: printf(%s\n, setlocale(LC_MESSAGES, NULL)); That still will end up like en_US.UTF-8, though; And it only yields the highest-priority language, I think. I wasn't clear on whether POSIX locale variables actually supported multiple languages with priorities. I have never seen that, though the original commit message indicated that LANGUAGE=x:y was a thing (I wasn't sure if that was a made-up thing, or something that libc actually supported). Debian's website has a nice writeup on the subject: http://www.debian.org/intro/cn#howtoset That seems to be about language settings in browsers, which are a much richer set of preferences than POSIX locales (I think). It would not be wrong to have that level of configuration for git's http requests, but I do not know if it is worth the effort. Mapping the user's gettext locale into an accept-language header seems like a straightforward way to communicate to the other side what the client is using to show errors (so that errors coming from the server can match). If that is the case, though, I wonder if we should actually be adding it as a git-protocol header so that all transports can benefit (i.e., we could be localizing human-readable error messages in upload-pack, receive-pack, etc). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 745224e0 gcc-4.9 emmintrin.h build error
On Thu, Jul 10, 2014 at 09:59:37PM +0200, Tuncer Ayaz wrote: The changes in 745224e0 don't seem to build here with gcc-4.9 on linux x64_64. Any idea what's wrong? Weird. It compiles fine on my x86_64 box (Debian unstable, gcc 4.9.0-10). Can you tell us more about your environment? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 745224e0 gcc-4.9 emmintrin.h build error
On Thu, 2014-07-10 at 21:59 +0200, Tuncer Ayaz wrote: The changes in 745224e0 don't seem to build here with gcc-4.9 on linux x64_64. Any idea what's wrong? CC credential-store.o In file included from /usr/lib/.../xmmintrin.h:31:0, What's in the ...? Because if you're using headers from a different version of gcc, that might explain it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: State coding guideline for error message punctuation
Jeff King p...@peff.net writes: On Mon, Jun 16, 2014 at 01:55:57PM +0100, Philip Oakley wrote: +Error Messages + + - We typically do not end error messages with a full stop. While + we've been rather inconsistent in the past, these days we mostly + settle on no punctuation. Unlike Junio, I do not mind spelling out guidance for error messages. However, I do not think the second sentence is adding anything here (everything in CodingGuidelines is subject to we did not always do it this way, but this is the preferred way now). So I'd drop it. And then add in more guidance. Besides no full stop, probably: 1. do not capitalize (unable to open %s, not Unable to open %s 2. maybe something on sentence structure / ordering? We tend to prefer cannot open 'foo': No such file or directory to foo: cannot open: No such file or directory. Perhaps there are others (we do not have to be exhaustive, but it makes sense to think for a moment while we are here). I do not want to forever be waiting for a reroll, so let's queue this and advance it to 'next' soonish, and refine the guidelines by further building on top of it as needed. Thanks. -- 8 -- From: Philip Oakley philipoak...@iee.org Date: Mon, 16 Jun 2014 13:55:57 +0100 Subject: [PATCH] doc: give some guidelines for error messages Clarify error message puntuation to reduce review workload. Signed-off-by: Philip Oakley philipoak...@iee.org Helped-by: Jeff King p...@peff.net Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/CodingGuidelines | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index f424dbd..f4137c6 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -264,6 +264,15 @@ For Python scripts: documentation for version 2.6 does not mention this prefix, it has been supported since version 2.6.0. +Error Messages + + - Do not end error messages with a full stop. + + - Do not capitalize (unable to open %s, not Unable to open %s) + + - Say what the error is first (cannot open %s, not %s: cannot open) + + Writing Documentation: Most (if not all) of the documentation pages are written in the -- 2.0.1-751-ge540734 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tag: support configuring --sort via .gitconfig
On Thu, 2014-07-10 at 00:07 -0400, Jeff King wrote:
On Wed, Jul 09, 2014 at 03:36:51PM -0700, Jacob Keller wrote:
+static int parse_sort_string(const char *arg)
+{
+ int sort = 0;
+ int flags = 0;
+
+ if (*arg == '-') {
+ flags |= REVERSE_SORT;
+ arg++;
+ }
+ if (starts_with(arg, version:)) {
+ sort = VERCMP_SORT;
+ arg += 8;
+ } else if (starts_with(arg, v:)) {
+ sort = VERCMP_SORT;
+ arg += 2;
+ } else
+ sort = STRCMP_SORT;
+ if (strcmp(arg, refname))
+ die(_(unsupported sort specification %s), arg);
+ sort |= flags;
+
+ return sort;
+}
I know this is existing code you are moving, but I noticed it looks ripe
for using skip_prefix. Perhaps while we are in the area we should do the
following on top of your patch (I'd also be happy for it be squashed
in, but that may be too much in one patch).
I am fine with this being another patch or squashed in. I didn't even
know that was available :) I also like putting the two equivalent
conditionals into the same if block.
Thanks,
Jake
-- 8 --
Subject: [PATCH] tag: use skip_prefix instead of magic numbers
We can make the parsing of the --sort parameter a bit more
readable by having skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King p...@peff.net
---
builtin/tag.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index a679e32..016df98 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -353,18 +353,14 @@ static int parse_sort_string(const char *arg)
int sort = 0;
int flags = 0;
- if (*arg == '-') {
+ if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
- arg++;
- }
- if (starts_with(arg, version:)) {
- sort = VERCMP_SORT;
- arg += 8;
- } else if (starts_with(arg, v:)) {
+
+ if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
sort = VERCMP_SORT;
- arg += 2;
- } else
+ else
sort = STRCMP_SORT;
+
if (strcmp(arg, refname))
die(_(unsupported sort specification %s), arg);
sort |= flags;
Re: [PATCH] makefile: add ability to run specific test files
On Wed, 2014-07-09 at 21:14 -0700, Junio C Hamano wrote: On Wed, Jul 9, 2014 at 4:49 PM, Keller, Jacob E jacob.e.kel...@intel.com wrote: On Wed, 2014-07-09 at 15:59 -0700, Junio C Hamano wrote: What kind of things are missing, exactly? Perhaps that is something you need to fix, instead of mucking with the top-level Makefile. It uses the git from my environment instead of the git I have built, which is bad since I don't really want to run make install. Are you sure about that? Try adding something like die(I am broken); at the very beginning of main() in git.c, rebuild your git (i.e. make, not make install) and then $ cd t $ sh ./t1234-test.sh -v for any of the test scripts. You should see any test piece that runs git sees git dying with that message. Otherwise, there is something wrong with git you are building. Unless you have a patch or two to t/test-lib.sh or something that breaks the test framework, you should be able to test what you just have built without getting affected by what is installed in your $PATH. After all, that is how we bootstrap git from a tarball without any installed version, and friends do not force friends install without testing first ;-) Ok, I'll give it a shot. All I know for sure right now is running the test directly passed and running from make test it failed. I'll see if that makes any difference. Thanks, Jake
Re: [PATCH] doc: State coding guideline for error message punctuation
On Thu, Jul 10, 2014 at 01:36:05PM -0700, Junio C Hamano wrote: Perhaps there are others (we do not have to be exhaustive, but it makes sense to think for a moment while we are here). I do not want to forever be waiting for a reroll, so let's queue this and advance it to 'next' soonish, and refine the guidelines by further building on top of it as needed. Yes, that looks good to me. Thanks for moving this forward. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] makefile: add ability to run specific test files
On Thu, Jul 10, 2014 at 08:39:57PM +, Keller, Jacob E wrote: Ok, I'll give it a shot. All I know for sure right now is running the test directly passed and running from make test it failed. When you say directly, I assume you mean cd t ./1234-xxx.sh. You can also run a single-shot test like: cd t make t1234-... which may make the environment more like make test. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 745224e0 gcc-4.9 emmintrin.h build error
On Thu, Jul 10, 2014 at 10:23 PM, Jeff King wrote: On Thu, Jul 10, 2014 at 09:59:37PM +0200, Tuncer Ayaz wrote: The changes in 745224e0 don't seem to build here with gcc-4.9 on linux x64_64. Any idea what's wrong? Weird. It compiles fine on my x86_64 box (Debian unstable, gcc 4.9.0-10). Can you tell us more about your environment? gcc version 4.9.0 20140604 (prerelease) I normally use a custom config.mak, but I get the error without it too, so it has no direct effect. Let me know if there's anything to try out for finding the difference. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 745224e0 gcc-4.9 emmintrin.h build error
On Thu, Jul 10, 2014 at 10:33 PM, David Turner wrote: On Thu, 2014-07-10 at 21:59 +0200, Tuncer Ayaz wrote: The changes in 745224e0 don't seem to build here with gcc-4.9 on linux x64_64. Any idea what's wrong? CC credential-store.o In file included from /usr/lib/.../xmmintrin.h:31:0, What's in the ...? Because if you're using headers from a different version of gcc, that might explain it. /usr/lib/gcc/x86_64-unknown-linux-gnu/4.9.0/include/emmintrin.h -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] Add submodule test harness
Junio C Hamano gits...@pobox.com writes:
Jens Lehmann jens.lehm...@web.de writes:
I agree, but this case is special. The test asserts that nobody
added, modified or removed *anything* inside the .git directory.
The reason for problem we are seeing here is that I have to
remove the core.worktree setting when moving the git directory
from .git/modules into the submodule work tree.
Hmph. Comparing the files with core.worktree removed sounds like a
workaround that knows too much about the implementation detail of
what is being tested. I am just wondering if core.worktree will
stay forever be the only thing that is special, or there may come
other things (perhaps as a fallout of integrating things like Duy's
multiple-worktree stuff).
But perhaps we cannot do better than this.
One thing we should be able to do (and must do) better is to
validate that core.worktree in the relocated config file actually
points at the right place. Unsetting before comparing may let us
compare the relocated one in .git/modules/$1/config with the one
that is embedded in the working tree (hence no .git/config), but the
way your how about this? patch does, we wouldn't catch a possible
breakage to the relocation code to point core.worktree to a bogus
location, I'm afraid.
Perhaps squashing this to 7e8e5af9 instead?
t/lib-submodule-update.sh | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index e441b98..fc1da84 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -110,18 +110,23 @@ replace_gitfile_with_git_dir () {
}
# Test that the .git directory in the submodule is unchanged (except for the
-# core.worktree setting, which we temporarily restore). Call this function
-# before test_submodule_content as the latter might write the index file
-# leading to false positive index differences.
+# core.worktree setting, which appears only in $GIT_DIR/modules/$1/config).
+# Call this function before test_submodule_content as the latter might
+# write the index file leading to false positive index differences.
test_git_directory_is_unchanged () {
(
- cd $1
- git config core.worktree ../../../$1
+ cd .git/modules/$1
+ # does core.worktree point at the right place?
+ test $(git config core.worktree) = ../../../$1
+ # remove it temporarily before comparing, as
+ # $1/.git/config lacks it...
+ git config --unset core.worktree
)
diff -r .git/modules/$1 $1/.git
(
- cd $1
- GIT_WORK_TREE=. git config --unset core.worktree
+ # ... and then restore.
+ cd .git/modules/$1
+ git config core.worktree ../../../$1
)
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 745224e0 gcc-4.9 emmintrin.h build error
On Thu, 2014-07-10 at 22:44 +0200, Tuncer Ayaz wrote:
On Thu, Jul 10, 2014 at 10:33 PM, David Turner wrote:
On Thu, 2014-07-10 at 21:59 +0200, Tuncer Ayaz wrote:
The changes in 745224e0 don't seem to build here with gcc-4.9 on
linux x64_64. Any idea what's wrong?
CC credential-store.o
In file included from /usr/lib/.../xmmintrin.h:31:0,
What's in the ...?
Because if you're using headers from a different version of gcc, that
might explain it.
/usr/lib/gcc/x86_64-unknown-linux-gnu/4.9.0/include/emmintrin.h
That seems fine to me.
It looks like the error messages are coming from inside the system's
header files (but this is sometimes misleading). If you just try to
compile
#include emmintrin.h
int main() { }
with whatever options you use for git, does that work? If not, I would
say that you have a compiler setup problem.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
On 2014-07-10 21.49, Junio C Hamano wrote:
[]
If we limit the case to Inherit permissions from the file we are
replacing by taking a lock on it, which is the topic of discussion
in this thread, we do not have to worry about how to configure the
value (we do not have to) and adding a new parameter to tell the
mode to hold-lock-file-for-update is unneeded (the function will
have a pathname of the original and can learn the current permission
bits itself).
So something like this:
(I will probably not have the time to make a proper patch :-(
diff --git a/lockfile.c b/lockfile.c
index 4899270..134d5c8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -156,6 +156,11 @@ static void resolve_symlink(struct strbuf *path)
/* Make sure errno contains a meaningful value on error */
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
+ int perms = 0666;
+ struct stat st;
+ if (!lstat(path, st))
+ perms = st.st_mode 0777;
+
if (!lock_file_list) {
/* One-time initialization */
sigchain_push_common(remove_lock_file_on_signal);
@@ -179,7 +184,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
if (!(flags LOCK_NODEREF))
resolve_symlink(lk-filename);
strbuf_addstr(lk-filename, LOCK_SUFFIX);
- lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+ lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, perms);
if (lk-fd 0) {
strbuf_reset(lk-filename);
return -1;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
Torsten Bögershausen tbo...@web.de writes:
On 2014-07-10 21.49, Junio C Hamano wrote:
[]
If we limit the case to Inherit permissions from the file we are
replacing by taking a lock on it, which is the topic of discussion
in this thread, we do not have to worry about how to configure the
value (we do not have to) and adding a new parameter to tell the
mode to hold-lock-file-for-update is unneeded (the function will
have a pathname of the original and can learn the current permission
bits itself).
So something like this:
Yeah, I think something along those lines may be sufficient and we
do not have to do anything when closing/committing, at least POSIX
systems. I do not know if other filesystems we may care about let
you open with 0400 and still write into it, though.
(I will probably not have the time to make a proper patch :-(
That's OK. I see many names on Cc: who are all capable of helping
us ;-)
diff --git a/lockfile.c b/lockfile.c
index 4899270..134d5c8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -156,6 +156,11 @@ static void resolve_symlink(struct strbuf *path)
/* Make sure errno contains a meaningful value on error */
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
+ int perms = 0666;
+ struct stat st;
+ if (!lstat(path, st))
+ perms = st.st_mode 0777;
+
if (!lock_file_list) {
/* One-time initialization */
sigchain_push_common(remove_lock_file_on_signal);
@@ -179,7 +184,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
if (!(flags LOCK_NODEREF))
resolve_symlink(lk-filename);
strbuf_addstr(lk-filename, LOCK_SUFFIX);
- lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+ lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, perms);
if (lk-fd 0) {
strbuf_reset(lk-filename);
return -1;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] tag: support configuring --sort via .gitconfig
On Thu, 2014-07-10 at 15:34 -0400, Jeff King wrote:
On Thu, Jul 10, 2014 at 10:59:36AM -0700, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
I know this is existing code you are moving, but I noticed it looks ripe
for using skip_prefix. Perhaps while we are in the area we should do the
following on top of your patch (I'd also be happy for it be squashed
in, but that may be too much in one patch).
I am tempted to suggest going the other way around, i.e. queue (an
equivalent of) this on jk/skip-prefix and merge it to 'next' and
then 'master' quickly.
I can go either way, but I tend to prefer building new things on top
of obviously correct clean-up, not the other way around.
Me too. I just didn't want to make more work for Jacob (in having to
rebase on top of mine) or for you (in having to do a non-obvious merge a
few days from now).
-Peff
I'm perfectly fine rebasing. :)
Thanks,
Jake
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
Re: 745224e0 gcc-4.9 emmintrin.h build error
On Thu, Jul 10, 2014 at 10:53 PM, David Turner wrote:
On Thu, 2014-07-10 at 22:44 +0200, Tuncer Ayaz wrote:
On Thu, Jul 10, 2014 at 10:33 PM, David Turner wrote:
On Thu, 2014-07-10 at 21:59 +0200, Tuncer Ayaz wrote:
The changes in 745224e0 don't seem to build here with gcc-4.9 on
linux x64_64. Any idea what's wrong?
CC credential-store.o
In file included from /usr/lib/.../xmmintrin.h:31:0,
What's in the ...?
Because if you're using headers from a different version of gcc, that
might explain it.
/usr/lib/gcc/x86_64-unknown-linux-gnu/4.9.0/include/emmintrin.h
That seems fine to me.
It looks like the error messages are coming from inside the system's
header files (but this is sometimes misleading). If you just try to
compile
#include emmintrin.h
int main() { }
with whatever options you use for git, does that work? If not, I would
say that you have a compiler setup problem.
The above test works on the same machine, so I'll
investigate what's going on when building git. Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 745224e0 gcc-4.9 emmintrin.h build error
Sorry, didn't test properly when I tried with/without config.mak, and PROFILE=BUILD was the problem. I had that in config.mak based on information gathered from INSTALL and Makefile. To be clear, is PROFILE=BUILD (still) supported? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] log: correctly identify mergetag signature verification status
Jeff King p...@peff.net writes:
Perhaps it would be easier to read (and would have made the logic error
you are fixing more obvious) as:
if (extra-len payload_size) {
if (!verify_signed_buffer(...))
status = 0; /* good; all other code paths leave it as -1 */
else if (verify_message.len = gpg_message_offset)
strbuf_addstr(verify_message, No signature\n);
}
That is, for each conditional to check one more thing needed for a good
signature, and then know that all other code paths leave status as -1.
Thanks. Let's do it this way, then.
log-tree.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 1982631..b4bbfe1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -446,16 +446,17 @@ static void show_one_mergetag(struct rev_info *opt,
payload_size = parse_signature(extra-value, extra-len);
status = -1;
- if (extra-len payload_size)
- if (verify_signed_buffer(extra-value, payload_size,
-extra-value + payload_size,
-extra-len - payload_size,
-verify_message, NULL)) {
- if (verify_message.len = gpg_message_offset)
- strbuf_addstr(verify_message, No
signature\n);
- else
- status = 0;
- }
+ if (extra-len payload_size) {
+ /* could have a good signature */
+ if (!verify_signed_buffer(extra-value, payload_size,
+ extra-value + payload_size,
+ extra-len - payload_size,
+ verify_message, NULL))
+ status = 0; /* good */
+ else if (verify_message.len = gpg_message_offset)
+ strbuf_addstr(verify_message, No signature\n);
+ /* otherwise we couldn't verify, which is shown as bad */
+ }
show_sig_lines(opt, status, verify_message.buf);
strbuf_release(verify_message);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] makefile: add ability to run specific test files
On Thu, 2014-07-10 at 04:14 +, Junio C Hamano wrote: On Wed, Jul 9, 2014 at 4:49 PM, Keller, Jacob E jacob.e.kel...@intel.com wrote: On Wed, 2014-07-09 at 15:59 -0700, Junio C Hamano wrote: What kind of things are missing, exactly? Perhaps that is something you need to fix, instead of mucking with the top-level Makefile. It uses the git from my environment instead of the git I have built, which is bad since I don't really want to run make install. Are you sure about that? Try adding something like die(I am broken); at the very beginning of main() in git.c, rebuild your git (i.e. make, not make install) and then $ cd t $ sh ./t1234-test.sh -v for any of the test scripts. You should see any test piece that runs git sees git dying with that message. Otherwise, there is something wrong with git you are building. Unless you have a patch or two to t/test-lib.sh or something that breaks the test framework, you should be able to test what you just have built without getting affected by what is installed in your $PATH. After all, that is how we bootstrap git from a tarball without any installed version, and friends do not force friends install without testing first ;-) This is even more interesting. I tried your die check, and it definitely runs the correct version of git. However, if I run the test directly: cd t ; sh t3200-branch.sh -v it passes. if I run: make test that particular test fails. If I have this patch applied, and I run make t/t3200-branch.sh it also fails. I have done this directly on current master branch. So something is differing between the two test runs. Also, if I run: make -C t t3200-branch.sh that passes, so it really *is* something setup by the main makefile. Any more suggestions? Thanks, Jake
Re: [PATCH v2] tag: support configuring --sort via .gitconfig
Keller, Jacob E jacob.e.kel...@intel.com writes: On Thu, 2014-07-10 at 15:34 -0400, Jeff King wrote: On Thu, Jul 10, 2014 at 10:59:36AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I know this is existing code you are moving, but I noticed it looks ripe for using skip_prefix. Perhaps while we are in the area we should do the following on top of your patch (I'd also be happy for it be squashed in, but that may be too much in one patch). I am tempted to suggest going the other way around, i.e. queue (an equivalent of) this on jk/skip-prefix and merge it to 'next' and then 'master' quickly. I can go either way, but I tend to prefer building new things on top of obviously correct clean-up, not the other way around. Me too. I just didn't want to make more work for Jacob (in having to rebase on top of mine) or for you (in having to do a non-obvious merge a few days from now). -Peff I'm perfectly fine rebasing. :) Alright, thanks. I am still not ready to push out today's integration result, but when it happens, Peff's tag: use skip_prefix should appear at ce85604, as a direct follow-up to the tip of already merged jk/skip-prefix topic which was 67a31f61 (http-push: refactor parsing of remote object names, 2014-06-19). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 745224e0 gcc-4.9 emmintrin.h build error
On Fri, 2014-07-11 at 00:12 +0200, Tuncer Ayaz wrote:
Sorry, didn't test properly when I tried with/without config.mak, and
PROFILE=BUILD was the problem. I had that in config.mak based on
information gathered from INSTALL and Makefile. To be clear, is
PROFILE=BUILD (still) supported?
For what it's worth, the problem seems to depend on the combination of
-DNO_NOTRETURN=1 and -fprofile-use. So I can trigger the same breakage
with this:
---
#define NO_NORETURN 1
#include git-compat-util.h
int main() {}
---
gcc -I. -c -fprofile-use=/tmp foo.c -o foo.o
Do we still need NO_NORETURN? Git seems to build without it under GCC
4.6.3 (Ubuntu's version).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4] linuxptp: add phc_ctl program to help debug PHC devices
This is an updated version of a script I wrote a couple years ago for debugging the PHC when writing a new driver. I figured that it might be handy for the LinuxPTP project to include, as it can give some insight into the PHC directly. I have updated it to make use of the shared code here, in order to reduce duplication. Hopefully this is of some use to everyone. Signed-off-by: Jacob Keller jacob.e.kel...@intel.com --- -v4 * fix manpage warnings and incorrect display * add phc_ctl to .gitignore .gitignore | 1 + makefile | 4 +- phc_ctl.8 | 108 phc_ctl.c | 561 + 4 files changed, 673 insertions(+), 1 deletion(-) create mode 100644 phc_ctl.8 create mode 100644 phc_ctl.c diff --git a/.gitignore b/.gitignore index 098dcdfe1ea7..0ca22afe2b9a 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ /phc2sys /pmc /ptp4l +/phc_ctl diff --git a/makefile b/makefile index e36835b05d40..e9f2f8fcf416 100644 --- a/makefile +++ b/makefile @@ -22,7 +22,7 @@ CC= $(CROSS_COMPILE)gcc VER = -DVER=$(version) CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS) LDLIBS = -lm -lrt $(EXTRA_LDFLAGS) -PRG= ptp4l pmc phc2sys hwstamp_ctl +PRG= ptp4l pmc phc2sys hwstamp_ctl phc_ctl OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \ filter.o fsm.o linreg.o mave.o mmedian.o msg.o ntpshm.o phc.o \ pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \ @@ -54,6 +54,8 @@ phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o phc.o phc2sys.o pi.o \ hwstamp_ctl: hwstamp_ctl.o version.o +phc_ctl: phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o + version.o: .version version.sh $(filter-out version.d,$(DEPEND)) .version: force diff --git a/phc_ctl.8 b/phc_ctl.8 new file mode 100644 index ..06982690410e --- /dev/null +++ b/phc_ctl.8 @@ -0,0 +1,108 @@ +.TH PHC_CTL 8 June 2014 linuxptp +.SH NAME +phc_ctl \- directly control PHC device clock + +.SH SYNOPSIS +.B phc_ctl +[ options ] device [ commands ] + +.SH DESCRIPTION +.B phc_ctl +is a program which can be used to directly control a PHC clock device. +Typically, it is used for debugging purposes, and has little use for general +control of the device. For general control of PHC clock devices, +.B phc2sys (8) +should be preferred. + +device may be either CLOCK_REALTIME, any /dev/ptpX device, or any ethernet +device which supports ethtool's get_ts_info ioctl. + +.SH OPTIONS +.TP +.BI \-l print-level +Set the maximum syslog level of messages which should be printed or sent to the +system logger. The default is 6 (LOG_INFO). +.TP +.BI \-q +Do not send messages to syslog. By default messages will be sent. +.TP +.BI \-Q +Do not print messages to standard output. By default messages will be printed. +.TP +.BI \-h +Display a help message. +.TP +.B \-v +Prints the software version and exits. + +.SH COMMANDS + +.B phc_ctl +is controlled by passing commands which take either an optional or required +parameter. The commands (outlined below) will control aspects of the PHC clock +device. These commands may be useful for inspecting or debugging the PHC +driver, but may have adverse side effects on running instances of +.B ptp4l (8) +or +.B phc2sys (8) + +.TP +.BI set seconds +Set the PHC clock time to the value specified in seconds. Defaults to reading +CLOCK_REALTIME if no value is provided. +.TP +.BI get +Get the current time of the PHC clock device. +.TP +.BI adj seconds +Adjust the PHC clock by an amount of seconds provided. This argument is required. +.TP +.BI freq ppb +Adjust the frequency of the PHC clock by the specified parts per billion. If no +argument is provided, it will attempt to read the current frequency and report +it. +.TP +.BI cmp +Compare the PHC clock device to CLOCK_REALTIME, using the best method available. +.TP +.BI caps +Display the device capabiltiies. This is the default command if no commands are +provided. +.TP +.BI wait seconds +Sleep the process for the specified period of time, waking up and resuming +afterwards. This command may be useful for sanity checking whether the PHC +clock is running as expected. + +The arguments specified in seconds are read as double precision floating point +values, and will scale to nanoseconds. This means providing a value of 5.5 +means 5 and one half seconds. This allows specifying fairly precise values for time. + +.SH EXAMPLES + +Read the current clock time from the device +.RS +\f(CWphc_ctl /dev/ptp0 get\fP +.RE + +Set the PHC clock time to CLOCK_REALTIME +.RS +\f(CWphc_ctl /dev/ptp0 set\fP +.RE + +Set PHC clock time to 0 (seconds since Epoch) +.RS +\f(CWphc_ctl /dev/ptp0 set 0.0\fP +.RE + +Quickly sanity check frequency slewing by setting slewing frequency by positive +10%, resetting clock to 0.0 time, waiting for 10 seconds, and then reading +time. The time read back should be (roughly) 11 seconds, since the clock was +slewed 10% faster. +.RS
[PATCH] gitignore: add .version as this is generated during a make
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index e0710ad5b294..098dcdfe1ea7 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ /*.d /*.o +/.version /hwstamp_ctl /phc2sys /pmc -- 2.0.1.475.g9b8d714 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitignore: add .version as this is generated during a make
Hi, Jacob Keller wrote: Subject: gitignore: add .version as this is generated during a make What program generates that file? When I build on a Debian machine, I get $ make [...] SUBDIR templates $ ls -la .version ls: cannot access .version: No such file or directory Curious, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitignore: add .version as this is generated during a make
Jonathan Nieder jrnie...@gmail.com writes: Jacob Keller wrote: Subject: gitignore: add .version as this is generated during a make What program generates that file? When I build on a Debian machine, I get $ make [...] SUBDIR templates $ ls -la .version ls: cannot access .version: No such file or directory Curious, Jonathan We add one for supporting a build from tarball, but I do not recall writing nor paying attention to a dot-version. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] linuxptp: add phc_ctl program to help debug PHC devices
Jacob Keller jacob.e.kel...@intel.com writes: This is an updated version of a script I wrote a couple years ago for I suspect that this is not for us ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] linuxptp: add phc_ctl program to help debug PHC devices
On Thu, 2014-07-10 at 16:20 -0700, Junio C Hamano wrote: Jacob Keller jacob.e.kel...@intel.com writes: This is an updated version of a script I wrote a couple years ago for I suspect that this is not for us ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Indeed. This was intended for the LinuxPTP project, and I accidentally sent here. Please just ignore these. Thanks, Jake
Re: [PATCH] gitignore: add .version as this is generated during a make
On Thu, 2014-07-10 at 16:13 -0700, Jonathan Nieder wrote: Hi, Jacob Keller wrote: Subject: gitignore: add .version as this is generated during a make What program generates that file? When I build on a Debian machine, I get $ make [...] SUBDIR templates $ ls -la .version ls: cannot access .version: No such file or directory Curious, Jonathan Sorry I accidentally set these two to the wrong mailing list. These were meant for the LinuxPTP project. OOPS Thanks, Jake
[PATCH 1/2] tag: use skip_prefix instead of magic numbers
Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.
Authored-by: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
builtin/tag.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7ccb6f3c581b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const
char *arg, int unset)
int *sort = opt-value;
int flags = 0;
- if (*arg == '-') {
+ if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
- arg++;
- }
- if (starts_with(arg, version:)) {
+
+ if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
*sort = VERCMP_SORT;
- arg += 8;
- } else if (starts_with(arg, v:)) {
- *sort = VERCMP_SORT;
- arg += 2;
- } else
- *sort = STRCMP_SORT;
+
if (strcmp(arg, refname))
die(_(unsupported sort specification %s), arg);
*sort |= flags;
--
2.0.1.475.g9b8d714
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
Add support for configuring default sort ordering for git tags. Command
line option will override this configured value, using the exact same
syntax.
Cc: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
- v4
* base on top of suggested change by Jeff King to use skip_prefix instead
Documentation/config.txt | 6 ++
Documentation/git-tag.txt | 1 +
builtin/tag.c | 46 --
t/t7004-tag.sh| 21 +
4 files changed, 60 insertions(+), 14 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..ad8e75fed988 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,12 @@ submodule.name.ignore::
--ignore-submodules option. The 'git submodule' commands are not
affected by this setting.
+tag.sort::
+ This variable is used to control the sort ordering of tags. It is
+ interpreted precisely the same way as --sort=value. If --sort is
+ given on the command line it will override the selection chosen in the
+ configuration. See linkgit:git-tag[1] for more details.
+
tar.umask::
This variable can be used to restrict the permission bits of
tar archive entries. The default is 0002, which turns off the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index b424a1bc48bb..2d246725aeb5 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -317,6 +317,7 @@ include::date-formats.txt[]
SEE ALSO
linkgit:git-check-ref-format[1].
+linkgit:git-config[1].
GIT
---
diff --git a/builtin/tag.c b/builtin/tag.c
index 7ccb6f3c581b..a53e27d4e7e4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -18,6 +18,8 @@
#include sha1-array.h
#include column.h
+static int tag_sort = 0;
+
static const char * const git_tag_usage[] = {
N_(git tag [-a|-s|-u key-id] [-f] [-m msg|-F file] tagname
[head]),
N_(git tag -d tagname...),
@@ -346,9 +348,33 @@ static const char tag_template_nocleanup[] =
Lines starting with '%c' will be kept; you may remove them
yourself if you want to.\n);
+static int parse_sort_string(const char *arg)
+{
+ int sort = 0;
+ int flags = 0;
+
+ if (skip_prefix(arg, -, arg))
+ flags |= REVERSE_SORT;
+
+ if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
+ sort = VERCMP_SORT;
+
+ if (strcmp(arg, refname))
+ die(_(unsupported sort specification %s), arg);
+ sort |= flags;
+
+ return sort;
+}
+
static int git_tag_config(const char *var, const char *value, void *cb)
{
- int status = git_gpg_config(var, value, cb);
+ int status;
+
+ if (!strcmp(var, tag.sort)) {
+ tag_sort = parse_sort_string(value);
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, column.))
@@ -522,17 +548,9 @@ static int parse_opt_points_at(const struct option *opt
__attribute__((unused)),
static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
{
int *sort = opt-value;
- int flags = 0;
- if (skip_prefix(arg, -, arg))
- flags |= REVERSE_SORT;
+ *sort = parse_sort_string(arg);
- if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
- *sort = VERCMP_SORT;
-
- if (strcmp(arg, refname))
- die(_(unsupported sort specification %s), arg);
- *sort |= flags;
return 0;
}
@@ -546,7 +564,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
- int cmdmode = 0, sort = 0;
+ int cmdmode = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -572,7 +590,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT__FORCE(force, N_(replace the tag if exists)),
OPT_COLUMN(0, column, colopts, N_(show tag list in
columns)),
{
- OPTION_CALLBACK, 0, sort, sort, N_(type), N_(sort
tags),
+ OPTION_CALLBACK, 0, sort, tag_sort, N_(type),
N_(sort tags),
PARSE_OPT_NONEG, parse_opt_sort
},
@@ -628,9 +646,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
copts.padding = 2;
run_column_filter(colopts, copts);
}
- if (lines != -1 sort)
+ if (lines != -1 tag_sort)
die(_(--sort and -n are incompatible));
- ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit,
sort);
+
[PATCH v3 0/2] alloc.c: remove alloc_raw_commit_node() function
Hi Jeff,
Another attempt to remove this intermediate allocator. ;-)
I didn't think I would like this approach before I started to look at
doing this patch, but now I think I rather like it!
The first patch uses an inline function (alloc_node()) to implement the
allocators, redefining the macro to declare the allocators to, basically:
static struct alloc_state something_state;
void *alloc_something_node(void)
{
return alloc_node(something_state, sizeof(struct something));
}
Then alloc_commit_node() can also use the alloc_node() function in its
implementation.
I checked that the alloc_node() function was actually inlined by gcc,
which it was for -O1 - -O3 (but not for -O0, obviously). The generated
assembler looked very similar (but not identical) to the code generated
without this patch. Also, I did a quick (and admittedly unsophisticated)
performance test. I could not detect any difference, within the noise
level of the timings, between the two. (Also, operf barely registered
any samples against the alloc functions, for './git log -p /dev/null').
I also checked the assembler generated by clang, and the story was _almost_
the same. For -O2 - -O3, clang produced very similar results to gcc.
For -O1, clang didn't inline alloc_node(), but used it as a 'leaf'
function; the blob allocator function 'fell into' alloc_node() and the
other DEFINE-ed allocators 'jmp'-ed to alloc_node() rather than call-ing
it. The alloc_commit_node() allocator did actually call alloc_node().
(probably because it was more complicated than a single return ...).
Note this was tested (on 32-bit Linux Mint 17) with gcc 4.8.2 and
clang 3.4. (Other compilers probably will behave differently ...)
The second patch is just a minor clean-up and could be squashed into
the first patch. (NOTE: I assume that you want c-index to start at
zero; if not ...)
ATB,
Ramsay Jones
Ramsay Allan Jones (2):
alloc.c: remove the alloc_raw_commit_node() function
alloc.c: remove the redundant commit_count variable
alloc.c | 50 ++
1 file changed, 30 insertions(+), 20 deletions(-)
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] alloc.c: remove the alloc_raw_commit_node() function
In order to encapsulate the setting of the unique commit index, commit
969eba63 (commit: push commit_index update into alloc_commit_node,
10-06-2014) introduced a (logically private) intermediary allocator
function. However, this function (alloc_raw_commit_node()) was declared
as a public function, which undermines its entire purpose.
Introduce an inline function, alloc_node(), which implements the main
logic of the allocator used by DEFINE_ALLOCATOR, and redefine the macro
in terms of the new function. In addition, use the new function in the
implementation of the alloc_commit_node() allocator, rather than the
intermediary allocator, which can now be removed.
Noticed by sparse (symbol 'alloc_raw_commit_node' was not declared.
Should it be static?).
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
---
alloc.c | 47 +--
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/alloc.c b/alloc.c
index eb22a45..d7c3605 100644
--- a/alloc.c
+++ b/alloc.c
@@ -19,22 +19,10 @@
#define BLOCKING 1024
#define DEFINE_ALLOCATOR(name, type) \
-static unsigned int name##_allocs; \
+static struct alloc_state name##_state;\
void *alloc_##name##_node(void)\
{ \
- static int nr; \
- static type *block; \
- void *ret; \
- \
- if (!nr) { \
- nr = BLOCKING; \
- block = xmalloc(BLOCKING * sizeof(type)); \
- } \
- nr--; \
- name##_allocs++;\
- ret = block++; \
- memset(ret, 0, sizeof(type)); \
- return ret; \
+ return alloc_node(name##_state, sizeof(type)); \
}
union any_object {
@@ -45,16 +33,39 @@ union any_object {
struct tag tag;
};
+struct alloc_state {
+ int count; /* total number of nodes allocated */
+ int nr;/* number of nodes left in current allocation */
+ void *p; /* first free node in current allocation */
+};
+
+static inline void *alloc_node(struct alloc_state *s, size_t node_size)
+{
+ void *ret;
+
+ if (!s-nr) {
+ s-nr = BLOCKING;
+ s-p = xmalloc(BLOCKING * node_size);
+ }
+ s-nr--;
+ s-count++;
+ ret = s-p;
+ s-p = (char *)s-p + node_size;
+ memset(ret, 0, node_size);
+ return ret;
+}
+
DEFINE_ALLOCATOR(blob, struct blob)
DEFINE_ALLOCATOR(tree, struct tree)
-DEFINE_ALLOCATOR(raw_commit, struct commit)
DEFINE_ALLOCATOR(tag, struct tag)
DEFINE_ALLOCATOR(object, union any_object)
+static struct alloc_state commit_state;
+
void *alloc_commit_node(void)
{
static int commit_count;
- struct commit *c = alloc_raw_commit_node();
+ struct commit *c = alloc_node(commit_state, sizeof(struct commit));
c-index = commit_count++;
return c;
}
@@ -66,13 +77,13 @@ static void report(const char *name, unsigned int count,
size_t size)
}
#define REPORT(name, type) \
-report(#name, name##_allocs, name##_allocs * sizeof(type) 10)
+report(#name, name##_state.count, name##_state.count * sizeof(type) 10)
void alloc_report(void)
{
REPORT(blob, struct blob);
REPORT(tree, struct tree);
- REPORT(raw_commit, struct commit);
+ REPORT(commit, struct commit);
REPORT(tag, struct tag);
REPORT(object, union any_object);
}
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] alloc.c: remove the redundant commit_count variable
The 'commit_count' static variable is used in alloc_commit_node()
to set the 'index' field of the commit structure to a unique value.
This variable assumes the same value as the 'count' field of the
'commit_state' allocator state structure, which may be used in its
place.
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
---
alloc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/alloc.c b/alloc.c
index d7c3605..c6687f9 100644
--- a/alloc.c
+++ b/alloc.c
@@ -64,9 +64,8 @@ static struct alloc_state commit_state;
void *alloc_commit_node(void)
{
- static int commit_count;
struct commit *c = alloc_node(commit_state, sizeof(struct commit));
- c-index = commit_count++;
+ c-index = commit_state.count - 1;
return c;
}
--
2.0.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] alloc.c: remove the alloc_raw_commit_node() function
On Fri, Jul 11, 2014 at 12:58:31AM +0100, Ramsay Jones wrote:
#define DEFINE_ALLOCATOR(name, type) \
-static unsigned int name##_allocs; \
+static struct alloc_state name##_state; \
void *alloc_##name##_node(void) \
{\
- static int nr; \
- static type *block; \
- void *ret; \
- \
- if (!nr) { \
- nr = BLOCKING; \
- block = xmalloc(BLOCKING * sizeof(type)); \
- } \
- nr--; \
- name##_allocs++;\
- ret = block++; \
- memset(ret, 0, sizeof(type)); \
- return ret; \
+ return alloc_node(name##_state, sizeof(type)); \
}
Yay. Not only does this solve the problem, but it gets rid of nasty
multi-line macro. In fact, I kind of wonder if we should just do away
with the macro entirely, and write out:
static struct alloc_state blob_state;
void alloc_blob_node(void)
{
return alloc_node(blob_state, sizeof(struct blob));
}
It's more lines, but it is probably less obfuscated to a reader.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable
On Fri, Jul 11, 2014 at 12:59:48AM +0100, Ramsay Jones wrote:
The 'commit_count' static variable is used in alloc_commit_node()
to set the 'index' field of the commit structure to a unique value.
This variable assumes the same value as the 'count' field of the
'commit_state' allocator state structure, which may be used in its
place.
I don't think we want to do this, because there is a bug in the current
code that I have not reported yet. :)
The code you're touching here was trying to make sure that each commit
gets a unique index, under the assumption that commits only get
allocated via alloc_commit_node. But I think that assumption is wrong.
We can also get commit objects by allocating an OBJ_NONE (e.g., via
lookup_unknown_object) and then converting it into an OBJ_COMMIT when we
find out what it is.
We should be allocating a commit index to it at that point (but we don't
currently), at which point the commit_count and commit_state.count
indices will no longer be in sync.
This only happens in a few places. Most calls will probably be via
lookup_commit (which gets called when we parse_object such an unknown
object), but there are others (e.g., peel_object may fill in the type).
So we could fix it with something like:
diff --git a/commit.c b/commit.c
index fb7897c..f4f4278 100644
--- a/commit.c
+++ b/commit.c
@@ -65,8 +65,11 @@ struct commit *lookup_commit(const unsigned char *sha1)
struct commit *c = alloc_commit_node();
return create_object(sha1, OBJ_COMMIT, c);
}
- if (!obj-type)
+ if (!obj-type) {
+ struct commit *c = (struct commit *)obj;
obj-type = OBJ_COMMIT;
+ c-index = alloc_commit_index();
+ }
return check_commit(obj, sha1, 0);
}
where alloc_commit_index() would be a thin wrapper around return
commit_count++. And then find and update any other callsites that need
it.
The downside is that it's hard to find those callsites. A safer
alternative would be to speculatively allocate a commit index for all
unknown objects. Then if any other code switches the type to commit,
they already have an index. But it also means we'll have holes in our
commit index space, which makes commit-slabs less efficient.
We do not call lookup_unknown_object all that often, though, so it might
be an OK tradeoff (and in at least one case, in diff_tree_stdin, I think
the code can be converted not to use lookup_unknown_object in the first
place). So worrying about the performance may not be worth it.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4 v6] cache-tree: Create/update cache-tree on checkout
When git checkout checks out a branch, create or update the
cache-tree so that subsequent operations are faster.
update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR. When
WRITE_TREE_REPAIR is set, portions of the cache-tree which do not
correspond to existing tree objects are invalidated (and portions which
do are marked as valid). No new tree objects are created.
Signed-off-by: David Turner dtur...@twitter.com
---
builtin/checkout.c| 8
cache-tree.c | 12 +++-
cache-tree.h | 1 +
t/t0090-cache-tree.sh | 19 ---
4 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07cf555..054214f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts
*opts,
}
}
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree))
+ cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const
*)active_cache,
+ active_nr, WRITE_TREE_SILENT |
WRITE_TREE_REPAIR);
+
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock_file))
die(_(unable to write new index file));
diff --git a/cache-tree.c b/cache-tree.c
index 7fa524a..f951d7d 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -239,9 +239,12 @@ static int update_one(struct cache_tree *it,
struct strbuf buffer;
int missing_ok = flags WRITE_TREE_MISSING_OK;
int dryrun = flags WRITE_TREE_DRY_RUN;
+ int repair = flags WRITE_TREE_REPAIR;
int to_invalidate = 0;
int i;
+ assert(!(dryrun repair));
+
*skip_count = 0;
if (0 = it-entry_count has_sha1_file(it-sha1))
@@ -374,7 +377,14 @@ static int update_one(struct cache_tree *it,
#endif
}
- if (dryrun)
+ if (repair) {
+ unsigned char sha1[20];
+ hash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);
+ if (has_sha1_file(sha1))
+ hashcpy(it-sha1, sha1);
+ else
+ to_invalidate = 1;
+ } else if (dryrun)
hash_sha1_file(buffer.buf, buffer.len, tree_type, it-sha1);
else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it-sha1)) {
strbuf_release(buffer);
diff --git a/cache-tree.h b/cache-tree.h
index f1923ad..666d18f 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -39,6 +39,7 @@ int update_main_cache_tree(int);
#define WRITE_TREE_IGNORE_CACHE_TREE 2
#define WRITE_TREE_DRY_RUN 4
#define WRITE_TREE_SILENT 8
+#define WRITE_TREE_REPAIR 16
/* error return codes */
#define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 6c33e28..98fb1ab 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes cache-tree'
'
test_expect_success 'git-add invalidates cache-tree' '
test_when_finished git reset --hard; git read-tree HEAD
- echo I changed this file foo
+ echo I changed this file foo
git add foo
test_invalid_cache_tree
'
test_expect_success 'update-index invalidates cache-tree' '
test_when_finished git reset --hard; git read-tree HEAD
- echo I changed this file foo
+ echo I changed this file foo
git update-index --add foo
test_invalid_cache_tree
'
@@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives
cache-tree' '
test_shallow_cache_tree
'
-test_expect_failure 'checkout gives cache-tree' '
+test_expect_success 'checkout gives cache-tree' '
+ git tag current
git checkout HEAD^
test_shallow_cache_tree
'
+test_expect_success 'checkout -b gives cache-tree' '
+ git checkout current
+ git checkout -b prev HEAD^
+ test_shallow_cache_tree
+'
+
+test_expect_success 'checkout -B gives cache-tree' '
+ git checkout current
+ git checkout -B prev HEAD^
+ test_shallow_cache_tree
+'
+
test_done
--
2.0.0.390.gcb682f8
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4 v6] test-dump-cache-tree: invalid trees are not errors
Do not treat known-invalid trees as errors even when their subtree_nr is
incorrect. Because git already knows that these trees are invalid,
an incorrect subtree_nr will not cause problems.
Add a couple of comments.
Signed-off-by: David Turner dtur...@twitter.com
---
test-dump-cache-tree.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..cbbbd8e 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -26,16 +26,16 @@ static int dump_cache_tree(struct cache_tree *it,
return 0;
if (it-entry_count 0) {
+ /* invalid */
dump_one(it, pfx, );
dump_one(ref, pfx, #(ref) );
- if (it-subtree_nr != ref-subtree_nr)
- errs = 1;
}
else {
dump_one(it, pfx, );
if (hashcmp(it-sha1, ref-sha1) ||
ref-entry_count != it-entry_count ||
ref-subtree_nr != it-subtree_nr) {
+ /* claims to be valid but is lying */
dump_one(ref, pfx, #(ref) );
errs = 1;
}
--
2.0.0.390.gcb682f8
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4 v6] cache-tree: subdirectory tests
Add tests to confirm that invalidation of subdirectories neither over-
nor under-invalidates.
Signed-off-by: David Turner dtur...@twitter.com
---
t/t0090-cache-tree.sh | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 98fb1ab..3a3342e 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -22,9 +22,10 @@ test_shallow_cache_tree () {
}
test_invalid_cache_tree () {
- echo invalid (0 subtrees) expect
- printf SHA #(ref) (%d entries, 0 subtrees)\n $(git ls-files|wc -l)
expect
- cmp_cache_tree expect
+ printf invalid %s ()\n $@
expect
+ test-dump-cache-tree | \
+ sed -n -e s/[0-9]* subtrees// -e '/#(ref)/d' -e '/^invalid /p'
actual
+ test_cmp expect actual
}
test_no_cache_tree () {
@@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '
test_invalid_cache_tree
'
+test_expect_success 'git-add in subdir invalidates cache-tree' '
+ test_when_finished git reset --hard; git read-tree HEAD
+ mkdir dirx
+ echo I changed this file dirx/foo
+ git add dirx/foo
+ test_invalid_cache_tree
+'
+
+test_expect_success 'git-add in subdir does not invalidate sibling cache-tree'
'
+ git tag no-children
+ test_when_finished git reset --hard no-children; git read-tree HEAD
+ mkdir dir1 dir2
+ test_commit dir1/a
+ test_commit dir2/b
+ echo I changed this file dir1/a
+ git add dir1/a
+ test_invalid_cache_tree dir1/
+'
+
test_expect_success 'update-index invalidates cache-tree' '
test_when_finished git reset --hard; git read-tree HEAD
echo I changed this file foo
--
2.0.0.390.gcb682f8
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4 v6] cache-tree: Write updated cache-tree after commit
During the commit process, update the cache-tree. Write this updated
cache-tree so that it's ready for subsequent commands.
Add test code which demonstrates that git commit now writes the cache
tree. Make all tests test the entire cache-tree, not just the root
level.
Signed-off-by: David Turner dtur...@twitter.com
---
builtin/commit.c | 9 +-
t/t0090-cache-tree.sh | 87 ++-
2 files changed, 81 insertions(+), 15 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..99c9054 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
discard_cache();
read_cache_from(index_lock.filename);
+ if (update_main_cache_tree(WRITE_TREE_SILENT) = 0)
+ write_cache(fd, active_cache, active_nr);
commit_style = COMMIT_NORMAL;
return index_lock.filename;
@@ -383,8 +385,12 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
if (!only !pathspec.nr) {
fd = hold_locked_index(index_lock, 1);
refresh_cache_or_die(refresh_flags);
- if (active_cache_changed) {
+ if (active_cache_changed
+ || !cache_tree_fully_valid(active_cache_tree)) {
update_main_cache_tree(WRITE_TREE_SILENT);
+ active_cache_changed = 1;
+ }
+ if (active_cache_changed) {
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(index_lock))
die(_(unable to write new_index file));
@@ -435,6 +441,7 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
fd = hold_locked_index(index_lock, 1);
add_remove_files(partial);
refresh_cache(REFRESH_QUIET);
+ update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(index_lock))
die(_(unable to write new_index file));
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 3a3342e..db7a8d0 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -8,7 +8,7 @@ cache-tree extension.
. ./test-lib.sh
cmp_cache_tree () {
- test-dump-cache-tree actual
+ test-dump-cache-tree | sed -e '/#(ref)/d' actual
sed s/$_x40/SHA/ actual filtered
test_cmp $1 filtered
}
@@ -16,8 +16,34 @@ cmp_cache_tree () {
# We don't bother with actually checking the SHA1:
# test-dump-cache-tree already verifies that all existing data is
# correct.
-test_shallow_cache_tree () {
- printf SHA (%d entries, 0 subtrees)\n $(git ls-files|wc -l) expect
+generate_expected_cache_tree_rec () {
+ dir=$1${1:+/}
+ parent=$2
+ # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
+ # We want to count only foo because it's the only direct child
+ subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq)
+ subtree_count=$(echo $subtrees|awk '$1 {++c} END {print c}')
+ entries=$(git ls-files|wc -l)
+ printf SHA $dir (%d entries, %d subtrees)\n $entries
$subtree_count
+ for subtree in $subtrees
+ do
+ cd $subtree
+ generate_expected_cache_tree_rec $dir$subtree $dir || return 1
+ cd ..
+ done
+ dir=$parent
+}
+
+generate_expected_cache_tree () {
+cwd=$(pwd)
+generate_expected_cache_tree_rec
+ret=$?
+cd $cwd
+return $ret
+}
+
+test_cache_tree () {
+ generate_expected_cache_tree expect
cmp_cache_tree expect
}
@@ -33,14 +59,14 @@ test_no_cache_tree () {
cmp_cache_tree expect
}
-test_expect_failure 'initial commit has cache-tree' '
+test_expect_success 'initial commit has cache-tree' '
test_commit foo
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'read-tree HEAD establishes cache-tree' '
git read-tree HEAD
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'git-add invalidates cache-tree' '
@@ -58,6 +84,18 @@ test_expect_success 'git-add in subdir invalidates
cache-tree' '
test_invalid_cache_tree
'
+cat before \EOF
+SHA (3 entries, 2 subtrees)
+SHA dir1/ (1 entries, 0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
+cat expect \EOF
+invalid (2 subtrees)
+invalid dir1/ (0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
test_expect_success 'git-add in subdir does not invalidate sibling cache-tree'
'
git tag no-children
test_when_finished git reset --hard no-children; git read-tree HEAD
@@ -65,8 +103,10 @@ test_expect_success 'git-add in subdir does not invalidate
sibling cache-tree'
Re: [PATCH v3 1/2] alloc.c: remove the alloc_raw_commit_node() function
On 11/07/14 01:09, Jeff King wrote:
On Fri, Jul 11, 2014 at 12:58:31AM +0100, Ramsay Jones wrote:
#define DEFINE_ALLOCATOR(name, type)\
-static unsigned int name##_allocs; \
+static struct alloc_state name##_state; \
void *alloc_##name##_node(void) \
{ \
-static int nr; \
-static type *block; \
-void *ret; \
-\
-if (!nr) { \
-nr = BLOCKING; \
-block = xmalloc(BLOCKING * sizeof(type)); \
-} \
-nr--; \
-name##_allocs++;\
-ret = block++; \
-memset(ret, 0, sizeof(type)); \
-return ret; \
+return alloc_node(name##_state, sizeof(type)); \
}
Yay. Not only does this solve the problem, but it gets rid of nasty
multi-line macro. In fact, I kind of wonder if we should just do away
with the macro entirely, and write out:
static struct alloc_state blob_state;
void alloc_blob_node(void)
{
return alloc_node(blob_state, sizeof(struct blob));
}
It's more lines, but it is probably less obfuscated to a reader.
Yeah, I'm not a fan of _large_ multi-line macros myself.
Now that DEFINE_ALLOCATOR has slimmed down, I don't mind it so much.
However, I agree that doing away with the macro leads to easier to
read code. (I also don't mind the extra lines).
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable
On 11/07/14 01:30, Jeff King wrote: On Fri, Jul 11, 2014 at 12:59:48AM +0100, Ramsay Jones wrote: The 'commit_count' static variable is used in alloc_commit_node() to set the 'index' field of the commit structure to a unique value. This variable assumes the same value as the 'count' field of the 'commit_state' allocator state structure, which may be used in its place. I don't think we want to do this, because there is a bug in the current code that I have not reported yet. :) :P OK, I will simply drop this one then. The code you're touching here was trying to make sure that each commit gets a unique index, under the assumption that commits only get allocated via alloc_commit_node. But I think that assumption is wrong. We can also get commit objects by allocating an OBJ_NONE (e.g., via lookup_unknown_object) and then converting it into an OBJ_COMMIT when we find out what it is. Hmm, I don't know how the object is converted, but the object allocator is actually allocating an 'union any_object', so it's allocating more space than for a struct object anyway. If you add an 'index' field to struct object, (and remove it from struct commit) it could be set in alloc_object_node(). ie _all_ node types get an index field. Hmm, that was just off the top of my head, so take with a pinch of salt. ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 0/3] git config cache special querying api utilizing the cache
Hi, [PATCH V8]: Moved the contents of config-set.c to config.c for future convenience. Reverted test 'find value with misspelled key' to the one in v5. See [10] for the discussion. [PATCH V7]: Style nits and a broken chain corrected in `t/t1308-config-set.sh`. See [9] for the nits. [PATCH V6]: Style nits and mistakes corrected. Diff between v6 and v5[8] is at the bottom. Thanks to Matthieu, Ramsay and Ram for their suggestions. [PATCH V5]: `config_set` now uses a single hashmap. Corrected style nits raised in the thread[7]. Thanks to Junio and Matthieu for their suggestions. [PATCH v4]: Introduced `config_set` construct which points to a ordered set of config-files cached as hashmaps. Added relevant API functions. For more details see the documentation. Rewrote the git_config_get* family to use `config_set` internally. Added tests for both config_set API and git_config_get family. Added type specific API functions which parses the found value and converts it into a specific type. Most of the changes implemented are the result of discussion in [6]. Thanks to Eric, Ramsay, Junio, Matthieu Karsten for their suggestions and review. [PATCH v3]: Added flag for NULL values that were causing segfaults in some cases. Added test-config for usage examples. Minor changes and corrections. Refer to discussion thread[5] for more details. Thanks to Matthieu, Jeff and Junio for their valuable suggestions. [PATCH v2]:Changed the string_list to a struct instead of pointer to a struct. Added string-list initilization functions. Minor mistakes corrected acoording to review comments[4]. Thanks to Eric and Matthieu for their review. [PATCH V1]:Most of the invaluable suggestions by Eric Sunshine, Torsten Bogershausen and Jeff King has been implemented[1]. Complete rewrite of config_cache*() family using git_config() as hook as suggested by Jeff. Thanks for the review. [RFC V2]: Improved according to the suggestions by Eric Sunshine and Torsten Bogershausen. Added cache invalidation when config file is changed.[2] I am using git_config_set_multivar_in_file() as an update hook. This is patch is for this year's GSoC. My project is Git Config API improvements. The link of my proposal is appended below [3]. The aim of this patch series is to generate a cache for querying values from the config files in a non-callback manner as the current method reads and parses the config files every time a value is queried for. The cache is generated from hooking the update_cache function to the current parsing and callback mechanism in config.c. It is implemented as an hashmap using the hashmap-api with variables and its corresponding values list as its members. The values in the list are sorted in order of increasing priority. The cache is initialised the first time when any of the new query functions is called. It is invalidated by using git_config_set_multivar_in_file() as an update hook. We add two new functions to the config-api, git_config_get_string() and git_config_get_string_multi() for querying in a non callback manner from the cache. [1] http://marc.info/?t=14017206626r=1w=2 [2] http://git.661346.n2.nabble.com/RFC-PATCH-0-2-Git-config-cache-amp-special-querying-api-utilizing-the-cache-td7611691.html [3] https://drive.google.com/file/d/0B4suZ-aHqDcnSUZJRXVTTnZUN1E/edit?usp=sharing [4] http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251369 [5] http://thread.gmane.org/gmane.comp.version-control.git/251704/ [6] http://thread.gmane.org/gmane.comp.version-control.git/252329/ [7] http://marc.info/?t=14042811521r=1w=2 [8] http://article.gmane.org/gmane.comp.version-control.git/252942/ [9] http://thread.gmane.org/gmane.comp.version-control.git/252959/ [10] http://article.gmane.org/gmane.comp.version-control.git/253113 Tanay Abhra (2): config set test-config .gitignore | 1 + Documentation/technical/api-config.txt | 134 +++ Makefile | 1 + cache.h| 31 config.c | 296 + t/t1308-config-set.sh | 170 +++ test-config.c | 125 ++ 7 files changed, 758 insertions(+) create mode 100755 t/t1308-config-set.sh create mode 100644 test-config.c -- 1.9.0.GIT -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 2/2] test-config: add tests for the config_set API
Expose the `config_set` C API as a set of simple commands in order to
facilitate testing. Add tests for the `config_set` API as well as for
`git_config_get_*()` family for the usual config files.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
.gitignore| 1 +
Makefile | 1 +
t/t1308-config-set.sh | 170 ++
test-config.c | 125 +
4 files changed, 297 insertions(+)
create mode 100755 t/t1308-config-set.sh
create mode 100644 test-config.c
diff --git a/.gitignore b/.gitignore
index 42294e5..7677533 100644
--- a/.gitignore
+++ b/.gitignore
@@ -177,6 +177,7 @@
/gitweb/static/gitweb.min.*
/test-chmtime
/test-ctype
+/test-config
/test-date
/test-delta
/test-dump-cache-tree
diff --git a/Makefile b/Makefile
index 07ea105..9544efb 100644
--- a/Makefile
+++ b/Makefile
@@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
new file mode 100755
index 000..87a29f1
--- /dev/null
+++ b/t/t1308-config-set.sh
@@ -0,0 +1,170 @@
+#!/bin/sh
+
+test_description='Test git config-set API in different settings'
+
+. ./test-lib.sh
+
+# 'check section.key value' verifies that the entry for section.key is
+# 'value'
+check() {
+ echo $2 expected
+ test-config get_value $1 actual 21
+ test_cmp expected actual
+}
+
+test_expect_success 'setup default config' '
+ cat .git/config EOF
+ [core]
+ penguin = very blue
+ Movie = BadPhysics
+ UPPERCASE = true
+ MixedCase = true
+ my =
+ foo
+ baz = sam
+ [Cores]
+ WhatEver = Second
+ baz = bar
+ [cores]
+ baz = bat
+ [CORES]
+ baz = ball
+ [my Foo bAr]
+ hi = mixed-case
+ [my FOO BAR]
+ hi = upper-case
+ [my foo bar]
+ hi = lower-case
+ [core]
+ baz = bat
+ baz = hask
+ [lamb]
+ chop = 65
+ head = none
+ [goat]
+ legs = 4
+ head = true
+ skin = false
+ nose = 1
+ horns
+ EOF
+'
+
+test_expect_success 'get value for a simple key' '
+ check core.penguin very blue
+'
+
+test_expect_success 'get value for a key with value as an empty string' '
+ check core.my
+'
+
+test_expect_success 'get value for a key with value as NULL' '
+ check core.foo (NULL)
+'
+
+test_expect_success 'upper case key' '
+ check core.UPPERCASE true
+'
+
+test_expect_success 'mixed case key' '
+ check core.MixedCase true
+'
+
+test_expect_success 'key and value with mixed case' '
+ check core.Movie BadPhysics
+'
+
+test_expect_success 'key with case sensitive subsection' '
+ check my.Foo bAr.hi mixed-case
+ check my.FOO BAR.hi upper-case
+ check my.foo bar.hi lower-case
+'
+
+test_expect_success 'key with case insensitive section header' '
+ check cores.baz ball
+ check Cores.baz ball
+ check CORES.baz ball
+ check coreS.baz ball
+'
+
+test_expect_success 'find value with misspelled key' '
+ echo Value not found for \my.fOo Bar.hi\ expect
+ test_must_fail test-config get_value my.fOo Bar.hi actual
+ test_cmp expect actual
+'
+
+test_expect_success 'find value with the highest priority' '
+ check core.baz hask
+'
+
+test_expect_success 'find integer value for a key' '
+ echo 65 expect
+ test-config get_int lamb.chop actual
+ test_cmp expect actual
+'
+
+test_expect_success 'find integer if value is non parse-able' '
+ echo 65 expect
+ test_must_fail test-config get_int lamb.head actual
+ test_must_fail test_cmp expect actual
+'
+
+test_expect_success 'find bool value for the entered key' '
+ cat expect -\EOF
+ 1
+ 0
+ 1
+ 1
+ 1
+ EOF
+ test-config get_bool goat.head actual
+ test-config get_bool goat.skin actual
+ test-config get_bool goat.nose actual
+ test-config get_bool goat.horns actual
+ test-config get_bool goat.legs actual
+ test_cmp expect actual
+'
+
+test_expect_success 'find multiple values' '
+ cat expect -\EOF
+ sam
+ bat
+ hask
+ EOF
+ test-config get_value_multi core.bazactual
+ test_cmp expect actual
+'
+
+test_expect_success 'find value from a configset' '
+ cat config2 -\EOF
+ [core]
+ baz = lama
+ [my]
+ new = silk
+ [core]
+
[PATCH v8 1/2] add `config_set` API for caching config-like files
Currently `git_config()` uses a callback mechanism and file rereads for config values. Due to this approach, it is not uncommon for the config files to be parsed several times during the run of a git program, with different callbacks picking out different variables useful to themselves. Add a `config_set`, that can be used to construct an in-memory cache for config-like files that the caller specifies (i.e., files like `.gitmodules`, `~/.gitconfig` etc.). Add two external functions `git_configset_get_value` and `git_configset_get_value_multi` for querying from the config sets. `git_configset_get_value` follows `last one wins` semantic (i.e. if there are multiple matches for the queried key in the files of the configset the value returned will be the last entry in `value_list`). `git_configset_get_value_multi` returns a list of values sorted in order of increasing priority (i.e. last match will be at the end of the list). Add type specific query functions like `git_configset_get_bool` and similar. Add a default `config_set`, `the_config_set` to cache all key-value pairs read from usual config files (repo specific .git/config, user wide ~/.gitconfig, XDG config and the global /etc/gitconfig). `the_config_set` is populated using `git_config()`. Add two external functions `git_config_get_value` and `git_config_get_value_multi` for querying in a non-callback manner from `the_config_set`. Also, add type specific query functions that are implemented as a thin wrapper around the `config_set` API. Signed-off-by: Tanay Abhra tanay...@gmail.com --- Documentation/technical/api-config.txt | 134 +++ cache.h| 31 config.c | 296 + 3 files changed, 461 insertions(+) diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt index 230b3a0..bb43830 100644 --- a/Documentation/technical/api-config.txt +++ b/Documentation/technical/api-config.txt @@ -77,6 +77,81 @@ To read a specific file in git-config format, use `git_config_from_file`. This takes the same callback and data parameters as `git_config`. +Querying For Specific Variables +--- + +For programs wanting to query for specific variables in a non-callback +manner, the config API provides two functions `git_config_get_value` +and `git_config_get_value_multi`. They both read values from an internal +cache generated previously from reading the config files. + +`int git_config_get_value(const char *key, const char **value)`:: + + Finds the highest-priority value for the configuration variable `key`, + stores the pointer to it in `value` and returns 0. When the + configuration variable `key` is not found, returns 1 without touching + `value`. The caller should not free or modify `value`, as it is owned + by the cache. + +`const struct string_list *git_config_get_value_multi(const char *key)`:: + + Finds and returns the value list, sorted in order of increasing priority + for the configuration variable `key`. When the configuration variable + `key` is not found, returns NULL. The caller should not free or modify + the returned pointer, as it is owned by the cache. + +`void git_config_clear(void)`:: + + Resets and invalidates the config cache. + +The config API also provides type specific API functions which do conversion +as well as retrieval for the queried variable, including: + +`int git_config_get_int(const char *key, int *dest)`:: + + Finds and parses the value to an integer for the configuration variable + `key`. Dies on error; otherwise, stores the value of the parsed integer in + `dest` and returns 0. When the configuration variable `key` is not found, + returns 1 without touching `dest`. + +`int git_config_get_ulong(const char *key, unsigned long *dest)`:: + + Similar to `git_config_get_int` but for unsigned longs. + +`int git_config_get_bool(const char *key, int *dest)`:: + + Finds and parses the value into a boolean value, for the configuration + variable `key` respecting keywords like true and false. Integer + values are converted into true/false values (when they are non-zero or + zero, respectively). Other values cause a die(). If parsing is successful, + stores the value of the parsed result in `dest` and returns 0. When the + configuration variable `key` is not found, returns 1 without touching + `dest`. + +`int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)`:: + + Similar to `git_config_get_bool`, except that integers are copied as-is, + and `is_bool` flag is unset. + +`int git_config_get_maybe_bool(const char *key, int *dest)`:: + + Similar to `git_config_get_bool`, except that it returns -1 on error + rather than dying. + +`int git_config_get_string(const char *key, const char **dest)`:: + +
