[PATCH] Fix minor typo in hook documentation
Signed-off-by: Martin Mosegaard Amdisen --- templates/hooks--update.sample | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/hooks--update.sample b/templates/hooks--update.sample index d847583..80ba941 100755 --- a/templates/hooks--update.sample +++ b/templates/hooks--update.sample @@ -1,6 +1,6 @@ #!/bin/sh # -# An example hook script to blocks unannotated tags from entering. +# An example hook script to block unannotated tags from entering. # Called by "git receive-pack" with arguments: refname sha1-old sha1-new # # To enable this hook, rename this file to "update". -- 2.6.4 -- To unsubscribe from this list: send the line "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 4/5] README.md: don't call git stupid in the title
"the stupid content tracker" was true in the early days of Git, but hardly applicable these days. "fast, scalable, distributed" describes Git more accuralety. Also, "stupid" can be seen as offensive by some people. Let's not use it in the very first words of the README. The new formulation is taken from the description of the Debian package. Signed-off-by: Matthieu Moy --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 5500f8a..e3ddc3e 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,5 @@ -Git - the stupid content tracker - +Git - fast, scalable, distributed revision control system += "git" can mean anything, depending on your mood. -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/5] README: use markdown syntax
This allows repository browsers like GitHub to display the content of the file nicely formatted. Signed-off-by: Matthieu Moy --- README => README.md | 7 ++- t/t7001-mv.sh | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) rename README => README.md (93%) diff --git a/README b/README.md similarity index 93% rename from README rename to README.md index 1083735..600779c 100644 --- a/README +++ b/README.md @@ -1,8 +1,5 @@ - - - Git - the stupid content tracker - - +Git - the stupid content tracker + "git" can mean anything, depending on your mood. diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 51dd2b4..4008fae 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -102,7 +102,7 @@ test_expect_success \ test_expect_success \ 'adding another file' \ -'cp "$TEST_DIRECTORY"/../README path0/README && +'cp "$TEST_DIRECTORY"/../README.md path0/README && git add path0/README && git commit -m add2 -a' -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/5] Make README more pleasant to read
Minor tweaks after discussion on v1 (for those who missed it, this
series makes README render nicely on GitHub and tries to present
important information early).
The result is here:
https://github.com/moy/git/tree/git-readme#readme
Changes since v1:
* Visible on the rendered page: resurect "the stupid content tracker"
at the bottom ("He described the tool as "the stupid content
tracker" and the name as (depending on your mood)") as suggested by
Junio. I first disagreed, but that's part of the explanation why Git
is called Git, so why not.
* Visible only in the source: change
# title
to
title
=
(I chose the first because it was more easy to type, but for someone
not familiar with markdown, the second makes it more obvious that
its' a title)
I kept the patch introducing explicit links on filenames. I do not
care deeply about it.
Matthieu Moy (5):
README: use markdown syntax
README.md: add hyperlinks on filenames
README.md: move the link to git-scm.com up
README.md: don't call git stupid in the title
README.md: move down historical explanation about the name
README => README.md | 56 +
t/t7001-mv.sh | 2 +-
2 files changed, 32 insertions(+), 26 deletions(-)
rename README => README.md (65%)
--
2.7.2.334.g35ed2ae.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] README.md: move the link to git-scm.com up
The documentation available on git-scm.com is nicely formatted. It's
better to point users to it than to the source code of the
documentation.
Signed-off-by: Matthieu Moy
---
README.md | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/README.md b/README.md
index e54f9e7..5500f8a 100644
--- a/README.md
+++ b/README.md
@@ -23,6 +23,9 @@ Torvalds with help of a group of hackers around the net.
Please read the file [INSTALL][] for installation instructions.
+Many Git online resources are accessible from http://git-scm.com/
+including full documentation and Git related tools.
+
See [Documentation/gittutorial.txt][] to get started, then see
[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
[Documentation/git-commandname.txt][] for documentation of each command.
@@ -35,9 +38,6 @@ CVS users may also want to read
[Documentation/gitcvs-migration.txt][]
("man gitcvs-migration" or "git help cvs-migration" if git is
installed).
-Many Git online resources are accessible from http://git-scm.com/
-including full documentation and Git related tools.
-
The user discussion and development of Git take place on the Git
mailing list -- everyone is welcome to post bug reports, feature
requests, comments and patches to git@vger.kernel.org (read
--
2.7.2.334.g35ed2ae.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] README.md: move down historical explanation about the name
The explanations about why the name was chosen are secondary compared to the description and link to the documentation. Some consider these explanations as good computer scientists joke, but other see it as needlessly offensive vocabulary. This patch preserves the historical joke, but gives it less importance by moving it to the end of the README, and makes it clear that it is a historical explanation, that does not necessarily reflect the state of mind of current developers. Signed-off-by: Matthieu Moy --- README.md | 24 +--- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index e3ddc3e..d1ffbb6 100644 --- a/README.md +++ b/README.md @@ -1,17 +1,6 @@ Git - fast, scalable, distributed revision control system = -"git" can mean anything, depending on your mood. - - - random three-letter combination that is pronounceable, and not - actually used by any common UNIX command. The fact that it is a - mispronunciation of "get" may or may not be relevant. - - stupid. contemptible and despicable. simple. Take your pick from the - dictionary of slang. - - "global information tracker": you're in a good mood, and it actually - works for you. Angels sing, and a light suddenly fills the room. - - "goddamn idiotic truckload of sh*t": when it breaks - Git is a fast, scalable, distributed revision control system with an unusually rich command set that provides both high-level operations and full access to internals. @@ -52,6 +41,19 @@ list the current status of various development topics to the mailing list. The discussion following them give a good reference for project status, development direction and remaining tasks. +The name "git" was given by Linus Torvalds when he wrote the very +first version. He described the tool as "the stupid content tracker" +and the name as (depending on your mood): + + - random three-letter combination that is pronounceable, and not + actually used by any common UNIX command. The fact that it is a + mispronunciation of "get" may or may not be relevant. + - stupid. contemptible and despicable. simple. Take your pick from the + dictionary of slang. + - "global information tracker": you're in a good mood, and it actually + works for you. Angels sing, and a light suddenly fills the room. + - "goddamn idiotic truckload of sh*t": when it breaks + [INSTALL]: INSTALL [Documentation/gittutorial.txt]: Documentation/gittutorial.txt [Documentation/giteveryday.txt]: Documentation/giteveryday.txt -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] README.md: add hyperlinks on filenames
Signed-off-by: Matthieu Moy
---
README.md | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/README.md b/README.md
index 600779c..e54f9e7 100644
--- a/README.md
+++ b/README.md
@@ -21,17 +21,17 @@ License version 2 (some parts of it are under different
licenses,
compatible with the GPLv2). It was originally written by Linus
Torvalds with help of a group of hackers around the net.
-Please read the file INSTALL for installation instructions.
+Please read the file [INSTALL][] for installation instructions.
-See Documentation/gittutorial.txt to get started, then see
-Documentation/giteveryday.txt for a useful minimum set of commands, and
-Documentation/git-commandname.txt for documentation of each command.
+See [Documentation/gittutorial.txt][] to get started, then see
+[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
+[Documentation/git-commandname.txt][] for documentation of each command.
If git has been correctly installed, then the tutorial can also be
read with "man gittutorial" or "git help tutorial", and the
documentation of each command with "man git-commandname" or "git help
commandname".
-CVS users may also want to read Documentation/gitcvs-migration.txt
+CVS users may also want to read [Documentation/gitcvs-migration.txt][]
("man gitcvs-migration" or "git help cvs-migration" if git is
installed).
@@ -41,7 +41,7 @@ including full documentation and Git related tools.
The user discussion and development of Git take place on the Git
mailing list -- everyone is welcome to post bug reports, feature
requests, comments and patches to git@vger.kernel.org (read
-Documentation/SubmittingPatches for instructions on patch submission).
+[Documentation/SubmittingPatches][] for instructions on patch submission).
To subscribe to the list, send an email with just "subscribe git" in
the body to majord...@vger.kernel.org. The mailing list archives are
available at http://news.gmane.org/gmane.comp.version-control.git/,
@@ -51,3 +51,10 @@ The maintainer frequently sends the "What's cooking" reports
that
list the current status of various development topics to the mailing
list. The discussion following them give a good reference for
project status, development direction and remaining tasks.
+
+[INSTALL]: INSTALL
+[Documentation/gittutorial.txt]: Documentation/gittutorial.txt
+[Documentation/giteveryday.txt]: Documentation/giteveryday.txt
+[Documentation/git-commandname.txt]: Documentation/git-commandname.txt
+[Documentation/gitcvs-migration.txt]: Documentation/gitcvs-migration.txt
+[Documentation/SubmittingPatches]: Documentation/SubmittingPatches
--
2.7.2.334.g35ed2ae.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings
From: Lars Schneider We assume Git developers have a reasonably modern compiler and recommend them to enable the DEVELOPER makefile knob to ensure their patches are clear of all compiler warnings the Git core project cares about. Enable the DEVELOPER makefile knob in the Travis-CI build. Suggested-by: Jeff King Signed-off-by: Lars Schneider --- This patch is the successor of "[PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement" [1] which enables compiler warnings for the Travis-CI build. Peff suggested to codify the knowledge about the compiler warnings the Git project cares about [2] which I have done here. The only problem is the "-Wold-style-declaration" compiler warning as this is only supported by GCC and not Clang. Should we ignore that warning or would you prefer to detect the GCC compiler and add the warning? The Linux kernel project does a similar thing [3]. Thanks, Lars [1] http://thread.gmane.org/gmane.comp.version-control.git/285752 [2] http://article.gmane.org/gmane.comp.version-control.git/285761 [3] https://github.com/torvalds/linux/blob/6dc390ad61ac8dfca5fa9b0823981fb6f7ec17a0/Makefile#L303-L306 .travis.yml| 2 +- Documentation/CodingGuidelines | 4 Makefile | 12 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index c3bf9c6..168ae21 100644 --- a/.travis.yml +++ b/.travis.yml @@ -15,12 +15,12 @@ addons: env: global: +- DEVELOPER=1 - P4_VERSION="15.2" - GIT_LFS_VERSION="1.1.0" - DEFAULT_TEST_TARGET=prove - GIT_PROVE_OPTS="--timer --jobs 3" - GIT_TEST_OPTS="--verbose --tee" -- CFLAGS="-g -O2 -Wall -Werror" - GIT_TEST_CLONE_2GB=YesPlease # t9810 occasionally fails on Travis CI OS X # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index c6e536f..1c676c2 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -171,6 +171,10 @@ For C programs: - We try to keep to at most 80 characters per line. + - As a Git developer we assume you have a reasonably modern compiler + and we recommend you to enable the DEVELOPER makefile knob to + ensure your patch is clear of all compiler warnings we care about. + - We try to support a wide range of C compilers to compile Git with, including old ones. That means that you should not use C99 initializers, even if a lot of compilers grok it. diff --git a/Makefile b/Makefile index fd19b54..9d4614d 100644 --- a/Makefile +++ b/Makefile @@ -380,6 +380,18 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip +ifdef DEVELOPER + CFLAGS += -Werror \ + -Wdeclaration-after-statement \ + -Wno-format-zero-length \ + -Wold-style-definition \ + -Woverflow \ + -Wpointer-arith \ + -Wstrict-prototypes \ + -Wunused \ + -Wvla +endif + # Create as necessary, replace existing, make ranlib unneeded. ARFLAGS = rcs -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] diff: activate diff.renames by default
Jeff King writes: > On Tue, Feb 23, 2016 at 06:44:58PM +0100, Matthieu Moy wrote: > >> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt >> index 1acd203..fdf5a79 100644 >> --- a/Documentation/diff-config.txt >> +++ b/Documentation/diff-config.txt >> @@ -111,7 +111,7 @@ diff.renames:: >> Whether and how Git detects renames. If set to "false", >> rename detection is disabled. If set to "true", basic rename >> detection is enable. If set to "copies" or "copy", Git will >> -detect copies, as well. Defaults to false. >> +detect copies, as well. Defaults to true. > > I wonder if we need to talk about plumbing versus porcelain here, as it > does not default to true for diff-tree, for example. But I guess that is > already the case (even setting it to true yourself does not affect > diff-tree). Yes, it was already the case. But it doesn't harm to document it while we're there. I've added this in v2: Note that this affects only 'git diff' Porcelain like linkgit:git-diff[1] and linkgit:git-log[1], and not lower level commands such as linkgit:git-diff-files[1]. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/5] activate diff.renames by default
Remarks on v1 applied:
* document that diff.renames applies only to porcelain
* use <<- instead of << in tests
* rename git_diff_ui_default_config to init_diff_ui_defaults
* f() -> f(void)
Interdiff follows:
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index fdf5a79..69389ae 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -111,7 +111,10 @@ diff.renames::
Whether and how Git detects renames. If set to "false",
rename detection is disabled. If set to "true", basic rename
detection is enable. If set to "copies" or "copy", Git will
- detect copies, as well. Defaults to true.
+ detect copies, as well. Defaults to true. Note that this
+ affects only 'git diff' Porcelain like linkgit:git-diff[1] and
+ linkgit:git-log[1], and not lower level commands such as
+ linkgit:git-diff-files[1].
diff.suppressBlankEmpty::
A boolean to inhibit the standard behavior of printing a space
diff --git a/builtin/commit.c b/builtin/commit.c
index 3cb4843..109742e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -186,7 +186,7 @@ static void status_init_config(struct wt_status *s,
config_fn_t fn)
gitmodules_config();
git_config(fn, s);
determine_whence(s);
- git_diff_ui_default_config();
+ init_diff_ui_defaults();
s->hints = advice_status_hints; /* must come after git_config() */
}
diff --git a/builtin/diff.c b/builtin/diff.c
index 8bd1fd5..343c6b8 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -318,7 +318,7 @@ int cmd_diff(int argc, const char **argv, const char
*prefix)
if (!no_index)
gitmodules_config();
- git_diff_ui_default_config();
+ init_diff_ui_defaults();
git_config(git_diff_ui_config, NULL);
init_revisions(&rev, prefix);
diff --git a/builtin/log.c b/builtin/log.c
index 6e34df3..c05a5f6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -103,7 +103,7 @@ static int log_line_range_callback(const struct option
*option, const char *arg,
static void init_log_defaults()
{
init_grep_defaults();
- git_diff_ui_default_config();
+ init_diff_ui_defaults();
}
static void cmd_log_init_defaults(struct rev_info *rev)
diff --git a/builtin/merge.c b/builtin/merge.c
index cf297d4..4cb4f6a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1187,7 +1187,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
else
head_commit = lookup_commit_or_die(head_sha1, "HEAD");
- git_diff_ui_default_config();
+ init_diff_ui_defaults();
git_config(git_merge_config, NULL);
if (branch_mergeoptions)
diff --git a/diff.c b/diff.c
index d5db898..b4dea07 100644
--- a/diff.c
+++ b/diff.c
@@ -168,7 +168,7 @@ long parse_algorithm_value(const char *value)
* never be affected by the setting of diff.renames
* the user happens to have in the configuration file.
*/
-void git_diff_ui_default_config()
+void init_diff_ui_defaults(void)
{
diff_detect_rename_default = 1;
}
diff --git a/diff.h b/diff.h
index 75686d5..0a3ce86 100644
--- a/diff.h
+++ b/diff.h
@@ -266,7 +266,7 @@ extern int parse_long_opt(const char *opt, const char
**argv,
const char **optarg);
extern int git_diff_basic_config(const char *var, const char *value, void *cb);
-extern void git_diff_ui_default_config();
+extern void init_diff_ui_defaults(void);
extern int git_diff_ui_config(const char *var, const char *value, void *cb);
extern void diff_setup(struct diff_options *);
extern int diff_opt_parse(struct diff_options *, const char **, int, const
char *);
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 15d99a3..c7e58b6 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -10,83 +10,83 @@ test_description='Test rename detection in diff engine.
. "$TEST_DIRECTORY"/diff-lib.sh
test_expect_success 'setup' '
- cat >path0 <<\EOF &&
-Line 1
-Line 2
-Line 3
-Line 4
-Line 5
-Line 6
-Line 7
-Line 8
-Line 9
-Line 10
-line 11
-Line 12
-Line 13
-Line 14
-Line 15
-EOF
- cat >expected <<\EOF &&
-diff --git a/path0 b/path1
-rename from path0
-rename to path1
a/path0
-+++ b/path1
-@@ -8,7 +8,7 @@ Line 7
- Line 8
- Line 9
- Line 10
--line 11
-+Line 11
- Line 12
- Line 13
- Line 14
-EOF
- cat >no-rename <<\EOF
-diff --git a/path0 b/path0
-deleted file mode 100644
-index fdbec44..000
a/path0
-+++ /dev/null
-@@ -1,15 +0,0 @@
--Line 1
--Line 2
--Line 3
--Line 4
--Line 5
--Line 6
--Line 7
--Line 8
--Line 9
--Line 10
--line 11
--Line 12
--Line 13
--Line 14
--Line 15
-diff --git a/path1 b/path1
-new file mode 100644
-index 000..752c50e
/dev/null
-+++ b/path1
-@@ -0,0 +1,15 @@
-+Line 1
-+Line 2
-+Line 3
-+Line 4
-+Line 5
-+Line 6
-+Line 7
-+Line 8
-+Line 9
-+Line 10
-+Line 11
-+Line 12
-+Line 13
-+Line 14
-+Line 15
-EOF
+ cat >path0 <<-\EOF &&
+
[PATCH v2 1/5] Documentation/diff-config: fix description of diff.renames
The description was misleading, since "set to any boolean value" include "set to false", and diff.renames=false does not enable basic detection, but actually disables it. Also, document that diff.renames only affects Porcelain. Signed-off-by: Matthieu Moy --- Documentation/diff-config.txt | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index 6eaa452..40e5de9 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -108,9 +108,13 @@ diff.renameLimit:: detection; equivalent to the 'git diff' option '-l'. diff.renames:: - Tells Git to detect renames. If set to any boolean value, it - will enable basic rename detection. If set to "copies" or - "copy", it will detect copies, as well. + Whether and how Git detects renames. If set to "false", + rename detection is disabled. If set to "true", basic rename + detection is enable. If set to "copies" or "copy", Git will + detect copies, as well. Defaults to false. Note that this + affects only 'git diff' Porcelain like linkgit:git-diff[1] and + linkgit:git-log[1], and not lower level commands such as + linkgit:git-diff-files[1]. diff.suppressBlankEmpty:: A boolean to inhibit the standard behavior of printing a space -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] t: add tests for diff.renames (true/false/unset)
The underlying machinery is well-tested, but the configuration option itself was tested only in t3400-rebase.sh. Signed-off-by: Matthieu Moy --- t/t4001-diff-rename.sh | 61 +- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh index 06b8828..f5239b5 100755 --- a/t/t4001-diff-rename.sh +++ b/t/t4001-diff-rename.sh @@ -27,7 +27,7 @@ test_expect_success 'setup' ' Line 14 Line 15 EOF - cat >expected <<-\EOF + cat >expected <<-\EOF && diff --git a/path0 b/path1 rename from path0 rename to path1 @@ -43,6 +43,50 @@ test_expect_success 'setup' ' Line 13 Line 14 EOF + cat >no-rename <<-\EOF + diff --git a/path0 b/path0 + deleted file mode 100644 + index fdbec44..000 + --- a/path0 + +++ /dev/null + @@ -1,15 +0,0 @@ + -Line 1 + -Line 2 + -Line 3 + -Line 4 + -Line 5 + -Line 6 + -Line 7 + -Line 8 + -Line 9 + -Line 10 + -line 11 + -Line 12 + -Line 13 + -Line 14 + -Line 15 + diff --git a/path1 b/path1 + new file mode 100644 + index 000..752c50e + --- /dev/null + +++ b/path1 + @@ -0,0 +1,15 @@ + +Line 1 + +Line 2 + +Line 3 + +Line 4 + +Line 5 + +Line 6 + +Line 7 + +Line 8 + +Line 9 + +Line 10 + +Line 11 + +Line 12 + +Line 13 + +Line 14 + +Line 15 + EOF ' test_expect_success \ @@ -68,6 +112,21 @@ test_expect_success \ 'validate the output.' \ 'compare_diff_patch current expected' +test_expect_success 'test diff.renames=true' ' + git -c diff.renames=true diff --cached $tree >current && + compare_diff_patch current expected +' + +test_expect_success 'test diff.renames=false' ' + git -c diff.renames=false diff --cached $tree >current && + compare_diff_patch current no-rename +' + +test_expect_success 'test diff.renames unset' ' + git diff --cached $tree >current && + compare_diff_patch current no-rename +' + test_expect_success 'favour same basenames over different ones' ' cp path1 another-path && git add another-path && -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] t4001-diff-rename: wrap file creations in a test
Signed-off-by: Matthieu Moy --- t/t4001-diff-rename.sh | 66 ++ 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh index 2f327b7..06b8828 100755 --- a/t/t4001-diff-rename.sh +++ b/t/t4001-diff-rename.sh @@ -9,21 +9,40 @@ test_description='Test rename detection in diff engine. . ./test-lib.sh . "$TEST_DIRECTORY"/diff-lib.sh -echo >path0 'Line 1 -Line 2 -Line 3 -Line 4 -Line 5 -Line 6 -Line 7 -Line 8 -Line 9 -Line 10 -line 11 -Line 12 -Line 13 -Line 14 -Line 15 +test_expect_success 'setup' ' + cat >path0 <<-\EOF && + Line 1 + Line 2 + Line 3 + Line 4 + Line 5 + Line 6 + Line 7 + Line 8 + Line 9 + Line 10 + line 11 + Line 12 + Line 13 + Line 14 + Line 15 + EOF + cat >expected <<-\EOF + diff --git a/path0 b/path1 + rename from path0 + rename to path1 + --- a/path0 + +++ b/path1 + @@ -8,7 +8,7 @@ Line 7 +Line 8 +Line 9 +Line 10 + -line 11 + +Line 11 +Line 12 +Line 13 +Line 14 + EOF ' test_expect_success \ @@ -43,22 +62,7 @@ test_expect_success \ test_expect_success \ 'git diff-index -p -M after rename and editing.' \ 'git diff-index -p -M $tree >current' -cat >expected <<\EOF -diff --git a/path0 b/path1 -rename from path0 -rename to path1 a/path0 -+++ b/path1 -@@ -8,7 +8,7 @@ Line 7 - Line 8 - Line 9 - Line 10 --line 11 -+Line 11 - Line 12 - Line 13 - Line 14 -EOF + test_expect_success \ 'validate the output.' \ -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/5] log: introduce init_log_defaults()
This is currently a wrapper around init_grep_defaults(), but will allow
adding more initialization in further patches.
Signed-off-by: Matthieu Moy
---
builtin/log.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 0d738d6..7f96c64 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -100,6 +100,11 @@ static int log_line_range_callback(const struct option
*option, const char *arg,
return 0;
}
+static void init_log_defaults()
+{
+ init_grep_defaults();
+}
+
static void cmd_log_init_defaults(struct rev_info *rev)
{
if (fmt_pretty)
@@ -416,7 +421,7 @@ int cmd_whatchanged(int argc, const char **argv, const char
*prefix)
struct rev_info rev;
struct setup_revision_opt opt;
- init_grep_defaults();
+ init_log_defaults();
git_config(git_log_config, NULL);
init_revisions(&rev, prefix);
@@ -527,7 +532,7 @@ int cmd_show(int argc, const char **argv, const char
*prefix)
struct pathspec match_all;
int i, count, ret = 0;
- init_grep_defaults();
+ init_log_defaults();
git_config(git_log_config, NULL);
memset(&match_all, 0, sizeof(match_all));
@@ -608,7 +613,7 @@ int cmd_log_reflog(int argc, const char **argv, const char
*prefix)
struct rev_info rev;
struct setup_revision_opt opt;
- init_grep_defaults();
+ init_log_defaults();
git_config(git_log_config, NULL);
init_revisions(&rev, prefix);
@@ -647,7 +652,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
struct rev_info rev;
struct setup_revision_opt opt;
- init_grep_defaults();
+ init_log_defaults();
git_config(git_log_config, NULL);
init_revisions(&rev, prefix);
@@ -1280,7 +1285,7 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
extra_hdr.strdup_strings = 1;
extra_to.strdup_strings = 1;
extra_cc.strdup_strings = 1;
- init_grep_defaults();
+ init_log_defaults();
git_config(git_format_config, NULL);
init_revisions(&rev, prefix);
rev.commit_format = CMIT_FMT_EMAIL;
--
2.7.2.334.g35ed2ae.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] diff: activate diff.renames by default
Rename detection is a very convenient feature, and new users shouldn't
have to dig in the documentation to benefit from it.
Potential objections to activating rename detection are that it
sometimes fail, and it is sometimes slow. But rename detection is
already activated by default in several cases like "git status" and "git
merge", so activating diff.renames does not fundamentally change the
situation. When the rename detection fails, it now fails consistently
between "git diff" and "git status".
This setting does not affect plumbing commands, hence well-written
scripts will not be affected.
Signed-off-by: Matthieu Moy
---
Documentation/diff-config.txt | 2 +-
builtin/commit.c | 1 +
builtin/diff.c| 1 +
builtin/log.c | 1 +
builtin/merge.c | 1 +
diff.c| 5 +
diff.h| 1 +
t/t4001-diff-rename.sh| 2 +-
t/t4013-diff-various.sh | 2 ++
t/t4014-format-patch.sh | 4 ++--
t/t4047-diff-dirstat.sh | 3 ++-
t/t4202-log.sh| 8
12 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 40e5de9..69389ae 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -111,7 +111,7 @@ diff.renames::
Whether and how Git detects renames. If set to "false",
rename detection is disabled. If set to "true", basic rename
detection is enable. If set to "copies" or "copy", Git will
- detect copies, as well. Defaults to false. Note that this
+ detect copies, as well. Defaults to true. Note that this
affects only 'git diff' Porcelain like linkgit:git-diff[1] and
linkgit:git-log[1], and not lower level commands such as
linkgit:git-diff-files[1].
diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..109742e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -186,6 +186,7 @@ static void status_init_config(struct wt_status *s,
config_fn_t fn)
gitmodules_config();
git_config(fn, s);
determine_whence(s);
+ init_diff_ui_defaults();
s->hints = advice_status_hints; /* must come after git_config() */
}
diff --git a/builtin/diff.c b/builtin/diff.c
index 52c98a9..343c6b8 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -318,6 +318,7 @@ int cmd_diff(int argc, const char **argv, const char
*prefix)
if (!no_index)
gitmodules_config();
+ init_diff_ui_defaults();
git_config(git_diff_ui_config, NULL);
init_revisions(&rev, prefix);
diff --git a/builtin/log.c b/builtin/log.c
index 7f96c64..c05a5f6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -103,6 +103,7 @@ static int log_line_range_callback(const struct option
*option, const char *arg,
static void init_log_defaults()
{
init_grep_defaults();
+ init_diff_ui_defaults();
}
static void cmd_log_init_defaults(struct rev_info *rev)
diff --git a/builtin/merge.c b/builtin/merge.c
index b98a348..4cb4f6a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1187,6 +1187,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
else
head_commit = lookup_commit_or_die(head_sha1, "HEAD");
+ init_diff_ui_defaults();
git_config(git_merge_config, NULL);
if (branch_mergeoptions)
diff --git a/diff.c b/diff.c
index 2136b69..b4dea07 100644
--- a/diff.c
+++ b/diff.c
@@ -168,6 +168,11 @@ long parse_algorithm_value(const char *value)
* never be affected by the setting of diff.renames
* the user happens to have in the configuration file.
*/
+void init_diff_ui_defaults(void)
+{
+ diff_detect_rename_default = 1;
+}
+
int git_diff_ui_config(const char *var, const char *value, void *cb)
{
if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
diff --git a/diff.h b/diff.h
index 70b2d70..0a3ce86 100644
--- a/diff.h
+++ b/diff.h
@@ -266,6 +266,7 @@ extern int parse_long_opt(const char *opt, const char
**argv,
const char **optarg);
extern int git_diff_basic_config(const char *var, const char *value, void *cb);
+extern void init_diff_ui_defaults(void);
extern int git_diff_ui_config(const char *var, const char *value, void *cb);
extern void diff_setup(struct diff_options *);
extern int diff_opt_parse(struct diff_options *, const char **, int, const
char *);
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index f5239b5..c7e58b6 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -124,7 +124,7 @@ test_expect_success 'test diff.renames=false' '
test_expect_success 'test diff.renames unset' '
git diff --cached $tree >current &&
- compare_diff_patch current no-rename
+ compare_diff_patch current expected
'
test_expect_success 'favour same basenames over different ones' '
diff --git a/t/t4013-diff-various.
[PATCH v2 1/2] push: remove "push.default is unset" warning message
The warning was important before the 2.0 transition, and remained
important for a while after, so that new users get push.default
explicitly in their configuration and do not experience inconsistent
behavior if they ever used an older version of Git.
The warning has been there since version 1.8.0 (Oct 2012), hence we can
expect the vast majority of current Git users to have been exposed to
it, and most of them have already set push.default explicitly. The
switch from 'matching' to 'simple' was planned for 2.0 (May 2014), but
actually happened only for 2.3 (Feb 2015).
Today, the warning is mostly seen by beginners, who have not set their
push.default configuration (yet). For many of them, the warning is
confusing because it talks about concepts that they have not learned and
asks them a choice that they are not able to make yet. See for example
http://stackoverflow.com/questions/13148066/warning-push-default-is-unset-its-implicit-value-is-changing-in-git-2-0
(1260 votes for the question, 1824 for the answer as of writing)
Remove the warning completely to avoid disturbing beginners. People who
still occasionally use an older version of Git will be exposed to the
warning through this old version.
Eventually, versions of Git without the warning will be deployed enough
and tutorials will not need to advise setting push.default anymore.
Signed-off-by: Matthieu Moy
---
builtin/push.c | 34 --
1 file changed, 34 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 960ffc3..270db40 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -204,37 +204,6 @@ static void setup_push_current(struct remote *remote,
struct branch *branch)
add_refspec(branch->name);
}
-static char warn_unspecified_push_default_msg[] =
-N_("push.default is unset; its implicit value has changed in\n"
- "Git 2.0 from 'matching' to 'simple'. To squelch this message\n"
- "and maintain the traditional behavior, use:\n"
- "\n"
- " git config --global push.default matching\n"
- "\n"
- "To squelch this message and adopt the new behavior now, use:\n"
- "\n"
- " git config --global push.default simple\n"
- "\n"
- "When push.default is set to 'matching', git will push local branches\n"
- "to the remote branches that already exist with the same name.\n"
- "\n"
- "Since Git 2.0, Git defaults to the more conservative 'simple'\n"
- "behavior, which only pushes the current branch to the corresponding\n"
- "remote branch that 'git pull' uses to update the current branch.\n"
- "\n"
- "See 'git help config' and search for 'push.default' for further
information.\n"
- "(the 'simple' mode was introduced in Git 1.7.11. Use the similar mode\n"
- "'current' instead of 'simple' if you sometimes use older versions of
Git)");
-
-static void warn_unspecified_push_default_configuration(void)
-{
- static int warn_once;
-
- if (warn_once++)
- return;
- warning("%s\n", _(warn_unspecified_push_default_msg));
-}
-
static int is_workflow_triangular(struct remote *remote)
{
struct remote *fetch_remote = remote_get(NULL);
@@ -253,9 +222,6 @@ static void setup_default_push_refspecs(struct remote
*remote)
break;
case PUSH_DEFAULT_UNSPECIFIED:
- warn_unspecified_push_default_configuration();
- /* fallthru */
-
case PUSH_DEFAULT_SIMPLE:
if (triangular)
setup_push_current(remote, branch);
--
2.7.2.334.g35ed2ae.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] Remove "push.default unset" warning
Junio and Peff both lean towards removing the message completely, and I think I'm convinced. We would have to do this in the future anyway. While we're there, improve the manual for git push as suggested by Philip Oakley. Matthieu Moy (2): push: remove "push.default is unset" warning message Documentation/git-push: document that 'simple' is the default Documentation/git-push.txt | 7 +++ builtin/push.c | 34 -- 2 files changed, 7 insertions(+), 34 deletions(-) -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] Documentation/git-push: document that 'simple' is the default
The default behavior is well documented already in git-config(1), but git-push(1) itself did not mention it at all. For users willing to learn how "git push" works but not how to configure it, this makes the documentation cumbersome to read. Make the git-push(1) page self-contained by adding a short summary of what 'push.default=simple' does, early in the page. Signed-off-by: Matthieu Moy --- Documentation/git-push.txt | 7 +++ 1 file changed, 7 insertions(+) diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 32482ce..a992793 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -37,6 +37,13 @@ the default `` by consulting `remote.*.push` configuration, and if it is not found, honors `push.default` configuration to decide what to push (See linkgit:git-config[1] for the meaning of `push.default`). +When neither the command-line nor the configuration specify what to +push, the default behavior is used, which corresponds to the `simple` +value for `push.default`: the current branch is pushed to the +corresponding upstream branch, but as a safety measure, the push is +aborted if the upstream branch does not have the same name as the +local one. + OPTIONS[[OPTIONS]] -- -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings
larsxschnei...@gmail.com writes: > --- a/Makefile > +++ b/Makefile > @@ -380,6 +380,18 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) > ALL_LDFLAGS = $(LDFLAGS) > STRIP ?= strip > > +ifdef DEVELOPER > + CFLAGS += -Werror \ > + -Wdeclaration-after-statement \ > + -Wno-format-zero-length \ > + -Wold-style-definition \ > + -Woverflow \ > + -Wpointer-arith \ > + -Wstrict-prototypes \ > + -Wunused \ > + -Wvla > +endif > + I guess you have tab-width=4. This portion looks ugly with tab-width=8. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Rebase performance
On Thu, Feb 25, 2016 at 7:50 AM, Duy Nguyen wrote: > On Thu, Feb 25, 2016 at 5:09 AM, Christian Couder > wrote: >> Another possibility would be to libify the "git apply" functionality >> and then to use the libified "git apply" in run_apply() instead of >> launching a separate "git apply" process. One benefit from this is >> that we could probably get rid of the read_cache_from() call at the >> end of run_apply() and this would likely further speed up things. Also >> avoiding to launch separate processes might be a win especially on >> Windows. > > The amount of global variables in apply.c is just scary. Libification > will need some cleanup first (i'm not against it though). Out of > curiosity, how long does it take to do "git update-index path>" on this repo? That would cover read_cache_from() and > write_cache(). While you're measuring, maybe sprinkle some > trace_performance() in apply.c:apply_patch() to get an idea where time > is most spent in? I did some experiment. The calls from am are basically for i in $PATCHES; do git apply --cached $i; git commit; done and we can approximate the libification version of that with git apply --cached $PATCHES (I hacked git-apply to do write-tree after each patch, close enough to git-commit). I tried it on my shallow-deepen series, 26 patches. The "for; do git-apply;done" command took 0m0.482s (real's time), taskset does not affect much for me, while the "libification" took just 0m0.105s. That's a very impressive number to aim for, and git.git is a small repo. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Some strange behavior of git
On 02/25, Olga Pshenichnikova wrote: > Hello, > we use git in our project. > What can be cause for further confusing behavior? > > git@ip5server:~$ git status > On branch master > Untracked files: > (use "git add ..." to include in what will be committed) > > app/addons/arliteks/ > > nothing added to commit but untracked files present (use "git add" to > track) > git@ip5server:~$ git clean -dn > Would remove app/addons/arliteks/ > Would remove design/ > Would remove js/ > Would remove var/langs/en/ > > Why I don't see all 4 directories in first command? Are the design/, js/ and var/langs/en/ directories empty? Git doesn't track empty directories, so they won't show up in git status, but with git clean -d you specify that Git should also remove untracked directories, which is why you see them here. > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Thomas -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 25/27] refs: add LMDB refs storage backend
On Thu, Feb 25, 2016 at 3:43 AM, David Turner wrote: >> > > > > I'm not sure I get this comment. D/F conflicts are no longer >> > > > > a >> > > > > thing >> > > > > for lmdb backend, right? >> > > > >> > > > I'm trying to avoid the lmdb backend creating a set of refs >> > > > that >> > > > the >> > > > files backend can't handle. This would make collaboration with >> > > > other >> > > > versions of git more difficult. >> > > >> > > It already is. If you create refs "foo" and "FOO" on case >> > > sensitive >> > > file system and clone it on a case-insensitive one, you face the >> > > same >> > > problem. We may have an optional configuration knob to prevent >> > > incompatibilities with files backend, but I think that should be >> > > done >> > > (and enforced if necessary) outside backends. >> > >> > Sure, the current state isn't perfect, but why make it worse? >> >> I see it from a different angle. The current state isn't perfect, but >> we will be moving to a better future where "files" backend may >> eventually be deprecated. Why hold back? >> >> But this line of reasoning only works if we have a new backend >> capable >> of replacing "files" without regressions or introducing new >> dependency. Which is why I suggest a new backend [1] (or implement >> Shawn's RefTree if it's proven as good with small repos) >> >> I have no problem if you want to stay strictly compatible with >> "files" >> though. >> >> [1] http://thread.gmane.org/gmane.comp.version-control.git/285893/foc >> us=286654 > > Won't RefTree have the same d/f conflict issue? It's trees all the way down if I understand it correctly, so yes RefTree should have d/f conflict issue as well. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 25/27] refs: add LMDB refs storage backend
On Thu, Feb 25, 2016 at 3:37 AM, David Turner wrote: > On Sat, 2016-02-20 at 15:59 +0700, Duy Nguyen wrote: >> On Thu, Feb 18, 2016 at 12:17 PM, David Turner < >> dtur...@twopensource.com> wrote: >> > LMDB has a few features that make it suitable for usage in git: >> > ... >> >> I'm reading lmdb documents and hitting the caveat section [1]. >> Random thoughts >> >> * "There is normally no pure read-only mode, since readers need write >> access to locks and lock file.". >> >> This will be a problem for server side that serves git:// protocol >> only. Some of those servers disable write access to the entire >> repository and git still works fine (but won't when lmdb is used). >> Should we do something in this case? Just tell server admins to relax >> file access, or use MDB_NOLOCK (and when? based on config var?) > > MDB_NOLOCK is a good idea. I'm going to add this to the "Weaknesses" > section of the docs and plan to fix it later, unless you feel strongly > otherwise. No I'm fine as long as it's documented. I was a bit disappointed when I found out about this because after reading lmdb paper I almost suggested we add lmdb to git.git as a submodule and prepare it to be the next default ref backend. The quest for the "next generation" default ref backend continues. >> * "Avoid long-lived transactions" >> >> OK we don't have a problem with this. But it makes me realize lmdb >> transactions do not map with ref transactions. We don't open lmdb >> transaction at ref_transaction_begin(), for example. Is it simply >> more >> convenient to do transactions the current way, or is it impossible or >> incorrect to attach lmdb transactions to ref_transaction_*()? > > That was what I did originally, but reviewers here noted that it had > some problems: > 1. What if, while a transaction is open, git opens a subprocess that > wants to make its own transaction? There can only be one writer > transaction open at a time. > 2. As you note, long-lived transactions. > > Also, the files backend also doesn't do any locking until the last > moment, and it's reasonable to try to be compatible with that. Some of these should be kept in the commit message for future reference. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings
On 02/25/2016 09:42 AM, larsxschnei...@gmail.com wrote: > From: Lars Schneider > > We assume Git developers have a reasonably modern compiler and recommend > them to enable the DEVELOPER makefile knob to ensure their patches are > clear of all compiler warnings the Git core project cares about. > > Enable the DEVELOPER makefile knob in the Travis-CI build. > > Suggested-by: Jeff King > Signed-off-by: Lars Schneider > --- > > This patch is the successor of "[PATCH v1] travis-ci: override CFLAGS > properly, > add -Wdeclaration-after-statement" [1] which enables compiler warnings for the > Travis-CI build. > > Peff suggested to codify the knowledge about the compiler warnings the Git > project cares about [2] which I have done here. > > The only problem is the "-Wold-style-declaration" compiler warning as this is > only supported by GCC and not Clang. Should we ignore that warning or would > you > prefer to detect the GCC compiler and add the warning? The Linux kernel > project > does a similar thing [3]. > > > Thanks, > Lars > > > [1] http://thread.gmane.org/gmane.comp.version-control.git/285752 > [2] http://article.gmane.org/gmane.comp.version-control.git/285761 > [3] > https://github.com/torvalds/linux/blob/6dc390ad61ac8dfca5fa9b0823981fb6f7ec17a0/Makefile#L303-L306 > > > .travis.yml| 2 +- > Documentation/CodingGuidelines | 4 > Makefile | 12 > 3 files changed, 17 insertions(+), 1 deletion(-) > > [...] > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines > index c6e536f..1c676c2 100644 > --- a/Documentation/CodingGuidelines > +++ b/Documentation/CodingGuidelines > @@ -171,6 +171,10 @@ For C programs: > > - We try to keep to at most 80 characters per line. > > + - As a Git developer we assume you have a reasonably modern compiler > + and we recommend you to enable the DEVELOPER makefile knob to > + ensure your patch is clear of all compiler warnings we care about. > + Instead of saying "enable the DEVELOPER makefile knob" could you be more explicit? Like maybe "create a file called `config.mak` and add the line `DEVELOPER=1` to it"? (Or whatever is your preferred way to tweak this setting.) > - We try to support a wide range of C compilers to compile Git with, > including old ones. That means that you should not use C99 > initializers, even if a lot of compilers grok it. > [...] Michael -- To unsubscribe from this list: send the line "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] unpack-trees: do not delete i-t-a entries in worktree even when forced
On Thu, Feb 25, 2016 at 6:23 AM, Junio C Hamano wrote: > Thinking about it more, I have to say that I do not agree with the > basic premise of this patch. I-T-A is not "may want to commit, but > they are untracked" at all. It is "I know I want to add, I just > cannot yet decide the exact contents". > > That is why "git add -N newfile && git grep string" would find the > string from newfile, and "git add -N newfile && git diff HEAD newfile" > would show the addition. We can agree to disagree. To me i-t-a is still a reminder, not a real tracked entry (with unknown content). > Sane people would expect that "git reset --hard HEAD" would behave > as "git diff HEAD | git apply --index -R" when your index is fully > merged, but this change will break the expectation. $ echo abc >abc $ git add -N abc $ git diff HEAD|LANG=C git apply --index -R error: abc: does not match index $ cat abc abc OK this is just nit-picking. We could probably make it work, though I'm not interested in doing that. > Earlier we changed "git commit" to pretend as if an I-T-A entry does > not exist in "git add -N newfile && git commit", but I think that > was a mistake that was caused by the same fuzzy thinking. > > 3f6d56de (commit: ignore intent-to-add entries instead of refusing, > 2012-02-07) does talk about the use of "git add -N" in conjunction > with "git status" and "git diff", but somehow nobody realized that > it was introducing inconsistency in the semantics. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git mv messed up file mapping if folders contain identical files
On Wed, Feb 24, 2016 at 04:38:11PM -0700, Bill Okara wrote: > Hi, > > I noticed the following 'git mv' issue with: > git version 2.6.4 > > > If there are identical files in different subfolders, 'git mv' the > root folder (and/or each file individually) will mess up the file path > mapping. that is, if having identical 'content.txt' file under > gitmvtest > |--demo/content.txt > |--dev/content.txt > |--prod/content.txt > > after doing the "git mv gitmvtest/resources > gitmvtest/src/main/resources", the 'git status' will show: > > renamed:gitmvtest/resources/demo/content.txt -> > gitmvtest/src/main/resources/demo/content.txt > renamed:gitmvtest/resources/prod/content.txt -> > gitmvtest/src/main/resources/dev/content.txt<== NOTE: > wrongly mapped the prod/content.txt to dev/content.txt > renamed:gitmvtest/resources/dev/content.txt -> > gitmvtest/src/main/resources/prod/content.txt<== NOTE: > wrongly mapped the dev/content.txt to prod/content.txt > > I tried running 'git mv' on each file individually, got the same problem: > > git mv gitmvtest/resources/demo/content.txt > > gitmvtest/src/main/resources/demo/content.txt > > git mv gitmvtest/resources/dev/content.txt > > gitmvtest/src/main/resources/dev/content.txt > > git mv gitmvtest/resources/prod/content.txt > > gitmvtest/src/main/resources/prod/content.txt > > > git status > renamed:gitmvtest/resources/demo/content.txt -> > gitmvtest/src/main/resources/demo/content.txt > renamed:gitmvtest/resources/prod/content.txt -> > gitmvtest/src/main/resources/dev/content.txt <== WRONG > renamed:gitmvtest/resources/dev/content.txt -> > gitmvtest/src/main/resources/prod/content.txt <== WRONG > > > NOTE: > === > if modified the content.txt in the 3 folders to contain different > data, then repeating the above 'git mv' will produce correct result, > > renamed:gitmvtest/resources/demo/content.txt -> > gitmvtest/src/main/resources/demo/content.txt <== CORRECT > renamed:gitmvtest/resources/dev/content.txt -> > gitmvtest/src/main/resources/dev/content.txt <== CORRECT > renamed:gitmvtest/resources/prod/content.txt -> > gitmvtest/src/main/resources/prod/content.txt <== CORRECT > > > > just want to see if this is a bug, user error (on my end), or?? > This looks like the same issue as submodule--helper list has: http://article.gmane.org/gmane.comp.version-control.git/287227 -- To unsubscribe from this list: send the line "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] upload-pack: use argv_array for pack_objects
Use the argv_array in the child_process structure, to avoid having to
manually maintain an array size.
Signed-off-by: Michael Procter
---
upload-pack.c | 25 +++--
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index b3f6653..dc802a0 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -90,35 +90,32 @@ static void create_pack_file(void)
"corruption on the remote side.";
int buffered = -1;
ssize_t sz;
- const char *argv[13];
- int i, arg = 0;
+ int i;
FILE *pipe_fd;
if (shallow_nr) {
- argv[arg++] = "--shallow-file";
- argv[arg++] = "";
+ argv_array_push(&pack_objects.args, "--shallow-file");
+ argv_array_push(&pack_objects.args, "");
}
- argv[arg++] = "pack-objects";
- argv[arg++] = "--revs";
+ argv_array_push(&pack_objects.args, "pack-objects");
+ argv_array_push(&pack_objects.args, "--revs");
if (use_thin_pack)
- argv[arg++] = "--thin";
+ argv_array_push(&pack_objects.args, "--thin");
- argv[arg++] = "--stdout";
+ argv_array_push(&pack_objects.args, "--stdout");
if (shallow_nr)
- argv[arg++] = "--shallow";
+ argv_array_push(&pack_objects.args, "--shallow");
if (!no_progress)
- argv[arg++] = "--progress";
+ argv_array_push(&pack_objects.args, "--progress");
if (use_ofs_delta)
- argv[arg++] = "--delta-base-offset";
+ argv_array_push(&pack_objects.args, "--delta-base-offset");
if (use_include_tag)
- argv[arg++] = "--include-tag";
- argv[arg++] = NULL;
+ argv_array_push(&pack_objects.args, "--include-tag");
pack_objects.in = -1;
pack_objects.out = -1;
pack_objects.err = -1;
pack_objects.git_cmd = 1;
- pack_objects.argv = argv;
if (start_command(&pack_objects))
die("git upload-pack: unable to fork git-pack-objects");
--
2.7.2.368.g02f4152
--
To unsubscribe from this list: send the line "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 v6 00/32] refs backend
A couple of build warnings I found, haven't really read the code yet. These two can easily be fixed refs/lmdb-backend.c: In function 'lmdb_read_raw_ref': refs/lmdb-backend.c:554:16: warning: unused variable 'err' [-Wunused-variable] struct strbuf err = STRBUF_INIT; ^ refs/lmdb-backend.c: In function 'lmdb_do_for_each_ref': refs/lmdb-backend.c:1625:15: warning: unused variable 'c' [-Wunused-variable] const char *c = resolve_ref_unsafe_submodule(submodule, refname, 0, oid.hash, ^ -Wshadow also gives a bunch of warnings, mostly about "transaction" and "env". Whether you want to fix them is really up to you. -- 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
[PATCH v2 00/20] Delete directories left empty after ref deletion
This re-roll addresses a few minor points that were brought up about v1 [1]: * "safe_create_leading_directories(): set errno on SCLD_EXISTS": * Set errno to ENOTDIR rather than EEXIST. * "raceproof_create_file(): new function": * Improve comments. * "rename_tmp_log(): use raceproof_create_file()": * Fix whitespace. * "rename_tmp_log(): improve error reporting": * Fix whitespace. This patch series is also available from my GitHub account [2] as branch delete-empty-refs-dirs. Thanks to Junio and Peff for their feedback about v1. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/286370 [2] http://github.com/mhagger/git Michael Haggerty (20): safe_create_leading_directories_const(): preserve errno safe_create_leading_directories(): set errno on SCLD_EXISTS raceproof_create_file(): new function lock_ref_sha1_basic(): use raceproof_create_file() rename_tmp_log(): use raceproof_create_file() rename_tmp_log(): improve error reporting log_ref_setup(): separate code for create vs non-create log_ref_setup(): improve robustness against races log_ref_setup(): pass the open file descriptor back to the caller log_ref_write_1(): don't depend on logfile log_ref_setup(): manage the name of the reflog file internally log_ref_write_1(): inline function try_remove_empty_parents(): rename parameter "name" -> "refname" try_remove_empty_parents(): don't trash argument contents try_remove_empty_parents(): don't accommodate consecutive slashes t5505: use "for-each-ref" to test for the non-existence of references delete_ref_loose(): derive loose reference path from lock delete_ref_loose(): inline function try_remove_empty_parents(): teach to remove parents of reflogs, too ref_transaction_commit(): clean up empty directories cache.h | 52 ++- refs/files-backend.c | 370 ++ refs/refs-internal.h | 9 +- sha1_file.c | 77 ++- t/t1400-update-ref.sh | 27 t/t5505-remote.sh | 2 +- 6 files changed, 351 insertions(+), 186 deletions(-) -- 2.7.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 v2 04/20] lock_ref_sha1_basic(): use raceproof_create_file()
Instead of coding the retry loop inline, use raceproof_create_file() to
make lock acquisition safe against directory creation/deletion races.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 47 +++
1 file changed, 19 insertions(+), 28 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b569762..a549942 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1889,6 +1889,19 @@ static int remove_empty_directories(struct strbuf *path)
return remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY);
}
+struct create_reflock_data {
+ struct lock_file *lk;
+ int lflags;
+};
+
+static int create_reflock(const char *path, void *cb)
+{
+ struct create_reflock_data *data = cb;
+
+ return hold_lock_file_for_update(data->lk, path, data->lflags) < 0
+ ? -1 : 0;
+}
+
/*
* Locks a ref returning the lock on success and NULL on failure.
* On failure errno is set to something meaningful.
@@ -1906,10 +1919,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
struct ref_lock *lock;
int last_errno = 0;
int type;
- int lflags = 0;
int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
int resolve_flags = 0;
- int attempts_remaining = 3;
+ struct create_reflock_data create_reflock_data = {NULL, 0};
assert(err);
@@ -1921,7 +1933,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
if (flags & REF_NODEREF) {
resolve_flags |= RESOLVE_REF_NO_RECURSE;
- lflags |= LOCK_NO_DEREF;
+ create_reflock_data.lflags |= LOCK_NO_DEREF;
}
refname = resolve_ref_unsafe(refname, resolve_flags,
@@ -1980,35 +1992,14 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
lock->orig_ref_name = xstrdup(orig_refname);
strbuf_git_path(&ref_file, "%s", refname);
- retry:
- switch (safe_create_leading_directories_const(ref_file.buf)) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining > 0)
- goto retry;
- /* fall through */
- default:
+ create_reflock_data.lk = lock->lk;
+
+ if (raceproof_create_file(ref_file.buf, create_reflock,
&create_reflock_data)) {
last_errno = errno;
- strbuf_addf(err, "unable to create directory for %s",
- ref_file.buf);
+ unable_to_lock_message(ref_file.buf, errno, err);
goto error_return;
}
- if (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) {
- last_errno = errno;
- if (errno == ENOENT && --attempts_remaining > 0)
- /*
-* Maybe somebody just deleted one of the
-* directories leading to ref_file. Try
-* again:
-*/
- goto retry;
- else {
- unable_to_lock_message(ref_file.buf, errno, err);
- goto error_return;
- }
- }
if (verify_lock(lock, old_sha1, mustexist, err)) {
last_errno = errno;
goto error_return;
--
2.7.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 v2 06/20] rename_tmp_log(): improve error reporting
* Don't capitalize error strings
* Report true paths of affected files
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2cb402d..ed29b3b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2430,10 +2430,11 @@ static int rename_tmp_log(const char *newrefname)
ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
if (ret) {
if (true_errno == EISDIR)
- error("Directory not empty: %s", path);
+ error("directory not empty: %s", path);
else
- error("unable to move logfile "TMP_RENAMED_LOG" to
logs/%s: %s",
- newrefname, strerror(true_errno));
+ error("unable to move logfile %s to %s: %s",
+ git_path(TMP_RENAMED_LOG), path,
+ strerror(true_errno));
}
free(path);
--
2.7.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 v2 13/20] try_remove_empty_parents(): rename parameter "name" -> "refname"
This is the standard nomenclature.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bd25ae2..543fd8e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2200,13 +2200,13 @@ static int pack_if_possible_fn(struct ref_entry *entry,
void *cb_data)
/*
* Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *name.
+ * Note: munges *refname.
*/
-static void try_remove_empty_parents(char *name)
+static void try_remove_empty_parents(char *refname)
{
char *p, *q;
int i;
- p = name;
+ p = refname;
for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
while (*p && *p != '/')
p++;
@@ -2224,7 +2224,7 @@ static void try_remove_empty_parents(char *name)
if (q == p)
break;
*q = '\0';
- if (rmdir(git_path("%s", name)))
+ if (rmdir(git_path("%s", refname)))
break;
}
}
--
2.7.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 v2 20/20] ref_transaction_commit(): clean up empty directories
When deleting/pruning references, remove any directories that are made
empty by the deletion of loose references or of reflogs. Otherwise such
empty directories can survive forever and accumulate over time. (Even
'pack-refs', which is smart enough to remove the parent directories of
loose references that it prunes, leaves directories that were already
empty.)
And now that ref_transaction_commit() takes care of deleting the parent
directories of loose references that it prunes, we don't have to do that
in prune_ref() anymore.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 37 +++--
refs/refs-internal.h | 9 -
t/t1400-update-ref.sh | 27 +++
3 files changed, 66 insertions(+), 7 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9ebb188..9973958 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2263,7 +2263,6 @@ static void prune_ref(struct ref_to_prune *r)
}
ref_transaction_free(transaction);
strbuf_release(&err);
- try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3261,6 +3260,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
+ update->flags |= REF_DELETED_LOOSE;
}
if (!(update->flags & REF_ISPRUNING))
@@ -3273,16 +3273,41 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
- for_each_string_list_item(ref_to_delete, &refs_to_delete)
- unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
+
+ /* Delete the reflogs of any references that were deleted: */
+ for_each_string_list_item(ref_to_delete, &refs_to_delete) {
+ if (!unlink_or_warn(git_path("logs/%s", ref_to_delete->string)))
+ try_remove_empty_parents(ref_to_delete->string,
+REMOVE_EMPTY_PARENTS_REFLOG);
+ }
+
clear_loose_ref_cache(&ref_cache);
cleanup:
transaction->state = REF_TRANSACTION_CLOSED;
- for (i = 0; i < n; i++)
- if (updates[i]->lock)
- unlock_ref(updates[i]->lock);
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (!update->lock)
+ continue;
+
+ if (update->flags & REF_DELETED_LOOSE) {
+ /*
+* The loose reference was deleted. We want to
+* delete any empty parent directories, but
+* that can only work after we have removed
+* the lockfile:
+*/
+ char *path = xstrdup(update->lock->ref_name);
+ unlock_ref(update->lock);
+ try_remove_empty_parents(path,
REMOVE_EMPTY_PARENTS_REF);
+ free(path);
+ } else {
+ unlock_ref(update->lock);
+ }
+ }
+
string_list_clear(&refs_to_delete, 0);
string_list_clear(&affected_refnames, 0);
return ret;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index c7dded3..dd09be1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -43,6 +43,12 @@
*/
/*
+ * Used as a flag in ref_update::flags when the loose reference has
+ * been deleted.
+ */
+#define REF_DELETED_LOOSE 0x100
+
+/*
* Return true iff refname is minimally safe. "Safe" here means that
* deleting a loose reference by this name will not do any damage, for
* example by causing a file that is not a reference to be deleted.
@@ -141,7 +147,8 @@ struct ref_update {
unsigned char old_sha1[20];
/*
* One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
-* REF_DELETING, and REF_ISPRUNING:
+* REF_DELETING, REF_ISPRUNING, REF_NEEDS_COMMIT, and
+* REF_DELETED_LOOSE:
*/
unsigned int flags;
struct ref_lock *lock;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index af1b20d..00284eb 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -191,6 +191,33 @@ test_expect_success \
git update-ref HEAD'" $A &&
test $A"' = $(cat .git/'"$m"')'
+test_expect_success "empty directory removal" '
+ git branch d1/d2/r1 HEAD &&
+ git branch d1/r2 HEAD &&
+ test -f .git/refs/heads/d1/d2/r1 &&
+ test -f .git/logs/refs/heads/d1/d2/r1 &&
+ git branch -d d1/d2/r1 &&
+ ! test -e .git/refs/heads/d1/d2 &&
+ ! test -e .git/logs/refs/heads/d1/d2
[PATCH v2 05/20] rename_tmp_log(): use raceproof_create_file()
Besides shortening the code, this saves an unnecessary call to
safe_create_leading_directories_const() in almost all cases.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 76 ++--
1 file changed, 32 insertions(+), 44 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a549942..2cb402d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2400,55 +2400,43 @@ out:
*/
#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
+static int rename_tmp_log_callback(const char *path, void *cb)
+{
+ int *true_errno = cb;
+
+ if (rename(git_path(TMP_RENAMED_LOG), path)) {
+ /*
+* rename(a, b) when b is an existing directory ought
+* to result in ISDIR, but Solaris 5.8 gives ENOTDIR.
+* Sheesh. Record the true errno for error reporting,
+* but report EISDIR to raceproof_create_file() so
+* that it knows to retry.
+*/
+ *true_errno = errno;
+ if (errno == ENOTDIR)
+ errno = EISDIR;
+ return -1;
+ } else {
+ return 0;
+ }
+}
+
static int rename_tmp_log(const char *newrefname)
{
- int attempts_remaining = 4;
- struct strbuf path = STRBUF_INIT;
- int ret = -1;
+ char *path = git_pathdup("logs/%s", newrefname);
+ int true_errno;
+ int ret;
- retry:
- strbuf_reset(&path);
- strbuf_git_path(&path, "logs/%s", newrefname);
- switch (safe_create_leading_directories_const(path.buf)) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining > 0)
- goto retry;
- /* fall through */
- default:
- error("unable to create directory for %s", newrefname);
- goto out;
- }
-
- if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
- if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining >
0) {
- /*
-* rename(a, b) when b is an existing
-* directory ought to result in ISDIR, but
-* Solaris 5.8 gives ENOTDIR. Sheesh.
-*/
- if (remove_empty_directories(&path)) {
- error("Directory not empty: logs/%s",
newrefname);
- goto out;
- }
- goto retry;
- } else if (errno == ENOENT && --attempts_remaining > 0) {
- /*
-* Maybe another process just deleted one of
-* the directories in the path to newrefname.
-* Try again from the beginning.
-*/
- goto retry;
- } else {
+ ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
+ if (ret) {
+ if (true_errno == EISDIR)
+ error("Directory not empty: %s", path);
+ else
error("unable to move logfile "TMP_RENAMED_LOG" to
logs/%s: %s",
- newrefname, strerror(errno));
- goto out;
- }
+ newrefname, strerror(true_errno));
}
- ret = 0;
-out:
- strbuf_release(&path);
+
+ free(path);
return ret;
}
--
2.7.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 v2 19/20] try_remove_empty_parents(): teach to remove parents of reflogs, too
Add a new "flags" parameter that tells the function whether to remove
empty parent directories of the loose reference file, of the reflog
file, or both. The new functionality is not yet used.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b137171..9ebb188 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2198,10 +2198,18 @@ static int pack_if_possible_fn(struct ref_entry *entry,
void *cb_data)
return 0;
}
+enum {
+ REMOVE_EMPTY_PARENTS_REF = 0x01,
+ REMOVE_EMPTY_PARENTS_REFLOG = 0x02
+};
+
/*
- * Remove empty parents, but spare refs/ and immediate subdirs.
+ * Remove empty parent directories associated with the specified
+ * reference and/or its reflog, but spare [logs/]refs/ and immediate
+ * subdirs. flags is a combination of REMOVE_EMPTY_PARENTS_REF and/or
+ * REMOVE_EMPTY_PARENTS_REFLOG.
*/
-static void try_remove_empty_parents(const char *refname)
+static void try_remove_empty_parents(const char *refname, unsigned int flags)
{
struct strbuf buf = STRBUF_INIT;
char *p, *q;
@@ -2216,7 +2224,7 @@ static void try_remove_empty_parents(const char *refname)
p++;
}
q = buf.buf + buf.len;
- while (1) {
+ while (flags & (REMOVE_EMPTY_PARENTS_REF |
REMOVE_EMPTY_PARENTS_REFLOG)) {
while (q > p && *q != '/')
q--;
if (q > p && *(q-1) == '/')
@@ -2224,8 +2232,12 @@ static void try_remove_empty_parents(const char *refname)
if (q == p)
break;
strbuf_setlen(&buf, q - buf.buf);
- if (rmdir(git_path("%s", buf.buf)))
- break;
+ if ((flags & REMOVE_EMPTY_PARENTS_REF) &&
+ rmdir(git_path("%s", buf.buf)))
+ flags &= ~REMOVE_EMPTY_PARENTS_REF;
+ if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) &&
+ rmdir(git_path("logs/%s", buf.buf)))
+ flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
}
strbuf_release(&buf);
}
@@ -2251,7 +2263,7 @@ static void prune_ref(struct ref_to_prune *r)
}
ref_transaction_free(transaction);
strbuf_release(&err);
- try_remove_empty_parents(r->name);
+ try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);
}
static void prune_refs(struct ref_to_prune *r)
--
2.7.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 v2 12/20] log_ref_write_1(): inline function
Now log_ref_write_1() doesn't do anything, so inline it.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 29 +++--
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 49713b3..bd25ae2 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2681,9 +2681,17 @@ static int log_ref_write_fd(int fd, const unsigned char
*old_sha1,
return 0;
}
-static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
- const unsigned char *new_sha1, const char *msg,
- int flags, struct strbuf *err)
+static int log_ref_write(const char *refname, const unsigned char *old_sha1,
+const unsigned char *new_sha1, const char *msg,
+int flags, struct strbuf *err)
+{
+ return files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,
+ err);
+}
+
+int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+ const unsigned char *new_sha1, const char *msg,
+ int flags, struct strbuf *err)
{
int logfd, result;
@@ -2714,21 +2722,6 @@ static int log_ref_write_1(const char *refname, const
unsigned char *old_sha1,
return 0;
}
-static int log_ref_write(const char *refname, const unsigned char *old_sha1,
-const unsigned char *new_sha1, const char *msg,
-int flags, struct strbuf *err)
-{
- return files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,
- err);
-}
-
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
- const unsigned char *new_sha1, const char *msg,
- int flags, struct strbuf *err)
-{
- return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);
-}
-
/*
* Write sha1 into the open lockfile, then close the lockfile. On
* errors, rollback the lockfile, fill in *err and
--
2.7.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 v2 17/20] delete_ref_loose(): derive loose reference path from lock
It is simpler to derive the path to the file that must be deleted from
"lock->ref_name" than from the lock_file object.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 46dcc23..7e825bd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2345,10 +2345,7 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag, struct strbuf *err)
* loose. The loose file name is the same as the
* lockfile name, minus ".lock":
*/
- char *loose_filename = get_locked_file_path(lock->lk);
- int res = unlink_or_msg(loose_filename, err);
- free(loose_filename);
- if (res)
+ if (unlink_or_msg(git_path("%s", lock->ref_name), err))
return 1;
}
return 0;
--
2.7.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 v2 11/20] log_ref_setup(): manage the name of the reflog file internally
Instead of writing the name of the reflog file into a strbuf that is
supplied by the caller but not needed there, write it into a local
temporary buffer and remove the strbuf parameter entirely.
And while we're adjusting the function signature, reorder the arguments
to move the input parameters before the output parameters.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 66 +++-
1 file changed, 29 insertions(+), 37 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4f25932..49713b3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2591,39 +2591,38 @@ static int open_or_create_logfile(const char *path,
void *cb)
}
/*
- * Create a reflog for a ref. Store its path to *logfile. If
- * force_create = 0, only create the reflog for certain refs (those
- * for which should_autocreate_reflog returns non-zero). Otherwise,
- * create it regardless of the reference name. If the logfile already
- * existed or was created, return 0 and set *logfd to the file
- * descriptor opened for appending to the file. If no logfile exists
- * and we decided not to create one, return 0 and set *logfd to -1. On
- * failure, fill in *err and return -1.
+ * Create a reflog for a ref. If force_create = 0, only create the
+ * reflog for certain refs (those for which should_autocreate_reflog
+ * returns non-zero). Otherwise, create it regardless of the reference
+ * name. If the logfile already existed or was created, return 0 and
+ * set *logfd to the file descriptor opened for appending to the file.
+ * If no logfile exists and we decided not to create one, return 0 and
+ * set *logfd to -1. On failure, fill in *err and return -1.
*/
-static int log_ref_setup(const char *refname,
-struct strbuf *logfile, int *logfd,
-struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname, int force_create,
+int *logfd, struct strbuf *err)
{
- strbuf_git_path(logfile, "logs/%s", refname);
+ char *logfile = git_pathdup("logs/%s", refname);
+ int ret = 0;
if (force_create || should_autocreate_reflog(refname)) {
- if (raceproof_create_file(logfile->buf, open_or_create_logfile,
logfd) < 0) {
+ if (raceproof_create_file(logfile, open_or_create_logfile,
logfd) < 0) {
if (errno == ENOENT) {
strbuf_addf(err, "unable to create directory
for %s: "
- "%s", logfile->buf,
strerror(errno));
+ "%s", logfile, strerror(errno));
} else if (errno == EISDIR) {
strbuf_addf(err, "there are still logs under
%s",
- logfile->buf);
+ logfile);
} else {
strbuf_addf(err, "unable to append to %s: %s",
- logfile->buf, strerror(errno));
+ logfile, strerror(errno));
}
- return -1;
+ ret = -1;
} else {
- adjust_shared_perm(logfile->buf);
+ adjust_shared_perm(logfile);
}
} else {
- *logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
+ *logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
if (*logfd < 0) {
if (errno == ENOENT || errno == EISDIR) {
/*
@@ -2634,27 +2633,25 @@ static int log_ref_setup(const char *refname,
*/
} else {
strbuf_addf(err, "unable to append to %s: %s",
- logfile->buf, strerror(errno));
- return -1;
+ logfile, strerror(errno));
+ ret = -1;
}
} else {
- adjust_shared_perm(logfile->buf);
+ adjust_shared_perm(logfile);
}
}
- return 0;
+ free(logfile);
+ return ret;
}
int safe_create_reflog(const char *refname, int force_create, struct strbuf
*err)
{
- int ret;
- struct strbuf sb = STRBUF_INIT;
- int fd;
+ int ret, fd;
- ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+ ret = log_ref_setup(refname, force_create, &fd, err);
if (!ret && fd >= 0)
close(fd);
- strbuf_release(&sb);
return ret;
}
@@ -2686,16 +2683,15 @@ static int log_ref_write_fd(int fd, const unsigned char
*old_sha1,
static
[PATCH v2 16/20] t5505: use "for-each-ref" to test for the non-existence of references
Instead of looking on the filesystem inside ".git/refs/remotes/origin", use "git for-each-ref" to check for leftover references under the remote's old name. Signed-off-by: Michael Haggerty --- t/t5505-remote.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 1a8e3b8..d2ac346 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -687,7 +687,7 @@ test_expect_success 'rename a remote' ' ( cd four && git remote rename origin upstream && - rmdir .git/refs/remotes/origin && + test -z "$(git for-each-ref refs/remotes/origin)" && test "$(git symbolic-ref refs/remotes/upstream/HEAD)" = "refs/remotes/upstream/master" && test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" && test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" && -- 2.7.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 v2 18/20] delete_ref_loose(): inline function
It was hardly doing anything anymore.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 25 +++--
1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7e825bd..b137171 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2336,21 +2336,6 @@ static int repack_without_refs(struct string_list
*refnames, struct strbuf *err)
return ret;
}
-static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
-{
- assert(err);
-
- if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
- /*
-* loose. The loose file name is the same as the
-* lockfile name, minus ".lock":
-*/
- if (unlink_or_msg(git_path("%s", lock->ref_name), err))
- return 1;
- }
- return 0;
-}
-
int delete_refs(struct string_list *refnames)
{
struct strbuf err = STRBUF_INIT;
@@ -3257,9 +3242,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update->flags & REF_DELETING) {
- if (delete_ref_loose(update->lock, update->type, err)) {
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
+ if (!(update->type & REF_ISPACKED) ||
+ update->type & REF_ISSYMREF) {
+ /* It is a loose reference. */
+ if (unlink_or_msg(git_path("%s",
update->lock->ref_name), err)) {
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
}
if (!(update->flags & REF_ISPRUNING))
--
2.7.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 v2 14/20] try_remove_empty_parents(): don't trash argument contents
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 543fd8e..1fcbb6f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2200,13 +2200,15 @@ static int pack_if_possible_fn(struct ref_entry *entry,
void *cb_data)
/*
* Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *refname.
*/
-static void try_remove_empty_parents(char *refname)
+static void try_remove_empty_parents(const char *refname)
{
+ struct strbuf buf = STRBUF_INIT;
char *p, *q;
int i;
- p = refname;
+
+ strbuf_addstr(&buf, refname);
+ p = buf.buf;
for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
while (*p && *p != '/')
p++;
@@ -2214,8 +2216,7 @@ static void try_remove_empty_parents(char *refname)
while (*p == '/')
p++;
}
- for (q = p; *q; q++)
- ;
+ q = buf.buf + buf.len;
while (1) {
while (q > p && *q != '/')
q--;
@@ -2223,10 +2224,11 @@ static void try_remove_empty_parents(char *refname)
q--;
if (q == p)
break;
- *q = '\0';
- if (rmdir(git_path("%s", refname)))
+ strbuf_setlen(&buf, q - buf.buf);
+ if (rmdir(git_path("%s", buf.buf)))
break;
}
+ strbuf_release(&buf);
}
/* make sure nobody touched the ref, and unlink */
--
2.7.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 v2 15/20] try_remove_empty_parents(): don't accommodate consecutive slashes
"refname" has already been checked by check_refname_format(), so it
cannot have consecutive slashes.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1fcbb6f..46dcc23 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2212,15 +2212,14 @@ static void try_remove_empty_parents(const char
*refname)
for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
while (*p && *p != '/')
p++;
- /* tolerate duplicate slashes; see check_refname_format() */
- while (*p == '/')
+ if (*p == '/')
p++;
}
q = buf.buf + buf.len;
while (1) {
while (q > p && *q != '/')
q--;
- while (q > p && *(q-1) == '/')
+ if (q > p && *(q-1) == '/')
q--;
if (q == p)
break;
--
2.7.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 v2 09/20] log_ref_setup(): pass the open file descriptor back to the caller
This function will most often be called by log_ref_write_1(), which
wants to append to the reflog file. In that case, it is silly to close
the file only for the caller to reopen it immediately. So, in the case
that the file was opened, pass the open file descriptor back to the
caller.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 37 +
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2cc9489..5937099 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2591,19 +2591,23 @@ static int open_or_create_logfile(const char *path,
void *cb)
}
/*
- * Create a reflog for a ref. If force_create = 0, the reflog will
- * only be created for certain refs (those for which
- * should_autocreate_reflog returns non-zero. Otherwise, create it
- * regardless of the ref name. Fill in *err and return -1 on failure.
+ * Create a reflog for a ref. Store its path to *logfile. If
+ * force_create = 0, only create the reflog for certain refs (those
+ * for which should_autocreate_reflog returns non-zero). Otherwise,
+ * create it regardless of the reference name. If the logfile already
+ * existed or was created, return 0 and set *logfd to the file
+ * descriptor opened for appending to the file. If no logfile exists
+ * and we decided not to create one, return 0 and set *logfd to -1. On
+ * failure, fill in *err and return -1.
*/
-static int log_ref_setup(const char *refname, struct strbuf *logfile, struct
strbuf *err, int force_create)
+static int log_ref_setup(const char *refname,
+struct strbuf *logfile, int *logfd,
+struct strbuf *err, int force_create)
{
- int logfd;
-
strbuf_git_path(logfile, "logs/%s", refname);
if (force_create || should_autocreate_reflog(refname)) {
- if (raceproof_create_file(logfile->buf, open_or_create_logfile,
&logfd) < 0) {
+ if (raceproof_create_file(logfile->buf, open_or_create_logfile,
logfd) < 0) {
if (errno == ENOENT) {
strbuf_addf(err, "unable to create directory
for %s: "
"%s", logfile->buf,
strerror(errno));
@@ -2617,11 +2621,10 @@ static int log_ref_setup(const char *refname, struct
strbuf *logfile, struct str
return -1;
} else {
adjust_shared_perm(logfile->buf);
- close(logfd);
}
} else {
- logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
- if (logfd < 0) {
+ *logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
+ if (*logfd < 0) {
if (errno == ENOENT || errno == EISDIR) {
/*
* The logfile doesn't already exist,
@@ -2636,7 +2639,6 @@ static int log_ref_setup(const char *refname, struct
strbuf *logfile, struct str
}
} else {
adjust_shared_perm(logfile->buf);
- close(logfd);
}
}
@@ -2647,8 +2649,11 @@ int safe_create_reflog(const char *refname, int
force_create, struct strbuf *err
{
int ret;
struct strbuf sb = STRBUF_INIT;
+ int fd;
- ret = log_ref_setup(refname, &sb, err, force_create);
+ ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+ if (!ret && fd >= 0)
+ close(fd);
strbuf_release(&sb);
return ret;
}
@@ -2684,17 +2689,17 @@ static int log_ref_write_1(const char *refname, const
unsigned char *old_sha1,
struct strbuf *logfile, int flags,
struct strbuf *err)
{
- int logfd, result, oflags = O_APPEND | O_WRONLY;
+ int logfd, result;
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, logfile, err, flags &
REF_FORCE_CREATE_REFLOG);
+ result = log_ref_setup(refname, logfile, &logfd, err,
+ flags & REF_FORCE_CREATE_REFLOG);
if (result)
return result;
- logfd = open(logfile->buf, oflags);
if (logfd < 0)
return 0;
result = log_ref_write_fd(logfd, old_sha1, new_sha1,
--
2.7.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 v2 02/20] safe_create_leading_directories(): set errno on SCLD_EXISTS
The exit path for SCLD_EXISTS wasn't setting errno, which some callers
use to generate error messages for the user. Fix the problem and
document that the function sets errno correctly to help avoid similar
regressions in the future.
While we're at it, document the difference between
safe_create_leading_directories() and
safe_create_leading_directories_const().
Signed-off-by: Michael Haggerty
---
cache.h | 10 --
sha1_file.c | 4 +++-
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index 26640b4..7d3f80c 100644
--- a/cache.h
+++ b/cache.h
@@ -950,8 +950,9 @@ int adjust_shared_perm(const char *path);
/*
* Create the directory containing the named path, using care to be
- * somewhat safe against races. Return one of the scld_error values
- * to indicate success/failure.
+ * somewhat safe against races. Return one of the scld_error values to
+ * indicate success/failure. On error, set errno to describe the
+ * problem.
*
* SCLD_VANISHED indicates that one of the ancestor directories of the
* path existed at one point during the function call and then
@@ -959,6 +960,11 @@ int adjust_shared_perm(const char *path);
* directory while we were working. To be robust against this kind of
* race, callers might want to try invoking the function again when it
* returns SCLD_VANISHED.
+ *
+ * The non _const version of this function temporarily modifies its
+ * path parameter while it is working but restores it before exiting.
+ * The _const version does not modify path (at the cost of having to
+ * make a temporary scratch copy).
*/
enum scld_error {
SCLD_OK = 0,
diff --git a/sha1_file.c b/sha1_file.c
index 568120e..94c6779 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -135,8 +135,10 @@ enum scld_error safe_create_leading_directories(char *path)
*slash = '\0';
if (!stat(path, &st)) {
/* path exists */
- if (!S_ISDIR(st.st_mode))
+ if (!S_ISDIR(st.st_mode)) {
+ errno = ENOTDIR;
ret = SCLD_EXISTS;
+ }
} else if (mkdir(path, 0777)) {
if (errno == EEXIST &&
!stat(path, &st) && S_ISDIR(st.st_mode))
--
2.7.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 v2 10/20] log_ref_write_1(): don't depend on logfile
It's unnecessary to pass a strbuf holding the reflog path up and down
the call stack now that it is hardly needed by the callers. Remove the
places where log_ref_write_1() uses it, in preparation for making it
internal to log_ref_setup().
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5937099..4f25932 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2705,14 +2705,14 @@ static int log_ref_write_1(const char *refname, const
unsigned char *old_sha1,
result = log_ref_write_fd(logfd, old_sha1, new_sha1,
git_committer_info(0), msg);
if (result) {
- strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
- strerror(errno));
+ strbuf_addf(err, "unable to append to %s: %s",
+ git_path("logs/%s", refname), strerror(errno));
close(logfd);
return -1;
}
if (close(logfd)) {
- strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
- strerror(errno));
+ strbuf_addf(err, "unable to append to %s: %s",
+ git_path("logs/%s", refname), strerror(errno));
return -1;
}
return 0;
--
2.7.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 v2 07/20] log_ref_setup(): separate code for create vs non-create
The behavior of this function (especially how it handles errors) is
quite different depending on whether we are willing to create the reflog
vs. whether we are only trying to open an existing reflog. So separate
the code paths.
This also simplifies the next steps.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 56 ++--
1 file changed, 37 insertions(+), 19 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ed29b3b..8fff1a8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2590,7 +2590,7 @@ static int commit_ref(struct ref_lock *lock)
*/
static int log_ref_setup(const char *refname, struct strbuf *logfile, struct
strbuf *err, int force_create)
{
- int logfd, oflags = O_APPEND | O_WRONLY;
+ int logfd;
strbuf_git_path(logfile, "logs/%s", refname);
if (force_create || should_autocreate_reflog(refname)) {
@@ -2599,36 +2599,54 @@ static int log_ref_setup(const char *refname, struct
strbuf *logfile, struct str
"%s", logfile->buf, strerror(errno));
return -1;
}
- oflags |= O_CREAT;
- }
-
- logfd = open(logfile->buf, oflags, 0666);
- if (logfd < 0) {
- if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))
- return 0;
+ logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
+ if (logfd < 0) {
+ if (errno == EISDIR) {
+ /*
+* The directory that is in the way might be
+* empty. Try to remove it.
+*/
+ if (remove_empty_directories(logfile)) {
+ strbuf_addf(err, "There are still logs
under "
+ "'%s'", logfile->buf);
+ return -1;
+ }
+ logfd = open(logfile->buf, O_APPEND | O_WRONLY
| O_CREAT, 0666);
+ }
- if (errno == EISDIR) {
- if (remove_empty_directories(logfile)) {
- strbuf_addf(err, "There are still logs under "
- "'%s'", logfile->buf);
+ if (logfd < 0) {
+ strbuf_addf(err, "unable to append to %s: %s",
+ logfile->buf, strerror(errno));
return -1;
}
- logfd = open(logfile->buf, oflags, 0666);
}
+ adjust_shared_perm(logfile->buf);
+ close(logfd);
+ } else {
+ logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
if (logfd < 0) {
- strbuf_addf(err, "unable to append to %s: %s",
- logfile->buf, strerror(errno));
- return -1;
+ if (errno == ENOENT || errno == EISDIR) {
+ /*
+* The logfile doesn't already exist,
+* but that is not an error; it only
+* means that we won't write log
+* entries to it.
+*/
+ } else {
+ strbuf_addf(err, "unable to append to %s: %s",
+ logfile->buf, strerror(errno));
+ return -1;
+ }
+ } else {
+ adjust_shared_perm(logfile->buf);
+ close(logfd);
}
}
- adjust_shared_perm(logfile->buf);
- close(logfd);
return 0;
}
-
int safe_create_reflog(const char *refname, int force_create, struct strbuf
*err)
{
int ret;
--
2.7.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 v2 01/20] safe_create_leading_directories_const(): preserve errno
Theoretically, free() is allowed to change errno. So preserve the errno
from safe_create_leading_directories() across the call to free().
Signed-off-by: Michael Haggerty
---
sha1_file.c | 4
1 file changed, 4 insertions(+)
diff --git a/sha1_file.c b/sha1_file.c
index aab1872..568120e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -164,10 +164,14 @@ enum scld_error safe_create_leading_directories(char
*path)
enum scld_error safe_create_leading_directories_const(const char *path)
{
+ int save_errno;
/* path points to cache entries, so xstrdup before messing with it */
char *buf = xstrdup(path);
enum scld_error result = safe_create_leading_directories(buf);
+
+ save_errno = errno;
free(buf);
+ errno = save_errno;
return result;
}
--
2.7.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 v2 03/20] raceproof_create_file(): new function
Add a function that tries to create a file and any containing
directories in a way that is robust against races with other processes
that might be cleaning up empty directories at the same time.
The actual file creation is done by a callback function, which, if it
fails, should set errno to EISDIR or ENOENT according to the convention
of open(). raceproof_create_file() detects such failures, and
respectively either tries to delete empty directories that might be in
the way of the file or tries to create the containing directories. Then
it retries the callback function.
This function is not yet used.
Signed-off-by: Michael Haggerty
---
cache.h | 42 +
sha1_file.c | 69 +
2 files changed, 111 insertions(+)
diff --git a/cache.h b/cache.h
index 7d3f80c..36b9c8b 100644
--- a/cache.h
+++ b/cache.h
@@ -976,6 +976,48 @@ enum scld_error {
enum scld_error safe_create_leading_directories(char *path);
enum scld_error safe_create_leading_directories_const(const char *path);
+/*
+ * Callback function for raceproof_create_file(). This function is
+ * expected to do something that makes dirname(path) permanent despite
+ * the fact that other processes might be cleaning up empty
+ * directories at the same time. Usually it will create a file named
+ * path, but alternatively it could create another file in that
+ * directory, or even chdir() into that directory. The function should
+ * return 0 if the action was completed successfully. On error, it
+ * should return a nonzero result and set errno.
+ * raceproof_create_file() treats two errno values specially:
+ *
+ * - ENOENT -- dirname(path) does not exist. In this case,
+ * raceproof_create_file() tries creating dirname(path)
+ * (and any parent directories, if necessary) and calls
+ * the function again.
+ *
+ * - EISDIR -- the file already exists and is a directory. In this
+ * case, raceproof_create_file() deletes the directory
+ * (recursively) if it is empty and calls the function
+ * again.
+ *
+ * Any other errno causes raceproof_create_file() to fail with the
+ * same return value and errno.
+ *
+ * Obviously, this function should be OK with being called again if it
+ * fails with ENOENT or EISDIR. In other scenarios it will not be
+ * called again.
+ */
+typedef int create_file_fn(const char *path, void *cb);
+
+/*
+ * Create a file in dirname(path) by calling fn, creating leading
+ * directories if necessary. Retry a few times in case we are racing
+ * with another process that is trying to clean up the directory that
+ * contains path. See the documentation for create_file_fn for more
+ * details.
+ *
+ * Return the value and set the errno that resulted from the most
+ * recent call of fn.
+ */
+int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
+
int mkdir_in_gitdir(const char *path);
extern char *expand_user_path(const char *path);
const char *enter_repo(const char *path, int strict);
diff --git a/sha1_file.c b/sha1_file.c
index 94c6779..344aeeb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -177,6 +177,75 @@ enum scld_error
safe_create_leading_directories_const(const char *path)
return result;
}
+int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
+{
+ /*
+* The number of times we will try to remove empty directories
+* in the way of path. This is only 1 because if another
+* process is racily creating directories that conflict with
+* us, we don't want to fight against them.
+*/
+ int remove_directories_remaining = 1;
+
+ /*
+* The number of times that we will try to create the
+* directories containing path. We are willing to attempt this
+* more than once, because another process could be trying to
+* clean up empty directories at the same time as we are
+* trying to create them.
+*/
+ int create_directories_remaining = 3;
+
+ /* A scratch copy of path, filled lazily if we need it: */
+ struct strbuf path_copy = STRBUF_INIT;
+
+ int save_errno;
+ int ret;
+
+retry_fn:
+ ret = fn(path, cb);
+ save_errno = errno;
+ if (!ret)
+ goto out;
+
+ if (errno == EISDIR && remove_directories_remaining > 0) {
+ /*
+* A directory is in the way. Maybe it is empty; try
+* to remove it:
+*/
+ if (!path_copy.len)
+ strbuf_addstr(&path_copy, path);
+
+ if (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY))
{
+ remove_directories_remaining--;
+ goto retry_fn;
+ }
+ } else if (errno == ENOENT && create_directories_remaining > 0) {
+ /*
+* Maybe the containi
[PATCH v2 08/20] log_ref_setup(): improve robustness against races
Change log_ref_setup() to use raceproof_create_file() to create the new
logfile. This makes it more robust against a race against another
process that might be trying to clean up empty directories while we are
trying to create a new logfile.
This also means that it will only call create_leading_directories() if
open() fails, which should be a net win. Even in the cases where we are
willing to create a new logfile, it will usually be the case that the
logfile already exists, or if not then that the directory containing the
logfile already exists. In such cases, we will save some work that was
previously done unconditionally.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 46 +-
1 file changed, 21 insertions(+), 25 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8fff1a8..2cc9489 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2582,6 +2582,14 @@ static int commit_ref(struct ref_lock *lock)
return 0;
}
+static int open_or_create_logfile(const char *path, void *cb)
+{
+ int *fd = cb;
+
+ *fd = open(path, O_APPEND | O_WRONLY | O_CREAT, 0666);
+ return (*fd < 0) ? -1 : 0;
+}
+
/*
* Create a reflog for a ref. If force_create = 0, the reflog will
* only be created for certain refs (those for which
@@ -2593,36 +2601,24 @@ static int log_ref_setup(const char *refname, struct
strbuf *logfile, struct str
int logfd;
strbuf_git_path(logfile, "logs/%s", refname);
+
if (force_create || should_autocreate_reflog(refname)) {
- if (safe_create_leading_directories(logfile->buf) < 0) {
- strbuf_addf(err, "unable to create directory for %s: "
- "%s", logfile->buf, strerror(errno));
- return -1;
- }
- logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
- if (logfd < 0) {
- if (errno == EISDIR) {
- /*
-* The directory that is in the way might be
-* empty. Try to remove it.
-*/
- if (remove_empty_directories(logfile)) {
- strbuf_addf(err, "There are still logs
under "
- "'%s'", logfile->buf);
- return -1;
- }
- logfd = open(logfile->buf, O_APPEND | O_WRONLY
| O_CREAT, 0666);
- }
-
- if (logfd < 0) {
+ if (raceproof_create_file(logfile->buf, open_or_create_logfile,
&logfd) < 0) {
+ if (errno == ENOENT) {
+ strbuf_addf(err, "unable to create directory
for %s: "
+ "%s", logfile->buf,
strerror(errno));
+ } else if (errno == EISDIR) {
+ strbuf_addf(err, "there are still logs under
%s",
+ logfile->buf);
+ } else {
strbuf_addf(err, "unable to append to %s: %s",
logfile->buf, strerror(errno));
- return -1;
}
+ return -1;
+ } else {
+ adjust_shared_perm(logfile->buf);
+ close(logfd);
}
-
- adjust_shared_perm(logfile->buf);
- close(logfd);
} else {
logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
if (logfd < 0) {
--
2.7.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] Change type of signed int flags to unsigned
“pattern” and “exclude” are two structs defined in attr.c and dir.h
respectively. Each contains a field named “flags” of type int, that
takes on values from the set of positive integers {1, 4, 8, 16}
enumerated through the macro EXC_FLAG_*.
That the most significant bit (used to store the sign) of these two
fields is not used in any special way, is observed from the fact
that, the flags fields (accessed within attr.c, dir.c, and
builtin/check-ignore.c) is either checked for it's value using the &
operator (e.g.: flags & EXC_FLAG_NODIR), or assigned a value of 0
first and then assigned any one of {1, 4, 8, 16} using the | operator
(e.g.: flags |= EXC_FLAG_NODIR). Hence, change the type of flags
to unsigned in both structs.
Furthermore, flags is passed by reference to the function
parse_exclude_pattern defined in dir.c, which accepts an “int *” type
for the flags argument. To avoid converting between pointers to
different types, change type of the flags argument to “unsigned *”.
While we’re at it, document the flags field of exclude to explicitly
state the values it’s supposed to take on.
Signed-off-by: Saurav Sachidanand
---
attr.c | 2 +-
dir.c | 4 ++--
dir.h | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/attr.c b/attr.c
index 086c08d..679e13c 100644
--- a/attr.c
+++ b/attr.c
@@ -124,7 +124,7 @@ struct pattern {
const char *pattern;
int patternlen;
int nowildcardlen;
- int flags; /* EXC_FLAG_* */
+ unsigned flags; /* EXC_FLAG_* */
};
/*
diff --git a/dir.c b/dir.c
index 552af23..d36fda7 100644
--- a/dir.c
+++ b/dir.c
@@ -459,7 +459,7 @@ int no_wildcard(const char *string)
void parse_exclude_pattern(const char **pattern,
int *patternlen,
- int *flags,
+ unsigned *flags,
int *nowildcardlen)
{
const char *p = *pattern;
@@ -500,7 +500,7 @@ void add_exclude(const char *string, const char *base,
{
struct exclude *x;
int patternlen;
- int flags;
+ unsigned flags;
int nowildcardlen;
parse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen);
diff --git a/dir.h b/dir.h
index 3ec3fb0..e34df5e 100644
--- a/dir.h
+++ b/dir.h
@@ -28,7 +28,7 @@ struct exclude {
int nowildcardlen;
const char *base;
int baselen;
- int flags;
+ unsigned flags; /* EXC_FLAG_* */
/*
* Counting starts from 1 for line numbers in ignore files,
@@ -244,7 +244,7 @@ extern struct exclude_list *add_exclude_list(struct
dir_struct *dir,
extern int add_excludes_from_file_to_list(const char *fname, const char *base,
int baselen,
struct exclude_list *el, int
check_index);
extern void add_excludes_from_file(struct dir_struct *, const char *fname);
-extern void parse_exclude_pattern(const char **string, int *patternlen, int
*flags, int *nowildcardlen);
+extern void parse_exclude_pattern(const char **string, int *patternlen,
unsigned *flags, int *nowildcardlen);
extern void add_exclude(const char *string, const char *base,
int baselen, struct exclude_list *el, int srcpos);
extern void clear_exclude_list(struct exclude_list *el);
--
2.7.1.339.g0233b80
This patch is for the suggested microproject for GSoC 2016 titled
"Use unsigned integral type for collection of bits."
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git mv messed up file mapping if folders contain identical files
On Thu, Feb 25, 2016 at 3:49 AM, Kevin Daudt wrote: > On Wed, Feb 24, 2016 at 04:38:11PM -0700, Bill Okara wrote: >> Hi, >> >> I noticed the following 'git mv' issue with: >> git version 2.6.4 >> >> >> If there are identical files in different subfolders, 'git mv' the >> root folder (and/or each file individually) will mess up the file path >> mapping. that is, if having identical 'content.txt' file under >> gitmvtest >> |--demo/content.txt >> |--dev/content.txt >> |--prod/content.txt >> >> after doing the "git mv gitmvtest/resources >> gitmvtest/src/main/resources", the 'git status' will show: >> >> renamed:gitmvtest/resources/demo/content.txt -> >> gitmvtest/src/main/resources/demo/content.txt >> renamed:gitmvtest/resources/prod/content.txt -> >> gitmvtest/src/main/resources/dev/content.txt<== NOTE: >> wrongly mapped the prod/content.txt to dev/content.txt >> renamed:gitmvtest/resources/dev/content.txt -> >> gitmvtest/src/main/resources/prod/content.txt<== NOTE: >> wrongly mapped the dev/content.txt to prod/content.txt >> >> I tried running 'git mv' on each file individually, got the same problem: >> > git mv gitmvtest/resources/demo/content.txt >> > gitmvtest/src/main/resources/demo/content.txt >> > git mv gitmvtest/resources/dev/content.txt >> > gitmvtest/src/main/resources/dev/content.txt >> > git mv gitmvtest/resources/prod/content.txt >> > gitmvtest/src/main/resources/prod/content.txt >> >> > git status >> renamed:gitmvtest/resources/demo/content.txt -> >> gitmvtest/src/main/resources/demo/content.txt >> renamed:gitmvtest/resources/prod/content.txt -> >> gitmvtest/src/main/resources/dev/content.txt <== WRONG >> renamed:gitmvtest/resources/dev/content.txt -> >> gitmvtest/src/main/resources/prod/content.txt <== WRONG >> >> >> NOTE: >> === >> if modified the content.txt in the 3 folders to contain different >> data, then repeating the above 'git mv' will produce correct result, >> >> renamed:gitmvtest/resources/demo/content.txt -> >> gitmvtest/src/main/resources/demo/content.txt <== CORRECT >> renamed:gitmvtest/resources/dev/content.txt -> >> gitmvtest/src/main/resources/dev/content.txt <== CORRECT >> renamed:gitmvtest/resources/prod/content.txt -> >> gitmvtest/src/main/resources/prod/content.txt <== CORRECT >> >> >> >> just want to see if this is a bug, user error (on my end), or?? >> > > This looks like the same issue as submodule--helper list has: > http://article.gmane.org/gmane.comp.version-control.git/287227 The submodule--helper is not called from within git-mv, so it may be a similar but not the same issue. ;) Looking through the code, the pathspec is not treated according to the newest style convention, I think it is one of the last places where the pathspec internals are poked with, instead of using parse_parsespec && match_parsespec. (That said it is very old hence often tested code in the wild. old code != bad code) Stefan -- To unsubscribe from this list: send the line "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: Segmentation fault found while fuzzing .pack file under 2.7.0.rc3
On Wed, Feb 24, 2016 at 10:48:27AM -0800, Junio C Hamano wrote: > >> We do not check if the offset of individual objects are within the > >> corresponding .pack file, either, and nth_packed_object_offset() > >> does return the data read from .idx file that is not checked for > >> sanity. use_pack(), which is the helper used by the callers of > >> nth_packed_object_offset() that finds the offset in the packfile for > >> each object, avoids allowing a read access to mapped pack data > >> beyond the end of it, so it is OK to return bogus value that was > >> read from the .idx file from this function, but there is one > >> computation the function itself does using a possibly bogus value > >> read from the disk: to find out where in the secondary offset table > >> in the .idx file the offset in the packfile is stored. > > > > Looks like this topic got dropped. I was reminded of it when somebody > > pointed me to a similar case[1] today which segfaults in a similar way (but > > this time was caused by actual filesystem corruption). > > > > Did you ever push the patch below further along? > > I do not think so, as I didn't "dig"; I recall trying to be explicit > that it was an illustration to prevent two extra and unnecessary > changes I alluded to in the earlier parts of the thread, not a real > patch. Thanks. I was planning to dig further, but I didn't want to duplicate any work. I've got a series which I'll post momentarily. -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
[PATCH 0/3] out-of-bounds access from corrupted .idx files
This series teaches Git to detect a few problems with corrupted .idx files, and adds tests for some more cases. There's conceptually some overlap with t5300, but I don't think it was covering any of these cases explicitly. There are two real bugs that could cause segfaults or bus errors via bogus reads (but never writes). On top of that, these are all problems in .idx files, which are usually generated locally. So I don't think there's anything particularly security interesting here. You'd need a situation where you convince somebody to read your .idx files (so maybe a multi-user server), and then I don't see how you'd turn it into remote code execution. I think with these patches, fuzzing .idx files should never result in any memory problems (though of course git will die()). Famous last words, of course. I stopped short of poking at other file formats, which might have similar issues. Obvious candidates are .bitmap files, and the on-disk $GIT_DIR/index. [1/3]: t5313: test bounds-checks of corrupted/malicious pack/idx files [2/3]: nth_packed_object_offset: bounds-check extended offset [3/3]: use_pack: handle signed off_t overflow -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
[PATCH 1/3] t5313: test bounds-checks of corrupted/malicious pack/idx files
Our on-disk .pack and .idx files may reference other data by
offset. We should make sure that we are not fooled by
corrupt data into accessing memory outside of our mmap'd
boundaries.
This patch adds a series of tests for offsets found in .pack
and .idx files. For the most part we get this right, but
there are two tests of .idx files marked as failures: we do
not bounds-check offsets in the v2 index's extended offset
table, nor do we handle .idx offsets that overflow a signed
off_t.
With these tests, we should have good coverage of all
offsets found in these files. Note that this doesn't cover
.bitmap files, which may have similar bugs.
Signed-off-by: Jeff King
---
t/t5313-pack-bounds-checks.sh | 179 ++
1 file changed, 179 insertions(+)
create mode 100755 t/t5313-pack-bounds-checks.sh
diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh
new file mode 100755
index 000..efc5197
--- /dev/null
+++ b/t/t5313-pack-bounds-checks.sh
@@ -0,0 +1,179 @@
+#!/bin/sh
+
+test_description='bounds-checking of access to mmapped on-disk file formats'
+. ./test-lib.sh
+
+clear_base () {
+ test_when_finished 'restore_base' &&
+ rm -f $base
+}
+
+restore_base () {
+ cp base-backup/* .git/objects/pack/
+}
+
+do_pack () {
+ pack_objects=$1; shift
+ sha1=$(
+ for i in $pack_objects
+ do
+ echo $i
+ done | git pack-objects "$@" .git/objects/pack/pack
+ ) &&
+ pack=.git/objects/pack/pack-$sha1.pack &&
+ idx=.git/objects/pack/pack-$sha1.idx &&
+ chmod +w $pack $idx &&
+ test_when_finished 'rm -f "$pack" "$idx"'
+}
+
+munge () {
+ printf "$3" | dd of="$1" bs=1 conv=notrunc seek=$2
+}
+
+# Offset in a v2 .idx to its initial and extended offset tables. For an index
+# with "nr" objects, this is:
+#
+# magic(4) + version(4) + fan-out(4*256) + sha1s(20*nr) + crc(4*nr),
+#
+# for the initial, and another ofs(4*nr) past that for the extended.
+#
+ofs_table () {
+ echo $((4 + 4 + 4*256 + 20*$1 + 4*$1))
+}
+extended_table () {
+ echo $(($(ofs_table "$1") + 4*$1))
+}
+
+test_expect_success 'set up base packfile and variables' '
+ # the hash of this content starts with ff, which
+ # makes some later computations much simpler
+ echo 74 >file &&
+ git add file &&
+ git commit -m base &&
+ git repack -ad &&
+ base=$(echo .git/objects/pack/*) &&
+ chmod +w $base &&
+ mkdir base-backup &&
+ cp $base base-backup/ &&
+ object=$(git rev-parse HEAD:file)
+'
+
+test_expect_success 'pack/index object count mismatch' '
+ do_pack $object &&
+ munge $pack 8 "\377\0\0\0" &&
+ clear_base &&
+
+ # We enumerate the objects from the completely-fine
+ # .idx, but notice later that the .pack is bogus
+ # and fail to show any data.
+ echo "$object missing" >expect &&
+ git cat-file --batch-all-objects --batch-check >actual &&
+ test_cmp expect actual &&
+
+ # ...and here fail to load the object (without segfaulting),
+ # but fallback to a good copy if available.
+ test_must_fail git cat-file blob $object &&
+ restore_base &&
+ git cat-file blob $object >actual &&
+ test_cmp file actual &&
+
+ # ...and make sure that index-pack --verify, which has its
+ # own reading routines, does not segfault.
+ test_must_fail git index-pack --verify $pack
+'
+
+test_expect_success 'matched bogus object count' '
+ do_pack $object &&
+ munge $pack 8 "\377\0\0\0" &&
+ munge $idx $((255 * 4)) "\377\0\0\0" &&
+ clear_base &&
+
+ # Unlike above, we should notice early that the .idx is totally
+ # bogus, and not even enumerate its contents.
+ >expect &&
+ git cat-file --batch-all-objects --batch-check >actual &&
+ test_cmp expect actual &&
+
+ # But as before, we can do the same object-access checks.
+ test_must_fail git cat-file blob $object &&
+ restore_base &&
+ git cat-file blob $object >actual &&
+ test_cmp file actual &&
+
+ test_must_fail git index-pack --verify $pack
+'
+
+# Note that we cannot check the fallback case for these
+# further .idx tests, as we notice the problem in functions
+# whose interface doesn't allow an error return (like use_pack()),
+# and thus we just die().
+#
+# There's also no point in doing enumeration tests, as
+# we are munging offsets here, which are about looking up
+# specific objects.
+
+test_expect_success 'bogus object offset (v1)' '
+ do_pack $object --index-version=1 &&
+ munge $idx $((4 * 256)) "\377\0\0\0" &&
+ clear_base &&
+ test_must_fail git cat-file blob $object &&
+ test_must_fail git index-pack --verify $pack
+'
+
+test_expect_success 'bogus object offset (v2, no msb)' '
+ do_pack $object --index-version=2 &&
+ munge $id
[PATCH 2/3] nth_packed_object_offset: bounds-check extended offset
If a pack .idx file has a corrupted offset for an object, we
may try to access an offset in the .idx or .pack file that
is larger than the file's size. For the .pack case, we have
use_pack() to protect us, which realizes the access is out
of bounds. But if the corrupted value asks us to look in the
.idx file's secondary 64-bit offset table, we blindly add it
to the mmap'd index data and access arbitrary memory.
We can fix this with a simple bounds-check compared to the
size we found when we opened the .idx file.
Note that there's similar code in index-pack that is
triggered only during "index-pack --verify". To support
both, we pull the bounds-check into a separate function,
which dies when it sees a corrupted file.
It would be nice if we could return an error, so that the
pack code could try to find a good copy of the object
elsewhere. Currently nth_packed_object_offset doesn't have
any way to return an error, but it could probably use "0" as
a sentinel value (since no object can start there). This is
the minimal fix, and we can improve the resilience later on
top.
Signed-off-by: Jeff King
---
builtin/index-pack.c | 1 +
cache.h | 10 ++
sha1_file.c | 15 +++
t/t5313-pack-bounds-checks.sh | 2 +-
4 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 6a01509..ad6a1d7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1514,6 +1514,7 @@ static void read_v2_anomalous_offsets(struct packed_git
*p,
if (!(off & 0x8000))
continue;
off = off & 0x7fff;
+ check_pack_index_ptr(p, &idx2[off * 2]);
if (idx2[off * 2])
continue;
/*
diff --git a/cache.h b/cache.h
index 83b688c..25a7040 100644
--- a/cache.h
+++ b/cache.h
@@ -1370,6 +1370,16 @@ extern void clear_delta_base_cache(void);
extern struct packed_git *add_packed_git(const char *path, size_t path_len,
int local);
/*
+ * Make sure that a pointer access into an mmap'd index file is within bounds,
+ * and can provide at least 8 bytes of data.
+ *
+ * Note that this is only necessary for variable-length segments of the file
+ * (like the 64-bit extended offset table), as we compare the size to the
+ * fixed-length parts when we open the file.
+ */
+extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
+
+/*
* Return the SHA-1 of the nth object within the specified packfile.
* Open the index if it is not already open. The return value points
* at the SHA-1 within the mmapped index. Return NULL if there is an
diff --git a/sha1_file.c b/sha1_file.c
index aab1872..d2ffd92 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2446,6 +2446,20 @@ const unsigned char *nth_packed_object_sha1(struct
packed_git *p,
}
}
+void check_pack_index_ptr(const struct packed_git *p, const void *vptr)
+{
+ const unsigned char *ptr = vptr;
+ const unsigned char *start = p->index_data;
+ const unsigned char *end = start + p->index_size;
+ if (ptr < start)
+ die("offset before start of pack index for %s (corrupt index?)",
+ p->pack_name);
+ /* No need to check for underflow; .idx files must be at least 8 bytes
*/
+ if (ptr >= end - 8)
+ die("offset beyond end of pack index for %s (truncated index?)",
+ p->pack_name);
+}
+
off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
{
const unsigned char *index = p->index_data;
@@ -2459,6 +2473,7 @@ off_t nth_packed_object_offset(const struct packed_git
*p, uint32_t n)
if (!(off & 0x8000))
return off;
index += p->num_objects * 4 + (off & 0x7fff) * 8;
+ check_pack_index_ptr(p, index);
return (((uint64_t)ntohl(*((uint32_t *)(index + 0 << 32) |
ntohl(*((uint32_t *)(index + 4)));
}
diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh
index efc5197..0717746 100755
--- a/t/t5313-pack-bounds-checks.sh
+++ b/t/t5313-pack-bounds-checks.sh
@@ -128,7 +128,7 @@ test_expect_success 'bogus object offset (v2, no msb)' '
test_must_fail git index-pack --verify $pack
'
-test_expect_failure 'bogus offset into v2 extended table' '
+test_expect_success 'bogus offset into v2 extended table' '
do_pack $object --index-version=2 &&
munge $idx $(ofs_table 1) "\377\0\0\0" &&
clear_base &&
--
2.7.2.695.gf3fde8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] use_pack: handle signed off_t overflow
A v2 pack index file can specify an offset within a packfile
of up to 2^64-1 bytes. On a system with a signed 64-bit
off_t, we can represent only up to 2^63-1. This means that a
corrupted .idx file can end up with a negative offset in the
pack code. Our bounds-checking use_pack function looks for
too-large offsets, but not for ones that have wrapped around
to negative. Let's do so, which fixes an out-of-bounds
access demonstrated in t5313.
Signed-off-by: Jeff King
---
sha1_file.c | 2 ++
t/t5313-pack-bounds-checks.sh | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/sha1_file.c b/sha1_file.c
index d2ffd92..9d223e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1076,6 +1076,8 @@ unsigned char *use_pack(struct packed_git *p,
die("packfile %s cannot be accessed", p->pack_name);
if (offset > (p->pack_size - 20))
die("offset beyond end of packfile (truncated pack?)");
+ if (offset < 0)
+ die("offset before end of packfile (broken .idx?)");
if (!win || !in_window(win, offset)) {
if (win)
diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh
index 0717746..a8a587a 100755
--- a/t/t5313-pack-bounds-checks.sh
+++ b/t/t5313-pack-bounds-checks.sh
@@ -136,7 +136,7 @@ test_expect_success 'bogus offset into v2 extended table' '
test_must_fail git index-pack --verify $pack
'
-test_expect_failure 'bogus offset inside v2 extended table' '
+test_expect_success 'bogus offset inside v2 extended table' '
# We need two objects here, so we can plausibly require
# an extended table (if the first object were larger than 2^31).
do_pack "$object $(git rev-parse HEAD)" --index-version=2 &&
--
2.7.2.695.gf3fde8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
donation
I, Liliane authenticate this email of 3.5M USD donation to you,please view my link: http://en.wikipedia.org/wiki/Liliane_Bettencourt and Email me on lilianbett...@gmail.com for more info -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git config: report when trying to modify a non-existing repo config
Hi Junio, On Wed, 24 Feb 2016, Junio C Hamano wrote: > Junio C Hamano writes: > > > Johannes Schindelin writes: > > > >> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh > >> index 91235b7..f62409e 100755 > >> --- a/t/t1308-config-set.sh > >> +++ b/t/t1308-config-set.sh > >> @@ -218,4 +218,13 @@ test_expect_success 'check line errors for malformed > >> values' ' > >>test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result > >> ' > >> > >> +test_expect_success 'error on modifying repo config without repo' ' > >> + mkdir no-repo && > >> + GIT_CEILING_DIRECTORIES=$(pwd) && > >> + export GIT_CEILING_DIRECTORIES && > >> + cd no-repo && > >> + test_must_fail git config a.b c 2>err && > >> + grep "not in a git directory" err > >> +' > >> + > >> test_done > > > > Please make it a habit to run tests that go up/down in the hierarchy > > in a subshell. It is not a good excuse that this new test happens > > to be at the end _right now_. > > I'd squash this in. Please do, unless you want me to resend with the squashed commit (I picked up the change from your 'pu' branch)? Ciao, Dscho -- To unsubscribe from this list: send the line "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: Some strange behavior of git
[Please keep everyone cc'd in the conversation, especially the mailing list. I added it back for now. Also please don't top post on the list] On 02/25, Olga Pshenichnikova wrote: > No, it isn't empty, but I found the problem. > Problem was that I handled subdirectories structure in exclude file: > > design/dir1 > design/dir2 > > But > > design/dir3 wasn't ignored and wasn't controlled. > > So, my problem take place when some sub directory both isn't ignored AND > isn't controlled even if it isn't empty. I think that's a bug, I think this directory should only be removed if -x is given. I haven't come up with a patch yet, but here's a test demonstrating the failure. diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index 86ceb38..0961007 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -653,4 +653,13 @@ test_expect_success 'git clean -d respects pathspecs (pathspec is prefix of dir) test_path_is_dir foobar ' +test_expect_failure 'git clean -d does not clean ignored files in subdir' ' + mkdir -p sub/dir && + >sub/dir/file && + test_when_finished rm .gitignore && + echo sub/dir/ >.gitignore && + git clean -df sub && + test_path_is_file sub/dir/file +' + test_done > Thank you for response! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git mv messed up file mapping if folders contain identical files
resent, forgot to reply to all...
I guess a bigger concern of this issue is the mess up of history. That
is, even if not doing an merge/update, just doing the 'git mv' will
messed up the file history, as shown in following:
// Add a new resources/qa/content.txt files with a new commit message:
> mkdir gitmvtest/resources/qa
> cp gitmvtest/resources/demo/content.txt gitmvtest/resources/qa/.
> git add .
>git commit -m "Add a new QA context.txt"
[master caba387] Add a new QA context.txt
1 file changed, 2 insertions(+)
// Do the git mv
> git checkout -b branch5
> git mv gitmvtest/resources gitmvtest/src/main/.
> git commit -m "Move resources to src/main/resources"
[branch5 dd44309] Move resources to src/main/resources
4 files changed, 0 insertions(+), 0 deletions(-)
rename gitmvtest/{resources/qa =>
src/main/resources/demo}/content.txt (100%)<-- WRONG
rename gitmvtest/{resources/prod =>
src/main/resources/dev}/content.txt (100%)<-- WRONG
rename gitmvtest/{resources/dev =>
src/main/resources/prod}/content.txt (100%)<-- WRONG
rename gitmvtest/{resources/demo =>
src/main/resources/qa}/content.txt (100%) <-- WRONG
// WRONG HISTORY
> git log --follow --oneline gitmvtest/src/main/resources/demo/content.txt
> <== demo/content.txt points to the new QA history
dd44309 Move resources to src/main/resources
caba387 Add a new QA context.txt<== WRONG HISTORY
thanks,
Bill
On Thu, Feb 25, 2016 at 6:56 AM, Stefan Beller wrote:
> On Thu, Feb 25, 2016 at 3:49 AM, Kevin Daudt wrote:
>> On Wed, Feb 24, 2016 at 04:38:11PM -0700, Bill Okara wrote:
>>> Hi,
>>>
>>> I noticed the following 'git mv' issue with:
>>> git version 2.6.4
>>>
>>>
>>> If there are identical files in different subfolders, 'git mv' the
>>> root folder (and/or each file individually) will mess up the file path
>>> mapping. that is, if having identical 'content.txt' file under
>>> gitmvtest
>>> |--demo/content.txt
>>> |--dev/content.txt
>>> |--prod/content.txt
>>>
>>> after doing the "git mv gitmvtest/resources
>>> gitmvtest/src/main/resources", the 'git status' will show:
>>>
>>> renamed:gitmvtest/resources/demo/content.txt ->
>>> gitmvtest/src/main/resources/demo/content.txt
>>> renamed:gitmvtest/resources/prod/content.txt ->
>>> gitmvtest/src/main/resources/dev/content.txt<== NOTE:
>>> wrongly mapped the prod/content.txt to dev/content.txt
>>> renamed:gitmvtest/resources/dev/content.txt ->
>>> gitmvtest/src/main/resources/prod/content.txt<== NOTE:
>>> wrongly mapped the dev/content.txt to prod/content.txt
>>>
>>> I tried running 'git mv' on each file individually, got the same problem:
>>> > git mv gitmvtest/resources/demo/content.txt
>>> > gitmvtest/src/main/resources/demo/content.txt
>>> > git mv gitmvtest/resources/dev/content.txt
>>> > gitmvtest/src/main/resources/dev/content.txt
>>> > git mv gitmvtest/resources/prod/content.txt
>>> > gitmvtest/src/main/resources/prod/content.txt
>>>
>>> > git status
>>> renamed:gitmvtest/resources/demo/content.txt ->
>>> gitmvtest/src/main/resources/demo/content.txt
>>> renamed:gitmvtest/resources/prod/content.txt ->
>>> gitmvtest/src/main/resources/dev/content.txt <== WRONG
>>> renamed:gitmvtest/resources/dev/content.txt ->
>>> gitmvtest/src/main/resources/prod/content.txt <== WRONG
>>>
>>>
>>> NOTE:
>>> ===
>>> if modified the content.txt in the 3 folders to contain different
>>> data, then repeating the above 'git mv' will produce correct result,
>>>
>>> renamed:gitmvtest/resources/demo/content.txt ->
>>> gitmvtest/src/main/resources/demo/content.txt <== CORRECT
>>> renamed:gitmvtest/resources/dev/content.txt ->
>>> gitmvtest/src/main/resources/dev/content.txt <== CORRECT
>>> renamed:gitmvtest/resources/prod/content.txt ->
>>> gitmvtest/src/main/resources/prod/content.txt <== CORRECT
>>>
>>>
>>>
>>> just want to see if this is a bug, user error (on my end), or??
>>>
>>
>> This looks like the same issue as submodule--helper list has:
>> http://article.gmane.org/gmane.comp.version-control.git/287227
>
> The submodule--helper is not called from within git-mv, so it may be
> a similar but not the same issue. ;)
>
> Looking through the code, the pathspec is not treated according to the newest
> style convention, I think it is one of the last places where the
> pathspec internals
> are poked with, instead of using parse_parsespec && match_parsespec.
> (That said it is very old hence often tested code in the wild. old
> code != bad code)
>
> Stefan
--
To unsubscribe from this list: send the line "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: Rebase performance
On Wed, Feb 24, 2016 at 11:09 PM, Christian Couder wrote: [Resent because I was accidentally in GMail's HTML mode and the ML rejected it] > If there was a config option called maybe "rebase.taskset" or > "rebase.setcpuaffinity" that could be set to ask the OS for all the > rebase child processes to be run on the same core, people who run many > rebases on big repos on big servers as we do at Booking.com could > easily benefit from a nice speed up. > > Technically the option may make git-rebase--am.sh call "git am" using > "taskset" (if taskset is available on the current OS). I think aside from issues with git-apply this would be an interesting feature to have in git. I.e. some general facility to intercept commands and inject a prefix command in front of them, whether that's taskset, nice/ionice, strace etc. > Another possibility would be to libify the "git apply" functionality > and then to use the libified "git apply" in run_apply() instead of > launching a separate "git apply" process. One benefit from this is > that we could probably get rid of the read_cache_from() call at the > end of run_apply() and this would likely further speed up things. Also > avoiding to launch separate processes might be a win especially on > Windows. Yeah that should help in this particular case and make the taskset redundant since the whole sequence of operations would all be on one core, right? At the risk of derailing this thread, a thing that would make rebase even faster I think would be to change it so that instead of applying a patch at a time to the working tree the whole operation takes place on temporary trees & commits and then we'll eventually move the branch pointer to that once it's finished. I.e. there's no reason for why a sequence of 1000 patches where a FOO.txt is changed from "hi1", "hi2", "hi3", ... would be noticeably slower than applying the same changes with git-fast-import. Of course this would require a lot of nuances, e.g. if there's a conflict we'd need to change the working tree & index as we do now before continuing. Has anyone looked into some advanced refactoring of the rebase process that would work like this, or has some feedback on why this would be dumb or that there's a better way to do it? -- To unsubscribe from this list: send the line "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: Some strange behavior of git
Thanx ) On 02/25/2016 07:14 PM, Thomas Gummerer wrote: [Please keep everyone cc'd in the conversation, especially the mailing list. I added it back for now. Also please don't top post on the list] On 02/25, Olga Pshenichnikova wrote: No, it isn't empty, but I found the problem. Problem was that I handled subdirectories structure in exclude file: design/dir1 design/dir2 But design/dir3 wasn't ignored and wasn't controlled. So, my problem take place when some sub directory both isn't ignored AND isn't controlled even if it isn't empty. I think that's a bug, I think this directory should only be removed if -x is given. I haven't come up with a patch yet, but here's a test demonstrating the failure. diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index 86ceb38..0961007 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -653,4 +653,13 @@ test_expect_success 'git clean -d respects pathspecs (pathspec is prefix of dir) test_path_is_dir foobar ' +test_expect_failure 'git clean -d does not clean ignored files in subdir' ' + mkdir -p sub/dir && + >sub/dir/file && + test_when_finished rm .gitignore && + echo sub/dir/ >.gitignore && + git clean -df sub && + test_path_is_file sub/dir/file +' + test_done Thank you for response! -- To unsubscribe from this list: send the line "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] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate
Hi, guys! > I sent in a patch (and I believe I CC'd you) that adds an option > http.emptyAuth that can be used in this case. It should make its way to > a future release. Somehow I've missed your letter... > The patch has been queued as 121061f6 (http: add option to try > authentication without username, 2016-02-15); perhaps you can help > by trying it out in your installation before it hits a released > version of Git? That way, if the patch does not fix your problem, > or it introduces an unexpected side effect, we would notice before > we include it in a future release. I've cherry-picked commit 121061f6 over version 2.4.10 and 2.7.1. In both cases it works exactly as expected. Please, let me know if I can help with something else regarding this issue. 2016-02-21 0:38 GMT+03:00 Junio C Hamano : > Dmitry Vilkov writes: > >> Hi guys! Any luck with fixing this issue? > > I think Brian suggested an alternative approach, to which you earler > responded > >>> That would be great! Definitely it will be much better solution than >>> patch I've proposed. > > The patch has been queued as 121061f6 (http: add option to try > authentication without username, 2016-02-15); perhaps you can help > by trying it out in your installation before it hits a released > version of Git? That way, if the patch does not fix your problem, > or it introduces an unexpected side effect, we would notice before > we include it in a future release. > > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] Documentation/diff-config: fix description of diff.renames
Matthieu Moy imag.fr> writes: > diff.renames:: > - Tells Git to detect renames. If set to any boolean value, it > - will enable basic rename detection. If set to "copies" or > - "copy", it will detect copies, as well. > + Whether and how Git detects renames. If set to "false", > + rename detection is disabled. If set to "true", basic rename > + detection is enable. If set to "copies" or "copy", Git will > + detect copies, as well. Defaults to false. Just a minor typo: s/enable/enabled/ Also, there is only one space between the second and third sentences. Just in case you haven't already fixed that. Regards, Felipe -- To unsubscribe from this list: send the line "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: Rebase performance
Ævar Arnfjörð Bjarmason writes: > At the risk of derailing this thread, a thing that would make rebase > even faster I think would be to change it so that instead of applying > a patch at a time to the working tree the whole operation takes place > on temporary trees & commits and then we'll eventually move the branch > pointer to that once it's finished. > > I.e. there's no reason for why a sequence of 1000 patches where a > FOO.txt is changed from "hi1", "hi2", "hi3", ... would be noticeably > slower than applying the same changes with git-fast-import. Also, not touching the worktree during rebase would have the advantage that if the final result doesn't change a file, we wouldn't need to touch this file at all, hence the next "make" (or whatever timestamp-using build system the user runs) would consider this file unchanged. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] Documentation/diff-config: fix description of diff.renames
Felipe Gonçalves Assis writes: > Matthieu Moy imag.fr> writes: > >> diff.renames:: >> -Tells Git to detect renames. If set to any boolean value, it >> -will enable basic rename detection. If set to "copies" or >> -"copy", it will detect copies, as well. >> +Whether and how Git detects renames. If set to "false", >> +rename detection is disabled. If set to "true", basic rename >> +detection is enable. If set to "copies" or "copy", Git will >> +detect copies, as well. Defaults to false. > > Just a minor typo: s/enable/enabled/ > Also, there is only one space between the second and third sentences. Indeed. Thanks. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2.1] Documentation/diff-config: fix description of diff.renames
The description was misleading, since "set to any boolean value" include "set to false", and diff.renames=false does not enable basic detection, but actually disables it. Also, document that diff.renames only affects Porcelain. Signed-off-by: Matthieu Moy --- Oops, trivial fix for typo noticed by Felipe. I'm resending just this one in case Junio wants to pick the latest version but I can obviously resend the whole if needed. Documentation/diff-config.txt | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index 6eaa452..b5e9bda 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -108,9 +108,13 @@ diff.renameLimit:: detection; equivalent to the 'git diff' option '-l'. diff.renames:: - Tells Git to detect renames. If set to any boolean value, it - will enable basic rename detection. If set to "copies" or - "copy", it will detect copies, as well. + Whether and how Git detects renames. If set to "false", + rename detection is disabled. If set to "true", basic rename + detection is enabled. If set to "copies" or "copy", Git will + detect copies, as well. Defaults to false. Note that this + affects only 'git diff' Porcelain like linkgit:git-diff[1] and + linkgit:git-log[1], and not lower level commands such as + linkgit:git-diff-files[1]. diff.suppressBlankEmpty:: A boolean to inhibit the standard behavior of printing a space -- 2.7.2.334.g35ed2ae.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings
Perhaps squash these two while queuing to address comments from you two? Thanks. Documentation/CodingGuidelines | 3 ++- Makefile | 18 +- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 1c676c2..0ddd368 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -173,7 +173,8 @@ For C programs: - As a Git developer we assume you have a reasonably modern compiler and we recommend you to enable the DEVELOPER makefile knob to - ensure your patch is clear of all compiler warnings we care about. + ensure your patch is clear of all compiler warnings we care about, + by e.g. "echo DEVELOPER=1 >>config.mak". - We try to support a wide range of C compilers to compile Git with, including old ones. That means that you should not use C99 diff --git a/Makefile b/Makefile index 9eb4032..7dc5b88 100644 --- a/Makefile +++ b/Makefile @@ -381,15 +381,15 @@ ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip ifdef DEVELOPER - CFLAGS += -Werror \ - -Wdeclaration-after-statement \ - -Wno-format-zero-length \ - -Wold-style-definition \ - -Woverflow \ - -Wpointer-arith \ - -Wstrict-prototypes \ - -Wunused \ - -Wvla +CFLAGS += -Werror \ + -Wdeclaration-after-statement \ + -Wno-format-zero-length \ + -Wold-style-definition \ + -Woverflow \ + -Wpointer-arith \ + -Wstrict-prototypes \ + -Wunused \ + -Wvla endif # Create as necessary, replace existing, make ranlib unneeded. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] Documentation/diff-config: fix description of diff.renames
Matthieu Moy writes: > The description was misleading, since "set to any boolean value" include > "set to false", and diff.renames=false does not enable basic detection, > but actually disables it. Also, document that diff.renames only affects > Porcelain. > > Signed-off-by: Matthieu Moy > --- > Documentation/diff-config.txt | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt > index 6eaa452..40e5de9 100644 > --- a/Documentation/diff-config.txt > +++ b/Documentation/diff-config.txt > @@ -108,9 +108,13 @@ diff.renameLimit:: > detection; equivalent to the 'git diff' option '-l'. > > diff.renames:: > - Tells Git to detect renames. If set to any boolean value, it > - will enable basic rename detection. If set to "copies" or > - "copy", it will detect copies, as well. > + Whether and how Git detects renames. If set to "false", > + rename detection is disabled. If set to "true", basic rename > + detection is enable. If set to "copies" or "copy", Git will Ahh, I earlier said "Not a new issue", but it is introduced here ;-) I'll patch them up. Thanks. > + detect copies, as well. Defaults to false. Note that this > + affects only 'git diff' Porcelain like linkgit:git-diff[1] and > + linkgit:git-log[1], and not lower level commands such as > + linkgit:git-diff-files[1]. > > diff.suppressBlankEmpty:: > A boolean to inhibit the standard behavior of printing a space -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/5] diff: activate diff.renames by default
Matthieu Moy writes: > diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt > index 40e5de9..69389ae 100644 > --- a/Documentation/diff-config.txt > +++ b/Documentation/diff-config.txt > @@ -111,7 +111,7 @@ diff.renames:: > Whether and how Git detects renames. If set to "false", > rename detection is disabled. If set to "true", basic rename > detection is enable. If set to "copies" or "copy", Git will Not a new issue, but s/enable/&d/, I think. > - detect copies, as well. Defaults to false. Note that this > + detect copies, as well. Defaults to true. Note that this > affects only 'git diff' Porcelain like linkgit:git-diff[1] and > linkgit:git-log[1], and not lower level commands such as > linkgit:git-diff-files[1]. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git submodule add --single-branch
I would like to do a shallow clone of a submodule like this: $ git submodule add --branch master --single-branch --depth 5 repo dir but it doesn't work. I think '--single-branch' is not yet implemented for submodules. With the clone command it works: $ git clone --branch master --single-branch --depth 5 repo dir Is it possible to do something like this with a submodule? Stefan -- To unsubscribe from this list: send the line "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: [PATCHv17 01/11] submodule-config: keep update strategy around
Stefan Beller writes:
> +int parse_submodule_update_strategy(const char *value,
> + struct submodule_update_strategy *dst)
> +{
> + free((void*)dst->command);
free((void *)dst->command);
"git tbdiff" is quite handy; it didn't spot any other lossage of
local tweak that was in 'pu', which is good.
Thanks, will replace.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git: submodule honor -c credential.* from command line
On Wed, Feb 24, 2016 at 11:11 PM, Jeff King wrote: > On Thu, Feb 25, 2016 at 02:00:36AM -0500, Jeff King wrote: > >> I think something like this would work: >> [...] >> but it does not seem to pass with your patch (even after I fixed up the >> weird "local" thing). I think the problem is that we ask >> submodule--helper to do the clone, and it uses local_repo_env. So in >> addition to your patch, you probably need a C version of the same thing >> which outputs to an argv_array. > > Something like this (which passes my test, but I didn't think hard about > it beyond that): I am having trouble getting the httpd tests to work.. The error.log generated contains the following: [Thu Feb 25 18:01:58.583832 2016] [core:crit] [pid 16376] AH00136: Server MUST relinquish startup privileges before accepting connections. Please ensure mod_unixd or other system security module is loaded. AH00016: Configuration Failed I have httpd 2.4, so I'm not sure exactly what the deal is here for these tests.. Any suggestions on what causes this? Thanks, Jake -- To unsubscribe from this list: send the line "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: [PATCHv17 05/11] run_processes_parallel: treat output of children as byte array
Stefan Beller writes:
> @@ -1135,11 +1135,11 @@ static int pp_collect_finished(struct
> parallel_processes *pp)
> strbuf_addbuf(&pp->buffered_output,
> &pp->children[i].err);
> strbuf_reset(&pp->children[i].err);
> } else {
> - fputs(pp->children[i].err.buf, stderr);
> + strbuf_write(&pp->children[i].err, stderr);
> strbuf_reset(&pp->children[i].err);
>
> /* Output all other finished child processes */
> - fputs(pp->buffered_output.buf, stderr);
> + strbuf_write(&pp->buffered_output, stderr);
> strbuf_reset(&pp->buffered_output);
>
> /*
> diff --git a/strbuf.c b/strbuf.c
> index 38686ff..71345cd 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -395,6 +395,12 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd,
> size_t hint)
> return cnt;
> }
>
> +ssize_t strbuf_write(struct strbuf *sb, FILE *f)
> +{
> + return fwrite(sb->buf, 1, sb->len, f);
> +}
Whenever I see a call to a function that takes size and nmemb
separately, I get worried about the case where nmemb is zero.
Hopefully everybody implements such a fwrite() as a no-op?
This may not matter in this patch as no caller checks the return
value from this function, but shouldn't the callers be a bit more
careful checking errors in the first place?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2.1] Documentation/diff-config: fix description of diff.renames
Matthieu Moy writes: > The description was misleading, since "set to any boolean value" include > "set to false", and diff.renames=false does not enable basic detection, > but actually disables it. Also, document that diff.renames only affects > Porcelain. > > Signed-off-by: Matthieu Moy > --- > Oops, trivial fix for typo noticed by Felipe. I'm resending just this > one in case Junio wants to pick the latest version but I can obviously > resend the whole if needed. Heh, I guess people independently spotted the same typo nearly simultaneously ;-) I think I got this fixed locally (and adjusted the last one to match). Thanks. > > Documentation/diff-config.txt | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt > index 6eaa452..b5e9bda 100644 > --- a/Documentation/diff-config.txt > +++ b/Documentation/diff-config.txt > @@ -108,9 +108,13 @@ diff.renameLimit:: > detection; equivalent to the 'git diff' option '-l'. > > diff.renames:: > - Tells Git to detect renames. If set to any boolean value, it > - will enable basic rename detection. If set to "copies" or > - "copy", it will detect copies, as well. > + Whether and how Git detects renames. If set to "false", > + rename detection is disabled. If set to "true", basic rename > + detection is enabled. If set to "copies" or "copy", Git will > + detect copies, as well. Defaults to false. Note that this > + affects only 'git diff' Porcelain like linkgit:git-diff[1] and > + linkgit:git-log[1], and not lower level commands such as > + linkgit:git-diff-files[1]. > > diff.suppressBlankEmpty:: > A boolean to inhibit the standard behavior of printing a space -- To unsubscribe from this list: send the line "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: [PATCHv17 01/11] submodule-config: keep update strategy around
On Thu, Feb 25, 2016 at 10:06 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> +int parse_submodule_update_strategy(const char *value,
>> + struct submodule_update_strategy *dst)
>> +{
>> + free((void*)dst->command);
>
> free((void *)dst->command);
>
> "git tbdiff" is quite handy; it didn't spot any other lossage of
> local tweak that was in 'pu', which is good.
>
> Thanks, will replace.
I usually use git diff .. origin/sb/feature to generate
the interdiffs,
so if you tweak things and I take these tweaks, then the interdiff is
not complete.
On the other hand if I miss the tweak, such a diff shows a revert of the tweak.
I remember applying this tweak; not sure how it got lost again.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git: submodule honor -c credential.* from command line
On Wed, Feb 24, 2016 at 11:11 PM, Jeff King wrote:
> static int clone_submodule(const char *path, const char *gitdir, const char
> *url,
>const char *depth, const char *reference, int
> quiet)
> {
> @@ -145,7 +166,7 @@ static int clone_submodule(const char *path, const char
> *gitdir, const char *url
> argv_array_push(&cp.args, path);
>
> cp.git_cmd = 1;
> - cp.env = local_repo_env;
> + add_submodule_repo_env(&cp.env_array);
> cp.no_stdin = 1;
>
> return run_command(&cp);
Ignore my previous comment. The cp.env API is *very* subtle. If the
line is just a name, it removes the environment variable, while
"name=value" adds it. That is definitely not what I was expecting
here, so I misread how it works.
I am sending a v3 with an extended test similar to the one that Jeff suggested.
Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] t/lib-http/apache.conf: load mod_unixd module in apache 2.4
From: Jacob Keller Since 2.4, apache will fail to load unless mod_unixd is loaded in order to drop privileges. Signed-off-by: Jacob Keller --- I am not sure why this wasn't there already, but I am unable to run httpd 2.4.18 without it, on Fedora 23. t/lib-httpd/apache.conf | 3 +++ 1 file changed, 3 insertions(+) diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 7d15e6d44c83..f667e7ce2f33 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -64,6 +64,9 @@ LockFile accept.lock LoadModule mpm_prefork_module modules/mod_mpm_prefork.so + + LoadModule unixd_module modules/mod_unixd.so + PassEnv GIT_VALGRIND -- 2.7.1.429.g45cd78e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] git: submodule honor -c credential.* from command line
From: Jacob Keller
Due to the way that the git-submodule code works, it clears all local
git environment variables before entering submodules. This is normally
a good thing since we want to clear settings such as GIT_WORKTREE and
other variables which would affect the operation of submodule commands.
However, GIT_CONFIG_PARAMETERS is special, and we actually do want to
preserve these settings. However, we do not want to preserve all
configuration as many things should be left specific to the parent
project.
Add a git submodule--helper function, sanitize-config, which shall be
used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
except a small subset that are known to be safe and necessary.
Replace all the calls to clear_local_git_env with a wrapped function
that filters GIT_CONFIG_PARAMETERS using the new helper and then
restores it to the filtered subset after clearing the rest of the
environment.
Signed-off-by: Jacob Keller
---
This version incorporates a bunch of suggestions from the review on v2,
including a new test using httpd to verify the credentials. In addition,
it includes a fix so that the C code in submodule--helper works as well.
Previously we used cp.env = local_repo_env to clear all the variables.
Instead, we want to clear everything except use the new filter. This is
somewhat subtle but makes sense once you read how the child_process env
code works.
Notes:
- v2
* Clarify which paramaters are left after the sanitization, and don't seem
to
indicate it is our goal to extend the list.
* add a comment in the submodule_config_ok function indicating the same
- v3
* Remove extraneous comments
* add strbuf_release calls
* rename sanitize_local_git_env
* remove use of local
* add C equivalent of sanitize_config
* add a test for the credential passing
builtin/submodule--helper.c | 86 +++-
git-submodule.sh | 36 ---
t/t5550-http-fetch-dumb.sh | 17 +
t/t7412-submodule--helper.sh | 25 +
4 files changed, 150 insertions(+), 14 deletions(-)
create mode 100755 t/t7412-submodule--helper.sh
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff179b5..4b43f5dc50e5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -124,6 +124,64 @@ static int module_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+
+/*
+ * Rules to sanitize configuration variables that are Ok to be passed into
+ * submodule operations from the parent project using "-c". Should only
+ * include keys which are both (a) safe and (b) necessary for proper
+ * operation.
+ */
+static int submodule_config_ok(const char *var)
+{
+ if (starts_with(var, "credential."))
+ return 1;
+ return 0;
+}
+
+static int sanitize_submodule_config(const char *var, const char *value, void
*data)
+{
+ struct strbuf quoted = STRBUF_INIT;
+ struct strbuf *out = data;
+
+ if (submodule_config_ok(var)) {
+ if (out->len)
+ strbuf_addch(out, ' ');
+
+ /*
+* The GIT_CONFIG_PARAMETERS parser is a bit picky and
+* requires that each var=value pair be quoted as a single
+* unit.
+*/
+ strbuf_addstr("ed, var);
+ strbuf_addch("ed, '=');
+ strbuf_addstr("ed, value);
+
+ sq_quote_buf(out, quoted.buf);
+ }
+
+ strbuf_release("ed);
+
+ return 0;
+}
+
+static void prepare_submodule_repo_env(struct argv_array *out)
+{
+ const char * const *var;
+
+ for (var = local_repo_env; *var; var++) {
+ if (!strcmp(*var, CONFIG_DATA_ENVIRONMENT)) {
+ struct strbuf sanitized_config = STRBUF_INIT;
+ git_config_from_parameters(sanitize_submodule_config,
+ &sanitized_config);
+ argv_array_pushf(out, "%s=%s", *var,
sanitized_config.buf);
+ strbuf_release(&sanitized_config);
+ } else {
+ argv_array_push(out, *var);
+ }
+ }
+
+}
+
static int clone_submodule(const char *path, const char *gitdir, const char
*url,
const char *depth, const char *reference, int quiet)
{
@@ -145,7 +203,7 @@ static int clone_submodule(const char *path, const char
*gitdir, const char *url
argv_array_push(&cp.args, path);
cp.git_cmd = 1;
- cp.env = local_repo_env;
+ prepare_submodule_repo_env(&cp.env_array);
cp.no_stdin = 1;
return run_command(&cp);
@@ -255,6 +313,31 @@ static int module_clone(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_sanitize_config(int argc, const char **argv, const char
*prefix)
+{
Re: git submodule add --single-branch
On Thu, Feb 25, 2016 at 9:54 AM, Alter Depp wrote: > I would like to do a shallow clone of a submodule like this: > $ git submodule add --branch master --single-branch --depth 5 repo dir > but it doesn't work. I think '--single-branch' is not yet implemented for > submodules. > > With the clone command it works: > $ git clone --branch master --single-branch --depth 5 repo dir > > Is it possible to do something like this with a submodule? Shallow submodules are not quite well supported. Example: $ git clone --depth 2 --recurse-submodules the depth argument applies to the main repository, and currently it is passed on to the submodules if any. The '--depth 2' suggests that you can operate on the 2 topmost revisions of the super project flawlessly, so if the submodule pointer in the superproject is advanced more than 2 commits between these 2 commits, you're missing content. So in this case you don't want to pass on the depth literally to the submodule, but rather find out which commits are referenced from the superproject and make sure those are included in the submodule. In your case of adding a submodule as a shallow repo, this problem is not there. However 'git sub add' will eventually call 'git clone' to get that submodule and '--single-branch' is implied by the existance of the '--depth' argument there. See the man page of git clone for that. So 'git submodule add --depth 5 ' should do what you'd expect. Unfortunately you cannot specify '--branch=master' though yet. Stefan > > Stefan > > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] t5313: test bounds-checks of corrupted/malicious pack/idx files
Am 25.02.2016 um 15:21 schrieb Jeff King:
> +munge () {
> + printf "$3" | dd of="$1" bs=1 conv=notrunc seek=$2
> +}
Instead of adding another call of dd, would it be an option to insert
the following patch at the front of this series and then use
test_overwrite_bytes?
8<
From: Johannes Sixt
Subject: [PATCH] tests: overwrite bytes in files using a perl script instead of
dd
The dd in my build environment on Windows crashes unpredictably. Work it
around by rewriting most instances with a helper function that uses perl
behind the scenes.
Signed-off-by: Johannes Sixt
---
t/t1060-object-corruption.sh | 2 +-
t/t5300-pack-object.sh| 8
t/t5302-pack-index.sh | 5 +++--
t/t5303-pack-corruption-resilience.sh | 2 +-
t/test-lib-functions.sh | 16
5 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index 3f87051..e3c5de8 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -12,7 +12,7 @@ obj_to_file() {
corrupt_byte() {
obj_file=$(obj_to_file "$1") &&
chmod +w "$obj_file" &&
- printf '\0' | dd of="$obj_file" bs=1 seek="$2" conv=notrunc
+ printf '\0' | test_overwrite_bytes "$obj_file" "$2"
}
test_expect_success 'setup corrupt repo' '
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index fc2be63..f45a101 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -226,7 +226,7 @@ test_expect_success \
test_expect_success \
'verify-pack catches a corrupted pack signature' \
'cat test-1-${packname_1}.pack >test-3.pack &&
- echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=2 &&
+ echo | test_overwrite_bytes test-3.pack 2 &&
if git verify-pack test-3.idx
then false
else :;
@@ -235,7 +235,7 @@ test_expect_success \
test_expect_success \
'verify-pack catches a corrupted pack version' \
'cat test-1-${packname_1}.pack >test-3.pack &&
- echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=7 &&
+ echo | test_overwrite_bytes test-3.pack 7 &&
if git verify-pack test-3.idx
then false
else :;
@@ -244,7 +244,7 @@ test_expect_success \
test_expect_success \
'verify-pack catches a corrupted type/size of the 1st packed object data' \
'cat test-1-${packname_1}.pack >test-3.pack &&
- echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=12 &&
+ echo | test_overwrite_bytes test-3.pack 12 &&
if git verify-pack test-3.idx
then false
else :;
@@ -255,7 +255,7 @@ test_expect_success \
'l=$(wc -c test-3.pack &&
- printf "%20s" "" | dd of=test-3.idx count=20 bs=1 conv=notrunc seek=$l &&
+ printf "%20s" "" | test_overwrite_bytes test-3.idx $l &&
if git verify-pack test-3.pack
then false
else :;
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index c2fc584..5a82f19 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -225,8 +225,9 @@ test_expect_success \
obj=$(git hash-object file_001) &&
nr=$(index_obj_nr ".git/objects/pack/pack-${pack1}.idx" $obj) &&
chmod +w ".git/objects/pack/pack-${pack1}.idx" &&
- printf | dd of=".git/objects/pack/pack-${pack1}.idx" conv=notrunc \
-bs=1 count=4 seek=$((8 + 256 * 4 + $(wc -l /dev/null || exit 1
done ;
+ open my $fh, "+<", $fname or die "open $fname: $!\n";
+ seek($fh, $offset, 0) or die "seek $fname: $!\n";
+ syswrite($fh, $bytes) or die "write $fname: $!\n";
+ close $fh or die "close $fname: $!\n";
+ ' "$@"
+}
+
# The following mingw_* functions obey POSIX shell syntax, but are actually
# bash scripts, and are meant to be used only with bash on Windows.
--
2.7.0.118.g90056ae
--
To unsubscribe from this list: send the line "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 v6 00/32] refs backend
On Thu, 2016-02-25 at 19:57 +0700, Duy Nguyen wrote: > A couple of build warnings I found, haven't really read the code yet. > These two can easily be fixed > > refs/lmdb-backend.c: In function 'lmdb_read_raw_ref': > refs/lmdb-backend.c:554:16: warning: unused variable 'err' [-Wunused > -variable] > struct strbuf err = STRBUF_INIT; > ^ > refs/lmdb-backend.c: In function 'lmdb_do_for_each_ref': > refs/lmdb-backend.c:1625:15: warning: unused variable 'c' [-Wunused > -variable] >const char *c = resolve_ref_unsafe_submodule(submodule, refname, > 0, oid.hash, >^ > > -Wshadow also gives a bunch of warnings, mostly about "transaction" > and "env". Whether you want to fix them is really up to you. > -- > Duy Will fix these, thanks. I've now configured my config.mak to do -Wall again, but -Wshadow produces a ton of complaints on the rest of git's code. We should probably fix those. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] refs: mark a file-local symbol as static
Signed-off-by: Ramsay Jones
---
Hi David,
No, you are not having flashbacks - you forgot to make the
register_ref_storage_backend() function static. ;-)
BTW, I still have two symbols showing as exported but not used, namely:
$ diff nsc psc1
...
32a35,36
> refs.o- resolve_ref_unsafe_submodule
> refs/files-backend.o - do_for_each_per_worktree_ref
$
Both of these symbols are used by the lmdb backend, so I'm assuming that
they are part of the 'refs API' and will (may?) be used by other alternate
reference backends. Is that the case?
(I don't have the time right now to look at the code!)
Thanks!
ATB,
Ramsay Jones
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 1c1d3ac..e448434 100644
--- a/refs.c
+++ b/refs.c
@@ -46,7 +46,7 @@ int ref_storage_backend_exists(const char *name)
return 0;
}
-void register_ref_storage_backend(struct ref_storage_be *be)
+static void register_ref_storage_backend(struct ref_storage_be *be)
{
be->next = refs_backends;
refs_backends = be;
--
2.7.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
Re: [PATCH v2 4/5] log: introduce init_log_defaults()
Matthieu Moy writes: > This is currently a wrapper around init_grep_defaults(), but will allow > adding more initialization in further patches. > > Signed-off-by: Matthieu Moy > --- > builtin/log.c | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/builtin/log.c b/builtin/log.c > index 0d738d6..7f96c64 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -100,6 +100,11 @@ static int log_line_range_callback(const struct option > *option, const char *arg, > return 0; > } > > +static void init_log_defaults() static void init_log_defaults(void). Locally patched up already; no need to resend. 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 v6 00/32] refs backend
On Thu, 2016-02-25 at 00:08 +, Ramsay Jones wrote: > > Ramsay Jones (1): > > refs: reduce the visibility of do_for_each_ref() > > Ah, sorry if it wasn't clear, but there is no need to keep this > as a separate patch - it should simply be squashed into the relevant > patch in your series. > > ATB, > Ramsay Jones It actually seemed cleaner to do it this way for some reason. -- To unsubscribe from this list: send the line "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 v6 00/32] refs backend
On Thu, Feb 25, 2016 at 11:27 AM, David Turner wrote: > On Thu, 2016-02-25 at 19:57 +0700, Duy Nguyen wrote: >> A couple of build warnings I found, haven't really read the code yet. >> These two can easily be fixed >> >> refs/lmdb-backend.c: In function 'lmdb_read_raw_ref': >> refs/lmdb-backend.c:554:16: warning: unused variable 'err' [-Wunused >> -variable] >> struct strbuf err = STRBUF_INIT; >> ^ >> refs/lmdb-backend.c: In function 'lmdb_do_for_each_ref': >> refs/lmdb-backend.c:1625:15: warning: unused variable 'c' [-Wunused >> -variable] >>const char *c = resolve_ref_unsafe_submodule(submodule, refname, >> 0, oid.hash, >>^ >> >> -Wshadow also gives a bunch of warnings, mostly about "transaction" >> and "env". Whether you want to fix them is really up to you. >> -- >> Duy > > Will fix these, thanks. I've now configured my config.mak to do -Wall > again, but -Wshadow produces a ton of complaints on the rest of git's > code. We should probably fix those. It is a proposal for the GSoC microprojects to fix all -Wshadow problems in say one file. Maybe we'll see some activity there :) -- To unsubscribe from this list: send the line "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] run-command: fix an 'different modifiers' sparse warning
Signed-off-by: Ramsay Jones
---
Hi Jeff,
If you need to re-roll your 'jk/epipe-in-async' branch, could you
please squash this into the relevant patch. (ie. "write_or_die:
handle EPIPE in async threads", 24-02-2016).
Thanks!
ATB,
Ramsay Jones
run-command.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/run-command.c b/run-command.c
index cd861bc..5dec18b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -625,7 +625,7 @@ int in_async(void)
return !pthread_equal(main_thread, pthread_self());
}
-void async_exit(int code)
+void NORETURN async_exit(int code)
{
pthread_exit((void *)(intptr_t)code);
}
@@ -675,7 +675,7 @@ int in_async(void)
return process_is_async;
}
-int async_exit(int code)
+int NORETURN async_exit(int code)
{
exit(code);
}
--
2.7.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] submodule--helper: fix 'dubious bitfield' sparse warnings
Signed-off-by: Ramsay Jones
---
Hi Stefan,
If you need to re-roll (err, one of your) 'sb/submodule-parallel-update'
branches, could you please squash this into the relevant patch.
(ie. "git submodule update: have a dedicated helper for cloning", 23-02-2016).
Thanks!
ATB,
Ramsay Jones
builtin/submodule--helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f8cdce9..482a3cc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -563,7 +563,7 @@ struct submodule_update_clone {
/* index into 'list', the list of submodules to look into for cloning */
int current;
struct module_list list;
- int warn_if_uninitialized : 1;
+ unsigned int warn_if_uninitialized : 1;
/* update parameter passed via commandline*/
struct submodule_update_strategy update;
/* configuration parameters which are passed on to the children */
@@ -575,7 +575,7 @@ struct submodule_update_clone {
/* lines to be output */
struct string_list projectlines;
/* If we want to stop as fast as possible and return an error */
- int quickstop : 1;
+ unsigned int quickstop : 1;
};
#define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
--
2.7.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] refs: document transaction semantics
Add some comments on ref transaction semantics to refs.h Signed-off-by: David Turner --- refs.h | 12 1 file changed, 12 insertions(+) diff --git a/refs.h b/refs.h index c0964f5..9b3eaf3 100644 --- a/refs.h +++ b/refs.h @@ -112,6 +112,11 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref); * If this succeeds, the ref updates will have taken place and * the transaction cannot be rolled back. * + * - Instead of `ref_transaction_commit`, use + * `initial_ref_transaction_commit()` if the ref database is known + * to be empty (e.g. during clone). This is likely to be much + * faster. + * * - 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 @@ -127,6 +132,13 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref); * * The message is appended to err without first clearing err. * err will not be '\n' terminated. + * + * Caveats + * --- + * + * Note that no locks are taken, and no refs are read, until + * `ref_transaction_commit` is called. So `ref_transaction_verify` + * won't report a verification failure until the commit is attempted. */ struct ref_transaction; -- 2.4.2.767.g62658d5-twtrsrc -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs: mark a file-local symbol as static
On Thu, 2016-02-25 at 19:28 +, Ramsay Jones wrote: > Signed-off-by: Ramsay Jones > --- > > Hi David, > > No, you are not having flashbacks - you forgot to make the > register_ref_storage_backend() function static. ;-) Oops! Must have lost that in all of the rebasing! Will fix, thanks. > BTW, I still have two symbols showing as exported but not used, > namely: > > $ diff nsc psc1 > ... > 32a35,36 > > refs.o - resolve_ref_unsafe_submodule > > refs/files-backend.o- do_for_each_per_worktree_ref > $ > > Both of these symbols are used by the lmdb backend, so I'm assuming > that > they are part of the 'refs API' and will (may?) be used by other > alternate > reference backends. Is that the case? Yes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs: document transaction semantics
I thought it would be better in the api docs, since it's a design decision that all backends should follow. On Thu, 2016-02-25 at 15:05 -0500, David Turner wrote: > Add some comments on ref transaction semantics to refs.h > > Signed-off-by: David Turner > --- > refs.h | 12 > 1 file changed, 12 insertions(+) > > diff --git a/refs.h b/refs.h > index c0964f5..9b3eaf3 100644 > --- a/refs.h > +++ b/refs.h > @@ -112,6 +112,11 @@ extern int dwim_log(const char *str, int len, > unsigned char *sha1, char **ref); > * If this succeeds, the ref updates will have taken place and > * the transaction cannot be rolled back. > * > + * - Instead of `ref_transaction_commit`, use > + * `initial_ref_transaction_commit()` if the ref database is known > + * to be empty (e.g. during clone). This is likely to be much > + * faster. > + * > * - 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 > @@ -127,6 +132,13 @@ extern int dwim_log(const char *str, int len, > unsigned char *sha1, char **ref); > * > * The message is appended to err without first clearing err. > * err will not be '\n' terminated. > + * > + * Caveats > + * --- > + * > + * Note that no locks are taken, and no refs are read, until > + * `ref_transaction_commit` is called. So `ref_transaction_verify` > + * won't report a verification failure until the commit is > attempted. > */ > struct ref_transaction; > -- To unsubscribe from this list: send the line "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 v6 03/32] files-backend: break out ref reading
On Wed, 2016-02-24 at 16:51 -0800, Junio C Hamano wrote: > Junio C Hamano writes: > > > ... > > You can of course standardize on signed int, but because this is a > > collection of flag bits, there is no reason not to choose unsigned. > > > > I _think_ I can fix everything up before pushing out, so please > > check what will appear on 'pu' before rerolling. > > I managed to whip them into shape enough to pass my compilation test > ;-) Other things I had to tweak were small things like whitespace > issues, initializing statics to 0 or NULL, etc. Got it, thanks. Is there some standard workflow here? I just rebased your version of my series (bd412fa) on top of the prior commit in pu (9db66d9), fixing the few conflicts. (I think there is in general something I'm missing about how to maintain a patch set under the git.git workflow). -- To unsubscribe from this list: send the line "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] run-command: fix an 'different modifiers' sparse warning
Ramsay Jones writes:
> Signed-off-by: Ramsay Jones
> ---
>
> Hi Jeff,
>
> If you need to re-roll your 'jk/epipe-in-async' branch, could you
> please squash this into the relevant patch. (ie. "write_or_die:
> handle EPIPE in async threads", 24-02-2016).
>
> Thanks!
I actually was planning to merge this to 'next' today, so I'll
squash it in without waiting for a reroll.
By the way, doesn't it bother anybody to give two different types to
the same function depending on NO_PTHREAD? It is not a new issue
added by this series, but async_exit() that claims to return int
does not (naturally) return anything, and sparse does not seem to
care (neither do we).
>
> ATB,
> Ramsay Jones
>
> run-command.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index cd861bc..5dec18b 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -625,7 +625,7 @@ int in_async(void)
> return !pthread_equal(main_thread, pthread_self());
> }
>
> -void async_exit(int code)
> +void NORETURN async_exit(int code)
> {
> pthread_exit((void *)(intptr_t)code);
> }
> @@ -675,7 +675,7 @@ int in_async(void)
> return process_is_async;
> }
>
> -int async_exit(int code)
> +int NORETURN async_exit(int code)
> {
> exit(code);
> }
--
To unsubscribe from this list: send the line "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 v6 03/32] files-backend: break out ref reading
David Turner writes: > Is there some standard workflow here? I just rebased your version of > my series (bd412fa) on top of the prior commit in pu (9db66d9), fixing > the few conflicts. (I think there is in general something I'm missing > about how to maintain a patch set under the git.git workflow). We usually do not see multiple large topics stomping on each other's toes, so it would be fair to say that we are still learning to find some standard workflow. The topic overlaps with what Stefan's submodule-parallel-update topic touches, and both topics are constantly rerolled, so I've been doing: * Pick a commit on 'master' as a stable base, and do not change this unless absolutely necessary. * Merge the tip of Stefan's series to it. * Apply your series on top (if you didn't rebase, but Stefan rerolled, this will be cherry-pick of your latest round). to rebuild dt/refs-backend-lmdb topic every time either one of you rerolled. -- To unsubscribe from this list: send the line "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] submodule--helper: fix 'dubious bitfield' sparse warnings
On Thu, Feb 25, 2016 at 11:42 AM, Ramsay Jones
wrote:
>
> Signed-off-by: Ramsay Jones
> ---
>
> Hi Stefan,
>
> If you need to re-roll (err, one of your) 'sb/submodule-parallel-update'
> branches, could you please squash this into the relevant patch.
> (ie. "git submodule update: have a dedicated helper for cloning", 23-02-2016).
>
> Thanks!
It's part of v17 already.
Thanks,
Stefan
>
> ATB,
> Ramsay Jones
>
> builtin/submodule--helper.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f8cdce9..482a3cc 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -563,7 +563,7 @@ struct submodule_update_clone {
> /* index into 'list', the list of submodules to look into for cloning
> */
> int current;
> struct module_list list;
> - int warn_if_uninitialized : 1;
> + unsigned int warn_if_uninitialized : 1;
> /* update parameter passed via commandline*/
> struct submodule_update_strategy update;
> /* configuration parameters which are passed on to the children */
> @@ -575,7 +575,7 @@ struct submodule_update_clone {
> /* lines to be output */
> struct string_list projectlines;
> /* If we want to stop as fast as possible and return an error */
> - int quickstop : 1;
> + unsigned int quickstop : 1;
> };
> #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
> SUBMODULE_UPDATE_STRATEGY_INIT, 0, NULL, NULL, NULL, NULL, \
> --
> 2.7.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
--
To unsubscribe from this list: send the line "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 v6 00/32] refs backend
On Fri, Feb 26, 2016 at 2:27 AM, David Turner wrote: > but -Wshadow produces a ton of complaints on the rest of git's > code. We should probably fix those. I know, but what I wrote was meant for new code only (e.g. make refs/lmdb-backend.o CFLAGS=-Wshadow is clean). I think renaming the global env and transaction to the_env and the_transaction probably does the trick. Again, this is controversial whether it's a good thing to do. So your choice. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] t5313: test bounds-checks of corrupted/malicious pack/idx files
Johannes Sixt writes:
> Am 25.02.2016 um 15:21 schrieb Jeff King:
>> +munge () {
>> +printf "$3" | dd of="$1" bs=1 conv=notrunc seek=$2
>> +}
>
> Instead of adding another call of dd, would it be an option to insert
> the following patch at the front of this series and then use
> test_overwrite_bytes?
It would be an option, but I'd want to merge this to 'next' today
without waiting for a reroll. Change from dd to custom script can
be done as a follow-up topic after the dust settles, if necessary.
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 v2] git: submodule honor -c credential.* from command line
Jacob Keller writes:
> On Wed, Feb 24, 2016 at 11:11 PM, Jeff King wrote:
>> static int clone_submodule(const char *path, const char *gitdir, const char
>> *url,
>>const char *depth, const char *reference, int
>> quiet)
>> {
>> @@ -145,7 +166,7 @@ static int clone_submodule(const char *path, const char
>> *gitdir, const char *url
>> argv_array_push(&cp.args, path);
>>
>> cp.git_cmd = 1;
>> - cp.env = local_repo_env;
>> + add_submodule_repo_env(&cp.env_array);
>> cp.no_stdin = 1;
>>
>> return run_command(&cp);
>
>
> Ignore my previous comment. The cp.env API is *very* subtle. If the
> line is just a name, it removes the environment variable, while
> "name=value" adds it. That is definitely not what I was expecting
> here, so I misread how it works.
I think that is modelled after how putenv(3) is made capable of
removing an existing environment variable.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs: document transaction semantics
David Turner writes: > I thought it would be better in the api docs, since it's a design > decision that all backends should follow. Makes sense; as this describes an already available API, it shouldn't have to wait for the remainder of your series and can just go to 'master' (or even to 'maint' if we wanted to), right? > On Thu, 2016-02-25 at 15:05 -0500, David Turner wrote: >> Add some comments on ref transaction semantics to refs.h >> >> Signed-off-by: David Turner >> --- >> refs.h | 12 >> 1 file changed, 12 insertions(+) >> >> diff --git a/refs.h b/refs.h >> index c0964f5..9b3eaf3 100644 >> --- a/refs.h >> +++ b/refs.h >> @@ -112,6 +112,11 @@ extern int dwim_log(const char *str, int len, >> unsigned char *sha1, char **ref); >> * If this succeeds, the ref updates will have taken place and >> * the transaction cannot be rolled back. >> * >> + * - Instead of `ref_transaction_commit`, use >> + * `initial_ref_transaction_commit()` if the ref database is known >> + * to be empty (e.g. during clone). This is likely to be much >> + * faster. >> + * >> * - 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 >> @@ -127,6 +132,13 @@ extern int dwim_log(const char *str, int len, >> unsigned char *sha1, char **ref); >> * >> * The message is appended to err without first clearing err. >> * err will not be '\n' terminated. >> + * >> + * Caveats >> + * --- >> + * >> + * Note that no locks are taken, and no refs are read, until >> + * `ref_transaction_commit` is called. So `ref_transaction_verify` >> + * won't report a verification failure until the commit is >> attempted. >> */ >> struct ref_transaction; >> -- To unsubscribe from this list: send the line "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: [PATCHv17 05/11] run_processes_parallel: treat output of children as byte array
On Thu, Feb 25, 2016 at 10:16 AM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> @@ -1135,11 +1135,11 @@ static int pp_collect_finished(struct
>> parallel_processes *pp)
>> strbuf_addbuf(&pp->buffered_output,
>> &pp->children[i].err);
>> strbuf_reset(&pp->children[i].err);
>> } else {
>> - fputs(pp->children[i].err.buf, stderr);
>> + strbuf_write(&pp->children[i].err, stderr);
>> strbuf_reset(&pp->children[i].err);
>>
>> /* Output all other finished child processes */
>> - fputs(pp->buffered_output.buf, stderr);
>> + strbuf_write(&pp->buffered_output, stderr);
>> strbuf_reset(&pp->buffered_output);
>>
>> /*
>> diff --git a/strbuf.c b/strbuf.c
>> index 38686ff..71345cd 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -395,6 +395,12 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd,
>> size_t hint)
>> return cnt;
>> }
>>
>> +ssize_t strbuf_write(struct strbuf *sb, FILE *f)
>> +{
>> + return fwrite(sb->buf, 1, sb->len, f);
>> +}
>
> Whenever I see a call to a function that takes size and nmemb
> separately, I get worried about the case where nmemb is zero.
> Hopefully everybody implements such a fwrite() as a no-op?
>
> This may not matter in this patch as no caller checks the return
> value from this function, but shouldn't the callers be a bit more
> careful checking errors in the first place?
If there is no other comment on the series, I plan on sending a patch
to fix this up afterwards to not further collide with the refs backend series.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
