* Re: [PATCHv2] tag: add --points-at list option
From: Jeff King @ 2012-02-07 21:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
In-Reply-To: <7v1uq61jkz.fsf@alter.siamese.dyndns.org>
On Tue, Feb 07, 2012 at 12:20:44PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> I think the following would show the pointed at tag too.
> >> $ git tag my-v1.7.9 v1.7.9
> >> $ ./git-tag -l --points-at v1.7.9
> >> my-v1.7.9
> >> v1.7.9
> >>
> >> vs.
> >>
> >> $ ./git-tag -l --points-at v1.7.9
> >> my-v1.7.9
> >>
> >> I found that I had to filter matching refnames.
> >
> > Ah, so you are trying _not_ to show lightweight tags (I thought you
> > meant you also wanted to show them)? But I still don't see why the code
> > I posted before wouldn't work in that case. The "object" field of v1.7.9
> > is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
> > would not match.
>
> I think he is trying to avoid saying "v1.7.9 points at itself", and wants
> to know not just the value of $(rev-parse v1.7.9) but the refname.
Hmm. I read his example again, and now I'm even more confused.
If I give an object name to --points-at, should or should not a
lightweight tag pointing to that object be found?
If not, then I don't see how "git tag --points-at v1.7.9" would find
v1.7.9. Because we would use get_sha1 to parse "v1.7.9", returning the
sha1 of the tag object. And then when trying to match, we would look at
each tag object, find its "object" line, and compare that. In the case
of considering whether to show the v1.7.9 tag, we would be comparing the
sha1 of the commit that it points to to the actual tag sha1 itself, and
not match.
But in that case, nor would we match "my-v1.7.9" above, as it is a
lightweight tag that also points to v1.7.9's tag object.
If we _do_ want to match lightweight tags, then in the matching phase we
look for both the sha1 contained in the tag ref, as well as the sha1 of
the thing the tag points to (_if_ it is a tag object). In that case, we
would find both v1.7.9 and my-v1.7.9.
So I am not sure which is preferable. But I don't see how you could or
would want to distinguish the two tags above. They are functionally
identical, in that they are both refs pointing to the exact same tag
object. If the example had started with "git tag -s my-v1.7.9 v1.7.9"
then it would make more sense to me.
-Peff
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Tom Grennan @ 2012-02-07 22:08 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, jasampler
In-Reply-To: <20120207213012.GA5846@sigill.intra.peff.net>
On Tue, Feb 07, 2012 at 04:30:12PM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 12:20:44PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> >> I think the following would show the pointed at tag too.
>> >> $ git tag my-v1.7.9 v1.7.9
>> >> $ ./git-tag -l --points-at v1.7.9
>> >> my-v1.7.9
>> >> v1.7.9
>> >>
>> >> vs.
>> >>
>> >> $ ./git-tag -l --points-at v1.7.9
>> >> my-v1.7.9
>> >>
>> >> I found that I had to filter matching refnames.
>> >
>> > Ah, so you are trying _not_ to show lightweight tags (I thought you
>> > meant you also wanted to show them)? But I still don't see why the code
>> > I posted before wouldn't work in that case. The "object" field of v1.7.9
>> > is not the sha1 of the v1.7.9 tag object, but rather some commit, so it
>> > would not match.
>>
>> I think he is trying to avoid saying "v1.7.9 points at itself", and wants
>> to know not just the value of $(rev-parse v1.7.9) but the refname.
>
>Hmm. I read his example again, and now I'm even more confused.
>
>If I give an object name to --points-at, should or should not a
>lightweight tag pointing to that object be found?
>
>If not, then I don't see how "git tag --points-at v1.7.9" would find
>v1.7.9. Because we would use get_sha1 to parse "v1.7.9", returning the
>sha1 of the tag object. And then when trying to match, we would look at
>each tag object, find its "object" line, and compare that. In the case
>of considering whether to show the v1.7.9 tag, we would be comparing the
>sha1 of the commit that it points to to the actual tag sha1 itself, and
>not match.
>
>But in that case, nor would we match "my-v1.7.9" above, as it is a
>lightweight tag that also points to v1.7.9's tag object.
>
>If we _do_ want to match lightweight tags, then in the matching phase we
>look for both the sha1 contained in the tag ref, as well as the sha1 of
>the thing the tag points to (_if_ it is a tag object). In that case, we
>would find both v1.7.9 and my-v1.7.9.
>
>So I am not sure which is preferable. But I don't see how you could or
>would want to distinguish the two tags above. They are functionally
>identical, in that they are both refs pointing to the exact same tag
>object. If the example had started with "git tag -s my-v1.7.9 v1.7.9"
>then it would make more sense to me.
v1 and v2 wouldn't list lightweight tags of the points-at objects.
Both versions behave like this:
$ git tag my-lw-v1.7.9 v1.7.9
$ git tag my-a-v1.7.9 v1.7.9
$ git tag my-s-v1.7.9 v1.7.9
$ git tag -l --points-at v1.7.9
my-a-v1.7.9
my-s-v1.7.9
While addressing Junio's comments I realized that by first matching the
sha's and not refnames like the following will show LW tags too.
So, v3 will act like this:
$ git tag my-lw-v1.7.9 v1.7.9
$ git tag my-a-v1.7.9 v1.7.9
$ git tag my-s-v1.7.9 v1.7.9
$ git tag -l --points-at v1.7.9
my-lw-v1.7.9
my-a-v1.7.9
my-s-v1.7.9
Note, w/o strcmp(pa->refname, refname), this shows the points-at too:
$ git tag my-lw-v1.7.9 v1.7.9
$ git tag my-a-v1.7.9 v1.7.9
$ git tag my-s-v1.7.9 v1.7.9
$ git tag -l --points-at v1.7.9
my-lw-v1.7.9
my-a-v1.7.9
my-s-v1.7.9
v1.7.9
Which I don't think we'd want.
static struct points_at *match_points_at(struct points_at *points_at,
const char *refname,
const unsigned char *sha1)
{
struct object *obj;
struct points_at *pa;
const unsigned char *tagged_sha1;
/* First look for lightweight tags - those with matching sha's
* but different names */
for (pa = points_at; pa; pa = pa->next)
if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
return pa;
obj = parse_object(sha1);
if (!obj || obj->type != OBJ_TAG)
return 0;
tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
while (points_at && hashcmp(points_at->sha1, tagged_sha1))
points_at = points_at->next;
return points_at;
}
--
TomG
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Jeff King @ 2012-02-08 0:25 UTC (permalink / raw)
To: Tom Grennan; +Cc: Junio C Hamano, git, jasampler
In-Reply-To: <20120207220806.GD6264@tgrennan-laptop>
On Tue, Feb 07, 2012 at 02:08:06PM -0800, Tom Grennan wrote:
> v1 and v2 wouldn't list lightweight tags of the points-at objects.
> Both versions behave like this:
> $ git tag my-lw-v1.7.9 v1.7.9
> $ git tag my-a-v1.7.9 v1.7.9
> $ git tag my-s-v1.7.9 v1.7.9
> $ git tag -l --points-at v1.7.9
> my-a-v1.7.9
> my-s-v1.7.9
I assume the 2nd and 3rd line should be:
$ git tag -a my-a-v1.7.9 v1.7.9
$ git tag -s my-s-v1.7.9 v1.7.9
> static struct points_at *match_points_at(struct points_at *points_at,
> const char *refname,
> const unsigned char *sha1)
> {
> struct object *obj;
> struct points_at *pa;
> const unsigned char *tagged_sha1;
>
> /* First look for lightweight tags - those with matching sha's
> * but different names */
> for (pa = points_at; pa; pa = pa->next)
> if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
> return pa;
OK, I see what you are trying to accomplish here. But I really don't
like it. Two complaints:
1. Why is the name of the tag relevant? That is, if you are interested
in lightweight tags, and you have two tag refs, "refs/tags/a" and
"refs/tags/b", both pointing to the same tag object, then in what
situation is it useful to show "a" but not "b"?
It seems to me you would either want lightweight tags or not. And I
thought not, because the point of this was to reveal signatures or
annotations about a tag. Your my-lw-v1.7.9 says neither. Why do we
want to show it?
Also, it's not symmetric. What if I say "git tag
--points-at=my-lw-v1.7.9"? Then I would get your signed and
annotated tags (even though they're _not_ saying anything about
ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying
anything about it either; in fact, it's the opposite!).
2. I thought --points-at was about providing an object name. But it's
not. It's about providing a particular string. So with this code,
"git tag --points-at=v1.7.9" and "git tag --points-at=$(git
rev-parse v1.7.9)" are two different things. Which seems odd and
un-git-like to me.
Your documentation says "Only list annotated or signed tags of the
given object", which implies to me that --points-at is an arbitrary
object specifier, not a specific tagname.
It seems like your rationale is just avoiding a mention of v1.7.9
because, hey, it was obviously on the command line and the user isn't
interested in it. But I don't think that's true. The user asked for
every tag pointing to v1.7.9's object, and v1.7.9 is such a tag. It is
no more or less true for v1.7.9 than it is for my-lw-v1.7.9.
-Peff
^ permalink raw reply
* (unknown),
From: mstormo @ 2012-02-08 0:41 UTC (permalink / raw)
To: nicedomain,
3zrw4srakb-qiujkyozk-tuxkvremuumrk.iussyzuxsumsgor.ius, bounces,
hotfix, 3je_lsrakb0qiujkyozk-tuxkvr4muumrk.iussyzuxsumsgor.ius,
rapidrespond-k-f76139e66429a993, msysgit, git, lznuaa
[-- Attachment #1: HTML text --]
[-- Type: text/html, Size: 849 bytes --]
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 1:45 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, jasampler
In-Reply-To: <20120208002554.GA6035@sigill.intra.peff.net>
On Tue, Feb 07, 2012 at 07:25:54PM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 02:08:06PM -0800, Tom Grennan wrote:
>
>> v1 and v2 wouldn't list lightweight tags of the points-at objects.
>> Both versions behave like this:
>> $ git tag my-lw-v1.7.9 v1.7.9
>> $ git tag my-a-v1.7.9 v1.7.9
>> $ git tag my-s-v1.7.9 v1.7.9
>> $ git tag -l --points-at v1.7.9
>> my-a-v1.7.9
>> my-s-v1.7.9
>
>I assume the 2nd and 3rd line should be:
>
> $ git tag -a my-a-v1.7.9 v1.7.9
> $ git tag -s my-s-v1.7.9 v1.7.9
Yes
>> static struct points_at *match_points_at(struct points_at *points_at,
>> const char *refname,
>> const unsigned char *sha1)
>> {
>> struct object *obj;
>> struct points_at *pa;
>> const unsigned char *tagged_sha1;
>>
>> /* First look for lightweight tags - those with matching sha's
>> * but different names */
>> for (pa = points_at; pa; pa = pa->next)
>> if (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))
>> return pa;
>
>OK, I see what you are trying to accomplish here. But I really don't
>like it. Two complaints:
>
> 1. Why is the name of the tag relevant? That is, if you are interested
> in lightweight tags, and you have two tag refs, "refs/tags/a" and
> "refs/tags/b", both pointing to the same tag object, then in what
> situation is it useful to show "a" but not "b"?
Yes, I suppose this is more "tags or aliases of <object>" rather than
"tags that point at <object>".
> It seems to me you would either want lightweight tags or not. And I
> thought not, because the point of this was to reveal signatures or
> annotations about a tag. Your my-lw-v1.7.9 says neither. Why do we
> want to show it?
Initially I didn't care about listing these lightweight tags (aliases)
but now I see that this could be useful to find turds in refs/tags.
$ git tag my-v.1.7.9 v1.7.9
...
$ git tag -l --points-at v1.7.9
my-v.1.7.9
Oops
> Also, it's not symmetric. What if I say "git tag
> --points-at=my-lw-v1.7.9"? Then I would get your signed and
> annotated tags (even though they're _not_ saying anything about
> ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying
> anything about it either; in fact, it's the opposite!).
Huh? As you noted, the lightweight tag is just an alternate reference,
so why wouldn't want to see the annotated and signed tags of that common
object?
$ ./git-tag -l --points-at tomg-lw-v1.7.9
tomg-annotate-v1.7.9
tomg-signed-v1.7.9
v1.7.9
$ ./git-tag -l --points-at v1.7.9
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
> 2. I thought --points-at was about providing an object name. But it's
> not. It's about providing a particular string. So with this code,
> "git tag --points-at=v1.7.9" and "git tag --points-at=$(git
> rev-parse v1.7.9)" are two different things. Which seems odd and
> un-git-like to me.
Yep,
$ ./git-tag -l --points-at $(git rev-parse v1.7.9)
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
v1.7.9
> Your documentation says "Only list annotated or signed tags of the
> given object", which implies to me that --points-at is an arbitrary
> object specifier, not a specific tagname.
Yes, I changed that in the patch that I've prepared but will revert this
if you'd rather not list these lightweight tags.
>It seems like your rationale is just avoiding a mention of v1.7.9
>because, hey, it was obviously on the command line and the user isn't
>interested in it.
Yes, exactly.
>But I don't think that's true. The user asked for every tag pointing to
>v1.7.9's object, and v1.7.9 is such a tag. It is no more or less true
>for v1.7.9 than it is for my-lw-v1.7.9.
My reaction when I tested this was, "don't tell me what I already know."
But consistency with $(git rev-parse ...) seems more important.
And as you noted, a sha1_array would save code and to me, less code is
always better.
Thanks,
TomG
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Nguyen Thai Ngoc Duy @ 2012-02-08 4:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vlioe1o1g.fsf@alter.siamese.dyndns.org>
2012/2/8 Junio C Hamano <gitster@pobox.com>:
> Jeff King <peff@peff.net> writes:
>
>>> > +A config callback should return 0 for success, or -1 if the variable
>>> > +could not be parsed properly.
>>>
>>> This matches what I have always thought, but I think I recently saw a
>>> series that adds callbacks that return 1 to mean "I have understood this
>>> variable, so callers should not look at it any more". It felt wrong, but
>>> I did not find anything in the config.c API framework to prvent such a
>>> local calling convention.
>>
>> ...
>> The "1 means I understood this" convention is used by userdiff_config. I
>> don't like that it is unlike every other config callback,...
>> Looking at the code again, though, ...
>> Hmm. Yeah. The userdiff calling convention dates back to late 2008....
>> So I think we could go back and simplify the userdiff_config code now.
>
> I remembered where I saw the new "offender"; it was nd/columns
> topic (Cc'ing Nguyễn).
nd/columns does use "1" convention in git_column_config(), but the
direct config callback function does not return 1 to config machinery.
All call sites follow this pattern:
int ret = git_column_config(key, var, "command", &colopts);
if (ret <= 0) return ret;
I think it's ok.
--
Duy
^ permalink raw reply
* Re: [PATCH v2 0/4] Deprecate "not allow as-is commit with i-t-a entries"
From: Nguyen Thai Ngoc Duy @ 2012-02-08 4:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7v8vke38a1.fsf@alter.siamese.dyndns.org>
2012/2/7 Junio C Hamano <gitster@pobox.com>:
> But when I said "let's admit that this is just fixing an UI mistake, no
> configuration, no options", I really meant it. Without the backward
> compatiblity "For now please do not fix this bug for me and keep being
> buggy until I get used to the non-buggy behaviour" fuss, which we never do
> to any bugfix.
Ahh I missed something again. Your patch looks good too.
> That is how we are planning to handle "git merge" update to spawn editor
> in interactive session in the next release. There is no "Please keep the
> buggy behaviour" option; only an environment variable to help when we
> mistake a scripted use as interactive, whose support is not going away
> because it is not about "until I get used to the new behaviour".
--
Duy
^ permalink raw reply
* git-subtree Ready for Inspection
From: David A. Greene @ 2012-02-08 3:49 UTC (permalink / raw)
To: git
I've put up a branch containing git-subtree at:
gitolite@sources.obbligato.org:git.git
The branch is called, uncreatively, "subtree." It should have open read
access. Please let me know if there's any trouble accessing it.
It includes the git-subtree history imported via git-subtree followed by
a number of changes to bring it in line with the current git practices,
as best I can determine at the moment.
Since this includes the entire history of git-subtree I don't know that
sending patches to the mailing list is helpful. Can someone fetch the
branch and take a look? I want to ensure I didn't do anything
egregiously terrible and would like feedback on any needed changes.
If the community wants the patches I added on top of the existing
git-subtree history sent to the list, I can certainly do that.
Thanks!
-Dave
^ permalink raw reply
* [PATCHv3] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 6:21 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
In-Reply-To: <20120208002554.GA6035@sigill.intra.peff.net>
Please see version 3 of the "points-at" feature. In addition to addressing
the comments on v2, this now lists lightweight tags to the given object.
Tom Grennan (1):
tag: add --points-at list option
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 51 insertions(+), 4 deletions(-)
--
1.7.8
^ permalink raw reply
* [PATCHv3] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 6:21 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
In-Reply-To: <20120208002554.GA6035@sigill.intra.peff.net>
This filters the list for tags of the given object.
Example,
john$ git tag v1.0-john v1.0
john$ git tag -l --points-at v1.0
v1.0-john
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5ead91e..124ed36 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
<tagname> [<commit> | <object>]
'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
[--column[=<options>] | --no-column] [<pattern>...]
'git tag' -v <tagname>...
@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.
--contains <commit>::
Only list tags which contain the specified commit.
+--points-at <object>::
+ Only list tags of the given object.
+
-m <msg>::
--message=<msg>::
Use the given tag message (instead of prompting).
diff --git a/builtin/tag.c b/builtin/tag.c
index 5fbd62c..c5da622 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -16,11 +16,13 @@
#include "revision.h"
#include "gpg-interface.h"
#include "column.h"
+#include "sha1-array.h"
static const char * const git_tag_usage[] = {
"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
"git tag -d <tagname>...",
- "git tag -l [-n[<num>]] [<pattern>...]",
+ "git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\"
+ "\n\t\t[<pattern>...]",
"git tag -v <tagname>...",
NULL
};
@@ -31,6 +33,7 @@ struct tag_filter {
struct commit_list *with_commit;
};
+static struct sha1_array points_at;
static unsigned int colopts;
static int match_pattern(const char **patterns, const char *ref)
@@ -44,6 +47,22 @@ static int match_pattern(const char **patterns, const char *ref)
return 0;
}
+static const unsigned char *match_points_at(const unsigned char *sha1)
+{
+ int i;
+ const unsigned char *tagged_sha1 = (unsigned char*)"";
+ struct object *obj = parse_object(sha1);
+
+ if (obj && obj->type == OBJ_TAG)
+ tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+ for (i = 0; i < points_at.nr; i++)
+ if (!hashcmp(points_at.sha1[i], sha1))
+ return sha1;
+ else if (!hashcmp(points_at.sha1[i], tagged_sha1))
+ return tagged_sha1;
+ return NULL;
+}
+
static int in_commit_list(const struct commit_list *want, struct commit *c)
{
for (; want; want = want->next)
@@ -141,6 +160,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
+ if (points_at.nr && !match_points_at(sha1))
+ return 0;
+
if (!filter->lines) {
printf("%s\n", refname);
return 0;
@@ -389,6 +411,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
return check_refname_format(sb->buf, 0);
}
+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+ const char *arg, int unset)
+{
+ unsigned char sha1[20];
+
+ if (unset) {
+ sha1_array_clear(&points_at);
+ return 0;
+ }
+ if (!arg)
+ return error(_("switch 'points-at' requires an object"));
+ if (get_sha1(arg, sha1))
+ return error(_("malformed object name '%s'"), arg);
+ sha1_array_append(&points_at, sha1);
+ return 0;
+}
+
int cmd_tag(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
@@ -432,6 +471,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
PARSE_OPT_LASTARG_DEFAULT,
parse_opt_with_commit, (intptr_t)"HEAD",
},
+ {
+ OPTION_CALLBACK, 0, "points-at", NULL, "object",
+ "print only tags of the object", 0, parse_opt_points_at
+ },
OPT_END()
};
@@ -478,8 +521,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
}
if (lines != -1)
die(_("-n option is only allowed with -l."));
- if (with_commit)
- die(_("--contains option is only allowed with -l."));
+ if (with_commit || points_at.nr)
+ die(_("--contains and --points-at options "
+ "are only allowed with -l."));
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Junio C Hamano @ 2012-02-08 6:40 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Jeff King, git
In-Reply-To: <CACsJy8AU3ZA1=Q3vahhP6Nr=FZNKd7oRJ04mtKVs+uvNqJeVaw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>> The "1 means I understood this" convention is used by userdiff_config. I
>>> don't like that it is unlike every other config callback,...
>>> Looking at the code again, though, ...
>>> Hmm. Yeah. The userdiff calling convention dates back to late 2008....
>>> So I think we could go back and simplify the userdiff_config code now.
>>
>> I remembered where I saw the new "offender"; it was nd/columns
>> topic (Cc'ing Nguyễn).
>
> nd/columns does use "1" convention in git_column_config(), but the
> direct config callback function does not return 1 to config machinery.
> All call sites follow this pattern:
>
> int ret = git_column_config(key, var, "command", &colopts);
> if (ret <= 0) return ret;
>
> I think it's ok.
I too think this should be acceptable, but that is not the point.
Your excuse that "the toplevel callback in my callchain never returns 1,
so overall the nd/columns series is ok" just muddies the water. It means
if later somebody wanted to use inner callback functions you use from the
git_column_config() callchain chain as a toplevel callback for whatever
reason, that will violate the "0 for success, or -1 for error" convention.
More importantly, if somebody wants to turn a top-level callback that
currently returns 0 into a sub callback used by his callback callchain, he
cannot change that existing callback to return 1 to tell him to short
circuit, because for other callers returning 1 would be a violation.
What I was getting at is that we probably should officially declare that
returning 1 to signal success is perfectly acceptable (and it probably
should mean the caller who called the callback function as a sub callback
should return immediately, taking it as a signal that the key has already
been handled), as the primary purpose of this thread to discuss Peff's
patch is to write these rules down.
Of course, all that relies on the audit of the git_config() machinery. I
think it is written to accept non-negative as success, and that is why I
said "I too think this should be acceptable" in the first place.
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Nguyen Thai Ngoc Duy @ 2012-02-08 6:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vmx8tzv3l.fsf@alter.siamese.dyndns.org>
On Wed, Feb 8, 2012 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Your excuse that "the toplevel callback in my callchain never returns 1,
> so overall the nd/columns series is ok" just muddies the water. It means
> if later somebody wanted to use inner callback functions you use from the
> git_column_config() callchain chain as a toplevel callback for whatever
> reason, that will violate the "0 for success, or -1 for error" convention.
> More importantly, if somebody wants to turn a top-level callback that
> currently returns 0 into a sub callback used by his callback callchain, he
> cannot change that existing callback to return 1 to tell him to short
> circuit, because for other callers returning 1 would be a violation.
>
> What I was getting at is that we probably should officially declare that
> returning 1 to signal success is perfectly acceptable (and it probably
> should mean the caller who called the callback function as a sub callback
> should return immediately, taking it as a signal that the key has already
> been handled), as the primary purpose of this thread to discuss Peff's
> patch is to write these rules down.
Or allow multiple callbacks from git_config(). The problem with is it
adds another common set of config vars. Now it's common to chain var
sets by doing
int config_callback(...)
{
....
return yet_another_callback(...);
}
int main()
{
git_config(config_callback, ...)
...
where yet_another_callback() hard codes another callback (usually
git_default_config). This is inflexible. If we allow multiple
callbacks, git_column_config() could be changed to return just 0 or -1
(i.e. 1 remains unsuccessful error code).
On second thought, I think calling git_config() multiple times, each
time with one callback, may work too. We may want to cache config
content to avoid accessing files too many times though.
> Of course, all that relies on the audit of the git_config() machinery. I
> think it is written to accept non-negative as success, and that is why I
> said "I too think this should be acceptable" in the first place.
--
Duy
^ permalink raw reply
* [StGit PATCH] Parse commit object header correctly
From: Junio C Hamano @ 2012-02-08 7:33 UTC (permalink / raw)
To: Karl Hasselström, Catalin Marinas
Cc: Andy Green (林安廸), git
In-Reply-To: <7vvcni1r5u.fsf@alter.siamese.dyndns.org>
To allow parsing the header produced by versions of Git newer than the
code written to parse it, all commit parsers are expected to skip unknown
header lines, so that newer types of header lines can be added safely.
The only three things that are promised are:
(1) the header ends with an empty line (just an LF, not "a blank line"),
(2) unknown lines can be skipped, and
(3) a header "field" begins with the field name, followed by a single SP
followed by the value.
The parser used by StGit, introduced by commit cbe4567 (New StGit core
infrastructure: repository operations, 2007-12-19), was accidentally a bit
too loose to lose information, and a bit too strict to raise exception
when dealing with a line it does not understand.
- It used "strip()" to lose whitespaces from both ends, risking a line
with only whitespaces to be mistaken as the end of the header.
- It used "k, v = line.split(None, 1)", blindly assuming that all header
lines (including the ones that the version of StGit may not understand)
can safely be split without raising an exception, which is not true if
there is no SP on the line.
This patch changes the parsing logic so that it:
(1) detects end of the hedaer correctly by treating only an empty line as
such;
(2) handles multi-line fields (a header line that begins with a single SP
is appended to the previous line after removing that leading SP but
retaining the LF between the line and the previous line) correctly;
(3) splits a line at the first SP to find the field name, but only does
so when there actually is SP on the line; and
(4) ignores lines that cannot be understood without barfing.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Earlier I sent a minimum parser fix that ignores multi-line fields, as
the fields StGit cares about are all single line. This patch also
teaches multi-line fields to the parser, so that later versions of
StGit can parse and use them if they choose to.
Python is not my primary language, so please take this with a grain of
salt.
Thanks.
stgit/lib/git.py | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 56287f6..f19371b 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -390,21 +390,34 @@ class CommitData(Immutable, Repr):
@return: A new L{CommitData} object
@rtype: L{CommitData}"""
cd = cls(parents = [])
- lines = list(s.splitlines(True))
+ raw_lines = list(s.splitlines(True))
+ lines = []
+ # Collapse multi-line header lines
+ for i in xrange(len(raw_lines)):
+ line = raw_lines[i]
+ if line == '\n':
+ cd.set_message(''.join(raw_lines[i+1:]))
+ break
+ if line[0] == ' ':
+ # continuation line
+ lines[-1] += '\n' + line[1:]
+ else:
+ lines.append(line)
for i in xrange(len(lines)):
- line = lines[i].strip()
- if not line:
- return cd.set_message(''.join(lines[i+1:]))
- key, value = line.split(None, 1)
- if key == 'tree':
- cd = cd.set_tree(repository.get_tree(value))
- elif key == 'parent':
- cd = cd.add_parent(repository.get_commit(value))
- elif key == 'author':
- cd = cd.set_author(Person.parse(value))
- elif key == 'committer':
- cd = cd.set_committer(Person.parse(value))
- assert False
+ line = lines[i].rstrip('\n')
+ ix = line.find(' ')
+ if 0 <= ix:
+ key, value = line[0:ix], line[ix+1:]
+ if key == 'tree':
+ cd = cd.set_tree(repository.get_tree(value))
+ elif key == 'parent':
+ cd = cd.add_parent(repository.get_commit(value))
+ elif key == 'author':
+ cd = cd.set_author(Person.parse(value))
+ elif key == 'committer':
+ cd = cd.set_committer(Person.parse(value))
+ return cd
+
class Commit(GitObject):
"""Represents a git commit object. All the actual data contents of the
^ permalink raw reply related
* Re: filter-branch ignoring gitattributes?
From: norbert.nemec @ 2012-02-08 8:23 UTC (permalink / raw)
To: git
In-Reply-To: <jgglh9$mrv$1@dough.gmane.org>
Meanwhile, I can answer my own question:
The "text" attribute works similar to a filter.
As with filters in general, it is only applied when git detects a
modification in the file. A file change is detected by the file
modification time on disc. To reread all files from disk and re-apply
the filters to all of them, you would typically delete the .git/index
file. In filter-branch, this is not possible, so instead you have to
"touch" all files in the --tree-filter command.
Am 03.02.12 13:55, schrieb norbert.nemec:
> Hi there,
>
> it seems that 'git filter-branch' ignores the setting in
> .git/info/attributes - is that correct?
>
> Does .gitattributes work reliably?
>
> Specifically, I tried
>
> git filter-branch \
> --tree-filter 'echo "* text=auto" > .gitattributes' \
> --tag-name-filter cat
> --prune-empty -- --all
>
> It seems to work, but I am not sure whether I missed anything.
>
> Greetings,
> Norbert
>
^ permalink raw reply
* Re: [StGit PATCH] Parse commit object header correctly
From: Michael Haggerty @ 2012-02-08 10:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Karl Hasselström, Catalin Marinas,
"Andy Green (林安廸)", git
In-Reply-To: <7vd39pzsmq.fsf_-_@alter.siamese.dyndns.org>
On 02/08/2012 08:33 AM, Junio C Hamano wrote:
> To allow parsing the header produced by versions of Git newer than the
> code written to parse it, all commit parsers are expected to skip unknown
> header lines, so that newer types of header lines can be added safely.
> The only three things that are promised are:
>
> (1) the header ends with an empty line (just an LF, not "a blank line"),
> (2) unknown lines can be skipped, and
> (3) a header "field" begins with the field name, followed by a single SP
> followed by the value.
>
> The parser used by StGit, introduced by commit cbe4567 (New StGit core
> infrastructure: repository operations, 2007-12-19), was accidentally a bit
> too loose to lose information, and a bit too strict to raise exception
> when dealing with a line it does not understand.
>
> - It used "strip()" to lose whitespaces from both ends, risking a line
> with only whitespaces to be mistaken as the end of the header.
>
> - It used "k, v = line.split(None, 1)", blindly assuming that all header
> lines (including the ones that the version of StGit may not understand)
> can safely be split without raising an exception, which is not true if
> there is no SP on the line.
>
> This patch changes the parsing logic so that it:
>
> (1) detects end of the hedaer correctly by treating only an empty line as
> such;
> (2) handles multi-line fields (a header line that begins with a single SP
> is appended to the previous line after removing that leading SP but
> retaining the LF between the line and the previous line) correctly;
> (3) splits a line at the first SP to find the field name, but only does
> so when there actually is SP on the line; and
> (4) ignores lines that cannot be understood without barfing.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * Earlier I sent a minimum parser fix that ignores multi-line fields, as
> the fields StGit cares about are all single line. This patch also
> teaches multi-line fields to the parser, so that later versions of
> StGit can parse and use them if they choose to.
>
> Python is not my primary language, so please take this with a grain of
> salt.
>
> Thanks.
>
> stgit/lib/git.py | 41 +++++++++++++++++++++++++++--------------
> 1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/stgit/lib/git.py b/stgit/lib/git.py
> index 56287f6..f19371b 100644
> --- a/stgit/lib/git.py
> +++ b/stgit/lib/git.py
> @@ -390,21 +390,34 @@ class CommitData(Immutable, Repr):
> @return: A new L{CommitData} object
> @rtype: L{CommitData}"""
> cd = cls(parents = [])
> - lines = list(s.splitlines(True))
> + raw_lines = list(s.splitlines(True))
str.splitlines() splits lines at any EOL pattern ('\n', '\r\n' or '\r'
alone). If you want to be sure to split only on '\n', I think the
simplest alternative is
raw_lines = s.split('\n')
str.split() and str.splitlines() already return lists, so it is not
necessary to wrap the result in list().
But please note that str.split() discards the split characters, and if
the last character in the string is '\n' then the last string in the
result list is the empty string.
> + lines = []
> + # Collapse multi-line header lines
> + for i in xrange(len(raw_lines)):
> + line = raw_lines[i]
The two previous lines can be written
for (i, line) in enumerate(raw_lines):
> + if line == '\n':
> + cd.set_message(''.join(raw_lines[i+1:]))
> + break
> + if line[0] == ' ':
> + # continuation line
> + lines[-1] += '\n' + line[1:]
In your original version, lines[-1] would already be LF-terminated, so
this line would create a double-LF in the string.
> + else:
> + lines.append(line)
> for i in xrange(len(lines)):
> - line = lines[i].strip()
> - if not line:
> - return cd.set_message(''.join(lines[i+1:]))
> - key, value = line.split(None, 1)
> - if key == 'tree':
> - cd = cd.set_tree(repository.get_tree(value))
> - elif key == 'parent':
> - cd = cd.add_parent(repository.get_commit(value))
> - elif key == 'author':
> - cd = cd.set_author(Person.parse(value))
> - elif key == 'committer':
> - cd = cd.set_committer(Person.parse(value))
> - assert False
> + line = lines[i].rstrip('\n')
> + ix = line.find(' ')
> + if 0 <= ix:
> + key, value = line[0:ix], line[ix+1:]
The above five lines can be written
for line in lines:
if ' ' in line:
key, value = line.rstrip('\n').split(' ', 1)
or (if the lack of a space should be treated more like an exception)
for line in lines:
try:
key, value = line.rstrip('\n').split(' ', 1)
except ValueError:
continue
> + if key == 'tree':
> + cd = cd.set_tree(repository.get_tree(value))
> + elif key == 'parent':
> + cd = cd.add_parent(repository.get_commit(value))
> + elif key == 'author':
> + cd = cd.set_author(Person.parse(value))
> + elif key == 'committer':
> + cd = cd.set_committer(Person.parse(value))
All in all, I would recommend something like (untested):
@return: A new L{CommitData} object
@rtype: L{CommitData}"""
cd = cls(parents = [])
lines = []
raw_lines = s.split('\n')
# Collapse multi-line header lines
for i, line in enumerate(raw_lines):
if not line:
cd.set_message('\n'.join(raw_lines[i+1:]))
break
if line.startswith(' '):
# continuation line
lines[-1] += '\n' + line[1:]
else:
lines.append(line)
for line in lines:
if ' ' in line:
key, value = line.split(' ', 1)
if key == 'tree':
cd = cd.set_tree(repository.get_tree(value))
elif key == 'parent':
cd = cd.add_parent(repository.get_commit(value))
elif key == 'author':
cd = cd.set_author(Person.parse(value))
elif key == 'committer':
cd = cd.set_committer(Person.parse(value))
return cd
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: cloning a tree which has detached branch checked out
From: Michael S. Tsirkin @ 2012-02-08 10:17 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, Jakub Narebski, git
In-Reply-To: <20120207171604.GA26614@sigill.intra.peff.net>
On Tue, Feb 07, 2012 at 12:16:04PM -0500, Jeff King wrote:
> On Tue, Feb 07, 2012 at 09:11:00AM -0800, Junio C Hamano wrote:
>
> > > This particular bug should have been fixed before that, even, with my
> > > c1921c1 (clone: always fetch remote HEAD, 2011-06-07). And it is tested
> > > explicitly in t5707,...
> > [...]
> > What is funny is "error: Trying to write ref HEAD with nonexistant object".
> > "git grep -e nonexistant f3fb0" does not register any hit.
>
> That misspelling of "nonexistent" was fixed by 7be8b3b (Fix typo:
> existant->existent, 2011-06-16), around the same time as my clone patch.
> Which really makes me wonder if the OP is accidentally running an old
> version of git during the tests.
Double checked and you are right: the box actually runs git 1.7.1.
Rechecked with 1.7.9.GIT and it behaves correctly.
My mistake, sorry about the noise.
--
MST
^ permalink raw reply
* Re: [StGit PATCH] Parse commit object header correctly
From: Frans Klaver @ 2012-02-08 10:43 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Karl Hasselström, Catalin Marinas,
"Andy Green (林安廸)", git
In-Reply-To: <4F3247CA.1020904@alum.mit.edu>
On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/08/2012 08:33 AM, Junio C Hamano wrote:
>
>> (1) detects end of the hedaer correctly by treating only an empty line as
>> such;
s/hedaer/header/;
>> + line = lines[i].rstrip('\n')
>> + ix = line.find(' ')
>> + if 0 <= ix:
>> + key, value = line[0:ix], line[ix+1:]
>
> The above five lines can be written
>
> for line in lines:
> if ' ' in line:
> key, value = line.rstrip('\n').split(' ', 1)
>
> or (if the lack of a space should be treated more like an exception)
>
> for line in lines:
> try:
> key, value = line.rstrip('\n').split(' ', 1)
> except ValueError:
> continue
This is generally considered more pythonic: "It's easier to ask for
forgiveness than to get permission".
>
>> + if key == 'tree':
>> + cd = cd.set_tree(repository.get_tree(value))
>> + elif key == 'parent':
>> + cd = cd.add_parent(repository.get_commit(value))
>> + elif key == 'author':
>> + cd = cd.set_author(Person.parse(value))
>> + elif key == 'committer':
>> + cd = cd.set_committer(Person.parse(value))
>
> All in all, I would recommend something like (untested):
>
> @return: A new L{CommitData} object
> @rtype: L{CommitData}"""
> cd = cls(parents = [])
> lines = []
> raw_lines = s.split('\n')
> # Collapse multi-line header lines
> for i, line in enumerate(raw_lines):
> if not line:
> cd.set_message('\n'.join(raw_lines[i+1:]))
> break
> if line.startswith(' '):
> # continuation line
> lines[-1] += '\n' + line[1:]
> else:
> lines.append(line)
>
> for line in lines:
> if ' ' in line:
> key, value = line.split(' ', 1)
> if key == 'tree':
> cd = cd.set_tree(repository.get_tree(value))
> elif key == 'parent':
> cd = cd.add_parent(repository.get_commit(value))
> elif key == 'author':
> cd = cd.set_author(Person.parse(value))
> elif key == 'committer':
> cd = cd.set_committer(Person.parse(value))
> return cd
One could also take the recommended python approach for
switch/case-like if/elif/else statements:
updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),
'parent': lambda cd, value:
cd.add_parent(repository.get_commit(value)),
'author': lambda cd, value: cd.set_author(Person.parse(value)),
'committer': lambda cd, value:
cd.set_committer(Person.parse(value))
}
for line in lines:
try:
key, value = line.split(' ', 1)
cd = updater[key](cd, value)
except ValueError:
continue
except KeyError:
continue
It documents about the same, but adds checking on double 'case'
statements. The resulting for loop is rather cleaner and the exception
approach becomes even more logical. I rather like the result, but I
guess it's mostly a matter of taste.
Cheers,
Frans
^ permalink raw reply
* [PATCH v2] Explicitly set X to avoid potential build breakage
From: Michael @ 2012-02-08 10:59 UTC (permalink / raw)
To: git
$X is appended to binary names for Windows builds (ie. git.exe).
Pollution from the environment can inadvertently trigger this behaviour,
resulting in 'git' turning into 'gitwhatever' without warning.
Signed-off-by: Michael Palimaka <kensington@astralcloak.net>
---
Makefile | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/Makefile b/Makefile
index 87fb30a..b21d40c 100644
--- a/Makefile
+++ b/Makefile
@@ -460,6 +460,9 @@ PROGRAM_OBJS += http-backend.o
PROGRAM_OBJS += sh-i18n--envsubst.o
PROGRAM_OBJS += credential-store.o
+# Binary suffix used for Windows builds
+X =
+
PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
--
1.7.8.4
^ permalink raw reply related
* Re: git-subtree Ready for Inspection
From: Jan Krüger @ 2012-02-08 12:35 UTC (permalink / raw)
To: David A. Greene; +Cc: git
In-Reply-To: <87r4y6ougl.fsf@smith.obbligato.org>
Hi Dave,
On 02/08/2012 04:49 AM, David A. Greene wrote:
> I've put up a branch containing git-subtree at:
>
> gitolite@sources.obbligato.org:git.git
A publicly accessible URL would be much more helpful. :-)
-Jan
^ permalink raw reply
* [PATCH] gitweb: Harden parse_commit and parse_commits
From: Jakub Narebski @ 2012-02-08 15:04 UTC (permalink / raw)
To: rajesh boyapati; +Cc: git
In-Reply-To: <201202071753.12436.jnareb@gmail.com>
On Tue, 7 Feb 2012, Jakub Narebski wrote:
> On Mon, 6 Feb 2012, rajesh boyapati wrote:
[...]
> > Then, I restarted gerrit server to take changes.
> > Now the error log of gerrit shows:
>
> > [2012-02-06 11:21:46,726] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-06 11:21:49,167] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Mon Feb 6 11:21:49
> > 2012] gitweb.cgi: Use of uninitialized value $commit_id in open at
> > /usr/lib/cgi-bin/gitweb.cgi line 2817.
> > [2012-02-06 11:21:49,169] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision ''
> [the same errors repeated few times]
>
> > <<<<<<<<<<<<<<<<
> > Previously, there is a error showing at line 4720. Now, with this patch,
> > that error has gone.
>
> As I said I was able to find a fix only for part of the issue.
> Unfortunately I was not able to reproduce this error in this form.
> Note that the error location doesn't help much, because it is more
> interesting for find which callers of parse_commits() pass undefined
> $commit_id.
>
> I can try to harden parse_commits() against bogus parameters; maybe
> this would help.
Does the following patch help, and does it fix the issue?
(Nb. you can try to simply change filename, and apply it with fuzz
against index.cgi file).
-- >8 -- ----- ----- ----- ----- ----- -- >8 --
From: Jakub Narebski <jnareb@gmail.com>
Subject: [PATCH] gitweb: Harden parse_commit and parse_commits
Gitweb has problems and gives errors when repository it shows is on
unborn branch (HEAD doesn't point to a valid commit), but there exist
other branches.
One of errors that shows in gitweb logs is undefined $commit_id in
parse_commits() subroutine. Therefore we harden both parse_commit()
and parse_commits() against undefined $commit_id, and against no
output from git-rev-list because HEAD doesn't point to a commit.
Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
gitweb/gitweb.perl | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f9535eb..1181aeb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3334,6 +3334,8 @@ sub parse_commit {
my ($commit_id) = @_;
my %co;
+ return unless defined $commit_id;
+
local $/ = "\0";
open my $fd, "-|", git_cmd(), "rev-list",
@@ -3343,7 +3345,9 @@ sub parse_commit {
$commit_id,
"--",
or die_error(500, "Open git-rev-list failed");
- %co = parse_commit_text(<$fd>, 1);
+ my $commit_text = <$fd>;
+ %co = parse_commit_text($commit_text, 1)
+ if defined $commit_text;
close $fd;
return %co;
@@ -3353,6 +3357,7 @@ sub parse_commits {
my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
my @cos;
+ return unless defined $commit_id;
$maxcount ||= 1;
$skip ||= 0;
--
1.7.9
^ permalink raw reply related
* Re: [PATCHv2] tag: add --points-at list option
From: Jeff King @ 2012-02-08 15:31 UTC (permalink / raw)
To: Tom Grennan; +Cc: Junio C Hamano, git, jasampler
In-Reply-To: <20120208014515.GE6264@tgrennan-laptop>
On Tue, Feb 07, 2012 at 05:45:15PM -0800, Tom Grennan wrote:
> > Also, it's not symmetric. What if I say "git tag
> > --points-at=my-lw-v1.7.9"? Then I would get your signed and
> > annotated tags (even though they're _not_ saying anything about
> > ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying
> > anything about it either; in fact, it's the opposite!).
>
> Huh? As you noted, the lightweight tag is just an alternate reference,
> so why wouldn't want to see the annotated and signed tags of that common
> object?
>
> $ ./git-tag -l --points-at tomg-lw-v1.7.9
> tomg-annotate-v1.7.9
> tomg-signed-v1.7.9
> v1.7.9
> $ ./git-tag -l --points-at v1.7.9
> tomg-annotate-v1.7.9
> tomg-lw-v1.7.9
> tomg-signed-v1.7.9
Sorry, I should have been more clear here (the word symmetric isn't
right; it _is_ symmetric). My understanding of the point of your
original feature was to mention things that talk about a tag (because
you wanted to know what signatures were made around it).
With tag objects this is easy, because they contain a pointer. But when
it comes to lightweight tags, you cannot tell in which direction the
"talking about" occurred[1]. That is, a lightweight tag of another tag
is just creating a new ref, which looks the same as the old ref. So
something like --points-at cannot say "X talks about Y", because it
might as well have been "Y talks about X".
So I think you are better off to mention both X and Y (or to mention
neither).
> > Your documentation says "Only list annotated or signed tags of the
> > given object", which implies to me that --points-at is an arbitrary
> > object specifier, not a specific tagname.
>
> Yes, I changed that in the patch that I've prepared but will revert this
> if you'd rather not list these lightweight tags.
I'm OK with not mentioning lightweight tags. I just feel it should be
all-or-nothing. It was specifically the "given object" that I took issue
with, since in your examples v1.7.9 was treated differently from its
sha1.
> My reaction when I tested this was, "don't tell me what I already know."
> But consistency with $(git rev-parse ...) seems more important.
> And as you noted, a sha1_array would save code and to me, less code is
> always better.
Thanks. Either I've convinced you, or I've made you so sick of the
discussion that you're agreeing. The system works. :)
-Peff
[1] Actually, a tag object embeds the name of the ref under which it was
originally created (so the refs/tags/v1.7.9 tag has a "tag v1.7.9"
header in it). So in some cases, you _can_ determine the "original" ref
of a lightweight tag versus other refs made about it later. I'm still
not sure --points-at is a good place to try to make that distinction,
though.
^ permalink raw reply
* Re: [PATCHv3] tag: add --points-at list option
From: Jeff King @ 2012-02-08 15:44 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <1328682076-23380-2-git-send-email-tmgrennan@gmail.com>
On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
> +static const unsigned char *match_points_at(const unsigned char *sha1)
> +{
> + int i;
> + const unsigned char *tagged_sha1 = (unsigned char*)"";
> + struct object *obj = parse_object(sha1);
> +
> + if (obj && obj->type == OBJ_TAG)
> + tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
This is not safe. A sha1 is not NUL-terminated, but is rather _always_
20 bytes. So when the object is not a tag, you do the hashcmp against
your single-byte string literal above, and we end up comparing whatever
garbage is in the data segment after the string literal.
What you want instead is the all-zeros sha1, like:
const unsigned char null_sha1[20] = { 0 };
Though we provide a null_sha1 global already. So doing:
const unsigned char *tagged_sha1 = null_sha1;
would be sufficient.
That being said, I don't know why you want to do both lookups in the
same loop of the points_at. If it's a lightweight tag and the tag
matches, you can get away with not parsing the object at all (although
to be fair, that is the minority case, so it is unlikely to matter).
Also, should we be producing an error if !obj? It would indicate a tag
that points to a bogus object.
> + for (i = 0; i < points_at.nr; i++)
> + if (!hashcmp(points_at.sha1[i], sha1))
> + return sha1;
> + else if (!hashcmp(points_at.sha1[i], tagged_sha1))
> + return tagged_sha1;
> + return NULL;
Why write your own linear search? sha1_array_lookup will do a binary
search for you.
Other than that, the patch looks OK to me.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-08 15:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, git
In-Reply-To: <7vmx8tzv3l.fsf@alter.siamese.dyndns.org>
On Tue, Feb 07, 2012 at 10:40:14PM -0800, Junio C Hamano wrote:
> What I was getting at is that we probably should officially declare that
> returning 1 to signal success is perfectly acceptable (and it probably
> should mean the caller who called the callback function as a sub callback
> should return immediately, taking it as a signal that the key has already
> been handled), as the primary purpose of this thread to discuss Peff's
> patch is to write these rules down.
>
> Of course, all that relies on the audit of the git_config() machinery. I
> think it is written to accept non-negative as success, and that is why I
> said "I too think this should be acceptable" in the first place.
I looked through it the other day when this came up, and I believe that
is the case. There is another related rule that should be considered,
though: should the return value from callbacks be propagated via
git_config to its caller?
It is the case already for config files that are read, but not for
config options parsed from the command line. It would not be too hard to
change, but I do not think any current callers care (as I mentioned
earlier, these days with the "void *" data pointer, sane callers will
simply pass in a pointer to a modifiable data area).
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-08 15:59 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git
In-Reply-To: <CACsJy8B1cMBbrUznOp95u8YmfLf5bbzox8g9cuQUwgf-vY1XrQ@mail.gmail.com>
On Wed, Feb 08, 2012 at 01:55:30PM +0700, Nguyen Thai Ngoc Duy wrote:
> Or allow multiple callbacks from git_config(). The problem with is it
> adds another common set of config vars. Now it's common to chain var
> sets by doing
>
> int config_callback(...)
> {
> ....
> return yet_another_callback(...);
> }
>
> int main()
> {
> git_config(config_callback, ...)
> ...
>
> where yet_another_callback() hard codes another callback (usually
> git_default_config). This is inflexible. If we allow multiple
> callbacks, git_column_config() could be changed to return just 0 or -1
> (i.e. 1 remains unsuccessful error code).
I don't think we need multiple callbacks. You could convert any such
call of:
git_config_multiple(cb1, cb2, cb3, NULL, NULL);
into:
int combining_callback(const char *var, const char *value, void *data)
{
if (cb1(var, value, data) < 0)
return -1;
if (cb2(var, value, data) < 0)
return -1;
if (cb3(var, value, data) < 0)
return -1;
return 0;
}
But note that you get the same "data" pointer in each case. If you
wanted different data pointers, you would need more support from the
config machinery. However, I'd rather that be spelled:
git_config(cb1, data1);
git_config(cb2, data2);
git_config(cb3, data3);
and if the efficiency isn't acceptable, then looking into caching the
key/value pairs.
> On second thought, I think calling git_config() multiple times, each
> time with one callback, may work too. We may want to cache config
> content to avoid accessing files too many times though.
Exactly. I would do that first, and then worry about efficiency later if
it comes up. The first time I saw that we cached config multiple times
in a program run (several years ago), I had the same thought. But I
don't think the performance impact for reading the config 2 or 3 times
instead of 1 was even measurable, so I stopped looking into it.
If were to adopt something like the "automatically run this program to
get the config value" proposal that has been floating around (and I am
not saying we should), _then_ I think it might make sense to cache the
values, because the sub-program might be expensive to run.
-Peff
^ permalink raw reply
* Re: [StGit PATCH] Parse commit object header correctly
From: Michael Haggerty @ 2012-02-08 16:17 UTC (permalink / raw)
To: Frans Klaver
Cc: Junio C Hamano, Karl Hasselström, Catalin Marinas,
"Andy Green (林安廸)", git
In-Reply-To: <CAH6sp9P=vNjLycgzoWzRbeEsW-kQ5e4HgGYf2jP1+u9rtWV4dg@mail.gmail.com>
On 02/08/2012 11:43 AM, Frans Klaver wrote:
> On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 02/08/2012 08:33 AM, Junio C Hamano wrote:
>>> + line = lines[i].rstrip('\n')
>>> + ix = line.find(' ')
>>> + if 0 <= ix:
>>> + key, value = line[0:ix], line[ix+1:]
>>
>> The above five lines can be written
>>
>> for line in lines:
>> if ' ' in line:
>> key, value = line.rstrip('\n').split(' ', 1)
>>
>> or (if the lack of a space should be treated more like an exception)
>>
>> for line in lines:
>> try:
>> key, value = line.rstrip('\n').split(' ', 1)
>> except ValueError:
>> continue
>
> This is generally considered more pythonic: "It's easier to ask for
> forgiveness than to get permission".
Given that Junio explicitly wanted to allow lines with no spaces, I
assume that lack of a space is not an error but rather a conceivable
future extension. If my assumption is correct, then it is misleading
(and inefficient) to handle it via an exception.
>>> + if key == 'tree':
>>> + cd = cd.set_tree(repository.get_tree(value))
>>> + elif key == 'parent':
>>> + cd = cd.add_parent(repository.get_commit(value))
>>> + elif key == 'author':
>>> + cd = cd.set_author(Person.parse(value))
>>> + elif key == 'committer':
>>> + cd = cd.set_committer(Person.parse(value))
>>
>> All in all, I would recommend something like (untested):
>>
>> @return: A new L{CommitData} object
>> @rtype: L{CommitData}"""
>> cd = cls(parents = [])
>> lines = []
>> raw_lines = s.split('\n')
>> # Collapse multi-line header lines
>> for i, line in enumerate(raw_lines):
>> if not line:
>> cd.set_message('\n'.join(raw_lines[i+1:]))
>> break
>> if line.startswith(' '):
>> # continuation line
>> lines[-1] += '\n' + line[1:]
>> else:
>> lines.append(line)
>>
>> for line in lines:
>> if ' ' in line:
>> key, value = line.split(' ', 1)
>> if key == 'tree':
>> cd = cd.set_tree(repository.get_tree(value))
>> elif key == 'parent':
>> cd = cd.add_parent(repository.get_commit(value))
>> elif key == 'author':
>> cd = cd.set_author(Person.parse(value))
>> elif key == 'committer':
>> cd = cd.set_committer(Person.parse(value))
>> return cd
>
> One could also take the recommended python approach for
> switch/case-like if/elif/else statements:
>
> updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),
> 'parent': lambda cd, value:
> cd.add_parent(repository.get_commit(value)),
> 'author': lambda cd, value: cd.set_author(Person.parse(value)),
> 'committer': lambda cd, value:
> cd.set_committer(Person.parse(value))
> }
> for line in lines:
> try:
> key, value = line.split(' ', 1)
> cd = updater[key](cd, value)
> except ValueError:
> continue
> except KeyError:
> continue
>
> It documents about the same, but adds checking on double 'case'
> statements. The resulting for loop is rather cleaner and the exception
> approach becomes even more logical. I rather like the result, but I
> guess it's mostly a matter of taste.
I know this approach and use it frequently, but when one has to resort
to lambdas and there are only four cases, it becomes IMHO less readable
than the if..else version.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ 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