Re: [Bug] data loss with cyclic alternates
On Fri, Jul 11, 2014 at 06:01:46PM +, Keller, Jacob E wrote: > > Yeah, don't do that. A thinks "eh, the other guy must have it" and > > B thinks the same. In general, do not prune or gc a repository > > other repositories borrow from, even if there is no cycle, because > > the borrowee does not know anythning about objects that it itself no > > longer needs but are still needed by its borrowers. > > > > Doesn't gc get run automatically at some points? Is the automatic gc run > setup to avoid this problem? No, the automatic gc doesn't avoid this. It can't in the general case, as the parent repository does not know how many or which children are pointed to it as an alternate. And the borrowing repository does not even need to have write permission to the parent, so it cannot write a backpointer. If people are using alternates, they should probably turn off gc.auto in the borrowee (it doesn't seem unreasonable to me to do so automatically via "clone -s" in cases where we can write to the alternates repo, and to issue a warning otherwise). -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: Error running 'git status' with Git version of current 'next' branch
On Wed, Jul 09, 2014 at 09:10:49AM +0200, Ralf Thielow wrote: > I'm getting the following error when calling 'git status' on one of > my projects. > > git: dir.c:739: last_exclude_matching_from_list: Assertion `x->baselen > == 0 || x->base[x->baselen - 1] == '/'' failed. > Aborted (core dumped) > > I'm using the current 'next', git version 2.0.1.664.g35b839c > I have bisected it to commit d3ccb7d (dir: remove PATH_MAX limitation). Thanks for bisecting. That commit has been reverted from next in favor of a nicer refactoring, though, so I don't think it's worth tracking down the bug in d3ccb7d. -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 Fri, Jul 11, 2014 at 12:12:55AM +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? I think none of the regular devs uses PROFILE, and it bit-rotted somewhat. Andi Kleen recently posted a series to fix it[1]. I can reproduce your problem without that series, but compiling with ak/profile-feedback-build merged in seems to work OK. [1] http://thread.gmane.org/gmane.comp.version-control.git/253005 -- 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-push.c: make CURLOPT_IOCTLDATA a usable pointer
On Sat, Jul 05, 2014 at 08:43:48PM -0400, Abbaad Haider wrote: > Fixes a small bug affecting push to remotes which use some sort of > multi-pass authentication. In particular the bug affected SabreDAV as > configured by Box.com [1]. Thanks. This looks like it was caused by the refactor in ebaaf31 (http-push: refactor curl_easy_setup madness, 2011-05-03), which moved the curl_easy call into a sub-function which took the buffer as a pointer, rather than accessing it as a local variable. > It must be a weird server configuration for the bug to have survived > this long. Someone should write a test for it. I think both dumb-http push-over-DAV and multi-pass authentication are rare, so finding a combination of the two took a while. I do not know enough about the server setup to know whether we could replicate this in our test apache setup (and nor do I particularly want to spend a lot of time figuring it out for the sake of testing push-over-DAV). > diff --git a/http-push.c b/http-push.c > index f2c56c8..bd42895 100644 > --- a/http-push.c > +++ b/http-push.c > @@ -199,7 +199,7 @@ static void curl_setup_http(CURL *curl, const char *url, > curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer); > #ifndef NO_CURL_IOCTL > curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer); > - curl_easy_setopt(curl, CURLOPT_IOCTLDATA, &buffer); > + curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer); Patch looks obviously correct. -- 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/4] 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
---
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 v8 4/4] 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
---
builtin/commit.c | 16 ++-
t/t0090-cache-tree.sh | 117 +++---
2 files changed, 117 insertions(+), 16 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..d6681c4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -342,6 +342,15 @@ 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) {
+ fd = open(index_lock.filename, O_WRONLY);
+ if (fd >= 0)
+ if (write_cache(fd, active_cache, active_nr) ==
0) {
+ close_lock_file(&index_lock);
+ }
+ }
+ else
+ fprintf(stderr, "FAiled to update main cache tree\n");
commit_style = COMMIT_NORMAL;
return index_lock.filename;
@@ -383,8 +392,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 +448,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..48c4240 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/" filtered &&
test_cmp "$1" filtered
}
@@ -16,14 +16,38 @@ 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 () {
+ (
+ generate_expected_cache_tree_rec
+ )
+}
+
+test_cache_tree () {
+ generate_expected_cache_tree >expect &&
cmp_cache_tree expect
}
test_invalid_cache_tree () {
printf "invalid %s ()\n" "" "$@"
>expect &&
- test-dump-cache-tree | \
+ test-dump-cache-tree |
sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p'
>actual &&
test_cmp expect actual
}
@@ -33,14 +57,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 +82,18 @@ test_expect_success 'g
[PATCH v8 1/4] 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
---
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 v8 3/4] cache-tree: subdirectory tests
Add tests to confirm that invalidation of subdirectories neither over-
nor under-invalidates.
Signed-off-by: David Turner
---
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
Re: [PATCH 2/2] dir: remove PATH_MAX limitation
On Sat, Jul 12, 2014 at 6:43 AM, Karsten Blees wrote: > Am 12.07.2014 00:29, schrieb Junio C Hamano: >> Karsten Blees writes: >> >>> Anyways, I'd like to kindly withdraw this patch in favor of Duy's version. >>> >>> http://article.gmane.org/gmane.comp.version-control.git/248310 >> >> Thanks; I've already reverted it from 'next'. >> >> Is Duy's patch still viable? >> > > I think so. It fixes the segfault with long paths on Windows as well > (Tested-by: ), uses strbuf APIs as Peff suggested, and initializes the > strbuf with PATH_MAX (i.e. no reallocs in the common case either ;-) ). > > AFAICT the first two patches of that series are also completely unrelated > to the untracked-cache, so we may want to fast-track these? > > [01/20] dir.c: coding style fix > [02/20] dir.h: move struct exclude declaration to top level > [03/20] prep_exclude: remove the artificial PATH_MAX limit > > ...perhaps with s/if (!dir->basebuf.alloc)/if (!dir->basebuf.buf)/ > > @Duy any reason for not signing off that series? That series still need a lot more work, but for those first three, if you want to fast track, you have my sign-offs. -- Duy -- 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 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, Jul 11, 2014 at 02:54:25PM -0700, Junio C Hamano wrote: > > Yeah, we're quite inconsistent there. In some cases we silently ignore > > something unknown (e.g., a color.diff.* slot that we do not understand), > > but in most cases if it is a config key we understand but a value we do > > not, we complain and die. > > Hm, that's bad---we've become less and less careful over time, > perhaps? I don't think so. I think we've always been not-very-lenient with parsing values. Two examples: 1. We sometimes extend a boolean variable to take additional values (like "auto" or "always"). Older gits will see those values and say "not a boolean, and also not a number" and barf. branch.autosetupmerge is an example of this (it learned "always" in 9ed36cf. 2. The parser for colors translates things like "bold" into ANSI codes. If it doesn't understand a keyword, it dies. If we add new color names or attributes, they will cause older git to die. There are many cases like this. Pretty much any enumerated value (like branch.autosetuprebase, for example) will face this whenever we add new possible values. So I do not think we have ever had a rule, but if we did, it is probably "silently ignore unknown keys, complain on unknown values". Which makes warning on tag.sort unlike most of the rest of the code. I do not mind it, though (I am one of the people it is most likely to help :) ). Just giving some context. -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 1/3] tag: use skip_prefix instead of magic numbers
On Fri, Jul 11, 2014 at 03:55:45PM -0700, Jacob Keller wrote: > From: Jeff King > > Make the parsing of the --sort parameter more readable by having > skip_prefix keep our pointer up to date. > > Signed-off-by: Jeff King > Signed-off-by: Jacob Keller > --- > Fixed issue with patch in that we dropped the reset to STRCMP_SORT, discovered > by Junio. I think what Junio queued in ce85604468 is already correct, so we are fine as long we build on that. -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 v8 17/17] api-trace.txt: add trace API documentation
Signed-off-by: Karsten Blees
---
Documentation/technical/api-trace.txt | 97 +++
1 file changed, 97 insertions(+)
create mode 100644 Documentation/technical/api-trace.txt
diff --git a/Documentation/technical/api-trace.txt
b/Documentation/technical/api-trace.txt
new file mode 100644
index 000..097a651
--- /dev/null
+++ b/Documentation/technical/api-trace.txt
@@ -0,0 +1,97 @@
+trace API
+=
+
+The trace API can be used to print debug messages to stderr or a file. Trace
+code is inactive unless explicitly enabled by setting `GIT_TRACE*` environment
+variables.
+
+The trace implementation automatically adds `timestamp file:line ... \n` to
+all trace messages. E.g.:
+
+
+23:59:59.123456 git.c:312 trace: built-in: git 'foo'
+00:00:00.01 builtin/foo.c:99foo: some message
+
+
+Data Structures
+---
+
+`struct trace_key`::
+
+ Defines a trace key (or category). The default (for API functions that
+ don't take a key) is `GIT_TRACE`.
++
+E.g. to define a trace key controlled by environment variable `GIT_TRACE_FOO`:
++
+
+static struct trace_key trace_foo = TRACE_KEY_INIT(FOO);
+
+static void trace_print_foo(const char *message)
+{
+ trace_print_key(&trace_foo, message);
+}
+
++
+Note: don't use `const` as the trace implementation stores internal state in
+the `trace_key` structure.
+
+Functions
+-
+
+`int trace_want(struct trace_key *key)`::
+
+ Checks whether the trace key is enabled. Used to prevent expensive
+ string formatting before calling one of the printing APIs.
+
+`void trace_disable(struct trace_key *key)`::
+
+ Disables tracing for the specified key, even if the environment
+ variable was set.
+
+`void trace_printf(const char *format, ...)`::
+`void trace_printf_key(struct trace_key *key, const char *format, ...)`::
+
+ Prints a formatted message, similar to printf.
+
+`void trace_argv_printf(const char **argv, const char *format, ...)``::
+
+ Prints a formatted message, followed by a quoted list of arguments.
+
+`void trace_strbuf(struct trace_key *key, const struct strbuf *data)`::
+
+ Prints the strbuf, without additional formatting (i.e. doesn't
+ choke on `%` or even `\0`).
+
+`uint64_t getnanotime(void)`::
+
+ Returns nanoseconds since the epoch (01/01/1970), typically used
+ for performance measurements.
++
+Currently there are high precision timer implementations for Linux (using
+`clock_gettime(CLOCK_MONOTONIC)`) and Windows (`QueryPerformanceCounter`).
+Other platforms use `gettimeofday` as time source.
+
+`void trace_performance(uint64_t nanos, const char *format, ...)`::
+`void trace_performance_since(uint64_t start, const char *format, ...)`::
+
+ Prints the elapsed time (in nanoseconds), or elapsed time since
+ `start`, followed by a formatted message. Enabled via environment
+ variable `GIT_TRACE_PERFORMANCE`. Used for manual profiling, e.g.:
++
+
+uint64_t start = getnanotime();
+/* code section to measure */
+trace_performance_since(start, "foobar");
+
++
+
+uint64_t t = 0;
+for (;;) {
+ /* ignore */
+ t -= getnanotime();
+ /* code section to measure */
+ t += getnanotime();
+ /* ignore */
+}
+trace_performance(t, "frotz");
+
--
2.0.0.406.g2e9ef9b
--
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 16/17] progress: simplify performance measurement by using getnanotime()
Calculating duration from a single uint64_t is simpler than from a struct
timeval. Change throughput measurement from gettimeofday() to
getnanotime().
Also calculate misec only if needed, and change integer division to integer
multiplication + shift, which should be slightly faster.
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
progress.c | 71 +++---
1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/progress.c b/progress.c
index 261314e..412e6b1 100644
--- a/progress.c
+++ b/progress.c
@@ -12,13 +12,14 @@
#include "gettext.h"
#include "progress.h"
#include "strbuf.h"
+#include "trace.h"
#define TP_IDX_MAX 8
struct throughput {
off_t curr_total;
off_t prev_total;
- struct timeval prev_tv;
+ uint64_t prev_ns;
unsigned int avg_bytes;
unsigned int avg_misecs;
unsigned int last_bytes[TP_IDX_MAX];
@@ -127,65 +128,65 @@ static void throughput_string(struct strbuf *buf, off_t
total,
void display_throughput(struct progress *progress, off_t total)
{
struct throughput *tp;
- struct timeval tv;
- unsigned int misecs;
+ uint64_t now_ns;
+ unsigned int misecs, count, rate;
+ struct strbuf buf = STRBUF_INIT;
if (!progress)
return;
tp = progress->throughput;
- gettimeofday(&tv, NULL);
+ now_ns = getnanotime();
if (!tp) {
progress->throughput = tp = calloc(1, sizeof(*tp));
if (tp) {
tp->prev_total = tp->curr_total = total;
- tp->prev_tv = tv;
+ tp->prev_ns = now_ns;
}
return;
}
tp->curr_total = total;
+ /* only update throughput every 0.5 s */
+ if (now_ns - tp->prev_ns <= 5)
+ return;
+
/*
-* We have x = bytes and y = microsecs. We want z = KiB/s:
+* We have x = bytes and y = nanosecs. We want z = KiB/s:
*
-* z = (x / 1024) / (y / 100)
-* z = x / y * 100 / 1024
-* z = x / (y * 1024 / 100)
+* z = (x / 1024) / (y / 10)
+* z = x / y * 10 / 1024
+* z = x / (y * 1024 / 10)
* z = x / y'
*
* To simplify things we'll keep track of misecs, or 1024th of a sec
* obtained with:
*
-* y' = y * 1024 / 100
-* y' = y / (100 / 1024)
-* y' = y / 977
+* y' = y * 1024 / 10
+* y' = y * (2^10 / 2^42) * (2^42 / 10)
+* y' = y / 2^32 * 4398
+* y' = (y * 4398) >> 32
*/
- misecs = (tv.tv_sec - tp->prev_tv.tv_sec) * 1024;
- misecs += (int)(tv.tv_usec - tp->prev_tv.tv_usec) / 977;
+ misecs = ((now_ns - tp->prev_ns) * 4398) >> 32;
- if (misecs > 512) {
- struct strbuf buf = STRBUF_INIT;
- unsigned int count, rate;
+ count = total - tp->prev_total;
+ tp->prev_total = total;
+ tp->prev_ns = now_ns;
+ tp->avg_bytes += count;
+ tp->avg_misecs += misecs;
+ rate = tp->avg_bytes / tp->avg_misecs;
+ tp->avg_bytes -= tp->last_bytes[tp->idx];
+ tp->avg_misecs -= tp->last_misecs[tp->idx];
+ tp->last_bytes[tp->idx] = count;
+ tp->last_misecs[tp->idx] = misecs;
+ tp->idx = (tp->idx + 1) % TP_IDX_MAX;
- count = total - tp->prev_total;
- tp->prev_total = total;
- tp->prev_tv = tv;
- tp->avg_bytes += count;
- tp->avg_misecs += misecs;
- rate = tp->avg_bytes / tp->avg_misecs;
- tp->avg_bytes -= tp->last_bytes[tp->idx];
- tp->avg_misecs -= tp->last_misecs[tp->idx];
- tp->last_bytes[tp->idx] = count;
- tp->last_misecs[tp->idx] = misecs;
- tp->idx = (tp->idx + 1) % TP_IDX_MAX;
-
- throughput_string(&buf, total, rate);
- strncpy(tp->display, buf.buf, sizeof(tp->display));
- strbuf_release(&buf);
- if (progress->last_value != -1 && progress_update)
- display(progress, progress->last_value, NULL);
- }
+ throughput_string(&buf, total, rate);
+ strncpy(tp->display, buf.buf, sizeof(tp->display));
+ strbuf_release(&buf);
+ if (progress->last_value != -1 && progress_update)
+ display(progress, progress->last_value, NULL);
}
int display_progress(struct progress *progress, unsigned n)
--
2.0.0.406.g2e9ef9b
--
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 15/17] wt-status: simplify performance measurement by using getnanotime()
Calculating duration from a single uint64_t is simpler than from a struct
timeval. Change performance measurement for 'advice.statusuoption' from
gettimeofday() to getnanotime().
Also initialize t_begin to prevent uninitialized variable warning.
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
wt-status.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 318a191..dfdc018 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -574,14 +574,11 @@ static void wt_status_collect_untracked(struct wt_status
*s)
{
int i;
struct dir_struct dir;
- struct timeval t_begin;
+ uint64_t t_begin = getnanotime();
if (!s->show_untracked_files)
return;
- if (advice_status_u_option)
- gettimeofday(&t_begin, NULL);
-
memset(&dir, 0, sizeof(dir));
if (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)
dir.flags |=
@@ -612,13 +609,8 @@ static void wt_status_collect_untracked(struct wt_status
*s)
free(dir.ignored);
clear_directory(&dir);
- if (advice_status_u_option) {
- struct timeval t_end;
- gettimeofday(&t_end, NULL);
- s->untracked_in_ms =
- (uint64_t)t_end.tv_sec * 1000 + t_end.tv_usec / 1000 -
- ((uint64_t)t_begin.tv_sec * 1000 + t_begin.tv_usec /
1000);
- }
+ if (advice_status_u_option)
+ s->untracked_in_ms = (getnanotime() - t_begin) / 100;
}
void wt_status_collect(struct wt_status *s)
--
2.0.0.406.g2e9ef9b
--
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 14/17] git: add performance tracing for git's main() function to debug scripts
Use trace_performance to measure and print execution time and command line
arguments of the entire main() function. In constrast to the shell's 'time'
utility, which measures total time of the parent process, this logs all
involved git commands recursively. This is particularly useful to debug
performance issues of scripted commands (i.e. which git commands were
called with which parameters, and how long did they execute).
Due to git's deliberate use of exit(), the implementation uses an atexit
routine rather than just adding trace_performance_since() at the end of
main().
Usage example: > GIT_TRACE_PERFORMANCE=~/git-trace.log git stash list
Creates a log file like this:
23:57:38.638765 trace.c:405 performance: 0.000310107 s: git command: 'git'
'rev-parse' '--git-dir'
23:57:38.644387 trace.c:405 performance: 0.000261759 s: git command: 'git'
'rev-parse' '--show-toplevel'
23:57:38.646207 trace.c:405 performance: 0.000304468 s: git command: 'git'
'config' '--get-colorbool' 'color.interactive'
23:57:38.648491 trace.c:405 performance: 0.000241667 s: git command: 'git'
'config' '--get-color' 'color.interactive.help' 'red bold'
23:57:38.650465 trace.c:405 performance: 0.000243063 s: git command: 'git'
'config' '--get-color' '' 'reset'
23:57:38.654850 trace.c:405 performance: 0.025126313 s: git command: 'git'
'stash' 'list'
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
Documentation/git.txt | 5 +
git.c | 2 ++
trace.c | 22 ++
trace.h | 1 +
4 files changed, 30 insertions(+)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 9d073f6..fcb8afa 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -938,6 +938,11 @@ Unsetting the variable, or setting it to empty, "0" or
starting with "PACK".
See 'GIT_TRACE' for available trace output options.
+'GIT_TRACE_PERFORMANCE'::
+ Enables performance related trace messages, e.g. total execution
+ time of each Git command.
+ See 'GIT_TRACE' for available trace output options.
+
'GIT_TRACE_SETUP'::
Enables trace messages printing the .git, working tree and current
working directory after Git has completed its setup phase.
diff --git a/git.c b/git.c
index 7780572..d4daeb8 100644
--- a/git.c
+++ b/git.c
@@ -568,6 +568,8 @@ int main(int argc, char **av)
git_setup_gettext();
+ trace_command_performance(argv);
+
/*
* "git-" is the same as "git ", but we obviously:
*
diff --git a/trace.c b/trace.c
index af64dbb..e583dc6 100644
--- a/trace.c
+++ b/trace.c
@@ -404,3 +404,25 @@ inline uint64_t getnanotime(void)
return now;
}
}
+
+static uint64_t command_start_time;
+static struct strbuf command_line = STRBUF_INIT;
+
+static void print_command_performance_atexit(void)
+{
+ trace_performance_since(command_start_time, "git command:%s",
+ command_line.buf);
+}
+
+void trace_command_performance(const char **argv)
+{
+ if (!trace_want(&trace_perf_key))
+ return;
+
+ if (!command_start_time)
+ atexit(print_command_performance_atexit);
+
+ strbuf_reset(&command_line);
+ sq_quote_argv(&command_line, argv, 0);
+ command_start_time = getnanotime();
+}
diff --git a/trace.h b/trace.h
index c843e68..ae6a332 100644
--- a/trace.h
+++ b/trace.h
@@ -17,6 +17,7 @@ extern void trace_repo_setup(const char *prefix);
extern int trace_want(struct trace_key *key);
extern void trace_disable(struct trace_key *key);
extern uint64_t getnanotime(void);
+extern void trace_command_performance(const char **argv);
#ifndef HAVE_VARIADIC_MACROS
--
2.0.0.406.g2e9ef9b
--
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 13/17] trace: add trace_performance facility to debug performance issues
Add trace_performance and trace_performance_since macros that print a
duration and an optional printf-formatted text to the file specified in
environment variable GIT_TRACE_PERFORMANCE.
These macros, in conjunction with getnanotime(), are intended to simplify
performance measurements from within the application (i.e. profiling via
manual instrumentation, rather than using an external profiling tool).
Unless enabled via GIT_TRACE_PERFORMANCE, these macros have no noticeable
impact on performance, so that test code for well known time killers may
be shipped in release builds. Alternatively, a developer could provide an
additional performance patch (not meant for master) that allows reviewers
to reproduce performance tests more easily, e.g. on other platforms or
using their own repositories.
Usage examples:
Simple use case (measure one code section):
uint64_t start = getnanotime();
/* code section to measure */
trace_performance_since(start, "foobar");
Complex use case (measure repetitive code sections):
uint64_t t = 0;
for (;;) {
/* ignore */
t -= getnanotime();
/* code section to measure */
t += getnanotime();
/* ignore */
}
trace_performance(t, "frotz");
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
trace.c | 47 +++
trace.h | 18 ++
2 files changed, 65 insertions(+)
diff --git a/trace.c b/trace.c
index b9d7272..af64dbb 100644
--- a/trace.c
+++ b/trace.c
@@ -169,6 +169,27 @@ void trace_strbuf_fl(const char *file, int line, struct
trace_key *key,
print_trace_line(key, &buf);
}
+static struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
+
+static void trace_performance_vprintf_fl(const char *file, int line,
+uint64_t nanos, const char *format,
+va_list ap)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!prepare_trace_line(file, line, &trace_perf_key, &buf))
+ return;
+
+ strbuf_addf(&buf, "performance: %.9f s", (double) nanos / 10);
+
+ if (format && *format) {
+ strbuf_addstr(&buf, ": ");
+ strbuf_vaddf(&buf, format, ap);
+ }
+
+ print_trace_line(&trace_perf_key, &buf);
+}
+
#ifndef HAVE_VARIADIC_MACROS
void trace_printf(const char *format, ...)
@@ -200,6 +221,23 @@ void trace_strbuf(const char *key, const struct strbuf
*data)
trace_strbuf_fl(NULL, 0, key, data);
}
+void trace_performance(uint64_t nanos, const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_performance_vprintf_fl(NULL, 0, nanos, format, ap);
+ va_end(ap);
+}
+
+void trace_performance_since(uint64_t start, const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_performance_vprintf_fl(NULL, 0, getnanotime() - start,
+format, ap);
+ va_end(ap);
+}
+
#else
void trace_printf_key_fl(const char *file, int line, struct trace_key *key,
@@ -220,6 +258,15 @@ void trace_argv_printf_fl(const char *file, int line,
const char **argv,
va_end(ap);
}
+void trace_performance_fl(const char *file, int line, uint64_t nanos,
+ const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_performance_vprintf_fl(file, line, nanos, format, ap);
+ va_end(ap);
+}
+
#endif /* HAVE_VARIADIC_MACROS */
diff --git a/trace.h b/trace.h
index 4b893a5..c843e68 100644
--- a/trace.h
+++ b/trace.h
@@ -31,6 +31,14 @@ extern void trace_argv_printf(const char **argv, const char
*format, ...);
extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);
+/* Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled. */
+__attribute__((format (printf, 2, 3)))
+extern void trace_performance(uint64_t nanos, const char *format, ...);
+
+/* Prints elapsed time since 'start' if GIT_TRACE_PERFORMANCE is enabled. */
+__attribute__((format (printf, 2, 3)))
+extern void trace_performance_since(uint64_t start, const char *format, ...);
+
#else
/*
@@ -79,6 +87,13 @@ extern void trace_strbuf(struct trace_key *key, const struct
strbuf *data);
#define trace_strbuf(key, data) \
trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data)
+#define trace_performance(nanos, ...) \
+ trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos, __VA_ARGS__)
+
+#define trace_performance_since(start, ...) \
+ trace_performance_fl(TRACE_CONTEXT, __LINE__, getnanotime() - (start), \
+__VA_ARGS__)
+
/* backend functions, use non-*fl macros instead */
__attribute__((format (printf, 4, 5)))
extern void trace_printf_key_fl(const char *file, int line, struct trace_key
*key,
@@ -88,6 +103,9 @@ extern void trace_argv_printf_fl(const char *file, int line,
const char **argv,
const ch
[PATCH v8 12/17] trace: add high resolution timer function to debug performance issues
Add a getnanotime() function that returns nanoseconds since 01/01/1970 as
unsigned 64-bit integer (i.e. overflows in july 2554). This is easier to
work with than e.g. struct timeval or struct timespec. Basing the timer on
the epoch allows using the results with other time-related APIs.
To simplify adaption to different platforms, split the implementation into
a common getnanotime() and a platform-specific highres_nanos() function.
The common getnanotime() function handles errors, falling back to
gettimeofday() if highres_nanos() isn't implemented or doesn't work.
getnanotime() is also responsible for normalizing to the epoch. The offset
to the system clock is calculated only once on initialization, i.e.
manually setting the system clock has no impact on the timer (except if
the fallback gettimeofday() is in use). Git processes are typically short
lived, so we don't need to handle clock drift.
The highres_nanos() function returns monotonically increasing nanoseconds
relative to some arbitrary point in time (e.g. system boot), or 0 on
failure. Providing platform-specific implementations should be relatively
easy, e.g. adapting to clock_gettime() as defined by the POSIX realtime
extensions is seven lines of code.
This version includes highres_nanos() implementations for:
* Linux: using clock_gettime(CLOCK_MONOTONIC)
* Windows: using QueryPerformanceCounter()
Todo:
* enable clock_gettime() on more platforms
* add Mac OSX version, e.g. using mach_absolute_time + mach_timebase_info
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
Makefile | 7 +
config.mak.uname | 1 +
trace.c | 82
trace.h | 1 +
4 files changed, 91 insertions(+)
diff --git a/Makefile b/Makefile
index 07ea105..80f4390 100644
--- a/Makefile
+++ b/Makefile
@@ -340,6 +340,8 @@ all::
#
# Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not
# return NULL when it receives a bogus time_t.
+#
+# Define HAVE_CLOCK_GETTIME if your platform has clock_gettime in librt.
GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1497,6 +1499,11 @@ ifdef GMTIME_UNRELIABLE_ERRORS
BASIC_CFLAGS += -DGMTIME_UNRELIABLE_ERRORS
endif
+ifdef HAVE_CLOCK_GETTIME
+ BASIC_CFLAGS += -DHAVE_CLOCK_GETTIME
+ EXTLIBS += -lrt
+endif
+
ifeq ($(TCLTK_PATH),)
NO_TCLTK = NoThanks
endif
diff --git a/config.mak.uname b/config.mak.uname
index 1ae675b..dad2618 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -34,6 +34,7 @@ ifeq ($(uname_S),Linux)
HAVE_PATHS_H = YesPlease
LIBC_CONTAINS_LIBINTL = YesPlease
HAVE_DEV_TTY = YesPlease
+ HAVE_CLOCK_GETTIME = YesPlease
endif
ifeq ($(uname_S),GNU/kFreeBSD)
HAVE_ALLOCA_H = YesPlease
diff --git a/trace.c b/trace.c
index f013958..b9d7272 100644
--- a/trace.c
+++ b/trace.c
@@ -275,3 +275,85 @@ int trace_want(struct trace_key *key)
{
return !!get_trace_fd(key);
}
+
+#ifdef HAVE_CLOCK_GETTIME
+
+static inline uint64_t highres_nanos(void)
+{
+ struct timespec ts;
+ if (clock_gettime(CLOCK_MONOTONIC, &ts))
+ return 0;
+ return (uint64_t) ts.tv_sec * 10 + ts.tv_nsec;
+}
+
+#elif defined (GIT_WINDOWS_NATIVE)
+
+static inline uint64_t highres_nanos(void)
+{
+ static uint64_t high_ns, scaled_low_ns;
+ static int scale;
+ LARGE_INTEGER cnt;
+
+ if (!scale) {
+ if (!QueryPerformanceFrequency(&cnt))
+ return 0;
+
+ /* high_ns = number of ns per cnt.HighPart */
+ high_ns = (10LL << 32) / (uint64_t) cnt.QuadPart;
+
+ /*
+* Number of ns per cnt.LowPart is 10^9 / frequency (or
+* high_ns >> 32). For maximum precision, we scale this factor
+* so that it just fits within 32 bit (i.e. won't overflow if
+* multiplied with cnt.LowPart).
+*/
+ scaled_low_ns = high_ns;
+ scale = 32;
+ while (scaled_low_ns >= 0x1LL) {
+ scaled_low_ns >>= 1;
+ scale--;
+ }
+ }
+
+ /* if QPF worked on initialization, we expect QPC to work as well */
+ QueryPerformanceCounter(&cnt);
+
+ return (high_ns * cnt.HighPart) +
+ ((scaled_low_ns * cnt.LowPart) >> scale);
+}
+
+#else
+# define highres_nanos() 0
+#endif
+
+static inline uint64_t gettimeofday_nanos(void)
+{
+ struct timeval tv;
+ gettimeofday(&tv, NULL);
+ return (uint64_t) tv.tv_sec * 10 + tv.tv_usec * 1000;
+}
+
+/*
+ * Returns nanoseconds since the epoch (01/01/1970), for performance tracing
+ * (i.e. favoring high precision over wall clock time accuracy).
+ */
+inline uint64_t getnanotime(void)
+{
+ static uint64_t offset;
+ if (offset > 1) {
+ /* initialization succeeded, return off
[PATCH v8 10/17] trace: move code around, in preparation to file:line output
No functional changes, just move stuff around so that the next patch isn't
that ugly...
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
trace.c | 36 ++--
trace.h | 12
2 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/trace.c b/trace.c
index 18e5d93..e8ce619 100644
--- a/trace.c
+++ b/trace.c
@@ -132,20 +132,20 @@ static void trace_vprintf(struct trace_key *key, const
char *format, va_list ap)
print_trace_line(key, &buf);
}
-void trace_printf_key(struct trace_key *key, const char *format, ...)
+void trace_argv_printf(const char **argv, const char *format, ...)
{
+ struct strbuf buf = STRBUF_INIT;
va_list ap;
- va_start(ap, format);
- trace_vprintf(key, format, ap);
- va_end(ap);
-}
-void trace_printf(const char *format, ...)
-{
- va_list ap;
+ if (!prepare_trace_line(NULL, &buf))
+ return;
+
va_start(ap, format);
- trace_vprintf(NULL, format, ap);
+ strbuf_vaddf(&buf, format, ap);
va_end(ap);
+
+ sq_quote_argv(&buf, argv, 0);
+ print_trace_line(NULL, &buf);
}
void trace_strbuf(struct trace_key *key, const struct strbuf *data)
@@ -159,20 +159,20 @@ void trace_strbuf(struct trace_key *key, const struct
strbuf *data)
print_trace_line(key, &buf);
}
-void trace_argv_printf(const char **argv, const char *format, ...)
+void trace_printf(const char *format, ...)
{
- struct strbuf buf = STRBUF_INIT;
va_list ap;
-
- if (!prepare_trace_line(NULL, &buf))
- return;
-
va_start(ap, format);
- strbuf_vaddf(&buf, format, ap);
+ trace_vprintf(NULL, format, ap);
va_end(ap);
+}
- sq_quote_argv(&buf, argv, 0);
- print_trace_line(NULL, &buf);
+void trace_printf_key(struct trace_key *key, const char *format, ...)
+{
+ va_list ap;
+ va_start(ap, format);
+ trace_vprintf(key, format, ap);
+ va_end(ap);
}
static const char *quote_crnl(const char *path)
diff --git a/trace.h b/trace.h
index 28c1089..b4800e7 100644
--- a/trace.h
+++ b/trace.h
@@ -13,15 +13,19 @@ struct trace_key {
#define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
-__attribute__((format (printf, 1, 2)))
-extern void trace_printf(const char *format, ...);
-__attribute__((format (printf, 2, 3)))
-extern void trace_argv_printf(const char **argv, const char *format, ...);
extern void trace_repo_setup(const char *prefix);
extern int trace_want(struct trace_key *key);
extern void trace_disable(struct trace_key *key);
+
+__attribute__((format (printf, 1, 2)))
+extern void trace_printf(const char *format, ...);
+
__attribute__((format (printf, 2, 3)))
extern void trace_printf_key(struct trace_key *key, const char *format, ...);
+
+__attribute__((format (printf, 2, 3)))
+extern void trace_argv_printf(const char **argv, const char *format, ...);
+
extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);
#endif /* TRACE_H */
--
2.0.0.406.g2e9ef9b
--
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 11/17] trace: add 'file:line' to all trace output
This is useful to see where trace output came from.
Add 'const char *file, int line' parameters to the printing functions and
rename them to *_fl.
Add trace_printf* and trace_strbuf macros resolving to the *_fl functions
and let the preprocessor fill in __FILE__ and __LINE__.
As the trace_printf* functions take a variable number of arguments, this
requires variadic macros (i.e. '#define foo(...) foo_impl(__VA_ARGS__)'.
Though part of C99, it is unclear whether older compilers support this.
Thus keep the old functions and only enable variadic macros for GNUC and
MSVC 2005+ (_MSC_VER 1400). This has the nice side effect that the old
C-style declarations serve as documentation how the macros are to be used.
Print 'file:line ' as prefix to each trace line. Align the remaining trace
output at column 40 to accommodate 18 char file names + 4 digit line
number (currently there are 30 *.c files of length 18 and just 11 of 19).
Trace output from longer source files (e.g. builtin/receive-pack.c) will
not be aligned.
Signed-off-by: Karsten Blees
---
git-compat-util.h | 4
trace.c | 72 +--
trace.h | 62 +++
3 files changed, 126 insertions(+), 12 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index b6f03b3..4dd065d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -704,6 +704,10 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#endif
#endif
+#if defined(__GNUC__) || (_MSC_VER >= 1400)
+#define HAVE_VARIADIC_MACROS 1
+#endif
+
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
* Always returns the return value of unlink(2).
diff --git a/trace.c b/trace.c
index e8ce619..f013958 100644
--- a/trace.c
+++ b/trace.c
@@ -85,7 +85,8 @@ void trace_disable(struct trace_key *key)
static const char err_msg[] = "Could not trace into fd given by "
"GIT_TRACE environment variable";
-static int prepare_trace_line(struct trace_key *key, struct strbuf *buf)
+static int prepare_trace_line(const char *file, int line,
+ struct trace_key *key, struct strbuf *buf)
{
static struct trace_key trace_bare = TRACE_KEY_INIT(BARE);
struct timeval tv;
@@ -108,6 +109,14 @@ static int prepare_trace_line(struct trace_key *key,
struct strbuf *buf)
strbuf_addf(buf, "%02d:%02d:%02d.%06ld ", tm.tm_hour, tm.tm_min,
tm.tm_sec, (long) tv.tv_usec);
+#ifdef HAVE_VARIADIC_MACROS
+ /* print file:line */
+ strbuf_addf(buf, "%s:%d ", file, line);
+ /* align trace output (column 40 catches most files names in git) */
+ while (buf->len < 40)
+ strbuf_addch(buf, ' ');
+#endif
+
return 1;
}
@@ -121,49 +130,52 @@ static void print_trace_line(struct trace_key *key,
struct strbuf *buf)
strbuf_release(buf);
}
-static void trace_vprintf(struct trace_key *key, const char *format, va_list
ap)
+static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,
+const char *format, va_list ap)
{
struct strbuf buf = STRBUF_INIT;
- if (!prepare_trace_line(key, &buf))
+ if (!prepare_trace_line(file, line, key, &buf))
return;
strbuf_vaddf(&buf, format, ap);
print_trace_line(key, &buf);
}
-void trace_argv_printf(const char **argv, const char *format, ...)
+static void trace_argv_vprintf_fl(const char *file, int line,
+ const char **argv, const char *format,
+ va_list ap)
{
struct strbuf buf = STRBUF_INIT;
- va_list ap;
- if (!prepare_trace_line(NULL, &buf))
+ if (!prepare_trace_line(file, line, NULL, &buf))
return;
- va_start(ap, format);
strbuf_vaddf(&buf, format, ap);
- va_end(ap);
sq_quote_argv(&buf, argv, 0);
print_trace_line(NULL, &buf);
}
-void trace_strbuf(struct trace_key *key, const struct strbuf *data)
+void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
+const struct strbuf *data)
{
struct strbuf buf = STRBUF_INIT;
- if (!prepare_trace_line(key, &buf))
+ if (!prepare_trace_line(file, line, key, &buf))
return;
strbuf_addbuf(&buf, data);
print_trace_line(key, &buf);
}
+#ifndef HAVE_VARIADIC_MACROS
+
void trace_printf(const char *format, ...)
{
va_list ap;
va_start(ap, format);
- trace_vprintf(NULL, format, ap);
+ trace_vprintf_fl(NULL, 0, NULL, format, ap);
va_end(ap);
}
@@ -171,10 +183,46 @@ void trace_printf_key(struct trace_key *key, const char
*format, ...)
{
va_list ap;
va_start(ap, format);
- trace_vprintf(key, format, ap);
+ trace_vprintf_fl(NULL, 0, key, format, ap);
+ va_end(ap);
+}
+
+void trace_argv_printf
[PATCH v8 08/17] trace: disable additional trace output for unit tests
Some unit-tests use trace output to verify internal state, and unstable
output such as timestamps and line numbers are not useful there.
Disable additional trace output if GIT_TRACE_BARE is set.
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
t/test-lib.sh | 4
trace.c | 6 ++
2 files changed, 10 insertions(+)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 81394c8..e37da5a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -109,6 +109,10 @@ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
export EDITOR
+# Tests using GIT_TRACE typically don't want : output
+GIT_TRACE_BARE=1
+export GIT_TRACE_BARE
+
if test -n "${TEST_GIT_INDEX_VERSION:+isset}"
then
GIT_INDEX_VERSION="$TEST_GIT_INDEX_VERSION"
diff --git a/trace.c b/trace.c
index 3d02bcc..a194b16 100644
--- a/trace.c
+++ b/trace.c
@@ -87,11 +87,17 @@ static const char err_msg[] = "Could not trace into fd
given by "
static int prepare_trace_line(struct trace_key *key, struct strbuf *buf)
{
+ static struct trace_key trace_bare = TRACE_KEY_INIT(BARE);
+
if (!trace_want(key))
return 0;
set_try_to_free_routine(NULL); /* is never reset */
+ /* unit tests may want to disable additional trace output */
+ if (trace_want(&trace_bare))
+ return 1;
+
/* add line prefix here */
return 1;
--
2.0.0.406.g2e9ef9b
--
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 09/17] trace: add current timestamp to all trace output
This is useful to tell apart trace output of separate test runs.
It can also be used for basic, coarse-grained performance analysis. Note
that the accuracy is tainted by writing to the trace file, and you have to
calculate the deltas yourself (which is next to impossible if multiple
threads or processes are involved).
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
trace.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/trace.c b/trace.c
index a194b16..18e5d93 100644
--- a/trace.c
+++ b/trace.c
@@ -88,6 +88,9 @@ static const char err_msg[] = "Could not trace into fd given
by "
static int prepare_trace_line(struct trace_key *key, struct strbuf *buf)
{
static struct trace_key trace_bare = TRACE_KEY_INIT(BARE);
+ struct timeval tv;
+ struct tm tm;
+ time_t secs;
if (!trace_want(key))
return 0;
@@ -98,7 +101,12 @@ static int prepare_trace_line(struct trace_key *key, struct
strbuf *buf)
if (trace_want(&trace_bare))
return 1;
- /* add line prefix here */
+ /* print current timestamp */
+ gettimeofday(&tv, NULL);
+ secs = tv.tv_sec;
+ localtime_r(&secs, &tm);
+ strbuf_addf(buf, "%02d:%02d:%02d.%06ld ", tm.tm_hour, tm.tm_min,
+ tm.tm_sec, (long) tv.tv_usec);
return 1;
}
--
2.0.0.406.g2e9ef9b
--
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 06/17] sha1_file: change GIT_TRACE_PACK_ACCESS logging to use trace API
This changes GIT_TRACE_PACK_ACCESS functionality as follows:
* supports the same options as GIT_TRACE (e.g. printing to stderr)
* no longer supports relative paths
* appends to the trace file rather than overwriting
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
Documentation/git.txt | 4 ++--
sha1_file.c | 30 --
2 files changed, 6 insertions(+), 28 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 75633e6..9d073f6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -925,11 +925,11 @@ Unsetting the variable, or setting it to empty, "0" or
"false" (case insensitive) disables trace messages.
'GIT_TRACE_PACK_ACCESS'::
- If this variable is set to a path, a file will be created at
- the given path logging all accesses to any packs. For each
+ Enables trace messages for all accesses to any packs. For each
access, the pack file name and an offset in the pack is
recorded. This may be helpful for troubleshooting some
pack-related performance problems.
+ See 'GIT_TRACE' for available trace output options.
'GIT_TRACE_PACKET'::
Enables trace messages for all packets coming in or out of a
diff --git a/sha1_file.c b/sha1_file.c
index 34d527f..7a110b5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -36,9 +36,6 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }
const unsigned char null_sha1[20];
-static const char *no_log_pack_access = "no_log_pack_access";
-static const char *log_pack_access;
-
/*
* This is meant to hold a *small* number of objects that you would
* want read_sha1_file() to be able to return, but yet you do not want
@@ -2085,27 +2082,9 @@ static void *read_object(const unsigned char *sha1, enum
object_type *type,
static void write_pack_access_log(struct packed_git *p, off_t obj_offset)
{
- static FILE *log_file;
-
- if (!log_pack_access)
- log_pack_access = getenv("GIT_TRACE_PACK_ACCESS");
- if (!log_pack_access)
- log_pack_access = no_log_pack_access;
- if (log_pack_access == no_log_pack_access)
- return;
-
- if (!log_file) {
- log_file = fopen(log_pack_access, "w");
- if (!log_file) {
- error("cannot open pack access log '%s' for writing:
%s",
- log_pack_access, strerror(errno));
- log_pack_access = no_log_pack_access;
- return;
- }
- }
- fprintf(log_file, "%s %"PRIuMAX"\n",
- p->pack_name, (uintmax_t)obj_offset);
- fflush(log_file);
+ static struct trace_key pack_access = TRACE_KEY_INIT(PACK_ACCESS);
+ trace_printf_key(&pack_access, "%s %"PRIuMAX"\n",
+p->pack_name, (uintmax_t)obj_offset);
}
int do_check_packed_object_crc;
@@ -2130,8 +2109,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
int delta_stack_nr = 0, delta_stack_alloc = UNPACK_ENTRY_STACK_PREALLOC;
int base_from_cache = 0;
- if (log_pack_access != no_log_pack_access)
- write_pack_access_log(p, obj_offset);
+ write_pack_access_log(p, obj_offset);
/* PHASE 1: drill down to the innermost base object */
for (;;) {
--
2.0.0.406.g2e9ef9b
--
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 07/17] trace: add infrastructure to augment trace output with additional info
To be able to add a common prefix or suffix to all trace output (e.g.
a timestamp or file:line of the caller), factor out common setup and
cleanup tasks of the trace* functions.
When adding a common prefix, it makes sense that the output of each trace
call starts on a new line. Add '\n' in case the caller forgot.
Note that this explicitly limits trace output to line-by-line, it is no
longer possible to trace-print just part of a line. Until now, this was
just an implicit assumption (trace-printing part of a line worked, but
messed up the trace file if multiple threads or processes were involved).
Thread-safety / inter-process-safety is also the reason why we need to do
the prefixing and suffixing in memory rather than issuing multiple write()
calls. Write_or_whine_pipe() / xwrite() is atomic unless the size exceeds
MAX_IO_SIZE (8MB, see wrapper.c). In case of trace_strbuf, this costs an
additional string copy (which should be irrelevant for performance in light
of actual file IO).
While we're at it, rename trace_strbuf's 'buf' argument, which suggests
that the function is modifying the buffer. Trace_strbuf() currently is the
only trace API that can print arbitrary binary data (without barfing on
'%' or stopping at '\0'), so 'data' seems more appropriate.
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
trace.c | 47 +--
trace.h | 2 +-
2 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/trace.c b/trace.c
index 8662b79..3d02bcc 100644
--- a/trace.c
+++ b/trace.c
@@ -85,17 +85,37 @@ void trace_disable(struct trace_key *key)
static const char err_msg[] = "Could not trace into fd given by "
"GIT_TRACE environment variable";
+static int prepare_trace_line(struct trace_key *key, struct strbuf *buf)
+{
+ if (!trace_want(key))
+ return 0;
+
+ set_try_to_free_routine(NULL); /* is never reset */
+
+ /* add line prefix here */
+
+ return 1;
+}
+
+static void print_trace_line(struct trace_key *key, struct strbuf *buf)
+{
+ /* append newline if missing */
+ if (buf->len && buf->buf[buf->len - 1] != '\n')
+ strbuf_addch(buf, '\n');
+
+ write_or_whine_pipe(get_trace_fd(key), buf->buf, buf->len, err_msg);
+ strbuf_release(buf);
+}
+
static void trace_vprintf(struct trace_key *key, const char *format, va_list
ap)
{
struct strbuf buf = STRBUF_INIT;
- if (!trace_want(key))
+ if (!prepare_trace_line(key, &buf))
return;
- set_try_to_free_routine(NULL); /* is never reset */
strbuf_vaddf(&buf, format, ap);
- trace_strbuf(key, &buf);
- strbuf_release(&buf);
+ print_trace_line(key, &buf);
}
void trace_printf_key(struct trace_key *key, const char *format, ...)
@@ -114,32 +134,31 @@ void trace_printf(const char *format, ...)
va_end(ap);
}
-void trace_strbuf(struct trace_key *key, const struct strbuf *buf)
+void trace_strbuf(struct trace_key *key, const struct strbuf *data)
{
- int fd = get_trace_fd(key);
- if (!fd)
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!prepare_trace_line(key, &buf))
return;
- write_or_whine_pipe(fd, buf->buf, buf->len, err_msg);
+ strbuf_addbuf(&buf, data);
+ print_trace_line(key, &buf);
}
void trace_argv_printf(const char **argv, const char *format, ...)
{
struct strbuf buf = STRBUF_INIT;
va_list ap;
- int fd = get_trace_fd(NULL);
- if (!fd)
+
+ if (!prepare_trace_line(NULL, &buf))
return;
- set_try_to_free_routine(NULL); /* is never reset */
va_start(ap, format);
strbuf_vaddf(&buf, format, ap);
va_end(ap);
sq_quote_argv(&buf, argv, 0);
- strbuf_addch(&buf, '\n');
- write_or_whine_pipe(fd, buf.buf, buf.len, err_msg);
- strbuf_release(&buf);
+ print_trace_line(NULL, &buf);
}
static const char *quote_crnl(const char *path)
diff --git a/trace.h b/trace.h
index d85ac4c..28c1089 100644
--- a/trace.h
+++ b/trace.h
@@ -22,6 +22,6 @@ extern int trace_want(struct trace_key *key);
extern void trace_disable(struct trace_key *key);
__attribute__((format (printf, 2, 3)))
extern void trace_printf_key(struct trace_key *key, const char *format, ...);
-extern void trace_strbuf(struct trace_key *key, const struct strbuf *buf);
+extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);
#endif /* TRACE_H */
--
2.0.0.406.g2e9ef9b
--
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 05/17] Documentation/git.txt: improve documentation of 'GIT_TRACE*' variables
Separate GIT_TRACE description into what it prints and how to configure where trace output is printed to. Change other GIT_TRACE_* descriptions to refer to GIT_TRACE. Add descriptions for GIT_TRACE_SETUP and GIT_TRACE_SHALLOW. Signed-off-by: Karsten Blees Signed-off-by: Junio C Hamano --- Documentation/git.txt | 50 ++ 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index 3bd68b0..75633e6 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -904,18 +904,25 @@ for further details. based on whether stdout appears to be redirected to a file or not. 'GIT_TRACE':: - If this variable is set to "1", "2" or "true" (comparison - is case insensitive), Git will print `trace:` messages on - stderr telling about alias expansion, built-in command - execution and external command execution. - If this variable is set to an integer value greater than 1 - and lower than 10 (strictly) then Git will interpret this - value as an open file descriptor and will try to write the - trace messages into this file descriptor. - Alternatively, if this variable is set to an absolute path - (starting with a '/' character), Git will interpret this - as a file path and will try to write the trace messages - into it. + Enables general trace messages, e.g. alias expansion, built-in + command execution and external command execution. ++ +If this variable is set to "1", "2" or "true" (comparison +is case insensitive), trace messages will be printed to +stderr. ++ +If the variable is set to an integer value greater than 2 +and lower than 10 (strictly) then Git will interpret this +value as an open file descriptor and will try to write the +trace messages into this file descriptor. ++ +Alternatively, if the variable is set to an absolute path +(starting with a '/' character), Git will interpret this +as a file path and will try to write the trace messages +into it. ++ +Unsetting the variable, or setting it to empty, "0" or +"false" (case insensitive) disables trace messages. 'GIT_TRACE_PACK_ACCESS':: If this variable is set to a path, a file will be created at @@ -925,10 +932,21 @@ for further details. pack-related performance problems. 'GIT_TRACE_PACKET':: - If this variable is set, it shows a trace of all packets - coming in or out of a given program. This can help with - debugging object negotiation or other protocol issues. Tracing - is turned off at a packet starting with "PACK". + Enables trace messages for all packets coming in or out of a + given program. This can help with debugging object negotiation + or other protocol issues. Tracing is turned off at a packet + starting with "PACK". + See 'GIT_TRACE' for available trace output options. + +'GIT_TRACE_SETUP':: + Enables trace messages printing the .git, working tree and current + working directory after Git has completed its setup phase. + See 'GIT_TRACE' for available trace output options. + +'GIT_TRACE_SHALLOW':: + Enables trace messages that can help debugging fetching / + cloning of shallow repositories. + See 'GIT_TRACE' for available trace output options. GIT_LITERAL_PATHSPECS:: Setting this variable to `1` will cause Git to treat all -- 2.0.0.406.g2e9ef9b -- 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 04/17] trace: improve trace performance
The trace API currently rechecks the environment variable and reopens the
trace file on every API call. This has the ugly side effect that errors
(e.g. file cannot be opened, or the user specified a relative path) are
also reported on every call. Performance can be improved by about factor
three by remembering the environment state and keeping the file open.
Replace the 'const char *key' parameter in the API with a pointer to a
'struct trace_key' that bundles the environment variable name with
additional, trace-internal state. Change the call sites of these APIs to
use a static 'struct trace_key' instead of a string constant.
In trace.c::get_trace_fd(), save and reuse the file descriptor in 'struct
trace_key'.
Add a 'trace_disable()' API, so that packet_trace() can cleanly disable
tracing when it encounters packed data (instead of using unsetenv()).
Signed-off-by: Karsten Blees
---
builtin/receive-pack.c | 2 +-
commit.h | 1 +
pkt-line.c | 8 ++--
shallow.c | 10 ++---
trace.c| 100 ++---
trace.h| 16 ++--
6 files changed, 78 insertions(+), 59 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..1451050 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -438,7 +438,7 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
uint32_t mask = 1 << (cmd->index % 32);
int i;
- trace_printf_key("GIT_TRACE_SHALLOW",
+ trace_printf_key(&trace_shallow,
"shallow: update_shallow_ref %s\n", cmd->ref_name);
for (i = 0; i < si->shallow->nr; i++)
if (si->used_shallow[i] &&
diff --git a/commit.h b/commit.h
index a9f177b..08ef643 100644
--- a/commit.h
+++ b/commit.h
@@ -235,6 +235,7 @@ extern void assign_shallow_commits_to_refs(struct
shallow_info *info,
int *ref_status);
extern int delayed_reachability_test(struct shallow_info *si, int c);
extern void prune_shallow(int show_only);
+extern struct trace_key trace_shallow;
int is_descendant_of(struct commit *, struct commit_list *);
int in_merge_bases(struct commit *, struct commit *);
diff --git a/pkt-line.c b/pkt-line.c
index bc63b3b..8bc89b1 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -3,7 +3,7 @@
char packet_buffer[LARGE_PACKET_MAX];
static const char *packet_trace_prefix = "git";
-static const char trace_key[] = "GIT_TRACE_PACKET";
+static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET);
void packet_trace_identity(const char *prog)
{
@@ -15,7 +15,7 @@ static void packet_trace(const char *buf, unsigned int len,
int write)
int i;
struct strbuf out;
- if (!trace_want(trace_key))
+ if (!trace_want(&trace_packet))
return;
/* +32 is just a guess for header + quoting */
@@ -27,7 +27,7 @@ static void packet_trace(const char *buf, unsigned int len,
int write)
if ((len >= 4 && starts_with(buf, "PACK")) ||
(len >= 5 && starts_with(buf+1, "PACK"))) {
strbuf_addstr(&out, "PACK ...");
- unsetenv(trace_key);
+ trace_disable(&trace_packet);
}
else {
/* XXX we should really handle printable utf8 */
@@ -43,7 +43,7 @@ static void packet_trace(const char *buf, unsigned int len,
int write)
}
strbuf_addch(&out, '\n');
- trace_strbuf(trace_key, &out);
+ trace_strbuf(&trace_packet, &out);
strbuf_release(&out);
}
diff --git a/shallow.c b/shallow.c
index 0b267b6..de07709 100644
--- a/shallow.c
+++ b/shallow.c
@@ -325,7 +325,7 @@ void prune_shallow(int show_only)
strbuf_release(&sb);
}
-#define TRACE_KEY "GIT_TRACE_SHALLOW"
+struct trace_key trace_shallow = TRACE_KEY_INIT(SHALLOW);
/*
* Step 1, split sender shallow commits into "ours" and "theirs"
@@ -334,7 +334,7 @@ void prune_shallow(int show_only)
void prepare_shallow_info(struct shallow_info *info, struct sha1_array *sa)
{
int i;
- trace_printf_key(TRACE_KEY, "shallow: prepare_shallow_info\n");
+ trace_printf_key(&trace_shallow, "shallow: prepare_shallow_info\n");
memset(info, 0, sizeof(*info));
info->shallow = sa;
if (!sa)
@@ -365,7 +365,7 @@ void remove_nonexistent_theirs_shallow(struct shallow_info
*info)
{
unsigned char (*sha1)[20] = info->shallow->sha1;
int i, dst;
- trace_printf_key(TRACE_KEY, "shallow:
remove_nonexistent_theirs_shallow\n");
+ trace_printf_key(&trace_shallow, "shallow:
remove_nonexistent_theirs_shallow\n");
for (i = dst = 0; i < info->nr_theirs; i++) {
if (i != dst)
info->theirs[dst] = info->theirs[i];
@@ -516,7 +516,7 @@ void assign_shallow_commits_to_refs(struct shallow_info
*info,
int *shallow, nr_shallow = 0;
struct pain
[PATCH v8 03/17] trace: remove redundant printf format attribute
trace_printf_key() is the only non-static function that duplicates the
printf format attribute in the .c file, remove it for consistency.
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
trace.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/trace.c b/trace.c
index 37a7fa9..3e31558 100644
--- a/trace.c
+++ b/trace.c
@@ -75,7 +75,6 @@ static void trace_vprintf(const char *key, const char
*format, va_list ap)
strbuf_release(&buf);
}
-__attribute__((format (printf, 2, 3)))
void trace_printf_key(const char *key, const char *format, ...)
{
va_list ap;
--
2.0.0.406.g2e9ef9b
--
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 02/17] trace: consistently name the format parameter
The format parameter to trace_printf functions is sometimes abbreviated
'fmt'. Rename to 'format' everywhere (consistent with POSIX' printf
specification).
Signed-off-by: Karsten Blees
Signed-off-by: Junio C Hamano
---
trace.c | 22 +++---
trace.h | 2 +-
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/trace.c b/trace.c
index 08180a9..37a7fa9 100644
--- a/trace.c
+++ b/trace.c
@@ -62,7 +62,7 @@ static int get_trace_fd(const char *key, int *need_close)
static const char err_msg[] = "Could not trace into fd given by "
"GIT_TRACE environment variable";
-static void trace_vprintf(const char *key, const char *fmt, va_list ap)
+static void trace_vprintf(const char *key, const char *format, va_list ap)
{
struct strbuf buf = STRBUF_INIT;
@@ -70,25 +70,25 @@ static void trace_vprintf(const char *key, const char *fmt,
va_list ap)
return;
set_try_to_free_routine(NULL); /* is never reset */
- strbuf_vaddf(&buf, fmt, ap);
+ strbuf_vaddf(&buf, format, ap);
trace_strbuf(key, &buf);
strbuf_release(&buf);
}
__attribute__((format (printf, 2, 3)))
-void trace_printf_key(const char *key, const char *fmt, ...)
+void trace_printf_key(const char *key, const char *format, ...)
{
va_list ap;
- va_start(ap, fmt);
- trace_vprintf(key, fmt, ap);
+ va_start(ap, format);
+ trace_vprintf(key, format, ap);
va_end(ap);
}
-void trace_printf(const char *fmt, ...)
+void trace_printf(const char *format, ...)
{
va_list ap;
- va_start(ap, fmt);
- trace_vprintf("GIT_TRACE", fmt, ap);
+ va_start(ap, format);
+ trace_vprintf("GIT_TRACE", format, ap);
va_end(ap);
}
@@ -106,7 +106,7 @@ void trace_strbuf(const char *key, const struct strbuf *buf)
close(fd);
}
-void trace_argv_printf(const char **argv, const char *fmt, ...)
+void trace_argv_printf(const char **argv, const char *format, ...)
{
struct strbuf buf = STRBUF_INIT;
va_list ap;
@@ -117,8 +117,8 @@ void trace_argv_printf(const char **argv, const char *fmt,
...)
return;
set_try_to_free_routine(NULL); /* is never reset */
- va_start(ap, fmt);
- strbuf_vaddf(&buf, fmt, ap);
+ va_start(ap, format);
+ strbuf_vaddf(&buf, format, ap);
va_end(ap);
sq_quote_argv(&buf, argv, 0);
diff --git a/trace.h b/trace.h
index 6cc4541..8fea50b 100644
--- a/trace.h
+++ b/trace.h
@@ -11,7 +11,7 @@ extern void trace_argv_printf(const char **argv, const char
*format, ...);
extern void trace_repo_setup(const char *prefix);
extern int trace_want(const char *key);
__attribute__((format (printf, 2, 3)))
-extern void trace_printf_key(const char *key, const char *fmt, ...);
+extern void trace_printf_key(const char *key, const char *format, ...);
extern void trace_strbuf(const char *key, const struct strbuf *buf);
#endif /* TRACE_H */
--
2.0.0.406.g2e9ef9b
--
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 01/17] trace: move trace declarations from cache.h to new trace.h
Also include direct dependencies (strbuf.h and git-compat-util.h for __attribute__) so that trace.h can be used independently of cache.h, e.g. in test programs. Signed-off-by: Karsten Blees Signed-off-by: Junio C Hamano --- cache.h | 13 ++--- trace.h | 17 + 2 files changed, 19 insertions(+), 11 deletions(-) create mode 100644 trace.h diff --git a/cache.h b/cache.h index cbe1935..466f6b3 100644 --- a/cache.h +++ b/cache.h @@ -7,6 +7,7 @@ #include "advice.h" #include "gettext.h" #include "convert.h" +#include "trace.h" #include SHA1_HEADER #ifndef git_SHA_CTX @@ -1377,17 +1378,7 @@ extern void *alloc_tag_node(void); extern void *alloc_object_node(void); extern void alloc_report(void); -/* trace.c */ -__attribute__((format (printf, 1, 2))) -extern void trace_printf(const char *format, ...); -__attribute__((format (printf, 2, 3))) -extern void trace_argv_printf(const char **argv, const char *format, ...); -extern void trace_repo_setup(const char *prefix); -extern int trace_want(const char *key); -__attribute__((format (printf, 2, 3))) -extern void trace_printf_key(const char *key, const char *fmt, ...); -extern void trace_strbuf(const char *key, const struct strbuf *buf); - +/* pkt-line.c */ void packet_trace_identity(const char *prog); /* add */ diff --git a/trace.h b/trace.h new file mode 100644 index 000..6cc4541 --- /dev/null +++ b/trace.h @@ -0,0 +1,17 @@ +#ifndef TRACE_H +#define TRACE_H + +#include "git-compat-util.h" +#include "strbuf.h" + +__attribute__((format (printf, 1, 2))) +extern void trace_printf(const char *format, ...); +__attribute__((format (printf, 2, 3))) +extern void trace_argv_printf(const char **argv, const char *format, ...); +extern void trace_repo_setup(const char *prefix); +extern int trace_want(const char *key); +__attribute__((format (printf, 2, 3))) +extern void trace_printf_key(const char *key, const char *fmt, ...); +extern void trace_strbuf(const char *key, const struct strbuf *buf); + +#endif /* TRACE_H */ -- 2.0.0.406.g2e9ef9b -- 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 4/4] cache-tree: Write updated cache-tree after commit
On Fri, Jul 11, 2014 at 7:22 PM, David Turner wrote:
> 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
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 3a3342e..57f263f 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -90,37 +128,86 @@ test_expect_success 'test-scrap-cache-tree works' '
>
> test_expect_success 'second commit has cache-tree' '
> test_commit bar &&
> - test_shallow_cache_tree
> + test_cache_tree
> +'
> +
> +test_expect_success 'commit -i gives cache-tree' '
> + git checkout current &&
> + cat <<-\EOT >foo.c &&
> + int foo()
> + {
> + return 42;
> + }
> + int bar()
> + {
> + return 42;
> + }
> + EOT
> + git add foo.c
> + test_invalid_cache_tree
> + git commit -m "add a file"
> + test_cache_tree
Broken &&-chain on all four lines above.
> + cat <<-\EOT >foo.c &&
> + int foo()
> + {
> + return 43;
> + }
> + int bar()
> + {
> + return 44;
> + }
> + EOT
> + (echo p; echo 1; echo; echo s; echo n; echo y; echo q) | git commit
> --interactive -m foo
Broken &&-chain.
Would a printf make this more readable?
printf "p\n1\n\ns\nn\ny\nq\n" | git commt ... &&
Perhaps not.
> + test_cache_tree
> +'
> +
> +test_expect_success 'commit in child dir has cache-tree' '
> + mkdir dir &&
> + >dir/child.t &&
> + git add dir/child.t &&
> + git commit -m dir/child.t &&
> + test_cache_tree
> '
>
> test_expect_success 'reset --hard gives cache-tree' '
> test-scrap-cache-tree &&
> git reset --hard &&
> - test_shallow_cache_tree
> + test_cache_tree
> '
>
> test_expect_success 'reset --hard without index gives cache-tree' '
> rm -f .git/index &&
> git reset --hard &&
> - test_shallow_cache_tree
> + test_cache_tree
> '
>
> test_expect_success 'checkout gives cache-tree' '
> git tag current &&
> git checkout HEAD^ &&
> - test_shallow_cache_tree
> + test_cache_tree
> '
>
> test_expect_success 'checkout -b gives cache-tree' '
> git checkout current &&
> git checkout -b prev HEAD^ &&
> - test_shallow_cache_tree
> + test_cache_tree
> '
>
> test_expect_success 'checkout -B gives cache-tree' '
> git checkout current &&
> git checkout -B prev HEAD^ &&
> - test_shallow_cache_tree
> + test_cache_tree
> +'
> +
> +test_expect_success 'partial commit gives cache-tree' '
> + git checkout -b partial no-children &&
> + test_commit one &&
> + test_commit two &&
> + echo "some change" >one.t &&
> + git add one.t &&
> + echo "some other change" >two.t &&
> + git commit two.t -m partial &&
> + test_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 v8 00/17] add performance tracing facility
Changes since v7: [04]: Fixed -Wextra compiler warnings, thanks to Ramsay Jones. [11]: Added #ifndef TRACE_CONTEXT, explained why __FILE__ ":" __FUNCTION__ doesn't work. [17]: New Documentation/technical/api-trace.txt Karsten Blees (17): trace: move trace declarations from cache.h to new trace.h trace: consistently name the format parameter trace: remove redundant printf format attribute trace: improve trace performance Documentation/git.txt: improve documentation of 'GIT_TRACE*' variables sha1_file: change GIT_TRACE_PACK_ACCESS logging to use trace API trace: add infrastructure to augment trace output with additional info trace: disable additional trace output for unit tests trace: add current timestamp to all trace output trace: move code around, in preparation to file:line output trace: add 'file:line' to all trace output trace: add high resolution timer function to debug performance issues trace: add trace_performance facility to debug performance issues git: add performance tracing for git's main() function to debug scripts wt-status: simplify performance measurement by using getnanotime() progress: simplify performance measurement by using getnanotime() api-trace.txt: add trace API documentation Documentation/git.txt | 59 -- Documentation/technical/api-trace.txt | 97 + Makefile | 7 + builtin/receive-pack.c| 2 +- cache.h | 13 +- commit.h | 1 + config.mak.uname | 1 + git-compat-util.h | 4 + git.c | 2 + pkt-line.c| 8 +- progress.c| 71 +++ sha1_file.c | 30 +-- shallow.c | 10 +- t/test-lib.sh | 4 + trace.c | 369 -- trace.h | 113 +++ wt-status.c | 14 +- 17 files changed, 629 insertions(+), 176 deletions(-) create mode 100644 Documentation/technical/api-trace.txt create mode 100644 trace.h -- 2.0.0.406.g2e9ef9b -- 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 2/2] dir: remove PATH_MAX limitation
Am 12.07.2014 00:29, schrieb Junio C Hamano: > Karsten Blees writes: > >> Anyways, I'd like to kindly withdraw this patch in favor of Duy's version. >> >> http://article.gmane.org/gmane.comp.version-control.git/248310 > > Thanks; I've already reverted it from 'next'. > > Is Duy's patch still viable? > I think so. It fixes the segfault with long paths on Windows as well (Tested-by: ), uses strbuf APIs as Peff suggested, and initializes the strbuf with PATH_MAX (i.e. no reallocs in the common case either ;-) ). AFAICT the first two patches of that series are also completely unrelated to the untracked-cache, so we may want to fast-track these? [01/20] dir.c: coding style fix [02/20] dir.h: move struct exclude declaration to top level [03/20] prep_exclude: remove the artificial PATH_MAX limit ...perhaps with s/if (!dir->basebuf.alloc)/if (!dir->basebuf.buf)/ @Duy any reason for not signing off that series? -- 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 4/4 v6] cache-tree: Write updated cache-tree after commit
On Fri, 2014-07-11 at 08:52 -0700, Junio C Hamano wrote:
> David Turner writes:
>
> > @@ -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.
>
> Is this statement now stale and needs to be removed?
I think it is still accurate; we still don't bother to check SHAs and
test-dump-cache-tree still does the comparison.
--
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 v7 3/4] cache-tree: subdirectory tests
Add tests to confirm that invalidation of subdirectories neither over-
nor under-invalidates.
Signed-off-by: David Turner
---
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 v7 4/4] 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
---
builtin/commit.c | 9 +++-
t/t0090-cache-tree.sh | 117 +++---
2 files changed, 110 insertions(+), 16 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..57f263f 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/" filtered &&
test_cmp "$1" filtered
}
@@ -16,14 +16,38 @@ 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 () {
+ (
+ generate_expected_cache_tree_rec
+ )
+}
+
+test_cache_tree () {
+ generate_expected_cache_tree >expect &&
cmp_cache_tree expect
}
test_invalid_cache_tree () {
printf "invalid %s ()\n" "" "$@"
>expect &&
- test-dump-cache-tree | \
+ test-dump-cache-tree |
sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p'
>actual &&
test_cmp expect actual
}
@@ -33,14 +57,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 +82,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
[PATCH v7 2/4] 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
---
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 v7 1/4] 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
---
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] fixup! symlinks: remove PATH_MAX limitation
Rename cache_def_free to cache_def_clear as it doesn't free the struct
cache_def, but just clears its content.
Signed-off-by: Karsten Blees
---
cache.h | 2 +-
preload-index.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 44aa439..378ee7f 100644
--- a/cache.h
+++ b/cache.h
@@ -1096,7 +1096,7 @@ struct cache_def {
int prefix_len_stat_func;
};
#define CACHE_DEF_INIT { STRBUF_INIT, 0, 0, 0 }
-static inline void cache_def_free(struct cache_def *cache)
+static inline void cache_def_clear(struct cache_def *cache)
{
strbuf_release(&cache->path);
}
diff --git a/preload-index.c b/preload-index.c
index 79ce8a9..c1fe3a3 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -63,7 +63,7 @@ static void *preload_thread(void *_data)
continue;
ce_mark_uptodate(ce);
} while (--nr > 0);
- cache_def_free(&cache);
+ cache_def_clear(&cache);
return NULL;
}
--
2.0.1.563.g66f467c.dirty
--
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/3] 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
Signed-off-by: Jacob Keller
---
Made parse_sort_string take a "var" parameter, and if given will only warn
about invalid parameter, instead of error.
Documentation/config.txt | 5
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 66 ++-
t/t7004-tag.sh| 36 ++
4 files changed, 93 insertions(+), 19 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
+tag.sort::
+ This variable controls the sort ordering of tags when displayed by
+ linkgit:git-tag[1]. Without the "--sort=" option provided, the
+ value of this variable will be used as the default.
+
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..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
Sort in a specific order. Supported type is "refname"
(lexicographic order), "version:refname" or "v:refname" (tag
names are treated as versions). Prepend "-" to reverse sort
- order.
+ order. When this option is not given, the sort order defaults to the
+ value configured for the 'tag.sort' variable if it exists, or
+ lexicographic order otherwise. See linkgit:git-config[1].
--column[=]::
--no-column::
@@ -317,6 +319,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 9d7643f127e7..97c5317c28e5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
#define SORT_MASK 0x7fff
#define REVERSE_SORT0x8000
+static int tag_sort;
+
struct tag_filter {
const char **patterns;
int lines;
@@ -346,9 +348,49 @@ static const char tag_template_nocleanup[] =
"Lines starting with '%c' will be kept; you may remove them"
" yourself if you want to.\n");
+/*
+ * Parse a sort string, and return 0 if parsed successfully. Will return
+ * non-zero when the sort string does not parse into a known type.
+ */
+static int parse_sort_string(const char *var, const char *value, int *sort)
+{
+ int type = 0, flags = 0;
+
+ if (skip_prefix(value, "-", &value))
+ flags |= REVERSE_SORT;
+
+ if (skip_prefix(value, "version:", &value) || skip_prefix(value, "v:",
&value))
+ type = VERCMP_SORT;
+ else
+ type = STRCMP_SORT;
+
+ if (strcmp(value, "refname")) {
+ if (!var)
+ return error(_("unsupported sort specification '%s'"),
value);
+ else {
+ warning(_("unsupported sort specification '%s' in
variable '%s'"),
+ var, value);
+ return -1;
+ }
+ }
+
+ *sort = (type | flags);
+
+ return 0;
+}
+
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")) {
+ if (!value)
+ return config_error_nonbool(var);
+ parse_sort_string(var, value, &tag_sort);
+ return 0;
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, "column."))
@@ -522,20 +564,8 @@ 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;
-
- if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
- *sort = VERCMP_SORT;
- else
- *sort = STRCMP_SORT;
-
- if (strcmp(arg, "refname"))
- die(_("unsupported sort specification %s"), arg);
- *sort |= flags;
- return 0;
+ return parse_sort_string(NULL, arg, sort);
}
int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -548,7 +578,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int a
[PATCH 2/3] tag: fix --sort tests to use cat<<-\EOF format
The --sort tests should use the better format for >expect to maintain indenting and ensure that no substitution is occurring. This makes parsing and understanding the tests a bit easier. Signed-off-by: Jacob Keller --- t/t7004-tag.sh | 40 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index e4ab0f5b6419..66010b0e7066 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' ' git tag foo1.6 && git tag foo1.10 && git tag -l --sort=refname "foo*" >actual && - cat >expectactual && - cat >expect actual && - cat >expect actual && - cat >expect
[PATCH 1/3] tag: use skip_prefix instead of magic numbers
From: Jeff King
Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King
Signed-off-by: Jacob Keller
---
Fixed issue with patch in that we dropped the reset to STRCMP_SORT, discovered
by Junio.
builtin/tag.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..9d7643f127e7 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:")) {
+
+ 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
+ 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
Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers
On Fri, 2014-07-11 at 15:44 -0700, Junio C Hamano wrote:
> Jacob Keller writes:
>
> > From: Jeff King
> >
> > Make the parsing of the --sort parameter more readable by having
> > skip_prefix keep our pointer up to date.
> >
> > Signed-off-by: Jeff King
> > Signed-off-by: Jacob Keller
> > ---
> > 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;
> > +
>
> By losing "*sort = STRCMP_SORT", the version after this patch would
> stop clearing what is pointed by opt->value, so
>
> git tag --sort=v:refname --sort=refname
>
> would no longer implement the "last one wins" semantics, no?
>
> Am I misreading the patch? I somehow thought Peff's original was
> clearing the variable...
>
> > if (strcmp(arg, "refname"))
> > die(_("unsupported sort specification %s"), arg);
> > *sort |= flags;
Indeed. My patch fixes this up but I will re-work this so we don't
introduce an inbetween bug :)
Thanks,
Jake
Re: [PATCH 3/4 v6] cache-tree: subdirectory tests
On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > >>> + sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p' > >>> >actual && > > > > Is the second one to remove "#(ref)", which appears for a good > > "reference" cache tree entry shown for comparison, necessary? Do > > they ever begin with "invalid"? If they ever begin with "invalid" > > that itself may even be a noteworthy breakage to catch, no? > > Answering to myself... > > Because test-dump-cache-tree uses DRY_RUN to create only an in-core > copy of tree object, and we notice that the reference cache-tree > created in the tests contains the object name of a tree that does > not yet exist in the object database. We get "invalid #(ref)" for > such node. > > In the ideal world, I think whoever tries to compare two cache-trees > (i.e. test-dump-cache-tree) should *not* care, because we are merely > trying to show what the correct tree object name for the node would > be, but this is only for testing, so the best way forward would be > to: > > - Stop using DRY_RUN in test-dump-cache-tree.c; > > - Stop the code to support DRY_RUN from cache-tree.c (nobody but >the test uses it); and > > - Drop the "-e '#(ref)/d'" from the above. > > I would think. Do you mean that I should do this in this patch set, or that it's a good idea for the future? Also, if we don't use DRY_RUN, won't test-dump-cache-tree add trees to the actual ODB, which would be odd for a test program? -- 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 3/4 v6] cache-tree: subdirectory tests
On Fri, 2014-07-11 at 08:27 -0700, Junio C Hamano wrote:
> Eric Sunshine writes:
>
> > On Thu, Jul 10, 2014 at 8:31 PM, David Turner
> > wrote:
> >> Add tests to confirm that invalidation of subdirectories neither over-
> >> nor under-invalidates.
> >>
> >> Signed-off-by: David Turner
> >> ---
> >> 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 &&
>
> Hmm. This will always expect that the top-level is invalid, even
> when $# is 0. It is OK if you never need to use this to test that a
> cache-tree is fully valid, but is it something we want to check?
We have test_cache_tree to check that it's fully valid.
> Existing tests are mostly about "cache-tree is populated fully at a
> few strategic, well known and easy places and then it degrades over
> time", but after all your series is adding more places to that set
> of "a few places", so we may want to make sure that future breakages
> to the new code paths that "repair" the cache-tree are caught by
> these tests.
This patchset un-failed "initial commit has cache-tree", and added
"commit in child dir has cache-tree" and "partial commit gives
cache-tree". I've just added a test for interactive commit; when you
take a look at the next patchset, you can let me know if this seems
sufficient to you.
--
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 1/3] tag: use skip_prefix instead of magic numbers
Jacob Keller writes:
> From: Jeff King
>
> Make the parsing of the --sort parameter more readable by having
> skip_prefix keep our pointer up to date.
>
> Signed-off-by: Jeff King
> Signed-off-by: Jacob Keller
> ---
> 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;
> +
By losing "*sort = STRCMP_SORT", the version after this patch would
stop clearing what is pointed by opt->value, so
git tag --sort=v:refname --sort=refname
would no longer implement the "last one wins" semantics, no?
Am I misreading the patch? I somehow thought Peff's original was
clearing the variable...
> 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: [PATCH 3/3] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 15:17 -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
> >
> >> + if (!strcmp(var, "tag.sort")) {
> >> + if (!value)
> >> + return config_error_nonbool(var);
> >> + status = parse_sort_string(value, &tag_sort);
> >> + if (status) {
> >> + warning("using default lexicographic sort order");
> >> + tag_sort = STRCMP_SORT;
> >> + }
> >
> > I think this is a good compromise of the issues we discussed earlier. As
> > you noted, it should probably be marked for translation. I'm also not
> > sure the message content is clear in all situations. If I have a broken
> > tag.sort, I get:
> >
> > $ git config tag.sort bogus
> > $ git tag >/dev/null
> > error: unsupported sort specification bogus
> > warning: using default lexicographic sort order
> >
> > Not too bad, though reminding me that the value "bogus" came from
> > tag.sort would be nice. But what if I override it:
> >
> > $ git tag --sort=v:refname >/dev/null
> > error: unsupported sort specification bogus
> > warning: using default lexicographic sort order
> >
> > That's actively wrong, because we are using v:refname from the
> > command-line. Perhaps we could phrase it like:
> >
> > warning: ignoring invalid config option tag.sort
> >
> > or similar, which helps both cases.
>
> Hmph. Looks like a mild-enough middle ground, I guess. As
> parse_sort_string() is private for "git tag" implementation, I
> actually would not mind if we taught a bit more context to it and
> phrase its messages differently. Perhaps something like this, where
> the config parser will tell what variable it came from with "var"
> and the command line parser will pass NULL there.
>
> static int parse_sort_string(const char *var, const char *value, int *sort)
> {
> ...
> if (strcmp(value, "refname")) {
> if (!var)
> return error(_("unsupported sort specification '%s'"),
> value);
> else {
> warning(_("unsupported sort specification '%s' in
> variable '%s'"),
> var, value);
> return -1;
> }
> }
>
> *sort = (type | flags);
>
> return 0;
> }
>
Ok.. I suppose that could be done.
Thanks,
Jake
> > As an aside, I also think the error line could more clearly mark the
> > literal contents of the variable. E.g., one of:
> >
> > error: unsupported sort specification: bogus
> >
> > or
> >
> > error: unsupported sort specification 'bogus'
>
> Yup.
Re: [PATCH 2/2] dir: remove PATH_MAX limitation
Karsten Blees writes: > Anyways, I'd like to kindly withdraw this patch in favor of Duy's version. > > http://article.gmane.org/gmane.comp.version-control.git/248310 Thanks; I've already reverted it from 'next'. Is Duy's patch still viable? -- 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] http: Add Accept-Language header if possible
On Fri, Jul 11, 2014 at 1:35 PM, Jeff King wrote:
> On Sat, Jul 12, 2014 at 01:52:53AM +0900, Yi EungJun wrote:
>> Add an Accept-Language header which indicates the user's preferred
>> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
>>
>> Examples:
>> LANGUAGE= -> ""
>> LANGUAGE=ko:en -> "Accept-Language: ko, en; q=0.9, *; q=0.1"
>> LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *; q=0.1"
>> LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *; q=0.1"
>>
>> This gives git servers a chance to display remote error messages in
>> the user's preferred language.
>
> Thanks, this is looking much nicer. Most of my comments are on style:
>
>> +static const char* get_preferred_languages() {
>> +const char* retval;
>
> A few style nits:
Also, this is C, not C++, so don't forget void:
static const char *get_preferred_languages(void)
{
> 1. We usually put a function's opening brace on a new line.
>
> 2. We usually put the asterisk in a pointer declaration with the
> variable name ("const char *retval"). This one appears elsewhere in
> the patch.
>
> 3. This line seems to be indented with spaces instead of tabs.
--
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 v1 3/4] hashmap: add simplified hashmap_get_from_hash() API
Karsten Blees writes: >> In other words, why isn't hashmap_get() more like this: >> ... >> with hashmap_entry_init() purely a static helper in hashmap.c? >> > 1. Performance OK. > 2. Simplicity > > Hashmap clients will typically provide small, type safe wrappers around the > hashmap API. OK. > 3. Compound keys > > The key doesn't always consist of just a single word. E.g. for struct > dir_entry, the key is [char *name, int len]. So an API like this: > > void *hashmap_get(const struct hashmap *map, const void *key) > > won't do in the general case (unless you require clients to define their > own key structure in addition to the entry structure...). Yeah, I was being silly. 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 1/2] symlinks: remove PATH_MAX limitation
Karsten Blees writes:
> Am 07.07.2014 20:30, schrieb Junio C Hamano:
>> Karsten Blees writes:
>>
>> The above cache_def_free(cache) does not free the cache itself, but
>> only its associated data, so the name cache_def_free() is somewhat
>> misleading.
>>
>
> You already merged this to master ("kb/path-max-must-go" lol), should
> I send a fixup! s/cache_def_free/cache_def_clear/ or is it OK as is?
I do not think a fix-up would hurt other topics in flight, so
please do so.
--
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 3/3] tag: support configuring --sort via .gitconfig
Jeff King writes:
> On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
>
>> +if (!strcmp(var, "tag.sort")) {
>> +if (!value)
>> +return config_error_nonbool(var);
>> +status = parse_sort_string(value, &tag_sort);
>> +if (status) {
>> +warning("using default lexicographic sort order");
>> +tag_sort = STRCMP_SORT;
>> +}
>
> I think this is a good compromise of the issues we discussed earlier. As
> you noted, it should probably be marked for translation. I'm also not
> sure the message content is clear in all situations. If I have a broken
> tag.sort, I get:
>
> $ git config tag.sort bogus
> $ git tag >/dev/null
> error: unsupported sort specification bogus
> warning: using default lexicographic sort order
>
> Not too bad, though reminding me that the value "bogus" came from
> tag.sort would be nice. But what if I override it:
>
> $ git tag --sort=v:refname >/dev/null
> error: unsupported sort specification bogus
> warning: using default lexicographic sort order
>
> That's actively wrong, because we are using v:refname from the
> command-line. Perhaps we could phrase it like:
>
> warning: ignoring invalid config option tag.sort
>
> or similar, which helps both cases.
Hmph. Looks like a mild-enough middle ground, I guess. As
parse_sort_string() is private for "git tag" implementation, I
actually would not mind if we taught a bit more context to it and
phrase its messages differently. Perhaps something like this, where
the config parser will tell what variable it came from with "var"
and the command line parser will pass NULL there.
static int parse_sort_string(const char *var, const char *value, int *sort)
{
...
if (strcmp(value, "refname")) {
if (!var)
return error(_("unsupported sort specification '%s'"),
value);
else {
warning(_("unsupported sort specification '%s' in
variable '%s'"),
var, value);
return -1;
}
}
*sort = (type | flags);
return 0;
}
> As an aside, I also think the error line could more clearly mark the
> literal contents of the variable. E.g., one of:
>
> error: unsupported sort specification: bogus
>
> or
>
> error: unsupported sort specification 'bogus'
Yup.
--
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 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 14:54 -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
> >
> >> Updated to include changes due to Junio's feedback. This has not resolved
> >> whether we should fail on a configuration error or simply warn. It appears
> >> that
> >> we actually seem to error out more than warn, so I am unsure what the
> >> correct
> >> action is here.
> >
> > Yeah, we're quite inconsistent there. In some cases we silently ignore
> > something unknown (e.g., a color.diff.* slot that we do not understand),
> > but in most cases if it is a config key we understand but a value we do
> > not, we complain and die.
>
> Hm, that's bad---we've become less and less careful over time,
> perhaps?
>
> As we want to be able to enhance semantics of existing configuration
> variables without having to introduce new but similar ones, we would
> really want to make sure that those who share the same .git/config
> or $HOME/.gitconfig across different versions of Git would not have
> to suffer too much (i.e. forcing them to "config --unset" when using
> their older Git is not nice).
>
> > It's probably user-unfriendly to be silent for those cases, though. The
> > user gets no feedback on why their config value is doing nothing.
> >
> > I tend to think that warning is not much better than erroring out. It is
> > helpful if you are running a single-shot of an old version (which is
> > something that I do a lot when testing old versions), but would quickly
> > become irritating if you were actually using an old version of git
> > day-to-day.
> >
> > I dunno. Maybe it is worth making life easier for people in the former
> > category.
>
> ... "former cat" meaning "less irritating for single-shot use"? I
> dunno...
>
> >> +static int parse_sort_string(const char *arg, int *sort)
> >> +{
> >> + int type = 0, flags = 0;
> >> +
> >> + if (skip_prefix(arg, "-", &arg))
> >> + flags |= REVERSE_SORT;
> >> +
> >> + if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> >> + type = VERCMP_SORT;
> >> + else
> >> + type = STRCMP_SORT;
> >> +
> >> + if (strcmp(arg, "refname"))
> >> + return error(_("unsupported sort specification %s"), arg);
> >> +
> >> + *sort = (type | flags);
> >> +
> >> + return 0;
> >> +}
> >
> > Regardless of how we handle the error, I think this version that
> > assembles the final bitfield at the end is easier to read than the
> > original.
>
> Yes, this part really is nicely done, I agree.
The current revision of the patch prints an error and warning about not
using the configured tag value. It does still run. I added a test to
ensure this as well.
The easiest way to change overall behavior is to change the git-config's
"die_on_error" to be false, so that we no longer die on configuration
errors.
It appears this would resolve the issue for many configuration options
(not all, as I think a few are still hard coded to die) but it would be
a change that is well outside the scope of this patch.
I think that for now behavior I added is good, as we *know* that
tag.sort will add new parameters in the near future, so it makes no
sense to have it die on a config that is only slightly newer than the
git version.
Glad I could help. Junio if you could review the v7 I sent a bit ago for
any possible mistakes that I forgot to clean up that would be great.
Thanks,
Jake
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
git-fast-import bug?
git-fast-import is not writing a commit even after a checkpoint/progress command. See my previous message "git p4 diff-tree ambiguous argument error". The error in git-p4 is caused by git not writing the commit even after git-fast-import has been given a checkpoint and progress command. On initial use of git p4 to sync a p4 repository, the commits are written properly. But on a subsequent run the commit is not flushed to the file system during the run. Specifically, I can stop the git-p4 command directly after the progress checkpoint command (see the checkpoint() function in git-p4). The file is not found. If I abort/exit the application at that point, the file appears. There is a pattern of behavior here that is consistent but I am unable to understand. A bare repository works fine. An already populated repository does not work until the app is quit. What would cause git-fast-import to _NOT_ flush the file? This certainly seems like a bug. But I don't know enough of the git internals to reproduce. Suggestions on how to test or isolate this problem? Thanks Reproduced consistently on two systems: $ git --version git version 1.8.5.2 (Apple Git-48) $ python --version Python 2.7.5 $ uname -a Darwin Kernel Version 13.3.0: Tue Jun 3 21:27:35 PDT 2014; root:xnu-2422.110.17~1/RELEASE_X86_64 x86_64 and $ git --version git version 1.7.12.4 $ python --version Python 2.6.6 OS: GNU/Linux 2.6.32-431.el6.x86_64 -- 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 3/3 v5] tag: support configuring --sort via .gitconfig
Jeff King writes:
> On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
>
>> Updated to include changes due to Junio's feedback. This has not resolved
>> whether we should fail on a configuration error or simply warn. It appears
>> that
>> we actually seem to error out more than warn, so I am unsure what the correct
>> action is here.
>
> Yeah, we're quite inconsistent there. In some cases we silently ignore
> something unknown (e.g., a color.diff.* slot that we do not understand),
> but in most cases if it is a config key we understand but a value we do
> not, we complain and die.
Hm, that's bad---we've become less and less careful over time,
perhaps?
As we want to be able to enhance semantics of existing configuration
variables without having to introduce new but similar ones, we would
really want to make sure that those who share the same .git/config
or $HOME/.gitconfig across different versions of Git would not have
to suffer too much (i.e. forcing them to "config --unset" when using
their older Git is not nice).
> It's probably user-unfriendly to be silent for those cases, though. The
> user gets no feedback on why their config value is doing nothing.
>
> I tend to think that warning is not much better than erroring out. It is
> helpful if you are running a single-shot of an old version (which is
> something that I do a lot when testing old versions), but would quickly
> become irritating if you were actually using an old version of git
> day-to-day.
>
> I dunno. Maybe it is worth making life easier for people in the former
> category.
... "former cat" meaning "less irritating for single-shot use"? I
dunno...
>> +static int parse_sort_string(const char *arg, int *sort)
>> +{
>> +int type = 0, flags = 0;
>> +
>> +if (skip_prefix(arg, "-", &arg))
>> +flags |= REVERSE_SORT;
>> +
>> +if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
>> +type = VERCMP_SORT;
>> +else
>> +type = STRCMP_SORT;
>> +
>> +if (strcmp(arg, "refname"))
>> +return error(_("unsupported sort specification %s"), arg);
>> +
>> +*sort = (type | flags);
>> +
>> +return 0;
>> +}
>
> Regardless of how we handle the error, I think this version that
> assembles the final bitfield at the end is easier to read than the
> original.
Yes, this part really is nicely done, I agree.
--
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/3 v7] 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
Signed-off-by: Jacob Keller
---
Updated warning texts based on Jeff's feedback. Also added translate specifier
to the warning string.
Documentation/config.txt | 5
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 61 ++-
t/t7004-tag.sh| 36
4 files changed, 90 insertions(+), 17 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
+tag.sort::
+ This variable controls the sort ordering of tags when displayed by
+ linkgit:git-tag[1]. Without the "--sort=" option provided, the
+ value of this variable will be used as the default.
+
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..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
Sort in a specific order. Supported type is "refname"
(lexicographic order), "version:refname" or "v:refname" (tag
names are treated as versions). Prepend "-" to reverse sort
- order.
+ order. When this option is not given, the sort order defaults to the
+ value configured for the 'tag.sort' variable if it exists, or
+ lexicographic order otherwise. See linkgit:git-config[1].
--column[=]::
--no-column::
@@ -317,6 +319,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..6e0a8ed4d1f9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
#define SORT_MASK 0x7fff
#define REVERSE_SORT0x8000
+static int tag_sort;
+
struct tag_filter {
const char **patterns;
int lines;
@@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =
"Lines starting with '%c' will be kept; you may remove them"
" yourself if you want to.\n");
+/*
+ * Parse a sort string, and return 0 if parsed successfully. Will return
+ * non-zero when the sort string does not parse into a known type.
+ */
+static int parse_sort_string(const char *arg, int *sort)
+{
+ int type = 0, flags = 0;
+
+ if (skip_prefix(arg, "-", &arg))
+ flags |= REVERSE_SORT;
+
+ if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
+ type = VERCMP_SORT;
+ else
+ type = STRCMP_SORT;
+
+ if (strcmp(arg, "refname"))
+ return error(_("unsupported sort specification '%s'"), arg);
+
+ *sort = (type | flags);
+
+ return 0;
+}
+
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")) {
+ if (!value)
+ return config_error_nonbool(var);
+ status = parse_sort_string(value, &tag_sort);
+ if (status) {
+ warning(_("ignoring configured sort specification from
'tag.sort'"));
+ tag_sort = STRCMP_SORT;
+ }
+ return 0;
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, "column."))
@@ -522,18 +561,8 @@ 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;
-
- 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;
+ return parse_sort_string(arg, sort);
}
int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -546,7 +575,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 m
[PATCH 2/3] tag: fix --sort tests to use cat<<-\EOF format
The --sort tests should use the better format for >expect to maintain indenting and ensure that no substitution is occurring. This makes parsing and understanding the tests a bit easier. Signed-off-by: Jacob Keller --- t/t7004-tag.sh | 40 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index e4ab0f5b6419..66010b0e7066 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' ' git tag foo1.6 && git tag foo1.10 && git tag -l --sort=refname "foo*" >actual && - cat >expectactual && - cat >expect actual && - cat >expect actual && - cat >expect
[PATCH 1/3] tag: use skip_prefix instead of magic numbers
From: Jeff King
Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King
Signed-off-by: Jacob Keller
---
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
Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 17:06 -0400, Jeff King wrote:
> On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
>
> > + if (!strcmp(var, "tag.sort")) {
> > + if (!value)
> > + return config_error_nonbool(var);
> > + status = parse_sort_string(value, &tag_sort);
> > + if (status) {
> > + warning("using default lexicographic sort order");
> > + tag_sort = STRCMP_SORT;
> > + }
>
> I think this is a good compromise of the issues we discussed earlier. As
> you noted, it should probably be marked for translation. I'm also not
> sure the message content is clear in all situations. If I have a broken
> tag.sort, I get:
>
> $ git config tag.sort bogus
> $ git tag >/dev/null
> error: unsupported sort specification bogus
> warning: using default lexicographic sort order
>
> Not too bad, though reminding me that the value "bogus" came from
> tag.sort would be nice. But what if I override it:
>
> $ git tag --sort=v:refname >/dev/null
> error: unsupported sort specification bogus
> warning: using default lexicographic sort order
>
> That's actively wrong, because we are using v:refname from the
> command-line. Perhaps we could phrase it like:
>
> warning: ignoring invalid config option tag.sort
>
ok that makes more sense. I can't really avoid printing the warning at
all since config parsing happens before the option parsing.
I like this wording. I will respin again :D
Thanks,
Jake
> or similar, which helps both cases.
>
> As an aside, I also think the error line could more clearly mark the
> literal contents of the variable. E.g., one of:
>
> error: unsupported sort specification: bogus
>
> or
>
> error: unsupported sort specification 'bogus'
>
> -Peff
Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig
On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
> + if (!strcmp(var, "tag.sort")) {
> + if (!value)
> + return config_error_nonbool(var);
> + status = parse_sort_string(value, &tag_sort);
> + if (status) {
> + warning("using default lexicographic sort order");
> + tag_sort = STRCMP_SORT;
> + }
I think this is a good compromise of the issues we discussed earlier. As
you noted, it should probably be marked for translation. I'm also not
sure the message content is clear in all situations. If I have a broken
tag.sort, I get:
$ git config tag.sort bogus
$ git tag >/dev/null
error: unsupported sort specification bogus
warning: using default lexicographic sort order
Not too bad, though reminding me that the value "bogus" came from
tag.sort would be nice. But what if I override it:
$ git tag --sort=v:refname >/dev/null
error: unsupported sort specification bogus
warning: using default lexicographic sort order
That's actively wrong, because we are using v:refname from the
command-line. Perhaps we could phrase it like:
warning: ignoring invalid config option tag.sort
or similar, which helps both cases.
As an aside, I also think the error line could more clearly mark the
literal contents of the variable. E.g., one of:
error: unsupported sort specification: bogus
or
error: unsupported sort specification 'bogus'
-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 3/3] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 13:51 -0700, Jacob Keller wrote:
> 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
> Signed-off-by: Jacob Keller
> ---
> Updated based on Junio's suggestions, as well as making sure that we don't
> bail
> if we can't understand config's option. We still print error message followed
> by a warning about using default order. In addition, added a few more tests to
> ensure that these work as expected.
>
> Documentation/config.txt | 5
> Documentation/git-tag.txt | 5 +++-
> builtin/tag.c | 61
> ++-
> t/t7004-tag.sh| 36
> 4 files changed, 90 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1d718bdb9662..c55c22ab7be9 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2354,6 +2354,11 @@ submodule..ignore::
> "--ignore-submodules" option. The 'git submodule' commands are not
> affected by this setting.
>
> +tag.sort::
> + This variable controls the sort ordering of tags when displayed by
> + linkgit:git-tag[1]. Without the "--sort=" option provided, the
> + value of this variable will be used as the default.
> +
> 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..320908369f06 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -99,7 +99,9 @@ OPTIONS
> Sort in a specific order. Supported type is "refname"
> (lexicographic order), "version:refname" or "v:refname" (tag
> names are treated as versions). Prepend "-" to reverse sort
> - order.
> + order. When this option is not given, the sort order defaults to the
> + value configured for the 'tag.sort' variable if it exists, or
> + lexicographic order otherwise. See linkgit:git-config[1].
>
> --column[=]::
> --no-column::
> @@ -317,6 +319,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..cb37b26159a6 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
> #define SORT_MASK 0x7fff
> #define REVERSE_SORT0x8000
>
> +static int tag_sort;
> +
> struct tag_filter {
> const char **patterns;
> int lines;
> @@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =
> "Lines starting with '%c' will be kept; you may remove them"
> " yourself if you want to.\n");
>
> +/*
> + * Parse a sort string, and return 0 if parsed successfully. Will return
> + * non-zero when the sort string does not parse into a known type.
> + */
> +static int parse_sort_string(const char *arg, int *sort)
> +{
> + int type = 0, flags = 0;
> +
> + if (skip_prefix(arg, "-", &arg))
> + flags |= REVERSE_SORT;
> +
> + if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> + type = VERCMP_SORT;
> + else
> + type = STRCMP_SORT;
> +
> + if (strcmp(arg, "refname"))
> + return error(_("unsupported sort specification %s"), arg);
> +
> + *sort = (type | flags);
> +
> + return 0;
> +}
> +
> 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")) {
> + if (!value)
> + return config_error_nonbool(var);
> + status = parse_sort_string(value, &tag_sort);
> + if (status) {
> + warning("using default lexicographic sort order");
Oops, I should also use warning(_("")) here as well I believe...
Sorry for thrash.
Thanks,
Jake
> + tag_sort = STRCMP_SORT;
> + }
> + return 0;
> + }
> +
> + status = git_gpg_config(var, value, cb);
> if (status)
> return status;
> if (starts_with(var, "column."))
> @@ -522,18 +561,8 @@ 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;
> -
> - 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 |= fl
[PATCH 1/3] tag: use skip_prefix instead of magic numbers
From: Jeff King
Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King
Signed-off-by: Jacob Keller
---
Fixed authorship. I don't expect this version to be taken, but it helps me in
review, and I figured it is good to send the whole series.
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 3/3] 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
Signed-off-by: Jacob Keller
---
Updated based on Junio's suggestions, as well as making sure that we don't bail
if we can't understand config's option. We still print error message followed
by a warning about using default order. In addition, added a few more tests to
ensure that these work as expected.
Documentation/config.txt | 5
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 61 ++-
t/t7004-tag.sh| 36
4 files changed, 90 insertions(+), 17 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
+tag.sort::
+ This variable controls the sort ordering of tags when displayed by
+ linkgit:git-tag[1]. Without the "--sort=" option provided, the
+ value of this variable will be used as the default.
+
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..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
Sort in a specific order. Supported type is "refname"
(lexicographic order), "version:refname" or "v:refname" (tag
names are treated as versions). Prepend "-" to reverse sort
- order.
+ order. When this option is not given, the sort order defaults to the
+ value configured for the 'tag.sort' variable if it exists, or
+ lexicographic order otherwise. See linkgit:git-config[1].
--column[=]::
--no-column::
@@ -317,6 +319,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..cb37b26159a6 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
#define SORT_MASK 0x7fff
#define REVERSE_SORT0x8000
+static int tag_sort;
+
struct tag_filter {
const char **patterns;
int lines;
@@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =
"Lines starting with '%c' will be kept; you may remove them"
" yourself if you want to.\n");
+/*
+ * Parse a sort string, and return 0 if parsed successfully. Will return
+ * non-zero when the sort string does not parse into a known type.
+ */
+static int parse_sort_string(const char *arg, int *sort)
+{
+ int type = 0, flags = 0;
+
+ if (skip_prefix(arg, "-", &arg))
+ flags |= REVERSE_SORT;
+
+ if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
+ type = VERCMP_SORT;
+ else
+ type = STRCMP_SORT;
+
+ if (strcmp(arg, "refname"))
+ return error(_("unsupported sort specification %s"), arg);
+
+ *sort = (type | flags);
+
+ return 0;
+}
+
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")) {
+ if (!value)
+ return config_error_nonbool(var);
+ status = parse_sort_string(value, &tag_sort);
+ if (status) {
+ warning("using default lexicographic sort order");
+ tag_sort = STRCMP_SORT;
+ }
+ return 0;
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, "column."))
@@ -522,18 +561,8 @@ 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;
-
- 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;
+ return parse_sort_string(arg, sort);
}
int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -546,7 +575,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
[PATCH 2/3] tag: fix --sort tests to use cat<<-\EOF format
The --sort tests should use the better format for >expect to maintain indenting and ensure that no substitution is occurring. This makes parsing and understanding the tests a bit easier. Signed-off-by: Jacob Keller --- t/t7004-tag.sh | 40 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index e4ab0f5b6419..66010b0e7066 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' ' git tag foo1.6 && git tag foo1.10 && git tag -l --sort=refname "foo*" >actual && - cat >expectactual && - cat >expect actual && - cat >expect actual && - cat >expect
Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 11:29 -0700, Junio C Hamano wrote: > "Keller, Jacob E" writes: > > > This is not how the rest of the current tests work. I will submit a > > patch which fixes up the current --sort tests (but not every test, for > > now) as well. > > I do not want to pile more work that is unrelated to the task at > hand on your plate, i.e. clean-up work, so I would assign a very low > priority to "fix up the current tests". At the same time, existing > mistakes are not excuses to introduce new similar ones, hence my > suggestions to the added code in the patch I saw. > It was trivial to fix at least the --sort tests, so I submitted a patch that goes before this one to fix those as well. Thanks, Jake
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 14:22 -0400, Jeff King wrote: > On Fri, Jul 11, 2014 at 06:11:08PM +, Keller, Jacob E wrote: > > > I personally prefer error out on options, even though it can make it a > > bit more difficult, though as far as I know unknown fields simply warn > > or are ignored. (ie: old versions of git just ignore unknown fields in > > configuration). > > Right, we _have_ to ignore unknown config options, because we > specifically allow other programs built on git to store their config > with us (and anyway, our callback style of parsing means that no single > callback knows about all of the keys). > > In the past we have staked out particular areas of the namespace, > though. E.g., the diff code said "I own all of color.diff.*, and if you > put in something I don't understand, I'll complain". That ended up being > annoying, and now we ignore slots we don't understand there. > > So old gits will always silently ignore tag.sort if they don't know > about it, and we can't change that. The only thing we can change is: > > > It's possible we should warn instead though, so that older gits work > > with new sorts that they don't understand. > > Right. I think other config variables in similar situations will barf. > This is backwards-compatible as long as the new variables are a superset > (i.e., we only add new understood values, never remove or change the > meaning of existing values). It's just not forwards-compatible. > So should I respin this so that config option doesn't error out? > > I am ok with warning but I don't know the best practice for how to warn > > here instead of failing. Returning error causes a fatal "bad config" > > message. Any thoughts? > > The simplest thing is ignoring the return from parse_sort_string and > just calling "return 0". That will still say "error:", but continue on. > If you really want it to say "warning:", I think you'll have to pass a > flag into parse_sort_string. I'm not sure if it's worth the effort. > > -Peff Ok this makes sense, I am fine leaving it as error. Should I respin to make it not die though? Thanks, Jake
Re: Topic sk/mingw-unicode-spawn-args breaks tests
Am 10.07.2014 22:05, schrieb Johannes Sixt: > 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 > After commenting out fchmod in config.c, I get similar results. At first glance, t0050 seems to fail because the unicode file name patches are still missing. t4041 tries to pass ISO-8859-1 encoded bytes on the command line, which simply doesn't work on Windows (all OS APIs 'talk' UTF-16). We have a fix for this in the msysgit fork [1] (but unfortunately in another branch, so Stepan couldn't know the patch is related). I suspect the other failures also fall in these two categories. [1] https://github.com/msysgit/git/commit/ef4a733c -- 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 2/2] dir: remove PATH_MAX limitation
Am 09.07.2014 18:33, schrieb Junio C Hamano: > Karsten Blees writes: > >> 'git status' segfaults if a directory is longer than PATH_MAX, because >> processing .gitignore files in prep_exclude() writes past the end of a >> PATH_MAX-bounded buffer. >> >> Remove the limitation by using strbuf instead. >> >> Note: this fix just 'abuses' strbuf as string allocator, len is always 0. >> prep_exclude() can probably be simplified using more strbuf APIs. > > In addition to what Jeff already said, I am afraid that this may > make things a lot worse for normal case. By sizing the strbuf to > absolute minimum as you dig deeper at each level, wouldn't you copy > and reallocate the buffer a lot more, compared to the case where > everything fits in the original buffer? > strbuf uses ALLOC_GROW, which resizes in (x+16)*1.5 steps (i.e. 24, 60, 114, 195, 316...). Max path len in git is 85, and linux and WebKit are 170ish. I don't think three or four extra reallocs per directory traversal will be noticeable. Anyways, I'd like to kindly withdraw this patch in favor of Duy's version. http://article.gmane.org/gmane.comp.version-control.git/248310 -- 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 1/2] symlinks: remove PATH_MAX limitation
Am 07.07.2014 20:30, schrieb Junio C Hamano:
> Karsten Blees writes:
>
> The above cache_def_free(cache) does not free the cache itself, but
> only its associated data, so the name cache_def_free() is somewhat
> misleading.
>
You already merged this to master ("kb/path-max-must-go" lol), should
I send a fixup! s/cache_def_free/cache_def_clear/ or is it OK as is?
--
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 v1 3/4] hashmap: add simplified hashmap_get_from_hash() API
Am 07.07.2014 19:43, schrieb Junio C Hamano:
> Karsten Blees writes:
>
>> Hashmap entries are typically looked up by just a key. The hashmap_get()
>> API expects an initialized entry structure instead, to support compound
>> keys. This flexibility is currently only needed by find_dir_entry() in
>> name-hash.c (and compat/win32/fscache.c in the msysgit fork). All other
>> (currently five) call sites of hashmap_get() have to set up a near emtpy
>
> s/emtpy/empty/;
>
>> entry structure, resulting in duplicate code like this:
>>
>> struct hashmap_entry keyentry;
>> hashmap_entry_init(&keyentry, hash(key));
>> return hashmap_get(map, &keyentry, key);
>>
>> Add a hashmap_get_from_hash() API that allows hashmap lookups by just
>> specifying the key and its hash code, i.e.:
>>
>> return hashmap_get_from_hash(map, hash(key), key);
>
> While I think the *_get_from_hash() is an improvement over the
> three-line sequence, I find it somewhat strange that callers of it
> still must compute the hash code themselves, instead of letting the
> hashmap itself to apply the user supplied hash function to the key
> to derive it. After all, the map must know how to compare two
> entries via a user-supplied cmpfn given at the map initialization
> time, and the algorithm to derive the hash code falls into the same
> category, in the sense that both must stay the same during the
> lifetime of a hashmap, no? Unless there is a situation where you
> need to be able to initialize a hashmap_entry without knowing which
> hashmap the entry will eventually be stored, it feels dubious API
> that exposes to the outside callers hashmap_entry_init() that takes
> the hash code without taking the hashmap itself.
>
> In other words, why isn't hashmap_get() more like this:
>
> void *hashmap_get(const struct hashmap *map, const void *key)
> {
> struct hashmap_entry keyentry;
> hashmap_entry_init(&keyentry, map->hash(key));
> return *find_entry_ptr(map, &keyentry, key);
> }
>
> with hashmap_entry_init() purely a static helper in hashmap.c?
>
1. Performance
Calculating hash codes is the most expensive operation when working with
hash tables (unless you already have a good hash such as sha1). And using
hash tables as a cache of some sort is probably the most common use case.
So you'll often have code like this:
1 unsigned int h = hash(key);
2 struct my_entry *e = hashmap_get_from_hash(&map, hash, key);
3 if (!e) {
4e = malloc(sizeof(*e));
5hashmap_entry_init(e, h);
6e->key = key;
6hashmap_add(&map, e);
7 }
Note that the hash code from line 1 can be reused in line 5. You cannot do
that if calculating the hash code is buried in the implementation.
Callbacks cannot be inlined either...
2. Simplicity
Most APIs take a user defined hashmap_entry structure, so you'd either need
two hash functions, or a hash function that can distinguish between the
'entry' and 'key-only' case, e.g.
unsigned int my_hash(const struct my_entry *entry, const void *keydata)
{
if (keydata)
return strhash(keydata);
else
return strhash(entry->key);
}
Hashmap clients will typically provide small, type safe wrappers around the
hashmap API. That's perfect for calculating the hash code without breaking
encapsulation. IMO there's no need to make things more complex by requiring
another callback.
3. Compound keys
The key doesn't always consist of just a single word. E.g. for struct
dir_entry, the key is [char *name, int len]. So an API like this:
void *hashmap_get(const struct hashmap *map, const void *key)
won't do in the general case (unless you require clients to define their
own key structure in addition to the entry structure...).
--
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 2/2] dir: remove PATH_MAX limitation
Am 05.07.2014 12:48, schrieb Duy Nguyen: > On Sat, Jul 5, 2014 at 5:42 AM, Karsten Blees wrote: >> 'git status' segfaults if a directory is longer than PATH_MAX, because >> processing .gitignore files in prep_exclude() writes past the end of a >> PATH_MAX-bounded buffer. >> >> Remove the limitation by using strbuf instead. >> >> Note: this fix just 'abuses' strbuf as string allocator, len is always 0. >> prep_exclude() can probably be simplified using more strbuf APIs. > > FYI I had a similar patch [1] that attempted to lazily strbuf_init() > instead so that strbuf_ API could be used. > > [1] http://article.gmane.org/gmane.comp.version-control.git/248310 > Sorry, I missed that one. In my version, strbuf_grow() does the lazy initialization (in fact, many strbuf_* functions can handle memset(0) strbufs just fine). I was simply too lazy to understand (again) how prep_exclude works exactly, and as string calculations like that have the tendency to be just 1 char off, I went for the obviously correct solution (i.e. s/dir->basebuf/dir->base.buf/ plus strbuf_grow() before we write the buffer). But IMO your version is much cleaner already. However, api-strbuf.txt says that buf != NULL is invariant after init, and alloc is "somehow private" :-) , so perhaps you should - if (!dir->basebuf.alloc) + if (!dir->basebuf.buf) strbuf_init(&dir->basebuf, PATH_MAX); -- 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 1/3] tag: use skip_prefix instead of magic numbers
Jeff King writes: > On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote: > >> Make the parsing of the --sort parameter more readable by having >> skip_prefix keep our pointer up to date. >> >> Authored-by: Jeff King > > I suspect Junio may just apply this on the version of the commit he has > upstream, so you may not need this as part of your series. You read me correctly ;-) That is what I was planning to do, to fork jk/tag-sort topic on top of the updated jk/skip-prefix topic. > However, for reference, the right way to handle authorship is with a > > From: Jeff King > > at the top of your message body. Git-am will pick that up and turn it > into the author field of the commit. 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 2/2 v4] tag: support configuring --sort via .gitconfig
"Keller, Jacob E" writes: > This is not how the rest of the current tests work. I will submit a > patch which fixes up the current --sort tests (but not every test, for > now) as well. I do not want to pile more work that is unrelated to the task at hand on your plate, i.e. clean-up work, so I would assign a very low priority to "fix up the current tests". At the same time, existing mistakes are not excuses to introduce new similar ones, hence my suggestions to the added code in the patch I saw. -- 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 Fri, Jul 11, 2014 at 11:25:43AM -0700, Junio C Hamano wrote: > Christian Couder writes: > > > On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: > >> Christian Couder writes: > >> > >>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: > >>> > > "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? > >>> > >>> ... and here you say we should report "your wish has been granted"... > >> > >> Normal way for "git replace" to report that is to exit with status 0 > >> and without any noise, I would think. > > > > In a similar case "git replace --edit" we error out instead of just > > exiting (with status 0), see: > > > > f22166b5fee7dc (replace: make sure --edit results in a different object) > > I do not care *too* deeply, but if you ask me, that may be a mistake > we may want to fix before the next release. Yeah, I also do not care too deeply, but I mentioned in the earlier review that I would expect it to just remove the replacement if it ends up generating the same object. -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: pitfall with empty commits during git rebase
On Fri, Jul 11, 2014 at 12:15:47PM +0200, Olaf Hering wrote: > Could not apply 6c5842320acc797d395afb5cdf373c2bfaebfa34... revert > > > Its not clear what '--allow-empty' refers to, git rebase does not seem to > understand this option. I think this is the same problem discussed recently in: http://thread.gmane.org/gmane.comp.version-control.git/251365 -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 v6 02/10] replace: add --graft option
Christian Couder writes: > On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: >> Christian Couder writes: >> >>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: >>> > "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? >>> >>> ... and here you say we should report "your wish has been granted"... >> >> Normal way for "git replace" to report that is to exit with status 0 >> and without any noise, I would think. > > In a similar case "git replace --edit" we error out instead of just > exiting (with status 0), see: > > f22166b5fee7dc (replace: make sure --edit results in a different object) I do not care *too* deeply, but if you ask me, that may be a mistake we may want to fix before the next release. -- 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 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, Jul 11, 2014 at 06:11:08PM +, Keller, Jacob E wrote: > I personally prefer error out on options, even though it can make it a > bit more difficult, though as far as I know unknown fields simply warn > or are ignored. (ie: old versions of git just ignore unknown fields in > configuration). Right, we _have_ to ignore unknown config options, because we specifically allow other programs built on git to store their config with us (and anyway, our callback style of parsing means that no single callback knows about all of the keys). In the past we have staked out particular areas of the namespace, though. E.g., the diff code said "I own all of color.diff.*, and if you put in something I don't understand, I'll complain". That ended up being annoying, and now we ignore slots we don't understand there. So old gits will always silently ignore tag.sort if they don't know about it, and we can't change that. The only thing we can change is: > It's possible we should warn instead though, so that older gits work > with new sorts that they don't understand. Right. I think other config variables in similar situations will barf. This is backwards-compatible as long as the new variables are a superset (i.e., we only add new understood values, never remove or change the meaning of existing values). It's just not forwards-compatible. > I am ok with warning but I don't know the best practice for how to warn > here instead of failing. Returning error causes a fatal "bad config" > message. Any thoughts? The simplest thing is ignoring the return from parse_sort_string and just calling "return 0". That will still say "error:", but continue on. If you really want it to say "warning:", I think you'll have to pass a flag into parse_sort_string. I'm not sure if it's worth the effort. -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 v8 2/2] test-config: add tests for the config_set API
Tanay Abhra writes:
> 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() {
(style) SP around both sides of ().
More importantly, will we ever have different kind of check in this
script, perhaps checking if all values for a multi-valued variables
appear in the output, checking if get_bool works, etc. in the
future? I would imagine the answer is yes, and in that case this
should be renamed to be a bit more specific (i.e. no "too generic"
helper called "check" would exist in the final result). Perhaps
call it "check_single", "check_get_value" or "check_value" (the last
one assumes that all your future checks are mostly about various
forms of "get")?
> + echo "$2" >expected &&
> + test-config get_value "$1" >actual 2>&1 &&
> + test_cmp expected actual
> +}
> +
> +test_expect_success 'setup default config' '
> + cat >.git/config << EOF
- No SP after redirection operator.
- If you are not using variable substition in the here-doc, it is
more friendly to quote the end-of-here-doc token to tell the
reader that they do not have to worry about them.
- There may be core.* variables that are crucial for correct
operation of the version of Git being tested, so wiping and
replacing .git/config wholesale is not a good idea. Appending
your config items is sufficient for the purpose of these tests.
i.e.
cat >>.git/config <<\EOF
...
EOF
> + [core]
I'd feel safer if you did not abuse [core] like this. All you care
about is the config parsing, and you do not want future versions of
Git introducing core.MixedCase to mean something meaningful that
affects how "git config" and other commands work.
> + penguin = very blue
> + Movie = BadPhysics
> + UPPERCASE = true
> + MixedCase = true
> + my =
> ...
> + 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"
> +'
You would also need to see how
check core.uppercase
check core.MIXEDCASE
behave (after moving them out of "core." hierarchy, of course), like
the following checks for case insensitivity of the first token. The
first and the last token are both supposed to be case insensitive,
no?
> +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 &&
Is test_must_fail suffice here? git_config_get_value() has two
kinds of non-zero return values (one for "error", the other for "not
found"). Shouldn't test-config let the caller tell these two kinds
apart in some way? It would be reasonable to do so with its exit
status, I would imagine, and in that case, test_expect_code may be
more appropriate here.
> + test_cmp expect actual
Are you sure the expect and actual should match here?
Oh by the way, in "check()" helper shell function you spelled
the correct answer "expected" but here you use "expect" (like almost
all the other existing tests). Perhaps s/expected/expect/ while we
fix the helper function?
> +'
> +
> +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
> +'
Perhaps
check_config () {
op=$1 key=$2 &&
shift &&
shift &&
printf "%s\n" "$@" >expect &&
test-config "$op" "$key" >actual &&
test_cmp expect actual
}
and use it like so?
check_config get_value core.mixedcase true
check_config get_int lamb.chop 65
check_config get_bool goat.head 1
check_config get_value_multi core.baz sam bat hask
> +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_
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 13:46 -0400, Jeff King wrote:
> On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
>
> > Updated to include changes due to Junio's feedback. This has not resolved
> > whether we should fail on a configuration error or simply warn. It appears
> > that
> > we actually seem to error out more than warn, so I am unsure what the
> > correct
> > action is here.
>
> Yeah, we're quite inconsistent there. In some cases we silently ignore
> something unknown (e.g., a color.diff.* slot that we do not understand),
> but in most cases if it is a config key we understand but a value we do
> not, we complain and die.
>
> It's probably user-unfriendly to be silent for those cases, though. The
> user gets no feedback on why their config value is doing nothing.
>
> I tend to think that warning is not much better than erroring out. It is
> helpful if you are running a single-shot of an old version (which is
> something that I do a lot when testing old versions), but would quickly
> become irritating if you were actually using an old version of git
> day-to-day.
>
> I dunno. Maybe it is worth making life easier for people in the former
> category.
>
> > +static int parse_sort_string(const char *arg, int *sort)
> > +{
> > + int type = 0, flags = 0;
> > +
> > + if (skip_prefix(arg, "-", &arg))
> > + flags |= REVERSE_SORT;
> > +
> > + if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> > + type = VERCMP_SORT;
> > + else
> > + type = STRCMP_SORT;
> > +
> > + if (strcmp(arg, "refname"))
> > + return error(_("unsupported sort specification %s"), arg);
> > +
> > + *sort = (type | flags);
> > +
> > + return 0;
> > +}
>
> Regardless of how we handle the error, I think this version that
> assembles the final bitfield at the end is easier to read than the
> original.
>
Yes, I figured setting it up all at the end makes more sense, and is
less prone to subtle bugs, since we don't directly modify sort using |=
or relying on particular values of sort initially.
I personally prefer error out on options, even though it can make it a
bit more difficult, though as far as I know unknown fields simply warn
or are ignored. (ie: old versions of git just ignore unknown fields in
configuration).
It's possible we should warn instead though, so that older gits work
with new sorts that they don't understand.
I am ok with warning but I don't know the best practice for how to warn
here instead of failing. Returning error causes a fatal "bad config"
message. Any thoughts?
Thanks,
Jake
> -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 1/3] tag: use skip_prefix instead of magic numbers
On Fri, 2014-07-11 at 13:50 -0400, Jeff King wrote: > On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote: > > > Make the parsing of the --sort parameter more readable by having > > skip_prefix keep our pointer up to date. > > > > Authored-by: Jeff King > > I suspect Junio may just apply this on the version of the commit he has > upstream, so you may not need this as part of your series. > > However, for reference, the right way to handle authorship is with a > > From: Jeff King > > at the top of your message body. Git-am will pick that up and turn it > into the author field of the commit. > Alright, thanks. Regards, Jake > -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: [Bug] data loss with cyclic alternates
On Fri, 2014-07-11 at 09:01 -0700, Junio C Hamano wrote: > Ephrim Khong writes: > > > git seems to have issues with alternates when cycles are present (repo > > A has B/objects as alternates, B has A/objects as alternates). > > Yeah, don't do that. A thinks "eh, the other guy must have it" and > B thinks the same. In general, do not prune or gc a repository > other repositories borrow from, even if there is no cycle, because > the borrowee does not know anythning about objects that it itself no > longer needs but are still needed by its borrowers. > Doesn't gc get run automatically at some points? Is the automatic gc run setup to avoid this problem? Thanks, Jake > -- > 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: pitfall with empty commits during git rebase
On Fri, 2014-07-11 at 12:15 +0200, Olaf Hering wrote:
> There is an incorrect message when doing "git rebase -i remote/branch".
> I have it only in german, see below. what happend is:
>
> #01 make changes on another host
> #02 copy patchfile to localhost
> #03 apply patchfile
> #04 git commit -avs # create commit#1
> #05 sleep 123456
> #06 make different changes on another host
> #07 copy patchfile to localhost
> #08 git show | patch -Rp1
> #09 git commit -avs # create commit#2
> #10 apply patchfile
> #11 git commit -avs # create commit#3
> #12 git rebase -i remote/branch
> pick commit#1 msg
> fcommit#2 msg
> fcommit#3 msg
>
>
> ".git/rebase-merge/git-rebase-todo" 21L, 707C geschrieben
> Sie fragten den jüngsten Commit nachzubessern, aber das würde diesen leer
> machen. Sie können Ihr Kommando mit --allow-empty wiederholen, oder diesen
> Commit mit "git reset HEAD^" vollständig entfernen.
> Rebase im Gange; auf c105589
> Sie sind gerade beim Rebase von Branch 'mybranch' auf 'c105589'.
>
> Keine Änderungen
>
> Could not apply 6c5842320acc797d395afb5cdf373c2bfaebfa34... revert
>
>
> Its not clear what '--allow-empty' refers to, git rebase does not seem to
> understand this option.
>
You should be able to fix this with
git commit --allow-empty
git rebase --continue
But yes the message could possibly be made a little clearer.
Thanks,
Jake
> I should have skipped step #09 to avoid the trouble.
> git version is 2.0.1. Please adjust the error msg above.
>
>
> Olaf
> --
> 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
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers
On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote: > Make the parsing of the --sort parameter more readable by having > skip_prefix keep our pointer up to date. > > Authored-by: Jeff King I suspect Junio may just apply this on the version of the commit he has upstream, so you may not need this as part of your series. However, for reference, the right way to handle authorship is with a From: Jeff King at the top of your message body. Git-am will pick that up and turn it into the author field of the commit. -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 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
> Updated to include changes due to Junio's feedback. This has not resolved
> whether we should fail on a configuration error or simply warn. It appears
> that
> we actually seem to error out more than warn, so I am unsure what the correct
> action is here.
Yeah, we're quite inconsistent there. In some cases we silently ignore
something unknown (e.g., a color.diff.* slot that we do not understand),
but in most cases if it is a config key we understand but a value we do
not, we complain and die.
It's probably user-unfriendly to be silent for those cases, though. The
user gets no feedback on why their config value is doing nothing.
I tend to think that warning is not much better than erroring out. It is
helpful if you are running a single-shot of an old version (which is
something that I do a lot when testing old versions), but would quickly
become irritating if you were actually using an old version of git
day-to-day.
I dunno. Maybe it is worth making life easier for people in the former
category.
> +static int parse_sort_string(const char *arg, int *sort)
> +{
> + int type = 0, flags = 0;
> +
> + if (skip_prefix(arg, "-", &arg))
> + flags |= REVERSE_SORT;
> +
> + if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> + type = VERCMP_SORT;
> + else
> + type = STRCMP_SORT;
> +
> + if (strcmp(arg, "refname"))
> + return error(_("unsupported sort specification %s"), arg);
> +
> + *sort = (type | flags);
> +
> + return 0;
> +}
Regardless of how we handle the error, I think this version that
assembles the final bitfield at the end is easier to read than the
original.
-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: git p4 diff-tree ambiguous argument error
More data points. I have reproduced the problem on $ git --version git version 1.8.5.2 (Apple Git-48) $ python --version Python 2.7.5 $ uname -a Darwin Kernel Version 13.3.0: Tue Jun 3 21:27:35 PDT 2014; root:xnu-2422.110.17~1/RELEASE_X86_64 x86_64 However, it the command is used on a new empty repository, there is no failure. Running a second time causes the failure. Thanks for any help you can offer in tracking down this problem! -- View this message in context: http://git.661346.n2.nabble.com/git-p4-diff-tree-ambiguous-argument-error-tp7614774p7614882.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 v3] http: Add Accept-Language header if possible
On Sat, Jul 12, 2014 at 01:52:53AM +0900, Yi EungJun wrote:
> Add an Accept-Language header which indicates the user's preferred
> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
>
> Examples:
> LANGUAGE= -> ""
> LANGUAGE=ko:en -> "Accept-Language: ko, en; q=0.9, *; q=0.1"
> LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *; q=0.1"
> LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *; q=0.1"
>
> This gives git servers a chance to display remote error messages in
> the user's preferred language.
Thanks, this is looking much nicer. Most of my comments are on style:
> +static const char* get_preferred_languages() {
> +const char* retval;
A few style nits:
1. We usually put a function's opening brace on a new line.
2. We usually put the asterisk in a pointer declaration with the
variable name ("const char *retval"). This one appears elsewhere in
the patch.
3. This line seems to be indented with spaces instead of tabs.
> + retval = getenv("LANGUAGE");
> + if (retval != NULL && retval[0] != '\0')
> + return retval;
> +
> + retval = setlocale(LC_MESSAGES, NULL);
> + if (retval != NULL && retval[0] != '\0'
> + && strcmp(retval, "C") != 0
> + && strcmp(retval, "POSIX") != 0)
> + return retval;
More style nits: we usually avoid writing out explicit comparisons with
NULL (and often with zero). So I'd write this as:
if (retval && *retval &&
strcmp(retval, "C") &&
strcmp(retval, "POSIX"))
but I think the NULL one is the only one we usually enforce explicitly.
> + const char *p1, *p2, *p3;
I had trouble following the logic with these variable names. Is it
possible to give them more descriptive names?
> + /* Don't add Accept-Language header if no language is preferred. */
> + if (p1 == NULL || p1[0] == '\0') {
> + strbuf_release(&buf);
> + return headers;
> + }
No need to strbuf_release a buffer that hasn't been used (assigning
STRBUF_INIT does not count as use).
> + /* Count number of preferred languages to decide precision of q-factor
> */
> + for (p3 = p1; *p3 != '\0'; p3++) {
> + if (*p3 == ':') {
> + num_langs++;
> + }
> + }
Style: we usually omit braces for one-liners. So:
for (p3 = p1; *p3; p3++)
if (*p3 == ':')
num_langs++;
(and elsewhere).
> + /* Decide the precision for q-factor on number of preferred languages.
> */
> + if (num_langs + 1 > 100) { /* +1 is for '*' */
> + q_precision = 0.001;
> + q_format = "; q=%.3f";
> + } else if (num_langs + 1 > 10) { /* +1 is for '*' */
> + q_precision = 0.01;
> + q_format = "; q=%.2f";
> + }
I don't mind this auto-precision too much, but I'm not sure it buys us
anything. We are still setting a hard-limit at 100, and it just means we
write "0.1" instead of "0.001" sometimes.
> + headers = curl_slist_append(headers, buf.buf);
> +
> + strbuf_release(&buf);
Do all versions of curl make a copy of buf.buf here?
I seem to recall that older versions want pointers passed to
curl_easy_setopt to stay around for the duration of the request (I do
not know about curl_slist, though).
> @@ -1020,6 +1143,8 @@ static int http_request(const char *url,
>fwrite_buffer);
> }
>
> + headers = add_accept_language(headers);
This means we do the whole parsing routine for each request we make (for
dumb-http, this can be quite a lot, though I suppose the parsing is not
especially expensive). Should we perhaps just cache the result in a
static strbuf? That would also make the pointer-lifetime issue above go
away.
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -946,6 +946,8 @@ int main(int argc, const char **argv)
> struct strbuf buf = STRBUF_INIT;
> int nongit;
>
> + git_setup_gettext();
Oops. We probably should have been doing this all along to localize the
messages on our side.
> +test_expect_success 'git client sends Accept-Language based on LANGUAGE,
> LC_ALL, LC_MESSAGES and LANG' '
> + test_must_fail env GIT_CURL_VERBOSE=1 LANGUAGE=ko_KR.UTF-8
> LC_ALL=de_DE.UTF-8 LC_MESSAGES=ja_JP.UTF-8 LANG=en_US.UTF-8 git clone
> "$HTTPD_URL/accept/language" 2>stderr &&
We usually try to avoid long lines (you can break them up with "\").
> + grep "^Accept-Language: ko-KR, \\*; q=0.1" stderr &&
I see you noticed the extra level of quoting necessary between v2 and
v3. :)
I wonder if these test cases might be more readable with a helper
function like:
check_language () {
echo "Accept-Language: $1" >expect &&
test_must_fail env \
GIT_CURL_VERBOSE=1 \
LANGUAGE=$2 \
LC_ALL=$3 \
LC_MESSAGES=$4 \
LANG=$5 \
git clone "$HTTPD_URL/accept/language" 2>stderr &&
Re: [PATCH v8 1/2] add `config_set` API for caching config-like files
Tanay Abhra writes:
> diff --git a/config.c b/config.c
> index ba882a1..aa58275 100644
> --- a/config.c
> +++ b/config.c
> @@ -9,6 +9,8 @@
> #include "exec_cmd.h"
> #include "strbuf.h"
> #include "quote.h"
> +#include "hashmap.h"
> +#include "string-list.h"
>
> struct config_source {
> struct config_source *prev;
> @@ -33,10 +35,23 @@ struct config_source {
> long (*do_ftell)(struct config_source *c);
> };
>
> +struct config_hash_entry {
> + struct hashmap_entry ent;
> + char *key;
> + struct string_list value_list;
> +};
> +
> static struct config_source *cf;
>
> static int zlib_compression_seen;
>
> +/*
> + * Default config_set that contains key-value pairs from the usual set of
> config
> + * config files (i.e repo specific .git/config, user wide ~/.gitconfig, XDG
> + * config file and the global /etc/gitconfig)
> + */
> +static struct config_set the_config_set;
> +
> static int config_file_fgetc(struct config_source *conf)
> {
> return fgetc(conf->u.file);
> @@ -1212,6 +1227,284 @@ int git_config(config_fn_t fn, void *data)
> return git_config_with_options(fn, data, NULL, 1);
> }
>
> +static int config_hash_add_value(const char *, const char *, struct hashmap
> *);
This is a funny forward declaration. If you define add-file and
friends that modify the config-set _after_ you define find-entry and
friends that are used to look-up, would you still need to do a
forward declaration?
In any case, please give names to these first two parameters as what
they are for can be unclear; because they are of the same type,
there is one less clue than there usually are.
The function signature makes it sound as if this is not specific to
"config"; what takes a hashmap, a key and a value and is called "add"
is a function to add pair to a hashmap. Why doesn't it
take "struct config_set"? In other words, I would have expected
static int config_set_add(struct config_set *, const char *key, const char
*value)
instead. Not a complaint, but is puzzled why you chose not to do
so.
I suspect almost everywhere in this patch, you would want to do
s/config_hash/config_set/. s/config_hash_entry/config_set_element/
might be a good idea, too. You have the concept of the "config
set", and each element of that set is a "config-set element", not a
"config-hash entry", isn't it?
> +static int config_hash_entry_cmp(const struct config_hash_entry *e1,
> + const struct config_hash_entry *e2, const void
> *unused)
> +{
> + return strcmp(e1->key, e2->key);
> +}
> +
> +static void configset_init(struct config_set *cs)
> +{
> + if (!cs->hash_initialized) {
> + hashmap_init(&cs->config_hash,
> (hashmap_cmp_fn)config_hash_entry_cmp, 0);
> + cs->hash_initialized = 1;
> + }
> +}
Have uninitializer here, immediately after you defined the initializer.
> +static int config_hash_callback(const char *key, const char *value, void *cb)
> +{
> + struct config_set *cs = cb;
> + config_hash_add_value(key, value, &cs->config_hash);
> + return 0;
> +}
> +
> +int git_configset_add_file(struct config_set *cs, const char *filename)
> +{
> + int ret = 0;
> + configset_init(cs);
> + ret = git_config_from_file(config_hash_callback, filename, cs);
> + if (!ret)
> + return 0;
> + else {
> + hashmap_free(&cs->config_hash, 1);
> + cs->hash_initialized = 0;
> + return -1;
Does calling configset_clear() do too much for the purpose of this
error path? In other words, is it deliberate that you do not touch
any string-list items you may have accumulated by calling the
callback?
> +static struct config_hash_entry *config_hash_find_entry(const char *key,
> + struct hashmap
> *config_hash)
> +{
> + struct config_hash_entry k;
> + struct config_hash_entry *found_entry;
> + char *normalized_key;
> + int ret;
> + /*
> + * `key` may come from the user, so normalize it before using it
> + * for querying entries from the hashmap.
> + */
> + ret = git_config_parse_key(key, &normalized_key, NULL);
> +
> + if (ret)
> + return NULL;
> +
> + hashmap_entry_init(&k, strhash(normalized_key));
> + k.key = normalized_key;
> + found_entry = hashmap_get(config_hash, &k, NULL);
> + free(normalized_key);
> + return found_entry;
> +}
> +
> +static struct string_list *configset_get_list(const char *key, struct
> config_set *cs)
> +{
> + struct config_hash_entry *e = config_hash_find_entry(key,
> &cs->config_hash);
> + return e ? &e->value_list : NULL;
> +}
> +
> +static int config_hash_add_value(const char *key, const char *value, struct
> hashmap *config_hash)
> +{
> + struct config_hash_entry *e;
> + e = config_hash_find_entry(key, config_hash);
> + /*
> + * Since the keys are being fed by git_config*() callback mechanism,
[PATCH 1/3] 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
Signed-off-by: Jacob Keller
---
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/3] tag: fix --sort tests to use cat<<-\EOF format
The --sort tests should use the better format for >expect to maintain indenting and ensure that no substitution is occurring. This makes parsing and understanding the tests a bit easier. Signed-off-by: Jacob Keller --- t/t7004-tag.sh | 40 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index e4ab0f5b6419..66010b0e7066 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' ' git tag foo1.6 && git tag foo1.10 && git tag -l --sort=refname "foo*" >actual && - cat >expectactual && - cat >expect actual && - cat >expect actual && - cat >expect
[PATCH 3/3 v5] 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
Signed-off-by: Jacob Keller
---
Updated to include changes due to Junio's feedback. This has not resolved
whether we should fail on a configuration error or simply warn. It appears that
we actually seem to error out more than warn, so I am unsure what the correct
action is here.
Documentation/config.txt | 5 +
Documentation/git-tag.txt | 5 -
builtin/tag.c | 56 +--
t/t7004-tag.sh| 25 +
4 files changed, 74 insertions(+), 17 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
+tag.sort::
+ This variable controls the sort ordering of tags when displayed by
+ linkgit:git-tag[1]. Without the "--sort=" option provided, the
+ value of this variable will be used as the default.
+
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..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
Sort in a specific order. Supported type is "refname"
(lexicographic order), "version:refname" or "v:refname" (tag
names are treated as versions). Prepend "-" to reverse sort
- order.
+ order. When this option is not given, the sort order defaults to the
+ value configured for the 'tag.sort' variable if it exists, or
+ lexicographic order otherwise. See linkgit:git-config[1].
--column[=]::
--no-column::
@@ -317,6 +319,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..f14aa346e0d4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
#define SORT_MASK 0x7fff
#define REVERSE_SORT0x8000
+static int tag_sort;
+
struct tag_filter {
const char **patterns;
int lines;
@@ -346,9 +348,41 @@ static const char tag_template_nocleanup[] =
"Lines starting with '%c' will be kept; you may remove them"
" yourself if you want to.\n");
+/*
+ * Parse a sort string, and return 0 if parsed successfully. Will return
+ * non-zero when the sort string does not parse into a known type.
+ */
+static int parse_sort_string(const char *arg, int *sort)
+{
+ int type = 0, flags = 0;
+
+ if (skip_prefix(arg, "-", &arg))
+ flags |= REVERSE_SORT;
+
+ if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
+ type = VERCMP_SORT;
+ else
+ type = STRCMP_SORT;
+
+ if (strcmp(arg, "refname"))
+ return error(_("unsupported sort specification %s"), arg);
+
+ *sort = (type | flags);
+
+ return 0;
+}
+
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")) {
+ if (!value)
+ return config_error_nonbool(var);
+ return parse_sort_string(value, &tag_sort);
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, "column."))
@@ -522,18 +556,8 @@ 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;
-
- 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;
+ return parse_sort_string(arg, sort);
}
int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -546,7 +570,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
Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
> Jacob Keller writes:
>
> > +
> > + if (strcmp(arg, "refname"))
> > + die(_("unsupported sort specification %s"), arg);
>
> Hmm. I _thought_ we try to catch unsupported option value coming
> from the command line and die but at the same time we try *not* to
> die but warn and whatever is sensible when the value comes from the
> configuration, so that .git/config or $HOME/.gitconfig can be shared
> by those who use different versions of Git.
>
> Do we already have many precedences where we see configuration value
> that our version of Git do not yet understand?
>
> Not a very strong objection; just something that worries me.
Based on a cursory glance at how the config is parsed, we actually
die_on_error for most options. Does it makes sense to warn or not? I am
not really sure here.. At any rate I updated this to be a bit cleaner
and use error( instead of die, so that it will work within how config is
supposed to.. Plus, this also makes it so that --sort shows the usage if
it's invalid.
Please comment more on the new set I am sending so we can really
determine what the correct course of action is here.
Regards,
Jake
Re: [PATCH 4/7] add object_as_type helper for casting objects
On Fri, Jul 11, 2014 at 11:45:58AM +0100, Ramsay Jones wrote:
> > @@ -1729,9 +1729,8 @@ static enum peel_status peel_object(const unsigned
> > char *name, unsigned char *sh
> >
> > if (o->type == OBJ_NONE) {
> > int type = sha1_object_info(name, NULL);
> > - if (type < 0)
> > + if (type < 0 || !object_as_type(o, type, 0))
> ^^^
>
> It is not possible here for object_as_type() to issue an error()
> report, but I had to go look at the code to check. (It would have
> been a change in behaviour, otherwise). So, it doesn't really matter
> what you pass to the quiet argument, but setting it to 1 _may_ help
> the next reader. (No, I'm not convinced either; the only reason I
> knew it had anything to do with error messages was because I had
> just read the code ...) Hmm, dunno.
Right, as I mentioned in the commit message, the type-check part of
object_type is a noop here. In that sense you could write this as just:
object_as_type(o, type. 1);
and ignore the return value. However, I'd rather err on the side of
checking for and reporting the error, even if we expect it to do nothing
right now. That decouples the two functions, and if object_as_type ever
learns to report other errors, then we automatically do the right
thing here.
-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 v8 1/2] add `config_set` API for caching config-like files
Tanay Abhra writes: > I had seen that there were checks for Syntax error or Non-existant files in > t1300-repo-config, for example, The code raising the syntax error is there, and tested. But the way the error code (eg. return -1 from git_config) is handled by your code is not. -- 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 v3] http: Add Accept-Language header if possible
Add an Accept-Language header which indicates the user's preferred
languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
Examples:
LANGUAGE= -> ""
LANGUAGE=ko:en -> "Accept-Language: ko, en; q=0.9, *; q=0.1"
LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *; q=0.1"
LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *; q=0.1"
This gives git servers a chance to display remote error messages in
the user's preferred language.
Signed-off-by: Yi EungJun
---
http.c | 125 +
remote-curl.c | 2 +
t/t5550-http-fetch-dumb.sh | 21
3 files changed, 148 insertions(+)
diff --git a/http.c b/http.c
index 3a28b21..a20f3e2 100644
--- a/http.c
+++ b/http.c
@@ -983,6 +983,129 @@ static void extract_content_type(struct strbuf *raw,
struct strbuf *type,
strbuf_addstr(charset, "ISO-8859-1");
}
+/*
+ * Guess the user's preferred languages from the value in LANGUAGE environment
+ * variable and LC_MESSAGES locale category.
+ *
+ * The result can be a colon-separated list like "ko:ja:en".
+ */
+static const char* get_preferred_languages() {
+const char* retval;
+
+ retval = getenv("LANGUAGE");
+ if (retval != NULL && retval[0] != '\0')
+ return retval;
+
+ retval = setlocale(LC_MESSAGES, NULL);
+ if (retval != NULL && retval[0] != '\0'
+ && strcmp(retval, "C") != 0
+ && strcmp(retval, "POSIX") != 0)
+ return retval;
+
+ return NULL;
+}
+
+/*
+ * Add an Accept-Language header which indicates user's preferred languages.
+ *
+ * Examples:
+ * LANGUAGE= -> ""
+ * LANGUAGE=ko:en -> "Accept-Language: ko, en; q=0.9, *; q=0.1"
+ * LANGUAGE=ko_KR.UTF-8:sr@latin -> "Accept-Language: ko-KR, sr; q=0.9, *;
q=0.1"
+ * LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *; q=0.1"
+ * LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *; q=0.1"
+ * LANGUAGE= LANG=C -> ""
+ */
+static struct curl_slist* add_accept_language(struct curl_slist *headers)
+{
+ const char *p1, *p2, *p3;
+ struct strbuf buf = STRBUF_INIT;
+ float q = 1.0;
+ float q_precision = 0.1;
+ int num_langs = 1;
+ char* q_format = "; q=%.1f";
+
+ p1 = get_preferred_languages();
+
+ /* Don't add Accept-Language header if no language is preferred. */
+ if (p1 == NULL || p1[0] == '\0') {
+ strbuf_release(&buf);
+ return headers;
+ }
+
+ /* Count number of preferred languages to decide precision of q-factor
*/
+ for (p3 = p1; *p3 != '\0'; p3++) {
+ if (*p3 == ':') {
+ num_langs++;
+ }
+ }
+
+ /* Decide the precision for q-factor on number of preferred languages.
*/
+ if (num_langs + 1 > 100) { /* +1 is for '*' */
+ q_precision = 0.001;
+ q_format = "; q=%.3f";
+ } else if (num_langs + 1 > 10) { /* +1 is for '*' */
+ q_precision = 0.01;
+ q_format = "; q=%.2f";
+ }
+
+ strbuf_addstr(&buf, "Accept-Language: ");
+
+ for (p2 = p1; q > q_precision; p2++) {
+ if ((*p2 == ':' || *p2 == '\0') && p1 != p2) {
+ if (q < 1.0) {
+ strbuf_addstr(&buf, ", ");
+ }
+
+ for (p3 = p1; p3 < p2; p3++) {
+ /* Replace '_' with '-'. */
+ if (*p3 == '_') {
+ strbuf_add(&buf, p1, p3 - p1);
+ strbuf_addstr(&buf, "-");
+ p1 = p3 + 1;
+ }
+
+ /* Chop off anything after '.' or '@'. */
+ if ((*p3 == '.' || *p3 == '@')) {
+ break;
+ }
+ }
+
+ if (p3 > p1) {
+ strbuf_add(&buf, p1, p3 - p1);
+ }
+
+ /* Put the q factor if only it is less than 1.0. */
+ if (q < 1.0) {
+ strbuf_addf(&buf, q_format, q);
+ }
+
+ q -= q_precision;
+ p1 = p2 + 1;
+
+ if (*p2 == '\0') {
+ break;
+ }
+ }
+ }
+
+ /* Don't add Accept-Language header if no language is preferred. */
+ if (q >= 1.0) {
+ strbuf_release(&buf);
+ return headers;
+ }
+
+ /* Add '*' with minimum q-factor greater than 0.0. */
+ strbuf_addstr(&buf, ", *");
+ strbuf_addf(&buf, q_format, q_precision);
+
+ headers = curl_slist_append(headers, buf.buf);
+
+
Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
> Jacob Keller writes:
>
> > 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
> > Signed-off-by: Jacob Keller
> > ---
> > - 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..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=". 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.
> > +
>
> This is not technically incorrect per-se, but the third sentence
> talks about "--sort" on "the command line" while this applies only
> to "the command line of the 'git tag' command" and nobody else's
> "--sort" option.
>
> Perhaps rephrasing it like this may be easier to understand?
>
> When "git tag" command is used to list existing tags,
> without "--sort=" option on the command line,
> the value of this variable is used as the default.
>
> This way, it would be clear, without explicitly saying anything,
> that the value will be spelled exactly the same way as you would
> spell the value for the --sort option on the command line.
>
> > 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].
>
> It is not particularly friendly to readers to refer to
> "git-config[1]" from any other page, if done without spelling out
> which variable the reader is expected to look up. Some addition
> to the description of the "--sort" option is needed if this SEE ALSO
> were to be any useful, I guess?
>
> --sort=::
> ... (existing description) ...
> When this option is not given, the sort order
> defaults to the value configured for the `tag.sort`
> variable, if exists, or lexicographic otherwise.
>
> or something like that, perhaps?
>
> > 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;
>
> Please do not initialize variables in bss segment to 0 by hand.
>
> If this variable is meant to take one of these *CMP_SORT values
> defined as macro later in this file, it is better to define this
> variable somewhere after and close to the definitions of the macros.
> Perhaps immediately after the "struct tag_filter" is declared?
>
> > @@ -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);
>
> Hmm. I _thought_ we try to catch unsupported option value coming
> from the command line and die but at the same time we try *not* to
> die but warn and whatever is sensible when the value comes from the
> configuration, so that .git/config or $HOME/.gitconfig can be shared
> by those who use different versions of Git.
>
> Do we already have many precedences where we see configuration value
> that our version of Git do not yet understand?
>
> Not a very strong objection; just something that worries me.
>
> > + 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);
> >
Re: [PATCH] http: Add Accept-Language header if possible
2014-07-12 1:24 GMT+09:00 Eric Sunshine : > On Fri, Jul 11, 2014 at 5:22 AM, Yi, EungJun wrote: >> 2014-07-09 6:52 GMT+09:00 Eric Sunshine : + grep "^Accept-Language: ko; q=1.000, en; q=0.999, \*; q=0.001" actual >>> >>> Do you want to \-escape the periods? (Or maybe use 'grep -F'?) >> >> I just want to match '*' character. I tried 'grep -F' but it does not help. > > I meant that the periods in your grep pattern are matching any > character. If you want to be very strict, so that they match only > period, then you should \-escape them. Oops, I misunderstood you. You are right. I'll \- escape them. -- 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 v8 1/2] add `config_set` API for caching config-like files
On 7/11/2014 7:51 PM, Matthieu Moy wrote:
> Hi,
>
> I had a closer look at error management (once more, sorry: I should have
> done this earlier...), and it seems to me that:
>
> * Not all errors are managed properly
>
> * Most error cases are untested
>
> Among the cases I can think of:
>
> * Syntax error when parsing the file
>
> * Non-existant file
>
> * Unreadable file (chmod -r)
>
I had seen that there were checks for Syntax error or Non-existant files in
t1300-repo-config, for example,
# malformed configuration files
test_expect_success 'barf on syntax error' '
cat >.git/config <<-\EOF &&
# broken section line
[section]
key garbage
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 3 " error
'
test_expect_success 'barf on incomplete section header' '
cat >.git/config <<-\EOF &&
# broken section line
[section
key = value
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 2 " error
'
test_expect_success 'barf on incomplete string' '
cat >.git/config <<-\EOF &&
# broken section line
[section]
key = "value string
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 3 " error
'
test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '
test_must_fail git config --file non-existing-config -l
'
They cover the same parsing code which I used for constructing the cache.
Still, more tests will not harm anyone, I will add more testcases accordingly
for the corner cases you raised. :)
> Tanay Abhra writes:
>
>> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::
>> +
>> +Parses the file and adds the variable-value pairs to the `config_set`,
>> +dies if there is an error in parsing the file.
>
> The return value is undocumented.
>
> If I read correctly, the only codepath from this to a syntax error sets
> die_on_error, hence "dies if there is an error in parsing the file" is
> correct.
>
> Still, there are errors like "unreadable file" or "no such file" that do
> not die (nor emit any error message, which is not very good for the
> user), and lead to returning -1 here.
>
> I'm not sure this distinction is right (why die on syntax error and
> continue running on unreadable file?).
>
> In any case, it should be documented and tested. I'll send a fixup patch
> with a few more example tests (probably insufficient).
>
I am not sure of this myself, I will have to look into it.
>> +static int git_config_check_init(void)
>> +{
>> +int ret = 0;
>> +if (the_config_set.hash_initialized)
>> +return 0;
>> +configset_init(&the_config_set);
>> +ret = git_config(config_hash_callback, &the_config_set);
>> +if (ret >= 0)
>> +return 0;
>> +else {
>> +hashmap_free(&the_config_set.config_hash, 1);
>> +the_config_set.hash_initialized = 0;
>> +return -1;
>> +}
>> +}
>
> We have the same convention for errors here. But a more serious issue is
> that the return value of this function is ignored most of the time.
>
> It seems git_config should never return a negative value, as it calls
> git_config_with_options -> git_config_early, which checks for file
> existance and permission before calling git_config_from_file. Indeed,
> Git's tests still pass after this:
>
> --- a/config.c
> +++ b/config.c
> @@ -1225,7 +1225,10 @@ int git_config_with_options(config_fn_t fn, void *data,
>
> int git_config(config_fn_t fn, void *data)
> {
> - return git_config_with_options(fn, data, NULL, 1);
> + int ret = git_config_with_options(fn, data, NULL, 1);
> + if (ret < 0)
> + die("Negative return value in git_config");
> + return ret;
> }
>
> Still, we can imagine cases like race condition between access_or_die()
> and git_config_from_file() in
>
> if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
> 0)) {
> ret += git_config_from_file(fn, git_etc_gitconfig(),
> data);
> found += 1;
> }
>
> where the function would indeed return -1. In any case, either we
> consider that git_config should never return -1, and we should die in
> this case, or we consider that it may happen, and that the "else" branch
> of the if/else above is not dead code, and then we can't just ignore the
> return value.
>
> I think we should just do something like this:
>
> diff --git a/config.c b/config.c
> index 74adbbd..5c023e8 100644
> --- a/config.c
> +++ b/config.c
> @@ -1428,7 +1428,7 @@ int git_configset_get_pathname(struct config_set *cs,
> const char *key, const cha
> return 1;
> }
>
> -static int git_config_check_init(void)
> +static void git_config_check_init(void)
> {
> int ret = 0;
>
Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
"Keller, Jacob E" writes: > On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote: > ... >> > +static int tag_sort = 0; >> >> Please do not initialize variables in bss segment to 0 by hand. >> >> If this variable is meant to take one of these *CMP_SORT values >> defined as macro later in this file, it is better to define this >> variable somewhere after and close to the definitions of the macros. >> Perhaps immediately after the "struct tag_filter" is declared? > > I put it just above the struct tag_filter now, as this puts it right > below the #defines regarding it's value. Either would be fine, but just to clarify. Because these macro definitions are for the .sort field of that structure, and the new tag_sort variable is the second user of that macro, my suggestion to put it _after_ was to be in line with "add new things at the end, when there is no compelling reason not to" below. >> When there is no reason to have things in a particular order, it is >> customary to add new things at the end, not in the front, unless the >> new thing is so much more important than everything else---but then >> we are no longer talking about the case where there is no reason to >> have things in a particular order ;-). 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
On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: > Christian Couder writes: > >> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: >> "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? >> >> ... and here you say we should report "your wish has been granted"... > > Normal way for "git replace" to report that is to exit with status 0 > and without any noise, I would think. In a similar case "git replace --edit" we error out instead of just exiting (with status 0), see: f22166b5fee7dc (replace: make sure --edit results in a different object) -- 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 2/2 v4] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
> Jacob Keller writes:
>
> > 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
> > Signed-off-by: Jacob Keller
> > ---
> > - 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..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=". 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.
> > +
>
> This is not technically incorrect per-se, but the third sentence
> talks about "--sort" on "the command line" while this applies only
> to "the command line of the 'git tag' command" and nobody else's
> "--sort" option.
>
> Perhaps rephrasing it like this may be easier to understand?
>
> When "git tag" command is used to list existing tags,
> without "--sort=" option on the command line,
> the value of this variable is used as the default.
>
> This way, it would be clear, without explicitly saying anything,
> that the value will be spelled exactly the same way as you would
> spell the value for the --sort option on the command line.
>
Makes sense.
> > 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].
>
> It is not particularly friendly to readers to refer to
> "git-config[1]" from any other page, if done without spelling out
> which variable the reader is expected to look up. Some addition
> to the description of the "--sort" option is needed if this SEE ALSO
> were to be any useful, I guess?
>
> --sort=::
> ... (existing description) ...
> When this option is not given, the sort order
> defaults to the value configured for the `tag.sort`
> variable, if exists, or lexicographic otherwise.
>
> or something like that, perhaps?
>
Alright, this sounds good too.
> > 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;
>
> Please do not initialize variables in bss segment to 0 by hand.
>
> If this variable is meant to take one of these *CMP_SORT values
> defined as macro later in this file, it is better to define this
> variable somewhere after and close to the definitions of the macros.
> Perhaps immediately after the "struct tag_filter" is declared?
>
I put it just above the struct tag_filter now, as this puts it right
below the #defines regarding it's value.
> > @@ -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);
>
> Hmm. I _thought_ we try to catch unsupported option value coming
> from the command line and die but at the same time we try *not* to
> die but warn and whatever is sensible when the value comes from the
> configuration, so that .git/config or $HOME/.gitconfig can be shared
> by those who use different versions of Git.
>
> Do we already have many precedences where we see configuration value
> that our version of Git do not yet understand?
>
> Not a very strong objection; just something that worries me.
>
I like this change, and I think I have a way to handle it. I will
re-work this so that the die is handled by the normal block.
> > + sort |= flags;
> > +
> >
Re: [Bug] data loss with cyclic alternates
Ephrim Khong writes: > git seems to have issues with alternates when cycles are present (repo > A has B/objects as alternates, B has A/objects as alternates). Yeah, don't do that. A thinks "eh, the other guy must have it" and B thinks the same. In general, do not prune or gc a repository other repositories borrow from, even if there is no cycle, because the borrowee does not know anythning about objects that it itself no longer needs but are still needed by its borrowers. -- 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 4/4 v6] cache-tree: Write updated cache-tree after commit
David Turner writes:
> @@ -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.
Is this statement now stale and needs to be removed?
> -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
> +}
As we always have had trouble between $PWD and $(pwd) on other
platforms, I'd prefer something simpler, like:
expected_cache_tree () {
(
# subshell to let it wander around freely
generate_expected_cache_tree_rec
)
}
Other than these two, the new tests were good from a cursory look.
The change to builtin/commit.c looked good, too.
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
