* Re: [PATCH] merge-tree: sometimes, d/f conflict is not an issue
From: Johannes Schindelin @ 2007-07-08 2:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Gerrit Pape, git, Rémi Vanicat
In-Reply-To: <7vabu765r0.fsf@assigned-by-dhcp.cox.net>
Hi,
On Sat, 7 Jul 2007, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > @@ -643,6 +643,20 @@ int threeway_merge(struct cache_entry **stages,
> > index = stages[0];
> > head = stages[o->head_idx];
> >
> > + /*
> > + * Special case: do not care about a dir/file conflict, when
> > + * the entries have not been touched.
> > + * IOW if the ancestors are identical to the remote, and the
> > + * index is the same as head, just take head.
> > + */
>
> Suppose paths "A" and "A/B" are involved, and you resolved with
> this logic to have "A" as a blob (so your HEAD does not have
> "A/B"). If the remote adds "A/B", what prevents the resulting
> index to have both "A" and "A/B" resolved at stage #0?
Hmm.
> A logic to do "if it is unchanged on one and changed in another,
> take changed one" already exists in later part of the code; your
> patch just circumvents D/F checks built into threeway_merge for
> this one case, and only because this one case happens to have
> reported. It doesn't feel right.
Well, for me the code in threeway_merge does not feel right. There is a
table in technical/trivial-merge.txt (which I not fully understand, since
nowhere in the table, there is a mention of the index, and nowhere is
explained what "ALT" is supposed to mean), but threeway_merge only
references the numbers. The code is not obvious to me.
I spent some hours staring on, and trying to make sense of it. Alas, I am
an idiot or something, since my brain feels like a mashed potato, and I
still do not understand the code. For example, it is not apparent to me
why the variable "head" should be set to NULL, when it was the
df_conflict_entry.
So yeah, this patch was marked as "PATCH", but the subject line is not
long enough for the proper prefix, which would have started like
"[This PATCH works for me, and I do not know how to make it better, since
I do not understand the code in threeway_merge(), and that does not make
me happy, but that is the way things are, and maybe someone more
intelligent than me recognizes what is meant by my little patch, and can
fix it up, ...]".
> IOW, don't make unpack-trees to make policy decisions on final
> resolution, unless it is operating under aggressive rule (where the
> caller explicitly allows it to make more than the "trivial" decisions).
> The caller (in this case, merge-recursive) should see A at stage #2 with
> A/B at stages #1 and #3 and decide what to do.
Okay, so you're saying that merge-recursive should use the aggressive
strategy?
Ciao,
Dscho
^ permalink raw reply
* [PATCH] gitweb: New cgi parameter: filter
From: Miklos Vajna @ 2007-07-08 1:35 UTC (permalink / raw)
To: git; +Cc: gitster
Currently the only supported value is "nomerges". This allows one to filter
merges from many actions, like rss, log and shortlog.
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
gitweb/gitweb.perl | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dc609f4..ce592cf 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -383,6 +383,13 @@ if (defined $hash_base) {
}
}
+our $filter = $cgi->param('filter');
+if (defined $filter) {
+ if ($filter != "nomerges") {
+ die_error(undef, "Invalid filter parameter");
+ }
+}
+
our $hash_parent_base = $cgi->param('hpb');
if (defined $hash_parent_base) {
if (!validate_refname($hash_parent_base)) {
@@ -534,6 +541,7 @@ sub href(%) {
action => "a",
file_name => "f",
file_parent => "fp",
+ filter => "filter",
hash => "h",
hash_parent => "hp",
hash_base => "hb",
@@ -1770,6 +1778,7 @@ sub parse_commits {
($arg ? ($arg) : ()),
("--max-count=" . $maxcount),
("--skip=" . $skip),
+ ((defined $filter and $filter == "nomerges") ? ("--no-merges") : ()),
$commit_id,
"--",
($filename ? ($filename) : ())
--
1.5.3.rc0.39.g46f7-dirty
^ permalink raw reply related
* Re: [PATCH] merge-tree: sometimes, d/f conflict is not an issue
From: Junio C Hamano @ 2007-07-08 1:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Gerrit Pape, git, Rémi Vanicat, gitster
In-Reply-To: <Pine.LNX.4.64.0707080148370.4093@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> @@ -643,6 +643,20 @@ int threeway_merge(struct cache_entry **stages,
> index = stages[0];
> head = stages[o->head_idx];
>
> + /*
> + * Special case: do not care about a dir/file conflict, when
> + * the entries have not been touched.
> + * IOW if the ancestors are identical to the remote, and the
> + * index is the same as head, just take head.
> + */
Suppose paths "A" and "A/B" are involved, and you resolved with
this logic to have "A" as a blob (so your HEAD does not have
"A/B"). If the remote adds "A/B", what prevents the resulting
index to have both "A" and "A/B" resolved at stage #0?
A logic to do "if it is unchanged on one and changed in another,
take changed one" already exists in later part of the code; your
patch just circumvents D/F checks built into threeway_merge for
this one case, and only because this one case happens to have
reported. It doesn't feel right.
IOW, don't make unpack-trees to make policy decisions on final
resolution, unless it is operating under aggressive rule (where
the caller explicitly allows it to make more than the "trivial"
decisions). The caller (in this case, merge-recursive) should
see A at stage #2 with A/B at stages #1 and #3 and decide what
to do.
^ permalink raw reply
* Re: [PATCH] Make attributes "-diff" and "diff" work as advertized
From: Johannes Schindelin @ 2007-07-08 0:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vy7hr69ky.fsf@assigned-by-dhcp.cox.net>
Hi,
well, it was a quick fix. I had more on my wishlist for today, and wanted
to be done with it quickly. The proper fix would be something like this, I
guess (which should not be applied without the changes to t4020, of
course):
diff.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index 04e7e91..e47921f 100644
--- a/diff.c
+++ b/diff.c
@@ -1113,13 +1113,13 @@ static void setup_diff_attr_check(struct git_attr_check *check)
static void diff_filespec_check_attr(struct diff_filespec *one)
{
+ int is_binary = -1;
struct git_attr_check attr_diff_check[2];
if (one->checked_attr)
return;
setup_diff_attr_check(attr_diff_check);
- one->is_binary = 0;
one->hunk_header_ident = NULL;
if (!git_checkattr(one->path, ARRAY_SIZE(attr_diff_check), attr_diff_check)) {
@@ -1128,9 +1128,9 @@ static void diff_filespec_check_attr(struct diff_filespec *one)
/* binaryness */
value = attr_diff_check[0].value;
if (ATTR_TRUE(value))
- ;
+ is_binary = 0;
else if (ATTR_FALSE(value))
- one->is_binary = 1;
+ is_binary = 1;
/* hunk header ident */
value = attr_diff_check[1].value;
@@ -1140,11 +1140,15 @@ static void diff_filespec_check_attr(struct diff_filespec *one)
one->hunk_header_ident = value;
}
- if (!one->data && DIFF_FILE_VALID(one))
- diff_populate_filespec(one, 0);
+ if (is_binary < 0) {
+ if (!one->data && DIFF_FILE_VALID(one))
+ diff_populate_filespec(one, 0);
- if (one->data)
- one->is_binary = buffer_is_binary(one->data, one->size);
+ if (one->data)
+ one->is_binary =
+ buffer_is_binary(one->data, one->size);
+ } else
+ one->is_binary = is_binary;
}
^ permalink raw reply related
* [PATCH] merge-tree: sometimes, d/f conflict is not an issue
From: Johannes Schindelin @ 2007-07-08 0:52 UTC (permalink / raw)
To: Gerrit Pape; +Cc: git, Rémi Vanicat, gitster
In-Reply-To: <20070625071819.8091.qmail@5e4088a43a10fd.315fe32.mid.smarden.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3427 bytes --]
When a merge has a d/f conflict on a path which was not touched
between the merge base(s) and the remote HEAD, and the index and
HEAD contain the same version for that path (even if empty), it
is not really a conflict.
Noticed by Rémi Vanicat, reported to the Git list by Gerrit Pape.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
The only peculiar result is that you can have an entry at
stage 0 for one path, and stages 1 and 3 for another path
where the first path is a prefix.
This can happen now, when you have deleted a directory
since the branching point, and put a file in the same place, but
the other side has changed files in that directory.
This change is reflected in the change to t3030.
I have no idea if the change is the best there is, but after
spending some hours trying to get my head around what is written
in Documentation/technical/trivial-merge.txt, and what is in
the function threeway_merge(), and still not understanding most of
it, there is not much more that I can do.
Funny note at the side: when I finally got to Gerrit's email again
today, gmane said _almost_ that it was 3 weeks, 3 days, 3 hours
and 3 minutes ago...
t/t3030-merge-recursive.sh | 2 +-
t/t3502-cherry-pick.sh | 31 +++++++++++++++++++++++++++++++
unpack-trees.c | 14 ++++++++++++++
3 files changed, 46 insertions(+), 1 deletions(-)
create mode 100755 t/t3502-cherry-pick.sh
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 607f57f..d413af1 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -450,7 +450,7 @@ test_expect_success 'merge-recursive d/f conflict result' '
echo "100644 $o1 0 a"
echo "100644 $o0 0 b"
echo "100644 $o0 0 c"
- echo "100644 $o6 2 d"
+ echo "100644 $o6 0 d"
echo "100644 $o0 1 d/e"
echo "100644 $o1 3 d/e"
) >expected &&
diff --git a/t/t3502-cherry-pick.sh b/t/t3502-cherry-pick.sh
new file mode 100755
index 0000000..da3d26e
--- /dev/null
+++ b/t/t3502-cherry-pick.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+test_description='test cherry-pick'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ echo foo > file &&
+ ln -s dangling link &&
+ git add file link &&
+ test_tick &&
+ git commit -m foo &&
+ git checkout -b branch &&
+ git rm link &&
+ test_tick &&
+ git commit -m "remove link" &&
+ mkdir link &&
+ echo bar > link/file &&
+ git add link/file &&
+ test_tick &&
+ git commit -m "add dir" &&
+ echo bar > file &&
+ git commit -m "change file" file &&
+ git checkout master
+'
+
+test_expect_success 'cherry-pick ignores unrelated dir/symlink conflict' '
+ git cherry-pick branch
+'
+
+test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index cac2411..080b016 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -643,6 +643,20 @@ int threeway_merge(struct cache_entry **stages,
index = stages[0];
head = stages[o->head_idx];
+ /*
+ * Special case: do not care about a dir/file conflict, when
+ * the entries have not been touched.
+ * IOW if the ancestors are identical to the remote, and the
+ * index is the same as head, just take head.
+ */
+ if (head && same(index, head)) {
+ for (i = 1; i < o->head_idx; i++)
+ if (!same(stages[i], remote))
+ break;
+ if (i == o->head_idx)
+ return merged_entry(head, index, o);
+ }
+
if (head == o->df_conflict_entry) {
df_conflict_head = 1;
head = NULL;
--
1.5.3.rc0.2712.g125b7f
^ permalink raw reply related
* git cvsimport branches not consistent with CVS branches
From: Gordon Heydon @ 2007-07-08 0:45 UTC (permalink / raw)
To: git
Hi,
After some investigation I found that git cvsimport was not importing
branches 100% correctly with CVS.
Git and CVS do branching very differently in that CVS this is done at
the file level (like everything else) and git does it repository wide.
So if I have a CVS repository with the files a, b and c and I branch b
with a `cvs tab -b BRANCH test` on the branch test I will just have the
file b.
If I do a git cvsimport on the test branch there will actually be the
files a, b and c
What I think needs to happen is that when git cvsimport created the
branch in the git repository it needs to delete all files from the
branch that were not branched.
Thanks in advance.
Gordon.
^ permalink raw reply
* Re: [PATCH] Make attributes "-diff" and "diff" work as advertized
From: Junio C Hamano @ 2007-07-08 0:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <Pine.LNX.4.64.0707071749220.4093@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> diff --git a/diff.c b/diff.c
> index 21e61af..e92db5c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1170,13 +1170,19 @@ static void diff_filespec_check_attr(struct diff_filespec *one)
> one->is_binary = 0;
> one->funcname_pattern_ident = NULL;
>
> + if (!one->data && DIFF_FILE_VALID(one))
> + diff_populate_filespec(one, 0);
> +
> + if (one->data)
> + one->is_binary = buffer_is_binary(one->data, one->size);
> +
> if (!git_checkattr(one->path, 1, &attr_diff_check)) {
> const char *value;
>
> /* binaryness */
> value = attr_diff_check.value;
> if (ATTR_TRUE(value))
> - ;
> + one->is_binary = 0;
I wanted to make sure we do not have to read data and run
buffer_is_binary() when attribute says we do not have to. I
wonder why moving the code around makes the difference.
> else if (ATTR_FALSE(value))
> one->is_binary = 1;
>
> @@ -1186,13 +1192,6 @@ static void diff_filespec_check_attr(struct diff_filespec *one)
> else
> one->funcname_pattern_ident = value;
> }
> -
> - if (!one->data && DIFF_FILE_VALID(one))
> - diff_populate_filespec(one, 0);
> -
> - if (one->data)
> - one->is_binary = buffer_is_binary(one->data, one->size);
> -
> }
Ah, I see, because these are done unconditionally. That was
silly of me.
^ permalink raw reply
* Re: Possible BUG in 'git log --parents'
From: Marco Costalba @ 2007-07-07 22:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <e5bfff550707070314q3b78ac65p9a8ff3130a7bfaef@mail.gmail.com>
On 7/7/07, Marco Costalba <mcostalba@gmail.com> wrote:
> On 7/7/07, Junio C Hamano <gitster@pobox.com> wrote:
>
>
> > git-log. However, I have a slight suspicion that we earlier
> > decided not to do so on purpose with git-log and for a good
> > reason. I do not have much energy left tonight to dig into this
> > right now, but mailing list log and commit log messages for
> > rev-list.c, builtin-log.c and builtin-rev-list.c might tell you
> > more.
> >
>
> I will take a look ...
>
The commit with the patch for git-rev-list is 884944239f2
I have found also the following commits, more or less related:
884944239f2 3381c790 2a0925be3
But I haven't found messages with downsides of using the same approach
also for git log.
So could you please apply the patch also for git-log if possible.
Thanks
Marco
^ permalink raw reply
* Re: unexpected git-cherry-pick conflict
From: Johannes Schindelin @ 2007-07-07 20:58 UTC (permalink / raw)
To: Gerrit Pape; +Cc: git
In-Reply-To: <20070625071819.8091.qmail@5e4088a43a10fd.315fe32.mid.smarden.org>
Hi,
On Mon, 25 Jun 2007, Gerrit Pape wrote:
> Hi, did you get to this yet?, not to stress you, just to make sure we
> don't forget about it.
Okay. Since now both you and Junio asked for it, and I made today a Git
day for me, I looked into this.
I'm not yet done, but preliminary results are:
- cherry-pick calls merge-recursive, which in turn calls unpack_trees(),
which in turn gives the ball to threeway_merge. But for d/f conflicts,
it goes wrong.
- Documentation/technical/trivial-merge.txt is maybe about a trivial
merge. But the document is not trivial in and of itself.
I'll keep going,
Dscho
^ permalink raw reply
* Re: [PATCH] gitweb: make search form generate pathinfo-style URLs
From: Junio C Hamano @ 2007-07-07 21:01 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git, jnareb, pasky, ltuikov
In-Reply-To: <1183057027.6108.4.camel@mattlaptop2>
Matt McCutchen <hashproduct@gmail.com> writes:
> The search form generated traditional-style URLs with a "p=" parameter
> even when the pathinfo feature was on. This patch makes it generate
> pathinfo-style URLs when appropriate.
>
> Signed-off-by: Matt McCutchen <hashproduct@gmail.com>
> ---
> gitweb/gitweb.perl | 12 +++++++++---
> 1 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index f36428e..7b0e110 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2215,12 +2215,18 @@ EOF
> } else {
> $search_hash = "HEAD";
> }
> + my $action = $my_uri;
> + my ($use_pathinfo) = gitweb_check_feature('pathinfo');
> + if ($use_pathinfo) {
> + $action .= "/$project";
> + } else {
> + $cgi->param("p", $project);
> + }
> $cgi->param("a", "search");
> $cgi->param("h", $search_hash);
> - $cgi->param("p", $project);
> - print $cgi->startform(-method => "get", -action => $my_uri) .
> + print $cgi->startform(-method => "get", -action => $action) .
> "<div class=\"search\">\n" .
> - $cgi->hidden(-name => "p") . "\n" .
> + (!$use_pathinfo && $cgi->hidden(-name => "p") . "\n") .
Ahhhhh. (!$use_pathinfo && blah) when $use_pathinfo is true
evaluates to an empty string, which saves you from protecting
yourself from undef. Perhaps clever, but is not terribly nice.
Other than that, the patch looks sane to me.
^ permalink raw reply
* Re: [PATCH] gitweb: snapshot cleanups & support for offering multiple formats
From: Junio C Hamano @ 2007-07-07 20:52 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git, jnareb, pasky, ltuikov
In-Reply-To: <1183053733.6108.0.camel@mattlaptop2>
Matt McCutchen <hashproduct@gmail.com> writes:
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index dbfb044..f36428e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -101,6 +101,15 @@ our $mimetypes_file = undef;
> # could be even 'utf-8' for the old behavior)
> our $fallback_encoding = 'latin1';
>
> +# information about snapshot formats that gitweb is capable of serving
> +# name => [mime type, filename suffix, --format for git-archive,
> +# compressor command suffix]
> +our %known_snapshot_formats = (
> + 'tgz' => ['application/x-gzip' , '.tar.gz' , 'tar', '| gzip' ],
> + 'tbz2' => ['application/x-bzip2', '.tar.bz2', 'tar', '| bzip2'],
> + 'zip' => ['application/zip' , '.zip' , 'zip', '' ],
> +);
> +
> # You define site-wide feature defaults here; override them with
> # $GITWEB_CONFIG as necessary.
> our %feature = (
> @@ -131,20 +140,22 @@ our %feature = (
> ...
> 'snapshot' => {
> 'sub' => \&feature_snapshot,
> 'override' => 0,
> - # => [content-encoding, suffix, program]
> - 'default' => ['x-gzip', 'gz', 'gzip']},
> + 'default' => ['tgz']},
>
> # Enable text search, which will list the commits which match author,
> # committer or commit text to a given string. Enabled by default.
This is a very nice clean-up, and I agree we should go this
route in the longer term.
This however will break people's existing gitweb configuration,
so if we were to do this it should be post 1.5.3, I would say.
^ permalink raw reply
* Re: [PATCH] remote: document the 'rm' subcommand
From: Junio C Hamano @ 2007-07-07 20:47 UTC (permalink / raw)
To: James Bowes; +Cc: Johannes Schindelin, git
In-Reply-To: <20070707152242.GC19782@crux.rdu.redhat.com>
Care to resend a three-patch series (implementation, this
documentation, and test suite updates)?
^ permalink raw reply
* Re: [PATCH] Per-path attribute based hunk header selection.
From: Junio C Hamano @ 2007-07-07 20:36 UTC (permalink / raw)
To: Johannes Schindelin
Cc: しらいしななこ,
Linus Torvalds, git
In-Reply-To: <Pine.LNX.4.64.0707071317060.4093@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Johannes, it strikes me that it is very odd having to add ".*$" at the
>> end and to surround the whole thing in a parentheses. Shouldn't the
>> ff_regexp() grabber simply pick the whole line? After all, that is what
>> GNU "diff -p -F RE" does.
>
> Yes, but then you can forget about your hierarchical idea.
Yeah, I've brought it up just for discussion, but I no longer
think the multi-line pattern that picks pieces and paste them
together is worth it.
^ permalink raw reply
* Re: [PATCH] path-list.c: always free strdup'ed paths
From: Johannes Schindelin @ 2007-07-07 20:27 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <468FF19E.8020406@lsrfire.ath.cx>
Hi,
On Sat, 7 Jul 2007, Ren? Scharfe wrote:
> Johannes Schindelin schrieb:
> >
> > On Sat, 7 Jul 2007, Ren? Scharfe wrote:
> >
> >> Always free .paths if .strdup_paths is set, no matter if the
> >> parameter free_items is set or not, plugging a minor memory leak.
> >> And to clarify the meaning of the flag, rename it to free_util,
> >> since it now only affects the freeing of the .util field.
> >
> > The rational was that it might very well be possible that the
> > pointers you hand to the path_list are already strdup()ed. So you do
> > not set strdup_paths, but you want them free()d.
>
> The patch doesn't take that away, i.e. .path fields are not freed if
> .strdup_paths is not set, both before and after the patch.
Yeah, the old code was buggy.
> And the workaround used in builtin-shortlog.c, viz. setting
> .strdup_paths just before calling path_list_clear(), still works.
Yeah, but it is ugly.
> > The .util field is in many cases something that is not trivially
> > free()d, but has to call a type-specific function, such as
> > path_list_clear() itself.
>
> Indeed; this is a good reason to separate freeing of .util and .path, no?
Yes.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] revision: allow selection of commits that do not match a pattern
From: Sven Verdoolaege @ 2007-07-07 20:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0707071957370.4093@racer.site>
On Sat, Jul 07, 2007 at 08:35:35PM +0100, Johannes Schindelin wrote:
> Why not keep it "add_grep", and do a
>
> struct grep_opt **filter = negated ?
> &revs->grep_neg_filter : &revs->grep_filter;
>
> Hm? You avoid an extra function that way.
[..]
>
> The parsing for "!" is again duplicated in add_message_grep(). Why not put
> it into add_grep(), and do
>
> negated = *pattern == '!';
> sprintf(pat, "%s^%s %s%s", negated ? "!" : "", field, prefix,
> pattern + negated);
>
> instead? No need to change the signature of add_grep(), and all callers
> get the '!' feature for free.
I can do these things, but they don't exactly improve readability, IMHO.
> > @@ -1249,6 +1277,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
> > compile_grep_patterns(revs->grep_filter);
> > }
> >
> > + if (revs->grep_neg_filter) {
> > + compile_grep_patterns(revs->grep_neg_filter);
> > + }
> > +
>
> Please lose the "{" and "}".
I may still need them for doing something with all_match...
> > @@ -1329,11 +1361,14 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
> >
> > static int commit_match(struct commit *commit, struct rev_info *opt)
> > {
> > - if (!opt->grep_filter)
> > - return 1;
> > - return grep_buffer(opt->grep_filter,
> > + return (!opt->grep_filter ||
> > + grep_buffer(opt->grep_filter,
> > + NULL, /* we say nothing, not even filename */
> > + commit->buffer, strlen(commit->buffer))) &&
> > + (!opt->grep_neg_filter ||
> > + !grep_buffer(opt->grep_neg_filter,
> > NULL, /* we say nothing, not even filename */
> > - commit->buffer, strlen(commit->buffer));
> > + commit->buffer, strlen(commit->buffer)));
> > }
>
> Urgh! That's not nice on my eyes.
You prefer
if (opt->grep_filter && !grep_buffer(opt->grep_filter,
NULL, /* we say nothing, not even filename */
commit->buffer, strlen(commit->buffer)))
return 0;
if (opt->grep_neg_filter && grep_buffer(opt->grep_neg_filter,
NULL, /* we say nothing, not even filename */
commit->buffer, strlen(commit->buffer)));
return 0;
return 1;
?
> Also, I suspect that the semantics are not yet clear, what should happen
> if all_match is unset.
So what are the semantics of all_match without negated matches?
It doesn't seem to be documented in git-rev-list.txt.
> BTW I suspect that a better way than having two filter lists is
> demonstrated in builtin-grep.c.
Could you be a bit more specific?
If you're talking about the GREP_NOT thing, then AFAICS that is line based
and I want these things to be commit based. That is I want to select
commits with either a or no lines that match a given pattern and not
commits that have a line that matches some patterns and not some others.
skimo
^ permalink raw reply
* Re: [PATCH] path-list.c: always free strdup'ed paths
From: René Scharfe @ 2007-07-07 20:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0707072036530.4093@racer.site>
Johannes Schindelin schrieb:
> Hi,
>
> On Sat, 7 Jul 2007, Ren? Scharfe wrote:
>
>> Always free .paths if .strdup_paths is set, no matter if the
>> parameter free_items is set or not, plugging a minor memory leak.
>> And to clarify the meaning of the flag, rename it to free_util,
>> since it now only affects the freeing of the .util field.
>
> The rational was that it might very well be possible that the
> pointers you hand to the path_list are already strdup()ed. So you do
> not set strdup_paths, but you want them free()d.
The patch doesn't take that away, i.e. .path fields are not freed if
.strdup_paths is not set, both before and after the patch. And the
workaround used in builtin-shortlog.c, viz. setting .strdup_paths just
before calling path_list_clear(), still works.
> The .util field is in many cases something that is not trivially
> free()d, but has to call a type-specific function, such as
> path_list_clear() itself.
Indeed; this is a good reason to separate freeing of .util and .path, no?
René
^ permalink raw reply
* Re: [PATCH] path-list.c: always free strdup'ed paths
From: Johannes Schindelin @ 2007-07-07 19:39 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <468FEC54.307@lsrfire.ath.cx>
Hi,
On Sat, 7 Jul 2007, Ren? Scharfe wrote:
> Always free .paths if .strdup_paths is set, no matter if the
> parameter free_items is set or not, plugging a minor memory leak.
> And to clarify the meaning of the flag, rename it to free_util,
> since it now only affects the freeing of the .util field.
The rational was that it might very well be possible that the pointers you
hand to the path_list are already strdup()ed. So you do not set
strdup_paths, but you want them free()d.
The .util field is in many cases something that is not trivially free()d,
but has to call a type-specific function, such as path_list_clear()
itself.
So I'm mildly negative about your patch.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] revision: allow selection of commits that do not match a pattern
From: Johannes Schindelin @ 2007-07-07 19:35 UTC (permalink / raw)
To: skimo; +Cc: Junio C Hamano, git
In-Reply-To: <20070707184224.GE1528MdfPADPa@greensroom.kotnet.org>
Hi,
On Sat, 7 Jul 2007, Sven Verdoolaege wrote:
> We do this by maintaining two lists of patterns, one for
> those that should match and one for those that should not match.
I would at least give one example in the commit message
> diff --git a/revision.c b/revision.c
> index 5184716..4b00ada 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -821,40 +821,65 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
> return 0;
> }
>
> -static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
> +static void add_grep_to_filter(struct grep_opt **filter, const char *ptn,
> + enum grep_pat_token what)
> {
> - if (!revs->grep_filter) {
> + if (!*filter) {
Why not keep it "add_grep", and do a
struct grep_opt **filter = negated ?
&revs->grep_neg_filter : &revs->grep_filter;
Hm? You avoid an extra function that way.
> +static void add_header_grep(struct rev_info *revs, const char *field,
> + const char *pattern)
> {
> char *pat;
> const char *prefix;
> int patlen, fldlen;
> + int negated = 0;
>
> fldlen = strlen(field);
> patlen = strlen(pattern);
> pat = xmalloc(patlen + fldlen + 10);
> prefix = ".*";
> + if (*pattern == '!') {
> + negated = 1;
> + pattern++;
> + }
> + if (pattern[0] == '\\' && pattern[1] == '!')
> + pattern++;
> if (*pattern == '^') {
> prefix = "";
> pattern++;
> }
> sprintf(pat, "^%s %s%s", field, prefix, pattern);
> - add_grep(revs, pat, GREP_PATTERN_HEAD);
> + add_grep(revs, pat, GREP_PATTERN_HEAD, negated);
> }
The parsing for "!" is again duplicated in add_message_grep(). Why not put
it into add_grep(), and do
negated = *pattern == '!';
sprintf(pat, "%s^%s %s%s", negated ? "!" : "", field, prefix,
pattern + negated);
instead? No need to change the signature of add_grep(), and all callers
get the '!' feature for free.
> @@ -1249,6 +1277,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
> compile_grep_patterns(revs->grep_filter);
> }
>
> + if (revs->grep_neg_filter) {
> + compile_grep_patterns(revs->grep_neg_filter);
> + }
> +
Please lose the "{" and "}".
> @@ -1329,11 +1361,14 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
>
> static int commit_match(struct commit *commit, struct rev_info *opt)
> {
> - if (!opt->grep_filter)
> - return 1;
> - return grep_buffer(opt->grep_filter,
> + return (!opt->grep_filter ||
> + grep_buffer(opt->grep_filter,
> + NULL, /* we say nothing, not even filename */
> + commit->buffer, strlen(commit->buffer))) &&
> + (!opt->grep_neg_filter ||
> + !grep_buffer(opt->grep_neg_filter,
> NULL, /* we say nothing, not even filename */
> - commit->buffer, strlen(commit->buffer));
> + commit->buffer, strlen(commit->buffer)));
> }
Urgh! That's not nice on my eyes.
Also, I suspect that the semantics are not yet clear, what should happen
if all_match is unset.
BTW I suspect that a better way than having two filter lists is
demonstrated in builtin-grep.c.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] path-list.c: always free strdup'ed paths
From: René Scharfe @ 2007-07-07 19:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Git Mailing List
Always free .paths if .strdup_paths is set, no matter if the
parameter free_items is set or not, plugging a minor memory leak.
And to clarify the meaning of the flag, rename it to free_util,
since it now only affects the freeing of the .util field.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
path-list.c | 14 ++++++++------
path-list.h | 2 +-
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/path-list.c b/path-list.c
index dcb4b3a..3d83b7b 100644
--- a/path-list.c
+++ b/path-list.c
@@ -76,16 +76,18 @@ struct path_list_item *path_list_lookup(const char *path, struct path_list *list
return list->items + i;
}
-void path_list_clear(struct path_list *list, int free_items)
+void path_list_clear(struct path_list *list, int free_util)
{
if (list->items) {
int i;
- if (free_items)
- for (i = 0; i < list->nr; i++) {
- if (list->strdup_paths)
- free(list->items[i].path);
+ if (list->strdup_paths) {
+ for (i = 0; i < list->nr; i++)
+ free(list->items[i].path);
+ }
+ if (free_util) {
+ for (i = 0; i < list->nr; i++)
free(list->items[i].util);
- }
+ }
free(list->items);
}
list->items = NULL;
diff --git a/path-list.h b/path-list.h
index ce5ffab..5931e2c 100644
--- a/path-list.h
+++ b/path-list.h
@@ -15,7 +15,7 @@ struct path_list
void print_path_list(const char *text, const struct path_list *p);
int path_list_has_path(const struct path_list *list, const char *path);
-void path_list_clear(struct path_list *list, int free_items);
+void path_list_clear(struct path_list *list, int free_util);
struct path_list_item *path_list_insert(const char *path, struct path_list *list);
struct path_list_item *path_list_lookup(const char *path, struct path_list *list);
^ permalink raw reply related
* Re: [PATCH] gitweb: prefer git_get_project_owner() over get_file_owner()
From: Luben Tuikov @ 2007-07-07 19:05 UTC (permalink / raw)
To: Junio C Hamano, Miklos Vajna
Cc: git, Petr Baudis, Jakub Narebski, Luben Tuikov
In-Reply-To: <7vzm2ckb3h.fsf@assigned-by-dhcp.cox.net>
--- Junio C Hamano <gitster@pobox.com> wrote:
> Miklos Vajna <vmiklos@frugalware.org> writes:
>
> > This way if $projects_list exists, it'll be used, otherwise get_file_owner()
> > will be used as before.
> >
> > Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
>
> The patch looks good to me.
>
> HOWEVER.
>
> It strikes me that repeated call to git_get_project_owner()
> would be way too inefficient. Not caller's fault.
>
> How about doing something like this on top of your patch?
Yes, this sounds sensible. ACK.
Luben
>
> gitweb/gitweb.perl | 28 ++++++++++++++++++++--------
> 1 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index dbfb044..f114585 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1465,12 +1465,12 @@ sub git_get_projects_list {
> return @list;
> }
>
> -sub git_get_project_owner {
> - my $project = shift;
> - my $owner;
> +our $gitweb_project_owner = undef;
> +sub git_get_project_list_from_file {
>
> - return undef unless $project;
> + return if (defined $gitweb_project_owner);
>
> + $gitweb_project_owner = {};
> # read from file (url-encoded):
> # 'git%2Fgit.git Linus+Torvalds'
> # 'libs%2Fklibc%2Fklibc.git H.+Peter+Anvin'
> @@ -1482,13 +1482,25 @@ sub git_get_project_owner {
> my ($pr, $ow) = split ' ', $line;
> $pr = unescape($pr);
> $ow = unescape($ow);
> - if ($pr eq $project) {
> - $owner = to_utf8($ow);
> - last;
> - }
> + $gitweb_project_owner->{$project} = to_utf8($ow);
> }
> close $fd;
> }
> +}
> +
> +sub git_get_project_owner {
> + my $project = shift;
> + my $owner;
> +
> + return undef unless $project;
> +
> + if (!defined $gitweb_project_owner) {
> + git_get_project_list_from_file();
> + }
> +
> + if (exists $gitweb_project_owner->{$project}) {
> + $owner = $gitweb_project_owner->{$project};
> + }
> if (!defined $owner) {
> $owner = get_file_owner("$projectroot/$project");
> }
>
>
^ permalink raw reply
* Re: [PATCH] diff-lib.c: don't strdup twice
From: Johannes Schindelin @ 2007-07-07 18:52 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <468FD91C.7040506@lsrfire.ath.cx>
Hi,
On Sat, 7 Jul 2007, Ren? Scharfe wrote:
> The static function read_directory in diff-lib.c is only ever called
> with struct path_list lists with .strdup_paths turned on, i.e.
> path_list_insert will strdup the paths for us (again). Let's take
> advantage of that and stop doing it twice.
Indeed.
Acked-by: Johannes E. Schindelin <johannes.schindelin@gmx.de>
Thanks,
Dscho
^ permalink raw reply
* [PATCH] revision: allow selection of commits that do not match a pattern
From: Sven Verdoolaege @ 2007-07-07 18:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.64.0707071831300.4093@racer.site>
We do this by maintaining two lists of patterns, one for
those that should match and one for those that should not match.
Signed-off-by: Sven Verdoolaege <skimo@kotnet.org>
---
Documentation/git-rev-list.txt | 8 +++++
revision.c | 59 +++++++++++++++++++++++++++++++--------
revision.h | 1 +
3 files changed, 56 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 20dcac6..a27e4dd 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -214,11 +214,19 @@ limiting may be applied.
Limit the commits output to ones with author/committer
header lines that match the specified pattern (regular expression).
+ A pattern starting with a '!' will show only commits that do
+ not match the remainder of the pattern.
+ To match lines starting with '!', escape the initial '!'
+ with a backslash.
--grep='pattern'::
Limit the commits output to ones with log message that
matches the specified pattern (regular expression).
+ A pattern starting with a '!' will show only commits that do
+ not match the remainder of the pattern.
+ To match lines starting with '!', escape the initial '!'
+ with a backslash.
--regexp-ignore-case::
diff --git a/revision.c b/revision.c
index 5184716..4b00ada 100644
--- a/revision.c
+++ b/revision.c
@@ -821,40 +821,65 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
return 0;
}
-static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
+static void add_grep_to_filter(struct grep_opt **filter, const char *ptn,
+ enum grep_pat_token what)
{
- if (!revs->grep_filter) {
+ if (!*filter) {
struct grep_opt *opt = xcalloc(1, sizeof(*opt));
opt->status_only = 1;
opt->pattern_tail = &(opt->pattern_list);
opt->regflags = REG_NEWLINE;
- revs->grep_filter = opt;
+ *filter = opt;
}
- append_grep_pattern(revs->grep_filter, ptn,
- "command line", 0, what);
+ append_grep_pattern(*filter, ptn, "command line", 0, what);
}
-static void add_header_grep(struct rev_info *revs, const char *field, const char *pattern)
+static void add_grep(struct rev_info *revs, const char *ptn,
+ enum grep_pat_token what, int negated)
+{
+ if (negated)
+ add_grep_to_filter(&revs->grep_neg_filter, ptn, what);
+ else
+ add_grep_to_filter(&revs->grep_filter, ptn, what);
+}
+
+static void add_header_grep(struct rev_info *revs, const char *field,
+ const char *pattern)
{
char *pat;
const char *prefix;
int patlen, fldlen;
+ int negated = 0;
fldlen = strlen(field);
patlen = strlen(pattern);
pat = xmalloc(patlen + fldlen + 10);
prefix = ".*";
+ if (*pattern == '!') {
+ negated = 1;
+ pattern++;
+ }
+ if (pattern[0] == '\\' && pattern[1] == '!')
+ pattern++;
if (*pattern == '^') {
prefix = "";
pattern++;
}
sprintf(pat, "^%s %s%s", field, prefix, pattern);
- add_grep(revs, pat, GREP_PATTERN_HEAD);
+ add_grep(revs, pat, GREP_PATTERN_HEAD, negated);
}
static void add_message_grep(struct rev_info *revs, const char *pattern)
{
- add_grep(revs, pattern, GREP_PATTERN_BODY);
+ int negated = 0;
+
+ if (*pattern == '!') {
+ negated = 1;
+ pattern++;
+ }
+ if (pattern[0] == '\\' && pattern[1] == '!')
+ pattern++;
+ add_grep(revs, pattern, GREP_PATTERN_BODY, negated);
}
static void add_ignore_packed(struct rev_info *revs, const char *name)
@@ -1212,6 +1237,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
if (revs->grep_filter)
revs->grep_filter->regflags |= regflags;
+ if (revs->grep_neg_filter)
+ revs->grep_neg_filter->regflags |= regflags;
+
if (show_merge)
prepare_show_merge(revs);
if (def && !revs->pending.nr) {
@@ -1249,6 +1277,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
compile_grep_patterns(revs->grep_filter);
}
+ if (revs->grep_neg_filter) {
+ compile_grep_patterns(revs->grep_neg_filter);
+ }
+
return left;
}
@@ -1329,11 +1361,14 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
static int commit_match(struct commit *commit, struct rev_info *opt)
{
- if (!opt->grep_filter)
- return 1;
- return grep_buffer(opt->grep_filter,
+ return (!opt->grep_filter ||
+ grep_buffer(opt->grep_filter,
+ NULL, /* we say nothing, not even filename */
+ commit->buffer, strlen(commit->buffer))) &&
+ (!opt->grep_neg_filter ||
+ !grep_buffer(opt->grep_neg_filter,
NULL, /* we say nothing, not even filename */
- commit->buffer, strlen(commit->buffer));
+ commit->buffer, strlen(commit->buffer)));
}
static struct commit *get_revision_1(struct rev_info *revs)
diff --git a/revision.h b/revision.h
index f46b4d5..9728d4c 100644
--- a/revision.h
+++ b/revision.h
@@ -84,6 +84,7 @@ struct rev_info {
/* Filter by commit log message */
struct grep_opt *grep_filter;
+ struct grep_opt *grep_neg_filter;
/* special limits */
int skip_count;
--
1.5.3.rc0.41.ge7772
^ permalink raw reply related
* [PATCH] diff-lib.c: don't strdup twice
From: René Scharfe @ 2007-07-07 18:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Git Mailing List
The static function read_directory in diff-lib.c is only ever called
with struct path_list lists with .strdup_paths turned on, i.e.
path_list_insert will strdup the paths for us (again). Let's take
advantage of that and stop doing it twice.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
diff-lib.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/diff-lib.c b/diff-lib.c
index 7fb19c7..92c0e39 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -24,7 +24,7 @@ static int read_directory(const char *path, struct path_list *list)
while ((e = readdir(dir)))
if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
- path_list_insert(xstrdup(e->d_name), list);
+ path_list_insert(e->d_name, list);
closedir(dir);
return 0;
^ permalink raw reply related
* [PATCH] Fix "apply --reverse" with regard to whitespace
From: Johannes Schindelin @ 2007-07-07 17:50 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git, gitster
In-Reply-To: <Pine.LNX.4.64.0707062155170.6977@iabervon.org>
"git apply" used to take check the whitespace in the wrong
direction.
Noticed by Daniel Barkalow.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
On Fri, 6 Jul 2007, Daniel Barkalow wrote:
> If you apply in reverse a patch which adds junk (e.g., terminal
> whitespace), it complains about the junk you're adding, even
> though (since it's in reverse) you're actually removing that
> junk.
This fixes it.
builtin-apply.c | 6 +++++-
t/t4116-apply-reverse.sh | 6 ++++++
2 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index c6f736c..0a0b4a9 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1003,12 +1003,16 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s
trailing++;
break;
case '-':
+ if (apply_in_reverse &&
+ new_whitespace != nowarn_whitespace)
+ check_whitespace(line, len);
deleted++;
oldlines--;
trailing = 0;
break;
case '+':
- if (new_whitespace != nowarn_whitespace)
+ if (!apply_in_reverse &&
+ new_whitespace != nowarn_whitespace)
check_whitespace(line, len);
added++;
newlines--;
diff --git a/t/t4116-apply-reverse.sh b/t/t4116-apply-reverse.sh
index a7f5905..9ae2b3a 100755
--- a/t/t4116-apply-reverse.sh
+++ b/t/t4116-apply-reverse.sh
@@ -82,4 +82,10 @@ test_expect_success 'apply in reverse without postimage' '
)
'
+test_expect_success 'reversing a whitespace introduction' '
+ sed "s/a/a /" < file1 > file1.new &&
+ mv file1.new file1 &&
+ git diff | git apply --reverse --whitespace=error
+'
+
test_done
--
1.5.3.rc0.2712.g125b7f
^ permalink raw reply related
* Re: [PATCH] revision: allow selection of commits that do not match a pattern
From: Johannes Schindelin @ 2007-07-07 17:33 UTC (permalink / raw)
To: skimo; +Cc: Junio C Hamano, git
In-Reply-To: <20070707165208.GC1528MdfPADPa@greensroom.kotnet.org>
Hi,
On Sat, 7 Jul 2007, Sven Verdoolaege wrote:
> On Sat, Jul 07, 2007 at 05:27:23PM +0100, Johannes Schindelin wrote:
> > I suspect that with this patch,
> >
> > git rev-list --not --grep bugfix HEAD
> >
> > does not work as expected. Why?
>
> Well... I guess that depends on what you expect...
Well, at least you hopefully that it is confusing. To use --not for grep
patterns _as well_ as for revision arguments.
> > Why not make "git rev-list --grep '!bugfix' HEAD" work?
> >
> > Yes, you would have to have a special exception that the prefix "!!"
> > actually matches an exclamation mark, but I'd be willing to live with
> > that.
>
> Hmm... what if you want to (not) match anything starting with
> one or more '!' ?
Yeah, that would not work.
> How about I add a '--invert-match' option that would
> apply to all following match options?
> Or we could escape the '!' with backslash.
If we want to match it at the beginning. Yes, that sounds more reasonable
to me.
Ciao,
Dscho
^ 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