* [PATCH] setup.c: Remove duplicate code from prefix_pathspec()
From: Lukas Fleischer @ 2013-03-14 0:24 UTC (permalink / raw)
To: git; +Cc: Andrew Wong, Junio C Hamano
Only check for ',' explicitly and handle both ')' and '\0' in the else
branch. strcspn() can only return ',', ')' and '\0' at this point so the
new code is completely equivalent to what we had before.
This also fixes following GCC warning:
setup.c: In function ‘get_pathspec’:
setup.c:207:8: warning: ‘nextat’ may be used uninitialized in this function [-Wmaybe-uninitialized]
setup.c:205:15: note: ‘nextat’ was declared here
Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
---
This removes the only warning that currently occurs when building the
next branch.
Junio: Feel free to squash this into Andrew's patch (805da4dee15b,
"setup.c: stop prefix_pathspec() from looping past the end of string").
setup.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/setup.c b/setup.c
index d2335a8..94c1e61 100644
--- a/setup.c
+++ b/setup.c
@@ -207,12 +207,11 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char
*copyfrom && *copyfrom != ')';
copyfrom = nextat) {
size_t len = strcspn(copyfrom, ",)");
- if (copyfrom[len] == '\0')
- nextat = copyfrom + len;
- else if (copyfrom[len] == ')')
- nextat = copyfrom + len;
- else if (copyfrom[len] == ',')
+ if (copyfrom[len] == ',')
nextat = copyfrom + len + 1;
+ else
+ /* handle ')' and '\0' */
+ nextat = copyfrom + len;
if (!len)
continue;
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
--
1.8.2.rc2.352.g908df73
^ permalink raw reply related
* Re: [PATCH 0/4] contrib/subtree: general updates
From: Miles Bader @ 2013-03-14 2:52 UTC (permalink / raw)
To: Paul Campbell; +Cc: git, David Greene
In-Reply-To: <1363213963-4065-1-git-send-email-pcampbell@kemitix.net>
Paul Campbell <pcampbell@kemitix.net> writes:
> James and Michael's patches add if clauses that use the
> bashism 'if []' rather than 'if test'.
"Bashism"...? I dunno how portable is, but "[" is an old unix alias
for "test" ... it certainly predates bash...
-miles
--
Occam's razor split hairs so well, I bought the whole argument!
^ permalink raw reply
* Re: [PATCH 0/4] contrib/subtree: general updates
From: Junio C Hamano @ 2013-03-14 3:30 UTC (permalink / raw)
To: Miles Bader; +Cc: Paul Campbell, git, David Greene
In-Reply-To: <874ngebsm9.fsf@catnip.gol.com>
Miles Bader <miles@gnu.org> writes:
> Paul Campbell <pcampbell@kemitix.net> writes:
>> James and Michael's patches add if clauses that use the
>> bashism 'if []' rather than 'if test'.
>
> "Bashism"...? I dunno how portable is, but "[" is an old unix alias
> for "test" ... it certainly predates bash...
Correct. [[ ... ]] is new and spelling out "test" indeed is more
traditionalist than [ ... ], but for contrib/subtree/ that does not
work with anything but bash, I do not think such a rewrite has much
merit in the first place. Being consistently "bash script" (as
opposed to being old-style) is more appropriate for it.
^ permalink raw reply
* Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree
From: David Aguilar @ 2013-03-14 3:41 UTC (permalink / raw)
To: John Keeping; +Cc: git, Junio C Hamano, Matt McClure, Tim Henigan
In-Reply-To: <796eafb6816b302c87873c8f4a1bd2225ce40c55.1363206651.git.john@keeping.me.uk>
On Wed, Mar 13, 2013 at 1:33 PM, John Keeping <john@keeping.me.uk> wrote:
> Some users like to edit files in their diff tool when using "git
> difftool --dir-diff --symlink" to compare against the working tree but
> difftool currently only created symlinks when a file contains unstaged
> changes.
>
> Change this behaviour so that symlinks are created whenever the
> right-hand side of the comparison has the same SHA1 as the file in the
> working tree.
>
> Note that textconv filters are handled in the same way as by git-diff
> and if a clean filter is not the inverse of its smudge filter we already
> get a null SHA1 from "diff --raw" and will symlink the file without
> going through the new hash-object based check.
>
> Reported-by: Matt McClure <matthewlmcclure@gmail.com>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> Documentation/git-difftool.txt | 4 +++-
> git-difftool.perl | 21 ++++++++++++++++++---
> t/t7800-difftool.sh | 14 ++++++++++++++
> 3 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> index e575fea..8361e6e 100644
> --- a/Documentation/git-difftool.txt
> +++ b/Documentation/git-difftool.txt
> @@ -72,7 +72,9 @@ with custom merge tool commands and has the same value as `$MERGED`.
> --symlinks::
> --no-symlinks::
> 'git difftool''s default behavior is create symlinks to the
> - working tree when run in `--dir-diff` mode.
> + working tree when run in `--dir-diff` mode and the right-hand
> + side of the comparison yields the same content as the file in
> + the working tree.
> +
> Specifying `--no-symlinks` instructs 'git difftool' to create copies
> instead. `--no-symlinks` is the default on Windows.
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 0a90de4..5f093ae 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -83,6 +83,21 @@ sub exit_cleanup
> exit($status | ($status >> 8));
> }
>
> +sub use_wt_file
> +{
> + my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
> + my $null_sha1 = '0' x 40;
> +
> + if ($sha1 eq $null_sha1) {
> + return 1;
> + } elsif (not $symlinks) {
> + return 0;
> + }
> +
> + my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> + return $sha1 eq $wt_sha1;
> +}
> +
> sub setup_dir_diff
> {
> my ($repo, $workdir, $symlinks) = @_;
> @@ -159,10 +174,10 @@ EOF
> }
>
> if ($rmode ne $null_mode) {
> - if ($rsha1 ne $null_sha1) {
> - $rindex .= "$rmode $rsha1\t$dst_path\0";
> - } else {
> + if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
> push(@working_tree, $dst_path);
> + } else {
> + $rindex .= "$rmode $rsha1\t$dst_path\0";
> }
> }
> }
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index eb1d3f8..8102ce1 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '
> echo "$diff" | stdin_contains file
> '
>
> +write_script .git/CHECK_SYMLINKS <<\EOF &&
Tiny nit. Is there any downside to leaving this file
at the root instead of inside the .git dir?
> +#!/bin/sh
> +test -L "$2/file" &&
> +test -L "$2/file2" &&
> +test -L "$2/sub/sub"
> +echo $?
> +EOF
> +
> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> + result=$(git difftool --dir-diff --symlink \
> + --extcmd "./.git/CHECK_SYMLINKS" branch HEAD) &&
> + test "$result" = 0
> +'
> +
How about something like this?
+ echo 0 >expect &&
+ git difftool --dir-diff --symlink \
+ --extcmd ./CHECK_SYMLINKS branch HEAD >actual &&
+ test_cmp expect actual
(sans gmail whitespace damage) so that we can keep it chained with &&.
Ah.. it seems your branch is based on master, perhaps?
There's stuff cooking in next for difftool's tests.
I'm not sure if this patch is based on top of them.
Can you rebase the tests so that the chaining is done like it is in 'next'?
--
David
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Michael Haggerty @ 2013-03-14 4:41 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <20130313215800.GA23838@sigill.intra.peff.net>
On 03/13/2013 10:58 PM, Jeff King wrote:
> On Wed, Mar 13, 2013 at 10:29:54AM -0700, Junio C Hamano wrote:
>
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> It is not
>>> clear to me whether the prohibition of tags outside of refs/tags should
>>> be made more airtight or whether the peeling of tags outside of
>>> refs/tags should be fixed.
>>
>> Retroactively forbidding presense/creation of tags outside the
>> designated refs/tags hierarchy will not fly. I think we should peel
>> them when we are reading from "# pack-refs with: peeled" version.
>>
>> Theoretically, we could introduce "# pack-refs with: fully-peeled"
>> that records peeled versions for _all_ annotated tags even in
>> unusual places, but that would be introducing problems to existing
>> versions of the software to cater to use cases that is not blessed
>> officially, so I doubt it has much value.
I think that instead of changing "peeled" to "fully-peeled", it would be
better to add "fully-peeled" as an additional keyword, like
# pack-refs with: peeled fully-peeled
Old readers would still see the "peeled" keyword and ignore the
fully-peeled keyword, and would be able to read the file correctly. See
below for more discussion.
> I agree. The REF_KNOWS_PEELED thing is an optimization, so it's OK to
> simply omit the optimization in the corner case, since we don't expect
> it to happen often. So what happens now is a bug, but it is a bug in the
> reader that mis-applies the optimization, and we can just fix the
> reader.
>
> I do not think there is any point in peeling while we are reading the
> pack-refs file; it is no more expensive to do so later, and in most
> cases we will not even bother peeling. We should simply omit the
> REF_KNOWS_PEELED bit so that later calls to peel_ref do not follow the
> optimization code path. Something like this:
>
> diff --git a/refs.c b/refs.c
> index 175b9fc..ee498c8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -824,7 +824,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
>
> refname = parse_ref_line(refline, sha1);
> if (refname) {
> - last = create_ref_entry(refname, sha1, flag, 1);
> + int this_flag = flag;
> + if (prefixcmp(refname, "refs/tags/"))
> + this_flag &= ~REF_KNOWS_PEELED;
> + last = create_ref_entry(refname, sha1, this_flag, 1);
> add_ref(dir, last);
> continue;
> }
It would also be possible to set the REF_KNOWS_PEELED for any entries
for which a peeled reference happens to be present in the packed-refs
file, though the code would never be triggered if the current writer is
not changed.
> I think with this patch, though, that upload-pack would end up having to
> read the object type of everything under refs/heads to decide whether it
> needs to be peeled (and in most cases, it does not, making the initial
> ref advertisement potentially much more expensive). How do we want to
> handle that? Should we teach upload-pack not to bother advertising peels
> outside of refs/tags? That would break people who expect tag
> auto-following to work for refs outside of refs/tags, but I am not sure
> that is a sane thing to expect.
Here is analysis of our options as I see them:
1. Accept that tags outside of refs/tags are not reliably advertised in
their peeled form. Document this deficiency and either:
a. Don't even bother trying to peel refs outside of refs/tags (the
status quo).
b. Change the pack-refs writer to write all peeled refs, but leave
the reader unchanged. This is a low-risk option that would cause
old and new clients to do the right thing when reading a full
packed-refs file, but an old or new client reading a non-full
packed-refs file would not realize that it is non-full and would
fail to advertise all peeled refs. Minor disadvantage: pack-refs
becomes slower.
2. Insist that tags outside of refs/tags are reliably advertised. I
see three ways to do it:
a. Without changing the packed-refs contents. This would require
upload-pack to read *all* references outside of refs/tags. (This
is what Peff's patch does.)
b. Write all peeled refs to packed-refs without changing the
packed-refs header. This would hardly help, as upload-pack
would still have to read all non-tag references outside of
refs/tags to be sure that none are tags.
c. Add a new keyword to the top of the packed-refs file as
described above. Then
* Old writer, new reader: the reader would know that some
peeled refs might be missing. upload-pack would have to
resolve refs outside of refs/tags, but could optionally write
a new-format packed-refs file to avoid repeating the work.
* New writer, new reader: the reader would know that all refs
are peeled properly and would not have to read any objects.
* Old writer, old reader: status quo; peeled refs are advertised
incompletely.
* New writer, old reader: This is the interesting case. The
current code in Git would read the header and see "peeled" but
ignore "fully-peeled". But the line-reading code would
nevertheless happily read and record peeled references no
matter what namespace they live in. It would also advertise
them correctly. One would have to check historical versions
and other clients to see whether they would also behave well.
d. Add some new notation, like a "^" on a line by itself, to mean
"I tried to peel this reference but it was not an annotated tag".
Old readers ignore such lines but new readers could take it
as an indication to set the REF_KNOWS_PEELED bit for that entry.
(It is not clear to me whether it would be permissible to make a
change like this without changing the header line.)
I think the best option would be 1b. Even though there would never be a
guarantee that all peeled references are recorded and advertised
correctly, the behavior would asymptotically approach correctness as old
Git versions are retired, and the situations where it fails are probably
rare and no worse than the status quo.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Jeff King @ 2013-03-14 5:24 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <51415516.2070702@alum.mit.edu>
On Thu, Mar 14, 2013 at 05:41:58AM +0100, Michael Haggerty wrote:
> Here is analysis of our options as I see them:
>
> 1. Accept that tags outside of refs/tags are not reliably advertised in
> their peeled form. Document this deficiency and either:
>
> a. Don't even bother trying to peel refs outside of refs/tags (the
> status quo).
When you say "not reliably advertised" you mean from upload-pack, right?
What about other callers? From my reading of peel_ref, anybody who calls
it to get a packed ref may actually get a wrong answer. So this is not
just about tag auto-following over fetch, but about other places, too
(however, a quick grep does not make it look like this other places are
all that commonly used).
Another fun fact: upload-pack did not use peel_ref until recently
(435c833, in v1.8.1). So while it is tempting to say "well, this was
always broken, and nobody cared", it was not really; it is a fairly
recent regression in 435c833.
> b. Change the pack-refs writer to write all peeled refs, but leave
> the reader unchanged. This is a low-risk option that would cause
> old and new clients to do the right thing when reading a full
> packed-refs file, but an old or new client reading a non-full
> packed-refs file would not realize that it is non-full and would
> fail to advertise all peeled refs. Minor disadvantage: pack-refs
> becomes slower.
This seems sane. The missing thing is a flag to tell the reader that it
was written by a newer version; I see you dealt with that case below.
I don't think pack-refs being a little bit slower matters. Checking the
types to peel is not even that much work; it's just that it adds up when
you do it for every no-op fetch's ref advertisement. But we can assume
that writing happens a lot less than reading; that is the point of
storing the peeled information in the first place. If that assumption is
not correct in some scenario, then those people should probably not be
packing their refs at all, so I think we can discount them from this
discussion.
> 2. Insist that tags outside of refs/tags are reliably advertised. I
> see three ways to do it:
>
> a. Without changing the packed-refs contents. This would require
> upload-pack to read *all* references outside of refs/tags. (This
> is what Peff's patch does.)
FWIW, this is what upload-pack used to do before I switched it to use
peel_ref. The savings are measurable, though they are not
ground-breaking. Still, I think your "fully-packed" proposal above lets
us keep the improvement without too much effort.
> b. Write all peeled refs to packed-refs without changing the
> packed-refs header. This would hardly help, as upload-pack
> would still have to read all non-tag references outside of
> refs/tags to be sure that none are tags.
Right, this seems silly; the new reader does extra work for
compatibility with an older writer, but the normal case is to use the
same version. The obvious optimization is to add a flag that says "hey,
I was written by a new writer". And that is your "fully-packed"
proposal, covered below.
> c. Add a new keyword to the top of the packed-refs file as
> described above. Then
>
> * Old writer, new reader: the reader would know that some
> peeled refs might be missing. upload-pack would have to
> resolve refs outside of refs/tags, but could optionally write
> a new-format packed-refs file to avoid repeating the work.
I think that is OK. For the most part, this is a temporary situation
that happens when you are moving from an older to a newer version of
git. If you are switching back and forth between versions, then we are
correct, but you don't get the benefit of the micro-optimization, which
seems fair.
> * New writer, new reader: the reader would know that all refs
> are peeled properly and would not have to read any objects.
This is the common case I think we should be optimizing for. And
obviously the outcome here is good. It's also fine without adding the
"fully-packed" flag, though.
> * Old writer, old reader: status quo; peeled refs are advertised
> incompletely.
Right. We can't fix those without a time machine, though.
> * New writer, old reader: This is the interesting case. The
> current code in Git would read the header and see "peeled" but
> ignore "fully-peeled". But the line-reading code would
> nevertheless happily read and record peeled references no
> matter what namespace they live in. It would also advertise
> them correctly. One would have to check historical versions
> and other clients to see whether they would also behave well.
Right. So we have pretty sane backwards-compatible behavior. I think if
other implementations are not happy seeing a new tag, they are wrong.
The whole point of the "#"-tags is for future expansion. Looking over
libgit2, it seems to ignore the "#"-line completely, so I think it would
behave OK (and it should be taught about fully-peeled, too, but that is
not our immediate problem).
> d. Add some new notation, like a "^" on a line by itself, to mean
> "I tried to peel this reference but it was not an annotated tag".
> Old readers ignore such lines but new readers could take it
> as an indication to set the REF_KNOWS_PEELED bit for that entry.
> (It is not clear to me whether it would be permissible to make a
> change like this without changing the header line.)
I don't see the point. The writer wants to provide REF_KNOWS_PEELED for
every entry in its list so that the reader (which is run more often)
does not have to put forth any effort. So this accomplishes the same
thing as a "anything without a peeled ref did not need peeling" flag,
but takes more space.
> I think the best option would be 1b. Even though there would never be a
> guarantee that all peeled references are recorded and advertised
> correctly, the behavior would asymptotically approach correctness as old
> Git versions are retired, and the situations where it fails are probably
> rare and no worse than the status quo.
Thanks for laying out the options. I think 1b or 2c are the only sane
paths forward. With either option, a newer writer produces something
that all implementations, old and new, should get right, and that is
primarily what we care about.
So the only question is how much work we want to put into making sure
the new reader handles the old writer correctly. Doing 2c is obviously
more rigorous, and it is not that much work to add the fully-packed
flag, but I kind of wonder if anybody even cares. We can just say "it's
a bug fix; run `git pack-refs` again if you care" and call it a day
(i.e., 1b).
I could go either way.
-Peff
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Jeff King @ 2013-03-14 5:32 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <20130314052448.GA2300@sigill.intra.peff.net>
On Thu, Mar 14, 2013 at 01:24:48AM -0400, Jeff King wrote:
> So the only question is how much work we want to put into making sure
> the new reader handles the old writer correctly. Doing 2c is obviously
> more rigorous, and it is not that much work to add the fully-packed
> flag, but I kind of wonder if anybody even cares. We can just say "it's
> a bug fix; run `git pack-refs` again if you care" and call it a day
> (i.e., 1b).
Urgh, for some reason I kept writing "fully-packed" but obviously I
meant "fully-peeled". Hopefully you figured it out.
Just as a quick sketch of how much work is in involved in 2c, I think
the complete solution would look like this (but note I haven't tested
this at all):
diff --git a/pack-refs.c b/pack-refs.c
index f09a054..261a6a6 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -27,6 +27,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
int flags, void *cb_data)
{
struct pack_refs_cb_data *cb = cb_data;
+ struct object *o;
int is_tag_ref;
/* Do not pack the symbolic refs */
@@ -39,14 +40,12 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
return 0;
fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
- if (is_tag_ref) {
- struct object *o = parse_object(sha1);
- if (o->type == OBJ_TAG) {
- o = deref_tag(o, path, 0);
- if (o)
- fprintf(cb->refs_file, "^%s\n",
- sha1_to_hex(o->sha1));
- }
+ o = parse_object(sha1);
+ if (o->type == OBJ_TAG) {
+ o = deref_tag(o, path, 0);
+ if (o)
+ fprintf(cb->refs_file, "^%s\n",
+ sha1_to_hex(o->sha1));
}
if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {
@@ -128,7 +127,7 @@ int pack_refs(unsigned int flags)
die_errno("unable to create ref-pack file structure");
/* perhaps other traits later as well */
- fprintf(cbdata.refs_file, "# pack-refs with: peeled \n");
+ fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n");
for_each_ref(handle_one_ref, &cbdata);
if (ferror(cbdata.refs_file))
diff --git a/refs.c b/refs.c
index 175b9fc..770abf4 100644
--- a/refs.c
+++ b/refs.c
@@ -808,6 +808,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
struct ref_entry *last = NULL;
char refline[PATH_MAX];
int flag = REF_ISPACKED;
+ int fully_peeled = 0;
while (fgets(refline, sizeof(refline), f)) {
unsigned char sha1[20];
@@ -818,13 +819,18 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
const char *traits = refline + sizeof(header) - 1;
if (strstr(traits, " peeled "))
flag |= REF_KNOWS_PEELED;
+ if (strstr(traits, " fully-peeled "))
+ fully_peeled = 1;
/* perhaps other traits later as well */
continue;
}
refname = parse_ref_line(refline, sha1);
if (refname) {
- last = create_ref_entry(refname, sha1, flag, 1);
+ int this_flag = flag;
+ if (!fully_peeled && prefixcmp(refname, "refs/tags/"))
+ this_flag &= ~REF_KNOWS_PEELED;
+ last = create_ref_entry(refname, sha1, this_flag, 1);
add_ref(dir, last);
continue;
}
So it's really not that much code. The bigger question is whether we
want to have to carry the "fully-peeled" tag forever, and how other
implementations would treat it.
-Peff
^ permalink raw reply related
* Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
From: Kevin Bracey @ 2013-03-14 6:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, David Aguilar, Ciaran Jessup, Scott Chacon, Alex Riesen
In-Reply-To: <7vehfj2neh.fsf@alter.siamese.dyndns.org>
On 13/03/2013 19:57, Junio C Hamano wrote:
> Kevin Bracey <kevin@bracey.fi> writes:
>
>> - echo "Added $4 in both, but differently."
>> + echo "ERROR: Added $4 in both, but differently."
>> + ret=1
> The problem you identified may be worth fixing, but I do not think
> this change is correct.
>
> This message is at the same severity level as the message on the
> other arm of this case that says "Auto-merging $4". In that other
> case arm, we are attempting a true three-way merge, and in this case
> arm, we are attempting a similar three-way merge using your "virtual
> base".
>
> Neither has found any error in this case arm yet. The messages are
> both "informational", not an error. I do not think you would want
> to set ret=1 until you see content conflict.
I disagree here. At the minute, it does set ret to 1 (but further down
the code - bringing it up here next to the "ERROR" print clarifies
that), and will report the merge as failed, conflict in the 3-way merge
or not. Which I think is correct.
We have to stop for user inspection here. We do have a fake base; we
can't trust the 3-way merge with it.
The virtual 3-way merge will take ABCDE and ABDE and produce ABCDE
without conflict. That's flat wrong if the real base they failed to tell
Git about was ABCDE.
Despite being useful, I'm still slightly uncomfortable that it can
produce something without any conflict markers. The user really needs to
look at properly.
(And one interesting related glitch, or at least thing that puzzled me
when it happened. This is from memory, so may be slightly mistaken, but
what seemed to happen was that if you have rerere enabled, then
mergetool tends to say "nothing to merge", because it relies on "rerere
remaining", which relies on conflict markers. I think you could still
force a mergetool up by specifying the specific file though.)
Maybe the virtual base itself should be different. Maybe it should put a
??????? marker in place of every unique line. So you get:
Left ABCEFGH
Right XABCDEFJH -> Merge result <|X>ABC<|D>EF<G|J>H
VBase ?ABC?EF??H
That actually feels like it may be the correct answer here. And it's
effectively what P4Merge does in its "2-way" mode I failed to invoke.
(At least for the result view).
Kevin
^ permalink raw reply
* Re: [PATCH/RFC] Changing submodule foreach --recursive to be depth-first, --parent option to execute command in supermodule as well
From: Eric Cousineau @ 2013-03-14 6:30 UTC (permalink / raw)
To: Phil Hord; +Cc: Jens Lehmann, Junio C Hamano, Heiko Voigt, git@vger.kernel.org
In-Reply-To: <CABURp0qBA6myf7_SuaxJSrePJHmh2v-nmtLRzKTtgAJxLkJ-tQ@mail.gmail.com>
From 59fb432e17a1aae9de26bbaaca7f09cc7f03b471 Mon Sep 17 00:00:00 2001
From: Eric Cousineau <eacousineau@gmail.com>
Date: Thu, 14 Mar 2013 01:19:53 -0500
Subject: [PATCH] submodule-foreach: Added in --post-order=<command> per Jens
Lehmann's suggestion
Signed-off-by: Eric Cousineau <eacousineau@gmail.com>
---
Made the scope of the patch only relate to --post-order.
Would we want to rename this to just --post=<command> ?
Anywho, here it is running in a test setup, where the structure is:
a
- b
- - d
- c
$ git submodule foreach --recursive --post-order 'echo Post $name' 'echo
Pre $path'
Entering 'b'
Pre b
Entering 'b/d'
Pre d
Entering 'b/d'
Post d
Entering 'b'
Post b
Entering 'c'
Pre c
Entering 'c'
Post c
An interesting note is that it fails with 'git submodule foreach
--post-order', but not 'git submodule foreach --post-order=', since it
simply interprets that as an empty command.
If that is important, I could add in a check for $# when parsing the
argument for --post-order=*.
git-submodule.sh | 39 ++++++++++++++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 5 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 004c034..9b70bc2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -10,7 +10,7 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force]
[--name <name>] [--reference <re
or: $dashless [--quiet] init [--] [<path>...]
or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch]
[-f|--force] [--rebase] [--reference <repository>] [--merge]
[--recursive] [--] [<path>...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit
<n>] [commit] [--] [<path>...]
- or: $dashless [--quiet] foreach [--recursive] <command>
+ or: $dashless [--quiet] foreach [--recursive]
[--post-order=<command>] <command>
or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
OPTIONS_SPEC=
. git-sh-setup
@@ -434,6 +434,8 @@ Use -f if you really want to add it." >&2
cmd_foreach()
{
# parse $args after "submodule ... foreach".
+ # Gratuitous (empty) local's to prevent recursive bleeding
+ local recursive= post_order=
while test $# -ne 0
do
case "$1" in
@@ -443,6 +445,15 @@ cmd_foreach()
--recursive)
recursive=1
;;
+ --post-order)
+ test "$#" = "1" && usage
+ post_order="$2"
+ shift
+ ;;
+ --post-order=*)
+ # Will skip empty commands
+ post_order=${1#*=}
+ ;;
-*)
usage
;;
@@ -453,7 +464,7 @@ cmd_foreach()
shift
done
- toplevel=$(pwd)
+ local toplevel=$(pwd)
# dup stdin so that it can be restored when running the external
# command in the subshell (and a recursive call to this function)
@@ -465,18 +476,36 @@ cmd_foreach()
die_if_unmatched "$mode"
if test -e "$sm_path"/.git
then
- say "$(eval_gettext "Entering '\$prefix\$sm_path'")"
+ local name prefix path message epitaph
+ message="$(eval_gettext "Entering '\$prefix\$sm_path'")"
+ epitaph="$(eval_gettext "Stopping at '\$sm_path'; script
returned non-zero status.")"
name=$(module_name "$sm_path")
(
prefix="$prefix$sm_path/"
clear_local_git_env
# we make $path available to scripts ...
path=$sm_path
+
+ sm_eval() {
+ say "$message"
+ eval "$@" || die "$epitaph"
+ }
+
cd "$sm_path" &&
- eval "$@" &&
+ sm_eval "$@" &&
if test -n "$recursive"
then
- cmd_foreach "--recursive" "$@"
+ if test -n "$post_order"
+ then
+ # Tried keeping flags as a variable, but was
having difficulty
+ cmd_foreach --recursive --post-order
"$post_order" "$@"
+ else
+ cmd_foreach --recursive "$@"
+ fi
+ fi &&
+ if test -n "$post_order"
+ then
+ sm_eval "$post_order"
fi
) <&3 3<&- ||
die "$(eval_gettext "Stopping at '\$sm_path'; script
returned non-zero status.")"
--
1.8.2.rc1.24.g06d67b8.dirty
^ permalink raw reply related
* Re: [PATCH v2 1/4] config: factor out config file stack management
From: Heiko Voigt @ 2013-03-14 6:36 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312190456.GC17099@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 03:04:56PM -0400, Jeff King wrote:
> On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:
>
> > > Can we throw in a comment at the top here with the expected usage? In
> > > particular, do_config_from is expecting the caller to have filled in
> > > certain fields (at this point, top->f and top->name), but there is
> > > nothing to make that clear.
> >
> > Of course. Will do that in the next iteration. How about I squash this in:
> > [...]
> > +/* The fields data, name and the source specific callbacks of top need
> > + * to be initialized before calling this function.
> > + */
> > static int do_config_from_source(struct config_source *top, config_fn_t fn, voi
>
> I think that is OK, but it may be even better to list the fields by
> name. Also, our multi-line comment style is:
>
> /*
> * Multi-line comment.
> */
Ok will do both.
> > I would add that to the third patch:
> >
> > config: make parsing stack struct independent from actual data source
> >
> > because that contains the final modification to config_file/config_source.
>
> It does not matter to the end result, but I find it helps with reviewing
> when the comment is added along with the function, and then expanded as
> the function is changed. It helps to understand the effects of later
> patches if they need to tweak comments.
To make the series more clear to others who read it later, I will add
the comment from the beginning.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v2 4/4] teach config parsing to read from strbuf
From: Heiko Voigt @ 2013-03-14 6:39 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312192959.GG17099@sigill.intra.peff.net>
On Tue, Mar 12, 2013 at 03:29:59PM -0400, Jeff King wrote:
> On Tue, Mar 12, 2013 at 05:42:54PM +0100, Heiko Voigt wrote:
>
> > > Your series does not actually add any callers of the new function. The
> > > obvious "patch 5/4" would be to plumb it into "git config --blob", and
> > > then we can just directly test it there (there could be other callers
> > > besides reading from a blob, of course, but I think the point of the
> > > series is to head in that direction).
> >
> > Since this is a split of the series mentioned above there are no real
> > callers yet. The main reason for the split was that I wanted to reduce
> > the review burden of one big series into multiple reviews of smaller
> > chunks. If you think it is useful to add the --blob option I can also
> > test from there. It could actually be useful to look at certain
> > .gitmodules options from the submodule script.
>
> I am on the fence. I do not want to create more work for you, but I do
> think it may come in handy, if only for doing submodule things from
> shell. And it is hopefully not a very large patch. I'd say try it, and
> if starts looking like it will be very ugly, the right thing may be to
> leave it until somebody really wants it.
Thats what I will do: Try it and see where I get.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 1/2] t2200: check that "add -u" limits itself to subdirectory
From: Jeff King @ 2013-03-14 6:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, git
In-Reply-To: <7vli9r2o06.fsf@alter.siamese.dyndns.org>
On Wed, Mar 13, 2013 at 10:44:25AM -0700, Junio C Hamano wrote:
> > +# Note that this is scheduled to change in Git 2.0, when
> > +# "git add -u" will become full-tree by default.
> > +test_expect_success 'update did not touch files at root' '
> > + cat >expect <<-\EOF &&
> > + check
> > + top
> > + EOF
> > + git diff-files --name-only >actual &&
> > + test_cmp expect actual
> > +'
>
> The last "git add -u" we have beforet his block is this test piece:
>
> test_expect_success 'update from a subdirectory' '
> (
> cd dir1 &&
> echo more >sub2 &&
> git add -u sub2
> )
> '
>
> That is not "git add -u" without pathspec, which is the only thing
> we are transitioning at Git 2.0 boundary.
Oops, you're right. I just saw the "cd" and totally missed the pathspec.
The correct test should be:
-- >8 --
Subject: [PATCH] t2200: check that "add -u" limits itself to subdirectory
This behavior is due to change in the future, but let's test
it anyway. That helps make sure we do not accidentally
switch the behavior too soon while we are working in the
area, and it means that we can easily verify the change when
we do make it.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t2200-add-update.sh | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 4cdebda..c317254 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -80,6 +80,22 @@ test_expect_success 'change gets noticed' '
'
+# Note that this is scheduled to change in Git 2.0, when
+# "git add -u" will become full-tree by default.
+test_expect_success 'non-limited update in subdir leaves root alone' '
+ (
+ cd dir1 &&
+ echo even more >>sub2 &&
+ git add -u
+ ) &&
+ cat >expect <<-\EOF &&
+ check
+ top
+ EOF
+ git diff-files --name-only >actual &&
+ test_cmp expect actual
+'
+
test_expect_success SYMLINKS 'replace a file with a symlink' '
rm foo &&
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related
* Re: [PATCH] Allow combined diff to ignore white-spaces
From: Johannes Sixt @ 2013-03-14 7:08 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: git, Junio C Hamano
In-Reply-To: <1363209683-10264-1-git-send-email-apelisse@gmail.com>
Am 3/13/2013 22:21, schrieb Antoine Pelisse:
> Currently, it's not possible to use the space-ignoring options (-b, -w,
> --ignore-space-at-eol) with combined diff. It makes it pretty impossible
> to read a merge between a branch that changed all tabs to spaces, and a
> branch with functional changes.
>
> Pass diff flags to diff engine, so that combined diff behaves as normal
> diff does with spaces.
> Also coalesce lines that are removed from both (or more) parents.
>
> It also means that a conflict-less merge done using a ignore-* strategy
> option will not show any conflict if shown in combined-diff using the
> same option.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> OK, I added some tests and coalesce similar lost lines (using the same flags
> we used for diff.
>
> combine-diff.c | 57 ++++++++++++++++++++++++++++++-----
> t/t4038-diff-combined.sh | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 125 insertions(+), 7 deletions(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 35d41cd..0f33983 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -5,6 +5,7 @@
> #include "diffcore.h"
> #include "quote.h"
> #include "xdiff-interface.h"
> +#include "xdiff/xmacros.h"
> #include "log-tree.h"
> #include "refs.h"
> #include "userdiff.h"
> @@ -122,7 +123,47 @@ static char *grab_blob(const unsigned char *sha1, unsigned int mode,
> return blob;
> }
>
> -static void append_lost(struct sline *sline, int n, const char *line, int len)
> +static int match_string_spaces(const char *line1, int len1,
> + const char *line2, int len2,
> + long flags)
> +{
> + if (flags & XDF_WHITESPACE_FLAGS) {
> + for (; len1 > 0 && XDL_ISSPACE(line1[len1 - 1]); len1--);
> + for (; len2 > 0 && XDL_ISSPACE(line2[len2 - 1]); len2--);
> + }
> +
> + if (!(flags & (XDF_IGNORE_WHITESPACE | XDF_IGNORE_WHITESPACE_CHANGE)))
> + return (len1 == len2 && !memcmp(line1, line2, len1));
> +
> + while (len1 > 0 && len2 > 0) {
> + len1--;
> + len2--;
> + if (XDL_ISSPACE(line1[len1]) || XDL_ISSPACE(line2[len2])) {
> + if ((flags & XDF_IGNORE_WHITESPACE_CHANGE) &&
> + (!XDL_ISSPACE(line1[len1]) || !XDL_ISSPACE(line2[len2])))
> + return 0;
> +
> + for (; len1 > 0 && XDL_ISSPACE(line1[len1]); len1--);
> + for (; len2 > 0 && XDL_ISSPACE(line2[len2]); len2--);
> + }
> + if (line1[len1] != line2[len2])
> + return 0;
> + }
> +
> + if (flags & XDF_IGNORE_WHITESPACE) {
> + // Consume remaining spaces
> + for (; len1 > 0 && XDL_ISSPACE(line1[len1 - 1]); len1--);
> + for (; len2 > 0 && XDL_ISSPACE(line2[len2 - 1]); len2--);
> + }
> +
> + // We matched full line1 and line2
> + if (!len1 && !len2)
> + return 1;
> +
> + return 0;
> +}
> +
> +static void append_lost(struct sline *sline, int n, const char *line, int len, long flags)
> {
> struct lline *lline;
> unsigned long this_mask = (1UL<<n);
> @@ -133,8 +174,8 @@ static void append_lost(struct sline *sline, int n, const char *line, int len)
> if (sline->lost_head) {
> lline = sline->next_lost;
> while (lline) {
> - if (lline->len == len &&
> - !memcmp(lline->line, line, len)) {
> + if (match_string_spaces(lline->line, lline->len,
> + line, len, flags)) {
> lline->parent_map |= this_mask;
> sline->next_lost = lline->next;
> return;
> @@ -162,6 +203,7 @@ struct combine_diff_state {
> int n;
> struct sline *sline;
> struct sline *lost_bucket;
> + long flags;
> };
>
> static void consume_line(void *state_, char *line, unsigned long len)
> @@ -201,7 +243,7 @@ static void consume_line(void *state_, char *line, unsigned long len)
> return; /* not in any hunk yet */
> switch (line[0]) {
> case '-':
> - append_lost(state->lost_bucket, state->n, line+1, len-1);
> + append_lost(state->lost_bucket, state->n, line+1, len-1, state->flags);
> break;
> case '+':
> state->sline[state->lno-1].flag |= state->nmask;
> @@ -215,7 +257,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
> struct sline *sline, unsigned int cnt, int n,
> int num_parent, int result_deleted,
> struct userdiff_driver *textconv,
> - const char *path)
> + const char *path, long flags)
> {
> unsigned int p_lno, lno;
> unsigned long nmask = (1UL << n);
> @@ -231,9 +273,10 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
> parent_file.ptr = grab_blob(parent, mode, &sz, textconv, path);
> parent_file.size = sz;
> memset(&xpp, 0, sizeof(xpp));
> - xpp.flags = 0;
> + xpp.flags = flags;
> memset(&xecfg, 0, sizeof(xecfg));
> memset(&state, 0, sizeof(state));
> + state.flags = flags;
> state.nmask = nmask;
> state.sline = sline;
> state.lno = 1;
> @@ -962,7 +1005,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
> elem->parent[i].mode,
> &result_file, sline,
> cnt, i, num_parent, result_deleted,
> - textconv, elem->path);
> + textconv, elem->path, opt->xdl_opts);
> }
>
> show_hunks = make_hunks(sline, cnt, num_parent, dense);
> diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
> index 614425a..ba8a56b 100755
> --- a/t/t4038-diff-combined.sh
> +++ b/t/t4038-diff-combined.sh
> @@ -113,4 +113,79 @@ test_expect_success 'check --cc --raw with forty trees' '
> grep "^::::::::::::::::::::::::::::::::::::::::[^:]" out
> '
>
> +test_expect_success 'setup combined ignore spaces' '
> + git checkout master &&
> + >test &&
> + git add test &&
> + git commit -m initial &&
> +
> + echo "
> + always coalesce
> + eol space coalesce \n\
> + space change coalesce
> + all spa ces coalesce
> + eol spaces \n\
> + space change
> + all spa ces" >test &&
This form of 'echo' is not sufficiently portable. How about:
tr -d Q <<-\EOF >test &&
always coalesce
eol space coalesce Q
space change coalesce
all spa ces coalesce
eol spaces Q
space change
all spa ces
EOF
> + git commit -m "change three" -a &&
> +
> + git checkout -b side HEAD^ &&
> + echo "
> + always coalesce
> + eol space coalesce
> + space change coalesce
> + all spaces coalesce
> + eol spaces
> + space change
> + all spaces" >test &&
> + git commit -m indent -a &&
> +
> + test_must_fail git merge master &&
> + echo "
> + eol spaces \n\
> + space change
> + all spa ces" > test &&
Ditto.
> + git commit -m merged -a
> +'
> +
> +test_expect_success 'check combined output (no ignore space)' '
> + git show | test_i18ngrep "^-\s*eol spaces" &&
> + git show | test_i18ngrep "^-\s*eol space coalesce" &&
> + git show | test_i18ngrep "^-\s*space change" &&
> + git show | test_i18ngrep "^-\s*space change coalesce" &&
> + git show | test_i18ngrep "^-\s*all spaces" &&
> + git show | test_i18ngrep "^-\s*all spaces coalesce" &&
> + git show | test_i18ngrep "^--\s*always coalesce"
This loses the exit code of git show. We usually write this as
git show >actual &&
grep "^- *eol spaces" &&
grep "^- *eol space coalesce" &&
...
(Same for later tests.)
There is nothing i18n-ish in the test patterns. Use regular grep.
BTW, there is compare_diff_patch() in diff-lib.sh. You can use it to
compare diff output to expected output. Then you do not need a grep
invocation for each line of the test file.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2 4/4] teach config parsing to read from strbuf
From: Jeff King @ 2013-03-14 7:10 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130314063933.GB4062@sandbox-ub.fritz.box>
On Thu, Mar 14, 2013 at 07:39:33AM +0100, Heiko Voigt wrote:
> > I am on the fence. I do not want to create more work for you, but I do
> > think it may come in handy, if only for doing submodule things from
> > shell. And it is hopefully not a very large patch. I'd say try it, and
> > if starts looking like it will be very ugly, the right thing may be to
> > leave it until somebody really wants it.
>
> Thats what I will do: Try it and see where I get.
I looked into this a little. The first sticking point is that
git_config_with_options needs to support the alternate source. Here's a
sketch of what I think the solution should look like, on top of your
patches.
diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..8d01b7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static const char *given_config_file;
static int use_global_config, use_system_config, use_local_config;
static const char *given_config_file;
+static const char *given_config_blob;
static int actions, types;
static const char *get_color_slot, *get_colorbool_slot;
static int end_null;
@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {
OPT_BOOLEAN(0, "system", &use_system_config, N_("use system config file")),
OPT_BOOLEAN(0, "local", &use_local_config, N_("use repository config file")),
OPT_STRING('f', "file", &given_config_file, N_("file"), N_("use given config file")),
+ OPT_STRING(0, "blob", &given_config_blob, N_("blob-id"), N_("read config from given blob object")),
OPT_GROUP(N_("Action")),
OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)
}
git_config_with_options(collect_config, &values,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
ret = !values.nr;
@@ -302,7 +305,8 @@ static void get_color(const char *def_color)
get_color_found = 0;
parsed_color[0] = '\0';
git_config_with_options(git_get_color_config, NULL,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
if (!get_color_found && def_color)
color_parse(def_color, "command line", parsed_color);
@@ -330,7 +334,8 @@ static int get_colorbool(int print)
get_colorbool_found = -1;
get_diff_color_found = -1;
git_config_with_options(git_get_colorbool_config, NULL,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
if (get_colorbool_found < 0) {
if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -348,6 +353,12 @@ static int get_colorbool(int print)
return get_colorbool_found ? 0 : 1;
}
+static void check_blob_write(void)
+{
+ if (given_config_blob)
+ die("writing config blobs is not supported");
+}
+
int cmd_config(int argc, const char **argv, const char *prefix)
{
int nongit = !startup_info->have_repository;
@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
builtin_config_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
- if (use_global_config + use_system_config + use_local_config + !!given_config_file > 1) {
+ if (use_global_config + use_system_config + use_local_config +
+ !!given_config_file + !!given_config_blob > 1) {
error("only one config file at a time.");
usage_with_options(builtin_config_usage, builtin_config_options);
}
@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_argc(argc, 0, 0);
if (git_config_with_options(show_all_config, NULL,
given_config_file,
+ given_config_blob,
respect_includes) < 0) {
if (given_config_file)
die_errno("unable to read config file '%s'",
@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_argc(argc, 0, 0);
if (!given_config_file && nongit)
die("not in a git directory");
+ if (given_config_blob)
+ die("editing blobs is not supported");
git_config(git_default_config, NULL);
launch_editor(given_config_file ?
given_config_file : git_path("config"),
@@ -457,6 +472,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
else if (actions == ACTION_SET) {
int ret;
+ check_blob_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
ret = git_config_set_in_file(given_config_file, argv[0], value);
@@ -466,18 +482,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)
return ret;
}
else if (actions == ACTION_SET_ALL) {
+ check_blob_write();
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_file,
argv[0], value, argv[2], 0);
}
else if (actions == ACTION_ADD) {
+ check_blob_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_file,
argv[0], value, "^$", 0);
}
else if (actions == ACTION_REPLACE_ALL) {
+ check_blob_write();
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_file,
@@ -500,6 +519,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
return get_value(argv[0], argv[1]);
}
else if (actions == ACTION_UNSET) {
+ check_blob_write();
check_argc(argc, 1, 2);
if (argc == 2)
return git_config_set_multivar_in_file(given_config_file,
@@ -509,12 +529,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
argv[0], NULL);
}
else if (actions == ACTION_UNSET_ALL) {
+ check_blob_write();
check_argc(argc, 1, 2);
return git_config_set_multivar_in_file(given_config_file,
argv[0], NULL, argv[1], 1);
}
else if (actions == ACTION_RENAME_SECTION) {
int ret;
+ check_blob_write();
check_argc(argc, 2, 2);
ret = git_config_rename_section_in_file(given_config_file,
argv[0], argv[1]);
@@ -525,6 +547,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
else if (actions == ACTION_REMOVE_SECTION) {
int ret;
+ check_blob_write();
check_argc(argc, 1, 1);
ret = git_config_rename_section_in_file(given_config_file,
argv[0], NULL);
diff --git a/cache.h b/cache.h
index a2621fa..9db5f74 100644
--- a/cache.h
+++ b/cache.h
@@ -1134,7 +1134,9 @@ extern int git_config_with_options(config_fn_t fn, void *,
extern int git_config_from_parameters(config_fn_t fn, void *data);
extern int git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
- const char *filename, int respect_includes);
+ const char *filename,
+ const char *blob_ref,
+ int respect_includes);
extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
extern int git_parse_ulong(const char *, unsigned long *);
extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index b8c8640..35aa8e2 100644
--- a/config.c
+++ b/config.c
@@ -1073,8 +1073,34 @@ int git_config_with_options(config_fn_t fn, void *data,
return ret == 0 ? found : ret;
}
+static int read_blob_reference(const char *ref,
+ struct strbuf *sb)
+{
+ unsigned char sha1[20];
+ enum object_type type;
+ void *buf;
+ unsigned long size;
+
+ if (get_sha1(ref, sha1) < 0)
+ return error("unable to resolve config blob '%s'", ref);
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf)
+ return error("unable to load config blob object '%s'", ref);
+ if (type != OBJ_BLOB) {
+ free(buf);
+ return error("reference '%s' does not point to a blob", ref);
+ }
+
+ /* read_sha1_file always NUL-terminates for us, so size+1 is OK */
+ strbuf_attach(sb, buf, size, size+1);
+ return 0;
+}
+
int git_config_with_options(config_fn_t fn, void *data,
- const char *filename, int respect_includes)
+ const char *filename,
+ const char *blob_ref,
+ int respect_includes)
{
char *repo_config = NULL;
int ret;
@@ -1093,6 +1119,15 @@ int git_config_with_options(config_fn_t fn, void *data,
*/
if (filename)
return git_config_from_file(fn, filename, data);
+ else if (blob_ref) {
+ struct strbuf buf = STRBUF_INIT;
+ if (read_blob_reference(blob_ref, &buf) < 0)
+ return -1;
+
+ ret = git_config_from_strbuf(fn, blob_ref, &buf, data);
+ strbuf_release(&buf);
+ return ret;
+ }
repo_config = git_pathdup("config");
ret = git_config_early(fn, data, repo_config);
@@ -1103,7 +1138,7 @@ int git_config(config_fn_t fn, void *data)
int git_config(config_fn_t fn, void *data)
{
- return git_config_with_options(fn, data, NULL, 1);
+ return git_config_with_options(fn, data, NULL, NULL, 1);
}
/*
^ permalink raw reply related
* Re: [PATCH v2 4/4] teach config parsing to read from strbuf
From: Jeff King @ 2013-03-14 7:39 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130314071046.GB6103@sigill.intra.peff.net>
On Thu, Mar 14, 2013 at 03:10:46AM -0400, Jeff King wrote:
> I looked into this a little. The first sticking point is that
> git_config_with_options needs to support the alternate source. Here's a
> sketch of what I think the solution should look like, on top of your
> patches.
Here it is again, with two changes:
1. Rather than handling the blob lookup inline in
git_config_with_options, it adds direct functions for reading
config from blob sha1s and blob references. I think this should
make it easier to reuse when you are trying to read .gitmodules
from C code.
2. It adds some basic tests.
I'll leave it here for tonight. The next step would be to rebase it on
your modified series (in particular, I think git_config_from_strbuf
should become git_config_from_buf, which will impact this).
Feel free to use, pick apart, rewrite, or discard as you see fit for
your series.
diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..8d01b7a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static const char *given_config_file;
static int use_global_config, use_system_config, use_local_config;
static const char *given_config_file;
+static const char *given_config_blob;
static int actions, types;
static const char *get_color_slot, *get_colorbool_slot;
static int end_null;
@@ -53,6 +54,7 @@ static struct option builtin_config_options[] = {
OPT_BOOLEAN(0, "system", &use_system_config, N_("use system config file")),
OPT_BOOLEAN(0, "local", &use_local_config, N_("use repository config file")),
OPT_STRING('f', "file", &given_config_file, N_("file"), N_("use given config file")),
+ OPT_STRING(0, "blob", &given_config_blob, N_("blob-id"), N_("read config from given blob object")),
OPT_GROUP(N_("Action")),
OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
@@ -218,7 +220,8 @@ static int get_value(const char *key_, const char *regex_)
}
git_config_with_options(collect_config, &values,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
ret = !values.nr;
@@ -302,7 +305,8 @@ static void get_color(const char *def_color)
get_color_found = 0;
parsed_color[0] = '\0';
git_config_with_options(git_get_color_config, NULL,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
if (!get_color_found && def_color)
color_parse(def_color, "command line", parsed_color);
@@ -330,7 +334,8 @@ static int get_colorbool(int print)
get_colorbool_found = -1;
get_diff_color_found = -1;
git_config_with_options(git_get_colorbool_config, NULL,
- given_config_file, respect_includes);
+ given_config_file, given_config_blob,
+ respect_includes);
if (get_colorbool_found < 0) {
if (!strcmp(get_colorbool_slot, "color.diff"))
@@ -348,6 +353,12 @@ static int get_colorbool(int print)
return get_colorbool_found ? 0 : 1;
}
+static void check_blob_write(void)
+{
+ if (given_config_blob)
+ die("writing config blobs is not supported");
+}
+
int cmd_config(int argc, const char **argv, const char *prefix)
{
int nongit = !startup_info->have_repository;
@@ -359,7 +370,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
builtin_config_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
- if (use_global_config + use_system_config + use_local_config + !!given_config_file > 1) {
+ if (use_global_config + use_system_config + use_local_config +
+ !!given_config_file + !!given_config_blob > 1) {
error("only one config file at a time.");
usage_with_options(builtin_config_usage, builtin_config_options);
}
@@ -438,6 +450,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_argc(argc, 0, 0);
if (git_config_with_options(show_all_config, NULL,
given_config_file,
+ given_config_blob,
respect_includes) < 0) {
if (given_config_file)
die_errno("unable to read config file '%s'",
@@ -450,6 +463,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
check_argc(argc, 0, 0);
if (!given_config_file && nongit)
die("not in a git directory");
+ if (given_config_blob)
+ die("editing blobs is not supported");
git_config(git_default_config, NULL);
launch_editor(given_config_file ?
given_config_file : git_path("config"),
@@ -457,6 +472,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
else if (actions == ACTION_SET) {
int ret;
+ check_blob_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
ret = git_config_set_in_file(given_config_file, argv[0], value);
@@ -466,18 +482,21 @@ int cmd_config(int argc, const char **argv, const char *prefix)
return ret;
}
else if (actions == ACTION_SET_ALL) {
+ check_blob_write();
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_file,
argv[0], value, argv[2], 0);
}
else if (actions == ACTION_ADD) {
+ check_blob_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_file,
argv[0], value, "^$", 0);
}
else if (actions == ACTION_REPLACE_ALL) {
+ check_blob_write();
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_file,
@@ -500,6 +519,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
return get_value(argv[0], argv[1]);
}
else if (actions == ACTION_UNSET) {
+ check_blob_write();
check_argc(argc, 1, 2);
if (argc == 2)
return git_config_set_multivar_in_file(given_config_file,
@@ -509,12 +529,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
argv[0], NULL);
}
else if (actions == ACTION_UNSET_ALL) {
+ check_blob_write();
check_argc(argc, 1, 2);
return git_config_set_multivar_in_file(given_config_file,
argv[0], NULL, argv[1], 1);
}
else if (actions == ACTION_RENAME_SECTION) {
int ret;
+ check_blob_write();
check_argc(argc, 2, 2);
ret = git_config_rename_section_in_file(given_config_file,
argv[0], argv[1]);
@@ -525,6 +547,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
else if (actions == ACTION_REMOVE_SECTION) {
int ret;
+ check_blob_write();
check_argc(argc, 1, 1);
ret = git_config_rename_section_in_file(given_config_file,
argv[0], NULL);
diff --git a/cache.h b/cache.h
index a2621fa..9db5f74 100644
--- a/cache.h
+++ b/cache.h
@@ -1134,7 +1134,9 @@ extern int git_config_with_options(config_fn_t fn, void *,
extern int git_config_from_parameters(config_fn_t fn, void *data);
extern int git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
- const char *filename, int respect_includes);
+ const char *filename,
+ const char *blob_ref,
+ int respect_includes);
extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
extern int git_parse_ulong(const char *, unsigned long *);
extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index b8c8640..5c492a8 100644
--- a/config.c
+++ b/config.c
@@ -1009,6 +1009,47 @@ int git_config_from_strbuf(config_fn_t fn, const char *name, struct strbuf *strb
return do_config_from_source(&top, fn, data);
}
+static int git_config_from_blob_sha1(config_fn_t fn,
+ const char *name,
+ const unsigned char *sha1,
+ void *data)
+{
+ struct strbuf sb = STRBUF_INIT;
+ enum object_type type;
+ void *buf;
+ unsigned long size;
+ int ret;
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf)
+ return error("unable to load config blob object '%s'", name);
+ if (type != OBJ_BLOB) {
+ free(buf);
+ return error("reference '%s' does not point to a blob", name);
+ }
+
+ /*
+ * read_sha1_file always NUL-terminates for us, so size+1 is OK;
+ * we could skip this step if we had git_config_from_buf
+ */
+ strbuf_attach(&sb, buf, size, size+1);
+ ret = git_config_from_strbuf(fn, name, &sb, data);
+ strbuf_release(&sb);
+
+ return ret;
+}
+
+static int git_config_from_blob_ref(config_fn_t fn,
+ const char *name,
+ void *data)
+{
+ unsigned char sha1[20];
+
+ if (get_sha1(name, sha1) < 0)
+ return error("unable to resolve config blob '%s'", name);
+ return git_config_from_blob_sha1(fn, name, sha1, data);
+}
+
const char *git_etc_gitconfig(void)
{
static const char *system_wide;
@@ -1074,7 +1115,9 @@ int git_config_with_options(config_fn_t fn, void *data,
}
int git_config_with_options(config_fn_t fn, void *data,
- const char *filename, int respect_includes)
+ const char *filename,
+ const char *blob_ref,
+ int respect_includes)
{
char *repo_config = NULL;
int ret;
@@ -1093,6 +1136,8 @@ int git_config_with_options(config_fn_t fn, void *data,
*/
if (filename)
return git_config_from_file(fn, filename, data);
+ else if (blob_ref)
+ return git_config_from_blob_ref(fn, blob_ref, data);
repo_config = git_pathdup("config");
ret = git_config_early(fn, data, repo_config);
@@ -1103,7 +1148,7 @@ int git_config(config_fn_t fn, void *data)
int git_config(config_fn_t fn, void *data)
{
- return git_config_with_options(fn, data, NULL, 1);
+ return git_config_with_options(fn, data, NULL, NULL, 1);
}
/*
diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
index e69de29..141ca21 100755
--- a/t/t1307-config-blob.sh
+++ b/t/t1307-config-blob.sh
@@ -0,0 +1,66 @@
+#!/bin/sh
+
+test_description='support for reading config from a blob'
+. ./test-lib.sh
+
+test_expect_success 'create config blob' '
+ cat >config <<-\EOF &&
+ [some]
+ value = 1
+ EOF
+ git add config &&
+ git commit -m foo
+'
+
+test_expect_success 'list config blob contents' '
+ echo some.value=1 >expect &&
+ git config --blob=HEAD:config --list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'fetch value from blob' '
+ echo true >expect &&
+ git config --blob=HEAD:config --bool some.value >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'reading from blob and file is an error' '
+ test_must_fail git config --blob=HEAD:config --system --list
+'
+
+test_expect_success 'reading from missing ref is an error' '
+ test_must_fail git config --blob=HEAD:doesnotexist --list
+'
+
+test_expect_success 'reading from non-blob is an error' '
+ test_must_fail git config --blob=HEAD --list
+'
+
+test_expect_success 'setting a value in a blob is an error' '
+ test_must_fail git config --blob=HEAD:config some.value foo
+'
+
+test_expect_success 'deleting a value in a blob is an error' '
+ test_must_fail git config --blob=HEAD:config --unset some.value
+'
+
+test_expect_success 'editing a blob is an error' '
+ test_must_fail git config --blob=HEAD:config --edit
+'
+
+test_expect_success 'parse errors in blobs are properly attributed' '
+ cat >config <<-\EOF &&
+ [some]
+ value = "
+ EOF
+ git add config &&
+ git commit -m broken &&
+
+ test_must_fail git config --blob=HEAD:config some.value 2>err &&
+
+ # just grep for our token as the exact error message is likely to
+ # change or be internationalized
+ grep "HEAD:config" err
+'
+
+test_done
^ permalink raw reply related
* Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree
From: John Keeping @ 2013-03-14 9:36 UTC (permalink / raw)
To: David Aguilar; +Cc: git, Junio C Hamano, Matt McClure, Tim Henigan
In-Reply-To: <CAJDDKr5M4pqmd9HSVT8hDJB9AxgV2RexN6B6v6ccTS6raWY_Qg@mail.gmail.com>
On Wed, Mar 13, 2013 at 08:41:29PM -0700, David Aguilar wrote:
> On Wed, Mar 13, 2013 at 1:33 PM, John Keeping <john@keeping.me.uk> wrote:
> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> > index eb1d3f8..8102ce1 100755
> > --- a/t/t7800-difftool.sh
> > +++ b/t/t7800-difftool.sh
> > @@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '
> > echo "$diff" | stdin_contains file
> > '
> >
> > +write_script .git/CHECK_SYMLINKS <<\EOF &&
>
> Tiny nit. Is there any downside to leaving this file
> at the root instead of inside the .git dir?
I followed what some of the other uses of write_script (in other tests)
did. I think putting it under .git is slightly better because it won't
show up as untracked in the repository but that shouldn't matter here,
so I'm happy to change it in a re-roll.
> > +#!/bin/sh
> > +test -L "$2/file" &&
> > +test -L "$2/file2" &&
> > +test -L "$2/sub/sub"
> > +echo $?
> > +EOF
> > +
> > +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> > + result=$(git difftool --dir-diff --symlink \
> > + --extcmd "./.git/CHECK_SYMLINKS" branch HEAD) &&
> > + test "$result" = 0
> > +'
> > +
>
> How about something like this?
>
> + echo 0 >expect &&
> + git difftool --dir-diff --symlink \
> + --extcmd ./CHECK_SYMLINKS branch HEAD >actual &&
> + test_cmp expect actual
>
> (sans gmail whitespace damage) so that we can keep it chained with &&.
I hadn't considered using test_cmp, if we go that way I wonder if we can
do slightly better for future debugging. Something like this perhaps?
+write_script .git/CHECK_SYMLINKS <<\EOF &&
+for f in file file2 sub/sub
+do
+ echo "$f"
+ readlink "$2/$f"
+done >actual
+EOF
+
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+ cat <<EOF >expect &&
+file
+$(pwd)/file
+file2
+$(pwd)/file2
+sub/sub
+$(pwd)/sub/sub
+EOF
+ git difftool --dir-diff --symlink \
+ --extcmd "./.git/CHECK_SYMLINKS" branch HEAD &&
+ test_cmp actual expect
+'
> Ah.. it seems your branch is based on master, perhaps?
>
> There's stuff cooking in next for difftool's tests.
> I'm not sure if this patch is based on top of them.
> Can you rebase the tests so that the chaining is done like it is in 'next'?
Yes it is based on master. The cleanup on next looks good, I'll base
the re-roll on that.
John
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: John Keeping @ 2013-03-14 9:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Aguilar, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <7v1ubj45ac.fsf@alter.siamese.dyndns.org>
On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote:
> Does the temporary checkout correctly apply the smudge filter and
> crlf conversion, by the way? If not, regardless of the topic in
> this thread, that may want to be fixed as well. I didn't check.
I've had a look at this and I think it will be much quicker for someone
more familiar with git-checkout-index to answer.
What git-difftool does is to create a temporary index containing only
the files that have changed (using git-update-index --index-info) and
then check this out with "git checkout-index --prefix=...". So I think
this question boils down to: does git-checkout-index still read
.gitattributes from the working tree if given --prefix?
John
^ permalink raw reply
* Re: [PATCH] status: hint the user about -uno if read_directory takes too long
From: Duy Nguyen @ 2013-03-14 10:22 UTC (permalink / raw)
To: Junio C Hamano, tboegi; +Cc: git, artagnon, robert.allan.zeh, finnag
In-Reply-To: <7vehfj46mu.fsf@alter.siamese.dyndns.org>
On Wed, Mar 13, 2013 at 10:21 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>> + statusUno::
>> + If collecting untracked files in linkgit:git-status[1]
>> + takes more than 2 seconds, hint the user that the option
>> + `-uno` could be used to stop collecting untracked files.
> Thanks, I like the idea
> could we make a "de-Luxe" version where
>
> statusUno is an integer, counting in milliseconds?
No problem.
On Wed, Mar 13, 2013 at 11:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The noise this introduces to the test suite is a bit irritating and
> makes us think twice if this really a good change.
I originally thought of two options, this or add an env flag in git
binary that turns this off in the test suite. The latter did not sound
good. But I forgot that we set a fake $HOME in the test suite, we
could disable this in $HOME/.gitconfig, less clutter in individual
tests.
>> static void wt_status_print_unmerged(struct wt_status *s)
>> + if (advice_status_uno && s->untracked_in_ms > 2000) {
>> + status_printf_ln(s, GIT_COLOR_NORMAL,
>> + _("It took %.2f seconds to collect untracked files."),
>> + (float)s->untracked_in_ms / 1000);
>> + status_printf_ln(s, GIT_COLOR_NORMAL,
>> + _("If it happens often, you may want to use option -uno"));
>> + status_printf_ln(s, GIT_COLOR_NORMAL,
>> + _("to speed up by stopping displaying untracked files"));
>> + }
>
> "to speed up by stopping displaying untracked files" does not look
> like giving a balanced suggestion. It is increasing the risk of
> forgetting about newly created files the user may want to add, but
> the risk is not properly warned.
How about "It took X ms to collect untracked files.\nCheck out the
option -u for a potential speedup"? I deliberately hide "no" so that
the user cannot blindly type and run it without reading document
first. We can give full explanation and warning there in the document.
> I tend to agree that the new advice would help users if phrased in a
> right way. Do we want them in COLOR_NORMAL, or do we want to make
> them stand out a bit more (do we have COLOR_BLINK ;-)?
There will be false positives (cold cache for example). So yeah
something more standing out is good but it should catch too much
attention. We're currently using red and green in status output. Maybe
this one can take blue.
PS. What about advertising index v4? I sent a patch some time ago to
put an advice in git-clone. I think it's a good place, but we could
place it somewhere else..
--
Duy
^ permalink raw reply
* Re: [RFC/PATCH] Documentation/technical/api-fswatch.txt: start with outline
From: Duy Nguyen @ 2013-03-14 10:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Karsten Blees, Ramkumar Ramachandra, Git List,
Torsten Bögershausen, Robert Zeh, Jeff King, Erik Faye-Lund,
Drew Northup
In-Reply-To: <7vtxof146d.fsf@alter.siamese.dyndns.org>
On Thu, Mar 14, 2013 at 2:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karsten Blees <karsten.blees@gmail.com> writes:
>
>> However, AFAIK inotify doesn't work recursively, so the daemon
>> would at least have to track the directory structure to be able to
>> register / unregister inotify handlers as directories come and go.
>
> Yes, and you would need one inotify per directory but you do not
> have an infinite supply of outstanding inotify watch (wasn't the
> limit like 8k per a single uid or something?), so the daemon must be
> prepared to say "I'll watch this, that and that directories, but the
> consumers should check other directories themselves."
Hey I did not know that. Webkit has about 6k leaf dirs and 182k files.
Watching the top N biggest directories would cover M% of cached files:
N M%
10 8.60
20 13.28
30 17.52
40 20.52
50 23.55
200 49.70
676 75.00
863 80.00
1486 90.00
So it's trade-off. We can cut some syscall cost off but we probably
need to pay some for inotify. And we definitely can't watch full
worktree. I don't know how costly it may be for watching many
directories. If it's not so costly, watching 256 or 512 dirs might be
enough.
What about Windows? Does the equivalent mechanism have similar limits?
> FWIW, I share your suspicion that an effort in the direction this
> thread suggests may end up duplicating what the caching vfs layer
> already does, and doing so poorly.
I'm still curious how it works out. Maybe it's not up to the original
expectation, but hopefully it will speed things up a bit.
--
Duy
^ permalink raw reply
* Re: Tag peeling peculiarities
From: Michael Haggerty @ 2013-03-14 11:28 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git discussion list
In-Reply-To: <20130314052448.GA2300@sigill.intra.peff.net>
On 03/14/2013 06:24 AM, Jeff King wrote:
> On Thu, Mar 14, 2013 at 05:41:58AM +0100, Michael Haggerty wrote:
>
>> Here is analysis of our options as I see them:
>>
>> 1. Accept that tags outside of refs/tags are not reliably advertised in
>> their peeled form. Document this deficiency and either:
>>
>> a. Don't even bother trying to peel refs outside of refs/tags (the
>> status quo).
>
> When you say "not reliably advertised" you mean from upload-pack, right?
> What about other callers? From my reading of peel_ref, anybody who calls
> it to get a packed ref may actually get a wrong answer. So this is not
> just about tag auto-following over fetch, but about other places, too
> (however, a quick grep does not make it look like this other places are
> all that commonly used).
Yes, this is a good point. I didn't audit all of the callers to see
whether any of them need 100% reliability, but I guess I should:
upload-pack.c:763: in send_ref(): This is the caller that we have been
talking about.
builtin/pack-objects.c:559: in mark_tagged(): This function is only
called for references under refs/tags.
builtin/pack-objects.c:2035: in add_ref_tag(): peel_ref() is called only
for refnames under refs/tags.
builtin/describe.c:147: in get_name(): peel_ref() is called only for
refnames under refs/tags.
builtin/show-ref.c:81: in show_ref(): this is broken too, as I showed in
my original email. This breakage was also introduced in 435c833.
Perhaps if peel_ref() *were* 100% reliable, we might be able to use it
to avoid object lookups in some other places.
> Another fun fact: upload-pack did not use peel_ref until recently
> (435c833, in v1.8.1). So while it is tempting to say "well, this was
> always broken, and nobody cared", it was not really; it is a fairly
> recent regression in 435c833.
I didn't realize that; thanks for pointing it out. Although peel_ref()
itself has been broken since it was introduced, at least in the sense
that it gives wrong answers for tags outside of refs/tags, prior to
435c833 it appears to only have been used for refs/tags.
>> I think the best option would be 1b. Even though there would never be a
>> guarantee that all peeled references are recorded and advertised
>> correctly, the behavior would asymptotically approach correctness as old
>> Git versions are retired, and the situations where it fails are probably
>> rare and no worse than the status quo.
>
> Thanks for laying out the options. I think 1b or 2c are the only sane
> paths forward. With either option, a newer writer produces something
> that all implementations, old and new, should get right, and that is
> primarily what we care about.
>
> So the only question is how much work we want to put into making sure
> the new reader handles the old writer correctly. Doing 2c is obviously
> more rigorous, and it is not that much work to add the fully-packed
> flag, but I kind of wonder if anybody even cares. We can just say "it's
> a bug fix; run `git pack-refs` again if you care" and call it a day
> (i.e., 1b).
>
> I could go either way.
On 03/14/2013 06:32 AM, Jeff King wrote:
> [...]
> So it's really not that much code. The bigger question is whether we
> want to have to carry the "fully-peeled" tag forever, and how other
> implementations would treat it.
I agree that 2c is also an attractive option. Its biggest advantage is
that it make peel_ref() reliable and therefore potentially usable for
other purposes. And probably the effort of carrying forward the
"fully-peeled" tag is no more work than the alternative of carrying
forward the knowledge that peel_ref() is unreliable.
Your patch looks about right aside from its lack of comments :-). I was
going to implement approximately the same thing in a patch series that I
am working on, but if you prefer then go ahead and submit your patch and
I will rebase my branch on top of it.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Fwd: Bug: git web--browse doesn't recognise browser on OS X
From: Timo Sand @ 2013-03-14 11:39 UTC (permalink / raw)
To: git
In-Reply-To: <CAMxBVSs6dJFnK78E2Da7t4V9ndJFRVDZEd1fR5QuCFz=u2Bnpw@mail.gmail.com>
Hi
I tried to open a website by runnin 'git web--browse http://google.com'
and it replied 'No known browser available'.
I also tried with '--browser=chrome' and '--browser=google-chrome' but
the responded with 'The browser chrome is not available as 'chrome'.'
I expected the command to open a new tab in my browser in each of the 3 tries.
This has worked for my system before.
OS X 10.8.2, git 1.8.2, Google Chrome 27.0.1438.7 dev
--
Timo Sand
timo.j.sand+sig@gmail.com
^ permalink raw reply
* [PATCH] git-tag: Allow --points-at syntax to create a tag pointing to specified commit
From: Michal Novotny @ 2013-03-14 12:30 UTC (permalink / raw)
To: git; +Cc: Michal Novotny
This patch adds the option to specify SHA-1 commit hash using
--points-at option of git tag to create a tag pointing to
the historical commit.
This was pretty easy in the past for the lightweight tags that are just simple
pointers (by creating .git/refs/tags/$tagname with SHA-1 hash) but it was not
possible for signed and annotated commits.
It's been tested for all of the tag types mentioned - lightweight tags, signed
tags and also annotated tags and everything is working fine in all scenarios.
Michal
Signed-off-by: Michal Novotny <minovotn@redhat.com>
---
builtin/tag.c | 32 ++++++++++++++++++++++++++------
sha1-lookup.h | 3 +++
2 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index f826688..f642acd 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -437,7 +437,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1, list = 0,
- delete = 0, verify = 0;
+ delete = 0, verify = 0, points_at_commit = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -521,8 +521,24 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("-n option is only allowed with -l."));
if (with_commit)
die(_("--contains option is only allowed with -l."));
- if (points_at.nr)
- die(_("--points-at option is only allowed with -l."));
+ if (points_at.nr) {
+ if (points_at.nr > 1)
+ die(_("--points-at option is only allowed with -l or a single "
+ "SHA-1 hash is allowed to create a tag to commit."));
+ else {
+ unsigned char *ref = points_at.sha1[0];
+
+ struct object *obj = parse_object(ref);
+ if ((obj != NULL) && (obj->type == OBJ_COMMIT)) {
+ memcpy(object, ref, 20);
+ points_at_commit = 1;
+ }
+ else
+ die(_("--points-at option points to an invalid commit"));
+
+ free(ref);
+ }
+ }
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
@@ -548,12 +564,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
tag = argv[0];
- object_ref = argc == 2 ? argv[1] : "HEAD";
if (argc > 2)
die(_("too many params"));
- if (get_sha1(object_ref, object))
- die(_("Failed to resolve '%s' as a valid ref."), object_ref);
+ /* Option --points-at option is setting this already */
+ if (!points_at_commit) {
+ object_ref = argc == 2 ? argv[1] : "HEAD";
+
+ if (get_sha1(object_ref, object))
+ die(_("Failed to resolve '%s' as a valid ref."), object_ref);
+ }
if (strbuf_check_tag_ref(&ref, tag))
die(_("'%s' is not a valid tag name."), tag);
diff --git a/sha1-lookup.h b/sha1-lookup.h
index 20af285..4eb03cf 100644
--- a/sha1-lookup.h
+++ b/sha1-lookup.h
@@ -1,6 +1,9 @@
#ifndef SHA1_LOOKUP_H
#define SHA1_LOOKUP_H
+unsigned char *translate_sha1_to_binary(const char *object_ref);
+int sha1_points_to_valid_commit(const char *object_ref);
+
typedef const unsigned char *sha1_access_fn(size_t index, void *table);
extern int sha1_pos(const unsigned char *sha1,
--
1.7.11.7
^ permalink raw reply related
* [PATCH v2] git-tag: Allow --points-at syntax to create a tag pointing to specified commit
From: Michal Novotny @ 2013-03-14 12:34 UTC (permalink / raw)
To: git; +Cc: Michal Novotny
This patch adds the option to specify SHA-1 commit hash using --points-at
option of git tag to create a tag pointing to a historical commit.
This was pretty easy in the past for the lightweight tags that are just simple
pointers (by creating .git/refs/tags/$tagname with SHA-1 hash) but it was not
possible for signed and annotated commits.
It's been tested for all of the tag types mentioned - lightweight tags, signed
tags and also annotated tags and everything is working fine in all scenarios
mentioned above.
Differences between v1 and v2 (this one):
- The bogus sha1-lookup.h hunk has been removed as it's not required and
I accidentally forgot to remove it before posting v1
Michal
Signed-off-by: Michal Novotny <minovotn@redhat.com>
---
builtin/tag.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index f826688..f642acd 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -437,7 +437,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1, list = 0,
- delete = 0, verify = 0;
+ delete = 0, verify = 0, points_at_commit = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -521,8 +521,24 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("-n option is only allowed with -l."));
if (with_commit)
die(_("--contains option is only allowed with -l."));
- if (points_at.nr)
- die(_("--points-at option is only allowed with -l."));
+ if (points_at.nr) {
+ if (points_at.nr > 1)
+ die(_("--points-at option is only allowed with -l or a single "
+ "SHA-1 hash is allowed to create a tag to commit."));
+ else {
+ unsigned char *ref = points_at.sha1[0];
+
+ struct object *obj = parse_object(ref);
+ if ((obj != NULL) && (obj->type == OBJ_COMMIT)) {
+ memcpy(object, ref, 20);
+ points_at_commit = 1;
+ }
+ else
+ die(_("--points-at option points to an invalid commit"));
+
+ free(ref);
+ }
+ }
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
@@ -548,12 +564,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
tag = argv[0];
- object_ref = argc == 2 ? argv[1] : "HEAD";
if (argc > 2)
die(_("too many params"));
- if (get_sha1(object_ref, object))
- die(_("Failed to resolve '%s' as a valid ref."), object_ref);
+ /* Option --points-at option is setting this already */
+ if (!points_at_commit) {
+ object_ref = argc == 2 ? argv[1] : "HEAD";
+
+ if (get_sha1(object_ref, object))
+ die(_("Failed to resolve '%s' as a valid ref."), object_ref);
+ }
if (strbuf_check_tag_ref(&ref, tag))
die(_("'%s' is not a valid tag name."), tag);
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH 2/2] add: respect add.updateroot config option
From: Matthieu Moy @ 2013-03-14 12:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, David Aguilar
In-Reply-To: <7vmwu747tf.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> It still has the same "the user saw
> it mentioned on stackoverflow and sets it without understanding that
> the behaviour is getting changed" effect.
I'm not so worried about this point, as the mechanism is temporary, and
it will take time before answers on stackoverflow reach the mass. The
initial state is that the option is not discoverable without reading the
documentation, and Jeff's documentation is very clear that this is
temporary. People reading the doc are usually not the kind of people
giving dumb answers on the web.
And even if some people set the option without understanding the
consequences, it's not that terrible. They'll just get the transition
period shortened.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [ANN] git-arr 0.11
From: Matthieu Moy @ 2013-03-14 12:43 UTC (permalink / raw)
To: Alberto Bertogli; +Cc: git
In-Reply-To: <20130313234143.GD14686@blitiri.com.ar>
Alberto Bertogli <albertito@blitiri.com.ar> writes:
> I wanted to let you know about git-arr, which is a git repository
> browser that can generate static HTML instead of having to run
> dynamically.
Can it run incrementally? I mean, if you have launched it for one
revision, does re-running it on the next revision regenerate the whole
set of pages, or does it only generate new pages?
If so, that could be a nice way to replace dynamic browsers for people
who do not have/want dynamic webpages, by just setting a hook or cron
job that generate the new pages when an update is made.
--
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