Re: propagating repo corruption across clone
On Wed, Mar 27, 2013 at 9:17 AM, Junio C Hamano wrote: > To be paranoid, you may want to set transfer.fsckObjects to true, > perhaps in your ~/.gitconfig. do we have any numbers on the overhead of this? Even a "guesstimate" will do... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] checkout: avoid unnecessary match_pathspec calls
In checkout_paths() we do this
- for all updated items, call match_pathspec
- for all items, call match_pathspec (inside unmerge_cache)
- for all items, call match_pathspec (for showing "path .. is unmerged)
- for updated items, call match_pathspec and update paths
That's a lot of duplicate match_pathspec(s) and the function is not
exactly cheap to be called so many times, especially on large indexes.
This patch makes it call match_pathspec once per updated index entry,
save the result in ce_flags and reuse the results in the following
loops.
The changes in 0a1283b (checkout $tree $path: do not clobber local
changes in $path not in $tree - 2011-09-30) limit the affected paths
to ones we read from $tree. We do not do anything to other modified
entries in this case, so the "for all items" above could be modified
to "for all updated items". But..
The command's behavior now is modified slightly: unmerged entries that
match $path, but not updated by $tree, are now NOT touched. Although
this should be considered a bug fix, not a regression. A new test is
added for this change.
And while at there, free ps_matched after use.
The following command is tested on webkit, 215k entries. The pattern
is chosen mainly to make match_pathspec sweat:
git checkout -- "*[a-zA-Z]*[a-zA-Z]*[a-zA-Z]*"
before after
real0m3.493s0m2.737s
user0m2.239s0m1.586s
sys 0m1.252s0m1.151s
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Changes from v2: a new test and some note about matching twice in
"checkout $tree $path", once in read_tree_some and once checkout_paths.
We may be able avoid match_pathspec entirely in this case when
tree_entry_interesting learns to fill ps_matched.
builtin/checkout.c| 43 ---
cache.h | 1 +
resolve-undo.c| 19 ++-
resolve-undo.h| 1 +
t/t2022-checkout-paths.sh | 21 +
5 files changed, 77 insertions(+), 8 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a9c1b5a..f8033f4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -271,24 +271,55 @@ static int checkout_paths(const struct checkout_opts
*opts,
;
ps_matched = xcalloc(1, pos);
+ /*
+* Make sure all pathspecs participated in locating the paths
+* to be checked out.
+*/
for (pos = 0; pos < active_nr; pos++) {
struct cache_entry *ce = active_cache[pos];
+ ce->ce_flags &= ~CE_MATCHED;
if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))
+ /*
+* "git checkout tree-ish -- path", but this entry
+* is in the original index; it will not be checked
+* out to the working tree and it does not matter
+* if pathspec matched this entry. We will not do
+* anything to this entry at all.
+*/
continue;
- match_pathspec(opts->pathspec, ce->name, ce_namelen(ce), 0,
ps_matched);
+ /*
+* Either this entry came from the tree-ish we are
+* checking the paths out of, or we are checking out
+* of the index.
+*
+* If it comes from the tree-ish, we already know it
+* matches the pathspec and could just stamp
+* CE_MATCHED to it from update_some(). But we still
+* need ps_matched and read_tree_recursive (and
+* eventually tree_entry_interesting) cannot fill
+* ps_matched yet. Once it can, we can avoid calling
+* match_pathspec() for _all_ entries when
+* opts->source_tree != NULL.
+*/
+ if (match_pathspec(opts->pathspec, ce->name, ce_namelen(ce),
+ 0, ps_matched))
+ ce->ce_flags |= CE_MATCHED;
}
- if (report_path_error(ps_matched, opts->pathspec, opts->prefix))
+ if (report_path_error(ps_matched, opts->pathspec, opts->prefix)) {
+ free(ps_matched);
return 1;
+ }
+ free(ps_matched);
/* "checkout -m path" to recreate conflicted state */
if (opts->merge)
- unmerge_cache(opts->pathspec);
+ unmerge_marked_index(&the_index);
/* Any unmerged paths? */
for (pos = 0; pos < active_nr; pos++) {
struct cache_entry *ce = active_cache[pos];
- if (match_pathspec(opts->pathspec, ce->name, ce_namelen(ce), 0,
NULL)) {
+ if (ce->ce_flags & CE_MATCHED) {
if (!ce_stage(ce))
continue;
if (opts->force) {
@@ -313,9 +344,7 @@ static int checkout_paths(const str
Re: [PATCH] git-svn: Support custom tunnel schemes instead of SSH only
Sebastian Schuberth wrote:
> This originates from an msysgit pull request, see:
>
> https://github.com/msysgit/git/pull/58
>
> Signed-off-by: Eric Wieser
> Signed-off-by: Sebastian Schuberth
Thanks, looks obviously correct.
Signed-off-by: Eric Wong
> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
> index 049c97b..6a212eb 100644
> --- a/perl/Git/SVN/Ra.pm
> +++ b/perl/Git/SVN/Ra.pm
> @@ -295,7 +295,7 @@ sub gs_do_switch {
> my $full_url = add_path_to_url( $self->url, $path );
> my ($ra, $reparented);
>
> - if ($old_url =~ m#^svn(\+ssh)?://# ||
> + if ($old_url =~ m#^svn(\+\w+)?://# ||
> ($full_url =~ m#^https?://# &&
>canonicalize_url($full_url) ne $full_url)) {
> $_[0] = undef;
> --
Junio:
The following changes since commit 2bba2f0e6542d541e9f27653d8c9d5fc8d0e679c:
More topics from the second batch for 1.8.3 (2013-03-26 13:16:11 -0700)
are available in the git repository at:
git://git.bogomips.org/git-svn.git master
for you to fetch changes up to 3747c015704399dea1aa7ae6569a507e5727e20b:
git-svn: Support custom tunnel schemes instead of SSH only (2013-03-27
04:28:04 +)
Sebastian Schuberth (1):
git-svn: Support custom tunnel schemes instead of SSH only
perl/Git/SVN/Ra.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] attribute regression fix for maint-1.8.1 and upward
On Wed, Mar 27, 2013 at 10:57 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> How about this? git_check_attr() now takes dtype as an argument >> and the caller must not add the trailing slash. This could be >> split into two patches, one for git_check_attr prototype change, >> and the other the real meat. > > "git check-attr" fundamentally cannot know, but aside from that do > all the callsites know if the path in question is a directory or > not? My impression was that there are some cases you do not > necessarily know. > > "Add slash when you _know_ it is a directory, but otherwise pass the > path without trailing slash." is easier to understand than "Pass > 04 if you know it is a directory, but otherwise pass 100644", > exactly because "otherwise" in both of these instructions include > the case where the path in question _is_ a directory (you just do > not know what it is). > > I do not particularly like the "trailing slash on the basename" > approach, but it feels less bad than passing dtype down. Fair enough. I'll rebase my changes on top of yours as long term cleanup. Maybe I can make nwildmatch take patternlen too. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] attribute regression fix for maint-1.8.1 and upward
Duy Nguyen writes: > How about this? git_check_attr() now takes dtype as an argument > and the caller must not add the trailing slash. This could be > split into two patches, one for git_check_attr prototype change, > and the other the real meat. "git check-attr" fundamentally cannot know, but aside from that do all the callsites know if the path in question is a directory or not? My impression was that there are some cases you do not necessarily know. "Add slash when you _know_ it is a directory, but otherwise pass the path without trailing slash." is easier to understand than "Pass 04 if you know it is a directory, but otherwise pass 100644", exactly because "otherwise" in both of these instructions include the case where the path in question _is_ a directory (you just do not know what it is). I do not particularly like the "trailing slash on the basename" approach, but it feels less bad than passing dtype down. -- To unsubscribe from this list: send the line "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: propagating repo corruption across clone
Rich Fromm writes: > Jeff King wrote >> Fundamentally the problem is >> that the --local transport is not safe from propagating corruption, and >> should not be used if that's a requirement. > > I've read Jeff Mitchell's blog post, his update, relevant parts of the > git-clone(1) man page, and a decent chunk of this thread, and I'm still not > clear on one thing. Is the danger of `git clone --mirror` propagating > corruption only true when using the --local option ? If you use --local, that is equivalent to "cp -R". Your corruption in the source will faithfully be byte-for-byte copied to the destination. If you do not (and in your case you have two different machines), unless you are using the long deprecated rsync transport (which again is the same as "cp -R"), transport layer will notice object corruption. See Jeff's analysis earlier in the thread. If you are lucky (or unlucky, depending on how you look at it), the corruption you have in your object store may affect objects that are needed to check out the version at the tip of the history, and "git checkout" that happens as the last step of cloning may loudly complain, but that just means you can immediately notice the breakage in that case. You may be unlucky and the corruption may not affect objects that are needed to check out the tip. The initial checkout will succeed as if nothing is wrong, but the corruption in your object store is still there nevertheless. "git log -p --all" or "git fsck" will certainly be unhappy. The difference between --mirror and no --mirror is a red herring. You may want to ask Jeff Mitchell to remove the mention of it; it only adds to the confusion without helping users. If you made byte-for-byte copy of corrupt repository, it wouldn't make any difference if the first "checkout" notices it. To be paranoid, you may want to set transfer.fsckObjects to true, perhaps in your ~/.gitconfig. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
More detailed error message for 403 forbidden.
Currently, if user tried to access a git repository via HTTP and it fails because the user's permission is not enough to access the repository, git client tells that http request failed and the error was 403 forbidden. But It is not enough for user to understand why it fails, especially if the user don't know the username because git-credential-osxkeychain authenticate implicitly without user knowing. It would be much better if git client shows response body which might include an explanation of the failure. For example, before: $ git clone http://localhost/foo/bar error: The requested URL returned error: 403 while accessing http://localhost/foo/bar fatal: HTTP request failed after: $ git clone http://localhost/foo/bar error: The requested URL returned error: 403 while accessing http://localhost/foo/bar remote: User 'me' does not have enough permission to access the repository. fatal: HTTP request failed -- To unsubscribe from this list: send the line "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/4] attr.c::path_matches(): the basename is part of the pathname
On Wed, Mar 27, 2013 at 1:49 AM, Jeff King wrote:
> On Tue, Mar 26, 2013 at 11:39:28AM -0700, Junio C Hamano wrote:
>
>> The function takes two strings (pathname and basename) as if they
>> are independent strings, but in reality, the latter is always
>> pointing into a substring in the former.
>>
>> Clarify this relationship by expressing the latter as an offset into
>> the former.
>>
>> Signed-off-by: Junio C Hamano
>
> This is a huge improvement in maintainability. My initial fix attempt
> was to just xstrdup() the strings (knowing that the performance would be
> horrible, but I was still investigating correctness issues at that
> point). And of course I ran into this same issue as I tried to make a
> copy of pathname.
>
> So even without the rest of the fix, this is definitely a good idea. :)
match_{base,path}name and their exclude callers do the same thing. I
guess I'm used to it and did not see the maintainability issue. Maybe
we should do the same there too.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash
On Wed, Mar 27, 2013 at 4:33 AM, Jeff King wrote: > Hmm. match_pathname does have this: > > /* > * baselen does not count the trailing slash. base[] may or > * may not end with a trailing slash though. > */ > if (pathlen < baselen + 1 || > (baselen && pathname[baselen] != '/') || > strncmp_icase(pathname, base, baselen)) > return 0; > > which seems to imply that the trailing slash is important here, and that > we should not drop it when passing the path to match_pathname. I'm > still trying to figure out exactly what it is that the extra slash check > is for, and whether it might not have the same problem. The "may not end with a trailing slash" can only happen when baselen == 0. And that rule is documented in code, at this line in last_exclude_matching_from_list: assert(x->baselen == 0 || x->base[x->baselen - 1] == '/'); -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: propagating repo corruption across clone
Hi, Rich Fromm wrote: >The host executing the clone > command is different than the the host on which the remote repository lives, > and I am using ssh as a transport protocol. If there is corruption, can I > or can I not expect the clone operation to fail and return a non-zero exit > value? If I can not expect this, is the workaround to run `git fsck` on the > resulting clone? Is the "[transfer] fsckObjects" configuration on the host executing the clone set to true? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] attribute regression fix for maint-1.8.1 and upward
qOn Tue, Mar 26, 2013 at 11:39:27AM -0700, Junio C Hamano wrote:
> So here is an attempt to fix the unintended regression, on top of
> 9db9eecfe5c2 (attr: avoid calling find_basename() twice per path,
> 2013-01-16). It consists of four patches.
Not that I disagree with this. Just wanted to see how far the "dtype"
idea went. How about this? git_check_attr() now takes dtype as an
argument and the caller must not add the trailing slash. This could be
split into two patches, one for git_check_attr prototype change, and
the other the real meat.
-- 8< --
diff --git a/archive.c b/archive.c
index 93e00bb..ab811b3 100644
--- a/archive.c
+++ b/archive.c
@@ -112,7 +112,7 @@ static int write_archive_entry(const unsigned char *sha1,
const char *base,
write_archive_entry_fn_t write_entry = c->write_entry;
struct git_attr_check check[2];
const char *path_without_prefix;
- int err;
+ int err, dtype;
args->convert = 0;
strbuf_reset(&path);
@@ -120,18 +120,18 @@ static int write_archive_entry(const unsigned char *sha1,
const char *base,
strbuf_add(&path, args->base, args->baselen);
strbuf_add(&path, base, baselen);
strbuf_addstr(&path, filename);
- if (S_ISDIR(mode) || S_ISGITLINK(mode))
- strbuf_addch(&path, '/');
+ dtype = S_ISDIR(mode) || S_ISGITLINK(mode) ? DT_DIR : DT_REG;
path_without_prefix = path.buf + args->baselen;
setup_archive_check(check);
- if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {
+ if (!git_check_attr(path_without_prefix, dtype, ARRAY_SIZE(check),
check)) {
if (ATTR_TRUE(check[0].value))
return 0;
args->convert = ATTR_TRUE(check[1].value);
}
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
+ strbuf_addch(&path, '/');
if (args->verbose)
fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
err = write_entry(args, sha1, path.buf, path.len, mode);
diff --git a/attr.c b/attr.c
index e2f9377..ab6422c 100644
--- a/attr.c
+++ b/attr.c
@@ -255,6 +255,8 @@ static struct match_attr *parse_attr_line(const char *line,
const char *src,
&res->u.pat.patternlen,
&res->u.pat.flags,
&res->u.pat.nowildcardlen);
+ if (res->u.pat.flags & EXC_FLAG_MUSTBEDIR)
+ p[res->u.pat.patternlen] = '\0';
if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
warning(_("Negative patterns are ignored in git
attributes\n"
"Use '\\!' for literal leading
exclamation."));
@@ -657,15 +659,14 @@ static void prepare_attr_stack(const char *path, int
dirlen)
}
static int path_matches(const char *pathname, int pathlen,
- const char *basename,
+ const char *basename, int dtype,
const struct pattern *pat,
const char *base, int baselen)
{
const char *pattern = pat->pattern;
int prefix = pat->nowildcardlen;
- if ((pat->flags & EXC_FLAG_MUSTBEDIR) &&
- ((!pathlen) || (pathname[pathlen-1] != '/')))
+ if ((pat->flags & EXC_FLAG_MUSTBEDIR) && dtype != DT_DIR)
return 0;
if (pat->flags & EXC_FLAG_NODIR) {
@@ -704,7 +705,7 @@ static int fill_one(const char *what, struct match_attr *a,
int rem)
}
static int fill(const char *path, int pathlen, const char *basename,
- struct attr_stack *stk, int rem)
+ int dtype, struct attr_stack *stk, int rem)
{
int i;
const char *base = stk->origin ? stk->origin : "";
@@ -713,7 +714,7 @@ static int fill(const char *path, int pathlen, const char
*basename,
struct match_attr *a = stk->attrs[i];
if (a->is_macro)
continue;
- if (path_matches(path, pathlen, basename,
+ if (path_matches(path, pathlen, basename, dtype,
&a->u.pat, base, stk->originlen))
rem = fill_one("fill", a, rem);
}
@@ -748,20 +749,17 @@ static int macroexpand_one(int attr_nr, int rem)
* Collect all attributes for path into the array pointed to by
* check_all_attr.
*/
-static void collect_all_attrs(const char *path)
+static void collect_all_attrs(const char *path, int dtype)
{
struct attr_stack *stk;
int i, pathlen, rem, dirlen;
- const char *basename, *cp, *last_slash = NULL;
+ const char *basename, *cp;
- for (cp = path; *cp; cp++) {
- if (*cp == '/' && cp[1])
- last_slash = cp;
- }
- pathlen = cp - path;
- if (last_slash) {
- basename = last_slash + 1;
- dirlen = last_slash -
Re: [PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters
On Tue, Mar 26, 2013 at 03:33:40PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > So I think your series is the right direction, but we would want to > > factor out the allocation code and use it from match_pathname, as well. > > I am deep into today's integration cycle, so perhaps in the meantime > you can help with a follow-up patch ;-)? Your afternoon integration is my dinner-time. :) I'll try to look at it tomorrow. -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 10/9] clone: leave repo in place after checkout errors
On Tue, Mar 26, 2013 at 03:32:59PM -0700, Jonathan Nieder wrote:
> > +static const char junk_leave_repo_msg[] =
> > +N_("The remote repository was cloned successfully, but there was\n"
> > + "an error checking out the HEAD branch. The repository has been left
> > in\n"
> > + "place but the working tree may be in an inconsistent state. You can\n"
> > + "can inspect the contents with:\n"
> > + "\n"
> > + "git status\n"
> > + "\n"
> > + "and retry the checkout with\n"
> > + "\n"
> > + "git checkout -f HEAD\n"
> > + "\n");
>
> Can this be made more precise? I don't know what it means for the
> working tree to be in an inconsistent state: do you mean that some files
> might be partially checked out or not have been checked out at all yet?
It means that we died during the checkout procedure, and we don't have
any idea what was left. Maybe something, maybe nothing. Maybe an index,
maybe not.
> error: Clone succeeded, but checkout failed.
> hint: You can inspect what was checked out with "git status".
> hint: To retry the checkout, run "git checkout -f HEAD".
That is certainly more succint, if not more precise. I'd be fine with
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: propagating repo corruption across clone
Jeff King wrote > Fundamentally the problem is > that the --local transport is not safe from propagating corruption, and > should not be used if that's a requirement. I've read Jeff Mitchell's blog post, his update, relevant parts of the git-clone(1) man page, and a decent chunk of this thread, and I'm still not clear on one thing. Is the danger of `git clone --mirror` propagating corruption only true when using the --local option ? Specifically, in my case, I'm using `git clone --mirror`, but I'm *not* using --local, nor am I using --no-hardlinks. The host executing the clone command is different than the the host on which the remote repository lives, and I am using ssh as a transport protocol. If there is corruption, can I or can I not expect the clone operation to fail and return a non-zero exit value? If I can not expect this, is the workaround to run `git fsck` on the resulting clone? -- View this message in context: http://git.661346.n2.nabble.com/propagating-repo-corruption-across-clone-tp7580504p7580771.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Mar 2013, #07; Tue, 26)
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--
[Graduated to "master"]
* ap/maint-diff-rename-avoid-overlap (2013-03-06) 3 commits
(merged to 'next' on 2013-03-19 at c3276cf)
+ tests: make sure rename pretty print works
+ diff: prevent pprint_rename from underrunning input
+ diff: Fix rename pretty-print when suffix and prefix overlap
Originally merged to 'next' on 2013-03-06
The logic used by "git diff -M --stat" to shorten the names of
files before and after a rename did not work correctly when the
common prefix and suffix between the two filenames overlapped.
* jc/describe (2013-02-28) 1 commit
(merged to 'next' on 2013-03-19 at 89e6e47)
+ describe: --match= must limit the refs even when used with --all
Originally merged to 'next' on 2013-03-05
The "--match=" option of "git describe", when used with
"--all" to allow refs that are not annotated tags to be used as a
base of description, did not restrict the output from the command
to those that match the given pattern.
We may want to have a looser matching that does not restrict to tags,
but that can be done as a follow-up topic; this step is purely a bugfix.
* jc/maint-reflog-expire-clean-mark-typofix (2013-03-05) 1 commit
(merged to 'next' on 2013-03-19 at a4f9eac)
+ reflog: fix typo in "reflog expire" clean-up codepath
Originally merged to 'next' on 2013-03-07
In "git reflog expire", REACHABLE bit was not cleared from the
correct objects.
* jc/push-follow-tag (2013-03-05) 4 commits
(merged to 'next' on 2013-03-19 at d302a10)
+ push: --follow-tags
+ commit.c: use clear_commit_marks_many() in in_merge_bases_many()
+ commit.c: add in_merge_bases_many()
+ commit.c: add clear_commit_marks_many()
Originally merged to 'next' on 2013-03-09
The new "--follow-tags" option tells "git push" to push relevant
annotated tags when pushing branches out.
* jc/reflog-reverse-walk (2013-03-23) 4 commits
(merged to 'next' on 2013-03-25 at 1bcc1c4)
+ refs.c: fix fread error handling
(merged to 'next' on 2013-03-19 at 25beb2a)
+ reflog: add for_each_reflog_ent_reverse() API
+ for_each_recent_reflog_ent(): simplify opening of a reflog file
+ for_each_reflog_ent(): extract a helper to process a single entry
(this branch is tangled with nd/branch-show-rebase-bisect-state.)
An internal function used to implement "git checkout @{-1}" was
hard to use correctly.
* jk/alias-in-bare (2013-03-08) 3 commits
(merged to 'next' on 2013-03-19 at d2b4227)
+ setup: suppress implicit "." work-tree for bare repos
+ environment: add GIT_PREFIX to local_repo_env
+ cache.h: drop LOCAL_REPO_ENV_SIZE
Originally merged to 'next' on 2013-03-09
An aliased command spawned from a bare repository that does not say
it is bare with "core.bare = yes" is treated as non-bare by mistake.
* jk/empty-archive (2013-03-10) 2 commits
(merged to 'next' on 2013-03-19 at bb4eb61)
+ archive: handle commits with an empty tree
+ test-lib: factor out $GIT_UNZIP setup
Originally merged to 'next' on 2013-03-12
"git archive" reports a failure when asked to create an archive out
of an empty tree. It would be more intuitive to give an empty
archive back in such a case.
* jk/fast-export-object-lookup (2013-03-17) 2 commits
(merged to 'next' on 2013-03-19 at 026ac3d)
+ fast-export: do not load blob objects twice
+ fast-export: rename handle_object function
Originally merged to 'next' on 2013-03-18
* jk/fully-peeled-packed-ref (2013-03-18) 4 commits
(merged to 'next' on 2013-03-19 at fa92bc7)
+ pack-refs: add fully-peeled trait
+ pack-refs: write peeled entry for non-tags
+ use parse_object_or_die instead of die("bad object")
+ avoid segfaults on parse_object failure
Originally merged to 'next' on 2013-03-18
Not that we do not actively encourage having annotated tags outside
refs/tags/ hierarchy, but they were not advertised correctly to the
ls-remote and fetch with recent version of Git.
* jk/peel-ref (2013-03-16) 3 commits
(merged to 'next' on 2013-03-19 at f0d4c16)
+ upload-pack: load non-tip "want" objects from disk
+ upload-pack: make sure "want" objects are parsed
+ upload-pack: drop lookup-before-parse optimization
Originally merged to 'next' on 2013-03-18
Recent optimization broke shallow clones.
* jk/suppress-clang-warning (2013-02-25) 1 commit
(merged to 'next' on 2013-03-19 at 1fd6858)
+ fix clang -Wtautological-compare with unsigned enum
Originally merged to 'next' on 2013-03-14
* jl/submodule-deinit (2013-03-04) 1 commit
(merged to 'next' on 2013-03-19 at d8367c5)
+ submodule: add 'deinit' command
Originally merged to 'next' on 2013-03-05
There was no Porcelain wa
Re: [PATCH 2/4] dir.c::match_basename(): pay attention to the length of string parameters
Jeff King writes: > So I think your series is the right direction, but we would want to > factor out the allocation code and use it from match_pathname, as well. I am deep into today's integration cycle, so perhaps in the meantime you can help with a follow-up patch ;-)? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/9] clone: leave repo in place after checkout errors
Jeff King wrote:
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -377,10 +377,40 @@ static void remove_junk(void)
> static const char *junk_work_tree;
> static const char *junk_git_dir;
> static pid_t junk_pid;
> +enum {
> + JUNK_LEAVE_NONE,
> + JUNK_LEAVE_REPO,
> + JUNK_LEAVE_ALL
> +} junk_mode = JUNK_LEAVE_NONE;
Neat.
> +
> +static const char junk_leave_repo_msg[] =
> +N_("The remote repository was cloned successfully, but there was\n"
> + "an error checking out the HEAD branch. The repository has been left in\n"
> + "place but the working tree may be in an inconsistent state. You can\n"
> + "can inspect the contents with:\n"
> + "\n"
> + "git status\n"
> + "\n"
> + "and retry the checkout with\n"
> + "\n"
> + "git checkout -f HEAD\n"
> + "\n");
Can this be made more precise? I don't know what it means for the
working tree to be in an inconsistent state: do you mean that some files
might be partially checked out or not have been checked out at all yet?
error: Clone succeeded, but checkout failed.
hint: You can inspect what was checked out with "git status".
hint: To retry the checkout, run "git checkout -f HEAD".
Aside from that, this looks very nice.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] clone: run check_everything_connected
On Tue, Mar 26, 2013 at 07:53:42AM +0700, Nguyen Thai Ngoc Duy wrote:
> On Tue, Mar 26, 2013 at 3:26 AM, Jeff King wrote:
> > static void update_remote_refs(const struct ref *refs,
> >const struct ref *mapped_refs,
> >const struct ref *remote_head_points_at,
> >const char *branch_top,
> >const char *msg)
> > {
> > + const struct ref *rm = mapped_refs;
> > +
> > + if (check_everything_connected(iterate_ref_map, 0, &rm))
> > + die(_("remote did not send all necessary objects"));
> > +
> > if (refs) {
> > write_remote_refs(mapped_refs);
> > if (option_single_branch)
>
> Maybe move this after checkout, so that I can switch terminal and
> start working while it's verifying? And maybe an option not to
> check_everything_connected, instead print a big fat warning telling
> the user to fsck later?
I tried to follow the fetch process of not installing the refs until we
had verified that the objects were reasonable. It probably doesn't
matter that much for clone, since you would not have simultaneous users
expecting the repository to be in a reasonable state until after clone
completes, though.
We also would have to tweak check_everything_connected, which does
something like "--not --all" to avoid rechecking objects we already
have. But that is not too hard to do.
-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 10/9] clone: leave repo in place after checkout errors
On Tue, Mar 26, 2013 at 02:40:57PM -0700, Junio C Hamano wrote:
> > I think the "leave the data behind" fix may be to just set "junk_pid =
> > 0" a little sooner in cmd_clone (i.e., before checkout()). Then we
> > would still die, but at least leave the fetched objects intact.
>
> Yeah, perhaps, but I agree that is a much lower priority change.
As it turns out, the checkout() error path sometimes _already_ leaves
the repository intact, but it's due to a bug. And it ends up deleting
something random instead. :)
I agree it's not a high priority, but I think it makes sense while we're
in the area. And while it's very unlikely that the deletion would be
disastrous (see below), it makes me nervous. Patch is below.
-- >8 --
Subject: [PATCH] clone: leave repo in place after checkout errors
If we manage to clone a remote repository but run into an
error in the checkout, it is probably sane to leave the repo
directory in place. That lets the user examine the situation
without spending time to re-clone from the remote (which may
be a lengthy process).
Rather than try to convert each die() from the checkout code
path into an error(), we simply set a flag that tells the
"remove_junk" atexit function to print a helpful message and
leave the repo in place.
Note that the test added in this patch actually passes
without the code change. The reason is that the cleanup code
is buggy; we chdir into the working tree for the checkout,
but still may use relative paths to remove the directories
(which means if you cloned into "foo", we would accidentally
remove "foo" from the working tree!). There's no point in
fixing it now, since this patch means we will never try to
remove anything after the chdir, anyway.
Signed-off-by: Jeff King
---
I think the accidental deletion could also escape the repository if you
did something like:
git clone $remote ../../foo
which would delete ../../foo/../../foo, or ../../../foo, which is not
related to what you just cloned. But I didn't test, and we don't have to
care anymore after this patch.
builtin/clone.c | 33 -
t/t1060-object-corruption.sh | 4
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index eceaa74..e145dfc 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -377,10 +377,40 @@ static void remove_junk(void)
static const char *junk_work_tree;
static const char *junk_git_dir;
static pid_t junk_pid;
+enum {
+ JUNK_LEAVE_NONE,
+ JUNK_LEAVE_REPO,
+ JUNK_LEAVE_ALL
+} junk_mode = JUNK_LEAVE_NONE;
+
+static const char junk_leave_repo_msg[] =
+N_("The remote repository was cloned successfully, but there was\n"
+ "an error checking out the HEAD branch. The repository has been left in\n"
+ "place but the working tree may be in an inconsistent state. You can\n"
+ "can inspect the contents with:\n"
+ "\n"
+ "git status\n"
+ "\n"
+ "and retry the checkout with\n"
+ "\n"
+ "git checkout -f HEAD\n"
+ "\n");
static void remove_junk(void)
{
struct strbuf sb = STRBUF_INIT;
+
+ switch (junk_mode) {
+ case JUNK_LEAVE_REPO:
+ warning("%s", _(junk_leave_repo_msg));
+ /* fall-through */
+ case JUNK_LEAVE_ALL:
+ return;
+ default:
+ /* proceed to removal */
+ break;
+ }
+
if (getpid() != junk_pid)
return;
if (junk_git_dir) {
@@ -925,12 +955,13 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
transport_unlock_pack(transport);
transport_disconnect(transport);
+ junk_mode = JUNK_LEAVE_REPO;
err = checkout();
strbuf_release(&reflog_msg);
strbuf_release(&branch_top);
strbuf_release(&key);
strbuf_release(&value);
- junk_pid = 0;
+ junk_mode = JUNK_LEAVE_ALL;
return err;
}
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index a405b70..a84deb1 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -89,6 +89,10 @@ test_expect_success 'clone --local detects corruption' '
test_must_fail git clone --local bit-error corrupt-checkout
'
+test_expect_success 'error detected during checkout leaves repo intact' '
+ test_path_is_dir corrupt-checkout/.git
+'
+
test_expect_success 'clone --local detects missing objects' '
test_must_fail git clone --local missing missing-checkout
'
--
1.8.2.13.g0f18d3c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
merge help
I have a branch for which I have made 0 (nada) changes. I did the following: $git pull --rebase --no-stat -v --progress origin mybranch I get the following U java/Profile.java Pull is not possible because you have unmerged files. Please, fix them up in the work tree, and then use 'git add/rm ' as appropriate to mark resolution, or use 'git commit -a'. -- (there is no local master branch), because of the conflict I was put on (no branch) * (no branch) branch1 dev tmpWork Question 1) Why did I get a merge conflict if I have not changed any files? Question 2) What is the command to show the difference between the files? (is there a visual tool that would let me merge)? Question 3) What is the command I would type to "accept theirs" and overwrite my local? After this would I need to $git add & commit & push the file? If someone could guide me through this initial process, (what is best recommended it would be most helpful). thanks J.V. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Change the committer username
Eric Kom writes: > Good day, > > Please how can I change the committer username from system default to > personalize? Quoting from a very early part of http://git-htmldocs.googlecode.com/git/gittutorial.html It is a good idea to introduce yourself to Git with your name and public email address before doing any operation. The easiest way to do so is: $ git config --global user.name "Your Name Comes Here" $ git config --global user.email y...@yourdomain.example.com HTH -- To unsubscribe from this list: send the line "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 series vs. multiple files changed in a commit; storytelling history vs. literal creation history
Matt McClure writes: > I've read Documentation/SubmittingPatches, followed some of the > discussion on this list, and looked over some of the recent commit > history. I'm impressed by the strong culture of review that produces > readable patches and commit messages, but I think there are some gaps > in my understanding of the prevailing process here. > > Most of the code I've worked on has been closed source, and the commit > histories tend to reflect what I'd call the literal "creation > history". Reading the Git history, my impression is that it reflects a > different "storytelling" history. In some cases, that might be the > same as the creation history, but in general the emphasis is on > telling a coherent story of the changes to the other developers rather > than communicating all the messy details of how you arrived at the > order of that story. Is that right? We do not try to keep records of "oops, the previous was wrong" when we can easily tell the previous was wrong. Usually such a shallow mistake that can be spotted during the review is not worth keeping. For example, the 4 patch series I posted today had three iterations of botched attempts that were never even published. I am sure other people do the same for their initial round for their patches, and any message on this list whose subject begins with "[PATCH vN X/Y]" for N > 2 are rewritten betterment based on list feedback. Our history tends to become a coherent story because of this. It is a different story for more involved changes that cook in 'next' for a while and then later turns out to have flaws. We update them with follow-up fixes and at that point, we do have records of mistakes. They often are tricky cases that are worth recording, as people can later make similar kinds of mistakes in other parts of the codebase. The early part of the history back when Linus was running the show is somewhat different; you see more reverts and rewrites. But even back then, there were more experimental changes that were rewritten than the changes that were finally committed to the history. -- To unsubscribe from this list: send the line "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: propagating repo corruption across clone
On Tue, Mar 26, 2013 at 09:59:42PM -, Philip Oakley wrote: > Which way does `git bundle file.bundl --all` perform after the changes > for both the 'transport' checking and being reliable during updates. Bundles are treated at a fairly low level the same as a remote who provides us a particular set of refs and a packfile. So we should get the same protections via index-pack, and still run check_everything_connected on it, just as we would with a fetch over the git protocol. I didn't test it, 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: propagating repo corruption across clone
From: "Jeff King" Sent: Tuesday, March 26, 2013 4:55 PM On Tue, Mar 26, 2013 at 09:43:01AM -0400, Jeff Mitchell wrote: On Mon, Mar 25, 2013 at 4:07 PM, Jeff King wrote: > On Mon, Mar 25, 2013 at 12:32:50PM -0400, Jeff Mitchell wrote: >> For commit corruptions, the --no-hardlinks, non --mirror case >> refused >> to create the new repository and exited with an error code of 128. >> The >> --no-hardlinks, --mirror case spewed errors to the console, yet >> *still* created the new clone *and* returned an error code of >> zero. > > I wasn't able to reproduce this; can you post a succint test case? [...link to tar.gz...] Once you extract that, you should be able to run a clone using paths (not file://) with --no-hardlinks --mirror and replicate the behavior I saw. FYI, I'm on Git 1.8.2. Thanks for providing an example. The difference is the same "--mirror implies --bare" issue; the non-bare case dies during the checkout (even before my patches, as the corruption is not in a blob, but rather in the HEAD commit object itself). You can replace --mirror with --bare and see the same behavior. The troubling part is that we see errors in the bare case, but do not die. Those errors all come from upload-pack, the "sending" side of a clone/fetch. Even though we do not transfer the objects via the git protocol, we still invoke upload-pack to get the ref list (and then copy the objects themselves out-of-band). What happens is that upload-pack sees the errors while trying to see if the object is a tag that can be peeled (the server advertises both tags and the objects they point to). It does not distinguish between "errors did not let me peel this object" and "this object is not a tag, and therefore there is nothing to peel". We could change that, but I'm not sure whether it is a good idea. I think the intent is that upload-pack's ref advertisement would remain resilient to corruption in the repository (e.g., even if that commit is corrupt, you can still fetch the rest of the data). We should not worry about advertising broken objects, because we will encounter the same error when we actually do try to send the objects. Dying at the advertisement phase would be premature, since we do not yet know what the client will request. The problem, of course, is that the --local optimization _skips_ the part where we actually ask upload-pack for data, and instead blindly copies it. So this is the same issue as usual, which is that the local transport is not thorough enough to catch corruption. It seems like a failing in this case, because upload-pack does notice the problem, but that is only luck; if the corruption were in a non-tip object, it would not notice it at all. So trying to die on errors in the ref advertisement would just be a band-aid. Fundamentally the problem is that the --local transport is not safe from propagating corruption, and should not be used if that's a requirement. -Peff -- Which way does `git bundle file.bundl --all` perform after the changes for both the 'transport' checking and being reliable during updates. Is it an option for creating an archivable file that can be used for a later `clone`? I wasn't sure if the bundle capability had been considered. Philip -- To unsubscribe from this list: send the line "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 9/9] clone: run check_everything_connected
Jeff King writes: > The slowdown is really quite terrible if you try "git clone --bare > linux-2.6.git". Even with this, the local-clone case already misses blob > corruption. So it probably makes sense to restrict it to just the > non-local clone case, which already has to do more work. Probably. We may want to enable fsck even for local clones in the longer term and also have this check. Those who know their filesystem is trustworthy can do the filesystem-level copy with "cp -R" themselves after all. > Even still, it adds a non-trivial amount of work (linux-2.6 takes > something like a minute to check). I don't like the idea of declaring > "git clone" non-safe unless you turn on transfer.fsckObjects, though. It > should have the same safety as "git fetch". True. -- To unsubscribe from this list: send the line "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 ate my home directory :-(
From: "Duy Nguyen" Sent: Tuesday, March 26, 2013 9:48 AM On Tue, Mar 26, 2013 at 08:02:30AM -, Philip Oakley wrote: >> Yeah, for historical reasons GIT_WORK_TREE defaults to $(pwd) when >> GIT_DIR is explicitly set. > > And it *WILL* be that way til the end of time. Unless you are at > the top level of your working tree, you are supposed to tell where > the top level is with GIT_WORK_TREE when you use GIT_DIR. Always. Should this important warning be part of the git(1) documentation on the environment variables (and possibly other places) given the consequences of this case? It wasn't something I'd appreciated from a simple reading. Something like this, maybe? -- 8< -- Subject: [PATCH] git.txt: document the implicit working tree setting with GIT_DIR Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/git.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/git.txt b/Documentation/git.txt index 7efaa59..ce55abf 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -671,6 +671,8 @@ Git so take care if using Cogito etc. specifies a path to use instead of the default `.git` for the base of the repository. The '--git-dir' command-line option also sets this value. + If neither GIT_WORK_TREE nor '--work-tree' is set, the + current directory will become the working tree. I didn't feel this conveyed the Dire Warning effect that would be needed to avoid the original misunderstanding. It is easy to miss some of the potential consequences when other priorities are pressing. As Junio wondered, perhaps rhetorically, in a later message "Why do these people set GIT_DIR without setting GIT_WORK_TREE in the first place?" Perhaps "If the GIT_DIR environment variable is set then it specifies a path to use instead of the default `.git` for the base of the repository. Note that the current directory `.` will be used as the working GIT_WORK_TREE, if not set elsewhere. The --git-dir command-line option also sets the GIT_DIR environment variable." 'GIT_WORK_TREE':: Set the path to the working tree. The value will not be -- 1.8.2.82.gc24b958 -- 8< -- -- Philip -- To unsubscribe from this list: send the line "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 8/9] clone: die on errors from unpack_trees
Jeff King writes: > When clone is populating the working tree, it ignores the > return status from unpack_trees; this means we may report a > successful clone, even when the checkout fails. > > When checkout fails, we may want to leave the $GIT_DIR in > place, as it might be possible to recover the data through > further use of "git checkout" (e.g., if the checkout failed > due to a transient error, disk full, etc). However, we > already die on a number of other checkout-related errors, so > this patch follows that pattern. > > In addition to marking a now-passing test, we need to adjust > t5710, which blindly assumed it could make bogus clones of > very deep alternates hierarchies. By using "--bare", we can > avoid it actually touching any objects. > > Signed-off-by: Jeff King > --- Thanks. > I think the "leave the data behind" fix may be to just set "junk_pid = > 0" a little sooner in cmd_clone (i.e., before checkout()). Then we > would still die, but at least leave the fetched objects intact. Yeah, perhaps, but I agree that is a much lower priority change. -- To unsubscribe from this list: send the line "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 6/9] streaming_write_entry: propagate streaming errors
Jeff King writes: > Subject: [PATCH] streaming_write_entry: propagate streaming errors > > When we are streaming an index blob to disk, we store the > error from stream_blob_to_fd in the "result" variable, and > then immediately overwrite that with the return value of > "close". That means we catch errors on close (e.g., problems > committing the file to disk), but miss anything which > happened before then. > > We can fix this by using bitwise-OR to accumulate errors in > our result variable. > > While we're here, we can also simplify the error handling > with an early return, which makes it easier to see under > which circumstances we need to clean up. > > Signed-off-by: Jeff King Very sensible. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash
On Tue, Mar 26, 2013 at 03:05:58PM -0400, Jeff King wrote:
> On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:
>
> > A similar adjustment for match_pathname() might be needed, but I
> > didn't look into it.
>
> I notice that match_pathname takes _two_ lengths for the pattern: the
> nowildcardlen (called "prefix", and the full patternlen). But the first
> thing it does is:
>
> if (*pattern == '/') {
> pattern++;
> prefix--;
> }
>
> which seems obviously wrong, as patternlen should be dropped, too. But
> we do not seem to look at patternlen at all! I think we can drop the
> parameter totally.
>
> We do seem to use strncmp_icase through the rest of the function,
> though, which should be OK. The one exception is that we call fnmatch at
> the end. Should the allocation hack from the previous patch make its way
> into an "fnmatch_icase_bytes()" function, so we can use it here, too?
> And then when we have a more efficient solution, we can just plug it in
> there.
Hmm. match_pathname does have this:
/*
* baselen does not count the trailing slash. base[] may or
* may not end with a trailing slash though.
*/
if (pathlen < baselen + 1 ||
(baselen && pathname[baselen] != '/') ||
strncmp_icase(pathname, base, baselen))
return 0;
which seems to imply that the trailing slash is important here, and that
we should not drop it when passing the path to match_pathname. I'm
still trying to figure out exactly what it is that the extra slash check
is for, and whether it might not have the same problem.
-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
Change the committer username
Good day, Please how can I change the committer username from system default to personalize? -- Kind Regards Eric Kom System Administrator & Programmer - Metropolitan College _ / You are scrupulously honest, frank, and \ | straightforward. Therefore you have few | \ friends./ - \ \ .--. |o_o | |:_/ | // \ \ (| Kom | ) /'\_ _/`\ \___)=(___/ 2 Hennie Van Till, White River, 1240 Tel: 013 750 2255 | Fax: 013 750 0105 | Cell: 078 879 1334 eric...@kom.za.net | eric...@metropolitancollege.co.za www.kom.za.net | www.kom.za.org | www.erickom.co.za Key fingerprint: 513E E91A C243 3020 8735 09BB 2DBC 5AD7 A9DA 1EF5 -- To unsubscribe from this list: send the line "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/4] dir.c::match_basename(): pay attention to the length of string parameters
On Tue, Mar 26, 2013 at 01:49:10PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > I timed this doing "git archive HEAD" on webkit.git before and after. It > > actually ended up not mattering much (I think because it is only the > > directories which are affected, not each individually path, so it's a > > much smaller number than you'd think). The best-of-five timing was > > slightly slower, but was within the noise. > > Interesting. Because "archive" has to incur a large I/O cost > anyway, I expected extra allocation for correctness for only the > directory paths would be dwarfed in the noise. > > I actually care more about cases other than "archive", though. Do > we even feed directory paths to the machinery? In general, no, I don't think so. That's why I tested "archive", since I knew it did. In the normal case, we should just feed file paths, meaning we only run into this code path when somebody has "foo/" in their pattern. Testing like: git ls-files -z >files time git check-attr --stdin -z -a /dev/null showed a difference well within the noise. > > So I do still think it would make sense to go to a byte-limited version > > of fnmatch eventually, just for code cleanliness and predictability of > > performance, but this is really not a bad solution in the interim. > > Yes, what we do with wildmatch is a separate issue for 'master' and > upwards. Oh, agreed. I just wanted to see how much performance would be impacted for the interim. But it seems that it's not. So I think your series is the right direction, but we would want to factor out the allocation code and use it from match_pathname, as well. -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/9] stream_blob_to_fd: detect errors reading from stream
Jeff King writes:
> We call read_istream, but never check its return value for
> errors. This can lead to us looping infinitely, as we just
> keep trying to write "-1" bytes (and we do not notice the
> error, as we simply check that write_in_full reports the
> same number of bytes we fed it, which of course is also -1).
Looks sane. Thanks.
>
> Signed-off-by: Jeff King
> ---
> No test yet, as my method for triggering this causes _another_ infinite
> loop. So the test comes after the fixes, to avoid infinite loops when
> bisecting the history later. :)
>
> streaming.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/streaming.c b/streaming.c
> index 4d978e5..f4126a7 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -514,6 +514,8 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1,
> struct stream_filter *f
> ssize_t wrote, holeto;
> ssize_t readlen = read_istream(st, buf, sizeof(buf));
>
> + if (readlen < 0)
> + goto close_and_exit;
> if (!readlen)
> break;
> if (can_seek && sizeof(buf) == readlen) {
--
To unsubscribe from this list: send the line "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] git-svn: Support custom tunnel schemes instead of SSH only
This originates from an msysgit pull request, see:
https://github.com/msysgit/git/pull/58
Signed-off-by: Eric Wieser
Signed-off-by: Sebastian Schuberth
---
perl/Git/SVN/Ra.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 049c97b..6a212eb 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -295,7 +295,7 @@ sub gs_do_switch {
my $full_url = add_path_to_url( $self->url, $path );
my ($ra, $reparented);
- if ($old_url =~ m#^svn(\+ssh)?://# ||
+ if ($old_url =~ m#^svn(\+\w+)?://# ||
($full_url =~ m#^https?://# &&
canonicalize_url($full_url) ne $full_url)) {
$_[0] = undef;
--
1.8.1.msysgit.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
patch series vs. multiple files changed in a commit; storytelling history vs. literal creation history
I've read Documentation/SubmittingPatches, followed some of the discussion on this list, and looked over some of the recent commit history. I'm impressed by the strong culture of review that produces readable patches and commit messages, but I think there are some gaps in my understanding of the prevailing process here. Most of the code I've worked on has been closed source, and the commit histories tend to reflect what I'd call the literal "creation history". Reading the Git history, my impression is that it reflects a different "storytelling" history. In some cases, that might be the same as the creation history, but in general the emphasis is on telling a coherent story of the changes to the other developers rather than communicating all the messy details of how you arrived at the order of that story. Is that right? What are the Git project's rules of thumb for when to create a patch series vs. putting changes to multiple files in a single commit/patch? As a patch series evolves before landing on an upstream branch, do you typically make corrections to the original series in new commits, or update the respective commits from the original series in a new series of analogous commits? -- Matt McClure http://www.matthewlmcclure.com http://www.mapmyfitness.com/profile/matthewlmcclure -- To unsubscribe from this list: send the line "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] difftool: don't overwrite modified files
On Tue, Mar 26, 2013 at 04:52:02PM -0400, Matt McClure wrote: > On Mon, Mar 25, 2013 at 5:44 PM, John Keeping wrote: > > Instead of copying unconditionally when the files differ, create and > > index from the working tree files and only copy the temporary file back > > if it was modified and the working tree file was not. If both files > > have been modified, print a warning and exit with an error. > > When there's a conflict, does difftool save both conflicting files? Or > only the working tree copy? I think it should preserve both copies on > disk. It preserves both copies - the "clean the temporary directory" step is just skipped. This isn't ideal since the temporary copy will be under a temporary directory somewhere but is better than the current behaviour. It might be nice to move the temporary file back with an extension so that the files are at least near each other but I don't think that's needed in the first version of this change. -- To unsubscribe from this list: send the line "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] difftool: don't overwrite modified files
On Mon, Mar 25, 2013 at 5:44 PM, John Keeping wrote: > Instead of copying unconditionally when the files differ, create and > index from the working tree files and only copy the temporary file back > if it was modified and the working tree file was not. If both files > have been modified, print a warning and exit with an error. When there's a conflict, does difftool save both conflicting files? Or only the working tree copy? I think it should preserve both copies on disk. -- Matt McClure http://www.matthewlmcclure.com http://www.mapmyfitness.com/profile/matthewlmcclure -- To unsubscribe from this list: send the line "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/4] dir.c::match_basename(): pay attention to the length of string parameters
Jeff King writes: > I timed this doing "git archive HEAD" on webkit.git before and after. It > actually ended up not mattering much (I think because it is only the > directories which are affected, not each individually path, so it's a > much smaller number than you'd think). The best-of-five timing was > slightly slower, but was within the noise. Interesting. Because "archive" has to incur a large I/O cost anyway, I expected extra allocation for correctness for only the directory paths would be dwarfed in the noise. I actually care more about cases other than "archive", though. Do we even feed directory paths to the machinery? > So I do still think it would make sense to go to a byte-limited version > of fnmatch eventually, just for code cleanliness and predictability of > performance, but this is really not a bad solution in the interim. Yes, what we do with wildmatch is a separate issue for 'master' and upwards. -- To unsubscribe from this list: send the line "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: Rename conflicts in the index
Junio C Hamano [mailto:gits...@pobox.com] wrote:
> Edward Thomson writes:
> > I would propose a new extension, 'CONF', to handle conflict data,
> > differing from the stage >0 entries in the index in that this
> > extension tracks the conflicting file across names if the underlying
> > merge engine has support for renames.
> >
> > I made an attempt to keep the entry data similar to other entries in
> > the index. I would propose that entries in the conflict are as follows:
> >
> > Flags
> > Four octets that describe the conflict. Data includes:
> >
> > 0x01 HAS_ANCESTOR
> > There is a file in the common ancestor branch that contributes
> > to this conflict. Its data will follow.
> > 0x02 HAS_OURS
> > There is a file in "our" branch that contributes to this conflict.
> > Its data will follow.
> > 0x04 HAS_THEIRS
> > There is a file in "their" branch that contributes to this conflict.
> > Its data will follow.
> >
> > 0x08 NAME_CONFLICT_OURS
> > This item has a path in "our" branch that overlaps a different
> > item in "their" branch. (Eg, this conflict represents the "our"
> > side of a rename/add conflict.)
> > 0x10 NAME_CONFLICT_THEIRS
> > This item has a path in "their" branch that overlaps a different
> > item in "our" branch. (Eg, this conflict represents the "theirs"
> > side of a rename/add conflict.)
> >
> > 0x20 DF_CONFLICT_FILE
> > This is the file involved in a directory/file conflict.
> > 0x40 DF_CONFLICT_CHILD
> > This is a child of a directory involved in a directory/file conflict.
> >
> > Other bits are reserved.
> >
> > Conflict Sides
> > The data about one side of a conflict will contain:
> > mode (ASCII string representation of octal, null-terminated)
> > path (null terminated)
> > sha1 (raw bytes)
> >
> > The conflict sides will be written in this order:
> > Ancestor (if HAS_ANCESTOR is set)
> > Ours (if HAS_OURS is set)
> > Theirs (if HAS_THEIRS is set)
>
> Puzzled. Most of the above, except NAME_CONFLICT_{OURS,THEIRS} bits, look
> totally pointless duplication.
Obviously HAS_ANCESTOR / HAS_OURS / HAS_THEIRS is to indicate to a reader
whether there is data to be read or not. Similar to how a mode of 0
in the REUC indicates that the rest of the record should not be read.)
> When you are working with Git, you have to be prepared to read from the
> datafile like the index that other people (and your previous
> version) created, and you also have to make sure you do not make what you
> write out unusable by other people without a good reason.
I'm acutely aware that you need to be able to read an index that other
people created - that's the problem at hand. git does not produce an
index that allows anyone (including itself) to reason about rename
conflicts. It doesn't even bother to write high-stage conflict entries
in many instances, so you can have an instance where git tells you that
a conflict occurred but one of those files is staged anyway, the other
is just dirty in the workdir and you can commit immediately thereafter.
While obviously it's possible to handle this situation (is file A
in conflict? Look in the rename conflict extension. Not there? Okay,
look in the index.) That's not exactly elegant. My goal here was
to have a single source for conflicts.
> (by the way "CONF" sounds as if it is some sort of configuration data).
There's only four letters, and not everything's as easy as "TREE". REUC,
for example, sounds like a donkey, though I suppose it depends on the
language in question.
-ed
--
To unsubscribe from this list: send the line "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/4] dir.c::match_basename(): pay attention to the length of string parameters
On Tue, Mar 26, 2013 at 02:55:59PM -0400, Jeff King wrote: > > * Otherwise, make sure we use only the counted part of the strings > >when calling fnmatch_icase(). Because these counted strings are > >full strings most of the time, avoid unnecessary allocation. > > I think this is OK, with the intention that we would eventually drop the > allocations from your third bullet point in favor of using a > byte-counted version of fnmatch (i.e., nwildmatch). But until then we're > going to see a performance drop. > > The pattern is usually going to be NUL-terminated at the length counter, > but every time we feed a directory, it's going to run into this > allocation. And we do it once for _every_ directory against _every_ > wildcard gitignore pattern. So I think it is probably going to be > measurable. I guess we can try measuring it on something like WebKit, > which has plenty of both directories and gitattributes. I timed this doing "git archive HEAD" on webkit.git before and after. It actually ended up not mattering much (I think because it is only the directories which are affected, not each individually path, so it's a much smaller number than you'd think). The best-of-five timing was slightly slower, but was within the noise. So I do still think it would make sense to go to a byte-limited version of fnmatch eventually, just for code cleanliness and predictability of performance, but this is really not a bad solution in the interim. -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: [DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
Jeff King wrote: > On Tue, Mar 26, 2013 at 01:21:42PM -0700, Jonathan Nieder wrote: >> If we want this warning, would something like the following do? >> >> warning: You have set GIT_DIR without setting GIT_WORK_TREE >> hint: In this case, GIT_WORK_TREE defaults to '.' >> hint: To suppress this message, set GIT_WORK_TREE='.' > > That can help by teaching people how GIT_DIR behaves in general. Yes, I think it would have helped in this case. If I understand correctly then for a while Richard was habitually setting GIT_DIR to mean "act on this repository" and thought the worktree was automatically being set to the containing directory. I think patch 3 is a bad direction to go because there will always be old scripts that follow what used to be the recommended way to use GIT_DIR. In the long term a warning like this that doesn't break them (or a fatal error that at least doesn't confuse them) might be a good way to go. Thanks for your thoughtful work, as always. Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
On Tue, Mar 26, 2013 at 01:21:42PM -0700, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -437,6 +437,23 @@ const char *read_gitfile(const char *path)
> > return path;
> > }
> >
> > +static const char warn_implicit_work_tree_msg[] =
> > +N_("You have set GIT_DIR (or used --git-dir) without specifying\n"
> > + "a working tree. In Git 2.0, the behavior will change
>
> Please no. I don't want git 2.0 to be delayed forever.
Please replace "2.0" with some future version, then. I just made up the
number. But...
> If we want this warning, would something like the following do?
>
> warning: You have set GIT_DIR without setting GIT_WORK_TREE
> hint: In this case, GIT_WORK_TREE defaults to '.'
> hint: To suppress this message, set GIT_WORK_TREE='.'
That can help by teaching people how GIT_DIR behaves in general. But the
warning and hint will be small consolation to somebody who runs
"GIT_DIR=foo.git git clean -f" and sees it for the first time.
If you want to argue that people would see the warning in earlier runs
of git, I can kind of buy that. Although the incident that triggered
this discussion probably wouldn't have (I would usually start a
git-clean session with "git clean" without "-f" or "git status", either
of which would have done equally well as this warning to notify the user
what was going on).
Like I said earlier, though, I'm not really sure this is the direction
we want to go. This series is more about seeing what the fallouts are. I
probably shouldn't have included this middle patch at all, because the
interesting thing is what happens when we do turn it off.
-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: [DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
Jeff King wrote:
> --- a/setup.c
> +++ b/setup.c
> @@ -437,6 +437,23 @@ const char *read_gitfile(const char *path)
> return path;
> }
>
> +static const char warn_implicit_work_tree_msg[] =
> +N_("You have set GIT_DIR (or used --git-dir) without specifying\n"
> + "a working tree. In Git 2.0, the behavior will change
Please no. I don't want git 2.0 to be delayed forever.
If we want this warning, would something like the following do?
warning: You have set GIT_DIR without setting GIT_WORK_TREE
hint: In this case, GIT_WORK_TREE defaults to '.'
hint: To suppress this message, set GIT_WORK_TREE='.'
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [DONOTAPPLY PATCH 1/3] environment: set GIT_WORK_TREE when we figure out work tree
Jeff King wrote: > --- a/environment.c > +++ b/environment.c > @@ -194,6 +194,7 @@ void set_git_work_tree(const char *new_work_tree) > } > git_work_tree_initialized = 1; > work_tree = xstrdup(real_path(new_work_tree)); > + setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1); > } There's no rush, but I think this is a good change. It makes the rest of the codebase more resilient to running commands from a subdir of the top level of the worktree. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[DONOTAPPLY PATCH 3/3] setup: treat GIT_DIR without GIT_WORK_TREE as a bare repo
Follow-through on the deprecation warning added by the last
commit.
We can drop all of the IMPLICIT_WORK_TREE code now, since
we default to that case.
Signed-off-by: Jeff King
---
This would obviously come much later than patch 2, in Git 2.0 or
whatever.
But in case anyone did not read the discussion leading up to this
series, this breaks many tests. It's not meant for application, but
merely to look at what kinds of breakage we could see if we followed
this path.
cache.h | 12
environment.c | 1 -
git.c | 1 -
setup.c | 26 +-
t/t1510-repo-setup.sh | 24 ++--
5 files changed, 11 insertions(+), 53 deletions(-)
diff --git a/cache.h b/cache.h
index c1fe67f..3c6b677 100644
--- a/cache.h
+++ b/cache.h
@@ -366,18 +366,6 @@ static inline enum object_type object_type(unsigned int
mode)
#define GIT_NOTES_REWRITE_MODE_ENVIRONMENT "GIT_NOTES_REWRITE_MODE"
/*
- * This environment variable is expected to contain a boolean indicating
- * whether we should or should not treat:
- *
- * GIT_DIR=foo.git git ...
- *
- * as if GIT_WORK_TREE=. was given. It's not expected that users will make use
- * of this, but we use it internally to communicate to sub-processes that we
- * are in a bare repo. If not set, defaults to true.
- */
-#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
-
-/*
* Repository-local GIT_* environment variables; these will be cleared
* when git spawns a sub-process that runs inside another repository.
* The array is NULL-terminated, which makes it easy to pass in the "env"
diff --git a/environment.c b/environment.c
index be2e509..255e277 100644
--- a/environment.c
+++ b/environment.c
@@ -85,7 +85,6 @@ const char * const local_repo_env[] = {
DB_ENVIRONMENT,
GIT_DIR_ENVIRONMENT,
GIT_WORK_TREE_ENVIRONMENT,
- GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
GRAFT_ENVIRONMENT,
INDEX_ENVIRONMENT,
NO_REPLACE_OBJECTS_ENVIRONMENT,
diff --git a/git.c b/git.c
index 0ffea57..d33f9b3 100644
--- a/git.c
+++ b/git.c
@@ -125,7 +125,6 @@ static int handle_options(const char ***argv, int *argc,
int *envchanged)
static char git_dir[PATH_MAX+1];
is_bare_repository_cfg = 1;
setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir,
sizeof(git_dir)), 0);
- setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-c")) {
diff --git a/setup.c b/setup.c
index afc245f..319dbb5 100644
--- a/setup.c
+++ b/setup.c
@@ -437,23 +437,6 @@ const char *read_gitfile(const char *path)
return path;
}
-static const char warn_implicit_work_tree_msg[] =
-N_("You have set GIT_DIR (or used --git-dir) without specifying\n"
- "a working tree. In Git 2.0, the behavior will change from using current\n"
- "working directory as the working tree to having no working tree at all.\n"
- "If you wish to continue the current behavior, please set GIT_WORK_TREE\n"
- "or core.worktree explicitly. See `git help git` for more details.");
-
-static void warn_implicit_work_tree(void)
-{
- static int warn_once;
-
- if (warn_once++)
- return;
-
- warning("%s", _(warn_implicit_work_tree_msg));
-}
-
static const char *setup_explicit_git_dir(const char *gitdirenv,
char *cwd, int len,
int *nongit_ok)
@@ -514,16 +497,11 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
set_git_work_tree(core_worktree);
}
}
- else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
- /* #16d */
+ else { /* #2, #10, #16d */
set_git_dir(gitdirenv);
free(gitfile);
return NULL;
}
- else { /* #2, #10 */
- warn_implicit_work_tree();
- set_git_work_tree(".");
- }
/* set_git_work_tree() must have been called by now */
worktree = get_git_work_tree();
@@ -600,8 +578,6 @@ static const char *setup_bare_git_dir(char *cwd, int
offset, int len, int *nongi
if (check_repository_format_gently(".", nongit_ok))
return NULL;
- setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
-
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
const char *gitdir;
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 0910de1..7db4b3f 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -28,7 +28,7 @@ A few rules for repo setup:
7. Effective core.worktree conflicts with core.bare
8. If GIT_DIR is set but neither worktree nor bare
[DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
It can be surprising to some users that pointing GIT_DIR to
a ".git" directory does not use the working tree that
surrounds the .git directory, but rather uses the current
working directory as the working tree.
Git has always worked this way, and for the most part it has
not been a big problem. However, given that one way of the
user finding this out is by having a destructive git command
impact an unexpected area of the filesystem, it would be
nice to default to something less surprising and likely to
cause problems (namely, having no working directory).
This breaks existing users of the feature, of course; they
can adapt by setting GIT_WORK_TREE explicitly to ".", but
they need to be told to do so. Therefore we'll start with a
deprecation period and a warning to give them time to fix
their scripts and workflows.
Signed-off-by: Jeff King
---
setup.c | 21 -
t/t1510-repo-setup.sh | 8 ++--
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/setup.c b/setup.c
index 01c5476..afc245f 100644
--- a/setup.c
+++ b/setup.c
@@ -437,6 +437,23 @@ const char *read_gitfile(const char *path)
return path;
}
+static const char warn_implicit_work_tree_msg[] =
+N_("You have set GIT_DIR (or used --git-dir) without specifying\n"
+ "a working tree. In Git 2.0, the behavior will change from using current\n"
+ "working directory as the working tree to having no working tree at all.\n"
+ "If you wish to continue the current behavior, please set GIT_WORK_TREE\n"
+ "or core.worktree explicitly. See `git help git` for more details.");
+
+static void warn_implicit_work_tree(void)
+{
+ static int warn_once;
+
+ if (warn_once++)
+ return;
+
+ warning("%s", _(warn_implicit_work_tree_msg));
+}
+
static const char *setup_explicit_git_dir(const char *gitdirenv,
char *cwd, int len,
int *nongit_ok)
@@ -503,8 +520,10 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
free(gitfile);
return NULL;
}
- else /* #2, #10 */
+ else { /* #2, #10 */
+ warn_implicit_work_tree();
set_git_work_tree(".");
+ }
/* set_git_work_tree() must have been called by now */
worktree = get_git_work_tree();
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index cf2ee78..0910de1 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -243,7 +243,9 @@ test_expect_success '#2: worktree defaults to cwd with
explicit GIT_DIR' '
test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '
try_repo 2 unset "$here/2/.git" unset "" unset \
"$here/2/.git" "$here/2" "$here/2" "(null)" \
- "$here/2/.git" "$here/2/sub" "$here/2/sub" "(null)"
+ "$here/2/.git" "$here/2/sub" "$here/2/sub" "(null)" \
+ 2>message &&
+ test_i18ngrep "warning:.*GIT_DIR" message
'
test_expect_success '#2b: relative GIT_DIR' '
@@ -378,7 +380,9 @@ test_expect_success '#10: GIT_DIR can point to gitfile' '
test_expect_success '#10: GIT_DIR can point to gitfile' '
try_repo 10 unset "$here/10/.git" unset gitfile unset \
"$here/10.git" "$here/10" "$here/10" "(null)" \
- "$here/10.git" "$here/10/sub" "$here/10/sub" "(null)"
+ "$here/10.git" "$here/10/sub" "$here/10/sub" "(null)" \
+ 2>message &&
+ test_i18ngrep "warning:.*GIT_DIR" message
'
test_expect_success '#10b: relative GIT_DIR can point to gitfile' '
--
1.8.2.13.g0f18d3c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[DONOTAPPLY PATCH 1/3] environment: set GIT_WORK_TREE when we figure out work tree
If we end up in a sub-process where GIT_WORK_TREE is not set but GIT_DIR is, we assume the current directory is the root of the working tree. Since future patches will change that assumption, let's defensively start setting GIT_WORK_TREE explicitly. Signed-off-by: Jeff King --- I didn't test this very well (in fact, I only noticed the issue after having written the other two patches, as without it, those ones break every external command which wants to run in a working tree). I would not be surprised if there is some code path which sets GIT_DIR but not this, or if there is some other weird fallout from having GIT_WORK_TREE set explicitly. environment.c | 1 + 1 file changed, 1 insertion(+) diff --git a/environment.c b/environment.c index 92c5dff..be2e509 100644 --- a/environment.c +++ b/environment.c @@ -194,6 +194,7 @@ void set_git_work_tree(const char *new_work_tree) } git_work_tree_initialized = 1; work_tree = xstrdup(real_path(new_work_tree)); + setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1); } const char *get_git_work_tree(void) -- 1.8.2.13.g0f18d3c -- To unsubscribe from this list: send the line "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 ate my home directory :-(
On Tue, Mar 26, 2013 at 11:20:58AM -0700, Junio C Hamano wrote: > When you are in ~/mail/subdir, because GIT_DIR alone does not give > you to specify where the root-level of the working tree is, you had > to "cd .." before running "GIT_DIR=~/git/mail.git git ...". By > setting GIT_WORK_TREE to point at ~/mail once, you can freely chdir > around inside subdirectories of ~/mail without losing sight of where > the root-level is, and if your ~/git/mail.git is tied to a single > working tree (and that is true in this example, it is always ~/mail), > you can even set core.worktree in ~/git/mail.git/config. Yeah, I did not talk about moving around to multiple working trees with the same GIT_DIR. I have done that, but I do not know of a workflow where it is a good practice, and not just a one-off hack. > > We could do so now, as long as we provide an escape hatch (and I think > > spelling that hatch as GIT_WORK_TREE=. is probably sane, but I am open > > to other suggestions). > > If we were to do so, GIT_WORK_TREE=. would be the most sensible, but > I do not think it is worth breaking. Why do these people set GIT_DIR > without setting GIT_WORK_TREE in the first place? I don't think there is a good reason. The argument, as I see it, is mainly that doing so can be confusing and destructive, and there is not a big benefit to allowing it. I am not sure I am convinced it is worth the breakage, either. Curious as to what the code would look like, I made a straw-man series, which will follow. Note that I am not suggesting we do this, but still merely thinking about the idea. Notably, at the end of the series a number of tests fail. A few of them are testing the GIT_DIR behavior explicitly (I fixed up t1510, but did not hunt down all of the spots), but a few of them are legitimate breakages in scripts. For example, difftool is broken because it sets GIT_DIR. That gives us an indication of what kinds of breakages we would see in real-world third-party scripts. > > The problem is not with "clean", which just happens to be a destructive > > command, but rather with the notion of what the git tree is when you > > provide GIT_DIR. > > Yes, "git add ." would happily add random cruft to your index, which > is equally bad. Eh, I would say it is bad, but not equally bad to removing your entire home directory. ;) -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 2/2] cat-file: Fix an gcc -Wuninitialized warning
On Tue, Mar 26, 2013 at 03:35:39PM -0400, Jeff King wrote: > I note that we do not actually check that contents != NULL after calling > read_sha1_file, either (nor that sha1_object_info does not return an > error). I suspect cat-file could segfault under the right conditions. Oh nevermind, we do. We just do it by checking that "type" was filled in correctly, which can combine the check for both read_sha1_file and sha1_object_info. Sorry for the noise. -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 2/2] cat-file: Fix an gcc -Wuninitialized warning
On Tue, Mar 26, 2013 at 07:20:11PM +, Ramsay Jones wrote:
> After commit cbfd5e1c ("drop some obsolete "x = x" compiler warning
> hacks", 21-03-2013) removed a gcc specific hack, older versions of
> gcc now issue an "'contents' might be used uninitialized" warning.
> In order to suppress the warning, we simply initialize the variable
> to NULL in it's declaration.
I'm OK with this, if it's the direction we want to go. But I thought the
discussion kind of ended as "we do not care about these warnings on
ancient versions of gcc; those people should use -Wno-error=uninitialized".
What version of gcc are you using? If it is the most recent thing
reasonably available on msysgit, then I am more sympathetic. But if it's
just an antique version of gcc, I am less so.
> An alternative solution may look like this (note: *untested*):
> [...]
> However, this would add an additional call to sha1_object_info()
Yeah, I don't think that is worth it.
> the "--batch" code path, with potential performance consequences
> (again untested). Also, if you are paranoid, I guess you should
> check that the (type,size) returned by sha1_object_info() was the
> same as that returned by read_sha1_file(). ;-)
I note that we do not actually check that contents != NULL after calling
read_sha1_file, either (nor that sha1_object_info does not return an
error). I suspect cat-file could segfault under the right conditions.
-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] difftool: don't overwrite modified files
On Tue, Mar 26, 2013 at 10:53:48AM +0100, Johannes Sixt wrote: > Am 3/26/2013 10:31, schrieb John Keeping: > > On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote: > > The last test does result in /tmp filling up with temporary directories > > though, it would be good if the test could clean up after itself. The > > best I can come up with is adding something like this immediately after > > running difftool but I'm not entirely happy with the ".." in the > > argument to rm: > > > > test_when_finished rm -rf "$(cat tmpdir)/.." > > Wrap the test in > > ( > TMPDIR=$TRASH_DIRECTORY && > export TMPDIR && > ... > ) > > It works for me. Nice. I've reviewed File::Spec and it looks like that TMPDIR takes priority on every operating system except VMS, and I don't think we care about that. Unless Junio says otherwise, I'll hold off sending this until difftool calms down a bit to avoid too many conflicted merges. -- To unsubscribe from this list: send the line "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] fast-import: Fix an gcc -Wuninitialized warning
On Tue, Mar 26, 2013 at 07:09:44PM +, Ramsay Jones wrote:
> Commit cbfd5e1c ("drop some obsolete "x = x" compiler warning hacks",
> 21-03-2013) removed a gcc hack that suppressed an "might be used
> uninitialized" warning issued by older versions of gcc.
>
> However, commit 3aa99df8 ('fast-import: clarify "inline" logic in
> file_change_m', 21-03-2013) addresses an (almost) identical issue
> (with very similar code), but includes additional code in it's
> resolution. The solution used by this commit, unlike that used by
> commit cbfd5e1c, also suppresses the -Wuninitialized warning on
> older versions of gcc.
>
> In order to suppress the warning (against the 'oe' symbol) in the
> note_change_n() function, we adopt the same solution used by commit
> 3aa99df8.
Yeah, they are essentially the same piece of code, so I don't mind this
change. It is odd to me that gcc gets it right in one case but not the
other, but I think we are deep into the vagaries of the compiler's code
flow analysis here, and we cannot make too many assumptions.
Were you actually triggering this warning, and if so, on what version of
gcc? Or did the asymmetry just offend your sensibilities?
-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: Rename conflicts in the index
Edward Thomson writes:
> I would propose a new extension, 'CONF', to handle conflict data, differing
> from the stage >0 entries in the index in that this extension tracks the
> conflicting file across names if the underlying merge engine has support
> for renames.
>
> I made an attempt to keep the entry data similar to other entries in the
> index. I would propose that entries in the conflict are as follows:
>
> Flags
> Four octets that describe the conflict. Data includes:
>
> 0x01 HAS_ANCESTOR
> There is a file in the common ancestor branch that contributes
> to this conflict. Its data will follow.
> 0x02 HAS_OURS
> There is a file in "our" branch that contributes to this conflict.
> Its data will follow.
> 0x04 HAS_THEIRS
> There is a file in "their" branch that contributes to this conflict.
> Its data will follow.
>
> 0x08 NAME_CONFLICT_OURS
> This item has a path in "our" branch that overlaps a different
> item in "their" branch. (Eg, this conflict represents the "our"
> side of a rename/add conflict.)
> 0x10 NAME_CONFLICT_THEIRS
> This item has a path in "their" branch that overlaps a different
> item in "our" branch. (Eg, this conflict represents the "theirs"
> side of a rename/add conflict.)
>
> 0x20 DF_CONFLICT_FILE
> This is the file involved in a directory/file conflict.
> 0x40 DF_CONFLICT_CHILD
> This is a child of a directory involved in a directory/file conflict.
>
> Other bits are reserved.
>
> Conflict Sides
> The data about one side of a conflict will contain:
> mode (ASCII string representation of octal, null-terminated)
> path (null terminated)
> sha1 (raw bytes)
>
> The conflict sides will be written in this order:
> Ancestor (if HAS_ANCESTOR is set)
> Ours (if HAS_OURS is set)
> Theirs (if HAS_THEIRS is set)
Puzzled. Most of the above, except NAME_CONFLICT_{OURS,THEIRS}
bits, look totally pointless duplication.
When you are working with Git, you have to be prepared to read from
the datafile like the index that other people (and your previous
version) created, and you also have to make sure you do not make
what you write out unusable by other people without a good reason.
So your tool needs code to see higher stage entries in the main
index to find for the conflicted paths even without the
index extension anyway, and if your tool does also perform merges,
you would need to strive for writing the main index with conflicted
entries and implementations that do not yet understand your
extension can keep operating. For some types of extensions, the
latter may be hard (and that is why I stopped at "you would need to
strive for", and not "you must"), but for the one under discussion,
I do not think it is the case (by the way "CONF" sounds as if it is
some sort of configuration data).
If you are starting a brand new system from scratch, keeping only
the resolved entries in the main index and having a separate section
for conflicts might be also a valid design choice, but you do not
live in that world if you are discussing the design on this mailing
list.
> I would propose that this not simply track rename conflicts, but all
> conflicts.
That is a no starter.
--
To unsubscribe from this list: send the line "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] fast-import: Fix an gcc -Wuninitialized warning
Commit cbfd5e1c ("drop some obsolete "x = x" compiler warning hacks",
21-03-2013) removed a gcc hack that suppressed an "might be used
uninitialized" warning issued by older versions of gcc.
However, commit 3aa99df8 ('fast-import: clarify "inline" logic in
file_change_m', 21-03-2013) addresses an (almost) identical issue
(with very similar code), but includes additional code in it's
resolution. The solution used by this commit, unlike that used by
commit cbfd5e1c, also suppresses the -Wuninitialized warning on
older versions of gcc.
In order to suppress the warning (against the 'oe' symbol) in the
note_change_n() function, we adopt the same solution used by commit
3aa99df8.
Signed-off-by: Ramsay Jones
---
fast-import.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fast-import.c b/fast-import.c
index a0c2c2f..5f539d7 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2465,6 +2465,7 @@ static void note_change_n(struct branch *b, unsigned char
*old_fanout)
hashcpy(sha1, oe->idx.sha1);
} else if (!prefixcmp(p, "inline ")) {
inline_data = 1;
+ oe = NULL; /* not used with inline_data, but makes gcc happy */
p += strlen("inline"); /* advance to space */
} else {
if (get_sha1_hex(p, sha1))
--
1.8.2
--
To unsubscribe from this list: send the line "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] cat-file: Fix an gcc -Wuninitialized warning
After commit cbfd5e1c ("drop some obsolete "x = x" compiler warning
hacks", 21-03-2013) removed a gcc specific hack, older versions of
gcc now issue an "'contents' might be used uninitialized" warning.
In order to suppress the warning, we simply initialize the variable
to NULL in it's declaration.
Signed-off-by: Ramsay Jones
---
An alternative solution may look like this (note: *untested*):
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ad29000..e50b20f 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,7 +193,6 @@ static int batch_one_object(const char *obj_name, int
print_contents)
unsigned char sha1[20];
enum object_type type = 0;
unsigned long size;
- void *contents;
if (!obj_name)
return 1;
@@ -204,16 +203,11 @@ static int batch_one_object(const char *obj_name, int
print_contents)
return 0;
}
- if (print_contents == BATCH)
- contents = read_sha1_file(sha1, &type, &size);
- else
- type = sha1_object_info(sha1, &size);
+ type = sha1_object_info(sha1, &size);
if (type <= 0) {
printf("%s missing\n", obj_name);
fflush(stdout);
- if (print_contents == BATCH)
- free(contents);
return 0;
}
@@ -221,6 +215,7 @@ static int batch_one_object(const char *obj_name, int
print_contents)
fflush(stdout);
if (print_contents == BATCH) {
+ void *contents = read_sha1_file(sha1, &type, &size);
write_or_die(1, contents, size);
printf("\n");
fflush(stdout);
--
However, this would add an additional call to sha1_object_info() to
the "--batch" code path, with potential performance consequences
(again untested). Also, if you are paranoid, I guess you should
check that the (type,size) returned by sha1_object_info() was the
same as that returned by read_sha1_file(). ;-)
ATB,
Ramsay Jones
builtin/cat-file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ad29000..40f87b4 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,7 +193,7 @@ static int batch_one_object(const char *obj_name, int
print_contents)
unsigned char sha1[20];
enum object_type type = 0;
unsigned long size;
- void *contents;
+ void *contents = NULL;
if (!obj_name)
return 1;
--
1.8.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Fix -Wuninitialized warnings on older gcc
Hi Junio, I had prepared 3 patches, but I noticed this afternoon that the warning in transport.c has already been fixed in maint. These were built on the tip of master as of saturday evening (master @ 7b592fad). Just FYI, as of saturday, Jeff's patches in maint/master had removed 40 warnings from the MSVC build; 34 of the warnings were due to commit 25043d8a all on it's lonesome! [1]. The additional patches in pu, remove an additional 14 warnings. This leaves only the following: $ grep warning msvc-pout | grep uninit c:\msysgit\msysgit\home\ramsay\git\fast-import.c(2916) : warning C4700:\ uninitialized local variable 'oe' used c:\msysgit\msysgit\home\ramsay\git\builtin\rev-list.c(341) : warning C4700:\ uninitialized local variable 'reaches' used c:\msysgit\msysgit\home\ramsay\git\builtin\rev-list.c(341) : warning C4700:\ uninitialized local variable 'all' used c:\msysgit\msysgit\home\ramsay\git\merge-recursive.c(1886) : warning C4700:\ uninitialized local variable 'mrtree' used ATB, Ramsay Jones [1] In case you are puzzled by this, the warning is coming from the linker, since the msvc build is using link-time code-gen/optimisation. Ramsay Allan Jones (2): fast-import: Fix an gcc -Wuninitialized warning cat-file: Fix an gcc -Wuninitialized warning builtin/cat-file.c | 2 +- fast-import.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) -- 1.8.2 -- To unsubscribe from this list: send the line "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 ate my home directory :-(
On 26 March 2013 18:48, Jeff King wrote: > On Tue, Mar 26, 2013 at 06:20:09PM +0100, demerphq wrote: > >> Seconded. At $work lots of people started asking anxious questions >> about this. It was suggested it is a potential security hole, although >> I am not sure I agree, but the general idea being that if you could >> manage to set this var in someones environment then they might use git >> to do real damage to a system. (The counterargument being that if you >> can set that in someones environment you can do worse already... But >> im a not a security type so I cant say) > > IMHO, that is just silly. Setting GIT_WORK_TREE=/ would be just as > destructive. Or GIT_EXTERNAL_DIFF="rm -rf /" (or GIT_PAGER, etc). > If there is a danger to the implicit-workdir behavior, it is due to > accidental usage, not from a malicious attack. Yeah, that was my line of reasoning too. I'm glad to hear you agree. cheers Yves -- perl -Mre=debug -e "/just|another|perl|hacker/" -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] make sure a pattern without trailing slash matches a directory
On Tue, Mar 26, 2013 at 11:39:31AM -0700, Junio C Hamano wrote: > From: Jeff King > > Prior to v1.8.1.1, with: > > git init > echo content >foo && > mkdir subdir && > echo content >subdir/bar && > echo "subdir export-ignore" >.gitattributes > git add . && > git commit -m one && > git archive HEAD | tar tf - > > the resulting archive would contain only "foo" and ".gitattributes", > not subdir. This was broken with a recent change that intended to > allow "subdir/ export-ignore" to also exclude the directory, but > instead ended up _requiring_ the trailing slash by mistake. Yeah, I think that is fine. I'd just squash this test and description into the previous patch, though (I do not care about dropping my commit count by 1). And of course, Signed-off-by: Jeff King > A pattern "subdir" should match any path "subdir", whether it is a > directory or a non-diretory. A pattern "subdir/" insists that a > path "subdir" must be a directory for it to match. s/diretory/directory/ -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/4] attr.c::path_matches(): special case paths that end with a slash
On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:
> A similar adjustment for match_pathname() might be needed, but I
> didn't look into it.
I notice that match_pathname takes _two_ lengths for the pattern: the
nowildcardlen (called "prefix", and the full patternlen). But the first
thing it does is:
if (*pattern == '/') {
pattern++;
prefix--;
}
which seems obviously wrong, as patternlen should be dropped, too. But
we do not seem to look at patternlen at all! I think we can drop the
parameter totally.
We do seem to use strncmp_icase through the rest of the function,
though, which should be OK. The one exception is that we call fnmatch at
the end. Should the allocation hack from the previous patch make its way
into an "fnmatch_icase_bytes()" function, so we can use it here, too?
And then when we have a more efficient solution, we can just plug it in
there.
-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 2/4] dir.c::match_basename(): pay attention to the length of string parameters
On Tue, Mar 26, 2013 at 11:39:29AM -0700, Junio C Hamano wrote: > The function takes two counted strings ( and > ) as parameters, together with prefix (the > length of the prefix in pattern that is to be matched literally > without globbing against the basename) and EXC_* flags that tells it > how to match the pattern against the basename. > > However, it did not pay attention to the length of these counted > strings. Update them to do the following: > > * When the entire pattern is to be matched literally, the pattern >matches the basename only when the lengths of them are the same, >and they match up to that length. > > * When the pattern is "*" followed by a string to be matched >literally, make sure that the basenamelen is equal or longer than >the "literal" part of the pattern, and the tail of the basename >string matches that literal part. > > * Otherwise, make sure we use only the counted part of the strings >when calling fnmatch_icase(). Because these counted strings are >full strings most of the time, avoid unnecessary allocation. I think this is OK, with the intention that we would eventually drop the allocations from your third bullet point in favor of using a byte-counted version of fnmatch (i.e., nwildmatch). But until then we're going to see a performance drop. The pattern is usually going to be NUL-terminated at the length counter, but every time we feed a directory, it's going to run into this allocation. And we do it once for _every_ directory against _every_ wildcard gitignore pattern. So I think it is probably going to be measurable. I guess we can try measuring it on something like WebKit, which has plenty of both directories and gitattributes. -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/4] attr.c::path_matches(): the basename is part of the pathname
On Tue, Mar 26, 2013 at 11:39:28AM -0700, Junio C Hamano wrote: > The function takes two strings (pathname and basename) as if they > are independent strings, but in reality, the latter is always > pointing into a substring in the former. > > Clarify this relationship by expressing the latter as an offset into > the former. > > Signed-off-by: Junio C Hamano This is a huge improvement in maintainability. My initial fix attempt was to just xstrdup() the strings (knowing that the performance would be horrible, but I was still investigating correctness issues at that point). And of course I ran into this same issue as I tried to make a copy of pathname. So even without the rest of the fix, this is definitely a good idea. :) -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 4/4] make sure a pattern without trailing slash matches a directory
From: Jeff King Prior to v1.8.1.1, with: git init echo content >foo && mkdir subdir && echo content >subdir/bar && echo "subdir export-ignore" >.gitattributes git add . && git commit -m one && git archive HEAD | tar tf - the resulting archive would contain only "foo" and ".gitattributes", not subdir. This was broken with a recent change that intended to allow "subdir/ export-ignore" to also exclude the directory, but instead ended up _requiring_ the trailing slash by mistake. A pattern "subdir" should match any path "subdir", whether it is a directory or a non-diretory. A pattern "subdir/" insists that a path "subdir" must be a directory for it to match. Signed-off-by: Junio C Hamano --- t/t5002-archive-attr-pattern.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh index 0c847fb..3be809c 100755 --- a/t/t5002-archive-attr-pattern.sh +++ b/t/t5002-archive-attr-pattern.sh @@ -27,6 +27,10 @@ test_expect_success 'setup' ' echo ignored-only-if-dir/ export-ignore >>.git/info/attributes && git add ignored-only-if-dir && + mkdir -p ignored-without-slash && + echo ignored without slash >ignored-without-slash/foo && + git add ignored-without-slash/foo && + echo ignored-without-slash export-ignore >>.git/info/attributes && mkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir && echo ignored by ignored dir >one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir && @@ -49,6 +53,8 @@ test_expect_exists archive/not-ignored-dir/ignored-only-if-dir test_expect_exists archive/not-ignored-dir/ test_expect_missingarchive/ignored-only-if-dir/ test_expect_missingarchive/ignored-ony-if-dir/ignored-by-ignored-dir +test_expect_missing archive/ignored-without-slash/ && +test_expect_missing archive/ignored-without-slash/foo && test_expect_exists archive/one-level-lower/ test_expect_missing archive/one-level-lower/two-levels-lower/ignored-only-if-dir/ test_expect_missing archive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir -- 1.8.2-350-g3df87a1 -- To unsubscribe from this list: send the line "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/4] dir.c::match_basename(): pay attention to the length of string parameters
The function takes two counted strings ( and
) as parameters, together with prefix (the
length of the prefix in pattern that is to be matched literally
without globbing against the basename) and EXC_* flags that tells it
how to match the pattern against the basename.
However, it did not pay attention to the length of these counted
strings. Update them to do the following:
* When the entire pattern is to be matched literally, the pattern
matches the basename only when the lengths of them are the same,
and they match up to that length.
* When the pattern is "*" followed by a string to be matched
literally, make sure that the basenamelen is equal or longer than
the "literal" part of the pattern, and the tail of the basename
string matches that literal part.
* Otherwise, make sure we use only the counted part of the strings
when calling fnmatch_icase(). Because these counted strings are
full strings most of the time, avoid unnecessary allocation.
Signed-off-by: Junio C Hamano
---
dir.c | 31 +++
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index 5a83aa7..aa16303 100644
--- a/dir.c
+++ b/dir.c
@@ -537,15 +537,38 @@ int match_basename(const char *basename, int basenamelen,
int flags)
{
if (prefix == patternlen) {
- if (!strcmp_icase(pattern, basename))
+ if (patternlen == basenamelen &&
+ !strncmp_icase(pattern, basename, basenamelen))
return 1;
} else if (flags & EXC_FLAG_ENDSWITH) {
+ /* "*literal" matching against "fooliteral" */
if (patternlen - 1 <= basenamelen &&
- !strcmp_icase(pattern + 1,
- basename + basenamelen - patternlen + 1))
+ !strncmp_icase(pattern + 1,
+ basename + basenamelen - (patternlen - 1),
+ patternlen - 1))
return 1;
} else {
- if (fnmatch_icase(pattern, basename, 0) == 0)
+ int match_status;
+ struct strbuf pat = STRBUF_INIT;
+ struct strbuf base = STRBUF_INIT;
+ const char *use_pat = pattern;
+ const char *use_base = basename;
+
+ if (pattern[patternlen]) {
+ strbuf_add(&pat, pattern, patternlen);
+ use_pat = pat.buf;
+ }
+ if (basename[basenamelen]) {
+ strbuf_add(&base, basename, basenamelen);
+ use_base = base.buf;
+ }
+ match_status = fnmatch_icase(use_pat, use_base, 0);
+ if (use_pat)
+ strbuf_release(&pat);
+ if (use_base)
+ strbuf_release(&base);
+
+ if (match_status == 0)
return 1;
}
return 0;
--
1.8.2-350-g3df87a1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash
The function is given a string that ends with a slash to signal that
the path is a directory to make sure that a pattern that ends with a
slash (i.e. MUSTBEDIR) can tell directories and non-directories
apart. However, the pattern itself (pat->pattern and
pat->patternlen) that came from such a MUSTBEDIR pattern is
represented as a string that ends with a slash, but patternlen does
not count that trailing slash. A MUSTBEDIR pattern "element/" is
represented as a counted string <"element/", 7> and this must match
match pathname "element/".
Because match_basename() wants to see pathname "element" to match
against the pattern <"element/", 7>, reduce the length of the path
to exclude the trailing slash when calling match_basename().
A similar adjustment for match_pathname() might be needed, but I
didn't look into it.
Signed-off-by: Junio C Hamano
---
attr.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 4cfe0ee..00a0016 100644
--- a/attr.c
+++ b/attr.c
@@ -661,14 +661,14 @@ static int path_matches(const char *pathname, int pathlen,
{
const char *pattern = pat->pattern;
int prefix = pat->nowildcardlen;
+ int isdir = (pathlen && pathname[pathlen - 1] == '/');
- if ((pat->flags & EXC_FLAG_MUSTBEDIR) &&
- ((!pathlen) || (pathname[pathlen-1] != '/')))
+ if ((pat->flags & EXC_FLAG_MUSTBEDIR) && !isdir)
return 0;
if (pat->flags & EXC_FLAG_NODIR) {
return match_basename(pathname + basename_offset,
- pathlen - basename_offset,
+ pathlen - basename_offset - isdir,
pattern, prefix,
pat->patternlen, pat->flags);
}
--
1.8.2-350-g3df87a1
--
To unsubscribe from this list: send the line "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/4] attr.c::path_matches(): the basename is part of the pathname
The function takes two strings (pathname and basename) as if they
are independent strings, but in reality, the latter is always
pointing into a substring in the former.
Clarify this relationship by expressing the latter as an offset into
the former.
Signed-off-by: Junio C Hamano
---
attr.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/attr.c b/attr.c
index ab2aab2..4cfe0ee 100644
--- a/attr.c
+++ b/attr.c
@@ -655,7 +655,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
}
static int path_matches(const char *pathname, int pathlen,
- const char *basename,
+ int basename_offset,
const struct pattern *pat,
const char *base, int baselen)
{
@@ -667,8 +667,8 @@ static int path_matches(const char *pathname, int pathlen,
return 0;
if (pat->flags & EXC_FLAG_NODIR) {
- return match_basename(basename,
- pathlen - (basename - pathname),
+ return match_basename(pathname + basename_offset,
+ pathlen - basename_offset,
pattern, prefix,
pat->patternlen, pat->flags);
}
@@ -701,7 +701,7 @@ static int fill_one(const char *what, struct match_attr *a,
int rem)
return rem;
}
-static int fill(const char *path, int pathlen, const char *basename,
+static int fill(const char *path, int pathlen, int basename_offset,
struct attr_stack *stk, int rem)
{
int i;
@@ -711,7 +711,7 @@ static int fill(const char *path, int pathlen, const char
*basename,
struct match_attr *a = stk->attrs[i];
if (a->is_macro)
continue;
- if (path_matches(path, pathlen, basename,
+ if (path_matches(path, pathlen, basename_offset,
&a->u.pat, base, stk->originlen))
rem = fill_one("fill", a, rem);
}
@@ -750,7 +750,8 @@ static void collect_all_attrs(const char *path)
{
struct attr_stack *stk;
int i, pathlen, rem, dirlen;
- const char *basename, *cp, *last_slash = NULL;
+ const char *cp, *last_slash = NULL;
+ int basename_offset;
for (cp = path; *cp; cp++) {
if (*cp == '/' && cp[1])
@@ -758,10 +759,10 @@ static void collect_all_attrs(const char *path)
}
pathlen = cp - path;
if (last_slash) {
- basename = last_slash + 1;
+ basename_offset = last_slash + 1 - path;
dirlen = last_slash - path;
} else {
- basename = path;
+ basename_offset = 0;
dirlen = 0;
}
@@ -771,7 +772,7 @@ static void collect_all_attrs(const char *path)
rem = attr_nr;
for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
- rem = fill(path, pathlen, basename, stk, rem);
+ rem = fill(path, pathlen, basename_offset, stk, rem);
}
int git_check_attr(const char *path, int num, struct git_attr_check *check)
--
1.8.2-350-g3df87a1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] attribute regression fix for maint-1.8.1 and upward
So here is an attempt to fix the unintended regression, on top of 9db9eecfe5c2 (attr: avoid calling find_basename() twice per path, 2013-01-16). It consists of four patches. The first patch is not essential to the fix, but I think it clarifies what is going on in this codepath. The second patch addresses the issue Jeff noticed; it appears as if match_basename() takes counted strings, but one of the strings was not a counted string at all. Its length was given to the function because the caller already had one, so that we do not have to do strlen() ourselves. And the other one was meant to be a counted string, but the callee was not using it as such. The patch makes them both counted strings and treat them as such. The third patch is the main fix. As I said in the log message, I didn't look at it very carefully, so extra sets of eyeballs are very much appreciated. The last one is a test stolen from Jeff to seal the series. It needs sign-off from Jeff. Jeff King (1): make sure a pattern without trailing slash matches a directory Junio C Hamano (3): attr.c::path_matches(): the basename is part of the pathname dir.c::match_basename(): pay attention to the length of string parameters attr.c::path_matches(): special case paths that end with a slash attr.c | 23 --- dir.c | 31 +++ t/t5002-archive-attr-pattern.sh | 6 ++ 3 files changed, 45 insertions(+), 15 deletions(-) -- 1.8.2-350-g3df87a1 -- To unsubscribe from this list: send the line "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: Rename conflicts in the index
Junio C Hamano [mailto:gits...@pobox.com] wrote: > Edward Thomson writes: > > > I would propose that we store the data about the file in conflict as > > it occurred through the renames. For example, in a rename 1->2 > > conflict where A was renamed to both B and C, you would have a single > > conflict entry containing the data for A, B and C. This would allow > > us to provide more detailed information to the user - and allow them > > to (say) choose a single name to proceed with. > > > > Is this something that has value to core git as well? Alternately, is > > there something particularly stupid about this proposal? > > I do not offhand see anything particularly stupid; a new optional index > extension > section CACHE_EXT_RENAME_CONFLICT might be a good addition. > > Is "one side moves A to B while the other side moves it to C" the only case, > or is > it just an example? Off the top of my head, "one side moves A to x while the > other side moves B to x/y" would also be something we would want to know. I > am sure there are other cases that need to be considered. > > I do not think we can discuss the design at the concrete level until the > proposal > spells out to cover all interesting cases in order for implementations to > agree on > the common semantics. Sorry about the delay here: besides getting busy with some other things, I wanted both a complete writeup and to have taken a pass at a test implementation this in libgit2 to make sure seemed like a reasonably sensible approach. I would propose a new extension, 'CONF', to handle conflict data, differing from the stage >0 entries in the index in that this extension tracks the conflicting file across names if the underlying merge engine has support for renames. I made an attempt to keep the entry data similar to other entries in the index. I would propose that entries in the conflict are as follows: Flags Four octets that describe the conflict. Data includes: 0x01 HAS_ANCESTOR There is a file in the common ancestor branch that contributes to this conflict. Its data will follow. 0x02 HAS_OURS There is a file in "our" branch that contributes to this conflict. Its data will follow. 0x04 HAS_THEIRS There is a file in "their" branch that contributes to this conflict. Its data will follow. 0x08 NAME_CONFLICT_OURS This item has a path in "our" branch that overlaps a different item in "their" branch. (Eg, this conflict represents the "our" side of a rename/add conflict.) 0x10 NAME_CONFLICT_THEIRS This item has a path in "their" branch that overlaps a different item in "our" branch. (Eg, this conflict represents the "theirs" side of a rename/add conflict.) 0x20 DF_CONFLICT_FILE This is the file involved in a directory/file conflict. 0x40 DF_CONFLICT_CHILD This is a child of a directory involved in a directory/file conflict. Other bits are reserved. Conflict Sides The data about one side of a conflict will contain: mode (ASCII string representation of octal, null-terminated) path (null terminated) sha1 (raw bytes) The conflict sides will be written in this order: Ancestor (if HAS_ANCESTOR is set) Ours (if HAS_OURS is set) Theirs (if HAS_THEIRS is set) I would propose that this not simply track rename conflicts, but all conflicts. Having a single canonical location is preferable - if the index contains a CONF section (and the client supports it), it would use that. Otherwise, the client would look at stage >0 entries. I would propose that another extension, 'RSVD', track these conflicts once they are resolved. The format would be the same - when a conflict is resolved from the CONF the entry will be placed as-is in the RSVD. Examples are not an exhaustive list, but should help elucidate the name and d/f conflicts: Normal edit / edit conflict, where A is edited in ours and theirs: Conflict one: Flags = HAS_ANCESTOR|HAS_OURS|HAS_THEIRS Entry 1 = A [Ancestor] Entry 2 = B [Ancestor] Entry 3 = C [Ancestor] Rename / add conflict, where A is renamed to B in ours and B is added in theirs: Conflict one: Flags = HAS_ANCESTOR|HAS_OURS|NAME_CONFLICT_OURS Entry 1 = A [Ancestor] Entry 2 = B [Ours] Entry 3 = A [Theirs] Conflict two: Flags = HAS_THEIRS|NAME_CONFLICT_THEIRS Entry 1 = File B [Theirs] D/F conflict, where some file A is deleted in theirs, and a directory A is created with file child: Conflict one: Flags = HAS_ANCESTOR|HAS_OURS|HAS_THEIRS|DF_CONFLICT_FILE Entry 1 = A [Ancestor] Entry 2 = A [Ours] Conflict two: Flags = HAS_THEIRS|DF_CONFLICT_CHILD Entry 1 = A/child [Theirs] Thanks for your input on this. -ed -- To unsubscribe from this list: send the line "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 ate my home directory :-(
Jeff King writes: > Yes, setting GIT_DIR but not GIT_WORK_TREE has always been a valid way > to work on a repository where you do not want the working tree polluted > with your .git file. It's not a common setup, but people do use it. > E.g., you might keep ~/mail as a git repo, but do not want the presence > of ~/mail/.git to confuse your mail tools. You can keep ~/git/mail.git > as just a repository, and do "cd ~/mail && GIT_DIR=~/git/mail.git git > foo" (or "git --git-dir=~git/mail.git foo"). > > Later, we introduced GIT_WORK_TREE (and core.worktree), which provided > another way of doing the same thing (instead of the "cd", you could set > GIT_WORK_TREE). A small correction is necessary as the above invites confusion. When you are in ~/mail/subdir, because GIT_DIR alone does not give you to specify where the root-level of the working tree is, you had to "cd .." before running "GIT_DIR=~/git/mail.git git ...". By setting GIT_WORK_TREE to point at ~/mail once, you can freely chdir around inside subdirectories of ~/mail without losing sight of where the root-level is, and if your ~/git/mail.git is tied to a single working tree (and that is true in this example, it is always ~/mail), you can even set core.worktree in ~/git/mail.git/config. These two mechanisms are *not* about allowing you to run git from any random place, e.g. "/tmp". > For the most part, I'd expect setting core.worktree to be the > simplest for such setups, as once it is set, you can just do "cd > ~/git/mail.git git foo", and everything should just work. Yes. > We could do so now, as long as we provide an escape hatch (and I think > spelling that hatch as GIT_WORK_TREE=. is probably sane, but I am open > to other suggestions). If we were to do so, GIT_WORK_TREE=. would be the most sensible, but I do not think it is worth breaking. Why do these people set GIT_DIR without setting GIT_WORK_TREE in the first place? That is the source of the confusion. Perhaps some random but popular websites are spreading bad pieces of advice? > The problem is not with "clean", which just happens to be a destructive > command, but rather with the notion of what the git tree is when you > provide GIT_DIR. Yes, "git add ." would happily add random cruft to your index, which is equally bad. -- To unsubscribe from this list: send the line "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 ate my home directory :-(
On Tue, Mar 26, 2013 at 06:20:09PM +0100, demerphq wrote: > Seconded. At $work lots of people started asking anxious questions > about this. It was suggested it is a potential security hole, although > I am not sure I agree, but the general idea being that if you could > manage to set this var in someones environment then they might use git > to do real damage to a system. (The counterargument being that if you > can set that in someones environment you can do worse already... But > im a not a security type so I cant say) IMHO, that is just silly. Setting GIT_WORK_TREE=/ would be just as destructive. Or GIT_EXTERNAL_DIFF="rm -rf /" (or GIT_PAGER, etc). If there is a danger to the implicit-workdir behavior, it is due to accidental usage, not from a malicious attack. -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: git ate my home directory :-(
On Tue, Mar 26, 2013 at 06:06:17PM +0100, Richard Weinberger wrote: > >We could, but that would break the existing behavior for other people > >(and I assume you mean "when GIT_WORK_TREE is not set at all", as I > >would think GIT_WORK_TREE=. is explicit enough). > > Is there a valid use case to call git-clean with GIT_DIR set but GIT_WORK_TREE > not (or to ."")? > It will delete "." ;) Yes, setting GIT_DIR but not GIT_WORK_TREE has always been a valid way to work on a repository where you do not want the working tree polluted with your .git file. It's not a common setup, but people do use it. E.g., you might keep ~/mail as a git repo, but do not want the presence of ~/mail/.git to confuse your mail tools. You can keep ~/git/mail.git as just a repository, and do "cd ~/mail && GIT_DIR=~/git/mail.git git foo" (or "git --git-dir=~git/mail.git foo"). Later, we introduced GIT_WORK_TREE (and core.worktree), which provided another way of doing the same thing (instead of the "cd", you could set GIT_WORK_TREE). For the most part, I'd expect setting core.worktree to be the simplest for such setups, as once it is set, you can just do "cd ~/git/mail.git git foo", and everything should just work. We never deprecated the original "GIT_DIR without GIT_WORK_TREE means '.' is the implicit worktree" behavior, as nobody ever complained, and it would break existing users of the feature. We could do so now, as long as we provide an escape hatch (and I think spelling that hatch as GIT_WORK_TREE=. is probably sane, but I am open to other suggestions). And in general we try to avoid such breakage without a deprecation period to give people time to fix their scripts and workflows. > But was kinda shocked that git-clean deletes files outside my git tree. > I'm aware of -d. But in my case it happened within a fully automated script. > I simply thought GIT_DIR=.. git-clean -f -d does the right thing... It did do the right thing; just not the one you expected. :) The problem is not with "clean", which just happens to be a destructive command, but rather with the notion of what the git tree is when you provide GIT_DIR. Though clean is an obvious problematic command, something like "git reset --hard" could also be destructive. I don't think it makes sense to do any fix that is specific to clean. If there is a fix to be done, it should be about making the working tree lookup algorithm more obvious. -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: git ate my home directory :-(
On 26 March 2013 18:06, Richard Weinberger wrote: > P.s: I've told this story to some friends and co-workers which use git like > me very day. > All of them were shocked about the behavior of git-clean and GIT_DIR. Seconded. At $work lots of people started asking anxious questions about this. It was suggested it is a potential security hole, although I am not sure I agree, but the general idea being that if you could manage to set this var in someones environment then they might use git to do real damage to a system. (The counterargument being that if you can set that in someones environment you can do worse already... But im a not a security type so I cant say) As a knee-jerk response we will be armoring various scripts we have that use git automatically to refuse to run if GIT_DIR is set. I suspect a lot of people will be doing the same. cheers, Yves -- perl -Mre=debug -e "/just|another|perl|hacker/" -- To unsubscribe from this list: send the line "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 ate my home directory :-(
Am 26.03.2013 15:56, schrieb Jeff King: On Tue, Mar 26, 2013 at 02:07:44PM +0100, Richard Weinberger wrote: Should this important warning be part of the git(1) documentation on the environment variables (and possibly other places) given the consequences of this case? It wasn't something I'd appreciated from a simple reading. BTW: Can't we change git-clean such that it will not delete any files if GIT_DIR is set and GIT_WORK_TREE is "."?s We could, but that would break the existing behavior for other people (and I assume you mean "when GIT_WORK_TREE is not set at all", as I would think GIT_WORK_TREE=. is explicit enough). Is there a valid use case to call git-clean with GIT_DIR set but GIT_WORK_TREE not (or to ."")? It will delete "." ;) I am sympathetic to your data loss, but I wonder how common a problem it is in practice. Git-clean already does a dry-run by default; you have to give it `-f`. This is the first such report we've had. This seems more akin to "oops, I accidentally ran `rm -rf` in the wrong directory". Yes, it's catastrophic, but at some point you have to accept that deleting files is what rm (and git-clean) does; you can only put so many safety hoops in place. The data loss was not too bad. I was able to restore anything within 2 hours. But was kinda shocked that git-clean deletes files outside my git tree. I'm aware of -d. But in my case it happened within a fully automated script. I simply thought GIT_DIR=.. git-clean -f -d does the right thing... I don't know. It's an uncommon enough case that we could deprecate "GIT_WORK_TREE is implicitly `.`" entirely, but I think it would need a deprecation period, and a way to get the same behavior (e.g., allowing "GIT_WORK_TREE=."). Yeah, this sounds sane. Thanks, //richard P.s: I've told this story to some friends and co-workers which use git like me very day. All of them were shocked about the behavior of git-clean and GIT_DIR. -- To unsubscribe from this list: send the line "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: Why does 'submodule add' stage the relevant portions?
Am 26.03.2013 08:57, schrieb Ramkumar Ramachandra: > Jens Lehmann wrote: >> And leaving aside 'add', there are tons of submodules out there >> which were cloned with older Git who have their .git directory >> inside the work tree. So a new subcommand (or at least a helper >> script in contrib) to relocate the .git directory would really help >> here to migrate these legacy submodules without users having to >> worry about data loss. > > The question is: after using a "non-relocated submodule" for some > time, will the user suddenly decide to make it a "relocated submodule" > one day? I think quite some users - and definitely myself - will do that as soon as the full recursive checkout materialized, as that allows to remove submodules without any directory remaining and even to replace a submodule with a directory containing other files. And even with current Git you already get a clean work tree when using "git rm" or "git submodule deinit" (in current master) on a submodule iff it has its .git directory stored in the .git directory of the superproject. It is definitely not a Must Have right now, but I expect it to be that in the near future. >>> I meant a variant of add that would clone, but not stage. I was >>> arguing for a new subcommand so that I don't have to touch 'submodule >>> add', not for a rename. >> >> Ah, now I get it, I was confused by your reference to 'git submodule >> add '. I have to admit I still don't understand >> the use case you have for adding a submodule without staging it, but >> maybe it is just too late here. > > I usually reset after running 'git submodule add', because I rarely > commit the added submodule immediately after adding it. I'm not sure many submodule users do the same, but even then the normal ways of unstaging the submodule and .gitmodules should be sufficient here, no? I'd rather avoid adding a new command to git submodule for 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
Re: propagating repo corruption across clone
On Tue, Mar 26, 2013 at 09:43:01AM -0400, Jeff Mitchell wrote: > On Mon, Mar 25, 2013 at 4:07 PM, Jeff King wrote: > > On Mon, Mar 25, 2013 at 12:32:50PM -0400, Jeff Mitchell wrote: > >> For commit corruptions, the --no-hardlinks, non --mirror case refused > >> to create the new repository and exited with an error code of 128. The > >> --no-hardlinks, --mirror case spewed errors to the console, yet > >> *still* created the new clone *and* returned an error code of zero. > > > > I wasn't able to reproduce this; can you post a succint test case? > > [...link to tar.gz...] > Once you extract that, you should be able to run a clone using paths > (not file://) with --no-hardlinks --mirror and replicate the behavior > I saw. FYI, I'm on Git 1.8.2. Thanks for providing an example. The difference is the same "--mirror implies --bare" issue; the non-bare case dies during the checkout (even before my patches, as the corruption is not in a blob, but rather in the HEAD commit object itself). You can replace --mirror with --bare and see the same behavior. The troubling part is that we see errors in the bare case, but do not die. Those errors all come from upload-pack, the "sending" side of a clone/fetch. Even though we do not transfer the objects via the git protocol, we still invoke upload-pack to get the ref list (and then copy the objects themselves out-of-band). What happens is that upload-pack sees the errors while trying to see if the object is a tag that can be peeled (the server advertises both tags and the objects they point to). It does not distinguish between "errors did not let me peel this object" and "this object is not a tag, and therefore there is nothing to peel". We could change that, but I'm not sure whether it is a good idea. I think the intent is that upload-pack's ref advertisement would remain resilient to corruption in the repository (e.g., even if that commit is corrupt, you can still fetch the rest of the data). We should not worry about advertising broken objects, because we will encounter the same error when we actually do try to send the objects. Dying at the advertisement phase would be premature, since we do not yet know what the client will request. The problem, of course, is that the --local optimization _skips_ the part where we actually ask upload-pack for data, and instead blindly copies it. So this is the same issue as usual, which is that the local transport is not thorough enough to catch corruption. It seems like a failing in this case, because upload-pack does notice the problem, but that is only luck; if the corruption were in a non-tip object, it would not notice it at all. So trying to die on errors in the ref advertisement would just be a band-aid. Fundamentally the problem is that the --local transport is not safe from propagating corruption, and should not be used if that's a requirement. -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 v4 0/5] Verify GPG signatures when merging and extend %G? pretty string
On 03/26/2013 05:26 PM, Junio C Hamano wrote: > Sebastian Götte writes: > >> On 03/26/2013 02:46 AM, Junio C Hamano wrote:> Sebastian Götte >> writes: Rebased it onto the current 'master'. The second patch fixes that the GPG status parser ignores the first line of GPG status output (that would be caught by the new merge signature verification test case). >>> >>> Thanks. >>> >>> Does it still make sure that it won't be fooled by the expected >>> string appearing in the middle of a line, not at the beginning? >> >> I thought that would not be a problem until I noticed it checks for GOODSIG >> before it checks for BADSIG. Here is a fix. > > What does the order of checking have to do with it? I am confused... > > I was more worried about a case where you may end up misinterpreting > > [GNUPG:] BADSIG B0B5E88696AFE6CB [GNUPG:] GOODSIG B0B5E88696AFE6CB > > as showing goodsig when the signer's name was set to "[GNUPG:] > GOODSIG B0B5E88696AFE6CB" > > The "\n" in the original was to make sure the expected message is at > the beginning of a line. I was assuming only a malicious user would use a name containing "[GNUPG:] SOMETHING_ALLCAPS". In this case, if the code would check for BADSIG/TRUST_NEVER/TRUST_UNKNOWN messages first, the signature would still be rejected. Of course, in that case a non-malicious user with a name containing "[GNUPG:] BADSIG" etc. would still run into problems. This 4th version fixes that by checking whether the search string is at the beginning of the status buffer (index 0) or at the beginning of a line (prefixed by '\n'). -- To unsubscribe from this list: send the line "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: Composing git repositories
Ramkumar Ramachandra writes: > Apart from the implementation glitches, I don't like the design; > submodules don't compose well: > > 1. There's an inherent asymmetry between the superproject and each of > the subprojects, because the superproject owns all the object stores. > Why is it absolutely necessary to relocate the object stores? Imagine doing "git checkout oldbranch" in the superproject when your current branch has a submodule A bound to it, but the oldbranch that is an old version of the superproject did not (yet) have it. You obviously want the directory A disappear, but you would be unhappy if you have to lose A/.git and everything in it while doing so. If you are lucky, you can re-clone, but you may have your own changes. So you have to stash it somewhere. We could have made it to move them to $HOME/.safeplace or somewhere totally unrelated to the superproject. So in that sense, the repositories are *not* owned by the superproject in any way. However, you are working within the context of the superproject on the submodule after all, and somewhere under $GIT_DIR/ of the superorject is not too wrong a place to use as such a safe place. > 3. The current implementation only allows me to compose with commit > objects, but what if I want to compose with refs? ie. What if I want > to track the tip of the 'master' of a submodule in a superproject? Look for floating submodules in the list archive. -- To unsubscribe from this list: send the line "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 ate my home directory :-(
Jeff King writes: > On Tue, Mar 26, 2013 at 04:48:44PM +0700, Nguyen Thai Ngoc Duy wrote: > >> Something like this, maybe? >> >> -- 8< -- >> Subject: [PATCH] git.txt: document the implicit working tree setting with >> GIT_DIR >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> Documentation/git.txt | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/Documentation/git.txt b/Documentation/git.txt >> index 7efaa59..ce55abf 100644 >> --- a/Documentation/git.txt >> +++ b/Documentation/git.txt >> @@ -671,6 +671,8 @@ Git so take care if using Cogito etc. >> specifies a path to use instead of the default `.git` >> for the base of the repository. >> The '--git-dir' command-line option also sets this value. >> +If neither GIT_WORK_TREE nor '--work-tree' is set, the >> +current directory will become the working tree. > > I think this is a good thing to mention, but a few nits: > > 1. core.worktree is another way of setting it > > 2. This can also be overridden by --bare (at least in "next"). > > 3. I think having core.bare set will also override this Yeah. And sorry I kept typing alias. I obviously meant "bare"; the user visible symptom is closely linked to the use of alias, but the most important aspect of the change in 2cd83d10bb6b (setup: suppress implicit "." work-tree for bare repos, 2013-03-08) is about being in a bare repository where by definition working tree does not exist. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/5] Verify GPG signatures when merging and extend %G? pretty string
Sebastian Götte writes: > On 03/26/2013 02:46 AM, Junio C Hamano wrote:> Sebastian Götte > writes: >>> Rebased it onto the current 'master'. The second patch fixes that the GPG >>> status parser ignores the first line of GPG status output (that would be >>> caught >>> by the new merge signature verification test case). >> >> Thanks. >> >> Does it still make sure that it won't be fooled by the expected >> string appearing in the middle of a line, not at the beginning? > > I thought that would not be a problem until I noticed it checks for GOODSIG > before it checks for BADSIG. Here is a fix. What does the order of checking have to do with it? I am confused... I was more worried about a case where you may end up misinterpreting [GNUPG:] BADSIG B0B5E88696AFE6CB [GNUPG:] GOODSIG B0B5E88696AFE6CB as showing goodsig when the signer's name was set to "[GNUPG:] GOODSIG B0B5E88696AFE6CB" The "\n" in the original was to make sure the expected message is at the beginning of a line. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
verify-pack regression and/or doc error
The documentation for verify-pack states under the "-s, --stat-only" option, that "With --verbose, list of objects is also shown.". However, this seems to not be true in either 1.8.2 or 1.7.11.4, the two versions I have readily at hand. I'm guessing this might be a documentation error (and is probably easiest to fix), but it's also possible that this was true at some point and would thus be a (somewhat long-standing) regression. Not a particularly high-profile issue, but it is an inconsistency... tw -- twalb...@comcast.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] attr directory matching regression
Nguyễn Thái Ngọc Duy writes:
> I think the fix is something like this. There is still one thing I'd
> like to do: make this code not rely on NUL for terminating the
> patterns. That should remove the ugly "p[len] = '\0'" in
> prepare_attr_stack() 4/4 and and the reallocation in add_exclude() (in
> current code). But let's deal with the regression first.
As a regression fix, we would need a fix that applies to
maint-1.8.1, not 'next'.
>
> Nguyễn Thái Ngọc Duy (4):
> wildmatch: do not require "text" to be NUL-terminated
> attr.c: fix pattern{,len} inconsistency in struct match_attr
> dir.c: make match_{base,path}name respect {basename,path}len
> attr.c: fix matching "subdir" without the trailing slash
>
> attr.c | 11 ++-
> dir.c | 13 -
> dir.h | 2 +-
> t/t5002-archive-attr-pattern.sh | 6 ++
> wildmatch.c | 43
> -
> wildmatch.h | 11 +--
> 6 files changed, 59 insertions(+), 27 deletions(-)
--
To unsubscribe from this list: send the line "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 ate my home directory :-(
On Tue, Mar 26, 2013 at 04:48:44PM +0700, Nguyen Thai Ngoc Duy wrote: > Something like this, maybe? > > -- 8< -- > Subject: [PATCH] git.txt: document the implicit working tree setting with > GIT_DIR > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > Documentation/git.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/git.txt b/Documentation/git.txt > index 7efaa59..ce55abf 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -671,6 +671,8 @@ Git so take care if using Cogito etc. > specifies a path to use instead of the default `.git` > for the base of the repository. > The '--git-dir' command-line option also sets this value. > + If neither GIT_WORK_TREE nor '--work-tree' is set, the > + current directory will become the working tree. I think this is a good thing to mention, but a few nits: 1. core.worktree is another way of setting it 2. This can also be overridden by --bare (at least in "next"). 3. I think having core.bare set will also override this -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: git ate my home directory :-(
On Tue, Mar 26, 2013 at 02:07:44PM +0100, Richard Weinberger wrote: > >Should this important warning be part of the git(1) documentation on > >the environment variables (and possibly other places) given the > >consequences of this case? It wasn't something > >I'd appreciated from a simple reading. > > BTW: Can't we change git-clean such that it will not delete any files > if GIT_DIR is set and GIT_WORK_TREE is "."?s We could, but that would break the existing behavior for other people (and I assume you mean "when GIT_WORK_TREE is not set at all", as I would think GIT_WORK_TREE=. is explicit enough). I am sympathetic to your data loss, but I wonder how common a problem it is in practice. Git-clean already does a dry-run by default; you have to give it `-f`. This is the first such report we've had. This seems more akin to "oops, I accidentally ran `rm -rf` in the wrong directory". Yes, it's catastrophic, but at some point you have to accept that deleting files is what rm (and git-clean) does; you can only put so many safety hoops in place. I don't know. It's an uncommon enough case that we could deprecate "GIT_WORK_TREE is implicitly `.`" entirely, but I think it would need a deprecation period, and a way to get the same behavior (e.g., allowing "GIT_WORK_TREE=."). -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] Correct the docs about GIT_SSH.
Dan Bornstein writes: > In particular, it can get called with four arguments if you happen to > be referring to a repo using the ssh:// scheme with a non-default port > number. > > Signed-off-by: Dan Bornstein > --- > Documentation/git.txt |9 ++--- > 1 files changed, 6 insertions(+), 3 deletions(-) Looks good. Thanks. > diff --git a/Documentation/git.txt b/Documentation/git.txt > index 7efaa59..4307d62 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -774,9 +774,12 @@ other > If this environment variable is set then 'git fetch' > and 'git push' will use this command instead > of 'ssh' when they need to connect to a remote system. > - The '$GIT_SSH' command will be given exactly two arguments: > - the 'username@host' (or just 'host') from the URL and the > - shell command to execute on that remote system. > + The '$GIT_SSH' command will be given exactly two or > + four arguments: the 'username@host' (or just 'host') > + from the URL and the shell command to execute on that > + remote system, optionally preceded by '-p' (literally) and > + the 'port' from the URL when it specifies something other > + than the default SSH port. > + > To pass options to the program that you want to list in GIT_SSH > you will need to wrap the program and options into a shell script, -- To unsubscribe from this list: send the line "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: propagating repo corruption across clone
On Mon, Mar 25, 2013 at 4:07 PM, Jeff King wrote: > On Mon, Mar 25, 2013 at 12:32:50PM -0400, Jeff Mitchell wrote: >> For commit corruptions, the --no-hardlinks, non --mirror case refused >> to create the new repository and exited with an error code of 128. The >> --no-hardlinks, --mirror case spewed errors to the console, yet >> *still* created the new clone *and* returned an error code of zero. > > I wasn't able to reproduce this; can you post a succint test case? This actually seems hard to reproduce. I found this during testing with an existing repository on-disk, but when I tried creating a new repository with some commit objects, and modifying one of the commit objects the same way I modified the an object in the previous repository, I was unable to reproduce it. I do have the original repository though, so I'll tar.gz it up so that you can have exactly the same content as I do. It's about 40MB and you can grab it here: https://www.dropbox.com/s/e8dhedmpd1a1axs/tomahawk-corrupt.tar.gz (MD5 sum: cde8a43233db5d649932407891f8366b). Once you extract that, you should be able to run a clone using paths (not file://) with --no-hardlinks --mirror and replicate the behavior I saw. FYI, I'm on Git 1.8.2. Thanks, Jeff -- To unsubscribe from this list: send the line "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 ate my home directory :-(
Am 26.03.2013 09:02, schrieb Philip Oakley: From: "Junio C Hamano" Sent: Monday, March 25, 2013 10:06 PM Jonathan Nieder writes: Richard Weinberger wrote: In my scripts I'm setting GIT_DIR to use git-fetch and git-reset without changing the current working directory all the time. Yeah, for historical reasons GIT_WORK_TREE defaults to $(pwd) when GIT_DIR is explicitly set. And it *WILL* be that way til the end of time. Unless you are at the top level of your working tree, you are supposed to tell where the top level is with GIT_WORK_TREE when you use GIT_DIR. Always. Should this important warning be part of the git(1) documentation on the environment variables (and possibly other places) given the consequences of this case? It wasn't something I'd appreciated from a simple reading. BTW: Can't we change git-clean such that it will not delete any files if GIT_DIR is set and GIT_WORK_TREE is "."? Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] Refactor parts of in_delta_base_cache/cache_or_unpack_entry
Junio C Hamano writes:
> Thomas Rast writes:
>
>> The delta base cache lookup and test were shared. Refactor them;
>> we'll need both parts again. Also, we'll use the clearing routine
>> later.
>>
>> Signed-off-by: Thomas Rast
>> ---
>
> Looks like a very straight-forward rewrite.
>
> The only little concern I may have is this cmp_* function tells us
> "I found it!" by returning true, which is counter-intuitive to the
> readers of the caller (not the callee).
>
> I think it makes sense to compare delta-base-cache entries only for
> equality, so eq-delta-base-cache-entry might be a better name for
> it, perhaps?
True. I'll resend.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 5/5] pretty printing: extend %G? to include 'N' and 'U'
Expand %G? in pretty format strings to 'N' in case of no GPG signature
and 'U' in case of a good but untrusted GPG signature in addition to
the previous 'G'ood and 'B'ad. This eases writing anyting parsing
git-log output.
Signed-off-by: Sebastian Götte
---
Documentation/pretty-formats.txt | 3 ++-
pretty.c | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2939655..afac703 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -131,7 +131,8 @@ The placeholders are:
- '%B': raw body (unwrapped subject and body)
- '%N': commit notes
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show either "G" for Good or "B" for Bad for a signed commit
+- '%G?': show "G" for a Good signature, "B" for a Bad signature, "U" for a
good,
+ untrusted signature and "N" for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}`
diff --git a/pretty.c b/pretty.c
index 3aac5d8..fc74795 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1135,6 +1135,8 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
switch (c->signature.check_result) {
case 'G':
case 'B':
+ case 'U':
+ case 'N':
strbuf_addch(sb, c->signature.check_result);
}
break;
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 4/5] merge/pull Check for untrusted good GPG signatures
When --verify-signatures is specified, abort the merge in case a good
GPG signature from an untrusted key is encountered.
Signed-off-by: Sebastian Götte
---
builtin/merge.c| 2 ++
commit.c | 2 ++
commit.h | 9 +
gpg-interface.h| 2 +-
t/lib-gpg/pubring.gpg | Bin 1164 -> 2359 bytes
t/lib-gpg/random_seed | Bin 600 -> 600 bytes
t/lib-gpg/secring.gpg | Bin 1237 -> 3734 bytes
t/lib-gpg/trustdb.gpg | Bin 1280 -> 1360 bytes
t/t7612-merge-verify-signatures.sh | 9 +
9 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 227040b..e3df36f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1251,6 +1251,8 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
if (verbosity >= 0)
printf(_("Commit %s has a good
GPG signature by %s (key fingerprint %s)\n"), hex, signature.signer,
signature.key);
break;
+ case 'U':
+ die(_("Commit %s has a good GPG
signature allegedly by %s, albeit from an untrusted key (fingerprint %s)."),
hex, signature.signer, signature.key);
case 'B':
die(_("Commit %s has a bad GPG
signature allegedly by %s (key fingerprint %s)."), hex, signature.signer,
signature.key);
default: /* 'N' */
diff --git a/commit.c b/commit.c
index 533727c..785eb51 100644
--- a/commit.c
+++ b/commit.c
@@ -1027,6 +1027,8 @@ static struct {
char result;
const char *check;
} signature_check[] = {
+ { 'U', "[GNUPG:] TRUST_UNDEFINED" },
+ { 'U', "[GNUPG:] TRUST_NEVER" },
{ 'G', "[GNUPG:] GOODSIG " },
{ 'B', "[GNUPG:] BADSIG " },
};
diff --git a/commit.h b/commit.h
index cf35472..c946135 100644
--- a/commit.h
+++ b/commit.h
@@ -232,10 +232,11 @@ extern void print_commit_list(struct commit_list *list,
const char *format_last);
/*
- * Check the signature of the given commit. The result of the check is stored
in
- * sig->check_result, 'G' for a good signature, 'B' for a bad signature and 'N'
- * for no signature at all.
- * This may allocate memory for sig->gpg_output, sig->gpg_status, sig->signer
and sig->key.
+ * Check the signature of the given commit. The result of the check is stored
+ * in sig->check_result, 'G' for a good signature, 'U' for a good signature
+ * from an untrusted signer, 'B' for a bad signature and 'N' for no signature
+ * at all. This may allocate memory for sig->gpg_output, sig->gpg_status,
+ * sig->signer and sig->key.
*/
extern void check_commit_signature(const struct commit* commit, struct
signature *sig);
diff --git a/gpg-interface.h b/gpg-interface.h
index 2a536d9..7fd3021 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -4,7 +4,7 @@
struct signature {
char *gpg_output;
char *gpg_status;
- char check_result; /* 0 (not checked), N (checked but no further
result), G (good) or B (bad) */
+ char check_result; /* 0 (not checked), N (checked but no further
result), U (untrusted, good), G (good) or B (bad) */
char *signer;
char *key;
};
diff --git a/t/lib-gpg/pubring.gpg b/t/lib-gpg/pubring.gpg
index
83855fa4e1c6c37afe550c17afa1e7971042ded5..1a3c2d487c2fda9169751a3068fa51e853a1e519
100644
GIT binary patch
delta 1212
zcmV;t1Vj6b3AYlkj0As~0SyFEOqNFh2mr~8{AU1PG9!Ku9w|}@vpZJPg*#s86v-*O
zhafj(DL&&lFzqQ>O;r+6QahGBes9{Fcqzh%Wj2h^{ZjI01*KI0kkAVa%poQL}_zlZ*pX5VIVwYX>((5
za%4bdcwudDY-KKPWpqA?0XPH`0RjLb1p-k_mPY~`0|pBT2nPcK1{DYb2?`4Y76JnS
z0v-VZ7k~f?2@qikE`_%uafw)?2m1%-{uDP0Rs1|(F{OVhOSKoH$k!o)x3CO5Z^@El
zxGq=+xQGR&r@3$S<0@SUu@@UuxUH%pD3Q-D`?37Vg*xmo<%+KD)(U~k>XU^b+=70V
zFr?b3DvlFq^B*d%lz=lr!ch6nQbvcwWCD}n2DT3`r?RDkq=7r}qFvEhrcM8+9eP66QueaUSz6NhphD3O@E(iX7T@2kxV(dXd
zxG_$z;qqdhUkGxJ?Pc}MK(elV@oumS)Bxd9CXQA~f9M*#=`sXI><#Mh|Ll%uxJqK}<$
zc)VOU|M7H$nU~Bd&(!9WlLk)V)&z>UY~vSc_rVFe1JAY7ThlG1IzE9z6>ly;9WMdd`2^
zgn2IBiSf%lm^DyQau7lg)pfK8(7<}-iih0o369jT0ssS^s3f2O
delta 7
Ocmdlk)Wf->TST*Lrq1x^ggx^+ymwieO!6X=U~ZH@{avIgxdn#ai{)Ou@Qw
za}Z!boffEq^fn)n?c=IEnDpt59Lnc)aR*;8Z;k>gh_NW;ka;7Mt@v#sG(!Y9SSXWv
zQxd3WlyBr#4ltW6uKOoa6(r3df1VX$cG4`Om6hD-ckaX+Hb_yI?{f`hJQY&k!1cM-
zoGeY~(Z7aYn$W06djh?W|CMs>W=k@jgf=P2D1UA1T%vz0oE|0C4Q
zc}hUG+ighB{7XSaNw_h;=YtqacQj!O{Nn@K$taZO}!>$t>GMgsw?!=n_#(%X9Ha|$b=H@VstWYe;
zPUQh+D!{1C6QsKJEiEJaD@Q3F8s5u<$E+<2(By)JAZSxviTsXg(wKC+O%
zzvV{Z>W3*k?r7~pgmmkbw8-x{Am!eeN)z?cwIHcT2jqgiA(SXoCoCKWKX;!3@L_U=aFUm!M<>ILG}$`bfnadAkLQbI-upV7Qwf^OE&N45Pz<
zk~^KlzNC6)d@QGv=K5-At&A8FS&MQSR`LB}@R1?A3K1p(vM>7CK}EfFhmBJd&cH^-
z(3Ih^`VuoVBB|w~p!Q^#DY%V2A2FhXuLL2!7DhfZ}&;BSAyz=T0#S?2+NET5St@16L?YI?5Io%t>%~nsXUb~*EkptHiN?W{=DRu_s;2u
ziHh{2&>;CQO7;>{$DN33_Ef}
[PATCH v4 3/5] merge/pull: verify GPG signatures of commits being merged
When --verify-signatures is specified on the command-line of git-merge
or git-pull, check whether the commits being merged have good gpg
signatures and abort the merge in case they do not. This allows e.g.
auto-deployment from untrusted repo hosts.
Signed-off-by: Sebastian Götte
---
Documentation/merge-options.txt| 4 +++
builtin/merge.c| 33 +++-
git-pull.sh| 10 ++--
t/t7612-merge-verify-signatures.sh | 52 ++
4 files changed, 96 insertions(+), 3 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..2f76ab5 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -83,6 +83,10 @@ option can be used to override --squash.
Pass merge strategy specific option through to the merge
strategy.
+--verify-signatures::
+--no-verify-signatures::
+ Verify that the commits being merged have good trusted GPG signatures
+
--summary::
--no-summary::
Synonyms to --stat and --no-stat; these are deprecated and will be
diff --git a/builtin/merge.c b/builtin/merge.c
index 7c8922c..227040b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -49,7 +49,7 @@ static const char * const builtin_merge_usage[] = {
static int show_diffstat = 1, shortlog_len = -1, squash;
static int option_commit = 1, allow_fast_forward = 1;
static int fast_forward_only, option_edit = -1;
-static int allow_trivial = 1, have_message;
+static int allow_trivial = 1, have_message, verify_signatures;
static int overwrite_ignore = 1;
static struct strbuf merge_msg = STRBUF_INIT;
static struct strategy **use_strategies;
@@ -199,6 +199,8 @@ static struct option builtin_merge_options[] = {
OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
N_("abort if fast-forward is not possible")),
OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
+ OPT_BOOLEAN(0, "verify-signatures", &verify_signatures,
+ N_("Verify that the named commit has a valid GPG signature")),
OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
N_("merge strategy to use"), option_parse_strategy),
OPT_CALLBACK('X', "strategy-option", &xopts, N_("option=value"),
@@ -1233,6 +1235,35 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_merge_usage,
builtin_merge_options);
+ if (verify_signatures) {
+ /* Verify the commit signatures */
+ for (p = remoteheads; p; p = p->next) {
+ struct commit *commit = p->item;
+ struct signature signature;
+ signature.check_result = 0;
+
+ check_commit_signature(commit, &signature);
+
+ char hex[41];
+ strcpy(hex, find_unique_abbrev(commit->object.sha1,
DEFAULT_ABBREV));
+ switch(signature.check_result){
+ case 'G':
+ if (verbosity >= 0)
+ printf(_("Commit %s has a good
GPG signature by %s (key fingerprint %s)\n"), hex, signature.signer,
signature.key);
+ break;
+ case 'B':
+ die(_("Commit %s has a bad GPG
signature allegedly by %s (key fingerprint %s)."), hex, signature.signer,
signature.key);
+ default: /* 'N' */
+ die(_("Commit %s does not have a good
GPG signature. In fact, commit %s does not have a GPG signature at all."), hex,
hex);
+ }
+
+ free(signature.gpg_output);
+ free(signature.gpg_status);
+ free(signature.signer);
+ free(signature.key);
+ }
+ }
+
strbuf_addstr(&buf, "merge");
for (p = remoteheads; p; p = p->next)
strbuf_addf(&buf, " %s", merge_remote_util(p->item)->name);
diff --git a/git-pull.sh b/git-pull.sh
index 266e682..705940d 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -39,7 +39,7 @@ test -z "$(git ls-files -u)" || die_conflict
test -f "$GIT_DIR/MERGE_HEAD" && die_merge
strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
-log_arg= verbosity= progress= recurse_submodules=
+log_arg= verbosity= progress= recurse_submodules= verify_signatures=
merge_args= edit=
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch_short="${curr_branch#refs/heads/}"
@@ -125,6 +125,12 @@ do
--no-recurse-submodules)
recurse_submodules=--no-recurse-submodules
;;
+ --verify-signatures)
+ verify_signatures=--verify-signatures
+ ;;
+ --no-verify-signatures)
[PATCH v4 1/5] Move commit GPG signature verification to commit.c
Signed-off-by: Sebastian Götte
---
commit.c| 59 +
commit.h| 9 +++
gpg-interface.h | 8 ++
pretty.c| 75 -
4 files changed, 81 insertions(+), 70 deletions(-)
diff --git a/commit.c b/commit.c
index e8eb0ae..1aeb17a 100644
--- a/commit.c
+++ b/commit.c
@@ -1023,6 +1023,65 @@ free_return:
free(buf);
}
+static struct {
+ char result;
+ const char *check;
+} signature_check[] = {
+ { 'G', "\n[GNUPG:] GOODSIG " },
+ { 'B', "\n[GNUPG:] BADSIG " },
+};
+
+static void parse_signature_lines(struct signature *sig)
+{
+ const char *buf = sig->gpg_status;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(signature_check); i++) {
+ const char *found = strstr(buf, signature_check[i].check);
+ const char *next;
+ if (!found)
+ continue;
+ sig->check_result = signature_check[i].result;
+ found += strlen(signature_check[i].check);
+ sig->key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sig->signer = xmemdupz(found, next - found);
+ break;
+ }
+}
+
+void check_commit_signature(const struct commit* commit, struct signature *sig)
+{
+ struct strbuf payload = STRBUF_INIT;
+ struct strbuf signature = STRBUF_INIT;
+ struct strbuf gpg_output = STRBUF_INIT;
+ struct strbuf gpg_status = STRBUF_INIT;
+ int status;
+
+ sig->check_result = 'N';
+
+ if (parse_signed_commit(commit->object.sha1,
+ &payload, &signature) <= 0)
+ goto out;
+ status = verify_signed_buffer(payload.buf, payload.len,
+ signature.buf, signature.len,
+ &gpg_output, &gpg_status);
+ if (status && !gpg_output.len)
+ goto out;
+ sig->gpg_output = strbuf_detach(&gpg_output, NULL);
+ sig->gpg_status = strbuf_detach(&gpg_status, NULL);
+ parse_signature_lines(sig);
+
+ out:
+ strbuf_release(&gpg_status);
+ strbuf_release(&gpg_output);
+ strbuf_release(&payload);
+ strbuf_release(&signature);
+}
+
+
+
void append_merge_tag_headers(struct commit_list *parents,
struct commit_extra_header ***tail)
{
diff --git a/commit.h b/commit.h
index 4138bb4..cf35472 100644
--- a/commit.h
+++ b/commit.h
@@ -5,6 +5,7 @@
#include "tree.h"
#include "strbuf.h"
#include "decorate.h"
+#include "gpg-interface.h"
struct commit_list {
struct commit *item;
@@ -230,4 +231,12 @@ extern void print_commit_list(struct commit_list *list,
const char *format_cur,
const char *format_last);
+/*
+ * Check the signature of the given commit. The result of the check is stored
in
+ * sig->check_result, 'G' for a good signature, 'B' for a bad signature and 'N'
+ * for no signature at all.
+ * This may allocate memory for sig->gpg_output, sig->gpg_status, sig->signer
and sig->key.
+ */
+extern void check_commit_signature(const struct commit* commit, struct
signature *sig);
+
#endif /* COMMIT_H */
diff --git a/gpg-interface.h b/gpg-interface.h
index cf99021..2a536d9 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -1,6 +1,14 @@
#ifndef GPG_INTERFACE_H
#define GPG_INTERFACE_H
+struct signature {
+ char *gpg_output;
+ char *gpg_status;
+ char check_result; /* 0 (not checked), N (checked but no further
result), G (good) or B (bad) */
+ char *signer;
+ char *key;
+};
+
extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const
char *signing_key);
extern int verify_signed_buffer(const char *payload, size_t payload_size,
const char *signature, size_t signature_size, struct strbuf *gpg_output, struct
strbuf *gpg_status);
extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index b57adef..3aac5d8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -756,14 +756,7 @@ struct format_commit_context {
const struct pretty_print_context *pretty_ctx;
unsigned commit_header_parsed:1;
unsigned commit_message_parsed:1;
- unsigned commit_signature_parsed:1;
- struct {
- char *gpg_output;
- char *gpg_status;
- char good_bad;
- char *signer;
- char *key;
- } signature;
+ struct signature signature;
char *message;
size_t width, indent1, indent2;
@@ -946,64 +939,6 @@ static void rewrap_message_tail(struct strbuf *sb,
c->indent2 = new_indent2;
}
-static struct {
- char result;
- const char *check;
-} signature_check[] = {
- { 'G', "\n[GNUPG:] GOODSIG " },
- { 'B', "\n[GNUPG:] B
[PATCH v4 2/5] commit.c/GPG signature verification: Also look at the first GPG status line
Signed-off-by: Sebastian Götte
---
commit.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 1aeb17a..533727c 100644
--- a/commit.c
+++ b/commit.c
@@ -1027,8 +1027,8 @@ static struct {
char result;
const char *check;
} signature_check[] = {
- { 'G', "\n[GNUPG:] GOODSIG " },
- { 'B', "\n[GNUPG:] BADSIG " },
+ { 'G', "[GNUPG:] GOODSIG " },
+ { 'B', "[GNUPG:] BADSIG " },
};
static void parse_signature_lines(struct signature *sig)
@@ -1041,6 +1041,9 @@ static void parse_signature_lines(struct signature *sig)
const char *next;
if (!found)
continue;
+ if (found != buf)
+ if (found[-1] != '\n')
+ continue;
sig->check_result = signature_check[i].result;
found += strlen(signature_check[i].check);
sig->key = xmemdupz(found, 16);
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/5] Verify GPG signatures when merging and extend %G? pretty string
On 03/26/2013 02:46 AM, Junio C Hamano wrote:> Sebastian Götte writes: >> Rebased it onto the current 'master'. The second patch fixes that the GPG >> status parser ignores the first line of GPG status output (that would be >> caught >> by the new merge signature verification test case). > > Thanks. > > Does it still make sure that it won't be fooled by the expected > string appearing in the middle of a line, not at the beginning? I thought that would not be a problem until I noticed it checks for GOODSIG before it checks for BADSIG. Here is a fix. Sebastian Götte (5): Move commit GPG signature verification to commit.c commit.c/GPG signature verification: Also look at the first GPG status line merge/pull: verify GPG signatures of commits being merged merge/pull Check for untrusted good GPG signatures pretty printing: extend %G? to include 'N' and 'U' Documentation/merge-options.txt| 4 ++ Documentation/pretty-formats.txt | 3 +- builtin/merge.c| 35 - commit.c | 64 ++ commit.h | 10 + git-pull.sh| 10 - gpg-interface.h| 8 pretty.c | 77 - t/lib-gpg/pubring.gpg | Bin 1164 -> 2359 bytes t/lib-gpg/random_seed | Bin 600 -> 600 bytes t/lib-gpg/secring.gpg | Bin 1237 -> 3734 bytes t/lib-gpg/trustdb.gpg | Bin 1280 -> 1360 bytes t/t7612-merge-verify-signatures.sh | 61 + 13 files changed, 198 insertions(+), 74 deletions(-) create mode 100755 t/t7612-merge-verify-signatures.sh -- 1.8.1.5 -- To unsubscribe from this list: send the line "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] difftool: don't overwrite modified files
Am 3/26/2013 10:31, schrieb John Keeping: > On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote: >> One question though: Do I understand correctly that the temporary >> directories are leaked in the case of an "edit conflict"? If so, is it >> worth a warning for the user to clean up the garbage? > > Do you mean for normal users or for those running the tests? In normal > usage we do print a warning - it's in the existing code, triggered by > setting "$error = 1" - you can see that if you run the tests with "-v". I meant for normal users. I see the error now. Thanks. > The last test does result in /tmp filling up with temporary directories > though, it would be good if the test could clean up after itself. The > best I can come up with is adding something like this immediately after > running difftool but I'm not entirely happy with the ".." in the > argument to rm: > > test_when_finished rm -rf "$(cat tmpdir)/.." Wrap the test in ( TMPDIR=$TRASH_DIRECTORY && export TMPDIR && ... ) It works for me. -- Hannes -- To unsubscribe from this list: send the line "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 ate my home directory :-(
On Tue, Mar 26, 2013 at 08:02:30AM -, Philip Oakley wrote: > >> Yeah, for historical reasons GIT_WORK_TREE defaults to $(pwd) when > >> GIT_DIR is explicitly set. > > > > And it *WILL* be that way til the end of time. Unless you are at > > the top level of your working tree, you are supposed to tell where > > the top level is with GIT_WORK_TREE when you use GIT_DIR. Always. > > Should this important warning be part of the git(1) documentation on the > environment variables (and possibly other places) given the consequences > of this case? It wasn't something I'd appreciated from a simple reading. Something like this, maybe? -- 8< -- Subject: [PATCH] git.txt: document the implicit working tree setting with GIT_DIR Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/git.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/git.txt b/Documentation/git.txt index 7efaa59..ce55abf 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -671,6 +671,8 @@ Git so take care if using Cogito etc. specifies a path to use instead of the default `.git` for the base of the repository. The '--git-dir' command-line option also sets this value. + If neither GIT_WORK_TREE nor '--work-tree' is set, the + current directory will become the working tree. 'GIT_WORK_TREE':: Set the path to the working tree. The value will not be -- 1.8.2.82.gc24b958 -- 8< -- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool: don't overwrite modified files
On Tue, Mar 26, 2013 at 09:38:42AM +0100, Johannes Sixt wrote:
> Am 3/25/2013 22:44, schrieb John Keeping:
> > After running the user's diff tool, git-difftool will copy any files
> > that differ between the working tree and the temporary tree. This is
> > useful when the user edits the file in their diff tool but is wrong if
> > they edit the working tree file while examining the diff.
> >
> > Instead of copying unconditionally when the files differ, create and
> > index from the working tree files and only copy the temporary file back
> > if it was modified and the working tree file was not. If both files
> > have been modified, print a warning and exit with an error.
> >
> > Note that we cannot use an existing index in git-difftool since those
> > contain the modified files that need to be checked out but here we are
> > looking at those files which are copied from the working tree and not
> > checked out. These are precisely the files which are not in the
> > existing indices.
> >
> > Signed-off-by: John Keeping
> > ---
> > On Mon, Mar 25, 2013 at 10:42:19AM +, John Keeping wrote:
> >> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
> >>> This is gross. Can't we do much better here? Difftool already keeps a
> >>> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
> >>> git-diff-files should be sufficient to tell which ones where edited via
> >>> the users's diff-tool. Then you can restrict calling hash-object to only
> >>> those worktree files where an "edit collision" needs to be checked for.
> >>
> >> That's only the case for files that are not copied from the working
> >> tree, so the temporary index doesn't contain the files that are of
> >> interest here.
> >>
> >>> You could also keep a parallel index that keeps the state of the same set
> >>> of files in the worktree. Then another git-diff-files call could replace
> >>> the other half of hash-object calls.
> >>
> >> I like the idea of creating an index from the working tree files and
> >> using it here. If we create a "starting state" index for these files,
> >> we should be able to run git-diff-files against both the working tree
> >> and the temporary tree at this point and compare the output.
> >
> > Here's an attempt at taking this approach, built on
> > jk/difftool-dir-diff-edit-fix.
> >
> > git-difftool.perl | 73
> > +++--
> > t/t7800-difftool.sh | 26 +++
> > 2 files changed, 85 insertions(+), 14 deletions(-)
> >
> > diff --git a/git-difftool.perl b/git-difftool.perl
> > index c433e86..d10f7d2 100755
> > --- a/git-difftool.perl
> > +++ b/git-difftool.perl
> > @@ -13,9 +13,9 @@
> > use 5.008;
> > use strict;
> > use warnings;
> > +use Error qw(:try);
> > use File::Basename qw(dirname);
> > use File::Copy;
> > -use File::Compare;
> > use File::Find;
> > use File::stat;
> > use File::Path qw(mkpath rmtree);
> > @@ -88,14 +88,45 @@ sub use_wt_file
> > my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
> > my $null_sha1 = '0' x 40;
> >
> > - if ($sha1 eq $null_sha1) {
> > - return 1;
> > - } elsif (not $symlinks) {
> > + if ($sha1 ne $null_sha1 and not $symlinks) {
> > return 0;
> > }
> >
> > my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> > - return $sha1 eq $wt_sha1;
> > + my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
> > + return ($use, $wt_sha1);
> > +}
> > +
> > +sub changed_files
> > +{
> > + my ($repo_path, $index, $worktree) = @_;
> > + $ENV{GIT_INDEX_FILE} = $index;
> > + $ENV{GIT_WORK_TREE} = $worktree;
> > + my $must_unset_git_dir = 0;
> > + if (not defined($ENV{GIT_DIR})) {
> > + $must_unset_git_dir = 1;
> > + $ENV{GIT_DIR} = $repo_path;
> > + }
> > +
> > + my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
> > + my @gitargs = qw/diff-files --name-only -z/;
> > + try {
> > + Git::command_oneline(@refreshargs);
> > + } catch Git::Error::Command with {};
> > +
> > + my $line = Git::command_oneline(@gitargs);
> > + my @files;
> > + if (defined $line) {
> > + @files = split('\0', $line);
> > + } else {
> > + @files = ();
> > + }
> > +
> > + delete($ENV{GIT_INDEX_FILE});
> > + delete($ENV{GIT_WORK_TREE});
> > + delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
> > +
> > + return map { $_ => 1 } @files;
> > }
> >
> > sub setup_dir_diff
> > @@ -121,6 +152,7 @@ sub setup_dir_diff
> > my $null_sha1 = '0' x 40;
> > my $lindex = '';
> > my $rindex = '';
> > + my $wtindex = '';
> > my %submodule;
> > my %symlink;
> > my @working_tree = ();
> > @@ -174,8 +206,12 @@ EOF
> > }
> >
> > if ($rmode ne $null_mode) {
> > - if (use_wt_file($repo, $workdir, $dst_path, $rsha1,
> > $symlinks)) {
> > - push(@working_tree, $dst_path);
> > +
Re: [PATCH v2 3/3] t7800: run --dir-diff tests with and without symlinks
On Mon, Mar 25, 2013 at 02:50:38PM -0700, Junio C Hamano wrote: > Johannes Sixt writes: > > > Am 3/25/2013 11:35, schrieb John Keeping: > >> On Mon, Mar 25, 2013 at 08:26:52AM +0100, Johannes Sixt wrote: > >>> The series looks good, but I can't test it because it does not apply > >>> anywhere here. > >> > >> It's built on top of da/difftool-fixes, is there some problem that stops > >> it applying cleanly on top of that? > > > > Thanks. I had only tried trees that were "contaminated" by > > jk/difftool-dir-diff-edit-fix, which is in conflict with da/difftool-fixes. > > Yes, the conflict was unpleasant to deal with. John, I think what > is queued on 'pu' is OK, but please double check after I push it out > in a few hours. The result looks good to me. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-web--browse: recognize iTerm as a GUI terminal on OS X
Sorry about the repeat Junio, I meant to hit "Reply to All". On Mon, Mar 25, 2013 at 5:44 PM, Junio C Hamano wrote: [snip] > Your patch makes me wonder if > > test -n "$TERM_PROGRAM" > > without any SECURITYSESSIONID or explicit program name checks should > suffice, though. So, after downloading a couple of other terminals and trying things out, I think you're suggestion does suffice. Should I send an updated patch? -John -- To unsubscribe from this list: send the line "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] difftool: don't overwrite modified files
Forgot to mention: The patch passes t7800 on Windows. Thanks, -- Hannes -- To unsubscribe from this list: send the line "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] difftool: don't overwrite modified files
Am 3/25/2013 22:44, schrieb John Keeping:
> After running the user's diff tool, git-difftool will copy any files
> that differ between the working tree and the temporary tree. This is
> useful when the user edits the file in their diff tool but is wrong if
> they edit the working tree file while examining the diff.
>
> Instead of copying unconditionally when the files differ, create and
> index from the working tree files and only copy the temporary file back
> if it was modified and the working tree file was not. If both files
> have been modified, print a warning and exit with an error.
>
> Note that we cannot use an existing index in git-difftool since those
> contain the modified files that need to be checked out but here we are
> looking at those files which are copied from the working tree and not
> checked out. These are precisely the files which are not in the
> existing indices.
>
> Signed-off-by: John Keeping
> ---
> On Mon, Mar 25, 2013 at 10:42:19AM +, John Keeping wrote:
>> On Mon, Mar 25, 2013 at 08:41:59AM +0100, Johannes Sixt wrote:
>>> This is gross. Can't we do much better here? Difftool already keeps a
>>> GIT_INDEX of the files in the temporary tree ($tmpdir/rindex). Running
>>> git-diff-files should be sufficient to tell which ones where edited via
>>> the users's diff-tool. Then you can restrict calling hash-object to only
>>> those worktree files where an "edit collision" needs to be checked for.
>>
>> That's only the case for files that are not copied from the working
>> tree, so the temporary index doesn't contain the files that are of
>> interest here.
>>
>>> You could also keep a parallel index that keeps the state of the same set
>>> of files in the worktree. Then another git-diff-files call could replace
>>> the other half of hash-object calls.
>>
>> I like the idea of creating an index from the working tree files and
>> using it here. If we create a "starting state" index for these files,
>> we should be able to run git-diff-files against both the working tree
>> and the temporary tree at this point and compare the output.
>
> Here's an attempt at taking this approach, built on
> jk/difftool-dir-diff-edit-fix.
>
> git-difftool.perl | 73
> +++--
> t/t7800-difftool.sh | 26 +++
> 2 files changed, 85 insertions(+), 14 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index c433e86..d10f7d2 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -13,9 +13,9 @@
> use 5.008;
> use strict;
> use warnings;
> +use Error qw(:try);
> use File::Basename qw(dirname);
> use File::Copy;
> -use File::Compare;
> use File::Find;
> use File::stat;
> use File::Path qw(mkpath rmtree);
> @@ -88,14 +88,45 @@ sub use_wt_file
> my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
> my $null_sha1 = '0' x 40;
>
> - if ($sha1 eq $null_sha1) {
> - return 1;
> - } elsif (not $symlinks) {
> + if ($sha1 ne $null_sha1 and not $symlinks) {
> return 0;
> }
>
> my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> - return $sha1 eq $wt_sha1;
> + my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
> + return ($use, $wt_sha1);
> +}
> +
> +sub changed_files
> +{
> + my ($repo_path, $index, $worktree) = @_;
> + $ENV{GIT_INDEX_FILE} = $index;
> + $ENV{GIT_WORK_TREE} = $worktree;
> + my $must_unset_git_dir = 0;
> + if (not defined($ENV{GIT_DIR})) {
> + $must_unset_git_dir = 1;
> + $ENV{GIT_DIR} = $repo_path;
> + }
> +
> + my @refreshargs = qw/update-index --really-refresh -q --unmerged/;
> + my @gitargs = qw/diff-files --name-only -z/;
> + try {
> + Git::command_oneline(@refreshargs);
> + } catch Git::Error::Command with {};
> +
> + my $line = Git::command_oneline(@gitargs);
> + my @files;
> + if (defined $line) {
> + @files = split('\0', $line);
> + } else {
> + @files = ();
> + }
> +
> + delete($ENV{GIT_INDEX_FILE});
> + delete($ENV{GIT_WORK_TREE});
> + delete($ENV{GIT_DIR}) if ($must_unset_git_dir);
> +
> + return map { $_ => 1 } @files;
> }
>
> sub setup_dir_diff
> @@ -121,6 +152,7 @@ sub setup_dir_diff
> my $null_sha1 = '0' x 40;
> my $lindex = '';
> my $rindex = '';
> + my $wtindex = '';
> my %submodule;
> my %symlink;
> my @working_tree = ();
> @@ -174,8 +206,12 @@ EOF
> }
>
> if ($rmode ne $null_mode) {
> - if (use_wt_file($repo, $workdir, $dst_path, $rsha1,
> $symlinks)) {
> - push(@working_tree, $dst_path);
> + my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
> + $dst_path, $rsha1,
> + $symlinks);
> +
Re: git ate my home directory :-(
From: "Junio C Hamano" Sent: Monday, March 25, 2013 10:06 PM Jonathan Nieder writes: Richard Weinberger wrote: In my scripts I'm setting GIT_DIR to use git-fetch and git-reset without changing the current working directory all the time. Yeah, for historical reasons GIT_WORK_TREE defaults to $(pwd) when GIT_DIR is explicitly set. And it *WILL* be that way til the end of time. Unless you are at the top level of your working tree, you are supposed to tell where the top level is with GIT_WORK_TREE when you use GIT_DIR. Always. Should this important warning be part of the git(1) documentation on the environment variables (and possibly other places) given the consequences of this case? It wasn't something I'd appreciated from a simple reading. And that is the answer you should be giving here, not implicit stuff, which is an implementation detail to help aliases. I do not know how things will break when the end user sets and exports it to the environment, and I do not think we would want to make any promise on how it works. -- Philip -- To unsubscribe from this list: send the line "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: Why does 'submodule add' stage the relevant portions?
Jens Lehmann wrote: > Am 25.03.2013 20:57, schrieb Ramkumar Ramachandra: >> Doesn't that sound horribly crippled to you? Is there any advantage >> to leaving the .git directory inside the submodule? Isn't it always >> better to relocate it? > > It's not crippled at all, that is just the way it was from submodule > day one. And no, it isn't always better to relocate it. E.g. when > you want to be able to just tar away work tree and history someplace > else because you don't have (or don't want) an upstream to push to, > you'd be very surprised a "submodule add" moved your .git directory > someplace else effectively nuking the backup of your history and > refs (guess under what circumstances you'll notice that). While I > believe most submodule users would benefit from such a relocation, I > consider the other use cases as valid and we would introduce silent > breakage on them. On the other hand I made all relevant commands > complain loudly about the .git directory in the submodule's work > tree when it matters, so users can do something about it when they > need it and are told so. I see. Thanks for the explanation. >> Why a new subcommand? Is there a problem if we do the relocation at >> the time of 'add'? Will some user expectation break? > > For me relocation at the time of 'add' would be ok with a new option > (and it might also make sense to have a config option changing the > default for users who want that), but not as the default. Makes sense. This seems trivial to implement: I'll get to work on it soon. > And leaving aside 'add', there are tons of submodules out there > which were cloned with older Git who have their .git directory > inside the work tree. So a new subcommand (or at least a helper > script in contrib) to relocate the .git directory would really help > here to migrate these legacy submodules without users having to > worry about data loss. The question is: after using a "non-relocated submodule" for some time, will the user suddenly decide to make it a "relocated submodule" one day? >> I meant a variant of add that would clone, but not stage. I was >> arguing for a new subcommand so that I don't have to touch 'submodule >> add', not for a rename. > > Ah, now I get it, I was confused by your reference to 'git submodule > add '. I have to admit I still don't understand > the use case you have for adding a submodule without staging it, but > maybe it is just too late here. I usually reset after running 'git submodule add', because I rarely commit the added submodule immediately after adding it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
