Re: [PATCH] refs: write packed_refs file using stdio
On Wed, Sep 10, 2014 at 01:21:27PM +0200, Michael Haggerty wrote:
> > + if (fclose(out))
> > + die_errno("write error");
> > + packed_ref_cache->lock->fd = -1;
>
> It might be a minuscule bit safer to set `lock->fd = -1` *before*
> calling `fclose()`.
Yeah, I considered that. The worst case is a signal coming in between
the two calls (or a somebody calling die() between the two :) ). In that
case the lockfile code will close() the fd again, which should be a noop
(since nobody will have opened it in the interim...right?).
That "since" assumption is the dangerous part. But on the other hand, if
we unset the fd first, then the lockfile code may fail to close it if it
is called before the fclose() (definitely by a signal in this case). I
think there are platforms where that would cause us to fail to remove
the file, which is annoying.
So I dunno. We cannot be atomic here. I could go either way.
> TBH, it makes me uncomfortable having code outside of `lockfile.c`
> having this level of intimacy with lockfile objects.
I kind of agree.
> I think it would be better to have a
>
> FILE *fopen_lock_file(struct *lock_file, const char *mode);
>
> that records the `FILE *` inside the `lockfile` instance, and to teach
> `commit_lock_file()` and its friends to call `fclose()` if the `FILE *`
> was created. I think that such a feature would encourage other lockfile
> users to use the more convenient and readable stdio API.
I was tempted by that. We could also do:
if (fflush(out))
die_errno("write error");
if (commit_lock_file(...))
...
which sidesteps the issue. We do then have to _later_ call fclose(out)
to free the handle memory, which is going to want to close the fd again.
Putting us back in the "let's hope nobody opened it in the meantime"
case from above.
-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] refs: write packed_refs file using stdio
On 09/10/2014 12:03 PM, Jeff King wrote:
> We write each line of a new packed-refs file individually
> using a write() syscall (and sometimes 2, if the ref is
> peeled). Since each line is only about 50-100 bytes long,
> this creates a lot of system call overhead.
>
> We can instead open a stdio handle around our descriptor and
> use fprintf to write to it. The extra buffering is not a
> problem for us, because nobody will read our new packed-refs
> file until we call commit_lock_file (by which point we have
> flushed everything).
>
> On a pathological repository with 8.5 million refs, this
> dropped the time to run `git pack-refs` from 20s to 6s.
>
> Signed-off-by: Jeff King
> ---
> Obviously that repo is ridiculous (but a sad reality for me).
>
> However, I think the benefits extend to smaller files, too. And it's
> pretty easy to do (and I actually think the resulting write_packed_entry
> is a lot easier to read, as well as lifting some arbitrary limits).
>
> cache.h| 2 ++
> refs.c | 39 ---
> write_or_die.c | 15 +++
> 3 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 4d5b76c..bc286ce 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1395,6 +1395,8 @@ extern const char *git_mailmap_blob;
>
> /* IO helper functions */
> extern void maybe_flush_or_die(FILE *, const char *);
> +__attribute__((format (printf, 2, 3)))
> +extern void fprintf_or_die(FILE *, const char *fmt, ...);
> extern int copy_fd(int ifd, int ofd);
> extern int copy_file(const char *dst, const char *src, int mode);
> extern int copy_file_with_time(const char *dst, const char *src, int mode);
> diff --git a/refs.c b/refs.c
> index 27927f2..f08faed 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2191,25 +2191,12 @@ struct ref_lock *lock_any_ref_for_update(const char
> *refname,
> * Write an entry to the packed-refs file for the specified refname.
> * If peeled is non-NULL, write it as the entry's peeled value.
> */
> -static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
> +static void write_packed_entry(FILE *fh, char *refname, unsigned char *sha1,
> unsigned char *peeled)
> {
> - char line[PATH_MAX + 100];
> - int len;
> -
> - len = snprintf(line, sizeof(line), "%s %s\n",
> -sha1_to_hex(sha1), refname);
> - /* this should not happen but just being defensive */
> - if (len > sizeof(line))
> - die("too long a refname '%s'", refname);
> - write_or_die(fd, line, len);
> -
> - if (peeled) {
> - if (snprintf(line, sizeof(line), "^%s\n",
> - sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)
> - die("internal error");
> - write_or_die(fd, line, PEELED_LINE_LENGTH);
> - }
> + fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
> + if (peeled)
> + fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
> }
>
> /*
> @@ -2217,13 +2204,12 @@ static void write_packed_entry(int fd, char *refname,
> unsigned char *sha1,
> */
> static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
> {
> - int *fd = cb_data;
> enum peel_status peel_status = peel_entry(entry, 0);
>
> if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
> error("internal error: %s is not a valid packed reference!",
> entry->name);
> - write_packed_entry(*fd, entry->name, entry->u.value.sha1,
> + write_packed_entry(cb_data, entry->name, entry->u.value.sha1,
> peel_status == PEEL_PEELED ?
> entry->u.value.peeled : NULL);
> return 0;
> @@ -2259,15 +2245,22 @@ int commit_packed_refs(void)
> get_packed_ref_cache(&ref_cache);
> int error = 0;
> int save_errno = 0;
> + FILE *out;
>
> if (!packed_ref_cache->lock)
> die("internal error: packed-refs not locked");
> - write_or_die(packed_ref_cache->lock->fd,
> - PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
>
> + out = fdopen(packed_ref_cache->lock->fd, "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,
> - &packed_ref_cache->lock->fd);
> + 0, write_packed_entry_fn, out);
> + if (fclose(out))
> + die_errno("write error");
> + packed_ref_cache->lock->fd = -1;
It might be a minuscule bit safer to set `lock->fd = -1` *before*
calling `fclose()`.
TBH, it makes me uncomfortable having code outside of `lockfile.c`
having this level of intimacy with lockfile objects. I think it would be
better to have a
[PATCH v2] refs: speed up is_refname_available
Our filesystem ref storage does not allow D/F conflicts; so
if "refs/heads/a/b" exists, we do not allow "refs/heads/a"
to exist (and vice versa). This falls out naturally for
loose refs, where the filesystem enforces the condition. But
for packed-refs, we have to make the check ourselves.
We do so by iterating over the entire packed-refs namespace
and checking whether each name creates a conflict. If you
have a very large number of refs, this is quite inefficient,
as you end up doing a large number of comparisons with
uninteresting bits of the ref tree (e.g., we know that all
of "refs/tags" is uninteresting in the example above, yet we
check each entry in it).
Instead, let's take advantage of the fact that we have the
packed refs stored as a trie of ref_entry structs. We can
find each component of the proposed refname as we walk
through the trie, checking for D/F conflicts as we go. For a
refname of depth N (i.e., 4 in the above example), we only
have to visit N nodes. And at each visit, we can binary
search the M names at that level, for a total complexity of
O(N lg M). ("M" is different at each level, of course, but
we can take the worst-case "M" as a bound).
In a pathological case of fetching 30,000 fresh refs into a
repository with 8.5 million refs, this dropped the time to
run "git fetch" from tens of minutes to ~30s.
This may also help smaller cases in which we check against
loose refs (which we do when renaming a ref), as we may
avoid a disk access for unrelated loose directories.
Note that the tests we add appear at first glance to be
redundant with what is already in t3210. However, the early
tests are not robust; they are run with reflogs turned on,
meaning that we are not actually testing
is_refname_available at all! The operations will still fail
because the reflogs will hit D/F conflicts in the
filesystem. To get a true test, we must turn off reflogs
(but we don't want to do so for the entire script, because
the point of turning them on was to cover some other cases).
Reviewed-by: Michael Haggerty
Signed-off-by: Jeff King
---
Sorry for the quick v2; Michael and I crossed emails off-list, and I
missed some of his review. This version has some minor style and comment
fixups.
refs.c | 122 +--
t/t3210-pack-refs.sh | 31 -
2 files changed, 120 insertions(+), 33 deletions(-)
diff --git a/refs.c b/refs.c
index 27927f2..eb2262a 100644
--- a/refs.c
+++ b/refs.c
@@ -779,37 +779,32 @@ static void prime_ref_dir(struct ref_dir *dir)
prime_ref_dir(get_ref_dir(entry));
}
}
-/*
- * Return true iff refname1 and refname2 conflict with each other.
- * Two reference names conflict if one of them exactly matches the
- * leading components of the other; e.g., "foo/bar" conflicts with
- * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
- * "foo/barbados".
- */
-static int names_conflict(const char *refname1, const char *refname2)
+
+static int entry_matches(struct ref_entry *entry, const char *refname)
{
- for (; *refname1 && *refname1 == *refname2; refname1++, refname2++)
- ;
- return (*refname1 == '\0' && *refname2 == '/')
- || (*refname1 == '/' && *refname2 == '\0');
+ return refname && !strcmp(entry->name, refname);
}
-struct name_conflict_cb {
- const char *refname;
- const char *oldrefname;
- const char *conflicting_refname;
+struct nonmatching_ref_data {
+ const char *skip;
+ struct ref_entry *found;
};
-static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
+static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
{
- struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
- if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
+ struct nonmatching_ref_data *data = vdata;
+
+ if (entry_matches(entry, data->skip))
return 0;
- if (names_conflict(data->refname, entry->name)) {
- data->conflicting_refname = entry->name;
- return 1;
- }
- return 0;
+
+ data->found = entry;
+ return 1;
+}
+
+static void report_refname_conflict(struct ref_entry *entry,
+ const char *refname)
+{
+ error("'%s' exists; cannot create '%s'", entry->name, refname);
}
/*
@@ -818,21 +813,84 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
* operation).
+ *
+ * Two reference names conflict if one of them exactly matches the
+ * leading components of the other; e.g., "foo/bar" conflicts with
+ * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
+ * "foo/barbados".
*/
static int is_refname_available(const char *refname, const char *oldrefname,
Re: [PATCH v4 00/32] Lockfile correctness and refactoring
On Wed, Sep 10, 2014 at 05:25:36PM +0700, Duy Nguyen wrote: > On Wed, Sep 10, 2014 at 3:13 PM, Jeff King wrote: > > I was running pack-refs on a repository with a very large number of > > loose refs (about 1.8 million). Needless to say, this ran very slowly > > and thrashed the disk, as that's almost 7G using 4K inodes. But it did > > eventually generate a packed-refs file, at which point it tried to prune > > the loose refs. > > Urghh.. ref advertisment for fetch/push would be unbelievably large. > Gotta look at the "git protocol v2" again soon.. Yes, we don't let normal fetchers see these repos. They're only for holding shared objects and the ref tips to keep them reachable. So we never fetch from them, even locally. We only fetch to them from normal repos (and never push to or from them at all). It's still rather painful just to do normal things, though. Every git operation loads the whole packed-refs file into memory. I'm biding my time on the ref-backend patches that are being worked on. :) -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 v4 00/32] Lockfile correctness and refactoring
On Wed, Sep 10, 2014 at 3:13 PM, Jeff King wrote: > I was running pack-refs on a repository with a very large number of > loose refs (about 1.8 million). Needless to say, this ran very slowly > and thrashed the disk, as that's almost 7G using 4K inodes. But it did > eventually generate a packed-refs file, at which point it tried to prune > the loose refs. Urghh.. ref advertisment for fetch/push would be unbelievably large. Gotta look at the "git protocol v2" again soon.. -- Duy -- 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] refs: speed up is_refname_available
Our filesystem ref storage does not allow D/F conflicts; so
if "refs/heads/a/b" exists, we do not allow "refs/heads/a"
to exist (and vice versa). This falls out naturally for
loose refs, where the filesystem enforces the condition. But
for packed-refs, we have to make the check ourselves.
We do so by iterating over the entire packed-refs namespace
and checking whether each name creates a conflict. If you
have a very large number of refs, this is quite inefficient,
as you end up doing a large number of comparisons with
uninteresting bits of the ref tree (e.g., we know that all
of "refs/tags" is uninteresting in the example above, yet we
check each entry in it).
Instead, let's take advantage of the fact that we have the
packed refs stored as a trie of ref_entry structs. We can
find each component of the proposed refname as we walk
through the trie, checking for D/F conflicts as we go. For a
refname of depth N (i.e., 4 in the above example), we only
have to visit N nodes. And at each visit, we can binary
search the M names at that level, for a total complexity of
O(N lg M). ("M" is different at each level, of course, but
we can take the worst-case "M" as a bound).
In a pathological case of fetching 30,000 fresh refs into a
repository with 8.5 million refs, this dropped the time to
run "git fetch" from tens of minutes to ~30s.
This may also help smaller cases in which we check against
loose refs (which we do when renaming a ref), as we may
avoid a disk access for unrelated loose directories.
Note that the tests we add appear at first glance to be
redundant with what is already in t3210. However, the early
tests are not robust; they are run with reflogs turned on,
meaning that we are not actually testing
is_refname_available at all! The operations will still fail
because the reflogs will hit D/F conflicts in the
filesystem. To get a true test, we must turn off reflogs
(but we don't want to do so for the entire script, because
the point of turning them on was to cover some other cases).
Signed-off-by: Jeff King
---
The diff's a bit hard to read, because it keeps useless lines like "{"
as context. I thought histogram and patience were supposed to be more
readable there, but they seem to produce the same thing.
This has a fairly straightforward conflict with the ref-transaction
stuff in pu. The "oldrefname" parameter to is_refname_available became a
list of items; we resolve it by teaching entry_matches to do the same
here. I pushed a proposed resolution up to:
git://github.com/peff/git.git resolution-refname-available
refs.c | 123 +--
t/t3210-pack-refs.sh | 31 -
2 files changed, 121 insertions(+), 33 deletions(-)
diff --git a/refs.c b/refs.c
index 27927f2..4e5639c 100644
--- a/refs.c
+++ b/refs.c
@@ -779,37 +779,32 @@ static void prime_ref_dir(struct ref_dir *dir)
prime_ref_dir(get_ref_dir(entry));
}
}
-/*
- * Return true iff refname1 and refname2 conflict with each other.
- * Two reference names conflict if one of them exactly matches the
- * leading components of the other; e.g., "foo/bar" conflicts with
- * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
- * "foo/barbados".
- */
-static int names_conflict(const char *refname1, const char *refname2)
+
+static int entry_matches(struct ref_entry *entry, const char *refname)
{
- for (; *refname1 && *refname1 == *refname2; refname1++, refname2++)
- ;
- return (*refname1 == '\0' && *refname2 == '/')
- || (*refname1 == '/' && *refname2 == '\0');
+ return refname && !strcmp(entry->name, refname);
}
-struct name_conflict_cb {
- const char *refname;
- const char *oldrefname;
- const char *conflicting_refname;
+struct nonmatching_ref_data {
+ const char *skip;
+ struct ref_entry *found;
};
-static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
+static int nonmatching_ref_fn(struct ref_entry *entry, void *vdata)
{
- struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
- if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
+ struct nonmatching_ref_data *data = vdata;
+
+ if (entry_matches(entry, data->skip))
return 0;
- if (names_conflict(data->refname, entry->name)) {
- data->conflicting_refname = entry->name;
- return 1;
- }
- return 0;
+
+ data->found = entry;
+ return 1;
+}
+
+static void report_refname_conflict(struct ref_entry *entry,
+ const char *refname)
+{
+ error("'%s' exists; cannot create '%s'", entry->name, refname);
}
/*
@@ -818,21 +813,85 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
* operation).
[PATCH] refs: write packed_refs file using stdio
We write each line of a new packed-refs file individually
using a write() syscall (and sometimes 2, if the ref is
peeled). Since each line is only about 50-100 bytes long,
this creates a lot of system call overhead.
We can instead open a stdio handle around our descriptor and
use fprintf to write to it. The extra buffering is not a
problem for us, because nobody will read our new packed-refs
file until we call commit_lock_file (by which point we have
flushed everything).
On a pathological repository with 8.5 million refs, this
dropped the time to run `git pack-refs` from 20s to 6s.
Signed-off-by: Jeff King
---
Obviously that repo is ridiculous (but a sad reality for me).
However, I think the benefits extend to smaller files, too. And it's
pretty easy to do (and I actually think the resulting write_packed_entry
is a lot easier to read, as well as lifting some arbitrary limits).
cache.h| 2 ++
refs.c | 39 ---
write_or_die.c | 15 +++
3 files changed, 33 insertions(+), 23 deletions(-)
diff --git a/cache.h b/cache.h
index 4d5b76c..bc286ce 100644
--- a/cache.h
+++ b/cache.h
@@ -1395,6 +1395,8 @@ extern const char *git_mailmap_blob;
/* IO helper functions */
extern void maybe_flush_or_die(FILE *, const char *);
+__attribute__((format (printf, 2, 3)))
+extern void fprintf_or_die(FILE *, const char *fmt, ...);
extern int copy_fd(int ifd, int ofd);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
diff --git a/refs.c b/refs.c
index 27927f2..f08faed 100644
--- a/refs.c
+++ b/refs.c
@@ -2191,25 +2191,12 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
* Write an entry to the packed-refs file for the specified refname.
* If peeled is non-NULL, write it as the entry's peeled value.
*/
-static void write_packed_entry(int fd, char *refname, unsigned char *sha1,
+static void write_packed_entry(FILE *fh, char *refname, unsigned char *sha1,
unsigned char *peeled)
{
- char line[PATH_MAX + 100];
- int len;
-
- len = snprintf(line, sizeof(line), "%s %s\n",
- sha1_to_hex(sha1), refname);
- /* this should not happen but just being defensive */
- if (len > sizeof(line))
- die("too long a refname '%s'", refname);
- write_or_die(fd, line, len);
-
- if (peeled) {
- if (snprintf(line, sizeof(line), "^%s\n",
-sha1_to_hex(peeled)) != PEELED_LINE_LENGTH)
- die("internal error");
- write_or_die(fd, line, PEELED_LINE_LENGTH);
- }
+ fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
+ if (peeled)
+ fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
}
/*
@@ -2217,13 +2204,12 @@ static void write_packed_entry(int fd, char *refname,
unsigned char *sha1,
*/
static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
{
- int *fd = cb_data;
enum peel_status peel_status = peel_entry(entry, 0);
if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
error("internal error: %s is not a valid packed reference!",
entry->name);
- write_packed_entry(*fd, entry->name, entry->u.value.sha1,
+ write_packed_entry(cb_data, entry->name, entry->u.value.sha1,
peel_status == PEEL_PEELED ?
entry->u.value.peeled : NULL);
return 0;
@@ -2259,15 +2245,22 @@ int commit_packed_refs(void)
get_packed_ref_cache(&ref_cache);
int error = 0;
int save_errno = 0;
+ FILE *out;
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
- write_or_die(packed_ref_cache->lock->fd,
-PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
+ out = fdopen(packed_ref_cache->lock->fd, "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,
-&packed_ref_cache->lock->fd);
+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;
error = -1;
diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..e7afe7a 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -49,6 +49,21 @@ void maybe_flush_or_die(FILE *f, const char *desc)
}
}
+void fprintf_or_die(FILE *f, const char *fmt, ...)
+{
+ va_list ap;
+ int ret;
+
+
Re: [PATCH v4 00/32] Lockfile correctness and refactoring
On Sat, Sep 06, 2014 at 09:50:14AM +0200, Michael Haggerty wrote: > Sorry for the long delay since v3. This version mostly cleans up a > couple more places where the lockfile object was left in an > ill-defined state. Thanks to Johannes Sixt and Torsten Bögershausen > for their review of v3. > > I believe that this series addresses all of the comments from v1 [1], > v2 [2], and v3 [3]. This looks pretty good to me overall. I did coincidentally have an interesting experience with our lockfile code earlier today, which I'd like to relate. I was running pack-refs on a repository with a very large number of loose refs (about 1.8 million). Needless to say, this ran very slowly and thrashed the disk, as that's almost 7G using 4K inodes. But it did eventually generate a packed-refs file, at which point it tried to prune the loose refs. To do so, we have to lock each ref before removing it (to protect against a simultaneous update). Each call to lock_ref_sha1_basic allocates a "struct lock_file", which then gets added to the global lock_file list. Each one contains a fixed PATH_MAX buffer (4K on this machine). After we're done updating the ref, we leak the lock_file struct, since there's no way to remove it from the list. As a result, git tried to allocate 7G of RAM and got OOM-killed (the machine had only 8G). In addition to thrashing the disk even harder, since there was no room left for disk cache while we touched millions of loose refs. :) Your change in this series to use a strbuf would make this a lot better. But I still wonder how hard it would be to just remove lock_file structs from the global list when they are committed or rolled back. That would presumably also make the "avoid transitory valid states" patch from your series a bit easier, too (you could prepare the lockfile in peace, and then link it in fully formed, and do the opposite when removing it). I think with your strbuf patch, this leak at least becomes reasonable. So maybe it's not worth going further. But I'd be interested to hear your thoughts since you've been touching the area recently. -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 v4 19/32] commit_lock_file(): rollback lock file on failure to rename
On Sat, Sep 06, 2014 at 09:50:33AM +0200, Michael Haggerty wrote: > 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. Unlike the previous patch, in this case the contents of the lockfile _are_ defined. So in theory a caller could somehow retry. I don't see any callers that want to do that, though (and besides, they would not know if the error came from the close or the rename), so I think we can consider that an uninteresting case until somebody creates such a caller (at which point they can take responsibility for extending the API). BTW, while grepping for commit_lock_file calls, I notice we often commit the shallow file without checking the return code. I'm not sure what we should do in each case, but I imagine that calling die() is probably better than continuing as if it succeeded. +cc Duy -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
