Re: [PATCH v2 000/142] Use the $( ... ) construct for command substitution instead of using the back-quotes
Junio C Hamano writes: > - Nobody has time or energy to go through 140+ patches in one go, >with enough concentration necessary to do so without making >mistakes (this applies to yourself, too---producing mechanical >replacement is a no-cost thing, finding mistakes in mechanical >replacement takes real effort). It's a bit less bad than it seems: 142 files changed, 609 insertions(+), 609 deletions(-) The first pass I did was a very quick one, but I already went through the patches I received (apparently not the whole series) and it wasn't that long. -- 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 1/6] t/perf-lib: factor boilerplate out of test_perf
About half of test_perf() is boilerplate, and half is
actually related to running the perf test. Let's split it
into two functions, so that we can reuse the boilerplate in
future commits.
Signed-off-by: Jeff King
---
t/perf/perf-lib.sh | 61 +++---
1 file changed, 35 insertions(+), 26 deletions(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index a8c9574..20f306a 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -150,8 +150,8 @@ exit $ret' >&3 2>&4
return "$eval_ret"
}
-
-test_perf () {
+test_wrapper_ () {
+ test_wrapper_func_=$1; shift
test_start_
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
@@ -162,35 +162,44 @@ test_perf () {
base=$(basename "$0" .sh)
echo "$test_count" >>"$perf_results_dir"/$base.subtests
echo "$1" >"$perf_results_dir"/$base.$test_count.descr
- if test -z "$verbose"; then
- printf "%s" "perf $test_count - $1:"
- else
- echo "perf $test_count - $1:"
- fi
- for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
- say >&3 "running: $2"
- if test_run_perf_ "$2"
- then
- if test -z "$verbose"; then
- printf " %s" "$i"
- else
- echo "* timing run
$i/$GIT_PERF_REPEAT_COUNT:"
- fi
+ base="$perf_results_dir"/"$perf_results_prefix$(basename "$0"
.sh)"."$test_count"
+ "$test_wrapper_func_" "$@"
+ fi
+
+ test_finish_
+}
+
+test_perf_ () {
+ if test -z "$verbose"; then
+ printf "%s" "perf $test_count - $1:"
+ else
+ echo "perf $test_count - $1:"
+ fi
+ for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
+ say >&3 "running: $2"
+ if test_run_perf_ "$2"
+ then
+ if test -z "$verbose"; then
+ printf " %s" "$i"
else
- test -z "$verbose" && echo
- test_failure_ "$@"
- break
+ echo "* timing run $i/$GIT_PERF_REPEAT_COUNT:"
fi
- done
- if test -z "$verbose"; then
- echo " ok"
else
- test_ok_ "$1"
+ test -z "$verbose" && echo
+ test_failure_ "$@"
+ break
fi
- base="$perf_results_dir"/"$perf_results_prefix$(basename "$0"
.sh)"."$test_count"
- "$TEST_DIRECTORY"/perf/min_time.perl test_time.* >"$base".times
+ done
+ if test -z "$verbose"; then
+ echo " ok"
+ else
+ test_ok_ "$1"
fi
- test_finish_
+ "$TEST_DIRECTORY"/perf/min_time.perl test_time.* >"$base".times
+}
+
+test_perf () {
+ test_wrapper_ test_perf_ "$@"
}
# We extend test_done to print timings at the end (./run disables this
--
1.9.1.601.g7ec968e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/6] t/perf: add perf tests for fetches from a bitmapped server
A server with bitmapped packs can serve a clone very
quickly. However, fetches are not necessarily made any
faster, because we spend a lot less time in object traversal
(which is what bitmaps help with) and more time finding
deltas (because we may have to throw out on-disk deltas if
the client does not have the other side).
As a first step to making this faster, this patch introduces
a new perf script to measure fetches into a repo of various
ages from a fully-bitmapped server.
We separately measure the work done by the server (in
pack-objects) and that done by the client (in index-pack).
Furthermore, we measure the size of the resulting pack.
Breaking it down like this (instead of just doing a regular
"git fetch") lets us see how much each side benefits from
any changes. And since we know the pack size, if we estimate
the network speed, then we can calculate a complete
wall-clock time for the operation.
Signed-off-by: Jeff King
---
t/perf/p5311-pack-bitmaps-fetch.sh | 45 ++
1 file changed, 45 insertions(+)
create mode 100755 t/perf/p5311-pack-bitmaps-fetch.sh
diff --git a/t/perf/p5311-pack-bitmaps-fetch.sh
b/t/perf/p5311-pack-bitmaps-fetch.sh
new file mode 100755
index 000..b045759
--- /dev/null
+++ b/t/perf/p5311-pack-bitmaps-fetch.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='performance of fetches from bitmapped packs'
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+test_expect_success 'create bitmapped server repo' '
+ git config pack.writebitmaps true &&
+ git config pack.writebitmaphashcache true &&
+ git repack -ad
+'
+
+# simulate a fetch from a repository that last fetched N days ago, for
+# various values of N. We do so by following the first-parent chain,
+# and assume the first entry in the chain that is N days older than the current
+# HEAD is where the HEAD would have been then.
+for days in 1 2 4 8 16 32 64 128; do
+ title=$(printf '%10s' "($days days)")
+ test_expect_success "setup revs from $days days ago" '
+ now=$(git log -1 --format=%ct HEAD) &&
+ then=$(($now - ($days * 86400))) &&
+ tip=$(git rev-list -1 --first-parent --until=$then HEAD) &&
+ {
+ echo HEAD &&
+ echo ^$tip
+ } >revs
+ '
+
+ test_perf "server $title" '
+ git pack-objects --stdout --revs \
+--thin --delta-base-offset \
+tmp.pack
+ '
+
+ test_size "size $title" '
+ wc -c http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 0/6] reuse deltas found by bitmaps
[tl;dr the patch is the same as before, but there is a script to measure its effects; please try it out on your repos] This is a continuation of the discussion here: http://thread.gmane.org/gmane.comp.version-control.git/239647 I'll summarize the story so far. Basically, the problem is that while pack bitmaps make the "Counting objects" phase of a fetch fast (which in turn makes clones very fast), they do not help as much for an incremental fetch. One, because we have a lot fewer objects to count, so there is less for us to speed up there. And two, we spend a lot more effort on delta compression for a fetch, because we are often throwing out on-disk deltas that we are not sure that the other side has. One common case is that the server side is mostly packed and has bitmaps, and the fetching client has some subset of its objects, but wants to bring itself up to the tip. Any delta that the server has on disk is generally eligible for reuse, because the base is either: 1. An object that the client already has. 2. An object that we are going to send as part of the new pack. However, we quite often do not notice case (1), because we only consider a subset of the client's objects as "preferred bases" for thin-packing. The reason for this is that traditionally it was expensive to enumerate all of the objects we know the client has. But with bitmaps, this is very cheap. So the basic idea is to better notice which objects the client has and change our delta reuse and preferred-base rules. There are three basic strategies I can think of for doing this: 1. Add every object the client has to the packing list as a preferred base. 2. When considering whether a delta can be reused, check the bitmaps to see if the client has the base. If so, allow reuse. 3. Do (2), but also add the object as a preferred base. I think that option (1) is probably a bad idea; we'll spend a lot of time generating a large packing list, and the huge number of objects will probably clog the delta window. Option (3) might or might not be a good idea. It would hopefully give us some relevant subset of the objects to use as preferred bases, in case other objects also need deltas. The implementation I'm including here is the one I've shown before, which does (2). Part of the reason that I'm reposting it before looking further into these options is that I've written a t/perf script that can help with the analysis. I've run it against git.git and linux.git. I'd be curious to see the results on other repositories that might have different delta patterns. The script simulates a fetch from a fully-packed server by a client who has not fetched in N days, for several values of N. It measures the time taken on the server (to run pack-objects), the time taken on the client (to run index-pack), and the size of the resulting pack. Here are the results for running it on linux.git (note that the script output has the tests interleaved, but I've rearranged it here for clarity): Test origin HEAD --- 5311.4: size (1 days) 1.0M 59.5K -94.1% 5311.8: size (2 days) 1.0M 59.5K -94.1% 5311.12: size (4 days) 1.1M 67.9K -93.5% 5311.16: size (8 days) 1.5M 130.1K -91.4% 5311.20: size(16 days) 3.7M 359.8K -90.3% 5311.24: size(32 days) 8.6M 1.4M -84.3% 5311.28: size(64 days) 68.3M 23.0M -66.3% 5311.32: size (128 days) 83.1M 35.1M -57.7% You can see that it produces much smaller packs, because we're getting better delta reuse (and bitmaps don't generally produce preferred bases at all, so we were probably failing to make thin packs at all). The percentage benefit lessens as we go further back, of course, because the thin objects will be at the "edge" of the pack (and the bigger the pack, the less of it is edge). So far so good...here are the server timings: Test origin HEAD --- 5311.3: server (1 days)0.29(0.33+0.03)0.14(0.11+0.02) -51.7% 5311.7: server (2 days)0.29(0.33+0.04)0.14(0.11+0.02) -51.7% 5311.11: server (4 days) 0.29(0.32+0.04)0.14(0.11+0.02) -51.7% 5311.15: server (8 days) 0.36(0.45+0.04)0.14(0.11+0.02) -61.1% 5311.19: server (16 days) 0.74(1.17+0.05)0.26(0.23+0.02) -64.9% 5311.23: server (32 days) 1.38(2.53+0.06)0.29(0.25+0.03) -79.0% 5311.27: server (64 days) 7.12(15.70+0.18) 0.43(0.38+0.07) -94.0% 5311.31: server (128 days) 7.60(16.72+0.19) 0.52(0.48+0.07) -93.2% Again, looking good. We'r
[PATCH 3/6] t/perf: add infrastructure for measuring sizes
The main objective of scripts in the perf framework is to
run "test_perf", which measures the time it takes to run
some operation. However, it can also be interesting to see
the change in the output size of certain operations.
This patch introduces test_size, which records a single
numeric output from the test and shows it in the aggregated
output (with pretty printing and relative size comparison).
Signed-off-by: Jeff King
---
t/perf/README | 20
t/perf/aggregate.perl | 48 +++-
t/perf/perf-lib.sh| 13 +
3 files changed, 76 insertions(+), 5 deletions(-)
diff --git a/t/perf/README b/t/perf/README
index 8848c14..09c400f 100644
--- a/t/perf/README
+++ b/t/perf/README
@@ -144,3 +144,23 @@ that
While we have tried to make sure that it can cope with embedded
whitespace and other special characters, it will not work with
multi-line data.
+
+Rather than tracking the performance by run-time as `test_perf` does, you
+may also track output size by using `test_size`. The stdout of the
+function should be a single numeric value, which will be captured and
+shown in the aggregated output. For example:
+
+ test_perf 'time foo' '
+ ./foo >foo.out
+ '
+
+ test_size 'output size'
+ wc -c ;
return undef if not defined $line;
close $fh or die "cannot close $name: $!";
- $line =~ /^(?:(\d+):)?(\d+):(\d+(?:\.\d+)?) (\d+(?:\.\d+)?)
(\d+(?:\.\d+)?)$/
- or die "bad input line: $line";
- my $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;
- return ($rt, $4, $5);
+ # times
+ if ($line =~ /^(?:(\d+):)?(\d+):(\d+(?:\.\d+)?) (\d+(?:\.\d+)?)
(\d+(?:\.\d+)?)$/) {
+ my $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;
+ return ($rt, $4, $5);
+ # size
+ } elsif ($line =~ /^\d+$/) {
+ return $&;
+ } else {
+ die "bad input line: $line";
+ }
}
sub relative_change {
@@ -29,14 +35,39 @@ sub relative_change {
sub format_times {
my ($r, $u, $s, $firstr) = @_;
+ # no value means we did not finish the test
if (!defined $r) {
return "";
}
+ # a single value means we have a size, not times
+ if (!defined $u) {
+ return format_size($r, $firstr);
+ }
+ # otherwise, we have real/user/system times
my $out = sprintf "%.2f(%.2f+%.2f)", $r, $u, $s;
$out .= ' ' . relative_change($r, $firstr) if defined $firstr;
return $out;
}
+sub human_size {
+ my $n = shift;
+ my @units = ('', qw(K M G));
+ while ($n > 900 && @units > 1) {
+ $n /= 1000;
+ shift @units;
+ }
+ return $n unless length $units[0];
+ return sprintf '%.1f%s', $n, $units[0];
+}
+
+sub format_size {
+ my ($size, $first) = @_;
+ # match the width of a time: 0.00(0.00+0.00)
+ my $out = sprintf '%15s', human_size($size);
+ $out .= ' ' . relative_change($size, $first) if defined $first;
+ return $out;
+}
+
my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests);
while (scalar @ARGV) {
my $arg = $ARGV[0];
@@ -139,7 +170,14 @@ sub have_slash {
my $firstr;
for my $i (0..$#dirs) {
my $d = $dirs[$i];
- $times{$prefixes{$d}.$t} =
[get_times("test-results/$prefixes{$d}$t.times")];
+ my $base = "test-results/$prefixes{$d}$t";
+ $times{$prefixes{$d}.$t} = [];
+ foreach my $type (qw(times size)) {
+ if (-e "$base.$type") {
+ $times{$prefixes{$d}.$t} =
[get_times("$base.$type")];
+ last;
+ }
+ }
my ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};
my $w = length format_times($r,$u,$s,$firstr);
$colwidth[$i] = $w if $w > $colwidth[$i];
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 20f306a..fb8e017 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -202,6 +202,19 @@ test_perf () {
test_wrapper_ test_perf_ "$@"
}
+test_size_ () {
+ say >&3 "running: $2"
+ if test_eval_ "$2" 3>"$base".size; then
+ test_ok_ "$1"
+ else
+ test_failure_ "$@"
+ fi
+}
+
+test_size () {
+ test_wrapper_ test_size_ "$@"
+}
+
# We extend test_done to print timings at the end (./run disables this
# and does it after running everything)
test_at_end_hook_ () {
--
1.9.1.601.g7ec968e
--
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/6] t/perf/aggregate: factor our percent calculations
This will let us reuse the code when we add new values to
aggregate besides times.
Signed-off-by: Jeff King
---
t/perf/aggregate.perl | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 15f7fc1..690cd8c 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -16,21 +16,24 @@ sub get_times {
return ($rt, $4, $5);
}
+sub relative_change {
+ my ($r, $firstr) = @_;
+ if ($firstr > 0) {
+ return sprintf "%+.1f%%", 100.0*($r-$firstr)/$firstr;
+ } elsif ($r == 0) {
+ return "=";
+ } else {
+ return "+inf";
+ }
+}
+
sub format_times {
my ($r, $u, $s, $firstr) = @_;
if (!defined $r) {
return "";
}
my $out = sprintf "%.2f(%.2f+%.2f)", $r, $u, $s;
- if (defined $firstr) {
- if ($firstr > 0) {
- $out .= sprintf " %+.1f%%", 100.0*($r-$firstr)/$firstr;
- } elsif ($r == 0) {
- $out .= " =";
- } else {
- $out .= " +inf";
- }
- }
+ $out .= ' ' . relative_change($r, $firstr) if defined $firstr;
return $out;
}
--
1.9.1.601.g7ec968e
--
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 5/6] pack-bitmap: save "have" bitmap from walk
When we do a bitmap walk, we save the result, which
represents (WANTs & ~HAVEs); i.e., every object we care
about visiting in our walk. However, we throw away the
haves bitmap, which can sometimes be useful, too. Save it
and provide an access function so code which has performed a
walk can query it.
Signed-off-by: Jeff King
---
pack-bitmap.c | 21 -
pack-bitmap.h | 2 ++
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index a31e529..f7d417b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -86,6 +86,9 @@ static struct bitmap_index {
/* Bitmap result of the last performed walk */
struct bitmap *result;
+ /* "have" bitmap from the last performed walk */
+ struct bitmap *haves;
+
/* Version of the bitmap index */
unsigned int version;
@@ -769,8 +772,8 @@ int prepare_bitmap_walk(struct rev_info *revs)
bitmap_and_not(wants_bitmap, haves_bitmap);
bitmap_git.result = wants_bitmap;
+ bitmap_git.haves = haves_bitmap;
- bitmap_free(haves_bitmap);
return 0;
}
@@ -1097,3 +1100,19 @@ int rebuild_existing_bitmaps(struct packing_data
*mapping,
bitmap_free(rebuild);
return 0;
}
+
+int bitmap_have(const unsigned char *sha1)
+{
+ int pos;
+
+ if (!bitmap_git.loaded)
+ return 0; /* no bitmap loaded */
+ if (!bitmap_git.haves)
+ return 0; /* walk had no "haves" */
+
+ pos = bitmap_position_packfile(sha1);
+ if (pos < 0)
+ return 0;
+
+ return bitmap_get(bitmap_git.haves, pos);
+}
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 8b7f4e9..a63ee6b 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -49,6 +49,8 @@ int prepare_bitmap_walk(struct rev_info *revs);
int reuse_partial_packfile_from_bitmap(struct packed_git **packfile, uint32_t
*entries, off_t *up_to);
int rebuild_existing_bitmaps(struct packing_data *mapping, khash_sha1
*reused_bitmaps, int show_progress);
+int bitmap_have(const unsigned char *sha1);
+
void bitmap_writer_show_progress(int show);
void bitmap_writer_set_checksum(unsigned char *sha1);
void bitmap_writer_build_type_index(struct pack_idx_entry **index, uint32_t
index_nr);
--
1.9.1.601.g7ec968e
--
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 6/6] pack-objects: reuse deltas for thin "have" objects
When we calculate the "wants" and "haves" for a pack, we
only add the objects in the boundary commits as preferred
bases. However, we know that every object reachable from the
"haves" could be a preferred base.
We probably don't want to add these to our preferred base
list, because they would clog the delta-search window.
However, there is no reason we cannot reuse an on-disk delta
against such a deep "have" base, avoiding the delta search
for that object altogether.
Signed-off-by: Jeff King
---
builtin/pack-objects.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 7950c43..92bd682 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -53,6 +53,7 @@ static unsigned long pack_size_limit;
static int depth = 50;
static int delta_search_threads;
static int pack_to_stdout;
+static int thin = 0;
static int num_preferred_base;
static struct progress *progress_state;
static int pack_compression_level = Z_DEFAULT_COMPRESSION;
@@ -1419,6 +1420,20 @@ static void check_object(struct object_entry *entry)
base_entry->delta_child = entry;
unuse_pack(&w_curs);
return;
+ } else if(thin && base_ref && bitmap_have(base_ref)) {
+ entry->type = entry->in_pack_type;
+ entry->delta_size = entry->size;
+ /*
+* XXX we'll leak this, but it's probably OK
+* since we'll exit immediately after the packing
+* is done
+*/
+ entry->delta = xcalloc(1, sizeof(*entry->delta));
+ hashcpy(entry->delta->idx.sha1, base_ref);
+ entry->delta->preferred_base = 1;
+ entry->delta->filled = 1;
+ unuse_pack(&w_curs);
+ return;
}
if (entry->type) {
@@ -2559,7 +2574,6 @@ static int option_parse_ulong(const struct option *opt,
int cmd_pack_objects(int argc, const char **argv, const char *prefix)
{
int use_internal_rev_list = 0;
- int thin = 0;
int all_progress_implied = 0;
const char *rp_av[6];
int rp_ac = 0;
--
1.9.1.601.g7ec968e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 000/142] Use the $( ... ) construct for command substitution instead of using the back-quotes
On Wed, Mar 26, 2014 at 3:02 AM, Matthieu Moy wrote: > Junio C Hamano writes: > >> - Nobody has time or energy to go through 140+ patches in one go, >>with enough concentration necessary to do so without making >>mistakes (this applies to yourself, too---producing mechanical >>replacement is a no-cost thing, finding mistakes in mechanical >>replacement takes real effort). > > It's a bit less bad than it seems: > > 142 files changed, 609 insertions(+), 609 deletions(-) > > The first pass I did was a very quick one, but I already went through > the patches I received (apparently not the whole series) and it wasn't > that long. I also gave v2 a read-through. Nothing jumped out as a red-flag. -- 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: [RFC/PATCH 1/4] test-lib: add test_dir_is_empty()
Am 25.03.2014 21:49, schrieb Junio C Hamano:
> Jens Lehmann writes:
>
>> For the upcoming submodule test framework we often need to assert that an
>> empty directory exists in the work tree. Add the test_dir_is_empty()
>> function which asserts that the given argument is an empty directory.
>>
>> Signed-off-by: Jens Lehmann
>> ---
>>
>> I believe this one is pretty straightforward (unless I missed that this
>> functionality already exists someplace I forgot to look ;-).
>
> I am not very thrilled to see that it depends on "." and ".." to
> always exist, which may be true for all POSIX filesystems, but
> still...
Agreed. I didn't find any one-liners to do that ("ls -A" isn't
POSIX), so I decided to wrap that in a function. Testing that
"rmdir" on the directory succeeds (because the directory is
empty) would kinda work, but then we'd have to re-create the
directory afterwards, which really doesn't sound like a good
strategy either as the test would manipulate the to-be-tested
object. I'm not terribly happy with depending on "." and ".."
either, but couldn't come up with something better. At least
the test should fail for any filesystem not having the dot
files ...
> Do expected callsites of this helper care if "$1" is a symbolic link
> that points at an empty directory?
Yep, a symbolic link pointing to an empty directory should make
the test fail.
> What do expected callsites really want to ensure? In other words,
> why do they care if the directory is empty? Is it to make sure,
> after some operation, they can "rmdir" the directory?
To assert that a submodule is created but *not* populated. This
is intended to catch any possible fallout from the recursive
checkout later, in case that would kick in when it shouldn't.
>> t/test-lib-functions.sh | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index 158e10a..93d10cd 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -489,6 +489,17 @@ test_path_is_dir () {
>> fi
>> }
>>
>> +# Check if the directory exists and is empty as expected, barf otherwise.
>> +test_dir_is_empty () {
>> +test_path_is_dir "$1" &&
>> +if test $(ls -a1 "$1" | wc -l) != 2
>> +then
>> +echo "Directory '$1' is not empty, it contains:"
>> +ls -la "$1"
>> +return 1
>> +fi
>> +}
>> +
>> test_path_is_missing () {
>> if [ -e "$1" ]
>> then
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Enable index-pack threading in msysgit.
Am 3/19/2014 1:46, schrieb sza...@chromium.org:
> This adds a Windows implementation of pread. Note that it is NOT
> safe to intersperse calls to read() and pread() on a file
> descriptor. According to the ReadFile spec, using the 'overlapped'
> argument should not affect the implicit position pointer of the
> descriptor. Experiments have shown that this is, in fact, a lie.
>
> To accomodate that fact, this change also incorporates:
>
> http://article.gmane.org/gmane.comp.version-control.git/196042
>
> which gives each index-pack thread its own file descriptor.
> ---
> builtin/index-pack.c | 21 -
> compat/mingw.c | 31 ++-
> compat/mingw.h | 3 +++
> config.mak.uname | 1 -
> 4 files changed, 49 insertions(+), 7 deletions(-)
t5302 does not pass with this patch (sz/mingw-index-pack-threaded).
It fails like this:
+ eval 'git index-pack --index-version=1 --stdin < "test-1-${pack1}.pack" &&
git prune-packed &&
git count-objects | ( read nr rest && test "$nr" -eq 1 ) &&
cmp "test-1-${pack1}.pack" ".git/objects/pack/pack-${pack1}.pack" &&
cmp "test-1-${pack1}.idx" ".git/objects/pack/pack-${pack1}.idx"'
++ git index-pack --index-version=1 --stdin
pack1c54d893dd9bf6645ecee2886ea72f2c2030bea1
++ git prune-packed
error: packfile
.git/objects/pack/pack-1c54d893dd9bf6645ecee2886ea72f2c2030bea1.pack does not
match index
warning: packfile
.git/objects/pack/pack-1c54d893dd9bf6645ecee2886ea72f2c2030bea1.pack cannot be
accessed
[... these 2 messages repeat ~250 times ...]
++ git count-objects
++ read nr rest
++ test 303 -eq 1
I haven't tested Duy's latest patch (index-pack: work around
thread-unsafe pread() yesterday), yet.
-- Hannes
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 2f37a38..c02dd4c 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -51,6 +51,7 @@ struct thread_local {
> #endif
> struct base_data *base_cache;
> size_t base_cache_used;
> + int pack_fd;
> };
>
> /*
> @@ -91,7 +92,8 @@ static off_t consumed_bytes;
> static unsigned deepest_delta;
> static git_SHA_CTX input_ctx;
> static uint32_t input_crc32;
> -static int input_fd, output_fd, pack_fd;
> +static const char *curr_pack;
> +static int input_fd, output_fd;
>
> #ifndef NO_PTHREADS
>
> @@ -134,6 +136,7 @@ static inline void unlock_mutex(pthread_mutex_t *mutex)
> */
> static void init_thread(void)
> {
> + int i;
> init_recursive_mutex(&read_mutex);
> pthread_mutex_init(&counter_mutex, NULL);
> pthread_mutex_init(&work_mutex, NULL);
> @@ -141,11 +144,17 @@ static void init_thread(void)
> pthread_mutex_init(&deepest_delta_mutex, NULL);
> pthread_key_create(&key, NULL);
> thread_data = xcalloc(nr_threads, sizeof(*thread_data));
> + for (i = 0; i < nr_threads; i++) {
> + thread_data[i].pack_fd = open(curr_pack, O_RDONLY);
> + if (thread_data[i].pack_fd == -1)
> + die_errno("unable to open %s", curr_pack);
> + }
> threads_active = 1;
> }
>
> static void cleanup_thread(void)
> {
> + int i;
> if (!threads_active)
> return;
> threads_active = 0;
> @@ -155,6 +164,8 @@ static void cleanup_thread(void)
> if (show_stat)
> pthread_mutex_destroy(&deepest_delta_mutex);
> pthread_key_delete(key);
> + for (i = 0; i < nr_threads; i++)
> + close(thread_data[i].pack_fd);
> free(thread_data);
> }
>
> @@ -288,13 +299,13 @@ static const char *open_pack_file(const char *pack_name)
> output_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR,
> 0600);
> if (output_fd < 0)
> die_errno(_("unable to create '%s'"), pack_name);
> - pack_fd = output_fd;
> + nothread_data.pack_fd = output_fd;
> } else {
> input_fd = open(pack_name, O_RDONLY);
> if (input_fd < 0)
> die_errno(_("cannot open packfile '%s'"), pack_name);
> output_fd = -1;
> - pack_fd = input_fd;
> + nothread_data.pack_fd = input_fd;
> }
> git_SHA1_Init(&input_ctx);
> return pack_name;
> @@ -542,7 +553,7 @@ static void *unpack_data(struct object_entry *obj,
>
> do {
> ssize_t n = (len < 64*1024) ? len : 64*1024;
> - n = pread(pack_fd, inbuf, n, from);
> + n = pread(get_thread_data()->pack_fd, inbuf, n, from);
> if (n < 0)
> die_errno(_("cannot pread pack file"));
> if (!n)
> @@ -1490,7 +1501,7 @@ static void show_pack_info(int stat_only)
> int cmd_index_pack(int argc, const char **argv, const char *prefix)
> {
> int i, fix_thin_pack = 0, verify = 0, stat_only = 0;
> - const char *curr_pack, *curr_index;
> + const char *curr_index;
> const char *index_n
Re: What's cooking in git.git (Mar 2014, #06; Tue, 25)
HI, On Tue, Mar 25, 2014 at 9:18 PM, Junio C Hamano wrote: > > * cc/interpret-trailers (2014-03-07) 11 commits > - Documentation: add documentation for 'git interpret-trailers' > - trailer: add tests for commands in config file > - trailer: execute command from 'trailer..command' > - trailer: add tests for "git interpret-trailers" > - trailer: add interpret-trailers command > - trailer: put all the processing together and print > - trailer: parse trailers from stdin > - trailer: process command line trailer arguments > - trailer: read and process config information > - trailer: process trailers from stdin and arguments > - trailers: add data structures and basic functions > > A new filter to programatically edit the tail end of the commit log > messages. > > Will merge to 'next'. I have a new version of this series, but I am not very happy with it. Especially, I am not very happy about not using strcasecmp() and instead lowercasing some parameters read from the config file. That's because it adds some lines and one more patch as I feel that I have to refactor lowercasing functions. I will send this new series soon anyway and we can choose if we want these lowercasing changes or not. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 000/142] Use the $( ... ) construct for command substitution instead of using the back-quotes
2014-03-26 8:44 GMT+01:00 Eric Sunshine : > On Wed, Mar 26, 2014 at 3:02 AM, Matthieu Moy > wrote: >> Junio C Hamano writes: >> >>> - Nobody has time or energy to go through 140+ patches in one go, >>>with enough concentration necessary to do so without making >>>mistakes (this applies to yourself, too---producing mechanical >>>replacement is a no-cost thing, finding mistakes in mechanical >>>replacement takes real effort). >> >> It's a bit less bad than it seems: >> >> 142 files changed, 609 insertions(+), 609 deletions(-) >> >> The first pass I did was a very quick one, but I already went through >> the patches I received (apparently not the whole series) and it wasn't >> that long. > > I also gave v2 a read-through. Nothing jumped out as a red-flag. Thank you all. I will change the comment as the maintainer wishes (75ee3d7078fbcc3b87a3ae5e0cf42f46256c5da4 for example) and send the other patches slowly (10 at a time every week, boring but that's okay). Best Regards -- 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: [RFC/PATCH 1/4] test-lib: add test_dir_is_empty()
On 03/26/2014 09:29 AM, Jens Lehmann wrote:
> Am 25.03.2014 21:49, schrieb Junio C Hamano:
>> Jens Lehmann writes:
>>
>>> For the upcoming submodule test framework we often need to assert that an
>>> empty directory exists in the work tree. Add the test_dir_is_empty()
>>> function which asserts that the given argument is an empty directory.
>>>
>>> Signed-off-by: Jens Lehmann
>>> ---
>>>
>>> I believe this one is pretty straightforward (unless I missed that this
>>> functionality already exists someplace I forgot to look ;-).
>>
>> I am not very thrilled to see that it depends on "." and ".." to
>> always exist, which may be true for all POSIX filesystems, but
>> still...
>
> Agreed. I didn't find any one-liners to do that ("ls -A" isn't
> POSIX), so I decided to wrap that in a function. Testing that
> "rmdir" on the directory succeeds (because the directory is
> empty) would kinda work, but then we'd have to re-create the
> directory afterwards, which really doesn't sound like a good
> strategy either as the test would manipulate the to-be-tested
> object. I'm not terribly happy with depending on "." and ".."
> either, but couldn't come up with something better. At least
> the test should fail for any filesystem not having the dot
> files ...
>
>> Do expected callsites of this helper care if "$1" is a symbolic link
>> that points at an empty directory?
>
> Yep, a symbolic link pointing to an empty directory should make
> the test fail.
>
>> What do expected callsites really want to ensure? In other words,
>> why do they care if the directory is empty? Is it to make sure,
>> after some operation, they can "rmdir" the directory?
>
> To assert that a submodule is created but *not* populated. This
> is intended to catch any possible fallout from the recursive
> checkout later, in case that would kick in when it shouldn't.
>
>>> t/test-lib-functions.sh | 11 +++
>>> 1 file changed, 11 insertions(+)
>>>
>>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>>> index 158e10a..93d10cd 100644
>>> --- a/t/test-lib-functions.sh
>>> +++ b/t/test-lib-functions.sh
>>> @@ -489,6 +489,17 @@ test_path_is_dir () {
>>> fi
>>> }
>>>
>>> +# Check if the directory exists and is empty as expected, barf otherwise.
>>> +test_dir_is_empty () {
>>> + test_path_is_dir "$1" &&
>>> + if test $(ls -a1 "$1" | wc -l) != 2
>>> + then
>>> + echo "Directory '$1' is not empty, it contains:"
>>> + ls -la "$1"
>>> + return 1
>>> + fi
>>> +}
>>> +
>>> test_path_is_missing () {
>>> if [ -e "$1" ]
>>> then
Why not do something like
test -z "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
I.e., make the test ignore "." and ".." without depending on their
existence?
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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 5/5] log: do not segfault on gmtime errors
On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote: > +# date is within 2^63-1, but enough to choke glibc's gmtime > +test_expect_success 'absurdly far-in-future dates produce sentinel' ' > + commit=$(munge_author_date HEAD 99) && > + echo "Thu Jan 1 00:00:00 1970 +" >expect && > + git log -1 --format=%ad $commit >actual && > + test_cmp expect actual > +' Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 162396404 -0700. I don't know if this is correct for the given input but the test fails. Charles. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Junio C Hamano writes:
> Elia Pinto writes:
>
>> The Git CodingGuidelines prefer the $( ... ) construct for command
>> substitution instead of using the back-quotes, or grave accents (`..`).
>>
>> The backquoted form is the historical method for command substitution,
>> and is supported by POSIX. However, all but the simplest uses become
>> complicated quickly. In particular, embedded command substitutions
>> and/or the use of double quotes require careful escaping with the backslash
>> character. Because of this the POSIX shell adopted the $(…) feature from
>> the Korn shell.
>>
>> The patch was generated by the simple script
>>
>> for _f in $(find . -name "*.sh")
>> do
>> sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
>> done
>
> "and then carefully proofread" is sorely needed here.
It would already help to skip comment lines.
--
David Kastrup
--
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: [RFC/PATCH 1/4] test-lib: add test_dir_is_empty()
Junio C Hamano writes: > Jens Lehmann writes: > >> For the upcoming submodule test framework we often need to assert that an >> empty directory exists in the work tree. Add the test_dir_is_empty() >> function which asserts that the given argument is an empty directory. >> >> Signed-off-by: Jens Lehmann >> --- >> >> I believe this one is pretty straightforward (unless I missed that this >> functionality already exists someplace I forgot to look ;-). > > I am not very thrilled to see that it depends on "." and ".." to > always exist, which may be true for all POSIX filesystems, but > still... Not even there, though few people will likely use / as their work tree... -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
David Kastrup writes:
> Junio C Hamano writes:
>
>> Elia Pinto writes:
>>
>>> The Git CodingGuidelines prefer the $( ... ) construct for command
>>> substitution instead of using the back-quotes, or grave accents (`..`).
>>>
>>> The backquoted form is the historical method for command substitution,
>>> and is supported by POSIX. However, all but the simplest uses become
>>> complicated quickly. In particular, embedded command substitutions
>>> and/or the use of double quotes require careful escaping with the backslash
>>> character. Because of this the POSIX shell adopted the $(…) feature from
>>> the Korn shell.
>>>
>>> The patch was generated by the simple script
>>>
>>> for _f in $(find . -name "*.sh")
>>> do
>>> sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
>>> done
>>
>> "and then carefully proofread" is sorely needed here.
>
> It would already help to skip comment lines.
Actually no: most comments including `...` are code examples that we
want to fix too, except 3 instances (see my other message).
--
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
Re: Borrowing objects from nearby repositories
On Mar 25, 2014, at 6:17 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Ævar Arnfjörð Bjarmason writes: >> 1) Introduce '--borrow' to `git-fetch`. This would behave similarly >>> to '--reference', except that it operates on a temporary basis, and >>> does not assume that the reference repository will exist after the >>> operation completes, so any used objects are copied into the local >>> objects database. In theory, this mechanism would be distinct from >>> --reference', so if both are used, some objects would be copied, and >>> some objects would be accessible via a reference repository referenced >>> by the alternates file. >>> >>> Isn't this the same as git clone --reference --no-hardlinks >>> ? >>> >>> Also without --no-hardlinks we're not assuming that the other repo >>> doesn't go away (you could rm-rf it), just that the files won't be >>> *modified*, which Git won't do, but you could manually do with other >>> tools, so the default is to hardlink. >> >> I think that the standard practice with the existing toolset is to >> clone with reference and then repack. That is: >> >>$ git clone --reference git://over/there mine >>$ cd mine >>$ git repack -a -d >> >> And then you can try this: >> >>$ mv .git/objects/info/alternates .git/objects/info/alternates.disabled >>$ git fsck >> >> to make sure that you are no longer borrowing anything from the >> borrowee. Once you are satisfied, you can remove the saved-away >> alternates.disabled file. > > Oh, I forgot to say that I am not opposed if somebody wants to teach > "git clone" a new option to copy its objects from two places, > (hopefully) the majority from near-by reference repository and the > remainder over the network, without permanently relying on the > former via the alternates mechanism. The implementation of such a > feature could even literally be "clone with reference first and then > repack" at least initially but even in the final version. That was actually one of my first ideas - adding some sort of '--auto-repack' option to git-clone. It's a relatively small change, and would work. However, keeping in mind my end goal of automating the feature to the point where you could run simply 'git clone ', an '--auto-repack' option is more difficult to undo. You would need a new parameter to disable the automatic adding of reference repositories, and a new parameter to undo '--auto-repack', and you'd have to remember to actually undo both of those settings. In contrast, if the new feature was '--borrow', and the evolution of the feature was a global configuration 'fetch.autoBorrow', then to turn it off temporarily, one only needs a single new parameter '--no-auto-borrow'. I think this is a cleaner approach than the former, although much more work. Thanks, - Andrew Keller -- 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 v2 00/17] git-ls
Compared to v1, git-ls now does not accept ls-files options (previous
git-ls is more like an alias of ls-files). I want this because ls may
take a different set of options than ls-files. Most functionality is
shared so if you're not happy with ls, you can fall back to ls-files.
New alias options are supported, -1 == --no-column, -R ==
--max-depth=-1. If more than one file criteria is chosen (e.g. "ls -cmo")
then --tag is implied. File order is fixed ("ls-files -cmo" actually
shows two or three separate listings, "ls -cmo" shows one sorted
listing). It also shows directories from the index.
Documentation is there. No tests yet because the behavior may still
need some polishing.
Nguyễn Thái Ngọc Duy (17):
ls_colors.c: add $LS_COLORS parsing code
ls_colors.c: parse color.ls.* from config file
ls_colors.c: add function to color a file name
ls_colors.c: highlight submodules like directories
ls-files: buffer full item in strbuf before printing
ls-files: add --color to highlight file names
ls-files: add --column
ls-files: support --max-depth
ls-files: split main ls-files logic into ls_files() function
Add git-ls, a user friendly version of ls-files and more
ls: -u does not imply showing stages
ls: add -R/--recursive short for --max-depth=-1
ls: add -1 short for --no-column in the spirit of GNU ls
ls: add -t back
ls: sort output and remove duplicates
ls: do not show duplicate cached entries
ls: show directories as well as files
.gitignore | 1 +
Documentation/config.txt | 22 ++
Documentation/git-ls-files.txt | 22 ++
Documentation/git-ls.txt (new) | 95
Makefile | 2 +
builtin.h | 1 +
builtin/ls-files.c | 446 +---
color.h| 10 +
command-list.txt | 1 +
git.c | 1 +
ls_colors.c (new) | 496 +
11 files changed, 1012 insertions(+), 85 deletions(-)
create mode 100644 Documentation/git-ls.txt
create mode 100644 ls_colors.c
--
1.9.1.345.ga1a145c
--
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 v2 02/17] ls_colors.c: parse color.ls.* from config file
This is the second (and preferred) source for color information. This
will override $LS_COLORS.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/config.txt | 11 +++
ls_colors.c | 26 ++
2 files changed, 37 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 73c8973..3fb754e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -909,6 +909,17 @@ color.status.::
to red). The values of these variables may be specified as in
color.branch..
+color.ls.::
+ Use customized color for file name colorization. If not set
+ and the environment variable LS_COLORS is set, color settings
+ from $LS_COLORS are used. `` can be `normal`, `file`,
+ `directory`, `symlink`, `fifo`, `socket`, `block`, `char`,
+ `missing`, `orphan`, `executable`, `door`, `setuid`, `setgid`,
+ `sticky`, `otherwritable`, `stickyotherwritable`, `cap`,
+ `multihardlink`. The values of these variables may be
+ specified as in color.branch..
+
+
color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/ls_colors.c b/ls_colors.c
index eb944f4..cef5a92 100644
--- a/ls_colors.c
+++ b/ls_colors.c
@@ -68,6 +68,14 @@ static const char *const indicator_name[] = {
NULL
};
+static const char* const config_name[] = {
+ "", "", "", "", "normal", "file", "directory", "symlink",
+ "fifo", "socket", "block", "char", "missing", "orphan", "executable",
+ "door", "setuid", "setgid", "sticky", "otherwritable",
+ "stickyotherwritable", "cap", "multihardlink", "",
+ NULL
+};
+
struct bin_str {
size_t len; /* Number of bytes */
const char *string; /* Pointer to the same */
@@ -285,6 +293,23 @@ static int get_funky_string(char **dest, const char **src,
int equals_end,
return state != ST_ERROR;
}
+static int ls_colors_config(const char *var, const char *value, void *cb)
+{
+ int slot;
+ if (!starts_with(var, "color.ls."))
+ return 0;
+ var += 9;
+ for (slot = 0; config_name[slot]; slot++)
+ if (!strcasecmp(var, config_name[slot]))
+ break;
+ if (!config_name[slot])
+ return 0;
+ if (!value)
+ return config_error_nonbool(var);
+ color_parse(value, var, ls_colors[slot]);
+ return 0;
+}
+
void parse_ls_color(void)
{
const char *p; /* Pointer to character being parsed */
@@ -395,4 +420,5 @@ void parse_ls_color(void)
if (!strcmp(ls_colors[LS_LN], "target"))
color_symlink_as_referent = 1;
+ git_config(ls_colors_config, NULL);
}
--
1.9.1.345.ga1a145c
--
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 v2 04/17] ls_colors.c: highlight submodules like directories
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/config.txt | 3 ++-
ls_colors.c | 8 +++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3fb754e..6bca55e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -913,7 +913,8 @@ color.ls.::
Use customized color for file name colorization. If not set
and the environment variable LS_COLORS is set, color settings
from $LS_COLORS are used. `` can be `normal`, `file`,
- `directory`, `symlink`, `fifo`, `socket`, `block`, `char`,
+ `directory`, `submodule`,
+ `symlink`, `fifo`, `socket`, `block`, `char`,
`missing`, `orphan`, `executable`, `door`, `setuid`, `setgid`,
`sticky`, `otherwritable`, `stickyotherwritable`, `cap`,
`multihardlink`. The values of these variables may be
diff --git a/ls_colors.c b/ls_colors.c
index 1125329..0cc4e9b 100644
--- a/ls_colors.c
+++ b/ls_colors.c
@@ -29,6 +29,8 @@ enum color_ls {
LS_MH, /* multi hardlink */
LS_CL, /* clear end of line */
+ LS_SUBMODULE,
+
MAX_LS
};
@@ -58,7 +60,8 @@ static char ls_colors[MAX_LS][COLOR_MAXLEN] = {
GIT_COLOR_BLACK_ON_GREEN,
"",
"",
- ""
+ "",
+ GIT_COLOR_BOLD_BLUE
};
static const char *const indicator_name[] = {
@@ -73,6 +76,7 @@ static const char* const config_name[] = {
"fifo", "socket", "block", "char", "missing", "orphan", "executable",
"door", "setuid", "setgid", "sticky", "otherwritable",
"stickyotherwritable", "cap", "multihardlink", "",
+ "submodule",
NULL
};
@@ -448,6 +452,8 @@ void color_filename(struct strbuf *sb, const char *name,
type = LS_DI;
} else if (S_ISLNK (mode))
type = (!linkok && *ls_colors[LS_OR]) ? LS_OR : LS_LN;
+ else if (S_ISGITLINK(mode))
+ type = LS_SUBMODULE;
else if (S_ISFIFO (mode))
type = LS_PI;
else if (S_ISSOCK (mode))
--
1.9.1.345.ga1a145c
--
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 v2 01/17] ls_colors.c: add $LS_COLORS parsing code
Reusing color settings from $LS_COLORS could give a native look and
feel on file coloring.
This code is basically from coreutils.git [1], rewritten to fit Git.
As this is from GNU ls, the environment variable CLICOLOR is not
tested. It is to be decided later whether we should ignore $LS_COLORS
if $CLICOLOR is not set on Mac or FreeBSD.
[1] commit 7326d1f1a67edf21947ae98194f98c38b6e9e527 file
src/ls.c. This is the last GPL-2 commit before coreutils turns to
GPL-3.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Makefile | 1 +
color.h | 8 ++
ls_colors.c (new) | 398 ++
3 files changed, 407 insertions(+)
create mode 100644 ls_colors.c
diff --git a/Makefile b/Makefile
index f818eec..f6a6e14 100644
--- a/Makefile
+++ b/Makefile
@@ -819,6 +819,7 @@ LIB_OBJS += list-objects.o
LIB_OBJS += ll-merge.o
LIB_OBJS += lockfile.o
LIB_OBJS += log-tree.o
+LIB_OBJS += ls_colors.o
LIB_OBJS += mailmap.o
LIB_OBJS += match-trees.o
LIB_OBJS += merge.o
diff --git a/color.h b/color.h
index 9a8495b..640fc48 100644
--- a/color.h
+++ b/color.h
@@ -45,6 +45,12 @@ struct strbuf;
#define GIT_COLOR_BG_MAGENTA "\033[45m"
#define GIT_COLOR_BG_CYAN "\033[46m"
+#define GIT_COLOR_WHITE_ON_RED"\033[37;41m"
+#define GIT_COLOR_WHITE_ON_BLUE "\033[37;44m"
+#define GIT_COLOR_BLACK_ON_YELLOW "\033[30;43m"
+#define GIT_COLOR_BLUE_ON_GREEN "\033[34;42m"
+#define GIT_COLOR_BLACK_ON_GREEN "\033[30;42m"
+
/* A special value meaning "no color selected" */
#define GIT_COLOR_NIL "NIL"
@@ -87,4 +93,6 @@ void color_print_strbuf(FILE *fp, const char *color, const
struct strbuf *sb);
int color_is_nil(const char *color);
+void parse_ls_color(void);
+
#endif /* COLOR_H */
diff --git a/ls_colors.c b/ls_colors.c
new file mode 100644
index 000..eb944f4
--- /dev/null
+++ b/ls_colors.c
@@ -0,0 +1,398 @@
+#include "cache.h"
+#include "color.h"
+
+enum color_ls {
+ LS_LC, /* left, unused */
+ LS_RC, /* right, unused */
+ LS_EC, /* end color, unused */
+ LS_RS, /* reset */
+ LS_NO, /* normal */
+ LS_FL, /* file, default */
+ LS_DI, /* directory */
+ LS_LN, /* symlink */
+
+ LS_PI, /* pipe */
+ LS_SO, /* socket */
+ LS_BD, /* block device */
+ LS_CD, /* char device */
+ LS_MI, /* missing file */
+ LS_OR, /* orphaned symlink */
+ LS_EX, /* executable */
+ LS_DO, /* Solaris door */
+
+ LS_SU, /* setuid */
+ LS_SG, /* setgid */
+ LS_ST, /* sticky */
+ LS_OW, /* other-writable */
+ LS_TW, /* ow with sticky */
+ LS_CA, /* cap */
+ LS_MH, /* multi hardlink */
+ LS_CL, /* clear end of line */
+
+ MAX_LS
+};
+
+static char ls_colors[MAX_LS][COLOR_MAXLEN] = {
+ "",
+ "",
+ "",
+ GIT_COLOR_RESET,
+ GIT_COLOR_NORMAL,
+ GIT_COLOR_NORMAL,
+ GIT_COLOR_BOLD_BLUE,
+ GIT_COLOR_BOLD_CYAN,
+
+ GIT_COLOR_YELLOW,
+ GIT_COLOR_BOLD_MAGENTA,
+ GIT_COLOR_BOLD_YELLOW,
+ GIT_COLOR_BOLD_YELLOW,
+ GIT_COLOR_NORMAL,
+ GIT_COLOR_NORMAL,
+ GIT_COLOR_BOLD_GREEN,
+ GIT_COLOR_BOLD_MAGENTA,
+
+ GIT_COLOR_WHITE_ON_RED,
+ GIT_COLOR_BLACK_ON_YELLOW,
+ GIT_COLOR_WHITE_ON_BLUE,
+ GIT_COLOR_BLUE_ON_GREEN,
+ GIT_COLOR_BLACK_ON_GREEN,
+ "",
+ "",
+ ""
+};
+
+static const char *const indicator_name[] = {
+ "lc", "rc", "ec", "rs", "no", "fi", "di", "ln",
+ "pi", "so", "bd", "cd", "mi", "or", "ex", "do",
+ "su", "sg", "st", "ow", "tw", "ca", "mh", "cl",
+ NULL
+};
+
+struct bin_str {
+ size_t len; /* Number of bytes */
+ const char *string; /* Pointer to the same */
+};
+
+struct color_ext_type {
+ struct bin_str ext; /* The extension we're looking for */
+ struct bin_str seq; /* The sequence to output when we do */
+ struct color_ext_type *next;/* Next in list */
+};
+
+static struct color_ext_type *color_ext_list = NULL;
+
+/*
+ * When true, in a color listing, color each symlink name according to the
+ * type of file it points to. Otherwise, color them according to the `ln'
+ * directive in LS_COLORS. Dangling (orphan) symlinks are treated specially,
+ * regardless. This is set when `ln=target' appears in LS_COLORS.
+ */
+static int color_symlink_as_referent;
+
+/*
+ * Parse a string as part of the LS_COLORS variable; this may involve
+ * decoding all kinds of escape characters. If equal
[PATCH v2 06/17] ls-files: add --color to highlight file names
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-ls-files.txt | 9 +
builtin/ls-files.c | 38 +++---
2 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index c0856a6..5c1b7f3 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -147,6 +147,15 @@ a space) at the start of each line:
possible for manual inspection; the exact format may change at
any time.
+--color[=]::
+ Color file names. The value must be always (default), never,
+ or auto.
+
+--no-color::
+ Turn off coloring, even when the configuration file gives the
+ default to color output, same as `--color=never`. This is the
+ default.
+
\--::
Do not interpret any more arguments as options.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 6e30592..2857b38 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
#include "resolve-undo.h"
#include "string-list.h"
#include "pathspec.h"
+#include "color.h"
static int abbrev;
static int show_deleted;
@@ -27,6 +28,7 @@ static int show_killed;
static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
+static int use_color;
static const char *prefix;
static int max_prefix_len;
@@ -60,7 +62,6 @@ static void write_name(struct strbuf *sb, const char *name)
strbuf_release(&sb2);
} else
quote_path_relative(name, real_prefix, sb);
- strbuf_addch(sb, line_terminator);
}
static void strbuf_fputs(struct strbuf *sb, FILE *fp)
@@ -68,6 +69,21 @@ static void strbuf_fputs(struct strbuf *sb, FILE *fp)
fwrite(sb->buf, sb->len, 1, fp);
}
+static void write_dir_entry(struct strbuf *sb, const struct dir_entry *ent)
+{
+ struct strbuf quoted = STRBUF_INIT;
+ struct stat st;
+ if (stat(ent->name, &st))
+ st.st_mode = 0;
+ write_name("ed, ent->name);
+ if (want_color(use_color))
+ color_filename(sb, ent->name, quoted.buf, st.st_mode, 1);
+ else
+ strbuf_addbuf(sb, "ed);
+ strbuf_addch(sb, line_terminator);
+ strbuf_release("ed);
+}
+
static void show_dir_entry(const char *tag, struct dir_entry *ent)
{
static struct strbuf sb = STRBUF_INIT;
@@ -81,7 +97,7 @@ static void show_dir_entry(const char *tag, struct dir_entry
*ent)
strbuf_reset(&sb);
strbuf_addstr(&sb, tag);
- write_name(&sb, ent->name);
+ write_dir_entry(&sb, ent);
strbuf_fputs(&sb, stdout);
}
@@ -146,6 +162,18 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+static void write_ce_name(struct strbuf *sb, const struct cache_entry *ce)
+{
+ struct strbuf quoted = STRBUF_INIT;
+ write_name("ed, ce->name);
+ if (want_color(use_color))
+ color_filename(sb, ce->name, quoted.buf, ce->ce_mode, 1);
+ else
+ strbuf_addbuf(sb, "ed);
+ strbuf_addch(sb, line_terminator);
+ strbuf_release("ed);
+}
+
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
static struct strbuf sb = STRBUF_INIT;
@@ -186,7 +214,7 @@ static void show_ce_entry(const char *tag, const struct
cache_entry *ce)
find_unique_abbrev(ce->sha1,abbrev),
ce_stage(ce));
}
- write_name(&sb, ce->name);
+ write_ce_name(&sb, ce);
strbuf_fputs(&sb, stdout);
if (debug_mode) {
const struct stat_data *sd = &ce->ce_stat_data;
@@ -523,6 +551,7 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
N_("if any is not in the index, treat this as an
error")),
OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
N_("pretend that paths removed since are
still present")),
+ OPT__COLOR(&use_color, N_("show color")),
OPT__ABBREV(&abbrev),
OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
OPT_END()
@@ -570,6 +599,9 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (want_color(use_color))
+ parse_ls_color();
+
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
--
1.9.1.345.ga1a145c
--
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 v2 05/17] ls-files: buffer full item in strbuf before printing
Buffering so that we can manipulate the strings (e.g. coloring)
further before finally printing them.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/ls-files.c | 48 +++-
1 file changed, 35 insertions(+), 13 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 47c3880..6e30592 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -47,18 +47,30 @@ static const char *tag_modified = "";
static const char *tag_skip_worktree = "";
static const char *tag_resolve_undo = "";
-static void write_name(const char *name)
+static void write_name(struct strbuf *sb, const char *name)
{
/*
* With "--full-name", prefix_len=0; this caller needs to pass
* an empty string in that case (a NULL is good for "").
*/
- write_name_quoted_relative(name, prefix_len ? prefix : NULL,
- stdout, line_terminator);
+ const char *real_prefix = prefix_len ? prefix : NULL;
+ if (!line_terminator) {
+ struct strbuf sb2 = STRBUF_INIT;
+ strbuf_addstr(sb, relative_path(name, real_prefix, &sb2));
+ strbuf_release(&sb2);
+ } else
+ quote_path_relative(name, real_prefix, sb);
+ strbuf_addch(sb, line_terminator);
+}
+
+static void strbuf_fputs(struct strbuf *sb, FILE *fp)
+{
+ fwrite(sb->buf, sb->len, 1, fp);
}
static void show_dir_entry(const char *tag, struct dir_entry *ent)
{
+ static struct strbuf sb = STRBUF_INIT;
int len = max_prefix_len;
if (len >= ent->len)
@@ -67,8 +79,10 @@ static void show_dir_entry(const char *tag, struct dir_entry
*ent)
if (!dir_path_match(ent, &pathspec, len, ps_matched))
return;
- fputs(tag, stdout);
- write_name(ent->name);
+ strbuf_reset(&sb);
+ strbuf_addstr(&sb, tag);
+ write_name(&sb, ent->name);
+ strbuf_fputs(&sb, stdout);
}
static void show_other_files(struct dir_struct *dir)
@@ -134,6 +148,7 @@ static void show_killed_files(struct dir_struct *dir)
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
+ static struct strbuf sb = STRBUF_INIT;
int len = max_prefix_len;
if (len >= ce_namelen(ce))
@@ -161,16 +176,18 @@ static void show_ce_entry(const char *tag, const struct
cache_entry *ce)
tag = alttag;
}
+ strbuf_reset(&sb);
if (!show_stage) {
- fputs(tag, stdout);
+ strbuf_addstr(&sb, tag);
} else {
- printf("%s%06o %s %d\t",
- tag,
- ce->ce_mode,
- find_unique_abbrev(ce->sha1,abbrev),
- ce_stage(ce));
+ strbuf_addf(&sb, "%s%06o %s %d\t",
+ tag,
+ ce->ce_mode,
+ find_unique_abbrev(ce->sha1,abbrev),
+ ce_stage(ce));
}
- write_name(ce->name);
+ write_name(&sb, ce->name);
+ strbuf_fputs(&sb, stdout);
if (debug_mode) {
const struct stat_data *sd = &ce->ce_stat_data;
@@ -206,7 +223,12 @@ static void show_ru_info(void)
printf("%s%06o %s %d\t", tag_resolve_undo, ui->mode[i],
find_unique_abbrev(ui->sha1[i], abbrev),
i + 1);
- write_name(path);
+ /*
+* With "--full-name", prefix_len=0; this caller needs
to pass
+* an empty string in that case (a NULL is good for "").
+*/
+ write_name_quoted_relative(path, prefix_len ? prefix :
NULL,
+ stdout, line_terminator);
}
}
}
--
1.9.1.345.ga1a145c
--
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 v2 10/17] Add git-ls, a user friendly version of ls-files and more
This is more user friendly version of ls-files: * it's automatically colored and columnized * it refreshes the index like all porcelain commands * it defaults to non-recursive behavior like ls * :(glob) is on by default so '*.c' means a.c but not a/b.c, use '**/*.c' for that. * auto pager Signed-off-by: Nguyễn Thái Ngọc Duy --- .gitignore | 1 + Documentation/config.txt | 10 ++ Documentation/git-ls.txt (new) | 82 ++ Makefile | 1 + builtin.h | 1 + builtin/ls-files.c | 70 command-list.txt | 1 + git.c | 1 + 8 files changed, 167 insertions(+) create mode 100644 Documentation/git-ls.txt diff --git a/.gitignore b/.gitignore index dc600f9..f91af81 100644 --- a/.gitignore +++ b/.gitignore @@ -76,6 +76,7 @@ /git-init-db /git-instaweb /git-log +/git-ls /git-ls-files /git-ls-remote /git-ls-tree diff --git a/Documentation/config.txt b/Documentation/config.txt index 6bca55e..87a6dcf 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -909,6 +909,12 @@ color.status.:: to red). The values of these variables may be specified as in color.branch.. +color.ls:: + A boolean to enable/disable color in the output of + linkgit:git-ls[1]. May be set to `always`, `false` (or + `never`) or `auto` (or `true`), in which case colors are used + only when the output is to a terminal. Defaults to false. + color.ls.:: Use customized color for file name colorization. If not set and the environment variable LS_COLORS is set, color settings @@ -981,6 +987,10 @@ column.clean:: Specify the layout when list items in `git clean -i`, which always shows files and directories in columns. See `column.ui` for details. +column.ls:: + Specify whether to output tag listing in `git ls` in columns. + See `column.ui` for details. + column.status:: Specify whether to output untracked files in `git status` in columns. See `column.ui` for details. diff --git a/Documentation/git-ls.txt b/Documentation/git-ls.txt new file mode 100644 index 000..67ca522 --- /dev/null +++ b/Documentation/git-ls.txt @@ -0,0 +1,82 @@ +git-ls(1) +=== + +NAME + +git-ls - List files + +SYNOPSIS + +[verse] +'git ls' (--[cached|deleted|others|ignored|unmerged|modified])* + (-[c|d|o|i|s|u|m])* + [options] [...] + +DESCRIPTION +--- +List files (by default in current working directory) that are in the +index. Depending on the chosen options, maybe only modified files in +working tree are shown, or untracked files... + +OPTIONS +--- +-c:: +--cached:: + Show cached files in the output (default) + +-d:: +--deleted:: + Show deleted files in the output + +-m:: +--modified:: + Show modified files in the output + +-o:: +--others:: + Show other (i.e. untracked) files in the output + +-i:: +--ignored:: + Show only ignored files in the output. When showing files in the + index, print only those matched by an exclude pattern. When + showing "other" files, show only those matched by an exclude + pattern. + +-u:: +--unmerged:: + Show unmerged files in the output (forces --stage) + +--color[=]:: + Color file names. The value must be always (default), never, + or auto. + +--no-color:: + Turn off coloring, even when the configuration file gives the + default to color output, same as `--color=never`. This is the + default. + +--column[=]:: +--no-column:: + Display files in columns. See configuration variable column.ui + for option syntax.`--column` and `--no-column` without options + are equivalent to 'always' and 'never' respectively. + +--max-depth :: + For each given on command line, descend at most + levels of directories. A negative value means no limit. + This option is ignored if contains active wildcards. + In other words if "a*" matches a directory named "a*", + "*" is matched literally so --max-depth is still effective. + +:: + Files to show. :(glob) magic is enabled and recursion disabled + by default. + +SEE ALSO + +linkgit:git-ls-files[1] + +GIT +--- +Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index f6a6e14..b0bc40a 100644 --- a/Makefile +++ b/Makefile @@ -584,6 +584,7 @@ BUILT_INS += git-cherry-pick$X BUILT_INS += git-format-patch$X BUILT_INS += git-fsck-objects$X BUILT_INS += git-init$X +BUILT_INS += git-ls$X BUILT_INS += git-merge-subtree$X BUILT_INS += git-show$X BUILT_INS += git-stage$X diff --git a/builtin.h b/builtin.h index c47c110..177aa7d 100644 --- a/builtin.h +++ b/builtin.h @@ -75,6 +75,7 @@ extern int cmd_index_pack(int argc, const char **argv, const char *prefix); extern int cmd_init_db(int argc, con
[PATCH v2 09/17] ls-files: split main ls-files logic into ls_files() function
This is a preparation step for the introduction of git-ls. "git ls"
has a different set of command line options, but it will eventually
call ls_files().
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/ls-files.c | 164 +++--
1 file changed, 85 insertions(+), 79 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 8eef423..20ca3f2 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -31,6 +31,8 @@ static int line_terminator = '\n';
static int debug_mode;
static int use_color;
static unsigned int colopts;
+static int max_depth = -1;
+static int show_tag;
static const char *prefix;
static int max_prefix_len;
@@ -42,6 +44,8 @@ static const char *with_tree;
static int exc_given;
static int exclude_args;
static struct string_list output = STRING_LIST_INIT_NODUP;
+static struct dir_struct dir;
+static struct string_list exclude_list = STRING_LIST_INIT_NODUP;
static const char *tag_cached = "";
static const char *tag_unmerged = "";
@@ -500,89 +504,12 @@ static int option_parse_exclude_standard(const struct
option *opt,
return 0;
}
-int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
+static int ls_files(const char **argv, const char *prefix)
{
- int require_work_tree = 0, show_tag = 0, i;
- int max_depth = -1;
+ int require_work_tree = 0, i;
const char *max_prefix;
- struct dir_struct dir;
struct exclude_list *el;
- struct string_list exclude_list = STRING_LIST_INIT_NODUP;
- struct option builtin_ls_files_options[] = {
- { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
- N_("paths are separated with NUL character"),
- PARSE_OPT_NOARG, option_parse_z },
- OPT_BOOL('t', NULL, &show_tag,
- N_("identify the file status with tags")),
- OPT_BOOL('v', NULL, &show_valid_bit,
- N_("use lowercase letters for 'assume unchanged'
files")),
- OPT_BOOL('c', "cached", &show_cached,
- N_("show cached files in the output (default)")),
- OPT_BOOL('d', "deleted", &show_deleted,
- N_("show deleted files in the output")),
- OPT_BOOL('m', "modified", &show_modified,
- N_("show modified files in the output")),
- OPT_BOOL('o', "others", &show_others,
- N_("show other files in the output")),
- OPT_BIT('i', "ignored", &dir.flags,
- N_("show ignored files in the output"),
- DIR_SHOW_IGNORED),
- OPT_BOOL('s', "stage", &show_stage,
- N_("show staged contents' object name in the output")),
- OPT_BOOL('k', "killed", &show_killed,
- N_("show files on the filesystem that need to be
removed")),
- OPT_BIT(0, "directory", &dir.flags,
- N_("show 'other' directories' name only"),
- DIR_SHOW_OTHER_DIRECTORIES),
- OPT_NEGBIT(0, "empty-directory", &dir.flags,
- N_("don't show empty directories"),
- DIR_HIDE_EMPTY_DIRECTORIES),
- OPT_BOOL('u', "unmerged", &show_unmerged,
- N_("show unmerged files in the output")),
- OPT_BOOL(0, "resolve-undo", &show_resolve_undo,
- N_("show resolve-undo information")),
- { OPTION_CALLBACK, 'x', "exclude", &exclude_list, N_("pattern"),
- N_("skip files matching pattern"),
- 0, option_parse_exclude },
- { OPTION_CALLBACK, 'X', "exclude-from", &dir, N_("file"),
- N_("exclude patterns are read from "),
- 0, option_parse_exclude_from },
- OPT_STRING(0, "exclude-per-directory", &dir.exclude_per_dir,
N_("file"),
- N_("read additional per-directory exclude patterns in
")),
- { OPTION_CALLBACK, 0, "exclude-standard", &dir, NULL,
- N_("add the standard git exclusions"),
- PARSE_OPT_NOARG, option_parse_exclude_standard },
- { OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
- N_("make the output relative to the project top
directory"),
- PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
- OPT_BOOL(0, "error-unmatch", &error_unmatch,
- N_("if any is not in the index, treat this as an
error")),
- OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
- N_("pretend that paths removed since are
still present")),
- OPT__COLOR(&use_color, N_("show color")),
- OPT_COLUMN(0, "column", &colopts, N_("show files in columns")),
-
[PATCH v2 08/17] ls-files: support --max-depth
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-ls-files.txt | 7 +++
builtin/ls-files.c | 7 +++
2 files changed, 14 insertions(+)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index cd52461..3c022eb 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -162,6 +162,13 @@ a space) at the start of each line:
for option syntax.`--column` and `--no-column` without options
are equivalent to 'always' and 'never' respectively.
+--max-depth ::
+ For each given on command line, descend at most
+ levels of directories. A negative value means no limit.
+ This option is ignored if contains active wildcards.
+ In other words if "a*" matches a directory named "a*",
+ "*" is matched literally so --max-depth is still effective.
+
\--::
Do not interpret any more arguments as options.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 335d3b0..8eef423 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -503,6 +503,7 @@ static int option_parse_exclude_standard(const struct
option *opt,
int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{
int require_work_tree = 0, show_tag = 0, i;
+ int max_depth = -1;
const char *max_prefix;
struct dir_struct dir;
struct exclude_list *el;
@@ -560,6 +561,9 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
N_("pretend that paths removed since are
still present")),
OPT__COLOR(&use_color, N_("show color")),
OPT_COLUMN(0, "column", &colopts, N_("show files in columns")),
+ { OPTION_INTEGER, 0, "max-depth", &max_depth, N_("depth"),
+ N_("descend at most levels"), PARSE_OPT_NONEG,
+ NULL, 1 },
OPT__ABBREV(&abbrev),
OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
OPT_END()
@@ -624,8 +628,11 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
+ (max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);
+ pathspec.max_depth = max_depth;
+ pathspec.recursive = 1;
/* Find common prefix for all pathspec's */
max_prefix = common_prefix(&pathspec);
--
1.9.1.345.ga1a145c
--
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 v2 07/17] ls-files: add --column
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-ls-files.txt | 6 ++
builtin/ls-files.c | 25 +
2 files changed, 31 insertions(+)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 5c1b7f3..cd52461 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -156,6 +156,12 @@ a space) at the start of each line:
default to color output, same as `--color=never`. This is the
default.
+--column[=]::
+--no-column::
+ Display files in columns. See configuration variable column.ui
+ for option syntax.`--column` and `--no-column` without options
+ are equivalent to 'always' and 'never' respectively.
+
\--::
Do not interpret any more arguments as options.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 2857b38..335d3b0 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -15,6 +15,7 @@
#include "string-list.h"
#include "pathspec.h"
#include "color.h"
+#include "column.h"
static int abbrev;
static int show_deleted;
@@ -29,6 +30,7 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int use_color;
+static unsigned int colopts;
static const char *prefix;
static int max_prefix_len;
@@ -39,6 +41,7 @@ static char *ps_matched;
static const char *with_tree;
static int exc_given;
static int exclude_args;
+static struct string_list output = STRING_LIST_INIT_NODUP;
static const char *tag_cached = "";
static const char *tag_unmerged = "";
@@ -66,6 +69,10 @@ static void write_name(struct strbuf *sb, const char *name)
static void strbuf_fputs(struct strbuf *sb, FILE *fp)
{
+ if (column_active(colopts)) {
+ string_list_append(&output, strbuf_detach(sb, NULL));
+ return;
+ }
fwrite(sb->buf, sb->len, 1, fp);
}
@@ -552,6 +559,7 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
N_("pretend that paths removed since are
still present")),
OPT__COLOR(&use_color, N_("show color")),
+ OPT_COLUMN(0, "column", &colopts, N_("show files in columns")),
OPT__ABBREV(&abbrev),
OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
OPT_END()
@@ -596,6 +604,18 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
if (dir.exclude_per_dir)
exc_given = 1;
+ finalize_colopts(&colopts, -1);
+ if (explicitly_enable_column(colopts)) {
+ if (!line_terminator)
+ die(_("--column and -z are incompatible"));
+ if (show_resolve_undo)
+ die(_("--column and --resolve-undo are incompatible"));
+ if (debug_mode)
+ die(_("--column and --debug are incompatible"));
+ }
+ if (column_active(colopts))
+ line_terminator = 0;
+
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
@@ -638,6 +658,11 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
if (show_resolve_undo)
show_ru_info();
+ if (column_active(colopts)) {
+ print_columns(&output, colopts, NULL);
+ string_list_clear(&output, 0);
+ }
+
if (ps_matched) {
int bad;
bad = report_path_error(ps_matched, &pathspec, prefix);
--
1.9.1.345.ga1a145c
--
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 v2 11/17] ls: -u does not imply showing stages
Showing full index entry information is something for ls-files only. The users of "git ls" may just want to know what entries are not unmerged. Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/ls-files.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 74eb3c2..1638983 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -527,7 +527,7 @@ static int ls_files(const char **argv, const char *prefix) } if (show_modified || show_others || show_deleted || (dir.flags & DIR_SHOW_IGNORED) || show_killed) require_work_tree = 1; - if (show_unmerged) + if (show_unmerged && !porcelain) /* * There's no point in showing unmerged unless * you also show the stage information. -- 1.9.1.345.ga1a145c -- 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 v2 03/17] ls_colors.c: add function to color a file name
Tthe new function is based on print_color_indicator() from commit
7326d1f1a67edf21947ae98194f98c38b6e9e527 in coreutils.git.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
color.h | 2 ++
ls_colors.c | 66 +
2 files changed, 68 insertions(+)
diff --git a/color.h b/color.h
index 640fc48..398369a 100644
--- a/color.h
+++ b/color.h
@@ -94,5 +94,7 @@ void color_print_strbuf(FILE *fp, const char *color, const
struct strbuf *sb);
int color_is_nil(const char *color);
void parse_ls_color(void);
+void color_filename(struct strbuf *sb, const char *name,
+ const char *display_name, mode_t mode, int linkok);
#endif /* COLOR_H */
diff --git a/ls_colors.c b/ls_colors.c
index cef5a92..1125329 100644
--- a/ls_colors.c
+++ b/ls_colors.c
@@ -422,3 +422,69 @@ void parse_ls_color(void)
color_symlink_as_referent = 1;
git_config(ls_colors_config, NULL);
}
+
+void color_filename(struct strbuf *sb, const char *name,
+ const char *display_name, mode_t mode, int linkok)
+{
+ int type;
+ struct color_ext_type *ext; /* Color extension */
+
+ if (S_ISREG (mode)) {
+ type = LS_FL;
+ if ((mode & S_ISUID) != 0)
+ type = LS_SU;
+ else if ((mode & S_ISGID) != 0)
+ type = LS_SG;
+ else if ((mode & (S_IXUSR | S_IXGRP | S_IXOTH)) != 0)
+ type = LS_EX;
+ } else if (S_ISDIR (mode)) {
+ if ((mode & S_ISVTX) && (mode & S_IWOTH))
+ type = LS_TW;
+ else if ((mode & S_IWOTH) != 0)
+ type = LS_OW;
+ else if ((mode & S_ISVTX) != 0)
+ type = LS_ST;
+ else
+ type = LS_DI;
+ } else if (S_ISLNK (mode))
+ type = (!linkok && *ls_colors[LS_OR]) ? LS_OR : LS_LN;
+ else if (S_ISFIFO (mode))
+ type = LS_PI;
+ else if (S_ISSOCK (mode))
+ type = LS_SO;
+ else if (S_ISBLK (mode))
+ type = LS_BD;
+ else if (S_ISCHR (mode))
+ type = LS_CD;
+#ifdef S_ISDOOR
+ else if (S_ISDOOR (mode))
+ type = LS_DO;
+#endif
+ else
+ /* Classify a file of some other type as C_ORPHAN. */
+ type = LS_OR;
+
+ /* Check the file's suffix only if still classified as C_FILE. */
+ ext = NULL;
+ if (type == LS_FL) {
+ /* Test if NAME has a recognized suffix. */
+ size_t len = strlen(name);
+ const char *p = name + len; /* Pointer to final \0.
*/
+ for (ext = color_ext_list; ext != NULL; ext = ext->next) {
+ if (ext->ext.len <= len &&
+ !strncmp(p - ext->ext.len, ext->ext.string,
ext->ext.len))
+ break;
+ }
+ }
+
+ if (display_name)
+ name = display_name;
+ if (ext)
+ strbuf_addf(sb, "\033[%.*sm%s%s",
+ (int)ext->seq.len, ext->seq.string,
+ name, GIT_COLOR_RESET);
+ else if (*ls_colors[type])
+ strbuf_addf(sb, "%s%s%s", ls_colors[type], name,
GIT_COLOR_RESET);
+ else
+ strbuf_addstr(sb, name);
+}
--
1.9.1.345.ga1a145c
--
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 v2 14/17] ls: add -t back
Tag "H" (cached) is not shown though because it's usually the majority
and becomes noise. Not showing it makes the other tags stand out. -t
is on by default if more than one file category is selected.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-ls.txt | 6 ++
builtin/ls-files.c | 27 +--
2 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-ls.txt b/Documentation/git-ls.txt
index 0480c42..126d9db 100644
--- a/Documentation/git-ls.txt
+++ b/Documentation/git-ls.txt
@@ -47,6 +47,12 @@ OPTIONS
--unmerged::
Show unmerged files in the output (forces --stage)
+-t::
+--tag::
+ Show a tag to indicate file type, helpful when multiple file
+ selections are used. See linkgit::git-ls-files[1] option `-t`
+ for more information.
+
-R::
--recursive::
Equivalent of --max-depth=-1 (infinite recursion).
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 014de05..392d273 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -515,16 +515,6 @@ static int ls_files(const char **argv, const char *prefix)
for (i = 0; i < exclude_list.nr; i++) {
add_exclude(exclude_list.items[i].string, "", 0, el,
--exclude_args);
}
- if (show_tag || show_valid_bit) {
- tag_cached = "H ";
- tag_unmerged = "M ";
- tag_removed = "R ";
- tag_modified = "C ";
- tag_other = "? ";
- tag_killed = "K ";
- tag_skip_worktree = "S ";
- tag_resolve_undo = "U ";
- }
if (show_modified || show_others || show_deleted || (dir.flags &
DIR_SHOW_IGNORED) || show_killed)
require_work_tree = 1;
if (show_unmerged && !porcelain)
@@ -578,6 +568,20 @@ static int ls_files(const char **argv, const char *prefix)
show_killed || show_modified || show_resolve_undo))
show_cached = 1;
+ if (show_tag == -1)
+ show_tag = (show_cached + show_deleted + show_others +
+ show_unmerged + show_killed + show_modified) > 1;
+ if (show_tag || show_valid_bit) {
+ tag_cached = porcelain ? " " : "H ";
+ tag_unmerged = "M ";
+ tag_removed = "R ";
+ tag_modified = "C ";
+ tag_other = "? ";
+ tag_killed = "K ";
+ tag_skip_worktree = "S ";
+ tag_resolve_undo = "U ";
+ }
+
if (max_prefix)
prune_cache(max_prefix);
if (with_tree) {
@@ -727,6 +731,8 @@ int cmd_ls(int argc, const char **argv, const char
*cmd_prefix)
N_("show unmerged files in the output")),
OPT_SET_INT('R', "recursive", &max_depth,
N_("shortcut for --max-depth=-1"), -1),
+ OPT_BOOL('t', "tag", &show_tag,
+ N_("identify the file status with tags")),
OPT__COLOR(&use_color, N_("show color")),
OPT_COLUMN(0, "column", &colopts, N_("show files in columns")),
OPT_SET_INT('1', NULL, &colopts,
@@ -756,6 +762,7 @@ int cmd_ls(int argc, const char **argv, const char
*cmd_prefix)
setup_standard_excludes(&dir);
use_color = -1;
max_depth = 0;
+ show_tag = -1;
argc = parse_options(argc, argv, prefix, builtin_ls_options,
ls_usage, 0);
--
1.9.1.345.ga1a145c
--
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 v2 12/17] ls: add -R/--recursive short for --max-depth=-1
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-ls.txt | 4
builtin/ls-files.c | 2 ++
2 files changed, 6 insertions(+)
diff --git a/Documentation/git-ls.txt b/Documentation/git-ls.txt
index 67ca522..10df6b0 100644
--- a/Documentation/git-ls.txt
+++ b/Documentation/git-ls.txt
@@ -47,6 +47,10 @@ OPTIONS
--unmerged::
Show unmerged files in the output (forces --stage)
+-R::
+--recursive::
+ Equivalent of --max-depth=-1 (infinite recursion).
+
--color[=]::
Color file names. The value must be always (default), never,
or auto.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1638983..772a6ce 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -725,6 +725,8 @@ int cmd_ls(int argc, const char **argv, const char
*cmd_prefix)
DIR_SHOW_IGNORED),
OPT_BOOL('u', "unmerged", &show_unmerged,
N_("show unmerged files in the output")),
+ OPT_SET_INT('R', "recursive", &max_depth,
+ N_("shortcut for --max-depth=-1"), -1),
OPT__COLOR(&use_color, N_("show color")),
OPT_COLUMN(0, "column", &colopts, N_("show files in columns")),
{ OPTION_INTEGER, 0, "max-depth", &max_depth, N_("depth"),
--
1.9.1.345.ga1a145c
--
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 v2 15/17] ls: sort output and remove duplicates
When you mix different file types, with ls-files you may get separate
listing. For example, "ls-files -cm" will show file "abc" twice: one
as part of cached list, one of modified list. With "ls" (and this
patch) they will be in a single sorted list (easier for the eye).
Duplicate entries are also removed. Note that display content is
compared, so if you have "-t" on, or you color file types differently,
you will get duplicate textual entries. This is good imo.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/ls-files.c | 30 +-
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 392d273..709d8b1 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -57,6 +57,13 @@ static const char *tag_modified = "";
static const char *tag_skip_worktree = "";
static const char *tag_resolve_undo = "";
+static int compare_output(const void *a_, const void *b_)
+{
+ const struct string_list_item *a = a_;
+ const struct string_list_item *b = b_;
+ return strcmp(a->util, b->util);
+}
+
static void write_name(struct strbuf *sb, const char *name)
{
/*
@@ -72,10 +79,12 @@ static void write_name(struct strbuf *sb, const char *name)
quote_path_relative(name, real_prefix, sb);
}
-static void strbuf_fputs(struct strbuf *sb, FILE *fp)
+static void strbuf_fputs(struct strbuf *sb, const char *full_name, FILE *fp)
{
- if (column_active(colopts)) {
- string_list_append(&output, strbuf_detach(sb, NULL));
+ if (column_active(colopts) || porcelain) {
+ struct string_list_item *it;
+ it = string_list_append(&output, strbuf_detach(sb, NULL));
+ it->util = (void*)full_name;
return;
}
fwrite(sb->buf, sb->len, 1, fp);
@@ -110,7 +119,7 @@ static void show_dir_entry(const char *tag, struct
dir_entry *ent)
strbuf_reset(&sb);
strbuf_addstr(&sb, tag);
write_dir_entry(&sb, ent);
- strbuf_fputs(&sb, stdout);
+ strbuf_fputs(&sb, ent->name, stdout);
}
static void show_other_files(struct dir_struct *dir)
@@ -227,7 +236,7 @@ static void show_ce_entry(const char *tag, const struct
cache_entry *ce)
ce_stage(ce));
}
write_ce_name(&sb, ce);
- strbuf_fputs(&sb, stdout);
+ strbuf_fputs(&sb, ce->name, stdout);
if (debug_mode) {
const struct stat_data *sd = &ce->ce_stat_data;
@@ -535,7 +544,7 @@ static int ls_files(const char **argv, const char *prefix)
if (debug_mode)
die(_("--column and --debug are incompatible"));
}
- if (column_active(colopts))
+ if (column_active(colopts) || porcelain)
line_terminator = 0;
if (require_work_tree && !is_inside_work_tree())
@@ -601,10 +610,13 @@ static int ls_files(const char **argv, const char *prefix)
if (show_resolve_undo)
show_ru_info();
- if (column_active(colopts)) {
- print_columns(&output, colopts, NULL);
- string_list_clear(&output, 0);
+ if (porcelain) {
+ qsort(output.items, output.nr, sizeof(*output.items),
+ compare_output);
+ string_list_remove_duplicates(&output, 0);
}
+ print_columns(&output, colopts, NULL);
+ string_list_clear(&output, 0);
if (ps_matched) {
int bad;
--
1.9.1.345.ga1a145c
--
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 v2 13/17] ls: add -1 short for --no-column in the spirit of GNU ls
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-ls.txt | 3 +++
builtin/ls-files.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/Documentation/git-ls.txt b/Documentation/git-ls.txt
index 10df6b0..0480c42 100644
--- a/Documentation/git-ls.txt
+++ b/Documentation/git-ls.txt
@@ -51,6 +51,9 @@ OPTIONS
--recursive::
Equivalent of --max-depth=-1 (infinite recursion).
+-1::
+ Equivalent of --no-column.
+
--color[=]::
Color file names. The value must be always (default), never,
or auto.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 772a6ce..014de05 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -729,6 +729,8 @@ int cmd_ls(int argc, const char **argv, const char
*cmd_prefix)
N_("shortcut for --max-depth=-1"), -1),
OPT__COLOR(&use_color, N_("show color")),
OPT_COLUMN(0, "column", &colopts, N_("show files in columns")),
+ OPT_SET_INT('1', NULL, &colopts,
+ N_("shortcut for --no-column"), COL_PARSEOPT),
{ OPTION_INTEGER, 0, "max-depth", &max_depth, N_("depth"),
N_("descend at most levels"), PARSE_OPT_NONEG,
NULL, 1 },
--
1.9.1.345.ga1a145c
--
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 v2 16/17] ls: do not show duplicate cached entries
With the current show_files() "ls -tcm" will show
foo.c
M foo.c
The first item is redundant. If "foo.c" is modified, we know it's in
the cache. Introduce show_files_compact to do that because ls-files is
plumbing and scripts may already depend on current display behavior.
Another difference in show_files_compact() is it does not show
skip-worktree (aka outside sparse checkout) entries anymore, which
makes sense in porcelain context.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/ls-files.c | 52 +++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 709d8b1..cd8e35c 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -337,6 +337,53 @@ static void show_files(struct dir_struct *dir)
}
}
+static void show_files_compact(struct dir_struct *dir)
+{
+ int i;
+
+ /* For cached/deleted files we don't need to even do the readdir */
+ if (show_others || show_killed) {
+ if (!show_others)
+ dir->flags |= DIR_COLLECT_KILLED_ONLY;
+ fill_directory(dir, &pathspec);
+ if (show_others)
+ show_other_files(dir);
+ if (show_killed)
+ show_killed_files(dir);
+ }
+ if (!(show_cached || show_stage || show_deleted || show_modified))
+ return;
+ for (i = 0; i < active_nr; i++) {
+ const struct cache_entry *ce = active_cache[i];
+ struct stat st;
+ int err, shown = 0;
+ if ((dir->flags & DIR_SHOW_IGNORED) &&
+ !ce_excluded(dir, ce))
+ continue;
+ if (show_unmerged && !ce_stage(ce))
+ continue;
+ if (ce->ce_flags & CE_UPDATE)
+ continue;
+ if (ce_skip_worktree(ce))
+ continue;
+ err = lstat(ce->name, &st);
+ if (show_deleted && err) {
+ show_ce_entry(tag_removed, ce);
+ shown = 1;
+ }
+ if (show_modified && ce_modified(ce, &st, 0)) {
+ show_ce_entry(tag_modified, ce);
+ shown = 1;
+ }
+ if (ce_stage(ce)) {
+ show_ce_entry(tag_unmerged, ce);
+ shown = 1;
+ }
+ if (!shown && show_cached)
+ show_ce_entry(tag_cached, ce);
+ }
+}
+
/*
* Prune the index to only contain stuff starting with "prefix"
*/
@@ -606,7 +653,10 @@ static int ls_files(const char **argv, const char *prefix)
refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
setup_pager();
}
- show_files(&dir);
+ if (porcelain)
+ show_files_compact(&dir);
+ else
+ show_files(&dir);
if (show_resolve_undo)
show_ru_info();
--
1.9.1.345.ga1a145c
--
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 v2 17/17] ls: show directories as well as files
The index does not store directories explicitly (except submodules) so
we have to figure them out from file list. The function
show_directories() deliberately generates duplicate directories and
expects the previous patch to remove duplicates.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/ls-files.c | 41 +
1 file changed, 41 insertions(+)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index cd8e35c..7e50192 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -27,6 +27,7 @@ static int show_resolve_undo;
static int show_modified;
static int show_killed;
static int show_valid_bit;
+static int show_dirs;
static int line_terminator = '\n';
static int debug_mode;
static int use_color;
@@ -337,6 +338,43 @@ static void show_files(struct dir_struct *dir)
}
}
+static void show_directories(const struct cache_entry *ce)
+{
+ static const char *last_directory;
+ struct strbuf sb = STRBUF_INIT;
+ const char *p = ce->name + prefix_len;
+ const char *sep;
+
+ if (last_directory) {
+ int len = strlen(last_directory);
+ if (!strncmp(ce->name, last_directory, len) &&
+ ce->name[len] == '/')
+ p += len + 1;
+ }
+
+ while (*p && (sep = strchr(p, '/'))) {
+ struct strbuf sb2 = STRBUF_INIT;
+ strbuf_reset(&sb);
+ strbuf_add(&sb, ce->name, sep - ce->name);
+ p = sep + 1;
+ if (!match_pathspec(&pathspec, sb.buf, sb.len,
+ prefix_len, NULL, 1))
+ continue;
+ write_name(&sb2, sb.buf);
+ if (want_color(use_color)) {
+ struct strbuf sb3 = STRBUF_INIT;
+ color_filename(&sb3, ce->name, sb2.buf, S_IFDIR, 1);
+ strbuf_release(&sb2);
+ sb2 = sb3;
+ }
+ if (show_tag)
+ strbuf_insert(&sb2, 0, tag_cached, strlen(tag_cached));
+ last_directory = strbuf_detach(&sb, NULL);
+ strbuf_fputs(&sb2, last_directory, NULL);
+ strbuf_release(&sb2);
+ }
+}
+
static void show_files_compact(struct dir_struct *dir)
{
int i;
@@ -357,6 +395,8 @@ static void show_files_compact(struct dir_struct *dir)
const struct cache_entry *ce = active_cache[i];
struct stat st;
int err, shown = 0;
+ if (show_dirs)
+ show_directories(ce);
if ((dir->flags & DIR_SHOW_IGNORED) &&
!ce_excluded(dir, ce))
continue;
@@ -825,6 +865,7 @@ int cmd_ls(int argc, const char **argv, const char
*cmd_prefix)
use_color = -1;
max_depth = 0;
show_tag = -1;
+ show_dirs = 1;
argc = parse_options(argc, argv, prefix, builtin_ls_options,
ls_usage, 0);
--
1.9.1.345.ga1a145c
--
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] builtin/apply.c: use iswspace() to detect line-ending-like chars
On Tue, Mar 25, 2014 at 6:54 AM, Junio C Hamano wrote: > As a tangent, I have a suspicion that the current implementation may > be wrong at the beginning of the string. Wouldn't it match " abc" > and "abc", even though these two strings shouldn't match? Wouldn't that be accomplished by just removing the leading whitespace check? I'm somewhat confused about what the function should match. I haven't grasped it. -- papanikge's surrogate email. I may reply back. http://www.5slingshots.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: Borrowing objects from nearby repositories
Andrew Keller writes: > On Mar 25, 2014, at 6:17 PM, Junio C Hamano wrote: > ... >>> I think that the standard practice with the existing toolset is to >>> clone with reference and then repack. That is: >>> >>>$ git clone --reference git://over/there mine >>>$ cd mine >>>$ git repack -a -d >>> >>> And then you can try this: >>> >>>$ mv .git/objects/info/alternates .git/objects/info/alternates.disabled >>>$ git fsck >>> >>> to make sure that you are no longer borrowing anything from the >>> borrowee. Once you are satisfied, you can remove the saved-away >>> alternates.disabled file. >> >> Oh, I forgot to say that I am not opposed if somebody wants to teach >> "git clone" a new option to copy its objects from two places, >> (hopefully) the majority from near-by reference repository and the >> remainder over the network, without permanently relying on the >> former via the alternates mechanism. The implementation of such a >> feature could even literally be "clone with reference first and then >> repack" at least initially but even in the final version. [Administrivia: please wrap your lines to a reasonable length] > That was actually one of my first ideas - adding some sort of > '--auto-repack' option to git-clone. It's a relatively small > change, and would work. However, keeping in mind my end goal of > automating the feature to the point where you could run simply > 'git clone ', an '--auto-repack' option is more difficult to > undo. You would need a new parameter to disable the automatic > adding of reference repositories, and a new parameter to undo > '--auto-repack', and you'd have to remember to actually undo both > of those settings. > > In contrast, if the new feature was '--borrow', and the evolution > of the feature was a global configuration 'fetch.autoBorrow', then > to turn it off temporarily, one only needs a single new parameter > '--no-auto-borrow'. I think this is a cleaner approach than the > former, although much more work. I think you may have misread me. With the "new option", I was hinting that the "clone --reference && repack && rm alternates" will be an acceptable internal implementation of the "--borrow" option that was mentioned in the thread. I am not sure where you got the "auto-repack" from. One of the reasons you may have misread me may be because I made it sound as if "this may work and when it works you will be happy, but if it does not work you did not lose very much" by mentioning "mv && fsck". That wasn't what I meant. The "repack -a" procedure is to make the borrower repository no longer dependent on the borrowee, and it is supposed to always work. In fact, this behaviour was the whole reason why "repack" later learned its "-l" option to disable it, because people who cloned with "--reference" in order to reduce the disk footprint by sharing older and more common objects [*1*] were rightfully surprised to see that the borrowed objects were copied over to their borrower repository when they ran "repack" [*2*]. Because this is "clone", there is nothing complex to "undo". Either it succeeds, or you remove the whole new directory if anything fails. I said "even in the final version" for a simple reason: you cannot cannot do realistically any better than the "clone --reference && repack -a d && rm alternates" sequence. But you would need to know a few things about how Git works in order to come to that realisation. Here are some: * "clone --borrow" (or whatever we end up calling the option) must talk to two repositories: - We will need to have one upload-pack session with the distant origin repository over the network, which will send a complete pack. - We need to also copy objects that weren't sent from the distant origin to our repository from the reference one. * A single "repack -a -d" (without "-l") after "clone --reference" is already a way to do exactly what you need---enumerate what are missing in the packfile that was received from the distant origin and come up with packfile(s) that contain all and only objects the cloned repository needs. * You cannot easily concatenate multiple packfiles into a single one (or append runs of objects to an existing packfile) to come up with a single packfile. You _could_ shoehorn the logic to "enumerate and read from the reference, and append them at the end of the packfile received from the distant origin repository" into the part that talks to the distant origin repository, but the object layout in the resulting packfile will be suboptimal [*3*] and the code complexity required to do so is not worth it [*4*]. [Footnotes] *1* From the point of view of supporting both camps, i.e. those who want their borrower repositories to keep sharing the objects with the borrowee repository and those who want to use a borrowee repository temporarily while cloning only to reduce the network cost from the distant upstream, the
Re: [PATCH/RFC 0/6] reuse deltas found by bitmaps
Jeff King writes: > 2. When considering whether a delta can be reused, check the bitmaps > to see if the client has the base. If so, allow reuse. > ... > The implementation I'm including here is the one I've shown before, > which does (2). Part of the reason that I'm reposting it before looking > further into these options is that I've written a t/perf script that can > help with the analysis. Conceptually, that feels like a natural extension for the "thin pack" transfer. I wouldn't foresee a correctness issue, as long as the fetcher runs "index-pack --fix-thin" at their end. -- 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] builtin/apply.c: use iswspace() to detect line-ending-like chars
George Papanikolaou writes: > On Tue, Mar 25, 2014 at 6:54 AM, Junio C Hamano wrote: >> As a tangent, I have a suspicion that the current implementation may >> be wrong at the beginning of the string. Wouldn't it match " abc" >> and "abc", even though these two strings shouldn't match? > > Wouldn't that be accomplished by just removing the leading whitespace check? Yes. I was wondering *what* semantics we want in the first place; how to implement what I suggested is so trivial that it goes without saying for the intended audiences of that comment ;-). > I'm somewhat confused about what the function should match. I haven't > grasped it. This function is used when attempting to resurrect a patch that is whitespace-damaged. The patch may want to change a line "a_bc" in the original into something else [*1*], and we may not find "a_bc" in the current source, but there may be "a__bc" (two spaces instead of one the whitespace-damaged patch claims to expect). By ignoring the amount of whitespaces, it forces "git apply" to consider that "a_bc" in the broken patch meant to refer to "a__bc" in reality. I _think_ the original motivation of ignore_ws_change was to match the "--ignore-space-change" option of "diff", i.e. "ignore changes in the amount of white space". I just checked the source (xdiff/xutils.c) and made sure that "git diff" does not treat the beginning of line any differently hence "_a_bc" and "a_bc" are not considered a match under its --ignore-space-change option. The current implementation of "apply --ignore-space-change" that ignores leading whitespaces (not "ignore changes in the amount of leading whitespaces") is likely to be a bug from this point of view. But I wanted to hear opinions from other Git experts [*2*]. Hence my "As a tangent, I have a suspicion". [Footnote] *1* This mode is not enabled by default. I am not even sure if anybody sane would (or should) use this option. Sure, the fuzzy match may be able to find the original line that the patch author may meant to patch even when it is whiltespace-damaged because it does not fully trust what the original lines exactly say (i.e. context lines prefixed by " " and old lines prefixed by "-"). What makes it sane for us to trust what the replacement lines (i.e. new lines prefixed by "+") in such a mangled patch says? *2* For example, somebody may be able to point out that "this is meant to match the option of the same name 'diff' has", which is my assumption that leads to the above discussion, may not be true. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 0/6] reuse deltas found by bitmaps
On Wed, Mar 26, 2014 at 10:33:41AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > 2. When considering whether a delta can be reused, check the bitmaps > > to see if the client has the base. If so, allow reuse. > > ... > > The implementation I'm including here is the one I've shown before, > > which does (2). Part of the reason that I'm reposting it before looking > > further into these options is that I've written a t/perf script that can > > help with the analysis. > > Conceptually, that feels like a natural extension for the "thin > pack" transfer. I wouldn't foresee a correctness issue, as long as > the fetcher runs "index-pack --fix-thin" at their end. Right, this is only enabled when --thin is negotiated during the protocol transfer. I don't think there's any correctness problem here. But I want to make sure we are doing the fastest thing. Here's another set of results comparing three runs: v1.9.0 (with no bitmaps), origin/master (bitmaps), and this series. Apologies for the long lines. Test v1.9.0origin HEAD - 5311.3: server (1 days)0.20(0.17+0.02) 0.29(0.34+0.04) +45.0% 0.14(0.12+0.02) -30.0% 5311.4: size (1 days) 56.2K 1.0M +1694.4% 59.5K +5.9% 5311.5: client (1 days)0.08(0.07+0.00) 0.03(0.04+0.00) -62.5% 0.08(0.07+0.00) +0.0% 5311.7: server (2 days)0.06(0.06+0.00) 0.28(0.33+0.04) +366.7% 0.14(0.11+0.02) +133.3% 5311.8: size (2 days) 56.2K 1.0M +1694.4% 59.5K +5.9% 5311.9: client (2 days)0.09(0.08+0.00) 0.03(0.04+0.00) -66.7% 0.09(0.08+0.00) +0.0% 5311.11: server (4 days) 0.21(0.18+0.02) 0.29(0.33+0.03) +38.1% 0.14(0.11+0.02) -33.3% 5311.12: size (4 days) 64.2K 1.1M +1536.6% 67.9K +5.8% 5311.13: client (4 days) 0.08(0.08+0.00) 0.04(0.03+0.01) -50.0% 0.09(0.07+0.01) +12.5% 5311.15: server (8 days) 0.22(0.21+0.02) 0.36(0.47+0.03) +63.6% 0.14(0.11+0.02) -36.4% 5311.16: size (8 days)125.7K 1.5M +1100.8% 130.1K +3.5% 5311.17: client (8 days) 0.11(0.10+0.00) 0.05(0.05+0.00) -54.5% 0.13(0.11+0.01) +18.2% 5311.19: server (16 days) 0.26(0.26+0.03) 0.76(1.20+0.04) +192.3% 0.25(0.21+0.04) -3.8% 5311.20: size(16 days)358.6K 3.7M +931.7% 359.8K +0.3% 5311.21: client (16 days) 0.28(0.27+0.01) 0.13(0.15+0.00) -53.6% 0.30(0.28+0.02) +7.1% 5311.23: server (32 days) 0.44(0.54+0.07) 1.39(2.54+0.04) +215.9% 0.28(0.23+0.04) -36.4% 5311.24: size(32 days) 1.1M 8.6M +652.4% 2.1M +83.9% 5311.25: client (32 days) 0.66(0.64+0.02) 0.32(0.38+0.01) -51.5% 0.69(0.67+0.04) +4.5% 5311.27: server (64 days) 2.78(4.02+0.17) 7.02(15.59+0.16) +152.5% 0.43(0.37+0.07) -84.5% 5311.28: size(64 days) 18.7M 68.3M +265.5% 24.5M +31.3% 5311.29: client (64 days) 6.25(6.29+0.16) 3.21(4.76+0.14) -48.6% 6.27(6.46+0.18) +0.3% 5311.31: server (128 days) 4.48(7.32+0.21) 7.56(16.60+0.16) +68.8% 0.51(0.45+0.10) -88.6% 5311.32: size (128 days) 25.8M 83.1M +222.3% 35.9M +39.2% 5311.33: client (128 days) 7.32(7.58+0.17) 3.94(5.87+0.18) -46.2% 7.15(7.80+0.20) -2.3% My previous results showed that this series was an improvement over straight bitmaps. But what it didn't show is that bitmaps actually provide a regression for some fetches (I think because we do not find the boundary commits at all, and do not have any preferred bases). Just looking at the 128-day case again, using bitmaps increased our server CPU time _and_ made a much bigger pack. This series not only fixes the CPU time regression, but it also drops the server CPU time to almost nothing. That's a nice improvement, and it makes perfect sense: we are reusing on-disk deltas instead of on-the-fly computing deltas against the preferred bases. And it does help with the size regression, though the end result is still a bit worse than v1.9.0. I'm not exactly sure what's going on here. My guess is that we have objects to send that are not deltas on disk (probably because they are at the tip of history and other things are deltas against them). We would still want to do delta compression of them against preferred bases, but we don't have any preferred bases in our list. So I think we still want to add back in some list of preferred base objects when bitmaps are in use. The question is, which objects? With bitmaps, it's easy to add in all of the objects the client has, but that is probably going to be counterproductive (I sti
Re: [PATCH 5/5] log: do not segfault on gmtime errors
On Wed, Mar 26, 2014 at 11:05:59AM +, Charles Bailey wrote: > On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote: > > +# date is within 2^63-1, but enough to choke glibc's gmtime > > +test_expect_success 'absurdly far-in-future dates produce sentinel' ' > > + commit=$(munge_author_date HEAD 99) && > > + echo "Thu Jan 1 00:00:00 1970 +" >expect && > > + git log -1 --format=%ad $commit >actual && > > + test_cmp expect actual > > +' > > Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 > 162396404 -0700. I don't know if this is correct for the given input > but the test fails. Ick. I am not sure that dates at that range are even meaningful (will October really exist in the year 162396404? Did they account for all the leap-seconds between then and now?). But at the same time, I cannot fault their gmtime for just extrapolating the current rules out as far as we ask them to. Unlike the FreeBSD thing that René brought up, this is not a problem in the code, but just in the test. So I think our options are basically: 1. Scrap the test as unportable. 2. Hard-code a few expected values. I'd be unsurprised if some other system comes up with a slightly different date in 162396404, so we may end up needing several of these. I think I'd lean towards (2), just because it is testing an actual feature of the code, and I'd like to continue doing so. And while we may end up with a handful of values, there's probably not _that_ many independent implementations of gmtime in the wild. -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 12/19] tree-diff: remove special-case diff-emitting code for empty-tree cases
On Tue, Mar 25, 2014 at 10:45:01AM -0700, Junio C Hamano wrote:
> Kirill Smelkov writes:
>
> >> > static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc
> >> > *t2)
> >> > {
> >> > struct name_entry *e1, *e2;
> >> > int cmp;
> >> >
> >> > +if (!t1->size)
> >> > +return t2->size ? +1 /* +∞ > c */ : 0 /* +∞ = +∞ */;
> >> > +else if (!t2->size)
> >> > +return -1; /* c < +∞ */
> >>
> >> Where do these "c" come from? I somehow feel that these comments
> >> are making it harder to understand what is going on.
> >
> > "c" means some finite "c"onstant here. When I was studying at school and
> > at the university, it was common to denote constants via this letter -
> > i.e. in algebra and operators they often show scalar multiplication as
> >
> > c·A (or α·A)
> >
> > etc. I understand it could maybe be confusing (but it came to me as
> > surprise), so would the following be maybe better:
> >
> > if (!t1->size)
> > return t2->size ? +1 /* +∞ > const */ : 0 /* +∞ = +∞ */;
> > else if (!t2->size)
> > return -1; /* const < +∞ */
> >
> > ?
>
> Not better at all, I am afraid. A "const" in the code usually means
> "something that does not change, as opposed to a variable", but what
> you are saying here is "t1 does not have an element but t2 still
> does. Pretend as if t1 has a virtual/fake element that is larger
> than any real element t2 may happen to have at the head of its
> queue", and you are labeling that "real element at the head of t2"
> as "const", but as the walker advances, the head element in t1 and
> t2 will change---they are not "const" in that sense, and the reader
> is left scratching his head seeing "const" there, wondering what the
> author of the comment meant.
I agree.
> "real" or "concrete" might be better a phrasing, but I do not think
> having "/* +inf > concrete */" there helps the reader understand
> what is going on in the first place. Perhaps:
>
> /*
> * When one side is empty, pretend that it has an element
> * that sorts later than what the other non-empty side has,
> * so that the caller advances the non-empty side without
> * touching the empty side.
> */
> if (!t1->size)
> return !t2->size ? 0 : 1;
> else if (!t2->size)
> return -1;
>
> or something?
Yes, that describe the reasoning without stranger symbols. How about
taking it further with
* NOTE empty (=invalid) descriptor(s) take part in comparison as
+infty,
* so that they sort *after* valid tree entries.
*
* Due to this convention, if trees are scanned in sorted order,
all
* non-empty descriptors will be processed first.
*/
static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc
*t2)
{
struct name_entry *e1, *e2;
int cmp;
/* empty descriptors sort after valid tree entries */
if (!t1->size)
return t2->size ? +1 : 0;
else if (!t2->size)
return -1;
?
On Tue, Mar 25, 2014 at 03:07:33PM -0700, Junio C Hamano wrote:
> Kirill Smelkov writes:
>
> > On Mon, Mar 24, 2014 at 02:18:10PM -0700, Junio C Hamano wrote:
> >> Kirill Smelkov writes:
> >>
> >> > via teaching tree_entry_pathcmp() how to compare empty tree descriptors:
> >>
> >> Drop this line, as you explain the "pretend empty compares bigger
> >> than anything else" idea later anyway? This early part of the
> >> proposed log message made me hiccup while reading it.
> >
> > Hmm, I was trying to show the big picture first and only then details...
>
> The subject should be sufficient for the big picture. "OK, we are
> removing the special casing" is what we expect the reader to get.
> Then, this
>
> >> > While walking trees, we iterate their entries from lowest to highest in
> >> > sort order, so empty tree means all entries were already went over.
>
> sets the background. "OK, the code walks two trees, both have
> sorted elements, in parallel." is what we want the reader to
> understand. Then the next part gives the idea of pretending that
> the empty-side always compare later than the non-empty side while
> doing that parallel walking (similar to "merge").
>
> So, yes, I think it is a good presentation order to give big picture
> punch-line first on the subject, some background and then the
> solution.
Ok, let it be this way and let's drop it.
Here is updated patch:
(please keep author email)
8<
From: Kirill Smelkov
Date: Mon, 24 Feb 2014 20:21:44 +0400
Subject: [PATCH v2] tree-diff: remove special-case diff-emitting code for
empty-tree cases
While walking trees, we iterate their entries from lowest to highest in
sort order, so empty tree means all entries were
Re: [PATCH v2 14/27] update-ref.c: Extract a new function, parse_next_sha1()
On 03/24/2014 01:56 PM, Michael Haggerty wrote: > +/* > + * For backwards compatibility, accept an empty string for create's > + * in binary mode to be equivalent to specifying zeros. > + */ > +#define PARSE_SHA1_ALLOW_EMPTY 0x02 The comment should say "update's", not "create's". -Brad -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 16/27] t1400: Test one mistake at a time
On 03/24/2014 01:56 PM, Michael Haggerty wrote: > Signed-off-by: Michael Haggerty > > t1400: Add a test of "update" with too few arguments > > Signed-off-by: Michael Haggerty This looks like a stray squash message. -Brad -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction
On 03/24/2014 01:56 PM, Michael Haggerty wrote: > Changes relative to v1: > > * Rename the functions associated with ref_transactions to be more > reminiscent of database transactions: > > * create_ref_transaction() -> ref_transaction_begin() > * free_ref_transaction() -> ref_transaction_rollback() > * queue_update_ref() -> ref_transaction_update() > * queue_create_ref() -> ref_transaction_create() > * queue_delete_ref() -> ref_transaction_delete() > * commit_ref_transaction() -> ref_transaction_commit() Those new names look better. > * Fix backwards compatibility of "git update-ref --stdin -z"'s > handling of the "create" command: allow to be the empty > string, treating it the same zeros. But deprecate this usage. The changes related to that look good. The new documentation is much clearer than my old wording. Series v2 looks good to me except for my responses to individual commits. Thanks, -Brad -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction
On 03/24/2014 01:56 PM, Michael Haggerty wrote: > +void ref_transaction_update(struct ref_transaction *transaction, > + const char *refname, > + unsigned char *new_sha1, unsigned char *old_sha1, > + int flags, int have_old); [snip] > +void ref_transaction_create(struct ref_transaction *transaction, > + const char *refname, > + unsigned char *new_sha1, > + int flags); [snip] > +void ref_transaction_delete(struct ref_transaction *transaction, > + const char *refname, > + unsigned char *old_sha1, > + int flags, int have_old); Perhaps we also need: void ref_transaction_verify(struct ref_transaction *transaction, const char *refname, unsigned char *old_sha1, int flags, int have_old); as equivalent to the "verify" command in "update-ref --stdin"? -Brad -- 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] t4212: handle systems with post-apocalyptic gmtime
One of the tests in t4212 checks our behavior when we feed
gmtime a date so far in the future that it gives up and
returns NULL.
But some gmtime implementations just refuse to quit. They
soldier on, giving us a glimpse of a chilly October evening
some 160 million years in the future (and presumably filled
with our leather-clad horseback-riding ape descendants).
Let's allow the test to match either the sentinel value
(i.e., what we want when gmtime gives up) or any reasonable
value returned by known implementations.
Reported-by: Charles Bailey
Signed-off-by: Jeff King
---
On top of jk/commit-dates-parsing-fix (though the test is already in
v1.9.1).
t/t4212-log-corrupt.sh | 23 +++
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh
index 85c6df4..08f982c 100755
--- a/t/t4212-log-corrupt.sh
+++ b/t/t4212-log-corrupt.sh
@@ -76,12 +76,27 @@ test_expect_success 'date parser recognizes time_t
overflow' '
test_cmp expect actual
'
-# date is within 2^63-1, but enough to choke glibc's gmtime
-test_expect_success 'absurdly far-in-future dates produce sentinel' '
+cmp_one_of () {
+ for candidate in "$@"; do
+ echo "$candidate" >expect &&
+ test_cmp expect actual &&
+ return 0
+ done
+ return 1
+}
+
+# date is within 2^63-1, but enough to choke glibc's gmtime.
+# We check that either the date broke gmtime (and we return the
+# usual epoch value), or gmtime gave us some sensible value.
+#
+# The sensible values are determined experimentally. The first
+# is from AIX.
+test_expect_success 'absurdly far-in-future dates' '
commit=$(munge_author_date HEAD 99) &&
- echo "Thu Jan 1 00:00:00 1970 +" >expect &&
git log -1 --format=%ad $commit >actual &&
- test_cmp expect actual
+ cmp_one_of \
+ "Thu Jan 1 00:00:00 1970 +" \
+ "Thu Oct 24 18:46:39 162396404 -0700"
'
test_done
--
1.9.1.656.ge8a0637
--
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 5/5] log: do not segfault on gmtime errors
Jeff King writes: > On Wed, Mar 26, 2014 at 11:05:59AM +, Charles Bailey wrote: > >> On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote: >> > +# date is within 2^63-1, but enough to choke glibc's gmtime >> > +test_expect_success 'absurdly far-in-future dates produce sentinel' ' >> > + commit=$(munge_author_date HEAD 99) && >> > + echo "Thu Jan 1 00:00:00 1970 +" >expect && >> > + git log -1 --format=%ad $commit >actual && >> > + test_cmp expect actual >> > +' >> >> Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 >> 162396404 -0700. I don't know if this is correct for the given input >> but the test fails. > > Ick. I am not sure that dates at that range are even meaningful (will > October really exist in the year 162396404? Did they account for all the > leap-seconds between then and now?). But at the same time, I cannot > fault their gmtime for just extrapolating the current rules out as far > as we ask them to. > > Unlike the FreeBSD thing that René brought up, this is not a problem in > the code, but just in the test. So I think our options are basically: > > 1. Scrap the test as unportable. > > 2. Hard-code a few expected values. I'd be unsurprised if some other > system comes up with a slightly different date in 162396404, so we > may end up needing several of these. > > I think I'd lean towards (2), just because it is testing an actual > feature of the code, and I'd like to continue doing so. And while we may > end up with a handful of values, there's probably not _that_ many > independent implementations of gmtime in the wild. Or "3. Just make sure that 'git log' does not segfault"? -- 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 5/5] log: do not segfault on gmtime errors
On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote: > > Unlike the FreeBSD thing that René brought up, this is not a problem in > > the code, but just in the test. So I think our options are basically: > > > > 1. Scrap the test as unportable. > > > > 2. Hard-code a few expected values. I'd be unsurprised if some other > > system comes up with a slightly different date in 162396404, so we > > may end up needing several of these. > > > > I think I'd lean towards (2), just because it is testing an actual > > feature of the code, and I'd like to continue doing so. And while we may > > end up with a handful of values, there's probably not _that_ many > > independent implementations of gmtime in the wild. > > Or "3. Just make sure that 'git log' does not segfault"? That would not test the FreeBSD case, which does not segfault, but returns a bogus sentinel value. I don't know how important that is. This is such a minor feature that it is not worth a lot of maintenance headache in the test. But I also do not know if this is going to be the last report, or we will have a bunch of other systems that need their own special values put into the test. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/17] ls-files: add --color to highlight file names
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Documentation/git-ls-files.txt | 9 +
> builtin/ls-files.c | 38 +++---
> 2 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index c0856a6..5c1b7f3 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -147,6 +147,15 @@ a space) at the start of each line:
> possible for manual inspection; the exact format may change at
> any time.
>
> +--color[=]::
> + Color file names. The value must be always (default), never,
> + or auto.
Here, the default is "always"...
> +--no-color::
> + Turn off coloring, even when the configuration file gives the
> + default to color output, same as `--color=never`. This is the
> + default.
But, here the default is "never".
> +
> \--::
> Do not interpret any more arguments as options.
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 6e30592..2857b38 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -14,6 +14,7 @@
> #include "resolve-undo.h"
> #include "string-list.h"
> #include "pathspec.h"
> +#include "color.h"
>
> static int abbrev;
> static int show_deleted;
> @@ -27,6 +28,7 @@ static int show_killed;
> static int show_valid_bit;
> static int line_terminator = '\n';
> static int debug_mode;
> +static int use_color;
>
> static const char *prefix;
> static int max_prefix_len;
> @@ -60,7 +62,6 @@ static void write_name(struct strbuf *sb, const char *name)
> strbuf_release(&sb2);
> } else
> quote_path_relative(name, real_prefix, sb);
> - strbuf_addch(sb, line_terminator);
> }
>
> static void strbuf_fputs(struct strbuf *sb, FILE *fp)
> @@ -68,6 +69,21 @@ static void strbuf_fputs(struct strbuf *sb, FILE *fp)
> fwrite(sb->buf, sb->len, 1, fp);
> }
>
> +static void write_dir_entry(struct strbuf *sb, const struct dir_entry *ent)
> +{
> + struct strbuf quoted = STRBUF_INIT;
> + struct stat st;
> + if (stat(ent->name, &st))
> + st.st_mode = 0;
> + write_name("ed, ent->name);
> + if (want_color(use_color))
> + color_filename(sb, ent->name, quoted.buf, st.st_mode, 1);
> + else
> + strbuf_addbuf(sb, "ed);
> + strbuf_addch(sb, line_terminator);
> + strbuf_release("ed);
> +}
> +
> static void show_dir_entry(const char *tag, struct dir_entry *ent)
> {
> static struct strbuf sb = STRBUF_INIT;
> @@ -81,7 +97,7 @@ static void show_dir_entry(const char *tag, struct
> dir_entry *ent)
>
> strbuf_reset(&sb);
> strbuf_addstr(&sb, tag);
> - write_name(&sb, ent->name);
> + write_dir_entry(&sb, ent);
> strbuf_fputs(&sb, stdout);
> }
>
> @@ -146,6 +162,18 @@ static void show_killed_files(struct dir_struct *dir)
> }
> }
>
> +static void write_ce_name(struct strbuf *sb, const struct cache_entry *ce)
> +{
> + struct strbuf quoted = STRBUF_INIT;
> + write_name("ed, ce->name);
> + if (want_color(use_color))
> + color_filename(sb, ce->name, quoted.buf, ce->ce_mode, 1);
> + else
> + strbuf_addbuf(sb, "ed);
> + strbuf_addch(sb, line_terminator);
> + strbuf_release("ed);
> +}
> +
> static void show_ce_entry(const char *tag, const struct cache_entry *ce)
> {
> static struct strbuf sb = STRBUF_INIT;
> @@ -186,7 +214,7 @@ static void show_ce_entry(const char *tag, const struct
> cache_entry *ce)
> find_unique_abbrev(ce->sha1,abbrev),
> ce_stage(ce));
> }
> - write_name(&sb, ce->name);
> + write_ce_name(&sb, ce);
> strbuf_fputs(&sb, stdout);
> if (debug_mode) {
> const struct stat_data *sd = &ce->ce_stat_data;
> @@ -523,6 +551,7 @@ int cmd_ls_files(int argc, const char **argv, const char
> *cmd_prefix)
> N_("if any is not in the index, treat this as
> an error")),
> OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
> N_("pretend that paths removed since are
> still present")),
> + OPT__COLOR(&use_color, N_("show color")),
> OPT__ABBREV(&abbrev),
> OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
> OPT_END()
> @@ -570,6 +599,9 @@ int cmd_ls_files(int argc, const char **argv, const char
> *cmd_prefix)
> if (require_work_tree && !is_inside_work_tree())
> setup_work_tree();
>
> + if (want_color(use_color))
> + parse_ls_color();
> +
> parse_pathspec(&pathspec, 0,
>PATHSPEC_PREFER_CWD |
>PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
>
Re: [PATCH v2 03/17] ls_colors.c: add function to color a file name
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy wrote:
> Tthe new function is based on print_color_indicator() from commit
s/Tthe/The/
> 7326d1f1a67edf21947ae98194f98c38b6e9e527 in coreutils.git.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> color.h | 2 ++
> ls_colors.c | 66
> +
> 2 files changed, 68 insertions(+)
>
> diff --git a/color.h b/color.h
> index 640fc48..398369a 100644
> --- a/color.h
> +++ b/color.h
> @@ -94,5 +94,7 @@ void color_print_strbuf(FILE *fp, const char *color, const
> struct strbuf *sb);
> int color_is_nil(const char *color);
>
> void parse_ls_color(void);
> +void color_filename(struct strbuf *sb, const char *name,
> + const char *display_name, mode_t mode, int linkok);
>
> #endif /* COLOR_H */
> diff --git a/ls_colors.c b/ls_colors.c
> index cef5a92..1125329 100644
> --- a/ls_colors.c
> +++ b/ls_colors.c
> @@ -422,3 +422,69 @@ void parse_ls_color(void)
> color_symlink_as_referent = 1;
> git_config(ls_colors_config, NULL);
> }
> +
> +void color_filename(struct strbuf *sb, const char *name,
> + const char *display_name, mode_t mode, int linkok)
> +{
> + int type;
> + struct color_ext_type *ext; /* Color extension */
> +
> + if (S_ISREG (mode)) {
> + type = LS_FL;
> + if ((mode & S_ISUID) != 0)
> + type = LS_SU;
> + else if ((mode & S_ISGID) != 0)
> + type = LS_SG;
> + else if ((mode & (S_IXUSR | S_IXGRP | S_IXOTH)) != 0)
> + type = LS_EX;
> + } else if (S_ISDIR (mode)) {
> + if ((mode & S_ISVTX) && (mode & S_IWOTH))
> + type = LS_TW;
> + else if ((mode & S_IWOTH) != 0)
> + type = LS_OW;
> + else if ((mode & S_ISVTX) != 0)
> + type = LS_ST;
> + else
> + type = LS_DI;
> + } else if (S_ISLNK (mode))
> + type = (!linkok && *ls_colors[LS_OR]) ? LS_OR : LS_LN;
> + else if (S_ISFIFO (mode))
> + type = LS_PI;
> + else if (S_ISSOCK (mode))
> + type = LS_SO;
> + else if (S_ISBLK (mode))
> + type = LS_BD;
> + else if (S_ISCHR (mode))
> + type = LS_CD;
> +#ifdef S_ISDOOR
> + else if (S_ISDOOR (mode))
> + type = LS_DO;
> +#endif
> + else
> + /* Classify a file of some other type as C_ORPHAN. */
> + type = LS_OR;
> +
> + /* Check the file's suffix only if still classified as C_FILE. */
> + ext = NULL;
> + if (type == LS_FL) {
> + /* Test if NAME has a recognized suffix. */
> + size_t len = strlen(name);
> + const char *p = name + len; /* Pointer to final
> \0. */
> + for (ext = color_ext_list; ext != NULL; ext = ext->next) {
> + if (ext->ext.len <= len &&
> + !strncmp(p - ext->ext.len, ext->ext.string,
> ext->ext.len))
> + break;
> + }
> + }
> +
> + if (display_name)
> + name = display_name;
> + if (ext)
> + strbuf_addf(sb, "\033[%.*sm%s%s",
> + (int)ext->seq.len, ext->seq.string,
> + name, GIT_COLOR_RESET);
> + else if (*ls_colors[type])
> + strbuf_addf(sb, "%s%s%s", ls_colors[type], name,
> GIT_COLOR_RESET);
> + else
> + strbuf_addstr(sb, name);
> +}
> --
> 1.9.1.345.ga1a145c
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime
Jeff King writes:
> +cmp_one_of () {
> + for candidate in "$@"; do
Style ;-)
> + echo "$candidate" >expect &&
> + test_cmp expect actual &&
> + return 0
> + done
> + return 1
> +}
It actually may be easier to understand if you write a trivial case
statement at the sole calling site of this helper function, though.
In any case, would it really be essential to make sure that the
output shows a phony (or a seemingly real) datestamp for this test?
The primary thing you wanted to achieve by the "gmtime gave us NULL,
let's substitute it with an arbitrary value to avoid dereferencing
the NULL" change was *not* that we see that same arbitrary value
comes out of the system, but that we do not die by attempting to
reference the NULL, I think. Not dying is the primary thing we want
to (and we already do) test, no?
> +# date is within 2^63-1, but enough to choke glibc's gmtime.
> +# We check that either the date broke gmtime (and we return the
> +# usual epoch value), or gmtime gave us some sensible value.
> +#
> +# The sensible values are determined experimentally. The first
> +# is from AIX.
> +test_expect_success 'absurdly far-in-future dates' '
> commit=$(munge_author_date HEAD 99) &&
> - echo "Thu Jan 1 00:00:00 1970 +" >expect &&
> git log -1 --format=%ad $commit >actual &&
> - test_cmp expect actual
> + cmp_one_of \
> + "Thu Jan 1 00:00:00 1970 +" \
> + "Thu Oct 24 18:46:39 162396404 -0700"
> '
>
> test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/17] ls-files: buffer full item in strbuf before printing
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy wrote:
> Buffering so that we can manipulate the strings (e.g. coloring)
> further before finally printing them.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/ls-files.c | 48 +++-
> 1 file changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 47c3880..6e30592 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -47,18 +47,30 @@ static const char *tag_modified = "";
> static const char *tag_skip_worktree = "";
> static const char *tag_resolve_undo = "";
>
> -static void write_name(const char *name)
> +static void write_name(struct strbuf *sb, const char *name)
> {
> /*
> * With "--full-name", prefix_len=0; this caller needs to pass
> * an empty string in that case (a NULL is good for "").
> */
> - write_name_quoted_relative(name, prefix_len ? prefix : NULL,
> - stdout, line_terminator);
> + const char *real_prefix = prefix_len ? prefix : NULL;
> + if (!line_terminator) {
> + struct strbuf sb2 = STRBUF_INIT;
> + strbuf_addstr(sb, relative_path(name, real_prefix, &sb2));
> + strbuf_release(&sb2);
> + } else
> + quote_path_relative(name, real_prefix, sb);
> + strbuf_addch(sb, line_terminator);
> +}
> +
> +static void strbuf_fputs(struct strbuf *sb, FILE *fp)
> +{
> + fwrite(sb->buf, sb->len, 1, fp);
> }
>
> static void show_dir_entry(const char *tag, struct dir_entry *ent)
> {
> + static struct strbuf sb = STRBUF_INIT;
> int len = max_prefix_len;
>
> if (len >= ent->len)
> @@ -67,8 +79,10 @@ static void show_dir_entry(const char *tag, struct
> dir_entry *ent)
> if (!dir_path_match(ent, &pathspec, len, ps_matched))
> return;
>
> - fputs(tag, stdout);
> - write_name(ent->name);
> + strbuf_reset(&sb);
> + strbuf_addstr(&sb, tag);
> + write_name(&sb, ent->name);
> + strbuf_fputs(&sb, stdout);
strbuf_release(&sb);
> }
>
> static void show_other_files(struct dir_struct *dir)
> @@ -134,6 +148,7 @@ static void show_killed_files(struct dir_struct *dir)
>
> static void show_ce_entry(const char *tag, const struct cache_entry *ce)
> {
> + static struct strbuf sb = STRBUF_INIT;
> int len = max_prefix_len;
>
> if (len >= ce_namelen(ce))
> @@ -161,16 +176,18 @@ static void show_ce_entry(const char *tag, const struct
> cache_entry *ce)
> tag = alttag;
> }
>
> + strbuf_reset(&sb);
'sb' is empty at this point. Why reset it?
> if (!show_stage) {
> - fputs(tag, stdout);
> + strbuf_addstr(&sb, tag);
> } else {
> - printf("%s%06o %s %d\t",
> - tag,
> - ce->ce_mode,
> - find_unique_abbrev(ce->sha1,abbrev),
> - ce_stage(ce));
> + strbuf_addf(&sb, "%s%06o %s %d\t",
> + tag,
> + ce->ce_mode,
> + find_unique_abbrev(ce->sha1,abbrev),
> + ce_stage(ce));
> }
> - write_name(ce->name);
> + write_name(&sb, ce->name);
> + strbuf_fputs(&sb, stdout);
strbuf_release(&sb);
> if (debug_mode) {
> const struct stat_data *sd = &ce->ce_stat_data;
>
> @@ -206,7 +223,12 @@ static void show_ru_info(void)
> printf("%s%06o %s %d\t", tag_resolve_undo,
> ui->mode[i],
>find_unique_abbrev(ui->sha1[i], abbrev),
>i + 1);
> - write_name(path);
> + /*
> +* With "--full-name", prefix_len=0; this caller
> needs to pass
> +* an empty string in that case (a NULL is good for
> "").
> +*/
> + write_name_quoted_relative(path, prefix_len ? prefix
> : NULL,
> + stdout, line_terminator);
> }
> }
> }
> --
> 1.9.1.345.ga1a145c
--
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: [RFC/PATCH 1/4] test-lib: add test_dir_is_empty()
Am 26.03.2014 11:43, schrieb Michael Haggerty:
> On 03/26/2014 09:29 AM, Jens Lehmann wrote:
>> Am 25.03.2014 21:49, schrieb Junio C Hamano:
>>> Jens Lehmann writes:
t/test-lib-functions.sh | 11 +++
1 file changed, 11 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 158e10a..93d10cd 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -489,6 +489,17 @@ test_path_is_dir () {
fi
}
+# Check if the directory exists and is empty as expected, barf otherwise.
+test_dir_is_empty () {
+ test_path_is_dir "$1" &&
+ if test $(ls -a1 "$1" | wc -l) != 2
+ then
+ echo "Directory '$1' is not empty, it contains:"
+ ls -la "$1"
+ return 1
+ fi
+}
+
test_path_is_missing () {
if [ -e "$1" ]
then
>
> Why not do something like
>
> test -z "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
>
> I.e., make the test ignore "." and ".." without depending on their
> existence?
Thanks, will do so in the next round.
--
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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 12:18:25PM -0700, Junio C Hamano wrote: > > + echo "$candidate" >expect && > > + test_cmp expect actual && > > + return 0 > > + done > > + return 1 > > +} > > It actually may be easier to understand if you write a trivial case > statement at the sole calling site of this helper function, though. Yeah, perhaps. I wanted to build on test_cmp because it has nice output for the failure case, but it is probably simple enough to do: output=$(cat actual) case "$output" in ... *) echo >&2 "unrecognized date: $output" > In any case, would it really be essential to make sure that the > output shows a phony (or a seemingly real) datestamp for this test? > > The primary thing you wanted to achieve by the "gmtime gave us NULL, > let's substitute it with an arbitrary value to avoid dereferencing > the NULL" change was *not* that we see that same arbitrary value > comes out of the system, but that we do not die by attempting to > reference the NULL, I think. Not dying is the primary thing we want > to (and we already do) test, no? I think there are really two separate behaviors we are testing here (and in the surrounding tests): 1. Don't segfault if gmtime returns NULL. 2. Whenever we cannot process a date (either because gmtime fails, or because we fail before even getting the value to gmtime), consistently return the sentinel date (so the reader can easily know it's bogus). Having the test be particular about its output helped us find a case where FreeBSD did not trigger (1), but did trigger (2), by returning a blanked "struct tm". I'm open to the argument that (2) is not worth worrying about that much if it is a hassle to test. But I don't think it is that much hassle (yet, anyway). -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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:25:36PM -0400, Jeff King wrote: > > The primary thing you wanted to achieve by the "gmtime gave us NULL, > > let's substitute it with an arbitrary value to avoid dereferencing > > the NULL" change was *not* that we see that same arbitrary value > > comes out of the system, but that we do not die by attempting to > > reference the NULL, I think. Not dying is the primary thing we want > > to (and we already do) test, no? > > I think there are really two separate behaviors we are testing here (and > in the surrounding tests): > > 1. Don't segfault if gmtime returns NULL. > > 2. Whenever we cannot process a date (either because gmtime fails, or > because we fail before even getting the value to gmtime), > consistently return the sentinel date (so the reader can easily > know it's bogus). > > Having the test be particular about its output helped us find a case > where FreeBSD did not trigger (1), but did trigger (2), by returning a > blanked "struct tm". > > I'm open to the argument that (2) is not worth worrying about that much > if it is a hassle to test. But I don't think it is that much hassle > (yet, anyway). That being said, is the AIX value actually right? I did not look closely at first, but just assumed that it was vaguely right. But: 99 / (86400 * 365) is something like 31 billion years in the future, not 160 million. A real date calculation will have a few tweaks (leap years, etc), but that is orders of magnitude off. So I am not sure that AIX is not actually just giving us utter crap. In that case, the test is not wrong; it's pickiness is actually finding a real problem. But I am not sure it is a problem worth solving. I do not want to get into heuristics deciding whether a particular platform's gmtime output is crap or not. That pushes this into the realm of "it's not worth testing", and we should stick to just testing that we did not segfault. -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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > That being said, is the AIX value actually right? I did not look closely > at first, but just assumed that it was vaguely right. But: > > 99 / (86400 * 365) > > is something like 31 billion years in the future, not 160 million. > A real date calculation will have a few tweaks (leap years, etc), but > that is orders of magnitude off. Assuming my math is right, then here is the most sensible patch, IMHO. -- >8 -- Subject: t4212: loosen far-in-future test for AIX One of the tests in t4212 checks our behavior when we feed gmtime a date so far in the future that it gives up and returns NULL. Some implementations, like AIX, may actually just provide us a bogus result instead. It's not worth it for us to come up with heuristics that guess whether the return value is sensible or not. On good platforms where gmtime reports the problem to us with NULL, we will print the epoch value. On bad platforms, we will print garbage. But our test should be written for the lowest common denominator so that it passes everywhere. Reported-by: Charles Bailey Signed-off-by: Jeff King --- t/t4212-log-corrupt.sh | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh index 85c6df4..bb843ab 100755 --- a/t/t4212-log-corrupt.sh +++ b/t/t4212-log-corrupt.sh @@ -77,11 +77,14 @@ test_expect_success 'date parser recognizes time_t overflow' ' ' # date is within 2^63-1, but enough to choke glibc's gmtime -test_expect_success 'absurdly far-in-future dates produce sentinel' ' +# +# Ideally we would check the output to make sure we replaced it with +# a useful sentinel value, but some platforms will actually hand us back +# a nonsensical date. It is not worth our time to try to evaluate these +# dates, so just make sure we didn't segfault or otherwise abort. +test_expect_success 'absurdly far-in-future dates' ' commit=$(munge_author_date HEAD 99) && - echo "Thu Jan 1 00:00:00 1970 +" >expect && - git log -1 --format=%ad $commit >actual && - test_cmp expect actual + git log -1 --format=%ad $commit ' test_done -- 1.9.1.656.ge8a0637 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/17] ls-files: add --column
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Documentation/git-ls-files.txt | 6 ++
> builtin/ls-files.c | 25 +
> 2 files changed, 31 insertions(+)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 5c1b7f3..cd52461 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -156,6 +156,12 @@ a space) at the start of each line:
> default to color output, same as `--color=never`. This is the
> default.
>
> +--column[=]::
> +--no-column::
> + Display files in columns. See configuration variable column.ui
> + for option syntax.`--column` and `--no-column` without options
Missing space after period.
> + are equivalent to 'always' and 'never' respectively.
> +
> \--::
> Do not interpret any more arguments as options.
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 2857b38..335d3b0 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -15,6 +15,7 @@
> #include "string-list.h"
> #include "pathspec.h"
> #include "color.h"
> +#include "column.h"
>
> static int abbrev;
> static int show_deleted;
> @@ -29,6 +30,7 @@ static int show_valid_bit;
> static int line_terminator = '\n';
> static int debug_mode;
> static int use_color;
> +static unsigned int colopts;
>
> static const char *prefix;
> static int max_prefix_len;
> @@ -39,6 +41,7 @@ static char *ps_matched;
> static const char *with_tree;
> static int exc_given;
> static int exclude_args;
> +static struct string_list output = STRING_LIST_INIT_NODUP;
>
> static const char *tag_cached = "";
> static const char *tag_unmerged = "";
> @@ -66,6 +69,10 @@ static void write_name(struct strbuf *sb, const char *name)
>
> static void strbuf_fputs(struct strbuf *sb, FILE *fp)
> {
> + if (column_active(colopts)) {
> + string_list_append(&output, strbuf_detach(sb, NULL));
> + return;
> + }
> fwrite(sb->buf, sb->len, 1, fp);
> }
>
> @@ -552,6 +559,7 @@ int cmd_ls_files(int argc, const char **argv, const char
> *cmd_prefix)
> OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
> N_("pretend that paths removed since are
> still present")),
> OPT__COLOR(&use_color, N_("show color")),
> + OPT_COLUMN(0, "column", &colopts, N_("show files in
> columns")),
> OPT__ABBREV(&abbrev),
> OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
> OPT_END()
> @@ -596,6 +604,18 @@ int cmd_ls_files(int argc, const char **argv, const char
> *cmd_prefix)
> if (dir.exclude_per_dir)
> exc_given = 1;
>
> + finalize_colopts(&colopts, -1);
> + if (explicitly_enable_column(colopts)) {
> + if (!line_terminator)
> + die(_("--column and -z are incompatible"));
> + if (show_resolve_undo)
> + die(_("--column and --resolve-undo are
> incompatible"));
> + if (debug_mode)
> + die(_("--column and --debug are incompatible"));
> + }
> + if (column_active(colopts))
> + line_terminator = 0;
> +
> if (require_work_tree && !is_inside_work_tree())
> setup_work_tree();
>
> @@ -638,6 +658,11 @@ int cmd_ls_files(int argc, const char **argv, const char
> *cmd_prefix)
> if (show_resolve_undo)
> show_ru_info();
>
> + if (column_active(colopts)) {
> + print_columns(&output, colopts, NULL);
> + string_list_clear(&output, 0);
> + }
> +
> if (ps_matched) {
> int bad;
> bad = report_path_error(ps_matched, &pathspec, prefix);
> --
> 1.9.1.345.ga1a145c
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/17] ls-files: support --max-depth
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Documentation/git-ls-files.txt | 7 +++
> builtin/ls-files.c | 7 +++
> 2 files changed, 14 insertions(+)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index cd52461..3c022eb 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -162,6 +162,13 @@ a space) at the start of each line:
> for option syntax.`--column` and `--no-column` without options
> are equivalent to 'always' and 'never' respectively.
>
> +--max-depth ::
Other options in this file are documented as:
--max-depth=::
> + For each given on command line, descend at most
> + levels of directories. A negative value means no limit.
> + This option is ignored if contains active wildcards.
> + In other words if "a*" matches a directory named "a*",
> + "*" is matched literally so --max-depth is still effective.
Would it be worthwhile to mention the default?
> +
> \--::
> Do not interpret any more arguments as options.
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 335d3b0..8eef423 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -503,6 +503,7 @@ static int option_parse_exclude_standard(const struct
> option *opt,
> int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
> {
> int require_work_tree = 0, show_tag = 0, i;
> + int max_depth = -1;
> const char *max_prefix;
> struct dir_struct dir;
> struct exclude_list *el;
> @@ -560,6 +561,9 @@ int cmd_ls_files(int argc, const char **argv, const char
> *cmd_prefix)
> N_("pretend that paths removed since are
> still present")),
> OPT__COLOR(&use_color, N_("show color")),
> OPT_COLUMN(0, "column", &colopts, N_("show files in
> columns")),
> + { OPTION_INTEGER, 0, "max-depth", &max_depth, N_("depth"),
> + N_("descend at most levels"), PARSE_OPT_NONEG,
> + NULL, 1 },
> OPT__ABBREV(&abbrev),
> OPT_BOOL(0, "debug", &debug_mode, N_("show debugging data")),
> OPT_END()
> @@ -624,8 +628,11 @@ int cmd_ls_files(int argc, const char **argv, const char
> *cmd_prefix)
>
> parse_pathspec(&pathspec, 0,
>PATHSPEC_PREFER_CWD |
> + (max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |
>PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
>prefix, argv);
> + pathspec.max_depth = max_depth;
> + pathspec.recursive = 1;
>
> /* Find common prefix for all pathspec's */
> max_prefix = common_prefix(&pathspec);
> --
> 1.9.1.345.ga1a145c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/19] tree-diff: rework diff_tree interface to be sha1 based
On Tue, Mar 25, 2014 at 10:46:32AM -0700, Junio C Hamano wrote:
> Kirill Smelkov writes:
>
> > What are the downsides of "__" prefix by the way?
>
> Aren't these names reserved for compiler/runtime implementations?
Yes, but there are precedents when people don't obey it widely and
in practice everything works :) Let it be something portable anyway -
how about diff_tree_sha1_low() ?
So corrected patch is below. If such suffixing will be accepted, I will
send follow-up patches corrected similiary.
( or please pull them from
git://repo.or.cz/git/kirr.git y6/tree-diff-walk-multitree )
Thanks,
Kirill
8<
From: Kirill Smelkov
Date: Mon, 24 Feb 2014 20:21:46 +0400
Subject: [PATCH v3] tree-diff: rework diff_tree interface to be sha1 based
In the next commit this will allow to reduce intermediate calls, when
recursing into subtrees - at that stage we know only subtree sha1, and
it is natural for tree walker to start from that phase. For now we do
diff_tree
show_path
diff_tree_sha1
diff_tree
...
and the change will allow to reduce it to
diff_tree
show_path
diff_tree
Also, it will allow to omit allocating strbuf for each subtree, and just
reuse the common strbuf via playing with its len.
The above-mentioned improvements go in the next 2 patches.
The downside is that try_to_follow_renames(), if active, we cause
re-reading of 2 initial trees, which was negligible based on my timings,
and which is outweighed cogently by the upsides.
NOTE To keep with the current interface and semantics, I needed to
rename the function from diff_tree() to diff_tree_sha1(). As
diff_tree_sha1() was already used, and the function we are talking here
is its more low-level helper, let's use convention for suffixing
such helpers with "_low". So the final renaming is
diff_tree() -> diff_tree_sha1_low()
Signed-off-by: Kirill Smelkov
---
Changes since v2:
- renamed __diff_tree_sha1() -> diff_tree_sha1_low() as the former
overlaps with reserved-for-implementation identifiers namespace.
tree-diff.c | 60
1 file changed, 28 insertions(+), 32 deletions(-)
diff --git a/tree-diff.c b/tree-diff.c
index f137f39..0277c5c 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -141,12 +141,17 @@ static void skip_uninteresting(struct tree_desc *t,
struct strbuf *base,
}
}
-static int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
-const char *base_str, struct diff_options *opt)
+static int diff_tree_sha1_low(const unsigned char *old, const unsigned char
*new,
+ const char *base_str, struct diff_options *opt)
{
+ struct tree_desc t1, t2;
+ void *t1tree, *t2tree;
struct strbuf base;
int baselen = strlen(base_str);
+ t1tree = fill_tree_descriptor(&t1, old);
+ t2tree = fill_tree_descriptor(&t2, new);
+
/* Enable recursion indefinitely */
opt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
@@ -159,39 +164,41 @@ static int diff_tree(struct tree_desc *t1, struct
tree_desc *t2,
if (diff_can_quit_early(opt))
break;
if (opt->pathspec.nr) {
- skip_uninteresting(t1, &base, opt);
- skip_uninteresting(t2, &base, opt);
+ skip_uninteresting(&t1, &base, opt);
+ skip_uninteresting(&t2, &base, opt);
}
- if (!t1->size && !t2->size)
+ if (!t1.size && !t2.size)
break;
- cmp = tree_entry_pathcmp(t1, t2);
+ cmp = tree_entry_pathcmp(&t1, &t2);
/* t1 = t2 */
if (cmp == 0) {
if (DIFF_OPT_TST(opt, FIND_COPIES_HARDER) ||
- hashcmp(t1->entry.sha1, t2->entry.sha1) ||
- (t1->entry.mode != t2->entry.mode))
- show_path(&base, opt, t1, t2);
+ hashcmp(t1.entry.sha1, t2.entry.sha1) ||
+ (t1.entry.mode != t2.entry.mode))
+ show_path(&base, opt, &t1, &t2);
- update_tree_entry(t1);
- update_tree_entry(t2);
+ update_tree_entry(&t1);
+ update_tree_entry(&t2);
}
/* t1 < t2 */
else if (cmp < 0) {
- show_path(&base, opt, t1, /*t2=*/NULL);
- update_tree_entry(t1);
+ show_path(&base, opt, &t1, /*t2=*/NULL);
+ update_tree_entry(&t1);
}
/* t1 > t2 */
else {
- show_path(&base, opt, /*t1=*/NULL, t2);
- update_tree_entry(t2);
Re: [PATCH v2 10/17] Add git-ls, a user friendly version of ls-files and more
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy wrote: > This is more user friendly version of ls-files: > > * it's automatically colored and columnized > * it refreshes the index like all porcelain commands > * it defaults to non-recursive behavior like ls > * :(glob) is on by default so '*.c' means a.c but not a/b.c, use > '**/*.c' for that. > * auto pager > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > .gitignore | 1 + > Documentation/config.txt | 10 ++ > Documentation/git-ls.txt (new) | 82 > ++ > Makefile | 1 + > builtin.h | 1 + > builtin/ls-files.c | 70 > command-list.txt | 1 + > git.c | 1 + > 8 files changed, 167 insertions(+) > create mode 100644 Documentation/git-ls.txt > > diff --git a/.gitignore b/.gitignore > index dc600f9..f91af81 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -76,6 +76,7 @@ > /git-init-db > /git-instaweb > /git-log > +/git-ls > /git-ls-files > /git-ls-remote > /git-ls-tree > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 6bca55e..87a6dcf 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -909,6 +909,12 @@ color.status.:: > to red). The values of these variables may be specified as in > color.branch.. > > +color.ls:: > + A boolean to enable/disable color in the output of > + linkgit:git-ls[1]. May be set to `always`, `false` (or > + `never`) or `auto` (or `true`), in which case colors are used > + only when the output is to a terminal. Defaults to false. > + > color.ls.:: > Use customized color for file name colorization. If not set > and the environment variable LS_COLORS is set, color settings > @@ -981,6 +987,10 @@ column.clean:: > Specify the layout when list items in `git clean -i`, which always > shows files and directories in columns. See `column.ui` for details. > > +column.ls:: > + Specify whether to output tag listing in `git ls` in columns. > + See `column.ui` for details. > + > column.status:: > Specify whether to output untracked files in `git status` in columns. > See `column.ui` for details. > diff --git a/Documentation/git-ls.txt b/Documentation/git-ls.txt > new file mode 100644 > index 000..67ca522 > --- /dev/null > +++ b/Documentation/git-ls.txt > @@ -0,0 +1,82 @@ > +git-ls(1) > +=== > + > +NAME > + > +git-ls - List files > + > +SYNOPSIS > + > +[verse] > +'git ls' (--[cached|deleted|others|ignored|unmerged|modified])* > + (-[c|d|o|i|s|u|m])* Don't you have [...] and (...) transposed? The way it's written, "-" and "--" are valid optional arguments. You probably meant: [--(cached|deleted|x|y|z)]... [-(c|d|x|y|z)]... > + [options] [...] However, you also have the generic [options] here, which covers all of the above. It probably would make sense to just use [options] and drop the enumerated list. > +DESCRIPTION > +--- > +List files (by default in current working directory) that are in the > +index. Depending on the chosen options, maybe only modified files in > +working tree are shown, or untracked files... > + > +OPTIONS > +--- > +-c:: > +--cached:: > + Show cached files in the output (default) "in the output" is superfluous. Perhaps drop it from each of the descriptions. > +-d:: > +--deleted:: > + Show deleted files in the output Is this showing only deleted file or including them in the list of files otherwise displayed? It's not clear from the description. Same question for the other options. > +-m:: > +--modified:: > + Show modified files in the output > + > +-o:: > +--others:: > + Show other (i.e. untracked) files in the output > + > +-i:: > +--ignored:: > + Show only ignored files in the output. When showing files in the > + index, print only those matched by an exclude pattern. When > + showing "other" files, show only those matched by an exclude > + pattern. > + > +-u:: > +--unmerged:: > + Show unmerged files in the output (forces --stage) This is the only mention of --stage in this document. Not sure what it's trying to say. > +--color[=]:: > + Color file names. The value must be always (default), never, > + or auto. Same problem mentioned in the other patch. Default is "always"... > + > +--no-color:: > + Turn off coloring, even when the configuration file gives the > + default to color output, same as `--color=never`. This is the > + default. But default is also "never". > + > +--column[=]:: > +--no-column:: > + Display files in columns. See configuration variable column.ui > + for option syntax.`--column` and `--no-column` without options Missing space after period. More below. > + are equiv
Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:40:43PM -0400, Jeff King wrote: > On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > > That being said, is the AIX value actually right? I did not look closely > > at first, but just assumed that it was vaguely right. But: > > > > 99 / (86400 * 365) > > > > is something like 31 billion years in the future, not 160 million. > > A real date calculation will have a few tweaks (leap years, etc), but > > that is orders of magnitude off. > > Assuming my math is right, then here is the most sensible patch, IMHO. > Perhaps hold onto this one for a little while longer. Splitting things out from the test is giving me some inconsistent results, there may be something else going wrong in our environment here. Charles. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 08:36:18PM +, Charles Bailey wrote: > On Wed, Mar 26, 2014 at 03:40:43PM -0400, Jeff King wrote: > > On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > > > > That being said, is the AIX value actually right? I did not look closely > > > at first, but just assumed that it was vaguely right. But: > > > > > > 99 / (86400 * 365) > > > > > > is something like 31 billion years in the future, not 160 million. > > > A real date calculation will have a few tweaks (leap years, etc), but > > > that is orders of magnitude off. > > > > Assuming my math is right, then here is the most sensible patch, IMHO. > > > > Perhaps hold onto this one for a little while longer. Splitting things > out from the test is giving me some inconsistent results, there may be > something else going wrong in our environment here. By the way, can you confirm that this is a 64-bit system? On a 32-bit system, we should be triggering different code paths (we fail at the strtoul level). Those should be checked by the previous tests, but I'd like to make sure. -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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 04:38:30PM -0400, Jeff King wrote: > > By the way, can you confirm that this is a 64-bit system? On a 32-bit > system, we should be triggering different code paths (we fail at the > strtoul level). Those should be checked by the previous tests, but I'd > like to make sure. Yes, we're only building 64-bit at the moment. -- 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 5/5] log: do not segfault on gmtime errors
Jeff King writes: > On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote: > >> > Unlike the FreeBSD thing that René brought up, this is not a problem in >> > the code, but just in the test. So I think our options are basically: >> > >> > 1. Scrap the test as unportable. >> > >> > 2. Hard-code a few expected values. I'd be unsurprised if some other >> > system comes up with a slightly different date in 162396404, so we >> > may end up needing several of these. >> > >> > I think I'd lean towards (2), just because it is testing an actual >> > feature of the code, and I'd like to continue doing so. And while we may >> > end up with a handful of values, there's probably not _that_ many >> > independent implementations of gmtime in the wild. >> >> Or "3. Just make sure that 'git log' does not segfault"? > > That would not test the FreeBSD case, which does not segfault, but > returns a bogus sentinel value. > > I don't know how important that is. This is such a minor feature that it > is not worth a lot of maintenance headache in the test. But I also do > not know if this is going to be the last report, or we will have a bunch > of other systems that need their own special values put into the test. I didn't quite realize that your objective of the change was to signal the failure of gmtime for all implementations, i.e. both (1) the ones that signal an error by giving NULL back to us and (2) the ones that fail to signal an error but leave bogus result (FreeBSD, but it appears AIX might be also giving us another bogus value), by using a single sane sentinel value. If we can do that consistently on every platform, that would indeed be great. But if that is the case, we should not have to maintain special case values in the test code, I would think. The test instead should expect the output to have that single sentinel value, and if the workaround code fails to detect a breakage in the platform gmtime(), the special case logic to catch these platform-specific breakages should go that "timestamp that cannot be grokked by gmtime---replace it with a sentinel" logic, no? -- 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 18/25] setup.c: support multi-checkout repo setup
Duy Nguyen writes: > On Tue, Mar 25, 2014 at 08:52:13PM +0700, Duy Nguyen wrote: >> On Mon, Mar 24, 2014 at 9:52 PM, Torsten Bögershausen wrote: >> > Did I report that t1501 fails when there is a softlink in $PWD ? >> > /home/tb/projects is a softlink to /disc5/projects/ >> >> Yes you did and I forgot. I have fixed it, running test suite and will >> send the reroll soon. > > Junio, it seems you have picked up all minor changes after > v5. Resending the whole series for one fix seems overkill. Could you > just --autosquash this one in? Gladly; thanks for a quick turnaround. > -- 8< -- > Subject: [PATCH] fixup! setup.c: support multi-checkout repo setup > > --- > t/t1501-worktree.sh | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh > index 2ac4424..e6ac7a4 100755 > --- a/t/t1501-worktree.sh > +++ b/t/t1501-worktree.sh > @@ -359,7 +359,7 @@ test_expect_success 'GIT_DIR set (1)' ' > ( > cd work && > GIT_DIR=../gitfile git rev-parse --git-common-dir >actual && > - echo "$TRASH_DIRECTORY/repo.git" >expect && > + test-path-utils real_path "$TRASH_DIRECTORY/repo.git" >expect && > test_cmp expect actual > ) > ' > @@ -370,7 +370,7 @@ test_expect_success 'GIT_DIR set (2)' ' > ( > cd work && > GIT_DIR=../gitfile git rev-parse --git-common-dir >actual && > - echo "$TRASH_DIRECTORY/repo.git" >expect && > + test-path-utils real_path "$TRASH_DIRECTORY/repo.git" >expect && > test_cmp expect actual > ) > ' > @@ -381,7 +381,7 @@ test_expect_success 'Auto discovery' ' > ( > cd work && > git rev-parse --git-common-dir >actual && > - echo "$TRASH_DIRECTORY/repo.git" >expect && > + test-path-utils real_path "$TRASH_DIRECTORY/repo.git" >expect && > test_cmp expect actual && > echo haha >data1 && > git add data1 && > @@ -399,7 +399,7 @@ test_expect_success '$GIT_DIR/common overrides > core.worktree' ' > ( > cd work && > git rev-parse --git-common-dir >actual && > - echo "$TRASH_DIRECTORY/repo.git" >expect && > + test-path-utils real_path "$TRASH_DIRECTORY/repo.git" >expect && > test_cmp expect actual && > echo haha >data2 && > git add data2 && -- 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 5/5] log: do not segfault on gmtime errors
On Wed, Mar 26, 2014 at 02:01:21PM -0700, Junio C Hamano wrote: > > I don't know how important that is. This is such a minor feature that it > > is not worth a lot of maintenance headache in the test. But I also do > > not know if this is going to be the last report, or we will have a bunch > > of other systems that need their own special values put into the test. > > I didn't quite realize that your objective of the change was to > signal the failure of gmtime for all implementations, I didn't realize it at the time I wrote the test, either. It was my goal to recognize such failures, but I didn't now at the time that there were failures that would be signaled by anything besides a NULL return. > the ones that signal an error by giving NULL back to us and (2) the > ones that fail to signal an error but leave bogus result (FreeBSD, > but it appears AIX might be also giving us another bogus value), by > using a single sane sentinel value. If we can do that consistently > on every platform, that would indeed be great. Agreed. I am not sure we can do so. For FreeBSD, I think it is not hard. I am not sure what the pattern is for AIX. I am hoping Charles will reveal some pattern, but there may not be one. > But if that is the case, we should not have to maintain special case > values in the test code, I would think. Right. All of my suggestions to accommodate special values in the test were assuming that the system gmtime was smarter than we are. That it produced a good value for this crazy time and _didn't_ fail. But after having done the basic math, I don't think that is what is going on here. > The test instead should expect the output to have that single sentinel > value, and if the workaround code fails to detect a breakage in the > platform gmtime(), the special case logic to catch these > platform-specific breakages should go that "timestamp that cannot be > grokked by gmtime---replace it with a sentinel" logic, no? Yes, exactly. That is the preferable solution, if we can come up with such logic. Personally I am not optimistic. The fallback is to accept that we cannot cover all cases, and just loosen the test (i.e., the second patch I posted today). -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] apply --ignore-space-change: lines with and without leading whitespaces do not match
The fuzzy_matchlines() function is used when attempting to resurrect
a patch that is whitespace-damaged, or when applying a patch that
was produced against an old codebase to the codebase after
indentation change.
The patch may want to change a line "a_bc" ("_" is used throught
this description for a whitespace to make it stand out) in the
original into something else, and we may not find "a_bc" in the
current source, but there may be "a__bc" (two spaces instead of one
the whitespace-damaged patch claims to expect). By ignoring the
amount of whitespaces, it forces "git apply" to consider that "a_bc"
in the broken patch meant to refer to "a__bc" in reality.
However, the implementation special cases a run of whitespaces at
the beginning of a line and makes "abc" match "_abc", even though a
whitespace in the middle of string never matches a 0-width gap,
e.g. "a_bc" does not match "abc". A run of whitespace at the end of
one string does not match a 0-width end of line on the other line,
either, e.g. "abc_" does not match "abc".
Fix this inconsistency by making the code skip leading whitespaces
only when both strings begin with a whitespace. This makes the
option mean the same as the option of the same name in "diff" and
"git diff".
Note that I am not sure if anybody sane should use this option in
the first place. The fuzzy match logic may be able to find the
original line that the patch author may have meant to touch because
it does not fully trust what the original lines say (i.e. context
lines prefixed by " " and old lines prefixed by "-" does not have to
exactly match the contents the patch is applied to). There is no
reason for us to trust what the replacement lines (i.e. new lines
prefixed by "+") say, either, but with this option enabled, we end
up copying these new lines with suspicious whitespace distributions
literally into the patched result. But as long as we keep it, we
should make it do its insane thing consistently.
Signed-off-by: Junio C Hamano
---
builtin/apply.c| 12 +++-
t/t4107-apply-ignore-whitespace.sh | 12
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 0189523..8b9d3de 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -300,11 +300,13 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
while ((*last2 == '\r') || (*last2 == '\n'))
last2--;
- /* skip leading whitespace */
- while (isspace(*s1) && (s1 <= last1))
- s1++;
- while (isspace(*s2) && (s2 <= last2))
- s2++;
+ /* skip leading whitespaces, if both begin with whitespace */
+ if (s1 <= last1 && s2 <= last2 && isspace(*s1) && isspace(*s2)) {
+ while (isspace(*s1) && (s1 <= last1))
+ s1++;
+ while (isspace(*s2) && (s2 <= last2))
+ s2++;
+ }
/* early return if both lines are empty */
if ((s1 > last1) && (s2 > last2))
return 1;
diff --git a/t/t4107-apply-ignore-whitespace.sh
b/t/t4107-apply-ignore-whitespace.sh
index b04fc8f..9e29b52 100755
--- a/t/t4107-apply-ignore-whitespace.sh
+++ b/t/t4107-apply-ignore-whitespace.sh
@@ -111,7 +111,6 @@ sed -e 's/T//g' > main.c.final <<\EOF
#include
void print_int(int num);
-T/* a comment */
int func(int num);
int main() {
@@ -154,7 +153,8 @@ test_expect_success 'patch2 reverse applies with
--ignore-space-change' '
git config apply.ignorewhitespace change
test_expect_success 'patch2 applies (apply.ignorewhitespace = change)' '
- git apply patch2.patch
+ git apply patch2.patch &&
+ test_cmp main.c.final main.c
'
test_expect_success 'patch3 fails (missing string at EOL)' '
@@ -165,12 +165,8 @@ test_expect_success 'patch4 fails (missing EOL at EOF)' '
test_must_fail git apply patch4.patch
'
-test_expect_success 'patch5 applies (leading whitespace)' '
- git apply patch5.patch
-'
-
-test_expect_success 'patches do not mangle whitespace' '
- test_cmp main.c main.c.final
+test_expect_success 'patch5 fails (leading whitespace differences matter)' '
+ test_must_fail git apply patch5.patch
'
test_expect_success 're-create file (with --ignore-whitespace)' '
--
1.9.1-497-g6f5ca27
--
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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > That being said, is the AIX value actually right? I did not look closely > at first, but just assumed that it was vaguely right. But: > > 99 / (86400 * 365) > > is something like 31 billion years in the future, not 160 million. > A real date calculation will have a few tweaks (leap years, etc), but > that is orders of magnitude off. Well, this is embarrassing, while moving this through the corporate firewall (aka typing on one machine while looking at another), I munged the date. It still doesn't seem right but at least you can now see the actual data. I stopped the test with --immediate and found the dangling commit that the test created and dumped it with the previous version of Git (well a 1.8.5.5 build) ibm: trash directory.t4212-log-corrupt $ git log -1 --pretty=raw 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca tree 64fd3796c57084e7b8cbae358ce37970b8e954f6 author A U Thor 99 -0700 committer C O Mitter 1112911993 -0700 foo ibm: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca Author: A U Thor Date: Thu Oct 24 18:46:39 1623969404 -0700 foo Same commit but dumped from a linux machine: linux: trash directory.t4212-log-corrupt $ git log -1 1fc17e734e4487c31bdfe05bb3d15618b69c4dca commit 1fc17e734e4487c31bdfe05bb3d15618b69c4dca Author: A U Thor Date: (null) foo Charles. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/19] tree-diff: rework diff_tree interface to be sha1 based
Kirill Smelkov writes: > On Tue, Mar 25, 2014 at 10:46:32AM -0700, Junio C Hamano wrote: >> Kirill Smelkov writes: >> >> > What are the downsides of "__" prefix by the way? >> >> Aren't these names reserved for compiler/runtime implementations? > > Yes, but there are precedents when people don't obey it widely and > in practice everything works :) I think you are alluding to the practice in the Linux kernel, but their requirement is vastly different---their product do not even link with libc and they always compile with specific selected versions of gcc, no? > Let it be something portable anyway - > how about diff_tree_sha1_low() ? Sure. As this is a file-scope static, I do not think the exact naming matters that much. Just FYI, we seem to use ll_ prefix (standing for low-level) in some places. 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 v2 19/27] refs: Add a concept of a reference transaction
On 03/26/2014 07:39 PM, Brad King wrote: > On 03/24/2014 01:56 PM, Michael Haggerty wrote: >> +void ref_transaction_update(struct ref_transaction *transaction, >> +const char *refname, >> +unsigned char *new_sha1, unsigned char *old_sha1, >> +int flags, int have_old); > [snip] >> +void ref_transaction_create(struct ref_transaction *transaction, >> +const char *refname, >> +unsigned char *new_sha1, >> +int flags); > [snip] >> +void ref_transaction_delete(struct ref_transaction *transaction, >> +const char *refname, >> +unsigned char *old_sha1, >> +int flags, int have_old); > > Perhaps we also need: > > void ref_transaction_verify(struct ref_transaction *transaction, > const char *refname, > unsigned char *old_sha1, > int flags, int have_old); > > as equivalent to the "verify" command in "update-ref --stdin"? Yes. That's already on my todo list for a future batch of patches. But first I was going to beef up the ref_update structure to handle verify actions directly rather than as updates with oldvalue==newvalue, probably by turning has_old into a flag with HAS_OLD and HAS_NEW bits. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 v2 00/27] Clean up update-refs --stdin and implement ref_transaction
On 03/26/2014 07:39 PM, Brad King wrote: > On 03/24/2014 01:56 PM, Michael Haggerty wrote: >> Changes relative to v1: >> >> * Rename the functions associated with ref_transactions to be more >> reminiscent of database transactions: >> >> * create_ref_transaction() -> ref_transaction_begin() >> * free_ref_transaction() -> ref_transaction_rollback() >> * queue_update_ref() -> ref_transaction_update() >> * queue_create_ref() -> ref_transaction_create() >> * queue_delete_ref() -> ref_transaction_delete() >> * commit_ref_transaction() -> ref_transaction_commit() > > Those new names look better. > >> * Fix backwards compatibility of "git update-ref --stdin -z"'s >> handling of the "create" command: allow to be the empty >> string, treating it the same zeros. But deprecate this usage. > > The changes related to that look good. The new documentation is > much clearer than my old wording. > > Series v2 looks good to me except for my responses to individual > commits. Thanks a lot for the review. Your other two comments are correct, of course, and I will fix them if there needs to be a re-roll. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 09:22:27PM +, Charles Bailey wrote: > On Wed, Mar 26, 2014 at 03:33:59PM -0400, Jeff King wrote: > > > > That being said, is the AIX value actually right? I did not look closely > > at first, but just assumed that it was vaguely right. But: > > > > 99 / (86400 * 365) > > > > is something like 31 billion years in the future, not 160 million. > > A real date calculation will have a few tweaks (leap years, etc), but > > that is orders of magnitude off. > > Well, this is embarrassing, while moving this through the corporate > firewall (aka typing on one machine while looking at another), I > munged the date. It still doesn't seem right but at least you can now > see the actual data. Hmm, so the year you got is actually: 1623969404. That still seems off to me by a factor 20. I don't know if this is really worth digging into that much further, but I wonder what you would get for timestamps of: 9 999 etc. Do we start generating weird values at some particular size? Or is AIX gmtime really more clever than I am, and is accounting for wobble of the Earth or something over the next billion years? -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 03/12] Move lower case functions into wrapper.c
The lowercase() function from config.c and the xstrdup_tolower()
function from daemon.c can benefit from being moved to the same
place because this way the latter can use the former.
Also let's make them available globally so we can use them from
other places like trailer.c.
Signed-off-by: Christian Couder
---
config.c | 6 --
daemon.c | 8
git-compat-util.h | 4
wrapper.c | 14 ++
4 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/config.c b/config.c
index 314d8ee..dde128e 100644
--- a/config.c
+++ b/config.c
@@ -146,12 +146,6 @@ int git_config_include(const char *var, const char *value,
void *data)
return ret;
}
-static void lowercase(char *p)
-{
- for (; *p; p++)
- *p = tolower(*p);
-}
-
void git_config_push_parameter(const char *text)
{
struct strbuf env = STRBUF_INIT;
diff --git a/daemon.c b/daemon.c
index eba1255..f9c63e9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -475,14 +475,6 @@ static void make_service_overridable(const char *name, int
ena)
die("No such service %s", name);
}
-static char *xstrdup_tolower(const char *str)
-{
- char *p, *dup = xstrdup(str);
- for (p = dup; *p; p++)
- *p = tolower(*p);
- return dup;
-}
-
static void parse_host_and_port(char *hostport, char **host,
char **port)
{
diff --git a/git-compat-util.h b/git-compat-util.h
index 614a5e9..2397706 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -727,4 +727,8 @@ void warn_on_inaccessible(const char *path);
/* Get the passwd entry for the UID of the current process. */
struct passwd *xgetpwuid_self(void);
+/* Lowercase strings */
+extern void lowercase(char *str);
+extern char *xstrdup_tolower(const char *str);
+
#endif
diff --git a/wrapper.c b/wrapper.c
index 0cc5636..c46026a 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -455,3 +455,17 @@ struct passwd *xgetpwuid_self(void)
errno ? strerror(errno) : _("no such user"));
return pw;
}
+
+void lowercase(char *p)
+{
+ for (; *p; p++)
+ *p = tolower(*p);
+}
+
+char *xstrdup_tolower(const char *str)
+{
+ char *dup = xstrdup(str);
+ lowercase(dup);
+ return dup;
+}
+
--
1.9.0.164.g3aa33cd.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 v8 06/12] trailer: parse trailers from stdin
Read trailers from stdin, parse them and put the result into a doubly linked
list.
Signed-off-by: Christian Couder
---
trailer.c | 76 +++
1 file changed, 76 insertions(+)
diff --git a/trailer.c b/trailer.c
index 1871843..3993840 100644
--- a/trailer.c
+++ b/trailer.c
@@ -50,6 +50,14 @@ static size_t alnum_len(const char *buf, size_t len)
return len;
}
+static inline int contains_only_spaces(const char *str)
+{
+ const char *s = str;
+ while (*s && isspace(*s))
+ s++;
+ return !*s;
+}
+
static void free_trailer_item(struct trailer_item *item)
{
free(item->conf.name);
@@ -501,3 +509,71 @@ static struct trailer_item *process_command_line_args(int
argc, const char **arg
return arg_tok_first;
}
+
+static struct strbuf **read_stdin(void)
+{
+ struct strbuf **lines;
+ struct strbuf sb = STRBUF_INIT;
+
+ if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+ die_errno(_("could not read from stdin"));
+
+ lines = strbuf_split(&sb, '\n');
+
+ strbuf_release(&sb);
+
+ return lines;
+}
+
+/*
+ * Return the the (0 based) index of the first trailer line
+ * or the line count if there are no trailers.
+ */
+static int find_trailer_start(struct strbuf **lines)
+{
+ int start, empty = 1, count = 0;
+
+ /* Get the line count */
+ while (lines[count])
+ count++;
+
+ /*
+* Get the start of the trailers by looking starting from the end
+* for a line with only spaces before lines with one ':'.
+*/
+ for (start = count - 1; start >= 0; start--) {
+ if (contains_only_spaces(lines[start]->buf)) {
+ if (empty)
+ continue;
+ return start + 1;
+ }
+ if (strchr(lines[start]->buf, ':')) {
+ if (empty)
+ empty = 0;
+ continue;
+ }
+ return count;
+ }
+
+ return empty ? count : start + 1;
+}
+
+static void process_stdin(struct trailer_item **in_tok_first,
+ struct trailer_item **in_tok_last)
+{
+ struct strbuf **lines = read_stdin();
+ int start = find_trailer_start(lines);
+ int i;
+
+ /* Print non trailer lines as is */
+ for (i = 0; lines[i] && i < start; i++)
+ printf("%s", lines[i]->buf);
+
+ /* Parse trailer lines */
+ for (i = start; lines[i]; i++) {
+ struct trailer_item *new = create_trailer_item(lines[i]->buf);
+ add_trailer_item(in_tok_first, in_tok_last, new);
+ }
+
+ strbuf_list_free(lines);
+}
--
1.9.0.164.g3aa33cd.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 v8 00/12] Add interpret-trailers builtin
This patch series implements a new command: git interpret-trailers and an infrastructure to process trailers that can be reused, for example in "commit.c". 1) Rationale: This command should help with RFC 822 style headers, called "trailers", that are found at the end of commit messages. (Note that these headers do not follow and are not intended to follow many rules that are in RFC 822. For example they do not follow the line breaking rules, the encoding rules and probably many other rules.) For a long time, these trailers have become a de facto standard way to add helpful information into commit messages. Until now git commit has only supported the well known "Signed-off-by: " trailer, that is used by many projects like the Linux kernel and Git. It is better to implement features for these trailers first in a new command rather than in builtin/commit.c, because this way the prepare-commit-msg and commit-msg hooks can reuse this command. 2) Current state: Currently the usage string of this command is: git interpret-trailers [--trim-empty] [([(=|:)])...] The following features are implemented: - the result is printed on stdout - the [[=]>] arguments are interpreted - a commit message read from stdin is interpreted - the "trailer..key" options in the config are interpreted - the "trailer..where" options are interpreted - the "trailer..ifExist" options are interpreted - the "trailer..ifMissing" options are interpreted - the "trailer..command" config works - $ARG can be used in commands - there are some tests - there is some documentation The following features are planned but not yet implemented: - add more tests related to commands - add examples in documentation - integration with "git commit" Possible improvements: - support GIT_COMMIT_PROTO env variable in commands 3) Changes since version 7, thanks to Junio: * improved handling of empty trailer token * clearer way to create 'expected' files in tests * other small test cleanups * improved commit message * new way to parse config keys * strcasecmp() is not used anymore in some config related functions Some values from the config file are lowercased instead. To enable that a new patch (3/12) is introduced to rationalize lowercase related functions. I am not very happy with these changes. Christian Couder (12): Add data structures and basic functions for commit trailers trailer: process trailers from stdin and arguments Move lower case functions into wrapper.c trailer: read and process config information trailer: process command line trailer arguments trailer: parse trailers from stdin trailer: put all the processing together and print trailer: add interpret-trailers command trailer: add tests for "git interpret-trailers" trailer: execute command from 'trailer..command' trailer: add tests for commands in config file Documentation: add documentation for 'git interpret-trailers' .gitignore | 1 + Documentation/git-interpret-trailers.txt | 123 ++ Makefile | 2 + builtin.h| 1 + builtin/interpret-trailers.c | 33 ++ config.c | 6 - daemon.c | 8 - git-compat-util.h| 4 + git.c| 1 + t/t7513-interpret-trailers.sh| 407 ++ trailer.c| 691 +++ trailer.h| 6 + wrapper.c| 14 + 13 files changed, 1283 insertions(+), 14 deletions(-) create mode 100644 Documentation/git-interpret-trailers.txt create mode 100644 builtin/interpret-trailers.c create mode 100755 t/t7513-interpret-trailers.sh create mode 100644 trailer.c create mode 100644 trailer.h -- 1.9.0.164.g3aa33cd.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 v8 04/12] trailer: read and process config information
Read the configuration to get trailer information, and then process
it and storing it in a doubly linked list.
The config information is stored in the list whose first item is
pointed to by:
static struct trailer_item *first_conf_item;
Signed-off-by: Christian Couder
---
trailer.c | 153 ++
1 file changed, 153 insertions(+)
diff --git a/trailer.c b/trailer.c
index 52108c2..b5a0943 100644
--- a/trailer.c
+++ b/trailer.c
@@ -25,6 +25,8 @@ struct trailer_item {
struct conf_info conf;
};
+static struct trailer_item *first_conf_item;
+
static int same_token(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
{
return !strncasecmp(a->token, b->token, alnum_len);
@@ -245,3 +247,154 @@ static void process_trailers_lists(struct trailer_item
**in_tok_first,
apply_arg_if_missing(in_tok_first, in_tok_last, arg_tok);
}
}
+
+static int set_where(struct conf_info *item, const char *value)
+{
+ if (!strcmp("after", value))
+ item->where = WHERE_AFTER;
+ else if (!strcmp("before", value))
+ item->where = WHERE_BEFORE;
+ else
+ return -1;
+ return 0;
+}
+
+static int set_if_exists(struct conf_info *item, const char *value)
+{
+ if (!strcmp("addifdifferent", value))
+ item->if_exists = EXISTS_ADD_IF_DIFFERENT;
+ else if (!strcmp("addifdifferentneighbor", value))
+ item->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+ else if (!strcmp("add", value))
+ item->if_exists = EXISTS_ADD;
+ else if (!strcmp("overwrite", value))
+ item->if_exists = EXISTS_OVERWRITE;
+ else if (!strcmp("donothing", value))
+ item->if_exists = EXISTS_DO_NOTHING;
+ else
+ return -1;
+ return 0;
+}
+
+static int set_if_missing(struct conf_info *item, const char *value)
+{
+ if (!strcmp("donothing", value))
+ item->if_missing = MISSING_DO_NOTHING;
+ else if (!strcmp("add", value))
+ item->if_missing = MISSING_ADD;
+ else
+ return -1;
+ return 0;
+}
+
+static struct trailer_item *get_conf_item(const char *name)
+{
+ struct trailer_item *item;
+ struct trailer_item *previous;
+
+ /* Look up item with same name */
+ for (previous = NULL, item = first_conf_item;
+item;
+previous = item, item = item->next) {
+ if (!strcasecmp(item->conf.name, name))
+ return item;
+ }
+
+ /* Item does not already exists, create it */
+ item = xcalloc(sizeof(struct trailer_item), 1);
+ item->conf.name = xstrdup(name);
+
+ if (!previous)
+ first_conf_item = item;
+ else {
+ previous->next = item;
+ item->previous = previous;
+ }
+
+ return item;
+}
+
+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,
+TRAILER_IF_EXISTS, TRAILER_IF_MISSING };
+
+static struct {
+ const char *name;
+ enum trailer_info_type type;
+} trailer_config_items[] = {
+ { "key", TRAILER_KEY },
+ { "command", TRAILER_COMMAND },
+ { "where", TRAILER_WHERE },
+ { "ifexists", TRAILER_IF_EXISTS },
+ { "ifmissing", TRAILER_IF_MISSING }
+};
+
+static int git_trailer_config(const char *conf_key, const char *value, void
*cb)
+{
+ const char *trailer_item, *variable_name;
+ struct trailer_item *item;
+ struct conf_info *conf;
+ char *name = NULL;
+ enum trailer_info_type type;
+ char *lowercase_value;
+ int i;
+
+ trailer_item = skip_prefix(conf_key, "trailer.");
+ if (!trailer_item)
+ return 0;
+
+ variable_name = strrchr(trailer_item, '.');
+ if (!variable_name) {
+ warning(_("two level trailer config variable %s"), conf_key);
+ return 0;
+ }
+
+ variable_name++;
+ for (i = 0; i < ARRAY_SIZE(trailer_config_items); i++) {
+ if (strcmp(trailer_config_items[i].name, variable_name))
+ continue;
+ name = xstrndup(trailer_item, variable_name - trailer_item -1);
+ type = trailer_config_items[i].type;
+ break;
+ }
+
+ if (!name)
+ return 0;
+
+ item = get_conf_item(name);
+ conf = &item->conf;
+ free(name);
+
+ switch (type) {
+ case TRAILER_KEY:
+ if (conf->key)
+ warning(_("more than one %s"), conf_key);
+ conf->key = xstrdup(value);
+ break;
+ case TRAILER_COMMAND:
+ if (conf->command)
+ warning(_("more than one %s"), conf_key);
+ conf->command = xstrdup(value);
+ break;
+ case TRAILER_WHERE:
+
[PATCH v8 07/12] trailer: put all the processing together and print
This patch adds the process_trailers() function that
calls all the previously added processing functions
and then prints the results on the standard output.
Signed-off-by: Christian Couder
---
trailer.c | 49 +
trailer.h | 6 ++
2 files changed, 55 insertions(+)
create mode 100644 trailer.h
diff --git a/trailer.c b/trailer.c
index 3993840..5bb29ae 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "trailer.h"
/*
* Copyright (c) 2013, 2014 Christian Couder
*/
@@ -68,6 +69,26 @@ static void free_trailer_item(struct trailer_item *item)
free(item);
}
+static void print_tok_val(const char *tok, const char *val)
+{
+ char c = tok[strlen(tok) - 1];
+ if (isalnum(c))
+ printf("%s: %s\n", tok, val);
+ else if (isspace(c) || c == '#')
+ printf("%s%s\n", tok, val);
+ else
+ printf("%s %s\n", tok, val);
+}
+
+static void print_all(struct trailer_item *first, int trim_empty)
+{
+ struct trailer_item *item;
+ for (item = first; item; item = item->next) {
+ if (!trim_empty || strlen(item->value) > 0)
+ print_tok_val(item->token, item->value);
+ }
+}
+
static void add_arg_to_input_list(struct trailer_item *in_tok,
struct trailer_item *arg_tok)
{
@@ -577,3 +598,31 @@ static void process_stdin(struct trailer_item
**in_tok_first,
strbuf_list_free(lines);
}
+
+static void free_all(struct trailer_item **first)
+{
+ while (*first) {
+ struct trailer_item *item = remove_first(first);
+ free_trailer_item(item);
+ }
+}
+
+void process_trailers(int trim_empty, int argc, const char **argv)
+{
+ struct trailer_item *in_tok_first = NULL;
+ struct trailer_item *in_tok_last = NULL;
+ struct trailer_item *arg_tok_first;
+
+ git_config(git_trailer_config, NULL);
+
+ /* Print the non trailer part of stdin */
+ process_stdin(&in_tok_first, &in_tok_last);
+
+ arg_tok_first = process_command_line_args(argc, argv);
+
+ process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
+
+ print_all(in_tok_first, trim_empty);
+
+ free_all(&in_tok_first);
+}
diff --git a/trailer.h b/trailer.h
new file mode 100644
index 000..9323b1e
--- /dev/null
+++ b/trailer.h
@@ -0,0 +1,6 @@
+#ifndef TRAILER_H
+#define TRAILER_H
+
+void process_trailers(int trim_empty, int argc, const char **argv);
+
+#endif /* TRAILER_H */
--
1.9.0.164.g3aa33cd.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 v8 10/12] trailer: execute command from 'trailer..command'
Let the user specify a command that will give on its standard output
the value to use for the specified trailer.
Signed-off-by: Christian Couder
---
trailer.c | 63 +++
1 file changed, 63 insertions(+)
diff --git a/trailer.c b/trailer.c
index 5bb29ae..eeb2f86 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,4 +1,5 @@
#include "cache.h"
+#include "run-command.h"
#include "trailer.h"
/*
* Copyright (c) 2013, 2014 Christian Couder
@@ -13,11 +14,14 @@ struct conf_info {
char *name;
char *key;
char *command;
+ unsigned command_uses_arg : 1;
enum action_where where;
enum action_if_exists if_exists;
enum action_if_missing if_missing;
};
+#define TRAILER_ARG_STRING "$ARG"
+
struct trailer_item {
struct trailer_item *previous;
struct trailer_item *next;
@@ -59,6 +63,13 @@ static inline int contains_only_spaces(const char *str)
return !*s;
}
+static inline void strbuf_replace(struct strbuf *sb, const char *a, const char
*b)
+{
+ const char *ptr = strstr(sb->buf, a);
+ if (ptr)
+ strbuf_splice(sb, ptr - sb->buf, strlen(a), b, strlen(b));
+}
+
static void free_trailer_item(struct trailer_item *item)
{
free(item->conf.name);
@@ -403,6 +414,7 @@ static int git_trailer_config(const char *conf_key, const
char *value, void *cb)
if (conf->command)
warning(_("more than one %s"), conf_key);
conf->command = xstrdup(value);
+ conf->command_uses_arg = !!strstr(conf->command,
TRAILER_ARG_STRING);
break;
case TRAILER_WHERE:
lowercase_value = xstrdup_tolower(value);
@@ -445,6 +457,44 @@ static int parse_trailer(struct strbuf *tok, struct strbuf
*val, const char *tra
return 0;
}
+static int read_from_command(struct child_process *cp, struct strbuf *buf)
+{
+ if (run_command(cp))
+ return error("running trailer command '%s' failed",
cp->argv[0]);
+ if (strbuf_read(buf, cp->out, 1024) < 1)
+ return error("reading from trailer command '%s' failed",
cp->argv[0]);
+ strbuf_trim(buf);
+ return 0;
+}
+
+static const char *apply_command(const char *command, const char *arg)
+{
+ struct strbuf cmd = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
+ struct child_process cp;
+ const char *argv[] = {NULL, NULL};
+ const char *result = "";
+
+ strbuf_addstr(&cmd, command);
+ if (arg)
+ strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
+
+ argv[0] = cmd.buf;
+ memset(&cp, 0, sizeof(cp));
+ cp.argv = argv;
+ cp.env = local_repo_env;
+ cp.no_stdin = 1;
+ cp.out = -1;
+ cp.use_shell = 1;
+
+ if (read_from_command(&cp, &buf))
+ strbuf_release(&buf);
+ else
+ result = strbuf_detach(&buf, NULL);
+
+ strbuf_release(&cmd);
+ return result;
+}
static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
{
@@ -467,6 +517,10 @@ static struct trailer_item *new_trailer_item(struct
trailer_item *conf_item,
duplicate_conf(&new->conf, &conf_item->conf);
new->token = xstrdup(conf_item->conf.key);
free(tok);
+ if (conf_item->conf.command_uses_arg || !val) {
+ new->value = apply_command(conf_item->conf.command,
val);
+ free(val);
+ }
} else
new->token = tok;
@@ -522,12 +576,21 @@ static struct trailer_item *process_command_line_args(int
argc, const char **arg
int i;
struct trailer_item *arg_tok_first = NULL;
struct trailer_item *arg_tok_last = NULL;
+ struct trailer_item *item;
for (i = 0; i < argc; i++) {
struct trailer_item *new = create_trailer_item(argv[i]);
add_trailer_item(&arg_tok_first, &arg_tok_last, new);
}
+ /* Add conf commands that don't use $ARG */
+ for (item = first_conf_item; item; item = item->next) {
+ if (item->conf.command && !item->conf.command_uses_arg) {
+ struct trailer_item *new = new_trailer_item(item, NULL,
NULL);
+ add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ }
+ }
+
return arg_tok_first;
}
--
1.9.0.164.g3aa33cd.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 v8 02/12] trailer: process trailers from stdin and arguments
Implement the logic to process trailers from stdin and arguments.
At the beginning trailers from stdin are in their own in_tok
doubly linked list, and trailers from arguments are in their own
arg_tok doubly linked list.
The lists are traversed and when an arg_tok should be "applied",
it is removed from its list and inserted into the in_tok list.
Signed-off-by: Christian Couder
---
trailer.c | 198 ++
1 file changed, 198 insertions(+)
diff --git a/trailer.c b/trailer.c
index db93a63..52108c2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -47,3 +47,201 @@ static size_t alnum_len(const char *buf, size_t len)
len--;
return len;
}
+
+static void free_trailer_item(struct trailer_item *item)
+{
+ free(item->conf.name);
+ free(item->conf.key);
+ free(item->conf.command);
+ free((char *)item->token);
+ free((char *)item->value);
+ free(item);
+}
+
+static void add_arg_to_input_list(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok)
+{
+ if (arg_tok->conf.where == WHERE_AFTER) {
+ arg_tok->next = in_tok->next;
+ in_tok->next = arg_tok;
+ arg_tok->previous = in_tok;
+ if (arg_tok->next)
+ arg_tok->next->previous = arg_tok;
+ } else {
+ arg_tok->previous = in_tok->previous;
+ in_tok->previous = arg_tok;
+ arg_tok->next = in_tok;
+ if (arg_tok->previous)
+ arg_tok->previous->next = arg_tok;
+ }
+}
+
+static int check_if_different(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len, int check_all)
+{
+ enum action_where where = arg_tok->conf.where;
+ do {
+ if (!in_tok)
+ return 1;
+ if (same_trailer(in_tok, arg_tok, alnum_len))
+ return 0;
+ /*
+* if we want to add a trailer after another one,
+* we have to check those before this one
+*/
+ in_tok = (where == WHERE_AFTER) ? in_tok->previous :
in_tok->next;
+ } while (check_all);
+ return 1;
+}
+
+static void apply_arg_if_exists(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len)
+{
+ switch (arg_tok->conf.if_exists) {
+ case EXISTS_DO_NOTHING:
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_OVERWRITE:
+ free((char *)in_tok->value);
+ in_tok->value = xstrdup(arg_tok->value);
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_ADD:
+ add_arg_to_input_list(in_tok, arg_tok);
+ break;
+ case EXISTS_ADD_IF_DIFFERENT:
+ if (check_if_different(in_tok, arg_tok, alnum_len, 1))
+ add_arg_to_input_list(in_tok, arg_tok);
+ else
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
+ if (check_if_different(in_tok, arg_tok, alnum_len, 0))
+ add_arg_to_input_list(in_tok, arg_tok);
+ else
+ free_trailer_item(arg_tok);
+ break;
+ }
+}
+
+static void remove_from_list(struct trailer_item *item,
+struct trailer_item **first)
+{
+ if (item->next)
+ item->next->previous = item->previous;
+ if (item->previous)
+ item->previous->next = item->next;
+ else
+ *first = item->next;
+}
+
+static struct trailer_item *remove_first(struct trailer_item **first)
+{
+ struct trailer_item *item = *first;
+ *first = item->next;
+ if (item->next) {
+ item->next->previous = NULL;
+ item->next = NULL;
+ }
+ return item;
+}
+
+static void process_input_token(struct trailer_item *in_tok,
+ struct trailer_item **arg_tok_first,
+ enum action_where where)
+{
+ struct trailer_item *arg_tok;
+ struct trailer_item *next_arg;
+
+ int after = where == WHERE_AFTER;
+ int tok_alnum_len = alnum_len(in_tok->token, strlen(in_tok->token));
+
+ for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+ next_arg = arg_tok->next;
+ if (!same_token(in_tok, arg_tok, tok_alnum_len))
+ continue;
+ if (arg_tok->conf.where != where)
+ continue;
+ remove_from_list(arg_tok, arg_tok_first);
+ apply_arg_if_exists(in_tok, arg_tok, tok_alnum_len);
+ /*
+
[PATCH v8 12/12] Documentation: add documentation for 'git interpret-trailers'
Signed-off-by: Christian Couder --- Documentation/git-interpret-trailers.txt | 123 +++ 1 file changed, 123 insertions(+) create mode 100644 Documentation/git-interpret-trailers.txt diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt new file mode 100644 index 000..75ae386 --- /dev/null +++ b/Documentation/git-interpret-trailers.txt @@ -0,0 +1,123 @@ +git-interpret-trailers(1) += + +NAME + +git-interpret-trailers - help add stuctured information into commit messages + +SYNOPSIS + +[verse] +'git interpret-trailers' [--trim-empty] [([(=|:)])...] + +DESCRIPTION +--- +Help add RFC 822-like headers, called 'trailers', at the end of the +otherwise free-form part of a commit message. + +This command is a filter. It reads the standard input for a commit +message and applies the `token` arguments, if any, to this +message. The resulting message is emited on the standard output. + +Some configuration variables control the way the `token` arguments are +applied to the message and the way any existing trailer in the message +is changed. They also make it possible to automatically add some +trailers. + +By default, a 'token=value' or 'token:value' argument will be added +only if no trailer with the same (token, value) pair is already in the +message. The 'token' and 'value' parts will be trimmed to remove +starting and trailing whitespace, and the resulting trimmed 'token' +and 'value' will appear in the message like this: + + +token: value + + +By default, if there are already trailers with the same 'token', the +new trailer will appear just after the last trailer with the same +'token'. Otherwise it will appear at the end of the message. + +Note that 'trailers' do not follow and are not intended to follow many +rules that are in RFC 822. For example they do not follow the line +breaking rules, the encoding rules and probably many other rules. + +OPTIONS +--- +--trim-empty:: + If the 'value' part of any trailer contains only whitespace, + the whole trailer will be removed from the resulting message. + +CONFIGURATION VARIABLES +--- + +trailer..key:: + This 'key' will be used instead of 'token' in the + trailer. After some alphanumeric characters, it can contain + some non alphanumeric characters like ':', '=' or '#' that will + be used instead of ':' to separate the token from the value in + the trailer, though the default ':' is more standard. + +trailer..where:: + This can be either `after`, which is the default, or + `before`. If it is `before`, then a trailer with the specified + token, will appear before, instead of after, other trailers + with the same token, or otherwise at the beginning, instead of + at the end, of all the trailers. + +trailer..ifexist:: + This option makes it possible to choose what action will be + performed when there is already at least one trailer with the + same token in the message. ++ +The valid values for this option are: `addIfDifferent` (this is the +default), `addIfDifferentNeighbor`, `add`, `overwrite` or `doNothing`. ++ +With `addIfDifferent`, a new trailer will be added only if no trailer +with the same (token, value) pair is already in the message. ++ +With `addIfDifferentNeighbor`, a new trailer will be added only if no +trailer with the same (token, value) pair is above or below the line +where the new trailer will be added. ++ +With `add`, a new trailer will be added, even if some trailers with +the same (token, value) pair are already in the message. ++ +With `overwrite`, the new trailer will overwrite an existing trailer +with the same token. ++ +With `doNothing`, nothing will be done, that is no new trailer will be +added if there is already one with the same token in the message. + +trailer..ifmissing:: + This option makes it possible to choose what action will be + performed when there is not yet any trailer with the same + token in the message. ++ +The valid values for this option are: `add` (this is the default) and +`doNothing`. ++ +With `add`, a new trailer will be added. ++ +With `doNothing`, nothing will be done. + +trailer..command:: + This option can be used to specify a shell command that will + be used to automatically add or modify a trailer with the + specified 'token'. ++ +When this option is specified, it is like if a special 'token=value' +argument is added at the end of the command line, where 'value' will +be given by the standard output of the specified command. ++ +If the command contains the `$ARG` string, this string will be +replaced with the 'value' part of an existing trailer with the same +token, if any, before the command is launched. + +SEE ALSO + +linkgit:git-commit[1] +
[PATCH v8 11/12] trailer: add tests for commands in config file
Signed-off-by: Christian Couder --- t/t7513-interpret-trailers.sh | 71 +++ 1 file changed, 71 insertions(+) diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index 417a4f3..0a1f3b6 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -333,4 +333,75 @@ test_expect_success 'using "ifMissing = doNothing"' ' test_cmp expected actual ' +test_expect_success 'with simple command' ' + git config trailer.sign.key "Signed-off-by: " && + git config trailer.sign.where "after" && + git config trailer.sign.ifExists "addIfDifferentNeighbor" && + git config trailer.sign.command "echo \"A U Thor \"" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: A U Thor + EOF + git interpret-trailers "review:" "fix=22" actual && + test_cmp expected actual +' + +test_expect_success 'with command using commiter information' ' + git config trailer.sign.ifExists "addIfDifferent" && + git config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: C O Mitter + EOF + git interpret-trailers "review:" "fix=22" actual && + test_cmp expected actual +' + +test_expect_success 'with command using author information' ' + git config trailer.sign.key "Signed-off-by: " && + git config trailer.sign.where "after" && + git config trailer.sign.ifExists "addIfDifferentNeighbor" && + git config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: A U Thor + EOF + git interpret-trailers "review:" "fix=22" actual && + test_cmp expected actual +' + +test_expect_success 'setup a commit' ' + echo "Content of the first commit." > a.txt && + git add a.txt && + git commit -m "Add file a.txt" +' + +test_expect_success 'with command using $ARG' ' + git config trailer.fix.ifExists "overwrite" && + git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" && + FIXED=$(git log -1 --oneline --format="%h (%s)" --abbrev-commit --abbrev=14 HEAD) && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-EOF && + Fixes: $FIXED + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + Signed-off-by: A U Thor + EOF + git interpret-trailers "review:" "fix=HEAD" actual && + test_cmp expected actual +' + test_done -- 1.9.0.164.g3aa33cd.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 v8 05/12] trailer: process command line trailer arguments
Parse the trailer command line arguments and put
the result into an arg_tok doubly linked list.
Signed-off-by: Christian Couder
---
trailer.c | 103 ++
1 file changed, 103 insertions(+)
diff --git a/trailer.c b/trailer.c
index b5a0943..1871843 100644
--- a/trailer.c
+++ b/trailer.c
@@ -398,3 +398,106 @@ static int git_trailer_config(const char *conf_key, const
char *value, void *cb)
}
return 0;
}
+
+static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char
*trailer)
+{
+ size_t len = strcspn(trailer, "=:");
+ if (len == 0)
+ return error(_("empty trailer token in trailer '%s'"), trailer);
+ if (len < strlen(trailer)) {
+ strbuf_add(tok, trailer, len);
+ strbuf_trim(tok);
+ strbuf_addstr(val, trailer + len + 1);
+ strbuf_trim(val);
+ } else {
+ strbuf_addstr(tok, trailer);
+ strbuf_trim(tok);
+ }
+ return 0;
+}
+
+
+static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
+{
+ *dst = *src;
+ if (src->name)
+ dst->name = xstrdup(src->name);
+ if (src->key)
+ dst->key = xstrdup(src->key);
+ if (src->command)
+ dst->command = xstrdup(src->command);
+}
+
+static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
+char *tok, char *val)
+{
+ struct trailer_item *new = xcalloc(sizeof(*new), 1);
+ new->value = val;
+
+ if (conf_item) {
+ duplicate_conf(&new->conf, &conf_item->conf);
+ new->token = xstrdup(conf_item->conf.key);
+ free(tok);
+ } else
+ new->token = tok;
+
+ return new;
+}
+
+static struct trailer_item *create_trailer_item(const char *string)
+{
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ struct trailer_item *item;
+ int tok_alnum_len;
+
+ if (parse_trailer(&tok, &val, string))
+ return NULL;
+
+ tok_alnum_len = alnum_len(tok.buf, tok.len);
+
+ /* Lookup if the token matches something in the config */
+ for (item = first_conf_item; item; item = item->next) {
+ if (!strncasecmp(tok.buf, item->conf.key, tok_alnum_len) ||
+ !strncasecmp(tok.buf, item->conf.name, tok_alnum_len)) {
+ strbuf_release(&tok);
+ return new_trailer_item(item,
+ NULL,
+ strbuf_detach(&val, NULL));
+ }
+ }
+
+ return new_trailer_item(NULL,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL));
+}
+
+static void add_trailer_item(struct trailer_item **first,
+struct trailer_item **last,
+struct trailer_item *new)
+{
+ if (!new)
+ return;
+ if (!*last) {
+ *first = new;
+ *last = new;
+ } else {
+ (*last)->next = new;
+ new->previous = *last;
+ *last = new;
+ }
+}
+
+static struct trailer_item *process_command_line_args(int argc, const char
**argv)
+{
+ int i;
+ struct trailer_item *arg_tok_first = NULL;
+ struct trailer_item *arg_tok_last = NULL;
+
+ for (i = 0; i < argc; i++) {
+ struct trailer_item *new = create_trailer_item(argv[i]);
+ add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+ }
+
+ return arg_tok_first;
+}
--
1.9.0.164.g3aa33cd.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 v8 01/12] Add data structures and basic functions for commit trailers
We will use a doubly linked list to store all information
about trailers and their configuration.
This way we can easily remove or add trailers to or from
trailer lists while traversing the lists in either direction.
Signed-off-by: Christian Couder
---
Makefile | 1 +
trailer.c | 49 +
2 files changed, 50 insertions(+)
create mode 100644 trailer.c
diff --git a/Makefile b/Makefile
index d4ce53a..ec67ae1 100644
--- a/Makefile
+++ b/Makefile
@@ -883,6 +883,7 @@ LIB_OBJS += submodule.o
LIB_OBJS += symlinks.o
LIB_OBJS += tag.o
LIB_OBJS += trace.o
+LIB_OBJS += trailer.o
LIB_OBJS += transport.o
LIB_OBJS += transport-helper.o
LIB_OBJS += tree-diff.o
diff --git a/trailer.c b/trailer.c
new file mode 100644
index 000..db93a63
--- /dev/null
+++ b/trailer.c
@@ -0,0 +1,49 @@
+#include "cache.h"
+/*
+ * Copyright (c) 2013, 2014 Christian Couder
+ */
+
+enum action_where { WHERE_AFTER, WHERE_BEFORE };
+enum action_if_exists { EXISTS_ADD_IF_DIFFERENT,
EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
+ EXISTS_ADD, EXISTS_OVERWRITE, EXISTS_DO_NOTHING };
+enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
+
+struct conf_info {
+ char *name;
+ char *key;
+ char *command;
+ enum action_where where;
+ enum action_if_exists if_exists;
+ enum action_if_missing if_missing;
+};
+
+struct trailer_item {
+ struct trailer_item *previous;
+ struct trailer_item *next;
+ const char *token;
+ const char *value;
+ struct conf_info conf;
+};
+
+static int same_token(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
+{
+ return !strncasecmp(a->token, b->token, alnum_len);
+}
+
+static int same_value(struct trailer_item *a, struct trailer_item *b)
+{
+ return !strcasecmp(a->value, b->value);
+}
+
+static int same_trailer(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
+{
+ return same_token(a, b, alnum_len) && same_value(a, b);
+}
+
+/* Get the length of buf from its beginning until its last alphanumeric
character */
+static size_t alnum_len(const char *buf, size_t len)
+{
+ while (len > 0 && !isalnum(buf[len - 1]))
+ len--;
+ return len;
+}
--
1.9.0.164.g3aa33cd.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 v8 08/12] trailer: add interpret-trailers command
This patch adds the "git interpret-trailers" command.
This command uses the previously added process_trailers()
function in trailer.c.
Signed-off-by: Christian Couder
---
.gitignore | 1 +
Makefile | 1 +
builtin.h| 1 +
builtin/interpret-trailers.c | 33 +
git.c| 1 +
5 files changed, 37 insertions(+)
create mode 100644 builtin/interpret-trailers.c
diff --git a/.gitignore b/.gitignore
index dc600f9..c2a0b19 100644
--- a/.gitignore
+++ b/.gitignore
@@ -74,6 +74,7 @@
/git-index-pack
/git-init
/git-init-db
+/git-interpret-trailers
/git-instaweb
/git-log
/git-ls-files
diff --git a/Makefile b/Makefile
index ec67ae1..94f7e95 100644
--- a/Makefile
+++ b/Makefile
@@ -947,6 +947,7 @@ BUILTIN_OBJS += builtin/hash-object.o
BUILTIN_OBJS += builtin/help.o
BUILTIN_OBJS += builtin/index-pack.o
BUILTIN_OBJS += builtin/init-db.o
+BUILTIN_OBJS += builtin/interpret-trailers.o
BUILTIN_OBJS += builtin/log.o
BUILTIN_OBJS += builtin/ls-files.o
BUILTIN_OBJS += builtin/ls-remote.o
diff --git a/builtin.h b/builtin.h
index c47c110..8ca0065 100644
--- a/builtin.h
+++ b/builtin.h
@@ -73,6 +73,7 @@ extern int cmd_hash_object(int argc, const char **argv, const
char *prefix);
extern int cmd_help(int argc, const char **argv, const char *prefix);
extern int cmd_index_pack(int argc, const char **argv, const char *prefix);
extern int cmd_init_db(int argc, const char **argv, const char *prefix);
+extern int cmd_interpret_trailers(int argc, const char **argv, const char
*prefix);
extern int cmd_log(int argc, const char **argv, const char *prefix);
extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);
extern int cmd_ls_files(int argc, const char **argv, const char *prefix);
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
new file mode 100644
index 000..0c8ca72
--- /dev/null
+++ b/builtin/interpret-trailers.c
@@ -0,0 +1,33 @@
+/*
+ * Builtin "git interpret-trailers"
+ *
+ * Copyright (c) 2013, 2014 Christian Couder
+ *
+ */
+
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "trailer.h"
+
+static const char * const git_interpret_trailers_usage[] = {
+ N_("git interpret-trailers [--trim-empty]
[([(=|:)])...]"),
+ NULL
+};
+
+int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+{
+ int trim_empty = 0;
+
+ struct option options[] = {
+ OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty
trailers")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_interpret_trailers_usage, 0);
+
+ process_trailers(trim_empty, argc, argv);
+
+ return 0;
+}
diff --git a/git.c b/git.c
index 7cf2953..63a03eb 100644
--- a/git.c
+++ b/git.c
@@ -380,6 +380,7 @@ static struct cmd_struct commands[] = {
{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
{ "init", cmd_init_db },
{ "init-db", cmd_init_db },
+ { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP },
{ "log", cmd_log, RUN_SETUP },
{ "ls-files", cmd_ls_files, RUN_SETUP },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
--
1.9.0.164.g3aa33cd.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 v8 09/12] trailer: add tests for "git interpret-trailers"
Signed-off-by: Christian Couder --- t/t7513-interpret-trailers.sh | 336 ++ 1 file changed, 336 insertions(+) create mode 100755 t/t7513-interpret-trailers.sh diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh new file mode 100755 index 000..417a4f3 --- /dev/null +++ b/t/t7513-interpret-trailers.sh @@ -0,0 +1,336 @@ +#!/bin/sh +# +# Copyright (c) 2013 Christian Couder +# + +test_description='git interpret-trailers' + +. ./test-lib.sh + +# When we want one trailing space at the end of each line, let's use sed +# to make sure that these spaces are not removed by any automatic tool. + +test_expect_success 'setup' ' + cat >basic_message <<-\EOF && + subject + + body + EOF + cat >complex_message_body <<-\EOF && + my subject + + my body which is long + and contains some special + chars like : = ? ! + + EOF + sed -e "s/ Z\$/ /" >complex_message_trailers <<-\EOF + Fixes: Z + Acked-by: Z + Reviewed-by: Z + Signed-off-by: Z + EOF +' + +test_expect_success 'without config' ' + sed -e "s/ Z\$/ /" >expected <<-\EOF && + ack: Peff + Reviewed-by: Z + Acked-by: Johan + EOF + git interpret-trailers "ack = Peff" "Reviewed-by" "Acked-by: Johan" >actual && + test_cmp expected actual +' + +test_expect_success '--trim-empty without config' ' + cat >expected <<-\EOF && + ack: Peff + Acked-by: Johan + EOF + git interpret-trailers --trim-empty "ack = Peff" "Reviewed-by" "Acked-by: Johan" "sob:" >actual && + test_cmp expected actual +' + +test_expect_success 'with config setup' ' + git config trailer.ack.key "Acked-by: " && + cat >expected <<-\EOF && + Acked-by: Peff + EOF + git interpret-trailers --trim-empty "ack = Peff" >actual && + test_cmp expected actual && + git interpret-trailers --trim-empty "Acked-by = Peff" >actual && + test_cmp expected actual && + git interpret-trailers --trim-empty "Acked-by :Peff" >actual && + test_cmp expected actual +' + +test_expect_success 'with config setup and = sign' ' + git config trailer.ack.key "Acked-by= " && + cat >expected <<-\EOF && + Acked-by= Peff + EOF + git interpret-trailers --trim-empty "ack = Peff" >actual && + test_cmp expected actual && + git interpret-trailers --trim-empty "Acked-by= Peff" >actual && + test_cmp expected actual && + git interpret-trailers --trim-empty "Acked-by : Peff" >actual && + test_cmp expected actual +' + +test_expect_success 'with config setup and # sign' ' + git config trailer.bug.key "Bug #" && + cat >expected <<-\EOF && + Bug #42 + EOF + git interpret-trailers --trim-empty "bug = 42" >actual && + test_cmp expected actual +' + +test_expect_success 'with commit basic message' ' + git interpret-trailers actual && + test_cmp basic_message actual +' + +test_expect_success 'with commit complex message' ' + cat complex_message_body complex_message_trailers >complex_message && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + EOF + git interpret-trailers actual && + test_cmp expected actual +' + +test_expect_success 'with commit complex message and args' ' + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Fixes: Z + Acked-by= Z + Acked-by= Peff + Reviewed-by: Z + Signed-off-by: Z + Bug #42 + EOF + git interpret-trailers "ack: Peff" "bug: 42" actual && + test_cmp expected actual +' + +test_expect_success 'with commit complex message, args and --trim-empty' ' + cat complex_message_body >expected && + cat >>expected <<-\EOF && + Acked-by= Peff + Bug #42 + EOF + git interpret-trailers --trim-empty "ack: Peff" "bug: 42" actual && + test_cmp expected actual +' + +test_expect_success 'using "where = before"' ' + git config trailer.bug.where "before" && + cat complex_message_body >expected && + sed -e "s/ Z\$/ /" >>expected <<-\EOF && + Bug #42 + Fixes: Z + Acked-by= Z + Acked-by= Peff + Reviewed-by: Z + Signed-off-by: Z + EOF + git interpret-trailers "ack: Peff" "bug: 42" actual && + test_cmp expected actual +' + +test_expect_success 'using "where = before" for a token in the middle of the message' ' + g
Re: [PATCH/RFC 0/6] reuse deltas found by bitmaps
Jeff King writes: > Just looking at the 128-day case again, using bitmaps increased our > server CPU time _and_ made a much bigger pack. This series not only > fixes the CPU time regression, but it also drops the server CPU time to > almost nothing. That's a nice improvement, and it makes perfect sense: > we are reusing on-disk deltas instead of on-the-fly computing deltas > against the preferred bases. True. > I think we could still add the objects from the tip of the client's HAVE > list. That should make the result at least per to the non-bitmap case, right? > This patch would still be a CPU win on top of that, because it > would reduce the number of objects which need a delta search in the > first place. Yes. > So I think the next steps are probably: > > 1. Measure the "all objects are preferred bases" approach and confirm > that it is bad. ;-) > 2. Measure the "reused deltas become preferred bases" approach. I > expect the resulting size to be slightly better than what I have > now, but not as good as v1.9.0's size (but taking less CPU time). Do you mean "the bases for reused deltas become preferred bases, so that we can deltify more objects off of them"? > 3. Measure the "figure out boundaries and add them as preferred bases, > like we do without bitmaps" approach. I expect this to behave > similarly to v1.9.0. Yes. > 4. Combine (2) and (3) and measure them. I'm _hoping_ this will give > us the best of both worlds, but I still want to do the individual > measurements so we can see where any improvement is coming from. Sensible. 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/6] t/perf-lib: factor boilerplate out of test_perf
Jeff King writes:
> About half of test_perf() is boilerplate, and half is
> actually related to running the perf test. Let's split it
> into two functions, so that we can reuse the boilerplate in
> future commits.
>
> Signed-off-by: Jeff King
> ---
> t/perf/perf-lib.sh | 61
> +++---
> 1 file changed, 35 insertions(+), 26 deletions(-)
These early steps somewhat conflict with another topic that is
stalled (due to having no real users) on 'pu'. I do not think we
would terribly mind dropping tg/perf-lib-test-perf-cleanup and have
it rebased if the author or somebody else wants to have it in my
tree later, but just FYI.
>
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index a8c9574..20f306a 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -150,8 +150,8 @@ exit $ret' >&3 2>&4
> return "$eval_ret"
> }
>
> -
> -test_perf () {
> +test_wrapper_ () {
> + test_wrapper_func_=$1; shift
> test_start_
> test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
> test "$#" = 2 ||
> @@ -162,35 +162,44 @@ test_perf () {
> base=$(basename "$0" .sh)
> echo "$test_count" >>"$perf_results_dir"/$base.subtests
> echo "$1" >"$perf_results_dir"/$base.$test_count.descr
> - if test -z "$verbose"; then
> - printf "%s" "perf $test_count - $1:"
> - else
> - echo "perf $test_count - $1:"
> - fi
> - for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
> - say >&3 "running: $2"
> - if test_run_perf_ "$2"
> - then
> - if test -z "$verbose"; then
> - printf " %s" "$i"
> - else
> - echo "* timing run
> $i/$GIT_PERF_REPEAT_COUNT:"
> - fi
> + base="$perf_results_dir"/"$perf_results_prefix$(basename "$0"
> .sh)"."$test_count"
> + "$test_wrapper_func_" "$@"
> + fi
> +
> + test_finish_
> +}
> +
> +test_perf_ () {
> + if test -z "$verbose"; then
> + printf "%s" "perf $test_count - $1:"
> + else
> + echo "perf $test_count - $1:"
> + fi
> + for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
> + say >&3 "running: $2"
> + if test_run_perf_ "$2"
> + then
> + if test -z "$verbose"; then
> + printf " %s" "$i"
> else
> - test -z "$verbose" && echo
> - test_failure_ "$@"
> - break
> + echo "* timing run $i/$GIT_PERF_REPEAT_COUNT:"
> fi
> - done
> - if test -z "$verbose"; then
> - echo " ok"
> else
> - test_ok_ "$1"
> + test -z "$verbose" && echo
> + test_failure_ "$@"
> + break
> fi
> - base="$perf_results_dir"/"$perf_results_prefix$(basename "$0"
> .sh)"."$test_count"
> - "$TEST_DIRECTORY"/perf/min_time.perl test_time.* >"$base".times
> + done
> + if test -z "$verbose"; then
> + echo " ok"
> + else
> + test_ok_ "$1"
> fi
> - test_finish_
> + "$TEST_DIRECTORY"/perf/min_time.perl test_time.* >"$base".times
> +}
> +
> +test_perf () {
> + test_wrapper_ test_perf_ "$@"
> }
>
> # We extend test_done to print timings at the end (./run disables this
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 0/6] reuse deltas found by bitmaps
On Wed, Mar 26, 2014 at 03:31:41PM -0700, Junio C Hamano wrote: > > I think we could still add the objects from the tip of the client's HAVE > > list. > > That should make the result at least per to the non-bitmap case, > right? That's my expectation. > > > 2. Measure the "reused deltas become preferred bases" approach. I > > expect the resulting size to be slightly better than what I have > > now, but not as good as v1.9.0's size (but taking less CPU time). > > Do you mean "the bases for reused deltas become preferred bases, so > that we can deltify more objects off of them"? Exactly. I'll update more when I have results for some of these. :) -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/RFC 0/6] reuse deltas found by bitmaps
On 03/26/2014 12:22 AM, Jeff King wrote: [tl;dr the patch is the same as before, but there is a script to measure its effects; please try it out on your repos] Here are results from one of our repos: Test origin HEAD - 5311.3: server (1 days)1.77(2.49+0.15) 0.76(0.65+0.12) -57.1% 5311.4: size (1 days) 7.8M3.1M -60.2% 5311.5: client (1 days)0.56(0.48+0.03) 1.18(0.97+0.05) +110.7% 5311.7: server (2 days)2.79(3.68+0.25) 0.77(0.65+0.14) -72.4% 5311.8: size (2 days) 24.2M6.8M -71.8% 5311.9: client (2 days)1.14(0.95+0.10) 2.72(2.33+0.13) +138.6% 5311.11: server (4 days) 3.70(4.76+0.28) 0.84(0.72+0.14) -77.3% 5311.12: size (4 days) 51.2M 18.9M -63.0% 5311.13: client (4 days) 2.29(2.02+0.29) 4.58(3.91+0.30) +100.0% 5311.15: server (8 days) 5.16(7.48+0.38) 1.20(1.18+0.21) -76.7% 5311.16: size (8 days) 78.3M 42.6M -45.5% 5311.17: client (8 days) 3.73(3.67+0.40) 6.59(5.95+0.51) +76.7% 5311.19: server (16 days) 6.48(10.27+0.52)1.60(1.85+0.27) -75.3% 5311.20: size(16 days) 97.5M 64.0M -34.4% 5311.21: client (16 days) 5.31(5.76+0.57) 8.99(8.61+0.68) +69.3% 5311.23: server (32 days) 8.56(14.00+0.67)2.31(2.91+0.37) -73.0% 5311.24: size(32 days)124.7M 92.0M -26.2% 5311.25: client (32 days) 7.69(9.20+0.76) 12.80(13.07+0.91) +66.4% 5311.27: server (64 days) 37.47(45.48+1.55) 29.75(29.58+0.96) -20.6% 5311.28: size(64 days)245.5M 207.1M -15.6% 5311.29: client (64 days) 15.11(18.26+1.58) 25.02(25.90+2.03) +65.6% 5311.31: server (128 days) 43.85(57.02+2.57) 29.88(29.54+1.35) -31.9% 5311.32: size (128 days)507.3M 461.6M -9.0% 5311.33: client (128 days) 27.13(31.54+3.32) 37.76(39.49+3.16) +39.2% I'm currently running tests on a much larger repo than that. The full perf run will take several hours on that, so the soonest I can likely publish results for that is tomorrow. -- 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] t4212: handle systems with post-apocalyptic gmtime
On Wed, Mar 26, 2014 at 05:57:41PM -0400, Jeff King wrote: > Hmm, so the year you got is actually: 1623969404. That still seems off > to me by a factor 20. I don't know if this is really worth digging into > that much further, but I wonder what you would get for timestamps of: > > 9 > > 999 > etc. > AIX goes negative at about the same time Linux and Solaris segfault: 999 Sun Apr 26 10:46:39 1970 -0700 Sat Mar 3 02:46:39 1973 -0700 9 Sat Sep 8 18:46:39 2001 -0700 99 Sat Nov 20 10:46:39 2286 -0700 999 Wed Nov 16 02:46:39 5138 -0700 Thu Sep 26 18:46:39 33658 -0700 9 Sun May 20 10:46:39 318857 -0700 99 Sat Nov 7 02:46:39 3170843 -0700 999 Sat Jul 4 18:46:39 31690708 -0700 Sat Jan 25 10:46:39 316889355 -0700 9 Wed Sep 6 02:46:39 -1126091476 -0700 99 Thu Oct 24 18:46:39 1623969404 -0700 So, very bogus values. Charles. -- 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 01/12] Add data structures and basic functions for commit trailers
Christian Couder writes: >> Subject: Re: [PATCH v8 01/12] Add data structures and basic functions for >> commit trailers As pointed out many times for GSoC microprojects students, limit the scope with "area:" prefix for the commit title, e.g. Subject: trailers: add data structures and basic functions Please also refer to what has already been queued on 'pu' to avoid wasting review bandwidth and mark patches that are unchanged as such (but do send them to the list for review, so that people who haven't seen the previous round can also comment). As far as I can tell, this is the same as 8d1c70e5 (trailers: add data structures and basic functions, 2014-03-06), so I'll queue the remainder on top of that commit already on 'pu', which incidentally will preserve the original author timestamp from the previous incarnation. -- 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 00/12] Add interpret-trailers builtin
Christian Couder writes: > Until now git commit has only supported the well known > "Signed-off-by: " trailer, that is used by many projects like > the Linux kernel and Git. > > It is better to implement features for these trailers first in a > new command rather than in builtin/commit.c, because this way the > prepare-commit-msg and commit-msg hooks can reuse this command. The "first" is somewhat questionable. It is better to keep builtin/commit.c uncontaminated by any more hard-wired logic, like what we have for the signed-off-by line. Any new things can and should be doable in hooks, and this filter would help writing these hooks. And that is why the design goal of the filter is to make it at least as powerful as the built-in logic we have for signed-off-by lines; that would allow us to later eject the hard-wired logic for signed-off-by line from the main codepath, if/when we wanted to. Alternatively, we could build a library-ish API around this filter code and replace the hard-wired logic for signed-off-by line with a call into that API, if/when we wanted to, but that requires (in addition to the "at least as powerful as the built-in logic") that the implementation of this stand-alone filter can be cleanly made into a reusable library, so that is a bit higher bar to cross than "everything can be doable with hooks" alternative. > 3) Changes since version 7, thanks to Junio: > > * improved handling of empty trailer token > * clearer way to create 'expected' files in tests > * other small test cleanups > * improved commit message > * new way to parse config keys > * strcasecmp() is not used anymore in some config related functions It is unclear which of the 12 patches are unchanged since the last round. Are reviewers expected to re-read all of them? > Some values from the config file are lowercased instead. > To enable that a new patch (3/12) is introduced to rationalize > lowercase related functions. I am not very happy with these > changes. I can see why you are not very happy. Perhaps it may make you happier if you did not move lowercase() at all, did the xstrdup_tolower() in a cleaner and more efficient way, and only used the latter in the code? -- 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 03/12] Move lower case functions into wrapper.c
Christian Couder writes:
> diff --git a/wrapper.c b/wrapper.c
> index 0cc5636..c46026a 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -455,3 +455,17 @@ struct passwd *xgetpwuid_self(void)
> errno ? strerror(errno) : _("no such user"));
> return pw;
> }
> +
> +void lowercase(char *p)
> +{
> + for (; *p; p++)
> + *p = tolower(*p);
> +}
> +
> +char *xstrdup_tolower(const char *str)
> +{
> + char *dup = xstrdup(str);
> + lowercase(dup);
> + return dup;
> +}
> +
As a pure code-movement step, this may be OK, but I am not sure if
both of them want to be public functions in this shape.
Perhaps
char *downcase_copy(const char *str)
{
char *copy = xmalloc(strlen(str) + 1);
int i;
for (i = 0; str[i]; i++)
copy[i] = tolower(str[i]);
copy[i] = '\0';
return copy;
}
may avoid having to copy things twice. Do you need the other
function exposed?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/17] ls-files: add --color to highlight file names
On Thu, Mar 27, 2014 at 2:13 AM, Eric Sunshine wrote: > On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy > wrote: >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> Documentation/git-ls-files.txt | 9 + >> builtin/ls-files.c | 38 +++--- >> 2 files changed, 44 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt >> index c0856a6..5c1b7f3 100644 >> --- a/Documentation/git-ls-files.txt >> +++ b/Documentation/git-ls-files.txt >> @@ -147,6 +147,15 @@ a space) at the start of each line: >> possible for manual inspection; the exact format may change at >> any time. >> >> +--color[=]:: >> + Color file names. The value must be always (default), never, >> + or auto. > > Here, the default is "always"... These (.txt changes in other patches as well) are mostly copy and paste from existing .txt files. You may want to grep through and fix other places as well, in a separate series. > >> +--no-color:: >> + Turn off coloring, even when the configuration file gives the >> + default to color output, same as `--color=never`. This is the >> + default. > > But, here the default is "never". What I mean is color is turned off by default for ls-files (in contrast, ls has color on by default). The default 'always' means that if you write --color without the part, then it's --color=always. How do I phrase to make it clear? -- 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 v2 05/17] ls-files: buffer full item in strbuf before printing
On Thu, Mar 27, 2014 at 2:22 AM, Eric Sunshine wrote:
>> static void show_dir_entry(const char *tag, struct dir_entry *ent)
>> {
>> + static struct strbuf sb = STRBUF_INIT;
>> int len = max_prefix_len;
>>
>> if (len >= ent->len)
>> @@ -67,8 +79,10 @@ static void show_dir_entry(const char *tag, struct
>> dir_entry *ent)
>> if (!dir_path_match(ent, &pathspec, len, ps_matched))
>> return;
>>
>> - fputs(tag, stdout);
>> - write_name(ent->name);
>> + strbuf_reset(&sb);
>> + strbuf_addstr(&sb, tag);
>> + write_name(&sb, ent->name);
>> + strbuf_fputs(&sb, stdout);
>
> strbuf_release(&sb);
Not strictly necessary because sb is static and will be reset at the
next call. I just want to lower the number of allocation (write_name
allocates some more). It may be a premature optimization though.
The same for changes in show_ce_entry().
--
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 v3] Clarify pre-push hook documentation
From: "Junio C Hamano" David Cowden writes: The documentation as-is does not mention that the pre-push hook is executed even when there is nothing to push. This can lead a new reader to believe there will always be lines fed to the script's standard input and cause minor confusion as to what is happening when there are no lines provided to the pre-push script. Signed-off-by: David Cowden --- Notes: I'm not sure if I've covered every case here. If there are more cases to consider, please let me know and I can update to include them. I do not think of any offhand, but a more important point that I was trying to get at was that we should not give an incorrect impression to the readers that the scenario that is described is the only case they need to be worried about by pretending to be exhaustive. The "may" in your wording "This may happen when" may be good enough to hint that these may not be the only cases. c.f. http://stackoverflow.com/questions/22585091/git-hooks-pre-push-script-does-not-receive-input-via-stdin Documentation/githooks.txt | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index d954bf6..1fd6da9 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -203,6 +203,15 @@ SHA-1>` will be 40 `0`. If the local commit was specified by something other than a name which could be expanded (such as `HEAD~`, or a SHA-1) it will be supplied as it was originally given. +The hook is executed regardless of whether changes will actually be pushed or +not. This may happen when 'git push' is called and: + + - the remote ref is already up to date, or + - pushing to the remote ref cannot be handled by a simple fast-forward + +In other words, the script is called for every push. In the event that nothing +is to be pushed, no data will be provided on the script's standard input. Doesn't an 'in other words' indicate it could be further tightened? Maybe "If there is nothing to push, the hook will still run, but the input line will be empty. Likewise the hook will still run for other cases such as: - the remote ref is already up to date, - pushing to the remote ref cannot be handled by a simple fast-forward, - etc." When two things are to be pushed, the script will see the two things. When one thing is to be pushed, the script will see the one thing. When no thing is to be pushed, the script will see no thing on its standard input. But isn't that obvious? I still wonder if we really need to single out that "nothing" case. The more important thing is that it is invoked even in the "0-thing pushed" case, and "the list of things pushed that is given to the hook happens to be empty" is an obvious natural fallout. Personally I think it should be mentioned in that paragraph, which is covering all the various special cases. The 'nothing' case often causes confusion when it's not specified in documentation. If this hook exits with a non-zero status, 'git push' will abort without pushing anything. Information about why the push is rejected may be sent to the user by writing to standard error. -- It may be that the documentation should include the caveat "Hooks, when enabled, are executed unconditionally by their calling functions. Script writers should ensure they handle all conditions." somewhere near the top of the page to cover all hooks, which IIRC started David's journey. That would allow my second paragraph "Likewise.." to be dropped. Philip -- [apologies for any whitespace damage] -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 0/6] reuse deltas found by bitmaps
On Wed, Mar 26, 2014 at 02:13:00PM -0400, Jeff King wrote:
> So I think the next steps are probably:
>
> 1. Measure the "all objects are preferred bases" approach and confirm
> that it is bad.
Below is a very rough patch to accomplish this. It just traverses the
"have" bitmap and adds every object with the "exclude" flag. The result
is as comically bad as I expected.
For a big fetch, it seems like it's working (numbers against v1.9.0):
5311.31: server (128 days) 4.49(7.35+0.23) 4.98(6.82+3.31) +10.9%
5311.32: size (128 days) 25.8M 32.0M +24.2%
5311.33: client (128 days) 7.17(7.38+0.20) 7.33(7.90+0.20) +2.2%
A modest increase in CPU time, and we get back most of our size
(remember that our "bad" case here is ~80M).
But for a small fetch...
5311.3: server (1 days)0.20(0.17+0.03) 4.39(4.03+6.59) +2095.0%
5311.4: size (1 days) 57.2K 59.5K +4.1%
5311.5: client (1 days)0.08(0.08+0.00) 0.08(0.08+0.00) +0.0%
Yikes. Besides spending lots of CPU on handling the enlarged object
list, notice that we still only dropped the size in the 128-day case to
32M. Which is almost exactly what the earlier "reuse on-disk deltas"
patch achieved.
What I think is happening is that we manage to reuse those on-disk
deltas (since they are now preferred bases, and we know we can). But we
never actually come up with any _new_ deltas, because the search window
is overwhelmed with candidates. So not only do we waste a huge amount of
CPU, but we just end up at the same not-quite-optimal result as before.
So this is a dead end, but I think it was good to double-check that.
The patch below is messy and would probably be better split into a few
patches, but I don't expect anyone to apply it (or even read it,
really). It's just for reference.
---
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0ee5f1f..1a5d401 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1026,7 +1026,7 @@ static int add_object_entry_from_bitmap(const unsigned
char *sha1,
if (have_duplicate_entry(sha1, 0, &index_pos))
return 0;
- create_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack,
offset);
+ create_object_entry(sha1, type, name_hash, flags, 0, index_pos, pack,
offset);
display_progress(progress_state, to_pack.nr_objects);
return 1;
@@ -2436,6 +2436,7 @@ static int get_object_list_from_bitmap(struct rev_info
*revs)
}
traverse_bitmap_commit_list(&add_object_entry_from_bitmap);
+ bitmap_have_foreach(&add_object_entry_from_bitmap);
return 0;
}
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 1bae7e8..f4e30f5 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -605,6 +605,7 @@ static void show_objects_for_type(
struct bitmap *objects,
struct ewah_bitmap *type_filter,
enum object_type object_type,
+ int flags,
show_reachable_fn show_reach)
{
size_t pos = 0, i = 0;
@@ -613,9 +614,6 @@ static void show_objects_for_type(
struct ewah_iterator it;
eword_t filter;
- if (bitmap_git.reuse_objects == bitmap_git.pack->num_objects)
- return;
-
ewah_iterator_init(&it, type_filter);
while (i < objects->word_alloc && ewah_iterator_next(&filter, &it)) {
@@ -640,7 +638,7 @@ static void show_objects_for_type(
if (bitmap_git.hashes)
hash = ntohl(bitmap_git.hashes[entry->nr]);
- show_reach(sha1, object_type, 0, hash, bitmap_git.pack,
entry->offset);
+ show_reach(sha1, object_type, flags, hash,
bitmap_git.pack, entry->offset);
}
pos += BITS_IN_WORD;
@@ -816,14 +814,17 @@ void traverse_bitmap_commit_list(show_reachable_fn
show_reachable)
{
assert(bitmap_git.result);
+ if (bitmap_git.reuse_objects == bitmap_git.pack->num_objects)
+ return;
+
show_objects_for_type(bitmap_git.result, bitmap_git.commits,
- OBJ_COMMIT, show_reachable);
+ OBJ_COMMIT, 0, show_reachable);
show_objects_for_type(bitmap_git.result, bitmap_git.trees,
- OBJ_TREE, show_reachable);
+ OBJ_TREE, 0, show_reachable);
show_objects_for_type(bitmap_git.result, bitmap_git.blobs,
- OBJ_BLOB, show_reachable);
+ OBJ_BLOB, 0, show_reachable);
show_objects_for_type(bitmap_git.result, bitmap_git.tags,
- OBJ_TAG, show_reachable);
+ OBJ_TAG, 0, show_reachable);
show_extended_objects(bitmap_git.result, show_reachable);
@@ -1090,3 +1091,18 @@ int bitmap_have(const unsigned char *sha1)
return bitmap_get(bitmap_git.haves, pos);
}
+
+void bitmap_have_foreach(show_reachable_fn show_reachable)
+{
+ if (!bitmap_git.haves)
+ return;
+
+ show_object
