Re: git-gui ignores core.hooksPath
On Wed, Apr 11, 2018 at 12:50 AM, Junio C Hamano wrote: > Chris Maes writes: > >> Is there any hope from here that anyone will pick up this / these >> changes? Will anyone else be assigned the main responsible for this >> git-gui repository? >> >> Just hoping to revive the discussion here, since the >> https://github.com/patthoyts/git-gui/ repository seems quite dead. > > It indeed does. > > I've played a patch-monkey in the past for git-gui and have a few > topics queued still in my tree, but that serves merely as a bookmark > that is slightly better than a pointer to the mailing list archive. > > We need a volunteer to take over this part of the subsystem; > somebody who actually uses it, passionate about improving it, and > can speak tcl/tk somewhat fluently (I qualify none of these three > criteria myself). > > Any takers? the last time this topic came up, Stefan (in Cc) offered to volunteer. Stefan, is this offer still open? I would support this. Best,
Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`
On 4 June 2018 at 23:55, Ævar Arnfjörð Bjarmason wrote:
> I think this makes more sense instead of this fix:
[...]
> -void refspec_item_init(struct refspec_item *item, const char *refspec, int
> fetch)
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int
> fetch)
> {
> memset(item, 0, sizeof(*item));
> + int ret = parse_refspec(item, refspec, fetch);
> + return ret;
> +}
Nit: Declaration after statement. Intriguingly, you do use a `ret`
variable, so I suspect you sort of thought about it but left the final
cleaning up for later. ;-)
> -void refspec_item_init(struct refspec_item *item, const char *refspec, int
> fetch);
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int
> fetch);
> +void refspec_item_init_or_die(struct refspec_item *item, const char
> *refspec, int fetch);
> void refspec_item_clear(struct refspec_item *item);
> void refspec_init(struct refspec *rs, int fetch);
> void refspec_append(struct refspec *rs, const char *refspec);
>
> I.e. let's fix the bug, but with this admittedly more verbose fix we're
> left with exactly two memset() in refspec.c, one for each type of struct
> that's initialized by the API.
>
> The reason this is difficult now is because the current API conflates
> the init function with an init_or_die, which is what most callers want,
> so let's just split those concerns up. Then we're left with one init
> function that does the memset.
I didn't test this, but it looks sane in general IMHO, and should fix
the issue I saw. Should we be worried that someone might already depend
on `refspec_item_init()` to die, and we'll silently break that
assumption?
Martin
Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack
On 04/06/18 18:06, Max Kirillov wrote:
> On Mon, Jun 04, 2018 at 01:31:58PM +0900, Junio C Hamano wrote:
>> Max Kirillov writes:
>>> + size_t n = xread(0, buf, chunk_length);
>>> + if (n < 0)
>>> + die_errno("Reading request failed");
>>
>> n that is of type size_t is unsigned and cannot be negative here.
>
Hmm, xread() returns an ssize_t, which is a signed type ...
> Thanks, fixing it
> Do you think sanity check for n <= chunk_length (the code
> will go mad in this case) is needed? As far as I can see n
> returns straight from system's read()
ATB,
Ramsay Jones
Re: [ANNOUNCE] Git v2.18.0-rc1
> On 04 Jun 2018, at 06:53, Junio C Hamano wrote: > > A release candidate Git v2.18.0-rc1 is now available for testing > at the usual places. It is comprised of 842 non-merge commits > since v2.17.0, contributed by 65 people, 20 of which are new faces. > > ... > > * The new "checkout-encoding" attribute can ask Git to convert the > contents to the specified encoding when checking out to the working > tree (and the other way around when checking in). Did you call the feature "checkout-encoding" here intentionally? The attribute is called "working-tree-encoding" in the final and merged round. Shouldn't we call it that way here too? Thanks, Lars
[PATCH 01/10] t: add tool to translate hash-related values
Add a test function helper, test_translate, that will produce its first
argument if the hash in use is SHA-1 and the second if its argument is
NewHash. Implement a mode that can read entries from a file as well for
reusability across tests.
For the moment, use the length of the empty blob to determine the hash
in use. In the future, we can change this code so that it can use the
configuration and learn about the difference in input, output, and
on-disk formats.
Implement two basic lookup charts, one for common invalid or synthesized
object IDs, and one for various facts about the hash function in use.
Signed-off-by: brian m. carlson
---
t/test-lib-functions.sh | 40
t/translate/hash-info | 9 +
t/translate/oid | 15 +++
3 files changed, 64 insertions(+)
create mode 100644 t/translate/hash-info
create mode 100644 t/translate/oid
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca0..0e7067460b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1147,3 +1147,43 @@ depacketize () {
}
'
}
+
+test_translate_f_ () {
+ local file="$TEST_DIRECTORY/translate/$2" &&
+ perl -e '
+ $delim = "\t";
+ ($hoidlen, $file, $arg) = @ARGV;
+ open($fh, "<", $file) or die "open: $!";
+ while (<$fh>) {
+ # Allow specifying other delimiters.
+ $delim = $1 if /^#!\sdelimiter\s(.)/;
+ next if /^#/;
+ @fields = split /$delim/, $_, 3;
+ if ($fields[0] eq $arg) {
+ print($hoidlen == 40 ? $fields[1] : $fields[2]);
+ last;
+ }
+ }
+ ' "$1" "$file" "$3"
+}
+
+# Without -f, print the first argument if we are using SHA-1 and the second if
+# we're using NewHash.
+# With -f FILE ARG, read the (by default) tab-delimited file from
+# t/translate/FILE, finding the first field matching ARG and printing either
the
+# second or third field depending on the hash in use.
+test_translate () {
+ local hoidlen=$(printf "%s" "$EMPTY_BLOB" | wc -c) &&
+ if [ "$1" = "-f" ]
+ then
+ shift &&
+ test_translate_f_ "$hoidlen" "$@"
+ else
+ if [ "$hoidlen" -eq 40 ]
+ then
+ printf "%s" "$1"
+ else
+ printf "%s" "$2"
+ fi
+ fi
+}
diff --git a/t/translate/hash-info b/t/translate/hash-info
new file mode 100644
index 00..36cbd9a8eb
--- /dev/null
+++ b/t/translate/hash-info
@@ -0,0 +1,9 @@
+# Various facts about the hash algorithm in use for easy access in tests.
+#
+# Several aliases are provided for easy recall.
+rawsz 20 32
+oidlen 20 32
+hexsz 40 64
+hexoidlen 40 64
+zero
+zero-oid
diff --git a/t/translate/oid b/t/translate/oid
new file mode 100644
index 00..8de0fd64af
--- /dev/null
+++ b/t/translate/oid
@@ -0,0 +1,15 @@
+# These are some common invalid and partial object IDs used in tests.
+0010001
0001
+0020002
0002
+0030003
0003
+0040004
0004
+0050005
0005
+0060006
0006
+0070007
0007
+# All zeros or Fs missing one or two hex segments.
+zero-1 000
000
+zero-2 00
00
+ff-1 fff
fff
+ff-2 ff
ff
+numeric0123456789012345678901234567890123456789
0123456789012345678901234567890123456789012345678901234567890123
+deadbeef deadbeefdeadbeefdeadbeefdeadbeefdeadbeef
[PATCH 02/10] t0000: use hash translation table
If the hash we're using is 32 bytes in size, attempting to insert a 20-byte object name won't work. Since these are synthesized objects that are almost all zeros, look them up in a translation table. Signed-off-by: brian m. carlson --- t/t-basic.sh | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/t/t-basic.sh b/t/t-basic.sh index af61d083b4..27ef9ecab2 100755 --- a/t/t-basic.sh +++ b/t/t-basic.sh @@ -978,12 +978,13 @@ test_expect_success SHA1 'validate object ID for a known tree' ' test_expect_success 'put invalid objects into the index' ' rm -f .git/index && - cat >badobjects <<-\EOF && - 100644 blob 1000dir/file1 - 100644 blob 2000dir/file2 - 100644 blob 3000dir/file3 - 100644 blob 4000dir/file4 - 100644 blob 5000dir/file5 + suffix=$(echo $ZERO_OID | sed -e "s/^.//") && + cat >badobjects <<-EOF && + 100644 blob $(test_translate -f oid 001)dir/file1 + 100644 blob $(test_translate -f oid 002)dir/file2 + 100644 blob $(test_translate -f oid 003)dir/file3 + 100644 blob $(test_translate -f oid 004)dir/file4 + 100644 blob $(test_translate -f oid 005)dir/file5 EOF git update-index --index-info
[PATCH 00/10] Hash-independent tests (part 3)
This is next in the series of improvements to make tests hash-independent. Introduced here is a test helper, test_translate, that allows lookups in tests based on the hash algorithm in use. Alternatives are either specified inline (the former for SHA-1, the latter for NewHash), or (more commonly) from a file based on a key. Some basic examples of translations are provided and used throughout the tests. The ultimate idea is that tests can simply drop a file into the translation directory and use their own test-specific translations if convenient. This series (and subsequent series) attempt to standardize slightly on our use of invalid object IDs in tests. Since the actual invalid IDs aren't usually very important, the translations I've made using the tables aren't necessarily entirely faithful: we will sometimes use different SHA-1 object IDs than before, but we substitute only invalid object IDs for different invalid ones. It's likely in the future that test_translate will support additional options depending on whether we want input, output, or internal formats. I had mentioned in a previous comment that a given test would be included in "the next series" (this one), but I redid the series and decided to split it into smaller pieces, so it isn't included. Sorry. Comments on any aspect of the series are welcome, but thoughts on design and naming would be especially valuable. brian m. carlson (10): t: add tool to translate hash-related values t: use hash translation table t0002: abstract away SHA-1-specific constants t0027: use $ZERO_OID t0064: make hash size independent t1006: make hash size independent t1400: switch hard-coded object ID to variable t1405: make hash size independent t1406: make hash-size independent t1407: make hash size independent t/t-basic.sh | 13 - t/t0002-gitfile.sh | 26 +- t/t0027-auto-crlf.sh | 14 +- t/t0064-sha1-array.sh | 49 +++--- t/t1006-cat-file.sh| 4 +-- t/t1400-update-ref.sh | 2 +- t/t1405-main-ref-store.sh | 4 +-- t/t1406-submodule-ref-store.sh | 6 ++--- t/t1407-worktree-ref-store.sh | 4 +-- t/test-lib-functions.sh| 40 +++ t/translate/hash-info | 9 +++ t/translate/oid| 15 +++ 12 files changed, 129 insertions(+), 57 deletions(-) create mode 100644 t/translate/hash-info create mode 100644 t/translate/oid
[PATCH 03/10] t0002: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for object IDs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t0002-gitfile.sh | 26 ++ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh index 3691023d51..020af4c53c 100755 --- a/t/t0002-gitfile.sh +++ b/t/t0002-gitfile.sh @@ -89,14 +89,16 @@ test_expect_success 'enter_repo non-strict mode' ' cd enter_repo && test_tick && test_commit foo && + git rev-parse HEAD >head-revision && mv .git .realgit && echo "gitdir: .realgit" >.git ) && + head=$(cat enter_repo/head-revision) && git ls-remote enter_repo >actual && - cat >expected <<-\EOF && - 946e985ab20de757ca5b872b16d64e92ff3803a9HEAD - 946e985ab20de757ca5b872b16d64e92ff3803a9refs/heads/master - 946e985ab20de757ca5b872b16d64e92ff3803a9refs/tags/foo + cat >expected <<-EOF && + $head HEAD + $head refs/heads/master + $head refs/tags/foo EOF test_cmp expected actual ' @@ -107,20 +109,20 @@ test_expect_success 'enter_repo linked checkout' ' git worktree add ../foo refs/tags/foo ) && git ls-remote foo >actual && - cat >expected <<-\EOF && - 946e985ab20de757ca5b872b16d64e92ff3803a9HEAD - 946e985ab20de757ca5b872b16d64e92ff3803a9refs/heads/master - 946e985ab20de757ca5b872b16d64e92ff3803a9refs/tags/foo + cat >expected <<-EOF && + $head HEAD + $head refs/heads/master + $head refs/tags/foo EOF test_cmp expected actual ' test_expect_success 'enter_repo strict mode' ' git ls-remote --upload-pack="git upload-pack --strict" foo/.git >actual && - cat >expected <<-\EOF && - 946e985ab20de757ca5b872b16d64e92ff3803a9HEAD - 946e985ab20de757ca5b872b16d64e92ff3803a9refs/heads/master - 946e985ab20de757ca5b872b16d64e92ff3803a9refs/tags/foo + cat >expected <<-EOF && + $head HEAD + $head refs/heads/master + $head refs/tags/foo EOF test_cmp expected actual '
[PATCH 09/10] t1406: make hash-size independent
Instead of hard-coding a 40-based constant, split the output of for-each-ref and for-each-reflog by field. Signed-off-by: brian m. carlson --- t/t1406-submodule-ref-store.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh index e093782cc3..d199d872fb 100755 --- a/t/t1406-submodule-ref-store.sh +++ b/t/t1406-submodule-ref-store.sh @@ -39,7 +39,7 @@ test_expect_success 'rename_refs() not allowed' ' ' test_expect_success 'for_each_ref(refs/heads/)' ' - $RUN for-each-ref refs/heads/ | cut -c 42- >actual && + $RUN for-each-ref refs/heads/ | cut -d" " -f 2- >actual && cat >expected <<-\EOF && master 0x0 new-master 0x0 @@ -48,7 +48,7 @@ test_expect_success 'for_each_ref(refs/heads/)' ' ' test_expect_success 'for_each_ref() is sorted' ' - $RUN for-each-ref refs/heads/ | cut -c 42- >actual && + $RUN for-each-ref refs/heads/ | cut -d" " -f 2- >actual && sort actual > expected && test_cmp expected actual ' @@ -65,7 +65,7 @@ test_expect_success 'verify_ref(new-master)' ' ' test_expect_success 'for_each_reflog()' ' - $RUN for-each-reflog | sort | cut -c 42- >actual && + $RUN for-each-reflog | sort | cut -d" " -f 2- >actual && cat >expected <<-\EOF && HEAD 0x1 refs/heads/master 0x0
[PATCH 10/10] t1407: make hash size independent
Instead of hard-coding a 40-based constant, split the output of for-each-ref and for-each-reflog by field. Signed-off-by: brian m. carlson --- t/t1407-worktree-ref-store.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh index 4623ae15c4..9a84858118 100755 --- a/t/t1407-worktree-ref-store.sh +++ b/t/t1407-worktree-ref-store.sh @@ -58,7 +58,7 @@ test_expect_success 'for_each_reflog()' ' mkdir -p .git/worktrees/wt/logs/refs/bisect && echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random && - $RWT for-each-reflog | cut -c 42- | sort >actual && + $RWT for-each-reflog | cut -d" " -f 2- | sort >actual && cat >expected <<-\EOF && HEAD 0x1 PSEUDO-WT 0x0 @@ -68,7 +68,7 @@ test_expect_success 'for_each_reflog()' ' EOF test_cmp expected actual && - $RMAIN for-each-reflog | cut -c 42- | sort >actual && + $RMAIN for-each-reflog | cut -d" " -f 2- | sort >actual && cat >expected <<-\EOF && HEAD 0x1 PSEUDO-MAIN 0x0
[PATCH 04/10] t0027: use $ZERO_OID
Use the ZERO_OID variable to express the all-zeros object ID so that it works with hash algorithms of all sizes. Signed-off-by: brian m. carlson --- t/t0027-auto-crlf.sh | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh index beb5927f77..14fcd3f49f 100755 --- a/t/t0027-auto-crlf.sh +++ b/t/t0027-auto-crlf.sh @@ -371,13 +371,13 @@ test_expect_success 'setup master' ' git checkout -b master && git add .gitattributes && git commit -m "add .gitattributes" . && - printf "\$Id: \$\nLINEONE\nLINETWO\nLINETHREE" >LF && - printf "\$Id: \$\r\nLINEONE\r\nLINETWO\r\nLINETHREE" >CRLF && - printf "\$Id: \$\nLINEONE\r\nLINETWO\nLINETHREE" >CRLF_mix_LF && - printf "\$Id: \$\nLINEONE\nLINETWO\rLINETHREE" >LF_mix_CR && - printf "\$Id: \$\r\nLINEONE\r\nLINETWO\rLINETHREE" >CRLF_mix_CR && - printf "\$Id: \$\r\nLINEONEQ\r\nLINETWO\r\nLINETHREE" | q_to_nul >CRLF_nul && - printf "\$Id: \$\nLINEONEQ\nLINETWO\nLINETHREE" | q_to_nul >LF_nul && + printf "\$Id: $ZERO_OID \$\nLINEONE\nLINETWO\nLINETHREE" >LF && + printf "\$Id: $ZERO_OID \$\r\nLINEONE\r\nLINETWO\r\nLINETHREE" >CRLF && + printf "\$Id: $ZERO_OID \$\nLINEONE\r\nLINETWO\nLINETHREE" >CRLF_mix_LF && + printf "\$Id: $ZERO_OID \$\nLINEONE\nLINETWO\rLINETHREE" >LF_mix_CR && + printf "\$Id: $ZERO_OID \$\r\nLINEONE\r\nLINETWO\rLINETHREE" >CRLF_mix_CR && + printf "\$Id: $ZERO_OID \$\r\nLINEONEQ\r\nLINETWO\r\nLINETHREE" | q_to_nul >CRLF_nul && + printf "\$Id: $ZERO_OID \$\nLINEONEQ\nLINETWO\nLINETHREE" | q_to_nul >LF_nul && create_NNO_MIX_files && git -c core.autocrlf=false add NNO_*.txt MIX_*.txt && git commit -m "mixed line endings" &&
[PATCH 05/10] t0064: make hash size independent
Compute test values of the appropriate size instead of hard-coding
40-character values. Rename the echo20 function to echoid, since the
values may be of varying sizes.
Signed-off-by: brian m. carlson
---
t/t0064-sha1-array.sh | 49 ---
1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
index 67484502a0..ab230179d2 100755
--- a/t/t0064-sha1-array.sh
+++ b/t/t0064-sha1-array.sh
@@ -3,30 +3,30 @@
test_description='basic tests for the SHA1 array implementation'
. ./test-lib.sh
-echo20 () {
+echoid () {
prefix="${1:+$1 }"
shift
while test $# -gt 0
do
- echo "$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
+ echo "$prefix$ZERO_OID" | sed -e "s/00/$1/g"
shift
done
}
test_expect_success 'ordered enumeration' '
- echo20 "" 44 55 88 aa >expect &&
+ echoid "" 44 55 88 aa >expect &&
{
- echo20 append 88 44 aa 55 &&
+ echoid append 88 44 aa 55 &&
echo for_each_unique
} | test-tool sha1-array >actual &&
test_cmp expect actual
'
test_expect_success 'ordered enumeration with duplicate suppression' '
- echo20 "" 44 55 88 aa >expect &&
+ echoid "" 44 55 88 aa >expect &&
{
- echo20 append 88 44 aa 55 &&
- echo20 append 88 44 aa 55 &&
+ echoid append 88 44 aa 55 &&
+ echoid append 88 44 aa 55 &&
echo for_each_unique
} | test-tool sha1-array >actual &&
test_cmp expect actual
@@ -34,8 +34,8 @@ test_expect_success 'ordered enumeration with duplicate
suppression' '
test_expect_success 'lookup' '
{
- echo20 append 88 44 aa 55 &&
- echo20 lookup 55
+ echoid append 88 44 aa 55 &&
+ echoid lookup 55
} | test-tool sha1-array >actual &&
n=$(cat actual) &&
test "$n" -eq 1
@@ -43,8 +43,8 @@ test_expect_success 'lookup' '
test_expect_success 'lookup non-existing entry' '
{
- echo20 append 88 44 aa 55 &&
- echo20 lookup 33
+ echoid append 88 44 aa 55 &&
+ echoid lookup 33
} | test-tool sha1-array >actual &&
n=$(cat actual) &&
test "$n" -lt 0
@@ -52,9 +52,9 @@ test_expect_success 'lookup non-existing entry' '
test_expect_success 'lookup with duplicates' '
{
- echo20 append 88 44 aa 55 &&
- echo20 append 88 44 aa 55 &&
- echo20 lookup 55
+ echoid append 88 44 aa 55 &&
+ echoid append 88 44 aa 55 &&
+ echoid lookup 55
} | test-tool sha1-array >actual &&
n=$(cat actual) &&
test "$n" -ge 2 &&
@@ -63,19 +63,24 @@ test_expect_success 'lookup with duplicates' '
test_expect_success 'lookup non-existing entry with duplicates' '
{
- echo20 append 88 44 aa 55 &&
- echo20 append 88 44 aa 55 &&
- echo20 lookup 66
+ echoid append 88 44 aa 55 &&
+ echoid append 88 44 aa 55 &&
+ echoid lookup 66
} | test-tool sha1-array >actual &&
n=$(cat actual) &&
test "$n" -lt 0
'
test_expect_success 'lookup with almost duplicate values' '
+ # n-1 5s
+ root=$(test_translate 555 \
+
555) &&
{
- echo "append " &&
- echo "append 555f" &&
- echo20 lookup 55
+ id1="${root}5" &&
+ id2="${root}f" &&
+ echo "append $id1" &&
+ echo "append $id2" &&
+ echoid lookup 55
} | test-tool sha1-array >actual &&
n=$(cat actual) &&
test "$n" -eq 0
@@ -83,8 +88,8 @@ test_expect_success 'lookup with almost duplicate values' '
test_expect_success 'lookup with single duplicate value' '
{
- echo20 append 55 55 &&
- echo20 lookup 55
+ echoid append 55 55 &&
+ echoid lookup 55
} | test-tool sha1-array >actual &&
n=$(cat actual) &&
test "$n" -ge 0 &&
[PATCH 08/10] t1405: make hash size independent
Instead of hard-coding a 40-based constant, split the output of for-each-ref and for-each-reflog by field. Signed-off-by: brian m. carlson --- t/t1405-main-ref-store.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh index a74c38b5fb..331899ddc4 100755 --- a/t/t1405-main-ref-store.sh +++ b/t/t1405-main-ref-store.sh @@ -54,7 +54,7 @@ test_expect_success 'for_each_ref(refs/heads/)' ' ' test_expect_success 'for_each_ref() is sorted' ' - $RUN for-each-ref refs/heads/ | cut -c 42- >actual && + $RUN for-each-ref refs/heads/ | cut -d" " -f 2- >actual && sort actual > expected && test_cmp expected actual ' @@ -71,7 +71,7 @@ test_expect_success 'verify_ref(new-master)' ' ' test_expect_success 'for_each_reflog()' ' - $RUN for-each-reflog | sort -k2 | cut -c 42- >actual && + $RUN for-each-reflog | sort -k2 | cut -d" " -f 2- >actual && cat >expected <<-\EOF && HEAD 0x1 refs/heads/master 0x0
[PATCH 06/10] t1006: make hash size independent
Compute the size of the tree and commit objects we're creating by checking for the size of an object ID and computing the resulting sizes accordingly. Signed-off-by: brian m. carlson --- t/t1006-cat-file.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 13dd510b2e..3b82051e2b 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -141,14 +141,14 @@ test_expect_success '--batch-check without %(rest) considers whole line' ' ' tree_sha1=$(git write-tree) -tree_size=33 +tree_size=$(($(test_translate -f hash-info rawsz) + 13)) tree_pretty_content="100644 blob $hello_sha1 hello" run_tests 'tree' $tree_sha1 $tree_size "" "$tree_pretty_content" commit_message="Initial commit" commit_sha1=$(echo_without_newline "$commit_message" | git commit-tree $tree_sha1) -commit_size=177 +commit_size=$(($(test_translate -f hash-info hexsz) + 137)) commit_content="tree $tree_sha1 author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> 00 + committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 00 +
[PATCH 07/10] t1400: switch hard-coded object ID to variable
Switch a hard-coded all-zeros object ID to use a variable instead. Signed-off-by: brian m. carlson --- t/t1400-update-ref.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index e1fd0f0ca8..ffaadf5f2d 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -346,7 +346,7 @@ test_expect_success "verifying $m's log (logged by config)" ' git update-ref $m $D cat >.git/logs/$m < 1117150320 -0500 +$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500 $C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500 $F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
Re: [PATCH 2/2] tests: make forging GPG signed commits and tags more robust
On Mon, Jun 04, 2018 at 03:39:26PM +0200, SZEDER Gábor wrote: > On rare occasions the given pattern occurs not only in the commit > message but in the GPG signature as well, and after it's replaced in > the signature the resulting signature becomes invalid, GPG will report > CRC error and that it couldn't find any signature, which will then > ultimately cause the test failure. Ooh, I hadn't seen these failures, but thanks for tracking this down. > Since in all three cases the pattern to be replaced during the forgery > is the first word of the commit message's subject line, and since the > GPG signature in the commit object is indented by a space, let's just > anchor those patterns to the beginning of the line to prevent this > issue. > > The test script 't7030-verify-tag.sh' creates a forged signed tag > object in a similar way by replacing the pattern "seventh", but the > GPG signature in tag objects is not indented by a space, so the above > solution is not applicable in this case. However, in the tag object > in question the pattern "seventh" occurs not only in the tag message > but in the 'tag' header as well. To create a forged tag object it's > sufficient to replace only one of the two occurences, so modify the > sed script to limit the pattern to the 'tag' header (i.e. a line > beginning with "tag ", which, because of the space character, can > never occur in the base64-encoded GPG signature). > > Note that the forgery in 't7004-tag.sh' is not affected by this issue: > while 't7004' does create a forged signed tag kind of the same way, > it replaces "signed-tag" in the tag object, which, because of the '-' > character, can never occur in the base64-encoded GPG signarute. This seems sane and obviously correct, and the other patch looked good, too. Thanks. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack
On Mon, Jun 04, 2018 at 12:44:09AM -0400, Jeff King wrote:
Thanks for the comments, I will do the things you proposed,
or try to and get back later if there are any issues. Some
notes below.
> On Sun, Jun 03, 2018 at 12:27:49AM +0300, Max Kirillov wrote:
> Since this is slightly less efficient, and because it only matters if
> the web server does not already close the pipe, should this have a
> run-time configuration knob, even if it defaults to
> safe-but-slightly-slower?
Personally, I of course don't want this. Also, I don't think
the difference is much noticeable. But you can never be sure
without trying. I'll try to measure some numbers.
>> +if (write_in_full(out, buf, n) < 0)
>> +die_errno("%s aborted reading request", prog_name);
>
> We don't necessarily know why the write failed. If it's EPIPE, then yes,
> the program probably did abort. But all we know is that write() failed.
> We should probably say something more generic like:
>
> die_errno("unable to write to '%s'");
>
> or similar.
Actually, it is already 3rd same error in this file. Maybe
deserve some refactoring. I will change the message also.
>> +test_expect_success 'setup repository' '
>> +test_commit c0 &&
>> +test_commit c1
>> +'
>> +
>> +hash_head=$(git rev-parse HEAD)
>> +hash_prev=$(git rev-parse HEAD~1)
>
> We generally prefer to have all commands, even ones we don't expect to
> fail, inside test_expect blocks (e.g., with a "setup" description).
Will the defined variables get to the next test? I'll try to
do as you describe.
>> +cat >fetch_body <> +0032want $hash_head
>> +0032have $hash_prev
>> +0009done
>> +EOF
>
> This depends on the size of the hash. That's always 40 for now, but is
> something that may change soon.
>
> We already have a packetize() helper; could we use it here?
Could you point me to it? I cannot find it.
My understanfing is that the current protocol assumes
40 symbols hash, so another hash length would be another
protocol, and since it's manually forged here it would
anyway has to be changeda.
>> +test_expect_success 'fetch plain truncated' '
>> +test_http_env upload \
>> +"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl
>> fetch_body.trunc git http-backend >act.out 2>act.err &&
>> +test_must_fail verify_http_result "200 OK"
>> +'
>
> Usually test_must_fail on a checking function like this is a sign that
> the check is not as robust as we'd like. If the function checks two
> things "A && B", then checking test_must_fail will only let us know
> "!A || !B", but you probably want to check both.
Well here I just want to know that the request has failed,
and we already know that it can fail in different ways,
but the test is not going to differentiate those ways.
> (We'd also generally not use test_must_fail with a non-git command, and
> just use a simple "! verify_http_result"; that would apply equally if
> gets split into two commands).
Will use ! there.
>> +sleep 1; # is interrupted by SIGCHLD
>> +if (!$exited) {
>> +close($out);
>> +die "Command did not exit after reading whole body";
>> +}
...
> Also, do we need to protect ourselves against other signals being
> delivered? E.g., if I resize my xterm and this process gets SIGWINCH, is
> it going to erroneously end the sleep and say "nope, no exited signal"?
I'll check, but what could I do? Should I add blocking other
signals there?
Company's Product
تحية طيبة، نحن نعيد بناء العراق بعد سنوات من الصراعات ونحن ندعو عليك أن تأخذ العقود. نحن مصممون على شراء منتجاتك بكميات كبيرة ، لاستخدامها في جميع محافظاتنا الثمانية عشر (المحافظات) حيث أن مهمة إعادة بناء العراق تغطي كل قطاع واحد وجانب مجتمعنا. سنقوم بإرسال معلومات منتجاتك إلى مكتب المشروع والمقاولات العراقية. سوف يقومون بفحص ملاءمة وضرورة منتجك والموافقة على العرض بالجملة علاقة تعاقد. أنا حاليا في مجلس إدارة المشروع العراقي والمقاولات مكتب ، مع صلاتي في أروقة السلطة ، نحن تماما واثق من الحصول على الموافقة. أيضا من المذكرة هي مسألة مختلفة اللوائح المالية بين بلدي العراق وبلدك. مثل ستحصل على 100٪ من خلال وزارة المالية العراقية قبل البدء في الإمدادات. عندما تلقيت الدفع ، سنقوم بذلك نتوقع عرض شهري ؛ كما قد يكون المبلغ المدرج في الميزانية للمنتج هائلة جدا لتفوق القدرة والقدرة على التوريد. كما يجب مراعاة أن عرض الأسعار الخاص بك يجب أن يكون CIF Port of Umm قصر. يرجى إرسال استجابة حتى أنني سوف تكشف عن المزيد من الإجرائية المعلومات لك على تأكيد إعادة. أنت أنت. Greetings, We are rebuilding Iraq after years of conflicts and we are inviting you to take up contracts. We are determined to purchase your products in large quantities, for use in all over our 18 governorates (provinces) as the task of re-building Iraq covers every single sector and facet of our society. We'll submit your products information to the Iraqi Project and Contracting Office. They will examine the propriety and necessity of your product and approve for bulk supply contracting relationship. I am currently on the board of the Iraqi Project and Contracting Office, With my connections in the corridors of power, we are quite confident of securing approval. Also of note is the issue of different financial regulations between my country Iraq and your country. As such you will be paid 100% through the Iraqi ministry of Finance before you commence supplies. When you've received payment, we would be expecting a monthly supply; as the sum budgeted for product may be quite enormous as to outstrip your capacity and capability to supply. A consideration also is that your quotation must be CIF Port of Umm Qasr. Please send response so that I will reveal more procedural information to you upon your re-confirmation. Thany You. Abdul Ghani
Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`
On Mon, Jun 04 2018, Martin Ågren wrote:
> We allocate a `struct refspec_item` on the stack without initializing
> it. In particular, its `dst` and `src` members will contain some random
> data from the stack. When we later call `refspec_item_clear()`, it will
> call `free()` on those pointers. So if the call to `parse_refspec()` did
> not assign to them, we will be freeing some random "pointers". This is
> undefined behavior.
>
> To the best of my understanding, this cannot currently be triggered by
> user-provided data. And for what it's worth, the test-suite does not
> trigger this with SANITIZE=address. It can be provoked by calling
> `valid_fetch_refspec(":*")`.
>
> Zero the struct, as is done in other users of `struct refspec_item`.
>
> Signed-off-by: Martin Ågren
> ---
> I found some time to look into this. It does not seem to be a
> user-visible bug, so not particularly critical.
>
> refspec.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/refspec.c b/refspec.c
> index ada7854f7a..7dd7e361e5 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -189,7 +189,10 @@ void refspec_clear(struct refspec *rs)
> int valid_fetch_refspec(const char *fetch_refspec_str)
> {
> struct refspec_item refspec;
> - int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> + int ret;
> +
> + memset(, 0, sizeof(refspec));
> + ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> refspec_item_clear();
> return ret;
> }
I think this makes more sense instead of this fix:
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae85..74a804f2e8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1077,7 +1077,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
- refspec_item_init(, value.buf, REFSPEC_FETCH);
+ refspec_item_init_or_die(, value.buf, REFSPEC_FETCH);
strbuf_reset();
diff --git a/builtin/pull.c b/builtin/pull.c
index 1f2ecf3a88..bb64631d98 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -684,7 +684,7 @@ static const char *get_tracking_branch(const char *remote,
const char *refspec)
const char *spec_src;
const char *merge_branch;
- refspec_item_init(, refspec, REFSPEC_FETCH);
+ refspec_item_init_or_die(, refspec, REFSPEC_FETCH);
spec_src = spec.src;
if (!*spec_src || !strcmp(spec_src, "HEAD"))
spec_src = "HEAD";
diff --git a/refspec.c b/refspec.c
index 78edc48ae8..8806df0fd2 100644
--- a/refspec.c
+++ b/refspec.c
@@ -124,11 +124,16 @@ static int parse_refspec(struct refspec_item *item, const
char *refspec, int fet
return 1;
}
-void refspec_item_init(struct refspec_item *item, const char *refspec, int
fetch)
+int refspec_item_init(struct refspec_item *item, const char *refspec, int
fetch)
{
memset(item, 0, sizeof(*item));
+ int ret = parse_refspec(item, refspec, fetch);
+ return ret;
+}
- if (!parse_refspec(item, refspec, fetch))
+void refspec_item_init_or_die(struct refspec_item *item, const char *refspec,
int fetch)
+{
+ if (!refspec_item_init(item, refspec, fetch))
die("Invalid refspec '%s'", refspec);
}
@@ -152,7 +157,7 @@ void refspec_append(struct refspec *rs, const char *refspec)
{
struct refspec_item item;
- refspec_item_init(, refspec, rs->fetch);
+ refspec_item_init_or_die(, refspec, rs->fetch);
ALLOC_GROW(rs->items, rs->nr + 1, rs->alloc);
rs->items[rs->nr++] = item;
@@ -191,7 +196,7 @@ void refspec_clear(struct refspec *rs)
int valid_fetch_refspec(const char *fetch_refspec_str)
{
struct refspec_item refspec;
- int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
+ int ret = refspec_item_init(, fetch_refspec_str, REFSPEC_FETCH);
refspec_item_clear();
return ret;
}
diff --git a/refspec.h b/refspec.h
index 3a9363887c..ed5d997f7f 100644
--- a/refspec.h
+++ b/refspec.h
@@ -32,7 +32,8 @@ struct refspec {
int fetch;
};
-void refspec_item_init(struct refspec_item *item, const char *refspec, int
fetch);
+int refspec_item_init(struct refspec_item *item, const char *refspec, int
fetch);
+void refspec_item_init_or_die(struct refspec_item *item, const char *refspec,
int fetch);
void refspec_item_clear(struct refspec_item *item);
void refspec_init(struct refspec *rs, int fetch);
void refspec_append(struct refspec *rs, const char *refspec);
I.e. let's fix the bug, but with this admittedly more verbose fix we're
left with exactly two memset() in refspec.c, one for each type of struct
that's initialized by the API.
The reason this is difficult now is because the current API conflates
the init function with an init_or_die, which is what most callers want,
so let's just split those concerns up. Then we're left with one init
function that does the memset.
[RFC PATCH 1/2] docs: reflect supported fetch options of git pull
`git pull` understands some options of `git fetch` which then uses in its operation. The documentation of `git pull` doesn't reflect this clearly, showing options that are not yet supported (e.g. `--deepen`) and omitting options that are supported (e.g. `--prune`). Make the documentation consistent with present behaviour by hiding unavailable options only. Reported-by: Marius Giurgi Signed-off-by: Rafael Ascensão --- Marius asked on freenode.#git if pull supported `--prune`, upon inspection seems like the man page was missing some of the supported options and listing others that are not supported via pull. Here's a quick summary of the changes to pull's documentation: add: remove: --dry-run --deepen= -p, --prune --shallow-since= --refmap=--shallow-exclude= -t, --tags-u, --update-head-ok -j, --jobs= Documentation/fetch-options.txt | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index 8631e365f..da17d27c1 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -14,6 +14,7 @@ linkgit:git-clone[1]), deepen or shorten the history to the specified number of commits. Tags for the deepened commits are not fetched. +ifndef::git-pull[] --deepen=:: Similar to --depth, except it specifies the number of commits from the current shallow boundary instead of from the tip of @@ -27,6 +28,7 @@ Deepen or shorten the history of a shallow repository to exclude commits reachable from a specified remote branch or tag. This option can be specified multiple times. +endif::git-pull[] --unshallow:: If the source repository is complete, convert a shallow @@ -42,10 +44,8 @@ the current repository has the same history as the source repository. .git/shallow. This option updates .git/shallow and accept such refs. -ifndef::git-pull[] --dry-run:: Show what would be done, without making any changes. -endif::git-pull[] -f:: --force:: @@ -63,6 +63,7 @@ ifndef::git-pull[] --multiple:: Allow several and arguments to be specified. No s may be specified. +endif::git-pull[] -p:: --prune:: @@ -76,8 +77,14 @@ ifndef::git-pull[] subject to pruning. Supplying `--prune-tags` is a shorthand for providing the tag refspec. + +ifdef::git-pull[] +See the PRUNING section on linkgit:git-fetch[1] for more details. +endif::git-pull[] +ifndef::git-pull[] See the PRUNING section below for more details. +endif::git-pull[] +ifndef::git-pull[] -P:: --prune-tags:: Before fetching, remove any local tags that no longer exist on @@ -89,9 +96,6 @@ See the PRUNING section below for more details. + See the PRUNING section below for more details. -endif::git-pull[] - -ifndef::git-pull[] -n:: endif::git-pull[] --no-tags:: @@ -101,7 +105,6 @@ endif::git-pull[] behavior for a remote may be specified with the remote..tagOpt setting. See linkgit:git-config[1]. -ifndef::git-pull[] --refmap=:: When fetching refs listed on the command line, use the specified refspec (can be given more than once) to map the @@ -119,6 +122,7 @@ ifndef::git-pull[] is used (though tags may be pruned anyway if they are also the destination of an explicit refspec; see `--prune`). +ifndef::git-pull[] --recurse-submodules[=yes|on-demand|no]:: This option controls if and under what conditions new commits of populated submodules should be fetched too. It can be used as a @@ -129,6 +133,7 @@ ifndef::git-pull[] when the superproject retrieves a commit that updates the submodule's reference to a commit that isn't already in the local submodule clone. +endif::git-pull[] -j:: --jobs=:: @@ -137,6 +142,7 @@ ifndef::git-pull[] submodules will be faster. By default submodules will be fetched one at a time. +ifndef::git-pull[] --no-recurse-submodules:: Disable recursive fetching of submodules (this has the same effect as using the `--recurse-submodules=no` option). @@ -153,7 +159,6 @@ ifndef::git-pull[] recursion (such as settings in linkgit:gitmodules[5] and linkgit:git-config[1]) override this option, as does specifying --[no-]recurse-submodules directly. -endif::git-pull[] -u:: --update-head-ok:: @@ -163,6 +168,7 @@ endif::git-pull[] to communicate with 'git fetch', and unless you are implementing your own Porcelain you are not supposed to use it. +endif::git-pull[] --upload-pack :: When given, and the repository to fetch from is handled -- 2.17.1
[RFC PATCH 2/2] pull: allow -e as a synonym for --edit
`git pull`'s documentation mentions that `--edit` can be used with short
option `-e`. But `git pull` doesn't understand `-e`.
To make things consistent, teach `git pull` `-e` for `--edit`
Signed-off-by: Rafael Ascensão
---
builtin/pull.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index e32d6cd5b..dd54f2e57 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -154,7 +154,7 @@ static struct option pull_options[] = {
OPT_PASSTHRU(0, "commit", _commit, NULL,
N_("perform a commit if the merge succeeds (default)"),
PARSE_OPT_NOARG),
- OPT_PASSTHRU(0, "edit", _edit, NULL,
+ OPT_PASSTHRU('e', "edit", _edit, NULL,
N_("edit message before committing"),
PARSE_OPT_NOARG),
OPT_PASSTHRU(0, "ff", _ff, NULL,
--
2.17.1
[PATCH] config.c: fix regression for core.safecrlf false
A regression introduced in 8462ff43e42ab67cecd16fdfb59451a53cc8a945 caused autocrlf rewrites to produce a warning message despite setting safecrlf=false. Signed-off-by: Anthony Sottile --- config.c| 2 +- t/t0020-crlf.sh | 10 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index fbbf0f8..de24e90 100644 --- a/config.c +++ b/config.c @@ -1233,7 +1233,7 @@ static int git_default_core_config(const char *var, const char *value) } eol_rndtrp_die = git_config_bool(var, value); global_conv_flags_eol = eol_rndtrp_die ? - CONV_EOL_RNDTRP_DIE : CONV_EOL_RNDTRP_WARN; + CONV_EOL_RNDTRP_DIE : 0; return 0; } diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh index 71350e0..5f05698 100755 --- a/t/t0020-crlf.sh +++ b/t/t0020-crlf.sh @@ -98,6 +98,16 @@ test_expect_success 'safecrlf: git diff demotes safecrlf=true to warn' ' ' +test_expect_success 'safecrlf: no warning with safecrlf=false' ' + git config core.autocrlf input && + git config core.safecrlf false && + + for w in I am all CRLF; do echo $w; done | append_cr >allcrlf && + git add allcrlf 2>err && + test_must_be_empty err +' + + test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' ' git config core.autocrlf false && git config core.safecrlf false && -- 2.7.4
Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases
On Mon, Jun 04, 2018 at 11:05:43PM +0800, Xiaolong Ye wrote: > When users specify the commit range with 'Z..C' pattern for format-patch, all > the parents of Z (including Z) would be marked as UNINTERESTING which would > prevent revision walk in prepare_bases from getting the prerequisite commits, > thus `git format-patch --base Z..C` won't be able to > generate > the list of prerequisite patch ids. Clear UNINTERESTING flag with > clear_object_flags solves this issue. > > Reported-by: Eduardo Habkost > Signed-off-by: Xiaolong Ye Thanks! The fix works for me. Tested-by: Eduardo Habkost -- Eduardo
Re: Regression (?) in core.safecrlf=false messaging
On 04.06.18 17:43, Anthony Sottile wrote: > On Mon, Jun 4, 2018 at 12:55 AM, Torsten Bögershausen wrote: >> >> Does the following patch fix your problem ? >> >> diff --git a/config.c b/config.c >> index 6f8f1d8c11..c625aec96a 100644 >> --- a/config.c >> +++ b/config.c >> @@ -1233,7 +1233,7 @@ static int git_default_core_config(const char *var, >> const char *value) >> } >> eol_rndtrp_die = git_config_bool(var, value); >> global_conv_flags_eol = eol_rndtrp_die ? >> - CONV_EOL_RNDTRP_DIE : CONV_EOL_RNDTRP_WARN; >> + CONV_EOL_RNDTRP_DIE : 0; >> return 0; >> } >> > > > Yes! After applying that patch: > > ``` > > $ PATH=$PWD/prefix/bin:$PATH bash test.sh > + git --version > git version 2.18.0.rc1.dirty > + rm -rf repo > + git init repo > Initialized empty Git repository in /tmp/git/repo/.git/ > + cd repo > + git config core.autocrlf input > + git config core.safecrlf false > + echo -en 'foo\r\nbar\r\n' > + git add f > > ``` > > Anthony > Good. Do you want to send the patch to the list ? (You don't have too, if you don't want, but as you already did all the work...)
Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config
On Mon, Jun 04, 2018 at 12:18:59PM -0400, Martin-Louis Bright wrote: > Why must the credentials must be deleted after receiving the 401 (or > any) error? What's the rationale for this? Because Git only tries a single credential per invocation. So if a helper provides one, it doesn't prompt. If you get a 401 and then the program aborts, invoking it again is just going to try the same credential over and over. Dropping the credential from the helper breaks out of that loop. In fact, this patch probably should give the user some advice in that regard (either in the documentation, or as a warning when we skip the rejection). If you _do_ have a bogus credential and set the new option, you'd need to reject it manually (you can do it with "git credential reject", but it's probably easier to just unset the option temporarily and re-invoke the original command). -Peff
Re: GDPR compliance best practices?
On Mon, Jun 04, 2018 at 09:47:18AM -0400, Theodore Y. Ts'o wrote: > For people who are doing real work on git repos, other commands that > we very much care about include "git log --author=", "git > tag --contains", "git blame", etc. I do not see how those, or anything but git clone (and even that only if author verification is requested) could possibly be affected in any significant way. Best wishes Peter -- Peter Backes, r...@helen.plasma.xg8.de
[PATCH 1/1] merge-recursive: give notice when submodule commit gets fast-forwarded
From: Leif Middelschulte
Since submodules are treated similarly to ordinary files (i.e. not as 'dumb'
pointers), an automatic merge should be mentioned if the user asks for it.
Just as it is mentioned for oridnary files.
Signed-off-by: Leif Middelschulte
---
merge-recursive.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/merge-recursive.c b/merge-recursive.c
index a4b91d17f..0990a135b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1093,10 +1093,20 @@ static int merge_submodule(struct merge_options *o,
/* Case #1: a is contained in b or vice versa */
if (in_merge_bases(commit_a, commit_b)) {
oidcpy(result, b);
+ if (show(o, 2))
+ output(o, 2, _("Auto-merging %s"), path);
+ else
+ ; /* no output */
+
return 1;
}
if (in_merge_bases(commit_b, commit_a)) {
oidcpy(result, a);
+ if (show(o, 2))
+ output(o, 2, _("Auto-merging %s"), path);
+ else
+ ; /* no output */
+
return 1;
}
--
2.15.1 (Apple Git-101)
[PATCH v4 0/1] merge-recursive: give notice when submodule commit gets fast-forwarded
From: Leif Middelschulte The provided patch is in response to Elijah Newren's [0] and Junio Hamano's [1] comments on my prior patch regarding the reasoning and implementation of a user notification during (clean) merges of submodules. [0] https://public-inbox.org/git/xmqqo9hg7554@gitster-ct.c.googlers.com/#t [1] https://public-inbox.org/git/xmqqzi0t1waf@gitster-ct.c.googlers.com/ Leif Middelschulte (1): Inform about Auto-merging of submodules during merge merge-recursive.c | 10 ++ 1 file changed, 10 insertions(+) -- 2.15.1 (Apple Git-101)
BUSINESS INTEREST/ PROPOSAL
Hello RE:BUSINESS INQUIRY/ PROPOSAL How are you doing today, i hope this mail finds you in a good and convenient position! My name is ZHAO DONG. I am the senior manager for Procurement, Hong Kong Refining Company (Sinopec Group Inc) I have been mandated to source crude oil from Libya for supply to our refineries. However, I have been able to establish a good relationship with the senior management of the Azzawya Oil Refining Company, Libya. I am now looking for a competent middle man to stand in between my company, Hong Kong Refining Company and the Azzawya Oil Refining Company of Libya for the sale and purchase of 2 Million Barrels Monthly for 36 Months. This is in order to take home a commission of USD5 to USD7 per barrel. This amount is payable to the middle man as commission. On your response I will give you further details you may need and proof of my identity. Kindly reply directly to zhaodong...@gmail.com or zhaodon...@yandex.com for further vital details you may need. Best Regards ZHAO DONG
Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`
On 06/04, Martin Ågren wrote:
> We allocate a `struct refspec_item` on the stack without initializing
> it. In particular, its `dst` and `src` members will contain some random
> data from the stack. When we later call `refspec_item_clear()`, it will
> call `free()` on those pointers. So if the call to `parse_refspec()` did
> not assign to them, we will be freeing some random "pointers". This is
> undefined behavior.
>
> To the best of my understanding, this cannot currently be triggered by
> user-provided data. And for what it's worth, the test-suite does not
> trigger this with SANITIZE=address. It can be provoked by calling
> `valid_fetch_refspec(":*")`.
>
> Zero the struct, as is done in other users of `struct refspec_item`.
>
> Signed-off-by: Martin Ågren
> ---
> I found some time to look into this. It does not seem to be a
> user-visible bug, so not particularly critical.
Thanks for fixing this. I don't think I noticed this because at some
point in developing this series I had a memset call in parse_refspec.
I don't remember why I ended up removing it, but maybe it would have
been better to leave it in there.
>
> refspec.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/refspec.c b/refspec.c
> index ada7854f7a..7dd7e361e5 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -189,7 +189,10 @@ void refspec_clear(struct refspec *rs)
> int valid_fetch_refspec(const char *fetch_refspec_str)
> {
> struct refspec_item refspec;
> - int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> + int ret;
> +
> + memset(, 0, sizeof(refspec));
> + ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> refspec_item_clear();
> return ret;
> }
> --
> 2.18.0.rc0.43.gb85e7bcbff
>
--
Brandon Williams
Re: [PATCH/RFC/BUG] unpack-trees.c: do not use "the_index"
On 06/02, Elijah Newren wrote: > On Fri, Jun 1, 2018 at 10:03 PM, Duy Nguyen wrote: > > On Fri, Jun 1, 2018 at 8:34 PM, Elijah Newren wrote: > >> On Fri, Jun 1, 2018 at 9:11 AM, Nguyễn Thái Ngọc Duy > >> wrote: > >>> This is more of a bug report than an actual fix because I'm not sure > >>> if "o->src_index" is always the correct answer instead of "the_index" > >>> here. But this is very similar to 7db118303a (unpack_trees: fix > >>> breakage when o->src_index != o->dst_index - 2018-04-23) and could > >>> potentially break things again... > > I'm pretty sure your patch is correct. Adding Brandon Williams to the > cc for comment since his patches came up in the analysis below... > > >> Actually, I don't think the patch will break anything in the current > >> code. Currently, all callers of unpack_trees() (even merge recursive > >> which uses different src_index and dst_index now) set src_index = > >> _index. So, these changes should continue to work as before (with > >> a minor possible exception of merge-recursive calling into other > >> functions from unpack-trees.c after unpack_trees() has finished..). > >> That's not to say that your patch is bug free, just that I think any > >> bugs shouldn't be triggerable from the current codebase. > > > > Ah.. I thought merge-recursive would do fancier things and used some > > temporary index. Good to know. > > Well, it does does use a temporary index, but for dst_index rather > than src_index. It then does some fancier things, but not until the > call to unpack_trees() is over. In particular, at that point, it > swaps the_index and tmp_index, reversing their roles so that now > tmp_index is the original index and the_index becomes the result after > unpack_trees() is run. That's done because I later want to use the > original index for calling verify_uptodate(). verify_uptodate() is > then used for was_tracked_and_matches() and was_tracked(). > > Anyway, the whole upshot of this is: > * merge-recursive uses src_index == _index for the unpack_trees() call. > * merge-recursive uses src_index == o->orig_index for subsequent > calls to verify_uptodate(), but verify_uptodate() doesn't call into > any of the sites you have modified. > > Further: > * Every other existing caller of unpack-trees in the code sets > src_index == _index, so this won't break any of them. > * There are only two callers in the codebase that set dst_index to > something other than _index -- diff-lib.c (which sets it to NULL) > and merge-recursive (which does the stuff described above). > > So, having done that analysis, I am now pretty convinced your patch > won't break anything. That's one half... > > >> Also, if any of the changes you made are wrong, what was there before > >> was also clearly wrong. So I think we're at least no worse off. > >> > >> But, I agree, it's not easy to verify what the correct thing should be > >> in all cases. I'll try to take a closer look in the next couple days. > > > > Thanks. I will also stare at this code some more in the next couple > > days trying to remember what these functions do. > > Your patch has two divisible parts: > > 1) Your modifications to > * clear_ce_flags_1() > * clear_ce_flags_dir() > * clear_ce_flags() > * mark_new_skip_worktree() > The clear_ce_flags*() functions are only called by each other and by > mark_new_skip_worktree(), which in turn is only called from > unpack_trees(). Also, in all of these, your change ends up only > modifying what index_state is passed to is_excluded_from_list(). > > 2) Your modifications to > * verify_clean_subdirectory() > * check_ok_to_remove() > In this case, the former is only called by the latter, and the latter > ends up only being called (via verify_absent_1()) from verify_absent() > and verify_absent_sparse(). > > I'll address each, in reverse order. > > 2) The stuff that affects verify_absent() > > While verify_absent() is not called from merge-recursive right now, it > was something I wanted to use in the future for very similar reasons > that verify_uptodate() started being called directly from > merge-recursive. In particular, if the rewrite of merge-recursive[A] > I want to do sets index_only when calling unpack_trees(), then does > the whole merge without touching the worktree, then at the end goes to > update the working tree, it will need to do extra checks. > verify_absent() will come in handy as one of those extra checks. For > that case, using the_index (the new index just created with lots of > changes) would be wrong in all the same ways that using the_index > caused massive problems for was_tracked() in merge-recursive (e.g. the > blow up of when Junio merged the original directory rename detection > series into master and subsequently reverted it); we'd instead want > src_index (which was the index that existed when merge was called) > instead. So, with this patch you've fixed some important bugs that I > would have hit later. > > [A]
[PATCH 6/6] fetch-pack: introduce negotiator API
Introduce the new files fetch-negotiator.{h,c}, which contains an API
behind which the details of negotiation are abstracted. Currently, only
one algorithm is available: the existing one.
This patch is written to be more easily reviewed: static functions are
moved verbatim from fetch-pack.c to negotiator/default.c, and it can be
seen that the lines replaced by negotiator->X() calls are present in the
X() functions respectively.
Signed-off-by: Jonathan Tan
---
Makefile | 2 +
fetch-negotiator.c | 7 ++
fetch-negotiator.h | 45 ++
fetch-pack.c | 207 ++-
negotiator/default.c | 173
negotiator/default.h | 8 ++
object.h | 3 +-
7 files changed, 282 insertions(+), 163 deletions(-)
create mode 100644 fetch-negotiator.c
create mode 100644 fetch-negotiator.h
create mode 100644 negotiator/default.c
create mode 100644 negotiator/default.h
diff --git a/Makefile b/Makefile
index 4bca65383..8f1df08ac 100644
--- a/Makefile
+++ b/Makefile
@@ -859,6 +859,7 @@ LIB_OBJS += ewah/ewah_bitmap.o
LIB_OBJS += ewah/ewah_io.o
LIB_OBJS += ewah/ewah_rlw.o
LIB_OBJS += exec-cmd.o
+LIB_OBJS += fetch-negotiator.o
LIB_OBJS += fetch-object.o
LIB_OBJS += fetch-pack.o
LIB_OBJS += fsck.o
@@ -891,6 +892,7 @@ LIB_OBJS += merge-blobs.o
LIB_OBJS += merge-recursive.o
LIB_OBJS += mergesort.o
LIB_OBJS += name-hash.o
+LIB_OBJS += negotiator/default.o
LIB_OBJS += notes.o
LIB_OBJS += notes-cache.o
LIB_OBJS += notes-merge.o
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
new file mode 100644
index 0..756c35d15
--- /dev/null
+++ b/fetch-negotiator.c
@@ -0,0 +1,7 @@
+#include "fetch-negotiator.h"
+#include "negotiator/default.h"
+
+void fetch_negotiator_init(struct fetch_negotiator *negotiator)
+{
+ default_negotiator_init(negotiator);
+}
diff --git a/fetch-negotiator.h b/fetch-negotiator.h
new file mode 100644
index 0..b717b80dd
--- /dev/null
+++ b/fetch-negotiator.h
@@ -0,0 +1,45 @@
+#ifndef FETCH_NEGOTIATOR
+#define FETCH_NEGOTIATOR
+
+#include "cache.h"
+#include "commit.h"
+
+struct fetch_negotiator {
+ /*
+* Before negotiation starts, indicate that the server is known to have
+* this commit.
+*/
+ void (*known_common)(struct fetch_negotiator *, struct commit *);
+
+ /*
+* Once this function is invoked, known_common() cannot be invoked any
+* more.
+*
+* Indicate that this commit and all its ancestors are to be checked
+* for commonality with the server.
+*/
+ void (*add_tip)(struct fetch_negotiator *, struct commit *);
+
+ /*
+* Once this function is invoked, known_common() and add_tip() cannot
+* be invoked any more.
+*
+* Return the next commit that the client should send as a "have" line.
+*/
+ const struct object_id *(*next)(struct fetch_negotiator *);
+
+ /*
+* Inform the negotiator that the server has the given commit. This
+* method must only be called on commits returned by next().
+*/
+ int (*ack)(struct fetch_negotiator *, struct commit *);
+
+ void (*release)(struct fetch_negotiator *);
+
+ /* internal use */
+ void *data;
+};
+
+void fetch_negotiator_init(struct fetch_negotiator *negotiator);
+
+#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index 54dd3feb8..a399a62e1 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -15,10 +15,10 @@
#include "connect.h"
#include "transport.h"
#include "version.h"
-#include "prio-queue.h"
#include "sha1-array.h"
#include "oidset.h"
#include "packfile.h"
+#include "fetch-negotiator.h"
static int transfer_unpack_limit = -1;
static int fetch_unpack_limit = -1;
@@ -36,13 +36,7 @@ static const char *alternate_shallow_file;
/* Remember to update object flag allocation in object.h */
#define COMPLETE (1U << 0)
-#define COMMON (1U << 1)
-#define COMMON_REF (1U << 2)
-#define SEEN (1U << 3)
-#define POPPED (1U << 4)
-#define ALTERNATE (1U << 5)
-
-static int marked;
+#define ALTERNATE (1U << 1)
/*
* After sending this many "have"s if we do not get any new ACK , we
@@ -50,11 +44,6 @@ static int marked;
*/
#define MAX_IN_VAIN 256
-struct data {
- struct prio_queue rev_list;
- int non_common_revs;
-};
-
static int multi_ack, use_sideband;
/* Allow specifying sha1 if it is a ref tip. */
#define ALLOW_TIP_SHA1 01
@@ -97,8 +86,8 @@ static void cache_one_alternate(const char *refname,
cache->items[cache->nr++] = obj;
}
-static void for_each_cached_alternate(struct data *data,
- void (*cb)(struct data *, struct object
*))
+static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
+ void (*cb)(struct fetch_negotiator *,
struct object *))
{
static int
[PATCH 5/6] fetch-pack: move common check and marking together
This enables the calculation of was_common and the invocation to
mark_common() to be abstracted into a single call to the negotiator API
(to be introduced in a subsequent patch).
Signed-off-by: Jonathan Tan
---
fetch-pack.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index ec92929bc..54dd3feb8 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -499,11 +499,14 @@ static int find_common(struct data *data, struct
fetch_pack_args *args,
case ACK_continue: {
struct commit *commit =
lookup_commit(result_oid);
+ int was_common;
if (!commit)
die(_("invalid commit %s"),
oid_to_hex(result_oid));
+ was_common = commit->object.flags &
COMMON;
+ mark_common(data, commit, 0, 1);
if (args->stateless_rpc
&& ack == ACK_common
-&& !(commit->object.flags & COMMON)) {
+&& !was_common) {
/* We need to replay the have
for this object
* on the next RPC request so
the peer knows
* it is in common with us.
@@ -520,7 +523,6 @@ static int find_common(struct data *data, struct
fetch_pack_args *args,
} else if (!args->stateless_rpc
|| ack != ACK_common)
in_vain = 0;
- mark_common(data, commit, 0, 1);
retval = 0;
got_continue = 1;
if (ack == ACK_ready)
--
2.17.0.768.g1526ddbba1.dirty
[PATCH 4/6] fetch-pack: make negotiation-related vars local
Reduce the number of global variables by making the priority queue and
the count of non-common commits in it local, passing them as a struct to
various functions where necessary.
This also helps in the case that fetch_pack() is invoked twice in the
same process (when tag following is required when using a transport that
does not support tag following), in that different priority queues will
now be used in each invocation, instead of reusing the possibly
non-empty one.
The struct containing these variables is named "data" to ease review of
a subsequent patch in this series - in that patch, this struct
definition and several functions will be moved to a negotiation-specific
file, and this allows the move to be verbatim.
Signed-off-by: Jonathan Tan
---
fetch-pack.c | 104 +--
1 file changed, 59 insertions(+), 45 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 192771a8f..ec92929bc 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -50,8 +50,12 @@ static int marked;
*/
#define MAX_IN_VAIN 256
-static struct prio_queue rev_list = { compare_commits_by_commit_date };
-static int non_common_revs, multi_ack, use_sideband;
+struct data {
+ struct prio_queue rev_list;
+ int non_common_revs;
+};
+
+static int multi_ack, use_sideband;
/* Allow specifying sha1 if it is a ref tip. */
#define ALLOW_TIP_SHA1 01
/* Allow request of a sha1 if it is reachable from a ref (possibly hidden
ref). */
@@ -93,7 +97,8 @@ static void cache_one_alternate(const char *refname,
cache->items[cache->nr++] = obj;
}
-static void for_each_cached_alternate(void (*cb)(struct object *))
+static void for_each_cached_alternate(struct data *data,
+ void (*cb)(struct data *, struct object
*))
{
static int initialized;
static struct alternate_object_cache cache;
@@ -105,10 +110,10 @@ static void for_each_cached_alternate(void (*cb)(struct
object *))
}
for (i = 0; i < cache.nr; i++)
- cb(cache.items[i]);
+ cb(data, cache.items[i]);
}
-static void rev_list_push(struct commit *commit, int mark)
+static void rev_list_push(struct data *data, struct commit *commit, int mark)
{
if (!(commit->object.flags & mark)) {
commit->object.flags |= mark;
@@ -116,19 +121,20 @@ static void rev_list_push(struct commit *commit, int mark)
if (parse_commit(commit))
return;
- prio_queue_put(_list, commit);
+ prio_queue_put(>rev_list, commit);
if (!(commit->object.flags & COMMON))
- non_common_revs++;
+ data->non_common_revs++;
}
}
-static int rev_list_insert_ref(const char *refname, const struct object_id
*oid)
+static int rev_list_insert_ref(struct data *data, const char *refname,
+ const struct object_id *oid)
{
struct object *o = deref_tag(parse_object(oid), refname, 0);
if (o && o->type == OBJ_COMMIT)
- rev_list_push((struct commit *)o, SEEN);
+ rev_list_push(data, (struct commit *)o, SEEN);
return 0;
}
@@ -136,7 +142,7 @@ static int rev_list_insert_ref(const char *refname, const
struct object_id *oid)
static int rev_list_insert_ref_oid(const char *refname, const struct object_id
*oid,
int flag, void *cb_data)
{
- return rev_list_insert_ref(refname, oid);
+ return rev_list_insert_ref(cb_data, refname, oid);
}
static int clear_marks(const char *refname, const struct object_id *oid,
@@ -156,7 +162,7 @@ static int clear_marks(const char *refname, const struct
object_id *oid,
when only the server does not yet know that they are common).
*/
-static void mark_common(struct commit *commit,
+static void mark_common(struct data *data, struct commit *commit,
int ancestors_only, int dont_parse)
{
if (commit != NULL && !(commit->object.flags & COMMON)) {
@@ -166,12 +172,12 @@ static void mark_common(struct commit *commit,
o->flags |= COMMON;
if (!(o->flags & SEEN))
- rev_list_push(commit, SEEN);
+ rev_list_push(data, commit, SEEN);
else {
struct commit_list *parents;
if (!ancestors_only && !(o->flags & POPPED))
- non_common_revs--;
+ data->non_common_revs--;
if (!o->parsed && !dont_parse)
if (parse_commit(commit))
return;
@@ -179,7 +185,7 @@ static void mark_common(struct commit *commit,
for (parents = commit->parents;
parents;
parents =
[PATCH 3/6] fetch-pack: in protocol v2, enqueue commons first
In do_fetch_pack_v2(), rev_list_insert_ref_oid() is invoked before everything_local(). This means that if we have a commit that is both our ref and their ref, it would be enqueued by rev_list_insert_ref_oid() as SEEN, and since it is thus already SEEN, everything_local() would not enqueue it. If everything_local() were invoked first, as it is in do_fetch_pack() for protocol v0, then everything_local() would enqueue it with COMMON_REF | SEEN. The addition of COMMON_REF ensures that its parents are not sent as "have" lines. Change the order in do_fetch_pack_v2() to be consistent with do_fetch_pack(), and to avoid sending unnecessary "have" lines. Signed-off-by: Jonathan Tan --- fetch-pack.c | 6 +++--- t/t5500-fetch-pack.sh | 35 +++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 2d090f612..192771a8f 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1372,14 +1372,14 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, for_each_ref(clear_marks, NULL); marked = 1; - for_each_ref(rev_list_insert_ref_oid, NULL); - for_each_cached_alternate(insert_one_alternate_object); - /* Filter 'ref' by 'sought' and those that aren't local */ if (everything_local(args, , sought, nr_sought)) state = FETCH_DONE; else state = FETCH_SEND_REQUEST; + + for_each_ref(rev_list_insert_ref_oid, NULL); + for_each_cached_alternate(insert_one_alternate_object); break; case FETCH_SEND_REQUEST: if (send_fetch_request(fd[1], args, ref, , diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 0680dec80..ad6a50ad6 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -808,6 +808,41 @@ test_expect_success 'fetch with --filter=blob:limit=0' ' fetch_filter_blob_limit_zero server server ' +test_expect_success 'use ref advertisement to prune "have" lines sent' ' + rm -rf server client && + git init server && + test_commit -C server aref_both_1 && + git -C server tag -d aref_both_1 && + test_commit -C server aref_both_2 && + + # The ref name that only the server has must be a prefix of all the + # others, to ensure that the client has the same information regardless + # of whether protocol v0 (which does not have ref prefix filtering) or + # protocol v2 (which does) is used. + git clone server client && + test_commit -C server aref && + test_commit -C client aref_client && + + # In both protocol v0 and v2, ensure that the parent of aref_both_2 is + # not sent as a "have" line. + + rm -f trace && + cp -r client clientv0 && + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \ + fetch origin aref && + grep "have $(git -C client rev-parse aref_client)" trace && + grep "have $(git -C client rev-parse aref_both_2)" trace && + ! grep "have $(git -C client rev-parse aref_both_2^)" trace && + + rm -f trace && + cp -r client clientv2 && + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \ + fetch origin aref && + grep "have $(git -C client rev-parse aref_client)" trace && + grep "have $(git -C client rev-parse aref_both_2)" trace && + ! grep "have $(git -C client rev-parse aref_both_2^)" trace +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd -- 2.17.0.768.g1526ddbba1.dirty
[PATCH 0/6] Refactor fetch negotiation into its own API
Both Stefan and Junio have suggested in [1] that I create a new negotiation API first, then implement a new algorithm, and I perhaps too optimistically said that doing it all at once should be fine. Extracting the API turns out to be more difficult than expected. Here are the patches that do that, without any new algorithms. The patches include 3 patches that eliminate some minor differences between protocol v0 and protocol v2, then 3 patches that implement the API. I tried to write tests for all of the former 3, but could only do so for one, because I didn't know how to construct situations that show a difference in behavior as an effect of such a code change (or, if such a situation is possible without custom transports). As for the latter 3, these are refactorings with no user-visible changes, and I have tried to write them in such a way that it is clear to a reviewer that the same functionality and logic is present before and after, and that they are just organized better. Overall, these do not change much functionality, but would be a good base for contributors (myself and even others) to experiment with negotiation algorithms. [1] https://public-inbox.org/git/20180521204340.260572-1-jonathanta...@google.com/ Jonathan Tan (6): fetch-pack: clear marks before everything_local() fetch-pack: truly stop negotiation upon ACK ready fetch-pack: in protocol v2, enqueue commons first fetch-pack: make negotiation-related vars local fetch-pack: move common check and marking together fetch-pack: introduce negotiator API Makefile | 2 + fetch-negotiator.c| 7 ++ fetch-negotiator.h| 45 + fetch-pack.c | 222 -- negotiator/default.c | 173 negotiator/default.h | 8 ++ object.h | 3 +- t/t5500-fetch-pack.sh | 35 +++ 8 files changed, 332 insertions(+), 163 deletions(-) create mode 100644 fetch-negotiator.c create mode 100644 fetch-negotiator.h create mode 100644 negotiator/default.c create mode 100644 negotiator/default.h -- 2.17.0.768.g1526ddbba1.dirty
[PATCH 1/6] fetch-pack: clear marks before everything_local()
If tag following is required when using a transport that does not
support tag following, fetch_pack() will be invoked twice in the same
process, necessitating a clearing of the object flags used by
fetch_pack() sometime during the second invocation. This is currently
done in find_common(), which means that the work done by
everything_local() in marking complete remote refs as COMMON_REF is
wasted.
To avoid this wastage, move this clearing from find_common() to its
parent function do_fetch_pack(), right before it calls
everything_local().
This has been occurring since the commit that introduced the clearing of
marks, 420e9af498 ("Fix tag following", 2008-03-19).
The corresponding code for protocol v2 in do_fetch_pack_v2() does not
have this problem, as the clearing of flags is done before any marking
(whether by rev_list_insert_ref_oid() or everything_local()).
Signed-off-by: Jonathan Tan
---
fetch-pack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index a320ce987..1358973a4 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -336,9 +336,6 @@ static int find_common(struct fetch_pack_args *args,
if (args->stateless_rpc && multi_ack == 1)
die(_("--stateless-rpc requires multi_ack_detailed"));
- if (marked)
- for_each_ref(clear_marks, NULL);
- marked = 1;
for_each_ref(rev_list_insert_ref_oid, NULL);
for_each_cached_alternate(insert_one_alternate_object);
@@ -1053,6 +1050,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args
*args,
if (!server_supports("deepen-relative") && args->deepen_relative)
die(_("Server does not support --deepen"));
+ if (marked)
+ for_each_ref(clear_marks, NULL);
+ marked = 1;
if (everything_local(args, , sought, nr_sought)) {
packet_flush(fd[1]);
goto all_done;
--
2.17.0.768.g1526ddbba1.dirty
[PATCH 2/6] fetch-pack: truly stop negotiation upon ACK ready
When "ACK %s ready" is received, find_common() clears rev_list in an
attempt to stop further "have" lines from being sent [1]. This appears
to work, despite the invocation to mark_common() in the "while" loop.
Though it is possible for mark_common() to invoke rev_list_push() (thus
making rev_list non-empty once more), it is more likely that the commits
in rev_list that have un-SEEN parents are also unparsed, meaning that
mark_common() is not invoked on them.
To avoid all this uncertainty, it is better to explicitly end the loop
when "ACK %s ready" is received instead of clearing rev_list. Remove the
clearing of rev_list and write "if (got_ready) break;" instead.
The corresponding code for protocol v2 in process_acks() does not have
the same problem, because the invoker of process_acks()
(do_fetch_pack_v2()) proceeds immediately to processing the packfile
upon "ACK %s ready". The clearing of rev_list here is thus redundant,
and this patch also removes it.
[1] The rationale is further described in the originating commit
f2cba9299b ("fetch-pack: Finish negotation if remote replies "ACK %s
ready"", 2011-03-14).
Signed-off-by: Jonathan Tan
---
fetch-pack.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 1358973a4..2d090f612 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -517,10 +517,8 @@ static int find_common(struct fetch_pack_args *args,
mark_common(commit, 0, 1);
retval = 0;
got_continue = 1;
- if (ack == ACK_ready) {
- clear_prio_queue(_list);
+ if (ack == ACK_ready)
got_ready = 1;
- }
break;
}
}
@@ -530,6 +528,8 @@ static int find_common(struct fetch_pack_args *args,
print_verbose(args, _("giving up"));
break; /* give up */
}
+ if (got_ready)
+ break;
}
}
done:
@@ -1281,7 +1281,6 @@ static int process_acks(struct packet_reader *reader,
struct oidset *common)
}
if (!strcmp(reader->line, "ready")) {
- clear_prio_queue(_list);
received_ready = 1;
continue;
}
--
2.17.0.768.g1526ddbba1.dirty
Re: [PATCH] add -p: fix counting empty context lines in edited patches
On Mon, Jun 4, 2018 at 6:08 AM, Phillip Wood wrote:
> On 01/06/18 21:03, Eric Sunshine wrote:
>> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood
>> wrote:
>>> + } elsif ($mode eq ' ' or $_ eq "\n") {
>>
>> Based upon a very cursory read of parts of git-add-interactive.perl,
>> do I understand correctly that we don't have to worry about $_ ever
>> being "\r\n" on Windows?
>
> Good question, I think the short answer no. If my understanding of the
> newline section of perlport [1] is correct then on Windows "\n" eq
> "\012" and the io layer replaces "\015\012" with "\n" when reading in
> 'text' mode (which I think is the default if you don't specify one when
> opening the file/process or with binmode()).
That was my interpretation, as well (though I didn't audit the code closely).
> As "\n" is only one
> character it would perhaps be better to test '$mode' rather than '$_'
> above - what do you think.
That could be clearer. As a reviewer, I had to spend extra brain
cycles wondering why $mode was used everywhere else but $_ in just
this one place. (Not that it was difficult to figure out.)
>>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>>> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
>>> +test_expect_success 'setup file' '
>>> + [...]
>>> +'
>>> +test_expect_success 'setup patch' '
>>> + [...]
>>> +'
>>> +test_expect_success 'setup expected' '
>>> + [...]
>>> +'
>>> +test_expect_success 'edit can strip spaces from empty context lines' '
>>> + test_write_lines e n q | git add -p 2>error &&
>>> + test_must_be_empty error &&
>>> + git diff >output &&
>>> + diff_cmp expected output
>>> +'
>>
>> I would have expected all the setup work to be contained directly in
>> the sole test which needs it rather than spread over three tests (two
>> of which are composed of a single command). Not a big deal, and not
>> worth a re-roll.
>
> Good point I was torn between that and matching the existing style in
> that file seems to be to create a million ancillary tests to do the set-up.
I see what you mean. Following existing practice in the file makes
sense, though breaking from that practice by bundling all the setup
into the single test which uses it wouldn't hurt either. It's a
judgment call (and not worrying about too much).
Re: [PATCH v7 2/2] http-backend: respect CONTENT_LENGTH for receive-pack
On Mon, Jun 04, 2018 at 01:31:58PM +0900, Junio C Hamano wrote:
> Max Kirillov writes:
>> +size_t n = xread(0, buf, chunk_length);
>> +if (n < 0)
>> +die_errno("Reading request failed");
>
> n that is of type size_t is unsigned and cannot be negative here.
Thanks, fixing it
Do you think sanity check for n <= chunk_length (the code
will go mad in this case) is needed? As far as I can see n
returns straight from system's read()
Re: [PATCH v4 00/21] Integrate commit-graph into 'fsck' and 'gc'
Sorry I forgot to --in-reply-to the previous version [1]
[1]
https://public-inbox.org/git/20180524162504.158394-1-dsto...@microsoft.com/T/#u
On 6/4/2018 12:52 PM, Derrick Stolee wrote:
Thanks for the feedback on v3. There were several small cleanups, but
perhaps the biggest change is the addition of "commit-graph: use
string-list API for input" which makes "commit-graph: add '--reachable'
option" much simpler.
The inter-diff is still reasonably large, but I'll send it in a
follow-up PR.
s/PR/message
Here is that diff:
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9a3abd87e7..d2eb3c8e9b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -900,7 +900,8 @@ the `GIT_NOTES_REF` environment variable. See
linkgit:git-notes[1].
core.commitGraph::
Enable git commit graph feature. Allows reading from the
- commit-graph file.
+ commit-graph file. See `gc.commitGraph` for automatically
+ maintaining the file.
core.sparseCheckout::
Enable "sparse checkout" feature. See section "Sparse checkout" in
@@ -1554,10 +1555,11 @@ gc.autoDetach::
if the system supports it. Default is true.
gc.commitGraph::
- If true, then gc will rewrite the commit-graph file after any
- change to the object database. If '--auto' is used, then the
- commit-graph will not be updated unless the threshold is met.
- See linkgit:git-commit-graph[1] for details.
+ If true, then gc will rewrite the commit-graph file when
+ linkgit:git-gc[1] is run. When using linkgit:git-gc[1]
+ '--auto' the commit-graph will be updated if housekeeping is
+ required. Default is false. See linkgit:git-commit-graph[1]
+ for details.
gc.logExpiry::
If the file gc.log exists, then `git gc --auto` won't run
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 17dd654a59..a6526b3592 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -119,9 +119,9 @@ The optional configuration variable `gc.packRefs`
determines if
it within all non-bare repos or it can be set to a boolean value.
This defaults to true.
-The optional configuration variable 'gc.commitGraph' determines if
-'git gc' runs 'git commit-graph write'. This can be set to a boolean
-value. This defaults to false.
+The optional configuration variable `gc.commitGraph` determines if
+'git gc' should run 'git commit-graph write'. This can be set to a
+boolean value. This defaults to false.
The optional configuration variable `gc.aggressiveWindow` controls how
much time is spent optimizing the delta compression of the objects in
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 20ce6437ae..76423b3fa5 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -39,7 +39,7 @@ static struct opts_commit_graph {
static int graph_verify(int argc, const char **argv)
{
- struct commit_graph *graph = 0;
+ struct commit_graph *graph = NULL;
char *graph_name;
static struct option builtin_commit_graph_verify_options[] = {
@@ -119,13 +119,9 @@ static int graph_read(int argc, const char **argv)
static int graph_write(int argc, const char **argv)
{
- const char **pack_indexes = NULL;
- int packs_nr = 0;
- const char **commit_hex = NULL;
- int commits_nr = 0;
- const char **lines = NULL;
- int lines_nr = 0;
- int lines_alloc = 0;
+ struct string_list *pack_indexes = NULL;
+ struct string_list *commit_hex = NULL;
+ struct string_list lines;
static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", _dir,
@@ -158,32 +154,23 @@ static int graph_write(int argc, const char **argv)
if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
- lines_nr = 0;
- lines_alloc = 128;
- ALLOC_ARRAY(lines, lines_alloc);
-
- while (strbuf_getline(, stdin) != EOF) {
- ALLOC_GROW(lines, lines_nr + 1, lines_alloc);
- lines[lines_nr++] = strbuf_detach(, NULL);
- }
-
- if (opts.stdin_packs) {
- pack_indexes = lines;
- packs_nr = lines_nr;
- }
- if (opts.stdin_commits) {
- commit_hex = lines;
- commits_nr = lines_nr;
- }
+ string_list_init(, 0);
+
+ while (strbuf_getline(, stdin) != EOF)
+ string_list_append(, strbuf_detach(,
NULL));
+
+ if (opts.stdin_packs)
+ pack_indexes =
+ if (opts.stdin_commits)
+ commit_hex =
}
write_commit_graph(opts.obj_dir,
pack_indexes,
- packs_nr,
[PATCH v4 04/21] commit: force commit to parse from object database
In anticipation of verifying commit-graph file contents against the
object database, create parse_commit_internal() to allow side-stepping
the commit-graph file and parse directly from the object database.
Due to the use of generation numbers, this method should not be called
unless the intention is explicit in avoiding commits from the
commit-graph file.
Signed-off-by: Derrick Stolee
---
commit.c | 9 +++--
commit.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 1d28677dfb..6eaed0174c 100644
--- a/commit.c
+++ b/commit.c
@@ -392,7 +392,7 @@ int parse_commit_buffer(struct commit *item, const void
*buffer, unsigned long s
return 0;
}
-int parse_commit_gently(struct commit *item, int quiet_on_missing)
+int parse_commit_internal(struct commit *item, int quiet_on_missing, int
use_commit_graph)
{
enum object_type type;
void *buffer;
@@ -403,7 +403,7 @@ int parse_commit_gently(struct commit *item, int
quiet_on_missing)
return -1;
if (item->object.parsed)
return 0;
- if (parse_commit_in_graph(item))
+ if (use_commit_graph && parse_commit_in_graph(item))
return 0;
buffer = read_sha1_file(item->object.oid.hash, , );
if (!buffer)
@@ -424,6 +424,11 @@ int parse_commit_gently(struct commit *item, int
quiet_on_missing)
return ret;
}
+int parse_commit_gently(struct commit *item, int quiet_on_missing)
+{
+ return parse_commit_internal(item, quiet_on_missing, 1);
+}
+
void parse_commit_or_die(struct commit *item)
{
if (parse_commit(item))
diff --git a/commit.h b/commit.h
index b5afde1ae9..5fde74fcd7 100644
--- a/commit.h
+++ b/commit.h
@@ -73,6 +73,7 @@ struct commit *lookup_commit_reference_by_name(const char
*name);
struct commit *lookup_commit_or_die(const struct object_id *oid, const char
*ref_name);
int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long
size, int check_graph);
+int parse_commit_internal(struct commit *item, int quiet_on_missing, int
use_commit_graph);
int parse_commit_gently(struct commit *item, int quiet_on_missing);
static inline int parse_commit(struct commit *item)
{
--
2.18.0.rc1
[PATCH v4 15/21] commit-graph: test for corrupted octopus edge
The commit-graph file has an extra chunk to store the parent int-ids for parents beyond the first parent for octopus merges. Our test repo has a single octopus merge that we can manipulate to demonstrate the 'verify' subcommand detects incorrect values in that chunk. Signed-off-by: Derrick Stolee --- t/t5318-commit-graph.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 6a873bfda8..cf67fb391a 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -248,6 +248,7 @@ test_expect_success 'git commit-graph verify' ' ' NUM_COMMITS=9 +NUM_OCTOPUS_EDGES=2 HASH_LEN=20 GRAPH_BYTE_VERSION=4 GRAPH_BYTE_HASH=5 @@ -274,6 +275,10 @@ GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4)) GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 3)) GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 11)) GRAPH_BYTE_COMMIT_DATE=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 12)) +GRAPH_COMMIT_DATA_WIDTH=$(($HASH_LEN + 16)) +GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \ +$GRAPH_COMMIT_DATA_WIDTH \* $NUM_COMMITS)) +GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4)) # usage: corrupt_graph_and_verify # Manipulates the commit-graph file at the position @@ -383,4 +388,9 @@ test_expect_success 'detect incorrect commit date' ' "commit date" ' +test_expect_success 'detect incorrect parent for octopus merge' ' + corrupt_graph_and_verify $GRAPH_BYTE_OCTOPUS "\01" \ + "invalid parent" +' + test_done -- 2.18.0.rc1
[PATCH v4 14/21] commit-graph: verify commit date
Signed-off-by: Derrick Stolee
---
commit-graph.c | 6 ++
t/t5318-commit-graph.sh | 6 ++
2 files changed, 12 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 5faecae2a7..47fdd62e88 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -982,6 +982,12 @@ int verify_commit_graph(struct commit_graph *g)
oid_to_hex(_oid),
graph_commit->generation,
max_generation + 1);
+
+ if (graph_commit->date != odb_commit->date)
+ graph_report("commit date for commit %s in commit-graph
is %"PRItime" != %"PRItime,
+oid_to_hex(_oid),
+graph_commit->date,
+odb_commit->date);
}
return verify_commit_graph_error;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a6ea1341dc..6a873bfda8 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -273,6 +273,7 @@ GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET +
$HASH_LEN))
GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4))
GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 3))
GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 11))
+GRAPH_BYTE_COMMIT_DATE=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 12))
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -377,4 +378,9 @@ test_expect_success 'detect incorrect generation number' '
"non-zero generation number"
'
+test_expect_success 'detect incorrect commit date' '
+ corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATE "\01" \
+ "commit date"
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 16/21] commit-graph: verify contents match checksum
The commit-graph file ends with a SHA1 hash of the previous contents. If
a commit-graph file has errors but the checksum hash is correct, then we
know that the problem is a bug in Git and not simply file corruption
after-the-fact.
Compute the checksum right away so it is the first error that appears,
and make the message translatable since this error can be "corrected" by
a user by simply deleting the file and recomputing. The rest of the
errors are useful only to developers.
Be sure to continue checking the rest of the file data if the checksum
is wrong. This is important for our tests, as we break the checksum as
we modify bytes of the commit-graph file.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 16 ++--
t/t5318-commit-graph.sh | 6 ++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 47fdd62e88..05879de26c 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -829,6 +829,7 @@ void write_commit_graph(const char *obj_dir,
oids.nr = 0;
}
+#define VERIFY_COMMIT_GRAPH_ERROR_HASH 2
static int verify_commit_graph_error;
static void graph_report(const char *fmt, ...)
@@ -848,8 +849,10 @@ static void graph_report(const char *fmt, ...)
int verify_commit_graph(struct commit_graph *g)
{
uint32_t i, cur_fanout_pos = 0;
- struct object_id prev_oid, cur_oid;
+ struct object_id prev_oid, cur_oid, checksum;
int generation_zero = 0;
+ struct hashfile *f;
+ int devnull;
if (!g) {
graph_report("no commit-graph file loaded");
@@ -868,6 +871,15 @@ int verify_commit_graph(struct commit_graph *g)
if (verify_commit_graph_error)
return verify_commit_graph_error;
+ devnull = open("/dev/null", O_WRONLY);
+ f = hashfd(devnull, NULL);
+ hashwrite(f, g->data, g->data_len - g->hash_len);
+ finalize_hashfile(f, checksum.hash, CSUM_CLOSE);
+ if (hashcmp(checksum.hash, g->data + g->data_len - g->hash_len)) {
+ graph_report(_("the commit-graph file has incorrect checksum
and is likely corrupt"));
+ verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH;
+ }
+
for (i = 0; i < g->num_commits; i++) {
struct commit *graph_commit;
@@ -905,7 +917,7 @@ int verify_commit_graph(struct commit_graph *g)
cur_fanout_pos++;
}
- if (verify_commit_graph_error)
+ if (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)
return verify_commit_graph_error;
for (i = 0; i < g->num_commits; i++) {
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index cf67fb391a..2297a44e7d 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -279,6 +279,7 @@ GRAPH_COMMIT_DATA_WIDTH=$(($HASH_LEN + 16))
GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \
$GRAPH_COMMIT_DATA_WIDTH \* $NUM_COMMITS))
GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
+GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 \* $NUM_OCTOPUS_EDGES))
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -393,4 +394,9 @@ test_expect_success 'detect incorrect parent for octopus
merge' '
"invalid parent"
'
+test_expect_success 'detect invalid checksum hash' '
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum"
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 17/21] fsck: verify commit-graph
If core.commitGraph is true, verify the contents of the commit-graph
during 'git fsck' using the 'git commit-graph verify' subcommand. Run
this check on all alternates, as well.
We use a new process for two reasons:
1. The subcommand decouples the details of loading and verifying a
commit-graph file from the other fsck details.
2. The commit-graph verification requires the commits to be loaded
in a specific order to guarantee we parse from the commit-graph
file for some objects and from the object database for others.
Signed-off-by: Derrick Stolee
---
Documentation/git-fsck.txt | 3 +++
builtin/fsck.c | 21 +
t/t5318-commit-graph.sh| 8
3 files changed, 32 insertions(+)
diff --git a/Documentation/git-fsck.txt b/Documentation/git-fsck.txt
index b9f060e3b2..ab9a93fb9b 100644
--- a/Documentation/git-fsck.txt
+++ b/Documentation/git-fsck.txt
@@ -110,6 +110,9 @@ Any corrupt objects you will have to find in backups or
other archives
(i.e., you can just remove them and do an 'rsync' with some other site in
the hopes that somebody else has the object you have corrupted).
+If core.commitGraph is true, the commit-graph file will also be inspected
+using 'git commit-graph verify'. See linkgit:git-commit-graph[1].
+
Extracted Diagnostics
-
diff --git a/builtin/fsck.c b/builtin/fsck.c
index ef78c6c00c..a6d5045b77 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -16,6 +16,7 @@
#include "streaming.h"
#include "decorate.h"
#include "packfile.h"
+#include "run-command.h"
#define REACHABLE 0x0001
#define SEEN 0x0002
@@ -45,6 +46,7 @@ static int name_objects;
#define ERROR_REACHABLE 02
#define ERROR_PACK 04
#define ERROR_REFS 010
+#define ERROR_COMMIT_GRAPH 020
static const char *describe_object(struct object *obj)
{
@@ -815,5 +817,24 @@ int cmd_fsck(int argc, const char **argv, const char
*prefix)
}
check_connectivity();
+
+ if (core_commit_graph) {
+ struct child_process commit_graph_verify = CHILD_PROCESS_INIT;
+ const char *verify_argv[] = { "commit-graph", "verify", NULL,
NULL, NULL, NULL };
+ commit_graph_verify.argv = verify_argv;
+ commit_graph_verify.git_cmd = 1;
+
+ if (run_command(_graph_verify))
+ errors_found |= ERROR_COMMIT_GRAPH;
+
+ prepare_alt_odb();
+ for (alt = alt_odb_list; alt; alt = alt->next) {
+ verify_argv[2] = "--object-dir";
+ verify_argv[3] = alt->path;
+ if (run_command(_graph_verify))
+ errors_found |= ERROR_COMMIT_GRAPH;
+ }
+ }
+
return errors_found;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 2297a44e7d..44d4c71f0b 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -399,4 +399,12 @@ test_expect_success 'detect invalid checksum hash' '
"incorrect checksum"
'
+test_expect_success 'git fsck (checks commit-graph)' '
+ cd "$TRASH_DIRECTORY/full" &&
+ git fsck &&
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum" &&
+ test_must_fail git fsck
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 21/21] commit-graph: update design document
The commit-graph feature is now integrated with 'fsck' and 'gc', so remove those items from the "Future Work" section of the commit-graph design document. Also remove the section on lazy-loading trees, as that was completed in an earlier patch series. Signed-off-by: Derrick Stolee --- Documentation/technical/commit-graph.txt | 22 -- 1 file changed, 22 deletions(-) diff --git a/Documentation/technical/commit-graph.txt b/Documentation/technical/commit-graph.txt index e1a883eb46..c664acbd76 100644 --- a/Documentation/technical/commit-graph.txt +++ b/Documentation/technical/commit-graph.txt @@ -118,9 +118,6 @@ Future Work - The commit graph feature currently does not honor commit grafts. This can be remedied by duplicating or refactoring the current graft logic. -- The 'commit-graph' subcommand does not have a "verify" mode that is - necessary for integration with fsck. - - After computing and storing generation numbers, we must make graph walks aware of generation numbers to gain the performance benefits they enable. This will mostly be accomplished by swapping a commit-date-ordered @@ -130,25 +127,6 @@ Future Work - 'log --topo-order' - 'tag --merged' -- Currently, parse_commit_gently() requires filling in the root tree - object for a commit. This passes through lookup_tree() and consequently - lookup_object(). Also, it calls lookup_commit() when loading the parents. - These method calls check the ODB for object existence, even if the - consumer does not need the content. For example, we do not need the - tree contents when computing merge bases. Now that commit parsing is - removed from the computation time, these lookup operations are the - slowest operations keeping graph walks from being fast. Consider - loading these objects without verifying their existence in the ODB and - only loading them fully when consumers need them. Consider a method - such as "ensure_tree_loaded(commit)" that fully loads a tree before - using commit->tree. - -- The current design uses the 'commit-graph' subcommand to generate the graph. - When this feature stabilizes enough to recommend to most users, we should - add automatic graph writes to common operations that create many commits. - For example, one could compute a graph on 'clone', 'fetch', or 'repack' - commands. - - A server could provide a commit graph file as part of the network protocol to avoid extra calculations by clients. This feature is only of benefit if the user is willing to trust the file, because verifying the file is correct -- 2.18.0.rc1
[PATCH v4 18/21] commit-graph: use string-list API for input
Signed-off-by: Derrick Stolee
---
builtin/commit-graph.c | 39 +--
commit-graph.c | 15 +++
commit-graph.h | 7 +++
3 files changed, 23 insertions(+), 38 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3079cde6f9..d8eb8278b3 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -118,13 +118,9 @@ static int graph_read(int argc, const char **argv)
static int graph_write(int argc, const char **argv)
{
- const char **pack_indexes = NULL;
- int packs_nr = 0;
- const char **commit_hex = NULL;
- int commits_nr = 0;
- const char **lines = NULL;
- int lines_nr = 0;
- int lines_alloc = 0;
+ struct string_list *pack_indexes = NULL;
+ struct string_list *commit_hex = NULL;
+ struct string_list lines;
static struct option builtin_commit_graph_write_options[] = {
OPT_STRING(0, "object-dir", _dir,
@@ -150,32 +146,23 @@ static int graph_write(int argc, const char **argv)
if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
- lines_nr = 0;
- lines_alloc = 128;
- ALLOC_ARRAY(lines, lines_alloc);
-
- while (strbuf_getline(, stdin) != EOF) {
- ALLOC_GROW(lines, lines_nr + 1, lines_alloc);
- lines[lines_nr++] = strbuf_detach(, NULL);
- }
-
- if (opts.stdin_packs) {
- pack_indexes = lines;
- packs_nr = lines_nr;
- }
- if (opts.stdin_commits) {
- commit_hex = lines;
- commits_nr = lines_nr;
- }
+ string_list_init(, 0);
+
+ while (strbuf_getline(, stdin) != EOF)
+ string_list_append(, strbuf_detach(, NULL));
+
+ if (opts.stdin_packs)
+ pack_indexes =
+ if (opts.stdin_commits)
+ commit_hex =
}
write_commit_graph(opts.obj_dir,
pack_indexes,
- packs_nr,
commit_hex,
- commits_nr,
opts.append);
+ string_list_clear(, 0);
return 0;
}
diff --git a/commit-graph.c b/commit-graph.c
index 05879de26c..c6070735c2 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -653,10 +653,8 @@ static void compute_generation_numbers(struct
packed_commit_list* commits)
}
void write_commit_graph(const char *obj_dir,
- const char **pack_indexes,
- int nr_packs,
- const char **commit_hex,
- int nr_commits,
+ struct string_list *pack_indexes,
+ struct string_list *commit_hex,
int append)
{
struct packed_oid_list oids;
@@ -697,10 +695,10 @@ void write_commit_graph(const char *obj_dir,
int dirlen;
strbuf_addf(, "%s/pack/", obj_dir);
dirlen = packname.len;
- for (i = 0; i < nr_packs; i++) {
+ for (i = 0; i < pack_indexes->nr; i++) {
struct packed_git *p;
strbuf_setlen(, dirlen);
- strbuf_addstr(, pack_indexes[i]);
+ strbuf_addstr(, pack_indexes->items[i].string);
p = add_packed_git(packname.buf, packname.len, 1);
if (!p)
die("error adding pack %s", packname.buf);
@@ -713,12 +711,13 @@ void write_commit_graph(const char *obj_dir,
}
if (commit_hex) {
- for (i = 0; i < nr_commits; i++) {
+ for (i = 0; i < commit_hex->nr; i++) {
const char *end;
struct object_id oid;
struct commit *result;
- if (commit_hex[i] && parse_oid_hex(commit_hex[i], ,
))
+ if (commit_hex->items[i].string &&
+ parse_oid_hex(commit_hex->items[i].string, ,
))
continue;
result = lookup_commit_reference_gently(, 1);
diff --git a/commit-graph.h b/commit-graph.h
index 71a39c5a57..1e1fc5 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -2,6 +2,7 @@
#define COMMIT_GRAPH_H
#include "git-compat-util.h"
+#include "string-list.h"
char *get_commit_graph_filename(const char *obj_dir);
@@ -47,10 +48,8 @@ struct commit_graph {
struct commit_graph *load_commit_graph_one(const char *graph_file);
void write_commit_graph(const char *obj_dir,
- const char **pack_indexes,
- int nr_packs,
-
[PATCH v4 13/21] commit-graph: verify generation number
While iterating through the commit parents, perform the generation
number calculation and compare against the value stored in the
commit-graph.
The tests demonstrate that having a different set of parents affects
the generation number calculation, and this value propagates to
descendants. Hence, we drop the single-line condition on the output.
Since Git will ship with the commit-graph feature without generation
numbers, we need to accept commit-graphs with all generation numbers
equal to zero. In this case, ignore the generation number calculation.
However, verify that we should never have a mix of zero and non-zero
generation numbers. Create a test that sets one commit to generation
zero and all following commits report a failure as they have non-zero
generation in a file that contains generation number zero.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 34 ++
t/t5318-commit-graph.sh | 11 +++
2 files changed, 45 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index d2a86e329e..5faecae2a7 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -842,10 +842,14 @@ static void graph_report(const char *fmt, ...)
va_end(ap);
}
+#define GENERATION_ZERO_EXISTS 1
+#define GENERATION_NUMBER_EXISTS 2
+
int verify_commit_graph(struct commit_graph *g)
{
uint32_t i, cur_fanout_pos = 0;
struct object_id prev_oid, cur_oid;
+ int generation_zero = 0;
if (!g) {
graph_report("no commit-graph file loaded");
@@ -907,6 +911,7 @@ int verify_commit_graph(struct commit_graph *g)
for (i = 0; i < g->num_commits; i++) {
struct commit *graph_commit, *odb_commit;
struct commit_list *graph_parents, *odb_parents;
+ uint32_t max_generation = 0;
hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
@@ -941,6 +946,9 @@ int verify_commit_graph(struct commit_graph *g)
oid_to_hex(_parents->item->object.oid),
oid_to_hex(_parents->item->object.oid));
+ if (graph_parents->item->generation > max_generation)
+ max_generation =
graph_parents->item->generation;
+
graph_parents = graph_parents->next;
odb_parents = odb_parents->next;
}
@@ -948,6 +956,32 @@ int verify_commit_graph(struct commit_graph *g)
if (odb_parents != NULL)
graph_report("commit-graph parent list for commit %s
terminates early",
oid_to_hex(_oid));
+
+ if (!graph_commit->generation) {
+ if (generation_zero == GENERATION_NUMBER_EXISTS)
+ graph_report("commit-graph has generation
number zero for commit %s, but non-zero elsewhere",
+oid_to_hex(_oid));
+ generation_zero = GENERATION_ZERO_EXISTS;
+ } else if (generation_zero == GENERATION_ZERO_EXISTS)
+ graph_report("commit-graph has non-zero generation
number for commit %s, but zero elsewhere",
+oid_to_hex(_oid));
+
+ if (generation_zero == GENERATION_ZERO_EXISTS)
+ continue;
+
+ /*
+* If one of our parents has generation GENERATION_NUMBER_MAX,
then
+* our generation is also GENERATION_NUMBER_MAX. Decrement to
avoid
+* extra logic in the following condition.
+*/
+ if (max_generation == GENERATION_NUMBER_MAX)
+ max_generation--;
+
+ if (graph_commit->generation != max_generation + 1)
+ graph_report("commit-graph generation for commit %s is
%u != %u",
+oid_to_hex(_oid),
+graph_commit->generation,
+max_generation + 1);
}
return verify_commit_graph_error;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ec0964112a..a6ea1341dc 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -272,6 +272,7 @@ GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET
GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN))
GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4))
GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 3))
+GRAPH_BYTE_COMMIT_GENERATION=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 11))
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -366,4 +367,14 @@ test_expect_success 'detect wrong parent' '
"commit-graph parent for"
'
+test_expect_success 'detect incorrect generation number'
[PATCH v4 20/21] gc: automatically write commit-graph files
The commit-graph file is a very helpful feature for speeding up git
operations. In order to make it more useful, make it possible to
write the commit-graph file during standard garbage collection
operations.
Add a 'gc.commitGraph' config setting that triggers writing a
commit-graph file after any non-trivial 'git gc' command. Defaults to
false while the commit-graph feature matures. We specifically do not
want to have this on by default until the commit-graph feature is fully
integrated with history-modifying features like shallow clones.
Helped-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Derrick Stolee
---
Documentation/config.txt | 10 +-
Documentation/git-gc.txt | 4
builtin/gc.c | 6 ++
t/t5318-commit-graph.sh | 14 ++
4 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 11f027194e..d2eb3c8e9b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -900,7 +900,8 @@ the `GIT_NOTES_REF` environment variable. See
linkgit:git-notes[1].
core.commitGraph::
Enable git commit graph feature. Allows reading from the
- commit-graph file.
+ commit-graph file. See `gc.commitGraph` for automatically
+ maintaining the file.
core.sparseCheckout::
Enable "sparse checkout" feature. See section "Sparse checkout" in
@@ -1553,6 +1554,13 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
+gc.commitGraph::
+ If true, then gc will rewrite the commit-graph file when
+ linkgit:git-gc[1] is run. When using linkgit:git-gc[1]
+ '--auto' the commit-graph will be updated if housekeeping is
+ required. Default is false. See linkgit:git-commit-graph[1]
+ for details.
+
gc.logExpiry::
If the file gc.log exists, then `git gc --auto` won't run
unless that file is more than 'gc.logExpiry' old. Default is
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 571b5a7e3c..a6526b3592 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -119,6 +119,10 @@ The optional configuration variable `gc.packRefs`
determines if
it within all non-bare repos or it can be set to a boolean value.
This defaults to true.
+The optional configuration variable `gc.commitGraph` determines if
+'git gc' should run 'git commit-graph write'. This can be set to a
+boolean value. This defaults to false.
+
The optional configuration variable `gc.aggressiveWindow` controls how
much time is spent optimizing the delta compression of the objects in
the repository when the --aggressive option is specified. The larger
diff --git a/builtin/gc.c b/builtin/gc.c
index 77fa720bd0..efd214a59f 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -20,6 +20,7 @@
#include "argv-array.h"
#include "commit.h"
#include "packfile.h"
+#include "commit-graph.h"
#define FAILED_RUN "failed to run %s"
@@ -34,6 +35,7 @@ static int aggressive_depth = 50;
static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
static int gc_auto_pack_limit = 50;
+static int gc_commit_graph = 0;
static int detach_auto = 1;
static timestamp_t gc_log_expire_time;
static const char *gc_log_expire = "1.day.ago";
@@ -121,6 +123,7 @@ static void gc_config(void)
git_config_get_int("gc.aggressivedepth", _depth);
git_config_get_int("gc.auto", _auto_threshold);
git_config_get_int("gc.autopacklimit", _auto_pack_limit);
+ git_config_get_bool("gc.commitgraph", _commit_graph);
git_config_get_bool("gc.autodetach", _auto);
git_config_get_expiry("gc.pruneexpire", _expire);
git_config_get_expiry("gc.worktreepruneexpire",
_worktrees_expire);
@@ -480,6 +483,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
if (pack_garbage.nr > 0)
clean_pack_garbage();
+ if (gc_commit_graph)
+ write_commit_graph_reachable(get_object_directory(), 0);
+
if (auto_gc && too_many_loose_objects())
warning(_("There are too many unreachable loose objects; "
"run 'git prune' to remove them."));
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index ffb2ed7c95..b24e8b6689 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -245,6 +245,20 @@ test_expect_success 'perform fast-forward merge in full
repo' '
test_cmp expect output
'
+test_expect_success 'check that gc computes commit-graph' '
+ cd "$TRASH_DIRECTORY/full" &&
+ git commit --allow-empty -m "blank" &&
+ git commit-graph write --reachable &&
+ cp $objdir/info/commit-graph commit-graph-before-gc &&
+ git reset --hard HEAD~1 &&
+ git config gc.commitGraph true &&
+ git gc &&
+ cp $objdir/info/commit-graph commit-graph-after-gc &&
+ ! test_cmp commit-graph-before-gc
[PATCH v4 19/21] commit-graph: add '--reachable' option
When writing commit-graph files, it can be convenient to ask for all
reachable commits (starting at the ref set) in the resulting file. This
is particularly helpful when writing to stdin is complicated, such as a
future integration with 'git gc'.
Signed-off-by: Derrick Stolee
---
Documentation/git-commit-graph.txt | 8 ++--
builtin/commit-graph.c | 16
commit-graph.c | 18 ++
commit-graph.h | 1 +
t/t5318-commit-graph.sh| 10 ++
5 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-commit-graph.txt
b/Documentation/git-commit-graph.txt
index a222cfab08..dececb79d7 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -38,12 +38,16 @@ Write a commit graph file based on the commits found in
packfiles.
+
With the `--stdin-packs` option, generate the new commit graph by
walking objects only in the specified pack-indexes. (Cannot be combined
-with --stdin-commits.)
+with `--stdin-commits` or `--reachable`.)
+
With the `--stdin-commits` option, generate the new commit graph by
walking commits starting at the commits specified in stdin as a list
of OIDs in hex, one OID per line. (Cannot be combined with
---stdin-packs.)
+`--stdin-packs` or `--reachable`.)
++
+With the `--reachable` option, generate the new commit graph by walking
+commits starting at all refs. (Cannot be combined with `--stdin-commits`
+or `--stdin-packs`.)
+
With the `--append` option, include all commits that are present in the
existing commit-graph file.
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index d8eb8278b3..76423b3fa5 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -9,7 +9,7 @@ static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--object-dir ]"),
N_("git commit-graph read [--object-dir ]"),
N_("git commit-graph verify [--object-dir ]"),
- N_("git commit-graph write [--object-dir ] [--append]
[--stdin-packs|--stdin-commits]"),
+ N_("git commit-graph write [--object-dir ] [--append]
[--reachable|--stdin-packs|--stdin-commits]"),
NULL
};
@@ -24,12 +24,13 @@ static const char * const builtin_commit_graph_read_usage[]
= {
};
static const char * const builtin_commit_graph_write_usage[] = {
- N_("git commit-graph write [--object-dir ] [--append]
[--stdin-packs|--stdin-commits]"),
+ N_("git commit-graph write [--object-dir ] [--append]
[--reachable|--stdin-packs|--stdin-commits]"),
NULL
};
static struct opts_commit_graph {
const char *obj_dir;
+ int reachable;
int stdin_packs;
int stdin_commits;
int append;
@@ -126,6 +127,8 @@ static int graph_write(int argc, const char **argv)
OPT_STRING(0, "object-dir", _dir,
N_("dir"),
N_("The object directory to store the graph")),
+ OPT_BOOL(0, "reachable", ,
+ N_("start walk at all refs")),
OPT_BOOL(0, "stdin-packs", _packs,
N_("scan pack-indexes listed by stdin for commits")),
OPT_BOOL(0, "stdin-commits", _commits,
@@ -139,11 +142,16 @@ static int graph_write(int argc, const char **argv)
builtin_commit_graph_write_options,
builtin_commit_graph_write_usage, 0);
- if (opts.stdin_packs && opts.stdin_commits)
- die(_("cannot use both --stdin-commits and --stdin-packs"));
+ if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)
+ die(_("use at most one of --reachable, --stdin-commits, or
--stdin-packs"));
if (!opts.obj_dir)
opts.obj_dir = get_object_directory();
+ if (opts.reachable) {
+ write_commit_graph_reachable(opts.obj_dir, opts.append);
+ return 0;
+ }
+
if (opts.stdin_packs || opts.stdin_commits) {
struct strbuf buf = STRBUF_INIT;
string_list_init(, 0);
diff --git a/commit-graph.c b/commit-graph.c
index c6070735c2..946bcfa98c 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -7,6 +7,7 @@
#include "packfile.h"
#include "commit.h"
#include "object.h"
+#include "refs.h"
#include "revision.h"
#include "sha1-lookup.h"
#include "commit-graph.h"
@@ -652,6 +653,23 @@ static void compute_generation_numbers(struct
packed_commit_list* commits)
}
}
+static int add_ref_to_list(const char *refname,
+ const struct object_id *oid,
+ int flags, void *cb_data)
+{
+ struct string_list *list = (struct string_list*)cb_data;
+ string_list_append(list, oid_to_hex(oid));
+ return 0;
+}
+
+void write_commit_graph_reachable(const char *obj_dir, int append)
+{
+ struct string_list list;
+
[PATCH v4 10/21] commit-graph: verify objects exist
In the 'verify' subcommand, load commits directly from the object
database to ensure they exist. Parse by skipping the commit-graph.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 17 +
t/t5318-commit-graph.sh | 7 +++
2 files changed, 24 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index a90cc2e6f4..0cf1b61d80 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -239,6 +239,7 @@ static struct commit_list **insert_parent_or_die(struct
commit_graph *g,
{
struct commit *c;
struct object_id oid;
+
hashcpy(oid.hash, g->chunk_oid_lookup + g->hash_len * pos);
c = lookup_commit();
if (!c)
@@ -890,5 +891,21 @@ int verify_commit_graph(struct commit_graph *g)
cur_fanout_pos++;
}
+ if (verify_commit_graph_error)
+ return verify_commit_graph_error;
+
+ for (i = 0; i < g->num_commits; i++) {
+ struct commit *odb_commit;
+
+ hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
+
+ odb_commit = (struct commit *)create_object(cur_oid.hash,
alloc_commit_node());
+ if (parse_commit_internal(odb_commit, 0, 0)) {
+ graph_report("failed to parse %s from object database",
+oid_to_hex(_oid));
+ continue;
+ }
+ }
+
return verify_commit_graph_error;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index c29eae47c9..cf60e48496 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -247,6 +247,7 @@ test_expect_success 'git commit-graph verify' '
git commit-graph verify >output
'
+NUM_COMMITS=9
HASH_LEN=20
GRAPH_BYTE_VERSION=4
GRAPH_BYTE_HASH=5
@@ -265,6 +266,7 @@ GRAPH_BYTE_FANOUT1=$(($GRAPH_FANOUT_OFFSET + 4 \* 4))
GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET + 4 \* 255))
GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET + 4 \* 256))
GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 8))
+GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 4 +
10))
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -334,4 +336,9 @@ test_expect_success 'detect incorrect OID order' '
"incorrect OID order"
'
+test_expect_success 'detect OID not in object database' '
+ corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_MISSING "\01" \
+ "from object database"
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 09/21] commit-graph: verify corrupt OID fanout and lookup
In the commit-graph file, the OID fanout chunk provides an index into
the OID lookup. The 'verify' subcommand should find incorrect values
in the fanout.
Similarly, the 'verify' subcommand should find out-of-order values in
the OID lookup.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 36
t/t5318-commit-graph.sh | 22 ++
2 files changed, 58 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 25d5edea82..a90cc2e6f4 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -840,6 +840,9 @@ static void graph_report(const char *fmt, ...)
int verify_commit_graph(struct commit_graph *g)
{
+ uint32_t i, cur_fanout_pos = 0;
+ struct object_id prev_oid, cur_oid;
+
if (!g) {
graph_report("no commit-graph file loaded");
return 1;
@@ -854,5 +857,38 @@ int verify_commit_graph(struct commit_graph *g)
if (!g->chunk_commit_data)
graph_report("commit-graph is missing the Commit Data chunk");
+ if (verify_commit_graph_error)
+ return verify_commit_graph_error;
+
+ for (i = 0; i < g->num_commits; i++) {
+ hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
+
+ if (i && oidcmp(_oid, _oid) >= 0)
+ graph_report("commit-graph has incorrect OID order: %s
then %s",
+oid_to_hex(_oid),
+oid_to_hex(_oid));
+
+ oidcpy(_oid, _oid);
+
+ while (cur_oid.hash[0] > cur_fanout_pos) {
+ uint32_t fanout_value = get_be32(g->chunk_oid_fanout +
cur_fanout_pos);
+ if (i != fanout_value)
+ graph_report("commit-graph has incorrect fanout
value: fanout[%d] = %u != %u",
+cur_fanout_pos, fanout_value, i);
+
+ cur_fanout_pos++;
+ }
+ }
+
+ while (cur_fanout_pos < 256) {
+ uint32_t fanout_value = get_be32(g->chunk_oid_fanout +
cur_fanout_pos);
+
+ if (g->num_commits != fanout_value)
+ graph_report("commit-graph has incorrect fanout value:
fanout[%d] = %u != %u",
+cur_fanout_pos, fanout_value, i);
+
+ cur_fanout_pos++;
+ }
+
return verify_commit_graph_error;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 846396665e..c29eae47c9 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -247,6 +247,7 @@ test_expect_success 'git commit-graph verify' '
git commit-graph verify >output
'
+HASH_LEN=20
GRAPH_BYTE_VERSION=4
GRAPH_BYTE_HASH=5
GRAPH_BYTE_CHUNK_COUNT=6
@@ -258,6 +259,12 @@ GRAPH_BYTE_OID_LOOKUP_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET + \
1 \* $GRAPH_CHUNK_LOOKUP_WIDTH))
GRAPH_BYTE_COMMIT_DATA_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET + \
2 \* $GRAPH_CHUNK_LOOKUP_WIDTH))
+GRAPH_FANOUT_OFFSET=$(($GRAPH_CHUNK_LOOKUP_OFFSET + \
+ $GRAPH_CHUNK_LOOKUP_WIDTH \* $GRAPH_CHUNK_LOOKUP_ROWS))
+GRAPH_BYTE_FANOUT1=$(($GRAPH_FANOUT_OFFSET + 4 \* 4))
+GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET + 4 \* 255))
+GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET + 4 \* 256))
+GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 8))
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -312,4 +319,19 @@ test_expect_success 'detect missing commit data chunk' '
"missing the Commit Data chunk"
'
+test_expect_success 'detect incorrect fanout' '
+ corrupt_graph_and_verify $GRAPH_BYTE_FANOUT1 "\01" \
+ "fanout value"
+'
+
+test_expect_success 'detect incorrect fanout final value' '
+ corrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 "\01" \
+ "fanout value"
+'
+
+test_expect_success 'detect incorrect OID order' '
+ corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ORDER "\01" \
+ "incorrect OID order"
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 12/21] commit-graph: verify parent list
The commit-graph file stores parents in a two-column portion of the
commit data chunk. If there is only one parent, then the second column
stores 0x to indicate no second parent.
The 'verify' subcommand checks the parent list for the commit loaded
from the commit-graph and the one parsed from the object database. Test
these checks for corrupt parents, too many parents, and wrong parents.
Add a boundary check to insert_parent_or_die() for when the parent
position value is out of range.
The octopus merge will be tested in a later commit.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 28
t/t5318-commit-graph.sh | 18 ++
2 files changed, 46 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 50a8e27910..d2a86e329e 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -240,6 +240,9 @@ static struct commit_list **insert_parent_or_die(struct
commit_graph *g,
struct commit *c;
struct object_id oid;
+ if (pos >= g->num_commits)
+ die("invalid parent position %"PRIu64, pos);
+
hashcpy(oid.hash, g->chunk_oid_lookup + g->hash_len * pos);
c = lookup_commit();
if (!c)
@@ -903,6 +906,7 @@ int verify_commit_graph(struct commit_graph *g)
for (i = 0; i < g->num_commits; i++) {
struct commit *graph_commit, *odb_commit;
+ struct commit_list *graph_parents, *odb_parents;
hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
@@ -920,6 +924,30 @@ int verify_commit_graph(struct commit_graph *g)
oid_to_hex(_oid),
oid_to_hex(get_commit_tree_oid(graph_commit)),
oid_to_hex(get_commit_tree_oid(odb_commit)));
+
+ graph_parents = graph_commit->parents;
+ odb_parents = odb_commit->parents;
+
+ while (graph_parents) {
+ if (odb_parents == NULL) {
+ graph_report("commit-graph parent list for
commit %s is too long",
+oid_to_hex(_oid));
+ break;
+ }
+
+ if (oidcmp(_parents->item->object.oid,
_parents->item->object.oid))
+ graph_report("commit-graph parent for %s is %s
!= %s",
+oid_to_hex(_oid),
+
oid_to_hex(_parents->item->object.oid),
+
oid_to_hex(_parents->item->object.oid));
+
+ graph_parents = graph_parents->next;
+ odb_parents = odb_parents->next;
+ }
+
+ if (odb_parents != NULL)
+ graph_report("commit-graph parent list for commit %s
terminates early",
+oid_to_hex(_oid));
}
return verify_commit_graph_error;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index c0c1248eda..ec0964112a 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -269,6 +269,9 @@ GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET +
$HASH_LEN \* 8))
GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 4 +
10))
GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \*
$NUM_COMMITS))
GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET
+GRAPH_BYTE_COMMIT_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN))
+GRAPH_BYTE_COMMIT_EXTRA_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4))
+GRAPH_BYTE_COMMIT_WRONG_PARENT=$(($GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 3))
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -348,4 +351,19 @@ test_expect_success 'detect incorrect tree OID' '
"root tree OID for commit"
'
+test_expect_success 'detect incorrect parent int-id' '
+ corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_PARENT "\01" \
+ "invalid parent"
+'
+
+test_expect_success 'detect extra parent int-id' '
+ corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_EXTRA_PARENT "\00" \
+ "is too long"
+'
+
+test_expect_success 'detect wrong parent' '
+ corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_WRONG_PARENT "\01" \
+ "commit-graph parent for"
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 11/21] commit-graph: verify root tree OIDs
The 'verify' subcommand must compare the commit content parsed from the
commit-graph against the content in the object database. Use
lookup_commit() and parse_commit_in_graph_one() to parse the commits
from the graph and compare against a commit that is loaded separately
and parsed directly from the object database.
Add checks for the root tree OID.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 17 -
t/t5318-commit-graph.sh | 7 +++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/commit-graph.c b/commit-graph.c
index 0cf1b61d80..50a8e27910 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -862,6 +862,8 @@ int verify_commit_graph(struct commit_graph *g)
return verify_commit_graph_error;
for (i = 0; i < g->num_commits; i++) {
+ struct commit *graph_commit;
+
hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
if (i && oidcmp(_oid, _oid) >= 0)
@@ -879,6 +881,11 @@ int verify_commit_graph(struct commit_graph *g)
cur_fanout_pos++;
}
+
+ graph_commit = lookup_commit(_oid);
+ if (!parse_commit_in_graph_one(g, graph_commit))
+ graph_report("failed to parse %s from commit-graph",
+oid_to_hex(_oid));
}
while (cur_fanout_pos < 256) {
@@ -895,16 +902,24 @@ int verify_commit_graph(struct commit_graph *g)
return verify_commit_graph_error;
for (i = 0; i < g->num_commits; i++) {
- struct commit *odb_commit;
+ struct commit *graph_commit, *odb_commit;
hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
+ graph_commit = lookup_commit(_oid);
odb_commit = (struct commit *)create_object(cur_oid.hash,
alloc_commit_node());
if (parse_commit_internal(odb_commit, 0, 0)) {
graph_report("failed to parse %s from object database",
oid_to_hex(_oid));
continue;
}
+
+ if (oidcmp(_commit_tree_in_graph_one(g,
graph_commit)->object.oid,
+ get_commit_tree_oid(odb_commit)))
+ graph_report("root tree OID for commit %s in
commit-graph is %s != %s",
+oid_to_hex(_oid),
+
oid_to_hex(get_commit_tree_oid(graph_commit)),
+
oid_to_hex(get_commit_tree_oid(odb_commit)));
}
return verify_commit_graph_error;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index cf60e48496..c0c1248eda 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -267,6 +267,8 @@ GRAPH_BYTE_FANOUT2=$(($GRAPH_FANOUT_OFFSET + 4 \* 255))
GRAPH_OID_LOOKUP_OFFSET=$(($GRAPH_FANOUT_OFFSET + 4 \* 256))
GRAPH_BYTE_OID_LOOKUP_ORDER=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 8))
GRAPH_BYTE_OID_LOOKUP_MISSING=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \* 4 +
10))
+GRAPH_COMMIT_DATA_OFFSET=$(($GRAPH_OID_LOOKUP_OFFSET + $HASH_LEN \*
$NUM_COMMITS))
+GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -341,4 +343,9 @@ test_expect_success 'detect OID not in object database' '
"from object database"
'
+test_expect_success 'detect incorrect tree OID' '
+ corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_TREE "\01" \
+ "root tree OID for commit"
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 03/21] commit-graph: parse commit from chosen graph
Before verifying a commit-graph file against the object database, we
need to parse all commits from the given commit-graph file. Create
parse_commit_in_graph_one() to target a given struct commit_graph.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index c09e87c3c2..472b41ec30 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -311,7 +311,7 @@ static int find_commit_in_graph(struct commit *item, struct
commit_graph *g, uin
}
}
-int parse_commit_in_graph(struct commit *item)
+static int parse_commit_in_graph_one(struct commit_graph *g, struct commit
*item)
{
uint32_t pos;
@@ -319,9 +319,21 @@ int parse_commit_in_graph(struct commit *item)
return 0;
if (item->object.parsed)
return 1;
+
+ if (find_commit_in_graph(item, g, ))
+ return fill_commit_in_graph(item, g, pos);
+
+ return 0;
+}
+
+int parse_commit_in_graph(struct commit *item)
+{
+ if (!core_commit_graph)
+ return 0;
+
prepare_commit_graph();
- if (commit_graph && find_commit_in_graph(item, commit_graph, ))
- return fill_commit_in_graph(item, commit_graph, pos);
+ if (commit_graph)
+ return parse_commit_in_graph_one(commit_graph, item);
return 0;
}
--
2.18.0.rc1
[PATCH v4 00/21] Integrate commit-graph into 'fsck' and 'gc'
Thanks for the feedback on v3. There were several small cleanups, but perhaps the biggest change is the addition of "commit-graph: use string-list API for input" which makes "commit-graph: add '--reachable' option" much simpler. The inter-diff is still reasonably large, but I'll send it in a follow-up PR. Thanks, -Stolee Derrick Stolee (21): commit-graph: UNLEAK before die() commit-graph: fix GRAPH_MIN_SIZE commit-graph: parse commit from chosen graph commit: force commit to parse from object database commit-graph: load a root tree from specific graph commit-graph: add 'verify' subcommand commit-graph: verify catches corrupt signature commit-graph: verify required chunks are present commit-graph: verify corrupt OID fanout and lookup commit-graph: verify objects exist commit-graph: verify root tree OIDs commit-graph: verify parent list commit-graph: verify generation number commit-graph: verify commit date commit-graph: test for corrupted octopus edge commit-graph: verify contents match checksum fsck: verify commit-graph commit-graph: use string-list API for input commit-graph: add '--reachable' option gc: automatically write commit-graph files commit-graph: update design document Documentation/config.txt | 10 +- Documentation/git-commit-graph.txt | 14 +- Documentation/git-fsck.txt | 3 + Documentation/git-gc.txt | 4 + Documentation/technical/commit-graph.txt | 22 -- builtin/commit-graph.c | 98 ++--- builtin/fsck.c | 21 ++ builtin/gc.c | 6 + commit-graph.c | 248 +-- commit-graph.h | 10 +- commit.c | 9 +- commit.h | 1 + t/t5318-commit-graph.sh | 201 ++ 13 files changed, 569 insertions(+), 78 deletions(-) -- 2.18.0.rc1
[PATCH v4 06/21] commit-graph: add 'verify' subcommand
If the commit-graph file becomes corrupt, we need a way to verify
that its contents match the object database. In the manner of
'git fsck' we will implement a 'git commit-graph verify' subcommand
to report all issues with the file.
Add the 'verify' subcommand to the 'commit-graph' builtin and its
documentation. The subcommand is currently a no-op except for
loading the commit-graph into memory, which may trigger run-time
errors that would be caught by normal use. Add a simple test that
ensures the command returns a zero error code.
If no commit-graph file exists, this is an acceptable state. Do
not report any errors.
Helped-by: Ramsay Jones
Signed-off-by: Derrick Stolee
---
Documentation/git-commit-graph.txt | 6 +
builtin/commit-graph.c | 38 ++
commit-graph.c | 23 ++
commit-graph.h | 2 ++
t/t5318-commit-graph.sh| 10
5 files changed, 79 insertions(+)
diff --git a/Documentation/git-commit-graph.txt
b/Documentation/git-commit-graph.txt
index 4c97b555cc..a222cfab08 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -10,6 +10,7 @@ SYNOPSIS
[verse]
'git commit-graph read' [--object-dir ]
+'git commit-graph verify' [--object-dir ]
'git commit-graph write' [--object-dir ]
@@ -52,6 +53,11 @@ existing commit-graph file.
Read a graph file given by the commit-graph file and output basic
details about the graph file. Used for debugging purposes.
+'verify'::
+
+Read the commit-graph file and verify its contents against the object
+database. Used to check for corrupted data.
+
EXAMPLES
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index f0875b8bf3..3079cde6f9 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -8,10 +8,16 @@
static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--object-dir ]"),
N_("git commit-graph read [--object-dir ]"),
+ N_("git commit-graph verify [--object-dir ]"),
N_("git commit-graph write [--object-dir ] [--append]
[--stdin-packs|--stdin-commits]"),
NULL
};
+static const char * const builtin_commit_graph_verify_usage[] = {
+ N_("git commit-graph verify [--object-dir ]"),
+ NULL
+};
+
static const char * const builtin_commit_graph_read_usage[] = {
N_("git commit-graph read [--object-dir ]"),
NULL
@@ -29,6 +35,36 @@ static struct opts_commit_graph {
int append;
} opts;
+
+static int graph_verify(int argc, const char **argv)
+{
+ struct commit_graph *graph = NULL;
+ char *graph_name;
+
+ static struct option builtin_commit_graph_verify_options[] = {
+ OPT_STRING(0, "object-dir", _dir,
+ N_("dir"),
+ N_("The object directory to store the graph")),
+ OPT_END(),
+ };
+
+ argc = parse_options(argc, argv, NULL,
+builtin_commit_graph_verify_options,
+builtin_commit_graph_verify_usage, 0);
+
+ if (!opts.obj_dir)
+ opts.obj_dir = get_object_directory();
+
+ graph_name = get_commit_graph_filename(opts.obj_dir);
+ graph = load_commit_graph_one(graph_name);
+ FREE_AND_NULL(graph_name);
+
+ if (!graph)
+ return 0;
+
+ return verify_commit_graph(graph);
+}
+
static int graph_read(int argc, const char **argv)
{
struct commit_graph *graph = NULL;
@@ -165,6 +201,8 @@ int cmd_commit_graph(int argc, const char **argv, const
char *prefix)
if (argc > 0) {
if (!strcmp(argv[0], "read"))
return graph_read(argc, argv);
+ if (!strcmp(argv[0], "verify"))
+ return graph_verify(argc, argv);
if (!strcmp(argv[0], "write"))
return graph_write(argc, argv);
}
diff --git a/commit-graph.c b/commit-graph.c
index fee8437ce3..c860cbe721 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -824,3 +824,26 @@ void write_commit_graph(const char *obj_dir,
oids.alloc = 0;
oids.nr = 0;
}
+
+static int verify_commit_graph_error;
+
+static void graph_report(const char *fmt, ...)
+{
+ va_list ap;
+ verify_commit_graph_error = 1;
+
+ va_start(ap, fmt);
+ vfprintf(stderr, fmt, ap);
+ fprintf(stderr, "\n");
+ va_end(ap);
+}
+
+int verify_commit_graph(struct commit_graph *g)
+{
+ if (!g) {
+ graph_report("no commit-graph file loaded");
+ return 1;
+ }
+
+ return verify_commit_graph_error;
+}
diff --git a/commit-graph.h b/commit-graph.h
index 96cccb10f3..71a39c5a57 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -53,4 +53,6 @@ void write_commit_graph(const char *obj_dir,
int nr_commits,
int
[PATCH v4 07/21] commit-graph: verify catches corrupt signature
This is the first of several commits that add a test to check that
'git commit-graph verify' catches corruption in the commit-graph
file. The first test checks that the command catches an error in
the file signature. This is a check that exists in the existing
commit-graph reading code.
Add a helper method 'corrupt_graph_and_verify' to the test script
t5318-commit-graph.sh. This helper corrupts the commit-graph file
at a certain location, runs 'git commit-graph verify', and reports
the output to the 'err' file. This data is filtered to remove the
lines added by 'test_must_fail' when the test is run verbosely.
Then, the output is checked to contain a specific error message.
Most messages from 'git commit-graph verify' will not be marked
for translation. There will be one exception: the message that
reports an invalid checksum will be marked for translation, as that
is the only message that is intended for a typical user.
Helped-by: Szeder Gábor
Signed-off-by: Derrick Stolee
---
t/t5318-commit-graph.sh | 43 +
1 file changed, 43 insertions(+)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 0830ef9fdd..c0c1ff09b9 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -235,9 +235,52 @@ test_expect_success 'perform fast-forward merge in full
repo' '
test_cmp expect output
'
+# the verify tests below expect the commit-graph to contain
+# exactly the commits reachable from the commits/8 branch.
+# If the file changes the set of commits in the list, then the
+# offsets into the binary file will result in different edits
+# and the tests will likely break.
+
test_expect_success 'git commit-graph verify' '
cd "$TRASH_DIRECTORY/full" &&
+ git rev-parse commits/8 | git commit-graph write --stdin-commits &&
git commit-graph verify >output
'
+GRAPH_BYTE_VERSION=4
+GRAPH_BYTE_HASH=5
+
+# usage: corrupt_graph_and_verify
+# Manipulates the commit-graph file at the position
+# by inserting the data, then runs 'git commit-graph verify'
+# and places the output in the file 'err'. Test 'err' for
+# the given string.
+corrupt_graph_and_verify() {
+ pos=$1
+ data="${2:-\0}"
+ grepstr=$3
+ cd "$TRASH_DIRECTORY/full" &&
+ test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+ cp $objdir/info/commit-graph commit-graph-backup &&
+ printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos"
conv=notrunc &&
+ test_must_fail git commit-graph verify 2>test_err &&
+ grep -v "^+" test_err >err
+ test_i18ngrep "$grepstr" err
+}
+
+test_expect_success 'detect bad signature' '
+ corrupt_graph_and_verify 0 "\0" \
+ "graph signature"
+'
+
+test_expect_success 'detect bad version' '
+ corrupt_graph_and_verify $GRAPH_BYTE_VERSION "\02" \
+ "graph version"
+'
+
+test_expect_success 'detect bad hash version' '
+ corrupt_graph_and_verify $GRAPH_BYTE_HASH "\02" \
+ "hash version"
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 08/21] commit-graph: verify required chunks are present
The commit-graph file requires the following three chunks:
* OID Fanout
* OID Lookup
* Commit Data
If any of these are missing, then the 'verify' subcommand should
report a failure. This includes the chunk IDs malformed or the
chunk count is truncated.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 9 +
t/t5318-commit-graph.sh | 29 +
2 files changed, 38 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index c860cbe721..25d5edea82 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -845,5 +845,14 @@ int verify_commit_graph(struct commit_graph *g)
return 1;
}
+ verify_commit_graph_error = 0;
+
+ if (!g->chunk_oid_fanout)
+ graph_report("commit-graph is missing the OID Fanout chunk");
+ if (!g->chunk_oid_lookup)
+ graph_report("commit-graph is missing the OID Lookup chunk");
+ if (!g->chunk_commit_data)
+ graph_report("commit-graph is missing the Commit Data chunk");
+
return verify_commit_graph_error;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index c0c1ff09b9..846396665e 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -249,6 +249,15 @@ test_expect_success 'git commit-graph verify' '
GRAPH_BYTE_VERSION=4
GRAPH_BYTE_HASH=5
+GRAPH_BYTE_CHUNK_COUNT=6
+GRAPH_CHUNK_LOOKUP_OFFSET=8
+GRAPH_CHUNK_LOOKUP_WIDTH=12
+GRAPH_CHUNK_LOOKUP_ROWS=5
+GRAPH_BYTE_OID_FANOUT_ID=$GRAPH_CHUNK_LOOKUP_OFFSET
+GRAPH_BYTE_OID_LOOKUP_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET + \
+ 1 \* $GRAPH_CHUNK_LOOKUP_WIDTH))
+GRAPH_BYTE_COMMIT_DATA_ID=$(($GRAPH_CHUNK_LOOKUP_OFFSET + \
+2 \* $GRAPH_CHUNK_LOOKUP_WIDTH))
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -283,4 +292,24 @@ test_expect_success 'detect bad hash version' '
"hash version"
'
+test_expect_success 'detect low chunk count' '
+ corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\02" \
+ "missing the .* chunk"
+'
+
+test_expect_success 'detect missing OID fanout chunk' '
+ corrupt_graph_and_verify $GRAPH_BYTE_OID_FANOUT_ID "\0" \
+ "missing the OID Fanout chunk"
+'
+
+test_expect_success 'detect missing OID lookup chunk' '
+ corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ID "\0" \
+ "missing the OID Lookup chunk"
+'
+
+test_expect_success 'detect missing commit data chunk' '
+ corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATA_ID "\0" \
+ "missing the Commit Data chunk"
+'
+
test_done
--
2.18.0.rc1
[PATCH v4 05/21] commit-graph: load a root tree from specific graph
When lazy-loading a tree for a commit, it will be important to select
the tree from a specific struct commit_graph. Create a new method that
specifies the commit-graph file and use that in
get_commit_tree_in_graph().
Signed-off-by: Derrick Stolee
---
commit-graph.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 472b41ec30..fee8437ce3 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -359,14 +359,20 @@ static struct tree *load_tree_for_commit(struct
commit_graph *g, struct commit *
return c->maybe_tree;
}
-struct tree *get_commit_tree_in_graph(const struct commit *c)
+static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,
+const struct commit *c)
{
if (c->maybe_tree)
return c->maybe_tree;
if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
- BUG("get_commit_tree_in_graph called from non-commit-graph
commit");
+ BUG("get_commit_tree_in_graph_one called from non-commit-graph
commit");
+
+ return load_tree_for_commit(g, (struct commit *)c);
+}
- return load_tree_for_commit(commit_graph, (struct commit *)c);
+struct tree *get_commit_tree_in_graph(const struct commit *c)
+{
+ return get_commit_tree_in_graph_one(commit_graph, c);
}
static void write_graph_chunk_fanout(struct hashfile *f,
--
2.18.0.rc1
[PATCH v4 02/21] commit-graph: fix GRAPH_MIN_SIZE
The GRAPH_MIN_SIZE macro should be the smallest size of a parsable
commit-graph file. However, the minimum number of chunks was wrong.
It is possible to write a commit-graph file with zero commits, and
that violates this macro's value.
Rewrite the macro, and use extra macros to better explain the magic
constants.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index bb54c1214c..c09e87c3c2 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -34,10 +34,11 @@
#define GRAPH_LAST_EDGE 0x8000
+#define GRAPH_HEADER_SIZE 8
#define GRAPH_FANOUT_SIZE (4 * 256)
#define GRAPH_CHUNKLOOKUP_WIDTH 12
-#define GRAPH_MIN_SIZE (5 * GRAPH_CHUNKLOOKUP_WIDTH + GRAPH_FANOUT_SIZE + \
- GRAPH_OID_LEN + 8)
+#define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
+ + GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)
char *get_commit_graph_filename(const char *obj_dir)
{
--
2.18.0.rc1
[PATCH v4 01/21] commit-graph: UNLEAK before die()
Signed-off-by: Derrick Stolee
---
builtin/commit-graph.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 37420ae0fd..f0875b8bf3 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -51,8 +51,11 @@ static int graph_read(int argc, const char **argv)
graph_name = get_commit_graph_filename(opts.obj_dir);
graph = load_commit_graph_one(graph_name);
- if (!graph)
+ if (!graph) {
+ UNLEAK(graph_name);
die("graph file %s does not exist", graph_name);
+ }
+
FREE_AND_NULL(graph_name);
printf("header: %08x %d %d %d %d\n",
--
2.18.0.rc1
Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config
Why must the credentials must be deleted after receiving the 401 (or
any) error? What's the rationale for this?
On Mon, Jun 4, 2018 at 10:47 AM, Jeff King wrote:
> On Mon, Jun 04, 2018 at 05:26:35AM -0700, lars.schnei...@autodesk.com wrote:
>
>> From: Lars Schneider
>>
>> If a Git HTTP server responds with 401 or 407, then Git tells the
>> credential helper to reject and delete the credentials. In general
>> this is good.
>>
>> However, in certain automation environments it is not desired to remove
>> credentials automatically. This is in particular the case if credentials
>> are only invalid temporarily (e.g. because of problems in the server's
>> authentication backend).
>>
>> Therefore, add the config "http.keepRejectedCredentials" which tells
>> Git to keep invalid credentials if set to "true".
>
> It seems like those servers should be returning a value besides "401" if
> it's a temporary error.
>
> But alas, we live in the real world, and your patch seems like a pretty
> sensible workaround for clients. This could be done at the helper layer,
> but I think in practice doing it here is going to be a lot more
> convenient (and doesn't preclude helpers having their own logic if
> people care to extend them in that direction).
>
>> It was considered to disable the credential deletion in credential.c
>> directly. This approach was not chosen as it could be confusing to
>> other callers of credential_reject() if the function does not do what
>> its name says (e.g. in imap-send.c).
>
> Yeah, I think "git credential" relies on that code, too, and you
> probably should be able to manually forget a credential at that plumbing
> layer.
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index ab641bf5a9..184aee8dbc 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1997,6 +1997,12 @@ http.emptyAuth::
>> a username in the URL, as libcurl normally requires a username for
>> authentication.
>>
>> +http.keepRejectedCredentials::
>> + Keep credentials in the credential helper that a Git server responded
>> + to with 401 (unauthorized) or 407 (proxy authentication required).
>> + This can be useful in automation environments where credentials might
>> + become temporarily invalid. The default is `false`.
>
> Looks good.
>
>> http.delegation::
>> Control GSSAPI credential delegation. The delegation is disabled
>> by default in libcurl since version 7.21.7. Set parameter to tell
>> diff --git a/http.c b/http.c
>> index b4bfbceaeb..ff6932813f 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -138,6 +138,7 @@ static int ssl_cert_password_required;
>> #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
>> static unsigned long http_auth_methods = CURLAUTH_ANY;
>> static int http_auth_methods_restricted;
>> +static int keep_rejected_credentials = 0;
>
> Minor nit, but we usually skip the redundant "= 0" for BSS variables.
>
>> @@ -403,6 +404,11 @@ static int http_options(const char *var, const char
>> *value, void *cb)
>> return 0;
>> }
>>
>> + if (!strcmp("http.keeprejectedcredentials", var)) {
>> + keep_rejected_credentials = git_config_bool(var, value);
>> + return 0;
>> + }
>> +
>> /* Fall back on the default ones */
>> return git_default_config(var, value, cb);
>> }
>> @@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results
>> *results)
>> return HTTP_MISSING_TARGET;
>> else if (results->http_code == 401) {
>> if (http_auth.username && http_auth.password) {
>> - credential_reject(_auth);
>> + if (!keep_rejected_credentials)
>> + credential_reject(_auth);
>
> The rest of the patch looks good.
>
> It's possible we'd eventually want a similar feature for other
> protocols, like IMAP. And that we'd in the long run prefer to have a
> single credential.keepRejected that covers them all. Or maybe not. Given
> that this is kind of a workaround, people might ultimately want
> protocol-specific options. So I'm happy to start with "http" for now and
> deal with other protocols down the road (if it's even necessary).
>
> Some scripts that use "git credential" may want to support this config
> option, too (I'm thinking of git-remote-mediawiki, which I believe
> uses it for http requests). But those can be added one by one to the
> porcelain scripts.
>
> So modulo the minor "= 0" nit, this all looks good to me.
>
> -Peff
Inquiry 04/06/2018
Hello, This is Ms Julian Smith and i am from Invictus Group Co.,LTD in United Kingdom. We are glad to know about your company from the web and we are interested in your products. Could you kindly send us your Latest catalog and price list for our trial order. Best Regards, Julian Smith Purchasing Manager
Re: Regression (?) in core.safecrlf=false messaging
On Mon, Jun 4, 2018 at 12:55 AM, Torsten Bögershausen wrote: > > Does the following patch fix your problem ? > > diff --git a/config.c b/config.c > index 6f8f1d8c11..c625aec96a 100644 > --- a/config.c > +++ b/config.c > @@ -1233,7 +1233,7 @@ static int git_default_core_config(const char *var, > const char *value) > } > eol_rndtrp_die = git_config_bool(var, value); > global_conv_flags_eol = eol_rndtrp_die ? > - CONV_EOL_RNDTRP_DIE : CONV_EOL_RNDTRP_WARN; > + CONV_EOL_RNDTRP_DIE : 0; > return 0; > } > Yes! After applying that patch: ``` $ PATH=$PWD/prefix/bin:$PATH bash test.sh + git --version git version 2.18.0.rc1.dirty + rm -rf repo + git init repo Initialized empty Git repository in /tmp/git/repo/.git/ + cd repo + git config core.autocrlf input + git config core.safecrlf false + echo -en 'foo\r\nbar\r\n' + git add f ``` Anthony
[PATCH] format-patch: clear UNINTERESTING flag before prepare_bases
When users specify the commit range with 'Z..C' pattern for format-patch, all
the parents of Z (including Z) would be marked as UNINTERESTING which would
prevent revision walk in prepare_bases from getting the prerequisite commits,
thus `git format-patch --base Z..C` won't be able to generate
the list of prerequisite patch ids. Clear UNINTERESTING flag with
clear_object_flags solves this issue.
Reported-by: Eduardo Habkost
Signed-off-by: Xiaolong Ye
---
builtin/log.c | 1 +
t/t4014-format-patch.sh | 6 --
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 4686f68594..01993de6fe 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1746,6 +1746,7 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
if (base_commit || base_auto) {
struct commit *base = get_base_commit(base_commit, list, nr);
reset_revision_walk();
+ clear_object_flags(UNINTERESTING);
prepare_bases(, base, list, nr);
}
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 028d5507a6..53880da7bb 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1554,13 +1554,15 @@ test_expect_success 'format-patch -o overrides
format.outputDirectory' '
test_expect_success 'format-patch --base' '
git checkout side &&
- git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual &&
+ git format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 &&
+ git format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&
echo >expected &&
echo "base-commit: $(git rev-parse HEAD~3)" >>expected &&
echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id
--stable | awk "{print \$1}")" >>expected &&
echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id
--stable | awk "{print \$1}")" >>expected &&
signature >> expected &&
- test_cmp expected actual
+ test_cmp expected actual1 &&
+ test_cmp expected actual2
'
test_expect_success 'format-patch --base errors out when base commit is in
revision list' '
--
2.16.GIT
Re: What's cooking in git.git (Jun 2018, #02; Mon, 4)
On Mon, Jun 04, 2018 at 04:18:17PM +0200, SZEDER Gábor wrote:
>
> > * jk/index-pack-maint (2018-06-01) 2 commits
> > (merged to 'next' on 2018-06-04 at c553a485e8)
> > + index-pack: handle --strict checks of non-repo packs
> > + prepare_commit_graft: treat non-repository as a noop
> >
> > "index-pack --strict" has been taught to make sure that it runs the
> > final object integrity checks after making the freshly indexed
> > packfile available to itself.
> >
> > Will cook in 'next'.
>
> These patches can't be applied directly on top of v2.17.1, or there
> was a wrong merge conflict resolution, or I don't know. Anyway,
> building 368b4e5906 (index-pack: handle --strict checks of non-repo
> packs, 2018-05-31) results in:
>
> CC builtin/index-pack.o
> builtin/index-pack.c: In function ‘final’:
> builtin/index-pack.c:1487:23: warning: passing argument 1 of
> ‘install_packed_git’ from incompatible pointer type
> [-Wincompatible-pointer-types]
> install_packed_git(the_repository, p);
Oh, right, I did have to adapt for the "the_repository" bits added on
master.
The diff to apply it on maint is just:
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 7b399478dd..3030c88d38 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1484,7 +1484,7 @@ static void final(const char *final_pack_name, const char
*curr_pack_name,
struct packed_git *p;
p = add_packed_git(final_index_name, strlen(final_index_name),
0);
if (p)
- install_packed_git(the_repository, p);
+ install_packed_git(p);
}
if (!from_stdin) {
-Peff
Re: [RFC PATCH v1] http: add http.keepRejectedCredentials config
On Mon, Jun 04, 2018 at 05:26:35AM -0700, lars.schnei...@autodesk.com wrote:
> From: Lars Schneider
>
> If a Git HTTP server responds with 401 or 407, then Git tells the
> credential helper to reject and delete the credentials. In general
> this is good.
>
> However, in certain automation environments it is not desired to remove
> credentials automatically. This is in particular the case if credentials
> are only invalid temporarily (e.g. because of problems in the server's
> authentication backend).
>
> Therefore, add the config "http.keepRejectedCredentials" which tells
> Git to keep invalid credentials if set to "true".
It seems like those servers should be returning a value besides "401" if
it's a temporary error.
But alas, we live in the real world, and your patch seems like a pretty
sensible workaround for clients. This could be done at the helper layer,
but I think in practice doing it here is going to be a lot more
convenient (and doesn't preclude helpers having their own logic if
people care to extend them in that direction).
> It was considered to disable the credential deletion in credential.c
> directly. This approach was not chosen as it could be confusing to
> other callers of credential_reject() if the function does not do what
> its name says (e.g. in imap-send.c).
Yeah, I think "git credential" relies on that code, too, and you
probably should be able to manually forget a credential at that plumbing
layer.
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ab641bf5a9..184aee8dbc 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1997,6 +1997,12 @@ http.emptyAuth::
> a username in the URL, as libcurl normally requires a username for
> authentication.
>
> +http.keepRejectedCredentials::
> + Keep credentials in the credential helper that a Git server responded
> + to with 401 (unauthorized) or 407 (proxy authentication required).
> + This can be useful in automation environments where credentials might
> + become temporarily invalid. The default is `false`.
Looks good.
> http.delegation::
> Control GSSAPI credential delegation. The delegation is disabled
> by default in libcurl since version 7.21.7. Set parameter to tell
> diff --git a/http.c b/http.c
> index b4bfbceaeb..ff6932813f 100644
> --- a/http.c
> +++ b/http.c
> @@ -138,6 +138,7 @@ static int ssl_cert_password_required;
> #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> static unsigned long http_auth_methods = CURLAUTH_ANY;
> static int http_auth_methods_restricted;
> +static int keep_rejected_credentials = 0;
Minor nit, but we usually skip the redundant "= 0" for BSS variables.
> @@ -403,6 +404,11 @@ static int http_options(const char *var, const char
> *value, void *cb)
> return 0;
> }
>
> + if (!strcmp("http.keeprejectedcredentials", var)) {
> + keep_rejected_credentials = git_config_bool(var, value);
> + return 0;
> + }
> +
> /* Fall back on the default ones */
> return git_default_config(var, value, cb);
> }
> @@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results
> *results)
> return HTTP_MISSING_TARGET;
> else if (results->http_code == 401) {
> if (http_auth.username && http_auth.password) {
> - credential_reject(_auth);
> + if (!keep_rejected_credentials)
> + credential_reject(_auth);
The rest of the patch looks good.
It's possible we'd eventually want a similar feature for other
protocols, like IMAP. And that we'd in the long run prefer to have a
single credential.keepRejected that covers them all. Or maybe not. Given
that this is kind of a workaround, people might ultimately want
protocol-specific options. So I'm happy to start with "http" for now and
deal with other protocols down the road (if it's even necessary).
Some scripts that use "git credential" may want to support this config
option, too (I'm thinking of git-remote-mediawiki, which I believe
uses it for http requests). But those can be added one by one to the
porcelain scripts.
So modulo the minor "= 0" nit, this all looks good to me.
-Peff
Re: [PATCH] upload-pack: reject shallow requests that would return nothing
On Mon, Jun 4, 2018 at 12:46 PM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> Shallow clones with --shallow-since or --shalow-exclude work by >> running rev-list to get all reachable commits, then draw a boundary >> between reachable and unreachable and send "shallow" requests based on >> that. >> >> The code does miss one corner case: if rev-list returns nothing, we'll >> have no border and we'll send no shallow requests back to the client >> (i.e. no history cuts). This essentially means a full clone (or a full >> branch if the client requests just one branch). One example is the >> oldest commit is older than what is specified by --shallow-since. > > "the newest commit is older than", isn't it? That is, the cutoff > point specified is newer than the existing history. Yes. As a result, the entirely history is cut, including the tip. --shallow-exclude could also lead to this situation if the user accidentally excludes everything. -- Duy
[PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`
We allocate a `struct refspec_item` on the stack without initializing
it. In particular, its `dst` and `src` members will contain some random
data from the stack. When we later call `refspec_item_clear()`, it will
call `free()` on those pointers. So if the call to `parse_refspec()` did
not assign to them, we will be freeing some random "pointers". This is
undefined behavior.
To the best of my understanding, this cannot currently be triggered by
user-provided data. And for what it's worth, the test-suite does not
trigger this with SANITIZE=address. It can be provoked by calling
`valid_fetch_refspec(":*")`.
Zero the struct, as is done in other users of `struct refspec_item`.
Signed-off-by: Martin Ågren
---
I found some time to look into this. It does not seem to be a
user-visible bug, so not particularly critical.
refspec.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/refspec.c b/refspec.c
index ada7854f7a..7dd7e361e5 100644
--- a/refspec.c
+++ b/refspec.c
@@ -189,7 +189,10 @@ void refspec_clear(struct refspec *rs)
int valid_fetch_refspec(const char *fetch_refspec_str)
{
struct refspec_item refspec;
- int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
+ int ret;
+
+ memset(, 0, sizeof(refspec));
+ ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
refspec_item_clear();
return ret;
}
--
2.18.0.rc0.43.gb85e7bcbff
Re: [PATCH v3 09/20] commit-graph: verify corrupt OID fanout and lookup
On Mon, Jun 4, 2018 at 1:32 PM, Derrick Stolee wrote:
> On 6/2/2018 12:38 AM, Duy Nguyen wrote:
>>
>> On Thu, May 24, 2018 at 6:25 PM, Derrick Stolee
>> wrote:
>>>
>>> + if (i && oidcmp(_oid, _oid) >= 0)
>>> + graph_report("commit-graph has incorrect OID
>>> order: %s then %s",
>>> +oid_to_hex(_oid),
>>> +oid_to_hex(_oid));
>>
>> Should these strings be marked for translation with _()?
>
> I've been asking myself "Is this message helpful to anyone other than a Git
> developer?" and for this series the only one that is helpful to an end-user
> is the message about the final hash. If the hash is correct, but these other
> messages appear, then there is a bug in the code that wrote the file.
> Otherwise, file corruption is more likely and the correct course of action
> is to delete and rebuild.
Dev-only strings like this are typically prefixed with "BUG:" or
"internal error:" (unless BUG() is a better choice). Git is
unfortunately not fully i18n-ized and devs from time to time still
forget to mark string for translations when appropriate, including me.
Because of this, we still have to slowly scan through the code base
and mark more strings for translation. Something to say clearly "not
translatable on purpose" would help a lot. If "BUG:" and friends are
too much noise, a /* no translate */ comment or some other form could
also help.
But your explanation to me still sounds like corrupted file in some
form, which should be translated unless it's too cryptic. commit-graph
format may be available in non-English languages and people can still
try to figure out the problem without relying entirely on git
developers.
--
Duy
Re: What's cooking in git.git (Jun 2018, #02; Mon, 4)
On 4 June 2018 at 14:57, Junio C Hamano wrote: > Here are the topics that have been cooking. Commits prefixed with > '-' are only in 'pu' (proposed updates) while commits prefixed with > '+' are in 'next'. The ones marked with '.' do not appear in any of > the integration branches, but I am still holding onto them. > > Generally I try to avoid sending issues of "What's cooking" report > in close succession, but as I plan to go offline for most of the > remainder of the week, here is how the tree looks like as of > tonight, just after tagging 2.18-rc1. Hopefully we'll have another > rc next week and then the real thing a week after that. > > You can find the changes described here in the integration branches > of the repositories listed at > > http://git-blame.blogspot.com/p/git-public-repositories.html > > > * rm/p4-submit-with-commit-option (2018-05-21) 1 commit > - git-p4: add options --commit and --disable-rebase > > Needs sign-off. I think the signed-off version was sent in just last week: http://lists-archives.com/git/925692-git-p4-add-options-commit-and-disable-rebase.html Thanks, Luke
Request for Partnership
Good day, This is a personal email directed to you for your consideration alone, I request that it remain and be treated as such only. My name is Mr. Mallam Tanko. I am working with one of the prime banks here in Burkina Faso. Here in this bank existed a dormant account for many years, which belong to one of our late foreign customer who died in a plane crash with his family members. When I discovered that there had been neither deposits nor withdrawals from this account for this long period, I decided to carry out a system investigation and discovered that none of the family member or relations of the late person are aware of this account, which means nobody will come to take it. The amount in this account stands at $2, 700, 000.00 (Two Million Seven Hundred Thousand USA Dollars). I want a foreign account where the bank will transfer this fund. I have access to very vital information that can be used to move the money. I have done my homework very well and i have the machineries in place to get it done since I am still in active service. If it was possible for me to do it alone I would not have bothered contacting you. Ultimately I need you to play an important role in the completion of this business transaction by acting as the Next of Kin to the late customer and of which i will back you up with relevant information. Infact, what the bank need is proof and information about the late customer which I will assist you on and all the details shall be sent to you once I hear from you, reply back for more details about the transaction. Reply if you are willing to do the business. Mr Mallam Tanko.
Re: What's cooking in git.git (Jun 2018, #02; Mon, 4)
> * jk/index-pack-maint (2018-06-01) 2 commits > (merged to 'next' on 2018-06-04 at c553a485e8) > + index-pack: handle --strict checks of non-repo packs > + prepare_commit_graft: treat non-repository as a noop > > "index-pack --strict" has been taught to make sure that it runs the > final object integrity checks after making the freshly indexed > packfile available to itself. > > Will cook in 'next'. These patches can't be applied directly on top of v2.17.1, or there was a wrong merge conflict resolution, or I don't know. Anyway, building 368b4e5906 (index-pack: handle --strict checks of non-repo packs, 2018-05-31) results in: CC builtin/index-pack.o builtin/index-pack.c: In function ‘final’: builtin/index-pack.c:1487:23: warning: passing argument 1 of ‘install_packed_git’ from incompatible pointer type [-Wincompatible-pointer-types] install_packed_git(the_repository, p); ^ In file included from builtin/index-pack.c:15:0: ./packfile.h:39:13: note: expected ‘struct packed_git *’ but argument is of type ‘struct repository *’ extern void install_packed_git(struct packed_git *pack); ^ builtin/index-pack.c:1487:4: error: too many arguments to function ‘install_packed_git’ install_packed_git(the_repository, p); ^ In file included from builtin/index-pack.c:15:0: ./packfile.h:39:13: note: declared here extern void install_packed_git(struct packed_git *pack); ^ Makefile:2121: recipe for target 'builtin/index-pack.o' failed make: *** [builtin/index-pack.o] Error 1
Re: What's cooking in git.git (Jun 2018, #02; Mon, 4)
On Mon, Jun 04, 2018 at 10:57:30PM +0900, Junio C Hamano wrote: > * jk/index-pack-maint (2018-06-01) 2 commits > (merged to 'next' on 2018-06-04 at c553a485e8) > + index-pack: handle --strict checks of non-repo packs > + prepare_commit_graft: treat non-repository as a noop > > "index-pack --strict" has been taught to make sure that it runs the > final object integrity checks after making the freshly indexed > packfile available to itself. > > Will cook in 'next'. This second patch fixes a regression in v2.18.0-rc1 and in v2.17.1. I don't know if we'd want to consider it for v2.18 or not (it should be able to be applied independently from the first). > * jk/branch-l-0-deprecation (2018-05-25) 5 commits > (merged to 'next' on 2018-05-30 at a94574dfd5) > + branch: customize "-l" warning in list mode > + branch: issue "-l" deprecation warning after pager starts > (merged to 'next' on 2018-04-11 at 9b2b0305dd) > + branch: deprecate "-l" option > + t: switch "branch -l" to "branch --create-reflog" > + t3200: unset core.logallrefupdates when testing reflog creation > (this branch is used by jk/branch-l-1-removal and > jk/branch-l-2-reincarnation.) > > The "-l" option in "git branch -l" is an unfortunate short-hand for > "--create-reflog", but many users, both old and new, somehow expect > it to be something else, perhaps "--list". This step deprecates > the short-hand and warns about the future removal of the it when it > is used. > > Will cook in 'next'. > Perhaps merge to 'master' immediately after 2.18 release? FWIW, I plan to re-roll this according to the discussion (with the intent that this would just get ejected when 'next' is rewound). But there is no rush, since that is all post-release. So you can see if I get around to it or not by then. ;) -Peff
Re: [PATCH] Use ZSH_NAME instead of ZSH_VERSION because it's redefined by git-completion.zsh
On 4 June 2018 at 05:40, Junio C Hamano wrote:
Rick van Hattem writes:
> > The `git-completion.zsh` unsets the `$ZSH_VERSION` which makes this check
> > moot. The result (at least for me) is that zsh segfaults because of all the
> > variables it's unsetting.
> > ---
>
> Overlong line, lack of sign-off.
Apologies for the long lines, I wrote the message on Github where this
message is properly formatted, apparently the submitgit script can be
considered broken as it truncates the message when converting to email.
The original message can be found here: https://github.com/git/git/pull/500
Below is the original message with proper formatting:
> A recent change (94408dc) broke zsh for me (actually segfault zsh when
> trying to use git completion)
>
> The reason is that the `git-completion.zsh` sets the `ZSH_VERSION`
> variable to an empty string:
> ...
> ZSH_VERSION='' . "$script"
> ...
>
> I'm not sure if @szeder or @gitster used a different zsh version for
> testing commit 94408dc but it segfaults zsh 5.5.1
> (x86_64-apple-darwin15.6.0) on my OS X 10.11.6 machine.
>
> The proposed fix is quite simple and shouldn't break any backwards
> compatibility.
Hopefully that clears a little bit of the confusion.
> > # Clear the variables caching builtins' options when (re-)sourcing
> > # the completion script.
> > -if [[ -n ${ZSH_VERSION-} ]]; then
> > +if [[ -n ${ZSH_NAME-} ]]; then
>
> I am not a zsh user, and I do not know how reliable $ZSH_NAME can be
> taken as an indication that we are running zsh and have already
> found a usable git-completion-.bash script.
>From what I gathered this variable has been available since 1995. But
I'm not ZSH expert...
You can search for ZSH_NAME in the 3.0 changelog:
http://zsh.sourceforge.net/Etc/changelog-3.0.html
> I think what the proposed log message refers to as "unsets" is this
> part of the script:
As mentioned above, I was referring to commit 94408dc which changed the
behaviour of the bash completion script.
Specifically:
...
if [[ -n ${ZSH_VERSION-} ]]; then
unset $(set |sed -ne
's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p')
2>/dev/null
else
unset $(compgen -v __gitcomp_builtin_)
fi
...
Because the ZSH script unsets the ZSH_VERSION variable (which is needed
because the bash script checks for that later in the script) it defaults
to the bash behaviour resulting in a segfault.
> If your ZSH_VERSION is empty, doesn't it indicate that the script
> did not find a usable git-completion.bash script (to which it
> outsources the bulk of the completion work)? I do agree segfaulting
> is not a friendly way to tell you that your setup is lacking to make
> it work, but I have a feeling that what you are seeing is an
> indication of a bigger problem, which will be sweeped under the rug
> with this patch but without getting fixed...
The git-completion.zsh script purposefully unsets the ZSH_VERSION
before including the git-completion.bash script like this:
...
ZSH_VERSION='' . "$script"
...
The reason for that is (presumably) the check that's used within the
git-completion.bash script to warn ZSH users:
...
if [[ -n ${ZSH_VERSION-} ]]; then
echo "WARNING: this script is deprecated, please see
git-completion.zsh" 1>&2
...
~rick
On 4 June 2018 at 05:40, Junio C Hamano wrote:
> Rick van Hattem writes:
>
>> The `git-completion.zsh` unsets the `$ZSH_VERSION` which makes this check
>> moot. The result (at least for me) is that zsh segfaults because of all the
>> variables it's unsetting.
>> ---
>
> Overlong line, lack of sign-off.
>
>> # Clear the variables caching builtins' options when (re-)sourcing
>> # the completion script.
>> -if [[ -n ${ZSH_VERSION-} ]]; then
>> +if [[ -n ${ZSH_NAME-} ]]; then
>
> I am not a zsh user, and I do not know how reliable $ZSH_NAME can be
> taken as an indication that we are running zsh and have already
> found a usable git-completion-.bash script.
>
> I think what the proposed log message refers to as "unsets" is this
> part of the script:
>
> ...
> zstyle -s ":completion:*:*:git:*" script script
> if [ -z "$script" ]; then
> local -a locations
> local e
> locations=(
> $(dirname
> ${funcsourcetrace[1]%:*})/git-completion.bash
> ...
> )
> for e in $locations; do
> test -f $e && script="$e" && break
> done
> fi
> ZSH_VERSION='' . "$script"
> ...
>
> If your ZSH_VERSION is empty, doesn't it indicate that the script
> did not find a usable git-completion.bash script (to which it
> outsources the bulk of the completion work)? I do agree segfaulting
> is not a friendly way to tell you that your setup is lacking to make
> it work, but I have a feeling that what you are seeing is an
> indication of a bigger problem,
What's cooking in git.git (Jun 2018, #02; Mon, 4)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. Generally I try to avoid sending issues of "What's cooking" report in close succession, but as I plan to go offline for most of the remainder of the week, here is how the tree looks like as of tonight, just after tagging 2.18-rc1. Hopefully we'll have another rc next week and then the real thing a week after that. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * bc/t3430-fixup (2018-06-04) 1 commit (merged to 'next' on 2018-06-04 at 892aab2dc8) + t3430: test clean-up Test fix. * bw/refspec-api (2018-06-01) 1 commit (merged to 'next' on 2018-06-04 at f244fe357b) + refspec-api: avoid uninitialized field in refspec item Hotfix. * jt/submodule-pull-recurse-rebase (2018-05-25) 1 commit (merged to 'next' on 2018-06-01 at cd3e2a1008) + submodule: do not pass null OID to setup_revisions "git pull -recurse-submodules --rebase", when the submodule repository's history did not have anything common between ours and the upstream's, failed to execute. We need to fetch from them to continue even in such a case. * nd/remote-update-doc (2018-06-04) 2 commits (merged to 'next' on 2018-06-04 at b2901b60ad) + remote: doc typofix (merged to 'next' on 2018-06-04 at 82cce447e5) + remote.txt: update documentation for 'update' command "git remote update" can take both a single remote nickname and a nickname for remote groups, but only one of them was documented. * rd/p4-doc-markup-env (2018-06-01) 1 commit (merged to 'next' on 2018-06-04 at 80212295cf) + p4.txt: Use backquotes for variable names Doc markup update. * tg/doc-sec-list (2018-06-01) 2 commits (merged to 'next' on 2018-06-04 at e1f80ffe09) + note git-secur...@googlegroups.com in more places + SubmittingPatches: replace numbered attributes with names Doc update. -- [New Topics] * ab/checkout-default-remote (2018-06-04) 8 commits - checkout & worktree: introduce checkout.defaultRemote - checkout: add advice for ambiguous "checkout " - builtin/checkout.c: use "ret" variable for return - checkout: pass the "num_matches" up to callers - checkout.c]: change "unique" member to "num_matches" - checkout.c: introduce an *_INIT macro - checkout.h: wrap the arguments to unique_tracking_name() - checkout tests: index should be clean after dwim checkout * nd/reject-empty-shallow-request (2018-06-04) 1 commit - upload-pack: reject shallow requests that would return nothing "git fetch --shallow-since=" that specifies the cut-off point that is newer than the existing history used to end up grabbing the entire history. Such a request now errors out. * pw/add-p-recount (2018-06-04) 1 commit - add -p: fix counting empty context lines in edited patches When user edits the patch in "git add -p" and the user's editor is set to strip trailing whitespaces indiscriminately, an empty line that is unchanged in the patch would become completely empty (instead of a line with a sole SP on it). The code introduced in Git 2.17 timeframe failed to parse such a patch, but now it learned to notice the situation and cope with it. Will merge to and cook in 'next'. * rd/comment-typofix-in-sha1-file (2018-06-04) 1 commit - sha1-file.c: correct $GITDIR to $GIT_DIR in a comment In code comment typofix Will merge to 'next'. * sg/update-ref-stdin-cleanup (2018-06-04) 1 commit - update-ref --stdin: use skip_prefix() Code cleanup. Will merge to 'next'. * cc/tests-without-assuming-ref-files-backend (2018-06-04) 1 commit - t9104: kosherly remove remote refs Instead of mucking with filesystem directly, use plumbing commands update-ref etc. to manipulate the refs in the tests. Will merge to 'next'. -- [Stalled] * ab/fetch-tags-noclobber (2018-05-16) 9 commits - fixup! push tests: assert re-pushing annotated tags - fetch: stop clobbering existing tags without --force - fetch tests: add a test clobbering tag behavior - fetch tests: correct a comment "remove it" -> "remove them" - push doc: correct lies about how push refspecs work - push tests: assert re-pushing annotated tags - push tests: add more testing for forced tag pushing - push tests: fix logic error in "push" test assertion - push tests: remove redundant 'git push' invocation Expecting a reboot of the discussion to take it to some conclusion and then a reroll. cf. cf. cf. cf. * pw/add-p-select (2018-03-16) 3 commits - add -p: optimize line selection for short hunks - add -p: allow line
Re: GDPR compliance best practices?
On Mon, Jun 04, 2018 at 12:16:16AM +0200, Peter Backes wrote: > > Verifying the commit ID by itself wouldn't be any less efficient than > before. Admitteldly, it wouldn't verify the author and authordate > integrity anymore without additional work. That would be some overhead, > sure, and could be done on demand, and would mostly affect clones. For people who are doing real work on git repos, other commands that we very much care about include "git log --author=", "git tag --contains", "git blame", etc. At least for any repo that *I* control, slow those down, and I wouldn't downgrade my git binary/repo just to make some imperialistic European bureaucrats happy. Cheers, - Ted
[PATCH 2/2] tests: make forging GPG signed commits and tags more robust
A couple of test scripts create forged GPG signed commits or tags to check that such forgery can't fool various git commands' signature verification. All but one of those test scripts are prone to occasional failures because the forgery creates a bogus GPG signature, and git commands error out with an unexpected error message, e.g. "Commit deadbeef does not have a GPG signature" instead of "... has a bad GPG signature". 't5573-pull-verify-signatures.sh', 't7510-signed-commit.sh' and 't7612-merge-verify-signatures.sh' create forged signed commits like this: git commit -S -m "bad on side" && git cat-file commit side-bad >raw && sed -e "s/bad/forged bad/" raw >forged && git hash-object -w -t commit forged >forged.commit On rare occasions the given pattern occurs not only in the commit message but in the GPG signature as well, and after it's replaced in the signature the resulting signature becomes invalid, GPG will report CRC error and that it couldn't find any signature, which will then ultimately cause the test failure. Since in all three cases the pattern to be replaced during the forgery is the first word of the commit message's subject line, and since the GPG signature in the commit object is indented by a space, let's just anchor those patterns to the beginning of the line to prevent this issue. The test script 't7030-verify-tag.sh' creates a forged signed tag object in a similar way by replacing the pattern "seventh", but the GPG signature in tag objects is not indented by a space, so the above solution is not applicable in this case. However, in the tag object in question the pattern "seventh" occurs not only in the tag message but in the 'tag' header as well. To create a forged tag object it's sufficient to replace only one of the two occurences, so modify the sed script to limit the pattern to the 'tag' header (i.e. a line beginning with "tag ", which, because of the space character, can never occur in the base64-encoded GPG signature). Note that the forgery in 't7004-tag.sh' is not affected by this issue: while 't7004' does create a forged signed tag kind of the same way, it replaces "signed-tag" in the tag object, which, because of the '-' character, can never occur in the base64-encoded GPG signarute. Signed-off-by: SZEDER Gábor --- t/t5573-pull-verify-signatures.sh | 2 +- t/t7030-verify-tag.sh | 2 +- t/t7510-signed-commit.sh | 3 +-- t/t7612-merge-verify-signatures.sh | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh index 9594e891f4..747775c147 100755 --- a/t/t5573-pull-verify-signatures.sh +++ b/t/t5573-pull-verify-signatures.sh @@ -29,7 +29,7 @@ test_expect_success GPG 'create repositories with signed commits' ' echo 4 >d && git add d && test_tick && git commit -S -m "bad" && git cat-file commit HEAD >raw && - sed -e "s/bad/forged bad/" raw >forged && + sed -e "s/^bad/forged bad/" raw >forged && git hash-object -w -t commit forged >forged.commit && git checkout $(cat forged.commit) ) && diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh index b4b49eeb08..291a1e2b07 100755 --- a/t/t7030-verify-tag.sh +++ b/t/t7030-verify-tag.sh @@ -74,7 +74,7 @@ test_expect_success GPG 'verify and show signatures' ' test_expect_success GPG 'detect fudged signature' ' git cat-file tag seventh-signed >raw && - sed -e "s/seventh/7th forged/" raw >forged1 && + sed -e "/^tag / s/seventh/7th forged/" raw >forged1 && git hash-object -w -t tag forged1 >forged1.tag && test_must_fail git verify-tag $(cat forged1.tag) 2>actual1 && grep "BAD signature from" actual1 && diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 663bf68def..6e2015ed9a 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -142,8 +142,7 @@ test_expect_success GPG 'show signed commit with signature' ' test_expect_success GPG 'detect fudged signature' ' git cat-file commit seventh-signed >raw && - - sed -e "s/seventh/7th forged/" raw >forged1 && + sed -e "s/^seventh/7th forged/" raw >forged1 && git hash-object -w -t commit forged1 >forged1.commit && test_must_fail git verify-commit $(cat forged1.commit) && git show --pretty=short --show-signature $(cat forged1.commit) >actual1 && diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh index e797c74112..e2b1df817a 100755 --- a/t/t7612-merge-verify-signatures.sh +++ b/t/t7612-merge-verify-signatures.sh @@ -23,7 +23,7 @@ test_expect_success GPG 'create signed commits' ' echo 3 >bar && git add bar && test_tick && git commit -S -m "bad on side" && git cat-file commit side-bad >raw && - sed -e "s/bad/forged bad/" raw >forged && + sed -e "s/^bad/forged
[PATCH 1/2] t7510-signed-commit: use 'test_must_fail'
The two tests 'detect fudged signature' and 'detect fudged signature with NUL' in 't7510-signed-commit.sh' check that 'git verify-commit' errors out when encountering a forged commit, but they do so by running ! git verify-commit ... Use 'test_must_fail' instead, because that would catch potential unexpected errors like a segfault as well. Signed-off-by: SZEDER Gábor --- t/t7510-signed-commit.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 762135adea..663bf68def 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -145,7 +145,7 @@ test_expect_success GPG 'detect fudged signature' ' sed -e "s/seventh/7th forged/" raw >forged1 && git hash-object -w -t commit forged1 >forged1.commit && - ! git verify-commit $(cat forged1.commit) && + test_must_fail git verify-commit $(cat forged1.commit) && git show --pretty=short --show-signature $(cat forged1.commit) >actual1 && grep "BAD signature from" actual1 && ! grep "Good signature from" actual1 @@ -156,7 +156,7 @@ test_expect_success GPG 'detect fudged signature with NUL' ' cat raw >forged2 && echo Qwik | tr "Q" "\000" >>forged2 && git hash-object -w -t commit forged2 >forged2.commit && - ! git verify-commit $(cat forged2.commit) && + test_must_fail git verify-commit $(cat forged2.commit) && git show --pretty=short --show-signature $(cat forged2.commit) >actual2 && grep "BAD signature from" actual2 && ! grep "Good signature from" actual2 -- 2.18.0.rc0.207.ga6211da864
Re: [PATCH v2] t/perf/run: Use proper "--get-regexp", not "--get-regex"
"Robert P. J. Day" writes: > one thing i don't see there, and it's based on an observation someone > once made (i believe on this list), is that even if there is > absolutely no ambiguity in a command, even if there are no pathspec > arguments, it's still worthwhile to add a trailing "--": > > $ git command options/treeish ... -- > > since that guarantees that git will waste no time trying to identify > any ambiguity since you're being so precise. is that worth mentioning > in that page? I do not think it is worth mentioning _anywhere_ if you sell its benefit as "even there is no ambiguity it won't spend cycles". The point of "git cmd X --" and "git cmd -- X" is that they save your human cycle, not machine cycle; you do not have to waste time wondering if you happen to have X as path in the working tree. That may be worth mentioning, but only "maybe" I would think. A more important reason is you may not _know_ beforehand if X you mean to be a rev also happens to be a path (or vice versa) when you are scripting. Writing 'git checkout master', 'git diff HEAD', etc., in a script you intend to be generic enough is risky if 'master', HEAD, etc. can be both rev and path at the same time, but that is already described in gitcli page ;-)
Re: [PATCH v3 15/20] commit-graph: test for corrupted octopus edge
On 6/2/2018 8:39 AM, Jakub Narebski wrote: Derrick Stolee writes: The commit-graph file has an extra chunk to store the parent int-ids for parents beyond the first parent for octopus merges. Our test repo has a single octopus merge that we can manipulate to demonstrate the 'verify' subcommand detects incorrect values in that chunk. If I understand it correctly the above means that our _reading_ code checks for validity (which then 'git commit-graph verify' uses), just there were not any tests for that. Signed-off-by: Derrick Stolee --- t/t5318-commit-graph.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 58adb8246d..240aef6add 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -248,6 +248,7 @@ test_expect_success 'git commit-graph verify' ' ' NUM_COMMITS=9 +NUM_OCTOPUS_EDGES=2 HASH_LEN=20 GRAPH_BYTE_VERSION=4 GRAPH_BYTE_HASH=5 @@ -274,6 +275,10 @@ GRAPH_BYTE_COMMIT_EXTRA_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 4` GRAPH_BYTE_COMMIT_WRONG_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 3` GRAPH_BYTE_COMMIT_GENERATION=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 8` GRAPH_BYTE_COMMIT_DATE=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN + 12` +GRAPH_COMMIT_DATA_WIDTH=`expr $HASH_LEN + 16` +GRAPH_OCTOPUS_DATA_OFFSET=`expr $GRAPH_COMMIT_DATA_OFFSET + \ + $GRAPH_COMMIT_DATA_WIDTH \* $NUM_COMMITS` +GRAPH_BYTE_OCTOPUS=`expr $GRAPH_OCTOPUS_DATA_OFFSET + 4` # usage: corrupt_graph_and_verify # Manipulates the commit-graph file at the position @@ -378,4 +383,9 @@ test_expect_success 'detect incorrect commit date' ' "commit date" ' +test_expect_success 'detect incorrect parent for octopus merge' ' + corrupt_graph_and_verify $GRAPH_BYTE_OCTOPUS "\01" \ + "invalid parent" +' So we change the int-id to non-existing commit, and check that commit-graph code checks for that. What about the case when there are octopus merges, but no EDGE chunk (which I think we can emulate by changing / corrupting number of chunks)? What about the case where int-id of edge in EDGE chunk is correct, that is points to a valid commit, but does not agree with what is in the object database (what parents octopus merge has in reality)? Do we detect the situation where the second parent value in the commit data stores an array position within a Large Edge chunk, but we do not reach a value with the most-significant bit on when reaching the end of Large Edge chunk? There are a few holes like this, but I think they are better suited to a follow-up series, as this series is already quite large. -Stolee
Re: [PATCH v3 19/20] gc: automatically write commit-graph files
On 6/2/2018 2:03 PM, Jakub Narebski wrote:
Derrick Stolee writes:
The commit-graph file is a very helpful feature for speeding up git
operations. In order to make it more useful, write the commit-graph file
by default during standard garbage collection operations.
I think you meant here "make it possible to write the commit-graph file
during standard garbage collection operations." (i.e. add "make it
possible" because it hides behind new config option, and remove "by
default" because currently it is not turned on by default).
Add a 'gc.commitGraph' config setting that triggers writing a
commit-graph file after any non-trivial 'git gc' command. Defaults to
false while the commit-graph feature matures. We specifically do not
want to turn this on by default until the commit-graph feature is fully
s/turn this on/have this on/ I think.
integrated with history-modifying features like shallow clones.
Two things.
First, shallow clones, replacement mechanims (git-replace) and grafts
are not "history-modifying" features; this name is in my opinion
reserved for history-rewriting features such as interactive rebase, the
`git filter-branch` feature or out-of-tree BFG Repo Cleaner or
reposurgeon tools. They alter the _view_ of history; they should be
IMVHO named "history-view-altering" features -- though I agree this is
mouthful.
Second, shouldn't we, as Martin Ågren said, warn about the issue in the
manpage for git-commit-graph?
Signed-off-by: Derrick Stolee
---
Documentation/config.txt | 6 ++
Documentation/git-gc.txt | 4
builtin/gc.c | 6 ++
t/t5318-commit-graph.sh | 14 ++
4 files changed, 30 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 11f027194e..9a3abd87e7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1553,6 +1553,12 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
+gc.commitGraph::
+ If true, then gc will rewrite the commit-graph file after any
+ change to the object database. If '--auto' is used, then the
+ commit-graph will not be updated unless the threshold is met.
What threshold? Ah, thresholds defined for `git gc --auto` (gc.auto,
gc.autoPackLimit, gc.logExpiry,...).
+ See linkgit:git-commit-graph[1] for details.
You missed declaring the default value for this config option.
+
gc.logExpiry::
If the file gc.log exists, then `git gc --auto` won't run
unless that file is more than 'gc.logExpiry' old. Default is
diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
index 571b5a7e3c..17dd654a59 100644
--- a/Documentation/git-gc.txt
+++ b/Documentation/git-gc.txt
@@ -119,6 +119,10 @@ The optional configuration variable `gc.packRefs`
determines if
it within all non-bare repos or it can be set to a boolean value.
This defaults to true.
+The optional configuration variable 'gc.commitGraph' determines if
+'git gc' runs 'git commit-graph write'. This can be set to a boolean
Should it be "runs" or "should run"?
+value. This defaults to false.
Should it be '...' or `...`? Below we have `gc.aggresiveWindow`, above
we have 'gc.commitGraph', for example.
+
The optional configuration variable `gc.aggressiveWindow` controls how
much time is spent optimizing the delta compression of the objects in
the repository when the --aggressive option is specified. The larger
diff --git a/builtin/gc.c b/builtin/gc.c
index 77fa720bd0..efd214a59f 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -20,6 +20,7 @@
#include "argv-array.h"
#include "commit.h"
#include "packfile.h"
+#include "commit-graph.h"
#define FAILED_RUN "failed to run %s"
@@ -34,6 +35,7 @@ static int aggressive_depth = 50;
static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
static int gc_auto_pack_limit = 50;
+static int gc_commit_graph = 0;
static int detach_auto = 1;
static timestamp_t gc_log_expire_time;
static const char *gc_log_expire = "1.day.ago";
@@ -121,6 +123,7 @@ static void gc_config(void)
git_config_get_int("gc.aggressivedepth", _depth);
git_config_get_int("gc.auto", _auto_threshold);
git_config_get_int("gc.autopacklimit", _auto_pack_limit);
+ git_config_get_bool("gc.commitgraph", _commit_graph);
git_config_get_bool("gc.autodetach", _auto);
git_config_get_expiry("gc.pruneexpire", _expire);
git_config_get_expiry("gc.worktreepruneexpire",
_worktrees_expire);
@@ -480,6 +483,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
if (pack_garbage.nr > 0)
clean_pack_garbage();
+ if (gc_commit_graph)
+ write_commit_graph_reachable(get_object_directory(), 0);
+
Nice.
Though now I wonder when appending should be used...
Appending is probably useless in the 'reachable' case, but is valuable
in the
[PATCH] t5318-commit-graph.sh: use core.commitGraph
The commit-graph tests should be checking that normal Git operations
succeed and have matching output with and without the commit-graph
feature enabled. However, the test was toggling 'core.graph' instead
of the correct 'core.commitGraph' variable.
Reported-by: Jakub Narebski
Signed-off-by: Derrick Stolee
---
t/t5318-commit-graph.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 77d85aefe7..59d0be2877 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -28,8 +28,8 @@ test_expect_success 'create commits and repack' '
'
graph_git_two_modes() {
- git -c core.graph=true $1 >output
- git -c core.graph=false $1 >expect
+ git -c core.commitGraph=true $1 >output
+ git -c core.commitGraph=false $1 >expect
test_cmp output expect
}
--
2.18.0.rc0
[RFC PATCH v1] http: add http.keepRejectedCredentials config
From: Lars Schneider
If a Git HTTP server responds with 401 or 407, then Git tells the
credential helper to reject and delete the credentials. In general
this is good.
However, in certain automation environments it is not desired to remove
credentials automatically. This is in particular the case if credentials
are only invalid temporarily (e.g. because of problems in the server's
authentication backend).
Therefore, add the config "http.keepRejectedCredentials" which tells
Git to keep invalid credentials if set to "true".
It was considered to disable the credential deletion in credential.c
directly. This approach was not chosen as it could be confusing to
other callers of credential_reject() if the function does not do what
its name says (e.g. in imap-send.c).
The Git-Credential-Manager-for-Windows already implements a similar
mechanism [1]. This solution aims to enable that feature for all
credential helper implementations.
[1]
https://github.com/Microsoft/Git-Credential-Manager-for-Windows/blob/0c1af463b33b0a0142f36f99c49ca8f83e86ee43/Shared/Cli/Functions/Common.cs#L484-L504
Signed-off-by: Lars Schneider
---
Notes:
Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/51993c2ff9
Checkout: git fetch https://github.com/larsxschneider/git keepcreds-v1 &&
git checkout 51993c2ff9
Documentation/config.txt | 6 ++
http.c | 12 ++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a9..184aee8dbc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1997,6 +1997,12 @@ http.emptyAuth::
a username in the URL, as libcurl normally requires a username for
authentication.
+http.keepRejectedCredentials::
+ Keep credentials in the credential helper that a Git server responded
+ to with 401 (unauthorized) or 407 (proxy authentication required).
+ This can be useful in automation environments where credentials might
+ become temporarily invalid. The default is `false`.
+
http.delegation::
Control GSSAPI credential delegation. The delegation is disabled
by default in libcurl since version 7.21.7. Set parameter to tell
diff --git a/http.c b/http.c
index b4bfbceaeb..ff6932813f 100644
--- a/http.c
+++ b/http.c
@@ -138,6 +138,7 @@ static int ssl_cert_password_required;
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
static unsigned long http_auth_methods = CURLAUTH_ANY;
static int http_auth_methods_restricted;
+static int keep_rejected_credentials = 0;
/* Modes for which empty_auth cannot actually help us. */
static unsigned long empty_auth_useless =
CURLAUTH_BASIC
@@ -403,6 +404,11 @@ static int http_options(const char *var, const char
*value, void *cb)
return 0;
}
+ if (!strcmp("http.keeprejectedcredentials", var)) {
+ keep_rejected_credentials = git_config_bool(var, value);
+ return 0;
+ }
+
/* Fall back on the default ones */
return git_default_config(var, value, cb);
}
@@ -1471,7 +1477,8 @@ static int handle_curl_result(struct slot_results
*results)
return HTTP_MISSING_TARGET;
else if (results->http_code == 401) {
if (http_auth.username && http_auth.password) {
- credential_reject(_auth);
+ if (!keep_rejected_credentials)
+ credential_reject(_auth);
return HTTP_NOAUTH;
} else {
#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
@@ -1485,7 +1492,8 @@ static int handle_curl_result(struct slot_results
*results)
}
} else {
if (results->http_connectcode == 407)
- credential_reject(_auth);
+ if (!keep_rejected_credentials)
+ credential_reject(_auth);
#if LIBCURL_VERSION_NUM >= 0x070c00
if (!curl_errorstr[0])
strlcpy(curl_errorstr,
base-commit: c2c7d17b030646b40e6764ba34a5ebf66aee77af
--
2.17.1
Re: GDPR compliance best practices?
Hi Peter, (lost the cc's) From: "Peter Backes" On Sun, Jun 03, 2018 at 11:28:43PM +0100, Philip Oakley wrote: It is here that Article 6 kicks in as to whether the 'organisation' can retain the data and continue to use it. Article 6 is not about continuing to use data. Article 6 is about having and even obtaining it in the first place. Correct, and that is the part I was refering to. Recipients of the particular meta data require it for the licencing purpose. Thus they can continue to have (and 'need') that data. It is that 'other side of the fence' view I mentioned. Article 17 and article 21 are about continuing to use data. For an open source project with an open source licence then an implict DCO applies for the meta data. It is the legal basis for the the release. Neither article 6 nor 17 or 21 have anything remotely like an "implicit DCO" as a legitimization for publishing employee data. I was refering to 'implict' in a reverse direction, that is, the DCO supports the legal basis to have and hold the data. The express licence terms in the various open source licences give the permission, and becomes one of these legally conflicting aspects The GDPR is very explicit about implicit stuff never being a basis for consent, if you want to imply that is your basis. And consent can be withdrawn at any time anyway. An open source license has nothing whatsoever to do with the question of version control metadata. A public version control system is not necessary to publish open source software. > - copyright is about distributing the program, not about distributing > version control metadata. It is specificaly about giving that right to copy by Jane Doe (but git gives no other information other than that supposedly globally unique 'author email'. I don't get what you are saying. As I said, a public version control system is not necessary to publish open source software. The two things may be intimately related in practice, but not in theory. Such is the law. It's the practice that is legal/illegal, decided in court (if it gets there) > - Being named is a right, not an obligation of the author. Hence, if > the author doesn't want his name published, the company doesn't have > legitimate grounds based in copyright for doing it anyway, against his > or her will. Git for Open Source is about open licencing by name. I'd agree that a closed corporate licence stays closed, but not forgotten. Again I don't get what you are saying. The author has a right to be named as the author, not an obligation. This has nothing whatsoever to do with the question of Open Source vs. closed corporate licenses. The question is which clause is being used to justify an action. Those corporate organisations want a legal basis for holding data, not a voluntary permisson (because folk may try and rescind that permission... ). Those in open source want to ensure that their licence is a legal basis for other folk to have copies, and that folk can show they have that permission. Those with a personal data view, will focus on the hope that they can remove permission, especially for companies that are doing things they find unacceptable, and maybe 'illegal' or unethical. The GDPR attempts to balance the different set of expectaions, and the overlaps will need to be negotiated. Different nations (and individuals) have different perceptions as to what is normal and reasonable thus focus on different aspects, not appreciating the Competeing Values that are present in the different Frameworks of their weltanshauung. If a closed source corporate does publish their closed data, they have real internal problems anyway regarding that contradiction! > Let's be honest: We do not know what legitimization exactly in each > specific case the git metadata is being distributed under. We should know, already. A specific licence [or limit] should be in place. We don't really want to have to let a court decide ;-) It is insufficient to have a license for distributing the program. The license is not a GDPR legitimization for git metadata. Distributing the program can be done without distributing the author's identity as part of the metadata of his commits. The law is never decided by technical means, unfortunately. It is. The GDPR refers to the state of the art of technology without defining it. Thus, technical means are very important in the GDPR. This may be something new for lawyers. If technology changes tomorrow, even without anything else changing, you may be breaking the GDPR by this simple fact tomorrow, while not breaking it today. They will still argue about what is the state of the art, and that if the art is hidden in some lab, then it's not available to meet the criteia. Again: Technology is very important in the GDPR. We know quantum computing can crack the codes, but when does it become the state of the art. SHA1 has been 'cracked' once in one special case, but that doesn't make it state
Re: how exactly can git config section names contain periods?
On Sun, Jun 03, 2018 at 06:35:44AM -0400, Robert P. J. Day wrote: > > > if (for some weird reason) i wanted to define a multi-level > > > subsection, > > > > You can't, there are no multi-level subsections, see above. > > no, i *get* that, what i was asking was if i wanted to simulate or > emulate such a thing ... or is that just getting too weird and there > is no compelling reason to want to go down that road? (which i am > totally prepared to accept.) You can do whatever you like with the subsection; its contents are generally dependent on the semantics of the key. E.g., remote..*, branch..*. That's why Git tries to be permissive with the syntax. So you are free to consider "foo.a.b.c.key" as some kind of multi-level hierarchy if that's useful to you. But you won't get any tool support from Git, and I don't think there is any compelling reason to use "." versus some other syntax (except that it perhaps looks better to the user). -Peff
Re: [PATCH v3 17/20] fsck: verify commit-graph
On 6/2/2018 12:17 PM, Jakub Narebski wrote:
Derrick Stolee writes:
If core.commitGraph is true, verify the contents of the commit-graph
during 'git fsck' using the 'git commit-graph verify' subcommand. Run
this check on all alternates, as well.
All right, so we have one config variable to control the use of
serialized commit-graph feaature. Nice.
We use a new process for two reasons:
1. The subcommand decouples the details of loading and verifying a
commit-graph file from the other fsck details.
All right, I can agree with that.
On the other hand using subcommand makes debugging harder, though not in
this case (well separated functionality that can be easily called with a
standalone command to be debugged).
2. The commit-graph verification requires the commits to be loaded
in a specific order to guarantee we parse from the commit-graph
file for some objects and from the object database for others.
I don't quite understand this. Could you explain it in more detail?
We use `lookup_commit()` when verifying the commit-graph. If these
commits were loaded earlier in the process and parsed directly from the
object database, then we aren't comparing the commit-graph file contents
against the ODB.
Signed-off-by: Derrick Stolee
---
Documentation/git-fsck.txt | 3 +++
builtin/fsck.c | 21 +
t/t5318-commit-graph.sh| 8
3 files changed, 32 insertions(+)
diff --git a/Documentation/git-fsck.txt b/Documentation/git-fsck.txt
index b9f060e3b2..ab9a93fb9b 100644
--- a/Documentation/git-fsck.txt
+++ b/Documentation/git-fsck.txt
@@ -110,6 +110,9 @@ Any corrupt objects you will have to find in backups or
other archives
(i.e., you can just remove them and do an 'rsync' with some other site in
the hopes that somebody else has the object you have corrupted).
+If core.commitGraph is true, the commit-graph file will also be inspected
Shouldn't we use `core.commitGraph` here?
+using 'git commit-graph verify'. See linkgit:git-commit-graph[1].
+
Extracted Diagnostics
-
diff --git a/builtin/fsck.c b/builtin/fsck.c
index ef78c6c00c..a6d5045b77 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -16,6 +16,7 @@
#include "streaming.h"
#include "decorate.h"
#include "packfile.h"
+#include "run-command.h"
#define REACHABLE 0x0001
#define SEEN 0x0002
@@ -45,6 +46,7 @@ static int name_objects;
#define ERROR_REACHABLE 02
#define ERROR_PACK 04
#define ERROR_REFS 010
+#define ERROR_COMMIT_GRAPH 020
Minor nitpick and a sidenote: I wonder if it wouldn't be better to
either use hexadecimal constants, or use (1 << n) for all ERROR_*
preprocesor constants.
static const char *describe_object(struct object *obj)
{
@@ -815,5 +817,24 @@ int cmd_fsck(int argc, const char **argv, const char
*prefix)
}
check_connectivity();
+
+ if (core_commit_graph) {
+ struct child_process commit_graph_verify = CHILD_PROCESS_INIT;
+ const char *verify_argv[] = { "commit-graph", "verify", NULL,
NULL, NULL, NULL };
I see that NULL at index 2 and 3 (at 3rd and 4th place) are here for
"--object-dir" and , the last one is
terminator for that case, but what is next to last NULL (at 5th place)
for?
+ commit_graph_verify.argv = verify_argv;
+ commit_graph_verify.git_cmd = 1;
+
+ if (run_command(_graph_verify))
+ errors_found |= ERROR_COMMIT_GRAPH;
+
+ prepare_alt_odb();
+ for (alt = alt_odb_list; alt; alt = alt->next) {
+ verify_argv[2] = "--object-dir";
+ verify_argv[3] = alt->path;
+ if (run_command(_graph_verify))
+ errors_found |= ERROR_COMMIT_GRAPH;
+ }
+ }
For performance reasons it may be better to start those 'git
commit-graph verify' commands asynchronously earlier, so that they can
run in parallel / concurrently wth other checks, and wait for them and
get their error code at the end of git-fsck run.
But that is probably better left for a separate commit.
+
return errors_found;
}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 2680a2ebff..4941937163 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -394,4 +394,12 @@ test_expect_success 'detect invalid checksum hash' '
"incorrect checksum"
'
+test_expect_success 'git fsck (checks commit-graph)' '
+ cd "$TRASH_DIRECTORY/full" &&
+ git fsck &&
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum" &&
+ test_must_fail git fsck
+'
All right; though the same caveats apply as with previous commit in
series. Perhaps it would be better to truncate commit-graph file, or
corrupt it in some 'random' place.
+
test_done
Best,
Re: [PATCH v3 16/20] commit-graph: verify contents match checksum
On 6/2/2018 11:52 AM, Jakub Narebski wrote:
Derrick Stolee writes:
The commit-graph file ends with a SHA1 hash of the previous contents. If
a commit-graph file has errors but the checksum hash is correct, then we
know that the problem is a bug in Git and not simply file corruption
after-the-fact.
Compute the checksum right away so it is the first error that appears,
and make the message translatable since this error can be "corrected" by
a user by simply deleting the file and recomputing. The rest of the
errors are useful only to developers.
Should we then provide --quiet / --verbose options, so that ordinary
user is not flooded with error messages meant for power users and Git
developers, then?
Be sure to continue checking the rest of the file data if the checksum
is wrong. This is important for our tests, as we break the checksum as
we modify bytes of the commit-graph file.
Well, we could have used sha1sum program, or test-sha1 helper to fix the
checksum after corrupting the commit-graph file...
Signed-off-by: Derrick Stolee
---
commit-graph.c | 16 ++--
t/t5318-commit-graph.sh | 6 ++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index d2b291aca2..a33600c584 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -841,6 +841,7 @@ void write_commit_graph(const char *obj_dir,
oids.nr = 0;
}
+#define VERIFY_COMMIT_GRAPH_ERROR_HASH 2
static int verify_commit_graph_error;
static void graph_report(const char *fmt, ...)
@@ -860,7 +861,9 @@ static void graph_report(const char *fmt, ...)
int verify_commit_graph(struct commit_graph *g)
{
uint32_t i, cur_fanout_pos = 0;
- struct object_id prev_oid, cur_oid;
+ struct object_id prev_oid, cur_oid, checksum;
+ struct hashfile *f;
+ int devnull;
if (!g) {
graph_report("no commit-graph file loaded");
@@ -879,6 +882,15 @@ int verify_commit_graph(struct commit_graph *g)
if (verify_commit_graph_error)
return verify_commit_graph_error;
+ devnull = open("/dev/null", O_WRONLY);
+ f = hashfd(devnull, NULL);
+ hashwrite(f, g->data, g->data_len - g->hash_len);
+ finalize_hashfile(f, checksum.hash, CSUM_CLOSE);
+ if (hashcmp(checksum.hash, g->data + g->data_len - g->hash_len)) {
+ graph_report(_("the commit-graph file has incorrect checksum and is
likely corrupt"));
+ verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH;
+ }
Is it the best way of calculating the SHA-1 checksum that out internal
APIs provide? Is it how SHA-1 checksum is calculated and checked for
packfiles?
This pattern is similar to hashfd_check() in csum-file.c, except we are
hashing the file data directly instead of re-creating it from scratch
(as is done for 'git index-pack --verify').
+
for (i = 0; i < g->num_commits; i++) {
struct commit *graph_commit;
@@ -916,7 +928,7 @@ int verify_commit_graph(struct commit_graph *g)
cur_fanout_pos++;
}
- if (verify_commit_graph_error)
+ if (verify_commit_graph_error & ~VERIFY_COMMIT_GRAPH_ERROR_HASH)
return verify_commit_graph_error;
So if we detected that checksum do not match, but we have not found an
error, we say that it is all right?
This only prevents us from stopping early. We will still report an error.
for (i = 0; i < g->num_commits; i++) {
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 240aef6add..2680a2ebff 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -279,6 +279,7 @@ GRAPH_COMMIT_DATA_WIDTH=`expr $HASH_LEN + 16`
GRAPH_OCTOPUS_DATA_OFFSET=`expr $GRAPH_COMMIT_DATA_OFFSET + \
$GRAPH_COMMIT_DATA_WIDTH \* $NUM_COMMITS`
GRAPH_BYTE_OCTOPUS=`expr $GRAPH_OCTOPUS_DATA_OFFSET + 4`
+GRAPH_BYTE_FOOTER=`expr $GRAPH_OCTOPUS_DATA_OFFSET + 4 \* $NUM_OCTOPUS_EDGES`
# usage: corrupt_graph_and_verify
# Manipulates the commit-graph file at the position
@@ -388,4 +389,9 @@ test_expect_success 'detect incorrect parent for octopus
merge' '
"invalid parent"
'
+test_expect_success 'detect invalid checksum hash' '
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum"
This would not work under GETTEXT_POISON, as the message is marked as
translatable, but corrupt_graph_and_verify uses 'grep' and not
'test_i18grep' from t/test-lib-functions.sh
I fixed this locally based on Szeder's comment.
+'
If it is pure checksum corruption, wouldn't this fail because it is not
a failure (exit code is 0)?
It is not zero, so the test passes.
Re: [PATCH v3 13/20] commit-graph: verify generation number
On 6/2/2018 8:23 AM, Jakub Narebski wrote:
Derrick Stolee writes:
While iterating through the commit parents, perform the generation
number calculation and compare against the value stored in the
commit-graph.
All right, that's good.
What about commit-graph files that have GENERATION_NUMBER_ZERO for all
its commits (because we verify single commit-graph file only, there
wouldn't be GENERATION_NUMBER_ZERO mixed with non-zero generation
numbers)?
Unless we can assume that no commit-graph file in the wild would have
GENERATION_NUMBER_ZERO.
I was expecting that we would not have files in the wild with
GENERATION_NUMBER_ZERO, but it looks like 2.18 will create files like
that. I'll put in logic to verify "all are GENERATION_NUMBER_ZERO or all
have 'correct' generation number".
The tests demonstrate that having a different set of parents affects
the generation number calculation, and this value propagates to
descendants. Hence, we drop the single-line condition on the output.
I don't understand what part of changes this paragraph of the commit
message refers to.
Anyway, changing parents may not lead to changed generation numbers;
take for example commit with single parent, which we change to other
commit with the same generation number.
The tests introduced in the previous commit change the parent list,
which then changes the generation number in some cases (the stored
generation number doesn't match the generation number computed based on
the loaded parents). Since we report as many errors as possible (instead
of failing on first error) those tests would fail if we say "the _only_
error should be the parent list". (This comes up again when we report
the hash is incorrect, which would appear in every test.)
The test introduced in this commit only changes the generation number,
so that test will have the one error.
Signed-off-by: Derrick Stolee
---
commit-graph.c | 18 ++
t/t5318-commit-graph.sh | 6 ++
Sidenote: I have just realized that it may be better to put
validation-related tests into different test file.
2 files changed, 24 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index fff22dc0c3..ead92460c1 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -922,6 +922,7 @@ int verify_commit_graph(struct commit_graph *g)
for (i = 0; i < g->num_commits; i++) {
struct commit *graph_commit, *odb_commit;
struct commit_list *graph_parents, *odb_parents;
+ uint32_t max_generation = 0;
hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
@@ -956,6 +957,9 @@ int verify_commit_graph(struct commit_graph *g)
oid_to_hex(_parents->item->object.oid),
oid_to_hex(_parents->item->object.oid));
+ if (graph_parents->item->generation > max_generation)
+ max_generation =
graph_parents->item->generation;
+
All right, that calculates the maximum of generation number of commit
parents.
graph_parents = graph_parents->next;
odb_parents = odb_parents->next;
}
@@ -963,6 +967,20 @@ int verify_commit_graph(struct commit_graph *g)
if (odb_parents != NULL)
graph_report("commit-graph parent list for commit %s
terminates early",
oid_to_hex(_oid));
+
+ /*
+* If one of our parents has generation GENERATION_NUMBER_MAX,
then
+* our generation is also GENERATION_NUMBER_MAX. Decrement to
avoid
+* extra logic in the following condition.
+*/
Nice trick.
+ if (max_generation == GENERATION_NUMBER_MAX)
+ max_generation--;
What about GENERATION_NUMBER_ZERO?
+
+ if (graph_commit->generation != max_generation + 1)
+ graph_report("commit-graph generation for commit %s is %u !=
%u",
+oid_to_hex(_oid),
+graph_commit->generation,
+max_generation + 1);
I think we should also check that generation numbers do not exceed
GENERATION_NUMBER_MAX... unless it is already taken care of?
We get that for free. First, the condition above would fail for at least
one commit. Second, we literally cannot store a value larger than
GENERATION_NUMBER_MAX in the commit-graph as there are only 30 bits
dedicated to the generation number.
}
return verify_commit_graph_error;
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 12f0d7f54d..673b0d37d5 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -272,6 +272,7 @@ GRAPH_BYTE_COMMIT_TREE=$GRAPH_COMMIT_DATA_OFFSET
GRAPH_BYTE_COMMIT_PARENT=`expr $GRAPH_COMMIT_DATA_OFFSET + $HASH_LEN`
Re: [PATCH v3 09/20] commit-graph: verify corrupt OID fanout and lookup
On 6/2/2018 12:38 AM, Duy Nguyen wrote:
On Thu, May 24, 2018 at 6:25 PM, Derrick Stolee wrote:
+ if (i && oidcmp(_oid, _oid) >= 0)
+ graph_report("commit-graph has incorrect OID order: %s then
%s",
+oid_to_hex(_oid),
+oid_to_hex(_oid));
Should these strings be marked for translation with _()?
I've been asking myself "Is this message helpful to anyone other than a
Git developer?" and for this series the only one that is helpful to an
end-user is the message about the final hash. If the hash is correct,
but these other messages appear, then there is a bug in the code that
wrote the file. Otherwise, file corruption is more likely and the
correct course of action is to delete and rebuild.
Thanks for being diligent in checking.
Thanks,
-Stolee
Re: [PATCH v3 06/20] commit-graph: add 'verify' subcommand
On 6/2/2018 5:19 PM, Jakub Narebski wrote:
Derrick Stolee writes:
Do we have a way to run individual steps of the test suite? I am
unfamiliar with that process.
The t/README describes three such ways in "Skipping Tests" section:
- GIT_SKIP_TESTS environment variable, which can either can match the
"t[0-9]{4}" part to skip the whole test, or t[0-9]{4} followed by
".$number" to say which particular test to skip
- For an individual test suite --run could be used to specify that
only some tests should be run or that some tests should be
excluded from a run (the latter with '!' prefix).
- 'prove' harness can also run individual tests; one of more useful
options is --state, which for example would allow to run only failed
tests with --state=failed,save ... if the tests were independent.
Adding the complexity of storing a copy of the commit-graph file for
re-use in a later test is wasted energy right now, because we need to
run the steps of the test that create the repo shape with the commits
laid out as set earlier in the test. This shape changes as we test
different states of the commit-graph (exists and contains all commits,
exists and doesn't contain all commits, etc.)
I think we can solve most of the problem by separating validation tests
(which all or almost all use the same commit-graph file) and other test;
putting them in different test scripts. This means that the more
complicated case would be limited to the subset of tests.
Anyway, if the setup stages are clearly separated and clearly marked as
such, we would be able to at least manually skip tests, or manually run
only a subset of tests.
Test independence is certainly something nice to have, but as the git
testsuite is not in best shape wrt this, it is not a requirement.
I'm all for making the test suite better. In this case, I will hold that
for a later series (that is entirely focused on that feature) as I
expect we will want to discuss the correct pattern in detail.
Thanks,
-Stolee
Re: [PATCH v2] t/perf/run: Use proper "--get-regexp", not "--get-regex"
On Sun, 3 Jun 2018, Philip Oakley wrote: > From: "Robert P. J. Day" > > On Sun, 3 Jun 2018, Thomas Gummerer wrote: > >> --get-regex works as the parse-option API allows abbreviations of > >> the full option to be specified as long as the abbreviation is > >> unambiguos. I don't know if this is documented anywhere other > >> than 'Documentation/technical/api-parse-options.txt' though. > > it's in `git help cli`: > > many commands allow a long option --option to be abbreviated only to > their unique prefix (e.g. if there is no other option whose name > begins with opt, you may be able to spell --opt to invoke the > --option flag), but you should fully spell them out when writing > your scripts; > > It's a worthwile read, even if the man page isn't flagged up that > often. agreed that it's a good read and should be referenced more often. one thing i don't see there, and it's based on an observation someone once made (i believe on this list), is that even if there is absolutely no ambiguity in a command, even if there are no pathspec arguments, it's still worthwhile to add a trailing "--": $ git command options/treeish ... -- since that guarantees that git will waste no time trying to identify any ambiguity since you're being so precise. is that worth mentioning in that page? rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [PATCH] upload-pack: reject shallow requests that would return nothing
Nguyễn Thái Ngọc Duy writes: > Shallow clones with --shallow-since or --shalow-exclude work by > running rev-list to get all reachable commits, then draw a boundary > between reachable and unreachable and send "shallow" requests based on > that. > > The code does miss one corner case: if rev-list returns nothing, we'll > have no border and we'll send no shallow requests back to the client > (i.e. no history cuts). This essentially means a full clone (or a full > branch if the client requests just one branch). One example is the > oldest commit is older than what is specified by --shallow-since. "the newest commit is older than", isn't it? That is, the cutoff point specified is newer than the existing history.
Re: does a stash *need* any reference to the branch on which it was created?
On Mon, 4 Jun 2018, Junio C Hamano wrote: > "Robert P. J. Day" writes: > > > i realize that, when you "git stash push", stash graciously > > saves the branch you were on as part of the commit message, but > > does any subsequent stash operation technically *need* that branch > > name? > > It is not "saves", but "the message it automatically generates > includes and as a human readable reminder". sorry, poor choice of words (particularly embarrassing as i'm such a stickler for wording :-P) > "git stash" does not have to read that message, as it is not > prepared to read and understand what you wrote after you ran your > own "git stash push -m 'my random message'" anyway. It is merely > for your consumption, especially when it appears in "git stash > list". right, that's what i wanted to confirm, thanks. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [PATCH] add -p: fix counting empty context lines in edited patches
On 01/06/18 21:03, Eric Sunshine wrote:
> On Fri, Jun 1, 2018 at 1:46 PM, Phillip Wood
> wrote:
>> recount_edited_hunk() introduced in commit 2b8ea7f3c7 ("add -p:
>> calculate offset delta for edited patches", 2018-03-05) required all
>> context lines to start with a space, empty lines are not counted. This
>> was intended to avoid any recounting problems if the user had
>> introduced empty lines at the end when editing the patch. However this
>> introduced a regression into 'git add -p' as it seems it is common for
>> editors to strip the trailing whitespace from empty context lines when
>> patches are edited thereby introducing empty lines that should be
>> counted. 'git apply' knows how to deal with such empty lines and POSIX
>> states that whether or not there is an space on an empty context line
>> is implementation defined [1].
>>
>> Fix the regression by counting lines consist solely of a newline as
>
> s/consist//
> --or--
> s/consist/that &/
Thanks, I'd intended to say 'that consist'
>> well as lines starting with a space as context lines and add a test to
>> prevent future regressions.
>>
>> Signed-off-by: Phillip Wood
>> ---
>> git-add--interactive.perl | 2 +-
>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {
>> - } elsif ($mode eq ' ') {
>> + } elsif ($mode eq ' ' or $_ eq "\n") {
>
> Based upon a very cursory read of parts of git-add-interactive.perl,
> do I understand correctly that we don't have to worry about $_ ever
> being "\r\n" on Windows?
>
Good question, I think the short answer no. If my understanding of the
newline section of perlport [1] is correct then on Windows "\n" eq
"\012" and the io layer replaces "\015\012" with "\n" when reading in
'text' mode (which I think is the default if you don't specify one when
opening the file/process or with binmode()). As "\n" is only one
character it would perhaps be better to test '$mode' rather than '$_'
above - what do you think.
[1] http://perldoc.perl.org/perlport.html#Newlines
>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
>> +test_expect_success 'setup file' '
>> + test_write_lines a "" b "" c >file &&
>> + git add file &&
>> + test_write_lines a "" d "" c >file
>> +'
>> +
>> +test_expect_success 'setup patch' '
>> + SP=" " &&
>> + NULL="" &&
>> + cat >patch <<-EOF
>> + [...]
>> + EOF
>> +'
>> +
>> +test_expect_success 'setup expected' '
>> + cat >expected <<-EOF
>> + [...]
>> + EOF
>> +'
>> +
>> +test_expect_success 'edit can strip spaces from empty context lines' '
>> + test_write_lines e n q | git add -p 2>error &&
>> + test_must_be_empty error &&
>> + git diff >output &&
>> + diff_cmp expected output
>> +'
>
> I would have expected all the setup work to be contained directly in
> the sole test which needs it rather than spread over three tests (two
> of which are composed of a single command). Not a big deal, and not
> worth a re-roll.
Good point I was torn between that and matching the existing style in
that file seems to be to create a million ancillary tests to do the set-up.
Thanks
Phillip
Re: [PATCH v4] json_writer: new routines to create data in JSON format
On 6/2/2018 1:13 AM, Jeff King wrote:
On Sat, Jun 02, 2018 at 06:41:06AM +0200, Duy Nguyen wrote:
On Mon, Mar 26, 2018 at 4:31 PM, wrote:
+static inline void assert_in_array(const struct json_writer *jw)
+{
+ if (!jw->open_stack.len)
+ die("json-writer: array: missing jw_array_begin()");
When you reroll, please consider marking all these string for
translation with _(), unless these are for machine consumption.
Actually, these are all basically asserts, I think. So ideally they'd be
BUG()s and not translated.
Yes, these are basically asserts. I'll convert them to BUG's and send
up another version this week.
Thanks
Jeff
Re: [PATCH] completion: complete remote names too
It was <2018-05-30 śro 17:37>, when Duy Nguyen wrote: > On Fri, May 25, 2018 at 12:48:42PM +0200, Łukasz Stelmach wrote: >> "git remote update" accepts both groups and single remotes. >> >> Signed-off-by: Łukasz Stelmach >> --- >> contrib/completion/git-completion.bash | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/contrib/completion/git-completion.bash >> b/contrib/completion/git-completion.bash >> index 961a0ed76..fb05bb2f9 100644 >> --- a/contrib/completion/git-completion.bash >> +++ b/contrib/completion/git-completion.bash >> @@ -2632,7 +2632,7 @@ _git_remote () >> __gitcomp_builtin remote_update >> ;; >> update,*) >> -__gitcomp "$(__git_get_config_variables "remotes")" >> +__gitcomp "$(__git_remotes) $(__git_get_config_variables >> "remotes")" > > Reviewed-by: me. > > The short commit description actually made me curious, which led to > more digging and finally this follow up patch. > > -- 8< -- > Subject: [PATCH] remote.txt: update documentation for 'update' command > > Commit b344e1614b (git remote update: Fallback to remote if group does > not exist - 2009-04-06) lets "git remote update" accept individual > remotes as well. Previously this command only accepted remote > groups. The commit updates the command syntax but not the actual > document of this subcommand. Update it. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > Documentation/git-remote.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt > index 595948da53..dd38587168 100644 > --- a/Documentation/git-remote.txt > +++ b/Documentation/git-remote.txt > @@ -186,8 +186,8 @@ actually prune them. > > 'update':: > > -Fetch updates for a named set of remotes in the repository as defined by > -remotes.. If a named group is not specified on the command line, > +Fetch updates for remotes or remote groups in the repository as defined by > +remotes.. If no group or remote is not specified on the command line, If neither group nor remote is specified on the command line, > the configuration parameter remotes.default will be used; if > remotes.default is not defined, all remotes which do not have the > configuration parameter remote..skipDefaultUpdate set to true will -- Łukasz Stelmach Samsung R Institute Poland Samsung Electronics signature.asc Description: PGP signature
Dear friend
Dear friend I am contacting you on a business deal of $17.5 Million US Dollars, ready for transfer into your account if we make this claim, we will share it 60%/40%. 100% risk free and it will be legally backed up with government approved If you are interested reply for more details. Best regards, Umar Bello +226 68874958
