* 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
* [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
* [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
* 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
* 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
* 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: [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
* (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: 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
* 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-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: Specifying revisions in the future
From: Jonathan Paugh @ 2012-02-07 21:25 UTC (permalink / raw)
To: Miles Bader
Cc: Andreas Schwab, Philip Oakley, Jakub Narebski, Matthieu Moy, git
In-Reply-To: <buosjiozity.fsf@dhlpc061.dev.necel.com>
On 02/05/2012 11:28 PM, Miles Bader wrote:
> Andreas Schwab <schwab@linux-m68k.org> writes:
>> The rule should be to follow the leftmost parent as far as possible.
>> That means that X+2->D is B.
>
> It might also be reasonable (and safer -- the user may not actually
> realize when there's an ambiguating branch-point) to simply have it
> abort with an error ("ambiguous future-ref specification") when
> there's any doubt... I suspect most uses would be very simple "+1"
> etc., and not crossing branch points.
>
> -miles
>
Perhaps default to --linear or --no-cross or such. Whenever there's
ambiguity, it will likely be harder for the user to think about than for
git to resolve it in some defined-as-sane way, at least for many users.
At any rate, I got the answer I needed for my use case (sorry for not
cc-ing the list, and thanks Jakub for that:
http://article.gmane.org/gmane.comp.version-control.git/189926/match=specify+revisions+future).
Still, forward-refs would still be really cool.
Jonathan
^ permalink raw reply
* Re: [PATCH] add -e: ignore dirty submodules
From: Jens Lehmann @ 2012-02-07 20:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7vk43z2gwc.fsf@alter.siamese.dyndns.org>
Am 07.02.2012 09:21, schrieb Junio C Hamano:
> So after all, this is a noise reduction patch, and I think that it is a
> good change.
I agree. While my first thought was that it might make sense to honor
the diff.ignoreSubmodules setting here too to produce the same
information "git status" gives (so you notice you might have forgotten
to commit something in the submodule), I now agree that doesn't make
sense in the context of add -e.
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Junio C Hamano @ 2012-02-07 20:20 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Grennan, git, gitster, jasampler
In-Reply-To: <20120207193632.GC32367@sigill.intra.peff.net>
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.
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: David Aguilar @ 2012-02-07 20:15 UTC (permalink / raw)
To: Jakub Narebski, Jeff King; +Cc: git
In-Reply-To: <m31uq63143.fsf@localhost.localdomain>
On Tue, Feb 7, 2012 at 11:16 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Jeff King <peff@peff.net> writes:
>
> [...]
>> Git-config could potentially help with that (and even simplify the
>> current code) by allowing something like:
>>
>> $ git config --list-with-sources
>> /home/peff/.gitconfig user.name=Jeff King
>> /home/peff/.gitconfig user.email=peff@peff.net
>> .git/config core.repositoryformatversion=0
>> .git/config core.bare=false
>> [etc]
>>
>> (you would use the "-z" form, of course, and the filenames would be
>> NUL-separated, but I made up a human-readable output format above for
>> illustration purposes).
>
> That would be _very_ nice to have (even without includes support).
I like this as well.
Thanks for digging deep into this one, Jeff. You've convinced me that
not following includes is the better default behavior.
You are correct in mentioning that what was really missing was
something akin to "git config --repo --list". Since that command
would be a shortcut for "-f .git/config" then it would be consistent
in behavior. The suggestion to have the app understand .[include]
path = and show separate panes for included files is an elegant
solution and definitely helpful for power users. I do have to write
more code but that's fine since it enables new functionality.
Jeff, you mentioned possibly adding a "backwards-compatible way" of
accessing this stuff and hiding it behind an environment variable. I
don't want to make us carry around backwards compat code paths just
for one particular use case so perhaps the best thing would be for me
to start preparing for this change now. I already have various places
where functionality is guarded behind a git version check. If what
we're talking about is git-cola adding "--include" when git >= 1.8(?)
then that works for me.
It should be noted that git-gui also uses `git config --global --list`
so I don't know if this has implications there. E.g. maybe things
like user.name won't be overridden if done via an included config
there.
RE: the caching -- we call git config a few times in various places.
Getting the user.* fields. Getting the diff.summary settings, etc. I
started tweaking app startup for speed and noticed all the git-config
calls and was able to replace a handful of calls with a single call,
which was nice. The difference is more pronounced on win32 (I am a
linux user but I do try to play nice with the msysgit folks).
Thanks Jeff,
--
David
^ permalink raw reply
* Re: git-svn: t9155 fails against subversion 1.7.0
From: Jonathan Nieder @ 2012-02-07 20:15 UTC (permalink / raw)
To: Ben Walton; +Cc: Eric Wong, Frans Klaver, Robin H. Johnson, Git Mailing List
In-Reply-To: <1328575605-sup-4117@pinkfloyd.chass.utoronto.ca>
Ben Walton wrote:
> This is still on my todo list as I'd like to have a working bridge
> (and to be able to run the svn test suite when packaging for
> solaris). I've not had the required time to dig into this once it
> proved to be non-trivial breakage though.
Thanks. Just a quick update: I can confirm that
- Something like your original patch will probably take care of this,
once a few residual svn bugs are ironed out.
- Unfortunately, there are a few residual svn bugs, such as
<http://subversion.tigris.org/issues/show_bug.cgi?id=4091>.
- The svn developers are very helpful. Hopefully among us we should
eventually make progress, given enough time. ;-)
Ah, but time is hard to find here, too...
Proposal: I'm willing to maintain drafts of a patch to fix the test
breakage. Send me patches and I'll blindly apply them to a dedicated
branch in a repo on repo.or.cz, so small improvements are easier to
make. (Or if someone else wants to take care of the same, I'd be even
happier.) Once it's working, we can send the result in some more
logical form to the list for application to mainline.
Ciao,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 0/4] Deprecate "not allow as-is commit with i-t-a entries"
From: Junio C Hamano @ 2012-02-07 19:55 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <7v8vke38a1.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Ahh, thanks.
>
> 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.
Given that a patch to do so (with or without your 1/4 which is an
independently good change) on top of an ancient v1.7.6 codebase looks like
this, I am inclined to think this is the way to go.
I still need to review the existing documentation to see if anything needs
to be fixed up, though.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] commit: ignore intent-to-add entries instead of refusing
Originally, "git add -N" was introduced to help users from forgetting to
add new files to the index before they ran "git commit -a". As an attempt
to help them further so that they do not forget to say "-a", "git commit"
to commit the index as-is was taught to error out, reminding the user that
they may have forgotten to add the final contents of the paths before
running the command.
This turned out to be a false "safety" that is useless. If the user made
changes to already tracked paths and paths added with "git add -N", and
then ran "git add" to register the final contents of the paths added with
"git add -N", "git commit" will happily create a commit out of the index,
without including the local changes made to the already tracked paths. It
was not a useful "safety" measure to prevent "forgetful" mistakes from
happening.
It turns out that this behaviour is not just a useless false "safety", but
actively hurts use cases of "git add -N" that were discovered later and
have become popular, namely, to tell Git to be aware of these paths added
by "git add -N", so that commands like "git status" and "git diff" would
include them in their output, even though the user is not interested in
including them in the next commit they are going to make.
Fix this ancient UI mistake, and instead make a commit from the index
ignoring the paths added by "git add -N" without adding real contents.
Based on the work by Nguyễn Thái Ngọc Duy, and helped by injection of
sanity from Jonathan Nieder and others on the Git mailing list.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The last hunk of t2203 is because the test assumed the previous one
failed to make a commit and "rezrov" in HEAD does not have "xyzzy".
Now that the previous test creates a commit and records "xyzzy" in
"rezrov", this test will fail with "Nothing to commit" without it.
cache-tree.c | 6 +++---
t/t2203-add-intent.sh | 8 +++++---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/cache-tree.c b/cache-tree.c
index f755590..ce0d0e3 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -158,7 +158,7 @@ static int verify_cache(struct cache_entry **cache,
funny = 0;
for (i = 0; i < entries; i++) {
struct cache_entry *ce = cache[i];
- if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) {
+ if (ce_stage(ce)) {
if (10 < ++funny) {
fprintf(stderr, "...\n");
break;
@@ -336,8 +336,8 @@ static int update_one(struct cache_tree *it,
mode, sha1_to_hex(sha1), entlen+baselen, path);
}
- if (ce->ce_flags & CE_REMOVE)
- continue; /* entry being removed */
+ if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
+ continue; /* entry being removed or placeholder */
strbuf_grow(&buffer, entlen + 100);
strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2543529..ec35409 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -32,7 +32,7 @@ test_expect_success 'intent to add does not clobber existing paths' '
! grep "$empty" actual
'
-test_expect_success 'cannot commit with i-t-a entry' '
+test_expect_success 'i-t-a entry is simply ignored' '
test_tick &&
git commit -a -m initial &&
git reset --hard &&
@@ -41,12 +41,14 @@ test_expect_success 'cannot commit with i-t-a entry' '
echo frotz >nitfol &&
git add rezrov &&
git add -N nitfol &&
- test_must_fail git commit -m initial
+ git commit -m second &&
+ test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
+ test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
'
test_expect_success 'can commit with an unrelated i-t-a entry in index' '
git reset --hard &&
- echo xyzzy >rezrov &&
+ echo bozbar >rezrov &&
echo frotz >nitfol &&
git add rezrov &&
git add -N nitfol &&
--
1.7.9.231.g87173
^ permalink raw reply related
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-07 19:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20120207180625.GA27189@sigill.intra.peff.net>
On Tue, Feb 07, 2012 at 01:06:25PM -0500, Jeff King wrote:
> Though having looked a lot at the config code recently, I wonder if
> git_config_early is really necessary. The only caller (besides
> git_config) is check_repository_format_early, which just wants to see if
> core.repositoryformatversion in a directory is sane.
>
> But why in the world would it want to do the full config lookup?
> Shouldn't it be checking git_config_from_file directly on the config
> file in the proposed repo?
I prepared the patch below to fix this, but it fails t0001. It turns out
that check_repository_format_gently checks not only stuff that should be
in .git/config, but also checks and remembers core.sharedrepository in
the process, which can come from anywhere.
So I think the "most correct" thing to do would be:
- check_repository_format_gently reads _only_ from .git/config
- core.sharedrepository should come from git_config_default
But that just creates more headaches. Callers like init_db would need to
call git_config separately. But of course they don't have the repo set
up properly, so they would want git_config_early. So we don't end up
getting to remove git_config_early, anyway.
And though it is slightly insane that you can do:
git config --global core.repositoryformatversion 0
or even:
git -c core.repositoryformatversion=0 ...
and it is respected, in practice nobody does this. So it's probably
better to just leave the code as-is.
-Peff
-- >8 --
Subject: don't look for repositoryformatversion outside of repo
In the check_repository_format_gently function, we use
git_config_early to parse the repository format version from
the .git/config file.
However, git_config_early looks in all of the sources of git
config, including /etc/gitconfig, ~/.gitconfig, and the
command line. But we should really only be interested in getting
the value from the repository's config file, since the point
of this function is to check the repository itself.
Therefore we can just feed our repo-config argument directly
to git_config_from_file. As a bonus, this means the subtle
distinction in using git_config_early versus git_config can
just go away.
Signed-off-by: Jeff King <peff@peff.net>
---
setup.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/setup.c b/setup.c
index 61c22e6..20509cf 100644
--- a/setup.c
+++ b/setup.c
@@ -319,17 +319,8 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
{
char repo_config[PATH_MAX+1];
- /*
- * git_config() can't be used here because it calls git_pathdup()
- * to get $GIT_CONFIG/config. That call will make setup_git_env()
- * set git_dir to ".git".
- *
- * We are in gitdir setup, no git dir has been found useable yet.
- * Use a gentler version of git_config() to check if this repo
- * is a good one.
- */
snprintf(repo_config, PATH_MAX, "%s/config", gitdir);
- git_config_early(check_repository_format_version, NULL, repo_config);
+ git_config_from_file(check_repository_format_version, repo_config, NULL);
if (GIT_REPO_VERSION < repository_format_version) {
if (!nongit_ok)
die ("Expected git repo version <= %d, found %d",
--
1.7.8.4.12.g3a22e3
^ permalink raw reply related
* Re: [PATCHv2] tag: add --points-at list option
From: Jeff King @ 2012-02-07 19:36 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <20120207192135.GC6264@tgrennan-laptop>
On Tue, Feb 07, 2012 at 11:22:50AM -0800, Tom Grennan wrote:
> >> Thanks, but I now realize that I also need to save the pointed at
> >> refname to detect lightweight tags that have matching sha's but
> >> different names.
> >
> >I'm not sure I understand. Wouldn't you match lightweight tags by the
> >sha1 they point at? Something like:
>
> 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.
Maybe I don't understand what you mean. Can you show a test case that
is buggy with the v2 version of the patch that you sent? I'm not sure in
the example above what is different between the two "git-tag"
invocations.
-Peff
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Tom Grennan @ 2012-02-07 19:22 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
In-Reply-To: <20120207191202.GA496@sigill.intra.peff.net>
On Tue, Feb 07, 2012 at 02:12:02PM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 11:02:28AM -0800, Tom Grennan wrote:
>
>> >Would using sha1_array save us from having to create our own data
>> >structure? As a bonus, it can do O(lg n) lookups, though I seriously
>> >doubt anyone will provide a large number of "--points-at".
>>
>> Thanks, but I now realize that I also need to save the pointed at
>> refname to detect lightweight tags that have matching sha's but
>> different names.
>
>I'm not sure I understand. Wouldn't you match lightweight tags by the
>sha1 they point at? Something like:
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.
> static int tag_points_at(struct sha1_array *sa,
> const unsigned char *sha1)
> {
> struct object *obj;
>
> /* Lightweight tag of an interesting sha1? */
> if (sha1_array_lookup(sa, sha1) >= 0)
> return 1;
>
> /* Otherwise, maybe a tag object pointing to an interesting sha1 */
> obj = parse_object(sha1);
> if (!obj)
> return 0; /* or probably we should even just die() */
> if (obj->type != OBJ_TAG)
> return 0;
> if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
> return 0;
> return 1;
> }
>
>> >Also, should you check "unset"? When we have options that build a list,
>> >usually doing "--no-foo" will clear the list. E.g., this:
>> >
>> > git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
>> >
>> >should look only for "baz".
>>
>> Ahh, so I just need to:
>> if (unset) {
>> if (*opt_value)
>> free_points_at(*opt_value);
>> *opt_value = NULL;
>> return 0;
>> }
>
>Yes, exactly.
>
>> >> + {
>> >> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> >> + "print only annotated|signed tags of the object",
>> >> + PARSE_OPT_LASTARG_DEFAULT,
>> >> + parse_opt_points_at, (intptr_t)NULL,
>> >> + },
>> >
>> >I think you can drop the LASTARG_DEFAULT here, as it is no longer
>> >optional, no?
>>
>> You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?
>
>Right. Though without flags, you can probably just use the OPT_CALLBACK
>wrapper, like:
>
> OPT_CALLBACK(0, "points-at", &points_at, "object",
> "print only annotated|signed tags of the object",
> parse_opt_points_at)
>
>Note that if you are going to handle lightweight tags, that description
>should probably be updated.
>
>-Peff
--
TomG
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-07 19:21 UTC (permalink / raw)
To: Jakub Narebski; +Cc: David Aguilar, git
In-Reply-To: <m31uq63143.fsf@localhost.localdomain>
On Tue, Feb 07, 2012 at 11:16:47AM -0800, Jakub Narebski wrote:
> > Git-config could potentially help with that (and even simplify the
> > current code) by allowing something like:
> >
> > $ git config --list-with-sources
> > /home/peff/.gitconfig user.name=Jeff King
> > /home/peff/.gitconfig user.email=peff@peff.net
> > .git/config core.repositoryformatversion=0
> > .git/config core.bare=false
> > [etc]
> >
> > (you would use the "-z" form, of course, and the filenames would be
> > NUL-separated, but I made up a human-readable output format above for
> > illustration purposes).
>
> That would be _very_ nice to have (even without includes support).
>
> Filenames would be git-quoted like in ls-tree / diff-tree output without -z,
> isn't it? And is that TAB or SPC as a separator?
Yeah, the output above is just illustrative. Without -z, I would do
quoted filename, TAB, and then $KEY=$VALUE (I didn't check whether we
quote an "=" in the subsection of a key, but we probably should).
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jakub Narebski @ 2012-02-07 19:16 UTC (permalink / raw)
To: Jeff King; +Cc: David Aguilar, git
In-Reply-To: <20120207173025.GA22225@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
[...]
> Git-config could potentially help with that (and even simplify the
> current code) by allowing something like:
>
> $ git config --list-with-sources
> /home/peff/.gitconfig user.name=Jeff King
> /home/peff/.gitconfig user.email=peff@peff.net
> .git/config core.repositoryformatversion=0
> .git/config core.bare=false
> [etc]
>
> (you would use the "-z" form, of course, and the filenames would be
> NUL-separated, but I made up a human-readable output format above for
> illustration purposes).
That would be _very_ nice to have (even without includes support).
Filenames would be git-quoted like in ls-tree / diff-tree output without -z,
isn't it? And is that TAB or SPC as a separator?
--
Jakub Narebski
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Jeff King @ 2012-02-07 19:12 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <20120207190228.GB6264@tgrennan-laptop>
On Tue, Feb 07, 2012 at 11:02:28AM -0800, Tom Grennan wrote:
> >Would using sha1_array save us from having to create our own data
> >structure? As a bonus, it can do O(lg n) lookups, though I seriously
> >doubt anyone will provide a large number of "--points-at".
>
> Thanks, but I now realize that I also need to save the pointed at
> refname to detect lightweight tags that have matching sha's but
> different names.
I'm not sure I understand. Wouldn't you match lightweight tags by the
sha1 they point at? Something like:
static int tag_points_at(struct sha1_array *sa,
const unsigned char *sha1)
{
struct object *obj;
/* Lightweight tag of an interesting sha1? */
if (sha1_array_lookup(sa, sha1) >= 0)
return 1;
/* Otherwise, maybe a tag object pointing to an interesting sha1 */
obj = parse_object(sha1);
if (!obj)
return 0; /* or probably we should even just die() */
if (obj->type != OBJ_TAG)
return 0;
if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
return 0;
return 1;
}
> >Also, should you check "unset"? When we have options that build a list,
> >usually doing "--no-foo" will clear the list. E.g., this:
> >
> > git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
> >
> >should look only for "baz".
>
> Ahh, so I just need to:
> if (unset) {
> if (*opt_value)
> free_points_at(*opt_value);
> *opt_value = NULL;
> return 0;
> }
Yes, exactly.
> >> + {
> >> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
> >> + "print only annotated|signed tags of the object",
> >> + PARSE_OPT_LASTARG_DEFAULT,
> >> + parse_opt_points_at, (intptr_t)NULL,
> >> + },
> >
> >I think you can drop the LASTARG_DEFAULT here, as it is no longer
> >optional, no?
>
> You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?
Right. Though without flags, you can probably just use the OPT_CALLBACK
wrapper, like:
OPT_CALLBACK(0, "points-at", &points_at, "object",
"print only annotated|signed tags of the object",
parse_opt_points_at)
Note that if you are going to handle lightweight tags, that description
should probably be updated.
-Peff
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Tom Grennan @ 2012-02-07 19:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
In-Reply-To: <20120207160527.GC14773@sigill.intra.peff.net>
On Tue, Feb 07, 2012 at 11:05:27AM -0500, Jeff King wrote:
>On Mon, Feb 06, 2012 at 11:01:16PM -0800, Tom Grennan wrote:
>
>> +struct points_at {
>> + struct points_at *next;
>> + unsigned char *sha1;
>> +};
>
>Would using sha1_array save us from having to create our own data
>structure? As a bonus, it can do O(lg n) lookups, though I seriously
>doubt anyone will provide a large number of "--points-at".
Thanks, but I now realize that I also need to save the pointed at
refname to detect lightweight tags that have matching sha's but
different names.
>> +static void free_points_at (struct points_at *points_at)
>> +{
>> + while (points_at) {
>> + struct points_at *next = points_at->next;
>> + free(points_at->sha1);
>> + free(points_at);
>> + points_at = next;
>> + }
>> +}
>
>Then this could go away in favor of sha1_array_clear.
>
>> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
>> +{
>> + struct points_at *new, **opt_value = (struct points_at **)opt->value;
>> + unsigned char *sha1;
>> +
>> + if (!arg)
>> + return error(_("missing <object>"));
>> + new = xmalloc(sizeof(struct points_at));
>> + sha1 = xmalloc(20);
>> + if (get_sha1(arg, sha1)) {
>> + free(new);
>> + free(sha1);
>> + return error(_("malformed object name '%s'"), arg);
>> + }
>> + new->sha1 = sha1;
>> + new->next = *opt_value;
>> + *opt_value = new;
>> + return 0;
>> +}
>
>And this can drop all of the memory management bits, like:
>
> unsigned char sha1[20];
>
> if (!arg)
> return error(_("missing <object>"));
> if (get_sha1(arg, sha1))
> return error(_("malformed object name '%s'"), arg);
> sha1_array_append(opt->value, sha1);
> return 0;
>
>Also, should you check "unset"? When we have options that build a list,
>usually doing "--no-foo" will clear the list. E.g., this:
>
> git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
>
>should look only for "baz".
Ahh, so I just need to:
if (unset) {
if (*opt_value)
free_points_at(*opt_value);
*opt_value = NULL;
return 0;
}
>> +static struct points_at *match_points_at(struct points_at *points_at,
>> + const unsigned char *sha1)
>> +{
>> + char *buf;
>> + struct tag *tag;
>> + unsigned long size;
>> + enum object_type type;
>> +
>> + buf = read_sha1_file(sha1, &type, &size);
>> + if (!buf)
>> + return NULL;
>> + if (type != OBJ_TAG
>> + || (tag = lookup_tag(sha1), !tag)
>> + || parse_tag_buffer(tag, buf, size) < 0) {
>> + free(buf);
>> + return NULL;
>> + }
>> + while (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))
>> + points_at = points_at->next;
>> + free(buf);
>> + return points_at;
>> +}
>
>Sorry, I threw a lot of object lookup code at you last time, so I think
>my point may have been lost in the noise. But I think this is slightly
>nicer as:
>
> static int tag_points_at(struct sha1_array *sa,
> const unsigned char *sha1)
> {
> struct object *obj = parse_object(sha1);
> if (!obj)
> return 0; /* or probably we should even just die() */
> if (obj->type != OBJ_TAG)
> return 0;
> if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
> return 0;
> return 1;
> }
>
>I.e., using parse_object lets you avoid dealing with memory management
>yourself. And as a bonus, it will reuse the cached information if you
>happen to have already parsed that object (not likely in typical
>repositories, but a huge win in certain pathological cases, like repos
>storing shared objects and refs for a large number of forks).
Aye
>> + {
>> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> + "print only annotated|signed tags of the object",
>> + PARSE_OPT_LASTARG_DEFAULT,
>> + parse_opt_points_at, (intptr_t)NULL,
>> + },
>
>I think you can drop the LASTARG_DEFAULT here, as it is no longer
>optional, no?
You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?
Thanks,
TomG
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Junio C Hamano @ 2012-02-07 18:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120207182302.GA31059@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> -- >8 --
> Subject: drop odd return value semantics from userdiff_config
> ...
> 5 files changed, 12 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 5a67c20..01956c8 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2050,14 +2050,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)
> return 0;
> }
>
> - switch (userdiff_config(var, value)) {
> - case 0:
> - break;
> - case -1:
> + if (userdiff_config(var, value) < 0)
> return -1;
> - default:
> - return 0;
> - }
>
> return git_default_config(var, value, cb);
> }
> ...
Looks very nice ;-)
^ 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