Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git
Junio C Hamano wrote: We may want to see it done the other way around, though. That is, to allow the pack-refs race fix, which was reviewed and has been cooking, graduate without having to depend on this API rewrite, so that we may be able to merge it down even to v2.2.x maintenance track. Good idea. When I send out the reroll of this API change today, it will be based on top of master + that patch. Thanks, Jonathan -- 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 06/14] lockfile: introduce flag for locks outside .git
Jonathan Nieder jrnie...@gmail.com writes: diff --git a/refs.c b/refs.c index 917f8fc..39e43cf 100644 --- a/refs.c +++ b/refs.c @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, */ goto retry; else - unable_to_lock_die(ref_file, errno); + unable_to_lock_die(ref_file, lflags, errno); } This has unfortunate interaction with 06839515 (lock_ref_sha1_basic: do not die on locking errors, 2014-11-19). The fact that the helper unable-to-lock-message() is now hidden inside lockfile.c does not help, either. -- 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 06/14] lockfile: introduce flag for locks outside .git
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: diff --git a/refs.c b/refs.c index 917f8fc..39e43cf 100644 --- a/refs.c +++ b/refs.c @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, */ goto retry; else -unable_to_lock_die(ref_file, errno); +unable_to_lock_die(ref_file, lflags, errno); } This has unfortunate interaction with 06839515 (lock_ref_sha1_basic: do not die on locking errors, 2014-11-19). I'll add that patch to the series. 06839515 becomes a little simpler with the updated API. Thanks, Jonathan -- 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 06/14] lockfile: introduce flag for locks outside .git
Junio C Hamano gits...@pobox.com writes: Jonathan Nieder jrnie...@gmail.com writes: diff --git a/refs.c b/refs.c index 917f8fc..39e43cf 100644 --- a/refs.c +++ b/refs.c @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, */ goto retry; else -unable_to_lock_die(ref_file, errno); +unable_to_lock_die(ref_file, lflags, errno); } This has unfortunate interaction with 06839515 (lock_ref_sha1_basic: do not die on locking errors, 2014-11-19). The fact that the helper unable-to-lock-message() is now hidden inside lockfile.c does not help, either. I tried to merge the 14-patch series with obvious fix-ups after dropping the rerere abortion change you sent separately and in duplicate and also dropping sb/copy-fd, but I've ran out of patience with this step, at least for today's integration cycle. Should we also drop jk/lock-ref-sha1-basec-return-errors topic as well? The 14-patch series may have been internally consistent and its individual patches, when each of them was taken alone by itself, may have made sense, but it appears that the aggregated whole these separate topics took their root from is inconsistent with itself in minor ways like this here and there X-. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git
Junio C Hamano wrote: I tried to merge the 14-patch series with obvious fix-ups after dropping the rerere abortion change you sent separately and in duplicate and also dropping sb/copy-fd, but I've ran out of patience with this step, at least for today's integration cycle. Should we also drop jk/lock-ref-sha1-basec-return-errors topic as well? I don't mind adding it to the series. Or feel free to hold off on picking up this 14-patch series until tomorrow's reroll. (Helping with today's integration was why I was thinking of re-sending. I should have been clearer about saying that I prefer you don't pick this up yet when I decided to give reviewers a little more time.) The 14-patch series may have been internally consistent and its individual patches, when each of them was taken alone by itself, may have made sense, but it appears that the aggregated whole these separate topics took their root from is inconsistent with itself in minor ways like this here and there X-. I don't follow. It's normal for an API change to affect code that uses the API --- what inconsistency are you talking about here? Puzzled, Jonathan -- 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 06/14] lockfile: introduce flag for locks outside .git
Jonathan Nieder jrnie...@gmail.com writes: The 14-patch series may have been internally consistent and its individual patches, when each of them was taken alone by itself, may have made sense, but it appears that the aggregated whole these separate topics took their root from is inconsistent with itself in minor ways like this here and there X-. I don't follow. It's normal for an API change to affect code that uses the API --- what inconsistency are you talking about here? I was under the impression that the purpose of the series was to propose an API update to be used together with the remainder of the system, not just update code in master, breaking unstated set of topics and leaving them behind without updating them for now. Such a series is perfectly fine as a feeler to see if people are happy with the updated API that it does not completely cover the topics in flight, but I wouldn't have had to waste time trying to figure out what you are doing differently to cause other topics in flight to break if such a feeler series were labeled clearly as RFC/PATCH, and/or these other topics need to be adjusted to this new API when this series settles, or somesuch. -- 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 06/14] lockfile: introduce flag for locks outside .git
Junio C Hamano gits...@pobox.com writes: Jonathan Nieder jrnie...@gmail.com writes: The 14-patch series may have been internally consistent and its individual patches, when each of them was taken alone by itself, may have made sense, but it appears that the aggregated whole these separate topics took their root from is inconsistent with itself in minor ways like this here and there X-. I don't follow. It's normal for an API change to affect code that uses the API --- what inconsistency are you talking about here? I was under the impression that the purpose of the series was to propose an API update to be used together with the remainder of the system, not just update code in master, breaking unstated set of topics and leaving them behind without updating them for now. ... About the inconsistent with itself bit. The other topic is a piece from the long-ish series by Ronnie, and somehow I had assumed this 14-patch series also originated from another part of that series, especially given that it is largely about refs. That's where itself came from. Looking at it again, this errno thing is more or less separate and independent, so your I don't follow is perfectly understandable to me now. Sorry about being grumpy. -- 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 06/14] lockfile: introduce flag for locks outside .git
Junio C Hamano wrote: I was under the impression that the purpose of the series was to propose an API update to be used together with the remainder of the system, not just update code in master, breaking unstated set of topics and leaving them behind without updating them for now. Got it --- before the reroll I will try a test-merge against 'next' and 'pu' (and likewise when sending future patches). -- 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 06/14] lockfile: introduce flag for locks outside .git
On Wed, Dec 03, 2014 at 03:29:51PM -0800, Jonathan Nieder wrote: Junio C Hamano wrote: I tried to merge the 14-patch series with obvious fix-ups after dropping the rerere abortion change you sent separately and in duplicate and also dropping sb/copy-fd, but I've ran out of patience with this step, at least for today's integration cycle. Should we also drop jk/lock-ref-sha1-basec-return-errors topic as well? I don't mind adding it to the series. Please do add it in, rather than dropping it. It fixes a real racy condition in which pack-refs --prune wants to lock a ref only to prune it, and it's OK to continue on if the lock fails (without that patch, pack-refs dies prematurely). -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 06/14] lockfile: introduce flag for locks outside .git
Jonathan Nieder jrnie...@gmail.com writes: Junio C Hamano wrote: I tried to merge the 14-patch series with obvious fix-ups after dropping the rerere abortion change you sent separately and in duplicate and also dropping sb/copy-fd, but I've ran out of patience with this step, at least for today's integration cycle. Should we also drop jk/lock-ref-sha1-basec-return-errors topic as well? I don't mind adding it to the series We may want to see it done the other way around, though. That is, to allow the pack-refs race fix, which was reviewed and has been cooking, graduate without having to depend on this API rewrite, so that we may be able to merge it down even to v2.2.x maintenance track. 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
[PATCH 06/14] lockfile: introduce flag for locks outside .git
When the lockfile API was introduced, it was only used for the index
file and error messages like
fatal: Unable to create '/path/to/foo.lock': File exists.
If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
were appropriate advice for that case.
Nowadays, the lockfile API is used for other files that need similar
lock-then-update semantics, including files not associated to any
repository, such as /etc/gitconfig, ../my.bundle, and /tmp/temp.marks.
Add a flag that makes the message stop referring to this repository
for such cases.
This should make it possible to print nicer error messages from
such non-core users of the lockfile API.
This introduces the flag. Later patches will use it.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Documentation/technical/api-lockfile.txt | 12
builtin/update-index.c | 2 +-
lockfile.c | 26 +-
lockfile.h | 5 +++--
refs.c | 4 ++--
5 files changed, 35 insertions(+), 14 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt
b/Documentation/technical/api-lockfile.txt
index 93b5f23..fa60cfd 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -124,6 +124,18 @@ LOCK_NO_DEREF::
for backwards-compatibility reasons can be a symbolic link
containing the name of the referred-to-reference.
+LOCK_OUTSIDE_REPOSITORY:
+
+ When the .lock file being created already exists, the error
+ message usually explains that another git process must have
+ crashed in the same Git repository. If `LOCK_OUTSIDE_REPOSITORY`
+ is set, then the message is replaced with something more
+ appropriate when updating files that are not inside a
+ repository.
++
+For example, this flag should be set when locking a configuration
+file in the user's home directory.
+
LOCK_DIE_ON_ERROR::
If a lock is already taken for the file, `die()` with an error
diff --git a/builtin/update-index.c b/builtin/update-index.c
index b0e3dc9..56abd18 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -943,7 +943,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_die(get_index_file(), lock_error);
+ unable_to_lock_die(get_index_file(), 0, lock_error);
}
if (write_locked_index(the_index, lock_file, COMMIT_LOCK))
die(Unable to write new index file);
diff --git a/lockfile.c b/lockfile.c
index 8685c68..102584f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -149,24 +149,32 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return lk-fd;
}
-void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
+void unable_to_lock_message(const char *path, int flags, int err,
+ struct strbuf *buf)
{
- if (err == EEXIST) {
+ if (err != EEXIST) {
+ strbuf_addf(buf, Unable to create '%s.lock': %s,
+ absolute_path(path), strerror(err));
+ } else if (flags LOCK_OUTSIDE_REPOSITORY) {
+ strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
+ If no other git process is currently running, this
probably means\n
+ another git process crashed earlier. Make sure no other
git process\n
+ is running and remove the file manually to continue.,
+ absolute_path(path), strerror(err));
+ } else {
strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
If no other git process is currently running, this
probably means a\n
git process crashed in this repository earlier. Make sure
no other git\n
process is running and remove the file manually to
continue.,
absolute_path(path), strerror(err));
- } else
- strbuf_addf(buf, Unable to create '%s.lock': %s,
- absolute_path(path), strerror(err));
+ }
}
-NORETURN void unable_to_lock_die(const char *path, int err)
+NORETURN void unable_to_lock_die(const char *path, int flags, int err)
{
struct strbuf buf = STRBUF_INIT;
- unable_to_lock_message(path, err, buf);
+ unable_to_lock_message(path, flags, err, buf);
die(%s, buf.buf);
}
@@ -175,7 +183,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const
char *path, int flags)
{
int fd = lock_file(lk, path,
