* Re: [PATCH 1/2] get_sha1_with_context: report features used in resolution
From: Junio C Hamano @ 2012-01-12 2:36 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, git, Albert Astals Cid
In-Reply-To: <20120111194210.GA12441@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Most callers generally treat get_sha1 as a black box, giving
> it a string from the user and expecting to get a sha1 in
> return. The get_sha1_with_context function gives callers
> more information about what happened while resolving the
> object name so they can make better decisions about how to
> use the result. We currently use this only to provide
> information about the path entry used to find a blob.
>
> We don't currently provide any information about the
> resolution rules that were used to reach the final object.
> Some callers may want these in order to enforce a policy
> that a particular subset of the lookup rules are used (e.g.,
> when serving remote requests).
>
> This patch adds a set of bit-fields that document the use of
> particular features during an object lookup.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The diffstat looks a little scary, but it is mostly just the internal
> get_sha1 functions learning to pass the object_context around.
Hmm, shouldn't this also cover peel_to_type()? That would have made it
also apply to the maintenance track.
^ permalink raw reply
* Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation
From: Junio C Hamano @ 2012-01-12 2:46 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Jeff King, git, Albert Astals Cid
In-Reply-To: <1326283958-30271-1-git-send-email-cmn@elego.de>
Carlos Martín Nieto <cmn@elego.de> writes:
> The tightening done in (ee27ca4a: archive: don't let remote clients
> get unreachable commits, 2011-11-17) went too far and disallowed
> HEAD:Documentation as it would try to find "HEAD:Documentation" as a
> ref.
I do not think it went too far. Actually we discussed this exact issue
when the topic was cooking, and saw no objections. The commit in question
itself advertises this restriction.
Why are we loosening it now? I do not see a compelling reason to do so.
> Only DWIM the "HEAD" part to see if it exists as a ref. Once we're
> sure that we've been given a valid ref, we follow the normal code
> path. This still disallows attempts to access commits which are not
> branch tips.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
> AFAICT this should still be safe. Using HEAD^:Documentation or
> <sha1>:Documentation still complains that HEAD^ and <sha1> aren't
> refs.
Having said that, I think I agree this is a safe thing to do, _if_ we want
to loosen it.
^ permalink raw reply
* Re: [PATCH 1/2] get_sha1_with_context: report features used in resolution
From: Jeff King @ 2012-01-12 2:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git, Albert Astals Cid
In-Reply-To: <7vmx9t4pgj.fsf@alter.siamese.dyndns.org>
On Wed, Jan 11, 2012 at 06:36:12PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Most callers generally treat get_sha1 as a black box, giving
> > it a string from the user and expecting to get a sha1 in
> > return. The get_sha1_with_context function gives callers
> > more information about what happened while resolving the
> > object name so they can make better decisions about how to
> > use the result. We currently use this only to provide
> > information about the path entry used to find a blob.
> >
> > We don't currently provide any information about the
> > resolution rules that were used to reach the final object.
> > Some callers may want these in order to enforce a policy
> > that a particular subset of the lookup rules are used (e.g.,
> > when serving remote requests).
> >
> > This patch adds a set of bit-fields that document the use of
> > particular features during an object lookup.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > The diffstat looks a little scary, but it is mostly just the internal
> > get_sha1 functions learning to pass the object_context around.
>
> Hmm, shouldn't this also cover peel_to_type()? That would have made it
> also apply to the maintenance track.
I don't see how peel_to_type is relevant. As far as get_sha1 is
concerned, the interesting thing is actually calling peel_onion. It does
get the context passed to it in my patch, but I didn't bother marking
that the peel feature was used (because it wasn't relevant to the policy
I wanted to implement in the follow-on patch).
But we could pretty easily mark the use of the peel feature, too.
I'm not sure what you mean about the maintenance track, though. AFAICT,
we don't separately call peel_to_type, but just potentially use it as
part of get_sha1_with_context. Am I missing something?
-Peff
^ permalink raw reply
* Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries
From: Nguyen Thai Ngoc Duy @ 2012-01-12 2:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Jeff King, Will Palmer
In-Reply-To: <7vr4z654m3.fsf@alter.siamese.dyndns.org>
2012/1/12 Junio C Hamano <gitster@pobox.com>:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> When running "commit" and "status" with files marked with "intent to add",
>>> I think there are three possible interpretations of what the user
>>> wants to do.
>> [ (1) thanks for stopping me, I had forgotten about that file;
>> (2) I changed my mind, please leave that file out; or (3) please
>> dwim and add the file ]
>>
>> I think (3) was a trick --- no one that does not use the "-a" option
>> would want that. :)
>
> I really wish it were the case, but I doubt it.
>
> People from other VCS background seem to still think that "commit" without
> paths should commit everything; after getting told that "what you add to
> the index is what you will commit", I can easily see this commint: but but
> but I told Git that I _want_ to add with -N! Why aren't you committing it?
I see "-N" just as an indication, not really an "add" operation.
Perhaps update-index is a better place for it.
>> (2) makes intent-to-add entries just like any other tracked index
>> entry with some un-added content.
>
> You are comparing files edited in the working tree without the user
> telling anything about them to Git (both tracked and untracked) and files
> the user explicitly told Git that the user hasn't made up her mind
> about. Why is it a good thing to make the latter behave "just like any
> other"?
The way I see this flag is "include these files in my diff in addition
to tracked files", and therefore should not have any effects at commit
time. I might turn some of those extra files to tracked some time if I
want to commit.
--
Duy
^ permalink raw reply
* Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation
From: Jeff King @ 2012-01-12 2:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git, Albert Astals Cid
In-Reply-To: <7vipkh4oyn.fsf@alter.siamese.dyndns.org>
On Wed, Jan 11, 2012 at 06:46:56PM -0800, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
>
> > The tightening done in (ee27ca4a: archive: don't let remote clients
> > get unreachable commits, 2011-11-17) went too far and disallowed
> > HEAD:Documentation as it would try to find "HEAD:Documentation" as a
> > ref.
>
> I do not think it went too far. Actually we discussed this exact issue
> when the topic was cooking, and saw no objections. The commit in question
> itself advertises this restriction.
I think you and I discussed it off list (I originally took this off-list
because the original issue did have some security implications). So I
don't think people necessarily had a chance to object.
> Why are we loosening it now? I do not see a compelling reason to do so.
I see it the opposite way. People are clearly using the "$ref:$path"
syntax. So why would we restrict them from doing so? There are no
security implications (i.e., they could always just grab $ref and
extract $path themselves). In my view, ee27ca4a was over-eager in its
restrictions because I wanted it to be simple and close the hole. Now we
can take our time adding more code to loosen it.
-Peff
^ permalink raw reply
* Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation
From: Jeff King @ 2012-01-12 2:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git, Albert Astals Cid
In-Reply-To: <20120112025445.GB25365@sigill.intra.peff.net>
On Wed, Jan 11, 2012 at 09:54:45PM -0500, Jeff King wrote:
> On Wed, Jan 11, 2012 at 06:46:56PM -0800, Junio C Hamano wrote:
>
> > Carlos Martín Nieto <cmn@elego.de> writes:
> >
> > > The tightening done in (ee27ca4a: archive: don't let remote clients
> > > get unreachable commits, 2011-11-17) went too far and disallowed
> > > HEAD:Documentation as it would try to find "HEAD:Documentation" as a
> > > ref.
> >
> > I do not think it went too far. Actually we discussed this exact issue
> > when the topic was cooking, and saw no objections. The commit in question
> > itself advertises this restriction.
>
> I think you and I discussed it off list (I originally took this off-list
> because the original issue did have some security implications). So I
> don't think people necessarily had a chance to object.
Here is the only on-list discussion:
http://article.gmane.org/gmane.comp.version-control.git/186366
Quoted below:
>> * jk/maint-1.6.2-upload-archive (2011-11-21) 1 commit
>> - archive: don't let remote clients get unreachable commits
>> (this branch is used by jk/maint-upload-archive.)
>>
>> * jk/maint-upload-archive (2011-11-21) 1 commit
>> - Merge branch 'jk/maint-1.6.2-upload-archive' into
>> jk/maint-upload-archive
>> (this branch uses jk/maint-1.6.2-upload-archive.)
>>
>> Will merge to 'next' after taking another look.
>
> Thanks. I also have some followup patches to re-loosen to at least
> trees reachable from refs. Do you want to leave the tightening to
> the maint track, and then consider the re-loosening for master?
I was planning to first have the really tight version graduate to
'master' and ship it in 1.7.9, while possibly merging that to 1.7.8.X
series. If we hear complaints from real users in the meantime before
or after such releases, we could apply loosening patch on top of these
topics and call them "regression fix", but I have been assuming that
nobody would have been using this backdoor for anything that really
matters.
So now we have heard a complaint. :)
-Peff
^ permalink raw reply
* Re: [PATCH v3 07/10] clone: --branch=<branch> always means refs/heads/<branch>
From: Nguyen Thai Ngoc Duy @ 2012-01-12 3:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7v4nw26mbe.fsf@alter.siamese.dyndns.org>
2012/1/12 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> It does not make sense to look outside refs/heads for HEAD's target
>> (src_ref_prefix can be set to "refs/" if --mirror is used) because ref
>> code only allows symref in form refs/heads/...
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> builtin/clone.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 05224d7..960242f 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -463,7 +463,7 @@ static void update_remote_refs(const struct ref *refs,
>> static void update_head(const struct ref *our, const struct ref *remote,
>> const char *msg)
>> {
>> - if (our) {
>> + if (our && !prefixcmp(our->name, "refs/heads/")) {
>> /* Local default branch link */
>> create_symref("HEAD", our->name, NULL);
>> if (!option_bare) {
>
> I think this makes sense. In the following three casees:
>
> - When cloning without --branch, if we could not find a branch that match
> HEAD at the remote;
>
> - When cloning with --branch, if we did not see such a branch at the
> remote; or
>
> - When cloning from an empty repository
>
> we come here with "our" set to NULL. Additionally, if the remote HEAD
> points outside refs/heads/ and the transport could detect that case
> (e.g. a helper that reads from ls-remote output), we can see our->name
> outside refs/heads/. Is there any other case where our is not NULL and
> our->name does not start with refs/heads/?
No, this patch pretty much guarantees that our->name must be inside
refs/heads, unless remote's HEAD points outside, which makes it a
broken remote and git-upload-pack should refuse to serve in the first
place. Until --branch=tag comes into the picture.
> The "else if (remote)" clause (not shown, outside the context) that
> follows still has comments that says it is a case for "Source had detached
> HEAD pointing somewhere", and needs to be adjusted for this change, no? It
> is "(1) we know the HEAD points at a non-branch, (2) HEAD may point at a
> branch but we do not know which one, or (3) we asked for a specific branch
> but it did not exist there" (cloning from an empty will have NULL in
> remote and the comment would not apply to that case).
Yes.
--
Duy
^ permalink raw reply
* Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation
From: Junio C Hamano @ 2012-01-12 3:03 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, git, Albert Astals Cid
In-Reply-To: <20120112025445.GB25365@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I see it the opposite way. People are clearly using the "$ref:$path"
> syntax. So why would we restrict them from doing so? There are no
> security implications (i.e., they could always just grab $ref and
> extract $path themselves). In my view, ee27ca4a was over-eager in its
> restrictions because I wanted it to be simple and close the hole. Now we
> can take our time adding more code to loosen it.
Ok, so it is more like a partial revert of whatever we did. In that case,
I'd take CMN's patch to limit the extent of the changes, as it more
closely matches the spirit of the original ee27ca4 (archive: don't let
remote clients get unreachable commits, 2011-11-17) that singled out and
catered to the need of "archive" command alone. It is already part of the
v1.7.8.1 release, so I would prefer a change to be stupid and simple.
^ permalink raw reply
* Re: leaky cherry-pick
From: Jeff King @ 2012-01-12 3:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ramkumar Ramachandra, Pete Wyckoff, git,
Nguyễn Thái Ngọc
In-Reply-To: <7v7h0x6b89.fsf@alter.siamese.dyndns.org>
On Wed, Jan 11, 2012 at 04:00:38PM -0800, Junio C Hamano wrote:
> Yeah, that is definitely a leak.
Here it is with a commit message. "valgrind git cherry-pick" reports no
leaks in the attr code after this (but unfortunately still lots of leaks
from unpack-trees).
-Peff
-- >8 --
Subject: [PATCH] attr: fix leak in free_attr_elem
This function frees the individual "struct match_attr"s we
have allocated, but forgot to free the array holding their
pointers, leading to a very minor memory leak.
Signed-off-by: Jeff King <peff@peff.net>
---
attr.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/attr.c b/attr.c
index 96eda0e..303751f 100644
--- a/attr.c
+++ b/attr.c
@@ -301,6 +301,7 @@ static void free_attr_elem(struct attr_stack *e)
}
free(a);
}
+ free(e->attrs);
free(e);
}
--
1.7.9.rc0.33.gd3c17
^ permalink raw reply related
* Re: [PATCH RFC] commit: allow to commit even if there are intent-to-add entries
From: Junio C Hamano @ 2012-01-12 3:05 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Jonathan Nieder, git, Jeff King, Will Palmer
In-Reply-To: <CACsJy8CHOuZMP4hWp6Lb_TbdfwSgofSg_0tDZ4oDcD0veie2Dw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>> I really wish it were the case, but I doubt it.
>>
>> People from other VCS background seem to still think that "commit" without
>> paths should commit everything; after getting told that "what you add to
>> the index is what you will commit", I can easily see this commint: but but
>> but I told Git that I _want_ to add with -N! Why aren't you committing it?
>
> I see "-N" just as an indication, not really an "add" operation.
But the thing is, what _you_, who knows a bit more than these new people
about Git, see does not matter, because they would not listen to you.
^ permalink raw reply
* Re: [PATCHv3 10/13] credentials: add "cache" helper
From: Jeff King @ 2012-01-12 3:07 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git
In-Reply-To: <20120111235009.GB30243@burratino>
On Wed, Jan 11, 2012 at 05:50:10PM -0600, Jonathan Nieder wrote:
> Thanks again for the fix. Here's another quick nit.
My favorite form for nits to come in: a patch.
> -- >8 --
> Subject: unix-socket: do not let close() or chdir() clobber errno during cleanup
> [...]
Looks good to me. Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation
From: Jeff King @ 2012-01-12 3:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git, Albert Astals Cid
In-Reply-To: <7vehv54o6v.fsf@alter.siamese.dyndns.org>
On Wed, Jan 11, 2012 at 07:03:36PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I see it the opposite way. People are clearly using the "$ref:$path"
> > syntax. So why would we restrict them from doing so? There are no
> > security implications (i.e., they could always just grab $ref and
> > extract $path themselves). In my view, ee27ca4a was over-eager in its
> > restrictions because I wanted it to be simple and close the hole. Now we
> > can take our time adding more code to loosen it.
>
> Ok, so it is more like a partial revert of whatever we did. In that case,
> I'd take CMN's patch to limit the extent of the changes, as it more
> closely matches the spirit of the original ee27ca4 (archive: don't let
> remote clients get unreachable commits, 2011-11-17) that singled out and
> catered to the need of "archive" command alone. It is already part of the
> v1.7.8.1 release, so I would prefer a change to be stupid and simple.
For a maint release, I am OK with that. In the long term, I'd rather my
patches go onto master (either for 1.7.9 or for later), as I think they
are the right way to do it.
-Peff
^ permalink raw reply
* Re: [PATCH] archive: re-allow HEAD:Documentation on a remote invocation
From: Junio C Hamano @ 2012-01-12 3:20 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, git, Albert Astals Cid
In-Reply-To: <20120112031022.GC26363@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> For a maint release, I am OK with that. In the long term, I'd rather my
> patches go onto master (either for 1.7.9 or for later), as I think they
> are the right way to do it.
I would prefer not to use it in 1.7.9, as CMN's patch will make its only
immediate user disappear. I think we are in agreement that the change to
carry context around is a longer term thing, and when we apply it, the
safety we added to "archive" will become simpler.
^ permalink raw reply
* [PATCH] tree_entry_interesting: make recursive mode default
From: Nguyễn Thái Ngọc Duy @ 2012-01-12 4:09 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jonathan Nieder, Linus Torvalds,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20120111063104.GA3153@burratino>
There is a bit of history behind this. Some time ago, t_e_i() only
supported prefix matching. diff-tree supported recursive and
non-recursive mode but it did not make any difference to prefix
matching.
Later on, t_e_i() gained limited recursion support to unify a similar
matching code used by git-grep. It introduced two new fields in struct
pathspec: max_depth and recursive. "recursive" field functions as a
feature switch so that this feature is off by default.
Some time after that, t_e_i() further gained wildcard support. With
wildcard matching, recursive and non-recursive diff-tree
mattered. "recursive" field was reused to distinguish recursion in
diff-tree.
This choice has a side effect that by default wildcard matching is in
non-recursive mode, which is not true. All current call sites except
"diff-tree without -r" (grep, traverse_commit_list, tree-walk and
general tree diff) prefer recursive mode.
This patch decouples the use of recursive field. The max_depth feature
switch is now controlled by max_depth_valid field. diff-tree recursion
is controlled by nonrecursive_diff_tree, which makes it recursive by
default.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Junio, log/diff family takes wildcards just fine (even before this
patch, just less reliable). If it's not mentioned in release notes
before, perhaps it's worth a line in 1.7.9 annoucement.
builtin/grep.c | 2 +-
cache.h | 3 ++-
dir.c | 4 ++--
t/t4010-diff-pathspec.sh | 8 ++++++++
tree-diff.c | 3 +--
tree-walk.c | 8 ++++----
6 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 9ce064a..c081bf4 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1051,7 +1051,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
paths = get_pathspec(prefix, argv + i);
init_pathspec(&pathspec, paths);
pathspec.max_depth = opt.max_depth;
- pathspec.recursive = 1;
+ pathspec.max_depth_valid = 1;
if (show_in_pager && (cached || list.nr))
die(_("--open-files-in-pager only works on the worktree"));
diff --git a/cache.h b/cache.h
index 10afd71..f58e05a 100644
--- a/cache.h
+++ b/cache.h
@@ -526,7 +526,8 @@ struct pathspec {
const char **raw; /* get_pathspec() result, not freed by free_pathspec() */
int nr;
unsigned int has_wildcard:1;
- unsigned int recursive:1;
+ unsigned int max_depth_valid:1;
+ unsigned int nonrecursive_diff_tree:1;
int max_depth;
struct pathspec_item {
const char *match;
diff --git a/dir.c b/dir.c
index 0a78d00..5af3567 100644
--- a/dir.c
+++ b/dir.c
@@ -258,7 +258,7 @@ int match_pathspec_depth(const struct pathspec *ps,
int i, retval = 0;
if (!ps->nr) {
- if (!ps->recursive || ps->max_depth == -1)
+ if (!ps->max_depth_valid || ps->max_depth == -1)
return MATCHED_RECURSIVELY;
if (within_depth(name, namelen, 0, ps->max_depth))
@@ -275,7 +275,7 @@ int match_pathspec_depth(const struct pathspec *ps,
if (seen && seen[i] == MATCHED_EXACTLY)
continue;
how = match_pathspec_item(ps->items+i, prefix, name, namelen);
- if (ps->recursive && ps->max_depth != -1 &&
+ if (ps->max_depth_valid && ps->max_depth != -1 &&
how && how != MATCHED_FNMATCH) {
int len = ps->items[i].len;
if (name[len] == '/')
diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
index fbc8cd8..af5134b 100755
--- a/t/t4010-diff-pathspec.sh
+++ b/t/t4010-diff-pathspec.sh
@@ -48,6 +48,14 @@ test_expect_success \
compare_diff_raw current expected'
cat >expected <<\EOF
+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M path1/file1
+EOF
+test_expect_success \
+ '"*file1" should show path1/file1' \
+ 'git diff-index --cached $tree -- "*file1" >current &&
+ compare_diff_raw current expected'
+
+cat >expected <<\EOF
:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M file0
EOF
test_expect_success \
diff --git a/tree-diff.c b/tree-diff.c
index 28ad6db..164693f 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -137,8 +137,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
enum interesting t2_match = entry_not_interesting;
/* Enable recursion indefinitely */
- opt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
- opt->pathspec.max_depth = -1;
+ opt->pathspec.nonrecursive_diff_tree = !DIFF_OPT_TST(opt, RECURSIVE);
strbuf_init(&base, PATH_MAX);
strbuf_add(&base, base_str, baselen);
diff --git a/tree-walk.c b/tree-walk.c
index f82dba6..04f1b81 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -588,7 +588,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
entry_not_interesting : all_entries_not_interesting;
if (!ps->nr) {
- if (!ps->recursive || ps->max_depth == -1)
+ if (!ps->max_depth_valid || ps->max_depth == -1)
return all_entries_interesting;
return within_depth(base->buf + base_offset, baselen,
!!S_ISDIR(entry->mode),
@@ -609,7 +609,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
if (!match_dir_prefix(base_str, match, matchlen))
goto match_wildcards;
- if (!ps->recursive || ps->max_depth == -1)
+ if (!ps->max_depth_valid || ps->max_depth == -1)
return all_entries_interesting;
return within_depth(base_str + matchlen + 1,
@@ -634,7 +634,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
* Match all directories. We'll try to
* match files later on.
*/
- if (ps->recursive && S_ISDIR(entry->mode))
+ if (!ps->nonrecursive_diff_tree && S_ISDIR(entry->mode))
return entry_interesting;
}
@@ -662,7 +662,7 @@ match_wildcards:
* Match all directories. We'll try to match files
* later on.
*/
- if (ps->recursive && S_ISDIR(entry->mode))
+ if (!ps->nonrecursive_diff_tree && S_ISDIR(entry->mode))
return entry_interesting;
}
return never_interesting; /* No matches */
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* Re: [PATCH] tree_entry_interesting: make recursive mode default
From: Junio C Hamano @ 2012-01-12 5:04 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Jonathan Nieder, Linus Torvalds
In-Reply-To: <1326341371-16628-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> There is a bit of history behind this. Some time ago, t_e_i() only
> supported prefix matching. diff-tree supported recursive and
> non-recursive mode but it did not make any difference to prefix
> matching.
>
> Later on, t_e_i() gained limited recursion support to unify a similar
> matching code used by git-grep. It introduced two new fields in struct
> pathspec: max_depth and recursive. "recursive" field functions as a
> feature switch so that this feature is off by default.
>
> Some time after that, t_e_i() further gained wildcard support. With
> wildcard matching, recursive and non-recursive diff-tree
> mattered. "recursive" field was reused to distinguish recursion in
> diff-tree.
>
> This choice has a side effect that by default wildcard matching is in
> non-recursive mode, which is not true. All current call sites except
> "diff-tree without -r" (grep, traverse_commit_list, tree-walk and
> general tree diff) prefer recursive mode.
>
> This patch decouples the use of recursive field. The max_depth feature
> switch is now controlled by max_depth_valid field. diff-tree recursion
> is controlled by nonrecursive_diff_tree, which makes it recursive by
> default.
Thanks, but I am curious about two (and a half) things.
- The "max_depth" option has perfectly good and natural "invalid"
sentinel values (i.e. 0 or negative). Why do we need a separate
bitfield?
- Special casing the non-recursive mode of diff-tree is perfectly
acceptable, but nonrecursive_diff_tree does not sound like a very good
name for it for two reasons. Perhaps there may be other users that want
the "surface only" behaviour, so having "diff_tree" in the name limits
its future (re)use. Also an option that is named negatively inevitably
invites "if (!opt->non_whatever)" double negative. Can we come up with
a better name, perhaps "onelevel_only" or something?
- Shouldn't "onelevel_only" be the same as limiting to a single depth
with "max_depth"?
^ permalink raw reply
* Re: [PATCH] tree_entry_interesting: make recursive mode default
From: Nguyen Thai Ngoc Duy @ 2012-01-12 5:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder, Linus Torvalds
In-Reply-To: <7v1ur54ikk.fsf@alter.siamese.dyndns.org>
2012/1/12 Junio C Hamano <gitster@pobox.com>:
> Thanks, but I am curious about two (and a half) things.
>
> - The "max_depth" option has perfectly good and natural "invalid"
> sentinel values (i.e. 0 or negative). Why do we need a separate
> bitfield?
We want infinite recursion by default, by max_depth default value is
0, which is non-recursive.
> - Special casing the non-recursive mode of diff-tree is perfectly
> acceptable, but nonrecursive_diff_tree does not sound like a very good
> name for it for two reasons. Perhaps there may be other users that want
> the "surface only" behaviour, so having "diff_tree" in the name limits
> its future (re)use. Also an option that is named negatively inevitably
> invites "if (!opt->non_whatever)" double negative. Can we come up with
> a better name, perhaps "onelevel_only" or something?
I'm bad at naming, any other names are welcome. onelevel_only sounds good.
> - Shouldn't "onelevel_only" be the same as limiting to a single depth
> with "max_depth"?
I thought of that but not sure it's equivalent to max_depth == 1 or
max_depth == 0, so I separated it for safety and clarity. max_depth
feature is driven by git-grep and there were a few interpretations how
it should behave last time, so I'm not sure if its behavior may change
in future.
--
Duy
^ permalink raw reply
* What's cooking in git.git (Jan 2012, #03; Wed, 11)
From: Junio C Hamano @ 2012-01-12 7:02 UTC (permalink / raw)
To: git
Here are the topics that have been cooking. Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in
'next'.
I plan to tag 1.7.9-rc1 sometime on Friday this week (see
http://tinyurl.com/gitcal for a calendar).
Here are the repositories that have my integration branches:
With maint, master, next, pu, todo:
git://git.kernel.org/pub/scm/git/git.git
git://repo.or.cz/alt-git.git
https://code.google.com/p/git-core/
https://github.com/git/git
With only maint and master:
git://git.sourceforge.jp/gitroot/git-core/git.git
git://git-core.git.sourceforge.net/gitroot/git-core/git-core
With all the topics and integration branches:
https://github.com/gitster/git
The preformatted documentation in HTML and man format are found in:
git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
git://repo.or.cz/git-{htmldocs,manpages}.git/
https://code.google.com/p/git-{htmldocs,manpages}.git/
https://github.com/gitster/git-{htmldocs,manpages}.git/
--------------------------------------------------
[New Topics]
* jc/pull-signed-tag (2012-01-11) 1 commit
- merge: use editor by default in interactive sessions
Per Linus's strong suggestion, sugarcoated (aka "taking blame for the
original UI screw-ups") so that it is easier for me to swallow and accept
a potentially huge backward incompatibility issue, "git merge" is made to
launch an editor to explain the merge in the merge commit by default in
interactive sessions.
* jc/request-pull-show-head-4 (2012-01-10) 1 commit
(merged to 'next' on 2012-01-11 at 8d98a6b)
+ request-pull: use the real fork point when preparing the message
Hopefully the final finishing touch to the request-pull script that was
updated during this cycle.
Will merge by 1.7.9-rc1.
* jk/maint-upload-archive (2012-01-11) 1 commit
(merged to 'next' on 2012-01-11 at 5c0bfa9)
+ archive: re-allow HEAD:Documentation on a remote invocation
Running "git archive" remotely and asking for a partial tree of a ref,
e.g. HEAD:$path was forbidden by a recent change to tighten security, but
was found to be overly restrictive.
Will merge by 1.7.9-rc1.
* nd/commit-ignore-i-t-a (2012-01-11) 2 commits
- commit: add --skip-intent-to-add to allow commit with i-t-a entries in index
- cache-tree: update API to take abitrary flags
* nd/maint-refname-in-hierarchy-check (2012-01-11) 1 commit
- Fix incorrect ref namespace check
Avoid getting confused by "ref/headxxx" and mistaking it as if it is under
the "refs/heads/" hierarchy.
Not urgent.
* rr/sequencer (2012-01-11) 2 commits
- sequencer: factor code out of revert builtin
- revert: prepare to move replay_action to header
Moving large chunk of code out of cherry-pick/revert for later reuse,
primarily to prepare for the next cycle.
Not urgent.
* ss/maint-msys-cvsexportcommit (2012-01-11) 2 commits
(merged to 'next' on 2012-01-11 at 007aab1)
+ git-cvsexportcommit: Fix calling Perl's rel2abs() on MSYS
+ t9200: On MSYS, do not pass Windows-style paths to CVS
Will merge by 1.7.9-rc1.
* tr/maint-mailinfo (2012-01-11) 1 commit
(merged to 'next' on 2012-01-11 at d9e344a)
+ mailinfo documentation: accurately describe non -k case
Will merge by 1.7.9-rc1.
* jk/credentials (2012-01-11) 3 commits
- unix-socket: do not let close() or chdir() clobber errno during cleanup
- credential-cache: report more daemon connection errors
- unix-socket: handle long socket pathnames
* pw/p4-view-updates (2012-01-11) 5 commits
- git-p4: add tests demonstrating spec overlay ambiguities
- git-p4: adjust test to adhere to stricter useClientSpec
- git-p4: clarify comment
- git-p4: fix verbose comment typo
- git-p4: only a single ... wildcard is supported
--------------------------------------------------
[Graduated to "master"]
* bw/maint-t8006-sed-incomplete-line (2012-01-03) 1 commit
+ Work around sed portability issue in t8006-blame-textconv
--------------------------------------------------
[Stalled]
* jc/advise-push-default (2011-12-18) 1 commit
- push: hint to use push.default=upstream when appropriate
Peff had a good suggestion outlining an updated code structure so that
somebody new can try to dip his or her toes in the development. Any
takers?
Waiting for a reroll.
* jc/split-blob (2011-12-01) 6 commits
. WIP (streaming chunked)
- chunked-object: fallback checkout codepaths
- bulk-checkin: support chunked-object encoding
- bulk-checkin: allow the same data to be multiply hashed
- new representation types in the packstream
- varint-in-pack: refactor varint encoding/decoding
Not ready.
At least pack-objects and fsck need to learn the new encoding for the
series to be usable locally, and then index-pack/unpack-objects needs to
learn it to be used remotely.
* jc/advise-i18n (2011-12-22) 1 commit
- i18n of multi-line advice messages
Allow localization of advice messages that tend to be longer and
multi-line formatted. For now this is deliberately limited to advise()
interface and not vreportf() in general as touching the latter has
interactions with error() that has plumbing callers whose prefix "error: "
should never be translated.
--------------------------------------------------
[Cooking]
* rs/diff-postimage-in-context (2012-01-06) 1 commit
(merged to 'next' on 2012-01-09 at 9635032)
+ xdiff: print post-image for common records instead of pre-image
Looked reasonable.
Not urgent.
* cb/push-quiet (2012-01-08) 3 commits
- t5541: avoid TAP test miscounting
- fix push --quiet: add 'quiet' capability to receive-pack
- server_supports(): parse feature list more carefully
Looked reasonable.
Not urgent.
* nd/clone-detached (2012-01-11) 11 commits
- clone: print advice on checking out detached HEAD
- clone: allow --branch to take a tag
- clone: refuse to clone if --branch points to bogus ref
- clone: --branch=<branch> always means refs/heads/<branch>
- fixup! acd8f20
- clone: delay cloning until after remote HEAD checking
- clone: factor out remote ref writing
- clone: factor out HEAD update code
- clone: factor out checkout code
- clone: write detached HEAD in bare repositories
- t5601: add missing && cascade
Looking good, but I may have screwed up the merge of this into 'pu'.
Not urgent.
* nd/clone-single-branch (2012-01-08) 1 commit
(merged to 'next' on 2012-01-09 at 6c3c759)
+ clone: add --single-branch to fetch only one branch
Looked reasonable.
Not urgent.
* jn/gitweb-unspecified-action (2012-01-09) 1 commit
- gitweb: Fix actionless dispatch for non-existent objects
Looked reasonable.
Not urgent.
* nd/index-pack-no-recurse (2012-01-09) 3 commits
- index-pack: eliminate unlimited recursion in get_delta_base()
- index-pack: eliminate recursion in find_unresolved_deltas
- Eliminate recursion in setting/clearing marks in commit list
The first one looked sensible; I am not sure if the second and third ones
take the right approach.
Will defer till the next cycle.
* mh/ref-api-rest (2011-12-12) 35 commits
- repack_without_ref(): call clear_packed_ref_cache()
- read_packed_refs(): keep track of the directory being worked in
- is_refname_available(): query only possibly-conflicting references
- refs: read loose references lazily
- read_loose_refs(): take a (ref_entry *) as argument
- struct ref_dir: store a reference to the enclosing ref_cache
- sort_ref_dir(): take (ref_entry *) instead of (ref_dir *)
- do_for_each_ref_in_dir*(): take (ref_entry *) instead of (ref_dir *)
- add_entry(): take (ref_entry *) instead of (ref_dir *)
- search_ref_dir(): take (ref_entry *) instead of (ref_dir *)
- find_containing_direntry(): use (ref_entry *) instead of (ref_dir *)
- add_ref(): take (ref_entry *) instead of (ref_dir *)
- read_packed_refs(): take (ref_entry *) instead of (ref_dir *)
- find_ref(): take (ref_entry *) instead of (ref_dir *)
- is_refname_available(): take (ref_entry *) instead of (ref_dir *)
- get_loose_refs(): return (ref_entry *) instead of (ref_dir *)
- get_packed_refs(): return (ref_entry *) instead of (ref_dir *)
- refs: wrap top-level ref_dirs in ref_entries
- get_ref_dir(): keep track of the current ref_dir
- do_for_each_ref(): only iterate over the subtree that was requested
- refs: sort ref_dirs lazily
- sort_ref_dir(): do not sort if already sorted
- refs: store references hierarchically
- refs.c: rename ref_array -> ref_dir
- struct ref_entry: nest the value part in a union
- check_refname_component(): return 0 for zero-length components
- free_ref_entry(): new function
- refs.c: reorder definitions more logically
- is_refname_available(): reimplement using do_for_each_ref_in_array()
- names_conflict(): simplify implementation
- names_conflict(): new function, extracted from is_refname_available()
- repack_without_ref(): reimplement using do_for_each_ref_in_array()
- do_for_each_ref_in_arrays(): new function
- do_for_each_ref_in_array(): new function
- do_for_each_ref(): correctly terminate while processesing extra_refs
The API for extra anchoring points may require rethought first; that would
hopefully make the "ref" part a lot simpler. And that is happening in
another topic (which has graduated to 'master').
Will defer till the next cycle.
* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits
- fixup! 15eaaf4
- git-svn, perl/Git.pm: extend Git::prompt helper for querying users
(merged to 'next' on 2012-01-05 at 954f125)
+ perl/Git.pm: "prompt" helper to honor GIT_ASKPASS and SSH_ASKPASS
The bottom one has been replaced with a rewrite based on comments from
Ævar. The second one needs more work, both in perl/Git.pm and prompt.c, to
give precedence to tty over SSH_ASKPASS when terminal is available.
Will defer till the next cycle.
* cb/git-daemon-tests (2012-01-08) 5 commits
(merged to 'next' on 2012-01-08 at 1db8351)
+ git-daemon tests: wait until daemon is ready
+ git-daemon: produce output when ready
+ git-daemon: add tests
+ dashed externals: kill children on exit
+ run-command: optionally kill children on exit
Will defer till the next cycle.
* jk/parse-object-cached (2012-01-06) 3 commits
(merged to 'next' on 2012-01-08 at 8c6fa4a)
+ upload-pack: avoid parsing tag destinations
+ upload-pack: avoid parsing objects during ref advertisement
+ parse_object: try internal cache before reading object db
These are a bit scary changes, but I do think they are worth doing.
Will defer till the next cycle.
* jn/maint-gitweb-grep-fix (2012-01-05) 2 commits
- gitweb: Harden "grep" search against filenames with ':'
- gitweb: Fix file links in "grep" search
Waiting for a confirmation from bug reporter.
^ permalink raw reply
* [PATCH] stash show: use default pretty format
From: Tay Ray Chuan @ 2012-01-12 7:05 UTC (permalink / raw)
To: Git Mailing List; +Cc: Junio C Hamano
By default (ie. when stash show is invoked without any arguments), the
diff stat of the stashed changes is displayed. Let git-diff decide the
default pretty format to use.
This gives git more consistency, as users who have set their
pretty.format config would naturally expect `git-stash show` to display
the diff in the same pretty format as the other diff-producing procelain
like git-log and git-show.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
git-stash.sh | 2 +-
t/t3903-stash.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index fe4ab28..a0db3de 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -271,7 +271,7 @@ list_stash () {
show_stash () {
assert_stash_like "$@"
- git diff ${FLAGS:---stat} $b_commit $w_commit
+ git diff ${FLAGS} $b_commit $w_commit
}
#
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index dbe2ac1..7a18af8 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -446,7 +446,7 @@ test_expect_success 'stash show - stashes on stack, stash-like argument' '
file | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
EOF
- git stash show ${STASH_ID} >actual &&
+ git stash show --stat ${STASH_ID} >actual &&
test_cmp expected actual
'
@@ -484,7 +484,7 @@ test_expect_success 'stash show - no stashes on stack, stash-like argument' '
file | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
EOF
- git stash show ${STASH_ID} >actual &&
+ git stash show --stat ${STASH_ID} >actual &&
test_cmp expected actual
'
--
1.7.9.rc0.132.ge406
^ permalink raw reply related
* Re: [PATCH 2/3] am: learn passing -b to mailinfo
From: Thomas Rast @ 2012-01-12 8:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzkdt4s9l.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> @@ -571,8 +574,8 @@ then
>> else
>> utf8=-n
>> fi
>> -if test "$(cat "$dotest/keep")" = t
>> -then
>> +keep=$(cat "$dotest/keep")
>> +if test "$keep" = t
>> keep=-k
>> fi
>
> Curious.
>
> Who writes 't' to $dotest/keep after this patch is applied?
Nobody; like the commit message says, I was just trying to help users
upgrading from one version to the next in the middle of an 'am', which
almost worked except for
> I suspect that this patch was not tested in a way to exercise this
> codepath; shell would have barfed when seeing the lack of "then" here, no?
(ouch)
> I also do not want to worry about "echo" portability issues that may come
> from an existing
>
> echo "$keep" >"$dotest/keep"
>
> that this patch does not touch.
Good point, thanks. I'll reroll with printf. Should I keep the
upgrade path compatibility?
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* [PATCH] diff --no-index: support more than one file pair
From: Nguyễn Thái Ngọc Duy @ 2012-01-12 9:09 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy
This allows you to do
git diff --no-index file1.old file1.new file2.old file2.new...
It could be seen as an abuse of "git --no-index", but it's very
tempting considering many bells and whistles git's diff machinery
provides.
Signed-off-by: A Clearcase user who has had enough with "ct diff"
---
Sorry I used git@vger as a personal archive, but this might benefit
others as well, I think.
diff-no-index.c | 38 +++++++++++++++++++++-----------------
1 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 3a36144..b4f6d06 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -199,12 +199,8 @@ void diff_no_index(struct rev_info *revs,
!path_outside_repo(argv[i+1])))
return;
}
- if (argc != i + 2)
- usagef("git diff %s <path> <path>",
- no_index ? "--no-index" : "[--no-index]");
-
diff_setup(&revs->diffopt);
- for (i = 1; i < argc - 2; ) {
+ for (i = 1; i < argc; ) {
int j;
if (!strcmp(argv[i], "--no-index"))
i++;
@@ -214,13 +210,19 @@ void diff_no_index(struct rev_info *revs,
}
else if (!strcmp(argv[i], "--"))
i++;
- else {
+ else if (argv[i][0] == '-') {
j = diff_opt_parse(&revs->diffopt, argv + i, argc - i);
if (!j)
die("invalid diff option/value: %s", argv[i]);
i += j;
}
+ else
+ break;
}
+ if ((argc - i) % 2)
+ usagef("git diff %s <path> <path>%s",
+ no_index ? "--no-index" : "[--no-index]",
+ no_index ? "[ <path> <path>...]" : "");
/*
* If the user asked for our exit code then don't start a
@@ -229,13 +231,15 @@ void diff_no_index(struct rev_info *revs,
if (!DIFF_OPT_TST(&revs->diffopt, EXIT_WITH_STATUS))
setup_pager();
+ /* argv now only contains paths */
+ argv += i;
+ argc -= i;
+
if (prefix) {
int len = strlen(prefix);
- const char *paths[3];
- memset(paths, 0, sizeof(paths));
- for (i = 0; i < 2; i++) {
- const char *p = argv[argc - 2 + i];
+ for (i = 0; i < argc; i++) {
+ const char *p = argv[i];
/*
* stdin should be spelled as '-'; if you have
* path that is '-', spell it as ./-.
@@ -243,12 +247,10 @@ void diff_no_index(struct rev_info *revs,
p = (strcmp(p, "-")
? xstrdup(prefix_filename(prefix, len, p))
: p);
- paths[i] = p;
+ argv[i] = p;
}
- diff_tree_setup_paths(paths, &revs->diffopt);
}
- else
- diff_tree_setup_paths(argv + argc - 2, &revs->diffopt);
+
revs->diffopt.skip_stat_unmatch = 1;
if (!revs->diffopt.output_format)
revs->diffopt.output_format = DIFF_FORMAT_PATCH;
@@ -260,9 +262,11 @@ void diff_no_index(struct rev_info *revs,
if (diff_setup_done(&revs->diffopt) < 0)
die("diff_setup_done failed");
- if (queue_diff(&revs->diffopt, revs->diffopt.pathspec.raw[0],
- revs->diffopt.pathspec.raw[1]))
- exit(1);
+ while (argv[0] && argv[1]) {
+ if (queue_diff(&revs->diffopt, argv[0], argv[1]))
+ exit(1);
+ argv += 2;
+ }
diff_set_mnemonic_prefix(&revs->diffopt, "1/", "2/");
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* git diff --word-diff problem
From: Ivan Shirokoff @ 2012-01-12 9:05 UTC (permalink / raw)
To: git
Hello.
I've got a couple of generated files with obfuscated code.
I want to word-diff them just to make sure that everything is right.
The thing is when word-diff gets oneline file without newline at the end
it compares it with regular line by line diff.
Here is an example. Two one line files generated with perl -e "print 'a
'x10" > file
git diff --word-diff=plain file1 file2
diff --git a/file1 b/file2
index 3526254..0515a63 100644
--- a/file1
+++ b/file2
@@ -1 +1 @@
[- a a a a a a a a a a-]
No newline at end of file
{+a a a a a ab a a a a+}
Git shows that the whole line is different.
And if I add newlines to that files everything works just as expected
git diff --word-diff=plain file1 file2
diff --git a/file1 b/file2
index 1756d83..1ec45b9 100644
--- a/file1
+++ b/file2
@@ -1,2 +1,2 @@
a a a a a [-a -]{+ab +}a a a a
Is that a bug or I've missed explanation in docs?
--
Ivan Shirokoff
^ permalink raw reply related
* Re: [PATCH] diff --no-index: support more than one file pair
From: Matthieu Moy @ 2012-01-12 9:14 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1326359371-13528-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This allows you to do
>
> git diff --no-index file1.old file1.new file2.old file2.new...
>
> It could be seen as an abuse of "git --no-index", but it's very
> tempting considering many bells and whistles git's diff machinery
> provides.
I find this very, very unintuitive. I tried with GNU diff:
diff 1 2 3 4
and it complained with "diff: extra operand `3'". I find
git diff --no-index file1.old file1.new
git diff --no-index file2.old file2.new
far more intuitive, less error prone (when you start having a
non-trivial list of arguments, it's hard to tell which is the new and
which is the old visually), ... So I'm curious why you prefer your
syntax.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] diff --no-index: support more than one file pair
From: Nguyen Thai Ngoc Duy @ 2012-01-12 9:17 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpq39bll1ua.fsf@bauges.imag.fr>
2012/1/12 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> This allows you to do
>>
>> git diff --no-index file1.old file1.new file2.old file2.new...
>>
>> It could be seen as an abuse of "git --no-index", but it's very
>> tempting considering many bells and whistles git's diff machinery
>> provides.
>
> I find this very, very unintuitive. I tried with GNU diff:
>
> diff 1 2 3 4
>
> and it complained with "diff: extra operand `3'". I find
>
> git diff --no-index file1.old file1.new
> git diff --no-index file2.old file2.new
>
> far more intuitive, less error prone (when you start having a
> non-trivial list of arguments, it's hard to tell which is the new and
> which is the old visually), ... So I'm curious why you prefer your
> syntax.
Single operation has its advantages:
- one pager
- one stat and summary
- might be easier to script (just throw them all to xargs)
- hell, i might even benefit from git copy/modify detection
--
Duy
^ permalink raw reply
* Re: [PATCH 2/2] diff --word-diff: use non-whitespace regex by default
From: Thomas Rast @ 2012-01-12 9:22 UTC (permalink / raw)
To: Tay Ray Chuan; +Cc: Git Mailing List, Junio C Hamano
In-Reply-To: <CALUzUxo3DcKqC6sQFQ1Oi0vgASFSHCcmOgHAj2_4c3vEjy663w@mail.gmail.com>
Tay Ray Chuan <rctay89@gmail.com> writes:
> On Thu, Jan 12, 2012 at 4:05 AM, Thomas Rast <trast@student.ethz.ch> wrote:
>> Tay Ray Chuan <rctay89@gmail.com> writes:
>>
>>> Factor out the comprehensive non-whitespace regex in use by PATTERNS and
>>> IPATTERN and use it as the word-diff regex for the default diff driver.
>>
>> Why?
Sorry for distracting you with the performance argument; it was mostly
the first thing that came to my mind that I could use to ask for the
motivation, and evaluation of tradeoffs, that both were missing from the
proposed commit message.
> But I think it's worthwhile to trade-off performance for a sensible
> default. Something like
>
> matrix[a,b,c]
> matrix[d,b,c]
>
> gives
>
> matrix[[-a-]{+d+},b,c]
>
> and when we have
>
> ImagineALanguageLikeFoo
> ImagineALanguageLikeBar
>
> we get
>
> ImagineALanguageLike[-Foo-]{+Bar+}
In that case (and I should have read the original patch), I am
definitely against this change. It turns the default word-diff into
character-diff, which is something entirely different, and frequently
useless precisely for the reason you state:
> (But I cheated. Foo and Bar have no common characters in common; if
> they did, the word diff would be messy.)
Case in point, consider my patch sent out yesterday
http://article.gmane.org/gmane.comp.version-control.git/188391
It consists of a one-hunk doc update. word-diff is not brilliant:
-k::
Usually the program [-'cleans up'-]{+removes email cruft from+} the Subject:
header line to extract the title line for the commit log
[-message,-]
[- among which (1) remove 'Re:' or 're:', (2) leading-]
[- whitespaces, (3) '[' up to ']', typically '[PATCH]', and-]
[- then prepends "[PATCH] ".-]{+message.+} This [-flag forbids-]{+option prevents+} this munging, and is most
useful when used to read back 'git format-patch -k' output.
[snip the rest as it's only {+}]
But character-diff tries too hard to find common subsequences:
$ g show HEAD^^ --word-diff-regex='[^[:space:]]' | xsel
-k::
Usually the program [-'cl-]{+remov+}e[-an-]s {+email cr+}u[-p'-]{+ft from+} the Subject:
header line to extract the title line for the commit log
message[-,-]
[- among which (1) remove 'Re:' or 're:', (2) leading-]
[- w-]{+. T+}hi[-te-]s[-paces, (3) '[' up t-] o[-']', ty-]p[-ically '[PATCH]', and-]t[-he-]{+io+}n pre[-p-]{+v+}en[-ds "[PATCH] ". This flag forbid-]{+t+}s this munging, and is most
useful when used to read back 'git format-patch -k' output.
[snip]
Wouldn't you agree that
w-]{+. T+}hi[-te-]s[-paces, (3) '[' up t-] o[-']', ty-]p[
is just line noise? The colors don't even help as most of it is removed
(red).
Regarding your examples
> [1] http://article.gmane.org/gmane.comp.version-control.git/105896
> [2] http://article.gmane.org/gmane.comp.version-control.git/105237
first please notice that both of them were written before (and actually
discussing) the introduction of the wordRegex feature. At this point,
we were trying to make up our minds w.r.t. how powerful the feature
needs to be. Nowadays (or in fact, starting a few days after those
emails) the user can easily achieve everything discussed here by setting
the wordRegex to taste.
That being said, I can see some arguments for changing the default to
split punctuation into a separate word. That is, whereas the current
default is semantically equivalent to a wordRegex of
[^[:space:]]*
(but has a faster code path) and your proposal is equivalent to
[^[:space:]]|UTF_8_GUARD
I think there is a case to be made for a default of
[^[:space:]]|([[:alnum:]]|UTF_8_GUARD)+
or some such. There's a lot of bikeshedding lurking in the (non)extent
of the [[:alnum:]] here, however.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] diff --no-index: support more than one file pair
From: Matthieu Moy @ 2012-01-12 9:30 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8BvA_o1+xrOx4hYhmwNWpsRnh5+mftb471h3yFW2b6vhA@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> Single operation has its advantages:
>
> - one pager
> - one stat and summary
I buy these, and I understand why they apply to "git diff" and not GNU
diff.
> - might be easier to script (just throw them all to xargs)
I don't see a use-case where a command produces old1 new1 old2 new2, but
if there is one, then "| xargs -n 2 diff" is the solution. You don't
need your patch.
> - hell, i might even benefit from git copy/modify detection
I don't see how, if you specify explicitely the pairs (old, new). You
may have such benefit if you let the command-line express "here's a
bunch of old files, and a bunch of new ones", but not with your proposed
syntax.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox