Re: [PATCHv4] sha1_file: fix iterating loose alternate objects
On Mon, Feb 02, 2015 at 12:00:17PM -0800, Junio C Hamano wrote: Jonathon Mah m...@jonathonmah.com writes: +test_expect_success 'prune: handle alternate object database' ' + test_create_repo A + (cd A + echo Hello World file1 + git add file1 + git commit -m Initial commit file1) + git clone -s A B + (cd B + echo foo bar file2 + git add file2 + git commit -m next commit file2 + git prune) +' The issue does not have much to do with introducing new path to the cloned repository, or the original having any specific content for that matter, so I am tempted to simplify the above to something like this intead: test_create_repo A git -C A commit --allow-empty -m initial commit git clone --shared A B git -C B commit --allow-empty -m next commit git -C B prune Yeah, I'd agree that more clearly demonstrates the issue (I didn't check that it actually triggers the failure, but presumably you did). I think we could also construct a more elaborate example where we fail to pick up an unreachable segment of history based on the mtime of a tip commit found only in the alternate (whereas this is only testing that we don't bungle the alternate filename so completely that prune barfs). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFD: should we do another 2.3-rc for t9001-noxmailer? I'd say not
Jeff King p...@peff.net writes: I thought at first that we also had a regression in pruning with alternates, but it looks like that bug actually went into v2.2. I still think we would want the fix fairly promptly, but it does not need to happen before v2.3 is released. Yes, this was regression in v2.2 we did not catch X-. The fix looks so obvious that it appears nothing should break, but that tends to be the famous last words, so... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-submodules.sh: fix '/././' path normalization
Am 30.01.2015 um 16:14 schrieb Patrick Steinhardt: When we add a new submodule the path of the submodule is being normalized. We fail to normalize multiple adjacent '/./', though. Thus 'path/to/././submodule' will become 'path/to/./submodule' where it should be 'path/to/submodule' instead. Thanks, nicely done and fixes the issue you noticed: Ack from me. Signed-off-by: Patrick Steinhardt p...@pks.im --- git-submodule.sh | 2 +- t/t7400-submodule-basic.sh | 17 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/git-submodule.sh b/git-submodule.sh index 9245abf..36797c3 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -423,7 +423,7 @@ cmd_add() sed -e ' s|//*|/|g s|^\(\./\)*|| - s|/\./|/|g + s|/\(\./\)*|/|g :start s|\([^/]*\)/\.\./|| tstart diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 7c88245..5811a98 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -171,6 +171,23 @@ test_expect_success 'submodule add with ./ in path' ' test_cmp empty untracked ' +test_expect_success 'submodule add with /././ in path' ' + echo refs/heads/master expect + empty + + ( + cd addtest + git submodule add $submodurl dotslashdotsubmod/././frotz/./ + git submodule init + ) + + rm -f heads head untracked + inspect addtest/dotslashdotsubmod/frotz ../../.. + test_cmp expect heads + test_cmp expect head + test_cmp empty untracked +' + test_expect_success 'submodule add with // in path' ' echo refs/heads/master expect empty -- To unsubscribe from this list: send the line 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] t9001: use older Getopt::Long boolean prefix '--no' rather than '--no-'
Kyle J. McKay mack...@gmail.com writes: On Feb 1, 2015, at 17:33, Junio C Hamano wrote: Kyle J. McKay mack...@gmail.com writes: use 5.008; So either that needs to change or the code should properly deal with the version of Getopt::Long that comes with 5.8.0. Since it's really not very difficult or invasive to add support for the no- variants, here's a patch to do so: Doesn't that approach add what does --no-no-chain-rely-to even mean? confusion to the resulting system? If that is not the case, then I am all for it, but otherwise, let's not. No. You have to append the '!' to get the automagic no prefix alternative(s), so while 'chain-reply-to!' means support chain-reply- to, nochain-reply-to and (if you have a new enough Getopt::Long) no- chain-reply-to, just using 'no-chain-reply-to' without the trailing !' means that nono-chain-reply-to and no-no-chain-reply-to remain invalid options that will generate an error. Ahh, I missed that ! suffix (or lack thereof). 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: RFD: should we do another 2.3-rc for t9001-noxmailer? I'd say not
On Sun, Feb 01, 2015 at 02:48:00PM -0800, Junio C Hamano wrote: I was reviewing the recent bugs and fixes for the last time, and was wondering if we want to do 2.3-rc3 with build fix for those with ancient cURL (tc/curl-vernum-output-broken-in-7.11) and workaround for those with Perl with older Getopt::Long (tc/t9001-noxmailer). - The former is not a regression between 2.2 and 2.3 (i.e. 2.2 already had the same use of curl-config output). - The latter, strictly speaking, is a regression in that tests used to pass but tests in 2.3 no longer pass for those with older Getopt::Long. But the latter is about a test script that lacks work-around, and more importantly, everybody has lived with unconditional X-mailer: output, and the minority with ancient Getopt::Long will survive without being to able to give the new --no-xmailer (or --noxmailer) option just fine. So currently I am leaning to keep these two fixes where they are and tag 2.3 final without them in a few days. Yeah, I think that is sensible, especially given that the ancient --noxmailer platform reportedly cannot even fully build with v2.2. I thought at first that we also had a regression in pruning with alternates, but it looks like that bug actually went into v2.2. I still think we would want the fix fairly promptly, but it does not need to happen before v2.3 is released. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4] sha1_file: fix iterating loose alternate objects
Jeff King p...@peff.net writes: On Mon, Feb 02, 2015 at 10:48:12AM -0800, Jonathon Mah wrote: The string in 'base' contains a path suffix to a specific object; when its value is used, the suffix must either be filled (as in stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared (as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb (introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and treated 'base' as a complete path to the base object directory, instead of a pointer to the base of the full path string. The trailing path after 'base' is still initialized to NUL, hiding the bug in some common cases. Additionally the descendent for_each_file_in_obj_subdir function swallows ENOENT, so an error only shows if the alternate's path was last filled with a valid object (where statting /path/to/existing/00/0bjectfile/00 fails). Signed-off-by: Jonathon Mah m...@jonathonmah.com --- Squashed test and fix. Thanks, this version looks good to me. Thanks, both of you. The analysis, the fix and the test all look reasonable. -- To unsubscribe from this list: send the line 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: [PATCHv4] sha1_file: fix iterating loose alternate objects
Jonathon Mah m...@jonathonmah.com writes: +test_expect_success 'prune: handle alternate object database' ' + test_create_repo A + (cd A + echo Hello World file1 + git add file1 + git commit -m Initial commit file1) + git clone -s A B + (cd B + echo foo bar file2 + git add file2 + git commit -m next commit file2 + git prune) +' The issue does not have much to do with introducing new path to the cloned repository, or the original having any specific content for that matter, so I am tempted to simplify the above to something like this intead: test_create_repo A git -C A commit --allow-empty -m initial commit git clone --shared A B git -C B commit --allow-empty -m next commit git -C B prune 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: [GIT PULL] l10n updates for 2.3.0
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: [PATCHv5] sha1_file: fix iterating loose alternate objects
On Mon, Feb 02, 2015 at 12:05:54PM -0800, Jonathon Mah wrote: Simplified test per Junio (verified that it fails before and passes now). Punting on Jeff's more elaborate example. I think that's fine. I started to try to create such an example, but it's actually rather tricky. If the alternate has the tip object, then one of these must be true: 1. It has all of the objects the tip depends on. 2. It is missing an object, and this tip is part of the referenced history. 3. It is missing an object, but this part of history is not referenced. In case (1), we do not care about deleting objects from the base repository; we already have another copy in the alternate. In case (2), the alternate is corrupt, and all bets are off. In case (3), we can only have dropped the object from the alternate by pruning it and keeping the tip object that refers to it. Which is the exact thing that this new code was added to avoid (to always keep depended-upon objects). So I actually do not see how the situation would come up in practice, and possibly we could drop the iteration of the alternates' loose objects entirely from this code. But certainly that is orthogonal to Jonathon's fix (which is a true regression for the less-exotic case that his test demonstrates). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link
On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano gits...@pobox.com wrote: + test_must_fail git apply --index patch + +' Is the empty line between the last test_must_fail and the closing `'` intentional? -- To unsubscribe from this list: send the line 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/4] apply: reject input that touches outside $cwd
On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote: By default, a patch that affects outside the working area is rejected as a mistake (or a mischief); Git itself does not create such a patch, unless the user bends backwards and specifies a non-standard prefix to git diff and friends. When `git apply` is used without either `--index` or `--cached` option as a better GNU patch, the user can pass `--unsafe-paths` option to override this safety check. This cannot be used to escape outside the working tree when using `--index` or `--cached` to apply the patch to the index. The new test was stolen from Jeff King with slight enhancements. I notice that this includes the symlink-crossing tests, marked as failures. Reading the series, I know what is going to happen later, but do you want to leave a note like: Note that we also add tests for leaving the working directory by crossing symlink boundaries, which is not addressed in this patch. That is a separate issue caused following symlinks, which will come later. or something to help later readers of git log? +--unsafe-paths:: + By default, a patch that affects outside the working area is + rejected as a mistake (or a mischief); Git itself never + creates such a patch unless the user bends backwards and + specifies nonstandard prefix to git diff and friends. Minor wordsmithing: the usual idiom is bend over backwards, and you probably want s/specifies/ a/. ++ +When `git apply` is used without either `--index` or `--cached` +option as a better GNU patch, the user can pass `--unsafe-paths` +option to override this safety check. Similarly, probably every instance of foo option would read better as the foo option. This cannot be used to escape +outside the working tree when using `--index` or `--cached` to apply +the patch to the index. I had trouble figuring out what this meant. Would it be simpler to just say: This option has no effect when `--index` or `--cached` is in use. Or is there some other subtlety that you are trying to convey that I am missing? -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 v2 4/4] apply: do not touch a file beyond a symbolic link
Because Git tracks symbolic links as symbolic links, a path that
has a symbolic link in its leading part (e.g. path/to/dir/file,
where path/to/dir is a symbolic link to somewhere else, be it
inside or outside the working tree) can never appear in a patch
that validly applies, unless the same patch first removes the
symbolic link to allow a directory to be created there.
Detect and reject such a patch.
Things to note:
- Unfortunately, we cannot reuse the has_symlink_leading_path()
from dir.c, as that is only about the working tree, but git
apply can be told to apply the patch only to the index or to
both the index and to the working tree.
- We cannot directly use has_symlink_leading_path() even when we
are applying only to the working tree, as an early patch of a
valid input may remove a symbolic link path/to/dir and then a
later patch of the input may create a path path/to/dir/file, but
git apply first checks the input without touching either the
index or the working tree. The leading symbolic link check must
be done on the interim result we compute in-core (i.e. after the
first patch, there is no path/to/dir symbolic link and it is
perfectly valid to create path/to/dir/file).
Similarly, when an input creates a symbolic link path/to/dir and
then creates a file path/to/dir/file, we need to flag it as an
error without actually creating path/to/dir symbolic link in the
filesystem.
Instead, for any patch in the input that leaves a path (i.e. a non
deletion) in the result, we check all leading paths against the
resulting tree that the patch would create by inspecting all the
patches in the input and then the target of patch application
(either the index or the working tree).
This way, we catch a mischief or a mistake to add a symbolic link
path/to/dir and a file path/to/dir/file at the same time, while
allowing a valid patch that removes a symbolic link path/to/dir and
then adds a file path/to/dir/file.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/apply.c | 109
t/t4122-apply-symlink-inside.sh | 70 ++
t/t4139-apply-escape.sh | 6 +--
3 files changed, 182 insertions(+), 3 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 60d821c..a760d97 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3486,6 +3486,103 @@ static int check_to_create(const char *new_name, int
ok_if_exists)
return 0;
}
+/*
+ * We need to keep track of how symlinks in the preimage are
+ * manipulated by the patches. A patch to add a/b/c where a/b
+ * is a symlink should not be allowed to affect the directory
+ * the symlink points at, but if the same patch removes a/b,
+ * it is perfectly fine, as the patch removes a/b to make room
+ * to create a directory a/b so that a/b/c can be created.
+ */
+static struct string_list symlink_changes;
+#define SYMLINK_GOES_AWAY 01
+#define SYMLINK_IN_RESULT 02
+
+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
+{
+ struct string_list_item *ent;
+
+ ent = string_list_lookup(symlink_changes, path);
+ if (!ent) {
+ ent = string_list_insert(symlink_changes, path);
+ ent-util = (void *)0;
+ }
+ ent-util = (void *)(what | ((uintptr_t)ent-util));
+ return (uintptr_t)ent-util;
+}
+
+static uintptr_t check_symlink_changes(const char *path)
+{
+ struct string_list_item *ent;
+
+ ent = string_list_lookup(symlink_changes, path);
+ if (!ent)
+ return 0;
+ return (uintptr_t)ent-util;
+}
+
+static void prepare_symlink_changes(struct patch *patch)
+{
+ for ( ; patch; patch = patch-next) {
+ if ((patch-old_name S_ISLNK(patch-old_mode))
+ (patch-is_rename || patch-is_delete))
+ /* the symlink at patch-old_name is removed */
+ register_symlink_changes(patch-old_name,
SYMLINK_GOES_AWAY);
+
+ if (patch-new_name S_ISLNK(patch-new_mode))
+ /* the symlink at patch-new_name is created or remains
*/
+ register_symlink_changes(patch-new_name,
SYMLINK_IN_RESULT);
+ }
+}
+
+static int path_is_beyond_symlink_1(struct strbuf *name)
+{
+ do {
+ unsigned int change;
+
+ while (--name-len name-buf[name-len] != '/')
+ ; /* scan backwards */
+ if (!name-len)
+ break;
+ name-buf[name-len] = '\0';
+ change = check_symlink_changes(name-buf);
+ if (change SYMLINK_IN_RESULT)
+ return 1;
+ if (change SYMLINK_GOES_AWAY)
+ /*
+* This cannot be return 0, because we may
+* see a new one created at a higher level.
+*/
+
[PATCH v2 0/4] git apply safety
git apply have been fairly careless about letting the input follow symbolic links, especially when used without the --index/--cached options (which was more or less deliberate to mimic what patch used to do). When the input tells it to modify a/b/c, and lstat(2) said that there is a/b/c that matches the preimage in the input, we happily overwrote it, even when a/b is a symbolic link that pointed somewhere, even outside the working tree. This series tightens things a bit for safety. (1) By default, we reject patches to .git/file, ../some/where, ./this/././that, etc., i.e. the names you cannot add to the index. Those who use git apply (without --index/--cached) as a replacement for GNU patch can use --unsafe-paths option to override this safety. This is what patch 1/4 does. (2) We do not allow a patch to depend on a location beyond a symbolic link (this includes a patch to remove a path beyond a symbolic link). This is patch 2/4 and 3/4. (3) We do not allow a patch to create result on a location beyond a symbolic link. This is patch 4/4. There is no knob to override the latter two points, as this is not a safety but is a correctness issue. Because Git keeps track of and can express changes to symbolic links, a patch that expects a file a/b/c to be tracked (either the patch adds it, or it modifies an existing file tehre) implicitly expects that there is no symbolic link a/b, so attempting to apply such a patch to a tree with a symbolic link at a/b, even when the link points at some directory, must detect that the target tree does not match what the patch's preimage expects and fail. The previous attempt begins at around here: http://thread.gmane.org/gmane.linux.kernel/1874498/focus=1878385 Junio C Hamano (4): apply: reject input that touches outside $cwd apply: do not read from the filesystem under --index apply: do not read from beyond a symbolic link apply: do not touch a file beyond a symbolic link Documentation/git-apply.txt | 14 +++- builtin/apply.c | 139 +++- t/t4122-apply-symlink-inside.sh | 89 + t/t4139-apply-escape.sh | 137 +++ 4 files changed, 377 insertions(+), 2 deletions(-) create mode 100755 t/t4139-apply-escape.sh -- 2.3.0-rc2-164-g799cdce -- To unsubscribe from this list: send the line 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/4] apply: do not read from beyond a symbolic link
We should reject a patch, whether it renames/copies dir/file to
elsewhere with or without modificiation, or updates dir/file in
place, if dir/ part is actually a symbolic link to elsewhere,
by making sure that the code to read the preimage does not read
from a path that is beyond a symbolic link.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/apply.c | 2 ++
t/t4122-apply-symlink-inside.sh | 19 +++
2 files changed, 21 insertions(+)
diff --git a/builtin/apply.c b/builtin/apply.c
index 05eaf54..60d821c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3145,6 +3145,8 @@ static int load_patch_target(struct strbuf *buf,
return read_file_or_gitlink(ce, buf);
else
return SUBMODULE_PATCH_WITHOUT_INDEX;
+ } else if (has_symlink_leading_path(name, strlen(name))) {
+ return error(_(reading from '%s' beyond a symbolic
link), name);
} else {
if (read_old_data(st, name, buf))
return error(_(read of %s failed), name);
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 70b3a06..035c080 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -52,4 +52,23 @@ test_expect_success 'check result' '
'
+test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
+ git reset --hard
+ mkdir -p arch/x86_64/dir
+ arch/x86_64/dir/file
+ git add arch/x86_64/dir/file
+ echo line arch/x86_64/dir/file
+ git diff patch
+ git reset --hard
+
+ mkdir arch/i386/dir
+ arch/i386/dir/file
+ ln -s ../i386/dir arch/x86_64/dir
+
+ test_must_fail git apply patch
+ test_must_fail git apply --cached patch
+ test_must_fail git apply --index patch
+
+'
+
test_done
--
2.3.0-rc2-164-g799cdce
--
To unsubscribe from this list: send the line 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/4] apply: reject input that touches outside $cwd
By default, a patch that affects outside the working area is
rejected as a mistake (or a mischief); Git itself does not create
such a patch, unless the user bends backwards and specifies a
non-standard prefix to git diff and friends.
When `git apply` is used without either `--index` or `--cached`
option as a better GNU patch, the user can pass `--unsafe-paths`
option to override this safety check. This cannot be used to escape
outside the working tree when using `--index` or `--cached` to apply
the patch to the index.
The new test was stolen from Jeff King with slight enhancements.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
Documentation/git-apply.txt | 14 -
builtin/apply.c | 26 +
t/t4139-apply-escape.sh | 137
3 files changed, 176 insertions(+), 1 deletion(-)
create mode 100755 t/t4139-apply-escape.sh
diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index f605327..a6e83a9 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -16,7 +16,7 @@ SYNOPSIS
[--ignore-space-change | --ignore-whitespace ]
[--whitespace=(nowarn|warn|fix|error|error-all)]
[--exclude=path] [--include=path] [--directory=root]
- [--verbose] [patch...]
+ [--verbose] [--unsafe-paths] [patch...]
DESCRIPTION
---
@@ -229,6 +229,18 @@ For example, a patch that talks about updating
`a/git-gui.sh` to `b/git-gui.sh`
can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by
running `git apply --directory=modules/git-gui`.
+--unsafe-paths::
+ By default, a patch that affects outside the working area is
+ rejected as a mistake (or a mischief); Git itself never
+ creates such a patch unless the user bends backwards and
+ specifies nonstandard prefix to git diff and friends.
++
+When `git apply` is used without either `--index` or `--cached`
+option as a better GNU patch, the user can pass `--unsafe-paths`
+option to override this safety check. This cannot be used to escape
+outside the working tree when using `--index` or `--cached` to apply
+the patch to the index.
+
Configuration
-
diff --git a/builtin/apply.c b/builtin/apply.c
index ef32e4f..c751ddf 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -50,6 +50,7 @@ static int apply_verbosely;
static int allow_overlap;
static int no_add;
static int threeway;
+static int unsafe_paths;
static const char *fake_ancestor;
static int line_termination = '\n';
static unsigned int p_context = UINT_MAX;
@@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int
ok_if_exists)
return 0;
}
+static void die_on_unsafe_path(struct patch *patch)
+{
+ const char *old_name = NULL;
+ const char *new_name = NULL;
+ if (patch-is_delete)
+ old_name = patch-old_name;
+ else if (!patch-is_new !patch-is_copy)
+ old_name = patch-old_name;
+ if (!patch-is_delete)
+ new_name = patch-new_name;
+
+ if (old_name !verify_path(old_name))
+ die(_(invalid path '%s'), old_name);
+ if (new_name !verify_path(new_name))
+ die(_(invalid path '%s'), new_name);
+}
+
/*
* Check and apply the patch in-core; leave the result in patch-result
* for the caller to write it out to the final destination.
@@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch)
}
}
+ if (!unsafe_paths)
+ die_on_unsafe_path(patch);
+
if (apply_data(patch, st, ce) 0)
return error(_(%s: patch does not apply), name);
patch-rejected = 0;
@@ -4379,6 +4400,8 @@ int cmd_apply(int argc, const char **argv, const char
*prefix_)
N_(make sure the patch is applicable to the current
index)),
OPT_BOOL(0, cached, cached,
N_(apply a patch without touching the working tree)),
+ OPT_BOOL(0, unsafe-paths, unsafe_paths,
+ N_(accept a patch to touch outside the working area)),
OPT_BOOL(0, apply, force_apply,
N_(also apply the patch (use with
--stat/--summary/--check))),
OPT_BOOL('3', 3way, threeway,
@@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char
*prefix_)
die(_(--cached outside a repository));
check_index = 1;
}
+ if (check_index)
+ unsafe_paths = 0;
+
for (i = 0; i argc; i++) {
const char *arg = argv[i];
int fd;
diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh
new file mode 100755
index 000..5e67179
--- /dev/null
+++ b/t/t4139-apply-escape.sh
@@ -0,0 +1,137 @@
+#!/bin/sh
+
+test_description='paths written by git-apply cannot escape the working tree'
+. ./test-lib.sh
+
+# tests
[PATCH v2 2/4] apply: do not read from the filesystem under --index
We currently read the preimage to apply a patch from the index only
when the --cached option is given. Do so also when the command is
running under the --index option. With --index, the index entry and
the working tree file for a path that is involved in a patch must be
identical, so this should not affect the result, but by reading from
the index, we will get the protection to avoid reading an unintended
path beyond a symbolic link automatically.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/apply.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index c751ddf..05eaf54 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3136,7 +3136,7 @@ static int load_patch_target(struct strbuf *buf,
const char *name,
unsigned expected_mode)
{
- if (cached) {
+ if (cached || check_index) {
if (read_file_or_gitlink(ce, buf))
return error(_(read of %s failed), name);
} else if (name) {
--
2.3.0-rc2-164-g799cdce
--
To unsubscribe from this list: send the line 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/4] apply: reject input that touches outside $cwd
On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:
+test_expect_failure 'symlink escape via ..' '
+ {
+ mkpatch_symlink tmp ..
+ mkpatch_add tmp/foo ../foo
+ } patch
+ test_must_fail git apply patch
+ test_path_is_missing ../foo
+'
By the way, does this patch (and the other symlink-escape ones) need to
be marked with the SYMLINKS prereq? For a pure-index application, it
should work anywhere, but I have a feeling that this git apply patch
may try to write the symlink to the filesystem, fail, and report failure
for the wrong reason. I don't have a SYMLINK-challenged filesystem to
test on, though.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link
Jeff King p...@peff.net writes: On Mon, Feb 02, 2015 at 03:27:30PM -0800, Junio C Hamano wrote: +static struct string_list symlink_changes; I notice we don't duplicate strings for this list. Are the paths we register here always stable? I think they should be, as they are part of the struct patch. Yeah, and I also forgot to free this string-list itself. Needs fixing. I think this means we'll be overly cautious with a patch that does: 1. add foo as a symlink 2. remove foo 3. add foo/bar which is perfectly OK No, such a patchset is broken. A valid git apply input must *not* depend on the order of patches in it. The consequence is that an input to 'git apply' must not mention the fate of each path at most once. create B by copying A and modify A in-place can appear in the same patchset in any order, and the new file B will have the contents from the original A, not the result of modifying A in-place, which is what will be in the resulting A. That is how git diff expresses renames and copies, and that is why rearranging the patchset using git diff -Oorderfile is safe. but we'll reject. I suspect this is making things much simpler for you, because now we don't have to worry about order of application that we were discussing the other day. It is not now this new decision made things simpler. git diff output and git apply application have been designed to work that way from day one. At least from day one of rename/copy feature. We probably should start thinking about ripping out the fn_table[] crud. It fundamentally cannot correctly work on an input that concatenates more than one git diff outputs that have renames and/or copies of the same file, and it never will, and that is not due to a bug in the implementation. The reason why the incremental application is a fundamentally flawed concept in the context of git apply is because you cannot tell the boundary between the original git diff outputs. Imagine that you have three versions, A, B and C, and the original two git diff -M A B and git diff -M B C output said this: (A - B) edit X in place and add two lines at the beginning create Z by copying X (B - C) create Y by renaming X and add a line at the end If you take git diff -M A C, it should say: (A - C) edit X in place and add two lines at the beginning create Y by copying X and add two lines at the beginning and a line at the end create Z by copying X Now, if you concatenate two git diff outputs and feed it to git apply, you would want it to express a patchset that goes from A to C, but think if you can really get such a semantics. edit X in place and add two lines at the beginning create Z by copying X create Y by renaming X and add a line at the end You fundamentally cannot tell that Z needs to be a copy of X before the change to X (which is what the usual git apply does), but Y needs to start from a copy of X after the change to X. There is no clue to tell git apply that there is a boundary between the first two operations and the third one. It is impossible for the concatenated patch to express the same thing as (A - C) patch does, without inventng some I am now switching to a new basis marker in the git apply input stream. +/* + * An attempt to read from or delete a path that is beyond + * a symbolic link will be prevented by load_patch_target() + * that is called at the beginning of apply_data(). We need + * to make sure that the patch result is not deposited to + * a path that is beyond a symbolic link ourselves. + */ +if (!patch-is_delete path_is_beyond_symlink(patch-new_name)) +return error(_(affected file '%s' is beyond a symbolic link), + patch-new_name); Do we need to check the patch-is_delete case here (with patch-old_name)? I had a suspicion that the new patch 3/4 to check the reading-side might help with that, but the comment here sounds like we do need to check here, too Hmm, the comment above was meant to tell you that we do not have to worry about the deletion case (because load_patch_target() will try to read the original to verify we are deleting what we expect to delete at the beginning of apply_data(), and it will notice that old_name is beyond a symbolic link), but we still need to check the non-deletion case. Strictly speaking, modify-in-place case does not have to depend on this code (the same load_patch_target() check will catch it because it wants to check the preimage). May need rephrasing to clarify but I thought it was clear enough. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link
On Mon, Feb 02, 2015 at 03:27:30PM -0800, Junio C Hamano wrote:
+static struct string_list symlink_changes;
I notice we don't duplicate strings for this list. Are the paths we
register here always stable? I think they should be, as they are part of
the struct patch.
+#define SYMLINK_GOES_AWAY 01
+#define SYMLINK_IN_RESULT 02
+
+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
+{
+ struct string_list_item *ent;
+
+ ent = string_list_lookup(symlink_changes, path);
+ if (!ent) {
+ ent = string_list_insert(symlink_changes, path);
+ ent-util = (void *)0;
+ }
+ ent-util = (void *)(what | ((uintptr_t)ent-util));
+ return (uintptr_t)ent-util;
+}
I was surprised to see this as a bit-field and not a current state as
we walk through the set of patches to apply. I think this means we'll be
overly cautious with a patch that does:
1. add foo as a symlink
2. remove foo
3. add foo/bar
which is perfectly OK, but we'll reject. I suspect this is making things
much simpler for you, because now we don't have to worry about order of
application that we were discussing the other day. If that is the
reason, then I think patches like the above are an acceptable casualty.
It seems rather far-fetched in the first place for a real patch (as
opposed to a mischievous one).
+ /*
+ * An attempt to read from or delete a path that is beyond
+ * a symbolic link will be prevented by load_patch_target()
+ * that is called at the beginning of apply_data(). We need
+ * to make sure that the patch result is not deposited to
+ * a path that is beyond a symbolic link ourselves.
+ */
+ if (!patch-is_delete path_is_beyond_symlink(patch-new_name))
+ return error(_(affected file '%s' is beyond a symbolic link),
+ patch-new_name);
Do we need to check the patch-is_delete case here (with patch-old_name)?
I had a suspicion that the new patch 3/4 to check the reading-side might
help with that, but the comment here sounds like we do need to check
here, too (and I am not clear on how 3/4 handles deleting from a patch
on the far side of a symlink we have just created).
It does seem to work, though. I'm just not sure how. :)
Here's the test addition I came up with, because it didn't look like we
were covering this case.
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 942c5cb..fbba8dd 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -89,6 +89,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link
(setup)' '
rm -fr arch/x86_64/dir
cat add_symlink.patch add_file.patch patch
+ cat add_symlink.patch del_file.patch tricky_del
mkdir arch/i386/dir
'
@@ -112,6 +113,20 @@ test_expect_success SYMLINKS 'do not follow symbolic link
(same input)' '
test_i18ngrep beyond a symbolic link error-ct
test_must_fail git ls-files --error-unmatch arch/x86_64/dir
test_must_fail git ls-files --error-unmatch arch/i386/dir
+
+ arch/i386/dir/file
+ git add arch/i386/dir/file
+ test_must_fail git apply tricky_del
+ test_path_is_file arch/i386/dir/file
+
+ test_must_fail git apply --index tricky_del
+ test_path_is_file arch/i386/dir/file
+ test_must_fail git ls-files --error-unmatch arch/x86_64/dir
+ git ls-files --error-unmatch arch/i386/dir
+
+ test_must_fail git apply --cached tricky_del
+ test_must_fail git ls-files --error-unmatch arch/x86_64/dir
+ git ls-files --error-unmatch arch/i386/dir
'
test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
@@ -125,6 +140,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link
(existing)' '
test_i18ngrep beyond a symbolic link error-wt-add
test_path_is_missing arch/i386/dir/file
+ mkdir arch/i386/dir
arch/i386/dir/file
test_must_fail git apply del_file.patch 2error-wt-del
test_i18ngrep beyond a symbolic link error-wt-del
--
To unsubscribe from this list: send the line 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: RFD: should we do another 2.3-rc for t9001-noxmailer? I'd say not
On 01/02/15 23:48, Junio C Hamano wrote: I was reviewing the recent bugs and fixes for the last time, and was wondering if we want to do 2.3-rc3 with build fix for those with ancient cURL (tc/curl-vernum-output-broken-in-7.11) and workaround for those with Perl with older Getopt::Long (tc/t9001-noxmailer). - The former is not a regression between 2.2 and 2.3 (i.e. 2.2 already had the same use of curl-config output). - The latter, strictly speaking, is a regression in that tests used to pass but tests in 2.3 no longer pass for those with older Getopt::Long. So currently I am leaning to keep these two fixes where they are and tag 2.3 final without them in a few days. Leaving them for a later release is fine by me. These two patches cover only what broke from 2.2.2 to 2.3, there are further patches needed to actually complete a build atleast on RHEL3. -tgc -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/19] Introduce an internal API to interact with the fsck machinery
On 02/02/2015 12:41 PM, Johannes Schindelin wrote: Hi all (in particular Junio), On 2015-01-31 22:04, Johannes Schindelin wrote: [...] switch to fsck.severity to address Michael's concerns that letting fsck.(error|warn|ignore)'s comma-separated lists possibly overriding each other partially; Having participated in the CodingStyle thread, I came to the conclusion that the fsck.severity solution favors syntax over intuitiveness. Therefore, I would like to support the case for `fsck.level.missingAuthor` (note that there is an extra .level. in contrast to earlier suggestions). Why level? The benefits: - it is very, very easy to understand - cumulative settings are intuitively cumulative, i.e. setting `fsck.level.missingAuthor` will leave `fsck.level.invalidEmail` completely unaffected - it is very easy to enquire and set the levels via existing `git config` calls Now, there is one downside, but *only* if we ignore Postel's law. Postel's law (be lenient in what you accept as input, but strict in your output) would dictate that our message ID parser accept both missing-author and missingAuthor if we follow the inconsistent practice of using lowercase-dashed keys on the command-line but CamelCased ones in the config. However, earlier Junio made very clear that the parser is required to fail to parse missing-author in the config, and to fail to parse missingAuthor on the command-line. Therefore, the design I recommend above will require two, minimally different parsers for essentially the same thing. IMHO this is a downside that is by far outweighed by the ease of use of the new feature, therefore I am willing to bear the burden of implementation. I again encourage you to consider skipping the implementation of command-line options entirely. It's not like users are going to want to use different options for different invocations. Let them use git -c fsck.level.missingAuthor=ignore fsck if they really want to play around, then git config fsck.level.missingAuthor ignore to make it permanent. After that they will never have to worry about that option again. And Postel needn't be offended :-) Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables
Hi, On 2015-02-01 23:34, Junio C Hamano wrote: Jeff King p...@peff.net writes: 1. I'm a user who has set my preferred core.whitespace in my ~/.gitconfig. A particular project I am working on uses an alternate tabwidth. How do I set that in the repo config without repeating my defaults? Isn't it cumulative? At least it should be (but I wouldn't be too surprised if the recent config reader caching broke it). Please note that it is really, really difficult for a regular Git user to figure out which whitespace settings are in effect using just `git config` whether it is cumulative or not. Also, please note it makes it hard on users that there are a bunch of config settings which *override* previous settings, and others that are *cumulative*. The latter we cannot change, and the former we cannot change for the whitespace settings. However, when introducing new settings (such as the fsck severity levels), we should go out of our way to keep things as simple as possible. For example, if a *simple* `git config` can answer the question Is feature XYZ turned on, I would consider the design superior to a design that requires additional code just to figure out whether feature XYZ is turned on, let alone to turn it on without affecting other settings. In other words, I would like to caution against recommending the whitespace setting as an example to follow: anybody who is unfamiliar with the whitespace setting will find the cumulative last-setting-wins (per item inside the whitespace-separated list, not per config setting) strategy confusing. On the other hand, if you offer `whitespace.tabWidth` and `whitespace.indentWithNoTab` separately, everything is really crystal clear to new users without having much explaining to do, let alone having to introduce new tooling. Ciao, Dscho P.S.: Of course I know that it is too late for `whitespace`, but it offers itself as a good subject to demonstrate the merits of the different suggestions. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/19] Introduce an internal API to interact with the fsck machinery
Hi all (in particular Junio), On 2015-01-31 22:04, Johannes Schindelin wrote: [...] switch to fsck.severity to address Michael's concerns that letting fsck.(error|warn|ignore)'s comma-separated lists possibly overriding each other partially; Having participated in the CodingStyle thread, I came to the conclusion that the fsck.severity solution favors syntax over intuitiveness. Therefore, I would like to support the case for `fsck.level.missingAuthor` (note that there is an extra .level. in contrast to earlier suggestions). The benefits: - it is very, very easy to understand - cumulative settings are intuitively cumulative, i.e. setting `fsck.level.missingAuthor` will leave `fsck.level.invalidEmail` completely unaffected - it is very easy to enquire and set the levels via existing `git config` calls Now, there is one downside, but *only* if we ignore Postel's law. Postel's law (be lenient in what you accept as input, but strict in your output) would dictate that our message ID parser accept both missing-author and missingAuthor if we follow the inconsistent practice of using lowercase-dashed keys on the command-line but CamelCased ones in the config. However, earlier Junio made very clear that the parser is required to fail to parse missing-author in the config, and to fail to parse missingAuthor on the command-line. Therefore, the design I recommend above will require two, minimally different parsers for essentially the same thing. IMHO this is a downside that is by far outweighed by the ease of use of the new feature, therefore I am willing to bear the burden of implementation. Do you agree? 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: git status should warn/error when it cannot lists a directory
On Mon, Feb 02, 2015 at 11:58:33AM -0500, Andrew Wong wrote: When git status recurses a directory that isn't readable (but executable), it should print out a warning/error. Currently, if there are untracked files in these directories, git wouldn't be able to discover them. Ideally, git status should return a non-zero exit code as well. Also, I think git add . would silently ignore such directories, which is probably a bad thing if you are relying on it to capture the whole directory's state. Similarly, I think we would ignore transient errors (like EMFILE) or other EACCES problems (like a mode 0700 directory owned by somebody else). The problem seems to be In read_directory_recursive() from dir.c. When opendir() returns null, we continue on ignoring any error. Is there a scenario where returning null is expected? We can simply call perror() here, but it would be nice if we can propagate the error to the exit code too. How would we do that? I think we should report an error on EACCES. Perhaps somebody is happy that git add ignores unreadable directories, but the right solution is for them to put those directories in their .gitignore (and/or use --ignore-errors). People may want to ignore ENOENT in this situation, though. That is a sign that somebody is racily modifying the directory while git is running. That's generally a bad idea, but it is not a big deal for us to skip such a directory (after all, we might racily have missed its existence in the first place, so all bets are off). From a cursory look, I'd agree that hitting the opendir() in read_directory_recursive is the right place to start. I'd silently ignore ENOENT, and propagate the rest. That code is too low-level to call die() directly, I think, so you will need to propagate the error back. Adding a new error-value to the enum path_treatment could work, but it will probably be rather clumsy getting it all the way back up to the original caller. It will probably be much easier to: 1. Give dir_struct an error flag, and set it whenever the traversal sees an error. Callers can check the flag at the appropriate level and ignore or die() as appropriate. 2. Teach dir_struct a quiet flag. If not set, emit a warning() deep in the code. Alternatively, you could collect a set of error-producing pathnames (along with their errno values), and the caller could decide whether to print them itself (this is similar to how DIR_COLLECT_IGNORED works). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: folder naming bug?
Awesome reply! That makes sense. So basically if I accidentally capitalize a folder name and commit it, I need to be very careful when I correct it. Definitely ran into this problem with my repo and ‘lost’ a few commits before I noticed something was off. -Kevin Coleman On Feb 3, 2015, at 12:23 AM, Jeff King p...@peff.net wrote: On Mon, Feb 02, 2015 at 11:52:21PM -0500, Kevin Coleman wrote: Yes, I am on a Mac. I just tried that, but I don’t think that completely fixed it. As you can see it tracks “foo/bar.md” and then it tracks “Foo/bar.md”. It still tracks both “foo” and “Foo” even tho only “Foo” exists in my dir after the rename. Yes, because your filesystem _is_ case insensitive, but now you have told git that it is not. In your example: 11:41:57 ~/test $ git init Initialized empty Git repository in /Users/kcoleman/test/.git/ 11:42:03 ~/test (master #) $ git config core.ignorecase false 11:42:06 ~/test (master #) $ mkdir foo 11:42:13 ~/test (master #) $ cd foo 11:42:26 ~/test/foo (master #) $ touch bar.md 11:42:30 ~/test/foo (master #) $ cd .. 11:42:32 ~/test (master #) $ git add . Now git has foo (lowercase) in the index. And that's what your filesystem has, too. 11:42:35 ~/test (master #) $ git commit -m first [master (root-commit) 6125a1d] first 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 foo/bar.md 11:42:39 ~/test (master) $ mv foo Foo 11:42:44 ~/test (master) $ ls Foo Now we still have foo in the index, but Foo in the filesystem. 11:42:46 ~/test (master) $ git status On branch master Untracked files: (use git add file... to include in what will be committed) Foo/ When git asks the filesystem lstat(foo) to find out if we still have it, the filesystem returns the entry for Foo (because it is case-insensitive). But when git asks the filesystem to iterate over all of the files, so it can check which ones are not tracked, it will get Foo, which of course is not in the index. So you do not see a deletion of foo, but you do see Foo as untracked. 11:42:48 ~/test (master) $ git add . 11:43:18 ~/test (master +) $ git commit -m second [master f78d025] second 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 Foo/bar.md And this tells git to look through the filesystem for untracked files and add them to the index. So it adds Foo. Now that you have both foo and Foo in the index, but the filesystem treats them the same, you can create more mayhem. If you were to update one entry but not the other (e.g., by writing to bar.md before doing the second commit), then git would be perpetually confused. _One_ of the files would always look like needed to be updated, because the filesystem cannot represent the situation that is in the index. And that is why git sets core.ignorecase in the first place. :) As to your original problem: git isn’t tracking folder renames when the case of the letters change, but it will track it if the folder changes names. Is this intentional? Yes, this is intentional. Your filesystem treats them as the same file, so git has to, as well. If your goal is to change the case that git records, then you should be able to do it with git mv. But git will never pick up a case change that you made separately in the filesystem, because it's indistinguishable from the filesystem simply picking a different case to store the file. And that does happen. For instance, if you switch between two branches with Foo and foo, most case-preserving filesystems will leave you with whichever version you had first (i.e., git asks the filesystem to open foo, and the filesystem says ah, I already have Foo; that must have been what you meant). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link
On Mon, Feb 02, 2015 at 05:56:55PM -0800, Junio C Hamano wrote: I think this means we'll be overly cautious with a patch that does: 1. add foo as a symlink 2. remove foo 3. add foo/bar which is perfectly OK No, such a patchset is broken. A valid git apply input must *not* depend on the order of patches in it. The consequence is that an input to 'git apply' must not mention the fate of each path at most once. Ah, right, I forgot we covered this already in the earlier discussion (but thanks for elaborating; I think the reason I forgot is that I did not really understand all of the implications). If we do not have to worry about that, then it's not a problem. + /* + * An attempt to read from or delete a path that is beyond + * a symbolic link will be prevented by load_patch_target() + * that is called at the beginning of apply_data(). We need + * to make sure that the patch result is not deposited to + * a path that is beyond a symbolic link ourselves. + */ + if (!patch-is_delete path_is_beyond_symlink(patch-new_name)) + return error(_(affected file '%s' is beyond a symbolic link), + patch-new_name); Do we need to check the patch-is_delete case here (with patch-old_name)? I had a suspicion that the new patch 3/4 to check the reading-side might help with that, but the comment here sounds like we do need to check here, too Hmm, the comment above was meant to tell you that we do not have to worry about the deletion case (because load_patch_target() will try to read the original to verify we are deleting what we expect to delete at the beginning of apply_data(), and it will notice that old_name is beyond a symbolic link), but we still need to check the non-deletion case. Strictly speaking, modify-in-place case does not have to depend on this code (the same load_patch_target() check will catch it because it wants to check the preimage). May need rephrasing to clarify but I thought it was clear enough. Ah, OK. I totally misread it, thinking that load_patch_target was what set up the symlink-table, and that was what you were referring to. It might be more clear after ...that is called at the beginning of apply_data() to add ...so we do not have to worry about that case here. -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
folder naming bug?
git isn’t tracking folder renames when the case of the letters change, but it will track it if the folder changes names. Is this intentional? Here is an example: 08:51:26 ~/test $ git init Initialized empty Git repository in /Users/kcoleman/test/.git/ 08:51:29 ~/test (master #) $ mkdir main 08:51:44 ~/test (master #) $ cd main 08:51:46 ~/test/main (master #) $ touch readme.md 08:51:50 ~/test/main (master #) $ ls readme.md 08:51:53 ~/test/main (master #) $ cd .. 08:51:54 ~/test (master #) $ git add . 08:51:59 ~/test (master #) $ git commit -m one [master (root-commit) b0fddf6] one 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 main/readme.md 08:52:04 ~/test (master) $ cd main 08:52:14 ~/test/main (master) $ cd .. 08:52:27 ~/test (master) $ mv main Main 08:53:51 ~/test (master) $ git status On branch master nothing to commit, working directory clean 08:53:53 ~/test (master) $ ls Main 08:54:02 ~/test (master) $ mv Main MainA 08:55:44 ~/test (master *) $ git status On branch master Changes not staged for commit: (use git add/rm file... to update what will be committed) (use git checkout -- file... to discard changes in working directory) deleted:main/readme.md Untracked files: (use git add file... to include in what will be committed) MainA/ no changes added to commit (use git add and/or git commit -a) 08:55:45 ~/test (master *) $-- To unsubscribe from this list: send the line 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: Relative paths don't work in .gitignore as would be expected.
2015-02-02 11:15 GMT-08:00 Junio C Hamano gits...@pobox.com: Stefan Beller stefanbel...@gmail.com writes: On 01.02.2015 14:51, /#!/JoePea wrote: I have this in my .gitignore: ./*.js I would expect that to cause git to ignore .js files in the same folder as .gitignore, but it doesn't do anything. However, this works: /*.js I'm not sure what this actually means because a leading slash is the root of some filesystem, Isn't gitignore(5) documentation reasonably clear? - If the pattern ends with a slash, it is removed for the purpose of the following description, but it would only find a match with a directory. In other words, foo/ will match a directory foo and paths underneath it, but will not match a regular file or a symbolic link foo (this is consistent with the way how pathspec works in general in Git). - If the pattern does not contain a slash /, Git treats it as a shell glob pattern and checks for a match against the pathname relative to the location of the .gitignore file (relative to the toplevel of the work tree if not from a .gitignore file). - A leading slash matches the beginning of the pathname. For example, /*.c matches cat-file.c but not mozilla-sha1/sha1.c. That's true, though you'd never (barely?) git version control an entire file system? When you have the entire file system under /.git, /var/ still would be the right way to spell a pattern to match only a directory (because of the trailing '/') whose name matches 'var' and lives in the root level of the filesystem (because of the leading '/' anchors the pattern to the same level as the file the pattern appears in, i.e. /.gitignore) and no other place. Ok, that's true. So when I started diving into the wonderful world of unix like operating system, one of the first things I was taught is / starts an absolute path, while ./foo or just foo starts a relative path. And this stuck with me. Now I realize git treats the repository root literally as the root and hence absolute paths starting with / make totally sense inside git as the world stops for git outside its work directory. (from man gitignore, though reading that and not finding a './' it may need improvement We do not allow relative pathname traversal with . or .., do we? Because we don't have to. It's always relative to the .gitignore file (foo/.gitignore can talk about bar/ and still mean foo/bar), so we don't need to express the relativity in any special way. I would be very hesitant to special case ./*.js to mean *.js files in the same directory as .gitignore appears, as I think it risks intelligent readers to infer ../foo/*.js or ../*.js would take effect, when placed in bar/.gitignore. A design that spreads an incorrect assumption/expectation is not a good one, I would have to say. I did not say I'd change the behavior of the ignore rules. I rather meant to say the documentation could be filled with examples for common patterns. That way you'd not be required to read all the rules and *think* about them, but rather can just copy/paste and hope it works. ;) Sorry if this sounded otherwise. -- To unsubscribe from this list: send the line 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/4] apply: reject input that touches outside $cwd
If I am allowed to to some load thinking: The commit msh header says: reject input that touches outside $cwd The commit message says: By default, a patch that affects outside the working area And the new command line option is this: --unsafe-paths (Which may be a good choice to pretend people from using it without having read the documentaion) (And isn't working area the same as work space ?) I have the slight feeling that there may be more unsafe-path situations coming up in the future.. Will --allow-outside-ws or --patch-outside-ws be better ? -- To unsubscribe from this list: send the line 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: folder naming bug?
Are you, by any chance, on MacOS? HFS+ by default is case-insensitive-but-case-preserving, and Git on MacOS by default runs with core.ignorecase = true as a result. If you set that to false does it change the behavior? Hope this helps, Bryan Turner On Tue, Feb 3, 2015 at 12:56 PM, Kevin Coleman kevin.cole...@sparkstart.io wrote: git isn’t tracking folder renames when the case of the letters change, but it will track it if the folder changes names. Is this intentional? Here is an example: 08:51:26 ~/test $ git init Initialized empty Git repository in /Users/kcoleman/test/.git/ 08:51:29 ~/test (master #) $ mkdir main 08:51:44 ~/test (master #) $ cd main 08:51:46 ~/test/main (master #) $ touch readme.md 08:51:50 ~/test/main (master #) $ ls readme.md 08:51:53 ~/test/main (master #) $ cd .. 08:51:54 ~/test (master #) $ git add . 08:51:59 ~/test (master #) $ git commit -m one [master (root-commit) b0fddf6] one 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 main/readme.md 08:52:04 ~/test (master) $ cd main 08:52:14 ~/test/main (master) $ cd .. 08:52:27 ~/test (master) $ mv main Main 08:53:51 ~/test (master) $ git status On branch master nothing to commit, working directory clean 08:53:53 ~/test (master) $ ls Main 08:54:02 ~/test (master) $ mv Main MainA 08:55:44 ~/test (master *) $ git status On branch master Changes not staged for commit: (use git add/rm file... to update what will be committed) (use git checkout -- file... to discard changes in working directory) deleted:main/readme.md Untracked files: (use git add file... to include in what will be committed) MainA/ no changes added to commit (use git add and/or git commit -a) 08:55:45 ~/test (master *) $-- To unsubscribe from this list: send the line 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: folder naming bug?
On Mon, Feb 02, 2015 at 11:52:21PM -0500, Kevin Coleman wrote: Yes, I am on a Mac. I just tried that, but I don’t think that completely fixed it. As you can see it tracks “foo/bar.md” and then it tracks “Foo/bar.md”. It still tracks both “foo” and “Foo” even tho only “Foo” exists in my dir after the rename. Yes, because your filesystem _is_ case insensitive, but now you have told git that it is not. In your example: 11:41:57 ~/test $ git init Initialized empty Git repository in /Users/kcoleman/test/.git/ 11:42:03 ~/test (master #) $ git config core.ignorecase false 11:42:06 ~/test (master #) $ mkdir foo 11:42:13 ~/test (master #) $ cd foo 11:42:26 ~/test/foo (master #) $ touch bar.md 11:42:30 ~/test/foo (master #) $ cd .. 11:42:32 ~/test (master #) $ git add . Now git has foo (lowercase) in the index. And that's what your filesystem has, too. 11:42:35 ~/test (master #) $ git commit -m first [master (root-commit) 6125a1d] first 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 foo/bar.md 11:42:39 ~/test (master) $ mv foo Foo 11:42:44 ~/test (master) $ ls Foo Now we still have foo in the index, but Foo in the filesystem. 11:42:46 ~/test (master) $ git status On branch master Untracked files: (use git add file... to include in what will be committed) Foo/ When git asks the filesystem lstat(foo) to find out if we still have it, the filesystem returns the entry for Foo (because it is case-insensitive). But when git asks the filesystem to iterate over all of the files, so it can check which ones are not tracked, it will get Foo, which of course is not in the index. So you do not see a deletion of foo, but you do see Foo as untracked. 11:42:48 ~/test (master) $ git add . 11:43:18 ~/test (master +) $ git commit -m second [master f78d025] second 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 Foo/bar.md And this tells git to look through the filesystem for untracked files and add them to the index. So it adds Foo. Now that you have both foo and Foo in the index, but the filesystem treats them the same, you can create more mayhem. If you were to update one entry but not the other (e.g., by writing to bar.md before doing the second commit), then git would be perpetually confused. _One_ of the files would always look like needed to be updated, because the filesystem cannot represent the situation that is in the index. And that is why git sets core.ignorecase in the first place. :) As to your original problem: git isn’t tracking folder renames when the case of the letters change, but it will track it if the folder changes names. Is this intentional? Yes, this is intentional. Your filesystem treats them as the same file, so git has to, as well. If your goal is to change the case that git records, then you should be able to do it with git mv. But git will never pick up a case change that you made separately in the filesystem, because it's indistinguishable from the filesystem simply picking a different case to store the file. And that does happen. For instance, if you switch between two branches with Foo and foo, most case-preserving filesystems will leave you with whichever version you had first (i.e., git asks the filesystem to open foo, and the filesystem says ah, I already have Foo; that must have been what you meant). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: folder naming bug?
Sorry for sending x2. I got a bounce notification the first time. Yes, I am on a Mac. I just tried that, but I don’t think that completely fixed it. As you can see it tracks “foo/bar.md” and then it tracks “Foo/bar.md”. It still tracks both “foo” and “Foo” even tho only “Foo” exists in my dir after the rename. I create a public repo on github with this repo https://github.com/KevinColemanInc/test I am running git version 2.2.2. 11:41:57 ~/test $ git init Initialized empty Git repository in /Users/kcoleman/test/.git/ 11:42:03 ~/test (master #) $ git config core.ignorecase false 11:42:06 ~/test (master #) $ mkdir foo 11:42:13 ~/test (master #) $ cd foo 11:42:26 ~/test/foo (master #) $ touch bar.md 11:42:30 ~/test/foo (master #) $ cd .. 11:42:32 ~/test (master #) $ git add . 11:42:35 ~/test (master #) $ git commit -m first [master (root-commit) 6125a1d] first 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 foo/bar.md 11:42:39 ~/test (master) $ mv foo Foo 11:42:44 ~/test (master) $ ls Foo 11:42:46 ~/test (master) $ git status On branch master Untracked files: (use git add file... to include in what will be committed) Foo/ nothing added to commit but untracked files present (use git add to track) 11:42:48 ~/test (master) $ git add . 11:43:18 ~/test (master +) $ git commit -m second [master f78d025] second 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 Foo/bar.md -Kevin Coleman On Feb 2, 2015, at 11:37 PM, Bryan Turner btur...@atlassian.com wrote: Are you, by any chance, on MacOS? HFS+ by default is case-insensitive-but-case-preserving, and Git on MacOS by default runs with core.ignorecase = true as a result. If you set that to false does it change the behavior? Hope this helps, Bryan Turner On Tue, Feb 3, 2015 at 12:56 PM, Kevin Coleman kevin.cole...@sparkstart.io wrote: git isn’t tracking folder renames when the case of the letters change, but it will track it if the folder changes names. Is this intentional? Here is an example: 08:51:26 ~/test $ git init Initialized empty Git repository in /Users/kcoleman/test/.git/ 08:51:29 ~/test (master #) $ mkdir main 08:51:44 ~/test (master #) $ cd main 08:51:46 ~/test/main (master #) $ touch readme.md 08:51:50 ~/test/main (master #) $ ls readme.md 08:51:53 ~/test/main (master #) $ cd .. 08:51:54 ~/test (master #) $ git add . 08:51:59 ~/test (master #) $ git commit -m one [master (root-commit) b0fddf6] one 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 main/readme.md 08:52:04 ~/test (master) $ cd main 08:52:14 ~/test/main (master) $ cd .. 08:52:27 ~/test (master) $ mv main Main 08:53:51 ~/test (master) $ git status On branch master nothing to commit, working directory clean 08:53:53 ~/test (master) $ ls Main 08:54:02 ~/test (master) $ mv Main MainA 08:55:44 ~/test (master *) $ git status On branch master Changes not staged for commit: (use git add/rm file... to update what will be committed) (use git checkout -- file... to discard changes in working directory) deleted:main/readme.md Untracked files: (use git add file... to include in what will be committed) MainA/ no changes added to commit (use git add and/or git commit -a) 08:55:45 ~/test (master *) $-- To unsubscribe from this list: send the line 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: [git-gui] bug report: Open existing repository dialog fails on submodules
2015-02-02 3:41 UTC-05:00, Chris Packham judge.pack...@gmail.com: [...] But it actually looks like git rev-parse --resolve-git-dir $path needs to be run inside a git repository _any_ git repository, which seems a bit backwards to me. [...] Indeed, looking at git-rev-parse(1), the correct option might be --show-toplevel, which will print the cwd if it is the top-level of a non-bare repository: cd $candidate test $(git rev-parse --show-toplevel) = $candidate or test $(git --git-dir=$candidate rev-parse --show-toplevel) = $candidate Of course Git will resolve symlinks at this point, so $candidate has to be resolved first for the equality to make sense. Other solution is to parse the gitdir: ... format and recurse, which is not exactly hard (provided you speak Tcl). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/19] Introduce an internal API to interact with the fsck machinery
Hi Michael, On 2015-02-02 13:43, Michael Haggerty wrote: On 02/02/2015 12:41 PM, Johannes Schindelin wrote: Hi all (in particular Junio), On 2015-01-31 22:04, Johannes Schindelin wrote: [...] switch to fsck.severity to address Michael's concerns that letting fsck.(error|warn|ignore)'s comma-separated lists possibly overriding each other partially; Having participated in the CodingStyle thread, I came to the conclusion that the fsck.severity solution favors syntax over intuitiveness. Therefore, I would like to support the case for `fsck.level.missingAuthor` (note that there is an extra .level. in contrast to earlier suggestions). Why level? Severity level, or error level. Maybe .severity. would be better? The benefits: - it is very, very easy to understand - cumulative settings are intuitively cumulative, i.e. setting `fsck.level.missingAuthor` will leave `fsck.level.invalidEmail` completely unaffected - it is very easy to enquire and set the levels via existing `git config` calls Now, there is one downside, but *only* if we ignore Postel's law. Postel's law (be lenient in what you accept as input, but strict in your output) would dictate that our message ID parser accept both missing-author and missingAuthor if we follow the inconsistent practice of using lowercase-dashed keys on the command-line but CamelCased ones in the config. However, earlier Junio made very clear that the parser is required to fail to parse missing-author in the config, and to fail to parse missingAuthor on the command-line. Therefore, the design I recommend above will require two, minimally different parsers for essentially the same thing. IMHO this is a downside that is by far outweighed by the ease of use of the new feature, therefore I am willing to bear the burden of implementation. I again encourage you to consider skipping the implementation of command-line options entirely. It's not like users are going to want to use different options for different invocations. Let them use git -c fsck.level.missingAuthor=ignore fsck if they really want to play around, then git config fsck.level.missingAuthor ignore to make it permanent. After that they will never have to worry about that option again. Unfortunately, I have to pass the `receive.fsck.*` settings from `git-receive-pack` to `git-unpack-objects` or `git-index-pack` via the command-line, because it is `git-receive-pack` that consumes the config setting, but it is one of `git-unpack-objects` and `git-index-pack` that has to act on it... And Postel needn't be offended :-) ;-) 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: [PATCH] t9001: use older Getopt::Long boolean prefix '--no' rather than '--no-'
On Feb 1, 2015, at 17:33, Junio C Hamano wrote: Kyle J. McKay mack...@gmail.com writes: use 5.008; So either that needs to change or the code should properly deal with the version of Getopt::Long that comes with 5.8.0. Since it's really not very difficult or invasive to add support for the no- variants, here's a patch to do so: Doesn't that approach add what does --no-no-chain-rely-to even mean? confusion to the resulting system? If that is not the case, then I am all for it, but otherwise, let's not. No. You have to append the '!' to get the automagic no prefix alternative(s), so while 'chain-reply-to!' means support chain-reply- to, nochain-reply-to and (if you have a new enough Getopt::Long) no- chain-reply-to, just using 'no-chain-reply-to' without the trailing '!' means that nono-chain-reply-to and no-no-chain-reply-to remain invalid options that will generate an error. -- To unsubscribe from this list: send the line 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 status should warn/error when it cannot lists a directory
When git status recurses a directory that isn't readable (but executable), it should print out a warning/error. Currently, if there are untracked files in these directories, git wouldn't be able to discover them. Ideally, git status should return a non-zero exit code as well. The problem seems to be In read_directory_recursive() from dir.c. When opendir() returns null, we continue on ignoring any error. Is there a scenario where returning null is expected? We can simply call perror() here, but it would be nice if we can propagate the error to the exit code too. How would we do that? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates
Signed-off-by: Jonathon Mah m...@jonathonmah.com
---
Adjust prune test directly, much nicer.
t/t5304-prune.sh | 13 +
t/t5710-info-alternate.sh | 4 ++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index e32e46d..e825be7 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -253,4 +253,17 @@ test_expect_success 'prune .git/shallow' '
test_path_is_missing .git/shallow
'
+test_expect_success 'prune: handle alternate object database' '
+ test_create_repo A cd A
+ echo Hello World file1
+ git add file1
+ git commit -m Initial commit file1
+ cd ..
+ git clone -l -s A B cd B
+ echo foo bar file2
+ git add file2
+ git commit -m next commit file2
+ git prune
+'
+
test_done
diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh
index 5a6e49d..d82844a 100755
--- a/t/t5710-info-alternate.sh
+++ b/t/t5710-info-alternate.sh
@@ -18,6 +18,7 @@ reachable_via() {
test_valid_repo() {
git fsck --full fsck.log
+ git prune
test_line_count = 0 fsck.log
}
@@ -47,8 +48,7 @@ test_expect_success 'preparing third repository' \
'git clone -l -s B C cd C
echo Goodbye, cruel world file3
git add file3
-git commit -m one more file3
-git repack -a -d -l
+git commit -m one more without packing file3
git prune'
cd $base_dir
--
2.3.0.rc2.2.g184f7a0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 2/2] sha1_file: fix iterating loose alternate objects
The string in 'base' contains a path suffix to a specific object; when
its value is used, the suffix must either be filled (as in
stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared
(as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb
(introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and
treated 'base' as a complete path to the base object directory,
instead of a pointer to the base of the full path string.
The trailing path after 'base' is still initialized to NUL, hiding the
bug in some common cases. Additionally the descendent
for_each_file_in_obj_subdir function swallows ENOENT, so an error only
shows if the alternate's path was last filled with a valid object
(where statting /path/to/existing/00/0bjectfile/00 fails).
Signed-off-by: Jonathon Mah m...@jonathonmah.com
---
sha1_file.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 30995e6..fcb1c4b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3396,9 +3396,13 @@ static int loose_from_alt_odb(struct
alternate_object_database *alt,
void *vdata)
{
struct loose_alt_odb_data *data = vdata;
- return for_each_loose_file_in_objdir(alt-base,
-data-cb, NULL, NULL,
-data-data);
+ int r;
+ alt-name[-1] = 0;
+ r = for_each_loose_file_in_objdir(alt-base,
+ data-cb, NULL, NULL,
+ data-data);
+ alt-name[-1] = '/';
+ return r;
}
int for_each_loose_object(each_loose_object_fn cb, void *data)
--
2.3.0.rc2.2.g184f7a0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] sha1_file: fix iterating loose alternate objects
On 2015-02-02, at 09:53, Jeff King p...@peff.net wrote: I think this is probably the best fix, and is the pattern we use elsewhere when touching alt-base. We _could_ further change this to have for_each_loose_file_in_objdir actually use alt-base as its scratch buffer, writing the object filenames into the end of it (i.e., what it was designed for). But: 1. We still need a strbuf scratch-buffer for the non-alternate object directory. So we'd have to push more code there to over-allocate the buffer, and then for_each_loose_file_in_objdir would assume we always feed it a buffer with the extra slop. That would work, but I find the strbuf approach a little safer; there's not an implicit over-allocation far away in the code preventing us from overflowing a buffer. 2. The reason for the existing alt-base behavior is that the sha1_file code gets fed objects one at a time, and don't want to pay strbuf overhead for each. With the iterator, we know we are going to hit a bunch of objects, so we only have to pay the strbuf overhead once for the iteration. So there's not the same performance penalty, and we can stick with the strbuf if we prefer it. Thanks for your feedback. I considered the same, and came to a similar conclusion. The strbuf cost is only once per alternate, so I feel on balance it's more robust to use alt-base consistently inside each function, rather than have this a more fragile special case to save allocation of only one path. Updated the test patch. Jonathon Mah m...@jonathonmah.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates
On Mon, Feb 02, 2015 at 10:33:02AM -0800, Jonathon Mah wrote: Signed-off-by: Jonathon Mah m...@jonathonmah.com --- Adjust prune test directly, much nicer. Agreed, this is much nicer. A few comments: +test_expect_success 'prune: handle alternate object database' ' This test fails, so we either need expect_failure here, or it just needs to be squashed in with the fix (I generally prefer the latter). + test_create_repo A cd A We generally prefer to chdir in a subshell, so that a failure in the test does not leave further tests in a confusing spot. Like: test_create_repo A ( cd A ... do stuff in repo ... # no need to cd .. ) .. do stuff outside repo ... + echo Hello World file1 Style nit: we prefer file1 with no space. + git add file1 + git commit -m Initial commit file1 + cd .. + git clone -l -s A B cd B -l is a noop these days. I don't think it is hurting, but I'd prefer not to propagate bad habits in our tests. diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh index 5a6e49d..d82844a 100755 --- a/t/t5710-info-alternate.sh +++ b/t/t5710-info-alternate.sh We can drop this change, then, right? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] diff: do not use creation-half of -B as a rename target candidate
Junio C Hamano gits...@pobox.com writes: * Here is what I am at the moment; I cannot quite explain (hence I cannot convince myself) why this is the right solution, but it seems to make the above sample case work without breaking any existing tests. It is possible that the tests that would break without the !p-score bit are expecting wrong results, but I didn't look at them in detail. Sadly, I think this is garbage. Do not consider creation-half of a broken pair, ever is too simple and cripples this case that starts with two files A and B that are quite different: $ git add A B $ mv A B.new $ mv B A $ mv B.new B $ git diff -B -M where the internal machinery breaks both A and B into these two file pairs: delete A(old) create A(new) delete B(old) create B(new) and then match them up to produce rename A to B rename B to A The rule need to be creation-half of a broken pair can be used as the destination of a rename, if and only if its corresponding deletion-half is used as the source of another rename elsewhere. Under that condition, a file A that is completely rewritten to become similar to another existing file B can be expressed as a rename of B, because A is renamed away to make room in the same change. Fixing this is turning out to be more complex than I originally hoped X-. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 2/2] sha1_file: fix iterating loose alternate objects
The string in 'base' contains a path suffix to a specific object; when
its value is used, the suffix must either be filled (as in
stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared
(as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb
(introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and
treated 'base' as a complete path to the base object directory,
instead of a pointer to the base of the full path string.
The trailing path after 'base' is still initialized to NUL, hiding the
bug in some common cases. Additionally the descendent
for_each_file_in_obj_subdir function swallows ENOENT, so an error only
shows if the alternate's path was last filled with a valid object
(where statting /path/to/existing/00/0bjectfile/00 fails).
Signed-off-by: Jonathon Mah m...@jonathonmah.com
---
sha1_file.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 30995e6..fcb1c4b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3396,9 +3396,13 @@ static int loose_from_alt_odb(struct
alternate_object_database *alt,
void *vdata)
{
struct loose_alt_odb_data *data = vdata;
- return for_each_loose_file_in_objdir(alt-base,
-data-cb, NULL, NULL,
-data-data);
+ int r;
+ alt-name[-1] = 0;
+ r = for_each_loose_file_in_objdir(alt-base,
+ data-cb, NULL, NULL,
+ data-data);
+ alt-name[-1] = '/';
+ return r;
}
int for_each_loose_object(each_loose_object_fn cb, void *data)
--
2.3.0.rc2.2.g184f7a0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 1/2] t5304-prune: demonstrate bug in pruning alternates
Signed-off-by: Jonathon Mah m...@jonathonmah.com --- Messed up the v2 patch, sorry. t/t5304-prune.sh | 13 + 1 file changed, 13 insertions(+) diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh index e32e46d..e825be7 100755 --- a/t/t5304-prune.sh +++ b/t/t5304-prune.sh @@ -253,4 +253,17 @@ test_expect_success 'prune .git/shallow' ' test_path_is_missing .git/shallow ' +test_expect_success 'prune: handle alternate object database' ' + test_create_repo A cd A + echo Hello World file1 + git add file1 + git commit -m Initial commit file1 + cd .. + git clone -l -s A B cd B + echo foo bar file2 + git add file2 + git commit -m next commit file2 + git prune +' + test_done -- 2.3.0.rc2.2.g184f7a0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables
Jeff King p...@peff.net writes: I didn't reply to the latter part of this message yesterday, because I wanted to think more on it. But is it such a bad thing to have them in conflict? Can't we define a set of rules that does what people expects? For example, by the last one wins principle, any time we see whitespace.tab-in-indent, it clears the setting of whitespace.indent-with-non-tab, and vice versa. This isn't represented syntactically in the config file, ... I agree. Both one-variable-per-knob and value-with-list-of-knobs do not express the semantic linkage between knobs; once we convince the users that one-variable-per-knob format does not mean they represent independent and orthgonal settings, the issue becomes a trade-off between * Is it concise to let end users skim through? * Is it easy to parse by scripters? By the way, this is the exact case I am concerned about for fsck.*. Our use case at GitHub would be something like: a. We set up sane defaults for fsck.* in /etc/gitconfig b. User complains that we will not accept their push, which contains objects with malformed committers. c. Support investigates, determines that the malformed objects are part of a well-established history, and that they are OK to enter. d. We relax fsck.committerIdent in that repo's $GIT_DIR/config file. Copy-and-pasting the rest of the rules from (a) into the repo config file in step (d) is not ideal. It probably can be worked around by the later-one-wins rule per item, e.g. after seeing fsck.ignore = A B C in /etc/gitconfig and then seeing fsck.error = B in $GIT_DIR/config, the latter will flip the three-way radio button for B from ignore to error (the other possible setting of the radio button is 'warn'), while leaving the three-way radio buttons for A and C set to ignore. We can (and have to) do the same with fsck.B = ignore in /etc/gitconfig that gets overridden by fsck.B = error in $GIT_DIR/config, and that comes _free_, which makes it an attractive proposition. As I already said, I am fine with fsck.missingTagger = ignore. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4] sha1_file: fix iterating loose alternate objects
The string in 'base' contains a path suffix to a specific object; when
its value is used, the suffix must either be filled (as in
stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared
(as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb
(introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and
treated 'base' as a complete path to the base object directory,
instead of a pointer to the base of the full path string.
The trailing path after 'base' is still initialized to NUL, hiding the
bug in some common cases. Additionally the descendent
for_each_file_in_obj_subdir function swallows ENOENT, so an error only
shows if the alternate's path was last filled with a valid object
(where statting /path/to/existing/00/0bjectfile/00 fails).
Signed-off-by: Jonathon Mah m...@jonathonmah.com
---
Squashed test and fix.
sha1_file.c | 10 +++---
t/t5304-prune.sh | 14 ++
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 30995e6..fcb1c4b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3396,9 +3396,13 @@ static int loose_from_alt_odb(struct
alternate_object_database *alt,
void *vdata)
{
struct loose_alt_odb_data *data = vdata;
- return for_each_loose_file_in_objdir(alt-base,
-data-cb, NULL, NULL,
-data-data);
+ int r;
+ alt-name[-1] = 0;
+ r = for_each_loose_file_in_objdir(alt-base,
+ data-cb, NULL, NULL,
+ data-data);
+ alt-name[-1] = '/';
+ return r;
}
int for_each_loose_object(each_loose_object_fn cb, void *data)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index e32e46d..c65cf9b 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -253,4 +253,18 @@ test_expect_success 'prune .git/shallow' '
test_path_is_missing .git/shallow
'
+test_expect_success 'prune: handle alternate object database' '
+ test_create_repo A
+ (cd A
+ echo Hello World file1
+ git add file1
+ git commit -m Initial commit file1)
+ git clone -s A B
+ (cd B
+ echo foo bar file2
+ git add file2
+ git commit -m next commit file2
+ git prune)
+'
+
test_done
--
2.3.0.rc2.2.g184f7a0
--
To unsubscribe from this list: send the line 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: [PATCHv4] sha1_file: fix iterating loose alternate objects
On Mon, Feb 02, 2015 at 10:48:12AM -0800, Jonathon Mah wrote: The string in 'base' contains a path suffix to a specific object; when its value is used, the suffix must either be filled (as in stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared (as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb (introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and treated 'base' as a complete path to the base object directory, instead of a pointer to the base of the full path string. The trailing path after 'base' is still initialized to NUL, hiding the bug in some common cases. Additionally the descendent for_each_file_in_obj_subdir function swallows ENOENT, so an error only shows if the alternate's path was last filled with a valid object (where statting /path/to/existing/00/0bjectfile/00 fails). Signed-off-by: Jonathon Mah m...@jonathonmah.com --- Squashed test and fix. Thanks, this version looks good to me. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] CodingGuidelines: describe naming rules for configuration variables
Michael Haggerty mhag...@alum.mit.edu writes: You make an interesting point: values that start as a list of independent booleans can grow dependencies over time. In retrospect, ISTM that a better interface for the indentation-related whitespace settings would have been something like * whitespace.indent -- a single value chosen from tabs-only | spaces-only | tabs-when-possible | anything * whitespace.tabwidth -- an integer value This would have made the mutual-exclusivity of those choices manifest in the style of configuration rather than hoping that the user notices that his settings contradict each other. Let's dig into this example some more by imagining some other hypothetical future extensions. Let's not; that line of thought entirely misses the point. If you start from one set of variables, you can define a structure (e.g. there are indentation-related and you must choose only one among them) over it after the fact. Once you have chosen a structure, you have to live with it. Either you make sure that a structure itself is extensible, or you make sure you accept a new variable only if it fits within a structure. Either way, you lose. You cannot predict the future, and you do not want to constrain those who will contribute to the project in the future. My aversion to one-variable-per-knob was primarily against the because that is how the variables are internally represented; a collection of enums that can be independently set argument. If we assume that one-variable-per-knob style implies variables that can be independently set, that _is_ defining the structure the future work has to live within. But as I and Peff discussed in the other sub(sub)thread, having two variables placed flatly in the namespace, e.g. ws.indentWithTab and ws.noTabInIndent, does not have to mean they are independent. And the opposite is also true; having these two knobs as possible elements of the value of a single variable does not imply they always have meaningful interactions. So I am fine with fsck.missingTagger = ignore/warn/error, as long as the argument that supports the style is not because fsck.missingTagger and fsck.malformedIdent are independent. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv5] sha1_file: fix iterating loose alternate objects
The string in 'base' contains a path suffix to a specific object; when
its value is used, the suffix must either be filled (as in
stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared
(as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb
(introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and
treated 'base' as a complete path to the base object directory,
instead of a pointer to the base of the full path string.
The trailing path after 'base' is still initialized to NUL, hiding the
bug in some common cases. Additionally the descendent
for_each_file_in_obj_subdir function swallows ENOENT, so an error only
shows if the alternate's path was last filled with a valid object
(where statting /path/to/existing/00/0bjectfile/00 fails).
Signed-off-by: Jonathon Mah m...@jonathonmah.com
---
Simplified test per Junio (verified that it fails before and passes now).
Punting on Jeff's more elaborate example.
sha1_file.c | 10 +++---
t/t5304-prune.sh | 8
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 30995e6..fcb1c4b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3396,9 +3396,13 @@ static int loose_from_alt_odb(struct
alternate_object_database *alt,
void *vdata)
{
struct loose_alt_odb_data *data = vdata;
- return for_each_loose_file_in_objdir(alt-base,
-data-cb, NULL, NULL,
-data-data);
+ int r;
+ alt-name[-1] = 0;
+ r = for_each_loose_file_in_objdir(alt-base,
+ data-cb, NULL, NULL,
+ data-data);
+ alt-name[-1] = '/';
+ return r;
}
int for_each_loose_object(each_loose_object_fn cb, void *data)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index e32e46d..0794d33 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -253,4 +253,12 @@ test_expect_success 'prune .git/shallow' '
test_path_is_missing .git/shallow
'
+test_expect_success 'prune: handle alternate object database' '
+ test_create_repo A
+ git -C A commit --allow-empty -m initial commit
+ git clone --shared A B
+ git -C B commit --allow-empty -m next commit
+ git -C B prune
+'
+
test_done
--
2.3.0.rc2.2.g184f7a0
--
To unsubscribe from this list: send the line 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: [PATCHv5] sha1_file: fix iterating loose alternate objects
Jeff King p...@peff.net writes: So I actually do not see how the situation would come up in practice, and possibly we could drop the iteration of the alternates' loose objects entirely from this code. But certainly that is orthogonal to Jonathon's fix (which is a true regression for the less-exotic case that his test demonstrates). Sure. This needs to go to both 'maint' and 'master', right? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5] sha1_file: fix iterating loose alternate objects
On Mon, Feb 02, 2015 at 12:49:23PM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: So I actually do not see how the situation would come up in practice, and possibly we could drop the iteration of the alternates' loose objects entirely from this code. But certainly that is orthogonal to Jonathon's fix (which is a true regression for the less-exotic case that his test demonstrates). Sure. This needs to go to both 'maint' and 'master', right? Yes (on the jk/prune-mtime topic). -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 2/2] Makes _do_open2 set _gitdir to actual path
If _is_git had to follow gitdir: ... files to reach the actual Git
directory, we set _gitdir to that final path.
---
lib/choose_repository.tcl | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 49ff641..641068d 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -338,13 +338,17 @@ method _git_init {} {
return 1
}
-proc _is_git {path} {
+proc _is_git {path {outdir_var }} {
+ if {$outdir_var ne } {
+ upvar 1 $outdir_var outdir
+ }
if {[file isfile $path]} {
set fp [open $path r]
gets $fp line
close $fp
if {[regexp ^gitdir: (.+)$ $line line link_target]} {
- return [_is_git [file join [file dirname $path]
$link_target]]
+ set link_target_abs [file join [file dirname $path]
$link_target]
+ return [_is_git $link_target_abs outdir]
}
return 0
}
@@ -352,12 +356,14 @@ proc _is_git {path} {
if {[file exists [file join $path HEAD]]
[file exists [file join $path objects]]
[file exists [file join $path config]]} {
+ set outdir $path
return 1
}
if {[is_Cygwin]} {
if {[file exists [file join $path HEAD]]
[file exists [file join $path objects.lnk]]
[file exists [file join $path config.lnk]]} {
+ set outdir $path
return 1
}
}
@@ -1103,7 +1109,7 @@ method _open_local_path {} {
}
method _do_open2 {} {
- if {![_is_git [file join $local_path .git]]} {
+ if {![_is_git [file join $local_path .git] actualgit]} {
error_popup [mc Not a Git repository: %s [file tail
$local_path]]
return
}
@@ -1116,7 +1122,7 @@ method _do_open2 {} {
}
_append_recentrepos [pwd]
- set ::_gitdir .git
+ set ::_gitdir $actualgit
set ::_prefix {}
set done 1
}
--
1.9.5.msysgit.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 1/2] Fixes _is_git
Function _git_dir would previously fail to accept a gitdir: ... file
as a valid Git repository.
---
lib/choose_repository.tcl | 10 ++
1 file changed, 10 insertions(+)
diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 92d6022..49ff641 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -339,6 +339,16 @@ method _git_init {} {
}
proc _is_git {path} {
+ if {[file isfile $path]} {
+ set fp [open $path r]
+ gets $fp line
+ close $fp
+ if {[regexp ^gitdir: (.+)$ $line line link_target]} {
+ return [_is_git [file join [file dirname $path]
$link_target]]
+ }
+ return 0
+ }
+
if {[file exists [file join $path HEAD]]
[file exists [file join $path objects]]
[file exists [file join $path config]]} {
--
1.9.5.msysgit.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 2/2] sha1_file: fix iterating loose alternate objects
On Sun, Feb 01, 2015 at 01:55:33PM -0800, Jonathon Mah wrote:
The string in 'base' contains a path suffix to a specific object; when
its value is used, the suffix must either be filled (as in
stat_sha1_file, open_sha1_file, check_and_freshen_nonlocal) or cleared
(as in prepare_packed_git) to avoid junk at the end. loose_from_alt_odb
(introduced in 660c889e46d185dc98ba78963528826728b0a55d) did neither and
treated 'base' as a complete path to the base object directory,
instead of a pointer to the base of the full path string.
The trailing path after 'base' is still initialized to NUL, hiding the
bug in some common cases. Additionally the descendent
for_each_file_in_obj_subdir function swallows ENOENT, so an error only
shows if the alternate's path was last filled with a valid object
(where statting /path/to/existing/00/0bjectfile/00 fails).
Thanks for catching this, and for a nice explanation.
diff --git a/sha1_file.c b/sha1_file.c
index 30995e6..fcb1c4b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3396,9 +3396,13 @@ static int loose_from_alt_odb(struct
alternate_object_database *alt,
void *vdata)
{
struct loose_alt_odb_data *data = vdata;
- return for_each_loose_file_in_objdir(alt-base,
- data-cb, NULL, NULL,
- data-data);
+ int r;
+ alt-name[-1] = 0;
+ r = for_each_loose_file_in_objdir(alt-base,
+ data-cb, NULL, NULL,
+ data-data);
+ alt-name[-1] = '/';
+ return r;
}
I think this is probably the best fix, and is the pattern we use
elsewhere when touching alt-base.
We _could_ further change this to have for_each_loose_file_in_objdir
actually use alt-base as its scratch buffer, writing the object
filenames into the end of it (i.e., what it was designed for). But:
1. We still need a strbuf scratch-buffer for the non-alternate object
directory. So we'd have to push more code there to over-allocate
the buffer, and then for_each_loose_file_in_objdir would assume
we always feed it a buffer with the extra slop. That would work,
but I find the strbuf approach a little safer; there's not an
implicit over-allocation far away in the code preventing us from
overflowing a buffer.
2. The reason for the existing alt-base behavior is that the
sha1_file code gets fed objects one at a time, and don't want to
pay strbuf overhead for each. With the iterator, we know we are
going to hit a bunch of objects, so we only have to pay the strbuf
overhead once for the iteration. So there's not the same
performance penalty, and we can stick with the strbuf if we prefer
it.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t5710-info-alternate: demonstrate bug in unpacked pruning
On Sun, Feb 01, 2015 at 01:55:00PM -0800, Jonathon Mah wrote:
Signed-off-by: Jonathon Mah m...@jonathonmah.com
---
t/t5710-info-alternate.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh
index 5a6e49d..d82844a 100755
--- a/t/t5710-info-alternate.sh
+++ b/t/t5710-info-alternate.sh
@@ -18,6 +18,7 @@ reachable_via() {
test_valid_repo() {
git fsck --full fsck.log
+ git prune
test_line_count = 0 fsck.log
}
@@ -47,8 +48,7 @@ test_expect_success 'preparing third repository' \
'git clone -l -s B C cd C
echo Goodbye, cruel world file3
git add file3
-git commit -m one more file3
-git repack -a -d -l
+git commit -m one more without packing file3
git prune'
Modifying a test like this makes me a little nervous because now the old
test is not checking the same thing (pruning when we are packed), and
it's not obvious whether the packing was important to the original test.
And it's not clear that this change is testing a totally unrelated
thing. I haven't looked closely, but would it be hard to introduce a
new test that more explicitly checks for the breakage?
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Relative paths don't work in .gitignore as would be expected.
Stefan Beller stefanbel...@gmail.com writes: On 01.02.2015 14:51, /#!/JoePea wrote: I have this in my .gitignore: ./*.js I would expect that to cause git to ignore .js files in the same folder as .gitignore, but it doesn't do anything. However, this works: /*.js I'm not sure what this actually means because a leading slash is the root of some filesystem, Isn't gitignore(5) documentation reasonably clear? - If the pattern ends with a slash, it is removed for the purpose of the following description, but it would only find a match with a directory. In other words, foo/ will match a directory foo and paths underneath it, but will not match a regular file or a symbolic link foo (this is consistent with the way how pathspec works in general in Git). - If the pattern does not contain a slash /, Git treats it as a shell glob pattern and checks for a match against the pathname relative to the location of the .gitignore file (relative to the toplevel of the work tree if not from a .gitignore file). - A leading slash matches the beginning of the pathname. For example, /*.c matches cat-file.c but not mozilla-sha1/sha1.c. That's true, though you'd never (barely?) git version control an entire file system? When you have the entire file system under /.git, /var/ still would be the right way to spell a pattern to match only a directory (because of the trailing '/') whose name matches 'var' and lives in the root level of the filesystem (because of the leading '/' anchors the pattern to the same level as the file the pattern appears in, i.e. /.gitignore) and no other place. (from man gitignore, though reading that and not finding a './' it may need improvement We do not allow relative pathname traversal with . or .., do we? I would be very hesitant to special case ./*.js to mean *.js files in the same directory as .gitignore appears, as I think it risks intelligent readers to infer ../foo/*.js or ../*.js would take effect, when placed in bar/.gitignore. A design that spreads an incorrect assumption/expectation is not a good one, I would have to say. -- To unsubscribe from this list: send the line 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-gui] bug report: Open existing repository dialog fails on submodules
Hi,
On Sat, Jan 31, 2015 at 10:46 AM, Rémi Rampin remiram...@gmail.com wrote:
Hi,
This bug report concerns git-gui. Apologies if this is not the right
mailing-list.
By submodule I mean a repository for which .git is not a regular Git
directory, but rather a gitdir: ... file.
While running git gui from such a directory will work fine, trying
to open it from the choose_repository window will fail with Not a Git
repository. This is because of the simplistic implementation of proc
_is_git in lib/choose_repository.tcl.
I suggest fixing that function, or using Git directly to perform that
check, for instance checking git rev-parse --show-toplevel. I'd
attempt a patch but my tcl-fu is weak.
I would have thought the following would work
--- 8 ---
Subject: [PATCH] git-gui: use git rev-parse to validate paths
The current _is_git function to validate a path as a git repository does
not handle a gitfiles which have been used for submodules for some time.
Instead of using a custom function let's just ask git rev-parse.
Signed-off-by: Chris Packham chris.pack...@alliedtelesis.co.nz
---
lib/choose_repository.tcl | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 92d6022..944ab50 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -339,19 +339,12 @@ method _git_init {} {
}
proc _is_git {path} {
- if {[file exists [file join $path HEAD]]
- [file exists [file join $path objects]]
- [file exists [file join $path config]]} {
+ puts $path
+ if {[catch {exec git rev-parse --resolve-git-dir $path}]} {
+ return 0
+ } else {
return 1
}
- if {[is_Cygwin]} {
- if {[file exists [file join $path HEAD]]
- [file exists [file join $path objects.lnk]]
- [file exists [file join $path config.lnk]]} {
- return 1
- }
- }
- return 0
}
proc _objdir {path} {
--
2.3.0.rc2
--- 8 ---
But it actually looks like git rev-parse --resolve-git-dir $path needs
to be run inside a git repository _any_ git repository, which seems a
bit backwards to me.
$ cd
$ git rev-parse --resolve-git-dir ~/src/git/.git
fatal: Not a git repository (or any parent up to mount point /home)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
$ cd ~/src/git
$ git rev-parse --resolve-git-dir ~/src/git-gui/.git
/home/chrisp/src/git-gui/.git
So one potential fix git a gui-gui bug, one new(?) bug in git rev-parse.
--
To unsubscribe from this list: send the line 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-gui] bug report: Open existing repository dialog fails on submodules
On Mon, Feb 2, 2015 at 9:41 PM, Chris Packham judge.pack...@gmail.com wrote:
Hi,
On Sat, Jan 31, 2015 at 10:46 AM, Rémi Rampin remiram...@gmail.com wrote:
Hi,
This bug report concerns git-gui. Apologies if this is not the right
mailing-list.
By submodule I mean a repository for which .git is not a regular Git
directory, but rather a gitdir: ... file.
While running git gui from such a directory will work fine, trying
to open it from the choose_repository window will fail with Not a Git
repository. This is because of the simplistic implementation of proc
_is_git in lib/choose_repository.tcl.
I suggest fixing that function, or using Git directly to perform that
check, for instance checking git rev-parse --show-toplevel. I'd
attempt a patch but my tcl-fu is weak.
I would have thought the following would work
--- 8 ---
Subject: [PATCH] git-gui: use git rev-parse to validate paths
The current _is_git function to validate a path as a git repository does
not handle a gitfiles which have been used for submodules for some time.
Instead of using a custom function let's just ask git rev-parse.
Signed-off-by: Chris Packham chris.pack...@alliedtelesis.co.nz
---
lib/choose_repository.tcl | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index 92d6022..944ab50 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -339,19 +339,12 @@ method _git_init {} {
}
proc _is_git {path} {
- if {[file exists [file join $path HEAD]]
- [file exists [file join $path objects]]
- [file exists [file join $path config]]} {
+ puts $path
+ if {[catch {exec git rev-parse --resolve-git-dir $path}]} {
+ return 0
+ } else {
return 1
}
- if {[is_Cygwin]} {
- if {[file exists [file join $path HEAD]]
- [file exists [file join $path objects.lnk]]
- [file exists [file join $path config.lnk]]} {
- return 1
- }
- }
- return 0
}
proc _objdir {path} {
--
2.3.0.rc2
--- 8 ---
But it actually looks like git rev-parse --resolve-git-dir $path needs
to be run inside a git repository _any_ git repository, which seems a
bit backwards to me.
$ cd
$ git rev-parse --resolve-git-dir ~/src/git/.git
fatal: Not a git repository (or any parent up to mount point /home)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
$ cd ~/src/git
$ git rev-parse --resolve-git-dir ~/src/git-gui/.git
/home/chrisp/src/git-gui/.git
So one potential fix git a gui-gui bug, one new(?) bug in git rev-parse.
Not a new one. Happens in 1.9.1. Still a bit counter-intuitive IMO.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
