Re: [PATCH v6 02/39] api-lockfile: revise and expand the documentation
On 09/30/2014 07:39 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> I was being repetitive because I didn't want the docs to depend on the >> user remembering what the "bss" section is (which, technically, is also >> not part of the C standard). I think a better way would be to just not >> mention "bss section" at all and reword the rest. Maybe something like >> >> The caller: >> >> * Allocates a variable `struct lock_file lock`, initialized to zeros. >> Because the `lock_file` structure is used in an `atexit(3)` handler, >> its storage has to remain valid throughout the life of the program; >> e.g., it should be a static variable or space allocated on the heap. >> >> Better? > > Somewhat. I like that you droped "BSS", though. > > Allocates a 'struct lock_file' either as a static variable > or on the heap, initialized to zeros. Once you use the > structure to call 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). OK, I'll use that. >>> It feels a bit conflicting between "must not be freed or ALTERED" >>> and "it may be REUSED". Drop "or altered" to reduce confusion? And >>> this repeats the third sentence I suggested to remove from the first >>> paragraph (above 'see below' refers here). >> >> The purpose of "or altered" is to make sure that users don't imagine >> that they should memset() the structure to zeros or something before >> reusing it (especially since the "caller" instructions earlier say that >> the object should be "initialized to zeros"). >> >> Would it help if I change >> >> s/altered/altered by the caller/ >> >> ? > > The fundamental rule is that callers outside the lockfile system must > not touch individual members of "struct lock_file" that is "live". > They must not free, they must not alter, they must not do anything > other than calling the lockfile API functions. > > While it is natural that the readers would understand such a rule > must be followed for a lockfile that is in either the initialized, > locked, closed-but-not-committed state, I agree that it is not just > possible but likely that people misunderstand and think that once a > lockfile is committed or rolled back it no longer has to follow that > rule. We would want to make sure readers do not fall into such a > misunderstanding. > > I dunno. Your "if you memset it to NULs, you will break the linked > list of the lock and the whole lockfile system and the element > cannot even be reused." may be the most important thing you may have > wanted to say, but it is not conveyed by that change at all, at > least to me. > > A small voice in the back of my skull keeps telling me that a rule > that is hard to document and explain is a rule that does not make > sense. Is it possible to allow commit and rollback to safely remove > the structure from the atexit(3) list (aka "no longer owned by the > lockfile subsystem")? Certainly it is possible. One would probably make lock_file an opaque data structure, always allocated on the heap within the lockfile subsystem, and maybe with a free list to reduce memory allocations. But it would be a lot of work and I don't see a commensurate payoff. There are not *that* many callers of the lockfile API. >>> Is it allowed to tell the name of this lock_file to other people and >>> let them read from it? The answer is yes but it is not obvious from >>> this description. >>> >>> Also mention how the above interact with reopen_lock_file() here? >> >> I'll take a stab at it, though TBH I haven't really studied the use case >> for reopen_lock_file(). You might be better able to provide insight into >> this topic. > > You would want to be able to do this: > > - hold a lock on a file (say, the index); > > - update it in preparation to commit it; > > - write it out and make sure the contents is on the disk platter, >in preparation to call another program and allow it (and nobody >else) to inspect the contents you wrote, while still holding the >lock yourself. In our set of API functions, close_lock_file is >what lets us do this. > > - further update it, write it out and commit. We need to read it >first, open(2) it to write, write(2), and commit_lock_file(). > > The set of API functions you described in the document, there is no > way to say "I already have a lock on that file, just let me open(2) > for writing because I have further updates" and that is where reopen > comes in. Thanks for the clarification. My upcoming reroll will document reopen_lock_file(). 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 v6 02/39] api-lockfile: revise and expand the documentation
On 09/30/2014 07:47 PM, Junio C Hamano wrote: > Jeff King writes: > >> On Tue, Sep 30, 2014 at 03:41:55PM +0200, Michael Haggerty wrote: >> >>> I didn't fix it because IMO the correct fix is to add a stdio-oriented >>> entry point to the lockfile API, and teach the lockfile code to handle >>> closing the FILE correctly when necessary. >> >> I think so, too, after our discussion[1] surrounding 9540ce5 (refs: write >> packed_refs file using stdio, 2014-09-10). > > Yeah, but we already write packed-refs via stdio, so the stdio > oriented lockfile API entry points can no longer be just on the > mythical todo list but needs to become reality before we can merge > this topic sanely. That's not the fault of this topic, which just moves the text of the "rule" to a different place in the file. And neither is it the fault of Peff's change to write packed-refs via stdio. It is the fault of 60b9004 Use atomic updates to the fast-import mark file (2007-03-08) which also fdopen()ed then fclose()d a lock_file::fd, and of 0c0478c Document lockfile API (2008-01-16) which documented the "rule" that had already been broken. >>> But I didn't want to add even more changes to this patch series, so I am >>> working on this as a separate patch series. I hope to submit it soon. >> >> Yay. > > Yay. 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
[PATCH 1/2] sha1-array: add test-sha1-array and basic tests
Signed-off-by: Rene Scharfe
---
.gitignore| 1 +
Makefile | 1 +
t/t0064-sha1-array.sh | 64 +++
test-sha1-array.c | 34 +++
4 files changed, 100 insertions(+)
create mode 100755 t/t0064-sha1-array.sh
create mode 100644 test-sha1-array.c
diff --git a/.gitignore b/.gitignore
index 5bfb234..9ec40fa 100644
--- a/.gitignore
+++ b/.gitignore
@@ -199,6 +199,7 @@
/test-revision-walking
/test-run-command
/test-sha1
+/test-sha1-array
/test-sigchain
/test-string-list
/test-subprocess
diff --git a/Makefile b/Makefile
index f34a2d4..356feb5 100644
--- a/Makefile
+++ b/Makefile
@@ -568,6 +568,7 @@ TEST_PROGRAMS_NEED_X += test-revision-walking
TEST_PROGRAMS_NEED_X += test-run-command
TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
TEST_PROGRAMS_NEED_X += test-sha1
+TEST_PROGRAMS_NEED_X += test-sha1-array
TEST_PROGRAMS_NEED_X += test-sigchain
TEST_PROGRAMS_NEED_X += test-string-list
TEST_PROGRAMS_NEED_X += test-subprocess
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
new file mode 100755
index 000..bd68789
--- /dev/null
+++ b/t/t0064-sha1-array.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='basic tests for the SHA1 array implementation'
+. ./test-lib.sh
+
+echo20() {
+ prefix="$1"
+ shift
+ while test $# -gt 0
+ do
+ echo "$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
+ shift
+ done
+}
+
+test_expect_success 'ordered enumeration' '
+ echo20 "" 44 55 88 aa >expect &&
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo for_each_unique
+ } | test-sha1-array >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ordered enumeration with duplicate suppression' '
+ echo20 "" 44 55 88 aa >expect &&
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "append " 88 44 aa 55 &&
+ echo for_each_unique
+ } | test-sha1-array >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'lookup' '
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "lookup " 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -eq 1
+'
+
+test_expect_success 'lookup non-existing entry' '
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "lookup " 33
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -lt 0
+'
+
+test_expect_success 'lookup with duplicates' '
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "lookup " 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -ge 2 &&
+ test "$n" -le 3
+'
+
+test_done
diff --git a/test-sha1-array.c b/test-sha1-array.c
new file mode 100644
index 000..ddc491e
--- /dev/null
+++ b/test-sha1-array.c
@@ -0,0 +1,34 @@
+#include "cache.h"
+#include "sha1-array.h"
+
+static void print_sha1(const unsigned char sha1[20], void *data)
+{
+ puts(sha1_to_hex(sha1));
+}
+
+int main(int argc, char **argv)
+{
+ struct sha1_array array = SHA1_ARRAY_INIT;
+ struct strbuf line = STRBUF_INIT;
+
+ while (strbuf_getline(&line, stdin, '\n') != EOF) {
+ const char *arg;
+ unsigned char sha1[20];
+
+ if (skip_prefix(line.buf, "append ", &arg)) {
+ if (get_sha1_hex(arg, sha1))
+ die("not a hexadecimal SHA1: %s", arg);
+ sha1_array_append(&array, sha1);
+ } else if (skip_prefix(line.buf, "lookup ", &arg)) {
+ if (get_sha1_hex(arg, sha1))
+ die("not a hexadecimal SHA1: %s", arg);
+ printf("%d\n", sha1_array_lookup(&array, sha1));
+ } else if (!strcmp(line.buf, "clear"))
+ sha1_array_clear(&array);
+ else if (!strcmp(line.buf, "for_each_unique"))
+ sha1_array_for_each_unique(&array, print_sha1, NULL);
+ else
+ die("unknown command: %s", line.buf);
+ }
+ return 0;
+}
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()
If the first 18 bytes of the SHA1's of all entries are the same then
sha1_pos() dies and reports that the lower and upper limits of the
binary search were the same that this wasn't supposed to happen. This
is wrong because the remaining two bytes could still differ.
Furthermore: It wouldn't be a problem if they actually were the same,
i.e. if all entries have the same SHA1. The code already handles
duplicates just fine otherwise. Simply remove the erroneous check.
Signed-off-by: Rene Scharfe
---
sha1-lookup.c | 2 --
t/t0064-sha1-array.sh | 20
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/sha1-lookup.c b/sha1-lookup.c
index 2dd8515..5f06921 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -84,8 +84,6 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t
nr,
die("BUG: assertion failed in binary search");
}
}
- if (18 <= ofs)
- die("cannot happen -- lo and hi are identical");
}
do {
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
index bd68789..3fcb8d8 100755
--- a/t/t0064-sha1-array.sh
+++ b/t/t0064-sha1-array.sh
@@ -61,4 +61,24 @@ test_expect_success 'lookup with duplicates' '
test "$n" -le 3
'
+test_expect_success 'lookup with almost duplicate values' '
+ {
+ echo "append " &&
+ echo "append 555f" &&
+ echo20 "lookup " 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -eq 0
+'
+
+test_expect_success 'lookup with single duplicate value' '
+ {
+ echo20 "append " 55 55 &&
+ echo20 "lookup " 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -ge 0 &&
+ test "$n" -le 1
+'
+
test_done
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] daemon: handle gethostbyname() error
If the user-supplied hostname can't be found then we should not use it.
We already avoid doing that in the non-NO_IPV6 case by checking if the
return value of getaddrinfo() is zero (success). Do the same in the
NO_IPV6 case and make sure the return value of gethostbyname() isn't
NULL before dereferencing this pointer.
Signed-off-by: Rene Scharfe
---
Most lines are just re-indented, not actually changed.
daemon.c | 27 ++-
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/daemon.c b/daemon.c
index 4dcfff9..a6f467e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -553,20 +553,21 @@ static void parse_host_arg(char *extra_args, int buflen)
static char addrbuf[HOST_NAME_MAX + 1];
hent = gethostbyname(hostname);
+ if (hent) {
+ ap = hent->h_addr_list;
+ memset(&sa, 0, sizeof sa);
+ sa.sin_family = hent->h_addrtype;
+ sa.sin_port = htons(0);
+ memcpy(&sa.sin_addr, *ap, hent->h_length);
+
+ inet_ntop(hent->h_addrtype, &sa.sin_addr,
+ addrbuf, sizeof(addrbuf));
- ap = hent->h_addr_list;
- memset(&sa, 0, sizeof sa);
- sa.sin_family = hent->h_addrtype;
- sa.sin_port = htons(0);
- memcpy(&sa.sin_addr, *ap, hent->h_length);
-
- inet_ntop(hent->h_addrtype, &sa.sin_addr,
- addrbuf, sizeof(addrbuf));
-
- free(canon_hostname);
- canon_hostname = xstrdup(hent->h_name);
- free(ip_address);
- ip_address = xstrdup(addrbuf);
+ free(canon_hostname);
+ canon_hostname = xstrdup(hent->h_name);
+ free(ip_address);
+ ip_address = xstrdup(addrbuf);
+ }
#endif
}
}
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] daemon: fix error message after bind()
Signed-off-by: Rene Scharfe
---
daemon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/daemon.c b/daemon.c
index a6f467e..090f6a4 100644
--- a/daemon.c
+++ b/daemon.c
@@ -924,7 +924,7 @@ static int setup_named_sock(char *listen_addr, int
listen_port, struct socketlis
}
if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
- logerror("Could not listen to %s: %s",
+ logerror("Could not bind to %s: %s",
ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
strerror(errno));
close(sockfd);
--
2.1.2
--
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 3/3] daemon: remove write-only variable maxfd
It became unused when 6573faff (NO_IPV6 support for git daemon) replaced
select() with poll().
Signed-off-by: Rene Scharfe
---
daemon.c | 4
1 file changed, 4 deletions(-)
diff --git a/daemon.c b/daemon.c
index 090f6a4..54a03bd 100644
--- a/daemon.c
+++ b/daemon.c
@@ -815,7 +815,6 @@ static const char *ip2str(int family, struct sockaddr *sin,
socklen_t len)
static int setup_named_sock(char *listen_addr, int listen_port, struct
socketlist *socklist)
{
int socknum = 0;
- int maxfd = -1;
char pbuf[NI_MAXSERV];
struct addrinfo hints, *ai0, *ai;
int gai;
@@ -883,9 +882,6 @@ static int setup_named_sock(char *listen_addr, int
listen_port, struct socketlis
ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
socklist->list[socklist->nr++] = sockfd;
socknum++;
-
- if (maxfd < sockfd)
- maxfd = sockfd;
}
freeaddrinfo(ai0);
--
2.1.2
--
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 v7 01/38] unable_to_lock_die(): rename function from unable_to_lock_index_die()
This function is used for other things besides the index, so rename it
accordingly.
Suggested-by: Jeff King
Signed-off-by: Michael Haggerty
Reviewed-by: Ronnie Sahlberg
Reviewed-by: Jonathan Nieder
---
builtin/update-index.c | 2 +-
cache.h| 2 +-
lockfile.c | 6 +++---
refs.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/update-index.c b/builtin/update-index.c
index e8c7fd4..6c95988 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -942,7 +942,7 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
exit(128);
- unable_to_lock_index_die(get_index_file(), lock_error);
+ unable_to_lock_die(get_index_file(), lock_error);
}
if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
die("Unable to write new index file");
diff --git a/cache.h b/cache.h
index 8206039..0a76d02 100644
--- a/cache.h
+++ b/cache.h
@@ -582,7 +582,7 @@ struct lock_file {
extern int unable_to_lock_error(const char *path, int err);
extern void unable_to_lock_message(const char *path, int err,
struct strbuf *buf);
-extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+extern NORETURN void unable_to_lock_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
extern int commit_lock_file(struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index 2a800ce..f1ce154 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -185,7 +185,7 @@ int unable_to_lock_error(const char *path, int err)
return -1;
}
-NORETURN void unable_to_lock_index_die(const char *path, int err)
+NORETURN void unable_to_lock_die(const char *path, int err)
{
struct strbuf buf = STRBUF_INIT;
@@ -198,7 +198,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const
char *path, int flags)
{
int fd = lock_file(lk, path, flags);
if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
- unable_to_lock_index_die(path, errno);
+ unable_to_lock_die(path, errno);
return fd;
}
@@ -209,7 +209,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
fd = lock_file(lk, path, flags);
if (fd < 0) {
if (flags & LOCK_DIE_ON_ERROR)
- unable_to_lock_index_die(path, errno);
+ unable_to_lock_die(path, errno);
return fd;
}
diff --git a/refs.c b/refs.c
index ffd45e9..0e32477 100644
--- a/refs.c
+++ b/refs.c
@@ -2225,7 +2225,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
*/
goto retry;
else
- unable_to_lock_index_die(ref_file, errno);
+ unable_to_lock_die(ref_file, errno);
}
return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
--
2.1.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 v7 11/38] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
There are a few places that use these values, so define constants for
them.
Signed-off-by: Michael Haggerty
---
cache.h| 4
lockfile.c | 11 ++-
refs.c | 7 ---
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/cache.h b/cache.h
index 0a76d02..24891a8 100644
--- a/cache.h
+++ b/cache.h
@@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce,
struct stat *st);
#define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not "needs
update" */
extern int refresh_index(struct index_state *, unsigned int flags, const
struct pathspec *pathspec, char *seen, const char *header_msg);
+/* String appended to a filename to derive the lockfile name: */
+#define LOCK_SUFFIX ".lock"
+#define LOCK_SUFFIX_LEN 5
+
struct lock_file {
struct lock_file *next;
int fd;
diff --git a/lockfile.c b/lockfile.c
index 2680dc9..23847fc 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -166,10 +166,11 @@ static char *resolve_symlink(char *p, size_t s)
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
/*
-* subtract 5 from size to make sure there's room for adding
-* ".lock" for the lock file name
+* subtract LOCK_SUFFIX_LEN from size to make sure there's
+* room for adding ".lock" for the lock file name:
*/
- static const size_t max_path_len = sizeof(lk->filename) - 5;
+ static const size_t max_path_len = sizeof(lk->filename) -
+ LOCK_SUFFIX_LEN;
if (!lock_file_list) {
/* One-time initialization */
@@ -194,7 +195,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
strcpy(lk->filename, path);
if (!(flags & LOCK_NODEREF))
resolve_symlink(lk->filename, max_path_len);
- strcat(lk->filename, ".lock");
+ strcat(lk->filename, LOCK_SUFFIX);
lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
if (0 <= lk->fd) {
lk->owner = getpid();
@@ -308,7 +309,7 @@ int commit_lock_file(struct lock_file *lk)
if (close_lock_file(lk))
return -1;
strcpy(result_file, lk->filename);
- i = strlen(result_file) - 5; /* .lock */
+ i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
result_file[i] = 0;
if (rename(lk->filename, result_file))
return -1;
diff --git a/refs.c b/refs.c
index 0e32477..73d6bae 100644
--- a/refs.c
+++ b/refs.c
@@ -79,7 +79,8 @@ out:
if (refname[1] == '\0')
return -1; /* Component equals ".". */
}
- if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
+ if (cp - refname >= LOCK_SUFFIX_LEN &&
+ !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
return -1; /* Refname ends with ".lock". */
return cp - refname;
}
@@ -2602,11 +2603,11 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
{
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+ int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
lock->lk->filename[i] = 0;
err = unlink_or_warn(lock->lk->filename);
- lock->lk->filename[i] = '.';
+ lock->lk->filename[i] = LOCK_SUFFIX[0];
if (err && errno != ENOENT)
return 1;
}
--
2.1.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 v7 00/38] Lockfile correctness and refactoring
Yet another iteration of my lockfile fixes and refactoring. Thanks to Junio for his comments about v6. I believe that this series addresses all of the comments from v1 [1], v2 [2], v3 [3], v4 [4], v5 [5], and v6 [6]. Changes since v6: * Rebased on current master to resolve conflicts with jk/write-packed-refs-via-stdio. Changes made in that branch removed the need for the following patch from v6: [13/39] prepare_index(): declare return value to be (const char *) * More improvements to the API documentation. I will separately submit patches to support stdio-based access to lockfiles. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/245609 [2] http://thread.gmane.org/gmane.comp.version-control.git/245801 [3] http://thread.gmane.org/gmane.comp.version-control.git/246222 [4] http://thread.gmane.org/gmane.comp.version-control.git/256564 [5] http://thread.gmane.org/gmane.comp.version-control.git/257159 [6] http://thread.gmane.org/gmane.comp.version-control.git/257504 Michael Haggerty (38): unable_to_lock_die(): rename function from unable_to_lock_index_die() api-lockfile: revise and expand the documentation close_lock_file(): exit (successfully) if file is already closed rollback_lock_file(): do not clear filename redundantly rollback_lock_file(): exit early if lock is not active rollback_lock_file(): set fd to -1 lockfile: unlock file if lockfile permissions cannot be adjusted hold_lock_file_for_append(): release lock on errors lock_file(): always initialize and register lock_file object lockfile.c: document the various states of lock_file objects cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN delete_ref_loose(): don't muck around in the lock_file's filename prepare_index(): declare return value to be (const char *) lock_file(): exit early if lockfile cannot be opened remove_lock_file(): call rollback_lock_file() commit_lock_file(): inline temporary variable commit_lock_file(): die() if called for unlocked lockfile object close_lock_file(): if close fails, roll back commit_lock_file(): rollback lock file on failure to rename api-lockfile: document edge cases dump_marks(): remove a redundant call to rollback_lock_file() git_config_set_multivar_in_file(): avoid call to rollback_lock_file() lockfile: avoid transitory invalid states struct lock_file: declare some fields volatile try_merge_strategy(): remove redundant lock_file allocation try_merge_strategy(): use a statically-allocated lock_file object commit_lock_file(): use a strbuf to manage temporary space Change lock_file::filename into a strbuf resolve_symlink(): use a strbuf for internal scratch space resolve_symlink(): take a strbuf parameter trim_last_path_component(): replace last_path_elm() Extract a function commit_lock_file_to() Rename LOCK_NODEREF to LOCK_NO_DEREF lockfile.c: rename static functions get_locked_file_path(): new function hold_lock_file_for_append(): restore errno before returning Move read_index() definition to read-cache.c lockfile.h: extract new header file for the functions in lockfile.c Documentation/technical/api-lockfile.txt | 242 -- builtin/add.c| 1 + builtin/apply.c | 1 + builtin/checkout-index.c | 2 +- builtin/checkout.c | 2 +- builtin/clone.c | 1 + builtin/commit.c | 17 +- builtin/describe.c | 1 + builtin/diff.c | 1 + builtin/gc.c | 2 +- builtin/merge.c | 16 +- builtin/mv.c | 2 +- builtin/read-tree.c | 1 + builtin/receive-pack.c | 1 + builtin/reflog.c | 4 +- builtin/reset.c | 1 + builtin/rm.c | 2 +- builtin/update-index.c | 3 +- bundle.c | 1 + cache-tree.c | 1 + cache.h | 20 +-- config.c | 16 +- credential-store.c | 1 + fast-import.c| 5 +- fetch-pack.c | 1 + lockfile.c | 284 +-- lockfile.h | 84 + merge-recursive.c| 1 + merge.c | 1 + read-cache.c | 21 ++- refs.c | 23 +-- rerere.c | 1 + sequencer.c | 1 + sha1_file.c | 1 + shallow.c| 7 +- test-scrap-cache-tree.c
[PATCH v7 03/38] close_lock_file(): exit (successfully) if file is already closed
Suggested-by: Jonathan Nieder
Signed-off-by: Michael Haggerty
---
lockfile.c | 6 +-
read-cache.c | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index f1ce154..d02c3bf 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -233,6 +233,10 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
int close_lock_file(struct lock_file *lk)
{
int fd = lk->fd;
+
+ if (fd < 0)
+ return 0;
+
lk->fd = -1;
return close(fd);
}
@@ -251,7 +255,7 @@ int commit_lock_file(struct lock_file *lk)
{
char result_file[PATH_MAX];
size_t i;
- if (lk->fd >= 0 && close_lock_file(lk))
+ if (close_lock_file(lk))
return -1;
strcpy(result_file, lk->filename);
i = strlen(result_file) - 5; /* .lock */
diff --git a/read-cache.c b/read-cache.c
index 2fc1182..5ffb1d7 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2042,7 +2042,7 @@ void set_alternate_index_output(const char *name)
static int commit_locked_index(struct lock_file *lk)
{
if (alternate_index_output) {
- if (lk->fd >= 0 && close_lock_file(lk))
+ if (close_lock_file(lk))
return -1;
if (rename(lk->filename, alternate_index_output))
return -1;
--
2.1.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 v7 12/38] delete_ref_loose(): don't muck around in the lock_file's filename
It's bad manners. Especially since there could be a signal during the
call to unlink_or_warn(), in which case the signal handler will see
the wrong filename and delete the reference file, leaving the lockfile
behind.
So make our own copy to work with.
Signed-off-by: Michael Haggerty
---
refs.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 73d6bae..a415131 100644
--- a/refs.c
+++ b/refs.c
@@ -2602,12 +2602,15 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
static int delete_ref_loose(struct ref_lock *lock, int flag)
{
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
- /* loose */
- int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;
-
- lock->lk->filename[i] = 0;
- err = unlink_or_warn(lock->lk->filename);
- lock->lk->filename[i] = LOCK_SUFFIX[0];
+ /*
+* loose. The loose file name is the same as the
+* lockfile name, minus ".lock":
+*/
+ char *loose_filename = xmemdupz(
+ lock->lk->filename,
+ strlen(lock->lk->filename) - LOCK_SUFFIX_LEN);
+ int err = unlink_or_warn(loose_filename);
+ free(loose_filename);
if (err && errno != ENOENT)
return 1;
}
--
2.1.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 v7 13/38] prepare_index(): declare return value to be (const char *)
Declare the return value to be const to make it clear that we aren't
giving callers permission to write over the string that it points at.
(The return value is the filename field of a struct lock_file, which
can be used by a signal handler at any time and therefore shouldn't be
tampered with.)
Signed-off-by: Michael Haggerty
---
builtin/commit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index b0fe784..70f5935 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -315,8 +315,8 @@ static void refresh_cache_or_die(int refresh_flags)
die_resolve_conflict("commit");
}
-static char *prepare_index(int argc, const char **argv, const char *prefix,
- const struct commit *current_head, int is_status)
+static const char *prepare_index(int argc, const char **argv, const char
*prefix,
+const struct commit *current_head, int
is_status)
{
struct string_list partial;
struct pathspec pathspec;
--
2.1.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 v7 02/38] api-lockfile: revise and expand the documentation
Document a couple more functions and the flags argument as used by hold_lock_file_for_update() and hold_lock_file_for_append(). Reorganize the document to make it more accessible. Helped-by: Jonathan Nieder Helped-by: Junio Hamano Signed-off-by: Michael Haggerty --- Documentation/technical/api-lockfile.txt | 221 +++ 1 file changed, 167 insertions(+), 54 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index dd89404..99830f3 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -3,20 +3,125 @@ lockfile API The lockfile API serves two purposes: -* Mutual exclusion. When we write out a new index file, first - we create a new file `$GIT_DIR/index.lock`, write the new - contents into it, and rename it to the final destination - `$GIT_DIR/index`. We try to create the `$GIT_DIR/index.lock` - file with O_EXCL so that we can notice and fail when somebody - else is already trying to update the index file. - -* Automatic cruft removal. After we create the "lock" file, we - may decide to `die()`, and we would want to make sure that we - remove the file that has not been committed to its final - destination. This is done by remembering the lockfiles we - created in a linked list and cleaning them up from an - `atexit(3)` handler. Outstanding lockfiles are also removed - when the program dies on a signal. +* 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 writing to the file + descriptor returned by those functions (also available via + `lock->fd`). + +When finished writing, the caller can: + +* Close the file descriptor and rename the lockfile to its final + destination by calling `commit_lock_file`. + +* 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`, + `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`, +`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)` yourself! +Otherwise the `struct lock_file` structure would still think that the +file descriptor needs to be closed, and a later call to +`commit_lock_file` or `rollback_lock_file` or program exit would +result in duplicate calls to `close(2)`. Worse yet, if you `close(2)` +and then later open another file descriptor for a completely different +purpose, then a call to `commit_lock_file` or `rollback_lock_file` +might close that unrelated file descriptor. + + +Error handling
[PATCH v7 04/38] rollback_lock_file(): do not clear filename redundantly
It is only necessary to clear the lock_file's filename field if it was not already clear. Signed-off-by: Michael Haggerty Reviewed-by: Ronnie Sahlberg Reviewed-by: Jonathan Nieder --- lockfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lockfile.c b/lockfile.c index d02c3bf..5330d6a 100644 --- a/lockfile.c +++ b/lockfile.c @@ -280,6 +280,6 @@ void rollback_lock_file(struct lock_file *lk) if (lk->fd >= 0) close(lk->fd); unlink_or_warn(lk->filename); + lk->filename[0] = 0; } - lk->filename[0] = 0; } -- 2.1.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 v7 30/38] resolve_symlink(): take a strbuf parameter
Change resolve_symlink() to take a strbuf rather than a string as
parameter. This simplifies the code and removes an arbitrary pathname
length restriction. It also means that lock_file's filename field no
longer needs to be initialized to a large size.
Helped-by: Torsten Bögershausen
Signed-off-by: Michael Haggerty
---
lockfile.c | 57 ++---
1 file changed, 22 insertions(+), 35 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index cc9b9cb..5f5bcff 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -109,58 +109,47 @@ static char *last_path_elm(char *p)
#define MAXDEPTH 5
/*
- * p = path that may be a symlink
- * s = full size of p
+ * path contains a path that might be a symlink.
*
- * If p is a symlink, attempt to overwrite p with a path to the real
- * file or directory (which may or may not exist), following a chain of
- * symlinks if necessary. Otherwise, leave p unmodified.
+ * If path is a symlink, attempt to overwrite it with a path to the
+ * real file or directory (which may or may not exist), following a
+ * chain of symlinks if necessary. Otherwise, leave path unmodified.
*
- * This is a best-effort routine. If an error occurs, p will either be
- * left unmodified or will name a different symlink in a symlink chain
- * that started with p's initial contents.
- *
- * Always returns p.
+ * This is a best-effort routine. If an error occurs, path will
+ * either be left unmodified or will name a different symlink in a
+ * symlink chain that started with the original path.
*/
-
-static char *resolve_symlink(char *p, size_t s)
+static void resolve_symlink(struct strbuf *path)
{
int depth = MAXDEPTH;
static struct strbuf link = STRBUF_INIT;
while (depth--) {
- if (strbuf_readlink(&link, p, strlen(p)) < 0)
+ if (strbuf_readlink(&link, path->buf, path->len) < 0)
break;
- if (is_absolute_path(link.buf)) {
+ if (is_absolute_path(link.buf))
/* absolute path simply replaces p */
- if (link.len < s)
- strcpy(p, link.buf);
- else {
- warning("%s: symlink too long", p);
- break;
- }
- } else {
+ strbuf_reset(path);
+ else {
/*
* link is a relative path, so replace the
* last element of p with it.
*/
- char *r = (char *)last_path_elm(p);
- if (r - p + link.len < s)
- strcpy(r, link.buf);
- else {
- warning("%s: symlink too long", p);
- break;
- }
+ char *r = last_path_elm(path->buf);
+ strbuf_setlen(path, r - path->buf);
}
+
+ strbuf_addbuf(path, &link);
}
strbuf_reset(&link);
- return p;
}
/* 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_file_on_signal);
@@ -175,7 +164,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
lk->fd = -1;
lk->active = 0;
lk->owner = 0;
- strbuf_init(&lk->filename, PATH_MAX);
+ strbuf_init(&lk->filename, pathlen + LOCK_SUFFIX_LEN);
lk->next = lock_file_list;
lock_file_list = lk;
lk->on_list = 1;
@@ -185,11 +174,9 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
path);
}
- strbuf_addstr(&lk->filename, path);
- if (!(flags & LOCK_NODEREF)) {
- resolve_symlink(lk->filename.buf, lk->filename.alloc);
- strbuf_setlen(&lk->filename, strlen(lk->filename.buf));
- }
+ strbuf_add(&lk->filename, path, pathlen);
+ if (!(flags & LOCK_NODEREF))
+ resolve_symlink(&lk->filename);
strbuf_addstr(&lk->filename, LOCK_SUFFIX);
lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
if (lk->fd < 0) {
--
2.1.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 v7 34/38] lockfile.c: rename static functions
* remove_lock_file() -> remove_lock_files()
* remove_lock_file_on_signal() -> remove_lock_files_on_signal()
Suggested-by: Torsten Bögershausen
Signed-off-by: Michael Haggerty
---
lockfile.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index a1cc08a..0a8c3c8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -57,7 +57,7 @@
static struct lock_file *volatile lock_file_list;
-static void remove_lock_file(void)
+static void remove_lock_files(void)
{
pid_t me = getpid();
@@ -68,9 +68,9 @@ static void remove_lock_file(void)
}
}
-static void remove_lock_file_on_signal(int signo)
+static void remove_lock_files_on_signal(int signo)
{
- remove_lock_file();
+ remove_lock_files();
sigchain_pop(signo);
raise(signo);
}
@@ -146,8 +146,8 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
if (!lock_file_list) {
/* One-time initialization */
- sigchain_push_common(remove_lock_file_on_signal);
- atexit(remove_lock_file);
+ sigchain_push_common(remove_lock_files_on_signal);
+ atexit(remove_lock_files);
}
if (lk->active)
--
2.1.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 v7 31/38] trim_last_path_component(): replace last_path_elm()
Rewrite last_path_elm() to take a strbuf parameter and to trim off the
last path name element in place rather than returning a pointer to the
beginning of the last path name element. This simplifies the function
a bit and makes it integrate better with its caller, which is now also
strbuf-based. Rename the function accordingly and a bit less tersely.
Signed-off-by: Michael Haggerty
---
lockfile.c | 38 --
1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 5f5bcff..56ad7e8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -76,32 +76,28 @@ static void remove_lock_file_on_signal(int signo)
}
/*
- * p = absolute or relative path name
+ * path = absolute or relative path name
*
- * Return a pointer into p showing the beginning of the last path name
- * element. If p is empty or the root directory ("/"), just return p.
+ * Remove the last path name element from path (leaving the preceding
+ * "/", if any). If path is empty or the root directory ("/"), set
+ * path to the empty string.
*/
-static char *last_path_elm(char *p)
+static void trim_last_path_component(struct strbuf *path)
{
- /* r starts pointing to null at the end of the string */
- char *r = strchr(p, '\0');
-
- if (r == p)
- return p; /* just return empty string */
-
- r--; /* back up to last non-null character */
+ int i = path->len;
/* back up past trailing slashes, if any */
- while (r > p && *r == '/')
- r--;
+ while (i && path->buf[i - 1] == '/')
+ i--;
/*
-* then go backwards until I hit a slash, or the beginning of
-* the string
+* then go backwards until a slash, or the beginning of the
+* string
*/
- while (r > p && *(r-1) != '/')
- r--;
- return r;
+ while (i && path->buf[i - 1] != '/')
+ i--;
+
+ strbuf_setlen(path, i);
}
@@ -131,14 +127,12 @@ static void resolve_symlink(struct strbuf *path)
if (is_absolute_path(link.buf))
/* absolute path simply replaces p */
strbuf_reset(path);
- else {
+ else
/*
* link is a relative path, so replace the
* last element of p with it.
*/
- char *r = last_path_elm(path->buf);
- strbuf_setlen(path, r - path->buf);
- }
+ trim_last_path_component(path);
strbuf_addbuf(path, &link);
}
--
2.1.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 v7 33/38] Rename LOCK_NODEREF to LOCK_NO_DEREF
This makes it harder to misread the name as LOCK_NODE_REF.
Suggested-by: Torsten Bögershausen
Signed-off-by: Michael Haggerty
---
Documentation/technical/api-lockfile.txt | 4 ++--
cache.h | 2 +-
lockfile.c | 2 +-
refs.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt
b/Documentation/technical/api-lockfile.txt
index aa7d822..a3cb69b 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -110,11 +110,11 @@ Flags
The following flags can be passed to `hold_lock_file_for_update` or
`hold_lock_file_for_append`:
-LOCK_NODEREF::
+LOCK_NO_DEREF::
Usually symbolic links in the destination path are resolved
and the lockfile is created by adding ".lock" to the resolved
- path. If `LOCK_NODEREF` is set, then the lockfile is created
+ path. If `LOCK_NO_DEREF` is set, then the lockfile is created
by adding ".lock" to the path argument itself. This option is
used, for example, when locking a symbolic reference, which
for backwards-compatibility reasons can be a symbolic link
diff --git a/cache.h b/cache.h
index 414e93c..7ea4e81 100644
--- a/cache.h
+++ b/cache.h
@@ -583,7 +583,7 @@ struct lock_file {
struct strbuf filename;
};
#define LOCK_DIE_ON_ERROR 1
-#define LOCK_NODEREF 2
+#define LOCK_NO_DEREF 2
extern int unable_to_lock_error(const char *path, int err);
extern void unable_to_lock_message(const char *path, int err,
struct strbuf *buf);
diff --git a/lockfile.c b/lockfile.c
index cf7f4d0..a1cc08a 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -169,7 +169,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
}
strbuf_add(&lk->filename, path, pathlen);
- if (!(flags & LOCK_NODEREF))
+ if (!(flags & LOCK_NO_DEREF))
resolve_symlink(&lk->filename);
strbuf_addstr(&lk->filename, LOCK_SUFFIX);
lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
diff --git a/refs.c b/refs.c
index 598f4eb..c10eaff 100644
--- a/refs.c
+++ b/refs.c
@@ -2192,7 +2192,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
lflags = 0;
if (flags & REF_NODEREF) {
refname = orig_refname;
- lflags |= LOCK_NODEREF;
+ lflags |= LOCK_NO_DEREF;
}
lock->ref_name = xstrdup(refname);
lock->orig_ref_name = xstrdup(orig_refname);
--
2.1.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 v7 20/38] api-lockfile: document edge cases
* Document the behavior of commit_lock_file() when it fails, namely that it rolls back the lock_file object and sets errno appropriately. * Document the behavior of rollback_lock_file() when called for a lock_file object that has already been committed or rolled back, namely that it is a NOOP. Signed-off-by: Michael Haggerty --- Documentation/technical/api-lockfile.txt | 20 ++-- 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index d3bf940..9805da0 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -100,6 +100,10 @@ unable_to_lock_die:: Emit an appropriate error message and `die()`. +Similarly, `commit_lock_file` and `close_lock_file` return 0 on +success. On failure they set `errno` appropriately, do their best to +roll back the lockfile, and return -1. + Flags - @@ -144,18 +148,22 @@ 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 + `hold_lock_file_for_append`, close the file descriptor, and rename the lockfile to its final destination. Return 0 upon - success or a negative value on failure to `close(2)` or - `rename(2)`. It is a bug to call `commit_lock_file()` for a - `lock_file` object that is not currently locked. + success. On failure, roll back the lock file and return -1, + with `errno` set to the value from the failing call to + `close(2)` or `rename(2)`. It is a bug to call + `commit_lock_file` for a `lock_file` object that is not + currently locked. 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 - remove the lockfile. + 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. close_lock_file:: @@ -163,7 +171,7 @@ close_lock_file:: earlier call to `hold_lock_file_for_update` or `hold_lock_file_for_append`, and close the file descriptor. Return 0 upon success. On failure to `close(2)`, return a - negative value and rollback the lock file. Usually + negative value and roll back the lock file. Usually `commit_lock_file` or `rollback_lock_file` should eventually be called if `close_lock_file` succeeds. -- 2.1.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 v7 23/38] lockfile: avoid transitory invalid states
Because remove_lock_file() can be called any time by the signal
handler, it is important that any lock_file objects that are in the
lock_file_list are always in a valid state. And since lock_file
objects are often reused (but are never removed from lock_file_list),
that means we have to be careful whenever mutating a lock_file object
to always keep it in a well-defined state.
This was formerly not the case, because part of the state was encoded
by setting lk->filename to the empty string vs. a valid filename. It
is wrong to assume that this string can be updated atomically; for
example, even
strcpy(lk->filename, value)
is unsafe. But the old code was even more reckless; for example,
strcpy(lk->filename, path);
if (!(flags & LOCK_NODEREF))
resolve_symlink(lk->filename, max_path_len);
strcat(lk->filename, ".lock");
During the call to resolve_symlink(), lk->filename contained the name
of the file that was being locked, not the name of the lockfile. If a
signal were raised during that interval, then the signal handler would
have deleted the valuable file!
We could probably continue to use the filename field to encode the
state by being careful to write characters 1..N-1 of the filename
first, and then overwrite the NUL at filename[0] with the first
character of the filename, but that would be awkward and error-prone.
So, instead of using the filename field to determine whether the
lock_file object is active, add a new field "lock_file::active" for
this purpose. Be careful to set this field only when filename really
contains the name of a file that should be deleted on cleanup.
Helped-by: Johannes Sixt
Signed-off-by: Michael Haggerty
---
cache.h | 1 +
lockfile.c | 37 ++---
read-cache.c | 1 +
3 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index 24891a8..8e25fce 100644
--- a/cache.h
+++ b/cache.h
@@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int
flags, const struct
struct lock_file {
struct lock_file *next;
+ volatile sig_atomic_t active;
int fd;
pid_t owner;
char on_list;
diff --git a/lockfile.c b/lockfile.c
index 728ce49..d35ac44 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -23,7 +23,7 @@
* used to prevent a forked process from closing a lockfile created by
* its parent.
*
- * A lock_file object can be in several states:
+ * 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
@@ -32,19 +32,27 @@
*
* - Locked, lockfile open (after hold_lock_file_for_update(),
* hold_lock_file_for_append(), or reopen_lock_file()). In this
- * state, the lockfile exists, filename holds the filename of the
- * lockfile, fd holds a file descriptor open for writing to the
- * lockfile, and owner holds the PID of the process that locked the
- * file.
+ * 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
+ * - 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(), rollback_lock_file(), a
- * failed attempt to lock, or a failed close_lock_file()). In this
- * state, filename[0] == '\0' and fd is -1. The object is left
- * registered in the lock_file_list, and on_list is set.
+ * failed attempt to lock, or a failed close_lock_file()). In this
+ * state:
+ * - active is unset
+ * - filename[0] == '\0' (usually, though there are transitory states
+ * in which this condition doesn't hold). Client code should *not*
+ * rely on this fact!
+ * - fd is -1
+ * - the object is left registered in the lock_file_list, and
+ * on_list is set.
*/
static struct lock_file *lock_file_list;
@@ -175,9 +183,13 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
atexit(remove_lock_file);
}
+ if (lk->active)
+ die("BUG: cannot lock_file(\"%s\") using active struct
lock_file",
+ path);
if (!lk->on_list) {
/* Initialize *lk and add it to lock_file_list: */
lk->fd = -1;
+ lk->active = 0;
lk->owner = 0;
lk->filename[0] = 0;
lk->next = lock_file_list;
@@ -199,6 +211,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return -1;
}
lk->owner = getpid();
+ lk->active = 1;
if (adjust_shared_perm(lk->filename)) {
int save_errno = errno;
error("cannot f
[PATCH v7 24/38] struct lock_file: declare some fields volatile
The function remove_lock_file_on_signal() is used as a signal handler.
It is not realistic to make the signal handler conform strictly to the
C standard, which is very restrictive about what a signal handler is
allowed to do. But let's increase the likelihood that it will work:
The lock_file_list global variable and several fields from struct
lock_file are used by the signal handler. Declare those values
"volatile" to (1) force the main process to write the values to RAM
promptly, and (2) prevent updates to these fields from being reordered
in a way that leaves an opportunity for a jump to the signal handler
while the object is in an inconsistent state.
We don't mark the filename field volatile because that would prevent
the use of strcpy(), and it is anyway unlikely that a compiler
re-orders a strcpy() call across other expressions. So in practice it
should be possible to get away without "volatile" in the "filename"
case.
Suggested-by: Johannes Sixt
Signed-off-by: Michael Haggerty
---
cache.h| 6 +++---
lockfile.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/cache.h b/cache.h
index 8e25fce..c2ea6f1 100644
--- a/cache.h
+++ b/cache.h
@@ -575,10 +575,10 @@ extern int refresh_index(struct index_state *, unsigned
int flags, const struct
#define LOCK_SUFFIX_LEN 5
struct lock_file {
- struct lock_file *next;
+ struct lock_file *volatile next;
volatile sig_atomic_t active;
- int fd;
- pid_t owner;
+ volatile int fd;
+ volatile pid_t owner;
char on_list;
char filename[PATH_MAX];
};
diff --git a/lockfile.c b/lockfile.c
index d35ac44..89043f5 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -55,7 +55,7 @@
* on_list is set.
*/
-static struct lock_file *lock_file_list;
+static struct lock_file *volatile lock_file_list;
static void remove_lock_file(void)
{
--
2.1.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 v7 35/38] get_locked_file_path(): new function
Add a function to return the path of the file that is locked by a
lock_file object. This reduces the knowledge that callers have to have
about the lock_file layout.
Suggested-by: Ronnie Sahlberg
Signed-off-by: Michael Haggerty
---
Documentation/technical/api-lockfile.txt | 5 +
cache.h | 1 +
lockfile.c | 9 +
refs.c | 4 +---
4 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt
b/Documentation/technical/api-lockfile.txt
index a3cb69b..d4484d1 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -143,6 +143,11 @@ hold_lock_file_for_append::
the existing contents of the file (if any) to the lockfile and
position its write pointer at the end of the file.
+get_locked_file_path::
+
+ Return the path of the file that is locked by the specified
+ lock_file object. The caller must free the memory.
+
commit_lock_file::
Take a pointer to the `struct lock_file` initialized with an
diff --git a/cache.h b/cache.h
index 7ea4e81..d19e57f 100644
--- a/cache.h
+++ b/cache.h
@@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int
err,
extern NORETURN void unable_to_lock_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
+extern char *get_locked_file_path(struct lock_file *);
extern int commit_lock_file_to(struct lock_file *, const char *path);
extern int commit_lock_file(struct lock_file *);
extern int reopen_lock_file(struct lock_file *);
diff --git a/lockfile.c b/lockfile.c
index 0a8c3c8..c51c6ec 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -257,6 +257,15 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
return fd;
}
+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)
+ die("BUG: get_locked_file_path() called for malformed lock
object");
+ return xmemdupz(lk->filename.buf, lk->filename.len - LOCK_SUFFIX_LEN);
+}
+
int close_lock_file(struct lock_file *lk)
{
int fd = lk->fd;
diff --git a/refs.c b/refs.c
index c10eaff..e40c47e 100644
--- a/refs.c
+++ b/refs.c
@@ -2606,9 +2606,7 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
* loose. The loose file name is the same as the
* lockfile name, minus ".lock":
*/
- char *loose_filename = xmemdupz(
- lock->lk->filename.buf,
- lock->lk->filename.len - LOCK_SUFFIX_LEN);
+ char *loose_filename = get_locked_file_path(lock->lk);
int err = unlink_or_warn(loose_filename);
free(loose_filename);
if (err && errno != ENOENT)
--
2.1.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 v7 26/38] try_merge_strategy(): use a statically-allocated lock_file object
Even the one lockfile object needn't be allocated each time the
function is called. Instead, define one statically-allocated
lock_file object and reuse it for every call.
Suggested-by: Jeff King
Signed-off-by: Michael Haggerty
---
builtin/merge.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 1ec3939..be07f27 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -656,14 +656,14 @@ static int try_merge_strategy(const char *strategy,
struct commit_list *common,
struct commit_list *remoteheads,
struct commit *head, const char *head_arg)
{
- struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+ static struct lock_file lock;
- hold_locked_index(lock, 1);
+ hold_locked_index(&lock, 1);
refresh_cache(REFRESH_QUIET);
if (active_cache_changed &&
- write_locked_index(&the_index, lock, COMMIT_LOCK))
+ write_locked_index(&the_index, &lock, COMMIT_LOCK))
return error(_("Unable to write index."));
- rollback_lock_file(lock);
+ rollback_lock_file(&lock);
if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
int clean, x;
@@ -695,13 +695,13 @@ static int try_merge_strategy(const char *strategy,
struct commit_list *common,
for (j = common; j; j = j->next)
commit_list_insert(j->item, &reversed);
- hold_locked_index(lock, 1);
+ hold_locked_index(&lock, 1);
clean = merge_recursive(&o, head,
remoteheads->item, reversed, &result);
if (active_cache_changed &&
- write_locked_index(&the_index, lock, COMMIT_LOCK))
+ write_locked_index(&the_index, &lock, COMMIT_LOCK))
die (_("unable to write %s"), get_index_file());
- rollback_lock_file(lock);
+ rollback_lock_file(&lock);
return clean ? 0 : 1;
} else {
return try_merge_command(strategy, xopts_nr, xopts,
--
2.1.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 v7 36/38] hold_lock_file_for_append(): restore errno before returning
Callers who don't pass LOCK_DIE_ON_ERROR might want to examine errno
to see what went wrong, so restore errno before returning.
In fact this function only has one caller, add_to_alternates_file(),
and it *does* use LOCK_DIE_ON_ERROR, but, you know, think of future
generations.
Signed-off-by: Michael Haggerty
---
lockfile.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/lockfile.c b/lockfile.c
index c51c6ec..b2f5d36 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -243,15 +243,22 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
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' for copying", path);
rollback_lock_file(lk);
- return error("cannot open '%s' for copying", path);
+ error("cannot open '%s' for copying", path);
+ errno = save_errno;
+ return -1;
}
} else if (copy_fd(orig_fd, fd)) {
+ int save_errno = errno;
+
if (flags & LOCK_DIE_ON_ERROR)
exit(128);
rollback_lock_file(lk);
+ errno = save_errno;
return -1;
}
return fd;
--
2.1.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 v7 17/38] commit_lock_file(): die() if called for unlocked lockfile object
It was previously a bug to call commit_lock_file() with a lock_file
object that was not active (an illegal access would happen within the
function). It was presumably never done, but this would be an easy
programming error to overlook. So before continuing, do a consistency
check that the lock_file object really is locked.
Helped-by: Johannes Sixt
Signed-off-by: Michael Haggerty
---
Documentation/technical/api-lockfile.txt | 3 ++-
lockfile.c | 3 +++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/technical/api-lockfile.txt
b/Documentation/technical/api-lockfile.txt
index 99830f3..6538610 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -147,7 +147,8 @@ commit_lock_file::
`hold_lock_file_for_append`, close the file descriptor and
rename the lockfile to its final destination. Return 0 upon
success or a negative value on failure to `close(2)` or
- `rename(2)`.
+ `rename(2)`. It is a bug to call `commit_lock_file()` for a
+ `lock_file` object that is not currently locked.
rollback_lock_file::
diff --git a/lockfile.c b/lockfile.c
index e148227..c897dd8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -301,6 +301,9 @@ int commit_lock_file(struct lock_file *lk)
{
char result_file[PATH_MAX];
+ if (!lk->filename[0])
+ die("BUG: attempt to commit unlocked object");
+
if (close_lock_file(lk))
return -1;
--
2.1.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 v7 38/38] lockfile.h: extract new header file for the functions in lockfile.c
Move the interface declaration for the functions in lockfile.c from cache.h to a new file, lockfile.h. Add #includes where necessary (and remove some redundant includes of cache.h by files that already include builtin.h). Move the documentation of the lock_file state diagram from lockfile.c to the new header file. Signed-off-by: Michael Haggerty --- builtin/add.c| 1 + builtin/apply.c | 1 + builtin/checkout-index.c | 2 +- builtin/checkout.c | 2 +- builtin/clone.c | 1 + builtin/commit.c | 1 + builtin/describe.c | 1 + builtin/diff.c | 1 + builtin/gc.c | 2 +- builtin/merge.c | 1 + builtin/mv.c | 2 +- builtin/read-tree.c | 1 + builtin/receive-pack.c | 1 + builtin/reflog.c | 2 +- builtin/reset.c | 1 + builtin/rm.c | 2 +- builtin/update-index.c | 1 + bundle.c | 1 + cache-tree.c | 1 + cache.h | 27 +--- config.c | 1 + credential-store.c | 1 + fast-import.c| 1 + fetch-pack.c | 1 + lockfile.c | 52 +- lockfile.h | 84 merge-recursive.c| 1 + merge.c | 1 + read-cache.c | 1 + refs.c | 1 + rerere.c | 1 + sequencer.c | 1 + sha1_file.c | 1 + shallow.c| 1 + test-scrap-cache-tree.c | 1 + 35 files changed, 118 insertions(+), 83 deletions(-) create mode 100644 lockfile.h diff --git a/builtin/add.c b/builtin/add.c index 352b85e..ae6d3e2 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -5,6 +5,7 @@ */ #include "cache.h" #include "builtin.h" +#include "lockfile.h" #include "dir.h" #include "pathspec.h" #include "exec_cmd.h" diff --git a/builtin/apply.c b/builtin/apply.c index 8714a88..69efb0e 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -7,6 +7,7 @@ * */ #include "cache.h" +#include "lockfile.h" #include "cache-tree.h" #include "quote.h" #include "blob.h" diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 05edd9e..383dccf 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -5,7 +5,7 @@ * */ #include "builtin.h" -#include "cache.h" +#include "lockfile.h" #include "quote.h" #include "cache-tree.h" #include "parse-options.h" diff --git a/builtin/checkout.c b/builtin/checkout.c index 8afdf2b..570bb09 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1,5 +1,5 @@ -#include "cache.h" #include "builtin.h" +#include "lockfile.h" #include "parse-options.h" #include "refs.h" #include "commit.h" diff --git a/builtin/clone.c b/builtin/clone.c index 3927edf..d3bf953 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -9,6 +9,7 @@ */ #include "builtin.h" +#include "lockfile.h" #include "parse-options.h" #include "fetch-pack.h" #include "refs.h" diff --git a/builtin/commit.c b/builtin/commit.c index f55e809..c230018 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -6,6 +6,7 @@ */ #include "cache.h" +#include "lockfile.h" #include "cache-tree.h" #include "color.h" #include "dir.h" diff --git a/builtin/describe.c b/builtin/describe.c index ee6a3b9..9103193 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "commit.h" #include "tag.h" #include "refs.h" diff --git a/builtin/diff.c b/builtin/diff.c index 0f247d2..4326fa5 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -4,6 +4,7 @@ * Copyright (c) 2006 Junio C Hamano */ #include "cache.h" +#include "lockfile.h" #include "color.h" #include "commit.h" #include "blob.h" diff --git a/builtin/gc.c b/builtin/gc.c index ced1456..005adbe 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -11,7 +11,7 @@ */ #include "builtin.h" -#include "cache.h" +#include "lockfile.h" #include "parse-options.h" #include "run-command.h" #include "sigchain.h" diff --git a/builtin/merge.c b/builtin/merge.c index be07f27..4513fad 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -9,6 +9,7 @@ #include "cache.h" #include "parse-options.h" #include "builtin.h" +#include "lockfile.h" #include "run-command.h" #include "diff.h" #include "refs.h" diff --git a/builtin/mv.c b/builtin/mv.c index 8883baa..563d05b 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -3,8 +3,8 @@ * * Copyright (C) 2006 Johannes Schindelin */ -#include "cache.h" #include "builtin.h" +#include "lockfile.h" #include "dir.h" #include "cache-tree.h" #include "string-list.h" diff --git a/builtin/read-tree.c b/builtin/read-tree.c index e7e1c33..43b47f7 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -5,6 +5,7 @@ */ #include "cache.h" +#include "lockfile.h" #include "object.h" #include "tree.h" #include "tree-walk
[PATCH v7 21/38] dump_marks(): remove a redundant call to rollback_lock_file()
When commit_lock_file() fails, it now always calls
rollback_lock_file() internally, so there is no need to call that
function here.
Signed-off-by: Michael Haggerty
Reviewed-by: Jonathan Nieder
---
fast-import.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 96b0f42..783c684 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1832,10 +1832,8 @@ static void dump_marks(void)
}
if (commit_lock_file(&mark_lock)) {
- int saved_errno = errno;
- rollback_lock_file(&mark_lock);
failure |= error("Unable to commit marks file %s: %s",
- export_marks_file, strerror(saved_errno));
+ export_marks_file, strerror(errno));
return;
}
}
--
2.1.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 v7 27/38] commit_lock_file(): use a strbuf to manage temporary space
Avoid relying on the filename length restrictions that are currently
checked by lock_file().
Signed-off-by: Michael Haggerty
---
lockfile.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 89043f5..1dd118f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -319,7 +319,8 @@ int reopen_lock_file(struct lock_file *lk)
int commit_lock_file(struct lock_file *lk)
{
- char result_file[PATH_MAX];
+ static struct strbuf result_file = STRBUF_INIT;
+ int err;
if (!lk->active)
die("BUG: attempt to commit unlocked object");
@@ -327,11 +328,12 @@ int commit_lock_file(struct lock_file *lk)
if (close_lock_file(lk))
return -1;
- strcpy(result_file, lk->filename);
/* remove ".lock": */
- result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
-
- if (rename(lk->filename, result_file)) {
+ strbuf_add(&result_file, lk->filename,
+ strlen(lk->filename) - LOCK_SUFFIX_LEN);
+ err = rename(lk->filename, result_file.buf);
+ strbuf_reset(&result_file);
+ if (err) {
int save_errno = errno;
rollback_lock_file(lk);
errno = save_errno;
--
2.1.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 v7 32/38] Extract a function commit_lock_file_to()
commit_locked_index(), when writing to an alternate index file, duplicates (poorly) the code in commit_lock_file(). And anyway, it shouldn't have to know so much about the internal workings of lockfile objects. So extract a new function commit_lock_file_to() that does the work common to the two functions, and call it from both commit_lock_file() and commit_locked_index(). Signed-off-by: Michael Haggerty --- Documentation/technical/api-lockfile.txt | 34 --- cache.h | 1 + lockfile.c | 40 +--- read-cache.c | 13 +++ 4 files changed, 50 insertions(+), 38 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index 9805da0..aa7d822 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -49,14 +49,14 @@ The caller: When finished writing, the caller can: * Close the file descriptor and rename the lockfile to its final - destination by calling `commit_lock_file`. + 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`, - `rollback_lock_file`, or `reopen_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 @@ -64,20 +64,19 @@ 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`, -`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler -will close and remove the lockfile, rolling back any uncommitted -changes. +`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)` yourself! Otherwise the `struct lock_file` structure would still think that the -file descriptor needs to be closed, and a later call to -`commit_lock_file` or `rollback_lock_file` or program exit would +file descriptor needs to be closed, and a commit or rollback would result in duplicate calls to `close(2)`. Worse yet, if you `close(2)` and then later open another file descriptor for a completely different -purpose, then a call to `commit_lock_file` or `rollback_lock_file` -might close that unrelated file descriptor. +purpose, then a commit or rollback might close that unrelated file +descriptor. Error handling @@ -100,9 +99,9 @@ unable_to_lock_die:: Emit an appropriate error message and `die()`. -Similarly, `commit_lock_file` and `close_lock_file` return 0 on -success. On failure they set `errno` appropriately, do their best to -roll back the lockfile, and return -1. +Similarly, `commit_lock_file`, `commit_lock_file_to`, and +`close_lock_file` return 0 on success. On failure they set `errno` +appropriately, do their best to roll back the lockfile, and return -1. Flags @@ -156,6 +155,12 @@ commit_lock_file:: `commit_lock_file` for a `lock_file` object that is not currently locked. +commit_lock_file_to:: + + Like `commit_lock_file()`, except that it takes an explicit + `path` argument to which the lockfile should be renamed. The + `path` must be on the same filesystem as the lock file. + rollback_lock_file:: Take a pointer to the `struct lock_file` initialized with an @@ -172,8 +177,9 @@ close_lock_file:: `hold_lock_file_for_append`, and close the file descriptor. Return 0 upon success. On failure to `close(2)`, return a negative value and roll back the lock file. Usually - `commit_lock_file` or `rollback_lock_file` should eventually - be called if `close_lock_file` succeeds. + `commit_lock_file`, `commit_lock_file_to`, or + `rollback_lock_file` should eventually be called if + `close_lock_file` succeeds. reopen_lock_file:: diff --git a/cache.h b/cache.h index f81d95f..414e93c 100644 --- a/cache.h +++ b/cache.h @@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int err, extern NORETURN void unable_to_lock_die(const char *path, int err); extern int hold_lock_file_for_update(struct lock_file *, const char *path, int); extern int hold_lock_file_for_append(struct lock_file *, const char *path, int); +extern int commit_lock_file_to(struct lock_file *, const char *path); extern int commit_lock_file(struct lock_file *);
[PATCH v7 25/38] try_merge_strategy(): remove redundant lock_file allocation
By the time the "if" block is entered, the lock_file instance from the
main function block is no longer in use, so re-use that one instead of
allocating a second one.
Note that the "lock" variable in the "if" block shadowed the "lock"
variable at function scope, so the only change needed is to remove the
inner definition.
Signed-off-by: Michael Haggerty
---
builtin/merge.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index dff043d..1ec3939 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -668,7 +668,6 @@ static int try_merge_strategy(const char *strategy, struct
commit_list *common,
if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
int clean, x;
struct commit *result;
- struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
struct commit_list *reversed = NULL;
struct merge_options o;
struct commit_list *j;
--
2.1.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 v7 37/38] Move read_index() definition to read-cache.c
lockfile.c contains the general API for locking any file. Code
specifically about the index file doesn't belong here.
Signed-off-by: Michael Haggerty
---
lockfile.c | 8
read-cache.c | 8
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index b2f5d36..63f4e94 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -340,14 +340,6 @@ int commit_lock_file(struct lock_file *lk)
return err;
}
-int hold_locked_index(struct lock_file *lk, int die_on_error)
-{
- return hold_lock_file_for_update(lk, get_index_file(),
-die_on_error
-? LOCK_DIE_ON_ERROR
-: 0);
-}
-
void rollback_lock_file(struct lock_file *lk)
{
if (!lk->active)
diff --git a/read-cache.c b/read-cache.c
index e887e23..9f137e7 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1367,6 +1367,14 @@ static int read_index_extension(struct index_state
*istate,
return 0;
}
+int hold_locked_index(struct lock_file *lk, int die_on_error)
+{
+ return hold_lock_file_for_update(lk, get_index_file(),
+die_on_error
+? LOCK_DIE_ON_ERROR
+: 0);
+}
+
int read_index(struct index_state *istate)
{
return read_index_from(istate, get_index_file());
--
2.1.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 v7 29/38] resolve_symlink(): use a strbuf for internal scratch space
Aside from shortening and simplifying the code, this removes another
place where the path name length is arbitrarily limited.
Signed-off-by: Michael Haggerty
---
lockfile.c | 33 -
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 85c8648..cc9b9cb 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -126,44 +126,35 @@ static char *last_path_elm(char *p)
static char *resolve_symlink(char *p, size_t s)
{
int depth = MAXDEPTH;
+ static struct strbuf link = STRBUF_INIT;
while (depth--) {
- char link[PATH_MAX];
- int link_len = readlink(p, link, sizeof(link));
- if (link_len < 0) {
- /* not a symlink anymore */
- return p;
- }
- else if (link_len < sizeof(link))
- /* readlink() never null-terminates */
- link[link_len] = '\0';
- else {
- warning("%s: symlink too long", p);
- return p;
- }
+ if (strbuf_readlink(&link, p, strlen(p)) < 0)
+ break;
- if (is_absolute_path(link)) {
+ if (is_absolute_path(link.buf)) {
/* absolute path simply replaces p */
- if (link_len < s)
- strcpy(p, link);
+ if (link.len < s)
+ strcpy(p, link.buf);
else {
warning("%s: symlink too long", p);
- return p;
+ break;
}
} else {
/*
-* link is a relative path, so I must replace the
+* link is a relative path, so replace the
* last element of p with it.
*/
char *r = (char *)last_path_elm(p);
- if (r - p + link_len < s)
- strcpy(r, link);
+ if (r - p + link.len < s)
+ strcpy(r, link.buf);
else {
warning("%s: symlink too long", p);
- return p;
+ break;
}
}
}
+ strbuf_reset(&link);
return p;
}
--
2.1.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 v7 28/38] Change lock_file::filename into a strbuf
For now, we still make sure to allocate at least PATH_MAX characters
for the strbuf because resolve_symlink() doesn't know how to expand
the space for its return value. (That will be fixed in a moment.)
Another alternative would be to just use a strbuf as scratch space in
lock_file() but then store a pointer to the naked string in struct
lock_file. But lock_file objects are often reused. By reusing the
same strbuf, we can avoid having to reallocate the string most times
when a lock_file object is reused.
Helped-by: Torsten Bögershausen
Signed-off-by: Michael Haggerty
---
builtin/commit.c | 12 ++--
builtin/reflog.c | 2 +-
cache.h | 2 +-
config.c | 14 +++---
lockfile.c | 53 -
read-cache.c | 4 ++--
refs.c | 6 +++---
shallow.c| 6 +++---
8 files changed, 47 insertions(+), 52 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 70f5935..f55e809 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -341,7 +341,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, 1);
+ setenv(INDEX_ENVIRONMENT, index_lock.filename.buf, 1);
if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
die(_("interactive add failed"));
@@ -352,7 +352,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);
+ read_cache_from(index_lock.filename.buf);
if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
if (reopen_lock_file(&index_lock) < 0)
die(_("unable to write index file"));
@@ -362,7 +362,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;
+ return index_lock.filename.buf;
}
/*
@@ -385,7 +385,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;
+ return index_lock.filename.buf;
}
/*
@@ -472,9 +472,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);
+ read_cache_from(false_lock.filename.buf);
- return false_lock.filename;
+ return false_lock.filename.buf;
}
static int run_status(FILE *fp, const char *index_file, const char *prefix,
int nowarn,
diff --git a/builtin/reflog.c b/builtin/reflog.c
index e8a8fb1..7c78b15 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
write_str_in_full(lock->lock_fd, "\n") != 1 ||
close_ref(lock) < 0)) {
status |= error("Couldn't write %s",
- lock->lk->filename);
+ lock->lk->filename.buf);
unlink(newlog_path);
} else if (rename(newlog_path, log_file)) {
status |= error("cannot rename %s to %s",
diff --git a/cache.h b/cache.h
index c2ea6f1..f81d95f 100644
--- a/cache.h
+++ b/cache.h
@@ -580,7 +580,7 @@ struct lock_file {
volatile int fd;
volatile pid_t owner;
char on_list;
- char filename[PATH_MAX];
+ struct strbuf filename;
};
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
diff --git a/config.c b/config.c
index 123ed29..2110779 100644
--- a/config.c
+++ b/config.c
@@ -2024,9 +2024,9 @@ int git_config_set_multivar_in_file(const char
*config_filename,
MAP_PRIVATE, in_fd, 0);
close(in_fd);
- if (chmod(lock->filename, st.st_mode & 0) < 0) {
+ if (chmod(lock->filename.buf, st.st_mode & 0) < 0) {
error("chmod on %s failed: %s",
- lock->filename, strerror(errno));
+ lock->filename.buf, strerror(errno));
ret = CONFIG_NO_WRITE;
goto out_free;
}
@
[PATCH v7 16/38] commit_lock_file(): inline temporary variable
Signed-off-by: Michael Haggerty
---
lockfile.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index f8205f6..e148227 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -300,12 +300,14 @@ int reopen_lock_file(struct lock_file *lk)
int commit_lock_file(struct lock_file *lk)
{
char result_file[PATH_MAX];
- size_t i;
+
if (close_lock_file(lk))
return -1;
+
strcpy(result_file, lk->filename);
- i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */
- result_file[i] = 0;
+ /* remove ".lock": */
+ result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
+
if (rename(lk->filename, result_file))
return -1;
lk->filename[0] = 0;
--
2.1.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 v7 08/38] hold_lock_file_for_append(): release lock on errors
If there is an error copying the old contents to the lockfile, roll
back the lockfile before exiting so that the lockfile is not held
until process cleanup.
Signed-off-by: Michael Haggerty
Reviewed-by: Jonathan Nieder
---
lockfile.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index d74de8d..f4ce79b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -219,13 +219,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
if (errno != ENOENT) {
if (flags & LOCK_DIE_ON_ERROR)
die("cannot open '%s' for copying", path);
- close(fd);
+ rollback_lock_file(lk);
return error("cannot open '%s' for copying", path);
}
} else if (copy_fd(orig_fd, fd)) {
if (flags & LOCK_DIE_ON_ERROR)
exit(128);
- close(fd);
+ rollback_lock_file(lk);
return -1;
}
return fd;
--
2.1.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 v7 19/38] commit_lock_file(): rollback lock file on failure to rename
If rename() fails, call rollback_lock_file() to delete the lock file
(in case it is still present) and reset the filename field to the
empty string so that the lockfile object is left in a valid state.
Signed-off-by: Michael Haggerty
---
lockfile.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/lockfile.c b/lockfile.c
index 1d18c67..728ce49 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -318,8 +318,13 @@ int commit_lock_file(struct lock_file *lk)
/* remove ".lock": */
result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;
- if (rename(lk->filename, result_file))
+ if (rename(lk->filename, result_file)) {
+ int save_errno = errno;
+ rollback_lock_file(lk);
+ errno = save_errno;
return -1;
+ }
+
lk->filename[0] = 0;
return 0;
}
--
2.1.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 v7 10/38] lockfile.c: document the various states of lock_file objects
Document the valid states of lock_file objects, how they get into each state, and how the state is encoded in the object's fields. Signed-off-by: Michael Haggerty Reviewed-by: Ronnie Sahlberg --- lockfile.c | 42 ++ 1 file changed, 42 insertions(+) diff --git a/lockfile.c b/lockfile.c index 81143e5..2680dc9 100644 --- a/lockfile.c +++ b/lockfile.c @@ -4,6 +4,48 @@ #include "cache.h" #include "sigchain.h" +/* + * File write-locks as used by Git. + * + * For an overview of how to use the lockfile API, please see + * + * Documentation/technical/api-lockfile.txt + * + * 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. + * + * A lockfile is owned by the process that created it. The lock_file + * object has an "owner" field that records its owner. This field is + * used to prevent a forked process from closing a lockfile created by + * its parent. + * + * A lock_file object can be in several states: + * + * - 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 the 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, filename holds the filename of the + * lockfile, fd holds a file descriptor open for writing to the + * lockfile, and owner holds the PID of the process that locked the + * file. + * + * - Locked, lockfile closed (after close_lock_file()). Same as the + * previous state, except that the lockfile is closed and fd is -1. + * + * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a + * failed attempt to lock). In this state, filename[0] == '\0' and + * fd is -1. The object is left registered in the lock_file_list, + * and on_list is set. + */ + static struct lock_file *lock_file_list; static void remove_lock_file(void) -- 2.1.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 v7 22/38] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()
After commit_lock_file() is called, then the lock_file object is
necessarily either committed or rolled back. So there is no need to
call rollback_lock_file() again in either of these cases.
Signed-off-by: Michael Haggerty
---
config.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/config.c b/config.c
index a677eb6..123ed29 100644
--- a/config.c
+++ b/config.c
@@ -2083,6 +2083,7 @@ int git_config_set_multivar_in_file(const char
*config_filename,
if (commit_lock_file(lock) < 0) {
error("could not commit config file %s", config_filename);
ret = CONFIG_NO_WRITE;
+ lock = NULL;
goto out_free;
}
--
2.1.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 v7 14/38] lock_file(): exit early if lockfile cannot be opened
This is a bit easier to read than the old version, which nested part
of the non-error code in an "if" block.
Signed-off-by: Michael Haggerty
Reviewed-by: Ronnie Sahlberg
Reviewed-by: Jonathan Nieder
---
lockfile.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 23847fc..a8f32e5 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -197,19 +197,18 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
resolve_symlink(lk->filename, max_path_len);
strcat(lk->filename, LOCK_SUFFIX);
lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
- if (0 <= lk->fd) {
- lk->owner = getpid();
- if (adjust_shared_perm(lk->filename)) {
- int save_errno = errno;
- error("cannot fix permission bits on %s",
- lk->filename);
- rollback_lock_file(lk);
- errno = save_errno;
- return -1;
- }
- }
- else
+ if (lk->fd < 0) {
lk->filename[0] = 0;
+ return -1;
+ }
+ lk->owner = getpid();
+ if (adjust_shared_perm(lk->filename)) {
+ int save_errno = errno;
+ error("cannot fix permission bits on %s", lk->filename);
+ rollback_lock_file(lk);
+ errno = save_errno;
+ return -1;
+ }
return lk->fd;
}
--
2.1.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 v7 09/38] lock_file(): always initialize and register lock_file object
The purpose of this patch is to make the state diagram for lock_file
objects simpler and deterministic.
If locking fails, lock_file() sometimes leaves the lock_file object
partly initialized, but sometimes not. It sometimes registers the
object in lock_file_list, but sometimes not. This makes the state
diagram for lock_file objects effectively indeterministic and hard to
reason about. A future patch will also change the filename field into
a strbuf, which needs more involved initialization, so it will become
even more important that the state of a lock_file object is
well-defined after a failed attempt to lock.
The ambiguity doesn't currently have any ill effects, because
lock_file objects cannot be removed from the lock_file_list anyway.
But to make it easier to document and reason about the code, make this
behavior inconsistent: *always* initialize the lock_file object and
*always* register it in lock_file_list the first time it is used,
regardless of whether an error occurs.
While we're at it, make sure that all of the lock_file fields are
initialized to values appropriate for an unlocked object; the caller
is only responsible for making sure that on_list is set to zero before
the first time it is used.
Signed-off-by: Michael Haggerty
---
lockfile.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index f4ce79b..81143e5 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -129,6 +129,22 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
*/
static const size_t max_path_len = sizeof(lk->filename) - 5;
+ if (!lock_file_list) {
+ /* One-time initialization */
+ sigchain_push_common(remove_lock_file_on_signal);
+ atexit(remove_lock_file);
+ }
+
+ if (!lk->on_list) {
+ /* Initialize *lk and add it to lock_file_list: */
+ lk->fd = -1;
+ lk->owner = 0;
+ lk->filename[0] = 0;
+ lk->next = lock_file_list;
+ lock_file_list = lk;
+ lk->on_list = 1;
+ }
+
if (strlen(path) >= max_path_len) {
errno = ENAMETOOLONG;
return -1;
@@ -139,16 +155,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
strcat(lk->filename, ".lock");
lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
if (0 <= lk->fd) {
- if (!lock_file_list) {
- sigchain_push_common(remove_lock_file_on_signal);
- atexit(remove_lock_file);
- }
lk->owner = getpid();
- if (!lk->on_list) {
- lk->next = lock_file_list;
- lock_file_list = lk;
- lk->on_list = 1;
- }
if (adjust_shared_perm(lk->filename)) {
int save_errno = errno;
error("cannot fix permission bits on %s",
--
2.1.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 v7 18/38] close_lock_file(): if close fails, roll back
If closing an open lockfile fails, then we cannot be sure of the
contents of the lockfile, so there is nothing sensible to do but
delete it. This change also insures that the lock_file object is left
in a defined state in this error path (namely, unlocked).
The only caller that is ultimately affected by this change is
try_merge_strategy() -> write_locked_index(), which can call
close_lock_file() via various execution paths. This caller uses a
static lock_file object which previously could have been reused after
a failed close_lock_file() even though it was still in locked state.
This change causes the lock_file object to be unlocked on failure,
thus fixing this error-handling path.
Signed-off-by: Michael Haggerty
---
Documentation/technical/api-lockfile.txt | 7 ---
lockfile.c | 28 ++--
2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt
b/Documentation/technical/api-lockfile.txt
index 6538610..d3bf940 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -162,9 +162,10 @@ 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`, and close the file descriptor.
- Return 0 upon success or a negative value on failure to
- close(2). Usually `commit_lock_file` or `rollback_lock_file`
- should be called after `close_lock_file`.
+ Return 0 upon success. On failure to `close(2)`, return a
+ negative value and rollback the lock file. Usually
+ `commit_lock_file` or `rollback_lock_file` should eventually
+ be called if `close_lock_file` succeeds.
reopen_lock_file::
diff --git a/lockfile.c b/lockfile.c
index c897dd8..1d18c67 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -37,13 +37,14 @@
* lockfile, and owner holds the PID of the process that locked the
* file.
*
- * - Locked, lockfile closed (after close_lock_file()). Same as the
- * previous state, except that the lockfile is closed and fd is -1.
+ * - 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(), rollback_lock_file(), or a
- * failed attempt to lock). In this state, filename[0] == '\0' and
- * fd is -1. The object is left registered in the lock_file_list,
- * and on_list is set.
+ * - Unlocked (after commit_lock_file(), rollback_lock_file(), a
+ * failed attempt to lock, or a failed close_lock_file()). In this
+ * state, filename[0] == '\0' and fd is -1. The object is left
+ * registered in the lock_file_list, and on_list is set.
*/
static struct lock_file *lock_file_list;
@@ -284,7 +285,13 @@ int close_lock_file(struct lock_file *lk)
return 0;
lk->fd = -1;
- return close(fd);
+ if (close(fd)) {
+ int save_errno = errno;
+ rollback_lock_file(lk);
+ errno = save_errno;
+ return -1;
+ }
+ return 0;
}
int reopen_lock_file(struct lock_file *lk)
@@ -330,7 +337,8 @@ void rollback_lock_file(struct lock_file *lk)
if (!lk->filename[0])
return;
- close_lock_file(lk);
- unlink_or_warn(lk->filename);
- lk->filename[0] = 0;
+ if (!close_lock_file(lk)) {
+ unlink_or_warn(lk->filename);
+ lk->filename[0] = 0;
+ }
}
--
2.1.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 v7 07/38] lockfile: unlock file if lockfile permissions cannot be adjusted
If the call to adjust_shared_perm() fails, lock_file returns -1, which
to the caller looks like any other failure to lock the file. So in
this case, roll back the lockfile before returning so that the lock
file is deleted immediately and the lockfile object is left in a
predictable state (namely, unlocked). Previously, the lockfile was
retained until process cleanup in this situation.
Signed-off-by: Michael Haggerty
Reviewed-by: Jonathan Nieder
---
lockfile.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lockfile.c b/lockfile.c
index 3df1e83..d74de8d 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -153,6 +153,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
int save_errno = errno;
error("cannot fix permission bits on %s",
lk->filename);
+ rollback_lock_file(lk);
errno = save_errno;
return -1;
}
--
2.1.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 v7 05/38] rollback_lock_file(): exit early if lock is not active
Eliminate a layer of nesting.
Signed-off-by: Michael Haggerty
Reviewed-by: Ronnie Sahlberg
Reviewed-by: Jonathan Nieder
---
lockfile.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 5330d6a..e55149a 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -276,10 +276,11 @@ int hold_locked_index(struct lock_file *lk, int
die_on_error)
void rollback_lock_file(struct lock_file *lk)
{
- if (lk->filename[0]) {
- if (lk->fd >= 0)
- close(lk->fd);
- unlink_or_warn(lk->filename);
- lk->filename[0] = 0;
- }
+ if (!lk->filename[0])
+ return;
+
+ if (lk->fd >= 0)
+ close(lk->fd);
+ unlink_or_warn(lk->filename);
+ lk->filename[0] = 0;
}
--
2.1.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 v7 15/38] remove_lock_file(): call rollback_lock_file()
It does just what we need.
Signed-off-by: Michael Haggerty
Reviewed-by: Jonathan Nieder
---
lockfile.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index a8f32e5..f8205f6 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -53,12 +53,8 @@ static void remove_lock_file(void)
pid_t me = getpid();
while (lock_file_list) {
- if (lock_file_list->owner == me &&
- lock_file_list->filename[0]) {
- if (lock_file_list->fd >= 0)
- close(lock_file_list->fd);
- unlink_or_warn(lock_file_list->filename);
- }
+ if (lock_file_list->owner == me)
+ rollback_lock_file(lock_file_list);
lock_file_list = lock_file_list->next;
}
}
--
2.1.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 v7 06/38] rollback_lock_file(): set fd to -1
When rolling back the lockfile, call close_lock_file() so that the lock_file's fd field gets set back to -1. This keeps the lock_file object in a valid state, which is important because these objects are allowed to be reused. It also makes it unnecessary to check whether the file has already been closed, because close_lock_file() takes care of that. Signed-off-by: Michael Haggerty Reviewed-by: Jonathan Nieder --- lockfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lockfile.c b/lockfile.c index e55149a..3df1e83 100644 --- a/lockfile.c +++ b/lockfile.c @@ -279,8 +279,7 @@ void rollback_lock_file(struct lock_file *lk) if (!lk->filename[0]) return; - if (lk->fd >= 0) - close(lk->fd); + close_lock_file(lk); unlink_or_warn(lk->filename); lk->filename[0] = 0; } -- 2.1.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 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()
On Wed, Oct 01, 2014 at 11:43:21AM +0200, René Scharfe wrote:
> If the first 18 bytes of the SHA1's of all entries are the same then
> sha1_pos() dies and reports that the lower and upper limits of the
> binary search were the same that this wasn't supposed to happen. This
> is wrong because the remaining two bytes could still differ.
>
> Furthermore: It wouldn't be a problem if they actually were the same,
> i.e. if all entries have the same SHA1. The code already handles
> duplicates just fine otherwise. Simply remove the erroneous check.
Yeah, I agree that assertion is just wrong.
Regarding duplicates: in sha1_entry_pos, we had to handle the "not
found" case specially, because we may have found the left-hand or
right-hand side of a run of duplicates, and we want to return the
correct slot where the new item would go (see the comment added by
171bdac). I think we don't have to deal with that here, because we are
just dealing with the initial "mi" selection. The actual binary search
is plain-vanilla, which handles that case just fine.
I wonder if it is worth adding a test (you test only that "not found"
produces a negative index, but not which index). Like:
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
index 3fcb8d8..7781129 100755
--- a/t/t0064-sha1-array.sh
+++ b/t/t0064-sha1-array.sh
@@ -42,12 +42,12 @@ test_expect_success 'lookup' '
'
test_expect_success 'lookup non-existing entry' '
+ echo -1 >expect &&
{
echo20 "append " 88 44 aa 55 &&
echo20 "lookup " 33
} | test-sha1-array >actual &&
- n=$(cat actual) &&
- test "$n" -lt 0
+ test_cmp expect actual
'
test_expect_success 'lookup with duplicates' '
@@ -61,6 +61,17 @@ test_expect_success 'lookup with duplicates' '
test "$n" -le 3
'
+test_expect_success 'lookup non-existing entry with duplicates' '
+ echo -5 >expect &&
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "lookup " 66
+ } | test-sha1-array >actual &&
+ test_cmp expect actual
+'
+
+
test_expect_success 'lookup with almost duplicate values' '
{
echo "append " &&
--
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] sha1-lookup: fix handling of duplicates in sha1_pos()
Am 01.10.2014 um 12:50 schrieb Jeff King:
On Wed, Oct 01, 2014 at 11:43:21AM +0200, René Scharfe wrote:
If the first 18 bytes of the SHA1's of all entries are the same then
sha1_pos() dies and reports that the lower and upper limits of the
binary search were the same that this wasn't supposed to happen. This
is wrong because the remaining two bytes could still differ.
Furthermore: It wouldn't be a problem if they actually were the same,
i.e. if all entries have the same SHA1. The code already handles
duplicates just fine otherwise. Simply remove the erroneous check.
Yeah, I agree that assertion is just wrong.
Regarding duplicates: in sha1_entry_pos, we had to handle the "not
found" case specially, because we may have found the left-hand or
right-hand side of a run of duplicates, and we want to return the
correct slot where the new item would go (see the comment added by
171bdac). I think we don't have to deal with that here, because we are
just dealing with the initial "mi" selection. The actual binary search
is plain-vanilla, which handles that case just fine.
I wonder if it is worth adding a test (you test only that "not found"
produces a negative index, but not which index). Like:
api-sha1-array.txt says about sha1_array_lookup: "If not found, returns
a negative integer", and that's what the test checks.
I actually like that the value is not specified for that case because no
existing caller actually uses it and it leaves room to implement the
function e.g. using bsearch(3).
I agree that adding a "lookup non-existing entry with duplicates" test
would make t0064 more complete, though.
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
index 3fcb8d8..7781129 100755
--- a/t/t0064-sha1-array.sh
+++ b/t/t0064-sha1-array.sh
@@ -42,12 +42,12 @@ test_expect_success 'lookup' '
'
test_expect_success 'lookup non-existing entry' '
+ echo -1 >expect &&
{
echo20 "append " 88 44 aa 55 &&
echo20 "lookup " 33
} | test-sha1-array >actual &&
- n=$(cat actual) &&
- test "$n" -lt 0
+ test_cmp expect actual
'
test_expect_success 'lookup with duplicates' '
@@ -61,6 +61,17 @@ test_expect_success 'lookup with duplicates' '
test "$n" -le 3
'
+test_expect_success 'lookup non-existing entry with duplicates' '
+ echo -5 >expect &&
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "lookup " 66
+ } | test-sha1-array >actual &&
+ test_cmp expect actual
+'
+
+
test_expect_success 'lookup with almost duplicate values' '
{
echo "append " &&
--
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 3/3] commit_packed_refs(): reimplement using fdopen_lock_file()
Signed-off-by: Michael Haggerty
---
refs.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 1d73f1d..a77458f 100644
--- a/refs.c
+++ b/refs.c
@@ -2309,16 +2309,13 @@ int commit_packed_refs(void)
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
- out = fdopen(packed_ref_cache->lock->fd, "w");
+ out = fdopen_lock_file(packed_ref_cache->lock, "w");
if (!out)
die_errno("unable to fdopen packed-refs descriptor");
fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
0, write_packed_entry_fn, out);
- if (fclose(out))
- die_errno("write error");
- packed_ref_cache->lock->fd = -1;
if (commit_lock_file(packed_ref_cache->lock)) {
save_errno = errno;
--
2.1.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 2/3] dump_marks(): reimplement using fdopen_lock_file()
Signed-off-by: Michael Haggerty
---
fast-import.c | 21 ++---
1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index deadc33..fee7906 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1794,20 +1794,18 @@ static void dump_marks_helper(FILE *f,
static void dump_marks(void)
{
static struct lock_file mark_lock;
- int mark_fd;
FILE *f;
if (!export_marks_file)
return;
- mark_fd = hold_lock_file_for_update(&mark_lock, export_marks_file, 0);
- if (mark_fd < 0) {
+ if (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) {
failure |= error("Unable to write marks file %s: %s",
export_marks_file, strerror(errno));
return;
}
- f = fdopen(mark_fd, "w");
+ f = fdopen_lock_file(&mark_lock, "w");
if (!f) {
int saved_errno = errno;
rollback_lock_file(&mark_lock);
@@ -1816,22 +1814,7 @@ static void dump_marks(void)
return;
}
- /*
-* Since the lock file was fdopen()'ed, it should not be close()'ed.
-* Assign -1 to the lock file descriptor so that commit_lock_file()
-* won't try to close() it.
-*/
- mark_lock.fd = -1;
-
dump_marks_helper(f, 0, marks);
- if (ferror(f) || fclose(f)) {
- int saved_errno = errno;
- rollback_lock_file(&mark_lock);
- failure |= error("Unable to write marks file %s: %s",
- export_marks_file, strerror(saved_errno));
- return;
- }
-
if (commit_lock_file(&mark_lock)) {
failure |= error("Unable to commit marks file %s: %s",
export_marks_file, strerror(errno));
--
2.1.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 1/3] fdopen_lock_file(): access a lockfile using stdio
Add a new function, fdopen_lock_file(), which returns a FILE pointer
open to the lockfile. If a stream is open on a lock_file object, it is
closed using fclose() on commit, rollback, or close_lock_file().
This change will allow callers to use stdio to write to a lockfile
without having to muck around in the internal representation of the
lock_file object (callers will be rewritten in upcoming commits).
Signed-off-by: Michael Haggerty
---
I thought about adding second stdio-oriented entrance points analogous
to hold_lock_file_for_update(), hold_lock_file_for_append(), and
reopen_lock_file(), but it seemed simpler to add just the one new
function instead of three or four. If using stdio on lockfiles becomes
more popular, we might want to add some helper functions to make it a
bit more convenient.
In close_lock_file(), if ferror() returns an error, then errno is not
necessarily still set in a way that reflects the original error. I
don't see a way to ensure that errno is set correctly in this
situation. But hopefully, callers are monitoring their calls to
fwrite()/fprintf() etc and will have noticed write errors on their own
already. If anybody can suggest an improvement here, please let me
know.
Documentation/technical/api-lockfile.txt | 34 +++
lockfile.c | 46
lockfile.h | 4 +++
3 files changed, 68 insertions(+), 16 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt
b/Documentation/technical/api-lockfile.txt
index d4484d1..93b5f23 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -42,9 +42,13 @@ The caller:
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 writing to the file
- descriptor returned by those functions (also available via
- `lock->fd`).
+* 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:
@@ -70,10 +74,10 @@ 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)` 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(2)`
+`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.
@@ -143,6 +147,13 @@ hold_lock_file_for_append::
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
+ (*without* rolling back the lockfile) on error. The stream is
+ closed automatically when `close_lock_file` is called or when
+ the file is committed or rolled back.
+
get_locked_file_path::
Return the path of the file that is locked by the specified
@@ -179,10 +190,11 @@ 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`, and close the file descriptor.
- Return 0 upon success. On failure to `close(2)`, return a
- negative value and roll back the lock file. Usually
- `commit_lock_file`, `commit_lock_file_to`, or
+ `hold_lock_file_for_append`. 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
+ file. Usually `commit_lock_file`, `commit_lock_file_to`, or
`rollback_lock_file` should eventually be called if
`close_lock_file` succeeds.
diff --git a/lockfile.c b/lockfile.c
index d27e61c..e046027 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -7,20 +7,29 @@
static struct lock_file *volatile lock_file_list;
-static void remove_lock_files(void)
+static void remove_lock_files(int skip_fclose)
{
pid_t me = getpid();
while (lock_file_list) {
- if (lock_file_list->owner == me)
+ if (lock_file_l
[PATCH 0/3] Support stdio access to lockfiles
This series applies on top of the series "Lockfile correctness and refactoring" (Junio's branch mh/lockfile). There are already two callers that write to lockfiles using stdio. But they currently need intimate knowledge of the lockfile implementation to work correctly; for example, they have to call fclose() themselves and set lk->fd to -1 to prevent the file from being closed again. This is awkward and error-prone. So provide official API support for stdio-based access to lockfiles. Add a new function fdopen_lock_file(), which returns a (FILE *) associated with an open lockfile, and teach close_lock_file() (and therefore also commit_lock_file(), rollback_lock_file(), etc.) to use fclose() instead of close() on lockfiles for which fdopen_lock_file() has been called. ...except in the signal handler, where calling fclose() is not permitted. In the signal handler call close() on any still-open lockfiles regardless of whether they have been fdopen()ed. Since the very next step is to delete the file, this should be OK. The second and third patches rewrite the two callers who currently fdopen() lockfiles to use the new function. I didn't look around for other lockfile users that might be simplified and/or sped up by converting them to use stdio; probably there are some. This improvement was initially discussed when the second fdopen() callsite was added [1] and also when discussing inconsistencies between the documentation and real life in the context of the mh/lockfile patch series [2]. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/256729/focus=256734 [2] http://thread.gmane.org/gmane.comp.version-control.git/257504/focus=257553 Michael Haggerty (3): fdopen_lock_file(): access a lockfile using stdio dump_marks(): reimplement using fdopen_lock_file() commit_packed_refs(): reimplement using fdopen_lock_file() Documentation/technical/api-lockfile.txt | 34 +++ fast-import.c| 21 ++- lockfile.c | 46 lockfile.h | 4 +++ refs.c | 5 +--- 5 files changed, 71 insertions(+), 39 deletions(-) -- 2.1.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 v7 09/38] lock_file(): always initialize and register lock_file object
Am 01.10.2014 um 12:28 schrieb Michael Haggerty: The purpose of this patch is to make the state diagram for lock_file objects simpler and deterministic. If locking fails, lock_file() sometimes leaves the lock_file object partly initialized, but sometimes not. It sometimes registers the object in lock_file_list, but sometimes not. This makes the state diagram for lock_file objects effectively indeterministic and hard to reason about. A future patch will also change the filename field into a strbuf, which needs more involved initialization, so it will become even more important that the state of a lock_file object is well-defined after a failed attempt to lock. The ambiguity doesn't currently have any ill effects, because lock_file objects cannot be removed from the lock_file_list anyway. But to make it easier to document and reason about the code, make this behavior inconsistent: *always* initialize the lock_file object and s/incon/con/, certainly? *always* register it in lock_file_list the first time it is used, regardless of whether an error occurs. While we're at it, make sure that all of the lock_file fields are initialized to values appropriate for an unlocked object; the caller is only responsible for making sure that on_list is set to zero before the first time it is used. -- 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 v7 09/38] lock_file(): always initialize and register lock_file object
On 10/01/2014 01:27 PM, René Scharfe wrote: > Am 01.10.2014 um 12:28 schrieb Michael Haggerty: >> The purpose of this patch is to make the state diagram for lock_file >> objects simpler and deterministic. >> >> If locking fails, lock_file() sometimes leaves the lock_file object >> partly initialized, but sometimes not. It sometimes registers the >> object in lock_file_list, but sometimes not. This makes the state >> diagram for lock_file objects effectively indeterministic and hard to >> reason about. A future patch will also change the filename field into >> a strbuf, which needs more involved initialization, so it will become >> even more important that the state of a lock_file object is >> well-defined after a failed attempt to lock. >> >> The ambiguity doesn't currently have any ill effects, because >> lock_file objects cannot be removed from the lock_file_list anyway. >> But to make it easier to document and reason about the code, make this >> behavior inconsistent: *always* initialize the lock_file object and > > s/incon/con/, certainly? Yes, thanks. Junio, if another reroll is not necessary, would you mind fixing this when applying? 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 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()
On Wed, Oct 01, 2014 at 01:10:12PM +0200, René Scharfe wrote: > >I wonder if it is worth adding a test (you test only that "not found" > >produces a negative index, but not which index). Like: > > api-sha1-array.txt says about sha1_array_lookup: "If not found, returns a > negative integer", and that's what the test checks. Hmm. I do not recall intentionally leaving the value unspecified; I think it is more that I was simply not thorough when writing the documentation. That being said... > I actually like that the value is not specified for that case because no > existing caller actually uses it and it leaves room to implement the > function e.g. using bsearch(3). Yeah, if no callers actually care right now, that is a reasonable argument for leaving the exact return value unspecified (and testing only what the documentation claims). > I agree that adding a "lookup non-existing entry with duplicates" test would > make t0064 more complete, though. Agreed. -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 1/3] fdopen_lock_file(): access a lockfile using stdio
On Wed, Oct 01, 2014 at 01:14:47PM +0200, Michael Haggerty wrote:
> I thought about adding second stdio-oriented entrance points analogous
> to hold_lock_file_for_update(), hold_lock_file_for_append(), and
> reopen_lock_file(), but it seemed simpler to add just the one new
> function instead of three or four. If using stdio on lockfiles becomes
> more popular, we might want to add some helper functions to make it a
> bit more convenient.
I think it makes sense to wait until we see more potential callers crop
up.
> In close_lock_file(), if ferror() returns an error, then errno is not
> necessarily still set in a way that reflects the original error. I
> don't see a way to ensure that errno is set correctly in this
> situation. But hopefully, callers are monitoring their calls to
> fwrite()/fprintf() etc and will have noticed write errors on their own
> already. If anybody can suggest an improvement here, please let me
> know.
I was careful in the packed-refs stdio caller to check all of my fprintf
calls (because I was using fclose myself). I wonder if it would be nicer
to back off from that and just depend on the ferror() call at
commit-time. The exact value of errno is not usually that important
after the open() has succeeded.
> -static void remove_lock_files(void)
> +static void remove_lock_files(int skip_fclose)
> {
> pid_t me = getpid();
>
> while (lock_file_list) {
> - if (lock_file_list->owner == me)
> + if (lock_file_list->owner == me) {
> + /* fclose() is not safe to call in a signal handler */
> + if (skip_fclose)
> + lock_file_list->fp = NULL;
I wondered when reading the commit message if it should mention this
signal-handling case (which you brought up in the cover letter). This
comment is probably enough, though.
> +FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
> +{
> + if (!lk->active)
> + die("BUG: fdopen_lock_file() called for unlocked object");
> + if (lk->fp)
> + die("BUG: fdopen_lock_file() called twice for file '%s'",
> lk->filename.buf);
I would have expected calling this twice to be a noop (i.e., make the
function more "give me the associated filehandle, and create one if
necessary"). But I don't think any current callers should need that, so
it probably makes sense to play it safe and die("BUG"), at least for
now.
> + if (fp) {
> + lk->fp = NULL;
> +
> + /*
> + * Note: no short-circuiting here; we want to fclose()
> + * in any case!
> + */
> + err = ferror(fp) | fclose(fp);
Would this be more clear as:
err = ferror(fp);
err |= fclose(fp);
-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/3] Support stdio access to lockfiles
On Wed, Oct 01, 2014 at 01:14:46PM +0200, Michael Haggerty wrote: > This series applies on top of the series "Lockfile correctness and > refactoring" (Junio's branch mh/lockfile). > > There are already two callers that write to lockfiles using stdio. But > they currently need intimate knowledge of the lockfile implementation > to work correctly; for example, they have to call fclose() themselves > and set lk->fd to -1 to prevent the file from being closed again. This > is awkward and error-prone. I think it's also wrong on systems where you cannot delete an open file. A signal or atexit handler will not be able to close the file (since it does not know the fd), and the unlink() will fail. IIRC, either cygwin or msysgit (or both?) is such a platform. > Michael Haggerty (3): > fdopen_lock_file(): access a lockfile using stdio > dump_marks(): reimplement using fdopen_lock_file() > commit_packed_refs(): reimplement using fdopen_lock_file() I had a few minor comments on the first patch, but I would also be OK with it going in as-is. The other two looked fine to me. Thanks for working on this. -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 1/2] sha1-array: add test-sha1-array and basic tests
On Wed, Oct 1, 2014 at 5:40 AM, René Scharfe wrote:
> Signed-off-by: Rene Scharfe
> ---
> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
> new file mode 100755
> index 000..bd68789
> --- /dev/null
> +++ b/t/t0064-sha1-array.sh
> @@ -0,0 +1,64 @@
> +#!/bin/sh
> +
> +test_description='basic tests for the SHA1 array implementation'
> +. ./test-lib.sh
> +
> +echo20() {
> + prefix="$1"
> + shift
> + while test $# -gt 0
> + do
> + echo "$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
Each caller of echo20() manually includes a space at the end of
$prefix. Would it make sense to instead have echo20() do this on
behalf of the caller?
echo "$prefix $1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
> + shift
> + done
> +}
> +
> +test_expect_success 'ordered enumeration' '
> + echo20 "" 44 55 88 aa >expect &&
> + {
> + echo20 "append " 88 44 aa 55 &&
Which would slightly reduce the burden on the caller and make it read
(very slightly) nicer:
echo20 append 88 44 aa 55 &&
> + echo for_each_unique
> + } | test-sha1-array >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'ordered enumeration with duplicate suppression' '
> + echo20 "" 44 55 88 aa >expect &&
> + {
> + echo20 "append " 88 44 aa 55 &&
> + echo20 "append " 88 44 aa 55 &&
> + echo for_each_unique
> + } | test-sha1-array >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'lookup' '
> + {
> + echo20 "append " 88 44 aa 55 &&
> + echo20 "lookup " 55
> + } | test-sha1-array >actual &&
> + n=$(cat actual) &&
> + test "$n" -eq 1
> +'
> +
> +test_expect_success 'lookup non-existing entry' '
> + {
> + echo20 "append " 88 44 aa 55 &&
> + echo20 "lookup " 33
> + } | test-sha1-array >actual &&
> + n=$(cat actual) &&
> + test "$n" -lt 0
> +'
> +
> +test_expect_success 'lookup with duplicates' '
> + {
> + echo20 "append " 88 44 aa 55 &&
> + echo20 "append " 88 44 aa 55 &&
> + echo20 "lookup " 55
> + } | test-sha1-array >actual &&
> + n=$(cat actual) &&
> + test "$n" -ge 2 &&
> + test "$n" -le 3
> +'
> +
> +test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()
On Wed, Oct 1, 2014 at 5:43 AM, René Scharfe wrote:
> If the first 18 bytes of the SHA1's of all entries are the same then
> sha1_pos() dies and reports that the lower and upper limits of the
> binary search were the same that this wasn't supposed to happen. This
> is wrong because the remaining two bytes could still differ.
>
> Furthermore: It wouldn't be a problem if they actually were the same,
> i.e. if all entries have the same SHA1. The code already handles
> duplicates just fine otherwise. Simply remove the erroneous check.
>
> Signed-off-by: Rene Scharfe
> ---
> sha1-lookup.c | 2 --
> t/t0064-sha1-array.sh | 20
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/sha1-lookup.c b/sha1-lookup.c
> index 2dd8515..5f06921 100644
> --- a/sha1-lookup.c
> +++ b/sha1-lookup.c
> @@ -84,8 +84,6 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t
> nr,
> die("BUG: assertion failed in binary search");
> }
> }
> - if (18 <= ofs)
> - die("cannot happen -- lo and hi are identical");
> }
>
> do {
> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
> index bd68789..3fcb8d8 100755
> --- a/t/t0064-sha1-array.sh
> +++ b/t/t0064-sha1-array.sh
> @@ -61,4 +61,24 @@ test_expect_success 'lookup with duplicates' '
> test "$n" -le 3
> '
>
> +test_expect_success 'lookup with almost duplicate values' '
> + {
> + echo "append " &&
> + echo "append 555f" &&
> + echo20 "lookup " 55
> + } | test-sha1-array >actual &&
> + n=$(cat actual) &&
> + test "$n" -eq 0
> +'
> +
> +test_expect_success 'lookup with single duplicate value' '
> + {
> + echo20 "append " 55 55 &&
> + echo20 "lookup " 55
> + } | test-sha1-array >actual &&
> + n=$(cat actual) &&
> + test "$n" -ge 0 &&
> + test "$n" -le 1
> +'
An alternative would be to introduce these two tests in patch 1/2 as
test_expect_failure and flip them to test_expect_success in this patch
which fixes the problem.
> +
> test_done
> --
> 2.1.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] sha1-array: add test-sha1-array and basic tests
Am 01.10.2014 um 16:11 schrieb Eric Sunshine:
On Wed, Oct 1, 2014 at 5:40 AM, René Scharfe wrote:
Signed-off-by: Rene Scharfe
---
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
new file mode 100755
index 000..bd68789
--- /dev/null
+++ b/t/t0064-sha1-array.sh
@@ -0,0 +1,64 @@
+#!/bin/sh
+
+test_description='basic tests for the SHA1 array implementation'
+. ./test-lib.sh
+
+echo20() {
+ prefix="$1"
+ shift
+ while test $# -gt 0
+ do
+ echo "$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
Each caller of echo20() manually includes a space at the end of
$prefix. Would it make sense to instead have echo20() do this on
behalf of the caller?
echo "$prefix $1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
That wouldn't work if the prefix is the empty string; we don't want a
space in that case (see the next echo20 call below).
But ${prefix:+$prefix } would do the trick. Thanks for the idea. :)
+ shift
+ done
+}
+
+test_expect_success 'ordered enumeration' '
+ echo20 "" 44 55 88 aa >expect &&
+ {
+ echo20 "append " 88 44 aa 55 &&
Which would slightly reduce the burden on the caller and make it read
(very slightly) nicer:
echo20 append 88 44 aa 55 &&
+ echo for_each_unique
+ } | test-sha1-array >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ordered enumeration with duplicate suppression' '
+ echo20 "" 44 55 88 aa >expect &&
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "append " 88 44 aa 55 &&
+ echo for_each_unique
+ } | test-sha1-array >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'lookup' '
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "lookup " 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -eq 1
+'
+
+test_expect_success 'lookup non-existing entry' '
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "lookup " 33
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -lt 0
+'
+
+test_expect_success 'lookup with duplicates' '
+ {
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "append " 88 44 aa 55 &&
+ echo20 "lookup " 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -ge 2 &&
+ test "$n" -le 3
+'
+
+test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] sha1-array: add test-sha1-array and basic tests
On Wed, Oct 01, 2014 at 10:11:04AM -0400, Eric Sunshine wrote:
> > +echo20() {
> > + prefix="$1"
> > + shift
> > + while test $# -gt 0
> > + do
> > + echo "$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
>
> Each caller of echo20() manually includes a space at the end of
> $prefix. Would it make sense to instead have echo20() do this on
> behalf of the caller?
>
> echo "$prefix $1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
Not always. For example:
> > +test_expect_success 'ordered enumeration' '
> > + echo20 "" 44 55 88 aa >expect &&
This does not.
> > + {
> > + echo20 "append " 88 44 aa 55 &&
>
> Which would slightly reduce the burden on the caller and make it read
> (very slightly) nicer:
>
> echo20 append 88 44 aa 55 &&
I agree that is more readable. But you'd have to make echo20 more like:
if test -n "$1"; then
prefix="$1 "
else
prefix=""
fi
which is not too bad.
-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 v2 1/2] sha1-array: add test-sha1-array and basic tests
Helped-by: Jeff King
Helped-by: Eric Sunshine
Signed-off-by: Rene Scharfe
---
Added a test for looking up a non-existing entry in an array that
contains duplicates, as suggested by Jeff. Changed echo20() to add
a space after the prefix as needed, as suggested by Eric.
.gitignore| 1 +
Makefile | 1 +
t/t0064-sha1-array.sh | 74 +++
test-sha1-array.c | 34 +++
4 files changed, 110 insertions(+)
create mode 100755 t/t0064-sha1-array.sh
create mode 100644 test-sha1-array.c
diff --git a/.gitignore b/.gitignore
index 5bfb234..9ec40fa 100644
--- a/.gitignore
+++ b/.gitignore
@@ -199,6 +199,7 @@
/test-revision-walking
/test-run-command
/test-sha1
+/test-sha1-array
/test-sigchain
/test-string-list
/test-subprocess
diff --git a/Makefile b/Makefile
index f34a2d4..356feb5 100644
--- a/Makefile
+++ b/Makefile
@@ -568,6 +568,7 @@ TEST_PROGRAMS_NEED_X += test-revision-walking
TEST_PROGRAMS_NEED_X += test-run-command
TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
TEST_PROGRAMS_NEED_X += test-sha1
+TEST_PROGRAMS_NEED_X += test-sha1-array
TEST_PROGRAMS_NEED_X += test-sigchain
TEST_PROGRAMS_NEED_X += test-string-list
TEST_PROGRAMS_NEED_X += test-subprocess
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
new file mode 100755
index 000..3f26e11
--- /dev/null
+++ b/t/t0064-sha1-array.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='basic tests for the SHA1 array implementation'
+. ./test-lib.sh
+
+echo20() {
+ prefix="${1:+$1 }"
+ shift
+ while test $# -gt 0
+ do
+ echo "$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1"
+ shift
+ done
+}
+
+test_expect_success 'ordered enumeration' '
+ echo20 "" 44 55 88 aa >expect &&
+ {
+ echo20 append 88 44 aa 55 &&
+ echo for_each_unique
+ } | test-sha1-array >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ordered enumeration with duplicate suppression' '
+ echo20 "" 44 55 88 aa >expect &&
+ {
+ echo20 append 88 44 aa 55 &&
+ echo20 append 88 44 aa 55 &&
+ echo for_each_unique
+ } | test-sha1-array >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'lookup' '
+ {
+ echo20 append 88 44 aa 55 &&
+ echo20 lookup 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -eq 1
+'
+
+test_expect_success 'lookup non-existing entry' '
+ {
+ echo20 append 88 44 aa 55 &&
+ echo20 lookup 33
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -lt 0
+'
+
+test_expect_success 'lookup with duplicates' '
+ {
+ echo20 append 88 44 aa 55 &&
+ echo20 append 88 44 aa 55 &&
+ echo20 lookup 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -ge 2 &&
+ test "$n" -le 3
+'
+
+test_expect_success 'lookup non-existing entry with duplicates' '
+ {
+ echo20 append 88 44 aa 55 &&
+ echo20 append 88 44 aa 55 &&
+ echo20 lookup 66
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -lt 0
+'
+
+test_done
diff --git a/test-sha1-array.c b/test-sha1-array.c
new file mode 100644
index 000..ddc491e
--- /dev/null
+++ b/test-sha1-array.c
@@ -0,0 +1,34 @@
+#include "cache.h"
+#include "sha1-array.h"
+
+static void print_sha1(const unsigned char sha1[20], void *data)
+{
+ puts(sha1_to_hex(sha1));
+}
+
+int main(int argc, char **argv)
+{
+ struct sha1_array array = SHA1_ARRAY_INIT;
+ struct strbuf line = STRBUF_INIT;
+
+ while (strbuf_getline(&line, stdin, '\n') != EOF) {
+ const char *arg;
+ unsigned char sha1[20];
+
+ if (skip_prefix(line.buf, "append ", &arg)) {
+ if (get_sha1_hex(arg, sha1))
+ die("not a hexadecimal SHA1: %s", arg);
+ sha1_array_append(&array, sha1);
+ } else if (skip_prefix(line.buf, "lookup ", &arg)) {
+ if (get_sha1_hex(arg, sha1))
+ die("not a hexadecimal SHA1: %s", arg);
+ printf("%d\n", sha1_array_lookup(&array, sha1));
+ } else if (!strcmp(line.buf, "clear"))
+ sha1_array_clear(&array);
+ else if (!strcmp(line.buf, "for_each_unique"))
+ sha1_array_for_each_unique(&array, print_sha1, NULL);
+ else
+ die("unknown command: %s", line.buf);
+ }
+ return 0;
+}
--
2.1.2
--
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/major
[PATCH v2 2/2] sha1-lookup: handle duplicates in sha1_pos()
If the first 18 bytes of the SHA1's of all entries are the same then
sha1_pos() dies and reports that the lower and upper limits of the
binary search were the same that this wasn't supposed to happen. This
is wrong because the remaining two bytes could still differ.
Furthermore: It wouldn't be a problem if they actually were the same,
i.e. if all entries have the same SHA1. The code already handles
duplicates just fine. Simply remove the erroneous check.
Signed-off-by: Rene Scharfe
---
sha1-lookup.c | 2 --
t/t0064-sha1-array.sh | 20
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/sha1-lookup.c b/sha1-lookup.c
index 2dd8515..5f06921 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -84,8 +84,6 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t
nr,
die("BUG: assertion failed in binary search");
}
}
- if (18 <= ofs)
- die("cannot happen -- lo and hi are identical");
}
do {
diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh
index 3f26e11..dbbe2e9 100755
--- a/t/t0064-sha1-array.sh
+++ b/t/t0064-sha1-array.sh
@@ -71,4 +71,24 @@ test_expect_success 'lookup non-existing entry with
duplicates' '
test "$n" -lt 0
'
+test_expect_success 'lookup with almost duplicate values' '
+ {
+ echo "append " &&
+ echo "append 555f" &&
+ echo20 lookup 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -eq 0
+'
+
+test_expect_success 'lookup with single duplicate value' '
+ {
+ echo20 append 55 55 &&
+ echo20 lookup 55
+ } | test-sha1-array >actual &&
+ n=$(cat actual) &&
+ test "$n" -ge 0 &&
+ test "$n" -le 1
+'
+
test_done
--
2.1.2
--
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 1/2] sha1-array: add test-sha1-array and basic tests
On Wed, Oct 01, 2014 at 05:00:33PM +0200, René Scharfe wrote: > Helped-by: Jeff King > Helped-by: Eric Sunshine > Signed-off-by: Rene Scharfe > --- > Added a test for looking up a non-existing entry in an array that > contains duplicates, as suggested by Jeff. Changed echo20() to add > a space after the prefix as needed, as suggested by Eric. Both patches look good to me. -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 v3 19/32] setup.c: support multi-checkout repo setup
Am 28.09.2014 um 03:22 schrieb Nguyễn Thái Ngọc Duy: > +test_expect_success 'GIT_DIR set (2)' ' > + echo "gitdir: repo.git/repos/foo" >gitfile && > + echo "$TRASH_DIRECTORY/repo.git" >repo.git/repos/foo/commondir && > + ( > + cd work && > + GIT_DIR=../gitfile git rev-parse --git-common-dir >actual && > + test-path-utils real_path "$TRASH_DIRECTORY/repo.git" >expect && > + test_cmp expect actual > + ) > +' This requires the following fixup because MinGW git will understand only DOS style absolute paths, but $TRASH_DIRECTORY is in POSIX-MSYS-style /c/foo/bar. --- 8< --- Subject: [PATCH] fixup! setup.c: support multi-checkout repo setup Signed-off-by: Johannes Sixt --- t/t1501-worktree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index e6ac7a4..4df7a2f 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -366,7 +366,7 @@ test_expect_success 'GIT_DIR set (1)' ' test_expect_success 'GIT_DIR set (2)' ' echo "gitdir: repo.git/repos/foo" >gitfile && - echo "$TRASH_DIRECTORY/repo.git" >repo.git/repos/foo/commondir && + echo "$(pwd)/repo.git" >repo.git/repos/foo/commondir && ( cd work && GIT_DIR=../gitfile git rev-parse --git-common-dir >actual && -- 2.0.0.12.gbcf935e -- 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 23/32] prune: strategies for linked checkouts
Am 28.09.2014 um 03:22 schrieb Nguyễn Thái Ngọc Duy: > +test_expect_success 'prune directories with gitdir pointing to nowhere' ' > + mkdir -p .git/worktrees/def/abc && > + : >.git/worktrees/def/def && > + echo "$TRASH_DIRECTORY"/nowhere >.git/worktrees/def/gitdir && > + git prune --worktrees --verbose >actual && > + test_i18ngrep "Removing worktrees/def: gitdir file points to > non-existent location" actual && > + ! test -d .git/worktrees/def && > + ! test -d .git/worktrees > +' > ... > +test_expect_success 'not prune recent checkouts' ' > + test_when_finished rm -r .git/worktrees > + mkdir zz && > + mkdir -p .git/worktrees/jlm && > + echo "$TRASH_DIRECTORY"/zz >.git/worktrees/jlm/gitdir && > + git prune --worktrees --verbose --expire=2.days.ago && > + test -d .git/worktrees/jlm > +' These require the following fixups because MinGW git will understand only DOS style absolute paths, but $TRASH_DIRECTORY is in POSIX-MSYS-style /c/foo/bar. --- 8< --- Subject: [PATCH] fixup! prune: strategies for linked checkouts Signed-off-by: Johannes Sixt --- t/t2026-prune-linked-checkouts.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t2026-prune-linked-checkouts.sh b/t/t2026-prune-linked-checkouts.sh index 3622800..170aefe 100755 --- a/t/t2026-prune-linked-checkouts.sh +++ b/t/t2026-prune-linked-checkouts.sh @@ -57,7 +57,7 @@ test_expect_success 'prune directories with invalid gitdir' ' test_expect_success 'prune directories with gitdir pointing to nowhere' ' mkdir -p .git/worktrees/def/abc && : >.git/worktrees/def/def && - echo "$TRASH_DIRECTORY"/nowhere >.git/worktrees/def/gitdir && + echo "$(pwd)"/nowhere >.git/worktrees/def/gitdir && git prune --worktrees --verbose >actual && test_i18ngrep "Removing worktrees/def: gitdir file points to non-existent location" actual && ! test -d .git/worktrees/def && @@ -76,7 +76,7 @@ test_expect_success 'not prune recent checkouts' ' test_when_finished rm -r .git/worktrees mkdir zz && mkdir -p .git/worktrees/jlm && - echo "$TRASH_DIRECTORY"/zz >.git/worktrees/jlm/gitdir && + echo "$(pwd)"/zz >.git/worktrees/jlm/gitdir && git prune --worktrees --verbose --expire=2.days.ago && test -d .git/worktrees/jlm ' -- 2.0.0.12.gbcf935e -- 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
Can you git clone -partial? I am looking to make a mobile app and it would be nice to have.
i am thinking about developing an app and I want to integrate it with git but I only want to store a portion of the file on disk. It is going to be in a mobile enviornment and I want to just get one file or a small group of files. I read that you can clone and then only look at one peice but is it possible just to clone one peice? And if not what would it take? Maybe this is a feature people would like to have. Just hypothetically, what would need to happen? Either git clonepartial /repo /file/or/folder/in/repo Or Git clone -partial /repo /file/or/folder/in/repo I guess you would need to keep the projects .git in order to maintain consistency, or would you? I'm going to do some more research about what .git does exactly. Cheers, James -- 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] init - Honour the global core.filemode setting
Hilco Wijbenga writes: > Perhaps I completely misunderstand the meaning of core.filemode but I > thought it determined whether Git cared about changes in file > properties? By setting it to "false", you tell Git that the filesystem you placed the repository does not correctly represent the filemode (especially the executable bit). "core.fileMode" in "git config --help" reads: core.fileMode If false, the executable bit differences between the index and the working tree are ignored; useful on broken filesystems like FAT. See git-update- index(1). The default is true, except git-clone(1) or git-init(1) will probe and set core.fileMode false if appropriate when the repository is created. Maybe our documentation is not clear enough. A contribution from somebody new to Git we would appreciate would be to point out which part of these sentences are unclear; that way, people can work on improving its phrasing. 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: Can you git clone -partial? I am looking to make a mobile app and it would be nice to have.
James Hancock writes: > i am thinking about developing an app and I want to integrate it with > git but I only want to store a portion of the file on disk. It is > going to be in a mobile enviornment and I want to just get one file or > a small group of files. > > I read that you can clone and then only look at one peice but is it > possible just to clone one peice? And if not what would it take? Maybe > this is a feature people would like to have. Just hypothetically, what > would need to happen? > > Either > git clonepartial /repo /file/or/folder/in/repo > Or > Git clone -partial /repo /file/or/folder/in/repo You keep saying "file", but the thing is, Git does not track file. It tracks history of collection of files. What are you trying to achieve, exactly? What does your "app" need out of that operation? Does it need these selected files with their history? Or does it only care about the contents of the selected files at the tip of the 'master' branch of that repository? I'd imagine that your answer would be the latter, and suspect that you may want to run "git archive --remote" with a pathspec to limit what gets grabbed. -- 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] git-prompt.sh: shorter equal upstream branch name
Richard Hansen writes:
>> and there is no hope to "fix" them to stick to
>> the bare-minimum POSIX,
>
> I don't think it'd be hard to convert it to pure POSIX if there was a
> desire to do so.
Not necessarily; if you make it so slow to be usable as a prompt
script, that is not a "conversion". Bash-isms in the script is
allowed for a reason, unfortunately.
> It would be unwise to go to great lengths to avoid Bashisms, but I think
> it would be smart to use POSIX syntax when it is easy to do so.
In general, I agree with you. People who know only bash tend to
overuse bash-isms where they are not necessary, leaving an
unreadable mess.
For the specific purpose of Julien's "if the tail part of this
string matches the other string, replace that with an equal sign",
${parameter/pattern/string} is a wrong bash-ism to use. But the
right solution to count the length of the other string and take a
substring of this string from its beginning would require other
bash-isms ${#parameter} and ${parameter:offset:length}.
And that's fine.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] daemon: handle gethostbyname() error
All three look trivially correct. 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 v7 09/38] lock_file(): always initialize and register lock_file object
Michael Haggerty writes: > Junio, if another reroll is not necessary, would you mind fixing this > when applying? No, I wouldn't. -- 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
VERY IMPORTANT!
Greetings, I am Sir Koffi Agogo. I saw your profile on LinkedIn.com and decided to contact you. I have something very important to discuss with you. It is a vital detail that will help you. I have sent you an e-mail two days ago but have not received your reply. You should contact me by this e-mail: sirkoffiago...@outlook.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 00/38] Lockfile correctness and refactoring
Overall I am very happy with this round ;-) Especially the last one, although I briefly debated myself if cache.h should include the new lockfile.h to reduce the churn, but I think that it is better without, so I like what you did there, too. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio
Jeff King writes:
> On Wed, Oct 01, 2014 at 01:14:47PM +0200, Michael Haggerty wrote:
>
>> ... If using stdio on lockfiles becomes
>> more popular, we might want to add some helper functions to make it a
>> bit more convenient.
>
> I think it makes sense to wait until we see more potential callers crop
> up.
Yup.
>> In close_lock_file(), if ferror() returns an error, then errno is not
>> necessarily still set in a way that reflects the original error. I
>> don't see a way to ensure that errno is set correctly in this
>> situation. But hopefully, callers are monitoring their calls to
>> fwrite()/fprintf() etc and will have noticed write errors on their own
>> already. If anybody can suggest an improvement here, please let me
>> know.
>
> I was careful in the packed-refs stdio caller to check all of my fprintf
> calls (because I was using fclose myself). I wonder if it would be nicer
> to back off from that and just depend on the ferror() call at
> commit-time.
That's a thought (I think the same can be said for "close-time").
>> -static void remove_lock_files(void)
>> +static void remove_lock_files(int skip_fclose)
>> {
>> pid_t me = getpid();
>>
>> while (lock_file_list) {
>> -if (lock_file_list->owner == me)
>> +if (lock_file_list->owner == me) {
>> +/* fclose() is not safe to call in a signal handler */
>> +if (skip_fclose)
>> +lock_file_list->fp = NULL;
>
> I wondered when reading the commit message if it should mention this
> signal-handling case (which you brought up in the cover letter). This
> comment is probably enough, though.
No strong opinion either way.
>> +FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
>> +{
>> +if (!lk->active)
>> +die("BUG: fdopen_lock_file() called for unlocked object");
>> +if (lk->fp)
>> +die("BUG: fdopen_lock_file() called twice for file '%s'",
>> lk->filename.buf);
>
> I would have expected calling this twice to be a noop (i.e., make the
> function more "give me the associated filehandle, and create one if
> necessary"). But I don't think any current callers should need that, so
> it probably makes sense to play it safe and die("BUG"), at least for
> now.
Interesting. One could imagine a sane call-chain whose top-level
creates a lockfile, associates a FILE * with it to write into it
itself, then calls set of helpers. You could pass only FILE * to
such helpers that does nothing other than writing to lk->fp to the
lockfile, but it would be cumbersome if a helper wants to have
access to the lockfile itself and FILE * (i.e. it writes and then
either commits or rolls back; name it foo_finish() or something).
Such a call-chain certainly would want a way to ask "I know this lk
is already associated with a FILE *; give me that". But that still
does not require "I do not know if this lk already has FILE * or
not, but I want a FILE * associated with it now. Peek or create."
So I think this is OK.
>> +if (fp) {
>> +lk->fp = NULL;
>> +
>> +/*
>> + * Note: no short-circuiting here; we want to fclose()
>> + * in any case!
>> + */
>> +err = ferror(fp) | fclose(fp);
>
> Would this be more clear as:
>
> err = ferror(fp);
> err |= fclose(fp);
No strong opinion either way.
Thanks, both.
--
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: Can you git clone -partial? I am looking to make a mobile app and it would be nice to have.
I see what you are saying with the file thing. I am not a git master and I am afraid that is showing in the words I use... The app I want to build is actually part of testing a Mobile Markdown Editor I am working on. I thought a good way to prove the system would be to make a simple app that allows you to edit your READMEs and other Markdown pages in your projects that use Git. They could be hosted on Github or elsewhere. It is mostly about testing the editor functionality on a larger scale. So, I want to just get the files I need out of the collection of file history and then put it back when and if I need to. But, I want to do this with as little network and data storage as possible (given the limitations of a mobile environment). I know most projects aren't very large as far as file size goes, but I am still wondering at what it would take to do this and if Git has the ability, or what would need to change for it to have the ability. Maybe it isn't worth it? I just want to be able to say why it isn't worth it with good reason. I read about git archive and I looks like it almost does what I want. I am not sure what happens underlying so my Git Noobness will show again here. I found documentation on how to us archive but I haven't been able to find something that tells me how archive does what it does. 1) Can you the push your changes in a file that you have used git archive for? Maybe you can do it just to set up a pull request or something. 2) How would the archive know when to pull updated changes? My understanding is that archive would just pull out the file but it would then be separated from its' parent. Is there a cost effective way to compare the archive to the master without downloading the whole history? Maybe I could keep a record of where the repo is and just have the program check the original repo for a new version? 3) I am wondering how intensive archive is? Do you need to get everything and then it narrow it down on your end or do you just grab the small part you need? How does the repo know what to give you? It says it compresses the data. Where does the compression happen? Cheers, James On Thu, Oct 2, 2014 at 1:38 AM, Junio C Hamano wrote: > James Hancock writes: > >> i am thinking about developing an app and I want to integrate it with >> git but I only want to store a portion of the file on disk. It is >> going to be in a mobile enviornment and I want to just get one file or >> a small group of files. >> >> I read that you can clone and then only look at one peice but is it >> possible just to clone one peice? And if not what would it take? Maybe >> this is a feature people would like to have. Just hypothetically, what >> would need to happen? >> >> Either >> git clonepartial /repo /file/or/folder/in/repo >> Or >> Git clone -partial /repo /file/or/folder/in/repo > > You keep saying "file", but the thing is, Git does not track file. > It tracks history of collection of files. > > What are you trying to achieve, exactly? What does your "app" need > out of that operation? Does it need these selected files with their > history? Or does it only care about the contents of the selected > files at the tip of the 'master' branch of that repository? > > I'd imagine that your answer would be the latter, and suspect that > you may want to run "git archive --remote" with a pathspec to limit > what gets grabbed. -- 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
Concurrent fetches and pushes
Are concurrent pushes and fetches to the same repository via HTTP supported? Considering this architecture: git --(smart HTTP protocol)--> apache -> git-http-backend git version is 2.1.1 on client and server. Multiple clients are fetching from and pushing to the same repositories, sometimes concurrently. Some pushes are failing with this error: To https://username:passw...@host.com/projectrepos/project.git + 3711f16...0bdfbc5 master -> master (forced update) error: RPC failed; result=52, HTTP code = 0 fatal: The remote end hung up unexpectedly remote: error: failed to lock refs/heads/master To https://username:passw...@host.com/projectrepos/project.git ! [remote rejected] master -> master (failed to lock) error: failed to push some refs to 'https://username:passw...@host.com/projectrepos/project.git' It says it failed to lock but we're not sure why. Error code 52 means it "got nothing". On the server logs, there are errors like these (I'm not sure if caused by fetch or push requests): error: Ref refs/heads/master is at f38a8a79752eac8a53fd195bc37fc56c0faf5cfa but expected a586117cd41604b8b2cb9776a98715c591071ad1 error: Ref refs/heads/master is at 6e1108c108ae464a37cbb3ccc84db934656cbcb4 but expected f38a8a79752eac8a53fd195bc37fc56c0faf5cfa fatal: git upload-pack: not our ref f38a8a79752eac8a53fd195bc37fc56c0faf5cfa error: Ref refs/heads/master is at 6e1108c108ae464a37cbb3ccc84db934656cbcb4 but expected f38a8a79752eac8a53fd195bc37fc56c0faf5cfa error: Ref refs/heads/master is at cc853c06962b3562e46cfefb3986abef914809b5 but expected 6e1108c108ae464a37cbb3ccc84db934656cbcb4 These errors don't happen when accessing a repository on the filesystem instead of via HTTP transport. Does git support concurrent fetch and push operations? Are there any restrictions to doing that via the HTTP backend? -- 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
Kedves Email felhasználói;
-- Kedves Email felhasználói; Túllépte a határt 23.432 tárolása az e-postafiók beállítva a WEB SERVICE / Administrator, és akkor problémái küldött és a bejövő üzenetek, amíg meg újból érvényesíti az e-mail címét. A szükséges eljárások nyújtottak be az alábbi a véleménye, ellenőrizze kattintva Az alábbi linkre és töltse ki az adatokat, hogy érvényesítse az e-mail címét. Kérjük, kattintson ide http://mailupdattw232.jigsy.com/ Növelni az e-mail kvótát az e-mail. Figyelem! Ennek elmulasztása azt eredményezi, hogy korlátozott hozzáférést a postafiók. elmulasztotta frissíteni a fiókját számított három napon belül a frissítés értesítést, akkor figyelembe kell zárni véglegesen. Tisztelettel, Rendszergazda ® -- 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_CONFIG should either apply to all commands, or none at all
Hi, Frédéric Brière wrote[1]: > This kind of stuff caused me a lot of hair-pulling: > > $ git config core.abbrev > 32 > git log --pretty=oneline --abbrev-commit > 89be foo > > Here's the source of the discrepancy: > > $ grep abbrev $GIT_CONFIG .git/config > git.conf: abbrev=32 > .git/config:abbrev=4 > > Since dc87183, $GIT_CONFIG is ignored by any other Git command, but it > *still* applies to git-config. This basically means that values > obtained via git-config are not necessarily those which are actually in > effect. > > The really frustrating part (for me, at least) is that for any tool > (gitweb in my case) which uses git-config, values from $GIT_CONFIG will > take effect for that tool, but not for any subsequent Git command. > > git-config(1) doesn't make this clear either; it mentions $GIT_CONFIG as > "the configuration", without saying explicitly that this environment > variable only applies to git-config. Yep. One possibility would be to do something like the following (A): 1) advertise in the git-config(1) manpage that the GIT_CONFIG environment variable only affects the behavior of the 'git config' command 2) introduce an environment variable GIT_I_AM_PORCELAIN. (If doing this, we could come up with a better name, but this is just an illustration.) Set and export that envvar in git-sh-setup.sh. When that environment variable is set, make git-config stop paying attention to GIT_CONFIG. That way, git commands that happen to be scripts would not be affected by the GIT_CONFIG setting any more. 3) Warn when 'git config' is called with GIT_CONFIG set, explaining that support will eventually be removed and that callers should pass --file= instead. 4) Once we're confident there are no scripts in the wild relying on that envvar, remove support for it. Another possibility (B): 1) Teach git's commands in C to respect the GIT_CONFIG environment variable. Semantics: only configuration from that file would be respected and all other configuration will be ignored. Advertise it in the git(1) manpage. 2) Gnash teeth a little but continue to support it. Yet another possibility (C): 1) Just skip to step (4) from plan (A). C is kind of temping. Do you know if there are scripts in the wild that rely on the GIT_CONFIG setting working? Thanks for reporting, Jonathan [1] http://bugs.debian.org/763712 -- 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 v22 0/24] rs/ref-transaction
Jonathan Nieder wrote: > Jonathan Nieder wrote: >> The next series from Ronnie's collection is available at >> https://code-review.googlesource.com/#/q/topic:ref-transaction in case >> someone wants a fresh series to look at. > > Here is the outcome of that review. It could use another set of eyes > (hint, hint) Another set of eyes arrived and helped. Here's a reroll. Jonathan Nieder (6): mv test: recreate mod/ directory instead of relying on stale copy branch -d: avoid repeated symref resolution packed-ref cache: forbid dot-components in refnames refs.c: do not permit err == NULL lockfile: remove unable_to_lock_error ref_transaction_commit: bail out on failure to remove a ref Junio C Hamano (1): reflog test: test interaction with detached HEAD Ronnie Sahlberg (17): wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as success wrapper.c: add a new function unlink_or_msg refs.c: add an err argument to delete_ref_loose refs.c: pass the ref log message to _create/delete/update instead of _commit rename_ref: don't ask read_ref_full where the ref came from refs.c: refuse to lock badly named refs in lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: pass a list of names to skip to is_refname_available refs.c: ref_transaction_commit: distinguish name conflicts from other errors fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static refs.c: change resolve_ref_unsafe reading argument to be a flags field branch -d: simplify by using RESOLVE_REF_READING flag test: put tests for handling of bad ref names in one place refs.c: allow listing and deleting badly named refs for-each-ref.c: improve message before aborting on broken ref remote rm/prune: print a message when writing packed-refs fails branch.c | 6 +- builtin/blame.c | 2 +- builtin/branch.c | 22 ++- builtin/checkout.c | 6 +- builtin/clone.c | 2 +- builtin/commit.c | 6 +- builtin/fetch.c | 34 ++-- builtin/fmt-merge-msg.c | 2 +- builtin/for-each-ref.c | 11 +- builtin/fsck.c | 2 +- builtin/log.c| 3 +- builtin/merge.c | 2 +- builtin/notes.c | 2 +- builtin/receive-pack.c | 9 +- builtin/remote.c | 20 ++- builtin/replace.c| 5 +- builtin/show-branch.c| 7 +- builtin/symbolic-ref.c | 2 +- builtin/tag.c| 4 +- builtin/update-ref.c | 13 +- bundle.c | 2 +- cache.h | 42 +++-- fast-import.c| 8 +- git-compat-util.h| 16 +- http-backend.c | 4 +- lockfile.c | 10 -- notes-merge.c| 2 +- reflog-walk.c| 5 +- refs.c | 438 --- refs.h | 40 +++-- remote.c | 11 +- sequencer.c | 8 +- t/t1400-update-ref.sh| 62 +++ t/t1413-reflog-detach.sh | 70 t/t1430-bad-ref-name.sh | 207 ++ t/t3200-branch.sh| 9 + t/t7001-mv.sh| 15 +- t/t9300-fast-import.sh | 30 transport-helper.c | 5 +- transport.c | 5 +- upload-pack.c| 2 +- walker.c | 5 +- wrapper.c| 28 ++- wt-status.c | 2 +- 44 files changed, 838 insertions(+), 348 deletions(-) create mode 100755 t/t1413-reflog-detach.sh create mode 100755 t/t1430-bad-ref-name.sh -- 2.0.0.450.ga793d96 --- Changes since v21: branch.c| 2 +- builtin/blame.c | 2 +- builtin/branch.c| 25 ++--- builtin/checkout.c | 6 +- builtin/clone.c | 2 +- builtin/commit.c| 2 +- builtin/fetch.c | 6 +- builtin/fmt-merge-msg.c | 2 +- builtin/for-each-ref.c | 11 +- builtin/fsck.c | 2 +- builtin/log.c | 4 +- builtin/merge.c | 2 +- builtin/notes.c | 2 +- builtin/receive-pack.c | 4 +- builtin/remote.c| 21 ++-- builtin/show-branch.c | 9 +- builtin/symbolic-ref.c | 2 +- builtin/update-ref.c| 3 +- bundle.c| 2 +- cache.h | 33 -- http-backend.c | 5 +- notes-merge.c | 2 +- reflog-walk.c | 6 +- refs.c | 263 +--- refs.h | 34 +++--- remote.c| 13 ++- sequencer.c | 4 +- t/t1400-update-ref.sh | 46 ++-- t/t1402-check-ref-format.sh | 48 t/t1413-reflog-detach.sh| 70 t/t1430-bad-ref-name.sh | 207 ++ t/t9300-fast-import.sh | 30 - tran
[PATCH 01/24] mv test: recreate mod/ directory instead of relying on stale copy
Date: Wed, 10 Sep 2014 14:01:46 -0700
The tests for 'git mv moves a submodule' functionality often run
commands like
git mv sub mod/sub
to move a submodule into a subdirectory. Just like plain /bin/mv,
this is supposed to succeed if the mod/ parent directory exists
and fail if it doesn't exist.
Usually these tests mkdir the parent directory beforehand, but some
instead rely on it being left behind by previous tests.
More precisely, when 'git reset --hard' tries to move to a state where
mod/sub is not present any more, it would perform the following
operations:
rmdir("mod/sub")
rmdir("mod")
The first fails with ENOENT because the test script removed mod/sub
with "rm -rf" already, so 'reset --hard' doesn't bother to move on to
the second, and the mod/ directory is kept around.
Better to explicitly remove and re-create the mod/ directory so later
tests don't have to depend on the directory left behind by the earlier
ones at all (making it easier to rearrange or skip some tests in the
file or to tweak 'reset --hard' behavior without breaking unrelated
tests).
Noticed while testing a patch that fixes the reset --hard behavior
described above.
Signed-off-by: Jonathan Nieder
Reviewed-by: Ronnie Sahlberg
---
As before.
t/t7001-mv.sh | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 54d7807..69f11bd 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -350,10 +350,11 @@ test_expect_success 'git mv moves a submodule with a .git
directory and .gitmodu
'
test_expect_success 'git mv moves a submodule with gitfile' '
- rm -rf mod/sub &&
+ rm -rf mod &&
git reset --hard &&
git submodule update &&
entry="$(git ls-files --stage sub | cut -f 1)" &&
+ mkdir mod &&
(
cd mod &&
git mv ../sub/ .
@@ -372,11 +373,12 @@ test_expect_success 'git mv moves a submodule with
gitfile' '
'
test_expect_success 'mv does not complain when no .gitmodules file is found' '
- rm -rf mod/sub &&
+ rm -rf mod &&
git reset --hard &&
git submodule update &&
git rm .gitmodules &&
entry="$(git ls-files --stage sub | cut -f 1)" &&
+ mkdir mod &&
git mv sub mod/sub 2>actual.err &&
! test -s actual.err &&
! test -e sub &&
@@ -390,11 +392,12 @@ test_expect_success 'mv does not complain when no
.gitmodules file is found' '
'
test_expect_success 'mv will error out on a modified .gitmodules file unless
staged' '
- rm -rf mod/sub &&
+ rm -rf mod &&
git reset --hard &&
git submodule update &&
git config -f .gitmodules foo.bar true &&
entry="$(git ls-files --stage sub | cut -f 1)" &&
+ mkdir mod &&
test_must_fail git mv sub mod/sub 2>actual.err &&
test -s actual.err &&
test -e sub &&
@@ -413,13 +416,14 @@ test_expect_success 'mv will error out on a modified
.gitmodules file unless sta
'
test_expect_success 'mv issues a warning when section is not found in
.gitmodules' '
- rm -rf mod/sub &&
+ rm -rf mod &&
git reset --hard &&
git submodule update &&
git config -f .gitmodules --remove-section submodule.sub &&
git add .gitmodules &&
entry="$(git ls-files --stage sub | cut -f 1)" &&
echo "warning: Could not find section in .gitmodules where path=sub"
>expect.err &&
+ mkdir mod &&
git mv sub mod/sub 2>actual.err &&
test_i18ncmp expect.err actual.err &&
! test -e sub &&
@@ -433,9 +437,10 @@ test_expect_success 'mv issues a warning when section is
not found in .gitmodule
'
test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules'
'
- rm -rf mod/sub &&
+ rm -rf mod &&
git reset --hard &&
git submodule update &&
+ mkdir mod &&
git mv -n sub mod/sub 2>actual.err &&
test -f sub/.git &&
git diff-index --exit-code HEAD &&
--
2.1.0.rc2.206.gedb03e5
--
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/24] wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as success
From: Ronnie Sahlberg
Date: Wed, 16 Jul 2014 11:01:18 -0700
Simplify the function warn_if_unremovable slightly. Additionally, change
behaviour slightly. If we failed to remove the object because the object
does not exist, we can still return success back to the caller since none of
the callers depend on "fail if the file did not exist".
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Change since v21:
- adjust caller to remove redundant errno check
git-compat-util.h | 7 +--
refs.c| 2 +-
wrapper.c | 14 ++
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index b6f03b3..611e77b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -706,11 +706,14 @@ void git_qsort(void *base, size_t nmemb, size_t size,
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
- * Always returns the return value of unlink(2).
+ * Returns 0 on success which includes trying to unlink an object that does
+ * not exist.
*/
int unlink_or_warn(const char *path);
/*
- * Likewise for rmdir(2).
+ * Preserves errno, prints a message, but gives no warning for ENOENT.
+ * Returns 0 on success which includes trying to remove a directory that does
+ * not exist.
*/
int rmdir_or_warn(const char *path);
/*
diff --git a/refs.c b/refs.c
index 7235574..d0565b1 100644
--- a/refs.c
+++ b/refs.c
@@ -2557,7 +2557,7 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
lock->lk->filename[i] = 0;
err = unlink_or_warn(lock->lk->filename);
lock->lk->filename[i] = '.';
- if (err && errno != ENOENT)
+ if (err)
return 1;
}
return 0;
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..c9605cd 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
static int warn_if_unremovable(const char *op, const char *file, int rc)
{
- if (rc < 0) {
- int err = errno;
- if (ENOENT != err) {
- warning("unable to %s %s: %s",
- op, file, strerror(errno));
- errno = err;
- }
- }
+ int err;
+ if (!rc || errno == ENOENT)
+ return 0;
+ err = errno;
+ warning("unable to %s %s: %s", op, file, strerror(errno));
+ errno = err;
return rc;
}
--
2.1.0.rc2.206.gedb03e5
--
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/24] wrapper.c: add a new function unlink_or_msg
From: Ronnie Sahlberg
Date: Wed, 16 Jul 2014 11:20:36 -0700
This behaves like unlink_or_warn except that on failure it writes the message
to its 'err' argument, which the caller can display in an appropriate way or
ignore.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
Reviewed-by: Michael Haggerty
---
As before.
git-compat-util.h | 9 +
wrapper.c | 14 ++
2 files changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 611e77b..5ee140c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -307,6 +307,8 @@ extern char *gitbasename(char *);
#include "wildmatch.h"
+struct strbuf;
+
/* General helper functions */
extern void vreportf(const char *prefix, const char *err, va_list params);
extern void vwritef(int fd, const char *prefix, const char *err, va_list
params);
@@ -710,6 +712,13 @@ void git_qsort(void *base, size_t nmemb, size_t size,
* not exist.
*/
int unlink_or_warn(const char *path);
+ /*
+ * Tries to unlink file. Returns 0 if unlink succeeded
+ * or the file already didn't exist. Returns -1 and
+ * appends a message to err suitable for
+ * 'error("%s", err->buf)' on error.
+ */
+int unlink_or_msg(const char *file, struct strbuf *err);
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
* Returns 0 on success which includes trying to remove a directory that does
diff --git a/wrapper.c b/wrapper.c
index c9605cd..ec7a08b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -438,6 +438,20 @@ static int warn_if_unremovable(const char *op, const char
*file, int rc)
return rc;
}
+int unlink_or_msg(const char *file, struct strbuf *err)
+{
+ int rc = unlink(file);
+
+ assert(err);
+
+ if (!rc || errno == ENOENT)
+ return 0;
+
+ strbuf_addf(err, "unable to unlink %s: %s",
+ file, strerror(errno));
+ return -1;
+}
+
int unlink_or_warn(const char *file)
{
return warn_if_unremovable("unlink", file, unlink(file));
--
2.1.0.rc2.206.gedb03e5
--
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 04/24] refs.c: add an err argument to delete_ref_loose
From: Ronnie Sahlberg
Date: Thu, 15 May 2014 08:25:23 -0700
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_ref_loose.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Changes since v21:
- s/delete_loose_ref/delete_ref_loose/ once in commit message (but
the other one still needs fixing)
refs.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index d0565b1..5609622 100644
--- a/refs.c
+++ b/refs.c
@@ -2548,16 +2548,16 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
return ret;
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+ int res, i = strlen(lock->lk->filename) - 5; /* .lock */
lock->lk->filename[i] = 0;
- err = unlink_or_warn(lock->lk->filename);
+ res = unlink_or_msg(lock->lk->filename, err);
lock->lk->filename[i] = '.';
- if (err)
+ if (res)
return 1;
}
return 0;
@@ -3604,7 +3604,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update->lock) {
- ret |= delete_ref_loose(update->lock, update->type);
+ ret |= delete_ref_loose(update->lock, update->type,
+ err);
if (!(update->flags & REF_ISPRUNING))
delnames[delnum++] = update->lock->ref_name;
}
--
2.1.0.rc2.206.gedb03e5
--
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 06/24] rename_ref: don't ask read_ref_full where the ref came from
From: Ronnie Sahlberg
Date: Wed, 30 Apr 2014 12:41:04 -0700
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
Reviewed-by: Michael Haggerty
---
As before.
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 99a9b86..39571f5 100644
--- a/refs.c
+++ b/refs.c
@@ -2671,7 +2671,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, &flag) &&
+ if (!read_ref_full(newrefname, sha1, 1, NULL) &&
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path("%s",
newrefname))) {
--
2.1.0.rc2.206.gedb03e5
--
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 05/24] refs.c: pass the ref log message to _create/delete/update instead of _commit
From: Ronnie Sahlberg
Date: Wed, 30 Apr 2014 12:22:42 -0700
Change the ref transaction API so that we pass the reflog message to the
create/delete/update functions instead of to ref_transaction_commit.
This allows different reflog messages for each ref update in a multi-ref
transaction.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Changes since v21:
- cleaned up commit message
- clarified ownership of msg in comment in refs.h
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 5 +++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 34 +-
refs.h | 12 ++--
sequencer.c| 4 ++--
walker.c | 5 ++---
11 files changed, 54 insertions(+), 44 deletions(-)
diff --git a/branch.c b/branch.c
index 37ac555..76a8ec9 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, &err) ||
- ref_transaction_commit(transaction, msg, &err))
+ null_sha1, 0, !forcing, msg, &err) ||
+ ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
strbuf_release(&err);
diff --git a/builtin/commit.c b/builtin/commit.c
index 9bf1003..d23e876 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1767,8 +1767,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, "HEAD", sha1,
current_head
? current_head->object.sha1 : NULL,
- 0, !!current_head, &err) ||
- ref_transaction_commit(transaction, sb.buf, &err)) {
+ 0, !!current_head, sb.buf, &err) ||
+ ref_transaction_commit(transaction, &err)) {
rollback_index_files();
die("%s", err.buf);
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 224fadc..d1f4cf7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -585,8 +585,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, &err) ||
- ref_transaction_commit(transaction, "push", &err)) {
+ new_sha1, old_sha1, 0, 1, "push",
+ &err) ||
+ ref_transaction_commit(transaction, &err)) {
ref_transaction_free(transaction);
rp_error("%s", err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 1fcd06d..9d03b84 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -169,8 +169,9 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(&err);
if (!transaction ||
- ref_transaction_update(transaction, ref, repl, prev, 0, 1, &err) ||
- ref_transaction_commit(transaction, NULL, &err))
+ ref_transaction_update(transaction, ref, repl, prev,
+ 0, 1, NULL, &err) ||
+ ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index f3f172f..70d28c5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
- 0, 1, &err) ||
- ref_transaction_commit(transaction, NULL, &err))
+ 0, 1, NULL, &err) ||
+ ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
if (force && !is_null_sha1(prev) && hashcmp(prev, object))
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 54a48c0..6c9be05 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -14,6 +14,7 @@ static const char * const git_update_ref_usage[] = {
static char line_termination = '\n';
static int update_flags;
+static const char *msg;
/*
* Parse one white
[PATCH 07/24] refs.c: refuse to lock badly named refs in lock_ref_sha1_basic
From: Ronnie Sahlberg
Date: Thu, 1 May 2014 10:40:10 -0700
Move the check for check_refname_format from lock_any_ref_for_update to
lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely. This has no visible impact to callers
except for the inability to lock badly named refs, which is not possible
today already for other reasons.(*)
Keep lock_any_ref_for_update as a no-op wrapper. It is the public facing
version of this interface and keeping it as a separate function will make
it easier to experiment with the internal lock_ref_sha1_basic signature.
(*) For example, if lock_ref_sha1_basic checks the refname format and
refuses to lock badly named refs, it will not be possible to delete
such refs because the first step of deletion is to lock the ref. We
currently already fail in that case because these refs are not recognized
to exist:
$ cp .git/refs/heads/master .git/refs/heads/echo...\*\*
$ git branch -D .git/refs/heads/echo...\*\*
error: branch '.git/refs/heads/echo...**' not found.
This has been broken for a while. Later patches in the series will start
repairing the handling of badly named refs.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Change since v21:
- clarified commit message
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 39571f5..3c2ce57 100644
--- a/refs.c
+++ b/refs.c
@@ -2091,6 +2091,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int missing = 0;
int attempts_remaining = 3;
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ errno = EINVAL;
+ return NULL;
+ }
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock->lock_fd = -1;
@@ -2182,8 +2187,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.1.0.rc2.206.gedb03e5
--
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 08/24] refs.c: call lock_ref_sha1_basic directly from commit
From: Ronnie Sahlberg
Date: Thu, 1 May 2014 10:43:39 -0700
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
Reviewed-by: Michael Haggerty
---
As before.
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 3c2ce57..f124c2b 100644
--- a/refs.c
+++ b/refs.c
@@ -3578,12 +3578,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- update->lock = lock_any_ref_for_update(update->refname,
- (update->have_old ?
- update->old_sha1 :
- NULL),
- update->flags,
- &update->type);
+ update->lock = lock_ref_sha1_basic(update->refname,
+ (update->have_old ?
+ update->old_sha1 :
+ NULL),
+ update->flags,
+ &update->type);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
--
2.1.0.rc2.206.gedb03e5
--
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/24] refs.c: pass a list of names to skip to is_refname_available
From: Ronnie Sahlberg
Date: Thu, 1 May 2014 11:16:07 -0700
Change is_refname_available to take a list of strings to exclude when
checking for conflicts instead of just one single name. We can already
exclude a single name for the sake of renames. This generalizes that support.
ref_transaction_commit already tracks a set of refs that are being deleted
in an array. This array is then used to exclude refs from being written to
the packed-refs file. At some stage we will want to change this array to a
struct string_list and then we can pass it to is_refname_available via the
call to lock_ref_sha1_basic. That will allow us to perform transactions
that perform multiple renames as long as there are no conflicts within the
starting or ending state.
For example, that would allow a single transaction that contains two
renames that are both individually conflicting:
m -> n/n
n -> m/m
No functional change intended yet.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Since v21:
- clarified commit message
- clarified comments
refs.c | 44 +---
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index f124c2b..6820c93 100644
--- a/refs.c
+++ b/refs.c
@@ -801,14 +801,16 @@ static int names_conflict(const char *refname1, const
char *refname2)
struct name_conflict_cb {
const char *refname;
- const char *oldrefname;
const char *conflicting_refname;
+ struct string_list *skiplist;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
- if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
+
+ if (data->skiplist &&
+ string_list_has_string(data->skiplist, entry->name))
return 0;
if (names_conflict(data->refname, entry->name)) {
data->conflicting_refname = entry->name;
@@ -820,17 +822,18 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
/*
* Return true iff a reference named refname could be created without
* conflicting with the name of an existing reference in dir. If
- * oldrefname is non-NULL, ignore potential conflicts with oldrefname
- * (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * skiplist is non-NULL, ignore potential conflicts with names in
+ * skiplist (e.g., because those refs are scheduled for deletion in
+ * the same operation). skiplist must be sorted.
*/
-static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+static int is_refname_available(const char *refname,
+ struct ref_dir *dir,
+ struct string_list *skiplist)
{
struct name_conflict_cb data;
data.refname = refname;
- data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skiplist = skiplist;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {
@@ -2080,6 +2083,7 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
*/
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
+ struct string_list *skiplist,
int flags, int *type_p)
{
char *ref_file;
@@ -2129,7 +2133,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing &&
-!is_refname_available(refname, NULL, get_packed_refs(&ref_cache)))
{
+!is_refname_available(refname, get_packed_refs(&ref_cache),
+ skiplist)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2187,7 +2192,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, NULL, flags, type_p);
}
/*
@@ -2648,6 +2653,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
const char *symref = NULL;
+ struct string_list skiplist = STRING_LIST_INIT_NODUP;
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
@@ -2659,11 +2665,18 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref)
return error("refname %s not foun
[PATCH 10/24] refs.c: ref_transaction_commit: distinguish name conflicts from other errors
From: Ronnie Sahlberg
Date: Fri, 16 May 2014 14:14:38 -0700
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either
when we lstat the new refname or if the name checking function reports that
the same type of conflict happened. In both cases, it means that we can not
create the new ref due to a name conflict.
Start defining specific return codes for _commit. TRANSACTION_NAME_CONFLICT
refers to a failure to create a ref due to a name conflict with another ref.
TRANSACTION_GENERIC_ERROR is for all other errors.
When "git fetch" is creating refs, name conflicts differ from other errors in
that they are likely to be resolved by running "git remote prune ".
"git fetch" currently inspects errno to decide whether to give that advice.
Once it switches to the transaction API, it can check for
TRANSACTION_NAME_CONFLICT instead.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Since v21:
- clarified commit message and updated to match code
- small code cleanups
- clarified API doc, introduced TRANSACTION_GENERIC_ERROR so both error
codes have names
refs.c | 26 --
refs.h | 9 +++--
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/refs.c b/refs.c
index 6820c93..623a1ae 100644
--- a/refs.c
+++ b/refs.c
@@ -3583,9 +3583,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err);
- if (ret)
+ if (ref_update_reject_duplicates(updates, n, err)) {
+ ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
+ }
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
@@ -3599,10 +3600,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update->flags,
&update->type);
if (!update->lock) {
+ ret = (errno == ENOTDIR)
+ ? TRANSACTION_NAME_CONFLICT
+ : TRANSACTION_GENERIC_ERROR;
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
update->refname);
- ret = 1;
goto cleanup;
}
}
@@ -3612,15 +3615,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update->new_sha1)) {
- ret = write_ref_sha1(update->lock, update->new_sha1,
-update->msg);
- update->lock = NULL; /* freed by write_ref_sha1 */
- if (ret) {
+ if (write_ref_sha1(update->lock, update->new_sha1,
+ update->msg)) {
+ update->lock = NULL; /* freed by write_ref_sha1
*/
if (err)
strbuf_addf(err, "Cannot update the ref
'%s'.",
update->refname);
+ ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
+ update->lock = NULL; /* freed by write_ref_sha1 */
}
}
@@ -3629,14 +3633,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update->lock) {
- ret |= delete_ref_loose(update->lock, update->type,
- err);
+ if (delete_ref_loose(update->lock, update->type, err))
+ ret = TRANSACTION_GENERIC_ERROR;
+
if (!(update->flags & REF_ISPRUNING))
delnames[delnum++] = update->lock->ref_name;
}
}
- ret |= repack_without_refs(delnames, delnum, err);
+ if (repack_without_refs(delnames, delnum, err))
+ ret = TRANSACTION_GENERIC_ERROR;
for (i = 0; i < delnum; i++)
unlink_or_warn(git_path("logs/%s", delnames[i]));
clear_loose_ref_cache(&ref_cache);
diff --git a/refs.h b/refs.h
index aded545..fd63b47 100644
--- a/refs.h
+++ b/refs.h
@@ -323,9 +323,14 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
/*
* Commit all of the changes that have been queued in transaction, as
- * atomically as possible. Return a nonzero value if there is a
- * problem.
+ * atomically as possible.
+ *
+ * Returns 0 for success, or one of the below error codes fo
[PATCH 11/24] fetch.c: change s_update_ref to use a ref transaction
From: Ronnie Sahlberg
Date: Mon, 28 Apr 2014 13:49:07 -0700
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Since v21:
- tweaked handling of ref_transaction_commit result (no functional
change)
builtin/fetch.c | 34 --
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..30b40f6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,23 +375,37 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv("GIT_REFLOG_ACTION");
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+ int ret, df_conflict = 0;
if (dry_run)
return 0;
if (!rla)
rla = default_rla.buf;
snprintf(msg, sizeof(msg), "%s: %s", rla, action);
- lock = lock_any_ref_for_update(ref->name,
- check_old ? ref->old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
+
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, msg, &err))
+ goto fail;
+
+ ret = ref_transaction_commit(transaction, &err);
+ if (ret) {
+ df_conflict = (ret == TRANSACTION_NAME_CONFLICT);
+ goto fail;
+ }
+
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
return 0;
+fail:
+ ref_transaction_free(transaction);
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
+ : STORE_REF_ERROR_OTHER;
}
#define REFCOL_WIDTH 10
--
2.1.0.rc2.206.gedb03e5
--
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/24] refs.c: make write_ref_sha1 static
From: Ronnie Sahlberg
Date: Mon, 28 Apr 2014 15:36:58 -0700
No external users call write_ref_sha1 any more so let's declare it static.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Since v21:
- punctuation fix in commit message
- grammar tweak in doc comment
refs.c | 10 --
refs.h | 3 ---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 623a1ae..f596a9f 100644
--- a/refs.c
+++ b/refs.c
@@ -2645,6 +2645,9 @@ static int rename_tmp_log(const char *newrefname)
return 0;
}
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+ const char *logmsg);
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -2900,8 +2903,11 @@ static int is_branch(const char *refname)
return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
}
-/* This function must return a meaningful errno */
-int write_ref_sha1(struct ref_lock *lock,
+/*
+ * Write sha1 into the ref specified by the lock. Make sure that errno
+ * is sane on error.
+ */
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index fd63b47..3bb16db 100644
--- a/refs.h
+++ b/refs.h
@@ -195,9 +195,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/*
* Setup reflog before using. Set errno to something meaningful on failure.
*/
--
2.1.0.rc2.206.gedb03e5
--
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 14/24] reflog test: test interaction with detached HEAD
From: Junio C Hamano
A proposed patch produced broken HEAD reflog entries when checking out
anything other than a branch. The testsuite still passed, so it took
a few days for the bug to be noticed.
Add tests checking the content of the reflog after detaching and
reattaching HEAD so we don't have to rely on manual testing to catch
such problems in the future.
[jn: using 'log -g --format=%H' instead of parsing --oneline output,
resetting state in each test so they can be safely reordered or
skipped]
Signed-off-by: Jonathan Nieder
Reviewed-by: Ronnie Sahlberg
---
New since v21. Thanks to Junio for noticing the bug.
t/t1413-reflog-detach.sh | 70
1 file changed, 70 insertions(+)
create mode 100755 t/t1413-reflog-detach.sh
diff --git a/t/t1413-reflog-detach.sh b/t/t1413-reflog-detach.sh
new file mode 100755
index 000..c730600
--- /dev/null
+++ b/t/t1413-reflog-detach.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='Test reflog interaction with detached HEAD'
+. ./test-lib.sh
+
+reset_state () {
+ git checkout master &&
+ cp saved_reflog .git/logs/HEAD
+}
+
+test_expect_success setup '
+ test_tick &&
+ git commit --allow-empty -m initial &&
+ git branch side &&
+ test_tick &&
+ git commit --allow-empty -m second &&
+ cat .git/logs/HEAD >saved_reflog
+'
+
+test_expect_success baseline '
+ reset_state &&
+ git rev-parse master master^ >expect &&
+ git log -g --format=%H >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'switch to branch' '
+ reset_state &&
+ git rev-parse side master master^ >expect &&
+ git checkout side &&
+ git log -g --format=%H >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'detach to other' '
+ reset_state &&
+ git rev-parse master side master master^ >expect &&
+ git checkout side &&
+ git checkout master^0 &&
+ git log -g --format=%H >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'detach to self' '
+ reset_state &&
+ git rev-parse master master master^ >expect &&
+ git checkout master^0 &&
+ git log -g --format=%H >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'attach to self' '
+ reset_state &&
+ git rev-parse master master master master^ >expect &&
+ git checkout master^0 &&
+ git checkout master &&
+ git log -g --format=%H >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'attach to other' '
+ reset_state &&
+ git rev-parse side master master master^ >expect &&
+ git checkout master^0 &&
+ git checkout side &&
+ git log -g --format=%H >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.1.0.rc2.206.gedb03e5
--
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/24] refs.c: change resolve_ref_unsafe reading argument to be a flags field
From: Ronnie Sahlberg
Date: Tue, 15 Jul 2014 12:59:36 -0700
resolve_ref_unsafe takes a boolean argument for reading (a nonexistent ref
resolves successfully for writing but not for reading). Change this to be
a flags field instead, and pass the new constant RESOLVE_REF_READING when
we want this behaviour.
While at it, swap two of the arguments in the function to put output
arguments at the end. As a nice side effect, this ensures that we can
catch callers that were unaware of the new API so they can be audited.
Give the wrapper functions resolve_refdup and read_ref_full the same
treatment for consistency.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Since v21:
- clarified commit message
- put output parameters last
branch.c| 2 +-
builtin/blame.c | 2 +-
builtin/branch.c| 9 ++---
builtin/checkout.c | 6 ++--
builtin/clone.c | 2 +-
builtin/commit.c| 2 +-
builtin/fmt-merge-msg.c | 2 +-
builtin/for-each-ref.c | 6 ++--
builtin/fsck.c | 2 +-
builtin/log.c | 3 +-
builtin/merge.c | 2 +-
builtin/notes.c | 2 +-
builtin/receive-pack.c | 4 +--
builtin/remote.c| 5 +--
builtin/show-branch.c | 7 ++--
builtin/symbolic-ref.c | 2 +-
bundle.c| 2 +-
cache.h | 23 ++--
http-backend.c | 4 ++-
notes-merge.c | 2 +-
reflog-walk.c | 5 +--
refs.c | 93 -
remote.c| 11 +++---
sequencer.c | 4 +--
transport-helper.c | 5 ++-
transport.c | 5 +--
upload-pack.c | 2 +-
wt-status.c | 2 +-
28 files changed, 124 insertions(+), 92 deletions(-)
diff --git a/branch.c b/branch.c
index 76a8ec9..adb07c6 100644
--- a/branch.c
+++ b/branch.c
@@ -186,7 +186,7 @@ int validate_new_branchname(const char *name, struct strbuf
*ref,
const char *head;
unsigned char sha1[20];
- head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
+ head = resolve_ref_unsafe("HEAD", 0, sha1, NULL);
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
die(_("Cannot force update the current branch."));
}
diff --git a/builtin/blame.c b/builtin/blame.c
index a52a279..5cbd38f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2292,7 +2292,7 @@ static struct commit *fake_working_tree_commit(struct
diff_options *opt,
commit->object.type = OBJ_COMMIT;
parent_tail = &commit->parents;
- if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
+ if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, head_sha1, NULL))
die("no such ref: HEAD");
parent_tail = append_parent(parent_tail, head_sha1);
diff --git a/builtin/branch.c b/builtin/branch.c
index 652b1d2..e5d1377 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -129,7 +129,8 @@ static int branch_merged(int kind, const char *name,
branch->merge[0] &&
branch->merge[0]->dst &&
(reference_name = reference_name_to_free =
-resolve_refdup(branch->merge[0]->dst, sha1, 1, NULL)) !=
NULL)
+resolve_refdup(branch->merge[0]->dst, RESOLVE_REF_READING,
+ sha1, NULL)) != NULL)
reference_rev = lookup_commit_reference(sha1);
}
if (!reference_rev)
@@ -233,7 +234,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, sha1, 0, &flags);
+ target = resolve_ref_unsafe(name, 0, sha1, &flags);
if (!target ||
(!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {
error(remote_branch
@@ -296,7 +297,7 @@ static char *resolve_symref(const char *src, const char
*prefix)
int flag;
const char *dst, *cp;
- dst = resolve_ref_unsafe(src, sha1, 0, &flag);
+ dst = resolve_ref_unsafe(src, 0, sha1, &flag);
if (!(dst && (flag & REF_ISSYMREF)))
return NULL;
if (prefix && (cp = skip_prefix(dst, prefix)))
@@ -862,7 +863,7 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
track = git_branch_track;
- head = resolve_refdup("HEAD", head_sha1, 0, NULL);
+ head = resolve_refdup("HEAD", 0, head_sha1, NULL);
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
if (!strcmp(head, "HEAD")) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..a5fef2d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -356,7 +356,7 @@ static int checkout_paths(const struct checkout_opts *opts,
[PATCH 15/24] branch -d: avoid repeated symref resolution
From: Jonathan Nieder
Date: Wed, 10 Sep 2014 18:22:48 -0700
If a repository gets in a broken state with too much symref nesting,
it cannot be repaired with "git branch -d":
$ git symbolic-ref refs/heads/nonsense refs/heads/nonsense
$ git branch -d nonsense
error: branch 'nonsense' not found.
Worse, "git update-ref --no-deref -d" doesn't work for such repairs
either:
$ git update-ref -d refs/heads/nonsense
error: unable to resolve reference refs/heads/nonsense: Too many levels of
symbolic links
Fix both by teaching resolve_ref_unsafe a new RESOLVE_REF_NO_RECURSE
flag and passing it when appropriate.
Callers can still read the value of a symref (for example to print a
message about it) with that flag set --- resolve_ref_unsafe will
resolve one level of symrefs and stop there.
Signed-off-by: Jonathan Nieder
Reviewed-by: Ronnie Sahlberg
---
Since v21:
- renamed flag from RESOLVE_REF_NO_DEREF to _NO_RECURSE
- more detail in API docs
- only set NO_RECURSE when deleting refs. Locking refs for non-deletion
updates needs to recurse to get old_sha1 for the reflog.
- add more tests (for symrefs to refs with bad names, which should also
be deletable now)
builtin/branch.c | 3 ++-
cache.h | 6 ++
refs.c| 17 +++--
refs.h| 2 ++
t/t1400-update-ref.sh | 34 ++
t/t3200-branch.sh | 9 +
6 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index e5d1377..a334380 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -234,7 +234,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, 0, sha1, &flags);
+ target = resolve_ref_unsafe(name, RESOLVE_REF_NO_RECURSE,
+ sha1, &flags);
if (!target ||
(!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {
error(remote_branch
diff --git a/cache.h b/cache.h
index 5b54911..5ca7f2b 100644
--- a/cache.h
+++ b/cache.h
@@ -970,6 +970,11 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* "writing" to the ref, the return value is the name of the ref
* that will actually be created or changed.
*
+ * If the RESOLVE_REF_NO_RECURSE flag is passed, only resolves one
+ * level of symbolic reference. The value stored in sha1 for a symbolic
+ * reference will always be null_sha1 in this case, and the return
+ * value is the reference that the symref refers to directly.
+ *
* If flags is non-NULL, set the value that it points to the
* combination of REF_ISPACKED (if the reference was found among the
* packed references), REF_ISSYMREF (if the initial reference was a
@@ -982,6 +987,7 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* errno is set to something meaningful on error.
*/
#define RESOLVE_REF_READING 0x01
+#define RESOLVE_REF_NO_RECURSE 0x02
extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags,
unsigned char *sha1, int *flags);
extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char
*sha1, int *flags);
diff --git a/refs.c b/refs.c
index 4916d16..490e788 100644
--- a/refs.c
+++ b/refs.c
@@ -1407,6 +1407,10 @@ const char *resolve_ref_unsafe(const char *refname, int
resolve_flags, unsigned
refname = refname_buffer;
if (flags)
*flags |= REF_ISSYMREF;
+ if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
+ hashclr(sha1);
+ return refname;
+ }
continue;
}
}
@@ -1463,13 +1467,17 @@ const char *resolve_ref_unsafe(const char *refname, int
resolve_flags, unsigned
buf = buffer + 4;
while (isspace(*buf))
buf++;
+ refname = strcpy(refname_buffer, buf);
+ if (resolve_flags & RESOLVE_REF_NO_RECURSE) {
+ hashclr(sha1);
+ return refname;
+ }
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
if (flags)
*flags |= REF_ISBROKEN;
errno = EINVAL;
return NULL;
}
- refname = strcpy(refname_buffer, buf);
}
}
@@ -2111,6 +2119,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (mustexist)
resolve_flags |= RESOLVE_REF_READING;
+ if (flags & REF_NODEREF && flags & REF_DELETING)
+ resolve_flags |= RE
[PATCH 16/24] branch -d: simplify by using RESOLVE_REF_READING flag
From: Ronnie Sahlberg
Date: Thu, 11 Sep 2014 10:34:36 -0700
When "git branch -d" reads the branch it is about to delete, it used
to avoid passing the RESOLVE_REF_READING ('treat missing ref as
error') flag because a symref pointing to a nonexistent ref would show
up as missing instead of as something that could be deleted. To check
if a ref is actually missing, we then check
- is it a symref?
- if not, did it resolve to null_sha1?
Now we pass RESOLVE_REF_NO_RECURSE and the correct information is
returned for a symref even when it points to a missing ref. Simplify
by relying on RESOLVE_REF_READING.
No functional change intended.
Signed-off-by: Jonathan Nieder
---
Split out from the 'fix handling of badly named refs' patch.
builtin/branch.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index a334380..a0c5601 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -234,10 +234,11 @@ static int delete_branches(int argc, const char **argv,
int force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, RESOLVE_REF_NO_RECURSE,
+ target = resolve_ref_unsafe(name,
+ RESOLVE_REF_READING
+ | RESOLVE_REF_NO_RECURSE,
sha1, &flags);
- if (!target ||
- (!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {
+ if (!target) {
error(remote_branch
? _("remote branch '%s' not found.")
: _("branch '%s' not found."), bname.buf);
--
2.1.0.rc2.206.gedb03e5
--
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/24] packed-ref cache: forbid dot-components in refnames
Since v1.7.9-rc1~10^2 (write_head_info(): handle "extra refs" locally,
2012-01-06), this trick to keep track of ".have" refs that are only
valid on the wire and not on the filesystem is not needed any more.
Simplify by removing support for the REFNAME_DOT_COMPONENT flag.
This means we'll be slightly stricter with invalid refs found in a
packed-refs file or during clone. read_loose_refs() already checks
for and skips refnames with .components so it is not affected.
Signed-off-by: Jonathan Nieder
Reviewed-by: Ronnie Sahlberg
---
Noticed while reviewing other patches.
refs.c | 14 +++---
refs.h | 6 +-
2 files changed, 4 insertions(+), 16 deletions(-)
diff --git a/refs.c b/refs.c
index 490e788..94d6d89 100644
--- a/refs.c
+++ b/refs.c
@@ -69,16 +69,8 @@ static int check_refname_component(const char *refname, int
flags)
out:
if (cp == refname)
return 0; /* Component has zero length. */
- if (refname[0] == '.') {
- if (!(flags & REFNAME_DOT_COMPONENT))
- return -1; /* Component starts with '.'. */
- /*
-* Even if leading dots are allowed, don't allow "."
-* as a component (".." is prevented by a rule above).
-*/
- if (refname[1] == '\0')
- return -1; /* Component equals ".". */
- }
+ if (refname[0] == '.')
+ return -1; /* Component starts with '.'. */
if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
return -1; /* Refname ends with ".lock". */
return cp - refname;
@@ -288,7 +280,7 @@ static struct ref_entry *create_ref_entry(const char
*refname,
struct ref_entry *ref;
if (check_name &&
- check_refname_format(refname,
REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
+ check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
die("Reference has invalid format: '%s'", refname);
len = strlen(refname) + 1;
ref = xmalloc(sizeof(struct ref_entry) + len);
diff --git a/refs.h b/refs.h
index b62c4c4..e48d8b9 100644
--- a/refs.h
+++ b/refs.h
@@ -226,7 +226,6 @@ extern int for_each_reflog(each_ref_fn, void *);
#define REFNAME_ALLOW_ONELEVEL 1
#define REFNAME_REFSPEC_PATTERN 2
-#define REFNAME_DOT_COMPONENT 4
/*
* Return 0 iff refname has the correct format for a refname according
@@ -234,10 +233,7 @@ extern int for_each_reflog(each_ref_fn, void *);
* If REFNAME_ALLOW_ONELEVEL is set in flags, then accept one-level
* reference names. If REFNAME_REFSPEC_PATTERN is set in flags, then
* allow a "*" wildcard character in place of one of the name
- * components. No leading or repeated slashes are accepted. If
- * REFNAME_DOT_COMPONENT is set in flags, then allow refname
- * components to start with "." (but not a whole component equal to
- * "." or "..").
+ * components. No leading or repeated slashes are accepted.
*/
extern int check_refname_format(const char *refname, int flags);
--
2.1.0.rc2.206.gedb03e5
--
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 18/24] test: put tests for handling of bad ref names in one place
From: Ronnie Sahlberg There's no straightforward way to grep for all tests dealing with invalid refs. Put them in a single test script so it is easy to see what functionality has not been exercised with bad ref names yet. Signed-off-by: Ronnie Sahlberg Signed-off-by: Jonathan Nieder --- New. t/t1400-update-ref.sh | 44 -- t/t1430-bad-ref-name.sh | 84 + t/t9300-fast-import.sh | 30 -- 3 files changed, 84 insertions(+), 74 deletions(-) create mode 100755 t/t1430-bad-ref-name.sh diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 7c8c41a..7b4707b 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -136,14 +136,6 @@ test_expect_success 'update-ref --no-deref -d can delete reference to bad ref' ' test_path_is_missing .git/refs/heads/ref-to-bad ' -test_expect_success 'update-ref --no-deref -d can delete reference to broken name' ' - git symbolic-ref refs/heads/badname refs/heads/broken...ref && - test_when_finished "rm -f .git/refs/heads/badname" && - test_path_is_file .git/refs/heads/badname && - git update-ref --no-deref -d refs/heads/badname && - test_path_is_missing .git/refs/heads/badname -' - test_expect_success '(not) create HEAD with old sha1' " test_must_fail git update-ref HEAD $A $B " @@ -408,12 +400,6 @@ test_expect_success 'stdin fails create with no ref' ' grep "fatal: create: missing " err ' -test_expect_success 'stdin fails create with bad ref name' ' - echo "create ~a $m" >stdin && - test_must_fail git update-ref --stdin err && - grep "fatal: invalid ref format: ~a" err -' - test_expect_success 'stdin fails create with no new value' ' echo "create $a" >stdin && test_must_fail git update-ref --stdin err && @@ -432,12 +418,6 @@ test_expect_success 'stdin fails update with no ref' ' grep "fatal: update: missing " err ' -test_expect_success 'stdin fails update with bad ref name' ' - echo "update ~a $m" >stdin && - test_must_fail git update-ref --stdin err && - grep "fatal: invalid ref format: ~a" err -' - test_expect_success 'stdin fails update with no new value' ' echo "update $a" >stdin && test_must_fail git update-ref --stdin err && @@ -456,12 +436,6 @@ test_expect_success 'stdin fails delete with no ref' ' grep "fatal: delete: missing " err ' -test_expect_success 'stdin fails delete with bad ref name' ' - echo "delete ~a $m" >stdin && - test_must_fail git update-ref --stdin err && - grep "fatal: invalid ref format: ~a" err -' - test_expect_success 'stdin fails delete with too many arguments' ' echo "delete $a $m $m" >stdin && test_must_fail git update-ref --stdin err && @@ -734,12 +708,6 @@ test_expect_success 'stdin -z fails create with no ref' ' grep "fatal: create: missing " err ' -test_expect_success 'stdin -z fails create with bad ref name' ' - printf $F "create ~a " "$m" >stdin && - test_must_fail git update-ref -z --stdin err && - grep "fatal: invalid ref format: ~a " err -' - test_expect_success 'stdin -z fails create with no new value' ' printf $F "create $a" >stdin && test_must_fail git update-ref -z --stdin err && @@ -764,12 +732,6 @@ test_expect_success 'stdin -z fails update with too few args' ' grep "fatal: update $a: unexpected end of input when reading " err ' -test_expect_success 'stdin -z fails update with bad ref name' ' - printf $F "update ~a" "$m" "" >stdin && - test_must_fail git update-ref -z --stdin err && - grep "fatal: invalid ref format: ~a" err -' - test_expect_success 'stdin -z emits warning with empty new value' ' git update-ref $a $m && printf $F "update $a" "" "" >stdin && @@ -802,12 +764,6 @@ test_expect_success 'stdin -z fails delete with no ref' ' grep "fatal: delete: missing " err ' -test_expect_success 'stdin -z fails delete with bad ref name' ' - printf $F "delete ~a" "$m" >stdin && - test_must_fail git update-ref -z --stdin err && - grep "fatal: invalid ref format: ~a" err -' - test_expect_success 'stdin -z fails delete with no old value' ' printf $F "delete $a" >stdin && test_must_fail git update-ref -z --stdin err && diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh new file mode 100755 index 000..c7b19b0 --- /dev/null +++ b/t/t1430-bad-ref-name.sh @@ -0,0 +1,84 @@ +#!/bin/sh + +test_description='Test handling of ref names that check-ref-format rejects' +. ./test-lib.sh + +test_expect_success setup ' + test_commit one +' + +test_expect_success 'fast-import: fail on invalid branch name ".badbranchname"' ' + test_when_finished "rm -f .git/objects/pack_* .git/objects/index_*" && + cat >input <<-INPUT_END && + commit .badbranchname + committer $
[PATCH 19/24] refs.c: allow listing and deleting badly named refs
From: Ronnie Sahlberg
Date: Wed, 3 Sep 2014 11:45:43 -0700
We currently do not handle badly named refs well:
$ cp .git/refs/heads/master .git/refs/heads/master.@\*@\\.
$ git branch
fatal: Reference has invalid format: 'refs/heads/master.@*@\.'
$ git branch -D master.@\*@\\.
error: branch 'master.@*@\.' not found.
Users cannot recover from a badly named ref without manually finding
and deleting the loose ref file or appropriate line in packed-refs.
Making that easier will make it easier to tweak the ref naming rules
in the future, for example to forbid shell metacharacters like '`'
and '"', without putting people in a state that is hard to get out of.
So allow "git branch --list" to show these refs and to allow "git
branch -d/-D" and "git update-ref -d" to delete them. Other commands
(for example to rename refs) will continue to not handle these refs
but can be changed in later patches.
Details:
In resolving functions, refuse to resolve refs that don't pass the
check-ref-format(1) check unless the new RESOLVE_REF_ALLOW_BAD_NAME
flag is passed. Even with RESOLVE_REF_ALLOW_BAD_NAME, refuse to
resolve refs that escape the refs/ directory and do not match the
pattern [A-Z_]* (think "HEAD" and "MERGE_HEAD").
In locking functions, refuse to act on badly named refs unless they
are being deleted and either are in the refs/ directory or match [A-Z_]*.
Just like other invalid refs, flag resolved, badly named refs with the
REF_ISBROKEN flag, treat them as resolving to null_sha1, and skip them
in all iteration functions except for for_each_rawref.
Flag badly named refs with a REF_BAD_NAME flag to make it easier for
future callers to notice and handle them specially.
In the transaction API, refuse to create or update badly named refs,
but allow deleting them (unless they escape refs/ and don't match
[A-Z_]*).
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
Since v21:
- clarified change description
- handle REF_ISBROKEN case too when printing "Deleted branch" message
- tighten fallback check, so commands like 'git branch -d ../../config'
won't work
- use REF_BAD_NAME so it's easier for callers (like for-each-ref in a
later patch) to notice that we decided a ref's name was bad
- set errno when failing to find ref in the packed-refs file
- consistently don't set REF_ISBROKEN for missing refs
- consistently clear sha1 when successfully resolving a badly named ref
- treat refs with bad names encountered as part of symref resolution the
same as such refs explicitly named
- always allow safe bad names when trying to lock a ref for deletion
(though some callers don't get that far yet)
- clearer API docs, more tests
builtin/branch.c| 9 +--
cache.h | 14 -
refs.c | 148 ++--
refs.h | 8 ++-
t/t1430-bad-ref-name.sh | 125 +++-
5 files changed, 265 insertions(+), 39 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index a0c5601..94aaea1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -236,7 +236,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
name = mkpathdup(fmt, bname.buf);
target = resolve_ref_unsafe(name,
RESOLVE_REF_READING
- | RESOLVE_REF_NO_RECURSE,
+ | RESOLVE_REF_NO_RECURSE
+ | RESOLVE_REF_ALLOW_BAD_NAME,
sha1, &flags);
if (!target) {
error(remote_branch
@@ -246,7 +247,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
continue;
}
- if (!(flags & REF_ISSYMREF) &&
+ if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
force)) {
ret = 1;
@@ -266,8 +267,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
? _("Deleted remote branch %s (was %s).\n")
: _("Deleted branch %s (was %s).\n"),
bname.buf,
- (flags & REF_ISSYMREF)
- ? target
+ (flags & REF_ISBROKEN) ? "broken"
+ : (flags & REF_ISSYMREF) ? target
: find_unique_abbrev(sha1, DEFAULT_ABBREV));
}
delete_branch_config(bname.buf);
diff --git a/cache.h b/cache.h
index 5ca7f2b..0c0ac60 100644
--- a/cache.h
+++ b/cache.h
@@ -978,16 +978,26 @@ extern int read_ref(const char *refname, unsigned
[PATCH 20/24] for-each-ref.c: improve message before aborting on broken ref
From: Ronnie Sahlberg
Date: Fri, 5 Sep 2014 14:35:17 -0700
Print a warning message for any badly named refs we find in the repo.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Jonathan Nieder
---
As before.
builtin/for-each-ref.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 20949b7..a88d681 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -853,6 +853,11 @@ static int grab_single_ref(const char *refname, const
unsigned char *sha1, int f
struct refinfo *ref;
int cnt;
+ if (flag & REF_BAD_NAME) {
+ warning("ignoring ref with broken name %s", refname);
+ return 0;
+ }
+
if (*cb->grab_pattern) {
const char **pattern;
int namelen = strlen(refname);
--
2.1.0.rc2.206.gedb03e5
--
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
