From 69ddd231fe8d4de64990d42b96d34940a7e43aef Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:32:07 -0400 Subject: [PATCH 01/16] cache.h: clarify documentation for git_path, et al The comment above these functions actually describes sha1_file_name, and comes from the very first revision of git. Commit 723c31f (Add "git_path()" and "head_ref()" helper functions., 2005-07-05) added git_path, pushing the comment away from the function it describes; later commits added more functions in this block. Let's fix the comment to describe these related functions in more detail. Let's also make sure to point out their safer alternatives (and move those alternatives below, which makes more sense when reading the file). Note that we do not need to move the existing comment to sha1_file_name. Commit d40d535 (sha1_file.c: document a bunch of functions defined in the file, 2014-02-21) already added a much more descriptive comment to it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/cache.h b/cache.h index 6bb7119032..8db884e3de 100644 --- a/cache.h +++ b/cache.h @@ -708,6 +708,18 @@ extern int check_repository_format(void); #define DATA_CHANGED 0x0020 #define TYPE_CHANGED 0x0040 +/* + * Return a statically allocated filename, either generically (mkpath), in + * the repository directory (git_path), or in a submodule's repository + * directory (git_path_submodule). In all cases, note that the result + * may be overwritten by another call to _any_ of the functions. Consider + * using the safer "dup" or "strbuf" formats below. + */ +extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2))); +extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2))); +extern const char *git_path_submodule(const char *path, const char *fmt, ...) + __attribute__((format (printf, 2, 3))); + extern char *mksnpath(char *buf, size_t n, const char *fmt, ...) __attribute__((format (printf, 3, 4))); extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...) @@ -717,11 +729,6 @@ extern char *git_pathdup(const char *fmt, ...) extern char *mkpathdup(const char *fmt, ...) __attribute__((format (printf, 1, 2))); -/* Return a statically allocated filename matching the sha1 signature */ -extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2))); -extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2))); -extern const char *git_path_submodule(const char *path, const char *fmt, ...) - __attribute__((format (printf, 2, 3))); extern void report_linked_checkout_garbage(void); /* From f5895fd399855e782ed0f86f1ac0531517efa0d9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:32:22 -0400 Subject: [PATCH 02/16] cache.h: complete set of git_path_submodule helpers The git_path function has "git_pathdup" and "strbuf_git_path" variants, but git_submodule_path only comes in the dangerous, static-buffer variant. That makes refactoring callers to use the safer functions hard (since they don't exist). Since we're already using a strbuf behind the scenes, it's easy to expose all three of these interfaces with thin wrappers. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 5 +++++ path.c | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/cache.h b/cache.h index 8db884e3de..6f74f33f41 100644 --- a/cache.h +++ b/cache.h @@ -724,10 +724,15 @@ extern char *mksnpath(char *buf, size_t n, const char *fmt, ...) __attribute__((format (printf, 3, 4))); extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...) __attribute__((format (printf, 2, 3))); +extern void strbuf_git_path_submodule(struct strbuf *sb, const char *path, + const char *fmt, ...) + __attribute__((format (printf, 3, 4))); extern char *git_pathdup(const char *fmt, ...) __attribute__((format (printf, 1, 2))); extern char *mkpathdup(const char *fmt, ...) __attribute__((format (printf, 1, 2))); +extern char *git_pathdup_submodule(const char *path, const char *fmt, ...) + __attribute__((format (printf, 2, 3))); extern void report_linked_checkout_garbage(void); diff --git a/path.c b/path.c index 10f4cbf6b7..9aad9a1878 100644 --- a/path.c +++ b/path.c @@ -224,11 +224,10 @@ const char *mkpath(const char *fmt, ...) return cleanup_path(pathname->buf); } -const char *git_path_submodule(const char *path, const char *fmt, ...) +static void do_submodule_path(struct strbuf *buf, const char *path, + const char *fmt, va_list args) { - struct strbuf *buf = get_pathname(); const char *git_dir; - va_list args; strbuf_addstr(buf, path); if (buf->len && buf->buf[buf->len - 1] != '/') @@ -242,13 +241,39 @@ const char *git_path_submodule(const char *path, const char *fmt, ...) } strbuf_addch(buf, '/'); - va_start(args, fmt); strbuf_vaddf(buf, fmt, args); - va_end(args); strbuf_cleanup_path(buf); +} + +const char *git_path_submodule(const char *path, const char *fmt, ...) +{ + va_list args; + struct strbuf *buf = get_pathname(); + va_start(args, fmt); + do_submodule_path(buf, path, fmt, args); + va_end(args); return buf->buf; } +char *git_pathdup_submodule(const char *path, const char *fmt, ...) +{ + va_list args; + struct strbuf buf = STRBUF_INIT; + va_start(args, fmt); + do_submodule_path(&buf, path, fmt, args); + va_end(args); + return strbuf_detach(&buf, NULL); +} + +void strbuf_git_path_submodule(struct strbuf *buf, const char *path, + const char *fmt, ...) +{ + va_list args; + va_start(args, fmt); + do_submodule_path(buf, path, fmt, args); + va_end(args); +} + int validate_headref(const char *path) { struct stat st; From bc192300c49a32e1ef297dc3fa78a101f28e2a4c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:32:30 -0400 Subject: [PATCH 03/16] t5700: modernize style The early part of this test is rather old, and does not follow our usual style guidelines. In particular: - the tests liberally chdir, and expect out-of-test "cd" commands to return them to a sane state - test commands aren't indented at all - there are a lot of minor formatting nits, like the opening quote of the test block on the wrong line, spaces after ">", etc This patch fixes the style issues, and uses a few helper functions, along with subshells and "git -C", to avoid changing the cwd of the main script. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5700-clone-reference.sh | 193 ++++++++++++++++--------------------- 1 file changed, 81 insertions(+), 112 deletions(-) diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index 3e783fc450..51d131a97a 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -10,49 +10,51 @@ base_dir=`pwd` U=$base_dir/UPLOAD_LOG -test_expect_success 'preparing first repository' \ -'test_create_repo A && cd A && -echo first > file1 && -git add file1 && -git commit -m initial' - -cd "$base_dir" - -test_expect_success 'preparing second repository' \ -'git clone A B && cd B && -echo second > file2 && -git add file2 && -git commit -m addition && -git repack -a -d && -git prune' - -cd "$base_dir" - -test_expect_success 'cloning with reference (-l -s)' \ -'git clone -l -s --reference B A C' - -cd "$base_dir" - -test_expect_success 'existence of info/alternates' \ -'test_line_count = 2 C/.git/objects/info/alternates' - -cd "$base_dir" +# create a commit in repo $1 with name $2 +commit_in () { + ( + cd "$1" && + echo "$2" >"$2" && + git add "$2" && + git commit -m "$2" + ) +} + +# check that there are $2 loose objects in repo $1 +test_objcount () { + echo "$2" >expect && + git -C "$1" count-objects >actual.raw && + cut -d' ' -f1 actual && + test_cmp expect actual +} + +test_expect_success 'preparing first repository' ' + test_create_repo A && + commit_in A file1 +' -test_expect_success 'pulling from reference' \ -'cd C && -git pull ../B master' +test_expect_success 'preparing second repository' ' + git clone A B && + commit_in B file2 && + git -C B repack -ad && + git -C B prune +' -cd "$base_dir" +test_expect_success 'cloning with reference (-l -s)' ' + git clone -l -s --reference B A C +' -test_expect_success 'that reference gets used' \ -'cd C && -echo "0 objects, 0 kilobytes" > expected && -git count-objects > current && -test_cmp expected current' +test_expect_success 'existence of info/alternates' ' + test_line_count = 2 C/.git/objects/info/alternates +' -cd "$base_dir" +test_expect_success 'pulling from reference' ' + git -C C pull ../B master +' -rm -f "$U.D" +test_expect_success 'that reference gets used' ' + test_objcount C 0 +' test_expect_success 'cloning with reference (no -l -s)' ' GIT_TRACE_PACKET=$U.D git clone --reference B "file://$(pwd)/A" D @@ -63,95 +65,64 @@ test_expect_success 'fetched no objects' ' ! grep " want" "$U.D" ' -cd "$base_dir" - -test_expect_success 'existence of info/alternates' \ -'test_line_count = 1 D/.git/objects/info/alternates' - -cd "$base_dir" - -test_expect_success 'pulling from reference' \ -'cd D && git pull ../B master' - -cd "$base_dir" - -test_expect_success 'that reference gets used' \ -'cd D && echo "0 objects, 0 kilobytes" > expected && -git count-objects > current && -test_cmp expected current' - -cd "$base_dir" +test_expect_success 'existence of info/alternates' ' + test_line_count = 1 D/.git/objects/info/alternates +' -test_expect_success 'updating origin' \ -'cd A && -echo third > file3 && -git add file3 && -git commit -m update && -git repack -a -d && -git prune' +test_expect_success 'pulling from reference' ' + git -C D pull ../B master +' -cd "$base_dir" +test_expect_success 'that reference gets used' ' + test_objcount D 0 +' -test_expect_success 'pulling changes from origin' \ -'cd C && -git pull origin' +test_expect_success 'updating origin' ' + commit_in A file3 && + git -C A repack -ad && + git -C A prune +' -cd "$base_dir" +test_expect_success 'pulling changes from origin' ' + git -C C pull origin +' # the 2 local objects are commit and tree from the merge -test_expect_success 'that alternate to origin gets used' \ -'cd C && -echo "2 objects" > expected && -git count-objects | cut -d, -f1 > current && -test_cmp expected current' - -cd "$base_dir" - -test_expect_success 'pulling changes from origin' \ -'cd D && -git pull origin' +test_expect_success 'that alternate to origin gets used' ' + test_objcount C 2 +' -cd "$base_dir" +test_expect_success 'pulling changes from origin' ' + git -C D pull origin +' # the 5 local objects are expected; file3 blob, commit in A to add it # and its tree, and 2 are our tree and the merge commit. -test_expect_success 'check objects expected to exist locally' \ -'cd D && -echo "5 objects" > expected && -git count-objects | cut -d, -f1 > current && -test_cmp expected current' - -cd "$base_dir" - -test_expect_success 'preparing alternate repository #1' \ -'test_create_repo F && cd F && -echo first > file1 && -git add file1 && -git commit -m initial' - -cd "$base_dir" - -test_expect_success 'cloning alternate repo #2 and adding changes to repo #1' \ -'git clone F G && cd F && -echo second > file2 && -git add file2 && -git commit -m addition' - -cd "$base_dir" +test_expect_success 'check objects expected to exist locally' ' + test_objcount D 5 +' -test_expect_success 'cloning alternate repo #1, using #2 as reference' \ -'git clone --reference G F H' +test_expect_success 'preparing alternate repository #1' ' + test_create_repo F && + commit_in F file1 +' -cd "$base_dir" +test_expect_success 'cloning alternate repo #2 and adding changes to repo #1' ' + git clone F G && + commit_in F file2 +' -test_expect_success 'cloning with reference being subset of source (-l -s)' \ -'git clone -l -s --reference A B E' +test_expect_success 'cloning alternate repo #1, using #2 as reference' ' + git clone --reference G F H +' -cd "$base_dir" +test_expect_success 'cloning with reference being subset of source (-l -s)' ' + git clone -l -s --reference A B E +' test_expect_success 'clone with reference from a tagged repository' ' ( - cd A && git tag -a -m 'tagged' HEAD + cd A && git tag -a -m tagged HEAD ) && git clone --reference=A A I ' @@ -168,8 +139,6 @@ test_expect_success 'prepare branched repository' ' ) ' -rm -f "$U.K" - test_expect_success 'fetch with incomplete alternates' ' git init K && echo "$base_dir/A/.git/objects" >K/.git/objects/info/alternates && From 77b9b1d13ac9e6b78ba676d4edb221b7d2273c62 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:34:46 -0400 Subject: [PATCH 04/16] add_to_alternates_file: don't add duplicate entries The add_to_alternates_file function blindly uses hold_lock_file_for_append to copy the existing contents, and then adds the new line to it. This has two minor problems: 1. We might add duplicate entries, which are ugly and inefficient. 2. We do not check that the file ends with a newline, in which case we would bogusly append to the final line. This is quite unlikely in practice, though, as we call this function only from git-clone, so presumably we are the only writers of the file (and we always add a newline). Instead of using hold_lock_file_for_append, let's copy the file line by line, which ensures all records are properly terminated. If we see an extra line, we can simply abort the update (there is no point in even copying the rest, as we know that it would be identical to the original). As a bonus, we also get rid of some calls to the static-buffer mkpath and git_path functions. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_file.c | 47 ++++++++++++++++++++++++++++++++------ t/t5700-clone-reference.sh | 5 ++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 1cee438422..3400b8be4c 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -404,13 +404,46 @@ void read_info_alternates(const char * relative_base, int depth) void add_to_alternates_file(const char *reference) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR); - const char *alt = mkpath("%s\n", reference); - write_or_die(fd, alt, strlen(alt)); - if (commit_lock_file(lock)) - die("could not close alternates file"); - if (alt_odb_tail) - link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0); + char *alts = git_pathdup("objects/info/alternates"); + FILE *in, *out; + + hold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR); + out = fdopen_lock_file(lock, "w"); + if (!out) + die_errno("unable to fdopen alternates lockfile"); + + in = fopen(alts, "r"); + if (in) { + struct strbuf line = STRBUF_INIT; + int found = 0; + + while (strbuf_getline(&line, in, '\n') != EOF) { + if (!strcmp(reference, line.buf)) { + found = 1; + break; + } + fprintf_or_die(out, "%s\n", line.buf); + } + + strbuf_release(&line); + fclose(in); + + if (found) { + rollback_lock_file(lock); + lock = NULL; + } + } + else if (errno != ENOENT) + die_errno("unable to read alternates file"); + + if (lock) { + fprintf_or_die(out, "%s\n", reference); + if (commit_lock_file(lock)) + die_errno("unable to move new alternates file into place"); + if (alt_odb_tail) + link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0); + } + free(alts); } int foreach_alt_odb(alt_odb_fn fn, void *cb) diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index 51d131a97a..ef1779f5ca 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -120,6 +120,11 @@ test_expect_success 'cloning with reference being subset of source (-l -s)' ' git clone -l -s --reference A B E ' +test_expect_success 'cloning with multiple references drops duplicates' ' + git clone -s --reference B --reference A --reference B A dups && + test_line_count = 2 dups/.git/objects/info/alternates +' + test_expect_success 'clone with reference from a tagged repository' ' ( cd A && git tag -a -m tagged HEAD From fcd12db6af118b70b5c15cf5fdd6800eeecc370a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:35:31 -0400 Subject: [PATCH 05/16] prefer git_pathdup to git_path in some possibly-dangerous cases Because git_path uses a static buffer that is shared with calls to git_path, mkpath, etc, it can be dangerous to assign the result to a variable or pass it to a non-trivial function. The value may change unexpectedly due to other calls. None of the cases changed here has a known bug, but they're worth converting away from git_path because: 1. It's easy to use git_pathdup in these cases. 2. They use constructs (like assignment) that make it hard to tell whether they're safe or not. The extra malloc overhead should be trivial, as an allocation should be an order of magnitude cheaper than a system call (which we are clearly about to make, since we are constructing a filename). The real cost is that we must remember to free the result. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fsck.c | 4 +++- fast-import.c | 4 +++- http-backend.c | 3 ++- notes-merge.c | 3 ++- refs.c | 14 ++++++++------ unpack-trees.c | 4 +++- 6 files changed, 21 insertions(+), 11 deletions(-) diff --git a/builtin/fsck.c b/builtin/fsck.c index f4b87e9b33..079470342f 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -243,13 +243,14 @@ static void check_unreachable_object(struct object *obj) printf("dangling %s %s\n", typename(obj->type), sha1_to_hex(obj->sha1)); if (write_lost_and_found) { - const char *filename = git_path("lost-found/%s/%s", + char *filename = git_pathdup("lost-found/%s/%s", obj->type == OBJ_COMMIT ? "commit" : "other", sha1_to_hex(obj->sha1)); FILE *f; if (safe_create_leading_directories_const(filename)) { error("Could not create lost-found"); + free(filename); return; } if (!(f = fopen(filename, "w"))) @@ -262,6 +263,7 @@ static void check_unreachable_object(struct object *obj) if (fclose(f)) die_errno("Could not finish '%s'", filename); + free(filename); } return; } diff --git a/fast-import.c b/fast-import.c index 2ad4fee07e..ad8848bef1 100644 --- a/fast-import.c +++ b/fast-import.c @@ -407,7 +407,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *); static void write_crash_report(const char *err) { - const char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid()); + char *loc = git_pathdup("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid()); FILE *rpt = fopen(loc, "w"); struct branch *b; unsigned long lu; @@ -415,6 +415,7 @@ static void write_crash_report(const char *err) if (!rpt) { error("can't write crash report %s: %s", loc, strerror(errno)); + free(loc); return; } @@ -488,6 +489,7 @@ static void write_crash_report(const char *err) fputs("-------------------\n", rpt); fputs("END OF CRASH REPORT\n", rpt); fclose(rpt); + free(loc); } static void end_packfile(void); diff --git a/http-backend.c b/http-backend.c index b977c006a4..bac40ef6e6 100644 --- a/http-backend.c +++ b/http-backend.c @@ -164,7 +164,7 @@ static void send_strbuf(const char *type, struct strbuf *buf) static void send_local_file(const char *the_type, const char *name) { - const char *p = git_path("%s", name); + char *p = git_pathdup("%s", name); size_t buf_alloc = 8192; char *buf = xmalloc(buf_alloc); int fd; @@ -191,6 +191,7 @@ static void send_local_file(const char *the_type, const char *name) } close(fd); free(buf); + free(p); } static void get_text_file(char *name) diff --git a/notes-merge.c b/notes-merge.c index 0b2b82c41f..b3d1dab51f 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -295,7 +295,7 @@ static void write_buf_to_worktree(const unsigned char *obj, const char *buf, unsigned long size) { int fd; - const char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj)); + char *path = git_pathdup(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj)); if (safe_create_leading_directories_const(path)) die_errno("unable to create directory for '%s'", path); if (file_exists(path)) @@ -320,6 +320,7 @@ static void write_buf_to_worktree(const unsigned char *obj, } close(fd); + free(path); } static void write_note_to_worktree(const unsigned char *obj, diff --git a/refs.c b/refs.c index 2db2975e08..93b250e754 100644 --- a/refs.c +++ b/refs.c @@ -1288,12 +1288,12 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) */ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs) { - const char *packed_refs_file; + char *packed_refs_file; if (*refs->name) - packed_refs_file = git_path_submodule(refs->name, "packed-refs"); + packed_refs_file = git_pathdup_submodule(refs->name, "packed-refs"); else - packed_refs_file = git_path("packed-refs"); + packed_refs_file = git_pathdup("packed-refs"); if (refs->packed && !stat_validity_check(&refs->packed->validity, packed_refs_file)) @@ -1312,6 +1312,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs) fclose(f); } } + free(packed_refs_file); return refs->packed; } @@ -1481,14 +1482,15 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs, { int fd, len; char buffer[128], *p; - const char *path; + char *path; if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN) return -1; path = *refs->name - ? git_path_submodule(refs->name, "%s", refname) - : git_path("%s", refname); + ? git_pathdup_submodule(refs->name, "%s", refname) + : git_pathdup("%s", refname); fd = open(path, O_RDONLY); + free(path); if (fd < 0) return resolve_gitlink_packed_ref(refs, refname, sha1); diff --git a/unpack-trees.c b/unpack-trees.c index d6cf84904f..7bb446a4af 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1029,10 +1029,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options if (!core_apply_sparse_checkout || !o->update) o->skip_sparse_checkout = 1; if (!o->skip_sparse_checkout) { - if (add_excludes_from_file_to_list(git_path("info/sparse-checkout"), "", 0, &el, 0) < 0) + char *sparse = git_pathdup("info/sparse-checkout"); + if (add_excludes_from_file_to_list(sparse, "", 0, &el, 0) < 0) o->skip_sparse_checkout = 1; else o->el = ⪙ + free(sparse); } memset(&o->result, 0, sizeof(o->result)); From e3cf230324f740653d4fb4a3087c2daf9da62029 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:35:38 -0400 Subject: [PATCH 06/16] prefer mkpathdup to mkpath in assignments As with the previous commit to git_path, assigning the result of mkpath is suspicious, since it is not clear whether we will still depend on the value after it may have been overwritten by subsequent calls. This patch converts low-hanging fruit to use mkpathdup instead of mkpath (with the downside that we must remember to free the result). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/repack.c | 24 +++++++++++++----------- refs.c | 6 ++++-- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index af7340c7ba..70b9b1eaf1 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -285,8 +285,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) failed = 0; for_each_string_list_item(item, &names) { for (ext = 0; ext < ARRAY_SIZE(exts); ext++) { - const char *fname_old; - char *fname; + char *fname, *fname_old; fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext].name); if (!file_exists(fname)) { @@ -294,7 +293,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) continue; } - fname_old = mkpath("%s/old-%s%s", packdir, + fname_old = mkpathdup("%s/old-%s%s", packdir, item->string, exts[ext].name); if (file_exists(fname_old)) if (unlink(fname_old)) @@ -302,10 +301,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (!failed && rename(fname, fname_old)) { free(fname); + free(fname_old); failed = 1; break; } else { string_list_append(&rollback, fname); + free(fname_old); } } if (failed) @@ -314,13 +315,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (failed) { struct string_list rollback_failure = STRING_LIST_INIT_DUP; for_each_string_list_item(item, &rollback) { - const char *fname_old; - char *fname; + char *fname, *fname_old; fname = mkpathdup("%s/%s", packdir, item->string); - fname_old = mkpath("%s/old-%s", packdir, item->string); + fname_old = mkpathdup("%s/old-%s", packdir, item->string); if (rename(fname_old, fname)) string_list_append(&rollback_failure, fname); free(fname); + free(fname_old); } if (rollback_failure.nr) { @@ -368,13 +369,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix) /* Remove the "old-" files */ for_each_string_list_item(item, &names) { for (ext = 0; ext < ARRAY_SIZE(exts); ext++) { - const char *fname; - fname = mkpath("%s/old-%s%s", - packdir, - item->string, - exts[ext].name); + char *fname; + fname = mkpathdup("%s/old-%s%s", + packdir, + item->string, + exts[ext].name); if (remove_path(fname)) warning(_("removing '%s' failed"), fname); + free(fname); } } diff --git a/refs.c b/refs.c index 93b250e754..e70941a090 100644 --- a/refs.c +++ b/refs.c @@ -3380,7 +3380,7 @@ static int commit_ref_update(struct ref_lock *lock, int create_symref(const char *ref_target, const char *refs_heads_master, const char *logmsg) { - const char *lockpath; + char *lockpath = NULL; char ref[1000]; int fd, len, written; char *git_HEAD = git_pathdup("%s", ref_target); @@ -3407,7 +3407,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master, error("refname too long: %s", refs_heads_master); goto error_free_return; } - lockpath = mkpath("%s.lock", git_HEAD); + lockpath = mkpathdup("%s.lock", git_HEAD); fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666); if (fd < 0) { error("Unable to open %s for writing", lockpath); @@ -3427,9 +3427,11 @@ int create_symref(const char *ref_target, const char *refs_heads_master, error_unlink_return: unlink_or_warn(lockpath); error_free_return: + free(lockpath); free(git_HEAD); return -1; } + free(lockpath); #ifndef NO_SYMLINK_HEAD done: From b21a5d66055e0a446e61a12d540406f2757f6263 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:35:49 -0400 Subject: [PATCH 07/16] remote.c: drop extraneous local variable from migrate_file It's an anti-pattern to assign the result of git_path to a variable, since other calls may reuse our buffer. In this case, we feed the result to unlink_or_warn immediately afterwards, so it's OK. However, it's nice to avoid assignment entirely, which makes it more obvious that there's no bug. We can just pass the result directly to unlink_or_warn, which is a known-simple function. As a bonus, the code flow is a little more obvious, as we eliminate an extra conditional (a reader does not have to wonder any more "under which circumstances is 'path' set?"). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/remote.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index cc3c7410f3..181668dedd 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -581,7 +581,6 @@ static int migrate_file(struct remote *remote) { struct strbuf buf = STRBUF_INIT; int i; - const char *path = NULL; strbuf_addf(&buf, "remote.%s.url", remote->name); for (i = 0; i < remote->url_nr; i++) @@ -601,11 +600,9 @@ static int migrate_file(struct remote *remote) return error(_("Could not append '%s' to '%s'"), remote->fetch_refspec[i], buf.buf); if (remote->origin == REMOTE_REMOTES) - path = git_path("remotes/%s", remote->name); + unlink_or_warn(git_path("remotes/%s", remote->name)); else if (remote->origin == REMOTE_BRANCHES) - path = git_path("branches/%s", remote->name); - if (path) - unlink_or_warn(path); + unlink_or_warn(git_path("branches/%s", remote->name)); return 0; } From f5b2dec1657e09a22f8b2aefa25d022988e3e467 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:36:19 -0400 Subject: [PATCH 08/16] refs.c: remove extra git_path calls from read_loose_refs In iterating over the loose refs in "refs/foo/", we keep a running strbuf with "refs/foo/one", "refs/foo/two", etc. But we also need to access these files in the filesystem, as ".git/refs/foo/one", etc. For this latter purpose, we make a series of independent calls to git_path(). These are safe (we only use the result to call stat()), but assigning the result of git_path is a suspicious pattern that we'd rather avoid. This patch keeps a running buffer with ".git/refs/foo/", and we can just append/reset each directory element as we loop. This matches how we handle the refnames. It should also be more efficient, as we do not keep formatting the same ".git/refs/foo" prefix (which can be arbitrarily deep). Technically we are dropping a call to strbuf_cleanup() on each generated filename, but that's OK; it wasn't doing anything, as we are putting in single-level names we read from the filesystem (so it could not possibly be cleaning up cruft like "./" in this instance). A clever reader may also note that the running refname buffer ("refs/foo/") is actually a subset of the filesystem path buffer (".git/refs/foo/"). We could get by with one buffer, indexing the length of $GIT_DIR when we want the refname. However, having tried this, the resulting code actually ends up a little more confusing, and the efficiency improvement is tiny (and almost certainly dwarfed by the system calls we are making). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index e70941a090..06f95c4c12 100644 --- a/refs.c +++ b/refs.c @@ -1352,19 +1352,23 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) { struct ref_cache *refs = dir->ref_cache; DIR *d; - const char *path; struct dirent *de; int dirnamelen = strlen(dirname); struct strbuf refname; + struct strbuf path = STRBUF_INIT; + size_t path_baselen; if (*refs->name) - path = git_path_submodule(refs->name, "%s", dirname); + strbuf_git_path_submodule(&path, refs->name, "%s", dirname); else - path = git_path("%s", dirname); + strbuf_git_path(&path, "%s", dirname); + path_baselen = path.len; - d = opendir(path); - if (!d) + d = opendir(path.buf); + if (!d) { + strbuf_release(&path); return; + } strbuf_init(&refname, dirnamelen + 257); strbuf_add(&refname, dirname, dirnamelen); @@ -1373,17 +1377,14 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) unsigned char sha1[20]; struct stat st; int flag; - const char *refdir; if (de->d_name[0] == '.') continue; if (ends_with(de->d_name, ".lock")) continue; strbuf_addstr(&refname, de->d_name); - refdir = *refs->name - ? git_path_submodule(refs->name, "%s", refname.buf) - : git_path("%s", refname.buf); - if (stat(refdir, &st) < 0) { + strbuf_addstr(&path, de->d_name); + if (stat(path.buf, &st) < 0) { ; /* silently ignore */ } else if (S_ISDIR(st.st_mode)) { strbuf_addch(&refname, '/'); @@ -1430,8 +1431,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) create_ref_entry(refname.buf, sha1, flag, 0)); } strbuf_setlen(&refname, dirnamelen); + strbuf_setlen(&path, path_baselen); } strbuf_release(&refname); + strbuf_release(&path); closedir(d); } From 07e3070d2aa69b0c8f1da5f103a87125ff4baaea Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:36:27 -0400 Subject: [PATCH 09/16] path.c: drop git_path_submodule There are no callers of the slightly-dangerous static-buffer git_path_submodule left. Let's drop it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 5 ++--- path.c | 10 ---------- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/cache.h b/cache.h index 6f74f33f41..7a4fa90e8c 100644 --- a/cache.h +++ b/cache.h @@ -713,12 +713,11 @@ extern int check_repository_format(void); * the repository directory (git_path), or in a submodule's repository * directory (git_path_submodule). In all cases, note that the result * may be overwritten by another call to _any_ of the functions. Consider - * using the safer "dup" or "strbuf" formats below. + * using the safer "dup" or "strbuf" formats below (in some cases, the + * unsafe versions have already been removed). */ extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2))); extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2))); -extern const char *git_path_submodule(const char *path, const char *fmt, ...) - __attribute__((format (printf, 2, 3))); extern char *mksnpath(char *buf, size_t n, const char *fmt, ...) __attribute__((format (printf, 3, 4))); diff --git a/path.c b/path.c index 9aad9a1878..94d7ec24e8 100644 --- a/path.c +++ b/path.c @@ -245,16 +245,6 @@ static void do_submodule_path(struct strbuf *buf, const char *path, strbuf_cleanup_path(buf); } -const char *git_path_submodule(const char *path, const char *fmt, ...) -{ - va_list args; - struct strbuf *buf = get_pathname(); - va_start(args, fmt); - do_submodule_path(buf, path, fmt, args); - va_end(args); - return buf->buf; -} - char *git_pathdup_submodule(const char *path, const char *fmt, ...) { va_list args; From 54b418f6985568bcb71c354ada204efe103b870e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 08:26:38 -0400 Subject: [PATCH 10/16] refs.c: simplify strbufs in reflog setup and writing Commit 1a83c24 (git_snpath(): retire and replace with strbuf_git_path(), 2014-11-30) taught log_ref_setup and log_ref_write_1 to take a strbuf parameter, rather than a bare string. It then makes an alias to the strbuf's "buf" field under the original name. This made the original diff much shorter, but the resulting code is more complicated that it needs to be. Since we've aliased the pointer, we drop our reference to the strbuf to ensure we don't accidentally change it. But if we simply drop our alias and use "logfile.buf" directly, we do not have to worry about this aliasing. It's a larger diff, but the resulting code is simpler. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/refs.c b/refs.c index 06f95c4c12..105f271e48 100644 --- a/refs.c +++ b/refs.c @@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname) * should_autocreate_reflog returns non-zero. Otherwise, create it * regardless of the ref name. Fill in *err and return -1 on failure. */ -static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create) +static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create) { int logfd, oflags = O_APPEND | O_WRONLY; - char *logfile; - strbuf_git_path(sb_logfile, "logs/%s", refname); - logfile = sb_logfile->buf; - /* make sure the rest of the function can't change "logfile" */ - sb_logfile = NULL; + strbuf_git_path(logfile, "logs/%s", refname); if (force_create || should_autocreate_reflog(refname)) { - if (safe_create_leading_directories(logfile) < 0) { + if (safe_create_leading_directories(logfile->buf) < 0) { strbuf_addf(err, "unable to create directory for %s: " - "%s", logfile, strerror(errno)); + "%s", logfile->buf, strerror(errno)); return -1; } oflags |= O_CREAT; } - logfd = open(logfile, oflags, 0666); + logfd = open(logfile->buf, oflags, 0666); if (logfd < 0) { if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR)) return 0; if (errno == EISDIR) { - if (remove_empty_directories(logfile)) { + if (remove_empty_directories(logfile->buf)) { strbuf_addf(err, "There are still logs under " - "'%s'", logfile); + "'%s'", logfile->buf); return -1; } - logfd = open(logfile, oflags, 0666); + logfd = open(logfile->buf, oflags, 0666); } if (logfd < 0) { strbuf_addf(err, "unable to append to %s: %s", - logfile, strerror(errno)); + logfile->buf, strerror(errno)); return -1; } } - adjust_shared_perm(logfile); + adjust_shared_perm(logfile->buf); close(logfd); return 0; } @@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1, static int log_ref_write_1(const char *refname, const unsigned char *old_sha1, const unsigned char *new_sha1, const char *msg, - struct strbuf *sb_log_file, int flags, + struct strbuf *logfile, int flags, struct strbuf *err) { int logfd, result, oflags = O_APPEND | O_WRONLY; - char *log_file; if (log_all_ref_updates < 0) log_all_ref_updates = !is_bare_repository(); - result = log_ref_setup(refname, sb_log_file, err, flags & REF_FORCE_CREATE_REFLOG); + result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG); if (result) return result; - log_file = sb_log_file->buf; - /* make sure the rest of the function can't change "log_file" */ - sb_log_file = NULL; - logfd = open(log_file, oflags); + logfd = open(logfile->buf, oflags); if (logfd < 0) return 0; result = log_ref_write_fd(logfd, old_sha1, new_sha1, git_committer_info(0), msg); if (result) { - strbuf_addf(err, "unable to append to %s: %s", log_file, + strbuf_addf(err, "unable to append to %s: %s", logfile->buf, strerror(errno)); close(logfd); return -1; } if (close(logfd)) { - strbuf_addf(err, "unable to append to %s: %s", log_file, + strbuf_addf(err, "unable to append to %s: %s", logfile->buf, strerror(errno)); return -1; } From d6549f3655212bd2de52df0137ceb59180424061 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:36:53 -0400 Subject: [PATCH 11/16] refs.c: avoid repeated git_path calls in rename_tmp_log Because it's not safe to store the static-buffer results of git_path for a long time, we end up formatting the same filename over and over. We can fix this by using a function-local strbuf to store the formatted pathname and avoid repeating ourselves. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/refs.c b/refs.c index 105f271e48..086fb1ad02 100644 --- a/refs.c +++ b/refs.c @@ -2930,9 +2930,13 @@ out: static int rename_tmp_log(const char *newrefname) { int attempts_remaining = 4; + struct strbuf path = STRBUF_INIT; + int ret = -1; retry: - switch (safe_create_leading_directories_const(git_path("logs/%s", newrefname))) { + strbuf_reset(&path); + strbuf_git_path(&path, "logs/%s", newrefname); + switch (safe_create_leading_directories_const(path.buf)) { case SCLD_OK: break; /* success */ case SCLD_VANISHED: @@ -2941,19 +2945,19 @@ static int rename_tmp_log(const char *newrefname) /* fall through */ default: error("unable to create directory for %s", newrefname); - return -1; + goto out; } - if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname))) { + if (rename(git_path(TMP_RENAMED_LOG), path.buf)) { if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) { /* * rename(a, b) when b is an existing * directory ought to result in ISDIR, but * Solaris 5.8 gives ENOTDIR. Sheesh. */ - if (remove_empty_directories(git_path("logs/%s", newrefname))) { + if (remove_empty_directories(path.buf)) { error("Directory not empty: logs/%s", newrefname); - return -1; + goto out; } goto retry; } else if (errno == ENOENT && --attempts_remaining > 0) { @@ -2966,10 +2970,13 @@ static int rename_tmp_log(const char *newrefname) } else { error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s", newrefname, strerror(errno)); - return -1; + goto out; } } - return 0; + ret = 0; +out: + strbuf_release(&path); + return ret; } static int rename_ref_available(const char *oldname, const char *newname) From 5f8ef5b84889e7792e929a0fc773cb0060a0a611 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:37:12 -0400 Subject: [PATCH 12/16] refs.c: avoid git_path assignment in lock_ref_sha1_basic Assigning the result of git_path is a bad pattern, because it's not immediately obvious how long you expect the content to stay valid (and it may be overwritten by subsequent calls). Let's use a function-local strbuf here instead, which we know is safe (we just have to remember to free it in all code paths). As a bonus, we get rid of a confusing variable-reuse ("ref_file" is used for two distinct purposes). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/refs.c b/refs.c index 086fb1ad02..4ed9f6b048 100644 --- a/refs.c +++ b/refs.c @@ -2408,7 +2408,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, unsigned int flags, int *type_p, struct strbuf *err) { - const char *ref_file; + struct strbuf ref_file = STRBUF_INIT; + struct strbuf orig_ref_file = STRBUF_INIT; const char *orig_refname = refname; struct ref_lock *lock; int last_errno = 0; @@ -2432,20 +2433,19 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, refname = resolve_ref_unsafe(refname, resolve_flags, lock->old_oid.hash, &type); if (!refname && errno == EISDIR) { - /* we are trying to lock foo but we used to + /* + * we are trying to lock foo but we used to * have foo/bar which now does not exist; * it is normal for the empty directory 'foo' * to remain. */ - ref_file = git_path("%s", orig_refname); - if (remove_empty_directories(ref_file)) { + strbuf_git_path(&orig_ref_file, "%s", orig_refname); + if (remove_empty_directories(orig_ref_file.buf)) { last_errno = errno; - if (!verify_refname_available(orig_refname, extras, skip, get_loose_refs(&ref_cache), err)) strbuf_addf(err, "there are still refs under '%s'", orig_refname); - goto error_return; } refname = resolve_ref_unsafe(orig_refname, resolve_flags, @@ -2485,10 +2485,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, } lock->ref_name = xstrdup(refname); lock->orig_ref_name = xstrdup(orig_refname); - ref_file = git_path("%s", refname); + strbuf_git_path(&ref_file, "%s", refname); retry: - switch (safe_create_leading_directories_const(ref_file)) { + switch (safe_create_leading_directories_const(ref_file.buf)) { case SCLD_OK: break; /* success */ case SCLD_VANISHED: @@ -2497,11 +2497,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, /* fall through */ default: last_errno = errno; - strbuf_addf(err, "unable to create directory for %s", ref_file); + strbuf_addf(err, "unable to create directory for %s", + ref_file.buf); goto error_return; } - if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) { + if (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) { last_errno = errno; if (errno == ENOENT && --attempts_remaining > 0) /* @@ -2511,7 +2512,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, */ goto retry; else { - unable_to_lock_message(ref_file, errno, err); + unable_to_lock_message(ref_file.buf, errno, err); goto error_return; } } @@ -2519,12 +2520,17 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, last_errno = errno; goto error_return; } - return lock; + goto out; error_return: unlock_ref(lock); + lock = NULL; + + out: + strbuf_release(&ref_file); + strbuf_release(&orig_ref_file); errno = last_errno; - return NULL; + return lock; } /* From 470e28d4e16dd994cd985914377fa8ccb5f86227 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:37:27 -0400 Subject: [PATCH 13/16] refs.c: remove_empty_directories can take a strbuf The first thing we do in this function is copy the input into a strbuf. Of the 4 callers, 3 of them already have a strbuf we could use. Let's just take the strbuf, and convert the remaining caller to use a strbuf, rather than a raw git_path. This is safer, anyway, as remove_dir_recursively is a non-trivial function that might use the pathname buffers itself (this is _probably_ OK, as the likely culprit would be calling resolve_gitlink_ref, but we do not pass the proper flags to ask it to avoid blowing away gitlinks). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/refs.c b/refs.c index 4ed9f6b048..84b1aff842 100644 --- a/refs.c +++ b/refs.c @@ -2290,25 +2290,14 @@ static int verify_lock(struct ref_lock *lock, return 0; } -static int remove_empty_directories(const char *file) +static int remove_empty_directories(struct strbuf *path) { - /* we want to create a file but there is a directory there; + /* + * we want to create a file but there is a directory there; * if that is an empty directory (or a directory that contains * only empty directories), remove them. */ - struct strbuf path; - int result, save_errno; - - strbuf_init(&path, 20); - strbuf_addstr(&path, file); - - result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); - save_errno = errno; - - strbuf_release(&path); - errno = save_errno; - - return result; + return remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY); } /* @@ -2440,7 +2429,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, * to remain. */ strbuf_git_path(&orig_ref_file, "%s", orig_refname); - if (remove_empty_directories(orig_ref_file.buf)) { + if (remove_empty_directories(&orig_ref_file)) { last_errno = errno; if (!verify_refname_available(orig_refname, extras, skip, get_loose_refs(&ref_cache), err)) @@ -2961,7 +2950,7 @@ static int rename_tmp_log(const char *newrefname) * directory ought to result in ISDIR, but * Solaris 5.8 gives ENOTDIR. Sheesh. */ - if (remove_empty_directories(path.buf)) { + if (remove_empty_directories(&path)) { error("Directory not empty: logs/%s", newrefname); goto out; } @@ -3046,7 +3035,14 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) && delete_ref(newrefname, sha1, REF_NODEREF)) { if (errno==EISDIR) { - if (remove_empty_directories(git_path("%s", newrefname))) { + struct strbuf path = STRBUF_INIT; + int result; + + strbuf_git_path(&path, "%s", newrefname); + result = remove_empty_directories(&path); + strbuf_release(&path); + + if (result) { error("Directory not empty: %s", newrefname); goto rollback; } @@ -3183,7 +3179,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str return 0; if (errno == EISDIR) { - if (remove_empty_directories(logfile->buf)) { + if (remove_empty_directories(logfile)) { strbuf_addf(err, "There are still logs under " "'%s'", logfile->buf); return -1; From 03f2c7731b59de75357730bc511ffa8847e1fb81 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:37:45 -0400 Subject: [PATCH 14/16] find_hook: keep our own static buffer The find_hook function returns the results of git_path, which is a static buffer shared by other path-related calls. Returning such a buffer is slightly dangerous, because it can be overwritten by seemingly unrelated functions. Let's at least keep our _own_ static buffer, so you can only get in trouble by calling find_hook in quick succession, which is less likely to happen and more obvious to notice. While we're at it, let's add some documentation of the function's limitations. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- run-command.c | 10 ++++++---- run-command.h | 5 +++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/run-command.c b/run-command.c index 4d73e90fad..28e1d55cb9 100644 --- a/run-command.c +++ b/run-command.c @@ -797,11 +797,13 @@ int finish_async(struct async *async) const char *find_hook(const char *name) { - const char *path = git_path("hooks/%s", name); - if (access(path, X_OK) < 0) - path = NULL; + static struct strbuf path = STRBUF_INIT; - return path; + strbuf_reset(&path); + strbuf_git_path(&path, "hooks/%s", name); + if (access(path.buf, X_OK) < 0) + return NULL; + return path.buf; } int run_hook_ve(const char *const *env, const char *name, va_list args) diff --git a/run-command.h b/run-command.h index 1103805af1..5b4425a3cb 100644 --- a/run-command.h +++ b/run-command.h @@ -52,6 +52,11 @@ int start_command(struct child_process *); int finish_command(struct child_process *); int run_command(struct child_process *); +/* + * Returns the path to the hook file, or NULL if the hook is missing + * or disabled. Note that this points to static storage that will be + * overwritten by further calls to find_hook and run_hook_*. + */ extern const char *find_hook(const char *name); LAST_ARG_MUST_BE_NULL extern int run_hook_le(const char *const *env, const char *name, ...); From 0ea68e429647cc5400fe8fa257056083b0a6459d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:37:55 -0400 Subject: [PATCH 15/16] get_repo_path: refactor path-allocation The get_repo_path function calls mkpath() and then does some non-trivial operations on it, like calling is_git_directory() and read_gitfile(). These are actually OK (they do not use more pathname static buffers themselves), but it takes a fair bit of work to verify. Let's use our own strbuf to store the path, and we can simply reuse it for each iteration of the loop (we can even avoid rewriting the beginning part, since we are trying a series of suffixes). To make the strbuf cleanup easier, we split out a thin wrapper. As a bonus, this wrapper can factor out the canonicalization that happens in all of the early-return code paths. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/clone.c | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 303a3a7eb6..5864ad15e1 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -99,51 +99,66 @@ static const char *argv_submodule[] = { "submodule", "update", "--init", "--recursive", NULL }; -static char *get_repo_path(const char *repo, int *is_bundle) +static const char *get_repo_path_1(struct strbuf *path, int *is_bundle) { static char *suffix[] = { "/.git", "", ".git/.git", ".git" }; static char *bundle_suffix[] = { ".bundle", "" }; + size_t baselen = path->len; struct stat st; int i; for (i = 0; i < ARRAY_SIZE(suffix); i++) { - const char *path; - path = mkpath("%s%s", repo, suffix[i]); - if (stat(path, &st)) + strbuf_setlen(path, baselen); + strbuf_addstr(path, suffix[i]); + if (stat(path->buf, &st)) continue; - if (S_ISDIR(st.st_mode) && is_git_directory(path)) { + if (S_ISDIR(st.st_mode) && is_git_directory(path->buf)) { *is_bundle = 0; - return xstrdup(absolute_path(path)); + return path->buf; } else if (S_ISREG(st.st_mode) && st.st_size > 8) { /* Is it a "gitfile"? */ char signature[8]; - int len, fd = open(path, O_RDONLY); + const char *dst; + int len, fd = open(path->buf, O_RDONLY); if (fd < 0) continue; len = read_in_full(fd, signature, 8); close(fd); if (len != 8 || strncmp(signature, "gitdir: ", 8)) continue; - path = read_gitfile(path); - if (path) { + dst = read_gitfile(path->buf); + if (dst) { *is_bundle = 0; - return xstrdup(absolute_path(path)); + return dst; } } } for (i = 0; i < ARRAY_SIZE(bundle_suffix); i++) { - const char *path; - path = mkpath("%s%s", repo, bundle_suffix[i]); - if (!stat(path, &st) && S_ISREG(st.st_mode)) { + strbuf_setlen(path, baselen); + strbuf_addstr(path, bundle_suffix[i]); + if (!stat(path->buf, &st) && S_ISREG(st.st_mode)) { *is_bundle = 1; - return xstrdup(absolute_path(path)); + return path->buf; } } return NULL; } +static char *get_repo_path(const char *repo, int *is_bundle) +{ + struct strbuf path = STRBUF_INIT; + const char *raw; + char *canon; + + strbuf_addstr(&path, repo); + raw = get_repo_path_1(&path, is_bundle); + canon = raw ? xstrdup(absolute_path(raw)) : NULL; + strbuf_release(&path); + return canon; +} + static char *guess_dir_name(const char *repo, int is_bundle, int is_bare) { const char *end = repo + strlen(repo), *start; From f932729cc7707390f4d6739be1573e93ceb9df22 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Aug 2015 05:38:57 -0400 Subject: [PATCH 16/16] memoize common git-path "constant" files One of the most common uses of git_path() is to pass a constant, like git_path("MERGE_MSG"). This has two drawbacks: 1. The return value is a static buffer, and the lifetime is dependent on other calls to git_path, etc. 2. There's no compile-time checking of the pathname. This is OK for a one-off (after all, we have to spell it correctly at least once), but many of these constant strings appear throughout the code. This patch introduces a series of functions to "memoize" these strings, which are essentially globals for the lifetime of the program. We compute the value once, take ownership of the buffer, and return the cached value for subsequent calls. cache.h provides a helper macro for defining these functions as one-liners, and defines a few common ones for global use. Using a macro is a little bit gross, but it does nicely document the purpose of the functions. If we need to touch them all later (e.g., because we learned how to change the git_dir variable at runtime, and need to invalidate all of the stored values), it will be much easier to have the complete list. Note that the shared-global functions have separate, manual declarations. We could do something clever with the macros (e.g., expand it to a declaration in some places, and a declaration _and_ a definition in path.c). But there aren't that many, and it's probably better to stay away from too-magical macros. Likewise, if we abandon the C preprocessor in favor of generating these with a script, we could get much fancier. E.g., normalizing "FOO/BAR-BAZ" into "git_path_foo_bar_baz". But the small amount of saved typing is probably not worth the resulting confusion to readers who want to grep for the function's definition. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- attr.c | 4 +- bisect.c | 7 ++- branch.c | 14 ++--- builtin/blame.c | 7 +-- builtin/commit.c | 32 +++++----- builtin/fetch.c | 4 +- builtin/merge.c | 30 ++++----- builtin/reset.c | 2 +- cache.h | 26 ++++++++ contrib/examples/builtin-fetch--tool.c | 4 +- dir.c | 4 +- fetch-pack.c | 2 +- path.c | 10 +++ rerere.c | 19 +++--- sequencer.c | 87 ++++++++++++-------------- shallow.c | 10 +-- wt-status.c | 8 +-- 17 files changed, 151 insertions(+), 119 deletions(-) diff --git a/attr.c b/attr.c index 8f2ac6c88c..086c08dcfa 100644 --- a/attr.c +++ b/attr.c @@ -490,6 +490,8 @@ static int git_attr_system(void) return !git_env_bool("GIT_ATTR_NOSYSTEM", 0); } +static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE) + static void bootstrap_attr_stack(void) { struct attr_stack *elem; @@ -531,7 +533,7 @@ static void bootstrap_attr_stack(void) debug_push(elem); } - elem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1); + elem = read_attr_from_file(git_path_info_attributes(), 1); if (!elem) elem = xcalloc(1, sizeof(*elem)); elem->origin = NULL; diff --git a/bisect.c b/bisect.c index 03d5cd9454..7a6498c8e9 100644 --- a/bisect.c +++ b/bisect.c @@ -420,10 +420,13 @@ static int read_bisect_refs(void) return for_each_ref_in("refs/bisect/", register_ref, NULL); } +static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") +static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") + static void read_bisect_paths(struct argv_array *array) { struct strbuf str = STRBUF_INIT; - const char *filename = git_path("BISECT_NAMES"); + const char *filename = git_path_bisect_names(); FILE *fp = fopen(filename, "r"); if (!fp) @@ -644,7 +647,7 @@ static void exit_if_skipped_commits(struct commit_list *tried, static int is_expected_rev(const struct object_id *oid) { - const char *filename = git_path("BISECT_EXPECTED_REV"); + const char *filename = git_path_bisect_expected_rev(); struct stat st; struct strbuf str = STRBUF_INIT; FILE *fp; diff --git a/branch.c b/branch.c index b0024353f4..e283683d9d 100644 --- a/branch.c +++ b/branch.c @@ -302,11 +302,11 @@ void create_branch(const char *head, void remove_branch_state(void) { - unlink(git_path("CHERRY_PICK_HEAD")); - unlink(git_path("REVERT_HEAD")); - unlink(git_path("MERGE_HEAD")); - unlink(git_path("MERGE_RR")); - unlink(git_path("MERGE_MSG")); - unlink(git_path("MERGE_MODE")); - unlink(git_path("SQUASH_MSG")); + unlink(git_path_cherry_pick_head()); + unlink(git_path_revert_head()); + unlink(git_path_merge_head()); + unlink(git_path_merge_rr()); + unlink(git_path_merge_msg()); + unlink(git_path_merge_mode()); + unlink(git_path_squash_msg()); } diff --git a/builtin/blame.c b/builtin/blame.c index 04e4864f8b..4db01c195c 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2227,20 +2227,19 @@ static struct commit_list **append_parent(struct commit_list **tail, const unsig static void append_merge_parents(struct commit_list **tail) { int merge_head; - const char *merge_head_file = git_path("MERGE_HEAD"); struct strbuf line = STRBUF_INIT; - merge_head = open(merge_head_file, O_RDONLY); + merge_head = open(git_path_merge_head(), O_RDONLY); if (merge_head < 0) { if (errno == ENOENT) return; - die("cannot open '%s' for reading", merge_head_file); + die("cannot open '%s' for reading", git_path_merge_head()); } while (!strbuf_getwholeline_fd(&line, merge_head, '\n')) { unsigned char sha1[20]; if (line.len < 40 || get_sha1_hex(line.buf, sha1)) - die("unknown line in '%s': %s", merge_head_file, line.buf); + die("unknown line in '%s': %s", git_path_merge_head(), line.buf); tail = append_parent(tail, sha1); } close(merge_head); diff --git a/builtin/commit.c b/builtin/commit.c index 7314f33885..4cbd5ff4de 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -166,9 +166,9 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset) static void determine_whence(struct wt_status *s) { - if (file_exists(git_path("MERGE_HEAD"))) + if (file_exists(git_path_merge_head())) whence = FROM_MERGE; - else if (file_exists(git_path("CHERRY_PICK_HEAD"))) { + else if (file_exists(git_path_cherry_pick_head())) { whence = FROM_CHERRY_PICK; if (file_exists(git_path(SEQ_DIR))) sequencer_in_use = 1; @@ -725,12 +725,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix, format_commit_message(commit, "fixup! %s\n\n", &sb, &ctx); hook_arg1 = "message"; - } else if (!stat(git_path("MERGE_MSG"), &statbuf)) { - if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0) + } else if (!stat(git_path_merge_msg(), &statbuf)) { + if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0) die_errno(_("could not read MERGE_MSG")); hook_arg1 = "merge"; - } else if (!stat(git_path("SQUASH_MSG"), &statbuf)) { - if (strbuf_read_file(&sb, git_path("SQUASH_MSG"), 0) < 0) + } else if (!stat(git_path_squash_msg(), &statbuf)) { + if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0) die_errno(_("could not read SQUASH_MSG")); hook_arg1 = "squash"; } else if (template_file) { @@ -1684,10 +1684,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (!reflog_msg) reflog_msg = "commit (merge)"; pptr = &commit_list_insert(current_head, pptr)->next; - fp = fopen(git_path("MERGE_HEAD"), "r"); + fp = fopen(git_path_merge_head(), "r"); if (fp == NULL) die_errno(_("could not open '%s' for reading"), - git_path("MERGE_HEAD")); + git_path_merge_head()); while (strbuf_getline(&m, fp, '\n') != EOF) { struct commit *parent; @@ -1698,8 +1698,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) } fclose(fp); strbuf_release(&m); - if (!stat(git_path("MERGE_MODE"), &statbuf)) { - if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0) + if (!stat(git_path_merge_mode(), &statbuf)) { + if (strbuf_read_file(&sb, git_path_merge_mode(), 0) < 0) die_errno(_("could not read MERGE_MODE")); if (!strcmp(sb.buf, "no-ff")) allow_fast_forward = 0; @@ -1775,12 +1775,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix) } ref_transaction_free(transaction); - unlink(git_path("CHERRY_PICK_HEAD")); - unlink(git_path("REVERT_HEAD")); - unlink(git_path("MERGE_HEAD")); - unlink(git_path("MERGE_MSG")); - unlink(git_path("MERGE_MODE")); - unlink(git_path("SQUASH_MSG")); + unlink(git_path_cherry_pick_head()); + unlink(git_path_revert_head()); + unlink(git_path_merge_head()); + unlink(git_path_merge_msg()); + unlink(git_path_merge_mode()); + unlink(git_path_squash_msg()); if (commit_index_files()) die (_("Repository has been updated, but unable to write\n" diff --git a/builtin/fetch.c b/builtin/fetch.c index 34b6f5ebca..df16d0a56e 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -591,7 +591,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, const char *what, *kind; struct ref *rm; char *url; - const char *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD"); + const char *filename = dry_run ? "/dev/null" : git_path_fetch_head(); int want_status; fp = fopen(filename, "a"); @@ -834,7 +834,7 @@ static void check_not_current_branch(struct ref *ref_map) static int truncate_fetch_head(void) { - const char *filename = git_path("FETCH_HEAD"); + const char *filename = git_path_fetch_head(); FILE *fp = fopen(filename, "w"); if (!fp) diff --git a/builtin/merge.c b/builtin/merge.c index 85c54dcd5a..a0edacab20 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -231,9 +231,9 @@ static struct option builtin_merge_options[] = { /* Cleans up metadata that is uninteresting after a succeeded merge. */ static void drop_save(void) { - unlink(git_path("MERGE_HEAD")); - unlink(git_path("MERGE_MSG")); - unlink(git_path("MERGE_MODE")); + unlink(git_path_merge_head()); + unlink(git_path_merge_msg()); + unlink(git_path_merge_mode()); } static int save_state(unsigned char *stash) @@ -338,7 +338,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead struct pretty_print_context ctx = {0}; printf(_("Squash commit -- not updating HEAD\n")); - filename = git_path("SQUASH_MSG"); + filename = git_path_squash_msg(); fd = open(filename, O_WRONLY | O_CREAT, 0666); if (fd < 0) die_errno(_("Could not write to '%s'"), filename); @@ -754,7 +754,7 @@ static void add_strategies(const char *string, unsigned attr) static void write_merge_msg(struct strbuf *msg) { - const char *filename = git_path("MERGE_MSG"); + const char *filename = git_path_merge_msg(); int fd = open(filename, O_WRONLY | O_CREAT, 0666); if (fd < 0) die_errno(_("Could not open '%s' for writing"), @@ -766,7 +766,7 @@ static void write_merge_msg(struct strbuf *msg) static void read_merge_msg(struct strbuf *msg) { - const char *filename = git_path("MERGE_MSG"); + const char *filename = git_path_merge_msg(); strbuf_reset(msg); if (strbuf_read_file(msg, filename, 0) < 0) die_errno(_("Could not read from '%s'"), filename); @@ -799,10 +799,10 @@ static void prepare_to_commit(struct commit_list *remoteheads) strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char); write_merge_msg(&msg); if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg", - git_path("MERGE_MSG"), "merge", NULL)) + git_path_merge_msg(), "merge", NULL)) abort_commit(remoteheads, NULL); if (0 < option_edit) { - if (launch_editor(git_path("MERGE_MSG"), NULL, NULL)) + if (launch_editor(git_path_merge_msg(), NULL, NULL)) abort_commit(remoteheads, NULL); } read_merge_msg(&msg); @@ -865,7 +865,7 @@ static int suggest_conflicts(void) FILE *fp; struct strbuf msgbuf = STRBUF_INIT; - filename = git_path("MERGE_MSG"); + filename = git_path_merge_msg(); fp = fopen(filename, "a"); if (!fp) die_errno(_("Could not open '%s' for writing"), filename); @@ -967,7 +967,7 @@ static void write_merge_state(struct commit_list *remoteheads) } strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1)); } - filename = git_path("MERGE_HEAD"); + filename = git_path_merge_head(); fd = open(filename, O_WRONLY | O_CREAT, 0666); if (fd < 0) die_errno(_("Could not open '%s' for writing"), filename); @@ -977,7 +977,7 @@ static void write_merge_state(struct commit_list *remoteheads) strbuf_addch(&merge_msg, '\n'); write_merge_msg(&merge_msg); - filename = git_path("MERGE_MODE"); + filename = git_path_merge_mode(); fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666); if (fd < 0) die_errno(_("Could not open '%s' for writing"), filename); @@ -1070,7 +1070,7 @@ static void handle_fetch_head(struct commit_list **remotes, struct strbuf *merge if (!merge_names) merge_names = &fetch_head_file; - filename = git_path("FETCH_HEAD"); + filename = git_path_fetch_head(); fd = open(filename, O_RDONLY); if (fd < 0) die_errno(_("could not open '%s' for reading"), filename); @@ -1204,7 +1204,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) int nargc = 2; const char *nargv[] = {"reset", "--merge", NULL}; - if (!file_exists(git_path("MERGE_HEAD"))) + if (!file_exists(git_path_merge_head())) die(_("There is no merge to abort (MERGE_HEAD missing).")); /* Invoke 'git reset --merge' */ @@ -1215,7 +1215,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (read_cache_unmerged()) die_resolve_conflict("merge"); - if (file_exists(git_path("MERGE_HEAD"))) { + if (file_exists(git_path_merge_head())) { /* * There is no unmerged entry, don't advise 'git * add/rm ', just 'git commit'. @@ -1226,7 +1226,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) else die(_("You have not concluded your merge (MERGE_HEAD exists).")); } - if (file_exists(git_path("CHERRY_PICK_HEAD"))) { + if (file_exists(git_path_cherry_pick_head())) { if (advice_resolve_conflict) die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n" "Please, commit your changes before you merge.")); diff --git a/builtin/reset.c b/builtin/reset.c index 4c08ddc1ca..c503e75a59 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -36,7 +36,7 @@ static const char *reset_type_names[] = { static inline int is_merge(void) { - return !access(git_path("MERGE_HEAD"), F_OK); + return !access(git_path_merge_head(), F_OK); } static int reset_index(const unsigned char *sha1, int reset_type, int quiet) diff --git a/cache.h b/cache.h index 7a4fa90e8c..402521e34d 100644 --- a/cache.h +++ b/cache.h @@ -735,6 +735,32 @@ extern char *git_pathdup_submodule(const char *path, const char *fmt, ...) extern void report_linked_checkout_garbage(void); +/* + * You can define a static memoized git path like: + * + * static GIT_PATH_FUNC(git_path_foo, "FOO"); + * + * or use one of the global ones below. + */ +#define GIT_PATH_FUNC(func, filename) \ + const char *func(void) \ + { \ + static char *ret; \ + if (!ret) \ + ret = git_pathdup(filename); \ + return ret; \ + } + +const char *git_path_cherry_pick_head(void); +const char *git_path_revert_head(void); +const char *git_path_squash_msg(void); +const char *git_path_merge_msg(void); +const char *git_path_merge_rr(void); +const char *git_path_merge_mode(void); +const char *git_path_merge_head(void); +const char *git_path_fetch_head(void); +const char *git_path_shallow(void); + /* * Return the name of the file in the local object database that would * be used to store a loose object with the specified sha1. The diff --git a/contrib/examples/builtin-fetch--tool.c b/contrib/examples/builtin-fetch--tool.c index ee1916641e..a3eb19de04 100644 --- a/contrib/examples/builtin-fetch--tool.c +++ b/contrib/examples/builtin-fetch--tool.c @@ -516,7 +516,7 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix) if (argc != 8) return error("append-fetch-head takes 6 args"); - filename = git_path("FETCH_HEAD"); + filename = git_path_fetch_head(); fp = fopen(filename, "a"); if (!fp) return error("cannot open %s: %s", filename, strerror(errno)); @@ -534,7 +534,7 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix) if (argc != 5) return error("fetch-native-store takes 3 args"); - filename = git_path("FETCH_HEAD"); + filename = git_path_fetch_head(); fp = fopen(filename, "a"); if (!fp) return error("cannot open %s: %s", filename, strerror(errno)); diff --git a/dir.c b/dir.c index 1d42811cfb..1ac8d5dd28 100644 --- a/dir.c +++ b/dir.c @@ -2185,6 +2185,8 @@ int remove_dir_recursively(struct strbuf *path, int flag) return remove_dir_recurse(path, flag, NULL); } +static GIT_PATH_FUNC(git_path_info_exclude, "info/exclude") + void setup_standard_excludes(struct dir_struct *dir) { const char *path; @@ -2199,7 +2201,7 @@ void setup_standard_excludes(struct dir_struct *dir) dir->untracked ? &dir->ss_excludes_file : NULL); /* per repository user preference */ - path = git_path("info/exclude"); + path = git_path_info_exclude(); if (!access_or_warn(path, R_OK, 0)) add_excludes_from_file_1(dir, path, dir->untracked ? &dir->ss_info_exclude : NULL); diff --git a/fetch-pack.c b/fetch-pack.c index a136772152..820251a8d8 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -948,7 +948,7 @@ static void update_shallow(struct fetch_pack_args *args, if (args->depth > 0 && alternate_shallow_file) { if (*alternate_shallow_file == '\0') { /* --unshallow */ - unlink_or_warn(git_path("shallow")); + unlink_or_warn(git_path_shallow()); rollback_lock_file(&shallow_lock); } else commit_lock_file(&shallow_lock); diff --git a/path.c b/path.c index 94d7ec24e8..95acbafa68 100644 --- a/path.c +++ b/path.c @@ -933,3 +933,13 @@ char *xdg_config_home(const char *filename) return mkpathdup("%s/.config/git/%s", home, filename); return NULL; } + +GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD") +GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD") +GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG") +GIT_PATH_FUNC(git_path_merge_msg, "MERGE_MSG") +GIT_PATH_FUNC(git_path_merge_rr, "MERGE_RR") +GIT_PATH_FUNC(git_path_merge_mode, "MERGE_MODE") +GIT_PATH_FUNC(git_path_merge_head, "MERGE_HEAD") +GIT_PATH_FUNC(git_path_fetch_head, "FETCH_HEAD") +GIT_PATH_FUNC(git_path_shallow, "shallow") diff --git a/rerere.c b/rerere.c index 94aea9a36f..6a517aa383 100644 --- a/rerere.c +++ b/rerere.c @@ -20,8 +20,6 @@ static int rerere_enabled = -1; /* automatically update cleanly resolved paths to the index */ static int rerere_autoupdate; -static char *merge_rr_path; - const char *rerere_path(const char *hex, const char *file) { return git_path("rr-cache/%s/%s", hex, file); @@ -37,7 +35,7 @@ static void read_rr(struct string_list *rr) { unsigned char sha1[20]; char buf[PATH_MAX]; - FILE *in = fopen(merge_rr_path, "r"); + FILE *in = fopen(git_path_merge_rr(), "r"); if (!in) return; while (fread(buf, 40, 1, in) == 1) { @@ -577,21 +575,21 @@ static void git_rerere_config(void) git_config(git_default_config, NULL); } +static GIT_PATH_FUNC(git_path_rr_cache, "rr-cache") + static int is_rerere_enabled(void) { - const char *rr_cache; int rr_cache_exists; if (!rerere_enabled) return 0; - rr_cache = git_path("rr-cache"); - rr_cache_exists = is_directory(rr_cache); + rr_cache_exists = is_directory(git_path_rr_cache()); if (rerere_enabled < 0) return rr_cache_exists; - if (!rr_cache_exists && mkdir_in_gitdir(rr_cache)) - die("Could not create directory %s", rr_cache); + if (!rr_cache_exists && mkdir_in_gitdir(git_path_rr_cache())) + die("Could not create directory %s", git_path_rr_cache()); return 1; } @@ -605,8 +603,7 @@ int setup_rerere(struct string_list *merge_rr, int flags) if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE)) rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE); - merge_rr_path = git_pathdup("MERGE_RR"); - fd = hold_lock_file_for_update(&write_lock, merge_rr_path, + fd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(), LOCK_DIE_ON_ERROR); read_rr(merge_rr); return fd; @@ -741,5 +738,5 @@ void rerere_clear(struct string_list *merge_rr) if (!has_rerere_resolution(name)) unlink_rr_item(name); } - unlink_or_warn(git_path("MERGE_RR")); + unlink_or_warn(git_path_merge_rr()); } diff --git a/sequencer.c b/sequencer.c index c4f4b7d571..147adfac81 100644 --- a/sequencer.c +++ b/sequencer.c @@ -21,6 +21,11 @@ const char sign_off_header[] = "Signed-off-by: "; static const char cherry_picked_prefix[] = "(cherry picked from commit "; +static GIT_PATH_FUNC(git_path_todo_file, SEQ_TODO_FILE) +static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE) +static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR) +static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE) + static int is_rfc2822_line(const char *buf, int len) { int i; @@ -186,7 +191,7 @@ static void print_advice(int show_hint, struct replay_opts *opts) * (typically rebase --interactive) wants to take care * of the commit itself so remove CHERRY_PICK_HEAD */ - unlink(git_path("CHERRY_PICK_HEAD")); + unlink(git_path_cherry_pick_head()); return; } @@ -467,7 +472,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) struct commit *base, *next, *parent; const char *base_label, *next_label; struct commit_message msg = { NULL, NULL, NULL, NULL }; - char *defmsg = NULL; struct strbuf msgbuf = STRBUF_INIT; int res, unborn = 0, allow; @@ -537,8 +541,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * reverse of it if we are revert. */ - defmsg = git_pathdup("MERGE_MSG"); - if (opts->action == REPLAY_REVERT) { base = commit; base_label = msg.label; @@ -585,12 +587,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) { res = do_recursive_merge(base, next, base_label, next_label, head, &msgbuf, opts); - write_message(&msgbuf, defmsg); + write_message(&msgbuf, git_path_merge_msg()); } else { struct commit_list *common = NULL; struct commit_list *remotes = NULL; - write_message(&msgbuf, defmsg); + write_message(&msgbuf, git_path_merge_msg()); commit_list_insert(base, &common); commit_list_insert(next, &remotes); @@ -628,11 +630,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) goto leave; } if (!opts->no_commit) - res = run_git_commit(defmsg, opts, allow); + res = run_git_commit(git_path_merge_msg(), opts, allow); leave: free_message(commit, &msg); - free(defmsg); return res; } @@ -756,24 +757,23 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list, static void read_populate_todo(struct commit_list **todo_list, struct replay_opts *opts) { - const char *todo_file = git_path(SEQ_TODO_FILE); struct strbuf buf = STRBUF_INIT; int fd, res; - fd = open(todo_file, O_RDONLY); + fd = open(git_path_todo_file(), O_RDONLY); if (fd < 0) - die_errno(_("Could not open %s"), todo_file); + die_errno(_("Could not open %s"), git_path_todo_file()); if (strbuf_read(&buf, fd, 0) < 0) { close(fd); strbuf_release(&buf); - die(_("Could not read %s."), todo_file); + die(_("Could not read %s."), git_path_todo_file()); } close(fd); res = parse_insn_buffer(buf.buf, todo_list, opts); strbuf_release(&buf); if (res) - die(_("Unusable instruction sheet: %s"), todo_file); + die(_("Unusable instruction sheet: %s"), git_path_todo_file()); } static int populate_opts_cb(const char *key, const char *value, void *data) @@ -813,12 +813,10 @@ static int populate_opts_cb(const char *key, const char *value, void *data) static void read_populate_opts(struct replay_opts **opts_ptr) { - const char *opts_file = git_path(SEQ_OPTS_FILE); - - if (!file_exists(opts_file)) + if (!file_exists(git_path_opts_file())) return; - if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0) - die(_("Malformed options sheet: %s"), opts_file); + if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0) + die(_("Malformed options sheet: %s"), git_path_opts_file()); } static void walk_revs_populate_todo(struct commit_list **todo_list, @@ -836,31 +834,29 @@ static void walk_revs_populate_todo(struct commit_list **todo_list, static int create_seq_dir(void) { - const char *seq_dir = git_path(SEQ_DIR); - - if (file_exists(seq_dir)) { + if (file_exists(git_path_seq_dir())) { error(_("a cherry-pick or revert is already in progress")); advise(_("try \"git cherry-pick (--continue | --quit | --abort)\"")); return -1; } - else if (mkdir(seq_dir, 0777) < 0) - die_errno(_("Could not create sequencer directory %s"), seq_dir); + else if (mkdir(git_path_seq_dir(), 0777) < 0) + die_errno(_("Could not create sequencer directory %s"), + git_path_seq_dir()); return 0; } static void save_head(const char *head) { - const char *head_file = git_path(SEQ_HEAD_FILE); static struct lock_file head_lock; struct strbuf buf = STRBUF_INIT; int fd; - fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR); + fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR); strbuf_addf(&buf, "%s\n", head); if (write_in_full(fd, buf.buf, buf.len) < 0) - die_errno(_("Could not write to %s"), head_file); + die_errno(_("Could not write to %s"), git_path_head_file()); if (commit_lock_file(&head_lock) < 0) - die(_("Error wrapping up %s."), head_file); + die(_("Error wrapping up %s."), git_path_head_file()); } static int reset_for_rollback(const unsigned char *sha1) @@ -877,8 +873,8 @@ static int rollback_single_pick(void) { unsigned char head_sha1[20]; - if (!file_exists(git_path("CHERRY_PICK_HEAD")) && - !file_exists(git_path("REVERT_HEAD"))) + if (!file_exists(git_path_cherry_pick_head()) && + !file_exists(git_path_revert_head())) return error(_("no cherry-pick or revert in progress")); if (read_ref_full("HEAD", 0, head_sha1, NULL)) return error(_("cannot resolve HEAD")); @@ -889,13 +885,11 @@ static int rollback_single_pick(void) static int sequencer_rollback(struct replay_opts *opts) { - const char *filename; FILE *f; unsigned char sha1[20]; struct strbuf buf = STRBUF_INIT; - filename = git_path(SEQ_HEAD_FILE); - f = fopen(filename, "r"); + f = fopen(git_path_head_file(), "r"); if (!f && errno == ENOENT) { /* * There is no multiple-cherry-pick in progress. @@ -905,18 +899,18 @@ static int sequencer_rollback(struct replay_opts *opts) return rollback_single_pick(); } if (!f) - return error(_("cannot open %s: %s"), filename, + return error(_("cannot open %s: %s"), git_path_head_file(), strerror(errno)); if (strbuf_getline(&buf, f, '\n')) { - error(_("cannot read %s: %s"), filename, ferror(f) ? - strerror(errno) : _("unexpected end of file")); + error(_("cannot read %s: %s"), git_path_head_file(), + ferror(f) ? strerror(errno) : _("unexpected end of file")); fclose(f); goto fail; } fclose(f); if (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\0') { error(_("stored pre-cherry-pick HEAD file '%s' is corrupt"), - filename); + git_path_head_file()); goto fail; } if (reset_for_rollback(sha1)) @@ -931,28 +925,27 @@ fail: static void save_todo(struct commit_list *todo_list, struct replay_opts *opts) { - const char *todo_file = git_path(SEQ_TODO_FILE); static struct lock_file todo_lock; struct strbuf buf = STRBUF_INIT; int fd; - fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR); + fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR); if (format_todo(&buf, todo_list, opts) < 0) - die(_("Could not format %s."), todo_file); + die(_("Could not format %s."), git_path_todo_file()); if (write_in_full(fd, buf.buf, buf.len) < 0) { strbuf_release(&buf); - die_errno(_("Could not write to %s"), todo_file); + die_errno(_("Could not write to %s"), git_path_todo_file()); } if (commit_lock_file(&todo_lock) < 0) { strbuf_release(&buf); - die(_("Error wrapping up %s."), todo_file); + die(_("Error wrapping up %s."), git_path_todo_file()); } strbuf_release(&buf); } static void save_opts(struct replay_opts *opts) { - const char *opts_file = git_path(SEQ_OPTS_FILE); + const char *opts_file = git_path_opts_file(); if (opts->no_commit) git_config_set_in_file(opts_file, "options.no-commit", "true"); @@ -1013,8 +1006,8 @@ static int continue_single_pick(void) { const char *argv[] = { "commit", NULL }; - if (!file_exists(git_path("CHERRY_PICK_HEAD")) && - !file_exists(git_path("REVERT_HEAD"))) + if (!file_exists(git_path_cherry_pick_head()) && + !file_exists(git_path_revert_head())) return error(_("no cherry-pick or revert in progress")); return run_command_v_opt(argv, RUN_GIT_CMD); } @@ -1023,14 +1016,14 @@ static int sequencer_continue(struct replay_opts *opts) { struct commit_list *todo_list = NULL; - if (!file_exists(git_path(SEQ_TODO_FILE))) + if (!file_exists(git_path_todo_file())) return continue_single_pick(); read_populate_opts(&opts); read_populate_todo(&todo_list, opts); /* Verify that the conflict has been resolved */ - if (file_exists(git_path("CHERRY_PICK_HEAD")) || - file_exists(git_path("REVERT_HEAD"))) { + if (file_exists(git_path_cherry_pick_head()) || + file_exists(git_path_revert_head())) { int ret = continue_single_pick(); if (ret) return ret; diff --git a/shallow.c b/shallow.c index 257d8115c7..4ded4a8cde 100644 --- a/shallow.c +++ b/shallow.c @@ -48,7 +48,7 @@ int is_repository_shallow(void) return is_shallow; if (!path) - path = git_path("shallow"); + path = git_path_shallow(); /* * fetch-pack sets '--shallow-file ""' as an indicator that no * shallow file should be used. We could just open it and it @@ -142,7 +142,7 @@ static void check_shallow_file_for_update(void) if (is_shallow == -1) die("BUG: shallow must be initialized by now"); - if (!stat_validity_check(&shallow_stat, git_path("shallow"))) + if (!stat_validity_check(&shallow_stat, git_path_shallow())) die("shallow file has changed since we read it"); } @@ -261,7 +261,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock, struct strbuf sb = STRBUF_INIT; int fd; - fd = hold_lock_file_for_update(shallow_lock, git_path("shallow"), + fd = hold_lock_file_for_update(shallow_lock, git_path_shallow(), LOCK_DIE_ON_ERROR); check_shallow_file_for_update(); if (write_shallow_commits(&sb, 0, extra)) { @@ -308,7 +308,7 @@ void prune_shallow(int show_only) strbuf_release(&sb); return; } - fd = hold_lock_file_for_update(&shallow_lock, git_path("shallow"), + fd = hold_lock_file_for_update(&shallow_lock, git_path_shallow(), LOCK_DIE_ON_ERROR); check_shallow_file_for_update(); if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) { @@ -317,7 +317,7 @@ void prune_shallow(int show_only) shallow_lock.filename.buf); commit_lock_file(&shallow_lock); } else { - unlink(git_path("shallow")); + unlink(git_path_shallow()); rollback_lock_file(&shallow_lock); } strbuf_release(&sb); diff --git a/wt-status.c b/wt-status.c index 04e01e4238..717fd48d13 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1171,7 +1171,7 @@ static void show_rebase_in_progress(struct wt_status *s, status_printf_ln(s, color, _(" (use \"git rebase --abort\" to check out the original branch)")); } - } else if (state->rebase_in_progress || !stat(git_path("MERGE_MSG"), &st)) { + } else if (state->rebase_in_progress || !stat(git_path_merge_msg(), &st)) { print_rebase_state(s, state, color); if (s->hints) status_printf_ln(s, color, @@ -1368,7 +1368,7 @@ void wt_status_get_state(struct wt_status_state *state, struct stat st; unsigned char sha1[20]; - if (!stat(git_path("MERGE_HEAD"), &st)) { + if (!stat(git_path_merge_head(), &st)) { state->merge_in_progress = 1; } else if (!stat(git_path("rebase-apply"), &st)) { if (!stat(git_path("rebase-apply/applying"), &st)) { @@ -1387,7 +1387,7 @@ void wt_status_get_state(struct wt_status_state *state, state->rebase_in_progress = 1; state->branch = read_and_strip_branch("rebase-merge/head-name"); state->onto = read_and_strip_branch("rebase-merge/onto"); - } else if (!stat(git_path("CHERRY_PICK_HEAD"), &st) && + } else if (!stat(git_path_cherry_pick_head(), &st) && !get_sha1("CHERRY_PICK_HEAD", sha1)) { state->cherry_pick_in_progress = 1; hashcpy(state->cherry_pick_head_sha1, sha1); @@ -1396,7 +1396,7 @@ void wt_status_get_state(struct wt_status_state *state, state->bisect_in_progress = 1; state->branch = read_and_strip_branch("BISECT_START"); } - if (!stat(git_path("REVERT_HEAD"), &st) && + if (!stat(git_path_revert_head(), &st) && !get_sha1("REVERT_HEAD", sha1)) { state->revert_in_progress = 1; hashcpy(state->revert_head_sha1, sha1);