[PATCH 19/19] ref_transaction_commit: bail out on failure to remove a ref
When removal of a loose or packed ref fails, bail out instead of
trying to finish the transaction. This way, a single error message
can be printed (instead of multiple messages being concatenated by
mistake) and the operator can try to solve the underlying problem
before there is a chance to muck things up even more.
In particular, when git fails to remove a ref, git goes on to try to
delete the reflog. Exiting early lets us keep the reflog.
When git succeeds in deleting a ref A and fails to remove a ref B, it
goes on to try to delete both reflogs. It would be better to just
remove the reflog for A, but that would be a more invasive change.
Failing early means we keep both reflogs, which puts the operator in a
good position to understand the problem and recover.
A long term goal is to avoid these problems altogether and roll back
the transaction on failure. That kind of transactionality will have
to wait for a later series (the plan for which is to make all
destructive work happen in a single update of the packed-refs file).
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
Thanks for reading.
refs.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 550223c..3b27758 100644
--- a/refs.c
+++ b/refs.c
@@ -3714,16 +3714,20 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- if (delete_ref_loose(update-lock, update-type, err))
+ if (delete_ref_loose(update-lock, update-type, err)) {
ret = -1;
+ goto cleanup;
+ }
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
}
- if (repack_without_refs(delnames, delnum, err))
+ if (repack_without_refs(delnames, delnum, err)) {
ret = -1;
+ goto cleanup;
+ }
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)
Junio C Hamano wrote:
Jonathan Nieder jrnie...@gmail.com writes:
These patches are also available from the git repository at
git://repo.or.cz/git/jrn.git tags/rs/ref-transaction
The tag fetched and built as-is seems to break 5514 among other
things (git remote rm segfaults).
Yeah, I noticed that right after sending the series out. :/
The patch below fixes it[1].
-- 8 --
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 11 Sep 2014 08:42:57 -0700
Subject: remote rm/prune: print a message when writing packed-refs fails
Until v2.1.0-rc0~22^2~11 (refs.c: add an err argument to
repack_without_refs, 2014-06-20), repack_without_refs forgot to
provide an error message when commit_packed_refs fails. Even today,
it only provides a message for callers that pass a non-NULL err
parameter. Internal callers in refs.c pass non-NULL err but
git remote does not.
That means that git remote rm and git remote prune can fail
without printing a message about why. Fix them by passing in a
non-NULL err parameter and printing the returned message.
This is the last caller to a ref handling function passing err ==
NULL. A later patch can drop support for err == NULL, avoiding such
problems in the future.
Change-Id: Ifb8a726ef03d0aa282a25a102313064d2e8ec283
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
[1] https://code-review.googlesource.com/1110
https://code-review.googlesource.com/1060
builtin/remote.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 6eaeee7..ef1ffc3 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,13 +750,16 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
+ struct strbuf err = STRBUF_INIT;
const char **branch_names;
int i, result = 0;
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
branch_names[i] = branches-items[i].string;
- result |= repack_without_refs(branch_names, branches-nr, NULL);
+ if (repack_without_refs(branch_names, branches-nr, err))
+ result |= error(%s, err.buf);
+ strbuf_release(err);
free(branch_names);
for (i = 0; i branches-nr; i++) {
@@ -1333,9 +1336,13 @@ static int prune_remote(const char *remote, int dry_run)
delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
for (i = 0; i states.stale.nr; i++)
delete_refs[i] = states.stale.items[i].util;
- if (!dry_run)
- result |= repack_without_refs(delete_refs,
- states.stale.nr, NULL);
+ if (!dry_run) {
+ struct strbuf err = STRBUF_INIT;
+ if (repack_without_refs(delete_refs, states.stale.nr,
+ err))
+ result |= error(%s, err.buf);
+ strbuf_release(err);
+ }
free(delete_refs);
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)
Junio C Hamano wrote:
Jonathan Nieder jrnie...@gmail.com writes:
Junio C Hamano wrote:
The tag fetched and built as-is seems to break 5514 among other
things (git remote rm segfaults).
Yeah, I noticed that right after sending the series out. :/
The patch below fixes it[1].
Is this meant to replace anything, or is it Oops, the previous ones
are broken, and this is to patch it up on top?
It's Oops, the next one (refs.c: do not permit err == NULL) is broken,
and this is to patch it up in advance. :)
But it should apply on top, too.
There were a few other minor changes, and I think with them the series
should be ready for next. My send and hope that more reviewers
appear strategy didn't really work, so I'll send a reroll of the
series as-is in an hour or so.
Here's an interdiff as a preview.
diff --git a/builtin/branch.c b/builtin/branch.c
index 5d5bc56..4bf931e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -238,9 +238,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
RESOLVE_REF_READING
| RESOLVE_REF_NODEREF
| RESOLVE_REF_ALLOW_BAD_NAME);
- if (!target ||
- (!(flags (REF_ISSYMREF|REF_ISBROKEN))
-is_null_sha1(sha1))) {
+ if (!target) {
error(remote_branch
? _(remote branch '%s' not found.)
: _(branch '%s' not found.), bname.buf);
@@ -268,8 +266,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
? _(Deleted remote branch %s (was %s).\n)
: _(Deleted branch %s (was %s).\n),
bname.buf,
- (flags REF_ISSYMREF)
- ? target
+ (flags REF_ISBROKEN) ? broken
+ : (flags REF_ISSYMREF) ? target
: find_unique_abbrev(sha1, DEFAULT_ABBREV));
}
delete_branch_config(bname.buf);
diff --git a/builtin/remote.c b/builtin/remote.c
index 6eaeee7..ef1ffc3 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,13 +750,16 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
+ struct strbuf err = STRBUF_INIT;
const char **branch_names;
int i, result = 0;
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
branch_names[i] = branches-items[i].string;
- result |= repack_without_refs(branch_names, branches-nr, NULL);
+ if (repack_without_refs(branch_names, branches-nr, err))
+ result |= error(%s, err.buf);
+ strbuf_release(err);
free(branch_names);
for (i = 0; i branches-nr; i++) {
@@ -1333,9 +1336,13 @@ static int prune_remote(const char *remote, int dry_run)
delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
for (i = 0; i states.stale.nr; i++)
delete_refs[i] = states.stale.items[i].util;
- if (!dry_run)
- result |= repack_without_refs(delete_refs,
- states.stale.nr, NULL);
+ if (!dry_run) {
+ struct strbuf err = STRBUF_INIT;
+ if (repack_without_refs(delete_refs, states.stale.nr,
+ err))
+ result |= error(%s, err.buf);
+ strbuf_release(err);
+ }
free(delete_refs);
}
--
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 v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)
Michael Haggerty wrote: Jonathan Nieder jrnie...@gmail.com writes: so I'll send a reroll of the series as-is in an hour or so. Jonathan: Is a current version of this patch series set up for review in Gerrit? Yes. (https://code-review.googlesource.com/#/q/project:git+topic:ref-transaction) Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 01/35] unable_to_lock_die(): rename function from unable_to_lock_index_die()
Michael Haggerty wrote: This function is used for other things besides the index, so rename it accordingly. Makes sense. [...] builtin/update-index.c | 2 +- cache.h| 2 +- lockfile.c | 6 +++--- refs.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 02/35] api-lockfile: expand the documentation
Michael Haggerty wrote: Document a couple more functions and the flags argument as used by hold_lock_file_for_update() and hold_lock_file_for_append(). Thanks. [...] --- 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 ++ +-- Context: this document has structure lockfile API Explanation of purpose (nice!). The functions - Quick descriptions of each of the four functions `hold_lock_file_for_update`, `commit_lock_file`, `rollback_lock_file`, `close_lock_file`. Reminder about lifetime of the lock_file structure. Description of cleanup convention (thou shalt either commit or roll back; if you forget to, the atexit handler will roll back for you). Long warning about the harder use cases. The above thou shalt was a lie --- you can also close_lock_file if you know what you're doing [jn: why is that function part of the public API?]. What's nice about the existing structure is that you can get a sense of how to use the API at a glance. Would there be a way to add this extra information while preserving that property? E.g.: lockfile API Nice brief explanation of purpose (is this the API I want to use?), as before. Calling sequence The caller: * Allocates a variable `struct lock_file lock` in the bss section or heap. Because the `lock_file` structure is used in an `atexit(3)` handler, its storage has to stay throughout the life of the program. It cannot be an auto variable allocated on the stack. * Attempts to create a lockfile by passing that variable and the filename of the final destination (e.g. `$GIT_DIR/index`) to `hold_lock_file_for_update` or `hold_lock_file_for_append`. + If the `die_on_error` flag is true, git dies if a lock is already taken for the file. * Writes new content for the destination file by writing to `lock-fd`. When finished writing, the caller can: * Close the file descriptor and rename the lockfile to its final destination by calling `commit_lock_file`. * Close the file descriptor and remove the lockfile by calling `rollback_lock_file`. * Close the file descriptor without removing or renaming the lockfile by calling `close_lock_file`. If you do not call one of `commit_lock_file`, `rollback_lock_file`, and `close_lock_file` and instead simply `exit(3)` from the program, an `atexit(3)` handler will close and remove the lockfile. You should never call `close(2)` on `lock-fd` yourself~ Otherwise the ... Error handling -- Functions return 0 on success, -1 on failure. errno is? isn't? meaningful on error. ... description of unable_to_lock_error and unable_to_lock_die here ... Flags - LOCK_NODEREF:: Usually symbolic links in the destination path are resolved and the lockfile is created by adding .lock to the resolved path. If `LOCK_NODEREF` is set, then the lockfile is created by adding .lock to the path argument itself. What is the user-visible effect of that flag? When would I want to pass that flag, and when wouldn't I? LOCK_DIE_ON_ERROR:: If a lock is already taken for the file, `die()` with an error message. If this option is not specified, trying to hold a lock file that is already taken will return -1 to the caller. Sensible? Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/35] rollback_lock_file(): do not clear filename redundantly
Michael Haggerty wrote: It is only necessary to clear the lock_file's filename field if it was not already clear. [...] --- a/lockfile.c +++ b/lockfile.c @@ -276,6 +276,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; Now that it does nothing when lk-filename[0] == '\0', this could be de-indented a little: if (!lk-filename[0]) return; if (lk-fd = 0) close(lk-fd); unlink_or_warn(lk-filename); lk-filename[0] = 0; This could technically double-close if interrupted by a signal that tries to remove the file again, right? Should this use close_lock_file which sets fd to -1 before closing? if (!lk-filename[0]) return; close_lock_file(lk); unlink_or_warn(lk-filename); lk-filename[0] = 0; With or without such changes, Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 04/35] rollback_lock_file(): exit early if lock is not active
Michael Haggerty wrote: Eliminate a layer of nesting. Oh --- guess I should have been more patient. :) Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 05/35] rollback_lock_file(): set fd to -1
Michael Haggerty wrote: --- a/lockfile.c +++ b/lockfile.c @@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk) return; if (lk-fd = 0) - close(lk-fd); + close_lock_file(lk); Doesn't need to be guarded by the 'if'. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 05/35] rollback_lock_file(): set fd to -1
Jonathan Nieder wrote: Michael Haggerty wrote: --- a/lockfile.c +++ b/lockfile.c @@ -276,7 +276,7 @@ void rollback_lock_file(struct lock_file *lk) return; if (lk-fd = 0) -close(lk-fd); +close_lock_file(lk); Doesn't need to be guarded by the 'if'. Err, yes it does. Why doesn't close_lock_file bail out early when fd 0? In any case, Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 06/35] lockfile: unlock file if lockfile permissions cannot be adjusted
Michael Haggerty wrote: Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- lockfile.c | 1 + 1 file changed, 1 insertion(+) Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 07/35] hold_lock_file_for_append(): release lock on errors
Michael Haggerty wrote:
--- a/lockfile.c
+++ b/lockfile.c
@@ -219,13 +219,13 @@ int hold_lock_file_for_append(struct lock_file *lk,
const char *path, int flags)
if (errno != ENOENT) {
if (flags LOCK_DIE_ON_ERROR)
die(cannot open '%s' for copying, path);
- close(fd);
+ rollback_lock_file(lk);
return error(cannot open '%s' for copying, path);
Makes sense.
Now that I'm here, I wonder a little at the error convention. If the
caller doesn't pass LOCK_DIE_ON_ERROR, are they supposed to be able to
use unable_to_lock_message? What errno would they pass in the err
parameter? Would callers want handle failure to acquire a lock
differently from other errors (e.g., by sleeping and trying again),
and if not, what is the optionally-die behavior in hold_lock_file
about?
In any case,
Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 09/35] lockfile.c: document the various states of lock_file objects
Michael Haggerty wrote: --- a/lockfile.c +++ b/lockfile.c @@ -4,6 +4,63 @@ #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: This overlaps a lot with the API doc, which makes me worry a little about it going out of date. Would improving the API doc help, or if aspects are especially relevant here, is there a way to summarize them more quickly to avoid some of the redundancy? [...] + * A lock_file object can be in several states: Would it make sense for this information to go near the definition of 'struct lock_file'? That's where I'd start if I were looking for information on what states a lock_file can be in. My two cents, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 10/35] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN
Michael Haggerty wrote: There are a few places that use these values, so define constants for them. Seems like a symptom of the API leaving out a useful helper (e.g., something that strips off the lock suffix and returns a memdupz'd filename). [...] --- a/cache.h +++ b/cache.h @@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not needs update */ extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); +/* String appended to a filename to derive the lockfile name: */ +#define LOCK_SUFFIX .lock +#define LOCK_SUFFIX_LEN 5 My suspicion is that error handling would be better if fewer callers outside of lockfile.c did the '- LOCK_SUFFIX_LEN' dance, so this seems like a step in the wrong direction. Adding constants in lockfile.c would make sense, though. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 11/35] delete_ref_loose(): don't muck around in the lock_file's filename
Michael Haggerty wrote: It's bad manners. Especially since there could be a signal during the call to unlink_or_warn(), in which case the signal handler will see the wrong filename and delete the reference file, leaving the lockfile behind. So make our own copy to work with. Nice. Could this be a helper, to help others avoid a PATH_MAX sized buffer too? [...] --- a/refs.c +++ b/refs.c @@ -2551,12 +2551,15 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err) static int delete_ref_loose(struct ref_lock *lock, int flag) [...] + /* + * loose. The loose file name is the same as the + * lockfile name, minus .lock: + */ + char *loose_filename = xmemdupz( + lock-lk-filename, + strlen(lock-lk-filename) - LOCK_SUFFIX_LEN); Ronnie mentioned that this would misbehave in the !lock-lk-filename[0] case, which shouldn't come up because lock_ref_sha1_basic dies on error but seems worth futureproofing against. Maybe something like assert(lock-lk-filename[0]); would be enough to make the assumption obvious for people reading. Otherwise looks good. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 12/35] prepare_index(): declare return value to be (const char *)
Michael Haggerty wrote: 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(-) Reviewed-by: Jonathan Nieder jrnie...@gmail.com I wonder if we should just bite the bullet and make lock_file an opaque struct with accessors for int fd and const char *filename. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 14/35] lock_file(): exit early if lockfile cannot be opened
Michael Haggerty wrote: lockfile.c | 23 +++ 1 file changed, 11 insertions(+), 12 deletions(-) Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 15/35] remove_lock_file(): call rollback_lock_file()
Michael Haggerty wrote: lockfile.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) Nice. Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 16/35] commit_lock_file(): inline temporary variable
Michael Haggerty wrote:
--- a/lockfile.c
+++ b/lockfile.c
@@ -311,12 +311,14 @@ int reopen_lock_file(struct lock_file *lk)
int commit_lock_file(struct lock_file *lk)
{
char result_file[PATH_MAX];
- size_t i;
+
if (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;
+
Same comment as the delete_ref_loose patch: the reader can gain some
piece of mind from an assertion at the top that makes sure filename is
valid (which it always should be, according to the API):
assert(lk-filename[0]);
It would also be nice to use the same xmemdupz-based code as in the
delete_ref_loose case (ideally via a helper), to avoid having to work
with a PATH_MAX-sized buffer.
Otherwise looks good.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 17/35] commit_lock_file(): die() if called for unlocked lockfile object
Michael Haggerty wrote:
--- a/lockfile.c
+++ b/lockfile.c
@@ -312,6 +312,9 @@ int commit_lock_file(struct lock_file *lk)
{
char result_file[PATH_MAX];
+ if (!lk-filename[0])
+ die(BUG: attempt to commit unlocked object);
Sure, this is fine instead of an assert() too. :)
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back
Michael Haggerty wrote: If closing an open lockfile fails, then we cannot be sure of the contents of the lockfile Is that true? It seems more like a bug in close_lock_file: if it fails, perhaps it should either set lk-fd back to fd or unlink the lockfile itself. What do other callers do on close_lock_file failure? Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 19/35] commit_lock_file(): rollback lock file on failure to rename
Michael Haggerty wrote: If rename() fails, call rollback_lock_file() to delete the lock file (in case it is still present) and reset the filename field to the empty string so that the lockfile object is left in a valid state. Can you spell out more what the goal is? Is the idea to keep the .lock file for a shorter period of time, so other processes can lock that file before the current process exits? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 20/35] api-lockfile: document edge cases
Michael Haggerty wrote: * Document the behavior of commit_lock_file() when it fails, namely that it rolls back the lock_file object and sets errno appropriately. * Document the behavior of rollback_lock_file() when called for a lock_file object that has already been committed or rolled back, namely that it is a NOOP. I think this would be easier to read in a separate error handling section. That way, when writing new code using the lockfile API, I can quickly find what functions to use and quickly find out what the error handling conventions are --- each use of the documentation wouldn't interfere with the other. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 21/35] dump_marks(): remove a redundant call to rollback_lock_file()
Michael Haggerty wrote: Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- fast-import.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Jonathan Nieder jrnie...@gmail.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 v5 22/35] git_config_set_multivar_in_file(): avoid call to rollback_lock_file()
Michael Haggerty wrote: After commit_lock_file() is called, then the lock_file object is necessarily either committed or rolled back. So there is no need to call rollback_lock_file() again in either of these cases. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu This seems to involve an unadvertised behavior change: before it wouldn't call git_config_clear() on failure, and after the patch it would. Intended? I think it would be clearer with the goto for exception handling maintained (and just a second 'lock = NULL' assignment). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 23/35] lockfile: avoid transitory invalid states
Michael Haggerty wrote:
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.
Nice.
[...]
--- a/cache.h
+++ b/cache.h
@@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned
int flags, const struct
struct lock_file {
struct lock_file *next;
+ volatile sig_atomic_t active;
int fd;
pid_t owner;
char on_list;
[...]
+++ b/lockfile.c
@@ -27,16 +27,19 @@
[...]
@@ -189,9 +198,14 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
atexit(remove_lock_file);
}
+ if (lk-active)
+ die(BUG: lock_file(\%s\) called with an active lock_file
object,
+ path);
The error message doesn't make it entirely obvious to me what went
wrong.
Maybe something like
die(BUG: cannot lock_file(\%s\) on active struct lock_file,
path);
lock_file already assumed on_list was initialized to zero but it
wasn't particularly obvious since everything else is blindly
scribbled over. Probably worth mentioning in the API docs that
the lock_file should be zeroed before calling hold_lock_file_...
[...]
@@ -326,6 +341,7 @@ int commit_lock_file(struct lock_file *lk)
if (rename(lk-filename, result_file))
goto rollback;
+ lk-active = 0;
lk-filename[0] = 0;
Is it useful to set filename[0] any more?
It seems potentially fragile to set both, since new code can appear
that only sets or checks one or the other. Would it make sense to
rename the filename field to make sure no new callers relying on the
filename[0] convention sneak in (with the new convention being that
the filename doesn't get cleared, to avoid problems)?
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)
Junio C Hamano wrote: Jonathan: Is a current version of this patch series set up to be fetched so that it can be reviewed outside Gerrit? The current tip is 06d707cb63e34fc55a18ecc47e668f3c44acae57 from https://code.googlesource.com/git (fetch-by-sha1 should work). Each reroll gets its own refname of the form refs/changes/62/1062/number with number increasing. The Download widget in the top-right corner of https://code-review.googlesource.com/1062 shows the refname. There are plenty of unaddressed comments from Michael, so I'd hold off on picking up the latest for pu for now. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)
Junio C Hamano wrote: Does the order of changes that appear in https://code-review.googlesource.com/#/q/project:git+branch:master+topic:ref-transaction have any significance? e.g. is a topic supposed to be a single strand of pearls on top of the branch, and the top one is the tip, or something? Alas no, though that would be a good feature. Search results are ordered by which was last updated (for example when someone adds a comment). The 'Related Changes' shown on the change screen are in topological order, with the tip at the top. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 09/35] lockfile.c: document the various states of lock_file objects
Michael Haggerty wrote:
I agree with your point about overlap. I will split the documentation
into two parts with less redundancy:
* Documentation/technical/api-lockfile.txt: How to use the API.
* lockfile.{c,h}: Internal implementation details.
I think the implementation details would get lost among the thousand
things in cache.h. So instead, I will add a commit on top of the patch
series that splits out a lockfile.h header file (which I think is a good
idea anyway), and moves the internal documentation there. Sound good?
Yep, a separate lockfile.h header file sounds sensible to me.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] use REALLOC_ARRAY for changing the allocation size of arrays
René Scharfe wrote: --- a/khash.h +++ b/khash.h (not really about this patch) Where did this file come from? Do we want to be able to sync with upstream to get later bugfixes (e.g., https://github.com/attractivechaos/klib/commit/000f0890)? If so, it might make sense to avoid unnecessary changes to the file so it's easier to see when it's safe to replace the file wholesale with a new version. Curious, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)
Junio C Hamano wrote: I know that a review-update cycle is still going in the dark at https://code-review.googlesource.com/#/q/topic:ref-transaction for this series. Eh, it's at least public and doesn't flood the list with rebased versions of the series. Would you prefer if there were some list archived on gmane with the automated mails from gerrit, to make it easier to look back at later? Are we almost there for v22 which hopefully be the final before we merge it to 'next' and go incremental? The patch fix handling of badly named refs[1] is still undergoing heavy churn. I think it's worth getting that one right. Thanks, Jonathan [1] https://code-review.googlesource.com/1070 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: `git log relative_path_to_object` does not respect the --work-tree path
Hi Roberto, Roberto Eduardo Decurnex Gorosito wrote: When passing objects to the `git log`, by just naming them or using the `--objects` option, relative paths are evaluated using the current working directory instead of the current working tree path. Why should they be relative to the worktree root? When you use relative paths within a worktree, they are not relative to the worktree root. For example, the following works within a clone of git.git: $ cd Documentation $ git log git.txt You might be looking for 'git -C directory', which chdirs to the named directory so paths are relative to there. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: $GIT_CONFIG should either apply to all commands, or none at all
Hi, Frédéric Brière wrote[1]: This kind of stuff caused me a lot of hair-pulling: $ git config core.abbrev 32 git log --pretty=oneline --abbrev-commit 89be foo Here's the source of the discrepancy: $ grep abbrev $GIT_CONFIG .git/config git.conf: abbrev=32 .git/config:abbrev=4 Since dc87183, $GIT_CONFIG is ignored by any other Git command, but it *still* applies to git-config. This basically means that values obtained via git-config are not necessarily those which are actually in effect. The really frustrating part (for me, at least) is that for any tool (gitweb in my case) which uses git-config, values from $GIT_CONFIG will take effect for that tool, but not for any subsequent Git command. git-config(1) doesn't make this clear either; it mentions $GIT_CONFIG as the configuration, without saying explicitly that this environment variable only applies to git-config. Yep. One possibility would be to do something like the following (A): 1) advertise in the git-config(1) manpage that the GIT_CONFIG environment variable only affects the behavior of the 'git config' command 2) introduce an environment variable GIT_I_AM_PORCELAIN. (If doing this, we could come up with a better name, but this is just an illustration.) Set and export that envvar in git-sh-setup.sh. When that environment variable is set, make git-config stop paying attention to GIT_CONFIG. That way, git commands that happen to be scripts would not be affected by the GIT_CONFIG setting any more. 3) Warn when 'git config' is called with GIT_CONFIG set, explaining that support will eventually be removed and that callers should pass --file= instead. 4) Once we're confident there are no scripts in the wild relying on that envvar, remove support for it. Another possibility (B): 1) Teach git's commands in C to respect the GIT_CONFIG environment variable. Semantics: only configuration from that file would be respected and all other configuration will be ignored. Advertise it in the git(1) manpage. 2) Gnash teeth a little but continue to support it. Yet another possibility (C): 1) Just skip to step (4) from plan (A). C is kind of temping. Do you know if there are scripts in the wild that rely on the GIT_CONFIG setting working? Thanks for reporting, Jonathan [1] http://bugs.debian.org/763712 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v22 0/24] rs/ref-transaction
Jonathan Nieder wrote: Jonathan Nieder wrote: The next series from Ronnie's collection is available at https://code-review.googlesource.com/#/q/topic:ref-transaction in case someone wants a fresh series to look at. Here is the outcome of that review. It could use another set of eyes (hint, hint) Another set of eyes arrived and helped. Here's a reroll. Jonathan Nieder (6): mv test: recreate mod/ directory instead of relying on stale copy branch -d: avoid repeated symref resolution packed-ref cache: forbid dot-components in refnames refs.c: do not permit err == NULL lockfile: remove unable_to_lock_error ref_transaction_commit: bail out on failure to remove a ref Junio C Hamano (1): reflog test: test interaction with detached HEAD Ronnie Sahlberg (17): wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as success wrapper.c: add a new function unlink_or_msg refs.c: add an err argument to delete_ref_loose refs.c: pass the ref log message to _create/delete/update instead of _commit rename_ref: don't ask read_ref_full where the ref came from refs.c: refuse to lock badly named refs in lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: pass a list of names to skip to is_refname_available refs.c: ref_transaction_commit: distinguish name conflicts from other errors fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static refs.c: change resolve_ref_unsafe reading argument to be a flags field branch -d: simplify by using RESOLVE_REF_READING flag test: put tests for handling of bad ref names in one place refs.c: allow listing and deleting badly named refs for-each-ref.c: improve message before aborting on broken ref remote rm/prune: print a message when writing packed-refs fails branch.c | 6 +- builtin/blame.c | 2 +- builtin/branch.c | 22 ++- builtin/checkout.c | 6 +- builtin/clone.c | 2 +- builtin/commit.c | 6 +- builtin/fetch.c | 34 ++-- builtin/fmt-merge-msg.c | 2 +- builtin/for-each-ref.c | 11 +- builtin/fsck.c | 2 +- builtin/log.c| 3 +- builtin/merge.c | 2 +- builtin/notes.c | 2 +- builtin/receive-pack.c | 9 +- builtin/remote.c | 20 ++- builtin/replace.c| 5 +- builtin/show-branch.c| 7 +- builtin/symbolic-ref.c | 2 +- builtin/tag.c| 4 +- builtin/update-ref.c | 13 +- bundle.c | 2 +- cache.h | 42 +++-- fast-import.c| 8 +- git-compat-util.h| 16 +- http-backend.c | 4 +- lockfile.c | 10 -- notes-merge.c| 2 +- reflog-walk.c| 5 +- refs.c | 438 --- refs.h | 40 +++-- remote.c | 11 +- sequencer.c | 8 +- t/t1400-update-ref.sh| 62 +++ t/t1413-reflog-detach.sh | 70 t/t1430-bad-ref-name.sh | 207 ++ t/t3200-branch.sh| 9 + t/t7001-mv.sh| 15 +- t/t9300-fast-import.sh | 30 transport-helper.c | 5 +- transport.c | 5 +- upload-pack.c| 2 +- walker.c | 5 +- wrapper.c| 28 ++- wt-status.c | 2 +- 44 files changed, 838 insertions(+), 348 deletions(-) create mode 100755 t/t1413-reflog-detach.sh create mode 100755 t/t1430-bad-ref-name.sh -- 2.0.0.450.ga793d96 --- Changes since v21: branch.c| 2 +- builtin/blame.c | 2 +- builtin/branch.c| 25 ++--- builtin/checkout.c | 6 +- builtin/clone.c | 2 +- builtin/commit.c| 2 +- builtin/fetch.c | 6 +- builtin/fmt-merge-msg.c | 2 +- builtin/for-each-ref.c | 11 +- builtin/fsck.c | 2 +- builtin/log.c | 4 +- builtin/merge.c | 2 +- builtin/notes.c | 2 +- builtin/receive-pack.c | 4 +- builtin/remote.c| 21 ++-- builtin/show-branch.c | 9 +- builtin/symbolic-ref.c | 2 +- builtin/update-ref.c| 3 +- bundle.c| 2 +- cache.h | 33 -- http-backend.c | 5 +- notes-merge.c | 2 +- reflog-walk.c | 6 +- refs.c | 263 +--- refs.h | 34 +++--- remote.c| 13 ++- sequencer.c | 4 +- t/t1400-update-ref.sh | 46 ++-- t/t1402-check-ref-format.sh | 48 t/t1413-reflog-detach.sh| 70 t/t1430-bad-ref-name.sh | 207 ++ t/t9300-fast-import.sh | 30 - transport
[PATCH 01/24] mv test: recreate mod/ directory instead of relying on stale copy
Date: Wed, 10 Sep 2014 14:01:46 -0700 The tests for 'git mv moves a submodule' functionality often run commands like git mv sub mod/sub to move a submodule into a subdirectory. Just like plain /bin/mv, this is supposed to succeed if the mod/ parent directory exists and fail if it doesn't exist. Usually these tests mkdir the parent directory beforehand, but some instead rely on it being left behind by previous tests. More precisely, when 'git reset --hard' tries to move to a state where mod/sub is not present any more, it would perform the following operations: rmdir(mod/sub) rmdir(mod) The first fails with ENOENT because the test script removed mod/sub with rm -rf already, so 'reset --hard' doesn't bother to move on to the second, and the mod/ directory is kept around. Better to explicitly remove and re-create the mod/ directory so later tests don't have to depend on the directory left behind by the earlier ones at all (making it easier to rearrange or skip some tests in the file or to tweak 'reset --hard' behavior without breaking unrelated tests). Noticed while testing a patch that fixes the reset --hard behavior described above. Signed-off-by: Jonathan Nieder jrnie...@gmail.com Reviewed-by: Ronnie Sahlberg sahlb...@google.com --- As before. t/t7001-mv.sh | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 54d7807..69f11bd 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -350,10 +350,11 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu ' test_expect_success 'git mv moves a submodule with gitfile' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod ( cd mod git mv ../sub/ . @@ -372,11 +373,12 @@ test_expect_success 'git mv moves a submodule with gitfile' ' ' test_expect_success 'mv does not complain when no .gitmodules file is found' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git rm .gitmodules entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod git mv sub mod/sub 2actual.err ! test -s actual.err ! test -e sub @@ -390,11 +392,12 @@ test_expect_success 'mv does not complain when no .gitmodules file is found' ' ' test_expect_success 'mv will error out on a modified .gitmodules file unless staged' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git config -f .gitmodules foo.bar true entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod test_must_fail git mv sub mod/sub 2actual.err test -s actual.err test -e sub @@ -413,13 +416,14 @@ test_expect_success 'mv will error out on a modified .gitmodules file unless sta ' test_expect_success 'mv issues a warning when section is not found in .gitmodules' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git config -f .gitmodules --remove-section submodule.sub git add .gitmodules entry=$(git ls-files --stage sub | cut -f 1) echo warning: Could not find section in .gitmodules where path=sub expect.err + mkdir mod git mv sub mod/sub 2actual.err test_i18ncmp expect.err actual.err ! test -e sub @@ -433,9 +437,10 @@ test_expect_success 'mv issues a warning when section is not found in .gitmodule ' test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update + mkdir mod git mv -n sub mod/sub 2actual.err test -f sub/.git git diff-index --exit-code HEAD -- 2.1.0.rc2.206.gedb03e5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/24] wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as success
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Jul 2014 11:01:18 -0700
Simplify the function warn_if_unremovable slightly. Additionally, change
behaviour slightly. If we failed to remove the object because the object
does not exist, we can still return success back to the caller since none of
the callers depend on fail if the file did not exist.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Change since v21:
- adjust caller to remove redundant errno check
git-compat-util.h | 7 +--
refs.c| 2 +-
wrapper.c | 14 ++
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index b6f03b3..611e77b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -706,11 +706,14 @@ void git_qsort(void *base, size_t nmemb, size_t size,
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
- * Always returns the return value of unlink(2).
+ * Returns 0 on success which includes trying to unlink an object that does
+ * not exist.
*/
int unlink_or_warn(const char *path);
/*
- * Likewise for rmdir(2).
+ * Preserves errno, prints a message, but gives no warning for ENOENT.
+ * Returns 0 on success which includes trying to remove a directory that does
+ * not exist.
*/
int rmdir_or_warn(const char *path);
/*
diff --git a/refs.c b/refs.c
index 7235574..d0565b1 100644
--- a/refs.c
+++ b/refs.c
@@ -2557,7 +2557,7 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
lock-lk-filename[i] = 0;
err = unlink_or_warn(lock-lk-filename);
lock-lk-filename[i] = '.';
- if (err errno != ENOENT)
+ if (err)
return 1;
}
return 0;
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..c9605cd 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
static int warn_if_unremovable(const char *op, const char *file, int rc)
{
- if (rc 0) {
- int err = errno;
- if (ENOENT != err) {
- warning(unable to %s %s: %s,
- op, file, strerror(errno));
- errno = err;
- }
- }
+ int err;
+ if (!rc || errno == ENOENT)
+ return 0;
+ err = errno;
+ warning(unable to %s %s: %s, op, file, strerror(errno));
+ errno = err;
return rc;
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/24] wrapper.c: add a new function unlink_or_msg
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Jul 2014 11:20:36 -0700
This behaves like unlink_or_warn except that on failure it writes the message
to its 'err' argument, which the caller can display in an appropriate way or
ignore.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
---
As before.
git-compat-util.h | 9 +
wrapper.c | 14 ++
2 files changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 611e77b..5ee140c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -307,6 +307,8 @@ extern char *gitbasename(char *);
#include wildmatch.h
+struct strbuf;
+
/* General helper functions */
extern void vreportf(const char *prefix, const char *err, va_list params);
extern void vwritef(int fd, const char *prefix, const char *err, va_list
params);
@@ -710,6 +712,13 @@ void git_qsort(void *base, size_t nmemb, size_t size,
* not exist.
*/
int unlink_or_warn(const char *path);
+ /*
+ * Tries to unlink file. Returns 0 if unlink succeeded
+ * or the file already didn't exist. Returns -1 and
+ * appends a message to err suitable for
+ * 'error(%s, err-buf)' on error.
+ */
+int unlink_or_msg(const char *file, struct strbuf *err);
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
* Returns 0 on success which includes trying to remove a directory that does
diff --git a/wrapper.c b/wrapper.c
index c9605cd..ec7a08b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -438,6 +438,20 @@ static int warn_if_unremovable(const char *op, const char
*file, int rc)
return rc;
}
+int unlink_or_msg(const char *file, struct strbuf *err)
+{
+ int rc = unlink(file);
+
+ assert(err);
+
+ if (!rc || errno == ENOENT)
+ return 0;
+
+ strbuf_addf(err, unable to unlink %s: %s,
+ file, strerror(errno));
+ return -1;
+}
+
int unlink_or_warn(const char *file)
{
return warn_if_unremovable(unlink, file, unlink(file));
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/24] refs.c: add an err argument to delete_ref_loose
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 15 May 2014 08:25:23 -0700
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_ref_loose.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Changes since v21:
- s/delete_loose_ref/delete_ref_loose/ once in commit message (but
the other one still needs fixing)
refs.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index d0565b1..5609622 100644
--- a/refs.c
+++ b/refs.c
@@ -2548,16 +2548,16 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
return ret;
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+ int res, i = strlen(lock-lk-filename) - 5; /* .lock */
lock-lk-filename[i] = 0;
- err = unlink_or_warn(lock-lk-filename);
+ res = unlink_or_msg(lock-lk-filename, err);
lock-lk-filename[i] = '.';
- if (err)
+ if (res)
return 1;
}
return 0;
@@ -3604,7 +3604,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type);
+ ret |= delete_ref_loose(update-lock, update-type,
+ err);
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/24] rename_ref: don't ask read_ref_full where the ref came from
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 30 Apr 2014 12:41:04 -0700
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
---
As before.
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 99a9b86..39571f5 100644
--- a/refs.c
+++ b/refs.c
@@ -2671,7 +2671,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, flag)
+ if (!read_ref_full(newrefname, sha1, 1, NULL)
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path(%s,
newrefname))) {
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/24] refs.c: pass the ref log message to _create/delete/update instead of _commit
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 30 Apr 2014 12:22:42 -0700
Change the ref transaction API so that we pass the reflog message to the
create/delete/update functions instead of to ref_transaction_commit.
This allows different reflog messages for each ref update in a multi-ref
transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Changes since v21:
- cleaned up commit message
- clarified ownership of msg in comment in refs.h
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 5 +++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 34 +-
refs.h | 12 ++--
sequencer.c| 4 ++--
walker.c | 5 ++---
11 files changed, 54 insertions(+), 44 deletions(-)
diff --git a/branch.c b/branch.c
index 37ac555..76a8ec9 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, err) ||
- ref_transaction_commit(transaction, msg, err))
+ null_sha1, 0, !forcing, msg, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
strbuf_release(err);
diff --git a/builtin/commit.c b/builtin/commit.c
index 9bf1003..d23e876 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1767,8 +1767,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, HEAD, sha1,
current_head
? current_head-object.sha1 : NULL,
- 0, !!current_head, err) ||
- ref_transaction_commit(transaction, sb.buf, err)) {
+ 0, !!current_head, sb.buf, err) ||
+ ref_transaction_commit(transaction, err)) {
rollback_index_files();
die(%s, err.buf);
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 224fadc..d1f4cf7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -585,8 +585,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, err) ||
- ref_transaction_commit(transaction, push, err)) {
+ new_sha1, old_sha1, 0, 1, push,
+ err) ||
+ ref_transaction_commit(transaction, err)) {
ref_transaction_free(transaction);
rp_error(%s, err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 1fcd06d..9d03b84 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -169,8 +169,9 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(err);
if (!transaction ||
- ref_transaction_update(transaction, ref, repl, prev, 0, 1, err) ||
- ref_transaction_commit(transaction, NULL, err))
+ ref_transaction_update(transaction, ref, repl, prev,
+ 0, 1, NULL, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index f3f172f..70d28c5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
- 0, 1, err) ||
- ref_transaction_commit(transaction, NULL, err))
+ 0, 1, NULL, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
if (force !is_null_sha1(prev) hashcmp(prev, object))
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 54a48c0..6c9be05 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -14,6 +14,7 @@ static const char * const git_update_ref_usage[] = {
static char line_termination = '\n';
static int update_flags;
+static const char *msg
[PATCH 07/24] refs.c: refuse to lock badly named refs in lock_ref_sha1_basic
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 10:40:10 -0700
Move the check for check_refname_format from lock_any_ref_for_update to
lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely. This has no visible impact to callers
except for the inability to lock badly named refs, which is not possible
today already for other reasons.(*)
Keep lock_any_ref_for_update as a no-op wrapper. It is the public facing
version of this interface and keeping it as a separate function will make
it easier to experiment with the internal lock_ref_sha1_basic signature.
(*) For example, if lock_ref_sha1_basic checks the refname format and
refuses to lock badly named refs, it will not be possible to delete
such refs because the first step of deletion is to lock the ref. We
currently already fail in that case because these refs are not recognized
to exist:
$ cp .git/refs/heads/master .git/refs/heads/echo...\*\*
$ git branch -D .git/refs/heads/echo...\*\*
error: branch '.git/refs/heads/echo...**' not found.
This has been broken for a while. Later patches in the series will start
repairing the handling of badly named refs.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Change since v21:
- clarified commit message
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 39571f5..3c2ce57 100644
--- a/refs.c
+++ b/refs.c
@@ -2091,6 +2091,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int missing = 0;
int attempts_remaining = 3;
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ errno = EINVAL;
+ return NULL;
+ }
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock-lock_fd = -1;
@@ -2182,8 +2187,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/24] refs.c: call lock_ref_sha1_basic directly from commit
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 10:43:39 -0700
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
---
As before.
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 3c2ce57..f124c2b 100644
--- a/refs.c
+++ b/refs.c
@@ -3578,12 +3578,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = lock_any_ref_for_update(update-refname,
- (update-have_old ?
- update-old_sha1 :
- NULL),
- update-flags,
- update-type);
+ update-lock = lock_ref_sha1_basic(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/24] refs.c: pass a list of names to skip to is_refname_available
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 11:16:07 -0700
Change is_refname_available to take a list of strings to exclude when
checking for conflicts instead of just one single name. We can already
exclude a single name for the sake of renames. This generalizes that support.
ref_transaction_commit already tracks a set of refs that are being deleted
in an array. This array is then used to exclude refs from being written to
the packed-refs file. At some stage we will want to change this array to a
struct string_list and then we can pass it to is_refname_available via the
call to lock_ref_sha1_basic. That will allow us to perform transactions
that perform multiple renames as long as there are no conflicts within the
starting or ending state.
For example, that would allow a single transaction that contains two
renames that are both individually conflicting:
m - n/n
n - m/m
No functional change intended yet.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Since v21:
- clarified commit message
- clarified comments
refs.c | 44 +---
1 file changed, 29 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index f124c2b..6820c93 100644
--- a/refs.c
+++ b/refs.c
@@ -801,14 +801,16 @@ static int names_conflict(const char *refname1, const
char *refname2)
struct name_conflict_cb {
const char *refname;
- const char *oldrefname;
const char *conflicting_refname;
+ struct string_list *skiplist;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
- if (data-oldrefname !strcmp(data-oldrefname, entry-name))
+
+ if (data-skiplist
+ string_list_has_string(data-skiplist, entry-name))
return 0;
if (names_conflict(data-refname, entry-name)) {
data-conflicting_refname = entry-name;
@@ -820,17 +822,18 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
/*
* Return true iff a reference named refname could be created without
* conflicting with the name of an existing reference in dir. If
- * oldrefname is non-NULL, ignore potential conflicts with oldrefname
- * (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * skiplist is non-NULL, ignore potential conflicts with names in
+ * skiplist (e.g., because those refs are scheduled for deletion in
+ * the same operation). skiplist must be sorted.
*/
-static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+static int is_refname_available(const char *refname,
+ struct ref_dir *dir,
+ struct string_list *skiplist)
{
struct name_conflict_cb data;
data.refname = refname;
- data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skiplist = skiplist;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) {
@@ -2080,6 +2083,7 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
*/
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
+ struct string_list *skiplist,
int flags, int *type_p)
{
char *ref_file;
@@ -2129,7 +2133,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing
-!is_refname_available(refname, NULL, get_packed_refs(ref_cache)))
{
+!is_refname_available(refname, get_packed_refs(ref_cache),
+ skiplist)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2187,7 +2192,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, NULL, flags, type_p);
}
/*
@@ -2648,6 +2653,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
struct stat loginfo;
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
+ struct string_list skiplist = STRING_LIST_INIT_NODUP;
if (log S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
@@ -2659,11 +2665,18 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref
[PATCH 10/24] refs.c: ref_transaction_commit: distinguish name conflicts from other errors
From: Ronnie Sahlberg sahlb...@google.com
Date: Fri, 16 May 2014 14:14:38 -0700
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either
when we lstat the new refname or if the name checking function reports that
the same type of conflict happened. In both cases, it means that we can not
create the new ref due to a name conflict.
Start defining specific return codes for _commit. TRANSACTION_NAME_CONFLICT
refers to a failure to create a ref due to a name conflict with another ref.
TRANSACTION_GENERIC_ERROR is for all other errors.
When git fetch is creating refs, name conflicts differ from other errors in
that they are likely to be resolved by running git remote prune remote.
git fetch currently inspects errno to decide whether to give that advice.
Once it switches to the transaction API, it can check for
TRANSACTION_NAME_CONFLICT instead.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Since v21:
- clarified commit message and updated to match code
- small code cleanups
- clarified API doc, introduced TRANSACTION_GENERIC_ERROR so both error
codes have names
refs.c | 26 --
refs.h | 9 +++--
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/refs.c b/refs.c
index 6820c93..623a1ae 100644
--- a/refs.c
+++ b/refs.c
@@ -3583,9 +3583,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err);
- if (ret)
+ if (ref_update_reject_duplicates(updates, n, err)) {
+ ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
+ }
/* Acquire all locks while verifying old values */
for (i = 0; i n; i++) {
@@ -3599,10 +3600,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type);
if (!update-lock) {
+ ret = (errno == ENOTDIR)
+ ? TRANSACTION_NAME_CONFLICT
+ : TRANSACTION_GENERIC_ERROR;
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
update-refname);
- ret = 1;
goto cleanup;
}
}
@@ -3612,15 +3615,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update-new_sha1)) {
- ret = write_ref_sha1(update-lock, update-new_sha1,
-update-msg);
- update-lock = NULL; /* freed by write_ref_sha1 */
- if (ret) {
+ if (write_ref_sha1(update-lock, update-new_sha1,
+ update-msg)) {
+ update-lock = NULL; /* freed by write_ref_sha1
*/
if (err)
strbuf_addf(err, Cannot update the ref
'%s'.,
update-refname);
+ ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
+ update-lock = NULL; /* freed by write_ref_sha1 */
}
}
@@ -3629,14 +3633,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type,
- err);
+ if (delete_ref_loose(update-lock, update-type, err))
+ ret = TRANSACTION_GENERIC_ERROR;
+
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
}
- ret |= repack_without_refs(delnames, delnum, err);
+ if (repack_without_refs(delnames, delnum, err))
+ ret = TRANSACTION_GENERIC_ERROR;
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
diff --git a/refs.h b/refs.h
index aded545..fd63b47 100644
--- a/refs.h
+++ b/refs.h
@@ -323,9 +323,14 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
/*
* Commit all of the changes that have been queued in transaction, as
- * atomically as possible. Return a nonzero value if there is a
- * problem.
+ * atomically as possible.
+ *
+ * Returns 0 for success, or one
[PATCH 11/24] fetch.c: change s_update_ref to use a ref transaction
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 13:49:07 -0700
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Since v21:
- tweaked handling of ref_transaction_commit result (no functional
change)
builtin/fetch.c | 34 --
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..30b40f6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,23 +375,37 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv(GIT_REFLOG_ACTION);
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+ int ret, df_conflict = 0;
if (dry_run)
return 0;
if (!rla)
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
- lock = lock_any_ref_for_update(ref-name,
- check_old ? ref-old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref-new_sha1, msg) 0)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ ref-old_sha1, 0, check_old, msg, err))
+ goto fail;
+
+ ret = ref_transaction_commit(transaction, err);
+ if (ret) {
+ df_conflict = (ret == TRANSACTION_NAME_CONFLICT);
+ goto fail;
+ }
+
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return 0;
+fail:
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
+ : STORE_REF_ERROR_OTHER;
}
#define REFCOL_WIDTH 10
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/24] refs.c: make write_ref_sha1 static
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 15:36:58 -0700
No external users call write_ref_sha1 any more so let's declare it static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Since v21:
- punctuation fix in commit message
- grammar tweak in doc comment
refs.c | 10 --
refs.h | 3 ---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 623a1ae..f596a9f 100644
--- a/refs.c
+++ b/refs.c
@@ -2645,6 +2645,9 @@ static int rename_tmp_log(const char *newrefname)
return 0;
}
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+ const char *logmsg);
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -2900,8 +2903,11 @@ static int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
-/* This function must return a meaningful errno */
-int write_ref_sha1(struct ref_lock *lock,
+/*
+ * Write sha1 into the ref specified by the lock. Make sure that errno
+ * is sane on error.
+ */
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index fd63b47..3bb16db 100644
--- a/refs.h
+++ b/refs.h
@@ -195,9 +195,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/*
* Setup reflog before using. Set errno to something meaningful on failure.
*/
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/24] reflog test: test interaction with detached HEAD
From: Junio C Hamano gits...@pobox.com
A proposed patch produced broken HEAD reflog entries when checking out
anything other than a branch. The testsuite still passed, so it took
a few days for the bug to be noticed.
Add tests checking the content of the reflog after detaching and
reattaching HEAD so we don't have to rely on manual testing to catch
such problems in the future.
[jn: using 'log -g --format=%H' instead of parsing --oneline output,
resetting state in each test so they can be safely reordered or
skipped]
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
New since v21. Thanks to Junio for noticing the bug.
t/t1413-reflog-detach.sh | 70
1 file changed, 70 insertions(+)
create mode 100755 t/t1413-reflog-detach.sh
diff --git a/t/t1413-reflog-detach.sh b/t/t1413-reflog-detach.sh
new file mode 100755
index 000..c730600
--- /dev/null
+++ b/t/t1413-reflog-detach.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='Test reflog interaction with detached HEAD'
+. ./test-lib.sh
+
+reset_state () {
+ git checkout master
+ cp saved_reflog .git/logs/HEAD
+}
+
+test_expect_success setup '
+ test_tick
+ git commit --allow-empty -m initial
+ git branch side
+ test_tick
+ git commit --allow-empty -m second
+ cat .git/logs/HEAD saved_reflog
+'
+
+test_expect_success baseline '
+ reset_state
+ git rev-parse master master^ expect
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'switch to branch' '
+ reset_state
+ git rev-parse side master master^ expect
+ git checkout side
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'detach to other' '
+ reset_state
+ git rev-parse master side master master^ expect
+ git checkout side
+ git checkout master^0
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'detach to self' '
+ reset_state
+ git rev-parse master master master^ expect
+ git checkout master^0
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'attach to self' '
+ reset_state
+ git rev-parse master master master master^ expect
+ git checkout master^0
+ git checkout master
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'attach to other' '
+ reset_state
+ git rev-parse side master master master^ expect
+ git checkout master^0
+ git checkout side
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_done
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/24] refs.c: change resolve_ref_unsafe reading argument to be a flags field
From: Ronnie Sahlberg sahlb...@google.com
Date: Tue, 15 Jul 2014 12:59:36 -0700
resolve_ref_unsafe takes a boolean argument for reading (a nonexistent ref
resolves successfully for writing but not for reading). Change this to be
a flags field instead, and pass the new constant RESOLVE_REF_READING when
we want this behaviour.
While at it, swap two of the arguments in the function to put output
arguments at the end. As a nice side effect, this ensures that we can
catch callers that were unaware of the new API so they can be audited.
Give the wrapper functions resolve_refdup and read_ref_full the same
treatment for consistency.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Since v21:
- clarified commit message
- put output parameters last
branch.c| 2 +-
builtin/blame.c | 2 +-
builtin/branch.c| 9 ++---
builtin/checkout.c | 6 ++--
builtin/clone.c | 2 +-
builtin/commit.c| 2 +-
builtin/fmt-merge-msg.c | 2 +-
builtin/for-each-ref.c | 6 ++--
builtin/fsck.c | 2 +-
builtin/log.c | 3 +-
builtin/merge.c | 2 +-
builtin/notes.c | 2 +-
builtin/receive-pack.c | 4 +--
builtin/remote.c| 5 +--
builtin/show-branch.c | 7 ++--
builtin/symbolic-ref.c | 2 +-
bundle.c| 2 +-
cache.h | 23 ++--
http-backend.c | 4 ++-
notes-merge.c | 2 +-
reflog-walk.c | 5 +--
refs.c | 93 -
remote.c| 11 +++---
sequencer.c | 4 +--
transport-helper.c | 5 ++-
transport.c | 5 +--
upload-pack.c | 2 +-
wt-status.c | 2 +-
28 files changed, 124 insertions(+), 92 deletions(-)
diff --git a/branch.c b/branch.c
index 76a8ec9..adb07c6 100644
--- a/branch.c
+++ b/branch.c
@@ -186,7 +186,7 @@ int validate_new_branchname(const char *name, struct strbuf
*ref,
const char *head;
unsigned char sha1[20];
- head = resolve_ref_unsafe(HEAD, sha1, 0, NULL);
+ head = resolve_ref_unsafe(HEAD, 0, sha1, NULL);
if (!is_bare_repository() head !strcmp(head, ref-buf))
die(_(Cannot force update the current branch.));
}
diff --git a/builtin/blame.c b/builtin/blame.c
index a52a279..5cbd38f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2292,7 +2292,7 @@ static struct commit *fake_working_tree_commit(struct
diff_options *opt,
commit-object.type = OBJ_COMMIT;
parent_tail = commit-parents;
- if (!resolve_ref_unsafe(HEAD, head_sha1, 1, NULL))
+ if (!resolve_ref_unsafe(HEAD, RESOLVE_REF_READING, head_sha1, NULL))
die(no such ref: HEAD);
parent_tail = append_parent(parent_tail, head_sha1);
diff --git a/builtin/branch.c b/builtin/branch.c
index 652b1d2..e5d1377 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -129,7 +129,8 @@ static int branch_merged(int kind, const char *name,
branch-merge[0]
branch-merge[0]-dst
(reference_name = reference_name_to_free =
-resolve_refdup(branch-merge[0]-dst, sha1, 1, NULL)) !=
NULL)
+resolve_refdup(branch-merge[0]-dst, RESOLVE_REF_READING,
+ sha1, NULL)) != NULL)
reference_rev = lookup_commit_reference(sha1);
}
if (!reference_rev)
@@ -233,7 +234,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, sha1, 0, flags);
+ target = resolve_ref_unsafe(name, 0, sha1, flags);
if (!target ||
(!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
error(remote_branch
@@ -296,7 +297,7 @@ static char *resolve_symref(const char *src, const char
*prefix)
int flag;
const char *dst, *cp;
- dst = resolve_ref_unsafe(src, sha1, 0, flag);
+ dst = resolve_ref_unsafe(src, 0, sha1, flag);
if (!(dst (flag REF_ISSYMREF)))
return NULL;
if (prefix (cp = skip_prefix(dst, prefix)))
@@ -862,7 +863,7 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
track = git_branch_track;
- head = resolve_refdup(HEAD, head_sha1, 0, NULL);
+ head = resolve_refdup(HEAD, 0, head_sha1, NULL);
if (!head)
die(_(Failed to resolve HEAD as a valid ref.));
if (!strcmp(head, HEAD)) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..a5fef2d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -356,7 +356,7 @@ static int checkout_paths(const struct checkout_opts
[PATCH 15/24] branch -d: avoid repeated symref resolution
From: Jonathan Nieder jrnie...@gmail.com
Date: Wed, 10 Sep 2014 18:22:48 -0700
If a repository gets in a broken state with too much symref nesting,
it cannot be repaired with git branch -d:
$ git symbolic-ref refs/heads/nonsense refs/heads/nonsense
$ git branch -d nonsense
error: branch 'nonsense' not found.
Worse, git update-ref --no-deref -d doesn't work for such repairs
either:
$ git update-ref -d refs/heads/nonsense
error: unable to resolve reference refs/heads/nonsense: Too many levels of
symbolic links
Fix both by teaching resolve_ref_unsafe a new RESOLVE_REF_NO_RECURSE
flag and passing it when appropriate.
Callers can still read the value of a symref (for example to print a
message about it) with that flag set --- resolve_ref_unsafe will
resolve one level of symrefs and stop there.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
Since v21:
- renamed flag from RESOLVE_REF_NO_DEREF to _NO_RECURSE
- more detail in API docs
- only set NO_RECURSE when deleting refs. Locking refs for non-deletion
updates needs to recurse to get old_sha1 for the reflog.
- add more tests (for symrefs to refs with bad names, which should also
be deletable now)
builtin/branch.c | 3 ++-
cache.h | 6 ++
refs.c| 17 +++--
refs.h| 2 ++
t/t1400-update-ref.sh | 34 ++
t/t3200-branch.sh | 9 +
6 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index e5d1377..a334380 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -234,7 +234,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, 0, sha1, flags);
+ target = resolve_ref_unsafe(name, RESOLVE_REF_NO_RECURSE,
+ sha1, flags);
if (!target ||
(!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
error(remote_branch
diff --git a/cache.h b/cache.h
index 5b54911..5ca7f2b 100644
--- a/cache.h
+++ b/cache.h
@@ -970,6 +970,11 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* writing to the ref, the return value is the name of the ref
* that will actually be created or changed.
*
+ * If the RESOLVE_REF_NO_RECURSE flag is passed, only resolves one
+ * level of symbolic reference. The value stored in sha1 for a symbolic
+ * reference will always be null_sha1 in this case, and the return
+ * value is the reference that the symref refers to directly.
+ *
* If flags is non-NULL, set the value that it points to the
* combination of REF_ISPACKED (if the reference was found among the
* packed references), REF_ISSYMREF (if the initial reference was a
@@ -982,6 +987,7 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* errno is set to something meaningful on error.
*/
#define RESOLVE_REF_READING 0x01
+#define RESOLVE_REF_NO_RECURSE 0x02
extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags,
unsigned char *sha1, int *flags);
extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char
*sha1, int *flags);
diff --git a/refs.c b/refs.c
index 4916d16..490e788 100644
--- a/refs.c
+++ b/refs.c
@@ -1407,6 +1407,10 @@ const char *resolve_ref_unsafe(const char *refname, int
resolve_flags, unsigned
refname = refname_buffer;
if (flags)
*flags |= REF_ISSYMREF;
+ if (resolve_flags RESOLVE_REF_NO_RECURSE) {
+ hashclr(sha1);
+ return refname;
+ }
continue;
}
}
@@ -1463,13 +1467,17 @@ const char *resolve_ref_unsafe(const char *refname, int
resolve_flags, unsigned
buf = buffer + 4;
while (isspace(*buf))
buf++;
+ refname = strcpy(refname_buffer, buf);
+ if (resolve_flags RESOLVE_REF_NO_RECURSE) {
+ hashclr(sha1);
+ return refname;
+ }
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
if (flags)
*flags |= REF_ISBROKEN;
errno = EINVAL;
return NULL;
}
- refname = strcpy(refname_buffer, buf);
}
}
@@ -2111,6 +2119,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (mustexist)
resolve_flags |= RESOLVE_REF_READING;
+ if (flags REF_NODEREF flags
[PATCH 16/24] branch -d: simplify by using RESOLVE_REF_READING flag
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 11 Sep 2014 10:34:36 -0700
When git branch -d reads the branch it is about to delete, it used
to avoid passing the RESOLVE_REF_READING ('treat missing ref as
error') flag because a symref pointing to a nonexistent ref would show
up as missing instead of as something that could be deleted. To check
if a ref is actually missing, we then check
- is it a symref?
- if not, did it resolve to null_sha1?
Now we pass RESOLVE_REF_NO_RECURSE and the correct information is
returned for a symref even when it points to a missing ref. Simplify
by relying on RESOLVE_REF_READING.
No functional change intended.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Split out from the 'fix handling of badly named refs' patch.
builtin/branch.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index a334380..a0c5601 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -234,10 +234,11 @@ static int delete_branches(int argc, const char **argv,
int force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, RESOLVE_REF_NO_RECURSE,
+ target = resolve_ref_unsafe(name,
+ RESOLVE_REF_READING
+ | RESOLVE_REF_NO_RECURSE,
sha1, flags);
- if (!target ||
- (!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
+ if (!target) {
error(remote_branch
? _(remote branch '%s' not found.)
: _(branch '%s' not found.), bname.buf);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/24] packed-ref cache: forbid dot-components in refnames
Since v1.7.9-rc1~10^2 (write_head_info(): handle extra refs locally,
2012-01-06), this trick to keep track of .have refs that are only
valid on the wire and not on the filesystem is not needed any more.
Simplify by removing support for the REFNAME_DOT_COMPONENT flag.
This means we'll be slightly stricter with invalid refs found in a
packed-refs file or during clone. read_loose_refs() already checks
for and skips refnames with .components so it is not affected.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
Noticed while reviewing other patches.
refs.c | 14 +++---
refs.h | 6 +-
2 files changed, 4 insertions(+), 16 deletions(-)
diff --git a/refs.c b/refs.c
index 490e788..94d6d89 100644
--- a/refs.c
+++ b/refs.c
@@ -69,16 +69,8 @@ static int check_refname_component(const char *refname, int
flags)
out:
if (cp == refname)
return 0; /* Component has zero length. */
- if (refname[0] == '.') {
- if (!(flags REFNAME_DOT_COMPONENT))
- return -1; /* Component starts with '.'. */
- /*
-* Even if leading dots are allowed, don't allow .
-* as a component (.. is prevented by a rule above).
-*/
- if (refname[1] == '\0')
- return -1; /* Component equals .. */
- }
+ if (refname[0] == '.')
+ return -1; /* Component starts with '.'. */
if (cp - refname = 5 !memcmp(cp - 5, .lock, 5))
return -1; /* Refname ends with .lock. */
return cp - refname;
@@ -288,7 +280,7 @@ static struct ref_entry *create_ref_entry(const char
*refname,
struct ref_entry *ref;
if (check_name
- check_refname_format(refname,
REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
+ check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
die(Reference has invalid format: '%s', refname);
len = strlen(refname) + 1;
ref = xmalloc(sizeof(struct ref_entry) + len);
diff --git a/refs.h b/refs.h
index b62c4c4..e48d8b9 100644
--- a/refs.h
+++ b/refs.h
@@ -226,7 +226,6 @@ extern int for_each_reflog(each_ref_fn, void *);
#define REFNAME_ALLOW_ONELEVEL 1
#define REFNAME_REFSPEC_PATTERN 2
-#define REFNAME_DOT_COMPONENT 4
/*
* Return 0 iff refname has the correct format for a refname according
@@ -234,10 +233,7 @@ extern int for_each_reflog(each_ref_fn, void *);
* If REFNAME_ALLOW_ONELEVEL is set in flags, then accept one-level
* reference names. If REFNAME_REFSPEC_PATTERN is set in flags, then
* allow a * wildcard character in place of one of the name
- * components. No leading or repeated slashes are accepted. If
- * REFNAME_DOT_COMPONENT is set in flags, then allow refname
- * components to start with . (but not a whole component equal to
- * . or ..).
+ * components. No leading or repeated slashes are accepted.
*/
extern int check_refname_format(const char *refname, int flags);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 18/24] test: put tests for handling of bad ref names in one place
From: Ronnie Sahlberg sahlb...@google.com There's no straightforward way to grep for all tests dealing with invalid refs. Put them in a single test script so it is easy to see what functionality has not been exercised with bad ref names yet. Signed-off-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- New. t/t1400-update-ref.sh | 44 -- t/t1430-bad-ref-name.sh | 84 + t/t9300-fast-import.sh | 30 -- 3 files changed, 84 insertions(+), 74 deletions(-) create mode 100755 t/t1430-bad-ref-name.sh diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 7c8c41a..7b4707b 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -136,14 +136,6 @@ test_expect_success 'update-ref --no-deref -d can delete reference to bad ref' ' test_path_is_missing .git/refs/heads/ref-to-bad ' -test_expect_success 'update-ref --no-deref -d can delete reference to broken name' ' - git symbolic-ref refs/heads/badname refs/heads/broken...ref - test_when_finished rm -f .git/refs/heads/badname - test_path_is_file .git/refs/heads/badname - git update-ref --no-deref -d refs/heads/badname - test_path_is_missing .git/refs/heads/badname -' - test_expect_success '(not) create HEAD with old sha1' test_must_fail git update-ref HEAD $A $B @@ -408,12 +400,6 @@ test_expect_success 'stdin fails create with no ref' ' grep fatal: create: missing ref err ' -test_expect_success 'stdin fails create with bad ref name' ' - echo create ~a $m stdin - test_must_fail git update-ref --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin fails create with no new value' ' echo create $a stdin test_must_fail git update-ref --stdin stdin 2err @@ -432,12 +418,6 @@ test_expect_success 'stdin fails update with no ref' ' grep fatal: update: missing ref err ' -test_expect_success 'stdin fails update with bad ref name' ' - echo update ~a $m stdin - test_must_fail git update-ref --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin fails update with no new value' ' echo update $a stdin test_must_fail git update-ref --stdin stdin 2err @@ -456,12 +436,6 @@ test_expect_success 'stdin fails delete with no ref' ' grep fatal: delete: missing ref err ' -test_expect_success 'stdin fails delete with bad ref name' ' - echo delete ~a $m stdin - test_must_fail git update-ref --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin fails delete with too many arguments' ' echo delete $a $m $m stdin test_must_fail git update-ref --stdin stdin 2err @@ -734,12 +708,6 @@ test_expect_success 'stdin -z fails create with no ref' ' grep fatal: create: missing ref err ' -test_expect_success 'stdin -z fails create with bad ref name' ' - printf $F create ~a $m stdin - test_must_fail git update-ref -z --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin -z fails create with no new value' ' printf $F create $a stdin test_must_fail git update-ref -z --stdin stdin 2err @@ -764,12 +732,6 @@ test_expect_success 'stdin -z fails update with too few args' ' grep fatal: update $a: unexpected end of input when reading oldvalue err ' -test_expect_success 'stdin -z fails update with bad ref name' ' - printf $F update ~a $m stdin - test_must_fail git update-ref -z --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin -z emits warning with empty new value' ' git update-ref $a $m printf $F update $a stdin @@ -802,12 +764,6 @@ test_expect_success 'stdin -z fails delete with no ref' ' grep fatal: delete: missing ref err ' -test_expect_success 'stdin -z fails delete with bad ref name' ' - printf $F delete ~a $m stdin - test_must_fail git update-ref -z --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin -z fails delete with no old value' ' printf $F delete $a stdin test_must_fail git update-ref -z --stdin stdin 2err diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh new file mode 100755 index 000..c7b19b0 --- /dev/null +++ b/t/t1430-bad-ref-name.sh @@ -0,0 +1,84 @@ +#!/bin/sh + +test_description='Test handling of ref names that check-ref-format rejects' +. ./test-lib.sh + +test_expect_success setup ' + test_commit one +' + +test_expect_success 'fast-import: fail on invalid branch name .badbranchname' ' + test_when_finished rm -f .git/objects/pack_* .git/objects/index_* + cat input -INPUT_END + commit .badbranchname
[PATCH 19/24] refs.c: allow listing and deleting badly named refs
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 3 Sep 2014 11:45:43 -0700
We currently do not handle badly named refs well:
$ cp .git/refs/heads/master .git/refs/heads/master.@\*@\\.
$ git branch
fatal: Reference has invalid format: 'refs/heads/master.@*@\.'
$ git branch -D master.@\*@\\.
error: branch 'master.@*@\.' not found.
Users cannot recover from a badly named ref without manually finding
and deleting the loose ref file or appropriate line in packed-refs.
Making that easier will make it easier to tweak the ref naming rules
in the future, for example to forbid shell metacharacters like '`'
and '', without putting people in a state that is hard to get out of.
So allow git branch --list to show these refs and to allow git
branch -d/-D and git update-ref -d to delete them. Other commands
(for example to rename refs) will continue to not handle these refs
but can be changed in later patches.
Details:
In resolving functions, refuse to resolve refs that don't pass the
check-ref-format(1) check unless the new RESOLVE_REF_ALLOW_BAD_NAME
flag is passed. Even with RESOLVE_REF_ALLOW_BAD_NAME, refuse to
resolve refs that escape the refs/ directory and do not match the
pattern [A-Z_]* (think HEAD and MERGE_HEAD).
In locking functions, refuse to act on badly named refs unless they
are being deleted and either are in the refs/ directory or match [A-Z_]*.
Just like other invalid refs, flag resolved, badly named refs with the
REF_ISBROKEN flag, treat them as resolving to null_sha1, and skip them
in all iteration functions except for for_each_rawref.
Flag badly named refs with a REF_BAD_NAME flag to make it easier for
future callers to notice and handle them specially.
In the transaction API, refuse to create or update badly named refs,
but allow deleting them (unless they escape refs/ and don't match
[A-Z_]*).
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Since v21:
- clarified change description
- handle REF_ISBROKEN case too when printing Deleted branch message
- tighten fallback check, so commands like 'git branch -d ../../config'
won't work
- use REF_BAD_NAME so it's easier for callers (like for-each-ref in a
later patch) to notice that we decided a ref's name was bad
- set errno when failing to find ref in the packed-refs file
- consistently don't set REF_ISBROKEN for missing refs
- consistently clear sha1 when successfully resolving a badly named ref
- treat refs with bad names encountered as part of symref resolution the
same as such refs explicitly named
- always allow safe bad names when trying to lock a ref for deletion
(though some callers don't get that far yet)
- clearer API docs, more tests
builtin/branch.c| 9 +--
cache.h | 14 -
refs.c | 148 ++--
refs.h | 8 ++-
t/t1430-bad-ref-name.sh | 125 +++-
5 files changed, 265 insertions(+), 39 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index a0c5601..94aaea1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -236,7 +236,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
name = mkpathdup(fmt, bname.buf);
target = resolve_ref_unsafe(name,
RESOLVE_REF_READING
- | RESOLVE_REF_NO_RECURSE,
+ | RESOLVE_REF_NO_RECURSE
+ | RESOLVE_REF_ALLOW_BAD_NAME,
sha1, flags);
if (!target) {
error(remote_branch
@@ -246,7 +247,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
continue;
}
- if (!(flags REF_ISSYMREF)
+ if (!(flags (REF_ISSYMREF|REF_ISBROKEN))
check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
force)) {
ret = 1;
@@ -266,8 +267,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
? _(Deleted remote branch %s (was %s).\n)
: _(Deleted branch %s (was %s).\n),
bname.buf,
- (flags REF_ISSYMREF)
- ? target
+ (flags REF_ISBROKEN) ? broken
+ : (flags REF_ISSYMREF) ? target
: find_unique_abbrev(sha1, DEFAULT_ABBREV));
}
delete_branch_config(bname.buf);
diff --git a/cache.h b/cache.h
index 5ca7f2b..0c0ac60 100644
--- a/cache.h
+++ b/cache.h
@@ -978,16 +978,26 @@ extern int read_ref
[PATCH 20/24] for-each-ref.c: improve message before aborting on broken ref
From: Ronnie Sahlberg sahlb...@google.com
Date: Fri, 5 Sep 2014 14:35:17 -0700
Print a warning message for any badly named refs we find in the repo.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
As before.
builtin/for-each-ref.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 20949b7..a88d681 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -853,6 +853,11 @@ static int grab_single_ref(const char *refname, const
unsigned char *sha1, int f
struct refinfo *ref;
int cnt;
+ if (flag REF_BAD_NAME) {
+ warning(ignoring ref with broken name %s, refname);
+ return 0;
+ }
+
if (*cb-grab_pattern) {
const char **pattern;
int namelen = strlen(refname);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 21/24] remote rm/prune: print a message when writing packed-refs fails
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 11 Sep 2014 08:42:57 -0700
Until v2.1.0-rc0~22^2~11 (refs.c: add an err argument to
repack_without_refs, 2014-06-20), repack_without_refs forgot to
provide an error message when commit_packed_refs fails. Even today,
it only provides a message for callers that pass a non-NULL err
parameter. Internal callers in refs.c pass non-NULL err but
git remote does not.
That means that git remote rm and git remote prune can fail
without printing a message about why. Fix them by passing in a
non-NULL err parameter and printing the returned message.
This is the last caller to a ref handling function passing err ==
NULL. A later patch can drop support for err == NULL, avoiding such
problems in the future.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
---
Since v21:
- s/without/about/ in the commit message
builtin/remote.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index c7f82f4..8517cfa 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,13 +750,16 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
+ struct strbuf err = STRBUF_INIT;
const char **branch_names;
int i, result = 0;
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
branch_names[i] = branches-items[i].string;
- result |= repack_without_refs(branch_names, branches-nr, NULL);
+ if (repack_without_refs(branch_names, branches-nr, err))
+ result |= error(%s, err.buf);
+ strbuf_release(err);
free(branch_names);
for (i = 0; i branches-nr; i++) {
@@ -1333,9 +1336,13 @@ static int prune_remote(const char *remote, int dry_run)
delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
for (i = 0; i states.stale.nr; i++)
delete_refs[i] = states.stale.items[i].util;
- if (!dry_run)
- result |= repack_without_refs(delete_refs,
- states.stale.nr, NULL);
+ if (!dry_run) {
+ struct strbuf err = STRBUF_INIT;
+ if (repack_without_refs(delete_refs, states.stale.nr,
+ err))
+ result |= error(%s, err.buf);
+ strbuf_release(err);
+ }
free(delete_refs);
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 22/24] refs.c: do not permit err == NULL
Date: Thu, 28 Aug 2014 16:42:37 -0700
Some functions that take a strbuf argument to append an error treat
!err as an indication that the message should be suppressed (e.g.,
ref_update_reject_duplicates). Others write the message to stderr on
!err (e.g., repack_without_refs). Others crash (e.g.,
ref_transaction_update).
Some of these behaviors are for historical reasons and others were
accidents. Luckily no callers pass err == NULL any more. Simplify
by consistently requiring the strbuf argument.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Since v21:
- dropped spurious 'to' from commit message
refs.c | 46 +++---
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/refs.c b/refs.c
index 0d3abb1..e13f843 100644
--- a/refs.c
+++ b/refs.c
@@ -2596,6 +2596,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
struct string_list_item *ref_to_delete;
int i, ret, removed = 0;
+ assert(err);
+
/* Look for a packed ref */
for (i = 0; i n; i++)
if (get_packed_ref(refnames[i]))
@@ -2606,13 +2608,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
- if (err) {
- unable_to_lock_message(git_path(packed-refs), errno,
- err);
- return -1;
- }
- unable_to_lock_error(git_path(packed-refs), errno);
- return error(cannot delete '%s' from packed refs,
refnames[i]);
+ unable_to_lock_message(git_path(packed-refs), errno, err);
+ return -1;
}
packed = get_packed_refs(ref_cache);
@@ -2638,7 +2635,7 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
/* Write what remains */
ret = commit_packed_refs();
- if (ret err)
+ if (ret)
strbuf_addf(err, unable to overwrite old ref-pack file: %s,
strerror(errno));
return ret;
@@ -2646,6 +2643,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
+ assert(err);
+
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/* loose */
int res, i = strlen(lock-lk-filename) - 5; /* .lock */
@@ -3495,6 +3494,8 @@ struct ref_transaction {
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
{
+ assert(err);
+
return xcalloc(1, sizeof(struct ref_transaction));
}
@@ -3534,6 +3535,8 @@ int ref_transaction_update(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: update called for transaction that is not open);
@@ -3566,6 +3569,8 @@ int ref_transaction_create(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: create called for transaction that is not open);
@@ -3597,6 +3602,8 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: delete called for transaction that is not open);
@@ -3659,13 +3666,14 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
struct strbuf *err)
{
int i;
+
+ assert(err);
+
for (i = 1; i n; i++)
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
- const char *str =
- Multiple updates for ref '%s' not allowed.;
- if (err)
- strbuf_addf(err, str, updates[i]-refname);
-
+ strbuf_addf(err,
+ Multiple updates for ref '%s' not
allowed.,
+ updates[i]-refname);
return 1;
}
return 0;
@@ -3679,6 +3687,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: commit called for transaction that is not open);
@@ -3715,9 +3725,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = (errno == ENOTDIR)
? TRANSACTION_NAME_CONFLICT
: TRANSACTION_GENERIC_ERROR
[PATCH 23/24] lockfile: remove unable_to_lock_error
Date: Thu, 28 Aug 2014 16:41:34 -0700
The former caller uses unable_to_lock_message now.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
As before.
cache.h| 1 -
lockfile.c | 10 --
2 files changed, 11 deletions(-)
diff --git a/cache.h b/cache.h
index 0c0ac60..f582b6c 100644
--- a/cache.h
+++ b/cache.h
@@ -558,7 +558,6 @@ 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 void unable_to_lock_message(const char *path, int err,
struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
diff --git a/lockfile.c b/lockfile.c
index a921d77..dbd4101 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -176,16 +176,6 @@ void unable_to_lock_message(const char *path, int err,
struct strbuf *buf)
absolute_path(path), strerror(err));
}
-int unable_to_lock_error(const char *path, int err)
-{
- struct strbuf buf = STRBUF_INIT;
-
- unable_to_lock_message(path, err, buf);
- error(%s, buf.buf);
- strbuf_release(buf);
- return -1;
-}
-
NORETURN void unable_to_lock_index_die(const char *path, int err)
{
struct strbuf buf = STRBUF_INIT;
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 24/24] ref_transaction_commit: bail out on failure to remove a ref
Date: Thu, 28 Aug 2014 17:01:35 -0700
When removal of a loose or packed ref fails, bail out instead of
trying to finish the transaction. This way, a single error message
can be printed (instead of multiple messages being concatenated by
mistake) and the operator can try to solve the underlying problem
before there is a chance to muck things up even more.
In particular, when git fails to remove a ref, git goes on to try to
delete the reflog. Exiting early lets us keep the reflog.
When git succeeds in deleting a ref A and fails to remove a ref B, it
goes on to try to delete both reflogs. It would be better to just
remove the reflog for A, but that would be a more invasive change.
Failing early means we keep both reflogs, which puts the operator in a
good position to understand the problem and recover.
A long term goal is to avoid these problems altogether and roll back
the transaction on failure. That kind of transactionality will have
to wait for a later series (the plan for which is to make all
destructive work happen in a single update of the packed-refs file).
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
That's the end of the series. Thanks for reading.
refs.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index e13f843..d9d327d 100644
--- a/refs.c
+++ b/refs.c
@@ -3753,16 +3753,20 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- if (delete_ref_loose(update-lock, update-type, err))
+ if (delete_ref_loose(update-lock, update-type, err)) {
ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
}
- if (repack_without_refs(delnames, delnum, err))
+ if (repack_without_refs(delnames, delnum, err)) {
ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Can I fetch an arbitrary commit by sha1?
Jeff King wrote: But I think Christian is arguing that the client side should complain that $sha1 is not a remote ref, and therefore not something we can fetch. This used to be the behavior until 6e7b66e (fetch: fetch objects by their exact SHA-1 object names, 2013-01-29). The idea there is that some refs may be kept hidden from the ref advertisement, but clients who learn about the sha1 out-of-band may fetch the tips of hidden refs. I'm not sure it is a feature that has been particularly well-used to date, though. I use it pretty often. The commits I'm fetching are pointed to directly by refs, but I don't care about what the ref is called and I want exactly that commit. The context is that the commit is mentioned in the gerrit web UI. Fetching by commit name feels simpler than getting the refs/changes/something ref, since I think in terms of commits instead of in terms of change numbers. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/24] refs.c: pass a list of names to skip to is_refname_available
Junio C Hamano wrote:
Jonathan Nieder jrnie...@gmail.com writes:
diff --git a/refs.c b/refs.c
index f124c2b..6820c93 100644
--- a/refs.c
+++ b/refs.c
@@ -801,14 +801,16 @@ static int names_conflict(const char *refname1, const
char *refname2)
struct name_conflict_cb {
const char *refname;
-const char *oldrefname;
const char *conflicting_refname;
+struct string_list *skiplist;
};
As cbe73331 (refs: speed up is_refname_available, 2014-09-10)
touches the same area and is now in 'master', the logic around here
in this series needs to be reworked.
Thanks for the heads up. (I hadn't realized the ref-transaction-1 was
part of 'master' --- yay!) Rebased and reworked:
https://code-review.googlesource.com/1027
I'll give a week or so for review comments on this version of the
series and then post a hopefully final version.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v22.5 09/24] refs.c: pass a list of names to skip to is_refname_available
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 11:16:07 -0700
commit 23acf975d74825789112a3a7ba97dbbdc76904f4 upstream.
Change is_refname_available to take a list of strings to exclude when
checking for conflicts instead of just one single name. We can already
exclude a single name for the sake of renames. This generalizes that support.
ref_transaction_commit already tracks a set of refs that are being deleted
in an array. This array is then used to exclude refs from being written to
the packed-refs file. At some stage we will want to change this array to a
struct string_list and then we can pass it to is_refname_available via the
call to lock_ref_sha1_basic. That will allow us to perform transactions
that perform multiple renames as long as there are no conflicts within the
starting or ending state.
For example, that would allow a single transaction that contains two
renames that are both individually conflicting:
m - n/n
n - m/m
No functional change intended yet.
Change-Id: I8c68f0a47eef25759d572436ba683cb7b1ccb7eb
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Junio C Hamano wrote:
Jonathan Nieder jrnie...@gmail.com writes:
Thanks for the heads up. (I hadn't realized the ref-transaction-1 was
part of 'master' --- yay!) Rebased and reworked:
https://code-review.googlesource.com/1027
Heh, I was sort of expecting that heavy-weight contributors that
throw many and/or large patch series would at least be paying
attention to What's cooking reports.
That's fair. I don't pay enough attention to the 'graduated' section.
Will take a look over there (I really hate having to context switch
only to review this series, though).
Here's a copy to save a trip.
refs.c | 50 --
1 file changed, 32 insertions(+), 18 deletions(-)
diff --git a/refs.c b/refs.c
index 83b2c77..05e42a7 100644
--- a/refs.c
+++ b/refs.c
@@ -785,13 +785,13 @@ static void prime_ref_dir(struct ref_dir *dir)
}
}
-static int entry_matches(struct ref_entry *entry, const char *refname)
+static int entry_matches(struct ref_entry *entry, const struct string_list
*list)
{
- return refname !strcmp(entry-name, refname);
+ return list string_list_has_string(list, entry-name);
}
struct nonmatching_ref_data {
- const char *skip;
+ const struct string_list *skip;
struct ref_entry *found;
};
@@ -815,16 +815,19 @@ static void report_refname_conflict(struct ref_entry
*entry,
/*
* Return true iff a reference named refname could be created without
* conflicting with the name of an existing reference in dir. If
- * oldrefname is non-NULL, ignore potential conflicts with oldrefname
- * (e.g., because oldrefname is scheduled for deletion in the same
+ * skip is non-NULL, ignore potential conflicts with refs in skip
+ * (e.g., because they are scheduled for deletion in the same
* operation).
*
* Two reference names conflict if one of them exactly matches the
* leading components of the other; e.g., foo/bar conflicts with
* both foo and with foo/bar/baz but not with foo/bar or
* foo/barbados.
+ *
+ * skip must be sorted.
*/
-static int is_refname_available(const char *refname, const char *oldrefname,
+static int is_refname_available(const char *refname,
+ const struct string_list *skip,
struct ref_dir *dir)
{
const char *slash;
@@ -838,12 +841,12 @@ static int is_refname_available(const char *refname,
const char *oldrefname,
* looking for a conflict with a leaf entry.
*
* If we find one, we still must make sure it is
-* not oldrefname.
+* not in skip.
*/
pos = search_ref_dir(dir, refname, slash - refname);
if (pos = 0) {
struct ref_entry *entry = dir-entries[pos];
- if (entry_matches(entry, oldrefname))
+ if (entry_matches(entry, skip))
return 1;
report_refname_conflict(entry, refname);
return 0;
@@ -876,13 +879,13 @@ static int is_refname_available(const char *refname,
const char *oldrefname,
/*
* We found a directory named refname. It is a
* problem iff it contains any ref that is not
-* oldrefname.
+* in skip.
*/
struct ref_entry *entry = dir-entries[pos];
struct ref_dir *dir = get_ref_dir(entry);
struct nonmatching_ref_data data;
- data.skip = oldrefname;
+ data.skip = skip;
sort_ref_dir(dir);
if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn,
data))
return
Re: [PATCH] bundle: plug minor memory leak in is_tag_in_date_range()
René Scharfe wrote:
--- a/bundle.c
+++ b/bundle.c
@@ -211,24 +211,28 @@ static int is_tag_in_date_range(struct object *tag,
struct rev_info *revs)
enum object_type type;
char *buf, *line, *lineend;
If buf is initialized to NULL, there is no need for separate out and
out_free labels.
With or without such a change,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/bundle.c w/bundle.c
index 4158e11..101cde0 100644
--- i/bundle.c
+++ w/bundle.c
@@ -209,7 +209,7 @@ static int is_tag_in_date_range(struct object *tag, struct
rev_info *revs)
{
unsigned long size;
enum object_type type;
- char *buf, *line, *lineend;
+ char *buf = NULL, *line, *lineend;
unsigned long date;
int result = 1;
@@ -221,17 +221,16 @@ static int is_tag_in_date_range(struct object *tag,
struct rev_info *revs)
goto out;
line = memmem(buf, size, \ntagger , 8);
if (!line++)
- goto out_free;
+ goto out;
lineend = memchr(line, '\n', buf + size - line);
line = memchr(line, '', lineend ? lineend - line : buf + size - line);
if (!line++)
- goto out_free;
+ goto out;
date = strtoul(line, NULL, 10);
result = (revs-max_age == -1 || revs-max_age date)
(revs-min_age == -1 || revs-min_age date);
-out_free:
- free(buf);
out:
+ free(buf);
return result;
}
--
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] use skip_prefix() to avoid more magic numbers
René Scharfe wrote:
Continue where ae021d87 (use skip_prefix to avoid magic numbers) left off
and use skip_prefix() in more places for determining the lengths of prefix
strings to avoid using dependent constants and other indirect methods.
Sounds promising.
[...]
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -81,14 +81,16 @@ static int parse_branch_color_slot(const char *var, int
ofs)
static int git_branch_config(const char *var, const char *value, void *cb)
{
+ const char *slot_name;
+
if (starts_with(var, column.))
return git_column_config(var, value, branch, colopts);
if (!strcmp(var, color.branch)) {
branch_use_color = git_config_colorbool(var, value);
return 0;
}
- if (starts_with(var, color.branch.)) {
- int slot = parse_branch_color_slot(var, 13);
+ if (skip_prefix(var, color.branch., slot_name)) {
+ int slot = parse_branch_color_slot(var, slot_name - var);
I wonder why the separate var and offset parameters exist in the first
place. Wouldn't taking a single const char * so the old code could use
'var + 13' instead of 'var, 13' have worked?
At first glance I thought this should be
int slot = parse_branch_color_slot(slot_name, 0);
to be simpler. At second glance, how about something like the following
on top?
[...]
--- a/builtin/log.c
+++ b/builtin/log.c
[...]
@@ -388,8 +390,8 @@ static int git_log_config(const char *var, const char
*value, void *cb)
default_show_root = git_config_bool(var, value);
return 0;
}
- if (starts_with(var, color.decorate.))
- return parse_decorate_color_config(var, 15, value);
+ if (skip_prefix(var, color.decorate., slot_name))
+ return parse_decorate_color_config(var, slot_name - var, value);
Likewise: the offset-based API seems odd now here, too.
[...]
--- a/pretty.c
+++ b/pretty.c
[...]
@@ -809,18 +808,19 @@ static void parse_commit_header(struct
format_commit_context *context)
int i;
for (i = 0; msg[i]; i++) {
+ const char *name;
ident instead of name, maybe? (since it's a 'name email timestamp'
field)
[...]
@@ -1522,7 +1518,7 @@ static void pp_header(struct pretty_print_context *pp,
int parents_shown = 0;
for (;;) {
- const char *line = *msg_p;
+ const char *name, *line = *msg_p;
Likewise.
With or without the changes below,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/builtin/branch.c w/builtin/branch.c
index 6785097..022a88e 100644
--- i/builtin/branch.c
+++ w/builtin/branch.c
@@ -62,19 +62,19 @@ static unsigned char merge_filter_ref[20];
static struct string_list output = STRING_LIST_INIT_DUP;
static unsigned int colopts;
-static int parse_branch_color_slot(const char *var, int ofs)
+static int parse_branch_color_slot(const char *slot)
{
- if (!strcasecmp(var+ofs, plain))
+ if (!strcasecmp(slot, plain))
return BRANCH_COLOR_PLAIN;
- if (!strcasecmp(var+ofs, reset))
+ if (!strcasecmp(slot, reset))
return BRANCH_COLOR_RESET;
- if (!strcasecmp(var+ofs, remote))
+ if (!strcasecmp(slot, remote))
return BRANCH_COLOR_REMOTE;
- if (!strcasecmp(var+ofs, local))
+ if (!strcasecmp(slot, local))
return BRANCH_COLOR_LOCAL;
- if (!strcasecmp(var+ofs, current))
+ if (!strcasecmp(slot, current))
return BRANCH_COLOR_CURRENT;
- if (!strcasecmp(var+ofs, upstream))
+ if (!strcasecmp(slot, upstream))
return BRANCH_COLOR_UPSTREAM;
return -1;
}
@@ -90,7 +90,7 @@ static int git_branch_config(const char *var, const char
*value, void *cb)
return 0;
}
if (skip_prefix(var, color.branch., slot_name)) {
- int slot = parse_branch_color_slot(var, slot_name - var);
+ int slot = parse_branch_color_slot(slot_name);
if (slot 0)
return 0;
if (!value)
diff --git i/builtin/log.c w/builtin/log.c
index 1202eba..68d5d30 100644
--- i/builtin/log.c
+++ w/builtin/log.c
@@ -391,7 +391,7 @@ static int git_log_config(const char *var, const char
*value, void *cb)
return 0;
}
if (skip_prefix(var, color.decorate., slot_name))
- return parse_decorate_color_config(var, slot_name - var, value);
+ return parse_decorate_color_config(var, slot_name, value);
if (!strcmp(var, log.mailmap)) {
use_mailmap_config = git_config_bool(var, value);
return 0;
diff --git i/log-tree.c w/log-tree.c
index cff7ac1..6402c19 100644
--- i/log-tree.c
+++ w/log-tree.c
@@ -56,9 +56,9 @@ static int parse_decorate_color_slot(const char *slot)
return -1;
}
-int parse_decorate_color_config(const char *var, const int
Re: [PATCH 2/3] t5304: use helper to report failure of test foo = bar
Jeff King wrote:
For small outputs, we sometimes use:
test $(some_cmd) = something we expect
instead of a full test_cmp. The downside of this is that
when it fails, there is no output at all from the script.
There's another downside to that construct: it loses the exit
status from some_cmd.
[...]
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -13,7 +13,7 @@ add_blob() {
before=$(git count-objects | sed s/ .*//)
BLOB=$(echo aleph_0 | git hash-object -w --stdin)
BLOB_FILE=.git/objects/$(echo $BLOB | sed s/^../\//)
- test $((1 + $before)) = $(git count-objects | sed s/ .*//)
+ verbose test $((1 + $before)) = $(git count-objects | sed s/ .*//)
So ideally this would be something like:
git count-objects output
verbose test $((1 + $before)) = $(sed s/ .*// output)
[...]
@@ -45,11 +45,11 @@ test_expect_success 'prune --expire' '
add_blob
git prune --expire=1.hour.ago
- test $((1 + $before)) = $(git count-objects | sed s/ .*//)
+ verbose test $((1 + $before)) = $(git count-objects | sed s/ .*//)
and likewise elsewhere in the file.
Alternatively, maybe there could be a helper in the same spirit as
test_cmp_rev?
test_object_count () {
git count-objects output
sed s/ .*// output count
printf %s\n $1 expect
test_cmp expect count
}
My two cents,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] t5304: use helper to report failure of test foo = bar
Jeff King wrote:
On Mon, Oct 13, 2014 at 09:10:22AM -0700, Jonathan Nieder wrote:
There's another downside to that construct: it loses the exit
status from some_cmd.
Yes, although I think in many cases it's not a big deal. For example,
here we lose the exit code of count-objects, but it also is very
unlikely to fail _and_ produce our expected output.
It could segfault after producing the good output, but sure,
count-objects code doesn't change very often.
[...]
One of my goals was to provide a more generic helper so that we don't
have to make little helpers like this for every command. So I'd much
rather something like:
test_output () {
printf %s\n $1 expect
shift
$@ output
test_cmp expect output
}
I agree with the principle in general.
Unfortunately that wouldn't help here --- the $@ is a command with a
pipe to sed in it and we still lose the exit status from
count-objects.
Hoping that clarifies,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] t5304: use helper to report failure of test foo = bar
Junio C Hamano wrote: On Mon, Oct 13, 2014 at 2:31 PM, Jonathan Nieder jrnie...@gmail.com wrote: It could segfault after producing the good output, but sure, count-objects code doesn't change very often. Doesn't change very often is not the issue. Here we are not testing if it can count correctly without crashing, which *is* the real reason why it is perfectly fine to use $(git count-objects | sed ...) pattern here. There of course should be a test for count-objects to make sure it counts correctly without crashing. That also makes sense, but it is a pretty big change from the general strategy used in git tests today. Currently they use -chaining to check the status from git commands used along the way. That way, unrelated bugs in git commands that are only used incidentally get caught, as long as they cause the command to crash. This has helped me in the past to find weird bugs early when they cause some particular command to crash on some platforms. Sometimes they are races that show up on more popular platforms later. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pass config slots as pointers instead of offsets
Junio C Hamano wrote: builtin/branch.c | 16 builtin/commit.c | 19 +-- builtin/log.c| 2 +- log-tree.c | 4 ++-- log-tree.h | 2 +- 5 files changed, 21 insertions(+), 22 deletions(-) Signed-off-by: Jonathan Nieder jrnie...@gmail.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
[PATCH v23 0/25] rs/ref-transaction (Use ref transactions, part 3)
This series by Ronnie was last seen on-list at [1]. It includes some improvements to the ref-transaction API, improves handling of poorly named refs (which should make it easier to tighten the refname format checks in the future), and is a stepping stone toward later series that use the ref-transaction API more and make it support alternate backends (yay!). The changes since (a merge of 'master' and) v22 are very minor and can be seen below[2]. The more important change is that it's rebased against current 'master'. Review comments from Michael and Junio were very helpful in getting this in shape. Thanks much to both. The series can also be found at git://repo.or.cz/git/jrn.git tags/rs/ref-transaction It is based against current 'master' (670a3c1d, 2014-10-14) and intended for 'next'. Thoughts welcome, as always. Improvements preferred in the form of patches on top of the series. Jonathan Nieder (6): mv test: recreate mod/ directory instead of relying on stale copy branch -d: avoid repeated symref resolution packed-ref cache: forbid dot-components in refnames refs.c: do not permit err == NULL lockfile: remove unable_to_lock_error ref_transaction_commit: bail out on failure to remove a ref Junio C Hamano (1): reflog test: test interaction with detached HEAD Ronnie Sahlberg (18): wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as success refs.c: lock_ref_sha1_basic is used for all refs wrapper.c: add a new function unlink_or_msg refs.c: add an err argument to delete_ref_loose refs.c: pass the ref log message to _create/delete/update instead of _commit rename_ref: don't ask read_ref_full where the ref came from refs.c: refuse to lock badly named refs in lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: pass a list of names to skip to is_refname_available refs.c: ref_transaction_commit: distinguish name conflicts from other errors fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static refs.c: change resolve_ref_unsafe reading argument to be a flags field branch -d: simplify by using RESOLVE_REF_READING test: put tests for handling of bad ref names in one place refs.c: allow listing and deleting badly named refs for-each-ref: skip and warn about broken ref names remote rm/prune: print a message when writing packed-refs fails branch.c | 6 +- builtin/blame.c | 2 +- builtin/branch.c | 22 ++- builtin/checkout.c | 6 +- builtin/clone.c | 2 +- builtin/commit.c | 6 +- builtin/fetch.c | 34 ++-- builtin/fmt-merge-msg.c | 2 +- builtin/for-each-ref.c | 11 +- builtin/fsck.c | 2 +- builtin/log.c| 3 +- builtin/merge.c | 2 +- builtin/notes.c | 2 +- builtin/receive-pack.c | 9 +- builtin/remote.c | 20 ++- builtin/replace.c| 5 +- builtin/show-branch.c| 7 +- builtin/symbolic-ref.c | 2 +- builtin/tag.c| 4 +- builtin/update-ref.c | 13 +- bundle.c | 2 +- cache.h | 44 +++-- fast-import.c| 8 +- git-compat-util.h| 16 +- http-backend.c | 4 +- lockfile.c | 10 -- lockfile.h | 1 - notes-merge.c| 2 +- reflog-walk.c| 5 +- refs.c | 446 --- refs.h | 44 +++-- remote.c | 11 +- sequencer.c | 8 +- t/t1400-update-ref.sh| 62 +++ t/t1413-reflog-detach.sh | 70 t/t1430-bad-ref-name.sh | 207 ++ t/t3200-branch.sh| 9 + t/t7001-mv.sh| 15 +- t/t9300-fast-import.sh | 30 transport-helper.c | 5 +- transport.c | 5 +- upload-pack.c| 2 +- walker.c | 5 +- wrapper.c| 28 ++- wt-status.c | 2 +- 45 files changed, 850 insertions(+), 351 deletions(-) create mode 100755 t/t1413-reflog-detach.sh create mode 100755 t/t1430-bad-ref-name.sh [1] http://thread.gmane.org/gmane.comp.version-control.git/254501/focus=257771 [2] cache.h | 11 --- git-compat-util.h | 4 +-- refs.c| 96 +-- refs.h| 6 +++- 4 files changed, 64 insertions(+), 53 deletions(-) diff --git a/cache.h b/cache.h index 209e8ba..0501f7d 100644 --- a/cache.h +++ b/cache.h @@ -983,7 +983,8 @@ extern int read_ref(const char *refname, unsigned char *sha1); * packed references), REF_ISSYMREF (if the initial reference was a * symbolic reference), REF_BAD_NAME (if the reference name is ill * formed --- see RESOLVE_REF_ALLOW_BAD_NAME below), and REF_ISBROKEN - * (if the ref is malformed). + * (if the ref is malformed or has a bad name). See refs.h
[PATCH 01/25] mv test: recreate mod/ directory instead of relying on stale copy
From: Jonathan Nieder jrnie...@gmail.com Date: Wed, 10 Sep 2014 14:01:46 -0700 The tests for 'git mv moves a submodule' functionality often run commands like git mv sub mod/sub to move a submodule into a subdirectory. Just like plain /bin/mv, this is supposed to succeed if the mod/ parent directory exists and fail if it doesn't exist. Usually these tests mkdir the parent directory beforehand, but some instead rely on it being left behind by previous tests. More precisely, when 'git reset --hard' tries to move to a state where mod/sub is not present any more, it would perform the following operations: rmdir(mod/sub) rmdir(mod) The first fails with ENOENT because the test script removed mod/sub with rm -rf already, so 'reset --hard' doesn't bother to move on to the second, and the mod/ directory is kept around. Better to explicitly remove and re-create the mod/ directory so later tests don't have to depend on the directory left behind by the earlier ones at all (making it easier to rearrange or skip some tests in the file or to tweak 'reset --hard' behavior without breaking unrelated tests). Noticed while testing a patch that fixes the reset --hard behavior described above. Signed-off-by: Jonathan Nieder jrnie...@gmail.com Reviewed-by: Ronnie Sahlberg sahlb...@google.com --- t/t7001-mv.sh | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 54d7807..69f11bd 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -350,10 +350,11 @@ test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu ' test_expect_success 'git mv moves a submodule with gitfile' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod ( cd mod git mv ../sub/ . @@ -372,11 +373,12 @@ test_expect_success 'git mv moves a submodule with gitfile' ' ' test_expect_success 'mv does not complain when no .gitmodules file is found' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git rm .gitmodules entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod git mv sub mod/sub 2actual.err ! test -s actual.err ! test -e sub @@ -390,11 +392,12 @@ test_expect_success 'mv does not complain when no .gitmodules file is found' ' ' test_expect_success 'mv will error out on a modified .gitmodules file unless staged' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git config -f .gitmodules foo.bar true entry=$(git ls-files --stage sub | cut -f 1) + mkdir mod test_must_fail git mv sub mod/sub 2actual.err test -s actual.err test -e sub @@ -413,13 +416,14 @@ test_expect_success 'mv will error out on a modified .gitmodules file unless sta ' test_expect_success 'mv issues a warning when section is not found in .gitmodules' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update git config -f .gitmodules --remove-section submodule.sub git add .gitmodules entry=$(git ls-files --stage sub | cut -f 1) echo warning: Could not find section in .gitmodules where path=sub expect.err + mkdir mod git mv sub mod/sub 2actual.err test_i18ncmp expect.err actual.err ! test -e sub @@ -433,9 +437,10 @@ test_expect_success 'mv issues a warning when section is not found in .gitmodule ' test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules' ' - rm -rf mod/sub + rm -rf mod git reset --hard git submodule update + mkdir mod git mv -n sub mod/sub 2actual.err test -f sub/.git git diff-index --exit-code HEAD -- 2.1.0.rc2.206.gedb03e5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/25] wrapper.c: remove/unlink_or_warn: simplify, treat ENOENT as success
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Jul 2014 11:01:18 -0700
Simplify the function warn_if_unremovable slightly. Additionally, change
behaviour slightly. If we failed to remove the object because the object
does not exist, we can still return success back to the caller since none of
the callers depend on fail if the file did not exist.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
git-compat-util.h | 7 +--
refs.c| 2 +-
wrapper.c | 14 ++
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index fb41118..d67592f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -777,11 +777,14 @@ void git_qsort(void *base, size_t nmemb, size_t size,
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
- * Always returns the return value of unlink(2).
+ * Returns 0 on success, which includes trying to unlink an object that does
+ * not exist.
*/
int unlink_or_warn(const char *path);
/*
- * Likewise for rmdir(2).
+ * Preserves errno, prints a message, but gives no warning for ENOENT.
+ * Returns 0 on success, which includes trying to remove a directory that does
+ * not exist.
*/
int rmdir_or_warn(const char *path);
/*
diff --git a/refs.c b/refs.c
index a77458f..2dcf6c6 100644
--- a/refs.c
+++ b/refs.c
@@ -2607,7 +2607,7 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
char *loose_filename = get_locked_file_path(lock-lk);
int err = unlink_or_warn(loose_filename);
free(loose_filename);
- if (err errno != ENOENT)
+ if (err)
return 1;
}
return 0;
diff --git a/wrapper.c b/wrapper.c
index 5b77d2a..8d4be66 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -466,14 +466,12 @@ int xmkstemp_mode(char *template, int mode)
static int warn_if_unremovable(const char *op, const char *file, int rc)
{
- if (rc 0) {
- int err = errno;
- if (ENOENT != err) {
- warning(unable to %s %s: %s,
- op, file, strerror(errno));
- errno = err;
- }
- }
+ int err;
+ if (!rc || errno == ENOENT)
+ return 0;
+ err = errno;
+ warning(unable to %s %s: %s, op, file, strerror(errno));
+ errno = err;
return rc;
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/25] refs.c: lock_ref_sha1_basic is used for all refs
From: Ronnie Sahlberg sahlb...@google.com Date: Thu, 2 Oct 2014 07:59:02 -0700 lock_ref_sha1_basic is used to lock refs that sit directly in the .git dir such as HEAD and MERGE_HEAD in addition to the more ordinary refs under refs/. Remove the note claiming otherwise. Signed-off-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 2dcf6c6..4f2564d 100644 --- a/refs.c +++ b/refs.c @@ -2134,7 +2134,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log) } /* - * Locks a refs/ ref returning the lock on success and NULL on failure. + * Locks a ref returning the lock on success and NULL on failure. * On failure errno is set to something meaningful. */ static struct ref_lock *lock_ref_sha1_basic(const char *refname, -- 2.1.0.rc2.206.gedb03e5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/25] wrapper.c: add a new function unlink_or_msg
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 16 Jul 2014 11:20:36 -0700
This behaves like unlink_or_warn except that on failure it writes the message
to its 'err' argument, which the caller can display in an appropriate way or
ignore.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
git-compat-util.h | 9 +
wrapper.c | 14 ++
2 files changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index d67592f..59ecf21 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -326,6 +326,8 @@ static inline char *git_find_last_dir_sep(const char *path)
#include wildmatch.h
+struct strbuf;
+
/* General helper functions */
extern void vreportf(const char *prefix, const char *err, va_list params);
extern void vwritef(int fd, const char *prefix, const char *err, va_list
params);
@@ -781,6 +783,13 @@ void git_qsort(void *base, size_t nmemb, size_t size,
* not exist.
*/
int unlink_or_warn(const char *path);
+ /*
+ * Tries to unlink file. Returns 0 if unlink succeeded
+ * or the file already didn't exist. Returns -1 and
+ * appends a message to err suitable for
+ * 'error(%s, err-buf)' on error.
+ */
+int unlink_or_msg(const char *file, struct strbuf *err);
/*
* Preserves errno, prints a message, but gives no warning for ENOENT.
* Returns 0 on success, which includes trying to remove a directory that does
diff --git a/wrapper.c b/wrapper.c
index 8d4be66..007ec0d 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -475,6 +475,20 @@ static int warn_if_unremovable(const char *op, const char
*file, int rc)
return rc;
}
+int unlink_or_msg(const char *file, struct strbuf *err)
+{
+ int rc = unlink(file);
+
+ assert(err);
+
+ if (!rc || errno == ENOENT)
+ return 0;
+
+ strbuf_addf(err, unable to unlink %s: %s,
+ file, strerror(errno));
+ return -1;
+}
+
int unlink_or_warn(const char *file)
{
return warn_if_unremovable(unlink, file, unlink(file));
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/25] refs.c: add an err argument to delete_ref_loose
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 15 May 2014 08:25:23 -0700
Add an err argument to delete_ref_loose so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_ref_loose.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 4f2564d..430857b 100644
--- a/refs.c
+++ b/refs.c
@@ -2597,7 +2597,7 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
return ret;
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/*
@@ -2605,9 +2605,9 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
* lockfile name, minus .lock:
*/
char *loose_filename = get_locked_file_path(lock-lk);
- int err = unlink_or_warn(loose_filename);
+ int res = unlink_or_msg(loose_filename, err);
free(loose_filename);
- if (err)
+ if (res)
return 1;
}
return 0;
@@ -3658,7 +3658,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type);
+ ret |= delete_ref_loose(update-lock, update-type,
+ err);
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/25] refs.c: pass the ref log message to _create/delete/update instead of _commit
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 30 Apr 2014 12:22:42 -0700
Change the ref transaction API so that we pass the reflog message to the
create/delete/update functions instead of to ref_transaction_commit.
This allows different reflog messages for each ref update in a multi-ref
transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 5 +++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 34 +-
refs.h | 12 ++--
sequencer.c| 4 ++--
walker.c | 5 ++---
11 files changed, 54 insertions(+), 44 deletions(-)
diff --git a/branch.c b/branch.c
index 9a2228e..884c62c 100644
--- a/branch.c
+++ b/branch.c
@@ -285,8 +285,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, err) ||
- ref_transaction_commit(transaction, msg, err))
+ null_sha1, 0, !forcing, msg, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
strbuf_release(err);
diff --git a/builtin/commit.c b/builtin/commit.c
index 81dc622..a7857ab 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1811,8 +1811,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, HEAD, sha1,
current_head
? current_head-object.sha1 : NULL,
- 0, !!current_head, err) ||
- ref_transaction_commit(transaction, sb.buf, err)) {
+ 0, !!current_head, sb.buf, err) ||
+ ref_transaction_commit(transaction, err)) {
rollback_index_files();
die(%s, err.buf);
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f2f6c67..df6c337 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -842,8 +842,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, err) ||
- ref_transaction_commit(transaction, push, err)) {
+ new_sha1, old_sha1, 0, 1, push,
+ err) ||
+ ref_transaction_commit(transaction, err)) {
ref_transaction_free(transaction);
rp_error(%s, err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 8020db8..85d39b5 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -171,8 +171,9 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(err);
if (!transaction ||
- ref_transaction_update(transaction, ref, repl, prev, 0, 1, err) ||
- ref_transaction_commit(transaction, NULL, err))
+ ref_transaction_update(transaction, ref, repl, prev,
+ 0, 1, NULL, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index a81b9e4..e633f4e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -733,8 +733,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
- 0, 1, err) ||
- ref_transaction_commit(transaction, NULL, err))
+ 0, 1, NULL, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
if (force !is_null_sha1(prev) hashcmp(prev, object))
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 54a48c0..6c9be05 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -14,6 +14,7 @@ static const char * const git_update_ref_usage[] = {
static char line_termination = '\n';
static int update_flags;
+static const char *msg;
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -198,7 +199,7 @@ static const
[PATCH 07/25] rename_ref: don't ask read_ref_full where the ref came from
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 30 Apr 2014 12:41:04 -0700
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index f5d7bd7..3c45615 100644
--- a/refs.c
+++ b/refs.c
@@ -2721,7 +2721,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, flag)
+ if (!read_ref_full(newrefname, sha1, 1, NULL)
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path(%s,
newrefname))) {
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/25] refs.c: call lock_ref_sha1_basic directly from commit
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 10:43:39 -0700
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 9c01623..b591b9c 100644
--- a/refs.c
+++ b/refs.c
@@ -3632,12 +3632,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = lock_any_ref_for_update(update-refname,
- (update-have_old ?
- update-old_sha1 :
- NULL),
- update-flags,
- update-type);
+ update-lock = lock_ref_sha1_basic(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/25] refs.c: refuse to lock badly named refs in lock_ref_sha1_basic
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 10:40:10 -0700
Move the check for check_refname_format from lock_any_ref_for_update to
lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely. This has no visible impact to callers
except for the inability to lock badly named refs, which is not possible
today already for other reasons.(*)
Keep lock_any_ref_for_update as a no-op wrapper. It is the public facing
version of this interface and keeping it as a separate function will make
it easier to experiment with the internal lock_ref_sha1_basic signature.
(*) For example, if lock_ref_sha1_basic checks the refname format and
refuses to lock badly named refs, it will not be possible to delete
such refs because the first step of deletion is to lock the ref. We
currently already fail in that case because these refs are not recognized
to exist:
$ cp .git/refs/heads/master .git/refs/heads/echo...\*\*
$ git branch -D .git/refs/heads/echo...\*\*
error: branch '.git/refs/heads/echo...**' not found.
This has been broken for a while. Later patches in the series will start
repairing the handling of badly named refs.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 3c45615..9c01623 100644
--- a/refs.c
+++ b/refs.c
@@ -2150,6 +2150,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int missing = 0;
int attempts_remaining = 3;
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ errno = EINVAL;
+ return NULL;
+ }
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock-lock_fd = -1;
@@ -2241,8 +2246,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/25] refs.c: pass a list of names to skip to is_refname_available
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 1 May 2014 11:16:07 -0700
Change is_refname_available to take a list of strings to exclude when
checking for conflicts instead of just one single name. We can already
exclude a single name for the sake of renames. This generalizes that support.
ref_transaction_commit already tracks a set of refs that are being deleted
in an array. This array is then used to exclude refs from being written to
the packed-refs file. At some stage we will want to change this array to a
struct string_list and then we can pass it to is_refname_available via the
call to lock_ref_sha1_basic. That will allow us to perform transactions
that perform multiple renames as long as there are no conflicts within the
starting or ending state.
For example, that would allow a single transaction that contains two
renames that are both individually conflicting:
m - n/n
n - m/m
No functional change intended yet.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 50 --
1 file changed, 32 insertions(+), 18 deletions(-)
diff --git a/refs.c b/refs.c
index b591b9c..a007cf3 100644
--- a/refs.c
+++ b/refs.c
@@ -787,13 +787,13 @@ static void prime_ref_dir(struct ref_dir *dir)
}
}
-static int entry_matches(struct ref_entry *entry, const char *refname)
+static int entry_matches(struct ref_entry *entry, const struct string_list
*list)
{
- return refname !strcmp(entry-name, refname);
+ return list string_list_has_string(list, entry-name);
}
struct nonmatching_ref_data {
- const char *skip;
+ const struct string_list *skip;
struct ref_entry *found;
};
@@ -817,16 +817,19 @@ static void report_refname_conflict(struct ref_entry
*entry,
/*
* Return true iff a reference named refname could be created without
* conflicting with the name of an existing reference in dir. If
- * oldrefname is non-NULL, ignore potential conflicts with oldrefname
- * (e.g., because oldrefname is scheduled for deletion in the same
+ * skip is non-NULL, ignore potential conflicts with refs in skip
+ * (e.g., because they are scheduled for deletion in the same
* operation).
*
* Two reference names conflict if one of them exactly matches the
* leading components of the other; e.g., foo/bar conflicts with
* both foo and with foo/bar/baz but not with foo/bar or
* foo/barbados.
+ *
+ * skip must be sorted.
*/
-static int is_refname_available(const char *refname, const char *oldrefname,
+static int is_refname_available(const char *refname,
+ const struct string_list *skip,
struct ref_dir *dir)
{
const char *slash;
@@ -840,12 +843,12 @@ static int is_refname_available(const char *refname,
const char *oldrefname,
* looking for a conflict with a leaf entry.
*
* If we find one, we still must make sure it is
-* not oldrefname.
+* not in skip.
*/
pos = search_ref_dir(dir, refname, slash - refname);
if (pos = 0) {
struct ref_entry *entry = dir-entries[pos];
- if (entry_matches(entry, oldrefname))
+ if (entry_matches(entry, skip))
return 1;
report_refname_conflict(entry, refname);
return 0;
@@ -878,13 +881,13 @@ static int is_refname_available(const char *refname,
const char *oldrefname,
/*
* We found a directory named refname. It is a
* problem iff it contains any ref that is not
-* oldrefname.
+* in skip.
*/
struct ref_entry *entry = dir-entries[pos];
struct ref_dir *dir = get_ref_dir(entry);
struct nonmatching_ref_data data;
- data.skip = oldrefname;
+ data.skip = skip;
sort_ref_dir(dir);
if (!do_for_each_entry_in_dir(dir, 0, nonmatching_ref_fn,
data))
return 1;
@@ -2139,6 +2142,7 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
*/
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
+ const struct string_list *skip,
int flags, int *type_p)
{
char *ref_file;
@@ -2188,7 +2192,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing
-!is_refname_available(refname, NULL, get_packed_refs(ref_cache)))
{
+!is_refname_available(refname, skip
[PATCH 11/25] refs.c: ref_transaction_commit: distinguish name conflicts from other errors
From: Ronnie Sahlberg sahlb...@google.com
Date: Fri, 16 May 2014 14:14:38 -0700
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either
when we lstat the new refname or if the name checking function reports that
the same type of conflict happened. In both cases, it means that we can not
create the new ref due to a name conflict.
Start defining specific return codes for _commit. TRANSACTION_NAME_CONFLICT
refers to a failure to create a ref due to a name conflict with another ref.
TRANSACTION_GENERIC_ERROR is for all other errors.
When git fetch is creating refs, name conflicts differ from other errors in
that they are likely to be resolved by running git remote prune remote.
git fetch currently inspects errno to decide whether to give that advice.
Once it switches to the transaction API, it can check for
TRANSACTION_NAME_CONFLICT instead.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 26 --
refs.h | 9 +++--
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/refs.c b/refs.c
index a007cf3..9d9bbeb 100644
--- a/refs.c
+++ b/refs.c
@@ -3637,9 +3637,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err);
- if (ret)
+ if (ref_update_reject_duplicates(updates, n, err)) {
+ ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
+ }
/* Acquire all locks while verifying old values */
for (i = 0; i n; i++) {
@@ -3653,10 +3654,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type);
if (!update-lock) {
+ ret = (errno == ENOTDIR)
+ ? TRANSACTION_NAME_CONFLICT
+ : TRANSACTION_GENERIC_ERROR;
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
update-refname);
- ret = 1;
goto cleanup;
}
}
@@ -3666,15 +3669,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update-new_sha1)) {
- ret = write_ref_sha1(update-lock, update-new_sha1,
-update-msg);
- update-lock = NULL; /* freed by write_ref_sha1 */
- if (ret) {
+ if (write_ref_sha1(update-lock, update-new_sha1,
+ update-msg)) {
+ update-lock = NULL; /* freed by write_ref_sha1
*/
if (err)
strbuf_addf(err, Cannot update the ref
'%s'.,
update-refname);
+ ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
+ update-lock = NULL; /* freed by write_ref_sha1 */
}
}
@@ -3683,14 +3687,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type,
- err);
+ if (delete_ref_loose(update-lock, update-type, err))
+ ret = TRANSACTION_GENERIC_ERROR;
+
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
}
- ret |= repack_without_refs(delnames, delnum, err);
+ if (repack_without_refs(delnames, delnum, err))
+ ret = TRANSACTION_GENERIC_ERROR;
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
diff --git a/refs.h b/refs.h
index 1a55cab..50b115a 100644
--- a/refs.h
+++ b/refs.h
@@ -326,9 +326,14 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
/*
* Commit all of the changes that have been queued in transaction, as
- * atomically as possible. Return a nonzero value if there is a
- * problem.
+ * atomically as possible.
+ *
+ * Returns 0 for success, or one of the below error codes for errors.
*/
+/* Naming conflict (for example, the ref names A and A/B conflict). */
+#define TRANSACTION_NAME_CONFLICT -1
+/* All other errors
[PATCH 12/25] fetch.c: change s_update_ref to use a ref transaction
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 13:49:07 -0700
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/fetch.c | 34 --
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 159fb7e..6ffd023 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -404,23 +404,37 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv(GIT_REFLOG_ACTION);
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+ int ret, df_conflict = 0;
if (dry_run)
return 0;
if (!rla)
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
- lock = lock_any_ref_for_update(ref-name,
- check_old ? ref-old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref-new_sha1, msg) 0)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ ref-old_sha1, 0, check_old, msg, err))
+ goto fail;
+
+ ret = ref_transaction_commit(transaction, err);
+ if (ret) {
+ df_conflict = (ret == TRANSACTION_NAME_CONFLICT);
+ goto fail;
+ }
+
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return 0;
+fail:
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
+ : STORE_REF_ERROR_OTHER;
}
#define REFCOL_WIDTH 10
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/25] refs.c: make write_ref_sha1 static
From: Ronnie Sahlberg sahlb...@google.com
Date: Mon, 28 Apr 2014 15:36:58 -0700
No external users call write_ref_sha1 any more so let's declare it static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
refs.c | 10 --
refs.h | 3 ---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 9d9bbeb..6b4236a 100644
--- a/refs.c
+++ b/refs.c
@@ -2706,6 +2706,9 @@ static int rename_ref_available(const char *oldname,
const char *newname)
return ret;
}
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+ const char *logmsg);
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -2950,8 +2953,11 @@ int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
-/* This function must return a meaningful errno */
-int write_ref_sha1(struct ref_lock *lock,
+/*
+ * Write sha1 into the ref specified by the lock. Make sure that errno
+ * is sane on error.
+ */
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index 50b115a..eea1044 100644
--- a/refs.h
+++ b/refs.h
@@ -197,9 +197,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/*
* Setup reflog before using. Set errno to something meaningful on failure.
*/
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/25] refs.c: change resolve_ref_unsafe reading argument to be a flags field
From: Ronnie Sahlberg sahlb...@google.com
Date: Tue, 15 Jul 2014 12:59:36 -0700
resolve_ref_unsafe takes a boolean argument for reading (a nonexistent ref
resolves successfully for writing but not for reading). Change this to be
a flags field instead, and pass the new constant RESOLVE_REF_READING when
we want this behaviour.
While at it, swap two of the arguments in the function to put output
arguments at the end. As a nice side effect, this ensures that we can
catch callers that were unaware of the new API so they can be audited.
Give the wrapper functions resolve_refdup and read_ref_full the same
treatment for consistency.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
branch.c| 2 +-
builtin/blame.c | 2 +-
builtin/branch.c| 9 ++---
builtin/checkout.c | 6 ++--
builtin/clone.c | 2 +-
builtin/commit.c| 2 +-
builtin/fmt-merge-msg.c | 2 +-
builtin/for-each-ref.c | 6 ++--
builtin/fsck.c | 2 +-
builtin/log.c | 3 +-
builtin/merge.c | 2 +-
builtin/notes.c | 2 +-
builtin/receive-pack.c | 4 +--
builtin/remote.c| 5 +--
builtin/show-branch.c | 7 ++--
builtin/symbolic-ref.c | 2 +-
bundle.c| 2 +-
cache.h | 23 ++--
http-backend.c | 4 ++-
notes-merge.c | 2 +-
reflog-walk.c | 5 +--
refs.c | 93 -
remote.c| 11 +++---
sequencer.c | 4 +--
transport-helper.c | 5 ++-
transport.c | 5 +--
upload-pack.c | 2 +-
wt-status.c | 2 +-
28 files changed, 124 insertions(+), 92 deletions(-)
diff --git a/branch.c b/branch.c
index 884c62c..4bab55a 100644
--- a/branch.c
+++ b/branch.c
@@ -170,7 +170,7 @@ int validate_new_branchname(const char *name, struct strbuf
*ref,
const char *head;
unsigned char sha1[20];
- head = resolve_ref_unsafe(HEAD, sha1, 0, NULL);
+ head = resolve_ref_unsafe(HEAD, 0, sha1, NULL);
if (!is_bare_repository() head !strcmp(head, ref-buf))
die(_(Cannot force update the current branch.));
}
diff --git a/builtin/blame.c b/builtin/blame.c
index 3838be2..303e217 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2286,7 +2286,7 @@ static struct commit *fake_working_tree_commit(struct
diff_options *opt,
commit-date = now;
parent_tail = commit-parents;
- if (!resolve_ref_unsafe(HEAD, head_sha1, 1, NULL))
+ if (!resolve_ref_unsafe(HEAD, RESOLVE_REF_READING, head_sha1, NULL))
die(no such ref: HEAD);
parent_tail = append_parent(parent_tail, head_sha1);
diff --git a/builtin/branch.c b/builtin/branch.c
index 6785097..6491bac 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -131,7 +131,8 @@ static int branch_merged(int kind, const char *name,
branch-merge[0]
branch-merge[0]-dst
(reference_name = reference_name_to_free =
-resolve_refdup(branch-merge[0]-dst, sha1, 1, NULL)) !=
NULL)
+resolve_refdup(branch-merge[0]-dst, RESOLVE_REF_READING,
+ sha1, NULL)) != NULL)
reference_rev = lookup_commit_reference(sha1);
}
if (!reference_rev)
@@ -235,7 +236,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, sha1, 0, flags);
+ target = resolve_ref_unsafe(name, 0, sha1, flags);
if (!target ||
(!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
error(remote_branch
@@ -299,7 +300,7 @@ static char *resolve_symref(const char *src, const char
*prefix)
int flag;
const char *dst;
- dst = resolve_ref_unsafe(src, sha1, 0, flag);
+ dst = resolve_ref_unsafe(src, 0, sha1, flag);
if (!(dst (flag REF_ISSYMREF)))
return NULL;
if (prefix)
@@ -869,7 +870,7 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
track = git_branch_track;
- head = resolve_refdup(HEAD, head_sha1, 0, NULL);
+ head = resolve_refdup(HEAD, 0, head_sha1, NULL);
if (!head)
die(_(Failed to resolve HEAD as a valid ref.));
if (!strcmp(head, HEAD))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b4decd5..5410dac 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -355,7 +355,7 @@ static int checkout_paths(const struct checkout_opts *opts,
if (write_locked_index(the_index, lock_file, COMMIT_LOCK))
die(_(unable to write new index
[PATCH 16/25] branch -d: avoid repeated symref resolution
Date: Wed, 10 Sep 2014 18:22:48 -0700
If a repository gets in a broken state with too much symref nesting,
it cannot be repaired with git branch -d:
$ git symbolic-ref refs/heads/nonsense refs/heads/nonsense
$ git branch -d nonsense
error: branch 'nonsense' not found.
Worse, git update-ref --no-deref -d doesn't work for such repairs
either:
$ git update-ref -d refs/heads/nonsense
error: unable to resolve reference refs/heads/nonsense: Too many levels of
symbolic links
Fix both by teaching resolve_ref_unsafe a new RESOLVE_REF_NO_RECURSE
flag and passing it when appropriate.
Callers can still read the value of a symref (for example to print a
message about it) with that flag set --- resolve_ref_unsafe will
resolve one level of symrefs and stop there.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/branch.c | 3 ++-
cache.h | 6 ++
refs.c| 17 +++--
refs.h| 2 ++
t/t1400-update-ref.sh | 34 ++
t/t3200-branch.sh | 9 +
6 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6491bac..2ad2d0b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -236,7 +236,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, 0, sha1, flags);
+ target = resolve_ref_unsafe(name, RESOLVE_REF_NO_RECURSE,
+ sha1, flags);
if (!target ||
(!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
error(remote_branch
diff --git a/cache.h b/cache.h
index 7200639..b735f3f 100644
--- a/cache.h
+++ b/cache.h
@@ -973,6 +973,11 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* writing to the ref, the return value is the name of the ref
* that will actually be created or changed.
*
+ * If the RESOLVE_REF_NO_RECURSE flag is passed, only resolves one
+ * level of symbolic reference. The value stored in sha1 for a symbolic
+ * reference will always be null_sha1 in this case, and the return
+ * value is the reference that the symref refers to directly.
+ *
* If flags is non-NULL, set the value that it points to the
* combination of REF_ISPACKED (if the reference was found among the
* packed references), REF_ISSYMREF (if the initial reference was a
@@ -985,6 +990,7 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* errno is set to something meaningful on error.
*/
#define RESOLVE_REF_READING 0x01
+#define RESOLVE_REF_NO_RECURSE 0x02
extern const char *resolve_ref_unsafe(const char *ref, int resolve_flags,
unsigned char *sha1, int *flags);
extern char *resolve_refdup(const char *ref, int resolve_flags, unsigned char
*sha1, int *flags);
diff --git a/refs.c b/refs.c
index f8d59ab..4fe263e 100644
--- a/refs.c
+++ b/refs.c
@@ -1467,6 +1467,10 @@ const char *resolve_ref_unsafe(const char *refname, int
resolve_flags, unsigned
refname = refname_buffer;
if (flags)
*flags |= REF_ISSYMREF;
+ if (resolve_flags RESOLVE_REF_NO_RECURSE) {
+ hashclr(sha1);
+ return refname;
+ }
continue;
}
}
@@ -1523,13 +1527,17 @@ const char *resolve_ref_unsafe(const char *refname, int
resolve_flags, unsigned
buf = buffer + 4;
while (isspace(*buf))
buf++;
+ refname = strcpy(refname_buffer, buf);
+ if (resolve_flags RESOLVE_REF_NO_RECURSE) {
+ hashclr(sha1);
+ return refname;
+ }
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
if (flags)
*flags |= REF_ISBROKEN;
errno = EINVAL;
return NULL;
}
- refname = strcpy(refname_buffer, buf);
}
}
@@ -2170,6 +2178,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (mustexist)
resolve_flags |= RESOLVE_REF_READING;
+ if (flags REF_NODEREF flags REF_DELETING)
+ resolve_flags |= RESOLVE_REF_NO_RECURSE;
refname = resolve_ref_unsafe(refname, resolve_flags,
lock-old_sha1, type);
@@ -3664,13 +3674,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Acquire all locks while verifying old values */
for (i = 0; i n; i
[PATCH 15/25] reflog test: test interaction with detached HEAD
From: Junio C Hamano gits...@pobox.com
Date: Sat, 13 Sep 2014 10:52:25 -0700
A proposed patch produced broken HEAD reflog entries when checking out
anything other than a branch. The testsuite still passed, so it took
a few days for the bug to be noticed.
Add tests checking the content of the reflog after detaching and
reattaching HEAD so we don't have to rely on manual testing to catch
such problems in the future.
[jn: using 'log -g --format=%H' instead of parsing --oneline output,
resetting state in each test so they can be safely reordered or
skipped]
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
t/t1413-reflog-detach.sh | 70
1 file changed, 70 insertions(+)
create mode 100755 t/t1413-reflog-detach.sh
diff --git a/t/t1413-reflog-detach.sh b/t/t1413-reflog-detach.sh
new file mode 100755
index 000..c730600
--- /dev/null
+++ b/t/t1413-reflog-detach.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='Test reflog interaction with detached HEAD'
+. ./test-lib.sh
+
+reset_state () {
+ git checkout master
+ cp saved_reflog .git/logs/HEAD
+}
+
+test_expect_success setup '
+ test_tick
+ git commit --allow-empty -m initial
+ git branch side
+ test_tick
+ git commit --allow-empty -m second
+ cat .git/logs/HEAD saved_reflog
+'
+
+test_expect_success baseline '
+ reset_state
+ git rev-parse master master^ expect
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'switch to branch' '
+ reset_state
+ git rev-parse side master master^ expect
+ git checkout side
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'detach to other' '
+ reset_state
+ git rev-parse master side master master^ expect
+ git checkout side
+ git checkout master^0
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'detach to self' '
+ reset_state
+ git rev-parse master master master^ expect
+ git checkout master^0
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'attach to self' '
+ reset_state
+ git rev-parse master master master master^ expect
+ git checkout master^0
+ git checkout master
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_expect_success 'attach to other' '
+ reset_state
+ git rev-parse side master master master^ expect
+ git checkout master^0
+ git checkout side
+ git log -g --format=%H actual
+ test_cmp expect actual
+'
+
+test_done
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 18/25] packed-ref cache: forbid dot-components in refnames
Date: Fri, 26 Sep 2014 12:22:22 -0700
Since v1.7.9-rc1~10^2 (write_head_info(): handle extra refs locally,
2012-01-06), this trick to keep track of .have refs that are only
valid on the wire and not on the filesystem is not needed any more.
Simplify by removing support for the REFNAME_DOT_COMPONENT flag.
This means we'll be slightly stricter with invalid refs found in a
packed-refs file or during clone. read_loose_refs() already checks
for and skips refnames with .components so it is not affected.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 14 +++---
refs.h | 6 +-
2 files changed, 4 insertions(+), 16 deletions(-)
diff --git a/refs.c b/refs.c
index 4fe263e..fe1352a 100644
--- a/refs.c
+++ b/refs.c
@@ -70,16 +70,8 @@ static int check_refname_component(const char *refname, int
flags)
out:
if (cp == refname)
return 0; /* Component has zero length. */
- if (refname[0] == '.') {
- if (!(flags REFNAME_DOT_COMPONENT))
- return -1; /* Component starts with '.'. */
- /*
-* Even if leading dots are allowed, don't allow .
-* as a component (.. is prevented by a rule above).
-*/
- if (refname[1] == '\0')
- return -1; /* Component equals .. */
- }
+ if (refname[0] == '.')
+ return -1; /* Component starts with '.'. */
if (cp - refname = LOCK_SUFFIX_LEN
!memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))
return -1; /* Refname ends with .lock. */
@@ -290,7 +282,7 @@ static struct ref_entry *create_ref_entry(const char
*refname,
struct ref_entry *ref;
if (check_name
- check_refname_format(refname,
REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
+ check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
die(Reference has invalid format: '%s', refname);
len = strlen(refname) + 1;
ref = xmalloc(sizeof(struct ref_entry) + len);
diff --git a/refs.h b/refs.h
index 79802d7..c61b8f4 100644
--- a/refs.h
+++ b/refs.h
@@ -229,7 +229,6 @@ extern int for_each_reflog(each_ref_fn, void *);
#define REFNAME_ALLOW_ONELEVEL 1
#define REFNAME_REFSPEC_PATTERN 2
-#define REFNAME_DOT_COMPONENT 4
/*
* Return 0 iff refname has the correct format for a refname according
@@ -237,10 +236,7 @@ extern int for_each_reflog(each_ref_fn, void *);
* If REFNAME_ALLOW_ONELEVEL is set in flags, then accept one-level
* reference names. If REFNAME_REFSPEC_PATTERN is set in flags, then
* allow a * wildcard character in place of one of the name
- * components. No leading or repeated slashes are accepted. If
- * REFNAME_DOT_COMPONENT is set in flags, then allow refname
- * components to start with . (but not a whole component equal to
- * . or ..).
+ * components. No leading or repeated slashes are accepted.
*/
extern int check_refname_format(const char *refname, int flags);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/25] branch -d: simplify by using RESOLVE_REF_READING
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 11 Sep 2014 10:34:36 -0700
When git branch -d reads the branch it is about to delete, it used
to avoid passing the RESOLVE_REF_READING ('treat missing ref as
error') flag because a symref pointing to a nonexistent ref would show
up as missing instead of as something that could be deleted. To check
if a ref is actually missing, we then check
- is it a symref?
- if not, did it resolve to null_sha1?
Now we pass RESOLVE_REF_NO_RECURSE and the correct information is
returned for a symref even when it points to a missing ref. Simplify
by relying on RESOLVE_REF_READING.
No functional change intended.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/branch.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ad2d0b..0c7aac0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -236,10 +236,11 @@ static int delete_branches(int argc, const char **argv,
int force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- target = resolve_ref_unsafe(name, RESOLVE_REF_NO_RECURSE,
+ target = resolve_ref_unsafe(name,
+ RESOLVE_REF_READING
+ | RESOLVE_REF_NO_RECURSE,
sha1, flags);
- if (!target ||
- (!(flags REF_ISSYMREF) is_null_sha1(sha1))) {
+ if (!target) {
error(remote_branch
? _(remote branch '%s' not found.)
: _(branch '%s' not found.), bname.buf);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 19/25] test: put tests for handling of bad ref names in one place
From: Ronnie Sahlberg sahlb...@google.com Date: Thu, 25 Sep 2014 15:02:39 -0700 There's no straightforward way to grep for all tests dealing with invalid refs. Put them in a single test script so it is easy to see what functionality has not been exercised with bad ref names yet. Signed-off-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- t/t1400-update-ref.sh | 44 -- t/t1430-bad-ref-name.sh | 84 + t/t9300-fast-import.sh | 30 -- 3 files changed, 84 insertions(+), 74 deletions(-) create mode 100755 t/t1430-bad-ref-name.sh diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 7c8c41a..7b4707b 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -136,14 +136,6 @@ test_expect_success 'update-ref --no-deref -d can delete reference to bad ref' ' test_path_is_missing .git/refs/heads/ref-to-bad ' -test_expect_success 'update-ref --no-deref -d can delete reference to broken name' ' - git symbolic-ref refs/heads/badname refs/heads/broken...ref - test_when_finished rm -f .git/refs/heads/badname - test_path_is_file .git/refs/heads/badname - git update-ref --no-deref -d refs/heads/badname - test_path_is_missing .git/refs/heads/badname -' - test_expect_success '(not) create HEAD with old sha1' test_must_fail git update-ref HEAD $A $B @@ -408,12 +400,6 @@ test_expect_success 'stdin fails create with no ref' ' grep fatal: create: missing ref err ' -test_expect_success 'stdin fails create with bad ref name' ' - echo create ~a $m stdin - test_must_fail git update-ref --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin fails create with no new value' ' echo create $a stdin test_must_fail git update-ref --stdin stdin 2err @@ -432,12 +418,6 @@ test_expect_success 'stdin fails update with no ref' ' grep fatal: update: missing ref err ' -test_expect_success 'stdin fails update with bad ref name' ' - echo update ~a $m stdin - test_must_fail git update-ref --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin fails update with no new value' ' echo update $a stdin test_must_fail git update-ref --stdin stdin 2err @@ -456,12 +436,6 @@ test_expect_success 'stdin fails delete with no ref' ' grep fatal: delete: missing ref err ' -test_expect_success 'stdin fails delete with bad ref name' ' - echo delete ~a $m stdin - test_must_fail git update-ref --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin fails delete with too many arguments' ' echo delete $a $m $m stdin test_must_fail git update-ref --stdin stdin 2err @@ -734,12 +708,6 @@ test_expect_success 'stdin -z fails create with no ref' ' grep fatal: create: missing ref err ' -test_expect_success 'stdin -z fails create with bad ref name' ' - printf $F create ~a $m stdin - test_must_fail git update-ref -z --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin -z fails create with no new value' ' printf $F create $a stdin test_must_fail git update-ref -z --stdin stdin 2err @@ -764,12 +732,6 @@ test_expect_success 'stdin -z fails update with too few args' ' grep fatal: update $a: unexpected end of input when reading oldvalue err ' -test_expect_success 'stdin -z fails update with bad ref name' ' - printf $F update ~a $m stdin - test_must_fail git update-ref -z --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin -z emits warning with empty new value' ' git update-ref $a $m printf $F update $a stdin @@ -802,12 +764,6 @@ test_expect_success 'stdin -z fails delete with no ref' ' grep fatal: delete: missing ref err ' -test_expect_success 'stdin -z fails delete with bad ref name' ' - printf $F delete ~a $m stdin - test_must_fail git update-ref -z --stdin stdin 2err - grep fatal: invalid ref format: ~a err -' - test_expect_success 'stdin -z fails delete with no old value' ' printf $F delete $a stdin test_must_fail git update-ref -z --stdin stdin 2err diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh new file mode 100755 index 000..c7b19b0 --- /dev/null +++ b/t/t1430-bad-ref-name.sh @@ -0,0 +1,84 @@ +#!/bin/sh + +test_description='Test handling of ref names that check-ref-format rejects' +. ./test-lib.sh + +test_expect_success setup ' + test_commit one +' + +test_expect_success 'fast-import: fail on invalid branch name .badbranchname' ' + test_when_finished rm -f .git/objects/pack_* .git/objects/index_* + cat input -INPUT_END
[PATCH 20/25] refs.c: allow listing and deleting badly named refs
From: Ronnie Sahlberg sahlb...@google.com
Date: Wed, 3 Sep 2014 11:45:43 -0700
We currently do not handle badly named refs well:
$ cp .git/refs/heads/master .git/refs/heads/master.@\*@\\.
$ git branch
fatal: Reference has invalid format: 'refs/heads/master.@*@\.'
$ git branch -D master.@\*@\\.
error: branch 'master.@*@\.' not found.
Users cannot recover from a badly named ref without manually finding
and deleting the loose ref file or appropriate line in packed-refs.
Making that easier will make it easier to tweak the ref naming rules
in the future, for example to forbid shell metacharacters like '`'
and '', without putting people in a state that is hard to get out of.
So allow branch --list to show these refs and allow branch -d/-D
and update-ref -d to delete them. Other commands (for example to
rename refs) will continue to not handle these refs but can be changed
in later patches.
Details:
In resolving functions, refuse to resolve refs that don't pass the
git-check-ref-format(1) check unless the new RESOLVE_REF_ALLOW_BAD_NAME
flag is passed. Even with RESOLVE_REF_ALLOW_BAD_NAME, refuse to
resolve refs that escape the refs/ directory and do not match the
pattern [A-Z_]* (think HEAD and MERGE_HEAD).
In locking functions, refuse to act on badly named refs unless they
are being deleted and either are in the refs/ directory or match [A-Z_]*.
Just like other invalid refs, flag resolved, badly named refs with the
REF_ISBROKEN flag, treat them as resolving to null_sha1, and skip them
in all iteration functions except for for_each_rawref.
Flag badly named refs (but not symrefs pointing to badly named refs)
with a REF_BAD_NAME flag to make it easier for future callers to
notice and handle them specially. For example, in a later patch
for-each-ref will use this flag to detect refs whose names can confuse
callers parsing for-each-ref output.
In the transaction API, refuse to create or update badly named refs,
but allow deleting them (unless they try to escape refs/ and don't match
[A-Z_]*).
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/branch.c| 9 +--
cache.h | 17 +-
refs.c | 148 ++--
refs.h | 12 +++-
t/t1430-bad-ref-name.sh | 125 +++-
5 files changed, 273 insertions(+), 38 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 0c7aac0..7e113d6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -238,7 +238,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
name = mkpathdup(fmt, bname.buf);
target = resolve_ref_unsafe(name,
RESOLVE_REF_READING
- | RESOLVE_REF_NO_RECURSE,
+ | RESOLVE_REF_NO_RECURSE
+ | RESOLVE_REF_ALLOW_BAD_NAME,
sha1, flags);
if (!target) {
error(remote_branch
@@ -248,7 +249,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
continue;
}
- if (!(flags REF_ISSYMREF)
+ if (!(flags (REF_ISSYMREF|REF_ISBROKEN))
check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
force)) {
ret = 1;
@@ -268,8 +269,8 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
? _(Deleted remote branch %s (was %s).\n)
: _(Deleted branch %s (was %s).\n),
bname.buf,
- (flags REF_ISSYMREF)
- ? target
+ (flags REF_ISBROKEN) ? broken
+ : (flags REF_ISSYMREF) ? target
: find_unique_abbrev(sha1, DEFAULT_ABBREV));
}
delete_branch_config(bname.buf);
diff --git a/cache.h b/cache.h
index b735f3f..0501f7d 100644
--- a/cache.h
+++ b/cache.h
@@ -981,16 +981,29 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* If flags is non-NULL, set the value that it points to the
* combination of REF_ISPACKED (if the reference was found among the
* packed references), REF_ISSYMREF (if the initial reference was a
- * symbolic reference) and REF_ISBROKEN (if the ref is malformed).
+ * symbolic reference), REF_BAD_NAME (if the reference name is ill
+ * formed --- see RESOLVE_REF_ALLOW_BAD_NAME below), and REF_ISBROKEN
+ * (if the ref is malformed or has a bad name). See refs.h for more detail
+ * on each flag.
*
* If ref is not a properly-formatted
[PATCH 22/25] remote rm/prune: print a message when writing packed-refs fails
From: Ronnie Sahlberg sahlb...@google.com
Date: Thu, 11 Sep 2014 08:42:57 -0700
Until v2.1.0-rc0~22^2~11 (refs.c: add an err argument to
repack_without_refs, 2014-06-20), repack_without_refs forgot to
provide an error message when commit_packed_refs fails. Even today,
it only provides a message for callers that pass a non-NULL err
parameter. Internal callers in refs.c pass non-NULL err but
git remote does not.
That means that git remote rm and git remote prune can fail
without printing a message about why. Fix them by passing in a
non-NULL err parameter and printing the returned message.
This is the last caller to a ref handling function passing err ==
NULL. A later patch can drop support for err == NULL, avoiding such
problems in the future.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/remote.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 42a533a..7f28f92 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -749,13 +749,16 @@ static int mv(int argc, const char **argv)
static int remove_branches(struct string_list *branches)
{
+ struct strbuf err = STRBUF_INIT;
const char **branch_names;
int i, result = 0;
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
branch_names[i] = branches-items[i].string;
- result |= repack_without_refs(branch_names, branches-nr, NULL);
+ if (repack_without_refs(branch_names, branches-nr, err))
+ result |= error(%s, err.buf);
+ strbuf_release(err);
free(branch_names);
for (i = 0; i branches-nr; i++) {
@@ -1332,9 +1335,13 @@ static int prune_remote(const char *remote, int dry_run)
delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
for (i = 0; i states.stale.nr; i++)
delete_refs[i] = states.stale.items[i].util;
- if (!dry_run)
- result |= repack_without_refs(delete_refs,
- states.stale.nr, NULL);
+ if (!dry_run) {
+ struct strbuf err = STRBUF_INIT;
+ if (repack_without_refs(delete_refs, states.stale.nr,
+ err))
+ result |= error(%s, err.buf);
+ strbuf_release(err);
+ }
free(delete_refs);
}
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 23/25] refs.c: do not permit err == NULL
Date: Thu, 28 Aug 2014 16:42:37 -0700
Some functions that take a strbuf argument to append an error treat
!err as an indication that the message should be suppressed (e.g.,
ref_update_reject_duplicates). Others write the message to stderr on
!err (e.g., repack_without_refs). Others crash (e.g.,
ref_transaction_update).
Some of these behaviors are for historical reasons and others were
accidents. Luckily no callers pass err == NULL any more. Simplify
by consistently requiring the strbuf argument.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 46 +++---
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/refs.c b/refs.c
index e7000f2..097fb4b 100644
--- a/refs.c
+++ b/refs.c
@@ -2646,6 +2646,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
struct string_list_item *ref_to_delete;
int i, ret, removed = 0;
+ assert(err);
+
/* Look for a packed ref */
for (i = 0; i n; i++)
if (get_packed_ref(refnames[i]))
@@ -2656,13 +2658,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
- if (err) {
- unable_to_lock_message(git_path(packed-refs), errno,
- err);
- return -1;
- }
- unable_to_lock_error(git_path(packed-refs), errno);
- return error(cannot delete '%s' from packed refs,
refnames[i]);
+ unable_to_lock_message(git_path(packed-refs), errno, err);
+ return -1;
}
packed = get_packed_refs(ref_cache);
@@ -2688,7 +2685,7 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
/* Write what remains */
ret = commit_packed_refs();
- if (ret err)
+ if (ret)
strbuf_addf(err, unable to overwrite old ref-pack file: %s,
strerror(errno));
return ret;
@@ -2696,6 +2693,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
+ assert(err);
+
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/*
* loose. The loose file name is the same as the
@@ -3551,6 +3550,8 @@ struct ref_transaction {
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
{
+ assert(err);
+
return xcalloc(1, sizeof(struct ref_transaction));
}
@@ -3590,6 +3591,8 @@ int ref_transaction_update(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: update called for transaction that is not open);
@@ -3622,6 +3625,8 @@ int ref_transaction_create(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: create called for transaction that is not open);
@@ -3653,6 +3658,8 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: delete called for transaction that is not open);
@@ -3715,13 +3722,14 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
struct strbuf *err)
{
int i;
+
+ assert(err);
+
for (i = 1; i n; i++)
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
- const char *str =
- Multiple updates for ref '%s' not allowed.;
- if (err)
- strbuf_addf(err, str, updates[i]-refname);
-
+ strbuf_addf(err,
+ Multiple updates for ref '%s' not
allowed.,
+ updates[i]-refname);
return 1;
}
return 0;
@@ -3735,6 +3743,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
+ assert(err);
+
if (transaction-state != REF_TRANSACTION_OPEN)
die(BUG: commit called for transaction that is not open);
@@ -3771,9 +3781,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = (errno == ENOTDIR)
? TRANSACTION_NAME_CONFLICT
: TRANSACTION_GENERIC_ERROR
[PATCH 21/25] for-each-ref: skip and warn about broken ref names
From: Ronnie Sahlberg sahlb...@google.com
Date: Fri, 5 Sep 2014 14:35:17 -0700
Print a warning message for any bad ref names we find in the repo and
skip them so callers don't have to deal with parsing them.
It might be useful in the future to have a flag where we would not
skip these refs for those callers that want to and are prepared (for
example by using a --format argument with %0 as a delimiter after the
ref name).
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/for-each-ref.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 492265d..3ee22b9 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -839,6 +839,11 @@ static int grab_single_ref(const char *refname, const
unsigned char *sha1, int f
struct refinfo *ref;
int cnt;
+ if (flag REF_BAD_NAME) {
+ warning(ignoring ref with broken name %s, refname);
+ return 0;
+ }
+
if (*cb-grab_pattern) {
const char **pattern;
int namelen = strlen(refname);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 25/25] ref_transaction_commit: bail out on failure to remove a ref
Date: Thu, 28 Aug 2014 17:01:35 -0700
When removal of a loose or packed ref fails, bail out instead of
trying to finish the transaction. This way, a single error message
can be printed (instead of multiple messages being concatenated by
mistake) and the operator can try to solve the underlying problem
before there is a chance to muck things up even more.
In particular, when git fails to remove a ref, git goes on to try to
delete the reflog. Exiting early lets us keep the reflog.
When git succeeds in deleting a ref A and fails to remove a ref B, it
goes on to try to delete both reflogs. It would be better to just
remove the reflog for A, but that would be a more invasive change.
Failing early means we keep both reflogs, which puts the operator in a
good position to understand the problem and recover.
A long term goal is to avoid these problems altogether and roll back
the transaction on failure. That kind of transactionality will have
to wait for a later series (the plan for which is to make all
destructive work happen in a single update of the packed-refs file).
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
Thanks for reading.
refs.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 097fb4b..0368ed4 100644
--- a/refs.c
+++ b/refs.c
@@ -3809,16 +3809,20 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- if (delete_ref_loose(update-lock, update-type, err))
+ if (delete_ref_loose(update-lock, update-type, err)) {
ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
}
- if (repack_without_refs(delnames, delnum, err))
+ if (repack_without_refs(delnames, delnum, err)) {
ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 24/25] lockfile: remove unable_to_lock_error
Date: Thu, 28 Aug 2014 16:41:34 -0700
The former caller uses unable_to_lock_message now.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Reviewed-by: Ronnie Sahlberg sahlb...@google.com
---
lockfile.c | 10 --
lockfile.h | 1 -
2 files changed, 11 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index d098ade..4f16ee7 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -162,16 +162,6 @@ void unable_to_lock_message(const char *path, int err,
struct strbuf *buf)
absolute_path(path), strerror(err));
}
-int unable_to_lock_error(const char *path, int err)
-{
- struct strbuf buf = STRBUF_INIT;
-
- unable_to_lock_message(path, err, buf);
- error(%s, buf.buf);
- strbuf_release(buf);
- return -1;
-}
-
NORETURN void unable_to_lock_die(const char *path, int err)
{
struct strbuf buf = STRBUF_INIT;
diff --git a/lockfile.h b/lockfile.h
index dc066d1..cd2ec95 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -71,7 +71,6 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NO_DEREF 2
-extern int unable_to_lock_error(const char *path, int err);
extern void unable_to_lock_message(const char *path, int err,
struct strbuf *buf);
extern NORETURN void unable_to_lock_die(const char *path, int err);
--
2.1.0.rc2.206.gedb03e5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] builtin/merge.c: drop a parameter that is never used
Junio C Hamano wrote: Since the very beginning when we added the renormalizing parameter to this function with 7610fa57 (merge-recursive --renormalize, 2010-08-05), nobody seems to have ever referenced it. Signed-off-by: Junio C Hamano gits...@pobox.com Reviewed-by: Jonathan Nieder jrnie...@gmail.com * ... or is there any renormalization the said commit meant to but forgot to do? I suspect it's related to this TODO from rerere.c::handle_cache: /* * NEEDSWORK: handle conflicts from merges with * merge.renormalize set, too */ ll_merge(result, path, mmfile[0], NULL, mmfile[1], ours, mmfile[2], theirs, NULL); But if someone has time for it, rather than plumbing in a useless parameter that goes nowhere, it would be better to add tests as a reminder of what is unimplemented. :) Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge sequencer: turn Conflicts: hint into a comment
Jeff King wrote: For the most part, combined-diff (and --cc) will show the interesting cases anyway. But if you take a whole file from one side of the merge, then there is nothing interesting for diff to show. Do people still want to get that more complete list of potentially interesting files? And if so, how do they do it? I think there really isn't a great way besides repeating the merge. If you have time to experiment with tr/remerge-diff from pu[1], that would be welcome. Maybe some day it can be the default behavior for 'log -p'. If that is the only casualty, I think it is probably a net-win. Yes. Thanks, Jonathan [1] http://thread.gmane.org/gmane.comp.version-control.git/256591 -- 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: Safe to interrupt »git gc --auto«?
Thomas Schwinge wrote: I couldn't find this answered in the documentation: if, instead of exiting right away, a »git gc --auto« actually commences its housekeeping tasks, is it safe to interrupt (C-c, SIGINT) the original git invocation at this point, or might this cause any inconsistencies, data loss, [...] Heh. If gc --auto happens in the middle of e.g. a rebase, then it's possible that there were more commits that were supposed to happen later. You'd need to run 'git rebase --continue' after interrupting the gc in that case. Interruption should never cause data loss, and as much as possible commands should finish their work before running gc --auto. Please let us know if some command is violating that. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
Eric Sunshine wrote: On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: With the introduction of check-mailmap, it is now possible to check .mailmap functionality directly rather than indirectly as a side-effect of other commands (such as git-shortlog), therefore, do so. Does this patch mean that we will now ignore future breakages in shortlog and blame if their mailmap integration becomes buggy? I am not convinced it is a good idea if that is what is going on. I meant to mention in the cover letter that this patch was open for debate, however, it does not eliminate all testing of these other commands. The tests in which git-check-mailmap is substituted for git-shortlog all worked against a simplistic two-commit repository. Those tests were checking the low-level mailmap functionality under various conditions and configurations; they were not especially checking any particular behavior of git-shortlog. There still remain a final eight tests which cover git-shortlog, git-log, and git-blame. These tests do check mailmap-related behavior of those commands, and they do so using a more involved seven-commit repository with complex mappings, so we have not necessarily lost any checks of mailmap integration for those commands. Would this patch become more palatable if I stated the above in the commit message? My current thinking is no --- the patch has as a justification Now we can test these aspects of .mailmap handling directly with a low-level tool instead of using the tool most people will use, so do so, which sounds an awful lot like Reduce test coverage of commonly used tools, because we can. But I imagine the actual motivation was something other than because we can. For example, maybe the idea is that after this patch, it should be easier to make cosmetic improvements to the shortlog, log, and blame output and only have to change those final 8 tests that are adequately covering the output? If you have such plans and this patch makes them easier, it could sound like a good patch as a stepping stone toward that. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] .mailmap: Map email addresses to names
Stefan Beller wrote: --- a/.mailmap +++ b/.mailmap @@ -5,99 +5,146 @@ [...] Chris Shoemaker c.shoema...@cox.net -Dan Johnson computerdr...@gmail.com Dana L. How dana...@gmail.com Dana L. How h...@deathvalley.cswitch.com Daniel Barkalow barka...@iabervon.org +Dan Johnson computerdr...@gmail.com Small nit: the sorting looks broken here and in similar places below (the usual ordering is Dan Dana Daniel). Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable
Junio C Hamano wrote:
The existing code triggers only when the configuration variable is
set to true. Once the variable is set to true in a more generic
configuration file (e.g. ~/.gitconfig), it cannot be overriden to
false in the repository specific one (e.g. .git/config).
[...]
--- a/http.c
+++ b/http.c
@@ -160,8 +160,7 @@ static int http_options(const char *var, const char
*value, void *cb)
if (!strcmp(http.sslcainfo, var))
return git_config_string(ssl_cainfo, var, value);
if (!strcmp(http.sslcertpasswordprotected, var)) {
- if (git_config_bool(var, value))
- ssl_cert_password_required = 1;
+ ssl_cert_password_required = git_config_bool(var, value);
Thanks for catching it. The documentation doesn't say anything about
this can only enable and cannot disable behavior and the usual
pattern is to allow later settings to override earlier ones, so this
change looks good.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar can
only enable behavior, but since it's documented, that's not as big
of a problem. Do you remember why it was written that way?
When that setting was first added[1], there was some mention of
autodetection if libcurl could learn to do that. Did it happen?
(Please forgive my ignorance.)
Thanks,
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/122793/focus=122816
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-clone.txt: remove the restriction on pushing from a shallow clone
Hi, Nguyễn Thái Ngọc Duy wrote: Since 52fed6e (receive-pack: check connectivity before concluding git push - 2011-09-02), receive-pack is prepared to deal with broken push, a shallow push can't cause any corruption. Update the document to reflect that. Hmm, what happens when pushing to servers without that commit? Do you think it should be applied to Debian squeeze for server operators that haven't upgraded yet to the current stable release? [...] --- a/Documentation/git-clone.txt +++ b/Documentation/git-clone.txt @@ -182,11 +182,13 @@ objects from the source repository into a pack in the cloned repository. --depth depth:: Create a 'shallow' clone with a history truncated to the specified number of revisions. A shallow repository has a - number of limitations (you cannot clone or fetch from - it, nor push from nor into it), but is adequate if you - are only interested in the recent history of a large project - with a long history, and would want to send in fixes - as patches. + number of limitations (you cannot clone or fetch from it, nor + push into it), but is adequate if you are only interested in + the recent history of a large project with a long history. ++ +Pushing from a shallow clone should be avoided if the git version on +the receiver end is older than v1.7.10, or any other git +implementation that does not perform connectivity check. git name-rev --tags tells me 52fed6e was applied during 1.7.8-rc0, so it might make sense to s/1.7.10/1.7.8/ here. Aside from that nit, Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
