Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag
On Fri, Mar 24, 2017 at 11:37:54PM -0700, Junio C Hamano wrote: > The hash that names a packfile is constructed by sorting all the > names of the objects contained in the packfile and running SHA-1 > hash over it. I think this MUST be hashed with collision-attack > detection. A malicious site can feed you a packfile that contains > objects the site crafts so that the sorted object names would result > in a collision-attack, ending up with one pack that contains a sets > of objects different from another pack that happens to have the same > packname, causing Git to say "Ah, this new pack must have the same > set of objects as the pack we already have" and discard it, > resulting in lost objects and a corrupt repository with missing > objects. I don't think this case really matters for collision detection. What's important is what Git does when it receives a brand-new packfile that would overwrite an existing one. It _should_ keep the old one, under the usual "existing data wins" rule. It should be easy to test, though: $ git init tmp && cd tmp $ git commit --allow-empty -m foo $ git gc $ touch -d yesterday .git/objects/pack/* $ ls -l .git/objects/pack -r--r--r-- 1 peff peff 1128 Mar 25 02:10 pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx -r--r--r-- 1 peff peff 153 Mar 25 02:10 pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack $ git index-pack --stdin <.git/objects/pack/*.pack pack 7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b $ ls -l .git/objects/pack -r--r--r-- 1 peff peff 1128 Mar 25 02:10 pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx -r--r--r-- 1 peff peff 153 Mar 25 02:10 pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack Looks like the timestamps were retained. And if we use strace, we can see what happens: $ strace git index-pac k--stdin <.git/objects/pack/*.pack link(".git/objects/pack/tmp_pack_YSrdWU", ".git/objects/pack/pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.pack") = -1 EEXIST (File exists) unlink(".git/objects/pack/tmp_pack_YSrdWU") = 0 link(".git/objects/pack/tmp_idx_O94NNU", ".git/objects/pack/pack-7e9d64ac27adc9ce1b12774dd287ff9bd8a9345b.idx") = -1 EEXIST (File exists) unlink(".git/objects/pack/tmp_idx_O94NNU") = 0 This is due to the link()/EEXIST handling in finalize_object_file. It has a FIXME for a collision check, so we could actually detect at that point whether we have a real collision, or if the other side just happened to send us the same pack. I wouldn't be surprised if the dumb-http walker is not so careful, though (but the solution is to make it careful, not to worry about a weak hash algorithm). The rest of your email all made sense to me. -Peff
Re: Will OpenSSL's license change impact us?
On Sat, Mar 25, 2017 at 10:52:47PM +0100, Ævar Arnfjörð Bjarmason wrote: > > If we want to consider performance-related concerns, I think the easier > > solution is using Nettle, which is LGPL 2.1. Considering that the > > current opinions for a new hash function are moving in the direction of > > SHA-3, which Nettle has, but OpenSSL does not, I think that might be a > > better decision overall. It was certainly the implementation I would > > use if I were to implement it. > > Yeah there's a lot of options open for just sha1-ing, but we also use > OpenSSL for TLS via imap-send. These days imap-send has basically two implementations: one that speaks imap itself (optionally using openssl), and one that just uses curl's imap support. If you build with NO_OPENSSL, the curl implementation kicks in by default. So I think any distro worried about licensing can just "make NO_OPENSSL" today and get full functionality. Curl may use openssl behind the scenes, of course, but distros already have to deal with that (at least on Debian, you can drop-in gnutls). -Peff
Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks
On Sat, Mar 25, 2017 at 2:30 PM, Dennis Kaarsemaker wrote: > >> - does this code do a reasonable thing when the path is a symbolic >>link that points at a directory? what does it mean to grab >>st.st_size for such a thing (and then go on to open() and xmmap() >>it)? > > No, it does something entirely unreasonable. I hadn't even thought of > testing with symlinks to directories, as my ulterior motive was the > next commit that makes it work with pipes. This will be fixed. To be quite honest, I do not mind it if the "toplevel pipe that came from the command line is treated as if it were a regular file" was the only change in this series, without doing anything for symbolic links. I do not use the process substitution myself, but I can see why sometimes it is handy to pass two process invocations on the command line of "diff" (if it were only one, then "-" with the usual redirection already works, but you cannot do two command using that syntax). Perhaps we can have only that part and perfect it first and have it ready for the next release, postponing the symlink dereferencing, which is a different issue?
[GSoC] Proposal: turn git-add--interactive.perl into a builtin
Hi there. First of all, I'd like to thank all of the support up to now with my microproject :). Here's a first draft of my proposal for Google Summer of Code '17, based on the "Convert scripts to builtins" idea. Please let me know what you think. --- SYNOPSIS There are many advantages to converting parts of git that are still scripts to C builtins, among which execution speed, improved compatibility and code deduplication. This proposal aims to apply this to git-add--interactive, one of the most useful features of Git. FEASIBILITY Many git scripts have attracted attention for being turned into builtins. There is ongoing work on git-stash (https://public-inbox.org/git/20170321053135.thk77soxc4irx...@sigill.intra.peff.net/), and porting interactive rebase is one of the ideas for this edition of GSoC. Not as much attention, however, has been directed to git-add--interactive. There was only one discussion regarding the feasibility of its porting (https://public-inbox.org/git/CAP8UFD2PcBsU6=FK4OHVrB7E98ycohS_0pYcbCBar=of1hl...@mail.gmail.com/). It resulted in a consensus that doing it would be a task too large – although interesting – for GSoC 2015 based on the amount of its lines of code. It is, however, only a few lines larger than git-rebase--interactive, which has been considered an appropriate idea. As such, it looks like a possible project for three months of full-time work. Aside from the benefits cited above, turning git-add--interactive into a builtin can reduce Git's dependency on Perl to the point where no "common" command would continue to rely on it. PROJECTED TIMELINE - Prior to May 4 -- Refine my basic knowledge of Perl -- Craft one or two small patches to some of Git's Perl components (preferentially to git-add--interactive itself) to improve my understanding of the language and of how Git's Perl scripts actually work - May 4 - May 30 -- Clarify implementation details with my mentor, and work on a more detailed roadmap for the project -- Investigate roughly how to replace command invocations from the script with actual builtin functions; which Git APIs in Perl already have functional equivalents in C; which parts will require a full rewrite. - May 30 - June 30 (start of coding period) -- Define the architecture of the builtin within git (which functions/interfaces will it have? where will its code reside?). -- Implement a small subset of the builtin (to be defined with my mentor) and glue it into the existing Perl script. Present this as a first patch to get feedback early regarding the implementation and avoid piling up mistakes early. -- Do necessary changes based on this initial review. -- Have roughly 1/3 of the script's functionality ported to C. - June 30 - July 28 -- Port the remainder of the script to a builtin. -- Have a weekly roadmap, sending a part of the patch every 15 days to the mailing list for review and to avoid massive commits by the end of GSoC. -- Apply suggestions from community reviews when possible; if not, save them for doing toward the end of GSoC (see below). (Note: due to a previous commitment, during a five-day period of July I will only be able to work part-time on GSoC. The actual week will be known over the next weeks.) - July 28 - August 29 -- By the start of this period, send a patch with the builtin fully implemented to the mailing list. -- Fix bugs, test extensively, possibly extend test coverage for git-add--interactive. -- Respond to the (predictably big) community feedback regarding the change. I currently work full-time in a payments company (see below), but in case of being accepted I am willing to quit my job some months early to dedicate myself fully to GSoC starting June. BIOGRAPHICAL INFORMATION My name is Daniel Ferreira and I'm a student from São Paulo, Brazil. I was accepted by Stanford University last year and I will start college this fall. I started coding C about six years ago writing up system modifications ("tweaks") for jailbroken iPhones. Since then, I have written/contributed to a couple of open-source projects like an IRC bot and other assorted things – all of them tracked on Git (https://github.com/theiostream). I have also developed a (closed-source) library in C for interacting with payment terminals in the company I have worked for over the last two years (Pagar.me). There, we use Git extensively for managing projects with around 20 people working concurrently. MICROPROJECT I have sent a series of patches to complete the microproject of converting recursive calls to readdir() into calls to dir_iterator. The most recent version can be found in https://public-inbox.org/git/1490465551-71056-2-git-send-email-bnm...@gmail.com/T/#u. Thanks, -- Daniel.
[PATCH v7 28/28] refs.h: add a note about sorting order of for_each_ref_*
Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.h | 4 ++-- t/t1405-main-ref-store.sh | 6 ++ t/t1406-submodule-ref-store.sh | 6 ++ 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/refs.h b/refs.h index 1a07f9d86f..49e97d7d5f 100644 --- a/refs.h +++ b/refs.h @@ -230,7 +230,7 @@ typedef int each_ref_fn(const char *refname, * it is not safe to modify references while an iteration is in * progress, unless the same callback function invocation that * modifies the reference also returns a nonzero value to immediately - * stop the iteration. + * stop the iteration. Returned references are sorted. */ int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data); @@ -370,7 +370,7 @@ int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void /* * Calls the specified function for each reflog file until it returns nonzero, - * and returns the value + * and returns the value. Reflog file order is unspecified. */ int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data); int for_each_reflog(each_ref_fn fn, void *cb_data); diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh index 77e1c130c2..490521f8cb 100755 --- a/t/t1405-main-ref-store.sh +++ b/t/t1405-main-ref-store.sh @@ -53,6 +53,12 @@ test_expect_success 'for_each_ref(refs/heads/)' ' test_cmp expected actual ' +test_expect_success 'for_each_ref() is sorted' ' + $RUN for-each-ref refs/heads/ | cut -c 42- >actual && + sort actual > expected && + test_cmp expected actual +' + test_expect_success 'resolve_ref(new-master)' ' SHA1=`git rev-parse new-master` && echo "$SHA1 refs/heads/new-master 0x0" >expected && diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh index 22214ebd32..13b5454c56 100755 --- a/t/t1406-submodule-ref-store.sh +++ b/t/t1406-submodule-ref-store.sh @@ -47,6 +47,12 @@ test_expect_success 'for_each_ref(refs/heads/)' ' test_cmp expected actual ' +test_expect_success 'for_each_ref() is sorted' ' + $RUN for-each-ref refs/heads/ | cut -c 42- >actual && + sort actual > expected && + test_cmp expected actual +' + test_expect_success 'resolve_ref(master)' ' SHA1=`git -C sub rev-parse master` && echo "$SHA1 refs/heads/master 0x0" >expected && -- 2.11.0.157.gd943d85
[PATCH v7 23/28] files-backend: avoid ref api targetting main ref store
A small step towards making files-backend works as a non-main ref store using the newly added store-aware API. For the record, `join` and `nm` on refs.o and files-backend.o tell me that files-backend no longer uses functions that defaults to get_main_ref_store(). I'm not yet comfortable at the idea of removing files_assert_main_repository() (or converting REF_STORE_MAIN to REF_STORE_WRITE). More staring and testing is required before that can happen. Well, except peel_ref(). I'm pretty sure that function is safe. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 84 ++-- 1 file changed, 49 insertions(+), 35 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index dec8540a0f..a5b405436f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1830,8 +1830,6 @@ static int files_peel_ref(struct ref_store *ref_store, int flag; unsigned char base[20]; - files_assert_main_repository(refs, "peel_ref"); - if (current_ref_iter && current_ref_iter->refname == refname) { struct object_id peeled; @@ -1841,7 +1839,8 @@ static int files_peel_ref(struct ref_store *ref_store, return 0; } - if (read_ref_full(refname, RESOLVE_REF_READING, base, &flag)) + if (refs_read_ref_full(ref_store, refname, + RESOLVE_REF_READING, base, &flag)) return -1; /* @@ -2009,15 +2008,15 @@ static struct ref_iterator *files_ref_iterator_begin( * on success. On error, write an error message to err, set errno, and * return a negative value. */ -static int verify_lock(struct ref_lock *lock, +static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock, const unsigned char *old_sha1, int mustexist, struct strbuf *err) { assert(err); - if (read_ref_full(lock->ref_name, - mustexist ? RESOLVE_REF_READING : 0, - lock->old_oid.hash, NULL)) { + if (refs_read_ref_full(ref_store, lock->ref_name, + mustexist ? RESOLVE_REF_READING : 0, + lock->old_oid.hash, NULL)) { if (old_sha1) { int save_errno = errno; strbuf_addf(err, "can't verify ref '%s'", lock->ref_name); @@ -2086,8 +2085,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; files_ref_path(refs, &ref_file, refname); - resolved = !!resolve_ref_unsafe(refname, resolve_flags, - lock->old_oid.hash, type); + resolved = !!refs_resolve_ref_unsafe(&refs->base, +refname, resolve_flags, +lock->old_oid.hash, type); if (!resolved && errno == EISDIR) { /* * we are trying to lock foo but we used to @@ -2104,8 +2104,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, refname); goto error_return; } - resolved = !!resolve_ref_unsafe(refname, resolve_flags, - lock->old_oid.hash, type); + resolved = !!refs_resolve_ref_unsafe(&refs->base, +refname, resolve_flags, +lock->old_oid.hash, type); } if (!resolved) { last_errno = errno; @@ -2143,7 +2144,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, goto error_return; } - if (verify_lock(lock, old_sha1, mustexist, err)) { + if (verify_lock(&refs->base, lock, old_sha1, mustexist, err)) { last_errno = errno; goto error_return; } @@ -2398,7 +2399,7 @@ static void try_remove_empty_parents(struct files_ref_store *refs, } /* make sure nobody touched the ref, and unlink */ -static void prune_ref(struct ref_to_prune *r) +static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; @@ -2406,7 +2407,7 @@ static void prune_ref(struct ref_to_prune *r) if (check_refname_format(r->name, 0)) return; - transaction = ref_transaction_begin(&err); + transaction = ref_store_transaction_begin(&refs->base, &err); if (!transaction || ref_transaction_delete(transaction, r->name, r->sha1, REF_ISPRUNING | REF_NODEREF, NULL, &err) || @@ -2420,10 +2421,10 @@ static void prune_ref(struct ref_to_prune *r) strbuf_release
[PATCH v7 25/28] t/helper: add test-ref-store to test ref-store functions
Signed-off-by: Nguyễn Thái Ngọc Duy --- Makefile| 1 + t/helper/.gitignore | 1 + t/helper/test-ref-store.c (new) | 277 3 files changed, 279 insertions(+) create mode 100644 t/helper/test-ref-store.c diff --git a/Makefile b/Makefile index a5a11e721a..5f3844e33e 100644 --- a/Makefile +++ b/Makefile @@ -622,6 +622,7 @@ TEST_PROGRAMS_NEED_X += test-parse-options TEST_PROGRAMS_NEED_X += test-path-utils TEST_PROGRAMS_NEED_X += test-prio-queue TEST_PROGRAMS_NEED_X += test-read-cache +TEST_PROGRAMS_NEED_X += test-ref-store TEST_PROGRAMS_NEED_X += test-regex TEST_PROGRAMS_NEED_X += test-revision-walking TEST_PROGRAMS_NEED_X += test-run-command diff --git a/t/helper/.gitignore b/t/helper/.gitignore index d6e8b36798..5f68aa8f8a 100644 --- a/t/helper/.gitignore +++ b/t/helper/.gitignore @@ -19,6 +19,7 @@ /test-path-utils /test-prio-queue /test-read-cache +/test-ref-store /test-regex /test-revision-walking /test-run-command diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c new file mode 100644 index 00..2d84c45ffe --- /dev/null +++ b/t/helper/test-ref-store.c @@ -0,0 +1,277 @@ +#include "cache.h" +#include "refs.h" + +static const char *notnull(const char *arg, const char *name) +{ + if (!arg) + die("%s required", name); + return arg; +} + +static unsigned int arg_flags(const char *arg, const char *name) +{ + return atoi(notnull(arg, name)); +} + +static const char **get_store(const char **argv, struct ref_store **refs) +{ + const char *gitdir; + + if (!argv[0]) { + die("ref store required"); + } else if (!strcmp(argv[0], "main")) { + *refs = get_main_ref_store(); + } else if (skip_prefix(argv[0], "submodule:", &gitdir)) { + struct strbuf sb = STRBUF_INIT; + int ret; + + ret = strbuf_git_path_submodule(&sb, gitdir, "objects/"); + if (ret) + die("strbuf_git_path_submodule failed: %d", ret); + add_to_alternates_memory(sb.buf); + strbuf_release(&sb); + + *refs = get_submodule_ref_store(gitdir); + } else + die("unknown backend %s", argv[0]); + + if (!*refs) + die("no ref store"); + + /* consume store-specific optional arguments if needed */ + + return argv + 1; +} + + +static int cmd_pack_refs(struct ref_store *refs, const char **argv) +{ + unsigned int flags = arg_flags(*argv++, "flags"); + + return refs_pack_refs(refs, flags); +} + +static int cmd_peel_ref(struct ref_store *refs, const char **argv) +{ + const char *refname = notnull(*argv++, "refname"); + unsigned char sha1[20]; + int ret; + + ret = refs_peel_ref(refs, refname, sha1); + if (!ret) + puts(sha1_to_hex(sha1)); + return ret; +} + +static int cmd_create_symref(struct ref_store *refs, const char **argv) +{ + const char *refname = notnull(*argv++, "refname"); + const char *target = notnull(*argv++, "target"); + const char *logmsg = *argv++; + + return refs_create_symref(refs, refname, target, logmsg); +} + +static int cmd_delete_refs(struct ref_store *refs, const char **argv) +{ + unsigned int flags = arg_flags(*argv++, "flags"); + struct string_list refnames = STRING_LIST_INIT_NODUP; + + while (*argv) + string_list_append(&refnames, *argv++); + + return refs_delete_refs(refs, &refnames, flags); +} + +static int cmd_rename_ref(struct ref_store *refs, const char **argv) +{ + const char *oldref = notnull(*argv++, "oldref"); + const char *newref = notnull(*argv++, "newref"); + const char *logmsg = *argv++; + + return refs_rename_ref(refs, oldref, newref, logmsg); +} + +static int each_ref(const char *refname, const struct object_id *oid, + int flags, void *cb_data) +{ + printf("%s %s 0x%x\n", oid_to_hex(oid), refname, flags); + return 0; +} + +static int cmd_for_each_ref(struct ref_store *refs, const char **argv) +{ + const char *prefix = notnull(*argv++, "prefix"); + + return refs_for_each_ref_in(refs, prefix, each_ref, NULL); +} + +static int cmd_resolve_ref(struct ref_store *refs, const char **argv) +{ + unsigned char sha1[20]; + const char *refname = notnull(*argv++, "refname"); + int resolve_flags = arg_flags(*argv++, "resolve-flags"); + int flags; + const char *ref; + + ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags, + sha1, &flags); + printf("%s %s 0x%x\n", sha1_to_hex(sha1), ref, flags); + return ref ? 0 : 1; +} + +static int cmd_verify_ref(struct ref_store *refs, const char **argv) +{ + const char *refname = notnull(*argv++, "refname"); + struct strbuf err = STRBUF_INIT; + int ret; + +
[PATCH v7 27/28] t1406: new tests for submodule ref store
Signed-off-by: Nguyễn Thái Ngọc Duy --- t/t1406-submodule-ref-store.sh (new +x) | 95 + 1 file changed, 95 insertions(+) create mode 100755 t/t1406-submodule-ref-store.sh diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh new file mode 100755 index 00..22214ebd32 --- /dev/null +++ b/t/t1406-submodule-ref-store.sh @@ -0,0 +1,95 @@ +#!/bin/sh + +test_description='test submodule ref store api' + +. ./test-lib.sh + +RUN="test-ref-store submodule:sub" + +test_expect_success 'setup' ' + git init sub && + ( + cd sub && + test_commit first && + git checkout -b new-master + ) +' + +test_expect_success 'pack_refs() not allowed' ' + test_must_fail $RUN pack-refs 3 +' + +test_expect_success 'peel_ref(new-tag)' ' + git -C sub rev-parse HEAD >expected && + git -C sub tag -a -m new-tag new-tag HEAD && + $RUN peel-ref refs/tags/new-tag >actual && + test_cmp expected actual +' + +test_expect_success 'create_symref() not allowed' ' + test_must_fail $RUN create-symref FOO refs/heads/master nothing +' + +test_expect_success 'delete_refs() not allowed' ' + test_must_fail $RUN delete-refs 0 FOO refs/tags/new-tag +' + +test_expect_success 'rename_refs() not allowed' ' + test_must_fail $RUN rename-ref refs/heads/master refs/heads/new-master +' + +test_expect_success 'for_each_ref(refs/heads/)' ' + $RUN for-each-ref refs/heads/ | cut -c 42- >actual && + cat >expected <<-\EOF && + master 0x0 + new-master 0x0 + EOF + test_cmp expected actual +' + +test_expect_success 'resolve_ref(master)' ' + SHA1=`git -C sub rev-parse master` && + echo "$SHA1 refs/heads/master 0x0" >expected && + $RUN resolve-ref refs/heads/master 0 >actual && + test_cmp expected actual +' + +test_expect_success 'verify_ref(new-master)' ' + $RUN verify-ref refs/heads/new-master +' + +test_expect_success 'for_each_reflog()' ' + $RUN for-each-reflog | sort | cut -c 42- >actual && + cat >expected <<-\EOF && + HEAD 0x1 + refs/heads/master 0x0 + refs/heads/new-master 0x0 + EOF + test_cmp expected actual +' + +test_expect_success 'for_each_reflog_ent()' ' + $RUN for-each-reflog-ent HEAD >actual && cat actual && + head -n1 actual | grep first && + tail -n2 actual | head -n1 | grep master.to.new +' + +test_expect_success 'for_each_reflog_ent_reverse()' ' + $RUN for-each-reflog-ent-reverse HEAD >actual && + head -n1 actual | grep master.to.new && + tail -n2 actual | head -n1 | grep first +' + +test_expect_success 'reflog_exists(HEAD)' ' + $RUN reflog-exists HEAD +' + +test_expect_success 'delete_reflog() not allowed' ' + test_must_fail $RUN delete-reflog HEAD +' + +test_expect_success 'create-reflog() not allowed' ' + test_must_fail $RUN create-reflog HEAD 1 +' + +test_done -- 2.11.0.157.gd943d85
[PATCH v7 24/28] refs: delete pack_refs() in favor of refs_pack_refs()
It only has one caller, not worth keeping just for convenience. Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/pack-refs.c | 2 +- refs.c | 5 - refs.h | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c index 39f9a55d16..b106a392a4 100644 --- a/builtin/pack-refs.c +++ b/builtin/pack-refs.c @@ -17,5 +17,5 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix) }; if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0)) usage_with_options(pack_refs_usage, opts); - return pack_refs(flags); + return refs_pack_refs(get_main_ref_store(), flags); } diff --git a/refs.c b/refs.c index 50ea24bd75..ec1f563824 100644 --- a/refs.c +++ b/refs.c @@ -1602,11 +1602,6 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags) return refs->be->pack_refs(refs, flags); } -int pack_refs(unsigned int flags) -{ - return refs_pack_refs(get_main_ref_store(), flags); -} - int refs_peel_ref(struct ref_store *refs, const char *refname, unsigned char *sha1) { diff --git a/refs.h b/refs.h index 37f4aa8bd5..1a07f9d86f 100644 --- a/refs.h +++ b/refs.h @@ -297,7 +297,6 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, * flags: Combination of the above PACK_REFS_* flags. */ int refs_pack_refs(struct ref_store *refs, unsigned int flags); -int pack_refs(unsigned int flags); /* * Flags controlling ref_transaction_update(), ref_transaction_create(), etc. -- 2.11.0.157.gd943d85
[PATCH v7 26/28] t1405: some basic tests on main ref store
Signed-off-by: Nguyễn Thái Ngọc Duy --- t/t1405-main-ref-store.sh (new +x) | 123 + 1 file changed, 123 insertions(+) create mode 100755 t/t1405-main-ref-store.sh diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh new file mode 100755 index 00..77e1c130c2 --- /dev/null +++ b/t/t1405-main-ref-store.sh @@ -0,0 +1,123 @@ +#!/bin/sh + +test_description='test main ref store api' + +. ./test-lib.sh + +RUN="test-ref-store main" + +test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' ' + test_commit one && + N=`find .git/refs -type f | wc -l` && + test "$N" != 0 && + $RUN pack-refs 3 && + N=`find .git/refs -type f | wc -l` +' + +test_expect_success 'peel_ref(new-tag)' ' + git rev-parse HEAD >expected && + git tag -a -m new-tag new-tag HEAD && + $RUN peel-ref refs/tags/new-tag >actual && + test_cmp expected actual +' + +test_expect_success 'create_symref(FOO, refs/heads/master)' ' + $RUN create-symref FOO refs/heads/master nothing && + echo refs/heads/master >expected && + git symbolic-ref FOO >actual && + test_cmp expected actual +' + +test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' ' + git rev-parse FOO -- && + git rev-parse refs/tags/new-tag -- && + $RUN delete-refs 0 FOO refs/tags/new-tag && + test_must_fail git rev-parse FOO -- && + test_must_fail git rev-parse refs/tags/new-tag -- +' + +test_expect_success 'rename_refs(master, new-master)' ' + git rev-parse master >expected && + $RUN rename-ref refs/heads/master refs/heads/new-master && + git rev-parse new-master >actual && + test_cmp expected actual && + test_commit recreate-master +' + +test_expect_success 'for_each_ref(refs/heads/)' ' + $RUN for-each-ref refs/heads/ | cut -c 42- >actual && + cat >expected <<-\EOF && + master 0x0 + new-master 0x0 + EOF + test_cmp expected actual +' + +test_expect_success 'resolve_ref(new-master)' ' + SHA1=`git rev-parse new-master` && + echo "$SHA1 refs/heads/new-master 0x0" >expected && + $RUN resolve-ref refs/heads/new-master 0 >actual && + test_cmp expected actual +' + +test_expect_success 'verify_ref(new-master)' ' + $RUN verify-ref refs/heads/new-master +' + +test_expect_success 'for_each_reflog()' ' + $RUN for-each-reflog | sort | cut -c 42- >actual && + cat >expected <<-\EOF && + HEAD 0x1 + refs/heads/master 0x0 + refs/heads/new-master 0x0 + EOF + test_cmp expected actual +' + +test_expect_success 'for_each_reflog_ent()' ' + $RUN for-each-reflog-ent HEAD >actual && + head -n1 actual | grep one && + tail -n2 actual | head -n1 | grep recreate-master +' + +test_expect_success 'for_each_reflog_ent_reverse()' ' + $RUN for-each-reflog-ent-reverse HEAD >actual && + head -n1 actual | grep recreate-master && + tail -n2 actual | head -n1 | grep one +' + +test_expect_success 'reflog_exists(HEAD)' ' + $RUN reflog-exists HEAD +' + +test_expect_success 'delete_reflog(HEAD)' ' + $RUN delete-reflog HEAD && + ! test -f .git/logs/HEAD +' + +test_expect_success 'create-reflog(HEAD)' ' + $RUN create-reflog HEAD 1 && + test -f .git/logs/HEAD +' + +test_expect_success 'delete_ref(refs/heads/foo)' ' + git checkout -b foo && + FOO_SHA1=`git rev-parse foo` && + git checkout --detach && + test_commit bar-commit && + git checkout -b bar && + BAR_SHA1=`git rev-parse bar` && + $RUN update-ref updating refs/heads/foo $BAR_SHA1 $FOO_SHA1 0 && + echo $BAR_SHA1 >expected && + git rev-parse refs/heads/foo >actual && + test_cmp expected actual +' + +test_expect_success 'delete_ref(refs/heads/foo)' ' + SHA1=`git rev-parse foo` && + git checkout --detach && + $RUN delete-ref msg refs/heads/foo $SHA1 0 && + test_must_fail git rev-parse refs/heads/foo -- +' + +test_done -- 2.11.0.157.gd943d85
[PATCH v7 21/28] refs: add new ref-store api
This is not meant to cover all existing API. It adds enough to test ref stores with the new test program test-ref-store, coming soon and to be used by files-backend.c. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 251 ++- refs.h | 74 +++ refs/files-backend.c | 13 +-- refs/refs-internal.h | 31 +-- 4 files changed, 270 insertions(+), 99 deletions(-) diff --git a/refs.c b/refs.c index 100a925bcc..c103f90b35 100644 --- a/refs.c +++ b/refs.c @@ -171,11 +171,23 @@ int refname_is_safe(const char *refname) return 1; } +char *refs_resolve_refdup(struct ref_store *refs, + const char *refname, int resolve_flags, + unsigned char *sha1, int *flags) +{ + const char *result; + + result = refs_resolve_ref_unsafe(refs, refname, resolve_flags, +sha1, flags); + return xstrdup_or_null(result); +} + char *resolve_refdup(const char *refname, int resolve_flags, unsigned char *sha1, int *flags) { - return xstrdup_or_null(resolve_ref_unsafe(refname, resolve_flags, - sha1, flags)); + return refs_resolve_refdup(get_main_ref_store(), + refname, resolve_flags, + sha1, flags); } /* The argument to filter_refs */ @@ -185,13 +197,20 @@ struct ref_filter { void *cb_data; }; -int read_ref_full(const char *refname, int resolve_flags, unsigned char *sha1, int *flags) +int refs_read_ref_full(struct ref_store *refs, const char *refname, + int resolve_flags, unsigned char *sha1, int *flags) { - if (resolve_ref_unsafe(refname, resolve_flags, sha1, flags)) + if (refs_resolve_ref_unsafe(refs, refname, resolve_flags, sha1, flags)) return 0; return -1; } +int read_ref_full(const char *refname, int resolve_flags, unsigned char *sha1, int *flags) +{ + return refs_read_ref_full(get_main_ref_store(), refname, + resolve_flags, sha1, flags); +} + int read_ref(const char *refname, unsigned char *sha1) { return read_ref_full(refname, RESOLVE_REF_READING, sha1, NULL); @@ -286,34 +305,52 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_li for_each_rawref(warn_if_dangling_symref, &data); } +int refs_for_each_tag_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data) +{ + return refs_for_each_ref_in(refs, "refs/tags/", fn, cb_data); +} + int for_each_tag_ref(each_ref_fn fn, void *cb_data) { - return for_each_ref_in("refs/tags/", fn, cb_data); + return refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data); } int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) { - return for_each_ref_in_submodule(submodule, "refs/tags/", fn, cb_data); + return refs_for_each_tag_ref(get_submodule_ref_store(submodule), +fn, cb_data); +} + +int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data) +{ + return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data); } int for_each_branch_ref(each_ref_fn fn, void *cb_data) { - return for_each_ref_in("refs/heads/", fn, cb_data); + return refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data); } int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) { - return for_each_ref_in_submodule(submodule, "refs/heads/", fn, cb_data); + return refs_for_each_branch_ref(get_submodule_ref_store(submodule), + fn, cb_data); +} + +int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data) +{ + return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data); } int for_each_remote_ref(each_ref_fn fn, void *cb_data) { - return for_each_ref_in("refs/remotes/", fn, cb_data); + return refs_for_each_remote_ref(get_main_ref_store(), fn, cb_data); } int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) { - return for_each_ref_in_submodule(submodule, "refs/remotes/", fn, cb_data); + return refs_for_each_remote_ref(get_submodule_ref_store(submodule), + fn, cb_data); } int head_ref_namespaced(each_ref_fn fn, void *cb_data) @@ -1120,14 +1157,17 @@ const char *find_descendant_ref(const char *dirname, return NULL; } -int rename_ref_available(const char *old_refname, const char *new_refname) +int refs_rename_ref_available(struct ref_store *refs, + const char *old_refname, + const char *new_refname) { struct string_list skip = STRING_LIST_INIT_NODUP; struct str
[PATCH v7 20/28] refs: rename get_ref_store() to get_submodule_ref_store() and make it public
This function is intended to replace *_submodule() refs API. It provides a ref store for a specific submodule, which can be operated on by a new set of refs API. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 12 refs.h | 11 +++ refs/refs-internal.h | 12 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/refs.c b/refs.c index 241b4227b2..100a925bcc 100644 --- a/refs.c +++ b/refs.c @@ -1171,7 +1171,7 @@ int head_ref(each_ref_fn fn, void *cb_data) static int do_for_each_ref(const char *submodule, const char *prefix, each_ref_fn fn, int trim, int flags, void *cb_data) { - struct ref_store *refs = get_ref_store(submodule); + struct ref_store *refs = get_submodule_ref_store(submodule); struct ref_iterator *iter; if (!refs) @@ -1344,10 +1344,10 @@ int resolve_gitlink_ref(const char *submodule, const char *refname, /* We need to strip off one or more trailing slashes */ char *stripped = xmemdupz(submodule, len); - refs = get_ref_store(stripped); + refs = get_submodule_ref_store(stripped); free(stripped); } else { - refs = get_ref_store(submodule); + refs = get_submodule_ref_store(submodule); } if (!refs) @@ -1460,13 +1460,17 @@ static void register_submodule_ref_store(struct ref_store *refs, submodule); } -struct ref_store *get_ref_store(const char *submodule) +struct ref_store *get_submodule_ref_store(const char *submodule) { struct strbuf submodule_sb = STRBUF_INIT; struct ref_store *refs; int ret; if (!submodule || !*submodule) { + /* +* FIXME: This case is ideally not allowed. But that +* can't happen until we clean up all the callers. +*/ return get_main_ref_store(); } diff --git a/refs.h b/refs.h index a6cd12267f..e6d8f67895 100644 --- a/refs.h +++ b/refs.h @@ -562,5 +562,16 @@ int reflog_expire(const char *refname, const unsigned char *sha1, int ref_storage_backend_exists(const char *name); struct ref_store *get_main_ref_store(void); +/* + * Return the ref_store instance for the specified submodule. For the + * main repository, use submodule==NULL; such a call cannot fail. For + * a submodule, the submodule must exist and be a nonbare repository, + * otherwise return NULL. If the requested reference store has not yet + * been initialized, initialize it first. + * + * For backwards compatibility, submodule=="" is treated the same as + * submodule==NULL. + */ +struct ref_store *get_submodule_ref_store(const char *submodule); #endif /* REFS_H */ diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 0cca280b5c..f20dde39ee 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -646,18 +646,6 @@ struct ref_store { void base_ref_store_init(struct ref_store *refs, const struct ref_storage_be *be); -/* - * Return the ref_store instance for the specified submodule. For the - * main repository, use submodule==NULL; such a call cannot fail. For - * a submodule, the submodule must exist and be a nonbare repository, - * otherwise return NULL. If the requested reference store has not yet - * been initialized, initialize it first. - * - * For backwards compatibility, submodule=="" is treated the same as - * submodule==NULL. - */ -struct ref_store *get_ref_store(const char *submodule); - const char *resolve_ref_recursively(struct ref_store *refs, const char *refname, int resolve_flags, -- 2.11.0.157.gd943d85
[PATCH v7 18/28] refs: move submodule code out of files-backend.c
files-backend is now initialized with a $GIT_DIR. Converting a submodule path to where real submodule gitdir is located is done in get_ref_store(). This gives a slight performance improvement for submodules since we don't convert submodule path to gitdir at every backend call like before. We pay that once at ref-store creation. More cleanup in files_downcast() and files_assert_main_repository() follows shortly. It's separate to keep noises from this patch. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 19 ++- refs/files-backend.c | 24 ++-- refs/refs-internal.h | 9 - 3 files changed, 20 insertions(+), 32 deletions(-) diff --git a/refs.c b/refs.c index 1f4c1a2347..d72b48a430 100644 --- a/refs.c +++ b/refs.c @@ -9,6 +9,7 @@ #include "refs/refs-internal.h" #include "object.h" #include "tag.h" +#include "submodule.h" /* * List of all available backends @@ -1413,9 +1414,9 @@ static struct ref_store *lookup_submodule_ref_store(const char *submodule) /* * Create, record, and return a ref_store instance for the specified - * submodule (or the main repository if submodule is NULL). + * gitdir. */ -static struct ref_store *ref_store_init(const char *submodule) +static struct ref_store *ref_store_init(const char *gitdir) { const char *be_name = "files"; struct ref_storage_be *be = find_ref_storage_backend(be_name); @@ -1424,7 +1425,7 @@ static struct ref_store *ref_store_init(const char *submodule) if (!be) die("BUG: reference backend %s is unknown", be_name); - refs = be->init(submodule); + refs = be->init(gitdir); return refs; } @@ -1433,7 +1434,7 @@ struct ref_store *get_main_ref_store(void) if (main_ref_store) return main_ref_store; - main_ref_store = ref_store_init(NULL); + main_ref_store = ref_store_init(get_git_dir()); return main_ref_store; } @@ -1474,8 +1475,16 @@ struct ref_store *get_ref_store(const char *submodule) if (!ret) return NULL; - refs = ref_store_init(submodule); + ret = submodule_to_gitdir(&submodule_sb, submodule); + if (ret) { + strbuf_release(&submodule_sb); + return NULL; + } + + refs = ref_store_init(submodule_sb.buf); register_submodule_ref_store(refs, submodule); + + strbuf_release(&submodule_sb); return refs; } diff --git a/refs/files-backend.c b/refs/files-backend.c index b62f374f9c..490f05a6f4 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -917,12 +917,6 @@ struct packed_ref_cache { struct files_ref_store { struct ref_store base; - /* -* The name of the submodule represented by this object, or -* NULL if it represents the main repository's reference -* store: -*/ - const char *submodule; char *gitdir; char *gitcommondir; char *packed_refs_path; @@ -982,22 +976,14 @@ static void clear_loose_ref_cache(struct files_ref_store *refs) * Create a new submodule ref cache and add it to the internal * set of caches. */ -static struct ref_store *files_ref_store_create(const char *submodule) +static struct ref_store *files_ref_store_create(const char *gitdir) { struct files_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; struct strbuf sb = STRBUF_INIT; - const char *gitdir = get_git_dir(); base_ref_store_init(ref_store, &refs_be_files); - if (submodule) { - refs->submodule = xstrdup(submodule); - refs->packed_refs_path = git_pathdup_submodule( - refs->submodule, "packed-refs"); - return ref_store; - } - refs->gitdir = xstrdup(gitdir); get_common_dir_noenv(&sb, gitdir); refs->gitcommondir = strbuf_detach(&sb, NULL); @@ -1014,8 +1000,7 @@ static struct ref_store *files_ref_store_create(const char *submodule) static void files_assert_main_repository(struct files_ref_store *refs, const char *caller) { - if (refs->submodule) - die("BUG: %s called for a submodule", caller); + /* This function is to be fixed up in the next patch */ } /* @@ -1206,11 +1191,6 @@ static void files_ref_path(struct files_ref_store *refs, struct strbuf *sb, const char *refname) { - if (refs->submodule) { - strbuf_git_path_submodule(sb, refs->submodule, "%s", refname); - return; - } - switch (ref_type(refname)) { case REF_TYPE_PER_WORKTREE: case REF_TYPE_PSEUDOREF: diff --git a/refs/refs-internal.h b/refs/refs-internal.h index f732473e1d..dfa1817929 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -482,12 +482,11 @@ struct ref
[PATCH v7 22/28] refs: new transaction related ref-store api
The transaction struct now takes a ref store at creation and will operate on that ref store alone. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 55 refs.h | 9 + refs/refs-internal.h | 1 + 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/refs.c b/refs.c index c103f90b35..50ea24bd75 100644 --- a/refs.c +++ b/refs.c @@ -630,16 +630,20 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1 return 0; } -int delete_ref(const char *msg, const char *refname, - const unsigned char *old_sha1, unsigned int flags) +int refs_delete_ref(struct ref_store *refs, const char *msg, + const char *refname, + const unsigned char *old_sha1, + unsigned int flags) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - if (ref_type(refname) == REF_TYPE_PSEUDOREF) + if (ref_type(refname) == REF_TYPE_PSEUDOREF) { + assert(refs == get_main_ref_store()); return delete_pseudoref(refname, old_sha1); + } - transaction = ref_transaction_begin(&err); + transaction = ref_store_transaction_begin(refs, &err); if (!transaction || ref_transaction_delete(transaction, refname, old_sha1, flags, msg, &err) || @@ -654,6 +658,13 @@ int delete_ref(const char *msg, const char *refname, return 0; } +int delete_ref(const char *msg, const char *refname, + const unsigned char *old_sha1, unsigned int flags) +{ + return refs_delete_ref(get_main_ref_store(), msg, refname, + old_sha1, flags); +} + int copy_reflog_msg(char *buf, const char *msg) { char *cp = buf; @@ -813,11 +824,20 @@ int read_ref_at(const char *refname, unsigned int flags, unsigned long at_time, return 1; } -struct ref_transaction *ref_transaction_begin(struct strbuf *err) +struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, + struct strbuf *err) { + struct ref_transaction *tr; assert(err); - return xcalloc(1, sizeof(struct ref_transaction)); + tr = xcalloc(1, sizeof(struct ref_transaction)); + tr->ref_store = refs; + return tr; +} + +struct ref_transaction *ref_transaction_begin(struct strbuf *err) +{ + return ref_store_transaction_begin(get_main_ref_store(), err); } void ref_transaction_free(struct ref_transaction *transaction) @@ -934,18 +954,20 @@ int update_ref_oid(const char *msg, const char *refname, old_oid ? old_oid->hash : NULL, flags, onerr); } -int update_ref(const char *msg, const char *refname, - const unsigned char *new_sha1, const unsigned char *old_sha1, - unsigned int flags, enum action_on_err onerr) +int refs_update_ref(struct ref_store *refs, const char *msg, + const char *refname, const unsigned char *new_sha1, + const unsigned char *old_sha1, unsigned int flags, + enum action_on_err onerr) { struct ref_transaction *t = NULL; struct strbuf err = STRBUF_INIT; int ret = 0; if (ref_type(refname) == REF_TYPE_PSEUDOREF) { + assert(refs == get_main_ref_store()); ret = write_pseudoref(refname, new_sha1, old_sha1, &err); } else { - t = ref_transaction_begin(&err); + t = ref_store_transaction_begin(refs, &err); if (!t || ref_transaction_update(t, refname, new_sha1, old_sha1, flags, msg, &err) || @@ -976,6 +998,15 @@ int update_ref(const char *msg, const char *refname, return 0; } +int update_ref(const char *msg, const char *refname, + const unsigned char *new_sha1, + const unsigned char *old_sha1, + unsigned int flags, enum action_on_err onerr) +{ + return refs_update_ref(get_main_ref_store(), msg, refname, new_sha1, + old_sha1, flags, onerr); +} + char *shorten_unambiguous_ref(const char *refname, int strict) { int i; @@ -1607,7 +1638,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master, int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { - struct ref_store *refs = get_main_ref_store(); + struct ref_store *refs = transaction->ref_store; return refs->be->transaction_commit(refs, transaction, err); } @@ -1726,7 +1757,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1, int initial_ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { - struct ref
[PATCH v7 19/28] files-backend: replace submodule_allowed check in files_downcast()
files-backend.c is unlearning submodules. Instead of having a specific check for submodules to see what operation is allowed, files backend now takes a set of flags at init. Each operation will check if the required flags is present before performing. For now we have four flags: read, write and odb access. Main ref store has all flags, obviously, while submodule stores are read-only and have access to odb (*). The "main" flag stays because many functions in the backend calls frontend ones without a ref store, so these functions always target the main ref store. Ideally the flag should be gone after ref-store-aware api is in place and used by backends. (*) Submodule code needs for_each_ref. Try take REF_STORE_ODB flag out. At least t3404 would fail. The "have access to odb" in submodule is a bit hacky since we don't know from he whether add_submodule_odb() has been called. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 15 +++--- refs/files-backend.c | 81 +--- refs/refs-internal.h | 9 +- 3 files changed, 71 insertions(+), 34 deletions(-) diff --git a/refs.c b/refs.c index d72b48a430..241b4227b2 100644 --- a/refs.c +++ b/refs.c @@ -1416,7 +1416,8 @@ static struct ref_store *lookup_submodule_ref_store(const char *submodule) * Create, record, and return a ref_store instance for the specified * gitdir. */ -static struct ref_store *ref_store_init(const char *gitdir) +static struct ref_store *ref_store_init(const char *gitdir, + unsigned int flags) { const char *be_name = "files"; struct ref_storage_be *be = find_ref_storage_backend(be_name); @@ -1425,7 +1426,7 @@ static struct ref_store *ref_store_init(const char *gitdir) if (!be) die("BUG: reference backend %s is unknown", be_name); - refs = be->init(gitdir); + refs = be->init(gitdir, flags); return refs; } @@ -1434,7 +1435,11 @@ struct ref_store *get_main_ref_store(void) if (main_ref_store) return main_ref_store; - main_ref_store = ref_store_init(get_git_dir()); + main_ref_store = ref_store_init(get_git_dir(), + (REF_STORE_READ | +REF_STORE_WRITE | +REF_STORE_ODB | +REF_STORE_MAIN)); return main_ref_store; } @@ -1481,7 +1486,9 @@ struct ref_store *get_ref_store(const char *submodule) return NULL; } - refs = ref_store_init(submodule_sb.buf); + /* pretend that add_submodule_odb() has been called */ + refs = ref_store_init(submodule_sb.buf, + REF_STORE_READ | REF_STORE_ODB); register_submodule_ref_store(refs, submodule); strbuf_release(&submodule_sb); diff --git a/refs/files-backend.c b/refs/files-backend.c index 490f05a6f4..d97a924860 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -916,6 +916,7 @@ struct packed_ref_cache { */ struct files_ref_store { struct ref_store base; + unsigned int store_flags; char *gitdir; char *gitcommondir; @@ -976,13 +977,15 @@ static void clear_loose_ref_cache(struct files_ref_store *refs) * Create a new submodule ref cache and add it to the internal * set of caches. */ -static struct ref_store *files_ref_store_create(const char *gitdir) +static struct ref_store *files_ref_store_create(const char *gitdir, + unsigned int flags) { struct files_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; struct strbuf sb = STRBUF_INIT; base_ref_store_init(ref_store, &refs_be_files); + refs->store_flags = flags; refs->gitdir = xstrdup(gitdir); get_common_dir_noenv(&sb, gitdir); @@ -994,13 +997,17 @@ static struct ref_store *files_ref_store_create(const char *gitdir) } /* - * Die if refs is for a submodule (i.e., not for the main repository). - * caller is used in any necessary error messages. + * Die if refs is not the main ref store. caller is used in any + * necessary error messages. */ static void files_assert_main_repository(struct files_ref_store *refs, const char *caller) { - /* This function is to be fixed up in the next patch */ + if (refs->store_flags & REF_STORE_MAIN) + return; + + die("BUG: unallowed operation (%s), only works " + "on main ref store\n", caller); } /* @@ -1009,9 +1016,9 @@ static void files_assert_main_repository(struct files_ref_store *refs, * files_ref_store is for a submodule (i.e., not for the main * repository). caller is used in any necessary error messages. */ -static struct files_ref_store *files_downcast( -
[PATCH v7 17/28] path.c: move some code out of strbuf_git_path_submodule()
refs is learning to avoid path rewriting that is done by strbuf_git_path_submodule(). Factor out this code so it could be reused by refs_* functions. Signed-off-by: Nguyễn Thái Ngọc Duy --- path.c | 35 +++ submodule.c | 31 +++ submodule.h | 6 ++ 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/path.c b/path.c index efcedafba6..03e7e33b6e 100644 --- a/path.c +++ b/path.c @@ -471,39 +471,19 @@ const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...) } /* Returns 0 on success, negative on failure. */ -#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1 static int do_submodule_path(struct strbuf *buf, const char *path, const char *fmt, va_list args) { - const char *git_dir; struct strbuf git_submodule_common_dir = STRBUF_INIT; struct strbuf git_submodule_dir = STRBUF_INIT; - const struct submodule *sub; - int err = 0; + int ret; - strbuf_addstr(buf, path); - strbuf_complete(buf, '/'); - strbuf_addstr(buf, ".git"); - - git_dir = read_gitfile(buf->buf); - if (git_dir) { - strbuf_reset(buf); - strbuf_addstr(buf, git_dir); - } - if (!is_git_directory(buf->buf)) { - gitmodules_config(); - sub = submodule_from_path(null_sha1, path); - if (!sub) { - err = SUBMODULE_PATH_ERR_NOT_CONFIGURED; - goto cleanup; - } - strbuf_reset(buf); - strbuf_git_path(buf, "%s/%s", "modules", sub->name); - } - - strbuf_addch(buf, '/'); - strbuf_addbuf(&git_submodule_dir, buf); + ret = submodule_to_gitdir(&git_submodule_dir, path); + if (ret) + goto cleanup; + strbuf_complete(&git_submodule_dir, '/'); + strbuf_addbuf(buf, &git_submodule_dir); strbuf_vaddf(buf, fmt, args); if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf)) @@ -514,8 +494,7 @@ static int do_submodule_path(struct strbuf *buf, const char *path, cleanup: strbuf_release(&git_submodule_dir); strbuf_release(&git_submodule_common_dir); - - return err; + return ret; } char *git_pathdup_submodule(const char *path, const char *fmt, ...) diff --git a/submodule.c b/submodule.c index 3200b7bb2b..a31f68812c 100644 --- a/submodule.c +++ b/submodule.c @@ -1596,3 +1596,34 @@ const char *get_superproject_working_tree(void) return ret; } + +int submodule_to_gitdir(struct strbuf *buf, const char *submodule) +{ + const struct submodule *sub; + const char *git_dir; + int ret = 0; + + strbuf_reset(buf); + strbuf_addstr(buf, submodule); + strbuf_complete(buf, '/'); + strbuf_addstr(buf, ".git"); + + git_dir = read_gitfile(buf->buf); + if (git_dir) { + strbuf_reset(buf); + strbuf_addstr(buf, git_dir); + } + if (!is_git_directory(buf->buf)) { + gitmodules_config(); + sub = submodule_from_path(null_sha1, submodule); + if (!sub) { + ret = -1; + goto cleanup; + } + strbuf_reset(buf); + strbuf_git_path(buf, "%s/%s", "modules", sub->name); + } + +cleanup: + return ret; +} diff --git a/submodule.h b/submodule.h index c8a0c9cb29..fce2fb64d2 100644 --- a/submodule.h +++ b/submodule.h @@ -81,6 +81,12 @@ extern int push_unpushed_submodules(struct sha1_array *commits, int dry_run); extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir); extern int parallel_submodules(void); +/* + * Given a submodule path (as in the index), return the repository + * path of that submodule in 'buf'. Return -1 on error or when the + * submodule is not initialized. + */ +int submodule_to_gitdir(struct strbuf *buf, const char *submodule); /* * Prepare the "env_array" parameter of a "struct child_process" for executing -- 2.11.0.157.gd943d85
[PATCH v7 07/28] files-backend: convert git_path() to strbuf_git_path()
git_path() and friends are going to be killed in files-backend.c in near future. And because there's a risk with overwriting buffer in git_path(), let's convert them all to strbuf_git_path(). We'll have easier time killing/converting strbuf_git_path() then because we won't have to worry about memory management again. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 130 ++- 1 file changed, 97 insertions(+), 33 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 6d0fcc88f9..5f9b0ab9bc 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2316,6 +2316,7 @@ enum { static void try_remove_empty_parents(const char *refname, unsigned int flags) { struct strbuf buf = STRBUF_INIT; + struct strbuf sb = STRBUF_INIT; char *p, *q; int i; @@ -2337,14 +2338,19 @@ static void try_remove_empty_parents(const char *refname, unsigned int flags) if (q == p) break; strbuf_setlen(&buf, q - buf.buf); - if ((flags & REMOVE_EMPTY_PARENTS_REF) && - rmdir(git_path("%s", buf.buf))) + + strbuf_reset(&sb); + strbuf_git_path(&sb, "%s", buf.buf); + if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf)) flags &= ~REMOVE_EMPTY_PARENTS_REF; - if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && - rmdir(git_path("logs/%s", buf.buf))) + + strbuf_reset(&sb); + strbuf_git_path(&sb, "logs/%s", buf.buf); + if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && rmdir(sb.buf)) flags &= ~REMOVE_EMPTY_PARENTS_REFLOG; } strbuf_release(&buf); + strbuf_release(&sb); } /* make sure nobody touched the ref, and unlink */ @@ -2506,11 +2512,16 @@ static int files_delete_refs(struct ref_store *ref_store, */ #define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log" -static int rename_tmp_log_callback(const char *path, void *cb) +struct rename_cb { + const char *tmp_renamed_log; + int true_errno; +}; + +static int rename_tmp_log_callback(const char *path, void *cb_data) { - int *true_errno = cb; + struct rename_cb *cb = cb_data; - if (rename(git_path(TMP_RENAMED_LOG), path)) { + if (rename(cb->tmp_renamed_log, path)) { /* * rename(a, b) when b is an existing directory ought * to result in ISDIR, but Solaris 5.8 gives ENOTDIR. @@ -2518,7 +2529,7 @@ static int rename_tmp_log_callback(const char *path, void *cb) * but report EISDIR to raceproof_create_file() so * that it knows to retry. */ - *true_errno = errno; + cb->true_errno = errno; if (errno == ENOTDIR) errno = EISDIR; return -1; @@ -2529,20 +2540,26 @@ static int rename_tmp_log_callback(const char *path, void *cb) static int rename_tmp_log(const char *newrefname) { - char *path = git_pathdup("logs/%s", newrefname); - int ret, true_errno; + struct strbuf path = STRBUF_INIT; + struct strbuf tmp = STRBUF_INIT; + struct rename_cb cb; + int ret; - ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno); + strbuf_git_path(&path, "logs/%s", newrefname); + strbuf_git_path(&tmp, TMP_RENAMED_LOG); + cb.tmp_renamed_log = tmp.buf; + ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb); if (ret) { if (errno == EISDIR) - error("directory not empty: %s", path); + error("directory not empty: %s", path.buf); else error("unable to move logfile %s to %s: %s", - git_path(TMP_RENAMED_LOG), path, - strerror(true_errno)); + tmp.buf, path.buf, + strerror(cb.true_errno)); } - free(path); + strbuf_release(&path); + strbuf_release(&tmp); return ret; } @@ -2583,10 +2600,17 @@ static int files_rename_ref(struct ref_store *ref_store, int flag = 0, logmoved = 0; struct ref_lock *lock; struct stat loginfo; - int log = !lstat(git_path("logs/%s", oldrefname), &loginfo); + struct strbuf sb_oldref = STRBUF_INIT; + struct strbuf sb_newref = STRBUF_INIT; + struct strbuf tmp_renamed_log = STRBUF_INIT; + int log, ret; struct strbuf err = STRBUF_INIT; - int ret; + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname); + strbuf_git_path(&sb_newref, "logs/%s", newrefname); + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG); + + log = !lstat(sb_oldref.buf, &loginfo); if (log && S_ISL
[PATCH v7 16/28] refs.c: make get_main_ref_store() public and use it
get_ref_store() will soon be renamed to get_submodule_ref_store(). Together with future get_worktree_ref_store(), the three functions provide an appropriate ref store for different operation modes. New APIs will be added to operate directly on ref stores. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 36 ++-- refs.h | 3 +++ refs/files-backend.c | 2 +- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/refs.c b/refs.c index 87f64271ac..1f4c1a2347 100644 --- a/refs.c +++ b/refs.c @@ -1314,7 +1314,7 @@ const char *resolve_ref_recursively(struct ref_store *refs, /* backend functions */ int refs_init_db(struct strbuf *err) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->init_db(refs, err); } @@ -1322,7 +1322,7 @@ int refs_init_db(struct strbuf *err) const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned char *sha1, int *flags) { - return resolve_ref_recursively(get_ref_store(NULL), refname, + return resolve_ref_recursively(get_main_ref_store(), refname, resolve_flags, sha1, flags); } @@ -1428,7 +1428,7 @@ static struct ref_store *ref_store_init(const char *submodule) return refs; } -static struct ref_store *get_main_ref_store(void) +struct ref_store *get_main_ref_store(void) { if (main_ref_store) return main_ref_store; @@ -1488,14 +1488,14 @@ void base_ref_store_init(struct ref_store *refs, /* backend functions */ int pack_refs(unsigned int flags) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->pack_refs(refs, flags); } int peel_ref(const char *refname, unsigned char *sha1) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->peel_ref(refs, refname, sha1); } @@ -1503,7 +1503,7 @@ int peel_ref(const char *refname, unsigned char *sha1) int create_symref(const char *ref_target, const char *refs_heads_master, const char *logmsg) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->create_symref(refs, ref_target, refs_heads_master, logmsg); @@ -1512,7 +1512,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master, int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->transaction_commit(refs, transaction, err); } @@ -1522,14 +1522,14 @@ int verify_refname_available(const char *refname, const struct string_list *skip, struct strbuf *err) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->verify_refname_available(refs, refname, extra, skip, err); } int for_each_reflog(each_ref_fn fn, void *cb_data) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); struct ref_iterator *iter; iter = refs->be->reflog_iterator_begin(refs); @@ -1540,7 +1540,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data) int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->for_each_reflog_ent_reverse(refs, refname, fn, cb_data); @@ -1549,14 +1549,14 @@ int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_data) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->for_each_reflog_ent(refs, refname, fn, cb_data); } int reflog_exists(const char *refname) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->reflog_exists(refs, refname); } @@ -1564,14 +1564,14 @@ int reflog_exists(const char *refname) int safe_create_reflog(const char *refname, int force_create, struct strbuf *err) { - struct ref_store *refs = get_ref_store(NULL); + struct ref_store *refs = get_main_ref_store(); return refs->be->create_reflog(refs, refname, force_create, err); } int delete_reflog(co
[PATCH v7 05/28] files-backend: add and use files_packed_refs_path()
Keep repo-related path handling in one place. This will make it easier to add submodule/multiworktree support later. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 29 ++--- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 9a9dc4e50c..f17daddffa 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -923,6 +923,8 @@ struct files_ref_store { */ const char *submodule; + char *packed_refs_path; + struct ref_entry *loose; struct packed_ref_cache *packed; }; @@ -985,7 +987,14 @@ static struct ref_store *files_ref_store_create(const char *submodule) base_ref_store_init(ref_store, &refs_be_files); - refs->submodule = xstrdup_or_null(submodule); + if (submodule) { + refs->submodule = xstrdup(submodule); + refs->packed_refs_path = git_pathdup_submodule( + refs->submodule, "packed-refs"); + return ref_store; + } + + refs->packed_refs_path = git_pathdup("packed-refs"); return ref_store; } @@ -1153,19 +1162,18 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) strbuf_release(&line); } +static const char *files_packed_refs_path(struct files_ref_store *refs) +{ + return refs->packed_refs_path; +} + /* * Get the packed_ref_cache for the specified files_ref_store, * creating it if necessary. */ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs) { - char *packed_refs_file; - - if (refs->submodule) - packed_refs_file = git_pathdup_submodule(refs->submodule, -"packed-refs"); - else - packed_refs_file = git_pathdup("packed-refs"); + const char *packed_refs_file = files_packed_refs_path(refs); if (refs->packed && !stat_validity_check(&refs->packed->validity, packed_refs_file)) @@ -1184,7 +1192,6 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref fclose(f); } } - free(packed_refs_file); return refs->packed; } @@ -2157,7 +2164,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags) } if (hold_lock_file_for_update_timeout( - &packlock, git_path("packed-refs"), + &packlock, files_packed_refs_path(refs), flags, timeout_value) < 0) return -1; /* @@ -2423,7 +2430,7 @@ static int repack_without_refs(struct files_ref_store *refs, return 0; /* no refname exists in packed refs */ if (lock_packed_refs(refs, 0)) { - unable_to_lock_message(git_path("packed-refs"), errno, err); + unable_to_lock_message(files_packed_refs_path(refs), errno, err); return -1; } packed = get_packed_refs(refs); -- 2.11.0.157.gd943d85
[PATCH v7 12/28] refs.c: introduce get_main_ref_store()
Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index e7606716dd..8aa33af4e8 100644 --- a/refs.c +++ b/refs.c @@ -1456,15 +1456,20 @@ static struct ref_store *ref_store_init(const char *submodule) return refs; } +static struct ref_store *get_main_ref_store(void) +{ + if (main_ref_store) + return main_ref_store; + + return ref_store_init(NULL); +} + struct ref_store *get_ref_store(const char *submodule) { struct ref_store *refs; if (!submodule || !*submodule) { - refs = lookup_ref_store(NULL); - - if (!refs) - refs = ref_store_init(NULL); + return get_main_ref_store(); } else { refs = lookup_ref_store(submodule); -- 2.11.0.157.gd943d85
[PATCH v7 03/28] files-backend.c: delete dead code in files_ref_iterator_begin()
It's not in the diff context, but files_downcast() is called before this check. If "refs" is NULL, we would have segfaulted before reaching the check here. And we should never see NULL refs in backend code (frontend should have caught it). Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 42c0cc14f3..caeb8188fd 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1897,9 +1897,6 @@ static struct ref_iterator *files_ref_iterator_begin( struct files_ref_iterator *iter; struct ref_iterator *ref_iterator; - if (!refs) - return empty_ref_iterator_begin(); - if (ref_paranoia < 0) ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); if (ref_paranoia) -- 2.11.0.157.gd943d85
[PATCH v7 08/28] files-backend: move "logs/" out of TMP_RENAMED_LOG
This makes reflog path building consistent, always in the form of strbuf_git_path(sb, "logs/%s", refname); It reduces the mental workload a bit in the next patch when that function call is converted. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 5f9b0ab9bc..3e0bafcaf9 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2510,7 +2510,7 @@ static int files_delete_refs(struct ref_store *ref_store, * IOW, to avoid cross device rename errors, the temporary renamed log must * live into logs/refs. */ -#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log" +#define TMP_RENAMED_LOG "refs/.tmp-renamed-log" struct rename_cb { const char *tmp_renamed_log; @@ -2546,7 +2546,7 @@ static int rename_tmp_log(const char *newrefname) int ret; strbuf_git_path(&path, "logs/%s", newrefname); - strbuf_git_path(&tmp, TMP_RENAMED_LOG); + strbuf_git_path(&tmp, "logs/%s", TMP_RENAMED_LOG); cb.tmp_renamed_log = tmp.buf; ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb); if (ret) { @@ -2608,7 +2608,7 @@ static int files_rename_ref(struct ref_store *ref_store, strbuf_git_path(&sb_oldref, "logs/%s", oldrefname); strbuf_git_path(&sb_newref, "logs/%s", newrefname); - strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG); + strbuf_git_path(&tmp_renamed_log, "logs/%s", TMP_RENAMED_LOG); log = !lstat(sb_oldref.buf, &loginfo); if (log && S_ISLNK(loginfo.st_mode)) { @@ -2633,7 +2633,7 @@ static int files_rename_ref(struct ref_store *ref_store, } if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) { - ret = error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s", + ret = error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s", oldrefname, strerror(errno)); goto out; } @@ -2719,7 +2719,7 @@ static int files_rename_ref(struct ref_store *ref_store, oldrefname, newrefname, strerror(errno)); if (!logmoved && log && rename(tmp_renamed_log.buf, sb_oldref.buf)) - error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s", + error("unable to restore logfile %s from logs/"TMP_RENAMED_LOG": %s", oldrefname, strerror(errno)); ret = 1; out: -- 2.11.0.157.gd943d85
[PATCH v7 13/28] refs: rename lookup_ref_store() to lookup_submodule_ref_store()
With get_main_ref_store() being used inside get_ref_store(), lookup_ref_store() is only used for submodule code path. Rename to reflect that and delete dead code. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 13 - 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/refs.c b/refs.c index 8aa33af4e8..a4a1a4ccfd 100644 --- a/refs.c +++ b/refs.c @@ -1395,17 +1395,13 @@ static struct ref_store *main_ref_store; static struct hashmap submodule_ref_stores; /* - * Return the ref_store instance for the specified submodule (or the - * main repository if submodule is NULL). If that ref_store hasn't - * been initialized yet, return NULL. + * Return the ref_store instance for the specified submodule. If that + * ref_store hasn't been initialized yet, return NULL. */ -static struct ref_store *lookup_ref_store(const char *submodule) +static struct ref_store *lookup_submodule_ref_store(const char *submodule) { struct submodule_hash_entry *entry; - if (!submodule) - return main_ref_store; - if (!submodule_ref_stores.tablesize) /* It's initialized on demand in register_ref_store(). */ return NULL; @@ -1471,7 +1467,7 @@ struct ref_store *get_ref_store(const char *submodule) if (!submodule || !*submodule) { return get_main_ref_store(); } else { - refs = lookup_ref_store(submodule); + refs = lookup_submodule_ref_store(submodule); if (!refs) { struct strbuf submodule_sb = STRBUF_INIT; @@ -1482,7 +1478,6 @@ struct ref_store *get_ref_store(const char *submodule) strbuf_release(&submodule_sb); } } - return refs; } -- 2.11.0.157.gd943d85
[PATCH v7 10/28] files-backend: add and use files_ref_path()
Keep repo-related path handling in one place. This will make it easier to add submodule/multiworktree support later. This automatically adds the "if submodule then use the submodule version of git_path" to other call sites too. But it does not mean those operations are submodule-ready. Not yet. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 47 +++ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index b0e0df9f66..741e52d143 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1180,6 +1180,18 @@ static void files_reflog_path(struct files_ref_store *refs, strbuf_git_path(sb, "logs/%s", refname); } +static void files_ref_path(struct files_ref_store *refs, + struct strbuf *sb, + const char *refname) +{ + if (refs->submodule) { + strbuf_git_path_submodule(sb, refs->submodule, "%s", refname); + return; + } + + strbuf_git_path(sb, "%s", refname); +} + /* * Get the packed_ref_cache for the specified files_ref_store, * creating it if necessary. @@ -1249,19 +1261,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) struct strbuf refname; struct strbuf path = STRBUF_INIT; size_t path_baselen; - int err = 0; - if (refs->submodule) - err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname); - else - strbuf_git_path(&path, "%s", dirname); + files_ref_path(refs, &path, dirname); path_baselen = path.len; - if (err) { - strbuf_release(&path); - return; - } - d = opendir(path.buf); if (!d) { strbuf_release(&path); @@ -1396,10 +1399,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, *type = 0; strbuf_reset(&sb_path); - if (refs->submodule) - strbuf_git_path_submodule(&sb_path, refs->submodule, "%s", refname); - else - strbuf_git_path(&sb_path, "%s", refname); + files_ref_path(refs, &sb_path, refname); path = sb_path.buf; @@ -1587,7 +1587,7 @@ static int lock_raw_ref(struct files_ref_store *refs, *lock_p = lock = xcalloc(1, sizeof(*lock)); lock->ref_name = xstrdup(refname); - strbuf_git_path(&ref_file, "%s", refname); + files_ref_path(refs, &ref_file, refname); retry: switch (safe_create_leading_directories(ref_file.buf)) { @@ -2056,7 +2056,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, if (flags & REF_DELETING) resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; - strbuf_git_path(&ref_file, "%s", refname); + files_ref_path(refs, &ref_file, refname); resolved = !!resolve_ref_unsafe(refname, resolve_flags, lock->old_oid.hash, type); if (!resolved && errno == EISDIR) { @@ -2355,7 +2355,7 @@ static void try_remove_empty_parents(struct files_ref_store *refs, strbuf_setlen(&buf, q - buf.buf); strbuf_reset(&sb); - strbuf_git_path(&sb, "%s", buf.buf); + files_ref_path(refs, &sb, buf.buf); if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf)) flags &= ~REMOVE_EMPTY_PARENTS_REF; @@ -2672,7 +2672,7 @@ static int files_rename_ref(struct ref_store *ref_store, struct strbuf path = STRBUF_INIT; int result; - strbuf_git_path(&path, "%s", newrefname); + files_ref_path(refs, &path, newrefname); result = remove_empty_directories(&path); strbuf_release(&path); @@ -3906,7 +3906,7 @@ static int files_transaction_commit(struct ref_store *ref_store, update->type & REF_ISSYMREF) { /* It is a loose reference. */ strbuf_reset(&sb); - strbuf_git_path(&sb, "%s", lock->ref_name); + files_ref_path(refs, &sb, lock->ref_name); if (unlink_or_msg(sb.buf, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; @@ -4206,19 +4206,18 @@ static int files_reflog_expire(struct ref_store *ref_store, static int files_init_db(struct ref_store *ref_store, struct strbuf *err) { + struct files_ref_store *refs = + files_downcast(ref_store, 0, "init_db"); struct strbuf sb = STRBUF_INIT; - /* Check validity (but we don't need the result): */ - files_downcast(ref_store, 0, "init_db"); - /* * Create .git/refs/{heads,tags}
[PATCH v7 14/28] refs.c: flatten get_ref_store() a bit
This helps the future changes in this code. And because get_ref_store() is destined to become get_submodule_ref_store(), the "get main store" code path will be removed eventually. After this the patch to delete that code will be cleaner. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 23 +-- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index a4a1a4ccfd..66dc84787d 100644 --- a/refs.c +++ b/refs.c @@ -1462,22 +1462,25 @@ static struct ref_store *get_main_ref_store(void) struct ref_store *get_ref_store(const char *submodule) { + struct strbuf submodule_sb = STRBUF_INIT; struct ref_store *refs; + int ret; if (!submodule || !*submodule) { return get_main_ref_store(); - } else { - refs = lookup_submodule_ref_store(submodule); + } - if (!refs) { - struct strbuf submodule_sb = STRBUF_INIT; + refs = lookup_submodule_ref_store(submodule); + if (refs) + return refs; - strbuf_addstr(&submodule_sb, submodule); - if (is_nonbare_repository_dir(&submodule_sb)) - refs = ref_store_init(submodule); - strbuf_release(&submodule_sb); - } - } + strbuf_addstr(&submodule_sb, submodule); + ret = is_nonbare_repository_dir(&submodule_sb); + strbuf_release(&submodule_sb); + if (!ret) + return NULL; + + refs = ref_store_init(submodule); return refs; } -- 2.11.0.157.gd943d85
[PATCH v7 09/28] files-backend: add and use files_reflog_path()
Keep repo-related path handling in one place. This will make it easier to add submodule/multiworktree support later. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 142 +++ 1 file changed, 86 insertions(+), 56 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 3e0bafcaf9..b0e0df9f66 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -165,7 +165,8 @@ static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store, const char *dirname, size_t len, int incomplete); static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry); -static int files_log_ref_write(const char *refname, const unsigned char *old_sha1, +static int files_log_ref_write(struct files_ref_store *refs, + const char *refname, const unsigned char *old_sha1, const unsigned char *new_sha1, const char *msg, int flags, struct strbuf *err); @@ -1167,6 +1168,18 @@ static const char *files_packed_refs_path(struct files_ref_store *refs) return refs->packed_refs_path; } +static void files_reflog_path(struct files_ref_store *refs, + struct strbuf *sb, + const char *refname) +{ + if (!refname) { + strbuf_git_path(sb, "logs"); + return; + } + + strbuf_git_path(sb, "logs/%s", refname); +} + /* * Get the packed_ref_cache for the specified files_ref_store, * creating it if necessary. @@ -2313,7 +2326,9 @@ enum { * subdirs. flags is a combination of REMOVE_EMPTY_PARENTS_REF and/or * REMOVE_EMPTY_PARENTS_REFLOG. */ -static void try_remove_empty_parents(const char *refname, unsigned int flags) +static void try_remove_empty_parents(struct files_ref_store *refs, +const char *refname, +unsigned int flags) { struct strbuf buf = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; @@ -2345,7 +2360,7 @@ static void try_remove_empty_parents(const char *refname, unsigned int flags) flags &= ~REMOVE_EMPTY_PARENTS_REF; strbuf_reset(&sb); - strbuf_git_path(&sb, "logs/%s", buf.buf); + files_reflog_path(refs, &sb, buf.buf); if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && rmdir(sb.buf)) flags &= ~REMOVE_EMPTY_PARENTS_REFLOG; } @@ -2538,15 +2553,15 @@ static int rename_tmp_log_callback(const char *path, void *cb_data) } } -static int rename_tmp_log(const char *newrefname) +static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname) { struct strbuf path = STRBUF_INIT; struct strbuf tmp = STRBUF_INIT; struct rename_cb cb; int ret; - strbuf_git_path(&path, "logs/%s", newrefname); - strbuf_git_path(&tmp, "logs/%s", TMP_RENAMED_LOG); + files_reflog_path(refs, &path, newrefname); + files_reflog_path(refs, &tmp, TMP_RENAMED_LOG); cb.tmp_renamed_log = tmp.buf; ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb); if (ret) { @@ -2606,9 +2621,9 @@ static int files_rename_ref(struct ref_store *ref_store, int log, ret; struct strbuf err = STRBUF_INIT; - strbuf_git_path(&sb_oldref, "logs/%s", oldrefname); - strbuf_git_path(&sb_newref, "logs/%s", newrefname); - strbuf_git_path(&tmp_renamed_log, "logs/%s", TMP_RENAMED_LOG); + files_reflog_path(refs, &sb_oldref, oldrefname); + files_reflog_path(refs, &sb_newref, newrefname); + files_reflog_path(refs, &tmp_renamed_log, TMP_RENAMED_LOG); log = !lstat(sb_oldref.buf, &loginfo); if (log && S_ISLNK(loginfo.st_mode)) { @@ -2671,7 +2686,7 @@ static int files_rename_ref(struct ref_store *ref_store, } } - if (log && rename_tmp_log(newrefname)) + if (log && rename_tmp_log(refs, newrefname)) goto rollback; logmoved = log; @@ -2786,10 +2801,15 @@ static int open_or_create_logfile(const char *path, void *cb) * set *logfd to -1. On failure, fill in *err, set *logfd to -1, and * return -1. */ -static int log_ref_setup(const char *refname, int force_create, +static int log_ref_setup(struct files_ref_store *refs, +const char *refname, int force_create, int *logfd, struct strbuf *err) { - char *logfile = git_pathdup("logs/%s", refname); + struct strbuf logfile_sb = STRBUF_INIT; + char *logfile; + + files_reflog_path(refs, &logfile_sb, refname); + logfile = strbuf_detach(&logfile_sb, NULL); if (force_create || should_autocreate_reflog(refname)) { if (raceproof_create_
[PATCH v7 11/28] files-backend: remove the use of git_path()
Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of deciding what goes where (*). The end goal is to pass $GIT_DIR only. A refs "view" of a linked worktree is a logical ref store that combines two files backends together. (*) Not entirely true since strbuf_git_path_submodule() still does path translation underneath. But that's for another patch. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 43 ++- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 741e52d143..9676cd32e4 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -923,7 +923,8 @@ struct files_ref_store { * store: */ const char *submodule; - + char *gitdir; + char *gitcommondir; char *packed_refs_path; struct ref_entry *loose; @@ -985,6 +986,8 @@ static struct ref_store *files_ref_store_create(const char *submodule) { struct files_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; + struct strbuf sb = STRBUF_INIT; + const char *gitdir = get_git_dir(); base_ref_store_init(ref_store, &refs_be_files); @@ -995,7 +998,11 @@ static struct ref_store *files_ref_store_create(const char *submodule) return ref_store; } - refs->packed_refs_path = git_pathdup("packed-refs"); + refs->gitdir = xstrdup(gitdir); + get_common_dir_noenv(&sb, gitdir); + refs->gitcommondir = strbuf_detach(&sb, NULL); + strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir); + refs->packed_refs_path = strbuf_detach(&sb, NULL); return ref_store; } @@ -1173,11 +1180,26 @@ static void files_reflog_path(struct files_ref_store *refs, const char *refname) { if (!refname) { - strbuf_git_path(sb, "logs"); + /* +* FIXME: of course this is wrong in multi worktree +* setting. To be fixed real soon. +*/ + strbuf_addf(sb, "%s/logs", refs->gitcommondir); return; } - strbuf_git_path(sb, "logs/%s", refname); + switch (ref_type(refname)) { + case REF_TYPE_PER_WORKTREE: + case REF_TYPE_PSEUDOREF: + strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname); + break; + case REF_TYPE_NORMAL: + strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname); + break; + default: + die("BUG: unknown ref type %d of ref %s", + ref_type(refname), refname); + } } static void files_ref_path(struct files_ref_store *refs, @@ -1189,7 +1211,18 @@ static void files_ref_path(struct files_ref_store *refs, return; } - strbuf_git_path(sb, "%s", refname); + switch (ref_type(refname)) { + case REF_TYPE_PER_WORKTREE: + case REF_TYPE_PSEUDOREF: + strbuf_addf(sb, "%s/%s", refs->gitdir, refname); + break; + case REF_TYPE_NORMAL: + strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname); + break; + default: + die("BUG: unknown ref type %d of ref %s", + ref_type(refname), refname); + } } /* -- 2.11.0.157.gd943d85
[PATCH v7 06/28] files-backend: make sure files_rename_ref() always reach the end
This is a no-op patch. It prepares the function so that we can release resources (to be added later in this function) before we return. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 42 -- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index f17daddffa..6d0fcc88f9 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2585,23 +2585,34 @@ static int files_rename_ref(struct ref_store *ref_store, struct stat loginfo; int log = !lstat(git_path("logs/%s", oldrefname), &loginfo); struct strbuf err = STRBUF_INIT; + int ret; - if (log && S_ISLNK(loginfo.st_mode)) - return error("reflog for %s is a symlink", oldrefname); + if (log && S_ISLNK(loginfo.st_mode)) { + ret = error("reflog for %s is a symlink", oldrefname); + goto out; + } if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - orig_sha1, &flag)) - return error("refname %s not found", oldrefname); + orig_sha1, &flag)) { + ret = error("refname %s not found", oldrefname); + goto out; + } - if (flag & REF_ISSYMREF) - return error("refname %s is a symbolic ref, renaming it is not supported", - oldrefname); - if (!rename_ref_available(oldrefname, newrefname)) - return 1; + if (flag & REF_ISSYMREF) { + ret = error("refname %s is a symbolic ref, renaming it is not supported", + oldrefname); + goto out; + } + if (!rename_ref_available(oldrefname, newrefname)) { + ret = 1; + goto out; + } - if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG))) - return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s", - oldrefname, strerror(errno)); + if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG))) { + ret = error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s", + oldrefname, strerror(errno)); + goto out; + } if (delete_ref(logmsg, oldrefname, orig_sha1, REF_NODEREF)) { error("unable to delete old %s", oldrefname); @@ -2657,7 +2668,8 @@ static int files_rename_ref(struct ref_store *ref_store, goto rollback; } - return 0; + ret = 0; + goto out; rollback: lock = lock_ref_sha1_basic(refs, oldrefname, NULL, NULL, NULL, @@ -2686,7 +2698,9 @@ static int files_rename_ref(struct ref_store *ref_store, error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s", oldrefname, strerror(errno)); - return 1; + ret = 1; + out: + return ret; } static int close_ref(struct ref_lock *lock) -- 2.11.0.157.gd943d85
[PATCH v7 15/28] refs.c: kill register_ref_store(), add register_submodule_ref_store()
This is the last function in this code (besides public API) that takes submodule argument and handles both main/submodule cases. Break it down, move main store registration in get_main_ref_store() and keep the rest in register_submodule_ref_store(). Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.c | 45 - 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/refs.c b/refs.c index 66dc84787d..87f64271ac 100644 --- a/refs.c +++ b/refs.c @@ -1412,29 +1412,6 @@ static struct ref_store *lookup_submodule_ref_store(const char *submodule) } /* - * Register the specified ref_store to be the one that should be used - * for submodule (or the main repository if submodule is NULL). It is - * a fatal error to call this function twice for the same submodule. - */ -static void register_ref_store(struct ref_store *refs, const char *submodule) -{ - if (!submodule) { - if (main_ref_store) - die("BUG: main_ref_store initialized twice"); - - main_ref_store = refs; - } else { - if (!submodule_ref_stores.tablesize) - hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0); - - if (hashmap_put(&submodule_ref_stores, - alloc_submodule_hash_entry(submodule, refs))) - die("BUG: ref_store for submodule '%s' initialized twice", - submodule); - } -} - -/* * Create, record, and return a ref_store instance for the specified * submodule (or the main repository if submodule is NULL). */ @@ -1448,7 +1425,6 @@ static struct ref_store *ref_store_init(const char *submodule) die("BUG: reference backend %s is unknown", be_name); refs = be->init(submodule); - register_ref_store(refs, submodule); return refs; } @@ -1457,7 +1433,25 @@ static struct ref_store *get_main_ref_store(void) if (main_ref_store) return main_ref_store; - return ref_store_init(NULL); + main_ref_store = ref_store_init(NULL); + return main_ref_store; +} + +/* + * Register the specified ref_store to be the one that should be used + * for submodule. It is a fatal error to call this function twice for + * the same submodule. + */ +static void register_submodule_ref_store(struct ref_store *refs, +const char *submodule) +{ + if (!submodule_ref_stores.tablesize) + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0); + + if (hashmap_put(&submodule_ref_stores, + alloc_submodule_hash_entry(submodule, refs))) + die("BUG: ref_store for submodule '%s' initialized twice", + submodule); } struct ref_store *get_ref_store(const char *submodule) @@ -1481,6 +1475,7 @@ struct ref_store *get_ref_store(const char *submodule) return NULL; refs = ref_store_init(submodule); + register_submodule_ref_store(refs, submodule); return refs; } -- 2.11.0.157.gd943d85
[PATCH v7 04/28] files-backend: delete dead code in files_init_db()
safe_create_dir() can do adjust_shared_perm() internally, and init-db has always created 'refs' in shared mode since the beginning, af6e277c5e (git-init-db: initialize shared repositories with --shared - 2005-12-22). So this code looks like extra adjust_shared_perm calls are unnecessary. And they are. But let's see why there are here in the first place. This code was added in 6fb5acfd8f (refs: add methods to init refs db - 2016-09-04). From the diff alone this looks like a faithful refactored code from init-db.c. But there is a subtle difference: Between the safe_create_dir() block and adjust_shared_perm() block in the old init-db.c, we may copy/recreate directories from the repo template. So it makes sense that adjust_shared_perm() is re-executed then to fix potential permission screwups. After 6fb5acfd8f, refs dirs are created after template is copied. Nobody will change directory permission again. So the extra adjust_shared_perm() is redudant. Delete them. Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 4 1 file changed, 4 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index caeb8188fd..9a9dc4e50c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -4107,10 +4107,6 @@ static int files_init_db(struct ref_store *ref_store, struct strbuf *err) */ safe_create_dir(git_path("refs/heads"), 1); safe_create_dir(git_path("refs/tags"), 1); - if (get_shared_repository()) { - adjust_shared_perm(git_path("refs/heads")); - adjust_shared_perm(git_path("refs/tags")); - } return 0; } -- 2.11.0.157.gd943d85
[PATCH v7 02/28] files-backend: make files_log_ref_write() static
Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10) but probably never used outside refs-internal.c Signed-off-by: Nguyễn Thái Ngọc Duy --- refs/files-backend.c | 9 ++--- refs/refs-internal.h | 4 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 50188e92f9..42c0cc14f3 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store, const char *dirname, size_t len, int incomplete); static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry); +static int files_log_ref_write(const char *refname, const unsigned char *old_sha1, + const unsigned char *new_sha1, const char *msg, + int flags, struct strbuf *err); static struct ref_dir *get_ref_dir(struct ref_entry *entry) { @@ -2831,9 +2834,9 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1, return 0; } -int files_log_ref_write(const char *refname, const unsigned char *old_sha1, - const unsigned char *new_sha1, const char *msg, - int flags, struct strbuf *err) +static int files_log_ref_write(const char *refname, const unsigned char *old_sha1, + const unsigned char *new_sha1, const char *msg, + int flags, struct strbuf *err) { int logfd, result; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index fa93c9a32e..f732473e1d 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -228,10 +228,6 @@ struct ref_transaction { enum ref_transaction_state state; }; -int files_log_ref_write(const char *refname, const unsigned char *old_sha1, - const unsigned char *new_sha1, const char *msg, - int flags, struct strbuf *err); - /* * Check for entries in extras that are within the specified * directory, where dirname is a reference directory name including -- 2.11.0.157.gd943d85
[PATCH v7 00/28] Remove submodule from files-backend.c
v7 is mostly about style changes except the one bug in test-ref-store.c, missing setup_git_directory(). There's one new patch, 03/28, which maps to the "if (!refs)" deletion in the interdiff. The one comment from v6 I haven't addressed in v7 is whether to delete REF_STORE_READ. But if it is deleted, I think I'm doing it in nd/worktree-kill-parse-ref, which is partly about cleanup refs code anyway, to avoid another re-roll in this series. diff --git a/refs.c b/refs.c index 77a39f8b17..ec1f563824 100644 --- a/refs.c +++ b/refs.c @@ -1523,18 +1523,15 @@ static struct ref_store *ref_store_init(const char *gitdir, struct ref_store *get_main_ref_store(void) { - struct ref_store *refs; - if (main_ref_store) return main_ref_store; - refs = ref_store_init(get_git_dir(), - (REF_STORE_READ | - REF_STORE_WRITE | - REF_STORE_ODB | - REF_STORE_MAIN)); - main_ref_store = refs; - return refs; + main_ref_store = ref_store_init(get_git_dir(), + (REF_STORE_READ | +REF_STORE_WRITE | +REF_STORE_ODB | +REF_STORE_MAIN)); + return main_ref_store; } /* diff --git a/refs/files-backend.c b/refs/files-backend.c index 4242486118..a5b405436f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1195,9 +1195,9 @@ static void files_reflog_path(struct files_ref_store *refs, } } -static void files_refname_path(struct files_ref_store *refs, - struct strbuf *sb, - const char *refname) +static void files_ref_path(struct files_ref_store *refs, + struct strbuf *sb, + const char *refname) { switch (ref_type(refname)) { case REF_TYPE_PER_WORKTREE: @@ -1283,7 +1283,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) struct strbuf path = STRBUF_INIT; size_t path_baselen; - files_refname_path(refs, &path, dirname); + files_ref_path(refs, &path, dirname); path_baselen = path.len; d = opendir(path.buf); @@ -1420,7 +1420,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, *type = 0; strbuf_reset(&sb_path); - files_refname_path(refs, &sb_path, refname); + files_ref_path(refs, &sb_path, refname); path = sb_path.buf; @@ -1608,7 +1608,7 @@ static int lock_raw_ref(struct files_ref_store *refs, *lock_p = lock = xcalloc(1, sizeof(*lock)); lock->ref_name = xstrdup(refname); - files_refname_path(refs, &ref_file, refname); + files_ref_path(refs, &ref_file, refname); retry: switch (safe_create_leading_directories(ref_file.buf)) { @@ -1949,8 +1949,6 @@ static struct ref_iterator *files_ref_iterator_begin( refs = files_downcast(ref_store, REF_STORE_READ | (ref_paranoia ? 0 : REF_STORE_ODB), "ref_iterator_begin"); - if (!refs) - return empty_ref_iterator_begin(); iter = xcalloc(1, sizeof(*iter)); ref_iterator = &iter->base; @@ -2086,7 +2084,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs, if (flags & REF_DELETING) resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; - files_refname_path(refs, &ref_file, refname); + files_ref_path(refs, &ref_file, refname); resolved = !!refs_resolve_ref_unsafe(&refs->base, refname, resolve_flags, lock->old_oid.hash, type); @@ -2387,7 +2385,7 @@ static void try_remove_empty_parents(struct files_ref_store *refs, strbuf_setlen(&buf, q - buf.buf); strbuf_reset(&sb); - files_refname_path(refs, &sb, buf.buf); + files_ref_path(refs, &sb, buf.buf); if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf)) flags &= ~REMOVE_EMPTY_PARENTS_REF; @@ -2709,7 +2707,7 @@ static int files_rename_ref(struct ref_store *ref_store, struct strbuf path = STRBUF_INIT; int result; - files_refname_path(refs, &path, newrefname); + files_ref_path(refs, &path, newrefname); result = remove_empty_directories(&path); strbuf_release(&path); @@ -2935,10 +2933,10 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1, return 0; } -int files_log_ref_write(struct files_ref_store *refs, - const char *refname, const unsigned char *old_sha1, - const un
[PATCH v7 01/28] refs.h: add forward declaration for structs used in this file
Signed-off-by: Nguyễn Thái Ngọc Duy --- refs.h | 4 1 file changed, 4 insertions(+) diff --git a/refs.h b/refs.h index 3df0d45ebb..2d6b6263fc 100644 --- a/refs.h +++ b/refs.h @@ -1,6 +1,10 @@ #ifndef REFS_H #define REFS_H +struct object_id; +struct strbuf; +struct string_list; + /* * Resolve a reference, recursively following symbolic refererences. * -- 2.11.0.157.gd943d85
[PATCH] notes: Fix note_tree_consolidate not to break the note_tree structure
After a note is removed, note_tree_consolidate is called to eliminate some useless nodes. The typical case is that if you had an int_node with 2 PTR_TYPE_NOTEs in it, and remove one of them, then the PTR_TYPE_INTERNAL pointer in the parent tree can be replaced with the remaining PTR_TYPE_NOTE. This works fine when PTR_TYPE_NOTEs are involved, but falls flat when other types are involved. To put things in more practical terms, let's say we start from an empty notes tree, and add 3 notes: - one for a sha1 that starts with 424 - one for a sha1 that starts with 428 - one for a sha1 that starts with 4c To keep track of this, note_tree.root will have a PTR_TYPE_INTERNAL at a[4], pointing to an int_node*. In turn, that int_node* will have a PTR_TYPE_NOTE at a[0xc], pointing to the leaf_node* with the key and value, and a PTR_TYPE_INTERNAL at a[2], pointing to another int_node*. That other int_node* will have 2 PTR_TYPE_NOTE, one at a[4] and the other at a[8]. When looking for the note for the sha1 starting with 428, get_note() will recurse through (simplified) root.a[4].a[2].a[8]. Now, if we remove the note for the sha1 that starts with 4c, we're left with a int_node* with only one PTR_TYPE_INTERNAL entry in it. After note_tree_consolidate runs, root.a[4] now points to what used to be pointed at by root.a[4].a[2]. Which means looking up for the note for the sha1 starting with 428 now fails because there is nothing at root.a[4].a[2] anymore: there is only root.a[4].a[4] and root.a[4].a[8], which don't match the expected structure for the lookup. So if all there is left in an int_node* is a PTR_TYPE_INTERNAL pointer, we can't safely remove it. I think the same applies for PTR_TYPE_SUBTREE pointers. IOW, only PTR_TYPE_NOTEs are safe to be moved to the parent int_node*. This doesn't have a practical effect on git because all that happens after a remove_note is a write_notes_tree, which just iterates the entire note tree, but this affects anything using libgit.a that would try to do lookups after removing notes. Signed-off-by: Mike Hommey --- notes.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/notes.c b/notes.c index 2bab961ac..542563b28 100644 --- a/notes.c +++ b/notes.c @@ -153,8 +153,8 @@ static struct leaf_node *note_tree_find(struct notes_tree *t, * How to consolidate an int_node: * If there are > 1 non-NULL entries, give up and return non-zero. * Otherwise replace the int_node at the given index in the given parent node - * with the only entry (or a NULL entry if no entries) from the given tree, - * and return 0. + * with the only NOTE entry (or a NULL entry if no entries) from the given + * tree, and return 0. */ static int note_tree_consolidate(struct int_node *tree, struct int_node *parent, unsigned char index) @@ -173,6 +173,8 @@ static int note_tree_consolidate(struct int_node *tree, } } + if (p && (GET_PTR_TYPE(p) != PTR_TYPE_NOTE)) + return -2; /* replace tree with p in parent[index] */ parent->a[index] = p; free(tree); -- 2.12.1.dirty
Re: [PATCH v6 18/27] files-backend: replace submodule_allowed check in files_downcast()
On Mon, Mar 20, 2017 at 4:18 AM, Michael Haggerty wrote: >> +/* ref_store_init flags */ >> +#define REF_STORE_READ (1 << 0) > > I asked [1] in reply to v5 whether `REF_STORE_READ` is really necessary > but I don't think you replied. Surely a reference store that we can't > read would be useless? Can't we just assume that any `ref_store` is > readable and drop this constant? I deleted it then I realized many lines like these files_downcast(ref_store, REF_STORE_READ, "read_raw_ref"); would become files_downcast(ref_store, 0, "read_raw_ref"); I think for newcomers, the former gives a better hint what the second argument is than a plain zero in the latter, so I'm inclined to keep it. -- Duy
Re: [PATCH/RFC] parse-options: add facility to make options configurable
On Sat, Mar 25, 2017 at 10:31 PM, Jeff King wrote: > On Sat, Mar 25, 2017 at 05:47:49PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> After looking at some of the internal APIs I'm thinking of replacing >> this pattern with a hashmap.c hashmap where the keys are a >> sprintf("%d:%s", short_name, long_name) to uniquely identify the >> option. There's no other obvious way to uniquely address an option. I >> guess I could just equivalently and more cheaply use the memory >> address of each option to identify them, but that seems a bit hacky. > > Rather than bolt this onto the parse-options code, what if it were a > separate mechanism that mapped config keys to options. E.g., imagine > something like: > > struct { > const char *config; > const char *option; > enum { > CONFIG_CMDLINE_BOOL, > CONFIG_CMDLINE_MAYBE_BOOL, > CONFIG_CMDLINE_VALUE > } type; > } config_cmdline_map[] = { > { "foo.one", "one", CONFIG_CMDLINE_BOOL }, > { "foo.two", "two", CONFIG_CMDLINE_VALUE }, > }; > > And then you "apply" that mapping by finding each item that's set in the > config, and then pretending that "--one" or "--two=" was set on > the command-line, before anything the user has said. This works as long > as the options use the normal last-one-wins rules, the user can > countermand with "--no-one", etc. > > You have to write one line for each config/option mapping, but I think > we would need some amount of per-option anyway (i.e., I think the > decision was that options would have to be manually approved for use in > the system). So rather than a flag in the options struct, it becomes a > line in this mapping. > > And you get two extra pieces of flexibility: > > 1. The config names can map to option names however makes sense; we're > not constrained by some programmatic rule (I think we _would_ > follow some general guidelines, but there are probably special > cases for historic config, etc). > > 2. A command can choose to apply one or more mappings, or not. So > porcelain like git-log would call: > >struct option options[] = {...}; >apply_config_cmdline_map(revision_config_mapping, options); >apply_config_cmdline_map(diff_config_mapping, options); >apply_config_cmdline_map(log_mapping, options); > > but plumbing like git-diff-tree wouldn't call any of those. > > I had in mind that apply_config_cmdline_map() would just call > parse_options, but I think even that is too constricting. The revision > and diff options don't use parse_options at all. So really, it would > probably be more like: > > struct argv_array fake_args = ARGV_ARRAY_INIT; > apply_config_cmdline_map(revision_config_mapping, &fake_args); > apply_config_cmdline_map(diff_config_mapping, &fake_args); > apply_config_cmdline_map(log_mapping, &fake_args); > argv_array_pushv(&fake_args, argv); /* add the real ones */ > > At this point we've recreated internally the related suggestion: > > [options] > log = --one --two=whatever > > which is the same as: > > [log] > one = true > two = whatever > > So hopefully it's clear that the two are functionally equivalent, and > differ only in syntax (in this case we manually decided which options > are safe to pull from the config, but we'd have to parse the options.log > string, too, and we could make the same decision there). I like the simplicity of this approach a lot. I.e. (to paraphrase it just to make sure we're on the same page): Skip all the complexity of reaching into the getopt guts, and just munge argc/argv given a config we can stick ahead of the getopt (struct options) spec, inject some options at the beginning if they're in the config, and off we go without any further changes to the getopt guts. There's two practical issues with this that are easy to solve with my current approach, but I can't find an easy solution to using this method. The first is that we're replacing the semantics of: "If you're specifying it on the command-line, we take it from there, otherwise we use your config, if set, regardless of how the option works" with: "We read your config, inject options implicitly at the start of the command line, and then append whatever command-line you give us" These two are not the same. Consider e.g. the commit.verbose config. With my current patch if have commit.verbose=1 in your config and do "commit --verbose" you just end up with a result equivalent to not having it in your config, but since the --verbose option can be supplied multiple times to increase verbosity with the injection method you'd end up with the equivalent of commit.verbose=2. I think the semantics I've implemented are much less confusing for the user, i.e. you can specify an option that's configurable and know that you're overriding your config, not potentially joining the command-line with whatever's in your config. We have a lot of options without last
EMAIL BENUTZER
-- Attn: Gewinner Ihre E-Mail-Adresse hat die Summe von 1,200.000.00 Euro gewonnen. Im Email Benützer Online Programm Weihnachtslotterie Navida , in Madrid Spanien . Wir schreiben ihnen ,um sich offiziel über die Auszeichnung zu Benachrichtigen , damit Sie sich mit der Zuständigen Vermittlerin in verbindung setzten können. Frau HEIDI MEIER, per E-Mail: heidi_mei...@aol.com Zur Bearbeitung und Freigabe Ihrer Zahlung per Scheck oder Banküberweisung. Bitte füllen Sie das untenstehende Formular aus. NAME: ___ FAMILIENNAME:_ ADRESSE:__ STADT: PLZ: LAND: ___ GEB: DATUM: __BERUF: FESTNETZ TEL.NR: MOBILETELEFON NR: ___ FAX: ___ EMAIL:___ DATE SIGNATURE:_ bitte füllen Sie das anschließende Formular vollständig aus und senden es per email zurück ! Hochachtungsvoll Francisco Blázquez Koordinator.
Re: Will OpenSSL's license change impact us?
On Sat, Mar 25, 2017 at 12:51:52AM +0100, Ævar Arnfjörð Bjarmason wrote: > They're changing their license[1] to Apache 2 which unlike the current > fuzzy compatibility with the current license[2] is explicitly > incompatible with GPLv2[3]. > > We use OpenSSL for SHA1 by default unless NO_OPENSSL=YesPlease. > > This still hasn't happened, but given the lifetime of git versions > packaged up by distros knowing sooner than later if this is going to > be a practical problem would be good. > > If so perhaps we could copy the relevant subset of the code int our > tree, or libressl's, or improve block-sha1. I think that most distros don't link against OpenSSL because they can't take advantage of the system library exception. I don't think that's going to change. If we want to consider performance-related concerns, I think the easier solution is using Nettle, which is LGPL 2.1. Considering that the current opinions for a new hash function are moving in the direction of SHA-3, which Nettle has, but OpenSSL does not, I think that might be a better decision overall. It was certainly the implementation I would use if I were to implement it. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: Will OpenSSL's license change impact us?
On Sat, Mar 25, 2017 at 10:44 PM, brian m. carlson wrote: > On Sat, Mar 25, 2017 at 12:51:52AM +0100, Ævar Arnfjörð Bjarmason wrote: >> They're changing their license[1] to Apache 2 which unlike the current >> fuzzy compatibility with the current license[2] is explicitly >> incompatible with GPLv2[3]. >> >> We use OpenSSL for SHA1 by default unless NO_OPENSSL=YesPlease. >> >> This still hasn't happened, but given the lifetime of git versions >> packaged up by distros knowing sooner than later if this is going to >> be a practical problem would be good. >> >> If so perhaps we could copy the relevant subset of the code int our >> tree, or libressl's, or improve block-sha1. > > I think that most distros don't link against OpenSSL because they can't > take advantage of the system library exception. I don't think that's > going to change. "ldd -r" against git itself on my Debian testing doesn't return libssl, but git-imap-send is dynamically linked to it: $ ldd -r /usr/lib/git-core/git-imap-send|grep ssl libssl.so.1.0.2 => /usr/lib/x86_64-linux-gnu/libssl.so.1.0.2 (0x7f244b2f6000) $ apt-cache show libssl1.0.2:amd64 Package: libssl1.0.2 Source: openssl1.0 Version: 1.0.2k-1 [...] Homepage: https://www.openssl.org > If we want to consider performance-related concerns, I think the easier > solution is using Nettle, which is LGPL 2.1. Considering that the > current opinions for a new hash function are moving in the direction of > SHA-3, which Nettle has, but OpenSSL does not, I think that might be a > better decision overall. It was certainly the implementation I would > use if I were to implement it. Yeah there's a lot of options open for just sha1-ing, but we also use OpenSSL for TLS via imap-send.
Re: Will OpenSSL's license change impact us?
On Sat, Mar 25, 2017 at 10:11 PM, Theodore Ts'o wrote: > On Sat, Mar 25, 2017 at 06:51:21PM +0100, Ęvar Arnfjörš Bjarmason wrote: >> In GPLv3 projects only, not GPLv2 projects. The paragraphs you're >> quoting all explicitly mention v3 only, so statements like >> "incompatible in one direction" only apply to Apache 2 && GPLv3, but >> don't at all apply to GPLv2, which is what we're using. > > It's complicated. > > It's fair enough to say that the FSF adopts a copyright maximalist > position, and by their interpretation, the two licenses are > incompatible, and it doesn't matter whether the two pieces of code are > linked staticaly, dynamically, or one calls the other over an RPC > call. > > Not everyone agrees with their legal analysis. May I suggest that we > not play amateur lawyer on the mailing list, and try to settle this > here? Each distribution can make its own decision, which may be based > on its legal advice, the local laws and legal precedents in which they > operate, etc. And indeed, different distributions have already come > to different conclusions with respect to various license compatibility > issues. (Examples: dynamically linking GPL programs with OpenSSL > libraries under the old license, distributing ZFS modules for Linux, > etc.) > > We don't expect lawyers to debug edge cases in a compiler's code > generation. Programmers shouldn't try to parse edge cases in the law, > or try to use a soldering iron, unless they have explicit training and > expertise to do so. :-) Yeah fully agree with the internet lawyering. I'm not looking for that, just seeing if someone knows if this might be an issue for at least some distros, then it's something for us to keep an eye on if OpenSSL's license changes, and a sane default for us to adopt might be to e.g. require that some flag be passed to the Makefile declaring "yes I'm OK with combining AL2 + GPLv2" if the OpenSSL version is newer than so-and-so.
Re: [PATCH/RFC] parse-options: add facility to make options configurable
On Sat, Mar 25, 2017 at 05:47:49PM +0100, Ævar Arnfjörð Bjarmason wrote: > After looking at some of the internal APIs I'm thinking of replacing > this pattern with a hashmap.c hashmap where the keys are a > sprintf("%d:%s", short_name, long_name) to uniquely identify the > option. There's no other obvious way to uniquely address an option. I > guess I could just equivalently and more cheaply use the memory > address of each option to identify them, but that seems a bit hacky. Rather than bolt this onto the parse-options code, what if it were a separate mechanism that mapped config keys to options. E.g., imagine something like: struct { const char *config; const char *option; enum { CONFIG_CMDLINE_BOOL, CONFIG_CMDLINE_MAYBE_BOOL, CONFIG_CMDLINE_VALUE } type; } config_cmdline_map[] = { { "foo.one", "one", CONFIG_CMDLINE_BOOL }, { "foo.two", "two", CONFIG_CMDLINE_VALUE }, }; And then you "apply" that mapping by finding each item that's set in the config, and then pretending that "--one" or "--two=" was set on the command-line, before anything the user has said. This works as long as the options use the normal last-one-wins rules, the user can countermand with "--no-one", etc. You have to write one line for each config/option mapping, but I think we would need some amount of per-option anyway (i.e., I think the decision was that options would have to be manually approved for use in the system). So rather than a flag in the options struct, it becomes a line in this mapping. And you get two extra pieces of flexibility: 1. The config names can map to option names however makes sense; we're not constrained by some programmatic rule (I think we _would_ follow some general guidelines, but there are probably special cases for historic config, etc). 2. A command can choose to apply one or more mappings, or not. So porcelain like git-log would call: struct option options[] = {...}; apply_config_cmdline_map(revision_config_mapping, options); apply_config_cmdline_map(diff_config_mapping, options); apply_config_cmdline_map(log_mapping, options); but plumbing like git-diff-tree wouldn't call any of those. I had in mind that apply_config_cmdline_map() would just call parse_options, but I think even that is too constricting. The revision and diff options don't use parse_options at all. So really, it would probably be more like: struct argv_array fake_args = ARGV_ARRAY_INIT; apply_config_cmdline_map(revision_config_mapping, &fake_args); apply_config_cmdline_map(diff_config_mapping, &fake_args); apply_config_cmdline_map(log_mapping, &fake_args); argv_array_pushv(&fake_args, argv); /* add the real ones */ At this point we've recreated internally the related suggestion: [options] log = --one --two=whatever which is the same as: [log] one = true two = whatever So hopefully it's clear that the two are functionally equivalent, and differ only in syntax (in this case we manually decided which options are safe to pull from the config, but we'd have to parse the options.log string, too, and we could make the same decision there). -Peff
Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks
On Fri, 2017-03-24 at 15:56 -0700, Junio C Hamano wrote: > diff --git a/diff.c b/diff.c > > index be11e4ef2b..2afecfb939 100644 > > --- a/diff.c > > +++ b/diff.c > > @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, > > unsigned int flags) > > s->size = xsize_t(st.st_size); > > if (!s->size) > > goto empty; > > - if (S_ISLNK(st.st_mode)) { > > + if (S_ISLNK(s->mode)) { > > This change is conceptually wrong. s->mode (often) comes from the > index but in this codepath, after finding that s->oid is not valid > or we want to read from the working tree instead (several lines > before this part), we are committed to read from the working tree > and check things with st.st_* fields, not s->mode, when we decide > what to do with the thing we find on the filesystem, no? Hmm, true. It just accidentally does the right thing because s->mode happens to always match the expectations of this code. I will pass on more information into diff_populate_filespec so an explicit check can be done here. > > @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, > > unsigned int flags) > > s->should_free = 1; > > return 0; > > } > > + if (S_ISLNK(st.st_mode)) { > > + stat(s->path, &st); > > + s->size = xsize_t(st.st_size); > > + } > > if (size_only) > > return 0; > > if ((flags & CHECK_BINARY) && > > I suspect that this would conflict with a recent topic. Possibly. I used the same base commit for the newer versions as that seems to be your preference. If there is a merge conflict, do you want me to rebase against current master? > But more importantly, this inserted code feels doubly wrong. > > - what allows us to unconditionally do "ah, symbolic link on the >disk--find the target of the link, not the symbolic link itself"? >We do not seem to be checking '--dereference' around here. The implicit check above (which you already noted is faulty) allows us to do this. So fixing the check above will also involve fixing this. > - does this code do a reasonable thing when the path is a symbolic >link that points at a directory? what does it mean to grab >st.st_size for such a thing (and then go on to open() and xmmap() >it)? No, it does something entirely unreasonable. I hadn't even thought of testing with symlinks to directories, as my ulterior motive was the next commit that makes it work with pipes. This will be fixed. Thanks very much for the thoroughness of your review! D.
Re: [PATCH/RFC] parse-options: add facility to make options configurable
On Fri, Mar 24, 2017 at 11:10:13PM +, Ævar Arnfjörð Bjarmason wrote: > On Sun, Mar 19, 2017 at 2:43 PM, Ævar Arnfjörð Bjarmason > wrote: > > I don't know if this is what Duy has in mind, but the facility I've > > described is purely an internal code reorganization issue. I.e. us > > not having to write custom code for each bultin every time we want to > > take an option from the command line || config. > > Here's an implementation of this I hacked up this evening. This is > very WIP as noted in the commit message / TODO comments, but it works! > I thought I'd send it to the list for comments on the general approach > before taking it much further. For what it's worth, I think this is a good design. It makes it easy to add options when needed, but it doesn't override the defaults for the entire command, which was my concern. The potential for removing a decent amount of likely duplicative code also makes me happy. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH] pretty: add extra headers and MIME boundary directly
On Sat, Mar 25, 2017 at 05:56:55PM +0100, René Scharfe wrote: > Am 25.03.2017 um 17:17 schrieb Jeff King: > > On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote: > > > @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, > > > struct commit *commit, > > > graph_show_oneline(opt->graph); > > > } > > > if (opt->mime_boundary) { > > > - static char subject_buffer[1024]; > > > static char buffer[1024]; > > > > We still have this other buffer, which ends up in stat_sep. It should > > probably get the same treatment, though I think the module boundaries > > make it a little more awkward. We look at it in diff_flush(), which > > otherwise doesn't need to know much about the pretty-printing. > > > > Perhaps stat_sep should be a callback? > > Yes, it would be nice to avoid it, but I haven't found a clean way, yet. In > diff.c, where it's used, we don't have commit and rev_info available (which > we'd have to pass to a callback, or consume right there), and that's > probably how it should be. Perhaps preparing the filename in advance and > passing that as a string together with mime_boundary and no_inline might be > the way to go. I think the no-allocation way with a callback would be something like the patch at the end of this email. Having to stuff the commit into rev_info is horrible, because rev_info shouldn't be carrying data specific to individual commits. But it's really no different than what's there now, which is stuffing a filled-out commit-specific buffer into the same struct. Doing it with a single allocated buffer would involve less callback rigmarole, but it doesn't change the fact that we are stuffing commit-specific data into the rev_info (via the diff_options member). And then you add on top the question of who is supposed to free it (which is punted on in the original by using a static; yech). The most correct way is that the caller of log_write_email_headers() and diff_flush() should have a function-local strbuf which holds the data, gets passed to diff_flush() as some kind opaque context, and then is freed afterwards. We don't have such a context, but if we were to abuse diff_options.stat_sep _temporarily_, that would still be a lot cleaner. I.e., something like this: struct strbuf stat_sep = STRBUF_INIT; /* may write into stat_sep, depending on options */ log_write_email_headers(..., &stat_sep); opt->diffopt.stat_sep = stat_sep.buf; diff_flush(&opt->diffopt); opt->diffopt.stat_sep = NULL; strbuf_release(&stat_sep); But it's a bit tricky because those two hunks happen in separate functions, which means passing the strbuf around. > > > + if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) { > > > + strbuf_addf(sb, > > > + "MIME-Version: 1.0\n" > > > > In the original, this would have been in "after_subject". Which means we > > would print it even if print_email_subject is not true. Why do we need > > to check it in the new conditional? > > No, we only would have printed it if log_write_email_headers() was called to > append it to the static buffer. print_email_subject is only set when we > call log_write_email_headers(), so checking it makes sure that we get the > same behavior as before. OK. So your logic is "you would always set print_email_subject alongside log_write_email_headers()", and mine is "you would always call log_write_email_headers() when opt->mime_boundary is set". Both are true today, and I do not see a big chance of them becoming untrue. So I'm OK with it either way. > > Not that I expect the behavior to be wrong either way; why would we have > > a mime boundary without setting print_email_subject? But I would think > > that "do we have a mime boundary" would be the right conditional to > > trigger printing it. > > FWIW, the test suite still passes with the print_email_subject check > removed. And currently only cmd_format_patch() sets mime_boundary, so we > don't need the check indeed. Yeah, while working on the other patch I looked at all the callers (there are only 2), and I think it's fine. I thought at first there might be problems with: git format-patch --attach --format=not-email:%s but we skip this code when it's a non-email format (and anyway, format-patch always sets print_email_subject whether the format is email or not). That command _does_ generate nonsense, because cmd_format_patch() insists on showing the trailing mime boundary even if the format is not email. I'm not sure there's a good reason to use a non-email format with format-patch in the first place, let alone with --attach. Arguably it should bail as soon as it sees the incompatible options. Anyway. Here's my attempt at the callback version of stat_sep. --- diff --git a/diff.c b/diff.c index a628ac3a9..d061f9e18 100644 --- a/diff.c +++ b/diff.c @@ -4819,10 +4819,9 @@ void diff_flush(struct diff_options *options) fprintf(options->file, "%s%
Re: [PATCH] bisect: add quit command
I found out about "git bisect reset HEAD" while working on "git bisect quit" but I think it's still worth it. Let me know. On Sat, Mar 25, 2017 at 3:17 PM, Edmundo Carmona Antoranz wrote: > git bisect quit will call git reset HEAD so that the working tree > remains at the current revision > --- > Documentation/git-bisect.txt | 12 > git-bisect.sh| 11 ++- > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt > index bdd915a66..de79e9e44 100644 > --- a/Documentation/git-bisect.txt > +++ b/Documentation/git-bisect.txt > @@ -23,6 +23,7 @@ on the subcommand: > git bisect terms [--term-good | --term-bad] > git bisect skip [(|)...] > git bisect reset [] > + git bisect quit > git bisect visualize > git bisect replay > git bisect log > @@ -120,6 +121,17 @@ bad revision, while `git bisect reset HEAD` will leave > you on the > current bisection commit and avoid switching commits at all. > > > +Bisect quit > +~~~ > + > +It's an equivalent of bisect reset but that stays at the current > +revision instead of taking your working tree to the starting revision. > + > + > +$ git bisect quit > + > + > + > Alternate terms > ~~~ > > diff --git a/git-bisect.sh b/git-bisect.sh > index ae3cb013e..adbff2c69 100755 > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -1,6 +1,6 @@ > #!/bin/sh > > -USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]' > +USAGE='[help|start|bad|good|new|old|terms|skip|next|quit|reset|visualize|replay|log|run]' > LONG_USAGE='git bisect help > print this long help message. > git bisect start [--term-{old,good}= --term-{new,bad}=] > @@ -20,6 +20,9 @@ git bisect next > find next bisection to test and check it out. > git bisect reset [] > finish bisection search and go back to commit. > +git bisect quit > + stop bisection on its tracks and stay on current revision. > + Equivalent to git bisect reset HEAD > git bisect visualize > show bisect status in gitk. > git bisect replay > @@ -433,6 +436,10 @@ Try 'git bisect reset '.")" > bisect_clean_state > } > > +bisect_quit() { > + bisect_reset "HEAD" > +} > + > bisect_clean_state() { > # There may be some refs packed during bisection. > git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* | > @@ -683,6 +690,8 @@ case "$#" in > bisect_visualize "$@" ;; > reset) > bisect_reset "$@" ;; > + quit) > + bisect_quit ;; > replay) > bisect_replay "$@" ;; > log) > -- > 2.11.0.rc1 >
[PATCH] bisect: add quit command
git bisect quit will call git reset HEAD so that the working tree remains at the current revision --- Documentation/git-bisect.txt | 12 git-bisect.sh| 11 ++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt index bdd915a66..de79e9e44 100644 --- a/Documentation/git-bisect.txt +++ b/Documentation/git-bisect.txt @@ -23,6 +23,7 @@ on the subcommand: git bisect terms [--term-good | --term-bad] git bisect skip [(|)...] git bisect reset [] + git bisect quit git bisect visualize git bisect replay git bisect log @@ -120,6 +121,17 @@ bad revision, while `git bisect reset HEAD` will leave you on the current bisection commit and avoid switching commits at all. +Bisect quit +~~~ + +It's an equivalent of bisect reset but that stays at the current +revision instead of taking your working tree to the starting revision. + + +$ git bisect quit + + + Alternate terms ~~~ diff --git a/git-bisect.sh b/git-bisect.sh index ae3cb013e..adbff2c69 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -1,6 +1,6 @@ #!/bin/sh -USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]' +USAGE='[help|start|bad|good|new|old|terms|skip|next|quit|reset|visualize|replay|log|run]' LONG_USAGE='git bisect help print this long help message. git bisect start [--term-{old,good}= --term-{new,bad}=] @@ -20,6 +20,9 @@ git bisect next find next bisection to test and check it out. git bisect reset [] finish bisection search and go back to commit. +git bisect quit + stop bisection on its tracks and stay on current revision. + Equivalent to git bisect reset HEAD git bisect visualize show bisect status in gitk. git bisect replay @@ -433,6 +436,10 @@ Try 'git bisect reset '.")" bisect_clean_state } +bisect_quit() { + bisect_reset "HEAD" +} + bisect_clean_state() { # There may be some refs packed during bisection. git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* | @@ -683,6 +690,8 @@ case "$#" in bisect_visualize "$@" ;; reset) bisect_reset "$@" ;; + quit) + bisect_quit ;; replay) bisect_replay "$@" ;; log) -- 2.11.0.rc1
Re: Will OpenSSL's license change impact us?
On Sat, Mar 25, 2017 at 06:51:21PM +0100, Ævar Arnfjörð Bjarmason wrote: > In GPLv3 projects only, not GPLv2 projects. The paragraphs you're > quoting all explicitly mention v3 only, so statements like > "incompatible in one direction" only apply to Apache 2 && GPLv3, but > don't at all apply to GPLv2, which is what we're using. It's complicated. It's fair enough to say that the FSF adopts a copyright maximalist position, and by their interpretation, the two licenses are incompatible, and it doesn't matter whether the two pieces of code are linked staticaly, dynamically, or one calls the other over an RPC call. Not everyone agrees with their legal analysis. May I suggest that we not play amateur lawyer on the mailing list, and try to settle this here? Each distribution can make its own decision, which may be based on its legal advice, the local laws and legal precedents in which they operate, etc. And indeed, different distributions have already come to different conclusions with respect to various license compatibility issues. (Examples: dynamically linking GPL programs with OpenSSL libraries under the old license, distributing ZFS modules for Linux, etc.) We don't expect lawyers to debug edge cases in a compiler's code generation. Programmers shouldn't try to parse edge cases in the law, or try to use a soldering iron, unless they have explicit training and expertise to do so. :-) - Ted
Re: [PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL
On Sat, Mar 25, 2017 at 12:24 AM, Johannes Schindelin wrote: > The collision detection is not for free, though: when using the SHA1DC > code, calculating the SHA-1 takes substantially longer than using > OpenSSL's (in some case hardware-accelerated) SHA1-routines, and this > happens even when switching off the collision detection in SHA1DC's code. [...]in some case*s*[...]
Re: t1503 broken ?
On 03/25/2017 02:05 PM, Duy Nguyen wrote: On Sat, Mar 25, 2017 at 07:26:14PM +0700, Duy Nguyen wrote: On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen wrote: On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen wrote: ./t1305-config-include.sh seems to be broken: not ok 19 - conditional include, $HOME expansion not ok 21 - conditional include, relative path let me guess, your "git" directory is in a symlink path? Yes I could reproduce it when I put my "git" in a symlink. There's a note in document about "Symlinks in `$GIT_DIR` are not resolved before matching" but failing tests is not acceptable. I'll fix it. The fix may be something like this. The problem is $GIT_DIR has symlinks resolved, but we don't do the same for other paths in this code. As a result, matching paths fails. I'm a bit concerned about the change in expand_user_path() because I'm not quite sure if it's a completely safe change. But at least could you try the patch and see if it passe the tests on your machine too? -- 8< -- diff --git a/config.c b/config.c index 1a4d855..fc4eae9 100644 --- a/config.c +++ b/config.c @@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat) return error(_("relative config include " "conditionals must come from files")); - strbuf_add_absolute_path(&path, cf->path); + strbuf_realpath(&path, cf->path, 1); slash = find_last_dir_sep(path.buf); if (!slash) die("BUG: how is this possible?"); @@ -213,7 +213,7 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) struct strbuf pattern = STRBUF_INIT; int ret = 0, prefix; - strbuf_add_absolute_path(&text, get_git_dir()); + strbuf_realpath(&text, get_git_dir(), 1); strbuf_add(&pattern, cond, cond_len); prefix = prepare_include_condition_pattern(&pattern); diff --git a/path.c b/path.c index 2224843..18eaac3 100644 --- a/path.c +++ b/path.c @@ -654,7 +654,7 @@ char *expand_user_path(const char *path) const char *home = getenv("HOME"); if (!home) goto return_null; - strbuf_addstr(&user_path, home); + strbuf_addstr(&user_path, real_path(home)); #ifdef GIT_WINDOWS_NATIVE convert_slashes(user_path.buf); #endif -- 8< -- Thanks for the fast reply. Yes, my path is a softlink - into a directory under NoBackup/ - to make the backup shorter. And I had forgotten about this :-( And yes, your patch fixes it- tested under Linux.
[PATCH v3 1/2] [GSoC] dir_iterator: iterate over dir after its contents
Create an option for the dir_iterator API to iterate over a directory path only after having iterated through its contents. This feature was predicted, although not implemented by 0fe5043 ("dir_iterator: new API for iterating over a directory tree", 2016-06-18). This is useful for recursively removing a directory and calling rmdir() on a directory only after all of its contents have been wiped. An "options" member has been added to the dir_iterator struct. It contains the "iterate_dirs_after_files" flag, that enables the feature when set to 1. Default behavior continues to be iterating over directory paths before its contents. Two inline functions have been added to dir_iterator's code to avoid code repetition inside dir_iterator_advance() and make code more clear. No particular functions or wrappers for setting the options struct's fields have been added to avoid either breaking the current dir_iterator API or over-engineering an extremely simple option architecture. Signed-off-by: Daniel Ferreira --- dir-iterator.c | 100 - dir-iterator.h | 7 2 files changed, 84 insertions(+), 23 deletions(-) diff --git a/dir-iterator.c b/dir-iterator.c index 34182a9..833d56a 100644 --- a/dir-iterator.c +++ b/dir-iterator.c @@ -50,6 +50,43 @@ struct dir_iterator_int { struct dir_iterator_level *levels; }; +static inline void push_dir_level(struct dir_iterator_int *iter, struct dir_iterator_level *level) +{ + level->dir_state = DIR_STATE_RECURSE; + ALLOC_GROW(iter->levels, iter->levels_nr + 1, + iter->levels_alloc); + level = &iter->levels[iter->levels_nr++]; + level->initialized = 0; +} + +static inline int pop_dir_level(struct dir_iterator_int *iter, struct dir_iterator_level *level) +{ + return --iter->levels_nr; +} + +static inline int set_iterator_data(struct dir_iterator_int *iter, struct dir_iterator_level *level) +{ + if (lstat(iter->base.path.buf, &iter->base.st) < 0) { + if (errno != ENOENT) + warning("error reading path '%s': %s", + iter->base.path.buf, + strerror(errno)); + return -1; + } + + /* +* We have to set these each time because +* the path strbuf might have been realloc()ed. +*/ + iter->base.relative_path = + iter->base.path.buf + iter->levels[0].prefix_len; + iter->base.basename = + iter->base.path.buf + level->prefix_len; + level->dir_state = DIR_STATE_ITER; + + return 0; +} + int dir_iterator_advance(struct dir_iterator *dir_iterator) { struct dir_iterator_int *iter = @@ -77,18 +114,16 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator) } level->initialized = 1; - } else if (S_ISDIR(iter->base.st.st_mode)) { + } else if (S_ISDIR(iter->base.st.st_mode) && + !iter->base.options.iterate_dirs_after_files) { if (level->dir_state == DIR_STATE_ITER) { /* * The directory was just iterated * over; now prepare to iterate into -* it. +* it (unless an option is set for us +* to do otherwise). */ - level->dir_state = DIR_STATE_RECURSE; - ALLOC_GROW(iter->levels, iter->levels_nr + 1, - iter->levels_alloc); - level = &iter->levels[iter->levels_nr++]; - level->initialized = 0; + push_dir_level(iter, level); continue; } else { /* @@ -104,7 +139,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator) * This level is exhausted (or wasn't opened * successfully); pop up a level. */ - if (--iter->levels_nr == 0) + if (pop_dir_level(iter, level) == 0) return dir_iterator_abort(dir_iterator); continue; @@ -120,16 +155,33 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator) de = readdir(level->dir); if (!de) { - /* This level is exhausted; pop up a level. */ + /* This level is exhausted */ if (errno) { warning("error reading directory %s: %s", i
[PATCH v3 2/2] [GSoC] remove_subtree(): reimplement using iterators
Use dir_iterator to traverse through remove_subtree()'s directory tree, avoiding the need for recursive calls to readdir(). Simplify remove_subtree()'s code. A conversion similar in purpose was previously done at 46d092a ("for_each_reflog(): reimplement using iterators", 2016-05-21). Signed-off-by: Daniel Ferreira --- entry.c | 32 +++- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/entry.c b/entry.c index c6eea24..670ffeb 100644 --- a/entry.c +++ b/entry.c @@ -2,6 +2,8 @@ #include "blob.h" #include "dir.h" #include "streaming.h" +#include "iterator.h" +#include "dir-iterator.h" static void create_directories(const char *path, int path_len, const struct checkout *state) @@ -46,29 +48,17 @@ static void create_directories(const char *path, int path_len, static void remove_subtree(struct strbuf *path) { - DIR *dir = opendir(path->buf); - struct dirent *de; - int origlen = path->len; - - if (!dir) - die_errno("cannot opendir '%s'", path->buf); - while ((de = readdir(dir)) != NULL) { - struct stat st; - - if (is_dot_or_dotdot(de->d_name)) - continue; - - strbuf_addch(path, '/'); - strbuf_addstr(path, de->d_name); - if (lstat(path->buf, &st)) - die_errno("cannot lstat '%s'", path->buf); - if (S_ISDIR(st.st_mode)) - remove_subtree(path); - else if (unlink(path->buf)) + struct dir_iterator *diter = dir_iterator_begin(path->buf); + diter->options.iterate_dirs_after_files = 1; + + while (dir_iterator_advance(diter) == ITER_OK) { + if (S_ISDIR(diter->st.st_mode)) { + if (rmdir(diter->path.buf)) + die_errno("cannot rmdir '%s'", path->buf); + } else if (unlink(diter->path.buf)) die_errno("cannot unlink '%s'", path->buf); - strbuf_setlen(path, origlen); } - closedir(dir); + if (rmdir(path->buf)) die_errno("cannot rmdir '%s'", path->buf); } -- 2.7.4 (Apple Git-66)
[PATCH v3 0/2] [GSoC] remove_subtree(): reimplement using iterators
This is the third version of the GSoC microproject of refactoring remove_subtree() from recursively using readdir() to use dir_iterator. Below are the threads for other versions: v1: https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730 v2: https://public-inbox.org/git/cacsjy8dxh-qpbblfafwpawusba9gvxa7x+mxljevykhk1zo...@mail.gmail.com/T/#t Duy suggested adding features to dir_iterator might go beyond the intention of a microproject, but I figured I might go for it to learn more about the project. The dir_iterator reimplementation has been tested in a separate binary I created (and linked with libgit.a) to reproduce remove_subtree()'s contents. As pointed out in the last thread, git's tests for this function were unable to catch a daunting bug I had introduced, and I still haven't been able to come up with a way to reproduce remove_subtree() being called. Any help? Thank you all again for all the reviews. Daniel Ferreira (2): dir_iterator: iterate over dir after its contents remove_subtree(): reimplement using iterators dir-iterator.c | 100 - dir-iterator.h | 7 entry.c| 32 +++--- 3 files changed, 95 insertions(+), 44 deletions(-) -- 2.7.4 (Apple Git-66)
Re: Will OpenSSL's license change impact us?
On Sat, Mar 25, 2017 at 5:57 PM, demerphq wrote: > On 25 March 2017 at 17:35, Ævar Arnfjörð Bjarmason wrote: >> On Sat, Mar 25, 2017 at 10:43 AM, demerphq wrote: >>> >>> >>> On 25 Mar 2017 10:18 a.m., "Ævar Arnfjörð Bjarmason" >>> wrote: >>> >>> On Sat, Mar 25, 2017 at 9:40 AM, demerphq wrote: On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason wrote: > They're changing their license[1] to Apache 2 which unlike the current > fuzzy compatibility with the current license[2] is explicitly > incompatible with GPLv2[3]. Are you sure there is an issue? From the Apache page on this: Apache 2 software can therefore be included in GPLv3 projects, because the GPLv3 license accepts our software into GPLv3 works. However, GPLv3 software cannot be included in Apache projects. The licenses are incompatible in one direction only, and it is a result of ASF's licensing philosophy and the GPLv3 authors' interpretation of copyright law. Which seems to be the opposite of the concern you are expressing. >>> >>> The Apache 2 license is indeed compatible with the GPLv3, but the Git >>> project explicitly uses GPLv2 with no "or later" clause >>> >>> >>> Read the paragraph immediately (I think) after the one I quoted where they >>> state the situation is the same with GPL v2. >> >> My understanding of that paragraph is that it's still laying out >> caveats about exactly how GPLv3 is compatible with Apache 2, when it >> is, when it isn't etc. But then it goes on to say: >> >> """ >> Despite our best efforts, the FSF has never considered the Apache >> License to be compatible with GPL version 2, citing the patent >> termination and indemnification provisions as restrictions not present >> in the older GPL license. The Apache Software Foundation believes that >> you should always try to obey the constraints expressed by the >> copyright holder when redistributing their work. >> """ >> >> So they're just deferring to the FSF saying it's incompatible, the >> FSF's statement: >> https://www.gnu.org/licenses/license-list.html#apache2 "this license >> is not compatible with GPL version 2". >> >> Anyway, I'm not a lawyer. Just thought I'd send some E-Mail about this >> since I noticed it, if it's an issue (and we could e.g. get the SFC to >> comment, Jeff?) we might need to add e.g. some checks / macros to >> ensure we're not compiling against an incompatible OpenSSL. > > Just for the record this what Apache says, with the part I was > referring to earlier in slash style italics, and a couple of a key > points in star style bold: > > quote > Apache 2 software *can therefore be included in GPLv3 projects*, > because the GPLv3 license accepts our software into GPLv3 works. > However, GPLv3 software cannot be included in Apache projects. *The > licenses are incompatible in one direction only*, and it is a result > of ASF's licensing philosophy and the GPLv3 authors' interpretation of > copyright law. > > This licensing incompatibility applies only when some Apache project > software becomes a derivative work of some GPLv3 software, because > then the Apache software would have to be distributed under GPLv3. > This would be incompatible with ASF's requirement that all Apache > software must be distributed under the Apache License 2.0. > > We avoid GPLv3 software because merely linking to it is considered by > the GPLv3 authors to create a derivative work. We want to honor their > license. Unless GPLv3 licensors relax this interpretation of their own > license regarding linking, our licensing philosophies are > fundamentally incompatible. /This is an identical issue for both GPLv2 > and GPLv3./ > quote > > I read that as saying that you can use Apache 2 code in GPL projects, > but you can't use GPL code in Apache projects. Which makes sense as > Apache 2 is more liberal than GPL. In GPLv3 projects only, not GPLv2 projects. The paragraphs you're quoting all explicitly mention v3 only, so statements like "incompatible in one direction" only apply to Apache 2 && GPLv3, but don't at all apply to GPLv2, which is what we're using.
Re: [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN
Junio C Hamano writes: > Which leads me to wonder if a more robust solution that is in line > with the original design of sha1dc/sha1.c code may be to do an > unconditional "#undef BIGENDIAN" before the above block, so that no > matter what the calling environment sets BIGENDIAN to (including > "0"), it gets ignored and we always use the auto-selection. So here is what I came up with as a replacement (this time as a proper patch not a comment on a patch). Dscho, could you see if this fixes your build? Also, could people on big-endian boxes see if this breaks your setting (relative to the state before this patch)? Thanks. -- >8 -- From: Junio C Hamano Date: Sat, 25 Mar 2017 10:05:13 -0700 Subject: [PATCH] sha1dc: avoid CPP macro collisions In an early part of sha1dc/sha1.c, the code checks the endianness of the target platform by inspecting common CPP macros defined on big-endian boxes, and sets BIGENDIAN macro to 1. If these common CPP macros are not defined, the code declares that the target platform is little endian and does nothing (most notably, it does not #undef its BIGENDIAN macro). The code that does so even has this comment Note that all MSFT platforms are little endian, so none of these will be defined under the MSC compiler. and later, the defined-ness of the BIGENDIAN macro is used to switch the implementation of sha1_load() macro. One thing the code did not anticipate is that somebody might define BIGENDIAN macro in some header it includes to 0 on a little-endian target platform. Because the auto-detection based on common macros do not touch BIGENDIAN macro when it detects a little-endian target, such a definition is still valid and then defined-ness test will say "Ah, BIGENDIAN is defined" and takes the wrong sha1_load(). As this auto-detection logic pretends as if it owns the BIGENDIAN macro by ignoring the setting that may come from the outside and by not explicitly unsetting when it decides that it is working for a little-endian target, solve this problem without breaking that assumption. Namely, we can rename BIGENDIAN this code uses to something much less generic, i.e. SHA1DC_BIGENDIAN. For extra protection, undef the macro on a little-endian target. It is possible to work it around by instead #undef BIGENDIAN in the auto-detection code, but a macro (or include) that happens later in the code can be implemented in terms of BIGENDIAN on Windows and it is possible that the implementation gets upset when it sees the CPP macro undef'ed (instead of set to 0). Renaming the private macro intended to be used only in this file to a less generic name relieves us from having to worry about that kind of breakage. Noticed-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- sha1dc/sha1.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c index 6dd0da3608..35e9dd5bf4 100644 --- a/sha1dc/sha1.c +++ b/sha1dc/sha1.c @@ -12,7 +12,7 @@ /* Because Little-Endian architectures are most common, - we only set BIGENDIAN if one of these conditions is met. + we only set SHA1DC_BIGENDIAN if one of these conditions is met. Note that all MSFT platforms are little endian, so none of these will be defined under the MSC compiler. If you are compiling on a big endian platform and your compiler does not define one of these, @@ -23,8 +23,9 @@ defined(__BIG_ENDIAN__) || defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \ defined(_MIPSEB) || defined(__MIPSEB) || defined(__MIPSEB__) -#define BIGENDIAN (1) - +#define SHA1DC_BIGENDIAN 1 +#else +#undef SHA1DC_BIGENDIAN #endif /*ENDIANNESS SELECTION*/ #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n @@ -35,11 +36,11 @@ #define sha1_mix(W, t) (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 16], 1)) -#if defined(BIGENDIAN) +#if defined(SHA1DC_BIGENDIAN) #define sha1_load(m, t, temp) { temp = m[t]; } #else #define sha1_load(m, t, temp) { temp = m[t]; sha1_bswap32(temp); } -#endif /*define(BIGENDIAN)*/ +#endif /* !defined(SHA1DC_BIGENDIAN) */ #define sha1_store(W, t, x)*(volatile uint32_t *)&W[t] = x -- 2.12.2-399-g034667a458
Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag
Junio C Hamano writes: > The hash that names a packfile is constructed by sorting all the > names of the objects contained in the packfile and running SHA-1 > hash over it. Sorry, but I need to make a correction here. This "SHA-1 over sorted object names" is a description of an ancient behaviour before 1190a1ac ("pack-objects: name pack files after trailer hash", 2013-12-05) happened. These days the pack name is the same as the csum-file checksum of the .pack contents. This however does not change the fact that the site that feeds us a packfile is in control of the hash, hence the name we give to the resulting packfile. Unlike the use of csum-file for the trailing hash for the index file, which is only to protect against bit flipping, "SHA-1 over .pack contents" done here is used to come up with a unique name used for identification and deduplication (of the packfile, not of individual objects), and the need for protection against collision attack attempts does not change between the implementation before 1190a1ac and after that commit.
Re: Will OpenSSL's license change impact us?
On 25 March 2017 at 17:35, Ævar Arnfjörð Bjarmason wrote: > On Sat, Mar 25, 2017 at 10:43 AM, demerphq wrote: >> >> >> On 25 Mar 2017 10:18 a.m., "Ævar Arnfjörð Bjarmason" >> wrote: >> >> On Sat, Mar 25, 2017 at 9:40 AM, demerphq wrote: >>> On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason >>> wrote: They're changing their license[1] to Apache 2 which unlike the current fuzzy compatibility with the current license[2] is explicitly incompatible with GPLv2[3]. >>> >>> Are you sure there is an issue? From the Apache page on this: >>> >>> Apache 2 software can therefore be included in GPLv3 projects, because >>> the GPLv3 license accepts our software into GPLv3 works. However, >>> GPLv3 software cannot be included in Apache projects. The licenses are >>> incompatible in one direction only, and it is a result of ASF's >>> licensing philosophy and the GPLv3 authors' interpretation of >>> copyright law. >>> >>> Which seems to be the opposite of the concern you are expressing. >> >> The Apache 2 license is indeed compatible with the GPLv3, but the Git >> project explicitly uses GPLv2 with no "or later" clause >> >> >> Read the paragraph immediately (I think) after the one I quoted where they >> state the situation is the same with GPL v2. > > My understanding of that paragraph is that it's still laying out > caveats about exactly how GPLv3 is compatible with Apache 2, when it > is, when it isn't etc. But then it goes on to say: > > """ > Despite our best efforts, the FSF has never considered the Apache > License to be compatible with GPL version 2, citing the patent > termination and indemnification provisions as restrictions not present > in the older GPL license. The Apache Software Foundation believes that > you should always try to obey the constraints expressed by the > copyright holder when redistributing their work. > """ > > So they're just deferring to the FSF saying it's incompatible, the > FSF's statement: > https://www.gnu.org/licenses/license-list.html#apache2 "this license > is not compatible with GPL version 2". > > Anyway, I'm not a lawyer. Just thought I'd send some E-Mail about this > since I noticed it, if it's an issue (and we could e.g. get the SFC to > comment, Jeff?) we might need to add e.g. some checks / macros to > ensure we're not compiling against an incompatible OpenSSL. Just for the record this what Apache says, with the part I was referring to earlier in slash style italics, and a couple of a key points in star style bold: quote Apache 2 software *can therefore be included in GPLv3 projects*, because the GPLv3 license accepts our software into GPLv3 works. However, GPLv3 software cannot be included in Apache projects. *The licenses are incompatible in one direction only*, and it is a result of ASF's licensing philosophy and the GPLv3 authors' interpretation of copyright law. This licensing incompatibility applies only when some Apache project software becomes a derivative work of some GPLv3 software, because then the Apache software would have to be distributed under GPLv3. This would be incompatible with ASF's requirement that all Apache software must be distributed under the Apache License 2.0. We avoid GPLv3 software because merely linking to it is considered by the GPLv3 authors to create a derivative work. We want to honor their license. Unless GPLv3 licensors relax this interpretation of their own license regarding linking, our licensing philosophies are fundamentally incompatible. /This is an identical issue for both GPLv2 and GPLv3./ quote I read that as saying that you can use Apache 2 code in GPL projects, but you can't use GPL code in Apache projects. Which makes sense as Apache 2 is more liberal than GPL. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Re: [PATCH] pretty: add extra headers and MIME boundary directly
Am 25.03.2017 um 17:17 schrieb Jeff King: On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote: @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, graph_show_oneline(opt->graph); } if (opt->mime_boundary) { - static char subject_buffer[1024]; static char buffer[1024]; We still have this other buffer, which ends up in stat_sep. It should probably get the same treatment, though I think the module boundaries make it a little more awkward. We look at it in diff_flush(), which otherwise doesn't need to know much about the pretty-printing. Perhaps stat_sep should be a callback? Yes, it would be nice to avoid it, but I haven't found a clean way, yet. In diff.c, where it's used, we don't have commit and rev_info available (which we'd have to pass to a callback, or consume right there), and that's probably how it should be. Perhaps preparing the filename in advance and passing that as a string together with mime_boundary and no_inline might be the way to go. diff --git a/pretty.c b/pretty.c index d0f86f5d85..56e668781a 100644 --- a/pretty.c +++ b/pretty.c @@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp, if (pp->after_subject) { strbuf_addstr(sb, pp->after_subject); } + if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) { + strbuf_addf(sb, + "MIME-Version: 1.0\n" In the original, this would have been in "after_subject". Which means we would print it even if print_email_subject is not true. Why do we need to check it in the new conditional? No, we only would have printed it if log_write_email_headers() was called to append it to the static buffer. print_email_subject is only set when we call log_write_email_headers(), so checking it makes sure that we get the same behavior as before. Not that I expect the behavior to be wrong either way; why would we have a mime boundary without setting print_email_subject? But I would think that "do we have a mime boundary" would be the right conditional to trigger printing it. FWIW, the test suite still passes with the print_email_subject check removed. And currently only cmd_format_patch() sets mime_boundary, so we don't need the check indeed. René
Re: [PATCH/RFC] parse-options: add facility to make options configurable
On Sat, Mar 25, 2017 at 12:10 AM, Ævar Arnfjörð Bjarmason wrote: > [...] > This is all very proof-of-concept, and uses the ugly hack of s/const > // for the options struct because I'm now keeping state in it, as > noted in one of the TODO comments that should be moved. > [...] > static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg, > - const struct option *options) > + struct option *options) > { > const struct option *all_opts = options; > [...] > @@ -313,7 +314,17 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, > const char *arg, > continue; > p->opt = rest + 1; > } > - return get_value(p, options, all_opts, flags ^ opt_flags); > + if (!(ret = get_value(p, options, all_opts, flags ^ > opt_flags))) { > + /* TODO: Keep some different state on the side > +* with info about what options we've > +* retrieved via the CLI for use in the loop > +* in parse_options_step, instead of making > +* the 'options' non-const > +*/ > + if (options->flags & PARSE_OPT_CONFIGURABLE) > + options->flags |= PARSE_OPT_VIA_CLI; > + } > + return ret; > } > [...] > + > + /* The loop above is driven by the argument vector, so we need > +* to make a second pass and find those options that are > +* configurable, and haven't been set via the command-line */ > + for (; options->type != OPTION_END; options++) { > + if (!(options->flags & PARSE_OPT_CONFIGURABLE)) > + continue; > + > + if (options->flags & PARSE_OPT_VIA_CLI) > + continue; > + > + /* TODO: Maybe factor the handling of OPTION_CALLBACK > +* in get_value() into a function. > +* > +* Do we also need to save away the state from the > +* loop above to handle unset? I think not, I think > +* we're always unset here by definition, right? > +*/ > + return (*options->conf_callback)(options, NULL, 1) ? (-1) : 0; > + } > + > [...] After looking at some of the internal APIs I'm thinking of replacing this pattern with a hashmap.c hashmap where the keys are a sprintf("%d:%s", short_name, long_name) to uniquely identify the option. There's no other obvious way to uniquely address an option. I guess I could just equivalently and more cheaply use the memory address of each option to identify them, but that seems a bit hacky. > @@ -110,6 +112,9 @@ struct option { > int flags; > parse_opt_cb *callback; > intptr_t defval; > + > + const char *conf_key; > + parse_opt_cb *conf_callback; > }; I've already found that this needs to be a char **conf_key, since several command-line options have multiple ways to spell the option name, e.g. add.ignoreerrors & add.ignore-errors, pack.writebitmaps & repack.writebitmaps etc.
Re: Will OpenSSL's license change impact us?
On Sat, Mar 25, 2017 at 10:43 AM, demerphq wrote: > > > On 25 Mar 2017 10:18 a.m., "Ævar Arnfjörð Bjarmason" > wrote: > > On Sat, Mar 25, 2017 at 9:40 AM, demerphq wrote: >> On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason >> wrote: >>> They're changing their license[1] to Apache 2 which unlike the current >>> fuzzy compatibility with the current license[2] is explicitly >>> incompatible with GPLv2[3]. >> >> Are you sure there is an issue? From the Apache page on this: >> >> Apache 2 software can therefore be included in GPLv3 projects, because >> the GPLv3 license accepts our software into GPLv3 works. However, >> GPLv3 software cannot be included in Apache projects. The licenses are >> incompatible in one direction only, and it is a result of ASF's >> licensing philosophy and the GPLv3 authors' interpretation of >> copyright law. >> >> Which seems to be the opposite of the concern you are expressing. > > The Apache 2 license is indeed compatible with the GPLv3, but the Git > project explicitly uses GPLv2 with no "or later" clause > > > Read the paragraph immediately (I think) after the one I quoted where they > state the situation is the same with GPL v2. My understanding of that paragraph is that it's still laying out caveats about exactly how GPLv3 is compatible with Apache 2, when it is, when it isn't etc. But then it goes on to say: """ Despite our best efforts, the FSF has never considered the Apache License to be compatible with GPL version 2, citing the patent termination and indemnification provisions as restrictions not present in the older GPL license. The Apache Software Foundation believes that you should always try to obey the constraints expressed by the copyright holder when redistributing their work. """ So they're just deferring to the FSF saying it's incompatible, the FSF's statement: https://www.gnu.org/licenses/license-list.html#apache2 "this license is not compatible with GPL version 2". Anyway, I'm not a lawyer. Just thought I'd send some E-Mail about this since I noticed it, if it's an issue (and we could e.g. get the SFC to comment, Jeff?) we might need to add e.g. some checks / macros to ensure we're not compiling against an incompatible OpenSSL. > > $ head -n 18 COPYING > > Note that the only valid version of the GPL as far as this project > is concerned is _this_ particular version of the license (ie v2, not > v2.2 or v3.x or whatever), unless explicitly otherwise stated. > > HOWEVER, in order to allow a migration to GPLv3 if that seems like > a good idea, I also ask that people involved with the project make > their preferences known. In particular, if you trust me to make that > decision, you might note so in your copyright message, ie something > like > > This file is licensed under the GPL v2, or a later version > at the discretion of Linus. > > might avoid issues. But we can also just decide to synchronize and > contact all copyright holders on record if/when the occasion arises. > > Linus Torvalds > >
Re: [PATCH] pretty: add extra headers and MIME boundary directly
On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote: > Use the after_subject member of struct pretty_print_context to pass the > extra_headers unchanged, and construct and add the MIME boundary headers > directly in pretty.c::pp_title_line() instead of writing both to a > static buffer in log-tree.c::log_write_email_headers() first. That's > easier, quicker and gets rid of said static buffer. I'm definitely pleased with the direction. A few comments: > @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, > struct commit *commit, > graph_show_oneline(opt->graph); > } > if (opt->mime_boundary) { > - static char subject_buffer[1024]; > static char buffer[1024]; We still have this other buffer, which ends up in stat_sep. It should probably get the same treatment, though I think the module boundaries make it a little more awkward. We look at it in diff_flush(), which otherwise doesn't need to know much about the pretty-printing. Perhaps stat_sep should be a callback? > diff --git a/pretty.c b/pretty.c > index d0f86f5d85..56e668781a 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp, > if (pp->after_subject) { > strbuf_addstr(sb, pp->after_subject); > } > + if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) { > + strbuf_addf(sb, > + "MIME-Version: 1.0\n" In the original, this would have been in "after_subject". Which means we would print it even if print_email_subject is not true. Why do we need to check it in the new conditional? Not that I expect the behavior to be wrong either way; why would we have a mime boundary without setting print_email_subject? But I would think that "do we have a mime boundary" would be the right conditional to trigger printing it. -Peff
Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages
Le mercredi 22 mars 2017 11:02:09 CET, vous avez écrit : > Jean-Noël Avila writes: > >> I am wondering if Documentation/po part should be a separate > >> repository, with a dedicated i18n/l10n coordinator. Would it make > >> it easier for (1) those who write code and doc without knowing other > >> languages, (2) those who update .pot and coordinate the l10n effort > >> for the documentation and (3) those who translate them if we keep > >> them in a single repository? > > > > This is one of the points raised in the first RFC mail. Splitting this > > part would help a lot manage the translations with their own workflow, > > would not clutter the main repo with files not really needed for > > packaging and would simplify dealing with the interaction with crowd > > translation websites which can directly push translation content to a > > git repo. > > As I was in favor of splitting it out, I was trying to gauge what > the downside of doing so would be, especially for those who are > doing the translation work (it is obvious that it would help > developers who are not translators, as nothing will change for them > if we keep this new thing as a separate project). There's one big downside of this splitting. The gitman-l10n project would not be autonomous without the specific cloning at the particular place in the git project. po4a needs the original asciidoc files to perform the transclusion of the translated content into the structure of the documents. The setup that you are proposing would rule out simple CI checks and would make it complicated for the translators to set up their working copy and check the resulting man pages. As I see it, there's the need for the Documentation folder to be contained in both project (while remaining the property of the git project). So I would think the other way around: for those interested in translated the documentation, some script would allow to checkout the git project inside the gitman-l10n project (like a kind of library). This would be mainly transparent for the git developers. > I'd prefer to start with the "optional gitman-l10n repository is > checked out at Documentation/po only by convention" approach, before > committing to bind it as a submodule. Once we got comfortable with > cooperating between these two projects, we do want to bind them > using the submodule mechanism, but not before. Obviously, my proposition would not allow to evolve towards such a setup, but is it really needed anyway ?
Re: t1503 broken ?
On Sat, Mar 25, 2017 at 07:26:14PM +0700, Duy Nguyen wrote: > On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen wrote: > > On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen wrote: > >> ./t1305-config-include.sh > >> seems to be broken: > >> not ok 19 - conditional include, $HOME expansion > >> not ok 21 - conditional include, relative path > > > > let me guess, your "git" directory is in a symlink path? > > Yes I could reproduce it when I put my "git" in a symlink. There's a > note in document about "Symlinks in `$GIT_DIR` are not resolved before > matching" but failing tests is not acceptable. I'll fix it. The fix may be something like this. The problem is $GIT_DIR has symlinks resolved, but we don't do the same for other paths in this code. As a result, matching paths fails. I'm a bit concerned about the change in expand_user_path() because I'm not quite sure if it's a completely safe change. But at least could you try the patch and see if it passe the tests on your machine too? -- 8< -- diff --git a/config.c b/config.c index 1a4d855..fc4eae9 100644 --- a/config.c +++ b/config.c @@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat) return error(_("relative config include " "conditionals must come from files")); - strbuf_add_absolute_path(&path, cf->path); + strbuf_realpath(&path, cf->path, 1); slash = find_last_dir_sep(path.buf); if (!slash) die("BUG: how is this possible?"); @@ -213,7 +213,7 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase) struct strbuf pattern = STRBUF_INIT; int ret = 0, prefix; - strbuf_add_absolute_path(&text, get_git_dir()); + strbuf_realpath(&text, get_git_dir(), 1); strbuf_add(&pattern, cond, cond_len); prefix = prepare_include_condition_pattern(&pattern); diff --git a/path.c b/path.c index 2224843..18eaac3 100644 --- a/path.c +++ b/path.c @@ -654,7 +654,7 @@ char *expand_user_path(const char *path) const char *home = getenv("HOME"); if (!home) goto return_null; - strbuf_addstr(&user_path, home); + strbuf_addstr(&user_path, real_path(home)); #ifdef GIT_WINDOWS_NATIVE convert_slashes(user_path.buf); #endif -- 8< -- -- Duy
Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators
On Sat, Mar 25, 2017 at 7:13 PM, Daniel Ferreira (theiostream) wrote: > You are correct, which shows that since all tests pass, we need to > come up with better cases for this function. > > As for a solution, I believe that the best way to go for it is to > dir_iterator's implementation to have an "Option to iterate over > directory paths before vs. after their contents" (something predicted > in the commit that created it). If it iterates over directories after > all of its contents (currently it does so before) we just need to > check if the entry is a directory and if so, rmdir() it. Does that > make sense? Yes. However since this is a microproject, intended to be done quickly just to get you familiarize with the community and the process, I suggest you look for another readdir() place that can be easily converted to using dir-iterator first so you could go through the whole thing. But of course I will not object you improving dir-iterator and clean remove_subtree() up ;-) -- Duy
Re: t1503 broken ?
On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen wrote: > On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen wrote: >> ./t1305-config-include.sh >> seems to be broken: >> not ok 19 - conditional include, $HOME expansion >> not ok 21 - conditional include, relative path > > let me guess, your "git" directory is in a symlink path? Yes I could reproduce it when I put my "git" in a symlink. There's a note in document about "Symlinks in `$GIT_DIR` are not resolved before matching" but failing tests is not acceptable. I'll fix it. -- Duy
[PATCH] pretty: add extra headers and MIME boundary directly
Use the after_subject member of struct pretty_print_context to pass the extra_headers unchanged, and construct and add the MIME boundary headers directly in pretty.c::pp_title_line() instead of writing both to a static buffer in log-tree.c::log_write_email_headers() first. That's easier, quicker and gets rid of said static buffer. Signed-off-by: Rene Scharfe --- builtin/log.c | 3 ++- log-tree.c| 26 ++ log-tree.h| 1 - pretty.c | 15 +++ 4 files changed, 19 insertions(+), 26 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 281af8c1ec..be564039c1 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -989,7 +989,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet)) return; - log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte); + log_write_email_headers(rev, head, &need_8bit_cte); + pp.after_subject = rev->extra_headers; for (i = 0; !need_8bit_cte && i < nr; i++) { const char *buf = get_commit_buffer(list[i], NULL); diff --git a/log-tree.c b/log-tree.c index 4618dd04ca..7049a17781 100644 --- a/log-tree.c +++ b/log-tree.c @@ -349,10 +349,8 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt) } void log_write_email_headers(struct rev_info *opt, struct commit *commit, -const char **extra_headers_p, int *need_8bit_cte_p) { - const char *extra_headers = opt->extra_headers; const char *name = oid_to_hex(opt->zero_commit ? &null_oid : &commit->object.oid); @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, graph_show_oneline(opt->graph); } if (opt->mime_boundary) { - static char subject_buffer[1024]; static char buffer[1024]; struct strbuf filename = STRBUF_INIT; *need_8bit_cte_p = -1; /* NEVER */ - snprintf(subject_buffer, sizeof(subject_buffer) - 1, -"%s" -"MIME-Version: 1.0\n" -"Content-Type: multipart/mixed;" -" boundary=\"%s%s\"\n" -"\n" -"This is a multi-part message in MIME " -"format.\n" -"--%s%s\n" -"Content-Type: text/plain; " -"charset=UTF-8; format=fixed\n" -"Content-Transfer-Encoding: 8bit\n\n", -extra_headers ? extra_headers : "", -mime_boundary_leader, opt->mime_boundary, -mime_boundary_leader, opt->mime_boundary); - extra_headers = subject_buffer; if (opt->numbered_files) strbuf_addf(&filename, "%d", opt->nr); @@ -413,7 +394,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, opt->diffopt.stat_sep = buffer; strbuf_release(&filename); } - *extra_headers_p = extra_headers; } static void show_sig_lines(struct rev_info *opt, int status, const char *bol) @@ -537,7 +517,6 @@ void show_log(struct rev_info *opt) struct log_info *log = opt->loginfo; struct commit *commit = log->commit, *parent = log->parent; int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40; - const char *extra_headers = opt->extra_headers; struct pretty_print_context ctx = {0}; opt->loginfo = NULL; @@ -597,8 +576,7 @@ void show_log(struct rev_info *opt) */ if (cmit_fmt_is_mail(opt->commit_format)) { - log_write_email_headers(opt, commit, &extra_headers, - &ctx.need_8bit_cte); + log_write_email_headers(opt, commit, &ctx.need_8bit_cte); ctx.rev = opt; ctx.print_email_subject = 1; } else if (opt->commit_format != CMIT_FMT_USERFORMAT) { @@ -672,7 +650,7 @@ void show_log(struct rev_info *opt) ctx.date_mode = opt->date_mode; ctx.date_mode_explicit = opt->date_mode_explicit; ctx.abbrev = opt->diffopt.abbrev; - ctx.after_subject = extra_headers; + ctx.after_subject = opt->extra_headers; ctx.preserve_subject = opt->preserve_subject; ctx.reflog_info = opt->reflog_info; ctx.fmt = opt->commit_format; diff --git a/log-tree.h b/log-tree.h index 48f11fb740..7f9c4f22b5 100644 --- a/log-tree.h +++ b/log-tree.h @@ -22,7 +22,6 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit, format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")") void show_
Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators
You are correct, which shows that since all tests pass, we need to come up with better cases for this function. As for a solution, I believe that the best way to go for it is to dir_iterator's implementation to have an "Option to iterate over directory paths before vs. after their contents" (something predicted in the commit that created it). If it iterates over directories after all of its contents (currently it does so before) we just need to check if the entry is a directory and if so, rmdir() it. Does that make sense? On Sat, Mar 25, 2017 at 8:51 AM, Duy Nguyen wrote: > On Sat, Mar 25, 2017 at 6:27 PM, Daniel Ferreira wrote: >> Use dir_iterator to traverse through remove_subtree()'s directory tree, >> avoiding the need for recursive calls to readdir(). Simplify >> remove_subtree()'s code. >> >> A conversion similar in purpose was previously done at 46d092a >> ("for_each_reflog(): reimplement using iterators", 2016-05-21). >> >> Signed-off-by: Daniel Ferreira >> --- >> >> This is a second-version patch of the Google Summer of Code microproject for >> refactoring recursive readdir() calls to use dir_iterator instead. v1 can be >> found in: >> >> https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730 >> >> Additionally, for debugging purposes I turned remove_subtree() into a no-op >> and ran git tests. Some failures were at: >> >> * t2000-checkout-cache-clash.sh >> * t2003-checkout-cache-mkdir.sh >> >> If you guys could check those files out and warn me if any additional tests >> would be welcome, please let me know. >> >> Thanks. >> >> entry.c | 28 +++- >> 1 file changed, 7 insertions(+), 21 deletions(-) >> >> diff --git a/entry.c b/entry.c >> index c6eea240b..3cb92592d 100644 >> --- a/entry.c >> +++ b/entry.c >> @@ -2,6 +2,8 @@ >> #include "blob.h" >> #include "dir.h" >> #include "streaming.h" >> +#include "iterator.h" >> +#include "dir-iterator.h" >> >> static void create_directories(const char *path, int path_len, >>const struct checkout *state) >> @@ -46,29 +48,13 @@ static void create_directories(const char *path, int >> path_len, >> >> static void remove_subtree(struct strbuf *path) >> { >> - DIR *dir = opendir(path->buf); >> - struct dirent *de; >> - int origlen = path->len; >> - >> - if (!dir) >> - die_errno("cannot opendir '%s'", path->buf); >> - while ((de = readdir(dir)) != NULL) { >> - struct stat st; >> - >> - if (is_dot_or_dotdot(de->d_name)) >> - continue; >> - >> - strbuf_addch(path, '/'); >> - strbuf_addstr(path, de->d_name); >> - if (lstat(path->buf, &st)) >> - die_errno("cannot lstat '%s'", path->buf); >> - if (S_ISDIR(st.st_mode)) >> - remove_subtree(path); >> - else if (unlink(path->buf)) >> + struct dir_iterator *diter = dir_iterator_begin(path->buf); >> + >> + while (dir_iterator_advance(diter) == ITER_OK) { >> + if (unlink(diter->path.buf)) >> die_errno("cannot unlink '%s'", path->buf); >> - strbuf_setlen(path, origlen); >> } >> - closedir(dir); >> + >> if (rmdir(path->buf)) >> die_errno("cannot rmdir '%s'", path->buf); > > Even though it's very nice that lots of code is deleted. This is not > entirely correct, is it? Before this patch, rmdir() is called for > every recursive remove_subtree() call. After this patch, it's only > called once (and likely fails unless you have no subdirectories). > >> } >> -- >> 2.12.1.433.g82305b74f.dirty >> > > > > -- > Duy
Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system
On Fri, Mar 24, 2017 at 12:55 AM, Junio C Hamano wrote: > Michael J Gruber writes: > >> Are we at a point where we can still rename the new feature at least? If >> yes, and keeping everything else is mandatory, than "workspace" or >> "working space" may be a serious contender for naming the new thing. > > I do not have a good answer to the first question, but workspace > does sound like a good name for what this feature is trying to > achieve. > Now is not too late to rename the command from worktree to workspace (and keep "worktree" as an alias that will be eventually deleted). Should we do it? I would keep file names, function names... unchanged though, not worth the amount of new conflicts. -- Duy
Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system
On Tue, Mar 21, 2017 at 10:48 PM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Tue, Mar 21, 2017 at 1:50 AM, Jonathan Nieder wrote: >>> Junio C Hamano wrote: Stefan Beller writes: >>> > While it may be true that you can have bare worktrees; I would question > why anyone wants to do this, as the only thing it provides is an > additional HEAD (plus its reflog). A more plausible situation is you start with a bare one as the primary and used to make local clones to do your work in the world before "git worktree". It would be a natural extension to your workflow to instead create worktrees of of that bare one as the primary worktree with secondaries with working trees. >>> >>> For what it's worth, this conversation makes me think it was a mistake >>> to call this construct a worktree. >> >> For the record, I am totally confused with Junio's last line, with two >> "with"s, "worktree" and "working trees" in the same phrase :D > > In case this wasn't just a tangential note, what I meant was: > > - In the old world, you may have had a single bare repository and >then made clones, each of which has a working tree (i.e. non-bare >clones), and worked inside these clones. > > - In the "git worktree" world, you can start from that same single >bare repository, but instead of cloning it, use "git worktree" to >create "worktree"s, each of which has a working tree, and work >inside these "worktree"s. > > and the latter would be a natural extension to the workflow the > former wanted to use. Yes I really want that, and even the ability to convert a normal one repo (with one working tree) to the latter, moving the repository to somewhere safe. >>> It's fine for the command to have one name and the documentation to >>> use a longer, clearer name to explain it. What should that longer, >>> clearer name be? >> >> No comments from me. I'll let you know that if Eric (or Junio?) didn't >> stop me, we would have had $GIT_DIR/repos now instead of >> $GIT_DIR/worktrees, just some extra confusion toppings. > > I forgot about that part of the history, but you are saying you > wanted to call these "repos", not "worktrees"? >From $GIT_DIR perspective (which points to $GIT_COMMON_DIR/worktrees/blah) then they do look like a repository with lots of part borrowed from $GIT_COMMON_DIR. I was simply saying I'm bad at naming things. "worktrees" is a better name than "repos". > I can see why > somebody (or me?) would stop that by fearing "repo" is a bit too > confusing with a "repository", in the same way that we are now > realizing that "worktree" is too similar to an old synonym we used > to call "working tree". -- Duy
Re: [PATCH v1 0/3] Add support for downloading blobs on demand
On Wed, Mar 22, 2017 at 11:52 PM, Ben Peart wrote: > We have a couple of patch series we’re working on (ObjectDB/Read-Object, > Watchman integration) Oops, sorry. I should be reworking the index-helper series for watchman support, but I haven't time for it. Yes I'm also eyeing Lars code as the communication channel for any external helper (which in turn can talk to watchman or whatever). I guess I'll let you do it then. -- Duy
Re: t1503 broken ?
On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen wrote: > ./t1305-config-include.sh > seems to be broken: > not ok 19 - conditional include, $HOME expansion > not ok 21 - conditional include, relative path let me guess, your "git" directory is in a symlink path? -- Duy
Re: [PATCH v6 24/27] t/helper: add test-ref-store to test ref-store functions
On Wed, Mar 22, 2017 at 8:37 PM, Jeff King wrote: > On Wed, Mar 22, 2017 at 09:34:12AM -0400, Jeff King wrote: > >> On Sat, Mar 18, 2017 at 09:03:34AM +0700, Nguyễn Thái Ngọc Duy wrote: >> >> > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c >> > new file mode 100644 >> >> When nd/files-backend-git-dir is merged to 'next', the new tests in >> t1405/t1406 break. They need this on top (which should probably just be >> squashed into this commit when you re-roll). >> >> -- >8 -- >> Subject: [PATCH] test-ref-store: setup git directory >> >> Without setting up the git directory, we rely on the ".git" >> fallback in setup_git_env(). This will cause us to abort >> once b1ef400ee (setup_git_env: avoid blind fall-back to >> ".git", 2016-10-20) is merged. > > After posting this, it occurred to me that "pu" should be showing the > same failure, but it doesn't. The reason is that e9e167145 (worktree.c: > kill parse_ref() in favor of refs_resolve_ref_unsafe(), 2017-03-18) from > nd/worktree-kill-parse-ref sneaks in the setup call. > > We should move it back to the addition of test-ref-store, though, since > nd/files-backend-git-dir may graduate separately. Right. I needed that setup_git_directory() for the third ref store. But now that I think about it, I need it for the first two as well because git_path() is always involved. Will fix (since it looks like files-backend-git-dir is not merged to next yet). -- Duy
Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators
On Sat, Mar 25, 2017 at 6:27 PM, Daniel Ferreira wrote: > Use dir_iterator to traverse through remove_subtree()'s directory tree, > avoiding the need for recursive calls to readdir(). Simplify > remove_subtree()'s code. > > A conversion similar in purpose was previously done at 46d092a > ("for_each_reflog(): reimplement using iterators", 2016-05-21). > > Signed-off-by: Daniel Ferreira > --- > > This is a second-version patch of the Google Summer of Code microproject for > refactoring recursive readdir() calls to use dir_iterator instead. v1 can be > found in: > > https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730 > > Additionally, for debugging purposes I turned remove_subtree() into a no-op > and ran git tests. Some failures were at: > > * t2000-checkout-cache-clash.sh > * t2003-checkout-cache-mkdir.sh > > If you guys could check those files out and warn me if any additional tests > would be welcome, please let me know. > > Thanks. > > entry.c | 28 +++- > 1 file changed, 7 insertions(+), 21 deletions(-) > > diff --git a/entry.c b/entry.c > index c6eea240b..3cb92592d 100644 > --- a/entry.c > +++ b/entry.c > @@ -2,6 +2,8 @@ > #include "blob.h" > #include "dir.h" > #include "streaming.h" > +#include "iterator.h" > +#include "dir-iterator.h" > > static void create_directories(const char *path, int path_len, >const struct checkout *state) > @@ -46,29 +48,13 @@ static void create_directories(const char *path, int > path_len, > > static void remove_subtree(struct strbuf *path) > { > - DIR *dir = opendir(path->buf); > - struct dirent *de; > - int origlen = path->len; > - > - if (!dir) > - die_errno("cannot opendir '%s'", path->buf); > - while ((de = readdir(dir)) != NULL) { > - struct stat st; > - > - if (is_dot_or_dotdot(de->d_name)) > - continue; > - > - strbuf_addch(path, '/'); > - strbuf_addstr(path, de->d_name); > - if (lstat(path->buf, &st)) > - die_errno("cannot lstat '%s'", path->buf); > - if (S_ISDIR(st.st_mode)) > - remove_subtree(path); > - else if (unlink(path->buf)) > + struct dir_iterator *diter = dir_iterator_begin(path->buf); > + > + while (dir_iterator_advance(diter) == ITER_OK) { > + if (unlink(diter->path.buf)) > die_errno("cannot unlink '%s'", path->buf); > - strbuf_setlen(path, origlen); > } > - closedir(dir); > + > if (rmdir(path->buf)) > die_errno("cannot rmdir '%s'", path->buf); Even though it's very nice that lots of code is deleted. This is not entirely correct, is it? Before this patch, rmdir() is called for every recursive remove_subtree() call. After this patch, it's only called once (and likely fails unless you have no subdirectories). > } > -- > 2.12.1.433.g82305b74f.dirty > -- Duy
[PATCH v2] [GSoC] remove_subtree(): reimplement using iterators
Use dir_iterator to traverse through remove_subtree()'s directory tree, avoiding the need for recursive calls to readdir(). Simplify remove_subtree()'s code. A conversion similar in purpose was previously done at 46d092a ("for_each_reflog(): reimplement using iterators", 2016-05-21). Signed-off-by: Daniel Ferreira --- This is a second-version patch of the Google Summer of Code microproject for refactoring recursive readdir() calls to use dir_iterator instead. v1 can be found in: https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730 Additionally, for debugging purposes I turned remove_subtree() into a no-op and ran git tests. Some failures were at: * t2000-checkout-cache-clash.sh * t2003-checkout-cache-mkdir.sh If you guys could check those files out and warn me if any additional tests would be welcome, please let me know. Thanks. entry.c | 28 +++- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/entry.c b/entry.c index c6eea240b..3cb92592d 100644 --- a/entry.c +++ b/entry.c @@ -2,6 +2,8 @@ #include "blob.h" #include "dir.h" #include "streaming.h" +#include "iterator.h" +#include "dir-iterator.h" static void create_directories(const char *path, int path_len, const struct checkout *state) @@ -46,29 +48,13 @@ static void create_directories(const char *path, int path_len, static void remove_subtree(struct strbuf *path) { - DIR *dir = opendir(path->buf); - struct dirent *de; - int origlen = path->len; - - if (!dir) - die_errno("cannot opendir '%s'", path->buf); - while ((de = readdir(dir)) != NULL) { - struct stat st; - - if (is_dot_or_dotdot(de->d_name)) - continue; - - strbuf_addch(path, '/'); - strbuf_addstr(path, de->d_name); - if (lstat(path->buf, &st)) - die_errno("cannot lstat '%s'", path->buf); - if (S_ISDIR(st.st_mode)) - remove_subtree(path); - else if (unlink(path->buf)) + struct dir_iterator *diter = dir_iterator_begin(path->buf); + + while (dir_iterator_advance(diter) == ITER_OK) { + if (unlink(diter->path.buf)) die_errno("cannot unlink '%s'", path->buf); - strbuf_setlen(path, origlen); } - closedir(dir); + if (rmdir(path->buf)) die_errno("cannot rmdir '%s'", path->buf); } -- 2.12.1.433.g82305b74f.dirty
t1503 broken ?
./t1305-config-include.sh seems to be broken: not ok 19 - conditional include, $HOME expansion not ok 21 - conditional include, relative path Both Mac and Linux. The problem seems to be the line git config test.two >actual && and git config test.four >actual && In both cases the config "test.two or test.four" is not found, at least that is what my debugging shows. After adding an exit 0 before the test_done gives this: find trash\ directory.t1305-config-include/ -name "bar*" trash directory.t1305-config-include/foo/.git/bar trash directory.t1305-config-include/foo/.git/bar3 trash directory.t1305-config-include/foo/.git/bar5 trash directory.t1305-config-include/foo/.git/bar2 trash directory.t1305-config-include/bar4 Where should bar2 and bar4 be ?
Re: Will OpenSSL's license change impact us?
On Sat, Mar 25, 2017 at 9:40 AM, demerphq wrote: > On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason wrote: >> They're changing their license[1] to Apache 2 which unlike the current >> fuzzy compatibility with the current license[2] is explicitly >> incompatible with GPLv2[3]. > > Are you sure there is an issue? From the Apache page on this: > > Apache 2 software can therefore be included in GPLv3 projects, because > the GPLv3 license accepts our software into GPLv3 works. However, > GPLv3 software cannot be included in Apache projects. The licenses are > incompatible in one direction only, and it is a result of ASF's > licensing philosophy and the GPLv3 authors' interpretation of > copyright law. > > Which seems to be the opposite of the concern you are expressing. The Apache 2 license is indeed compatible with the GPLv3, but the Git project explicitly uses GPLv2 with no "or later" clause: $ head -n 18 COPYING Note that the only valid version of the GPL as far as this project is concerned is _this_ particular version of the license (ie v2, not v2.2 or v3.x or whatever), unless explicitly otherwise stated. HOWEVER, in order to allow a migration to GPLv3 if that seems like a good idea, I also ask that people involved with the project make their preferences known. In particular, if you trust me to make that decision, you might note so in your copyright message, ie something like This file is licensed under the GPL v2, or a later version at the discretion of Linus. might avoid issues. But we can also just decide to synchronize and contact all copyright holders on record if/when the occasion arises. Linus Torvalds
Re: Will OpenSSL's license change impact us?
On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason wrote: > They're changing their license[1] to Apache 2 which unlike the current > fuzzy compatibility with the current license[2] is explicitly > incompatible with GPLv2[3]. Are you sure there is an issue? From the Apache page on this: Apache 2 software can therefore be included in GPLv3 projects, because the GPLv3 license accepts our software into GPLv3 works. However, GPLv3 software cannot be included in Apache projects. The licenses are incompatible in one direction only, and it is a result of ASF's licensing philosophy and the GPLv3 authors' interpretation of copyright law. Which seems to be the opposite of the concern you are expressing. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"