* Re: [StGit PATCH] Parse commit object header correctly
From: Junio C Hamano @ 2012-02-09 3:58 UTC (permalink / raw)
To: Frans Klaver
Cc: Michael Haggerty, Karl Hasselström, Catalin Marinas,
Andy Green (林安廸), git
In-Reply-To: <op.v9dl1e0v0aolir@keputer.lokaal>
"Frans Klaver" <fransklaver@gmail.com> writes:
>>>> for line in lines:
>>>> try:
>>>> key, value = line.rstrip('\n').split(' ', 1)
>>>> except ValueError:
>>>> continue
>>>
>>> This is generally considered more pythonic: "It's easier to ask for
>>> forgiveness than to get permission".
>>
>> Given that Junio explicitly wanted to allow lines with no spaces, I
>> assume that lack of a space is not an error but rather a conceivable
>> future extension. If my assumption is correct, then it is misleading
>> (and inefficient) to handle it via an exception.
>
> I find the documenting more convincing than the efficiency, but from
> the phrasing I think you do too.
A line that consists entirely of non-SP may or may not a conceivable
future extension, but the point is to "skip without barfing anything you
do not understand".
I wouldn't oppose the rewrite that uses try/except ValueError if
"everything in this try block will parse what I understand correctly, and
any ValueError exception this try block throws is an indication that I
encountered what I do not understand and I must skip" is the more pythonic
way to express that principle. Python is not my primary language as I
said, and in addition StGit may have its own style I haven't learned.
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-09 3:30 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:
> 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?
So the patch would look something like this. However, is the actual
filename really what callers want? It seems like in David's case, an
annotation of "repo", "global", or "system" (possibly in addition to the
filename) would be the most useful (because in the git-cola UI, it is
still nice to list things as "repo" or "global" instead of spewing the
whole filename at the user -- but you would still want the individual
filename for handling updates of includes).
---
builtin/config.c | 20 ++++++++++++++++++--
cache.h | 1 +
config.c | 7 +++++++
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index d35c06a..e0333f7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -2,6 +2,7 @@
#include "cache.h"
#include "color.h"
#include "parse-options.h"
+#include "quote.h"
static const char *const builtin_config_usage[] = {
"git config [options]",
@@ -41,6 +42,7 @@ static int end_null;
#define ACTION_SET_ALL (1<<12)
#define ACTION_GET_COLOR (1<<13)
#define ACTION_GET_COLORBOOL (1<<14)
+#define ACTION_LIST_WITH_SOURCES (1<<15)
#define TYPE_BOOL (1<<0)
#define TYPE_INT (1<<1)
@@ -64,6 +66,7 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "rename-section", &actions, "rename section: old-name new-name", ACTION_RENAME_SECTION),
OPT_BIT(0, "remove-section", &actions, "remove a section: name", ACTION_REMOVE_SECTION),
OPT_BIT('l', "list", &actions, "list all", ACTION_LIST),
+ OPT_BIT(0, "list-with-sources", &actions, "list all variables with sources", ACTION_LIST_WITH_SOURCES),
OPT_BIT('e', "edit", &actions, "opens an editor", ACTION_EDIT),
OPT_STRING(0, "get-color", &get_color_slot, "slot", "find the color configured: [default]"),
OPT_STRING(0, "get-colorbool", &get_colorbool_slot, "slot", "find the color setting: [stdout-is-tty]"),
@@ -86,6 +89,18 @@ static void check_argc(int argc, int min, int max) {
static int show_all_config(const char *key_, const char *value_, void *cb)
{
+ int show_sources = *(int *)cb;
+
+ if (show_sources) {
+ const char *fn = current_config_filename();
+ if (!fn)
+ fn = "";
+ if (end_null)
+ printf("%s%c", fn, '\0');
+ else
+ write_name_quoted(fn, stdout, '\t');
+ }
+
if (value_)
printf("%s%c%s%c", key_, delim, value_, term);
else
@@ -418,9 +433,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
usage_with_options(builtin_config_usage, builtin_config_options);
}
- if (actions == ACTION_LIST) {
+ if (actions == ACTION_LIST || actions == ACTION_LIST_WITH_SOURCES) {
+ int show_sources = actions & ACTION_LIST_WITH_SOURCES ? 1 : 0;
check_argc(argc, 0, 0);
- if (git_config(show_all_config, NULL) < 0) {
+ if (git_config(show_all_config, &show_sources) < 0) {
if (config_exclusive_filename)
die_errno("unable to read config file '%s'",
config_exclusive_filename);
diff --git a/cache.h b/cache.h
index 9bd8c2d..98b1d09 100644
--- a/cache.h
+++ b/cache.h
@@ -1138,6 +1138,7 @@ extern const char *get_log_output_encoding(void);
extern const char *get_commit_output_encoding(void);
extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+extern const char *current_config_filename(void);
extern const char *config_exclusive_filename;
diff --git a/config.c b/config.c
index 40f9c6d..7b4094b 100644
--- a/config.c
+++ b/config.c
@@ -1564,3 +1564,10 @@ int config_error_nonbool(const char *var)
{
return error("Missing value for '%s'", var);
}
+
+const char *current_config_filename(void)
+{
+ if (cf && cf->name)
+ return cf->name;
+ return NULL;
+}
--
1.7.9.rc2.14.g3da2b
^ permalink raw reply related
* Re: [PATCH v2 0/4] Deprecate "not allow as-is commit with i-t-a entries"
From: Nguyen Thai Ngoc Duy @ 2012-02-09 2:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7v8vkdz0sg.fsf@alter.siamese.dyndns.org>
On Thu, Feb 9, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ok, the strategy part is now behind us, but I have this slight suspicion
> (I didn't look at the code nor tried it out myself---I don't have time to
> do this myself today) that using this codepath might result in a corrupt
> cache-tree, whose entries point at a section of the index it covers as a
> subtree of the whole project but with incorrect counters or something like
> that. It would be good to make sure this "just ignoring i-t-a" is doing
> the right thing not to the resulting commit, but in the resulting index as
> well, before we go forward with this change.
I looked a little bit and I think i-t-a entries do not break
cache-tree. CE_REMOVE ones might though because they are taken into
account in entry_count field, but they are not written down to file.
Will look into it tonight.
--
Duy
^ permalink raw reply
* Re: [PATCH-master] tag: add --points-at list option
From: Jeff King @ 2012-02-09 1:44 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <1328742223-24419-2-git-send-email-tmgrennan@gmail.com>
On Wed, Feb 08, 2012 at 03:03:43PM -0800, Tom Grennan wrote:
> 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
> v1.0
>
> Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
This version looks fine to me. Thanks.
-Peff
^ permalink raw reply
* [PATCH-master] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 23:03 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
In-Reply-To: <20120208205857.GA22479@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
v1.0
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-tag.txt | 6 ++++-
builtin/tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
t/t7004-tag.sh | 39 +++++++++++++++++++++++++++++++++++
3 files changed, 93 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 53ff5f6..8d32b9a 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,8 @@ 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>] [<pattern>...]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
+ [<pattern>...]
'git tag' -v <tagname>...
DESCRIPTION
@@ -86,6 +87,9 @@ OPTIONS
--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 31f02e8..27c3557 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -15,11 +15,13 @@
#include "diff.h"
#include "revision.h"
#include "gpg-interface.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
};
@@ -30,6 +32,8 @@ struct tag_filter {
struct commit_list *with_commit;
};
+static struct sha1_array points_at;
+
static int match_pattern(const char **patterns, const char *ref)
{
/* no pattern means match everything */
@@ -41,6 +45,24 @@ static int match_pattern(const char **patterns, const char *ref)
return 0;
}
+static const unsigned char *match_points_at(const char *refname,
+ const unsigned char *sha1)
+{
+ const unsigned char *tagged_sha1 = NULL;
+ struct object *obj;
+
+ if (sha1_array_lookup(&points_at, sha1) >= 0)
+ return sha1;
+ obj = parse_object(sha1);
+ if (!obj)
+ die(_("malformed object at '%s'"), refname);
+ if (obj->type == OBJ_TAG)
+ tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+ if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+ return tagged_sha1;
+ return NULL;
+}
+
static int in_commit_list(const struct commit_list *want, struct commit *c)
{
for (; want; want = want->next)
@@ -105,6 +127,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
+ if (points_at.nr && !match_points_at(refname, sha1))
+ return 0;
+
if (!filter->lines) {
printf("%s\n", refname);
return 0;
@@ -375,6 +400,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;
@@ -417,6 +459,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()
};
@@ -448,6 +494,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("-n option is only allowed with -l."));
if (with_commit)
die(_("--contains option is only allowed with -l."));
+ if (points_at.nr)
+ die(_("--points-at option is only allowed with -l."));
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..f61e398 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
test_must_fail git tag -v -s
'
+# check points-at
+
+test_expect_success '--points-at cannot be used in non-list mode' '
+ test_must_fail git tag --points-at=v4.0 foo
+'
+
+test_expect_success '--points-at finds lightweight tags' '
+ echo v4.0 >expect &&
+ git tag --points-at v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of commits' '
+ git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
+ echo annotated-v4.0 >expect &&
+ git tag -l --points-at v4.0 "annotated*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of tags' '
+ git tag -m "describing the v4.0 tag object" \
+ annotated-again-v4.0 annotated-v4.0 &&
+ cat >expect <<-\EOF &&
+ annotated-again-v4.0
+ annotated-v4.0
+ EOF
+ git tag --points-at=annotated-v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple --points-at are OR-ed together' '
+ cat >expect <<-\EOF &&
+ v2.0
+ v3.0
+ EOF
+ git tag --points-at=v2.0 --points-at=v3.0 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.8
^ permalink raw reply related
* [PATCH-master] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 23:03 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
In-Reply-To: <20120208205857.GA22479@sigill.intra.peff.net>
The following applies the "points-at" feature to master and includes unit tests
from Jeff King <peff@peff.net>
Tom Grennan (1):
tag: add --points-at list option
Documentation/git-tag.txt | 6 ++++-
builtin/tag.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
t/t7004-tag.sh | 39 +++++++++++++++++++++++++++++++++++
3 files changed, 93 insertions(+), 2 deletions(-)
--
1.7.8
^ permalink raw reply
* Re: git p4 submit bugs (submit fails due to keyword expansion mismatch and gives bad instructions for how to proceed)
From: Luke Diamand @ 2012-02-08 22:27 UTC (permalink / raw)
To: Eric Scouten; +Cc: git
In-Reply-To: <CAEe=O8o-BG0xNGQEweezPu8cj+Brt1Km_anhEJBg0W7Q=rLbkw@mail.gmail.com>
On 08/02/12 21:49, Eric Scouten wrote:
> I've been experimenting with git-p4 as a front-end to Perforce as
> described in http://www.perforce.com/blog/120113/git-perforce-client,
> but two bugs are causing me major headaches:
>
> (1) git-p4 doesn't work with P4 keyword expansion. (I gather there's a
> fork of git-p4 that makes it work, but the current official distro
> doesn't.) This can lead to some ugly failed submits down the road.
> Which leads me to ...
Do you mean those awful $RCS$ keywords?
I had a go at creating some code that would cope, but in the end I'm
afraid I gave up.
My strategy was to try the patch, if it failed, look for RCS keywords,
and then zap them in the destination p4 file and then try the patch again.
It worked for some cases reasonably well (deletion is a bit trickier),
but it failed horribly if anyone was crazy enough to copy a p4 file with
expanded RCS keywords into the git repo. So at that point, I gave up in
disgust and just wrote a pre-commit script to reject all changes
involving $RCS$.
Basically, my feeling now is that RCS keywords are utterly bonkers, and
anyone stupid enough to use them has probably got bigger problems than
just git-p4 - I hope that's not you though :-)
>
> (2) When a git p4 submit fails, the error message for how to resolve
> it is bogus. The instructions say "Please resolve and submit the
> conflict manually and continue afterwards with git-p4 submit
> --continue", but when I do that, I get:
>
> $ git p4 submit --continue
> Usage: git-p4 submit [options] [name of git branch to submit into
> perforce depot]
> git-p4: error: no such option: --continue
Yes, that _is_ annoying.
>
> Ugh.
>
> (P.S. If there's a better place to post bug reports, etc., please
> inform. Thanks.)
>
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 2/2] submodules: always use a relative path from gitdir to work tree
From: Jens Lehmann @ 2012-02-08 22:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Antony Male, Phil Hord
In-Reply-To: <4F32F252.7050105@web.de>
Since recently a submodule with name <name> has its git directory in the
.git/modules/<name> directory of the superproject while the work tree
contains a gitfile pointing there. To make that work the git directory has
the core.worktree configuration set in its config file to point back to
the work tree.
That core.worktree is an absolute path set by the initial clone of the
submodule. A relative path is preferable here because it allows the
superproject to be moved around without invalidating that setting, so
compute and set that relative path after cloning or reactivating the
submodule.
This also fixes a bug when moving a submodule around inside the
superproject, as the current code forgot to update the setting to the new
submodule work tree location.
Enhance t7400 to ensure that future versions won't re-add absolute paths
by accident and that moving a superproject won't break submodules.
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
git-submodule.sh | 9 +++++++++
t/t7400-submodule-basic.sh | 20 ++++++++++++++++++++
2 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 2a93c61..3463d6d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -169,6 +169,15 @@ module_clone()
fi
echo "gitdir: $rel_gitdir" >"$path/.git"
+
+ a=$(cd "$gitdir" && pwd)
+ b=$(cd "$path" && pwd)
+ while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
+ do
+ a=${a#*/} b=${b#*/};
+ done
+ rel=$(echo $a | sed -e 's|[^/]*|..|g')
+ (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
}
#
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2b70b22..b377a7a 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -81,6 +81,13 @@ test_expect_success 'submodule add' '
test ! -s actual &&
echo "gitdir: ../.git/modules/submod" >expect &&
test_cmp expect submod/.git &&
+ (
+ cd submod &&
+ git config core.worktree >actual &&
+ echo "../../../submod" >expect &&
+ test_cmp expect actual &&
+ rm -f actual expect
+ ) &&
git submodule init
) &&
@@ -500,4 +507,17 @@ test_expect_success 'relative path works with user@host:path' '
)
'
+test_expect_success 'moving the superproject does not break submodules' '
+ (
+ cd addtest &&
+ git submodule status >expect
+ )
+ mv addtest addtest2 &&
+ (
+ cd addtest2 &&
+ git submodule status >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
1.7.9.190.gf8e73.dirty
^ permalink raw reply related
* Re: [PATCHv4] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 22:15 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
In-Reply-To: <20120208205857.GA22479@sigill.intra.peff.net>
On Wed, Feb 08, 2012 at 03:58:57PM -0500, Jeff King wrote:
>On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:
>
>> 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
>
>And probably "v1.0", as well, in this iteration. :)
Yep.
>The patch content itself looks good to me, except:
>
>> --- 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>...]
>
>What's this "column" stuff doing here? The nd/columns topic is still in
>"next", isn't it? Did you base this on "next" or "pu"?
>
>Usually topics should be based on master, so they can graduate
>independently of each other. In this case, it might make sense to build
>on top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is
>even necessary here (my fixes ended up not being too closely related, I
>think).
Yes, it's no longer related to jk/maint-tag-show-fixes.
I've prepared a rebase patch to master and will add these tests.
Thanks,
TomG
>Other than that, I think the patch is fine. There are no tests, so
>perhaps these should be squashed in:
>
>diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>index e93ac73..f61e398 100755
>--- a/t/t7004-tag.sh
>+++ b/t/t7004-tag.sh
>@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
> test_must_fail git tag -v -s
> '
>
>+# check points-at
>+
>+test_expect_success '--points-at cannot be used in non-list mode' '
>+ test_must_fail git tag --points-at=v4.0 foo
>+'
>+
>+test_expect_success '--points-at finds lightweight tags' '
>+ echo v4.0 >expect &&
>+ git tag --points-at v4.0 >actual &&
>+ test_cmp expect actual
>+'
>+
>+test_expect_success '--points-at finds annotated tags of commits' '
>+ git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
>+ echo annotated-v4.0 >expect &&
>+ git tag -l --points-at v4.0 "annotated*" >actual &&
>+ test_cmp expect actual
>+'
>+
>+test_expect_success '--points-at finds annotated tags of tags' '
>+ git tag -m "describing the v4.0 tag object" \
>+ annotated-again-v4.0 annotated-v4.0 &&
>+ cat >expect <<-\EOF &&
>+ annotated-again-v4.0
>+ annotated-v4.0
>+ EOF
>+ git tag --points-at=annotated-v4.0 >actual &&
>+ test_cmp expect actual
>+'
>+
>+test_expect_success 'multiple --points-at are OR-ed together' '
>+ cat >expect <<-\EOF &&
>+ v2.0
>+ v3.0
>+ EOF
>+ git tag --points-at=v2.0 --points-at=v3.0 >actual &&
>+ test_cmp expect actual
>+'
>+
> test_done
>--
>1.7.9.rc2.14.g3da2b
>
^ permalink raw reply
* [PATCH 1/2] submodules: always use a relative path to gitdir
From: Jens Lehmann @ 2012-02-08 22:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Antony Male, Phil Hord
In-Reply-To: <4F32F252.7050105@web.de>
Since recently a submodule with name <name> has its git directory in the
.git/modules/<name> directory of the superproject while the work tree
contains a gitfile pointing there. When the submodule git directory needs
to be cloned because it is not found in .git/modules/<name> the clone
command will write an absolute path into the gitfile. When no clone is
necessary the git directory will be reactivated by the git-submodule.sh
script by writing a relative path into the gitfile.
This is inconsistent, as the behavior depends on the submodule having been
cloned before into the .git/modules of the superproject. A relative path
is preferable here because it allows the superproject to be moved around
without invalidating the gitfile. We do that by always writing the
relative path into the gitfile, which overwrites the absolute path the
clone command may have written there.
This is only the first step to make superprojects movable again like they
were before the separate-git-dir approach was introduced. The second step
is to use a relative path in core.worktree too.
Enhance t7400 to ensure that future versions won't re-add absolute paths
by accident.
While at it also replace an if/else construct evaluating the presence
of the 'reference' option with a single line of bash code.
Reported-by: Antony Male <antony.male@gmail.com>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
git-submodule.sh | 11 ++++-------
t/t7400-submodule-basic.sh | 2 ++
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 9bb2e13..2a93c61 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -160,18 +160,15 @@ module_clone()
if test -d "$gitdir"
then
mkdir -p "$path"
- echo "gitdir: $rel_gitdir" >"$path/.git"
rm -f "$gitdir/index"
else
mkdir -p "$gitdir_base"
- if test -n "$reference"
- then
- git-clone $quiet "$reference" -n "$url" "$path" --separate-git-dir "$gitdir"
- else
- git-clone $quiet -n "$url" "$path" --separate-git-dir "$gitdir"
- fi ||
+ git clone $quiet -n ${reference:+"$reference"} \
+ --separate-git-dir "$gitdir" "$url" "$path" ||
die "$(eval_gettext "Clone of '\$url' into submodule path '\$path' failed")"
fi
+
+ echo "gitdir: $rel_gitdir" >"$path/.git"
}
#
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 695f7af..2b70b22 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -79,6 +79,8 @@ test_expect_success 'submodule add' '
cd addtest &&
git submodule add -q "$submodurl" submod >actual &&
test ! -s actual &&
+ echo "gitdir: ../.git/modules/submod" >expect &&
+ test_cmp expect submod/.git &&
git submodule init
) &&
--
1.7.9.190.gb17a42.dirty
^ permalink raw reply related
* [PATCH 0/2] submodules: Use relative paths to gitdir and work tree
From: Jens Lehmann @ 2012-02-08 22:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Antony Male, Phil Hord
This patch series replaces all absolute paths pointing from submodule work
trees to its gitdir and back with relative paths as discussed in $gmane/187785.
The motivation is to make superprojects movable again. They lost this ability
with the move of the git directory of submodules into the .git/modules directory
of the superproject. While fixing that a bug which would hit when moving the
submodule inside the superproject was also fixed.
Jens Lehmann (2):
submodules: always use a relative path to gitdir
submodules: always use a relative path from gitdir to work tree
git-submodule.sh | 23 ++++++++++++++++-------
t/t7400-submodule-basic.sh | 22 ++++++++++++++++++++++
2 files changed, 38 insertions(+), 7 deletions(-)
--
1.7.9.190.gb17a42.dirty
^ permalink raw reply
* git p4 submit bugs (submit fails due to keyword expansion mismatch and gives bad instructions for how to proceed)
From: Eric Scouten @ 2012-02-08 21:49 UTC (permalink / raw)
To: git
I've been experimenting with git-p4 as a front-end to Perforce as
described in http://www.perforce.com/blog/120113/git-perforce-client,
but two bugs are causing me major headaches:
(1) git-p4 doesn't work with P4 keyword expansion. (I gather there's a
fork of git-p4 that makes it work, but the current official distro
doesn't.) This can lead to some ugly failed submits down the road.
Which leads me to ...
(2) When a git p4 submit fails, the error message for how to resolve
it is bogus. The instructions say "Please resolve and submit the
conflict manually and continue afterwards with git-p4 submit
--continue", but when I do that, I get:
$ git p4 submit --continue
Usage: git-p4 submit [options] [name of git branch to submit into
perforce depot]
git-p4: error: no such option: --continue
Ugh.
(P.S. If there's a better place to post bug reports, etc., please
inform. Thanks.)
-Eric
^ permalink raw reply
* Re: Git documentation at kernel.org
From: Clemens Buchacher @ 2012-02-08 21:34 UTC (permalink / raw)
To: ftpadmin; +Cc: Petr Onderka, git
In-Reply-To: <CAPyqok3USqMxm0gNf_T9vnCoicp9XSwpWUCYJ8jh79h=V_UuOA@mail.gmail.com>
Hi,
Please restore access to the following files when possible. Some sites
are referencing those, including kernel.org itself:
http://www.kernel.org/pub/software/scm/git/docs/git.html
and references therein
http://www.kernel.org/pub/software/scm/git/docs/howto/using-merge-subtree.html
referenced by
https://git.wiki.kernel.org/articles/g/i/t/GitFaq_ebc3.html#How_do_I_clone_a_subdirectory.3F
Also, it would be great if the git wiki could be made editable again.
Thank you for your consideration.
Regards,
Clemens
^ permalink raw reply
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
From: Jeff King @ 2012-02-08 21:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
In-Reply-To: <7vty337rug.fsf@alter.siamese.dyndns.org>
On Mon, Feb 06, 2012 at 10:13:27AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > OK, that's easy enough to do. Should we show lightweight tags to commits
> > for backwards compatibility (and just drop the parse_signature junk in
> > that case)? The showing of blobs or trees is the really bad thing, I
> > think.
>
> For now, dropping 3/3 and queuing this instead...
>
> ---
> Subject: tag: do not show non-tag contents with "-n"
Since jk/maint-tag-show-fixes is still in pu, perhaps we can squash in
this test from my 3/3:
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..0db0f6a 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -586,6 +586,19 @@ test_expect_success \
test_cmp expect actual
'
+test_expect_success 'annotations for blobs are empty' '
+ blob=$(git hash-object -w --stdin <<-\EOF
+ Blob paragraph 1.
+
+ Blob paragraph 2.
+ EOF
+ ) &&
+ git tag tag-blob $blob &&
+ echo "tag-blob " >expect &&
+ git tag -n1 -l tag-blob >actual &&
+ test_cmp expect actual
+'
+
# trying to verify annotated non-signed tags:
test_expect_success GPG \
If we want to be more thorough, I can write up a more complete test
battery making sure tags and commits are both shown, but blobs and trees
are not.
-Peff
^ permalink raw reply related
* Re: [PATCHv4] tag: add --points-at list option
From: Jeff King @ 2012-02-08 20:58 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <1328731972-13137-2-git-send-email-tmgrennan@gmail.com>
On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:
> 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
And probably "v1.0", as well, in this iteration. :)
The patch content itself looks good to me, except:
> --- 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>...]
What's this "column" stuff doing here? The nd/columns topic is still in
"next", isn't it? Did you base this on "next" or "pu"?
Usually topics should be based on master, so they can graduate
independently of each other. In this case, it might make sense to build
on top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is
even necessary here (my fixes ended up not being too closely related, I
think).
Other than that, I think the patch is fine. There are no tests, so
perhaps these should be squashed in:
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..f61e398 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
test_must_fail git tag -v -s
'
+# check points-at
+
+test_expect_success '--points-at cannot be used in non-list mode' '
+ test_must_fail git tag --points-at=v4.0 foo
+'
+
+test_expect_success '--points-at finds lightweight tags' '
+ echo v4.0 >expect &&
+ git tag --points-at v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of commits' '
+ git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
+ echo annotated-v4.0 >expect &&
+ git tag -l --points-at v4.0 "annotated*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of tags' '
+ git tag -m "describing the v4.0 tag object" \
+ annotated-again-v4.0 annotated-v4.0 &&
+ cat >expect <<-\EOF &&
+ annotated-again-v4.0
+ annotated-v4.0
+ EOF
+ git tag --points-at=annotated-v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple --points-at are OR-ed together' '
+ cat >expect <<-\EOF &&
+ v2.0
+ v3.0
+ EOF
+ git tag --points-at=v2.0 --points-at=v3.0 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.9.rc2.14.g3da2b
^ permalink raw reply related
* [PATCHv4] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 20:12 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
In-Reply-To: <20120208185750.GA22220@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 | 52 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 53 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..f3051c7 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,24 @@ static int match_pattern(const char **patterns, const char *ref)
return 0;
}
+static const unsigned char *match_points_at(const char *refname,
+ const unsigned char *sha1)
+{
+ const unsigned char *tagged_sha1 = NULL;
+ struct object *obj;
+
+ if (sha1_array_lookup(&points_at, sha1) >= 0)
+ return sha1;
+ obj = parse_object(sha1);
+ if (!obj)
+ die(_("malformed object at '%s'"), refname);
+ if (obj->type == OBJ_TAG)
+ tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
+ if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+ 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 +162,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
+ if (points_at.nr && !match_points_at(refname, sha1))
+ return 0;
+
if (!filter->lines) {
printf("%s\n", refname);
return 0;
@@ -389,6 +413,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 +473,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 +523,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
* [PATCHv4] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 20:12 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
In-Reply-To: <20120208185750.GA22220@sigill.intra.peff.net>
Please see the following patch that I hoped is the last version of the
"points-at" feature. Thank you for your patience.
Tom Grennan (1):
tag: add --points-at list option
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 52 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 53 insertions(+), 4 deletions(-)
--
1.7.8
^ permalink raw reply
* Re: [StGit PATCH] Parse commit object header correctly
From: Frans Klaver @ 2012-02-08 20:04 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Karl Hasselström, Catalin Marinas,
Andy Green (林安廸), git
In-Reply-To: <4F32A014.1000608@alum.mit.edu>
On Wed, 08 Feb 2012 17:17:24 +0100, Michael Haggerty
<mhagger@alum.mit.edu> wrote:
> On 02/08/2012 11:43 AM, Frans Klaver wrote:
>> On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty
>> <mhagger@alum.mit.edu> wrote:
>>> On 02/08/2012 08:33 AM, Junio C Hamano wrote:
>>>> + line = lines[i].rstrip('\n')
>>>> + ix = line.find(' ')
>>>> + if 0 <= ix:
>>>> + key, value = line[0:ix], line[ix+1:]
>>>
>>> The above five lines can be written
>>>
>>> for line in lines:
>>> if ' ' in line:
>>> key, value = line.rstrip('\n').split(' ', 1)
>>>
>>> or (if the lack of a space should be treated more like an exception)
>>>
>>> for line in lines:
>>> try:
>>> key, value = line.rstrip('\n').split(' ', 1)
>>> except ValueError:
>>> continue
>>
>> This is generally considered more pythonic: "It's easier to ask for
>> forgiveness than to get permission".
>
> Given that Junio explicitly wanted to allow lines with no spaces, I
> assume that lack of a space is not an error but rather a conceivable
> future extension. If my assumption is correct, then it is misleading
> (and inefficient) to handle it via an exception.
I find the documenting more convincing than the efficiency, but from the
phrasing I think you do too.
>>> for line in lines:
>>> if ' ' in line:
>>> key, value = line.split(' ', 1)
>>> if key == 'tree':
>>> cd = cd.set_tree(repository.get_tree(value))
>>> elif key == 'parent':
>>> cd = cd.add_parent(repository.get_commit(value))
>>> elif key == 'author':
>>> cd = cd.set_author(Person.parse(value))
>>> elif key == 'committer':
>>> cd = cd.set_committer(Person.parse(value))
>>> return cd
>>
>> One could also take the recommended python approach for
>> switch/case-like if/elif/else statements:
>>
>> updater = { 'tree': lambda cd, value:
>> cd.set_tree(repository.get_tree(value),
>> 'parent': lambda cd, value:
>> cd.add_parent(repository.get_commit(value)),
>> 'author': lambda cd, value:
>> cd.set_author(Person.parse(value)),
>> 'committer': lambda cd, value:
>> cd.set_committer(Person.parse(value))
>> }
>> for line in lines:
>> try:
>> key, value = line.split(' ', 1)
>> cd = updater[key](cd, value)
>> except ValueError:
>> continue
>> except KeyError:
>> continue
>>
>> It documents about the same, but adds checking on double 'case'
>> statements. The resulting for loop is rather cleaner and the exception
>> approach becomes even more logical. I rather like the result, but I
>> guess it's mostly a matter of taste.
>
> I know this approach and use it frequently, but when one has to resort
> to lambdas and there are only four cases, it becomes IMHO less readable
> than the if..else version.
Well, as I said, its largely a matter of taste; four items is a corner
case to me when thinking maintainability vs. readability. On the other
hand, this doesn't seem like an oft-changing piece of code, so a longer
list of if..elif..else shouldn't be a problem either.
Frans
^ permalink raw reply
* [PATCH] column: Fix some compiler and sparse warnings
From: Ramsay Jones @ 2012-02-08 19:13 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, GIT Mailing-list
Some versions of gcc complain as follows:
CC column.o
column.c: In function `git_config_column':
column.c:313: warning: 'set' might be used uninitialized in \
this function
The 'set' variable is not in fact used uninitialised, but in order to
suppress the warning, we rework the code slightly to ensure gcc does
not mis-diagnose the variable usage.
Also, sparse complains as follows:
SP pager.c
pager.c:134:5: warning: symbol 'term_columns' was not declared. \
Should it be static?
In order to fix the warning, we add an include of the column.h header,
which contains an appropriate extern declaration of term_columns().
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Nguyen,
If you need to re-roll your "nd/columns" branch, could you please squash
this patch (or some variant) into it. Thanks!
[I haven't checked, but I'm guessing that the pager.c change would be
squashed into commit cb0850f (Save terminal width before setting up pager,
04-02-2012), whereas the column.c change would be squashed into commit
ac21f2b (Add git-column and column mode parsing, 04-02-2012)]
ATB,
Ramsay Jones
column.c | 6 +++---
| 1 +
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/column.c b/column.c
index f001021..98328cf 100644
--- a/column.c
+++ b/column.c
@@ -310,16 +310,16 @@ static int parse_option(const char *arg, int len,
{ OPTION, "color", COL_ANSI },
{ OPTION, "dense", COL_DENSE },
};
- int i, set, name_len;
+ int i;
for (i = 0; i < ARRAY_SIZE(opts); i++) {
+ int set = 1, name_len;
+
if (opts[i].type == OPTION) {
if (len > 2 && !strncmp(arg, "no", 2)) {
arg += 2;
len -= 2;
set = 0;
- } else {
- set = 1;
}
}
--git a/pager.c b/pager.c
index 37d554d..fe203a7 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,7 @@
#include "cache.h"
#include "run-command.h"
#include "sigchain.h"
+#include "column.h"
#ifndef DEFAULT_PAGER
#define DEFAULT_PAGER "less"
--
1.7.9
^ permalink raw reply related
* Re: [PATCHv3] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 18:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
In-Reply-To: <20120208184332.GF6264@tgrennan-laptop>
On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:
>On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:
>>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
>>
>>Also, should we be producing an error if !obj? It would indicate a tag
>>that points to a bogus object.
>
>I think the test of (obj) is redundant as this should be caught
>by get_sha1() in parse_opt_points_at()
I'm wrong. That tests the sha of the point-at argument, not the
sha/objects of the refs/tags entry. I'll add...
if (!obj)
die(_("invalid tag, 'refs/tags/%s'"), refname);
--
TomG
^ permalink raw reply
* Re: [PATCHv3] tag: add --points-at list option
From: Jeff King @ 2012-02-08 18:57 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <20120208184332.GF6264@tgrennan-laptop>
On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:
> >Though we provide a null_sha1 global already. So doing:
> >
> > const unsigned char *tagged_sha1 = null_sha1;
> >
> >would be sufficient.
>
> Or just initialize at test tagged_sha1 with NULL.
Oh yeah, that is even better.
> >That being said, I don't know why you want to do both lookups in the
> >same loop of the points_at. If it's a lightweight tag and the tag
> >matches, you can get away with not parsing the object at all (although
> >to be fair, that is the minority case, so it is unlikely to matter).
>
> Yes, I think your saying that the lightweight search could go before the
> tag object search like this.
Exactly, though:
> static const unsigned char *match_points_at(const unsigned char *sha1)
> {
> const unsigned char *tagged_sha1 = NULL;
> struct object *obj = parse_object(sha1);
>
> if (sha1_array_lookup(&points_at, sha1) >= 0)
> return sha1;
> if (obj && obj->type == OBJ_TAG)
> tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
You can delay the relatively expensive parse_object until you find the
results of the first lookup (though like I said earlier, it is unlikely to
matter, as it only helps in the positive-match case. Out of N tags, you
will likely end up parsing N-1 of them anyway).
> >Also, should we be producing an error if !obj? It would indicate a tag
> >that points to a bogus object.
>
> I think the test of (obj) is redundant as this should be caught
> by get_sha1() in parse_opt_points_at()
No, it's not redundant. get_sha1 is purely about looking up the name and
finding a sha1. parse_object is about looking up the object represented
by that sha1 in the object db. get_sha1 can sometimes involve parsing
objects (e.g., looking for "foo^1" will need to parse the commit object
at "foo"), but does not have to.
Besides which, you are not calling parse_object on the sha1 from
--points-at, but rather the sha1 for each tag ref given to us by
for_each_tag_ref.
> >Why write your own linear search? sha1_array_lookup will do a binary
> >search for you.
>
> Well, it's only a linear search of the points_at command arguments.
> But by that reasoning, might as well do two sha1_array_lookups like
> above and save some code b/c "less code is always better"(TM).
Right. I expect the N to be small in this case, so I doubt it matters.
But two sha1_array_lookups is still asymptotically smaller, because the
expensive operation is hashcmp(). So two binary searches is O(2*lg n),
whereas a linear walk with 2 hashcmps per item is O(2*n).
> >Other than that, the patch looks OK to me.
>
> Thanks, I'll send what I hope to be the final version later today.
Thanks for working on this and being so responsive to review.
-Peff
^ permalink raw reply
* Re: [PATCH] Fix build problems related to profile-directed optimization
From: Ted Ts'o @ 2012-02-08 18:53 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <1328508017-7277-1-git-send-email-tytso@mit.edu>
Junio, any comments on my most recent spin of this patch? Any changes
you'd like to see?
Thanks,
- Ted
^ permalink raw reply
* Re: [PATCHv3] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 18:43 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
In-Reply-To: <20120208154442.GB8773@sigill.intra.peff.net>
On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
>
>> +static const unsigned char *match_points_at(const unsigned char *sha1)
>> +{
>> + int i;
>> + const unsigned char *tagged_sha1 = (unsigned char*)"";
>> + struct object *obj = parse_object(sha1);
>> +
>> + if (obj && obj->type == OBJ_TAG)
>> + tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
>
>This is not safe. A sha1 is not NUL-terminated, but is rather _always_
>20 bytes. So when the object is not a tag, you do the hashcmp against
>your single-byte string literal above, and we end up comparing whatever
>garbage is in the data segment after the string literal.
Yikes! That was dumb.
>What you want instead is the all-zeros sha1, like:
>
> const unsigned char null_sha1[20] = { 0 };
>
>Though we provide a null_sha1 global already. So doing:
>
> const unsigned char *tagged_sha1 = null_sha1;
>
>would be sufficient.
Or just initialize at test tagged_sha1 with NULL.
static const unsigned char *match_points_at(const unsigned char *sha1)
{
int i;
const unsigned char *tagged_sha1 = NULL;
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 (tagged_sha1 &&
!hashcmp(points_at.sha1[i], tagged_sha1))
return tagged_sha1;
return NULL;
}
>That being said, I don't know why you want to do both lookups in the
>same loop of the points_at. If it's a lightweight tag and the tag
>matches, you can get away with not parsing the object at all (although
>to be fair, that is the minority case, so it is unlikely to matter).
Yes, I think your saying that the lightweight search could go before the
tag object search like this.
static const unsigned char *match_points_at(const unsigned char *sha1)
{
const unsigned char *tagged_sha1 = NULL;
struct object *obj = parse_object(sha1);
if (sha1_array_lookup(&points_at, sha1) >= 0)
return sha1;
if (obj && obj->type == OBJ_TAG)
tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
return tagged_sha1;
return NULL;
}
>Also, should we be producing an error if !obj? It would indicate a tag
>that points to a bogus object.
I think the test of (obj) is redundant as this should be caught
by get_sha1() in parse_opt_points_at()
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;
}
>> + for (i = 0; i < points_at.nr; i++)
>> + if (!hashcmp(points_at.sha1[i], sha1))
>> + return sha1;
>> + else if (!hashcmp(points_at.sha1[i], tagged_sha1))
>> + return tagged_sha1;
>> + return NULL;
>
>Why write your own linear search? sha1_array_lookup will do a binary
>search for you.
Well, it's only a linear search of the points_at command arguments.
But by that reasoning, might as well do two sha1_array_lookups like
above and save some code b/c "less code is always better"(TM).
>Other than that, the patch looks OK to me.
Thanks, I'll send what I hope to be the final version later today.
--
TomG
^ 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-08 17:34 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <CACsJy8DSM3kPXJ4oYexCLs5qp6YdZ4Mf9RrGo78a0tHkRaTe4g@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 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.
Ok, the strategy part is now behind us, but I have this slight suspicion
(I didn't look at the code nor tried it out myself---I don't have time to
do this myself today) that using this codepath might result in a corrupt
cache-tree, whose entries point at a section of the index it covers as a
subtree of the whole project but with incorrect counters or something like
that. It would be good to make sure this "just ignoring i-t-a" is doing
the right thing not to the resulting commit, but in the resulting index as
well, before we go forward with this change.
^ permalink raw reply
* Re: [StGit PATCH] Parse commit object header correctly
From: Michael Haggerty @ 2012-02-08 16:17 UTC (permalink / raw)
To: Frans Klaver
Cc: Junio C Hamano, Karl Hasselström, Catalin Marinas,
"Andy Green (林安廸)", git
In-Reply-To: <CAH6sp9P=vNjLycgzoWzRbeEsW-kQ5e4HgGYf2jP1+u9rtWV4dg@mail.gmail.com>
On 02/08/2012 11:43 AM, Frans Klaver wrote:
> On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 02/08/2012 08:33 AM, Junio C Hamano wrote:
>>> + line = lines[i].rstrip('\n')
>>> + ix = line.find(' ')
>>> + if 0 <= ix:
>>> + key, value = line[0:ix], line[ix+1:]
>>
>> The above five lines can be written
>>
>> for line in lines:
>> if ' ' in line:
>> key, value = line.rstrip('\n').split(' ', 1)
>>
>> or (if the lack of a space should be treated more like an exception)
>>
>> for line in lines:
>> try:
>> key, value = line.rstrip('\n').split(' ', 1)
>> except ValueError:
>> continue
>
> This is generally considered more pythonic: "It's easier to ask for
> forgiveness than to get permission".
Given that Junio explicitly wanted to allow lines with no spaces, I
assume that lack of a space is not an error but rather a conceivable
future extension. If my assumption is correct, then it is misleading
(and inefficient) to handle it via an exception.
>>> + if key == 'tree':
>>> + cd = cd.set_tree(repository.get_tree(value))
>>> + elif key == 'parent':
>>> + cd = cd.add_parent(repository.get_commit(value))
>>> + elif key == 'author':
>>> + cd = cd.set_author(Person.parse(value))
>>> + elif key == 'committer':
>>> + cd = cd.set_committer(Person.parse(value))
>>
>> All in all, I would recommend something like (untested):
>>
>> @return: A new L{CommitData} object
>> @rtype: L{CommitData}"""
>> cd = cls(parents = [])
>> lines = []
>> raw_lines = s.split('\n')
>> # Collapse multi-line header lines
>> for i, line in enumerate(raw_lines):
>> if not line:
>> cd.set_message('\n'.join(raw_lines[i+1:]))
>> break
>> if line.startswith(' '):
>> # continuation line
>> lines[-1] += '\n' + line[1:]
>> else:
>> lines.append(line)
>>
>> for line in lines:
>> if ' ' in line:
>> key, value = line.split(' ', 1)
>> if key == 'tree':
>> cd = cd.set_tree(repository.get_tree(value))
>> elif key == 'parent':
>> cd = cd.add_parent(repository.get_commit(value))
>> elif key == 'author':
>> cd = cd.set_author(Person.parse(value))
>> elif key == 'committer':
>> cd = cd.set_committer(Person.parse(value))
>> return cd
>
> One could also take the recommended python approach for
> switch/case-like if/elif/else statements:
>
> updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),
> 'parent': lambda cd, value:
> cd.add_parent(repository.get_commit(value)),
> 'author': lambda cd, value: cd.set_author(Person.parse(value)),
> 'committer': lambda cd, value:
> cd.set_committer(Person.parse(value))
> }
> for line in lines:
> try:
> key, value = line.split(' ', 1)
> cd = updater[key](cd, value)
> except ValueError:
> continue
> except KeyError:
> continue
>
> It documents about the same, but adds checking on double 'case'
> statements. The resulting for loop is rather cleaner and the exception
> approach becomes even more logical. I rather like the result, but I
> guess it's mostly a matter of taste.
I know this approach and use it frequently, but when one has to resort
to lambdas and there are only four cases, it becomes IMHO less readable
than the if..else version.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox