* 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
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Junio C Hamano @ 2012-02-07 18:44 UTC (permalink / raw)
To: Jeff King; +Cc: git, Nguyễn Thái Ngọc Duy
In-Reply-To: <20120207180625.GA27189@sigill.intra.peff.net>
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).
^ permalink raw reply
* Re: [PATCH 2/2] config: add include directive
From: Jeff King @ 2012-02-07 18:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v62fj60ya.fsf@alter.siamese.dyndns.org>
On Mon, Feb 06, 2012 at 02:39:41PM -0800, Junio C Hamano wrote:
> > +Includes
> > +~~~~~~~~
> > +
> > +You can include one config file from another by setting the special
> > +`include.path` variable to the name of the file to be included. The
> > +included file is expanded immediately, as if its contents had been
> > +found at the location of the include directive. If the value of the
> > +`include.path` variable is a relative path, the path is considered to be
> > +relative to the configuration file in which the include directive was
> > +found. See below for examples.
>
> If the file referenced by this directive does not exist, what should
> happen? Should it be signalled as an error? Should it stop the whole
> calling process with die()?
I silently ignore it. My thinking was that you might want to have
something like:
[include]
path = .gitconfig-local
in a stock .gitconfig that you ship to all of your machines[1]. Then
machines that need it can put things in .gitconfig-local, and those that
don't can just ignore it.
It is a tradeoff, of course, in that typos will be silently ignored. For
this use case, you could also just create an empty .gitconfig-local on
machines that don't have anything to put there.
[1] Actually, a similar use might be a ~/.gitconfig that is shared by a
mounted home directory (e.g., via NFS) NFS, and a ~/.gitconfig-$HOST
that is specific to each machine. The current code doesn't expand
environment variables (nor tildes), but perhaps it should.
> I think "die() when we are honoring the include, ignore when we are not"
> would be a good way to handle this, as it allows us to catch mistakes
> while allowing the user to fix broken configuration files using "git
> config --unset include.path", but I may be overlooking something.
The writing path does not use the include callback wrapper at all; so
include.path can be manipulated just as any other variable, and the
value is not treated specially.
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-07 18:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <7vpqdq1pxt.fsf@alter.siamese.dyndns.org>
On Tue, Feb 07, 2012 at 10:03:26AM -0800, Junio C Hamano wrote:
> > But isn't "git cola" a git-config editing program?
>
> Yes, that is really the right "rhetorical" question to ask. It needs to
> know what it is editing, so it at least needs to be _aware_ of the
> inclusion, where each item comes from, and how to edit contents of _one_
> particular file.
>
> And that issue is not something new that was introduced by include.path.
True, but it was made more complex. David has already dealt with the 3
sources in his code. Adding includes expands this to N sources, and his
code needs to adapt. It would be nice if we could at least make the
adaptation backwards-compatible and as painless as possible.
But I just see the "right" answer in a complex case needing user input
that a simple call to "git config" can't provide. That is, pretending
that "git config --global" is still a single file for reading and
writing can get you something that _works_, but it will make a mess of
the user's config. The most elegant thing to me is to expand git-cola's
config editor into an N-pane editor instead of a 2-pane editor (not 3,
because it doesn't make sense to edit /etc/gitconfig with it, and nor
does the current version support it). People without includes wouldn't
notice the difference, and people with includes might appreciate the
power and flexibility.
-Peff
^ permalink raw reply
* Re: User authentication in GIT
From: Sitaram Chamarty @ 2012-02-07 18:26 UTC (permalink / raw)
To: supadhyay; +Cc: git
In-Reply-To: <1328632848471-7262934.post@n2.nabble.com>
On Tue, Feb 7, 2012 at 10:10 PM, supadhyay <supadhyay@imany.com> wrote:
> Thank you Johan,freak.
>
> you have clear my doubts at some extent and I think let me work on it.
>
> I will work on giloite and get back to you.
I've also been working on some nice pictures... maybe they will help,
who knows...
http://sitaramc.github.com/gitolite/pictures.html
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-07 18:23 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:
> So I think we could go back and simplify the userdiff_config code now.
That patch would look like this.
-- >8 --
Subject: drop odd return value semantics from userdiff_config
When the userdiff_config function was introduced in be58e70
(diff: unify external diff and funcname parsing code,
2008-10-05), it used a return value convention unlike any
other config callback. Like other callbacks, it used "-1" to
signal error. But it returned "1" to indicate that it found
something, and "0" otherwise; other callbacks simply
returned "0" to indicate that no error occurred.
This distinction was necessary at the time, because the
userdiff namespace overlapped slightly with the color
configuration namespace. So "diff.color.foo" could mean "the
'foo' slot of diff coloring" or "the 'foo' component of the
"color" userdiff driver". Because the color-parsing code
would die on an unknown color slot, we needed the userdiff
code to indicate that it had matched the variable, letting
us bypass the color-parsing code entirely.
Later, in 8b8e862 (ignore unknown color configuration,
2009-12-12), the color-parsing code learned to silently
ignore unknown slots. This means we no longer need to
protect userdiff-matched variables from reaching the
color-parsing code.
We can therefore change the userdiff_config calling
convention to a more normal one. This drops some code from
each caller, which is nice. But more importantly, it reduces
the cognitive load for readers who may wonder why
userdiff_config is unlike every other config callback.
There's no need to add a new test confirming that this
works; t4020 already contains a test that sets
diff.color.external.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/blame.c | 8 +-------
builtin/cat-file.c | 8 +-------
builtin/grep.c | 7 ++-----
diff.c | 7 ++-----
userdiff.c | 19 ++++++-------------
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);
}
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 07bd984..8ed501f 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -226,14 +226,8 @@ static const char * const cat_file_usage[] = {
static int git_cat_file_config(const char *var, const char *value, void *cb)
{
- 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);
}
diff --git a/builtin/grep.c b/builtin/grep.c
index 5c2ae94..dc6de83 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -325,11 +325,8 @@ static int grep_config(const char *var, const char *value, void *cb)
struct grep_opt *opt = cb;
char *color = NULL;
- switch (userdiff_config(var, value)) {
- case 0: break;
- case -1: return -1;
- default: return 0;
- }
+ if (userdiff_config(var, value) < 0)
+ return -1;
if (!strcmp(var, "grep.extendedregexp")) {
if (git_config_bool(var, value))
diff --git a/diff.c b/diff.c
index 7e15426..a656f5e 100644
--- a/diff.c
+++ b/diff.c
@@ -177,11 +177,8 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return 0;
}
- switch (userdiff_config(var, value)) {
- case 0: break;
- case -1: return -1;
- default: return 0;
- }
+ if (userdiff_config(var, value) < 0)
+ return -1;
if (!prefixcmp(var, "diff.color.") || !prefixcmp(var, "color.diff.")) {
int slot = parse_diff_color_slot(var, 11);
diff --git a/userdiff.c b/userdiff.c
index 76109da..1e7184f 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -210,14 +210,7 @@ static int parse_funcname(struct userdiff_funcname *f, const char *k,
if (git_config_string(&f->pattern, k, v) < 0)
return -1;
f->cflags = cflags;
- return 1;
-}
-
-static int parse_string(const char **d, const char *k, const char *v)
-{
- if (git_config_string(d, k, v) < 0)
- return -1;
- return 1;
+ return 0;
}
static int parse_tristate(int *b, const char *k, const char *v)
@@ -226,13 +219,13 @@ static int parse_tristate(int *b, const char *k, const char *v)
*b = -1;
else
*b = git_config_bool(k, v);
- return 1;
+ return 0;
}
static int parse_bool(int *b, const char *k, const char *v)
{
*b = git_config_bool(k, v);
- return 1;
+ return 0;
}
int userdiff_config(const char *k, const char *v)
@@ -246,13 +239,13 @@ int userdiff_config(const char *k, const char *v)
if ((drv = parse_driver(k, v, "binary")))
return parse_tristate(&drv->binary, k, v);
if ((drv = parse_driver(k, v, "command")))
- return parse_string(&drv->external, k, v);
+ return git_config_string(&drv->external, k, v);
if ((drv = parse_driver(k, v, "textconv")))
- return parse_string(&drv->textconv, k, v);
+ return git_config_string(&drv->textconv, k, v);
if ((drv = parse_driver(k, v, "cachetextconv")))
return parse_bool(&drv->textconv_want_cache, k, v);
if ((drv = parse_driver(k, v, "wordregex")))
- return parse_string(&drv->word_regex, k, v);
+ return git_config_string(&drv->word_regex, k, v);
return 0;
}
--
1.7.8.4.12.g3a22e3
^ permalink raw reply related
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-07 18:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbopb61cd.fsf@alter.siamese.dyndns.org>
On Mon, Feb 06, 2012 at 02:31:14PM -0800, Junio C Hamano wrote:
> > +Config files are parsed linearly, and each variable found is passed to a
> > +caller-provided callback function. The callback function is responsible
> > +for any actions to be taken on the config option, and is free to ignore
> > +some options (it is not uncommon for the configuration to be parsed
> > +several times during the run of a git program, with different callbacks
> > +picking out different variables useful to themselves).
>
> It woud be easeier to read if you stopped the sentence after "some
> options" and made the "It is not uncommon..." a first-class sentence
> outside the parentheses.
Thanks. Putting in too many parenthetical phrases is a bad habit of
mine. Usually I notice around the time I try nesting a parenthetical
phrase instead of existing parentheses, and go back and at least promote
the outer one to a real sentence. :)
Will fix for the re-roll.
> > +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 return value is propagated from git_parse_file. I assume the
original intent was that you could actually use it to return an integer
from your callback. In practice, though, callbacks either modify global
data (for older instances), or modify a pointer passed through the void
data pointer.
The "1 means I understood this" convention is used by userdiff_config. I
don't like that it is unlike every other config callback, but I think
it's necessary to deal with the ambiguity of "diff.color.*", which could
be either a userdiff entry or a diff color. E.g., if we see
"diff.color.binary", we know that it is the "binary" variable of the
"color" diff driver, not the color spec for the "binary" slot.
Looking at the code again, though, it seems that there is no overlap
between the userdiff slots and the color slots, and that the
color-parsing code will silently ignore any unknown slots. So it would
be safe to further investigate diff.color.binary as a color, as we would
silently ignore it.
Hmm. Yeah. The userdiff calling convention dates back to late 2008. At
that time, parse_diff_color_slot would die() if it did not understand
the slot, making the "I understood this" flag required. Then later, in
8b8e862 (ignore unknown color configuration, 2009-12-12), it was
relaxed.
So I think we could go back and simplify the userdiff_config code now.
> > +There is a special version of `git_config` called `git_config_early`
> > +that takes an additional parameter to specify the repository config.
> > +This should be used early in a git program when the repository location
> > +has not yet been determined (and calling the usual lazy-evaluation
> > +lookup rules would yield an incorrect location).
>
> Do you want to say somethink like "Ordinary programs should not have to
> worry about git_config_early()"? Differently put, if you are learning the
> config API by reading this document and cannot tell which one you should
> be calling, you are way too inexperienced to call git_config_early() and
> you would always want to call git_config()?
Yes, I think that would be sensible.
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?
-Peff
^ permalink raw reply
* Re: [PATCHv2] tag: add --points-at list option
From: Tom Grennan @ 2012-02-07 18:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, jasampler
In-Reply-To: <7vd39r2g8o.fsf@alter.siamese.dyndns.org>
On Tue, Feb 07, 2012 at 12:35:19AM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +struct points_at {
>> + struct points_at *next;
>> + unsigned char *sha1;
>> +};
>
>struct points_at {
> struct points_at *next;
> unsigned char sha1[20];
>};
>
>would save you from having to allocate and free always in pairs, no?
Yep
>> +static void free_points_at (struct points_at *points_at)
>
>Please lose the SP before (.
Oops
>> + if (type != OBJ_TAG
>> + || (tag = lookup_tag(sha1), !tag)
>> + || parse_tag_buffer(tag, buf, size) < 0) {
>
>Even though I personally prefer to cascade a long expression like this, so
>that you see a parse tree when you tilt your head 90-degrees to the left,
>I think the prevalent style in Git codebase is
>
> if (A-long-long-expression ||
> B-long-long-expression ||
> C-long-long-expression) {
>
>Also we try to avoid assignment in the conditional.
I like to compact multiple conditions to a common exit but also appreciate
the fear and loathing of comma's.
While rearranging this I finally understand how to include lightweight tags.
struct points_at *pa;
const unsigned char *tagged_sha1 = (const unsigned char *)"";
/* 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;
buf = read_sha1_file(sha1, &type, &size);
if (buf) {
if (type == OBJ_TAG) {
tag = lookup_tag(sha1);
if (parse_tag_buffer(tag, buf, size) >= 0)
tagged_sha1 = tag->tagged->sha1;
}
free(buf);
}
while (points_at && hashcmp(points_at->sha1, tagged_sha1))
points_at = points_at->next;
return points_at;
For example,
$ ./git-tag tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -a tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -s tomg-lw-v1.7.9 v1.7.9
$ ./git-tag -s tomg-README HEAD:README
$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README
tomg-README
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README \*v1.7.9
tomg-annotate-v1.7.9
tomg-lw-v1.7.9
tomg-signed-v1.7.9
>> @@ -432,6 +500,12 @@ 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", &points_at, "object",
>> + "print only annotated|signed tags of the object",
>> + PARSE_OPT_LASTARG_DEFAULT,
>> + parse_opt_points_at, (intptr_t)NULL,
>> + },
>
>If you are going to reject NULL anyway, do you still need to mark this as
>lastarg-default?
>
>Looking for example in parse-options.h, I found this:
>
> #define OPT_STRING_LIST(s, l, v, a, h) \
> { OPTION_CALLBACK, (s), (l), (v), (a), \
> (h), 0, &parse_opt_string_list }
>
>which is used by "git clone" to mark its -c option.
>
>Running "git clone -c" gives me
>
> error: switch 'c' requires a value
>
>without any extra code in the caller of parse_options().
Cool
>Other than that, looks cleanly done.
>
>Thanks. I'll take another look after I wake up in the morning.
Thanks, I'll send v3 later today.
--
TomG
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Junio C Hamano @ 2012-02-07 18:03 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:
> So fundamentally, includes make the idea of "overwrite this value" much
> more complex.
I do not think it is anything new that include.path brings. To give the
user a complete view of "what customization applies when working in this
repository?", and to allow the user to edit variables in the right file
among usual three places, the tool already needs to be aware of possible
places and give users a way to tell where the edit needs to go anyway.
include.path only adds one more thing for the tool to be aware of.
With your example, the editor can show
git config -f .git/config --list
with "include.path=~/.gitident" listed as one of the key=value pairs
without showing key=value pairs included from that file. Or it can show
user.name in effect is this value from .git/config, and optionally also
show that there are other definitions of user.name in ~/.gitconfig (which
we use as if we have "include.path=~/.gitconfig" at the top of .git/config
file) or ~/.gitident specified with include.path.
The tool needs to make it easy to jump to ~/.gitident; it needs to know
what include.path means. The user can edit the value in ~/.gitident, or
after looking at ~/.gitident, choose to come back to .git/config and add
an overriding entry there.
> But isn't "git cola" a git-config editing program?
Yes, that is really the right "rhetorical" question to ask. It needs to
know what it is editing, so it at least needs to be _aware_ of the
inclusion, where each item comes from, and how to edit contents of _one_
particular file.
And that issue is not something new that was introduced by include.path.
^ permalink raw reply
* Re: STGIT: Deathpatch in linus tree
From: Junio C Hamano @ 2012-02-07 17:37 UTC (permalink / raw)
To: Andy Green (林安廸), Catalin Marinas; +Cc: git
In-Reply-To: <4F3120D4.1050604@warmcat.com>
"Andy Green (林安廸)" <andy@warmcat.com> writes:
> [agreen@build linux-2.6]$ git reset --hard 105e518
> HEAD is now at 105e518 Merge tag 'hwmon-fixes-for-3.3-rc3' of
> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging
> [agreen@build linux-2.6]$ stg push
> Error: Unhandled exception:
> Traceback (most recent call last):
> File "/usr/lib/python2.7/site-packages/stgit/main.py", line 152, in _main
> ret = command.func(parser, options, args)
> File "/usr/lib/python2.7/site-packages/stgit/commands/push.py", line
> 68, in func
> check_clean_iw = clean_iw)
> File "/usr/lib/python2.7/site-packages/stgit/lib/transaction.py", line
> 95, in __init__
> self.__current_tree = self.__stack.head.data.tree
> File "/usr/lib/python2.7/site-packages/stgit/lib/git.py", line 426, in
> data
> self.__repository.cat_object(self.sha1))
> File "/usr/lib/python2.7/site-packages/stgit/lib/git.py", line 408, in
> parse
> assert False
> AssertionError
Stgit at least at version 0.15 (which is you used, and which is what I
checked its source for) assumes it knows every possible kind of header
that can be recorded in the commit object and hits this assert when it
seems something it does not understand.
Version 0.16 seems to have removed this assert, like this hunk on the file
between 0.15 and 0.16:
@@ -404,8 +404,6 @@ class CommitData(Immutable, Repr):
cd = cd.set_author(Person.parse(value))
elif key == 'committer':
cd = cd.set_committer(Person.parse(value))
- else:
- assert False
assert False
The above code in StGit that tries to parse commit object header is
broken, and I am surprised that nobody caught it back in late 2006 when
the optional encoding header was added to the commit object, which the
above does not understand.
I am not sure if that removal of the extra assertion is enough, though.
It does not skip lines that it does not understand until the first blank
line, i.e. the end of the header, as it should. Instead it has this
before starting to inspect if a line in the header is what it knows about:
line = lines[i].strip()
if not line:
return cd.set_message(''.join(lines[i+1:]))
key, value = line.split(None, 1)
I do not think this will work on a continuation line inside a header that
has only one token, and it will also ignore the leading whitespace that
protects the continuation line.
I think the loop should at least changed line this, until StGit starts
caring about multi-line headers with continuation lines.
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 56287f6..f2b284d 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -392,18 +392,20 @@ class CommitData(Immutable, Repr):
cd = cls(parents = [])
lines = list(s.splitlines(True))
for i in xrange(len(lines)):
- line = lines[i].strip()
+ line = lines[i]
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))
+ 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))
assert False
class Commit(GitObject):
^ permalink raw reply related
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Ronan Keryell @ 2012-02-07 17:27 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <201202071531.08385.jnareb@gmail.com>
>>>>> On Tue, 7 Feb 2012 15:31:07 +0100, Jakub Narebski <jnareb@gmail.com> said:
>> Agreed, but AFAICS (and modulo the addition of pre-rewrite and
>> pre/post-push hooks mentioned earlier) all of the things
>> discussed so far in this thread can be implemented as hooks.
Jakub> That would be nice.
Jakub> And the new hooks (pre-rewrite, pre/post-push) would be
Jakub> useful anyway, I think.
Yes, to deal with file metadata in git for example. :-)
--
Ronan KERYELL |\/ Phone: +1 408 844 4720
Wild Systems / HPC Project, Inc. |/) Cell: +33 613 143 766
5201 Great America Parkway #3241 K Ronan.Keryell@hpc-project.com
Santa Clara, CA 95054 |\ skype:keryell
USA | \ http://hpc-project.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