* Re: [PATCH] Use longer alias names in subdirectory tests
From: Junio C Hamano @ 2012-12-29 3:42 UTC (permalink / raw)
To: Aaron Schrab; +Cc: git
In-Reply-To: <1356735786-24001-1-git-send-email-aaron@schrab.com>
Aaron Schrab <aaron@schrab.com> writes:
> When testing aliases in t/t1020-subdirectory.sh use longer names so that
> they're less likely to conflict with a git-* command somewhere in the
> $PATH.
Thanks.
In the longer term we might want to rethink the way we run the tests
so that random $PATH the user has has less chance of interacting
with our tests (we had a similar topic around completion output
recently), but until that happens, I think this is a good change.
^ permalink raw reply
* Re: [PATCH v3 19/19] Add git-check-ignore sub-command
From: Adam Spiers @ 2012-12-29 3:32 UTC (permalink / raw)
To: git list
In-Reply-To: <20121229012352.GA20379@pacific.linksys.moosehall>
On Sat, Dec 29, 2012 at 01:23:52AM +0000, Adam Spiers wrote:
> FYI, attached is the diff between check-ignore-v3 and my current
> check-ignore, which is available at github:
>
> https://github.com/aspiers/git/commits/check-ignore
[snipped]
> diff --git a/pathspec.c b/pathspec.c
> index 6724121..3789b14 100644
> --- a/pathspec.c
> +++ b/pathspec.c
[snipped]
> -void validate_path(const char *path, const char *prefix)
> +void die_if_path_beyond_symlink(const char *path, const char *prefix)
[snipped]
> diff --git a/pathspec.h b/pathspec.h
> index c251441..1961b19 100644
> --- a/pathspec.h
> +++ b/pathspec.h
> @@ -1,6 +1,11 @@
> +#ifndef PATHSPEC_H
> +#define PATHSPEC_H
> +
> extern char *find_used_pathspec(const char **pathspec);
> extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
> extern const char *treat_gitlink(const char *path);
> extern void treat_gitlinks(const char **pathspec);
> extern void validate_path(const char *path, const char *prefix);
^^^^^^^^^^^^^
I forgot to rename this one. Will be fixed in v4.
^ permalink raw reply
* Re: [PATCH 1/4] hooks: Add function to check if a hook exists
From: Junio C Hamano @ 2012-12-29 2:08 UTC (permalink / raw)
To: Aaron Schrab; +Cc: git
In-Reply-To: <1356735452-21667-2-git-send-email-aaron@schrab.com>
Aaron Schrab <aaron@schrab.com> writes:
> Create find_hook() function to determine if a given hook exists and is
> executable. If it is the path to the script will be returned, otherwise
> NULL is returned.
Sounds like a sensible thing to do. To make sure the API is also
sensible, all the existing hooks should be updated to use this API,
no?
> This is in support for an upcoming run_hook_argv() function which will
> expect the full path to the hook script as the first element in the
> argv_array.
There is currently a public function called run_hook() that squats
on the good name with a kludgy API that is too specific to using
separate index file. Back when it was a private helper in the
implementation of "git commit", it was perfectly fine, but it was
exported without giving much thought on the API.
If you are introducing a new run_hook_* function, give it a generic
enough API that lets all the existing hook callers to use it. I
would imagine that the API requirement may be modelled after
run_command() API so that we can pass argv[] and tweak the hook's
environ[], as well as feeding its stdin and possibly reading from
its stdout. That would be very useful.
^ permalink raw reply
* Re: [PATCH 0/4] pre-push hook support
From: Junio C Hamano @ 2012-12-29 2:01 UTC (permalink / raw)
To: Aaron Schrab; +Cc: git
In-Reply-To: <1356735452-21667-1-git-send-email-aaron@schrab.com>
Aaron Schrab <aaron@schrab.com> writes:
> There have been at least a couple of submissions to add support for a
> pre-push hook, which were rejected at least partially because they didn't
> provide enough information to a hook script for it to determine what was
> to be pushed any better than a separate wrapper around the 'git push'
> command would be able to do. In this series I attempt to address that
> problem.
>
> The first two patches in this series do a little bit of refactoring in
> order to make it easier to call hooks with a variable number of arguments.
>
> The third patch actually adds support for calling a pre-push hook. If it
> exists, it will be called with the name and URL of the destination remote
> (if a named remote isn't being used, the URL will be supplied for both)
> followed by another argument for each ref being pushed; these arguments
> take the form:
>
> <local ref>:<local sha1>:<remote ref>:<remote sha1>
One lesson we learned long time ago while doing hooks is to avoid
unbound number of command line arguments and instead feed them from
the standard input. I think this should do the same.
> This should provide enough information for a script to easily determine
> the set of commits that is being pushed, and thus make a decision if that
> should be allowed.
How does the hook communicate its decision to the calling Git?
Will it be "all-or-none", or "I'll allow these but not those"?
^ permalink raw reply
* Re: [PATCH v3 18/19] setup.c: document get_pathspec()
From: Junio C Hamano @ 2012-12-29 1:36 UTC (permalink / raw)
To: Adam Spiers; +Cc: git list
In-Reply-To: <CAOkDyE89fm5_z2V=VW5n+8XAvB6tE+DqciXttbhX29X8mWjXTQ@mail.gmail.com>
Adam Spiers <git@adamspiers.org> writes:
> I've added this sentence to the top of the comments above
> get_pathspec():
>
> /*
> * N.B. get_pathspec() is deprecated in favor of the "struct pathspec"
> * based interface - see pathspec_magic above.
> *
> [...]
>
> That should be sufficient to discourage people from adding new users
> of get_pathspec().
Yeah, that sounds sensible.
Thanks, and happy new year to you ;-)
^ permalink raw reply
* Re: [PATCH v3 19/19] Add git-check-ignore sub-command
From: Adam Spiers @ 2012-12-29 1:23 UTC (permalink / raw)
To: git list
In-Reply-To: <7vtxr5ho01.fsf@alter.siamese.dyndns.org>
On Fri, Dec 28, 2012 at 01:21:02PM -0800, Junio C Hamano wrote:
> Adam Spiers <git@adamspiers.org> writes:
> > +static void output_exclude(const char *path, struct exclude *exclude)
> > +{
> > + char *bang = exclude->flags & EXC_FLAG_NEGATIVE ? "!" : "";
> > + char *dir = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
>
> That's inconsistent. Either s/bang/negated/ or s/dir/slash/ (but
> obviously not both).
OK. I'll make the use of parentheses there consistent too.
> > +static int check_ignore(const char *prefix, const char **pathspec)
> > +{
> > + struct dir_struct dir;
> > +...
> > + if (pathspec) {
> > +...
> > + } else {
> > + printf("no pathspec\n");
> > + }
>
> Is this an error message, an informative warning, or what? The
> command is presumably for script consumption downstream of a pipe.
> Does it make sense to emit this message to their standard input?
> Does it even have to be output? Especially what should happen when
> the user says "--quiet"?
>
> Perhaps
>
> if (!quiet)
> fprintf(stderr, "no pathspec given.\n");
>
> or something?
Hmm. I suspect this was an unfinished remnant of one of my early
prototypes, because this code path never gets hit. There's even a
test which checks that a fatal error is generated when no pathspecs
are given via argv.
However, the test for behaviour when no pathspecs are given via
--stdin *is* missing. In this case, I think the code you suggest
above makes sense for generating a warning, and the existing behaviour
of returning an exit code of 1 also seems correct. I have added your
code and a test to cover it.
> > +int cmd_check_ignore(int argc, const char **argv, const char *prefix)
> > +{
> > + int num_ignored = 0;
>
> I do not think you need to initialize this one.
Fixed.
> > + if (stdin_paths) {
> > + num_ignored = check_ignore_stdin_paths(prefix);
> > + } else {
> > + num_ignored = check_ignore(prefix, argv);
> > + maybe_flush_or_die(stdout, "ignore to stdout");
> > + }
> > +
> > + return num_ignored > 0 ? 0 : 1;
>
> Given that num_ignored will always be >=0, that is a funny way to
> say
>
> return !num_ignored;
Personally I find that harder to read, but I'm not a regular C
programmer so I'll defer to your sense of style.
> or if you plan to report a non-fatal error as negative return from
> the two check_ignore* functions,
I don't think that's necessary here, but I'll bear it in mind.
> > +stderr_contains () {
> > + regexp="$1"
> > + if grep -q "$regexp" "$HOME/stderr"
>
> Please do not use "grep -q"; the output from commands in test
> harness is normally hidden already. This only makes script more
> cluttered and robs debuggers of potentially useful output when the
> test script is run with "-v".
Fixed.
> > +test_check_ignore () {
> > + args="$1" expect_code="${2:-0}" global_args="$3"
> > +
> > + init_vars &&
> > + rm -f "$HOME/stdout" "$HOME/stderr" "$HOME/cmd" &&
> > + echo git $global_args check-ignore $quiet_opt $verbose_opt $args \
> > + >"$HOME/cmd" &&
>
> Does a debugger needs this "cmd" file when we already have "-v" option?
Yes, I think it's useful; for example:
$ ./t0008-ignores.sh -i -v
[... test fails ...]
$ cd trash\ directory.t0008-ignores
$ gdb --args ../../$(<cmd)
> > +test_expect_success_multi () {
> > + prereq=
> > + if test $# -eq 4
> > + then
> > + prereq=$1
> > + shift
> > + fi
> > + testname="$1" expect_verbose="$2" code="$3"
> > +
> > + expect=$( echo "$expect_verbose" | sed -e 's/.* //' )
> > +
> > + test_expect_success $prereq "$testname" "
> > + expect '$expect' &&
> > + $code
> > + "
>
> This is brittle when $expect may have single quotes, isn't it?
Currently $expect will never have single quotes, but I grant this may
change in the future.
> Perhaps
>
> test_expect_success $prereq "$testname" '
> expect "$expect" && eval "$code"
> '
>
> may fix it,
Yes that works, thanks.
> but in general I hate to see each test script trying to
> invent their own mini-harness like this (and getting them wrong).
test_expect_success_multi is specific to check-ignore and avoids a
huge amount of code duplication. I can't think of a better approach
but if you can then I'm all ears.
I believe this only leaves two items from your review of v3 which are
yet to be resolved:
1. What should validate_path() should be renamed to in order to avoid
ambiguity with other path validation functions? Currently my
preferred choice is die_if_path_beyond_symlink() but I don't really
mind - I'll go with that unless I hear someone prefers another
name.
2. The now-public functions fill_pathspec_matches() and
find_used_pathspec(), need to be documented, and in particular need
to mention that they traverse the index not a tree or the working
directory. I'll work on improving my understanding of these
functions to the point where I can document them accurately (but
hints are still welcome).
Once these are both resolved, I'll re-roll patches 16--19 only of the
v3 series, labelled as v4.
FYI, attached is the diff between check-ignore-v3 and my current
check-ignore, which is available at github:
https://github.com/aspiers/git/commits/check-ignore
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index c825736..06e250e 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -27,8 +27,8 @@ static const struct option check_ignore_options[] = {
static void output_exclude(const char *path, struct exclude *exclude)
{
- char *bang = exclude->flags & EXC_FLAG_NEGATIVE ? "!" : "";
- char *dir = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
+ char *bang = exclude->flags & EXC_FLAG_NEGATIVE ? "!" : "";
+ char *slash = exclude->flags & EXC_FLAG_MUSTBEDIR ? "/" : "";
if (!null_term_line) {
if (!verbose) {
write_name_quoted(path, stdout, '\n');
@@ -36,7 +36,7 @@ static void output_exclude(const char *path, struct exclude *exclude)
quote_c_style(exclude->el->src, NULL, stdout, 0);
printf(":%d:%s%s%s\t",
exclude->srcpos,
- bang, exclude->pattern, dir);
+ bang, exclude->pattern, slash);
quote_c_style(path, NULL, stdout, 0);
fputc('\n', stdout);
}
@@ -47,7 +47,7 @@ static void output_exclude(const char *path, struct exclude *exclude)
printf("%s%c%d%c%s%s%s%c%s%c",
exclude->el->src, '\0',
exclude->srcpos, '\0',
- bang, exclude->pattern, dir, '\0',
+ bang, exclude->pattern, slash, '\0',
path, '\0');
}
}
@@ -57,8 +57,10 @@ static int check_ignore(const char *prefix, const char **pathspec)
{
struct dir_struct dir;
const char *path;
- char *seen = NULL;
- int num_ignored = 0;
+ char *seen;
+ int num_ignored = 0, i;
+ struct path_exclude_check check;
+ struct exclude *exclude;
/* read_cache() is only necessary so we can watch out for submodules. */
if (read_cache() < 0)
@@ -68,38 +70,36 @@ static int check_ignore(const char *prefix, const char **pathspec)
dir.flags |= DIR_COLLECT_IGNORED;
setup_standard_excludes(&dir);
- if (pathspec) {
- int i;
- struct path_exclude_check check;
- struct exclude *exclude;
-
- path_exclude_check_init(&check, &dir);
- if (!seen)
- seen = find_used_pathspec(pathspec);
- for (i = 0; pathspec[i]; i++) {
- const char *full_path;
- path = pathspec[i];
- full_path = prefix_path(prefix, prefix
- ? strlen(prefix) : 0, path);
- full_path = treat_gitlink(full_path);
- validate_path(full_path, prefix);
- if (!seen[i] && path[0]) {
- int dtype = DT_UNKNOWN;
- exclude = last_exclude_matching_path(&check, full_path,
- -1, &dtype);
- if (exclude) {
- if (!quiet)
- output_exclude(path, exclude);
- num_ignored++;
- }
+ if (!pathspec || !*pathspec) {
+ if (!quiet)
+ fprintf(stderr, "no pathspec given.\n");
+ return 0;
+ }
+
+ path_exclude_check_init(&check, &dir);
+ seen = find_used_pathspec(pathspec);
+ for (i = 0; pathspec[i]; i++) {
+ const char *full_path;
+ path = pathspec[i];
+ full_path = prefix_path(prefix, prefix
+ ? strlen(prefix) : 0, path);
+ full_path = treat_gitlink(full_path);
+ die_if_path_beyond_symlink(full_path, prefix);
+ if (!seen[i] && path[0]) {
+ int dtype = DT_UNKNOWN;
+ exclude = last_exclude_matching_path(&check, full_path,
+ -1, &dtype);
+ if (exclude) {
+ if (!quiet)
+ output_exclude(path, exclude);
+ num_ignored++;
}
}
- free(seen);
- clear_directory(&dir);
- path_exclude_check_clear(&check);
- } else {
- printf("no pathspec\n");
}
+ free(seen);
+ clear_directory(&dir);
+ path_exclude_check_clear(&check);
+
return num_ignored;
}
@@ -136,7 +136,7 @@ static int check_ignore_stdin_paths(const char *prefix)
int cmd_check_ignore(int argc, const char **argv, const char *prefix)
{
- int num_ignored = 0;
+ int num_ignored;
git_config(git_default_config, NULL);
@@ -144,7 +144,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
check_ignore_usage, 0);
if (stdin_paths) {
- if (0 < argc)
+ if (argc > 0)
die(_("cannot specify pathnames with --stdin"));
} else {
if (null_term_line)
@@ -166,5 +166,5 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
maybe_flush_or_die(stdout, "ignore to stdout");
}
- return num_ignored > 0 ? 0 : 1;
+ return !num_ignored;
}
diff --git a/pathspec.c b/pathspec.c
index 6724121..3789b14 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -36,9 +36,10 @@ char *find_used_pathspec(const char **pathspec)
}
/*
- * Check whether path refers to a submodule, or something inside a
- * submodule. If the former, returns the path with any trailing slash
- * stripped. If the latter, dies with an error message.
+ * Check the index to see whether path refers to a submodule, or
+ * something inside a submodule. If the former, returns the path with
+ * any trailing slash stripped. If the latter, dies with an error
+ * message.
*/
const char *treat_gitlink(const char *path)
{
@@ -78,9 +79,9 @@ void treat_gitlinks(const char **pathspec)
/*
* Dies if the given path refers to a file inside a symlinked
- * directory.
+ * directory in the index.
*/
-void validate_path(const char *path, const char *prefix)
+void die_if_path_beyond_symlink(const char *path, const char *prefix)
{
if (has_symlink_leading_path(path, strlen(path))) {
int len = prefix ? strlen(prefix) : 0;
@@ -90,7 +91,8 @@ void validate_path(const char *path, const char *prefix)
/*
* Normalizes argv relative to prefix, via get_pathspec(), and then
- * runs validate_path() on each path in the normalized list.
+ * runs die_if_path_beyond_symlink() on each path in the normalized
+ * list.
*/
const char **validate_pathspec(const char **argv, const char *prefix)
{
@@ -99,7 +101,7 @@ const char **validate_pathspec(const char **argv, const char *prefix)
if (pathspec) {
const char **p;
for (p = pathspec; *p; p++) {
- validate_path(*p, prefix);
+ die_if_path_beyond_symlink(*p, prefix);
}
}
diff --git a/pathspec.h b/pathspec.h
index c251441..1961b19 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -1,6 +1,11 @@
+#ifndef PATHSPEC_H
+#define PATHSPEC_H
+
extern char *find_used_pathspec(const char **pathspec);
extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
extern const char *treat_gitlink(const char *path);
extern void treat_gitlinks(const char **pathspec);
extern void validate_path(const char *path, const char *prefix);
extern const char **validate_pathspec(const char **argv, const char *prefix);
+
+#endif /* PATHSPEC_H */
diff --git a/setup.c b/setup.c
index 03d6d5c..9570147 100644
--- a/setup.c
+++ b/setup.c
@@ -250,8 +250,12 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char
}
/*
- * prefix - a path relative to the root of the working tree
- * pathspec - a list of paths underneath the prefix path
+ * N.B. get_pathspec() is deprecated in favor of the "struct pathspec"
+ * based interface - see pathspec_magic above.
+ *
+ * Arguments:
+ * - prefix - a path relative to the root of the working tree
+ * - pathspec - a list of paths underneath the prefix path
*
* Iterates over pathspec, prepending each path with prefix,
* and return the resulting list.
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 3937ca4..8780b1e 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -39,7 +39,7 @@ test_stderr () {
stderr_contains () {
regexp="$1"
- if grep -q "$regexp" "$HOME/stderr"
+ if grep "$regexp" "$HOME/stderr"
then
return 0
else
@@ -86,10 +86,10 @@ test_expect_success_multi () {
expect=$( echo "$expect_verbose" | sed -e 's/.* //' )
- test_expect_success $prereq "$testname" "
- expect '$expect' &&
- $code
- "
+ test_expect_success $prereq "$testname" '
+ expect "$expect" &&
+ eval "$code"
+ '
for quiet_opt in '-q' '--quiet'
do
@@ -164,6 +164,15 @@ test_expect_success_multi 'empty command line' '' '
stderr_contains "fatal: no path specified"
'
+test_expect_success_multi '--stdin with empty STDIN' '' '
+ test_check_ignore "--stdin" 1 </dev/null &&
+ if test -n "$quiet_opt"; then
+ test_stderr ""
+ else
+ test_stderr "no pathspec given."
+ fi
+'
+
test_expect_success '-q with multiple args' '
expect "" &&
test_check_ignore "-q one two" 128 &&
^ permalink raw reply related
* Re: Lockless Refs?
From: Martin Fick @ 2012-12-29 1:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, Jeff King, git
In-Reply-To: <7vlicijepv.fsf@alter.siamese.dyndns.org>
On Friday, December 28, 2012 09:58:36 am Junio C Hamano
wrote:
> Martin Fick <mfick@codeaurora.org> writes:
> > 3) To create a ref, it must be renamed from the null
> > file (sha 0000...) to the new value just as if it were
> > being updated from any other value, but there is one
> > extra condition: before renaming the null file, a full
> > directory scan must be done to ensure that the null
> > file is the only file in the directory...
>
> While you are scanning this directory to make sure it is
> empty,
The objective is not to scan for an empty dir, but to scan
for the existence of only the null file.
> I am contemplating to create the same ref with a
> different value. You finished checking but haven't
> created the null.
The scan needs to happen after creating the null, not before,
so I don't believe the rest of the scenario below is possible
then?
> I have also scanned, created the null
> and renamed it to my value. Now you try to create the
> null, succeed, and then rename. We won't know which of
> the two non-null values are valid, but worse yet, I think
> one of them should have failed in the first place.
> Sounds like we would need some form of locking around
> here. Is your goal "no locks", or "less locks"?
(answered below)
> > I don't know how this new scheme could be made to work
> > with the current scheme,...
>
> It is much more important to know if/why yours is better
> than the current scheme in the first place.
The goal is: "no locks which do not have a clearly defined
reliable recovery procedure".
Stale locks without a reliable recovery procedure will lead
to stolen locks. At this point it is only a matter of luck
whether this leads to data loss or not. So I do hope to
convince people first that the current scheme is bad, not that
my scheme is better! My scheme was proposed to get people
thinking that we may not have to use locks to get reliable
updates.
> Without an
> analysis on how the new scheme interacts with the packed
> refs and gives better behaviour, that is kinda difficult.
Fair enough. I will attempt this if the basic idea seems at
least sane? I do hope that eventually the packed-refs piece
and its locking will be reconsidered also; as Michael pointed
out it has issues already. So, I am hoping to get people
thinking more about lockless approaches to all the pieces. I
think I have some solutions to some of the other pieces also,
but I don't want to overwhelm the discussion all at once
(especially if my first piece is shown to be flawed, or if no
one has any interest in eliminating the current locks?)
> I think transition plans can wait until that is done. If
> it is not even marginally better, we do not have to worry
> about transitioning at all. If it is only marginally
> better, the transition has to be designed to be no impact
> to the existing repositories. If it is vastly better, we
> might be able to afford a flag day.
OK, makes sense, I jumped the gun a bit,
-Martin
^ permalink raw reply
* Re: [PATCH v3 18/19] setup.c: document get_pathspec()
From: Adam Spiers @ 2012-12-29 0:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
In-Reply-To: <CAOkDyE-UXGhe1aiWy_1_y7cvrmfvivSBxY9LHudOmeZD=M-12A@mail.gmail.com>
On Fri, Dec 28, 2012 at 8:40 PM, Adam Spiers <git@adamspiers.org> wrote:
> On Fri, Dec 28, 2012 at 8:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Adam Spiers <git@adamspiers.org> writes:
>>
>>> Since we have just created a new pathspec-handling library, now is a
>>> good time to add some comments explaining get_pathspec().
>>>
>>> Signed-off-by: Adam Spiers <git@adamspiers.org>
>>> ---
>>
>> Yes, but we would rather not to see new users of this function added
>> to our codebase in its current form, as explained in the nearby
>> comment. We would want to migrate everybody to "struct pathspec"
>> based interface to support magic pathspecs in the longer term.
>
> I see. Please feel free to drop that patch from the series or amend
> as you see fit.
I've added this sentence to the top of the comments above
get_pathspec():
/*
* N.B. get_pathspec() is deprecated in favor of the "struct pathspec"
* based interface - see pathspec_magic above.
*
[...]
That should be sufficient to discourage people from adding new users
of get_pathspec().
^ permalink raw reply
* Re: [RFC] pack-objects: compression level for non-blobs
From: Jeff King @ 2012-12-29 0:41 UTC (permalink / raw)
To: David Michael Barr; +Cc: Git Mailing List
In-Reply-To: <1353911154-23495-1-git-send-email-b@rr-dav.id.au>
On Mon, Nov 26, 2012 at 05:25:54PM +1100, David Michael Barr wrote:
> The intent is to allow selective recompression of pack data.
> For small objects/deltas the overhead of deflate is significant.
> This may improve read performance for the object graph.
>
> I ran some unscientific experiments with the chromium repository.
> With pack.graphcompression = 0, there was a 2.7% increase in pack size.
> I saw a 35% improvement with cold caches and 43% otherwise on git log --raw.
There wasn't much response to this, but those numbers are encouraging. I
was curious to replicate them, as well as to break it down by trees and
commits. I also wanted to test on more repositories, as well as on both
SSDs and spinning disks (for cold-cache numbers). Maybe that will catch
more people's interest.
As you mentioned in your follow-up, I ran into the "delta size changed"
problem. Not sure if it is related, but I noticed here:
> @@ -379,6 +396,13 @@ static unsigned long write_reuse_object(struct sha1file *f, struct object_entry
> offset += entry->in_pack_header_size;
> datalen -= entry->in_pack_header_size;
>
> + if (!pack_to_stdout &&
> + pack_graph_compression_seen &&
> + check_pack_compressed(p, &w_curs, offset) != !!compression_level(entry->actual_type)) {
> + unuse_pack(&w_curs);
> + return write_no_reuse_object(f, entry, limit, usable_delta);
> + }
> +
...that we seem to re-compress more than necessary. If I instrument that
block with a message to stderr and run "git repack -ad" repeatedly
without changing the config in between, runs after the first should
never re-compress, right? But they seem to. I'm not sure if your
check_pack_compressed heuristic is off or something else. It may or may
not be related to the "delta sized change" failure.
But we can leave this side a bit for a moment. Conceptually there are
two interesting things going on in your patch:
1. Per-object-type compression levels
2. Auto-recompression when levels change.
We can figure out (2) later. The meat of the idea is (1), and the patch
for that is much simpler. In fact, we can test it out with entirely
stock git by creating separate tree, commit, and blob packs, each with
different compression. So that's what I did for my timing, just to keep
things simple.
I timed git.git, linux-2.6.git, and WebKit.git. For each repo, I tested
it with four pack compression scenarios:
1. all objects at -1 (zlib default)
2. commits at 0, everything else at -1
3. trees at 0, everything else at -1
4. commits and trees at 0, everything else at -1
For each scenario, I timed "git rev-list --count --all" to traverse all
commits (which roughly models things like merge-base, ahead/behind
counts, etc), and then the same thing with "--objects" to traverse all
objects (which would roughly match what "git prune" or the "counting
objects" phase of packing would do). For each command, I timed both warm
and cold disk cache (the latter via "echo 3 >/proc/sys/vm/drop_caches").
Each timing is a best-of-five. The timings were done on a machine with
an SSD (which probably matters for cold-cache; I have some spinning disk
numbers later).
Here are the git.git numbers:
Pack | Size | Cold Revs | Warm Revs | Cold Objects | Warm Objects
-------+---------------+-------------+-------------+--------------+--------------
none | 41.48 | 0.78 | 0.33 | 2.35 | 1.94
commit | 49.34 (+18%) | 0.57 (-26%) | 0.09 (-74%) | 2.48 (+5%) | 1.70 (-12%)
tree | 45.43 (+9%) | 0.80 (+3%) | 0.33 (0%) | 2.11 (-9%) | 1.74 (-10%)
both | 53.31 (+28%) | 0.79 (+1%) | 0.08 (-75%) | 2.27 (-3%) | 1.49 (-23%)
The pack column specifies which scenario (i.e., what was left
uncompressed). The size column is the size of the object-dir (in
megabytes). The other columns are times to run each command in
wall-clock seconds. Percentages are comparisons to the baseline "none"
case (i.e., the status quo).
So you can see that it's a big win for warm-cache pure-commit
traversals. As a sanity check, we can see that the tree-only case is not
helped at all there (because we do not look at trees at all). The
cold-cache case is helped, too, but that benefit goes away (and even
hurts slightly, but that is probably within the noise) when we also
leave the trees uncompressed.
The full-objects traversal doesn't fare quite as well, though there's
still some improvement. I think it argues for leaving both uncompressed,
as the warm case really benefits when both are uncompressed. You lose
the cold-cache benefits in the revs-only case, though.
Here are the numbers for linux-2.6.git:
Pack | Size | Cold Revs | Warm Revs | Cold Objects | Warm Objects
-------+---------------+-------------+-------------+--------------+--------------
none | 864.61 | 8.66 | 4.07 | 42.76 | 36.32
commit | 970.46 (+12%) | 8.87 (+2%) | 1.02 (-74%) | 42.94 (0%) | 33.43 (-7%)
tree | 895.37 (+3%) | 9.08 (+4%) | 4.07 (0%) | 36.01 (-15%) | 29.62 (-18%)
both | 1001.25 (+15%) | 8.90 (+2%) | 1.03 (-74%) | 35.57 (-16%) | 26.25 (-27%)
Similar warm-cache numbers, but the cold cache for the revs-only case is
hurt a little bit more.
And here's WebKit.git (sizes are in gigabytes this time):
Pack | Size | Cold Revs | Warm Revs | Cold Objects | Warm Objects
-------+---------------+-------------+-------------+--------------+--------------
none | 3.46 | 1.61 | 1.38 | 20.46 | 18.72
commit | 3.54 (+2%) | 1.42 (-11%) | 0.34 (-75%) | 20.42 (0%) | 17.57 (-6%)
tree | 3.59 (+3%) | 1.61 (0%) | 1.39 (0%) | 16.01 (-21%) | 14.00 (-25%)
both | 3.67 (+6%) | 1.45 (-10%) | 0.34 (-75%) | 15.94 (-22%) | 12.91 (-31%)
Pretty similar again (slightly better on the full object traversal).
And finally, for comparison, here are the numbers from a (much slower)
machine that has spinning disks (albeit in a mirrored raid, which should
improve read times) on git.git:
Pack | Size | Cold Revs | Warm Revs | Cold Objects | Warm Objects
-------+---------------+-------------+-------------+--------------+--------------
none | 41.35 | 1.85 | 0.64 | 5.58 | 3.91
commit | 49.23 (+19%) | 1.94 (+4%) | 0.14 (-77%) | 5.51 (-1%) | 3.40 (-12%)
tree | 45.27 (+9%) | 1.78 (-3%) | 0.64 (0%) | 5.13 (-8%) | 3.53 (-9%)
both | 53.16 (+28%) | 1.83 (-1%) | 0.14 (-77%) | 4.96 (-11%) | 3.32 (-14%)
Surprisingly not all that different than the SSD times. Which may mean I
screwed something up. I'm happy to make my test harness available if
anybody else feels like timing on their repos or machines. But it does
point to potentially leaving commits uncompressed, and possibly trees.
I wonder if we could do even better, though. For a traversal, we only
need to look at the commit header. We could potentially do a progressive
inflate and stop before getting to the commit message (which is the bulk
of the data, and the part that is most likely to benefit from
compression).
-Peff
^ permalink raw reply
* Re: [PATCH v3 16/19] pathspec.c: move reusable code from builtin/add.c
From: Adam Spiers @ 2012-12-29 0:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
In-Reply-To: <CAOkDyE8vSyT=eJ4FxRwYgz7Jzqu1+0LSzxtq9iSSzJEgTD1M0g@mail.gmail.com>
On Fri, Dec 28, 2012 at 8:45 PM, Adam Spiers <git@adamspiers.org> wrote:
> On Fri, Dec 28, 2012 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Adam Spiers <git@adamspiers.org> writes:
>>
>>> diff --git a/pathspec.h b/pathspec.h
>>> new file mode 100644
>>> index 0000000..8bb670b
>>> --- /dev/null
>>> +++ b/pathspec.h
>>> @@ -0,0 +1,5 @@
>>> +extern char *find_used_pathspec(const char **pathspec);
>>> +extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
>>> +extern const char *treat_gitlink(const char *path);
>>> +extern void treat_gitlinks(const char **pathspec);
>>> +extern const char **validate_pathspec(const char **argv, const char *prefix);
>>
>> Protect this against multiple inclusion with "#ifndef PATHSPEC_H".
>
> Yep good idea, how should I submit this? It will cause a trivially
> resolvable conflict with the next patch in the series (17/19):
>
> pathspec.c: extract new validate_path() for reuse
I was wrong about that - it didn't cause a conflict, although it does
marginally change the context at the end of the pathspec.h hunk in the
above patch.
> but I'd prefer not to re-roll 16--19 when just 16 and 17 are sufficient.
Based on your other feedback, all of 16--19 require changes, and as
things stand, conveniently nothing earlier in the series does, so I'll
re-roll those four once the outstanding issues are all resolved.
^ permalink raw reply
* Re: Hold your fire, please
From: Junio C Hamano @ 2012-12-28 23:52 UTC (permalink / raw)
To: git
In-Reply-To: <7vd2y1rix1.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Primarily in order to force me concentrate on the releng for the
> upcoming release, and also to encourage contributors to focus on
> finding and fixing any last minute regressions (rather than
> distracting others by showing publicly scratching their itches), I
> won't be queuing any patch that is not a regression fix, at least
> for the next few days. I may not even review them.
>
> Thanks.
>
> And have a nice holiday if you are in areas where it is a holiday
> season ;-)
Just as a friendly reminder, I am reposting this to remind people.
^ permalink raw reply
* [PATCH] Use longer alias names in subdirectory tests
From: Aaron Schrab @ 2012-12-28 23:03 UTC (permalink / raw)
To: git, gitster
When testing aliases in t/t1020-subdirectory.sh use longer names so that
they're less likely to conflict with a git-* command somewhere in the
$PATH.
I have a git-ss command in my path which prevents the 'ss' alias from
being used. This command will always fail for git.git, causing the test
to fail. Even if the command succeeded, that would be a false success
for the test since the alias wasn't actually used. A longer, more
descriptive name will make it much less likely that somebody has a
command in their $PATH which will shadow the alias created for the test.
While here, use a longer name for the 'test' alias as well since that is
also short and meaningful enough to make it not unlikely that somebody
would have a command in their $PATH which will shadow that as well.
Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
t/t1020-subdirectory.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index e23ac0e..1e2945e 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -111,19 +111,19 @@ test_expect_success 'read-tree' '
test_expect_success 'alias expansion' '
(
- git config alias.ss status &&
+ git config alias.test-status-alias status &&
cd dir &&
git status &&
- git ss
+ git test-status-alias
)
'
test_expect_success NOT_MINGW '!alias expansion' '
pwd >expect &&
(
- git config alias.test !pwd &&
+ git config alias.test-alias-directory !pwd &&
cd dir &&
- git test >../actual
+ git test-alias-directory >../actual
) &&
test_cmp expect actual
'
@@ -131,9 +131,9 @@ test_expect_success NOT_MINGW '!alias expansion' '
test_expect_success 'GIT_PREFIX for !alias' '
printf "dir/" >expect &&
(
- git config alias.test "!sh -c \"printf \$GIT_PREFIX\"" &&
+ git config alias.test-alias-directory "!sh -c \"printf \$GIT_PREFIX\"" &&
cd dir &&
- git test >../actual
+ git test-alias-directory >../actual
) &&
test_cmp expect actual
'
--
1.8.1.rc3.16.g47d6ba6
^ permalink raw reply related
* Re: [PATCH] Remove the suggestion to use parsecvs, which is currently broken.
From: Heiko Voigt @ 2012-12-28 23:01 UTC (permalink / raw)
To: Eric S. Raymond; +Cc: git
In-Reply-To: <20121228162025.8565E4413A@snark.thyrsus.com>
Hi,
On Fri, Dec 28, 2012 at 11:20:25AM -0500, Eric S. Raymond wrote:
> The parsecvs code has been neglected for a long time, and the only
> public version does not even build correctly. I have been handed
> control of the project and intend to fix this, but until I do it
> cannot be recommended.
You mean: It does not build correctly with a current version of git?
Since it links with the git source code it probably needs a version of
gits source code around the time of the last commits.
Maybe you could add that information to the parsecvs compile
instructions? I think just because it takes some effort to compile does
not justify to remove this useful pointer here. When I was converting a
legacy cvs repository this pointer would have helped me a lot.
It is the tool we were/are actively using to convert old repositories at
$dayjob.
Cheers Heiko
^ permalink raw reply
* [PATCH 3/4] push: Add support for pre-push hooks
From: Aaron Schrab @ 2012-12-28 22:57 UTC (permalink / raw)
To: git
In-Reply-To: <1356735452-21667-1-git-send-email-aaron@schrab.com>
Add support for a pre-push hook which can be used to determine if the
set of refs to be pushed is suitable for the target repository. The
hook should be supplied with:
1. name of the remote being used, or the URL if not using a named
remote
2. the URL to which we're pushing
3. descriptions of what references are to be pushed
Each reference to be pushed should be described in a separate parameter
to the hook script in the form:
<local ref>:<local sha1>:<remote ref>:<remote sha1>
This will allow the script to determine if the push is acceptable based
on the target repository and branch(es), the commits which are to be
pushed, and even the source branches in some cases.
Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
Documentation/githooks.txt | 28 +++++++++
builtin/push.c | 1 +
t/t5571-pre-push-hook.sh | 145 ++++++++++++++++++++++++++++++++++++++++++++
transport.c | 25 ++++++++
transport.h | 1 +
5 files changed, 200 insertions(+)
create mode 100755 t/t5571-pre-push-hook.sh
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b9003fe..e9539bb 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -176,6 +176,34 @@ save and restore any form of metadata associated with the working tree
(eg: permissions/ownership, ACLS, etc). See contrib/hooks/setgitperms.perl
for an example of how to do this.
+pre-push
+~~~~~~~~
+
+This hook is called by 'git push' and can be used to prevent a push from
+taking place. The hook is called with a variable number of parameters.
+
+The first parameters provide the name and location of the destination
+remote, if a named remote is not being used both values will be the same.
+
+Remaining parameters provide information about the commits which are to be
+pushed and the ref names being used. These arguments take the form:
+
+ <local ref>:<local sha1>:<remote ref>:<remote sha1>
+
+For instance, if the command +git push origin master:foreign+ were run the
+hook would be called with a third arugment similar to:
+
+ refs/heads/master:67890:refs/heads/foreign:12345
+
+although the full, 40-character SHA1s would be supplied. If the foreign ref
+does not yet exist the `<remote SHA1>` will be 40 `0`. If a ref is to be
+deleted, the `<local ref>` will be supplied as `(delete)` and the `<local
+SHA1>` will be 40 `0`. If the local commit was specified by something other
+than a name which could be expanded (such as `HEAD~`, or a SHA1) it will be
+supplied as it was originally given.
+
+If this hook exits with a non-zero status, 'git push' will abort.
+
[[pre-receive]]
pre-receive
~~~~~~~~~~~
diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..c33fb9b 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -399,6 +399,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
OPT_BIT(0, "prune", &flags, N_("prune locally removed refs"),
TRANSPORT_PUSH_PRUNE),
+ OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
OPT_END()
};
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
new file mode 100755
index 0000000..5444c9b
--- /dev/null
+++ b/t/t5571-pre-push-hook.sh
@@ -0,0 +1,145 @@
+#!/bin/sh
+
+test_description='check pre-push hooks'
+. ./test-lib.sh
+
+# Setup hook that always succeeds
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+HOOK="$HOOKDIR/pre-push"
+mkdir -p "$HOOKDIR"
+cat >"$HOOK" <<EOF
+#!/bin/sh
+exit 0
+EOF
+chmod +x "$HOOK"
+
+test_expect_success 'setup' '
+ git config push.default upstream &&
+ git init --bare repo1 &&
+ git remote add parent1 repo1 &&
+ test_commit one &&
+ git push parent1 HEAD:foreign
+'
+cat >"$HOOK" <<EOF
+#!/bin/sh
+exit 1
+EOF
+
+COMMIT1="$(git rev-parse HEAD)"
+export COMMIT1
+
+test_expect_success 'push with failing hook' '
+ test_commit two &&
+ test_must_fail git push parent1 HEAD
+'
+
+test_expect_success '--no-verify bypasses hook' '
+ git push --no-verify parent1 HEAD
+'
+
+COMMIT2="$(git rev-parse HEAD)"
+export COMMIT2
+
+cat >"$HOOK" <<'EOF'
+#!/bin/sh -ex
+test "$#" = 3
+test "$1" = parent1
+test "$2" = repo1
+test "$3" = "refs/heads/master:$COMMIT2:refs/heads/foreign:$COMMIT1"
+EOF
+
+test_expect_success 'push with hook' '
+ git push parent1 master:foreign
+'
+
+test_expect_success 'add a branch' '
+ git checkout -b other &&
+ test_commit three
+'
+
+COMMIT3="$(git rev-parse HEAD)"
+export COMMIT3
+
+cat >"$HOOK" <<'EOF'
+#!/bin/sh -ex
+test "$#" = 4
+test "$1" = parent1
+test "$2" = repo1
+test "$3" = "refs/heads/other:$COMMIT3:refs/heads/foreign:$COMMIT2"
+test "$4" = "refs/heads/master:$COMMIT2:refs/heads/new:$_z40"
+EOF
+
+test_expect_success 'push multiple refs' '
+ git push parent1 other:foreign master:new
+'
+
+test_expect_success 'add a branch with an upstream' '
+ git checkout -t -b tracking parent1/foreign &&
+ test_commit four
+'
+COMMIT4="$(git rev-parse HEAD)"
+export COMMIT4
+
+cat >"$HOOK" <<'EOF'
+#!/bin/sh -ex
+test "$#" = 3
+test "$1" = parent1
+test "$2" = repo1
+test "$3" = "refs/heads/tracking:$COMMIT4:refs/heads/foreign:$COMMIT3"
+EOF
+
+test_expect_success 'push to upstream branch' '
+ git push &&
+ git checkout other
+'
+
+cat >"$HOOK" <<'EOF'
+#!/bin/sh -ex
+test "$#" = 3
+test "$1" = parent1
+test "$2" = repo1
+test "$3" = "(delete):$_z40:refs/heads/new:$COMMIT2"
+EOF
+
+test_expect_success 'push deletion' '
+ git push parent1 :new
+'
+
+cat >"$HOOK" <<'EOF'
+#!/bin/sh -ex
+test "$#" = 3
+test "$1" = repo2
+test "$2" = repo2
+test "$3" = "refs/heads/other:$COMMIT3:refs/heads/new:$_z40"
+EOF
+
+test_expect_success 'push to URL' '
+ git init --bare repo2 &&
+ git push repo2 other:new
+'
+
+ABBR=$(expr substr $COMMIT3 1 8)
+export ABBR
+
+cat >"$HOOK" <<'EOF'
+#!/bin/sh -ex
+test "$#" = 4
+test "$3" = "HEAD~:$COMMIT2:refs/heads/commitish:$_z40"
+test "$4" = "$ABBR:$COMMIT3:refs/heads/sha:$_z40"
+EOF
+
+test_expect_success 'push commit' '
+ git push parent1 HEAD~:refs/heads/commitish $ABBR:refs/heads/sha
+'
+
+cat >"$HOOK" <<'EOF'
+#!/bin/sh -ex
+test "$#" = 3
+test "$3" = "refs/tags/one:$COMMIT1:refs/tags/tagpush:$_z40"
+EOF
+
+test_expect_success 'push tag' '
+ git push parent1 one:tagpush
+'
+
+test_done
diff --git a/transport.c b/transport.c
index 9932f40..b0c9a15 100644
--- a/transport.c
+++ b/transport.c
@@ -1052,6 +1052,7 @@ int transport_push(struct transport *transport,
int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
int push_ret, ret, err;
+ char *hook;
if (flags & TRANSPORT_PUSH_ALL)
match_flags |= MATCH_REFS_ALL;
@@ -1069,6 +1070,30 @@ int transport_push(struct transport *transport,
flags & TRANSPORT_PUSH_MIRROR,
flags & TRANSPORT_PUSH_FORCE);
+ if (!(flags & TRANSPORT_PUSH_NO_HOOK) && (hook = find_hook("pre-push"))) {
+ struct ref *r;
+ struct argv_array argv = ARGV_ARRAY_INIT;
+
+ argv_array_push(&argv, hook);
+ argv_array_push(&argv, transport->remote->name);
+ argv_array_push(&argv, transport->url);
+
+ for (r = remote_refs; r; r = r->next) {
+ if (!r->peer_ref) continue;
+ if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;
+ if (r->status == REF_STATUS_UPTODATE) continue;
+
+ argv_array_pushf(&argv, "%s:%s:%s:%s",
+ r->peer_ref->name, sha1_to_hex(r->new_sha1),
+ r->name, sha1_to_hex(r->old_sha1));
+ }
+
+ ret = run_hook_argv(NULL, argv);
+ argv_array_clear(&argv);
+ if (ret)
+ return -1;
+ }
+
if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
struct ref *ref = remote_refs;
for (; ref; ref = ref->next)
diff --git a/transport.h b/transport.h
index 4a61c0c..3bc9863 100644
--- a/transport.h
+++ b/transport.h
@@ -104,6 +104,7 @@ struct transport {
#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
#define TRANSPORT_PUSH_PRUNE 128
#define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256
+#define TRANSPORT_PUSH_NO_HOOK 512
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
--
1.7.10.4
^ permalink raw reply related
* [PATCH 4/4] Add sample pre-push hook script
From: Aaron Schrab @ 2012-12-28 22:57 UTC (permalink / raw)
To: git
In-Reply-To: <1356735452-21667-1-git-send-email-aaron@schrab.com>
Create a sample of a script for a pre-push hook. The main purpose is to
illustrate how a script may parse the parameters which are supplied to
such a hook. The script may also be useful to some people as-is for
avoiding to push commits which are marked as a work in progress.
Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
templates/hooks--pre-push.sample | 63 ++++++++++++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)
create mode 100644 templates/hooks--pre-push.sample
diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
new file mode 100644
index 0000000..1d3b4a3
--- /dev/null
+++ b/templates/hooks--pre-push.sample
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+# An example hook script to verify what is about to be pushed.
+# Called by "git push" after it has checked the remote status, but before
+# anything has been pushed. If this script exits with a non-zero status
+# nothing will be pushed.
+#
+# This hook is called with the following parameters:
+#
+# $1 -- Name of the remote to which the push is being done
+# $2 -- URL to which the push is being done
+#
+# If pushing without using a named remote those arguments will be equal.
+#
+# Further arguments provide information about the commits which are being
+# pushed in the form:
+#
+# <local ref>:<local sha1>:<remote ref>:<remote sha1>
+#
+# This sample shows how to prevent push of commits where the log
+# message starts with "WIP" (work in progress).
+
+remote="$1"
+url="$2"
+shift 2
+
+z40=0000000000000000000000000000000000000000
+
+old_ifs="$IFS"
+for to_push in "$@"
+do
+ # Split the value into its parts
+ IFS=:
+ set -- $to_push
+ IFS="$old_ifs"
+
+ local_ref="$1"
+ local_sha="$2"
+ remote_ref="$3"
+ remote_sha="$4"
+
+ if [ "$local_sha" = $z40 ]
+ then
+ range=''
+ # Handle deletes
+ else
+ if [ "$remote_sha" = $z40 ]
+ then
+ range="$local_sha"
+ else
+ range="$remote_sha..$local_sha"
+ fi
+
+ commit=`git rev-list -n 1 --grep '^WIP' "$range"`
+ if [ -n "$commit" ]
+ then
+ echo "Found WIP commit in $local_ref, not pushing"
+ exit 1
+ fi
+ fi
+done
+
+exit 0
--
1.7.10.4
^ permalink raw reply related
* [PATCH 2/4] hooks: support variable number of parameters
From: Aaron Schrab @ 2012-12-28 22:57 UTC (permalink / raw)
To: git
In-Reply-To: <1356735452-21667-1-git-send-email-aaron@schrab.com>
Define the run_hook_argv() function to allow hooks to be created where
the number of parameters to be passed is variable. The existing
run_hook() function uses stdarg to allow it to receive a variable number
of arguments, but the number of arguments that a given caller is passing
is fixed at compile time. This function will allow the caller of a hook
to determine the number of arguments to pass when preparing to call the
hook.
The first use of this function will be for a pre-push hook which will
add an argument for every reference which is to be pushed.
Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
run-command.c | 20 +++++++++++++-------
run-command.h | 2 ++
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/run-command.c b/run-command.c
index 49c8fa0..e07202b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,7 +2,6 @@
#include "run-command.h"
#include "exec_cmd.h"
#include "sigchain.h"
-#include "argv-array.h"
#ifndef SHELL_PATH
# define SHELL_PATH "/bin/sh"
@@ -746,10 +745,8 @@ char *find_hook(const char *name)
int run_hook(const char *index_file, const char *name, ...)
{
- struct child_process hook;
struct argv_array argv = ARGV_ARRAY_INIT;
- const char *p, *env[2];
- char index[PATH_MAX];
+ const char *p;
va_list args;
int ret;
@@ -764,6 +761,17 @@ int run_hook(const char *index_file, const char *name, ...)
argv_array_push(&argv, p);
va_end(args);
+ ret = run_hook_argv(index_file, argv);
+ argv_array_clear(&argv);
+ return ret;
+}
+
+int run_hook_argv(const char *index_file, struct argv_array argv)
+{
+ struct child_process hook;
+ char index[PATH_MAX];
+ const char *env[2];
+
memset(&hook, 0, sizeof(hook));
hook.argv = argv.argv;
hook.no_stdin = 1;
@@ -775,7 +783,5 @@ int run_hook(const char *index_file, const char *name, ...)
hook.env = env;
}
- ret = run_command(&hook);
- argv_array_clear(&argv);
- return ret;
+ return run_command(&hook);
}
diff --git a/run-command.h b/run-command.h
index 221ce33..12faa5b 100644
--- a/run-command.h
+++ b/run-command.h
@@ -1,6 +1,7 @@
#ifndef RUN_COMMAND_H
#define RUN_COMMAND_H
+#include "argv-array.h"
#ifndef NO_PTHREADS
#include <pthread.h>
#endif
@@ -47,6 +48,7 @@ int run_command(struct child_process *);
extern char *find_hook(const char *name);
extern int run_hook(const char *index_file, const char *name, ...);
+extern int run_hook_argv(const char *index_file, struct argv_array);
#define RUN_COMMAND_NO_STDIN 1
#define RUN_GIT_CMD 2 /*If this is to be git sub-command */
--
1.7.10.4
^ permalink raw reply related
* [PATCH 0/4] pre-push hook support
From: Aaron Schrab @ 2012-12-28 22:57 UTC (permalink / raw)
To: git
There have been at least a couple of submissions to add support for a
pre-push hook, which were rejected at least partially because they didn't
provide enough information to a hook script for it to determine what was
to be pushed any better than a separate wrapper around the 'git push'
command would be able to do. In this series I attempt to address that
problem.
The first two patches in this series do a little bit of refactoring in
order to make it easier to call hooks with a variable number of arguments.
The third patch actually adds support for calling a pre-push hook. If it
exists, it will be called with the name and URL of the destination remote
(if a named remote isn't being used, the URL will be supplied for both)
followed by another argument for each ref being pushed; these arguments
take the form:
<local ref>:<local sha1>:<remote ref>:<remote sha1>
This should provide enough information for a script to easily determine
the set of commits that is being pushed, and thus make a decision if that
should be allowed.
The final patch adds a sample pre-push hook script which will deny
attempts to push commits that are marked as a work in progress.
Aaron Schrab (4):
hooks: Add function to check if a hook exists
hooks: support variable number of parameters
push: Add support for pre-push hooks
Add sample pre-push hook script
Documentation/githooks.txt | 28 ++++++++
builtin/push.c | 1 +
run-command.c | 35 ++++++---
run-command.h | 3 +
t/t5571-pre-push-hook.sh | 145 ++++++++++++++++++++++++++++++++++++++
templates/hooks--pre-push.sample | 63 +++++++++++++++++
transport.c | 25 +++++++
transport.h | 1 +
8 files changed, 292 insertions(+), 9 deletions(-)
create mode 100755 t/t5571-pre-push-hook.sh
create mode 100644 templates/hooks--pre-push.sample
--
1.7.10.4
^ permalink raw reply
* [PATCH 1/4] hooks: Add function to check if a hook exists
From: Aaron Schrab @ 2012-12-28 22:57 UTC (permalink / raw)
To: git
In-Reply-To: <1356735452-21667-1-git-send-email-aaron@schrab.com>
Create find_hook() function to determine if a given hook exists and is
executable. If it is the path to the script will be returned, otherwise
NULL is returned.
This is in support for an upcoming run_hook_argv() function which will
expect the full path to the hook script as the first element in the
argv_array. This also makes it simple for places that can use a hook to
check if a hook exists before doing, possibly lengthy, setup work which
would be pointless if no such hook is present.
The returned value is left as a static value from get_pathname() rather
than a duplicate because it is anticipated that the return value will
either be used as a boolean, or immediately added to an argv_array list
which would result in it being duplicated at that point.
Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
run-command.c | 15 +++++++++++++--
run-command.h | 1 +
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/run-command.c b/run-command.c
index 3b982e4..49c8fa0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -735,6 +735,15 @@ int finish_async(struct async *async)
#endif
}
+char *find_hook(const char *name)
+{
+ char *path = git_path("hooks/%s", name);
+ if (access(path, X_OK) < 0)
+ path = NULL;
+
+ return path;
+}
+
int run_hook(const char *index_file, const char *name, ...)
{
struct child_process hook;
@@ -744,11 +753,13 @@ int run_hook(const char *index_file, const char *name, ...)
va_list args;
int ret;
- if (access(git_path("hooks/%s", name), X_OK) < 0)
+ p = find_hook(name);
+ if (!p)
return 0;
+ argv_array_push(&argv, p);
+
va_start(args, name);
- argv_array_push(&argv, git_path("hooks/%s", name));
while ((p = va_arg(args, const char *)))
argv_array_push(&argv, p);
va_end(args);
diff --git a/run-command.h b/run-command.h
index 850c638..221ce33 100644
--- a/run-command.h
+++ b/run-command.h
@@ -45,6 +45,7 @@ int start_command(struct child_process *);
int finish_command(struct child_process *);
int run_command(struct child_process *);
+extern char *find_hook(const char *name);
extern int run_hook(const char *index_file, const char *name, ...);
#define RUN_COMMAND_NO_STDIN 1
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH v3 00/19] new git check-ignore sub-command
From: Junio C Hamano @ 2012-12-28 21:31 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Adam Spiers, git list
In-Reply-To: <CALWbr2wEVzjJ_Y+W9BmakvXCwdFR3OjVH+15tPaDeXsrwaO86w@mail.gmail.com>
Antoine Pelisse <apelisse@gmail.com> writes:
> I think they will interact, but I need to have a deeper look to Adam's series.
> If it does, do you want me to base my work on the top of his branch ?
Not necessarily. If it becomes absolutely necessary to introduce
patch dependencies, I would rather see an addition of new command
rebased on a fix.
I just wanted to make sure that parties touching the same area of
the codebase (and me who will be merging their efforts) are aware of
what is going on.
^ permalink raw reply
* Re: [PATCH v3 00/19] new git check-ignore sub-command
From: Junio C Hamano @ 2012-12-28 21:23 UTC (permalink / raw)
To: Adam Spiers; +Cc: git list
In-Reply-To: <1356575558-2674-1-git-send-email-git@adamspiers.org>
After skimming the series twice quickly, I found that the early part
of refactorings are excellently done. Making existing private
functions into public needs a lot more careful thought on namings, I
think, though.
The end result looks promising. Thanks for a pleasant read.
^ permalink raw reply
* Re: [PATCH v3 19/19] Add git-check-ignore sub-command
From: Junio C Hamano @ 2012-12-28 21:21 UTC (permalink / raw)
To: Adam Spiers; +Cc: git list
In-Reply-To: <1356575558-2674-20-git-send-email-git@adamspiers.org>
Adam Spiers <git@adamspiers.org> writes:
> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
> new file mode 100644
> index 0000000..c825736
> --- /dev/null
> +++ b/builtin/check-ignore.c
> @@ -0,0 +1,170 @@
> +#include "builtin.h"
> +#include "cache.h"
> +#include "dir.h"
> +#include "quote.h"
> +#include "pathspec.h"
> +#include "parse-options.h"
> +
> +static int quiet, verbose, stdin_paths;
> +static const char * const check_ignore_usage[] = {
> +"git check-ignore [options] pathname...",
> +"git check-ignore [options] --stdin < <list-of-paths>",
> +NULL
> +};
> +
> +static int null_term_line;
> +
> +static const struct option check_ignore_options[] = {
> + OPT__QUIET(&quiet, N_("suppress progress reporting")),
> + OPT__VERBOSE(&verbose, N_("be verbose")),
> + OPT_GROUP(""),
> + OPT_BOOLEAN(0, "stdin", &stdin_paths,
> + N_("read file names from stdin")),
> + OPT_BOOLEAN('z', NULL, &null_term_line,
> + N_("input paths are terminated by a null character")),
> + OPT_END()
> +};
> +
> +static void output_exclude(const char *path, struct exclude *exclude)
> +{
> + char *bang = exclude->flags & EXC_FLAG_NEGATIVE ? "!" : "";
> + char *dir = (exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
That's inconsistent. Either s/bang/negated/ or s/dir/slash/ (but
obviously not both).
> +static int check_ignore(const char *prefix, const char **pathspec)
> +{
> + struct dir_struct dir;
> +...
> + if (pathspec) {
> +...
> + } else {
> + printf("no pathspec\n");
> + }
Is this an error message, an informative warning, or what? The
command is presumably for script consumption downstream of a pipe.
Does it make sense to emit this message to their standard input?
Does it even have to be output? Especially what should happen when
the user says "--quiet"?
Perhaps
if (!quiet)
fprintf(stderr, "no pathspec given.\n");
or something?
> +int cmd_check_ignore(int argc, const char **argv, const char *prefix)
> +{
> + int num_ignored = 0;
I do not think you need to initialize this one.
> + if (stdin_paths) {
> + num_ignored = check_ignore_stdin_paths(prefix);
> + } else {
> + num_ignored = check_ignore(prefix, argv);
> + maybe_flush_or_die(stdout, "ignore to stdout");
> + }
> +
> + return num_ignored > 0 ? 0 : 1;
Given that num_ignored will always be >=0, that is a funny way to
say
return !num_ignored;
or if you plan to report a non-fatal error as negative return from
the two check_ignore* functions, perhaps:
return num_ignored <= 0;
> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
> new file mode 100755
> index 0000000..3937ca4
> --- /dev/null
> +++ b/t/t0008-ignores.sh
> @@ -0,0 +1,595 @@
> +#!/bin/sh
> +
> +test_description=check-ignore
> +
> +. ./test-lib.sh
> +
> +init_vars () {
> + global_excludes="$(pwd)/global-excludes"
> +}
> +
> +enable_global_excludes () {
> + init_vars
> + git config core.excludesfile "$global_excludes"
> +}
> +
> +expect_in () {
> + dest="$HOME/expected-$1" text="$2"
> + if test -z "$text"
> + then
> + >"$dest" # avoid newline
> + else
> + echo "$text" >"$dest"
> + fi
> +}
> +
> +expect () {
> + expect_in stdout "$1"
> +}
> +
> +expect_from_stdin () {
> + cat >"$HOME/expected-stdout"
> +}
> +
> +test_stderr () {
> + expected="$1"
> + expect_in stderr "$1" &&
> + test_cmp "$HOME/expected-stderr" "$HOME/stderr"
> +}
> +
> +stderr_contains () {
> + regexp="$1"
> + if grep -q "$regexp" "$HOME/stderr"
Please do not use "grep -q"; the output from commands in test
harness is normally hidden already. This only makes script more
cluttered and robs debuggers of potentially useful output when the
test script is run with "-v".
> +test_check_ignore () {
> + args="$1" expect_code="${2:-0}" global_args="$3"
> +
> + init_vars &&
> + rm -f "$HOME/stdout" "$HOME/stderr" "$HOME/cmd" &&
> + echo git $global_args check-ignore $quiet_opt $verbose_opt $args \
> + >"$HOME/cmd" &&
Does a debugger needs this "cmd" file when we already have "-v" option?
> +test_expect_success_multi () {
> + prereq=
> + if test $# -eq 4
> + then
> + prereq=$1
> + shift
> + fi
> + testname="$1" expect_verbose="$2" code="$3"
> +
> + expect=$( echo "$expect_verbose" | sed -e 's/.* //' )
> +
> + test_expect_success $prereq "$testname" "
> + expect '$expect' &&
> + $code
> + "
This is brittle when $expect may have single quotes, isn't it?
Perhaps
test_expect_success $prereq "$testname" '
expect "$expect" && eval "$code"
'
may fix it, but in general I hate to see each test script trying to
invent their own mini-harness like this (and getting them wrong).
^ permalink raw reply
* Re: [PATCH v3 16/19] pathspec.c: move reusable code from builtin/add.c
From: Adam Spiers @ 2012-12-28 21:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
In-Reply-To: <7vy5ghhpi0.fsf@alter.siamese.dyndns.org>
On Fri, Dec 28, 2012 at 8:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
>> diff --git a/pathspec.c b/pathspec.c
>> new file mode 100644
>> index 0000000..8aea0d2
>> --- /dev/null
>> +++ b/pathspec.c
>> @@ -0,0 +1,99 @@
>> +#include "cache.h"
>> +#include "dir.h"
>> +#include "pathspec.h"
>> +
>> +void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
>> +{
>
> It did not matter while it was an implementation detail of "git
> add", but as a public function, something needs to clarify that this
> "fill"s matches *against the index*, not against a tree or the files
> in the current directory. The same comment applies to all the
> internal functions this patch exposes to the outside world.
Prior to submitting the v3 series, I attempted to understand
fill_pathspec_matches() and find_used_pathspec() well enough to
document them all, but I failed. Perhaps some kind soul could explain
what is the exact purpose of these functions, and in particular the
role of char *seen in both?
^ permalink raw reply
* Re: [PATCH v3 17/19] pathspec.c: extract new validate_path() for reuse
From: Adam Spiers @ 2012-12-28 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
In-Reply-To: <7v38ypj490.fsf@alter.siamese.dyndns.org>
On Fri, Dec 28, 2012 at 8:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
>> This will be reused by a new git check-ignore command.
>>
>> Signed-off-by: Adam Spiers <git@adamspiers.org>
>> ---
>> pathspec.c | 20 ++++++++++++++------
>> pathspec.h | 1 +
>> 2 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/pathspec.c b/pathspec.c
>> index 8aea0d2..6724121 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -77,9 +77,20 @@ void treat_gitlinks(const char **pathspec)
>> }
>>
>> /*
>> + * Dies if the given path refers to a file inside a symlinked
>> + * directory.
>> + */
>> +void validate_path(const char *path, const char *prefix)
>
> The name needs to be a lot more specific.
>
> There may be 47 different kinds of "validations" various callers may
> want to do on a path, but this function only caters to one kind of
> callers that want to make sure that the path refers to something
> that we would directly add to our index.
>
>> +{
>> + if (has_symlink_leading_path(path, strlen(path))) {
>> + int len = prefix ? strlen(prefix) : 0;
>> + die(_("'%s' is beyond a symbolic link"), path + len);
>> + }
>> +}
Good point. Which do you prefer of these suggested names?
- die_if_path_beyond_symlink()
- validate_path_not_beyond_symlink()
- die_if_symlink_leading_path()
- validate_no_symlink_leading_path()
- validate_path_addable_to_index()
Or something else?
^ permalink raw reply
* Re: [PATCH v3 16/19] pathspec.c: move reusable code from builtin/add.c
From: Junio C Hamano @ 2012-12-28 20:48 UTC (permalink / raw)
To: Adam Spiers; +Cc: git list
In-Reply-To: <1356575558-2674-17-git-send-email-git@adamspiers.org>
Adam Spiers <git@adamspiers.org> writes:
> diff --git a/pathspec.c b/pathspec.c
> new file mode 100644
> index 0000000..8aea0d2
> --- /dev/null
> +++ b/pathspec.c
> @@ -0,0 +1,99 @@
> +#include "cache.h"
> +#include "dir.h"
> +#include "pathspec.h"
> +
> +void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
> +{
It did not matter while it was an implementation detail of "git
add", but as a public function, something needs to clarify that this
"fill"s matches *against the index*, not against a tree or the files
in the current directory. The same comment applies to all the
internal functions this patch exposes to the outside world.
^ permalink raw reply
* Re: [PATCH v3 16/19] pathspec.c: move reusable code from builtin/add.c
From: Adam Spiers @ 2012-12-28 20:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
In-Reply-To: <7v8v8hj4t0.fsf@alter.siamese.dyndns.org>
On Fri, Dec 28, 2012 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Adam Spiers <git@adamspiers.org> writes:
>
>> diff --git a/pathspec.h b/pathspec.h
>> new file mode 100644
>> index 0000000..8bb670b
>> --- /dev/null
>> +++ b/pathspec.h
>> @@ -0,0 +1,5 @@
>> +extern char *find_used_pathspec(const char **pathspec);
>> +extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
>> +extern const char *treat_gitlink(const char *path);
>> +extern void treat_gitlinks(const char **pathspec);
>> +extern const char **validate_pathspec(const char **argv, const char *prefix);
>
> Protect this against multiple inclusion with "#ifndef PATHSPEC_H".
Yep good idea, how should I submit this? It will cause a trivially
resolvable conflict with the next patch in the series (17/19):
pathspec.c: extract new validate_path() for reuse
but I'd prefer not to re-roll 16--19 when just 16 and 17 are sufficient.
^ 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