Re: [PATCH v8 4/4] cache-tree: Write updated cache-tree after commit
On Mon, Jul 14, 2014 at 7:15 PM, Duy Nguyen pclo...@gmail.com wrote: It makes me wonder if a cleaner way of rebuilding cache-treei in this case is from git-add--interactive.perl, or by simply spawn git update-index --rebuild-cache-tree after running git-add--interactive.perl. We could check if the cache-tree has fully been populated by add -i and limit the rebuilding by git commit -p main process, but if add -i did not do so, there is no reason why git commit -p should not do so, without relying on the implementation of add -i to do so. At least to me, what you suggested sounds nothing more than a cop-out; instead of lifting the limitation of the API by enhancing it with reopen-lock-file, punting to shift the burden elsewhere. I am not quite sure why that is cleaner, but perhaps I am missing something. In the longer run, it would be plausible that somebody would want to rewrite git-add -i and will make it just a C API call away without having to spawn a separate process. You cannot punt by saying make 'add -i' responsible for it at that point; you will be doing what 'add -i' would be doing yourself, no? -- 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 RFC v2 07/19] rebase -i: The replay of root commits is not shown with --verbose
Fabian Ruch baf...@gmail.com wrote: you're the original author of the code touched by this patch. Is the second -q option really a simple copy-and-paste of the first or am I overlooking something here? I'd like to confirm this as, in retrospect, I feel a bit uncertain about the hasty claim in the log message. It was a while ago and I don't remember the details of this patch I'm afraid, but your rationale here looks sensible to me. I tend to find git too noisy by default rather than too quiet, so it wouldn't surprise me if I never thought to try the --verbose version. Cheers, Chris. -- 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 v8 4/4] cache-tree: Write updated cache-tree after commit
On Mon, Jul 14, 2014 at 11:38:06PM -0700, Junio C Hamano wrote:
On Mon, Jul 14, 2014 at 7:15 PM, Duy Nguyen pclo...@gmail.com wrote:
It makes me wonder if a cleaner way of rebuilding cache-treei in this
case is from git-add--interactive.perl, or by simply spawn git
update-index --rebuild-cache-tree after running
git-add--interactive.perl.
We could check if the cache-tree has fully been populated by
add -i and limit the rebuilding by git commit -p main process,
but if add -i did not do so, there is no reason why git commit -p
should not do so, without relying on the implementation of add -i
to do so.
At least to me, what you suggested sounds nothing more than
a cop-out; instead of lifting the limitation of the API by enhancing
it with reopen-lock-file, punting to shift the burden elsewhere. I
am not quite sure why that is cleaner, but perhaps I am missing
something.
Reopen-lock-file to me does not sound like an enhancement, at least in
the index update context. We have always updated the index by writing
to a new file, then rename. Now if the new write_locked_index() blows
up after reopen_lock_file(), we have no way but die() because
index_lock.filename can no longer be trusted.
Doing it in a separate process keeps the tradition. And if the second
write fails, we still have good index_lock.filename. We could also do
the same here with something like this without new process (lightly
tested):
-- 8 --
diff --git a/builtin/commit.c b/builtin/commit.c
index 606c890..c2b4875 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -340,6 +340,18 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
discard_cache();
read_cache_from(index_lock.filename);
+ if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
+ static struct lock_file second_index_lock;
+ hold_lock_file_for_update(second_index_lock,
+ index_lock.filename,
+ LOCK_DIE_ON_ERROR);
+ if (write_locked_index(the_index, second_index_lock,
+ COMMIT_LOCK)) {
+ warning(_(Failed to update main cache tree));
+ rollback_lock_file(second_index_lock);
+ }
+ } else
+ warning(_(Failed to update main cache tree));
commit_style = COMMIT_NORMAL;
return index_lock.filename;
-- 8 --
I was worried about the dependency between second_index_lock and
index_lock, but at cleanup, all we do is rollback, which is safe
regardless of dependencies. Just need to make sure we commit
second_index_lock before index_lock.
In the longer run, it would be plausible that somebody would want
to rewrite git-add -i and will make it just a C API call away without
having to spawn a separate process. You cannot punt by saying
make 'add -i' responsible for it at that point; you will be doing
what 'add -i' would be doing yourself, no?
Yes, but at current rewrite rate I'd bet it won't happen in the next
two years at least.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/2] add `config_set` API for caching config-like files
On 7/11/2014 7:51 PM, Matthieu Moy wrote: +`int git_configset_add_file(struct config_set *cs, const char *filename)`:: + +Parses the file and adds the variable-value pairs to the `config_set`, +dies if there is an error in parsing the file. The return value is undocumented. If I read correctly, the only codepath from this to a syntax error sets die_on_error, hence dies if there is an error in parsing the file is correct. Still, there are errors like unreadable file or no such file that do not die (nor emit any error message, which is not very good for the user), and lead to returning -1 here. I'm not sure this distinction is right (why die on syntax error and continue running on unreadable file?). I checked the whole codebase and in all of the cases if they cannot read a file they return -1 and continue running. So, I have updated the API to document the return value. I think if the file is unreadable. we must continue running as no harm has been done yet, worse is parsing a file with wrong syntax which can cause reading wrong config values. So the decision to die on syntax error sounds alright to me. Cheers, Tanay Abhra. -- 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 v8 2/2] test-config: add tests for the config_set API
Junio C Hamano gitster at pobox.com writes:
Tanay Abhra tanayabh at gmail.com writes:
diff --git a/test-config.c b/test-config.c
new file mode 100644
index 000..dc313c2
--- /dev/null
+++ b/test-config.c
at at -0,0 +1,125 at at
+
+
+int main(int argc, char **argv)
+{
+ int i, val;
+ const char *v;
+ const struct string_list *strptr;
+ struct config_set cs;
+ git_configset_init(cs);
+
+ if (argc 2) {
+ fprintf(stderr, Please, provide a command name on the
command-line\n);
+ return 1;
+ } else if (argc == 3 !strcmp(argv[1], get_value)) {
+ if (!git_config_get_value(argv[2], v)) {
+ if (!v)
+ printf((NULL)\n);
This one is dubious. Is this for things like
(in .git/config)
[receive]
fsckobjects
Yes, it was meant for the above case.
and asking with
$ git config receive.fsckobjects
which I think gives an empty string? We may want to be consistent.
$ git config -l
shows NULL values as foo.bar
empty values as foo.bar=
So there is a difference between the two.
$ git config receive.fsckobjects does covert the NULL value to a (empty
string).
I had to diffrentiate between the two, so I took the path printf takes for
NULL strings, it prints them as (NULL).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] sha1_file: do not add own object directory as alternate
When adding alternate object directories, we try not to add the
directory of the current repository to avoid cycles. Unfortunately,
that test was broken, since it compared an absolute with a relative
path.
Signed-off-by: Ephrim Khong dr.kh...@gmail.com
---
Since v2: Added Johannes' comments.
sha1_file.c| 13 +
t/t7702-repack-cyclic-alternate.sh | 24
2 files changed, 33 insertions(+), 4 deletions(-)
create mode 100755 t/t7702-repack-cyclic-alternate.sh
diff --git a/sha1_file.c b/sha1_file.c
index 34d527f..88fd168 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -268,9 +268,9 @@ static struct alternate_object_database **alt_odb_tail;
* SHA1, an extra slash for the first level indirection, and the
* terminating NUL.
*/
-static int link_alt_odb_entry(const char *entry, const char *relative_base,
int depth)
+static int link_alt_odb_entry(const char *entry, const char *relative_base,
+ int depth, const char *normalized_objdir)
{
- const char *objdir = get_object_directory();
struct alternate_object_database *ent;
struct alternate_object_database *alt;
int pfxlen, entlen;
@@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, const char
*relative_base, int
return -1;
}
}
- if (!strcmp(ent-base, objdir)) {
+ if (!strcmp_icase(ent-base, normalized_objdir)) {
free(ent);
return -1;
}
@@ -344,6 +344,7 @@ static void link_alt_odb_entries(const char *alt, int len,
int sep,
struct string_list entries = STRING_LIST_INIT_NODUP;
char *alt_copy;
int i;
+ struct strbuf objdirbuf = STRBUF_INIT;
if (depth 5) {
error(%s: ignoring alternate object stores, nesting too deep.,
@@ -351,6 +352,9 @@ static void link_alt_odb_entries(const char *alt, int len,
int sep,
return;
}
+ strbuf_addstr(objdirbuf, absolute_path(get_object_directory()));
+ normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
+
alt_copy = xmemdupz(alt, len);
string_list_split_in_place(entries, alt_copy, sep, -1);
for (i = 0; i entries.nr; i++) {
@@ -361,11 +365,12 @@ static void link_alt_odb_entries(const char *alt, int
len, int sep,
error(%s: ignoring relative alternate object store %s,
relative_base, entry);
} else {
- link_alt_odb_entry(entry, relative_base, depth);
+ link_alt_odb_entry(entry, relative_base, depth,
objdirbuf.buf);
}
}
string_list_clear(entries, 0);
free(alt_copy);
+ strbuf_release(objdirbuf);
}
void read_info_alternates(const char * relative_base, int depth)
diff --git a/t/t7702-repack-cyclic-alternate.sh
b/t/t7702-repack-cyclic-alternate.sh
new file mode 100755
index 000..8341d46
--- /dev/null
+++ b/t/t7702-repack-cyclic-alternate.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+#
+# Copyright (c) 2014 Ephrim Khong
+#
+
+test_description='repack involving cyclic alternate'
+. ./test-lib.sh
+
+test_expect_success setup '
+ GIT_OBJECT_DIRECTORY=.git//../.git/objects
+ export GIT_OBJECT_DIRECTORY
+ touch a
+ git add a
+ git commit -m 1
+ git repack -adl
+ echo $(pwd)/.git/objects/../objects .git/objects/info/alternates
+'
+
+test_expect_success 're-packing repository with itsself as alternate' '
+ git repack -adl
+ git fsck
+'
+
+test_done
--
1.8.4.3
--
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 v8 1/2] add `config_set` API for caching config-like files
Tanay Abhra tanay...@gmail.com writes:
I checked the whole codebase and in all of the cases if they cannot read a
file
they return -1 and continue running.
More precisely: in git_config_from_file, any fopen failure results in
return -1. But in git_config_early (one caller of
git_config_from_file()), the file is checked before access, e.g.:
if (git_config_system() !access_or_die(git_etc_gitconfig(), R_OK,
0)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
data);
found += 1;
}
Essentially, if a config file does not exist, it's skipped (obviously
desirable), but if some really weird error occur (if err == ENOENT ||
err == ENOTDIR || ((flag ACCESS_EACCES_OK) err == EACCES is false,
from access_eacces_ok() in wrapper.c), then the process dies.
Permission denied errors are allowed for user-wide config, but not for
others. Read the log for commit 4698c8feb for more details.
Anyway, this is the part of the code you're not touching.
(I actually consider it as a bug that git config --file no-such-file
foo.bar and git config --file unreadable-file foo.bar fail without
error message, but your commit does not change this).
I think if the file is unreadable. we must continue running as no harm has
been
done yet, worse is parsing a file with wrong syntax which can cause reading
wrong config values. So the decision to die on syntax error sounds alright
to me.
In the case of git_config_check_init(), I think it makes sense to die,
because file accesses are protected with access_or_die(), so the return
value can be negative only if something really went wrong.
If you chose not to die, then you should check the return value in the
callers of git_config_check_init().
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] Win32: Unicode file name support (dirent)
From: Karsten Blees bl...@dcon.de
Date: Sat, 14 Jan 2012 22:01:09 +0100
Changes opendir/readdir to use Windows Unicode APIs and convert between
UTF-8/UTF-16.
Removes parameter checks that are already covered by xutftowcs_path. This
changes detection of ENAMETOOLONG from MAX_PATH - 2 to MAX_PATH (matching
is_dir_empty in mingw.c). If name + /* or the resulting absolute path is
too long, FindFirstFile fails and errno is set through err_win_to_posix.
Increases the size of dirent.d_name to accommodate the full
WIN32_FIND_DATA.cFileName converted to UTF-8 (UTF-16 to UTF-8 conversion
may grow by factor three in the worst case).
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Stepan Kasal ka...@ucw.cz
---
compat/win32/dirent.c | 30 ++
compat/win32/dirent.h | 2 +-
2 files changed, 11 insertions(+), 21 deletions(-)
diff --git a/compat/win32/dirent.c b/compat/win32/dirent.c
index 82a515c..52420ec 100644
--- a/compat/win32/dirent.c
+++ b/compat/win32/dirent.c
@@ -6,10 +6,10 @@ struct DIR {
int dd_stat; /* 0-based index */
};
-static inline void finddata2dirent(struct dirent *ent, WIN32_FIND_DATAA *fdata)
+static inline void finddata2dirent(struct dirent *ent, WIN32_FIND_DATAW *fdata)
{
- /* copy file name from WIN32_FIND_DATA to dirent */
- memcpy(ent-d_name, fdata-cFileName, sizeof(ent-d_name));
+ /* convert UTF-16 name to UTF-8 */
+ xwcstoutf(ent-d_name, fdata-cFileName, sizeof(ent-d_name));
/* Set file type, based on WIN32_FIND_DATA */
if (fdata-dwFileAttributes FILE_ATTRIBUTE_DIRECTORY)
@@ -20,25 +20,15 @@ static inline void finddata2dirent(struct dirent *ent,
WIN32_FIND_DATAA *fdata)
DIR *opendir(const char *name)
{
- char pattern[MAX_PATH];
- WIN32_FIND_DATAA fdata;
+ wchar_t pattern[MAX_PATH + 2]; /* + 2 for '/' '*' */
+ WIN32_FIND_DATAW fdata;
HANDLE h;
int len;
DIR *dir;
- /* check that name is not NULL */
- if (!name) {
- errno = EINVAL;
+ /* convert name to UTF-16 and check length MAX_PATH */
+ if ((len = xutftowcs_path(pattern, name)) 0)
return NULL;
- }
- /* check that the pattern won't be too long for FindFirstFileA */
- len = strlen(name);
- if (len + 2 = MAX_PATH) {
- errno = ENAMETOOLONG;
- return NULL;
- }
- /* copy name to temp buffer */
- memcpy(pattern, name, len + 1);
/* append optional '/' and wildcard '*' */
if (len !is_dir_sep(pattern[len - 1]))
@@ -47,7 +37,7 @@ DIR *opendir(const char *name)
pattern[len] = 0;
/* open find handle */
- h = FindFirstFileA(pattern, fdata);
+ h = FindFirstFileW(pattern, fdata);
if (h == INVALID_HANDLE_VALUE) {
DWORD err = GetLastError();
errno = (err == ERROR_DIRECTORY) ? ENOTDIR :
err_win_to_posix(err);
@@ -72,8 +62,8 @@ struct dirent *readdir(DIR *dir)
/* if first entry, dirent has already been set up by opendir */
if (dir-dd_stat) {
/* get next entry and convert from WIN32_FIND_DATA to dirent */
- WIN32_FIND_DATAA fdata;
- if (FindNextFileA(dir-dd_handle, fdata)) {
+ WIN32_FIND_DATAW fdata;
+ if (FindNextFileW(dir-dd_handle, fdata)) {
finddata2dirent(dir-dd_dir, fdata);
} else {
DWORD lasterr = GetLastError();
diff --git a/compat/win32/dirent.h b/compat/win32/dirent.h
index 8838cd6..058207e 100644
--- a/compat/win32/dirent.h
+++ b/compat/win32/dirent.h
@@ -10,7 +10,7 @@ typedef struct DIR DIR;
struct dirent {
unsigned char d_type; /* file type to prevent lstat after readdir
*/
- char d_name[MAX_PATH]; /* file name */
+ char d_name[MAX_PATH * 3]; /* file name (* 3 for UTF-8 conversion) */
};
DIR *opendir(const char *dirname);
--
2.0.0.9635.g0be03cb
--
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 0/3] fix test suite with mingw-unicode patches
Hello Hannes, attached please find the patches that Karsten pointed out: 1) The unicode file name support was omitted from his unicode patch series; my mistake, sorry. There is still big part missing: support for unicode environment; I can only hope the tests would choke on that. 2) Windows cannot pass non-UTF parameters (commit messages in this case): original patch by Pat Thoyts was extended to apply to other similar cases: the commit msg is passed through stdin. If there are still problems remaining, please tell us. Thanks, Stepan Karsten Blees (2): Win32: Unicode file name support (except dirent) Win32: Unicode file name support (dirent) Pat Thoyts and Stepan Kasal(1): tests: do not pass iso8859-1 encoded parameter compat/mingw.c | 198 +-- compat/mingw.h | 18 +++- compat/win32/dirent.c| 30 ++ compat/win32/dirent.h| 2 +- t/t4041-diff-submodule-option.sh | 6 +- t/t4205-log-pretty-formats.sh| 2 +- t/t6006-rev-list-format.sh | 4 +- t/t7102-reset.sh | 8 +- 8 files changed, 184 insertions(+), 84 deletions(-) -- 2.0.0.9635.g0be03cb -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] tests: do not pass iso8859-1 encoded parameter
From: Pat Thoyts pattho...@users.sourceforge.net
Date: Mon, 2 Sep 2013 15:44:54 +0100
git commit -m with some iso8859-1 encoded stuff is doomed to fail in MinGW,
because Windows don't let you pass encoded bytes to a process (CreateProcessW
always takes a UTF-16LE encoded string).
It is safe to pass the iso8859-1 message using a file or a pipe.
Thanks-to: Karsten Blees bl...@dcon.de
Author: Stepan Kasal ka...@ucw.cz
Signed-off-by: Stepan Kasal ka...@ucw.cz
---
t/t4041-diff-submodule-option.sh | 6 --
t/t4205-log-pretty-formats.sh| 2 +-
t/t6006-rev-list-format.sh | 4 ++--
t/t7102-reset.sh | 8 ++--
4 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 463d63b..e432896 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -26,8 +26,10 @@ add_file () {
echo $name $name
git add $name
test_tick
- msg_added_iso88591=$(echo Add $name ($added $name) |
iconv -f utf-8 -t $test_encoding)
- git -c i18n.commitEncoding=$test_encoding commit -m
$msg_added_iso88591
+ # git commit -m would break MinGW, as Windows refuse
to pass
+ # $test_encoding encoded parameter to git.
+ echo Add $name ($added $name) | iconv -f utf-8 -t
$test_encoding |
+ git -c i18n.commitEncoding=$test_encoding commit -F -
done /dev/null
git rev-parse --short --verify HEAD
)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index c84ec9a..349c531 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -31,7 +31,7 @@ test_expect_success 'set up basic repos' '
git add foo
test_tick
git config i18n.commitEncoding $test_encoding
- git commit -m $(commit_msg $test_encoding)
+ commit_msg $test_encoding | git commit -F -
git add bar
test_tick
git commit -m add bar
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 88ed319..a02a45a 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -35,13 +35,13 @@ test_expect_success 'setup' '
: foo
git add foo
git config i18n.commitEncoding $test_encoding
- git commit -m $added_iso88591
+ echo $added_iso88591 | git commit -F -
head1=$(git rev-parse --verify HEAD)
head1_short=$(git rev-parse --verify --short $head1)
tree1=$(git rev-parse --verify HEAD:)
tree1_short=$(git rev-parse --verify --short $tree1)
echo $changed foo
- git commit -a -m $changed_iso88591
+ echo $changed_iso88591 | git commit -a -F -
head2=$(git rev-parse --verify HEAD)
head2_short=$(git rev-parse --verify --short $head2)
tree2=$(git rev-parse --verify HEAD:)
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index ee703be..98bcfe2 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -44,7 +44,9 @@ test_expect_success 'creating initial files and commits' '
echo 1st line 2nd file secondfile
echo 2nd line 2nd file secondfile
- git -c i18n.commitEncoding=$test_encoding commit -a -m $(commit_msg
$test_encoding)
+ # git commit -m would break MinGW, as Windows refuse to pass
+ # $test_encoding encoded parameter to git.
+ commit_msg $test_encoding | git -c i18n.commitEncoding=$test_encoding
commit -a -F -
head5=$(git rev-parse --verify HEAD)
'
# git log --pretty=oneline # to see those SHA1 involved
@@ -334,7 +336,9 @@ test_expect_success 'redoing the last two commits should
succeed' '
echo 1st line 2nd file secondfile
echo 2nd line 2nd file secondfile
- git -c i18n.commitEncoding=$test_encoding commit -a -m $(commit_msg
$test_encoding)
+ # git commit -m would break MinGW, as Windows refuse to pass
+ # $test_encoding encoded parameter to git.
+ commit_msg $test_encoding | git -c i18n.commitEncoding=$test_encoding
commit -a -F -
check_changes $head5
'
--
2.0.0.9635.g0be03cb
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] Win32: Unicode file name support (except dirent)
From: Karsten Blees bl...@dcon.de
Date: Thu, 15 Mar 2012 18:21:28 +0100
Replaces Windows ANSI APIs dealing with file- or path names with their
Unicode equivalent, adding UTF-8/UTF-16LE conversion as necessary.
The dirent API (opendir/readdir/closedir) is updated in a separate commit.
Adds trivial wrappers for access, chmod and chdir.
Adds wrapper for mktemp (needed for both mkstemp and mkdtemp).
The simplest way to convert a repository with legacy-encoded (e.g. Cp1252)
file names to UTF-8 ist to checkout with an old msysgit version and
git add --all git commit with the new version.
Includes a fix for bug reported by John Chen:
On Windows XP (not Win7), directories cannot be deleted while a find handle
is open, causing Deletion of directory '...' failed. Should I try again?
prompts.
Prior to this commit, these failures were silently ignored due to
strbuf_free in is_dir_empty resetting GetLastError to ERROR_SUCCESS.
Close the find handle in is_dir_empty so that git doesn't block deletion
of the directory even after all other applications have released it.
Reported-by: John Chen john0...@gmail.com
Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Stepan Kasal ka...@ucw.cz
---
compat/mingw.c | 198 ++---
compat/mingw.h | 18 --
2 files changed, 160 insertions(+), 56 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 3baaa4d..c19e3d9 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1,6 +1,7 @@
#include ../git-compat-util.h
#include win32.h
#include conio.h
+#include wchar.h
#include ../strbuf.h
#include ../run-command.h
@@ -198,14 +199,16 @@ static int ask_yes_no_if_possible(const char *format, ...)
}
}
-#undef unlink
int mingw_unlink(const char *pathname)
{
int ret, tries = 0;
+ wchar_t wpathname[MAX_PATH];
+ if (xutftowcs_path(wpathname, pathname) 0)
+ return -1;
/* read-only files cannot be removed */
- chmod(pathname, 0666);
- while ((ret = unlink(pathname)) == -1 tries ARRAY_SIZE(delay)) {
+ _wchmod(wpathname, 0666);
+ while ((ret = _wunlink(wpathname)) == -1 tries ARRAY_SIZE(delay)) {
if (!is_file_in_use_error(GetLastError()))
break;
/*
@@ -221,45 +224,45 @@ int mingw_unlink(const char *pathname)
while (ret == -1 is_file_in_use_error(GetLastError())
ask_yes_no_if_possible(Unlink of file '%s' failed.
Should I try again?, pathname))
- ret = unlink(pathname);
+ ret = _wunlink(wpathname);
return ret;
}
-static int is_dir_empty(const char *path)
+static int is_dir_empty(const wchar_t *wpath)
{
- struct strbuf buf = STRBUF_INIT;
- WIN32_FIND_DATAA findbuf;
+ WIN32_FIND_DATAW findbuf;
HANDLE handle;
-
- strbuf_addf(buf, %s\\*, path);
- handle = FindFirstFileA(buf.buf, findbuf);
- if (handle == INVALID_HANDLE_VALUE) {
- strbuf_release(buf);
+ wchar_t wbuf[MAX_PATH + 2];
+ wcscpy(wbuf, wpath);
+ wcscat(wbuf, L\\*);
+ handle = FindFirstFileW(wbuf, findbuf);
+ if (handle == INVALID_HANDLE_VALUE)
return GetLastError() == ERROR_NO_MORE_FILES;
- }
- while (!strcmp(findbuf.cFileName, .) ||
- !strcmp(findbuf.cFileName, ..))
- if (!FindNextFile(handle, findbuf)) {
- strbuf_release(buf);
- return GetLastError() == ERROR_NO_MORE_FILES;
+ while (!wcscmp(findbuf.cFileName, L.) ||
+ !wcscmp(findbuf.cFileName, L..))
+ if (!FindNextFileW(handle, findbuf)) {
+ DWORD err = GetLastError();
+ FindClose(handle);
+ return err == ERROR_NO_MORE_FILES;
}
FindClose(handle);
- strbuf_release(buf);
return 0;
}
-#undef rmdir
int mingw_rmdir(const char *pathname)
{
int ret, tries = 0;
+ wchar_t wpathname[MAX_PATH];
+ if (xutftowcs_path(wpathname, pathname) 0)
+ return -1;
- while ((ret = rmdir(pathname)) == -1 tries ARRAY_SIZE(delay)) {
+ while ((ret = _wrmdir(wpathname)) == -1 tries ARRAY_SIZE(delay)) {
if (!is_file_in_use_error(GetLastError()))
errno = err_win_to_posix(GetLastError());
if (errno != EACCES)
break;
- if (!is_dir_empty(pathname)) {
+ if (!is_dir_empty(wpathname)) {
errno = ENOTEMPTY;
break;
}
@@ -276,16 +279,26 @@ int mingw_rmdir(const char *pathname)
while (ret == -1 errno == EACCES
is_file_in_use_error(GetLastError())
ask_yes_no_if_possible(Deletion of directory '%s' failed.
Should I
[PATCH] .gitignore: Add git-verify-commit
builtin/verify-commit.c was added in commit d07b00b (verify-commit: scriptable commit signature verification, 2014-06-23), update .gitignore to ignore the generated file. Signed-off-by: Øyvind A. Holm su...@sunbase.org --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 42294e5..edb1dcf 100644 --- a/.gitignore +++ b/.gitignore @@ -165,6 +165,7 @@ /git-upload-archive /git-upload-pack /git-var +/git-verify-commit /git-verify-pack /git-verify-tag /git-web--browse -- 2.0.1.563.g66f467c -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] fix test suite with mingw-unicode patches
Hello, I'm sorry that I have to reply to my own mail, but I forgot this: Karsten Blees (2): Win32: Unicode file name support (except dirent) .. has this one squashed in: Win32: fix detection of empty directories in is_dir_empty https://github.com/msysgit/git/commit/91db148 Win32: Unicode file name support (dirent) Both of theese patches are in msysgit for more than 2 years. Pat Thoyts and Stepan Kasal(1): tests: do not pass iso8859-1 encoded parameter This one is relatively new: replaces git commit -m by git commit -F - to work around a Windows bug^H^H^Hfeature. Stepan -- 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 v9 0/3] git config cache special querying api utilizing the cache
Hi, [PATCH v9]: Applied most of the review comments mentioned in [11]. Mostly asthetic changes. test-config now clears the config_set before exiting. Most of the tests now use the check-config function. check_config_init() now handles return values correctly. Diff between v8 and v9 is at the bottom. Thanks to Junio and Matthieu for the review. [PATCH V8]: Moved the contents of config-set.c to config.c for future convenience. Reverted test 'find value with misspelled key' to the one in v5. See [10] for the discussion. [PATCH V7]: Style nits and a broken chain corrected in `t/t1308-config-set.sh`. See [9] for the nits. [PATCH V6]: Style nits and mistakes corrected. Diff between v6 and v5[8] is at the bottom. Thanks to Matthieu, Ramsay and Ram for their suggestions. [PATCH V5]: `config_set` now uses a single hashmap. Corrected style nits raised in the thread[7]. Thanks to Junio and Matthieu for their suggestions. [PATCH v4]: Introduced `config_set` construct which points to a ordered set of config-files cached as hashmaps. Added relevant API functions. For more details see the documentation. Rewrote the git_config_get* family to use `config_set` internally. Added tests for both config_set API and git_config_get family. Added type specific API functions which parses the found value and converts it into a specific type. Most of the changes implemented are the result of discussion in [6]. Thanks to Eric, Ramsay, Junio, Matthieu Karsten for their suggestions and review. [PATCH v3]: Added flag for NULL values that were causing segfaults in some cases. Added test-config for usage examples. Minor changes and corrections. Refer to discussion thread[5] for more details. Thanks to Matthieu, Jeff and Junio for their valuable suggestions. [PATCH v2]:Changed the string_list to a struct instead of pointer to a struct. Added string-list initilization functions. Minor mistakes corrected acoording to review comments[4]. Thanks to Eric and Matthieu for their review. [PATCH V1]:Most of the invaluable suggestions by Eric Sunshine, Torsten Bogershausen and Jeff King has been implemented[1]. Complete rewrite of config_cache*() family using git_config() as hook as suggested by Jeff. Thanks for the review. [RFC V2]: Improved according to the suggestions by Eric Sunshine and Torsten Bogershausen. Added cache invalidation when config file is changed.[2] I am using git_config_set_multivar_in_file() as an update hook. This is patch is for this year's GSoC. My project is Git Config API improvements. The link of my proposal is appended below [3]. The aim of this patch series is to generate a cache for querying values from the config files in a non-callback manner as the current method reads and parses the config files every time a value is queried for. The cache is generated from hooking the update_cache function to the current parsing and callback mechanism in config.c. It is implemented as an hashmap using the hashmap-api with variables and its corresponding values list as its members. The values in the list are sorted in order of increasing priority. The cache is initialised the first time when any of the new query functions is called. It is invalidated by using git_config_set_multivar_in_file() as an update hook. We add two new functions to the config-api, git_config_get_string() and git_config_get_string_multi() for querying in a non callback manner from the cache. [1] http://marc.info/?t=14017206626r=1w=2 [2] http://git.661346.n2.nabble.com/RFC-PATCH-0-2-Git-config-cache-amp-special-querying-api-utilizing-the-cache-td7611691.html [3] https://drive.google.com/file/d/0B4suZ-aHqDcnSUZJRXVTTnZUN1E/edit?usp=sharing [4] http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251369 [5] http://thread.gmane.org/gmane.comp.version-control.git/251704/ [6] http://thread.gmane.org/gmane.comp.version-control.git/252329/ [7] http://marc.info/?t=14042811521r=1w=2 [8] http://article.gmane.org/gmane.comp.version-control.git/252942/ [9] http://thread.gmane.org/gmane.comp.version-control.git/252959/ [10] http://article.gmane.org/gmane.comp.version-control.git/253113 [11] http://thread.gmane.org/gmane.comp.version-control.git/253248 Tanay Abhra (2): config set test-config .gitignore | 1 + Documentation/technical/api-config.txt | 135 Makefile | 1 + cache.h| 31 config.c | 276 + t/t1308-config-set.sh | 212 + test-config.c | 140 + 7 files changed, 796 insertions(+) create mode 100755 t/t1308-config-set.sh
[PATCH v9 2/2] test-config: add tests for the config_set API
Expose the `config_set` C API as a set of simple commands in order to
facilitate testing. Add tests for the `config_set` API as well as for
`git_config_get_*()` family for the usual config files.
Signed-off-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
.gitignore| 1 +
Makefile | 1 +
t/t1308-config-set.sh | 212 ++
test-config.c | 140 +
4 files changed, 354 insertions(+)
create mode 100755 t/t1308-config-set.sh
create mode 100644 test-config.c
diff --git a/.gitignore b/.gitignore
index 42294e5..7677533 100644
--- a/.gitignore
+++ b/.gitignore
@@ -177,6 +177,7 @@
/gitweb/static/gitweb.min.*
/test-chmtime
/test-ctype
+/test-config
/test-date
/test-delta
/test-dump-cache-tree
diff --git a/Makefile b/Makefile
index b92418d..e070eb8 100644
--- a/Makefile
+++ b/Makefile
@@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
new file mode 100755
index 000..94085eb
--- /dev/null
+++ b/t/t1308-config-set.sh
@@ -0,0 +1,212 @@
+#!/bin/sh
+
+test_description='Test git config-set API in different settings'
+
+. ./test-lib.sh
+
+# 'check_config get_* section.key value' verifies that the entry for
+# section.key is 'value'
+check_config () {
+ case $1 in
+ expect_code)
+ if [ $# -lt 5 ];
+ then
+ expect
+ else
+ printf %s\n $5 expect
+ fi
+ test_expect_code $2 test-config $3 $4 actual
+ ;;
+ *)
+ op=$1 key=$2
+ shift
+ shift
+ printf %s\n $@ expect
+ test-config $op $key actual
+ ;;
+ esac
+ test_cmp expect actual
+}
+
+test_expect_success 'setup default config' '
+ cat .git/config \EOF
+ [case]
+ penguin = very blue
+ Movie = BadPhysics
+ UPPERCASE = true
+ MixedCase = true
+ my =
+ foo
+ baz = sam
+ [Cores]
+ WhatEver = Second
+ baz = bar
+ [cores]
+ baz = bat
+ [CORES]
+ baz = ball
+ [my Foo bAr]
+ hi = mixed-case
+ [my FOO BAR]
+ hi = upper-case
+ [my foo bar]
+ hi = lower-case
+ [case]
+ baz = bat
+ baz = hask
+ [lamb]
+ chop = 65
+ head = none
+ [goat]
+ legs = 4
+ head = true
+ skin = false
+ nose = 1
+ horns
+ EOF
+'
+
+test_expect_success 'get value for a simple key' '
+ check_config get_value case.penguin very blue
+'
+
+test_expect_success 'get value for a key with value as an empty string' '
+ check_config get_value case.my
+'
+
+test_expect_success 'get value for a key with value as NULL' '
+ check_config get_value case.foo (NULL)
+'
+
+test_expect_success 'upper case key' '
+ check_config get_value case.UPPERCASE true
+ check_config get_value case.uppercase true
+'
+
+test_expect_success 'mixed case key' '
+ check_config get_value case.MixedCase true
+ check_config get_value case.MIXEDCASE true
+ check_config get_value case.mixedcase true
+'
+
+test_expect_success 'key and value with mixed case' '
+ check_config get_value case.Movie BadPhysics
+'
+
+test_expect_success 'key with case sensitive subsection' '
+ check_config get_value my.Foo bAr.hi mixed-case
+ check_config get_value my.FOO BAR.hi upper-case
+ check_config get_value my.foo bar.hi lower-case
+'
+
+test_expect_success 'key with case insensitive section header' '
+ check_config get_value cores.baz ball
+ check_config get_value Cores.baz ball
+ check_config get_value CORES.baz ball
+ check_config get_value coreS.baz ball
+'
+
+test_expect_success 'key with case insensitive section header variable' '
+ check_config get_value CORES.BAZ ball
+ check_config get_value cores.baz ball
+ check_config get_value cores.BaZ ball
+ check_config get_value cOreS.bAz ball
+'
+
+test_expect_success 'find value with misspelled key' '
+ check_config expect_code 1 get_value my.fOo Bar.hi Value not found
for \my.fOo Bar.hi\
+'
+
+test_expect_success 'find value with the highest priority' '
+ check_config get_value case.baz hask
+'
+
+test_expect_success 'find integer value for a key' '
+ check_config get_int
[PATCH v9 1/2] add `config_set` API for caching config-like files
Currently `git_config()` uses a callback mechanism and file rereads for config values. Due to this approach, it is not uncommon for the config files to be parsed several times during the run of a git program, with different callbacks picking out different variables useful to themselves. Add a `config_set`, that can be used to construct an in-memory cache for config-like files that the caller specifies (i.e., files like `.gitmodules`, `~/.gitconfig` etc.). Add two external functions `git_configset_get_value` and `git_configset_get_value_multi` for querying from the config sets. `git_configset_get_value` follows `last one wins` semantic (i.e. if there are multiple matches for the queried key in the files of the configset the value returned will be the last entry in `value_list`). `git_configset_get_value_multi` returns a list of values sorted in order of increasing priority (i.e. last match will be at the end of the list). Add type specific query functions like `git_configset_get_bool` and similar. Add a default `config_set`, `the_config_set` to cache all key-value pairs read from usual config files (repo specific .git/config, user wide ~/.gitconfig, XDG config and the global /etc/gitconfig). `the_config_set` is populated using `git_config()`. Add two external functions `git_config_get_value` and `git_config_get_value_multi` for querying in a non-callback manner from `the_config_set`. Also, add type specific query functions that are implemented as a thin wrapper around the `config_set` API. Signed-off-by: Matthieu Moy matthieu@imag.fr Signed-off-by: Tanay Abhra tanay...@gmail.com --- Documentation/technical/api-config.txt | 135 cache.h| 31 config.c | 276 + 3 files changed, 442 insertions(+) diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt index 230b3a0..48255a2 100644 --- a/Documentation/technical/api-config.txt +++ b/Documentation/technical/api-config.txt @@ -77,6 +77,81 @@ To read a specific file in git-config format, use `git_config_from_file`. This takes the same callback and data parameters as `git_config`. +Querying For Specific Variables +--- + +For programs wanting to query for specific variables in a non-callback +manner, the config API provides two functions `git_config_get_value` +and `git_config_get_value_multi`. They both read values from an internal +cache generated previously from reading the config files. + +`int git_config_get_value(const char *key, const char **value)`:: + + Finds the highest-priority value for the configuration variable `key`, + stores the pointer to it in `value` and returns 0. When the + configuration variable `key` is not found, returns 1 without touching + `value`. The caller should not free or modify `value`, as it is owned + by the cache. + +`const struct string_list *git_config_get_value_multi(const char *key)`:: + + Finds and returns the value list, sorted in order of increasing priority + for the configuration variable `key`. When the configuration variable + `key` is not found, returns NULL. The caller should not free or modify + the returned pointer, as it is owned by the cache. + +`void git_config_clear(void)`:: + + Resets and invalidates the config cache. + +The config API also provides type specific API functions which do conversion +as well as retrieval for the queried variable, including: + +`int git_config_get_int(const char *key, int *dest)`:: + + Finds and parses the value to an integer for the configuration variable + `key`. Dies on error; otherwise, stores the value of the parsed integer in + `dest` and returns 0. When the configuration variable `key` is not found, + returns 1 without touching `dest`. + +`int git_config_get_ulong(const char *key, unsigned long *dest)`:: + + Similar to `git_config_get_int` but for unsigned longs. + +`int git_config_get_bool(const char *key, int *dest)`:: + + Finds and parses the value into a boolean value, for the configuration + variable `key` respecting keywords like true and false. Integer + values are converted into true/false values (when they are non-zero or + zero, respectively). Other values cause a die(). If parsing is successful, + stores the value of the parsed result in `dest` and returns 0. When the + configuration variable `key` is not found, returns 1 without touching + `dest`. + +`int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)`:: + + Similar to `git_config_get_bool`, except that integers are copied as-is, + and `is_bool` flag is unset. + +`int git_config_get_maybe_bool(const char *key, int *dest)`:: + + Similar to `git_config_get_bool`, except that it returns -1 on error + rather than dying. + +`int
Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig
On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote: I realize that I am reinventing the error-reporting wheel on a sleepy Sunday afternoon without having thought about it much, so there is probably some gotcha or case that makes this ugly, or perhaps it just ends up verbose in practice. But one can dream. Just for fun... Yes, that is fun. I actually think your In 'version:pefname' and 'wersion:refname', we want be able to report 'pefname' and 'wersion' are misspelled, and returning -1 or enum would not cut it is a good argument. The callee wants to have flexibility on _what_ to report, just as the caller wants to have flexibility on _how_. In this particular code path, I think the former far outweighs the latter, and my suggestion I called silly might not be so silly but may have struck the right balance. I dunno. If you absolutely need to have both, you would need something like your approach, of course, but I am not sure if it is worth it. I am not sure how well this meshes with i18n (I know the for fun does not even attempt to, but if we tried to, I suspect it may become even uglier). We would also need to override both error and warning routines and have the reporter tag the errors in these two categories, no? Do we want to go this way? Should I respin my patch (again)? I'm not sure we really need to get that complex.. I do like parsing errors a bit cleaner and indicating what part is bad.. Note that our current parsing method does not make it really possible to indicate which part is wrong. Thanks, Jake
Re: [PATCH v9 1/2] add `config_set` API for caching config-like files
Tanay Abhra tanay...@gmail.com writes:
diff --git a/config.c b/config.c
index ba882a1..89e2d67 100644
--- a/config.c
+++ b/config.c
...
+const struct string_list *git_configset_get_value_multi(struct config_set
*cs, const char *key)
+{
+ struct config_set_element *e = configset_find_element(cs, key);
+ return e ? e-value_list : NULL;
+}
Somehow I find the placement of this function out of place. Didn't
you get the same impression while proofreading the entire file after
you are done writing this patch?
The right place for it would probably be immediately before
git_configset_get_value(), I would think.
+int git_configset_add_file(struct config_set *cs, const char *filename)
+{
+ int ret = 0;
+ ret = git_config_from_file(config_hash_callback, filename, cs);
+ if (!ret)
+ return 0;
+ else {
+ git_configset_clear(cs);
+ return -1;
+ }
+}
If I add contents from file A successfully and then attempt to
further add contents from file B which fails, I would lose
contents I read from A?
It would not be worth trying to make it transactional (i.e. making
sure cs contains what was there before the config-from-file was
called if it failed), so in such a case we may end up seeing a
mixture of full contents from A and partial contents from B, which
is just as useless as a cleared configset, so it is not wrong to
clear it per-se. An alternative might be to return without
clearing, as we are leaving the configset in a useless state either
way and give the caller a choice between clearing it and continue,
and dying without even spending unnecessary cycles to clear. That
might be more consistent with what git_config_check_init() does,
where you die without bothering to clear what was half-read into the
configset.
Whatever behaviour is chosen in the error codepath, it needs to be
documented.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/2] test-config: add tests for the config_set API
Tanay Abhra tanay...@gmail.com writes:
Expose the `config_set` C API as a set of simple commands in order to
facilitate testing. Add tests for the `config_set` API as well as for
`git_config_get_*()` family for the usual config files.
Signed-off-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
.gitignore| 1 +
Makefile | 1 +
t/t1308-config-set.sh | 212
++
test-config.c | 140 +
4 files changed, 354 insertions(+)
create mode 100755 t/t1308-config-set.sh
create mode 100644 test-config.c
diff --git a/.gitignore b/.gitignore
index 42294e5..7677533 100644
--- a/.gitignore
+++ b/.gitignore
@@ -177,6 +177,7 @@
/gitweb/static/gitweb.min.*
/test-chmtime
/test-ctype
+/test-config
/test-date
/test-delta
/test-dump-cache-tree
diff --git a/Makefile b/Makefile
index b92418d..e070eb8 100644
--- a/Makefile
+++ b/Makefile
@@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
new file mode 100755
index 000..94085eb
--- /dev/null
+++ b/t/t1308-config-set.sh
@@ -0,0 +1,212 @@
+#!/bin/sh
+
+test_description='Test git config-set API in different settings'
+
+. ./test-lib.sh
+
+# 'check_config get_* section.key value' verifies that the entry for
+# section.key is 'value'
+check_config () {
+ case $1 in
+ expect_code)
+ if [ $# -lt 5 ];
+ then
Spell out test and drop unnecessary semicolon, i.e.
if test $# -lt 5
then
+ expect
+ else
+ printf %s\n $5 expect
The other expecting success side of this function allows to expect
more than one line of output, but this one only allows you to expect
at most one line? Why?
+ fi
+ test_expect_code $2 test-config $3 $4 actual
+ ;;
+ *)
+ op=$1 key=$2
+ shift
+ shift
+ printf %s\n $@ expect
+ test-config $op $key actual
+ ;;
+ esac
+ test_cmp expect actual
Perhaps you meant to say something like this instead?
if test $1 = expect_code
then
expect_code=$2 shift shift
else
expect_code=0
fi
op=$1 key=$2 shift shift
if test $# != 0
then
printf %s\n $@
fi expect
test_expect_code $expet_code test-config $op $key actual
test_cmp expect actual
I dunno.
+test_expect_success 'setup default config' '
+ cat .git/config \EOF
So the default .git/config that was prepared by git init is
discarded and replaced with this? Shouldn't it be
cat .git/config \EOF
instead?
+test_expect_success 'find multiple values' '
+ cat expect -\EOF
+ sam
+ bat
+ hask
+ EOF
+ test-config get_value_multi case.bazactual
+ test_cmp expect actual
+'
Hmmm, wasn't the whole point of the helper to allow us to make
things like the above into a one-liner, perhaps like this?
check_config get_value_multi case.baz sam bat hask
I suspect the same applies to most if not all uses of test-config
in the remainder of this patch.
--
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 3/3] tag: support configuring --sort via .gitconfig
Keller, Jacob E jacob.e.kel...@intel.com writes: On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote: I realize that I am reinventing the error-reporting wheel on a sleepy Sunday afternoon without having thought about it much, so there is probably some gotcha or case that makes this ugly, or perhaps it just ends up verbose in practice. But one can dream. Just for fun... Yes, that is fun. I actually think your In 'version:pefname' and 'wersion:refname', we want be able to report 'pefname' and 'wersion' are misspelled, and returning -1 or enum would not cut it is a good argument. The callee wants to have flexibility on _what_ to report, just as the caller wants to have flexibility on _how_. In this particular code path, I think the former far outweighs the latter, and my suggestion I called silly might not be so silly but may have struck the right balance. I dunno. If you absolutely need to have both, you would need something like your approach, of course, but I am not sure if it is worth it. I am not sure how well this meshes with i18n (I know the for fun does not even attempt to, but if we tried to, I suspect it may become even uglier). We would also need to override both error and warning routines and have the reporter tag the errors in these two categories, no? Do we want to go this way? I do not speak for Peff, but I personally think this is just a fun demonstration, nothing more, and my gut feeling is that it would make things unnecessary complex without much real gain to pursue it further. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/2] add `config_set` API for caching config-like files
On 7/15/2014 9:16 PM, Junio C Hamano wrote:
Tanay Abhra tanay...@gmail.com writes:
diff --git a/config.c b/config.c
index ba882a1..89e2d67 100644
--- a/config.c
+++ b/config.c
...
+const struct string_list *git_configset_get_value_multi(struct config_set
*cs, const char *key)
+{
+struct config_set_element *e = configset_find_element(cs, key);
+return e ? e-value_list : NULL;
+}
Somehow I find the placement of this function out of place. Didn't
you get the same impression while proofreading the entire file after
you are done writing this patch?
The right place for it would probably be immediately before
git_configset_get_value(), I would think.
Noted. My fault, will correct in the reroll.
+int git_configset_add_file(struct config_set *cs, const char *filename)
+{
+int ret = 0;
+ret = git_config_from_file(config_hash_callback, filename, cs);
+if (!ret)
+return 0;
+else {
+git_configset_clear(cs);
+return -1;
+}
+}
If I add contents from file A successfully and then attempt to
further add contents from file B which fails, I would lose
contents I read from A?
It would not be worth trying to make it transactional (i.e. making
sure cs contains what was there before the config-from-file was
called if it failed), so in such a case we may end up seeing a
mixture of full contents from A and partial contents from B, which
is just as useless as a cleared configset, so it is not wrong to
clear it per-se. An alternative might be to return without
clearing, as we are leaving the configset in a useless state either
way and give the caller a choice between clearing it and continue,
and dying without even spending unnecessary cycles to clear. That
might be more consistent with what git_config_check_init() does,
where you die without bothering to clear what was half-read into the
configset.
Whatever behaviour is chosen in the error codepath, it needs to be
documented.
Since syntactical errors in reading the file will cause a die when we
use either `git_config_from_file` or `git_config`, I don't think
incomplete parsing would trigger the error path, as it will die before
reaching there.
So, the best way would be just to return without clearing and let the
user take the decision if he wants to go on with the incomplete
config_set or clear it when he sees that configset_add_file() failed.
Die in git_config_check_init() is never triggered expect in rare race
conditions like between access_or_die() and git_config_from_file() in
if (git_config_system() !access_or_die(git_etc_gitconfig(), R_OK,
0)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
data);
found += 1;
}
which I think would never happen in reality, dunno. I think I will take
out the die() in git_config_check_init(). Thus, the behavior of both
functions git_config_check_init() git_configset_add_file will be consistent.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[no subject]
Hi, I have tried some methods introduced in the network, but always failed. Some big files committed by me to a very old branch then the files deleted and new branches were continuously created. Now the checkout directory has grown to about 80 megabytes. What's the right way to permenently erase those garbage big files? Thanks in advance. - Woody Wu from mobile phone -- 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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
On Tue, Jul 8, 2014 at 8:02 AM, Michael Haggerty mhag...@alum.mit.edu wrote:
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
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.
If lock_ref_sha1_basic fails the check_refname_format test, set errno to
EINVAL before returning NULL. This to guarantee that we will not return an
error without updating errno.
This leaves lock_any_ref_for_updates as a no-op wrapper which could be
removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.
This changes semantics for lock_ref_sha1_basic slightly. With this change
it is no longer possible to open a ref that has a badly name which breaks
s/badly name/bad name,/
any codepaths that tries to open and repair badly named refs. The normal refs
s/tries/try/
API should not allow neither creating nor accessing refs with invalid names.
s/not allow neither/allow neither/
If we need such recovery code we could add it as an option to git fsck and
have
git fsck be the only sanctioned way of bypassing the normal API and checks.
I like the sentiment, but in the real world I'm not sure we can take
such a step based only on good intentions. Which callers would be
affected? Where is this git fsck code that would be needed to help
people rescue their repos?
I think the repair things are already busted since a while, so I don't
think this will make things worse,
but I will change it to make it better than this patch and better than
current master, please see below.
current git
$ 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.
git fsck does report that there is a problem with a broken ref
fatal: Reference has invalid format: 'refs/heads/master.@*@\.'
But I don't think there is any way to fix this other than manually
deleting the file from the command line.
(this is because we need to do a resolve_ref_unsafe which will not
work and if we can not do a resolve_ref_unsafe we can not delete the
ref,
there is also the issue where we can not read the ref into ref-cache)
What I suggest doing here is to create a new patch towards the end of
the series that will :
* change the resolve_ref_unsafe(... , int reading, ...) argument to be
a bitmask of flags with
#define RESOLVE_REF_READING 0x01 (== current flag)
#define RESOLVE_REF_ALLOW_BAD_NAME 0x02 (== disable checking the
refname format during resolve)
* then provide the flag for RESOLVE_REF_ALLOW_BAD_NAME for the cases
where we try to resolve a ref in builtin/branch.c where we try to
delete a ref
* then also pass the same flag to lock_ref_sha1_basic when we are
deleting a ref from transaction_commit so we can disable the check
refname check there too.
I think that is all that is needed but I will see if there are
additional changes needed once I implement it.
This will mean that the semantics will become :
* you can not create or access a branch with a bad name since both
resolving it and locking it will fail.
* but you can always delete a branch regardless of whether the name is
good or bad.
(this will also mean that you will be able to rename a bad branch name
to a new good name)
which I think would be pretty sane semantics.
I will implement these changes as a separate patch.
Comments?
regards
ronnie sahlberg
I can also imagine that we will tighten up the check_refname_format
checks in the future; for example, I think it would be a good idea to
prohibit reference names that start with '-' because it is almost
impossible to work with them (their names look like command-line
options). If we ever make a change like that, we will need some amount
of tolerance in git versions around the transition.
So...I like the idea of enforcing refname checks at the lowest level
possible, but I think that the change you propose is too abrupt. I
think it needs either more careful analysis showing that it won't hurt
anybody, or some kind of tooling or non-strict mode that people can use
to fix their repositories.
Michael
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 389a55f..bccf8c3 100644
--- a/refs.c
+++ b/refs.c
@@ -2088,6 +2088,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;
@@ -2179,8 +2184,6
Big repository cannot be reduced
Hi, I have tried some methods introduced in the network, but always failed. Some big files committed by me to a very old branch then the files deleted and new branches were continuously created. Now the checkout directory has grown to about 80 megabytes. What's the right way to permenently erase those garbage big files? Thanks in advance. - Woody Wu from mobile phone -- 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 v8 4/4] cache-tree: Write updated cache-tree after commit
Duy Nguyen pclo...@gmail.com writes: On Mon, Jul 14, 2014 at 11:38:06PM -0700, Junio C Hamano wrote: On Mon, Jul 14, 2014 at 7:15 PM, Duy Nguyen pclo...@gmail.com wrote: It makes me wonder if a cleaner way of rebuilding cache-treei in this case is from git-add--interactive.perl, or by simply spawn git update-index --rebuild-cache-tree after running git-add--interactive.perl. We could check if the cache-tree has fully been populated by add -i and limit the rebuilding by git commit -p main process, but if add -i did not do so, there is no reason why git commit -p should not do so, without relying on the implementation of add -i to do so. At least to me, what you suggested sounds nothing more than a cop-out; instead of lifting the limitation of the API by enhancing it with reopen-lock-file, punting to shift the burden elsewhere. I am not quite sure why that is cleaner, but perhaps I am missing something. Reopen-lock-file to me does not sound like an enhancement, at least in the index update context. We have always updated the index by writing to a new file, then rename. Time to step back and think, I think. What is the real point of writing into *.lock and renaming? It serves two purposes: (1) everybody adheres to that convention---if we managed to take the lock index.lock, nobody else will compete and interfere with us until we rename it to index. (2) in the meantime, others can still read the old contents in the original index. With take lock, write to a temporary, commit by rename or rollback by delete, we can have the usual transactional semantics. While we are working on it, nobody is allowed to muck with the same file, because everybody pays attention to *.lock. People will not see what we are doing until we release the lock because we are not writing into the primary file. And people can see what we did in its entirety once we are done because we close and rename to commit our changes atomically. Think what CLOSE_LOCK adds to that and you would appreciate its usefulness and at the same time realize its limitation. By allowing us to flush what we wrote to the disk without releasing the lock, we can give others (e.g. subprograms we spawn) controlled access to the new contents we prepared before we commit the result to the outside world. The access is controlled because we are in control when we tell these others to peek into or update the temporary file *.lock. The original implementaiton of CLOSE_LOCK is limited in that you can do so only once; you take a lock, you do your thing, you close, you let (one or more) others see it, and you commit (or rollback). You cannot do another of your thing once you close with the original implementation because there was no way to reopen. What do you gain by your proposal to lock index.lock file? We know we already have index.lock, so nobody should be competing on mucking with its contents with us and we gain nothing by writing into index.lock.lock and renaming it to index.lock. We are in total control of the lifetime of index.lock, when we spawn subprograms on it to let them write to it, when we ourselves write to it, when we spawn subprograms on it to let them read from it, all under the lock we have on the index, i.e. index.lock. The only thing use of another temporary file (that does not have to be a lock on index.lock, by the way, because we have total control over when and who updates the file while we hold the index.lock) would give you is that it allows you to make the success of the do another of your thing step optional. While holding the lock, you close and let add -i work on it, and after it returns, instead of reopening, you write into yet another index.lock.lock, expecting that it might fail and when it fails you can roll it back, leaving the contents add -i left in index.lock intact. If you do not use the extra temporary file, you start from index.lock left by add -i, write the updated index into index.lock and if you fail to write, you have to roll back the entire index---you lose the option to use the index left by add -i without repopulated cache-tree. But in the index update context, I do not think such a complexity is not necessary. If something fails, we should fail and roll back the entire index. Now if the new write_locked_index() blows up after reopen_lock_file(), we have no way but die() because index_lock.filename can no longer be trusted. If that is the case, lock.filename[] is getting clobbered by somebody too early, isn't it? I think Michael's mh/lockfile topic (dropped from my tree due to getting stale without rerolling) used a separate flag to indicate the validity without mucking with the filename member, and we may want to do something like that as the right solution to that problem. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/2] test-config: add tests for the config_set API
On 7/15/2014 9:27 PM, Junio C Hamano wrote: +test_expect_success 'setup default config' ' +cat .git/config \EOF So the default .git/config that was prepared by git init is discarded and replaced with this? Shouldn't it be cat .git/config \EOF instead? Most of tests like t1300-repo-config.sh or t1303-wacky-config.sh clears the default config, will it be okay to clear it in this test series also? I need it for test_expect_success 'proper error on error in default config files' ' which requires me to compare the line no at which the error was found. +test_expect_success 'find multiple values' ' +cat expect -\EOF +sam +bat +hask +EOF +test-config get_value_multi case.bazactual +test_cmp expect actual +' Hmmm, wasn't the whole point of the helper to allow us to make things like the above into a one-liner, perhaps like this? check_config get_value_multi case.baz sam bat hask Noted and corrected. I suspect the same applies to most if not all uses of test-config in the remainder of this patch. I cant use it for configset_get_value_* as it may have variable number of files as arguments. :) Thanks, Tanay Abhra. -- 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 3/3] tag: support configuring --sort via .gitconfig
On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote: Keller, Jacob E jacob.e.kel...@intel.com writes: On Mon, 2014-07-14 at 10:17 -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote: I realize that I am reinventing the error-reporting wheel on a sleepy Sunday afternoon without having thought about it much, so there is probably some gotcha or case that makes this ugly, or perhaps it just ends up verbose in practice. But one can dream. Just for fun... Yes, that is fun. I actually think your In 'version:pefname' and 'wersion:refname', we want be able to report 'pefname' and 'wersion' are misspelled, and returning -1 or enum would not cut it is a good argument. The callee wants to have flexibility on _what_ to report, just as the caller wants to have flexibility on _how_. In this particular code path, I think the former far outweighs the latter, and my suggestion I called silly might not be so silly but may have struck the right balance. I dunno. If you absolutely need to have both, you would need something like your approach, of course, but I am not sure if it is worth it. I am not sure how well this meshes with i18n (I know the for fun does not even attempt to, but if we tried to, I suspect it may become even uglier). We would also need to override both error and warning routines and have the reporter tag the errors in these two categories, no? Do we want to go this way? I do not speak for Peff, but I personally think this is just a fun demonstration, nothing more, and my gut feeling is that it would make things unnecessary complex without much real gain to pursue it further. I agree. But what about going back to the older setup where the caller can output correct error message? I'm ok with using an enum style return, to be completely honest. I would prefer this, actually. Thanks, Jake
Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
Michael Haggerty wrote: So...I like the idea of enforcing refname checks at the lowest level possible, but I think that the change you propose is too abrupt. I think it needs either more careful analysis showing that it won't hurt anybody, or some kind of tooling or non-strict mode that people can use to fix their repositories. The recovery code has been broken for a while, so I don't see harm in this change. How to take care of the recovery use case is another question. FWIW I also would prefer if git update-ref -d or git branch -D could be used to delete corrupt refs instead of having to use fsck (since a fsck run can take a while), but that's a question for a later series. In an ideal world, the low-level functions would allow *reading* and *deleting* poorly named refs (even without any special flag) but not creating them. Is that doable? The main complication I can see is iteration: would iteration skip poorly named refs and warn, or would something more complicated be needed? 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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
Ronnie Sahlberg wrote: What I suggest doing here is to create a new patch towards the end of the series that will : * change the resolve_ref_unsafe(... , int reading, ...) argument to be a bitmask of flags with #define RESOLVE_REF_READING 0x01 (== current flag) #define RESOLVE_REF_ALLOW_BAD_NAME 0x02 (== disable checking the refname format during resolve) * then provide the flag for RESOLVE_REF_ALLOW_BAD_NAME for the cases where we try to resolve a ref in builtin/branch.c where we try to delete a ref * then also pass the same flag to lock_ref_sha1_basic when we are deleting a ref from transaction_commit so we can disable the check refname check there too. Yeah, that sounds lovely. I wasn't able to reproduce a regression or convince myself there is one so I think it's okay if that happens in a separate series. But doing it now would be fine, 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 3/3] tag: support configuring --sort via .gitconfig
Keller, Jacob E jacob.e.kel...@intel.com writes: On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote: ... Yes, that is fun. I actually think your In 'version:pefname' and 'wersion:refname', we want be able to report 'pefname' and 'wersion' are misspelled, and returning -1 or enum would not cut it is a good argument. The callee wants to have flexibility on _what_ to report, just as the caller wants to have flexibility on _how_. In this particular code path, I think the former far outweighs the latter, and my suggestion I called silly might not be so silly but may have struck the right balance. I dunno. ... I agree. But what about going back to the older setup where the caller can output correct error message? I'm ok with using an enum style return, to be completely honest. I would prefer this, actually. Depends on which older setup you mean, I think. The one that does not let us easily give more context dependent diagnoses that lets us distinguish between version:pefname and version:refname by returning only -1 or an enum? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] fix test suite with mingw-unicode patches
Stepan Kasal ka...@ucw.cz writes: Hello Hannes, attached please find the patches that Karsten pointed out: 1) The unicode file name support was omitted from his unicode patch series; my mistake, sorry. There is still big part missing: support for unicode environment; I can only hope the tests would choke on that. 2) Windows cannot pass non-UTF parameters (commit messages in this case): original patch by Pat Thoyts was extended to apply to other similar cases: the commit msg is passed through stdin. If there are still problems remaining, please tell us. Thanks, Stepan Karsten Blees (2): Win32: Unicode file name support (except dirent) Win32: Unicode file name support (dirent) Pat Thoyts and Stepan Kasal(1): tests: do not pass iso8859-1 encoded parameter Thanks. I'll queue these and wait for Windows folks to respond. With favourable feedback they can go directly from pu to master, I would think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/2] test-config: add tests for the config_set API
Tanay Abhra tanay...@gmail.com writes: ... I need it for test_expect_success 'proper error on error in default config files' ' which requires me to compare the line no at which the error was found. I see. We may want to later rethink the way you validate the line numbers, but in the meantime I think the version posted should suffice, if you cannot do better than that. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig
On Tue, 2014-07-15 at 11:17 -0700, Junio C Hamano wrote:
Keller, Jacob E jacob.e.kel...@intel.com writes:
On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote:
...
Yes, that is fun.
I actually think your In 'version:pefname' and 'wersion:refname',
we want be able to report 'pefname' and 'wersion' are misspelled,
and returning -1 or enum would not cut it is a good argument. The
callee wants to have flexibility on _what_ to report, just as the
caller wants to have flexibility on _how_. In this particular code
path, I think the former far outweighs the latter, and my suggestion
I called silly might not be so silly but may have struck the right
balance. I dunno.
...
I agree. But what about going back to the older setup where the caller
can output correct error message? I'm ok with using an enum style
return, to be completely honest. I would prefer this, actually.
Depends on which older setup you mean, I think. The one that does
not let us easily give more context dependent diagnoses that lets us
distinguish between version:pefname and version:refname by returning
only -1 or an enum?
I am going to re-submit this with an enum-style return. I am also
changing how we parse so that we can correctly report whether the sort
function or sort atom is incorrect.
Thanks,
Jake
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
Jonathan Nieder jrnie...@gmail.com writes: How to take care of the recovery use case is another question. FWIW I also would prefer if git update-ref -d or git branch -D could be used to delete corrupt refs instead of having to use fsck (since a fsck run can take a while), but that's a question for a later series. Good thinking. In an ideal world, the low-level functions would allow *reading* and *deleting* poorly named refs (even without any special flag) but not creating them. Is that doable? The main complication I can see is iteration: would iteration skip poorly named refs and warn, or would something more complicated be needed? I somehow thought that was what we have always designed for, which DO_FOR_EACH_INCLUDE_BROKEN was a part of. -- 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 v1 1/3] dir.c: coding style fix
Am 15.07.2014 00:30, schrieb Junio C Hamano: Karsten Blees karsten.bl...@gmail.com writes: From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= pclo...@gmail.com Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com Signed-off-by: Karsten Blees bl...@dcon.de --- Thanks for forwarding. I'll fix-up the Yikes (see how these two lines show the same name in a very different way), but how did you produce the above? Is there some fix we need in the toolchain that produces patch e-mails? Hmmm...I simply thought that this is how its supposed to work. Mail headers can only contain US-ASCII, so the RFC 2047 Q-encoded-word generated by git-format-patch looked good to me. It seems git-mailinfo doesn't handle line breaks in header lines in the mail body. I.e. if you remove the LF in the 'From:' line, everything is fine, despite the Q-encoding. Now, 'git-format-patch --from' seems to work around the problem, but only for the 'From:' line. AFAICT there's no such option for 'Subject:', e.g. if you want to paste a patch after a scissors line, you're on your own (see the example in the git-format-patch(1) discussion section, with 'Subject:' both Q-encoded and line-wrapped). Perhaps it should be clarified that git-format-patch output is not suitable for pasting into mail clients? Or it should print headers in plain text and let git-send-email handle the conversions? -- 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 v1 1/3] dir.c: coding style fix
Karsten Blees karsten.bl...@gmail.com writes:
Am 15.07.2014 00:30, schrieb Junio C Hamano:
Karsten Blees karsten.bl...@gmail.com writes:
From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
pclo...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Signed-off-by: Karsten Blees bl...@dcon.de
---
Thanks for forwarding. I'll fix-up the Yikes (see how these two
lines show the same name in a very different way), but how did you
produce the above? Is there some fix we need in the toolchain that
produces patch e-mails?
Hmmm...I simply thought that this is how its supposed to work. Mail
headers can only contain US-ASCII, so the RFC 2047 Q-encoded-word
generated by git-format-patch looked good to me.
But that quoted one is *NOT* a mail header. It is the first line of
the payload of your message, and should be in plain text just like
the remainder, e.g. S-o-b line that has the same name.
Perhaps it should be clarified that git-format-patch output is not
suitable for pasting into mail clients? Or it should print headers
in plain text and let git-send-email handle the conversions?
If the former is missing, then we should definitely add it to the
documentation. We often see new people pasting the From line
meant for /etc/magic and unwanted {From,Subject,Date}: in the body.
We may also want to add an option to tell it to produce an output
that is suitable for pasting into mail clients. Hint, hint...
--
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 3/3] tag: support configuring --sort via .gitconfig
Junio C Hamano gits...@pobox.com writes: Keller, Jacob E jacob.e.kel...@intel.com writes: On Tue, 2014-07-15 at 09:03 -0700, Junio C Hamano wrote: ... Yes, that is fun. I actually think your In 'version:pefname' and 'wersion:refname', we want be able to report 'pefname' and 'wersion' are misspelled, and returning -1 or enum would not cut it is a good argument. The callee wants to have flexibility on _what_ to report, just as the caller wants to have flexibility on _how_. In this particular code path, I think the former far outweighs the latter, and my suggestion I called silly might not be so silly but may have struck the right balance. I dunno. ... I agree. But what about going back to the older setup where the caller can output correct error message? I'm ok with using an enum style return, to be completely honest. I would prefer this, actually. Depends on which older setup you mean, I think. The one that does not let us easily give more context dependent diagnoses that lets us distinguish between version:pefname and version:refname by returning only -1 or an enum? In case it was not clear, I do not think -1 or enum is a good idea. -- 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 3/3] tag: support configuring --sort via .gitconfig
Keller, Jacob E jacob.e.kel...@intel.com writes: I am going to re-submit this with an enum-style return. I am also changing how we parse so that we can correctly report whether the sort function or sort atom is incorrect. Oh, our mails crossed, I guess. As long as it will leave the door open for later enhancements for more context sensitive error diagnosis, I do not particularly mind a solution around enum. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] rebase--am: use --cherry-pick instead of --ignore-if-in-upstream
When using `git format-patch --ignore-if-in-upstream` we are only
allowed to give a single revision range. In the next commit we will
want to add an additional exclusion revision in order to handle fork
points correctly, so convert `git-rebase--am` to use a symmetric
difference with `--cherry-pick --right-only`.
This does not change the result of the format-patch invocation, just how
we spell the arguments.
Signed-off-by: John Keeping j...@keeping.me.uk
---
git-rebase--am.sh | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index ca20e1e..902bf2d 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -29,7 +29,13 @@ skip)
;;
esac
-test -n $rebase_root root_flag=--root
+if test -z $rebase_root
+ # this is now equivalent to ! -z $upstream
+then
+ revisions=$upstream...$orig_head
+else
+ revisions=$onto...$orig_head
+fi
ret=0
if test -n $keep_empty
@@ -38,14 +44,15 @@ then
# empty commits and even if it didn't the format doesn't really lend
# itself well to recording empty patches. fortunately, cherry-pick
# makes this easy
- git cherry-pick ${gpg_sign_opt:+$gpg_sign_opt} --allow-empty
$revisions
+ git cherry-pick ${gpg_sign_opt:+$gpg_sign_opt} --allow-empty \
+ --right-only $revisions
ret=$?
else
rm -f $GIT_DIR/rebased-patches
- git format-patch -k --stdout --full-index --ignore-if-in-upstream \
+ git format-patch -k --stdout --full-index --cherry-pick --right-only \
--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
- $root_flag $revisions $GIT_DIR/rebased-patches
+ $revisions $GIT_DIR/rebased-patches
ret=$?
if test 0 != $ret
--
2.0.1.472.g6f92e5f.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] sha1_file: do not add own object directory as alternate
Ephrim Khong dr.kh...@gmail.com writes:
@@ -320,7 +320,7 @@ static int link_alt_odb_entry(const char *entry, const
char *relative_base, int
return -1;
}
}
- if (!strcmp(ent-base, objdir)) {
+ if (!strcmp_icase(ent-base, normalized_objdir)) {
Not a problem with your patch, but we should rethink the name of
this function when the code base is more quiet. It always makes me
wonder if it is something similar to strcasecmp(), but in fact it is
not. It is meant to be used *only* for pathnames; pathname_cmp() or
something that has path in its name would be appropriate, but it
is wrong to call it str-anything.
@@ -344,6 +344,7 @@ static void link_alt_odb_entries(const char *alt, int
len, int sep,
struct string_list entries = STRING_LIST_INIT_NODUP;
char *alt_copy;
int i;
+ struct strbuf objdirbuf = STRBUF_INIT;
if (depth 5) {
error(%s: ignoring alternate object stores, nesting too deep.,
@@ -351,6 +352,9 @@ static void link_alt_odb_entries(const char *alt, int
len, int sep,
return;
}
+ strbuf_addstr(objdirbuf, absolute_path(get_object_directory()));
+ normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
This is somewhat a strange usage of a strbuf.
- it relies on that normalize_path_copy() only shrinks, never
lengthens, which is not too bad;
- if the operation ever shrinks, objdirbuf.len becomes
meaningless. The allocated length is objdirbuf.alloc, length
of the string is strlen(objdirbuf.buf).
- abspath.c::absolute_path() is still restricted to PATH_MAX, so
you are not gaining much by using strbuf here.
But at least this patch is not making things any worse, so
@@ -361,11 +365,12 @@ static void link_alt_odb_entries(const char *alt, int
len, int sep,
error(%s: ignoring relative alternate object store %s,
relative_base, entry);
} else {
- link_alt_odb_entry(entry, relative_base, depth);
+ link_alt_odb_entry(entry, relative_base, depth,
objdirbuf.buf);
}
}
string_list_clear(entries, 0);
free(alt_copy);
+ strbuf_release(objdirbuf);
}
void read_info_alternates(const char * relative_base, int depth)
diff --git a/t/t7702-repack-cyclic-alternate.sh
b/t/t7702-repack-cyclic-alternate.sh
new file mode 100755
index 000..8341d46
--- /dev/null
+++ b/t/t7702-repack-cyclic-alternate.sh
Do we really have to waste a new test file only for this test?
Don't we have any test that already uses alternate that these two
new test pieces can be added to?
$ git grep info/alternates t/
seems to show a few existing ones, including 1450 (fsck) and 7700
(repack) that look very relevant (I didn't check what the tests in
them are about, though).
--
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] doc: State coding guideline for error message punctuation
From: Junio C Hamano gits...@pobox.com Sent: Thursday, July 10, 2014 9:36 PM Jeff King p...@peff.net writes: On Mon, Jun 16, 2014 at 01:55:57PM +0100, Philip Oakley wrote: +Error Messages + + - We typically do not end error messages with a full stop. While + we've been rather inconsistent in the past, these days we mostly + settle on no punctuation. Unlike Junio, I do not mind spelling out guidance for error messages. However, I do not think the second sentence is adding anything here (everything in CodingGuidelines is subject to we did not always do it this way, but this is the preferred way now). So I'd drop it. And then add in more guidance. Besides no full stop, probably: 1. do not capitalize (unable to open %s, not Unable to open %s 2. maybe something on sentence structure / ordering? We tend to prefer cannot open 'foo': No such file or directory to foo: cannot open: No such file or directory. Perhaps there are others (we do not have to be exhaustive, but it makes sense to think for a moment while we are here). I do not want to forever be waiting for a reroll, so let's queue this and advance it to 'next' soonish, and refine the guidelines by further building on top of it as needed. Sorry, I've been away on vacation for the last few weeks. I hadn't been sure what to include in a re-roll without it gaining too much mission creep. Thanks for keeping an eye on it. Thanks. -- 8 -- From: Philip Oakley philipoak...@iee.org Date: Mon, 16 Jun 2014 13:55:57 +0100 Subject: [PATCH] doc: give some guidelines for error messages Clarify error message puntuation to reduce review workload. Signed-off-by: Philip Oakley philipoak...@iee.org Helped-by: Jeff King p...@peff.net Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/CodingGuidelines | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index f424dbd..f4137c6 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -264,6 +264,15 @@ For Python scripts: documentation for version 2.6 does not mention this prefix, it has been supported since version 2.6.0. +Error Messages + + - Do not end error messages with a full stop. + + - Do not capitalize (unable to open %s, not Unable to open %s) + + - Say what the error is first (cannot open %s, not %s: cannot open) + + Writing Documentation: Most (if not all) of the documentation pages are written in the -- 2.0.1-751-ge540734 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] sha1_file: do not add own object directory as alternate
On Tue, Jul 15, 2014 at 7:29 AM, Ephrim Khong dr.kh...@gmail.com wrote: When adding alternate object directories, we try not to add the directory of the current repository to avoid cycles. Unfortunately, that test was broken, since it compared an absolute with a relative path. Signed-off-by: Ephrim Khong dr.kh...@gmail.com --- diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh new file mode 100755 index 000..8341d46 --- /dev/null +++ b/t/t7702-repack-cyclic-alternate.sh @@ -0,0 +1,24 @@ +#!/bin/sh +# +# Copyright (c) 2014 Ephrim Khong +# + +test_description='repack involving cyclic alternate' +. ./test-lib.sh + +test_expect_success setup ' + GIT_OBJECT_DIRECTORY=.git//../.git/objects + export GIT_OBJECT_DIRECTORY + touch a Since the existence of 'a' is significant here, not its timestamp, it would be clearer to create the file with: a + git add a + git commit -m 1 + git repack -adl + echo $(pwd)/.git/objects/../objects .git/objects/info/alternates +' + +test_expect_success 're-packing repository with itsself as alternate' ' + git repack -adl + git fsck +' + +test_done -- 1.8.4.3 -- 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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder jrnie...@gmail.com wrote: Michael Haggerty wrote: So...I like the idea of enforcing refname checks at the lowest level possible, but I think that the change you propose is too abrupt. I think it needs either more careful analysis showing that it won't hurt anybody, or some kind of tooling or non-strict mode that people can use to fix their repositories. The recovery code has been broken for a while, so I don't see harm in this change. How to take care of the recovery use case is another question. FWIW I also would prefer if git update-ref -d or git branch -D could be used to delete corrupt refs instead of having to use fsck (since a fsck run can take a while), but that's a question for a later series. In an ideal world, the low-level functions would allow *reading* and *deleting* poorly named refs (even without any special flag) but not creating them. Is that doable? That should be doable. Ill add these repairs as 3-4 new patches at the end of the current patch series. The main complication I can see is iteration: would iteration skip poorly named refs and warn, or would something more complicated be needed? Right now, git branch will error and abort right away when it finds the first bad ref. I haven't checked the exact spot it does this yet but I suspect it is when building the ref-cache. I will solve the cases for create and delete for now. What/how to handle iterables will require more thought. Right now, since these refs will be filtered out and never end up in ref-cache, either from loose refs or from packed refs it does mean that anyone that uses an iterator is guaranteed to only get refs with valid names passed back to them. We would need to audit all code that uses iterators and make sure it can handle the case where the callback is suddenly invoked with a bad refname. 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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
On Tue, Jul 15, 2014 at 11:34 AM, Junio C Hamano gits...@pobox.com wrote: Jonathan Nieder jrnie...@gmail.com writes: How to take care of the recovery use case is another question. FWIW I also would prefer if git update-ref -d or git branch -D could be used to delete corrupt refs instead of having to use fsck (since a fsck run can take a while), but that's a question for a later series. Good thinking. In an ideal world, the low-level functions would allow *reading* and *deleting* poorly named refs (even without any special flag) but not creating them. Is that doable? The main complication I can see is iteration: would iteration skip poorly named refs and warn, or would something more complicated be needed? I somehow thought that was what we have always designed for, which DO_FOR_EACH_INCLUDE_BROKEN was a part of. I think that include broken only handles the case where the ref itself is bad, not when the refname is bad. I.e. it affects cases where the sha1 does not exist or the symref points to nowhere. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/4] cache-tree: Create/update cache-tree on checkout
On 07/13/2014 10:28 PM, David Turner wrote: From: David Turner dtur...@twopensource.com [] diff --git a/cache-tree.c b/cache-tree.c index 7fa524a..f951d7d 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -239,9 +239,12 @@ static int update_one(struct cache_tree *it, struct strbuf buffer; int missing_ok = flags WRITE_TREE_MISSING_OK; int dryrun = flags WRITE_TREE_DRY_RUN; + int repair = flags WRITE_TREE_REPAIR; int to_invalidate = 0; int i; + assert(!(dryrun repair)); I think something in the spirit of die(dryrun and repaiir can not be used together\n) Would be nicer to the user as well as being more reliable (as assert may be a no-op in some systems) -- 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] rebase: omit patch-identical commits with --fork-point
Thanks, John. My test script is working fine for me now with these patches. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] sha1_file: do not add own object directory as alternate
Ephrim Khong dr.kh...@gmail.com writes: diff --git a/t/t7702-repack-cyclic-alternate.sh b/t/t7702-repack-cyclic-alternate.sh new file mode 100755 index 000..8341d46 --- /dev/null +++ b/t/t7702-repack-cyclic-alternate.sh @@ -0,0 +1,24 @@ +#!/bin/sh +# +# Copyright (c) 2014 Ephrim Khong +# + +test_description='repack involving cyclic alternate' +. ./test-lib.sh + +test_expect_success setup ' + GIT_OBJECT_DIRECTORY=.git//../.git/objects + export GIT_OBJECT_DIRECTORY Do you need this artificially strange environment settings for the problem to manifest itself, or is it sufficient to have a non-canonical pathname in the info/alternates file? Exporting an environment early in the test and having later tests in the file depend on it makes it harder to debug when things go wrong, than leaving an info/alternates file in the repository, primarily because the latter can be inspected more easily in the trash directory after t7702-*.sh -i dies, hence the above question. + touch a Don't use 'touch' if you are not interested in timestams. Write this as a because what you care about here in this test is that an empty file 'a' exists, so that you can run git add on it. + git add a + git commit -m 1 + git repack -adl + echo $(pwd)/.git/objects/../objects .git/objects/info/alternates +' + +test_expect_success 're-packing repository with itsself as alternate' ' + git repack -adl + git fsck +' + +test_done -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/8] alloc.c: remove the alloc_raw_commit_node() function
Jeff King p...@peff.net writes:
#define DEFINE_ALLOCATOR(name, type) \
+static struct alloc_state name##_state; \
void *alloc_##name##_node(void) \
{\
+ return alloc_node(name##_state, sizeof(type)); \
}
This is really nice. Thanks.
+static struct alloc_state commit_state;
+
void *alloc_commit_node(void)
{
static int commit_count;
- struct commit *c = alloc_raw_commit_node();
+ struct commit *c = alloc_node(commit_state, sizeof(struct commit));
c-index = commit_count++;
return c;
}
--
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 2/8] alloc: write out allocator definitions
Jeff King p...@peff.net writes:
Because the allocator functions for tree, blobs, etc are all
very similar, we originally used a macro to avoid repeating
ourselves. Since the prior commit, though, the heavy lifting
is done by an inline helper function. The macro does still
save us a few lines, but at some readability cost. It
obfuscates the function definitions (and makes them hard to
find via grep).
Much worse, though, is the fact that it isn't used
consistently for all allocators. Somebody coming later may
be tempted to modify DEFINE_ALLOCATOR, but they would miss
alloc_commit_node, which is treated specially.
Let's just drop the macro and write everything out
explicitly.
Signed-off-by: Jeff King p...@peff.net
---
alloc.c | 38 +++---
1 file changed, 27 insertions(+), 11 deletions(-)
...
+static struct alloc_state blob_state;
+void *alloc_blob_node(void)
+{
+ struct blob *b = alloc_node(blob_state, sizeof(struct blob));
+ return b;
+}
I think the change makes the code nicer overall, but it looks
strange to see a (void *) that was returned by alloc_node()
implicitly casted to (struct blob *) by assignment to b and then
again implicitly casted to (void *) by it being the return type of
the function.
Is there a reason why it is not like so?
void *alloc_blob_node(void)
{
return alloc_node(blob_state, sizeof(struct blob));
}
I may have missed previous discussion on it, in which case I'd
apologize in advance.
--
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/8] move setting of object-type to alloc_* functions
Jeff King p...@peff.net writes:
diff --git a/alloc.c b/alloc.c
index 03e458b..fd2e32d 100644
--- a/alloc.c
+++ b/alloc.c
@@ -52,6 +52,7 @@ static struct alloc_state blob_state;
void *alloc_blob_node(void)
{
struct blob *b = alloc_node(blob_state, sizeof(struct blob));
+ b-object.type = OBJ_BLOB;
return b;
}
Ahh, please disregard my question on 2/8 ;-)
--
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 3/3] tag: support configuring --sort via .gitconfig
On Tue, 2014-07-15 at 12:12 -0700, Junio C Hamano wrote: Keller, Jacob E jacob.e.kel...@intel.com writes: I am going to re-submit this with an enum-style return. I am also changing how we parse so that we can correctly report whether the sort function or sort atom is incorrect. Oh, our mails crossed, I guess. As long as it will leave the door open for later enhancements for more context sensitive error diagnosis, I do not particularly mind a solution around enum. Hmm. I looked at coding it this way, and it actually makes it less sensitive than I would like. I'm not a fan of the extra value parameter, but I do like a more proper error display, and indeed one that is more precise. I'll try to have a new series posted soon which takes these into account. Regards, Jake
Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
On Tue, Jul 15, 2014 at 12:34 PM, Ronnie Sahlberg sahlb...@google.com wrote: On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder jrnie...@gmail.com wrote: Michael Haggerty wrote: So...I like the idea of enforcing refname checks at the lowest level possible, but I think that the change you propose is too abrupt. I think it needs either more careful analysis showing that it won't hurt anybody, or some kind of tooling or non-strict mode that people can use to fix their repositories. The recovery code has been broken for a while, so I don't see harm in this change. How to take care of the recovery use case is another question. FWIW I also would prefer if git update-ref -d or git branch -D could be used to delete corrupt refs instead of having to use fsck (since a fsck run can take a while), but that's a question for a later series. In an ideal world, the low-level functions would allow *reading* and *deleting* poorly named refs (even without any special flag) but not creating them. Is that doable? That should be doable. Ill add these repairs as 3-4 new patches at the end of the current patch series. The main complication I can see is iteration: would iteration skip poorly named refs and warn, or would something more complicated be needed? Right now, git branch will error and abort right away when it finds the first bad ref. I haven't checked the exact spot it does this yet but I suspect it is when building the ref-cache. I will solve the cases for create and delete for now. What/how to handle iterables will require more thought. Right now, since these refs will be filtered out and never end up in ref-cache, either from loose refs or from packed refs it does mean that anyone that uses an iterator is guaranteed to only get refs with valid names passed back to them. We would need to audit all code that uses iterators and make sure it can handle the case where the callback is suddenly invoked with a bad refname. Thanks, Jonathan The following seems to do the trick to allow deleting a bad ref. We would need something for the iterator too. Since this touches the same areas that my ~100 other ref transaction patches that are queued up do, I would like to wait applying this patch until once the next few series are finished and merged. (to avoid having to do a lot of rebases and fix legio of merge conflicts that this would introduce in my branches). Treat this as an approximation on what the fix to repair git branch -D will look like once the time comes. regards ronnie sahlberg === commit a2514213999a192c9995a3a5e1479d7d09e2c083 Author: Ronnie Sahlberg sahlb...@google.com Date: Tue Jul 15 12:59:36 2014 -0700 refs.c: fix handling of badly named refs 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. But we can not really recover from a badly named ref with less than manually deleting the .git/refs/heads/refname file. Change resolve_ref_unsafe to take a flags field instead of a 'reading' boolean and update all callers that used a non-zero value for reading to pass the flag RESOLVE_REF_READING instead. Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make resolve_ref_unsafe skip checking the refname for sanity and use this from branch.c so that we will be able to call resolve_ref_unsafe on such refs when trying to delete it. Add checks for refname sanity when updating (not deleting) a ref in ref_transaction_update and in ref_transaction_create to make the transaction fail if an attempt is made to create/update a badly named ref. Since all ref changes will later go through the transaction layer this means we no longer need to check for and fail for bad refnames in lock_ref_sha1_basic. Change lock_ref_sha1_basic to not fail for bad refnames. Just check the refname, and print an error, and remember that the refname is bad so that we can skip calling verify_lock(). Signed-off-by: Ronnie Sahlberg sahlb...@google.com diff --git a/builtin/blame.c b/builtin/blame.c index 662e3fe..76340e2 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2278,7 +2278,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, head_sha1, RESOLVE_REF_READING, 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..5c95656 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]
[PATCH v8 0/4] tag: configure default tag sort via .gitconfig
This patch series is hopefully the final version of a series of patches which updates git-tag to allow the .gitconfig variable 'tag.sort' to be used as the default sort parameter. This version uses a new set/pop error routine setup which enables correctly modifying the error routine to handle the config vs the command line. In addition, I split the patch such that all the changes to the original parse_opt_sort are shown, and the following patch which extracts this function is just a function extraction with no changes, making it easier to review the new changes. One other minor bug fix is included as well. Jacob Keller (4): usage: make error functions a stack tag: fix --sort tests to use cat-\EOF format tag: update parsing to be more precise regarding errors tag: support configuring --sort via .gitconfig Documentation/config.txt | 5 +++ Documentation/git-tag.txt | 5 ++- builtin/tag.c | 97 --- git-compat-util.h | 1 + t/t7004-tag.sh| 76 +++-- usage.c | 29 -- 6 files changed, 167 insertions(+), 46 deletions(-) -- 2.0.1.475.g9b8d714 -- 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 v8 1/4] usage: make error functions a stack
Let error routine be a stack of error functions so that callers can
temporarily override the error_routine and then pop their modification
off the stack. This enables customizing error for a small code segment.
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
This is a modification of Peff's original idea for handling multiple error
routines. I simplified it by not having the collect and other routines. I only
modify set_error_routine to be a push operation, with pop_error_routine being
its opposite. I don't let pop_error_routine remove all the error routines,
instead only doing one with an assert check that we never call it too many
times.
This enables temporarily modifying the error routine and then popping back to
the previous value.
git-compat-util.h | 1 +
usage.c | 29 ++---
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 9de318071083..6d0416c90ad8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -344,6 +344,7 @@ static inline int const_error(void)
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err,
va_list params));
extern void set_error_routine(void (*routine)(const char *err, va_list
params));
+extern void pop_error_routine(void);
extern void set_die_is_recursing_routine(int (*routine)(void));
extern int starts_with(const char *str, const char *prefix);
diff --git a/usage.c b/usage.c
index ed146453cabe..fd9126a7ca0b 100644
--- a/usage.c
+++ b/usage.c
@@ -57,18 +57,41 @@ static int die_is_recursing_builtin(void)
* (ugh), so keep things static. */
static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) =
usage_builtin;
static NORETURN_PTR void (*die_routine)(const char *err, va_list params) =
die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
static int (*die_is_recursing)(void) = die_is_recursing_builtin;
+struct error_func_list {
+ void (*func)(const char *, va_list);
+ struct error_func_list *next;
+};
+static struct error_func_list default_error_func = { error_builtin };
+static struct error_func_list *error_funcs = default_error_func;
+
void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list
params))
{
die_routine = routine;
}
+/* push error routine onto the error function stack */
void set_error_routine(void (*routine)(const char *err, va_list params))
{
- error_routine = routine;
+ struct error_func_list *efl = xmalloc(sizeof(*efl));
+ efl-func = routine;
+ efl-next = error_funcs;
+ error_funcs = efl;
+}
+
+/* pop a single error routine off of the error function stack, thus reverting
+ * to previous error. Should always be paired with a set_error_routine */
+void pop_error_routine(void)
+{
+ assert(error_funcs != default_error_func);
+
+ struct error_func_list *efl = error_funcs;
+ if (efl-next) {
+ error_funcs = efl-next;
+ free(efl);
+ }
}
void set_die_is_recursing_routine(int (*routine)(void))
@@ -144,7 +167,7 @@ int error(const char *err, ...)
va_list params;
va_start(params, err);
- error_routine(err, params);
+ error_funcs-func(err, params);
va_end(params);
return -1;
}
--
2.0.1.475.g9b8d714
--
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 v8 4/4] tag: support configuring --sort via .gitconfig
Add support for configuring default sort specification for git tags.
Command line option will override the configured value. Both methods use
the same syntax. Make use of (set/pop)_error_routine to temporarily
modify error reporting when parsing as a configuration option.
Cc: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
Documentation/config.txt | 5 ++
Documentation/git-tag.txt | 5 +-
builtin/tag.c | 124 --
t/t7004-tag.sh| 36 ++
4 files changed, 120 insertions(+), 50 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule.name.ignore::
--ignore-submodules option. The 'git submodule' commands are not
affected by this setting.
+tag.sort::
+ This variable controls the sort ordering of tags when displayed by
+ linkgit:git-tag[1]. Without the --sort=value option provided, the
+ value of this variable will be used as the default.
+
tar.umask::
This variable can be used to restrict the permission bits of
tar archive entries. The default is 0002, which turns off the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index b424a1bc48bb..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
Sort in a specific order. Supported type is refname
(lexicographic order), version:refname or v:refname (tag
names are treated as versions). Prepend - to reverse sort
- order.
+ order. When this option is not given, the sort order defaults to the
+ value configured for the 'tag.sort' variable if it exists, or
+ lexicographic order otherwise. See linkgit:git-config[1].
--column[=options]::
--no-column::
@@ -317,6 +319,7 @@ include::date-formats.txt[]
SEE ALSO
linkgit:git-check-ref-format[1].
+linkgit:git-config[1].
GIT
---
diff --git a/builtin/tag.c b/builtin/tag.c
index 7d82526e76be..603cf688368c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
#define SORT_MASK 0x7fff
#define REVERSE_SORT0x8000
+static int tag_sort;
+
struct tag_filter {
const char **patterns;
int lines;
@@ -346,9 +348,76 @@ static const char tag_template_nocleanup[] =
Lines starting with '%c' will be kept; you may remove them
yourself if you want to.\n);
+static int parse_sort_string(const char *arg, int *sort)
+{
+ char *value, *separator, *type, *atom;
+ int flags = 0, function = 0, err = 0;
+
+ /* skip the '-' prefix for reverse sort order first */
+ if (skip_prefix(arg, -, arg))
+ flags |= REVERSE_SORT;
+
+ /* duplicate string so we can modify it in place */
+ value = xstrdup(arg);
+
+ /* determine the sort function and the sorting atom */
+ separator = strchr(value, ':');
+ if (separator) {
+ /* split the string at the separator with a NULL byte */
+ *separator = '\0';
+ type = value;
+ atom = separator + 1;
+ } else {
+ /* we have no separator, so assume the whole string is the *
atom */
+ type = NULL;
+ atom = value;
+ }
+
+ if (type) {
+ if (!strcmp(type, version) || !strcmp(type, v))
+ function = VERCMP_SORT;
+ else {
+ err = error(_(unsupported sort function '%s'), type);
+ goto abort;
+ }
+
+ } else
+ function = STRCMP_SORT;
+
+ /* for now, only the refname is a valid atom */
+ if (atom strcmp(atom, refname)) {
+ err = error(_(unsupported sort specification '%s'), atom);
+ goto abort;
+ }
+
+ *sort = (flags | function);
+
+abort:
+ free(value);
+ return err;
+}
+
+static void error_bad_sort_config(const char *err, va_list params)
+{
+ vreportf(warning: tag.sort has , err, params);
+}
+
static int git_tag_config(const char *var, const char *value, void *cb)
{
- int status = git_gpg_config(var, value, cb);
+ int status;
+
+ if (!strcmp(var, tag.sort)) {
+ if (!value)
+ return config_error_nonbool(var);
+
+ set_error_routine(error_bad_sort_config);
+ parse_sort_string(value, tag_sort);
+ pop_error_routine();
+
+ return 0;
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, column.))
@@ -522,51 +591,8 @@ static int parse_opt_points_at(const struct option *opt
__attribute__((unused)),
static int
[PATCH v8 2/4] tag: fix --sort tests to use cat-\EOF format
The --sort tests should use the better format for expect to maintain indenting and ensure that no substitution is occurring. This makes parsing and understanding the tests a bit easier. Signed-off-by: Jacob Keller jacob.e.kel...@intel.com --- t/t7004-tag.sh | 40 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index e4ab0f5b6419..66010b0e7066 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' ' git tag foo1.6 git tag foo1.10 git tag -l --sort=refname foo* actual - cat expect EOF -foo1.10 -foo1.3 -foo1.6 -EOF + cat expect -\EOF + foo1.10 + foo1.3 + foo1.6 + EOF test_cmp expect actual ' test_expect_success 'version sort' ' git tag -l --sort=version:refname foo* actual - cat expect EOF -foo1.3 -foo1.6 -foo1.10 -EOF + cat expect -\EOF + foo1.3 + foo1.6 + foo1.10 + EOF test_cmp expect actual ' test_expect_success 'reverse version sort' ' git tag -l --sort=-version:refname foo* actual - cat expect EOF -foo1.10 -foo1.6 -foo1.3 -EOF + cat expect -\EOF + foo1.10 + foo1.6 + foo1.3 + EOF test_cmp expect actual ' test_expect_success 'reverse lexical sort' ' git tag -l --sort=-refname foo* actual - cat expect EOF -foo1.6 -foo1.3 -foo1.10 -EOF + cat expect -\EOF + foo1.6 + foo1.3 + foo1.10 + EOF test_cmp expect actual ' -- 2.0.1.475.g9b8d714 -- 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 v8 3/4] tag: update parsing to be more precise regarding errors
Update the parsing of sort string specifications so that it is able to
properly detect errors in the function type and allowed atoms.
Cc: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
This function should replace the one I think is already on one of the branches.
This version fully updates the parse_sort_opt to be like what will be the
parse_sort_string function. I have ensured that the next patch is purely a move
of this function, and does not contain modifications to make this easier to
review. Instead of using skip_prefix, I use strchr and check for the separator.
builtin/tag.c | 55 +--
1 file changed, 41 insertions(+), 14 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7d82526e76be 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -522,24 +522,51 @@ static int parse_opt_points_at(const struct option *opt
__attribute__((unused)),
static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
{
int *sort = opt-value;
- int flags = 0;
+ char *value, *separator, *type, *atom;
+ int flags = 0, function = 0, err = 0;
- if (*arg == '-') {
+ /* skip the '-' prefix for reverse sort order first */
+ if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
- arg++;
+
+ /* duplicate string so we can modify it in place */
+ value = xstrdup(arg);
+
+ /* determine the sort function and the sorting atom */
+ separator = strchr(value, ':');
+ if (separator) {
+ /* split the string at the separator with a NULL byte */
+ *separator = '\0';
+ type = value;
+ atom = separator + 1;
+ } else {
+ /* we have no separator, so assume the whole string is the *
atom */
+ type = NULL;
+ atom = value;
}
- if (starts_with(arg, version:)) {
- *sort = VERCMP_SORT;
- arg += 8;
- } else if (starts_with(arg, v:)) {
- *sort = VERCMP_SORT;
- arg += 2;
+
+ if (type) {
+ if (!strcmp(type, version) || !strcmp(type, v))
+ function = VERCMP_SORT;
+ else {
+ err = error(_(unsupported sort function '%s'), type);
+ goto abort;
+ }
+
} else
- *sort = STRCMP_SORT;
- if (strcmp(arg, refname))
- die(_(unsupported sort specification %s), arg);
- *sort |= flags;
- return 0;
+ function = STRCMP_SORT;
+
+ /* for now, only the refname is a valid atom */
+ if (atom strcmp(atom, refname)) {
+ err = error(_(unsupported sort specification '%s'), atom);
+ goto abort;
+ }
+
+ *sort = (flags | function);
+
+abort:
+ free(value);
+ return err;
}
int cmd_tag(int argc, const char **argv, const char *prefix)
--
2.0.1.475.g9b8d714
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/4] cache-tree: Create/update cache-tree on checkout
Torsten Bögershausen tbo...@web.de writes: On 07/13/2014 10:28 PM, David Turner wrote: From: David Turner dtur...@twopensource.com [] diff --git a/cache-tree.c b/cache-tree.c index 7fa524a..f951d7d 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -239,9 +239,12 @@ static int update_one(struct cache_tree *it, struct strbuf buffer; int missing_ok = flags WRITE_TREE_MISSING_OK; int dryrun = flags WRITE_TREE_DRY_RUN; +int repair = flags WRITE_TREE_REPAIR; int to_invalidate = 0; int i; + assert(!(dryrun repair)); I think something in the spirit of die(dryrun and repaiir can not be used together\n) Would be nicer to the user as well as being more reliable (as assert may be a no-op in some systems) While it is a good suggestion *not* to attempt validating the end-user input with assert() for the reason you state, I think for this particular case, these flags only come from the code and assert() to catch programming errors would be sufficient. Besides, as discussed elsewhere, WRITE_TREE_DRY_RUN should not be used, and the support for it should be dropped. It is broken in that the code path that leads to update_one() may correctly compute tree object names and stuff them in the cache-tree, higher layer code would then complain on such a cache-tree that records tree objects that do not exist. -- 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 3/3] tag: support configuring --sort via .gitconfig
On Tue, 2014-07-15 at 12:12 -0700, Junio C Hamano wrote:
Keller, Jacob E jacob.e.kel...@intel.com writes:
I am going to re-submit this with an enum-style return. I am also
changing how we parse so that we can correctly report whether the sort
function or sort atom is incorrect.
Oh, our mails crossed, I guess. As long as it will leave the door
open for later enhancements for more context sensitive error
diagnosis, I do not particularly mind a solution around enum.
I just sent a v8 of the series. I think I mostly followed Peff's idea of
using a pop_error_routine function, but not as complex as his was. This
overall results in more accurate errors, and doesn't clutter the
original parse_sort_string with too much knowledge about what particular
value is being parsed. Hopefully we can finally converge on a good set
of patches.
Thanks,
Jake
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
Re: [PATCH v2 3/4] use new config API for worktree configurations of submodules
Heiko Voigt hvo...@hvoigt.net writes: Can there be any caller that include and use submodule-config.h that does not need anythjing from submodule.h? Or vice versa? It just did not look like these two headers describe independent subsystems but they almost always have to go hand-in-hand. And if that is the case, perhaps it is not such a good idea to add it as a new header. That was where my question came from. The reason for a separate module was because we add quite some lines of code for it. $ wc -l submodule.c 1068 submodule.c $ wc -l submodule-config.c 435 submodule-config.c Because of this I would like to keep the c-files separate. OK. I do not feel too strongly. It just looked odd that a change needs to add a new header file without having to change the code in existing files at all. Any other thing that still needs fixing in the series, or is it now ready for 'next'? -- 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 v8 0/4] tag: configure default tag sort via .gitconfig
Jacob Keller jacob.e.kel...@intel.com writes: This patch series is hopefully the final version of a series of patches which updates git-tag to allow the .gitconfig variable 'tag.sort' to be used as the default sort parameter. This version uses a new set/pop error routine setup which enables correctly modifying the error routine to handle the config vs the command line. In addition, I split the patch such that all the changes to the original parse_opt_sort are shown, and the following patch which extracts this function is just a function extraction with no changes, making it easier to review the new changes. One other minor bug fix is included as well. Jacob Keller (4): usage: make error functions a stack tag: fix --sort tests to use cat-\EOF format tag: update parsing to be more precise regarding errors tag: support configuring --sort via .gitconfig The first one looked somewhat tricky, but looked nice overall from a cursory reading. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 1/4] usage: make error functions a stack
Jacob Keller jacob.e.kel...@intel.com writes:
extern void set_error_routine(void (*routine)(const char *err, va_list
params));
+extern void pop_error_routine(void);
pop that undoes set smells somewhat weird. Perhaps we should rename
set to push? That would allow us catch possible topics that add new
calls to set_error_routine() as well by forcing the system not to
link when they are merged without necessary fixes.
+/* push error routine onto the error function stack */
void set_error_routine(void (*routine)(const char *err, va_list params))
{
- error_routine = routine;
+ struct error_func_list *efl = xmalloc(sizeof(*efl));
+ efl-func = routine;
+ efl-next = error_funcs;
+ error_funcs = efl;
+}
+
+/* pop a single error routine off of the error function stack, thus reverting
+ * to previous error. Should always be paired with a set_error_routine */
+void pop_error_routine(void)
+{
+ assert(error_funcs != default_error_func);
+
+ struct error_func_list *efl = error_funcs;
decl-after-stmt. Can be fixed easily by flipping the above two
lines.
+ if (efl-next) {
+ error_funcs = efl-next;
+ free(efl);
+ }
}
void set_die_is_recursing_routine(int (*routine)(void))
@@ -144,7 +167,7 @@ int error(const char *err, ...)
va_list params;
va_start(params, err);
- error_routine(err, params);
+ error_funcs-func(err, params);
va_end(params);
return -1;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] fix test suite with mingw-unicode patches
Am 15.07.2014 20:20, schrieb Junio C Hamano:
Stepan Kasal ka...@ucw.cz writes:
Hello Hannes,
attached please find the patches that Karsten pointed out:
1) The unicode file name support was omitted from his unicode patch
series; my mistake, sorry. There is still big part missing: support
for unicode environment; I can only hope the tests would choke on
that.
2) Windows cannot pass non-UTF parameters (commit messages in this
case): original patch by Pat Thoyts was extended to apply to other
similar cases: the commit msg is passed through stdin.
If there are still problems remaining, please tell us.
Thanks,
Stepan
Karsten Blees (2):
Win32: Unicode file name support (except dirent)
Win32: Unicode file name support (dirent)
Pat Thoyts and Stepan Kasal(1):
tests: do not pass iso8859-1 encoded parameter
Thanks. I'll queue these and wait for Windows folks to respond.
With favourable feedback they can go directly from pu to master, I
would think.
Looking good. After fixing the ELOOP and fchmod issues (see followup
patches), there are 9 test failures left. Only one of these is
environment related, and for the rest we have fixes in the msysgit
fork:
* t0081-line-buffer: 1
Using file descriptor other than 0, 1, 2.
https://github.com/msysgit/git/commit/4940c51a
* t0110-urlmatch-normalization: 1
Passing binary data on the command line...would have to teach
test-urlmatch-normalization.c to read from stdin or file.
https://github.com/msysgit/git/commit/be0d6dee
* t4036-format-patch-signer-mime: 1
not ok 4 - format with non ASCII signer name
#
# GIT_COMMITTER_NAME=はまの ふにおう \
# git format-patch -s --stdout -1 output
# grep Content-Type output
#
Passing non-ASCII by environment variable, will be fixed by Unicode environment
support.
* t4201-shortlog: 3
Passing binary data on the command line ('git-commit -m').
https://github.com/msysgit/git/commit/3717ce1b
* t4210-log-i18n: 2
Passing binary data on the command line ('git log --grep=$latin1_e').
https://github.com/msysgit/git/commit/dd2defa3
* t7001-mv: 6
cp -P fails in MinGW - perhaps use the long option forms (--no-dereference)?
https://github.com/msysgit/git/commit/00764ca1
* t8001-annotate/t8002-blame: 5
Msys.dll thinks '-L/regex/' is an absolute path and expands to
'-LC:/msysgit/regex/'.
https://github.com/msysgit/git/commit/2d52168a
* t8005-blame-i18n: 4
Passing binary data on the command line ('git-commit --author -m').
https://github.com/msysgit/git/commit/3717ce1b
* t9902-completion: 2
Must use 'pwd -W' to get Windows-style absolute paths.
https://github.com/msysgit/git/commit/9b612448
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] MinGW: fix compile error due to missing ELOOP
MinGW and MSVC before 2010 don't define ELOOP, use EMLINK (aka Too many links) instead. Signed-off-by: Karsten Blees bl...@dcon.de --- compat/mingw.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/compat/mingw.h b/compat/mingw.h index 405c08f..510530c 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -35,6 +35,9 @@ typedef int socklen_t; #ifndef EWOULDBLOCK #define EWOULDBLOCK EAGAIN #endif +#ifndef ELOOP +#define ELOOP EMLINK +#endif #define SHUT_WR SD_SEND #define SIGHUP 1 -- 2.0.1.779.g26aeac4.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] config: use chmod() instead of fchmod()
There is no fchmod() on native Windows platforms (MinGW and MSVC), and the
equivalent Win32 API (SetFileInformationByHandle) requires Windows Vista.
Use chmod() instead.
Signed-off-by: Karsten Blees bl...@dcon.de
---
config.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index ba882a1..9767c4b 100644
--- a/config.c
+++ b/config.c
@@ -1636,8 +1636,8 @@ int git_config_set_multivar_in_file(const char
*config_filename,
MAP_PRIVATE, in_fd, 0);
close(in_fd);
- if (fchmod(fd, st.st_mode 0) 0) {
- error(fchmod on %s failed: %s,
+ if (chmod(lock-filename, st.st_mode 0) 0) {
+ error(chmod on %s failed: %s,
lock-filename, strerror(errno));
ret = CONFIG_NO_WRITE;
goto out_free;
@@ -1815,8 +1815,8 @@ int git_config_rename_section_in_file(const char
*config_filename,
fstat(fileno(config_file), st);
- if (fchmod(out_fd, st.st_mode 0) 0) {
- ret = error(fchmod on %s failed: %s,
+ if (chmod(lock-filename, st.st_mode 0) 0) {
+ ret = error(chmod on %s failed: %s,
lock-filename, strerror(errno));
goto out;
}
--
2.0.1.779.g26aeac4.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] refs.c: add a public is_branch function
Both refs.c and fsck.c have their own private copies of the is_branch function.
Delete the is_branch function from fsck.c and make the version in refs.c
public.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/fsck.c | 5 -
refs.c | 2 +-
refs.h | 2 ++
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index fc150c8..a473622 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -482,11 +482,6 @@ static int fsck_handle_reflog(const char *logname, const
unsigned char *sha1, in
return 0;
}
-static int is_branch(const char *refname)
-{
- return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
-}
-
static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int
flag, void *cb_data)
{
struct object *obj;
diff --git a/refs.c b/refs.c
index dc45774..dc44802 100644
--- a/refs.c
+++ b/refs.c
@@ -2817,7 +2817,7 @@ static int log_ref_write(const char *refname, const
unsigned char *old_sha1,
return 0;
}
-static int is_branch(const char *refname)
+int is_branch(const char *refname)
{
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
diff --git a/refs.h b/refs.h
index 4e3050d..8b4a3f2 100644
--- a/refs.h
+++ b/refs.h
@@ -125,6 +125,8 @@ extern int repack_without_refs(const char **refnames, int
n);
extern int ref_exists(const char *);
+extern int is_branch(const char *refname);
+
/*
* If refname is a non-symbolic reference that refers to a tag object,
* and the tag can be (recursively) dereferenced to a non-tag object,
--
2.0.1.442.g7fe6834.dirty
--
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] remove duplicate of is_branch
Jun, List Please find a trivial patch that makes refs.c:is_branch public. This allows us to delete the identical copy of is_branch in fsck.c Ronnie Sahlberg (1): refs.c: add a public is_branch function builtin/fsck.c | 5 - refs.c | 2 +- refs.h | 2 ++ 3 files changed, 3 insertions(+), 6 deletions(-) -- 2.0.1.442.g7fe6834.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs.c: add a public is_branch function
Ronnie Sahlberg wrote: Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/fsck.c | 5 - refs.c | 2 +- refs.h | 2 ++ 3 files changed, 3 insertions(+), 6 deletions(-) Makes sense -- thanks. (This is an old one: v1.5.4-rc4~27 (2008-01-15), v1.5.4-rc4~30 (2008-01-15). Most of the running time of fsck is per-object, not per-ref, so maintainability here seems worth the performance cost.) 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 v8 1/4] usage: make error functions a stack
On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
Jacob Keller jacob.e.kel...@intel.com writes:
extern void set_error_routine(void (*routine)(const char *err, va_list
params));
+extern void pop_error_routine(void);
pop that undoes set smells somewhat weird. Perhaps we should rename
set to push? That would allow us catch possible topics that add new
calls to set_error_routine() as well by forcing the system not to
link when they are merged without necessary fixes.
I thought about changing set too, but wasn't sure that made sense..?
That does make more sense now though. There *are* valid use cases where
a set_error_routine is used without a pop, (the one current use, I
think).
I'll update this patch with that change.
+/* push error routine onto the error function stack */
void set_error_routine(void (*routine)(const char *err, va_list params))
{
- error_routine = routine;
+ struct error_func_list *efl = xmalloc(sizeof(*efl));
+ efl-func = routine;
+ efl-next = error_funcs;
+ error_funcs = efl;
+}
+
+/* pop a single error routine off of the error function stack, thus
reverting
+ * to previous error. Should always be paired with a set_error_routine */
+void pop_error_routine(void)
+{
+ assert(error_funcs != default_error_func);
+
+ struct error_func_list *efl = error_funcs;
decl-after-stmt. Can be fixed easily by flipping the above two
lines.
Oh, right yes. I'll fix that in a resend as well.
Thanks,
Jake
Re: [PATCH v8 1/4] usage: make error functions a stack
On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote: Jacob Keller jacob.e.kel...@intel.com writes: extern void set_error_routine(void (*routine)(const char *err, va_list params)); +extern void pop_error_routine(void); pop that undoes set smells somewhat weird. Perhaps we should rename set to push? That would allow us catch possible topics that add new calls to set_error_routine() as well by forcing the system not to link when they are merged without necessary fixes. Also curious what your thoughts on making every set_*_routine to be a stack? For now I was only planning on error but it maybe makes sense to change them all? Thanks, Jake
[PATCH v9 4/4] tag: support configuring --sort via .gitconfig
Add support for configuring default sort specification for git tags.
Command line option will override the configured value. Both methods use
the same syntax. Make use of (set/pop)_error_routine to temporarily
modify error reporting when parsing as a configuration option.
Cc: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
Documentation/config.txt | 5 ++
Documentation/git-tag.txt | 5 +-
builtin/tag.c | 124 --
t/t7004-tag.sh| 36 ++
4 files changed, 120 insertions(+), 50 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule.name.ignore::
--ignore-submodules option. The 'git submodule' commands are not
affected by this setting.
+tag.sort::
+ This variable controls the sort ordering of tags when displayed by
+ linkgit:git-tag[1]. Without the --sort=value option provided, the
+ value of this variable will be used as the default.
+
tar.umask::
This variable can be used to restrict the permission bits of
tar archive entries. The default is 0002, which turns off the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index b424a1bc48bb..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
Sort in a specific order. Supported type is refname
(lexicographic order), version:refname or v:refname (tag
names are treated as versions). Prepend - to reverse sort
- order.
+ order. When this option is not given, the sort order defaults to the
+ value configured for the 'tag.sort' variable if it exists, or
+ lexicographic order otherwise. See linkgit:git-config[1].
--column[=options]::
--no-column::
@@ -317,6 +319,7 @@ include::date-formats.txt[]
SEE ALSO
linkgit:git-check-ref-format[1].
+linkgit:git-config[1].
GIT
---
diff --git a/builtin/tag.c b/builtin/tag.c
index 7d82526e76be..091536c61eab 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
#define SORT_MASK 0x7fff
#define REVERSE_SORT0x8000
+static int tag_sort;
+
struct tag_filter {
const char **patterns;
int lines;
@@ -346,9 +348,76 @@ static const char tag_template_nocleanup[] =
Lines starting with '%c' will be kept; you may remove them
yourself if you want to.\n);
+static int parse_sort_string(const char *arg, int *sort)
+{
+ char *value, *separator, *type, *atom;
+ int flags = 0, function = 0, err = 0;
+
+ /* skip the '-' prefix for reverse sort order first */
+ if (skip_prefix(arg, -, arg))
+ flags |= REVERSE_SORT;
+
+ /* duplicate string so we can modify it in place */
+ value = xstrdup(arg);
+
+ /* determine the sort function and the sorting atom */
+ separator = strchr(value, ':');
+ if (separator) {
+ /* split the string at the separator with a NULL byte */
+ *separator = '\0';
+ type = value;
+ atom = separator + 1;
+ } else {
+ /* we have no separator, so assume the whole string is the *
atom */
+ type = NULL;
+ atom = value;
+ }
+
+ if (type) {
+ if (!strcmp(type, version) || !strcmp(type, v))
+ function = VERCMP_SORT;
+ else {
+ err = error(_(unsupported sort function '%s'), type);
+ goto abort;
+ }
+
+ } else
+ function = STRCMP_SORT;
+
+ /* for now, only the refname is a valid atom */
+ if (atom strcmp(atom, refname)) {
+ err = error(_(unsupported sort specification '%s'), atom);
+ goto abort;
+ }
+
+ *sort = (flags | function);
+
+abort:
+ free(value);
+ return err;
+}
+
+static void error_bad_sort_config(const char *err, va_list params)
+{
+ vreportf(warning: tag.sort has , err, params);
+}
+
static int git_tag_config(const char *var, const char *value, void *cb)
{
- int status = git_gpg_config(var, value, cb);
+ int status;
+
+ if (!strcmp(var, tag.sort)) {
+ if (!value)
+ return config_error_nonbool(var);
+
+ push_error_routine(error_bad_sort_config);
+ parse_sort_string(value, tag_sort);
+ pop_error_routine();
+
+ return 0;
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, column.))
@@ -522,51 +591,8 @@ static int parse_opt_points_at(const struct option *opt
__attribute__((unused)),
static int
[PATCH v9 3/4] tag: update parsing to be more precise regarding errors
Update the parsing of sort string specifications so that it is able to
properly detect errors in the function type and allowed atoms.
Cc: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
builtin/tag.c | 55 +--
1 file changed, 41 insertions(+), 14 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7d82526e76be 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -522,24 +522,51 @@ static int parse_opt_points_at(const struct option *opt
__attribute__((unused)),
static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
{
int *sort = opt-value;
- int flags = 0;
+ char *value, *separator, *type, *atom;
+ int flags = 0, function = 0, err = 0;
- if (*arg == '-') {
+ /* skip the '-' prefix for reverse sort order first */
+ if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
- arg++;
+
+ /* duplicate string so we can modify it in place */
+ value = xstrdup(arg);
+
+ /* determine the sort function and the sorting atom */
+ separator = strchr(value, ':');
+ if (separator) {
+ /* split the string at the separator with a NULL byte */
+ *separator = '\0';
+ type = value;
+ atom = separator + 1;
+ } else {
+ /* we have no separator, so assume the whole string is the *
atom */
+ type = NULL;
+ atom = value;
}
- if (starts_with(arg, version:)) {
- *sort = VERCMP_SORT;
- arg += 8;
- } else if (starts_with(arg, v:)) {
- *sort = VERCMP_SORT;
- arg += 2;
+
+ if (type) {
+ if (!strcmp(type, version) || !strcmp(type, v))
+ function = VERCMP_SORT;
+ else {
+ err = error(_(unsupported sort function '%s'), type);
+ goto abort;
+ }
+
} else
- *sort = STRCMP_SORT;
- if (strcmp(arg, refname))
- die(_(unsupported sort specification %s), arg);
- *sort |= flags;
- return 0;
+ function = STRCMP_SORT;
+
+ /* for now, only the refname is a valid atom */
+ if (atom strcmp(atom, refname)) {
+ err = error(_(unsupported sort specification '%s'), atom);
+ goto abort;
+ }
+
+ *sort = (flags | function);
+
+abort:
+ free(value);
+ return err;
}
int cmd_tag(int argc, const char **argv, const char *prefix)
--
2.0.1.475.g9b8d714
--
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 v9 2/4] tag: fix --sort tests to use cat-\EOF format
The --sort tests should use the better format for expect to maintain indenting and ensure that no substitution is occurring. This makes parsing and understanding the tests a bit easier. Signed-off-by: Jacob Keller jacob.e.kel...@intel.com --- t/t7004-tag.sh | 40 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index e4ab0f5b6419..66010b0e7066 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' ' git tag foo1.6 git tag foo1.10 git tag -l --sort=refname foo* actual - cat expect EOF -foo1.10 -foo1.3 -foo1.6 -EOF + cat expect -\EOF + foo1.10 + foo1.3 + foo1.6 + EOF test_cmp expect actual ' test_expect_success 'version sort' ' git tag -l --sort=version:refname foo* actual - cat expect EOF -foo1.3 -foo1.6 -foo1.10 -EOF + cat expect -\EOF + foo1.3 + foo1.6 + foo1.10 + EOF test_cmp expect actual ' test_expect_success 'reverse version sort' ' git tag -l --sort=-version:refname foo* actual - cat expect EOF -foo1.10 -foo1.6 -foo1.3 -EOF + cat expect -\EOF + foo1.10 + foo1.6 + foo1.3 + EOF test_cmp expect actual ' test_expect_success 'reverse lexical sort' ' git tag -l --sort=-refname foo* actual - cat expect EOF -foo1.6 -foo1.3 -foo1.10 -EOF + cat expect -\EOF + foo1.6 + foo1.3 + foo1.10 + EOF test_cmp expect actual ' -- 2.0.1.475.g9b8d714 -- 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 v9 1/4] usage: make error functions a stack
Rename set_error_routine to be push_error_routine, and add a new
pop_error_routine. This allows temporary modifications of the error
routine over a small block of code.
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
Renamed set_error_routine to push_error_routine in order to match the
pop_error_routine.
git-compat-util.h | 3 ++-
run-command.c | 2 +-
usage.c | 32
3 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 9de318071083..b650e3cb6cdc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -343,7 +343,8 @@ static inline int const_error(void)
#endif
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err,
va_list params));
-extern void set_error_routine(void (*routine)(const char *err, va_list
params));
+extern void push_error_routine(void (*routine)(const char *err, va_list
params));
+extern void pop_error_routine(void);
extern void set_die_is_recursing_routine(int (*routine)(void));
extern int starts_with(const char *str, const char *prefix);
diff --git a/run-command.c b/run-command.c
index be07d4ad335b..a611b819c25d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -360,7 +360,7 @@ fail_pipe:
set_cloexec(child_err);
}
set_die_routine(die_child);
- set_error_routine(error_child);
+ push_error_routine(error_child);
close(notify_pipe[0]);
set_cloexec(notify_pipe[1]);
diff --git a/usage.c b/usage.c
index ed146453cabe..3fe26771f7e6 100644
--- a/usage.c
+++ b/usage.c
@@ -57,18 +57,42 @@ static int die_is_recursing_builtin(void)
* (ugh), so keep things static. */
static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) =
usage_builtin;
static NORETURN_PTR void (*die_routine)(const char *err, va_list params) =
die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
static int (*die_is_recursing)(void) = die_is_recursing_builtin;
+struct error_func_list {
+ void (*func)(const char *, va_list);
+ struct error_func_list *next;
+};
+static struct error_func_list default_error_func = { error_builtin };
+static struct error_func_list *error_funcs = default_error_func;
+
void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list
params))
{
die_routine = routine;
}
-void set_error_routine(void (*routine)(const char *err, va_list params))
+/* push error routine onto the error function stack */
+void push_error_routine(void (*routine)(const char *err, va_list params))
{
- error_routine = routine;
+ struct error_func_list *efl = xmalloc(sizeof(*efl));
+ efl-func = routine;
+ efl-next = error_funcs;
+ error_funcs = efl;
+}
+
+/* pop a single error routine off of the error function stack, thus reverting
+ * to previous error. Should always be paired with a push_error_routine */
+void pop_error_routine(void)
+{
+ struct error_func_list *efl = error_funcs;
+
+ assert(error_funcs != default_error_func);
+
+ if (efl-next) {
+ error_funcs = efl-next;
+ free(efl);
+ }
}
void set_die_is_recursing_routine(int (*routine)(void))
@@ -144,7 +168,7 @@ int error(const char *err, ...)
va_list params;
va_start(params, err);
- error_routine(err, params);
+ error_funcs-func(err, params);
va_end(params);
return -1;
}
--
2.0.1.475.g9b8d714
--
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/20] refs.c: make prune_ref use a transaction to delete the ref
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 27 ---
refs.h | 14 --
2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index dbf6af9..186df37 100644
--- a/refs.c
+++ b/refs.c
@@ -25,6 +25,11 @@ static unsigned char refname_disposition[256] = {
};
/*
+ * Used as a flag to ref_transaction_delete when a loose ref is being
+ * pruned.
+ */
+#define REF_ISPRUNING 0x0100
+/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component is
* not legal. It is legal if it is something reasonable to have under
@@ -2379,17 +2384,24 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (check_refname_format(r-name + 5, 0))
return;
- lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
- if (lock) {
- unlink_or_warn(git_path(%s, r-name));
- unlock_ref(lock);
- try_remove_empty_parents(r-name);
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_delete(transaction, r-name, r-sha1,
+ REF_ISPRUNING, 1, err) ||
+ ref_transaction_commit(transaction, NULL, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return;
}
+ ref_transaction_free(transaction);
+ try_remove_empty_parents(r-name);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3598,8 +3610,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- delnames[delnum++] = update-lock-ref_name;
ret |= delete_ref_loose(update-lock, update-type);
+ if (!(update-flags REF_ISPRUNING))
+ delnames[delnum++] = update-lock-ref_name;
}
}
diff --git a/refs.h b/refs.h
index 65dd593..aad846c 100644
--- a/refs.h
+++ b/refs.h
@@ -170,9 +170,19 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/** Locks any ref (for 'HEAD' type refs). */
+/*
+ * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),
+ * ref_transaction_create(), etc.
+ * REF_NODEREF: act on the ref directly, instead of dereferencing
+ * symbolic references.
+ *
+ * Flags = 0x100 are reserved for internal use.
+ */
#define REF_NODEREF0x01
-/* errno is set to something meaningful on failure */
+/*
+ * Locks any ref (for 'HEAD' type refs) and sets errno to something
+ * meaningful on failure.
+ */
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p);
--
2.0.1.442.g7fe6834.dirty
--
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 15/20] refs.c: make lock_ref_sha1 static
No external callers reference lock_ref_sha1 any more so lets declare it static.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
refs.h | 6 --
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index ff4e799..10cea87 100644
--- a/refs.c
+++ b/refs.c
@@ -2170,7 +2170,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
{
char refpath[PATH_MAX];
if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index 3f37c65..65dd593 100644
--- a/refs.h
+++ b/refs.h
@@ -170,12 +170,6 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/*
- * Locks a refs/ ref returning the lock on success and NULL on failure.
- * On failure errno is set to something meaningful.
- */
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1);
-
/** Locks any ref (for 'HEAD' type refs). */
#define REF_NODEREF0x01
/* errno is set to something meaningful on failure */
--
2.0.1.442.g7fe6834.dirty
--
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 07/20] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/commit.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 5e2221c..668fa6a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1627,11 +1627,12 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
- struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = parents;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (argc == 2 !strcmp(argv[1], -h))
usage_with_options(builtin_commit_usage,
builtin_commit_options);
@@ -1753,16 +1754,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_release(author_ident);
free_commit_extra_headers(extra);
- ref_lock = lock_any_ref_for_update(HEAD,
- !current_head
- ? NULL
- : current_head-object.sha1,
- 0, NULL);
- if (!ref_lock) {
- rollback_index_files();
- die(_(cannot lock HEAD ref));
- }
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(sb, nl + 1 - sb.buf);
@@ -1771,10 +1762,17 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
- if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) {
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, HEAD, sha1,
+ current_head ?
+ current_head-object.sha1 : NULL,
+ 0, !!current_head, err) ||
+ ref_transaction_commit(transaction, sb.buf, err)) {
rollback_index_files();
- die(_(cannot update HEAD ref));
+ die(%s, err.buf);
}
+ ref_transaction_free(transaction);
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
--
2.0.1.442.g7fe6834.dirty
--
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/20] walker.c: use ref transaction for ref updates
Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.
Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collisions that the previous locking would
protect against and cause the fetch to fail for are even more rare.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
walker.c | 59 +++
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..60d9f9e 100644
--- a/walker.c
+++ b/walker.c
@@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target,
const char **write_ref)
int walker_fetch(struct walker *walker, int targets, char **target,
const char **write_ref, const char *write_ref_log_details)
{
- struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+ struct strbuf ref_name = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction = NULL;
unsigned char *sha1 = xmalloc(targets * 20);
- char *msg;
- int ret;
+ char *msg = NULL;
int i;
save_commit_buffer = 0;
- for (i = 0; i targets; i++) {
- if (!write_ref || !write_ref[i])
- continue;
-
- lock[i] = lock_ref_sha1(write_ref[i], NULL);
- if (!lock[i]) {
- error(Can't lock ref %s, write_ref[i]);
- goto unlock_and_fail;
+ if (write_ref) {
+ transaction = ref_transaction_begin(err);
+ if (!transaction) {
+ error(%s, err.buf);
+ goto rollback_and_fail;
}
}
-
if (!walker-get_recover)
for_each_ref(mark_complete, NULL);
for (i = 0; i targets; i++) {
if (interpret_target(walker, target[i], sha1[20 * i])) {
error(Could not interpret response from server '%s' as
something to pull, target[i]);
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i targets; i++) {
if (!write_ref || !write_ref[i])
continue;
- ret = write_ref_sha1(lock[i], sha1[20 * i], msg ? msg : fetch
(unknown));
- lock[i] = NULL;
- if (ret)
- goto unlock_and_fail;
+ strbuf_reset(ref_name);
+ strbuf_addf(ref_name, refs/%s, write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name.buf,
+ sha1[20 * i], NULL, 0, 0,
+ err)) {
+ error(%s, err.buf);
+ goto rollback_and_fail;
+ }
+ }
+ if (write_ref) {
+ if (ref_transaction_commit(transaction,
+ msg ? msg : fetch (unknown),
+ err)) {
+ error(%s, err.buf);
+ goto rollback_and_fail;
+ }
+ ref_transaction_free(transaction);
}
- free(msg);
+ free(msg);
return 0;
-unlock_and_fail:
- for (i = 0; i targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
+rollback_and_fail:
+ ref_transaction_free(transaction);
+ free(msg);
+ strbuf_release(err);
+ strbuf_release(ref_name);
return -1;
}
--
2.0.1.442.g7fe6834.dirty
--
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/20] refs.c: update ref_transaction_delete to check for error and return status
Change ref_transaction_delete() to do basic error checking and return
non-zero of error. Update all callers to check the return for
ref_transaction_delete(). There are currently no conditions in _delete that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 5 +++--
refs.c | 16 +++-
refs.h | 12
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 41121fa..7c9c248 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(delete %s: extra input: %s, refname, next);
- ref_transaction_delete(transaction, refname, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_delete(transaction, refname, old_sha1,
+ update_flags, have_old, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index c49f1c6..40f04f4 100644
--- a/refs.c
+++ b/refs.c
@@ -3469,19 +3469,25 @@ int ref_transaction_create(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+ if (have_old !old_sha1)
+ die(BUG: have_old is true but old_sha1 is NULL);
+
+ update = add_update(transaction, refname);
update-flags = flags;
update-have_old = have_old;
if (have_old) {
assert(!is_null_sha1(old_sha1));
hashcpy(update-old_sha1, old_sha1);
}
+ return 0;
}
int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index b648819..71389a1 100644
--- a/refs.h
+++ b/refs.h
@@ -308,11 +308,15 @@ int ref_transaction_create(struct ref_transaction
*transaction,
* Add a reference deletion to transaction. If have_old is true, then
* old_sha1 holds the value that the reference should have had before
* the update (which must not be the null SHA-1).
+ * Function returns 0 on success and non-zero on failure. A failure to delete
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
*/
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err);
/*
* Commit all of the changes that have been queued in transaction, as
--
2.0.1.442.g7fe6834.dirty
--
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/20] refs.c: remove lock_ref_sha1
lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial refs/
part of the ref path name, the initial refs/ that this caller had already
stripped off before calling lock_ref_sha1.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 27c629f..dbf6af9 100644
--- a/refs.c
+++ b/refs.c
@@ -2170,15 +2170,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
-{
- char refpath[PATH_MAX];
- if (check_refname_format(refname, 0))
- return NULL;
- strcpy(refpath, mkpath(refs/%s, refname));
- return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2388,8 +2379,12 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock = lock_ref_sha1(r-name + 5, r-sha1);
+ struct ref_lock *lock;
+
+ if (check_refname_format(r-name + 5, 0))
+ return;
+ lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
if (lock) {
unlink_or_warn(git_path(%s, r-name));
unlock_ref(lock);
--
2.0.1.442.g7fe6834.dirty
--
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/20] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the state of a transaction in a new state field. Check the field for
sanity, i.e. that state must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 39 ++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 9cb7908..d015285 100644
--- a/refs.c
+++ b/refs.c
@@ -3387,6 +3387,25 @@ struct ref_update {
};
/*
+ * Transaction states.
+ * OPEN: The transaction is in a valid state and can accept new updates.
+ * An OPEN transaction can be committed.
+ * CLOSED: If an open transaction is successfully committed the state will
+ * change to CLOSED. No further changes can be made to a CLOSED
+ * transaction.
+ * CLOSED means that all updates have been successfully committed and
+ * the only thing that remains is to free the completed transaction.
+ * ERROR: The transaction has failed and is no longer committable.
+ * No further changes can be made to a CLOSED transaction and it must
+ * be rolled back using transaction_free.
+ */
+enum ref_transaction_state {
+ REF_TRANSACTION_OPEN = 0,
+ REF_TRANSACTION_CLOSED = 1,
+ REF_TRANSACTION_ERROR = 2,
+};
+
+/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
* as atomically as possible. This structure is opaque to callers.
@@ -3395,6 +3414,7 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_state state;
};
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
@@ -3437,6 +3457,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ die(BUG: update called for transaction that is not open);
+
if (have_old !old_sha1)
die(BUG: have_old is true but old_sha1 is NULL);
@@ -3457,6 +3480,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ die(BUG: create called for transaction that is not open);
+
if (!new_sha1 || is_null_sha1(new_sha1))
die(BUG: create ref with null new_sha1);
@@ -3477,6 +3503,9 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ die(BUG: delete called for transaction that is not open);
+
if (have_old !old_sha1)
die(BUG: have_old is true but old_sha1 is NULL);
@@ -3532,8 +3561,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
- if (!n)
+ if (transaction-state != REF_TRANSACTION_OPEN)
+ die(BUG: commit called for transaction that is not open);
+
+ if (!n) {
+ transaction-state = REF_TRANSACTION_CLOSED;
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3595,6 +3629,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(ref_cache);
cleanup:
+ transaction-state = ret ? REF_TRANSACTION_ERROR
+ : REF_TRANSACTION_CLOSED;
+
for (i = 0; i n; i++)
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
--
2.0.1.442.g7fe6834.dirty
--
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/20] fast-import.c: change update_branch to use ref transactions
Change update_branch() to use ref transactions for updates.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
fast-import.c | 24 ++--
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..d5206ee 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,8 +1679,9 @@ found_entry:
static int update_branch(struct branch *b)
{
static const char *msg = fast-import;
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
unsigned char old_sha1[20];
+ struct strbuf err = STRBUF_INIT;
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
@@ -1689,29 +1690,32 @@ static int update_branch(struct branch *b)
delete_ref(b-name, old_sha1, 0);
return 0;
}
- lock = lock_any_ref_for_update(b-name, old_sha1, 0, NULL);
- if (!lock)
- return error(Unable to lock %s, b-name);
if (!force_update !is_null_sha1(old_sha1)) {
struct commit *old_cmit, *new_cmit;
old_cmit = lookup_commit_reference_gently(old_sha1, 0);
new_cmit = lookup_commit_reference_gently(b-sha1, 0);
- if (!old_cmit || !new_cmit) {
- unlock_ref(lock);
+ if (!old_cmit || !new_cmit)
return error(Branch %s is missing commits., b-name);
- }
if (!in_merge_bases(old_cmit, new_cmit)) {
- unlock_ref(lock);
warning(Not updating %s
(new tip %s does not contain %s),
b-name, sha1_to_hex(b-sha1),
sha1_to_hex(old_sha1));
return -1;
}
}
- if (write_ref_sha1(lock, b-sha1, msg) 0)
- return error(Unable to update %s, b-name);
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
+ 0, 1, err) ||
+ ref_transaction_commit(transaction, msg, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return -1;
+ }
+ ref_transaction_free(transaction);
return 0;
}
--
2.0.1.442.g7fe6834.dirty
--
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/20] replace.c: use the ref transaction functions for updates
Update replace.c to use ref transactions for updates. Reviewed-by: Jonathan Nieder jrnie...@gmail.com Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/replace.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index 1bb491d..7528f3d 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -153,7 +153,8 @@ static int replace_object_sha1(const char *object_ref, unsigned char prev[20]; enum object_type obj_type, repl_type; char ref[PATH_MAX]; - struct ref_lock *lock; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; obj_type = sha1_object_info(object, NULL); repl_type = sha1_object_info(repl, NULL); @@ -166,12 +167,14 @@ static int replace_object_sha1(const char *object_ref, check_ref_valid(object, prev, ref, sizeof(ref), force); - lock = lock_any_ref_for_update(ref, prev, 0, NULL); - if (!lock) - die(%s: cannot lock the ref, ref); - if (write_ref_sha1(lock, repl, NULL) 0) - die(%s: cannot update the ref, ref); + transaction = ref_transaction_begin(err); + if (!transaction || + ref_transaction_update(transaction, ref, repl, prev, + 0, !is_null_sha1(prev), err) || + ref_transaction_commit(transaction, NULL, err)) + die(%s, err.buf); + ref_transaction_free(transaction); return 0; } -- 2.0.1.442.g7fe6834.dirty -- 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/20] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/receive-pack.c | 96 +-
1 file changed, 63 insertions(+), 33 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..91099ad 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -194,7 +194,7 @@ static void write_head_info(void)
struct command {
struct command *next;
- const char *error_string;
+ char *error_string;
unsigned int skip_update:1,
did_not_exist:1;
int index;
@@ -468,19 +468,18 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
-static const char *update(struct command *cmd, struct shallow_info *si)
+static char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd-ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
const char *namespaced_name;
unsigned char *old_sha1 = cmd-old_sha1;
unsigned char *new_sha1 = cmd-new_sha1;
- struct ref_lock *lock;
/* only refs/... are allowed */
if (!starts_with(name, refs/) || check_refname_format(name + 5, 0)) {
rp_error(refusing to create funny ref '%s' remotely, name);
- return funny refname;
+ return xstrdup(funny refname);
}
strbuf_addf(namespaced_name_buf, %s%s, get_git_namespace(), name);
@@ -498,20 +497,20 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
rp_error(refusing to update checked out branch: %s,
name);
if (deny_current_branch == DENY_UNCONFIGURED)
refuse_unconfigured_deny();
- return branch is currently checked out;
+ return xstrdup(branch is currently checked out);
}
}
if (!is_null_sha1(new_sha1) !has_sha1_file(new_sha1)) {
error(unpack should have generated %s,
but I can't find it!, sha1_to_hex(new_sha1));
- return bad pack;
+ return xstrdup(bad pack);
}
if (!is_null_sha1(old_sha1) is_null_sha1(new_sha1)) {
if (deny_deletes starts_with(name, refs/heads/)) {
rp_error(denying ref deletion for %s, name);
- return deletion prohibited;
+ return xstrdup(deletion prohibited);
}
if (!strcmp(namespaced_name, head_name)) {
@@ -526,7 +525,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
if (deny_delete_current == DENY_UNCONFIGURED)
refuse_unconfigured_deny_delete_current();
rp_error(refusing to delete the current
branch: %s, name);
- return deletion of the current branch
prohibited;
+ return xstrdup(deletion of the current branch
prohibited);
}
}
}
@@ -544,19 +543,19 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
old_object-type != OBJ_COMMIT ||
new_object-type != OBJ_COMMIT) {
error(bad sha1 objects for %s, name);
- return bad ref;
+ return xstrdup(bad ref);
}
old_commit = (struct commit *)old_object;
new_commit = (struct commit *)new_object;
if (!in_merge_bases(old_commit, new_commit)) {
rp_error(denying non-fast-forward %s
(you should pull first), name);
- return non-fast-forward;
+ return xstrdup(non-fast-forward);
}
}
if (run_update_hook(cmd)) {
rp_error(hook declined to update %s, name);
- return hook declined;
+ return xstrdup(hook declined);
}
if (is_null_sha1(new_sha1)) {
@@ -571,24 +570,32 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
}
if (delete_ref(namespaced_name, old_sha1, 0)) {
rp_error(failed to delete %s, name);
- return failed to delete;
+ return xstrdup(failed to delete);
}
return NULL; /* good */
}
else {
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+
if (shallow_update si-shallow_ref[cmd-index]
update_shallow_ref(cmd, si))
- return shallow error;
-
-
[PATCH 13/20] fast-import.c: use a ref transaction when dumping tags
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
fast-import.c | 29 +++--
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index d5206ee..a95e1be 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1734,15 +1734,32 @@ static void dump_tags(void)
{
static const char *msg = fast-import;
struct tag *t;
- struct ref_lock *lock;
- char ref_name[PATH_MAX];
+ struct strbuf ref_name = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+ transaction = ref_transaction_begin(err);
+ if (!transaction) {
+ failure |= error(%s, err.buf);
+ goto cleanup;
+ }
for (t = first_tag; t; t = t-next_tag) {
- sprintf(ref_name, tags/%s, t-name);
- lock = lock_ref_sha1(ref_name, NULL);
- if (!lock || write_ref_sha1(lock, t-sha1, msg) 0)
- failure |= error(Unable to update %s, ref_name);
+ strbuf_reset(ref_name);
+ strbuf_addf(ref_name, refs/tags/%s, t-name);
+
+ if (ref_transaction_update(transaction, ref_name.buf, t-sha1,
+ NULL, 0, 0, err)) {
+ failure |= error(%s, err.buf);
+ goto cleanup;
+ }
}
+ if (ref_transaction_commit(transaction, msg, err))
+ failure |= error(%s, err.buf);
+
+ cleanup:
+ ref_transaction_free(transaction);
+ strbuf_release(ref_name);
+ strbuf_release(err);
}
static void dump_marks_helper(FILE *f,
--
2.0.1.442.g7fe6834.dirty
--
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 20/20] refs.c: make delete_ref use a transaction
Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.
Change delete_ref to return 0 on success and 1 on failure instead of the
previous 0 on success either 1 or -1 on failure.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 34 +-
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 186df37..0017d9c 100644
--- a/refs.c
+++ b/refs.c
@@ -2544,11 +2544,6 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
return ret;
}
-static int repack_without_ref(const char *refname)
-{
- return repack_without_refs(refname, 1, NULL);
-}
-
static int delete_ref_loose(struct ref_lock *lock, int flag)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
@@ -2566,24 +2561,21 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
{
- struct ref_lock *lock;
- int ret = 0, flag = 0;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
- lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
- if (!lock)
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_delete(transaction, refname, sha1, delopt,
+ sha1 !is_null_sha1(sha1), err) ||
+ ref_transaction_commit(transaction, NULL, err)) {
+ error(%s, err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(err);
return 1;
- ret |= delete_ref_loose(lock, flag);
-
- /* removing the loose one could have resurrected an earlier
-* packed one. Also, if it was not loose we need to repack
-* without it.
-*/
- ret |= repack_without_ref(lock-ref_name);
-
- unlink_or_warn(git_path(logs/%s, lock-ref_name));
- clear_loose_ref_cache(ref_cache);
- unlock_ref(lock);
- return ret;
+ }
+ ref_transaction_free(transaction);
+ return 0;
}
/*
--
2.0.1.442.g7fe6834.dirty
--
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/20] refs.c: remove the update_ref_write function
Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly. This changes the return status for _commit from
1 to -1 on failures when writing to the ref. Eventually we will want
_commit to start returning more detailed error conditions than the current
simple success/failure. For example if the commit failed due to name
conflicts etc.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 35 +--
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 091c343..27c629f 100644
--- a/refs.c
+++ b/refs.c
@@ -,25 +,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static int update_ref_write(const char *action, const char *refname,
- const unsigned char *sha1, struct ref_lock *lock,
- struct strbuf *err, enum action_on_err onerr)
-{
- if (write_ref_sha1(lock, sha1, action) 0) {
- const char *str = Cannot update the ref '%s'.;
- if (err)
- strbuf_addf(err, str, refname);
-
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- return 1;
- }
- return 0;
-}
-
/**
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
@@ -3604,14 +3585,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update-new_sha1)) {
- ret = update_ref_write(msg,
- update-refname,
- update-new_sha1,
- update-lock, err,
- UPDATE_REFS_QUIET_ON_ERR);
- update-lock = NULL; /* freed by update_ref_write */
- if (ret)
+ ret = write_ref_sha1(update-lock, update-new_sha1,
+msg);
+ update-lock = NULL; /* freed by write_ref_sha1 */
+ if (ret) {
+ const char *str = Cannot update the ref '%s'.;
+
+ if (err)
+ strbuf_addf(err, str, update-refname);
goto cleanup;
+ }
}
}
--
2.0.1.442.g7fe6834.dirty
--
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 11/20] refs.c: change update_ref to use a transaction
Change the update_ref helper function to use a ref transaction internally.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 28
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index d015285..ff4e799 100644
--- a/refs.c
+++ b/refs.c
@@ -3523,11 +3523,31 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr)
{
- struct ref_lock *lock;
- lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
- if (!lock)
+ struct ref_transaction *t;
+ struct strbuf err = STRBUF_INIT;
+
+ t = ref_transaction_begin(err);
+ if (!t ||
+ ref_transaction_update(t, refname, sha1, oldval, flags,
+ !!oldval, err) ||
+ ref_transaction_commit(t, action, err)) {
+ const char *str = update_ref failed for ref '%s': %s;
+
+ ref_transaction_free(t);
+ switch (onerr) {
+ case UPDATE_REFS_MSG_ON_ERR:
+ error(str, refname, err.buf);
+ break;
+ case UPDATE_REFS_DIE_ON_ERR:
+ die(str, refname, err.buf);
+ break;
+ case UPDATE_REFS_QUIET_ON_ERR:
+ break;
+ }
+ strbuf_release(err);
return 1;
- return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+ }
+ return 0;
}
static int ref_update_compare(const void *r1, const void *r2)
--
2.0.1.442.g7fe6834.dirty
--
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 01/20] refs.c: change ref_transaction_create to do error checking and return status
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 4 +++-
refs.c | 18 --
refs.h | 48 +---
3 files changed, 56 insertions(+), 14 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3067b11..41121fa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(create %s: extra input: %s, refname, next);
- ref_transaction_create(transaction, refname, new_sha1, update_flags);
+ if (ref_transaction_create(transaction, refname, new_sha1,
+ update_flags, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 3f05e88..c49f1c6 100644
--- a/refs.c
+++ b/refs.c
@@ -3449,18 +3449,24 @@ int ref_transaction_update(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die(BUG: create ref with null new_sha1);
+
+ update = add_update(transaction, refname);
- assert(!is_null_sha1(new_sha1));
hashcpy(update-new_sha1, new_sha1);
hashclr(update-old_sha1);
update-flags = flags;
update-have_old = 1;
+ return 0;
}
void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index c5376ce..b648819 100644
--- a/refs.h
+++ b/refs.h
@@ -10,6 +10,38 @@ struct ref_lock {
int force_write;
};
+/*
+ * A ref_transaction represents a collection of ref updates
+ * that should succeed or fail together.
+ *
+ * Calling sequence
+ *
+ * - Allocate and initialize a `struct ref_transaction` by calling
+ * `ref_transaction_begin()`.
+ *
+ * - List intended ref updates by calling functions like
+ * `ref_transaction_update()` and `ref_transaction_create()`.
+ *
+ * - Call `ref_transaction_commit()` to execute the transaction.
+ * If this succeeds, the ref updates will have taken place and
+ * the transaction cannot be rolled back.
+ *
+ * - At any time call `ref_transaction_free()` to discard the
+ * transaction and free associated resources. In particular,
+ * this rolls back the transaction if it has not been
+ * successfully committed.
+ *
+ * Error handling
+ * --
+ *
+ * On error, transaction functions append a message about what
+ * went wrong to the 'err' argument. The message mentions what
+ * ref was being updated (if any) when the error occurred so it
+ * can be passed to 'die' or 'error' as-is.
+ *
+ * The message is appended to err without first clearing err.
+ * err will not be '\n' terminated.
+ */
struct ref_transaction;
/*
@@ -248,7 +280,7 @@ struct ref_transaction *ref_transaction_begin(void);
* it must not have existed beforehand.
* Function returns 0 on success and non-zero on failure. A failure to update
* means that the transaction as a whole has failed and will need to be
- * rolled back. On failure the err buffer will be updated.
+ * rolled back.
*/
int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
@@ -262,11 +294,15 @@ int ref_transaction_update(struct ref_transaction
*transaction,
* that the reference should have after the update; it must not be the
* null SHA-1. It is verified that the reference does not exist
* already.
+ * Function returns 0 on success and non-zero on failure. A failure to create
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+ const
[PATCH 10/20] branch.c: use ref transaction for all ref updates
Change create_branch to use a ref transaction when creating the new branch.
This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 30 --
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..c1eae00 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
int force, int reflog, int clobber_head,
int quiet, enum branch_track track)
{
- struct ref_lock *lock = NULL;
struct commit *commit;
unsigned char sha1[20];
char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
die(_(Not a valid branch point: '%s'.), start_name);
hashcpy(sha1, commit-object.sha1);
- if (!dont_change_ref) {
- lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
- if (!lock)
- die_errno(_(Failed to lock ref for update));
- }
-
- if (reflog)
- log_all_ref_updates = 1;
-
if (forcing)
snprintf(msg, sizeof msg, branch: Reset to %s,
start_name);
@@ -301,13 +291,25 @@ void create_branch(const char *head,
snprintf(msg, sizeof msg, branch: Created from %s,
start_name);
+ if (reflog)
+ log_all_ref_updates = 1;
+
+ if (!dont_change_ref) {
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+
+ 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))
+ die(%s, err.buf);
+ ref_transaction_free(transaction);
+ }
+
if (real_ref track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
- if (!dont_change_ref)
- if (write_ref_sha1(lock, sha1, msg) 0)
- die_errno(_(Failed to write ref));
-
strbuf_release(ref);
free(real_ref);
}
--
2.0.1.442.g7fe6834.dirty
--
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 00/20] ref transactions part 2
This is the next 20 patches from my originally big patch series and follow the previous 19 patches that is now in juns tree. These patches were numbered 20-39 in the original 48-patch series. Changes since these patches were in the original series: - Addressing concerns from mhagger's review Ronnie Sahlberg (20): refs.c: change ref_transaction_create to do error checking and return status refs.c: update ref_transaction_delete to check for error and return status refs.c: make ref_transaction_begin take an err argument refs.c: add transaction.status and track OPEN/CLOSED/ERROR tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make lock_ref_sha1 static refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction branch.c | 30 +++--- builtin/commit.c | 24 +++-- builtin/receive-pack.c | 96 +--- builtin/replace.c | 15 +-- builtin/tag.c | 15 +-- builtin/update-ref.c | 11 ++- fast-import.c | 53 +++ refs.c | 242 - refs.h | 78 sequencer.c| 27 -- walker.c | 59 +++- 11 files changed, 403 insertions(+), 247 deletions(-) -- 2.0.1.442.g7fe6834.dirty -- 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/20] sequencer.c: use ref transactions for all ref updates
Change to use ref transactions for all updates to refs.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
sequencer.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 9230474..cf17c69 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,32 @@ static int error_dirty_index(struct replay_opts *opts)
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
- struct ref_lock *ref_lock;
+ struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
- int ret;
+ struct strbuf err = STRBUF_INIT;
read_cache();
if (checkout_fast_forward(from, to, 1))
- exit(128); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
- 0, NULL);
- if (!ref_lock)
- return error(_(Failed to lock HEAD during fast_forward_to));
+ exit(1); /* the callee should have complained already */
strbuf_addf(sb, %s: fast-forward, action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, HEAD,
+ to, unborn ? null_sha1 : from,
+ 0, 1, err) ||
+ ref_transaction_commit(transaction, sb.buf, err)) {
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(sb);
+ strbuf_release(err);
+ return -1;
+ }
strbuf_release(sb);
- return ret;
+ ref_transaction_free(transaction);
+ return 0;
}
static int do_recursive_merge(struct commit *base, struct commit *next,
--
2.0.1.442.g7fe6834.dirty
--
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/20] tag.c: use ref transactions when doing updates
Change tag.c to use ref transactions for all ref updates.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/tag.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..1aa88a2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
- struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_CMDMODE('l', list, cmdmode, N_(list tag names), 'l'),
{ OPTION_INTEGER, 'n', NULL, lines, N_(n),
@@ -701,11 +702,13 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
- lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
- if (!lock)
- die(_(%s: cannot lock the ref), ref.buf);
- if (write_ref_sha1(lock, object, NULL) 0)
- die(_(%s: cannot update the ref), ref.buf);
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, object, prev,
+ 0, 1, err) ||
+ ref_transaction_commit(transaction, NULL, err))
+ die(%s, err.buf);
+ ref_transaction_free(transaction);
if (force !is_null_sha1(prev) hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
--
2.0.1.442.g7fe6834.dirty
--
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/20] refs.c: make ref_transaction_begin take an err argument
Add an err argument to _begin so that on non-fatal failures in future ref
backends we can report a nice error back to the caller.
While _begin can currently never fail for other reasons than OOM, in which
case we die() anyway, we may add other types of backends in the future.
For example, a hypothetical MySQL backend could fail in _begin with
Can not connect to MySQL server. No route to host.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 2 +-
refs.c | 2 +-
refs.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7c9c248..c6ad0be 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -365,7 +365,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
die(Refusing to perform update with empty message.);
if (read_stdin) {
- transaction = ref_transaction_begin();
+ transaction = ref_transaction_begin(err);
if (delete || no_deref || argc 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
diff --git a/refs.c b/refs.c
index 40f04f4..9cb7908 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,7 +3397,7 @@ struct ref_transaction {
size_t nr;
};
-struct ref_transaction *ref_transaction_begin(void)
+struct ref_transaction *ref_transaction_begin(struct strbuf *err)
{
return xcalloc(1, sizeof(struct ref_transaction));
}
diff --git a/refs.h b/refs.h
index 71389a1..3f37c65 100644
--- a/refs.h
+++ b/refs.h
@@ -262,7 +262,7 @@ enum action_on_err {
* Begin a reference transaction. The reference transaction must
* be freed by calling ref_transaction_free().
*/
-struct ref_transaction *ref_transaction_begin(void);
+struct ref_transaction *ref_transaction_begin(struct strbuf *err);
/*
* The following functions add a reference check or update to a
--
2.0.1.442.g7fe6834.dirty
--
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 00/20] ref transactions part 2
Hi Michael, Here is the next set of 20 patches of those you already reviewed. I cut this patch off just before patch #40 in the previous series. These are the patches numbered 20-39 in the original series. I think I have addressed your concerns here so If you could take a quick look and hopefully bless them that would be awesome! Thanks ronnie sahlberg On Tue, Jul 15, 2014 at 4:33 PM, Ronnie Sahlberg sahlb...@google.com wrote: This is the next 20 patches from my originally big patch series and follow the previous 19 patches that is now in juns tree. These patches were numbered 20-39 in the original 48-patch series. Changes since these patches were in the original series: - Addressing concerns from mhagger's review Ronnie Sahlberg (20): refs.c: change ref_transaction_create to do error checking and return status refs.c: update ref_transaction_delete to check for error and return status refs.c: make ref_transaction_begin take an err argument refs.c: add transaction.status and track OPEN/CLOSED/ERROR tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make lock_ref_sha1 static refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction branch.c | 30 +++--- builtin/commit.c | 24 +++-- builtin/receive-pack.c | 96 +--- builtin/replace.c | 15 +-- builtin/tag.c | 15 +-- builtin/update-ref.c | 11 ++- fast-import.c | 53 +++ refs.c | 242 - refs.h | 78 sequencer.c| 27 -- walker.c | 59 +++- 11 files changed, 403 insertions(+), 247 deletions(-) -- 2.0.1.442.g7fe6834.dirty -- 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 3/3] tag: support configuring --sort via .gitconfig
On Tue, Jul 15, 2014 at 09:03:53AM -0700, Junio C Hamano wrote: Do we want to go this way? I do not speak for Peff, but I personally think this is just a fun demonstration, nothing more, and my gut feeling is that it would make things unnecessary complex without much real gain to pursue it further. Yeah, it is a little too complicated for what it is buying us here. If we had a real error stack with allocated error types or something, then callers could do more useful programmatic things with it. But: 1. We usually only care about system errors in such a case, and get by with using errno. 2. I would not want to annotate all of the library-ish functions with case-specific return types. That is a lot of work for little gain. I think my favorite of the suggestions so far is basically the two-line: error: sort specification is bad... warning: ignoring invalid tag.sort There are tons of places in git where we already do this kind of error chaining over multiple lines (and multiple calls to error()), and it doesn't require any new code or techniques. But what is in v8 is not so bad from my cursory glance. -Peff PS I am traveling this week and will probably be a lot less responsive. Please don't let me hold up your conversations. -- 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 16/20] refs.c: remove the update_ref_lock function
Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index 10cea87..091c343 100644
--- a/refs.c
+++ b/refs.c
@@ -,24 +,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static struct ref_lock *update_ref_lock(const char *refname,
- const unsigned char *oldval,
- int flags, int *type_p,
- enum action_on_err onerr)
-{
- struct ref_lock *lock;
- lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
- if (!lock) {
- const char *str = Cannot lock the ref '%s'.;
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- }
- return lock;
-}
-
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
struct strbuf *err, enum action_on_err onerr)
@@ -3602,12 +3584,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = update_ref_lock(update-refname,
- (update-have_old ?
- update-old_sha1 : NULL),
- update-flags,
- update-type,
- UPDATE_REFS_QUIET_ON_ERR);
+ update-lock = lock_any_ref_for_update(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.0.1.442.g7fe6834.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 4/4] tag: support configuring --sort via .gitconfig
On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:
+static void error_bad_sort_config(const char *err, va_list params)
+{
+ vreportf(warning: tag.sort has , err, params);
+}
This feels a little like an abuse of the prefix field of vreportf, but
as you probably saw in my for fun patch, doing it right means
formatting into a buffer and then reformatting that (which we're
already doing again in vreportf, but less flexibly). I dunno.
At any rate, this should be marked for translation, no?
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] MinGW: fix compile error due to missing ELOOP
Karsten Blees wrote: MinGW and MSVC before 2010 don't define ELOOP, use EMLINK (aka Too many links) instead. [...] +#ifndef ELOOP +#define ELOOP EMLINK +#endif This could use #define ELOOP WSAELOOP as an alternative. But it shouldn't matter since git doesn't look for EMLINK anywhere (EMLINK = 31, WSAELOOP = wsabaseerr+62 = 10062). 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 v9 4/4] tag: support configuring --sort via .gitconfig
On Tue, 2014-07-15 at 19:42 -0400, Jeff King wrote:
On Tue, Jul 15, 2014 at 04:32:59PM -0700, Jacob Keller wrote:
+static void error_bad_sort_config(const char *err, va_list params)
+{
+ vreportf(warning: tag.sort has , err, params);
+}
This feels a little like an abuse of the prefix field of vreportf, but
as you probably saw in my for fun patch, doing it right means
formatting into a buffer and then reformatting that (which we're
already doing again in vreportf, but less flexibly). I dunno.
At any rate, this should be marked for translation, no?
-Peff
Oh, yes it probably should. It's definitely a little bit abusive of the
prefix field, but that seemed easier.
Thanks,
Jake
