Re: [PATCH v2 17/25] commit_lock_file(): make committing an unlocked lockfile a NOP
On 04/07/2014 09:31 PM, Jeff King wrote: On Mon, Apr 07, 2014 at 01:33:59AM +0200, Michael Haggerty wrote: 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 guard the file-renaming code with an if statement to change committing an unlocked file into a NOP. [...] Yeah, I would have expected a die(BUG) here. I think it is worth making it a fatal mistake and catching it. Rolling back an uninitialized lockfile is probably OK; we are canceling an operation that never started. But committing a lockfile that we didn't actually fill out could be a sign of a serious error, and we may be propagating a bogus success code. E.g., imagine that receive-pack claims to have written your ref, but actually commit_lock_file was a silent NOP. I'd much rather have it die loudly so we can track down the case. OK, I will change this to a fatal error in v3. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 1/3] rebase: avoid non-function use of return on FreeBSD
Kyle J. McKay mack...@gmail.com writes:
So I suggest that in the interest of fixing rebase on FreeBSD in an
expeditious fashion, patches 1/3 and 2/3 get picked up (see note
below) now and that the follow-on patch below, after being enhanced to
pass all tests, be submitted separately at some future point.
Seems a good plan to me.
Needs-Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
From: Matthieu Moy matthieu@grenoble-inp.fr
Subject: [PATCH 4/3] rebase: stop using . within function
Move the whole run_specific_rebase_internal function to
git-rebase--$type.
The .-ed script defines the complete function, and then the
function is used from the toplevel script.
The goal is to avoid using tricky features that may trigger
bugs on some shells.
The result is simpler, just using the basic pattern:
1. use '. file' to import a set of functions
2. then use these functions
Needs-Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
git-rebase--am.sh | 3 +--
git-rebase--interactive.sh | 3 +--
git-rebase--merge.sh | 3 +--
git-rebase.sh | 40 +---
4 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 2d3f6d55..b48b3e90 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -4,7 +4,7 @@
# Copyright (c) 2010 Junio C Hamano.
#
-git_rebase__am() {
+run_specific_rebase_infile() {
case $action in
continue)
git am --resolved --resolvemsg=$resolvemsg
@@ -75,4 +75,3 @@ git_rebase__am() {
move_to_original_branch
}
-git_rebase__am
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 42164f11..a7670eb0 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -810,7 +810,7 @@ add_exec_commands () {
mv $1.new $1
}
-git_rebase__interactive() {
+run_specific_rebase_infile() {
case $action in
continue)
# do we have anything to commit?
@@ -1044,4 +1044,3 @@ EOF
git update-ref ORIG_HEAD $orig_head
do_rest
}
-git_rebase__interactive
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index b5f05bf5..9550e656 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -101,7 +101,7 @@ finish_rb_merge () {
say All done.
}
-git_rebase__merge() {
+run_specific_rebase_infile() {
case $action in
continue)
read_state
@@ -153,4 +153,3 @@ git_rebase__merge() {
finish_rb_merge
}
-git_rebase__merge
diff --git a/git-rebase.sh b/git-rebase.sh
index 07e2bd48..9e105626 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -175,7 +175,7 @@ run_specific_rebase () {
export GIT_EDITOR
autosquash=
fi
- . git-rebase--$type
+ run_specific_rebase_infile
ret=$?
if test $ret -eq 0
then
@@ -353,6 +353,26 @@ then
die $(gettext The --edit-todo action can only be used during
interactive rebase.)
fi
+if test -n $rebase_root test -z $onto
+then
+ test -z $interactive_rebase interactive_rebase=implied
+fi
+
+if test -n $interactive_rebase
+then
+ type=interactive
+ state_dir=$merge_dir
+elif test -n $do_merge
+then
+ type=merge
+ state_dir=$merge_dir
+else
+ type=am
+ state_dir=$apply_dir
+fi
+
+. git-rebase--$type
+
case $action in
continue)
# Sanity check
@@ -407,24 +427,6 @@ and run me again. I am stopping in case you still have
something
valuable there.')
fi
-if test -n $rebase_root test -z $onto
-then
- test -z $interactive_rebase interactive_rebase=implied
-fi
-
-if test -n $interactive_rebase
-then
- type=interactive
- state_dir=$merge_dir
-elif test -n $do_merge
-then
- type=merge
- state_dir=$merge_dir
-else
- type=am
- state_dir=$apply_dir
-fi
-
if test -z $rebase_root
then
case $# in
--
1.8.5
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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: Our official home page and logo for the Git project
So this is really bikeshedding at its finest. I'd personally do agree on the logo proposed in the first mail by Junio. However who is the core community, who am I to judge? So maybe the decision process on this issue may need a more centrally steered opinion, so why not call for votes and weight the votes by #number of commits in git.git? 2014-04-13 10:53 GMT+02:00 Javier Domingo Cansino javier...@gmail.com: I think it is a suitable logo. It might not be the one I would think of, but I see with good eyes using it as one of the project logos. Javier Domingo Cansino -- 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 -- 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/27] refs: add a concept of a reference transaction
On 04/07/2014 09:13 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
+void ref_transaction_create(struct ref_transaction *transaction,
+const char *refname,
+unsigned char *new_sha1,
+int flags)
+{
+struct ref_update *update = add_update(transaction, refname);
+
+assert(!is_null_sha1(new_sha1));
+hashcpy(update-new_sha1, new_sha1);
+hashclr(update-old_sha1);
+update-flags = flags;
+update-have_old = 1;
+}
+
+void ref_transaction_delete(struct ref_transaction *transaction,
+const char *refname,
+unsigned char *old_sha1,
+int flags, int have_old)
+{
+struct ref_update *update = add_update(transaction, refname);
+
+update-flags = flags;
+update-have_old = have_old;
+if (have_old) {
+assert(!is_null_sha1(old_sha1));
+hashcpy(update-old_sha1, old_sha1);
+}
+}
These assert()s will often turn into no-op in production builds. If
it is a bug in the code (i.e. the callers are responsible for
catching these conditions and issuing errors, and there are actually
such checks implemented in the callers), it is fine to have these as
assert()s, but otherwise these should be 'if (...) die(BUG:)', I
think.
I forgot to confirm that the callers *do* verify that they don't pass
incorrect values to ref_transaction_create() and
ref_transaction_delete(). But if they wouldn't, then die(BUG:) would
not be appropriate either, would it? It would have to be die(you silly
user...).
Another reason I'm comfortable with only having assert()s in this case
is that even if the preconditions are not met, nothing really crazy
happens. If I were guarding against something nastier, like a buffer
overflow, then I would more likely have used die(BUG:) instead.
It is not material to this discussion, but I wonder how often production
builds use NDEBUG. I just checked that Debian does not; i.e.,
assertions are enabled for git there. Personally I wouldn't run code
built with NDEBUG unless building for a severely resource-constrained
device, and even then I'd be pretty nervous about it.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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: Our official home page and logo for the Git project
On Fri, Apr 11, 2014 at 9:25 PM, Junio C Hamano gits...@pobox.com wrote: The motion is about this: Outside people, like the party who approached us about putting our logo on their trinket, seem to associate that logo we see on git-scm.com today with our project, but we never officially said it was our logo (we did not endorse that git-scm.com is our official home page, either, for that matter). It is silly for us to have to say Ehh, that is a logo that was randomly done and slapped on git-scm.com which is not even our official home page, and the logo is licensed CC-BY by somebody else. Go talk to them., every time such a request comes. Please help us by letting us answer Yup, that is a logo (among others) that represents our project, and we are OK with you using it to help promote our project instead. That is what I meant by our official logo in the first message. So,... seconds? Seconded. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 00/25] Lockfile correctness and refactoring
Round v3. Thanks to Johannes Sixt and Peff for feedback on v2. This version addresses all issues raised for v1 [1] and v2 [2]. Changes since v2: * Instead of keeping track of whether a lock_file object is active via a new bit in a flags bitmask, store it in a separate volatile sig_atomic_t struct member. This makes it a little less undefined when this field is accessed by the signal handler. * Add some other volatile qualifiers to values used by the signal handler. * Define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN in cache.h and use them both inside and outside lockfile.c. (In v2, only LOCK_SUFFIX_LEN was defined and it was only used within lockfile.c, because the other potential users in refs.c were rewritten anyway. But that rewriting is no longer included in the patch series, so it makes sense to define these constants as part of the public lockfile API.) * Swap order of first two patches because the documentation lists unable_to_lock_die() under its new name. * die() (instead of NOP) if commit_lock_file() is called for an unlocked lock_file object. * Rebase to current master (there were no conflicts). [1] http://thread.gmane.org/gmane.comp.version-control.git/245609 [2] http://thread.gmane.org/gmane.comp.version-control.git/245801 Michael Haggerty (25): unable_to_lock_die(): rename function from unable_to_lock_index_die() api-lockfile: expand the documentation rollback_lock_file(): do not clear filename redundantly 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 add lock_file object to lock_file_list 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 *) write_packed_entry_fn(): convert cb_data into a (const int *) 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 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_elm(): replace last_path_elm() Documentation/technical/api-lockfile.txt | 40 - builtin/commit.c | 16 +- builtin/merge.c | 15 +- builtin/reflog.c | 2 +- builtin/update-index.c | 2 +- cache.h | 15 +- config.c | 6 +- lockfile.c | 275 ++- refs.c | 29 ++-- shallow.c| 6 +- 10 files changed, 247 insertions(+), 159 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 01/25] 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 p...@peff.net
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
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 ba54e19..0bab053 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -919,7 +919,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_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock_file))
diff --git a/cache.h b/cache.h
index 107ac61..3a873a4 100644
--- a/cache.h
+++ b/cache.h
@@ -543,7 +543,7 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
-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 8fbcb6a..d4bfb3f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -181,7 +181,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)
{
die(%s, unable_to_lock_message(path, err));
}
@@ -190,7 +190,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;
}
@@ -201,7 +201,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 28d5eca..67fe8b7 100644
--- a/refs.c
+++ b/refs.c
@@ -2118,7 +2118,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;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 17/25] 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 recorded
by whether lk-filename was the empty string or 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 would have been 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 j.s...@viscovery.net
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
cache.h| 1 +
lockfile.c | 45 +
2 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/cache.h b/cache.h
index 825cd0a..b7af173 100644
--- a/cache.h
+++ b/cache.h
@@ -539,6 +539,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 1453a7a..50a0541 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -27,11 +27,14 @@
* Instead of (3), the change can be rolled back by deleting lockfile.
*
* This module keeps track of all locked files in lock_file_list.
- * When the first file is locked, it registers an atexit(3) handler;
- * when the program exits, the handler rolls back any files that have
- * been locked but were never committed or rolled back.
+ * When the first file is locked, it registers an atexit(3) handler
+ * and a signal handler; when the program exits, the handler rolls
+ * back any files that have been locked but were never committed or
+ * rolled back.
*
- * A lock_file object can be in several states:
+ * Because the signal handler can be called at any time, a lock_file
+ * object must always be in a well-defined state. The possible states
+ * 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
@@ -39,18 +42,29 @@
* registered in the lock_file_list, and on_list is set.
*
* - Locked, lockfile open (after hold_lock_file_for_update() or
- * hold_lock_file_for_append()). 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.
+ * hold_lock_file_for_append()). In this state:
+ * - the lockfile exists
+ * - active is set
+ * - filename holds the filename of the lockfile
+ * - fd holds a file descriptor open for writing to the lockfile
+ * - 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 close_lock_file() or an
+ * unsuccessful commit_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 rollback_lock_file(), a successful
+ * commit_lock_file(), or a failed attempt to lock). In this state:
+ * - active is unset
+ * - filename[0] == '\0' (usually, though there are transitory states
+ * in which this condition doesn't hold)
+ * - fd is -1
+ * - the object is left registered in the lock_file_list, and
+ * on_list is set.
*
* See Documentation/api-lockfile.txt for more information.
*/
@@ -183,9 +197,12 @@
[PATCH v3 15/25] commit_lock_file(): inline temporary variable
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
lockfile.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 477bf4b..664b0c3 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -288,12 +288,14 @@ int close_lock_file(struct lock_file *lk)
int commit_lock_file(struct lock_file *lk)
{
char result_file[PATH_MAX];
- size_t i;
+
if (lk-fd = 0 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;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 04/25] 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.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
lockfile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lockfile.c b/lockfile.c
index 7701267..1122542 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -275,7 +275,7 @@ void rollback_lock_file(struct lock_file *lk)
{
if (lk-filename[0]) {
if (lk-fd = 0)
- close(lk-fd);
+ close_lock_file(lk);
unlink_or_warn(lk-filename);
lk-filename[0] = 0;
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 12/25] write_packed_entry_fn(): convert cb_data into a (const int *)
This makes it obvious that we have no plans to change the integer
pointed to, which is actually the fd field from a struct lock_file.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 20c483b..cb2f825 100644
--- a/refs.c
+++ b/refs.c
@@ -2177,7 +2177,7 @@ 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;
+ const int *fd = cb_data;
enum peel_status peel_status = peel_entry(entry, 0);
if (peel_status != PEEL_PEELED peel_status != PEEL_NON_TAG)
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 06/25] 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 mhag...@alum.mit.edu
---
lockfile.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index b101f77..e78a35f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -212,13 +212,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;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 16/25] 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. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- Documentation/technical/api-lockfile.txt | 4 +++- lockfile.c | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index b53e300..9a94ead 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -68,7 +68,9 @@ commit_lock_file:: with an earlier call to `hold_lock_file_for_update()`, close the file descriptor and rename the lockfile to its final destination. Returns 0 upon success, a negative - value on failure to close(2) or rename(2). + 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. rollback_lock_file:: diff --git a/lockfile.c b/lockfile.c index 664b0c3..1453a7a 100644 --- a/lockfile.c +++ b/lockfile.c @@ -292,6 +292,9 @@ int commit_lock_file(struct lock_file *lk) if (lk-fd = 0 close_lock_file(lk)) return -1; + if (!lk-filename[0]) + die(BUG: attempt to commit unlocked object); + strcpy(result_file, lk-filename); /* remove .lock: */ result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0; -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 10/25] delete_ref_loose(): don't muck around in the lock_file's filename
It's bad manners. Especially since, if unlink_or_warn() failed, the
memory wasn't restored to its original contents.
So make our own copy to work with.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 2805b31..20c483b 100644
--- a/refs.c
+++ b/refs.c
@@ -2486,12 +2486,15 @@ static int repack_without_ref(const char *refname)
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;
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 25/25] trim_last_path_elm(): 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 also
strbuf-based. Rename the function accordingly.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
lockfile.c | 38 --
1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 65f56ef..2515577 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -91,32 +91,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_elm(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);
}
@@ -146,14 +142,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_elm(path);
strbuf_addbuf(path, link);
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 09/25] 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 mhag...@alum.mit.edu
---
If I were confident that it would always be inlined, I would have
used
#define LOCK_SUFFIX_LEN strlen(LOCK_SUFFIX)
Or maybe even have omitted this constant and write strlen(LOCK_SUFFIX)
inline where it is needed instead. But I'm not so I didn't.
It's possible I missed other code that uses .lock or the magic
number 5, but these are the only ones I found.
cache.h| 4
lockfile.c | 12 ++--
refs.c | 7 ---
3 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 3a873a4..825cd0a 100644
--- a/cache.h
+++ b/cache.h
@@ -533,6 +533,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 8f6652c..1ce0e87 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -172,14 +172,14 @@ static char *resolve_symlink(char *p, size_t s)
return p;
}
-
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 */
@@ -202,7 +202,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();
@@ -296,7 +296,7 @@ int commit_lock_file(struct lock_file *lk)
if (lk-fd = 0 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 67fe8b7..2805b31 100644
--- a/refs.c
+++ b/refs.c
@@ -63,7 +63,8 @@ static int check_refname_component(const char *refname, int
flags)
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;
}
@@ -2486,11 +2487,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;
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 19/25] 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 mhag...@alum.mit.edu
---
builtin/merge.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 66d8843..9b7e7cb 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -671,7 +671,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));
int index_fd;
struct commit_list *reversed = NULL;
struct merge_options o;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 22/25] 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.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/commit.c | 12 ++--
builtin/reflog.c | 2 +-
cache.h | 2 +-
config.c | 6 +++---
lockfile.c | 45 +++--
refs.c | 6 +++---
shallow.c| 6 +++---
7 files changed, 36 insertions(+), 43 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 8ffb3ef..38c137f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -330,7 +330,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));
@@ -341,10 +341,10 @@ 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);
commit_style = COMMIT_NORMAL;
- return index_lock.filename;
+ return index_lock.filename.buf;
}
/*
@@ -368,7 +368,7 @@ static const char *prepare_index(int argc, const char
**argv, const char *prefix
close_lock_file(index_lock))
die(_(unable to write new_index file));
commit_style = COMMIT_NORMAL;
- return index_lock.filename;
+ return index_lock.filename.buf;
}
/*
@@ -453,9 +453,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 c12a9784..d7df34e 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 9019c7d..319251b 100644
--- a/cache.h
+++ b/cache.h
@@ -543,7 +543,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 6821cef..78c3dad 100644
--- a/config.c
+++ b/config.c
@@ -1706,7 +1706,7 @@ out_free:
return ret;
write_err_out:
- ret = write_error(lock-filename);
+ ret = write_error(lock-filename.buf);
goto out_free;
}
@@ -1821,7 +1821,7 @@ int git_config_rename_section_in_file(const char
*config_filename,
}
store.baselen = strlen(new_name);
if (!store_write_section(out_fd, new_name)) {
- ret = write_error(lock-filename);
+ ret = write_error(lock-filename.buf);
goto out;
}
/*
@@ -1847,7 +1847,7 @@ int git_config_rename_section_in_file(const char
*config_filename,
continue;
length = strlen(output);
if (write_in_full(out_fd, output, length) != length) {
- ret = write_error(lock-filename);
+ ret = write_error(lock-filename.buf);
[PATCH v3 21/25] 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 mhag...@alum.mit.edu
---
lockfile.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index fce53f1..0aa2998 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -305,7 +305,8 @@ int close_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-fd = 0 close_lock_file(lk))
return -1;
@@ -313,11 +314,12 @@ int commit_lock_file(struct lock_file *lk)
if (!lk-active)
die(BUG: attempt to commit unlocked object);
- 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)
return -1;
lk-active = 0;
lk-filename[0] = 0;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 20/25] 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 p...@peff.net
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/merge.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 9b7e7cb..1efa7b6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -657,16 +657,16 @@ static int try_merge_strategy(const char *strategy,
struct commit_list *common,
struct commit_list *remoteheads,
struct commit *head, const char *head_arg)
{
+ static struct lock_file lock;
int index_fd;
- struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
- index_fd = hold_locked_index(lock, 1);
+ index_fd = hold_locked_index(lock, 1);
refresh_cache(REFRESH_QUIET);
if (active_cache_changed
(write_cache(index_fd, active_cache, active_nr) ||
-commit_locked_index(lock)))
+commit_locked_index(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;
@@ -699,14 +699,14 @@ 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);
- index_fd = hold_locked_index(lock, 1);
+ index_fd = hold_locked_index(lock, 1);
clean = merge_recursive(o, head,
remoteheads-item, reversed, result);
if (active_cache_changed
(write_cache(index_fd, active_cache, active_nr)
||
-commit_locked_index(lock)))
+commit_locked_index(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,
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 23/25] 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 mhag...@alum.mit.edu
---
lockfile.c | 33 -
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index f552e33..59ed758 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -141,44 +141,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;
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 13/25] 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 mhag...@alum.mit.edu
---
lockfile.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 1ce0e87..ba791d5 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -204,16 +204,16 @@ 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)) {
- error(cannot fix permission bits on %s, lk-filename);
- rollback_lock_file(lk);
- return -1;
- }
- }
- else
+ if (lk-fd 0) {
lk-filename[0] = 0;
+ return -1;
+ }
+ lk-owner = getpid();
+ if (adjust_shared_perm(lk-filename)) {
+ error(cannot fix permission bits on %s, lk-filename);
+ rollback_lock_file(lk);
+ return -1;
+ }
return lk-fd;
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 24/25] 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.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
lockfile.c | 53 +++--
1 file changed, 19 insertions(+), 34 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 59ed758..65f56ef 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -124,53 +124,40 @@ 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;
}
static int lock_file(struct lock_file *lk, const char *path, int flags)
@@ -189,16 +176,14 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
lk-active = 0;
lk-owner = 0;
lk-on_list = 1;
- strbuf_init(lk-filename, PATH_MAX);
+ strbuf_init(lk-filename, 0);
lk-next = lock_file_list;
lock_file_list = lk;
}
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));
- }
+ 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) {
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 14/25] remove_lock_file(): call rollback_lock_file()
It does just what we need.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
lockfile.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index ba791d5..477bf4b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -63,12 +63,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;
}
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 18/25] 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 increase the chance that the signal handler will see a
valid object state.
Suggested-by: Johannes Sixt j.s...@viscovery.net
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
cache.h| 6 +++---
lockfile.c | 2 +-
refs.c | 5 +++--
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/cache.h b/cache.h
index b7af173..9019c7d 100644
--- a/cache.h
+++ b/cache.h
@@ -538,10 +538,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 50a0541..fce53f1 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -69,7 +69,7 @@
* See Documentation/api-lockfile.txt for more information.
*/
-static struct lock_file *lock_file_list;
+static struct lock_file *volatile lock_file_list;
static const char *alternate_index_output;
static void remove_lock_file(void)
diff --git a/refs.c b/refs.c
index cb2f825..db8057c 100644
--- a/refs.c
+++ b/refs.c
@@ -2213,15 +2213,16 @@ int commit_packed_refs(void)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
int error = 0;
+ int fd;
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));
+ fd = packed_ref_cache-lock-fd;
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, fd);
if (commit_lock_file(packed_ref_cache-lock))
error = -1;
packed_ref_cache-lock = NULL;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 07/25] lock_file(): always add lock_file object to lock_file_list
It used to be that if locking failed, lock_file() usually did not
register the lock_file object in lock_file_list but sometimes it did.
This confusion was compounded if lock_file() was called via
hold_lock_file_for_append(), which has its own failure modes.
The ambiguity didn't have any ill effects, because lock_file objects
cannot be removed from the lock_file_list anyway. But it is
unnecessary to leave this behavior inconsistent.
So change lock_file() to *always* ensure that the lock_file object is
registered in lock_file_list regardless of whether an error occurs.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
lockfile.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index e78a35f..120a6d6 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -130,6 +130,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-on_list = 1;
+ lk-filename[0] = 0;
+ lk-next = lock_file_list;
+ lock_file_list = lk;
+ }
+
if (strlen(path) = max_path_len)
return -1;
strcpy(lk-filename, path);
@@ -138,16 +154,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)) {
error(cannot fix permission bits on %s, lk-filename);
rollback_lock_file(lk);
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 08/25] 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 mhag...@alum.mit.edu --- lockfile.c | 51 +++ 1 file changed, 51 insertions(+) diff --git a/lockfile.c b/lockfile.c index 120a6d6..8f6652c 100644 --- a/lockfile.c +++ b/lockfile.c @@ -4,6 +4,57 @@ #include cache.h #include sigchain.h +/* + * File write-locks as used by Git. + * + * When a file at $FILENAME needs to be written, it is done as + * follows: + * + * 1. Obtain a lock on the file by creating a lockfile at path + *$FILENAME.lock. The file is opened for read/write using O_CREAT + *and O_EXCL mode to ensure that it doesn't already exist. Such a + *lock file is respected by writers *but not by readers*. + * + *Usually, if $FILENAME is a symlink, then it is resolved, and the + *file ultimately pointed to is the one that is locked and later + *replaced. However, if LOCK_NODEREF is used, then $FILENAME + *itself is locked and later replaced, even if it is a symlink. + * + * 2. Write the new file contents to the lockfile. + * + * 3. Move the lockfile to its final destination using rename(2). + * + * Instead of (3), the change can be rolled back by deleting lockfile. + * + * This module keeps track of all locked files in lock_file_list. + * When the first file is locked, it registers an atexit(3) handler; + * when the program exits, the handler rolls back any files that have + * been locked but were never committed or rolled back. + * + * 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() or + * hold_lock_file_for_append()). 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. + * + * See Documentation/api-lockfile.txt for more information. + */ + static struct lock_file *lock_file_list; static const char *alternate_index_output; -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 11/25] 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 mhag...@alum.mit.edu
---
builtin/commit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..8ffb3ef 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -302,8 +302,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)
{
int fd;
struct string_list partial;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 05/25] 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 mhag...@alum.mit.edu
---
lockfile.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 1122542..b101f77 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
lock_file_list = lk;
lk-on_list = 1;
}
- if (adjust_shared_perm(lk-filename))
- return error(cannot fix permission bits on %s,
-lk-filename);
+ if (adjust_shared_perm(lk-filename)) {
+ error(cannot fix permission bits on %s, lk-filename);
+ rollback_lock_file(lk);
+ return -1;
+ }
}
else
lk-filename[0] = 0;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 02/25] api-lockfile: 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(). Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- Documentation/technical/api-lockfile.txt | 36 +--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index dd89404..b53e300 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -28,9 +28,39 @@ hold_lock_file_for_update:: the final destination (e.g. `$GIT_DIR/index`) and a flag `die_on_error`. Attempt to create a lockfile for the destination and return the file descriptor for writing - to the file. If `die_on_error` flag is true, it dies if - a lock is already taken for the file; otherwise it - returns a negative integer to the caller on failure. + to the file. The flags parameter is a combination of ++ +-- +LOCK_NODEREF:: + + Usually symbolic links in path are resolved in path and the + lockfile is created by adding .lock to the resolved path; + however, if `LOCK_NODEREF` is set, then the lockfile is + created by adding .lock to the path argument itself. + +LOCK_DIE_ON_ERROR:: + + If a lock is already taken for the file, `die()` with an error + message. If this option is not specified, return a negative + integer to the caller on failure. +-- + +hold_lock_file_for_append:: + + Like `hold_lock_file_for_update()`, except that additionally + the existing contents of the file (if any) are copied to the + lockfile and its write pointer is positioned at the end of the + file before returning. + +unable_to_lock_error:: + + Emit an error describing that there was an error locking the + specified path. The err parameter should be the errno of the + problem that caused the failure. + +unable_to_lock_die:: + + Like `unable_to_lock_error()`, but also `die()`. commit_lock_file:: -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 03/25] 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 mhag...@alum.mit.edu --- lockfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lockfile.c b/lockfile.c index d4bfb3f..7701267 100644 --- a/lockfile.c +++ b/lockfile.c @@ -277,6 +277,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; } -- 1.9.1 -- 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: Get all tips quickly
Hi Michael, Ævar,
Thank you very much for your answers.
Each of 'git show-ref’ and ‘git for-each-ref’ is 2 times faster than ‘git log
--branches --tags --remotes’ on “warmed up FS caches, but take the same time
on “cold” FS.
It seems that all these approaches internally walk down from all references.
So, given that there is no way to get just tips (I was actually hoping that Git
might be storing them somewhere else), we will stick to ‘git show ref -d’.
Thanks a lot,
Kirill.
On 13 Apr 2014, at 22:29 , Ævar Arnfjörð Bjarmason ava...@gmail.com wrote:
Tried git for-each-ref and the various options it has?
Doing this for 35k tags is still going to be non-trivial.
On 13 Apr 2014, at 23:20 , Michael Haggerty mhag...@alum.mit.edu wrote:
On 04/13/2014 04:19 PM, Kirill Likhodedov wrote:
What is fastest possible way to get all “tips” (leafs of the Git log
graph) in a Git repository with hashes of commits they point to?
We at JetBrains are tuning performance of Git log integration in our
IntelliJ IDEA and want to get all tips as fast as possible. Currently
we use 'git log —branches --tags --remotes --no-walk’, but the
problem is that it is slow if there are a lot of references. In our
repository we have about 35K tags, and therefore the tags is the main
slowdown. On the other hand, we have just a couple of dozens of tips
as well as branch references, and `git log --branches --remotes` is
very fast.
So we are searching a way to get tags pointing to the graph leafs
faster.
The fastest ways to get all references plus the commits that are pointed
at by annotated references would probably be `git show-ref -d`. The
funny-looking entries like refs/tags/v1.7.0^{} are the annotated tags
peeled to the object that they ultimately refer. But this command
doesn't tell the types of the objects, and there can be trees and blobs
mixed in.
If your question is also to figure out the minimum set of references
that are needed to include all tips (i.e., commits with no descendants),
then the answer is trickier. There is a command that should do what you
say:
git merge-base --independent commit...
but (1) with a lot of references, your arguments wouldn't all fit on the
command line (recursive use of xargs might be needed), (2) I don't know
if merge-base --independent is programmed to work efficiently on so
many inputs, and (3) I don't know of a cheap way of getting a list of
all commits referred to by references (i.e., dereferencing annotated
tags but ignoring references/annotated tags that refer to trees or blobs).
Another approach is to start by finding the leaf commits by SHA-1. You
can do this by listing all commits, and listing all commits' parents,
and then finding the objects that appear in the first list but not the
second. This could look like
comm -23 \
(git log --all --pretty=format:'%H' | sort -u) \
(git log --all --pretty=format:'%P' | tr ' ' '\n' | sort -u)
If you want reference names corresponding to these SHA-1s, you could use
name-rev to convert the SHA-1s into refnames:
git rev-parse --symbolic-full-name $(
comm -23 \
(git log --all --pretty=format:'%H' | sort -u) \
(git log --all --pretty=format:'%P' | tr ' ' '\n' | sort -u) |
git name-rev --stdin --name-only
)
The rev-parse --symbolic-full-name is needed because name-ref seems
only able to emit abbreviated reference names.
In practice, you might want to cache some of the results to avoid having
to do a full tree traversal every time.
We also tried to read tags by manually parsing .git files (it is
faster than invoking git log), but unfortunately annotated tags in
.git/refs/tags/ are written without the hashes they point to (unlike
.git/packed-refs).
I strongly recommend against parsing these files yourselves. Your
software would not be robust against any future changes to the file
formats etc.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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
GitMinutes about Git for Windows
Dear friends of Git for Windows, it was very delightful to be on the show, hosted by Thomas Ferris Nicolaisen: http://episodes.gitminutes.com/2014/04/gitminutes-28-johannes-schindelin-on.html Enjoy, Johannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Silly time stamps
On Wed, 09 Apr 2014 20:50:57 +, Mahmoud Asshole wrote: ... This was raised previously[1], but none of the responses are convincing. Please go directly to the special hell of debugging timezone-related stuff in customer data, for which this extra bit of information is heaven-sent. Do not collect additional bile, and do not reappear here. Andreas -- Totally trivial. Famous last words. From: Linus Torvalds torvalds@*.org Date: Fri, 22 Jan 2010 07:29:21 -0800 -- 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: [msysGit] GitMinutes about Git for Windows
On Mon, Apr 14, 2014 at 5:14 PM, Johannes Schindelin johannes.schinde...@gmx.de wrote: Dear friends of Git for Windows, it was very delightful to be on the show, hosted by Thomas Ferris Nicolaisen: http://episodes.gitminutes.com/2014/04/gitminutes-28-johannes-schindelin-on.html Really enjoyable, 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 v4 2/3] refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase
Change delete_ref_loose()) to just flag that a ref is to be deleted but do
not actually unlink the files.
Change commit_ref_lock() so that it will unlink refs that are flagged for
deletion.
Change all callers of delete_ref_loose() to explicitely call commit_ref_lock()
to commit the deletion.
The new pattern for deleting loose refs thus become:
lock = lock_ref_sha1_basic() (or varient of)
delete_ref_loose(lock)
unlock_ref(lock) | commit_ref_lock(lock)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 32
refs.h | 2 ++
2 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/refs.c b/refs.c
index 646afd7..a14addb 100644
--- a/refs.c
+++ b/refs.c
@@ -2484,16 +2484,9 @@ static int repack_without_ref(const char *refname)
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 */
-
- lock-lk-filename[i] = 0;
- err = unlink_or_warn(lock-lk-filename);
- lock-lk-filename[i] = '.';
- if (err errno != ENOENT)
- return 1;
- }
+ lock-delete_ref = 1;
+ lock-delete_flag = flag;
+
return 0;
}
@@ -2515,7 +2508,7 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
unlink_or_warn(git_path(logs/%s, lock-ref_name));
clear_loose_ref_cache(ref_cache);
- unlock_ref(lock);
+ ret |= commit_ref_lock(lock);
return ret;
}
@@ -2868,7 +2861,20 @@ int write_ref_sha1(struct ref_lock *lock,
int commit_ref_lock(struct ref_lock *lock)
{
- if (!lock-skipped_write commit_ref(lock)) {
+ if (lock-delete_ref) {
+ int flag = lock-delete_flag;
+
+ if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
+ /* loose */
+ int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+
+ lock-lk-filename[i] = 0;
+ err = unlink_or_warn(lock-lk-filename);
+ lock-lk-filename[i] = '.';
+ if (err errno != ENOENT)
+ return 1;
+ }
+ } else if (!lock-skipped_write commit_ref(lock)) {
error(Couldn't set %s, lock-ref_name);
unlock_ref(lock);
return -1;
@@ -3487,6 +3493,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (update-lock) {
delnames[delnum++] = update-refname;
ret |= delete_ref_loose(update-lock, update-type);
+ ret |= commit_ref_lock(update-lock);
+ update-lock = NULL;
}
}
diff --git a/refs.h b/refs.h
index f14a417..223be30 100644
--- a/refs.h
+++ b/refs.h
@@ -9,6 +9,8 @@ struct ref_lock {
int lock_fd;
int force_write;
int skipped_write;
+ int delete_ref;
+ int delete_flag;
};
struct ref_transaction;
--
1.9.1.505.gd05696d
--
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 2/2] commit: add --ignore-submodules[=when] parameter
Ronald Weiss weiss.ron...@gmail.com writes:
On 8. 4. 2014 20:43, Jens Lehmann wrote:
Useful when values for commit are 'all' (default) or 'none'. The others
('dirty' and 'untracked') have same effect as 'none', as commit is only
interested in whether the submodule's HEAD differs from what is commited
in the superproject.
Unless it outputs a status message, then 'dirty' and 'untracked' do
influence what is shown there. Apart from that (and maybe tests for
these two cases ;-) this is looking good to me.
OK, I updated the patch for commit to take that into account. Also, I
rebased both patches onto current master. Sending them in a moment.
If you don't have any more complaints, can I add Acked-by: you and
resend the patches to Junio?
It is not When I see no more complaints, I'll resend with your
Ack. An Ack is a positive thing, not lack of discovery of further
issues.
Rather, it is more like I'll wait for your Acks and then I'll
resend with your Ack, or If they look good, reply with Ack and let
the maintainer pick them up.
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 v4 3/3] refs.c: change ref_transaction_commit to run the commit loops once all work is finished
During a transaction commit we will both update and delete refs.
Since both update and delete now use the same pattern
lock = lock_ref_sha1_basic() (or varient of)
write_ref_sha1(lock)/delete_ref_loose(lock)
unlock_ref(lock) | commit_ref_lock(lock)
we can now simplify ref_transaction_commit to have one loop that locks all
involved refs.
A second loop that writes or flags for deletion, but does not commit, all
the refs.
And a final third loop that commits all the refs once all the work and
preparations are complete.
This makes updating/deleting multiple refs more atomic since we will not start
the commit phase until all the preparations have completed successfully.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 39 ++-
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/refs.c b/refs.c
index a14addb..87193c7 100644
--- a/refs.c
+++ b/refs.c
@@ -3467,42 +3467,47 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
}
}
- /* Perform updates first so live commits remain referenced */
+ /* Prepare all the updates/deletes */
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- if (!is_null_sha1(update-new_sha1)) {
+ if (!is_null_sha1(update-new_sha1))
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
update-lock, onerr);
- if (ret)
- unlock_ref(update-lock);
- else
- commit_ref_lock(update-lock);
- update-lock = NULL;
- if (ret)
- goto cleanup;
+ else {
+ delnames[delnum++] = update-refname;
+ ret = delete_ref_loose(update-lock, update-type);
}
+ if (ret)
+ goto cleanup;
}
- /* Perform deletes now that updates are safely completed */
+ ret |= repack_without_refs(delnames, delnum);
+ for (i = 0; i delnum; i++)
+ unlink_or_warn(git_path(logs/%s, delnames[i]));
+ clear_loose_ref_cache(ref_cache);
+
+ /* Perform updates first so live commits remain referenced */
+ for (i = 0; i n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update-lock !update-lock-delete_ref) {
+ ret |= commit_ref_lock(update-lock);
+ update-lock = NULL;
+ }
+ }
+ /* And finally perform all deletes */
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
if (update-lock) {
- delnames[delnum++] = update-refname;
- ret |= delete_ref_loose(update-lock, update-type);
ret |= commit_ref_lock(update-lock);
update-lock = NULL;
}
}
- ret |= repack_without_refs(delnames, delnum);
- for (i = 0; i delnum; i++)
- unlink_or_warn(git_path(logs/%s, delnames[i]));
- clear_loose_ref_cache(ref_cache);
-
cleanup:
for (i = 0; i n; i++)
if (updates[i]-lock)
--
1.9.1.505.gd05696d
--
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 v4 0/3] Make update refs more atomic
refs.c:ref_transaction_commit() intermingles doing updates and checks with actually applying changes to the refs in loops that abort on error. This is done one ref at a time and means that if an error is detected that will fail the operation partway through the list of refs to update we will end up with some changes applied to disk and others not. Without having transaction support from the filesystem, it is hard to make an update that involves multiple refs to guarantee atomicity, but we can do a somewhat better than we currently do. These patches change the update and delete functions to use a three call pattern of 1, lock 2, update, or flag for deletion 3, apply on disk (rename() or unlink()) When a transaction is commited we first do all the locking, preparations and most of the error checking before we actually start applying any changes to the filesystem store. This means that more of the error cases that will fail the commit will trigger before we start doing any changes to the actual files. This should make the changes of refs in refs_transaction_commit slightly more atomic. Version 4: * Fix a bug in fast-import.c:dump_tags and make sure no tests fail Version 3: * Rebased onto mhagger/ref-transactions. * Removed the patch to do update/delete from a single loop. Version 2: Updates and fixes based on Junio's feedback. * Fix the subject line for patches so they comply with the project standard. * Redo the update/delete loops so that we maintain the correct order of operations. Perform all updates first, then perform the deletes. * Add an additional patch that allows us to do the update/delete in the correct order from within a single loop by first sorting the refs so that deletes are after all non-deletes. Ronnie Sahlberg (3): refs.c: split writing and commiting a ref into two separate functions refs.c: split delete_ref_loose() into a separate flag-for-deletion and commit phase refs.c: change ref_transaction_commit to run the commit loops once all work is finished branch.c | 10 -- builtin/commit.c | 5 +++ builtin/fetch.c| 7 +++- builtin/receive-pack.c | 4 +++ builtin/replace.c | 6 +++- builtin/tag.c | 6 +++- fast-import.c | 18 -- refs.c | 98 +- refs.h | 6 sequencer.c| 4 +++ walker.c | 4 +++ 11 files changed, 129 insertions(+), 39 deletions(-) -- 1.9.1.505.gd05696d -- 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 2/2] Verify index file before we opportunistically update it
Duy Nguyen pclo...@gmail.com writes: On Sat, Apr 12, 2014 at 11:19 AM, Junio C Hamano gits...@pobox.com wrote: In the spectrum between useful and insane, there is a point where we should just tell the insane: don't do it then. ... A process' dying is a way of telling people this is insane without having to draw a line between dos and dont's in documents. Fair enough. Instead of that is insane and you may end up recording what you do not intend to, being able to say that is insane and you will see some commands failing when you may end up being in such a situation is a lot more explicit. Thanks for injecting some sanity to me. Serializing write access to make both competing processes is nice, but that's a separate step that may or may not be worth pursuing. And I'm towards not worth pursuing. I think we are in agreement on that point. I would want to see the proposed read without locking, compute heavily and then take a lock and revalidate before writing it out, or fail done with a focus on not hurting the sane use pattern, i.e. making the revalidate part as light as possible. I however cannot seem to find any optimization opportunities other than the open, read and compare the checksum. Two I thought and discarded are: - checking timestamp of the index file itself, and failing when it has changed (without opening or reading the checksum) is not good; it is optimizing for the wrong case, because we would need to check the checksum anyway when the timestamps match. - checking i-num of the index file itself, and failing when it has changed (without opening or reading the checksum) is not good, either. Creating index.lock, writing to it and renaming it to index, keeping the index during the whole period, would ensure that the index file that results with this single cycle would get a different i-num from the original, but if that is done twice or more, the same i-num can be reused and defeat the check. -- 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 v4 1/3] refs.c: split writing and commiting a ref into two separate functions
Change the function write_ref_sha1() to just write the ref but not
commit the ref or the lockfile.
Add a new function commit_ref_lock() that will commit the change done by
a previous write_ref_sha1().
Update all callers of write_ref_sha1() to call commit_ref_lock().
The new pattern for updating a ref is now :
lock = lock_ref_sha1_basic() (or varient of)
write_ref_sha1(lock)
unlock_ref(lock) | commit_ref_lock(lock)
Once write_ref_sha1() returns, the new ref has been written and the lock
file has been closed.
At that stage we can then either call unlock_ref() which will abort the
update and delete the lock file withouth applying it, or call
commit_ref_lock() which will rename the lock file onto the ref file.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 10 --
builtin/commit.c | 5 +
builtin/fetch.c| 7 ++-
builtin/receive-pack.c | 4
builtin/replace.c | 6 +-
builtin/tag.c | 6 +-
fast-import.c | 18 --
refs.c | 41 +++--
refs.h | 4
sequencer.c| 4
walker.c | 4
11 files changed, 92 insertions(+), 17 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..903ea75 100644
--- a/branch.c
+++ b/branch.c
@@ -304,9 +304,15 @@ void create_branch(const char *head,
if (real_ref track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
- if (!dont_change_ref)
- if (write_ref_sha1(lock, sha1, msg) 0)
+ if (!dont_change_ref) {
+ if (write_ref_sha1(lock, sha1, msg) 0) {
+ unlock_ref(lock);
die_errno(_(Failed to write ref));
+ }
+ if (commit_ref_lock(lock) 0) {
+ die_errno(_(Failed to commit ref));
+ }
+ }
strbuf_release(ref);
free(real_ref);
diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..3d8a3a8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1686,9 +1686,14 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
die(_(cannot lock HEAD ref));
}
if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) {
+ unlock_ref(ref_lock);
rollback_index_files();
die(_(cannot update HEAD ref));
}
+ if (commit_ref_lock(ref_lock) 0) {
+ rollback_index_files();
+ die(_(cannot commit HEAD ref));
+ }
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..ebfb854 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -388,7 +388,12 @@ static int s_update_ref(const char *action,
if (!lock)
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref-new_sha1, msg) 0)
+ if (write_ref_sha1(lock, ref-new_sha1, msg) 0) {
+ unlock_ref(lock);
+ return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+ STORE_REF_ERROR_OTHER;
+ }
+ if (commit_ref_lock(lock) 0)
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
return 0;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..4760274 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -587,8 +587,12 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return failed to lock;
}
if (write_ref_sha1(lock, new_sha1, push)) {
+ unlock_ref(lock);
return failed to write; /* error() already called */
}
+ if (commit_ref_lock(lock))
+ return failed to commit; /* error() already called */
+
return NULL; /* good */
}
}
diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..c09ff49 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -160,8 +160,12 @@ static int replace_object(const char *object_ref, const
char *replace_ref,
lock = lock_any_ref_for_update(ref, prev, 0, NULL);
if (!lock)
die(%s: cannot lock the ref, ref);
- if (write_ref_sha1(lock, repl, NULL) 0)
+ if (write_ref_sha1(lock, repl, NULL) 0) {
+ unlock_ref(lock);
die(%s: cannot update the ref, ref);
+ }
+ if (commit_ref_lock(lock) 0)
+ die(%s: cannot commit the ref, ref);
return 0;
}
diff --git a/builtin/tag.c b/builtin/tag.c
index 40356e3..8653a64 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -644,8
Re: [PATCH] git-rebase: Print name of rev when using shorthand
Brian Gesiak modoca...@gmail.com writes:
The output from a successful invocation of the shorthand command
git rebase - is something like Fast-forwarded HEAD to @{-1},
which includes a relative reference to a revision. Other commands
that use the shorthand -, such as git checkout -, typically
display the symbolic name of the revision.
Change rebase to output the symbolic name of the revision when using
the shorthand. For the example above, the new output is
Fast-forwarded HEAD to master, assuming @{-1} is a reference to
master.
- Use git name-rev to retreive the name of the rev.
- Update the tests in light of this new behavior.
Requested-by: John Keeping j...@keeping.me.uk
Signed-off-by: Brian Gesiak modoca...@gmail.com
---
What the patch wants to implement sounds sensible, but I do not
think name-rev is a right tool for this. Imagine the case where
there are more than one branches whose tip points at the commit you
came from. name-rev will not be able to pick correctly which one to
report.
Also think what happens if you were previously on a detached HEAD?
I think you would want to use something like:
upstream_name=$(git rev-parse --symbolic-full-name @{-1})
if test -n $upstream
then
upstream_name=${upstream_name#refs/heads/}
else
upstream_name=@{-1}
fi
if the change is to be made at that point in the code.
I also wonder if git rebase @{-1} deserve a similar translation
like you are giving git rebase -.
Previous discussion on this issue:
http://article.gmane.org/gmane.comp.version-control.git/244340
git-rebase.sh | 2 +-
t/t3400-rebase.sh | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index 2c75e9f..ab0e081 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -455,7 +455,7 @@ then
*) upstream_name=$1
if test $upstream_name = -
then
- upstream_name=@{-1}
+ upstream_name=`git name-rev --name-only @{-1}`
fi
shift
;;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 80e0a95..2b99940 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -91,7 +91,7 @@ test_expect_success 'rebase from ambiguous branch name' '
test_expect_success 'rebase off of the previous branch using -' '
git checkout master
git checkout HEAD^
- git rebase @{-1} expect.messages
+ git rebase master expect.messages
OK.
git merge-base master HEAD expect.forkpoint
git checkout master
@@ -100,8 +100,6 @@ test_expect_success 'rebase off of the previous branch
using -' '
git merge-base master HEAD actual.forkpoint
test_cmp expect.forkpoint actual.forkpoint
- # the next one is dubious---we may want to say -,
- # instead of @{-1}, in the message
test_i18ncmp expect.messages actual.messages
'
--
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 2/2] commit: add --ignore-submodules[=when] parameter
On 14. 4. 2014 20:30, Junio C Hamano wrote:
Ronald Weiss weiss.ron...@gmail.com writes:
On 8. 4. 2014 20:43, Jens Lehmann wrote:
Useful when values for commit are 'all' (default) or 'none'. The others
('dirty' and 'untracked') have same effect as 'none', as commit is only
interested in whether the submodule's HEAD differs from what is commited
in the superproject.
Unless it outputs a status message, then 'dirty' and 'untracked' do
influence what is shown there. Apart from that (and maybe tests for
these two cases ;-) this is looking good to me.
OK, I updated the patch for commit to take that into account. Also, I
rebased both patches onto current master. Sending them in a moment.
If you don't have any more complaints, can I add Acked-by: you and
resend the patches to Junio?
It is not When I see no more complaints, I'll resend with your
Ack. An Ack is a positive thing, not lack of discovery of further
issues.
I'm really sorry if the tone of my message sounded harsh to you, it
wasn't meant like that at all.
Rather, it is more like I'll wait for your Acks and then I'll
resend with your Ack, or If they look good, reply with Ack and let
the maintainer pick them up.
OK.
--
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 5/5] completion: fix completion of certain aliases
Gábor Szeder sze...@ira.uka.de writes: words[] is just fine, we never modify it after it is filled by _get_comp_words_by_ref() at the very beginning. Hmph. I would have understood if the latter were we never look at it (to decide what to do). we never modify it does not sound like an enough justification behind words[] is just fine---maybe I am not reading you correctly. The root of the problem is that the expected position of the name of the git command in __git_complete_remote_or_refspec() is hardcoded as words[1], but that is not the case when: 1) it's an alias, as in Felipe's example: git p oriTAB, because while the index is ok, the content is not. 2) in presence of options of the main git command: git -c foo=bar push oriTAB, because the index is off. 3) the command is a shell alias for which the user explicitly set the completion function with __git_complete() (at his own risk): alias gp=git push; __git_complete gp _git_push; gp oriTAB Neither the index nor the content are ok. Fixing the hard-coded indexing would only solve 2) but not 1) and 3), as it obviously couldn't turn the git or shell alias into a git command on its own. Felipe's patch only deals with 1), as it only kicks in in case of a git alias. Yeah, do completions for commands (not just for the ones that use remote-or-refspec Felipe's patch addresses) have trouble with the latter two in general? If that is the case,... Communicating the name of the git command to __git_complete_remote_or_refspec() by its callers via a new variable as suggested by Junio, or perhaps by an additional parameter to the function is IMHO the right thing to do, because, unless I'm missing something, it would make all three cases work. ... while the above analysis may be correct, taking Felipe's patch to address only (1) and leaving a solution to the more general words[1] problem for other patches on top might not be too bad an approach. Unless (A) remote-or-refspec thing is the primary offender, and other commands do not suffer from the words[1] problem, in which case I tend to agree that an additional parameter would be the way to go (there are only a few callers of the function); or (B) even if words[1] problem is more widespread, such a more general solution to all three issues can be coded cleanly and quickly, without having to have Felipe's patch as a stop-gap measure. that is. I'll keep Felipe's patch in the meantime to 'pu'. 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/2] Makefile: use curl-config to determine curl flags
Dave Borowitz dborow...@google.com writes: curl-config should always be installed alongside a curl distribution, and its purpose is to provide flags for building against libcurl, so use it instead of guessing flags and dependent libraries. Allow overriding CURL_CONFIG to a custom path to curl-config, to compile against a curl installation other than the first in PATH. Use this only when CURLDIR is not explicitly specified, to continue supporting older builds. Signed-off-by: Dave Borowitz dborow...@google.com Sounds logically the right thing to do. Was there a particular platform that needed this (i.e. cannot be made to work with the existing CURLDIR and guessing flags and dependeent libraries) that may be worth mentioning in the log message? --- Makefile | 35 +++ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index 2128ce3..d6330bc 100644 --- a/Makefile +++ b/Makefile @@ -34,8 +34,12 @@ all:: # git-http-push are not built, and you cannot use http:// and https:// # transports (neither smart nor dumb). # +# Define CURL_CONFIG to the path to a curl-config binary other than the +# default 'curl-config'. +# # Define CURLDIR=/foo/bar if your curl header and library files are in -# /foo/bar/include and /foo/bar/lib directories. +# /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, +# but is less robust. # # Define NO_EXPAT if you do not have expat installed. git-http-push is # not built, and you cannot push using http:// and https:// transports (dumb). @@ -143,9 +147,11 @@ all:: # # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin). # -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). +# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix). Only used +# if CURLDIR is set. # -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). +# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix). Only +# used if CURLDIR is set. # # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin). # @@ -1121,18 +1127,23 @@ else # Try -Wl,-rpath=$(CURLDIR)/$(lib) in such a case. BASIC_CFLAGS += -I$(CURLDIR)/include CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl + ifdef NEEDS_SSL_WITH_CURL + CURL_LIBCURL += -lssl + ifdef NEEDS_CRYPTO_WITH_SSL + CURL_LIBCURL += -lcrypto + endif + endif + ifdef NEEDS_IDN_WITH_CURL + CURL_LIBCURL += -lidn + endif else - CURL_LIBCURL = -lcurl - endif - ifdef NEEDS_SSL_WITH_CURL - CURL_LIBCURL += -lssl - ifdef NEEDS_CRYPTO_WITH_SSL - CURL_LIBCURL += -lcrypto + CURL_CONFIG ?= curl-config + BASIC_CFLAGS += $(shell $(CURL_CONFIG) --cflags) + CURL_LIBCURL = $(shell $(CURL_CONFIG) --libs) + ifeq $(CURL_LIBCURL) + $(error curl not detected; try setting CURLDIR) endif endif - ifdef NEEDS_IDN_WITH_CURL - CURL_LIBCURL += -lidn - endif REMOTE_CURL_PRIMARY = git-remote-http$X REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$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 v4 0/3] Make update refs more atomic
Thanks; will queue. -- 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] prompt: fix missing file errors in zsh
Felipe Contreras felipe.contre...@gmail.com writes:
zsh seems to have a bug while redirecting the stderr of the 'read'
command:
% read foo 2 /dev/null foo
zsh: no such file or directory: foo
Which causes errors to be displayed when certain files are missing.
Let's add a convenience function to manually check if the file is
readable before calling read.
OK. I wondered if this can become conditional to zsh (with the
common code still using eread, just the implementation of shell
function being different from zsh and bash), but I doubt it would be
worth it (other people can contradict me, of course).
Will queue. Thanks.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
contrib/completion/git-prompt.sh | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/contrib/completion/git-prompt.sh
b/contrib/completion/git-prompt.sh
index 7b732d2..1526086 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -259,6 +259,13 @@ __git_ps1_colorize_gitstring ()
r=$c_clear$r
}
+eread ()
+{
+ f=$1
+ shift
+ test -r $f read $@ $f
+}
+
# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
# when called from PS1 using command substitution
# in this mode it prints text to add to bash PS1 prompt (includes branch
name)
@@ -321,9 +328,9 @@ __git_ps1 ()
local step=
local total=
if [ -d $g/rebase-merge ]; then
- read b 2/dev/null $g/rebase-merge/head-name
- read step 2/dev/null $g/rebase-merge/msgnum
- read total 2/dev/null $g/rebase-merge/end
+ eread $g/rebase-merge/head-name b
+ eread $g/rebase-merge/msgnum step
+ eread $g/rebase-merge/end total
if [ -f $g/rebase-merge/interactive ]; then
r=|REBASE-i
else
@@ -331,10 +338,10 @@ __git_ps1 ()
fi
else
if [ -d $g/rebase-apply ]; then
- read step 2/dev/null $g/rebase-apply/next
- read total 2/dev/null $g/rebase-apply/last
+ eread $g/rebase-apply/next step
+ eread $g/rebase-apply/last total
if [ -f $g/rebase-apply/rebasing ]; then
- read b 2/dev/null $g/rebase-apply/head-name
+ eread $g/rebase-apply/head-name b
r=|REBASE
elif [ -f $g/rebase-apply/applying ]; then
r=|AM
@@ -358,7 +365,7 @@ __git_ps1 ()
b=$(git symbolic-ref HEAD 2/dev/null)
else
local head=
- if ! read head 2/dev/null $g/HEAD; then
+ if ! eread $g/HEAD head; then
if [ $pcmode = yes ]; then
PS1=$ps1pc_start$ps1pc_end
fi
--
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 v9 0/6] transport-helper: fixes
Felipe Contreras felipe.contre...@gmail.com writes: These patches add support for remote helpers --force, --dry-run, and reporting forced update. Changes since v8: --- a/transport-helper.c +++ b/transport-helper.c @@ -734,7 +734,7 @@ static int push_update_ref_status(struct strbuf *buf, } (*ref)-status = status; - (*ref)-forced_update = forced; + (*ref)-forced_update |= forced; (*ref)-remote_status = msg; return !(status == REF_STATUS_OK); } Hmph, isn't v8 already in 'master' as of 90e6255a (Merge branch 'fc/transport-helper-fixes', 2014-03-18)? Felipe Contreras (4): transport-helper: mismerge fix transport-helper: don't update refs in dry-run transport-helper: add 'force' to 'export' helpers transport-helper: check for 'forced update' message Richard Hansen (2): test-hg.sh: tests are now expected to pass remote-bzr: support the new 'force' option Documentation/gitremote-helpers.txt | 4 contrib/remote-helpers/git-remote-bzr | 31 ++- contrib/remote-helpers/test-bzr.sh| 22 +- contrib/remote-helpers/test-hg.sh | 4 ++-- git-remote-testgit.sh | 18 ++ t/t5801-remote-helpers.sh | 13 + transport-helper.c| 25 + 7 files changed, 105 insertions(+), 12 deletions(-) -- 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 4/5] transport-helper: trivial cleanup
Felipe Contreras felipe.contre...@gmail.com writes:
It's simpler to store the file names directly, and form the fast-export
arguments only when needed, and re-use the same strbuf with a format.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
transport-helper.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index b068ea5..2747f98 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -195,15 +195,9 @@ static struct child_process *get_helper(struct transport
*transport)
} else if (!strcmp(capname, signed-tags)) {
data-signed_tags = 1;
} else if (starts_with(capname, export-marks )) {
- struct strbuf arg = STRBUF_INIT;
- strbuf_addstr(arg, --export-marks=);
- strbuf_addstr(arg, capname + strlen(export-marks ));
- data-export_marks = strbuf_detach(arg, NULL);
+ data-export_marks = xstrdup(capname +
strlen(export-marks ));
} else if (starts_with(capname, import-marks)) {
- struct strbuf arg = STRBUF_INIT;
- strbuf_addstr(arg, --import-marks=);
- strbuf_addstr(arg, capname + strlen(import-marks ));
- data-import_marks = strbuf_detach(arg, NULL);
+ data-import_marks = xstrdup(capname +
strlen(import-marks ));
} else if (starts_with(capname, no-private-update)) {
data-no_private_update = 1;
} else if (mandatory) {
@@ -429,6 +423,7 @@ static int get_exporter(struct transport *transport,
struct child_process *helper = get_helper(transport);
int argc = 0, i;
memset(fastexport, 0, sizeof(*fastexport));
+ struct strbuf tmp = STRBUF_INIT;
Will fix up decl-after-stmt while queuing.
/* we need to duplicate helper-in because we want to use it after
* fastexport is done with it. */
@@ -438,10 +433,14 @@ static int get_exporter(struct transport *transport,
fastexport-argv[argc++] = --use-done-feature;
fastexport-argv[argc++] = data-signed_tags ?
--signed-tags=verbatim : --signed-tags=warn-strip;
- if (data-export_marks)
- fastexport-argv[argc++] = data-export_marks;
- if (data-import_marks)
- fastexport-argv[argc++] = data-import_marks;
+ if (data-export_marks) {
+ strbuf_addf(tmp, --export-marks=%s, data-export_marks);
+ fastexport-argv[argc++] = strbuf_detach(tmp, NULL);
+ }
+ if (data-import_marks) {
+ strbuf_addf(tmp, --import-marks=%s, data-import_marks);
+ fastexport-argv[argc++] = strbuf_detach(tmp, NULL);
+ }
for (i = 0; i revlist_args-nr; i++)
fastexport-argv[argc++] = revlist_args-items[i].string;
--
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 2/2] commit: add --ignore-submodules[=when] parameter
Ronald Weiss weiss.ron...@gmail.com writes:
On 14. 4. 2014 20:30, Junio C Hamano wrote:
Ronald Weiss weiss.ron...@gmail.com writes:
On 8. 4. 2014 20:43, Jens Lehmann wrote:
Useful when values for commit are 'all' (default) or 'none'. The others
('dirty' and 'untracked') have same effect as 'none', as commit is only
interested in whether the submodule's HEAD differs from what is commited
in the superproject.
Unless it outputs a status message, then 'dirty' and 'untracked' do
influence what is shown there. Apart from that (and maybe tests for
these two cases ;-) this is looking good to me.
OK, I updated the patch for commit to take that into account. Also, I
rebased both patches onto current master. Sending them in a moment.
If you don't have any more complaints, can I add Acked-by: you and
resend the patches to Junio?
It is not When I see no more complaints, I'll resend with your
Ack. An Ack is a positive thing, not lack of discovery of further
issues.
I'm really sorry if the tone of my message sounded harsh to you, it
wasn't meant like that at all.
No, that wasn't harsh at all. I just did not want to get a patch
with Acked-by with somebody's name on it, when there is not yet an
Ack, as that will confuse me greatly. My mental bandwidth is not
wide enough to keep track of all the in-flight topics I haven't yet
picked up.
I should be the one to say sorry if my message sounded harsh.
Thanks.
Rather, it is more like I'll wait for your Acks and then I'll
resend with your Ack, or If they look good, reply with Ack and let
the maintainer pick them up.
OK.
--
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/27] refs: add a concept of a reference transaction
Michael Haggerty mhag...@alum.mit.edu writes:
I forgot to confirm that the callers *do* verify that they don't pass
incorrect values to ref_transaction_create() and
ref_transaction_delete(). But if they wouldn't, then die(BUG:) would
not be appropriate either, would it? It would have to be die(you silly
user...).
Having the assert() there gives a confused message to the readers
(at least it did to this reader).
assert() implies that callers that are not buggy should not give
input that triggers the condition, which would mean it is the
callers' responsibility to sanity check the user-input to reject
creating a ref at 0{40} or deleting a ref whose old value is 0{40},
which in turn means these input are errors that need to be diagnosed
and documented.
But as you said below...
... even if the preconditions are not met, nothing really crazy
happens.
I agree that it also is perfectly fine to treat such input as
not-an-input-error.
It is a signal that these checks are not 'if (...) die()' that the
code may take that stance.
I cannot tell which interpretation the code is trying to implement.
Any one of the following I can understand:
(1) drop the assert(), declaring that the user input is perfectly
fine;
(2) keep the assert(), reject such user input at the callers, and
document that these are invalid inputs;
(3) replace the assert() with 'if (...) die(you silly user...)',
and document that these are invalid inputs; or
(4) same as (3) but replace with warn(), declaring such input as
suspicious.
but the current assert() makes the code look cannot decide ;-).
I would consider the first two more sensible than the other two, and
am leaning slightly towards (1) over (2).
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 5/5] transport-helper: fix sync issue on crashes
Felipe Contreras felipe.contre...@gmail.com writes:
When a remote helper crashes while pushing we should revert back to the
state before the push, however, it's possible that `git fast-export`
already finished its job, and therefore has exported the marks already.
This creates a synchronization problem because from that moment on
`git fast-{import,export}` will have marks that the remote helper is not
aware of and all further commands fail (if those marks are referenced).
The fix is to tell `git fast-export` to export to a temporary file, and
only after the remote helper has finishes successfully, move to the
final destination.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
This seems to be based on a somewhat older codebase; I tried to be
careful while adjusting the patch to the current codebase, but
please give it an eyeball to see if I didn't make any silly mistake
when I push today's integration result out in a few hours.
Thanks.
t/t5801-remote-helpers.sh | 17 -
transport-helper.c| 13 +++--
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 613f69a..cf7fd43 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -207,6 +207,17 @@ test_expect_success 'push update refs failure' '
)
'
+clean_mark () {
+ cut -f 2 -d ' ' $1 | git cat-file --batch-check | grep commit | sort
$(basename $1)
+}
+
+cmp_marks () {
+ test_when_finished rm -rf git.marks testgit.marks
+ clean_mark .git/testgit/$1/git.marks
+ clean_mark .git/testgit/$1/testgit.marks
+ test_cmp git.marks testgit.marks
+}
+
test_expect_success 'proper failure checks for fetching' '
(GIT_REMOTE_TESTGIT_FAILURE=1
export GIT_REMOTE_TESTGIT_FAILURE
@@ -221,7 +232,11 @@ test_expect_success 'proper failure checks for pushing' '
(GIT_REMOTE_TESTGIT_FAILURE=1
export GIT_REMOTE_TESTGIT_FAILURE
cd local
- test_must_fail git push --all
+ git checkout -b crash master
+ echo crash file
+ git commit -a -m crash
+ test_must_fail git push --all
+ cmp_marks origin
)
'
diff --git a/transport-helper.c b/transport-helper.c
index 2747f98..090c863 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -434,7 +434,7 @@ static int get_exporter(struct transport *transport,
fastexport-argv[argc++] = data-signed_tags ?
--signed-tags=verbatim : --signed-tags=warn-strip;
if (data-export_marks) {
- strbuf_addf(tmp, --export-marks=%s, data-export_marks);
+ strbuf_addf(tmp, --export-marks=%s.tmp, data-export_marks);
fastexport-argv[argc++] = strbuf_detach(tmp, NULL);
}
if (data-import_marks) {
@@ -901,7 +901,16 @@ static int push_refs_with_export(struct transport
*transport,
if (finish_command(exporter))
die(Error while running fast-export);
- return push_update_refs_status(data, remote_refs);
+ if (push_update_refs_status(data, remote_refs))
+ return 1;
+
+ if (data-export_marks) {
+ strbuf_addf(buf, %s.tmp, data-export_marks);
+ rename(buf.buf, data-export_marks);
+ strbuf_release(buf);
+ }
+
+ return 0;
}
static int push_refs(struct transport *transport,
--
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: On interpret-trailers standalone tool
Christian Couder chrisc...@tuxfamily.org writes: However, I am wondering if the current everything on the command line is instruction to the command is too limiting to allow the use of the tool both as a filter and as a tool that can work on one or more files named on the command line. If we start from there, the only way to later add these arguments are names of the files to be operated on is to add --file file1 --file file2... options, which feels quite backwards as a UNIX tool. Yeah, except that we could add for example a '-o' option that would take a directory as argument and that would mean that the command should operate on all the files in this directory. It would be like the -o option of the format-patch command. For output for which users do not know offhand what files are to be produced, giving a single directory with -o makes tons of sense, but for input, naming each individual file (and with help with shell globs *) is a lot more natural UNIX tool way, I would think. Take everything from this directory cannot be substitute for that, even though the reverse (i.e. by naming the input files with dir/*) is true. It is not a viable replacement. First, if you think that the command might often be used along with format-patch, ... I am not singling out format-patch output. Any text file/stream that has the commit log message may benefit from the trailers filter, and format-patch output is merely one very obvious example. As to the detection of the end of commit log message, the current EOF is where the log message ends (but we would remote trailing blank line) can easily be updated to EOF or the first three-dash line. Third, if trailers arguments are passed to the command using an option like -z token=value or -z token:value, it would be nice to the user for consistency if the same option could be used when passing the same arguments to git commit and perhaps other commands like git rebase, git cherry-pick and so on. This means that we now have to choose carefully the name of this option. Perhaps we can just give it a long name now like --trailer and care later about a short name,... Absolutely. That is a very sensible way to go. Fourth, some users might want the command to be passed some files as input, but they might not want the command to modify these input files. They might prefer the command to write its ouput into another set of output files. Maybe a syntax like cat or sed is not very well suited for this kind of use, while having a -o option for the output directory and a -i option for the input directory (if different from the output dir) would be nicer. Sure. I would expect we would require something like Perl's '-i' (in-place rewrite) option for this sequence to really work: git format-patch -o there -5 git that-command --options -i there/* and without, I would expect the output to come to its standard output. 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: Clear an invalid password out of the credential-cache?
On Sat, Apr 12, 2014 at 09:12:57AM -0400, Jason Pyeron wrote:
Is it me or is the only way to clear a single invalid password out of the
credential-cache is by git credential-cache exit?
Try the reject action:
$ : remember a credential
$ url() { echo url=https://example.com; }
$ (url; echo username=me; echo password=foo) | git credential-cache store
$ : get it again
$ url | git credential-cache get
username=me
password=foo
$ : forget it
$ url | git credential-cache erase
$ : now produces nothing
$ url | git credential-cache get
Git should do this for you automatically if it tries to use the password
and gets rejected (but only if the rejection is a password rejection,
like an HTTP 401).
-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] rebase: avoid non-function use of return on FreeBSD
Matthieu Moy matthieu@grenoble-inp.fr writes: Kyle J. McKay mack...@gmail.com writes: So I suggest that in the interest of fixing rebase on FreeBSD in an expeditious fashion, patches 1/3 and 2/3 get picked up (see note below) now and that the follow-on patch below, after being enhanced to pass all tests, be submitted separately at some future point. Seems a good plan to me. OK, should I take that an Ack on 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/3] rebase: avoid non-function use of return on FreeBSD
Kyle J. McKay mack...@gmail.com writes:
For convenience, here are the diffs using -w:
|--- a/git-rebase--am.sh
|+++ b/git-rebase--am.sh
|@@ -4,6 +4,7 @@
# Copyright (c) 2010 Junio C Hamano.
#
+git_rebase__am() {
case $action in
continue)
git am --resolved --resolvemsg=$resolvemsg
|@@ -73,3 +74,5 @@ then
fi
move_to_original_branch
+}
+git_rebase__am
I think we would want to see the actual change formatted this way
(without needing to pass -w to git show), as it will make it
clear that this artificial extra level of define the whole thing
inside a function and then make a single call to it is a workaround
of specific shell's glitch that we would not have to have in an
ideal world ;-)
Besides that would make it less likely to cause conflicts with the
real changes in flight.
Please double check what I queued on 'pu' when I push out today's
integration result.
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 v3 19/27] refs: add a concept of a reference transaction
On 04/14/2014 11:25 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
I forgot to confirm that the callers *do* verify that they don't pass
incorrect values to ref_transaction_create() and
ref_transaction_delete(). But if they wouldn't, then die(BUG:) would
not be appropriate either, would it? It would have to be die(you silly
user...).
Having the assert() there gives a confused message to the readers
(at least it did to this reader).
assert() implies that callers that are not buggy should not give
input that triggers the condition, which would mean it is the
callers' responsibility to sanity check the user-input to reject
creating a ref at 0{40} or deleting a ref whose old value is 0{40},
which in turn means these input are errors that need to be diagnosed
and documented.
In v2 of this patch series, I didn't forbid calling create with new_sha
== 0{40}, because, even though it's not what the user would think of as
creating a reference, the code didn't care--it would just verify that
the reference didn't exist before and then leave it undefined.
Then in [1] you commented:
Sounds a bit crazy that you can ask create, which verifies the
absense of the thing, to delete a thing.
I reacted to your comment by changing the documentation to forbid
calling create with new_sha1 == 0{40}, and enforced the rule using an
assert(). At the same time I added an analogous restriction that if
delete is called with have_old set, then old_sha1 must not be 0{40}.
In retrospect, you might have been objecting more to the misleading
docstring than to the behavior as implemented at the time. The
docstring implied that create could actually be used to delete a
reference, but that was not true: it always checked that the reference
didn't exist beforehand. So at worst it could leave a non-existent
reference un-created. Sorry for the confusion.
But as you said below...
... even if the preconditions are not met, nothing really crazy
happens.
I agree that it also is perfectly fine to treat such input as
not-an-input-error.
That was the case in v2.
It is a signal that these checks are not 'if (...) die()' that the
code may take that stance.
I cannot tell which interpretation the code is trying to implement.
Any one of the following I can understand:
(1) drop the assert(), declaring that the user input is perfectly
fine;
(2) keep the assert(), reject such user input at the callers, and
document that these are invalid inputs;
(3) replace the assert() with 'if (...) die(you silly user...)',
and document that these are invalid inputs; or
(4) same as (3) but replace with warn(), declaring such input as
suspicious.
but the current assert() makes the code look cannot decide ;-).
I would consider the first two more sensible than the other two, and
am leaning slightly towards (1) over (2).
The current status in v3 is that (2) is implemented.
Obviously I don't feel strongly about it, given that I implemented (1)
in v2. But I think that this restriction makes code at the calling site
easier to understand: create now always means create; i.e., if the
transaction goes through, then the reference exists afterwards. And
delete always means delete; i.e., afterwards, there is one less
reference in the world.
Michael
[1] http://mid.gmane.org/xmqqtxaczvod@gitster.dls.corp.google.com
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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 v2 6/9] branch: display publish branch
On Sat, Apr 12, 2014 at 10:05:15AM -0500, Felipe Contreras wrote:
As you can see; some branches are published, others are not. The ones that are
not published don't have a @{publish}, and `git branch -v` doesn't show them.
Why is that hard to understand?
Do you ever push the unpublished branches anywhere at all? If not, then
you would not have a tracking branch. E.g., git _would_ push to remote
gh, branch refs/heads/topic, but there is no remote tracking branch
refs/remotes/gh/topic, because you have never actually pushed there.
So there is no @{publish} branch.
Or do you have some branches in a state where they are pushed, but not
published? It wasn't clear to me from your example if your pu or
dev/remote/hg-extra ever get pushed.
I do not use git branch -v myself, so I don't personally care that
much how it behaves. But I do use a separate script that does the same
thing, and I would want it to show the ahead/behind relationship between
each branch and where it would be pushed to (and as I said, I define
mine with refspecs). Right now it uses nasty hackery to guess at where
things will be pushed, but ideally it would ask git via @{push} or some
similar mechanism.
If the former (you do not actually push them), then I think the
semantics I am looking for and the ones you want would coincide. If not,
then I think we are really talking about two different things.
-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
