Re: The different EOL behavior between libgit2-based software and official Git
On 06/19/2014 04:59 AM, Yue Lin Ho wrote: Hi: ^_^ I did some test on the EOL behavior between official git and libgit2-based software(TortoiseGit). Then, I got that they have different EOL behavior. The blob stored in repository is a text file with mixed EOLs. Even set core.autocrlf = true, official git checkout the file as it is(means still *mixed EOLs* there). But, libgit2 checkout it with *All CRLF EOLs*. * The steps: * set core.autocrlf = false * add file with mixed EOLs * set core.autocrlf = true * delete that file in the working tree * checkout that file * examine the EOL If you are interested in this, you might take a look at my testing repository on GitHub. (https://github.com/YueLinHo/TestAutoCrlf) Thank you. Yue Lin Ho (I send a similar mail to msysgit, I'm not sure if this came trough) Sorry being late, I don't think there is something wrong with Git. The core.autocrlf is the old crlf handling, which has been in Git for a long time. If you exactly know what you are doing, know exactly which tools are doing what, convince everybody who pulls or pushes to that repo to use the same local config then it may be useful. In short: I would strongly recommend to use gitattributes, please see below. tb@msygit ~/temp $ git clone https://github.com/YueLinHo/TestAutoCrlf.git Cloning into 'TestAutoCrlf'... [snip] $ cd TestAutoCrlf/ tb@msygit ~/temp/TestAutoCrlf (master) $ ls CRLF.txt LF.txt MIX-more_CRLF.txt MIX-more_LF.txt Readme.md ## Check how the file looks like: $ od -c MIX-more_LF.txt 000 L i n e 1 \n l i n e ( 2 ) \r 020 \n l i n e 3 . \n t h i s i s 040 l i n e 4 \n l i n e 060 N o . 5 \n L i n e N u m b e 100 r 6 \n 104 ### The file has one CRLF, the rest is LF, exactly how it had been ### commited. So this is what we expect. Or do I miss something ? ### Tell Git that MIX-more_LF.txt is a text file: $ echo MIX-more_LF.txt text .gitattributes ### Verify that MIX-more_LF.txt is text, all other files ### are binary (or -text in Git language) $ git check-attr text * CRLF.txt: text: unspecified LF.txt: text: unspecified MIX-more_CRLF.txt: text: unspecified MIX-more_LF.txt: text: set Readme.md: text: unspecified ### Now ask Git to normalize the line endings in the working tree $ rm MIX-more_LF.txt tb@msygit ~/temp/TestAutoCrlf (master) $ git checkout MIX-more_LF.txt ## Check what we got: tb@msygit ~/temp/TestAutoCrlf (master) $ od -c MIX-more_LF.txt 000 L i n e 1 \r \n l i n e ( 2 ) 020 \r \n l i n e 3 . \r \n t h i s 040 i s l i n e 4 \r \n l i n 060 e N o . 5 \r \n L i n e N 100 u m b e r 6 \r \n 111 # (This is under Windows, under Linux I would expect only LF) # See core.eol for for information ## ## Now we need to normalize the file in the repo, ## All line endings should be LF, otherwise Git ## things the file is modified: $ git status On branch master Your branch is up-to-date with 'origin/master'. Changes not staged for commit: (use git add file... to update what will be committed) (use git checkout -- file... to discard changes in working directory) modified: MIX-more_LF.txt Untracked files: (use git add file... to include in what will be committed) .gitattributes no changes added to commit (use git add and/or git commit -a) ### ### Do the normalization: tb@msygit ~/temp/TestAutoCrlf (master) $ git add MIX-more_LF.txt .gitattributes tb@msygit ~/temp/TestAutoCrlf (master) $ git commit -m MIX-more_LF.txt is text [master 200d874] MIX-more_LF.txt is text Committer: unknown tb@msysgit Your name and email address were configured automatically based on your username and hostname. Please check that they are accurate. You can suppress this message by setting them explicitly: git config --global user.name Your Name git config --global user.email y...@example.com After doing this, you may fix the identity used for this commit with: git commit --amend --reset-author 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 .gitattributes tb@msygit ~/temp/TestAutoCrlf (master) $ From now on, MIX-more_LF.txt is treated as text by Git, and get CRLF under Windows. I think there is nothing wrong with Git here. If libgit2 does something different, we need to ask the libgit2 project, which is independent from Git -- 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: The different EOL behavior between libgit2-based software and official Git
Hi Torsten Bögershausen: Since you mail to msysGit first, I reply there. (https://groups.google.com/forum/#!topic/msysgit/EDD3RipNeBQ) Thank you again. ^_^ Yue Lin Ho -- View this message in context: http://git.661346.n2.nabble.com/The-different-EOL-behavior-between-libgit2-based-software-and-official-Git-tp7613670p7613681.html Sent from the git mailing list archive at Nabble.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 v3 2/5] tree-walk.c: remove name_compare() function
Jonathan, On Wed, Jun 18, 2014 at 12:03:59PM -0700, Jonathan Nieder wrote: Jeremiah Mahler wrote: Remove the duplicate name_compare() function and use the one provided by read-cache.c. I'd squash this into patch 1/5. --- Notes: There is one small difference between the old function and the new one. The old one returned -N and +N whereas the new one returns -1 and +1. However, there is no place where the magnitude was needed, so this change will not alter its behavior. This is useful information for anyone looking back at the patch in the future, so it belongs above the three-dash divider. Makes sense. I will add it to the log message. Thanks, Jonathan Thanks, -- Jeremiah Mahler jmmah...@gmail.com http://github.com/jmahler -- 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 0/5] cleanup duplicate name_compare() functions
Jonathan, On Wed, Jun 18, 2014 at 12:14:07PM -0700, Jonathan Nieder wrote: Jeremiah Mahler wrote: Jeremiah Mahler (5): cache: rename cache_name_compare() to name_compare() tree-walk.c: remove name_compare() function unpack-trees.c: remove name_compare() function dir.c: rename to name_compare() name-hash.c: rename to name_compare() cache.h| 2 +- dir.c | 3 +-- name-hash.c| 2 +- read-cache.c | 23 +-- tree-walk.c| 10 -- unpack-trees.c | 11 --- 6 files changed, 16 insertions(+), 35 deletions(-) After looking at the patches I suspect this should be a single patch. That way it's bisectable, and the changes outside of read-cache.c are small enough that it's not too much of a burden to review as a single patch. That would be a pain to bisect if the partial application of the patch set left the system in a broken state. Good suggestion. The code change looked good. Thanks and hope that helps, Jonathan Thanks, -- Jeremiah Mahler jmmah...@gmail.com http://github.com/jmahler -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4] cleanup duplicate name_compare() functions
Version 4 of the patch series to cleanup the duplicate name_compare() functions. The previous patch series would have left the system in a broken state if only part of the patches were applied. This version condenses all the patches in to a single working patch as Jonathan Nieder suggested [1]. [1]: http://marc.info/?l=gitm=140311885416615w=2 Jeremiah Mahler (1): cleanup duplicate name_compare() functions cache.h| 2 +- dir.c | 3 +-- name-hash.c| 2 +- read-cache.c | 23 +-- tree-walk.c| 10 -- unpack-trees.c | 11 --- 6 files changed, 16 insertions(+), 35 deletions(-) -- 2.0.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4] cleanup duplicate name_compare() functions
Both unpack-trees.c and read-cache.c have their own name_compare()
function, which are identical. And read-cache.c has a
cache_name_compare() function which is nearly identical to
name_compare() [1]. The cache_name_compare() function is not specific
to a cache, other than by being part of cache.h.
Generalize the cache_name_compare() function by renaming it to
name_compare(). Simplify the cache_name_stage_compare() function using
name_compare(). Then change the few instances which used
cache_name_compare() to name_compare() [2].
[1] cache_name_compare() is not identical to name_compare(). The former
returns +1, -1, whereas the latter returns +N, -N. But there is no
place where name_compare() is used that needs the magnitude so this
difference does not alter its behavior.
[2] The instances where cache_name_compare() is used have nothing to do
with a cache. The new name, name_compare(), makes it clear that no
cache is involved.
Signed-off-by: Jeremiah Mahler jmmah...@gmail.com
---
cache.h| 2 +-
dir.c | 3 +--
name-hash.c| 2 +-
read-cache.c | 23 +--
tree-walk.c| 10 --
unpack-trees.c | 11 ---
6 files changed, 16 insertions(+), 35 deletions(-)
diff --git a/cache.h b/cache.h
index c498a30..e3205fe 100644
--- a/cache.h
+++ b/cache.h
@@ -1027,7 +1027,7 @@ extern int validate_headref(const char *ref);
extern int base_name_compare(const char *name1, int len1, int mode1, const
char *name2, int len2, int mode2);
extern int df_name_compare(const char *name1, int len1, int mode1, const char
*name2, int len2, int mode2);
-extern int cache_name_compare(const char *name1, int len1, const char *name2,
int len2);
+extern int name_compare(const char *name1, size_t len1, const char *name2,
size_t len2);
extern int cache_name_stage_compare(const char *name1, int len1, int stage1,
const char *name2, int len2, int stage2);
extern void *read_object_with_reference(const unsigned char *sha1,
diff --git a/dir.c b/dir.c
index 797805d..e65888d 100644
--- a/dir.c
+++ b/dir.c
@@ -1354,8 +1354,7 @@ static int cmp_name(const void *p1, const void *p2)
const struct dir_entry *e1 = *(const struct dir_entry **)p1;
const struct dir_entry *e2 = *(const struct dir_entry **)p2;
- return cache_name_compare(e1-name, e1-len,
- e2-name, e2-len);
+ return name_compare(e1-name, e1-len, e2-name, e2-len);
}
static struct path_simplify *create_simplify(const char **pathspec)
diff --git a/name-hash.c b/name-hash.c
index be7c4ae..e2bea88 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -179,7 +179,7 @@ static int same_name(const struct cache_entry *ce, const
char *name, int namelen
* Always do exact compare, even if we want a case-ignoring comparison;
* we do the quick exact one first, because it will be the common case.
*/
- if (len == namelen !cache_name_compare(name, namelen, ce-name, len))
+ if (len == namelen !name_compare(name, namelen, ce-name, len))
return 1;
if (!icase)
diff --git a/read-cache.c b/read-cache.c
index 9f56d76..158241d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -434,18 +434,26 @@ int df_name_compare(const char *name1, int len1, int
mode1,
return c1 - c2;
}
-int cache_name_stage_compare(const char *name1, int len1, int stage1, const
char *name2, int len2, int stage2)
+int name_compare(const char *name1, size_t len1, const char *name2, size_t
len2)
{
- int len = len1 len2 ? len1 : len2;
- int cmp;
-
- cmp = memcmp(name1, name2, len);
+ size_t min_len = (len1 len2) ? len1 : len2;
+ int cmp = memcmp(name1, name2, min_len);
if (cmp)
return cmp;
if (len1 len2)
return -1;
if (len1 len2)
return 1;
+ return 0;
+}
+
+int cache_name_stage_compare(const char *name1, int len1, int stage1, const
char *name2, int len2, int stage2)
+{
+ int cmp;
+
+ cmp = name_compare(name1, len1, name2, len2);
+ if (cmp)
+ return cmp;
if (stage1 stage2)
return -1;
@@ -454,11 +462,6 @@ int cache_name_stage_compare(const char *name1, int len1,
int stage1, const char
return 0;
}
-int cache_name_compare(const char *name1, int len1, const char *name2, int
len2)
-{
- return cache_name_stage_compare(name1, len1, 0, name2, len2, 0);
-}
-
static int index_name_stage_pos(const struct index_state *istate, const char
*name, int namelen, int stage)
{
int first, last;
diff --git a/tree-walk.c b/tree-walk.c
index 4dc86c7..5dd9a71 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -144,16 +144,6 @@ struct tree_desc_x {
struct tree_desc_skip *skip;
};
-static int name_compare(const char *a, int a_len,
- const char *b, int b_len)
-{
- int len = (a_len b_len) ? a_len : b_len;
- int
Re: [PATCH v3 0/5] cleanup duplicate name_compare() functions
On Thu, Jun 19, 2014 at 01:04:32AM -0700, Jeremiah Mahler wrote: After looking at the patches I suspect this should be a single patch. That way it's bisectable, and the changes outside of read-cache.c are small enough that it's not too much of a burden to review as a single patch. That would be a pain to bisect if the partial application of the patch set left the system in a broken state. Good suggestion. One trick I use, especially when refactoring, is to use an interactive rebase to test each commit in isolation, like: GIT_EDITOR='sed -i /^pick .*/aexec make -j8 test' git rebase -i After picking each commit, that will run the tests on each one[1]. If it fails, the rebase will pause. You can fix any problems, test to your satisfaction, commit --amend, and then rebase --continue to keep going. -Peff [1] Of course it can be rather time-consuming for a large series. I often just compile-test at first, and then do a final make test pass when I think everything is right. -- 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] strbuf: add xstrdup_fmt helper
On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote: str = xstrdup_fmt(fmt, some, args); --- I'm open to suggestions on the name. This really is the same thing conceptually as the GNU asprintf(), but the interface is different (that function takes a pointer-to-pointer as an out-parameter, and returns the number of characters return). Naming it with anything dup certainly feels wrong. The returned string is not a duplicate of anything. I was somewhat inspired by mkpathdup and friends. It is quite simialr to mkpathdup, except that it is not limited to paths (and does not do any path-specific munging). I agree something with printf in the name is probably more descriptive, though. I wonder if our callers can instead use asprintf(3) with its slightly more quirky API (and then we supply compat/asprintf.c for non-GNU platforms). Right now we only have three call sites, but if we anticipate that printf-like format into an allocated piece of memory will prove be generally useful in our code base, following an API that other people already have established may give our developers one less thing that they have to learn. I considered that, but I do find asprintf's interface unnecessarily annoying (you can't return a value directly, and you run afoul of const issues when passing pointers to pointers). As you noted, it's not _too_ bad, but we really get nothing from the integer return type. AFAICT, it helps with: 1. You know how many characters are in the string. If you cared about that here, you would just use a strbuf directly, which is much more flexible. 2. The error handling is different. But our x-variant would just die() on errors anyway, so we do not care. So modeling after asprintf feels like carrying around unnecessary baggage (and I am not convinced asprintf is in wide enough use, nor that the function is complicated enough to care about developer familiarity). Part of me is tempted to call it xasprintf anyway, and use our own interface. GNU does not get to squat on that useful name. ;) That is probably unnecessarily confusing, though. As usual, I would expect we would have xasprintf wrapper around it to die instead of returning -1 upon allocation failure. Would it be crazy to just call it xprintf? By our current scheme that would be a wrapper for printf, which means it is potentially confusing. Literally any other unused letter would be fine. dprintf for detach? I dunno. -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] alloc.c: remove alloc_raw_commit_node() function
On Wed, Jun 18, 2014 at 11:30:50PM +0100, Ramsay Jones wrote: So, the patch below is a slight variation on the original patch. I'm still slightly concerned about portability, but given that it has been at least a decade since I last used a (pre-ANSI) compiler which had a problem with this ... For a while some people were compiling git with pretty antique compilers, but I do not know if that is the case any more (Junio noted recently that we have had trailing commas on arrays and enums in builtin/clean.c for the past year, and nobody has complained). Maybe those systems have all died off, or maybe those people only upgrade every few years, and we are due for an onslaught of portability fixes soon. :) [I have several versions of the C standard that I can use to check up on the legalise, but I'm not sure I can be bothered! ;-) ] It was actually pretty easy to find. I only have C99, but empty macro arguments are listed as one of the changes since C89 in the foreward. diff --git a/alloc.c b/alloc.c index eb22a45..5392d13 100644 --- a/alloc.c +++ b/alloc.c @@ -18,9 +18,12 @@ #define BLOCKING 1024 -#define DEFINE_ALLOCATOR(name, type) \ +#define PUBLIC +#define PRIVATE static + +#define DEFINE_ALLOCATOR(scope, name, type) \ I am not sure offhand whether this is more portable or not (it would depend on order of macro expansion, and I am not brave enough to read through the preprocessor section of the standard carefully). Quick reading online suggests that it's OK, but that an extra level of macro expansion would not be. And of course the Internet is never wrong. :) I'm inclined to go with it, and deal with it later if we get a portability complaint (at which point we are no worse off than we are right now). -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] alloc.c: remove alloc_raw_commit_node() function
On 19/06/14 10:19, Jeff King wrote: On Wed, Jun 18, 2014 at 11:30:50PM +0100, Ramsay Jones wrote: So, the patch below is a slight variation on the original patch. I'm still slightly concerned about portability, but given that it has been at least a decade since I last used a (pre-ANSI) compiler which had a problem with this ... For a while some people were compiling git with pretty antique compilers, but I do not know if that is the case any more (Junio noted recently that we have had trailing commas on arrays and enums in builtin/clean.c for the past year, and nobody has complained). Maybe those systems have all died off, or maybe those people only upgrade every few years, and we are due for an onslaught of portability fixes soon. :) :-P You never know ... [I have several versions of the C standard that I can use to check up on the legalise, but I'm not sure I can be bothered! ;-) ] It was actually pretty easy to find. I only have C99, but empty macro arguments are listed as one of the changes since C89 in the foreward. Ah yes, having prompted me to look, it took all of 2 minutes to find it in the C11 .pdf file I have right here ... diff --git a/alloc.c b/alloc.c index eb22a45..5392d13 100644 --- a/alloc.c +++ b/alloc.c @@ -18,9 +18,12 @@ #define BLOCKING 1024 -#define DEFINE_ALLOCATOR(name, type)\ +#define PUBLIC +#define PRIVATE static + +#define DEFINE_ALLOCATOR(scope, name, type) \ I am not sure offhand whether this is more portable or not (it would depend on order of macro expansion, and I am not brave enough to read through the preprocessor section of the standard carefully). I suspect it is _slightly_ more portable, but I wouldn't bet any money on it! I may take a look at the standard (it wouldn't be the first time I've looked this up), but it would not help in this situation anyway. The fact that a given compiler does/does-not conform to the standard is useless knowledge if we need to support them. Quick reading online suggests that it's OK, but that an extra level of macro expansion would not be. And of course the Internet is never wrong. :) ;-) I'm inclined to go with it, and deal with it later if we get a portability complaint (at which point we are no worse off than we are right now). Hmm, well my initial reaction was to send the more conservative patch first. I wasn't so worried about inlining the code from the macro (I doubt it will change), but I understand (and would often agree with) your concern. I would be happy with either solution, so I'll let yourself and Junio decide! (sloping shoulders, or what. :) ). ATB, Ramsay Jones -- 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: What's cooking in git.git (Jun 2014, #04; Tue, 17)
On Thu, Jun 19, 2014 at 12:25 AM, Junio C Hamano gits...@pobox.com wrote: [Stalled] * nd/multiple-work-trees (2014-03-25) 28 commits - count-objects: report unused files in $GIT_DIR/repos/... - gc: support prune --repos - gc: style change -- no SP before closing bracket - prune: strategies for linked checkouts - checkout: detach if the branch is already checked out elsewhere - checkout: clean up half-prepared directories in --to mode - checkout: support checking out into a new working directory - use new wrapper write_file() for simple file writing - wrapper.c: wrapper to open a file, fprintf then close - setup.c: support multi-checkout repo setup - setup.c: detect $GIT_COMMON_DIR check_repository_format_gently() - setup.c: convert check_repository_format_gently to use strbuf - setup.c: detect $GIT_COMMON_DIR in is_git_directory() - setup.c: convert is_git_directory() to use strbuf - git-stash: avoid hardcoding $GIT_DIR/logs/ - *.sh: avoid hardcoding $GIT_DIR/hooks/... - git-sh-setup.sh: use rev-parse --git-path to get $GIT_DIR/objects - $GIT_COMMON_DIR: a new environment variable - commit: use SEQ_DIR instead of hardcoding sequencer - fast-import: use git_path() for accessing .git dir instead of get_git_dir() - reflog: avoid constructing .lock path with git_path - *.sh: respect $GIT_INDEX_FILE - git_path(): be aware of file relocation in $GIT_DIR - path.c: group git_path(), git_pathdup() and strbuf_git_path() together - path.c: rename vsnpath() to do_git_path() - git_snpath(): retire and replace with strbuf_git_path() - path.c: make get_pathname() call sites return const char * - path.c: make get_pathname() return strbuf instead of static buffer A replacement for contrib/workdir/git-new-workdir that does not rely on symbolic links and make sharing of objects and refs safer by making the borrowee and borrowers aware of each other. Anything I can do to get this going again? The only thing I just found (and have not fixed) is, I think $GIT_DIR/info/excludes and $GIT_DIR/info/sparse-checkout should be per-worktree, not shared. -- 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
Tool/Scripts - For maintaining different branches on a repo
Hi, I have a single repo with different kinds of branches say 4 branches. Developers will send a patches wrt to specific branch. Is there any opensource tool/script that does applying patches/maintaining the branches in repo w/o manual intervention? thanks! -- Jagan. -- 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: [RFC] rebase --root: Empty root commit is replaced with sentinel
On 06/18/2014 02:10 PM, Fabian Ruch wrote: `rebase` supports the option `--root` both with and without `--onto`. The case where `--onto` is not specified is handled by creating a sentinel commit and squashing the root commit into it. The sentinel commit refers to the empty tree and does not have a log message associated with it. Its purpose is that `rebase` can rely on having a rebase base even without `--onto`. The combination of `--root` and no `--onto` implies an interactive rebase. When `--preserve-merges` is not specified on the `rebase` command line, `rebase--interactive` uses `--cherry-pick` with git-rev-list to put the initial to-do list together. If the root commit is empty, it is treated as a cherry-pick of the sentinel commit and omitted from the todo-list. This is unexpected because the user does not know of the sentinel commit. I see that your new tests below both use --keep-empty. Without --keep-empty, I would have expected empty commits to be discarded by design. If that is the case, then there is only a bug if --keep-empty is used, and I think you should mention that option earlier in this description. Also, I think this bug strikes if *any* of the commits to be rebased is empty, not only the first commit. Add a test case. Create an empty root commit, run `rebase --root` and check that it is still there. If the branch consists of the root commit only, the bug described above causes the resulting history to consist of the sentinel commit only. If the root commit has children, the resulting history contains neither the root nor the sentinel commit. This behaviour is the same with `--keep-empty`. Signed-off-by: Fabian Ruch baf...@gmail.com --- Notes: Hi, This is not a fix yet. It is actually OK to add failing tests to the test suite, but they must be added with 'test_expect_failure' instead of 'test_expect_success'. Though of course it is preferred if the new test is followed by a commit that fixes it :-) We are currently special casing in `do_pick` and whether the current head is the sentinel commit is not a special case that would fit into `do_pick`'s interface description. What if we added the feature of creating root commits to `do_pick`, using `commit-tree` just like when creating the sentinel commit? We would have to add another special case (`test -z $onto`) to where the to-do list is put together in `rebase--interactive`. An empty `$onto` would imply git rev-list $orig_head to form the to-do list. The rebase comment in the commit message editor would have to become something similar to Rebase $shortrevisions as new history , which might be even less confusing than mentioning the hash of the sentinel commit. Since you are working on a hammer, I'm tempted to see this problem as a nail. Would it make it easier to encode the special behavior into the todo list itself?: pick --orphan 0cf23b1 New initial commit pick 144a852 Second commit pick 255f8de Third commit Michael t/t3412-rebase-root.sh | 27 +++ 1 file changed, 27 insertions(+) diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh index 0b52105..a4fe3c7 100755 --- a/t/t3412-rebase-root.sh +++ b/t/t3412-rebase-root.sh @@ -278,4 +278,31 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' ' test_cmp expect-conflict-p out ' +test_expect_success 'rebase --root recreates empty root commit' ' + echo Initial expected.msg + # commit the empty tree, no parents + empty_tree=$(git hash-object -t tree /dev/null) + empty_root_commit=$(git commit-tree $empty_tree -F expected.msg) + git checkout -b empty-root-commit-only $empty_root_commit + # implies interactive + git rebase --keep-empty --root + git show --pretty=format:%s HEAD actual.msg + test_cmp actual.msg expected.msg +' + +test_expect_success 'rebase --root recreates empty root commit (subsequent commits)' ' + echo Initial expected.msg + # commit the empty tree, no parents + empty_tree=$(git hash-object -t tree /dev/null) + empty_root_commit=$(git commit-tree $empty_tree -F expected.msg) + git checkout -b empty-root-commit $empty_root_commit + file + git add file + git commit -m file + # implies interactive + git rebase --keep-empty --root + git show --pretty=format:%s HEAD^ actual.msg + test_cmp actual.msg expected.msg +' + test_done -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] diff: mark any file larger than core.bigfilethreshold binary
Am Donnerstag, den 29.05.2014, 19:57 +0700 schrieb Nguyễn Thái Ngọc Duy:
Hi,
sorry for chiming in so late.
I've just played around with patch 3 and 4 of that series.
And I like it very much as I work often with large files so any further
enhancement in that area is really nice.
(see comments below)
Too large files may lead to failure to allocate memory. If it happens
here, it could impact quite a few commands that involve
diff. Moreover, too large files are inefficient to compare anyway (and
most likely non-text), so mark them binary and skip looking at their
content.
Noticed-by: Dale R. Worley wor...@alum.mit.edu
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
diff.c | 26 ++
diffcore.h | 1 +
t/t1050-large.sh | 4
3 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/diff.c b/diff.c
index 54281cb..0a2f865 100644
--- a/diff.c
+++ b/diff.c
@@ -2185,8 +2185,8 @@ int diff_filespec_is_binary(struct diff_filespec *one)
one-is_binary = one-driver-binary;
else {
if (!one-data DIFF_FILE_VALID(one))
- diff_populate_filespec(one, 0);
- if (one-data)
+ diff_populate_filespec(one,
DIFF_POPULATE_IS_BINARY);
+ if (one-is_binary == -1 one-data)
one-is_binary = buffer_is_binary(one-data,
one-size);
if (one-is_binary == -1)
@@ -2721,6 +2721,11 @@ int diff_populate_filespec(struct diff_filespec *s,
unsigned int flags)
}
if (size_only)
return 0;
+ if ((flags DIFF_POPULATE_IS_BINARY)
+ s-size big_file_threshold s-is_binary == -1) {
+ s-is_binary = 1;
+ return 0;
+ }
Why do you check for s-is_binary == -1 here? I think it does not matter
what s_is_binary says here.
fd = open(s-path, O_RDONLY);
if (fd 0)
goto err_empty;
@@ -2742,16 +2747,21 @@ int diff_populate_filespec(struct diff_filespec *s,
unsigned int flags)
}
else {
enum object_type type;
- if (size_only) {
+ if (size_only || (flags DIFF_POPULATE_IS_BINARY)) {
type = sha1_object_info(s-sha1, s-size);
if (type 0)
die(unable to read %s, sha1_to_hex(s-sha1));
- } else {
- s-data = read_sha1_file(s-sha1, type, s-size);
- if (!s-data)
- die(unable to read %s, sha1_to_hex(s-sha1));
- s-should_free = 1;
+ if (size_only)
+ return 0;
+ if (s-size big_file_threshold s-is_binary == -1)
{
same as above.
+ s-is_binary = 1;
+ return 0;
+ }
}
+ s-data = read_sha1_file(s-sha1, type, s-size);
+ if (!s-data)
+ die(unable to read %s, sha1_to_hex(s-sha1));
+ s-should_free = 1;
}
return 0;
}
diff --git a/diffcore.h b/diffcore.h
index a186d7c..e7760d9 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -56,6 +56,7 @@ extern void fill_filespec(struct diff_filespec *, const
unsigned char *,
int, unsigned short);
#define DIFF_POPULATE_SIZE_ONLY 1
+#define DIFF_POPULATE_IS_BINARY 2
extern int diff_populate_filespec(struct diff_filespec *, unsigned int);
extern void diff_free_filespec_data(struct diff_filespec *);
extern void diff_free_filespec_blob(struct diff_filespec *);
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index 333909b..4d922e2 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -112,6 +112,10 @@ test_expect_success 'diff --raw' '
git diff --raw HEAD^
'
+test_expect_success 'diff --stat' '
+ git diff --stat HEAD^ HEAD
+'
+
test_expect_success 'hash-object' '
git hash-object large1
'
I would also add a note to the documentation e. g:
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9f467d3..7a2f27d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -499,7 +499,8 @@ core.bigFileThreshold::
Files larger than this size are stored deflated, without
attempting delta compression. Storing large files without
delta compression avoids excessive memory usage, at the
- slight expense of increased disk usage.
+ slight expense of increased disk usage. Additionally files
+ larger than this size are allways treated as binary.
+
Default is 512 MiB on all platforms. This should be reasonable
for most
Re: [RFC] rebase --root: Empty root commit is replaced with sentinel
Hi Michael, thanks for your reply. On 06/19/2014 01:35 PM, Michael Haggerty wrote: On 06/18/2014 02:10 PM, Fabian Ruch wrote: `rebase` supports the option `--root` both with and without `--onto`. The case where `--onto` is not specified is handled by creating a sentinel commit and squashing the root commit into it. The sentinel commit refers to the empty tree and does not have a log message associated with it. Its purpose is that `rebase` can rely on having a rebase base even without `--onto`. The combination of `--root` and no `--onto` implies an interactive rebase. When `--preserve-merges` is not specified on the `rebase` command line, `rebase--interactive` uses `--cherry-pick` with git-rev-list to put the initial to-do list together. If the root commit is empty, it is treated as a cherry-pick of the sentinel commit and omitted from the todo-list. This is unexpected because the user does not know of the sentinel commit. I see that your new tests below both use --keep-empty. Without --keep-empty, I would have expected empty commits to be discarded by design. If that is the case, then there is only a bug if --keep-empty is used, and I think you should mention that option earlier in this description. Now that you mention it, --keep-empty is crucial for this to be a bug (except for the case where the branch consists solely of empty commits). I intended to use --keep-empty merely as a pedagogic tool so nobody would get confused about what is on the to-do list. Also, I think this bug strikes if *any* of the commits to be rebased is empty, not only the first commit. Ah, I really did not deduce that all empty commits would disappear with --root and --keep-empty. Thanks. Add a test case. Create an empty root commit, run `rebase --root` and check that it is still there. If the branch consists of the root commit only, the bug described above causes the resulting history to consist of the sentinel commit only. If the root commit has children, the resulting history contains neither the root nor the sentinel commit. This behaviour is the same with `--keep-empty`. Signed-off-by: Fabian Ruch baf...@gmail.com --- Notes: Hi, This is not a fix yet. It is actually OK to add failing tests to the test suite, but they must be added with 'test_expect_failure' instead of 'test_expect_success'. Though of course it is preferred if the new test is followed by a commit that fixes it :-) I did not plan to have this accepted but to amend the patch with a fix later on. Also, I hoped the ready-to-apply tests would give someone else a smoother start when taking over and compensate for a possibly incomprehensible problem description. We are currently special casing in `do_pick` and whether the current head is the sentinel commit is not a special case that would fit into `do_pick`'s interface description. What if we added the feature of creating root commits to `do_pick`, using `commit-tree` just like when creating the sentinel commit? We would have to add another special case (`test -z $onto`) to where the to-do list is put together in `rebase--interactive`. An empty `$onto` would imply git rev-list $orig_head to form the to-do list. The rebase comment in the commit message editor would have to become something similar to Rebase $shortrevisions as new history , which might be even less confusing than mentioning the hash of the sentinel commit. Since you are working on a hammer, I'm tempted to see this problem as a nail. Would it make it easier to encode the special behavior into the todo list itself?: pick --orphan 0cf23b1 New initial commit pick 144a852 Second commit pick 255f8de Third commit While I agree to enable pick to create orphan commits, I don't think a user option --orphan is of much help. Firstly, does --orphan make sense for any commit but the first one on the to-do list? Secondly, does --orphan make sense when we are rebasing onto another branch? The second point is related to the first in the sense that pick --orphan would be used on a commit that is understood to have a parent. Michael Fabian t/t3412-rebase-root.sh | 27 +++ 1 file changed, 27 insertions(+) diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh index 0b52105..a4fe3c7 100755 --- a/t/t3412-rebase-root.sh +++ b/t/t3412-rebase-root.sh @@ -278,4 +278,31 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' ' test_cmp expect-conflict-p out ' +test_expect_success 'rebase --root recreates empty root commit' ' +echo Initial expected.msg +# commit the empty tree, no parents +empty_tree=$(git hash-object -t tree /dev/null) +empty_root_commit=$(git commit-tree $empty_tree -F expected.msg) +git checkout -b empty-root-commit-only $empty_root_commit +# implies interactive +
Re: [RFC] rebase --root: Empty root commit is replaced with sentinel
On 06/19/2014 02:39 PM, Fabian Ruch wrote: Hi Michael, thanks for your reply. On 06/19/2014 01:35 PM, Michael Haggerty wrote: On 06/18/2014 02:10 PM, Fabian Ruch wrote: `rebase` supports the option `--root` both with and without `--onto`. The case where `--onto` is not specified is handled by creating a sentinel commit and squashing the root commit into it. The sentinel commit refers to the empty tree and does not have a log message associated with it. Its purpose is that `rebase` can rely on having a rebase base even without `--onto`. The combination of `--root` and no `--onto` implies an interactive rebase. When `--preserve-merges` is not specified on the `rebase` command line, `rebase--interactive` uses `--cherry-pick` with git-rev-list to put the initial to-do list together. If the root commit is empty, it is treated as a cherry-pick of the sentinel commit and omitted from the todo-list. This is unexpected because the user does not know of the sentinel commit. I see that your new tests below both use --keep-empty. Without --keep-empty, I would have expected empty commits to be discarded by design. If that is the case, then there is only a bug if --keep-empty is used, and I think you should mention that option earlier in this description. Now that you mention it, --keep-empty is crucial for this to be a bug (except for the case where the branch consists solely of empty commits). I intended to use --keep-empty merely as a pedagogic tool so nobody would get confused about what is on the to-do list. Also, I think this bug strikes if *any* of the commits to be rebased is empty, not only the first commit. Ah, I really did not deduce that all empty commits would disappear with --root and --keep-empty. Thanks. Add a test case. Create an empty root commit, run `rebase --root` and check that it is still there. If the branch consists of the root commit only, the bug described above causes the resulting history to consist of the sentinel commit only. If the root commit has children, the resulting history contains neither the root nor the sentinel commit. This behaviour is the same with `--keep-empty`. Signed-off-by: Fabian Ruch baf...@gmail.com --- Notes: Hi, This is not a fix yet. It is actually OK to add failing tests to the test suite, but they must be added with 'test_expect_failure' instead of 'test_expect_success'. Though of course it is preferred if the new test is followed by a commit that fixes it :-) I did not plan to have this accepted but to amend the patch with a fix later on. Also, I hoped the ready-to-apply tests would give someone else a smoother start when taking over and compensate for a possibly incomprehensible problem description. We are currently special casing in `do_pick` and whether the current head is the sentinel commit is not a special case that would fit into `do_pick`'s interface description. What if we added the feature of creating root commits to `do_pick`, using `commit-tree` just like when creating the sentinel commit? We would have to add another special case (`test -z $onto`) to where the to-do list is put together in `rebase--interactive`. An empty `$onto` would imply git rev-list $orig_head to form the to-do list. The rebase comment in the commit message editor would have to become something similar to Rebase $shortrevisions as new history , which might be even less confusing than mentioning the hash of the sentinel commit. Since you are working on a hammer, I'm tempted to see this problem as a nail. Would it make it easier to encode the special behavior into the todo list itself?: pick --orphan 0cf23b1 New initial commit pick 144a852 Second commit pick 255f8de Third commit While I agree to enable pick to create orphan commits, I don't think a user option --orphan is of much help. Firstly, does --orphan make sense for any commit but the first one on the to-do list? Secondly, does --orphan make sense when we are rebasing onto another branch? The second point is related to the first in the sense that pick --orphan would be used on a commit that is understood to have a parent. --orphan as a user option would only really make sense if we get around to supporting interactive rebase of arbitrary DAGs. Perhaps a more practical problem with --orphan is that it makes it harder for the user to change the order of the first two commits. Another possible construct would be a separate orphan command: orphan pick 0cf23b1 New initial commit pick 144a852 Second commit pick 255f8de Third commit But these are just wild ideas. I haven't thought enough about the problem to advocate anything. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message
search for equivalent commits by patch-id
Hello, Is there a way to find all equivalent commits by patch-id? Something similar to: git branch -a --commit commit but instead of commit to search by patch-id. thanks Vicentiu -- 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
Surprising 'git-describe --all --match' behavior.
Hello, Just playing with it, got some surprises: $ git --version git version 1.9.3 $ git describe --all heads/v3.5 $ git describe --all --match 'v*' tags/v3.5.6b2-4-gab4bf78 $ git describe --all --match 'heads/v*' fatal: No names found, cannot describe anything. ... heads/v3.5 matches neither 'v*' nor 'heads/v*'? $ git describe --all --match 'v*' tags/v3.5.6b2-4-gab4bf78 $ git describe --all --match 'tags/v*' fatal: No names found, cannot describe anything. ... git matches short names when outputs full names? Is it a defect, or what do I miss? -- Sergei. -- 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 v19 03/48] refs.c: constify the sha arguments for ref_transaction_create|delete|update
ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.
Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 ---
refs.h | 7 ---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index d9cac6d..21ed465 100644
--- a/refs.c
+++ b/refs.c
@@ -3359,7 +3359,8 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3373,7 +3374,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3387,7 +3388,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 7d946f6..8c7f9c4 100644
--- a/refs.h
+++ b/refs.h
@@ -240,7 +240,8 @@ struct ref_transaction *ref_transaction_begin(void);
*/
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
@@ -251,7 +252,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
*/
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags);
/*
@@ -261,7 +262,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
*/
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
--
2.0.0.438.g337c581
--
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 v19 00/48] Use ref transactions
This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions This patch series is based on current master and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most of the ref updates to become atomic when there are failures locking or writing to a ref. This version completes the work to convert all ref updates to use transactions. Now that all updates are through transactions I will start working on cleaning up the reading of refs and to create an api for managing reflogs but all that will go in a different patch series. Version 19: Changes based on M Haggertys review: 1, remove a stray redundant line in refs.c: log_ref_write should try to return meaningful errno 2, fix a typo in a comment in refs.c: verify_lock should set errno to something meaningful 3, move a patch to change delete_loose_ref so it happens later in the series refs.c: add an err argument to delete_ref_loose Ronnie Sahlberg (48): refs.c: remove ref_transaction_rollback refs.c: ref_transaction_commit should not free the transaction refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging lockfile.c: add a new public function unable_to_lock_message lockfile.c: make lock_file return a meaningful errno on failurei refs.c: add an err argument to repack_without_refs refs.c: make sure log_ref_setup returns a meaningful errno refs.c: verify_lock should set errno to something meaningful refs.c: make remove_empty_directories always set errno to something sane refs.c: commit_packed_refs to return a meaningful errno on failure refs.c: make resolve_ref_unsafe set errno to something meaningful on error refs.c: log_ref_write should try to return meaningful errno refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: make update_ref_write update a strbuf on failure update-ref: use err argument to get error from ref_transaction_commit refs.c: remove the onerr argument to ref_transaction_commit refs.c: change ref_transaction_update() to do error checking and return status 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 refs.c: add an err argument to delete_ref_loose refs.c: pass the ref log message to _create/delete/update instead of _commit refs.c: pass NULL as *flags to read_ref_full refs.c: move the check for valid refname to lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: pass a skip list to name_conflict_fn refs.c: propagate any errno==ENOTDIR from _commit back to the callers fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static branch.c | 30 +-- builtin/commit.c | 24 ++- builtin/fetch.c| 36 ++-- builtin/receive-pack.c | 97 ++ builtin/remote.c | 5 +- builtin/replace.c | 17 +- builtin/tag.c | 15 +- builtin/update-ref.c | 34 ++-- cache.h| 4 +- fast-import.c | 55 -- lockfile.c | 39 ++-- refs.c | 505 - refs.h | 131 + sequencer.c| 24 ++- walker.c | 58 +++--- wrapper.c | 14 +- 16 files changed, 710 insertions(+), 378 deletions(-) -- 2.0.0.438.g337c581 -- 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 v19 44/48] refs.c: call lock_ref_sha1_basic directly from commit
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 5a7fb34..2cc662c 100644
--- a/refs.c
+++ b/refs.c
@@ -3595,12 +3595,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = lock_any_ref_for_update(update-refname,
- (update-have_old ?
- update-old_sha1 :
- NULL),
- update-flags,
- update-type);
+ update-lock = lock_ref_sha1_basic(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
--
2.0.0.438.g337c581
--
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 v19 48/48] refs.c: make write_ref_sha1 static
No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 10 --
refs.h | 3 ---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index f4234b9..1529a26 100644
--- a/refs.c
+++ b/refs.c
@@ -2665,6 +2665,9 @@ static int rename_tmp_log(const char *newrefname)
return 0;
}
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+ const char *logmsg);
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -2914,8 +2917,11 @@ static int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
-/* This function must return a meaningful errno */
-int write_ref_sha1(struct ref_lock *lock,
+/*
+ * Writes sha1 into the ref specified by the lock. Makes sure that errno
+ * is sane on error.
+ */
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index 5c0543d..db463d0 100644
--- a/refs.h
+++ b/refs.h
@@ -203,9 +203,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/*
* Setup reflog before using. Set errno to something meaningful on failure.
*/
--
2.0.0.438.g337c581
--
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 v19 25/48] 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 | 17 ++--- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index 1bb491d..054f5ef 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.0.438.g337c581 -- 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 v19 16/48] refs.c: make update_ref_write update a strbuf on failure
Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 115f143..003b313 100644
--- a/refs.c
+++ b/refs.c
@@ -3353,10 +3353,13 @@ static struct ref_lock *update_ref_lock(const char
*refname,
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
- enum action_on_err onerr)
+ 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;
@@ -3477,7 +3480,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
- return update_ref_write(action, refname, sha1, lock, onerr);
+ return update_ref_write(action, refname, sha1, lock, NULL, onerr);
}
static int ref_update_compare(const void *r1, const void *r2)
@@ -3559,7 +3562,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
- update-lock, onerr);
+ update-lock, err, onerr);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
--
2.0.0.438.g337c581
--
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 v19 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
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
any codepaths that tries to open and repair badly named refs. The normal refs
API should not allow neither creating nor accessing refs with invalid names.
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.
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 d6467c8..5a7fb34 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 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.0.0.438.g337c581
--
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 v19 38/48] 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 | 29 +
refs.h | 14 --
2 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 441c0bc..7c46d13 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)
@@ -3596,8 +3608,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 4ac4a7d..3b321c2 100644
--- a/refs.h
+++ b/refs.h
@@ -177,9 +177,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.0.438.g337c581
--
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 v19 30/48] 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 | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 8c695ba..8b2c598 100644
--- a/refs.c
+++ b/refs.c
@@ -3524,11 +3524,28 @@ 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.0.438.g337c581
--
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 v19 46/48] refs.c: propagate any errno==ENOTDIR from _commit back to the callers
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when
we lstat the new refname and it returns ENOTDIR or if the name checking
function reports that the same type of conflict happened. In both cases it
means that we can not create the new ref due to a name conflict.
For these cases, save the errno value and abort and make sure that the caller
can see errno==ENOTDIR.
Also start defining specific return codes for _commit, assign -1 as a generic
error and -2 as the error that refers to a name conflict. Callers can (and
should) use that return value inspecting errno directly.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 22 +++---
refs.h | 6 ++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index 6b5fc09..f4234b9 100644
--- a/refs.c
+++ b/refs.c
@@ -3579,7 +3579,7 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- int ret = 0, delnum = 0, i;
+ int ret = 0, delnum = 0, i, df_conflict = 0;
const char **delnames;
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
@@ -3597,9 +3597,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err);
- if (ret)
+ if (ref_update_reject_duplicates(updates, n, err)) {
+ ret = -1;
goto cleanup;
+ }
/* Acquire all locks while verifying old values */
for (i = 0; i n; i++) {
@@ -3613,10 +3614,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-type,
delnames, delnum);
if (!update-lock) {
+ if (errno == ENOTDIR)
+ df_conflict = 1;
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
update-refname);
- ret = 1;
+ ret = -1;
goto cleanup;
}
}
@@ -3634,6 +3637,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (err)
strbuf_addf(err, str, update-refname);
+ ret = -1;
goto cleanup;
}
}
@@ -3644,14 +3648,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type,
- err);
+ if (delete_ref_loose(update-lock, update-type, err))
+ ret = -1;
+
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
}
- ret |= repack_without_refs(delnames, delnum, err);
+ if (repack_without_refs(delnames, delnum, err))
+ ret = -1;
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
@@ -3664,6 +3670,8 @@ cleanup:
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
free(delnames);
+ if (df_conflict)
+ ret = -2;
return ret;
}
diff --git a/refs.h b/refs.h
index f24b2c1..5c0543d 100644
--- a/refs.h
+++ b/refs.h
@@ -333,7 +333,13 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
+ * If the transaction is already in failed state this function will return
+ * an error.
+ * Function returns 0 on success, -1 for generic failures and
+ * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name
+ * collision (ENOTDIR).
*/
+#define UPDATE_REFS_NAME_CONFLICT -2
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err);
--
2.0.0.438.g337c581
--
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 v19 40/48] refs.c: add an err argument to delete_ref_loose
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.
Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().
Simplify warn_if_unremovable.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c| 33 -
wrapper.c | 14 ++
2 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index 15cc5f9..fee7d76 100644
--- a/refs.c
+++ b/refs.c
@@ -2544,16 +2544,38 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
return ret;
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int add_err_if_unremovable(const char *op, const char *file,
+ struct strbuf *e, int rc)
+{
+ int err = errno;
+ if (rc 0 errno != ENOENT) {
+ strbuf_addf(e, unable to %s %s: %s,
+ op, file, strerror(errno));
+ errno = err;
+ return -1;
+ }
+ return 0;
+}
+
+static int unlink_or_err(const char *file, struct strbuf *err)
+{
+ if (err)
+ return add_err_if_unremovable(unlink, file, err,
+ unlink(file));
+ else
+ return unlink_or_warn(file);
+}
+
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock-lk-filename) - 5; /* .lock */
+ int res, i = strlen(lock-lk-filename) - 5; /* .lock */
lock-lk-filename[i] = 0;
- err = unlink_or_warn(lock-lk-filename);
+ res = unlink_or_err(lock-lk-filename, err);
lock-lk-filename[i] = '.';
- if (err errno != ENOENT)
+ if (res)
return 1;
}
return 0;
@@ -3600,7 +3622,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- ret |= delete_ref_loose(update-lock, update-type);
+ ret |= delete_ref_loose(update-lock, update-type,
+ err);
if (!(update-flags REF_ISPRUNING))
delnames[delnum++] = update-lock-ref_name;
}
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..740e193 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
static int warn_if_unremovable(const char *op, const char *file, int rc)
{
- if (rc 0) {
- int err = errno;
- if (ENOENT != err) {
- warning(unable to %s %s: %s,
- op, file, strerror(errno));
- errno = err;
- }
- }
+ int err;
+ if (rc = 0 || errno == ENOENT)
+ return rc;
+ err = errno;
+ warning(unable to %s %s: %s, op, file, strerror(errno));
+ errno = err;
return rc;
}
--
2.0.0.438.g337c581
--
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 v19 45/48] refs.c: pass a skip list to name_conflict_fn
Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 41 ++---
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index 2cc662c..6b5fc09 100644
--- a/refs.c
+++ b/refs.c
@@ -801,15 +801,18 @@ static int names_conflict(const char *refname1, const
char *refname2)
struct name_conflict_cb {
const char *refname;
- const char *oldrefname;
const char *conflicting_refname;
+ const char **skip;
+ int skipnum;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
- if (data-oldrefname !strcmp(data-oldrefname, entry-name))
- return 0;
+ int i;
+ for (i = 0; i data-skipnum; i++)
+ if (!strcmp(entry-name, data-skip[i]))
+ return 0;
if (names_conflict(data-refname, entry-name)) {
data-conflicting_refname = entry-name;
return 1;
@@ -822,15 +825,18 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* conflicting with the name of an existing reference in dir. If
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with.
*/
-static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+static int is_refname_available(const char *refname,
+ struct ref_dir *dir,
+ const char **skip, int skipnum)
{
struct name_conflict_cb data;
data.refname = refname;
- data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skip = skip;
+ data.skipnum = skipnum;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) {
@@ -2077,7 +2083,8 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
/* This function should make sure errno is meaningful on error */
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
- int flags, int *type_p)
+ int flags, int *type_p,
+ const char **skip, int skipnum)
{
char *ref_file;
const char *orig_refname = refname;
@@ -2126,7 +2133,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing
-!is_refname_available(refname, NULL, get_packed_refs(ref_cache)))
{
+!is_refname_available(refname, get_packed_refs(ref_cache),
+ skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2184,7 +2192,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
}
/*
@@ -2676,10 +2684,12 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref)
return error(refname %s not found, oldrefname);
- if (!is_refname_available(newrefname, oldrefname,
get_packed_refs(ref_cache)))
+ if (!is_refname_available(newrefname, get_packed_refs(ref_cache),
+ oldrefname, 1))
return 1;
- if (!is_refname_available(newrefname, oldrefname,
get_loose_refs(ref_cache)))
+ if (!is_refname_available(newrefname, get_loose_refs(ref_cache),
+ oldrefname, 1))
return 1;
if (log rename(git_path(logs/%s, oldrefname),
git_path(TMP_RENAMED_LOG)))
@@ -2709,7 +2719,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
logmoved = log;
- lock =
[PATCH v19 10/48] refs.c: verify_lock should set errno to something meaningful
Making errno when returning from verify_lock() meaningful, which
should almost but not completely fix
* a bug in git fetch's s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
ENOTDIR makes sense as a sign that a file was in the way of a
directory we wanted to create. Should git fetch also look for
ENOTEMPTY or EEXIST to catch cases where a directory was in the way
of a file to be created?
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 4
refs.h | 6 +-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 9ea519c..a48f805 100644
--- a/refs.c
+++ b/refs.c
@@ -1932,18 +1932,22 @@ int refname_match(const char *abbrev_name, const char
*full_name)
return 0;
}
+/* This function should make sure errno is meaningful on error */
static struct ref_lock *verify_lock(struct ref_lock *lock,
const unsigned char *old_sha1, int mustexist)
{
if (read_ref_full(lock-ref_name, lock-old_sha1, mustexist, NULL)) {
+ int save_errno = errno;
error(Can't verify ref %s, lock-ref_name);
unlock_ref(lock);
+ errno = save_errno;
return NULL;
}
if (hashcmp(lock-old_sha1, old_sha1)) {
error(Ref %s is at %s but expected %s, lock-ref_name,
sha1_to_hex(lock-old_sha1), sha1_to_hex(old_sha1));
unlock_ref(lock);
+ errno = EBUSY;
return NULL;
}
return lock;
diff --git a/refs.h b/refs.h
index 82cc5cb..8d6cac7 100644
--- a/refs.h
+++ b/refs.h
@@ -137,11 +137,15 @@ 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. **/
+/*
+ * 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 */
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p);
--
2.0.0.438.g337c581
--
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 v19 41/48] refs.c: pass the ref log message to _create/delete/update instead of _commit
Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/fetch.c| 3 +--
builtin/receive-pack.c | 5 +++--
builtin/replace.c | 4 ++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 34 +-
refs.h | 8
sequencer.c| 4 ++--
walker.c | 5 ++---
12 files changed, 52 insertions(+), 44 deletions(-)
diff --git a/branch.c b/branch.c
index c1eae00..e0439af 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, err) ||
- ref_transaction_commit(transaction, msg, err))
+ null_sha1, 0, !forcing, msg, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 668fa6a..c499826 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1767,8 +1767,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, HEAD, sha1,
current_head ?
current_head-object.sha1 : NULL,
- 0, !!current_head, err) ||
- ref_transaction_commit(transaction, sb.buf, err)) {
+ 0, !!current_head, sb.buf, err) ||
+ ref_transaction_commit(transaction, err)) {
rollback_index_files();
die(%s, err.buf);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..faa1233 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -673,10 +673,9 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
-
if (rc STORE_REF_ERROR_DF_CONFLICT)
error(_(some local refs could not be updated; try running\n
- 'git remote prune %s' to remove any old, conflicting
+ 'git remote prune %s' to remove any old, conflicting
branches), remote_name);
abort:
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b51f8ae..0ed1ddd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -585,8 +585,9 @@ static char *update(struct command *cmd, struct
shallow_info *si)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, err) ||
- ref_transaction_commit(transaction, push, err)) {
+ new_sha1, old_sha1, 0, 1, push,
+ err) ||
+ ref_transaction_commit(transaction, err)) {
char *str = strbuf_detach(err, NULL);
ref_transaction_free(transaction);
diff --git a/builtin/replace.c b/builtin/replace.c
index 054f5ef..8e699f2 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -170,8 +170,8 @@ static int replace_object_sha1(const char *object_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))
+ 0, !is_null_sha1(prev), NULL, err) ||
+ ref_transaction_commit(transaction, err))
die(%s, err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index c9bfc9a..74af63e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
- 0, !is_null_sha1(prev), err) ||
- ref_transaction_commit(transaction, NULL, err))
+ 0, !is_null_sha1(prev), NULL, err) ||
+
[PATCH v19 35/48] 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 db8e0a0..b063baa 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)
@@ -3600,12 +3582,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.0.438.g337c581
--
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 v19 15/48] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 61570c9..115f143 100644
--- a/refs.c
+++ b/refs.c
@@ -3488,6 +3488,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+ struct strbuf *err,
enum action_on_err onerr)
{
int i;
@@ -3495,6 +3496,9 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
const char *str =
Multiple updates for ref '%s' not allowed.;
+ if (err)
+ strbuf_addf(err, str, updates[i]-refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]-refname); break;
@@ -3525,7 +3529,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
--
2.0.0.438.g337c581
--
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 v19 47/48] fetch.c: change s_update_ref to use a ref transaction
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/fetch.c | 33 +++--
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index faa1233..52f1ebc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,23 +375,36 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv(GIT_REFLOG_ACTION);
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+ int ret, df_conflict = 0;
if (dry_run)
return 0;
if (!rla)
rla = default_rla.buf;
snprintf(msg, sizeof(msg), %s: %s, rla, action);
- lock = lock_any_ref_for_update(ref-name,
- check_old ? ref-old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref-new_sha1, msg) 0)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ ref-old_sha1, 0, check_old, msg, err))
+ goto fail;
+
+ ret = ref_transaction_commit(transaction, err);
+ if (ret == UPDATE_REFS_NAME_CONFLICT)
+ df_conflict = 1;
+ if (ret)
+ goto fail;
+
+ ref_transaction_free(transaction);
return 0;
+fail:
+ ref_transaction_free(transaction);
+ error(%s, err.buf);
+ strbuf_release(err);
+ return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
+ : STORE_REF_ERROR_OTHER;
}
#define REFCOL_WIDTH 10
--
2.0.0.438.g337c581
--
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 v19 37/48] 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 6792938..441c0bc 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.0.438.g337c581
--
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 v19 33/48] 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 collissions that the previous locking would
protect against and cause the fetch to fail for to be 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.0.438.g337c581
--
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 v19 04/48] refs.c: allow passing NULL to ref_transaction_free
Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free
easier to use and more similar to plain 'free'.
In particular, it lets us rollback unconditionally as part of cleanup code
after setting 'transaction = NULL' if a transaction has been committed or
rolled back already.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/refs.c b/refs.c
index 21ed465..1d6dece 100644
--- a/refs.c
+++ b/refs.c
@@ -3338,6 +3338,9 @@ void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
+
for (i = 0; i transaction-nr; i++)
free(transaction-updates[i]);
--
2.0.0.438.g337c581
--
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 v19 42/48] refs.c: pass NULL as *flags to read_ref_full
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 3521033..d6467c8 100644
--- a/refs.c
+++ b/refs.c
@@ -2688,7 +2688,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, flag)
+ if (!read_ref_full(newrefname, sha1, 1, NULL)
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path(%s,
newrefname))) {
--
2.0.0.438.g337c581
--
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 v19 08/48] refs.c: add an err argument to repack_without_refs
Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to this function.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/remote.c | 5 +++--
refs.c | 19 ++-
refs.h | 3 ++-
3 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index c9102e8..401feb3 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -755,7 +755,7 @@ static int remove_branches(struct string_list *branches)
branch_names = xmalloc(branches-nr * sizeof(*branch_names));
for (i = 0; i branches-nr; i++)
branch_names[i] = branches-items[i].string;
- result |= repack_without_refs(branch_names, branches-nr);
+ result |= repack_without_refs(branch_names, branches-nr, NULL);
free(branch_names);
for (i = 0; i branches-nr; i++) {
@@ -1333,7 +1333,8 @@ static int prune_remote(const char *remote, int dry_run)
for (i = 0; i states.stale.nr; i++)
delete_refs[i] = states.stale.items[i].util;
if (!dry_run)
- result |= repack_without_refs(delete_refs,
states.stale.nr);
+ result |= repack_without_refs(delete_refs,
+ states.stale.nr, NULL);
free(delete_refs);
}
diff --git a/refs.c b/refs.c
index e9d53e4..67a0217 100644
--- a/refs.c
+++ b/refs.c
@@ -2456,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-int repack_without_refs(const char **refnames, int n)
+int repack_without_refs(const char **refnames, int n, struct strbuf *err)
{
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
struct string_list_item *ref_to_delete;
- int i, removed = 0;
+ int i, ret, removed = 0;
/* Look for a packed ref */
for (i = 0; i n; i++)
@@ -2473,6 +2473,11 @@ int repack_without_refs(const char **refnames, int n)
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(0)) {
+ if (err) {
+ unable_to_lock_message(git_path(packed-refs), errno,
+ err);
+ return -1;
+ }
unable_to_lock_error(git_path(packed-refs), errno);
return error(cannot delete '%s' from packed refs,
refnames[i]);
}
@@ -2499,12 +2504,16 @@ int repack_without_refs(const char **refnames, int n)
}
/* Write what remains */
- return commit_packed_refs();
+ ret = commit_packed_refs();
+ if (ret err)
+ strbuf_addf(err, unable to overwrite old ref-pack file: %s,
+ strerror(errno));
+ return ret;
}
static int repack_without_ref(const char *refname)
{
- return repack_without_refs(refname, 1);
+ return repack_without_refs(refname, 1, NULL);
}
static int delete_ref_loose(struct ref_lock *lock, int flag)
@@ -3508,7 +3517,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
}
}
- ret |= repack_without_refs(delnames, delnum);
+ ret |= repack_without_refs(delnames, delnum, err);
for (i = 0; i delnum; i++)
unlink_or_warn(git_path(logs/%s, delnames[i]));
clear_loose_ref_cache(ref_cache);
diff --git a/refs.h b/refs.h
index 64f25d9..65f7637 100644
--- a/refs.h
+++ b/refs.h
@@ -122,7 +122,8 @@ extern void rollback_packed_refs(void);
*/
int pack_refs(unsigned int flags);
-extern int repack_without_refs(const char **refnames, int n);
+extern int repack_without_refs(const char **refnames, int n,
+ struct strbuf *err);
extern int ref_exists(const char *);
--
2.0.0.438.g337c581
--
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 v19 31/48] 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..b51f8ae 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 v19 23/48] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status 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 | 40 +++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 9cb7908..8c695ba 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,8 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_state state;
+ int status;
};
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
@@ -3437,6 +3458,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 +3481,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 +3504,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 +3562,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 +3630,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.0.438.g337c581
--
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 v19 20/48] 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 | 55 +---
3 files changed, 63 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..33b4383 100644
--- a/refs.h
+++ b/refs.h
@@ -10,6 +10,45 @@ 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.
+ * This allows the caller to prepare preamble text to the generated
+ * error message:
+ *
+ * strbuf_addf(err, Error while doing foo-bar: );
+ * if (ref_transaction_update(..., err)) {
+ * ret = error(%s, err.buf);
+ * goto cleanup;
+ * }
+ */
struct ref_transaction;
/*
@@ -248,7 +287,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 +301,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,
-
[PATCH v19 39/48] 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 7c46d13..15cc5f9 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.0.438.g337c581
--
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 v19 27/48] 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 | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..fd8acaf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ 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(1); /* 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));
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, from,
+ 0, !unborn, 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.0.438.g337c581
--
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 v19 26/48] 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.0.438.g337c581
--
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 v19 28/48] 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 | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..cd929dc 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;
- unsigned char old_sha1[20];
+ 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.0.438.g337c581
--
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 v19 32/48] 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 cd929dc..191fc47 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.0.438.g337c581
--
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 v19 18/48] refs.c: remove the onerr argument to ref_transaction_commit
Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++---
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aec9004..88ab785 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- if (ref_transaction_commit(transaction, msg, err,
- UPDATE_REFS_QUIET_ON_ERR))
+ if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
ref_transaction_free(transaction);
return 0;
diff --git a/refs.c b/refs.c
index 003b313..4f78bd9 100644
--- a/refs.c
+++ b/refs.c
@@ -3491,8 +3491,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
- struct strbuf *err,
- enum action_on_err onerr)
+ struct strbuf *err)
{
int i;
for (i = 1; i n; i++)
@@ -3502,22 +3501,13 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
if (err)
strbuf_addf(err, str, updates[i]-refname);
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR:
- error(str, updates[i]-refname); break;
- case UPDATE_REFS_DIE_ON_ERR:
- die(str, updates[i]-refname); break;
- case UPDATE_REFS_QUIET_ON_ERR:
- break;
- }
return 1;
}
return 0;
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr)
+ const char *msg, struct strbuf *err)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3532,7 +3522,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
@@ -3544,7 +3534,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
(update-have_old ?
update-old_sha1 : NULL),
update-flags,
- update-type, onerr);
+ update-type,
+ UPDATE_REFS_QUIET_ON_ERR);
if (!update-lock) {
if (err)
strbuf_addf(err, Cannot lock the ref '%s'.,
@@ -3562,7 +3553,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
- update-lock, err, onerr);
+ update-lock, err,
+ UPDATE_REFS_QUIET_ON_ERR);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index e588ff8..163b45c 100644
--- a/refs.h
+++ b/refs.h
@@ -282,8 +282,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr);
+ const char *msg, struct strbuf *err);
/*
* Free an existing transaction and all associated data.
--
2.0.0.438.g337c581
--
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
[PATCH v19 24/48] 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..c9bfc9a 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, !is_null_sha1(prev), 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.0.438.g337c581
--
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 v19 22/48] 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 _being 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 eeababd..e729ea9 100644
--- a/refs.h
+++ b/refs.h
@@ -269,7 +269,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.0.438.g337c581
--
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 v19 21/48] 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 33b4383..eeababd 100644
--- a/refs.h
+++ b/refs.h
@@ -315,11 +315,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.0.438.g337c581
--
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 v19 36/48] 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 b063baa..6792938 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
@@ -3602,14 +3583,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.0.438.g337c581
--
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 v19 01/48] refs.c: remove ref_transaction_rollback
We do not yet need both a rollback and a free function for transactions.
Remove ref_transaction_rollback and use ref_transaction_free instead.
At a later stage we may reintroduce a rollback function if we want to start
adding reusable transactions and similar.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 +--
refs.h | 16 +++-
2 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index dc45774..6d841a0 100644
--- a/refs.c
+++ b/refs.c
@@ -3334,7 +3334,7 @@ struct ref_transaction *ref_transaction_begin(void)
return xcalloc(1, sizeof(struct ref_transaction));
}
-static void ref_transaction_free(struct ref_transaction *transaction)
+void ref_transaction_free(struct ref_transaction *transaction)
{
int i;
@@ -3345,11 +3345,6 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
free(transaction);
}
-void ref_transaction_rollback(struct ref_transaction *transaction)
-{
- ref_transaction_free(transaction);
-}
-
static struct ref_update *add_update(struct ref_transaction *transaction,
const char *refname)
{
diff --git a/refs.h b/refs.h
index 4e3050d..cfd1832 100644
--- a/refs.h
+++ b/refs.h
@@ -219,18 +219,12 @@ enum action_on_err {
/*
* Begin a reference transaction. The reference transaction must
- * eventually be commited using ref_transaction_commit() or rolled
- * back using ref_transaction_rollback().
+ * eventually be commited using ref_transaction_commit() or freed by
+ * calling ref_transaction_free().
*/
struct ref_transaction *ref_transaction_begin(void);
/*
- * Roll back a ref_transaction and free all associated data.
- */
-void ref_transaction_rollback(struct ref_transaction *transaction);
-
-
-/*
* The following functions add a reference check or update to a
* ref_transaction. In all of them, refname is the name of the
* reference to be affected. The functions make internal copies of
@@ -238,7 +232,6 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
* can be REF_NODEREF; it is passed to update_ref_lock().
*/
-
/*
* Add a reference update to transaction. new_sha1 is the value that
* the reference should have after the update, or zeros if it should
@@ -280,6 +273,11 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, enum action_on_err onerr);
+/*
+ * Free an existing transaction and all associated data.
+ */
+void ref_transaction_free(struct ref_transaction *transaction);
+
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
--
2.0.0.438.g337c581
--
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 v19 34/48] 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 8b2c598..db8e0a0 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 e729ea9..4ac4a7d 100644
--- a/refs.h
+++ b/refs.h
@@ -177,12 +177,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.0.438.g337c581
--
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 v19 13/48] refs.c: make resolve_ref_unsafe set errno to something meaningful on error
Making errno when returning from resolve_ref_unsafe() meaningful,
which should fix
* a bug in lock_ref_sha1_basic, where it assumes EISDIR
means it failed due to a directory being in the way
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
cache.h | 2 +-
refs.c | 19 +++
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 8b12aa8..e7ec626 100644
--- a/cache.h
+++ b/cache.h
@@ -979,7 +979,7 @@ extern int read_ref(const char *refname, unsigned char
*sha1);
* NULL. If more than MAXDEPTH recursive symbolic lookups are needed,
* give up and return NULL.
*
- * errno is sometimes set on errors, but not always.
+ * errno is set to something meaningful on error.
*/
extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1,
int reading, int *flag);
extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading,
int *flag);
diff --git a/refs.c b/refs.c
index 7a815be..211429d 100644
--- a/refs.c
+++ b/refs.c
@@ -1334,6 +1334,7 @@ static const char *handle_missing_loose_ref(const char
*refname,
}
}
+/* This function needs to return a meaningful errno on failure */
const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int
reading, int *flag)
{
int depth = MAXDEPTH;
@@ -1344,8 +1345,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
if (flag)
*flag = 0;
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ errno = EINVAL;
return NULL;
+ }
for (;;) {
char path[PATH_MAX];
@@ -1353,8 +1356,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
char *buf;
int fd;
- if (--depth 0)
+ if (--depth 0) {
+ errno = ELOOP;
return NULL;
+ }
git_snpath(path, sizeof(path), %s, refname);
@@ -1416,9 +1421,13 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
return NULL;
}
len = read_in_full(fd, buffer, sizeof(buffer)-1);
- close(fd);
- if (len 0)
+ if (len 0) {
+ int save_errno = errno;
+ close(fd);
+ errno = save_errno;
return NULL;
+ }
+ close(fd);
while (len isspace(buffer[len-1]))
len--;
buffer[len] = '\0';
@@ -1435,6 +1444,7 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
(buffer[40] != '\0' !isspace(buffer[40]))) {
if (flag)
*flag |= REF_ISBROKEN;
+ errno = EINVAL;
return NULL;
}
return refname;
@@ -1447,6 +1457,7 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
if (flag)
*flag |= REF_ISBROKEN;
+ errno = EINVAL;
return NULL;
}
refname = strcpy(refname_buffer, buf);
--
2.0.0.438.g337c581
--
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 v19 29/48] 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.0.438.g337c581
--
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 v19 19/48] refs.c: change ref_transaction_update() to do error checking and return status
Update ref_transaction_update() do some basic error checking and return
non-zero on error. Update all callers to check ref_transaction_update() for
error. There are currently no conditions in _update that will return error but
there will be in the future. Add an err argument that will be updated on
failure. In future patches we will start doing both locking and checking
for name conflicts in _update instead of _commit at which time this function
will start returning errors for these conditions.
Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 12 +++-
refs.c | 18 --
refs.h | 14 +-
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 88ab785..3067b11 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
static char line_termination = '\n';
static int update_flags;
+static struct strbuf err = STRBUF_INIT;
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -197,8 +198,9 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(update %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(verify %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, err))
+ die(%s, err.buf);
update_flags = 0;
free(refname);
@@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
- struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, msg, N_(reason), N_(reason of the
update)),
OPT_BOOL('d', NULL, delete, N_(delete the reference)),
diff --git a/refs.c b/refs.c
index 4f78bd9..3f05e88 100644
--- a/refs.c
+++ b/refs.c
@@ -3428,19 +3428,25 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ 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);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
update-have_old = have_old;
if (have_old)
hashcpy(update-old_sha1, old_sha1);
+ return 0;
}
void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 163b45c..c5376ce 100644
--- a/refs.h
+++ b/refs.h
@@ -246,12 +246,16 @@ struct ref_transaction *ref_transaction_begin(void);
* be deleted. If have_old is true, then old_sha1 holds the value
* that the reference should have had before the update, or zeros if
* 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.
*/
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char
[PATCH v19 11/48] refs.c: make remove_empty_directories always set errno to something sane
Making errno when returning from remove_empty_directories() more obviously meaningful, which should provide some peace of mind for people auditing lock_ref_sha1_basic. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index a48f805..cc69581 100644 --- a/refs.c +++ b/refs.c @@ -1960,14 +1960,16 @@ static int remove_empty_directories(const char *file) * only empty directories), remove them. */ struct strbuf path; - int result; + int result, save_errno; strbuf_init(path, 20); strbuf_addstr(path, file); result = remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY); + save_errno = errno; strbuf_release(path); + errno = save_errno; return result; } @@ -2056,6 +2058,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log) return logs_found; } +/* This function should make sure errno is meaningful on error */ static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) -- 2.0.0.438.g337c581 -- 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 v19 14/48] refs.c: log_ref_write should try to return meaningful errno
Making errno from write_ref_sha1() meaningful, which should fix
* a bug in git checkout -b where it prints strerror(errno)
despite errno possibly being zero or clobbered
* a bug in git fetch's s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 28 +++-
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 211429d..61570c9 100644
--- a/refs.c
+++ b/refs.c
@@ -2859,8 +2859,19 @@ static int log_ref_write(const char *refname, const
unsigned char *old_sha1,
len += copy_msg(logrec + len - 1, msg) - 1;
written = len = maxlen ? write_in_full(logfd, logrec, len) : -1;
free(logrec);
- if (close(logfd) != 0 || written != len)
- return error(Unable to append to %s, log_file);
+ if (written != len) {
+ int save_errno = errno;
+ close(logfd);
+ error(Unable to append to %s, log_file);
+ errno = save_errno;
+ return -1;
+ }
+ if (close(logfd)) {
+ int save_errno = errno;
+ error(Unable to append to %s, log_file);
+ errno = save_errno;
+ return -1;
+ }
return 0;
}
@@ -2869,14 +2880,17 @@ static int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
+/* This function must return a meaningful errno */
int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
struct object *o;
- if (!lock)
+ if (!lock) {
+ errno = EINVAL;
return -1;
+ }
if (!lock-force_write !hashcmp(lock-old_sha1, sha1)) {
unlock_ref(lock);
return 0;
@@ -2886,19 +2900,23 @@ int write_ref_sha1(struct ref_lock *lock,
error(Trying to write ref %s with nonexistent object %s,
lock-ref_name, sha1_to_hex(sha1));
unlock_ref(lock);
+ errno = EINVAL;
return -1;
}
if (o-type != OBJ_COMMIT is_branch(lock-ref_name)) {
error(Trying to write non-commit object %s to branch %s,
sha1_to_hex(sha1), lock-ref_name);
unlock_ref(lock);
+ errno = EINVAL;
return -1;
}
if (write_in_full(lock-lock_fd, sha1_to_hex(sha1), 40) != 40 ||
- write_in_full(lock-lock_fd, term, 1) != 1
- || close_ref(lock) 0) {
+ write_in_full(lock-lock_fd, term, 1) != 1 ||
+ close_ref(lock) 0) {
+ int save_errno = errno;
error(Couldn't write %s, lock-lk-filename);
unlock_ref(lock);
+ errno = save_errno;
return -1;
}
clear_loose_ref_cache(ref_cache);
--
2.0.0.438.g337c581
--
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 v19 17/48] update-ref: use err argument to get error from ref_transaction_commit
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 22617af..aec9004 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, msg, N_(reason), N_(reason of the
update)),
OPT_BOOL('d', NULL, delete, N_(delete the reference)),
@@ -359,18 +360,17 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die(Refusing to perform update with empty message.);
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
if (delete || no_deref || argc 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
+ if (ref_transaction_commit(transaction, msg, err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die(%s, err.buf);
ref_transaction_free(transaction);
- return ret;
+ return 0;
}
if (end_null)
--
2.0.0.438.g337c581
--
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 v19 02/48] refs.c: ref_transaction_commit should not free the transaction
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 1 +
refs.c | 1 -
refs.h | 5 ++---
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..1fd7a89 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -369,6 +369,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
update_refs_stdin();
ret = ref_transaction_commit(transaction, msg,
UPDATE_REFS_DIE_ON_ERR);
+ ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.c b/refs.c
index 6d841a0..d9cac6d 100644
--- a/refs.c
+++ b/refs.c
@@ -3509,7 +3509,6 @@ cleanup:
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
free(delnames);
- ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.h b/refs.h
index cfd1832..7d946f6 100644
--- a/refs.h
+++ b/refs.h
@@ -219,8 +219,7 @@ enum action_on_err {
/*
* Begin a reference transaction. The reference transaction must
- * eventually be commited using ref_transaction_commit() or freed by
- * calling ref_transaction_free().
+ * be freed by calling ref_transaction_free().
*/
struct ref_transaction *ref_transaction_begin(void);
@@ -268,7 +267,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
/*
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
- * problem. The ref_transaction is freed by this function.
+ * problem.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, enum action_on_err onerr);
--
2.0.0.438.g337c581
--
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 v19 12/48] refs.c: commit_packed_refs to return a meaningful errno on failure
Making errno when returning from commit_packed_refs() meaningful,
which should fix
* a bug in git clone where it prints strerror(errno) based on
errno, despite errno possibly being zero and potentially having
been clobbered by that point
* the same kind of bug in git pack-refs
and prepares for repack_without_refs() to get a meaningful
error message when commit_packed_refs() fails without falling into
the same bug.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 10 +-
refs.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index cc69581..7a815be 100644
--- a/refs.c
+++ b/refs.c
@@ -2239,11 +2239,16 @@ int lock_packed_refs(int flags)
return 0;
}
+/*
+ * Commit the packed refs changes.
+ * On error we must make sure that errno contains a meaningful value.
+ */
int commit_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(ref_cache);
int error = 0;
+ int save_errno = 0;
if (!packed_ref_cache-lock)
die(internal error: packed-refs not locked);
@@ -2253,10 +2258,13 @@ int commit_packed_refs(void)
do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
0, write_packed_entry_fn,
packed_ref_cache-lock-fd);
- if (commit_lock_file(packed_ref_cache-lock))
+ if (commit_lock_file(packed_ref_cache-lock)) {
+ save_errno = errno;
error = -1;
+ }
packed_ref_cache-lock = NULL;
release_packed_ref_cache(packed_ref_cache);
+ errno = save_errno;
return error;
}
diff --git a/refs.h b/refs.h
index 8d6cac7..e588ff8 100644
--- a/refs.h
+++ b/refs.h
@@ -98,6 +98,7 @@ extern void add_packed_ref(const char *refname, const
unsigned char *sha1);
* Write the current version of the packed refs cache from memory to
* disk. The packed-refs file must already be locked for writing (see
* lock_packed_refs()). Return zero on success.
+ * Sets errno to something meaningful on error.
*/
extern int commit_packed_refs(void);
--
2.0.0.438.g337c581
--
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 v19 05/48] refs.c: add a strbuf argument to ref_transaction_commit for error logging
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 2 +-
refs.c | 6 +-
refs.h | 5 -
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1fd7a89..22617af 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg,
+ ret = ref_transaction_commit(transaction, msg, NULL,
UPDATE_REFS_DIE_ON_ERR);
ref_transaction_free(transaction);
return ret;
diff --git a/refs.c b/refs.c
index 1d6dece..db05602 100644
--- a/refs.c
+++ b/refs.c
@@ -3444,7 +3444,8 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3473,6 +3474,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err)
+ strbuf_addf(err, Cannot lock the ref '%s'.,
+ update-refname);
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index 8c7f9c4..09d3564 100644
--- a/refs.h
+++ b/refs.h
@@ -269,9 +269,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr);
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr);
/*
* Free an existing transaction and all associated data.
--
2.0.0.438.g337c581
--
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 v18 16/48] refs.c: add an err argument to delete_ref_loose
I have resent v19 that does 1, remove the spurios redundant errno line 2, fixes the type and 3, reorders the patch as mentioned previously in this thread. This I hope will be the final version of the series we will need. regards ronnie sahlberg On Wed, Jun 18, 2014 at 3:38 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 06/19/2014 12:27 AM, Ronnie Sahlberg wrote: It looks like we need to reorder two of the patches. This patch needs to be moved to later in the series and happen after the delete_ref conversion : refs.c: make delete_ref use a transaction refs.c: add an err argument to delete_ref_loose That agrees with what I have found out since my first email. The failures go away starting with the later commit that you mentioned. I will respin a v19 with these patches reordered. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v19 06/48] lockfile.c: add a new public function unable_to_lock_message
Introducing a new unable_to_lock_message helper, which has nicer
semantics than unable_to_lock_error and cleans up lockfile.c a little.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
cache.h| 2 ++
lockfile.c | 22 --
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index cbe1935..8b12aa8 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_message(const char *path, int err,
+ struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..464031b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,33 +157,35 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return lk-fd;
}
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
{
- struct strbuf buf = STRBUF_INIT;
-
if (err == EEXIST) {
- strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
+ strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
If no other git process is currently running, this
probably means a\n
git process crashed in this repository earlier. Make sure
no other git\n
process is running and remove the file manually to
continue.,
absolute_path(path), strerror(err));
} else
- strbuf_addf(buf, Unable to create '%s.lock': %s,
+ strbuf_addf(buf, Unable to create '%s.lock': %s,
absolute_path(path), strerror(err));
- return strbuf_detach(buf, NULL);
}
int unable_to_lock_error(const char *path, int err)
{
- char *msg = unable_to_lock_message(path, err);
- error(%s, msg);
- free(msg);
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_message(path, err, buf);
+ error(%s, buf.buf);
+ strbuf_release(buf);
return -1;
}
NORETURN void unable_to_lock_index_die(const char *path, int err)
{
- die(%s, unable_to_lock_message(path, err));
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_message(path, err, buf);
+ die(%s, buf.buf);
}
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
--
2.0.0.438.g337c581
--
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 v19 09/48] refs.c: make sure log_ref_setup returns a meaningful errno
Making errno when returning from log_ref_setup() meaningful,
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 27 +++
refs.h | 4 +++-
2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index 67a0217..9ea519c 100644
--- a/refs.c
+++ b/refs.c
@@ -2751,6 +2751,7 @@ static int copy_msg(char *buf, const char *msg)
return cp - buf;
}
+/* This function must set a meaningful errno on failure */
int log_ref_setup(const char *refname, char *logfile, int bufsize)
{
int logfd, oflags = O_APPEND | O_WRONLY;
@@ -2761,9 +2762,12 @@ int log_ref_setup(const char *refname, char *logfile,
int bufsize)
starts_with(refname, refs/remotes/) ||
starts_with(refname, refs/notes/) ||
!strcmp(refname, HEAD))) {
- if (safe_create_leading_directories(logfile) 0)
- return error(unable to create directory for %s,
-logfile);
+ if (safe_create_leading_directories(logfile) 0) {
+ int save_errno = errno;
+ error(unable to create directory for %s, logfile);
+ errno = save_errno;
+ return -1;
+ }
oflags |= O_CREAT;
}
@@ -2774,15 +2778,22 @@ int log_ref_setup(const char *refname, char *logfile,
int bufsize)
if ((oflags O_CREAT) errno == EISDIR) {
if (remove_empty_directories(logfile)) {
- return error(There are still logs under '%s',
-logfile);
+ int save_errno = errno;
+ error(There are still logs under '%s',
+ logfile);
+ errno = save_errno;
+ return -1;
}
logfd = open(logfile, oflags, 0666);
}
- if (logfd 0)
- return error(Unable to append to %s: %s,
-logfile, strerror(errno));
+ if (logfd 0) {
+ int save_errno = errno;
+ error(Unable to append to %s: %s, logfile,
+ strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
}
adjust_shared_perm(logfile);
diff --git a/refs.h b/refs.h
index 65f7637..82cc5cb 100644
--- a/refs.h
+++ b/refs.h
@@ -158,7 +158,9 @@ extern void unlock_ref(struct ref_lock *lock);
/** Writes sha1 into the ref specified by the lock. **/
extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-/** Setup reflog before using. **/
+/*
+ * Setup reflog before using. Set errno to something meaningful on failure.
+ */
int log_ref_setup(const char *refname, char *logfile, int bufsize);
/** Reads log for the value of ref during at_time. **/
--
2.0.0.438.g337c581
--
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 v19 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
Making errno when returning from lock_file() meaningful, which should
fix
* an existing almost-bug in lock_ref_sha1_basic where it assumes
errno==ENOENT is meaningful and could waste some work on retries
* an existing bug in repack_without_refs where it prints
strerror(errno) and picks advice based on errno, despite errno
potentially being zero and potentially having been clobbered by
that point
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
lockfile.c | 17 -
refs.c | 1 +
refs.h | 1 +
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 464031b..a921d77 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
return p;
}
-
+/* Make sure errno contains a meaningful value on error */
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
/*
@@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
*/
static const size_t max_path_len = sizeof(lk-filename) - 5;
- if (strlen(path) = max_path_len)
+ if (strlen(path) = max_path_len) {
+ errno = ENAMETOOLONG;
return -1;
+ }
strcpy(lk-filename, path);
if (!(flags LOCK_NODEREF))
resolve_symlink(lk-filename, max_path_len);
@@ -148,9 +150,13 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
lock_file_list = lk;
lk-on_list = 1;
}
- if (adjust_shared_perm(lk-filename))
- return error(cannot fix permission bits on %s,
-lk-filename);
+ if (adjust_shared_perm(lk-filename)) {
+ int save_errno = errno;
+ error(cannot fix permission bits on %s,
+ lk-filename);
+ errno = save_errno;
+ return -1;
+ }
}
else
lk-filename[0] = 0;
@@ -188,6 +194,7 @@ NORETURN void unable_to_lock_index_die(const char *path,
int err)
die(%s, buf.buf);
}
+/* This should return a meaningful errno on failure */
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
{
int fd = lock_file(lk, path, flags);
diff --git a/refs.c b/refs.c
index db05602..e9d53e4 100644
--- a/refs.c
+++ b/refs.c
@@ -2212,6 +2212,7 @@ static int write_packed_entry_fn(struct ref_entry *entry,
void *cb_data)
return 0;
}
+/* This should return a meaningful errno on failure */
int lock_packed_refs(int flags)
{
struct packed_ref_cache *packed_ref_cache;
diff --git a/refs.h b/refs.h
index 09d3564..64f25d9 100644
--- a/refs.h
+++ b/refs.h
@@ -82,6 +82,7 @@ extern void warn_dangling_symrefs(FILE *fp, const char
*msg_fmt, const struct st
/*
* Lock the packed-refs file for writing. Flags is passed to
* hold_lock_file_for_update(). Return 0 on success.
+ * Errno is set to something meaningful on error.
*/
extern int lock_packed_refs(int flags);
--
2.0.0.438.g337c581
--
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] strbuf: add xstrdup_fmt helper
Jeff King p...@peff.net writes: On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote: str = xstrdup_fmt(fmt, some, args); --- I'm open to suggestions on the name. This really is the same thing conceptually as the GNU asprintf(), but the interface is different (that function takes a pointer-to-pointer as an out-parameter, and returns the number of characters return). Naming it with anything dup certainly feels wrong. The returned string is not a duplicate of anything. I was somewhat inspired by mkpathdup and friends. That name is from mkpath gives its result in a static buffer and forces the callers to xstrdup() if they want to keep the result; this is a thin wrapper to do so for the caller. As there is no str_fmt() that gives its result in a static, so... I considered that, but I do find asprintf's interface unnecessarily annoying... Yes, it is annoying. Would it be crazy to just call it xprintf? By our current scheme that would be a wrapper for printf, which means it is potentially confusing. Literally any other unused letter would be fine. dprintf for detach? I dunno. If we twist the logic behind the 'mkpathdup' name a little bit, perhaps we can call it sprintf_dup or something. That is, sprintf wants a fixed preallocated piece of memory to print into, and we relieve the callers of having to do so, perhaps? dprintf, mprintf, etc. are also fine by me. -- 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] strbuf: add xstrdup_fmt helper
Am 19.06.2014 11:05, schrieb Jeff King: On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote: str = xstrdup_fmt(fmt, some, args); --- I'm open to suggestions on the name. This really is the same thing conceptually as the GNU asprintf(), but the interface is different (that function takes a pointer-to-pointer as an out-parameter, and returns the number of characters return). Naming it with anything dup certainly feels wrong. The returned string is not a duplicate of anything. I was somewhat inspired by mkpathdup and friends. It is quite simialr to mkpathdup, except that it is not limited to paths (and does not do any path-specific munging). I agree something with printf in the name is probably more descriptive, though. I wonder if our callers can instead use asprintf(3) with its slightly more quirky API (and then we supply compat/asprintf.c for non-GNU platforms). Right now we only have three call sites, but if we anticipate that printf-like format into an allocated piece of memory will prove be generally useful in our code base, following an API that other people already have established may give our developers one less thing that they have to learn. I considered that, but I do find asprintf's interface unnecessarily annoying (you can't return a value directly, and you run afoul of const issues when passing pointers to pointers). As you noted, it's not _too_ bad, but we really get nothing from the integer return type. AFAICT, it helps with: 1. You know how many characters are in the string. If you cared about that here, you would just use a strbuf directly, which is much more flexible. 2. The error handling is different. But our x-variant would just die() on errors anyway, so we do not care. So modeling after asprintf feels like carrying around unnecessary baggage (and I am not convinced asprintf is in wide enough use, nor that the function is complicated enough to care about developer familiarity). Part of me is tempted to call it xasprintf anyway, and use our own interface. GNU does not get to squat on that useful name. ;) That is probably unnecessarily confusing, though. As usual, I would expect we would have xasprintf wrapper around it to die instead of returning -1 upon allocation failure. Would it be crazy to just call it xprintf? By our current scheme that would be a wrapper for printf, which means it is potentially confusing. Literally any other unused letter would be fine. dprintf for detach? I dunno. I agree that dup doesn't fit and that potential callers don't need the length of the generated string (or should use strbuf otherwise). Including print in the name is not necessary, though -- die(), error() and friends work fine without them. What about simply removing the dup_ part and using xstrfmt? René -- 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: Conventions on struct copying?
brian m. carlson sand...@crustytoothpaste.net writes: I don't know of any place we explicitly copy structs like this,... which should be a reason enough. The first concrete guideline is just imitate the existing code. but I don't know of any prohibition against it, either. So now you know ;-). -- 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: Surprising 'git-describe --all --match' behavior.
Sergei Organov o...@javad.com writes: Just playing with it, got some surprises: $ git --version git version 1.9.3 $ git describe --all heads/v3.5 $ git describe --all --match 'v*' tags/v3.5.6b2-4-gab4bf78 $ git describe --all --match 'heads/v*' fatal: No names found, cannot describe anything. I think $ git describe --help ... --match pattern Only consider tags matching the given glob(7) pattern, excluding the refs/tags/ prefix. This can be used to avoid leaking private tags from the repository. ... is poorly phrased, especially its excluding part. What it wants to say is You give pattern but without refs/tags/, because the program helpfully always prepend refs/tags/ to your pattern and limit the output to those that match. Hence you gave 'v*' as pattern and limited the output to those that match 'refs/tags/v*' (or you gave 'heads/v*' and limited to 'refs/tags/heads/v*'). -- 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: Conventions on struct copying?
-Original Message- From: Junio C Hamano Sent: Thursday, June 19, 2014 13:11 brian m. carlson sand...@crustytoothpaste.net writes: I don't know of any place we explicitly copy structs like this,... which should be a reason enough. The first concrete guideline is just imitate the existing code. but I don't know of any prohibition against it, either. So now you know ;-). To expand, on that do not trust the compiler to do deep copies. http://stackoverflow.com/questions/2302351/assign-one-struct-to-another-in-c Hit #1 on https://www.google.com/search?q=c+assignment+of+struct -- -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- - - - Jason Pyeron PD Inc. http://www.pdinc.us - - Principal Consultant 10 West 24th Street #100- - +1 (443) 269-1555 x333Baltimore, Maryland 21218 - - - -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- This message is copyright PD Inc, subject to license 20080407P00. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] cleanup duplicate name_compare() functions
Jeremiah Mahler jmmah...@gmail.com writes: Both unpack-trees.c and read-cache.c have their own name_compare() function, which are identical. And read-cache.c has a cache_name_compare() function which is nearly identical to name_compare() [1]. The cache_name_compare() function is not specific to a cache, other than by being part of cache.h. 'other than by designed to be used only for comparing names in the cache entries' is probably more accurate, I would think. Generalize the cache_name_compare() function by renaming it to name_compare(). Simplify the cache_name_stage_compare() function using name_compare(). Then change the few instances which used cache_name_compare() to name_compare() [2]. [1] cache_name_compare() is not identical to name_compare(). The former returns +1, -1, whereas the latter returns +N, -N. But there is no place where name_compare() is used that needs the magnitude so this difference does not alter its behavior. You chose to use the one that loses the information by unifying these two into the variant that only returns -1/0/+1. We know that it does not matter for the current callers, but is it expected that no future callers will benefit by having the magnitude information? [2] The instances where cache_name_compare() is used have nothing to do with a cache. The new name, name_compare(), makes it clear that no cache is involved. This is redundant and should be dropped, as you already said is not specific to a cache earlier. Signed-off-by: Jeremiah Mahler jmmah...@gmail.com --- cache.h| 2 +- dir.c | 3 +-- name-hash.c| 2 +- read-cache.c | 23 +-- tree-walk.c| 10 -- unpack-trees.c | 11 --- 6 files changed, 16 insertions(+), 35 deletions(-) diff --git a/cache.h b/cache.h index c498a30..e3205fe 100644 --- a/cache.h +++ b/cache.h @@ -1027,7 +1027,7 @@ extern int validate_headref(const char *ref); extern int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); extern int df_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); -extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2); +extern int name_compare(const char *name1, size_t len1, const char *name2, size_t len2); extern int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2); extern void *read_object_with_reference(const unsigned char *sha1, diff --git a/dir.c b/dir.c index 797805d..e65888d 100644 --- a/dir.c +++ b/dir.c @@ -1354,8 +1354,7 @@ static int cmp_name(const void *p1, const void *p2) const struct dir_entry *e1 = *(const struct dir_entry **)p1; const struct dir_entry *e2 = *(const struct dir_entry **)p2; - return cache_name_compare(e1-name, e1-len, - e2-name, e2-len); + return name_compare(e1-name, e1-len, e2-name, e2-len); } static struct path_simplify *create_simplify(const char **pathspec) diff --git a/name-hash.c b/name-hash.c index be7c4ae..e2bea88 100644 --- a/name-hash.c +++ b/name-hash.c @@ -179,7 +179,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen * Always do exact compare, even if we want a case-ignoring comparison; * we do the quick exact one first, because it will be the common case. */ - if (len == namelen !cache_name_compare(name, namelen, ce-name, len)) + if (len == namelen !name_compare(name, namelen, ce-name, len)) return 1; The existing code is somewhat strange; while the update is correct in the context of this patch, it may further want to be fixed in a later patch to either !name_compare(name, namelen, ce-name, len) or len == namelen !memcmp(name, ce-name, len) The patch text looks good. 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 v4] cleanup duplicate name_compare() functions
On Thu, Jun 19, 2014 at 11:03 AM, Junio C Hamano gits...@pobox.com wrote: You chose to use the one that loses the information by unifying these two into the variant that only returns -1/0/+1. We know that it does not matter for the current callers, but is it expected that no future callers will benefit by having the magnitude information? Heh, I was being silly, partly fooled by your reference to magnitude. You are not losing information at all, because the caller cannot tell if the return value came from an earlier memcmp(), whose only guarantee is that the sign of the returned value is all that matters, or from the later subtraction between lengths. So unifying to the -1/0/+1 variant is entirely justifiable. It is just your rationale was a bit misleading. We often represent our strings as a counted string, i.e. a pair of the pointer to the beginning of the string and its length, and the string may not be NUL terminated to that length. To compare a pair of such counted strings, unpack-trees.c and read-cache.c implement their own name_compare() functions identically. In addition, cache_name_compare() function in read-cache.c is nearly identical. The only difference is when one string is the prefix of the other string, in which case the former returns -1/+1 to show which one is longer and the latter returns the difference of the lengths to show the same information. Unify these three functions by using the implementation from cache_name_compare(). This does not make any difference to the existing and future callers, as they must be paying attention only to the sign of the returned value (and not the magnitude) because the original implementations of these two functions return values returned by memcmp(3) when the one string is not a prefix of the other string, and the only thing memcmp(3) guarantees its callers is the sign of the returned value, not the magnitude. or something like that, perhaps? -- 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: Surprising 'git-describe --all --match' behavior.
Sergei Organov o...@javad.com writes: Will something break if it won't helpfully prepend refs/tags/ once --all is given? describe --all --match 'v*' will no longer match a tag v1.2.3, and forces the users to say describe --match 'refs/tags/v*', and these users will probably see it as a new breakage, I would imagine. -- 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 v2 07/14] bisect: add t6041 for submodule updates
Test that the bisect command updates the work tree as expected. To make
that work with the new submodule test framework a git_bisect helper
function is added. This adds a commit after the one given to be switched
to and makes that one the bad commit. The starting point is then given to
bisect as the good commit which makes bisect change the work tree to the
commit in between, which is the commit given.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Changes to v1:
*) use expect instead of expected
t/t6041-bisect-submodule.sh | 32
1 file changed, 32 insertions(+)
create mode 100755 t/t6041-bisect-submodule.sh
diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
new file mode 100755
index 000..c6b7aa6
--- /dev/null
+++ b/t/t6041-bisect-submodule.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='bisect can handle submodules'
+
+. ./test-lib.sh
+. $TEST_DIRECTORY/lib-submodule-update.sh
+
+git_bisect () {
+ git status -su expect
+ ls -1pR * expect
+ tar czf $TRASH_DIRECTORY/tmp.tgz *
+ GOOD=$(git rev-parse --verify HEAD)
+ git checkout $1
+ echo foo bar
+ git add bar
+ git commit -m bisect bad
+ BAD=$(git rev-parse --verify HEAD)
+ git reset --hard HEAD^^
+ git submodule update
+ git bisect start
+ git bisect good $GOOD
+ rm -rf *
+ tar xzf $TRASH_DIRECTORY/tmp.tgz
+ git status -su actual
+ ls -1pR * actual
+ test_cmp expect actual
+ git bisect bad $BAD
+}
+
+test_submodule_switch git_bisect
+
+test_done
--
2.0.0.406.gf4dce28
--
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: Surprising 'git-describe --all --match' behavior.
Junio C Hamano gits...@pobox.com writes: Sergei Organov o...@javad.com writes: Will something break if it won't helpfully prepend refs/tags/ once --all is given? describe --all --match 'v*' will no longer match a tag v1.2.3, and forces the users to say describe --match 'refs/tags/v*', No, descirbe --match 'v*' or describe --tags --match 'v*' depending on what they actually meant. Notice my once --all is given above. Those who used --all meant to match against all the refs, no? and these users will probably see it as a new breakage, I would imagine. But why would anybody use --all --match if they only meant --tags --match or even just --match alone? Was it historically --all that was first introduced, maybe? -- Sergei. -- 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 v2 01/14] test-lib: add test_dir_is_empty()
For the upcoming submodule test framework we often need to assert that an
empty directory exists in the work tree. Add the test_dir_is_empty()
function which asserts that the given argument is an empty directory.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Changes to v1:
*) Added -n to test expression (you have this as SQUASH??? commit in pu)
t/test-lib-functions.sh | 11 +++
1 file changed, 11 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index c617c82..acd9a55 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -489,6 +489,17 @@ test_path_is_dir () {
fi
}
+# Check if the directory exists and is empty as expected, barf otherwise.
+test_dir_is_empty () {
+ test_path_is_dir $1
+ if test -n $(ls -a1 $1 | egrep -v '^\.\.?$')
+ then
+ echo Directory '$1' is not empty, it contains:
+ ls -la $1
+ return 1
+ fi
+}
+
test_path_is_missing () {
if [ -e $1 ]
then
--
2.0.0.406.gf4dce28
--
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 v2 02/14] submodules: Add the lib-submodule-update.sh test library
Add this test library to simplify covering all combinations of submodule
update scenarios without having to add those to a test of each work tree
manipulating command over and over again.
The functions test_submodule_switch() and test_submodule_forced_switch()
are intended to be called from a test script with a single argument. This
argument is either a work tree manipulating command (including any command
line options) or a function (when more than a single git command is needed
to switch work trees from the current HEAD to another commit). This
command (or function) is passed a target branch as argument. The two new
functions check that each submodule transition is handled as expected,
which currently means that submodule work trees are not affected until
git submodule update is called. The forced variant is for commands
using their '-f' or '--hard' option and expects them to overwrite local
modifications as a result. Each of these two functions contains 14
tests_expect_* calls.
Calling one of these test functions the first time creates a repository
named submodule_update_repo. At first it contains two files, then a
single submodule is added in another commit followed by commits covering
all relevant submodule modifications. This repository is newly cloned into
the submodule_update for each test_expect_* to avoid interference
between different parts of the test functions (some to-be-tested commands
also manipulate refs along with the work tree, e.g. git reset).
Follow-up commits will then call these two test functions for all work
tree manipulating commands (with a combination of all their options
relevant to what they do with the work tree) making sure they work as
expected. Later this test library will be extended to cover merges
resulting in conflicts too. Also it is intended to be easily extendable
for the recursive update functionality, where even more combinations of
submodule modifications have to be tested for.
This version documents two bugs in current Git with expected failures:
*) When a submodule is replaced with a tracked file of the same name the
submodule work tree including any local modifications (and even the
whole history if it uses a .git directory instead of a gitfile!) is
silently removed.
*) Forced work tree updates happily manipulate files in the directory of a
submodule that has just been removed in the superproject (but is of
course still present in the work tree due to the way submodules are
currently handled). This becomes dangerous when files in the submodule
directory are overwritten by files from the new superproject commit, as
any modifications to the submodule files will be lost) and is expected
to also destroy history in the - admittedly unlikely case - the new
commit adds a file named .git to the submodule directory.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Changes to v1:
*) avoid non-portable cp -a
*) use diff -r without git when comparing directory trees
*) simplify retrieving the SHA-1 of sub1
*) call test_git_directory_is_unchanged before test_submodule_content (this
was the reason for two test failures after removing the git before
diff as test_submodule_content would sometimes modify the index)
t/lib-submodule-update.sh | 632 ++
1 file changed, 632 insertions(+)
create mode 100755 t/lib-submodule-update.sh
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
new file mode 100755
index 000..03217b2
--- /dev/null
+++ b/t/lib-submodule-update.sh
@@ -0,0 +1,632 @@
+# Create a submodule layout used for all tests below.
+#
+# The following use cases are covered:
+# - New submodule (no_submodule = add_sub1)
+# - Removed submodule (add_sub1 = remove_sub1)
+# - Updated submodule (add_sub1 = modify_sub1)
+# - Submodule updated to invalid commit (add_sub1 = invalid_sub1)
+# - Submodule updated from invalid commit (invalid_sub1 = valid_sub1)
+# - Submodule replaced by tracked files in directory (add_sub1 =
+# replace_sub1_with_directory)
+# - Directory containing tracked files replaced by submodule
+# (replace_sub1_with_directory = replace_directory_with_sub1)
+# - Submodule replaced by tracked file with the same name (add_sub1 =
+# replace_sub1_with_file)
+# - Tracked file replaced by submodule (replace_sub1_with_file =
+# replace_file_with_sub1)
+#
+# --O-O
+# / ^ replace_directory_with_sub1
+# / replace_sub1_with_directory
+#/O
+# / ^
+# / modify_sub1
+# O--O---O
+# ^ ^\ ^
+# | | \ remove_sub1
+# | | -O-O
+# | | \ ^ replace_file_with_sub1
+# | |\ replace_sub1_with_file
+# | add_sub1 --O-O
+# no_submodule^ valid_sub1
+# invalid_sub1
+#
+create_lib_submodule_repo () {
+ git init
[PATCH v2 13/14] stash: add t3906 for submodule updates
Test that the stash apply command updates the work tree as expected for
changes which don't result in conflicts. To make that work add a helper
function that uses read-tree to apply the changes of the target commit
to the work tree, then stashes these changes and at last applies that
stash.
Implement the KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES switch
and reuse two other already present switches to expect the known
failure that stash does ignore submodule changes.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Changes to v1:
*) use expect instead of expected
t/lib-submodule-update.sh | 23 ++-
t/t3906-stash-submodule.sh | 24
2 files changed, 42 insertions(+), 5 deletions(-)
create mode 100755 t/t3906-stash-submodule.sh
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index a38cf52..2a504b2 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -219,7 +219,14 @@ test_submodule_switch () {
command=$1
# Appearing submodule #
# Switching to a commit letting a submodule appear creates empty dir ...
- test_expect_success $command: added submodule creates empty directory
'
+ if test $KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES = 1
+ then
+ # Restoring stash fails to restore submodule index entry
+ RESULT=failure
+ else
+ RESULT=success
+ fi
+ test_expect_$RESULT $command: added submodule creates empty directory
'
prolog
reset_work_tree_to no_submodule
(
@@ -233,7 +240,7 @@ test_submodule_switch () {
)
'
# ... and doesn't care if it already exists ...
- test_expect_success $command: added submodule leaves existing empty
directory alone '
+ test_expect_$RESULT $command: added submodule leaves existing empty
directory alone '
prolog
reset_work_tree_to no_submodule
(
@@ -262,7 +269,7 @@ test_submodule_switch () {
'
# Replacing a tracked file with a submodule produces an empty
# directory ...
- test_expect_success $command: replace tracked file with submodule
creates empty directory '
+ test_expect_$RESULT $command: replace tracked file with submodule
creates empty directory '
prolog
reset_work_tree_to replace_sub1_with_file
(
@@ -302,7 +309,13 @@ test_submodule_switch () {
Disappearing submodule ###
# Removing a submodule doesn't remove its work tree ...
- test_expect_success $command: removed submodule leaves submodule
directory and its contents in place '
+ if test $KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES = 1
+ then
+ RESULT=failure
+ else
+ RESULT=success
+ fi
+ test_expect_$RESULT $command: removed submodule leaves submodule
directory and its contents in place '
prolog
reset_work_tree_to add_sub1
(
@@ -314,7 +327,7 @@ test_submodule_switch () {
)
'
# ... especially when it contains a .git directory.
- test_expect_success $command: removed submodule leaves submodule
containing a .git directory alone '
+ test_expect_$RESULT $command: removed submodule leaves submodule
containing a .git directory alone '
prolog
reset_work_tree_to add_sub1
(
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
new file mode 100755
index 000..d7219d6
--- /dev/null
+++ b/t/t3906-stash-submodule.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+test_description='stash apply can handle submodules'
+
+. ./test-lib.sh
+. $TEST_DIRECTORY/lib-submodule-update.sh
+
+git_stash () {
+ git status -su expect
+ ls -1pR * expect
+ git read-tree -u -m $1
+ git stash
+ git status -su actual
+ ls -1pR * actual
+ test_cmp expect actual
+ git stash apply
+}
+
+KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
+KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
+KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
+test_submodule_switch git_stash
+
+test_done
--
2.0.0.406.gf4dce28
--
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 v2 09/14] rebase: add t3426 for submodule updates
Test that the rebase command updates the work tree as expected for
changes which don't result in conflicts. To make that work add two
helper functions that add a commit only touching files and then
revert it. This allows to rebase the target commit over these two
and to compare the result.
Set KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR to
document that replace directory with submodule fails for an
interactive rebase because a directory sub1 already exists.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Changes to v1:
*) fix broken -chain (you have this as SQUASH??? commit in pu)
*) use expect instead of expected
t/t3426-rebase-submodule.sh | 46 +
1 file changed, 46 insertions(+)
create mode 100755 t/t3426-rebase-submodule.sh
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
new file mode 100755
index 000..d5b896d
--- /dev/null
+++ b/t/t3426-rebase-submodule.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='rebase can handle submodules'
+
+. ./test-lib.sh
+. $TEST_DIRECTORY/lib-submodule-update.sh
+. $TEST_DIRECTORY/lib-rebase.sh
+
+git_rebase () {
+ git status -su expect
+ ls -1pR * expect
+ git checkout -b ours HEAD
+ echo x file1
+ git add file1
+ git commit -m add_x
+ git revert HEAD
+ git status -su actual
+ ls -1pR * actual
+ test_cmp expect actual
+ git rebase $1
+}
+
+test_submodule_switch git_rebase
+
+git_rebase_interactive () {
+ git status -su expect
+ ls -1pR * expect
+ git checkout -b ours HEAD
+ echo x file1
+ git add file1
+ git commit -m add_x
+ git revert HEAD
+ git status -su actual
+ ls -1pR * actual
+ test_cmp expect actual
+ set_fake_editor
+ echo fake-editor.sh .git/info/exclude
+ git rebase -i $1
+}
+
+KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
+# The real reason replace directory with submodule fails is because a
+# directory sub1 exists, but we reuse the suppression added for merge here
+test_submodule_switch git_rebase_interactive
+
+test_done
--
2.0.0.406.gf4dce28
--
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 v2 14/14] revert: add t3513 for submodule updates
Test that the revert command updates the work tree as expected (for
submodule changes which don't result in conflicts). Add a helper function
to first revert the checked out target commit to make the last revert
produce the to-be-tested work tree.
Set the KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT and
KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR switches to
document that revert has the similar failures.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Changes to v1:
*) use expect instead of expected
t/t3513-revert-submodule.sh | 32
1 file changed, 32 insertions(+)
create mode 100755 t/t3513-revert-submodule.sh
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
new file mode 100755
index 000..a1c4e02
--- /dev/null
+++ b/t/t3513-revert-submodule.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='revert can handle submodules'
+
+. ./test-lib.sh
+. $TEST_DIRECTORY/lib-submodule-update.sh
+
+# Create a revert that moves from HEAD (including any test modifications to
+# the work tree) to $1 by first checking out $1 and reverting it. Reverting
+# the revert is the transition we test for. We tar the current work tree
+# first so we can restore the work tree test setup after doing the checkout
+# and revert. We test here that the restored work tree content is identical
+# to that at the beginning. The last revert is then tested by the framework.
+git_revert () {
+ git status -su expect
+ ls -1pR * expect
+ tar czf $TRASH_DIRECTORY/tmp.tgz *
+ git checkout $1
+ git revert HEAD
+ rm -rf *
+ tar xzf $TRASH_DIRECTORY/tmp.tgz
+ git status -su actual
+ ls -1pR * actual
+ test_cmp expect actual
+ git revert HEAD
+}
+
+KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
+KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
+test_submodule_switch git_revert
+
+test_done
--
2.0.0.406.gf4dce28
--
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: What's cooking in git.git (Jun 2014, #04; Tue, 17)
Duy Nguyen pclo...@gmail.com writes: On Thu, Jun 19, 2014 at 12:25 AM, Junio C Hamano gits...@pobox.com wrote: [Stalled] * nd/multiple-work-trees (2014-03-25) 28 commits - count-objects: report unused files in $GIT_DIR/repos/... - gc: support prune --repos - gc: style change -- no SP before closing bracket - prune: strategies for linked checkouts - checkout: detach if the branch is already checked out elsewhere - checkout: clean up half-prepared directories in --to mode - checkout: support checking out into a new working directory - use new wrapper write_file() for simple file writing - wrapper.c: wrapper to open a file, fprintf then close - setup.c: support multi-checkout repo setup - setup.c: detect $GIT_COMMON_DIR check_repository_format_gently() - setup.c: convert check_repository_format_gently to use strbuf - setup.c: detect $GIT_COMMON_DIR in is_git_directory() - setup.c: convert is_git_directory() to use strbuf - git-stash: avoid hardcoding $GIT_DIR/logs/ - *.sh: avoid hardcoding $GIT_DIR/hooks/... - git-sh-setup.sh: use rev-parse --git-path to get $GIT_DIR/objects - $GIT_COMMON_DIR: a new environment variable - commit: use SEQ_DIR instead of hardcoding sequencer - fast-import: use git_path() for accessing .git dir instead of get_git_dir() - reflog: avoid constructing .lock path with git_path - *.sh: respect $GIT_INDEX_FILE - git_path(): be aware of file relocation in $GIT_DIR - path.c: group git_path(), git_pathdup() and strbuf_git_path() together - path.c: rename vsnpath() to do_git_path() - git_snpath(): retire and replace with strbuf_git_path() - path.c: make get_pathname() call sites return const char * - path.c: make get_pathname() return strbuf instead of static buffer A replacement for contrib/workdir/git-new-workdir that does not rely on symbolic links and make sharing of objects and refs safer by making the borrowee and borrowers aware of each other. Anything I can do to get this going again? The only thing I just found (and have not fixed) is, I think $GIT_DIR/info/excludes and $GIT_DIR/info/sparse-checkout should be per-worktree, not shared. I threw it into the stalled category because you said even you do not use it yourself (you may also have said that it has issues already known and not fixed) in the first place. If we want to resuscitate it, a good point to start is to refresh it with a reroll to address known issues, I would guess. 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 v4] cleanup duplicate name_compare() functions
Junio, On Thu, Jun 19, 2014 at 11:03:03AM -0700, Junio C Hamano wrote: Jeremiah Mahler jmmah...@gmail.com writes: Both unpack-trees.c and read-cache.c have their own name_compare() function, which are identical. And read-cache.c has a cache_name_compare() function which is nearly identical to name_compare() [1]. The cache_name_compare() function is not specific to a cache, other than by being part of cache.h. 'other than by designed to be used only for comparing names in the cache entries' is probably more accurate, I would think. Generalize the cache_name_compare() function by renaming it to name_compare(). Simplify the cache_name_stage_compare() function using name_compare(). Then change the few instances which used cache_name_compare() to name_compare() [2]. [1] cache_name_compare() is not identical to name_compare(). The former returns +1, -1, whereas the latter returns +N, -N. But there is no place where name_compare() is used that needs the magnitude so this difference does not alter its behavior. You chose to use the one that loses the information by unifying these two into the variant that only returns -1/0/+1. We know that it does not matter for the current callers, but is it expected that no future callers will benefit by having the magnitude information? [2] The instances where cache_name_compare() is used have nothing to do with a cache. The new name, name_compare(), makes it clear that no cache is involved. This is redundant and should be dropped, as you already said is not specific to a cache earlier. Signed-off-by: Jeremiah Mahler jmmah...@gmail.com --- cache.h| 2 +- dir.c | 3 +-- name-hash.c| 2 +- read-cache.c | 23 +-- tree-walk.c| 10 -- unpack-trees.c | 11 --- 6 files changed, 16 insertions(+), 35 deletions(-) diff --git a/cache.h b/cache.h index c498a30..e3205fe 100644 --- a/cache.h +++ b/cache.h @@ -1027,7 +1027,7 @@ extern int validate_headref(const char *ref); extern int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); extern int df_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); -extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2); +extern int name_compare(const char *name1, size_t len1, const char *name2, size_t len2); extern int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2); extern void *read_object_with_reference(const unsigned char *sha1, diff --git a/dir.c b/dir.c index 797805d..e65888d 100644 --- a/dir.c +++ b/dir.c @@ -1354,8 +1354,7 @@ static int cmp_name(const void *p1, const void *p2) const struct dir_entry *e1 = *(const struct dir_entry **)p1; const struct dir_entry *e2 = *(const struct dir_entry **)p2; - return cache_name_compare(e1-name, e1-len, - e2-name, e2-len); + return name_compare(e1-name, e1-len, e2-name, e2-len); } static struct path_simplify *create_simplify(const char **pathspec) diff --git a/name-hash.c b/name-hash.c index be7c4ae..e2bea88 100644 --- a/name-hash.c +++ b/name-hash.c @@ -179,7 +179,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen * Always do exact compare, even if we want a case-ignoring comparison; * we do the quick exact one first, because it will be the common case. */ - if (len == namelen !cache_name_compare(name, namelen, ce-name, len)) + if (len == namelen !name_compare(name, namelen, ce-name, len)) return 1; The existing code is somewhat strange; while the update is correct in the context of this patch, it may further want to be fixed in a later patch to either !name_compare(name, namelen, ce-name, len) or len == namelen !memcmp(name, ce-name, len) I did not notice that, good catch. Since that line is going to be changed I can make a short fixup patch before the main patch and avoid the rename. The patch text looks good. Thanks. -- Jeremiah Mahler jmmah...@gmail.com http://github.com/jmahler -- 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 v2] dropping manual malloc calculations
On Thu, Jun 19, 2014 at 09:49:41AM -0700, Junio C Hamano wrote: If we twist the logic behind the 'mkpathdup' name a little bit, perhaps we can call it sprintf_dup or something. That is, sprintf wants a fixed preallocated piece of memory to print into, and we relieve the callers of having to do so, perhaps? Right, that is the dup I was thinking of. Anyway, I think René's xstrfmt is my favorite of the suggestions, and you seemed to like it, too. Here is a re-roll using that name. The first two patches are the same except for the name change. The rest are more applications of it (and other techniques) found by grepping for malloc.*strlen. The diffstat is very encouraging, and I think the results are much more readable (in addition to being more obviously correct). The last two also fix real bugs, but I doubt either is a problem in practice. builtin/apply.c | 4 +-- builtin/fetch.c | 9 ++ builtin/fmt-merge-msg.c | 7 ++--- builtin/name-rev.c | 5 +--- builtin/receive-pack.c | 5 +--- builtin/show-branch.c | 10 +++ environment.c | 12 +++- http-push.c | 24 +--- http-walker.c | 6 ++-- match-trees.c | 9 ++ merge-recursive.c | 53 --- merge.c | 42 +-- remote.c| 6 +--- sequencer.c | 9 ++ sha1_name.c | 5 +--- shell.c | 6 +--- strbuf.c| 19 + strbuf.h| 9 ++ unpack-trees.c | 17 --- walker.c| 18 ++-- 20 files changed, 117 insertions(+), 158 deletions(-) [01/10]: strbuf: add xstrfmt helper [02/10]: use xstrfmt in favor of manual size calculations [03/10]: use xstrdup instead of xmalloc + strcpy [04/10]: use xstrfmt to replace xmalloc + sprintf [05/10]: use xstrfmt to replace xmalloc + strcpy/strcat [06/10]: setup_git_env: use git_pathdup instead of xmalloc + sprintf [07/10]: sequencer: use argv_array_pushf [08/10]: merge: use argv_array when spawning merge strategy [09/10]: walker_fetch: fix minor memory leak [10/10]: unique_path: fix unlikely heap overflow -- 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 v2 01/10] strbuf: add xstrfmt helper
You can use a strbuf to build up a string from parts, and
then detach it. In the general case, you might use multiple
strbuf_add* functions to do the building. However, in many
cases, a single strbuf_addf is sufficient, and we end up
with:
struct strbuf buf = STRBUF_INIT;
...
strbuf_addf(buf, fmt, some, args);
str = strbuf_detach(buf, NULL);
We can make this much more readable (and avoid introducing
an extra variable, which can clutter the code) by
introducing a convenience function:
str = xstrfmt(fmt, some, args);
Signed-off-by: Jeff King p...@peff.net
---
strbuf.c | 19 +++
strbuf.h | 9 +
2 files changed, 28 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index ac62982..12c7865 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -600,3 +600,22 @@ char *xstrdup_tolower(const char *string)
result[i] = '\0';
return result;
}
+
+char *xstrvfmt(const char *fmt, va_list ap)
+{
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_vaddf(buf, fmt, ap);
+ return strbuf_detach(buf, NULL);
+}
+
+char *xstrfmt(const char *fmt, ...)
+{
+ va_list ap;
+ char *ret;
+
+ va_start(ap, fmt);
+ ret = xstrvfmt(fmt, ap);
+ va_end(ap);
+
+ return ret;
+}
diff --git a/strbuf.h b/strbuf.h
index e9ad03e..a594c24 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -187,4 +187,13 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);
char *xstrdup_tolower(const char *);
+/*
+ * Create a newly allocated string using printf format. You can do this easily
+ * with a strbuf, but this provides a shortcut to save a few lines.
+ */
+__attribute__((format (printf, 1, 0)))
+char *xstrvfmt(const char *fmt, va_list ap);
+__attribute__((format (printf, 1, 2)))
+char *xstrfmt(const char *fmt, ...);
+
#endif /* STRBUF_H */
--
2.0.0.566.gfe3e6b2
--
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 v2 03/10] use xstrdup instead of xmalloc + strcpy
This is one line shorter, and makes sure the length in the
malloc and copy steps match.
Signed-off-by: Jeff King p...@peff.net
---
builtin/receive-pack.c | 5 +
http-push.c| 6 ++
http-walker.c | 3 +--
3 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..18458e8 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -614,12 +614,9 @@ static void run_update_post_hook(struct command *commands)
argv[0] = hook;
for (argc = 1, cmd = commands; cmd; cmd = cmd-next) {
- char *p;
if (cmd-error_string || cmd-did_not_exist)
continue;
- p = xmalloc(strlen(cmd-ref_name) + 1);
- strcpy(p, cmd-ref_name);
- argv[argc] = p;
+ argv[argc] = xstrdup(cmd-ref_name);
argc++;
}
argv[argc] = NULL;
diff --git a/http-push.c b/http-push.c
index de00d16..95650a0 100644
--- a/http-push.c
+++ b/http-push.c
@@ -767,15 +767,13 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int
tag_closed)
if (tag_closed ctx-cdata) {
if (!strcmp(ctx-name, DAV_ACTIVELOCK_OWNER)) {
- lock-owner = xmalloc(strlen(ctx-cdata) + 1);
- strcpy(lock-owner, ctx-cdata);
+ lock-owner = xstrdup(ctx-cdata);
} else if (!strcmp(ctx-name, DAV_ACTIVELOCK_TIMEOUT)) {
if (starts_with(ctx-cdata, Second-))
lock-timeout =
strtol(ctx-cdata + 7, NULL, 10);
} else if (!strcmp(ctx-name, DAV_ACTIVELOCK_TOKEN)) {
- lock-token = xmalloc(strlen(ctx-cdata) + 1);
- strcpy(lock-token, ctx-cdata);
+ lock-token = xstrdup(ctx-cdata);
git_SHA1_Init(sha_ctx);
git_SHA1_Update(sha_ctx, lock-token,
strlen(lock-token));
diff --git a/http-walker.c b/http-walker.c
index 1516c5e..ab6a4fe 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -566,8 +566,7 @@ struct walker *get_http_walker(const char *url)
struct walker *walker = xmalloc(sizeof(struct walker));
data-alt = xmalloc(sizeof(*data-alt));
- data-alt-base = xmalloc(strlen(url) + 1);
- strcpy(data-alt-base, url);
+ data-alt-base = xstrdup(url);
for (s = data-alt-base + strlen(data-alt-base) - 1; *s == '/'; --s)
*s = 0;
--
2.0.0.566.gfe3e6b2
--
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 v2 02/10] use xstrfmt in favor of manual size calculations
In many parts of the code, we do an ugly and error-prone
malloc like:
const char *fmt = something %s;
buf = xmalloc(strlen(foo) + 10 + 1);
sprintf(buf, fmt, foo);
This makes the code brittle, and if we ever get the
allocation wrong, is a potential heap overflow. Let's
instead favor xstrfmt, which handles the allocation
automatically, and makes the code shorter and more readable.
Signed-off-by: Jeff King p...@peff.net
---
These could actually be squashed into later commits, I suppose, but I
left it separate since it had already been reviewed.
remote.c | 6 +-
unpack-trees.c | 17 ++---
2 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/remote.c b/remote.c
index 0e9459c..bf27e44 100644
--- a/remote.c
+++ b/remote.c
@@ -170,7 +170,6 @@ static struct branch *make_branch(const char *name, int len)
{
struct branch *ret;
int i;
- char *refname;
for (i = 0; i branches_nr; i++) {
if (len ? (!strncmp(name, branches[i]-name, len)
@@ -186,10 +185,7 @@ static struct branch *make_branch(const char *name, int
len)
ret-name = xstrndup(name, len);
else
ret-name = xstrdup(name);
- refname = xmalloc(strlen(name) + strlen(refs/heads/) + 1);
- strcpy(refname, refs/heads/);
- strcpy(refname + strlen(refs/heads/), ret-name);
- ret-refname = refname;
+ ret-refname = xstrfmt(refs/heads/%s, ret-name);
return ret;
}
diff --git a/unpack-trees.c b/unpack-trees.c
index 97fc995..c237370 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -56,17 +56,15 @@ void setup_unpack_trees_porcelain(struct
unpack_trees_options *opts,
int i;
const char **msgs = opts-msgs;
const char *msg;
- char *tmp;
const char *cmd2 = strcmp(cmd, checkout) ? cmd : switch branches;
+
if (advice_commit_before_merge)
msg = Your local changes to the following files would be
overwritten by %s:\n%%s
Please, commit your changes or stash them before you
can %s.;
else
msg = Your local changes to the following files would be
overwritten by %s:\n%%s;
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);
- sprintf(tmp, msg, cmd, cmd2);
- msgs[ERROR_WOULD_OVERWRITE] = tmp;
- msgs[ERROR_NOT_UPTODATE_FILE] = tmp;
+ msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
+ xstrfmt(msg, cmd, cmd2);
msgs[ERROR_NOT_UPTODATE_DIR] =
Updating the following directories would lose untracked files
in it:\n%s;
@@ -76,12 +74,9 @@ void setup_unpack_trees_porcelain(struct
unpack_trees_options *opts,
Please move or remove them before you can %s.;
else
msg = The following untracked working tree files would be %s
by %s:\n%%s;
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(removed) +
strlen(cmd2) - 4);
- sprintf(tmp, msg, removed, cmd, cmd2);
- msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(overwritten) +
strlen(cmd2) - 4);
- sprintf(tmp, msg, overwritten, cmd, cmd2);
- msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;
+
+ msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrfmt(msg, removed, cmd,
cmd2);
+ msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrfmt(msg,
overwritten, cmd, cmd2);
/*
* Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
--
2.0.0.566.gfe3e6b2
--
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 v2 04/10] use xstrfmt to replace xmalloc + sprintf
This is one line shorter, and makes sure the length in the
malloc and sprintf steps match.
These conversions are very straightforward; we can drop the
malloc entirely, and replace the sprintf with xstrfmt.
Signed-off-by: Jeff King p...@peff.net
---
Just a note on one thing I would look for as a reviewer:
In theory this could introduce a time-of-use error (either we xstrfmt
at the malloc, in which case the arguments to format might not be
ready yet, or we xstrfmt where the old sprintf was, in which case the
pointer is uninitialized earlier). In practice, this is not an issue.
The two are almost always right next to each other. And even when they
are not, the xmalloc already runs strlen() on the arguments, so it
should be safe to do xstrfmt there, too. I.e., if there is a bug, it
was already there. :)
builtin/fmt-merge-msg.c | 7 ++-
builtin/show-branch.c | 10 --
http-push.c | 18 +-
http-walker.c | 3 +--
match-trees.c | 9 ++---
merge-recursive.c | 12
6 files changed, 18 insertions(+), 41 deletions(-)
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3906eda..c462e19 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -178,11 +178,8 @@ static int handle_line(char *line, struct merge_parents
*merge_parents)
int len = strlen(origin);
if (origin[0] == '\'' origin[len - 1] == '\'')
origin = xmemdupz(origin + 1, len - 2);
- } else {
- char *new_origin = xmalloc(strlen(origin) + strlen(src) + 5);
- sprintf(new_origin, %s of %s, origin, src);
- origin = new_origin;
- }
+ } else
+ origin = xstrfmt(%s of %s, origin, src);
if (strcmp(., src))
origin_data-is_local_branch = 0;
string_list_append(origins, origin)-util = origin_data;
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d873172..5fd4e4e 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -755,7 +755,7 @@ int cmd_show_branch(int ac, const char **av, const char
*prefix)
}
for (i = 0; i reflog; i++) {
- char *logmsg, *m;
+ char *logmsg;
const char *msg;
unsigned long timestamp;
int tz;
@@ -770,11 +770,9 @@ int cmd_show_branch(int ac, const char **av, const char
*prefix)
msg = (none);
else
msg++;
- m = xmalloc(strlen(msg) + 200);
- sprintf(m, (%s) %s,
- show_date(timestamp, tz, 1),
- msg);
- reflog_msg[i] = m;
+ reflog_msg[i] = xstrfmt((%s) %s,
+ show_date(timestamp, tz, 1),
+ msg);
free(logmsg);
sprintf(nth_desc, %s@{%d}, *av, base+i);
append_ref(nth_desc, sha1, 1);
diff --git a/http-push.c b/http-push.c
index 95650a0..390f74c 100644
--- a/http-push.c
+++ b/http-push.c
@@ -854,8 +854,7 @@ static struct remote_lock *lock_remote(const char *path,
long timeout)
struct xml_ctx ctx;
char *escaped;
- url = xmalloc(strlen(repo-url) + strlen(path) + 1);
- sprintf(url, %s%s, repo-url, path);
+ url = xstrfmt(%s%s, repo-url, path);
/* Make sure leading directories exist for the remote ref */
ep = strchr(url + strlen(repo-url) + 1, '/');
@@ -1115,7 +1114,7 @@ static void remote_ls(const char *path, int flags,
void (*userFunc)(struct remote_ls_ctx *ls),
void *userData)
{
- char *url = xmalloc(strlen(repo-url) + strlen(path) + 1);
+ char *url = xstrfmt(%s%s, repo-url, path);
struct active_request_slot *slot;
struct slot_results results;
struct strbuf in_buffer = STRBUF_INIT;
@@ -1131,8 +1130,6 @@ static void remote_ls(const char *path, int flags,
ls.userData = userData;
ls.userFunc = userFunc;
- sprintf(url, %s%s, repo-url, path);
-
strbuf_addf(out_buffer.buf, PROPFIND_ALL_REQUEST);
dav_headers = curl_slist_append(dav_headers, Depth: 1);
@@ -1534,10 +1531,9 @@ static void update_remote_info_refs(struct remote_lock
*lock)
static int remote_exists(const char *path)
{
- char *url = xmalloc(strlen(repo-url) + strlen(path) + 1);
+ char *url = xstrfmt(%s%s, repo-url, path);
int ret;
- sprintf(url, %s%s, repo-url, path);
switch (http_get_strbuf(url, NULL, NULL)) {
case HTTP_OK:
@@ -1557,12 +1553,9 @@ static int remote_exists(const char *path)
static void fetch_symref(const
[PATCH v2 05/10] use xstrfmt to replace xmalloc + strcpy/strcat
It's easy to get manual allocation calculations wrong, and
the use of strcpy/strcat raise red flags for people looking
for buffer overflows (though in this case each site was
fine).
It's also shorter to use xstrfmt, and the printf-format
tends to be easier for a reader to see what the final string
will look like.
Signed-off-by: Jeff King p...@peff.net
---
By the way, I think that the tip_name allocation in name_rev leaks
badly, but it's a little tricky to fix (we sometimes hand off ownership
of the variable, and sometimes not). However, this patch does not make
it any worse, and nobody is complaining, so I left it for now.
builtin/apply.c| 4 +---
builtin/fetch.c| 9 ++---
builtin/name-rev.c | 5 +
sha1_name.c| 5 +
shell.c| 6 +-
5 files changed, 6 insertions(+), 23 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 9c5724e..b796910 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1281,9 +1281,7 @@ static int parse_git_header(const char *line, int len,
unsigned int size, struct
*/
patch-def_name = git_header_name(line, len);
if (patch-def_name root) {
- char *s = xmalloc(root_len + strlen(patch-def_name) + 1);
- strcpy(s, root);
- strcpy(s + root_len, patch-def_name);
+ char *s = xstrfmt(%s%s, root, patch-def_name);
free(patch-def_name);
patch-def_name = s;
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..40d989f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1053,16 +1053,11 @@ static int fetch_one(struct remote *remote, int argc,
const char **argv)
refs = xcalloc(argc + 1, sizeof(const char *));
for (i = 0; i argc; i++) {
if (!strcmp(argv[i], tag)) {
- char *ref;
i++;
if (i = argc)
die(_(You need to specify a tag
name.));
- ref = xmalloc(strlen(argv[i]) * 2 + 22);
- strcpy(ref, refs/tags/);
- strcat(ref, argv[i]);
- strcat(ref, :refs/tags/);
- strcat(ref, argv[i]);
- refs[j++] = ref;
+ refs[j++] = xstrfmt(refs/tags/%s:refs/tags/%s,
+ argv[i], argv[i]);
} else
refs[j++] = argv[i];
}
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index c824d4e..3c8f319 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -33,10 +33,7 @@ static void name_rev(struct commit *commit,
return;
if (deref) {
- char *new_name = xmalloc(strlen(tip_name)+3);
- strcpy(new_name, tip_name);
- strcat(new_name, ^0);
- tip_name = new_name;
+ tip_name = xstrfmt(%s^0, tip_name);
if (generation)
die(generation: %d, but deref?, generation);
diff --git a/sha1_name.c b/sha1_name.c
index 2b6322f..5e95690 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1252,10 +1252,7 @@ static void diagnose_invalid_sha1_path(const char
*prefix,
die(Path '%s' exists on disk, but not in '%.*s'.,
filename, object_name_len, object_name);
if (errno == ENOENT || errno == ENOTDIR) {
- char *fullname = xmalloc(strlen(filename)
-+ strlen(prefix) + 1);
- strcpy(fullname, prefix);
- strcat(fullname, filename);
+ char *fullname = xstrfmt(%s%s, prefix, filename);
if (!get_tree_entry(tree_sha1, fullname,
sha1, mode)) {
diff --git a/shell.c b/shell.c
index 5c0d47a..ace62e4 100644
--- a/shell.c
+++ b/shell.c
@@ -46,11 +46,7 @@ static int is_valid_cmd_name(const char *cmd)
static char *make_cmd(const char *prog)
{
- char *prefix = xmalloc((strlen(prog) + strlen(COMMAND_DIR) + 2));
- strcpy(prefix, COMMAND_DIR);
- strcat(prefix, /);
- strcat(prefix, prog);
- return prefix;
+ return xstrfmt(%s/%s, COMMAND_DIR, prog);
}
static void cd_to_homedir(void)
--
2.0.0.566.gfe3e6b2
--
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 v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
This is shorter, harder to get wrong, and more clearly
captures the intent.
Signed-off-by: Jeff King p...@peff.net
---
I wondered if there was a reason to avoid this (because we are in
setup_git_env, which can potentially be called by git_pathdup). But the
git_graft_file initialization below already uses it, and I
double-checked that it is safe once git_dir is set.
environment.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/environment.c b/environment.c
index 4dac5e9..4de7b81 100644
--- a/environment.c
+++ b/environment.c
@@ -135,15 +135,11 @@ static void setup_git_env(void)
gitfile = read_gitfile(git_dir);
git_dir = xstrdup(gitfile ? gitfile : git_dir);
git_object_dir = getenv(DB_ENVIRONMENT);
- if (!git_object_dir) {
- git_object_dir = xmalloc(strlen(git_dir) + 9);
- sprintf(git_object_dir, %s/objects, git_dir);
- }
+ if (!git_object_dir)
+ git_object_dir = git_pathdup(objects);
git_index_file = getenv(INDEX_ENVIRONMENT);
- if (!git_index_file) {
- git_index_file = xmalloc(strlen(git_dir) + 7);
- sprintf(git_index_file, %s/index, git_dir);
- }
+ if (!git_index_file)
+ git_index_file = git_pathdup(index);
git_graft_file = getenv(GRAFT_ENVIRONMENT);
if (!git_graft_file)
git_graft_file = git_pathdup(info/grafts);
--
2.0.0.566.gfe3e6b2
--
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 v2 07/10] sequencer: use argv_array_pushf
This avoids a manual allocation calculation, and is shorter
to boot.
Signed-off-by: Jeff King p...@peff.net
---
sequencer.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..2fea824 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -396,18 +396,13 @@ static int run_git_commit(const char *defmsg, struct
replay_opts *opts,
{
struct argv_array array;
int rc;
- char *gpg_sign;
argv_array_init(array);
argv_array_push(array, commit);
argv_array_push(array, -n);
- if (opts-gpg_sign) {
- gpg_sign = xmalloc(3 + strlen(opts-gpg_sign));
- sprintf(gpg_sign, -S%s, opts-gpg_sign);
- argv_array_push(array, gpg_sign);
- free(gpg_sign);
- }
+ if (opts-gpg_sign)
+ argv_array_pushf(array, -S%s, opts-gpg_sign);
if (opts-signoff)
argv_array_push(array, -s);
if (!opts-edit) {
--
2.0.0.566.gfe3e6b2
--
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 v2 08/10] merge: use argv_array when spawning merge strategy
This is shorter, and avoids a rather complicated set of
allocation and free steps.
Signed-off-by: Jeff King p...@peff.net
---
merge.c | 42 +-
1 file changed, 13 insertions(+), 29 deletions(-)
diff --git a/merge.c b/merge.c
index 70f1000..1fa6e52 100644
--- a/merge.c
+++ b/merge.c
@@ -18,39 +18,23 @@ int try_merge_command(const char *strategy, size_t xopts_nr,
const char **xopts, struct commit_list *common,
const char *head_arg, struct commit_list *remotes)
{
- const char **args;
- int i = 0, x = 0, ret;
+ struct argv_array args = ARGV_ARRAY_INIT;
+ int i, ret;
struct commit_list *j;
- struct strbuf buf = STRBUF_INIT;
- args = xmalloc((4 + xopts_nr + commit_list_count(common) +
- commit_list_count(remotes)) * sizeof(char *));
- strbuf_addf(buf, merge-%s, strategy);
- args[i++] = buf.buf;
- for (x = 0; x xopts_nr; x++) {
- char *s = xmalloc(strlen(xopts[x])+2+1);
- strcpy(s, --);
- strcpy(s+2, xopts[x]);
- args[i++] = s;
- }
- for (j = common; j; j = j-next)
- args[i++] = xstrdup(merge_argument(j-item));
- args[i++] = --;
- args[i++] = head_arg;
- for (j = remotes; j; j = j-next)
- args[i++] = xstrdup(merge_argument(j-item));
- args[i] = NULL;
- ret = run_command_v_opt(args, RUN_GIT_CMD);
- strbuf_release(buf);
- i = 1;
- for (x = 0; x xopts_nr; x++)
- free((void *)args[i++]);
+ argv_array_pushf(args, merge-%s, strategy);
+ for (i = 0; i xopts_nr; i++)
+ argv_array_pushf(args, --%s, xopts[i]);
for (j = common; j; j = j-next)
- free((void *)args[i++]);
- i += 2;
+ argv_array_push(args, merge_argument(j-item));
+ argv_array_push(args, --);
+ argv_array_push(args, head_arg);
for (j = remotes; j; j = j-next)
- free((void *)args[i++]);
- free(args);
+ argv_array_push(args, merge_argument(j-item));
+
+ ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
+ argv_array_clear(args);
+
discard_cache();
if (read_cache() 0)
die(_(failed to read the cache));
--
2.0.0.566.gfe3e6b2
--
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 v2 09/10] walker_fetch: fix minor memory leak
We sometimes allocate msg on the heap, but will fail to
free it if we hit the failure code path. We can instead keep
a separate variable that is safe to be freed no matter how
we get to the failure code path.
While we're here, we can also do two readability
improvements:
1. Use xstrfmt instead of a manual malloc/sprintf
2. Due to the maybe we allocate msg, maybe we don't
strategy, the logic for deciding which message to show
was split into two parts. Since the deallocation is now
pushed onto a separate variable, this is no longer a
concern, and we can keep all of the logic in the same
place.
Signed-off-by: Jeff King p...@peff.net
---
walker.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..0148264 100644
--- a/walker.c
+++ b/walker.c
@@ -253,7 +253,8 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
{
struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
unsigned char *sha1 = xmalloc(targets * 20);
- char *msg;
+ const char *msg;
+ char *to_free = NULL;
int ret;
int i;
@@ -285,21 +286,19 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
if (loop(walker))
goto unlock_and_fail;
- if (write_ref_log_details) {
- msg = xmalloc(strlen(write_ref_log_details) + 12);
- sprintf(msg, fetch from %s, write_ref_log_details);
- } else {
- msg = NULL;
- }
+ if (write_ref_log_details)
+ msg = to_free = xstrfmt(fetch from %s, write_ref_log_details);
+ else
+ msg = fetch (unknown);
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));
+ ret = write_ref_sha1(lock[i], sha1[20 * i], msg);
lock[i] = NULL;
if (ret)
goto unlock_and_fail;
}
- free(msg);
+ free(to_free);
return 0;
@@ -307,6 +306,7 @@ unlock_and_fail:
for (i = 0; i targets; i++)
if (lock[i])
unlock_ref(lock[i]);
+ free(to_free);
return -1;
}
--
2.0.0.566.gfe3e6b2
--
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 v2 10/10] unique_path: fix unlikely heap overflow
When merge-recursive creates a unique filename, it uses a
template like:
path~branch_%d
where the final _%d is filled by an incrementing counter
until we find a unique name. We allocate 8 characters for
the counter, but there is no logic to limit the size of the
integer.
Of course, this is extremely unlikely, as you would need a
hundred million collisions to trigger the problem. Even if
an attacker constructed a specialized repo, it is unlikely
that the victim would have the patience to run the merge.
However, we can make it trivially correct (and hopefully
more readable) by using a strbuf.
Signed-off-by: Jeff King p...@peff.net
---
merge-recursive.c | 41 ++---
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 532a1da..398a734 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -601,25 +601,36 @@ static int remove_file(struct merge_options *o, int clean,
return 0;
}
+/* add a string to a strbuf, but converting / to _ */
+static void add_flattened_path(struct strbuf *out, const char *s)
+{
+ size_t i = out-len;
+ strbuf_addstr(out, s);
+ for (; i out-len; i++)
+ if (out-buf[i] == '/')
+ out-buf[i] = '_';
+}
+
static char *unique_path(struct merge_options *o, const char *path, const char
*branch)
{
- char *newpath = xmalloc(strlen(path) + 1 + strlen(branch) + 8 + 1);
+ struct strbuf newpath = STRBUF_INIT;
int suffix = 0;
struct stat st;
- char *p = newpath + strlen(path);
- strcpy(newpath, path);
- *(p++) = '~';
- strcpy(p, branch);
- for (; *p; ++p)
- if ('/' == *p)
- *p = '_';
- while (string_list_has_string(o-current_file_set, newpath) ||
- string_list_has_string(o-current_directory_set, newpath) ||
- lstat(newpath, st) == 0)
- sprintf(p, _%d, suffix++);
-
- string_list_insert(o-current_file_set, newpath);
- return newpath;
+ size_t base_len;
+
+ strbuf_addf(newpath, %s~, path);
+ add_flattened_path(newpath, branch);
+
+ base_len = newpath.len;
+ while (string_list_has_string(o-current_file_set, newpath.buf) ||
+ string_list_has_string(o-current_directory_set, newpath.buf) ||
+ lstat(newpath.buf, st) == 0) {
+ strbuf_setlen(newpath, base_len);
+ strbuf_addf(newpath, _%d, suffix++);
+ }
+
+ string_list_insert(o-current_file_set, newpath.buf);
+ return strbuf_detach(newpath, NULL);
}
static int dir_in_way(const char *path, int check_working_copy)
--
2.0.0.566.gfe3e6b2
--
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
Should branches be objects?
[I'm a list newbie here, but a git power user.] If branches were objects... - one could see the history of branches, including - how commits were grouped when pushed/pulled (push 5 commits, and the branch object will record that its head moved by those five commits at once) - rebase history (git log branch-object - better than git reflog!) - object transactional APIs would be used to update branches Branch objects might be purely local, recording what was done in a local repo to a branch, but they might be pullable, to make branch history viewable in clones. Just a thought, Nico -- -- 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: Surprising 'git-describe --all --match' behavior.
Sergei Organov o...@javad.com writes: Junio C Hamano gits...@pobox.com writes: Sergei Organov o...@javad.com writes: Will something break if it won't helpfully prepend refs/tags/ once --all is given? describe --all --match 'v*' will no longer match a tag v1.2.3, and forces the users to say describe --match 'refs/tags/v*', No, descirbe --match 'v*' or describe --tags --match 'v*' depending on what they actually meant. Notice my once --all is given above. ... Those who used --all meant to match against all the refs, no? I noticed it when I responded and ignored it as unworkable, because it would make the interface inconsistent by making the meaning of one option (i.e. --match) change depending on an unrelated option (i.e. --all or --tags). You can argue both ways: Those who read the doc and used --match did mean to limit to tags. The thing is, you cannot change it without risking to break existing usage. That does not necessarily mean you can never change anything. You only need to craft a careful transition plan to minimize the pain for those who will be broken, and the end result will be good if the pain is small enough and the benefit is large enough ;) -- 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/16] http-push: refactor parsing of remote object names
We get loose object names like objects/??/... from the
remote side, and need to convert them to their hex
representation.
The code to do so is rather hard to follow, as it uses some
calculated lengths whose origins are hard to understand and
verify (e.g., the path must be exactly 49 characters long.
why? Why doesn't the strcpy overflow obj_hex, which is the
same length as path?).
We can simplify this a bit by using skip_prefix, using standard
40- and 20-character buffers for hex and binary sha1s, and
adding some comments.
We also drop a totally bogus comment that claims strlcpy
cannot be used because path is not NUL-terminated. Right
between a call to strlen(path) and strcpy(path).
Signed-off-by: Jeff King p...@peff.net
---
I found this one while doing the xstrfmt series, but it actually doesn't
need xstrfmt, and does need skip_prefix, so it probably goes better on
the skip-prefix topic.
It's still a little more magical than I would like, but I think this is
the best we can do while still building on get_sha1_hex. Parsing it
left-to-right would be better, but we would essentially end up
reimplementing get_sha1_hex.
http-push.c | 38 +++---
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/http-push.c b/http-push.c
index 26dfa67..c5c95e8 100644
--- a/http-push.c
+++ b/http-push.c
@@ -719,14 +719,10 @@ static int fetch_indices(void)
return ret;
}
-static void one_remote_object(const char *hex)
+static void one_remote_object(const unsigned char *sha1)
{
- unsigned char sha1[20];
struct object *obj;
- if (get_sha1_hex(hex, sha1) != 0)
- return;
-
obj = lookup_object(sha1);
if (!obj)
obj = parse_object(sha1);
@@ -1020,26 +1016,38 @@ static void remote_ls(const char *path, int flags,
void (*userFunc)(struct remote_ls_ctx *ls),
void *userData);
+/* extract hex from sharded xx/x{40} filename */
+static int get_sha1_hex_from_objpath(const char *path, unsigned char *sha1)
+{
+ char hex[40];
+
+ if (strlen(path) != 41)
+ return -1;
+
+ memcpy(hex, path, 2);
+ path += 2;
+ path++; /* skip '/' */
+ memcpy(hex, path, 38);
+
+ return get_sha1_hex(hex, sha1);
+}
+
static void process_ls_object(struct remote_ls_ctx *ls)
{
unsigned int *parent = (unsigned int *)ls-userData;
- char *path = ls-dentry_name;
- char *obj_hex;
+ const char *path = ls-dentry_name;
+ unsigned char sha1[20];
if (!strcmp(ls-path, ls-dentry_name) (ls-flags IS_DIR)) {
remote_dir_exists[*parent] = 1;
return;
}
- if (strlen(path) != 49)
+ if (!skip_prefix(path, objects/, path) ||
+ get_sha1_hex_from_objpath(path, sha1))
return;
- path += 8;
- obj_hex = xmalloc(strlen(path));
- /* NB: path is not null-terminated, can not use strlcpy here */
- memcpy(obj_hex, path, 2);
- strcpy(obj_hex + 2, path + 3);
- one_remote_object(obj_hex);
- free(obj_hex);
+
+ one_remote_object(sha1);
}
static void process_ls_ref(struct remote_ls_ctx *ls)
--
2.0.0.566.gfe3e6b2
--
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 17/16] http-push: refactor parsing of remote object names
On Thu, Jun 19, 2014 at 05:58:10PM -0400, Jeff King wrote:
It's still a little more magical than I would like, but I think this is
the best we can do while still building on get_sha1_hex. Parsing it
left-to-right would be better, but we would essentially end up
reimplementing get_sha1_hex.
Just for fun, that would look something like this (on top):
diff --git a/http-push.c b/http-push.c
index c5c95e8..2425c61 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1016,20 +1016,34 @@ static void remote_ls(const char *path, int flags,
void (*userFunc)(struct remote_ls_ctx *ls),
void *userData);
+static inline int parse_one_hex(unsigned char **to, const char **from)
+{
+ unsigned int val;
+
+ /* avoid reading past end-of-string for from[1] */
+ if (!(*from)[0])
+ return -1;
+ val = (hexval((*from)[0]) 4) | hexval((*from)[1]);
+ if (val ~0xff)
+ return -1;
+ *(*to)++ = val;
+ (*from) += 2;
+ return 0;
+}
+
/* extract hex from sharded xx/x{40} filename */
static int get_sha1_hex_from_objpath(const char *path, unsigned char *sha1)
{
- char hex[40];
+ int i;
- if (strlen(path) != 41)
+ if (parse_one_hex(sha1, path) 0)
return -1;
-
- memcpy(hex, path, 2);
- path += 2;
- path++; /* skip '/' */
- memcpy(hex, path, 38);
-
- return get_sha1_hex(hex, sha1);
+ if (*path++ != '/')
+ return -1;
+ for (i = 1; i 20; i++)
+ if (parse_one_hex(sha1, path) 0)
+ return -1;
+ return 0;
}
static void process_ls_object(struct remote_ls_ctx *ls)
In theory we could factor parse_one_hex to share with get_sha1_hex, but
I am hesitant to touch get_sha1_hex, as it is used in a lot of tight
loops.
-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
