RE: git blame breaking on repository with CRLF files
Hello Torsten, Torsten Bögershausen, Sonntag, 9. August 2015 22:20: > On 2015-08-08 07.58, Torsten Bögershausen wrote: >> On 2015-08-07 18.32, Benkstein, Frank wrote: >>> I am working working on Linux and am examining code in a git repository I do >>> not know much about. I am only looking at files, not changing anything. On >>> some files in the repository I get " (Not Committed Yet" for all >>> lines >>> when running "git blame". I checked with "git status", "git reset", "git >>> clean" that the files are indeed in the repository and unmodified. I >>> noticed >>> that this only happens with git v2.5.0. With git v2.4.0 it looks correct, >>> i.e. >>> the output has proper commit ids, Author names and dates.. With "git >>> bisect" I >>> tracked this down to the following commit: >>> >>> commit 4bf256d67a85bed1e175ecc2706322eafe4489ca (HEAD, refs/bisect/bad) >>> Author: Torsten Bögershausen >>> Date: Sun May 3 18:38:01 2015 +0200 >>> >>> blame: CRLF in the working tree and LF in the repo >>> >>> Digging further, it seems that most files in the repository are checked in >>> with >>> CRLF line endings. In my working tree these are checked out as LF >> Do I understand it right that you have files in the repo with CRLF ? >> And these files are checked out with LF in the working tree ? >> Are the files marked with .gitattributes ? >> Or does the file have mixed line endings ? >> >> (Unless I missed something: Git never strips CRLF into LF at checkout, >> so I wonder how you ended up in this situation) You were right. They are CRLF in my working tree. My editor tricked me. >> Is there a way to reproduce it? >> > Actually I could reproduce the following: > CRLF in repo, CRLF in working tree, core.autocrlf= true. > > This is an old limitation (or call it bug), which has been there for a long > time, (I tested with Git v1.7.0 from 2010). > > Thanks for the report, we will see if anybody is able to fix it. > I can probably contribute some test cases. You are correct that it is also wrong in git v1.7.0. However, it is correct in v2.4.0. Another bisect gave me this commit which was included in v2.0.1: commit 4d4813a52f3722854a54bab046f4abfec13ef6ae Author: brian m. carlson Date: Sat Apr 26 23:10:40 2014 + blame: correctly handle files regardless of autocrlf So this still looks like a regression v2.5.0 to me. Regards, Frank.
[PATCH 0/17] removing questionable uses of git_path
Recently Michael and I were working on a patch series (not yet published), which did something like: const char *path = git_path("foo"); ... do stuff with path ... for_each_ref(some_callback, NULL); ... do some other stuff ... unlink(path); Clever readers may have spotted the bug immediately, but we did not, until we found that random loose refs were being deleted from the repository. The problem is that git_path uses a static buffer that gets overwritten by subsequent calls. The ref code uses it to iterate over all of the loose refs in a directory, so our original path is trashed before for_each_ref returns. Except to make it even more exciting, git_path actually has a ring of _four_ buffers, so any trivial test you write will probably work just fine; it's only when you use a real repository that it causes problems (and then, only if the code path is such that the loose refs were not previously accessed and cached!). Michael likened git_path to "a hand-grenade with the pin pulled out", and I tend to agree. On the other hand, it's pretty darn useful to be able to get a quick path without having to deal with memory allocation and ownership. This patch series tries to document the danger, and remove some of the more questionable uses. I don't know whether this is fixing any actual latent bugs; I traced a number of the code paths manually, but never found a bug. There were some near misses, though, which make me believe that seemingly-unrelated refactoring could introduce a bug. I stopped short of trying to eradicate git_path entirely, and settled for: git grep -E '[^_](git_|mk)path\(' producing a fairly tame-looking set of function calls. It's OK to pass the result of git_path() to a system call, or something that is a thin wrapper around one (e.g., strbuf_read_file). I think this takes us most of the way there. I left out a few cases where introducing allocations would have been awkward, and I verified that there were no bugs (e.g., rerere_path). And I left out a few spots that conflict with topics in "next" (and luckily, in all cases what is in next makes the problem go away, so we do not have to follow-up for those sites). Along the way, there are a few cleanups (e.g., I polished off the recent hold_lock_file_for_append topic which was on the list, as it had some problematic calls). [01/17]: cache.h: clarify documentation for git_path, et al [02/17]: cache.h: complete set of git_path_submodule helpers [03/17]: t5700: modernize style [04/17]: add_to_alternates_file: don't add duplicate entries [05/17]: remove hold_lock_file_for_append [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases [07/17]: prefer mkpathdup to mkpath in assignments [08/17]: remote.c: drop extraneous local variable from migrate_file [09/17]: refs.c: remove extra git_path calls from read_loose refs [10/17]: path.c: drop git_path_submodule [11/17]: refs.c: simplify strbufs in reflog setup and writing [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic [14/17]: refs.c: remove_empty_directories can take a strbuf [15/17]: find_hook: keep our own static buffer [16/17]: get_repo_path: refactor path-allocation [17/17]: memoize common git-path "constant" files -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/17] 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 --- cache.h | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/cache.h b/cache.h index 6bb7119..8db884e 100644 --- a/cache.h +++ b/cache.h @@ -708,6 +708,18 @@ extern int check_repository_format(void); #define DATA_CHANGED0x0020 #define TYPE_CHANGED0x0040 +/* + * 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); /* -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/17] 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 --- cache.h | 5 + path.c | 35 ++- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/cache.h b/cache.h index 8db884e..6f74f33 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 10f4cbf..9aad9a1 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; -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/17] 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 --- 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 3e783fc..51d131a 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
[PATCH 04/17] 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 --- This is a polishing of the thread at: http://thread.gmane.org/gmane.comp.version-control.git/270341 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 1cee438..3400b8b 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 51d131a..ef1779f 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 -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/17] 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 --- 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 af7340c..70b9b1e 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 93b250e..e70941a 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(co
[PATCH 06/17] 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 --- 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 f4b87e9..0794703 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 2ad4fee..ad8848b 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 b977c00..bac40ef 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 0b2b82c..b3d1dab 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 2db2975..93b250e 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_packe
[PATCH 05/17] remove hold_lock_file_for_append
From: Jim Hill No users of hold_lock_file_for_append remain, so remove it. hold_lock_file_for_append copies its target file internally. This makes it too heavyweight for true append-only logging and too limited for anything else (which probably wants to process the contents). It shouldn't be used. [jk: tweaked commit message, and dropped declaration and documentation, too] Signed-off-by: Jim Hill Signed-off-by: Jeff King --- And this is the obvious continuation of the last patch. Documentation/technical/api-lockfile.txt | 26 -- lockfile.c | 38 lockfile.h | 7 ++ 3 files changed, 11 insertions(+), 60 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index 93b5f23..0f5c481 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -40,7 +40,7 @@ The caller: * Attempts to create a lockfile by passing that variable and the path of the final destination (e.g. `$GIT_DIR/index`) to - `hold_lock_file_for_update` or `hold_lock_file_for_append`. + `hold_lock_file_for_update`. * Writes new content for the destination file by either: @@ -64,8 +64,7 @@ When finished writing, the caller can: Even after the lockfile is committed or rolled back, the `lock_file` object must not be freed or altered by the caller. However, it may be -reused; just pass it to another call of `hold_lock_file_for_update` or -`hold_lock_file_for_append`. +reused; just pass it to another call of `hold_lock_file_for_update`. If the program exits before you have called one of `commit_lock_file`, `commit_lock_file_to`, `rollback_lock_file`, or `close_lock_file`, an @@ -111,8 +110,7 @@ appropriately, do their best to roll back the lockfile, and return -1. Flags - -The following flags can be passed to `hold_lock_file_for_update` or -`hold_lock_file_for_append`: +The following flags can be passed to `hold_lock_file_for_update`: LOCK_NO_DEREF:: @@ -141,12 +139,6 @@ hold_lock_file_for_update:: above). Attempt to create a lockfile for the destination and return the file descriptor for writing to the file. -hold_lock_file_for_append:: - - Like `hold_lock_file_for_update`, but before returning copy - the existing contents of the file (if any) to the lockfile and - position its write pointer at the end of the file. - fdopen_lock_file:: Associate a stdio stream with the lockfile. Return NULL @@ -162,8 +154,8 @@ get_locked_file_path:: commit_lock_file:: Take a pointer to the `struct lock_file` initialized with an - earlier call to `hold_lock_file_for_update` or - `hold_lock_file_for_append`, close the file descriptor, and + earlier call to `hold_lock_file_for_update`, + close the file descriptor, and rename the lockfile to its final destination. Return 0 upon success. On failure, roll back the lock file and return -1, with `errno` set to the value from the failing call to @@ -180,8 +172,8 @@ commit_lock_file_to:: rollback_lock_file:: Take a pointer to the `struct lock_file` initialized with an - earlier call to `hold_lock_file_for_update` or - `hold_lock_file_for_append`, close the file descriptor and + earlier call to `hold_lock_file_for_update`, + close the file descriptor and remove the lockfile. It is a NOOP to call `rollback_lock_file()` for a `lock_file` object that has already been committed or rolled back. @@ -189,8 +181,8 @@ rollback_lock_file:: close_lock_file:: Take a pointer to the `struct lock_file` initialized with an - earlier call to `hold_lock_file_for_update` or - `hold_lock_file_for_append`. Close the file descriptor (and + earlier call to `hold_lock_file_for_update`. + Close the file descriptor (and the file pointer if it has been opened using `fdopen_lock_file`). Return 0 upon success. On failure to `close(2)`, return a negative value and roll back the lock diff --git a/lockfile.c b/lockfile.c index 993bb82..b1ceec6 100644 --- a/lockfile.c +++ b/lockfile.c @@ -249,44 +249,6 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path, return fd; } -int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) -{ - int fd, orig_fd; - - fd = lock_file(lk, path, flags); - if (fd < 0) { - if (flags & LOCK_DIE_ON_ERROR) - unable_to_lock_die(path, errno); - return fd; - } - - orig_fd = open(path, O_RDONLY); - if (orig_fd < 0) { - if (errno != ENOENT) { - int save_errno = errno; - - if (flags & LOCK_DIE_ON_ERROR) - die("cannot open '%s'
[PATCH 08/17] 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 --- builtin/remote.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index cc3c741..181668d 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; } -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/17] 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 --- refs.c | 23 +-- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index e70941a..06f95c4 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); } -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/17] 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 --- cache.h | 5 ++--- path.c | 10 -- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/cache.h b/cache.h index 6f74f33..7a4fa90 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 9aad9a1..94d7ec2 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; -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/17] 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 --- refs.c | 38 +++--- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/refs.c b/refs.c index 06f95c4..3666132 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 *log_file, 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, log_file, 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(log_file->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", log_file->buf, strerror(errno)); close(logfd); return -1; } if (close(logfd)) { - strbuf_
[PATCH 14/17] 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 --- refs.c | 34 +++--- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/refs.c b/refs.c index 8566677..ec1d06c 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; -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/17] 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 --- refs.c | 32 +++- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/refs.c b/refs.c index 61a318f..8566677 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; } /* -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/17] 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 --- refs.c | 21 ++--- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/refs.c b/refs.c index 3666132..61a318f 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) -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/17] 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 --- 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 4d73e90..28e1d55 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 1103805..5b4425a 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, ...); -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/17] 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 --- builtin/clone.c | 43 +-- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 303a3a7..5864ad1 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; -- 2.5.0.414.g670f2a4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/17] 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 --- This one is probably the most contentious, as it has very far-reaching effects. But it really reduces the number of sites that need audited by quite a lot. 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 8f2ac6c..086c08d 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 03d5cd9..7a6498c 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 b002435..e283683 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")); - un
[PATCHv3 1/2] config: add '--names-only' option to list only variable names
'git config' can only show values or name-value pairs, so if a shell script needs the names of set config variables it has to run 'git config --list' or '--get-regexp' and parse the output to separate config variable names from their values. However, such a parsing can't cope with multi-line values. Though 'git config' can produce null-terminated output for newline-safe parsing, that's of no use in such a case, becase shells can't cope with null characters. Even our own bash completion script suffers from these issues. Help the completion script, and shell scripts in general, by introducing the '--names-only' option to modify the output of '--list' and '--get-regexp' to list only the names of config variables, so they don't have to perform error-prone post processing to separate variable names from their values anymore. Signed-off-by: SZEDER Gábor --- Documentation/git-config.txt | 10 +++--- builtin/config.c | 14 -- contrib/completion/git-completion.bash | 1 + t/t1300-repo-config.sh | 22 ++ 4 files changed, 42 insertions(+), 5 deletions(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 02ec096faa..ba76097c06 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -14,13 +14,13 @@ SYNOPSIS 'git config' [] [type] --replace-all name value [value_regex] 'git config' [] [type] [-z|--null] --get name [value_regex] 'git config' [] [type] [-z|--null] --get-all name [value_regex] -'git config' [] [type] [-z|--null] --get-regexp name_regex [value_regex] +'git config' [] [type] [-z|--null] [--names-only] --get-regexp name_regex [value_regex] 'git config' [] [type] [-z|--null] --get-urlmatch name URL 'git config' [] --unset name [value_regex] 'git config' [] --unset-all name [value_regex] 'git config' [] --rename-section old_name new_name 'git config' [] --remove-section name -'git config' [] [-z|--null] -l | --list +'git config' [] [-z|--null] [--names-only] -l | --list 'git config' [] --get-color name [default] 'git config' [] --get-colorbool name [stdout-is-tty] 'git config' [] -e | --edit @@ -159,7 +159,7 @@ See also <>. -l:: --list:: - List all variables set in config file. + List all variables set in config file, along with their values. --bool:: 'git config' will ensure that the output is "true" or "false" @@ -190,6 +190,10 @@ See also <>. output without getting confused e.g. by values that contain line breaks. +--names-only:: + Output only the names of config variables for `--list` or + `--get-regexp`. + --get-colorbool name [stdout-is-tty]:: Find the color setting for `name` (e.g. `color.diff`) and output diff --git a/builtin/config.c b/builtin/config.c index 7188405f7e..307980ab50 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -13,6 +13,7 @@ static char *key; static regex_t *key_regexp; static regex_t *regexp; static int show_keys; +static int omit_values; static int use_key_regexp; static int do_all; static int do_not_match; @@ -78,6 +79,7 @@ static struct option builtin_config_options[] = { OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH), OPT_GROUP(N_("Other")), OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")), + OPT_BOOL(0, "names-only", &omit_values, N_("names only")), OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")), OPT_END(), }; @@ -91,7 +93,7 @@ static void check_argc(int argc, int min, int max) { static int show_all_config(const char *key_, const char *value_, void *cb) { - if (value_) + if (!omit_values && value_) printf("%s%c%s%c", key_, delim, value_, term); else printf("%s%c", key_, term); @@ -117,6 +119,10 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value strbuf_addstr(buf, key_); must_print_delim = 1; } + if (omit_values) { + strbuf_addch(buf, term); + return 0; + } if (types == TYPE_INT) sprintf(value, "%"PRId64, git_config_int64(key_, value_ ? value_ : "")); @@ -549,7 +555,11 @@ int cmd_config(int argc, const char **argv, const char *prefix) default: usage_with_options(builtin_config_usage, builtin_config_options); } - + if (omit_values && + !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) { + error("--names-only is only applicable to --list or --get-regexp"); + usage_with_options(builtin_config_usage, builtin_config_options); + } if (actions == ACTION_LIST) { check_argc(argc, 0, 0); if (git_config_with_options(show_all_config, NUL
[PATCHv3 0/2] 'git config --names-only' to help the completion script
This is a reroll of 'sg/config-name-only'. * Instead of the two new listing options of the previous round add one new option '--names-only' to modify the output of '--list' and '--get-regexp' options, as suggested in previous discussions. * Reorganized the commit messages: don't go into details of the completion bug in the first patch modifying builtin/config.c, talk about that in the second patch. SZEDER Gábor (2): config: add '--names-only' option to list only variable names completion: list variable names reliably with 'git config --names-only' Documentation/git-config.txt | 10 +++--- builtin/config.c | 14 -- contrib/completion/git-completion.bash | 16 t/t1300-repo-config.sh | 22 ++ 4 files changed, 45 insertions(+), 17 deletions(-) -- 2.5.0.245.gff6622b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only'
Recenty I created a multi-line branch description with '.' and '=' characters on one of the lines, and noticed that fragments of that line show up when completing set variable names for 'git config', e.g.: $ git config --get branch.b.description Branch description to fool the completion script with a second line containing dot . and equals = characters. $ git config --unset ... second line containing dot . and equals ... The completion script runs 'git config --list' and processes its output to strip the values and keep only the variable names. It does so by looking for lines containing '.' and '=' and outputting everything before the '=', which was fooled by my multi-line branch description. A similar issue exists with aliases and pretty format aliases with multi-line values, but in that case 'git config --get-regexp' is run and lines in its output are simply stripped after the first space, so subsequent lines don't even have to contain '.' and '=' to fool the completion script. Use the new '--names-only' option added in the previous commit to list config variable names reliably in both cases, without error-prone post processing. Signed-off-by: SZEDER Gábor --- contrib/completion/git-completion.bash | 15 +++ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 6eaab141e2..7200828fc4 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -744,9 +744,8 @@ __git_compute_porcelain_commands () __git_get_config_variables () { local section="$1" i IFS=$'\n' - for i in $(git --git-dir="$(__gitdir)" config --get-regexp "^$section\..*" 2>/dev/null); do - i="${i#$section.}" - echo "${i/ */}" + for i in $(git --git-dir="$(__gitdir)" config --names-only --get-regexp "^$section\..*" 2>/dev/null); do + echo "${i#$section.}" done } @@ -1774,15 +1773,7 @@ __git_config_get_set_variables () c=$((--c)) done - git --git-dir="$(__gitdir)" config $config_file --list 2>/dev/null | - while read -r line - do - case "$line" in - *.*=*) - echo "${line/=*/}" - ;; - esac - done + git --git-dir="$(__gitdir)" config $config_file --names-only --list 2>/dev/null } _git_config () -- 2.5.0.245.gff6622b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 00/16] Introduce a tempfile module
This is a re-roll of the tempfile patch series [1]. I'm sorry for the long delay getting v2 out. Thanks to Junio and Johannes Sixt for their feedback about v1. I think I have addressed all of their points. This version is very similar to v1 in spirit, though quite a few details have changed. The main difference is that I add some more wrapper functions for both lockfile and tempfile (a) to add some abstraction and (b) so that users of the former don't need to know that it is based on the latter: * Add new lockfile wrappers around the corresponding tempfile functions: * lockfile: * fdopen_lock_file() * close_lock_file() * reopen_lock_file() * Add accessors: * lockfile: * get_lock_file_path() * get_lock_file_fd() * get_lock_file_fp() * tempfile: * is_tempfile_active() * get_tempfile_path() * get_tempfile_fd() * get_tempfile_fp() Other changes in this version: * Make some trivial wrapper functions inline. * Change create_bundle() to dup() the file descriptor that it passes to write_pack_data() so that it doesn't have to tinker with lock->tempfile.fd to prevent the file from being closed twice. * Move some docs about the implementation from tempfile.h to tempfile.c. * Rename register_tempfile_object() to prepare_tempfile_object() to reduce confusion with register_tempfile(). Remove its "path" parameter and add a docstring. * Simplify some `die("BUG:...")` error messages. This series applies to the same commit as v1, namely v2.4.3-368-g7974889. There is one small conflict when merging to master or next or (pu minus gitster/mh/tempfile). This patch series is also available from my GitHub fork [2] as branch "tempfile". [1] http://thread.gmane.org/gmane.comp.version-control.git/270998 [2] https://github.com/mhagger/git Michael Haggerty (16): Move lockfile documentation to lockfile.h and lockfile.c create_bundle(): duplicate file descriptor to avoid closing it twice lockfile: add accessors get_lock_file_fd() and get_lock_file_fp() lockfile: add accessor get_lock_file_path() commit_lock_file(): use get_locked_file_path() tempfile: a new module for handling temporary files prepare_tempfile_object(): new function, extracted from create_tempfile() tempfile: add several functions for creating temporary files register_tempfile(): new function to handle an existing temporary file write_shared_index(): use tempfile module setup_temporary_shallow(): use tempfile module diff: use tempfile module lock_repo_for_gc(): compute the path to "gc.pid" only once gc: use tempfile module to handle gc.pid file credential-cache--daemon: delete socket from main() credential-cache--daemon: use tempfile module Documentation/technical/api-lockfile.txt | 220 Makefile | 1 + builtin/commit.c | 15 +- builtin/gc.c | 32 +-- bundle.c | 26 ++- config.c | 14 +- credential-cache--daemon.c | 25 +-- credential-store.c | 2 +- diff.c | 29 +-- lockfile.c | 205 +++ lockfile.h | 336 +-- read-cache.c | 40 +--- refs.c | 18 +- shallow.c| 41 +--- tempfile.c | 305 tempfile.h | 271 + 16 files changed, 951 insertions(+), 629 deletions(-) delete mode 100644 Documentation/technical/api-lockfile.txt create mode 100644 tempfile.c create mode 100644 tempfile.h -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 02/16] create_bundle(): duplicate file descriptor to avoid closing it twice
write_pack_data() passes bundle_fd to start_command() to be used as the stdout of pack-objects. But start_command() closes its stdout if it is > 1. This is a problem if bundle_fd is the fd of a lock_file, because commit_lock_file() will also try to close the fd. So the old code suppressed commit_lock_file()'s usual behavior of closing the file descriptor by setting the lock_file object's fd field to -1. But this is not really kosher. Code here shouldn't be mutating fields within the lock_file object. Instead, duplicate the file descriptor before passing it to write_pack_data(). Then that function can close its copy without closing the copy held in the lock_file object. Signed-off-by: Michael Haggerty --- This is new since v1. I like that it is better decoupled than the old code, but let me know if you think otherwise. Actually, it seems to me that start_command()'s special case of not closing fd==0 is weird. I suppose that is because fd==0 is used to mean "no redirections" whereas 0 also happens to be the fd for stdin. But I don't want to dig into that now. bundle.c | 26 -- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/bundle.c b/bundle.c index f732c92..b9dacc0 100644 --- a/bundle.c +++ b/bundle.c @@ -235,7 +235,9 @@ out: return result; } -static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs) + +/* Write the pack data to bundle_fd, then close it if it is > 1. */ +static int write_pack_data(int bundle_fd, struct rev_info *revs) { struct child_process pack_objects = CHILD_PROCESS_INIT; int i; @@ -250,13 +252,6 @@ static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_inf if (start_command(&pack_objects)) return error(_("Could not spawn pack-objects")); - /* -* start_command closed bundle_fd if it was > 1 -* so set the lock fd to -1 so commit_lock_file() -* won't fail trying to close it. -*/ - lock->fd = -1; - for (i = 0; i < revs->pending.nr; i++) { struct object *object = revs->pending.objects[i].item; if (object->flags & UNINTERESTING) @@ -416,10 +411,21 @@ int create_bundle(struct bundle_header *header, const char *path, bundle_to_stdout = !strcmp(path, "-"); if (bundle_to_stdout) bundle_fd = 1; - else + else { bundle_fd = hold_lock_file_for_update(&lock, path, LOCK_DIE_ON_ERROR); + /* +* write_pack_data() will close the fd passed to it, +* but commit_lock_file() will also try to close the +* lockfile's fd. So make a copy of the file +* descriptor to avoid trying to close it twice. +*/ + bundle_fd = dup(bundle_fd); + if (bundle_fd < 0) + die_errno("unable to dup file descriptor"); + } + /* write signature */ write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); @@ -445,7 +451,7 @@ int create_bundle(struct bundle_header *header, const char *path, return -1; /* write pack */ - if (write_pack_data(bundle_fd, &lock, &revs)) + if (write_pack_data(bundle_fd, &revs)) return -1; if (!bundle_to_stdout) { -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 15/16] credential-cache--daemon: delete socket from main()
main() is responsible for cleaning up the socket in the case of errors, so it is reasonable to also make it responsible for cleaning it up when there are no errors. This change also makes the next step easier. Signed-off-by: Michael Haggerty --- credential-cache--daemon.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index c2f0049..a671b2b 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -221,7 +221,6 @@ static void serve_cache(const char *socket_path, int debug) ; /* nothing */ close(fd); - unlink(socket_path); } static const char permissions_advice[] = @@ -280,5 +279,7 @@ int main(int argc, const char **argv) serve_cache(socket_path, debug); + unlink(socket_path); + return 0; } -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 06/16] tempfile: a new module for handling temporary files
A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty --- Makefile | 1 + lockfile.c | 261 - lockfile.h | 73 +++-- tempfile.c | 238 +++ tempfile.h | 167 +++ 5 files changed, 470 insertions(+), 270 deletions(-) create mode 100644 tempfile.c create mode 100644 tempfile.h diff --git a/Makefile b/Makefile index 54ec511..2573f89 100644 --- a/Makefile +++ b/Makefile @@ -786,6 +786,7 @@ LIB_OBJS += string-list.o LIB_OBJS += submodule.o LIB_OBJS += symlinks.o LIB_OBJS += tag.o +LIB_OBJS += tempfile.o LIB_OBJS += trace.o LIB_OBJS += trailer.o LIB_OBJS += transport.o diff --git a/lockfile.c b/lockfile.c index 3904803..e1d68f7 100644 --- a/lockfile.c +++ b/lockfile.c @@ -2,90 +2,8 @@ * Copyright (c) 2005, Junio C Hamano */ -/* - * State diagram and cleanup - * - - * - * This module keeps track of all locked files in `lock_file_list` for - * use at cleanup. This list and the `lock_file` objects that comprise - * it must be kept in self-consistent states at all time, because the - * program can be interrupted any time by a signal, in which case the - * signal handler will walk through the list attempting to clean up - * any open lock files. - * - * The possible states of a `lock_file` object are as follows: - * - * - Uninitialized. In this state the object's `on_list` field must be - * zero but the rest of its contents need not be initialized. As - * soon as the object is used in any way, it is irrevocably - * registered in `lock_file_list`, and `on_list` is set. - * - * - Locked, lockfile open (after `hold_lock_file_for_update()`, - * `hold_lock_file_for_append()`, or `reopen_lock_file()`). In this - * state: - * - * - the lockfile exists - * - `active` is set - * - `filename` holds the filename of the lockfile - * - `fd` holds a file descriptor open for writing to the lockfile - * - `fp` holds a pointer to an open `FILE` object if and only if - * `fdopen_lock_file()` has been called on the object - * - `owner` holds the PID of the process that locked the file - * - * - Locked, lockfile closed (after successful `close_lock_file()`). - * Same as the previous state, except that the lockfile is closed - * and `fd` is -1. - * - * - Unlocked (after `commit_lock_file()`, `commit_lock_file_to()`, - * `rollback_lock_file()`, a failed attempt to lock, or a failed - * `close_lock_file()`). In this state: - * - * - `active` is unset - * - `filename` is empty (usually, though there are transitory - * states in which this condition doesn't hold). Client code should - * *not* rely on the filename being empty in this state. - * - `fd` is -1 - * - the object is left registered in the `lock_file_list`, and - * `on_list` is set. - * - * A lockfile is owned by the process that created it. The `lock_file` - * has an `owner` field that records the owner's PID. This field is - * used to prevent a forked process from closing a lockfile created by - * its parent. - */ - #include "cache.h" #include "lockfile.h" -#include "sigchain.h" - -static struct lock_file *volatile lock_file_list; - -static void remove_lock_files(int skip_fclose) -{ - pid_t me = getpid(); - - while (lock_file_list) { - if (lock_file_list->owner == me) { - /* fclose() is not safe to call in a signal handler */ - if (skip_fclose) - lock_file_list->fp = NULL; - rollback_lock_file(lock_file_list); - } - lock_file_list = lock_file_list->next; - } -} - -static void remove_lock_files_on_exit(void) -{ - remove_lock_files(0); -} - -static void remove_lock_files_on_signal(int signo) -{ - remove_lock_files(1); - sigchain_pop(signo); - raise(signo); -} /* * path = absolute or relative path name @@ -154,60 +72,17 @@ static void resolve_symlink(struct strbuf *path) /* Make sure errno contains a meaningful value on error */ static int lock_file(struct lock_file *lk, const char *path, int flags) { - size_t pathlen = strlen(path); - - if (!lock_file_list) { - /* One-time initialization */ - sigchain_push_common(remove_lock_files_on_signal); - atexit(remove_lock_files_on_exit); - } + int fd; + struct strbuf filename = STRBUF_INIT; - if (lk->active) - die("BUG: cannot lock_file(\"%s\") using active struct
[PATCH v2 12/16] diff: use tempfile module
Signed-off-by: Michael Haggerty --- diff.c | 29 +++-- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/diff.c b/diff.c index 7500c55..dc95247 100644 --- a/diff.c +++ b/diff.c @@ -2,6 +2,7 @@ * Copyright (C) 2005 Junio C Hamano */ #include "cache.h" +#include "tempfile.h" #include "quote.h" #include "diff.h" #include "diffcore.h" @@ -312,7 +313,7 @@ static struct diff_tempfile { const char *name; /* filename external diff should read from */ char hex[41]; char mode[10]; - char tmp_path[PATH_MAX]; + struct tempfile tempfile; } diff_temp[2]; typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len); @@ -564,25 +565,16 @@ static struct diff_tempfile *claim_diff_tempfile(void) { die("BUG: diff is failing to clean up its tempfiles"); } -static int remove_tempfile_installed; - static void remove_tempfile(void) { int i; for (i = 0; i < ARRAY_SIZE(diff_temp); i++) { - if (diff_temp[i].name == diff_temp[i].tmp_path) - unlink_or_warn(diff_temp[i].name); + if (is_tempfile_active(&diff_temp[i].tempfile)) + delete_tempfile(&diff_temp[i].tempfile); diff_temp[i].name = NULL; } } -static void remove_tempfile_on_signal(int signo) -{ - remove_tempfile(); - sigchain_pop(signo); - raise(signo); -} - static void print_line_count(FILE *file, int count) { switch (count) { @@ -2817,8 +2809,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, strbuf_addstr(&template, "XX_"); strbuf_addstr(&template, base); - fd = git_mkstemps(temp->tmp_path, PATH_MAX, template.buf, - strlen(base) + 1); + fd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1); if (fd < 0) die_errno("unable to create temp-file"); if (convert_to_working_tree(path, @@ -2828,8 +2819,8 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp, } if (write_in_full(fd, blob, size) != size) die_errno("unable to write temp-file"); - close(fd); - temp->name = temp->tmp_path; + close_tempfile(&temp->tempfile); + temp->name = get_tempfile_path(&temp->tempfile); strcpy(temp->hex, sha1_to_hex(sha1)); temp->hex[40] = 0; sprintf(temp->mode, "%06o", mode); @@ -2854,12 +2845,6 @@ static struct diff_tempfile *prepare_temp_file(const char *name, return temp; } - if (!remove_tempfile_installed) { - atexit(remove_tempfile); - sigchain_push_common(remove_tempfile_on_signal); - remove_tempfile_installed = 1; - } - if (!S_ISGITLINK(one->mode) && (!one->sha1_valid || reuse_worktree_file(name, one->sha1, 1))) { -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 16/16] credential-cache--daemon: use tempfile module
Use the tempfile module to ensure that the socket file gets deleted on program exit. Signed-off-by: Michael Haggerty --- credential-cache--daemon.c | 26 ++ 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c index a671b2b..eef6fce 100644 --- a/credential-cache--daemon.c +++ b/credential-cache--daemon.c @@ -1,23 +1,11 @@ #include "cache.h" +#include "tempfile.h" #include "credential.h" #include "unix-socket.h" #include "sigchain.h" #include "parse-options.h" -static const char *socket_path; - -static void cleanup_socket(void) -{ - if (socket_path) - unlink(socket_path); -} - -static void cleanup_socket_on_signal(int sig) -{ - cleanup_socket(); - sigchain_pop(sig); - raise(sig); -} +static struct tempfile socket_file; struct credential_cache_entry { struct credential item; @@ -256,6 +244,7 @@ static void check_socket_directory(const char *path) int main(int argc, const char **argv) { + const char *socket_path; static const char *usage[] = { "git-credential-cache--daemon [opts] ", NULL @@ -272,14 +261,11 @@ int main(int argc, const char **argv) if (!socket_path) usage_with_options(usage, options); - check_socket_directory(socket_path); - - atexit(cleanup_socket); - sigchain_push_common(cleanup_socket_on_signal); + check_socket_directory(socket_path); + register_tempfile(&socket_file, socket_path); serve_cache(socket_path, debug); - - unlink(socket_path); + delete_tempfile(&socket_file); return 0; } -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 10/16] write_shared_index(): use tempfile module
Signed-off-by: Michael Haggerty --- read-cache.c | 38 ++ 1 file changed, 6 insertions(+), 32 deletions(-) diff --git a/read-cache.c b/read-cache.c index 96cb9a3..89be226 100644 --- a/read-cache.c +++ b/read-cache.c @@ -5,6 +5,7 @@ */ #define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" +#include "tempfile.h" #include "lockfile.h" #include "cache-tree.h" #include "refs.h" @@ -2136,54 +2137,27 @@ static int write_split_index(struct index_state *istate, return ret; } -static char *temporary_sharedindex; - -static void remove_temporary_sharedindex(void) -{ - if (temporary_sharedindex) { - unlink_or_warn(temporary_sharedindex); - free(temporary_sharedindex); - temporary_sharedindex = NULL; - } -} - -static void remove_temporary_sharedindex_on_signal(int signo) -{ - remove_temporary_sharedindex(); - sigchain_pop(signo); - raise(signo); -} +static struct tempfile temporary_sharedindex; static int write_shared_index(struct index_state *istate, struct lock_file *lock, unsigned flags) { struct split_index *si = istate->split_index; - static int installed_handler; int fd, ret; - temporary_sharedindex = git_pathdup("sharedindex_XX"); - fd = mkstemp(temporary_sharedindex); + fd = mks_tempfile(&temporary_sharedindex, git_path("sharedindex_XX")); if (fd < 0) { - free(temporary_sharedindex); - temporary_sharedindex = NULL; hashclr(si->base_sha1); return do_write_locked_index(istate, lock, flags); } - if (!installed_handler) { - atexit(remove_temporary_sharedindex); - sigchain_push_common(remove_temporary_sharedindex_on_signal); - } move_cache_to_base_index(istate); ret = do_write_index(si->base, fd, 1); - close(fd); if (ret) { - remove_temporary_sharedindex(); + delete_tempfile(&temporary_sharedindex); return ret; } - ret = rename(temporary_sharedindex, -git_path("sharedindex.%s", sha1_to_hex(si->base->sha1))); - free(temporary_sharedindex); - temporary_sharedindex = NULL; + ret = rename_tempfile(&temporary_sharedindex, + git_path("sharedindex.%s", sha1_to_hex(si->base->sha1))); if (!ret) hashcpy(si->base_sha1, si->base->sha1); return ret; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 09/16] register_tempfile(): new function to handle an existing temporary file
Allow an existing file to be registered with the tempfile-handling infrastructure; in particular, arrange for it to be deleted on program exit. This can be used if the temporary file has to be created in a more complicated way than just open(). For example: * If the file itself needs to be created via the lockfile API * If it is not a regular file (e.g., a socket) Signed-off-by: Michael Haggerty --- tempfile.c | 8 tempfile.h | 8 2 files changed, 16 insertions(+) diff --git a/tempfile.c b/tempfile.c index 0b5d8ce..0af7ebf 100644 --- a/tempfile.c +++ b/tempfile.c @@ -137,6 +137,14 @@ int create_tempfile(struct tempfile *tempfile, const char *path) return tempfile->fd; } +void register_tempfile(struct tempfile *tempfile, const char *path) +{ + prepare_tempfile_object(tempfile); + strbuf_add_absolute_path(&tempfile->filename, path); + tempfile->owner = getpid(); + tempfile->active = 1; +} + int mks_tempfile_sm(struct tempfile *tempfile, const char *template, int suffixlen, int mode) { diff --git a/tempfile.h b/tempfile.h index a30e12c..4219fe4 100644 --- a/tempfile.h +++ b/tempfile.h @@ -92,6 +92,14 @@ struct tempfile { */ extern int create_tempfile(struct tempfile *tempfile, const char *path); +/* + * Register an existing file as a tempfile, meaning that it will be + * deleted when the program exits. The tempfile is considered closed, + * but it can be worked with like any other closed tempfile (for + * example, it can be opened using reopen_tempfile()). + */ +extern void register_tempfile(struct tempfile *tempfile, const char *path); + /* * mks_tempfile functions -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 07/16] prepare_tempfile_object(): new function, extracted from create_tempfile()
This makes the next step easier. The old code used to use "path" to set the initial length of tempfile->filename. This was not helpful because path was usually relative whereas the value stored to filename will be absolute. So just initialize the length to 0. Signed-off-by: Michael Haggerty --- tempfile.c | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/tempfile.c b/tempfile.c index d835818..d840f04 100644 --- a/tempfile.c +++ b/tempfile.c @@ -85,11 +85,11 @@ static void remove_tempfiles_on_signal(int signo) raise(signo); } -/* Make sure errno contains a meaningful value on error */ -int create_tempfile(struct tempfile *tempfile, const char *path) +/* + * Initialize *tempfile if necessary and add it to tempfile_list. + */ +static void prepare_tempfile_object(struct tempfile *tempfile) { - size_t pathlen = strlen(path); - if (!tempfile_list) { /* One-time initialization */ sigchain_push_common(remove_tempfiles_on_signal); @@ -97,21 +97,27 @@ int create_tempfile(struct tempfile *tempfile, const char *path) } if (tempfile->active) - die("BUG: create_tempfile called for active object"); + die("BUG: prepare_tempfile_object called for active object"); if (!tempfile->on_list) { /* Initialize *tempfile and add it to tempfile_list: */ tempfile->fd = -1; tempfile->fp = NULL; tempfile->active = 0; tempfile->owner = 0; - strbuf_init(&tempfile->filename, pathlen); + strbuf_init(&tempfile->filename, 0); tempfile->next = tempfile_list; tempfile_list = tempfile; tempfile->on_list = 1; } else if (tempfile->filename.len) { /* This shouldn't happen, but better safe than sorry. */ - die("BUG: create_tempfile called for improperly-reset object"); + die("BUG: prepare_tempfile_object called for improperly-reset object"); } +} + +/* Make sure errno contains a meaningful value on error */ +int create_tempfile(struct tempfile *tempfile, const char *path) +{ + prepare_tempfile_object(tempfile); strbuf_add_absolute_path(&tempfile->filename, path); tempfile->fd = open(tempfile->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666); -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 03/16] lockfile: add accessors get_lock_file_fd() and get_lock_file_fp()
We are about to move those members, so change client code to read them through accessor functions. Signed-off-by: Michael Haggerty --- credential-store.c | 2 +- lockfile.c | 14 ++ lockfile.h | 3 +++ read-cache.c | 2 +- refs.c | 12 +++- 5 files changed, 26 insertions(+), 7 deletions(-) diff --git a/credential-store.c b/credential-store.c index f692509..00aea3a 100644 --- a/credential-store.c +++ b/credential-store.c @@ -52,7 +52,7 @@ static void print_entry(struct credential *c) static void print_line(struct strbuf *buf) { strbuf_addch(buf, '\n'); - write_or_die(credential_lock.fd, buf->buf, buf->len); + write_or_die(get_lock_file_fd(&credential_lock), buf->buf, buf->len); } static void rewrite_credential_file(const char *fn, struct credential *c, diff --git a/lockfile.c b/lockfile.c index 2369eff..df9c704 100644 --- a/lockfile.c +++ b/lockfile.c @@ -364,6 +364,20 @@ FILE *fdopen_lock_file(struct lock_file *lk, const char *mode) return lk->fp; } +int get_lock_file_fd(struct lock_file *lk) +{ + if (!lk->active) + die("BUG: get_lock_file_fd() called for unlocked object"); + return lk->fd; +} + +FILE *get_lock_file_fp(struct lock_file *lk) +{ + if (!lk->active) + die("BUG: get_lock_file_fp() called for unlocked object"); + return lk->fp; +} + char *get_locked_file_path(struct lock_file *lk) { if (!lk->active) diff --git a/lockfile.h b/lockfile.h index a483cc9..d9dfbc9 100644 --- a/lockfile.h +++ b/lockfile.h @@ -203,6 +203,9 @@ extern NORETURN void unable_to_lock_die(const char *path, int err); */ extern FILE *fdopen_lock_file(struct lock_file *lk, const char *mode); +extern int get_lock_file_fd(struct lock_file *lk); +extern FILE *get_lock_file_fp(struct lock_file *lk); + /* * Return the path of the file that is locked by the specified * lock_file object. The caller must free the memory. diff --git a/read-cache.c b/read-cache.c index 723d48d..96cb9a3 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2112,7 +2112,7 @@ static int commit_locked_index(struct lock_file *lk) static int do_write_locked_index(struct index_state *istate, struct lock_file *lock, unsigned flags) { - int ret = do_write_index(istate, lock->fd, 0); + int ret = do_write_index(istate, get_lock_file_fd(lock), 0); if (ret) return ret; assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) != diff --git a/refs.c b/refs.c index a742d79..0f49a62 100644 --- a/refs.c +++ b/refs.c @@ -3162,6 +3162,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock, { static char term = '\n'; struct object *o; + int fd; o = parse_object(sha1); if (!o) { @@ -3178,8 +3179,9 @@ static int write_ref_to_lockfile(struct ref_lock *lock, errno = EINVAL; return -1; } - if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 || - write_in_full(lock->lk->fd, &term, 1) != 1 || + fd = get_lock_file_fd(lock->lk); + if (write_in_full(fd, sha1_to_hex(sha1), 40) != 40 || + write_in_full(fd, &term, 1) != 1 || close_ref(lock) < 0) { int save_errno = errno; error("Couldn't write %s", lock->lk->filename.buf); @@ -4264,10 +4266,10 @@ int reflog_expire(const char *refname, const unsigned char *sha1, status |= error("couldn't write %s: %s", log_file, strerror(errno)); } else if (update && - (write_in_full(lock->lk->fd, + (write_in_full(get_lock_file_fd(lock->lk), sha1_to_hex(cb.last_kept_sha1), 40) != 40 || -write_str_in_full(lock->lk->fd, "\n") != 1 || -close_ref(lock) < 0)) { + write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 || + close_ref(lock) < 0)) { status |= error("couldn't write %s", lock->lk->filename.buf); rollback_lock_file(&reflog_lock); -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 13/16] lock_repo_for_gc(): compute the path to "gc.pid" only once
Signed-off-by: Michael Haggerty --- builtin/gc.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 36fe333..c41354b 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -199,6 +199,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) uintmax_t pid; FILE *fp; int fd; + char *pidfile_path; if (pidfile) /* already locked */ @@ -207,12 +208,13 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) if (gethostname(my_host, sizeof(my_host))) strcpy(my_host, "unknown"); - fd = hold_lock_file_for_update(&lock, git_path("gc.pid"), + pidfile_path = git_pathdup("gc.pid"); + fd = hold_lock_file_for_update(&lock, pidfile_path, LOCK_DIE_ON_ERROR); if (!force) { static char locking_host[128]; int should_exit; - fp = fopen(git_path("gc.pid"), "r"); + fp = fopen(pidfile_path, "r"); memset(locking_host, 0, sizeof(locking_host)); should_exit = fp != NULL && @@ -236,6 +238,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) if (fd >= 0) rollback_lock_file(&lock); *ret_pid = pid; + free(pidfile_path); return locking_host; } } @@ -246,7 +249,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) strbuf_release(&sb); commit_lock_file(&lock); - pidfile = git_pathdup("gc.pid"); + pidfile = pidfile_path; sigchain_push_common(remove_pidfile_on_signal); atexit(remove_pidfile); -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 04/16] lockfile: add accessor get_lock_file_path()
Signed-off-by: Michael Haggerty --- builtin/commit.c | 15 --- config.c | 14 +++--- lockfile.c | 7 +++ lockfile.h | 6 ++ refs.c | 6 +++--- shallow.c| 6 +++--- 6 files changed, 34 insertions(+), 20 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 254477f..96aee0c 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -324,6 +324,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix struct string_list partial; struct pathspec pathspec; int refresh_flags = REFRESH_QUIET; + const char *ret; if (is_status) refresh_flags |= REFRESH_UNMERGED; @@ -344,7 +345,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix die(_("unable to create temporary index")); old_index_env = getenv(INDEX_ENVIRONMENT); - setenv(INDEX_ENVIRONMENT, index_lock.filename.buf, 1); + setenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1); if (interactive_add(argc, argv, prefix, patch_interactive) != 0) die(_("interactive add failed")); @@ -355,7 +356,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix unsetenv(INDEX_ENVIRONMENT); discard_cache(); - read_cache_from(index_lock.filename.buf); + read_cache_from(get_lock_file_path(&index_lock)); if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) { if (reopen_lock_file(&index_lock) < 0) die(_("unable to write index file")); @@ -365,7 +366,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix warning(_("Failed to update main cache tree")); commit_style = COMMIT_NORMAL; - return index_lock.filename.buf; + return get_lock_file_path(&index_lock); } /* @@ -388,7 +389,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) die(_("unable to write new_index file")); commit_style = COMMIT_NORMAL; - return index_lock.filename.buf; + return get_lock_file_path(&index_lock); } /* @@ -475,9 +476,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix die(_("unable to write temporary index file")); discard_cache(); - read_cache_from(false_lock.filename.buf); - - return false_lock.filename.buf; + ret = get_lock_file_path(&false_lock); + read_cache_from(ret); + return ret; } static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn, diff --git a/config.c b/config.c index ab46462..adf8b53 100644 --- a/config.c +++ b/config.c @@ -2056,9 +2056,9 @@ int git_config_set_multivar_in_file(const char *config_filename, MAP_PRIVATE, in_fd, 0); close(in_fd); - if (chmod(lock->filename.buf, st.st_mode & 0) < 0) { + if (chmod(get_lock_file_path(lock), st.st_mode & 0) < 0) { error("chmod on %s failed: %s", - lock->filename.buf, strerror(errno)); + get_lock_file_path(lock), strerror(errno)); ret = CONFIG_NO_WRITE; goto out_free; } @@ -2138,7 +2138,7 @@ out_free: return ret; write_err_out: - ret = write_error(lock->filename.buf); + ret = write_error(get_lock_file_path(lock)); goto out_free; } @@ -2239,9 +2239,9 @@ int git_config_rename_section_in_file(const char *config_filename, fstat(fileno(config_file), &st); - if (chmod(lock->filename.buf, st.st_mode & 0) < 0) { + if (chmod(get_lock_file_path(lock), st.st_mode & 0) < 0) { ret = error("chmod on %s failed: %s", - lock->filename.buf, strerror(errno)); + get_lock_file_path(lock), strerror(errno)); goto out; } @@ -2262,7 +2262,7 @@ int git_config_rename_section_in_file(const char *config_filename, } store.baselen = strlen(new_name); if (!store_write_section(out_fd, new_name)) { - ret = write_error(lock->filename.buf); + ret = write_error(get_lock_file_path(lock)); goto out; } /* @@ -2288,7 +228
[PATCH v2 11/16] setup_temporary_shallow(): use tempfile module
Signed-off-by: Michael Haggerty --- shallow.c | 35 +++ 1 file changed, 7 insertions(+), 28 deletions(-) diff --git a/shallow.c b/shallow.c index 7973e74..2ba29a5 100644 --- a/shallow.c +++ b/shallow.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "tempfile.h" #include "lockfile.h" #include "commit.h" #include "tag.h" @@ -208,50 +209,28 @@ int write_shallow_commits(struct strbuf *out, int use_pack_protocol, return write_shallow_commits_1(out, use_pack_protocol, extra, 0); } -static struct strbuf temporary_shallow = STRBUF_INIT; - -static void remove_temporary_shallow(void) -{ - if (temporary_shallow.len) { - unlink_or_warn(temporary_shallow.buf); - strbuf_reset(&temporary_shallow); - } -} - -static void remove_temporary_shallow_on_signal(int signo) -{ - remove_temporary_shallow(); - sigchain_pop(signo); - raise(signo); -} +static struct tempfile temporary_shallow; const char *setup_temporary_shallow(const struct sha1_array *extra) { struct strbuf sb = STRBUF_INIT; int fd; - if (temporary_shallow.len) - die("BUG: attempt to create two temporary shallow files"); - if (write_shallow_commits(&sb, 0, extra)) { - strbuf_addstr(&temporary_shallow, git_path("shallow_XX")); - fd = xmkstemp(temporary_shallow.buf); - - atexit(remove_temporary_shallow); - sigchain_push_common(remove_temporary_shallow_on_signal); + fd = xmks_tempfile(&temporary_shallow, git_path("shallow_XX")); if (write_in_full(fd, sb.buf, sb.len) != sb.len) die_errno("failed to write to %s", - temporary_shallow.buf); - close(fd); + get_tempfile_path(&temporary_shallow)); + close_tempfile(&temporary_shallow); strbuf_release(&sb); - return temporary_shallow.buf; + return get_tempfile_path(&temporary_shallow); } /* * is_repository_shallow() sees empty string as "no shallow * file". */ - return temporary_shallow.buf; + return get_tempfile_path(&temporary_shallow); } void setup_alternate_shallow(struct lock_file *shallow_lock, -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 14/16] gc: use tempfile module to handle gc.pid file
Signed-off-by: Michael Haggerty --- builtin/gc.c | 25 + 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index c41354b..bfe589f 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -11,6 +11,7 @@ */ #include "builtin.h" +#include "tempfile.h" #include "lockfile.h" #include "parse-options.h" #include "run-command.h" @@ -42,20 +43,7 @@ static struct argv_array prune = ARGV_ARRAY_INIT; static struct argv_array prune_worktrees = ARGV_ARRAY_INIT; static struct argv_array rerere = ARGV_ARRAY_INIT; -static char *pidfile; - -static void remove_pidfile(void) -{ - if (pidfile) - unlink(pidfile); -} - -static void remove_pidfile_on_signal(int signo) -{ - remove_pidfile(); - sigchain_pop(signo); - raise(signo); -} +static struct tempfile pidfile; static void git_config_date_string(const char *key, const char **output) { @@ -201,7 +189,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) int fd; char *pidfile_path; - if (pidfile) + if (is_tempfile_active(&pidfile)) /* already locked */ return NULL; @@ -248,11 +236,8 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) write_in_full(fd, sb.buf, sb.len); strbuf_release(&sb); commit_lock_file(&lock); - - pidfile = pidfile_path; - sigchain_push_common(remove_pidfile_on_signal); - atexit(remove_pidfile); - + register_tempfile(&pidfile, pidfile_path); + free(pidfile_path); return NULL; } -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 05/16] commit_lock_file(): use get_locked_file_path()
First beef up the sanity checking in get_locked_file_path() to match that in commit_lock_file(). Then rewrite commit_lock_file() to use get_locked_file_path() for its pathname computation. Signed-off-by: Michael Haggerty --- lockfile.c | 28 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/lockfile.c b/lockfile.c index 5e954ba..3904803 100644 --- a/lockfile.c +++ b/lockfile.c @@ -389,8 +389,10 @@ char *get_locked_file_path(struct lock_file *lk) { if (!lk->active) die("BUG: get_locked_file_path() called for unlocked object"); - if (lk->filename.len <= LOCK_SUFFIX_LEN) + if (lk->filename.len <= LOCK_SUFFIX_LEN || + strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX)) die("BUG: get_locked_file_path() called for malformed lock object"); + /* remove ".lock": */ return xmemdupz(lk->filename.buf, lk->filename.len - LOCK_SUFFIX_LEN); } @@ -458,22 +460,16 @@ int commit_lock_file_to(struct lock_file *lk, const char *path) int commit_lock_file(struct lock_file *lk) { - static struct strbuf result_file = STRBUF_INIT; - int err; + char *result_path = get_locked_file_path(lk); - if (!lk->active) - die("BUG: attempt to commit unlocked object"); - - if (lk->filename.len <= LOCK_SUFFIX_LEN || - strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX)) - die("BUG: lockfile filename corrupt"); - - /* remove ".lock": */ - strbuf_add(&result_file, lk->filename.buf, - lk->filename.len - LOCK_SUFFIX_LEN); - err = commit_lock_file_to(lk, result_file.buf); - strbuf_reset(&result_file); - return err; + if (commit_lock_file_to(lk, result_path)) { + int save_errno = errno; + free(result_path); + errno = save_errno; + return -1; + } + free(result_path); + return 0; } void rollback_lock_file(struct lock_file *lk) -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 08/16] tempfile: add several functions for creating temporary files
Add several functions for creating temporary files with automatically-generated names, analogous to mkstemps(), but also arranging for the files to be deleted on program exit. The functions are named according to a pattern depending how they operate. They will be used to replace many places in the code where temporary files are created and cleaned up ad-hoc. Signed-off-by: Michael Haggerty --- tempfile.c | 53 ++ tempfile.h | 96 ++ 2 files changed, 149 insertions(+) diff --git a/tempfile.c b/tempfile.c index d840f04..0b5d8ce 100644 --- a/tempfile.c +++ b/tempfile.c @@ -137,6 +137,59 @@ int create_tempfile(struct tempfile *tempfile, const char *path) return tempfile->fd; } +int mks_tempfile_sm(struct tempfile *tempfile, + const char *template, int suffixlen, int mode) +{ + prepare_tempfile_object(tempfile); + + strbuf_add_absolute_path(&tempfile->filename, template); + tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode); + if (tempfile->fd < 0) { + strbuf_reset(&tempfile->filename); + return -1; + } + tempfile->owner = getpid(); + tempfile->active = 1; + return tempfile->fd; +} + +int mks_tempfile_tsm(struct tempfile *tempfile, +const char *template, int suffixlen, int mode) +{ + const char *tmpdir; + + prepare_tempfile_object(tempfile); + + tmpdir = getenv("TMPDIR"); + if (!tmpdir) + tmpdir = "/tmp"; + + strbuf_addf(&tempfile->filename, "%s/%s", tmpdir, template); + tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode); + if (tempfile->fd < 0) { + strbuf_reset(&tempfile->filename); + return -1; + } + tempfile->owner = getpid(); + tempfile->active = 1; + return tempfile->fd; +} + +int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode) +{ + int fd; + struct strbuf full_template = STRBUF_INIT; + + strbuf_add_absolute_path(&full_template, template); + fd = mks_tempfile_m(tempfile, full_template.buf, mode); + if (fd < 0) + die_errno("Unable to create temporary file '%s'", + full_template.buf); + + strbuf_release(&full_template); + return fd; +} + FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode) { if (!tempfile->active) diff --git a/tempfile.h b/tempfile.h index bcc229f..a30e12c 100644 --- a/tempfile.h +++ b/tempfile.h @@ -92,6 +92,102 @@ struct tempfile { */ extern int create_tempfile(struct tempfile *tempfile, const char *path); + +/* + * mks_tempfile functions + * + * The following functions attempt to create and open temporary files + * with names derived automatically from a template, in the manner of + * mkstemps(), and arrange for them to be deleted if the program ends + * before they are deleted explicitly. There is a whole family of such + * functions, named according to the following pattern: + * + * x?mks_tempfile_t?s?m?() + * + * The optional letters have the following meanings: + * + * x - die if the temporary file cannot be created. + * + * t - create the temporary file under $TMPDIR (as opposed to + * relative to the current directory). When these variants are + * used, template should be the pattern for the filename alone, + * without a path. + * + * s - template includes a suffix that is suffixlen characters long. + * + * m - the temporary file should be created with the specified mode + * (otherwise, the mode is set to 0600). + * + * None of these functions modify template. If the caller wants to + * know the (absolute) path of the file that was created, it can be + * read from tempfile->filename. + * + * On success, the functions return a file descriptor that is open for + * writing the temporary file. On errors, they return -1 and set errno + * appropriately (except for the "x" variants, which die() on errors). + */ + +/* See "mks_tempfile functions" above. */ +extern int mks_tempfile_sm(struct tempfile *tempfile, + const char *template, int suffixlen, int mode); + +/* See "mks_tempfile functions" above. */ +static inline int mks_tempfile_s(struct tempfile *tempfile, +const char *template, int suffixlen) +{ + return mks_tempfile_sm(tempfile, template, suffixlen, 0600); +} + +/* See "mks_tempfile functions" above. */ +static inline int mks_tempfile_m(struct tempfile *tempfile, +const char *template, int mode) +{ + return mks_tempfile_sm(tempfile, template, 0, mode); +} + +/* See "mks_tempfile functions" above. */ +static inline int mks_tempfile(struct tempfile *tempfile, + const char *template) +{ + return mks_tempfile_sm
[PATCH v2 01/16] Move lockfile documentation to lockfile.h and lockfile.c
Rearrange/rewrite it somewhat to fit its new environment. Signed-off-by: Michael Haggerty --- Documentation/technical/api-lockfile.txt | 220 --- lockfile.c | 53 ++ lockfile.h | 290 --- 3 files changed, 283 insertions(+), 280 deletions(-) delete mode 100644 Documentation/technical/api-lockfile.txt diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt deleted file mode 100644 index 93b5f23..000 --- a/Documentation/technical/api-lockfile.txt +++ /dev/null @@ -1,220 +0,0 @@ -lockfile API - - -The lockfile API serves two purposes: - -* Mutual exclusion and atomic file updates. When we want to change a - file, we create a lockfile `.lock`, write the new file - contents into it, and then rename the lockfile to its final - destination ``. We create the `.lock` file with - `O_CREAT|O_EXCL` so that we can notice and fail if somebody else has - already locked the file, then atomically rename the lockfile to its - final destination to commit the changes and unlock the file. - -* Automatic cruft removal. If the program exits after we lock a file - but before the changes have been committed, we want to make sure - that we remove the lockfile. This is done by remembering the - lockfiles we have created in a linked list and setting up an - `atexit(3)` handler and a signal handler that clean up the - lockfiles. This mechanism ensures that outstanding lockfiles are - cleaned up if the program exits (including when `die()` is called) - or if the program dies on a signal. - -Please note that lockfiles only block other writers. Readers do not -block, but they are guaranteed to see either the old contents of the -file or the new contents of the file (assuming that the filesystem -implements `rename(2)` atomically). - - -Calling sequence - - -The caller: - -* Allocates a `struct lock_file` either as a static variable or on the - heap, initialized to zeros. Once you use the structure to call the - `hold_lock_file_*` family of functions, it belongs to the lockfile - subsystem and its storage must remain valid throughout the life of - the program (i.e. you cannot use an on-stack variable to hold this - structure). - -* Attempts to create a lockfile by passing that variable and the path - of the final destination (e.g. `$GIT_DIR/index`) to - `hold_lock_file_for_update` or `hold_lock_file_for_append`. - -* Writes new content for the destination file by either: - - * writing to the file descriptor returned by the `hold_lock_file_*` -functions (also available via `lock->fd`). - - * calling `fdopen_lock_file` to get a `FILE` pointer for the open -file and writing to the file using stdio. - -When finished writing, the caller can: - -* Close the file descriptor and rename the lockfile to its final - destination by calling `commit_lock_file` or `commit_lock_file_to`. - -* Close the file descriptor and remove the lockfile by calling - `rollback_lock_file`. - -* Close the file descriptor without removing or renaming the lockfile - by calling `close_lock_file`, and later call `commit_lock_file`, - `commit_lock_file_to`, `rollback_lock_file`, or `reopen_lock_file`. - -Even after the lockfile is committed or rolled back, the `lock_file` -object must not be freed or altered by the caller. However, it may be -reused; just pass it to another call of `hold_lock_file_for_update` or -`hold_lock_file_for_append`. - -If the program exits before you have called one of `commit_lock_file`, -`commit_lock_file_to`, `rollback_lock_file`, or `close_lock_file`, an -`atexit(3)` handler will close and remove the lockfile, rolling back -any uncommitted changes. - -If you need to close the file descriptor you obtained from a -`hold_lock_file_*` function yourself, do so by calling -`close_lock_file`. You should never call `close(2)` or `fclose(3)` -yourself! Otherwise the `struct lock_file` structure would still think -that the file descriptor needs to be closed, and a commit or rollback -would result in duplicate calls to `close(2)`. Worse yet, if you close -and then later open another file descriptor for a completely different -purpose, then a commit or rollback might close that unrelated file -descriptor. - - -Error handling --- - -The `hold_lock_file_*` functions return a file descriptor on success -or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On -errors, `errno` describes the reason for failure. Errors can be -reported by passing `errno` to one of the following helper functions: - -unable_to_lock_message:: - - Append an appropriate error message to a `strbuf`. - -unable_to_lock_error:: - - Emit an appropriate error message using `error()`. - -unable_to_lock_die:: - - Emit an appropriate error message and `die()`. - -Similarly, `commit_lock_file`, `commit_lock_file_to`, and -`close_lo
Re: Error pushing new branch: value too great for base (error token is...
Apologies for the delay in reply! I tried your suggestion and it works. Thanks! :) I'm curious why integer comparison is throwing error. Shouldn't i be comparing numbers with numeric operator? On Mon, Aug 10, 2015 at 3:23 PM, Gaurav Chhabra wrote: > Apologies for the delay in reply! I tried your suggestion and it works. > Thanks! :) > > I'm curious why integer comparison is throwing error. Shouldn't i be > comparing numbers with numeric operator? > > > On Wed, Aug 5, 2015 at 11:23 PM, Eric Sunshine > wrote: >> >> On Wed, Aug 5, 2015 at 1:32 PM, Gaurav Chhabra >> wrote: >> > I had written the following code to check whether a push is for branch >> > deletion: >> > >> > #!/bin/bash >> > >> > NULL="" >> > if [[ "$new_sha" -eq "$NULL" ]]; then # Line 17 >> > remote: Stdin: [] >> > [9226289d2416af4cb7365d7aaa5e382bdb3d9a89] [refs/heads/rel-a] >> > remote: >> > remote: hooks/pre-receive: line 17: [[: >> > 9226289d2416af4cb7365d7aaa5e382bdb3d9a89: value too great for base >> > (error token is "922628 >> > 9d2416af4cb7365d7aaa5e382bdb3d9a89") >> > >> > Although the new branch gets pushed to remote but i'm not sure why i'm >> > getting this error and how can i fix it. I checked online and i get >> > few links where folks had similar issue but in each such case, the >> > error token was 08 or 09. I still tried the suggestion of using "10#" >> > in front of my $new_sha variable but to no avail. >> > >> > Any suggestions? >> >> Yes, try using the string comparison '=' operator rather than the >> numeric comparison operator '-eq'. >> >> if [[ "$new_sha" = "$NULL" ]]; then > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms
On Sun, 9 Aug 2015 14:13:33 -0400 Eric Sunshine wrote: > On Wed, Aug 5, 2015 at 3:17 AM, Jan Viktorin > wrote: > > Do I understand well that you are complaining about too > > narrow commmit message? > > Yes, I'm a complainer. ;-) It's minor, though, not a big deal, and > certainly not worth a re-roll if that was the only issue. In fact, > other than the undesirable "Supported:" line in the documentation, all > comments on v2 were minor and not demanding of a re-roll. :) > > > I am trying to figure out how to write a test. It is > > not very clear to me, what the testing suite does. My > > attempt looks this way at the moment: > > > > 1657 do_smtp_auth_test() { > > 1658 git send-email \ > > 1659 --from="Example " \ > > 1660 --to=some...@example.com \ > > 1661 --smtp-server="$(pwd)/fake.sendmail" \ > > 1662 --smtp-auth="$1" \ > > 1663 -v \ > > 1664 0001-*.patch \ > > 1665 2>errors >out > > 1666 } > > 1667 > > 1668 test_expect_success $PREREQ 'accepts SMTP AUTH mechanisms (see > > RFC-4422, p. 8)' ' 1669 do_smtp_auth_test "PLAIN LOGIN > > CRAM-MD5 DIGEST-MD5 GSSAPI EXTERNAL ANONYMOUS" && 1670 > > do_smtp_auth_test "ABCDEFGHIKLMNOPQRSTUVWXYZ 0123456789_-" > > Wouldn't this one fail the regex check you added which limits the > length to 20 characters? Yes, it would fail. But it does not work anyway... > > > 1671 ' > > 1672 > > 1673 test_expect_success $PREREQ 'does not accept non-RFC-4422 > > strings for SMTP AUTH' ' 1674 test_must_fail > > do_smtp_auth_test "../ATTACK" && 1675 test_must_fail > > do_smtp_auth_test "TOO-LONG-BUT-VALID-STRING" && 1676 > > test_must_fail do_smtp_auth_test "no-lower-case-sorry" 1677 ' > > > > * I do not know yet, what to check after each do_smtp_auth_test > > call. > > If you were able somehow to capture the interaction with > Auth::SASL::Perl, then you'd probably want to test if it received the > whitelisted mechanisms specified via --smtp-auth, however... (see > below) --smtp-debug > > > * Perhaps, each case should have its own test_expect_success call? > > The grouping seems okay as-is. > > > * Why send-email -v does not generate any output? > > As far as I know, git-send-email doesn't accept a -v flag. True, I confused it with --smtp-debug. However, what I did not understand was the testing framework. The TAP harness discards everything (I expected some automatic redirection to a file for each test.). Later I found the --verbose option that allows to see some output from tests. > > > (I found a directory 'trash directory.t9001-send-email', however, > > the errors file is always empty.) > > Was it empty even for the cases which should have triggered the > validation regex to invoke die()? > > > * Is there any other place where the files out, errors are placed? > > No. > > > * I have no idea what the fake.sendmail does (I could see its > > contents but still...). Is it suitable for my tests? > > It dumps its command-line arguments to one file ("commandline") and > its stdin to another ("msgtxt"), but otherwise does no work. This is > useful for tests which need to make sure that the command-line and/or > message content gets augmented in some way, but won't help your case > since it can't capture the script's interaction with > Authen::SASL::Perl. I can see it now. Either Perl implementation or a sendmail binary is used. Unfortunately, this is very unfriendly for such testing. > > > * Should I check the behaviour '--smtp-auth overrides > > sendemail.smtpAuth'? > > That would be nice, but there doesn't seem to be a good way to do it > via an existing testing mechanism since you can't check the > git-sendemail's interaction with Auth::SASL::Perl. The same holds for > your question above about what to check after each do_smtp_auth_test() > call. > > One possibility which comes to mind is to create a fake > Authen::SASL::Perl which merely dumps its input mechanisms to a file, > and arrange for the Perl search path to find the fake one instead. You > could then check the output file to see if it reflects your > expectations. However, this may be overkill and perhaps not worth the > effort (especially if you're not a Perl programmer). I think that Authen::SASL::Perl mock would not help. I wanted to create some fake sendmail (but this is impossible as stated above because then the perl modules are not used). So the only way would be to provide some fake socket with a static content on the other side. This is really an overkill to just test the few lines of code. So, what more can I do for this feature? I think that the basic regex test is OK. It can accept lowercase letters and do an explicit uppercase call. I do not like to rely on internals of the SASL library. As you could see, the SASL::Perl does not check its inputs in a very good way and its code is quite unclear (strange for a library providing securi
Re: [PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing
On 08/10/2015 11:36 AM, Jeff King wrote: > 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 > --- > refs.c | 38 +++--- > 1 file changed, 15 insertions(+), 23 deletions(-) > > diff --git a/refs.c b/refs.c > index 06f95c4..3666132 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) > { > [...] > @@ -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 *log_file, int flags, > struct strbuf *err) > { > [...] Nice change. How about taking this opportunity to name the parameters ("logfile" vs. "log_file") consistently? Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Feature: git stash pop --always-drop
I would find it useful to ask 'git stash pop' to always drop the stash after applying it to the working tree, even if there were conflicts. (Only if there was some hard error, such as an I/O error updating some of the files, should the stash be left on the stack.) Would a patch for such an --always-drop flag be accepted? -- Ed Avis -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/17] memoize common git-path "constant" files
On 08/10/2015 11:38 AM, Jeff King wrote: > [...] > 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. > [...] I was wondering whether this memoization could interact badly with update_common_dir(). For example, if any of the memoized functions were called before git_common_dir is initialized, then the pre-git_common_dir value would continue to be used even if git_common_dir is changed afterwards. But I believe it is taboo to call git_path() before setup_git_env(), so I think this is not a problem. Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/17] removing questionable uses of git_path
On 08/10/2015 11:27 AM, Jeff King wrote: > [...] The problem is that git_path uses a static buffer that gets overwritten > by subsequent calls. [...] > > [01/17]: cache.h: clarify documentation for git_path, et al > [02/17]: cache.h: complete set of git_path_submodule helpers > [03/17]: t5700: modernize style > [04/17]: add_to_alternates_file: don't add duplicate entries > [05/17]: remove hold_lock_file_for_append > [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases > [07/17]: prefer mkpathdup to mkpath in assignments > [08/17]: remote.c: drop extraneous local variable from migrate_file > [09/17]: refs.c: remove extra git_path calls from read_loose refs > [10/17]: path.c: drop git_path_submodule > [11/17]: refs.c: simplify strbufs in reflog setup and writing > [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log > [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic > [14/17]: refs.c: remove_empty_directories can take a strbuf > [15/17]: find_hook: keep our own static buffer > [16/17]: get_repo_path: refactor path-allocation > [17/17]: memoize common git-path "constant" files I read through all of the patches (except for 03) and didn't see any problems. Thanks, Peff, for defusing some grenades :-) Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing
On Mon, Aug 10, 2015 at 12:34:05PM +0200, Michael Haggerty wrote: > How about taking this opportunity to name the parameters ("logfile" vs. > "log_file") consistently? Thanks, that's a good suggestion. Here's a re-roll of this patch: -- >8 -- Subject: [PATCH] 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 --- refs.c | 38 +++--- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/refs.c b/refs.c index 06f95c4..105f271 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) { -
Bug report Windows 10
My Windows notebook got updated to Windows 10 and now my Git Bash doesn't start and when I open an existing repository from Git Gui, I am getting next error: "0 [main] us 0 init_cheap: VirtualAlloc pointer is null, Win32 error 487 AllocationBase 0x0, BaseAddress 0x6857, RegionSize 0x41, State 0x1 C:\Program Files (x86)\Git\bin\sh.exe: *** Couldn't reserve space for cygwin's heap, Win32 error 0" I am running GIT version 1.9.5 (latest build for Windows). Can someone help me? Thanks for your reply. Regards, Michel. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/17] memoize common git-path "constant" files
On Mon, Aug 10, 2015 at 02:05:14PM +0200, Michael Haggerty wrote: > I was wondering whether this memoization could interact badly with > update_common_dir(). For example, if any of the memoized functions were > called before git_common_dir is initialized, then the pre-git_common_dir > value would continue to be used even if git_common_dir is changed > afterwards. But I believe it is taboo to call git_path() before > setup_git_env(), so I think this is not a problem. Calling git_path() actually ends up in setup_git_env(), because we lazily call it from get_git_dir(). So if somebody tries to tweak common_dir stuff too late, we could in theory cache a bogus value; but the real problem is generating the bogosity in the first place. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug report Windows 10
On Mon, 10 Aug 2015 14:26:44 +0200 "MS-Informatique" wrote: > My Windows notebook got updated to Windows 10 and now my Git Bash > doesn't start and when I open an existing repository from Git Gui, I > am getting next error: > "0 [main] us 0 init_cheap: VirtualAlloc pointer is null, Win32 error > 487 AllocationBase 0x0, BaseAddress 0x6857, RegionSize 0x41, > State 0x1 > C:\Program Files (x86)\Git\bin\sh.exe: *** Couldn't reserve space for > cygwin's heap, Win32 error 0" > > I am running GIT version 1.9.5 (latest build for Windows). This version is essentially abandoned because the development happens on packaging the 2.x series for Windows, and using the updated runtime (MSYS2 instead of MSYS). So please try this: 1) Fetch the latest version of the new Git for Windows available -- currently its 2.4.6 RC5 [1]. Supposedly you should try 64-bit version. 2) If the bug still does manifest itself, prease report it to the project's tracker [2]. 1. https://github.com/git-for-windows/git/releases/tag/v2.4.6.windows.1 2. https://github.com/git-for-windows/git/issues/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Feature: git stash pop --always-drop
On Mon, Aug 10, 2015 at 10:42:30AM +, Ed Avis wrote: > I would find it useful to ask 'git stash pop' to always drop the stash after > applying it to the working tree, even if there were conflicts. (Only if there > was some hard error, such as an I/O error updating some of the files, should > the stash be left on the stack.) Hmm. That seems rather dangerous, but hey, if it's not the default, I guess it is your own foot that you are shooting. > Would a patch for such an --always-drop flag be accepted? I doubt you will get a good answer to that question; the attitude here is usually "well, we would have to see the patch". For instance, I don't know how easy it will be to tell merge conflicts apart from I/O errors. Figuring that out will probably be part of a rough draft patch. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Feature: git stash pop --always-drop
An alternative would be for git stash to always print the name of the stash it is applying. Then you can drop it afterwards by name and be sure you got the right one. Printing the name of the stash sounds like a reasonable bit of chatter to add anyway, do you agree? -- Ed Avis -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug report Windows 10
Hi, On 2015-08-10 14:26, MS-Informatique wrote: > My Windows notebook got updated to Windows 10 and now my Git Bash doesn't > start and when I open an existing repository from Git Gui, I am getting next > error: > "0 [main] us 0 init_cheap: VirtualAlloc pointer is null, Win32 error 487 > AllocationBase 0x0, BaseAddress 0x6857, RegionSize 0x41, State > 0x1 > C:\Program Files (x86)\Git\bin\sh.exe: *** Couldn't reserve space for > cygwin's heap, Win32 error 0" > > I am running GIT version 1.9.5 (latest build for Windows). > > Can someone help me? First of all, the home page of Git for Windows has hints where to report bugs (and where to look for possible resolutions first). Second, this issue is so common that I wrote a wiki page about it: https://github.com/git-for-windows/git/wiki/32-bit-issues Short version: reinstall Git for Windows. Preferably a 64-bit Git for Windows 2.x from https://git-for-windows.github.io/. And you might want to heed the advice given in http://git-for-windows.github.io/#contribute when you want to report bugs ;-) Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Feature: git stash pop --always-drop
On Mon, Aug 10, 2015 at 12:50:51PM +, Ed Avis wrote: > An alternative would be for git stash to always print the name of the stash > it is applying. Then you can drop it afterwards by name and be sure you got > the right one. Printing the name of the stash sounds like a reasonable > bit of chatter to add anyway, do you agree? Yeah, I agree that makes sense. We already show something like: Dropped refs/stash@{0} (31cb86c3d700d241e315d989f460e3e83f84fa19) when dropping. Perhaps model the message after that: Applying refs/stash@{0} (31cb86c3d700d241e315d989f460e3e83f84fa19) Or maybe it would be useful to actually show the stash subject, though the defaults are not very informative (just "WIP on master", and then some totally irrelevant commit subject). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names
On Mon, Aug 10, 2015 at 11:46:06AM +0200, SZEDER Gábor wrote: > 'git config' can only show values or name-value pairs, so if a shell > script needs the names of set config variables it has to run 'git config > --list' or '--get-regexp' and parse the output to separate config > variable names from their values. However, such a parsing can't cope > with multi-line values. Though 'git config' can produce null-terminated > output for newline-safe parsing, that's of no use in such a case, becase > shells can't cope with null characters. > > Even our own bash completion script suffers from these issues. > > Help the completion script, and shell scripts in general, by introducing > the '--names-only' option to modify the output of '--list' and > '--get-regexp' to list only the names of config variables, so they don't > have to perform error-prone post processing to separate variable names > from their values anymore. Nice. The whole thing looks very neatly done. I have only one minor nit: the option "--names-only" is _almost_ the same as the "--name-only" diff option which is somewhat similar. Obviously they do different things and do not need to match, but I wonder if it would create less annoyance to just give them the same name. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Feature: git stash pop --always-drop
Jeff King peff.net> writes: >>An alternative would be for git stash to always print the name of the stash >>it is applying. > Applying refs/stash@{0} (31cb86c3d700d241e315d989f460e3e83f84fa19) Yes, that's the one. >Or maybe it would be useful to actually show the stash subject, That could be nice to see, but is not a substitute for the SHA. If the stash pop failed because of conflicts then it could even print To drop this stash manually, run 'git stash drop abcde...' Another feature I would like to see is a kind of atomic stash apply, where either the whole change can be applied to the working tree without conflicts, or nothing happens. -- Ed Avis -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only'
On Mon, Aug 10, 2015 at 11:46:07AM +0200, SZEDER Gábor wrote: > Use the new '--names-only' option added in the previous commit to list > config variable names reliably in both cases, without error-prone post > processing. > > Signed-off-by: SZEDER Gábor This looks like a pretty straight-forward application of part 1, and the resulting code is much nicer to read. Both patches: Acked-by: Jeff King though I do not think my "ack" on completion code is worth anything. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Feature: git stash pop --always-drop
On Mon, Aug 10, 2015 at 01:43:07PM +, Ed Avis wrote: > Jeff King peff.net> writes: > > >>An alternative would be for git stash to always print the name of the stash > >>it is applying. > > > Applying refs/stash@{0} (31cb86c3d700d241e315d989f460e3e83f84fa19) > > Yes, that's the one. > > >Or maybe it would be useful to actually show the stash subject, > > That could be nice to see, but is not a substitute for the SHA. I think you'd be _technically_ OK without the sha1 in the "applying message", because you can refer to it as stash@{0} until it is dropped, and the drop message does mention the sha1. But that seems needlessly complicated for the user. I agree that including the sha1 is reasonable (though we might want to use an abbreviated one if there is other stuff to go on the line). > If the stash pop failed because of conflicts then it could even print > > To drop this stash manually, run 'git stash drop abcde...' Yup, that makes sense. You might want to make it optional an advice.* config key, though. I also wondered if the "dropped" message is sufficiently clear to new users. The point of it, I think, is to allow a final "oops, I didn't mean to do that" moment. But there are no instructions for how one would re-create the same stash. It might be that showing instructions on even successful drops would quickly get annoying, though. I dunno. I tend to turn off most of our advice config myself. > Another feature I would like to see is a kind of atomic stash apply, where > either the whole change can be applied to the working tree without conflicts, > or nothing happens. I think that may be a bit harder, as the merge machinery would have to know how to be atomic. Still, I agree it's a good goal if you'd like to work on it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Feature: git stash pop --always-drop
> Yes, my use case is that I get confused about whether the stash has been dropped or not and whether I might have stashed something else in the meantime. So for me plain 'git stash drop' feels a bit dangerous. Jeff King peff.net> writes: >I also wondered if the "dropped" message is >sufficiently clear to new users. The point of it, I think, is to allow a >final "oops, I didn't mean to do that" moment. But there are no >instructions for how one would re-create the same stash. Right - myself I didn't even realize that recreating the stash was possible (though I was vaguely aware that old stashes float around somewhere until they are garbage collected many months later). git stash is a relatively infrequent operation and quite exotic, so it wouldn't hurt to add lots of chatter to it. >>Another feature I would like to see is a kind of atomic stash apply, >I think that may be a bit harder, as the merge machinery would have to >know how to be atomic. If git merge-recursive had a --dry-run flag that might take care of it. -- Ed Avis -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
File Hash for Windows executable
Hello, I've downloaded the Windows installer for Git, but can't seem to find hashes published anywhere for verification. Since the downloaded file doesn't seem to be signed, I'll need another way to verify it's integrity before running it on my system. Are hashes published somewhere that I'm missing? If not, is this planned for the future? Thank you, Ryan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Feature: git stash pop --always-drop
Ed Avis writes: > Yes, my use case is that I get confused about whether the stash has been > dropped or not and whether I might have stashed something else in the > meantime. So for me plain 'git stash drop' feels a bit dangerous. Then "git stash apply" followed by "git stash drop" would be a pair of good workflow elements for you, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Feature: git stash pop --always-drop
Junio C Hamano pobox.com> writes: >>Yes, my use case is that I get confused about whether the stash has been >>dropped or not and whether I might have stashed something else in the >>meantime. So for me plain 'git stash drop' feels a bit dangerous. > >Then "git stash apply" followed by "git stash drop" would be a pair >of good workflow elements for you, no? I like ordinary 'git stash pop' when it applies cleanly. Only in the cases where it has conflicts and leaves the stash in place does it get a bit awkward. I manually resolve the conflicts and then 'git stash drop', but that last step is a bit dangerous because it might drop an unrelated stash if I have done some other stashing in the meantime. If 'git stash pop' (and 'apply') would always print the name of the stash, then it would be easy to drop that particular stash afterwards. Running one too many or one too few 'git stash drop' commands would no longer cause problems. Printing the name of the stash would, for me, largely remove the need for an --always-drop option to git stash, which is what I at first suggested. -- Ed Avis -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 5/5] clone: abort if no dir name could be guessed
Due to various components of the URI being stripped off it may happen that we fail to guess a directory name. We currently error out with a message that it is impossible to create the working tree '' in such cases. Instead, error out early with a sensible error message hinting that a directory name should be specified manually on the command line. Signed-off-by: Patrick Steinhardt --- builtin/clone.c | 4 1 file changed, 4 insertions(+) diff --git a/builtin/clone.c b/builtin/clone.c index e7f16ff..5169746 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -215,6 +215,10 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare) len = end - start; strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git"); + if (!len || (len == 1 && *start == '/')) + die("No directory name could be guessed.\n" + "Please specify a directory on the command line"); + if (is_bare) dir = xstrfmt("%.*s.git", (int)len, start); else -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/5] clone: add tests for output directory
From: Jeff King When we run "git clone $url", clone guesses from the $url what to name the local output directory. We don't have any test coverage of this, so let's add some basic tests. This reveals a few problems: - cloning "foo.git/" does not properly remove the ".git"; this is a recent regression from 7e837c6 (clone: simplify string handling in guess_dir_name(), 2015-07-09) - likewise, cloning foo/.git does not seem to handle the bare case (we should end up in foo.git, but we try to use foo/.git on the local end), which also comes from 7e837c6. - cloning the root is not very smart about URL parsing, and usernames and port numbers may end up in the directory name All of these tests are marked as failures. Signed-off-by: Jeff King --- t/t5603-clone-dirname.sh | 103 +++ 1 file changed, 103 insertions(+) create mode 100755 t/t5603-clone-dirname.sh diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh new file mode 100755 index 000..b009a87 --- /dev/null +++ b/t/t5603-clone-dirname.sh @@ -0,0 +1,103 @@ +#!/bin/sh + +test_description='check output directory names used by git-clone' +. ./test-lib.sh + +# we use a fake ssh wrapper that ignores the arguments +# entirely; we really only care that we get _some_ repo, +# as the real test is what clone does on the local side +test_expect_success 'setup ssh wrapper' ' + write_script "$TRASH_DIRECTORY/ssh-wrapper" <<-\EOF && + git upload-pack "$TRASH_DIRECTORY" + EOF + GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" && + export GIT_SSH && + export TRASH_DIRECTORY +' + +# make sure that cloning $1 results in local directory $2 +test_clone_dir () { + url=$1; shift + dir=$1; shift + expect=success + bare=non-bare + clone_opts= + for i in "$@"; do + case "$i" in + fail) + expect=failure + ;; + bare) + bare=bare + clone_opts=--bare + ;; + esac + done + test_expect_$expect "clone of $url goes to $dir ($bare)" " + rm -rf $dir && + git clone $clone_opts $url && + test_path_is_dir $dir + " +} + +# basic syntax with bare and non-bare variants +test_clone_dir host:foo foo +test_clone_dir host:foo foo.git bare +test_clone_dir host:foo.git foo +test_clone_dir host:foo.git foo.git bare +test_clone_dir host:foo/.git foo +test_clone_dir host:foo/.git foo.git bare fail + +# similar, but using ssh URL rather than host:path syntax +test_clone_dir ssh://host/foo foo +test_clone_dir ssh://host/foo foo.git bare +test_clone_dir ssh://host/foo.git foo +test_clone_dir ssh://host/foo.git foo.git bare +test_clone_dir ssh://host/foo/.git foo +test_clone_dir ssh://host/foo/.git foo.git bare fail + +# we should remove trailing slashes and .git suffixes +test_clone_dir ssh://host/foo/ foo +test_clone_dir ssh://host/foo/// foo +test_clone_dir ssh://host/foo.git/ foo fail +test_clone_dir ssh://host/foo.git/// foo fail +test_clone_dir ssh://host/foo///.git/ foo +test_clone_dir ssh://host/foo/.git/// foo + +test_clone_dir host:foo/ foo +test_clone_dir host:foo/// foo +test_clone_dir host:foo.git/ foo fail +test_clone_dir host:foo.git/// foo fail +test_clone_dir host:foo///.git/ foo +test_clone_dir host:foo/.git/// foo + +# omitting the path should default to the hostname +test_clone_dir ssh://host/ host +test_clone_dir ssh://host:1234/ host fail +test_clone_dir ssh://user@host/ host fail +test_clone_dir host:/ host fail + +# auth materials should be redacted +test_clone_dir ssh://user:password@host/ host fail +test_clone_dir ssh://user:password@host:1234/ host fail +test_clone_dir ssh://user:passw@rd@host:1234/ host fail +test_clone_dir user@host:/ host fail +test_clone_dir user:password@host:/ host fail +test_clone_dir user:pass@wrd@host:/ host fail + +# auth-like material should not be dropped +test_clone_dir ssh://host/foo@bar foo@bar +test_clone_dir ssh://host/f...@bar.git foo@bar +test_clone_dir ssh://user:password@host/foo@bar foo@bar +test_clone_dir ssh://user:passw@rd@host/f...@bar.git foo@bar + +test_clone_dir host:/foo@bar foo@bar +test_clone_dir host:/f...@bar.git foo@bar +test_clone_dir user:password@host:/foo@bar foo@bar +test_clone_dir user:passw@rd@host:/f...@bar.git foo@bar + +# trailing port-like numbers should not be stripped for paths +test_clone_dir ssh://user:password@host/test:1234 1234 +test_clone_dir ssh://user:password@host/test:1234.git 1234 + +test_done -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/5] clone: do not include authentication data in guessed dir
If the URI contains authentication data and the URI's path component is empty we fail to guess a sensible directory name. E.g. cloning a repository 'ssh://user:passw...@example.com/' we guess a directory name 'passw...@example.com' where we would want the hostname only, e.g. 'example.com'. The naive way of just adding '@' as a path separator would break cloning repositories like 'foo/b...@baz.git' (which would currently become 'bar@baz' but would then become 'baz' only). Instead fix this by first dropping the scheme and then greedily scanning for an '@' sign until we find the first path separator. Signed-off-by: Patrick Steinhardt --- builtin/clone.c | 41 +++-- t/t5603-clone-dirname.sh | 4 ++-- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index bf45199..da51792 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -146,30 +146,51 @@ static char *get_repo_path(const char *repo, int *is_bundle) static char *guess_dir_name(const char *repo, int is_bundle, int is_bare) { - const char *end = repo + strlen(repo), *start; + const char *end = repo + strlen(repo), *start, *ptr; size_t len; char *dir; /* +* Skip scheme. +*/ + start = strstr(repo, "://"); + if (start == NULL) + start = repo; + else + start += 3; + + /* +* Skip authentication data. The stripping does happen +* greedily, such that we strip up to the last '@' inside +* the host part. +*/ + for (ptr = start; ptr < end && !is_dir_sep(*ptr); ptr++) { + if (*ptr == '@') + start = ptr + 1; + } + + /* * Strip trailing spaces, slashes and /.git */ - while (repo < end && (is_dir_sep(end[-1]) || isspace(end[-1]))) + while (start < end && (is_dir_sep(end[-1]) || isspace(end[-1]))) end--; - if (end - repo > 5 && is_dir_sep(end[-5]) && + if (end - start > 5 && is_dir_sep(end[-5]) && !strncmp(end - 4, ".git", 4)) { end -= 5; - while (repo < end && is_dir_sep(end[-1])) + while (start < end && is_dir_sep(end[-1])) end--; } /* -* Find last component, but be prepared that repo could have -* the form "remote.example.com:foo.git", i.e. no slash -* in the directory part. +* Find last component. To remain backwards compatible we +* also regard colons as path separators, such that +* cloning a repository 'foo:bar.git' would result in a +* directory 'bar' being guessed. */ - start = end; - while (repo < start && !is_dir_sep(start[-1]) && start[-1] != ':') - start--; + ptr = end; + while (start < ptr && !is_dir_sep(ptr[-1]) && ptr[-1] != ':') + ptr--; + start = ptr; /* * Strip .{bundle,git}. diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh index 10f5d09..a9aba72 100755 --- a/t/t5603-clone-dirname.sh +++ b/t/t5603-clone-dirname.sh @@ -74,11 +74,11 @@ test_clone_dir host:foo/.git/// foo # omitting the path should default to the hostname test_clone_dir ssh://host/ host test_clone_dir ssh://host:1234/ host fail -test_clone_dir ssh://user@host/ host fail +test_clone_dir ssh://user@host/ host test_clone_dir host:/ host fail # auth materials should be redacted -test_clone_dir ssh://user:password@host/ host fail +test_clone_dir ssh://user:password@host/ host test_clone_dir ssh://user:password@host:1234/ host fail test_clone_dir ssh://user:passw@rd@host:1234/ host fail test_clone_dir user@host:/ host fail -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 4/5] clone: do not use port number as dir name
If the URI contains a port number and the URI's path component is empty we fail to guess a sensible directory name. E.g. cloning a repository 'ssh://example.com:/' we guess a directory name '' where we would want the hostname only, e.g. 'example.com'. We need to take care to not drop trailing port-like numbers in certain cases. E.g. when cloning a repository 'foo/bar:.git' we want to guess the directory name '' instead of 'bar'. Thus, we have to first check the stripped URI for path separators and only strip port numbers if there are path separators present. This heuristic breaks when cloning a repository 'bar:.git', though. Signed-off-by: Patrick Steinhardt --- builtin/clone.c | 17 + t/t5603-clone-dirname.sh | 14 +++--- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index da51792..e7f16ff 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -182,6 +182,23 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare) } /* +* Strip trailing port number if we've got only a +* hostname (that is, there is no dir separator but a +* colon). This check is required such that we do not +* strip URI's like '/foo/bar:.git', which should +* result in a dir '' being guessed due to backwards +* compatibility. +*/ + if (memchr(start, '/', end - start) == NULL + && memchr(start, ':', end - start) != NULL) { + ptr = end; + while (start < ptr && isdigit(ptr[-1]) && ptr[-1] != ':') + ptr--; + if (start < ptr && ptr[-1] == ':') + end = ptr - 1; + } + + /* * Find last component. To remain backwards compatible we * also regard colons as path separators, such that * cloning a repository 'foo:bar.git' would result in a diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh index a9aba72..664d0ab 100755 --- a/t/t5603-clone-dirname.sh +++ b/t/t5603-clone-dirname.sh @@ -73,17 +73,17 @@ test_clone_dir host:foo/.git/// foo # omitting the path should default to the hostname test_clone_dir ssh://host/ host -test_clone_dir ssh://host:1234/ host fail +test_clone_dir ssh://host:1234/ host test_clone_dir ssh://user@host/ host -test_clone_dir host:/ host fail +test_clone_dir host:/ host # auth materials should be redacted test_clone_dir ssh://user:password@host/ host -test_clone_dir ssh://user:password@host:1234/ host fail -test_clone_dir ssh://user:passw@rd@host:1234/ host fail -test_clone_dir user@host:/ host fail -test_clone_dir user:password@host:/ host fail -test_clone_dir user:pass@wrd@host:/ host fail +test_clone_dir ssh://user:password@host:1234/ host +test_clone_dir ssh://user:passw@rd@host:1234/ host +test_clone_dir user@host:/ host +test_clone_dir user:password@host:/ host +test_clone_dir user:pass@wrd@host:/ host # auth-like material should not be dropped test_clone_dir ssh://host/foo@bar foo@bar -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/5] clone: use computed length in guess_dir_name
From: Jeff King Commit 7e837c6 (clone: simplify string handling in guess_dir_name(), 2015-07-09) changed clone to use strip_suffix instead of hand-rolled pointer manipulation. However, strip_suffix will strip from the end of a NUL-terminated string, and we may have already stripped some characters (like directory separators, or "/.git"). This leads to commands like: git clone host:foo.git/ failing to strip the ".git". We must instead convert our pointer arithmetic into a computed length and feed that to strip_suffix_mem, which will then reduce the length further for us. It would be nicer if we could drop the pointer manipulation entirely, and just continually strip using strip_suffix. But that doesn't quite work for two reasons: 1. The early suffixes we're stripping are not constant; we need to look for is_dir_sep, which could be one of several characters. 2. Mid-way through the stripping we compute the pointer "start", which shows us the beginning of the pathname. Which really give us two lengths to work with: the offset from the start of the string, and from the start of the path. By using pointers for the early part, we can just compute the length from "start" when we need it. Signed-off-by: Jeff King --- builtin/clone.c | 3 ++- t/t5603-clone-dirname.sh | 12 ++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 303a3a7..bf45199 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -174,7 +174,8 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare) /* * Strip .{bundle,git}. */ - strip_suffix(start, is_bundle ? ".bundle" : ".git" , &len); + len = end - start; + strip_suffix_mem(start, &len, is_bundle ? ".bundle" : ".git"); if (is_bare) dir = xstrfmt("%.*s.git", (int)len, start); diff --git a/t/t5603-clone-dirname.sh b/t/t5603-clone-dirname.sh index b009a87..10f5d09 100755 --- a/t/t5603-clone-dirname.sh +++ b/t/t5603-clone-dirname.sh @@ -46,7 +46,7 @@ test_clone_dir host:foo foo.git bare test_clone_dir host:foo.git foo test_clone_dir host:foo.git foo.git bare test_clone_dir host:foo/.git foo -test_clone_dir host:foo/.git foo.git bare fail +test_clone_dir host:foo/.git foo.git bare # similar, but using ssh URL rather than host:path syntax test_clone_dir ssh://host/foo foo @@ -54,20 +54,20 @@ test_clone_dir ssh://host/foo foo.git bare test_clone_dir ssh://host/foo.git foo test_clone_dir ssh://host/foo.git foo.git bare test_clone_dir ssh://host/foo/.git foo -test_clone_dir ssh://host/foo/.git foo.git bare fail +test_clone_dir ssh://host/foo/.git foo.git bare # we should remove trailing slashes and .git suffixes test_clone_dir ssh://host/foo/ foo test_clone_dir ssh://host/foo/// foo -test_clone_dir ssh://host/foo.git/ foo fail -test_clone_dir ssh://host/foo.git/// foo fail +test_clone_dir ssh://host/foo.git/ foo +test_clone_dir ssh://host/foo.git/// foo test_clone_dir ssh://host/foo///.git/ foo test_clone_dir ssh://host/foo/.git/// foo test_clone_dir host:foo/ foo test_clone_dir host:foo/// foo -test_clone_dir host:foo.git/ foo fail -test_clone_dir host:foo.git/// foo fail +test_clone_dir host:foo.git/ foo +test_clone_dir host:foo.git/// foo test_clone_dir host:foo///.git/ foo test_clone_dir host:foo/.git/// foo -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/5] Improve guessing of repository names
This is version 5 of my patch series which aims to improve guessed directory names in several cases. This version now includes the patches from Jeff which were previously a separate patch series. Besides fixing behavior when cloning a repository with trailing '.git' suffix they also add a new test suite that verifies guessed directory names. I've amended 'clone: add tests for output directory' to add additional tests (as proposed by Junio). Changes to my own patches include improved commit messages detailing why certain changes are done the way they are done and a change to authentication-data-stripping, such that we now strip greedily until the last '@' sign in the host part (proposed by Junio, as well). Jeff King (2): clone: add tests for output directory clone: use computed length in guess_dir_name Patrick Steinhardt (3): clone: do not include authentication data in guessed dir clone: do not use port number as dir name clone: abort if no dir name could be guessed builtin/clone.c | 65 +- t/t5603-clone-dirname.sh | 103 +++ 2 files changed, 157 insertions(+), 11 deletions(-) create mode 100755 t/t5603-clone-dirname.sh -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: File Hash for Windows executable
Hi Ryan, On 2015-08-10 15:54, Kiser, Ryan Lee wrote: > I've downloaded the Windows installer for Git, Which one. > but can't seem to find hashes published anywhere for verification. Since the > downloaded file doesn't seem to be signed, The newest ones from https://git-for-windows.github.io/ are signed. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager
Hi Peff, On 2015-08-10 07:23, Jeff King wrote: > diff --git a/compat/pipe-id.c b/compat/pipe-id.c > new file mode 100644 > index 000..4764c5f > --- /dev/null > +++ b/compat/pipe-id.c > @@ -0,0 +1,25 @@ > +#include "git-compat-util.h" > +#include "compat/pipe-id.h" > +#include "strbuf.h" > + > +const char *pipe_id_get(int fd) > +{ > + static struct strbuf id = STRBUF_INIT; > + struct stat st; > + > + if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode)) > + return NULL; Just a quick note: it seems that this check is not really working on Windows. I tested this by running this test case manually (because TTY is not set on Windows): > +test_expect_success TTY 'no color when paged program writes to pipe' ' > + test_config alias.externallog "!git log | cat >log.out" && > + test_config color.ui auto && > + test_terminal env TERM=vt100 git -p externallog && > + test_line_count = 0 paginated.out && > + test -s log.out && > + ! colorful log.out > +' The output is "colorful" ;-) I hope to find some time tomorrow to figure out some workaround that makes this work on Windows. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule: implement `module_name` as a builtin helper
On Fri, Aug 7, 2015 at 11:20 PM, Junio C Hamano wrote: > Junio C Hamano writes: > ... if you really want to go the "thread" route, the first thing to try would be to see if a few places we already use threads for parallelism (namely, "grep", "pack-objects", "preload-index" and "index-pack") can be factored out and model your new API around the commonality among them. >> >> And obviously, doing your pool API around threads will allow you to >> throw future per-thread function that do not involve run_command() >> at all at your API, and it will make it easy to adapt the current >> threaded parts of the system to the API. > > Just a few random thoughts before going to bed and going offline for > the weekend... > > Eventually, we would want to do "submodule update" of a top-level > project that has 500 submodules underneath, but obviously we would > not want to blindly spawn 500 threads, each of which runs "fetch", > all at the same time. We'd want to limit the parallelism to a sane > limit (say, 16 or 32), stuff 500 work units to a queue, from which > that many number of worker bees grab work unit one by one to process > and then come back to ask for more work. This is exactly what is currently implemented and works in patch "[RFC PATCH 2/4] Add a workdispatcher to get work done in parallel" > > And we would eventually want to be able to do this even when these > 500 submodules are spread across multiple levels of nested > submodules (e.g. top-level may have 8 submodules, and they have 16 > nested subsubmodules each on average, each of which may have 4 > nested subsubsubmodules on average). Specifying -j16 at the top > level and apportioning the parallelism to recursive invoation of > "submodule update" in such a way that the overall process is > efficient and without waste would be a bit tricky. > > In such a nested submodule case, we may want to instead try to > enumerate these 500 submodules upfront with unbounded parallelism The problem here is we need to have finished cloning at least one top level submodule before we can add any further sub-submodules into the work queue. So if there are only 4 top level submodules we'd have a slow start. If we have only one top level work queue, the deeper nesting levels will not explode unbound, but eventually we will reach the 16 threads and keep working with them, and once git submodule is ported to C we don't need to have process invocations, but can rely on the just the threading done right. And then we it should be rather easy to only use one top level task queue. > (e.g. the top-level will ask 4 worker bees to process immediate 8 > submodules, and they each spawn 4 worker bees to process their > immediate 16 submodules, and so on---it is unbounded because we do > not know upfront how deep the nesting is). > > Let's call that a recursive module_list. You would want out of a > recursive module_list: > > - the path to the submodule (or "." for the top-level) to indicate >where in the nested hierarchy the information came from; > > - the information the flat module_list gives for that location. I only see value in this recursive module_list approach for updating the work tree (fetch --recurse-submodules=yes, instead of cloning) as you already have most of the submodules there (there may be new submodules after fetching). Also collecting 500 submodule information is in the order of reading 500 files. But then we need to do 500 fetches. And doing a fetch takes some orders of magnitude longer than reading a file, so I am not convinced a parallel collection of work to be done is a good first step? > > Since you already have module_list() function natively callable from > C and also it is available via "git submodule--helper module_list", > implementing a recursive module_list would be a good first proof of > concept exercise for your "thread pool" engine. You can employ the > "dual implementation" trick to call > > - a version that tells the thread to run the native C version of >module_list(), > > - another version that tells the thread to run_command() >"submodule--helper module_list" in the top-level and nested >submodules. > > and collect and compare their results and performance. > > That will not just be a good proof of concept for the pool > implementation. > > Once you have such a recursive module_list, you can use it as a way > to easily obtain such a "unified view" list of all submodules. That > can be used to stuff a flat work unit queue to implement reasonably > bounded parallelism. > > Your recursive "submoule update" implementation could be > > - Run recursive module_list to stuff the work queue with these 500 >submodules (possibly spread across in top-level and in nested >submodules, or all 500 in the flat top-level); > > - Start N worker bees, and tell them to pick from that work queue, >each element of which tells them to process which submodule that >resides in where (ei
RE: File Hash for Windows executable
Ah, perfect. Thank you. I was looking at 1.9.5 from http://www.git-scm.com/download Thanks for pointing me at the right place. -Ryan -Original Message- From: Johannes Schindelin [mailto:johannes.schinde...@gmx.de] Sent: Monday, August 10, 2015 12:10 PM To: Kiser, Ryan Lee Cc: git@vger.kernel.org Subject: Re: File Hash for Windows executable Hi Ryan, On 2015-08-10 15:54, Kiser, Ryan Lee wrote: > I've downloaded the Windows installer for Git, Which one. > but can't seem to find hashes published anywhere for verification. Since the > downloaded file doesn't seem to be signed, The newest ones from https://git-for-windows.github.io/ are signed. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names
SZEDER Gábor writes: > 'git config' can only show values or name-value pairs, so if a shell > script needs the names of set config variables it has to run 'git config > --list' or '--get-regexp' and parse the output to separate config > variable names from their values. However, such a parsing can't cope > with multi-line values. Though 'git config' can produce null-terminated > output for newline-safe parsing, that's of no use in such a case, becase s/becase/because/; > shells can't cope with null characters. > > Even our own bash completion script suffers from these issues. > > Help the completion script, and shell scripts in general, by introducing > the '--names-only' option to modify the output of '--list' and > '--get-regexp' to list only the names of config variables, so they don't > have to perform error-prone post processing to separate variable names > from their values anymore. I agree with Peff that "--names-only" has a subtle difference with an existing and well known subcommand option and it would be a bit irritating to remember which options is for which command. > diff --git a/builtin/config.c b/builtin/config.c > index 7188405f7e..307980ab50 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -13,6 +13,7 @@ static char *key; > static regex_t *key_regexp; > static regex_t *regexp; > static int show_keys; > +static int omit_values; > static int use_key_regexp; > static int do_all; > static int do_not_match; > ... > @@ -91,7 +93,7 @@ static void check_argc(int argc, int min, int max) { > > static int show_all_config(const char *key_, const char *value_, void *cb) > { > - if (value_) > + if (!omit_values && value_) H. As we have "show_keys", if (show_values && value_) would be a lot more intuitive, no? > @@ -117,6 +119,10 @@ static int format_config(struct strbuf *buf, const char > *key_, const char *value > strbuf_addstr(buf, key_); > must_print_delim = 1; > } > + if (omit_values) { > + strbuf_addch(buf, term); > + return 0; > + } This hunk makes me wonder what the assignment to "must_print_delim" is about. When the code is told to show only keys and not values, it shouldn't even have to worry about key_delim, but that assignment is done to control exactly that. It happens that you are lucky that you can "return 0" early here so that the assignment does not have any effect, but still conceptually the code structure is made ugly by this patch. Isn't it more like the existing "show_keys" can be replaced/enhanced with a single "show" tri-state toggle that chooses one among: * show both keys and values (for --list) * show only keys (for your new feature) * show only value (for --get) perhaps? I see get_urlmatch() abuses show_keys variable in a strange way, and it may not be as trivial as removing show_keys and replacing it with a new tri-state toggle, though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager
On Mon, Aug 10, 2015 at 06:38:10PM +0200, Johannes Schindelin wrote: > > +const char *pipe_id_get(int fd) > > +{ > > + static struct strbuf id = STRBUF_INIT; > > + struct stat st; > > + > > + if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode)) > > + return NULL; > > Just a quick note: it seems that this check is not really working on > Windows. I tested this by running this test case manually (because TTY > is not set on Windows): Yeah, I'm not too surprised. I'm guessing your st_ino for pipes are all just the same or something. Or maybe S_ISFIFO doesn't pass (we don't technically need it, I don't think, and could just drop that check). Anyway, I had planned that we would need to stick a big "#ifdef WINDOWS" around these two functions. > I hope to find some time tomorrow to figure out some workaround that > makes this work on Windows. Cool. I can't comment on Windows-specific stuff, but I'm happy to review the rest of it. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/17] removing questionable uses of git_path
Jeff King writes: > The problem is that git_path uses a static buffer that gets overwritten > by subsequent calls. As the rotating static buffer pattern used in get_pathname() was modeled after sha1_to_hex(), we have the same issue there. They are troubles waiting to happen unless the callers are careful. > producing a fairly tame-looking set of function calls. It's OK to pass > the result of git_path() to a system call, or something that is a thin > wrapper around one (e.g., strbuf_read_file). That is a short and good rule to follow, but the problem is that not everybody has good taste to interpret the above rule and apply it with an eye toward maintainability X-<. > Along the way, there are a few cleanups (e.g., I polished off the recent > hold_lock_file_for_append topic which was on the list, as it had some > problematic calls). > > [01/17]: cache.h: clarify documentation for git_path, et al > [02/17]: cache.h: complete set of git_path_submodule helpers > [03/17]: t5700: modernize style > [04/17]: add_to_alternates_file: don't add duplicate entries > [05/17]: remove hold_lock_file_for_append > [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases > [07/17]: prefer mkpathdup to mkpath in assignments > [08/17]: remote.c: drop extraneous local variable from migrate_file > [09/17]: refs.c: remove extra git_path calls from read_loose refs > [10/17]: path.c: drop git_path_submodule > [11/17]: refs.c: simplify strbufs in reflog setup and writing > [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log > [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic > [14/17]: refs.c: remove_empty_directories can take a strbuf > [15/17]: find_hook: keep our own static buffer > [16/17]: get_repo_path: refactor path-allocation > [17/17]: memoize common git-path "constant" files Nice. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file.c: rename move_temp_to_file() to finalize_temp_file()
Jeff King writes: > On Fri, Aug 07, 2015 at 05:24:29PM -0700, Junio C Hamano wrote: > >> Since 5a688fe4 ("core.sharedrepository = 0mode" should set, not >> loosen, 2009-03-25), we kept reminding ourselves: >> >> NEEDSWORK: this should be renamed to finalize_temp_file() as >> "moving" is only a part of what it does, when no patch between >> master to pu changes the call sites of this function. >> >> without doing anything about it. Let's do so. >> >> The purpose of this function was not to move but to finalize. The >> detail of the primarily implementation of finalizing was to link the >> temporary file to its final name and then to unlink, which wasn't >> even "moving". The alternative implementation did "move" by calling >> rename(2), which is a fun tangent. > > This is definitely a better name. But while we are touching the area, my > other pet peeve about this function is that it is really specific to > moving _object_ temp files around. It started life as static-local to > sha1-file.c, so not mentioning objects is OK, but when it became a > global, it's a bit confusing. > > Maybe finalize_object_file() would be a better name? > > -Peff OK, will fix. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6] notes: handle multiple worktrees
Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using find_shared_symref and die if we find one. This prevents simultaneous merges to the same notes branch from different worktrees. Signed-off-by: David Turner --- This reroll addresses Eric Sunshine's comments on v5. --- builtin/notes.c | 6 t/t3320-notes-merge-worktrees.sh | 72 2 files changed, 78 insertions(+) create mode 100755 t/t3320-notes-merge-worktrees.sh diff --git a/builtin/notes.c b/builtin/notes.c index 63f95fc..0423480 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -19,6 +19,7 @@ #include "string-list.h" #include "notes-merge.h" #include "notes-utils.h" +#include "branch.h" static const char * const git_notes_usage[] = { N_("git notes [--ref ] [list []]"), @@ -825,10 +826,15 @@ static int merge(int argc, const char **argv, const char *prefix) update_ref(msg.buf, default_notes_ref(), result_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); else { /* Merge has unresolved conflicts */ + char *existing; /* Update .git/NOTES_MERGE_PARTIAL with partial merge result */ update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); /* Store ref-to-be-updated into .git/NOTES_MERGE_REF */ + existing = find_shared_symref("NOTES_MERGE_REF", default_notes_ref()); + if (existing) + die(_("A notes merge into %s is already in-progress at %s"), + default_notes_ref(), existing); if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL)) die("Failed to store link to current notes ref (%s)", default_notes_ref()); diff --git a/t/t3320-notes-merge-worktrees.sh b/t/t3320-notes-merge-worktrees.sh new file mode 100755 index 000..a7beef2 --- /dev/null +++ b/t/t3320-notes-merge-worktrees.sh @@ -0,0 +1,72 @@ +#!/bin/sh +# +# Copyright (c) 2015 Twitter, Inc +# + +test_description='Test merging of notes trees in multiple worktrees' + +. ./test-lib.sh + +test_expect_success 'setup commit' ' + test_commit tantrum +' + +commit_tantrum=$(git rev-parse tantrum^{commit}) + +test_expect_success 'setup notes ref (x)' ' + git config core.notesRef refs/notes/x && + git notes add -m "x notes on tantrum" tantrum +' + +test_expect_success 'setup local branch (y)' ' + git update-ref refs/notes/y refs/notes/x && + git config core.notesRef refs/notes/y && + git notes remove tantrum +' + +test_expect_success 'setup remote branch (z)' ' + git update-ref refs/notes/z refs/notes/x && + git config core.notesRef refs/notes/z && + git notes add -f -m "conflicting notes on tantrum" tantrum +' + +test_expect_success 'modify notes ref ourselves (x)' ' + git config core.notesRef refs/notes/x && + git notes add -f -m "more conflicting notes on tantrum" tantrum +' + +test_expect_success 'create some new worktrees' ' + git worktree add -b newbranch worktree master && + git worktree add -b newbranch2 worktree2 master +' + +test_expect_success 'merge z into y fails and sets NOTES_MERGE_REF' ' + git config core.notesRef refs/notes/y && + test_must_fail git notes merge z && + echo "ref: refs/notes/y" > expect && + test_cmp .git/NOTES_MERGE_REF expect +' + +test_expect_success 'merge z into y while mid-merge in another workdir fails' ' + ( + cd worktree && + git config core.notesRef refs/notes/y && + test_must_fail git notes merge z 2>err && + grep "A notes merge into refs/notes/y is already in-progress at" err + ) && + test_path_is_missing .git/worktrees/worktree/NOTES_MERGE_REF +' + +test_expect_success 'merge z into x while mid-merge on y succeeds' ' + ( + cd worktree2 && + git config core.notesRef refs/notes/x && + test_must_fail git notes merge z 2>&1 >out && + grep "Automatic notes merge failed" out && + grep -v "A notes merge into refs/notes/x is already in-progress in" out + ) && + echo "ref: refs/notes/x" > expect && + test_cmp .git/worktrees/worktree2/NOTES_MERGE_REF expect +' + +test_done -- 2.0.4.315.gad8727a-twtrsrc -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/17] removing questionable uses of git_path
On Mon, Aug 10, 2015 at 10:31:32AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > The problem is that git_path uses a static buffer that gets overwritten > > by subsequent calls. > > As the rotating static buffer pattern used in get_pathname() was > modeled after sha1_to_hex(), we have the same issue there. They are > troubles waiting to happen unless the callers are careful. Yeah, I think Michael mentioned that to me off-list. I don't _think_ sha1_to_hex is nearly such a problem, because we tend to store sha1s in their binary form. So sha1_to_hex is almost always an argument to fprintf() or similar. Of course there are some exceptions. :) I notice we pass one return value to add_pending_object, which was almost certainly horribly broken before 31faeb2 started strdup'ing object_array names. So certainly it would be nice to audit them all, but there are over 600 calls. Given the likelihood of finding a useful bug, I'm not sure it's the greatest use of developer time. > > producing a fairly tame-looking set of function calls. It's OK to pass > > the result of git_path() to a system call, or something that is a thin > > wrapper around one (e.g., strbuf_read_file). > > That is a short and good rule to follow, but the problem is that not > everybody has good taste to interpret the above rule and apply it with > an eye toward maintainability X-<. Yeah. Part of me wants to eradicate git_path entirely. My series takes out over half of the calls, but there are still close to 100, I think. I think it would make the code worse to convert all of them naively. We could provide format-aware wrappers for some filesystem functions, like: git_stat(&st, "%s", refname); or something. That feels horribly coupled compared to: stat(git_path("%s", refname), &st); but it makes it very clear what the memory ownership/lifetime rules are. Anyway, part of my goal with the series was not just to clean up the existing suspicious spots, but to raise awareness of the issue. At least for me, I know it's something I will look for more carefully when reviewing patches. Once bitten, twice shy. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] notes: handle multiple worktrees
Sorry, that should have included the first patch as well. Will re-send as .v7 On Mon, 2015-08-10 at 13:43 -0400, David Turner wrote: > Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using > find_shared_symref and die if we find one. This prevents simultaneous > merges to the same notes branch from different worktrees. > > Signed-off-by: David Turner > --- > > This reroll addresses Eric Sunshine's comments on v5. > > --- > builtin/notes.c | 6 > t/t3320-notes-merge-worktrees.sh | 72 > > 2 files changed, 78 insertions(+) > create mode 100755 t/t3320-notes-merge-worktrees.sh > > diff --git a/builtin/notes.c b/builtin/notes.c > index 63f95fc..0423480 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -19,6 +19,7 @@ > #include "string-list.h" > #include "notes-merge.h" > #include "notes-utils.h" > +#include "branch.h" > > static const char * const git_notes_usage[] = { > N_("git notes [--ref ] [list []]"), > @@ -825,10 +826,15 @@ static int merge(int argc, const char **argv, const > char *prefix) > update_ref(msg.buf, default_notes_ref(), result_sha1, NULL, > 0, UPDATE_REFS_DIE_ON_ERR); > else { /* Merge has unresolved conflicts */ > + char *existing; > /* Update .git/NOTES_MERGE_PARTIAL with partial merge result */ > update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL, > 0, UPDATE_REFS_DIE_ON_ERR); > /* Store ref-to-be-updated into .git/NOTES_MERGE_REF */ > + existing = find_shared_symref("NOTES_MERGE_REF", > default_notes_ref()); > + if (existing) > + die(_("A notes merge into %s is already in-progress at > %s"), > + default_notes_ref(), existing); > if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL)) > die("Failed to store link to current notes ref (%s)", > default_notes_ref()); > diff --git a/t/t3320-notes-merge-worktrees.sh > b/t/t3320-notes-merge-worktrees.sh > new file mode 100755 > index 000..a7beef2 > --- /dev/null > +++ b/t/t3320-notes-merge-worktrees.sh > @@ -0,0 +1,72 @@ > +#!/bin/sh > +# > +# Copyright (c) 2015 Twitter, Inc > +# > + > +test_description='Test merging of notes trees in multiple worktrees' > + > +. ./test-lib.sh > + > +test_expect_success 'setup commit' ' > + test_commit tantrum > +' > + > +commit_tantrum=$(git rev-parse tantrum^{commit}) > + > +test_expect_success 'setup notes ref (x)' ' > + git config core.notesRef refs/notes/x && > + git notes add -m "x notes on tantrum" tantrum > +' > + > +test_expect_success 'setup local branch (y)' ' > + git update-ref refs/notes/y refs/notes/x && > + git config core.notesRef refs/notes/y && > + git notes remove tantrum > +' > + > +test_expect_success 'setup remote branch (z)' ' > + git update-ref refs/notes/z refs/notes/x && > + git config core.notesRef refs/notes/z && > + git notes add -f -m "conflicting notes on tantrum" tantrum > +' > + > +test_expect_success 'modify notes ref ourselves (x)' ' > + git config core.notesRef refs/notes/x && > + git notes add -f -m "more conflicting notes on tantrum" tantrum > +' > + > +test_expect_success 'create some new worktrees' ' > + git worktree add -b newbranch worktree master && > + git worktree add -b newbranch2 worktree2 master > +' > + > +test_expect_success 'merge z into y fails and sets NOTES_MERGE_REF' ' > + git config core.notesRef refs/notes/y && > + test_must_fail git notes merge z && > + echo "ref: refs/notes/y" > expect && > + test_cmp .git/NOTES_MERGE_REF expect > +' > + > +test_expect_success 'merge z into y while mid-merge in another workdir > fails' ' > + ( > + cd worktree && > + git config core.notesRef refs/notes/y && > + test_must_fail git notes merge z 2>err && > + grep "A notes merge into refs/notes/y is already in-progress > at" err > + ) && > + test_path_is_missing .git/worktrees/worktree/NOTES_MERGE_REF > +' > + > +test_expect_success 'merge z into x while mid-merge on y succeeds' ' > + ( > + cd worktree2 && > + git config core.notesRef refs/notes/x && > + test_must_fail git notes merge z 2>&1 >out && > + grep "Automatic notes merge failed" out && > + grep -v "A notes merge into refs/notes/x is already in-progress > in" out > + ) && > + echo "ref: refs/notes/x" > expect && > + test_cmp .git/worktrees/worktree2/NOTES_MERGE_REF expect > +' > + > +test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/2] worktrees: add find_shared_symref
Add a new function, find_shared_symref, which contains the heart of die_if_checked_out, but works for any symref, not just HEAD. Refactor die_if_checked_out to use the same infrastructure as find_shared_symref. Soon, we will use find_shared_symref to protect notes merges in worktrees. Signed-off-by: David Turner --- Please disregard v6. This version addresses Eric Sunshine's comments on v5. It fixes an error message and cleans up the code. --- branch.c | 46 ++ branch.h | 8 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/branch.c b/branch.c index c85be07..07c8b1e 100644 --- a/branch.c +++ b/branch.c @@ -311,21 +311,23 @@ void remove_branch_state(void) unlink(git_path("SQUASH_MSG")); } -static void check_linked_checkout(const char *branch, const char *id) +static char *find_linked_symref(const char *symref, const char *branch, + const char *id) { struct strbuf sb = STRBUF_INIT; struct strbuf path = STRBUF_INIT; struct strbuf gitdir = STRBUF_INIT; + char *existing = NULL; /* -* $GIT_COMMON_DIR/HEAD is practically outside -* $GIT_DIR so resolve_ref_unsafe() won't work (it -* uses git_path). Parse the ref ourselves. +* $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside +* $GIT_DIR so resolve_ref_unsafe() won't work (it uses +* git_path). Parse the ref ourselves. */ if (id) - strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id); + strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref); else - strbuf_addf(&path, "%s/HEAD", get_git_common_dir()); + strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref); if (!strbuf_readlink(&sb, path.buf, 0)) { if (!starts_with(sb.buf, "refs/") || @@ -347,33 +349,53 @@ static void check_linked_checkout(const char *branch, const char *id) strbuf_rtrim(&gitdir); } else strbuf_addstr(&gitdir, get_git_common_dir()); - skip_prefix(branch, "refs/heads/", &branch); strbuf_strip_suffix(&gitdir, ".git"); - die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf); + + existing = strbuf_detach(&gitdir, NULL); done: strbuf_release(&path); strbuf_release(&sb); strbuf_release(&gitdir); + + return existing; } -void die_if_checked_out(const char *branch) +char *find_shared_symref(const char *symref, const char *target) { struct strbuf path = STRBUF_INIT; DIR *dir; struct dirent *d; + char *existing; - check_linked_checkout(branch, NULL); + if ((existing = find_linked_symref(symref, target, NULL))) + return existing; strbuf_addf(&path, "%s/worktrees", get_git_common_dir()); dir = opendir(path.buf); strbuf_release(&path); if (!dir) - return; + return NULL; while ((d = readdir(dir)) != NULL) { if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, "..")) continue; - check_linked_checkout(branch, d->d_name); + existing = find_linked_symref(symref, target, d->d_name); + if (existing) + goto done; } +done: closedir(dir); + + return existing; +} + +void die_if_checked_out(const char *branch) +{ + char *existing; + + existing = find_shared_symref("HEAD", branch); + if (existing) { + skip_prefix(branch, "refs/heads/", &branch); + die(_("'%s' is already checked out at '%s'"), branch, existing); + } } diff --git a/branch.h b/branch.h index 58aa45f..d3446ed 100644 --- a/branch.h +++ b/branch.h @@ -59,4 +59,12 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name); */ extern void die_if_checked_out(const char *branch); +/* + * Check if a per-worktree symref points to a ref in the main worktree + * or any linked worktree, and return the path to the exising worktree + * if it is. Returns NULL if there is no existing ref. The caller is + * responsible for freeing the returned path. + */ +extern char *find_shared_symref(const char *symref, const char *target); + #endif -- 2.0.4.315.gad8727a-twtrsrc -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/2] notes: handle multiple worktrees
Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using find_shared_symref and die if we find one. This prevents simultaneous merges to the same notes branch from different worktrees. Signed-off-by: David Turner --- builtin/notes.c | 6 t/t3320-notes-merge-worktrees.sh | 72 2 files changed, 78 insertions(+) create mode 100755 t/t3320-notes-merge-worktrees.sh diff --git a/builtin/notes.c b/builtin/notes.c index 63f95fc..0423480 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -19,6 +19,7 @@ #include "string-list.h" #include "notes-merge.h" #include "notes-utils.h" +#include "branch.h" static const char * const git_notes_usage[] = { N_("git notes [--ref ] [list []]"), @@ -825,10 +826,15 @@ static int merge(int argc, const char **argv, const char *prefix) update_ref(msg.buf, default_notes_ref(), result_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); else { /* Merge has unresolved conflicts */ + char *existing; /* Update .git/NOTES_MERGE_PARTIAL with partial merge result */ update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL, 0, UPDATE_REFS_DIE_ON_ERR); /* Store ref-to-be-updated into .git/NOTES_MERGE_REF */ + existing = find_shared_symref("NOTES_MERGE_REF", default_notes_ref()); + if (existing) + die(_("A notes merge into %s is already in-progress at %s"), + default_notes_ref(), existing); if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL)) die("Failed to store link to current notes ref (%s)", default_notes_ref()); diff --git a/t/t3320-notes-merge-worktrees.sh b/t/t3320-notes-merge-worktrees.sh new file mode 100755 index 000..a7beef2 --- /dev/null +++ b/t/t3320-notes-merge-worktrees.sh @@ -0,0 +1,72 @@ +#!/bin/sh +# +# Copyright (c) 2015 Twitter, Inc +# + +test_description='Test merging of notes trees in multiple worktrees' + +. ./test-lib.sh + +test_expect_success 'setup commit' ' + test_commit tantrum +' + +commit_tantrum=$(git rev-parse tantrum^{commit}) + +test_expect_success 'setup notes ref (x)' ' + git config core.notesRef refs/notes/x && + git notes add -m "x notes on tantrum" tantrum +' + +test_expect_success 'setup local branch (y)' ' + git update-ref refs/notes/y refs/notes/x && + git config core.notesRef refs/notes/y && + git notes remove tantrum +' + +test_expect_success 'setup remote branch (z)' ' + git update-ref refs/notes/z refs/notes/x && + git config core.notesRef refs/notes/z && + git notes add -f -m "conflicting notes on tantrum" tantrum +' + +test_expect_success 'modify notes ref ourselves (x)' ' + git config core.notesRef refs/notes/x && + git notes add -f -m "more conflicting notes on tantrum" tantrum +' + +test_expect_success 'create some new worktrees' ' + git worktree add -b newbranch worktree master && + git worktree add -b newbranch2 worktree2 master +' + +test_expect_success 'merge z into y fails and sets NOTES_MERGE_REF' ' + git config core.notesRef refs/notes/y && + test_must_fail git notes merge z && + echo "ref: refs/notes/y" > expect && + test_cmp .git/NOTES_MERGE_REF expect +' + +test_expect_success 'merge z into y while mid-merge in another workdir fails' ' + ( + cd worktree && + git config core.notesRef refs/notes/y && + test_must_fail git notes merge z 2>err && + grep "A notes merge into refs/notes/y is already in-progress at" err + ) && + test_path_is_missing .git/worktrees/worktree/NOTES_MERGE_REF +' + +test_expect_success 'merge z into x while mid-merge on y succeeds' ' + ( + cd worktree2 && + git config core.notesRef refs/notes/x && + test_must_fail git notes merge z 2>&1 >out && + grep "Automatic notes merge failed" out && + grep -v "A notes merge into refs/notes/x is already in-progress in" out + ) && + echo "ref: refs/notes/x" > expect && + test_cmp .git/worktrees/worktree2/NOTES_MERGE_REF expect +' + +test_done -- 2.0.4.315.gad8727a-twtrsrc -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/5] Improve guessing of repository names
Patrick Steinhardt writes: > This is version 5 of my patch series which aims to improve > guessed directory names in several cases. > > This version now includes the patches from Jeff which were > previously a separate patch series. Besides fixing behavior when > cloning a repository with trailing '.git' suffix they also add a > new test suite that verifies guessed directory names. I've > amended 'clone: add tests for output directory' to add additional > tests (as proposed by Junio). > > Changes to my own patches include improved commit messages > detailing why certain changes are done the way they are done and > a change to authentication-data-stripping, such that we now strip > greedily until the last '@' sign in the host part (proposed by > Junio, as well). > > Jeff King (2): > clone: add tests for output directory > clone: use computed length in guess_dir_name > > Patrick Steinhardt (3): > clone: do not include authentication data in guessed dir > clone: do not use port number as dir name > clone: abort if no dir name could be guessed > > builtin/clone.c | 65 +- > t/t5603-clone-dirname.sh | 103 > +++ > 2 files changed, 157 insertions(+), 11 deletions(-) > create mode 100755 t/t5603-clone-dirname.sh Looks good. I'll forge your sign-off for two patches from Peff and queue the whole thing. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Error when cloning with weird local directory
Torsten Bögershausen writes: > So I think that git clone can be slighty more consistant here. Sure. >> I _think_ taking notice of "word://" (with doubled slashes) and >> treating it specially will not introduce any new issue; while it is >> still OK for users to have a local directory called "word:", if they >> meant a subdirectory of it, they wouldn't have typed double-slashes >> there. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: resolving a (possibly remote) branch HEAD to a hash
per...@pluto.rain.com (Perry Hutchison) writes: >> ... we do not say "append 'refs/remotes//' for various >> values of and see if such a ref exists" when resolving >> an abbreviated refname 'master'. > > checkout appears to. You are referring to this part of the documentation: If is not found but there does exist a tracking branch in exactly one remote (call it ) with a matching name, treat as equivalent to $ git checkout -b --track / A reader needs to read this part of the documentation a bit more carefully in order to notice that it never says it is equivalent to: $ git checkout -b -t ;# NOT CORRECT This behaviour was brought in by somebody who thought that, in the context of "checkout" (and only in that context), it is clear that missing that can only mean the sole / and make that signal something more than what the user told "checkout" to do: "If you want to check out a , and it does not exist yet, you must wanted to create your own and start it at the same commit as somebody else has at the tip of his ". This "clever" dwim is very specific to the way you interact with "checkout" and generally does not apply when you want to run anything other than "checkout", e.g. "rev-parse" or "log". But it is _so_ convenient a short-cut, that it lets new people form into an illusion that could be naming /. That is an incorrect perception. The rationale behind "signal something more" above goes like this: if the user wanted to detach the head at the same commit as somebody else's , she would explicitly have written $ git checkout / to do so. Because / is the shortest valid way to name that remote-tracking branch (i.e. exactly because is not a valid abbreviation for /), we can treat $ git checkout when is not a local branch name specially. It is sad and ironic that this checkout-specific DWIM works only because does not mean /, but presence of the DWIM gives a wrong illusion that it does X-<. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git blame breaking on repository with CRLF files
Torsten Bögershausen writes: > Actually I could reproduce the following: > CRLF in repo, CRLF in working tree, core.autocrlf= true. What should happen in such a case? Wouldn't autocrlf=true want to strip CRLF down to LF? Shouldn't it? And if so, "blame" is correct to say that you are changing the line endings of all your lines, as what you _would_ commit if you were to commit the tracked files in your working tree would be different from what is in the index, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation
Adam Dinwoodie writes: > If the desired goal is that Cygwin's link(2) acts like POSIX link(2) > on network drives, I'm not convinced that's possible, at least not by > emulating `core.createObject = rename` in the Cygwin library > layer. The way core.createObject=rename makes things work is by avoiding link(2) in the first place and using rename(2) instead. You might be able to emulate rename(2) of A to B by doing a link(2) of A to B and then unlink(2) of A, but I do not think it is reasonable for the system call emulation layer to detect a sequence of link followed by unlink and use rename, i.e. emulationg the other way around. So I suspect "fix in Cygwin" is a non-starter. But in the end, I'd prefer the choice of the compiled-in default up to the port maintainers. You earlier said: This problem was reported on the Cygwin mailing list at https://cygwin.com/ml/cygwin/2015-08/msg00102.html (amongst others) and is being applied as a manual patch to the Cygwin builds until the patch is taken here. so my preference is to see Cygwin continue to do so for a couple release cycles of ours to make sure all Cygwin end-users are happy and consider the flip of the default a good change for them, and then the official Cygwin packager of Git sends a patch our way. https://cygwin.com/ml/cygwin/2015-08/msg00102.html seems to indicate that somebody called Adam Dinwoodie is wearing Git maintainer hat, so perhaps you may be that "official Cygwin packager of Git" ;-) I agree with everything you said in that message to Peter---the patch should be included when you hear reports of `git config core.createObject rename` helping more people. After versions of Cygwin Git package with such a change proves good, let's reduce the workload of Cygwin Git maintainer by upstreaming that change to my tree. But not before. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re
Your Donation to you is ready, contact melc...@gmail.com for more details -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/4] submodule config lookup API
On Mon, Jun 15, 2015 at 2:48 PM, Junio C Hamano wrote: > Thanks. Will replace and wait for comments from others. I have reviewed the patches carefully and they look good to me. As Git is a large project and I was active in other parts until now, I noticed that there are subtle differences in style as when compared to the refs code. One example would be the way comments are written. In d378e35d256348f (Patch 1, implement submodule config API for lookup of .gitmodules values) the comments for the data structures in submodule-config.c seem to have a non exposed "headline" and if more is needed proper sentences with capitalized starts and punctuation at the end. In the refs code there are only sentences IIRC. Most of the commits touching submodule.{c,h} do not prefix their commit message with "submodule:" The style is no show stopper of course, just an observation from someone moving into a different area of code. Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git blame breaking on repository with CRLF files
On 2015-08-10 20.48, Junio C Hamano wrote: > Torsten Bögershausen writes: > >> Actually I could reproduce the following: >> CRLF in repo, CRLF in working tree, core.autocrlf= true. > > What should happen in such a case? Wouldn't autocrlf=true want to > strip CRLF down to LF? Shouldn't it? A problem is, that "git status" would report a file as changed, when it have been committed with CRLF and core.autocrlf was false. The only "change" that "git status" would trigger on would be the EOL normalization. So if core.autocrlf would be set true later, git status reports files as changed. Long story short: Once commited with CRLF, the files will not be normalized in a modern git: >From convert.c: if (crlf_action == CRLF_GUESS) { /* * If the file in the index has any CR in it, do not convert. * This is the new safer autocrlf handling. */ if (has_cr_in_index(path)) return 0; } - commit fd6cce9e89ab5ac1125a3b5f5611048ad22379e7 Author: Eyvind Bernhardsen Date: Wed May 19 22:43:10 2010 +0200 Add per-repository eol normalization Change the semantics of the "crlf" attribute so that it enables end-of-line normalization when it is set, regardless of "core.autocrlf". Add a new setting for "crlf": "auto", which enables end-of-line conversion but does not override the automatic text file detection. Add a new attribute "eol" with possible values "crlf" and "lf". When set, this attribute enables normalization and forces git to use CRLF or LF line endings in the working directory, respectively. The line ending style to be used for normalized text files in the working directory is set using "core.autocrlf". When it is set to "true", CRLFs are used in the working directory; when set to "input" or "false", LFs are used. - So "git status" is somewhat improved, but "git blame" is not. (My feeling/suspicion is that has_cr_in_index() should be replaced by has_cr_in_latest_commit() to have "git status" consistent with "git blame", but more analyzes may be needed.) A different approach could be to ignore the EOL differences completely in "git blame" (when core.autocrlf is set and the file is text, or when the "text" attribute is set). > And if so, "blame" is correct > to say that you are changing the line endings of all your lines, as > what you _would_ commit if you were to commit the tracked files in > your working tree would be different from what is in the index, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] worktree: add 'list' command
'git worktree list' will list the main worktree followed by any linked worktrees which were created using 'git worktree add'. Signed-off-by: Michael Rappazzo --- Documentation/git-worktree.txt | 6 +++- builtin/worktree.c | 67 ++ t/t2027-worktree-list.sh | 30 +++ 3 files changed, 102 insertions(+), 1 deletion(-) create mode 100755 t/t2027-worktree-list.sh diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 3387e2f..efb8e9d 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -11,6 +11,7 @@ SYNOPSIS [verse] 'git worktree add' [-f] [--detach] [-b ] [] 'git worktree prune' [-n] [-v] [--expire ] +'git worktree list' DESCRIPTION --- @@ -59,6 +60,10 @@ prune:: Prune working tree information in $GIT_DIR/worktrees. +list:: + +List the main worktree followed by all of the linked worktrees. + OPTIONS --- @@ -167,7 +172,6 @@ performed manually, such as: - `remove` to remove a linked worktree and its administrative files (and warn if the worktree is dirty) - `mv` to move or rename a worktree and update its administrative files -- `list` to list linked worktrees - `lock` to prevent automatic pruning of administrative files (for instance, for a worktree on a portable device) diff --git a/builtin/worktree.c b/builtin/worktree.c index 6a264ee..d90bdee 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -10,6 +10,7 @@ static const char * const worktree_usage[] = { N_("git worktree add [] "), N_("git worktree prune []"), + N_("git worktree list []"), NULL }; @@ -316,6 +317,70 @@ static int add(int ac, const char **av, const char *prefix) return add_worktree(path, cmd.argv); } +static int list(int ac, const char **av, const char *prefix) +{ + int main_only = 0; + struct option options[] = { + OPT_BOOL(0, "main-only", &main_only, N_("only list the main worktree")), + OPT_END() + }; + + ac = parse_options(ac, av, prefix, options, worktree_usage, 0); + if (ac) + usage_with_options(worktree_usage, options); + + struct strbuf main_path = STRBUF_INIT; + const char* common_dir = get_git_common_dir(); + int is_bare = is_bare_repository(); + if (is_bare) { + strbuf_addstr(&main_path, absolute_path(common_dir)); + strbuf_strip_suffix(&main_path, "/."); + } else if (!strcmp(common_dir, ".git")) { + /* within a worktree, the common dir only returns ".git" */ + strbuf_addstr(&main_path, get_git_work_tree()); + } else { + strbuf_addstr(&main_path, common_dir); + strbuf_strip_suffix(&main_path, "/.git"); + } + + printf("%s\n", main_path.buf); + + if (!is_bare) { + strbuf_addstr(&main_path, "/.git"); + } + strbuf_addstr(&main_path, "/worktrees"); + + if (is_directory(main_path.buf)) { + DIR *dir = opendir(main_path.buf); + if (dir) { + struct dirent *d; + struct stat st; + struct strbuf path = STRBUF_INIT; + struct strbuf other_worktree_path = STRBUF_INIT; + while ((d = readdir(dir)) != NULL) { + if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, "..")) + continue; + strbuf_reset(&other_worktree_path); + strbuf_addf(&other_worktree_path, "%s/%s/gitdir", main_path.buf, d->d_name); + + if (stat(other_worktree_path.buf, &st)) + continue; + + strbuf_reset(&path); + strbuf_read_file(&path, other_worktree_path.buf, st.st_size); + strbuf_strip_suffix(&path, "/.git\n"); + + printf("%s\n", path.buf); + } + strbuf_release(&other_worktree_path); + strbuf_release(&path); + } + closedir(dir); + } + strbuf_release(&main_path); + return 0; +} + int cmd_worktree(int ac, const char **av, const char *prefix) { struct option options[] = { @@ -328,5 +393,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix) return add(ac - 1, av + 1, prefix); if (!strcmp(av[1], "prune")) return prune(ac - 1, av + 1, prefix); + if (!strcmp(av[1], "list")) + return list(ac - 1, av + 1, prefix); usage_with_options(worktree_usage, options); } diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh new file mode 100755 index 000..8e3dbbc --- /dev/nu
[PATCH v3] worktree: add 'list' command
Differences from [v2](http://www.mail-archive.com/git@vger.kernel.org/msg75467.html) - removed unintended whitespace changes - cleanup based on comments from v2 Michael Rappazzo (1): worktree: add 'list' command Documentation/git-worktree.txt | 6 +++- builtin/worktree.c | 67 ++ t/t2027-worktree-list.sh | 30 +++ 3 files changed, 102 insertions(+), 1 deletion(-) create mode 100755 t/t2027-worktree-list.sh -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] worktree: add 'list' command
Michael Rappazzo writes: > +static int list(int ac, const char **av, const char *prefix) > +{ > + int main_only = 0; > + struct option options[] = { > + OPT_BOOL(0, "main-only", &main_only, N_("only list the main > worktree")), > + OPT_END() > + }; Hmm, main-only is still there? > + > + ac = parse_options(ac, av, prefix, options, worktree_usage, 0); > + if (ac) > + usage_with_options(worktree_usage, options); > + > + struct strbuf main_path = STRBUF_INIT; > + const char* common_dir = get_git_common_dir(); Asterisks stick to the variable names, not types. > + int is_bare = is_bare_repository(); Please do not introduce decl-after-stmt. > + if (is_bare) { > + strbuf_addstr(&main_path, absolute_path(common_dir)); Hmm, interesting. Because .git/config is shared, core.bare read from that tells us if the "main" one is bare, even if you start this command from one of its linked worktrees. So in that sense, this test of is_bare correctly tells if "main" one is a bare repository. But that by itself feels wrong. Doesn't the presense of a working tree mean that you should not get "is_bare==true" in such a case (i.e. your "main" one is bare, you are in a linked worktree of it that has the index and the working tree)? Duy? Eric? What do you guys think? There are many codepaths that change their behaviour (e.g. if we create reflogs by default) based on the return value of is_bare_repository(). If I am reading this correctly, I _think_ a new working area that was prepared with "git worktree add" out of a bare repository would not work well, as these operations behave as if we do not have a working tree. Perhaps is_bare_repository() in such a working area _should_ say "No", even if core.bare in the shared bare one is set to true. > + strbuf_strip_suffix(&main_path, "/."); In any case, what is that stripping of "/." about? Who is adding that extra trailing string? What I am getting at is (1) perhaps it shouldn't be adding that in the first place, and (2) if some other code is randomly adding "/." at the end, what guarantees you that you would need to strip it only once here---if the other code added that twice, don't you have to repeatedly remove "/." from the end? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Error pushing new branch: value too great for base (error token is...
On Mon, Aug 10, 2015 at 2:54 AM, Gaurav Chhabra wrote: > Apologies for the delay in reply! I tried your suggestion and it > works. Thanks! :) > > I'm curious why integer comparison is throwing error. Shouldn't i be > comparing numbers with numeric operator? > Yes, but shell doesn't treat hex numbers as numbers. So it will work only if the string is a decimal number. Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] worktrees: add find_shared_symref
On Mon, Aug 10, 2015 at 1:52 PM, David Turner wrote: > worktrees: add find_shared_symref s/worktrees/branch/ perhaps? > Add a new function, find_shared_symref, which contains the heart of > die_if_checked_out, but works for any symref, not just HEAD. Refactor > die_if_checked_out to use the same infrastructure as > find_shared_symref. > > Soon, we will use find_shared_symref to protect notes merges in > worktrees. > > Signed-off-by: David Turner > --- > This version addresses Eric Sunshine's comments on v5. It fixes an error > message and cleans up the code. All issues identified in previous versions seem to have been addressed, and nothing else pops out at me. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/17] remove hold_lock_file_for_append
Jeff King writes: > No users of hold_lock_file_for_append remain, so remove it. This does not seem to have anything to do with rotating static buffers used in get_pathname(); the only effect it has is to conflict heavily with Michael's tempfile topic X-<. Perhaps this should be part of Michael's tempfile topic? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] worktrees: add find_shared_symref
On Mon, 2015-08-10 at 18:30 -0400, Eric Sunshine wrote: > On Mon, Aug 10, 2015 at 1:52 PM, David Turner > wrote: > > worktrees: add find_shared_symref > > s/worktrees/branch/ perhaps? Do you mean "this is in branch.c, so should be labeled with branch"? Because this change is mostly about non-branch refs, so I think saying "branch" is confusing. That's why I labelled it "worktrees"; that's the broad topic that's being addressed. > > Add a new function, find_shared_symref, which contains the heart of > > die_if_checked_out, but works for any symref, not just HEAD. Refactor > > die_if_checked_out to use the same infrastructure as > > find_shared_symref. > > > > Soon, we will use find_shared_symref to protect notes merges in > > worktrees. > > > > Signed-off-by: David Turner > > --- > > This version addresses Eric Sunshine's comments on v5. It fixes an error > > message and cleans up the code. > > All issues identified in previous versions seem to have been > addressed, and nothing else pops out at me. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Error pushing new branch: value too great for base (error token is...
On Mon, Aug 10, 2015 at 6:29 PM, Jacob Keller wrote: > On Mon, Aug 10, 2015 at 2:54 AM, Gaurav Chhabra > wrote: >> Apologies for the delay in reply! I tried your suggestion and it >> works. Thanks! :) >> >> I'm curious why integer comparison is throwing error. Shouldn't i be >> comparing numbers with numeric operator? > > Yes, but shell doesn't treat hex numbers as numbers. So it will work > only if the string is a decimal number. This particular case deserves a bit more explanation. The expression in question was this: if [[ "$new_sha" -eq "$NULL" ]]; then where 'new_sha' was 9226289d2416af4cb7365d7aaa5e382bdb3d9a89. In Bash, inside the [[ .. ]], it did attempt evaluating the SHA1 as a *decimal* number, however, when it encountered the "d", it complained that it was outside the allowed range of decimal digits ("0"..."9"). Had the SHA1 been prefixed by a "0x", the [[...]] context would have dealt with it just fine. Outside the [[...]] context, arguments to -eq do need to be base-10 integers. Nevertheless, a SHA1 is effective an opaque value. There's little, if anything, to be gained by treating it as a numeric quantity, hence string '=' makes more sense than numeric '-eq'. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] worktrees: add find_shared_symref
On Mon, Aug 10, 2015 at 6:42 PM, David Turner wrote: > On Mon, 2015-08-10 at 18:30 -0400, Eric Sunshine wrote: >> On Mon, Aug 10, 2015 at 1:52 PM, David Turner >> wrote: >> > worktrees: add find_shared_symref >> >> s/worktrees/branch/ perhaps? > > Do you mean "this is in branch.c, so should be labeled with branch"? > > Because this change is mostly about non-branch refs, so I think saying > "branch" is confusing. That's why I labelled it "worktrees"; that's the > broad topic that's being addressed. Okay. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/17] path.c: drop git_path_submodule
Jeff King writes: > There are no callers of the slightly-dangerous static-buffer > git_path_submodule left. Let's drop it. There are a few callers added on 'pu', though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/17] path.c: drop git_path_submodule
Junio C Hamano writes: > Jeff King writes: > >> There are no callers of the slightly-dangerous static-buffer >> git_path_submodule left. Let's drop it. > > There are a few callers added on 'pu', though. Actually there is only one. Here is a proposed evil merge. diff --git a/submodule.c b/submodule.c index dfe8b7b..7ab08a1 100644 --- a/submodule.c +++ b/submodule.c @@ -120,34 +120,35 @@ void stage_updated_gitmodules(void) static int add_submodule_odb(const char *path) { struct alternate_object_database *alt_odb; - const char *objects_directory; + struct strbuf objects_directory = STRBUF_INIT; int ret = 0; - objects_directory = git_path_submodule(path, "objects/"); - if (!is_directory(objects_directory)) { + strbuf_git_path_submodule(&objects_directory, "objects/", "%s", path); + if (!is_directory(objects_directory.buf)) { ret = -1; goto done; } /* avoid adding it twice */ for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next) - if (alt_odb->name - alt_odb->base == strlen(objects_directory) && - !strcmp(alt_odb->base, objects_directory)) + if (alt_odb->name - alt_odb->base == objects_directory.len && + !strcmp(alt_odb->base, objects_directory.buf)) goto done; - alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb)); + alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb)); alt_odb->next = alt_odb_list; - strcpy(alt_odb->base, objects_directory); - alt_odb->name = alt_odb->base + strlen(objects_directory); + strcpy(alt_odb->base, objects_directory.buf); + alt_odb->name = alt_odb->base + objects_directory.len; alt_odb->name[2] = '/'; alt_odb->name[40] = '\0'; alt_odb->name[41] = '\0'; alt_odb_list = alt_odb; /* add possible alternates from the submodule */ - read_info_alternates(objects_directory, 0); + read_info_alternates(objects_directory.buf, 0); prepare_alt_odb(); done: + strbuf_release(&objects_directory); return ret; } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms
On Mon, Aug 10, 2015 at 6:06 AM, Jan Viktorin wrote: > On Sun, 9 Aug 2015 14:13:33 -0400 > Eric Sunshine wrote: >> One possibility which comes to mind is to create a fake >> Authen::SASL::Perl which merely dumps its input mechanisms to a file, >> and arrange for the Perl search path to find the fake one instead. You >> could then check the output file to see if it reflects your >> expectations. However, this may be overkill and perhaps not worth the >> effort (especially if you're not a Perl programmer). > > I think that Authen::SASL::Perl mock would not help. I wanted to create > some fake sendmail (but this is impossible as stated above because > then the perl modules are not used). So the only way would be to > provide some fake socket with a static content on the other side. This > is really an overkill to just test the few lines of code. Agreed. > So, what more can I do for this feature? I don't have any further suggestions. Other than the unwanted "Supported:" line in the documentation and the couple style issues[1], the patch seems sufficiently complete, as-is. The validation regex gets a "meh" from me merely because it's not clear how beneficial it will be in practice, but that's not an outright objection; I don't feel strongly about it either way. [1]: http://article.gmane.org/gmane.comp.version-control.git/275150 > I think that the basic regex test is OK. It can accept lowercase > letters and do an explicit uppercase call. I do not like to rely on > internals of the SASL library. As you could see, the SASL::Perl does > not check its inputs in a very good way and its code is quite unclear > (strange for a library providing security features). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html